feat(shadow_eval)!: gate the per-key budget on dollar spend instead of turns - #37555
Conversation
|
@greptileai can you reply here with a score and why that score? |
|
bugbot run |
|
Score: 4/5 Here's why: What's done well:
What holds it to 4/5:
|
Greptile SummaryThis PR replaces shadow-evaluation turn budgets with per-key dollar budgets and records shadow plus judge costs on each attempt
Confidence Score: 3/5The PR should not merge until billed costs remain accounted for across failed persistence and unjudgeable shadow responses The new budget can permanently undercount real evaluation spend, allowing jobs to continue billing after their configured dollar limit Files Needing Attention: litellm/integrations/shadow_eval_logger.py
|
| Filename | Overview |
|---|---|
| litellm/integrations/shadow_eval_logger.py | Adds spend-based admission and billed-cost recording, but loses costs after persistence failures and on successful empty shadow responses |
| litellm/proxy/management_endpoints/auto_router_endpoints.py | Updates lifecycle SQL and response aggregation to use persisted shadow plus judge spend while retaining legacy turn behavior |
| litellm/types/management_endpoints/auto_router_endpoints.py | Replaces the start request's turn limit with a validated dollar budget and exposes per-key spend |
| litellm-proxy-extras/litellm_proxy_extras/migrations/20260819000000_shadow_eval_max_budget/migration.sql | Additively introduces nullable max_budget and non-null shadow_cost while preserving legacy rows |
| ui/litellm-dashboard/src/app/(dashboard)/cost-optimization/_components/ShadowEvalSection.tsx | Replaces the turn-budget input and display with per-key dollar-budget and total-spend UI |
Comments Outside Diff (1)
-
litellm/integrations/shadow_eval_logger.py, line 831-832 (link)Empty Responses Lose Billed Cost
When a billed shadow response has no judgeable text, this branch records zero cost, allowing subsequent sampling beyond
max_budget.Knowledge Base Used: Auto-router Shadow Evaluations
Reviews (1): Last reviewed commit: "feat(shadow_eval)!: gate the per-key bud..." | Re-trigger Greptile
| ) | ||
| await self._jobs_cache.async_set_cache(_JOBS_CACHE_KEY, jobs) | ||
| self._job_starts = {} # rebind-ok: new generation, counts absorbed into the fill | ||
| self._job_costs = {} # rebind-ok: new generation, costs absorbed into the fill # mutable-ok: per-generation counter |
There was a problem hiding this comment.
Cache Refill Drops Failed Writes
When an attempt write fails, refill clears its local cost and rebuilds spend from persisted rows, allowing billing beyond max_budget.
Knowledge Base Used: Auto-router Shadow Evaluations
There was a problem hiding this comment.
Intentional: spend derives from persisted rows, the file's core invariant. A failed write loses its sample and spend alike, exactly as the turn budget did; the valve bounds the residue
There was a problem hiding this comment.
Acknowledged — deriving spend only from persisted rows is a clean invariant, and failed-write residue being bounded by the valve is consistent with how the turn budget handled the same case. Makes sense.
There was a problem hiding this comment.
Revisited in c033429: attempt costs now advance the proxy's cross-pod spend counter, so a failed row write can no longer reopen the gate
| for job in (await self._active_jobs()).get(str(api_key_hash), ()) | ||
| if datetime.now(timezone.utc) < job.ends_at | ||
| and job.attempts + self._job_starts.get(job.id, 0) < job.max_turns | ||
| and (job.max_budget is None or job.spend + self._job_costs.get(job.id, 0.0) < job.max_budget) |
There was a problem hiding this comment.
Medium: Spend budget is not reserved before dispatch
This check only includes completed attempts. A key holder can send a burst of sampled requests while every pod still sees the same spend; each pod can launch up to 16 shadow-and-judge pipelines concurrently and independently consume toward the full cap before _job_costs or the database is updated. Since forwarded generation parameters include user-controlled output limits and new jobs have a 10,000-attempt valve, the user can drive eval charges well beyond max_budget. Use a shared atomic per-job reservation before dispatch and reconcile it with actual cost afterward, or otherwise enforce a cross-pod reservation that bounds the maximum in-flight spend.
PR overviewThis pull request changes shadow evaluation so each key’s evaluation budget is enforced using dollar spend rather than turn count. It updates the shadow evaluation logger’s budget tracking and dispatch behavior. One issue has been addressed, but spend enforcement remains vulnerable to concurrent oversubscription because costs are recorded only after evaluation attempts complete. A key holder can burst sampled requests across workers and trigger shadow-and-judge charges substantially beyond the configured maximum budget. Shared atomic reservation before dispatch is still needed to make the dollar cap reliable. Open issues (1)
Fixed/addressed: 1 · PR risk: 6/10 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
77c84ac to
82a488b
Compare
|
Addressed the silent rename in 82a488b: a request still sending max_turns now gets a 422 naming max_budget as the replacement, with a pinning test, and the live proof in the description shows the response. A blanket extra forbid would also reject harmless unknown fields the rest of the API tolerates, so the rejection is targeted at the one retired knob On the stop guard's two correlated subqueries: each subquery is bounded by the 10,000-row valve per leg through the job_id index and the stop endpoint is a rare admin action, so the CTE consolidation buys nothing measurable here On the default: the dollar exposure is now explicit per key in the request and the form, unlike a turn count whose dollar value was unbounded by construction, and a 100-key job requires naming 100 keys deliberately. Happy to lower it if you disagree Same sha also fixes Bugbot's two findings: billed shadow cost is now recorded on every exit path, empty replies and pipeline errors included @greptileai can you reply here with a score and why that score? |
|
bugbot run |
|
4/5 — the core concerns from the original review are addressed and the implementation is clean. Here's the breakdown: What's solidCost extraction is correct. Every exit path records
422 on Migration is safe. Test coverage is thorough. The new tests cover: spend-gated skip paths, the pod-local cost accumulator closing the budget within a cache generation, legacy turn-gated jobs, empty-reply cost billing, post-shadow pipeline error cost billing, the 422 rejection, and the SQL shape assertions for both the sweep and stop guard. What prevents a 5Multi-pod overshoot can be significant at small budgets. Stub-backed proof only. Real-provider re-run is acknowledged as owed. The local stub uses a fixed deterministic cost, which is a good test rig, but deployment-pricing behavior at a real provider (the reason The stop guard's correlated subqueries. Two The implementation is correct, the migration is backward-compatible, and the test suite covers the right cases including the edge paths that existed as bugs before. The remaining items are documented caveats or polish rather than correctness issues. |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 82a488b. Configure here.
82a488b to
c033429
Compare
|
c033429 closes the multi-pod overshoot with the codebase's established owner: attempt costs advance the proxy's cross-pod spend counter (spend:shadow_eval:{leg}) via the same increment path key budgets use, and every pipeline re-checks get_current_spend before its shadow call, with the fill-time DB sum as the counter's authoritative floor. The pod-local accumulator is deleted. Overshoot is now bounded by samples already in flight when the cap crosses, and a two-pod test pins the shared-counter behavior The proof in the description is also fully real now: every leg is claude-haiku-4-5 through the litellm sandbox gateway, real provider calls priced by the real cost map, before and after legs both. No stubs remain @greptileai can you reply here with a score and why that score? |
|
bugbot run |
|
Score: 4.5/5 Here is what lands the score where it is. What works well
What leaves 0.5 on the table
Neither of these is a blocker; both are bounded failure modes with documented limits. The implementation is correct, the tests are substantive, and the real-call proof in the description closes the loop. |
c033429 to
d632e60
Compare
|
Rebased onto current staging: the lint job needs .github/actions/detect-changes, which my base predated. Zero diff change. @greptileai can you reply here with a score and why that score? |
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit c033429. Configure here.
d632e60 to
8c19cbf
Compare
|
8c19cbf aligns the counter wrappers with the budget owner's failure semantics: a raising read (fail-closed enforcement) now skips the sample, increment failures log at warning with the outage mode named, and the SQL block documents the two spend sources. @greptileai can you reply here with a score and why that score? |
|
bugbot run |
|
The three claims about commit
Looking at the diff, all three are present: Raising read skips the sample ( try:
spend = await self._read_job_spend(...)
except Exception as e: # unverifiable budget: skip the sample rather than spend on it
verbose_logger.warning("shadow_eval: budget unverifiable for %s, sample skipped: %s", job.id, e)
returnCorrect fail-closed semantics. Increment failure logs at warning ( except Exception as e:
verbose_logger.warning("shadow_eval: spend counter increment failed for %s: %s", counter_key, e)The docstring names the degradation mode ("Redis outage freezes the counter, gate degrades to fill floor, overshoot grows to in-flight plus one TTL"), but the warning message itself doesn't inline that description — an operator reading logs won't immediately know what the system's behavior is under that failure. SQL block documents two spend sources — The two-try restructuring in One real gap: the warning for counter increment failures doesn't name the degradation mode inline. Score: 4/5. The semantics are correct — fail-closed read, fire-and-forget write, cost recorded on every billed exit — and the injectable reader/writer pair makes the wrappers properly testable. The only gap is the warning message not carrying the degradation mode inline. |
|
On the two 4/5 notes: LiteLLM_ShadowEvalAttempt has carried an index on (job_id) since its first migration, so the SUM subqueries are index-served and valve-bounded. The zero-cost interaction is documented where SHADOW_EVAL_TURN_VALVE is defined; that valve exists precisely because zero-cost attempts cannot consume spend budget |
8c19cbf to
b8bdcb5
Compare
|
b8bdcb5 fixes Bugbot's HiddenParams finding and the lint budget breach (reportPrivateUsage suppression with reason). The image-scan red is base-image busybox/openssl CVEs with upstream fixes, unrelated to this diff and not a required check. @greptileai can you reply here with a score and why that score? |
|
bugbot run |
|
Score: 4/5 Here's what's working well and the one thing holding it back from 5: Why it's strong: The dual-gate admission design is correct — the
Cost is recorded on every exit path — empty shadow reply, pipeline error between shadow and judge, judge failure — so the counter never undercounts and the gate can always close. The tests The migration is correctly additive (the The cross-pod spend counter is the same one key budgets use, so counter keys and invalidation semantics can't drift from auth's. The one structural concern:
Everything else — sweep SQL, stop guard SQL, |
|
On the private-import note: agreed it is the one unenforced seam. A public wrapper adds surface to the owner for no behavior change, so leaving it suppressed with the reason; a rename would surface as the warning-logged degradation plus a grep hit in the renamer's sweep |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit b8bdcb5. Configure here.
b8bdcb5 to
46e3987
Compare
|
46e3987 adds the public increment_spend_counter wrapper in the counter owner (suppression gone) and rebases over merged #37389, converting its per-key Budget used column to dollars with a turn fallback for legacy rows. @greptileai can you reply here with a score and why that score? |
|
bugbot run |
|
Score: 5/5 Here's the full picture on commit 46e3987: What the commit does:
Why it holds up:
The one acknowledged operational caveat — old pods mid-deploy price attempts as judge-only — is documented and bounded by deploy duration, and doesn't affect gate correctness post-deploy. |
46e3987 to
785a1a2
Compare
|
785a1a2 is test-only: it pins the owner contract refuting Bugbot's cold-counter finding (a reset counter degrades to the fill floor, never zero). No production change since your 5/5 sha. @greptileai can you reply here with a score and why that score? |
|
bugbot run |
|
5/5 — no production change, and the new tests are doing real work. The key test that refutes the cold-counter concern exercises the actual # counter at 0.05, fallback 0.42 → max wins → 0.42 ✓
assert await proxy_server.get_current_spend(counter_key=counter_key, fallback_spend=0.42, max_budget=1.0) == 0.42
# key deleted (cold) → counter reads 0, fallback 0.42 → still 0.42 ✓
proxy_server.spend_counter_cache.in_memory_cache.delete_cache(key=counter_key)
assert await proxy_server.get_current_spend(counter_key=counter_key, fallback_spend=0.42, max_budget=1.0) == 0.42That directly pins the owner contract: a Redis expiry degrades to the ≤10s-stale DB sum (the The remaining new tests also fill real gaps: sibling-pod counter visibility, legacy |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 785a1a2. Configure here.
|
Ran a live E2E QA of the per-key spend budget on this branch at 785a1a2 (proxy :4000, real Anthropic claude-haiku-4-5 deployments behind auto-router Budget gating works. Job started via UI with
422 on max_turns + legacy turn gating
Setup noteIf Screen recording of the full run: /home/ubuntu/screencasts/rec-774d3a10-3be6-440c-9404-94910e6c185f/rec-774d3a10-3be6-440c-9404-94910e6c185f-edited.mp4 |
785a1a2 to
f5d959a
Compare
|
f5d959a clears the two frontend-lint warnings: keySpent's boolean split into named conditions and the legacy fixture object named in the test. No behavior change. @greptileai can you reply here with a score and why that score? |
|
bugbot run |
|
The two fixes are both correct and clean.
Named fixture — Both are no-behavior-change as stated. For the commit specifically: 5/5. It does exactly what it says — two targeted lint fixes, no logic altered, and the resulting code is clearer than before. For the PR overall, the feature is well-structured: the migration is additive and backward-compatible, the NULL guard for legacy jobs is consistent throughout (DB schema, API types, UI display, and tests all handle the |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit f5d959a. Configure here.
TLDR
Problem this solves:
How it solves it:
max_budget(USD per key) instead ofmax_turnsresponse_costfigure, so deployment pricing is respectedUser Flow
Before: an operator wants to cap a shadow eval's own overhead at a dollar amount, and there is no way to say that
{"router_name": "my-router", "api_key_ids": ["<hash>"], "judge_model": "judge", "shadow_percentage": 100, "max_budget": 0.01}max_budgetfield is silently ignored and the response shows only"max_turns": 200"judge_spend": 0.0even though the spend logs bill real dollars for every judge callAfter: the same request caps the eval at one cent per key
"max_budget": 0.01"max_budget": 0.01on their key, and sampling stops as soon as the key's recorded shadow plus judge spend reaches it, checked against a cross-pod counter"spend": 0.012327, "status": "completed", matching what was billedRelevant issues
Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
uv run pytest tests/test_litellm/<your_test_file>.py -v. Leave the suites (make test-unit-*,make test-unit) to CI: it finishes in ~15 minutes where a laptop takes an hour or more@greptileaito re-request a review after pushing changes)Screenshots / Proof of Fix
Rig: local proxy from this branch on :4321 with real Postgres (all migrations plus this PR's). Every deployment (
cheap/strong/judge, plus auto-routermy-router) points at claude-haiku-4-5-20251001 through the litellm sandbox gateway, so every shadow and judge call is a real provider call priced by the real cost map and costing real money. No stubs and no mock responses anywhere in this runBefore (3f2e0ba)
A dollar budget cannot be expressed and judge_spend lies
curl -s http://127.0.0.1:4321/auto_router/shadow_eval/start -H "Authorization: Bearer sk-1234" -d '{"router_name": "my-router", "api_key_ids": ["<hash>"], "judge_model": "judge", "shadow_percentage": 100, "max_turns": 2, "max_budget": 0.001}'creates the job;max_budgetis silently ignored, the response carries only"max_turns": 2{"model": "cheap", "max_tokens": 30, "messages": [{"role": "user", "content": "real before turn N: name one prime number"}]})"attempt_count": 2, "judge_spend": 0.0, "status": "completed": the turn count gated it, and judge_spend reads zero because name-based pricing missed for theopenai/-prefixed deploymentLiteLLM_SpendLogsshows the judge legs billing real money: 2 rows taggedshadow_eval_judgetotaling $0.001160, uncappable by any knobAfter (f5d959a; behavior captured at c033429, since unchanged on these paths)
The dollar budget bounds the job with real per-attempt costs
curl -s http://127.0.0.1:4321/auto_router/shadow_eval/start -H "Authorization: Bearer sk-1234" -d '{"router_name": "my-router", "api_key_ids": ["<hash>"], "judge_model": "judge", "shadow_percentage": 100, "max_budget": 0.01}'returns"max_budget": 0.01, "max_turns": 10000, "status": "running"{"model": "cheap", "max_tokens": 500, "messages": [{"role": "user", "content": "real after turn N: write a 250 word overview of a different chemical element"}]})"attempt_count": 4, "spend": 0.012327, "judge_spend": 0.005002, "status": "completed": sampling stopped the moment recorded spend crossed the cap, with the 10,000 turn valve untouchedSELECT outcome, shadow_cost, judge_cost FROM "LiteLLM_ShadowEvalAttempt" ...shows every row carrying real billed costs: shadow $0.001785 to $0.001870, judge $0.001232 to $0.001266curl -s -X POST .../auto_router/shadow_eval/<job>/stopanswers 400"Job <job> is already completed"A caller still sending the retired max_turns is told, not silently defaulted
curl -s http://127.0.0.1:4321/auto_router/shadow_eval/start ... -d '{"router_name": "my-router", "api_key_ids": ["<hash>"], "judge_model": "judge", "shadow_percentage": 100, "max_turns": 200}'"max_turns was replaced by max_budget, the per-key USD cap on the eval's own spend"A pre-migration job keeps its turn budget
UPDATE "LiteLLM_ShadowEvalJob" SET max_budget = NULL, max_turns = 2 WHERE group_id = '<job>'"attempt_count": 2, "max_budget": null, "spend": 0.00114, "status": "completed": the turn budget gated it, the recorded spend is display onlyType
🆕 New Feature
Caveats (if any)
max_turnswithmax_budgetFinal Attestation
Note
Medium Risk
Breaking API change plus new spend-budget gating that writes Redis spend counters and attempt costs. Mispricing or counter failure can overshoot or skip samples, but it reuses existing budget primitives and fail-closed reads.
Overview
Breaking: starting a shadow eval now takes per-key
max_budget(USD, default $10) instead ofmax_turns. Callers still sendingmax_turnsget a 422. New jobs write a 10k-turn valve only to stop zero-cost error loops; pre-migration jobs keepmax_budgetnull and stay turn-gated.Each attempt now stores
shadow_costplusjudge_cost, priced from the billedresponse_coststamp (price-map fallback). Sampling, sweep, stop, and derived status all complete a key when recorded spend hits the cap. Admission re-checks the sharedspend:shadow_eval:{job_id}counter so pods stop before the next provider call; unverifiable spend skips the sample.The dashboard start form and per-key “budget used” column show dollars (
$spend / $max_budget), with a turns fallback for legacy jobs.Reviewed by Cursor Bugbot for commit f5d959a. Bugbot is set up for automated code reviews on this repo. Configure here.