fix: isolate llm fallback errors and prevent output replay - #2480
fix: isolate llm fallback errors and prevent output replay#2480chenghao-mou wants to merge 3 commits into
Conversation
Read terminal errors from each child stream, stop outer retries after forwarded output, and propagate cancellation to active children. Fixes #2477 Addresses AGT-3492 Co-authored-by: Dan Tran <7690403+dtran26@users.noreply.github.com>
chenghao-mou
added
the
review_effort:high
🦋 Changeset detectedLatest commit: e82da41 The changes in this PR will be included in the next version bump. This PR includes changesets to release 39 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
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: 1 flag
Not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
Wrap retryable failures after output in a non-retryable APIError instead of changing connection options. Keep the provider error as cause and preserve errors that are already terminal. Co-authored-by: Dan Tran <7690403+dtran26@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Make APIError.retryable writable so fallback can stop outer retries without replacing the provider error. Preserve its class, identity, and metadata; remove the wrapper and cause support added for it. Co-authored-by: Dan Tran <7690403+dtran26@users.noreply.github.com>
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| { llm: llm.label(), ...extra }, | ||
| 'failed after sending chunk, skip retrying. Set `retryOnChunkSent` to `true` to enable.', | ||
| ); | ||
| if (error instanceof APIError) error.retryable = false; |
There was a problem hiding this comment.
🟡 Shared provider errors lose retries
When a provider reuses an APIError, FallbackAdapter permanently marks it terminal after one partial response. Later requests receiving that instance skip configured retries without emitting output.
Learn more
The provider creates and owns the error object. Marking that object non-retryable stores request-specific fallback state on an object that can outlive this stream. Any later stream that throws the same instance enters LLMStream, sees retryable === false, and terminates immediately.
Example: A provider caches one APITimeoutError and throws it for two requests. The first request emits text before failing, so fallback changes the cached error. The second request fails before output, but its configured outer retry never runs because the cached error remains non-retryable.
Recommended fix: Keep the terminal-after-output decision on this FallbackLLMStream or its parent retry state instead of mutating the provider error. Preserve the original error instance when emitting it, while explicitly telling the outer LLMStream not to retry this stream failure.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Concurrent LLM requests could inherit another stream's failure and replay text or tool calls. Scope fallback failures to their child stream, stop outer retries after output when
retryOnChunkSentis false, and cancel active children when the parent closes.Fixes #2477. Adopts @dtran26's diagnosis and reproduction; local probes also confirmed child-retry and cancellation failures. Addresses AGT-3492.
Validated with 13 regression cases, the original reproducers, and 2,623 passing core tests.
Initial prompt and agent context
Model: GPT-6