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 ba3cc10..8f43cba 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 @@ -3,6 +3,7 @@ use Illuminate\Database\Migrations\Migration; use Illuminate\Database\Schema\Blueprint; use Illuminate\Support\Facades\DB; +use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Schema; /** @@ -18,10 +19,22 @@ use Illuminate\Support\Facades\Schema; * * Duplicates are DELETED rather than archived. An archived row goes on claiming * the id, and a unique index knows nothing about `archived_at`. Deleting is safe - * because the row rebuilds itself: the next ensure() finds the Price at Stripe - * through the adoption step and writes the row again. `subscription_addons` - * holds the Stripe id as text, not as a foreign key, so no booking loses its - * price. + * because the row rebuilds itself — but not through adoption: the survivor keeps + * the shared Price, so AdoptStripePrice's `claimed` callback reports it taken no + * matter which row asks, and the deleted row's own tuple — necessarily a + * DIFFERENT one, since the composite unique on (addon_key, reverse_charge, + * amount_cents, currency, interval) from 2026_07_31_200000 forbids two rows + * sharing a tuple — mints a fresh Price of its own instead, under its own + * tuple-specific idempotency key (AddonPrices::ensure() falling through to + * createPrice()). `subscription_addons` holds the Stripe id as text, not as a + * foreign key, so no booking loses its price either way. + * + * 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 + * run this migration gets can check the survivor's `amount_cents` against what + * Stripe actually charges for that Price — the lowest id is written first, not + * necessarily right, and nothing else will say what was discarded. */ return new class extends Migration { @@ -36,9 +49,35 @@ return new class extends Migration ->pluck('stripe_price_id'); foreach ($shared as $priceId) { + $rows = DB::table('stripe_addon_prices')->where('stripe_price_id', $priceId)->get(); + // The lowest id stays — the row that was written first, which is the // one anything already billing is likeliest to have been reading. - $keep = DB::table('stripe_addon_prices')->where('stripe_price_id', $priceId)->min('id'); + $keep = (int) $rows->min('id'); + $kept = $rows->first(fn ($row) => (int) $row->id === $keep); + $discarded = $rows->reject(fn ($row) => (int) $row->id === $keep); + + // The only forensic trace this delete ever leaves — see the class + // docblock for why that has to be enough. + Log::warning('stripe: kept one row of several claiming one price, deleted the rest', [ + 'stripe_price_id' => $priceId, + 'kept' => [ + 'id' => $kept->id, + 'addon_key' => $kept->addon_key, + 'reverse_charge' => (bool) $kept->reverse_charge, + 'amount_cents' => $kept->amount_cents, + 'currency' => $kept->currency, + 'interval' => $kept->interval, + ], + 'deleted' => $discarded->map(fn ($row) => [ + 'id' => $row->id, + 'addon_key' => $row->addon_key, + 'reverse_charge' => (bool) $row->reverse_charge, + 'amount_cents' => $row->amount_cents, + 'currency' => $row->currency, + 'interval' => $row->interval, + ])->values()->all(), + ]); DB::table('stripe_addon_prices') ->where('stripe_price_id', $priceId) diff --git a/tests/Feature/Billing/StripePriceAdoptionTest.php b/tests/Feature/Billing/StripePriceAdoptionTest.php index aaf86ef..de92800 100644 --- a/tests/Feature/Billing/StripePriceAdoptionTest.php +++ b/tests/Feature/Billing/StripePriceAdoptionTest.php @@ -471,7 +471,7 @@ it('lets no two module rows claim one stripe price', function () { ->toThrow(UniqueConstraintViolationException::class); }); -it('deletes the newer of two rows already sharing one stripe price when the migration runs', function () { +it('keeps only the lowest-id row in every group of duplicates, and leaves the rest of the table alone', function () { // This is the one branch of 2026_07_31_210000_one_row_per_stripe_price that // deletes rows from a live table, and it cannot be reached through the // ordinary migrated schema: RefreshDatabase already ran this migration, so @@ -482,7 +482,7 @@ it('deletes the newer of two rows already sharing one stripe price when the migr // delete branch instead of finding nothing to do. Schema::table('stripe_addon_prices', fn (Blueprint $table) => $table->dropUnique('stripe_addon_prices_price_unique')); - // Two rows sharing one Price, differing in `reverse_charge` so the + // Group one: two rows sharing a Price, differing in `reverse_charge` so the // pre-existing COMPOSITE unique index (addon_key, reverse_charge, // amount_cents, currency, interval) does not itself refuse the insert — // the VAT-rate-zero case Block D warned about, where only the shared @@ -501,7 +501,34 @@ it('deletes the newer of two rows already sharing one stripe price when the migr 'created_at' => now(), 'updated_at' => now(), ]); - // On its own Price entirely, and must survive untouched. + // Group two: THREE rows sharing a second, different Price — its own tuple + // dimension varied per row (interval, then currency) so the composite index + // never fires here either. One group of two proves the loop runs; this one + // proves it runs more than once AND that a single iteration can delete more + // than one row. + $kept2 = DB::table('stripe_addon_prices')->insertGetId([ + 'addon_key' => 'priority_support', 'reverse_charge' => false, + 'amount_cents' => 3900, 'net_cents' => 3900, 'currency' => 'EUR', 'interval' => 'month', + 'stripe_product_id' => 'prod_support', 'stripe_price_id' => 'price_shared_two', + 'created_at' => now(), 'updated_at' => now(), + ]); + + $duplicate2a = DB::table('stripe_addon_prices')->insertGetId([ + 'addon_key' => 'priority_support', 'reverse_charge' => false, + 'amount_cents' => 3900, 'net_cents' => 3900, 'currency' => 'EUR', 'interval' => 'year', + 'stripe_product_id' => 'prod_support', 'stripe_price_id' => 'price_shared_two', + 'created_at' => now(), 'updated_at' => now(), + ]); + + $duplicate2b = DB::table('stripe_addon_prices')->insertGetId([ + 'addon_key' => 'priority_support', 'reverse_charge' => false, + 'amount_cents' => 3900, 'net_cents' => 3900, 'currency' => 'USD', 'interval' => 'month', + 'stripe_product_id' => 'prod_support', 'stripe_price_id' => 'price_shared_two', + 'created_at' => now(), 'updated_at' => now(), + ]); + + // On its own Price entirely — not part of any duplicate group — and must + // survive completely untouched. $unrelated = DB::table('stripe_addon_prices')->insertGetId([ 'addon_key' => 'extra_storage', 'reverse_charge' => false, 'amount_cents' => 1000, 'net_cents' => 1000, 'currency' => 'EUR', 'interval' => 'month', @@ -514,13 +541,35 @@ it('deletes the newer of two rows already sharing one stripe price when the migr // migration's up() a second time in this same process. (require database_path('migrations/2026_07_31_210000_one_row_per_stripe_price.php'))->up(); - // The LOWER id survives — the row that was written first, which is the one - // anything already billing is likeliest to have been reading. Were that - // ordering silently inverted, the surviving row could be one nothing ever - // pointed at. - expect(DB::table('stripe_addon_prices')->where('id', $kept)->exists())->toBeTrue() + $keptRow = DB::table('stripe_addon_prices')->where('id', $kept)->first(); + $kept2Row = DB::table('stripe_addon_prices')->where('id', $kept2)->first(); + $unrelatedRow = DB::table('stripe_addon_prices')->where('id', $unrelated)->first(); + + // The LOWER id survives in each group — the row that was written first, + // which is the one anything already billing is likeliest to have been + // reading. Were that ordering silently inverted, the surviving row could be + // one nothing ever pointed at. Read back, not merely checked for existence: + // a survivor that exists but was mangled in the process — the wrong + // stripe_price_id, or an amount_cents that no longer matches what the + // shared Price actually charges — would be exactly the "kept the right row + // but damaged it" failure existence alone cannot catch. + expect($keptRow)->not->toBeNull() + ->and($keptRow->stripe_price_id)->toBe('price_shared') + ->and((int) $keptRow->amount_cents)->toBe(2900) + ->and($kept2Row)->not->toBeNull() + ->and($kept2Row->stripe_price_id)->toBe('price_shared_two') + ->and((int) $kept2Row->amount_cents)->toBe(3900) + ->and($unrelatedRow)->not->toBeNull() + ->and($unrelatedRow->stripe_price_id)->toBe('price_untouched') + ->and((int) $unrelatedRow->amount_cents)->toBe(1000) + // Every duplicate in both groups is gone — the higher id from group + // one, and BOTH higher ids from group two's three-way tie. ->and(DB::table('stripe_addon_prices')->where('id', $duplicate)->exists())->toBeFalse() - ->and(DB::table('stripe_addon_prices')->where('id', $unrelated)->exists())->toBeTrue(); + ->and(DB::table('stripe_addon_prices')->where('id', $duplicate2a)->exists())->toBeFalse() + ->and(DB::table('stripe_addon_prices')->where('id', $duplicate2b)->exists())->toBeFalse() + // Six rows went in, three survive: nothing beyond the two kept + // duplicates and the untouched row is left standing. + ->and(DB::table('stripe_addon_prices')->count())->toBe(3); // And the index is back in force: a further row on a third, otherwise // distinct tuple, but the SAME Price id, is refused by stripe_price_id