fix(exceptions): don't map upstream 400 to RateLimitError on substring match - #27915
fix(exceptions): don't map upstream 400 to RateLimitError on substring match#27915mateo-berri wants to merge 1 commit into
Conversation
…ng match OpenAI rejects unknown body fields with a 400 whose message echoes the field name back (Unknown parameter / Unrecognized request arguments supplied). The exception mapper was misclassifying those as 429 RateLimitError whenever the field name contained a rate plus limit substring (eg the literal name of an internal proxy stash key leaked into the body by another bug). Two compounding causes in is_error_str_rate_limit. The substring regex had no word boundary, so it matched embedded identifiers. The mapper called the heuristic unconditionally even when original_exception.status_code was 400. Fix one: anchor the substring regex with a leading word boundary. Embedded identifiers no longer match. All standalone phrases still do (Rate limit exceeded, rate_limit_exceeded, rate-limit-exceeded, You hit a rate limit). Fix two: skip the heuristic entirely on a known 400 at the OpenAI / OpenAI-compatible / Mistral mapping site. A 400 is unambiguous per RFC 9110 -- the upstream rejected the body, not the rate. Don't second-guess it. Anthropic's mapper was already correct. Tests: regex anchor tests for substring vs standalone matching. End-to-end mapper test that builds a real openai.BadRequestError with status 400 and the wire messages we saw, asserts the result is litellm.BadRequestError (not RateLimitError). Plus a 429-path regression guard to prove the gate didn't break legitimate throttling detection. Co-authored-by: Mateo Wang <mateo-berri@users.noreply.github.com>
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR fixes a misclassification bug where upstream OpenAI 400 responses whose body contained
Confidence Score: 4/5Safe to merge; the two changes are minimal and targeted, and the tests confirm the core regression is fixed without breaking the genuine 429 path. Both changes are narrow and well-tested. The only gap is that the leading \b anchor doesn't protect field names that start without an underscore (e.g. rate_limit_descriptors) when status_code is absent — the status-code gate covers that in the common case, but the heuristic alone still has a residual false-positive surface. This is a minor hardening gap rather than a present defect for the reported bug. No files require special attention beyond the noted regex boundary edge case in exception_mapping_utils.py.
|
| Filename | Overview |
|---|---|
| litellm/litellm_core_utils/exception_mapping_utils.py | Adds a \b word-boundary anchor to the rate-limit regex and a status-code-400 gate before the string heuristic; correctly prevents misclassification of upstream 400s as RateLimitError when field names embed "rate_limit" |
| tests/test_litellm/litellm_core_utils/test_exception_mapping_utils.py | New tests cover: field-name substring false-positive regression, standalone rate-limit true-positive preservation, end-to-end exception_type 400→BadRequestError mapping, and 429 regression guard; no real network calls, all mock-based |
Reviews (1): Last reviewed commit: "fix(exceptions): do not map upstream 400..." | Re-trigger Greptile
| # (e.g. "Unknown parameter: _litellm_rate_limit_descriptors") don't | ||
| # get misclassified as throttling. Without the boundary, the field | ||
| # name's embedded ``rate_limit`` matched and turned a 400 into a 429. | ||
| if re.search(r"\brate[\s_\-]*limit", _error_str_lower): |
There was a problem hiding this comment.
Partial anchor — trailing word boundary missing
\brate[\s_\-]*limit anchors the start of rate but not the end of limit, so a field name like "rate_limit_descriptors" (no leading underscore, e.g. "Unknown parameter: 'rate_limit_descriptors'.") still matches when there is no preceding underscore. The status-code gate at line 414 saves the case where status_code == 400 is known, but if status_code is None (some SDKs don't expose it), the string heuristic still fires on those identifiers and returns True. Adding a trailing \b (\brate[\s_\-]*limit\b) would fully close this, but note it would also stop matching rate_limit_exceeded (where _ follows limit) — so the right fix is probably a trailing (?:[\s\-]|$) boundary or a more explicit positive-lookahead rather than \b.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
🚅 Hi, thanks for the PR! I'm Agent Shin, the automated triage bot for this repository. What's this and why am I getting it? I read the description against our contribution rubric. Here's how it lined up: What you got right:
What's still missing:
If the description isn't updated in the next 2 hours, I'll auto-close this PR. That's not us saying we don't care about the change; we want the open-PR list to mirror what a maintainer can act on right now, so contributors don't get lost in a backlog. A closed PR is a soft "park this for later," not a rejection. Take your time; everything below still works after the close. During the grace period: just update the PR description with the missing pieces. No need to ping me; I'll re-check on the next sweep and skip the auto-close if it now passes. See what counts as QA proof for the full rubric (a linked issue alone isn't enough; it covers context, not proof). If the PR does get auto-closed in 2 hours, you still have easy recovery paths:
Internal BerriAI contributors: this rubric doesn't apply to you; ping a maintainer. (I'm an LLM, so I'm not infallible. If you think I got this wrong, ping a maintainer; they'll override me.) |
|
🚅 Hi, thanks for the PR! I'm Agent Shin, the automated triage bot for this repository. What's this and why am I getting it? I read the description against our contribution rubric. Here's how it lined up: What you got right:
What's still missing:
Closing this PR isn't a rejection of the change. We want the open-PR list to mirror what a maintainer can act on right now, so contributors don't get lost in a backlog. A closed PR is a soft "park this for later"; your work is still here, the diff is still here, and getting it reopened is one comment away. Take your time. To bring this PR back:
What "end-to-end QA proof" means, since it's the most common gap: at least one of a short before/after screen recording / video (the bug reproducing, then the fix working; for a brand-new feature, a recording of it working end-to-end), a screenshot (or before/after screenshots) of it working, or the exact commands you ran paired with their real output against the real system. Running Internal BerriAI contributors: this rubric doesn't apply to you; ping a maintainer. (I'm an LLM, so I'm not infallible. If you think I got this wrong, comment |
Problem
OpenAI's
Unknown parameter: '<field>'/Unrecognized request arguments supplied: <field1>, <field2>400s, which echo the rejected field name back in the message, get misclassified as 429RateLimitErrorwhenever the field name containsrate_limit(or anyrate[\s_\-]*limitsubstring) — e.g._litellm_rate_limit_descriptors.Concretely: while reproducing the v3 limiter leak (PR #27001 → PR #27913), OpenAI returned
and LiteLLM surfaced it to the client as
That breaks two contracts:
429 throttling_errorand assume "back off and retry" — but the upstream rejected the body, retrying won't help. Loops forever on a malformed request.Root cause
In
litellm/litellm_core_utils/exception_mapping_utils.py:Cause 1 — unanchored regex:
ExceptionCheckers.is_error_str_rate_limitusesre.search(r"rate[\s_\-]*limit", _error_str_lower). No word boundary, so it matchesrate_limitas a substring of any identifier. Real OpenAI 400 body fields named withrate_limit(or LiteLLM-internal field names like_litellm_rate_limit_descriptorsleaked into the body) all trip the heuristic.Cause 2 — heuristic runs before status code: Inside the OpenAI / OpenAI-compatible / Mistral branch of
exception_type,is_error_str_rate_limitis called unconditionally — before the status-code branch that handles 400 →BadRequestError. So an explicit upstream 400 never gets a chance to map correctly when the string matches.Anthropic's branch was already correct: it routes 400/413 to
BadRequestErrorvia status code, no string heuristic up front.(Companion observation in PR #16482, which tightened
\b429\bagainst429substrings — same shape of bug, different token.)Fix
Anchor the regex with a leading word boundary:
\brate[\s_\-]*limit. Embedded identifiers like_litellm_rate_limit_descriptorsno longer match (the preceding_is a word char so\bfails). Standalone phrases all still match:Rate limit exceeded,rate_limit_exceeded,rate-limit-exceeded,You hit a rate limit.Status-code gate at the mapping site: when
getattr(original_exception, "status_code", None) == 400, skip the string heuristic entirely. A 400 is unambiguous per RFC 9110 — the upstream rejected the body, not the rate. Don't second-guess it.Both fixes are independent and complementary: the regex fix protects unknown-status-code paths (some upstream errors don't carry
status_code), and the gate protects against any future heuristic that could mistake a 400 message for a 429.Tests
tests/test_litellm/litellm_core_utils/test_exception_mapping_utils.py:test_is_error_str_rate_limit_ignores_field_name_substring— three forms of the OpenAI 400 wire message all return False.test_is_error_str_rate_limit_still_matches_standalone_token— proves the tightening didn't regress legitimate detections.TestExceptionTypeStatusCodeGate— end-to-end mapper tests. Build a realopenai.BadRequestErrorwith status 400 and the wire messages we saw, runexception_type, assert the result islitellm.BadRequestError(notRateLimitError).test_openai_429_still_maps_to_rate_limit— confirms the genuine 429 path is untouched.Related
Companion to #27913 (which fixes the underlying v3 limiter leak that this misclassification was hiding).
Slack Thread