Repository navigation
fix(router): fail over to a sibling deployment on non-retryable 4xx errors - #46003
thibault-linktree wants to merge 13 commits into
Conversation
…rrors should_retry_this_error raised every non-retryable status except 401/403 straight to the caller, so a 404 model_not_found or 400 from one leg of a multi-deployment group surfaced to the client even when another deployment in the group was healthy. The broken leg was only benched afterwards by cooldown, so the first caller in each cooldown window still saw the failure A non-retryable error now retries when a healthy deployment that has not already refused this request exists. The existing retry skip list (_retry_skipped_deployment_ids) steers the retry away from the deployments that refused, and the retry stops as soon as none is left, so a request every leg rejects is not repeated in place (the concern behind BerriAI#19216). Single-deployment groups and retry policies behave as before, and the refusing deployment still goes into cooldown
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 Reviews |
Merging this PR will not alter performance1 warning
Comparing |
The router coverage gate requires every router.py function to be named in a test, and the helper had one caller
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
A guardrail intervention raised by a retry attempt is a 400, so once non-retryable errors could fail over it would be forwarded to an untried sibling deployment instead of reaching the caller. The first attempt already re-raised guardrail interventions; the retry loop now does too
BrickerP
left a comment
There was a problem hiding this comment.
Review (COMMENTED)
Approach looks sound. The bug in #46002 is real: should_retry_this_error used to raise every non-retryable status (except 401/403) before the retry loop could use the existing _retry_skipped_deployment_ids path, so the first caller in each cooldown window ate the 4xx even when a healthy sibling existed. Computing the skip set before the check, requiring a known refusing deployment id plus an untried healthy sibling, keeping single-deployment and “every leg refused” behavior (#19216), leaving cooldown untouched, and re-raising guardrail interventions inside the retry loop all match the intended contract. The respx integration tests plus the should_retry_this_error decision table (and the guardrail regression) give good confidence.
Residuals / gaps / risks
- Failover is bounded by
num_retries, not group size. A group with more bad legs thannum_retries + 1can still miss a healthy sibling after the retry budget is exhausted. Worth a short docs/changelog note (or guidance thatnum_retriesshould be ≥ group size − 1 for this behavior). num_retries=0still surfaces the 4xx immediately even whenshould_retry_this_errorwould returnTruefor an untried sibling — the laterif num_retries > 0/else: raisegate wins. Operators on the default-zero path will not get same-request sibling failover unless they set retries.- Client-error amplification (acknowledged in the PR): a genuinely malformed request that every provider rejects now costs one call per deployment (bounded by
num_retries). Same path applies to 422 and other non-retryable 4xx vialitellm.should_retry, not only 404 — there is no per-status opt-out (e.g. “404 only”). Fine for the reported misconfigured-model case; worth calling out for multi-leg groups. _DEPLOYMENTS_ADAPTER.validate_python(...): ifhealthy_deploymentsever has an unexpected shape, aValidationErrorcould mask the original upstream exception. A narrow try/except that falls back to raisingerrorwould be safer.- Test gaps: sibling failover for 422 (and other non-400/404 non-retryable codes) is implied by
should_retrybut not covered by the new respx cases; also no explicit case for “healthy sibling exists butnum_retries=0still raises.” Existing 400/422 unit tests still only assert the empty-tried_deployment_idsraise path (good for #19216), not the new positive path for those codes. - Observability: failover rides generic
log_retry/x-litellm-attempted-retries; no distinct signal for “non-retryable sibling failover,” which may make the new behavior hard to spot in production metrics.
Tests: meaningful coverage is present (404/400 sibling success, cooldown still applied, single-deployment one-shot, N refusing then success, all-refuse stops once per leg, decision table, guardrail-on-retry). CI was still settling when I looked; worth confirming the new router unit cases stay green on the full matrix.
Not blocking merge from my side — this is a clean, well-scoped fix for a real routing hole. The residuals above are mostly documentation / edge hardening.
…all outputs, non-stream Codex (0.1.26)
- Chat -> Anthropic reads capabilities.thinking.types from the model list: a model
that takes only adaptive thinking (Opus 4.7 and later) gets {type: adaptive} and
output_config.effort, and no budget_tokens or sampling values, which it refuses.
A budget model gets no thinking on a turn that continues an unsigned tool loop
(anomalyco/opencode#54693). The list is read on demand on a cold start.
- 402, 408, 502, 504 and 529 fail over to the next account; compaction fails over
on 401/402/403 too (BerriAI/litellm#46003).
- A clean EOF with no finish reason is an error, not response.completed, on the
chat -> Responses and Anthropic -> chat streams; a chat pass-through read error
reaches the next stage (router-for-me/CLIProxyAPI#6533).
- The upstream transport pings idle HTTP/2 connections (CLIProxyAPI#6534).
- A Responses input with a call and no output gets a stand-in output after the
batch, on every route (openai/codex#44604).
- A non-streaming Responses caller on Codex gets one JSON body: intact asks for
the stream and folds it (decolua/9router#4733).
- Bifrost no longer sends an empty Anthropic-Beta header to Codex/Antigravity.
…over_non_retryable # Conflicts: # litellm/router.py
|
Automatic reviews are disabled for this repository. |
|
@litereview code-review |
Review not startedLiteLLM team members: sign in to LiteReview with your work Google account and connect GitHub, then post your review request again. Everyone else: comment |
…over_non_retryable # Conflicts: # litellm/router.py
…over_non_retryable Upstream taught the retry path to fail an UnsupportedEndpointError over to a sibling. The generic non-retryable rule here already covers that case, so should_retry_this_error keeps the generic check and reuses upstream's _every_deployment_refused helper and _DEPLOYMENTS_ADAPTER
…over_non_retryable # Conflicts: # litellm/router.py
…over_non_retryable # Conflicts: # tests/unit/test_router/test_router.py
…over_non_retryable Upstream moved the router internals from router.py into router_python_core.py, so the non-retryable failover rule, the tried-deployment ordering and the guardrail re-raise are ported there. The new core test that expected a 400 from both deployments of a group to stop after one call now expects each deployment to be tried once, like the auth error case next to it
…over_non_retryable Upstream started re-raising failed agentic follow-up calls in the retry loop at the same spot that re-raises guardrail interventions here, so the loop now re-raises either one, matching the first-attempt check
…over_non_retryable Upstream deleted the Python router core (BerriAI#50178), so the failover moves to the Rust core. should_retry retries a non-retryable failure while a healthy deployment of the group is untried, the retry loop records the failed deployment before deciding, and a guardrail rejection on a retry is returned as is. The should_retry_this_error and guardrail unit tests move to router-core.
…over_non_retryable # Conflicts: # litellm-rust/crates/router-core/src/call.rs
Handwritten 1-3 sentence description
[insert handwritten description here]
Details
TLDR
Problem this solves:
How it solves it:
User Flow
Before: a developer calling a model group with one broken and one healthy deployment gets a 404 even though the group could have answered
gpt-failoverwith a deployment whose model id OpenAI no longer serves, plus a healthygpt-5.4-minideployment"model": "gpt-failover"No fallback was attempted.After: the same first request succeeds on the healthy deployment
gpt-failoverwith a deployment whose model id OpenAI no longer serves, plus a healthygpt-5.4-minideployment"model": "gpt-failover"x-litellm-attempted-retries: 1Relevant issues
Fixes #46002
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
uv run pytest tests/unit/<your_test_file>.py -v. Leave the suites (make test-unit-*,make test-unit) to CI: it finishes in ~15 minutes where a laptop takes an hour or moreDelays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
Both runs hit the real OpenAI API through a local proxy on port 4000 running 2 workers that share one dedicated Redis, started fresh for each run with
docker run -d --rm --name litellm-qa-redis -p 127.0.0.1:6391:6379 redis:7.2.1. The native router extension was rebuilt at each SHA withuv pip install -e .. The config is a copy oflitellm/proxy/dev_config.yamlwith the block below added.broken-legpoints at a model id OpenAI answers 404 for and carries most of the weight, so the first request lands on itBefore (1432e23)
The log shows two workers serving,
Started server process [75866]andStarted server process [75867]deployment:broken-leg:cooldown, so only later callers are routed around itAfter (5ee17bd)
Start the proxy the same way. The log shows
Started server process [75669]andStarted server process [75670]Send the same request
The caller is failed over to the healthy deployment inside the same request
deployment:broken-leg:cooldown, so later requests go straight to the healthy deploymentType
🐛 Bug Fix
Changes
Upstream moved the router onto the Rust core (#50178), so the fix lives in
litellm-rust/crates/router-core.should_retryinretry.rsused to refuse every non-retryable status other than 401 and 403, and everyNotFoundError, so a 404 or 400 from one deployment never got a second try in its group.RetryContextnow carrieshas_untried_deployment, and those failures are retried only while it is trueIn
call.rsthe retry loop already pushed each refusing deployment ontoskipped_idsso the next pick avoids it. It now records that id before deciding, and setshas_untried_deploymentwhen a healthy deployment outsideskipped_idsis left. Once every deployment has refused, the loop stops and returns the last error. Streams and the Pythonshould_retry_this_errormember passfalse, so they behave as beforeA guardrail rejection is also a 400, so the loop now returns a guardrail intervention raised by a retry as is, the same way the first attempt already did. Without that, a prompt a guardrail rejected on a retry could be forwarded to an unguarded sibling
Rust tests in
retry.rscover the 400, 404 and 422 decision with and without an untried deployment.tests/call_loop.rscovers a 400 retried on the group's other deployment, a 400 from every deployment trying each once and then taking the fallback (this replaces upstream'sa_bad_request_falls_back_without_retrying, which pinned the old behavior), and a guardrail rejection on a retry not reaching the third deploymentThe Python tests in
tests/unit/test_router/test_router.pydrive real HTTP through respx against the native core. They cover 404 and 400 failing over to a sibling, the 404 deployment still landing in cooldown, a three-deployment group failing over past two refusing deployments, a single-deployment group making exactly one call, and a request every deployment rejects making one call per deployment and raising the last error.tests/unit/test_router/core/test_retries_cooldowns.pyexpects a 400, context window or content policy error to try each deployment of a two-deployment group onceMutation check on the Rust change. Never reporting an untried deployment turns 4 tests red, ignoring which deployments already refused turns 2 red, and dropping the guardrail check in the loop turns 2 red
Caveats (if any)
Medium
num_retries: 0the first 4xx still reaches the callernum_retriesto 2 when unsetnum_retries + 1can still failLow
num_retries, and single-deployment groups still make one callFinal Attestation