Repository navigation
fix(budget): settle an adapter fault on its delivery instead of the remaining budget - #163
hasitpbhatt wants to merge 29 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
✅ No findings — nothing to flag in this PR. Great work!
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 228 calls · 12.6M tokens · 94% 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
cb08e64 to
4cdf36a
Compare
|
@xizhuomengcontin , broke #91 into 4 stacked PRs, hopefully these will pass the Orca reviews now. |
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: 274 calls · 16.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
4cdf36a to
ff6468a
Compare
There was a problem hiding this comment.
🐳 OrcaCode Review
✅ No findings — nothing to flag in this PR. Great work!
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 319 calls · 20.8M tokens · 96% 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
ff6468a to
a6061ab
Compare
There was a problem hiding this comment.
🐳 OrcaCode Review
✅ No findings — nothing to flag in this PR. Great work!
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 283 calls · 18.7M 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
a6061ab to
6e1d570
Compare
There was a problem hiding this comment.
🐳 OrcaCode Review
✅ No findings — nothing to flag in this PR. Great work!
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 257 calls · 16M 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
6e1d570 to
e572dcd
Compare
There was a problem hiding this comment.
🐳 OrcaCode Review
✅ No findings — nothing to flag in this PR. Great work!
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 266 calls · 16.4M 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
|
Merge-order carve-out (see #165): this PR's "do not merge before #163" banner applies to #161/#162, not to #160. This PR is second in the stack order (after #160, before #162/#161). It must absorb newer commits from #162 (6f8a2a4) and #160 (7ca80a5, cap-clamped monotonic boot-seed repair) and be rebased before #161/#162 re-review; see #165. any build of this PR requires the Also: #163 currently lacks this stack's newer commits pushed after its last update (e.g. #162's |
e572dcd to
661d5c9
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. |
661d5c9 to
fe02ac4
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. |
|
@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
✅ No findings — nothing to flag in this PR. Great work!
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 267 calls · 16.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
|
Heads-up: #160 landed ( #160's push 4 rewrote Still open here, both from 09-25 and both anchored to live lines:
The second one is the same shape as the adapter-fault case I measured on #161 (full remaining allowance charged for a server-side fault, Full stack status is in #165. Rebase onto |
fe02ac4 to
e44979d
Compare
There was a problem hiding this comment.
🐳 OrcaCode Review
✅ No findings — nothing to flag in this PR. Great work!
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 340 calls · 22.4M tokens · 94% 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
…nforced budget_limit_cents from CreateKey A budget-only caller (model_allowlist=None, budget_limit_cents set) passed every one of Continuum-AI-Corp#151's _require_* helpers, because they only inspected model_allowlist. Such a key could mint an unrestricted child, list every key in its workspace, and revoke siblings. Extend all three helpers plus list_keys to treat allowlist OR budget as restricted. CreateKey.budget_limit_cents was reviewed-blocked (P1): the endpoint advertised a per-key budget that nothing enforces until the budget stack (Continuum-AI-Corp#161-Continuum-AI-Corp#163) lands, so it is removed. Restore it as a follow-up once Continuum-AI-Corp#161 merges. tests/integration/test_keys_authz.py now matches: restricted callers get 403 from POST, GET, DELETE; an unrestricted caller keeps full CRUD.
…ll what the log says A capped key forced `stream_options.include_usage` onto the blocking request too. That parameter only decides whether the last frame of a stream reports usage — a non-streaming completion always carries it — and LiteLLM forwards it without looking at `stream`, so OpenAI rejected every budgeted blocking request outright. The cap made the endpoint unusable rather than enforced. The fail-closed settlement also moved the counter without moving the row it was charging for, so a key could be exhausted by an amount no query over its request history reproduced. Both paths now record the charged amount on the row.
Every request for a budgeted key issued two identical `SELECT spent_microcents WHERE id = ?` round trips: `is_exhausted` loaded the counter to decide the 429 and the snapshot of the remaining allowance loaded it again straight after, on the same session with nothing written in between. The fix is a seam rather than an inlined comparison because the number and the boolean cannot both come from one read otherwise, and the route's single call is where a pre-check that has more to do than read a column will hang it.
…ng budget (cherry picked from commit 02c7bdc)
…normalize Anthropic usage keys Two holes in the enforcement layer Continuum-AI-Corp#161 added: - The blocking fail-closed arm fired whenever response had no truthy usage, so an EMPTY 200 (usage {}, or usage missing with empty content) permanently exhausted a budgeted key — inconsistent with the streaming twin that treats a {0,0} usage as known-zero, and with a client simply not waiting. Gate on delivered content: fail closed only when real content was returned and the cost is unaccountable. - _build_log_row read only prompt_tokens/completion_tokens, so a usage frame keyed input_tokens/output_tokens (what /v1/messages forwards) normalized to zero tokens and defeated every token-keyed gate. Read both shapes.
The three findings from the review of the previous push shared one root
cause: the charge was decided by four independent signals, none of which
owned the decision. usage_seen was set by whichever handler ran, presence
of a usage dict was treated as measurement, and nothing anywhere looked
at whether content had actually been delivered.
Handlers now record facts and nothing else. The provider-error, adapter
and client-disconnect arms set stream_ending; the loop sets
stream_completed the moment the upstream stream ends, before our own
[DONE] framing, so where a client disconnects stops being able to change
the bill. One function (_unmeasured_charge) turns those facts plus the
delivery characters into the charge, and the blocking path calls the same
function, so the two can no longer drift apart.
Consequences, all of them previously reachable holes:
- An empty unmeasured stream settles at zero instead of consuming the
whole remaining allowance, matching the blocking gate.
- A client that hangs up at the trailing [DONE] frame is charged what a
client that stayed connected is charged. Reading the full answer and
leaving was the cheapest way to use a capped key.
- A usage frame with no countable token key ({"total_tokens": 123}) is
unmeasured, on both paths. It used to normalize to zero tokens and bill
a delivered completion nothing.
- A blocking completion with content and no usage is priced from what it
returned, not charged the lifetime allowance. The old rule billed a
customer their entire budget for one request whose upstream omitted a
field.
- include_usage is no longer forced on a client that explicitly declined
it. Declining is priced from the delivery; a provider that ignores the
usage frame we requested still fails closed.
- The 1-microcent floor: a short delivery estimates below the smallest
representable amount, and a delivery that costs nothing is exactly what
the cap must not allow.
Rows now record the estimated tokens behind an estimated charge, so a
non-zero cost never sits on a zero-token row.
Tests: the empty unmeasured stream, the disconnect at [DONE], tokenless
usage on both paths, and the re-split of the fail-closed cases (requested
and dropped usage vs a client that declined it).
A debug print guarded by an env var was left inside _unmeasured_charge while working through the settlement rework: it reads an environment variable on every unmeasured settlement and writes to stdout inside a request handler.
The streaming retry loop detaches each commit so the in-flight stream cannot abort it, then waits for the task on the cancellation path. When the task is cancelled itself, that await raises and the arm passed over it with `pass`. Before enforcement that cost one log row; now it costs a budget charge, because the row and the settlement share a transaction: nothing is written, spent_microcents never moves, and nothing is logged. The cap reopens for exactly the key whose settlement failed, with no trace. The arm now re-runs the write inside anyio.CancelScope(shield=True) — the same primitive the rest of the teardown uses, and the only way an await survives a cancellation already unwinding us. `_commit_row` is idempotent on trace_id, so a commit that landed with a lost ack is recognised rather than charged twice. If even the shielded retry cannot land, the loss is logged with the amount instead of passing over it. This belongs here rather than in the park PR: enforcement is what makes the arm cost a charge, and this PR is where enforcement becomes live. A review finding it on this diff is right.
… live helpers _settle_unmeasured_stream died when settlement became one decision function: no runtime path calls it anymore, and its logic lives in _estimate_usage plus _unmeasured_charge. Carrying it (and a test file for it) would ship dead code with a passing suite. The unit tests are re-pointed at the live helpers so the character math and the countable-usage gate stay pinned, and _blocking_delivery_has_content says which rung consumes it.
…on it A park larger than the remaining allowance moved nowhere and stopped the scan, so the counter could sit below the cap on a key refused by a row nothing would ever shrink. Apply the allowance oldest-debt-first and rewrite the oversized row to its remainder, which keeps the invariant the fold exists to hold: either the queue is empty or the counter is exactly on the cap. The over-claim stays a row, so it still blocks and folds for free if the cap is raised. created_at alone is not a total order (second resolution on SQLite, ties on Postgres), and two workers computing the same fold have to agree on which row is the partial one, so the scan breaks ties on trace_id.
`checkfirst` asks the catalog and then creates, and the ask cannot see another worker's uncommitted CREATE, so two boots racing an upgrade both issue it and one is rejected. That was the only startup DDL outside `_apply_ddl`, and on Postgres the error aborts the transaction the rest of startup runs in: the worker never comes up, and never reaches the fold this table exists to feed. Generalise `_apply_ddl` to a callable so dialect-generated DDL goes through the same savepoint, and teach `_already_applied` the shape this collision actually takes in Postgres — a unique violation on the catalog, not an "already exists".
The retry handler rolls the failed attempt back before deciding, and that is an await — so it is where a cancellation aimed at the request can land. An exception raised inside a handler is not caught by the same try's other arms, so the cancellation escaped past every give-up below it: the row was already lost, the park never happened, and the key's cap reopened for the cost it had just been served. Treat it like the other arm's rollback failure and carry on to the give-up. The next attempt then still finds the session poisoned, so what lands is one park; what matters is that the settlement is accounted for rather than dropped.
`_insert_park` answered "not durable" to every failure except a unique violation, including a COMMIT that applied and lost its ack. The caller then stored the amount in `_unsettled` beside the row it had just written, another worker folded and deleted the row, and the stale memory copy re-filed as a new park under a `trace_id` that no longer collided — billing one delivery twice and leaving the key pinned on its cap with no debt left to fold. Ask the database what landed instead of guessing, and drop the memory hold for any row a fold actually billed. An unreadable park ledger folded into the total as zero, which dispatched a key whose cap was held shut only by parks this worker could not see. The two reads are not the same connection either: the counter rides the request's, the ledger opens a new one, so a checkout timeout hid every park while the request worked fine. `pending_parked_spend` now answers `None` for unknown and the pre-check maps unknown onto the cap. Oldest-debt-first was decided by `trace_id`, because `created_at` came from `CURRENT_TIMESTAMP` — one second wide on SQLite, and a recovered outage re-files a whole batch inside a single pre-check. The row stamps itself Python-side at sub-second resolution so the tiebreak is the rare path it is documented as.
A commit whose acknowledgement was lost, combined with a durability probe that also failed, parks an obligation whose charge is already in spent_microcents. Folding it re-charges the same delivery, and because the fold deletes the park row there is nothing left to correct it. The request-log row and the charge share one transaction, so the log row proves the charge landed: clear those parks without billing them, and drop the matching in-memory hold so a later pre-check cannot re-file them.
`test_parked_queue_drains_oldest_first_as_the_cap_opens` failed on
Windows: the column default stamps `created_at` from the wall clock, and
two back-to-back `datetime.now()` calls return the SAME value there
(the clock ticks every ~15.6ms), so two parks written moments apart tied
and fell through to the `trace_id` tiebreak. "t-new" sorted before
"t-old", the younger debt was billed first, and the fold trimmed the
older one instead. On ubuntu CI the stamps differ, the test passes, and
the failure mode stays invisible.
Pin the stamps in both tests so the assertions are about fold ordering
rather than the host, and verified the FIFO test still bites by reversing
the offsets: it then fails with exactly the original `{'t-old': 1500}`.
The ordering guarantee is real but weaker than the docstring claimed.
`trace_id` is a uuid4, so parks sharing a stamp break the tie arbitrarily
— no schema change here because nothing in the accounting depends on it:
the parked total is conserved either way and the counter still reaches
min(cap, spent + debt). Only which row is trimmed differs, so the
docstring now says so and a new test pins the invariant that holds either
way (total conserved, remainder stays visible, queue drains to empty).
…ssing-usage fail-closed on delivered content
The streaming commit retry loop detached each commit task and only
awaited it on the give-up path; when the task itself was cancelled
(loop teardown, direct cancel), the inner `except BaseException: pass`
swallowed it, so the delivered stream recorded no row, charged nothing,
and filed no park -- the cap reopened for exactly the key whose
settlement failed. Run the same `_give_up_settlement` probe-and-park as
every sibling arm before propagating.
The blocking fail-closed arm's fail-closed raise fired whenever `usage`
was absent -- including an empty 200 -- while the same delivery with
`usage: {0, 0}` correctly settled at 0. Gate the missing-usage
disjunct on delivered content so an empty response can never exhaust
the key.
Continuum-AI-Corp#161 simplified the pre-check to a plain counter read, which is correct there because nothing parks yet. This rung reintroduces budget_precheck precisely so a parked obligation is folded before the 429 decision, and routing the gate through read_spent would silently skip that: a key whose settlement failed would keep dispatching as if it had never spent anything, which is the entire state durable recovery exists to close. Also reconciles the blocking settlement tests with the unified decision function now in the tree: a usage-less blocking completion is priced from what it returned (floored at one microcent), never charged the remaining allowance, and the parked obligation a write outage leaves behind is that same amount.
Ported from Continuum-AI-Corp#161, where the arm that swallowed a cancelled detached commit task cost only a log row; enforcement is what makes it cost a charge, so the fix belongs on the rung where enforcement lands. This rung already had the stronger form: _give_up_settlement probes durability and parks the obligation, so the cost keeps counting even when the write can no longer land at all — which is why #161s shielded re-run of the write is not reproduced here, and why #161s regression test (which asserts the retry lands a row) is not either. test_streaming_commit_task_ cancellation_gives_up pins this path on this rung.
Rebasing onto Continuum-AI-Corp#161 brought its shielded-retry test along. It asserts a retried write lands a row, which is what Continuum-AI-Corp#161 does; here the cancelled commit task is probed and parked instead, which test_streaming_commit_task_cancellation_gives_up already pins. Carrying both would assert two different mechanisms for one arm.
… cost Two ways the blocking path mis-charged a budgeted key. budget_precheck folded parked spend on its own session while the request-scoped session's SQLite snapshot stayed pinned to the pre-fold counter, so the re-read returned the stale value and the 429 gate could miss an exhausted key. The same stale snapshot made the follow-up UPDATE rewrite committed spend back down, permanently erasing the folded park. End the request session's transaction before re-reading, and settle on a fresh session like the streaming path. The fail-closed raise also overwrote an authoritative cost: the adapter attaches _orca_meta.cost_usd independently of usage, so a response priced by litellm was replaced by the whole remaining allowance. Only raise when the cost is genuinely unknown. (cherry picked from commit b9bfc23)
fb30ddb to
a3c0c5a
Compare
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: 328 calls · 20.6M tokens · 96% 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
| fallback_model=row_values.get("model_requested"), | ||
| ) | ||
| actual = max(actual, charge) | ||
| row_values["cost_microcents"] = actual |
There was a problem hiding this comment.
🟠 P1 Gate the streaming cost-unknown arm on a genuinely unknown cost, as the blocking path now is
This commit's own fix (blocking path, line ~1713) added and not (response.get("_orca_meta") or {}).get("cost_usd") and not (log.cost_microcents or 0) so a response already priced by litellm is never replaced by the whole remaining allowance. The streaming path's _settlement_amount first branch — if not _countable_usage(agg_usage) — has no such guard: it unconditionally runs actual = max(actual, charge) with charge from _unmeasured_charge, which for a completed stream with policy=_REMAINING (the default when the engine auto-injected include_usage=True) returns max(0, cap - spent) — the full remaining lifetime allowance. If a stream's usage frame carries a real cost (cost_usd / _orca_meta.cost_usd) but no countable token key (the {"total_tokens": 123}-style upstream this code itself documents as real), _build_log_row's Tier-1 pricing already recorded an authoritative non-zero row_values["cost_microcents"], and this branch then overwrites it with the entire remaining budget, charging the key cap-minus-spent for one request whose cost was known and small. The same overwrite the commit fixed on the blocking path is still present on the streaming path.
This branch has not been deployed
2 participants
Orca-Code-Review — push 10
❌ 1 finding blocks merge
PR 4 of a 4-PR stack replacing #91. Builds on #160, #161, and #162 — review commit
4cdf36aonly (the diff below is cumulative until those merge). Top of stack: full feature, all green.Review scope: adapter-fault settlement fairness.
main's blocking-HTTPException status guard (refactor(chat): guard the blocking HTTPException arm so it can't log status 200 #159) that the stack's file snapshot had predated.OrcaCode Review: PASSED, no findings. Full suite: 488 unit + 262 integration passed; ruff clean.