From cb9781d2e7e6f0b9839569c1c3b1c89f8eacc549 Mon Sep 17 00:00:00 2001 From: nexxo Date: Thu, 30 Jul 2026 13:07:24 +0200 Subject: [PATCH] Name the mechanism that actually rebuilds the row, and log what the delete removed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The migration's own docblock justified an irreversible delete with a mechanism that cannot fire: after the dedupe the deleted row's Price id is always still claimed, by the survivor, so AdoptStripePrice's `claimed` callback always skips it. The rebuild is real, but it goes through a fresh createPrice() under the deleted row's own tuple-specific idempotency key, not through adoption. A comment whose whole job is to justify deleting production rows has to name the mechanism that runs, not the one that structurally cannot — this is the fourth false justification this branch has carried, and the incident the whole feature exists for started the same way. The delete itself left no trace: down() restores the index, not the rows, and two rows on one Price necessarily have different tuples, so at most one can match what the Price actually charges — after the one real run there would be nothing to check the survivor against. Added a Log::warning per duplicate group naming the shared Price, the row kept, and every row thrown away. Test: read back stripe_price_id and amount_cents on the surviving and unrelated rows instead of only checking they still exist, closing the "kept the right row but damaged it" gap. Added a second duplicate group of three rows sharing one Price so the migration's foreach runs more than once and a single iteration deletes more than one row. Re-verified the "lowest id survives" assertion is load-bearing by flipping min('id') to max('id') during this fix and confirming the test fails there, then reverting. Co-Authored-By: Claude Opus 5 --- ..._07_31_210000_one_row_per_stripe_price.php | 49 ++++++++++++-- .../Billing/StripePriceAdoptionTest.php | 67 ++++++++++++++++--- 2 files changed, 102 insertions(+), 14 deletions(-) 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