Say a run was cancelled when it stops, not when Cancel is pressed - #195
Merged
Merged
Conversation
Reported against a chain running GitHub Copilot CLI over ACP: Cancel was pressed, the panel said the run was cancelled, the agent carried on working, and the Cancel button was gone so there was no way to ask again. Two causes, both in the source. agent-runner.ts yielded `cancelled` immediately after calling abort(). abort() only fires listeners; the listener that kills the process is asynchronous and best-effort, and the yield neither waited for it nor checked it. It fired even when there was no active run to abort. The ACP driver did the same from the other side: on abort it returned `cancelled` and killed the process afterwards, in a finally. Both panels then compute isRunning as "no terminal event seen yet", which is sticky, and render Cancel only while isRunning. So the optimistic `cancelled` withdrew the only lever while the process kept editing files. The first cause is an inaccuracy; the second is a loss of control, and it is the one that made the situation unrecoverable. `cancelled` now comes from the process's own exit. shared.ts stops suppressing the child's close handler after an abort - that handler is the only thing that knows the process actually died - and a process that outlives the request produces a failed naming that rather than a cancelled that did not happen. The ACP driver defers its terminal event to runProcess, which owns the child and can see it go. A new non-terminal `cancelling` event carries what was attempted, so "asked the process to stop" and "nothing was running" are distinguishable by a caller. Non-terminal is the point: isTerminal does not include it, so isRunning stays true, the Cancel control stays, and a second press is accepted. The chain adapter emits it too - it previously returned an empty stream, leaving the panel no sign the click had landed on exactly the path the report came from. terminateProcessTree now reports whether the kill could be issued instead of swallowing the outcome, and treats POSIX ESRCH as success rather than failure: the process being already gone is what the caller wanted. The kill-confirmation wait is injectable. A ten-second constant a test cannot reach is a branch a test does not cover, and the survived-the-kill path is precisely the one worth covering. Three existing tests asserted the old behaviour and now assert the new one. The compiler found both hosts' exhaustive event switches, which is how "both hosts must handle it" stopped being an intention. Task 5.4 is left open and named: the ACP wait-for-exit path is the part this change adds for the adapter the report came from, and nothing yet exercises it - the existing suite drives an in-process peer with no child. Typecheck and lint clean; every suite green except git.push.test.ts, already tracked as core-test-worker-contention. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Reported against a chain running GitHub Copilot CLI over ACP: Cancel pressed → the panel said the run was cancelled → the agent carried on working → the Cancel button was gone, so there was no way to ask again.
Two causes
1.
cancelledwas a report of intent.agent-runner.ts:abort()only fires listeners. The listener that kills the process is asynchronous and best-effort, and theyieldneither waited for it nor checked it — it fired even when there was no active run at all. The ACP driver did the same from the other side: on abort it returnedcancelled, and killed the process afterwards in afinally.2. The panel then removed the only control. Both panels compute
isRunningas "no terminal event seen yet" — which is sticky — and render Cancel only whileisRunning.The first is an inaccuracy. The second is a loss of control, and it is what made the situation unrecoverable: even a kill that works nine times in ten leaves the tenth with an agent editing files and nothing to press.
The fix
cancellednow comes from the process's own exit.shared.tsstops suppressing the child'sclosehandler after an abort — that handler is the only thing that knows the process actually died. A process that outlives the request produces afailednaming that, rather than acancelledthat did not happen. The ACP driver defers its terminal event torunProcess, which owns the child and can see it go.A new non-terminal
cancellingevent carries what was attempted, so "asked the process to stop" and "nothing was running" are distinguishable. Non-terminal is the point:isTerminalexcludes it →isRunningstays true → the Cancel control stays and a second press is accepted. The chain adapter emits it too; it previously returned an empty stream, leaving the panel no sign the click had landed — on exactly the path this was reported from.terminateProcessTreereports whether the kill could be issued instead of swallowing the outcome, and treats POSIXESRCHas success: the process being already gone is what the caller wanted.The kill-confirmation wait is injectable. A ten-second constant a test cannot reach is a branch a test does not cover — and the survived-the-kill path is precisely the one worth covering.
Notes
Left open, and named
Task 5.4 — the ACP
runProcesswait-for-exit path is what this change adds for the adapter the report came from, and nothing yet exercises it: the existing suite drives an in-process ACP peer with no child, so it cannot reach the branch that waits on a realclose. Writing it needs a fake child the test can hold open, the same shapeshared.test.tsalready uses.Task 6.5 is human-only: reproduce the original report and confirm the process stops, the label does not say "Cancelled" while output arrives, and the control is still there if it did not work.
Test plan
npm run typecheck— cleannpm run lint— cleanagent-runner12/12,shared8/8,harness-chain-runner37/37,HarnessChainPanel9/9git.push.test.ts, already tracked ascore-test-worker-contentionand untouched here🤖 Generated with Claude Code