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(); +});