Repository navigation
fix(session): recover from stalled model requests - #1426
anandgupta42 wants to merge 11 commits into
Conversation
A model request that never answered hung the session until an external kill, an idle-stream timeout ended it as an unknown error, and a stream that died part-way left its partial output in the message on retry. - Arm a 300s first-byte (response headers) timeout by default; it is configurable per provider through `options.headerTimeout`. - Watch Bedrock Converse binary event streams with the idle timeout too, not only SSE. - Classify a header timeout and an idle-stream timeout as retryable `APIError`s that say "The model stopped responding" (same mapping as upstream OpenCode), so the existing backoff retries them. - Before a retry, discard what the failed attempt streamed (text, reasoning, step-start, tool calls still pending). If the attempt already dispatched a tool call, do not retry, so the tool is never re-run; the session ends with an error naming the tool. - Never retry after a user cancel. Closes #1424 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- Refuse a retry once the failed attempt finished its step, so cost and tokens are not counted twice and the answer is not produced twice. - Explain on any `APIError` why a retry was refused, and stop logging that case as exhausted retries. - Keep a failed partial-output cleanup from escaping the catch block. - Do not append "gave up after N retries" when the user cancelled. - Test the discard rule directly. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (7)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe provider now applies header and stream timeouts, including to Bedrock event streams. Timeout errors can trigger bounded session retries. Before retrying, the session removes eligible partial output and prevents retry when a tool call has already been dispatched. ChangesStalled request recovery
Priority: ⬆️ High Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant SessionProcessor
participant ProviderFetch
participant MessageV2
participant StallRecovery
participant SessionRetry
SessionProcessor->>ProviderFetch: start stream attempt
ProviderFetch->>MessageV2: timeout or stream error
MessageV2-->>SessionProcessor: retryable APIError
SessionProcessor->>StallRecovery: discard partial attempt
StallRecovery-->>SessionProcessor: removed parts or dispatched action
SessionProcessor->>SessionRetry: wait before eligible retry
SessionRetry-->>SessionProcessor: backoff completed
Merge Risk: 🟡 Moderate · up to After a tool acts and the model stalls, the next turn can lack the tool’s history, allowing the model to repeat the action. Preserve that history before merging unless this limitation is explicitly accepted. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. I’m a rabbit watching headers arrive, Comment |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: b5139e51-571b-463a-9747-dbdede1b5289) |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 8 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0e99ddc5b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Use flat exports with a self re-export in `stall-recovery.ts`, per the package conventions. - Only the idle watchdog's `ResponseStreamError` is retryable; websocket transport errors keep their previous handling. - Make `SessionRetry.sleep` reject at once on an already-aborted signal so a Stop that lands just before the backoff is not delayed. - Log "max retry attempts reached" and append "gave up after N retries" only when attempts were actually exhausted. - Document that `headerTimeout: false` removes the hang protection. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: 71f9c537-4c70-4831-8b74-8145ece3058e) |
Replace the coercer reset on retry with `discardUnstarted()`, which drops only tool inputs that started streaming but were never called. Ids already allocated (including by earlier calls in the same message) stay reserved, so a retry that reuses a raw provider id can neither pair with the discarded start nor collide with an earlier call. Also make the stall tests deterministic: disable snapshot tracking in the test config (its background git work was what leaked an "All fibers interrupted" error at scope close) instead of sleeping. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: 7e582a30-82d2-4e20-a0a2-403013fd8df3) |
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
`discardUnstarted()` now removes the discarded attempt's allocations, so the per-id slot ordinals that `settled()` and `executionID()` index by stay aligned with what actually executes, while ids of earlier calls remain reserved. Tests cover both the collision and the ordinal alignment cases. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: f57ef6bb-8c5c-4794-80c4-8e832ac34505) |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Files Reviewed (5 files)
Fix these issues in Kilo Cloud Review based on HEAD Previous Review Summaries (4 snapshots, latest commit f4a5cc7)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit f4a5cc7)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (12 files)
Fix these issues in Kilo Cloud Review based on HEAD Previous review (commit 2613ba5)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Review based on HEAD Previous review (commit fc5f189)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (7 files)
Fix these issues in Kilo Cloud Review based on current HEAD Previous review (commit 689b47e)Status: 8 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (10 files)
Fix these issues in Kilo Cloud Review based on current HEAD Reviewed by gpt-6-sol · Input: 92 · Output: 33K · Cached: 7.6M Review guidance: REVIEW.md from base branch |
- Replay an assistant step that ended with "The model stopped responding" and already ran tools, as an aborted step is, so the next turn knows those side effects happened instead of losing them from context. - Match the Bedrock event-stream content type case-insensitively. - Show "Model stopped responding" in the TUI for the retried-out error too. - Tighten the stall tests: assert the stalled request's own socket closed, start the backoff cancel only after the stall fired, clear test timers on every exit, and cover parts that predate the failed attempt. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: cdcb5346-922f-4c25-8e34-2372b02837e9) |
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc5f18953d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Narrow the replay exception for a stalled step to messages where a tool actually completed, so partial text or tool input that never ran is not fed to the next turn. Also wait for the torn-down requests' close events in the stall tests before asserting on them. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: 0987efd5-5531-46d2-add5-9c4cff0fee3c) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2613ba519b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The replay of an errored assistant step is a pre-existing limitation for every error, and handling it correctly (per-part filtering, all retryable errors, errored tools) is a separate change. Keep this PR to stall detection and the retry rules; the known limit is documented in the PR. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: 47e20d98-838e-40d0-a999-ba84bdf9f5db) |
…lean up atomically - The retry guard now also refuses once any tool's execute() began during the attempt, counted in the processor from the tool wrapper's `beginToolExecution`, because the AI SDK can start a tool before its part leaves `pending` or its tool-call event is persisted. - Remove a failed attempt's parts in one delete statement so a cleanup failure cannot leave the attempt half deleted. - Race the whole custom fetch against the first-byte deadline, so work such as Vertex credential acquisition that ignores the abort signal is bounded. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: 3e45f513-52a4-4e14-92ec-8fed25cfd73f) |
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/test/session/stall-recovery.test.ts">
<violation number="1" location="packages/opencode/test/session/stall-recovery.test.ts:555">
P3: This callback is triggered by request arrival, not by creation of the pending tool part, so it can mark execution before any part exists. Wait for and assert the `call_1` part is pending before invoking `beginToolExecution`; the final `parts.some(...)` only checks state after the attempt.</violation>
</file>
<file name="packages/opencode/src/provider/provider.ts">
<violation number="1" location="packages/opencode/src/provider/provider.ts:2049">
P2: This race only observes the header deadline, so caller cancellation cannot settle the wrapper while a custom fetch is awaiting work that ignores `init.signal`. Vertex awaits credential acquisition before forwarding the signal, leaving the request pending until credentials resolve or the 300-second timeout; race the request signal as well.</violation>
</file>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b852ba47d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Race the custom fetch against the combined request signal (first-byte deadline, caller cancel, timeouts), not only the deadline, so a Stop is not held up by pre-request work that ignores the signal. The tool-race test now waits for the pending tool part before reporting the execution started. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: f4e7bfbf-6fb1-4f17-a074-5a8c4ea5300d) |
| const removed = new Set<string>(stale.map((part) => part.id)) | ||
| if (stale.length > 0) { | ||
| Database.use((db) => { | ||
| db.delete(PartTable) |
There was a problem hiding this comment.
CRITICAL: Persist retry cleanup in the synchronized session event log
This raw PartTable delete removes the failed attempt locally, but the following Bus.publish(MessageV2.Event.PartRemoved, ...) is only a legacy notification; it does not append SessionV1.Event.PartRemoved to the durable core event log. The shipped workspace sync replays that log into another workspace via SessionProjector, which has the original PartUpdated events but no removal. After a stalled attempt is retried, the other workspace therefore retains the discarded text, step-start, or pending tool alongside the retry's output, and future sync cannot correct it. Publish/project durable part-removal events as part of the atomic cleanup, rather than deleting only the local projection.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Not changed in this PR. The cleanup persists a removal the same way the existing Session.removePart in session/index.ts does: a delete on PartTable followed by Bus.publish(MessageV2.Event.PartRemoved, ...). Session.updatePart writes parts through the same path. The bulk delete replaced a loop over Session.removePart so the cleanup is one statement; it did not change which event log the removal reaches. If removals made through this path are not replayed by workspace sync, that applies to every existing caller of Session.removePart and is a separate change to how this module persists part events, outside the stall-retry fix.
This branch has not been deployed
1 participant
Issue for this PR
Closes #1424
Type of change
What does this PR do?
Why. A session never recovers when a model request goes quiet. In a recorded benchmark, 10 of 135 sessions on one model (GPT-6.1 Sol via Bedrock Mantle) logged a request and then nothing until the harness killed them at its 30 minute limit: no error, no retry, and in 7 of the 10 the finish-time validators never ran. (10 of 135: estimate from recorded data.)
Root cause (verified in code).
What changed (before, after).
Spec. A stalled request is aborted and retried with the existing backoff at most 5 times; partial output is never duplicated; a dispatched tool is never re-run; exhaustion ends with a clear error; cancel is immediate.
Tenant/user impact. A request silent for 300s is now retried instead of hanging. Users could also notice: (1) a persistent stall takes about 31 minutes to surface at the default (6 attempts of 300s plus backoff), so lower headerTimeout where that matters; (2) the 300s first-byte deadline now also bounds custom fetches such as Vertex credential acquisition; a healthy request silent for over 300s before headers (slow local prompt evaluation, non-streaming calls) would be cut off, so raise headerTimeout there; (3) the no-retry-after-started-tool rule covers every retryable error: a mid-stream 5xx after a tool started used to be retried (re-running the tool) and now ends with an error, and, as for any errored step, its tool history is not replayed next turn; (4) the TUI shows "Model stopped responding" for the exhausted error; (5) a Stop just before a backoff ends the wait at once. Validators are unchanged: they run after a clean stop, so a recovered stall reaches them; an exhausted one skips them.
Deployment readiness. No migration, dependency or config; revert restores old behaviour.
How did you verify your code works?
Screenshots / recordings
Not a UI change.
Checklist
🤖 Generated with Claude Code
Note
Medium Risk
Changes core provider fetch timeouts, session retry semantics, and tool-call safety rules; mis-tuned
headerTimeoutcould cut off legitimately slow streams, and the no-retry-after-tool-start rule alters behavior for any retryable error mid-step.Overview
Sessions that hung or died on silent providers now abort stalled requests, retry with backoff (up to 5 times), and clean up partial output so retries do not duplicate streamed text or re-run tools.
Provider layer: Most providers get a 300s default
headerTimeout(configurable viaoptions.headerTimeout, orfalseto disable). The stream idle watchdog now covers Amazon Bedrock event streams as well as SSE. Custom fetches that ignore abort signals (e.g. Vertex credential work) are raced against the first-byte deadline.Session layer: Header and idle-stream timeouts map to retryable "The model stopped responding" errors. Before each retry,
StallRecoveryremoves text, reasoning, step-start, and pending tool parts from the failed attempt; retries are blocked once a tool has started executing or a step has finished, with a clear error message. User cancel skips retry and fails fast during retry backoff if the signal is already aborted.Docs / TUI:
providers.mddocumentsheaderTimeoutandchunkTimeout; notifications treat the new exhausted-stall message as "Model stopped responding".Reviewed by Cursor Bugbot for commit a689e5a. Bugbot is set up for automated code reviews on this repo. Configure here.