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 89b477e..818e08c 100644 --- a/RunCommand.Test/RunCommandTests.cs +++ b/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); + + /// + /// 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 0bc9b75..2440dc5 100644 --- a/RunCommand/RunCommand.cs +++ b/RunCommand/RunCommand.cs @@ -484,7 +484,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); } } catch (OperationCanceledException)