fix(agent): honor a cancel that lands while a turn is being set up - #76472
fix(agent): honor a cancel that lands while a turn is being set up#76472charlesvien wants to merge 3 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
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
|
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 |
|
Reviews (1): Last reviewed commit: "fix(agent): decide the setup-window canc..." | Re-trigger Greptile |
|
@haacked this is your PostHog/code#3809 remade in the monorepo (main there is frozen after the desktop import). Your commits are intact, only paths moved. Can you review and approve if it looks good? |
|
Note 🤖 stamphog reviewed Gates denied on size/tier (297 lines/10 files, single-area fix) and a deny-list "auth" hit that's a false positive (only match is the word "authorship" in the PR description, not an auth-code change); the manifest-only Node engine bump has no scripts/lifecycle changes. The change itself is a well-tested concurrency fix in the desktop agent, but it has zero human reviews (only bot reactions), so it needs a human sign-off before merging given the gate denial.
Gate mechanics and policy version
|
|
@tatoalo can you review this one? haacked made it on the code repo (PostHog/code#3809) and I remade it here after the desktop import. |
2 participants
Ports PostHog/code#3809 into the monorepo (PostHog/code
mainis frozen after the desktop import; open PRs there are remade here). Commits preserve the original authorship.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 code?
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.After the port: from
products/desktop/,pnpm install --frozen-lockfile,pnpm typecheck(24/24 clean) and the full@posthog/agentsuite (1773 tests across 101 files, all passing, the new race file's 6 cases included).Automatic notifications
Docs update
N/A
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
This PR is a mechanical port of PostHog/code#3809 (author: haacked) into
products/desktop/, done with Claude Code following the repo's /porting-code-prs skill:git am -3 --directory=products/desktop/over the source patch series, preserving the original commits and authorship. It applied without conflicts; no source changes beyond the path relocation.Created with PostHog Code