Repository navigation
feat(think,ai-chat): live "recovering…" status to chat clients (#1620) + flaky-test retries - #1648
Conversation
…at clients (#1620) When a durable chat turn is interrupted (a deploy/eviction, or a stream-stall watchdog abort) and resumes, clients had no "in progress" signal — the turn looked frozen until it completed or a terminal error was replayed (#1619). This adds the remaining "progress" half: a live "recovering…" status, surfaced as a new `useAgentChat.isRecovering` flag. ## What changed - **Protocol:** new `cf_agent_chat_recovering` frame (`CHAT_MESSAGE_TYPES` in `agents/chat`, `MessageType` + an `OutgoingMessage` variant in ai-chat), payload `{ recovering: boolean, id? }`. - **Server (think + ai-chat):** `_setChatRecovering(active, requestId?)` broadcasts the frame AND persists a durable record, but only on a genuine transition (so a deploy/reconnect storm doesn't spam the wire). Set when a recovery continuation is scheduled (`_updateChatRecoveryIncident("scheduled")`); cleared on EVERY terminal outcome — completed/skipped/failed via the incident update, and exhausted + normal completion via `_recordTerminalChatStatus` (think) / `_exhaustChatRecovery` (ai-chat) — so the indicator can never spin forever. - **Connect hydration (think):** the recovering status is replayed in `_buildIdleConnectMessages`, so a client that joins mid-recovery reads the turn as working rather than frozen. (`@cloudflare/ai-chat` broadcasts the live signal but has no idle-connect hydration yet — deferred to #1645.) - **Staleness guard:** the durable record carries an `at` timestamp; a record older than the max recovery window (15m) is treated as not-recovering in both `_setChatRecovering` and hydration — so an abandoned recovery (DO went idle before it could exhaust) can't pin the indicator "on" forever or suppress a fresh signal. - **Client:** `useAgentChat` returns `isRecovering` (distinct from `isStreaming` — a recovering turn isn't producing tokens yet; most UIs treat `isStreaming || isRecovering` as "busy"). Set from the frame, cleared on a terminal response / stream-resume / chat-clear and on unmount. ## Telemetry note An earlier draft added a durable `getChatRecoveryEvents` query API + per-DO table (N2). It was dropped on review: it added a public RPC + a table to every agent for something the existing `chat:recovery:*` observability events + the app's analytics bridge already cover (and which the recovery design treats as the intended integration point). For recovery telemetry, subscribe to `chat:recovery:*` and route to your sink. ## Tests / docs / example - think: recovering broadcast + connect-hydration + clear-on-terminal. - ai-chat: durable recovering record set/clear. - react (Playwright): `isRecovering` set from the frame and cleared on resolve. - docs: `chat-agents.md` (recovering-status subsection + `isRecovering` in the `useAgentChat` table) and `packages/think/README.md`. - example: `examples/deploy-churn` renders a live "Recovering…" pill. Backward-compatible: clients that don't understand the frame ignore it; existing `useAgentChat` consumers are unaffected. Co-authored-by: Cursor <cursoragent@cursor.com>
We've been seeing intermittent flakiness across the test suites (real `wrangler dev` + network in e2e, browser/Playwright timing, Workers-runtime startup). - **Retries:** add `retry: 3` (matching the existing convention already used by `think`/`ai-chat` workers suites) to every leaf vitest config that lacked it — agents (workers/react/e2e/browser/webmcp/chat/x402/cli), ai-chat (react/e2e), think (e2e), codemode (+browser), shell, worker-bundler, voice (workers/react), voice-providers/telnyx, experimental/forever-chat, and the example suites. The aggregator root configs (`agents`/`ai-chat`/`voice` `vitest.config.ts`) only declare `projects:` and delegate to these, so retry lives in the leaves. Playwright configs already set `retries`. Retries only re-run failing tests, so a healthy run has zero overhead. - **Nightly e2e timeout:** raise both `.github/workflows/nightly.yml` jobs (`e2e-ai-chat`, `e2e-think`) from 15m/10m to `timeout-minutes: 30`, so a slow (but recovering) real-deploy/Playwright run + retries doesn't get killed mid-flight. No source changes; CI/test hygiene only. Co-authored-by: Cursor <cursoragent@cursor.com>
🦋 Changeset detectedLatest commit: 1fec7a4 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 |
|
/bonk do a deep review, look for edge cases, is this solid |
There was a problem hiding this comment.
Deep Review — recovering status (#1620) + flaky-test retries
Went through every changed file, traced all terminal paths, and looked for edge cases. This is solid work.
Architecture assessment
The design is clean: a single durable key (cf:chat:recovering) acts as the source of truth, set on "scheduled" transitions and cleared on every terminal outcome. The terminal coverage is thorough — I traced all paths to confirm:
_updateChatRecoveryIncidentcoversscheduled(set),completed/skipped/failed(clear)_exhaustChatRecoverycovers theexhaustedpath with a direct_setChatRecovering(false)call_recordTerminalChatStatus(think only) provides belt-and-suspenders: any turn-level terminal (normal completion, error, interruption) also clears
The transition guard (if (activeExisting) return for set, if (!existing) return for clear) is the right move — it prevents broadcast storms during deploy churn where recovery is re-detected many times. The staleness guard (15m max window check) is a smart safety net for abandoned DOs.
Edge cases I checked
-
Stale record after DO goes idle without terminal — Handled. The
attimestamp +CHAT_RECOVERY_MAX_WINDOW_MScheck treats old records as not-recovering in both_setChatRecovering(won't suppress a new signal) and_buildIdleConnectMessages(won't replay to connecting clients). Good. -
Double-clear / double-set — Both are idempotent. Setting when already active is a no-op; clearing when already clear is a no-op. Neither broadcasts spuriously.
-
Client connects mid-recovery (think) — Covered by
_buildIdleConnectMessageshydration. The recovering frame is replayed alongside the transcript. Mutual exclusivity with the last-terminal replay is natural since terminal clears recovering. -
Client connects mid-recovery (ai-chat) — Explicitly deferred to #1645. The comment in the code and the changeset both call this out. Live broadcast + clear still work regardless.
-
React hook cleanup —
isRecoveringis cleared on unmount (cleanup function setsfalse), on clear, on stream resume (the turn is now live), and on terminal response (data.done). No path where it can stick. -
Backward compat — Clients that don't understand
cf_agent_chat_recoveringignore it (standard WebSocket protocol behavior). ExistinguseAgentChatconsumers won't break sinceisRecoveringis additive.
One subtlety worth noting (not a blocker)
In _setChatRecovering, when clearing (active === false), the requestId parameter is ignored in favor of existing.requestId via the fallback requestId = requestId ?? existing.requestId. This means the recovering: false broadcast always carries the id from the stored record, which is the correct recovery-root id from when it was set. This is actually the right behavior — the clear should reference the same turn the set did — but it's worth knowing that the parameter is effectively unused on the clear path (the callers all pass undefined or omit it when clearing).
Test coverage
- Think: broadcast + connect-hydration + clear-on-terminal — covers the full lifecycle
- ai-chat: durable record set/clear — covers the persistence layer
- React (Playwright):
isRecoveringset from frame, cleared on resolve — covers the client contract
All three layers are tested. The test in think uses seedIncidentForTest + updateIncidentForTest to drive the state machine without needing a real model call, which is the right approach for a unit test.
Flaky-test retries
The retry: 3 additions are uniform across all leaf vitest configs that lacked it. Aggregator configs correctly delegate to leaves. The nightly timeout bump from 15m/10m to 30m is reasonable given retries + real deploy timing. No source changes in this commit — pure CI hygiene.
Minor observations (informational, not blocking)
- The
_setChatRecoveringimplementations in think.ts and ai-chat/index.ts are essentially identical (~30 lines). If the pattern gets any more complex in the future, it might be worth extracting to the sharedagents/chattoolkit, but for now the duplication is fine — both classes have different broadcast helpers and different connect-hydration strategies. - The deploy-churn example's recovering pill is a nice touch for demonstrating the feature. Clean Kumo design system usage.
LGTM. The feature is well-bounded, all terminal paths clear the indicator, the staleness guard prevents permanent stuck states, and the backward compatibility story is solid.
|
Review posted with approval on PR #1648. Here's the summary: This PR is solid. I traced all terminal paths and edge cases: What's good:
One subtlety noted (not a blocker): The The flaky-test retries are uniform and well-scoped — |
agents
@cloudflare/ai-chat
@cloudflare/codemode
hono-agents
@cloudflare/shell
@cloudflare/think
@cloudflare/voice
@cloudflare/worker-bundler
commit: |
Closes #1620.
Two things, bundled: the #1620 recovering-status feature (the substantive change) and a small test-infra pass (retries + nightly timeout) to address the flakiness we've been seeing.
1. Live "recovering…" status (#1620)
When a durable chat turn is interrupted (a deploy/eviction, or a stream-stall watchdog abort) and resumes, clients had no "in progress" signal — the turn looked frozen until it completed or a terminal error was replayed. #1619 fixed the terminal half (failed/exhausted turns surface + replay on connect). This is the remaining progress half: a live "recovering…" status.
Server
cf_agent_chat_recoveringframe —CHAT_MESSAGE_TYPESinagents/chat,MessageType+ anOutgoingMessagevariant in@cloudflare/ai-chat; payload{ recovering: boolean, id? }._setChatRecovering(active, requestId?)(think + ai-chat): broadcasts the frame and persists a durable record, but only on a genuine transition (so a deploy/reconnect storm doesn't spam the wire). Set when a recovery continuation is scheduled; cleared on every terminal outcome (completed/skipped/failed via the incident update; exhausted + normal completion via_recordTerminalChatStatus/_exhaustChatRecovery) so the indicator can never spin forever._buildIdleConnectMessages, so a client that joins mid-recovery reads the turn as working, not frozen. (@cloudflare/ai-chatbroadcasts the live signal but has no idle-connect hydration yet — deferred to ai-chat: chat-recovery exhaustion has no terminal-status hydration (disconnected client misses the terminal banner on reconnect) #1645.)attimestamp; a record older than the max recovery window (15m) is treated as not-recovering in both the setter and hydration — so an abandoned recovery (DO went idle before it could exhaust) can't pin the indicator on forever or suppress a fresh signal. This aligns with the max-window: a legitimate recovery always terminalizes (and clears) by then.Client
useAgentChatreturns a newisRecoveringflag — distinct fromisStreaming(a recovering turn isn't producing tokens yet); most UIs renderisStreaming || isRecoveringas "busy". Set from the frame; cleared on a terminal response / stream-resume / chat-clear / unmount.Telemetry note (scope decision)
An earlier draft also added a durable
getChatRecoveryEventsquery API + per-DO table (the "N2" item). Dropped on review: it added a public RPC + a table to every agent for something the existingchat:recovery:*observability events + the app's analytics bridge already cover (and which the recovery design treats as the intended integration point). For recovery telemetry, subscribe tochat:recovery:*and route to your sink.Backward-compatible
Clients that don't understand the frame ignore it; existing
useAgentChatconsumers are unaffected.Tests / docs / example
isRecoveringset from the frame, cleared on resolve.docs/chat-agents.md(recovering-status subsection +isRecoveringin theuseAgentChattable) andpackages/think/README.md.examples/deploy-churnrenders a live "Recovering…" pill.Changeset:
agents/@cloudflare/think/@cloudflare/ai-chatminor.2. Flaky-test retries + nightly timeout (second commit)
We've been seeing intermittent flakiness (real
wrangler dev+ network in e2e, browser/Playwright timing, Workers-runtime startup).retry: 3added to every leaf vitest config that lacked it (matching the convention already used bythink/ai-chatworkers suites): agents (workers/react/e2e/browser/webmcp/chat/x402/cli), ai-chat (react/e2e), think (e2e), codemode (+browser), shell, worker-bundler, voice (workers/react), voice-providers/telnyx, experimental/forever-chat, and the example suites. Aggregator root configs only declareprojects:and delegate, so retry lives in the leaves. Playwright configs already setretries. Retries only re-run failing tests → zero overhead on a healthy run..github/workflows/nightly.ymljobs (e2e-ai-chat,e2e-think) raised from 15m/10m →timeout-minutes: 30, so a slow-but-recovering real-deploy/Playwright run + retries isn't killed mid-flight.No source changes in this commit — CI/test hygiene only.
Test plan
npm run check— all 91 projects typecheck; format + lint cleanpackages/thinkunit suite (recovering-status set/clear/hydration tests)packages/ai-chatworkers suite + react Playwright (isRecovering)Made with Cursor