amadan
Review twenty-four answered: the committed retirement is durable against every handler cleanup, the handler's commit is tested, and old reservations are committed
Round twenty-four (Opus accept with fixes, no P1 or P2; Astra reject, one P2): a request whose commit failed released the reservation unconditionally, which could remove another request's committed one, reopen checkout before the tombstone was written, and leave the deletion refusing after it. - A failed commit releases only a pending reservation (releasePendingBillingRetirement); the one definite pre-write refusal (no bucket) is checked before the commit and releases only a pending one; after the commit nothing releases (Astra 1, Opus P3-2). - The deletion handler is tested with a hook on the bucket's first write: the retirement is committed at that moment, a dispute filed then lands on the account, the deletion finishes, the review and the rejected event are listed under the deleted account (Opus P3-1, Astra 2). - A reservation from before the commit column is marked committed when the column is added (Opus P3-3). - "Listed and resolvable" is "listed, held with no stage-one disposition" everywhere; the release's comment names its callers (none); the roll rule names the older binary's deletion (Opus P3-4, claims 1 and 2). Five mutants in a tree copy; four die, one is reachable only through a transient database error no test injects, and the dispositions say so. The full console, plans and billingstripe packages pass on this tree (console 577 s, with the gate's timeout; a first run hit the default ten minutes under load on an unrelated test). Review eighteen's P2 stays recorded as open until a round ends with both accepting.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
8 files changed,
+270
−41
docs/superpowers/specs/2026-10-02-intent-retirement-protocol-design.md+1 −1docs/superpowers/specs/DISPOSITIONS-multi-currency.md+21 −3internal/console/billingnamed_test.go+179 −1internal/console/billingoperator.go+3 −1internal/console/billingretirement.go+21 −5internal/console/billingstore.go+23 −13internal/console/ownership.go+20 −15internal/console/store.go+2 −2
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 5cedfbf20..f15726a3e 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 and resolvable under the deleted account (review twenty-three). **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 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). |
| - **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/DISPOSITIONS-multi-currency.md b/docs/superpowers/specs/DISPOSITIONS-multi-currency.md| index 0b35d7df1..f00024cb6 100644 |
| --- a/docs/superpowers/specs/DISPOSITIONS-multi-currency.md |
| +++ b/docs/superpowers/specs/DISPOSITIONS-multi-currency.md |
| @@ -517,8 +517,8 @@ The gate on `2618403f` **failed** (`gate-currency-2618403f.txt`, exit 2): one ex |
| | Finding | Disposition | Where | Counterexample test | |
| | --- | --- | --- | --- | |
| -| Astra 1 (P2): a dispute committed after the retirement's reservation and before the account's tombstone was published did not stop the publication; the deletion then refused at its last step, leaving an account retired in the deployment's manifest and present here, under a dispute stage one cannot resolve. | Fixed by a point of no return in the database: `commitBillingRetirement`, taken after the reservation and before the first write outside, re-reads every condition in its own transaction and marks the reservation committed. Filing a checkout refusal or a checkout dispute withdraws a reservation that is not committed, in the evidence's own transaction. SQLite serialises the two: either the evidence withdraws first and the commit refuses (nothing outside was touched; the reservation is released), or the commit is first and the evidence lands on an account whose retirement finishes (the deletion and the reap pass a review that opened after the commit). | `billingstore.go` `commitBillingRetirement`, `cancelPendingRetirementTx`, `fileBillingReviewTx`, `checkBillingRetirement`; `ownership.go` the deletion handler, `DeleteAccount`; `store.go` `ReapAccount` | `TestARetirementIsCommittedInTheDatabaseAndEvidenceSerialisesAgainstIt` (three cases) | |
| -| Opus P3-2: evidence first seen after an account was deleted landed where no operator looked (the listing left the account blank; the door answered 404). | Fixed: the operator doors look accounts up whether or not they are deleted; the listing marks such a row `account_deleted`. | `billingoperator.go`; `billingstore.go` `accountEvenIfDeleted` | the same test, second case | |
| +| Astra 1 (P2): a dispute committed after the retirement's reservation and before the account's tombstone was published did not stop the publication; the deletion then refused at its last step, leaving an account retired in the deployment's manifest and present here, under a dispute stage one cannot resolve. | Fixed by a point of no return in the database: `commitBillingRetirement`, taken after the reservation and before the first write outside, re-reads every condition in its own transaction and marks the reservation committed. Filing a checkout refusal or a checkout dispute withdraws a reservation that is not committed, in the evidence's own transaction. SQLite serialises the two: either the evidence withdraws first and the commit refuses (nothing outside was touched; the reservation is released), or the commit is first and the evidence lands on an account whose retirement finishes (the deletion and the reap pass a review that opened after the commit), listed under the deleted account and held, like every dispute. SQLite serialises the two for binaries that have the commit step; an older binary during a roll does not, which the roll rule covers. | `billingstore.go` `commitBillingRetirement`, `cancelPendingRetirementTx`, `fileBillingReviewTx`, `checkBillingRetirement`; `ownership.go` the deletion handler, `DeleteAccount`; `store.go` `ReapAccount` | `TestARetirementIsCommittedInTheDatabaseAndEvidenceSerialisesAgainstIt` (three cases) | |
| +| Opus P3-2: evidence first seen after an account was deleted landed where no operator looked (the listing left the account blank; the door answered 404). | Fixed: the operator doors look accounts up whether or not they are deleted; the listing marks such a row `account_deleted`; the door finds it and holds it (every such review is a dispute). | `billingoperator.go`; `billingstore.go` `accountEvenIfDeleted` | the same test, second case | |
| | Astra 2 (P3): one session on a listing page that did not decode discarded the page's other sessions, one of which could be a contradiction of an archived fate. | Fixed: the client decodes each listed session on its own and returns the ones that decoded with the decode error. | `billingstripe/catalog.go` | `TestListCheckouts_AMalformedSessionKeepsTheOthersOnItsPage` | |
| | Opus P3-1: the late-refusal dispute used a third key for an intent consumed with no session. | Fixed: the intent id, as the record and the classification. | `billingstore.go` `fileCheckoutRefusal` | | |
| | Opus P3-3: the recovery site's filter of the hold it caused had no test. | Tested. | | `TestThePassDoesNotFailOnAHoldItsRecoveryCaused` | |
| @@ -526,5 +526,23 @@ The gate on `2618403f` **failed** (`gate-currency-2618403f.txt`, exit 2): one ex |
| Mutation checks, in a tree copy, eleven, all die: checkout evidence not withdrawing a pending retirement; the commit not re-reading the conditions, and not needing a reservation (each needed its own test case, added); the deletion refusing a review opened after the commit, and ignoring reviews whether or not committed; the listing leaving a deleted account blank; the door answering 404 for one; a committed reservation withdrawn; a session that does not decode dropping its page; the recovery's hold the pass's failure; the late refusal of a session not kept. Output: `currency-mutants-round23.txt` in the run directory's logs. |
| -Stated and not changed: the reap of an account the manifest already records as retired passes an open checkout review (the review stays listed and resolvable under the deleted account); the structural guard's wrapper check is lexical; no test runs two console processes (the serialisation is tested through the store's transactions in sequence, which is what SQLite's one writer makes of two consoles). |
| +Stated and not changed: the reap of an account the manifest already records as retired passes an open checkout review (the review stays listed under the deleted account, held); the structural guard's wrapper check is lexical; no test runs two console processes (the serialisation is tested through the store's transactions in sequence, which is what SQLite's one writer makes of two consoles). |
| + |
| + |
| +## Review twenty-four (reviewed `e152df30`; Opus accepted with fixes, Astra rejected) |
| + |
| +Reports: `OPUS-REVIEW-multi-currency-review24.md`, `CODEX-FINDINGS-multi-currency-review24.md`. Opus: the point of no return answers round twenty-three's P2 for this binary, no P1 or P2, four P3s and two claims, and again "I would count review eighteen's P2 as closed". Astra: one P2 at the same boundary, one P3; "I would not yet close review eighteen's P2 as a whole". Both are right about the one thing: the committed marker was not durable against a losing request's cleanup. **Review eighteen's P2 stays recorded as open.** The gate on `e152df30` passed (22:54Z, alone). |
| + |
| +| Finding | Disposition | Where | Counterexample test | |
| +| --- | --- | --- | --- | |
| +| Astra 1 (P2), Opus P3-2: a request whose commit failed on something transient released the reservation unconditionally, which removed another request's committed one; a checkout could then open before the tombstone was written, and the deletion refused after it. The no-release-store branch released the same way. | Fixed: a failed commit releases only a pending reservation (`releasePendingBillingRetirement`); the one definite pre-write refusal (no bucket at all) is checked before the commit and releases only a pending one too; after the commit nothing releases. `releaseBillingRetirement` has no handler caller left. | `ownership.go` the deletion handler; `billingstore.go` | `TestARetirementIsCommittedInTheDatabaseAndEvidenceSerialisesAgainstIt`, "a losing request's cleanup does not touch a committed retirement" (the marker survives; a checkout is still refused) | |
| +| Opus P3-1: the handler's own call of the commit was untested; removing it restored the race with green tests. | Tested through the handler with a hook on the bucket's first write: the retirement is committed at that moment, a dispute filed then (by the production classification) lands on the account, the deletion finishes, and the review is listed under the deleted account. | `ownership.go` | `TestTheDeletionHandlerCommitsBeforeTheTombstone` | |
| +| Opus P3-3: a reservation from before the commit column read as pending, so evidence could withdraw one an earlier build had kept after an ambiguous failure outside. | Fixed: when the column is added, existing rows are marked committed at their start time, once. | `billingretirement.go` `migrateBillingRetirement` | `TestAReservationFromBeforeTheCommitColumnIsCommitted` | |
| +| Astra 2 (P3): the rejected-events listing left a deleted account's name blank. | Fixed: the same deleted-aware lookup. | `billingoperator.go` | `TestTheDeletionHandlerCommitsBeforeTheTombstone`, last assertion | |
| +| 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. |
| + |
| +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. |
diff --git a/internal/console/billingnamed_test.go b/internal/console/billingnamed_test.go| index 4707e90ac..2aa2c5ab8 100644 |
| --- a/internal/console/billingnamed_test.go |
| +++ b/internal/console/billingnamed_test.go |
| @@ -6,11 +6,15 @@ import ( |
| "errors" |
| "fmt" |
| "net/http" |
| + "net/url" |
| "strings" |
| + "sync" |
| "testing" |
| "time" |
| + "github.com/carlosframework/platform/internal/domains" |
| "github.com/carlosframework/platform/internal/plans" |
| + "github.com/carlosframework/platform/internal/release" |
| ) |
| // Review twenty-one's counterexamples. One rule answers them: a |
| @@ -960,7 +964,7 @@ func TestADisputeAboutAConsumedIntentWithNoSessionIsKeyedByTheIntent(t *testing. |
| // anything outside it is written. Checkout evidence that lands before it |
| // withdraws the pending retirement, and the commit then refuses; evidence |
| // that lands after it stays with the account, which is deleted, and is |
| -// listed and resolvable under the deleted account. (Review twenty-three, |
| +// listed under the deleted account, held like every dispute. (Review twenty-three, |
| // Astra 1; Opus P3-2.) |
| func TestARetirementIsCommittedInTheDatabaseAndEvidenceSerialisesAgainstIt(t *testing.T) { |
| // On an account that can be deleted (the fixture's own is the ops |
| @@ -1120,6 +1124,30 @@ func TestARetirementIsCommittedInTheDatabaseAndEvidenceSerialisesAgainstIt(t *te |
| t.Fatalf("the door for the deleted account's dispute: %d %v; want it found and held, not 404", status, answer) |
| } |
| }) |
| + t.Run("a losing request's cleanup does not touch a committed retirement", func(t *testing.T) { |
| + fx, f, acct, _ := ended(t) |
| + // Request A reserves and commits and is writing the tombstone. |
| + 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) |
| + } |
| + // Request B's commit failed on something transient, and it cleans |
| + // up: only a pending reservation goes. |
| + if err := fx.st.releasePendingBillingRetirement(acct.ID); err != nil { |
| + t.Fatal(err) |
| + } |
| + if r, c := reserved(t, fx, acct); !r || !c { |
| + t.Fatalf("after the losing request's cleanup: reserved %v committed %v; want A's commit kept", r, c) |
| + } |
| + // And no checkout opens meanwhile. |
| + state, _ := fx.st.billingAccount(acct.ID) |
| + request := billingIntent{AccountID: acct.ID, Plan: plans.PlanPersonal, Terms: plans.CurrentTermsVersion, PriceBook: plans.CurrentPriceBook, Currency: "gbp", Band: "D", Price: currencyPriceID(plans.PlanPersonal, "D", plans.GBP), Quantity: 1, Customer: state.Customer, Country: "GB", Members: 1} |
| + if _, err := fx.st.beginBillingCheckout(request, f.now.Add(3*time.Hour)); !errors.Is(err, errBillingConflict) { |
| + t.Fatalf("a checkout under a committed retirement: %v; want it refused", err) |
| + } |
| + }) |
| t.Run("a review of another kind withdraws nothing", func(t *testing.T) { |
| fx, f, acct, _ := ended(t) |
| if err := fx.st.reserveBillingRetirement(acct.ID, f.now.Add(2*time.Hour)); err != nil { |
| @@ -1177,3 +1205,153 @@ func TestThePassDoesNotFailOnAHoldItsRecoveryCaused(t *testing.T) { |
| t.Fatalf("state %+v, session %s; want the refusal filed naming the recovered session, which was expired", st, f.sessionStatus) |
| } |
| } |
| + |
| +// hookedReleases is the deployment bucket with a hook on its first write: |
| +// what happens on another console while this one is between its commit |
| +// and its tombstone. |
| +type hookedReleases struct { |
| + release.Store |
| + once sync.Once |
| + onPut func() |
| +} |
| + |
| +func (h *hookedReleases) Put(ctx context.Context, key string, body []byte, ifMatch string) (string, error) { |
| + h.once.Do(h.onPut) |
| + return h.Store.Put(ctx, key, body, ifMatch) |
| +} |
| + |
| +// The deletion handler commits the retirement before it writes the |
| +// tombstone, and a dispute that lands between the two does not stop the |
| +// deletion: the account is deleted, the review is listed under it. (Review |
| +// twenty-four, Opus P3-1: the production call site of the point of no |
| +// return.) |
| +func TestTheDeletionHandlerCommitsBeforeTheTombstone(t *testing.T) { |
| + fx, f, _ := newCurrencyFixture(t) |
| + c := newBrowser(t) |
| + signUp(t, fx, c, "code-delete-billing") |
| + _, sqid, err := domains.MintAccount(context.Background(), fx.rel, "Testco", "paul@keymail.dev", time.Now()) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if n, err := MaterializeAccounts(context.Background(), fx.rel, fx.st); err != nil || n != 1 { |
| + t.Fatalf("materialize = %d %v, want 1", n, err) |
| + } |
| + acct, _ := fx.st.AccountBySqid(sqid) |
| + csrf := sessionToken(t, fx, c) |
| + // The account buys, and the purchase ends. |
| + f.customerCountry = "GB" |
| + f.mu.Lock() |
| + f.sqid = sqid |
| + 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.otherSubscriptions = map[string]bool{"sub_hand": true} |
| + f.mu.Unlock() |
| + if code := f.webhookPost(t, fx, "evt_own_testco", "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) |
| + } |
| + // On the first write to the bucket: the retirement must already be |
| + // committed, and a subscription made by hand naming the consumed |
| + // intent is read by another console. |
| + var committedAtWrite bool |
| + fx.srv.cfg.Releases = &hookedReleases{Store: fx.rel, onPut: func() { |
| + var at int64 |
| + if err := fx.st.db.QueryRow(`SELECT committed_at FROM billing_retirements WHERE account_id=?`, acct.ID).Scan(&at); err == nil && at != 0 { |
| + committedAtWrite = true |
| + } |
| + if code := f.webhookPost(t, fx, "evt_hand_mid_deletion", "customer.subscription.created", "sub_hand"); code != http.StatusNoContent { |
| + t.Errorf("the hand-made subscription during the deletion: %d", code) |
| + } |
| + }} |
| + status, _, body := postGated(t, fx, c, fx.ts.URL+"/a/"+sqid+"/delete", url.Values{"csrf": {csrf}, "confirm": {"Testco"}}) |
| + if status != http.StatusSeeOther { |
| + t.Fatalf("delete = %d:\n%s", status, body) |
| + } |
| + if !committedAtWrite { |
| + t.Fatal("the tombstone was written before the retirement was committed") |
| + } |
| + if a, _ := fx.st.AccountBySqid(sqid); a != nil { |
| + t.Fatal("the account survived its deletion") |
| + } |
| + if n := disputesOf(t, fx, acct, intent.SessionID, "%"); n != 1 { |
| + t.Fatalf("%d disputes; want the evidence that landed mid-deletion recorded", n) |
| + } |
| + token := billingOperatorToken(t, fx, "ops@keymail.dev", "code-billing-op-mid-deletion") |
| + _, listing := getJSON(t, fx.ts.URL+"/api/billing/reviews", token) |
| + rows, _ := listing["reviews"].([]any) |
| + found := false |
| + for _, r := range rows { |
| + row, _ := r.(map[string]any) |
| + if row["reason"] == billingFateDisputedReason && row["account"] == sqid && row["account_deleted"] == true { |
| + found = true |
| + } |
| + } |
| + if !found { |
| + t.Fatalf("the deleted account's dispute is not listed: %v", listing) |
| + } |
| + // A rejected event recorded with the account keeps the account's |
| + // name in the operator's list after the deletion. |
| + _, rejected := getJSON(t, fx.ts.URL+"/api/billing/rejected", token) |
| + rows, _ = rejected["rejected"].([]any) |
| + named := false |
| + for _, r := range rows { |
| + row, _ := r.(map[string]any) |
| + if row["id"] == "evt_hand_mid_deletion" && row["account"] == sqid { |
| + named = true |
| + } |
| + } |
| + if !named { |
| + t.Fatalf("the deleted account's rejected event is not listed under it: %v", rejected) |
| + } |
| +} |
| + |
| +// A reservation from before the commit column exists was kept by an |
| +// earlier build after an ambiguous failure outside, with the tombstone |
| +// possibly written: it is marked committed once, when the column is added, |
| +// so evidence does not withdraw it and a checkout does not open on a |
| +// tombstoned account. A reservation made after the column exists is |
| +// pending until its commit. (Review twenty-four, Opus P3-3.) |
| +func TestAReservationFromBeforeTheCommitColumnIsCommitted(t *testing.T) { |
| + fx, f, acct := newCurrencyFixture(t) |
| + for _, q := range []string{ |
| + `DROP TRIGGER IF EXISTS billing_refused_intent_delete_barrier`, |
| + `ALTER TABLE billing_retirements DROP COLUMN committed_at`, |
| + } { |
| + if _, err := fx.st.db.Exec(q); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + if _, err := fx.st.db.Exec(`INSERT INTO billing_retirements(account_id,started_at) VALUES(?,?)`, acct.ID, f.now.Unix()); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := fx.st.migrateBillingRetirement(); 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 != f.now.Unix() { |
| + t.Fatalf("the old reservation after the column was added: 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) |
| + } |
| + if _, err := fx.st.db.Exec(`INSERT INTO billing_retirements(account_id,started_at) VALUES(?,?)`, acct.ID+1, f.now.Unix()); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := fx.st.migrateBillingRetirement(); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := fx.st.db.QueryRow(`SELECT committed_at FROM billing_retirements WHERE account_id=?`, acct.ID+1).Scan(&committed); err != nil || committed != 0 { |
| + t.Fatalf("a new reservation after a later start: committed_at %d %v; want pending", committed, err) |
| + } |
| +} |
diff --git a/internal/console/billingoperator.go b/internal/console/billingoperator.go| index 58c509561..4efddb625 100644 |
| --- a/internal/console/billingoperator.go |
| +++ b/internal/console/billingoperator.go |
| @@ -311,7 +311,9 @@ func (s *Server) handleAPIBillingRejected(w http.ResponseWriter, r *http.Request |
| } |
| for i, id := range accountIDs { |
| if id != 0 { |
| - if acct, err := s.store.AccountByID(id); err == nil && acct != nil { |
| + // Whether or not the account is deleted since: the event was |
| + // recorded with it. |
| + if acct, _, err := s.store.accountEvenIfDeleted(id); err == nil && acct != nil { |
| out[i].Account = acct.Sqid |
| } |
| } |
diff --git a/internal/console/billingretirement.go b/internal/console/billingretirement.go| index 3ff6aa1f2..ab65e5c20 100644 |
| --- a/internal/console/billingretirement.go |
| +++ b/internal/console/billingretirement.go |
| @@ -169,16 +169,32 @@ func (s *Store) migrateBillingRetirement() error { |
| // another customer's is seen as the contradiction it is. |
| {"billing_retired_intents", "customer", `TEXT NOT NULL DEFAULT ''`}, |
| {"billing_retired_intents", "session_fate", `TEXT NOT NULL DEFAULT ''`}, |
| - // An account's retirement is committed in the database before |
| - // anything outside it is written (commitBillingRetirement): until |
| - // then checkout evidence cancels it, and from then on evidence |
| - // lands on an account that is being retired and stays visible. |
| - {"billing_retirements", "committed_at", `INTEGER NOT NULL DEFAULT 0`}, |
| } { |
| if err := s.addColumnIfMissing(c[0], c[1], c[2]); err != nil { |
| return err |
| } |
| } |
| + // An account's retirement is committed in the database before anything |
| + // outside it is written (commitBillingRetirement): until then checkout |
| + // evidence cancels it, and from then on evidence lands on an account |
| + // that is being retired and stays visible. A reservation from before |
| + // 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 { |
| + 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 { |
| + return err |
| + } |
| + } |
| + } |
| tx, err := s.db.Begin() |
| if err != nil { |
| return err |
diff --git a/internal/console/billingstore.go b/internal/console/billingstore.go| index 5891150ad..31fc21c6d 100644 |
| --- a/internal/console/billingstore.go |
| +++ b/internal/console/billingstore.go |
| @@ -1508,9 +1508,9 @@ func advanceBillingMonthTx(tx *sql.Tx, x billingMonthRecord, state, invoiceID, i |
| // retirement is reserved and when it is committed; once committed (the |
| // point of no return, in this database, before anything outside it is |
| // written) a review that opens afterwards does not stop the deletion |
| -// finishing, and is listed and resolvable under the deleted account |
| -// instead (the operator doors look accounts up whether or not they are |
| -// deleted). |
| +// finishing, and is listed under the deleted account instead, held with no |
| +// stage-one disposition like every dispute (the operator doors look |
| +// accounts up whether or not they are deleted). |
| func checkBillingRetirement(tx *sql.Tx, accountID int64, committed bool) error { |
| if held, err := quantityObligationTx(tx, accountID); err != nil { |
| return err |
| @@ -1602,15 +1602,30 @@ func (s *Store) reserveBillingRetirement(accountID int64, now time.Time) error { |
| return tx.Commit() |
| } |
| -// releaseBillingRetirement removes a reservation. It is called only when the |
| -// retirement is abandoned before ANY external write has happened (for |
| -// example, no release store is configured, so tombstoneAccount refuses before |
| -// touching the bucket). An ambiguous failure AFTER a write may have begun |
| +// 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 |
| +// reservation is not this request's to release. |
| +func (s *Store) releasePendingBillingRetirement(accountID int64) error { |
| + _, err := s.db.Exec(`DELETE FROM billing_retirements WHERE account_id=? AND committed_at=0`, accountID) |
| + return err |
| +} |
| + |
| // accountEvenIfDeleted and accountBySqidEvenIfDeleted are the operator |
| // doors' lookups: a billing review of a deleted account is still a person's |
| -// to see and resolve. |
| +// to see, and to hold (every review that can open after an account's |
| +// retirement is committed is a dispute, which stage one does not resolve). |
| func (s *Store) accountEvenIfDeleted(id int64) (*Account, bool, error) { |
| var a Account |
| var created, deleted int64 |
| @@ -1637,11 +1652,6 @@ func (s *Store) accountBySqidEvenIfDeleted(sqid string) (*Account, bool, error) |
| return s.accountEvenIfDeleted(id) |
| } |
| -func (s *Store) releaseBillingRetirement(accountID int64) error { |
| - _, err := s.db.Exec(`DELETE FROM billing_retirements WHERE account_id=?`, accountID) |
| - return err |
| -} |
| - |
| func (s *Store) readyBillingCustomer(intent billingIntent, customer string) error { |
| if customer == "" { |
| return errBillingIdentity |
diff --git a/internal/console/ownership.go b/internal/console/ownership.go| index 7fb896ac1..f8fe24745 100644 |
| --- a/internal/console/ownership.go |
| +++ b/internal/console/ownership.go |
| @@ -154,12 +154,26 @@ func (s *Server) handleDeleteAccount(w http.ResponseWriter, r *http.Request) { |
| http.Redirect(w, r, "/a/"+acct.Sqid+"/settings", http.StatusSeeOther) |
| 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). |
| + if s.cfg.Releases == nil { |
| + log.Printf("console: tombstone account %s: %v", acct.Sqid, errNoReleaseStore) |
| + if rerr := s.store.releasePendingBillingRetirement(acct.ID); rerr != nil { |
| + log.Printf("console: release retirement reservation %s: %v", acct.Sqid, rerr) |
| + } |
| + s.setFlash(w, "err", deletionAdvice(errNoReleaseStore)) |
| + http.Redirect(w, r, "/a/"+acct.Sqid+"/settings", http.StatusSeeOther) |
| + return |
| + } |
| // The point of no return, in the database, before anything outside it: |
| // a checkout review that opened since the reservation (and withdrew |
| // it) refuses here, and the reservation it withdrew is not re-made. |
| if err := s.store.commitBillingRetirement(acct.ID, time.Now()); err != nil { |
| log.Printf("console: account retirement: %s", billingDiagnostic(err)) |
| - if rerr := s.store.releaseBillingRetirement(acct.ID); rerr != nil { |
| + // Only a pending reservation is released: the failure may be that |
| + // another request committed first, and its retirement goes on. |
| + if rerr := s.store.releasePendingBillingRetirement(acct.ID); rerr != nil { |
| log.Printf("console: release retirement reservation %s: %v", acct.Sqid, rerr) |
| } |
| s.setFlash(w, "err", deletionAdvice(err)) |
| @@ -183,20 +197,11 @@ func (s *Server) handleDeleteAccount(w http.ResponseWriter, r *http.Request) { |
| // on its own, which is the bug this replaces. |
| if err := s.tombstoneAccount(r.Context(), acct); err != nil { |
| log.Printf("console: tombstone account %s: %v", acct.Sqid, err) |
| - if errors.Is(err, errNoReleaseStore) { |
| - // Definite pre-write refusal: nothing external was touched, so the |
| - // reservation must not persist and permanently bar this account |
| - // from ever opening a checkout once billing is on. |
| - if rerr := s.store.releaseBillingRetirement(acct.ID); rerr != nil { |
| - log.Printf("console: release retirement reservation %s: %v", acct.Sqid, rerr) |
| - } |
| - s.setFlash(w, "err", deletionAdvice(err)) |
| - http.Redirect(w, r, "/a/"+acct.Sqid+"/settings", http.StatusSeeOther) |
| - return |
| - } |
| - // Ambiguous failure after a bucket write may have begun: KEEP the |
| - // reservation so a retry can finish retirement, and no new checkout can |
| - // open in the meantime. |
| + // A failure here is after the commit and may be after a bucket |
| + // write began: the committed reservation is KEPT, so a retry can |
| + // finish the retirement and no new checkout opens meanwhile. (The |
| + // one definite pre-write refusal, no bucket at all, is checked |
| + // before the commit.) |
| s.setFlash(w, "err", "Could not retire this account in the deployment bucket, so nothing "+ |
| "was deleted — an account deleted only here would come back on the next restart. "+ |
| "Try again once the bucket is reachable.") |
diff --git a/internal/console/store.go b/internal/console/store.go| index 288927f2b..f6c46a1f3 100644 |
| --- a/internal/console/store.go |
| +++ b/internal/console/store.go |
| @@ -1365,8 +1365,8 @@ func (s *Store) ReapAccount(accountID int64, now time.Time) error { |
| defer tx.Rollback() |
| // The reap follows a retirement the deployment's manifest already |
| // records (made here after its commit, or on another console): an open |
| - // checkout review does not stop it, and stays listed and resolvable |
| - // under the deleted account. |
| + // checkout review does not stop it, and stays listed under the deleted |
| + // account, held. |
| if err := checkBillingRetirement(tx, accountID, true); err != nil { |
| return err |
| } |