Repository navigation
perf(chat): derive the recovery progress marker from the stream log - #2223
Merged
Merged
Conversation
The chat recovery budget keys off one monotonic number: did the turn produce new durable content since the last attempt, and how much work has it done since the incident opened. Both hosts maintained it as a Durable Object KV counter bumped per credited chunk — a get and a put per tool call and per text or reasoning segment, on the streaming hot path — even though the chunk log they flush to already is the durable record of produced content. ResumableStream.progressMarker() now reads it from the log: a live stream contributes its log tail, a settled stream its final cursor, and a stream whose rows are deleted (the cutover, the reclaim on the next start, a clear) has its segments folded into a retired total first, so the sum never moves when rows go away. A parent forwarding a sub-agent's output credits one unit explicitly, since that output goes to clients and not to the parent's own log. Nothing is written per chunk; one row is written per stream retired and per explicit credit. A reconnect replay or a recovery re-persist reads the log without appending, so neither can fake progress (#1637); compaction rewrites the transcript, not the log (#1628). The old KV counter is read once per isolate and seeded into the retired total by max, so an in-flight incident never sees the marker read lower after the upgrade; the key is left in place so a rollback resumes from a value no lower than it left. AIChatAgent now flushes a settled tool result the moment it is stored, as Think already did: the result is durable before the next packed flush and counts as progress immediately.
🦋 Changeset detectedLatest commit: f7a442d The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 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 |
Contributor
🟡 agents import sizesMeasured 265 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 (49)
All 265 current runtime imports
Reported by agent-think[bot]. |
agents
@cloudflare/ai-chat
@cloudflare/codemode
hono-agents
@cloudflare/shell
@cloudflare/think
@cloudflare/voice
@cloudflare/worker-bundler
commit: |
…retry a failed seed Review follow-ups on the derived progress marker. The retire is now a property of deletion rather than of the caller: the Streams sync aperture gains an onDelete hook that every delete path runs with the row and its cursor before removal, and the chat adapter retires chat rows there. A chat stream deleted through the capability's public delete() no longer lowers the marker. The hosts mirror the marker's durable part back to the pre-derivation KV key after each retire and each credit — one put per stream retired, none per chunk — so a build rolled back to the counter never reads a marker lower than an incident recorded under the derived one. A rejected legacy-counter read is no longer cached: the memoized seed promise clears itself on failure so the next evaluation retries instead of failing for the isolate's life.
…construction Both hosts construct the chat stream adapter inside a startup step that the runtime retries after a failed start, and each construction registered another deletion hook on the same Streams capability, so a stream deleted afterwards would have been retired once per construction. The aperture's onDelete now returns its unsubscribe, and the adapter replaces the hook of any adapter constructed earlier on the same capability.
…s rows Nothing calls it: the cutover's discard drops a completed stream's rows in the settle transaction, and reclaim() on the next start() catches the rest, so the per-stream discard has no caller in source, tests or docs.
…e-derive the legacy flag after a rolled-back cutover Think's cutover never passed a discard option, so an agent-tool child turn's stream rows were deleted with its message and a parent attaching after completion replayed nothing. The cutover now keeps the rows for a child turn, decided from the run map as ai-chat already does, and the child's next start() reclaims them. The settle's tail read can fold a v1 stream's rows inside the cutover transaction and, if they were the last, drop the legacy table and clear the in-memory flag. A throwing commit rolled the table back but not the flag, so the restored rows became invisible. The flag is now re-derived from the schema when the transaction leaves it changed.
Merged
tombeckenham
added a commit
to tombeckenham/agents-tom
that referenced
this pull request
Oct 7, 2026
…ry for transient reader errors Ports upstream cloudflare#2194, cloudflare#2190, cloudflare#2223, cloudflare#2360, cloudflare#2385 and cloudflare#2393 to AGUIChatAgent. - Recovery attempts are __cf_internal_chat_recovery Task runs dispatched to the model handoff; a root agent's chat turn is a Task run (facets stay on fibers). The origin-ids scope moves into the dispatch wrapper. - The progress marker is derived from the stream log; a settled TOOL_CALL_RESULT is flushed as it lands. - A platform-transient reader error routes into bounded recovery like a stall: onChatRecovery is consulted with the live turn's stash data, persist:false drops the partial, a blank new turn is retried, and a running attempt that reschedules leaves the incident to its successor. - A cancel during backoff cancels the queued recovery. - An auto-continuation that fails before streaming sends an error frame and reaches onChatResponse. The agui test worker follows the Tasks transport and the derived marker.
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
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.
What
The chat recovery forward-progress marker is derived from the stream log instead of being a KV counter bumped per credited chunk.
Both hosts (
Think,AIChatAgent) used to callbumpChatRecoveryProgresson the streaming hot path: a Durable Object KV get and put per credited chunk. Per turn that is two per tool call, one per text or reasoning segment start, one per five seconds of deltas, and one per five seconds of forwarded sub-agent output. The chunk log those same hosts flush to is already the durable record of produced content, so the counter was a second copy of information the log holds.ResumableStream.progressMarker()now reads it from the log:start(),clearAll()) its segments are folded into a retired total first, in the same synchronous block, so the sum never moves when rows go away;creditProgress(), throttled as before, because that output goes to clients and not to the parent's own log.The invariants the counter carried hold by construction. A reconnect replay or a recovery re-persist reads the log without appending, so neither can fake progress (#1637). Compaction rewrites the transcript, not the log (#1628). The marker only grows.
The Streams sync aperture gains
cursor(streamId)andonDelete(hook). The retire is a property of deletion, not of the caller: every path that removes a stream's rows (the publicdelete(), the aperture's deletes, a cutover's discard) runs the hook with the row and its cursor first, in the same synchronous block and, for a cutover, the same transaction. The chat adapter registers one hook that retires chat rows, so a chat stream deleted through the public capability can no longer lower the marker. The hook is owned per capability: a host whose startup retried constructs the adapter again, and the new adapter replaces the earlier hook rather than adding to it, so a deletion is retired once.AIChatAgentnow flushes a settled tool result (tool-output-available/-error/-denied) to SQLite the moment it is stored, asThinkalready did. It captures a completed, often non-idempotent side effect that an eviction before the next packed flush would lose, and under the derived marker the flush is what makes a settled result count as progress immediately, as it did under the per-chunk counter. Streaming deltas keep the buffer's packing.Also in this PR
Three follow-ups from triaging the Devin review of #2216 against the merged code. They touch the same cutover and stream-adapter paths, so they ride here.
discardoption, so a child turn's stream rows were deleted with its message and a parent attaching after completion replayed nothing. ai-chat already kept child rows; Think now does the same through the run map, and the child's nextstart()reclaims them. Devin's note on feat(streams): rollover block log and atomic stream → session cutover #2216 marked this resolved, which was true only for ai-chat.discardCompletedhad no caller since the cutover deletes rows itself. Removed.Cost
Pinned on real DO SQLite by
progress-marker.test.ts: the marker counts flushed segments, is unchanged by a replay, a cutover, a reclaim and a clear, and a sequence that stores 36 chunks across three streams writes under 40 rows in total. The streams write-parity bench moves from+2to+3rows per reclaimed turn, which is the retire row.Upgrade
The unit changed from "credited chunk" to "durable segment": one per ten packed streaming chunks, one per settled tool result, one per explicit credit. For delta-heavy turns that is a finer measure than the old counter, which credited a long text segment once per five seconds, so
DEFAULT_CHAT_RECOVERY_MAX_WORKmoves from 1 000 to 10 000 to stay as generous as before in the new unit while remaining finite. The no-progress window is unaffected since it only asks whether the marker moved.The old KV counter is read once per isolate and folded into its own column of the progress row by max, beside the retired total, so it can never swallow segments retired before the seed landed and a repeated seed is idempotent. The hosts memoize the seed as a promise so two concurrent reads both wait for it, and an incident's work baseline is never captured from an unseeded marker; a failed read is not cached, so a transient storage error does not block recovery for the isolate's life.
Rollback stays safe in the other direction too. The hosts mirror the durable part of the marker back to the KV key after each retire and each credit (one put per stream retired, none per chunk), so a build rolled back to the counter never reads a marker lower than an incident recorded under this one.
bumpChatRecoveryProgressstays exported and deprecated.One known one-time effect: the first post-upgrade read counts an in-flight stream's live segments on top of the credits the old counter already banked for them, which reads as progress once and hands an in-flight incident one extra no-progress window.
Tests
agents:legacy-fold.test.tsgains the rolled-back cutover case (restored v1 rows stay visible; the next append continues at the old cursor). Newprogress-marker.test.ts(segment counting across cutover, reclaim, clear and replay; a chat row deleted through the public capability; the host mirror fires per retire and per credit; an adapter constructed three times retires a stream once; the legacy seed beside retired segments, idempotent); streams suites, chat unit suites, and the session migration suite pass with the parity pin updated.@cloudflare/think:agent-tools.test.tsgains a parent that attaches after the child completed and still gets its chunks. Both hosts: the reconnect-immunity, N9 forwarding, silent-child, compaction-immunity and work-budget tests now drive and observe the derived marker. The two discussion: recovery/repair ergonomics so products don't need a full onChatRecovery override (persist-false footgun, ask_user preserve-as-text, onExhausted payload) #1631 exhaustion tests seed their incident with the marker as it stands, since the seeded stream's segments now count. One ai-chat concurrency test registered itsdonelistener after awaiting a broadcast on a second socket and relied on cross-socket delivery order; it now registers first. Both full workers projects pass.npm run typecheck,oxfmt --check .,oxlintclean.Follows #2216: the cutover it introduced is where a stream's segments are retired into the total.