Kill the child when ExecuteAsync is cancelled, as Execute does [patch]

CLAUDE.md

@@ -146,7 +146,7 @@ Tests use **MSTest.Sdk** targeting net10.0 only. The test project (`Essentials.T
 - `RandomProviderTests.cs` - Contract tests over all 4 random providers (bounds, validation, shuffle and sampling invariants, uniformity of a range that does not divide 2^32), plus reference sequences for Xoshiro and Pcg produced by an independent transcription of each published algorithm. Those vectors make a seeded sequence part of the package contract: changing one is a breaking change and this is where it surfaces
 - `DistributionProviderTests.cs` - Tests all 10 distributions three ways: contract properties every distribution must have (CDF bounded and non-decreasing, quantile inverts it, density integrates to one, masses sum to one, survival function complements the CDF), reference values computed outside the codebase, and empirical checks that seeded samples match the analytic moments and deciles
 - `CacheProviderTests.cs` - Tests cache operations including expiration, and that a legitimately cached `null` is honoured rather than treated as a miss: `Get`/`GetAsync` return it instead of throwing, and `GetOrAdd`/`GetOrAddAsync` return it without re-invoking the factory. `TryGet`'s `bool` is the sole authority on presence — a null check on the value is not a proxy for it. It also pins that an expired entry's removal leaves a value set concurrently under the same key in place (a key type that runs the `Set` between `TryGet`'s read and its remove makes that interleaving deterministic), and that a `TimeSpan.MaxValue` or `TimeSpan.MinValue` time-to-live saturates rather than overflowing
-- `CommandExecutorTests.cs` - Tests command execution, including the synchronous path, cancellation before and during a run, a working directory that does not exist, that the synchronous and asynchronous paths capture the same bytes for a command whose output ends without a terminator, and that `ExecuteAndGetOutput` throws unwrapped. `ICommandExecutor`'s own synchronous defaults are reached through a test double that declares only the asynchronous members, since `NativeCommandExecutor` replaces them
+- `CommandExecutorTests.cs` - Tests command execution, including the synchronous path, cancellation before and during a run (and that both paths kill a cancelled command rather than leaving it to finish, issue #53), a working directory that does not exist, that the synchronous and asynchronous paths capture the same bytes for a command whose output ends without a terminator, and that `ExecuteAndGetOutput` throws unwrapped. `ICommandExecutor`'s own synchronous defaults are reached through a test double that declares only the asynchronous members, since `NativeCommandExecutor` replaces them
 - `EncodingProviderTests.cs` - Tests Base64 and Hex encoding
 - `ObfuscationProviderTests.cs` - Tests all obfuscation providers via round-trip (obfuscate → deobfuscate)
 - `FileSystemProviderTests.cs` - Tests filesystem operations

Essentials.CommandExecutors.Native/NativeCommandExecutor.cs

@@ -62,28 +62,35 @@ public async Task<CommandResult> ExecuteAsync(string command, IReadOnlyDictionar
 
 			process.Start();
 
+			try
+			{
 #if NET7_0_OR_GREATER
-			Task<string> stdoutTask = process.StandardOutput.ReadToEndAsync(cancellationToken);
-			Task<string> stderrTask = process.StandardError.ReadToEndAsync(cancellationToken);
+				Task<string> stdoutTask = process.StandardOutput.ReadToEndAsync(cancellationToken);
+				Task<string> stderrTask = process.StandardError.ReadToEndAsync(cancellationToken);
 #else
-			Task<string> stdoutTask = process.StandardOutput.ReadToEndAsync();
-			Task<string> stderrTask = process.StandardError.ReadToEndAsync();
+				Task<string> stdoutTask = process.StandardOutput.ReadToEndAsync();
+				Task<string> stderrTask = process.StandardError.ReadToEndAsync();
 #endif
 
 #if NET5_0_OR_GREATER
-			await process.WaitForExitAsync(cancellationToken).ConfigureAwait(false);
+				await process.WaitForExitAsync(cancellationToken).ConfigureAwait(false);
 #else
-			await Task.Run(process.WaitForExit, cancellationToken).ConfigureAwait(false);
+				await Task.Run(process.WaitForExit, cancellationToken).ConfigureAwait(false);
 #endif
 
-			string stdout = await stdoutTask.ConfigureAwait(false);
-			string stderr = await stderrTask.ConfigureAwait(false);
+				string stdout = await stdoutTask.ConfigureAwait(false);
+				string stderr = await stderrTask.ConfigureAwait(false);
 
-			return new CommandResult(process.ExitCode, stdout, stderr);
-		}
-		catch (OperationCanceledException)
-		{
-			return Cancelled();
+				return new CommandResult(process.ExitCode, stdout, stderr);
+			}
+			catch (OperationCanceledException)
+			{
+				// Disposing the process does not end the child, so a cancelled command would otherwise run on
+				// to completion, side effects and all, after the caller was told it had been cancelled. The
+				// synchronous path kills it too. See issue #53.
+				TryKill(process);
+				return Cancelled();
+			}
 		}
 		catch (InvalidOperationException ex)
 		{
@@ -203,15 +210,20 @@ private static ProcessStartInfo CreateStartInfo(string command, IReadOnlyDiction
 	}
 
 	/// <summary>
-	/// Kills a process that is being abandoned because the operation was cancelled, ignoring the races
-	/// where it has already exited or was never started.
+	/// Kills a process that is being abandoned because the operation was cancelled, along with the
+	/// commands the shell started where the platform can reach them, ignoring the races where it has
+	/// already exited or was never started.
 	/// </summary>
 	/// <param name="process">The process to kill.</param>
 	private static void TryKill(Process process)
 	{
 		try
 		{
+#if NETCOREAPP3_0_OR_GREATER
+			process.Kill(entireProcessTree: true);
+#else
 			process.Kill();
+#endif
 		}
 		catch (Exception ex) when (ex is InvalidOperationException or System.ComponentModel.Win32Exception)
 		{

Essentials.Tests/CommandExecutorTests.cs

@@ -235,6 +235,80 @@ public void CommandExecutor_Sync_Execute_Cancels_A_Running_Process(ICommandExecu
 			$"{providerName} sync should return on cancellation rather than waiting {SleepSeconds}s for the child (took {stopwatch.Elapsed})");
 	}
 
+	/// <summary>
+	/// Regression test for issue #53: cancelling <c>ExecuteAsync</c> reported "cancelled" but left the child
+	/// running, so a command whose side effect came after the cancellation still performed it. Both paths
+	/// must stop the command, not just stop waiting for it.
+	/// </summary>
+	/// <param name="executor">The executor under test.</param>
+	/// <param name="providerName">The name of the executor, for assertion messages.</param>
+	[TestMethod]
+	[DynamicData(nameof(CommandExecutors))]
+	public void CommandExecutor_Cancellation_Stops_The_Command(ICommandExecutor executor, string providerName)
+	{
+		string asyncMarker = Path.Join(Path.GetTempPath(), $"ktsu-cancel-async-{Guid.NewGuid():N}");
+		string syncMarker = Path.Join(Path.GetTempPath(), $"ktsu-cancel-sync-{Guid.NewGuid():N}");
+
+		TimeSpan sideEffectDelay = TimeSpan.FromSeconds(SideEffectDelaySeconds);
+
+		try
+		{
+			Stopwatch asyncRun = Stopwatch.StartNew();
+			using (CancellationTokenSource cts = new(CancellationDelay))
+			{
+				CommandResult result = executor.ExecuteAsync(SleepThenTouchCommand(asyncMarker), cancellationToken: cts.Token).Result;
+				Assert.IsFalse(result.Success, $"{providerName} async should report the cancellation");
+			}
+
+			// A run that took as long as the command's own wait proves nothing either way: the command may
+			// have finished before the cancellation was even observed.
+			Assert.IsLessThan(sideEffectDelay, asyncRun.Elapsed, $"{providerName} async should observe the cancellation before the command's side effect is due");
+
+			Stopwatch syncRun = Stopwatch.StartNew();
+			using (CancellationTokenSource cts = new(CancellationDelay))
+			{
+				CommandResult result = executor.Execute(SleepThenTouchCommand(syncMarker), cancellationToken: cts.Token);
+				Assert.IsFalse(result.Success, $"{providerName} sync should report the cancellation");
+			}
+
+			Assert.IsLessThan(sideEffectDelay, syncRun.Elapsed, $"{providerName} sync should observe the cancellation before the command's side effect is due");
+
+			// Outlast both commands' own waits, counted from when the later one started, so a child that
+			// survived has had time to write its marker.
+			TimeSpan remaining = sideEffectDelay + TimeSpan.FromSeconds(2) - syncRun.Elapsed;
+			if (remaining > TimeSpan.Zero)
+			{
+				_ = TestContext.CancellationToken.WaitHandle.WaitOne(remaining);
+			}
+
+			Assert.IsFalse(File.Exists(asyncMarker), $"{providerName} async should kill a cancelled command before it reaches its side effect");
+			Assert.IsFalse(File.Exists(syncMarker), $"{providerName} sync should kill a cancelled command before it reaches its side effect");
+		}
+		finally
+		{
+			File.Delete(asyncMarker);
+			File.Delete(syncMarker);
+		}
+	}
+
+	/// <summary>
+	/// How long the command in <see cref="CommandExecutor_Cancellation_Stops_The_Command"/> waits before
+	/// creating its marker. It has to be far past <see cref="CancellationDelay"/>, with room for a loaded CI
+	/// runner to be slow to deliver the cancellation: at two seconds a Windows runner once observed it only
+	/// as the command finished.
+	/// </summary>
+	private const int SideEffectDelaySeconds = 10;
+
+	/// <summary>
+	/// Builds a command that waits <see cref="SideEffectDelaySeconds"/> and then creates <paramref name="marker"/>.
+	/// </summary>
+	/// <param name="marker">The file the command creates once its wait is over.</param>
+	/// <returns>The platform's command.</returns>
+	private static string SleepThenTouchCommand(string marker) =>
+		RuntimeInformation.IsOSPlatform(OSPlatform.Windows)
+			? $"{SleepCommand(SideEffectDelaySeconds)} & type nul > \"{marker}\""
+			: $"{SleepCommand(SideEffectDelaySeconds)}; touch '{marker}'";
+
 	/// <summary>
 	/// Tests that a working directory that does not exist is reported as a failed result rather than
 	/// escaping as an exception from either path.