From d6ec09a9b40e98131b72246bd2c8499d86bff53d Mon Sep 17 00:00:00 2001 From: nexxo Date: Thu, 30 Jul 2026 17:39:15 +0200 Subject: [PATCH] Recognise the product Stripe already has MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gap beside the guard. A run that creates a product and dies before storing its id leaves an orphan exactly as a price does, and past the key's twenty-four hours the next run makes a second one — after which activePricesFor() is asked about the wrong product, price recognition goes blind for the whole family, and every orphaned price on the first product is unreachable. Two things are unlike the price side, deliberately. No money gate: a product carries no amount, so its metadata is the whole of the proof. And a duplicate is reported, never deactivated — archiving a price is provably harmless because Stripe keeps billing subscriptions already on it, but deactivating a product makes its prices unsellable and contracts can be running on those. Silent about strangers, too. This asks for every active product on the account, so an operator selling something else through it is an ordinary state, not a finding worth a line on every run. Co-Authored-By: Claude Opus 5 --- app/Console/Commands/SyncStripeCatalogue.php | 38 ++--- app/Providers/AppServiceProvider.php | 8 + app/Services/Billing/AddonPrices.php | 19 ++- app/Services/Billing/AdoptStripeProduct.php | 137 ++++++++++++++++++ .../Billing/StripeProductAdoptionTest.php | 113 +++++++++++++++ 5 files changed, 286 insertions(+), 29 deletions(-) create mode 100644 app/Services/Billing/AdoptStripeProduct.php create mode 100644 tests/Feature/Billing/StripeProductAdoptionTest.php diff --git a/app/Console/Commands/SyncStripeCatalogue.php b/app/Console/Commands/SyncStripeCatalogue.php index b34071c..7f32e41 100644 --- a/app/Console/Commands/SyncStripeCatalogue.php +++ b/app/Console/Commands/SyncStripeCatalogue.php @@ -8,6 +8,7 @@ use App\Models\PlanVersion; use App\Models\Subscription; use App\Services\Billing\AddonCatalogue; use App\Services\Billing\AddonPrices; +use App\Services\Billing\AdoptStripeProduct; use App\Services\Billing\PlanPrices; use App\Services\Billing\TaxTreatment; use App\Services\Stripe\StripeClient; @@ -101,32 +102,25 @@ class SyncStripeCatalogue extends Command $created++; if (! $dryRun) { - $productId = $stripe->createProduct( + $metadata = ['plan_family' => $family->key, 'plan_family_id' => (string) $family->id]; + + // Asked BEFORE minting, for the same reason the Price side + // asks: a run that died between Stripe's create and our + // update left a Product this row does not know, and the key + // below stops protecting it after twenty-four hours. + // plan_family_id is what proves such a Product is this + // family's. + $productId = app(AdoptStripeProduct::class)($metadata, ['plan_family_id']); + + $productId ??= $stripe->createProduct( $family->name, - ['plan_family' => $family->key, 'plan_family_id' => (string) $family->id], + $metadata, // 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. + // beyond them. What stops a second Product for one + // family is the adoption step above. idempotencyKey: "clupilot-product-{$family->id}", ); + $family->update(['stripe_product_id' => $productId]); } } diff --git a/app/Providers/AppServiceProvider.php b/app/Providers/AppServiceProvider.php index 27c012d..1c5d75f 100644 --- a/app/Providers/AppServiceProvider.php +++ b/app/Providers/AppServiceProvider.php @@ -13,6 +13,7 @@ use App\Mail\Transport\MailboxTransport; use App\Models\Customer; use App\Models\MaintenanceNotification; use App\Provisioning\PipelineRegistry; +use App\Services\Billing\AdoptStripeProduct; use App\Services\Billing\CustomDomainAccess; use App\Services\Dns\FileHostDnsDirectory; use App\Services\Dns\HetznerDnsClient; @@ -57,6 +58,13 @@ class AppServiceProvider extends ServiceProvider config('provisioning.pipelines', []), )); + // Singletons because both carry a record across one run that the caller + // reads afterwards: which products were duplicates, and how many prices + // were adopted rather than created. Resolved per call they would each + // start empty — SyncStripeCatalogue asks the container for PlanPrices + // once per catalogue row. + $this->app->singleton(AdoptStripeProduct::class); + // Real I/O implementations; tests swap in fakes via app()->instance(). $this->app->bind(RemoteShell::class, PhpseclibRemoteShell::class); $this->app->bind(WireguardHub::class, LocalWireguardHub::class); diff --git a/app/Services/Billing/AddonPrices.php b/app/Services/Billing/AddonPrices.php index ecb49bd..7f01d10 100644 --- a/app/Services/Billing/AddonPrices.php +++ b/app/Services/Billing/AddonPrices.php @@ -284,14 +284,19 @@ final class AddonPrices return $existing; } - return $this->stripe->createProduct( + $metadata = ['addon' => $addonKey]; + + // Asked before minting. A run that created this Product and died before + // any row carried its id leaves an orphan — and a second Product would + // make activePricesFor() ask about the wrong one, blinding the Price + // recognition for this module entirely. + $adopted = app(AdoptStripeProduct::class)($metadata, ['addon']); + + return $adopted ?? $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. + $metadata, + // A retry's guard for twenty-four hours and nothing beyond them; + // what stops a second Product is the adoption step above. idempotencyKey: "clupilot-addon-product-{$addonKey}", ); } diff --git a/app/Services/Billing/AdoptStripeProduct.php b/app/Services/Billing/AdoptStripeProduct.php new file mode 100644 index 0000000..4f5f46a --- /dev/null +++ b/app/Services/Billing/AdoptStripeProduct.php @@ -0,0 +1,137 @@ + + */ + public array $duplicates = []; + + public function __construct(private readonly StripeClient $stripe) {} + + /** + * The id of an existing Stripe Product to use instead of creating one. + * + * @param array $metadata what the create call would send + * @param array $identifying metadata keys that mark a Product as ours + */ + public function __invoke(array $metadata, array $identifying): ?string + { + $metadata = array_map(fn ($value) => (string) $value, $metadata); + + $candidates = []; + + foreach ($this->stripe->activeProducts() as $product) { + if ($this->contradicts($product['metadata'], $metadata)) { + continue; + } + + if (! $this->confirms($product['metadata'], $metadata, $identifying)) { + continue; + } + + $candidates[] = $product; + } + + if ($candidates === []) { + return null; + } + + // Oldest first, and on a tie the lower id, so two runs agree. The tie is + // not merely theoretical: createProduct()'s counter and plantProduct()'s + // default both start at 1, so a minted Product and a planted orphan can + // share a `created` — the same collision AdoptStripePrice's usort breaks + // the same way, documented at FakeStripeClient::createPrice(). + usort($candidates, fn (array $a, array $b) => [$a['created'], $a['id']] <=> [$b['created'], $b['id']]); + + $adopted = array_shift($candidates); + + foreach ($candidates as $duplicate) { + $this->duplicates[] = $duplicate['id']; + + Log::warning('stripe: a second product claims to be the same thing — left alone, not deactivated', [ + 'product' => $duplicate['id'], + 'adopted' => $adopted['id'], + 'metadata' => $metadata, + ]); + } + + Log::info('stripe: adopted an existing product instead of creating a second one', [ + 'product' => $adopted['id'], + 'metadata' => $metadata, + ]); + + return $adopted['id']; + } + + /** + * @param array $found + * @param array $expected + */ + private function contradicts(array $found, array $expected): bool + { + foreach ($expected as $key => $value) { + if (array_key_exists($key, $found) && $found[$key] !== $value) { + return true; + } + } + + return false; + } + + /** + * Failing to contradict is not enough — an empty metadata bag contradicts + * nothing, and this list holds every Product on the account. + * + * @param array $found + * @param array $expected + * @param array $identifying + */ + private function confirms(array $found, array $expected, array $identifying): bool + { + foreach ($identifying as $key) { + if (isset($found[$key]) && $found[$key] === ($expected[$key] ?? null)) { + return true; + } + } + + return false; + } +} diff --git a/tests/Feature/Billing/StripeProductAdoptionTest.php b/tests/Feature/Billing/StripeProductAdoptionTest.php new file mode 100644 index 0000000..3379629 --- /dev/null +++ b/tests/Feature/Billing/StripeProductAdoptionTest.php @@ -0,0 +1,113 @@ +stripe = new FakeStripeClient; + app()->instance(StripeClient::class, $this->stripe); +}); + +it('adopts an orphaned family product instead of minting a second one', function () { + // A run that got as far as creating the Product and died before storing its + // id: Stripe has it, plan_families does not. + $family = PlanFamily::query()->orderBy('tier')->firstOrFail(); + $family->update(['stripe_product_id' => null]); + + $this->stripe->plantProduct('prod_orphan_family', $family->name, [ + 'plan_family' => $family->key, + 'plan_family_id' => (string) $family->id, + ]); + + app(Kernel::class)->call('stripe:sync-catalogue'); + + // Counting every product would prove nothing — the same run mints one for + // each of the OTHER families. What must hold is that this family's id is + // carried by exactly one product, and that it is the planted one. + expect($family->refresh()->stripe_product_id)->toBe('prod_orphan_family') + ->and(collect($this->stripe->products) + ->filter(fn (array $p) => ($p['metadata']['plan_family_id'] ?? null) === (string) $family->id) + ->keys()->all())->toBe(['prod_orphan_family']); +}); + +it('adopts an orphaned module product instead of minting a second one', function () { + $this->stripe->plantProduct('prod_orphan_module', 'Priority Support', [ + 'addon' => 'priority_support', + ]); + + $before = count($this->stripe->products); + + app(AddonPrices::class)->ensure( + 'priority_support', 2900, 'EUR', Subscription::TERM_MONTHLY, TaxTreatment::domestic(), + ); + + expect(StripeAddonPrice::query() + ->where('addon_key', 'priority_support') + ->value('stripe_product_id'))->toBe('prod_orphan_module') + ->and(count($this->stripe->products))->toBe($before); +}); + +it('adopts nothing a product does not prove about itself', function () { + // What an operator selling something else through the same Stripe account + // leaves lying around. Silent, unlike the Price side: we list every product + // on the account, so warning about each foreign one would fire on every run + // for as long as it exists. + Log::spy(); + + $this->stripe->plantProduct('prod_stranger', 'Etwas ganz anderes', []); + + expect(app(AdoptStripeProduct::class)( + ['addon' => 'priority_support'], ['addon'], + ))->toBeNull(); + + Log::shouldNotHaveReceived('warning'); +}); + +it('adopts nothing that contradicts what we would have written', function () { + $this->stripe->plantProduct('prod_other_module', 'Collabora Pro', ['addon' => 'collabora_pro']); + + expect(app(AdoptStripeProduct::class)( + ['addon' => 'priority_support'], ['addon'], + ))->toBeNull(); +}); + +it('takes the oldest of two matching products and leaves the other alone', function () { + Log::spy(); + + $this->stripe->plantProduct('prod_second', 'Priority Support', ['addon' => 'priority_support'], created: 200); + $this->stripe->plantProduct('prod_first', 'Priority Support', ['addon' => 'priority_support'], created: 100); + + $adopt = app(AdoptStripeProduct::class); + + expect($adopt(['addon' => 'priority_support'], ['addon']))->toBe('prod_first') + // NOT deactivated. Its prices would become unsellable, and a contract + // can be running on one of them — the asymmetry with archivePrice(). + ->and($adopt->duplicates)->toBe(['prod_second']); + + Log::shouldHaveReceived('warning')->once(); +});