fix(auth): retry transient upstream overload (503/529) - #4584
Conversation
shouldRetryAfterError only ever retried 429. Every other status returned early, so a 503 or a 529 was handed straight back to the caller on the first attempt and request-retry had no effect on it. Those two statuses describe a transient capacity dip upstream rather than a problem with the request or the credential, and providers document them as retryable. Unlike 429 they rarely carry a Retry-After header, which is why the existing hint-driven path cannot cover them: retryAfterFromError returns nil and the retry is dropped. Retry them on the same credential using a bounded exponential backoff (1s, doubling, capped at 8s), still gated by retryAllowed so request-retry and any per-auth override keep bounding the attempts, and still clamped to maxWait. Rotating credentials is deliberately not attempted since upstream capacity is not credential-specific. 429 behaviour is unchanged: it continues to require a Retry-After hint. Observed on an Anthropic OAuth deployment where 529 responses to claude-opus-5 (~0.4% of requests) surfaced to clients unretried.
|
This pull request targeted The base branch has been automatically changed to |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dff0fbecc1
ℹ️ 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".
| } | ||
| return wait, true | ||
| } | ||
| if isTransientOverloadStatus(status) { |
There was a problem hiding this comment.
Check overload before rejecting the existing cooldown
For a 503 returned through the normal Execute path, MarkResult first assigns the credential the default one-minute transient cooldown, so closestCooldownWait reports a wait of about 60 seconds. With the shipped max-retry-interval: 30, the preceding wait > maxWait check returns false before this new overload branch is reached, meaning 503s still are not retried under the default configuration. Evaluate the overload-specific backoff before rejecting the recorded cooldown (or otherwise exempt overload statuses from that early return); the direct unit tests miss this because they call shouldRetryAfterError without first recording the failure.
Useful? React with 👍 / 👎.
|
Thanks for targeting the retry decision point. I found two integration gaps that the helper-level tests do not exercise:
The same policy question applies to gateway-transient 502/504 responses: the documented Suggested regression shape: initial request -> 503 -> |
|
Follow-up on the integration gaps above: draft PR #4643 now contains a working same-route, pre-accounting retry path for synchronous Codex streaming 502/503/504 responses, with manager-level coverage in The tests verify that the first direct error does not cool or rotate the selected auth when the configured recovery retry succeeds, and that only the winning attempt reaches the caller. Exhaustion preserves the final structured status/error. This does not replace this PR's broader non-streaming/Anthropic 529 scope, but it provides a tested integration shape for avoiding the |
Problem
Manager.shouldRetryAfterErrorgates every retry behind a single status check:https://github.com/router-for-me/CLIProxyAPI/blob/main/sdk/cliproxy/auth/conductor_selection.go#L744-L746
Anything that is not
429returns early, so thefor attempt := 0; ; attempt++loops inconductor_execution.go(Execute,ExecuteCount,ExecuteStream) break on the very first failure.request-retrytherefore has no effect on503 Service Unavailableor on the non-standard529that Anthropic returns asoverloaded_error.Both statuses describe a transient capacity dip upstream rather than a bad request or a bad credential, and providers document them as retryable — Anthropic's errors reference lists
529 overloaded_errorand asks clients to retry with backoff. The proxy already knows about529;sdk/api/handlers/claude/code_handlers.gomaps it tooverloaded_errorwhen shaping the response, it just never retries it.The existing hint-driven path cannot cover these, because unlike
429they rarely carry aRetry-Afterheader, soretryAfterFromErrorreturnsniland the retry is dropped even if the status check is relaxed.Measured on an Anthropic OAuth deployment:
529accounted for ~0.4% ofclaude-opus-5requests over a day, and every one of them surfaced to the client unretried.Change
Add a transient-overload branch ahead of the
429gate:503and529only1s, doubling, capped at8s, so a highrequest-retrycannot turn a capacity dip into a multi-minute stallretryAllowed, sorequest-retryand any per-authRequestRetryOverridekeep bounding the attemptsmaxWait(max-retry-interval)429behaviour is unchanged and still requires aRetry-Afterhint.Tests
New
sdk/cliproxy/auth/overload_retry_test.gocovers the retry decision, the backoff schedule and its cap, themaxWaitclamp,request-retrybounding, that unrelated statuses (400,401,500,502) are untouched, and that429still needs its hint.Without the source change 4 of the 6 fail; with it the package is green:
All pre-existing
shouldRetryAfterErrortests still pass.Notes for review
Two judgement calls worth a second opinion:
8scap. Chosen sorequest-retry: 10cannot produce a multi-minute stall. Happy to make it a config knob instead if you would rather not hard-code it.maxWait. The429path rejects whenRetry-AfterexceedsmaxWait; this branch clamps, since the wait is self-computed rather than provider-supplied and rejecting our own backoff for exceeding a ceiling seemed wrong. Easy to flip for consistency if you prefer.