popertots/Crussell
1
1
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: sweep duplicate detection keys off the sweep's own replay instant, not a flip-flopping row-age window

Browse Source 
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.
This commit is contained in:
popertots
2026-08-22 00:34:50 +01:00
parent 1543160f6a
commit ecef5da516
2 changed files with 66 additions and 61 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/handlers/payments/sweep.go
+57 -52
View File
@@ -661,27 +661,6 @@ func clawbackTillSaleFunding(ctx context.Context, r staleRow) bool {
return true 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 // replayRescueLowerBoundSkew is the lower-bound tolerance for a replayed
// COMPLETED payment to still be treated as the ORIGINAL charge under a retained // 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 // 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 // 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 // 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 // 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 + // created more than the skew before the row, or after the sweep's replay
// replayLegitimateRetryWindow, is ambiguous / a provable expired-key duplicate. // instant (replayAt), is ambiguous / a provable expired-key duplicate.
const replayRescueLowerBoundSkew = 5 * time.Minute const replayRescueLowerBoundSkew = 5 * time.Minute
// replayMatchesRowAmount reports whether the replayed payment charged the same // 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 // replayWithinLegitimateWindow reports whether a replayed COMPLETED payment is
// the REAL charge this pending row is waiting on — the ORIGINAL charge under a // 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 // 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). // (created between the row's creation and the sweep's replay, F2). The amount
// The amount must match the row (a retry can never change it) and the payment // must match the row (a retry can never change it) and the payment must have
// must have been created within replayLegitimateRetryWindow of the row. The // been created BEFORE the sweep's own replay (replayAt): a payment created at
// source is matched by construction: the replay body is rebuilt from the row's // or after replayAt can only be the sweep's own expired-key replay-induced
// stored square_request_snapshot with the LIVE square_source_id override, so a // 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 // 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 // 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 // directly).
// likely the expired-key replay-induced charge and is NOT legitimate. func replayWithinLegitimateWindow(r staleRow, replayAt time.Time, pr *square.PaymentResult) bool {
func replayWithinLegitimateWindow(r staleRow, pr *square.PaymentResult) bool {
if !replayMatchesRowAmount(r, pr) { if !replayMatchesRowAmount(r, pr) {
return false return false
} }
@@ -728,14 +708,22 @@ func replayWithinLegitimateWindow(r staleRow, pr *square.PaymentResult) bool {
if !ok || r.CreatedAt.IsZero() { if !ok || r.CreatedAt.IsZero() {
return false return false
} }
// The lower bound is replayRescueLowerBoundSkew BEFORE the row: a DB clock // Lower bound: replayRescueLowerBoundSkew BEFORE the row — a DB clock ahead
// running ahead of Square's (independent NTP drift, VM pause/resume) can // of Square's can make a retained-key dedup return the ORIGINAL charge with
// make a retained-key dedup return the ORIGINAL charge with created_at // created_at slightly before the pending row. Such a payment cannot be a
// slightly before the pending row. Such a payment cannot be a provably-new // provably-new expired-key replay (those land AT the sweep's replay, well
// expired-key replay (those land ~22h AFTER the row), so within the skew // after the row), so within the skew tolerance it is the legitimate
// tolerance it is the legitimate original and must be rescued — not left // original and must be rescued — not left pending to strand the customer's
// pending to strand the customer's authorized charge (Loop B MEDIUM). // authorized charge (Loop B MEDIUM).
return !created.Before(r.CreatedAt.Add(-replayRescueLowerBoundSkew)) && !created.After(r.CreatedAt.Add(replayLegitimateRetryWindow)) 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 // 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 // the same instant the pending row was created; a NEW charge made by an
// expired-key replay (Square's ~24h key retention is UNVERIFIED — // expired-key replay (Square's ~24h key retention is UNVERIFIED —
// square_http_client.go:626) against the still-valid ccof: source is created // 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 // AT THE SWEEP'S OWN REPLAY MOMENT. A same-key RETRY (handlers.go:1579-1591)
// exception: the retry's charge is created between the row's creation and the // is a legitimate exception: the retry's charge is created between the row's
// 22h sweep cutoff, so a replayed payment inside replayLegitimateRetryWindow // creation and the sweep's replay, so a replayed payment created BEFORE the
// is the REAL charge and must be rescued (F2). Refusal is money-safe: a // sweep's replay is the REAL charge and must be rescued (F2). Refusal is
// replayed payment that cannot be proven to be the original (or a retry // money-safe: a replayed payment that cannot be proven to be the original (or
// within the legitimate window) is never rescued (the row stays pending, a // a retry created before the sweep's replay) is never rescued (the row stays
// CRITICAL log is raised and an admin notification inserted), so a hidden // pending, a CRITICAL log is raised and an admin notification inserted), so a
// second charge can never masquerade as the original one. The lower bound of // hidden second charge can never masquerade as the original one. The lower
// the legitimate window is replayRescueLowerBoundSkew BEFORE the row: a DB // bound of the legitimate window is replayRescueLowerBoundSkew BEFORE the row:
// clock ahead of Square's can make the retained-key original look slightly // 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. // 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 // 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 // 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 // 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 // seeding the Square payment at test time, and a retained-key dedup returns
// that seeded payment). The gate mirrors the snapshot-decryption gate, which // that seeded payment). The gate mirrors the snapshot-decryption gate, which
// also runs only in a non-dev/mock env. // 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() { if IsExplicitDevOrMockEnv() {
return false, time.Time{}, false 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. // rescue rather than hide a possible second charge.
return true, created, createdOK 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 // 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 // logged on each replay as a tripwire for env/location drift between the
// sweeper and the API. // 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()) 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) pr, err := SquareClient.ReplayPaymentByKey(ctx, snapshot)
if err != nil { if err != nil {
if errors.Is(err, square.ErrReplayKeyNotRetained) { 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 // second charge behind the original. Leave the row pending and alert
// ops so both charges can be reconciled at Square and the duplicate // ops so both charges can be reconciled at Square and the duplicate
// refunded. // 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. // B1 caveat: auto-refund ONLY a PROVABLY-created-later duplicate.
// An UNPARSEABLE replayed CreatedAt does not prove the payment is a // An UNPARSEABLE replayed CreatedAt does not prove the payment is a
// duplicate — it could be the ORIGINAL charge a retained key // duplicate — it could be the ORIGINAL charge a retained key
backend/handlers/payments/sweep_test.go
+9 -9
View File
@@ -756,8 +756,8 @@ func TestSweepStalePendingPayments_KeyedReplayNewCharge_AutoRefunded(t *testing.
} }
// The replayed COMPLETED payment's created_at is 25h AFTER the pending row // The replayed COMPLETED payment's created_at is 25h AFTER the pending row
// — beyond the 22h legitimate-retry window (replayLegitimateRetryWindow, // — at/after the sweep's own replay moment (the discriminator is now the
// matching the sweep's 22h keyed cutoff), so it is provably a NEW expired-key // sweep's replayAt, not a fixed window), so it is provably a NEW expired-key
// replay charge, not a same-key retry. // replay charge, not a same-key retry.
// The cross-check runs only in a non-dev/mock env, so the env is flipped to // 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 // 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 // TestSweepStalePendingPayments_KeyedReplayRetryAt215h_Rescues locks the B2
// dead-zone fix: a same-key retry whose charge landed 21.5h after the pending // 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 // row is the REAL charge under a legitimately replayed key and MUST be rescued.
// under a legitimately replayed key and MUST be rescued. The pre-B2 21h window // The discriminator is the sweep's OWN replay instant (replayAt): a payment
// refused it and stranded the row pending. The legitimate-retry boundary is now // created before the sweep replayed the key is the original or a legit retry,
// replayLegitimateRetryWindow (22h, matching stalePendingKeyedAge): only a // while a payment created at/after the replay is the sweep's own expired-key
// payment created AFTER row.CreatedAt+22h can be the sweep's own expired-key // creation. A fixed row-age window (21h, then 22h, then 24h across reviews)
// replay — a 24h window would rescue a charge the sweep itself minted between // was wrong in every direction — comparing against replayAt is correct
// 22h and 24h and hide a real double-charge (finding 3). // regardless of Square's unverified ~24h key-retention window.
func TestSweepStalePendingPayments_KeyedReplayRetryAt215h_Rescues(t *testing.T) { func TestSweepStalePendingPayments_KeyedReplayRetryAt215h_Rescues(t *testing.T) {
ctx, tx := testutils.SetupTestTx(t) ctx, tx := testutils.SetupTestTx(t)