Say what the sweep did not check, before telling anyone to archive
stripe:sweep-orphan-prices described itself as listing "what AdoptStripePrice will not take over". It computes nothing of the sort: it lists every active Price at one of our Products that no register row knows, adoptable or not — the only question it asks is whether a row holds the id. And then it invited the destructive path, with no word anywhere about running stripe:sync-catalogue first. Following that advice can make a plan unsellable. Archive an orphan the next sync would have adopted and the sync can no longer see it: activePricesFor() asks Stripe for active prices only, so adoption returns null and createPrice() fires under the same key the crashed run used. For twenty-four hours Stripe answers a repeated key by replaying the stored response — the archived Price's id, in the body it had while it was live. The register writes it down as live, and the checkout is refused. This is the same class of defect the branch exists against: a comment that was eloquent and wrong. - The docblock now says what the command lists, names the twenty-four-hour replay so nobody has to rediscover it, and the ordering requirement is in $description and printed on every path that found something, --archive included. The reassurance that archiving is harmless is now scoped to what THIS codebase sells; a payment link an operator built by hand against one of our Products is withdrawn all the same, and says so. - stripe:sync-catalogue --dry-run says "created or adopted" again. A dry run counts intents without calling ensure(), so it cannot know which Stripe already holds — and a dry run is the first thing an operator runs after an interrupted one. The live line keeps both figures, where they are knowable. - The sweep's plan side gets a test: an orphan at a family Product is reported and a known plan Price never is. Deleting either half of the lookup turns it red; the five tests it joins all stayed green for the products() half. - The throttle test pinned "once" and neither "per price" nor "per day". A key mutated to a constant would have silenced every orphan after the first and kept it green — the silent no-op the branch is built against. It now plants a second orphan, and travels a day to hear the first one again. - Two of my own documents corrected. The spec sentence claiming an adoptable orphan never appears in this list is what the implementation faithfully built; it holds only if a sync has run since the orphan appeared, which nothing enforced. And the two adoption classes do not carry the same run log: $adoptions on the Price side, $duplicates on the Product side. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>feature/host-bootstrap
parent
571acd0013
commit
a43be4186a
|
|
@ -18,10 +18,33 @@ use Illuminate\Console\Command;
|
|||
* was the right call and it left an operator with a log line and no way to act
|
||||
* on it.
|
||||
*
|
||||
* **What this lists is wider than that log line.** Every active Price at one of
|
||||
* our Products that no row of ours knows — adoptable or not. The only question
|
||||
* asked below is whether a register row holds the id; nothing here re-asks
|
||||
* AdoptStripePrice whether it would have taken the Price over.
|
||||
*
|
||||
* **So stripe:sync-catalogue runs first.** Until it has, an orphan listed here
|
||||
* may be one the next sync would simply have adopted, and --archive on that one
|
||||
* is worse than it looks. Archiving it makes it invisible to the very step that
|
||||
* would have rescued it: activePricesFor() asks Stripe for ACTIVE prices only,
|
||||
* so adoption finds nothing and the sync falls through to createPrice() — under
|
||||
* the same idempotency key the crashed run used, because the key is built from
|
||||
* what is being sold and everything the call puts on the wire (see
|
||||
* App\Services\Stripe\IdempotencyKey), and nothing about either has changed. For
|
||||
* twenty-four hours Stripe answers a repeated key by replaying the response it
|
||||
* stored rather than doing anything: the archived Price's id, in the body it had
|
||||
* while it was still live. The register writes that id down as the live Price
|
||||
* for the row, and a checkout on it is refused.
|
||||
*
|
||||
* Without --archive this reports and touches nothing. With it, the orphans are
|
||||
* archived at Stripe, which is harmless for the reason it always is in this
|
||||
* codebase: Stripe goes on billing every subscription already on an archived
|
||||
* Price. It stops being SOLD, and nothing here is sold on a Price no row knows.
|
||||
* archived at Stripe, which is harmless for everything THIS codebase sells, for
|
||||
* the reason it always is here: Stripe goes on billing every subscription
|
||||
* already on an archived Price, it only stops being SOLD — and nothing this
|
||||
* codebase sells is sold on a Price no row knows. That reasoning reaches no
|
||||
* further than this codebase. A Price an operator wired up by hand — a payment
|
||||
* link built in Stripe's own dashboard against one of our Products — is exactly
|
||||
* the Price carrying none of our metadata described above, and --archive
|
||||
* withdraws it too.
|
||||
*/
|
||||
class SweepOrphanStripePrices extends Command
|
||||
{
|
||||
|
|
@ -29,7 +52,7 @@ class SweepOrphanStripePrices extends Command
|
|||
{--archive : Stop selling the orphans instead of only listing them}
|
||||
{--dry-run : Show what would be archived without touching Stripe}';
|
||||
|
||||
protected $description = 'List the Stripe prices at our products that no row knows, and optionally archive them';
|
||||
protected $description = 'List the Stripe prices at our products that no row knows, and optionally archive them — run stripe:sync-catalogue first';
|
||||
|
||||
public function handle(StripeClient $stripe): int
|
||||
{
|
||||
|
|
@ -87,10 +110,18 @@ class SweepOrphanStripePrices extends Command
|
|||
return self::SUCCESS;
|
||||
}
|
||||
|
||||
// Said on every path that found something, including after --archive
|
||||
// has already run: the only question asked above is whether a row holds
|
||||
// the id, so an orphan in this list may be one stripe:sync-catalogue
|
||||
// would have adopted. Archiving that one is the failure this class's
|
||||
// docblock describes, and an operator who has just done it needs to
|
||||
// know as much as one who is about to.
|
||||
$this->warn(' Run stripe:sync-catalogue first — an orphan listed here may be one the next sync would adopt.');
|
||||
|
||||
$this->info(match (true) {
|
||||
$dryRun && $archive => "{$found} orphan(s) would be archived. Run without --dry-run to archive them.",
|
||||
$archive => "{$found} orphan(s) archived. Existing subscriptions on them keep billing.",
|
||||
default => "{$found} orphan(s) found. Run with --archive to stop selling them.",
|
||||
default => "{$found} orphan(s) found. Once the sync has run, --archive stops selling what is still listed.",
|
||||
});
|
||||
|
||||
return self::SUCCESS;
|
||||
|
|
|
|||
|
|
@ -194,8 +194,14 @@ class SyncStripeCatalogue extends Command
|
|||
return self::SUCCESS;
|
||||
}
|
||||
|
||||
// "or adopted" on the dry run, and only there. A dry run counts intents
|
||||
// without ever calling ensure(), so it cannot know which of them Stripe
|
||||
// already holds a Price for — and a dry run is the first thing an
|
||||
// operator runs after an interrupted one, which is exactly when some of
|
||||
// these will be adopted rather than minted. The live line below has
|
||||
// both figures because by then the adoption step has answered.
|
||||
$this->info($dryRun
|
||||
? "{$created} object(s) would be created. Run without --dry-run to create them."
|
||||
? "{$created} object(s) would be created or adopted. Run without --dry-run to create them."
|
||||
: "{$minted} object(s) created, {$adopted} adopted in Stripe.");
|
||||
|
||||
return self::SUCCESS;
|
||||
|
|
|
|||
|
|
@ -223,6 +223,20 @@ Kette an einer anderen Stelle, und mehrere brechen sie **nach** der Zahlung.
|
|||
`stripe:reprice-subscriptions` (je zuerst `--dry-run`). Der Abgleich legt jetzt
|
||||
**zwei** Preise je Paket an; bis er lief, kann ein geprüfter EU-Firmenkunde nicht
|
||||
bestellen — das ist Absicht.
|
||||
- `stripe:sweep-orphan-prices` listet jeden **aktiven** Stripe-Preis an unseren
|
||||
Produkten, den **keine** Zeile in `stripe_addon_prices` oder
|
||||
`stripe_plan_prices` kennt — mit ID, Betrag, Währung, Intervall und Metadaten.
|
||||
Ohne Argument wird nichts angefasst; mit `--archive` werden die aufgelisteten
|
||||
Preise bei Stripe stillgelegt, also **nicht mehr verkauft** (laufende Abos
|
||||
darauf verrechnet Stripe weiter). **Zuerst `stripe:sync-catalogue` laufen
|
||||
lassen:** das Kommando prüft nur, ob eine Zeile die ID kennt, nicht ob der
|
||||
Wiedererkennungsschritt die Waise übernommen hätte. Eine übernehmbare Waise
|
||||
vorher stillzulegen macht sie für den Abgleich unsichtbar (er fragt Stripe nur
|
||||
nach aktiven Preisen) — er fällt dann auf `createPrice()` mit demselben
|
||||
Idempotenz-Schlüssel zurück, den der abgebrochene Lauf benutzt hat, und Stripe
|
||||
antwortet 24 Stunden lang mit der gespeicherten Antwort: der ID des
|
||||
stillgelegten Preises. Danach steht im Register ein Preis, auf den Stripe
|
||||
keinen Checkout mehr öffnet.
|
||||
|
||||
---
|
||||
|
||||
|
|
|
|||
|
|
@ -138,9 +138,25 @@ kann etwas mit ihr tun, ohne von Hand in Stripes Oberfläche zu gehen.
|
|||
- **`--dry-run`** wie bei `stripe:sync-catalogue`, damit die beiden Kommandos
|
||||
sich gleich anfühlen.
|
||||
|
||||
Eine Waise, die der Wiedererkennungsschritt hätte übernehmen können, taucht hier
|
||||
gar nicht erst auf — sie wurde beim letzten Abgleich übernommen. Was übrig
|
||||
bleibt, ist genau das, was §12 als „nicht übernehmbar" beschrieben hat.
|
||||
**Berichtigt am 2026-07-30.** Hier stand: „Eine Waise, die der
|
||||
Wiedererkennungsschritt hätte übernehmen können, taucht hier gar nicht erst auf
|
||||
— sie wurde beim letzten Abgleich übernommen." Das stimmt nur, **wenn seit dem
|
||||
Auftauchen der Waise ein Abgleich gelaufen ist**, und genau diese Vorbedingung
|
||||
erzwingt das Kommando nicht. Es fragt ausschließlich, ob eine Zeile die ID
|
||||
kennt; es fragt `AdoptStripePrice` nicht, ob die Waise übernehmbar wäre. Die
|
||||
Liste ist deshalb **weiter** als das, was §12 „nicht übernehmbar" nannte, und
|
||||
die Implementierung hat den falschen Satz getreu nachgebaut.
|
||||
|
||||
Die Vorbedingung lautet: **zuerst `stripe:sync-catalogue`, dann dieses
|
||||
Kommando.** Sie steht im Klassenkommentar, in `$description`, in der
|
||||
Berichtsausgabe und im Betriebsteil des Handoffs — nicht als Ordnungsliebe,
|
||||
sondern weil `--archive` auf eine übernehmbare Waise sie für den Abgleich
|
||||
unsichtbar macht (`activePricesFor()` fragt nur aktive Preise ab). Der Abgleich
|
||||
fällt dann auf `createPrice()` mit demselben Idempotenz-Schlüssel zurück, den
|
||||
der abgebrochene Lauf benutzt hat, und Stripe antwortet 24 Stunden lang mit der
|
||||
gespeicherten Antwort — der ID des stillgelegten Preises, im Zustand von
|
||||
damals. Das Register hält den Tarif dann für live auf einem Preis, auf den
|
||||
Stripe keinen Checkout öffnet.
|
||||
|
||||
## 5. Die Warnung wird gedrosselt
|
||||
|
||||
|
|
@ -250,7 +266,10 @@ Und an `StripeIdempotencyKeyTest.php`: Blättern **wirft** bei fehlender `id`
|
|||
- `app/Services/Billing/AddonPrices.php` — Produkt-Wiedererkennung in
|
||||
`product()`
|
||||
- `app/Providers/AppServiceProvider.php` — beide Adoptions-Klassen als
|
||||
Singleton, damit ihr Lauf-Protokoll (`$duplicates`) trägt
|
||||
Singleton, damit ihr Lauf-Protokoll über den ganzen Lauf trägt. Es heißt in
|
||||
den beiden Klassen **nicht** gleich: `AdoptStripePrice::$adoptions` zählt die
|
||||
übernommenen Preise, `AdoptStripeProduct::$duplicates` sammelt die IDs der
|
||||
doppelten Produkte.
|
||||
- `app/Console/Commands/SyncStripeCatalogue.php` — Produkt-Wiedererkennung,
|
||||
getrennte Zählung, Kommentar berichtigt
|
||||
- `tests/Feature/Billing/StripePriceAdoptionTest.php`,
|
||||
|
|
|
|||
|
|
@ -655,7 +655,7 @@ it('still adopts a price that carries every one of Stripe\'s defaults', function
|
|||
expect(adoptModulePrice())->toBe('price_ordinary');
|
||||
});
|
||||
|
||||
it('warns about an unexplained price once a day, not once a booking', function () {
|
||||
it('warns about an unexplained price once a day, per price, not once a booking', function () {
|
||||
Log::spy();
|
||||
|
||||
$this->stripe->plantPrice('price_by_hand', 'prod_support', 3480, 'EUR', 'month', []);
|
||||
|
|
@ -667,6 +667,32 @@ it('warns about an unexplained price once a day, not once a booking', function (
|
|||
adoptModulePrice();
|
||||
|
||||
Log::shouldHaveReceived('warning')->once();
|
||||
|
||||
// PER PRICE. A second unexplained orphan is a second thing to say, not a
|
||||
// repeat of the first — and "once" alone does not pin that: a key mutated
|
||||
// to a constant would silence every orphan after this file's first one and
|
||||
// leave the assertion above green. That is the silent no-op this whole
|
||||
// branch is built against.
|
||||
$this->stripe->plantPrice('price_by_hand_too', 'prod_support', 3480, 'EUR', 'month', []);
|
||||
|
||||
adoptModulePrice();
|
||||
|
||||
Log::shouldHaveReceived('warning')->twice();
|
||||
|
||||
// PER DAY. An orphan is a STATE, so the line is due again tomorrow — the
|
||||
// throttle exists to thin it out, not to say it once and never again. Past
|
||||
// the TTL by a minute rather than exactly on it, so this asserts the expiry
|
||||
// and not a boundary comparison.
|
||||
$this->travelTo(now()->addDay()->addMinute());
|
||||
|
||||
adoptModulePrice();
|
||||
|
||||
// Asked of the FIRST price by name: after the travel both orphans warn
|
||||
// again, and a count over all warnings could not tell "price_by_hand spoke
|
||||
// twice" from "price_by_hand_too spoke twice and the first stayed silent".
|
||||
Log::shouldHaveReceived('warning')
|
||||
->withArgs(fn (string $message, array $context) => $context['price'] === 'price_by_hand')
|
||||
->twice();
|
||||
});
|
||||
|
||||
it('breaks a tie on created by the price id, so two runs agree', function () {
|
||||
|
|
|
|||
|
|
@ -1,6 +1,8 @@
|
|||
<?php
|
||||
|
||||
use App\Models\PlanPrice;
|
||||
use App\Models\StripeAddonPrice;
|
||||
use App\Models\StripePlanPrice;
|
||||
use App\Services\Stripe\FakeStripeClient;
|
||||
use App\Services\Stripe\StripeClient;
|
||||
use Illuminate\Contracts\Console\Kernel;
|
||||
|
|
@ -47,6 +49,43 @@ it('never reports a price a row knows', function () {
|
|||
->assertSuccessful();
|
||||
});
|
||||
|
||||
it('sees the plan side too: a family product orphan is reported, a known plan price never', function () {
|
||||
// Every other test in this file plants its orphan under the MODULE Product,
|
||||
// so both halves of the plan side went untested: the PlanFamily half of
|
||||
// products() and the StripePlanPrice half of the $known set. Blind to
|
||||
// either, the command misses the LARGER half — a family Product collects a
|
||||
// Price per version, term, treatment and rate change, a module Product four.
|
||||
//
|
||||
// Both assertions sit in one test on purpose: each fails for one of the two
|
||||
// lines. Drop the PlanFamily pluck from products() and the family Product is
|
||||
// never visited, so the orphan below goes unreported. Drop the
|
||||
// StripePlanPrice pluck from $known and the row's own Price becomes an
|
||||
// orphan the command names.
|
||||
$row = PlanPrice::query()->firstOrFail();
|
||||
$familyProduct = (string) $row->version->family->stripe_product_id;
|
||||
|
||||
$knownPlanPrice = (string) StripePlanPrice::query()
|
||||
->where('plan_price_id', $row->id)
|
||||
->where('reverse_charge', false)
|
||||
->value('stripe_price_id');
|
||||
|
||||
// At the family's Product and at an amount no row of ours holds — what the
|
||||
// sync leaves behind when it dies between Stripe's create and our insert,
|
||||
// seen from the sweep's side.
|
||||
$this->stripe->plantPrice('price_family_orphan', $familyProduct,
|
||||
12300, (string) $row->currency, 'month', []);
|
||||
|
||||
$this->artisan('stripe:sweep-orphan-prices')
|
||||
->expectsOutputToContain('price_family_orphan')
|
||||
->doesntExpectOutputToContain($knownPlanPrice)
|
||||
->assertSuccessful();
|
||||
|
||||
expect($familyProduct)->not->toBe('')
|
||||
->and($knownPlanPrice)->not->toBe('')
|
||||
// Nothing was touched: this is the reporting path, not --archive.
|
||||
->and($this->stripe->archived)->toBe([]);
|
||||
});
|
||||
|
||||
it('stops selling the orphan when asked to', function () {
|
||||
$this->artisan('stripe:sweep-orphan-prices', ['--archive' => true])
|
||||
->assertSuccessful();
|
||||
|
|
|
|||
Loading…
Reference in New Issue