Repository navigation
fix(router): don't cool down parent deployment on advisor sub-call failure - #33792
yassin-berriai merged 4 commits into
Conversation
…ilure Advisor orchestration issues a sub-call to a different provider/credentials than the selected deployment. When that sub-call fails (e.g. a 401 because no advisor API key is configured), the exception propagates up and the router's deployment_callback_on_failure attributes it to the healthy parent deployment's model_info.id, cooling it down and rejecting unrelated callers to the same model group. Tag advisor sub-call failures on the exception and skip cooldown for them in deployment_callback_on_failure. The exception is tagged rather than wrapped so its type is preserved and retry/fallback classification and the client-facing error are unchanged. Genuine executor/deployment failures are untagged and still cool down as before. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Greptile SummaryThis PR prevents the router from cooling down a healthy deployment when an advisor sub-call (which targets a different provider/credentials) fails. The fix tags the exception at the point of failure and short-circuits
Confidence Score: 4/5The core fix is correct and well-tested; the main open question is whether AdvisorMaxIterationsError should also be exempt from incrementing the failure counter. The tagged-exception approach is sound and the two test classes exercise the happy and unhappy paths. The AdvisorMaxIterationsError path goes untagged, which can still increment the failure counter for the healthy deployment over repeated max-iteration errors, albeit not trigger full cooldown unless allowed_fails is reached. Importing an Anthropic-specific helper into the generic router is a minor coupling concern. litellm/router.py and advisor.py — specifically the untagged AdvisorMaxIterationsError path and the provider-specific import in the router.
|
| Filename | Overview |
|---|---|
| litellm/llms/anthropic/experimental_pass_through/messages/interceptors/advisor.py | Adds tagging helpers and wraps the advisor sub-call in try/except to mark failures; AdvisorMaxIterationsError is not tagged so the failure counter can still be incremented for that path. |
| litellm/router.py | Adds an early-return guard in deployment_callback_on_failure for tagged advisor sub-call failures; imports an Anthropic-specific advisor helper directly into the generic router. |
| tests/test_litellm/llms/anthropic/messages/test_advisor_orchestration.py | Adds two new async tests verifying that advisor sub-call failures are tagged while executor failures are not; tests use mocks with no real network calls. |
| tests/test_litellm/test_router.py | Adds TestAdvisorSubCallCooldown with two tests covering the tagged and untagged paths through deployment_callback_on_failure; correctly verifies cooldown list membership. |
Comments Outside Diff (1)
-
litellm/llms/anthropic/experimental_pass_through/messages/interceptors/advisor.py, line 145-150 (link)AdvisorMaxIterationsErrornot tagged — failure counter still incrementedWhen the advisor loop exceeds
max_uses,AdvisorMaxIterationsErrorpropagates without the advisor-sub-call tag. The router'sdeployment_callback_on_failurewill callincrement_deployment_failures_for_current_minutefor the parent deployment even though the executor succeeded on every iteration. Over multiple such timeouts on the same model group, the failure counter can reachallowed_fails, cooling down the healthy deployment — the same class of bug this PR fixes for the sub-call path.
Reviews (1): Last reviewed commit: "fix(router): don't cool down parent depl..." | Re-trigger Greptile
| from litellm.llms.anthropic.experimental_pass_through.messages.interceptors.advisor import ( | ||
| is_advisor_sub_call_failure, | ||
| ) | ||
|
|
||
| if is_advisor_sub_call_failure(exception): | ||
| verbose_router_logger.debug( | ||
| "Router: Exiting 'deployment_callback_on_failure' without cooldown. " | ||
| "Failure originated from an advisor sub-call, not the selected deployment." | ||
| ) | ||
| return False |
There was a problem hiding this comment.
Provider-specific import in generic router component
Importing is_advisor_sub_call_failure from a deeply-nested Anthropic advisor module inside deployment_callback_on_failure tightly couples the generic router to a single provider's orchestration feature. Any other interceptor (Bedrock, Vertex, etc.) that wants the same "skip cooldown" guarantee would need a separate import here. A more maintainable pattern would be to check a shared, generic exception attribute (e.g., a utility in litellm/utils.py or a base interceptor sentinel) so the router stays provider-agnostic.
Rule Used: What: Avoid writing provider-specific code outside... (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!
Runtime test results (LIT-4565)Tested on a live litellm proxy at
After the fix (commit
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…_subcall_cooldown Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
yassin-berriai
left a comment
There was a problem hiding this comment.
Reviewed the fix end to end. The core change is correct, minimal, and the live-proxy before/after is exactly the right kind of proof. A few notes below
What holds up
The tagging mechanism survives end to end. Logging._failure_handler_helper_fn stores the same exception object via self.model_call_details["exception"] = exception (litellm_logging.py:2698), which is what deployment_callback_on_failure later reads through kwargs.get("exception"), so the setattr tag is preserved by object identity. The 429 to 200 flip in the proof confirms it in practice
Scoping is right. The try/except wraps only the advisor sub-call (advisor.py:136-150); the executor call and its re-calls after injection sit outside it, so genuine deployment failures stay untagged and keep cooling down. test_executor_failure_is_not_tagged locks that in
deployment_callback_on_failure is the only cooldown-on-failure path this scenario hits. async_deployment_callback_on_failure only bumps RPM, and the other _set_cooldown_deployments at router.py:7304 fires solely on a pre-call RateLimitError, so the single fix location is complete for this case
The tests are real regressions with good mutation kill: removing the setattr, widening the try to cover the executor, or dropping the early return each break a test, and the positive test_untagged_auth_error_cools_down_deployment guards that normal cooldown still works
Findings
-
AdvisorMaxIterationsErroris untagged, so the same healthy-deployment attribution can still happen through the max-iterations path. It is raised at advisor.py:127, outside the try/except, even though the executor succeeded on every iteration.deployment_callback_on_failurethen callsincrement_deployment_failures_for_current_minutefor the parent, and over repeated max-iteration errors on the same group the counter can reachallowed_failsand cool the healthy deployment down, which is the same class of bug this PR fixes for the sub-call path. It is an orchestration-level failure rather than a deployment-health one, so it belongs in the same exemption; tagging it before the raise closes the gap. This is the one substantive item, and it lines up with the Greptile 4/5 open question -
The generic router now imports an Anthropic-specific helper, and does it in the function body.
deployment_callback_on_failureadds afrom litellm.llms.anthropic...advisor import is_advisor_sub_call_failureinside the method. Two things here: the repo convention is no new in-function imports (the only sanctioned exception is proxy-only deps, which this is not), and a provider-agnostic router taking a dependency on an anthropic module is the coupling Greptile flagged. Both resolve together by moving the sentinel plus themark_/is_predicate into a provider-neutral util that the router imports at module top. If the in-function placement was there to avoid a circular import, that relocation also removes the cycle -
Minor test nit:
test_untagged_auth_error_cools_down_deploymentis marked@pytest.mark.asyncioand declaredasyncbut awaits nothing and calls a sync method, while its sibling is a plain syncdefdoing the same kind of work. Not a problem, just inconsistent
CI
The two red shards, proxy-server and proxy-endpoints, are not from this diff. The failures are ValueError: not enough values to unpack (expected 2, got 0) out of router.get_configured_token_limits via litellm/proxy/utils.py:6139 in test_team_model_name_translation.py, code this PR does not touch. Newer open PRs pass these same shards, so this is staging drift captured at this PR's merge ref; a rebase onto current litellm_internal_staging and a re-run should clear both. Worth flipping the "passes all CI/CD checks" checklist item back until it is green
Net: fix is correct and well-tested. Addressing finding 1 (tag AdvisorMaxIterationsError, with a test covering that path) is what I would gate on; findings 2 and 3 are quality
…tral util Address review on LIT-4565: move the cooldown-exemption marker into litellm/router_utils/cooldown_handlers.py so the router imports it at module top instead of an in-function anthropic import, and extend the exemption to AdvisorMaxIterationsError so a max-iterations orchestration failure no longer cools down the healthy executor deployment. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…itellm_lit_4565_advisor_subcall_cooldown
|
|
|
Thanks for the thorough review @yassin-berriai. Addressed in 99ddba0 (plus a merge of current Finding 1: Finding 2: the sentinel and the Finding 3: I kept CI: merged current |
…ilure (BerriAI#33792) * fix(router): don't cool down parent deployment on advisor sub-call failure Advisor orchestration issues a sub-call to a different provider/credentials than the selected deployment. When that sub-call fails (e.g. a 401 because no advisor API key is configured), the exception propagates up and the router's deployment_callback_on_failure attributes it to the healthy parent deployment's model_info.id, cooling it down and rejecting unrelated callers to the same model group. Tag advisor sub-call failures on the exception and skip cooldown for them in deployment_callback_on_failure. The exception is tagged rather than wrapped so its type is preserved and retry/fallback classification and the client-facing error are unchanged. Genuine executor/deployment failures are untagged and still cool down as before. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * refactor(router): tag advisor orchestration failures via provider-neutral util Address review on LIT-4565: move the cooldown-exemption marker into litellm/router_utils/cooldown_handlers.py so the router imports it at module top instead of an in-function anthropic import, and extend the exemption to AdvisorMaxIterationsError so a max-iterations orchestration failure no longer cools down the healthy executor deployment. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --------- Co-authored-by: shivam <shivam@berri.ai> Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Relevant issues
Linear ticket
Resolves LIT-4565
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Screenshots / Proof of Fix
Live proxy, real Fireworks executor + real Anthropic advisor sub-call (no mocks). The proxy is started with no
ANTHROPIC_API_KEYso the advisor sub-call gets a genuine 401, which is the condition from the ticket.claude-sonnet-5maps to a single non-native deployment (fireworks_ai/accounts/fireworks/models/gpt-oss-120b); the advisor tool resolves toapi.anthropic.comdirectly, the same non-native path Bedrock takesConfig used
Both runs send the same two requests back to back: first an advisor request (forced
tool_choiceso the advisor sub-call always fires), then an unrelated well-formed request to the same model groupBefore the fix, at base commit
c5b4456401The advisor sub-call's 401 cooled down the healthy parent deployment, so the unrelated caller is rejected with a 429 naming the deployment in
cooldown_listAfter the fix, at commit
28b46ae93cThe advisor request still surfaces its own 401 (that is the advisor being misconfigured, unchanged), but the parent deployment is no longer cooled down, so the unrelated caller succeeds with a 200
Type
🐛 Bug Fix
Changes
The advisor orchestration handler issues a sub-call to the advisor model, which resolves to a different provider and credentials than the deployment the router selected for the parent request. When that sub-call fails, for example a 401 because no advisor API key is configured, the exception propagates up to
Router.deployment_callback_on_failure, which keys cooldown off the parent deployment'smodel_info.idand never consulted whether the failure actually came from the deployment. The healthy deployment gets cooled down and unrelated callers to the same model group are rejectedA failure that originates from advisor orchestration now carries a signal to that effect. The sentinel and its
mark_advisor_orchestration_failure/is_advisor_orchestration_failurepredicate live inlitellm/router_utils/cooldown_handlers.py, which is provider-neutral and already imported by the router, sodeployment_callback_on_failurereads the tag via a module-top import rather than reaching into an Anthropic module.AdvisorOrchestrationHandler.handletags two orchestration-level failures: the advisor sub-call (wrapped in a try/except) andAdvisorMaxIterationsErrorwhen the loop exhaustsmax_useswhile the executor keeps succeeding.deployment_callback_on_failurereturns early without incrementing failures or setting cooldown when the tag is present. The exception is tagged rather than wrapped, so its type, the router's retry and fallback classification, and the client-facing error are all unchanged. Genuine executor or deployment failures are never tagged and keep cooling down exactly as beforeRegression tests
tests/test_litellm/llms/anthropic/messages/test_advisor_orchestration.pyasserts that a failing advisor sub-call and a max-iterations failure both propagate a tagged exception, while an executor failure does not get taggedtests/test_litellm/test_router.py::TestAdvisorSubCallCooldownasserts thatdeployment_callback_on_failurecools the deployment down for an untagged auth error but skips cooldown for a tagged advisor orchestration failureFinal Attestation
Link to Devin session: https://app.devin.ai/sessions/d3f8fab3d4fa460887ced329ee7c1b44
Requested by: @shivamrawat1