Kill the process tree whenever a cancelled wait ends the call [patch] - #94
Merged
Merged
Conversation
RunAsync relied on a cancellation registration alone to kill the process. A token runs its callbacks newest first, and WaitForExitAsync registers its own after the kill's. The wait could therefore observe cancellation, end the call, and dispose the kill registration before that callback had run. The call returned OperationCanceledException with the whole process tree still running. The wait is now wrapped so that a cancelled wait calls TryKill itself before rethrowing. A second kill of a process that has already gone is a no-op. The new test cancels a command with a descendant 40 times through a linked timeout token, which cancels on a timer thread. It requires each descendant to have exited. It reads /proc so a reparented zombie counts as gone. Before the fix it failed in 3 of 5 runs locally; after the fix, 0 of 5. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TAvt7dvjcukkH3y6mtUL16
|
matt-edmondson
pushed a commit
that referenced
this pull request
Sep 29, 2026
Resolves the conflicts with #94 (kill on cancellation) and #97 (spin regression test). The read-fault kill now sits inside the else branch of #94's try/catch (OperationCanceledException), and the tests from both sides are kept. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TRHs5nFW38XRh3KGTHYjE6
matt-edmondson
pushed a commit
to ktsu-dev/GitBranchStateCache
that referenced
this pull request
Sep 29, 2026
RunCommand 1.9.1 kills the process tree whenever a cancelled wait ends the call (ktsu-dev/RunCommand#94). Before that, GitRunner's timeout could return with the tree still running, which failed RunAsync_ExceedingItsTimeout_ReportsTimedOutAndKillsTheTree on every CI leg. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TAvt7dvjcukkH3y6mtUL16
This was referenced Sep 29, 2026
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
2 participants
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.
A cancelled
ExecuteAsynccould return with the whole process tree still running.Cause
RunAsyncrelied oncancellationToken.Register(() => TryKill(process))alone to kill the process. ACancellationTokenSourceruns its callbacks newest first, andWaitForExitAsync(cancellationToken)registers its own callback after the kill's. So the wait can observe cancellation first. It then completes theTask.WhenAll, ends the call, and disposes the kill registration, all before the kill callback has run. Disposing a registration whose callback hasn't started removes it, so the kill is skipped or lands late.Fix
The wait is now wrapped in a
try. OnOperationCanceledExceptionit callsTryKill(process)before rethrowing, so the kill is issued before the call returns.TryKillalready checksHasExited, so a second kill is a no-op.Where it showed up
GitBranchStateCache#51 moves
GitRunnerontoRunCommand.ExecuteAsync. There,RunAsync_ExceedingItsTimeout_ReportsTimedOutAndKillsTheTreefailed on the ubuntu and windows CI legs.shand itssleepwere both still alive after the timeout.I ran that test locally with GitBranchStateCache pointed at this project:
main: 3 of 8 runs failedTest
ExecuteAsyncShouldKillTheProcessTreeEveryTimeItIsCancelledrunssh -c 'sleep 30 & echo $!; wait'40 times. Each time it cancels through a 200 ms timeout linked into the token, which is the GitRunner shape and cancels on a timer thread. It then requires eachsleepto have exited within 5 s./proc/<pid>/statrather than usingProcess.HasExited. Once its parent is killed, thesleepis reparented, and it can sit as a zombie until its new parent reaps it. A zombie has exited, so it counts as gone. I found this matters: counting zombies as alive makes the check fail with or without the fix.main, 3 of 5 runs failed. With the fix, 0 of 5 failed. The test takes about 10 s.Assert.Inconclusiveelsewhere, matching the neighbouring descendant test). The code it covers is platform independent.Full suite: 52 passed, 2 skipped (the Windows-only elevation self-skips).
🤖 Generated with Claude Code
https://claude.ai/code/session_01TAvt7dvjcukkH3y6mtUL16
Generated by Claude Code