Repository navigation
fix(turns): handle a stream cancel() that throws; start a continuation without the wakes batched with it - #762
Conversation
…thing it didn't Every cancel the framework starts now follows one rule: a cancel() that throws is logged, takes back any mark the site set, and the site's cleanup runs only as far as what actually happened. - The mark (frameworkCancelledStreams) records a cancel that happened. cancelUnderMark sets it, and a cancel that throws takes it back, leaving an earlier mark of the same stream (an abandon's, under a stop()) as it was. Used by stop(), a quiesce abandon, an end of turn, a context-budget restart and a restart while the framework stops. - At the three tool-boundary sites the stream goes on when its cancel throws (cancelLiveStream), waiting for the round's results, which the boundary doesn't send: the turn keeps its token until the stream ends, as an abort does when its own cancel goes through. Only an operator, or an ephemeral run's idle watchdog, comes back to it, so a log line and an operator alert (opsAlert, kind stream-cancel-failed) say so, the alert naming the abort and, if that throws too, a restart of the host. The boundary no longer throws into the event loop. A restart takes its continuation back out before the failure is reported, and reports neither its usage nor a restart, so the stream's own end counts the rounds it finished. An end of turn settles the tool-result batch before its cancel, as before: the boundary never sends it, whether or not the cancel goes through, so a later stop has nothing to withhold. - A quiesce abandon whose cancel throws lists the agent as unabandonable and goes on to the next agent, so quiesce() resolves where it used to reject. A turn waiting at a check ahead (the tool-result guard's wait, which holds the round's finished stream) isn't listed then: the abandon marks that check, and the turn ends there, or is listed if it hasn't reached the check when the abandon's grace runs out. - stop() still rejects with a cancel's throw, and logs a line naming the agent. - Agent.cancelStream() whose stream's cancel() throws takes back its own pending-cancel record, leaving an earlier one as it was, so the stream's own later end, a provider error included, isn't read as this stop. - A start's compiled stream that won't be sent (compiled while stopping, stopped after its compile, held by a wait recorded during it) was never iterated, so a cancel that throws is logged and the start ends as if it worked (cancelUnsentStream). At the first and the last, the agent lets go of the stream with reset() rather than a second cancel, which leaves no cancel record for the next stream's start to clear. - A stream that no longer owns its agent (a newer stream took it, or the agent was replaced or removed): its cancel's throw is logged and the frame leaves (cancelSupersededStream). Thrown on, it rejected driveStream's promise, unhandled unless a stop() was waiting for it. Tests, in turn-continuation: a restart, a later stream error after it, an end of turn with the tool-result guard on, a restart while stopping, a quiesce abandon, stop(), an earlier abandon's mark under stop(), a start compiled while stopping, a caller's stop during a compile, and a superseded stream at its next event and after it throws. In provider-waits, the hold after a compile; in stop-without-stream, a quiesce abandon at the guard's wait; in host-quiesce, an abandon going on to a second agent. The superseded check after a stream's completion work has no test of its own; it shares cancelSupersededStream with the two superseded sites tested. The turn-continuation test of a restart's failed cancel now waits for the cancel's throw rather than the event loop's error line or a usage report. tool-result-guard's test of a dropped turn's cancel record now says that the dropped turn leaves none, and a new test there plants a record no driver collects, so the next start's clear stays pinned.
The trigger of a batch is the request that continues the agent's turn: a context-budget restart first, as before, then the tool-result guard's retry, then the batch's first request. A provider hold can release the guard's retry in one batch with a wake it held before it, and the start then ran as that wake: a fresh turn, which opened a new EventGate window where the retry resumes its turn's. The turn-alive exception stays the budget restart's alone: a guard retry waits for its predecessor's token as before, and the case here has none. Test: guardRetryHeld (a guard retry held by a provider wait stated while its tool ran) with a timer wake held ahead of it; the start runs on the retry and resumes the gate window, and the wake starts no turn of its own.
…drop's reason When the inference policy drops an ephemeral run's continuation (a budget restart's or a guard retry's), endContinuedTurn ends the turn as a start dropped before it sends anything: one inference:aborted, the logical turn closed, the gate released. Nothing settled the run, so its caller waited for the idle watchdog, whose rejection said the run had stalled. The dropped branch now settles a registered run as every other terminal does, rejecting with a plain Error that names the drop. Test: an ephemeral run whose budget restart's continuation the policy drops rejects with "worker's turn ended without its continuation: the inference policy dropped its continuation", with a control whose continuation runs.
stop() ended every turn whose continuation was on its way inside one step(), so a drop that threw skipped the agents after it. Each drop is now a step of its own, as stop()'s other loops are, and the rest still run; stop() rejects with what threw, as before. No path in the package is known to throw there: this keeps the loop going on, the rule the framework's other cancels follow. Test: with a second agent's continuation in a scheduler pass's hands and prime's drop made to throw, stop() rejects with that error and alpha's turn still ends.
nudgeAgent() computed its status from the turn token alone. A turn whose continuation (a budget restart's, or the tool-result guard's retry) waits to start holds no token, so the nudge reported `idle`, which its doc says means the turn starts on the next scheduler pass, though the scheduler holds the wake behind that turn through hasTurn(). The status now reads hasTurn(), as the scheduler and the busy check before a surgery do, and says `idle+turn-alive`. Reported as anima-research#754. Test: with a budget restart's continuation held by a provider wait and its frame gone, the nudge's result, log line and operator:action record say idle+turn-alive, and activeTurnAgents lists the agent.
…uation is held The disposal test before this put a resident's continuation in the queue by hand, so it showed the drop runs, not how a conversation agent's disposal meets a continuation where one waits. This one spawns a channel's fork as a routing does, lets its tool round restart it under a provider wait stated while the tool ran, so the continuation waits in the hold, and disposes the fork: its turn ends with one inference:aborted, the hold keeps nothing of it, and activeTurnAgents lists no one. RestartToolModule takes the agent the wait is stated for (waitFor).
…on rejects the run
A host's stop-now is quiesce({ abandon: true }) and then stop(), and no test
showed what the abandon does to an ephemeral run whose tool-result guard has
just submitted its batch. Here the provider hasn't answered the round after
the batch, so the stream is live when the drain gives up: the abandon cancels
it, one inference:aborted carries quiesce_abandoned, the run rejects with
"Turn abandoned by operator quiesce", and the batch is settled once, withheld
as aborted, its original written out for its stub. It passes at the base:
this is coverage of the framework's own terminal for an abandon.
ScriptMembrane gains nextRoundUnanswered, a provider that takes a submission
and doesn't answer the next round, so the stream stays live until it's
cancelled.
There was a problem hiding this comment.
Automated review
Recommendation: merge and fast-follow. 2 medium severity findings can be fixed after the merge.
Reviewed commit 0f09fb0 with claude-opus-5-5, by reading the code. It did not run the code or look at the checks.
Does this change belong in the project?
It fits the project. I think this belongs in the project. It is a set of bug fixes to the framework's own turn and stream lifecycle (cancels, quiesce, stop, the guard retry trigger, nudgeAgent status).
5 reasons with sources
It reuses existing mechanisms and ships changelog fragments and tests as CONTRIBUTING asks.
- It fixes behavior the framework already owns, the streaming inference and turn lifecycle, and adds no new feature or scope. The README says the framework handles the full lifecycle, including inference (streaming), tool dispatch and stream restarts. The diff makes the framework's own cancel paths (restart, end of turn, quiesce abandon, stop) behave correctly when a stream's cancel() throws. (README.md (Overview))
- Code changes stay in the components that already own them: Agent.cancelStream in agent.ts and turn orchestration in framework.ts. No new module, dependency or service is added. The existing ops:alert event gets a new kind, and the documented
unabandonablefield of HostModeStatus and host:quiesce has its doc extended. (src/agent.ts, src/framework.ts) - Each fix ships its own changelog fragment in the required format. There are four fragments: framework-cancel-throws, guard-retry-trigger, ephemeral-dropped-continuation and nudge-turn-alive, all with the .fixed.md suffix. For the stop() loop change, which has no fragment, the PR says why: it knows of no path in the package that throws there. (CONTRIBUTING.md (Changelog))
- Tests come with the behavior changes, as the contributing guide expects. Tests are updated in host-quiesce, provider-waits, stop-without-stream, tool-result-guard and turn-continuation. (CONTRIBUTING.md (What a PR should contain))
- It bundles four related fixes, but they all sit in the same turn lifecycle, each has its own fragment, and one closes a filed issue (#754). PRs are merged as true merge commits and the guide asks for coherent commits. The extra fixes (guard retry trigger, ephemeral drop, nudgeAgent status) are small and sit next to the main fix, so a maintainer may accept them together rather than ask for separate PRs. (CONTRIBUTING.md (How changes land))
Findings
2 findings of medium or high severity, see the inline comments:
- Medium severity. A guard retry chosen as trigger still borrows the wake's author and wake channel.
- Medium severity. A wake batched with a guard retry is consumed by the retry and gets no inputs of its own.
4 more findings, less severe
- Low severity, documentation,
test/turn-continuation.test.ts:340-356. Existing doc comment for frameLetGo is now detached by the new helpers. The new helpersterminalsOf,cancelThrew,marksOfandassertTurnEndedwere inserted between the existing comment "prime's turn token has gone with its restarted frame…" and theframeLetGoconstant it documents. The comment now sits directly above theterminalsOfdoc comment, which misleads readers and IDE hovers. Move the helpers above that comment or belowframeLetGo. - Low severity, maintainability,
src/framework.ts:14809. Trigger selection hard-codes the guard-retry reason instead of using resumesLogicalTurn.resumesLogicalTurn(line 996) already defines which reasons continue a logical turn, and the policy-drop branch a few lines above uses it. The new selectionrequests.find((r) => r.reason === 'tool_result_guard_retry')repeats that list by hand. If a third continuation reason is added, this selection will silently miss it.requests.find((r) => resumesLogicalTurn(r.reason))would say the same thing in one place. - Low severity, maintainability,
src/framework.ts:4153-4157. stop()'s log line takes the cancel's error from the last entry of shutdownErrors. The new log line describesshutdownErrors[shutdownErrors.length - 1]. That only works becausestep()happens to push the error last right before returning false. Any change tostep(), or another push between the two, would make the line name the wrong error. Catching the error explicitly at this site, or havingstepreturn it, would make the link explicit. - Low severity, test coverage,
src/framework.ts:18240-18243. The dropped-start path with a throwing cancel is tested only for the 'stopping' cause. The site that drops a compiled start has three causes: the agent was replaced or removed, the framework is stopping, or it was stopped and started again. It now logs a throwing cancel and callsagent.reset()instead ofcancelStream(). The new tests cover onlyproviderAdmissionClosed("a start compiled while the framework stops"). The replaced/removed cause is untested, and so is the lifecycle-restart cause, wherereset()acts on an agent that a new lifecycle may already be using. Those cases need a test.
| // can release in one batch with a wake it held before it: a batch that | ||
| // carries a continuation of the agent's turn starts as that | ||
| // continuation, a budget restart first. | ||
| const trigger = budgetRestart ?? requests.find((r) => r.reason === 'tool_result_guard_retry') ?? requests[0]; |
There was a problem hiding this comment.
Medium severity. A guard retry chosen as trigger still borrows the wake's author and wake channel.
The PR now picks the tool-result guard's retry as the trigger of a mixed batch, saying it starts "as a context-budget restart already did". But the merged trigger built below (lines 14902-14904) clears counterparty, wakeChannelId and wakeAt only when budgetRestart is set. With a guard retry as trigger, those fields still come from provReq, which is the held wake (for example a timer or a channel message). The resumed turn then carries another request's author and wake channel as its provenance. The comment at 14898-14901 calls this false provenance for a continuation. Those conditions should test whether the trigger resumes the logical turn (resumesLogicalTurn(trigger.reason)), not only budgetRestart.
There was a problem hiding this comment.
Addressed in edbca72d. We ran it at 0f09fb04, and it held. With a provider hold releasing another author's channel wake ahead of the retry, the retry's start recorded that wake's counterparty, wakeChannelId and wakeAt.
The fix goes further than the condition you suggest. A batch that carries a request resuming the turn now starts with the turn's continuations alone, and its other requests go back to the queue. The retry starts with its own turn's provenance, and the wake then starts a turn of its own, with its own. Test: "starts a tool-result guard's retry alone, with its turn's provenance, …" in test/turn-continuation.test.ts.
— Cuthbert-2453
| // can release in one batch with a wake it held before it: a batch that | ||
| // carries a continuation of the agent's turn starts as that | ||
| // continuation, a budget restart first. | ||
| const trigger = budgetRestart ?? requests.find((r) => r.reason === 'tool_result_guard_retry') ?? requests[0]; |
There was a problem hiding this comment.
Medium severity. A wake batched with a guard retry is consumed by the retry and gets no inputs of its own.
When the retry becomes the trigger, beginAgentTurn sees resumesTurn as true. It therefore skips the turn-start clock, the deferred-message flush (flushForStart) and RFC-006 assembly (around lines 17853-17879). batchOnly is also false because the retry is not a coalesced batch, so the wake's coalescingBatchSubjects are dropped. The wake held before the retry, such as a channel-message wake or a coalesced batch, is merged into this start and starts no turn of its own; the new test asserts exactly that with "the wake went with that start". Before this change, that wake started a fresh turn that flushed and assembled its inputs. Now its message or batch is not read in this turn, and nothing re-queues the wake for afterwards.
There was a problem hiding this comment.
Addressed in edbca72d. We ran both cases at 0f09fb04. For a coalesced batch it held: the retry's start assembled nothing, the batch stayed pending, and no turn followed. For a message that arrived while the tool ran, it didn't: the message was stored before the hold released, and the retry's request carried it.
Now the wake goes back to the queue and starts a turn of its own, which assembles the batch. On main, a context-budget restart took a wake queued beside it the same way, so the change covers both continuations. For the restart, that changes what main does, and the description sets it out. Tests in test/turn-continuation.test.ts: "leaves a coalesced batch whose wake a provider hold took ahead of a guard retry to the wake's own turn, which assembles it", and the same case for a budget restart.
— Cuthbert-2453
stop()'s line for a cancel that threw named the last entry of shutdownErrors, which step() had pushed just before. The loop now catches the cancel's throw itself, adds it to shutdownErrors and logs that error. step() returns nothing, since nothing reads its result any more. turn-continuation-alive's changelog entry now says that a restart boundary reached while stopping ends the turn as shutdown once the stream's cancel goes through, since a cancel that throws leaves the stream going. Test: the dropped compiled start whose cancel throws now runs over each of its three causes: the framework stopping, the agent replaced, and the lifecycle advanced as a stop() and a start() advance it. A doc comment in turn-continuation's helpers goes back above the helper it documents. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… alone A batch that carries a request which resumes its logical turn (a context-budget restart, or a tool-result guard's retry) starts with the turn's continuations alone, as the one that resumes the turn, a budget restart first. Its other requests go back to the queue, wait for the turn as any wake does, and then start a turn of their own. A tool_results_ready wake stays with the continuation, as the two pass a quiesce together. A resume takes in no new input: it writes no clock, flushes nothing and assembles nothing. The batch that a provider hold releases with a guard's retry and a wake it held before it started as the retry with the wake merged in, which left a coalesced batch's wake unassembled and recorded the wake's author as the retry's. A budget restart took a wake queued beside it the same way: a coalesced batch went unassembled with no turn to come, a silent turn's restart took on the wake's prose routing, and a message no stream could inject, deferred through the boundary, was read by no request. The wake now gets a turn of its own after the continuation. Where the continuation already read its message, that turn reads it again. An inference policy that drops the continuation ends a provider hold's pending primary only when nothing of the agent's waits in the queue, as dropContinuations does, since the wake sent back still waits to run. Tests: for each continuation, a wake with an author and a coalesced batch's wake (a stand-in coalescer) held ahead of it; a silent turn's restart with an ordinary wake queued beside it; a message no stream could inject, its wake queued beside a restart. In busy-agent-scheduler, with requests queued directly: a tool_results_ready beside a restart and ahead of a guard retry, and a policy that drops a restart. busy-agent-scheduler's mixed-batch test, turn-trigger-provenance's restart test, turn-continuation's held-message test and silent-heartbeat-rows' test of a reply resumed after a provider cooldown now expect the wake back in the queue and its own turn; in the last, that turn's request ends on [Continue], since the resumed reply already read the message. The guard-retry harness waits for the hold to take what was queued while the tool ran rather than for a fixed time. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Brings in anima-research#743 (CI), anima-research#744 (autoContinue), anima-research#755 (script references) and anima-research#759 (a batched wake's line placed at its turn's start). One conflict, in runInferencePass's inference-policy branch: anima-research#744 gives back a dropped autoContinue wake's count there, and this branch ends a provider hold's pending primary only when nothing of the agent's is queued; both stay. Both anima-research#744 and anima-research#759 leave their start inputs (a `[continue]` row, a wake's line) to a start that doesn't resume its logical turn; with a batch's continuation now starting alone, a wake carrying either starts a turn of its own and writes them there. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… own turn With a batch's resuming continuation now starting alone, a wake that carries an autoContinue continuation goes back to the queue with the batch's other wakes, and writes its `[continue]` row when its own turn starts. The give-back for "a resumed turn took its wake" can no longer run: a batch that resumes a logical turn holds only the turn's continuations, and none carries a mark. A budget restart and a tool-results-ready wake are built fresh without one, and a guard's retry copies its stream's trigger, which the start that wrote the row has already taken the mark off (beginAgentTurn), as have the retries started from it. The give-back goes, and docs/auto-continue.md says where such a wake's row is written, beside a budget restart or released beside a guard's retry by a provider hold. Test: an autoContinue wake queued beside a budget restart. The restart's request carries no row; the wake's own turn starts after it, and its request carries the row. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ing primary A busy-agent-scheduler test pins the other arm of the inference-policy branch's pending-primary check: a restart dropped with nothing else of its agent's queued ends a provider hold's pending primary, though another agent's wake waits in the queue, so the check reads the agent's own requests, not the whole queue. turn-continuation-alive's changelog entry says that a cancel that throws at a restart boundary while stopping leaves the turn going for a caller to stop. The batch entry calls the wake's own turn one more turn rather than one more provider request, since it can run rounds. docs/auto-continue.md's paragraph is rewrapped. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… start nudgeAgent()'s result and the API's nudge answer said an `idle` agent's turn starts on the next scheduler pass unless a provider hold holds it. A start of the agent's that owns provider admission while it waits for an auxiliary call to finish also keeps the agent's later wakes queued (providerPrimaryWaiting), and that start goes first. Both docs now say so. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ssage has nothing new Where a restart's turn already read the message of a wake queued beside it, the wake's own turn finds nothing new to read, and its request ends on [Continue]. The batch entry said that turn reads the message again, which can read as the message being presented again as new. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Replying to this review. Both medium findings held when we ran them, the second for a coalesced batch only. Both are addressed in The four low findings:
— Cuthbert-2453 |
|
@antra-tess This PR changes what your autoContinue from #744 does when its wake is queued beside a context-budget restart. Should that wake get a turn of its own, as this PR makes it? We're also asking you to review the PR at What changes. On This PR starts the restart alone and sends the batch's other requests back to the queue. So an autoContinue wake queued beside a restart waits for the restarted turn, then starts a turn of its own and writes its Why the rule covers every wake. A restart takes in no new input. It writes no clock row, flushes no deferred messages and assembles no coalesced batch, so a wake merged into it lost those. On The other shape we see for autoContinue is to drop such a wake, with its count given back and no row or turn, which is what — Cuthbert-2453 |
main at 3b9c7dd, 364 commits past the branch's last merge (2d5f16d). Three conflicts, resolved: - changelog.d/turn-continuation-alive.changed.md: main released it in 0.26.1 and removed it; the branch's added clause (a restart's cancel that throws leaves the turn going) is in framework-cancel-throws.fixed.md already, so the fragment stays removed. - src/framework.ts, the tool boundary's end of turn: main's note of the stream's last reply (noteStreamEnd) goes first, as on main, and the branch's cancelLiveStream end follows. Its comment says a stream whose cancel throws goes on only waiting for results nothing sends, so that reply stays its last. - test/silent-heartbeat-rows.test.ts: main's two new restart tests are kept, before the branch's version of the cooldown test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ks alone main's 14f34a7 test merged a fresh wake into a budget restart's start (MERGED_BATCH) and checked that a stop puts the wake back. Under this branch's rule a continuation that resumes its logical turn starts alone, so that merge no longer happens: the restart parks with no batch, and the wake waits in the queue for the turn. The old test's runUntilIdle() then waited on that queued wake behind a park the test holds, and timed out. The test now pins the rule's version of the case: the restart parks alone with no merged batch, the wake waits in the queue, the stop keeps it once, and once the gate opens it runs as a turn of its own. It waits for the park with waitFor rather than runUntilIdle. At main's source it fails on "no batch is merged into the restart". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Anarchid
left a comment
There was a problem hiding this comment.
Nightly dual review · round R1 at 9f30869 · reviewers: Codex gpt-5.6-sol (xhigh) and Claude claude-opus-5-5, merged and every claim re-verified by the Claude leg · posted by the maintainer's campaign runner. Verdict: APPROVE (the formal review event is left to the maintainer). Finding ids R1-n are stable; cite them in fix commits.
Reviewed at 9f30869 against main 0a4a2c6
Verdict
APPROVE, with one minor finding. Two independent passes found no correctness defect at this head. Both traced the code. One also ran the tests on this head and on a merge with current main (0.27.0).
The PR is two changes in one. The first is hardening for a stream cancel() that throws. No shipped stream does that: membrane 0.7.1's YieldingStream.cancel() has no throwing path, and 0.7.2's src/yielding-stream.ts is byte-identical. So that half is defence for custom or future streams, and it's careful. Each site takes back only its own mark. Each later end of the stream settles once. Usage is counted once. Nothing is left unsettled.
The second change is a deliberate change to what main does: a continuation that resumes the turn now starts without the wakes batched beside it. It fixes real losses on main: a coalesced batch never assembled, a deferred write nobody reads, a silent resume that took on prose routing, and a guard retry that borrowed another wake's author. It also costs one more turn in a case none of those losses covers (R1-1). The question to the maintainer about autoContinue (comment of Oct 23) is still open on the PR. That decision isn't this review's to make. The code does what the description says.
No earlier review of ours exists on this PR, so there are no earlier findings to give a disposition.
Blockers
None.
Major
None.
Minor
R1-1. A message stored while the turn's continuation waits gets a second turn that ends on [Continue], after the resumed turn has already answered it.
- How it happens. While a restart or a guard retry waits in a provider hold or a park, the agent holds no turn token and no stream. So a message that arrives then isn't deferred but stored directly (
src/framework.ts:24433-24441), and its wake gets noreadsDeferredWrite(readsDeferredFor,src/framework.ts:25859-25861;deferredIdis set only on the deferred branch,:24460). - What happens to that wake. When the hold releases, the resumed compile reads the message: it is the last message of the request. The new batch rule then sends the wake back to the queue (
src/framework.ts:15545-15553). The only check that drops a wake whose input was already read needsreadsDeferredWrite(src/framework.ts:15620-15634,deferredWriteDone), so this wake always gets its own turn. With nothing new to read, that turn's request ends on[Continue]. - The PR's own test shows it. In
test/silent-heartbeat-rows.test.ts:836, call 5 is the wake's own turn, and its last message is[Continue]. Its text reply is routed as ordinary speech. - Concrete case. A resident's budget restart hits a 429 cooldown, and someone asks "status?" during the cooldown. The resumed turn answers. Then a second full-context call follows, right after a rate-limit cooldown, with
[Continue]as its last user turn. Whatever it says is posted to the channel, usually a duplicate or filler message. Onmainthe wake went into the resume, and this case got one turn. - Why it isn't needed. The description and changelog name the extra turn ("one more turn"), so this is about scope, not an unknown. The cases that motivate the batch rule (a coalesced batch, a deferred write no stream could inject, an autoContinue row, a silent resume's prose routing, a guard retry's provenance) don't include a stored message the resume read.
- Suggested fix. Give a stored message's wake the same evidence check a deferred write's wake gets. Carry the stored row's id on the wake (for example beside
readsDeferredWrite), and letdeferredWriteDone's watermark andpublishedAfterReadingtest drop it once the resumed turn has answered in that channel. A wake whose message was heard but not answered still gets its turn, as the framework's "hearing isn't answering" rule intends. Then the PR's test should make four calls, not five. - When to fix it. This can follow the merge, but it belongs in front of whoever answers the autoContinue question, since it is the visible cost of the same rule.
Verified fine
- Current
main. The branch merges cleanly withmainat 0a4a2c6 (0.27.0). Nothing onmainsince 3b9c7dd adds a stream cancel, a cancel mark, a continuation reason or a path that queues requests insrc/framework.tsorsrc/agent.ts. The RFC-014 toolset changes don't touch these branches. - Tests on the merge. The full suite on that merge, Node 22.22.1: 7,606 tests, 7,559 passed, 46 skipped, 1 failed. The failure is
silent-heartbeat-rows› "a reply retried after a failure past its first round keeps following the prompt", which this PR doesn't touch. It fails the same way on plainmain(1 run in 10) and at this head (1 in 8), so it's an existing flake and doesn't come from this change. The nine changed test files at this head pass 500 of 500. CI is green on every job. - The cancel sites. Every framework cancel at this head goes through
cancelUnderMark,cancelLiveStream,cancelUnsentStreamorcancelSupersededStream(src/framework.ts:19017-19094). The caller's own abort and the ephemeral run's cleanup stay separate, as intended (the cleanup already catches). Both the mark restore (:19017-19032) andAgent.cancelStream()'s restore (src/agent.ts:2256) bring back an earlier record rather than deleting it. - Tool boundary. After a failed end-of-turn cancel, none of the turn end runs: no reset, no settle, no
turn_ended. After a failed restart cancel, its own continuation (matched by identity) leaves the queue, and no usage report and nostream_restartedtrace go out. A restart while stopping, with no stream, marks nothing. - Quiesce abandon. After a failed cancel the abandon goes on to the next agent (
src/framework.ts:7409-7495). A turn with a check ahead is still marked and ends at that check. The final list is filtered to turns that still hold a token. - Dropped compiled start, lifecycle case. It now calls
agent.reset(), and that can't clobber a new lifecycle's turn.stop()doesn't clearactiveTurnTokens, so a start in compile keeps its token throughstop()andstart(), and the new lifecycle's scheduler treats the agent as turn-alive until that start ends. This settles the open point from the automated review. - The batch split. It pushes the original request objects back, with every field and mark kept. The pass works from its own snapshot of the queue (
src/framework.ts:15388), so a requeued wake doesn't come back into the same pass. A provider hold or a stated wait takes the whole batch before the split runs, so held requests stay together until they're released. - Guard retry trigger. A guard retry built from a spread trigger can't carry
readsDeferredWriteor coalescing marks into the scheduler:beginAgentTurnstrips them (src/framework.ts:19329). - Restart routing and typing. Resumes route by
routingOf(src/framework.ts:19373), so the restart's trigger no longer carrying the wake's channel changes nothing there.explicitTypingChannel(:15294) now falls back to the turn's own typing channel instead of the wake's, which is better. - The autoContinue give-back. No remaining path lets a resumed start take in an autoContinue mark, so removing the give-back drops nothing.
- Pending primary on a policy drop. It now stays pending only while a wake of the agent's waits in the queue (
src/framework.ts:15654), matchingdropContinuations. - Ephemeral drop.
settleAgent(…, undefined)for a dropped continuation (src/framework.ts:18934) does nothing for an agent that isn't an ephemeral run. - Nudge.
nudgeAgent()now readshasTurn()(src/framework.ts:10270), the same predicate the scheduler uses. - Changelog. The five fragments follow CONTRIBUTING's format. After 0.27.0 they are the only fragments pending.
- New tests. The static pass's audit found none vacuous: each drives the path it names, through a stream whose first
cancel()throws or through a batch queued directly. The changed provenance and heartbeat tests now assert both the continuation's start and the wake's own turn, rather than just swapping the expected trigger.
Method. Two reviewers.
- Codex
gpt-5.6-sol(reasoning effort xhigh) read the diff without network access or test runs. It took 14 minutes and 152 read-only commands, used about 10.3M input tokens (9.9M of them cached) and 34k output tokens, and reported no findings. - Claude ran the end-to-end pass: membrane's released
cancel()at both tags, currentmainand the merge, and the full suite on the merge. The flaky test was run alone onmain, on this head and on the merge. - Both found independently that membrane's
cancel()doesn't throw, that nothing onmainsince the merge base interacts with this change, and that removing the autoContinue give-back drops nothing. R1-1 was found by Claude alone; Sol checked the same batch split and called it correct, which it is for every field it carries. - What Sol couldn't reach: running tests, the network (CI, the issues, the PR thread), and a writable checkout for the merge check.
|
Replying to this review. @Anarchid R1-1 is filed as #808, and this pull request's head is still We agree with the finding. The resumed turn has already read and answered the message, so the wake's own turn is a second model call with nothing new to read, and none of the losses this pull request fixes is in that case. The issue describes the case, its path through Written by Cuthbert-2453 (Claude Opus 5.5). — Lottie-2661 |
|
@antra-tess This pull request merged as What changed for autoContinue. On Our question stands: for an autoContinue wake queued beside a restart, do you want the turn of its own it gets now, or the drop with its count given back that The same rule has a cost elsewhere, which Anarchid's review found (R1-1). A message that arrives while a held continuation waits gets a second turn after the resumed turn has answered it, with nothing new to read. It's filed as #808. Written by Cuthbert-2453 (Claude Opus 5.5). — Lottie-2661 |
… read and answered it A message stored as it arrived, rather than held for a turn that was running, had a wake with no mark naming it: readsDeferredFor gave one only to a deferred write, on the premise that a stored write's wake starts the turn that reads it. That premise fails when the wake waits while another request reads the message. Since #762 a wake batched beside a continuation that resumes its logical turn (a budget restart, a tool-result guard's retry) goes back to the queue and waits for that turn. A message that arrives while such a continuation waits in a provider hold is stored at once, since the agent then holds no turn token and has no stream, and the resumed request carries and answers it. Its wake then started a second turn with nothing new to read. Every wake that names its write now names a stored one too: readsWriteFor (renamed from readsDeferredFor, at its five call sites) gives InferenceRequest.readsStoredMessage, the row's id, when the write was stored for the wake's agent. The scheduler judges it as it judges a deferred write's wake: storedMessageDone finds the row on the store's current branch with getMessage, and writeAnswered, split out of deferredWriteDone so both use one judgment, drops the wake once the row is at or below the answered watermark on the current branch and, for a channel wake not marked ambient, the agent has published there having read it. A row that isn't on the current branch drops the wake, as a deferred write that's gone does. A store that throws keeps the wake and logs it. The new mark is carried wherever readsDeferredWrite is: the quiesce park's coalescing key, sameInferenceRequest for a provider hold, the merged trigger (kept only when every merged request shares it), and beginAgentTurn's strip, so a held turn's retry comes back as an ordinary wake. WritePlacement gains messageId, which addMessage already filled at runtime for the gate's self-wake notice. The red-first case is silent-heartbeat-rows' cooldown test, turned back to four calls: the resumed call ends on the message, and its wake starts no turn. Beside it: a resumed request that fails keeps the wake (five calls), a message removed before the resumed request drops it, and a channel message keeps its wake unless the resumed turn's words were delivered in its channel. heard-mid-turn-wake's unit test of the mark turns round to name the stored row. mcpl-coalescing gains a stored twin of each mark test: parked apart, merged only when shared, held apart in a provider hold, stripped from a held retry, and kept when the store can't be read. continuation-starts-alone.changed.md's "one more turn" sentence now says the wake is dropped where the restarted turn read and answered its message. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… kind's mark, a shared merge, a hold released by hand From the review of afcd501: - Each of the five wake kinds the fixed fragment names (a push, a module's one message, a background script's wake, a channel message, the gate's self-wake notice) is now pinned to give its wake the stored row's id, in heard-mid-turn-wake: the turn its wake would start names that row. Each fails at #762's head, where no stored mark exists. - The merge tests of both marks gain a pass whose two wakes name one write: the merged trigger keeps the mark, as the every-shares rule says. - The held-restart helper in silent-heartbeat-rows holds the restart with a 60 s cooldown and releases it by hand once the case is set up, where a 1 s hold ended on its own timer and a loaded host could end it before the message arrived or the row was removed. - continuation-starts-alone.changed.md now gives the drop's terms (a channel wake not marked ambient waits for the agent to speak there), and stored-message-wake.added.md says how messageId reaches a wake: the gate's addMessage hook fills the placement it then passes to requestInference as after. No host call takes messageId itself. - The readsStoredMessage doc names the ambient exemption; the scheduler comment no longer says a wake sent back from a resuming continuation then starts a turn of its own; markContextConsumed's comment names storedMessageDone beside deferredWriteDone as a reader of the answered watermark. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
main at 773a145, with #762 merged (a697939) and the train's other merges after the branch's base. One conflict, resolved: - test/silent-heartbeat-rows.test.ts: main removed transientFailure's doc comment along with its retryAfterMs; the branch added unretriedFailure just above it. Both are kept: unretriedFailure with its own doc, transientFailure as main has it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…turn main's anima-research#762 starts a turn's continuation alone (a budget restart, or a tool-result guard's retry a provider hold released): the batch's other requests go back to the queue and later start a turn of their own. Before it, such a wake joined the private continuation's start and was private. After it, its own turn carried nothing from the private turn, so a channel message whose wake arrived while the private turn was open got a public reply after all, against this change's rule that a channel message sharing an open turn with private input gets no inferred reply in its channel. Its own turn's request ends on `[Continue]`, after a private exchange it can only continue in public. A quiesce's parking of a continuation's siblings had the same gap on this branch before the merge. Where either path sends a continuation's siblings back to the queue, each now also continues the open turn (`continuesTurns` with the routing's turn id), so the scheduler's existing rule makes its start private exactly when that turn is private as it stands then. A public turn's siblings carry a public id, and start as on main. The id is the routing's, since a held guard retry carries its start's caller turns, and a turn opened by a private surface's message is private without one. Tests: a room's wake that arrives while a private turn runs waits for its budget restart, then starts a turn of its own that speaks in no channel, and the same during a quiesce; a channel turn's does so in its channel, as on main; and a wake a provider hold took ahead of a private turn's guard retry starts a private turn of its own. The breaking fragment's Unchanged line names the exception, and the changed fragment says it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This PR changes what the framework does when a stream's
cancel()throws during one of the framework's own cancels. The framework now removes the mark it recorded for that cancel, so the stream's later end isn't read as a cancel, and it logs the error. If the stream is still running, its turn stays open until the stream ends. If the stream was never sent, the start ends as if the cancel had worked.The PR also starts a turn's continuation, a context-budget restart or a tool-result guard's retry, without the other requests in its scheduler batch. And it fixes two smaller things in the same turn lifecycle. An ephemeral run whose continuation the inference policy drops now rejects at once, and
nudgeAgent()reportsidle+turn-alivewhile a turn waits for its continuation (#754).One part changes what
maindoes. A wake queued beside a context-budget restart used to run inside the restarted turn. It now waits for that turn to end and then starts a turn of its own. The section on batches under "Changes" has the details.Closes #754.
Problem
The framework cancels a turn's stream itself at a context-budget restart, at a tool result that ends the turn, at a restart while the framework stops, in a quiesce abandon, and in
stop(). Before each cancel it records a mark on the stream saying that a cancel happened (frameworkCancelledStreams), and the code that handles the stream's end reads that mark. Onmainat 8843507, when the stream'scancel()threw:abortInference()ended the turn without aninference:abortedevent and left its logical turn open. After a failed cancel at a restart while stopping, the turn was reported as ended byshutdown.quiesce({ abandon: true })rejected with the cancel's error.inference:failedwith the cancel's error. This happened to a start compiled while the framework stops, to one a caller stops during its compile, and to one held by a provider wait recorded during its compile. The stream was never iterated, so it had sent nothing.stop()was waiting for that loop.This PR treats a
cancel()that throws as having cancelled nothing. By our reading, membrane 0.7.1's owncancel()doesn't throw.Changes
A cancel that throws removes its mark. An earlier mark on the stream stays, such as a quiesce abandon's when
stop()cancels after it.Agent.cancelStream()likewise puts its own record of the stop back as it was, so the stream's later end, including a providererror, isn't read as that stop.At the three tool-boundary sites, the stream keeps running. These are a restart, an end of turn, and a restart while the framework stops. The stream waits for the round's tool results, which the boundary doesn't send. So the turn keeps its token until the stream ends, and an abort ends the stream when the abort's own cancel succeeds. A log line and an operator alert (
ops:alert, kindstream-cancel-failed) report this, because only an operator, or an ephemeral run's idle watchdog, will act on that turn. The alert says that aborting the agent tries the cancel again, and that if that cancel also throws, restarting the host process ends the turn. A restart whose cancel throws queues no continuation and reports neither a restart nor the round's usage, so the stream's own end counts its rounds. An end of turn still settles the tool-result guard's batch before its cancel, as onmain, so a later stop has no batch to withhold.A quiesce abandon whose cancel throws logs the error, lists the agent in
unabandonable, and goes on to the other agents' turns, soquiesce()resolves. A turn waiting at a later check isn't listed. An example is the tool-result guard's wait, which holds the round's finished stream. The abandon marks that check, and the turn ends there. The turn is listed only if it hasn't reached the check when the abandon's grace period runs out.stop()still rejects with the error, and now logs a line naming the agent.A start whose compiled stream is never sent logs a cancel that throws and ends as if the cancel had worked. When the start is dropped (its agent was replaced or removed, or the framework is stopping or was started again) or a provider wait holds it, the agent discards that stream with
reset()instead of cancelling it a second time.A stream that no longer owns its agent (a newer stream took the agent, or the agent was replaced or removed) logs a cancel that throws, and the loop that drives it exits.
A scheduler batch that contains a continuation of the turn now starts the continuation alone. A continuation is a context-budget restart or a tool-result guard's retry. The batch's other requests go back to the queue, wait for the turn to end as any wake does, and then start a turn of their own. A start that resumes a turn takes in no new input. It writes no clock row, flushes no deferred messages, and assembles no coalesced batch. So a wake merged into such a start got no turn of its own:
mainthe start ran as that wake, as a fresh turn, and opened a new EventGate window, where the retry continues its turn's window. Now it runs as the retry, with its own turn's provenance (counterparty,wakeChannelId,wakeAt), and the wake then starts a turn of its own.maina wake queued beside a restart was taken into the restart's turn. That happened whether the wake was queued while the turn ran or released together with the restart by a provider hold. A coalesced batch carried by that wake wasn't assembled there, and no turn of its own followed. A silent turn's restart (suppressProse) that took an ordinary wake resumed with prose routing. A message that no stream could inject, deferred through the tool boundary, was read by no later request. An autoContinue wake wrote no[continue]row and got no turn of its own.If the continuation already read the wake's message, the wake's own turn has nothing new to read, and its request ends on
[Continue]. If an inference policy drops the continuation, the provider hold's pending primary stays pending for the wake that went back to the queue. Since a continuation now starts alone, the autoContinue give-back for "a resumed turn took its wake" can no longer run. A batch that resumes a turn holds only the turn's continuations, and none of them carries an autoContinue mark. This PR removes the give-back, anddocs/auto-continue.mdnow says where such a wake's row is written. Atool_results_readywake stays in the batch with the continuation, as the two already stay together through a quiesce.An ephemeral run whose continuation the inference policy drops now rejects as its turn ends, with a plain
Errorwhose message names the drop. It used to wait for its idle watchdog.stop()ends each continuation's turn in a separate step, as its other loops do. A drop that throws no longer skips the agents after it. We know of no path in the package that throws there, so this change has no changelog fragment.nudgeAgent()now readshasTurn(). It reports an agent whose turn waits for its continuation asidle+turn-alive, as the scheduler sees it (nudgeAgent reports "idle" while the agent's turn waits for its continuation #754). Its doc now says that anidleagent's turn starts on the next scheduler pass, with two exceptions. A provider hold (a stated wait or a capacity cooldown) can hold the wake. Or a start of the agent's that waits for an auxiliary call to finish runs first.What callers notice
mainat 8843507quiesce({ abandon: true })when a turn's stream'scancel()throwsunabandonableunless a later check ends its turnabortInference()after a failed cancel at a restart or an end of turn, when the abort's own cancel succeedsinference:aborted, and the logical turn stays openinference:abortedwith the caller's reasonshutdowninference:abortedwith the caller's reasoncancel()throwsinference:failedwith the cancel's errorworker)nudgeAgent()while the turn waits for its continuationidleidle+turn-alive, in the result, the log line and theoperator:actionrecord[continue]row[Continue]The batch rows compare with
mainat 2d5f16d. This branch has since mergedmainat 3b9c7dd, and the tests of those rows fail at their claims there too (red-first, below). For each cancel that throws, a new log line names the agent and the site. At the three tool-boundary sites, thestream-cancel-failedoperator alert names them too.Tests
npm test, at this head, which mergesmainat 3b9c7dd: 7,576 tests, 7,532 passed, 0 failed, 44 skipped. CI's second step (stop-with-work-in-flight,stop-with-host-operationsandconversation-routing) passes 136 of 136. Each of the 315 test files run alone gives the same totals.code_execution's 11 test files under Bun, as CI runs them, pass 423 of 423. All on Linux, Node 24.22.1, npm 11.19.1 and Bun 1.4.4.The cancel that throws. Each site is tested with a stream whose first
cancel()throws:test/turn-continuation.test.ts: a restart, a provider error after it, an end of turn with the tool-result guard on, a restart while stopping, a quiesce abandon,stop(), an earlier abandon's mark understop(), a start compiled while stopping, after its agent was replaced, or with the lifecycle advanced as astop()and astart()advance it, a caller's stop during a compile, and a superseded stream at its next event and after it throws;test/provider-waits.test.ts: the hold after a compile;test/stop-without-stream.test.ts: an abandon at the guard's wait;test/host-quiesce.test.ts: an abandon that goes on to a second agent.A batch that contains a continuation, in
turn-continuation:In
busy-agent-scheduler, with requests queued directly: atool_results_readywake stays with a restart while an ordinary wake goes back to the queue; a batch with atool_results_readyahead of a guard retry starts as the retry; and an inference policy that drops a restart leaves a provider hold's pending primary for the wake sent back.The other fixes:
turn-continuation);stop()'s drop loop with one drop made to throw (turn-continuation);operator:actionrecord (turn-continuation).Coverage of behaviour
mainalready has, passing there too:turn-continuation);tool-result-guard);tool-result-guard). The test of a dropped turn's record next to it now says that the dropped turn leaves none.busy-agent-scheduler).Existing tests changed:
busy-agent-scheduler's test of a budget restart batched behind an ordinary wake now expects the wake back in the queue.silent-heartbeat-rows' test of a reply resumed after a provider cooldown, with a message that arrived during it, now expects the message's wake to start a turn of its own after the resumed one. That turn has nothing new to read, so its request ends on[Continue].turn-trigger-provenance's test of a restart batched with an addressed wake now expects the restart to start alone, with neither the wake's author nor its channel, and the wake to start a turn of its own with both.held-turn-retry's test of a budget restart's start merged with a fresh wake (fix(scheduler): a turn a provider hold keeps for its retry is alive #774) now expects the restart to park alone, the wake to wait in the queue, a stop to keep the wake, and the wake then to run as a turn of its own. It waits for the park rather than forrunUntilIdle(), which waits with the queued wake.tool-result-guardis renamed, as above.Red-first: we ran this head's nine changed test files against
main'ssrcat 3b9c7dd. Each test of a fix or a change above fails there at its claim, 34 in all. The new and changed tests that pass there are the four coverage tests, the renamed dropped-turn test, fix(turns): a turn stays alive while its continuation waits to start #722's restart test, and the ephemeral run's control.Removals, at this head: we removed, one at a time, each of 21 parts of the cancel handling, four parts of the batch rule,
stop()'s catch of the cancel's error, and the two lines ofmain's code that the coverage tests of a cancel record and of an ephemeral abandon rely on, and ran the relevant tests after each removal. Each removal made at least one test fail, except one: with the end-of-turn site ending the turn anyway, a test ran until its 120 s timeout cancelled it. The other fixes are a line or two each, and red-first covers them.Not verified
src/framework.ts:19934-19941). That check calls the same function as the two superseded-stream sites the tests do cover.pendingBatches()and records the framework's calls toassemble(). No test runs that case against a real MCPL server.tool_results_readycases queue their requests directly. We know of no path that queues one beside a continuation that resumes the turn.What this leaves
mainthis happens to a released wake that the turn sent back to the queue. This PR adds a second case: the wake a batch sends back when an inference policy then drops the batch's restart. Ending the pending primary at the withdrawal doesn't work, because refreshing a coalesced batch withdraws its wake and then queues a replacement. A provider hold's pending primary isn't ended when the agent's last wake is withdrawn #764 tracks this.changelog.d/framework-cancel-throws.fixed.md,changelog.d/guard-retry-trigger.fixed.md,changelog.d/continuation-starts-alone.changed.md,changelog.d/ephemeral-dropped-continuation.fixed.mdandchangelog.d/nudge-turn-alive.fixed.md.Ada-1017 (Claude Opus 5.5) decided the rule for a cancel that throws, the rule for a batch that contains a continuation, and the scope of each fix. Barnaby-2576 (Claude Opus 5.5) asked for the runs at
mainthat showed the cases of the throwing cancel, the guard retry's trigger and the ephemeral run. An internal review read the whole change, and a second read its fixes. Phoebe-2419 (Claude Opus 5.5) read the change and found that a failed end-of-turn cancel left the guard's batch for a stop to withhold, and that only an operator could end such a turn. Gladys-2413 (Claude Opus 5.5) weighed each part of the batch rule against the least change, and asked whether a message no stream could inject went unread onmainbeside a restart, which a test now shows it did. Crispin-2406 (Claude Opus 5.5) read the wake's own turn as the agent receives it. Hilda-2681 (Claude Opus 5.5) read the merge ofmain, and how the batch rule meets a held turn's retry. Cuthbert-2453 (Claude Opus 5.5) wrote the PR, contributing through nissa-seru.Description checked and posted by Fiona-2757 (Claude Opus 5.5), and this version by Lottie-2661 (Claude Opus 5.5).
🤖 Written using Anthropic's Claude Opus 5.5 through the Rookery's tooling, and posted from the shared
nissa-seruaccount.