From ef95867fac597555a09c0a5d4b918da2bd2a6061 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 7 Oct 2026 11:33:24 +0000 Subject: [PATCH 1/4] Kill the child when ExecuteAsync is cancelled, as Execute does [patch] Cancelling ExecuteAsync returned "Operation was cancelled." and disposed the Process, but disposing does not end the child, so the command ran on to completion and performed its side effects. The cancellation handler now kills the process before returning, like the synchronous path. TryKill also kills the whole process tree where the platform supports it (netcoreapp3.0+), so commands the shell started go with it on both paths. Fixes #53 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01D7wytU6jsZ1cH3f6grCTz5 --- CLAUDE.md | 2 +- .../NativeCommandExecutor.cs | 38 +++++++++---- Essentials.Tests/CommandExecutorTests.cs | 57 +++++++++++++++++++ 3 files changed, 85 insertions(+), 12 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index dc50833..99efeaf 100644 --- a/CLAUDE.md +++ b/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 diff --git a/Essentials.CommandExecutors.Native/NativeCommandExecutor.cs b/Essentials.CommandExecutors.Native/NativeCommandExecutor.cs index fed0507..e706952 100644 --- a/Essentials.CommandExecutors.Native/NativeCommandExecutor.cs +++ b/Essentials.CommandExecutors.Native/NativeCommandExecutor.cs @@ -62,24 +62,35 @@ public async Task ExecuteAsync(string command, IReadOnlyDictionar process.Start(); + try + { #if NET7_0_OR_GREATER - Task stdoutTask = process.StandardOutput.ReadToEndAsync(cancellationToken); - Task stderrTask = process.StandardError.ReadToEndAsync(cancellationToken); + Task stdoutTask = process.StandardOutput.ReadToEndAsync(cancellationToken); + Task stderrTask = process.StandardError.ReadToEndAsync(cancellationToken); #else - Task stdoutTask = process.StandardOutput.ReadToEndAsync(); - Task stderrTask = process.StandardError.ReadToEndAsync(); + Task stdoutTask = process.StandardOutput.ReadToEndAsync(); + Task 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); + 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 (OperationCanceledException) { @@ -203,15 +214,20 @@ private static ProcessStartInfo CreateStartInfo(string command, IReadOnlyDiction } /// - /// 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. /// /// The process to kill. 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) { diff --git a/Essentials.Tests/CommandExecutorTests.cs b/Essentials.Tests/CommandExecutorTests.cs index e14c724..49b5242 100644 --- a/Essentials.Tests/CommandExecutorTests.cs +++ b/Essentials.Tests/CommandExecutorTests.cs @@ -235,6 +235,63 @@ 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})"); } + /// + /// Regression test for issue #53: cancelling ExecuteAsync 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. + /// + /// The executor under test. + /// The name of the executor, for assertion messages. + [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}"); + + try + { + 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"); + } + + using (CancellationTokenSource cts = new(CancellationDelay)) + { + CommandResult result = executor.Execute(SleepThenTouchCommand(syncMarker), cancellationToken: cts.Token); + Assert.IsFalse(result.Success, $"{providerName} sync should report the cancellation"); + } + + // Outlast the command's own sleep, so a child that survived has had time to write its marker. + Thread.Sleep(TimeSpan.FromSeconds(SideEffectDelaySeconds + 2)); + + 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); + } + } + + /// + /// How long the command in waits before + /// creating its marker. It has to be well past . + /// + private const int SideEffectDelaySeconds = 2; + + /// + /// Builds a command that waits and then creates . + /// + /// The file the command creates once its wait is over. + /// The platform's command. + private static string SleepThenTouchCommand(string marker) => + RuntimeInformation.IsOSPlatform(OSPlatform.Windows) + ? $"{SleepCommand(SideEffectDelaySeconds)} & type nul > \"{marker}\"" + : $"{SleepCommand(SideEffectDelaySeconds)}; touch '{marker}'"; + /// /// Tests that a working directory that does not exist is reported as a failed result rather than /// escaping as an exception from either path. From 8f6c77581277f054524405ea3467e5345a47e967 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 7 Oct 2026 12:13:36 +0000 Subject: [PATCH 2/4] Drop the outer cancellation catch that the inner one made unreachable Every await that can observe the token now sits inside the inner try, so no OperationCanceledException can reach the outer handler. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01D7wytU6jsZ1cH3f6grCTz5 --- Essentials.CommandExecutors.Native/NativeCommandExecutor.cs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/Essentials.CommandExecutors.Native/NativeCommandExecutor.cs b/Essentials.CommandExecutors.Native/NativeCommandExecutor.cs index e706952..e474d5b 100644 --- a/Essentials.CommandExecutors.Native/NativeCommandExecutor.cs +++ b/Essentials.CommandExecutors.Native/NativeCommandExecutor.cs @@ -92,10 +92,6 @@ public async Task ExecuteAsync(string command, IReadOnlyDictionar return Cancelled(); } } - catch (OperationCanceledException) - { - return Cancelled(); - } catch (InvalidOperationException ex) { return new CommandResult(-1, string.Empty, ex.Message); From aa9e762ae0d097cc70628c7841a131010bb9f810 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 7 Oct 2026 12:28:15 +0000 Subject: [PATCH 3/4] Give the cancellation test room for a slow runner to deliver the token With a two-second side effect a loaded Windows runner observed the 250 ms cancellation only as the command finished, so the marker existed and the test blamed the kill. The command now waits ten seconds, each run asserts it returned well before that (a run that did not proves nothing either way), and the final wait is measured from the later run's start. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01D7wytU6jsZ1cH3f6grCTz5 --- Essentials.Tests/CommandExecutorTests.cs | 25 ++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/Essentials.Tests/CommandExecutorTests.cs b/Essentials.Tests/CommandExecutorTests.cs index 49b5242..e5553b5 100644 --- a/Essentials.Tests/CommandExecutorTests.cs +++ b/Essentials.Tests/CommandExecutorTests.cs @@ -249,22 +249,37 @@ public void CommandExecutor_Cancellation_Stops_The_Command(ICommandExecutor exec 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"); } - // Outlast the command's own sleep, so a child that survived has had time to write its marker. - Thread.Sleep(TimeSpan.FromSeconds(SideEffectDelaySeconds + 2)); + 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) + { + Thread.Sleep(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"); @@ -278,9 +293,11 @@ public void CommandExecutor_Cancellation_Stops_The_Command(ICommandExecutor exec /// /// How long the command in waits before - /// creating its marker. It has to be well past . + /// creating its marker. It has to be far past , 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. /// - private const int SideEffectDelaySeconds = 2; + private const int SideEffectDelaySeconds = 10; /// /// Builds a command that waits and then creates . From 9ccf95a068080bb5e18bf6bc3abf7f7181611707 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 7 Oct 2026 12:57:40 +0000 Subject: [PATCH 4/4] Wait on the test's cancellation token instead of Thread.Sleep Same wait for a side effect that must not happen, but it ends early when the test run is cancelled, and it clears Sonar S2925. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01D7wytU6jsZ1cH3f6grCTz5 --- Essentials.Tests/CommandExecutorTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Essentials.Tests/CommandExecutorTests.cs b/Essentials.Tests/CommandExecutorTests.cs index e5553b5..7fed12b 100644 --- a/Essentials.Tests/CommandExecutorTests.cs +++ b/Essentials.Tests/CommandExecutorTests.cs @@ -278,7 +278,7 @@ public void CommandExecutor_Cancellation_Stops_The_Command(ICommandExecutor exec TimeSpan remaining = sideEffectDelay + TimeSpan.FromSeconds(2) - syncRun.Elapsed; if (remaining > TimeSpan.Zero) { - Thread.Sleep(remaining); + _ = TestContext.CancellationToken.WaitHandle.WaitOne(remaining); } Assert.IsFalse(File.Exists(asyncMarker), $"{providerName} async should kill a cancelled command before it reaches its side effect");