fix(billing): reserve-then-settle spend caps to close concurrency gap#799
fix(billing): reserve-then-settle spend caps to close concurrency gap#799devin-ai-integration[bot] wants to merge 5 commits into
Conversation
Co-authored-by: rohith500 <rohith500@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
PR author is not in the allowed authors list. |
|
Claude finished @devin-ai-integration[bot]'s task —— View job 🤖 Claude Code
Posted one advisory PR review (#pullrequestreview-4735207438) flagging six 3+ line comment blocks that could be trimmed without losing genuine WHY:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
|
||
| expires := time.Now().UTC().Add(p.TTL) | ||
| expiresTS := pgtype.Timestamptz{Time: expires, Valid: true} | ||
| month := utcMonthDate() |
There was a problem hiding this comment.
Reservation month differs from debit
High Severity
Spend caps are reserved against the UTC month from Go at arm time, but DebitOrgCredits still buckets org/user/key spend using Postgres NOW(). Long requests (up to 600s) can arm in one month, settle the hold on that month’s reserved_usd_micros, and record spend in the next month without a matching reservation there.
Reviewed by Cursor Bugbot for commit bbe828c. Configure here.
| if ok { | ||
| proxy.LogDispatchErrorClass(log, cls, armErr) | ||
| writeGeminiError(c, cls.Status, "RESOURCE_EXHAUSTED", cls.Message) | ||
| return |
There was a problem hiding this comment.
Gemini arm maps all errors quota
Low Severity
When ArmSpendReservations fails with a classified dispatch error, the Gemini handler always sets the JSON error.status to RESOURCE_EXHAUSTED instead of geminiErrorStatus(cls.Kind), unlike the proxy error path below. Billing-unavailable (503) responses can carry a quota-style status while the HTTP code is not 429.
Triggered by learned rule: New sentinel errors must be mapped in all API surface handlers
Reviewed by Cursor Bugbot for commit bbe828c. Configure here.
workweave-bot
left a comment
workweave-bot
left a comment
There was a problem hiding this comment.
Advisory only — comment-length nits. Won't block merge.
| // runSpendReservationSweep releases expired spend-cap reservations every | ||
| // minute. Stuck reserved incorrectly blocks spend, so this is much more | ||
| // aggressive than the session-pin GC. Every replica runs its own ticker; | ||
| // DELETE … RETURNING is idempotent across replicas. |
There was a problem hiding this comment.
| // runSpendReservationSweep releases expired spend-cap reservations every | |
| // minute. Stuck reserved incorrectly blocks spend, so this is much more | |
| // aggressive than the session-pin GC. Every replica runs its own ticker; | |
| // DELETE … RETURNING is idempotent across replicas. | |
| // runSpendReservationSweep releases expired spend reservations every minute. | |
| // Stuck reserved headroom incorrectly blocks spend; DELETE … RETURNING is idempotent across replicas. |
Was 4 lines; the session-pin comparison and "every replica" observation restate the code — the non-obvious WHY (stuck reservations = blocked headroom) fits in two.
| // ArmSpendReservations reserves all applicable spend caps in one transaction | ||
| // after identity is on ctx and before Proxy*. Returns a release func for | ||
| // defer (no-ops after DebitForInference settles). Nil billing is a no-op. | ||
| // Agent-shadow evaluation traffic skips entirely (no reservation, no gate), | ||
| // matching the billing middleware skips from #787 — synthetic eval must not | ||
| // consume production spend budget. |
There was a problem hiding this comment.
| // ArmSpendReservations reserves all applicable spend caps in one transaction | |
| // after identity is on ctx and before Proxy*. Returns a release func for | |
| // defer (no-ops after DebitForInference settles). Nil billing is a no-op. | |
| // Agent-shadow evaluation traffic skips entirely (no reservation, no gate), | |
| // matching the billing middleware skips from #787 — synthetic eval must not | |
| // consume production spend budget. | |
| // ArmSpendReservations reserves all applicable spend caps in one transaction | |
| // before Proxy*. Shadow eval skips entirely — synthetic traffic must not consume | |
| // production spend budget (parity with billing middleware, #787). |
Was 6 lines; lines 2-3 restated the signature; collapsed to three without losing the shadow-eval WHY.
| // checkUserMonthlySpendLimit is retained for unit tests and as a read-only | ||
| // helper. Request-path enforcement uses billing.ArmSpendReservations (combined | ||
| // reserve of org/key/user caps) in API handlers before Proxy*. |
There was a problem hiding this comment.
| // checkUserMonthlySpendLimit is retained for unit tests and as a read-only | |
| // helper. Request-path enforcement uses billing.ArmSpendReservations (combined | |
| // reserve of org/key/user caps) in API handlers before Proxy*. | |
| // checkUserMonthlySpendLimit is retained for unit tests only; request-path | |
| // enforcement moved to ArmSpendReservations in API handlers. |
Was 3 lines narrating the architecture; the WHY fits in two.
| // ReserveApplicableCaps reserves every applicable spend cap (org monthly, | ||
| // api-key lifetime, user monthly) in one transaction. Returns a SpendHold | ||
| // (possibly with empty IDs when no caps apply). Billing override skips all. |
There was a problem hiding this comment.
| // ReserveApplicableCaps reserves every applicable spend cap (org monthly, | |
| // api-key lifetime, user monthly) in one transaction. Returns a SpendHold | |
| // (possibly with empty IDs when no caps apply). Billing override skips all. | |
| // ReserveApplicableCaps reserves all applicable caps in one TX. Billing override skips all. |
Was 3 lines listing scope names (already in param names) and restating return type; one line keeps the non-obvious override note.
| // ArmSpendReservations reserves applicable caps and returns a release func | ||
| // for defer. Call after identity is resolved and before Proxy*. The release | ||
| // func no-ops after a successful DebitForInference (MarkSettled). |
There was a problem hiding this comment.
| // ArmSpendReservations reserves applicable caps and returns a release func | |
| // for defer. Call after identity is resolved and before Proxy*. The release | |
| // func no-ops after a successful DebitForInference (MarkSettled). | |
| // ArmSpendReservations reserves applicable caps and returns a defer-safe release func | |
| // that no-ops once MarkSettled is called by DebitForInference. |
Was 3 lines; "Call after identity is resolved and before Proxy*" narrates call order visible from context — trimmed to two.
| // utcNow is the clock for reservation month bucketing. Tests override it to | ||
| // prove Ensure/Bump/Insert share one Go-computed month even when Postgres | ||
| // NOW() would land in a different UTC month. |
There was a problem hiding this comment.
| // utcNow is the clock for reservation month bucketing. Tests override it to | |
| // prove Ensure/Bump/Insert share one Go-computed month even when Postgres | |
| // NOW() would land in a different UTC month. | |
| // utcNow is the clock for reservation month bucketing; tests override it so | |
| // Ensure/Bump/Insert share one Go-computed month even across a UTC midnight. |
Was 3 lines; second sentence restated the first — condensed to two without losing the non-obvious invariant.
|
Claude finished @devin-ai-integration[bot]'s task —— View job 🤖 Claude Code
Posted one advisory PR review (#pullrequestreview-4735243865) flagging three 3-line comment blocks that can be trimmed:
|
| } | ||
| if HasOverrideFromContext(ctx) { | ||
| return &SpendHold{}, nil | ||
| } |
There was a problem hiding this comment.
Override skips API key cap
Medium Severity
When a billing override is active, ReserveApplicableCaps returns an empty hold and never reserves the API-key lifetime cap. Inference routes no longer run WithAPIKeySpendCap, so capped keys on override orgs can keep serving until spent_usd_micros exceeds the cap without a gate.
Reviewed by Cursor Bugbot for commit 1df6581. Configure here.
workweave-bot
left a comment
workweave-bot
left a comment
There was a problem hiding this comment.
Advisory only — comment-length nits. Won't block merge.
| // DefaultReserveAmountMicros is the v1 fixed reservation slot ($1). Tuned as a | ||
| // p95-ish turn cost approximation — not exact. Override via | ||
| // ROUTER_SPEND_RESERVE_USD_MICROS. |
There was a problem hiding this comment.
| // DefaultReserveAmountMicros is the v1 fixed reservation slot ($1). Tuned as a | |
| // p95-ish turn cost approximation — not exact. Override via | |
| // ROUTER_SPEND_RESERVE_USD_MICROS. | |
| // DefaultReserveAmountMicros is the v1 fixed reservation slot ($1) — p95-ish turn cost, | |
| // not exact. Override via ROUTER_SPEND_RESERVE_USD_MICROS. |
Was 3 lines; collapses to two without losing the tuning rationale.
| // ReserveSpendCaps atomically reserves every applicable scope in one | ||
| // transaction. On any scope failure the whole TX rolls back (no partial | ||
| // holds). Returns reservation ids (possibly empty when no caps apply). |
There was a problem hiding this comment.
| // ReserveSpendCaps atomically reserves every applicable scope in one | |
| // transaction. On any scope failure the whole TX rolls back (no partial | |
| // holds). Returns reservation ids (possibly empty when no caps apply). | |
| // ReserveSpendCaps atomically reserves every applicable scope in one TX; | |
| // any scope failure rolls back the whole TX (no partial holds). |
Was 3 lines; the "Returns reservation ids" sentence restates the signature — dropped.
| // ReleaseSpendReservations consumes reservation ids via DELETE … RETURNING | ||
| // and decrements denormalized reserved only when a row was returned. | ||
| // Idempotent: already-gone ids are no-ops. |
There was a problem hiding this comment.
| // ReleaseSpendReservations consumes reservation ids via DELETE … RETURNING | |
| // and decrements denormalized reserved only when a row was returned. | |
| // Idempotent: already-gone ids are no-ops. | |
| // ReleaseSpendReservations deletes reservation ids and decrements denormalized | |
| // reserved. Idempotent: already-gone ids are no-ops. |
Was 3 lines; collapsed to two.
|
Claude finished @devin-ai-integration[bot]'s task —— View job 🤖 Claude Code
Posted one advisory PR review (#pullrequestreview-4735276279) flagging one 3-line comment block:
All other added comment blocks in the diff were 2 lines or fewer (the earlier 3+ line blocks were addressed by commits |
workweave-bot
left a comment
workweave-bot
left a comment
There was a problem hiding this comment.
Advisory only — comment-length nit. Won't block merge.
workweave-bot
left a comment
workweave-bot
left a comment
There was a problem hiding this comment.
Advisory only — comment-length nit. Won't block merge.
| // ErrOrgMonthlySpendLimitReached is returned when the org's current UTC-month | ||
| // spend (including in-flight reservations) cannot fit another reserve slot. | ||
| // Mapped to HTTP 402. |
There was a problem hiding this comment.
| // ErrOrgMonthlySpendLimitReached is returned when the org's current UTC-month | |
| // spend (including in-flight reservations) cannot fit another reserve slot. | |
| // Mapped to HTTP 402. | |
| // ErrOrgMonthlySpendLimitReached is returned when the org's current UTC-month | |
| // spend (including in-flight reservations) cannot fit another reserve slot. Mapped to HTTP 402. |
Was 3 lines; "Mapped to HTTP 402." fits on line 2, matching the adjacent ErrUserMonthlySpendLimitReached pattern.
|
Claude finished @devin-ai-integration[bot]'s task —— View job 🤖 Claude Code
Posted one advisory PR review (#pullrequestreview-4735322230) flagging one 3-line comment block:
All other added comment blocks in the diff were 2 lines or fewer (the earlier 3+ line blocks were addressed by commits |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 4 total unresolved issues (including 3 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4701229. Configure here.
| safeGo(logger, "spend-reservation-sweep", func() { | ||
| runSpendReservationSweep(context.Background(), billingSvc) | ||
| }) | ||
| logger.Info("Spend reservation TTL sweeper enabled", "interval", "1m") |
There was a problem hiding this comment.
Billing boot check misses reservations
Medium Severity
The PR adds reserve-then-settle paths that require migration 0040_spend-reservations, but CheckBillingTablesExist still only verifies the seven pre-0040 billing tables. Managed boot can enable billing and start the reservation sweeper while 0040 is missing, so ArmSpendReservations hits SQL errors and inference routes fail closed with 503 until the migration is applied manually.
Reviewed by Cursor Bugbot for commit 4701229. Configure here.
workweave-bot
left a comment
workweave-bot
left a comment
There was a problem hiding this comment.
Advisory only — comment-length nit. Won't block merge.
| // ArmSpendReservations reserves all applicable spend caps in one transaction | ||
| // before Proxy*. Shadow eval skips entirely — synthetic traffic must not consume | ||
| // production spend budget (parity with billing middleware, #787). |
There was a problem hiding this comment.
Was 3 lines; fits in two without losing the shadow-eval WHY.
workweave-bot
left a comment
workweave-bot
left a comment
There was a problem hiding this comment.
Advisory only — comment-length nit. Won't block merge.
| // ArmSpendReservations reserves all applicable spend caps in one transaction | ||
| // before Proxy*. Shadow eval skips entirely — synthetic traffic must not consume | ||
| // production spend budget (parity with billing middleware, #787). |
There was a problem hiding this comment.
| // ArmSpendReservations reserves all applicable spend caps in one transaction | |
| // before Proxy*. Shadow eval skips entirely — synthetic traffic must not consume | |
| // production spend budget (parity with billing middleware, #787). | |
| // ArmSpendReservations reserves applicable spend caps before Proxy*. | |
| // Shadow eval skips — synthetic traffic must not consume production budget. |
Was 3 lines; "in one transaction" and the #787 cross-reference add words without adding WHY that isn't already in context — fits in two.
|
Claude finished @devin-ai-integration[bot]'s task —— View job 🤖 Claude Code
All previously flagged blocks were addressed by commits |
2 participants
Summary
Compliant rewrite of #795 by @rohith500 (fixes #793) — production logic, migrations, and SQLC queries preserved verbatim; only the DB-backed tests were relocated to comply with repo conventions.
Spend caps (org monthly, user monthly, api-key lifetime) had a check-then-act race: concurrent requests could all pass the preflight
SELECTbefore any debit landed, overshooting a hard cap by a multiple of the limit. This replaces the gate with reserve-then-settle:billing.Service.ArmSpendReservationsreserves all applicable caps once per request (after identity, before dispatch) in one Postgres TX; each scope is an atomic conditionalUPDATE ... SET reserved = reserved + R WHERE spent + reserved + R <= limit RETURNING id— zero rows maps to the scope'sErr*SpendLimitReached/Err*SpendCapReachedsentinel.DELETE FROM spend_reservations WHERE id = @id RETURNING ..., decrementing the denormalizedreserved_usd_microsonly when a row returns. Settle happens in the same TX asDebitInference; a defer-based release covers early exits; a 1-minute TTL sweeper (runSpendReservationSweep, TTL = request timeout + 5m) covers crashes.ROUTER_SPEND_RESERVE_USD_MICROS, default $1;ROUTER_SPEND_RESERVE_TTL, default 15m). Worst-case overshoot isadmitted * max(0, actual - R)withadmitted <= floor(limit/R)— bounded, no longer scaling with concurrency.Delta from #795: the three live-Postgres
*_test.gofiles underinternal/postgres/were moved to a runnable script,scripts/spend_reservation_check/(concurrency regression, Go-computed-month consistency, TTL sweep), per the root AGENTS.md rule that DB-backed integration checks live underscripts/rather thaninternal/test files.make precommitandmake generatepass clean.Credit: original design, implementation, and investigation by @rohith500 in #795.
Link to Devin session: https://app.devin.ai/sessions/e5f91192d0f64062b5113b1c94b5928f
Requested by: @steventohme