fix(think,ai-chat): wall-clock-keyed-to-progress recovery budget + alarm debounce (#1637)#1638
Merged
Merged
Conversation
🦋 Changeset detectedLatest commit: b8092ef 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: |
…unce (#1637) Recovery budget no longer keyed primarily on raw attempt count (wrong under deploy churn — one rollout fires several alarms). Now: - PRIMARY: no-progress wall clock (5 min) keyed to `lastProgressAt`, which resets on every progress-bearing attempt — a turn that keeps producing content survives churn indefinitely; a stuck turn seals within 5 min. - DEBOUNCE: alarms within 30s collapse into one attempt (a rollout's reconnect storm is one logical interruption, not N). - SECONDARY: attempt cap raised to 10 (default) as a high backstop; resets on progress. - ABSOLUTE: existing 15-min incident-age ceiling kept as the non-resetting final stop. Adds `lastProgressAt` to the incident, a `nowMs` test-clock injection, and an `ageIncidentForTest` harness; reworks the budget tests for debounce and adds no-progress-window + debounce tests. think-side only — ai-chat mirror and the production-time (reconnect-immune) progress signal are the next commits. Co-authored-by: Cursor <cursoragent@cursor.com>
) Same budget redesign as the think commit: lastProgressAt + 5-min no-progress window (primary) + 30s alarm debounce + secondary cap raised to 10 + the existing 15-min absolute ceiling, plus nowMs test-clock injection and an ageIncidentForTest harness. Budget tests reworked for debounce; no-progress and debounce tests added. Full ai-chat suite green (478). Co-authored-by: Cursor <cursoragent@cursor.com>
…onnect-immune) (#1637) The no-progress window needs a growth-based, reconnect-immune progress signal: the persist-time bump fired on client reconnects (resume-ack -> _persistOrphanedStream) and on re-persisting unchanged content, so a reconnecting-but-stuck turn would reset `lastProgressAt` forever and dodge the timeout. Move the bump to `_storeChunkDurably`'s flush (production time): it fires only on genuinely new durable content and never on reconnects/recovery re-persists. `_storeChunkDurably` is now async (awaited in the stream loop; per-flush, not per-chunk). Adds a reconnect-immunity test (flush advances progress; an orphan re-persist does not). ai-chat's equivalent (its different streaming model) is the next commit. Co-authored-by: Cursor <cursoragent@cursor.com>
…ct-immune) (#1637) ai-chat equivalent of the think production-signal commit. Its streaming model has no _storeChunkDurably, so the bump goes on production milestones in the single chunk chokepoint `_storeStreamChunk` (text-start / reasoning-start / tool-input-available / tool-output-available|error|denied) via `_maybeBumpRecoveryProgress` — fires only on genuinely new streamed content, never on reconnects/recovery re-persists. `_storeStreamChunk` (and the plaintext `_broadcastTextEvent`) are now async; removed the persist-time bump from `_persistOrphanedStream`. Adds a reconnect-immunity test. Full ai-chat suite green (479). Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…ityForTest The progress-bump production-time move made _storeChunkDurably async (Promise<void>), but probeToolResultDurabilityForTest still cast it as returning void, so its store() calls discarded the promise and let _bumpChatRecoveryProgress' storage get/put float as a fire-and-forget rejection. Type the cast as Promise<void> and await all store() calls so storage writes settle deterministically (matches the sibling probeProgressReconnectImmunityForTest helper). Co-authored-by: Cursor <cursoragent@cursor.com>
threepointone
force-pushed
the
feat/recovery-wall-clock-budget
branch
from
June 1, 2026 02:01
76205cb to
b8092ef
Compare
Merged
threepointone
added a commit
that referenced
this pull request
Jun 1, 2026
…en up (#1631) Two paths could throw away a partial assistant message holding completed, often non-idempotent tool results — the most valuable, least-reproducible state in a turn: 1. Framework budget exhaustion sealed the turn (terminal status + banner) BEFORE the orphaned stream was persisted, so settled tool results were discarded and re-run on the next message. Exhaustion now persists the settled partial first, reusing the normal path's gating so it can't duplicate an already-saved partial. (This now also covers N1/#1638's wall-clock and no-progress exhaustion paths, not just the attempt cap.) 2. A subclass onChatRecovery returning { persist: false } silently dropped the settled partial. Settled work is now NEVER dropped: { persist: false } only suppresses persistence of a partial that has nothing settled to lose; a partial carrying settled tool results is persisted regardless. An app can no longer accidentally discard completed work, and never needs { persist: true } just to stay safe. A safe default beats a warning about an unsafe one (R1). Adds _shouldPersistOrphanedPartial / _partialHasSettledToolResults helpers. Applied identically to @cloudflare/think and @cloudflare/ai-chat. Rebased onto main (dropping the already-merged #1633 commit) and adapted the exhaustion test to N1/#1638's alarm-debounce (age the seeded incident past the debounce window so the wake counts as a genuine attempt and exhausts). Co-authored-by: Cursor <cursoragent@cursor.com>
3 tasks
threepointone
added a commit
that referenced
this pull request
Jun 1, 2026
…en up (#1631) Two paths could throw away a partial assistant message holding completed, often non-idempotent tool results — the most valuable, least-reproducible state in a turn: 1. Framework budget exhaustion sealed the turn (terminal status + banner) BEFORE the orphaned stream was persisted, so settled tool results were discarded and re-run on the next message. Exhaustion now persists the settled partial first, reusing the normal path's gating so it can't duplicate an already-saved partial. (This now also covers N1/#1638's wall-clock and no-progress exhaustion paths, not just the attempt cap.) 2. A subclass onChatRecovery returning { persist: false } silently dropped the settled partial. Settled work is now NEVER dropped: { persist: false } only suppresses persistence of a partial that has nothing settled to lose; a partial carrying settled tool results is persisted regardless. An app can no longer accidentally discard completed work, and never needs { persist: true } just to stay safe. A safe default beats a warning about an unsafe one (R1). Adds _shouldPersistOrphanedPartial / _partialHasSettledToolResults helpers. Applied identically to @cloudflare/think and @cloudflare/ai-chat. Tests: - Unit (think + ai-chat): exhaustion preserves a settled TOOL RESULT (not just text); { persist: false } never drops settled tool results, and is honored for a text-only partial. (Exhaustion test aged past N1/#1638's alarm-debounce so the wake counts as a genuine attempt and actually exhausts.) - E2E (think, real SIGKILL): a recordStep loop whose onChatRecovery returns { persist: false, continue: false } is killed mid-turn; after recovery the settled tool results produced before the kill are still in the durable transcript (R1) and the turn does not continue. (ThinkPersistFalseE2EAgent.) NOTE on coverage: the EXHAUSTION-with-prior-settled-work path stays unit-tested (not e2e) on purpose — under N1's budget, settling a tool result IS forward progress that resets the budget and prevents exhaustion, so that scenario can't be forced deterministically under real churn. Rebased onto main (dropping the already-merged #1633 commit). Co-authored-by: Cursor <cursoragent@cursor.com>
threepointone
added a commit
that referenced
this pull request
Jun 1, 2026
…en up (#1631) (#1634) Two paths could throw away a partial assistant message holding completed, often non-idempotent tool results — the most valuable, least-reproducible state in a turn: 1. Framework budget exhaustion sealed the turn (terminal status + banner) BEFORE the orphaned stream was persisted, so settled tool results were discarded and re-run on the next message. Exhaustion now persists the settled partial first, reusing the normal path's gating so it can't duplicate an already-saved partial. (This now also covers N1/#1638's wall-clock and no-progress exhaustion paths, not just the attempt cap.) 2. A subclass onChatRecovery returning { persist: false } silently dropped the settled partial. Settled work is now NEVER dropped: { persist: false } only suppresses persistence of a partial that has nothing settled to lose; a partial carrying settled tool results is persisted regardless. An app can no longer accidentally discard completed work, and never needs { persist: true } just to stay safe. A safe default beats a warning about an unsafe one (R1). Adds _shouldPersistOrphanedPartial / _partialHasSettledToolResults helpers. Applied identically to @cloudflare/think and @cloudflare/ai-chat. Tests: - Unit (think + ai-chat): exhaustion preserves a settled TOOL RESULT (not just text); { persist: false } never drops settled tool results, and is honored for a text-only partial. (Exhaustion test aged past N1/#1638's alarm-debounce so the wake counts as a genuine attempt and actually exhausts.) - E2E (think, real SIGKILL): a recordStep loop whose onChatRecovery returns { persist: false, continue: false } is killed mid-turn; after recovery the settled tool results produced before the kill are still in the durable transcript (R1) and the turn does not continue. (ThinkPersistFalseE2EAgent.) NOTE on coverage: the EXHAUSTION-with-prior-settled-work path stays unit-tested (not e2e) on purpose — under N1's budget, settling a tool result IS forward progress that resets the budget and prevents exhaustion, so that scenario can't be forced deterministically under real churn. Rebased onto main (dropping the already-merged #1633 commit). Co-authored-by: Cursor <cursoragent@cursor.com>
threepointone
added a commit
that referenced
this pull request
Jun 1, 2026
…sted payload (#1631) Lets products build a terminal-state policy without re-deriving anything: - ChatRecoveryContext (onChatRecovery) gains recoveryRootRequestId — the stable request id for the whole continuation chain, the right key for per-incident budget tracking / fresh-incident detection (no re-deriving from message IDs). - ChatRecoveryExhaustedContext (onExhausted) gains recoveryRootRequestId, terminalMessage (exact user-facing text), partialText/partialParts (what the turn produced before it was given up on), and streamId/createdAt — enough to render/persist a terminal banner AND emit correlated terminal telemetry (msSinceTurnStart, stream correlation) directly. streamId + createdAt were added after verifying the payload against the actual consumer (g3's _emitExhaustedRecovery): it reads both from the recovery context for telemetry, and they already exist on ChatRecoveryContext (the Pick source), so adding them to the exhausted context is additive and unblocks re-homing the exhaustion handler onto onExhausted with zero re-derivation (D4). Shared types in `agents`; wired through think + ai-chat (_exhaustChatRecovery now receives streamId + createdAt). Test agents capture the exhausted context; tests assert both contexts (incl. streamId + createdAt) in both packages. Rebased onto main (dropping the merged #1633/#1634/#1635 commits); adapted the exhausted-ctx test to N1/#1638's alarm-debounce and gave the think harness an explicit return shape (the context's MessagePart[] over-instantiates the RPC stub type). Co-authored-by: Cursor <cursoragent@cursor.com>
Merged
3 tasks
threepointone
added a commit
that referenced
this pull request
Jun 1, 2026
…sted payload (#1631) (#1636) Lets products build a terminal-state policy without re-deriving anything: - ChatRecoveryContext (onChatRecovery) gains recoveryRootRequestId — the stable request id for the whole continuation chain, the right key for per-incident budget tracking / fresh-incident detection (no re-deriving from message IDs). - ChatRecoveryExhaustedContext (onExhausted) gains recoveryRootRequestId, terminalMessage (exact user-facing text), partialText/partialParts (what the turn produced before it was given up on), and streamId/createdAt — enough to render/persist a terminal banner AND emit correlated terminal telemetry (msSinceTurnStart, stream correlation) directly. streamId + createdAt were added after verifying the payload against the actual consumer (g3's _emitExhaustedRecovery): it reads both from the recovery context for telemetry, and they already exist on ChatRecoveryContext (the Pick source), so adding them to the exhausted context is additive and unblocks re-homing the exhaustion handler onto onExhausted with zero re-derivation (D4). Shared types in `agents`; wired through think + ai-chat (_exhaustChatRecovery now receives streamId + createdAt). Test agents capture the exhausted context; tests assert both contexts (incl. streamId + createdAt) in both packages. Rebased onto main (dropping the merged #1633/#1634/#1635 commits); adapted the exhausted-ctx test to N1/#1638's alarm-debounce and gave the think harness an explicit return shape (the context's MessagePart[] over-instantiates the RPC stub type). Co-authored-by: Cursor <cursoragent@cursor.com>
This was referenced Jun 1, 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
1 participant
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.
Closes #1637.
Stack / base
Built on the compaction-immune progress counter from #1633 (#1628), so this PR's base is
fix/chat-recovery-progress-monotonic, notmain— GitHub will retarget tomainonce #1633 merges. It's the structural follow-up in the #1631 recovery hardening series.Problem
After #1623's bounded recovery, the budget is capped primarily by attempt count (
attempt > maxAttempts) plus a 15-min incident-age ceiling. Under continuous deploys the attempt count is the wrong primary bound, and a healthy in-flight turn gets sealed while still advancing.Producer-side evidence (customer session): one prompt → 23 successful LLM calls / 16.8k output tokens / ~6 min, 0/23 errored, 0 MB (not OOM) — yet recovery exhausted and sealed. Why:
Change
Budget is now wall-clock-keyed to forward progress, attempt cap demoted to a secondary backstop:
lastProgressAtresets on every progress-bearing attempt; exhaust only whennow - lastProgressAt > 5min. A turn that keeps producing content survives churn indefinitely; a stuck turn seals within 5 min.TaskSubAgent/sub-agent recovery path (extends Think).maxAttempts.Progress signal moved to production time (required, not cosmetic)
The no-progress window only works if "progress" means new content was produced. The previous bump fired at persist time — which also fires on a client reconnect (
_handleStreamResumeAck→_persistOrphanedStream) and on re-persisting unchanged content. Either would resetlastProgressAtwithout real progress, so a reconnecting-but-stuck turn would dodge the timeout forever. The bump now happens at production time:_storeChunkDurably(_storeChunkDurablyis now async)._storeChunkDurably): on production milestones (text-start/reasoning-start/tool-input-available/tool-output-*) in the single chunk chokepoint_storeStreamChunk.Both are growth-based and immune to reconnects/recovery re-persists. Builds on #1633's durable, compaction-immune counter.
Tests
nowMstest-clock injection + anageIncidentForTestharness make timing deterministic). The defaultmaxAttemptsassertion updated 6 → 10.Verification
tscclean (think + ai-chat);oxlintclean;oxfmtapplied.Test plan
examples/deploy-churn+ the customer's chaos harness (the acceptance test: a healthy multi-tool turn survives 90s-ish churn; a genuinely stuck turn seals at ~5 min)Made with Cursor