fix(router): resolve retry_policy by exception hierarchy, add ServiceUnavailableErrorRetries and DefaultRetries - #35853
Conversation
…rrorRetries in retry policy
Greptile SummaryThe PR resolves router retry counts through exception inheritance and adds configurable retry handling for service-unavailable and otherwise-unmapped errors.
Confidence Score: 5/5The PR appears safe to merge, with only the previously reported non-blocking generated-comment issue still outstanding. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| litellm/router_utils/get_retry_from_policy.py | Replaces ordered exception checks with an MRO-based mapping and a configurable default fallback. |
| litellm/types/router.py | Extends RetryPolicy with service-unavailable and catch-all retry counts. |
| tests/test_litellm/router_utils/test_get_retry_from_policy.py | Covers every specific policy field, hierarchy precedence, defaults, unrelated errors, and model-group policy selection. |
| tests/test_litellm/test_router.py | Verifies configured policies control actual upstream attempt counts for mapped and default error paths. |
| tests/test_litellm/test_router_per_deployment_num_retries.py | Updates the amplification expectation to reflect the now-effective internal-server retry policy. |
| ui/litellm-dashboard/src/app/(dashboard)/models-and-endpoints/components/ModelRetrySettingsTab.tsx | Adds dashboard choices for service-unavailable and catch-all retry policies. |
| ui/litellm-dashboard/src/lib/http/schema.d.ts | Adds generated declaration fields for the two new retry-policy settings. |
Reviews (3): Last reviewed commit: "test(router): fake the upstream with res..." | Re-trigger Greptile
| RateLimitErrorRetries?: number | null; | ||
| /** Serviceunavailableerrorretries */ |
There was a problem hiding this comment.
New generated documentation comment
The generated declaration adds a documentation comment for ServiceUnavailableErrorRetries, contrary to the repository convention against adding comments unless explicitly requested; remove it or adjust generation so the checked-in declaration follows that convention.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…itellm_retry_policy_503 # Conflicts: # litellm/types/router.py # tests/test_litellm/test_router.py
…ltRetries Replace the hand-ordered isinstance ladder in get_num_retries_from_retry_policy with a class-to-field mapping walked along the exception's MRO, most specific class first. A RetryPolicy field can no longer go silently dead the way InternalServerErrorRetries did, and subclasses such as ContentPolicyViolationError or MidStreamFallbackError pick up their parent's field when they have none of their own. Add a DefaultRetries catch-all so errors without a dedicated field (BadGatewayError, APIConnectionError, NotFoundError, ...) can be governed by the policy too. Specific fields still win over DefaultRetries. Wiring the previously dead InternalServerErrorRetries changes one test expectation: a policy of 2 now overrides a per-deployment num_retries of 5, so the amplification test sees 3 upstream requests instead of 6. Expose DefaultRetries as "All other errors" in the Admin UI retry settings tab and ratchet the lint budgets down by the violations this branch fixed.
|
@greptileai re review |
…t test The test-quality gate rejects patching litellm.acompletion, and faking the HTTP boundary is the stronger test anyway: the 503, 500 and 502 responses now travel through the real OpenAI SDK and exception mapping before the router decides how many times to retry. Adds a case showing that a 503 key does not govern a 502.
|
@greptileai re review |
be76dfa
into
litellm_internal_staging
TLDR
Problem this solves:
How it solves it:
User Flow
Before: a proxy admin whose provider is returning 503s wants those requests to fail fast, but every request is retried anyway
model_group_retry_policy: {gpt-5.6: {InternalServerErrorRetries: 0}}underrouter_settingsand restart the proxy"model": "gpt-5.6"litellm.ServiceUnavailableError, and the provider's access log shows 3 requests for that one callAfter: the same admin names the error they see, or all errors, and each failed request makes exactly one upstream attempt
ServiceUnavailableErrorRetries: 0to the same model group, orDefaultRetries: 0to cover every error type, and restart the proxy"model": "gpt-5.6"Relevant issues
Linear ticket
Resolves LIT-5202
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
uv run pytest tests/test_litellm/<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 more@greptileaito re-request a review after pushing changes)Screenshots / Proof of Fix
A provider outage cannot be summoned on demand, so an openai deployment points at a local upstream on 127.0.0.1:19503 that answers every POST with a chosen 5xx status and counts the attempts it receives.
GET /status/503picks the status and zeroes the counter,GET /resetzeroes it,GET /reports{"attempts": n}. Proxy config:Each case below runs this loop, with
$PATH_and$BODYset for the endpoint named in the case heading (/v1/messagesadds"max_tokens": 16,/v1/responsessends"input": "hi"):Before (d23bec8)
Every request makes 3 upstream attempts no matter which key is set. The 503 and catch-all keys do not exist yet and are ignored, and the 500 key exists but is never consulted
/v1/chat/completions
PORT=14503 PATH_=/v1/chat/completions/v1/messages
PORT=14503 PATH_=/v1/messages/v1/responses
PORT=14503 PATH_=/v1/responsesAfter (541ab50)
The group whose key matches the error makes exactly 1 attempt. A key for a different error does not leak (the 503 key still lets 500 and 502 retry),
DefaultRetries: 0stops retries on every status, and the group with no policy still makes 3 attempts/v1/chat/completions
PORT=14505 PATH_=/v1/chat/completions/v1/messages
PORT=14505 PATH_=/v1/messages/v1/responses
PORT=14505 PATH_=/v1/responsesType
🐛 Bug Fix
Changes
get_num_retries_from_retry_policyused to be a hand-ordered chain ofisinstancechecks, which is howInternalServerErrorRetriessat unused since May 2024 and why 503, 502, connection and not-found errors had no key at all. It now holds one mapping from exception class toRetryPolicyfield and walks the exception's class hierarchy (its MRO) from the most specific class outward, returning the first field that is set. Subclasses inherit their parent's field when they have none of their own, soContentPolicyViolationErrorfalls back toBadRequestErrorRetriesandMidStreamFallbackErrortoServiceUnavailableErrorRetries. When no class in the chain has a set field, the newDefaultRetriesapplies, so any error can be governed without adding a field per classRetryPolicygainsServiceUnavailableErrorRetriesandDefaultRetries. The Admin UI retry settings tab shows them as "ServiceUnavailableError (503)" and "All other errors", andschema.d.tscarries the new fieldsTests:
tests/test_litellm/router_utils/test_get_retry_from_policy.pyis parametrized over everyRetryPolicyfield, so adding a field without wiring it fails the suite. It also pins that a field does not leak to unrelated errors, that subclasses prefer their own field and fall back to the parent's, thatDefaultRetriescovers 502 and 404, and that a specific field beatsDefaultRetries.tests/test_litellm/test_router.pypoints a deployment at a respx-faked upstream that answers 503, 500 or 502 and counts the HTTP requests the router makes, so the responses travel through the real OpenAI SDK and exception mapping. It covers the 503, 500 andDefaultRetriespaths and pins that a 503 key does not govern a 502. The amplification test intest_router_per_deployment_num_retries.pynow expects 3 upstream requests instead of 6, becauseInternalServerErrorRetries=2finally overrides the per-deploymentnum_retries=5the way the docs always said it wouldCaveats (if any)
Medium
InternalServerErrorRetriesnow honor it on 500s; it was silently ignored beforenum_retriesloses to a matching retry_policy field for that error, same as it always did for 429 and 400Low
MidStreamFallbackErrorinheritsServiceUnavailableErrorRetriesbecause it subclasses that errorServiceUnavailableErrorRetriesandDefaultRetrieslive in the docs repo and need a follow-up PRschema.d.tswas updated by hand for the two new fields only; the schema sync CI check verifies itFinal Attestation