Repository navigation
fix(think): preserve scheduled retries across restarts - #2297
Merged
Merged
Conversation
Startup treated only continuation callbacks as outstanding recovery, so an already-scheduled retry could lose its running submission after restart. Recognize both callbacks at the existing submission-recovery seam for Tasks and legacy schedules. Cover pending retries, request isolation, terminal Tasks, cancellation, empty opened streams, and persisted restart completion. Refs #2276.
Review found two remaining interruption windows: the recovery callback signaled handoff before the successor turn was durably accepted, and a non-retained successor could settle before ledger completion. Rebind the running submission to the accepted successor before handoff, retain terminal stream evidence for running submissions, and settle the ledger from that evidence at startup without rerunning the turn. Replace the e2e table-surgery staging with inert seeded evidence driven through two real restarts so production owns retry scheduling and linkage.
Review found the acceptance callback lived on the agent instance, so any concurrent root chat turn admitted during the successor's pre-admission awaits could claim the handoff, rebind the submission to a foreign request, and release the recovery Task early. Hold the callback in successor-scoped async context so only the intended turn's chain can signal acceptance.
…ollision gaps Review of the handoff work found three follow-ups: facet-hosted recoveries never signaled acceptance, so the recovery step awaited the whole model turn against its 15-minute timeout; stream reclamation deleted the terminal evidence retained for running submissions as soon as any new turn started; and the recovery lookup matched submission and request identities without precedence, letting a collision touch the wrong submission. ResumableStream now owns a durable retained-row lifecycle (retain at settlement, skip at reclaim, release when settled) and Think releases pins at every ledger settlement and on startup. Facet successors signal acceptance once their fiber is durably created. Exact submission identity takes precedence over request-id fallback, and new recovery payloads carry the stable submission id.
🦋 Changeset detectedLatest commit: 0f7d380 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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: |
Startup and recovery reconciliation inferred a running submission's outcome from stream lifecycle state, but streams also settle as completed on abort and on overflow retry, and the structured output only ever lived in memory. A crash between stream settlement and the ledger write could report a cancelled submission as completed, complete an overflow segment whose answer was never persisted, or emit a completed workflow event with an undefined output.
Record the turn's terminal outcome ('completed' | 'aborted' | 'retry') and its structured output on the submission row in the same transaction that settles the stream. Reconciliation settles from that recorded fact, passing the recorded output through to workflow notifications; stream-status inference remains only as a fallback for legacy rows, and a retry stamp is never completion evidence.
Contributor
🟡 agents import sizesMeasured 343 runtime imports as minified bundles. The primary size is gzip; raw minified size is included for diagnosis. An existing import growing by more than 10% is marked red. This report is informational.
Compared Changed imports (1)
All 343 current runtime imports
Reported by agent-think[bot]. |
…ention The branch carried two overlapping restart-evidence mechanisms: retained terminal stream rows (round 3) and durable outcome stamps (round 4). Stamps are the atomic recorded fact, so consolidate on them before the unreleased retention API becomes contractual: - agents: remove retain/release/listRetained and the retained metadata, reclaim exclusion, and updateMetadata plumbing. Clear the pending cutover marker only after settlement succeeds in finalizePending so a failed settlement stays retryable, matching cutover. - think: stamp result_status='error' atomically with markError so errored-turn evidence survives foreign stream reclamation without pinning rows; startup and recovered settlement read the stamp with pending recovery still taking priority. Submission streams are again discarded at cutover; the stamp is the evidence. Remove all pin bookkeeping and clear stamps on cancel/interrupt terminal writes. - tests: replace retention assertions with stamp-lifecycle assertions; new agents settlement tests cover persist rollback and retryable finalizePending.
Comment on lines
+13735
to
+13747
| /** Write only inside the transaction that settles this exact request's stream. */ | ||
| private _recordSubmissionTurnResult( | ||
| requestId: string, | ||
| result: SubmissionTurnResult | ||
| ): void { | ||
| const row = this._readRunningSubmissionForRecovery(requestId); | ||
| if (!row || row.request_id !== requestId) return; | ||
| this.sql` | ||
| UPDATE cf_think_submissions | ||
| SET result_status = ${result.status}, | ||
| output_json = ${result.status === "completed" && result.output !== undefined ? JSON.stringify(result.output) : null} | ||
| WHERE submission_id = ${row.submission_id} AND status = 'running' | ||
| `; |
Contributor
There was a problem hiding this comment.
Merged
This was referenced Sep 23, 2026
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.
Problem
After a restart, Think could mark a submission as failed even though its recovery was still in progress — or had already succeeded. Review found the reported bug plus adjacent gaps, all breaking the same invariant: a running submission must always be resolvable to live or terminal evidence at startup.
output: undefined.Fix
Recognize both kinds of recovery work at startup; keep the submission bound to whichever side currently owns it across the handoff (the handoff signal is scoped to the intended successor's call chain, on both the Tasks and facet-fiber paths); and settle the ledger from a durable record of the turn's actual outcome without rerunning the turn or repeating callbacks.
Each turn's terminal outcome (
completed,aborted,retry, orerror) and structured output are recorded on the submission row in the same transaction that settles its stream, so restarts settle from recorded fact rather than inference: aborts stay aborted, recovered workflow completions keep their exact output, an overflow retry segment is never mistaken for an answer, and the record survives unrelated turns reclaiming stream rows. Terminal-stream inference remains only as a fallback for rows written before this change. Exact submission IDs take precedence over request-ID fallback, and new recovery payloads carry the stable submission identity (old payloads still match).The
agentschange is deliberately tiny (changeset: patch): exposeResumableStream.pendingCutoverId, and makefinalizePendingclear its pending marker only after settlement succeeds so a failed settlement stays retryable — matchingcutover. An earlier revision added a retained-stream lifecycle toResumableStream; the outcome record made it redundant, so it was removed before release rather than shipped as API surface.Scope note
This fixes more than the original startup-detection gap: closing only item 1 still failed deterministic interruption tests for the reported symptom (see the reporter's third scenario in #2276, whose request-ID analysis matches item 2 exactly). The commits land on clean boundaries —
f620ff54(item 1),2984fba5+dfa76d65(item 2),91ee77ba(items 3–5),ea424ebe(item 6),0f7d380f(consolidation onto the outcome record +finalizePendingretry fix) — and can be split on request.Validation: Deterministic red-first tests for every window above (including foreign-turn interference, foreign-stream reclamation with a real process restart, abort/output crash windows, facet handoff, and ID collisions), full Think (936) and agents suites, submission-recovery e2e (9), build, and
pnpm run check— all passing with retries disabled.Fixes #2276