amadan
Retirement protocol: review twenty-six's fixes (the losing commit must lose, the evidence's withdrawal asserted, three comments and two design sentences corrected)
Review twenty-six is the first in which both reviewers accept: Opus accepts (no P1 or P2, review eighteen's P2 closed on this branch), Astra accepts with fixes (one P3, seven claims). The fixes: The two-console test's losing-writer ordering required nothing of the stale write; it now requires SQLITE_BUSY_SNAPSHOT (517), so the other console's commit is the losing one the test names, and the test fails if the other console's read is removed. Its first ordering asserted the commit's refusal, which the open review gives anyway; it now asserts that the evidence withdrew the pending reservation, and fails if fileBillingReviewTx stops calling cancelPendingRetirementTx. Four mutants in a tree copy, all die (currency-mutants-round26.txt). Comments: a reservation is one row per account, so the release is the account's pending one, not "this request's own"; a filing after the intent is gone classifies against the archive and files a dispute where the evidence contradicts the recorded fate, rather than "nothing"; the refusal retries are one, five and fifteen minutes, then an hour after each try from the fourth. The design says no test runs two console processes (one runs two connections to one file) and names both exceptions to "a fate comes from a read". The dispositions no longer say Astra's A/B/C ordering is the one the test drives, nor that the round-twenty-four mutant needed a database fault; a section for review twenty-six and both reports are in the tree.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
8 files changed,
+439
−18
docs/superpowers/specs/2026-10-02-intent-retirement-protocol-design.md+2 −2docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review26.md+262 −0docs/superpowers/specs/DISPOSITIONS-multi-currency.md+17 −2docs/superpowers/specs/OPUS-REVIEW-multi-currency-review26.md+130 −0internal/console/billingnamed_test.go+13 −5internal/console/billingretirement.go+5 −4internal/console/billingstore.go+6 −3internal/console/ownership.go+4 −2
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 c9faefe37..3061eafb7 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 |
| @@ -110,14 +110,14 @@ Astra's third review (`CODEX-FINDINGS-intent-retirement-design-review3.md`) reje |
| ## Stage one as built |
| -Built in `internal/console/billingretirement.go` and the paths it touches, with tests in `billingretirement_test.go` and `billingevidence_test.go` for the sequences the reviews named (they are not one test per interleaving: no test runs two consoles, and a second actor is stood in for by a hook inside the fake provider or by an answer handed to the store by hand). Where the build differs from the text above, this is what it does and why. |
| +Built in `internal/console/billingretirement.go` and the paths it touches, with tests in `billingretirement_test.go` and `billingevidence_test.go` for the sequences the reviews named (they are not one test per interleaving: no test runs two console processes; one test, `TestTwoConsolesAtTheRetirementsPointOfNoReturn`, runs two connections to one database file, which is what two consoles are to SQLite short of process death, and elsewhere a second actor is stood in for by a hook inside the fake provider or by an answer handed to the store by hand). Where the build differs from the text above, this is what it does and why. |
| - **The evidence invariant** (added after round nineteen, which showed the first build of stage one recorded evidence at some call sites and returned early at others, and filed a refusal and its subscription in two transactions; made structural after round twenty, which showed the round-nineteen build still chose, call site by call site, whether and against which intent to record). Every read of a checkout session or a subscription from the provider is made by one of three methods (`readCheckout`, `listCheckouts`, `readSubscription`), and each hands what it read to the store before its caller sees it; a test parses the package and fails on any other use of the client's three reads. The store finds the purchase by what the console itself recorded, never by what the answer claims: a session by its id, against the standing intent bound to it or referenced to it by its review, the retirement record, or the kept resolution's evidence (`purchaseOfSessionTx`); a subscription by the account and intent it names. It classifies in one transaction against the purchase as it is at that instant, standing, retired, or kept and consumed (`noteSessionSeen`, `noteSubscriptionSeen`), judging identity against the recorded account, intent and customer (the archives keep the customer), and records the session's fate, an attached subscription, or a dispute. A refusal and the subscription that caused it are one transaction (`fileCheckoutRefusal`); filing for an intent that went meanwhile classifies the subscription against the archive in that same transaction. What this does not cover, on purpose: the answers to a create and to an expiry (replays under an idempotency key, which prove nothing about the present; every decision after one is taken on a read or on the store's own checks of the answer), a read before any refusal review exists (there is no refused purchase for it to be evidence about; the reads that then file the refusal are handed in again once it exists), and an identity-less or still-open read before any fate is recorded (nothing is recorded to contradict; with no fate both outcomes are held). |
| - **Where a refusal is filed that round nineteen's build answered bare** (review twenty, Opus P2-1): a subscription that names the bound standing intent and is not its completed session's own, on the purchase's own terms or not, files the refusal with itself attached whether or not a review existed; and one that names a standing intent never bound to a session files it with no session named. A completed session that names no subscription yet is a retry, as it is for the binder: it says nothing about which subscription is its own, and an honest purchase is not refused on it. |
| - **A dispute is not resolvable.** The resolution door refuses a `billing_checkout_fate_disputed` review, so its hold on the account's invoices stays. A kept intent that is disputed is not offered to the buyer again and does not record. |
| - **Events about a purchase that no longer stands are settled with the recorded account** (the binder: found by the session's id), in the rejected list, not retried for days; the one exception is the kept purchase's own completion delivered again, which is the account's recorded purchase. |
| - **No generation counter.** Revision 3 put a generation on the review, to be compared across an actor's provider work. Stage one needs none, and not because a caller's read is current (it may not be): every fact a resolution reads is monotonic and is read inside the resolution's own transaction, and every classification of evidence is made inside the transaction that records it. A fate is written once and never rewritten; an attached subscription is only ever added, and only while the intent stands; a dispute is only ever set; the review's protocol mark does not change while it is open. Evidence that contradicts a recorded fate is never discarded for arriving later or earlier, or for the caller having read the purchase before it changed: it is a dispute. |
| -- **A fate comes from a read, never from an expiry answer** (one exception, inside one transaction: the ordinary expiry retirement of an intent with no review writes `expired` from the proof it was handed, which may be an expiry answer, immediately before it deletes the row; the delete barrier reads it and nothing else can). `advanceRefusal` reads the pinned session, requests the expiry of a bound session still open, and reads again; only a read whose identity clauses all hold is recorded. A payment that beats the expiry shows as `complete` in that second read. |
| +- **A fate comes from a read, never from an expiry answer** (two exceptions, each inside one transaction: the ordinary expiry retirement of an intent with no review writes `expired` from the proof it was handed, which may be an expiry answer, immediately before it deletes the row, and the delete barrier reads it and nothing else can; and a create answer that shows its session completed or paid is recorded as a completion when the refusal it causes is filed, `refuseSession`, which only ever holds: a session recorded complete is never retired as expired). `advanceRefusal` reads the pinned session, requests the expiry of a bound session still open, and reads again; only a read whose identity clauses all hold is recorded. A payment that beats the expiry shows as `complete` in that second read. |
| - **Webhooks: evidence before acknowledgement, idempotently, not one transaction.** The binder and reconcile hand what they read to the store's classification before any answer (see the evidence invariant), each transaction changing nothing when repeated, and only then is the event recorded as rejected and seen. A crash between the two leaves the event unseen, the provider delivers it again, and the evidence writes are no-ops. A dispute is keyed by its session and its wording, so the same contradiction delivered twice files one review once. |
| - **`keep` does not re-run reconcile's judgement of the subscription.** The door requires the bound session to be the intent's own, the intent's purchase, complete, paid and carrying a subscription, and the store requires the recorded fate complete, nothing attached but the session's own subscription, and no dispute. A subscription that is still not the purchase is refused again by the next reconcile, which files the review's next occurrence: keep closes nothing for good. (The read-only judgement revision 3 proposed needs reconcile's checks factored out; not done.) |
| - **A stale observation after `keep`** (a pass that read a mismatch before the keep and files after it) reopens the review as a new occurrence while the intent still stands: one more hold, visible, never a silent record. A subscription other than the kept session's own that is seen while the kept intent still stands is attached and reopens the review the same way; seen after the purchase recorded and the intent was consumed, it is a dispute for the kept session. A dispute raised between the `keep` and the record stops the record as a conflict. |
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review26.md b/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review26.md| new file mode 100644 |
| index 000000000..5a60ecc46 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review26.md |
| @@ -0,0 +1,262 @@ |
| +**Review twenty-five: closure** |
| + |
| +All platform references below are at **35356ad0**. Site references are at **01b62f7**. This was source-only review: no files changed, tests run, or network commands issued. |
| + |
| +| Previous finding or request | Result at this commit | Would verification pass without the fix? | |
| +|---|---|---| |
| +| **Astra 1, P2 / Opus P3-1:** migration could leave old reservations pending | **Closed.** The column check, `ALTER`, and backfill share one transaction: `internal/console/billingretirement.go:188`. | **No.** The aborting trigger in `billingnamed_test.go:1347` makes the backfill fail, then asserts that the column also disappeared. The former autocommit implementation fails that assertion. | |
| +| **Astra 2, P3 / Opus P3-3:** the handler could regress to unconditional release | **Closed structurally.** Unconditional release exists only in `billingretirement_export_test.go:8`; both handler cleanup sites call pending-only release. | **Distinguish build from tests.** Reintroducing the call into the handler can still compile in a same-package test build, where the helper exists. An ordinary production build fails. The build gate, rather than a handler regression test, closes this mutant. | |
| +| **Opus P3-2:** no-bucket release was untested | **Closed.** `deletion_test.go:135` checks that the reservation disappeared; line 141 checks that checkout can subsequently start. | **No.** Removing release, or committing before this preflight, leaves the barrier and fails the assertions. | |
| +| **Opus RRRR addition:** two connections to one database | **Closed, with the P3 assertion weakness below.** `billingnamed_test.go:1398` opens another `Store` on the fixture’s actual database file. | Removing the commit/recheck or pending-only cleanup breaks the relevant safety assertions. The losing-writer subtest does **not**, however, require its write to fail. | |
| +| **Opus RRRR addition:** unconditional release outside production | **Closed**, as above. | Production build fails if a handler calls the test-only method. | |
| +| **Opus claim 1:** tombstone comment described release after `errNoReleaseStore` | **Corrected.** The no-bucket decision is before commitment; later failures retain the reservation. | Documentation correction; no independent test counterfactual. | |
| +| **Opus claim 2:** design omitted the migration’s crash window | **Corrected.** The design now specifies the same transaction, matching the implementation. | The migration regression distinguishes the former implementation. | |
| + |
| +**Astra’s P2 severity for the migration was right.** An old reservation may represent an ambiguous external tombstone write. Making it withdrawable can reopen checkout while account retirement is already underway. That is a safety-boundary failure, not merely a diagnostic or migration inconvenience. |
| + |
| +A second `Store` here **is a second console in the SQLite sense relevant to this race**: a separate database handle and connection, independent transactions and snapshots, sharing one WAL database and its single writer. Separate operating-system processes are unnecessary to establish that ordering. It is not a process-death test or a mixed-binary rollout test. |
| + |
| +The new test exercises Opus’s three named orders: |
| + |
| +1. Evidence on connection B before A commits. |
| +2. Evidence on B after A commits. |
| +3. B establishes a read snapshot, A commits, B attempts its write, then B cleans up. |
| + |
| +It does **not** exercise Astra’s different A/B/C sequence involving the no-bucket release of a shared pending reservation. The dispositions incorrectly say it does. |
| + |
| +**Findings** |
| + |
| +**P3 — The losing-writer test does not require the failure it claims to establish.** |
| + |
| +`internal/console/billingnamed_test.go:1496` |
| + |
| +The test accepts both a failed and a successful stale write, and accepts any error as the expected failure. Its explanation that some SQLite builds permit this stale-snapshot promotion is not a sound description of the WAL snapshot established here. |
| + |
| +Concrete counterexample: remove the transaction’s reservation read at line 1490. The second transaction no longer establishes the old snapshot; its update can succeed, be rolled back, and the test still passes. Thus the test still protects pending-only cleanup, but no longer establishes “a losing commit’s cleanup.” |
| + |
| +**Smallest acceptable fix:** require the write to fail with the driver’s expected SQLite busy/snapshot classification, then roll back and retain the existing committed-marker assertion. No production change is needed. |
| + |
| +**No new P1 or P2 found. Review eighteen’s P2 is closed on this branch.** The remaining P3 does not reopen it. That conclusion applies to this implementation under the stated rollout rule stopping older consoles; it is not a claim that the new schema makes arbitrary older binaries safe. |
| + |
| +The following are the material conclusions behind that assessment. |
| + |
| +**Money, quotes, cookies, and the public page — A–C, E–K, M–Q, U, Z** |
| + |
| +For the requested GBP purchase: |
| + |
| +1. `GET ?country=IN¤cy=gbp` selects India’s affordability band and an explicit GBP choice. The billing page signs the country, band, GBP, choice flag, account, book, session token, and member count: `billingpage.go:446`. |
| +2. The one form posts those quoted values alongside its actual controls: `templates/billing.html:44`. |
| +3. `billingFormChoice` requires a present currency field to contain exactly one sold, lowercase value. Empty, repeated, padded, uppercase, and unknown values fail: `billinghandlers.go:176`. |
| +4. Checkout compares the **effective** currency against the signed quote. A changed country, changed currency, forged quote, or invalid signature redirects to an account-local quote before checkout creation: `billinghandlers.go:71`. |
| +5. `startBillingCheckoutQuoted` compares the quoted membership count, and `beginBillingCheckout` checks membership, customer, held currency, and retirement again inside its transaction: `billingservice.go:452`, `billingstore.go:352`. |
| +6. The intent pins GBP and the book. For a one-member Personal purchase in India, the current book’s annual unit amount is **770 GBP minor units**. Country band changes allowances independently of currency. |
| +7. Checkout creation, ownership checks, binding, and subscription reconciliation preserve and verify those purchase terms: `billingservice.go:221`, `:249`, `:819`, `:1182`. |
| +8. `applyBillingStateDated` writes the account and assignment’s book/currency in the purchase transaction: `billingstore.go:677`. |
| +9. Invoice history reads those assignments; `AllowanceForMonth` derives the applicable allowance and price terms; the calculation uses GBP overage rates and floor: `billinginvoicestore.go:527`, `plans/month.go:146`, `billingcalculation.go:119`. |
| +10. Invoice creation/recovery and quantity changes verify the stored currency again: `billinginvoice.go:558`, `:884`, `:1004`; `billingquantity.go:1008`, `:1105`. |
| + |
| +I found no path in that chain silently falling back to EUR, converting money, or repricing an existing calculation using the current book. |
| + |
| +The quote signature is server-keyed, derived from the billing secret. Knowing the disclosed CSRF token and hidden fields does not let the buyer forge it. Another session, account, book, band, member count, or changed session token invalidates it. Reusing an unchanged genuine quote is allowed; a quote in another tab does not revoke it. The membership checks prevent an old member count being charged. |
| + |
| +Plan is a price input, but all offered plans are shown under the signed pricing context. Quantity is derived from membership and the plan minimum, rather than accepted as an arbitrary posted quantity. Postal code can change tax; the quoted subscription figures are explicitly before VAT, with tax determined at checkout. |
| + |
| +A missing currency control is not treated like a present empty control. Against a book console it means “no explicit choice,” so the effective country default must still agree with the signed quote. Against a legacy console it retains legacy behavior. Forging `currency_chosen` cannot authorize a different unseen amount. The surviving mutation that passes an already-matching default as an explicit choice can be economically equivalent; that is not evidence of a currency escape. |
| + |
| +The quote door checks session, CSRF, then membership and builds a fixed account billing path with encoded query values: `billinghandlers.go:137`. Posted values cannot make it an external redirect or a purchase operation. |
| + |
| +Cookie behavior matches the stated contract: present-empty clears, absent falls back, invalid does not overwrite. Consequently, an invalid parameter can leave an older cookie available on a later request where the parameter is absent. That is preservation, not resurrection of a cleared cookie. Checkout itself does not consult these cookies: they influence the rendered form, whose quote must pass validation. |
| + |
| +A previously purchased currency overrides URL and cookie suggestions and is offered alone. That includes **legacy EUR purchases without book columns**: `billingpage.go:291`. The transaction also rejects a stale request built before the account acquired a different customer or held currency. |
| + |
| +The refined calculation trigger matches the **positive paid spans** used by `AllowanceForMonth`, including supersession across both assignment tables: `billingstore.go:208`. Same-second supersession contributes zero; a free or inactive assignment contributes no paid span; renewal and quantity assignments terminate the preceding span at their effective instant. `period_end` is the recorded subscription’s paid-through boundary, not the next assignment’s effective time. |
| + |
| +I found neither a book-priced positive span that slips through the legacy-calculation barrier nor a valid legacy-only month refused by the refined trigger. `retireEarlierBarrierTrigger` confirms the replacement exists before dropping the old name in a transaction: `billingstore.go:1007`. A failed migration can leave **both** triggers temporarily, conservatively; it does not remove the old one while the replacement is missing. Successful restart is idempotent. An older binary can recreate the old trigger, which is another reason the rollback prohibition matters. |
| + |
| +The inclusions use the charged quantity’s pooled allowance divided into per-member figures: `billingpage.go:395`. The unavailable Personal row uses a valid one-member quotation and remains unavailable; teams above the minimum use their actual charged quantity. For the embedded terms, the shares are exact at the displayed precision, including fractional shares. This is a property of the current resource quantities, not a universal guarantee of three-decimal formatting for arbitrary future terms. Band A follows the approved sentence construction. |
| + |
| +Book parsing and validation reject duplicate/unknown structure, incomplete price matrices, inconsistent EUR legacy values, invalid rates, and arithmetic overflow: `plans/pricebook.go:152`, `:231`, `:547`, `:601`. Catalog configuration supports the required current matrix alongside complete configured historical catalogs. The provisioning checks accept at least the required keys and the price’s own currency option at its own amount; they do not require exactly 36 prices across the entire Stripe account. Its regression partly checks script text, rather than executing the printed jq program: `plans/provision_test.go:23`. |
| + |
| +The public export contains commercial prices, resources, bands, and country mappings; I found no customer or provider credentials disclosed. |
| + |
| +For **section K**, the site’s committed price book is byte-for-byte equal to this platform commit’s public export. Its renderer: |
| + |
| +- Preserves exact overage rates and validates whole-cent monetary formatting through `hack/money.mjs`. |
| +- Scales inclusions by band and checks the required exactness. |
| +- Uses the exported country list and each option’s default currency. |
| +- Always supplies both link parameters; currency is empty unless explicitly chosen, and country is the selected country or empty. |
| +- Requires exactly `PRICEBOOK_ALLOW_DRAFT=1` for the draft exception and uses the first-line marker/check in its build path. |
| +- Checks typed approved figures against exported values using whole-token matching. |
| + |
| +A locale-derived display is intentionally not a transmitted selection. With no country or currency selected, the console can consequently show a different default; that complies with ruling 9. |
| + |
| +The site tests check slot values and rendered output in each currency. They **cannot distinguish a literal identical to the slot’s current derived value** from derivation. The current test explicitly states that limitation at `test/pricing-pricebook.test.mjs:202`; a changed-book rebuild would distinguish it. |
| + |
| +**Ownership, evidence, recording, and resolution — D, L, R–DD, EE–CCC, MMM–ZZZ2** |
| + |
| +`ownSession` checks mode, intent metadata, client reference, account metadata, pinned customer, and bound session ID: `billingservice.go:273`. Purchase identity is then checked separately by `intentSession`: plan, band, terms, price terms, and currency. |
| + |
| +It is deliberately **not the first operation on every read**: evidence classification happens first. Classifying a contradictory answer against the console’s own archived identity is necessary to retain evidence; it does not authorize charging or binding that answer. |
| + |
| +The two create sites apply ownership before acting on a refused session returned in `billingstripe.Error.Session`. Unowned create/listing answers are not expired. A bound session is different: `ownBoundSession` can file its review and request expiry using the console’s **persisted bound ID**, despite contradictory response metadata. That is the right distinction. Leaving that known payable session open merely because its metadata changed would surrender authority the console already possesses; expiring an arbitrary returned ID would exceed that authority. |
| + |
| +Review filing precedes expiry. If expiry fails, the review survives and subsequent reads retry. If completion wins, a fresh read records completion rather than declaring expiry. A crash between review and expiry leaves an actionable durable hold. |
| + |
| +All checkout/subscription provider reads in the examined console paths pass through: |
| + |
| +| Read wrapper | Callers covered | |
| +|---|---| |
| +| `readCheckout`, `billingretirement.go:872` | Binder, reconciliation, pass, start/resume, operator keep, refusal advancement | |
| +| `listCheckouts`, `:891` | Lost-create recovery, including successfully decoded earlier pages when a later page fails | |
| +| `readSubscription`, `:909` | Reconciliation, invoice preflight, quantity worker | |
| + |
| +A recording failure prevents a successful object being delivered to the caller. Provider errors can return before recording because no successful read is available. An answer carrying a different requested ID is not attributed to the requested purchase. The listing wrapper records its successfully read objects before returning a listing error. |
| + |
| +The structural test catches selectors for the classified provider methods, including aliases and method values; the exported-method inventory catches additions to this client until classified. It is a source guard, not a proof against every possible future provider abstraction or every control-flow rewrite. |
| + |
| +The explicit evidence exclusions remain conservative: |
| + |
| +- Create/expiry responses are not generally fresh-read evidence; ordinary retirement validates expiry proof, and reviewed retirement requires the subsequent read. |
| +- A session read before refusal is not retained as refusal evidence unless it causes filing, when it is submitted again. |
| +- Identity-less/open reads before a fate supply no terminal proof, so resolution remains held. |
| +- An ordinary expired or never-created intent deleted before any named subscription was observed has no archive. A later subscription becomes a rejected event rather than a financial hold. |
| + |
| +That last exception includes **both** ordinary expiry and cleanup of an intent that never had a session. |
| + |
| +Session lookup prefers the standing bound purchase, then its referenced unbound case, then retirement, then kept resolution: `billingretirement.go:442`. Rebinding guards prevent normal operation from creating conflicting session ownership. Archived comparisons use recorded account/customer/intent identity, so edited metadata cannot transfer a dispute to the account it now names. |
| + |
| +Subscription classification records a named standing intent even before review. After retirement or consumption it compares with the relevant archive. The recorded subscription’s own renewal/cancellation and an agreeing replay do not become extra-subscription disputes. A different subscription naming a consumed intent does. |
| + |
| +The binder and reconcile now agree on rejecting mismatched purchase identity. Reconcile waits when the bound session is incomplete or does not yet name its subscription; the named subscription observation has already been recorded. That retry does not silently attach a hand-made subscription as the session’s own. |
| + |
| +A correct first purchase can record only through the transaction that consumes the exact standing intent, with no open refusal and no dispute: `billingstore.go:868`. Renewals of the already-recorded subscription do not consume another intent. If consumption fails, account changes, assignments, event insertion, entitlement work, and quantity-refresh work in that transaction roll back together. |
| + |
| +An exact replay of a recorded purchase does not create a new record. A retired intent cannot be used by the binder fast path to record a replacement purchase. Retirement checks are account-scoped; another account’s retirement is not sufficient. |
| + |
| +Resolution is keyed to the reviewed intent and occurrence. It archives retirement and session identity, marks the matching quantity events, and deletes only that intent in one transaction: `billingstore.go:1133`. Missing outcome means `retire`, including old clients and migrated resolution rows. Outcome participates in replay comparison and audit detail. |
| + |
| +`keep` checks the console’s bound session and refuses gone, unbound, unpaid, incomplete, mismatched, or disputed cases. **It does not certify the subscription’s full purchase identity.** A still-wrong subscription can reopen the review at reconciliation. This limitation is explicit in the governing design and does not allow a bad purchase to record. |
| + |
| +The shared `quantityOutcomesSQL` supplies the processing guard, startup classification, and obligation check: `billingquantityaudit.go:61`. The guard is replaced when its text differs. No startup heal remains that clears a gap merely because event outcomes now look valid; pre-history and clock-rollback holds survive. |
| + |
| +Rejected-event access requires an authenticated operator. Results are bounded to 200, newest first with a deterministic tie-breaker, and account lookup occurs after closing rows: `billingoperator.go:280`. Deleted accounts remain identifiable. These IDs are disclosed to operators, not ordinary members. Rejected events are not a complete ledger of every malformed HTTP request or every unowned create response. |
| + |
| +**Intent exits and retirement — DDD–LLL, AAAA–UUUU** |
| + |
| +The lifecycle invariant at this commit is: |
| + |
| +> An open refusal prevents ordinary recording and removal of its intent. Observed named subscriptions survive removal through a hold or archive classification. Reviewed retirement requires terminal proof and no outstanding obligation. Account retirement has a durable commit boundary; later evidence remains visible against the deleted account. |
| + |
| +Every production intent deletion or session-binding statement falls into these cases: |
| + |
| +| Statement | Paths and result | |
| +|---|---| |
| +| `billingstore.go:458`, `:461` — bind session | Checkout save and binder. Same-session replay is allowed; another purchase’s session or a conflicting review reference is refused. | |
| +| `:553` — ordinary expiry deletion | Click, pass, refusal cleanup. Validated expiry proof is required. A named subscription causes refusal/hold instead of deletion. | |
| +| `:604` — unready cleanup | Start, pass, deletion preparation. Ready/bound/reviewed cases are not discarded; named subscriptions cause a hold. | |
| +| `:890` — purchase consumption | First-purchase record only. Exact intent consumption, consumed archive, and disputes for additional subscriptions share the transaction. | |
| +| `:1208` — operator retirement | Exact reviewed intent, resolution gate, event disposition, and retirement archive share the transaction. | |
| +| Account deletion/reap | Account identity is soft-deleted; billing evidence and archives are retained. There is no cascade silently removing these intents/evidence. | |
| + |
| +Thus a subscription observed **before** ordinary cleanup prevents deletion; observed around consumption it is classified on one side or the other of the same serialized transaction; observed afterwards it is compared with the archive. The stated unarchived exceptions remain the two cases described above. |
| + |
| +`billing_consumed_intents` is written only when an intent was actually consumed. Its `INSERT OR REPLACE` is not permission to reuse an intent: intent creation uses a fresh random identity and consumption checks the standing row. Ordinary later events do not overwrite that archive. |
| + |
| +The refusal resolution gate reads facts inside its own transaction. Fate does not change; obligations accumulate; disputes persist; protocol eligibility is fixed while the review is open. Payment details can be refreshed, but do not turn a completed session into proof of safe retirement. No caller-side generation counter is needed for the authority this gate grants. |
| + |
| +A disputed, pre-protocol, attached-subscription, or paid-but-not-kept case remains held. Stage one offers no operator disposition for those disputes. The listing makes them discoverable; that is not the same as making them resolvable. |
| + |
| +The delete barrier is additional protection, not a substitute for the store gates or a compatibility guarantee for older binaries. Its drop/recreation is transactional. Old binaries lacking the account-retirement commit step remain unsafe during a mixed roll. |
| + |
| +The deletion handler’s current sequence is: |
| + |
| +| Step | Evidence, another request, or process death | |
| +|---|---| |
| +| Prepare resting intent — `ownership.go:148` | Reads/reconciles through the evidence paths. Failure cannot bypass the reservation’s obligation check. | |
| +| Reserve — `:151` | Pending reservation blocks checkout. New checkout evidence withdraws it and leaves a hold. Reservations are shared per account, not owned per request. | |
| +| No-bucket preflight — `:160` | Releases only pending state. Another request’s committed state survives. A pending shared reservation may disappear, forcing another request’s commit to fail safely. | |
| +| Commit — `:172` | Rechecks conditions transactionally. Evidence before commitment prevents it; evidence after commitment cannot withdraw it. | |
| +| Tombstone — `:198` | Failure or process death retains commitment, blocking checkout. A retry can finish the external operation. | |
| +| Local deletion — `:211` | Billing evidence survives under the deleted account. A tombstoned account can be reaped without treating a later review as authority to resurrect it. | |
| + |
| +After commitment, a new review is honestly a retiring/deleted account’s review. The listing supplies `account_deleted`, and resolution lookup can find deleted accounts: `billingoperator.go:206`, `:345`. The dispute still has no stage-one disposition. |
| + |
| +There are exactly four production statements that update/delete `billing_retirements`: |
| + |
| +| Location | Requirement/effect | |
| +|---|---| |
| +| `billingretirement.go:203` | During column introduction only, marks existing zero-valued reservations committed using `started_at`. | |
| +| `billingstore.go:1548` | Evidence cancellation deletes only `committed_at=0`. | |
| +| `billingstore.go:1578` | Commit changes pending zero to a positive timestamp. | |
| +| `billingstore.go:1610` | Cleanup deletes only `committed_at=0`. | |
| + |
| +**No production path withdraws a committed reservation.** |
| + |
| +SQLite rolls back `ALTER TABLE ADD COLUMN` with this transaction. Death before commit leaves neither the new column nor its backfill committed; death afterwards leaves both. If another console committed migration before this transaction’s schema read, the pragma sees the column and skips marking. A transaction that established an older snapshot cannot safely promote itself over that writer; failure aborts startup rather than exposing a partially migrated console. |
| + |
| +A second start leaves newly created pending reservations pending. This does not repair a database already left with the column but incomplete marking by an earlier faulty migration; the recorded rollout exception still needs to be respected. |
| + |
| +**Tests and the cost to an honest buyer** |
| + |
| +The migration regression, no-bucket regression, handler tombstone hook, and two-connection safety assertions discriminate their relevant fixes. The fake-provider hooks establish controlled transaction orderings, not simultaneous provider execution or process-death recovery. |
| + |
| +The four later tests addressing review twenty’s surviving mutants now isolate the named effects: individual identity clauses, reopening after keep before successful reconciliation can mask it, the first refusal’s fate from one provider read, and the click’s second contradictory read. The replacement-session/late-completion tests use distinct session IDs and assert that the retired purchase does not record. |
| + |
| +I cannot certify historical mutation-run counts from source. Review four now qualifies the important limitations: script-text checks, successful-migration ordering coverage, and a redundant exact-intent deletion predicate behind an earlier exact-intent lookup. Those qualifications matter; “every removed line must independently fail a test” would be false. |
| + |
| +For honest purchases: |
| + |
| +- A subscription event arriving before session completion is recorded and retried; subsequent matching completion can record normally. |
| +- Duplicate events and the same subscription delivered repeatedly do not manufacture extra obligations. |
| +- A failed payment, abandoned authentication, or delayed method only becomes the conservative dead-subscription hold if Stripe exposes a subscription naming the still-standing intent and that purchase then cannot complete. |
| +- A provider replacement with a different subscription ID can become an additional-subscription dispute. |
| +- A completed session without its subscription ID remains a retry; it is not prematurely declared a mismatched purchase. |
| + |
| +The latter provider sequences need rehearsal. I found no additional source-established regression for an honest buyer, relative to the stated earlier behavior, that depends only on a known ordinary completed-purchase sequence rather than those unverified provider behaviors. |
| + |
| +The refusal pass has separate reconciliation/refusal deadlines, a three-case refusal budget, and longest-due ordering. Both deadlines inherit caller cancellation: `billingpass.go:162`. Production shutdown currently ends the process rather than cancelling that caller context; the protocol tolerates interrupted steps. These bounds prevent one refused case monopolizing a turn, not an absolute throughput guarantee under unlimited incoming work. |
| + |
| +**Claims the code contradicts** |
| + |
| +1. **The dispositions claim Astra’s A/B/C ordering is tested.** |
| + `docs/superpowers/specs/DISPOSITIONS-multi-currency.md:558` describes pending-row removal by B, A’s failed commit, C’s new committed reservation, then A’s cleanup. `billingnamed_test.go:1476` instead uses a stale snapshot against one reservation. Correct the description; the structural production fix does not require pretending these are the same test. |
| + |
| +2. **The surviving handler mutant was not reachable only through a transient database error.** |
| + `DISPOSITIONS-multi-currency.md:545` is disproved by Astra’s deterministic shared-reservation sequence above. The mutant is now structurally closed, but the historical explanation should say so explicitly. |
| + |
| +3. **Pending reservations are not request-owned.** |
| + `internal/console/ownership.go:158` says “only this request’s own pending reservation.” The schema and release predicate identify the account and pending state, not the request. Say “the account’s pending reservation.” |
| + |
| +4. **Late refusal filing does not always file nothing after the intent disappears.** |
| + The comment at `internal/console/billingstore.go:1216` is superseded by the consumed-intent dispute path at line 1263. Update it to describe archive classification. |
| + |
| +5. **The backoff comment advances to an hour one attempt too soon.** |
| + `internal/console/billingretirement.go:1002` says an hour after three tries. The update at line 833 uses the old counter: successive unsuccessful attempts schedule approximately **1, 5, 15, then 60 minutes**. |
| + |
| +6. **“No test runs two consoles” now needs qualification.** |
| + The design at `2026-10-02-intent-retirement-protocol-design.md:113` should say no test runs two console **processes**. There is now a valid two-connection SQLite test. |
| + |
| +7. **“Fate comes from a read” has more than the one exception named in the design paragraph.** |
| + Besides ordinary expiry retirement, the header explicitly permits a refused create answer showing completion/payment to record completion as a conservative hold: `billingretirement.go:79`, `billingservice.go:366`. Align the design’s exception wording with that code. |
| + |
| +These are accuracy corrections, not evidence of another lost-obligation path. |
| + |
| +**What must be verified against Stripe test mode before enablement** |
| + |
| +| Run | What it must establish | |
| +|---|---| |
| +| Read back all 36 catalog prices, including expanded `currency_options` | Exact book amounts, currency, interval, tax behavior, metadata, and acceptance of only the own-currency option shape. | |
| +| Complete USD/EUR/GBP purchases, including India + explicit GBP and default-country purchases | Correct form-to-session selection; Adaptive Pricing disabled; session, subscription, customer handling, and recorded assignments agree. | |
| +| Repeat with legacy EUR customers and customers whose Stripe default differs | A held currency remains usable without silent conversion or permanent refusal from provider customer behavior. | |
| +| Exercise successful card, decline/retry, abandoned 3-D Secure, and delayed-payment flows | Whether subscriptions exist before completion, retain their IDs, or get replaced; identify every flow entering a stage-one hold. This is the first protocol enablement gate. | |
| +| Race expiry against payment and repeat the expiry idempotency key | Fresh GET establishes expired versus complete; replayed expiry responses are not mistaken for present state; payment cannot be discarded as expiry. | |
| +| Deliver checkout/subscription/invoice events out of order and repeatedly | One legitimate purchase records once; missing session subscription IDs retry; old/retired completions settle without recording replacements. | |
| +| Lose create responses and force paginated recovery, including later-page failure | Original-key recovery, customer scoping, identity fields, partial-page evidence retention, and no duplicate payable checkout. | |
| +| Alter test-object metadata/customer/purchase identity where Stripe permits it | Provider read shapes match the ownership assumptions; bound expiry uses the persisted ID; unowned responses are not expired. | |
| +| Run GBP and other-currency quantity additions, renewal, cancellation, and late old-subscription events | Same-currency proration and verification; renewals consume no unrelated intent; expected event ordering does not create false first-purchase reviews. | |
| +| Generate overage invoices below/at/above floors, interrupt each creation/recovery stage, then retry | Exact rates, rounding, stored-currency lines/totals, request-hash stability, and recovery without conversion or duplicate invoices. | |
| +| Rehearse reviewed retire/keep, provider failure, restart, and post-retirement evidence | Durable hold until proof; exact resolution replay; paid/attached/disputed cases remain held and visible, including after account deletion. | |
| + |
| +Nothing further in the production code or tests is required to close **review eighteen’s P2**. Before accepting this round without qualifications, fix the P3 assertion and the inaccurate claims above. Enabling billing separately requires the provider rehearsal, the documented rollout restrictions, and the owner’s outstanding disposition decisions and approvals. |
| + |
| +**Verdict: accept with fixes.** |
| \ 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 659f3a0db..c28af3cfb 100644 |
| --- a/docs/superpowers/specs/DISPOSITIONS-multi-currency.md |
| +++ b/docs/superpowers/specs/DISPOSITIONS-multi-currency.md |
| @@ -542,7 +542,7 @@ Reports: `OPUS-REVIEW-multi-currency-review24.md`, `CODEX-FINDINGS-multi-currenc |
| | Opus P3-4: a doc comment detached by an insertion. | Fixed: the lookups moved below the release functions, and the release's comment says who calls it (no handler). | `billingstore.go` | | |
| | Opus claims 1 and 2, Astra's "resolvable": a deleted account's review is listed and held with no stage-one disposition, not resolvable; the serialisation holds for binaries with the commit step, and an older binary during a roll reproduces the race. | Corrected everywhere the words appeared (the design, the dispositions, two comments); the roll rule names the older binary's deletion and reap. | design, comments, `ENABLEMENT.md` on the launch branch | | |
| -Mutation checks, in a tree copy, five. Four die: a losing request's cleanup removing a committed retirement (the store function); the handler writing the tombstone before the commit; the rejected listing leaving a deleted account blank; an old reservation reading as pending. One survives, the handler's failed commit calling the unconditional release: the only way the handler's commit fails with a committed row present is a transient database error, which no test injects (the store refuses a commit over an open review before any row is committed, and answers nil for one already committed). The store-level test covers the release the handler now calls. Output: `currency-mutants-round24.txt` in the run directory's logs. |
| +Mutation checks, in a tree copy, five. Four die: a losing request's cleanup removing a committed retirement (the store function); the handler writing the tombstone before the commit; the rejected listing leaving a deleted account blank; an old reservation reading as pending. One survives, the handler's failed commit calling the unconditional release. (Recorded then as reachable only through a transient database error; review twenty-five showed an ordinary ordering reaches it, since a reservation is one row per account: a no-bucket request's release removes the shared pending row, the first request's commit fails with `errBillingRetirement`, a third request reserves and commits, then the first request's cleanup runs. Closed structurally in `35356ad0`: the unconditional release is no longer production code.) The store-level test covers the release the handler now calls. Output: `currency-mutants-round24.txt` in the run directory's logs. |
| 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. |
| @@ -555,9 +555,24 @@ Reports: `OPUS-REVIEW-multi-currency-review25.md`, `CODEX-FINDINGS-multi-currenc |
| | --- | --- | --- | --- | |
| | 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 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. The two-connection test drives a stale-snapshot ordering (the other console's commit has read the reservation, this console commits, the other's write fails as a stale snapshot, its cleanup runs), not Astra's (B's preflight removes the shared pending row; A's commit fails; C reserves and commits; A cleans up): the two end in the same state at the cleanup, a committed row present when the pending-only release runs, and that is what the assertion checks. 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. |
| + |
| +## Review twenty-six (reviewed `35356ad0`; Opus accepted, Astra accepted with fixes) |
| + |
| +Reports: `OPUS-REVIEW-multi-currency-review26.md`, `CODEX-FINDINGS-multi-currency-review26.md`. The first round in which both reviewers accept. Opus: no P1 or P2; every finding of review twenty-five closed; "review eighteen's P2 is closed on this branch"; three P3s. Astra: no new P1 or P2; "review eighteen's P2 is closed on this branch", under the roll rule that stops older consoles; one P3 and seven claims to correct before accepting without qualification. Both say Astra's P2 severity for the migration was right and that the round-twenty-four mutant was reachable by an ordinary ordering, not only a database fault. **Review eighteen's P2 is recorded closed at this round's answer**, with the merge of the branch waiting on the confirming review of that answer and CI on its tip. The gate on `35356ad0` passed (23:57Z, alone) and CI passed. |
| + |
| +| Finding | Disposition | Where | Counterexample test | |
| +| --- | --- | --- | --- | |
| +| Astra P3, Opus P3-3: the losing-writer ordering accepted a stale write that succeeded, so it did not establish the losing commit it names (remove the other console's read and the test still passes). | The stale write must fail with `SQLITE_BUSY_SNAPSHOT` (extended code 517, which the driver prints), as a write from a snapshot that predates another commit does in WAL and `busy_timeout` does not retry. The comment about "some SQLite builds" was wrong and is gone. | `billingnamed_test.go` | the subtest fails if the other console's read is removed (mutant M2) | |
| +| Opus P3-2: ordering one asserted the commit's refusal, which the open review gives anyway, not the withdrawal the comment claims. | After the other console's evidence the test asserts no reservation row remains, then the refusal. | `billingnamed_test.go` | fails if `fileBillingReviewTx` stops calling `cancelPendingRetirementTx` (mutant M1) | |
| +| Opus P3-1, Astra claims 3, 4, 5: three stale comments (a pending reservation is "this request's own"; a filing after the intent is gone "files nothing"; hourly "once tried three times"). | Corrected: a reservation is one row per account and the release is the account's pending one; a late filing classifies against the archive and files a dispute where the evidence contradicts the recorded fate; the delays are one, five and fifteen minutes, then an hour after each try from the fourth. | `ownership.go`, `billingstore.go`, `billingretirement.go` | | |
| +| Astra claims 1 and 2: the dispositions said Astra's A/B/C ordering is tested (the test drives a stale-snapshot ordering that ends in the same state) and that the round-twenty-four mutant was reachable only through a transient database error (an ordinary ordering reaches it). | Both corrected in the sections for reviews twenty-four and twenty-five. | this file | | |
| +| Astra claims 6 and 7: the design said no test runs two consoles (one now runs two connections to one file) and named one exception to "a fate comes from a read" (there are two: the ordinary expiry retirement, and a create answer showing completion recorded as a hold when the refusal is filed). | Both corrected in "Stage one as built". | the design | | |
| + |
| +Stated and not changed: the second connection is a second `Store` in one process on one WAL file, which is what two consoles are to SQLite short of process death (the transaction-abort regression stands in for a death); the `BEGIN DEFERRED` migration can lose liveness, not safety, when two consoles start together (the loser fails to start and starts clean next time), as the old code did with "duplicate column"; the roll rule covers older binaries' deletion and reap. |
| + |
diff --git a/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review26.md b/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review26.md| new file mode 100644 |
| index 000000000..96a35fa32 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review26.md |
| @@ -0,0 +1,130 @@ |
| +**Verdict: accept.** I found no P1 or P2. Every finding of review twenty-five is closed, and the two additions I asked for under RRRR are in. **Review eighteen's P2 is closed on this branch.** What is left are three stale comments and two small gaps in the test, all P3. |
| + |
| +This was a source-only review at `35356ad0`, using `git show` throughout. I modified nothing and ran nothing. The delta `1f1368b0..35356ad0` touches only five files: `billingretirement.go` (the migration), `billingstore.go` (the unconditional release removed), `ownership.go` (a comment), `billingretirement_export_test.go` (new), and two test files. It touches no pricing, form, quote, cookie, barrier-trigger, export, binder, reconcile or site code. My conclusions on A–K, L–ZZZ2 and AAAA–PPPP, and on the site at `01b62f7`, therefore stand as reported at `e152df30` and `1f1368b0`. |
| + |
| +## Review twenty-five, finding by finding |
| + |
| +| Finding | Closed? | Would its test pass without the fix? | |
| +|---|---|---| |
| +| **Astra 1 (P2) / Opus P3-1:** the column migration was not atomic | **Closed.** At `billingretirement.go:188-212` the `pragma_table_info` check, the `ALTER` and the `UPDATE` run in one transaction on `tx`. Nothing inside calls `s.db`, which matters because the pool has one connection and a call on `s.db` would deadlock. SQLite rolls back `ADD COLUMN` with its transaction, because the change to `sqlite_schema` is ordinary transactional data. | **No.** `billingnamed_test.go:1347-1375`: a `BEFORE UPDATE` trigger aborts the marking. Without the transaction, the `ALTER` autocommits and the `hasColumn != 0` assertion fails. An abort stands in faithfully for a death: either way the WAL frames of an uncommitted transaction are discarded. The retry then marks the row. | |
| +| **Opus P3-2:** the no-bucket preflight's release was untested | **Closed.** `deletion_test.go:132-142` asserts that no row remains and that `beginBillingCheckout` succeeds. | **No.** If the release call is dropped, or the check is moved after the commit, a row remains (pending or committed) and `beginBillingCheckout` refuses at `billingstore.go:363`. | |
| +| **Opus P3-3 / Astra 2 (P3):** the handler could call the unconditional release again | **Closed, by a stronger means than a test.** `releaseBillingRetirement` now exists only in `billingretirement_export_test.go`. A handler calling it would fail `go build`, which the CI cross-compile gate runs. It would still compile under `go test`, so it is the build that kills the mutant, not a test. A grep finds no production `DELETE FROM billing_retirements` without `committed_at=0`. | Not applicable: the mutant no longer compiles into the binary. | |
| +| **RRRR addition 1:** a test on two connections | **Done** (`TestTwoConsolesAtTheRetirementsPointOfNoReturn`). See below. | See below. | |
| +| **RRRR addition 2:** the unconditional release out of production | **Done.** See the row above. | | |
| +| **Opus claim 1:** the `errNoReleaseStore` comment | **Corrected** (`ownership.go:619-623`). | n/a | |
| +| **Opus claim 2:** "marked when the column is added" | **Corrected** in the design (`:128`). It is now true. | n/a | |
| + |
| +Astra's report at round twenty-five also listed three stale claims. The dispositions do not answer them and the code still carries them; see P3-1. |
| + |
| +**Where the reviewers disagreed on severity, Astra's reading was right.** |
| +- **The migration window (Astra P2, Opus P3).** Its consequence is exactly eighteen's P2: a committed, possibly tombstoned, reservation reads as pending, evidence withdraws it, and checkout reopens. The owner's standard for stage one names "process death between any two steps". A death between two adjacent autocommitted statements is such a step, so the code at `1f1368b0` did not meet that standard. My P3 rested on rarity, which the standard does not allow. |
| +- **The mutant (Astra: reachable by ordinary orderings).** Astra's sequence needs no database fault: a no-bucket request B releases the pending row, A's commit returns an ordinary `errBillingRetirement`, then C reserves and commits before A's cleanup. One console is enough for it, because `SetMaxOpenConns(1)` serialises transactions but not the steps of a handler. "Only through a transient database error" was wrong. |
| + |
| +## Two consoles: is the second Store a second console to SQLite? |
| + |
| +**Yes, in the sense that matters.** |
| +- `Open(file)` builds its own `*sql.DB` with its own single connection (`store.go:61-65`), so it runs its own transactions. |
| +- The fixture's database is a file in `t.TempDir()` (`server_test.go:53`), opened in WAL mode. |
| +- SQLite coordinates two connections in one process through its own inode-level lock table and the shared `-shm` index. The writer lock, the snapshots and `SQLITE_BUSY_SNAPSHOT` behave as they would between two processes. |
| +- What a second process adds and this test lacks is real process death. The transaction-abort regression above stands in for that. |
| +- Opening the second handle also runs `migrate()` on a database that is already migrated and in use. That incidentally exercises a second console's start (TTTT, GGGG). |
| + |
| +**The three orderings are the ones my report named:** |
| +1. reserve, then evidence, then commit; |
| +2. commit, then evidence, then delete; |
| +3. a losing commit's cleanup. |
| + |
| +| Ordering | Closed? | Would its test pass without the code? | |
| +|---|---|---| |
| +| **(1)** evidence on the other handle before the commit | Yes. The dispute goes through `fileBillingReviewTx` (`billingstore.go:957`) and withdraws the pending row; the commit refuses. | **It would pass even without the withdrawal.** `commitBillingRetirement`'s `checkBillingRetirement` refuses on the open dispute anyway. The test proves the commit refuses, not what the comment says, which is that the evidence withdraws the row (P3-2). | |
| +| **(2)** evidence after the commit | Yes. | **No.** If `cancelPendingRetirementTx` lost its `committed_at=0` clause, the dispute would delete the committed row. `DeleteAccount` would then read the account as not committed and refuse on the dispute, failing the last assertion. | |
| +| **(3)** a losing commit's cleanup | Yes. | **No**, if `releasePendingBillingRetirement` loses its clause. The test does not drive the handler, but the handler can no longer reach anything else. | |
| + |
| +Ordering (3) is my `SQLITE_BUSY_SNAPSHOT` ordering, not Astra's. Astra's ordering (row missing, a third request commits, then the cleanup runs) needs no second connection, and at the cleanup it leaves the same state: a committed row present when the pending-only release runs. The dispositions' "Astra's deterministic ordering … is tested at the store with a second connection" is therefore loose but harmless. |
| + |
| +Of the three, only (3) uses the second connection in a way one connection could not: (1) and (2) are sequential committed transactions, which one handle would see identically. Ordering (3) is also the only one that depends on WAL, and it would fail without it: the reasoning is in P3-3. |
| + |
| +## SSSS: the migration as one transaction |
| + |
| +- **Rollback.** SQLite rolls back `ALTER TABLE … ADD COLUMN` with its transaction: yes. |
| +- **Another console's migration committed first.** `Begin` is `BEGIN DEFERRED` under modernc with a bare path. |
| + - If the other commit landed before this transaction's first read, the pragma sees the column and the function returns nil. |
| + - If it landed after this transaction's read, this `ALTER` must upgrade a stale snapshot and gets `SQLITE_BUSY_SNAPSHOT`, which `busy_timeout` does not retry. `migrate` fails, `Open` returns an error, this console does not start, and nothing of it is written. The next start sees the column and skips. |
| + - This costs liveness, not safety, and it is no worse than before: the old code failed with "duplicate column" in the same window. `BEGIN IMMEDIATE` would remove it. That is optional, not required. |
| +- **A death at each point.** Before the commit, nothing persists. After the commit, the column and the marking both persist. |
| +- **A fresh database.** `billingstore.go:112` creates the table without the column, so the `ALTER` runs once on an empty table. |
| + |
| +## TTTT: every production statement that writes `billing_retirements` |
| + |
| +| Statement | What it requires of `committed_at` | |
| +|---|---| |
| +| `billingstore.go:1548` `cancelPendingRetirementTx` (evidence, through `fileBillingReviewTx:957`) | `DELETE … AND committed_at=0` | |
| +| `billingstore.go:1610` `releasePendingBillingRetirement` (the handler's no-bucket and failed-commit paths) | `DELETE … AND committed_at=0` | |
| +| `billingstore.go:1578` `commitBillingRetirement` | `UPDATE` to a nonzero value, only after reading reserved and not committed in the same transaction. A stale read cannot overwrite a committed row (`BUSY_SNAPSHOT`). | |
| +| `billingstore.go:1599` `reserveBillingRetirement` | `INSERT OR IGNORE`, which never changes an existing row | |
| +| `billingretirement.go:203` the migration | Moves 0 to `started_at`, only in the transaction that adds the column | |
| + |
| +There is no foreign key or cascade, and `DeleteAccount` leaves the row. **No production path can withdraw a committed reservation.** |
| + |
| +## UUUU: is review eighteen's P2 closed? |
| + |
| +**Yes, on this branch.** Both conditions I named at round twenty-five are met. Astra's two stated conditions are met as well: |
| +- the atomic migration with an interrupted-migration regression; |
| +- the call-site mutant killed, by removing the function rather than by a handler test, which is stronger. |
| + |
| +From the code, I cannot construct any interleaving that does any of these things: |
| +- deletes or consumes an intent while its session is payable; |
| +- releases a hold with a subscription or money outstanding; |
| +- withdraws a committed retirement. |
| + |
| +That covers process death between any two steps, two handles on one database, duplicate events, and late provider answers. |
| + |
| +Nothing on this branch stands in the way. What remains is outside it: |
| +- **The roll rule.** An older binary has no commit step and an unconditional release, so older consoles are stopped before this build starts and are never rolled back to. |
| +- **Test-mode item 1:** whether a subscription can exist before its session completes. |
| +- **Stage two:** a disposition for disputes. |
| +- **Decision 15** and **the owner's approvals.** |
| + |
| +## Findings |
| + |
| +### P3-1. Three stale claims from Astra's twenty-fifth report are still in the code, unanswered in the dispositions |
| +- **`ownership.go:157-158`:** "only this request's own pending reservation is released". Reservations are one row per account (`INSERT OR IGNORE`), so this can release another request's pending row. That is safe because the commit re-reads, but the comment is wrong. |
| +- **`billingstore.go:1222-1224`:** a filing after the resolution "files nothing". When the intent has gone, `fileCheckoutRefusal` (`:1249-1275`) classifies the subscription and can file a dispute against what was consumed. |
| +- **`billingretirement.go:1002`:** "an hour later once it has been tried three times". `noteExpiryTry` (`:833-838`) switches on the counter before it increments, so the delays are 1, 5 and 15 minutes, and hourly only from the fourth try. |
| +- **Fix:** correct the three comments and add them to the dispositions. |
| + |
| +### P3-2. Ordering (1) does not assert the withdrawal it claims (`billingnamed_test.go:1445-1457`) |
| +- **Scenario:** delete the `cancelPendingRetirementTx` call from `fileBillingReviewTx`. The subtest still passes, because the commit refuses on the open dispute. |
| +- **Fix:** after `other.noteSubscriptionSeen`, assert that no `billing_retirements` row remains for the account. |
| + |
| +### P3-3. Ordering (3) tolerates the stale write succeeding (`billingnamed_test.go:1497-1502`) |
| +- **Scenario:** in WAL, an `UPDATE` from a transaction whose snapshot predates another commit always fails with `SQLITE_BUSY_SNAPSHOT`. |
| + - The comment's "some SQLite builds let the stale writer through" is wrong for WAL. |
| + - If the journal were not WAL, the test would not reach that line: `fx`'s commit would wait out `busy_timeout` behind the reader and `t.Fatal`. |
| +- **Fix:** require `werr != nil` (ideally `SQLITE_BUSY_SNAPSHOT`), so that the test establishes the losing commit it names. |
| + |
| +## Claims the code contradicts |
| + |
| +- The three comments in P3-1. |
| +- The comment in ordering (3) on stale writers (P3-3). |
| +- The dispositions' line that Astra's ordering "is tested at the store with a second connection". The test drives the `BUSY_SNAPSHOT` ordering. Astra's ordering reduces to the same final assertion, but it is not the sequence that is driven. |
| + |
| +Everything else in the delta matches the code, including the design sentence at `:128` and the comment at `ownership.go:619-623`. |
| + |
| +## What must be verified against Stripe test mode before enabling |
| + |
| +This delta adds no provider dependency, so the list is unchanged from round twenty-five: |
| +1. **Subscription before completion:** whether Checkout creates a Subscription before its session completes, under abandoned 3-D Secure, a declined card followed by another, and a delayed debit. This decides how often an honest buyer meets the hold. It is the first check and a gate. |
| +2. **`incomplete_expired`:** 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 their exact currencies, own-only `currency_options`, and Adaptive Pricing disabled. |
| +8. **The GBP purchase:** the one-form purchase at `?country=IN¤cy=gbp`, end to end. |
| +9. **Expiry racing payment:** with idempotency keys and the fresh-read proof. |
| +10. **Lost-create recovery:** recovery by listing, including pagination. |
| +11. **GBP overages and quantity:** invoices, their recovery, and proration. |
| +12. **Legacy EUR:** request hashes and customer reuse unchanged. |
| + |
| +**Verdict: accept.** P3-1 to P3-3 are fixes to make, not gates. Before enabling: the roll rule, decision 15, the rehearsal and its first-check gate, and the owner's approvals. |
diff --git a/internal/console/billingnamed_test.go b/internal/console/billingnamed_test.go| index bb2f9c31d..27e749850 100644 |
| --- a/internal/console/billingnamed_test.go |
| +++ b/internal/console/billingnamed_test.go |
| @@ -1450,6 +1450,12 @@ func TestTwoConsolesAtTheRetirementsPointOfNoReturn(t *testing.T) { |
| if err := other.noteSubscriptionSeen("cdef", intent.ID, "sub_hand", f.now.Add(2*time.Hour)); err != nil { |
| t.Fatal(err) |
| } |
| + // The evidence itself withdrew the pending reservation, in its own |
| + // transaction; the commit is refused on the review besides. |
| + var rows int |
| + if err := fx.st.db.QueryRow(`SELECT count(*) FROM billing_retirements WHERE account_id=?`, acct.ID).Scan(&rows); err != nil || rows != 0 { |
| + t.Fatalf("after the other console's evidence: %d reservation rows %v; want the pending one withdrawn", rows, 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) |
| } |
| @@ -1495,11 +1501,13 @@ func TestTwoConsolesAtTheRetirementsPointOfNoReturn(t *testing.T) { |
| } |
| _, 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") |
| + // In WAL a write from a transaction whose snapshot predates another |
| + // commit fails with SQLITE_BUSY_SNAPSHOT (extended code 517), which |
| + // busy_timeout does not retry: the other console's commit is the |
| + // losing one, as named, and the driver reports the code in its |
| + // message. |
| + if werr == nil || !strings.Contains(werr.Error(), "(517)") { |
| + t.Fatalf("the other console's stale write: %v; want SQLITE_BUSY_SNAPSHOT (517)", werr) |
| } |
| if err := other.releasePendingBillingRetirement(acct.ID); err != nil { |
| t.Fatal(err) |
diff --git a/internal/console/billingretirement.go b/internal/console/billingretirement.go| index 9bac4901e..1383ae5fc 100644 |
| --- a/internal/console/billingretirement.go |
| +++ b/internal/console/billingretirement.go |
| @@ -999,10 +999,11 @@ func (s *Server) advanceRefusal(ctx context.Context, intent billingIntent, acct |
| // billingPassRefusals works a bounded number of refused intents after |
| // the pass's other work: of those whose try is due, the ones that have |
| // been due longest. A refusal that was just tried is due again a minute |
| -// later at the earliest, and an hour later once it has been tried three |
| -// times (noteExpiryTry), so one that stays due is reached within a few |
| -// passes however many others there are; taken in the order they stand, |
| -// the same old ones could use the budget for ever. |
| +// later at the earliest, then five and fifteen minutes after the next |
| +// two tries, and an hour after each try from the fourth (noteExpiryTry), |
| +// so one that stays due is reached within a few passes however many |
| +// others there are; taken in the order they stand, the same old ones |
| +// could use the budget for ever. |
| func (s *Server) billingPassRefusals(ctx context.Context, refused []billingIntent, now time.Time) []error { |
| var failures []error |
| type due struct { |
diff --git a/internal/console/billingstore.go b/internal/console/billingstore.go| index 52a6e113d..2c156704a 100644 |
| --- a/internal/console/billingstore.go |
| +++ b/internal/console/billingstore.go |
| @@ -1219,9 +1219,12 @@ func (s *Store) resolveBillingReview(r billingReviewResolution, now time.Time) e |
| // the intent's id, which is what the operator's resolution then retires or |
| // keeps), |
| // with the session's id as the review's reference (the intent may never |
| -// have been bound to it), and only while that intent still stands, so a |
| -// binding that read the session before the resolution and files after it |
| -// files nothing. |
| +// have been bound to it). While the intent stands the review is keyed by |
| +// it; once the intent is gone, fileCheckoutRefusal classifies what the |
| +// session names against the archive (a consumed intent's purchase, a |
| +// retired or kept one) and files a dispute where the evidence |
| +// contradicts the recorded fate, so a binding that read the session |
| +// before the resolution and files after it files that, not nothing. |
| func (s *Store) fileCheckoutRefusedReview(intent billingIntent, sessionID string, observed time.Time) error { |
| return s.fileCheckoutRefusal(intent, sessionID, "", observed) |
| } |
diff --git a/internal/console/ownership.go b/internal/console/ownership.go| index e5578aba0..e8ba6aed4 100644 |
| --- a/internal/console/ownership.go |
| +++ b/internal/console/ownership.go |
| @@ -155,8 +155,10 @@ func (s *Server) handleDeleteAccount(w http.ResponseWriter, r *http.Request) { |
| return |
| } |
| // No bucket to write the tombstone to is known before the point of no |
| - // return: nothing is committed, and only this request's own pending |
| - // reservation is released (a committed one may be another request's). |
| + // return: nothing is committed, and only the account's pending |
| + // reservation is released (a reservation is one row per account, not |
| + // per request; a committed one may be another request's past the |
| + // point of no return, and nothing in production code releases it). |
| if s.cfg.Releases == nil { |
| log.Printf("console: tombstone account %s: %v", acct.Sqid, errNoReleaseStore) |
| if rerr := s.store.releasePendingBillingRetirement(acct.ID); rerr != nil { |