fix(ptu): hand the prune a plain delete filter the query builder can serialise - #37571
Conversation
…serialise The bounded sweep built its predicate as a read-only mapping view, which the query builder refuses to serialise, so the nightly job raised as soon as a config-declared deployment was priced. The charges were already written by then, which is why the run looked like it had produced its rows. The in-memory table these tests run against accepts any mapping, so only a live run caught it. A predicate builder now returns a plain dict and is asserted as one, and the catch-up pass has a test covering a config-declared reservation.
|
@greptileai @cursor review this. It fixes a crash shipped in #37556: the prune's delete predicate was a read-only mapping the query builder refuses to serialise. |
|
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 db015e9. Configure here.
Greptile SummaryThe PR replaces the PTU prune predicate’s read-only mapping with independently constructed plain dictionaries and expands regression coverage.
Confidence Score: 5/5The PR appears safe to merge with no blocking failure remaining. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| litellm/proxy/spend_tracking/ptu_flat_cost_rollup.py | Constructs fresh plain-dictionary prune filters while preserving the existing bounded and unbounded deletion scopes. |
| tests/test_litellm/proxy/spend_tracking/test_ptu_flat_cost_rollup.py | Adds focused predicate-shape and config-declared catch-up coverage. |
Reviews (2): Last reviewed commit: "refactor(ptu): build the prune predicate..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
tin-berri
left a comment
There was a problem hiding this comment.
Reviewed the PTU nightly rollup crash fix. Root cause and fix line up: _prune_unrefreshed_sentinel_rows built its bounded delete predicate via MappingProxyType({**unbounded, "model": {...}}), and prisma's query builder refuses to serialize a mappingproxy, so the job died mid-run — after the sentinel charge rows were already upserted, which is why the crash was easy to miss in the UI (the day's cost still showed up correctly, but the lapsed-window alert and catch-up pass never got to run). The fix replaces it with a plain dict via a new _prune_filter helper, used for both the bounded and unbounded sweep. The proof-of-fix is convincing: before/after runs against real Postgres show exit=1/TypeError becoming exit=0/RollupResult(...), checking the actual exit code rather than skimming output — which is explicitly called out as how the defect reached staging in the first place. Good catch that the in-memory test double couldn't have caught this (it accepts any mapping), so the new test asserts the predicate's concrete dict type directly, plus a second test drives the catch-up pass over a config-declared reservation (the path the crash was blocking). Small, well-scoped, no auth/security surface — CI green (only the non-blocking benchmarks check still pending, no failures).
Both filter shapes are known upfront, so the bounded one is constructed directly rather than by mutating a value already declared Final. The catch-up test took two independent clock reads, which disagree across UTC midnight; it now derives both the reservation start and the expected last charged day from a single read, matching the three sibling tests.
|
@greptileai re-review 6742549. The prune predicate is now built in one shot, and the catch-up test takes a single clock read. |
|
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 6742549. Configure here.
TLDR
Problem this solves:
How it solves it:
User Flow
Before: an admin who turns on PTU attribution with a deployment in
config.yamlsees the day's costs appear and everything after it stopLITELLM_ENABLE_PTU_COST_ATTRIBUTION=Trueand a provisioned-throughput deployment declared inconfig.yamlAfter: the same job finishes
Relevant issues
Linear ticket
Refs LIT-5809
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
Real Postgres, the real scheduled entry point, one config-declared PTU deployment at 100 units and $0.02 an hour. Both sides run the same three lines against the same database, and the exit code is checked rather than the output skimmed, which is how the defect reached staging in the first place.
Before (c2b3c4b)
The charge is there, which is what makes the failure easy to miss: the upserts precede the sweep, so the usage view fills in and the run dies afterwards
After (db015e9)
The run completes, so the lapsed-window alert and the catch-up pass that follow it both get to run
Type
🐛 Bug Fix
Changes
_prune_unrefreshed_sentinel_rowsbuilt its bounded predicate withMappingProxyType, andprisma/builder.pyrefuses to serialise amappingproxy. A predicate builder now returns a plain dict for both the bounded and unbounded sweeps.The unit suite could not catch this: its in-memory table accepts any mapping, so the delete looked fine. The new test asserts the predicate's concrete type, and a second one drives the catch-up pass over a config-declared reservation, which is the path the crash was blocking.
QA runbook
Caveats (if any)
Final Attestation
Note
Cursor Bugbot is generating a summary for commit db015e9. Configure here.