Skip to content

fix(auth): retry transient upstream overload (503/529) - #4584

Open
codeg-dev wants to merge 1 commit into
router-for-me:devfrom
codeg-dev:fix/retry-transient-overload
Open

fix(auth): retry transient upstream overload (503/529)#4584
codeg-dev wants to merge 1 commit into
router-for-me:devfrom
codeg-dev:fix/retry-transient-overload

Conversation

@codeg-dev

Copy link
Copy Markdown

Problem

Manager.shouldRetryAfterError gates every retry behind a single status check:

https://github.com/router-for-me/CLIProxyAPI/blob/main/sdk/cliproxy/auth/conductor_selection.go#L744-L746

if status != http.StatusTooManyRequests {
    return 0, false
}

Anything that is not 429 returns early, so the for attempt := 0; ; attempt++ loops in conductor_execution.go (Execute, ExecuteCount, ExecuteStream) break on the very first failure. request-retry therefore has no effect on 503 Service Unavailable or on the non-standard 529 that Anthropic returns as overloaded_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_error and asks clients to retry with backoff. The proxy already knows about 529; sdk/api/handlers/claude/code_handlers.go maps it to overloaded_error when shaping the response, it just never retries it.

The existing hint-driven path cannot cover these, because unlike 429 they rarely carry a Retry-After header, so retryAfterFromError returns nil and the retry is dropped even if the status check is relaxed.

Measured on an Anthropic OAuth deployment: 529 accounted for ~0.4% of claude-opus-5 requests over a day, and every one of them surfaced to the client unretried.

Change

Add a transient-overload branch ahead of the 429 gate:

  • applies to 503 and 529 only
  • retries on the same credential — upstream capacity is not credential-specific, so rotation is deliberately not attempted and the quota/cooldown path above is left untouched
  • bounded exponential backoff: 1s, doubling, capped at 8s, so a high request-retry cannot turn a capacity dip into a multi-minute stall
  • still gated by retryAllowed, so request-retry and any per-auth RequestRetryOverride keep bounding the attempts
  • still clamped to maxWait (max-retry-interval)

429 behaviour is unchanged and still requires a Retry-After hint.

Tests

New sdk/cliproxy/auth/overload_retry_test.go covers the retry decision, the backoff schedule and its cap, the maxWait clamp, request-retry bounding, that unrelated statuses (400, 401, 500, 502) are untouched, and that 429 still needs its hint.

Without the source change 4 of the 6 fail; with it the package is green:

--- FAIL: TestShouldRetryAfterErrorRetriesTransientOverload
--- FAIL: TestShouldRetryAfterErrorOverloadBackoffDoublesAndCaps
--- FAIL: TestShouldRetryAfterErrorOverloadClampsToMaxWait
--- FAIL: TestShouldRetryAfterErrorOverloadHonoursRequestRetry
$ go test ./sdk/cliproxy/auth/
ok      github.com/router-for-me/CLIProxyAPI/v7/sdk/cliproxy/auth

$ gofmt -l sdk/cliproxy/auth/ && go vet ./sdk/cliproxy/auth/ && go build ./...

All pre-existing shouldRetryAfterError tests still pass.

Notes for review

Two judgement calls worth a second opinion:

  • 8s cap. Chosen so request-retry: 10 cannot produce a multi-minute stall. Happy to make it a config knob instead if you would rather not hard-code it.
  • Clamp vs reject at maxWait. The 429 path rejects when Retry-After exceeds maxWait; 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.

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.
@github-actions

Copy link
Copy Markdown

This pull request targeted main.

The base branch has been automatically changed to dev.

@github-actions
github-actions Bot changed the base branch from main to dev July 26, 2026 09:32

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Copy Markdown

Thanks for targeting the retry decision point. I found two integration gaps that the helper-level tests do not exercise:

  1. MarkResult runs before shouldRetryAfterError. For a 503 it creates the normal transient cooldown (60s by default). shouldRetryAfterError then finds that cooldown first and returns false when it exceeds the default max-retry-interval: 30, before reaching this PR's overload branch. As a result, the default single-credential configuration still will not retry a 503. A manager-level Execute/ExecuteStream test where the executor returns 503 once and then succeeds, using the real default transient cooldown and a 30s max wait, should expose this.

  2. The PR says overload retries stay on the same credential, but executeMixedOnce performs cross-credential/model fallback before the outer retry decision. A provider-wide 503/529 can therefore still replay across several credentials before this backoff runs. The implementation needs either status-aware fallback suppression or a retry path that explicitly reuses the selected credential.

The same policy question applies to gateway-transient 502/504 responses: the documented request-retry statuses include 408/500/502/503/504, but this patch intentionally leaves 502/504 unretried. At minimum, tests should lock down whether those statuses are deliberately excluded or supported.

Suggested regression shape: initial request -> 503 -> MarkResult establishes transient cooldown -> bounded 1s backoff -> exactly one retry on the same credential -> success, with no other credential attempted.

@jroth1111

Copy link
Copy Markdown

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 639999d8.

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 MarkResult cooldown and cross-credential replay problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants