From 993adb0c9cae58f905d16fc57d3b55b935feaacf Mon Sep 17 00:00:00 2001 From: Deepanshu Date: Tue, 25 Aug 2026 11:37:06 -0400 Subject: [PATCH] fix(rate-limiting): fold scope_by_key_hash into the bucket-key fingerprint _policy_fingerprint hashed limit/period_seconds/enabled_for/disabled_for/ apply_to_key_alias but not scope_by_key_hash, even though _DedupSignature already treats it as a distinct policy. _hash_tag's key_hash-derived suffix is empty whenever key_hash resolves to None, so a key-hash-scoped entry and an otherwise-identical unscoped entry collided onto the same counter for any call with no resolvable virtual key. --- .../hooks/model_based_tag_rate_limits_hook.py | 18 ++++++++---- .../test_model_based_tag_rate_limits_hook.py | 28 +++++++++++++++++-- 2 files changed, 38 insertions(+), 8 deletions(-) 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 44c62b004b6..2026706f6e3 100644 --- a/litellm/proxy/hooks/model_based_tag_rate_limits_hook.py +++ b/litellm/proxy/hooks/model_based_tag_rate_limits_hook.py @@ -700,18 +700,24 @@ 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 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 + on `limit`, `period_seconds`, `scope_by_key_hash`, 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. `scope_by_key_hash` specifically needs its + own slot here rather than relying on `_hash_tag`'s `key_hash`-derived + suffix to carry it: that suffix is empty whenever `key_hash` resolves to + `None` (no virtual key on the call), which would otherwise collide an + unscoped entry with a key-hash-scoped one that agrees on every other + field. Hashed to a fixed-length digest for the same reason `_fixed_length_identity` hashes `tag_value`: an operator's own `enabled_for`/`disabled_for`/`apply_to_key_alias` list has no length bound. """ fingerprint_source: Final = ( entry.limit, entry.period_seconds, + entry.scope_by_key_hash, _scope_signature(entry.enabled_for), _scope_signature(entry.disabled_for), entry.apply_to_key_alias, 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 9bde510972f..95e751277cf 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 @@ -106,6 +106,7 @@ def _expected_bucket_key( limit: float = 1, enabled_for: dict | None = None, disabled_for: dict | None = None, + scope_by_key_hash: bool = False, ) -> str: """ Builds the exact key the real code would compute (via _hash_tag's @@ -130,6 +131,7 @@ def _expected_bucket_key( period_seconds=period_seconds, enabled_for=enabled_for, disabled_for=disabled_for, + scope_by_key_hash=scope_by_key_hash, ), deployment_scope=deployment_scope, team_scope=team_scope, @@ -599,6 +601,28 @@ def test_bucket_key_differs_for_same_named_entries_with_different_scoping_only() assert excluding_u1 != excluding_u2 +def test_bucket_key_differs_for_same_named_entries_diverging_only_on_scope_by_key_hash(): + """ + _DedupSignature already folds scope_by_key_hash into dedup (two + deployments declaring the same name/tag_id but different + scope_by_key_hash become two distinct _ConfiguredLimit entries, not one + merged one), but _policy_fingerprint didn't fold it into the bucket-key + hash. When a request's key_hash resolves to None -- e.g. no virtual key + on the call -- both entries' key_hash-derived suffix is empty too, so an + unscoped entry and a key-hash-scoped entry that otherwise share every + other field collided onto the identical counter, letting one entry's + admission or accounting silently corrupt the other's. + """ + now = 0.0 + unscoped = _expected_bucket_key( + "grp", "requests", "daily", "end_user_id", "u1", 86400, now, limit=100, scope_by_key_hash=False + ) + key_hash_scoped_but_no_key_present = _expected_bucket_key( + "grp", "requests", "daily", "end_user_id", "u1", 86400, now, limit=100, scope_by_key_hash=True, key_hash=None + ) + assert unscoped != key_hash_scoped_but_no_key_present + + # --------------------------------------------------------------------------- # _build_group_limits -- scoping fields fold into the dedup signature # --------------------------------------------------------------------------- @@ -1730,10 +1754,10 @@ async def test_log_success_event_accounts_against_the_key_hash_admission_checked now = time_controller.now().timestamp() keyed_bucket = _expected_bucket_key( - "grp", "tokens", "daily", "end_user_id", "u1", 86400, now, key_hash="keyA", limit=500000 + "grp", "tokens", "daily", "end_user_id", "u1", 86400, now, key_hash="keyA", limit=500000, scope_by_key_hash=True ) unkeyed_bucket = _expected_bucket_key( - "grp", "tokens", "daily", "end_user_id", "u1", 86400, now, key_hash=None, limit=500000 + "grp", "tokens", "daily", "end_user_id", "u1", 86400, now, key_hash=None, limit=500000, scope_by_key_hash=True ) assert ( float(await limiter.internal_usage_cache.async_get_cache(key=keyed_bucket, litellm_parent_otel_span=None))