feat(router): treat configured finish reasons as deployment failures - #41825
paoloantinori wants to merge 10 commits into
Conversation
- one _handle_mapped_finish_reason_failure (account, gate, raise) replaces the duplicated checkpoint blocks and the double exception build - _generic_fallback_available shared by the refusal and knob availability gates - accounting honors a deployment-level cooldown_time like deployment_callback_on_failure - map+stash of the native finish reason lives in one helper (map_finish_reason_and_stash_native) shared by Choices.__init__ and the anthropic transform - knob-off requests pay a single None check at every checkpoint
- warn at construction that the knob applies to non-streaming responses only - warn when a key names a healthy terminal reason in the mapped OpenAI set - a deployment without model_info id still raises into the fallback chain when one can serve; only the accounting needs the id
|
|
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if not self.treat_finish_reason_as_failure: | ||
| return None | ||
| if not (response.choices and len(response.choices) > 0): | ||
| return None |
There was a problem hiding this comment.
choices[0] is inspected, so configured failures in later choices bypass cooldown and fallback handling
Knowledge Base Used: Model invocation runtime
There was a problem hiding this comment.
Keeping the choices[0] read: it matches the adjacent _should_raise_content_policy_error check this knob generalizes (same block, two lines above), and a single completion carries one terminal reason across its choices. Consistency with the sibling check is the behavior a reviewer can predict here.
There was a problem hiding this comment.
That’s fair. _should_raise_content_policy_error is the adjacent precedent in the same block, and this response shape carries one terminal reason across its choices, so reading choices[0] is intentional and predictable here. I’m withdrawing this finding; no change is needed.
| fallbacks: list = [], | ||
| context_window_fallbacks: list = [], | ||
| content_policy_fallbacks: list = [], | ||
| treat_finish_reason_as_failure: dict[str, str] | None = None, |
There was a problem hiding this comment.
/config/update persists this argument without validation. Invalid mappings are ignored live but fail the next Router startup
There was a problem hiding this comment.
Verified against the update path: /config/update validates keys, persists router_settings to the config store, and never constructs a Router in place, so an invalid value surfaces at the next startup. That is the existing behavior for every init-validated Router parameter, and the ValueError names the offending value, so the failure is loud and actionable. Nothing specific to this knob.
There was a problem hiding this comment.
Thanks for verifying. Since /config/update already persists init-validated router_settings without reconstructing Router, and construction raises an explicit ValueError on restart, this is existing behavior shared by other Router parameters rather than a regression specific to this knob. I withdraw the finding.
| ## Exception classes a treat_finish_reason_as_failure value may name: resolved from litellm at | ||
| ## use time, validated at Router construction. |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Keeping the two module-level notes: the ## label blocks match the surrounding house style, including the ## CHECK CONTENT FILTER ERROR ## checkpoints this change extends. They carry the allowlist semantics a reader cannot get from the frozenset alone.
There was a problem hiding this comment.
That context is fair. These are concise semantic labels rather than narrative comments: they document the allowlist’s purpose and the healthy-reason warning, and they match the surrounding ## checkpoint style. I retract the comment; no change is needed.
| ) | ||
| return resolved is not None | ||
|
|
||
| def _handle_mapped_finish_reason_failure(self, model: str, deployment: dict, reason: str, kwargs: dict) -> None: |
There was a problem hiding this comment.
dict and Any parameters violate the repository's fully typed requirement. Replace them with precise types before merging
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!
There was a problem hiding this comment.
The new helper signatures use Mapping[str, Any], and the earlier bare dict parameters were converted in 2210f94: the repository's type-discipline gate holds every LIT rule at or below the base count on this branch. Any as the value type matches the file-wide convention.
There was a problem hiding this comment.
You're right. The new helper signatures in litellm/router.py:8691 use Mapping[str, Any], and the earlier bare dict parameters were converted in 2210f94. Since Any matches the file-wide convention and the type-discipline count is not regressing, the typing finding is resolved. No further change is needed.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
| async def _send(_client: httpx.AsyncClient, request: httpx.Request, **kwargs: Any) -> httpx.Response: | ||
| return await self.send(request, **kwargs) | ||
|
|
||
| monkeypatch.setattr(httpx.AsyncClient, "send", _send) |
There was a problem hiding this comment.
httpx.AsyncClient.send globally violates the dependency-injection testing rule. Use an injected transport before merging
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!
There was a problem hiding this comment.
Keeping the transport patch: it mirrors the FakeAnthropicUpstream pattern in tests/router_unit_tests/test_router_anthropic_messages_fallback.py, the anchor file this suite was written against. The router does not forward caller-injected clients, so the transport is the seam that exercises the real pipeline.
There was a problem hiding this comment.
That context changes my assessment. The existing test_router_anthropic_messages_fallback.py establishes transport interception as the accepted seam for these router pipeline tests, and the monkeypatch fixture restores httpx.AsyncClient.send after each test. Since the router does not forward caller-injected clients, this is appropriate here. I withdraw the finding; no change is needed.
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
| litellm_router_instance=self, | ||
| deployment_id=deployment_id, | ||
| ) | ||
| _set_cooldown_deployments( |
There was a problem hiding this comment.
Low: Caller-controlled responses poison shared deployment health
Finish reasons can depend on caller-controlled input, such as prompt size or output limits. When one of those reasons is configured, a client can repeatedly trigger it to increment the deployment-wide failure counter and cool down the deployment for every tenant; with RateLimitError and multiple deployments, the cooldown can occur after one request. Keep fallback handling request-scoped for caller-induced reasons, or require a separate explicit option before applying them to shared failure and cooldown accounting.
There was a problem hiding this comment.
Known and deliberate. The same caller-induced parking already applies to content_policy_fallbacks today (a client whose prompts trip content filters cools the deployment for everyone), so the knob inherits an existing operator tradeoff rather than introducing one. Construction warns when a key names a healthy terminal reason such as stop or length, which is the caller-controlled vector; mapping provider-quota reasons like model_context_window_exceeded is the intended use and is not caller-controlled.
PR overviewThis PR updates the router to treat configured response finish reasons as deployment failures, integrating them into failure and cooldown handling. One security issue remains open. Caller-influenced finish reasons can affect shared deployment health accounting, allowing a client to trigger cooldowns that disrupt routing for other tenants when the feature is configured. No issues have yet been addressed. Open issues (1)
Fixed/addressed: 0 · PR risk: 5/10 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…lpers by name - Mapping annotations and Final locals keep every LIT rule at or below the base count - the stash helper builds under a new name; its mutable copy carries a reason - direct helper calls in the test satisfy the router code-coverage name check
|
The documentation check stays red until BerriAI/litellm-docs#1544 lands: the validation test requires every |
|
The |
2 participants
Why
A provider can report a terminal condition as a stop reason on an HTTP 200. When a z.ai GLM Coding Plan account runs out of credits, every response comes back 200 with stop_reason "model_context_window_exceeded" and empty content: no failure is counted, allowed_fails never trips, cooldown never engages, fallbacks never fire (#38535 has the outage evidence). Upstream already special-cases one reason this way: #39157 raises on the Anthropic safeguard refusal in the generic dispatch, and #39274 arms content-filter fallbacks on default fallbacks alone. This PR generalizes that pattern into operator config.
Scope
Router(treat_finish_reason_as_failure={"model_context_window_exceeded": "RateLimitError"}): a reason to exception-name map, allowlisted and validated at construction, accepted fromrouter_settingsyaml through the existingget_valid_argspath._completionand_acompletioncheckpoints, and in_ageneric_api_call_with_fallbacks_helpergated onanthropic_messages(reading the rawstop_reasonbefore any translation). Detection matches both the mapped finish reason and the pre-mapping value inprovider_specific_fields["native_finish_reason"]._set_cooldown_deploymentspair thatdeployment_callback_on_failureruns, honoring a deployment-levelcooldown_time. The raise into the fallback chain fires only when a generic fallback can serve. Raising sites account explicitly, because a post-200 raise never flows through litellm's failure callbacks.native_finish_reasonthrough the sharedmap_finish_reason_and_stash_native, matching whatChoices.__init__already does for other providers._aanthropic_messages_streaming_iterator), anymap_finish_reasonchange (the global passthrough stays closed per fix(core): preserve unmapped finish_reason and add 'refusal' mapping #23800), the refusal special case.Tradeoffs
Reading the pre-mapping reason instead of changing the mapping keeps the knob opt-in; #23800 settled against a global passthrough. Explicit accounting at the raise sites does more than the content-filter path does today, which cools nothing without callbacks; that engagement is the issue's core ask.
Blast Radius
Zero behavior change unless the knob is configured, except the
native_finish_reasonstash, which makes the anthropic chat path consistent with the other providers.Verification
tests/router_unit_tests/test_router_finish_reason_failure.py: fallback fires and cooldown engages on both dispatch shapes; a no-fallback config returns the response unchanged and still parks the deployment; the knob unset keeps the old behavior; an unknown exception name fails at construction. Five tests green, andtest_router_anthropic_messages_fallback.pystays green (23 passed). Refs #38535.