fix(rate-limiting): close scoping-field dedup gaps and reject NaN limits

resolve_any's own routing-group dedup key omitted the four scoping fields
(included_values, excluded_values, enabled_for, disabled_for), unlike
_build_group_limits's dedup signature which already folded them in --
members disagreeing only on scope collapsed to whichever model_name
sorted first, silently applying the wrong member's policy. Folds the same
four fields into resolve_any's own key.

included_values/excluded_values and TagRateLimitScope.values entered the
dedup signature in raw config order with no normalization, so two
deployments declaring the identical set in a different order were treated
as genuinely divergent policies instead of deduping to one chain-wide
entry. Both now normalize to a sorted, deduplicated tuple at construction.

A NaN limit made atomic requests/concurrency admission admit indefinitely
while read-only tokens/dollars checks rejected every tagged request,
since NaN compares false against every ordering operator either way.
Rejected at config load time.
This commit is contained in:
Deepanshu 2026-08-21 18:14:47 -04:00
parent 1b1c22991f
commit db989da2af
3 changed files with 161 additions and 0 deletions

View file

@ -468,6 +468,10 @@ class _LimitsIndex:
limit.entry.limit,
limit.entry.period_seconds,
limit.entry.scope_by_key_hash,
limit.entry.included_values,
limit.entry.excluded_values,
_scope_signature(limit.entry.enabled_for),
_scope_signature(limit.entry.disabled_for),
limit.deployment_scope,
limit.team_scope,
)

View file

@ -4,6 +4,7 @@ litellm.Router Types - includes RouterConfig, UpdateRouterConfig, ModelInfo etc
import datetime
import enum
import math
from collections.abc import Mapping
from dataclasses import dataclass
from typing import Any, ClassVar, Final, Generic, Literal, TypeVar, get_type_hints
@ -157,6 +158,22 @@ class TagRateLimitScope(BaseModel):
raise ValueError("values must be a non-empty list of strings")
return self
@model_validator(mode="after")
def _normalize_values(self) -> "TagRateLimitScope":
# Sorted and deduplicated: `values` is only ever used for membership
# tests (see _entry_applies), never order-dependent, but is also
# folded verbatim into the dedup signature two deployments' entries
# are compared by (see _scope_signature) -- an unsorted tuple would
# make config-order alone, not policy, decide whether two entries
# dedup to one shared bucket or wrongly split into two.
# object.__setattr__ bypasses this frozen model's own assignment
# guard -- returning a replacement instance from an "after" validator
# is silently ignored when constructing via __init__ (only takes
# effect via model_validate), so mutating in place is the only way
# this normalization reliably applies regardless of construction path.
object.__setattr__(self, "values", tuple(sorted(set(self.values))))
return self
class TagRateLimitEntry(BaseModel):
name: str
@ -199,6 +216,19 @@ class TagRateLimitEntry(BaseModel):
model_config = ConfigDict(protected_namespaces=())
@model_validator(mode="after")
def _validate_limit(self) -> "TagRateLimitEntry":
# NaN compares False against every ordering operator, so a NaN limit
# makes the atomic requests/concurrency check-and-increment (which
# rejects when the new value exceeds the limit) never reject --
# admitting indefinitely -- while the read-only tokens/dollars check
# (which admits when the current value is under the limit) never
# admits, rejecting every tagged request. Either outcome silently
# defeats the entry; reject it at config load time instead.
if math.isnan(self.limit):
raise ValueError("limit must not be NaN")
return self
@model_validator(mode="after")
def _validate_period_seconds(self) -> "TagRateLimitEntry":
if self.period_seconds <= 0:
@ -230,6 +260,19 @@ class TagRateLimitEntry(BaseModel):
raise ValueError("excluded_values must be a non-empty list of strings when set")
return self
@model_validator(mode="after")
def _normalize_included_and_excluded_values(self) -> "TagRateLimitEntry":
# Sorted and deduplicated for the same reason as
# TagRateLimitScope._normalize_values: only ever used for membership
# tests, but also folded verbatim into the dedup signature, where an
# unsorted tuple would make config-order alone decide whether two
# deployments' entries dedup to one shared bucket.
if self.included_values is not None:
self.included_values = tuple(sorted(set(self.included_values)))
if self.excluded_values is not None:
self.excluded_values = tuple(sorted(set(self.excluded_values)))
return self
class TagRateLimitGroup(BaseModel):
limits: tuple[TagRateLimitEntry, ...] = ()

View file

@ -235,6 +235,23 @@ def test_extract_team_id_ignores_a_forged_value_in_the_non_authoritative_field()
assert _extract_team_id(request_kwargs, "litellm_metadata") == "real-team"
# ---------------------------------------------------------------------------
# TagRateLimitEntry -- limit validation
# ---------------------------------------------------------------------------
def test_tag_rate_limit_entry_rejects_nan_limit():
"""
NaN compares False against every ordering operator, so a NaN limit makes
the atomic requests/concurrency check-and-increment (rejects when the new
value exceeds the limit) admit indefinitely, while the read-only
tokens/dollars check (admits when the current value is under the limit)
rejects every tagged request -- either way silently defeating the entry.
"""
with pytest.raises(ValidationError, match="limit must not be NaN"):
TagRateLimitEntry(name="n", limit=float("nan"), period_seconds=60)
# ---------------------------------------------------------------------------
# TagRateLimitEntry -- period_seconds validation
# ---------------------------------------------------------------------------
@ -442,6 +459,30 @@ def test_tag_rate_limit_entry_rejects_enabled_for_missing_values():
TagRateLimitEntry(name="daily", limit=1, period_seconds=60, enabled_for={"tag_id": "company_id"})
def test_tag_rate_limit_entry_normalizes_included_and_excluded_values_order_and_duplicates():
"""
These fields are only ever used for membership tests (order never
matters for behavior) but are folded verbatim into the dedup signature
two deployments' entries are compared by -- an unsorted, undeduplicated
tuple would make config-order alone, not policy, decide whether two
entries dedup to one shared bucket or wrongly split into two.
"""
entry = TagRateLimitEntry(
name="daily",
limit=1,
period_seconds=60,
included_values=("b", "a", "a"),
excluded_values=("d", "c"),
)
assert entry.included_values == ("a", "b")
assert entry.excluded_values == ("c", "d")
def test_tag_rate_limit_scope_normalizes_values_order_and_duplicates():
scope = TagRateLimitScope(tag_id="company_id", values=("1032", "1001", "1001"))
assert scope.values == ("1001", "1032")
# ---------------------------------------------------------------------------
# _build_group_limits -- scoping fields fold into the dedup signature
# ---------------------------------------------------------------------------
@ -515,6 +556,43 @@ def test_build_group_limits_chain_wide_when_excluded_values_agree():
assert configured[0].deployment_scope is None
def test_build_group_limits_chain_wide_when_excluded_values_agree_in_different_order():
"""
Two deployments declaring the identical excluded_values set, just in a
different config order, must dedup to one chain-wide entry -- config
order is not a policy difference. Relies on TagRateLimitEntry's own
normalization (sorting) of included_values/excluded_values at
construction time, not on this dedup path re-sorting them itself.
"""
deployments = [
_deployment(
"grp",
"dep-1",
{
"token_limits": {
"limits": [
{"name": "daily", "limit": 500, "period_seconds": 86400, "excluded_values": ["u1", "u2"]}
]
}
},
),
_deployment(
"grp",
"dep-2",
{
"token_limits": {
"limits": [
{"name": "daily", "limit": 500, "period_seconds": 86400, "excluded_values": ["u2", "u1"]}
]
}
},
),
]
configured = _build_group_limits(deployments, "tokens")
assert len(configured) == 1
assert configured[0].deployment_scope is None
# ---------------------------------------------------------------------------
# async_filter_deployments -- enforcement
# ---------------------------------------------------------------------------
@ -764,6 +842,42 @@ def test_resolve_any_keeps_divergent_signatures_across_member_model_names_separa
assert {c.entry.limit for c in resolved} == {1, 2}
def test_resolve_any_keeps_divergent_excluded_values_across_member_model_names_separate():
"""
resolve_any's own dedup key omitted included_values/excluded_values/
enabled_for/disabled_for, so two routing-group members agreeing on
tag_id/limit/period_seconds but declaring different excluded_values
collapsed to whichever model_name sorted first -- silently applying the
wrong member's policy (and, for the discarded one, no enforcement or
accounting at all for callers only that policy covers). This is the same
class of bug test_build_group_limits_per_deployment_when_excluded_values_diverge
already guards against for the sibling load-balanced-group dedup path.
"""
concurrency_limits_excluding_u1 = {
"concurrency_limits": {
"limits": [
{"name": "inflight", "tag_id": "end_user_id", "limit": 1, "period_seconds": 300, "excluded_values": ["u1"]}
]
}
}
concurrency_limits_excluding_u2 = {
"concurrency_limits": {
"limits": [
{"name": "inflight", "tag_id": "end_user_id", "limit": 1, "period_seconds": 300, "excluded_values": ["u2"]}
]
}
}
index = _build_limits_index(
[
_deployment("backend-a", "dep-a", concurrency_limits_excluding_u1),
_deployment("backend-b", "dep-b", concurrency_limits_excluding_u2),
]
)
resolved = index.resolve_any("my-group", team_id=None, candidate_model_names=("backend-a", "backend-b"))
assert len(resolved) == 2
assert {c.entry.excluded_values for c in resolved} == {("u1",), ("u2",)}
def test_resolve_any_picks_the_same_resolved_group_regardless_of_hash_seed():
"""
Two members with an identical signature dedup to whichever one