fix(rate_limiter): enforce TPM pre-call in dynamic_rate_limiter_v3 priority pools#34592
Conversation
…iority pools Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Greptile SummaryThis PR adds read-only TPM checks to dynamic rate-limiter priority and model-wide descriptors.
Confidence Score: 3/5This PR should not merge until CHECK_ONLY ignores token usage from an expired window instead of potentially returning a false 429. The window and token keys have independent TTL timelines, while the new zero-increment branch deliberately reads the token counter even after the window expires, allowing completed usage from the prior window to block requests in the next one. Files Needing Attention: litellm/proxy/hooks/parallel_request_limiter_v3.py
|
| Filename | Overview |
|---|---|
| litellm/proxy/hooks/dynamic_rate_limiter_v3.py | Sends CHECK_ONLY for pre-call token enforcement while retaining post-call token accounting. |
| litellm/proxy/hooks/parallel_request_limiter_v3.py | Implements read-only atomic counters, but expired windows can still read prior-window token usage and falsely reject requests. |
| tests/test_litellm/proxy/hooks/test_dynamic_rate_limiter_v3.py | Adds coverage for priority-pool TPM rejection and non-mutating admission below the limit. |
| tests/test_litellm/proxy/hooks/test_parallel_request_limiter_v3.py | Covers CHECK_ONLY payload construction, boundary enforcement, and isolation from RPM checks, but not window/counter TTL skew. |
Reviews (1): Last reviewed commit: "fix(rate_limiter): enforce TPM pre-call ..." | Re-trigger Greptile
| local current_counter | ||
| if window_expired then | ||
| if window_expired and increment > 0 then | ||
| current_counter = 0 | ||
| else | ||
| current_counter = tonumber(redis.call('GET', counter_key) or 0) |
There was a problem hiding this comment.
This is deliberate, and resetting on window expiry is exactly what made the ticketed bug possible.
The window key is only ever written by counters that advance, and token counters don't: they're written post-call by async_log_success_event through async_increment_tokens_with_ttl_preservation, which touches the counter key alone. Treating a missing or rolled window as "usage is zero" therefore discards every token recorded so far, which is how a pool sitting at 100k tokens against a 180 TPM reservation sailed through unenforced.
Staleness is bounded by the counter's own TTL, which is window_size and is preserved rather than refreshed on increments, so a token counter cannot outlive one window. That is the same contract the key/team/user TPM path has always run on; is_cache_list_over_limit reads the raw counter value and never resets it against a window key either. So this branch matches the enforcement semantics already in production for every other TPM dimension, rather than inventing a new one.
There was a problem hiding this comment.
You're right, and I missed the asymmetry between who writes the window key and who writes the counter key.
My concern assumed both keys are always co-written, so a missing window → counter reset was safe. But for check-only (TPM) counters that's inverted: the counter is written post-call by async_log_success_event while the window key is only ever written by advancing (RPM-style) counters. Resetting current_counter = 0 on a missing window therefore discards live token usage, which is precisely the bug this PR fixes.
Relying on the counter's own TTL (window_size, preserved by async_increment_tokens_with_ttl_preservation) to bound staleness is the correct contract here, and it matches what is_cache_list_over_limit already does for key/team/user TPM. I'm satisfied with the approach.
| Span = Any | ||
| InternalUsageCache = Any | ||
|
|
||
| CHECK_ONLY: Final[Literal["check_only"]] = "check_only" |
There was a problem hiding this comment.
CHECK_ONLY is now imported by another hook but is defined inside this limiter implementation rather than constants.py, coupling consumers to the implementation module and bypassing the repository convention for shared sentinel values.
Rule Used: What: Require sentinel-like variables (e.g., `_NEG... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
TLDR
Problem this solves:
How it solves it:
CHECK_ONLYmarker enforces a counter without advancing itcurrent >= limitRelevant issues
Linear ticket
Resolves LIT-4800
Pre-Submission checklist
@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
Both runs used the same proxy config (a 100 TPM model group,
dynamic_rate_limiter_v3,priority_reservationset so the master key lands in the shared default pool at 50 TPM) and the same loop of six realgpt-4o-minicalls costing real $:Before, at
579f41d57f(base). Twelve calls, 180 tokens against a 100 TPM model group and a 50 TPM pool, nothing is ever refused:After, at
6735375de7(this branch). The pool serves 60 tokens' worth of traffic and then refuses:Type
🐛 Bug Fix
Changes
dynamic_rate_limiter_v3sends{"requests": 1, "tokens": 0}intoatomic_check_and_increment_by_nbecause a request's token count is unknowable before the model answers._build_descriptor_atomic_payloadthen dropped every counter whose increment was non-positive, so the token key never reached the Lua KEYS list; no TPM limit was read, compared, or enforced. On a model group withtpmand norpmthe enforced descriptor set collapsed to zero keys and the pre-call hook returned OK with an empty status list.The increment could not simply be relaxed to allow zero, because zero already means "do not track this dimension" for
check_and_increment_tokens, which passes tokens only and relies onshould_rate_limitfor RPM; letting zero through there would double-enforce RPM. So the two meanings are now distinct values:RATE_LIMIT_CHECK_ONLYcounters reach the Lua script withincrement = 0. Two adjustments make that a real check:max(increment, 1), so a check-only counter rejects atcurrent >= limitexactly like RPM's increment of 1 does, rather than leaking the request sitting precisely at the reservationToken counters are written post-call by
async_log_success_event, which increments the counter key and never touches the window key. A check-only counter therefore reads the raw counter value instead of treating a missing window as an empty one, matching how key/team/user TPM already reads throughis_cache_list_over_limit; the counter's TTL is what bounds staleness. Both the Lua path and the in-memory fallback carry the same semantics.Since both enforced descriptors carry
tokens_per_unit, this restores model-wide TPM enforcement in this hook as well as the priority pool's.One property is unchanged: tokens are only known post-call, so a simultaneous burst still passes the read together. The pre-call check enforces against prior completed usage, not a hard cap.
Final Attestation
Link to Devin session: https://app.devin.ai/sessions/1a3869d0d9c64bdfa4c313091d87650e
Requested by: @shivamrawat1