Skip to content

Deliver a cancellation to the runner that owns the run - #197

Merged
VeryComplexAndLongName merged 1 commit into
mainfrom
fix/cancel-routing-and-process-entries
Sep 3, 2026
Merged

Deliver a cancellation to the runner that owns the run#197
VeryComplexAndLongName merged 1 commit into
mainfrom
fix/cancel-routing-and-process-entries

Conversation

@VeryComplexAndLongName

Copy link
Copy Markdown
Owner

Task 6.5's human check found the previous fix did not work. A chain on copilot-cli-acp, Cancel pressed four times: four "cancel" entries appeared in Processes and hung, and the agent finished its work undisturbed.

Three defects. One of them mine.

1. The cancellation went to the wrong runner

The webview sends no agentId on a cancel — it does not know one:

transport.send({ kind: "cancel", cwd, runId: activeRunId, context: {...} });

So resolveRunner(command.agentId) fell through to DEFAULT_AGENT_ID and handed the cancel to claude-cli's runner. activeRuns is a closure per runner instance, so the run registered under copilot-cli-acp was invisible to it — the cancel reported "nothing to cancel" and the agent carried on.

Cancelling any agent other than claude-cli had never worked. The previous change only made the failure legible, by giving the report an attempted field. AiPanel now remembers which agent each runId was started against and routes the cancel there.

2. The hanging entries are a regression I introduced

Because the cancel took the ordinary-command path, trackHarnessProcess registered a WorkbenchProcess whose execute promise waits for completed/failed/cancelled. Before my change a cancel emitted cancelled and it settled; now it emits cancelling, which fell into default: return — so it never settled. Four presses, four permanent entries.

Fixed on both sides, because either alone leaves the other latent:

  • a cancel no longer registers a process at all — it is a signal about a run, not a run;
  • that switch lists every non-terminal kind by name instead of a bare default.

3. default: is where a protocol change goes unnoticed

Adding cancelling failed the build in the two exhaustive switches over Event["kind"], and said nothing here — because default handles everything, including what it has never heard of.

The two that broke were the two that were safe. Worth remembering the next time an event kind is added.

4. The chain path said nothing when it accepted a cancel

chainRunner.cancel() returning true simply returned, so between the click and the chain's own cancelled — which now arrives only once the process is gone — the panel showed nothing at all. It posts cancelling now, the same event the standalone host gets from asAgentRunner(), which this path bypasses.

Verification

Both new tests were checked by removing the fix and re-running, not by reasoning about them:

FAIL  never registers a process for a cancel command
FAIL  routes a cancel to the runner that owns the run, not to the default agent

They pass with the fix in place.

Also carries the acp-session-driver test that closes task 5.4, written in this working tree: it holds a fake child open after abort, asserts the terminal event stays pending, then emits close and gets cancelled. Verified it is a real test, not a checkmark.

Test plan

  • npm run typecheck — clean
  • npm run lint — clean
  • packages/extension — 29/29
  • New tests confirmed to fail without the fix
  • Only failing suite is git.push.test.ts, already tracked as core-test-worker-contention
  • CI green
  • Task 7.6, human: repeat on copilot-cli-acp — the process must stop, no "cancel" entry may appear, and the panel must say it is cancelling

🤖 Generated with Claude Code

Task 6.5's human check found the previous fix did not work. A chain on
copilot-cli-acp, Cancel pressed four times: four "cancel" entries
appeared in Processes and hung at a percentage, and the agent finished
its work undisturbed. Three defects, one of them mine.

The cancellation went to the wrong runner, so nothing was ever aborted.
The webview sends no agentId on a cancel - it does not know one - so
resolveRunner(command.agentId) fell through to DEFAULT_AGENT_ID and gave
the cancel to claude-cli's runner. activeRuns is a closure per runner
instance, so the run registered under copilot-cli-acp was invisible to
it. The cancel reported "nothing to cancel" and the agent carried on.

Cancelling any agent other than claude-cli had never worked. The previous
change only made the failure legible, by giving the report an `attempted`
field. AiPanel now remembers which agent each runId was started against
and routes the cancel there.

The hanging entries are a regression I introduced. Because the cancel
took the ordinary-command path, trackHarnessProcess registered a
WorkbenchProcess whose promise waits for completed/failed/cancelled.
Before, a cancel emitted `cancelled` and it settled; now it emits
`cancelling`, which fell into `default: return`, so it never settled.
Fixed on both sides, because either alone leaves the other latent: a
cancel no longer registers a process at all, and that switch now lists
every non-terminal kind by name.

Worth recording why the compiler missed it. Adding `cancelling` failed
the build in the two exhaustive switches over Event["kind"] and said
nothing here, because `default` handles everything including what it has
never heard of. The two that broke were the two that were safe.

And the extension's chain path emitted nothing when it accepted a cancel:
chainRunner.cancel() returning true simply returned, so between the click
and the chain's own `cancelled` - which now arrives only once the process
is gone - the panel showed nothing at all. It posts `cancelling` now, the
same event the standalone host gets from asAgentRunner(), which this path
bypasses.

Both new tests were verified by removing the fix and re-running, not by
reasoning about them. They fail without it and pass with it.

Also carries the acp-session-driver test that closes task 5.4, written in
this working tree: it holds a fake child open after abort, asserts the
terminal event stays pending, then emits close and gets `cancelled`.

Typecheck and lint clean; extension 29/29; the only failing suite is
git.push.test.ts, already tracked as core-test-worker-contention.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@VeryComplexAndLongName
VeryComplexAndLongName merged commit 499fbf6 into main Sep 3, 2026
7 checks passed
@VeryComplexAndLongName
VeryComplexAndLongName deleted the fix/cancel-routing-and-process-entries branch September 3, 2026 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant