fix(proxy): enforce max_parallel_requests as a per-slot concurrency gauge - #32441
Conversation
|
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR fixes a long-standing bug where
Confidence Score: 5/5Safe to merge — the concurrency gauge design is structurally sound, all four release paths are wired correctly, and the atomic Lua scripts prevent the check/acquire race that made the old windowed approach ineffective. The redesign addresses all three compounding defects described in the PR. The pre-call hook acquires a slot only after the windowed RPM/TPM check passes, so a windowed rejection never strands an acquired slot. The stashed slot ID ensures that success, failure, proxy-rejection, and disconnect callbacks each release exactly the slot they own. The in-memory fallback correctly treats a cached integer as real occupancy. The slot-not-released issue in async_post_call_failure_hook is fixed. The structural multi-gauge release concern is also addressed: the stashed acquisition carries all counter_keys. Test coverage is thorough. No files require special attention — the change is well-isolated to the parallel request limiter and its direct callers.
|
| Filename | Overview |
|---|---|
| litellm/proxy/hooks/parallel_request_limiter_v3.py | Core change: moves max_parallel_requests from windowed counters to Redis sorted-set gauge with atomic Lua scripts; slot acquire, release, and idempotency logic are correct; all four release paths (success, failure, proxy rejection, disconnect) now properly release the acquired slot; previous issue with async_post_call_failure_hook is fixed; in-memory fallback correctly handles integer mirrors from Redis; structural multi-gauge release is correct. |
| tests/test_litellm/proxy/hooks/test_parallel_request_limiter_v3.py | Tests updated to reflect the slot-registry model with new coverage for window-roll correctness, rejected requests not consuming slots, Redis script args contract, in-memory fallback with mirrored integers, post-call failure hook idempotency, and disconnect with stale key config. |
| litellm/proxy/utils.py | _release_max_parallel_requests_on_disconnect now accepts request_data to pass the stashed acquisition to the limiter, fixing the old guard that keyed on the key object's current max_parallel_requests value rather than whether a slot was actually acquired. |
| litellm/proxy/common_request_processing.py | One-line change: passes request_data to _release_max_parallel_requests_on_disconnect so the disconnect handler can locate the stashed slot acquisition. |
| litellm/proxy/proxy_server.py | One-line change: passes request_data to the disconnect release in async_data_generator. |
Reviews (3): Last reviewed commit: "fix(proxy): release the parallel slot on..." | Re-trigger Greptile
|
@greptileai please review the current head 2de5429 |
|
@greptileai please review the current head c681833 |
|
Any ETA on this merge? |
9495012 to
9ce90a2
Compare
…auge The v3 rate limiter tracked max_parallel_requests with the same sliding-window machinery as RPM/TPM. A concurrency gauge cannot live on a windowed counter: every window roll reset the counter to 1 while requests were still in flight, the completion decrements for those forgotten requests then drove the counter negative, and rejected requests left stranded increments that nothing released. Under sustained load a key with max_parallel_requests=5 let backend concurrency climb to the full client concurrency (observed 60 on a live proxy) while the proxy kept returning 429s for everyone else Replace the windowed counter with a per-slot registry (Redis sorted set of slot ids scored by acquire time, with an asyncio-locked in-memory fallback): admission atomically prunes expired slots and registers a new slot id only when in_flight + 1 <= limit, so rejected requests never occupy a slot; success, failure, and client-disconnect paths release exactly the slot id this request acquired (stashed in the request metadata channels), so a release without a matching acquire or a double-fired callback can never free another request's slot; and a slot leaked by a crashed worker is pruned individually after its TTL even under continuous traffic Resolves LIT-4259 Fixes #16011
…in the in-memory fallback Address review findings on the slot-registry gauge: the acquisition stash now carries the gauge counter keys alongside the slot id, so the release paths free the slot from every gauge it was registered under instead of hardcoding the api_key scope, and the disconnect release keys off the stashed acquisition instead of the key object's current max_parallel_requests configuration (which can change mid-request). The in-memory fallback now treats a cached integer (the count mirrored from the last successful Redis script call) as real occupancy, carrying it forward as a floored counter during a Redis outage instead of restarting from an empty registry
async_post_call_failure_hook is the only callback that fires when a downstream hook (guardrail, budget check) rejects a request after the rate limiter's pre-call hook acquired a slot; async_log_failure_event is a completion-level callback and never runs for proxy-side rejections. Release the stashed acquisition at the top of the hook, before the TPM reservation guard, so those slots do not linger for the full slot TTL and wedge the key at its limit under moderate rejection rates. Clearing the acquisition marker keeps the release idempotent when a later failure callback runs in the same flow
…rror, and TPM rejection release Four behaviors of the slot-registry gauge had no direct test: a successful completion releasing exactly its acquired slot, read_only callers counting in-flight slots through the count script (and degrading to the local mirror when the script fails) without acquiring, the Redis release script mirroring returned counts into the local cache, and the TPM reservation rejection releasing the already-acquired slot before raising
…er annotations The slot-gauge code added Tuple/List/Dict and Optional[...] annotations, pushing the UP006 and UP045 strict-rule totals past their ceilings in ruff-strict-budget.json. Convert only the annotations this branch introduces to builtin generics and PEP 604 unions, leaving the rest of the module untouched.
9ce90a2 to
d29a777
Compare
561b679
into
litellm_internal_staging
Relevant issues
Fixes #16011
Linear ticket
Resolves LIT-4259
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@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
All runs below are against a live proxy (
python litellm/proxy/proxy_cli.py --config config.yaml --port 4259) with real Postgres and Redis containers. Measuring backend concurrency requires an upstream slow enough to keep requests in flight across rate-limit windows, so the concurrency measurement uses a local OpenAI-compatible upstream that sleeps 12s per request and records its own max concurrency (a real provider answers in under a second, which hides the window-roll bug the issue reports; vLLM under load behaves like the slow upstream). A real OpenAI call is included at the end to show end-user behavior against a real providerSetup: virtual key with the limit from the issue
Load: 60 concurrent closed-loop workers for 45s (the issue's oha scenario),
LITELLM_RATE_LIMIT_WINDOW_SIZE=10so in-flight requests span window rollsBefore the fix (current
litellm_internal_staging):The upstream saw 60 concurrent requests on a key limited to 5; concurrency climbed monotonically (
new max concurrency: 1 ... 60in the upstream log) exactly as the vLLM logs in the issue showRunning: 200 reqson a limited keyAfter the fix (same proxy command, same key, same load):
Backend concurrency never exceeds the configured limit of 5, and throughput matches the theoretical ceiling for 12s requests (45s / 12s x 5 slots is about 18 completions)
Controlled burst, 8 simultaneous requests against the limit-5 key:
The gauge in Redis is now a per-slot registry instead of a windowed counter (three requests in flight):
Real provider (OpenAI
gpt-5.4-mini, real API key, real money), key withmax_parallel_requests: 2, 8 simultaneous requests:and a single call after the burst drains, showing the response and the parallel-request headers:
Independent end to end verification
A separate agent session verified the fix from a fresh clone with its own Postgres, Redis, slow mock upstream, and virtual key. On the base branch litellm_internal_staging at 6f6bd45 the bug reproduced cleanly: backend concurrency climbed past the limit and pinned at 60 on a key limited to 5 (670x429, 180x200). On the PR branch at c681833, with Redis flushed and the same config and key, the identical 60-worker load held backend max_active at exactly 5 for the entire run (3765x429, 15x200), the 8-request burst returned exactly 5x200 and 3x429 with upstream max_active 5 during the burst, and the gauge key in Redis was a zset whose ZCARD was 5 with exactly 5 slot ids scored by acquire time and a TTL of 3596 seconds
Type
🐛 Bug Fix
Changes
max_parallel_requestsis a concurrency gauge, but the v3 limiter enforced it through the same sliding-window counters as RPM/TPM. Three compounding defects made the limit meaningless under sustained load: every window roll reset the counter to 1 while requests were still in flight, so each roll admitted a freshlimitof concurrency on top of what was already running; the completion decrements for those forgotten requests then drove the counter negative, admittinglimit + Nmore; and the increment-before-check pattern left a stranded +1 for every 429-rejected request that no callback ever releasedThis PR moves
max_parallel_requestsout of the windowed counters onto a per-slot registry. In Redis the gauge key becomes a sorted set of per-request slot ids scored by acquire time, and admission runs a Lua script that prunes expired slots and registers a new slot id only whenin_flight + 1 <= limit, so a rejected request never occupies a slot. Without Redis, an asyncio-locked in-memory registry provides the same semantics per worker. The acquired slot id is stashed in the request metadata channels (the same plumbing the TPM reservation uses), and the success, failure, and client-disconnect paths release exactly that slot id. Releasing by id makes double-fired callbacks and proxy-side rejections structurally harmless: removing an absent member is a no-op, so a release can never free a slot owned by another in-flight request. Slots leaked by a crashed worker are pruned individually afterPARALLEL_REQUEST_SLOT_TTL_SECONDSeven while the key stays busy, which the old whole-key TTL could not doThe windowed RPM/TPM logic is unchanged;
should_rate_limitruns the gauge phase after the windowed check so a windowed rejection never strands an acquired slot. Regression tests cover the window roll not resetting the gauge, rejected requests not consuming slots, releases without a matching acquire not freeing foreign slots, idempotent double releases, the Lua argument contract, and the disconnect release pathTwo follow-up commits address review findings. The acquisition stash now carries the gauge counter keys alongside the slot id so the release paths free the slot from every gauge it was registered under, the disconnect release keys off the stashed acquisition rather than the key object's mutable configuration, and the in-memory fallback treats a cached integer (the count mirrored from the last successful Redis script call) as real occupancy instead of restarting from an empty registry during a Redis outage. async_post_call_failure_hook now releases the stashed slot before the TPM reservation guard, since it is the only callback that fires when a downstream hook such as a guardrail or budget check rejects a request after the slot was acquired; without it those slots lingered for the full slot TTL
During a rolling upgrade, pods on the previous build treat the gauge key as a string counter while upgraded pods use a sorted set; both sides catch the resulting WRONGTYPE script failure and degrade to per-pod in-memory enforcement, and the conflict clears itself once the previous build's keys expire after the rollout completes