Repository navigation
feat(budget): enforce the lifetime cap in the chat request path [3/3] - #170
hasitpbhatt wants to merge 32 commits into
Conversation
![orcacode-review[bot]](https://avatars.githubusercontent.com/in/4254738?s=60&v=4){kind=small}
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 1 issue in this PR: 🟠 1 P1.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 251 calls · 14.1M tokens · 95% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
9a0f5f6 to
a82b232
Compare
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 2 issues in this PR: 🟠 2 P1.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 240 calls · 14.5M tokens · 95% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
b763f38 to
0857a86
Compare
There was a problem hiding this comment.
⚠️ Outdated review — the PR head changed from b763f38 to 0857a86 during the review; these findings describe a diff the PR no longer has.
These findings describeb763f38e51ce. Re-run the review to check the PR as it stands now.
The gate evaluated to blocked, but it is not enforced here: branch protection will not see it.
🐳 OrcaCode Review
Found 1 issue in this PR: 🟠 1 P1.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 257 calls · 15.5M tokens · 95% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 2 issues in this PR: 🟠 2 P1.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 244 calls · 14.5M tokens · 95% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
Characters-to-tokens estimation and usage detection for requests whose upstream never reported a measurement. No database or route state, so the pricing rules can be tested in isolation before settlement uses them.
`countable_usage` accepted any countable key that was merely present, so a
usage frame of `{"prompt_tokens": 0, "completion_tokens": 0}` — what LiteLLM
emits when the upstream reported nothing — counted as measured. Settling on it
records a zero cost for a delivered completion, which is exactly the bypass the
guard exists to prevent.
Only a countable key holding a positive number is now a measurement. A zero on
one side still counts when the other side is positive, so a prompt-only request
is unaffected. `bool` is rejected explicitly because `True` is an `int`.
A genuinely empty completion still prices at zero through
`blocking_delivery_chars` and `estimate_usage`, so this cannot overcharge.
text_chars only counted dict parts ({"text": "..."}), so an Anthropic-style
prompt of bare strings (`["hello"]`) priced at 0 characters. blocking_delivery_chars
counts the same shape on a completion, so a budgeted key's prompt was
under-priced whenever the wire used plain-string parts — the precondition is
/v1/messages forwarding Anthropic-style content, reachable by real clients.
Count plain string parts in text_chars the way blocking_delivery_chars does,
keeping the dict branch, so a prompt and a completion of the same content
price identical characters.
Verified: text_chars(["hello","world"]) == 10; a mixed list prices only its
textual parts.
Regression for the bare-string pricing fix: a list of plain strings (`["hello", "world"]`) and a mixed list with bare-string parts now have expected character counts. Mirrors blocking_delivery_chars, which already accepted str parts, so prompt and completion price the same characters.
…t on delivery, count tool_use parts - message_chars: prompt side counts tool_calls as well as content, matching the delivery path, so tool-call-only assistant turns no longer price at zero - countable_usage(delivered=True): a delivered completion needs a positive completion-side count; a prompt-only frame no longer settles it at zero - _part_texts: Anthropic tool_use parts count name and string input, shared by text_chars and blocking_delivery_chars so the two cannot drift
… with a test text_chars intentionally counts whitespace; blocking_delivery_chars skips it only for the delivered-or-not decision. Streamed deltas are priced through text_chars, so dropping whitespace there would undercount real output.
…vered opaque parts
- tool_use: count json.dumps(input or {}), the same serialization the
Anthropic and Gemini translators bill, instead of dropping object input
- delivery: count message.refusal, which reaches the client as output
- delivery: a non-text delivered part (image, audio) counts as one character
so it floors to one token and never settles at zero; the prompt-side
text_chars still counts it exactly
A non-text prompt part (image, tool_result) was billed but counted zero, so an image-only prompt floored to zero tokens. Such a part now counts one character, so it floors to one token. Both prompt and delivery paths share _part_texts.
…racter A tool_result's billable text is its content (string or text parts). It was priced as a single opaque character, undercharging multi-turn tool prompts by the length of every tool output. Its text is now counted; nested non-text parts still floor to one character.
0857a86 to
a32307b
Compare
🐳 OrcaCode ReviewThis pull request was not reviewed. A maintainer of this repository can comment No comments were posted for this commit. Details
OrcaCode Review — Route Smarter. Ship Safer. Spend Less. |
a32307b to
603595e
Compare
🐳 OrcaCode ReviewThis pull request was not reviewed. A maintainer of this repository can comment No comments were posted for this commit. Details
OrcaCode Review — Route Smarter. Ship Safer. Spend Less. |
2 similar comments
🐳 OrcaCode ReviewThis pull request was not reviewed. A maintainer of this repository can comment No comments were posted for this commit. Details
OrcaCode Review — Route Smarter. Ship Safer. Spend Less. |
🐳 OrcaCode ReviewThis pull request was not reviewed. A maintainer of this repository can comment No comments were posted for this commit. Details
OrcaCode Review — Route Smarter. Ship Safer. Spend Less. |
…ements A settlement that cannot be recorded leaves a delivered cost with no trace: the request-log row and the charge share one transaction, so both roll back together. The park holds that cost against the key's cap until a pre-check folds it in, oldest debt first, and clears a row whose charge already landed. Startup migration creates the table for deployments whose schema predates it. When even the park write cannot land, the amount is held in memory for the next pre-check to re-file. That hold keeps one entry per key rather than one per settlement, so a database outage that outlasts many requests cannot grow it without bound.
A settlement can merge into the key's in-memory hold while the park write is in flight. Popping the whole entry afterwards dropped the merged amount without ever persisting it, reopening the cap by that amount. Re-file until the write lands on a stable hold.
1. Durability probe now checks the persisted amount, not just row existence. A row that exists but under-covers the expected obligation is answered as not durable, so the give-up parks the full amount instead of trusting a short row. 2. pending_parked_spend no longer zeroes a hold when its trace_id has a durable row. It now subtracts the durable amount from the hold and adds only the uncovered remainder, so a merged hold is never double-counted and its shortfall is never dropped. 3. Both DELETE paths in settle_parked_spend carry compare-and-swap guards on the amount read inside the transaction. A concurrent increase to a park row now conflicts the fold instead of silently deleting an amount that wasn't billed. 4. Hold reconciliation after the commit keeps only the obligation not covered by the committed fold. A new trace_id prevents the remainder from colliding with a park row that was just deleted. The move==0 path (already-charged cleanup only) still reconciles holds. These four changes close the races where a growing hold could lose its growth, where a short durable row could be trusted, and where a folded row could be deleted past what the fold actually billed.
The previous design merged all failed settlements for a key into one hold entry under the first trace_id, so a fold that cleared an already-charged trace_id would silently delete the entire hold — losing other settlements that shared that trace_id. Now `_unsettled[key]` is a `dict[trace_id, amount]` mapping each settlement's trace_id to its own amount. Every call site is updated: * `_hold` keeps obligations separate; a trace_id is never reused for a different amount than the maximum seen. * `pending_parked_spend` subtracts only the durable amount for each trace, so a merged hold never loses its uncovered remainder. * `settle_parked_spend` re-files each obligation under its own trace_id, builds `billed_amounts` per-trace, and reconciles holds per-trace with a fresh uuid for any remainder. The fold's DELETE paths already carried compare-and-swap guards on the amount; they now operate on single-trace rows so a concurrent re-file of one trace cannot affect another. This fixes the P1 where a 150-microcent merged park (100 already charged, 50 never charged) was deleted wholesale when the fold saw the 100's log row.
…im, update comment When a fold trims a park row (the row fits only partially in the remaining allowance), the trim branch moved the allowance into the counter but never recorded the amount covered in `billed_amounts`. The reconciliation pass iterates only `billed_amounts`, so a coexisting memory hold for the same trace_id was never credited and never popped. `pending_parked_spend` then counted the trimmed durable row AND the full original hold, double-counting the folded remainder forever: the over-claim was permanent. Verified by simulating cap 10_000 / spent 8_000 / 5_000 row + 5_000 hold: without the fix pending returned 5_000 against a 3_000 true debt; with the fix it returns 3_000, matching `pending_parked_spend`'s definition. Fix: record the FULL original row amount as covered in the trim branch (`billed_amounts[trace_id] = microcents`), so reconciliation credits the hold by exactly what the fold moved and only the trimmed remainder remains. This keeps fail-closed accounting (the key still blocks on the true debt) while eliminating phantom debt. Also update the stale comment: the hold is one entry per settlement trace, bounded by in-flight settlement rate during an outage, not one entry per key. Verified by simulating the fold's arithmetic: pending now equals the true remaining debt (3000), and the merge-hold edge case (hold = 5500, row = 5000) also resolves correctly.
…, stop cancel-time double-bill - app/main.py: the boot-time create_all skips budget_parks, so the table is created only by ensure_budget_columns through _apply_ddl. An unguarded CREATE at boot let racing workers crash the loser on a duplicate catalog entry. - spend.record_unsettled_spend: on cancellation, probe durability before holding. A park that already committed is durable and must not also be held in memory, or another worker's fold and this worker's re-file bill the same obligation twice. - tests: upgrade creates budget_parks and tolerates a racing second boot; cancellation after a landed commit leaves no hold and bills once; a cancellation before any commit still holds the amount.
…ancel asyncio.shield protects the probe from the first cancel only. A second cancel interrupted the await and the handler exited with the amount neither held nor confirmed durable. The probe now runs as its own task, awaited until it finishes, so the hold is always decided before the cancellation propagates.
…path comments budget_parks is no longer created by create_all at boot, so the guarded branch in ensure_budget_columns is the creator for fresh databases as well as upgrades. The cancel-path comment now describes the probe loop, not shield.
…and guarded deletes Regression tests for the two P1 races, already fixed in 9f06929: - durability probe answers False when the row covers less than expected - pending counts the hold excess over a short durable row (100+50=150) - fold bills the durable part and keeps the excess held when re-file fails - stale-amount delete matches nothing; raised row bills in full
A key at its cap is refused with 429 before any upstream call is made, and a request that is served records its cost against the cap in the same transaction as the request-log row. A settlement that cannot be recorded after every retry is parked against the cap, so a delivery that was served cannot reopen it.
…ed charges for unpriceable models - Park the charge when the last-chance settlement retry fails with any error - Charge at most 1 microcent for measured-but-unpriceable blocking completions - Fix duplicate comment and clarify logging; tests cover the new behaviors
The two commit-cancellation arms returned normally once the detached commit (or the shielded last-chance retry) succeeded, which discarded the CancelledError. Fall through to the re-raise so teardown proceeds.
- A completed stream with no usage frame charges the remaining allowance only when the model's price is unknown; a catalog entry priced 0/0 costs nothing. - A cancellation during the blocking commit parks the charge when the row did not land, then propagates. The durability probe is shielded so it can answer while a give-up is unwinding a cancellation. - Route the parked-charge test through the HTTP path.
…d fails
The settlement durability probe fails closed: any DB error is answered as
"not durable", so the obligation is parked. When the commit landed but its
ack was lost and the read fails too, the charge is already in
`spent_microcents` and the park bills it a second time.
That double-count is intentional, not an oversight. We must choose between:
1. Fail closed (chosen) - park on an unreadable database. Cost: a key can be
billed twice for one delivery, in the narrow window where a commit landed,
its ack was lost, and the probe also could not read.
2. Fail open - trust an unreadable database to have recorded the charge.
Cost: the cap reopens for exactly the key whose settlement we are
unwinding, which is the failure the park exists to prevent.
Over-charging a capped key is the smaller harm: it is bounded by the key's
lifetime cap, it is visible in the park ledger, and it converges as the fold
runs. Under-charging it lets delivered spend through uncounted, silently, for
as long as the outage lasts.
Changing this later means making the probe able to distinguish the two cases -
e.g. reading `spent_microcents` directly rather than probing for the log row -
rather than flipping the boolean.
Comment only; no behaviour change.
The previous commit made the completed-without-usage branch consult `_has_known_price`, which is True for ANY catalog hit — so a gpt-4o-mini stream that completed without a usage frame was charged 0 instead of the remaining allowance. That is exactly the "opt out of measurement and stream for free" outcome `_unmeasured_charge` documents as the reason the branch exists. Charge zero only when the catalog entry is priced 0.0 in and 0.0 out. Every other model — priced or unknown — pays the remaining allowance. Also correct the parked-charge test: it now parks an amount larger than the cap through the public `record_unsettled_spend` entry point and asserts 429 through the HTTP path, so it fails if the pre-check is removed. The previous version passed with enforcement disabled.
…e remainder
`countable_usage` now rejects a zero-placeholder usage frame, which routes it
into `_unmeasured_charge`. With `policy=_REMAINING` that returned `cap - spent`,
so one request whose provider omits token counts consumed a key's entire
remaining lifetime budget — the opposite failure from the one the guard against
`{"prompt_tokens": 0}` exists to prevent.
The whole-remainder charge has no justification here: `delivered` is
`agg_output_chars > 0`, so whenever this branch is reached the delivered length
is already known. Every delivered character is counted and priced, which is
what stops a client from opting out of measurement and streaming for free.
`_unmeasured_charge` no longer takes `policy`, `cap` or `spent`, and the
`_REMAINING` / `_ESTIMATE` constants and the `usage_requested` flag are gone —
an unmeasured delivery now prices identically whether the stream completed or
faulted, because the ending does not change what the client received.
Verified by driving the function directly: a 400-character delivery on a priced
model charges 6_150 microcents rather than the 10_000-microcent remaining cap; a
1-character delivery charges 75; a known-free model still charges nothing; and
an upstream fault with nothing delivered still settles at zero.
…vers all endings
`text_chars` only counted dict parts (`{"text": "..."}`), so an Anthropic-style
content list of bare strings priced a prompt at zero characters while
`blocking_delivery_chars` counted the same shape on a completion. Accept a
plain `str` part in `text_chars` the way the completion path already does, so
the same content prices identically as prompt and completion.
`_unmeasured_charge` scoped the known-free-model exemption to
`ending == _STREAM_COMPLETED`, charging a free model whose stream ended in a
fault for characters the upstream never billed for. The exemption is now
unconditional: a catalog entry priced 0.0 in and 0.0 out costs nothing
regardless of ending, because the ending does not change the upstream's
agreed price for that delivery.
Replace the stale `_unsettled` comment that claimed a per-key bound, which the
per-trace dict no longer provides; state the real invariant and the unbounded
during outage trade instead.
Verified by driving the functions directly: bare-string and mixed content lists
are counted in both prompt and completion; free model + any ending -> (0, None);
priced model deliveries price from delivered chars only.
The streaming delivery counter read only `delta.get("content")`, so a tool call
a stream delivered contributed 0 characters to `agg_output_chars`. With no usage
frame, `delivered = agg_output_chars > 0` was False and the stream settled as a
zero delivery: the prompt-only charge (or zero for a fault) on arbitrarily large
streamed tool-call output. `blocking_delivery_chars` prices the same payload in
full through `tool_call_text`, and `_unmeasured_charge` exists precisely so the
two paths cannot drift apart — this was that drift.
Accumulate `tool_call_text(delta.get("tool_calls"))` into `agg_output_chars`
alongside the content text, mirroring the blocking path. `tool_call_text` is
streaming-safe: it handles dicts without a `function` key and accumulates
name/arguments split across chunks, verified by execution. A tool call now adds
its function name and string arguments to the delivery count, so an unmeasured
stream prices the characters the client actually received.
Verified by execution: a streaming tool-call delta previously added 0; it now
adds "get_weather{\"city\":\"Osaka\"}" (27 chars), matching the blocking path.
…eck, fix streamed tool-call count - prompt pricing uses message_chars (content + tool_calls) on both the streaming and blocking settlement paths - countable_usage receives delivered=, so a prompt-only usage frame no longer settles a delivered completion at zero - streamed tool-call count used chars += str, raising TypeError whenever a delta carried tool_calls; count len() of the returned text
…crocent The streaming settlement returned the row's 0 cost untouched when usage was countable, so a custom/unlisted model streamed at zero recorded cost forever. Mirrors the blocking twin's fail-closed floor: countable tokens with no known price charge 1 microcent.
…ancel coverage - spend: folding a park row writes a budget_folded tombstone in the same transaction; re-filing a tombstoned hold drops it, and a row re-filed in the race window is deleted unbilled. The cross-worker double-charge (spent 100 -> 200 for one obligation) no longer reproduces. - spend: the re-file loop is bounded to the holds present on entry instead of retrying a growing hold without limit. - migrate/main: budget_folded is created only through the guarded _apply_ddl path, like budget_parks. - pricing: the one-character non-text floor is documented as an approximation, with blocking/streaming parity tests pinning every part shape and the whitespace divergence. - enforcement: settlement-matrix tests (zero usage with/without delivery, pre-delivery failure, zero/negative/absent caps, every _unmeasured_charge ending) and a second-cancel probe test for the durable-row case.
a2b7b62 to
ef94d3c
Compare
|
@orcacode review |
🐳 OrcaCode ReviewThis pull request was not reviewed. A maintainer of this repository can comment No comments were posted for this commit. Details
OrcaCode Review — Route Smarter. Ship Safer. Spend Less. |
|
@orcacode review |
🐳 OrcaCode ReviewThis pull request was not reviewed. A maintainer of this repository can comment No comments were posted for this commit. Details
OrcaCode Review — Route Smarter. Ship Safer. Spend Less. |
|
@orcacode review |
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 1 issue in this PR: 🟠 1 P1.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 277 calls · 17.8M tokens · 95% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
| if not current: | ||
| _unsettled.pop(key, None) | ||
| # If the amount grew during the write, leave it in place for the | ||
| # next pre-check. |
There was a problem hiding this comment.
🟠 P1 Re-filing a hold after another worker trimmed its park row re-bills the already-billed portion
When a worker's fold only partially bills a park row (room < row amount), it trims the row to its remainder and, unlike a full bill, writes NO tombstone (lines 502-513). The trimming worker's own in-memory hold is reconciled, but a hold for the same trace in a DIFFERENT worker survives untouched. Sequence: (1) worker R parks obligation T=150 (durable row T=150 + R's memory hold T=150). (2) worker F (no hold) folds with room=100: bills 100 into spent_microcents, trims row T to 50, no tombstone. (3) R's next pre-check re-files its stale hold: _insert_park(T, 150) hits IntegrityError on the existing row and its raise path (lines 141-150) sets the row back to microcents=150 — re-asserting the full 150 even though 100 of it was already billed into the counter. R then pops its hold. (4) The durable row now claims 150 outstanding when only 50 is; pending_parked_spend returns stored 150 + held 0 = 150. When room next becomes positive (counter only grows, so the next fold with room requires a cap raise), the fold bills room again from the phantom 150 — e.g. cap raised 200→300 bills 100 of a true 50 remainder, over-billing the key by 50 and exhausting it at 300 when its true lifetime spend is 250; each further cap raise bills the leftover phantom again. Fix: the trim must leave a durable record of the billed amount (e.g. tombstone the trace with microcents=billed on a trim too, or store original amount alongside the remainder), and the re-file/_insert_park raise path must reconcile a hold against a trimmed row by the billed portion (raise only to amount - already_billed) instead of restoring the full original obligation.
2 participants
Orca-Code-Review — push 1
❌ 1 finding blocks merge
Wires
ApiKey.budget_limit_centsintoexecute_chat: a capped key at its cap isrefused with 429 before dispatch, and a request that is served leaves the counter
moved by exactly what it cost, in the same transaction as its request-log row.
What lands here
request refused before touching an upstream consumes no budget.
commit, and a persisted
trace_idproves the charge also landed, so a retrynever double-charges.
(
app.budget_pricing), with the ending decided in one place(
_unmeasured_charge) so no handler can price its own ending differently fromits siblings.
against the cap rather than dropping it.
budget_folded), so a memory hold thatoutlives another worker's fold is dropped at re-file instead of billed twice.
Fixes included
returned normally once the detached commit succeeded, discarding theCancelledError. They now fall through to the re-raise.usage branch charged
cap - spentfor every model. It now charges zero onlywhen the catalog entry is priced 0.0 in and 0.0 out.
propagating, and the durability probe runs to a decision even under a second
cancel.
settlement returned the row's 0 cost untouched for countable tokens on a
custom/unlisted model. It now mirrors the blocking twin's 1-microcent
fail-closed floor.
commit + lost ack + failed probe) was re-filed after another worker folded
it: spent 100 → 200 for one obligation. Folds now tombstone fully-billed
traces in the same transaction; re-filing a tombstoned hold drops it, a row
re-filed in the race window is deleted unbilled, and pending spend excludes
both. The old "fail-closed double-count" trade-off below no longer applies.
Trade-offs accepted deliberately
(
max(actual, charge)). Deliberate never-under-charge.budget_limit_cents=0rejects every request, including free models.Negative behaves the same; absent means uncapped. Pinned by tests.
character (one token), not its provider-side cost, which is unknowable here.
Counting base64 bytes would overcharge far more. Documented in
app/budget_pricing.py.Review state
OrcaCode Review (local) at
a2b7b62: passed, no findings. All reviewthreads resolved (0 open).
Verification
budget/enforce: 914 passing, ruff clean — includingsettlement-matrix tests (zero usage with/without delivery, pre-delivery
failure, zero/negative/absent caps, every
_unmeasured_chargeending),tombstone regression tests, blocking/streaming parity tests, and
cancellation boundary tests.
libstdc++; CI should not need the local workaround.Part of the budget stack tracked in #165. Merge order: helpers → models → enforce.