fix(agents,think,ai-chat): count a sub-agent's progress as the orchestrating parent's recovery progress (N9)#1641
Conversation
…trating parent's recovery progress (N9) A parent turn whose work is "run a sub-agent and await its result" produced no recoverable content of its own, so under deploy churn the PARENT's own chat-recovery no-progress window could exhaust while the child was still healthily streaming — abandoning the turn as `interrupted` and collecting an interrupted result even though the child went on to complete. Reproduced by the `examples/deploy-churn --mode subagent` harness: the parent exhausted at `attempt 6/6` with `progress: 1` while the child self-healed all 30 steps. Root cause: `_forwardAgentToolStream` (base Agent) only relayed child chunks to the parent's connections; it never touched the parent's durable recovery progress marker (which lives in the chat layer, not base Agent). Fix: add a base-Agent seam `_onAgentToolStreamProgress()` (no-op by default), invoked by `_forwardAgentToolStream` once per read iteration that actually forwarded >=1 child chunk. Think and AIChatAgent override it to bump their recovery-progress marker (throttled in-memory to 5s, reset per isolate so the first forwarded chunk after any restart always credits). Covers both the live `runAgentTool` path and the recovery `_reconcileAgentToolRuns` re-attach path. The bump is best-effort (wrapped so a failure never breaks the child stream). Design guard: credit is granted only on a genuinely-forwarded chunk — a silent/hung child forwards nothing, so the parent still exhausts on its own no-progress timer and a stuck sub-agent can never pin recovery open forever. This completes the sub-agent recovery story started by #1630/#1640: the child self-heals and the parent both re-attaches to it AND keeps its own recovery alive while doing so. Validation: - Unit (think + ai-chat): forwarding a child stream advances the marker and resets the parent's attempt budget like in-band content; a silent child does not credit, so the cap still binds. - SIGKILL e2e: the natural-agentTool() task-amplification scenario stays BOUNDED and the parent collects `completed` (no regression to #1640). - Real-deploy `deploy-churn --mode subagent` (3 mid-loop deploys): verdict flipped NOT COLLECTED -> RE-ATTACHED; the parent's incident shows progress: 8, attempt 5/6 (did not exhaust). Also fixed a harness gap: reset() now clears the parent transcript so a reused session doesn't short-circuit the mock model. Co-authored-by: Cursor <cursoragent@cursor.com>
agents
@cloudflare/ai-chat
@cloudflare/codemode
hono-agents
@cloudflare/shell
@cloudflare/think
@cloudflare/voice
@cloudflare/worker-bundler
commit: |
|
/bonk do a deep review on this, look for edge cases, is the ux/dx good |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Deep Review — PR #1641 (N9: parent progress on child stream)
I read every changed file, the surrounding recovery infrastructure (_forwardAgentToolStream, _bumpChatRecoveryProgress, _beginChatRecoveryIncident, _reconcileAgentToolRuns), and the existing test suites for both Think and AIChatAgent. This is a solid, well-scoped fix. Detailed findings below.
Architecture & Design
The layering is clean:
- Base
Agentgets a no-op_onAgentToolStreamProgress()hook — correct, because base Agent has no recovery budget. ThinkandAIChatAgentboth override it identically: throttle in-memory, then delegate to_bumpChatRecoveryProgress().- The hook fires inside
_forwardAgentToolStream— the single shared codepath for both liverunAgentTooland recovery_reconcileAgentToolRuns. This is the right place; no paths are missed.
The critical invariant — credit only on genuinely forwarded chunks, never merely on attachment — is enforced by the forwardedSinceProgress boolean that only flips true inside forwardChunk(). A silent child that enqueues zero chunks before closing its stream never sets this flag, so the hook never fires. This is explicitly tested in both packages (the "silent/hung sub-agent" tests).
Edge Cases Examined
1. Throttle reset across isolates (the key correctness property)
_lastAgentToolStreamProgressAt is an in-memory field, initialized to 0. After a deploy resets the isolate, it's 0 again, so the very first forwarded chunk on a fresh recovery attempt always passes the now - 0 >= 5000 check and bumps. This is correct — it guarantees every recovery attempt that observes any child output registers at least one progress tick, which is all the no-progress window needs.
The test harnesses explicitly reset _lastAgentToolStreamProgressAt = 0 before calling _forwardAgentToolStream to model this. Good.
2. Rapid child streaming (storage write pressure)
With AGENT_TOOL_STREAM_PROGRESS_BUMP_THROTTLE_MS = 5_000, a child streaming at, say, 50 chunks/sec only triggers one _bumpChatRecoveryProgress per 5 seconds. Since each bump is two storage ops (get + put), that's ~0.4 ops/sec — negligible relative to the child's own chunk persistence. No concern here.
3. _bumpChatRecoveryProgress failure
The hook call in _forwardAgentToolStream (line ~7820) is wrapped in try { ... } catch { }. A transient storage failure during the bump is swallowed — the stream continues uninterrupted, and the next read iteration retries. This is the right behavior: progress crediting is advisory, and a child stream that the user is watching should never break because of a parent-side bookkeeping failure.
4. Multiple concurrent children (parallel re-attach)
In _reconcileAgentToolRuns, multiple children are re-attached in parallel. Each _forwardAgentToolStream call independently fires _onAgentToolStreamProgress(). Since the throttle is time-based (not per-runId), fast-completing parallel children may only register one bump across all of them — but that's fine, because one bump per throttle window is all the recovery budget needs to see. The throttle prevents storage thrashing under high fanout.
5. The forwardedSinceProgress flag resets correctly
After the hook fires (line ~7812), forwardedSinceProgress is reset to false. The next chunk sets it true again. If the hook throws, the catch swallows it but forwardedSinceProgress was already set to false before the try. Wait — actually, looking more carefully:
if (forwardedSinceProgress) {
forwardedSinceProgress = false;
try {
await this._onAgentToolStreamProgress();
} catch {
// Ignore and keep forwarding
}
}This is correct. The flag is reset before the await, so even if the hook fails, we don't retry it on the same chunk batch — the next chunk will re-set the flag naturally. And we don't lose the "already forwarded" signal because the purpose of the flag is "at least one chunk was forwarded since last progress check", not "progress was successfully recorded". The next read iteration's chunks will set it again.
6. The deploy-churn harness reset fix
The reset() method in DeployChurnAgent now calls await this.clearMessages() — this prevents a reused session from having stale tool results in the transcript that would cause the mock model to short-circuit to "DONE" without dispatching the child. Small but load-bearing fix for the harness.
Test Coverage
Both Think and AIChatAgent have mirrored test pairs:
- N9 positive: forwarding 3 chunks bumps
after > start, budget resets to attempt 1. - N9 negative (silent child): forwarding 0 chunks leaves
after == start, budget does NOT reset, exhaustion proceeds normally.
The test harness design (forwardChildStreamProgressForTest) is well-structured: it resets the in-memory throttle to model a fresh isolate, creates a synthetic ReadableStream, and drives it through the real _forwardAgentToolStream — exercising the actual production codepath rather than mocking it. The _broadcastAgentToolEvent calls during forwarding are harmless (no live connections in the test), and the progress counter is read directly from storage.
DX/UX Assessment
For end users: Zero new public API. A parent using agentTool() / runAgentTool() now survives churn automatically when its child streams. The _onAgentToolStreamProgress hook is internal (_-prefixed, protected). No configuration knobs to tune. This is the right call — the previous behavior (parent exhausting while child streams) was a pure bug, not a policy choice.
For framework extenders: The protected async _onAgentToolStreamProgress() seam in base Agent is available for custom subclasses that implement their own recovery budget. The JSDoc clearly explains the contract: called only after forwarded chunks, should be throttled, failures are swallowed. Clean extension point.
Nits & Observations (non-blocking)
-
Code duplication between Think and AIChatAgent: The
_onAgentToolStreamProgressoverride,_lastAgentToolStreamProgressAtfield,_bumpChatRecoveryProgress,_chatRecoveryProgressMarker, and the entire incident management system are copy-pasted between the two classes. This is a known consequence of the dual-class architecture (Think vs AIChatAgent) and is noted in the codebase conventions, but it's worth flagging that this PR adds ~33 lines to each that are character-for-character identical. If there's ever a plan to extract the recovery budget logic into a shared mixin or intoagents/chat, this would be a good candidate. -
AGENT_TOOL_STREAM_PROGRESS_BUMP_THROTTLE_MSduplicated as a constant: Both Think and AIChatAgent defineconst AGENT_TOOL_STREAM_PROGRESS_BUMP_THROTTLE_MS = 5_000at file scope. If one is ever changed without the other, the behavior would silently diverge. Consider defining it once inagents/chatand importing, or at least adding a comment cross-referencing the other definition. -
Changeset accuracy: The changeset says "this completes the sub-agent recovery story started by the stable-runId + bounded re-attach fix (#1630)". The PR body correctly notes N10 is still out of scope (re-attaching a still-recoverable
interruptedrow on a later restart). Might be worth linking N10 as a follow-up issue for tracking.
Verdict
This is a well-contained, targeted bug fix that addresses a real production failure (parent exhausting its recovery budget while a healthy child streams). The design invariant (credit only on forwarded output, never on mere attachment) is correct and well-tested. The layering (base Agent seam → chat-recovery override → in-memory throttle → durable bump) is clean. No new public surface, no config to tune, no behavioral regressions for non-sub-agent workloads.
The code is ready to merge. The only follow-up work is the acknowledged N10 backstop for extreme churn where the child itself is repeatedly reset.
|
Review posted on PR #1641. Here's a summary of findings: Verdict: Ready to merge. This is a clean, well-scoped bug fix. Key findings:
|
1 participant
Summary
A parent turn whose work is "run a sub-agent and
awaitits result" produces no recoverable content of its own. Under deploy churn, the parent's own chat-recovery no-progress window could therefore exhaust while the child was still healthily streaming — the parent abandoned its turn asinterruptedand collected an interrupted result even though the child went on to complete.This is the other half of the sub-agent recovery story: #1630/#1640 made the child self-heal and made the parent re-attach to it, but the parent's own recovery budget still expired while it waited. It's what still forced a customer-side
MAX_TASK_TURN_RETRIEScap.Reproduced by the
examples/deploy-churn --mode subagentharness with realwrangler deploys mid-loop: the parent exhausted atattempt 6/6,progress: 1while the child self-healed all 30 steps with no amplification.Root cause
_forwardAgentToolStream(baseAgent) only relayed child chunks to the parent's connections via_broadcastAgentToolEvent. It never touched the parent's durable recovery-progress marker (_bumpChatRecoveryProgress), which lives in the chat layer (Think/AIChatAgent), not in baseAgent. So an awaiting parent banked zero forward progress, and the no-progress window (lastProgressAt, #1638) tripped.Fix
Agentseam_onAgentToolStreamProgress()(no-op by default), invoked by_forwardAgentToolStreamonce per read iteration that actually forwarded ≥1 child chunk.ThinkandAIChatAgentoverride it to bump their recovery-progress marker, throttled in-memory (AGENT_TOOL_STREAM_PROGRESS_BUMP_THROTTLE_MS = 5s, reset per isolate — so the first forwarded chunk after any restart always credits, guaranteeing each recovery attempt that observes child output registers progress).runAgentToolpath and the recovery_reconcileAgentToolRuns/ re-attach path (all route through_forwardAgentToolStream).Design guard (the important invariant)
Credit is granted only on a genuinely-forwarded chunk — never merely because a child is attached. A silent / hung child forwards nothing → no credit → the parent still exhausts on its own no-progress timer, so a stuck sub-agent can never pin a parent's recovery open forever. (Covered by a dedicated unit test.)
No new public API / config
Zero new app-facing surface — a parent using
agentTool()/runAgentTool()now survives churn while its child streams, automatically. The seam is an internal (_-prefixed) protected hook for the framework's own chat-recovery agents.Validation (all three layers green)
_forwardAgentToolStreamadvances the durable marker and resets the parent's attempt budget exactly like in-band content (mirrors the existing recovery budget should be wall-clock-keyed-to-progress + alarm-debounced, not raw attempt count (deploy churn) #1637/fix(think,ai-chat): wall-clock-keyed-to-progress recovery budget + alarm debounce (#1637) #1638 budget-reset test); a silent child (0 chunks) does not credit, so the cap still binds. New harness seamforwardChildStreamProgressForTestin both packages. Full suites green (think 460, ai-chat 482).task-amplificationnatural-agentTool()scenario stays BOUNDED (child 30/30, 1 bounded re-run) and the parent collectscompleted— no regression to fix(agents,think,ai-chat): re-attach to still-running sub-agent runs on parent recovery (#1630) #1640's re-attach.deploy-churn --mode subagent, 3 mid-loopwrangler deploys): verdict flippedNOT COLLECTED→RE-ATTACHED—parentChildStatus: completed, child 30/30, and the parent's recovery incident showsprogress: 8,attempt: 5/6(did not exhaust) vs the priorprogress: 1, attempt 6/6 → interrupted.deploy-churn'sreset()now clears the parent's chat transcript, so a reused session no longer silently short-circuits the sub-agent mock model to "DONE".npm run checkclean (sherif, exports, oxfmt, oxlint, typecheck — 91 projects).Follow-up (deliberately out of scope — tracked as N10)
N9 keeps the parent alive while the child streams new output; it is not an infinite hold (by design — a silent child must let the parent exhaust). Under extreme churn where the child itself is repeatedly reset and produces no new forwarded chunks between two parent attempts, the parent can still climb to the cap (the real-deploy run collected at
attempt 5/6— one beat from exhausting). The hardening backstop — re-attach a still-recoverableinterruptedrow on a later restart in_reconcileAgentToolRuns(using #1640's soft-interrupted+ N6's transcript-reconcile to repair it tocompleted) — is left as a separate follow-up because it touches the same fragile reconcile code and needs its own real-deploy gate.Semver
Marked all three packages
patch— this is a behavior bug fix with no new public API (the seam is an internal protected hook). Happy to bumpagentstominorif reviewers prefer to treat the new protected hook as additive.Test plan
task-amplificatione2e BOUNDED + parent collectscompleteddeploy-churn --mode subagentreal-deploy verdict = RE-ATTACHEDnpm run checkcleanMade with Cursor