Repository navigation
fix(core): cap gateway timeouts and log scheduled retries - #54544
FahadBinHussain wants to merge 2 commits into
Conversation
github-actions
Bot
added
the
needs:compliance
|
The following comment was made by an LLM, it may be inaccurate: |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
The change looks sound. I checked it by running the retry policy tests against the PR and against base's retry.ts: the new gateway-timeout cases fail on base and pass with the PR. packages/core type-checks.
Public API is unchanged: the edit is internal retry policy plus a log line.
One small inline point on isTimeout: the Timeout tag check is unreachable, and the message check could be narrowed. Nothing blocking.
| // upstream in-band with `Streaming response failed: [504] Provider timed out after Nms`, and the | ||
| // status never reaches the reason. The status and message checks keep those attempts on the same | ||
| // cap as local transport timeouts instead of the full general allowance. | ||
| const isTimeout = (error: AIError) => |
There was a problem hiding this comment.
Two small things here:
error.reason._tag === "Timeout"can never take effect.isRetryablereturnsfalseforTimeout, soinput.retryis already false and the cap never applies. It can be dropped.- The message regex runs on every reason type, not just the in-band gateway failures this targets. Today it also catches e.g. the Session WebSocket
idle-timeout/connect-timeouttransport errors ("Timed out waiting for WebSocket data"), and anyRateLimitor other message that happens to mention a timeout. Limiting the text match toProviderInternal/UnknownProvider(the shapes the Zen 504 arrives in) would keep the heuristic to the case it's meant for.
There was a problem hiding this comment.
both addressed in c40ef3f - dropped the Timeout tag check and restricted the status/message match to ProviderInternal/UnknownProvider, so websocket idle/connect timeouts and rate-limit messages stay on the general allowance. added a regression test for the websocket idle-timeout shape.
There was a problem hiding this comment.
Thanks, I checked c40ef3f: the unreachable Timeout tag check is gone, and the 408/504 status and message checks now apply only to ProviderInternal/UnknownProvider errors. Local Transport timeouts still count toward the cap. The new idle-timeout test is a useful guard, and session-runner-retry.test.ts passes (6/6) on the new head. That covers my inline point.
github-actions
Bot
removed
the
needs:compliance
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
|
Automated PR Cleanup Thank you for contributing to opencode. Due to the high volume of PRs from users and AI agents, we periodically close older PRs using automated criteria so maintainers can focus review time on the most active and community-supported contributions. This PR was closed because it matched the following cleanup criteria:
PRs created within the last month are not affected by this cleanup. If you believe this PR was closed incorrectly, or if you are still actively working on it, please leave a comment explaining why it should be reopened. A maintainer can review and reopen it if appropriate. Thanks again for taking the time to contribute. |
Issue for this PR
Closes #54538
Type of change
What does this PR do?
the timeout retry cap already existed (
MAX_TIMEOUT_RETRIES = 3inSessionRunnerRetry), but it only fired for local transport timeouts (Transport+code === "Timeout"). gateway-reported provider timeouts come back as ordinary failures instead: opencode zen answers a dead upstream in-band withStreaming response failed: [504] Provider timed out after 362397ms, which classifies asProviderInternal, so those attempts took the full general allowance — ~10 retries of ~6 minutes each, about an hour of a session stuck busy while every api call returnsSessionBusyError. that is exactly the outcome the cap's own comment says it exists to prevent.this changes
isTimeoutto also match http statuses 408/504 and a timeout phrase in the message, restricted to theProviderInternal/UnknownProvidershapes gateway failures arrive in, so the heuristic does not swallow websocket idle/connect timeouts or unrelated messages. fast failures like 502 and rate limits keep the general allowance, since retrying those costs milliseconds, while each 504 already burned minutes.it also adds one warning log line per scheduled retry (attempt, delay, error type, message). previously a hang-then-recover loop left zero trace in the logs — only the final drain failure logs — which made the failure in #54538 take a day to pin down.
How did you verify your code works?
packages/core/test/session-runner-retry.test.tscovers the gateway 504 message, a 504 status without a timeout message, and the negative controls (502, plain internal, websocket idle timeout) keeping the full allowance. the new gateway cases fail against base and pass with the PR, reviewed by the automated retry-policy check on this PR.bun run checkpasses (36/36).Screenshots / recordings
not a UI change.
Checklist