diff --git a/app/Console/Commands/SyncStripeCatalogue.php b/app/Console/Commands/SyncStripeCatalogue.php index 839041a..b34071c 100644 --- a/app/Console/Commands/SyncStripeCatalogue.php +++ b/app/Console/Commands/SyncStripeCatalogue.php @@ -104,9 +104,27 @@ class SyncStripeCatalogue extends Command $productId = $stripe->createProduct( $family->name, ['plan_family' => $family->key, 'plan_family_id' => (string) $family->id], - // Keyed on our row, so a crash between Stripe creating - // the product and us storing its id gives back the same - // product on the next run rather than a second one. + // Covers a RETRY, for twenty-four hours, and nothing + // after that: Stripe forgets a key at the end of them. + // A crash between Stripe creating the Product and us + // storing its id therefore leaves an orphan, and the + // next run past the expiry makes a second Product for + // this family. There is no recognition step for Products + // — App\Services\Billing\AdoptStripePrice covers Prices + // only — so nothing here ever asks Stripe what Products + // it already has. A KNOWN GAP, not something this key + // closes, and a worse one than a duplicate Price: + // activePricesFor() would then be asked about the new + // Product, and the Price recognition goes blind for the + // whole family. + // + // IdempotencyKey::forProduct() folds the name and the + // metadata into what goes on the wire, so a renamed + // family under an unstored id now mints a second Product + // where it used to answer HTTP 400 for a day. That is + // the trade this branch chose deliberately: a blockade + // reaches a paying customer, a duplicate Product does + // not. idempotencyKey: "clupilot-product-{$family->id}", ); $family->update(['stripe_product_id' => $productId]); @@ -130,9 +148,16 @@ class SyncStripeCatalogue extends Command return self::SUCCESS; } + // "or adopted", because the count is taken BEFORE ensure() runs and + // ensure() may find the Price already at Stripe and adopt it instead of + // creating anything — see App\Services\Billing\AdoptStripePrice. Telling + // which of the two happened is the whole purpose of that step, so the + // first thing an operator reads after an interrupted run must not assert + // a creation that did not take place. The log entry adoption writes names + // the Price it took over. $this->info($dryRun - ? "{$created} object(s) would be created. Run without --dry-run to create them." - : "{$created} object(s) created in Stripe."); + ? "{$created} object(s) would be created or adopted. Run without --dry-run to do it." + : "{$created} object(s) created or adopted in Stripe."); return self::SUCCESS; } diff --git a/app/Services/Billing/AddonPrices.php b/app/Services/Billing/AddonPrices.php index e2aa6db..ecb49bd 100644 --- a/app/Services/Billing/AddonPrices.php +++ b/app/Services/Billing/AddonPrices.php @@ -166,7 +166,7 @@ final class AddonPrices // second Price for one figure is AdoptStripePrice above, not this: // Stripe forgets a key after twenty-four hours. The metadata is // folded in by IdempotencyKey inside the client, so changing the - // format below can never again refuse the call for a day. + // format above can never again refuse the call for a day. idempotencyKey: "clupilot-addon-price-{$addonKey}-{$interval}-{$amount}-{$currency}" .($reverseCharge ? '-rc' : ''), ); @@ -287,6 +287,11 @@ final class AddonPrices return $this->stripe->createProduct( app(AddonCatalogue::class)->name($addonKey), ['addon' => $addonKey], + // A retry's guard for twenty-four hours and nothing beyond them, the + // same as the plan side's in SyncStripeCatalogue — and the same known + // gap: there is no recognition step for Products, so a run that + // created this one and died before any row carried its id leaves an + // orphan nothing here will ever ask Stripe about. idempotencyKey: "clupilot-addon-product-{$addonKey}", ); } @@ -317,6 +322,17 @@ final class AddonPrices // one Price for both — the idempotency key saw to that — so there is // nothing to correct here beyond letting the first row stand. // + // Since the unique index on `stripe_price_id` below, this catch has a + // SECOND meaning: another TUPLE claims this Price id, which is two + // callers adopting the same orphan at once. The outcome then is no row + // for this tuple at all, not "the first row stands" — the row that + // exists belongs to the other tuple. That self-heals on the next + // ensure(): the orphan is claimed, so adoption refuses it and + // createPrice() mints this tuple its own Price. No money moves in the + // meantime, because both callers matched the same `unit_amount` + // before adopting; what a caller gets back here is the Price id it + // asked for, and it charges what was asked. + // // What keeps two ROWS off one Price id is the `claimed` callback in // ensure(), asked before minting. The unique index on // `stripe_price_id` (`stripe_addon_prices_price_unique`, since diff --git a/app/Services/Stripe/FakeStripeClient.php b/app/Services/Stripe/FakeStripeClient.php index 311b188..25d4ebc 100644 --- a/app/Services/Stripe/FakeStripeClient.php +++ b/app/Services/Stripe/FakeStripeClient.php @@ -220,8 +220,13 @@ class FakeStripeClient implements StripeClient 'interval' => $interval, 'metadata' => $metadata, // Stripe stamps every object with its creation time and - // AdoptStripePrice takes the OLDEST of several orphans. A counter - // is enough and beats a clock: it cannot tie. + // AdoptStripePrice takes the OLDEST of several orphans. Counted + // rather than clocked, because two Prices minted in the same second + // would share a timestamp — but the two counters are independent: + // plantPrice() stamps `created: 1` of its own accord, which ties with + // a Price minted here while this list was still empty. Any test whose + // outcome depends on which of two prices is older passes `created:` + // itself, as the adoption tests do. 'created' => count($this->prices) + 1, ]; @@ -254,11 +259,14 @@ class FakeStripeClient implements StripeClient // does not fail either, and a test that scripts an outage for a refund // must not have its catalogue sync change behaviour underneath it. // - // No interval_count/usage_type filter either, unlike HttpStripeClient's: - // this fake only ever holds prices its own createPrice()/plantPrice() put - // here, and neither lets a caller set anything but a standard monthly or - // yearly recurrence — there is nothing non-standard a test could plant, - // so the filter would have nothing to do. Not an oversight. + // None of HttpStripeClient's charging-property filter either — the four + // fields it names (interval_count, usage_type, transform_quantity, + // billing_scheme). This fake only ever holds prices its own + // createPrice()/plantPrice() put here, and neither takes an argument for + // any of them: there is nothing non-standard a test could plant, so the + // filter would have nothing to do. Not an oversight. What that filter + // does is proven where it lives, over Http::fake — see + // tests/Feature/Billing/StripeIdempotencyKeyTest.php. $found = []; foreach ($this->prices as $id => $price) { diff --git a/app/Services/Stripe/HttpStripeClient.php b/app/Services/Stripe/HttpStripeClient.php index 2058442..fc90f32 100644 --- a/app/Services/Stripe/HttpStripeClient.php +++ b/app/Services/Stripe/HttpStripeClient.php @@ -170,15 +170,35 @@ class HttpStripeClient implements StripeClient foreach ($data as $price) { $recurring = (array) ($price['recurring'] ?? []); - // Only prices we could have minted ourselves: createPrice() never - // sets either field, so Stripe defaults both, and anything else is - // not a standard monthly/yearly Price. Skipped here rather than - // returned, because AdoptStripePrice trusts the amount/currency/ - // interval triple as the WHOLE recurrence — a Price billed every - // three months, or on metered usage, would otherwise pass that - // triple on 'month' alone and be adopted as if it billed monthly. + // Only prices we could have minted ourselves. createPrice() sends + // product, unit_amount, currency, recurring[interval] and metadata + // and nothing else, so Stripe defaults every other field that + // decides what a Price CHARGES — and anything but those defaults + // is a Price this platform did not write. Skipped here rather than + // returned, because AdoptStripePrice compares amount, currency and + // interval only and trusts that triple as the whole of what a + // Price charges: + // + // - interval_count multiplies the period — 3 bills every three + // months — while `interval` still reads 'month'; + // - usage_type other than 'licensed' bills metered usage; + // - a transform_quantity divides the quantity before charging, + // and modules are billed BY quantity — SyncStripeAddonItems + // sums a pack into one item at quantity n — so divide_by 10 + // would charge a customer holding three for one; + // - billing_scheme 'tiered' keeps the money in tiers, leaving + // unit_amount null. Read as 0 below, so such a Price fails the + // amount match only while the caller's own figure is not 0 — + // and PlanPrices::ensure() has no zero guard, so that is not a + // condition to rest on. + // + // An ABSENT key is Stripe's default and no reason to reject. + // Read the other way round this filter would refuse every + // legitimate price and turn recognition into a permanent no-op. if ((int) ($recurring['interval_count'] ?? 1) !== 1 - || ($recurring['usage_type'] ?? 'licensed') !== 'licensed') { + || ($recurring['usage_type'] ?? 'licensed') !== 'licensed' + || (array) ($price['transform_quantity'] ?? []) !== [] + || ($price['billing_scheme'] ?? 'per_unit') !== 'per_unit') { continue; } diff --git a/app/Services/Stripe/StripeClient.php b/app/Services/Stripe/StripeClient.php index 45e6d30..0ffed51 100644 --- a/app/Services/Stripe/StripeClient.php +++ b/app/Services/Stripe/StripeClient.php @@ -138,15 +138,24 @@ interface StripeClient * here I can sell on?". `currency` comes back UPPER CASE, the way our own * tables hold it. * - * Only prices WE could have created ourselves. createPrice() never sets - * `recurring.interval_count` or `recurring.usage_type`, so anything besides - * Stripe's own defaults for them — one, and `licensed` — is filtered out - * here rather than returned. AdoptStripePrice trusts the amount/currency/ - * interval triple in the shape below as the WHOLE of a Price's recurrence; - * a Price billed every three months, or on metered usage, would otherwise - * pass that triple on `interval => 'month'` alone and be adopted as if it - * billed monthly — which is adoption moving money, the one thing nothing - * here may do. + * FOUR properties are checked, and they are named because the list is what + * the contract is: `recurring.interval_count` is 1, `recurring.usage_type` + * is `licensed`, there is no `transform_quantity`, and `billing_scheme` is + * `per_unit`. Those are Stripe's own defaults, and createPrice() sets none of + * the four — so anything else is a Price this platform did not write, and is + * filtered out here rather than returned. + * + * The reason it is filtered at all: AdoptStripePrice compares the amount, + * currency and interval of the shape below and trusts that triple as the + * WHOLE of what a Price charges. A Price billed every three months, on + * metered usage, dividing the quantity before charging, or pricing in tiers + * would otherwise pass that triple on `interval => 'month'` alone and be + * adopted as if it charged our figure per unit per month — which is adoption + * moving money, the one thing nothing here may do. + * + * Not a list of everything Stripe can put on a Price: it is the list of + * fields known to change what one CHARGES. A fifth would have to be added + * here, at the boundary, before the shape below is handed on. * * @return array}> */ diff --git a/database/migrations/2026_07_31_210000_one_row_per_stripe_price.php b/database/migrations/2026_07_31_210000_one_row_per_stripe_price.php index 8f43cba..359efba 100644 --- a/database/migrations/2026_07_31_210000_one_row_per_stripe_price.php +++ b/database/migrations/2026_07_31_210000_one_row_per_stripe_price.php @@ -29,6 +29,14 @@ use Illuminate\Support\Facades\Schema; * createPrice()). `subscription_addons` holds the Stripe id as text, not as a * foreign key, so no booking loses its price either way. * + * MariaDB does not run DDL in a transaction, so the deletes below are already + * committed by the time `Schema::table()` adds the index — a row inserted between + * the two on a Price id that is still shared aborts the migration with the deletes + * standing. Re-running is safe (the dedupe is idempotent and the index is simply + * not there yet), but the sweep and anything else calling + * AddonPrices::ensure() — a customer's module booking included — should be + * quiesced for the one real run this gets. + * * The `Log::warning` below is the only record this ever gets: `down()` restores * the index, not the rows it deleted. It names the shared Price, the row kept, * and every row thrown away, so an operator reading the log after the one real diff --git a/docs/superpowers/plans/2026-07-30-stripe-price-adoption.md b/docs/superpowers/plans/2026-07-30-stripe-price-adoption.md index 5bfa20f..6334685 100644 --- a/docs/superpowers/plans/2026-07-30-stripe-price-adoption.md +++ b/docs/superpowers/plans/2026-07-30-stripe-price-adoption.md @@ -23,6 +23,52 @@ --- +## Korrekturen + +**Dieses Dokument ist nicht geprüft, sondern korrigiert.** Die Reviews der +einzelnen Tasks haben **fünf Defekte im Plan selbst** gefunden — nicht in der +Umsetzung. Sie stehen hier, damit niemand die Anweisungen weiter unten für +verifiziert hält. Der vollständige Verlauf, Task für Task, liegt in +`.superpowers/sdd/2026-07-30-stripe-price-adoption/progress.md`. + +1. **Ein zerstörerischer Befehl.** Task 6, Step 5 verlangte + `php artisan migrate:fresh --env=testing`. Es gibt in diesem Repo keine + `.env.testing`, also hätte der Befehl die echte, von laufenden Containern + benutzte Entwicklungsdatenbank neu gebaut. Der Umsetzer hat den Schritt + verweigert — richtig — und den Index über den SQLite-Migrationslauf der Suite + belegt. Der Schritt steht unten durchgestrichen mit Warnung, statt gelöscht. +2. **Eine Begründung, deren Mechanismus nicht feuern kann.** Der + Migrations-Kommentar im vorgeschriebenen Codeblock von Task 6 sagt, die + gelöschte Zeile baue sich wieder auf, indem `ensure()` den Preis „through the + adoption step" bei Stripe findet. Genau das kann nicht passieren: nach dem + Entdoppeln beansprucht die überlebende Zeile diese Preis-ID, also lehnt + `AdoptStripePrice` sie über die `claimed`-Prüfung immer ab. Der Wiederaufbau + ist echt, läuft aber über `createPrice()` unter dem tupel-eigenen Schlüssel. + Die ausgelieferte Migration ist berichtigt; der Plantext schreibt die falsche + Fassung weiter vor. Die Commit-Botschaft in Step 8 sagt nur „the row rebuilds + itself on the next ensure()" und nennt den Mechanismus nicht — das ist wahr, + aber es ist nicht die Begründung, die ausgeliefert wurde. +3. **Ein tautologischer Test.** Der einzige Test, der in Task 4 die + Auftraggeber-Regel „jede Buchung friert ihren Preis ein" bewachte, prüfte + `subscription_addons.stripe_price_id` nach einem `stripe:sync-catalogue`-Lauf — + eine Spalte, die dieses Kommando nie schreibt. Er war grün, bevor + `AddonPrices.php` überhaupt angefasst war. +4. **Zwei Plan-Tests, die mit entfernter Übernahme bestanden.** Beide neuen Tests + aus Task 5 liefen grün, nachdem der `adopt()`-Aufruf entfernt war. Ursache: + sie erzeugten ihre Waise über einen echten Sync-Lauf, sodass der + Schlüssel-Ledger des Fakes dieselbe Preis-ID zurückspielte — also genau der + Zustand (Schlüssel noch in Kraft), in dem es nichts zu beweisen gibt. Der + Vorfall vom 29.07. konnte erst **nach** dem Ablauf des Schlüssels entstehen. +5. **Drei falsche Testzahlen.** Die „Expected"-Zeilen von Task 1, 2 und 3 nennen + sieben, zehn und acht Tests; die dort vorgeschriebenen Dateien enthalten sechs, + neun und sieben. + +Die Abschnitte darunter sind **absichtlich nicht umgeschrieben** — die +vorgeschriebenen Codeblöcke bleiben stehen, wie sie umgesetzt wurden, damit Plan +und Verlauf vergleichbar bleiben. Diese Liste ist die Korrektur. + +--- + ## Dateistruktur **Neu** @@ -1769,13 +1815,11 @@ php artisan test tests/Feature/Billing/StripePriceAdoptionTest.php Expected: PASS, vierzehn Tests. -- [ ] **Step 5: Prove the deduplication itself, on a real migration run** +- [ ] ~~**Step 5: Prove the deduplication itself, on a real migration run**~~ — **NICHT AUSFÜHREN** -```bash -php artisan migrate:fresh --env=testing && php artisan migrate:status | tail -3 -``` +~~`php artisan migrate:fresh --env=testing && php artisan migrate:status | tail -3`~~ -Expected: die neue Migration steht als `Ran`. (Sie hat auf einer leeren Tabelle nichts zu entdoppeln — der Test in Step 1 prüft den Index, und der Entdopplungszweig ist gegen eine Live-Tabelle geschrieben, in der es vermutlich keine Doppel gibt.) +> **Warnung, nicht stillschweigend gelöscht, weil die Falle es wert ist:** Dieser Befehl darf nicht laufen — es gibt in diesem Repo **keine** `.env.testing`, also greift die Verbindung aus `.env` und `migrate:fresh` löscht und baut die **echte Entwicklungsdatenbank** neu, die laufende Container benutzen. Der Index ist durch den SQLite-Migrationslauf der Suite belegt (Step 1 und Step 4); ein zweiter Beleg ist keinen Datenverlust wert. - [ ] **Step 6: Mark the Block D item done** diff --git a/tests/Feature/Billing/StripeIdempotencyKeyTest.php b/tests/Feature/Billing/StripeIdempotencyKeyTest.php index 3ea6478..ead4498 100644 --- a/tests/Feature/Billing/StripeIdempotencyKeyTest.php +++ b/tests/Feature/Billing/StripeIdempotencyKeyTest.php @@ -192,9 +192,53 @@ it('skips a price it could not have created itself, so the money gate never trus // A hand-duplicated dashboard price keeps our metadata, so nothing // downstream could tell it apart, and the module would bill quarterly. + // + // None of the three planted prices carries `transform_quantity` or + // `billing_scheme` at all, and the ordinary one still comes back — so this + // also holds the other direction of the next test's filter: an absent key is + // Stripe's default, not a reason to reject. expect(collect($prices)->pluck('id')->all())->toBe(['price_monthly']); }); +it('skips a price that would charge our figure for the wrong quantity', function () { + Http::fake([ + 'api.stripe.com/*' => Http::response([ + 'data' => [ + // Spelled out as Stripe actually sends them for an ordinary + // price: transform_quantity null, billing_scheme per_unit. + ['id' => 'price_ordinary', 'unit_amount' => 3480, 'currency' => 'eur', + 'created' => 100, 'recurring' => ['interval' => 'month'], + 'transform_quantity' => null, 'billing_scheme' => 'per_unit', + 'metadata' => ['addon' => 'priority_support']], + ['id' => 'price_divided', 'unit_amount' => 3480, 'currency' => 'eur', + 'created' => 101, 'recurring' => ['interval' => 'month'], + 'transform_quantity' => ['divide_by' => 10, 'round' => 'up'], + 'billing_scheme' => 'per_unit', + 'metadata' => ['addon' => 'priority_support']], + ['id' => 'price_tiered', 'unit_amount' => null, 'currency' => 'eur', + 'created' => 102, 'recurring' => ['interval' => 'month'], + 'billing_scheme' => 'tiered', + 'metadata' => ['addon' => 'priority_support']], + ], + 'has_more' => false, + ]), + ]); + + $prices = (new HttpStripeClient)->activePricesFor('prod_1'); + + // The concrete harm: modules are billed BY quantity — SyncStripeAddonItems + // sums a pack into ONE item at quantity n — so price_divided is our exact + // figure, on our Product, carrying our `addon` key, and adoption would take + // it. A customer holding three would then be charged ceil(3/10) = 1. + // + // price_tiered is refused by name rather than by luck: Stripe reports + // unit_amount null for a tiered price, activePricesFor() casts that to 0, and + // the amount match alone only rejects it while the caller's own figure is not + // 0 — which PlanPrices::ensure(), unlike AddonPrices::ensure(), does not + // guarantee. + expect(collect($prices)->pluck('id')->all())->toBe(['price_ordinary']); +}); + it('writes metadata onto a price that already exists', function () { Http::fake(['api.stripe.com/*' => Http::response(['id' => 'price_a'])]); diff --git a/tests/Feature/Billing/StripePriceAdoptionTest.php b/tests/Feature/Billing/StripePriceAdoptionTest.php index de92800..df68ab1 100644 --- a/tests/Feature/Billing/StripePriceAdoptionTest.php +++ b/tests/Feature/Billing/StripePriceAdoptionTest.php @@ -10,6 +10,7 @@ use App\Services\Billing\AdoptStripePrice; use App\Services\Billing\PlanPrices; use App\Services\Billing\TaxTreatment; use App\Services\Stripe\FakeStripeClient; +use App\Services\Stripe\IdempotencyKey; use App\Services\Stripe\StripeClient; use Illuminate\Contracts\Console\Kernel; use Illuminate\Database\Schema\Blueprint; @@ -199,10 +200,38 @@ it('does not block a booking because the metadata format moved', function () { 'stripe_price_id' => 'price_yearly_already_known', ]); + // Stripe's own side of that morning, and load-bearing for the same reason as + // the cleared ledger in 'takes over an orphaned package price' below — only + // the other way round. Without an entry here createPrice() would simply mint, + // and the test could not tell "adoption prevented the 400" from "no 400 was + // possible". So the ledger is seeded with what the pre-9da1358 call left + // behind: the id it was answered with, and a fingerprint over the metadata + // that call sent — `addon` alone, before tax_treatment existed. The entry sits + // under the header today's code sends, because that is the only way a ledger + // keyed by header can put this createPrice() in front of a poisoned key: the + // fingerprint is folded into the header now, so a changed call also changes + // the header, which is the branch's OTHER half of the same fix. Seeded + // directly, therefore, and honestly so — what it stands in for is Stripe + // holding this call's key against different parameters. + $this->stripe->keys[IdempotencyKey::forPrice( + 'clupilot-addon-price-priority_support-month-3480-EUR', + 'prod_support', 3480, 'EUR', 'month', moduleMetadata(), + )] = [ + 'id' => 'price_1TygdEC7u8NpJ8pOt3nsoyYw', + 'fingerprint' => IdempotencyKey::fingerprint(IdempotencyKey::priceParameters( + 'prod_support', 3480, 'EUR', 'month', ['addon' => 'priority_support'], + )), + ]; + $id = app(AddonPrices::class)->ensure( 'priority_support', 2900, 'EUR', Subscription::TERM_MONTHLY, TaxTreatment::domestic(), ); + // Adoption is what keeps createPrice() from ever being reached. Take the + // adopt() call out of AddonPrices::ensure() and this test does not merely + // return the wrong id — it dies inside the fake with Stripe's own sentence, + // 'Keys for idempotent requests can only be used with the same parameters + // they were first used with.' That is the 2026-07-29 blockade, reproduced. expect($id)->toBe('price_1TygdEC7u8NpJ8pOt3nsoyYw') ->and($this->stripe->metadataUpdates)->toHaveCount(1); });