From fb2cbb59c73d50f587c25050a5048e0f7f3028ab Mon Sep 17 00:00:00 2001 From: Matt Edmondson Date: Wed, 19 Aug 2026 20:10:17 +1000 Subject: [PATCH] [patch] Throw on cancellation instead of returning a killed process's exit code Cancellation reached the wait two ways at once: the token registration killed the process, and WaitForExitAsync separately observed the token. Killing the process made it exit quickly enough that the normal-exit path could complete before the cancellation path faulted the await, so RunAsync fell through to return the killed process's exit code (-1 on Windows) and threw nothing. A caller mapping non-zero exit codes onto failures therefore reported a cancelled command as a genuine failure of the underlying tool, silently and non-deterministically. Re-check the token after the wait so both paths end the same way. The regression test cancels after 1ms and repeats 50 times: a single attempt still throws most of the time even with the bug present, so one iteration cannot pin the race down. Fixes #38 --- RunCommand.Test/RunCommandTests.cs | 24 +++++++++++++++++++++++- RunCommand/RunCommand.cs | 9 ++++++++- 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/RunCommand.Test/RunCommandTests.cs b/RunCommand.Test/RunCommandTests.cs index e3e265c..91f1721 100644 --- a/RunCommand.Test/RunCommandTests.cs +++ b/RunCommand.Test/RunCommandTests.cs @@ -1,4 +1,4 @@ -// Copyright (c) 2023-2026 ktsu-dev contributors +// Copyright (c) 2023-2026 ktsu-dev contributors namespace ktsu.RunCommand.Test; @@ -391,4 +391,26 @@ public async Task ExecuteAsyncShouldTerminateProcessWhenCancelledWhileRunning() // rather than merely abandoned. await Assert.ThrowsAsync(() => execution).ConfigureAwait(false); } + + [TestMethod] + public async Task ExecuteAsyncShouldThrowRatherThanReturnAnExitCodeWhenCancellationWinsTheRace() + { + (string fileName, string[] arguments) = GetSleepCommand(); + + // Cancelling this close to the start puts two paths in a near dead heat: the registration + // kills the process, and the kill makes it exit fast enough that the wait can observe a + // normal exit before it observes the token. Losing that race returns the killed process's + // exit code instead of throwing, so a caller cannot tell cancellation from real failure. + // A single attempt still throws most of the time, which is why this repeats: 50 attempts + // make a false pass vanishingly unlikely. + for (int attempt = 0; attempt < 50; attempt++) + { + using CancellationTokenSource cancellationTokenSource = new(); + cancellationTokenSource.CancelAfter(TimeSpan.FromMilliseconds(1)); + + await Assert.ThrowsAsync( + () => RunCommand.ExecuteAsync(fileName, arguments, new OutputHandler(), cancellationTokenSource.Token), + $"Attempt {attempt} returned an exit code instead of throwing.").ConfigureAwait(false); + } + } } diff --git a/RunCommand/RunCommand.cs b/RunCommand/RunCommand.cs index 18d7bb5..1947383 100644 --- a/RunCommand/RunCommand.cs +++ b/RunCommand/RunCommand.cs @@ -1,4 +1,4 @@ -// Copyright (c) 2023-2026 ktsu-dev contributors +// Copyright (c) 2023-2026 ktsu-dev contributors namespace ktsu.RunCommand; @@ -358,6 +358,13 @@ private static async Task RunAsync(ProcessStartInfo startInfo, OutputHandle await Task.WhenAll(outputReader.Start(), process.WaitForExitAsync(cancellationToken)).ConfigureAwait(false); } + // Cancellation reaches the wait two ways at once: the registration above kills the process, + // and the wait separately observes the token. The kill makes the process exit fast enough + // that the normal-exit path can win, which would return the killed process's exit code and + // throw nothing. Re-checking here makes both paths end the same way, so a cancelled call is + // never mistaken for a command that genuinely failed. + cancellationToken.ThrowIfCancellationRequested(); + return process.ExitCode; }