Repository navigation
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.
🦋 Changeset detectedLatest commit: 91ee77b 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 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 (8)
All 343 current runtime imports
Reported by agent-think[bot]. |
agents
@cloudflare/ai-chat
@cloudflare/codemode
hono-agents
@cloudflare/shell
@cloudflare/think
@cloudflare/voice
@cloudflare/worker-bundler
commit: |
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.
| const acceptance = recoveredTurnAcceptanceContext.getStore(); | ||
| const onAccepted = | ||
| acceptance?.agent === this ? acceptance.onAccepted : undefined; | ||
| const run = (): Promise<T> => { | ||
| // Tasks accepts the run durably before invoking its live closure. Rebind | ||
| // submission ownership before telling the recovery Task it may settle, | ||
| // so startup can always find either predecessor or successor evidence. | ||
| onAccepted?.(requestId); |
There was a problem hiding this comment.
🔴 Facet recovery never hands off
When recovery runs on a facet, _runChatRecoveryFiber takes the legacy-fiber branch before onAccepted is captured. The recovery dispatcher then waits for the full model run inside a 15-minute recovery step. Long turns time out, while shorter turns block later lifecycle jobs.
Learn more
Facet-hosted chat turns use the legacy fiber engine because routed sub-agents cannot create chat Tasks. The new acceptance callback is read only after that branch returns, so a facet successor never calls the recovery dispatcher's handoff signal. The dispatcher therefore keeps the recovery Task or compatibility schedule attached until the complete model turn returns. The shared recovery Task bounds that callback with a 15-minute step timeout in createChatRecoveryTaskDefinition, despite chat turns supporting longer execution. It also keeps the Lifecycle alarm job occupied, delaying unrelated work on the same object.
Example: A routed Think facet recovers a provider turn that takes 20 minutes. The recovery callback never hands off, its continuation step times out after 15 minutes, and the platform can retry a turn that was still executing.
Recommended fix: Add an acceptance signal for the legacy-fiber branch at the point _runFiberWithStashWrapper has durably created the successor fiber. Preserve the same ordering as the Task branch: persist or rebind the submission first, then invoke onTurnStarted. Add facet recovery tests covering the handoff boundary and a queued lifecycle job.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (this._agentToolRunsByRequestId.get(requestId)) return false; | ||
| this._ensureSubmissionTable(); | ||
| const running = this.sql<{ submission_id: string }>` | ||
| SELECT submission_id FROM cf_think_submissions | ||
| WHERE request_id = ${requestId} AND status = 'running' | ||
| LIMIT 1 | ||
| `; | ||
| return running.length === 0; |
There was a problem hiding this comment.
🔴 Next turn erases recovery evidence
If another turn starts before recovery updates the ledger, ResumableStream.start deletes the retained terminal stream. A crash then leaves no recovery evidence, so startup marks the completed submission as error.
Learn more
A recovered successor keeps its terminal stream because its non-retained chat Task can disappear before _completeRecoveredSubmission updates the submission row. After the queued turn finishes, recovery still awaits incident bookkeeping before that ledger update. Another admitted turn can start during this await. ResumableStream.start() calls reclaim(), which deletes every terminal chat stream without checking whether a running submission still depends on it. A restart after that deletion cannot find an active recovery Task, successor chat Task, or terminal stream.
Example: Recovery successor r2 finishes and retains a completed stream while submission s1 is still running. An unrelated turn r3 starts while incident completion is awaiting storage, deleting r2's stream. The object restarts before s1 is finalized, and startup changes s1 to error instead of completed.
Recommended fix: Prevent reclaim from deleting terminal streams referenced by running submission rows, or atomically settle the submission ledger before making the stream reclaimable. Add a test that pauses incident completion, starts an unrelated stream, then runs startup reconciliation.
Was this helpful? React with 👍 or 👎 to provide feedback.
| WHERE (submission_id = ${recoveredRequestId} OR request_id = ${recoveredRequestId}) | ||
| AND status = 'running' | ||
| LIMIT 1 |
There was a problem hiding this comment.
🟡 Recovery IDs select wrong submission
When one submission ID equals another submission’s successor request ID, _readRunningSubmissionForRecovery can select either running row. request_id is non-unique in the submission schema. The callback can rebind or terminalize the wrong submission.
Learn more
Submission IDs are caller-controlled primary keys, while successor request IDs occupy a separate, non-unique column. Recovery payloads normally carry the stable submission ID, but the lookup now treats both namespaces as interchangeable. Two running rows can therefore match one value, and LIMIT 1 provides no deterministic ownership rule. The same identity ambiguity also affects scheduled-recovery matching during startup.
Example: Submission A is recovering under successor request ID r2. After observing r2, a caller creates submission ID r2; both remain running. Recovery for the new r2 submission can select A through its request_id and complete or rebind A instead.
Recommended fix: Preserve the identity kind in recovery payloads and query submission_id first when the payload carries the stable submission identity. If successor IDs must also be accepted for compatibility, use an explicit fallback only when no submission-ID match exists, and apply the same precedence in _hasScheduledChatRecovery.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
/devin review |
…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.
01c321b to
91ee77b
Compare
|
Superseded by #2297 (identical content on a fresh branch) to work around a stuck automated review run on this PR. |
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.
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 the turn's retained terminal stream without rerunning the turn or repeating callbacks.
ResumableStreamgains a small additive lifecycle for that evidence: rows can be retained at settlement, are skipped by reclamation, and become reclaimable again onrelease(). Think pins a running submission's terminal stream until its ledger settles and releases pins at every settlement path and on startup. Exact submission IDs take precedence over request-ID fallback, and new recovery payloads carry the stable submission identity (old payloads still match).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 + theagentsretention API, changeset: minor) — 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, facet handoff, and ID collisions), full Think (936) and agents (3,244) suites, submission-recovery e2e, build, and
pnpm run check— all passing with retries disabled.Fixes #2276