refactor(chat): run recovery continuations on Tasks - #2194
Conversation
🦋 Changeset detectedLatest commit: cb6dff7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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: |
Two confirmed gaps (pinned red-first in memory-limit.test.ts): 1. The driver's in-process dispatch retry converted a memory-limit reset into a silent success — the retry found the half-claimed run not due, returned void, and the wake was deleted without the breaker ever engaging. Memory-limit resets now defer to the alarm boundary like code-update resets: the isolate is condemned either way. 2. The run row outlives the breaker's queue-row policy: startup reconciliation re-derives due-now wakes from it, resurrecting a doomed run through backoff and past sealing. MemoryLimitContext now carries the striking job's identity, and Tasks applies the breaker to the run itself — demoted to the backoff wake on a strike (claim stripped so reconcile honors the deadline), terminally failed (task:failed, TaskMemoryLimitSealed) when the breaker seals. Generic for every definition: the striking run is the one that exhausted memory. Claude-Session: https://claude.ai/code/session_011QZUJztM1rMTsHEC7mbcbz
Replace root-agent recovery schedule rows with chained runs of the reserved __cf_internal_chat_recovery Task definition shared by AI Chat and Think. Initial attempts deduplicate by incident, delayed retries use step.sleep, and the existing bounded callback methods still detach at model handoff. Legacy and routed dynamic-agent schedules remain as compatibility shims. Make Tasks breaker-safe for recovery definitions: condemned-isolate failures escape journal retries, flagged framework definitions carry queue membership, and onMemoryLimit aligns or seals their authoritative run rows so startup cannot resurrect purged work. Think's submission sweep now inspects Tasks rather than Scheduler rows. AI Chat and Think require agents >=0.23.1, the release containing the shared definition and internal enqueue aperture.
Apply retain: false through one terminal cleanup path shared by completed, failed, and cancelled runs. A memory-limit seal now emits task:failed, removes the run and journal, cancels its wake, and releases its idempotency key. Make the breaker regression tolerate only the intentional workerd isolate reset and use per-attempt Durable Object names so Vitest retries cannot collide with durable rows from the failed attempt.
Preserve #2192's exact atomic helper shape while checking the Task transport through a separate same-RPC method. Align chat peer ranges with the pending changesets release batch, which publishes this work in agents 0.23.0.
0dbc1a3 to
310dfce
Compare
Upgrade existing Lifecycle job tables before reading the recovery-loop flag. Let Task wakes defer after one JobDriver attempt because ReplayStep owns their durable retry budget, including reconciliation of older wake rows. Keep pre-handoff chat failures on the current Task or schedule, and enqueue a replacement only after the bounded callback has handed off to the model turn.
When bounded Task dispatch returns before an attempt, preserve a durable late-memory-limit marker if that detached attempt later OOMs. The next alarm rethrows the canonical signal inside JobDriver so existing strike, backoff, and sealing policy remains authoritative. Mark detached job outcomes so their alarm is not treated as a clean breaker cycle, preserve marker wakes through startup reconciliation, and cover queued and warm attempts plus marker cleanup.
When work previously detached from an alarm settles without a memory reset, enqueue one no-op Task wake. That wake restores any authoritative run deadline and gives the existing JobDriver a clean alarm boundary to clear stale strikes. Late OOMs keep using the existing late-memory-limit marker.
Keep Lifecycle's deadman alarm armed while it joins promises registered at bounded job handoffs. Classify the whole dynamically growing alarm batch once: all clean work clears prior strikes, while any memory reset enters the existing breaker once with its captured executing job. Tasks now only registers attempts at its five-second handoff, and AI Chat and Think register post-handoff model work so Task and Scheduler transports share the same breaker. Remove the Task-specific late marker jobs and their state.
…g the alarm Lifecycle: `trackAlarmWork` no longer holds the alarm invocation open or coalesces physical re-arms. Handed-off work is classified when it settles: a memory reset records a strike against the job that handed it off, one strike per reset however many flows observe it, and strikes clear only once no handed-off work is outstanding and the last of it settled clean. Attribution follows the dispatch's async context and alarms in flight are counted, so overlapping invocations (tests drive alarm() by hand while the pool auto-fires the physical alarm) stay correct. Tasks: drop the per-definition recoveryLoop grouping and its composition root plumbing (setTaskRecoveryLoopDefinitionResolver, Agent._recoveryLoopTaskDefinitions, the recoveryLoop option on _registerInternalTaskDefinition). onMemoryLimit acts only on the run whose wake struck, stripping the claim and pushing the deadline so the reclaim still sees an interrupted attempt. onJob hands off one canonical promise per attempt, #syncWake skips same-values pushes, and runAttached actually attaches instead of racing the warm start. Chat: both hosts run the bounded recovery callbacks through the shared dispatchChatRecoveryToHandoff and register the recovery Task with hooks keyed by callback name. Design docs, changeset, stale Scheduler/recovery-engine comments and tests updated; new tests pin one-strike-per-reset and the startup re-arm. Claude-Session: https://claude.ai/code/session_011DVPXpp9SyXM1ChxvkQCTB
Two correctness fixes from Devin Review, both against the previous commit's job-driver.ts: - Overlapping alarm() invocations attributed a memory-limit strike to whichever job a shared instance field (#executingRow) last pointed at, not the job that actually struck — the struck job could escape backoff/sealing while an unrelated one absorbed it. Replaced with an AttributedPlatformFailure carrying the row out of the throw itself, correct by construction regardless of overlap. - The quiescence-clear check for the strike counter read stale snapshots of #alarmsInFlight/#outstandingAlarmWork depending on which of two transitions (an alarm ending, a handoff settling) ran first, so a strike recorded in the gap between checks could never clear. Fixed by having each transition re-check both counters fresh, with no yield point between the mutation and the check on either side. Fixing the second issue introduced a regression caught by the full suite: the quiescence check started running unconditionally in runAlarm's finally, including right after a strike was just recorded, immediately clearing what had just been set. Restored the placement-based guard (only a clean pass triggers the check) that the prior design relied on. New regression test isolates the attribution fix from Tasks' own active-attempt tracking, which independently and correctly re-tracks a Task run an overlapping alarm re-dispatches and was masking the bug in two earlier attempts at this test. Claude-Session: https://claude.ai/code/session_011DVPXpp9SyXM1ChxvkQCTB
Third correctness fix from Devin Review against job-driver.ts, following the same shape as the two already fixed: a strike could be fully recorded (durable counter written, isolate reset scheduled via setTimeout(0)), and then a slower, unrelated clean handoff still outstanding at that moment could settle in the gap before the reset actually lands, find the alarm domain quiescent, and clear what was just recorded. Fixed with #strikeRecordedThisIsolate: once any flow records a strike in this isolate, no later settlement may clear the durable counter for the rest of this isolate's life. The isolate always resets shortly after a recorded strike, so the next genuinely clean cycle runs in a fresh isolate with a fresh JobDriver and the flag back at its default — there is no cross-isolate state to reset. New regression test (oomBeforeCleanSibling) orders a run's own strike distinctly before a separately tracked clean sibling; verified against the pre-fix code that it exercises the intended scenario and passes correctly with the fix. I could not force this specific narrow race window (bounded by workerd's own near-instant setTimeout(0) teardown scheduling) to reproducibly fail pre-fix within this test harness after several timing attempts, so this is a positive regression guard rather than a failing-then-passing proof like the other two fixes in this PR. The fix itself is verified by direct code tracing, matching Devin's own independent diagnosis of the same race. Claude-Session: https://claude.ai/code/session_011DVPXpp9SyXM1ChxvkQCTB
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 1 new potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| // Set before the first await below: closes the window as tightly as | ||
| // possible against a still-outstanding clean sibling reaching quiescence | ||
| // and clearing what this event is about to record. | ||
| this.#strikeRecordedThisIsolate = true; |
There was a problem hiding this comment.
Replaces the imperative _registerInternalTaskDefinition/_internalTaskDefinitions mechanism on Agent (a Map populated via a protected method, five near-identical one-line wrapper methods across AIChatAgent and Think just to call it) with a plain `register(name, definition)` method directly on Tasks. Framework code calls this.tasks.register(...) once per reserved name from its own constructor; it throws if the name lacks the `__cf` prefix or is already registered, so registration composes correctly no matter how many subclass layers exist or what any of them do with their own taskDefinitions field — it no longer depends on Agent's own field-override machinery at all. Agent's constructor keeps setTaskDefinitionResolver, but its only remaining job is bridging the end user's own overridable taskDefinitions field, which genuinely cannot be read at Tasks-construction time (a further-downstream subclass's field initializer runs only after every constructor up the chain returns) — register() has no such problem since it's called eagerly from each host's own constructor, after this.tasks already exists. design/rfc-fibers.md updated to describe the new mechanism (an adversarial review caught the stale reference to the old resolver-based path). New tests cover register()'s own validation branches directly (missing __cf prefix, empty name, duplicate registration, collision with a constructor-declared definition, and that a registered definition is reachable only through the internal aperture, never the public run()) — the only real definitions that previously exercised it (chat turn, chat recovery, messenger reply) never hit any of its failure paths. No public API surface changes: docs/agents/tasks.md and the existing changeset need no edits, matching every other __DO_NOT_USE_WILL_BREAK__- style internal aperture on Tasks. Claude-Session: https://claude.ai/code/session_011DVPXpp9SyXM1ChxvkQCTB
🔴 agents import sizesMeasured 289 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 (98)
All 287 current runtime imports
Reported by agent-think[bot]. |
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 1 new potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| this.#store.write( | ||
| `UPDATE cf_agents_task_runs | ||
| SET generation = NULL, | ||
| next_at = CASE | ||
| WHEN next_at IS NULL OR next_at < ? THEN ? | ||
| ELSE next_at | ||
| END, | ||
| updated_at = ? | ||
| WHERE run_id = ? | ||
| AND state IN ('pending', 'waiting', 'running')`, | ||
| [context.nextTime, context.nextTime, now, runId] | ||
| ); |
cf_agents_jobs (and its recovery_loop column) has never shipped in a release — the whole table was introduced in this same unreleased branch (#2175, then extended with recovery_loop in #2190/#2194). No deployed Durable Object can have this table without the column already present: CREATE TABLE IF NOT EXISTS already includes it in the same statement. The pragma_table_info probe + ALTER TABLE fallback was defending against a schema history that cannot exist yet. Removes the dead branch, its regression test (which only proved the migration path itself, not anything a real caller depends on), and the two doc/changeset sentences describing it. Claude-Session: https://claude.ai/code/session_011DVPXpp9SyXM1ChxvkQCTB
…heduler bridge Tasks now mirrors a routed sub-agent's run deadline to the root's job queue while the run row and step journal stay put: only the root owns the physical alarm, so only the wake needs to cross that boundary. onRoute handles syncWake/dispatch/memoryLimit; a routed strike forwards to the owning facet's own onAlarmMemoryLimit hook, the same bridge Scheduler already used, since the facet's own Lifecycle never observes the root's alarm directly. AIChatAgent and Think's _enqueueChatRecovery now always uses Tasks, removing the parentPath-gated Scheduler fallback and the now-dead chatRecoverySchedulePolicy/chatRecoveryRedeferPolicy helpers. Claude-Session: https://claude.ai/code/session_011DVPXpp9SyXM1ChxvkQCTB
…recovery-on-tasks
Fixes a typecheck failure in packages/ai-chat/src/tests/tsconfig.json CI caught: the test-only OOM Task definition added in the routed-wake work needed the child's own constructor to register it, which used AgentContext without importing it. Claude-Session: https://claude.ai/code/session_011DVPXpp9SyXM1ChxvkQCTB
- onRoute's "dispatch" case awaited #executeRun directly, bypassing onJob's active-run claim refresh and its five-second dispatch budget. A routed run already active on the facet lost its wake when the root's mirror job settled with no reschedule, and a long-running routed handler blocked the root's whole alarm cycle. Extracted the shared bounded-dispatch logic into #dispatchRun, used by both onJob locally and onRoute's "dispatch" case, whose wake outcome now flows back as the RPC's own return value instead of being discarded. - onMemoryLimit only forwarded a routed run's SEALED strike to its owning facet. A non-sealed strike backed off the root's mirror job but left the facet's own claim (generation, next_at) untouched; any facet startup before the backoff elapsed read that claim as an interrupted attempt and reconciled it due again now, resurrecting the run through the breaker. Forward every strike, sealed or not. Verified the backoff fix is real: reverting to sealed-only forwarding reproduces the failure (generation stays set) in the new "backs off a routed facet's own claim" test. The active-run and dispatch-budget fixes reuse #dispatchRun verbatim from the local path, already covered by the existing local dispatch tests; a dedicated routed-side regression test for the concurrent-active and >5s-budget cases was not added given the cost of reliably constructing those races across a real DO-RPC boundary in this harness. Claude-Session: https://claude.ai/code/session_011DVPXpp9SyXM1ChxvkQCTB
…ancel stale wakes on facet deletion Two more Devin-flagged bugs in the routed-Tasks work: - Routed tasks bypassed the memory breaker. A routed dispatch that exceeded the five-second budget called trackAlarmWork from inside onRoute on the facet, but that call only attaches to a live alarm invocation's AsyncLocalStorage scope — onRoute runs in response to an incoming RPC, not the facet's own alarm, so the call silently did nothing. Every redispatch of a routed run goes through this same path (root owns the physical alarm), so a routed run that regularly overran the budget could OOM repeatedly without the breaker ever seeing it. Moved the budget race to the root side instead: root now races its own await of the routed RPC call, and on budget keeps the still-pending call itself tracked via trackAlarmWork, which works because onJob runs inside root's own live alarm scope. The facet's onRoute dispatch handler no longer needs its own budget or tracking at all — it just fully awaits, since the call keeps running on the facet regardless of whether root is still waiting on it. Verified this is actually safe with a deployed repro (not simulated via ctx.abort(), which is deliberately deferred and wouldn't reject an in-flight caller): a callee DO whose isolate is killed by the platform's real memory-limit enforcement while mid-flight on an RPC call reliably rejects the caller's pending promise with the platform's own "exceeded its memory limit" text — exactly what the breaker already matches on. Confirmed across 3 trials against a real deployment, then deleted the repro worker. Added a regression test whose failure lands after root's own budget elapses (a real >5s delay, not a simulated abort) and confirmed it fails when reverted to the old (silently-inert) tracking call. - Deleting a facet left its routed Task wake mirrored on the root forever, since only Scheduler's routed rows were cleaned up on facet-subtree deletion. Added the same __DO_NOT_USE_WILL_BREAK__cleanupRoutePrefix aperture to Tasks, mirroring Scheduler's, and wired it into the same cleanupPrefix call site. Claude-Session: https://claude.ai/code/session_011DVPXpp9SyXM1ChxvkQCTB
…; retry a failed post-handoff redefer Two more Devin-flagged bugs from the latest routed-Tasks review pass: - Once root started racing its own await of a routed dispatch (the RPC-tracking fix earlier today), a budget win now returns undefined as root's own job outcome. JobDriver deletes the mirror job on an undefined outcome — but only when the row is still marked "running" from that same dispatch. #executeRun now pushes the claim deadline via #syncWake immediately after claiming, before the handler runs: that push clears the row's in-flight marker (job-queue's own "newer durable intent wins" guard), so root's later stale undefined outcome no-ops against it instead of deleting a still-live claim. A hung or interrupted routed attempt keeps its alarm either way now, independent of whether this specific attempt happens to settle cleanly, fail, or never resolve at all. - dispatchChatRecoveryToHandoff swallowed a failed post-handoff redefer entirely. The Task that dispatched it has already settled by the time redefer runs, so nothing else owned that incident — a transient failure to enqueue the replacement abandoned recovery silently. Wrapped it in a bounded retry (tryN, 3 attempts) and surface the final failure through the same onDetachedError channel an unowned detached failure already uses, rather than a bare swallowed catch. Both verified non-vacuous: reverting each fix reproduces the exact failure Devin described (the mirror job actually gets deleted; the redefer failure is actually silent) in new regression tests, then passes again restored. The claim-mirroring test uses a real 6.5s delayed failure, not a simulated one. Claude-Session: https://claude.ai/code/session_011DVPXpp9SyXM1ChxvkQCTB
…duplicate runs The redefer retry added last commit (tryN, 3 attempts) has its own bug: Tasks acceptance can throw after already durably inserting the run row — most likely on the wake-mirror push, not the insert itself — so a rejected enqueue does not prove nothing was created. Retrying the same unkeyed "redefer" enqueue (intentionally unkeyed for a genuinely new attempt, per chatRecoveryTaskRunOptions) could create up to three replacement runs for one incident instead of joining the first. dispatchChatRecoveryToHandoff now generates one dedupe key per failure and passes it to every retried redefer call; chatRecoveryTaskRunOptions keys the run by it (runId) when supplied, so a retry after a partial success joins that same row instead of duplicating it. Verified non-vacuous: reverting chatRecoveryTaskRunOptions's use of the key reproduces exactly the gap in the new "keys the run by dedupeKey" test. Claude-Session: https://claude.ai/code/session_011DVPXpp9SyXM1ChxvkQCTB
…sting run The dedupe fix from the last commit prevents a retried enqueue from creating a duplicate run, but joining the existing row alone doesn't help if that row's wake was never durably pushed in the first place. #accept can throw after already inserting the run — most likely on the wake-mirror #syncWake call itself, not the insert — so a retry that finds the existing row and returns accepted:false was reporting success against a run nothing would ever wake again. The join branch now calls #syncWake before returning, for every caller (runId or idempotencyKey match), not just chat recovery's retried redefer — any Tasks caller retrying acceptance after a partial failure benefits the same way. Cheap in the common case (#syncWake already no-ops when the mirror already matches). Verified non-vacuous: reverting the added #syncWake call and rerunning the new "repairs a missing wake mirror" capability test reproduces the gap exactly — the mirror job stays missing after the retry joins the run. Claude-Session: https://claude.ai/code/session_011DVPXpp9SyXM1ChxvkQCTB
…imit bridge Scheduler's routed memory-limit compatibility bridge existed to deliver a sealed strike to a dynamic agent whose chat-recovery schedule row was purged, back when routed chat recovery ran on Scheduler. Chat recovery now always uses Tasks (this PR), which has its own routed memory-limit bridge (setTaskRoutedMemoryLimitHandler), so this path carries no live traffic. Removed setSchedulerRoutedMemoryLimitHandler, the WeakMap backing it, the "memoryLimit" SchedulerRouteMessage variant, and Scheduler's onMemoryLimit/onRoute handling of it, plus Agent's wiring. Deliberately left in place: LEGACY_RECOVERY_LOOP_CALLBACKS' migration of pre-existing legacy schedule rows (still needed regardless of routing, so the local alarm breaker still counts them), and MemoryLimitContext.purgedRecoveryLoopJobs itself (now unread by any capability, but a generic hook a future routed capability with purge-as-a-pack semantics could still use — Tasks backs off one run at a time instead). Full suites still pass (2012 agents, 661 ai-chat, 888 think). Claude-Session: https://claude.ai/code/session_011DVPXpp9SyXM1ChxvkQCTB
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
4 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| * the owning capability and delete this bridge. | ||
| */ | ||
| async onMemoryLimit(context: MemoryLimitContext): Promise<void> { | ||
| if (!context.sealed) return; |
There was a problem hiding this comment.
🟡 Legacy child recovery never terminalizes
When a migrated routed recovery schedule seals after repeated memory resets, missing onMemoryLimit routing leaves its child chat active. Users remain stuck in recovery.
Prompt for agents
Preserve terminal memory-limit delivery for routed Scheduler recovery rows that can survive an upgrade. Existing _chatRecoveryContinue and _chatRecoveryRetry rows remain in the root Lifecycle queue with owner_path and recoveryLoop set. If one seals the breaker, the root purges it, while the recovery incident lives on the dynamic-agent owner. Restore an equivalent routed sealing bridge across packages/agents/src/schedules/scheduler.ts and packages/agents/src/index.ts, or add a migration/drain mechanism that guarantees no routed legacy recovery row can still execute before removing the bridge.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
This is a known, deliberate tradeoff, not an oversight — flagged before removing the bridge and confirmed. You're right about the exact consequence: a legacy routed _chatRecoveryContinue/_chatRecoveryRetry row, if one still exists on a fleet that hasn't restarted since before this PR, purges silently on seal instead of notifying its owning facet, since Scheduler no longer forwards that strike.
Restoring the bridge or adding a formal drain guarantee is more machinery to keep a narrowing, already-legacy population correctly draining, for a scenario (a routed schedule row surviving from before Tasks took over routed chat recovery, AND that specific row hitting the breaker's seal threshold) that's already rare and only gets rarer. The call here was to accept it rather than carry the compatibility bridge indefinitely — leaving it not resolved so it's visible to reviewers rather than resolving it myself.
Base
Rebased onto
mainafter #2190 merged. The rebase also incorporates #2191's Tasks write optimizations and #2192's deterministic Durable Object test helpers.Why this changed
Root AI Chat and Think recovery still used Scheduler rows even though Tasks had become the durable replay primitive. Moving recovery onto Tasks exposed a broader alarm-lifetime issue: a job can return at a bounded handoff so Lifecycle can continue its due batch while the promise it started is still running. A memory reset from that promise still belongs to the alarm that started it, but it no longer reaches the alarm's call stack unless Lifecycle retains that relationship.
This is an alarm-domain concern, not Task or Scheduler policy.
Final architecture
Lifecycle owns bounded alarm work
trackAlarmWork(promise)lets a bounded job handoff register the work it started with the current alarm.Tasks is only an adapter to that boundary
retain: falseremoves completed, failed, and cancelled runs, journals, wakes, and idempotency keys.Chat recovery uses Tasks without losing Scheduler compatibility
__cf_internal_chat_recoveryTask runs._chatRecoveryContinueand_chatRecoveryRetryreturn at model handoff, preserving queue liveness.tasks.list().Upgrade and breaker correctness
cf_agents_jobstables are upgraded idempotently withrecovery_loopbefore any query references it.step.doimmediately.What we found during review
The first downstream approach used Task-specific late-OOM and clean-settlement marker jobs. It handled one detached Task, but it made Tasks responsible for alarm policy and failed when several promises belonged to the same alarm: one clean Task could clear strikes before a sibling reported OOM. It also left Scheduler-originated recovery as a separate case.
The final version removes those Task marker jobs and moves promise retention and group classification into Lifecycle. Tasks, Scheduler-driven chat recovery, and future alarm-backed capabilities now use one small core boundary instead of reimplementing breaker behavior downstream.
Compatibility and release
_chatRecoveryContinue/_chatRecoveryRetryScheduler rows still dispatch through the retained methods and drain normally.agents >=0.23.0, the pending changesets release batch containing the shared runtime support.Verification
Current head:
56e4635f.git diff --checkpasses.Regression coverage in the branch includes: