fix(ptu): never retract a flat charge for a deployment the run cannot see - #37793
Conversation
|
@greptileai @cursor review this. Live proxy and real Postgres evidence is in the description, base versus head on the same scenarios. |
|
bugbot run |
Greptile SummaryThe PR bounds PTU sentinel-row pruning to deployments visible to the current rollup, preserving charges that the run cannot reassess.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| litellm/proxy/spend_tracking/ptu_flat_cost_rollup.py | Bounds stale PTU charge pruning to scanned deployment IDs and avoids deletion for an empty scan. |
| tests/test_litellm/proxy/spend_tracking/test_ptu_flat_cost_rollup.py | Updates pruning expectations and uses concise behavioral-rationale docstrings consistent with the surrounding tests. |
Reviews (2): Last reviewed commit: "fix(ptu): never retract a flat charge fo..." | Re-trigger Greptile
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 450004c. Configure here.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
… see The sweep ran unbounded whenever no config.yaml deployment was present, deleting the day's sentinel rows for deployments absent from the run's own view. A written charge records capacity that was reserved, so the only rows a run may retract are the ones it can reassess: a deployment it scanned and then declined to charge, because the window closed or the PTU config was removed. It is now always bounded to the ids it scanned
450004c to
58fb068
Compare
|
@greptileai re-review 58fb068. Tightened the docstring that restated setup; the others state why the behaviour is right, matching this file. |
|
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 58fb068. Configure here.
tin-berri
left a comment
There was a problem hiding this comment.
Real billing-integrity fix, and the direction of the fix matters here. The old logic treated a "database-only" run (no config.yaml records involved) as license to sweep unbounded — delete any stale sentinel row regardless of whether this run could actually see that deployment. That's backwards for billing: a deployment retiring or a router failing to register it isn't evidence the reservation never happened, it just means this run can't reassess it. The fix always bounds the prune to scanned_ids, so a charge is only retracted when the run positively confirms the window closed or the config was removed — never just because the deployment vanished from view.
Good test coverage, including test_a_run_that_scanned_nothing_issues_no_delete_statements for the master-key-rotation model-table-wipe window (a real, previously-noted race in this area) — a run that sees zero deployments now correctly touches zero rows instead of wiping the day's charges. `test_a_charge_the_run_cannot_reassess_is_left_alone" directly flips the old test's assertion to prove the new conservative behavior. Small, focused diff, CI green. Approved.
TLDR
Problem this solves:
How it solves it:
config_sourcedand the unbounded branch are goneUser Flow
Before: an admin retires one of a team's provisioned-throughput deployments and finds the capacity it already served silently erased from the team's bill
LITELLM_ENABLE_PTU_COST_ATTRIBUTION=Trueand two provisioned-throughput deployments owned by one teamAfter: the same retirement leaves the record intact
Relevant issues
Linear ticket
Refs LIT-5809
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)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
Real Postgres, the real scheduled entry point, no mocks. One team owns a live reservation and a retired one, and the retired deployment's charge sits on the day the nightly job sweeps.
Before (ff02d5c)
The retired reservation's charge is gone, and no rerun restores it
After (58fb068)
Type
🐛 Bug Fix
Changes
_LoadedDeployments.config_sourceddecided whether the sweep was bounded, and it was false whenever the router held no config.yaml deployment, which is every database-only proxy. The sweep then deleted the day's sentinel rows for deployments the run had never read, including ones another host declares and ones that were simply retired.A written charge records capacity that was reserved. The only rows a run can justly retract are the ones it can reassess, meaning deployments it scanned and then declined to charge, so the prune is now always bounded to
scanned_idsand the field is deleted. With nothing scanned the sweep issues no statements at all, which is the same protection the empty-scan case already relied on during a master-key rotation.Caveats (if any)
Final Attestation