fix(proxy): prevent no-Redis spend-counter reseed race - #35150
Conversation
…ent doubled team spend A recurring production incident intermittently rejected every request for an affected team even though the persisted LiteLLM_TeamTable.spend remained below its budget. The budget error reported "Budget has been exceeded! Team=coding Current cost: 31786.6605882599, Max budget: 20000.0", where Current cost was exactly 2x the persisted spend of 15893.33029412995. Every team member was falsely rejected for roughly ten minutes until the counter TTL expired, after which the incident could recur unpredictably. The no-Redis failure was an unlocked writer race in one Python process. First, a cold counter entered SpendCounterReseed.coalesced(), acquired its per-counter asyncio lock, read DB spend B, and yielded while awaiting the database. Second, _repair_stale_spend_counter(), or reseed_spend_counter_from_db() through budget reservation reconciliation, read the same DB spend and wrote B directly to the counter without acquiring that lock. Third, the cold reseed resumed and used additive async_increment_cache(key, B), leaving the enforcement counter at 2B. The original singleflight lock did not prevent this because repair writers bypassed it. This race does not need Redis or multiple workers: it occurs in a single worker with only the in-memory cache. The Redis branch already uses atomic SET NX from upstream PR BerriAI#27854, but the no-Redis additive branch and the unlocked repair route introduced by PR BerriAI#30684 remained vulnerable. This is the same spend-counter class of problem tracked in upstream issue BerriAI#27735. If left unfixed, the active budgeted value can be doubled. The enforcement counter reaches 2x real spend, or Nx real spend when repeated interleavings stack the DB value, causing every request for the team to be falsely rejected with "Budget has been exceeded" despite real headroom. TTL expiry temporarily heals the counter, making the outage recurrent and difficult to predict. Serialize stale-counter repairs on SpendCounterReseed's per-counter lock, which also covers reservation reconciliation through reseed_spend_counter_from_db(). In the no-Redis cold-seed branch, re-check the in-memory counter after the DB await and use the value already written by another lifecycle writer instead of additively applying DB spend. Serialize no-Redis request-cost increments on that same lock so a legitimate cost waits for the seed and is applied as B + cost rather than being lost or mistaken for an initialized counter. Add asynchronous regressions for a cold reseed racing a direct repair, a reservation-reconciliation reseed plus a request increment, and a request increment during the cold reseed. The tests prove the final counter remains B or B + cost rather than doubling.
|
|
|
@greptileai review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80a58f6921
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cached = spend_counter_cache.in_memory_cache.get_cache(key=counter_key) | ||
| if cached is not None: | ||
| current_value = float(cached) |
There was a problem hiding this comment.
When a no-Redis worker has a cold reseed waiting on the database and processes more than SPEND_COUNTER_RESEED_LOCKS_MAX_SIZE distinct counters meanwhile, _get_lock() can evict the still-held lock; a subsequent increment for the original key then acquires a new lock and writes only the request cost. Once the DB read returns, this new cache check adopts that partial value and discards the historical DB spend, allowing the affected budget to be undercounted until expiry. Keep held or awaited locks non-evictable, or otherwise verify that the registry still maps the key to the active lock before treating this cached value as a serialized writer
Useful? React with 👍 / 👎.
| 3. Seed the counter through `SpendCounterReseed.coalesced`, which safely | ||
| initializes a cold counter without clobbering concurrent writes. |
There was a problem hiding this comment.
The repository instructions import CLAUDE.md, whose first coding rule prohibits adding comments unless the user explicitly requests them; this commit adds explanatory docstring prose without such a request. Remove the new prose and keep the implementation self-explanatory instead
AGENTS.md reference: AGENTS.md:L1-L1
Useful? React with 👍 / 👎.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryThis PR prevents concurrent no-Redis spend-counter operations from corrupting budget state
Confidence Score: 5/5The PR appears safe to merge The previously reported active-lock eviction path is fixed because registry entries are marked in use before pruning and remain ineligible for eviction until all holders and waiters leave; no blocking failure remains
|
| Filename | Overview |
|---|---|
| litellm/proxy/db/spend_counter_reseed.py | Reworks lock-registry pruning to retain active locks and makes reseeds adopt intervening in-memory writes |
| litellm/proxy/proxy_server.py | Serializes no-Redis counter repairs and increments with reseed operations |
| tests/test_litellm/proxy/proxy_server/test_spend_counters.py | Adds coverage confirming the no-Redis increment path retains its existing cache behavior |
| tests/test_litellm/proxy/test_proxy_server.py | Adds controlled concurrency regressions for counter writers and active-lock registry pressure |
Reviews (6): Last reviewed commit: "Merge remote-tracking branch 'upstream/l..." | Re-trigger Greptile
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54fe5833c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| idle_key = next( | ||
| ( | ||
| key | ||
| for key, counter_lock in SpendCounterReseed._locks.items() | ||
| if counter_lock.users == 0 and not counter_lock.lock.locked() | ||
| ), |
There was a problem hiding this comment.
When more distinct counters are active than SPEND_COUNTER_RESEED_LOCKS_MAX_SIZE (especially when operators lower the configurable cap), every acquisition and release scans the ordered registry while holding the event-loop registry lock; if all entries are active, each scan traverses the entire collection. A burst of slow cold reseeds therefore causes quadratic Python work and blocks unrelated counter operations on the same event loop; track idle entries separately or maintain eviction eligibility without a full scan
Useful? React with 👍 / 👎.
| counter_lock = SpendCounterReseed._get_or_create_lock(counter_key) | ||
| counter_lock.users += 1 |
There was a problem hiding this comment.
Each lock acquisition now mutates the shared _CounterLock.users instance in place, with a matching decrement later, despite the imported repository guideline explicitly prohibiting mutation and recommending frozen dataclasses. Represent active usage without mutating a shared dataclass so this new synchronization state follows the repository's required convention
AGENTS.md reference: AGENTS.md:L1-L1
Useful? React with 👍 / 👎.
# Conflicts: # .github/actions/setup-uv-with-retries/action.yml
|
@yuneng-berri, could you review this spend-counter concurrency fix when you have a chance? Final head |
5b71e24 to
c0018b4
Compare
|
recheck |
|
recheck |
… fix/no-redis-spend-counter-reseed-race
|
@greptileai please re-review the latest head after synchronizing the current staging base. |
|
recheck |
2 participants
TLDR
Problem this solves:
When the proxy runs without Redis, a team's in-memory spend counter can get seeded with twice the team's actual spend from the database. Cause: two code paths write the same counter concurrently — a cold-cache reseed (which reads DB spend, awaits, then adds it to the counter) and the stale-counter repair / reservation-reconciliation path (which writes DB spend to the counter without taking the reseed lock). When they interleave, the DB spend is applied twice.
Result in production: every request from that team is rejected with
Budget has been exceeded! Team=... Current cost: 31786.66, Max budget: 20000.0even though the real spend was ~15,893 — exactly half. The outage lasts until the counter's 60s TTL expires, then recurs unpredictably.How it solves it:
All writers of a spend counter (cold reseed, stale-counter repair, reservation reconciliation, and no-Redis request-cost increments) now serialize on the existing per-counter lock, and the cold reseed re-checks the cache after its DB read — so it adopts a value another writer already seeded instead of adding DB spend on top of it.
Relevant issues
Fixes #27735
Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
make lint.04833c35575cb80a0199f1c0e883a145db5142d6at 5/5 and marked it safe to merge.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
This is an in-process no-Redis concurrency regression, so the proof uses deterministic controlled interleavings rather than a billable live-model request.
Before (
60729f733ec7dd1d2a37826c3bb776e27daa6d11),SpendCounterReseed.coalesced()awaited the DB read while holding its per-counter lock, but_repair_stale_spend_counter()and reservation reconciliation wrote the same counter outside that lock. The cold reseed then additively applied DB spend again.After syncing the final branch head (
04833c35575cb80a0199f1c0e883a145db5142d6):make lintalso passes against the synchronized staging base.The added regressions prove that:
B, not2B;B + cost;The identical patch was also verified against latest upstream
main(cad32fd9bc9cbbe3524269c24ffb399fe0481771): 384 passed.Type
🐛 Bug Fix
Changes
SpendCounterReseed's existing per-counter lock.DB spend + request cost.SET NXbehavior introduced by fix(spend_counter): seed Redis counter via SET NX to prevent cross-pod double-seed #27854.Final Attestation