From 94f7ede316d8fa99c3093e5a70802670b4e2ac13 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 24 Sep 2026 20:32:14 +0000 Subject: [PATCH] Bound the output pump by the cancellation token [patch] Cancelling a run could hang forever. AsyncProcessStreamReader waited for both pipes to reach end of stream, and end of stream means every handle on the write end has closed. Killing the command closes only the handles the command itself held, so a descendant that inherited one and outlived its parent kept the pipe open and the wait never ended -- contradicting the documented contract that cancellation kills the process and lets the await proceed. Start now takes the cancellation token and races each wait against it, giving up on a pending read rather than trying to cancel it. Cancelling a read is not an option worth relying on here: StreamReader has no cancellable overload on every target, and a pipe read is not reliably interruptible even where one exists. Abandoned reads get a continuation that observes their eventual fault, so a cancelled run cannot trip TaskScheduler.UnobservedTaskException later. The netstandard2.0/2.1 targets were hit harder, since only a single-process kill is available there and even a non-detached child triggered the same wait. Fixes #78. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Wi82CWVJxLjZR27vTkenFm --- RunCommand.Test/RunCommandTests.cs | 38 +++++++++++ RunCommand/AsyncProcessStreamReader.cs | 88 ++++++++++++++++++++++++-- RunCommand/RunCommand.cs | 2 +- 3 files changed, 123 insertions(+), 5 deletions(-) diff --git a/RunCommand.Test/RunCommandTests.cs b/RunCommand.Test/RunCommandTests.cs index 8fb5335..03fd4c4 100644 --- a/RunCommand.Test/RunCommandTests.cs +++ b/RunCommand.Test/RunCommandTests.cs @@ -468,6 +468,44 @@ await Assert.ThrowsAsync( } } + [TestMethod] + public async Task ExecuteAsyncShouldReturnWhenCancelledWhileADetachedDescendantHoldsTheOutputPipe() + { + if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) + { + Assert.Inconclusive("Needs a shell that can orphan a child out of its own process tree while that child keeps the pipe it inherited. The wait this covers is in platform independent code, so the other legs cover it."); + } + + using CancellationTokenSource cancellationTokenSource = new(); + + // The inner shell backgrounds a sleep and exits immediately, so that sleep is reparented to + // init and is no longer a descendant the entire-process-tree kill can walk to — but it still + // holds the standard output and standard error handles it inherited. The outer sleep keeps + // the process this call owns alive, so cancellation is what ends it. Killing that process + // therefore closes neither pipe's write end, and end of stream never arrives. + // + // setsid is not enough here: it gives the child its own session but leaves its parent alone, + // so the kill still reaches it. + Task execution = RunCommand.ExecuteAsync( + "sh", + ["-c", "sh -c 'sleep 30 &'; sleep 30"], + new OutputHandler(), + cancellationTokenSource.Token); + + await cancellationTokenSource.CancelAsync().ConfigureAwait(false); + + // Bounded rather than a bare await: before the fix this call never returns, and a test that + // hangs takes the whole run down with it instead of reporting a failure. + Task finished = await Task.WhenAny(execution, Task.Delay(TimeSpan.FromSeconds(10))).ConfigureAwait(false); + + Assert.AreSame( + execution, + finished, + "Expected a cancelled call to return promptly rather than wait on a pipe an orphaned descendant still holds open."); + + await Assert.ThrowsAsync(() => execution).ConfigureAwait(false); + } + [TestMethod] public async Task ExecuteAsyncShouldStartTheProcessInTheGivenWorkingDirectory() { diff --git a/RunCommand/AsyncProcessStreamReader.cs b/RunCommand/AsyncProcessStreamReader.cs index 1a8c809..c04be15 100644 --- a/RunCommand/AsyncProcessStreamReader.cs +++ b/RunCommand/AsyncProcessStreamReader.cs @@ -3,6 +3,7 @@ namespace ktsu.RunCommand; using System.Diagnostics; +using System.Threading; internal sealed class AsyncProcessStreamReader(Process process, OutputHandler outputHandler) : IDisposable { @@ -32,8 +33,34 @@ public void Dispose() errorStream.Dispose(); } - internal async Task Start() + /// + /// Pumps the process's standard output and standard error to the handler until both pipes reach + /// end of stream, or until is signalled. + /// + /// + /// A read of a redirected pipe returns on new data or on end of stream, and nothing else — there + /// is no token that reaches into it, and a pipe read is not reliably interruptible even on the + /// targets whose offers a cancellable overload. So cancellation is + /// handled by giving up on the pending read rather than by cancelling it. + /// + /// That distinction is the whole point. End of stream means every handle on the write end has + /// closed, and killing the command closes only the handles the command itself held: one that a + /// descendant inherited and carried past its parent's death keeps the pipe open. Waiting for end + /// of stream after a kill therefore waits on something that may never happen, which left a + /// cancelled call hanging indefinitely. + /// + /// + /// The token the caller cancelled the run with. + internal async Task Start(CancellationToken cancellationToken) { + TaskCompletionSource cancellationSource = new(); + + using CancellationTokenRegistration registration = cancellationToken.Register( + static state => ((TaskCompletionSource)state!).TrySetResult(true), + cancellationSource); + + Task cancelled = cancellationSource.Task; + Task outputTask = Task.CompletedTask; Task errorTask = Task.CompletedTask; @@ -59,16 +86,69 @@ internal async Task Start() errorTask = ReadAndCallback(errorStream, errorBuffer, outputHandler.HandleStandardErrorData, isStandardOutput: false); } - await Task.WhenAny(outputTask, errorTask).ConfigureAwait(false); + Task first = await Task.WhenAny(outputTask, errorTask, cancelled).ConfigureAwait(false); + if (ReferenceEquals(first, cancelled)) + { + Abandon(outputTask, errorTask); + return; + } } while (!process.HasExited); - await Task.WhenAll(outputTask, errorTask).ConfigureAwait(false); + if (!await DrainOrAbandon(outputTask, errorTask, cancelled).ConfigureAwait(false)) + { + return; + } // Read any remaining data after process exit. outputTask = ReadAndCallback(outputStream, outputBuffer, outputHandler.HandleStandardOutputData, isStandardOutput: true); errorTask = ReadAndCallback(errorStream, errorBuffer, outputHandler.HandleStandardErrorData, isStandardOutput: false); - await Task.WhenAll(outputTask, errorTask).ConfigureAwait(false); + _ = await DrainOrAbandon(outputTask, errorTask, cancelled).ConfigureAwait(false); + } + + /// + /// Waits for both reads to finish, unless cancellation gets there first. + /// + /// + /// when both reads finished, so the caller may carry on; + /// when cancellation won and the reads were abandoned. + /// + private static async Task DrainOrAbandon(Task outputTask, Task errorTask, Task cancelled) + { + Task reads = Task.WhenAll(outputTask, errorTask); + + if (ReferenceEquals(await Task.WhenAny(reads, cancelled).ConfigureAwait(false), cancelled)) + { + Abandon(outputTask, errorTask); + return false; + } + + // Awaited rather than returned so that a read that failed still throws here, which is what + // carries a decode error out to the caller. + await reads.ConfigureAwait(false); + return true; + } + + /// + /// Leaves reads this call has given up on to end however they end, observing the result. + /// + /// + /// Disposing the readers ends an abandoned read, but it ends by faulting, and a faulted task + /// nobody ever looks at raises when it is + /// finalized. Looking at it here keeps a cancelled run from tripping that on an unrelated thread + /// later. A read that never ends at all costs a buffer until the process handle is released, + /// which is the price of not waiting on a pipe the caller has already walked away from. + /// + private static void Abandon(params Task[] reads) + { + foreach (Task read in reads) + { + _ = read.ContinueWith( + static abandoned => _ = abandoned.Exception, + CancellationToken.None, + TaskContinuationOptions.OnlyOnFaulted | TaskContinuationOptions.ExecuteSynchronously, + TaskScheduler.Default); + } } private async Task ReadAndCallback(StreamReader streamReader, char[] buffer, Action? onData, bool isStandardOutput) => diff --git a/RunCommand/RunCommand.cs b/RunCommand/RunCommand.cs index bcf37bb..731bf16 100644 --- a/RunCommand/RunCommand.cs +++ b/RunCommand/RunCommand.cs @@ -463,7 +463,7 @@ private static async Task RunAsync(ProcessStartInfo startInfo, OutputHandle else { using AsyncProcessStreamReader outputReader = new(process, outputHandler); - await Task.WhenAll(outputReader.Start(), process.WaitForExitAsync(cancellationToken)).ConfigureAwait(false); + await Task.WhenAll(outputReader.Start(cancellationToken), process.WaitForExitAsync(cancellationToken)).ConfigureAwait(false); } // Cancellation reaches the wait two ways at once: the registration above kills the process,