diff --git a/litellm/proxy/hooks/model_based_tag_rate_limits_hook.py b/litellm/proxy/hooks/model_based_tag_rate_limits_hook.py index 25dbe9aa420..8cfef5b31bd 100644 --- a/litellm/proxy/hooks/model_based_tag_rate_limits_hook.py +++ b/litellm/proxy/hooks/model_based_tag_rate_limits_hook.py @@ -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, ) diff --git a/litellm/types/router.py b/litellm/types/router.py index 8cf676f1a04..ab42a7966ec 100644 --- a/litellm/types/router.py +++ b/litellm/types/router.py @@ -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, ...] = () diff --git a/tests/test_litellm/proxy/hooks/test_model_based_tag_rate_limits_hook.py b/tests/test_litellm/proxy/hooks/test_model_based_tag_rate_limits_hook.py index 996b56f706e..f2e23fc9b00 100644 --- a/tests/test_litellm/proxy/hooks/test_model_based_tag_rate_limits_hook.py +++ b/tests/test_litellm/proxy/hooks/test_model_based_tag_rate_limits_hook.py @@ -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