Kill the command and rethrow when an output read fails [patch]

CLAUDE.md

@@ -152,6 +152,11 @@ A process can exit with tens of kilobytes still in the pipe, so stopping at exit
 read is raced against the caller's cancellation token, so a descendant that keeps the pipe open
 cannot hang a cancelled call.
 
+A read that fails (the handler throws, or a strict `Encoding` rejects the bytes) stops draining its
+pipe, so the command blocks as soon as that pipe fills and never exits. The reader therefore throws
+the first failure without waiting for the other stream, and `RunAsync` kills the process tree
+before rethrowing it rather than waiting for an exit that will not come.
+
 ### Process configuration
 
 On Windows, `LoadUserProfile` is set to true for proper environment variable expansion.

RunCommand.Test/RunCommandTests.cs

@@ -1101,6 +1101,84 @@ public async Task ExecuteAsyncShouldNotSpinWhileACommandThatClosedItsOutputKeeps
 			$"Expected the reader to wait rather than spin, but the process used {used.TotalMilliseconds:F0} ms of CPU during a {runMilliseconds} ms run.");
 	}
 
+	[TestMethod]
+	public async Task ExecuteAsyncShouldThrowAndKillTheCommandWhenTheOutputHandlerThrows() =>
+		await AssertAFaultedReadEndsTheCall(
+			"echo first",
+			new OutputHandler(_ => throw new InvalidOperationException("handler failed"))).ConfigureAwait(false);
+
+	[TestMethod]
+	public async Task ExecuteAsyncShouldThrowAndKillTheCommandWhenAStrictEncodingRejectsTheOutput() =>
+		await AssertAFaultedReadEndsTheCall(
+			@"printf '\377\n'",
+			new OutputHandler(encoding: new UTF8Encoding(encoderShouldEmitUTF8Identifier: false, throwOnInvalidBytes: true))).ConfigureAwait(false);
+
+	/// <summary>
+	/// Runs a command that makes the first read fault and then writes far more than a pipe holds,
+	/// and requires the call to rethrow that fault promptly with the command no longer running.
+	/// </summary>
+	/// <remarks>
+	/// A faulted read stops draining its pipe, so the command blocks writing to it and never exits.
+	/// Waiting for that exit is what hung the call.
+	/// </remarks>
+	private static async Task AssertAFaultedReadEndsTheCall(string faultingOutput, OutputHandler handler, [CallerMemberName] string testName = "")
+	{
+		if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
+		{
+			Assert.Inconclusive("Needs procfs to tell whether the command is still running. The fault handling this covers is in platform independent code, so the other legs cover it.");
+		}
+
+		string pidFile = Path.Join(Path.GetTempPath(), $"{nameof(RunCommandTests)}.{testName}.pid");
+		File.Delete(pidFile);
+
+		try
+		{
+			// exec keeps the writer on the pid written to the file, so the check below looks at the
+			// very process that is blocked on the full pipe.
+			Task<int> execution = RunCommand.ExecuteAsync(
+				"sh",
+				["-c", $"echo $$ > '{pidFile}'; {faultingOutput}; sleep 0.5; exec head -c 1000000 /dev/zero"],
+				handler);
+
+			// Bounded rather than a bare await: before the fix this call never returns.
+			Task finished = await Task.WhenAny(execution, Task.Delay(TimeSpan.FromSeconds(10))).ConfigureAwait(false);
+
+			Assert.AreSame(execution, finished, "Expected a faulted read to end the call rather than wait on a command blocked writing to it.");
+			Assert.IsTrue(execution.IsFaulted, "Expected the fault to reach the caller.");
+			Assert.IsNotInstanceOfType<OperationCanceledException>(execution.Exception!.InnerException);
+
+			string pid = (await File.ReadAllTextAsync(pidFile).ConfigureAwait(false)).Trim();
+			Assert.IsFalse(IsRunning(pid), $"Expected the command (pid {pid}) to have been killed.");
+		}
+		finally
+		{
+			File.Delete(pidFile);
+		}
+	}
+
+	/// <summary>
+	/// Reports whether a process is still running, counting a zombie as gone since it has exited.
+	/// </summary>
+	private static bool IsRunning(string pid)
+	{
+		string statPath = $"/proc/{pid}/stat";
+		if (!File.Exists(statPath))
+		{
+			return false;
+		}
+
+		try
+		{
+			string stat = File.ReadAllText(statPath);
+			// The state follows the parenthesised command name, which may itself contain spaces.
+			return stat[stat.LastIndexOf(')') + 2] != 'Z';
+		}
+		catch (IOException)
+		{
+			return false;
+		}
+	}
+
 	/// <summary>
 	/// Returns a command that prints a file's contents unchanged.
 	/// </summary>

RunCommand/AsyncProcessStreamReader.cs

@@ -1,4 +1,4 @@
-// Copyright (c) 2023-2026 ktsu-dev contributors
+// Copyright (c) 2023-2026 ktsu-dev contributors
 
 namespace ktsu.RunCommand;
 
@@ -70,25 +70,43 @@ internal async Task Start(CancellationToken cancellationToken)
 	}
 
 	/// <summary>
-	/// Waits for both reads to finish, unless cancellation gets there first.
+	/// Waits for both reads to finish, unless cancellation gets there first or a read fails.
 	/// </summary>
+	/// <remarks>
+	/// A read that fails stops draining its pipe, so the command blocks as soon as it fills that
+	/// pipe, and never closes the other one either. Waiting for the other read to finish would then
+	/// wait forever, so the first failure is thrown straight away, leaving the other read abandoned.
+	/// </remarks>
 	/// <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);
+		List<Task> pending = [outputTask, errorTask, cancelled];
 
-		if (ReferenceEquals(await Task.WhenAny(reads, cancelled).ConfigureAwait(false), cancelled))
+		while (pending.Count > 1)
 		{
-			Abandon(outputTask, errorTask);
-			return false;
+			Task finished = await Task.WhenAny(pending).ConfigureAwait(false);
+
+			if (ReferenceEquals(finished, cancelled))
+			{
+				Abandon(outputTask, errorTask);
+				return false;
+			}
+
+			if (finished.IsFaulted)
+			{
+				Abandon(outputTask, errorTask);
+
+				// Awaited rather than inspected so that the handler's exception, or a decode error
+				// from a strict encoding, reaches the caller as itself.
+				await finished.ConfigureAwait(false);
+			}
+
+			_ = pending.Remove(finished);
 		}
 
-		// 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;
 	}
 

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);
@@ -159,7 +159,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, CancellationToken cancellationToken)
 		=> await ExecuteAsync(command, new OutputHandler(), cancellationToken).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);
@@ -484,7 +484,21 @@
 			else
 			{
 				using AsyncProcessStreamReader outputReader = new(process, outputHandler);
-				await Task.WhenAll(outputReader.Start(cancellationToken), process.WaitForExitAsync(cancellationToken)).ConfigureAwait(false);
+
+				try
+				{
+					await outputReader.Start(cancellationToken).ConfigureAwait(false);
+				}
+				catch
+				{
+					// A failed read has stopped draining a pipe, so the command blocks once it fills it
+					// and would never exit on its own. Kill it so the failure can be reported rather
+					// than waited on, and so the command is not left behind blocked on the write.
+					TryKill(process);
+					throw;
+				}
+
+				await process.WaitForExitAsync(cancellationToken).ConfigureAwait(false);
 			}
 		}
 		catch (OperationCanceledException)