Harden money-safety re-review findings: nonce-independent idempotency keys, reconciliation-required logging, terminal-state classification

Re-review (2 Oracle + security + QA + librarian + context-miner) surfaced fixes, all applied: (1) UserPaymentModal and the account-page gift-card buy now key the cached idempotency key on a stable 'new-card' sentinel instead of the cnon: nonce, so clearing the nonce on a failed charge no longer regenerates the key — a lost-response retry now dedups at Square instead of double-charging (the tip flows already keyed on amount only). (2) The create-with-redeem clawback now logs CRITICAL when the guarded balance reversal is blocked (previously silent), and its transaction DELETE is scoped to this sale instead of deleting every transaction on the card. (3) reconcileStalePaymentAtSquare now treats APPROVED/PENDING as non-terminal (leave pending) instead of definitively failed, matching Square's documented state machine. (4) GetTillCheckoutStatus returns 404 for a sale already swept to failed instead of reporting a live state. (5) The critical-payment scan job skips candidates whose booking was hard-deleted, so one orphan can no longer silence all critical alerts.

backend/handlers/payments/sweep.go

@@ -259,11 +259,28 @@ func reconcileStalePaymentAtSquare(ctx context.Context, table, squarePaymentID s
 		log.Printf("Stale pending %s reconcile for Square payment %s hit an ambiguous error (%v) — leaving pending for a later sweep run", table, squarePaymentID, err)
 		return staleReconcileLeavePending
 	}
-	if pr.Status != "COMPLETED" {
+	// Square's terminal payment states are COMPLETED, CANCELED, FAILED;
+	// APPROVED (authorization-only, delayed capture) and PENDING are
+	// NON-terminal — both can still transition to COMPLETED (via
+	// CompletePayment or delay_action=COMPLETE), so clawing back the funded
+	// gift card on either would risk reversing a charge that later lands.
+	// This app always creates payments with autocomplete (default true), so
+	// it never produces APPROVED/PENDING rows today, but the classification
+	// must match Square's documented state machine (librarian-verified
+	// 2026-05-20).
+	switch pr.Status {
+	case "COMPLETED":
+		return staleReconcileCompleted
+	case "CANCELED", "FAILED":
+		log.Printf("Stale pending %s is %q at Square — marking failed", table, pr.Status)
+		return staleReconcileDefinitivelyFailed
+	case "APPROVED", "PENDING":
+		log.Printf("Stale pending %s is %q at Square (non-terminal) — leaving pending for a later sweep run", table, pr.Status)
+		return staleReconcileLeavePending
+	default:
 		log.Printf("Stale pending %s is %q at Square — marking failed", table, pr.Status)
 		return staleReconcileDefinitivelyFailed
 	}
-	return staleReconcileCompleted
 }
 
 // squarePaymentErrorIsNotFound reports whether a GetPayment error proves the

backend/handlers/payments/till.go

@@ -127,9 +127,12 @@ func revertGiftCardFunding(ctx context.Context, action, giftCardID string, amoun
 	}
 
 	if action == "create" {
-		// A newly created card has exactly one funding transaction (this
+		// A newly created card's transactions are scoped to THIS sale's
-		// request's purchase) — remove it, then the card itself.
+		// funding (reference_type='till_sale' AND reference_id=sale id) — never
-		if _, err := tx.Exec(ctx, `DELETE FROM gift_card_transactions WHERE gift_card_id = $1`, giftCardID); err != nil {
+		// a wholesale delete, which would destroy the value of a different
+		// idempotency-keyed top-up sale that funded the same card before this
+		// create resolved. Then remove the card itself.
+		if _, err := tx.Exec(ctx, `DELETE FROM gift_card_transactions WHERE gift_card_id = $1 AND reference_type = 'till_sale' AND reference_id = $2`, giftCardID, tillSaleID); err != nil {
 			return fmt.Errorf("failed to delete gift card transaction: %w", err)
 		}
 		if _, err := tx.Exec(ctx, `DELETE FROM gift_cards WHERE id = $1`, giftCardID); err != nil {
@@ -138,12 +141,21 @@ func revertGiftCardFunding(ctx context.Context, action, giftCardID string, amoun
 		// If the card was immediately redeemed to a user balance in this
 		// request, reverse that credit (guarded so it can never go negative).
 		if redeemToUserID != nil && *redeemToUserID != "" {
-			if _, err := tx.Exec(ctx, `
+			bTag, bErr := tx.Exec(ctx, `
 				UPDATE user_giftcard_balances
 				SET balance = user_giftcard_balances.balance - $1, updated_at = NOW()
 				WHERE user_id = $2 AND balance >= $1
-			`, amount, *redeemToUserID); err != nil {
+			`, amount, *redeemToUserID)
-				return fmt.Errorf("failed to reverse redeemed gift card balance: %w", err)
+			if bErr != nil {
+				return fmt.Errorf("failed to reverse redeemed gift card balance: %w", bErr)
+			}
+			if bTag.RowsAffected() == 0 {
+				// The guard blocked the reversal because some of the credited
+				// balance was already spent. The sale is still marked failed
+				// below — do not fail the whole clawback tx — but the
+				// un-reversed credit must be flagged for manual reconciliation
+				// (mirrors the top-up branch).
+				log.Printf("CRITICAL: ... MANUAL RECONCILIATION REQUIRED: create-with-redeem clawback for gift card %s could not fully reverse the £%.2f balance credited to user %s (balance < amount)", giftCardID, amount, *redeemToUserID)
 			}
 		}
 	} else {
@@ -945,6 +957,18 @@ func GetTillCheckoutStatus(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	// A sale already swept to 'failed' (e.g. its checkout was cancelled as
+	// stale, or a definitive decline clawed back the card) must never report a
+	// live payment state: the poll would otherwise tell the admin the terminal
+	// is still waiting when the sale can no longer be completed. Fail loudly so
+	// a stale checkout that later resolves at Square is surfaced for manual
+	// reconciliation instead of silently confusing the operator.
+	if currentStatus == "failed" {
+		log.Printf("Till sale %s is already failed but checkout %s is still being polled — refusing to report a live state", tillSaleID, checkoutID)
+		http.Error(w, "Till sale already failed — manual reconciliation required", http.StatusNotFound)
+		return
+	}
+
 	paymentResult, err := SquareClient.GetCheckout(r.Context(), checkoutID)
 	if err != nil {
 		if errors.Is(err, square.ErrCheckoutPending) {

backend/internal/jobs/cleanup.go

@@ -329,6 +329,14 @@ func ScanCriticalPaymentLogs(ctx context.Context) (int, error) {
 			  AND an.booking_id IS NOT DISTINCT FROM src.booking_id
 			  AND an.acknowledged_at IS NULL
 		)
+		-- admin_notifications.booking_id is a hard FK to bookings(id); a
+		-- candidate whose booking was hard-deleted (7-year retention) would
+		-- otherwise fail the whole INSERT and silently kill ALL critical
+		-- alerts. Skip orphans: keep NULL booking_ids (till sales) and only
+		-- payments/refunds whose booking still exists.
+		AND (src.booking_id IS NULL OR EXISTS (
+			SELECT 1 FROM bookings b WHERE b.id = src.booking_id
+		))
 	`)
 	if err != nil {
 		return 0, fmt.Errorf("failed to scan critical payment logs: %w", err)

frontend/src/lib/components/payments/UserPaymentModal.svelte

@@ -404,8 +404,13 @@
 		}
 
 		// Cache the idempotency key per amount+type+card so a lost-response
-		// retry reuses it (backend dedups) instead of double-charging.
+		// retry reuses it (backend dedups) instead of double-charging. The
-		const cardKey = cardId ?? `new:${newCardToken ?? ''}`;
+		// new-card identity is a STABLE sentinel, NOT the cnon: nonce: the
+		// nonce is one-shot and cleared on a failed charge, so keying on it
+		// would regenerate the key on retry and a network-timeout retry (where
+		// the charge actually landed) would double-charge. The nonce changes
+		// per tokenize but represents the same logical card intent.
+		const cardKey = cardId ?? 'new-card';
 		if (
 			!payIdempotencyKey ||
 			payKeyedAmount !== amountCents ||

frontend/src/routes/account/+page.svelte

@@ -317,7 +317,13 @@
 			// Cache the idempotency key per amount+card so a lost-response retry
 			// reuses the same key (backend dedups) instead of double-charging.
 			// Regenerate when the amount or card changes.
-			const cardKey = cardId || `new:${newCardToken ?? ''}`;
+			// Cache the idempotency key per amount+card so a lost-response
+			// retry reuses it (backend dedups) instead of double-charging. The
+			// new-card identity is a STABLE sentinel, NOT the cnon: nonce: the
+			// nonce is one-shot and cleared on a failed charge, so keying on it
+			// would regenerate the key on retry and a network-timeout retry
+			// (where the charge actually landed) would double-charge.
+			const cardKey = cardId || 'new-card';
 			if (!buyIdempotencyKey || buyKeyedAmount !== buyAmount || buyKeyedCard !== cardKey) {
 				buyIdempotencyKey = generateIdempotencyKey();
 				buyKeyedAmount = buyAmount;