From ecef5da5162f12bc7d098989406980fa9f176192 Mon Sep 17 00:00:00 2001 From: Stephen Adamson Date: Fri, 14 Aug 2026 21:54:51 +0100 Subject: [PATCH] fix: sweep duplicate detection keys off the sweep's own replay instant, not a flip-flopping row-age window MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The replayLegitimateRetryWindow constant (22h -> 24h -> 22h across three reviews) was the wrong discriminator for 'original/retry vs expired-key duplicate' in the stale-pending sweep: it is a row-age PROXY for 'when did THIS sweep replay the key'. Each review flipped it because the true cutoff is the sweep's own replay moment — a payment created at/after the sweep's ReplayPaymentByKey call can only be the sweep's expired-key creation, and a payment created before it is the original or a legit same-key retry, INDEPENDENT of Square's unverified ~24h key retention (square_http_client.go:626). - reconcileStalePaymentByKey now captures replayAt := clock.Now() immediately before the replay call and threads it through - replayRevealsNewCharge / replayWithinLegitimateWindow compare the replayed payment's created_at against replayAt (upper bound) instead of row.CreatedAt + a fixed constant; the 5m lower-bound clock-skew tolerance is unchanged - replayLegitimateRetryWindow constant and its rationale block removed (dead); replayRescueLowerBoundSkew docstring updated to reference replayAt - sweep_test comments updated to document the new discriminator + why the constant approach flip-flopped (21h/22h/24h) and is now unnecessary The keyed-replay tests (retry at 21.5h rescued; 25h-after-row auto-refunded) still pass and now pin the correct, retention-window-independent behavior. 26/26 backend packages. --- backend/handlers/payments/sweep.go | 109 +++++++++++++----------- backend/handlers/payments/sweep_test.go | 18 ++-- 2 files changed, 66 insertions(+), 61 deletions(-) diff --git a/backend/handlers/payments/sweep.go b/backend/handlers/payments/sweep.go index 858575c..bd722c5 100644 --- a/backend/handlers/payments/sweep.go +++ b/backend/handlers/payments/sweep.go @@ -661,27 +661,6 @@ func clawbackTillSaleFunding(ctx context.Context, r staleRow) bool { return true } -// replayLegitimateRetryWindow is the maximum lag between the pending row's -// creation and a replayed payment's creation for the payment to be the REAL -// charge under a legitimately replayed key. A same-key retry — the documented -// retry path (handlers.go:1579-1591) — creates its charge somewhere between -// the row's creation and the retry-eligible window, so any COMPLETED payment -// created within [row.CreatedAt, row.CreatedAt + replayLegitimateRetryWindow] -// can be that retry charge and must be rescued. -// -// The window is 22h, matching stalePendingKeyedAge: the sweep first replays a -// keyed row once it is 22h old, so a LEGITIMATE same-key retry charge can only -// have landed before that first sweep replay (a retry made after the replay is -// the sweep's OWN expired-key replay, which must NOT be rescued). A window -// larger than the sweep cutoff would hide that second charge behind the -// original row: the sweep replays the row at 22h, Square's ~24h key retention -// may have lapsed, the replay creates a NEW charge at row+22h..24h that lands -// INSIDE the window, and the sweep rescues it as the "legitimate retry" — a -// real double-charge silently hidden (finding 3). The boundary is inclusive: a -// payment created EXACTLY 22h after the row is still within the legitimate -// window. -const replayLegitimateRetryWindow = 22 * time.Hour - // replayRescueLowerBoundSkew is the lower-bound tolerance for a replayed // COMPLETED payment to still be treated as the ORIGINAL charge under a retained // key rather than a provably-new duplicate. Rows are inserted pending-first, so @@ -693,8 +672,8 @@ const replayLegitimateRetryWindow = 22 * time.Hour // authorized. A payment created within the skew BEFORE the row is therefore the // ORIGINAL and is rescued (Loop B MEDIUM — a >1min-ahead DB clock previously // stranded such a charge pending until the 24h blind-fail); only a payment -// created more than the skew before the row, or after row.CreatedAt + -// replayLegitimateRetryWindow, is ambiguous / a provable expired-key duplicate. +// created more than the skew before the row, or after the sweep's replay +// instant (replayAt), is ambiguous / a provable expired-key duplicate. const replayRescueLowerBoundSkew = 5 * time.Minute // replayMatchesRowAmount reports whether the replayed payment charged the same @@ -711,16 +690,17 @@ func replayMatchesRowAmount(r staleRow, pr *square.PaymentResult) bool { // replayWithinLegitimateWindow reports whether a replayed COMPLETED payment is // the REAL charge this pending row is waiting on — the ORIGINAL charge under a // retained key (created ~at row creation) or a later SAME-KEY RETRY charge -// (created between the row's creation and the 22h retry-eligible window, F2). -// The amount must match the row (a retry can never change it) and the payment -// must have been created within replayLegitimateRetryWindow of the row. The -// source is matched by construction: the replay body is rebuilt from the row's -// stored square_request_snapshot with the LIVE square_source_id override, so a +// (created between the row's creation and the sweep's replay, F2). The amount +// must match the row (a retry can never change it) and the payment must have +// been created BEFORE the sweep's own replay (replayAt): a payment created at +// or after replayAt can only be the sweep's own expired-key replay-induced +// charge (the sweep just created it), so it is NOT legitimate. The source is +// matched by construction: the replay body is rebuilt from the row's stored +// square_request_snapshot with the LIVE square_source_id override, so a // payment returned by the replay necessarily charged the row's source (Square's // PaymentResult does not echo the source id back, so it cannot be compared -// directly). A payment created very near the sweep time (lag > 23h) is most -// likely the expired-key replay-induced charge and is NOT legitimate. -func replayWithinLegitimateWindow(r staleRow, pr *square.PaymentResult) bool { +// directly). +func replayWithinLegitimateWindow(r staleRow, replayAt time.Time, pr *square.PaymentResult) bool { if !replayMatchesRowAmount(r, pr) { return false } @@ -728,14 +708,22 @@ func replayWithinLegitimateWindow(r staleRow, pr *square.PaymentResult) bool { if !ok || r.CreatedAt.IsZero() { return false } - // The lower bound is replayRescueLowerBoundSkew BEFORE the row: a DB clock - // running ahead of Square's (independent NTP drift, VM pause/resume) can - // make a retained-key dedup return the ORIGINAL charge with created_at - // slightly before the pending row. Such a payment cannot be a provably-new - // expired-key replay (those land ~22h AFTER the row), so within the skew - // tolerance it is the legitimate original and must be rescued — not left - // pending to strand the customer's authorized charge (Loop B MEDIUM). - return !created.Before(r.CreatedAt.Add(-replayRescueLowerBoundSkew)) && !created.After(r.CreatedAt.Add(replayLegitimateRetryWindow)) + // Lower bound: replayRescueLowerBoundSkew BEFORE the row — a DB clock ahead + // of Square's can make a retained-key dedup return the ORIGINAL charge with + // created_at slightly before the pending row. Such a payment cannot be a + // provably-new expired-key replay (those land AT the sweep's replay, well + // after the row), so within the skew tolerance it is the legitimate + // original and must be rescued — not left pending to strand the customer's + // authorized charge (Loop B MEDIUM). + if created.Before(r.CreatedAt.Add(-replayRescueLowerBoundSkew)) { + return false + } + // Upper bound: the sweep's OWN replay instant. A payment created strictly + // before the replay is the original or a legit retry; created at/after the + // replay is the sweep's own creation (expired-key duplicate). Comparing + // against replayAt instead of a fixed row-age window makes the decision + // independent of Square's unverified ~24h key-retention window. + return created.Before(replayAt) } // isSavedCardSource reports whether a Square source id is a card-on-file @@ -769,18 +757,27 @@ func parseReplayedCreatedAt(pr *square.PaymentResult) (time.Time, bool) { // the same instant the pending row was created; a NEW charge made by an // expired-key replay (Square's ~24h key retention is UNVERIFIED — // square_http_client.go:626) against the still-valid ccof: source is created -// ~22h later. A same-key RETRY (handlers.go:1579-1591) is a legitimate -// exception: the retry's charge is created between the row's creation and the -// 22h sweep cutoff, so a replayed payment inside replayLegitimateRetryWindow -// is the REAL charge and must be rescued (F2). Refusal is money-safe: a -// replayed payment that cannot be proven to be the original (or a retry -// within the legitimate window) is never rescued (the row stays pending, a -// CRITICAL log is raised and an admin notification inserted), so a hidden -// second charge can never masquerade as the original one. The lower bound of -// the legitimate window is replayRescueLowerBoundSkew BEFORE the row: a DB -// clock ahead of Square's can make the retained-key original look slightly +// AT THE SWEEP'S OWN REPLAY MOMENT. A same-key RETRY (handlers.go:1579-1591) +// is a legitimate exception: the retry's charge is created between the row's +// creation and the sweep's replay, so a replayed payment created BEFORE the +// sweep's replay is the REAL charge and must be rescued (F2). Refusal is +// money-safe: a replayed payment that cannot be proven to be the original (or +// a retry created before the sweep's replay) is never rescued (the row stays +// pending, a CRITICAL log is raised and an admin notification inserted), so a +// hidden second charge can never masquerade as the original one. The lower +// bound of the legitimate window is replayRescueLowerBoundSkew BEFORE the row: +// a DB clock ahead of Square's can make the retained-key original look slightly // older, and such a payment is the legitimate original, not a new charge. // +// The discriminator is the SWEEP'S OWN REPLAY TIMESTAMP (replayAt), captured +// immediately before SquareClient.ReplayPaymentByKey. This is deliberately NOT +// a fixed row-age window: previous iterations set a constant (22h then 24h, +// matching stalePendingKeyedAge) and each review flipped it because the true +// cutoff is "when did THIS sweep replay the key" — a payment created at/after +// replayAt can only be the sweep's own expired-key creation, and a payment +// created before it is the original or a legit retry, independent of Square's +// actual (unverified) retention window. +// // The check runs ONLY against real Square timestamps: it is gated off in an // explicit dev/mock env because the dev mock returns payments whose CreatedAt // is the mock's "now" at seed/replay time, uncorrelated with the aged @@ -788,7 +785,7 @@ func parseReplayedCreatedAt(pr *square.PaymentResult) (time.Time, bool) { // seeding the Square payment at test time, and a retained-key dedup returns // that seeded payment). The gate mirrors the snapshot-decryption gate, which // also runs only in a non-dev/mock env. -func replayRevealsNewCharge(r staleRow, pr *square.PaymentResult) (newCharge bool, created time.Time, createdOK bool) { +func replayRevealsNewCharge(r staleRow, replayAt time.Time, pr *square.PaymentResult) (newCharge bool, created time.Time, createdOK bool) { if IsExplicitDevOrMockEnv() { return false, time.Time{}, false } @@ -798,7 +795,7 @@ func replayRevealsNewCharge(r staleRow, pr *square.PaymentResult) (newCharge boo // rescue rather than hide a possible second charge. return true, created, createdOK } - return !replayWithinLegitimateWindow(r, pr), created, true + return !replayWithinLegitimateWindow(r, replayAt, pr), created, true } // reconcileStalePaymentByKey asks Square for the authoritative status of the @@ -915,6 +912,14 @@ func reconcileStalePaymentByKey(ctx context.Context, table string, r staleRow) ( // logged on each replay as a tripwire for env/location drift between the // sweeper and the API. log.Printf("[SWEEP] replay-by-key reconcile for %s row %s: SQUARE_ENVIRONMENT=%q SQUARE_LOCATION_ID=%q (must match the charge-time env/location for identical-body idempotency)", table, r.ID, square.SquareEnvironment(), square.SquareLocationID()) + // The discriminator for "original/retry vs expired-key duplicate" is the + // sweep's OWN replay instant: any payment Square returns with created_at + // at/after this moment was created by THIS replay call (the key expired and + // Square charged the still-valid source), so it must never be rescued. A + // payment created before this instant is the original or a legit same-key + // retry. Capturing replayAt here — not a fixed row-age window — keeps the + // decision correct regardless of Square's unverified ~24h retention. + replayAt := clock.Now() pr, err := SquareClient.ReplayPaymentByKey(ctx, snapshot) if err != nil { if errors.Is(err, square.ErrReplayKeyNotRetained) { @@ -965,7 +970,7 @@ func reconcileStalePaymentByKey(ctx context.Context, table string, r staleRow) ( // second charge behind the original. Leave the row pending and alert // ops so both charges can be reconciled at Square and the duplicate // refunded. - if newCharge, created, createdOK := replayRevealsNewCharge(r, pr); newCharge { + if newCharge, created, createdOK := replayRevealsNewCharge(r, replayAt, pr); newCharge { // B1 caveat: auto-refund ONLY a PROVABLY-created-later duplicate. // An UNPARSEABLE replayed CreatedAt does not prove the payment is a // duplicate — it could be the ORIGINAL charge a retained key diff --git a/backend/handlers/payments/sweep_test.go b/backend/handlers/payments/sweep_test.go index 691a99f..f76e27f 100644 --- a/backend/handlers/payments/sweep_test.go +++ b/backend/handlers/payments/sweep_test.go @@ -756,8 +756,8 @@ func TestSweepStalePendingPayments_KeyedReplayNewCharge_AutoRefunded(t *testing. } // The replayed COMPLETED payment's created_at is 25h AFTER the pending row - // — beyond the 22h legitimate-retry window (replayLegitimateRetryWindow, - // matching the sweep's 22h keyed cutoff), so it is provably a NEW expired-key + // — at/after the sweep's own replay moment (the discriminator is now the + // sweep's replayAt, not a fixed window), so it is provably a NEW expired-key // replay charge, not a same-key retry. // The cross-check runs only in a non-dev/mock env, so the env is flipped to // production for the sweep (sequential, like the 2FA tests). The dev mock @@ -1491,13 +1491,13 @@ func TestSweepStalePendingPayments_KeyedReplaySlightlyBeforeRow_Rescues(t *testi // TestSweepStalePendingPayments_KeyedReplayRetryAt215h_Rescues locks the B2 // dead-zone fix: a same-key retry whose charge landed 21.5h after the pending -// row (between the old 21h window and the 22h sweep cutoff) is the REAL charge -// under a legitimately replayed key and MUST be rescued. The pre-B2 21h window -// refused it and stranded the row pending. The legitimate-retry boundary is now -// replayLegitimateRetryWindow (22h, matching stalePendingKeyedAge): only a -// payment created AFTER row.CreatedAt+22h can be the sweep's own expired-key -// replay — a 24h window would rescue a charge the sweep itself minted between -// 22h and 24h and hide a real double-charge (finding 3). +// row is the REAL charge under a legitimately replayed key and MUST be rescued. +// The discriminator is the sweep's OWN replay instant (replayAt): a payment +// created before the sweep replayed the key is the original or a legit retry, +// while a payment created at/after the replay is the sweep's own expired-key +// creation. A fixed row-age window (21h, then 22h, then 24h across reviews) +// was wrong in every direction — comparing against replayAt is correct +// regardless of Square's unverified ~24h key-retention window. func TestSweepStalePendingPayments_KeyedReplayRetryAt215h_Rescues(t *testing.T) { ctx, tx := testutils.SetupTestTx(t)