fix(router): stop per-deployment num_retries from double-counting as provider max_retries - #34129
Conversation
|
|
Greptile SummaryThis PR prevents provider retries from multiplying Router retries. The main changes are:
Confidence Score: 5/5This looks safe to merge.
|
| Filename | Overview |
|---|---|
| litellm/main.py | Checks both metadata sources for the Router marker and disables provider retries for routed calls. |
| tests/test_litellm/test_router_per_deployment_num_retries.py | Adds request-counting tests for routed and direct retry behavior and closes the injected async session. |
Reviews (2): Last reviewed commit: "fix(router): make router-origin check ro..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Thanks, addressed the two robustness items in d10a956:
On the "direct calls using Please re-review the current head d10a956 |
|
CI status: 76/77 green. The single red, |
|
@greptileai please review the current head d10a956 — it addresses the metadata/litellm_metadata detection robustness and closes the injected async test clients since your last pass on e346c83 |
…provider max_retries A model group with one deployment and num_retries set in the deployment's litellm_params sent (1 + num_retries) ** 2 requests upstream instead of 1 + num_retries. The deployment's num_retries reached litellm.completion, which copied it onto max_retries and set it on the provider client, so the provider SDK retried num_retries times inside each of the Router's 1 + num_retries attempts. The Router is the sole retry owner for routed calls, so completion() now forces the provider-SDK max_retries to 0 whenever the call originates from the Router/proxy (detected via model_group in the request metadata) and only keeps the num_retries to max_retries alias for direct, non-routed litellm calls (the instructor use case). This also stops a request- or deployment-level max_retries from nesting on top of the Router's retries. Resolves LIT-4385
Address review: detect the router marker in both metadata and litellm_metadata independently (a non-empty metadata without model_group no longer hides a model_group in litellm_metadata), and close the injected async clients in the test fixture.
d10a956 to
688b064
Compare
fcaf673
into
litellm_internal_staging
Relevant issues
Regression from #18975 (which resolved #18968). That change taught the Router to read a per-deployment
num_retriesfromlitellm_params, but the same value also reachedlitellm.completionand became the provider client'smax_retries, so the two retry layers multipliedLinear ticket
Resolves LIT-4385
Pre-Submission checklist
Screenshots / Proof of Fix
This is a retry-counting bug, so the proof counts the actual upstream requests one proxy call produces. A real provider cannot be made to fail deterministically on every attempt, so (exactly as the ticket's own reproduction does) the deployment points at a tiny local upstream that returns 500 on every
POST /v1/chat/completionsand keeps a request counter. The proxy is real, the OpenAI SDK path is real, only the upstream is a controllable failing stand-inConfig used for both runs (matches the ticket: global
num_retries: 1, deploymentnum_retries: 5):Before (unfixed, commit
212a9213c4)One proxy request produced 36 upstream requests, which is
(1 + 5) ** 2After (this PR, commit
d10a956cab)One proxy request produced exactly 6 upstream requests, which is
1 + num_retries. The Router owns the retries and the provider SDK no longer retries on top of itEdge case on the same fixed proxy, proving retries do not nest when a
max_retriesis also supplied in the request body:Independent e2e verification
Devin independently reproduced the same before/after on a fresh clone: one proxy call against an always-500 counting mock upstream produced exactly 36 upstream requests on the unfixed base and exactly 6 on this branch, same config and same single curl, only the code differing
Full-resolution recording: https://raw.githubusercontent.com/yassin-berriai/litellm-pr-media/main/lit-4385/num_retries_amplification.mp4
Type
🐛 Bug Fix
Changes
For a routed call,
litellm.completionpreviously copied the incomingnum_retriesontomax_retriesunconditionally, andmax_retriesis what gets written onto the provider client (_set_dynamic_params_on_client). A deploymentnum_retries: 5therefore drove both the Router's retry loop and the provider SDK's internal retries, so every one of the Router's1 + num_retriesattempts fanned out into1 + num_retriesupstream callsThe Router is the sole retry owner for routed requests, so
completionnow forces the provider-SDKmax_retriesto 0 whenever the call originates from the Router or proxy (detected viamodel_groupin the request metadata, the same signal the@clientwrapper already uses). Thenum_retriestomax_retriesalias is kept only for direct, non-routedlitellmcalls, which is the documented instructor use case. Becausemax_retriesis forced to 0 for routed calls, amax_retriesset at the request or deployment level can no longer nest provider-SDK retries under the Router either, sonum_retries(Router-owned) andmax_retries(provider-SDK, direct calls only) now have distinct, non-overlapping meaningsTests extend the existing mapped file and count real upstream requests through the full Router completion path by injecting a counting transport via
litellm.aclient_session. They cover the deployment, request, deployment-max_retries, retry-policy-present, and global-num_retriescases, and assert the direct-call alias is preservedFinal Attestation