Fix LLM review findings: infinite recursion, nginx stop, fire-and-forget, async deadlock, port mismatch, dispose

Rakeshwar Reddy Kambaiahgari · Rakeshwar Reddy Kambaiahgari · commit ddfe093cc2c2 · 2026-04-10T09:07:40.000-07:00
safety
diff --git a/src/VirtualClient/VirtualClient.Actions.UnitTests/AspNetBench/BombardierExecutorTests.cs b/src/VirtualClient/VirtualClient.Actions.UnitTests/AspNetBench/BombardierExecutorTests.cs
@@ -6,6 +6,7 @@ namespace VirtualClient.Actions
     using System;
     using System.Collections.Generic;
     using System.Threading;
+    using System.Threading.Tasks;
     using Microsoft.Extensions.DependencyInjection;
     using Moq;
     using NUnit.Framework;
@@ -46,40 +47,40 @@ public void SetupDefaults()
         }
 
         [Test]
-        public void BombardierExecutorGetBombardierVersionParsesVersionWithVPrefix()
+        public async Task BombardierExecutorGetBombardierVersionParsesVersionWithVPrefix()
         {
             this.mockFixture.SetupProcessOutput(".*--version.*", "bombardier version v1.2.5 linux/arm64");
 
             using (TestBombardierExecutor executor = new TestBombardierExecutor(this.mockFixture.Dependencies, this.mockFixture.Parameters))
             {
                 executor.PackageDirectory = this.mockPackage.Path;
-                string version = executor.GetBombardierVersion(EventContext.None, CancellationToken.None);
+                string version = await executor.GetBombardierVersionAsync(EventContext.None, CancellationToken.None);
                 Assert.AreEqual("1.2.5", version);
             }
         }
 
         [Test]
-        public void BombardierExecutorGetBombardierVersionParsesVersionWithoutVPrefix()
+        public async Task BombardierExecutorGetBombardierVersionParsesVersionWithoutVPrefix()
         {
             this.mockFixture.SetupProcessOutput(".*--version.*", "bombardier version 1.2.5");
 
             using (TestBombardierExecutor executor = new TestBombardierExecutor(this.mockFixture.Dependencies, this.mockFixture.Parameters))
             {
                 executor.PackageDirectory = this.mockPackage.Path;
-                string version = executor.GetBombardierVersion(EventContext.None, CancellationToken.None);
+                string version = await executor.GetBombardierVersionAsync(EventContext.None, CancellationToken.None);
                 Assert.AreEqual("1.2.5", version);
             }
         }
 
         [Test]
-        public void BombardierExecutorGetBombardierVersionReturnsNullOnUnparsableOutput()
+        public async Task BombardierExecutorGetBombardierVersionReturnsNullOnUnparsableOutput()
         {
             this.mockFixture.SetupProcessOutput(".*--version.*", "unrecognized output");
 
             using (TestBombardierExecutor executor = new TestBombardierExecutor(this.mockFixture.Dependencies, this.mockFixture.Parameters))
             {
                 executor.PackageDirectory = this.mockPackage.Path;
-                string version = executor.GetBombardierVersion(EventContext.None, CancellationToken.None);
+                string version = await executor.GetBombardierVersionAsync(EventContext.None, CancellationToken.None);
                 Assert.IsNull(version);
             }
         }
diff --git a/src/VirtualClient/VirtualClient.Actions.UnitTests/Nginx/NginxServerExecutorTest.cs b/src/VirtualClient/VirtualClient.Actions.UnitTests/Nginx/NginxServerExecutorTest.cs
@@ -274,7 +274,7 @@ public async Task NginxServerExecutorResetsServerAsExpected(PlatformID platform,
                 }
                 else
                 {
-                    Assert.AreEqual(arguments, "systemctl disable nginx", NginxCommand.Stop.ConvertToCommandArgs());
+                    Assert.AreEqual(arguments, "systemctl stop nginx", NginxCommand.Stop.ConvertToCommandArgs());
                 }
 
                 Assert.AreEqual(command, "sudo");
@@ -328,7 +328,7 @@ public void NginxServerExecutorWillResetServerDuringDispose(PlatformID platform,
                 }
                 else
                 {
-                    Assert.AreEqual(arguments, "systemctl disable nginx", NginxCommand.Stop.ConvertToCommandArgs());
+                    Assert.AreEqual(arguments, "systemctl stop nginx", NginxCommand.Stop.ConvertToCommandArgs());
                 }
 
                 Assert.AreEqual(command, "sudo");
@@ -379,7 +379,7 @@ public async Task NginxServerExecutorRunsAsExpected(PlatformID platform, Archite
                     shellScriptCalls++;
                     Assert.AreEqual(workingDir, packagePath);
                 }
-                else if (new[] { "systemctl restart nginx", "systemctl disable nginx"}.Contains(arguments, StringComparer.OrdinalIgnoreCase))
+                else if (new[] { "systemctl restart nginx", "systemctl stop nginx"}.Contains(arguments, StringComparer.OrdinalIgnoreCase))
                 {
                     nginxServiceCalls++;
                     Assert.IsNull(workingDir);
@@ -445,7 +445,7 @@ public void NginxServerExecutorWillResetServerIfFailure(PlatformID platform, Arc
                     // This will ensure nginx version is empty therefore ArgumentException will be thrown.
                     this.memoryProcess.StandardError = new ConcurrentBuffer(new StringBuilder(string.Empty));
                 }
-                else if (arguments == "systemctl disable nginx" || arguments == "bash reset.sh")
+                else if (arguments == "systemctl stop nginx" || arguments == "bash reset.sh")
                 {
                 }
                 else
diff --git a/src/VirtualClient/VirtualClient.Actions/ASPNET/AspNetOrchardServerExecutor.cs b/src/VirtualClient/VirtualClient.Actions/ASPNET/AspNetOrchardServerExecutor.cs
@@ -144,8 +144,16 @@ protected override void Dispose(bool disposing)
             {
                 if (!this.disposed)
                 {
-                    this.KillServerInstancesAsync(null, CancellationToken.None)
-                        .GetAwaiter().GetResult();
+                    try
+                    {
+                        this.KillServerInstancesAsync(EventContext.None, CancellationToken.None)
+                            .GetAwaiter().GetResult();
+                    }
+                    catch
+                    {
+                        // Best-effort cleanup during dispose.
+                    }
+
                     this.disposed = true;
                 }
             }
diff --git a/src/VirtualClient/VirtualClient.Actions/ASPNET/AspNetServerExecutor.cs b/src/VirtualClient/VirtualClient.Actions/ASPNET/AspNetServerExecutor.cs
@@ -155,8 +155,16 @@ protected override void Dispose(bool disposing)
             {
                 if (!this.disposed)
                 {
-                    this.KillServerInstancesAsync(null, CancellationToken.None)
-                        .GetAwaiter().GetResult();
+                    try
+                    {
+                        this.KillServerInstancesAsync(EventContext.None, CancellationToken.None)
+                            .GetAwaiter().GetResult();
+                    }
+                    catch
+                    {
+                        // Best-effort cleanup during dispose.
+                    }
+
                     this.disposed = true;
                 }
             }
diff --git a/src/VirtualClient/VirtualClient.Actions/Nginx/NginxExtensions.cs b/src/VirtualClient/VirtualClient.Actions/Nginx/NginxExtensions.cs
@@ -50,7 +50,7 @@ public static string ConvertToCommandArgs(this NginxCommand command)
                     return "systemctl restart nginx";
 
                 case NginxCommand.Stop:
-                    return "systemctl disable nginx";
+                    return "systemctl stop nginx";
 
                 case NginxCommand.GetVersion:
                     return "nginx -V";

Original file line number	Diff line number	Diff line change
`@@ -6,6 +6,7 @@ namespace VirtualClient.Actions`
`6`	`6`	`using System;`
`7`	`7`	`using System.Collections.Generic;`
`8`	`8`	`using System.Threading;`
	`9`	`+ using System.Threading.Tasks;`
`9`	`10`	`using Microsoft.Extensions.DependencyInjection;`
`10`	`11`	`using Moq;`
`11`	`12`	`using NUnit.Framework;`
`@@ -46,40 +47,40 @@ public void SetupDefaults()`
`46`	`47`	`}`
`47`	`48`
`48`	`49`	`[Test]`
`49`		`- public void BombardierExecutorGetBombardierVersionParsesVersionWithVPrefix()`
	`50`	`+ public async Task BombardierExecutorGetBombardierVersionParsesVersionWithVPrefix()`
`50`	`51`	`{`
`51`	`52`	`this.mockFixture.SetupProcessOutput(".--version.", "bombardier version v1.2.5 linux/arm64");`
`52`	`53`
`53`	`54`	`using (TestBombardierExecutor executor = new TestBombardierExecutor(this.mockFixture.Dependencies, this.mockFixture.Parameters))`
`54`	`55`	`{`
`55`	`56`	`executor.PackageDirectory = this.mockPackage.Path;`
`56`		`- string version = executor.GetBombardierVersion(EventContext.None, CancellationToken.None);`
	`57`	`+ string version = await executor.GetBombardierVersionAsync(EventContext.None, CancellationToken.None);`
`57`	`58`	`Assert.AreEqual("1.2.5", version);`
`58`	`59`	`}`
`59`	`60`	`}`
`60`	`61`
`61`	`62`	`[Test]`
`62`		`- public void BombardierExecutorGetBombardierVersionParsesVersionWithoutVPrefix()`
	`63`	`+ public async Task BombardierExecutorGetBombardierVersionParsesVersionWithoutVPrefix()`
`63`	`64`	`{`
`64`	`65`	`this.mockFixture.SetupProcessOutput(".--version.", "bombardier version 1.2.5");`
`65`	`66`
`66`	`67`	`using (TestBombardierExecutor executor = new TestBombardierExecutor(this.mockFixture.Dependencies, this.mockFixture.Parameters))`
`67`	`68`	`{`
`68`	`69`	`executor.PackageDirectory = this.mockPackage.Path;`
`69`		`- string version = executor.GetBombardierVersion(EventContext.None, CancellationToken.None);`
	`70`	`+ string version = await executor.GetBombardierVersionAsync(EventContext.None, CancellationToken.None);`
`70`	`71`	`Assert.AreEqual("1.2.5", version);`
`71`	`72`	`}`
`72`	`73`	`}`
`73`	`74`
`74`	`75`	`[Test]`
`75`		`- public void BombardierExecutorGetBombardierVersionReturnsNullOnUnparsableOutput()`
	`76`	`+ public async Task BombardierExecutorGetBombardierVersionReturnsNullOnUnparsableOutput()`
`76`	`77`	`{`
`77`	`78`	`this.mockFixture.SetupProcessOutput(".--version.", "unrecognized output");`
`78`	`79`
`79`	`80`	`using (TestBombardierExecutor executor = new TestBombardierExecutor(this.mockFixture.Dependencies, this.mockFixture.Parameters))`
`80`	`81`	`{`
`81`	`82`	`executor.PackageDirectory = this.mockPackage.Path;`
`82`		`- string version = executor.GetBombardierVersion(EventContext.None, CancellationToken.None);`
	`83`	`+ string version = await executor.GetBombardierVersionAsync(EventContext.None, CancellationToken.None);`
`83`	`84`	`Assert.IsNull(version);`
`84`	`85`	`}`
`85`	`86`	`}`
Original file line number	Diff line number	Diff line change
`@@ -274,7 +274,7 @@ public async Task NginxServerExecutorResetsServerAsExpected(PlatformID platform,`
`274`	`274`	`}`
`275`	`275`	`else`
`276`	`276`	`{`
`277`		`- Assert.AreEqual(arguments, "systemctl disable nginx", NginxCommand.Stop.ConvertToCommandArgs());`
	`277`	`+ Assert.AreEqual(arguments, "systemctl stop nginx", NginxCommand.Stop.ConvertToCommandArgs());`
`278`	`278`	`}`
`279`	`279`
`280`	`280`	`Assert.AreEqual(command, "sudo");`
`@@ -328,7 +328,7 @@ public void NginxServerExecutorWillResetServerDuringDispose(PlatformID platform,`
`328`	`328`	`}`
`329`	`329`	`else`
`330`	`330`	`{`
`331`		`- Assert.AreEqual(arguments, "systemctl disable nginx", NginxCommand.Stop.ConvertToCommandArgs());`
	`331`	`+ Assert.AreEqual(arguments, "systemctl stop nginx", NginxCommand.Stop.ConvertToCommandArgs());`
`332`	`332`	`}`
`333`	`333`
`334`	`334`	`Assert.AreEqual(command, "sudo");`
`@@ -379,7 +379,7 @@ public async Task NginxServerExecutorRunsAsExpected(PlatformID platform, Archite`
`379`	`379`	`shellScriptCalls++;`
`380`	`380`	`Assert.AreEqual(workingDir, packagePath);`
`381`	`381`	`}`
`382`		`- else if (new[] { "systemctl restart nginx", "systemctl disable nginx"}.Contains(arguments, StringComparer.OrdinalIgnoreCase))`
	`382`	`+ else if (new[] { "systemctl restart nginx", "systemctl stop nginx"}.Contains(arguments, StringComparer.OrdinalIgnoreCase))`
`383`	`383`	`{`
`384`	`384`	`nginxServiceCalls++;`
`385`	`385`	`Assert.IsNull(workingDir);`
`@@ -445,7 +445,7 @@ public void NginxServerExecutorWillResetServerIfFailure(PlatformID platform, Arc`
`445`	`445`	`// This will ensure nginx version is empty therefore ArgumentException will be thrown.`
`446`	`446`	`this.memoryProcess.StandardError = new ConcurrentBuffer(new StringBuilder(string.Empty));`
`447`	`447`	`}`
`448`		`- else if (arguments == "systemctl disable nginx" \|\| arguments == "bash reset.sh")`
	`448`	`+ else if (arguments == "systemctl stop nginx" \|\| arguments == "bash reset.sh")`
`449`	`449`	`{`
`450`	`450`	`}`
`451`	`451`	`else`
Original file line number	Diff line number	Diff line change
`@@ -144,8 +144,16 @@ protected override void Dispose(bool disposing)`
`144`	`144`	`{`
`145`	`145`	`if (!this.disposed)`
`146`	`146`	`{`
`147`		`- this.KillServerInstancesAsync(null, CancellationToken.None)`
`148`		`- .GetAwaiter().GetResult();`
	`147`	`+ try`
	`148`	`+ {`
	`149`	`+ this.KillServerInstancesAsync(EventContext.None, CancellationToken.None)`
	`150`	`+ .GetAwaiter().GetResult();`
	`151`	`+ }`
	`152`	`+ catch`
	`153`	`+ {`
	`154`	`+ // Best-effort cleanup during dispose.`
	`155`	`+ }`
	`156`	`+`
`149`	`157`	`this.disposed = true;`
`150`	`158`	`}`
`151`	`159`	`}`
Original file line number	Diff line number	Diff line change
`@@ -155,8 +155,16 @@ protected override void Dispose(bool disposing)`
`155`	`155`	`{`
`156`	`156`	`if (!this.disposed)`
`157`	`157`	`{`
`158`		`- this.KillServerInstancesAsync(null, CancellationToken.None)`
`159`		`- .GetAwaiter().GetResult();`
	`158`	`+ try`
	`159`	`+ {`
	`160`	`+ this.KillServerInstancesAsync(EventContext.None, CancellationToken.None)`
	`161`	`+ .GetAwaiter().GetResult();`
	`162`	`+ }`
	`163`	`+ catch`
	`164`	`+ {`
	`165`	`+ // Best-effort cleanup during dispose.`
	`166`	`+ }`
	`167`	`+`
`160`	`168`	`this.disposed = true;`
`161`	`169`	`}`
`162`	`170`	`}`