Fix gift-card, till, and refund paths: nonce-direct charges, customer_id forwarding, blocking refund lock

BuyGiftCard and till online_square charge the cnon: nonce directly (no synthetic card-on-file); BuyGiftCard.IdempotencyKey is now validate-required (empty key previously collided on the UNIQUE constraint). Saved-card branches forward customer_id and lazily provision legacy cards. lockCancellationPayments uses the deliberately-blocking xact lock so a cancellation never silently drops a refund under contention (admins see the full manual refund round-trip).

backend/handlers/payments/giftcards.go

@@ -81,7 +81,11 @@ type BuyGiftCardRequest struct {
 	CardID            *string `json:"card_id,omitempty"`
 	NewCardToken      *string `json:"new_card_token,omitempty"`
 	SaveCard          bool    `json:"save_card"`
-	IdempotencyKey    string  `json:"idempotency_key"`
+	// IdempotencyKey is required (R2): an empty key would be stored as '' on
+	// the pending payments row, and a second empty-key purchase would 500 on
+	// the UNIQUE(payments.idempotency_key) constraint. The frontend always
+	// sends a per-purchase UUID; the max=64 matches the column width.
+	IdempotencyKey    string  `json:"idempotency_key" validate:"required,max=64"`
 	VerificationToken *string `json:"verification_token,omitempty"`
 }
 
@@ -879,6 +883,22 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	// R2: idempotency_key is required (validate:"required,max=64"). An empty
+	// key would be stored as '' on the pending payments row and a second
+	// empty-key purchase would 500 on the UNIQUE(payments.idempotency_key)
+	// constraint. The frontend always sends a per-purchase UUID. The fallback
+	// below is defense-in-depth only — the validator rejects the empty key
+	// first, but if it is ever relaxed the fallback keeps the UNIQUE
+	// constraint from firing.
+	if err := validators.Validate.Struct(&req); err != nil {
+		log.Printf("Failed to process request: %v", err)
+		http.Error(w, "Invalid request", http.StatusBadRequest)
+		return
+	}
+	if req.IdempotencyKey == "" {
+		req.IdempotencyKey = uniqueTillKey()
+	}
+
 	allowedAmounts := map[int64]bool{1000: true, 2000: true, 5000: true}
 	if !allowedAmounts[req.Amount] {
 		http.Error(w, "Invalid amount. Must be £10, £20, or £50.", http.StatusBadRequest)
@@ -909,6 +929,8 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) {
 	// executing the gift-card creation (2× value for 1 charge). Mirrors the tip
 	// advisory-lock pattern (handlers.go). Lock is keyed on the idempotency key
 	// so distinct purchases are unaffected; falls back to userID when absent.
+	// Bounded try-lock (R6) so a contended lock never blocks the pool across
+	// the Square round-trip.
 	lockKey := req.IdempotencyKey
 	if lockKey == "" {
 		lockKey = userID
@@ -920,13 +942,17 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	defer pinConn.Release()
-	if _, err := pinConn.Exec(ctx, `
-		SELECT pg_advisory_lock(hashtext('crussell:giftcard:' || $1))
-	`, lockKey); err != nil {
+	lockOK, err := acquireAdvisoryLock(ctx, pinConn, "crussell:giftcard:"+lockKey)
+	if err != nil {
 		log.Printf("Failed to acquire gift-card serialization lock for %s: %v", lockKey, err)
 		http.Error(w, "internal server error", http.StatusInternalServerError)
 		return
 	}
+	if !lockOK {
+		log.Printf("Gift-card serialization lock for %s not acquired within bound — a purchase is already in progress", lockKey)
+		http.Error(w, "Purchase in progress, try again", http.StatusConflict)
+		return
+	}
 	defer func() {
 		if _, err := pinConn.Exec(context.Background(), `
 			SELECT pg_advisory_unlock(hashtext('crussell:giftcard:' || $1))
@@ -976,38 +1002,44 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) {
 
 	var sourceID string
 	var savedCardID *string
+	var savedCardCustomerID string
 
 	if req.NewCardToken != nil && *req.NewCardToken != "" {
-		// P14: when the card is being SAVED, provision (or reuse) the user's
-		// Square customer profile BEFORE tokenizing so the new card is created
-		// against that customer. One-off non-save charges pass "" — a cnon:
-		// nonce charge needs no customer.
-		squareCustomerID := ""
+		// 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 purchases, creating an orphan card at Square for a
+		// payment that only ever uses the nonce once.
 		if req.SaveCard {
-			var custErr error
-			squareCustomerID, custErr = paymentService.EnsureSquareCustomer(ctx, userID)
+			// 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).
+			squareCustomerID, custErr := paymentService.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
 			}
-		}
-		cardOnFile, err := SquareClient.CreateCardOnFile(ctx, 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
-		}
+			cardOnFile, err := SquareClient.CreateCardOnFile(ctx, 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
 
-		sourceID = cardOnFile.CardID
-
-		if req.SaveCard {
-			cardID, err := paymentService.SaveCardForUser(ctx, userID, cardOnFile.CardID, cardOnFile.Brand, cardOnFile.Last4, cardOnFile.ExpMonth, cardOnFile.ExpYear, cardOnFile.Fingerprint)
+			cardID, err := paymentService.SaveCardForUser(ctx, 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 purchase: use the cnon: nonce DIRECTLY as the
+			// source. No card-on-file is created (nothing to orphan, no
+			// customer needed).
+			sourceID = *req.NewCardToken
 		}
 	} else if req.CardID != nil {
 		card, err := paymentService.GetCardByID(ctx, *req.CardID, userID)
@@ -1020,8 +1052,22 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) {
 			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 := paymentService.EnsureSquareCustomerForSavedCard(ctx, *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
 	}
 
 	amountPounds := float64(req.Amount) / 100.0
@@ -1117,6 +1163,10 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) {
 		Amount:            req.Amount,
 		Currency:          "GBP",
 		SourceID:          sourceID,
+		// CustomerID carries the saved-card row's Square customer id on ccof:
+		// charges (save-card path); a cnon: nonce charge (one-off) needs none
+		// (R6).
+		CustomerID:        savedCardCustomerID,
 		IdempotencyKey:    req.IdempotencyKey,
 		Note:              "Gift Card Purchase",
 		BuyerEmail:        buyerEmail,

backend/handlers/payments/refunds.go

@@ -106,7 +106,7 @@ func CalculateRefundForCancellation(
 // manual guard read precedes the cancellation's commit). Locks are acquired in
 // ascending payment_id order (matching processChargeGroup) to avoid deadlocks,
 // and only for card methods the manual handler can touch.
-func lockCancellationPayments(ctx context.Context, q db.Querier, payments []paymentRow) error {
+func lockCancellationPayments(ctx context.Context, tx pgx.Tx, payments []paymentRow) error {
 	var ids []string
 	for _, p := range payments {
 		if p.PaymentMethod == "online_square" || p.PaymentMethod == "in_person_card" {
@@ -118,7 +118,15 @@ func lockCancellationPayments(ctx context.Context, q db.Querier, payments []paym
 	}
 	sort.Strings(ids)
 	for _, pid := range ids {
-		if _, err := q.Exec(ctx, `SELECT pg_advisory_xact_lock(hashtext('crussell:refund:' || $1))`, pid); err != nil {
+		// Blocking xact lock (NOT the bounded try-lock used elsewhere): this is
+		// the admin-only cancellation path, and the manual RefundPayment handler
+		// can hold the same key across its up-to-30s Square round-trip. A timed
+		// out acquire here would abort the cancellation transaction — the caller
+		// (manage.go) would commit the cancellation with ZERO refund rows and no
+		// sweep retry could ever recover the money. Blocking guarantees the
+		// refund runs; the lock auto-releases at the caller's commit/rollback.
+		// See acquireAdvisoryXactLockBlocking for the full rationale.
+		if err := acquireAdvisoryXactLockBlocking(ctx, tx, "crussell:refund:"+pid); err != nil {
 			return fmt.Errorf("failed to acquire cancellation refund lock for payment %s: %w", pid, err)
 		}
 	}
@@ -749,10 +757,15 @@ func processChargeGroup(ctx context.Context, chargeID string, rows []pendingChar
 
 	locked := 0
 	for _, pid := range paymentIDs {
-		if _, err := pinConn.Exec(ctx, `
-			SELECT pg_advisory_lock(hashtext('crussell:refund:' || $1))
-		`, pid); err != nil {
-			log.Printf("Failed to acquire refund lock for payment %s (charge %s): %v", pid, chargeID, err)
+		// Bounded try-lock (R6) so the sweep never blocks a pool connection
+		// while the manual handler holds the same key across its Square call.
+		ok, lockErr := acquireAdvisoryLock(ctx, pinConn, "crussell:refund:"+pid)
+		if lockErr != nil {
+			log.Printf("Failed to acquire refund lock for payment %s (charge %s): %v", pid, chargeID, lockErr)
+			break
+		}
+		if !ok {
+			log.Printf("Refund lock for payment %s (charge %s) not acquired within bound — a refund is in progress", pid, chargeID)
 			break
 		}
 		locked++
@@ -1083,11 +1096,16 @@ func processManualPaymentGroup(ctx context.Context, paymentID string, rows []man
 		return 0, err
 	}
 	defer pinConn.Release()
-	if _, err := pinConn.Exec(ctx, `
-		SELECT pg_advisory_lock(hashtext('crussell:refund:' || $1))
-	`, paymentID); err != nil {
+	// Bounded try-lock (R6) so the sweep never blocks a pool connection while
+	// the manual handler holds the same key across its Square call.
+	lockOK, err := acquireAdvisoryLock(ctx, pinConn, "crussell:refund:"+paymentID)
+	if err != nil {
 		return 0, err
 	}
+	if !lockOK {
+		log.Printf("Refund lock for payment %s not acquired within bound — a manual refund is in progress; leaving rows pending for the next sweep", paymentID)
+		return 0, nil
+	}
 	defer func() {
 		if _, err := pinConn.Exec(context.Background(), `
 			SELECT pg_advisory_unlock(hashtext('crussell:refund:' || $1))

backend/handlers/payments/till.go

@@ -247,7 +247,9 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) {
 	// constraint after the funding already committed. Mirrors the gift-card
 	// advisory-lock pattern (giftcards.go). Lock is keyed on the idempotency
 	// key so distinct sales are unaffected; falls back to a per-request key
-	// when absent (client-supplied key is always used in practice).
+	// when absent (client-supplied key is always used in practice). Bounded
+	// try-lock (R6) so a contended lock never blocks the pool across the Square
+	// round-trip.
 	lockKey := req.IdempotencyKey
 	if lockKey == "" {
 		lockKey = "till-" + rand.Text()
@@ -259,13 +261,17 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	defer pinConn.Release()
-	if _, err := pinConn.Exec(ctx, `
-		SELECT pg_advisory_lock(hashtext('crussell:till:' || $1))
-	`, lockKey); err != nil {
+	lockOK, err := acquireAdvisoryLock(ctx, pinConn, "crussell:till:"+lockKey)
+	if err != nil {
 		log.Printf("Failed to acquire till-sale serialization lock for %s: %v", lockKey, err)
 		http.Error(w, "internal server error", http.StatusInternalServerError)
 		return
 	}
+	if !lockOK {
+		log.Printf("Till-sale serialization lock for %s not acquired within bound — a sale is already in progress", lockKey)
+		http.Error(w, "Sale in progress, try again", http.StatusConflict)
+		return
+	}
 	defer func() {
 		if _, err := pinConn.Exec(context.Background(), `
 			SELECT pg_advisory_unlock(hashtext('crussell:till:' || $1))
@@ -531,17 +537,45 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) {
 			}
 		}
 
+		// The till path is not user-scoped, so fetch the card's owner along with
+		// the charge details — the owner is needed to lazily provision a Square
+		// customer if the row predates P14 (R6).
+		var cardUserID sql.NullString
 		err = tx.QueryRow(ctx, `
-			SELECT square_card_id, COALESCE(square_customer_id, '')
+			SELECT user_id, COALESCE(square_card_id, ''), COALESCE(square_customer_id, '')
 			FROM user_saved_cards
 			WHERE id = $1 AND deleted_at IS NULL
-		`, *req.UserSavedCardID).Scan(&savedCardSqCardID, &savedCardCustomerID)
+		`, *req.UserSavedCardID).Scan(&cardUserID, &savedCardSqCardID, &savedCardCustomerID)
 		if err != nil {
 			log.Printf("Failed to get saved card details: %v", err)
 			http.Error(w, "Card not found", http.StatusNotFound)
 			return
 		}
 
+		// A ccof: source can NEVER be charged without a CustomerID — Square
+		// rejects the payment. Legacy pre-P14 rows have an empty
+		// square_customer_id; provision + persist for the card's owner BEFORE
+		// charging (R6).
+		if savedCardCustomerID == "" {
+			if !cardUserID.Valid {
+				http.Error(w, "Saved card has no owner and cannot be charged", http.StatusBadRequest)
+				return
+			}
+			provisioned, provErr := service.EnsureSquareCustomer(ctx, cardUserID.String)
+			if provErr != nil {
+				log.Printf("Failed to provision Square customer for saved card %s (user %s): %v", *req.UserSavedCardID, cardUserID.String, provErr)
+				http.Error(w, "Failed to process card", http.StatusInternalServerError)
+				return
+			}
+			savedCardCustomerID = provisioned
+			if _, upErr := db.Conn.Exec(ctx, `
+				UPDATE user_saved_cards SET square_customer_id = $1
+				WHERE id = $2
+			`, provisioned, *req.UserSavedCardID); upErr != nil {
+				log.Printf("Failed to persist Square customer id on saved card %s (non-fatal): %v", *req.UserSavedCardID, upErr)
+			}
+		}
+
 		if req.IdempotencyKey == "" {
 			req.IdempotencyKey = uniqueTillKey()
 		}
@@ -697,29 +731,23 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) {
 			paymentResult, squareErr = SquareClient.CreatePayment(ctx, paymentReq)
 		} else if req.PaymentMethod == "online_square" {
 			// PCI-DSS: raw PANs are never accepted. The admin till must supply a
-			// Square Web Payments nonce (cnon:xxx), tokenized via the Cards API.
+			// Square Web Payments nonce (cnon:xxx).
 			if req.CardToken == "" {
 				log.Printf("online_square till sale missing card_token for gift card %s", giftCardID)
 				http.Error(w, "card_token is required for online_square payment — use a Square Web Payments nonce", http.StatusBadRequest)
 				return
 			}
-			// Tokenize the till card. The reference_id is the synthetic
-			// "till-<giftCardID>" namespace, NOT a real user — this card is
-			// ephemeral (used once for this charge) and is never stored in
-			// user_saved_cards or re-listed. The prefix can't collide with a
-			// real CHAR(12)-hex user ID. No Square customer is provisioned for
-			// it ("" as the customerID): a cnon: nonce charge needs none.
-			cardOnFile, cardErr := SquareClient.CreateCardOnFile(ctx, "till-"+giftCardID, req.CardToken, "")
-			if cardErr != nil {
-				log.Printf("Failed to tokenize card: %v", cardErr)
-				http.Error(w, "Card tokenization failed", http.StatusInternalServerError)
-				return
-			}
-
+			// Charge the cnon: nonce DIRECTLY (R6). The old code tokenized the
+			// till card via CreateCardOnFile under a synthetic "till-<giftCardID>"
+			// reference and charged the resulting ccof: id — but that card was
+			// never saved or re-listed anywhere, so the CreateCardOnFile call was
+			// pure overhead AND would have charged a card-on-file source without a
+			// customer (Square rejects ccof without CustomerID). A cnon: nonce
+			// needs neither card-on-file nor customer.
 			paymentReq := square.CreatePaymentReq{
 				Amount:            penceAmount,
 				Currency:          "GBP",
-				SourceID:          cardOnFile.CardID,
+				SourceID:          req.CardToken,
 				IdempotencyKey:    req.IdempotencyKey,
 				Note:              "Gift Card " + req.Action,
 				BuyerEmail:        buyerEmail,
@@ -854,6 +882,7 @@ func GetTillCheckoutStatus(w http.ResponseWriter, r *http.Request) {
 		// path uses. Two concurrent polls of the same checkout could both run
 		// the UPDATE + VAT (idempotent today, but a double-apply is a latent
 		// bug). Lock on the till-sale id so only one goroutine completes it.
+		// Bounded try-lock (R6) so a contended lock never blocks the pool.
 		pinConn, err := db.Conn.Acquire(r.Context())
 		if err != nil {
 			log.Printf("Failed to acquire connection for till-completion lock: %v", err)
@@ -861,13 +890,17 @@ func GetTillCheckoutStatus(w http.ResponseWriter, r *http.Request) {
 			return
 		}
 		defer pinConn.Release()
-		if _, err := pinConn.Exec(r.Context(), `
-			SELECT pg_advisory_lock(hashtext('crussell:tillcomplete:' || $1))
-		`, tillSaleID); err != nil {
+		lockOK, err := acquireAdvisoryLock(r.Context(), pinConn, "crussell:tillcomplete:"+tillSaleID)
+		if err != nil {
 			log.Printf("Failed to acquire till-completion serialization lock for %s: %v", tillSaleID, err)
 			http.Error(w, "internal server error", http.StatusInternalServerError)
 			return
 		}
+		if !lockOK {
+			log.Printf("Till-completion serialization lock for %s not acquired within bound — a poll is already completing this sale", tillSaleID)
+			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:tillcomplete:' || $1))