amadan
Retirement protocol: the commit column's migration is one transaction and no handler can release a committed reservation
Review twenty-five's answer. Astra's P2: the column's addition and the marking of reservations from before it were two statements with no transaction around them, so a death between them, or a second console starting in the window, left an older build's kept reservation reading as pending, and evidence could then withdraw it. The pragma check, the ALTER and the UPDATE are now one transaction, and a regression aborts the marking with a trigger and sees the column roll back with it, then the retried migration mark the row. Opus P3-3 and Astra's P3: the unconditional release is no longer production code. It lives in billingretirement_export_test.go, for building states in tests; the deletion handler's only release is the pending-only one, and the choice cannot be unmade without adding the function back. Two consoles at the point of no return are tested on two connections to one file: evidence before the commit withdraws it, evidence after it lands on the committed account, and a losing request's cleanup leaves the winner's commit standing. Opus P3-2: the no-bucket preflight's release is asserted (no reservation remains, a checkout opens afterwards). The tombstone comment no longer says the caller releases on errNoReleaseStore. The design says the marking happens in the migration's transaction. The dispositions have a section for review twenty-five and both reports are in the tree.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
10 files changed,
+542
−22
docs/superpowers/specs/2026-10-02-intent-retirement-protocol-design.md+1 −1docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review25.md+181 −0docs/superpowers/specs/DISPOSITIONS-multi-currency.md+15 −0docs/superpowers/specs/OPUS-REVIEW-multi-currency-review25.md+142 −0internal/console/billingnamed_test.go+155 −0internal/console/billingretirement.go+21 −6internal/console/billingretirement_export_test.go+11 −0internal/console/billingstore.go+0 −11internal/console/deletion_test.go+12 −0internal/console/ownership.go+4 −4
CI passed — run details
diff --git a/docs/superpowers/specs/2026-10-02-intent-retirement-protocol-design.md b/docs/superpowers/specs/2026-10-02-intent-retirement-protocol-design.md| index f15726a3e..c9faefe37 100644 |
| --- a/docs/superpowers/specs/2026-10-02-intent-retirement-protocol-design.md |
| +++ b/docs/superpowers/specs/2026-10-02-intent-retirement-protocol-design.md |
| @@ -125,7 +125,7 @@ Built in `internal/console/billingretirement.go` and the paths it touches, with |
| - **An unbound intent under a review that names no session** is still reconciled by the pass, which is how its session is found. Nothing records, retires or cleans up an intent under an open review on that path: the store's record, expiry and cleanup each refuse it. (Round twenty's build let the cleanup of an intent that never had a session delete it under its review; review twenty-one.) |
| - **Named subscriptions** (review twenty-one, answered as one rule). A subscription that names a standing intent is recorded from the first read, whether or not a refusal review exists. Nothing deletes an intent past one. The record of the purchase keeps what it consumed (`billing_consumed_intents`: the intent, its session, the subscription it recorded with) and in the same transaction makes every other subscription seen naming the intent a dispute; a subscription that names the intent afterwards is compared with that record and is a dispute. The expiry of the intent's session, and the cleanup of an intent that never had a session, do not delete an intent a subscription was seen naming: they file the refusal, with it attached, and the intent stands (an expired session has no subscription and an intent with no session never had a checkout, so that subscription was made outside checkout). `retire` is held while anything is attached, whether or not the intent still stands. What stays outside: a subscription first seen naming an intent that went with nothing seen naming it and no purchase (one that expired in the ordinary way; one that never had a session and was cleaned up) is a rejected event with its account and no hold, because nothing is kept of such an intent. What the rule costs: a subscription seen naming an intent holds that intent's expiry and cleanup whatever the subscription's own state, a dead one included, into a refusal stage one can only hold; and a second subscription at a purchase's record is a dispute stage one cannot resolve. If the provider ever makes a subscription before its checkout session completes, an honest buyer meets this and is held with no disposition; whether it does is the first check of the test-mode rehearsal, and a gate on enabling (review twenty-two). |
| - **Only a first purchase consumes an intent**: the record of a subscription that is not yet the account's recorded one, judged in the record's transaction. A later event of the account's recorded subscription deletes no intent, whatever its metadata names. |
| -- **A refusal filed late.** A refusal of a session filed for an intent that a purchase consumed meanwhile is kept as a dispute against what was consumed, in the filing's transaction. **Account retirement** refuses while a checkout refusal or dispute is open, and has a point of no return in the database (`commitBillingRetirement`), taken before anything outside is written: checkout evidence filed before it withdraws the pending retirement; evidence filed after it stays with the account, whose deletion finishes, and is listed under the deleted account, held with no stage-one disposition like every dispute (review twenty-three). An older binary has no commit step: during a roll its deletion reproduces the race, which the roll rule covers (older consoles are stopped before this build starts; a reservation from before the commit column is marked committed when the column is added). **A click** reads the intent's state again after each provider read and before it discloses a new link, and offers nothing under an open review (review twenty-two). |
| +- **A refusal filed late.** A refusal of a session filed for an intent that a purchase consumed meanwhile is kept as a dispute against what was consumed, in the filing's transaction. **Account retirement** refuses while a checkout refusal or dispute is open, and has a point of no return in the database (`commitBillingRetirement`), taken before anything outside is written: checkout evidence filed before it withdraws the pending retirement; evidence filed after it stays with the account, whose deletion finishes, and is listed under the deleted account, held with no stage-one disposition like every dispute (review twenty-three). An older binary has no commit step: during a roll its deletion reproduces the race, which the roll rule covers (older consoles are stopped before this build starts; a reservation from before the commit column is marked committed in the same transaction that adds the column, so no death or second console between the two leaves it pending). **A click** reads the intent's state again after each provider read and before it discloses a new link, and offers nothing under an open review (review twenty-two). |
| - **Binding.** A session id is written onto an intent in one place, which refuses a session that is already another purchase's (another intent's, a retired, kept or consumed one's) and one that is not the session the intent's open review names. |
| - **Creating an intent** re-reads the account's customer and the currency it is held to in its own transaction; a request built from an older reading is refused. |
| - **The refusals' turn** takes the longest-due first. **A listing** that fails on a later page records the pages it read. **An answer carrying another session's id** records nothing. |
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review25.md b/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review25.md| new file mode 100644 |
| index 000000000..1894e151b |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review25.md |
| @@ -0,0 +1,181 @@ |
| +All platform references are at **`1f1368b0`**; pricing-site references are at **`01b62f7`**. This was source-only review. Nothing was modified or tested, and no network commands were run. |
| + |
| +**Twenty-fourth review, finding by finding** |
| + |
| +| Item | Closed? | Would its test pass without its fix? | |
| +|---|---|---| |
| +| **Opus P3-1:** handler’s commit call untested | **Closed.** `billingnamed_test.go:1228` drives the deletion handler and inspects the marker at the bucket’s first write. | **No.** Removing the commit or moving it after the tombstone fails the explicit assertion at `:1280`. The hook also files a real webhook and checks deletion and attribution. | |
| +| **Opus P3-2:** losing request releases another request’s committed reservation | **Closed in the handler’s code**, `ownership.go:176`, `billingstore.go:1620`. | **No** if the conditional release helper becomes unconditional. **Yes** if only the handler is changed back to calling unconditional release. That surviving mutant remains a coverage gap. | |
| +| **Opus P3-3:** pre-column reservations become pending | **Partially closed.** Successful migration preserves them, but interruption between its two writes defeats the conversion. Finding 1 below. | **No** if the backfill is removed. **Yes** with the crash window intact: `billingnamed_test.go:1324` never interrupts migration. | |
| +| **Opus P3-4:** detached function comment | **Closed**, `billingstore.go:1605`. | Not applicable: comment placement. | |
| +| **Opus claim 1:** deleted-account reviews are “resolvable” | **Corrected** in the governing design and changed comments: they are listed and held without a stage-one disposition. | The existing **409, not 404** assertion establishes visibility and the hold, not resolution. | |
| +| **Opus claim 2:** serialization also protects older binaries | **Correctly qualified** in the governing design: older binaries lack the commit step and must be stopped. | No mixed-binary test establishes the operational rollout. The referenced launch-branch `ENABLEMENT.md` is outside the pinned platform tree inspected here. | |
| +| **Astra 1, P2:** committed marker is removable by cleanup | **Closed in current handler paths**, with the call-site test limitation above. | Helper regression fails; handler-call regression still passes. | |
| +| **Astra 2, P3:** rejected listing loses a deleted account’s name | **Closed**, `billingoperator.go:316`. | **No.** The assertion at `billingnamed_test.go:1309` requires the deleted account’s sqid on the rejected event. | |
| + |
| +**Astra’s P2 severity was right for the shared cleanup defect.** Removing the marker could reopen checkout while another request published the tombstone, then prevent local deletion because a new payable intent existed. That is a broken retirement boundary, not merely a diagnostic problem. Neither unusual Stripe behavior nor a successful payment is needed. |
| + |
| +**Findings, most severe first** |
| + |
| +1. **P2 — Adding the commit column and preserving old reservations are not atomic.** |
| + |
| + **Where:** `internal/console/billingretirement.go:184`, particularly `:190–198`. |
| + |
| + The migration checks whether `committed_at` exists, executes `ALTER TABLE`, then executes the backfill through a separate `s.db.Exec`. Its transaction begins only afterwards. |
| + |
| + **Concrete scenario:** |
| + |
| + 1. An older binary retained a reservation following an ambiguous tombstone write. |
| + 2. Migration commits the addition of `committed_at DEFAULT 0`. |
| + 3. The process dies, or the backfill fails, before the next statement completes. |
| + 4. Restart sees the column and permanently skips the backfill. |
| + 5. Checkout evidence treats that old reservation as pending and deletes it through `cancelPendingRetirementTx` (`billingstore.go:1547`). |
| + |
| + The database has now lost the barrier protecting an account whose external tombstone may already exist. Startup materialization does not guarantee rescue: an unreadable manifest is skipped (`materialize.go:68`), allowing the console to continue with its existing local account. A new checkout can then pass the reservation check at `billingstore.go:363`; later reaping refuses its standing intent. |
| + |
| + This also admits a startup interleaving: another opener can observe the added column before the first opener backfills it. |
| + |
| + **Smallest acceptable fix:** put the column-existence check, `ALTER TABLE`, and legacy-row backfill in one SQLite transaction. Preserve the distinction between legacy reservations and newly created pending reservations. |
| + |
| + Add a regression that forces failure between addition and backfill, verifies rollback, then retries migration. A trigger that aborts the backfill can exercise this without killing a process. The successful-migration test at `billingnamed_test.go:1324` does not cover this boundary. |
| + |
| +2. **P3 — The failed-commit handler cleanup still has no regression that protects its choice of release function.** |
| + |
| + **Where:** `internal/console/ownership.go:176`; `internal/console/billingnamed_test.go:1127`; dispositions `:545`. |
| + |
| + Current production code is correct. However, changing that single handler call back to `releaseBillingRetirement` restores the previous P2 while the new tests remain green. The test calls `releasePendingBillingRetirement` directly at `billingnamed_test.go:1138`. |
| + |
| + The surviving mutant is not exclusively reachable through a transient database error. The handler configurations support this ordering: |
| + |
| + 1. Request A reserves and pauses. |
| + 2. A no-bucket request B shares that pending reservation, then removes it during preflight cleanup. |
| + 3. A’s commit returns ordinary `errBillingRetirement` because the reservation is missing. |
| + 4. Before A cleans up, request C reserves and commits. |
| + 5. A’s **mutated unconditional cleanup** removes C’s committed reservation. |
| + |
| + The current conditional cleanup preserves C’s marker. |
| + |
| + **Smallest acceptable fix:** add a deterministic handler regression around failed commit and cleanup, with another request’s committed reservation present when cleanup runs. Assert that the marker survives and checkout remains blocked. It need not run two processes or depend on a naturally occurring database error. |
| + |
| + I would not waive this regression for the retirement boundary merely because the helper itself is tested. |
| + |
| +**Committed-marker durability and deletion, step by step** |
| + |
| +For reservations correctly marked committed, **all current handler cleanup paths preserve the marker**: |
| + |
| +| Step or failure | Evidence, competing requests and process death | |
| +|---|---| |
| +| Intent preparation — `ownership.go:148` | Preparation uses the ordinary guarded expiry/cleanup paths. A payable, reviewed or subscription-named intent remains and prevents reservation. A completed purchase records and then blocks retirement. | |
| +| Reservation — `:151` | `reserveBillingRetirement` checks obligations in its transaction. The reservation blocks new checkout and month freezing. Death leaves it blocking until a deletion retry or qualifying evidence withdraws it. | |
| +| No-bucket preflight — `:160` | Happens before commit. Cleanup removes only pending rows. It cannot erase another request’s committed row. | |
| +| Commit — `:172` | Rechecks the retirement conditions and requires a reservation. Evidence committed first withdraws the pending reservation; commit then refuses before external writes. Commit first makes later evidence belong to the retiring account. | |
| +| Failed commit — `:176` | Conditional cleanup cannot remove a committed row, including one committed after the failure but before cleanup. Its handler call remains insufficiently tested. | |
| +| Tombstone — `:198` | Every failure retains the committed reservation. Death before, during or after the write leaves checkout blocked. A later deletion retry can continue. | |
| +| Local deletion — `:211` | Post-commit checkout disputes do not prevent completion. The account row and billing evidence survive deletion. If the tombstone succeeded but local deletion failed, retry or materialization can finish. | |
| + |
| +A handler **can remove another request’s pending reservation**: reservations are account-scoped, not request-owned. That is safe here because the other request must subsequently prove the reservation still exists before committing. No handler now calls unconditional release. |
| + |
| +`ReapAccount` correctly treats an already-retired manifest as past the external boundary (`store.go:1370`). It still refuses an active subscription, standing intent, unsettled month or quantity obligation. A dispute remains attributable to the deleted account; it does not disappear into account deletion. |
| + |
| +The operator listing supplies the sqid and `account_deleted`; the resolution door finds the account and answers **409 held**. That provides identification and preserves the obligation. It provides no adjudication mechanism, as stage one expressly states. |
| + |
| +**Other requested source conclusions** |
| + |
| +- **GBP purchase, A–C, M–Q and U.** `GET ?country=IN¤cy=gbp` renders band D in GBP; Personal is £7.70 annually. The signed form posts GBP as a choice. The checkout handler compares the effective purchase currency with the signed quote before starting checkout (`billinghandlers.go:71`). The intent, Checkout request, binding, subscription reconciliation, account state and assignments retain that book/currency identity. Monthly allowance and calculation retain GBP; invoice creation, recovery, line verification and quantity updates use the stored currency. I found no remaining path that converts this purchase or silently prices it in EUR. |
| + |
| +- **One form and signatures.** A changed country/currency or fabricated quote redirects before purchase. Present-empty, repeated, unsupported, padded or differently cased currency fields receive 400 on both POST doors. A missing field selects the country default, subject to quote comparison; legacy configuration remains EUR-only. The signature at `billingpage.go:446` uses a billing-secret-derived key and covers session token, account, book, country, band, currency, choice flag and member count. The disclosed form cannot sign another quote. Another tab does not invalidate an otherwise matching signed quote. Plan is omitted because all offered plans are priced on that page; quantity follows the signed membership count and plan minimum. Postal code affects location/tax validation, not the displayed exclusive-tax plan price. |
| + |
| +- **Quote endpoint and cookies.** The quote endpoint performs its gates and redirects to the account’s billing page using encoded parameters; it does not create a purchase. Cookies affect rendering, not checkout authority. Present-empty suppresses fallback and clears; absent falls back; invalid leaves the existing cookie unchanged. Consequently, an old choice can reappear on a later parameter-absent request after an invalid parameter, consistent with “invalid sets nothing.” A held currency overrides URL/cookie suggestions and is the sole option, including legacy EUR (`billingpage.go:304`). |
| + |
| +- **Ownership, D/L/R and subsequent refinements.** `ownSession` checks mode, intent, both account identifiers, pinned customer and bound session ID (`billingservice.go:273`). Create refusals and recovery cannot acquire authority from matching currency alone. Provider observations are recorded before the caller’s ownership decision; recording contradictory evidence is not permission to bind or charge. The deliberate expiry exception uses the console’s already-bound ID through `advanceRefusal`, not an ID supplied by conflicting metadata. Leaving an unbound, unowned session open is appropriate; expiring the console’s trusted bound ID is also appropriate. |
| + |
| +- **Binder and reconcile.** Their purchase checks agree on plan, band, terms, book and currency. A completed wrong-purchase session produces the intent-keyed refusal once while open. A matching subscription arriving before completion, or before the completed session names its subscription, retries rather than inventing a refusal (`billingservice.go:1022`, `:1033`). Named subscriptions are nevertheless recorded before that return. A different named subscription cannot subsequently vanish during consumption. |
| + |
| +- **Barrier, E/H/N/T.** The refined trigger at `billingstore.go:208` matches positive paid spans used by `AllowanceForMonth`: active, nonfree, clipped by month boundaries, paid-through and the next assignment across both tables. Same-second supersession uses revision order and can contribute zero. Free/inactive assignments truncate earlier spans without themselves counting. `period_end` means the provider’s paid-through endpoint, not supersession time. I found no valid-history counterexample permitting an older v1 freeze where this build uses book history, or refusing this build’s legitimate legacy-only v1. Trigger retirement checks that the new trigger exists before dropping the old one. A crash can leave both names temporarily, not neither; restart completes retirement. This is separate from Finding 1. |
| + |
| +- **Inclusions, validation and export, F/G/J.** Inclusions come from pooled allowances at charged quantity, with unavailable Personal handled separately. Current terms produce exact per-member figures across bands and quantities; band A matches the approved sentences. Book validation rejects malformed or duplicate keys, missing entries, inconsistent rounding/EUR values and overflowing arithmetic. Configuration requires the current catalog and whole optional historical catalogs, with distinct price IDs. Provisioning accepts at least the required book keys and the console’s own-only `currency_options` rule. The public export exposes commercial terms, not account or provider secrets. |
| + |
| +- **Pricing site, K.** At `01b62f7`, prices, exact overage rates, floors, country options/defaults and scaled inclusions derive from the export. Both link parameters are always present; currency is populated only when chosen (`src/site.js:77`). Locale-only GBP can therefore lead to a console USD default when no country or currency was chosen: that is the owner’s explicit suggestion-only rule. The first-line marker and exact `"1"` draft override are implemented (`build.mjs:34`, `:376`), with the check integrated into the ordinary build. Wrong-valued amounts in slots are checked across currencies; replacing a derived value with the **identical current literal** remains observationally indistinguishable, and the tests acknowledge that limit. |
| + |
| +**The lifecycle invariant at this commit** |
| + |
| +A reviewed intent remains standing until an allowed transactional disposition. A first purchase cannot consume it under an open refusal or dispute. Observed named subscriptions survive consumption and reviewed retirement; later incompatible evidence becomes a durable account dispute. |
| + |
| +The mutation points are: |
| + |
| +| Mutation | Paths reaching it and protection | |
| +|---|---| |
| +| Bind session — `billingstore.go:458`, `:461` | `saveBillingCheckout` and binder share `bindIntentSession`. Another purchase’s session and a session contradicting the review’s reference are refused. Rebinding the same session is allowed. | |
| +| Ordinary expiry deletion — `:553` | Click, pass, deletion preparation and refused-session cleanup. Exact proof is required; open reviews prevent deletion; observed named subscriptions cause a refusal instead. | |
| +| Unready cleanup — `:604` | Start, pass and account-deletion preparation. Requires no created session; open reviews prevent deletion; named subscriptions cause a refusal. | |
| +| First-purchase consumption — `:890` | Production reconciliation calls `applyBillingStateDated` with the first-purchase requirement (`billingservice.go:1142`). Exact intent consumption, account/assignment writes, event recording, entitlement work and other-subscription disputes commit together. Failure rolls them all back. Renewals do not consume a newer intent. | |
| +| Operator retirement — `:1208` | Resolves and archives exactly the reviewed account/intent, with its session/reference, customer and fate; closes only its pending checkout quantity events. Another standing intent is untouched. | |
| +| Account deletion/reap | Marks the account deleted rather than cascading away billing evidence. Existing intents block it. Finding 1 is the remaining migration hole in its reservation boundary. | |
| + |
| +`billing_consumed_intents` is written when an intent is actually consumed. Its historical subscription comparison prevents an honest old subscription replay from becoming a dispute after a later purchase. Intent IDs are generated identifiers; binding refuses reused archived session IDs. |
| + |
| +The two documented unarchived exceptions remain: ordinary expiry and never-created cleanup **with no subscription previously observed naming the intent**. A subscription first observed afterwards can become only a rejected event. Those are explicit limits, not evidence that was recorded and then erased. |
| + |
| +The three wrappers are the only direct session/subscription reads in the console: `billingretirement.go:857`, `:876`, `:894`. Start/resume, binder, reconcile, recovery, pass, operator gate, quantity worker and invoice preflight use them. Storage failure prevents delivery of the object to the caller. Partial listings preserve and record decodable observations before returning their error. |
| + |
| +The exclusions remain material: |
| + |
| +- Create/expiry answers can be idempotency replays; they generally do not prove current fate. |
| +- Session reads before refusal are not retained as refusal evidence; the observation causing filing is handed in again. |
| +- Identity-less/open observations before any fate do not establish one. Their earlier assertion is lost; a later valid expiry can become the first admissible proof. |
| +- Wrong-ID responses are not accepted as evidence about either session. |
| +- The account’s already-recorded subscription is excluded from named-intent classification. |
| + |
| +Those qualifications are necessary whenever describing “nothing observed is dropped.” |
| + |
| +`keep` still validates the session, not the subscription’s complete purchase identity. A wrong subscription can reopen the refusal on reconciliation; the governing design expressly accepts that behavior. Extra obligations, disputes and migrated reviews remain held. Old clients omitting outcome still request `retire`; they do not acquire authority over paid cases. |
| + |
| +The refusal pass has separate caller-cancellable time, a three-item budget and oldest-due ordering. Failed expiry preserves the review; payment winning expiry records completion. The quantity-history healing sweep is gone, and `quantityOutcomesSQL` supplies the guard, sweep and obligation check. Existing pre-history and clock-rollback holds are not silently cleared. |
| + |
| +I found no additional demonstrated regression for an honest buyer, relative to the named-subscription design, based on provider behavior established by this source. Whether failed authentication, delayed payment or provider replacement produces additional subscriptions remains a rehearsal question. Duplicate delivery alone does not create a second obligation. |
| + |
| +**Tests and claims the code contradicts** |
| + |
| +The four previously surviving round-twenty mutations now have discriminating assertions: |
| + |
| +| Mutation | Why the revised test fails without the fix | |
| +|---|---| |
| +| Ignore one identity clause | `billingretirement_test.go:942` varies clauses individually and requires the dispute. | |
| +| Do not reopen on attachment after `keep` | `billingretirement_test.go:815` uses an incomplete subscription, preventing reconcile’s later refusal from masking classification. | |
| +| Do not record the session that caused filing | `billingevidence_test.go:315` requires the completion evidence with exactly one provider request. | |
| +| Offer a session contradicted only by the second read | `billingevidence_test.go:294` checks no destination, pending status and the specific dispute. | |
| + |
| +The transaction-failure test exercises `fileCheckoutRefusal`, rather than merely demonstrating SQLite rollback. The listing tests distinguish arrival order from either ID sort direction. The revised fake’s distinct session IDs and late replay assertions materially test non-recording of the retired purchase. |
| + |
| +These establish their controlled sequences, not every two-console interleaving. The structural guard catches selectors through aliases, method values/expressions and other receiver types, and requires classification of new exported client methods. Its wrapper-order check is lexical, not a control-flow proof. |
| + |
| +The review-three/four qualifications now matter: provisioning tests inspect script text; deleting a divisibility assertion over unchanged exact data need not fail; equivalent or redundant guards can survive mutation. I cannot certify historical mutation runs from source. I found no additional concrete shared-cookie, fixture-order, temporary-root or wall-clock defect in the inspected tests. |
| + |
| +Remaining contradicted claims are: |
| + |
| +- **“Old reservations are marked committed when the column is added”**, dispositions `:540` and governing design `:128`: only if both autocommitted migration statements finish. Finding 1. |
| +- **The surviving handler mutant needs a transient database error**, dispositions `:545`: that misses a new commitment between an ordinary failed commit and cleanup. Finding 2. |
| +- **“Only this request’s own pending reservation”**, `ownership.go:158`: there is no request ownership. The conditional deletion can cancel another request’s pending reservation; the commit recheck makes that safe. |
| +- **A late filing “files nothing”**, `billingstore.go:1222`: after consumption, `fileCheckoutRefusal` can file the surviving dispute at `:1271`. The comment predates that protection. |
| +- **Hourly retry after three tries**, `billingretirement.go:987`: the SQL uses the previous counter. Delays are 1, 5, 15 minutes, then hourly after the fourth unsuccessful try. |
| + |
| +**What would close review eighteen’s P2** |
| + |
| +I would require the atomic migration fix and its interrupted-migration regression, plus the handler cleanup regression that kills the surviving call-site mutant. Then the code issues found in this review would be closed. |
| + |
| +The owner’s deferred dispositions, provider rehearsal and rollout restrictions are separate enablement conditions. Conservative cases intentionally held by stage one do not, by themselves, require inventing another code defect or blocking acceptance of that limited protocol. |
| + |
| +**What must be verified against Stripe test mode before enabling** |
| + |
| +1. **Catalog identities:** all 36 prices, amounts, currencies, recurrence, metadata and own-only `currency_options`. Establish that provisioning output passes the console’s actual validation. |
| +2. **GBP purchase:** India plus explicit GBP, country defaults and held legacy EUR. Establish matching Checkout, subscription and item currency/amount, with fixed-book Adaptive Pricing disabled. |
| +3. **Event ordering and failed payment:** subscription-before-completion, abandoned 3-D Secure, declined-card retry and delayed methods. Establish whether honest buyers encounter attached dead/replacement subscriptions and therefore require an additional approved disposition. |
| +4. **Expiry racing payment:** retained URLs, concurrent expiry, lost answers and idempotent replays, followed by GET. Establish that proven expiry is terminal and unpaid, while a winning payment becomes completion rather than false expiry. |
| +5. **Identity and corrections:** GET/list/expiry response fields, metadata changes and delayed reads after correction. Establish the identity and freshness assumptions used by terminal proof. |
| +6. **Lost-create recovery:** creation succeeds but its answer is lost; recover by the original key and customer listing. Establish that recovery finds the same purchase without creating another payable session. |
| +7. **Metered invoices:** GBP—and controls in USD/EUR—through floor, rounding, draft recovery, item recovery, finalization and payment. Establish exact stored-currency amounts throughout. |
| +8. **Quantity and renewal:** changes, prorations, delayed/lost answers and renewal boundaries. Establish the stored price/currency and dating assumptions used by allowance history. |
| +9. **Legacy replay:** original EUR request shapes and existing idempotency keys. Establish unchanged legacy behavior and refusal of unexpected converted identities. |
| +10. **Financial settlement:** inspect payments, PaymentIntents, invoices, refunds and future collection after `incomplete_expired` and cancellation. Establish what remains owed before any new disposition policy is approved. |
| + |
| +**Verdict: reject.** |
| \ No newline at end of file |
diff --git a/docs/superpowers/specs/DISPOSITIONS-multi-currency.md b/docs/superpowers/specs/DISPOSITIONS-multi-currency.md| index f00024cb6..659f3a0db 100644 |
| --- a/docs/superpowers/specs/DISPOSITIONS-multi-currency.md |
| +++ b/docs/superpowers/specs/DISPOSITIONS-multi-currency.md |
| @@ -546,3 +546,18 @@ Mutation checks, in a tree copy, five. Four die: a losing request's cleanup remo |
| Stated and not changed: no test runs two console processes (the losing request's cleanup is driven directly); an older binary has no commit step (the roll rule); a deleted account's dispute is held with no stage-one disposition, like every dispute. |
| + |
| +## Review twenty-five (reviewed `1f1368b0`; Opus accepted with fixes, Astra rejected) |
| + |
| +Reports: `OPUS-REVIEW-multi-currency-review25.md`, `CODEX-FINDINGS-multi-currency-review25.md`. Opus: round twenty-four's finding closed on every cleanup path, no P1 or P2, three P3s and two claims; "for this binary, review eighteen's P2 is closed", with two additions wanted before saying so without qualification (a two-connection test; the unconditional release out of production code). Astra: one P2 and one P3; the committed marker durable on every cleanup path for reservations correctly marked; the P2 is the migration. **Review eighteen's P2 stays recorded as open until a round ends with both accepting.** The gate on `1f1368b0` passed (23:33Z, alone) and CI passed. |
| + |
| +| Finding | Disposition | Where | Counterexample test | |
| +| --- | --- | --- | --- | |
| +| Astra 1 (P2), Opus P3-1: the column's addition and the marking of old reservations were two statements outside a transaction; a death between them, or a second console starting in the window, left an older build's kept reservation reading as pending, which evidence could then withdraw. | Fixed: the check, the `ALTER` and the `UPDATE` are one transaction. | `billingretirement.go` `migrateBillingRetirement` | `TestAReservationFromBeforeTheCommitColumnIsCommitted`: a trigger aborts the marking, the addition rolls back with it, and the retried migration marks the row | |
| +| Opus P3-2: the no-bucket preflight's release was untested. | Tested: no reservation remains and a checkout opens afterwards. | `deletion_test.go` | `TestDeletionRefusedWhenTheBucketIsUnreachable` | |
| +| Opus P3-3, Astra 2 (P3): the handler's choice of the pending-only release had no regression; a handler could call the unconditional release again with green tests. | The unconditional release is no longer production code: it lives in a `_test.go` file, for building states. A handler cannot call it, so the choice cannot be unmade without adding it back. Astra's deterministic ordering (A's commit fails because B's preflight removed the shared pending row; C reserves and commits; A cleans up) is tested at the store with a second connection: A's cleanup is the pending-only release and C's commit survives. A handler-level pause between a failed commit and its cleanup has no seam, and none is added. | `billingretirement_export_test.go`; `billingnamed_test.go` | `TestTwoConsolesAtTheRetirementsPointOfNoReturn` (three orderings on two connections to one file) | |
| +| Opus RRRR: sequential tests are not two connections. | Two connections now: a second `Store` opened on the fixture's file plays the other console for evidence before the commit, evidence after it, and a losing commit's cleanup. | | the same test | |
| +| Opus claims 1 and 2: the `tombstoneAccount` comment said the caller releases on `errNoReleaseStore`; the design said old reservations are marked "when the column is added" without the crash window. | Corrected; the roll rule on the launch branch names the exception for a database that ran `e152df30` (none did: billing is off and it was never rolled). | `ownership.go`, design, `ENABLEMENT.md` | | |
| + |
| +Stated and not changed: the two-connection test is two handles in one process on one WAL file, which is what two consoles are to SQLite, and not two processes; a deleted account's dispute is held with no stage-one disposition; the roll rule covers an older binary's deletion and reap. |
| + |
diff --git a/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review25.md b/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review25.md| new file mode 100644 |
| index 000000000..edb6d9a28 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review25.md |
| @@ -0,0 +1,142 @@ |
| +**Verdict: accept with fixes.** I found no P1 or P2. Both reviewers' round-24 finding is closed: a losing request's cleanup can no longer remove a committed retirement on any of the handler's three cleanup paths. Three P3s and two stale claims remain, and none of them gates the round. |
| + |
| +On review eighteen's P2: nothing in this branch's code stands in the way of closing it for this binary. Below, under RRRR, I name the two small additions that would let me close it without qualification. |
| + |
| +This was a source-only review, using `git show 1f1368b0:` throughout. I modified nothing and ran nothing. The delta `e152df30..1f1368b0` touches only these files: `ownership.go`, `billingstore.go`, `billingretirement.go`, `billingoperator.go`, `store.go`, `billingnamed_test.go`, and the docs. It touches no pricing, form, quote, cookie, trigger, export or site code. My conclusions on A–K, L–ZZZ2, AAAA–OOOO and the site at `01b62f7` therefore stand as reported at `e152df30`. |
| + |
| +## Round twenty-four, finding by finding |
| + |
| +| Finding | Closed? | Would its test pass without the fix? | |
| +|---|---|---| |
| +| **Astra 1 (P2) / Opus P3-2:** a failed commit released a committed row | **Closed.** The handler's commit-failure branch (`ownership.go:176`) and the no-bucket branch (`:162`) both call `releasePendingBillingRetirement` (`billingstore.go:1620`, `AND committed_at=0`). Once the commit has happened, nothing releases. A grep finds no non-test caller of `releaseBillingRetirement`. | **The store test fails without the fix:** with an unconditional delete, the marker goes and `beginBillingCheckout` succeeds. **The handler's choice is untested.** Putting `releaseBillingRetirement` back at `:176` is the surviving mutant (see QQQQ). | |
| +| **Opus P3-1:** the handler's commit call was untested | **Closed.** `TestTheDeletionHandlerCommitsBeforeTheTombstone` hooks the first `Put` on the bucket. `TombstoneAccount` (`domains/mint.go:254`) writes only through `Put`. | **It fails without the fix.** Remove the commit and `committedAtWrite` is false. Move the commit after the tombstone and the same assertion fails. Drop the `committed` bypass and `DeleteAccount` refuses, so "the account survived" fails. The test also fails if the hook never runs. | |
| +| **Opus P3-3:** reservations from before the column read as pending | **Closed on a database that never ran `e152df30`**, apart from a crash window (P3-1 below). | **It fails without the fix:** `committed_at` stays 0 and the first assertion fails. | |
| +| **Opus P3-4:** a detached doc comment | **Closed.** The release functions now come before the lookups, and the comment names its callers. | n/a | |
| +| **Opus claims 1–2:** "resolvable"; "SQLite serialises" | **Corrected** in the design (`:128`), the dispositions, `checkBillingRetirement`, `ReapAccount` and the test comment. | n/a | |
| +| **Astra 2 (P3):** the rejected listing left a deleted account blank | **Closed** (`billingoperator.go:314`, `accountEvenIfDeleted`). | **It fails without the fix.** The last assertion of the handler test wants `account == sqid` after the deletion. | |
| + |
| +**Severity, from the code at `e152df30`: Astra's P2 was right.** |
| +- Within one console, a commit could not fail with a committed row present except on an I/O fault. `SetMaxOpenConns(1)` (`store.go:65`) serialises transactions, and `commitBillingRetirement` returns nil when it sees a committed row. In that case P3 would have been fair. |
| +- Across two consoles on one WAL database, the failure is deterministic, not a random fault: |
| + 1. B's deferred transaction reads. |
| + 2. A commits. |
| + 3. B's write upgrade returns `SQLITE_BUSY_SNAPSHOT`, which `busy_timeout` does not retry. |
| + 4. B's unconditional release deletes A's committed row. |
| +- At `e152df30`, the `errNoReleaseStore` branch also came after the commit. A console with no bucket deleted the committed row with no contention at all. |
| +- The protocol explicitly claims two consoles on one database, so the deletion handler reproduced the P2 state. |
| + |
| +**Is the committed marker durable now?** Yes, on every cleanup path the handler has: |
| + |
| +| Cleanup path | What happens now | |
| +|---|---| |
| +| A failed commit | Releases only a pending row (`:176`). | |
| +| No bucket | Checked before the commit (`:159-168`), and releases only a pending row. | |
| +| A tombstone that fails after the commit | Releases nothing (`:198-209`). | |
| + |
| +The only other writer that deletes from `billing_retirements` is `cancelPendingRetirementTx`, which is also pending-only. |
| + |
| +**Can any handler still release a reservation it cannot know is its own?** Yes, but only a pending one, and that is safe. Reservations share one row per account (`INSERT OR IGNORE`), so "its own" has no meaning for a pending row. If request B's cleanup removes request A's pending row: |
| +- A's commit then reads `!reserved` and refuses with `errBillingRetirement`. |
| +- It does so before anything outside the database is written. |
| +- The commit re-reads every condition in one transaction, so a row re-made by a third request is judged afresh. |
| + |
| +The only cost is liveness: A's user sees "Could not delete this account". |
| + |
| +## Findings |
| + |
| +### P3-1. The column migration is not atomic |
| +- **Where:** `billingretirement.go:184-197`. The `ALTER` and the `UPDATE` are two separate `Exec`s, outside the transaction that follows. |
| +- **Scenario 1 (crash):** |
| + 1. The process dies after the `ALTER` and before the `UPDATE`. |
| + 2. At the next start, `hasCommitted=1`, so the `UPDATE` never runs. |
| + 3. An older build's kept reservation now reads as pending. |
| + 4. Evidence withdraws it and checkout reopens on a possibly tombstoned account. That is exactly P3-3. |
| +- **Scenario 2 (two consoles starting together):** console B finishes its migration and serves a deletion's pending reservation in the window between A's `ALTER` and A's `UPDATE`. A's `UPDATE` then marks B's pending row committed. The window is microseconds. |
| +- **Scenario 3 (the `e152df30` gap):** a database that ran `e152df30` already has the column with no backfill, so the backfill never runs. Billing is off and `e152df30` was never rolled, so this only needs saying in the roll rule. |
| +- **Fix:** run the `pragma_table_info` check, the `ALTER` and the `UPDATE` in one transaction (SQLite DDL is transactional), and state the `e152df30` exception in the roll rule. |
| + |
| +### P3-2. The no-bucket preflight's release is untested |
| +- **Where:** `ownership.go:159-168`, with `TestDeletionRefusedWhenTheBucketIsUnreachable` (`deletion_test.go:118`). |
| +- **Scenario:** move the `Releases == nil` check back after the commit, or delete the release call. The test still passes, because it asserts only that the account survives. |
| +- **Effect:** the live account keeps a committed reservation. Checkout is blocked at `billingstore.go:363` for good, and no retry on that console can finish. |
| +- **Fix:** assert in that test that no reservation row remains, and that a checkout `beginBillingCheckout` call afterwards is not refused. |
| + |
| +### P3-3. The surviving mutant can be ruled out at compile time |
| +- **Where:** `billingstore.go:1605-1614`. |
| +- **Scenario:** `releaseBillingRetirement` stays in production code "for tests and for an operator's repair by hand". There is no operator door for that repair. AGENTS.md's API/CLI rule makes that a repair by direct database access, not a supported path. A future edit, or the round's surviving mutant, can call it from a handler with green tests. |
| +- **Fix:** move it into a `_test.go` file. The mutant then cannot compile. |
| + |
| +## Claims the code contradicts |
| + |
| +1. **`ownership.go:614-624`.** The comment on `tombstoneAccount` and `errNoReleaseStore` still says the caller tells this refusal apart "so … release the retirement reservation". The handler now checks for it before the commit. If `tombstoneAccount` ever returned it after the commit, the handler would keep the committed row, which is the right thing but the opposite of what the comment says. |
| +2. **The P3-3 disposition and the design (`:128`)** say "a reservation from before the commit column is marked committed when the column is added". That holds only on a database that never ran `e152df30`, and only if the process does not die between the two statements (P3-1). |
| + |
| +The QQQQ wording is addressed below. |
| + |
| +## PPPP: the deletion handler, step by step |
| + |
| +| Step | Another console's checkout evidence | Another deletion request | This process dies here | |
| +|---|---|---|---| |
| +| Preparation (`resolveIntentForDeletion`) | Recorded under the protocol as usual. The reservation then refuses on the open review or the intent. | Each runs its own preparation through the protocol. | Nothing is reserved and nothing outside is touched. | |
| +| Reserve (`:151`) | Lands after: `fileBillingReviewTx` withdraws the pending row in the same transaction, then the commit refuses. | Shares the row (`INSERT OR IGNORE`). | A pending row stays. Checkout (`:363`) and month freezes (`:1400`) are blocked until evidence withdraws it or a retry commits. The owner is not told why. This is the same as in older builds, not new. | |
| +| No-bucket preflight (`:159`) | As for reserve. | Releases only pending rows, so another request's committed retirement is untouched. | As for reserve. | |
| +| Commit (`:172`) | Evidence first: the commit refuses and nothing outside is written. Commit first: the evidence becomes a dispute that `DeleteAccount` and `ReapAccount` pass, listed as `account_deleted` and held with a 409. | The second request finds the row committed and gets nil, then tombstones (idempotent: `mint.go:245`). Its `DeleteAccount` may fail cosmetically after the first succeeds. | The committed row stays and the account is live. Checkout is blocked. Evidence lands as a dispute on a live account, visible and held. A retry finishes the deletion: reserve passes the review clause, the commit returns nil, then tombstone and delete. | |
| +| Tombstone (`:198`) | As after the commit. | Idempotent, with one compare-and-swap retry. | The manifest is retired and the row is committed. The next materialize runs `ReapAccount`, which passes reviews. After the commit no intent, open month or active subscription can arise, so the reap finishes. | |
| +| Local delete (`:211`) | As after the commit. | As above. | Same as dying after the tombstone. | |
| + |
| +In every state, a later retry or reap finishes the deletion. Nothing between the commit and the local delete reopens checkout. |
| + |
| +## QQQQ: the surviving mutant |
| + |
| +**True, with one correction of wording.** |
| +- Within one console, the commit cannot fail with a committed row present except on a database fault, for the serialisation reason above. |
| +- Across two consoles, "a transient database error" is the deterministic `SQLITE_BUSY_SNAPSHOT` of the exact race Astra described. It is not a random fault. |
| + |
| +**Acceptable**, because the handler's code is right by inspection and the store function it calls is tested. Either P3-3's move, or a test with two `*Store` handles opened on one WAL file, would kill the mutant. The two-handle test does not need two processes. Its order: |
| +1. B begins, then reads. |
| +2. A commits. |
| +3. B's update fails. |
| +4. Run the handler's cleanup. |
| +5. Assert the marker survives. |
| + |
| +## RRRR: what would make me say yes |
| + |
| +**For this binary, yes: review eighteen's P2 is closed.** No interleaving I can construct from the code deletes an intent while its session is payable or a subscription is outstanding. None releases a hold with money outstanding, or tombstones an account whose local deletion then refuses. |
| + |
| +To close it without qualification, I would want these two additions: |
| +- **Sequential tests are not two connections.** Add the two-handle WAL test above for the three orderings: |
| + 1. reserve, then evidence, then commit; |
| + 2. commit, then evidence, then delete; |
| + 3. a losing commit's cleanup. |
| +- **The mutant is alive.** Make P3-3's move, so the unconditional release cannot be called from a handler. |
| + |
| +What stands outside the branch: |
| +- **The roll rule:** an older binary has no commit step and its release is unconditional, so older consoles must be stopped before this build starts and never rolled back. |
| +- **Stripe test-mode item 1:** whether a subscription can exist before its session completes. This decides how often honest buyers meet the hold, but not whether the hold is safe. |
| +- **Stage two:** a disposition for disputes. They are held by design. |
| + |
| +## OOOO: required before acceptance? |
| + |
| +No code is required. P3-1 to P3-3 and the two claims are fixes to make, not gates. What remains: |
| +- decision 15; |
| +- the test-mode rehearsal and its first-check gate; |
| +- the roll rule, now also naming the `e152df30` backfill exception; |
| +- the owner's approvals. |
| + |
| +## What to verify in Stripe test mode before enabling |
| + |
| +This delta adds no provider dependency. The list is the one from round twenty-four, unchanged: |
| +1. **Subscription before completion:** whether Checkout creates a Subscription before the session completes, under abandoned 3-D Secure, a declined card followed by another, and a delayed debit. |
| +2. **`incomplete_expired` remains:** the invoices, charges and PaymentIntents such a subscription leaves. |
| +3. **Failed debits:** SEPA and Bacs through a failed debit. |
| +4. **Event order:** `customer.subscription.created` against `checkout.session.completed`. |
| +5. **Metadata edits:** whether a session's or a subscription's metadata can be edited after creation, and by whom. |
| +6. **Canceled subscriptions:** what can still change on one. |
| +7. **Catalog identities:** all 36 prices with exact currencies, own-only `currency_options`, and Adaptive Pricing disabled. |
| +8. **The GBP purchase:** the one-form purchase at `?country=IN¤cy=gbp` from end to end. |
| +9. **Expiry against payment:** expiry racing a payment, with idempotency keys and the fresh-read proof. |
| +10. **Lost-create recovery:** recovery by listing, including pagination and the per-element decode. |
| +11. **GBP overages and quantity:** invoices, their recovery, and proration. |
| +12. **Legacy EUR:** request hashes and customer reuse unchanged. |
| + |
| +**Verdict: accept with fixes.** The fixes are P3-1 to P3-3 and the two claims. Before enabling billing: decision 15, the rehearsal gate and the roll rule. |
diff --git a/internal/console/billingnamed_test.go b/internal/console/billingnamed_test.go| index 2aa2c5ab8..bb2f9c31d 100644 |
| --- a/internal/console/billingnamed_test.go |
| +++ b/internal/console/billingnamed_test.go |
| @@ -1341,6 +1341,38 @@ func TestAReservationFromBeforeTheCommitColumnIsCommitted(t *testing.T) { |
| if err := fx.st.db.QueryRow(`SELECT committed_at FROM billing_retirements WHERE account_id=?`, acct.ID).Scan(&committed); err != nil || committed != f.now.Unix() { |
| t.Fatalf("the old reservation after the column was added: committed_at %d %v; want its start time", committed, err) |
| } |
| + // The column and the marking are one transaction: a marking that |
| + // fails (a trigger here; a death between the two statements in life) |
| + // leaves no column behind to make the next start skip the marking. |
| + fx2, f2, acct2 := newCurrencyFixture(t) |
| + for _, q := range []string{ |
| + `DROP TRIGGER IF EXISTS billing_refused_intent_delete_barrier`, |
| + `ALTER TABLE billing_retirements DROP COLUMN committed_at`, |
| + `CREATE TRIGGER test_no_backfill BEFORE UPDATE ON billing_retirements BEGIN SELECT RAISE(ABORT, 'test_no_backfill'); END`, |
| + } { |
| + if _, err := fx2.st.db.Exec(q); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + if _, err := fx2.st.db.Exec(`INSERT INTO billing_retirements(account_id,started_at) VALUES(?,?)`, acct2.ID, f2.now.Unix()); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := fx2.st.migrateBillingRetirement(); err == nil || !strings.Contains(err.Error(), "test_no_backfill") { |
| + t.Fatalf("the migration whose marking fails: %v; want it to fail", err) |
| + } |
| + var hasColumn int |
| + if err := fx2.st.db.QueryRow(`SELECT count(*) FROM pragma_table_info('billing_retirements') WHERE name='committed_at'`).Scan(&hasColumn); err != nil || hasColumn != 0 { |
| + t.Fatalf("after the failed marking the column exists (%d, %v); want the addition rolled back with it", hasColumn, err) |
| + } |
| + if _, err := fx2.st.db.Exec(`DROP TRIGGER test_no_backfill`); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := fx2.st.migrateBillingRetirement(); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := fx2.st.db.QueryRow(`SELECT committed_at FROM billing_retirements WHERE account_id=?`, acct2.ID).Scan(&committed); err != nil || committed != f2.now.Unix() { |
| + t.Fatalf("the old reservation after the retried migration: committed_at %d %v; want its start time", committed, err) |
| + } |
| // Run again, it changes nothing; and a new reservation is pending. |
| if err := fx.st.migrateBillingRetirement(); err != nil { |
| t.Fatal(err) |
| @@ -1355,3 +1387,126 @@ func TestAReservationFromBeforeTheCommitColumnIsCommitted(t *testing.T) { |
| t.Fatalf("a new reservation after a later start: committed_at %d %v; want pending", committed, err) |
| } |
| } |
| + |
| +// Two consoles are two connections to one database, not two steps of one. |
| +// A second store handle on the fixture's file plays the other console for |
| +// the three orderings at the retirement's point of no return: evidence |
| +// before the commit withdraws it; evidence after the commit lands on the |
| +// retiring account; a losing request's commit fails under the winner's |
| +// and its cleanup leaves the winner's committed row. (Review twenty-five, |
| +// Opus RRRR.) |
| +func TestTwoConsolesAtTheRetirementsPointOfNoReturn(t *testing.T) { |
| + second := func(t *testing.T, fx *consoleFixture) *Store { |
| + t.Helper() |
| + var file string |
| + if err := fx.st.db.QueryRow(`SELECT file FROM pragma_database_list WHERE name='main'`).Scan(&file); err != nil || file == "" { |
| + t.Fatalf("the fixture's database file: %q %v", file, err) |
| + } |
| + other, err := Open(file) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + t.Cleanup(func() { other.Close() }) |
| + return other |
| + } |
| + ended := func(t *testing.T) (*consoleFixture, *fakeBillingStripe, *Account, billingIntent) { |
| + t.Helper() |
| + fx, f, ops := newCurrencyFixture(t) |
| + if err := fx.st.CreateAccount(ops.ID+100, "cdef", "Other", "paul@keymail.dev", time.Now()); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := fx.st.AddMember(ops.ID+100, "paul@keymail.dev", RoleOwner, time.Now()); err != nil && !strings.Contains(err.Error(), "already") { |
| + t.Fatal(err) |
| + } |
| + acct, _ := fx.st.AccountBySqid("cdef") |
| + f.customerCountry = "GB" |
| + f.mu.Lock() |
| + f.sqid = "cdef" |
| + f.mu.Unlock() |
| + if dest, err := fx.srv.startBillingCheckout(context.Background(), acct, plans.PlanPersonal, "GB", "", "", f.now); err != nil || dest == "" { |
| + t.Fatalf("first click: dest %q err %v", dest, err) |
| + } |
| + intent, _ := fx.st.billingCheckoutIntent(acct.ID) |
| + f.mu.Lock() |
| + f.sessionStatus, f.subscriptionPrice = "complete", intent.Price |
| + f.mu.Unlock() |
| + if code := f.webhookPost(t, fx, "evt_own_two", "customer.subscription.created", "sub_paid"); code != http.StatusNoContent { |
| + t.Fatalf("the purchase: %d", code) |
| + } |
| + f.mu.Lock() |
| + f.subscriptionStatus = "canceled" |
| + f.mu.Unlock() |
| + if err := fx.srv.reconcileBillingSubscription(context.Background(), "sub_paid", "", f.now.Add(time.Hour)); err != nil { |
| + t.Fatal(err) |
| + } |
| + return fx, f, acct, intent |
| + } |
| + t.Run("evidence on the other console before the commit", func(t *testing.T) { |
| + fx, f, acct, intent := ended(t) |
| + other := second(t, fx) |
| + if err := fx.st.reserveBillingRetirement(acct.ID, f.now.Add(2*time.Hour)); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := other.noteSubscriptionSeen("cdef", intent.ID, "sub_hand", f.now.Add(2*time.Hour)); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := fx.st.commitBillingRetirement(acct.ID, f.now.Add(2*time.Hour)); !errors.Is(err, errBillingRetirement) { |
| + t.Fatalf("the commit after the other console's evidence: %v; want it refused", err) |
| + } |
| + }) |
| + t.Run("evidence on the other console after the commit", func(t *testing.T) { |
| + fx, f, acct, intent := ended(t) |
| + other := second(t, fx) |
| + if err := fx.st.reserveBillingRetirement(acct.ID, f.now.Add(2*time.Hour)); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := fx.st.commitBillingRetirement(acct.ID, f.now.Add(2*time.Hour)); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := other.noteSubscriptionSeen("cdef", intent.ID, "sub_hand", f.now.Add(2*time.Hour)); err != nil { |
| + t.Fatal(err) |
| + } |
| + if n := disputesOf(t, fx, acct, intent.SessionID, "%"); n != 1 { |
| + t.Fatalf("%d disputes; want the other console's evidence recorded", n) |
| + } |
| + if err := fx.st.DeleteAccount(acct.ID, f.now.Add(3*time.Hour)); err != nil { |
| + t.Fatalf("the deletion after the commit: %v", err) |
| + } |
| + }) |
| + t.Run("a losing commit on the other console and its cleanup", func(t *testing.T) { |
| + fx, f, acct, _ := ended(t) |
| + other := second(t, fx) |
| + if err := fx.st.reserveBillingRetirement(acct.ID, f.now.Add(2*time.Hour)); err != nil { |
| + t.Fatal(err) |
| + } |
| + // The other console's commit has read the reservation and is |
| + // about to write; this console commits first; the other's write |
| + // fails on its stale snapshot, and its cleanup runs. |
| + tx, err := other.db.Begin() |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + var reserved bool |
| + if err := tx.QueryRow(`SELECT EXISTS(SELECT 1 FROM billing_retirements WHERE account_id=?)`, acct.ID).Scan(&reserved); err != nil || !reserved { |
| + t.Fatalf("the other console's read: %v %v", reserved, err) |
| + } |
| + if err := fx.st.commitBillingRetirement(acct.ID, f.now.Add(2*time.Hour)); err != nil { |
| + t.Fatal(err) |
| + } |
| + _, werr := tx.Exec(`UPDATE billing_retirements SET committed_at=? WHERE account_id=?`, f.now.Add(2*time.Hour).Unix(), acct.ID) |
| + tx.Rollback() |
| + if werr == nil { |
| + // Some SQLite builds let the stale writer through once the |
| + // first writer is done; then the row is committed either |
| + // way, and the cleanup below is what the test is about. |
| + t.Log("the other console's write went through after the first commit") |
| + } |
| + if err := other.releasePendingBillingRetirement(acct.ID); err != nil { |
| + t.Fatal(err) |
| + } |
| + var committed int64 |
| + if err := fx.st.db.QueryRow(`SELECT committed_at FROM billing_retirements WHERE account_id=?`, acct.ID).Scan(&committed); err != nil || committed == 0 { |
| + t.Fatalf("after the losing console's cleanup: committed_at %d %v; want the winner's commit kept", committed, err) |
| + } |
| + }) |
| +} |
diff --git a/internal/console/billingretirement.go b/internal/console/billingretirement.go| index ab65e5c20..9bac4901e 100644 |
| --- a/internal/console/billingretirement.go |
| +++ b/internal/console/billingretirement.go |
| @@ -181,19 +181,34 @@ func (s *Store) migrateBillingRetirement() error { |
| // the column exists was kept by an earlier build after an ambiguous |
| // failure outside, with the tombstone possibly written: it is a |
| // committed one, and is marked so once, when the column is added. |
| - var hasCommitted int |
| - if err := s.db.QueryRow(`SELECT count(*) FROM pragma_table_info('billing_retirements') WHERE name='committed_at'`).Scan(&hasCommitted); err != nil { |
| - return err |
| - } |
| - if hasCommitted == 0 { |
| + // The check, the column and the marking are one transaction (SQLite's |
| + // DDL is transactional): a death between the column and the marking, |
| + // or a second console starting in that window, cannot leave an old |
| + // reservation reading as pending. |
| + if err := func() error { |
| + tx, err := s.db.Begin() |
| + if err != nil { |
| + return err |
| + } |
| + defer tx.Rollback() |
| + var hasCommitted int |
| + if err := tx.QueryRow(`SELECT count(*) FROM pragma_table_info('billing_retirements') WHERE name='committed_at'`).Scan(&hasCommitted); err != nil { |
| + return err |
| + } |
| + if hasCommitted != 0 { |
| + return nil |
| + } |
| for _, stmt := range []string{ |
| `ALTER TABLE billing_retirements ADD COLUMN committed_at INTEGER NOT NULL DEFAULT 0`, |
| `UPDATE billing_retirements SET committed_at=started_at WHERE committed_at=0`, |
| } { |
| - if _, err := s.db.Exec(stmt); err != nil { |
| + if _, err := tx.Exec(stmt); err != nil { |
| return err |
| } |
| } |
| + return tx.Commit() |
| + }(); err != nil { |
| + return err |
| } |
| tx, err := s.db.Begin() |
| if err != nil { |
diff --git a/internal/console/billingretirement_export_test.go b/internal/console/billingretirement_export_test.go| new file mode 100644 |
| index 000000000..0bfdc2f95 |
| --- /dev/null |
| +++ b/internal/console/billingretirement_export_test.go |
| @@ -0,0 +1,11 @@ |
| +package console |
| + |
| +// releaseBillingRetirement removes a reservation, committed or not. It is |
| +// a test's tool for building a state: no handler may call it, because a |
| +// request can never know that a committed reservation is its own and not |
| +// another request's whose tombstone is being written, and in production |
| +// code it does not exist. |
| +func (s *Store) releaseBillingRetirement(accountID int64) error { |
| + _, err := s.db.Exec(`DELETE FROM billing_retirements WHERE account_id=?`, accountID) |
| + return err |
| +} |
diff --git a/internal/console/billingstore.go b/internal/console/billingstore.go| index 31fc21c6d..52a6e113d 100644 |
| --- a/internal/console/billingstore.go |
| +++ b/internal/console/billingstore.go |
| @@ -1602,17 +1602,6 @@ func (s *Store) reserveBillingRetirement(accountID int64, now time.Time) error { |
| return tx.Commit() |
| } |
| -// releaseBillingRetirement removes a reservation, committed or not. No |
| -// handler calls it: a request can never know that a committed reservation |
| -// is its own and not another request's whose tombstone is being written. |
| -// It remains for tests and for an operator's repair by hand. An ambiguous failure AFTER a write may have begun |
| -// must keep the reservation, so that path never calls this: a retry can then |
| -// finish retirement, but no second checkout can open in the meantime. |
| -func (s *Store) releaseBillingRetirement(accountID int64) error { |
| - _, err := s.db.Exec(`DELETE FROM billing_retirements WHERE account_id=?`, accountID) |
| - return err |
| -} |
| - |
| // releasePendingBillingRetirement releases a reservation that was not |
| // committed. It is what a failed commit calls: the failure may be another |
| // request's commit having won (two tabs, two consoles), whose committed |
diff --git a/internal/console/deletion_test.go b/internal/console/deletion_test.go| index 471c9c1ee..affe72fed 100644 |
| --- a/internal/console/deletion_test.go |
| +++ b/internal/console/deletion_test.go |
| @@ -16,6 +16,7 @@ import ( |
| "github.com/carlosframework/platform/internal/domains" |
| "github.com/carlosframework/platform/internal/mail" |
| "github.com/carlosframework/platform/internal/objstore" |
| + "github.com/carlosframework/platform/internal/plans" |
| "github.com/carlosframework/platform/internal/release" |
| ) |
| @@ -128,6 +129,17 @@ func TestDeletionRefusedWhenTheBucketIsUnreachable(t *testing.T) { |
| if a, _ := fx.st.AccountBySqid(sqid); a == nil { |
| t.Fatal("the account was deleted locally even though the bucket could not be told") |
| } |
| + // Nothing was committed and no reservation remains: the account goes |
| + // on as it was, and can open a checkout. |
| + a, _ := fx.st.AccountBySqid(sqid) |
| + var reserved int |
| + if err := fx.st.db.QueryRow(`SELECT count(*) FROM billing_retirements WHERE account_id=?`, a.ID).Scan(&reserved); err != nil || reserved != 0 { |
| + t.Fatalf("%d reservation(s) left by a refused deletion (%v); want none", reserved, err) |
| + } |
| + request := billingIntent{AccountID: a.ID, Plan: plans.PlanPersonal, Terms: plans.CurrentTermsVersion, Band: "A", Price: "price_personal", Quantity: 1, Members: 1} |
| + if _, err := fx.st.beginBillingCheckout(request, time.Now()); err != nil { |
| + t.Fatalf("a checkout after the refused deletion: %v; want it allowed", err) |
| + } |
| } |
| func readManifest(t *testing.T, s release.Store, sqid string) domains.AccountManifest { |
diff --git a/internal/console/ownership.go b/internal/console/ownership.go| index f8fe24745..e5578aba0 100644 |
| --- a/internal/console/ownership.go |
| +++ b/internal/console/ownership.go |
| @@ -617,10 +617,10 @@ func purgeAdvice(err error, app string) string { |
| // and says so: the alternative is a console-only deletion that undoes itself, |
| // which is worse than a refusal because it looks like it worked. |
| // errNoReleaseStore is returned by tombstoneAccount when no release store is |
| -// configured, so the caller can tell this DEFINITE pre-write refusal (nothing |
| -// external was touched) apart from an ambiguous failure after a bucket write |
| -// may have begun. The former is safe to unwind — release the retirement |
| -// reservation — while the latter must keep the reservation for recovery. |
| +// configured. The deletion handler checks for that before the retirement's |
| +// commit (and releases only its pending reservation then); after the |
| +// commit every failure of the tombstone, this one included, keeps the |
| +// committed reservation, because a bucket write may have begun. |
| var errNoReleaseStore = errors.New("console: no deployment release store is configured on this box") |
| func (s *Server) tombstoneAccount(ctx context.Context, acct *Account) error { |