feat(budget): spend accounting schema, migration and library [1/4] - #160
hasitpbhatt wants to merge 4 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: 174 calls · 5.1M tokens · 92% 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
hasitpbhatt
changed the title
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.
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: 153 calls · 5.7M tokens · 92% 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 commit: | ||
| await db.commit() | ||
| return False |
There was a problem hiding this comment.
🟠 P1 Wire the budget gate and charge into the request path, or the cap is never enforced and the seeded counter is permanently stale
The new budget subsystem is entirely inert in production. grep across app/ and packages/ shows the only references to is_exhausted/charge_budget/read_spent are the unit tests; no request path (middleware auth, key_validator, deps, execute_chat, native-protocol adapters) calls them, and nothing advances spent_microcents after boot. The existing equivalents this feature mirrors all run per request: the allowlist gate in execute_chat (chat.py:365) rejects before dispatch, validate_api_key checks is_active/revoked_at on every request, and the RequestLog writeback records each request's cost. The new "hard lifetime limit" does none of these: (1) a key with budget_limit_cents set is dispatched past its cap indefinitely — the spend limit the feature claims to enforce ("enforces ApiKey.budget_limit_cents", "the cap is a hard lifetime limit", "so the cap holds even under concurrent requests") is never applied; (2) the migration's every-boot seed (migrate.py:113, WHERE spent_microcents = 0) sets the counter once from historical logs, and since nothing charges after that, the counter is a frozen boot-time snapshot that diverges from actual spend the moment traffic flows — the module's own invariant "a log row and its charge are one commit, so this SUM is the lifetime counter" has no code that maintains it. An operator who sets budget_limit_cents believing this feature caps the key gets no limit and a counter that under-reports spend (for a capped key with history, spend is under-counted by everything after the last boot; for a key created post-boot with a budget and no history, the counter stays 0 forever). Fix: in execute_chat (and the native-protocol surfaces via log_status/execute_chat), before dispatch call is_exhausted when kc.budget_limit_cents is not None (scale by MICROCENTS_PER_CENT) and reject with 429/403; after the upstream call, call charge_budget(commit=False) in the same transaction that inserts the RequestLog and commit once, including on the streaming finalize path where the row is written (and, since chat.py's blocking path swallows commit errors, treat a failed commit as a lost charge and let the next boot's seed recover only while the counter is 0).
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: 169 calls · 6.4M tokens · 92% 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
This branch has not been deployed
1 participant
Orca-Code-Review — push 3
✅ no blocking findings
What this PR is
PR 1 of 4 replacing #91 (same feature, split for reviewability; #91 will be closed once this stack is up).
Scope: the budget schema and the accounting library, and nothing else. No request-path behavior changes — deliberately. This PR does not make
budget_limit_centsenforced, and it is not supposed to.spent_microcentscolumn, startup migration,packages.auth.spendlibrary + unit testsis_exhausted/charge_budgetintoexecute_chat— the enforcement itselfOn the open "dead code" P1
The review bot flagged
charge_budget/is_exhausted/read_spentas having zero callers inapp/and called that dead code. That is accurate about this diff and is the intended shape of the stack: a landable-but-unused library is how the schema and the atomic-charge semantics get reviewed on their own, before they change anyone's requests. The wiring exists and is up for review right now in #161, which adds the pre-dispatchis_exhaustedcheck and the single-transactioncharge_budget(commit=False)alongside theRequestLoginsert.Merging #160 first is safe: it only adds a column, a startup migration and a module nothing imports.
Included
spent_microcentscounter + BIGINT widening onApiKey; spend index onrequests_log.ensure_budget_columnsstartup migration (upgrade seeding from request history, concurrent-boot race guard), wired into lifespan.packages.auth.spend:charge_budget(single atomic UPDATE, never exceeds cap, clamps instead),read_spent,is_exhausted.Review state
OrcaCode Review posted CHANGES_REQUESTED with 1 P1 on push 1 — the stack-scope item discussed above. It is not currently passing, and this description previously claimed otherwise; corrected.
Tests: targeted unit suite green. Lint: ruff clean.