amadan
Multi-currency: the tenth review's findings
Reconcile judges the intent's own, complete session with intentSession after ownership, as the binder does, and a mismatch files the same intent-keyed review and records nothing, so a subscription event before the completion event meets the same judgement; the binder's inline comparison is intentSession too. ownSession requires the session's account metadata to name the account, so a session naming two accounts is nobody's on every path. The single-review reader carries the reference. Tests for each, the fake's session metadata separable from the subscription's, the post-resolution test replaying the retired session's own late completion; dispositions corrected and "Review ten".
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
5 files changed,
+226
−32
docs/superpowers/specs/DISPOSITIONS-multi-currency.md+24 −8internal/console/billingcurrency_test.go+166 −8internal/console/billingservice.go+19 −10internal/console/billingservice_test.go+15 −4internal/console/billingstore.go+2 −2
CI failed — run details
diff --git a/docs/superpowers/specs/DISPOSITIONS-multi-currency.md b/docs/superpowers/specs/DISPOSITIONS-multi-currency.md| index d5c34df1d..568ca6c11 100644 |
| --- a/docs/superpowers/specs/DISPOSITIONS-multi-currency.md |
| +++ b/docs/superpowers/specs/DISPOSITIONS-multi-currency.md |
| @@ -67,7 +67,7 @@ Reports: `OPUS-REVIEW-multi-currency-review2.md`, `CODEX-FINDINGS-multi-currency |
| | Opus P3-4: rollback leaves stale columns that fail closed; an older build on a month where a legacy subscription ended and a book one began. | The second case is the barrier above. The first is a roll note (below). | | | |
| | Design text stale (six columns; `ErrAssignmentHistory`; 36 or 48 keys; Adaptive Pricing without the legacy exception). | Corrected. | design | | |
| -**Roll note.** The billing environment and the console binary change together (and, added at review eight: once any `billing_checkout_currency_refused` review has been resolved, no binary from before the eighth-round commit may start against the database: an earlier binary's startup sweep does not know the outcome `checkout_refused_resolved` and would mark the account's quantity history unproven; this binary's sweep clears such a mark again on the next start, so a roll back and forward heals, but the account is held in between): this build needs `CONSOLE_BILLING_PRICE_BOOK` and the 36 keys, and the build before it exits on them. Once any account has a book-priced row, no binary from before price books may run against the database: its reads of the account rows and intents would fail closed, and the database refuses its freezes over book history, but a legacy subscription it recorded for such an account would leave the account's `currency` and `price_book` columns stale and this build refusing the account for good. Billing is off everywhere today, so this is a note for the first roll. |
| +**Roll note.** The billing environment and the console binary change together (and, added at review eight: once any `billing_checkout_currency_refused` review has been resolved, no binary from before the eighth-round commit may start against the database: an earlier binary's startup sweep does not know the outcome `checkout_refused_resolved` and would mark the account's quantity history unproven; nothing clears such a mark [review nine removed the sweep that did, because it could not tell that mark from the holds set on purpose]: the account's months never freeze and it cannot retire until an operator's door that does not yet exist clears it, so the prohibition is absolute): this build needs `CONSOLE_BILLING_PRICE_BOOK` and the 36 keys, and the build before it exits on them. Once any account has a book-priced row, no binary from before price books may run against the database: its reads of the account rows and intents would fail closed, and the database refuses its freezes over book history, but a legacy subscription it recorded for such an account would leave the account's `currency` and `price_book` columns stale and this build refusing the account for good. Billing is off everywhere today, so this is a note for the first roll. |
| Mutation checks on these fixes, nine in a scratch copy: eight failed their tests (the barrier absent, a bound session not expired on either resume path or by the pass, a paid session expired and unreviewed, a second currency among the options, case-folded duplicates, another intent's session kept). One survived, redundant with the count of used keys: a partial catalog of another book is refused by that count whichever check is removed. |
| @@ -105,7 +105,7 @@ The fixes are in the commit that adds this section and have not been read by a r |
| ## Review four (at 8958392b; Opus accepted with fixes, Astra rejected) |
| -Reports: `OPUS-REVIEW-multi-currency-review4.md`, `CODEX-FINDINGS-multi-currency-review4.md`. Both found the third round's code changes present and no path that stores or sends money in a currency other than the purchase's (Astra marked ownership, the crafted form and the trigger migration partly closed, which its findings were about); the disputed trigger reading went Astra's way. Every finding of either that is marked fixed below has a test that fails without it; eight mutants, one per fix, were killed in a scratch copy; two items are left open and say so. |
| +Reports: `OPUS-REVIEW-multi-currency-review4.md`, `CODEX-FINDINGS-multi-currency-review4.md`. Both found the third round's code changes present and no path that stores or sends money in a currency other than the purchase's (Astra marked ownership, the crafted form and the trigger migration partly closed, which its findings were about); the disputed trigger reading went Astra's way. Every code finding of either that is marked fixed below has a test that fails without it, with three qualifications review ten named: the provisioning script's checks are tested by reading the script's text, the trigger's create-then-drop order is tested by a migration that succeeds (not by one that would fail in the other order), and a repeated resolution returns before its delete, so the delete's intent predicate is tested by the first resolution alone; eight mutants, one per fix, were killed in a scratch copy at the time (the runs are not reproducible from this file); two items are left open and say so. |
| | Finding | Disposition | Where | Reproduction | |
| | --- | --- | --- | --- | |
| @@ -173,7 +173,7 @@ The fixes are in the commit that adds this section and have not been read by a r |
| ## Review seven (at 124eab27; Opus accepted with fixes, Astra rejected) |
| -Reports: `OPUS-REVIEW-multi-currency-review7.md`, `CODEX-FINDINGS-multi-currency-review7.md`. The two agree on every finding this round, and on the first two as P2 (the outcome list, and the binder's second read): the outcome the resolution writes was admitted by the processing guard and unknown to the two other validators, so a resolved refusal held the account for good with nothing open to clear it. Both closed the sixth round's quote-key and quantity findings. Everything of either is fixed below with a test that fails without it; four mutants were killed in a scratch copy. |
| +Reports: `OPUS-REVIEW-multi-currency-review7.md`, `CODEX-FINDINGS-multi-currency-review7.md`. The two agree on every finding this round, and on the first as P2 (the outcome list; the binder's second read was Opus P3 and Astra P2, as its row says): the outcome the resolution writes was admitted by the processing guard and unknown to the two other validators, so a resolved refusal held the account for good with nothing open to clear it. Both closed the sixth round's quote-key and quantity findings. Everything of either is fixed below with a test that fails without it; four mutants were killed in a scratch copy. |
| | Finding | Disposition | Where | Reproduction | |
| | --- | --- | --- | --- | |
| @@ -191,13 +191,13 @@ The fixes are in the commit that adds this section and have not been read by a r |
| ## Review eight (at 3e941f00; Opus accepted with fixes, Astra rejected) |
| -Reports: `OPUS-REVIEW-multi-currency-review8.md`, `CODEX-FINDINGS-multi-currency-review8.md`. Both found every seventh-round finding closed; neither found a P1, and Astra's one P2 was in the binder's review routing, not in a money path. Opus: a rollback trap the seventh round's outcome created, and the refused session's id not shown to the operator while the review is open. Astra: an owned completed session whose terms version changed with its currency was refused before the review branch (a variant of its round-four finding 6), the reference not carried through the operator API, and an already-open review from an earlier build never learning its reference. All fixed below, each with a test that fails without it; four mutants were killed in a scratch copy. |
| +Reports: `OPUS-REVIEW-multi-currency-review8.md`, `CODEX-FINDINGS-multi-currency-review8.md`. Both found every seventh-round code finding closed (Astra marked the reference handling partly closed, which its round-eight findings below are about); neither found a P1, and Astra's one P2 was in the binder's review routing, not in a money path. Opus: a rollback trap the seventh round's outcome created, and the refused session's id not shown to the operator while the review is open. Astra: an owned completed session whose terms version changed with its currency was refused before the review branch (a variant of its round-four finding 6), the reference not carried through the operator API, and an already-open review from an earlier build never learning its reference. All fixed below, each with a test that fails without it; four mutants were killed in a scratch copy. |
| | Finding | Disposition | Where | Reproduction | |
| | --- | --- | --- | --- | |
| | Astra 1 (P2): the binder checked plan, band and terms version before ownership, so an owned completed session whose terms version changed with its currency was refused with no review. | Fixed: ownership first; then any mismatch of plan, band, terms, price terms or currency on an owned completed session files the one review. | `billingservice.go` `bindBillingCheckoutFor` | `TestBoundSessionCompletedInAnotherCurrencyIsReviewedOnce` (terms changed with the currency, through the pass and through a fresh webhook) | |
| -| Opus P3-1: a binary from before the seventh round, rolled back to after a refusal was resolved, marks the account's quantity history unproven and nothing clears it. | Fixed in two parts: the roll note says no binary from before the eighth-round commit may start after a refusal has been resolved; and this binary's startup sweep clears a gap it (or an earlier binary) marked under state `history_unproven` when every processed event carries an outcome the current list admits, so a roll back and forward heals. The state label is recomputed by the next summary write. | `billingquantityaudit.go`, dispositions roll note | `TestStartupHealsAGapFromAnOutcomeAdmittedSince` (a gap from the new outcome is cleared; one with a bad outcome behind it stays) | |
| -| Opus P3-2, Astra 2 (P3): the refused session's id was not shown to the operator while the review was open. | Fixed: the review reader and the operator API carry `reference`; the test reads the API as an operator. | `billingstore.go` `billingReview`, `billingoperator.go` | `TestOperatorSeesTheRefusedSessionWhileTheReviewIsOpen`, `TestUnboundRefusedSessionIsRecordedByItsId` | |
| +| Opus P3-1: a binary from before the seventh round, rolled back to after a refusal was resolved, marks the account's quantity history unproven and nothing clears it. | Fixed in two parts at the time: the roll note, and a startup sweep that cleared such a gap. [Superseded in review nine: the sweep could not tell that gap from the holds set on purpose and was removed; the roll note alone stands, and such a gap stays held.] | `billingquantityaudit.go`, dispositions roll note | `TestStartupHealsAGapFromAnOutcomeAdmittedSince` (a gap from the new outcome is cleared; one with a bad outcome behind it stays) | |
| +| Opus P3-2, Astra 2 (P3): the refused session's id was not shown to the operator while the review was open. | Fixed: the operator listing and API carry `reference`; the test reads the API as an operator. [Review ten: the single-review reader `billingReview` did not select it; it does from the tenth-round commit.] | `billingstore.go` `billingReview`, `billingoperator.go` | `TestOperatorSeesTheRefusedSessionWhileTheReviewIsOpen`, `TestUnboundRefusedSessionIsRecordedByItsId` | |
| | Astra 3 (P3): a review filed by the seventh build (no reference) never learned the session's id when the session was met again. | Fixed: a filing that meets an open review fills an empty reference without a new occurrence. | `billingstore.go` `fileCheckoutRefusedReview` | `TestUnboundRefusedSessionIsRecordedByItsId` (reference blanked, the pass meets the session again: filled, occurrences unchanged) | |
| | Opus claims: "under the membership lock" (design, two comments); "generated checks" (two dispositions rows); "removing the check was killed" (review four's list); the two-account fixture's comment ("B's create response was not lost": B stops before a session is created); the negative webhook cases assert no status; the migration test cannot show the atomicity itself. | Corrected: the compare runs under the billing lock and the intent snapshot's membership-locked check holds the count; the plans test reads the script's text; the review-four sentence is withdrawn; the fixture's comment says what happens; the webhook cases assert 204 or 503 per case; the migration row says the ordering is the guarantee and no test shows the atomicity. | comments, this file, tests | | |
| @@ -209,14 +209,30 @@ The fixes are in the commit that adds this section and have not been read by a r |
| ## Review nine (at f8a23ef3; Opus accepted with fixes, Astra rejected) |
| -Reports: `OPUS-REVIEW-multi-currency-review9.md`, `CODEX-FINDINGS-multi-currency-review9.md`. Both found every eighth-round finding closed and agreed on the one P2: the startup sweep added in round eight to heal a gap marked under an obsolete outcome list could not tell such a mark from the pre-history default or the clock-rollback hold, which read the same (`history_unproven`), and would have cleared those on the next start. Fixed below with the tests both asked for; one P3 from Opus fixed too. |
| +Reports: `OPUS-REVIEW-multi-currency-review9.md`, `CODEX-FINDINGS-multi-currency-review9.md`. Both found every eighth-round code finding closed (the stale sentences of this file that Astra listed were corrected in part, the rest in the tenth-round commit) and agreed on the one P2: the startup sweep added in round eight to heal a gap marked under an obsolete outcome list could not tell such a mark from the pre-history default or the clock-rollback hold, which read the same (`history_unproven`), and would have cleared those on the next start. Fixed below with the tests both asked for; one P3 from Opus fixed too. |
| | Finding | Disposition | Where | Reproduction | |
| | --- | --- | --- | --- | |
| | Opus P2-1, Astra 1 (P2): the heal sweep cleared gaps set on purpose. | Fixed by removal: no sweep clears a gap; a gap's cause is not recorded. The roll note forbids the rollback that would make an obsolete-outcome mark; clearing one is an operator's door that does not exist yet (open). | `billingquantityaudit.go` | `TestQuantityHistoryMigrationMarksOldSummaryUnproven` and `TestQuantityAuditClockRollbackPreservesEventAndHolds` (each now restarts the store and asserts the gap stays); `TestAGapFromAnObsoleteOutcomeListStaysHeld` (the mark stays, the account stays held) | |
| -| Opus P3-1: `intentSession` did not judge plan, band or terms version, so an open session with one of them altered was offered and paid, then reviewed, where it could have been expired. | Fixed: `intentSession` compares plan, band and terms version too, so the resume paths refuse and expire such a session. | `billingservice.go` | `TestCheckoutNeverOffersASessionInAnotherCurrency` ("an open session whose plan, band or terms changed is refused and expired") | |
| +| Opus P3-1: `intentSession` did not judge plan, band or terms version, so an open session with one of them altered was offered and paid, then reviewed, where it could have been expired. | Fixed: `intentSession` compares plan and band too, so the resume paths refuse and expire such a session. (The terms-version clause it also gained is redundant: `priceTermsFromMetadata` already carried the terms version into the price-terms comparison, and that iteration of the test passes without it; the plan and band clauses are the fix.) | `billingservice.go` | `TestCheckoutNeverOffersASessionInAnotherCurrency` ("an open session whose plan, band or terms changed is refused and expired") | |
| | Opus claims: six stale sentences in this file (the review-eight "no P2", "generated checks" twice, "dropped first", "removing the check is killed", review seven's "the one P2"). | Corrected in this commit. | this file | | |
| Open after this round: an operator's door to clear a quantity-history gap whose cause the operator has established (none exists; the account stays held); the site's `bands.p3` sentence; two unreviewed system strings on the billing page; everything that needs Stripe test mode. |
| The fixes are in the commit that adds this section and have not been read by a reviewer. |
| + |
| +## Review ten (reviewed `faf1bd25`; Opus accepted with fixes, Astra accepted with fixes) |
| + |
| +Reports: `OPUS-REVIEW-multi-currency-review10.md`, `CODEX-FINDINGS-multi-currency-review10.md`. The first round in which both reviewers accept. Both found every ninth-round code finding closed (the plan and band clauses of `intentSession` with their tests; the terms-version clause redundant, as the row above now says), nothing new in a money path, and the same P3: reconcile judged the bound session on ownership and currency alone. Both lists of stale sentences in this file are corrected in place. Fixed below, each with a test that fails without it; three mutants were killed in a scratch copy (the account-metadata clause removed; reconcile's purchase judgement removed; the single reader's reference column removed). |
| + |
| +| Finding | Disposition | Where | Reproduction | |
| +| --- | --- | --- | --- | |
| +| Opus P3-1, Astra 1 (P3): reconcile checked the bound session with `ownSession` and the currency, not `intentSession`, so a subscription event delivered before the completion event recorded a purchase whose session the binder would have reviewed for its plan or band. | Fixed: reconcile judges the intent's own, complete session with `intentSession` after ownership, and a mismatch files the same intent-keyed review the binder files and records nothing; the intent stands. The binder's inline comparison is `intentSession` too, so the purchase judgement has one definition. | `billingservice.go` `reconcileBillingSubscription`, `bindBillingCheckoutFor` | `TestASubscriptionEventBeforeTheCompletionMeetsTheSameJudgement` (the session's metadata alone altered, plan and band in turn, through the fake's new `sessionMeta` knob; `customer.subscription.created` first: one review keyed by the intent with the session as its reference, nothing recorded, the intent standing; then the completion and a second subscription event: still one) | |
| +| Astra 2 (P3): a session whose `client_reference_id` is A's and whose `metadata.account_sqid` is B's passed `ownSession` and `intentSession`, so the resume and listing paths could offer or keep what the binder would refuse. | Fixed: `ownSession` requires the account metadata to name the account too; such a session is nobody's on every path, and, being nobody's, is not expired either. | `billingservice.go` `ownSession` | `TestOwnSessionClauses` ("another account in the metadata"); `TestASessionNamingTwoAccountsIsNobodys` (a bound open session read back so is not offered, not expired, the intent left as it was; a listed one is not bound, and the same listing binds it once its accounts agree) | |
| +| Opus P3-2, Astra claims: the roll note promised a heal that review nine removed. | Corrected: the prohibition is absolute; a mark an older binary makes stays until an operator's door that does not exist. | this file, roll note | | |
| +| Astra claims: the single-review reader `billingReview` did not carry `reference`. | Fixed: it selects the column; the resolution test reads the review singly. | `billingstore.go` `billingReview` | `TestRefusedPaidSessionReviewThroughTheWebhookAndItsResolution` | |
| +| Astra claims: the post-resolution test replayed an open replacement session, not the retired session's late completion, because the fake had one session id. | Fixed: the second purchase binds `cs_test_second` (the fake answers any session path with its one session, so a console that asked for another id sees the mismatch), and the late webhook for `cs_test_paid` is answered, once, with the retired session as it was (complete, EUR, the retired intent's metadata): nothing recorded, reviewed or expired, the standing intent untouched. | `billingservice_test.go`, `billingcurrency_test.go` | the same test | |
| +| Astra claims: the site test named "the book is the platform export" checked the schema, the currencies and a source note, not equality. | Renamed for what it checks (site `3f1610d`); equality with the platform's export is checked by hand against the commit the source note names, as before. | site `test/pricing-pricebook.test.mjs` | | |
| +| Both: stale sentences in this file (review seven's agreement sentence; "every seventh-round finding closed"; the review-eight heal row; the terms-version row; "generated"/"uniformly distinguishing" in review four; the review-nine summary). | Corrected in place, each with the correction in brackets where the original sentence is history. | this file | | |
| + |
| +Still open after this round, as both reviewers list: everything that needs Stripe test mode (the list in both reports and in `ENABLEMENT.md` section 6, now with session metadata immutability, line-item reads and webhook ordering added), the two unreviewed system strings, and the operator's door for a quantity-history gap. The eleventh review reads this commit. |
diff --git a/internal/console/billingcurrency_test.go b/internal/console/billingcurrency_test.go| index ecb969fec..2a85a2d46 100644 |
| --- a/internal/console/billingcurrency_test.go |
| +++ b/internal/console/billingcurrency_test.go |
| @@ -2656,7 +2656,7 @@ func TestOwnSessionClauses(t *testing.T) { |
| b := fx.srv.billing |
| intent := billingIntent{ID: "0123456789abcdef0123456789abcdef0123456789abcdef", Customer: "cus_paid"} |
| good := func() *billingstripe.Session { |
| - return &billingstripe.Session{ID: "cs_a", LiveMode: b.cfg.LiveMode, CustomerID: "cus_paid", ClientReferenceID: acct.Sqid, Metadata: map[string]string{"checkout_intent": intent.ID}} |
| + return &billingstripe.Session{ID: "cs_a", LiveMode: b.cfg.LiveMode, CustomerID: "cus_paid", ClientReferenceID: acct.Sqid, Metadata: map[string]string{"checkout_intent": intent.ID, "account_sqid": acct.Sqid}} |
| } |
| if !b.ownSession(good(), intent, acct) { |
| t.Fatal("the intent's own session is not its own") |
| @@ -2667,7 +2667,10 @@ func TestOwnSessionClauses(t *testing.T) { |
| "another intent": func(s *billingstripe.Session, _ *billingIntent) { |
| s.Metadata["checkout_intent"] = "fedcba9876543210fedcba9876543210fedcba9876543210" |
| }, |
| - "another account": func(s *billingstripe.Session, _ *billingIntent) { s.ClientReferenceID = "zzz" }, |
| + "another account": func(s *billingstripe.Session, _ *billingIntent) { s.ClientReferenceID = "zzz" }, |
| + "another account in the metadata": func(s *billingstripe.Session, _ *billingIntent) { |
| + s.Metadata["account_sqid"] = "zzz" |
| + }, |
| "another customer": func(s *billingstripe.Session, _ *billingIntent) { s.CustomerID = "cus_other" }, |
| "not the bound session": func(_ *billingstripe.Session, i *billingIntent) { i.SessionID = "cs_b" }, |
| } { |
| @@ -2874,6 +2877,11 @@ func TestRefusedPaidSessionReviewThroughTheWebhookAndItsResolution(t *testing.T) |
| if open, n := review(); open != 1 || n != 1 { |
| t.Fatalf("after the webhook alone: %d open, %d occurrences; want the review filed by it", open, n) |
| } |
| + // The single-review reader, which a resolution quotes, carries the |
| + // session too. |
| + if one, err := fx.st.billingReview(acct.ID, intent.ID, billingCheckoutRefusedReason); err != nil || one.Reference != intent.SessionID || one.Occurrences != 1 { |
| + t.Fatalf("the review read singly: %+v %v; want the session as its reference", one, err) |
| + } |
| // A member joins while the refused intent stands: a quantity event |
| // against the intent, which the resolution must close. (Such events |
| // are kept for live, team-plan intents; the fixture's intent is made |
| @@ -2931,12 +2939,17 @@ func TestRefusedPaidSessionReviewThroughTheWebhookAndItsResolution(t *testing.T) |
| if err := fx.st.db.QueryRow(`SELECT count(*) FROM billing_quantity_changes WHERE account_id=? AND history_gap<>0`, acct.ID).Scan(&gaps); err != nil || gaps != 0 { |
| t.Fatalf("%d history gaps after a second start (%v)", gaps, err) |
| } |
| - // A new purchase opens a new intent; a binding of the old session that |
| - // read it before the resolution files nothing now, and a resolution |
| - // named for the old intent (already resolved: it returns before any |
| - // delete) touches the new one not at all. |
| + // A new purchase opens a new intent, bound to a session of its own |
| + // (cs_test_second); a binding of the old session that read it before |
| + // the resolution files nothing now, and a resolution named for the old |
| + // intent (already resolved: it returns before any delete) touches the |
| + // new one not at all. |
| + oldMeta := map[string]string{} |
| f.mu.Lock() |
| - f.sessionStatus, f.sessionCurrency = "open", "" |
| + for k, v := range f.meta { |
| + oldMeta[k] = v |
| + } |
| + f.sessionStatus, f.sessionCurrency, f.sessionID = "open", "", "cs_test_second" |
| f.mu.Unlock() |
| // (The fake's sessions expire an hour after its clock: the new purchase |
| // is made within that hour.) |
| @@ -2960,14 +2973,159 @@ func TestRefusedPaidSessionReviewThroughTheWebhookAndItsResolution(t *testing.T) |
| if after, err := fx.st.billingCheckoutIntent(acct.ID); err != nil || after.ID != second.ID { |
| t.Fatalf("the new intent after a resolution named for the old: %+v %v", after, err) |
| } |
| + if bound, err := fx.st.billingCheckoutIntent(acct.ID); err != nil || bound.SessionID != "cs_test_second" { |
| + t.Fatalf("the second intent's session: %+v %v", bound, err) |
| + } |
| _ = fx.srv.billingPass(context.Background(), f.now.Add(2*time.Hour)) |
| + // The retired session's own completion, delivered late: the provider |
| + // answers the read of cs_test_paid with that session as it was, paid |
| + // in EUR for the retired intent. It is nobody's now: not the standing |
| + // intent's, and its own intent is retired, so nothing is recorded, |
| + // reviewed or expired. |
| + f.mu.Lock() |
| + f.sessionAnswers = []func(*fakeBillingStripe){func(f *fakeBillingStripe) { |
| + f.sessionID, f.sessionStatus, f.sessionCurrency, f.sessionMeta = "cs_test_paid", "complete", "eur", oldMeta |
| + }} |
| + f.mu.Unlock() |
| send("evt_again") |
| if open, n := review(); open != 0 || n != 1 { |
| - t.Fatalf("after resolution, a pass and a webhook: %d open, %d occurrences; want the review to stay resolved", open, n) |
| + t.Fatalf("after resolution, a pass and the retired session's late completion: %d open, %d occurrences; want the review to stay resolved", open, n) |
| } |
| if state, _ := fx.st.billingAccount(acct.ID); state.Subscription != "" || f.expireCalls != 0 { |
| t.Fatalf("state %+v, %d expiries; want nothing recorded or expired", state, f.expireCalls) |
| } |
| + if after, err := fx.st.billingCheckoutIntent(acct.ID); err != nil || after.ID != second.ID || after.SessionID != "cs_test_second" { |
| + t.Fatalf("the standing intent after the late completion: %+v %v", after, err) |
| + } |
| +} |
| + |
| +// A session that names two accounts (one in its client reference, another |
| +// in its metadata) is nobody's: not offered again, not bound from a |
| +// listing, and not expired either, since the console cannot say whose it |
| +// is. (Review ten, Astra 2: the binder refused such a session and the |
| +// resume and listing paths did not.) |
| +func TestASessionNamingTwoAccountsIsNobodys(t *testing.T) { |
| + click := func(fx *consoleFixture, f *fakeBillingStripe, acct *Account, after time.Duration) (string, error) { |
| + return fx.srv.startBillingCheckout(context.Background(), acct, plans.PlanPersonal, "GB", "", "", f.now.Add(after)) |
| + } |
| + t.Run("a bound open session read back with another account in its metadata is not offered", func(t *testing.T) { |
| + fx, f, acct := newCurrencyFixture(t) |
| + f.customerCountry = "GB" |
| + if dest, err := click(fx, f, acct, 0); err != nil || dest == "" { |
| + t.Fatalf("first click: dest %q err %v", dest, err) |
| + } |
| + before, _ := fx.st.billingCheckoutIntent(acct.ID) |
| + f.mu.Lock() |
| + f.meta["account_sqid"] = "zzz" |
| + f.mu.Unlock() |
| + if dest, err := click(fx, f, acct, time.Minute); dest != "" || !errors.Is(err, errBillingIdentity) { |
| + t.Fatalf("the session named another account: dest %q err %v; want no destination", dest, err) |
| + } |
| + if f.expireCalls != 0 || f.sessionStatus != "open" { |
| + t.Fatalf("%d expiries, session %s; want a session that is nobody's left alone", f.expireCalls, f.sessionStatus) |
| + } |
| + if after, err := fx.st.billingCheckoutIntent(acct.ID); err != nil || after.ID != before.ID || after.SessionID != before.SessionID { |
| + t.Fatalf("intent %+v %v; want it as it was", after, err) |
| + } |
| + }) |
| + t.Run("a listed session with another account in its metadata is not bound", func(t *testing.T) { |
| + fx, f, acct := newCurrencyFixture(t) |
| + f.customerCountry, f.failCreate = "GB", true |
| + if _, err := fx.srv.startBillingCheckout(context.Background(), acct, plans.PlanPersonal, "GB", "", "", f.now); err == nil { |
| + t.Fatal("fixture: the create response was not lost") |
| + } |
| + intent, err := fx.st.billingCheckoutIntent(acct.ID) |
| + if err != nil || intent.SessionID != "" { |
| + t.Fatalf("fixture: intent %+v %v", intent, err) |
| + } |
| + terms := intent.priceTerms() |
| + f.mu.Lock() |
| + f.failCreate, f.sessionStatus, f.sessionCurrency = false, "open", "gbp" |
| + // The intent's purchase in every respect but the account metadata. |
| + f.sessionMeta = map[string]string{"checkout_intent": intent.ID, "account_sqid": "zzz", "plan": intent.Plan, "band": intent.Band, "terms_version": terms.TermsVersion, "price_book": terms.PriceBook, "currency": string(terms.Currency)} |
| + f.mu.Unlock() |
| + _ = fx.srv.billingPass(context.Background(), f.now.Add(time.Minute)) |
| + after, err := fx.st.billingCheckoutIntent(acct.ID) |
| + if err != nil || after.SessionID != "" || f.expireCalls != 0 { |
| + t.Fatalf("intent %+v %v, %d expiries; want the listed session neither bound nor expired", after, err, f.expireCalls) |
| + } |
| + // And with the metadata agreeing, the same listing binds it: the |
| + // fixture is otherwise the intent's own. |
| + f.mu.Lock() |
| + f.sessionMeta["account_sqid"] = acct.Sqid |
| + f.mu.Unlock() |
| + _ = fx.srv.billingPass(context.Background(), f.now.Add(2*time.Minute)) |
| + if bound, err := fx.st.billingCheckoutIntent(acct.ID); err != nil || bound.SessionID == "" { |
| + t.Fatalf("the same session with its accounts agreeing: %+v %v; want it bound", bound, err) |
| + } |
| + }) |
| +} |
| + |
| +// A subscription event that arrives before the completion event for the |
| +// intent's own session, completed for another plan, meets the same |
| +// judgement as the completion would: nothing is recorded, the review is |
| +// filed once, keyed by the intent, and the intent stands. (Review ten, |
| +// both reviewers: reconcile judged the bound session on ownership and |
| +// currency alone, so delivery order decided whether a plan or band |
| +// mismatch was reviewed or recorded.) |
| +func TestASubscriptionEventBeforeTheCompletionMeetsTheSameJudgement(t *testing.T) { |
| + for _, field := range []string{"plan", "band"} { |
| + fx, f, acct := newCurrencyFixture(t) |
| + f.customerCountry = "GB" |
| + if dest, err := fx.srv.startBillingCheckout(context.Background(), acct, plans.PlanPersonal, "GB", "", "", f.now); err != nil || dest == "" { |
| + t.Fatalf("%s: first click: dest %q err %v", field, dest, err) |
| + } |
| + intent, err := fx.st.billingCheckoutIntent(acct.ID) |
| + if err != nil || intent.SessionID == "" { |
| + t.Fatalf("%s: fixture: intent %+v %v", field, intent, err) |
| + } |
| + // The subscription is the intent's purchase exactly; the session's |
| + // metadata alone differs from it. |
| + sessionMeta := map[string]string{} |
| + f.mu.Lock() |
| + for k, v := range f.meta { |
| + sessionMeta[k] = v |
| + } |
| + sessionMeta[field] = map[string]string{"plan": plans.PlanBusiness, "band": plans.BandD}[field] |
| + f.sessionStatus, f.subscriptionPrice, f.sessionMeta = "complete", intent.Price, sessionMeta |
| + f.mu.Unlock() |
| + if code := f.webhookPost(t, fx, "evt_sub_first_"+field, "customer.subscription.created", "sub_paid"); code != http.StatusNoContent { |
| + t.Fatalf("%s: the subscription event: %d", field, code) |
| + } |
| + review := func() (open int, occurrences int64, reference string) { |
| + err := fx.st.db.QueryRow(`SELECT occurrences, reference FROM billing_reviews WHERE account_id=? AND subscription=? AND reason=? AND resolved_at=0`, acct.ID, intent.ID, billingCheckoutRefusedReason).Scan(&occurrences, &reference) |
| + if errors.Is(err, sql.ErrNoRows) { |
| + return 0, 0, "" |
| + } |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + return 1, occurrences, reference |
| + } |
| + if open, n, ref := review(); open != 1 || n != 1 || ref != intent.SessionID { |
| + t.Fatalf("%s: after the subscription event alone: %d open, %d occurrences, reference %q; want one review keyed by the intent with the session", field, open, n, ref) |
| + } |
| + if state, _ := fx.st.billingAccount(acct.ID); state.Subscription != "" { |
| + t.Fatalf("%s: the subscription was recorded before the session was judged: %+v", field, state) |
| + } |
| + if after, err := fx.st.billingCheckoutIntent(acct.ID); err != nil || after.ID != intent.ID { |
| + t.Fatalf("%s: intent %+v %v; want it standing", field, after, err) |
| + } |
| + // The completion event after it, and the subscription event again: |
| + // still one review, nothing recorded. |
| + if code := f.webhookPost(t, fx, "evt_done_"+field, "checkout.session.completed", intent.SessionID); code != http.StatusNoContent { |
| + t.Fatalf("%s: the completion event: %d", field, code) |
| + } |
| + if code := f.webhookPost(t, fx, "evt_sub_again_"+field, "customer.subscription.updated", "sub_paid"); code != http.StatusNoContent { |
| + t.Fatalf("%s: the subscription event again: %d", field, code) |
| + } |
| + if open, n, _ := review(); open != 1 || n != 1 { |
| + t.Fatalf("%s: after the completion and a second subscription event: %d open, %d occurrences; want the one review", field, open, n) |
| + } |
| + if state, _ := fx.st.billingAccount(acct.ID); state.Subscription != "" || f.expireCalls != 0 { |
| + t.Fatalf("%s: state %+v, %d expiries; want nothing recorded or expired", field, state, f.expireCalls) |
| + } |
| + } |
| } |
| // A refused, paid session found by the listing (the create response was |
diff --git a/internal/console/billingservice.go b/internal/console/billingservice.go| index 9d7e58c30..f4fecd799 100644 |
| --- a/internal/console/billingservice.go |
| +++ b/internal/console/billingservice.go |
| @@ -255,12 +255,14 @@ func (b *billingRuntime) intentSession(session *billingstripe.Session, intent bi |
| } |
| // ownSession reports whether a session is this intent's at all: this |
| -// account's, in this mode, carrying the intent's id, for the intent's own |
| -// customer (once the intent has one), and, once the intent is bound, the |
| -// very session it is bound to. Only such a session is ever expired, |
| -// offered, bound or recorded on the intent's behalf. |
| +// account's (its client reference and its account metadata both, so a |
| +// session that names two accounts is nobody's), in this mode, carrying |
| +// the intent's id, for the intent's own customer (once the intent has |
| +// one), and, once the intent is bound, the very session it is bound to. |
| +// Only such a session is ever expired, offered, bound or recorded on the |
| +// intent's behalf. |
| func (b *billingRuntime) ownSession(session *billingstripe.Session, intent billingIntent, acct *Account) bool { |
| - if session == nil || session.ID == "" || session.LiveMode != b.cfg.LiveMode || session.Metadata["checkout_intent"] != intent.ID || session.ClientReferenceID != acct.Sqid { |
| + if session == nil || session.ID == "" || session.LiveMode != b.cfg.LiveMode || session.Metadata["checkout_intent"] != intent.ID || session.ClientReferenceID != acct.Sqid || session.Metadata["account_sqid"] != acct.Sqid { |
| return false |
| } |
| if intent.Customer != "" && session.CustomerID != intent.Customer { |
| @@ -793,9 +795,17 @@ func (s *Server) reconcileBillingSubscription(ctx context.Context, id, eventID s |
| if !b.ownSession(session, intent, acct) || session.SubscriptionID != id || session.CustomerID != sub.CustomerID { |
| return errBillingIdentity |
| } |
| - // A purchase priced from a book was paid in the book's currency for |
| - // the buyer's choice, with no conversion on the session. |
| - if !priceTerms.Legacy() && !session.FixedCurrency(string(priceTerms.Currency)) { |
| + // And the purchase the intent describes: plan, band, terms, price |
| + // terms, and for a book purchase its currency with no conversion |
| + // on it. The intent's own session completed for anything else is |
| + // the review the binder files for it (bindBillingCheckoutFor), so |
| + // a subscription event that arrives before the completion event |
| + // meets the same judgement and records nothing; the intent stands |
| + // until the operator resolves the review. |
| + if !b.intentSession(session, intent, acct) { |
| + if err := s.store.fileCheckoutRefusedReview(intent, session.ID, now); err != nil { |
| + return err |
| + } |
| return errBillingIdentity |
| } |
| if session.PaymentStatus != "paid" && session.PaymentStatus != "no_payment_required" { |
| @@ -967,8 +977,7 @@ func (s *Server) bindBillingCheckoutFor(ctx context.Context, sessionID string, e |
| if !b.ownSession(session, intent, acct) { |
| return "", errBillingIdentity |
| } |
| - sessionTerms, termsErr := priceTermsFromMetadata(session.Metadata) |
| - if intent.Plan != session.Metadata["plan"] || intent.Terms != session.Metadata["terms_version"] || intent.Band != session.Metadata["band"] || termsErr != nil || sessionTerms != intent.priceTerms() || (!sessionTerms.Legacy() && !session.FixedCurrency(string(sessionTerms.Currency))) { |
| + if !b.intentSession(session, intent, acct) { |
| // This intent's own session (its id, account, customer, mode), |
| // completed for another plan, band or terms, on other price terms |
| // or in another currency than it was asked for. Nothing is recorded from it, and a person |
diff --git a/internal/console/billingservice_test.go b/internal/console/billingservice_test.go| index 49d940069..48de1e2b4 100644 |
| --- a/internal/console/billingservice_test.go |
| +++ b/internal/console/billingservice_test.go |
| @@ -49,8 +49,12 @@ type fakeBillingStripe struct { |
| // answering a read of one session with another). |
| sessionCustomer, sessionID string |
| // sessionSqid, when set, is the client_reference_id the session names |
| - // in place of the fixture account's. |
| + // in place of the fixture account's. sessionMeta, when set, is the |
| + // metadata the session carries in place of the metadata the checkout |
| + // was created with (meta), which the subscription keeps: the provider |
| + // answering with session metadata that differs from the subscription's. |
| sessionSqid string |
| + sessionMeta map[string]string |
| // sessionAnswers, when set, are applied to the fake in order, one per |
| // rendering of the session, before it is rendered: the provider |
| // answering successive reads of one session differently. |
| @@ -171,7 +175,11 @@ func (f *fakeBillingStripe) session() map[string]any { |
| if f.sessionSqid != "" { |
| sqid = f.sessionSqid |
| } |
| - return map[string]any{"currency": currency, "id": id, "url": "https://checkout.stripe.com/c/paid", "status": f.sessionStatus, "expires_at": f.now.Add(time.Hour).Unix(), "livemode": false, "customer": customer, "subscription": sub, "payment_status": payment, "client_reference_id": sqid, "metadata": f.meta, "customer_details": map[string]any{"address": map[string]string{"country": "IE"}}} |
| + meta := f.meta |
| + if f.sessionMeta != nil { |
| + meta = f.sessionMeta |
| + } |
| + return map[string]any{"currency": currency, "id": id, "url": "https://checkout.stripe.com/c/paid", "status": f.sessionStatus, "expires_at": f.now.Add(time.Hour).Unix(), "livemode": false, "customer": customer, "subscription": sub, "payment_status": payment, "client_reference_id": sqid, "metadata": meta, "customer_details": map[string]any{"address": map[string]string{"country": "IE"}}} |
| } |
| func (f *fakeBillingStripe) serve(w http.ResponseWriter, r *http.Request) { |
| @@ -236,7 +244,7 @@ func (f *fakeBillingStripe) serve(w http.ResponseWriter, r *http.Request) { |
| return |
| } |
| out = f.session() |
| - case r.URL.Path == "/v1/checkout/sessions/cs_test_paid/expire" && r.Method == http.MethodPost: |
| + case strings.HasPrefix(r.URL.Path, "/v1/checkout/sessions/") && strings.HasSuffix(r.URL.Path, "/expire") && r.Method == http.MethodPost: |
| f.expireCalls++ |
| if f.failExpire > 0 { |
| f.failExpire-- |
| @@ -251,7 +259,10 @@ func (f *fakeBillingStripe) serve(w http.ResponseWriter, r *http.Request) { |
| } |
| f.sessionStatus = "expired" |
| out = map[string]any{"id": "cs_test_paid", "url": "https://checkout.stripe.com/c/paid", "status": "expired", "customer": "cus_paid", "livemode": false, "client_reference_id": f.sqid, "metadata": f.meta} |
| - case r.URL.Path == "/v1/checkout/sessions/cs_test_paid": |
| + case strings.HasPrefix(r.URL.Path, "/v1/checkout/sessions/") && r.Method == http.MethodGet: |
| + // Any id: the session rendered is the fake's one session, whose id |
| + // is sessionID or cs_test_paid; a console that asked for another |
| + // id sees the mismatch, as it would from the provider. |
| out = f.session() |
| case r.URL.Path == "/v1/coupons/coupon_house": |
| if f.coupon != nil { |
diff --git a/internal/console/billingstore.go b/internal/console/billingstore.go| index b0cc207fb..c2e61fc1e 100644 |
| --- a/internal/console/billingstore.go |
| +++ b/internal/console/billingstore.go |
| @@ -783,8 +783,8 @@ type billingReview struct { |
| // Occurrences it returns is the value resolveBillingReview is conditional on. |
| func (s *Store) billingReview(accountID int64, subscription, reason string) (billingReview, error) { |
| x := billingReview{AccountID: accountID, Subscription: subscription, Reason: reason} |
| - err := s.db.QueryRow(`SELECT occurrences,created_at,last_seen_at,resolved_at FROM billing_reviews WHERE account_id=? AND subscription=? AND reason=?`, |
| - accountID, subscription, reason).Scan(&x.Occurrences, &x.CreatedAt, &x.LastSeenAt, &x.ResolvedAt) |
| + err := s.db.QueryRow(`SELECT occurrences,created_at,last_seen_at,resolved_at,reference FROM billing_reviews WHERE account_id=? AND subscription=? AND reason=?`, |
| + accountID, subscription, reason).Scan(&x.Occurrences, &x.CreatedAt, &x.LastSeenAt, &x.ResolvedAt, &x.Reference) |
| return x, err |
| } |