From 82e6f7286dfb8f86f28029990a298f9f9c68b112 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 28 Sep 2026 22:25:49 +0000 Subject: [PATCH] Kill the command and rethrow when an output read fails [patch] A handler that throws, or a strict encoding that rejects the output, faulted one read loop. Nothing read that pipe any more, so the command blocked once it filled it, while ExecuteAsync waited on the other stream and on an exit that never came. The call hung and the exception never surfaced. The reader now throws the first failed read straight away, abandoning the other, and RunAsync kills the process tree before rethrowing. Fixes #89 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01TRHs5nFW38XRh3KGTHYjE6 --- CLAUDE.md | 5 ++ RunCommand.Test/RunCommandTests.cs | 78 ++++++++++++++++++++++++++ RunCommand/AsyncProcessStreamReader.cs | 36 +++++++++--- RunCommand/RunCommand.cs | 16 +++++- 4 files changed, 125 insertions(+), 10 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 31b11f3..ca78b82 100644 --- a/CLAUDE.md +++ b/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. diff --git a/RunCommand.Test/RunCommandTests.cs b/RunCommand.Test/RunCommandTests.cs index 3a34051..a545de8 100644 --- a/RunCommand.Test/RunCommandTests.cs +++ b/RunCommand.Test/RunCommandTests.cs @@ -971,6 +971,84 @@ public async Task ExecuteAsyncShouldDeliverOutputStillInThePipeWhenTheProcessExi } } + [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); + + /// + /// 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. + /// + /// + /// 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. + /// + 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 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(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); + } + } + + /// + /// Reports whether a process is still running, counting a zombie as gone since it has exited. + /// + 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; + } + } + /// /// Returns a command that prints a file's contents unchanged. /// diff --git a/RunCommand/AsyncProcessStreamReader.cs b/RunCommand/AsyncProcessStreamReader.cs index 9d0a2d4..e7d437e 100644 --- a/RunCommand/AsyncProcessStreamReader.cs +++ b/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) } /// - /// 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. /// + /// + /// 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. + /// /// /// 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); + List 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; } diff --git a/RunCommand/RunCommand.cs b/RunCommand/RunCommand.cs index e3da840..37854a0 100644 --- a/RunCommand/RunCommand.cs +++ b/RunCommand/RunCommand.cs @@ -482,7 +482,21 @@ private static async Task RunAsync(ProcessStartInfo startInfo, OutputHandle 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); } // Cancellation reaches the wait two ways at once: the registration above kills the process,