Repository navigation
fix(permits,state): enforce expiry on the second reservation path; make budget release once-only without a row lock; refuse SQLite in production - #304
Conversation
…ath; make dispatch-budget release once-only without a row lock; refuse SQLite in production Three fixes from the audit findings #302 listed as not addressed. Each was verified against current main and each ships a regression test confirmed to fail against the unfixed source. 1. reserve_budget spent against expired permits. #298 added the expiry predicate to authorize_and_reserve's guarded update, but reserve_budget is a second reservation path and did not get it. An expired permit keeps status="active" in storage (expiry is a dynamic check, not a stored state), so the WHERE clause matched and budget moved. Adds the same expires_at > now term, plus precise denial reasons (permit_expired / permit_not_active) instead of reporting every miss as budget_exceeded. Engine-independent: this was wrong on PostgreSQL too. Test: an active permit whose expires_at is moved into the past is refused with permit_expired and spent_credits stays 0. Against the unfixed source the reservation succeeds (DID NOT RAISE). 2. release_dispatch_budget_once was not once-only. It read budget_released_at under with_for_update=True and then decremented the permit. SQLAlchemy silently drops FOR UPDATE on engines that do not support it, so two concurrent callers both observed NULL and both released — the permit was credited back twice for one dispatch. The claim is now a guarded UPDATE (NULL -> now, rowcount == 1 wins); only the winner decrements. The decrement itself was already atomic; it was the claim that was not. Test: two concurrent releases must yield exactly one True and exactly one decrement. Against the unfixed source both return True. 3. SQLite was a permitted production durable-state backend. STATE_BACKEND=memory was refused in production-like environments but sqlite was not, and auto-resolution would select it from a SQLite DATABASE_URL — the ordinary local-development setting. That is the deployment-level enabler for every remaining FOR UPDATE-is-a-no-op finding in the money path. Both the explicit and the auto path now fail closed. Tests: production-like + STATE_BACKEND=sqlite with SQLITE_URL set, and auto-resolution from a sqlite DATABASE_URL, both raise DurableStateConfigError. test_sqlite_without_url_refuses_production_like previously asserted the refusal reason was the missing SQLITE_URL, which implied a configured URL would be accepted. Its assertion is updated to the stricter reason; the behaviour it guards (production refuses this config) is unchanged. Also: the docs sweep in test_onboarding_contract.py walked .claude/worktrees/, a gitignored path holding full checkouts, so a copy of an already-excluded file failed the sweep for anyone using a git worktree there. Excluded .claude/. Tests: 1711 passed, 32 skipped; production_trust 6 passed; ruff clean; mypy clean across 172 source files; demo and red-team trust-plane gates --assert clean. Not addressed here: the remaining read-modify-write findings (wallet balance, reclaim_child_wallet double credit, refund reconciliation clobbering reservations, velocity counters) and the local-governed reconciler crash window. Fix 3 removes their production reachability; converting them to guarded updates is separate work. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FsXYfTHDLiFNQ24R5JPN7H
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_b900326e-f9d5-4055-84ba-01365f4e1c72) |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
#304 landed three fixes that overlap this branch: the reserve_budget expiry predicate, the once-only dispatch-budget release, and refusing SQLite as a durable-state backend in production. Resolved as follows, on the merits rather than by branch preference: release_dispatch_budget_once -- took main's version wholesale. It keeps the state pre-check that yields the precise dispatch_budget_release_state_invalid error and adds the guarded claim as the authoritative gate. That is correct and preserves error semantics; my variant folded state into the predicate, which is not clearly better, so there is no reason to churn landed code. Kept both tests: main's uses real concurrency via asyncio.gather and adds a third-call no-op assertion, mine forces the interleave deterministically so it cannot pass by scheduling luck. They prove the same property by different means and neither subsumes the other. reserve_budget -- kept this branch's classification. main's new site returns "permit_not_active" and compares expires_at bare; the three other sites in the same file (authorize_and_reserve at :515/:525 and :585/:592) return f"permit_{status}" and normalize through to_naive_utc. main's site is the outlier in its own file, and a caller should not get permit_revoked from one reservation path and permit_not_active from the other for the same condition. The bare comparison is also a naive/aware TypeError waiting for a driver that returns tz-aware timestamps. No test pins "permit_not_active", so nothing in main regresses. The reserve comment merges both explanations. Note on the SQLite guard in #304: it refuses sqlite as the *durable-state* backend, which is real and welcome, but it does not close the case this branch's findings actually turn on. STATE_BACKEND=redis with REDIS_URL set satisfies that check, leaving DATABASE_URL free to be SQLite -- and the money and permit paths run on the ORM engine, not on durable state. Neither validate_trust_mode_config nor app/db/database.py refuses a SQLite DATABASE_URL in a production-like environment. Tracked as follow-up. Validation after the merge: 1785 passed / 32 skipped (app + b2a_sdk); ruff and mypy 2.3.1 clean over 173 files; doc references resolve; OpenAPI parity matches.
…ents Eleven of the twelve findings closed by #305 were live for one reason: `SELECT ... FOR UPDATE` is a silent no-op on SQLite, and nothing in this repository refused SQLite in production. The guarded UPDATEs fixed the paths; this refuses the configuration that made the old ones exploitable. #304 added a SQLite refusal, but it guards `STATE_BACKEND` -- the key/value durable-state store -- and the money and permit paths write through the ORM engine `DATABASE_URL` builds, not through that store. The gap between the two is reachable and not exotic: `STATE_BACKEND=redis` with a `REDIS_URL` satisfies the state check completely while `DATABASE_URL` stays SQLite. `_resolve_backend`'s `auto` branch reaches the same posture without anyone choosing it, since it falls through to redis when `DATABASE_URL` is not a Postgres DSN. `validate_trust_mode_config` -- whose stated job is refusing unsafe deploy postures -- now also refuses: - a SQLite `DATABASE_URL`, file and in-memory spellings alike. The in-memory ones are not a lesser case: they lose the ledger on restart as well as dropping the locks. - an empty `DATABASE_URL`. Wallets, permits, receipts and the ledger are relational; without one `get_engine()` returns `None` and the trust plane has nowhere durable to record what it authorized. Refusing SQLite while permitting *nothing* would not be a coherent guard. `is_sqlite_url` is new and deliberately not `sqlite_path_from_url`: that helper answers "is there a durable file here?" and returns "" for in-memory URLs, which is the opposite of what a safety check needs. The production-posture fixture gains the `DATABASE_URL` a real production deployment has, so it now models a complete posture rather than one that happened to omit the database. Local development is unaffected and a test pins that: SQLite stays accepted outside production-like environments, which is what this suite itself runs on.
… behind (#310) Production-like environments now refuse a SQLite or empty DATABASE_URL at boot — the configuration that made #305's eleven row-lock findings live. Separate control from #304's STATE_BACKEND guard: that one governs the key/value store, this one the ORM engine the money paths write through. Four guarded writes from #305 gained the regression tests they shipped without (refund credit, reconciled refund, operator permit release, Stripe clawback), each proven by reverting the service. Velocity reversal now decrements each counter under the period marker it was recorded against. Six defects fixed, found by negative-path tests and review: - unknown wallet on /v1/billing/charge answered 500; now 404 - non-finite units reached ledger arithmetic; negative units inverted the debit into a credit (100 -> 110, logged as a debit); both refused at the engine - dry-run session endpoints confirmed another tenant's session exists and echoed its owning wallet id; inaccessible and unknown sessions now answer an identical 404 - revert_session reported "No changes made to real wallet" after a concurrent commit had charged it (100 -> 96); end_session now claims via _claim_session so exactly one of end/commit/revert wins - simulate_charge's write-back could resurrect a claimed session (Redis: recreated with all charges, committable twice; memory: KeyError as 500); the write is now conditional - commit/revert answered 200 with success:false when the session was gone; now 404, keyed on the empty wallet_id so an insufficient-funds commit keeps its 200 SDK bumped to 0.5.0 (source) with latest_release_tag distinguishing the published python-sdk-v0.4.0. 1857 passed / 31 skipped on the merged tree; ruff, mypy, doc refs, OpenAPI parity, trust release + coverage gates clean; PostgreSQL suites green. Every regression test confirmed failing against its unfixed source, including the concurrency proofs driven through a fake Redis client that yields inside every call.
Summary
Three of the findings #302 listed under "Not addressed here". Each was re-verified against current
mainrather than taken from the description — one of them (reserve_budgetexpiry) was still open only because #298 fixed the other reservation path — and each ships a regression test confirmed to fail against the unfixed source.1.
reserve_budgetspent against expired permits — engine-independent#298 added
expires_at > nowtoauthorize_and_reserve's guarded update.reserve_budgetis a second reservation path and never got it:sa_update(PermitModel)authorize_and_reservereserve_budgetrelease_budget/release_dispatch_budget_onceAn expired permit keeps
status="active"in storage (expiry is a dynamic check, not a stored state), so the WHERE clause matched and budget moved. This was wrong on PostgreSQL too — no row-lock behaviour required.Also replaces the catch-all denial: a miss now reports
permit_expired/permit_not_activeinstead of calling every failurepermit_budget_exceeded.Test: an active permit whose
expires_atis moved into the past is refused withpermit_expired, andspent_creditsstays0. Unfixed:DID NOT RAISE— the expired permit is charged.2.
release_dispatch_budget_oncewas not once-onlyIt read
budget_released_atunderwith_for_update=True, then decremented the permit. SQLAlchemy silently dropsFOR UPDATEon engines that don't support it, so two concurrent callers both sawNULLand both released — the permit is credited back twice for one dispatch.The decrement was already atomic (clamped
CASE); the claim was not. It's now a guardedUPDATE(NULL → now), and only the caller withrowcount == 1proceeds.Test: two concurrent releases must yield exactly one
Trueand exactly one decrement. Unfixed, the assertion fails with2 = len([True, True])— the double release reproduces.3. SQLite was a permitted production durable-state backend — the enabler
STATE_BACKEND=memorywas refused in production-like environments;sqlitewas not, andautoresolution would select it from a SQLiteDATABASE_URL— the ordinary local-development setting. That is the deployment-level enabler for every remainingFOR UPDATE-is-a-no-op finding in the money path. Both the explicit and the auto path now fail closed with an explanatory error.Tests: production-like +
STATE_BACKEND=sqlitewithSQLITE_URLset, and auto-resolution from a sqliteDATABASE_URL, both raiseDurableStateConfigError.Incidental
The docs sweep in
test_onboarding_contract.pywalked.claude/worktrees/— a gitignored path holding full checkouts — so a copy of a file the exclude list already forgave failed the sweep. This fails for anyone using a git worktree there;.claude/is now excluded. (This is what made the suite red locally;main's own CI was unaffected.)Type of Change
Validation
pytest -q -m "not production_trust"— 1711 passed, 32 skippedproduction_trust— 6 passedruff check .clean;mypy app/clean across 172 source filesdemo_trust_plane.py --assertandred_team_trust_plane.py --assertclean (audit chain valid, tampered audit correctly rejected)Risk
Fix 3 is a deployment-contract change: a production-like service configured with
STATE_BACKEND=sqlite, or with a SQLiteDATABASE_URLunderauto, now refuses to boot instead of running on an unsafe backend. That is the intended fail-closed behaviour and matchesdocs/deploy-railway.md, which already requiresSTATE_BACKEND=postgres. The live deployment is unaffected — it runsSTATE_BACKEND=postgres, verified against production at thea928fd6frelease.Fixes 1 and 2 only ever deny more or release once instead of twice; neither can debit anything extra.
Not addressed here
Still open from #302's list, and not fixed by this PR — fix 3 removes their production reachability, but the code-level races remain wherever
FOR UPDATEis a no-op:sa_updatetouchesWalletModelanywhere)reclaim_child_walletable to credit a parent twiceoperation_kind='local', debit committed,ledger_entry_id IS NULL) matching neither reconciler branchConverting those to guarded updates is separate work, worth its own PR.
Generated by Claude Code
Note
Medium Risk
Production-like services configured with SQLite durable state will fail to boot instead of running on an unsafe backend; permit changes only tighten denials and prevent double budget release.
Overview
Hardens permit budget paths and production durable-state selection so expired permits cannot be charged, dispatch budget is credited back exactly once under concurrency, and production-like deploys cannot boot on SQLite.
Permits:
reserve_budgetnow matchesauthorize_and_reserveby requiringexpires_at > nowin its guardedUPDATE, and failed reserves distinguishpermit_expired/permit_not_activeinstead of alwayspermit_budget_exceeded.release_dispatch_budget_onceclaimsbudget_released_atwith a guardedUPDATE(NULL → now) before decrementingspent_credits, so only one concurrent caller releases budget whenFOR UPDATEis ineffective.Durable state: Explicit
STATE_BACKEND=sqliteand auto-resolution from a SQLiteDATABASE_URLraiseDurableStateConfigErrorin production-like environments; auto no longer falls back to SQLite there.Tests / incidental: Regression tests for concurrent release, expired
reserve_budget, and production SQLite refusal; docs sweep excludes.claude/worktrees.Reviewed by Cursor Bugbot for commit d37af5f. Configure here.