Repository navigation
fix(chat): surface terminal recovery outcome to clients that reconnect after it ended (#1645) - #1690
Conversation
…t after it ended (#1645) When a durable chat turn exhausts recovery (e.g. during a deploy / WebSocket reconnect storm) while no client is connected, the terminal error was only broadcast transiently. A client that connected afterward ran the standard resume probe, was told `RESUME_NONE`, and the failed turn stayed frozen — it never learned the turn had ended. Root cause: the shared `useAgentChat` client only surfaces a terminal `{ done: true, error: true }` frame as `useChat.error` when that frame arrives on the transport's *stream reader* (the resume handshake). A bare broadcast frame received outside an active stream read is dropped. `@cloudflare/ai-chat` had no durable record at all; `@cloudflare/think` persisted one but only replayed it as a bare on-connect frame, which the client dropped — so both presented the same frozen-turn symptom. Fix (both packages): persist the terminal outcome durably and replay it over the resume handshake on the next reconnect: STREAM_RESUMING (id = terminal requestId) -> client STREAM_RESUME_ACK -> terminal error frame on the resumed stream This is the only path that becomes `useChat.error` on the real client. The record is retained across the ACK so concurrent reconnects (multiple tabs) each learn the outcome. Record lifecycle (cleared when superseded), covering every path: - a new client-initiated chat request, - any later turn that completes successfully — including turns driven purely server-side via `saveMessages` / auto-continuation with no client request in between, - clearing the conversation (WS `chat-clear` and the programmatic `clearMessages()` API) so a stale exhaustion can't replay onto a freshly-emptied chat. Cleanup: `@cloudflare/think` no longer emits the bare terminal frame in `_buildIdleConnectMessages` (it was dead for the real client and redundant with the handshake). The live "recovering…" status (#1620) is still replayed there — it's a plain status frame the client handles on connect. Naming unified across both packages so the implementations are structurally parallel: `_recordChatTerminal` / `_clearChatTerminal` / `_pendingChatTerminal` / `_replayTerminalOnResume` / `_replayTerminalOnAck`, keyed by the shared `CHAT_LAST_TERMINAL_KEY`. Tests (real Durable Object + real WebSocket, each new guard verified red without its fix): - replays the terminal exhaustion to a client that reconnects after it ended (ai-chat + think), - clears the record when a later server-side turn succeeds (ai-chat), - drops the record when the conversation is cleared (ai-chat + think), - existing think hydration tests updated to assert the durable record (which the handshake replays) instead of the removed on-connect frame. ai-chat 563 tests pass, think 553 tests pass; `pnpm run check` green.
🦋 Changeset detectedLatest commit: b963bd3 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: |
There was a problem hiding this comment.
🟡 Think missing eager terminal-clear on new chat request, allowing stale error replay after user submits a new message
In packages/ai-chat/src/index.ts:832, the PR explicitly clears the terminal record when a new chat request is submitted, with the comment "A genuinely-new turn supersedes any pending terminal record". The same guard is not applied to Think's _handleChatRequest (packages/think/src/think.ts:6858). Before this PR the gap was harmless — Think delivered the terminal as a bare on-connect frame that the client silently dropped. After this PR the terminal is delivered via the resume handshake (_replayTerminalOnResume), a path the client actually processes as useChat.error. So if a user submits a new message but refreshes the page before the new turn starts streaming (the window between request persistence and inference start), a reconnecting client hits the terminal-replay branch (no active stream, no pending continuation) and sees the stale exhaustion error from the previous failed turn — even though they've already moved on.
(Refers to lines 6858-6866)
Prompt for agents
In Think's _handleChatRequest (around line 6858 in think.ts), add a _clearChatTerminal() call after the concurrency-decision drop check and before entering the turn queue — mirroring what ai-chat does at its line 832. The placement should be after the concurrency action === 'drop' early return (line 6911) and before releasePendingEnqueue (line 6914), e.g.:
// A genuinely-new turn supersedes any pending terminal record (#1645)
await this._clearChatTerminal();
This ensures that a reconnecting client who has already submitted a new message does not see a stale exhaustion error replayed via the resume handshake. The same reasoning that motivated the guard in ai-chat's _handleChatRequest applies here — Think now delivers terminals over the resume handshake (not the dropped bare-frame path), so the stale record is actively harmful.
Was this helpful? React with 👍 or 👎 to provide feedback.
…nnect race (#1645) `@cloudflare/ai-chat` clears the pending terminal record at submit time (index.ts) so a stale exhaustion can't replay once the user has moved on. `@cloudflare/think`'s `_handleChatRequest` lacked the equivalent eager clear — it relied solely on the turn-completion clear. That left a race: after the user submits a new turn but before it starts streaming (the enqueue / pre-stream window), a reconnecting client hits the terminal-replay branch (no active stream, no pending continuation) and is served the previous failed turn's error over the resume handshake. The gap was harmless before this PR (the terminal was sent as a bare on-connect frame the client dropped); it became user-visible once the terminal is delivered over the handshake. Fix: clear the terminal record in `_handleChatRequest` right after the concurrency decision accepts the submit, mirroring ai-chat. Test: drives a real WS submit with the new turn parked in `beforeStep` (via `setBeforeStepAsyncDelay`) so the observed clear must be the eager submit-time clear, not the turn-completion clear. Verified red without the fix (the stale `root-old` terminal survives the submit).
Follow-up: closed a reconnect race in Think (review feedback)Validated a review comment — it was correct. The race: after a user submits a new turn but before it starts streaming (the enqueue / pre-stream window), a reconnecting client hits the terminal-replay branch (no active stream, no pending continuation) and gets served the previous failed turn's error over the resume handshake — even though they've moved on. Harmless before this PR (the bare on-connect frame was dropped by the client); user-visible now that the terminal is delivered over the handshake. Fix: Test: drives a real WS submit with the new turn parked in think now at 554 tests; |
… just completed (#1645) The response-hook drain loop in `_runExclusiveChatTurn` cleared the pending terminal record only when `chatResult.status === "completed"`. An `"aborted"` turn (a server-side `saveMessages` / `continueLastTurn` cancelled mid-stream via its `options.signal`) left the stale terminal in place, so after a recovery exhaustion it would replay to the next reconnecting client over the resume handshake even though the conversation had moved on. Think already clears on any non-error/non-interrupted status (its `_recordTerminalChatStatus`), so this also removes a cross-package inconsistency. The client-submit path clears eagerly, so the gap was only reachable for purely server-side turns. Fix: clear the record on `"completed"` OR `"aborted"` (only a fresh error should leave a terminal to replay). Test: drive a server-side turn aborted via a pre-aborted signal (the stream loop breaks immediately and pushes an `"aborted"` result through the drain loop) after an exhaustion, and assert the record is cleared. Verified red without the fix (the stale terminal survives the abort).
Follow-up: clear terminal on aborted server-side turns (review feedback)Validated another review comment — correct. The drain loop in Think already clears on any non-error/non-interrupted status ( Fix: clear on Test: drive a server-side turn aborted via a pre-aborted signal (the stream loop breaks immediately and pushes an ai-chat now at 564 tests; |
…te on exhaustion The exhaustion path broadcasts the terminal banner first (deliberate — the broadcast touches no storage, so it stays resilient to a storage write rejecting mid-deploy), then did two storage writes: `_markRecoveredSubmissionInterrupted` followed by `_recordTerminalChatStatus`. That left the #1645 durable terminal record — the one a disconnected client replays over the resume handshake on reconnect — as the LAST write, so a failure in the (independent) submission-row write would skip it. Hoist `_recordTerminalChatStatus` ahead of `_markRecoveredSubmissionInterrupted` so the reconnect-critical record isn't hostage to an unrelated write. Keeps broadcast-first (so a connected client still sees the banner even if storage is rejecting); the two storage writes are independent (durable terminal key + recovering-status clear vs. the `cf_think_submissions` row), so reordering them is safe. Does NOT adopt ai-chat's persist-before-broadcast ordering: ordering can't rescue a terminal-record write that itself fails, so that would gain no reconnect reliability under storage failure while losing the banner resilience. No behavior change when storage is healthy; existing exhaustion tests (which assert both the durable record and the interrupted submission row) cover the reorder.
Follow-up: ordering observation (broadcast vs persist on exhaustion)The observation is factually correct — ai-chat persists-then-broadcasts, Think broadcasts-then-persists — but the stated risk needs a correction, and the fix is narrower than "flip Think to match ai-chat". Ordering can't rescue a write that itself fails. If the terminal-record write rejects mid-deploy, the record is missing regardless of whether it ran before or after the broadcast — persist-first doesn't save it (the persist is the failing op). When storage is healthy, both orderings persist and broadcast fine. So flipping Think to persist-first gains no reconnect reliability under failure, while losing Think's deliberate guarantee that a connected client still sees the terminal banner when storage is rejecting (the broadcast touches no storage). The real (marginal) issue is intra-storage: Think did two storage writes in the order Fix ( No behavior change when storage is healthy; existing exhaustion tests (which assert both the durable record and the interrupted submission row) cover it. think still 554 tests; |
…nect replay (#1645) ai-chat only persisted a durable terminal record on recovery EXHAUSTION (`_exhaustChatRecovery`). A terminal non-exhaustion error — e.g. a provider 500 surfaced as a stream `error` part — was broadcast transiently and returned `status: "error"` to the response-hook drain loop, which neither cleared nor recorded it. A client disconnected at that moment never learned the turn failed and stayed frozen on reconnect (RESUME_NONE), the exact failure #1645 targets — ai-chat had only closed it for the exhaustion sub-case. Think already records on `error` (its `_fireResponseHook` → `_recordTerminalChatStatus`), so this also removes a cross-package parity gap. Fix: in the drain loop, record a durable terminal when `chatResult.status === "error"` (body = the error message). Recoverable failures (deploy/eviction/stall) don't reach the drain loop as `error` — they arrive as `aborted`, or not at all (isolate gone) with exhaustion recording its own terminal — so this can't pre-empt recovery. Test: drive a server-side turn that ends in a stream error, assert the durable record is written AND replays over the resume handshake on reconnect (terminal error frame, not a bare RESUME_NONE). Verified red without the fix (record is null). Changeset updated.
Follow-up: durably record non-exhaustion errors in ai-chat (review feedback)Validated — correct, and squarely within #1645's intent so I closed it. ai-chat only persisted a terminal record on recovery exhaustion ( Think already records on Fix: in the drain loop, record a durable terminal on Test: drive a server-side turn that ends in a stream error, then assert the durable record is written and replays over the resume handshake on reconnect (terminal error frame, not a bare ai-chat now at 565 tests; |
Summary
Fixes #1645. When a durable chat turn exhausts recovery (e.g. during a deploy / WebSocket reconnect storm) while no client is connected, the terminal error was only broadcast transiently. A client that connected afterward ran the standard resume probe, was told
RESUME_NONE, and the failed turn stayed frozen — it never learned the turn had ended.This affects both
@cloudflare/ai-chatand@cloudflare/think.Root cause
The shared
useAgentChatclient only surfaces a terminal{ done: true, error: true }frame asuseChat.errorwhen that frame arrives on the transport's stream reader — i.e. via the resume handshake. A bare broadcast frame received outside an active stream read is dropped.@cloudflare/ai-chathad no durable terminal record at all.@cloudflare/thinkpersisted one, but replayed it only as a bare on-connect frame that the client drops.So both presented the identical frozen-turn symptom.
Fix
Persist the terminal outcome durably and replay it over the resume handshake on the next reconnect:
This is the only path that becomes
useChat.erroron the real client. The record is retained across the ACK so concurrent reconnects (multiple tabs) each learn the outcome.Record lifecycle — cleared when superseded
The durable record is dropped on every path that makes a stale exhaustion irrelevant:
saveMessages/ auto-continuation with no client request in between (closes a gap whereai-chatonly cleared on a new client submit),chat-clearpath and the programmaticclearMessages()API — so a stale exhaustion can't replay onto a freshly-emptied chat.Cleanup
@cloudflare/thinkno longer emits the bare terminal frame in_buildIdleConnectMessages— it was dead for the real client and redundant with the handshake. The live "recovering…" status (Surface a live "recovering…" status to chat clients during durable recovery #1620) is still replayed there; it's a plain status frame the client handles on connect._recordChatTerminal/_clearChatTerminal/_pendingChatTerminal/_replayTerminalOnResume/_replayTerminalOnAck, keyed by the sharedCHAT_LAST_TERMINAL_KEY.Tests
Real Durable Object + real WebSocket tests driving the exact reconnect handshake the client sends. Every new guard was verified red without its fix:
saveMessages) turn succeeds — ai-chatWhy not a browser Playwright test?
Neither package has a harness that mounts
useAgentChatin a browser, and the gap lives at the protocol/transport layer. These real-DO + real-WS tests drive the exact client handshake and are the faithful equivalent; the resumed-stream →useChat.errorsurfacing is covered by existing transport tests. A browser-mount test is the only remaining inch of confidence and can be added as a follow-up.Verification
@cloudflare/ai-chat: 563 tests pass@cloudflare/think: 553 tests passpnpm run checkgreen (sherif + export checks + oxfmt + oxlint + typecheck, 92 projects)Changeset
.changeset/chat-terminal-replay-on-reconnect.md— both packages,patch.