Repository navigation
fix(think): retry pre-first-chunk stalls and call onChatRecovery for stalls (#1941, #2042) - #2328
Conversation
🦋 Changeset detectedLatest commit: e1128b2 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: |
| const retryTargetUserId = | ||
| input.partialParts.length === 0 ? await this._latestUserLeafId() : null; | ||
| const recoveryKind: ChatRecoveryKind = retryTargetUserId | ||
| ? "retry" | ||
| : "continue"; |
There was a problem hiding this comment.
🟡 Dropped partial mislabels recovery kind
With persist: false after streamed chunks, onChatRecovery receives recoveryKind: "continue" before _chatRecoveryRetry is scheduled. Provider-specific hooks can resume a discarded partial instead of preparing a retry.
Learn more
recoveryKind tells the hook whether recovery retries the unanswered user turn or continues a partial assistant turn. These lines classify solely from the pre-hook partial. Later, persist: false discards a text-only partial, leaves the user as the session leaf, and schedules _chatRecoveryRetry at the retry branch. The hook therefore sees a different action from the one ultimately scheduled.
Example: A provider emits text, stalls, and stores a response ID with stash(). The hook sees "continue" and attempts provider-side continuation, then returns persist: false. Think discards that partial and retries the original user message instead.
Recommended fix: Decide the effective recovery kind before invoking onChatRecovery, or define and expose a separate post-persistence action. If persist must remain a hook result, avoid promising that ctx.recoveryKind describes the eventual scheduled action.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Documented rather than changed (e1128b2): recoveryKind reports what was detected when the hook runs, and the hook's persist: false decides what happens next. scheduleRecovery already allows the reported kind to differ from the one the incident opened with. docs/think/index.md now explains this.
| if (outcome === "exhausted" || outcome === "declined") { | ||
| // `_routeStallToBoundedRecovery` already delivered the terminal UX | ||
| // (configured `terminalMessage` + done/error frame + `onExhausted` + | ||
| // submission interrupted), identical to deploy-recovery exhaustion. | ||
| // Finalize the stream and return WITHOUT the generic terminal path, | ||
| // which would otherwise re-broadcast the raw stall error. | ||
| // (exhaustion: configured `terminalMessage` + `onExhausted`; declined: | ||
| // the declined message), with the done/error frame and the submission | ||
| // marked interrupted. Finalize the stream and return WITHOUT the | ||
| // generic terminal path, which would re-broadcast the raw stall error. | ||
| if (!streamFinalized) { | ||
| this._errorResumableStream(streamId, requestId); | ||
| streamFinalized = true; |
There was a problem hiding this comment.
There was a problem hiding this comment.
Intended. A declined stall is terminal in the same way as exhausted recovery (#1644), and onInterrupted is the terminal signal RPC callers already handle for that case.
0eec3e9 to
7098573
Compare
⚪ agents import sizesMeasured 343 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 No import sizes changed. All 343 current runtime imports
Reported by agent-think[bot]. |
7098573 to
40d9357
Compare
40d9357 to
0d55c50
Compare
0d55c50 to
6ffdfda
Compare
…stalls (#1941, #2042) A stall before the first assistant chunk left the user's message as the leaf, but `_routeStallToBoundedRecovery` always scheduled `_chatRecoveryContinue`, whose `continueLastTurn()` skips when the leaf is not an assistant message. The turn ended with no answer and no terminal hook. It now schedules `_chatRecoveryRetry` for the unanswered user message whenever nothing was persisted (including after `persist: false`). The stall path also never called `onChatRecovery`. It now does, with the live turn's latest `stash()` data and start time, and honors `persist: false` (settled tool results are always kept) and `continue: false`. A throwing hook marks the incident failed and surfaces the stall as a terminal error. Docs explain that `chat()` callers bridging to HTTP must close the response in `onInterrupted()`, which is why the reporter's SSE stream stayed open. Fixes #1941 Fixes #2042 Co-authored-by: Cursor <cursoragent@cursor.com>
_persistAssistantMessage skips a message that stripping leaves empty (only internal final-answer parts), but the stall path still reported its id, so recovery scheduled a continuation for an assistant message that does not exist and the user turn went unanswered. Report an id only when something was saved. Co-authored-by: Cursor <cursoragent@cursor.com>
6ffdfda to
e1128b2
Compare
Problem
#1941: A stream stall before the first assistant chunk left the user's message as the leaf.
_routeStallToBoundedRecoveryalways scheduled_chatRecoveryContinue, whosecontinueLastTurn()skips when the leaf is not an assistant message. The turn ended with no answer, no error, and no terminal hook.#2042: The stall path never called
onChatRecovery. Sopersistandcontinuewere ignored for stalls, andchat()callers bridging to HTTP (SSE) never learned that the turn had ended.Fix
persist: false) and the leaf is still the user's message, it schedules_chatRecoveryRetryto re-answer that message.onChatRecoverywith the live turn's lateststash()data and its real start time.persist: falseis honoured, except that settled tool results are always kept.continue: falsemarks the incident skipped and broadcasts the declined terminal state.docs/think/index.md,docs/think/sub-agents.md) describe stall recovery. They also explain thatchat()callers bridging to HTTP must close their response inonInterrupted(), which is why the reporter's SSE stream stayed open.Validation:
persist: false, and throwing-hook paths. All of them fail without the fix.pnpm run checkpassed.Fixes #1941
Fixes #2042
Thanks to @arthurfiorette and @bartrumb for the detailed reports.
Stacked PR 7 of 10. Base: #2327.