fix(agent): honor a cancel that lands while a turn is being set up - #3809
Open
haacked wants to merge 2 commits into
Open
fix(agent): honor a cancel that lands while a turn is being set up#3809haacked wants to merge 2 commits into
haacked wants to merge 2 commits into
Conversation
A cancel arriving after `prompt()` was called but before the prompt reached
the SDK was silently dropped. Setup can take seconds: `ensureLocalToolsConnected`
("pre-prompt") awaits an MCP status RPC bounded at 5s, and an unknown slash
command reloads skills first. Incoming messages are dispatched concurrently, so
a cancel in that window found an empty turn queue and no active turn.
`interrupt()` had nothing to settle and armed no force-cancel backstop, and
setup went on to hand the message to the SDK, which ran the turn to completion
and reported `end_turn` while tools kept executing.
`session.cancelSeq` counts cancels. `prompt()` snapshots it on entry, before any
await, and re-checks it once setup finishes: a higher count means the cancel
targets this prompt, so it returns `cancelled` without pushing to the SDK. That
is the last point where the turn can still be stopped, and no await separates
the check from the push. `session.cancelled` is left standing, because an
earlier turn may still be settling against it; activation clears it for the next
turn that runs.
Cancels landing outside that window are unchanged. Once a turn is queued
`interrupt()` sweeps it, and once it is active the existing backstop applies.
`cancel()` now assigns `interruptReason` on every call, including when the
client supplies none, so it always describes the current cancel instead of
reporting a stale reason left by an earlier one.
Raise the package's `engines.node` to match the root's Node 22 requirement,
which the new test's `Promise.withResolvers` depends on.
Claude-Session: https://claude.ai/code/session_01KT3ZQ47bqT9mW4fi458Q6V
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
React Doctor found no issues in the changed files. 🎉 Reviewed by React Doctor for commit |
Contributor
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 1 should fix, 0 consider. Published 1 finding (view the review). |
Contributor
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
The bail also required `session.cancelled`, which any turn's activation clears. A local-only command skips the pre-prompt status check the stalled prompt is parked in, so it can queue and activate in that window; the reset erased the very cancel the check exists to catch, and the cancelled prompt went to the SDK anyway. `cancelSeq` is monotonic and incremented only in `cancel()`, in the same synchronous block that sets `cancelled`, so comparing it against the entry snapshot survives an unrelated activation. The flag stays untouched here, because an earlier turn may still be settling against it. Claude-Session: https://claude.ai/code/session_01KT3ZQ47bqT9mW4fi458Q6V
haacked
marked this pull request as ready for review
July 25, 2026 05:26
Contributor
|
Reviews (1): Last reviewed commit: "fix(agent): decide the setup-window canc..." | Re-trigger Greptile |
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.
Problem
A cancel arriving while
prompt()was still setting up got silently dropped. That setup can run for seconds:ensureLocalToolsConnectedawaits an MCP status RPC bounded at 5s, and an unknown slash command reloads skills before that. Incoming messages are dispatched concurrently, so a cancel landing in that window found an empty turn queue and no active turn.interrupt()had nothing to settle and armed no force-cancel backstop, so setup went on to hand the message to the SDK. The turn then ran to completion and reportedend_turn, with tools still executing after the user pressed stop.Changes
session.cancelSeqcounts cancels on the session.prompt()snapshots it on entry before any await, then re-checks it once setup finishes. A higher count means the cancel targets this prompt, so it returnscancelledwithout pushing to the SDK. That is the last point where the turn can still be stopped, and no await separates the check from the push.The count is the whole test.
session.cancelledbelongs to the session, andactivateTurnclears it for whichever turn runs next, so a prompt that raced past the stalled one into the queue erases it — a local-only command such as/contextskips the pre-prompt status check entirely and can get there first. The count survives that. The flag itself is deliberately left standing on this path, because an earlier turn may still be settling against it.Cancels landing outside that window behave as before. Once a turn is queued
interrupt()sweeps it, and once it is active the existing backstop applies.cancel()now assignsinterruptReasonon every call, including when the client supplies none, so a turn no longer reports a stale reason left by an earlier cancel.The Codex adapter takes
cancelSeq: 0as an inert field. It resetscancelledsynchronously at prompt entry and refuses concurrent prompts, so it has no equivalent window.The package's
engines.nodemoves to>=22.19.0to match the root, which the new test'sPromise.withResolversrequires.One downstream effect worth knowing about: focusing a worktree broadcasts a
moving_to_worktreecancel to local sessions, and that cancel now registers in the setup window where it used to slip through. Autoresearch treats anycancelledas a user stop and pauses the run. This already happens on main whenever the cancel lands on an active turn, so the change makes it consistent rather than introducing it. Whether that case should be aninterruptedwith auto-recovery instead of apausedis a separate question inautoresearch.ts.How did you test this?
New test file covering the race, 6 cases. Neutralizing the early return fails 4 of them, so they guard the behavior rather than just describing it. The sixth case drives the concurrent interleaving: one prompt stalled in setup, a cancel, then a
/contextthat queues and activates in that window. Putting thesession.cancelledterm back into the check fails that case alone.Full
@posthog/agentsuite: 1648 tests across 98 files, all passing.pnpm typecheckclean.Automatic notifications