From d3344e92b08fe2936e7dc71cbd47afd2b0b86c4a Mon Sep 17 00:00:00 2001 From: Matthew Edmondson Date: Tue, 6 Oct 2026 03:28:33 +0000 Subject: [PATCH 1/2] [patch] Treat a partly failed process-tree kill as best effort, so Cancel() never throws Process.Kill(entireProcessTree: true) reports descendants it could not terminate (owned by another user, or elevated) as AggregateException, which TryKill did not catch. It escaped the cancellation registration out of the caller's Cancel(), replaced the OperationCanceledException RunAsync rethrows, and hid a handler's real exception. TryKill now catches it, and takes the kill as a parameter so a test can make it fail. Fixes #107 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Bm33TiYrofqKs4oGc3UGUU --- RunCommand.Test/RunCommandTests.cs | 32 ++++++++++++++++++++++++++ RunCommand/RunCommand.cs | 37 +++++++++++++++++++++++------- 2 files changed, 61 insertions(+), 8 deletions(-) diff --git a/RunCommand.Test/RunCommandTests.cs b/RunCommand.Test/RunCommandTests.cs index 818e08c..b9abcbb 100644 --- a/RunCommand.Test/RunCommandTests.cs +++ b/RunCommand.Test/RunCommandTests.cs @@ -450,6 +450,38 @@ public async Task ExecuteAsyncShouldTerminateProcessWhenCancelledWhileRunning() await Assert.ThrowsAsync(() => execution).ConfigureAwait(false); } + [TestMethod] + public void CancelShouldNotThrowWhenTheTreeKillCannotTerminateEveryDescendant() + { + (string fileName, string[] arguments) = GetSleepCommand(); + using Process process = Process.Start(new ProcessStartInfo(fileName, arguments) { UseShellExecute = false })!; + + try + { + // What Process.Kill(entireProcessTree: true) throws when a descendant belongs to another + // user or runs elevated. + static void PartlyFailingKill(Process _) => + throw new AggregateException( + "Not all processes in the process tree could be terminated.", + new System.ComponentModel.Win32Exception(1, "Operation not permitted")); + + // Registered the way RunAsync registers its kill, so this is the caller's Cancel(). + using CancellationTokenSource cancellationTokenSource = new(); + using CancellationTokenRegistration registration = cancellationTokenSource.Token.Register( + () => RunCommand.TryKill(process, PartlyFailingKill)); + + cancellationTokenSource.Cancel(); + + // Called directly as well, as the catch blocks in RunAsync do: an exception here would + // replace the OperationCanceledException or the handler failure being reported. + RunCommand.TryKill(process, PartlyFailingKill); + } + finally + { + process.Kill(entireProcessTree: true); + } + } + [TestMethod] public async Task ExecuteAsyncShouldThrowRatherThanReturnAnExitCodeWhenCancellationWinsTheRace() { diff --git a/RunCommand/RunCommand.cs b/RunCommand/RunCommand.cs index 2440dc5..3ee34b8 100644 --- a/RunCommand/RunCommand.cs +++ b/RunCommand/RunCommand.cs @@ -529,19 +529,26 @@ private static async Task RunAsync(ProcessStartInfo startInfo, OutputHandle return process.ExitCode; } - private static void TryKill(Process process) + private static void TryKill(Process process) => TryKill(process, Kill); + + /// + /// Kills through , treating every way the kill + /// can fail as best effort. + /// + /// + /// This runs inside the cancellation token's registration, so anything it throws comes out of + /// the caller's , and it runs in catch blocks, + /// where anything it throws replaces the exception being reported. It must never throw. + /// + /// The process to kill. + /// Performs the kill. A parameter so tests can make it fail. + internal static void TryKill(Process process, Action kill) { try { if (!process.HasExited) { -#if NETSTANDARD2_0 || NETSTANDARD2_1 - // Process.Kill(bool) requires .NET Core 3.0 or later, so the older targets can only - // terminate the process itself and not any grandchildren it spawned. - process.Kill(); -#else - process.Kill(entireProcessTree: true); -#endif + kill(process); } } catch (InvalidOperationException) @@ -556,5 +563,19 @@ private static void TryKill(Process process) { // Terminating a remote process is not supported. } + catch (AggregateException) + { + // Kill(entireProcessTree) reports descendants it could not terminate, such as ones owned + // by another user or running elevated, this way. It carries on past each failure, so + // everything it could kill has already been killed. + } } + +#if NETSTANDARD2_0 || NETSTANDARD2_1 + // Process.Kill(bool) requires .NET Core 3.0 or later, so the older targets can only terminate + // the process itself and not any grandchildren it spawned. + private static void Kill(Process process) => process.Kill(); +#else + private static void Kill(Process process) => process.Kill(entireProcessTree: true); +#endif } From 4642a2bf5a71b9a6d7e5d86651e2c77c731158f0 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 6 Oct 2026 07:32:50 +0000 Subject: [PATCH 2/2] Assert that Cancel() returns and that only the failing kill ran SonarCloud S2699 flagged the test as having no assertion. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01JE3qjjjTXN28xUcTq2h6vD --- RunCommand.Test/RunCommandTests.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/RunCommand.Test/RunCommandTests.cs b/RunCommand.Test/RunCommandTests.cs index 8c3bf01..767cfbc 100644 --- a/RunCommand.Test/RunCommandTests.cs +++ b/RunCommand.Test/RunCommandTests.cs @@ -471,10 +471,14 @@ static void PartlyFailingKill(Process _) => () => RunCommand.TryKill(process, PartlyFailingKill)); cancellationTokenSource.Cancel(); + Assert.IsTrue(cancellationTokenSource.IsCancellationRequested, "Expected Cancel() to return normally after the kill failed."); // Called directly as well, as the catch blocks in RunAsync do: an exception here would // replace the OperationCanceledException or the handler failure being reported. RunCommand.TryKill(process, PartlyFailingKill); + + // Only the injected kill ran, so the process is still alive for the finally block to end. + Assert.IsFalse(process.HasExited, "Expected only the failing kill to have run."); } finally {