feat(budget): park lost settlements durably and fold them at pre-check - #162
hasitpbhatt wants to merge 20 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.
Not attached to a line. GitHub only accepts an inline comment on a line this PR changes. The findings below point somewhere else, so they are listed here instead of being dropped.
app/routes/chat.py (line 1154): 🟠 P1 Settle the AdapterError branch like the sibling provider-error branch so a budgeted key isn't charged its entire remaining allowance for an internal adapter fault
The new budget settlement keys the fail-closed "charge the full remaining allowance" rule on usage_seen being False (see _settlement_amount, lines 935-953). The sibling except Exception branch (lines 1155-1205) explicitly sets agg_usage = _settle_unmeasured_stream(...) and usage_seen = True before the finally runs _finalize(), so a mid-stream provider failure is charged only the delivery estimate — its own comment says the alternative "would charge (and exhaust) the key's entire remaining budget" for every transient failure. The except AdapterError: branch (an internal protocol-adapter bug — "our own fault, not the caller's") does NOT do this: it just closes the stream and returns, leaving usage_seen False. The finally then runs _finalize(), whose _settlement_amount() computes max(recorded_cost≈0, cap - spent_snapshot) = the entire remaining allowance, and charge_budget clamps spent_microcents to the cap. So a single AdapterError on a budgeted key — even one that delivered nothing — permanently exhausts the key and bills the customer's whole remaining budget for our own bug. This contradicts the explicit design intent expressed in the sibling branch and in test_budgeted_stream_midstream_error_charges_actual_only. Fix: before return in the AdapterError branch, add agg_usage = _settle_unmeasured_stream(agg_usage, agg_output_chars, body) and usage_seen = True, mirroring the except Exception branch.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 269 calls · 15M 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
The upgrade seed is a correlated SUM over requests_log, and the index that would serve it was created further down the same function — so the one boot that runs the seed was also the one that could not use the index, and later boots do neither. Build the index first and pin the order with a test. Also states the two contracts the schema leaves implicit: the seed counts soft-deleted request rows on purpose, because restoring an accrued lifetime total can only ever tighten a cap, and cap_microcents is budget_limit_cents scaled to microcents rather than the column itself.
The seed ran only inside the branch that added the column, and the two statements do not vouch for each other: on SQLite the ALTER is durable the instant it executes while the seed is DML in the transaction a kill — or the `database is locked` this very aggregate provokes on an upgrade that overlaps the old machine's writes — rolls back. Gating on the column's absence made that half-applied boot the only one that could ever have seeded, so every key predating the release kept a full fresh allowance forever, silently, which is exactly the outcome the seed exists to prevent. It runs on every boot now, restricted to keys that hold a cap, and the statement is idempotent because a log row and its charge are one commit — a key already holding spend has nothing to restore. Three more from the same review: - Gate the Postgres BIGINT widen on the reflected type rather than the column's name, which was present forever and so took ACCESS EXCLUSIVE on api_keys at every start. - Correct the model comment justifying that widen with a client-supplied budget no route accepts, in the wrong unit. - Make the upgrade tests able to fail: the legacy fixture had one key and one log row, so a seed that dropped its correlation predicate and stamped every key with the table total passed. It now has three keys with three histories, pins the half-applied boot above, and the concurrency test runs over a file instead of `:memory:`'s StaticPool, where two "independent" sessions shared one connection and the atomic guard never met a concurrent writer.
47cdc8b to
ddacb57
Compare
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 1 issue in this PR: 🟠 1 P1.
Not attached to a line. GitHub only accepts an inline comment on a line this PR changes. The findings below point somewhere else, so they are listed here instead of being dropped.
app/routes/chat.py (line 1164): 🟠 P1 AdapterError mid-stream leaves usage_seen False and exhausts the key's entire remaining budget
The new fail-closed settlement (_settlement_amount, lines 935-966) charges the full remaining allowance (cap - kc._budget_spent) whenever a stream ends with usage_seen == False. The cancel branch (line 1132) and the provider-error branch (line 1215) both mark the delivery as known — agg_usage = _settle_unmeasured_stream(...) + usage_seen = True — before _finalize() runs. The except AdapterError branch (lines 1146-1164) does neither: it closes the upstream and returns, so _finalize() runs with usage_seen == False and agg_usage == {}. _settlement_amount() then raises row_values["cost_microcents"] to the key's whole remaining allowance and charge_budget moves the counter by that amount, permanently exhausting a budgeted key on our own adapter bug. This is exactly the defect the commit fixed for the provider-error branch (its test comment: "Before the fix, usage_seen stayed False in that branch and every transient provider failure permanently exhausted the key (charged cap - spent)") and for client disconnects; an AdapterError is the same class of mid-stream failure whose error response IS delivered to the client (the adapter emits a native error event), so per the commit's own policy it should be priced from _settle_unmeasured_stream(agg_usage, agg_output_chars, body) and marked known. Fix: in the AdapterError branch, before return, add agg_usage = _settle_unmeasured_stream(agg_usage, agg_output_chars, body); usage_seen = True (mirroring the except Exception branch).
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 276 calls · 15.6M 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
…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.
…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.
ddacb57 to
eac6f2a
Compare
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 2 issues in this PR: 🟠 2 P1.
Not attached to a line. GitHub only accepts an inline comment on a line this PR changes. The findings below point somewhere else, so they are listed here instead of being dropped.
app/routes/chat.py (line 1168): 🟠 P1 Settle the AdapterError branch before returning so a budgeted key is not charged its full remaining allowance
The except AdapterError: branch (a mid-stream failure of the proxy's own SSE adapter, after content has already been forwarded) returns without calling _settle_unmeasured_stream and without setting usage_seen = True, so _finalize → _settlement_amount() runs with usage_seen still False and charges max(actual, cap - spent) — the key's entire remaining lifetime allowance — and records that amount on the row. The two sibling failure branches in this same commit were explicitly refined to avoid exactly this: the except Exception provider-error branch (line 1197) and the cancel branch (line 1129) both settle with _settle_unmeasured_stream and mark usage_seen = True first, and the test test_budgeted_stream_midstream_error_charges_actual_only documents that leaving usage_seen False "permanently exhausted the key (charged cap - spent)" and calls that a bug. An adapter fault (our own bug, e.g. the malformed-chunk case in tests/integration/test_adapter_failure_attribution.py) hits the identical pattern: a budgeted key's whole remaining allowance is consumed for a few delivered tokens the caller cannot steer — the exact over-bill _settle_unmeasured_stream's docstring says must not happen. The adapter-fault integration tests use uncapped keys, so this path is untested. Fix: before return, mirror the generic branch — agg_usage = _settle_unmeasured_stream(agg_usage, agg_output_chars, body) and usage_seen = True — so _finalize prices the delivered content instead of the full remaining cap.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 272 calls · 15.2M 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
| .where(ApiKey.id == key, ApiKey.spent_microcents == spent) | ||
| .values(spent_microcents=spent + move) | ||
| ) | ||
| if charged.rowcount != 1: |
There was a problem hiding this comment.
🟠 P1 Fold parked obligations without cross-checking requests_log, double-billing a delivery whose charge already landed
settle_parked_spend bills every BudgetPark row for the key into spent_microcents with no check against requests_log. The park's trace_id is the settlement's trace_id — the same key the request-log row carries — so requests_log can prove whether the charge already landed. The chain that reaches this without three faults: (1) the last commit attempt in _commit_row/the blocking retry loop APPLIES (INSERT requests_log + UPDATE api_keys SET spent_microcents commit atomically) but loses its ack, raising; (2) _give_up_settlement (app/routes/chat.py:116) asks _durable() and that probe fails — a DB blip makes _settlement_is_durable catch Exception and return False, or a cancellation lands mid-probe and the except asyncio.CancelledError arm parks unconditionally (chat.py:126-132) — so record_unsettled_spend parks the amount (durably, or in _unsettled when the park write also fails); (3) on the next pre-check for the same key, budget_precheck → settle_parked_spend re-files the memory hold and folds the row into the counter. The delivery is then charged twice: once by the landed commit, once by the fold; the ledger shows one requests_log row but spent_microcents moved by 2×. The module docstring claims this double charge needs "three faults at once … another worker folding the row before this one retries", but the memory-hold path is folded by the same process's next pre-check, so the two faults the code is explicitly built to absorb (ack loss + probe failure/cancellation) suffice. The integration tests do not cover this: the ack-loss tests' probe works and asserts pending_parked_spend == 0; the write-outage test's commit genuinely never lands (rowcount 0), so its fold is not a double charge. Fix: in settle_parked_spend, drop a park row without billing when EXISTS (SELECT 1 FROM requests_log WHERE requests_log.trace_id = BudgetPark.trace_id) (the row and the charge are one transaction, so the row's presence proves the charge landed) — apply the same guard when re-filing _unsettled memory holds; and in _give_up_settlement's cancellation arm, probe durability instead of parking blind before propagating.
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.
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 1 issue in this PR: 🟠 1 P1.
Not attached to a line. GitHub only accepts an inline comment on a line this PR changes. The findings below point somewhere else, so they are listed here instead of being dropped.
app/routes/chat.py (line 1164): 🟠 P1 Mark the settlement known in the AdapterError branch, or a budgeted key is charged its full remaining allowance on an adapter fault
The streaming generator's failure branches all settle the delivery before _finalize runs: the cancel branch and the provider-error branch both call _settle_unmeasured_stream(...) and set usage_seen = True (lines 1132, 1198), precisely so _finalize's _settlement_amount() (line 952: if not usage_seen: actual = max(actual, cap - _budget_spent)) charges only the delivered estimate instead of the whole remaining allowance. The except AdapterError: branch (an adapter bug mid-stream — the deployment's own fault, not the client's and not the provider's) sets neither: it just sets status/error_type, closes the stream and returns. The finally (line 1210) then runs _finalize() → _commit_row() → _settlement_amount() with usage_seen still False, so the row records cap - kc._budget_spent (the key's FULL remaining allowance, e.g. 100_000 microcents for a fresh 10-cent key) and charge_budget moves the counter to the cap — the key is maxed out by a single adapter fault that delivered nothing or a few chunks, where the sibling provider-error branch would charge ~0 (empty delivery) or the character estimate. Fix: mirror the sibling branches before the return — agg_usage = _settle_unmeasured_stream(agg_usage, agg_output_chars, body) then usage_seen = True.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 237 calls · 13.3M 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 1 issue in this PR: 🟠 1 P1.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 289 calls · 18M 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
| settle_amount = max( | ||
| log.cost_microcents or 0, | ||
| kc._budget_cap - (getattr(kc, "_budget_spent", 0) or 0), | ||
| ) |
There was a problem hiding this comment.
🟠 P1 Charge a delivered-but-unpriceable completion the full remaining allowance, not 0
The fail-closed rule ("a delivered completion can never cost nothing") triggers only on not response.get("usage") (blocking path, lines 1328-1336) and on not usage_seen (streaming _settlement_amount, lines 951-958). It assumes "usage frame present ⇒ cost known and non-zero". Nothing guarantees that: _compute_cost_microcents (line 369+) returns 0 when LiteLLM's response_cost is absent (custom/BYOK upstreams, _orca_meta absent) AND the model isn't in CATALOG — the normal case for this router's own BYOK use case. A budgeted key (budget_limit_cents set, default allow-all allowlist) pointed at such a model gets a usage frame, so usage_seen=True/response.get("usage") is truthy, the fail-closed charge is skipped, settle_amount = log.cost_microcents = 0, and spent_microcents never moves. The 429 gate (spent >= cap, line 640) never trips, so the lifetime cap is bypassed entirely while the operator keeps paying the upstream for the client's unbounded use. Fix: when the cost computes to 0 on a successful delivered response for a budgeted key, treat the cost as unknown and charge the remaining allowance (or at least require cost_microcents > 0 to be treated as known) in both the blocking gate and _settlement_amount.
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: 237 calls · 13.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
| - (getattr(kc, "_budget_spent", 0) or 0), | ||
| ) | ||
| row_values["cost_microcents"] = actual | ||
| return actual |
There was a problem hiding this comment.
🟠 P1 Gate the streaming "tokens but no price" fail-closed on a real completion, like the blocking path does
d26d14f added an elif to _settlement_amount() that charges a budgeted key the FULL remaining allowance whenever the row has nonzero tokens and the model has no known price (_has_known_price False — a custom upstream or a model absent from CATALOG). Unlike the blocking path, which gates its equivalent rule on status_code < 400 and isinstance(response, dict) and response (a genuinely delivered completion), the streaming elif has no status/error gate. The error branches (mid-stream upstream failure, status 503; adapter error, 500; client disconnect, 499) each call _settle_unmeasured_stream(...) to price the delivered content, set usage_seen = True, and then finally runs _finalize(). Because the estimate always contains prompt_tokens >= 1 (and completion_tokens >= 1 whenever any content was forwarded, or even on an empty bail via caller_bailed=True), the elif fires for any of those failure branches on an unpriceable model, and actual = max(0, cap - spent) charges the key's entire remaining lifetime budget and records that amount on the log row. Consequences: a transient mid-stream upstream failure, an adapter bug, or a client hangup on a non-catalog/custom-upstream model permanently exhausts the key (429 for all future requests) and books spend that never occurred — exactly what the three branch comments ("settling first ensures _finalize never charges the key's full remaining allowance...", "price what reached the client... instead of charging a budgeted key its entire remaining budget", "unknown is not licence to bill the whole remaining allowance for a few sentences the user chose to stop reading") and test_budgeted_stream_midstream_error_charges_actual_only (which only passes because gpt-4o-mini is catalog-priced) say must not happen. The blocking sibling charges ~0 for the same upstream error. Fix: make the elif require a normal completion (e.g. status_code == 200 and error_type is None, or track whether the usage came from a real usage frame rather than the error branches' estimate) — the not usage_seen branch already fail-closes a 200 stream whose provider omitted usage.
| pending = await pending_parked_spend(key) | ||
| if pending is None: | ||
| return cap_microcents | ||
| return spent + pending |
There was a problem hiding this comment.
🟠 P1 budget_precheck's first read can be stale when a concurrent fold empties the park queue, understating the spend and bypassing the 429 gate
budget_precheck reads spent through the request-scoped session db (line 353) before consulting the park ledger. On SQLite the request session's read transaction snapshot was pinned earlier (key validation's SELECT, or router_cache.get_router(db)'s SELECTs on a cold start), and on READ COMMITTED... on SQLite the snapshot stays pinned until a commit/rollback. The fix in eac6f2a added db.rollback() before the SECOND read (line 373) but only inside if pending:. If a concurrent request/worker's settle_parked_spend commits a fold — moving a parked obligation into spent_microcents and deleting the park row — in the window between the request session's snapshot-establishing read and line 353, then line 353 returns the pre-fold counter while line 354 (pending_parked_spend, a fresh factory session) returns 0. With pending == 0 the rollback-refresh never runs, so the function returns the stale spent (understated by the folded amount). A key whose true spend + pending is at/over the cap is then dispatched instead of 429'd — the hard-cap gate is bypassed by exactly the amount another worker just folded. This is the same snapshot-staleness class eac6f2a fixed for the same-precheck fold, left open for the concurrent-other-worker fold. Narrow (SQLite, concurrent fold, queue emptied) but it is a cap-enforcement check returning a wrong (too-low) value.
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: 268 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
| settle_amount = max( | ||
| log.cost_microcents or 0, | ||
| kc._budget_cap - (getattr(kc, "_budget_spent", 0) or 0), | ||
| ) |
There was a problem hiding this comment.
🟠 P1 Blocking fail-closed charge fires for a zero-token empty completion, unlike the streaming twin
The blocking path's cost-unknown gate lacks the token>0 guard that the streaming path's _settlement_amount has. Streaming (lines 989-994): fail-closed only when not actual AND (input_tokens or output_tokens) AND not _has_known_price(...) — an empty delivery (usage frame with 0 tokens) from a model absent from the catalog settles at its recorded 0 ("an empty delivery ... is known-zero", per the comment). Blocking (lines 1368-1382): for response whose usage is present but 0/0 and whose model is not in the catalog (custom upstream LiteLLM), the second OR arm is not (log.cost_microcents or 0) and not _has_known_price(...) — both true — so the gate fires and settle_amount = cap - kc._budget_spent, i.e. the key's ENTIRE remaining allowance is charged (counter clamped to cap, row records the full amount) for a completion that delivered zero tokens. The same delivery on the streaming path charges 0. A single empty completion from an unknown model therefore exhausts a budgeted key's lifetime cap and 429s everything after it, with a cost on the row nothing accounts for. The mirror rule the blocking path claims ("Mirroring the streaming rule", test_budgeted_blocking_without_usage_charges_remaining) requires the token guard. Fix: add and ((response.get("usage") or {}).get("prompt_tokens") or (response.get("usage") or {}).get("completion_tokens")) to the second arm (mirroring streaming), so only a usage-less OR token-bearing unpriceable delivery fails closed.
| trace_id=trace_id, api_key_id=key, microcents=amount | ||
| ): | ||
| _unsettled.pop((held_key, trace_id), None) | ||
| move = 0 |
There was a problem hiding this comment.
🟠 P1 Re-filing a memory hold after a concurrent fold deleted the row bills the same delivery twice
When record_unsettled_spend's _insert_park commit is applied but its ack is lost AND the _park_is_durable probe also fails, the obligation exists BOTH as a durable budget_parks row and as a process-local _unsettled[(key, trace)] hold. If another worker's settle_parked_spend then folds the durable row — UPDATE api_keys SET spent = spent + A WHERE spent == old (no cap guard needed; room was computed from its own read), DELETE the row — and this worker's next pre-check runs, settle_parked_spend re-files the hold: _insert_park now finds no row (it was deleted) and INSERTs a brand-new park row for the same trace_id, pops the hold, then folds that new row and moves spent by A a second time. Concretely: cap=10000, spent=0, one delivered cost A=3000. Worker B folds the lost-ack row: spent→3000, row deleted. Worker A (holding the memory copy) re-files and folds: CAS UPDATE ... WHERE spent == 3000 matches, spent→6000. The key is charged 6000 for one 3000 delivery while below its cap, exhausting it 3000 early. The code's own comment at lines 314-318 describes this exact mechanism, and budget_park.py's docstring dismisses it on the ground that "the extra charge lands on a key that had already breached its cap" — that premise is wrong: the fold's CAS matches any current spent, so the second charge lands on a key that is below cap. Fix (the tombstone the author considered): when a fold bills a row, leave a durable marker keyed by trace_id (e.g., rewrite the row to microcents=0 / a billed flag instead of deleting) so the re-file always collides with the unique trace_id and _insert_park reports already-durable; without it the re-file inserts a fresh row and the same delivery is charged twice.
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: 249 calls · 14.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
| "UPDATE api_keys SET spent_microcents = (" | ||
| " SELECT CAST(COALESCE(SUM(cost_microcents), 0) AS BIGINT) FROM requests_log " | ||
| " WHERE requests_log.api_key_id = api_keys.id" | ||
| ") WHERE spent_microcents = 0 AND budget_limit_cents IS NOT NULL" |
There was a problem hiding this comment.
🟠 P1 Make the lifetime-spend seed monotonic so legacy rows committed during the rollout overlap are not lost forever
The seed UPDATE api_keys SET spent_microcents = (SELECT SUM(cost_microcents) FROM requests_log ...) WHERE spent_microcents = 0 AND budget_limit_cents IS NOT NULL runs at every boot, but the spent_microcents = 0 gate makes it one-shot per key: the first boot after upgrade snapshots the SUM at that instant. The comment above the statement itself anticipates "an upgrade that overlaps the old machine's writes" — the previous release keeps serving against the same database and keeps committing requests_log rows with real cost_microcents. Any such row that commits after the boot's seed snapshot is absent from the SUM, and the moment the new release charges even one request for that capped key (charge_budget), spent_microcents leaves 0, so every later boot's seed skips the key and the late legacy row is never counted. The key's lifetime spend is then permanently understated by the amount the old release recorded during the overlap, and the hard cap (which is only enforced against this counter) lets the key serve past its true lifetime spend. The re-seed-every-boot mechanism only repairs a seed that rolled back, not rows that arrive after a successful seed. Fix: make the seed monotonic instead of gated on exactly zero — e.g. SET spent_microcents = max(spent_microcents, (SELECT CAST(COALESCE(SUM(cost_microcents),0) AS BIGINT) FROM requests_log WHERE requests_log.api_key_id = api_keys.id)) WHERE budget_limit_cents IS NOT NULL — since every new charge also writes a log row, the SUM is always >= the counter, so the max tops the counter up with late legacy rows without double-counting and never moves it down.
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: 283 calls · 16.5M 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
|
|
||
| limit_type = cols.get("budget_limit_cents") | ||
| if is_postgres and limit_type is not None and not isinstance(limit_type, BigInteger): | ||
| await _apply_ddl( |
There was a problem hiding this comment.
🟠 P1 Seed UPDATE that races a live budget charge permanently drops the key's historical spend
The boot seed UPDATE api_keys SET spent_microcents = (SUM of requests_log cost) WHERE spent_microcents = 0 AND budget_limit_cents IS NOT NULL is a read-modify-write with no guard against concurrent charges. In a rolling/multi-worker deployment the app serves traffic while another worker runs this (the comment about overlapping "the old machine's writes" acknowledges exactly that). If a request's log+charge commit lands before the seed statement's snapshot on Postgres, spent_microcents is already 20, the WHERE spent_microcents = 0 fails for that key, and the seed skips it — leaving the counter at 20 while the true lifetime total is 120. The gate spent_microcents = 0 means every later boot also skips the key, so the historical 100 is never restored: the key is granted budget headroom equal to its entire pre-upgrade lifetime spend, permanently understating the cap (a capped key can spend its historical budget again). The comment argues the seed is idempotent ("the SUM is the lifetime counter"), which holds only if the counter starts from the seed; the race breaks that invariant and nothing repairs it. Fix: seed with the WHERE clause evaluated against the same row version the SUM came from, e.g. run the per-key restore as UPDATE ... WHERE spent_microcents = 0 inside a transaction that also locks/guards the row, or re-run the SUM for a key whenever its counter is behind the SUM (compare-and-set on the computed total), or seed only while the app is guaranteed quiescent.
This branch has not been deployed
1 participant
Orca-Code-Review — push 9
❌ 1 finding blocks merge
PR 3 of a 4-PR stack replacing #91. Builds on #160 and #161 — review commit
ddacb57only (the diff below is cumulative until those merge).Merge order: do not merge this before #163. The
except AdapterError:branchbelow returns without settling, so
_finalizeseesusage_seen=Falseand_settlement_amountcharges a budgeted key its entire remaining lifetime budgetfor a server-side adapter fault. This is known and owned by #163 (
4cdf36a),which settles an adapter fault like the provider-error branch instead — it is
listed here so it does not read as an unfixed gap in this PR's own scope, and it
needs no separate fix.
Review scope: durable recovery — a lost settlement keeps counting against the cap across restarts and workers.
budget_parkstable (one row per settlement, keyed bytrace_id, created by the startup migration): park writes are idempotent, so an ack-lost commit retries into the same key instead of double-billing.spent_microcentsin one CAS-guarded transaction (charge + row deletes); concurrent folders cannot double-bill, and over-remainder parks stay visible while keeping the key exhausted.OrcaCode Review: PASSED, no findings. Tests: budget unit + integration green; ruff clean.
Known gap, pre-existing and deliberately out of scope here:
app/routes/chat.py:1062— theexcept BaseException: passthat followsawait commit_taskon the streaming teardown. A secondCancelledErroraimed atthat await rather than at the commit leaves the settlement neither charged nor
parked, and the arm swallows it. Blamed to
mainat9021c8f; this stack neitherintroduces nor touches it.