fix(agent-core-v2): settle killed subagent runs only after the child loop goes idle - #1759
Conversation
…loop goes idle - awaitRun: cancel by turn id so aborts also reach turns still queued, not just the active turn - awaitTurn: cancel first, then wait for turn.result instead of racing against the abort signal, so task settlement, the task.killed notification, and the resume guard never observe the run as finished while the loop is still unwinding - rethrow the original abort reason so consumers keep matching it by identity - add regression test for the manual stop -> auto-resume "already running" race
🦋 Changeset detectedLatest commit: 4572bb8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4572bb8a8f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| /** | ||
| * Await a turn's terminal result, cancelling it first when the run aborts. | ||
| * | ||
| * This deliberately does NOT race `turn.result` against the abort signal: | ||
| * the loop only goes idle once `runTurn` unwinds (`releaseActiveTurn`), and |
There was a problem hiding this comment.
packages/agent-core-v2/AGENTS.md says comments in this package must live solely in the top-of-file /** */ block and never beside functions, methods, or statements. This new helper-level JSDoc, along with the inline cancellation notes added in the same function, violates that package rule, so please fold the rationale into the file header or remove it.
Useful? React with 👍 / 👎.
…loop goes idle (MoonshotAI#1759) - awaitRun: cancel by turn id so aborts also reach turns still queued, not just the active turn - awaitTurn: cancel first, then wait for turn.result instead of racing against the abort signal, so task settlement, the task.killed notification, and the resume guard never observe the run as finished while the loop is still unwinding - rethrow the original abort reason so consumers keep matching it by identity - add regression test for the manual stop -> auto-resume "already running" race
1 participant
Related Issue
No linked issue — the problem is explained below.
Problem
Manually stopping a background subagent and resuming it right after (e.g. the main agent auto-resuming on the
task.killednotification) could fail with an "already running and cannot run concurrently" error. The killed task used to settle — and send its notification — the moment the abort landed, while the child agent loop was still unwinding (runTurnhad not yet released the active turn). The resume guard (ensureOwnedIdleSubagent, which readsloop.status().state) therefore still saw the loop as running and rejected the resume.What changed
In
packages/agent-core-v2/src/session/subagent/runAgentTurn.ts:awaitRunnow cancels by turn id (loop.cancel(turn.id, reason)) instead ofloop.cancel(undefined, …), so an abort also reaches this run's turn while it is still queued, not just the loop's currently active turn.awaitTurnno longer racesturn.resultagainst the abort signal. On abort it cancels the turn first, then waits for the turn's terminal result. The loop only goes idle oncerunTurnunwinds (releaseActiveTurn), so task settlement, thetask.killednotification, and the resume guard no longer observe the run as finished before that. A turn that never responds to the cancel stays bounded by the task layer's SIGTERM grace, not by an early rejection here.signal.throwIfAborted()) so consumers that match the reason by identity (isAbortError/error === sink.signal.reason) still tell a kill apart from a failure.abortPromisehelper.Regression coverage in
test/agent/task/idle-notification-repro.test.ts: a new "kill ordering vs child loop unwind" test with a child agent whose in-flight LLM call only rejects 200ms after the abort lands. It asserts thatstop()settles the task askilledonly after the child loop reportsidle, and that thetask.killednotification then reaches the main agent.Checklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.