Bound the output pump by the cancellation token

RunCommand.Test/RunCommandTests.cs

@@ -468,6 +468,44 @@
 		}
 	}
 
+	[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<int> 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<OperationCanceledException>(() => execution).ConfigureAwait(false);
+	}
+
 	[TestMethod]
 	public async Task ExecuteAsyncShouldStartTheProcessInTheGivenWorkingDirectory()
 	{

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 @@
 		errorStream.Dispose();
 	}
 
-	internal async Task Start()
+	/// <summary>
+	/// Pumps the process's standard output and standard error to the handler until both pipes reach
+	/// end of stream, or until <paramref name="cancellationToken"/> is signalled.
+	/// </summary>
+	/// <remarks>
+	/// 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 <see cref="StreamReader"/> offers a cancellable overload. So cancellation is
+	/// handled by giving up on the pending read rather than by cancelling it.
+	/// <para>
+	/// 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.
+	/// </para>
+	/// </remarks>
+	/// <param name="cancellationToken">The token the caller cancelled the run with.</param>
+	internal async Task Start(CancellationToken cancellationToken)
 	{
+		TaskCompletionSource<bool> cancellationSource = new();
+
+		using CancellationTokenRegistration registration = cancellationToken.Register(
+			static state => ((TaskCompletionSource<bool>)state!).TrySetResult(true),
+			cancellationSource);
+
+		Task cancelled = cancellationSource.Task;
+
 		Task outputTask = Task.CompletedTask;
 		Task errorTask = Task.CompletedTask;
 
@@ -59,16 +86,69 @@
 				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);
+	}
+
+	/// <summary>
+	/// Waits for both reads to finish, unless cancellation gets there first.
+	/// </summary>
+	/// <returns>
+	/// <see langword="true"/> when both reads finished, so the caller may carry on;
+	/// <see langword="false"/> when cancellation won and the reads were abandoned.
+	/// </returns>
+	private static async Task<bool> 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;
+	}
+
+	/// <summary>
+	/// Leaves reads this call has given up on to end however they end, observing the result.
+	/// </summary>
+	/// <remarks>
+	/// Disposing the readers ends an abandoned read, but it ends by faulting, and a faulted task
+	/// nobody ever looks at raises <see cref="TaskScheduler.UnobservedTaskException"/> 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.
+	/// </remarks>
+	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<string>? onData, bool isStandardOutput) =>

RunCommand/RunCommand.cs

@@ -23,7 +23,7 @@
 	/// </summary>
 	/// <param name="command">The command to execute.</param>
 	/// <returns>The exit code of the executed process.</returns>
 	[Obsolete("A command string is split on its first space, which cannot handle an executable path "
 		+ "containing spaces. Use the overload taking a file name and an argument list instead.")]
 	public static int Execute(string command) =>
 		ExecuteAsync(command).Result;
@@ -34,7 +34,7 @@
 	/// <param name="command">The command to execute.</param>
 	/// <param name="outputHandler">The handler for processing command output.</param>
 	/// <returns>The exit code of the executed process.</returns>
 	[Obsolete("A command string is split on its first space, which cannot handle an executable path "
 		+ "containing spaces. Use the overload taking a file name and an argument list instead.")]
 	public static int Execute(string command, OutputHandler outputHandler) =>
 		ExecuteAsync(command, outputHandler).Result;
@@ -45,7 +45,7 @@
 	/// <param name="command">The command to execute.</param>
 	/// <param name="elevation">The privilege level under which to run the command.</param>
 	/// <returns>The exit code of the executed process.</returns>
 	[Obsolete("A command string is split on its first space, which cannot handle an executable path "
 		+ "containing spaces. Use the overload taking a file name and an argument list instead.")]
 	public static int Execute(string command, Elevation elevation) =>
 		ExecuteAsync(command, elevation).Result;
@@ -61,7 +61,7 @@
 	/// </param>
 	/// <param name="elevation">The privilege level under which to run the command.</param>
 	/// <returns>The exit code of the executed process.</returns>
 	[Obsolete("A command string is split on its first space, which cannot handle an executable path "
 		+ "containing spaces. Use the overload taking a file name and an argument list instead.")]
 	public static int Execute(string command, OutputHandler outputHandler, Elevation elevation) =>
 		ExecuteAsync(command, outputHandler, elevation).Result;
@@ -108,7 +108,7 @@
 	/// </summary>
 	/// <param name="command">The command to execute.</param>
 	/// <returns>A task representing the asynchronous operation with the process exit code.</returns>
 	[Obsolete("A command string is split on its first space, which cannot handle an executable path "
 		+ "containing spaces. Use the overload taking a file name and an argument list instead.")]
 	public static async Task<int> ExecuteAsync(string command)
 		=> await ExecuteAsync(command, new OutputHandler()).ConfigureAwait(false);
@@ -119,7 +119,7 @@
 	/// <param name="command">The command to execute.</param>
 	/// <param name="outputHandler">The handler for processing command output.</param>
 	/// <returns>A task representing the asynchronous operation with the process exit code.</returns>
 	[Obsolete("A command string is split on its first space, which cannot handle an executable path "
 		+ "containing spaces. Use the overload taking a file name and an argument list instead.")]
 	public static async Task<int> ExecuteAsync(string command, OutputHandler outputHandler)
 		=> await ExecuteAsync(command, outputHandler, Elevation.Default).ConfigureAwait(false);
@@ -130,7 +130,7 @@
 	/// <param name="command">The command to execute.</param>
 	/// <param name="elevation">The privilege level under which to run the command.</param>
 	/// <returns>A task representing the asynchronous operation with the process exit code.</returns>
 	[Obsolete("A command string is split on its first space, which cannot handle an executable path "
 		+ "containing spaces. Use the overload taking a file name and an argument list instead.")]
 	public static async Task<int> ExecuteAsync(string command, Elevation elevation)
 		=> await ExecuteAsync(command, new(), elevation).ConfigureAwait(false);
@@ -146,7 +146,7 @@
 	/// </param>
 	/// <param name="elevation">The privilege level under which to run the command.</param>
 	/// <returns>A task representing the asynchronous operation with the process exit code.</returns>
 	[Obsolete("A command string is split on its first space, which cannot handle an executable path "
 		+ "containing spaces. Use the overload taking a file name and an argument list instead.")]
 	public static async Task<int> ExecuteAsync(string command, OutputHandler outputHandler, Elevation elevation)
 		=> await ExecuteAsync(command, outputHandler, elevation, CancellationToken.None).ConfigureAwait(false);
@@ -174,7 +174,7 @@
 	/// A token that, when cancelled, terminates the running process and its children.
 	/// </param>
 	/// <returns>A task representing the asynchronous operation with the process exit code.</returns>
 	[Obsolete("A command string is split on its first space, which cannot handle an executable path "
 		+ "containing spaces. Use the overload taking a file name and an argument list instead.")]
 	public static async Task<int> ExecuteAsync(string command, OutputHandler outputHandler, CancellationToken cancellationToken)
 		=> await ExecuteAsync(command, outputHandler, Elevation.Default, cancellationToken).ConfigureAwait(false);
@@ -195,7 +195,7 @@
 	/// </param>
 	/// <returns>A task representing the asynchronous operation with the process exit code.</returns>
 	/// <exception cref="OperationCanceledException">The token was cancelled before the process exited.</exception>
 	[Obsolete("A command string is split on its first space, which cannot handle an executable path "
 		+ "containing spaces. Use the overload taking a file name and an argument list instead.")]
 	public static async Task<int> ExecuteAsync(string command, OutputHandler outputHandler, Elevation elevation, CancellationToken cancellationToken)
 	{
@@ -463,7 +463,7 @@
 		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,