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