A seat held by another payment is visible to the cart adjudicating against it - #325
Closed
perigrin wants to merge 1 commit into
Closed
A seat held by another payment is visible to the cart adjudicating against it#325perigrin wants to merge 1 commit into
perigrin wants to merge 1 commit into
Conversation
…ainst it Closes the first and largest gap in #319, which turned out to be a money defect rather than only a coverage one. cart_seat_state scoped its lookup by payment_id, so a live seat held by a DIFFERENT payment -- a free enrolment with payment_id IS NULL, an admin add, an earlier purchase -- read as 'none'. The caller then adjudicated as though the child were unseated and tried to insert, colliding with the uniqueness rule inside a settlement Stripe had already captured. Same failure as #315, reached through a different door: the transaction rolls back, the die releases the webhook dedup claim, and every redelivery reproduces it. There is now a fourth state, 'foreign', and it is deliberately not 'seated'. payment_fits_session already counts foreign rows in $taken, so crediting one to this cart's %granted would count the same seat twice and under-count the capacity left for the next sibling in the cart. The test pins exactly that: two children, one already seated by another payment, and the sibling who genuinely needs a seat still gets one. A cart that paid for a seat the child already holds is owed its share back. There is nothing to seat and nothing of ours to demote -- the row that exists belongs to whoever paid for it first -- but the parent paid. It goes through the same refund_share_for path as a lost seat, with the same manual-review fallback when the share cannot be computed. Why nothing caught this: it is the conjunction trap #319 describes. Three mechanisms shape "one seat per child per session" -- this filter, payment_fits_session's payment exclusion, and the uniqueness rule -- and every fixture in the suite creates its enrollment rows FROM the cart under test, so all three agree and the tests grade only their aggregate. Removing the filter left four files green. Mutation-verified three ways: dropping the payment_id filter, reporting a foreign row as 'seated', and taking the foreign branch without owing the refund all fail now. Files=281 Tests=2415 PASS. Note on sequencing: this branch is off origin/main, so #324's partial-index migration is not in it. The collision the test reproduces is against the total constraint; after #324 it is against the live-only index. Same collision for an active foreign row, so the fix holds either way, but the two want a rebase check before both land. Claude-Session: https://claude.ai/code/session_01UMLwCP8cMNQ2kc4LnfeVc2
Member
Author
|
Folded into #324 (merge Shipping these separately would have shipped two partial fixes. The review of #324 found that the partial index closes only the All commits and tests from this branch are in #324. Nothing is dropped. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the first and largest gap in #319 — which turned out to be a money
defect, not only a coverage one.
The bug
cart_seat_statescoped its lookup bypayment_id, so a live seat held by adifferent payment read as
'none':payment_id IS NULLThe caller then adjudicated as though the child were unseated and tried to
insert — colliding with the uniqueness rule inside a settlement Stripe had
already captured. The transaction rolls back, the
diereleases the webhookdedup claim by design, and every redelivery reproduces it.
Same failure as #315, reached through a different door.
The fix
A fourth state,
foreign, and it is deliberately notseated.payment_fits_sessionalready counts foreign rows in$taken, so crediting oneto this cart's
%grantedwould count the same seat twice and under-countthe capacity left for the next sibling. The second subtest pins exactly that:
two children, one already seated by another payment, and the sibling who
genuinely needs a seat still gets one.
A cart that paid for a seat the child already holds is owed its share back.
There is nothing to seat and nothing of ours to demote — the row that exists
belongs to whoever paid for it first — but the parent paid. It goes through the
same
refund_share_forpath as a lost seat, with the same manual-reviewfallback when the share cannot be computed.
Why nothing caught it
The conjunction trap #319 describes. Three mechanisms shape "one seat per child
per session" — this filter,
payment_fits_session's payment exclusion, and theuniqueness rule — and every fixture in the suite creates its enrollment rows
from the cart under test, so all three agree and the tests grade only their
aggregate. Removing the filter left four files green.
Test evidence
Full suite:
Files=281, Tests=2415, Result: PASS. Mutation-verified three ways— all fail now:
payment_idfilterseatedt/stripe-live/andt/playwright/were not run.Sequencing
This branch is off
main, so #324's partial-index migration is not in it.The collision the test reproduces is against the total constraint; after #324 it
is against the live-only index. Same collision for an active foreign row, so
the fix holds either way — but the two want a rebase check before both land.
https://claude.ai/code/session_01UMLwCP8cMNQ2kc4LnfeVc2