From c2570fc1e43ec498449956f1f68f5dda1582cda8 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 28 Sep 2026 17:12:31 +0000 Subject: [PATCH] Kill the process tree whenever a cancelled wait ends the call [patch] RunAsync relied on a cancellation registration alone to kill the process. A token runs its callbacks newest first, and WaitForExitAsync registers its own after the kill's. The wait could therefore observe cancellation, end the call, and dispose the kill registration before that callback had run. The call returned OperationCanceledException with the whole process tree still running. The wait is now wrapped so that a cancelled wait calls TryKill itself before rethrowing. A second kill of a process that has already gone is a no-op. The new test cancels a command with a descendant 40 times through a linked timeout token, which cancels on a timer thread. It requires each descendant to have exited. It reads /proc so a reparented zombie counts as gone. Before the fix it failed in 3 of 5 runs locally; after the fix, 0 of 5. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01TAvt7dvjcukkH3y6mtUL16 --- RunCommand.Test/RunCommandTests.cs | 85 ++++++++++++++++++++++++++++++ RunCommand/RunCommand.cs | 23 ++++++-- 2 files changed, 103 insertions(+), 5 deletions(-) diff --git a/RunCommand.Test/RunCommandTests.cs b/RunCommand.Test/RunCommandTests.cs index 3a34051..97c1191 100644 --- a/RunCommand.Test/RunCommandTests.cs +++ b/RunCommand.Test/RunCommandTests.cs @@ -471,6 +471,91 @@ await Assert.ThrowsAsync( } } + [TestMethod] + public async Task ExecuteAsyncShouldKillTheProcessTreeEveryTimeItIsCancelled() + { + if (!RuntimeInformation.IsOSPlatform(OSPlatform.Linux)) + { + Assert.Inconclusive("Reads a descendant's state from /proc. The kill this covers is in platform independent code, so the Linux leg covers it."); + } + + // A token runs its callbacks newest first, and the wait registers its own after the kill's. + // So the wait can observe cancellation, end the call, and dispose the kill's registration + // before that callback has run, and the call returns with its whole tree still running. The + // wait only wins that race some of the time, which is why this repeats. The shape is a + // caller's own deadline linked into its token, which cancels on a timer thread. + for (int attempt = 0; attempt < 40; attempt++) + { + TaskCompletionSource descendant = new(TaskCreationOptions.RunContinuationsAsynchronously); + OutputHandler handler = new( + chunk => + { + if (int.TryParse(chunk.Trim(), out int id)) + { + descendant.TrySetResult(id); + } + }, + _ => { }); + + using CancellationTokenSource timeout = new(TimeSpan.FromMilliseconds(200)); + using CancellationTokenSource linked = CancellationTokenSource.CreateLinkedTokenSource(CancellationToken.None, timeout.Token); + + Task execution = RunCommand.ExecuteAsync( + "sh", + ["-c", "sleep 30 & echo $!; wait"], + handler, + new CommandOptions { StandardInput = StandardInputMode.Closed }, + linked.Token); + + int descendantId = await descendant.Task.WaitAsync(TimeSpan.FromSeconds(10)).ConfigureAwait(false); + + await Assert.ThrowsAsync(() => execution).ConfigureAwait(false); + + Assert.IsTrue( + await HasExitedAsync(descendantId).ConfigureAwait(false), + $"Attempt {attempt}: descendant {descendantId} was still running after its call was cancelled."); + } + } + + /// + /// Waits up to five seconds for a process to exit. + /// + /// + /// Reads /proc rather than asking , because the process is not a child + /// of this one: once its parent is killed it is reparented, and it can sit as a zombie until its + /// new parent reaps it. A zombie has exited, so it counts as gone. + /// + private static async Task HasExitedAsync(int processId) + { + for (int check = 0; check < 50; check++) + { + string status; + + try + { + status = await File.ReadAllTextAsync($"/proc/{processId}/stat").ConfigureAwait(false); + } + catch (IOException) + { + return true; + } + catch (UnauthorizedAccessException) + { + return true; + } + + // The state follows the parenthesised command name, which may itself contain spaces. + if (status[(status.LastIndexOf(')') + 2)..].StartsWith('Z')) + { + return true; + } + + await Task.Delay(100).ConfigureAwait(false); + } + + return false; + } + [TestMethod] public async Task ExecuteAsyncShouldReturnWhenCancelledWhileADetachedDescendantHoldsTheOutputPipe() { diff --git a/RunCommand/RunCommand.cs b/RunCommand/RunCommand.cs index e3da840..0bc9b75 100644 --- a/RunCommand/RunCommand.cs +++ b/RunCommand/RunCommand.cs @@ -475,14 +475,27 @@ private static async Task RunAsync(ProcessStartInfo startInfo, OutputHandle // would leave the child running unsupervised. using CancellationTokenRegistration registration = cancellationToken.Register(() => TryKill(process)); - if (useElevation) + try { - await process.WaitForExitAsync(cancellationToken).ConfigureAwait(false); + if (useElevation) + { + await process.WaitForExitAsync(cancellationToken).ConfigureAwait(false); + } + else + { + using AsyncProcessStreamReader outputReader = new(process, outputHandler); + await Task.WhenAll(outputReader.Start(cancellationToken), process.WaitForExitAsync(cancellationToken)).ConfigureAwait(false); + } } - else + catch (OperationCanceledException) { - using AsyncProcessStreamReader outputReader = new(process, outputHandler); - await Task.WhenAll(outputReader.Start(cancellationToken), process.WaitForExitAsync(cancellationToken)).ConfigureAwait(false); + // The registration alone does not guarantee the kill. A token runs its callbacks newest + // first, and the wait registered its own after this one, so the wait can end this call + // and dispose the registration before the kill callback has run. That leaves the whole + // tree running unsupervised. Killing here as well makes it certain; a second kill of a + // process that has already gone is a no-op. + TryKill(process); + throw; } // Cancellation reaches the wait two ways at once: the registration above kills the process,