Refactor payment charge paths into shared helpers; classify Square failures

Extracts resolveChargeSource (new-card vs saved-card vs one-off nonce, with Square customer provisioning) and shared advisory-lock + post-charge recheck helpers into charge_helpers.go. Adds errors.go with chargeFailureStatus: transport/5xx/context and 429/408/425 map to 503 (retryable), structured 4xx declines map to 402, used across all four charge paths. Also fixes the no-client-key refund fallback to append a crypto/rand suffix (distinct same-amount partial refunds no longer collide) and adds refundResumeKey so legacy NULL idempotency_key rows resume with a derived key instead of an empty one.

backend/handlers/payments/charge_helpers.go

@@ -0,0 +1,167 @@
+package payments
+
+import (
+	"context"
+	"errors"
+	"log"
+	"net/http"
+
+	"crussell/db"
+
+	"github.com/jackc/pgx/v5"
+	"github.com/jackc/pgx/v5/pgxpool"
+)
+
+// resolveChargeSource resolves the Square payment source for a card charge,
+// shared by CreateBookingPayment, CreateTipPayment, BuyGiftCard, and the
+// CreateTerminalPayment saved-card branch (which passes the booking user's id
+// and a nil new-card token).
+//
+// New-card path (cnon: nonce): the nonce is used DIRECTLY for one-off charges
+// (no card-on-file is created — the old tokenize-then-charge flow left orphan
+// cards at Square). When saveCard is true the user's Square customer is
+// provisioned FIRST and the card is tokenized against it (a ccof: source MUST
+// carry its customer — R6), then saved via SaveCardForUser.
+//
+// Saved-card path (ccof:): a saved-card row predating P14 has an empty
+// square_customer_id; the user's Square customer is lazily provisioned and
+// persisted on the row BEFORE charging (a ccof: source can never be charged
+// without a CustomerID).
+//
+// On any error the helper writes the HTTP response and returns ok=false — the
+// caller must return immediately.
+func resolveChargeSource(ctx context.Context, w http.ResponseWriter, svc *PaymentService, userID string, newCardToken, cardID *string, saveCard bool, notFoundMsg string) (sourceID string, savedCardID *string, squareCustomerID string, ok bool) {
+	if newCardToken != nil && *newCardToken != "" {
+		if saveCard {
+			sqCustomerID, custErr := svc.EnsureSquareCustomer(ctx, userID)
+			if custErr != nil {
+				log.Printf("Failed to provision Square customer for user %s: %v", userID, custErr)
+				http.Error(w, "Failed to process card", http.StatusInternalServerError)
+				return "", nil, "", false
+			}
+			cardOnFile, err := SquareClient.CreateCardOnFile(ctx, userID, *newCardToken, sqCustomerID)
+			if err != nil {
+				log.Printf("Failed to create card on file: %v", err)
+				http.Error(w, "Failed to process card", http.StatusInternalServerError)
+				return "", nil, "", false
+			}
+			sourceID = cardOnFile.CardID
+			squareCustomerID = sqCustomerID
+			// CreateCardOnFile runs before the charge. If the subsequent payment
+			// fails, this card-on-file is intentionally NOT deleted: the pending
+			// record's retry re-creates it via the deterministic sha256
+			// idempotency key (the SAVE path), and Square returns the same card —
+			// deleting it would break that retry.
+			cardID, err := svc.SaveCardForUser(ctx, userID, sqCustomerID, cardOnFile.CardID, cardOnFile.Brand, cardOnFile.Last4, cardOnFile.ExpMonth, cardOnFile.ExpYear, cardOnFile.Fingerprint)
+			if err != nil {
+				log.Printf("Failed to save card: %v", err)
+			} else {
+				savedCardID = &cardID
+			}
+		} else {
+			// One-off new-card charge: use the cnon: nonce DIRECTLY as the
+			// source. No card-on-file is created (nothing to orphan, no
+			// customer needed).
+			sourceID = *newCardToken
+		}
+		if savedCardID == nil && saveCard {
+			log.Printf("Card was not saved despite save_card=true for user %s", userID)
+		}
+		return sourceID, savedCardID, squareCustomerID, true
+	}
+	if cardID != nil {
+		card, err := svc.GetCardByID(ctx, *cardID, userID)
+		if err != nil {
+			if errors.Is(err, pgx.ErrNoRows) {
+				http.Error(w, notFoundMsg, http.StatusNotFound)
+				return "", nil, "", false
+			}
+			log.Printf("Failed to get card: %v", err)
+			http.Error(w, "internal server error", http.StatusInternalServerError)
+			return "", nil, "", false
+		}
+		if card.SquareCustomerID == "" {
+			if userID == "" {
+				// Defensive parity with the original saved-card block: a card
+				// with no bookable owner cannot be provisioned. Unreachable in
+				// practice — GetCardByID above filters on user_id and would
+				// have 404'd for an empty owner.
+				http.Error(w, "Saved card has no owner and cannot be charged", http.StatusBadRequest)
+				return "", nil, "", false
+			}
+			provisioned, provErr := svc.EnsureSquareCustomerForSavedCard(ctx, *cardID, userID)
+			if provErr != nil {
+				log.Printf("Failed to provision Square customer for saved card %s (user %s): %v", *cardID, userID, provErr)
+				http.Error(w, "Failed to process card", http.StatusInternalServerError)
+				return "", nil, "", false
+			}
+			card.SquareCustomerID = provisioned
+		}
+		return card.SquareCardID, cardID, card.SquareCustomerID, true
+	}
+	// Neither a new-card token nor a saved card — validation upstream
+	// (ValidateCardInfo) guarantees one of them is present.
+	return "", nil, "", false
+}
+
+// acquireBookingPaymentLock acquires a pinned pool connection and a bounded
+// try-lock (R6) on lockKey, serializing payment attempts per booking (the core
+// defence against the two-tab double-payment race). A blocking pg_advisory_lock
+// would hold the pinned pool connection for the full Square round-trip of
+// whichever request holds the lock; the bounded try-lock loop gives up after
+// ~3s and surfaces a 409 instead of exhausting the pool. On any failure the
+// helper writes the HTTP response and returns ok=false — the caller must
+// return. On success the caller MUST defer releaseBookingPaymentLock(pinConn,
+// lockKey): the lock and connection stay held for the whole handler so
+// pg_advisory_unlock runs on the SAME session that acquired the lock.
+func acquireBookingPaymentLock(ctx context.Context, w http.ResponseWriter, lockKey, conflictMsg string) (*pgxpool.Conn, bool) {
+	pinConn, err := db.Conn.Acquire(ctx)
+	if err != nil {
+		log.Printf("Failed to acquire connection for payment lock (%s): %v", lockKey, err)
+		http.Error(w, "internal server error", http.StatusInternalServerError)
+		return nil, false
+	}
+	lockOK, err := acquireAdvisoryLock(ctx, pinConn, lockKey)
+	if err != nil {
+		pinConn.Release()
+		log.Printf("Failed to acquire payment serialization lock %s: %v", lockKey, err)
+		http.Error(w, "internal server error", http.StatusInternalServerError)
+		return nil, false
+	}
+	if !lockOK {
+		pinConn.Release()
+		log.Printf("Payment serialization lock %s not acquired within bound — a payment is already in progress", lockKey)
+		http.Error(w, conflictMsg, http.StatusConflict)
+		return nil, false
+	}
+	return pinConn, true
+}
+
+// releaseBookingPaymentLock releases the advisory lock acquired by
+// acquireBookingPaymentLock and returns the pinned connection to the pool.
+// Both run on the same session that holds the lock.
+func releaseBookingPaymentLock(pinConn *pgxpool.Conn, lockKey string) {
+	if _, err := pinConn.Exec(context.Background(), `
+		SELECT pg_advisory_unlock(hashtext($1))
+	`, lockKey); err != nil {
+		log.Printf("Failed to release payment serialization lock %s: %v", lockKey, err)
+	}
+	pinConn.Release()
+}
+
+// recheckBookingPayable re-reads the booking status after a Square charge
+// succeeded (R9): a concurrent cancellation/eviction can move the booking out
+// of a payable state between the pre-charge status check and the charge
+// completing. A charge landing on a cancelled/lapsed/no-show booking must NOT
+// be recorded as a completed payment — the cancellation refund path computes
+// refunds from completed payments and would silently exclude it. Returns the
+// re-read status and whether a completed payment is still allowed; the caller
+// owns the CRITICAL logging, the mark-failed write (whose target and
+// transaction semantics differ per path), and the 409 conflict response.
+func recheckBookingPayable(ctx context.Context, q db.Querier, bookingID string) (string, bool, error) {
+	var status string
+	if err := q.QueryRow(ctx, `SELECT status FROM bookings WHERE id = $1`, bookingID).Scan(&status); err != nil {
+		return "", false, err
+	}
+	return status, bookingStatusAllowsCompletedPayment(status), nil
+}

backend/handlers/payments/errors.go

@@ -0,0 +1,54 @@
+package payments
+
+import (
+	"context"
+	"errors"
+	"net/http"
+
+	"crussell/internal/square"
+)
+
+// chargeFailureStatus classifies a SquareClient.CreatePayment error into the
+// HTTP status a payment handler should return:
+//
+//   - 503 (Service Unavailable) for AMBIGUOUS failures: transport/network
+//     errors, Square 5xx responses, context cancellation/deadline, and the
+//     retryable 4xx statuses 429 (rate limited), 408 (request timeout), and
+//     425 (too early) — the money state at Square is unknown, so the frontend
+//     should treat it as a retry (the pending record is resumed on a same-key
+//     retry). Square's own docs treat 429 as "retry later"; mapping it (or a
+//     timeout/early request) to 402 would mislabel a retryable condition as a
+//     permanent decline.
+//   - 402 (Payment Required) for DEFINITIVE declines: a structured Square
+//     error (squareAPIError) carrying any OTHER 4xx status (400/402/422 etc.)
+//     means Square positively rejected the charge (card declined/expired,
+//     AVS/CVV failure) — retrying with the same inputs cannot succeed.
+//
+// A nil error is never expected (callers only invoke this on the error path);
+// it maps to 402 defensively. The dev mock returns plain errors for simulated
+// failures, which classify as 503 (ambiguous) — correct for a mock standing in
+// for an unreachable Square.
+func chargeFailureStatus(err error) int {
+	if err == nil {
+		return http.StatusPaymentRequired
+	}
+	if errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled) {
+		return http.StatusServiceUnavailable
+	}
+	status := square.ErrorStatusCode(err)
+	if status == 0 || status >= 500 {
+		return http.StatusServiceUnavailable
+	}
+	// Retryable/ambiguous 4xx carve-outs: 429 (RATE_LIMITED), 408 (request
+	// timeout), and 425 (too early) are not definitive declines — Square's
+	// docs tell clients to retry later. Classify them as 503 so the pending
+	// record stays resumable on a same-key retry instead of being labelled a
+	// permanent decline. True declines (400/402/422 etc.) fall through to 402.
+	if status == http.StatusTooManyRequests || status == http.StatusRequestTimeout || status == http.StatusTooEarly {
+		return http.StatusServiceUnavailable
+	}
+	if status >= 400 && status < 500 {
+		return http.StatusPaymentRequired
+	}
+	return http.StatusServiceUnavailable
+}

backend/handlers/payments/handlers.go

@@ -288,7 +288,7 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) {
 	// one booking). Use a unique key per payment: retries of a lost response
 	// are handled by the Square-side key for card payments, and cash/giftcard
 	// are DB-committed synchronously.
-	idempotencyKey := uniqueTipKey()
+	idempotencyKey := uniqueChargeKey("tip-")
 
 	// Route based on payment method
 	if req.PaymentMethod != nil && (*req.PaymentMethod == "cash" || *req.PaymentMethod == "giftcard") {
@@ -514,34 +514,13 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) {
 			http.Error(w, "internal server error", http.StatusInternalServerError)
 			return
 		}
-		card, err := service.GetCardByID(r.Context(), *req.UserSavedCardID, bookingUserID.String)
-		if err != nil {
-			if errors.Is(err, pgx.ErrNoRows) {
-				http.Error(w, "Saved card not found", http.StatusNotFound)
-				return
-			}
-			log.Printf("Failed to get saved card: %v", err)
-			http.Error(w, "internal server error", http.StatusInternalServerError)
+		// Resolve the saved-card Square source for the booking's user (the
+		// card's owner, not the admin) — shared new-card-vs-saved-card
+		// resolution, see resolveChargeSource for the R6 rationale.
+		sourceID, _, savedCardCustomerID, sourceOK := resolveChargeSource(r.Context(), w, service, bookingUserID.String, nil, req.UserSavedCardID, false, "Saved card not found")
+		if !sourceOK {
 			return
 		}
-		// A ccof: source can NEVER be charged without a CustomerID — Square
-		// rejects the payment. A saved-card row created before P14 has an empty
-		// square_customer_id; lazily provision the booking user's Square
-		// customer and persist it on the row BEFORE charging (R6). A card with
-		// no bookable owner cannot be provisioned — refuse the charge.
-		if card.SquareCustomerID == "" {
-			if !bookingUserID.Valid {
-				http.Error(w, "Saved card has no owner and cannot be charged", http.StatusBadRequest)
-				return
-			}
-			provisioned, provErr := service.EnsureSquareCustomerForSavedCard(r.Context(), *req.UserSavedCardID, bookingUserID.String)
-			if provErr != nil {
-				log.Printf("Failed to provision Square customer for saved card %s (user %s): %v", *req.UserSavedCardID, bookingUserID.String, provErr)
-				http.Error(w, "Failed to process card", http.StatusInternalServerError)
-				return
-			}
-			card.SquareCustomerID = provisioned
-		}
 
 		// Serialize saved-card charges per booking (same lock as online booking
 		// payments) so concurrent double-clicks can't both pass the idempotency
@@ -550,31 +529,11 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) {
 		// would hold the pinned pool connection for the full Square round-trip of
 		// whichever request holds the lock, and ~4 concurrent same-booking
 		// requests would exhaust the whole pool.
-		pinConn, err := db.Conn.Acquire(r.Context())
-		if err != nil {
-			log.Printf("Failed to acquire connection for saved-card lock: %v", err)
-			http.Error(w, "internal server error", http.StatusInternalServerError)
-			return
-		}
-		defer pinConn.Release()
-		lockOK, err := acquireAdvisoryLock(r.Context(), pinConn, "crussell:payment:"+bookingID)
-		if err != nil {
-			log.Printf("Failed to acquire saved-card serialization lock for %s: %v", bookingID, err)
-			http.Error(w, "internal server error", http.StatusInternalServerError)
-			return
-		}
+		pinConn, lockOK := acquireBookingPaymentLock(r.Context(), w, "crussell:payment:"+bookingID, "Payment in progress, try again")
 		if !lockOK {
-			log.Printf("Saved-card serialization lock for %s not acquired within bound — a payment is in progress", bookingID)
-			http.Error(w, "Payment in progress, try again", http.StatusConflict)
 			return
 		}
-		defer func() {
-			if _, err := pinConn.Exec(context.Background(), `
-				SELECT pg_advisory_unlock(hashtext('crussell:payment:' || $1))
-			`, bookingID); err != nil {
-				log.Printf("Failed to release saved-card serialization lock for %s: %v", bookingID, err)
-			}
-		}()
+		defer releaseBookingPaymentLock(pinConn, "crussell:payment:"+bookingID)
 
 		// Deterministic idempotency key: booking+type+amount+card. A network
 		// retry with the same inputs derives the same key → dedup, never a
@@ -666,8 +625,8 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) {
 		paymentResult, err := SquareClient.CreatePayment(r.Context(), square.CreatePaymentReq{
 			Amount:         amount,
 			Currency:       "GBP",
-			SourceID:       card.SquareCardID,
-			CustomerID:     card.SquareCustomerID,
+			SourceID:       sourceID,
+			CustomerID:     savedCardCustomerID,
 			IdempotencyKey: scKey,
 			ReferenceID:    bookingID,
 			Note:           req.PaymentType,
@@ -675,7 +634,7 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) {
 		})
 		if err != nil {
 			log.Printf("Failed to process saved-card payment: %v", err)
-			http.Error(w, "Payment failed", http.StatusPaymentRequired)
+			http.Error(w, "Payment failed", chargeFailureStatus(err))
 			return
 		}
 
@@ -687,14 +646,14 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) {
 		// completed (the cancellation refund path computes refunds from
 		// completed payments). Mark the row failed and alert ops: money was
 		// taken at Square and MUST be refunded manually.
-		var recheckStatus string
-		if err := db.Conn.QueryRow(r.Context(), `SELECT status FROM bookings WHERE id = $1`, bookingID).Scan(&recheckStatus); err != nil {
+		recheckStatus, payable, err := recheckBookingPayable(r.Context(), db.Conn, bookingID)
+		if err != nil {
 			log.Printf("CRITICAL: Square payment %s was processed for booking %s but re-reading booking status failed: %v — manual reconciliation required",
 				paymentResult.SquarePayID, bookingID, err)
 			http.Error(w, "internal server error", http.StatusInternalServerError)
 			return
 		}
-		if !bookingStatusAllowsCompletedPayment(recheckStatus) {
+		if !payable {
 			log.Printf("CRITICAL: Square payment %s was processed but booking %s is now %q — marking saved-card payment %s failed; money taken at Square MUST be refunded manually",
 				paymentResult.SquarePayID, bookingID, recheckStatus, paymentID)
 			if _, upErr := db.Conn.Exec(r.Context(), `UPDATE payments SET status = 'failed' WHERE id = $1`, paymentID); upErr != nil {
@@ -751,31 +710,11 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) {
 	// second live Square checkout for the same booking while the first is in
 	// flight. Bounded try-lock (R6) so a contended lock never blocks the pool
 	// across the Square round-trip.
-	pinConn, err := db.Conn.Acquire(r.Context())
-	if err != nil {
-		log.Printf("Failed to acquire connection for terminal checkout lock: %v", err)
-		http.Error(w, "internal server error", http.StatusInternalServerError)
-		return
-	}
-	defer pinConn.Release()
-	lockOK, err := acquireAdvisoryLock(r.Context(), pinConn, "crussell:payment:"+bookingID)
-	if err != nil {
-		log.Printf("Failed to acquire terminal checkout serialization lock for %s: %v", bookingID, err)
-		http.Error(w, "internal server error", http.StatusInternalServerError)
-		return
-	}
+	pinConn, lockOK := acquireBookingPaymentLock(r.Context(), w, "crussell:payment:"+bookingID, "Payment in progress, try again")
 	if !lockOK {
-		log.Printf("Terminal checkout serialization lock for %s not acquired within bound — a checkout is in progress", bookingID)
-		http.Error(w, "Payment in progress, try again", http.StatusConflict)
 		return
 	}
-	defer func() {
-		if _, err := pinConn.Exec(context.Background(), `
-			SELECT pg_advisory_unlock(hashtext('crussell:payment:' || $1))
-		`, bookingID); err != nil {
-			log.Printf("Failed to release terminal checkout serialization lock for %s: %v", bookingID, err)
-		}
-	}()
+	defer releaseBookingPaymentLock(pinConn, "crussell:payment:"+bookingID)
 
 	if existing := activeTerminalCheckoutID(r.Context(), bookingID); existing != "" {
 		if err := json.NewEncoder(w).Encode(CheckoutResponse{
@@ -997,9 +936,11 @@ func GetCheckoutStatus(w http.ResponseWriter, r *http.Request) {
 	// CreateTerminalPayment sets reference_id = bookingID; without this check,
 	// polling the wrong checkout ID would attach its payment to a different
 	// booking (admin-only route, but a mis-scoped charge is a data-integrity
-	// bug worth rejecting).
-	if paymentResult.ReferenceID != "" && paymentResult.ReferenceID != bookingID {
-		log.Printf("Checkout %s references booking %s, not %s — refusing to record", checkoutID, paymentResult.ReferenceID, bookingID)
+	// bug worth rejecting). An EMPTY reference_id is also rejected: a checkout
+	// created outside this app with no reference must not be attachable to a
+	// booking (S-1) — fail closed on anything that is not exactly this booking.
+	if paymentResult.ReferenceID == "" || paymentResult.ReferenceID != bookingID {
+		log.Printf("Checkout %s does not reference booking %s (reference_id=%q) — refusing to record", checkoutID, bookingID, paymentResult.ReferenceID)
 		http.Error(w, "Checkout does not belong to this booking", http.StatusBadRequest)
 		return
 	}
@@ -1176,7 +1117,12 @@ func GetCheckoutStatus(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	http.Error(w, "Payment failed", http.StatusPaymentRequired)
+	// No non-COMPLETED fallthrough here: GetCheckout (via getCheckoutHTTP)
+	// only returns a nil error for a COMPLETED checkout — a non-COMPLETED
+	// status or an expired/cancelled checkout surfaces as an error, which was
+	// already handled above (ErrCheckoutPending → PENDING, everything else →
+	// 500). The previous trailing `http.Error(w, "Payment failed", 402)` was
+	// unreachable dead code and has been removed.
 }
 
 // IsValidBookingStatusForPayment returns true if the booking status allows
@@ -1310,32 +1256,11 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) {
 	// blocking pg_advisory_lock. A blocking lock would pin the pool connection
 	// for the whole Square round-trip (~30s), so ~4 concurrent same-booking
 	// payments would exhaust the default pool and hang every request.
-	pinConn, err := db.Conn.Acquire(r.Context())
-	if err != nil {
-		log.Printf("Failed to acquire connection for payment lock: %v", err)
-		http.Error(w, "Internal server error", http.StatusInternalServerError)
-		return
-	}
-	defer pinConn.Release()
-
-	lockOK, err := acquireAdvisoryLock(r.Context(), pinConn, "crussell:payment:"+bookingID)
-	if err != nil {
-		log.Printf("Failed to acquire payment serialization lock for %s: %v", bookingID, err)
-		http.Error(w, "Internal server error", http.StatusInternalServerError)
-		return
-	}
+	pinConn, lockOK := acquireBookingPaymentLock(r.Context(), w, "crussell:payment:"+bookingID, "Payment in progress, try again")
 	if !lockOK {
-		log.Printf("Payment serialization lock for %s not acquired within bound — a payment is already in progress", bookingID)
-		http.Error(w, "Payment in progress, try again", http.StatusConflict)
 		return
 	}
-	defer func() {
-		if _, err := pinConn.Exec(context.Background(), `
-			SELECT pg_advisory_unlock(hashtext('crussell:payment:' || $1))
-		`, bookingID); err != nil {
-			log.Printf("Failed to release payment serialization lock for %s: %v", bookingID, err)
-		}
-	}()
+	defer releaseBookingPaymentLock(pinConn, "crussell:payment:"+bookingID)
 
 	// Now that we hold the serialization lock, begin a transaction and re-check
 	// the booking status inside it. If another request (e.g. from a different tab)
@@ -1461,85 +1386,12 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) {
 	var sourceID string
 	var savedCardID *string
 	var savedCardCustomerID string
-
-	if req.NewCardToken != nil && *req.NewCardToken != "" {
-		// A cnon: nonce charge needs NO card-on-file and NO customer (R6). The
-		// old code tokenized every new card via CreateCardOnFile even for
-		// one-off non-save charges, which (a) created an orphan card at Square
-		// for a payment that only ever uses the nonce once, and (b) would have
-		// charged the resulting ccof: source without a CustomerID — Square
-		// rejects a card-on-file source that carries no customer.
-		if req.SaveCard {
-			// Save path: provision (or reuse) the user's Square customer BEFORE
-			// tokenizing so the new card is created against that customer, and
-			// forward the customer id on the charge. A ccof: source MUST carry
-			// its customer (R6) — the charge below sets
-			// CustomerID = savedCardCustomerID = squareCustomerID.
-			squareCustomerID, custErr := service.EnsureSquareCustomer(r.Context(), userID)
-			if custErr != nil {
-				log.Printf("Failed to provision Square customer for user %s: %v", userID, custErr)
-				http.Error(w, "Failed to process card", http.StatusInternalServerError)
-				return
-			}
-			cardOnFile, err := SquareClient.CreateCardOnFile(r.Context(), userID, *req.NewCardToken, squareCustomerID)
-			if err != nil {
-				log.Printf("Failed to create card on file: %v", err)
-				http.Error(w, "Failed to process card", http.StatusInternalServerError)
-				return
-			}
-			sourceID = cardOnFile.CardID
-			savedCardCustomerID = squareCustomerID
-
-			// CreateCardOnFile runs before the charge. If the subsequent payment
-			// fails, this card-on-file is intentionally NOT deleted: the pending
-			// record's retry re-creates it via the deterministic sha256
-			// idempotency key (the SAVE path), and Square returns the same card —
-			// deleting it would break that retry.
-			cardID, err := service.SaveCardForUser(r.Context(), userID, squareCustomerID, cardOnFile.CardID, cardOnFile.Brand, cardOnFile.Last4, cardOnFile.ExpMonth, cardOnFile.ExpYear, cardOnFile.Fingerprint)
-			if err != nil {
-				log.Printf("Failed to save card: %v", err)
-			} else {
-				savedCardID = &cardID
-			}
-		} else {
-			// One-off new-card charge: use the cnon: nonce DIRECTLY as the
-			// source. No card-on-file is created (nothing to orphan, no
-			// customer needed) — this also eliminates the orphan-card
-			// accumulation the old non-save tokenize-then-charge flow left
-			// behind at Square.
-			sourceID = *req.NewCardToken
-		}
-		if savedCardID == nil && req.SaveCard {
-			log.Printf("Card was not saved despite save_card=true for user %s", userID)
-		}
-	} else if req.CardID != nil {
-		card, err := service.GetCardByID(r.Context(), *req.CardID, userID)
-		if err != nil {
-			if errors.Is(err, pgx.ErrNoRows) {
-				http.Error(w, "Card not found", http.StatusNotFound)
-				return
-			}
-			log.Printf("Failed to get card: %v", err)
-			http.Error(w, "internal server error", http.StatusInternalServerError)
-			return
-		}
-		// A ccof: source can NEVER be charged without a CustomerID — Square
-		// rejects the payment. A saved-card row created before P14 has an empty
-		// square_customer_id; lazily provision the user's Square customer and
-		// persist it on the row BEFORE charging (R6). Provisioning failure
-		// aborts the charge with a 500.
-		if card.SquareCustomerID == "" {
-			provisioned, provErr := service.EnsureSquareCustomerForSavedCard(r.Context(), *req.CardID, userID)
-			if provErr != nil {
-				log.Printf("Failed to provision Square customer for saved card %s (user %s): %v", *req.CardID, userID, provErr)
-				http.Error(w, "Failed to process card", http.StatusInternalServerError)
-				return
-			}
-			card.SquareCustomerID = provisioned
-		}
-		sourceID = card.SquareCardID
-		savedCardID = req.CardID
-		savedCardCustomerID = card.SquareCustomerID
+	// Resolve the new-card-vs-saved-card Square source (shared with
+	// CreateTipPayment, BuyGiftCard, and the saved-card branch of
+	// CreateTerminalPayment — see resolveChargeSource for the R6 rationale).
+	sourceID, savedCardID, savedCardCustomerID, sourceOK := resolveChargeSource(r.Context(), w, service, userID, req.NewCardToken, req.CardID, req.SaveCard, "Card not found")
+	if !sourceOK {
+		return
 	}
 
 	// If there is no pending record to reuse, insert one NOW and commit the
@@ -1609,7 +1461,7 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) {
 	paymentResult, err := SquareClient.CreatePayment(r.Context(), paymentReq)
 	if err != nil {
 		log.Printf("Failed to create payment: %v", err)
-		http.Error(w, "Payment failed", http.StatusPaymentRequired)
+		http.Error(w, "Payment failed", chargeFailureStatus(err))
 		return
 	}
 
@@ -1643,14 +1495,14 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) {
 	// manually. The pending row is marked 'failed' in the tx below, so
 	// idempotency dedup still blocks a second Square charge, but the row no
 	// longer shows pending — the frontend's retry gets a 409 Conflict.
-	var recheckStatus string
-	if err := tx2.QueryRow(r.Context(), `SELECT status FROM bookings WHERE id = $1`, bookingID).Scan(&recheckStatus); err != nil {
+	recheckStatus, payable, err := recheckBookingPayable(r.Context(), tx2, bookingID)
+	if err != nil {
 		log.Printf("CRITICAL: Square payment %s (ID=%s) was processed but re-reading booking %s status failed: %v — manual reconciliation required",
 			paymentResult.Status, paymentResult.SquarePayID, bookingID, err)
 		http.Error(w, "internal server error", http.StatusInternalServerError)
 		return
 	}
-	if !bookingStatusAllowsCompletedPayment(recheckStatus) {
+	if !payable {
 		log.Printf("CRITICAL: Square payment %s (ID=%s) for booking %s was processed but booking is now %q — marking payment failed; money taken at Square MUST be refunded manually",
 			paymentResult.Status, paymentResult.SquarePayID, bookingID, recheckStatus)
 		if _, upErr := tx2.Exec(r.Context(), `UPDATE payments SET status = 'failed' WHERE id = $1`, paymentID); upErr != nil {
@@ -1838,6 +1690,24 @@ func applyEligibleCampaignsAtPayment(ctx context.Context, q db.Querier, bookingI
 // The primary record carries the Square payment ID for refund routing; split
 // records share the same SquarePaymentID so the refund loop can avoid duplicate
 // Square API calls while still creating audit records.
+//
+// MONEY INVARIANT (deliberately kept exact): the returned records always
+// partition paymentAmount — deposit + balance + tip === paymentAmount exactly
+// (every component is rounded to the cent and the parts are derived from one
+// another, so no rounding residue exists). The sum of the split records can
+// therefore never exceed the amount actually charged at Square. When deposit
+// AND balance are both zero (booking already fully paid) the tip record alone
+// carries the whole payment — the primary must NOT be appended as well, or the
+// amount would be recorded twice (see the tip block below).
+//
+// Discounts do NOT change this: a discount is applied at payment time as a
+// SEPARATE ledger payment row (payment_method='discount'), and GetBookingPaymentInfo
+// excludes those rows from TotalPaid (as do the refund and deposit-threshold
+// computations). buildSplitRecords therefore runs against the full booking
+// total and the REAL money already paid, so a discounted booking can at worst
+// over-allocate toward balance and under-allocate toward tip (a bookkeeping
+// simplification, not an overcharge) — the partition still equals the charged
+// amount. See TestBuildSplitRecords_DiscountBooking_TipOverflow_SumNeverExceedsCharge.
 func buildSplitRecords(primary PaymentRecord, reqPaymentType string, info *BookingPaymentInfo, paymentAmount float64) []PaymentRecord {
 	// After the booking starts there is no deposit protection window —
 	// record the payment as a single entry with its original type.
@@ -1896,14 +1766,13 @@ func buildSplitRecords(primary PaymentRecord, reqPaymentType string, info *Booki
 		splitIdx++
 	}
 
-	// If neither deposit nor balance was created (deposit exhausted, booking
-	// fully paid), the primary is still a valid record — use it directly.
-	if len(records) == 0 {
-		primary.Fees = 0
-		records = append(records, primary)
-	}
-
-	// Tip record — overflow beyond the booking total.
+	// 3. Tip record — overflow beyond the booking total. Appended BEFORE the
+	// primary fallback below: when BOTH the deposit and balance portions are
+	// zero (deposit room exhausted AND the booking already fully paid — e.g. a
+	// discounted booking whose TotalPaid, which excludes discount rows, has
+	// reached the full total), the tip record carries the ENTIRE payment.
+	// Appending the primary first would double-count the charged amount
+	// (primary at the full amount + tip at the same full amount).
 	if tipPortion > 0.004 {
 		tip := primary
 		tip.PaymentType = "tip"
@@ -1917,10 +1786,12 @@ func buildSplitRecords(primary PaymentRecord, reqPaymentType string, info *Booki
 		records = append(records, tip)
 	}
 
-	// If nothing was appended (shouldn't happen given validation upstream),
-	// return the primary as a fallback.
+	// Defensive fallback: nothing was appended (deposit, balance, AND tip all
+	// zero — impossible given paymentAmount is validated > 0 upstream, so this
+	// is a pure safety net). The primary is still a valid single record.
 	if len(records) == 0 {
-		return []PaymentRecord{primary}
+		primary.Fees = 0
+		records = append(records, primary)
 	}
 	return records
 }
@@ -2105,16 +1976,22 @@ func RefundPayment(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	// Deterministic idempotency key so a same-key retry (network timeout)
-	// does not create a second Square refund. When the client supplies an
-	// idempotency key (one per distinct refund attempt, reused on retry), use
-	// it — the amount-derived fallback would collide on two DISTINCT partial
-	// refunds of the same amount, silently swallowing the second. The client
-	// key is hashed+truncated: Square's idempotency-key limit is 45 chars, and
+	// Idempotency key for the refund. When the client supplies one (a UUID
+	// generated per distinct refund attempt and REUSED on retry), the key is
+	// hashed and truncated: Square's idempotency-key limit is 45 chars, and
 	// paymentID (12) + "-refund-" (8) + a full 36-char UUID (56 total) would
 	// be rejected with a 400. The hash stays deterministic, so a same-key
 	// retry still dedups.
-	idempotencyKey := paymentID + "-refund-" + strconv.FormatInt(req.Amount, 10)
+	//
+	// When the client sends NO key, the fallback must be UNIQUE per refund
+	// attempt: the old amount-derived key (paymentID + "-refund-" + amount)
+	// collided on two DISTINCT partial refunds of the same amount, and the
+	// dedup lookup silently swallowed the second. The fallback appends a fresh
+	// crypto/rand hex suffix so distinct same-amount refunds never collide;
+	// a lost-response no-key retry still resumes via the (payment_id, amount)
+	// pending fallback below. 6 random bytes (12 hex chars) keeps the full key
+	// ≤45 chars even for a 9-digit pence amount.
+	idempotencyKey := paymentID + "-refund-" + strconv.FormatInt(req.Amount, 10) + "-" + randomHexSuffix(6)
 	if req.IdempotencyKey != "" {
 		ikHash := sha256.Sum256([]byte(req.IdempotencyKey))
 		idempotencyKey = paymentID + "-refund-" + fmt.Sprintf("%x", ikHash)[:24]
@@ -2227,7 +2104,7 @@ func RefundPayment(w http.ResponseWriter, r *http.Request) {
 			reissueReq := square.RefundPaymentReq{
 				PaymentID:      refundSqPaymentID,
 				Amount:         resumeAmount,
-				IdempotencyKey: existingRefundKey.String,
+				IdempotencyKey: refundResumeKey(paymentID, resumeAmount, existingRefundKey.String),
 				Reason:         existingRefundReason.String,
 			}
 			reissueResult, reissueErr := SquareClient.RefundPayment(r.Context(), reissueReq)
@@ -2512,12 +2389,28 @@ func RefundPayment(w http.ResponseWriter, r *http.Request) {
 // pending fallback. Using the stored key lets Square return the original
 // refund if the prior attempt actually completed (response loss), so no second
 // refund can ever be issued for a row whose money state is unknown.
+// refundResumeKey returns the row's OWN stored idempotency key when present, or
+// a fresh deterministic fallback when the row predates keyed refunds (legacy
+// NULL idempotency_key). Square's RefundPayment REQUIRES a non-empty
+// idempotency key — re-issuing with "" returns a 400 INVALID_REQUEST_ERROR,
+// which classifies as ambiguous and leaves the refund pending forever. The
+// fallback mirrors the no-client-key shape (paymentID + "-refund-" + amount +
+// "-" + hex) and stays ≤45 chars (12 + 8 + up-to-9 + 1 + 12 ≈ 42), so the
+// re-issue is never rejected for length either. A legit stored key is ALWAYS
+// reused so Square's same-key dedup keeps returning the original refund.
+func refundResumeKey(paymentID string, amount int64, storedKey string) string {
+	if storedKey != "" {
+		return storedKey
+	}
+	return paymentID + "-refund-" + strconv.FormatInt(amount, 10) + "-" + randomHexSuffix(6)
+}
+
 func resumeManualPendingRefund(w http.ResponseWriter, r *http.Request, paymentID string, payment *PaymentRecord, refundID string, refundAmount float64, refundReason, refundKey string) {
 	resumeAmount := int64(math.Round(refundAmount * 100))
 	resumeReq := square.RefundPaymentReq{
 		PaymentID:      *payment.SquarePaymentID,
 		Amount:         resumeAmount,
-		IdempotencyKey: refundKey,
+		IdempotencyKey: refundResumeKey(paymentID, resumeAmount, refundKey),
 		Reason:         refundReason,
 	}
 	resumeResult, resumeErr := SquareClient.RefundPayment(r.Context(), resumeReq)
@@ -2685,122 +2578,29 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) {
 	// request fields alone (bookingID + amount would dedupe distinct tips).
 	idempotencyKey := req.IdempotencyKey
 	if idempotencyKey == "" {
-		idempotencyKey = uniqueTipKey()
+		idempotencyKey = uniqueChargeKey("tip-")
 	}
 
-	// Resolve the card source ID — same pattern as CreateBookingPayment.
+	// Resolve the card source ID — same pattern as CreateBookingPayment (see
+	// resolveChargeSource for the R6 rationale).
 	var sourceID string
 	var savedCardID *string
 	var savedCardCustomerID string
-
-	if req.NewCardToken != nil && *req.NewCardToken != "" {
-		// A cnon: nonce charge needs NO card-on-file and NO customer (R6). The
-		// old code tokenized every new card via CreateCardOnFile even for
-		// one-off non-save charges, which (a) created an orphan card at Square
-		// for a payment that only ever uses the nonce once, and (b) would have
-		// charged the resulting ccof: source without a CustomerID — Square
-		// rejects a card-on-file source that carries no customer.
-		if req.SaveCard {
-			// Save path: provision (or reuse) the user's Square customer BEFORE
-			// tokenizing so the new card is created against that customer, and
-			// forward the customer id on the charge. A ccof: source MUST carry
-			// its customer (R6) — the charge below sets
-			// CustomerID = savedCardCustomerID = squareCustomerID.
-			squareCustomerID, custErr := service.EnsureSquareCustomer(r.Context(), userID)
-			if custErr != nil {
-				log.Printf("Failed to provision Square customer for user %s: %v", userID, custErr)
-				http.Error(w, "Failed to process card", http.StatusInternalServerError)
-				return
-			}
-			cardOnFile, err := SquareClient.CreateCardOnFile(r.Context(), userID, *req.NewCardToken, squareCustomerID)
-			if err != nil {
-				log.Printf("Failed to create card on file: %v", err)
-				http.Error(w, "Failed to process card", http.StatusInternalServerError)
-				return
-			}
-			sourceID = cardOnFile.CardID
-			savedCardCustomerID = squareCustomerID
-
-			// CreateCardOnFile runs before the charge. If the subsequent payment
-			// fails, this card-on-file is intentionally NOT deleted: the pending
-			// record's retry re-creates it via the deterministic sha256
-			// idempotency key (the SAVE path), and Square returns the same card —
-			// deleting it would break that retry.
-			cardID, err := service.SaveCardForUser(r.Context(), userID, squareCustomerID, cardOnFile.CardID, cardOnFile.Brand, cardOnFile.Last4, cardOnFile.ExpMonth, cardOnFile.ExpYear, cardOnFile.Fingerprint)
-			if err != nil {
-				log.Printf("Failed to save card: %v", err)
-			} else {
-				savedCardID = &cardID
-			}
-		} else {
-			// One-off new-card tip: use the cnon: nonce DIRECTLY as the source.
-			// No card-on-file is created (nothing to orphan, no customer needed).
-			sourceID = *req.NewCardToken
-		}
-		if savedCardID == nil && req.SaveCard {
-			log.Printf("Card was not saved despite save_card=true for user %s", userID)
-		}
-	} else if req.CardID != nil {
-		card, err := service.GetCardByID(r.Context(), *req.CardID, userID)
-		if err != nil {
-			if errors.Is(err, pgx.ErrNoRows) {
-				http.Error(w, "Card not found", http.StatusNotFound)
-				return
-			}
-			log.Printf("Failed to get card: %v", err)
-			http.Error(w, "internal server error", http.StatusInternalServerError)
-			return
-		}
-		// A ccof: source can NEVER be charged without a CustomerID — Square
-		// rejects the payment. A saved-card row created before P14 has an empty
-		// square_customer_id; lazily provision the user's Square customer and
-		// persist it on the row BEFORE charging (R6).
-		if card.SquareCustomerID == "" {
-			provisioned, provErr := service.EnsureSquareCustomerForSavedCard(r.Context(), *req.CardID, userID)
-			if provErr != nil {
-				log.Printf("Failed to provision Square customer for saved card %s (user %s): %v", *req.CardID, userID, provErr)
-				http.Error(w, "Failed to process card", http.StatusInternalServerError)
-				return
-			}
-			card.SquareCustomerID = provisioned
-		}
-		sourceID = card.SquareCardID
-		savedCardID = req.CardID
-		savedCardCustomerID = card.SquareCustomerID
+	sourceID, savedCardID, savedCardCustomerID, sourceOK := resolveChargeSource(r.Context(), w, service, userID, req.NewCardToken, req.CardID, req.SaveCard, "Card not found")
+	if !sourceOK {
+		return
 	}
 
 	// Serialize tip attempts for this booking to prevent concurrent duplicate
 	// tip payments across browser tabs or retries. Uses a PostgreSQL session-level
 	// advisory lock scoped to the booking ID.
-	// See CreateBookingPayment lines 815-846 for the same pattern.
 	// Bounded try-lock (R6) so a contended lock never blocks the pool across
 	// the Square round-trip.
-	pinConn, err := db.Conn.Acquire(r.Context())
-	if err != nil {
-		log.Printf("Failed to acquire connection for tip lock: %v", err)
-		http.Error(w, "Internal server error", http.StatusInternalServerError)
-		return
-	}
-	defer pinConn.Release()
-
-	lockOK, err := acquireAdvisoryLock(r.Context(), pinConn, "crussell:tip:"+bookingID)
-	if err != nil {
-		log.Printf("Failed to acquire tip serialization lock for %s: %v", bookingID, err)
-		http.Error(w, "Internal server error", http.StatusInternalServerError)
-		return
-	}
+	pinConn, lockOK := acquireBookingPaymentLock(r.Context(), w, "crussell:tip:"+bookingID, "Payment in progress, try again")
 	if !lockOK {
-		log.Printf("Tip serialization lock for %s not acquired within bound — a tip payment is already in progress", bookingID)
-		http.Error(w, "Payment in progress, try again", http.StatusConflict)
 		return
 	}
-	defer func() {
-		if _, err := pinConn.Exec(context.Background(), `
-			SELECT pg_advisory_unlock(hashtext('crussell:tip:' || $1))
-		`, bookingID); err != nil {
-			log.Printf("Failed to release tip serialization lock for %s: %v", bookingID, err)
-		}
-	}()
+	defer releaseBookingPaymentLock(pinConn, "crussell:tip:"+bookingID)
 
 	// Step 1: Insert payment record in 'pending' state inside a DB transaction.
 	// Square is NOT called yet — if the tx fails, no harm done.
@@ -2944,7 +2744,7 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) {
 	if err != nil {
 		log.Printf("Failed to create tip payment: %v", err)
 		// Payment record intentionally left as 'pending' for manual retry.
-		http.Error(w, "Payment failed", http.StatusPaymentRequired)
+		http.Error(w, "Payment failed", chargeFailureStatus(err))
 		return
 	}
 
@@ -2956,14 +2756,14 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) {
 	// would silently exclude it. Mark the tip row failed and alert ops: money
 	// was taken at Square and MUST be refunded manually (mirrors
 	// CreateBookingPayment's post-charge recheck).
-	var tipRecheckStatus string
-	if err := db.Conn.QueryRow(r.Context(), `SELECT status FROM bookings WHERE id = $1`, bookingID).Scan(&tipRecheckStatus); err != nil {
+	tipRecheckStatus, tipPayable, err := recheckBookingPayable(r.Context(), db.Conn, bookingID)
+	if err != nil {
 		log.Printf("CRITICAL: Square tip payment %s (ID=%s) was processed but re-reading booking %s status failed: %v — manual reconciliation required",
 			paymentResult.Status, paymentResult.SquarePayID, bookingID, err)
 		http.Error(w, "internal server error", http.StatusInternalServerError)
 		return
 	}
-	if !bookingStatusAllowsCompletedPayment(tipRecheckStatus) {
+	if !tipPayable {
 		log.Printf("CRITICAL: Square tip payment %s (ID=%s) for booking %s was processed but booking is now %q — marking tip %s failed; money taken at Square MUST be refunded manually",
 			paymentResult.Status, paymentResult.SquarePayID, bookingID, tipRecheckStatus, paymentID)
 		if _, upErr := db.Conn.Exec(r.Context(), `UPDATE payments SET status = 'failed' WHERE id = $1`, paymentID); upErr != nil {
@@ -3246,9 +3046,29 @@ func ReleasePaymentLock(w http.ResponseWriter, r *http.Request) {
 	w.WriteHeader(http.StatusNoContent)
 }
 
-// uniqueTipKey generates a unique idempotency key for tip payments where the
-// client did not supply one. Client-supplied UUIDs handle retry dedup; this
-// fallback only needs uniqueness so identical tips don't collapse.
-func uniqueTipKey() string {
-	return "tip-" + rand.Text()
+// uniqueChargeKey generates a unique idempotency key under the given prefix
+// (e.g. "tip-", "till-") where the client did not supply one. Client-supplied
+// keys handle retry dedup; this fallback only needs uniqueness so two
+// legitimate identical requests never collapse on the same key. Deliberately
+// NOT derived from request fields — two identical requests would hash to the
+// same key (the "tip" fallback must not dedupe two distinct equal tips on one
+// booking). Shared by the tip/till flows, which used to carry two identical
+// copies (uniqueTipKey/uniqueTillKey) differing only in the prefix string.
+func uniqueChargeKey(prefix string) string {
+	return prefix + rand.Text()
+}
+
+// randomHexSuffix returns n random bytes hex-encoded (2n hex chars) from
+// crypto/rand, used to disambiguate idempotency fallback keys that would
+// otherwise collide on deterministic inputs (e.g. the no-client-key refund
+// key). Falls back to a masked monotonic timestamp if the OS entropy source
+// errors — effectively impossible on Linux (crypto/rand.Read blocks until
+// entropy is available) — keeping the same width so the key stays within
+// Square's 45-char idempotency-key limit.
+func randomHexSuffix(n int) string {
+	b := make([]byte, n)
+	if _, err := rand.Read(b); err != nil {
+		return fmt.Sprintf("%0*x", 2*n, time.Now().UnixNano()&(int64(1)<<(8*int64(n))-1))
+	}
+	return fmt.Sprintf("%x", b)
 }