-
-
Notifications
You must be signed in to change notification settings - Fork 10.1k
fix(rate_limiter): enforce TPM pre-call in dynamic_rate_limiter_v3 priority pools #34592
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
devin-ai-integration
wants to merge
2
commits into
litellm_internal_staging
Choose a base branch
from
litellm_fix_priority_tpm_enforcement
base: litellm_internal_staging
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+304
−18
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the window key expires before the independently updated token counter, this branch reads usage from the previous window instead of resetting it, causing the first request in the new window to receive a false 429.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_eventthroughasync_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_sizeand 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_limitreads 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_eventwhile the window key is only ever written by advancing (RPM-style) counters. Resettingcurrent_counter = 0on 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 byasync_increment_tokens_with_ttl_preservation) to bound staleness is the correct contract here, and it matches whatis_cache_list_over_limitalready does for key/team/user TPM. I'm satisfied with the approach.