Throw on cancellation instead of returning a killed process's exit code - #42
Merged
Conversation
… 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
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #38.
Problem
RunAsyncdelivered cancellation two ways at once — a registration that killed the process, andWaitForExitAsync(cancellationToken)racing to observe the same token. Killing the process made it exit fast enough that the normal-exit path could win, in which case the method returned the killed process's exit code (-1on Windows) and threw nothing.A consumer mapping non-zero exit codes onto failures reported a cancelled command as a genuine failure of the underlying tool, with empty output and no way to tell the two apart.
Fix
Re-check the token after the wait and before returning, so both paths converge on
OperationCanceledException. Covers the elevated and redirected branches alike.Testing
ExecuteAsyncShouldThrowRatherThanReturnAnExitCodeWhenCancellationWinsTheRacecancels after 1 ms and repeats 50 times. A single attempt still throws roughly 60% of the time with the bug present, so one iteration cannot pin the race down; 50 makes a false pass vanishingly unlikely.Confirmed failing before the fix (attempt 1 returned an exit code) and passing after. Full suite: 25 passed, 2 skipped (elevated tests self-skip to avoid a UAC prompt).
🤖 Generated with Claude Code
https://claude.ai/code/session_01LwKTWcmxe5mh6DsLRj5NGQ