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>
Three ways it did not. It replaced metadata where Stripe merges — and merging is
the property AdoptStripePrice's "write only when it differs" comparison is built
around. It handed metadata back uncast where the wire returns strings, so a test
could pass on an int that production's strict === rejects forever. And a tie on
`created` fell to insertion order, so two runs could adopt different prices.
Paging now throws instead of stopping when Stripe says there is another page and
the last item carries no id. invoiceLines() stops there and it costs a short
document; here it costs an unseen orphan and a second live price for one figure.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
AdoptStripePrice says in its own docblock that no adoption can move money, and
could compare three properties. The four that also decide what a Price charges
were filtered in HttpStripeClient — where FakeStripeClient cannot express them,
so no test of the class could reach the promise it makes, and a third client
implementation would drop the check in silence.
The client reports them now and the caller decides. Absent keys stay Stripe's
defaults, which is the direction that matters: read the other way round the gate
refuses every legitimate price and recognition becomes a no-op nothing alerts on.
And the unexplained-orphan warning is a state, not an event. It fired on every
sweep and every customer booking for as long as the orphan existed; once a day
per price is enough to act on.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2321854 removed it. That commit's own work — discarding an unconfirmed
registration and sweeping abandoned ones — is untouched and correct; what went
with it was every file of the Stripe price adoption merged an hour earlier:
AdoptStripePrice, IdempotencyKey, the unique-index migration, both test files,
the spec, the plan, and the edits in six more.
Nothing was lost. The commits stayed in history and the files stayed on disk,
untracked, which is what a staged deletion from a stale tree leaves behind.
Restored from 4eb90c8, the reviewed head, by explicit path — the fourteen paths
2321854 damaged and not one more, so the ten files that commit legitimately
added or changed keep exactly what it gave them.
This is the failure the repo's own rule exists to prevent: stage by path, never
`git add -A`, because a second session's index does not know what a first one
merged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
"Abmelden und neu registrieren" was advice that could not work. Signing out
frees nothing: the address is still held by the unique index, so registering
again with the address somebody meant fails — and every abandoned attempt
stayed in `users` for ever. Reported exactly that way, and it was right.
The button discards the account instead. Only an UNCONFIRMED one, and only
one with nothing behind it: the webhook creates a customer's login from a
paid checkout, so a user can sit on this page unconfirmed while already
having a contract, and deleting that is data loss dressed up as a
convenience. It refuses and says where to go. A confirmed account is closed,
not discarded — that is ConfirmCloseAccount, with an invoice history behind
it.
R23: confirmed in a modal. The modal mutates nothing; it dispatches the event
the page listens for, so the checks stay in the one place they already are
rather than being duplicated where they could drift.
And nothing has to be discarded by hand. clupilot:prune-unverified removes
registrations nobody confirmed after five days — long enough for somebody who
signed up on a Friday and found the mail in a spam folder on Monday, short
enough that a typo does not hold the correct address for a month. It skips a
confirmed account whatever its age, and any account with a customer record
behind it, matched on user_id AND on the address, because either link means
somebody is a customer. Every removal is logged with the address: a line that
disappears is what somebody asks about later, and a count answers nothing.
The deadline is said on the page as well as in the mail. Somebody who never
received the mail is looking at the page, and the deadline is the reason they
do not have to do anything about an attempt they abandon.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The money gate compared amount, currency and interval and left the rest to a
filter that excluded two fields. A recurring, licensed, monthly price with
transform_quantity divide_by 10, at our exact figure, on our product, carrying
our addon key passed all five conditions. Modules bill BY quantity — a pack is
one item at quantity n — so a customer holding three would have been charged
one. A tiered price was excluded only by accident: Stripe reports unit_amount
null for it and we read that as 0, which fails the amount match unless the
caller's own figure is 0, and PlanPrices::ensure() has no zero guard.
activePricesFor() now also skips a non-empty transform_quantity and any
billing_scheme other than per_unit. Absent keys stay Stripe's defaults, which
is the direction that matters: read the other way round the filter would refuse
every legitimate price and turn recognition into a permanent no-op. The
contract docblock names the four properties instead of claiming to cover all of
them, and says the list is not everything Stripe can put on a price — only
what is known to change what one charges.
And a comment that says a gap is covered is worse than the gap. The sync said a
crash between Stripe creating a product and us storing its id gives back the
same product next run: true for twenty-four hours, false afterwards, and the
exact claim this branch was written to refute. There is no recognition step for
products at all, so an interrupted run leaves an orphan and the next run past
the expiry mints a second one — worse than a duplicate price, because
activePricesFor() then looks at the wrong product and recognition goes blind
for the whole family. Written down as a known gap at both createProduct call
sites, with the trade this branch chose: folding name and metadata into the key
means a renamed family now mints a second product where it used to answer 400,
and a blockade reaches a paying customer while a duplicate product does not.
The sweep no longer reports "created" for what it adopted — the count is taken
before ensure() runs, and telling those two apart is the whole point.
The test named after 2026-07-29 could not reproduce it: the fake's key ledger
was empty, so createPrice() could not have thrown and the test could not tell
"adoption prevented the 400" from "no 400 was possible". Seeded now with the
key today's call sends and a fingerprint over the metadata the pre-9da1358 call
sent. Remove the adopt() call from AddonPrices::ensure() and the test dies with
Stripe's own sentence instead of an assertion.
Also: the plan document still instructed migrate:fresh --env=testing. There is
no .env.testing here, so it targets the live development database; struck
through with the reason, not deleted, and a Korrekturen section records the
five defects the plan's own reviews found in it. Four comments corrected —
"below" that meant above, a counter claimed unable to tie that ties with a
planted price, the unique index's second meaning for remember()'s catch, and
that MariaDB DDL is not transactional so the dedupe commits before the index.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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 <noreply@anthropic.com>
2026_07_31_210000_one_row_per_stripe_price deletes rows from a live table —
the one place in this branch where a mistake loses data, not merely a place
where "read the migration" is enough. Nothing exercised that branch: every
other test in this file starts from an empty, already-migrated table, so the
delete never had anything to find.
RefreshDatabase has already run the migration by the time any test starts, so
its own unique index refuses the very duplicate this needs to insert. The new
test drops the index first, puts two rows on one Price back the way a
pre-migration table actually looked, and re-runs up() (require, not
require_once, the way the other migration-replay tests in this suite already
do) to reach the branch directly.
It asserts the row with the LOWER id survives, not merely that a row survives:
that ordering is a decision made on the operator's behalf, and it is the row
anything already billing is likeliest to have been reading. Checked by
temporarily inverting min('id') to max('id') during this work — the test
failed exactly there, then passed again once reverted.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The plan register has had this since 2026_07_30_110000. The module register
never did, so two rows could claim one Price and archiving the one would have
withdrawn the Price the other was still selling — Block D of the real-run
handoff.
It is also the net under the adoption step: at a VAT rate of nought both
treatments charge the same amount, and the check that refuses an already-claimed
Price is then the only thing keeping the two apart.
Duplicates are deleted, not archived. An archived row goes on claiming the id
and a unique index knows nothing about archived_at; the row rebuilds itself on
the next ensure(), and subscription_addons holds the Stripe id as text rather
than a foreign key, so no booking loses its price.
AddonPrices::remember()'s catch comment described the guard as the only thing
standing between two rows and one Price; now that the index exists, rewritten
to describe it as the net under that guard instead.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two things the plan's own text asked back. First, the idempotency-key
comment lost its substance when Task 5 rewrote it: the charged amount and
the treatment are in the key because at a rate of nought the two Prices
are the same amount and one key would have Stripe hand back a single
object for both — a maintainer who simplified that key would only
rediscover why from a unique-constraint violation. Restored alongside the
newer sentence about what actually stops a second Price (adoption, not
this key), so the module and plan sides read alike again. Spelled out
"twenty-four hours" in both places in the same comment, matching the
sibling.
Second, nothing pinned identifying: ['plan_price_id'] — the one decision
that makes the plan side different from the module side, because a
family's Product carries a Price for every version, term and treatment,
so plan_family alone cannot say which row a Price belongs to. Added a
test that plants a Price proving only the family, confirmed it fails
when identifying is loosened to ['plan_family'], and reverted.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both new tests passed whether adoption ran or not: Stripe forgets an
idempotency key after twenty-four hours, and that expiry is the only
condition under which the 2026-07-29 duplicate was ever minted. With the
sync's own key still in the fake's ledger, createPrice() replayed the
right answer on its own, so testing the recognition step with the key
still in force tested nothing.
Clearing $this->stripe->keys before the second call is what lets
createPrice() actually mint, which is what gives both tests something to
fail on. The second test needed one more thing: this row's own domestic
Price is a legitimate orphan the moment its register row is gone, so it
had to be archived first — otherwise adoption would rightly reclaim it
before ever weighing the wrong-owner Price, and the refusal this test
exists to prove would never be reached.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Same defect, same fix, one difference: a family Product carries every version
and every term, so the amount alone does not say which row a Price belongs to.
plan_price_id in the metadata does, and it is what has to agree before a Price
is taken over.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The comment from the last commit had it backwards. It called the two
original assertions "not the guard" and claimed the three DB-state
assertions fail when adoption reaches the wrong figure — both false.
AdoptStripePrice performs no database writes at all, so nothing it does can
fail an assertion that reads the 2900-net row's own table state, singly or
in combination. The `archived` assertion — the only one that reads what
Stripe was actually told — is the one adoption can fail, by archiving the
frozen booking's own Price as a false duplicate of the new figure's orphan.
Left as a trusted comment, this would have read as an invitation to delete
the real guard as redundant scaffolding around the "real" three. Reworded to
say what the DB-state assertions actually defend instead — archiveSuperseded()
losing its net_cents scoping, or remember() rewritten as an updateOrCreate()
keyed without amount_cents — and to name where each of adoption's two guards
(the amount match, the claimed callback) is tested in isolation elsewhere in
this file, since neither is isolated here.
Also closed the `archived_at` assertion: it read null for a row that is
still live and for one that is gone or rewritten onto the wrong net_cents
alike. An `exists()` check now stands beside it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two Important findings from review, both against the plan's own text.
The frozen-booking test could not fail for a defect this diff introduces.
Digging into why turned up two problems, not one. The two assertions the
review named were never exercised: sync-catalogue never writes
subscription_addons.stripe_price_id, and the orphan planted for the new
figure sits at a different amount than the booking's own price, so
archiveSuperseded's net_cents scoping was never going to reach it either.
Underneath that, the test's own orphan was inert regardless of any of that:
with no StripeAddonPrice row seeded first, AddonPrices::product() minted its
own Stripe Product instead of reusing 'prod_support', so the competing
orphan planted under that literal id sat on a product nothing ever asked
about. Fixed both — seeded the product the way the first two tests in this
file already do, added the three assertions the review specified, and a
comment on what the two original assertions do and do not prove.
Verified the fix actually bites, since a test whose teeth are asserted but
never demonstrated is what produced this finding in the first place. With
just the amount check in AdoptStripePrice disabled, the booking still
survived: the `claimed` callback this diff wires into AddonPrices::ensure()
caught it as a second, independent guard. Only disabling both together
reproduced the failure — the honest demonstration, not the single-line
sabotage first suggested, which this diff's own defense-in-depth absorbs.
The remember() comment credited a unique index stripe_addon_prices does not
have — that index sits on stripe_plan_prices, the plan side. Reworded to
name what actually keeps two rows off one Price id today (the `claimed`
callback), what 2026_07_31_210000_one_row_per_stripe_price adds as a net
under it, and why two rows sharing one Price is dangerous: archiving one —
the ordinary response to a superseded figure — would withdraw the very
Price the other row is still selling.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The sweep that died on 2026-07-29 left price_1TygdEC7u8NpJ8pOt3nsoyYw at Stripe
and no row here. This is the step that finds it: same money, same interval, our
metadata on it, unclaimed — taken over and brought up to date instead of
duplicated.
The comment on the key was the root of it. It read "keyed on what the Price IS",
and a key is not that: identity is product, amount, currency and interval, and
recognising those is AdoptStripePrice's job. Stripe forgets a key after a day;
it never was the guard against a duplicate.
A booking stays where it was sold. Adoption only ever matches an identical
amount, so a frozen booking cannot be moved by it — there is a test that says so.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
activePricesFor() reported interval alone, never interval_count or usage_type.
A Price we could not have created ourselves — billed every three months, or on
metered usage — could carry our own metadata (Stripe's dashboard "duplicate
price" copies it) and pass AdoptStripePrice's amount/currency/interval check on
'month' alone. Adopting it would move money onto a different recurrence, which
is exactly what nothing here may do.
Filtered at the boundary instead of in the predicate: HttpStripeClient now
skips anything besides Stripe's own defaults (interval_count 1, usage_type
licensed), so activePricesFor() can promise callers it returns only prices we
could have minted. FakeStripeClient needed no filter — it only ever holds
prices its own createPrice()/plantPrice() made, all standard — but says so, so
the omission doesn't read as forgotten.
Two more silent-failure paths, closed with the same three lines each:
- The metadata-differs check was a bare !==, which is key-order sensitive and
blind to Stripe's own merge. A Price carrying an extra key of its own, or
metadata handed back in a different order, could never compare equal and
would be rewritten every sweep forever. Compared through
array_intersect_key() plus ksort() on both sides instead, so only what WE
would send is what gets compared.
- Metadata is normalised to strings once, up front, so an un-cast value from a
caller can't make confirms() fail forever and silently turn adoption off for
that Price.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An abandoned run leaves an orphan: Stripe made the Price, our insert never
happened, and the table that decides everything afterwards does not know it
exists. The key covers a day; after that the next run makes a second live Price
for the same money.
What may be taken over is narrow. Same amount, currency and interval — a Price
at another figure would move money, and nothing here may: a running contract
keeps the Price it was sold on, and a booking stays frozen at what it cost that
day. Plus proof in the metadata, because an unexplained active Price at the
right money is what somebody clicking through Stripe's dashboard leaves behind,
and adopting that is worse than minting a second one.
Several orphans: the oldest is adopted — likeliest to be the one a lost row was
billing on — and the rest are archived, which stops them being sold and moves
nobody.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
nexxo