Repository navigation
fix(agents,think): recover server-tool turns, terminalize thrown recovery, re-run alarms killed by a script-upgrade supersede - #1659
Merged
Conversation
…ery give-up Two chat-recovery failures could leave a turn wedged at a half-finished assistant message after a deploy/eviction, with no terminal banner. 1. Server-tool recovery deadlock When a server-side tool's execute() was interrupted by an eviction, the recovered turn's orphaned tool part was persisted at `input-available`. No client `tool-result` will ever arrive for a server tool, so `waitUntilStable` (the gate before `continueLastTurn`) could never converge — `hasPendingInteraction()` stayed true forever. The recovery continuation burned its whole attempt budget on a wait that could not succeed, and `_repairToolTranscriptParts` (which would flip the orphan to an errored result) only runs inside the inference loop, downstream of that gate. Fix: narrow the recovery stability predicate. An `input-available` part is now only "pending" when it is genuinely client-resolvable — a registered client tool whose result the SPA can replay, or an `approval-requested` part. A dead server-tool orphan no longer blocks stability, so the wait converges, `continueLastTurn` runs, and the existing transcript-repair pass flips the orphan to `output-error` and the model continues. Client tools are persisted at request acceptance and restored in onStart, so the predicate always classifies them correctly during the wait (no client-tool recovery regression). `waitUntilStable` / `hasPendingInteraction` are recovery-only, so the change is tightly scoped. 2. Silent seal on a thrown recovery callback `onExhausted` only fired on the clean budget-exhaustion branch. Any error thrown by `_chatRecoveryContinue` / `_chatRecoveryRetry` was marked `failed` and re-thrown; `Agent._executeScheduleCallback` swallows a non-reset error and `alarm()` then deletes the one-shot recovery row — terminating the turn with no `onExhausted` event and no terminal banner (the user stares at a frozen message until they send something new). Fix: route a non-reset throw through the same exhaustion path (`_exhaustRecoveryGiveUp`, reason `recovery_error`) so `onExhausted` fires and the configured `terminalMessage` banner is delivered, then return without re-throwing. A genuine Durable Object code-update reset is still re-thrown so the platform re-runs recovery on the fresh isolate. The underlying error is logged for operators. Hardening `_exhaustChatRecovery` now broadcasts the terminal banner before its bookkeeping storage writes, and `_exhaustRecoveryGiveUp`'s incident read/write are best-effort, so a storage failure during give-up can no longer suppress the user-visible terminalization. Tests: predicate (client/approval orphan stays pending; server orphan does not, and waitUntilStable converges) and throw handling (non-reset terminalizes via onExhausted + banner; code-update reset re-throws and does not terminalize). Full think suite (487) passes; typecheck + lint clean. Behavior note: a thrown recovery callback now emits `chat:recovery:exhausted` (reason `recovery_error`) instead of `chat:recovery:failed`. Co-authored-by: Cursor <cursoragent@cursor.com>
🦋 Changeset detectedLatest commit: a26ef02 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
agents
@cloudflare/ai-chat
@cloudflare/codemode
hono-agents
@cloudflare/shell
@cloudflare/think
@cloudflare/voice
@cloudflare/worker-bundler
commit: |
…d" supersede, not just "code was updated"
A one-shot scheduled callback (e.g. a queued submission's drain alarm, or a
chat-recovery continuation) that is killed mid-flight by the platform replacing
the isolate was only re-run when the error matched
`/reset because its code was updated/i`. The platform surfaces the *same*
superseded-isolate failure class as a second message —
"This script has been upgraded. Please send a new request to connect to the new
version." (a stub/connection to a superseded script) — which fell through to
`_executeScheduleCallback`'s swallow-and-delete branch: the one-shot row was
deleted and the work abandoned.
For a `submitMessages` turn this orphaned the pending submission with no driver
(the drain alarm was deleted, inbound reconnects were canceled during the
bounce), so the turn sat untouched until something unrelated woke the Durable
Object minutes later — an indefinite user-facing spinner with no terminal
event. A normal deploy bounce surfaced as "reset because its code was updated"
and recovered, which is why the two looked inconsistent.
Fix: broaden the superseded-isolate matcher to recognize both messages, so
either causes the one-shot row to be preserved and re-run on the fresh isolate
under the at-least-once alarm guarantee (which then runs `onStart` →
`_recoverSubmissionsOnStart` → `_startSubmissionDrain` to re-drive the
submission). Applied in lockstep to:
- `isDurableObjectCodeUpdateReset` in the base `Agent` scheduler
(`packages/agents/src/index.ts`) — the primary path (the drain/recovery
one-shot alarm); also benefits `@cloudflare/ai-chat`, which shares the
scheduler.
- `_isDeployCodeUpdateReset` in `Think` — so a supersede thrown inside a
`_chatRecoveryContinue`/`_chatRecoveryRetry` callback re-throws (defers +
re-runs) instead of terminalizing via `onExhausted`.
"Network connection lost." is deliberately NOT treated as a supersede: it is a
connection error that may succeed on in-process retry (gated by the CF
`retryable` property via `isErrorRetryable`), not an isolate replacement, so it
stays on the normal retry path.
Tests:
- agents: a one-shot callback that throws each error string — the two
supersede messages preserve the row + reject `alarm()` (deferred); an
ordinary error and "Network connection lost." swallow + delete the row.
- think: a supersede thrown in a recovery callback re-throws (defer) and does
NOT terminalize, alongside the existing code-update-reset and non-reset
cases.
Full suites pass: agents 1295, think 493; typecheck + lint clean.
Co-authored-by: Cursor <cursoragent@cursor.com>
… platform phrase Review hardening for the supersede-defer fix: match "this script has been upgraded" rather than the looser "script has been upgraded" substring, so an ordinary application error that merely mentions an upgraded script can't be misclassified as a platform supersede. A false positive matters more now that the match defers + re-runs (a misclassified, genuinely-failing one-shot callback would re-run on the platform's alarm retries instead of being abandoned). Adds a lookalike-error guard test. No change to the real platform strings, which still match. Co-authored-by: Cursor <cursoragent@cursor.com>
Merged
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
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.
Summary
Three chat-recovery / durable-execution failures that could leave a turn wedged at a half-finished assistant message after a deploy/eviction, with no terminal banner — the turn never recovers and never terminalizes until something unrelated wakes the Durable Object. All three were reproduced from production incidents on a server-tool-heavy app (a preview tool whose
execute()bridges to the SPA over HTTP and parks awaiting external I/O).execute(). (think)onExhausted/ banner ever fires. (think)"script upgraded"supersede is not re-run — only"reset because its code was updated"was recognized as a deferrable platform kill, so the sibling supersede message orphaned the work with no driver. (agents core + think)This is a follow-up in the same
#1649/#1651/#1657recovery-hardening line; it targets the server-tool + deploy-recovery half of the "interrupted tool" symptom (whereas #1657 fixed the client-tool + normal/auto-continuation half). See "Relationship to #1657" below.Bug 1 — server-tool recovery deadlock
Repro
execute()awaits external I/O (e.g. a long-poll bridge: the SPA fetches the tool input and posts the result back over HTTP; the in-flight promise lives only in DO memory).tool-input-availablechunk is flushed durably.execute()is parked. The DO is evicted; the in-memory promise is gone._handleInternalFiberRecoverypersists the orphaned partial (the resumable stream is restored from SQLite, sostreamStillActiveis true →_persistOrphanedStreamruns), landing a new assistant message with the tool part atstate: "input-available"._chatRecoveryContinue, which callswaitUntilStablebeforecontinueLastTurn.hasPendingInteraction()returnstruebecause of theinput-availablepart — but nothing will ever resolve it: there is no clienttool-resultfor a server tool, no in-memory_pendingInteractionPromise(it died with the isolate), and_repairToolTranscriptParts(which would flip it to an errored result) only runs inside_runInferenceLoop, i.e. downstream of the very gate that won't converge.waitUntilStabletimes out every attempt; the continuation reschedules until the budget drains (pure waste — the wait can never converge).hasPendingInteractioncan't tell "a client is about to post a result over WS" from "a server tool'sexecute()promise died with the DO". For a server tool the second case is permanent.Fix
Narrow the recovery stability predicate. An
input-availablepart is now only "pending" when it is genuinely client-resolvable:approval-requested— a reconnecting client can replay the approval; orinput-availablefor a client tool (its name is in the last request's client-tool set) — the SPA can replay thetool-result.A server tool's
input-availableis no longer treated as pending.waitUntilStableconverges,continueLastTurnruns, and the existing transcript-repair pass flips the orphan tooutput-errorso the model continues with an errored tool result.Why client-tool recovery is preserved: client tools are persisted to config at request acceptance (
_persistClientTools, before any tool streams) and restored inonStart(_restoreClientTools), whichalarm()awaits before dispatching the recovery callback. So_lastClientToolsis always populated during the wait and a client-tool orphan is correctly kept "pending".waitUntilStable/hasPendingInteractionare recovery-only (their sole callers are_chatRecoveryContinue/_chatRecoveryRetry), so the change is tightly scoped and can't affect a normal turn.Bug 2 — silent seal on a thrown recovery callback
Root cause
onExhaustedonly fired on the clean budget-exhaustion branch. Any error thrown by_chatRecoveryContinue/_chatRecoveryRetryafter the incident was opened was markedfailedand re-thrown.Agent._executeScheduleCallbackre-throws only a Durable Object code-update reset (to preserve the one-shot row for re-run on the fresh isolate); it swallows any other error and returns normally, after whichalarm()deletes the one-shot row. Net: the turn is sealed with noonExhaustedevent and no terminal banner — the half-finished message wedges silently until the user sends something new. (This matched the production trace: "recovery alarm threw, no terminal event, message frozen ~6 min until the user typed again".)Fix
Route a non-reset throw through the same exhaustion path as a budget give-up (
_exhaustRecoveryGiveUp, reasonrecovery_error) soonExhaustedfires and the configuredterminalMessagebanner is delivered, then return without re-throwing. A genuine code-update / supersede reset is still re-thrown so the platform re-runs recovery on the fresh isolate (see Bug 3). The underlying error is logged for operators (the give-up records only therecovery_errorcategory).Hardening (closes the original telemetry observation)
_exhaustChatRecoverynow broadcasts the terminal banner before its bookkeeping storage writes (_markRecoveredSubmissionInterrupted/_recordTerminalChatStatus), so a failing write can't suppress the user-visible terminalization._exhaustRecoveryGiveUp's incident read/write are best-effort (try/catch): a storage failure during give-up degrades to "maybe a second banner" rather than "silently drop the turn".Bug 3 — one-shot alarm killed by a
"script upgraded"supersede is not re-runRoot cause
The deploy-survival mechanism hinges on one matcher.
Agent._executeScheduleCallbackonly defers (re-throws → preserves the one-shot row → the platform re-runs it on the fresh isolate under the at-least-once alarm guarantee) when the error matchesisDurableObjectCodeUpdateReset, whose regex was/reset because its code was updated/i. The platform surfaces the same superseded-isolate failure class as a second message —"This script has been upgraded. Please send a new request to connect to the new version."(a stub/connection to a superseded script) — which fell through to the swallow-and-delete branch.For a
submitMessagesturn, the driver is a one-shot drain alarm (_scheduleSubmissionDrain→this.schedule(0, "_drainThinkSubmissions", …)). When it dies with"script upgraded", the row is deleted and the submission is left with no driver: no alarm, and inbound reconnects are canceled during the bounce, soonStart(which would run_recoverSubmissionsOnStart→_startSubmissionDrainto re-drive it) never fires. The turn sits orphaned for minutes until something unrelated wakes the DO — an indefinite user-facing spinner with no terminal event. A normal deploy bounce surfaces as"reset because its code was updated"and recovers, which is exactly why the two looked inconsistent in the field.Fix
Broaden the superseded-isolate matcher to recognize both messages, so either causes the one-shot row to be preserved and re-run on the fresh isolate. Applied in lockstep to:
isDurableObjectCodeUpdateResetin the baseAgentscheduler (packages/agents/src/index.ts) — the primary path (the submission drain / recovery one-shot alarm). This also benefits@cloudflare/ai-chat, which shares the scheduler._isDeployCodeUpdateResetinThink— so a supersede thrown inside a_chatRecoveryContinue/_chatRecoveryRetrycallback re-throws (defers + re-runs) instead of terminalizing viaonExhausted(correcting Bug 2's classification for this class)."Network connection lost."is deliberately not treated as a supersede: it is a connection error that may succeed on in-process retry (gated by the CFretryableproperty viaisErrorRetryable), not an isolate replacement, so it stays on the normal retry path.Relationship to #1657 (complementary, not overlapping)
Both fix the same customer-facing symptom — the default repair message
"The tool call was interrupted before a result was recorded."— and both live inpackages/think/src/think.tsaround the_pendingInteractionPromisecontinuation barrier, but they address different root causes in non-overlapping code:_enqueueInteractionApplyserialization) and (b) a mid-stream client result dropped before persist (_streamingAssistant). It makes the client result land so the repair backstop doesn't spuriously error it.waitUntilStable, a thrown recovery callback seals silently, and a"script upgraded"kill orphans the drain alarm.They reinforce each other: #1657 reduces spurious client-tool
input-availableorphans; this PR ensures a genuinely-dead server orphan no longer wedges recovery. Rebased cleanly on top of #1657.Tests
waitUntilStablesuite): a client-toolinput-availableorphan stays pending (SPA can replay); anapproval-requestedorphan stays pending; a server-toolinput-availableorphan does not block stability andwaitUntilStablereturnstrue.onExhaustedwith reasonrecovery_error, delivers the banner, seals the incidentexhausted) and does not re-throw; a code-update reset and a"script has been upgraded"supersede each re-throw (defer + re-run) and do not terminalize.schedule.test.ts): a one-shot callback throwing"reset because its code was updated"or"This script has been upgraded…"preserves the row and rejectsalarm()(deferred); an ordinary error and"Network connection lost."swallow + delete the row.Verification
tsc --noEmitforagentsandthink— cleanoxlint(agents+thinksrc) — 0 warnings / 0 errors;oxfmt— cleanagents— 1295 passed (full workers suite);@cloudflare/think— 493 passedChangeset
patchforagents(Bug 3 scheduler fix) and@cloudflare/think(Bugs 1 & 2).Behavior note
A thrown recovery callback now emits
chat:recovery:exhausted(reasonrecovery_error) instead ofchat:recovery:failed, and the recovered submission'serror_messageis the configuredterminalMessage(the raw error is logged). For observability bridges keyed onchat:recovery:*this is more accurate (the turn is terminalized, not left ambiguously failed).Follow-ups (out of scope)
@cloudflare/ai-chat(duplicated recovery layer); note ai-chat has no transcript-repair pass, so Bug 1 there needs the predicate change plus a repair step. (Bug 3's base-scheduler fix already covers ai-chat.)ChatRecoveryExhaustedContext(additive API) if observability bridges want the cause string, not just therecovery_errorcategory.Made with Cursor