From 7633035cf103e7a4cadc9d313aa71baa9e8bafe3 Mon Sep 17 00:00:00 2001 From: Deepanshu Date: Tue, 25 Aug 2026 09:24:37 -0400 Subject: [PATCH] fix(rate-limiting): drop redundant included/excluded_values, fix success-event metadata resolution TagRateLimitEntry no longer has included_values/excluded_values: enabled_for or disabled_for targeting the entry's own tag_id is functionally identical, so the extra fields only added surface area. Types, dedup/fingerprint signatures, hook logic, tests, and the generated dashboard schema are updated accordingly. Live proxy verification of the remaining scoping fields surfaced a real bug in both hooks' async_log_success_event: get_metadata_variable_name_from_kwargs only checks whether a "litellm_metadata" key is present on kwargs["litellm_params"], not whether it holds anything. For a plain chat completion, that key is always present (set to None) alongside the real, populated "metadata" dict, so token/dollar accounting silently read no tags and no identity, letting per-tag token and dollar limits go unenforced. Both hooks now resolve the authoritative field by checking it actually holds a populated dict, matching the value-truthiness check litellm_logging.py's own tag resolution already uses, instead of relying on key presence alone. --- .../hooks/global_tag_rate_limits_hook.py | 11 +- .../hooks/model_based_tag_rate_limits_hook.py | 103 +++---- litellm/types/router.py | 27 +- .../hooks/test_global_tag_rate_limits_hook.py | 47 +++ .../test_model_based_tag_rate_limits_hook.py | 286 +++++++++++------- ui/litellm-dashboard/src/lib/http/schema.d.ts | 4 - 6 files changed, 287 insertions(+), 191 deletions(-) diff --git a/litellm/proxy/hooks/global_tag_rate_limits_hook.py b/litellm/proxy/hooks/global_tag_rate_limits_hook.py index 29a0939151e..66cdd51b609 100644 --- a/litellm/proxy/hooks/global_tag_rate_limits_hook.py +++ b/litellm/proxy/hooks/global_tag_rate_limits_hook.py @@ -77,6 +77,7 @@ from litellm.proxy.hooks.model_based_tag_rate_limits_hook import ( _PartitionKey, # pyright: ignore[reportPrivateUsage] # reused across module boundaries, see module docstring _PartitionOperations, # pyright: ignore[reportPrivateUsage] # reused across module boundaries, see module docstring _policy_fingerprint, # pyright: ignore[reportPrivateUsage] # reused across module boundaries, see module docstring + _resolve_success_event_metadata_variable_name, # pyright: ignore[reportPrivateUsage] # reused across module boundaries, see module docstring ) from litellm.proxy.hooks.parallel_request_limiter_v3 import ( _PROXY_MaxParallelRequestsHandler_v3, # pyright: ignore[reportPrivateUsage] # reused across module boundaries, matching model_based_tag_rate_limits_hook's identical import @@ -347,7 +348,7 @@ class _PROXY_GlobalTagRateLimitsHook( # pyright: ignore[reportUnusedClass] # o tag_value = _extract_identity(tags, entry.tag_id) if tag_value is None: continue - if not _entry_applies(entry, tag_value, tags, key_alias): + if not _entry_applies(entry, tags, key_alias): continue effective_key_hash = key_hash if entry.scope_by_key_hash else None if unit == "concurrency": @@ -545,8 +546,12 @@ class _PROXY_GlobalTagRateLimitsHook( # pyright: ignore[reportUnusedClass] # o if standard_logging_object is None: return + # kwargs here is Logging.model_call_details, not the router's flat + # request kwargs admission sees: metadata/litellm_metadata are never + # top-level here, only nested under kwargs["litellm_params"] (see + # Logging.update_environment_variables). litellm_params_for_metadata: Final = kwargs.get("litellm_params") or kwargs - metadata_variable_name: Final = get_metadata_variable_name_from_kwargs(litellm_params_for_metadata) + metadata_variable_name: Final = _resolve_success_event_metadata_variable_name(litellm_params_for_metadata) key_hash: Final = _extract_key_hash(litellm_params_for_metadata, metadata_variable_name) key_alias: Final = _extract_key_alias(litellm_params_for_metadata, metadata_variable_name) @@ -575,7 +580,7 @@ class _PROXY_GlobalTagRateLimitsHook( # pyright: ignore[reportUnusedClass] # o tag_value = _extract_identity(tags, entry.tag_id) if tag_value is None: continue - if not _entry_applies(entry, tag_value, tags, key_alias): + if not _entry_applies(entry, tags, key_alias): continue increment_value = increment_by_unit[unit] if increment_value == 0: 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 f18f4a82d66..44c62b004b6 100644 --- a/litellm/proxy/hooks/model_based_tag_rate_limits_hook.py +++ b/litellm/proxy/hooks/model_based_tag_rate_limits_hook.py @@ -46,22 +46,20 @@ _LIMIT_UNITS: Final[tuple[_LimitUnit, ...]] = ("tokens", "requests", "dollars", # to fold `enabled_for`/`disabled_for` into `_DedupSignature` below without # depending on TagRateLimitScope's own hashability. _ScopeSignature: TypeAlias = tuple[str, tuple[str, ...]] | None -# (tag_id, name, limit, period_seconds, scope_by_key_hash, included_values, -# excluded_values, enabled_for, disabled_for, apply_to_key_alias) -- the -# fields that decide whether two deployments' entries are the same rate -# limit for dedup purposes; see _build_group_limits. Two deployments that -# agree on the first five but disagree on any scoping field are declaring -# genuinely different policies (e.g. one excludes a user the other doesn't) -# and must not be merged into one shared bucket -- the same class of bug -# this signature already guards against for a plain divergent `limit`. +# (tag_id, name, limit, period_seconds, scope_by_key_hash, enabled_for, +# disabled_for, apply_to_key_alias) -- the fields that decide whether two +# deployments' entries are the same rate limit for dedup purposes; see +# _build_group_limits. Two deployments that agree on the first five but +# disagree on any scoping field are declaring genuinely different policies +# (e.g. one excludes a user the other doesn't) and must not be merged into +# one shared bucket -- the same class of bug this signature already guards +# against for a plain divergent `limit`. _DedupSignature: TypeAlias = tuple[ str, str, float, int, bool, - tuple[str, ...] | None, - tuple[str, ...] | None, _ScopeSignature, _ScopeSignature, tuple[str, ...] | None, @@ -213,23 +211,23 @@ def _scope_signature(scope: TagRateLimitScope | None) -> _ScopeSignature: return None if scope is None else (scope.tag_id, scope.values) -def _entry_applies(entry: TagRateLimitEntry, tag_value: str, tags: Sequence[str], key_alias: str | None) -> bool: +def _entry_applies(entry: TagRateLimitEntry, tags: Sequence[str], key_alias: str | None) -> bool: """ - Applies `entry`'s own scoping fields (`included_values`/`excluded_values`/ - `enabled_for`/`disabled_for`/`apply_to_key_alias`), evaluated in this - order -- deny overrides allow, checked before either allowlist: + Applies `entry`'s own scoping fields (`enabled_for`/`disabled_for`/ + `apply_to_key_alias`), evaluated in this order -- deny overrides allow, + checked before either allowlist: - 1. `excluded_values`: `tag_value` is in it -> doesn't apply. - 2. `included_values`: `tag_value` is NOT in it -> doesn't apply. - 3. `disabled_for`: the gate tag (a tag OTHER than `entry.tag_id`, - resolved via `disabled_for.tag_id`) is present and its value is in - `disabled_for.values` -> doesn't apply. Absent gate tag never - triggers this -- nothing to match against a denylist. - 4. `enabled_for`: the gate tag is absent, or present but its value is + 1. `disabled_for`: the gate tag (often a SECOND, independent tag, but + `disabled_for.tag_id` can equally be set to this entry's own + `tag_id` to gate on a subset of its own resolved identity) is + present and its value is in `disabled_for.values` -> doesn't apply. + Absent gate tag never triggers this -- nothing to match against a + denylist. + 2. `enabled_for`: the gate tag is absent, or present but its value is NOT in `enabled_for.values` -> doesn't apply. Unlike `disabled_for`, absence DOES fail this check -- an allowlist gate requires an explicit match, so "not tagged at all" means "not in scope". - 5. `apply_to_key_alias`: the calling key's own alias is absent, or + 3. `apply_to_key_alias`: the calling key's own alias is absent, or present but not in the list -> doesn't apply. Same allowlist semantics as `enabled_for` -- a key with no alias set never satisfies this gate. @@ -237,10 +235,6 @@ def _entry_applies(entry: TagRateLimitEntry, tag_value: str, tags: Sequence[str] An entry with none of these fields set always applies -- this is the unscoped behavior every existing entry has today, unchanged. """ - if entry.excluded_values is not None and tag_value in entry.excluded_values: - return False - if entry.included_values is not None and tag_value not in entry.included_values: - return False if entry.disabled_for is not None: disabled_gate_value: Final = _extract_identity(tags, entry.disabled_for.tag_id) if disabled_gate_value is not None and disabled_gate_value in entry.disabled_for.values: @@ -258,6 +252,26 @@ def _deployment_id(deployment: Mapping[str, object]) -> str | None: return (deployment.get("model_info") or _EMPTY_MAPPING).get("id") +def _resolve_success_event_metadata_variable_name( + litellm_params_for_metadata: Mapping[str, object], +) -> Literal["metadata", "litellm_metadata"]: + """`get_metadata_variable_name_from_kwargs` only checks key presence, which + misresolves at `async_log_success_event` time: `kwargs["litellm_params"]` + always carries a `litellm_metadata` key (typically `None`) alongside the + real, populated `metadata` dict for a standard (non + LITELLM_METADATA_ROUTES) request, so the key-presence check always picks + `litellm_metadata` there and silently reads no tags/identity at all. + Requiring the value to actually be a populated dict, matching + `_get_request_tags`'s own truthiness check in litellm_logging.py, only + ever prefers `litellm_metadata` when it is genuinely the field the proxy + wrote identity/tags into (LITELLM_METADATA_ROUTES pre-seed it before + admission runs, so it is always a populated dict by success time there).""" + litellm_metadata: Final = litellm_params_for_metadata.get("litellm_metadata") + if isinstance(litellm_metadata, Mapping) and litellm_metadata: + return "litellm_metadata" + return "metadata" + + def _extract_team_id(request_kwargs: Mapping[str, object], metadata_variable_name: str) -> str | None: """Reads `user_api_key_team_id` from only the one field `get_metadata_variable_name_from_kwargs` names as authoritative for this @@ -378,8 +392,6 @@ def _build_group_limits(deployments: Sequence[Mapping[str, object]], unit: _Limi entry.limit, entry.period_seconds, entry.scope_by_key_hash, - entry.included_values, - entry.excluded_values, _scope_signature(entry.enabled_for), _scope_signature(entry.disabled_for), entry.apply_to_key_alias, @@ -494,8 +506,6 @@ 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.entry.apply_to_key_alias, @@ -690,20 +700,18 @@ def _fixed_length_identity(tag_value: str) -> str: def _policy_fingerprint(entry: TagRateLimitEntry) -> str: """ Two entries can share a `name` and `tag_id` while genuinely disagreeing - on `limit`, `period_seconds`, or any of the four scoping fields -- + on `limit`, `period_seconds`, or any of the scoping fields -- `_DedupSignature`/`resolve_any` already treat that as two distinct policies (see `distinct_signature_count_by_name` in `_build_group_limits`), so the Redis/in-memory bucket key must too, or two differently-configured entries that happen to share a name check and charge the identical counter. Hashed to a fixed-length digest for the same reason `_fixed_length_identity` hashes `tag_value`: an operator's own - `included_values`/`excluded_values` list has no length bound. + `enabled_for`/`disabled_for`/`apply_to_key_alias` list has no length bound. """ fingerprint_source: Final = ( entry.limit, entry.period_seconds, - entry.included_values, - entry.excluded_values, _scope_signature(entry.enabled_for), _scope_signature(entry.disabled_for), entry.apply_to_key_alias, @@ -782,7 +790,7 @@ def _classify_check( tag_value: Final = _extract_identity(tags, configured_limit.entry.tag_id) if tag_value is None: return None - if not _entry_applies(configured_limit.entry, tag_value, tags, key_alias): + if not _entry_applies(configured_limit.entry, tags, key_alias): return None key_hash: Final = ( _extract_key_hash(request_kwargs, metadata_variable_name) if configured_limit.entry.scope_by_key_hash else None @@ -821,7 +829,7 @@ def _increment_operation_for_limit( tag_value: Final = _extract_identity(tags, configured_limit.entry.tag_id) if tag_value is None: return None - if not _entry_applies(configured_limit.entry, tag_value, tags, key_alias): + if not _entry_applies(configured_limit.entry, tags, key_alias): return None if configured_limit.unit not in increment_by_unit: return None # "requests" is accounted atomically at admission, not here @@ -1476,23 +1484,8 @@ class _PROXY_ModelBasedTagRateLimitsHook( # pyright: ignore[reportUnusedClass] # top-level here, only nested under kwargs["litellm_params"] (see # Logging.update_environment_variables). litellm_params_for_metadata: Final = kwargs.get("litellm_params") or kwargs - metadata_variable_name: Final = get_metadata_variable_name_from_kwargs(litellm_params_for_metadata) - # standard_logging_object.metadata.user_api_key_team_id is built from - # whatever litellm_logging.py resolves as "the" metadata dict for the - # payload, not necessarily the same metadata_variable_name-authoritative - # field admission itself used -- reading straight from kwargs with the - # exact same helper admission calls (_extract_team_id) keeps this - # bucket identical to the one admission already scoped the check - # against, on every route regardless of which field is authoritative. + metadata_variable_name: Final = _resolve_success_event_metadata_variable_name(litellm_params_for_metadata) team_id: Final = _extract_team_id(litellm_params_for_metadata, metadata_variable_name) - # standard_logging_object.metadata.user_api_key_hash is only ever - # populated when the raw value happens to look like a SHA-256 hash - # (see litellm_logging.py's get_standard_logging_metadata), so it - # silently drops to None for any key whose hash doesn't pass that - # shape check even though admission's own _extract_key_hash reads - # the same field unconditionally -- reading straight from kwargs - # here instead keeps this bucket identical to the one admission - # already scoped the check against. key_hash: Final = _extract_key_hash(litellm_params_for_metadata, metadata_variable_name) key_alias: Final = _extract_key_alias(litellm_params_for_metadata, metadata_variable_name) # model_group is the caller-visible name, which Router deliberately @@ -1528,12 +1521,6 @@ class _PROXY_ModelBasedTagRateLimitsHook( # pyright: ignore[reportUnusedClass] if not configured: return - # Resolving the field name against kwargs itself always picks the - # "metadata" default, so on LITELLM_METADATA_ROUTES (/v1/messages, - # /responses, ...) this would read the caller's native, tag-less - # metadata instead of the real, server-computed litellm_metadata.tags - # admission already used -- metadata_variable_name above is already - # resolved against litellm_params_for_metadata to avoid that. tags: Final = _get_tags_from_request_kwargs(kwargs, metadata_variable_name=metadata_variable_name) if not tags: return diff --git a/litellm/types/router.py b/litellm/types/router.py index 7e0a617ef65..28f30e7e0ee 100644 --- a/litellm/types/router.py +++ b/litellm/types/router.py @@ -197,16 +197,11 @@ class TagRateLimitEntry(BaseModel): # evict another entry's active counters; setting this gives the entry # its own dedicated partition instead. max_in_memory_cache_size: int | None = None - # Scope this entry to a subset of its own resolved `tag_id` value -- - # e.g. hand-picking a handful of identities without needing a second - # tag at all. `excluded_values` is checked before `included_values` - # (deny overrides allow) when both happen to be set on the same entry. - included_values: tuple[str, ...] | None = None - excluded_values: tuple[str, ...] | None = None - # Gate this entry on a SECOND, independent tag rather than its own - # `tag_id` -- e.g. `enabled_for: {tag_id: company_id, values: ["1032"]}` - # to scope an override to one company's traffic without enumerating - # every one of that company's end_user_id values by hand. + # Gate this entry on a tag -- often a SECOND, independent tag (e.g. + # `enabled_for: {tag_id: company_id, values: ["1032"]}` to scope an + # override to one company's traffic), but `tag_id` can equally be set to + # this same entry's own `tag_id` to scope by a subset of its own + # resolved identity instead, without a second tag at all. # `disabled_for` is checked first (deny overrides allow) when both are # set. An absent gate tag never satisfies `enabled_for` (an allowlist # gate requires an explicit match) but never triggers `disabled_for` @@ -271,26 +266,18 @@ class TagRateLimitEntry(BaseModel): return self @model_validator(mode="after") - def _validate_included_and_excluded_values(self) -> "TagRateLimitEntry": - if self.included_values is not None and not self.included_values: - raise ValueError("included_values must be a non-empty list of strings when set") - if self.excluded_values is not None and not self.excluded_values: - raise ValueError("excluded_values must be a non-empty list of strings when set") + def _validate_apply_to_key_alias(self) -> "TagRateLimitEntry": if self.apply_to_key_alias is not None and not self.apply_to_key_alias: raise ValueError("apply_to_key_alias must be a non-empty list of strings when set") return self @model_validator(mode="after") - def _normalize_included_and_excluded_values(self) -> "TagRateLimitEntry": + def _normalize_apply_to_key_alias(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))) # mutable-ok: frozen before escaping - if self.excluded_values is not None: - self.excluded_values = tuple(sorted(set(self.excluded_values))) # mutable-ok: frozen before escaping if self.apply_to_key_alias is not None: self.apply_to_key_alias = tuple(sorted(set(self.apply_to_key_alias))) # mutable-ok: frozen before escaping return self diff --git a/tests/test_litellm/proxy/hooks/test_global_tag_rate_limits_hook.py b/tests/test_litellm/proxy/hooks/test_global_tag_rate_limits_hook.py index de8543159dc..0114ac7bc25 100644 --- a/tests/test_litellm/proxy/hooks/test_global_tag_rate_limits_hook.py +++ b/tests/test_litellm/proxy/hooks/test_global_tag_rate_limits_hook.py @@ -454,6 +454,53 @@ async def test_dollar_limit_accounts_usage_and_rejects_once_over(time_controller ) +@pytest.mark.asyncio +async def test_log_success_event_accounts_when_litellm_params_carries_a_null_litellm_metadata_key( + time_controller, monkeypatch +): + """ + kwargs at async_log_success_event time is Logging.model_call_details, not + the flat dict admission sees -- for a plain (non LITELLM_METADATA_ROUTES) + chat completion, kwargs["litellm_params"] carries a "litellm_metadata" key + that is always present but set to None, alongside the real, populated + "metadata" dict. get_metadata_variable_name_from_kwargs only checks key + presence, so it always resolved to "litellm_metadata" here and read no + tags/identity at all, silently dropping every token/dollar/key-hash/alias + accounting for this route shape. + """ + monkeypatch.setattr( + litellm, + "global_tag_rate_limits", + { + "dollar_limits": { + "limits": [{"name": "daily_spend", "tag_id": "end_user_id", "limit": 10.0, "period_seconds": 86400}] + } + }, + ) + hook = _make_hook(time_controller) + + data = _data(["end_user_id:u1"], call_id="call-1") + await hook.async_pre_call_hook(user_api_key_dict=_key(), cache=DualCache(), data=data, call_type="completion") + kwargs = { + "litellm_call_id": "call-1", + "litellm_params": { + "litellm_metadata": None, + "metadata": {"tags": ["end_user_id:u1"], "user_api_key": "hash"}, + }, + "standard_logging_object": {"total_tokens": 0, "response_cost": 12.0}, + } + await hook.async_log_success_event(kwargs=kwargs, response_obj=None, start_time=0, end_time=0) + await asyncio.sleep(0) + + with pytest.raises(ProxyRateLimitError): + await hook.async_pre_call_hook( + user_api_key_dict=_key(), + cache=DualCache(), + data=_data(["end_user_id:u1"], call_id="call-2"), + call_type="completion", + ) + + @pytest.mark.asyncio async def test_dollar_limit_respects_apply_to_key_alias_at_accounting_time(time_controller, monkeypatch): """The entry only applies to `premium-key`; a non-listed key's spend must 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 4f68ecae819..9bde510972f 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 @@ -18,7 +18,9 @@ import litellm from litellm.caching.dual_cache import DualCache from litellm.proxy.common_utils.proxy_rate_limit_error import ProxyRateLimitError from litellm.proxy.hooks.model_based_tag_rate_limits_hook import ( + _BACKGROUND_TASKS, _CONCURRENCY_MIN_SAFETY_TTL_SECONDS, + _PENDING_CONCURRENCY_KEYS_FIELD, _bucket_key, _bucket_ttl_seconds, _build_group_limits, @@ -29,12 +31,9 @@ from litellm.proxy.hooks.model_based_tag_rate_limits_hook import ( _extract_key_hash, _extract_team_id, _fixed_length_identity, - _BACKGROUND_TASKS, _inflight_key, _partition_key, - _PENDING_CONCURRENCY_KEYS_FIELD, _PROXY_ModelBasedTagRateLimitsHook, - _queue_pending_concurrency_reservations, ) from litellm.types.router import RoutingGroup, TagRateLimitEntry, TagRateLimitScope @@ -105,8 +104,6 @@ def _expected_bucket_key( resolved_group: str | None = None, key_hash: str | None = None, limit: float = 1, - included_values: tuple | None = None, - excluded_values: tuple | None = None, enabled_for: dict | None = None, disabled_for: dict | None = None, ) -> str: @@ -116,7 +113,7 @@ def _expected_bucket_key( tag value into a literal string -- the internal key format (hashed or not) is an implementation detail these tests shouldn't hardcode. - `limit` and the four scoping fields default to values that produce a + `limit` and the scoping fields default to values that produce a stable fingerprint for tests that don't care about it, but must be passed matching the real entry's own configuration whenever a test's router declares a `limit` other than 1 (or any scoping) for the entry @@ -131,8 +128,6 @@ def _expected_bucket_key( tag_id=tag_id, limit=limit, period_seconds=period_seconds, - included_values=included_values, - excluded_values=excluded_values, enabled_for=enabled_for, disabled_for=disabled_for, ), @@ -384,41 +379,41 @@ def test_build_group_limits_empty_when_no_deployment_configures_unit(): # --------------------------------------------------------------------------- -# _entry_applies -- included_values / excluded_values / enabled_for / disabled_for +# _entry_applies -- enabled_for / disabled_for / apply_to_key_alias # --------------------------------------------------------------------------- -def test_entry_applies_with_none_of_the_four_fields_set(): +def test_entry_applies_with_none_of_the_scoping_fields_set(): entry = TagRateLimitEntry(name="daily", tag_id="end_user_id", limit=500, period_seconds=86400) - assert _entry_applies(entry, "u1", ["end_user_id:u1"], None) is True + assert _entry_applies(entry, ["end_user_id:u1"], None) is True -def test_entry_applies_excludes_a_listed_value(): +def test_entry_applies_disabled_for_on_its_own_tag_id_excludes_a_listed_value(): + """disabled_for's `tag_id` can be set to the entry's own tag_id, gating on + a subset of its own resolved identity rather than a second tag.""" entry = TagRateLimitEntry( - name="daily", tag_id="end_user_id", limit=500, period_seconds=86400, excluded_values=("u1",) + name="daily", + tag_id="end_user_id", + limit=500, + period_seconds=86400, + disabled_for=TagRateLimitScope(tag_id="end_user_id", values=("u1",)), ) - assert _entry_applies(entry, "u1", ["end_user_id:u1"], None) is False + assert _entry_applies(entry, ["end_user_id:u1"], None) is False + assert _entry_applies(entry, ["end_user_id:u2"], None) is True -def test_entry_applies_admits_a_value_not_on_the_exclusion_list(): +def test_entry_applies_enabled_for_on_its_own_tag_id_restricts_to_a_listed_value(): + """enabled_for's `tag_id` can likewise be set to the entry's own tag_id, + admitting only a hand-picked subset of its own resolved identity.""" entry = TagRateLimitEntry( - name="daily", tag_id="end_user_id", limit=500, period_seconds=86400, excluded_values=("u1",) + name="daily", + tag_id="end_user_id", + limit=500, + period_seconds=86400, + enabled_for=TagRateLimitScope(tag_id="end_user_id", values=("u2", "u3")), ) - assert _entry_applies(entry, "u2", ["end_user_id:u2"], None) is True - - -def test_entry_applies_rejects_a_value_missing_from_the_inclusion_list(): - entry = TagRateLimitEntry( - name="daily", tag_id="end_user_id", limit=500, period_seconds=86400, included_values=("u2", "u3") - ) - assert _entry_applies(entry, "u1", ["end_user_id:u1"], None) is False - - -def test_entry_applies_admits_a_value_on_the_inclusion_list(): - entry = TagRateLimitEntry( - name="daily", tag_id="end_user_id", limit=500, period_seconds=86400, included_values=("u2", "u3") - ) - assert _entry_applies(entry, "u2", ["end_user_id:u2", "company_id:1032"], None) is True + assert _entry_applies(entry, ["end_user_id:u1"], None) is False + assert _entry_applies(entry, ["end_user_id:u2"], None) is True def test_entry_applies_matches_an_enabled_for_gate(): @@ -429,7 +424,7 @@ def test_entry_applies_matches_an_enabled_for_gate(): period_seconds=86400, enabled_for=TagRateLimitScope(tag_id="company_id", values=("1032",)), ) - assert _entry_applies(entry, "u1", ["end_user_id:u1", "company_id:1032"], None) is True + assert _entry_applies(entry, ["end_user_id:u1", "company_id:1032"], None) is True def test_entry_applies_skips_when_enabled_for_gate_tag_is_absent(): @@ -444,7 +439,7 @@ def test_entry_applies_skips_when_enabled_for_gate_tag_is_absent(): period_seconds=86400, enabled_for=TagRateLimitScope(tag_id="company_id", values=("1032",)), ) - assert _entry_applies(entry, "u1", ["end_user_id:u1"], None) is False + assert _entry_applies(entry, ["end_user_id:u1"], None) is False def test_entry_applies_skips_when_disabled_for_gate_matches(): @@ -455,7 +450,7 @@ def test_entry_applies_skips_when_disabled_for_gate_matches(): period_seconds=86400, disabled_for=TagRateLimitScope(tag_id="company_id", values=("1032",)), ) - assert _entry_applies(entry, "u1", ["end_user_id:u1", "company_id:1032"], None) is False + assert _entry_applies(entry, ["end_user_id:u1", "company_id:1032"], None) is False def test_entry_applies_when_disabled_for_gate_tag_is_absent(): @@ -468,41 +463,41 @@ def test_entry_applies_when_disabled_for_gate_tag_is_absent(): period_seconds=86400, disabled_for=TagRateLimitScope(tag_id="company_id", values=("1032",)), ) - assert _entry_applies(entry, "u1", ["end_user_id:u1"], None) is True + assert _entry_applies(entry, ["end_user_id:u1"], None) is True -def test_entry_applies_excluded_values_overrides_a_matching_enabled_for_gate(): - """Deny (identity-level excluded_values) takes effect independently of - whether the enabled_for gate itself matched.""" +def test_entry_applies_disabled_for_overrides_a_matching_enabled_for_gate(): + """Deny (disabled_for) takes effect independently of whether the + enabled_for gate itself matched, even when both target the same tag.""" entry = TagRateLimitEntry( name="daily", tag_id="end_user_id", limit=500, period_seconds=86400, enabled_for=TagRateLimitScope(tag_id="company_id", values=("1032",)), - excluded_values=("u1",), + disabled_for=TagRateLimitScope(tag_id="end_user_id", values=("u1",)), ) - assert _entry_applies(entry, "u1", ["end_user_id:u1", "company_id:1032"], None) is False + assert _entry_applies(entry, ["end_user_id:u1", "company_id:1032"], None) is False def test_entry_applies_with_apply_to_key_alias_unset_applies_to_every_key(): entry = TagRateLimitEntry(name="daily", tag_id="end_user_id", limit=500, period_seconds=86400) - assert _entry_applies(entry, "u1", ["end_user_id:u1"], "any-key-alias") is True - assert _entry_applies(entry, "u1", ["end_user_id:u1"], None) is True + assert _entry_applies(entry, ["end_user_id:u1"], "any-key-alias") is True + assert _entry_applies(entry, ["end_user_id:u1"], None) is True def test_entry_applies_admits_a_key_alias_on_the_allowlist(): entry = TagRateLimitEntry( name="daily", tag_id="end_user_id", limit=500, period_seconds=86400, apply_to_key_alias=("team-a-key",) ) - assert _entry_applies(entry, "u1", ["end_user_id:u1"], "team-a-key") is True + assert _entry_applies(entry, ["end_user_id:u1"], "team-a-key") is True def test_entry_applies_rejects_a_key_alias_missing_from_the_allowlist(): entry = TagRateLimitEntry( name="daily", tag_id="end_user_id", limit=500, period_seconds=86400, apply_to_key_alias=("team-a-key",) ) - assert _entry_applies(entry, "u1", ["end_user_id:u1"], "team-b-key") is False + assert _entry_applies(entry, ["end_user_id:u1"], "team-b-key") is False def test_entry_applies_rejects_when_key_has_no_alias_but_allowlist_is_set(): @@ -511,7 +506,7 @@ def test_entry_applies_rejects_when_key_has_no_alias_but_allowlist_is_set(): entry = TagRateLimitEntry( name="daily", tag_id="end_user_id", limit=500, period_seconds=86400, apply_to_key_alias=("team-a-key",) ) - assert _entry_applies(entry, "u1", ["end_user_id:u1"], None) is False + assert _entry_applies(entry, ["end_user_id:u1"], None) is False # --------------------------------------------------------------------------- @@ -519,16 +514,6 @@ def test_entry_applies_rejects_when_key_has_no_alias_but_allowlist_is_set(): # --------------------------------------------------------------------------- -def test_tag_rate_limit_entry_rejects_empty_included_values(): - with pytest.raises(ValidationError, match="included_values must be a non-empty list"): - TagRateLimitEntry(name="daily", limit=1, period_seconds=60, included_values=()) - - -def test_tag_rate_limit_entry_rejects_empty_excluded_values(): - with pytest.raises(ValidationError, match="excluded_values must be a non-empty list"): - TagRateLimitEntry(name="daily", limit=1, period_seconds=60, excluded_values=()) - - def test_tag_rate_limit_scope_rejects_empty_values(): with pytest.raises(ValidationError, match="values must be a non-empty list"): TagRateLimitScope(tag_id="company_id", values=()) @@ -539,25 +524,6 @@ 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") @@ -609,10 +575,26 @@ def test_bucket_key_differs_for_same_named_entries_with_different_limits(): def test_bucket_key_differs_for_same_named_entries_with_different_scoping_only(): now = 0.0 excluding_u1 = _expected_bucket_key( - "grp", "requests", "daily", "end_user_id", "u2", 86400, now, limit=100, excluded_values=("u1",) + "grp", + "requests", + "daily", + "end_user_id", + "u2", + 86400, + now, + limit=100, + disabled_for={"tag_id": "end_user_id", "values": ["u1"]}, ) excluding_u2 = _expected_bucket_key( - "grp", "requests", "daily", "end_user_id", "u2", 86400, now, limit=100, excluded_values=("u2",) + "grp", + "requests", + "daily", + "end_user_id", + "u2", + 86400, + now, + limit=100, + disabled_for={"tag_id": "end_user_id", "values": ["u2"]}, ) assert excluding_u1 != excluding_u2 @@ -622,10 +604,10 @@ def test_bucket_key_differs_for_same_named_entries_with_different_scoping_only() # --------------------------------------------------------------------------- -def test_build_group_limits_per_deployment_when_excluded_values_diverge(): +def test_build_group_limits_per_deployment_when_disabled_for_diverges(): """ Regression test: two deployments agreeing on tag_id/limit/period_seconds - but declaring different excluded_values are genuinely different + but declaring different disabled_for scopes are genuinely different policies and must not be silently merged into one shared bucket -- the same class of bug test_build_group_limits_per_deployment_when_values_diverge already guards against for a plain divergent limit value. @@ -637,7 +619,12 @@ def test_build_group_limits_per_deployment_when_excluded_values_diverge(): { "token_limits": { "limits": [ - {"name": "daily", "limit": 500, "period_seconds": 86400, "excluded_values": ["u1"]} + { + "name": "daily", + "limit": 500, + "period_seconds": 86400, + "disabled_for": {"tag_id": "end_user_id", "values": ["u1"]}, + } ] } }, @@ -648,7 +635,12 @@ def test_build_group_limits_per_deployment_when_excluded_values_diverge(): { "token_limits": { "limits": [ - {"name": "daily", "limit": 500, "period_seconds": 86400, "excluded_values": ["u2"]} + { + "name": "daily", + "limit": 500, + "period_seconds": 86400, + "disabled_for": {"tag_id": "end_user_id", "values": ["u2"]}, + } ] } }, @@ -660,7 +652,7 @@ def test_build_group_limits_per_deployment_when_excluded_values_diverge(): assert scopes == {("dep-1",), ("dep-2",)} -def test_build_group_limits_chain_wide_when_excluded_values_agree(): +def test_build_group_limits_chain_wide_when_disabled_for_agrees(): deployments = [ _deployment( "grp", @@ -668,7 +660,12 @@ def test_build_group_limits_chain_wide_when_excluded_values_agree(): { "token_limits": { "limits": [ - {"name": "daily", "limit": 500, "period_seconds": 86400, "excluded_values": ["u1"]} + { + "name": "daily", + "limit": 500, + "period_seconds": 86400, + "disabled_for": {"tag_id": "end_user_id", "values": ["u1"]}, + } ] } }, @@ -679,7 +676,12 @@ def test_build_group_limits_chain_wide_when_excluded_values_agree(): { "token_limits": { "limits": [ - {"name": "daily", "limit": 500, "period_seconds": 86400, "excluded_values": ["u1"]} + { + "name": "daily", + "limit": 500, + "period_seconds": 86400, + "disabled_for": {"tag_id": "end_user_id", "values": ["u1"]}, + } ] } }, @@ -690,13 +692,13 @@ 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(): +def test_build_group_limits_chain_wide_when_disabled_for_agrees_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. + Two deployments declaring the identical disabled_for values set, just in + a different config order, must dedup to one chain-wide entry -- config + order is not a policy difference. Relies on TagRateLimitScope's own + normalization (sorting) of values at construction time, not on this + dedup path re-sorting them itself. """ deployments = [ _deployment( @@ -705,7 +707,12 @@ def test_build_group_limits_chain_wide_when_excluded_values_agree_in_different_o { "token_limits": { "limits": [ - {"name": "daily", "limit": 500, "period_seconds": 86400, "excluded_values": ["u1", "u2"]} + { + "name": "daily", + "limit": 500, + "period_seconds": 86400, + "disabled_for": {"tag_id": "end_user_id", "values": ["u1", "u2"]}, + } ] } }, @@ -716,7 +723,12 @@ def test_build_group_limits_chain_wide_when_excluded_values_agree_in_different_o { "token_limits": { "limits": [ - {"name": "daily", "limit": 500, "period_seconds": 86400, "excluded_values": ["u2", "u1"]} + { + "name": "daily", + "limit": 500, + "period_seconds": 86400, + "disabled_for": {"tag_id": "end_user_id", "values": ["u2", "u1"]}, + } ] } }, @@ -976,28 +988,40 @@ 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(): +def test_resolve_any_keeps_divergent_disabled_for_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 + resolve_any's own dedup key omitted enabled_for/disabled_for/ + apply_to_key_alias, so two routing-group members agreeing on + tag_id/limit/period_seconds but declaring different disabled_for scopes 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 + class of bug test_build_group_limits_per_deployment_when_disabled_for_diverges 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"]} + { + "name": "inflight", + "tag_id": "end_user_id", + "limit": 1, + "period_seconds": 300, + "disabled_for": {"tag_id": "end_user_id", "values": ["u1"]}, + } ] } } concurrency_limits_excluding_u2 = { "concurrency_limits": { "limits": [ - {"name": "inflight", "tag_id": "end_user_id", "limit": 1, "period_seconds": 300, "excluded_values": ["u2"]} + { + "name": "inflight", + "tag_id": "end_user_id", + "limit": 1, + "period_seconds": 300, + "disabled_for": {"tag_id": "end_user_id", "values": ["u2"]}, + } ] } } @@ -1009,7 +1033,7 @@ def test_resolve_any_keeps_divergent_excluded_values_across_member_model_names_s ) 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",)} + assert {c.entry.disabled_for.values for c in resolved} == {("u1",), ("u2",)} def test_resolve_any_picks_the_same_resolved_group_regardless_of_hash_seed(): @@ -1118,7 +1142,7 @@ def _company_tiered_cap_router(default_limit: int, override_limit: int) -> "lite "limit": override_limit, "period_seconds": 86400, "enabled_for": {"tag_id": "company_id", "values": ["1032"]}, - "excluded_values": ["u1"], + "disabled_for": {"tag_id": "end_user_id", "values": ["u1"]}, }, ] } @@ -1133,9 +1157,9 @@ async def test_filter_deployments_scoped_override_skips_for_an_excluded_identity """ Company-tiered-cap example from the plan: a stricter override entry gated to one company via enabled_for, with a handful of named users - excluded from it via excluded_values. An excluded user must fall - through to the unscoped default entry entirely -- the override never - enforces or accounts for them. + excluded from it via disabled_for on the entry's own tag_id. An excluded + user must fall through to the unscoped default entry entirely -- the + override never enforces or accounts for them. """ limiter = _make_limiter(time_controller) router = _company_tiered_cap_router(default_limit=3, override_limit=1) @@ -1165,9 +1189,9 @@ async def test_filter_deployments_scoped_override_skips_for_an_excluded_identity async def test_filter_deployments_scoped_override_enforces_for_a_non_excluded_identity_in_scope(time_controller): """ The same override applies, and enforces its own stricter limit, for a - company-1032 user who is not on excluded_values, proving the two - entries are independently enforced rather than one silently replacing - the other. + company-1032 user who is not disabled_for's excluded identity, proving + the two entries are independently enforced rather than one silently + replacing the other. """ limiter = _make_limiter(time_controller) router = _company_tiered_cap_router(default_limit=3, override_limit=1) @@ -1398,6 +1422,56 @@ async def test_log_success_event_increments_configured_units(time_controller): assert await limiter.internal_usage_cache.async_get_cache(key=request_key, litellm_parent_otel_span=None) is None +@pytest.mark.asyncio +async def test_log_success_event_accounts_when_litellm_params_carries_a_null_litellm_metadata_key(time_controller): + """ + kwargs at async_log_success_event time is Logging.model_call_details, not + the flat dict admission sees -- for a plain (non LITELLM_METADATA_ROUTES) + chat completion, kwargs["litellm_params"] carries a "litellm_metadata" key + that is always present but set to None, alongside the real, populated + "metadata" dict. get_metadata_variable_name_from_kwargs only checks key + presence, so it always resolved to "litellm_metadata" here and read no + tags/identity at all, silently dropping every token/dollar accounting for + this route shape. + """ + limiter = _make_limiter(time_controller) + router = litellm.Router( + model_list=[ + _deployment( + "grp", + "dep-1", + { + "token_limits": { + "limits": [{"name": "daily", "tag_id": "end_user_id", "limit": 500000, "period_seconds": 86400}] + } + }, + ) + ] + ) + limiter.update_variables(llm_router=router) + + kwargs = { + "litellm_params": { + "litellm_metadata": None, + "metadata": {"tags": ["end_user_id:u1"]}, + }, + "standard_logging_object": { + "model_group": "grp", + "model_id": "dep-1", + "total_tokens": 42, + "response_cost": 0.01, + }, + } + await limiter.async_log_success_event(kwargs=kwargs, response_obj=None, start_time=0, end_time=0) + await asyncio.sleep(0) + + now = time_controller.now().timestamp() + token_key = _expected_bucket_key("grp", "tokens", "daily", "end_user_id", "u1", 86400, now, limit=500000) + assert ( + float(await limiter.internal_usage_cache.async_get_cache(key=token_key, litellm_parent_otel_span=None)) == 42.0 + ) + + @pytest.mark.asyncio async def test_log_success_event_reads_nested_litellm_metadata_when_that_is_authoritative(time_controller): """ diff --git a/ui/litellm-dashboard/src/lib/http/schema.d.ts b/ui/litellm-dashboard/src/lib/http/schema.d.ts index 5e55f6fd018..a6549da1543 100644 --- a/ui/litellm-dashboard/src/lib/http/schema.d.ts +++ b/ui/litellm-dashboard/src/lib/http/schema.d.ts @@ -35116,10 +35116,6 @@ export interface components { apply_to_key_alias?: string[] | null; disabled_for?: components["schemas"]["TagRateLimitScope"] | null; enabled_for?: components["schemas"]["TagRateLimitScope"] | null; - /** Excluded Values */ - excluded_values?: string[] | null; - /** Included Values */ - included_values?: string[] | null; /** Key Ttl Seconds */ key_ttl_seconds?: number | null; /** Limit */