Fix review findings: BuyGiftCard concurrency lock, amount guards, NULL scan, mock dedup, docs

N1 (HIGH) — BuyGiftCard concurrent same-key retry could double-issue gift
cards (2× value for 1 charge). Added pg_advisory_lock on the idempotency key
(mirroring the tip pattern) acquired before the idempotency check, so
concurrent same-key retries serialize and only one executes gift-card
creation.

N2 — Amount-equality guards in both reuse branches (CreateTipPayment and
BuyGiftCard). A same-key retry with a different amount now returns 400
instead of silently mutating the pending record's books/VAT/refund caps.

N3 — test coverage:
- TestBuyGiftCard_RetryPending_ReattemptsCharge: pending record + same-key
  retry re-attempts, reuses the record (count=1), completes, and issues the
  gift card exactly once.
- TestCreateCheckoutHTTP_DeviceOptionsWireShape: httptest.Server asserts
  device_id is under checkout.device_options (not top-level). Extracted
  createCheckoutHTTPWithClient for injectable base URL.
- MockClient.CreatePayment now dedups on idempotency key (paymentByKey map),
  matching real Square behaviour.

N4 — Corrected the savepoint comments in handlers.go and giftcards.go: the
savepoint only exists in the test harness; in production db.Conn.Begin is a
plain tx and the status UPDATE runs on a separate pooled connection. Commit
is a harmless no-op in prod but required in tests.

Bonus bug fixed: CheckIdempotencyByKey scanned NULL booking_id/gift_card_id
(gift-card purchases) into plain string, failing with 'cannot scan NULL'.
Now uses sql.NullString.

Docs: Technical Manual.md:53 and Feature Catalog.md (2.1, 2.5) corrected —
no longer claim Web Payments SDK is live; new-card entry is documented as
pending P11, saved-card flow works via ccof tokens, dev mock rejects raw PANs.

backend/handlers/payments/giftcards.go

@@ -1,6 +1,7 @@
 package payments
 
 import (
+	"context"
 	"database/sql"
 	"encoding/json"
 	"errors"
@@ -891,6 +892,37 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) {
 
 	paymentService := NewPaymentService()
 
+	// Serialize gift-card purchase attempts on the idempotency key to prevent
+	// concurrent same-key retries from both reusing a pending record and both
+	// 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.
+	lockKey := req.IdempotencyKey
+	if lockKey == "" {
+		lockKey = userID
+	}
+	pinConn, err := db.Conn.Acquire(ctx)
+	if err != nil {
+		log.Printf("Failed to acquire connection for gift-card lock: %v", err)
+		http.Error(w, "internal server error", http.StatusInternalServerError)
+		return
+	}
+	defer pinConn.Release()
+	if _, err := pinConn.Exec(ctx, `
+		SELECT pg_advisory_lock(hashtext('crussell:giftcard:' || $1))
+	`, lockKey); 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
+	}
+	defer func() {
+		if _, err := pinConn.Exec(context.Background(), `
+			SELECT pg_advisory_unlock(hashtext('crussell:giftcard:' || $1))
+		`, lockKey); err != nil {
+			log.Printf("Failed to release gift-card serialization lock for %s: %v", lockKey, err)
+		}
+	}()
+
 	// Idempotency: only short-circuit when the existing record is 'completed'.
 	// A 'pending' record means the previous Square call failed — returning it
 	// as 200 would show a success without ever charging. Re-attempt below with
@@ -909,6 +941,14 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) {
 				return
 			}
 			if existing.Status == "pending" {
+				// Guard the amount: a retry with a different amount must not
+				// reuse the pending record (gift card would be issued at the
+				// new amount against the old charge record).
+				if int64(existing.Amount*100) != req.Amount {
+					log.Printf("Gift card retry amount mismatch: pending record %s has %.2f, request has %d pence", existing.ID, existing.Amount, req.Amount)
+					http.Error(w, "Amount does not match the pending gift card payment", http.StatusBadRequest)
+					return
+				}
 				reusePendingID = existing.ID
 				log.Printf("[PAYMENTS] Reusing pending payment %s for idempotent gift-card retry (key %s)", existing.ID, req.IdempotencyKey)
 			}
@@ -1015,10 +1055,11 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) {
 		}
 	}
 
-	// Commit the (nested/savepoint) transaction in the reuse path too. It is
-	// empty, but committing releases the savepoint so the deferred rollback
-	// becomes a no-op — otherwise that rollback would undo the status UPDATE
-	// executed later on the same connection.
+	// Commit in the reuse path too. No rows were written, but the commit is
+	// required in the test harness: there the context carries an outer test tx,
+	// so Begin creates a nested savepoint whose deferred rollback would
+	// otherwise undo the status UPDATE executed later on the same connection.
+	// In production Begin is a plain tx and this commit is a harmless no-op.
 	if reusePendingID != "" {
 		if err := tx.Commit(ctx); err != nil {
 			log.Printf("Failed to commit buy transaction (reuse): %v", err)

backend/handlers/payments/giftcards_test.go

@@ -1189,6 +1189,81 @@ func TestBuyGiftCard_Idempotency(t *testing.T) {
 	}
 }
 
+// TestBuyGiftCard_RetryPending_ReattemptsCharge verifies that a same-key retry
+// after a failed Square call (record left 'pending') re-attempts the charge and
+// completes — it must NOT return the stale pending record as a false success,
+// and must NOT issue the gift card twice.
+func TestBuyGiftCard_RetryPending_ReattemptsCharge(t *testing.T) {
+	t.Parallel()
+	ctx, tx := testutils.SetupTestTx(t)
+
+	userID, err := fixtures.CreateTestUser(tx)
+	if err != nil {
+		t.Fatalf("failed to create user: %v", err)
+	}
+
+	token := jwt.GenerateTestToken(userID, "verified_email")
+
+	idempotencyKey := "buy-gc-pending-retry"
+
+	// Seed a PENDING payment record with the same key — simulates a prior
+	// attempt where the Square call failed.
+	_, err = tx.Exec(ctx, `
+		INSERT INTO payments (payment_type, payment_method, status, amount, idempotency_key, created_at, updated_at, created_by)
+		VALUES ('full', 'online_square', 'pending', 20.00, $1, NOW(), NOW(), $2)
+	`, idempotencyKey, userID)
+	if err != nil {
+		t.Fatalf("failed to seed pending payment: %v", err)
+	}
+
+	reqBody := map[string]interface{}{
+		"amount":          2000,
+		"recipient_type":  "self",
+		"new_card_token":  "cnon:card-nonce-ok",
+		"idempotency_key": idempotencyKey,
+	}
+
+	body1, _ := json.Marshal(reqBody)
+	req1 := httptest.NewRequest("POST", "/api/user/giftcards/buy", bytes.NewBuffer(body1))
+	req1.Header.Set("Authorization", "Bearer "+token)
+	req1.Header.Set("Content-Type", "application/json")
+	req1 = req1.WithContext(db.ContextWithTx(req1.Context(), tx.(pgx.Tx)))
+
+	w1 := httptest.NewRecorder()
+	r1 := chi.NewRouter()
+	r1.Use(mw.RequireAuth)
+	r1.Post("/api/user/giftcards/buy", BuyGiftCard)
+	r1.ServeHTTP(w1, req1)
+
+	if w1.Code != http.StatusOK && w1.Code != http.StatusCreated {
+		t.Fatalf("buy request: expected 200/201, got %d. Body: %s", w1.Code, w1.Body.String())
+	}
+
+	// Exactly one payment record for the key, now completed.
+	var payCount int
+	var payStatus string
+	err = tx.QueryRow(ctx, "SELECT COUNT(*), MAX(status) FROM payments WHERE idempotency_key = $1", idempotencyKey).Scan(&payCount, &payStatus)
+	if err != nil {
+		t.Fatalf("failed to query payments: %v", err)
+	}
+	if payCount != 1 {
+		t.Errorf("expected 1 payment record (reuse, not duplicate), got %d", payCount)
+	}
+	if payStatus != "completed" {
+		t.Errorf("expected pending record to be completed after retry, got %s", payStatus)
+	}
+
+	// Exactly one gift card issued for the single charge.
+	var cardCount int
+	err = tx.QueryRow(ctx, "SELECT COUNT(*) FROM gift_cards WHERE redeemed_by = $1", userID).Scan(&cardCount)
+	if err != nil {
+		t.Fatalf("failed to query gift cards: %v", err)
+	}
+	if cardCount != 1 {
+		t.Errorf("expected 1 gift card issued, got %d", cardCount)
+	}
+}
+
 // TestAdminCreateGiftCard_ExpiryDateIsNull verifies that gift cards created via
 // CreateGiftCard no longer have expiry_date set (rolling 24-month expiry via last_used_at).
 func TestAdminCreateGiftCard_ExpiryDateIsNull(t *testing.T) {

backend/handlers/payments/handlers.go

@@ -1937,6 +1937,14 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) {
 		return
 	case err == nil && existingStatus.String == "pending":
 		// Previous Square call failed — reuse the pending record and re-attempt.
+		// Guard the amount: a retry with a different amount must not mutate the
+		// original record (books, VAT, refund caps) or silently charge the new
+		// amount against the old record.
+		if int64(existingAmount.Float64*100) != req.Amount {
+			log.Printf("Tip retry amount mismatch: pending record %s has %d pence, request has %d pence", existingID.String, int64(existingAmount.Float64*100), req.Amount)
+			http.Error(w, "Amount does not match the pending tip payment", http.StatusBadRequest)
+			return
+		}
 		paymentID = existingID.String
 		reusePendingRecord = true
 	case err != nil && !errors.Is(err, pgx.ErrNoRows):
@@ -1967,10 +1975,12 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) {
 		ApplyVATToBookingPayment(r.Context(), tx, paymentID)
 	}
 
-	// Always commit the (possibly nested/savepoint) transaction. In the reuse
-	// path the savepoint is empty, but committing it releases the savepoint so
-	// the deferred rollback becomes a no-op — otherwise that rollback would
-	// undo the status UPDATE executed later on the same connection.
+	// Always commit the transaction. In the reuse path no rows were written,
+	// but the commit is required in the test harness: there the context carries
+	// an outer test tx, so Begin creates a nested savepoint whose deferred
+	// rollback would otherwise undo the status UPDATE executed later on the
+	// same connection. In production Begin is a plain tx and this commit is a
+	// harmless no-op that keeps both paths identical.
 	if err := tx.Commit(r.Context()); err != nil {
 		log.Printf("Failed to commit transaction: %v", err)
 		http.Error(w, "internal server error", http.StatusInternalServerError)

backend/handlers/payments/service.go

@@ -5,6 +5,7 @@ import (
 	"crussell/clock"
 	"crussell/db"
 	"crussell/internal/square"
+	"database/sql"
 	"errors"
 	"fmt"
 	"log"
@@ -296,6 +297,9 @@ func (s *PaymentService) CheckIdempotency(ctx context.Context, bookingID, idempo
 
 func (s *PaymentService) CheckIdempotencyByKey(ctx context.Context, idempotencyKey string) (*PaymentRecord, error) {
 	var p PaymentRecord
+	// booking_id and gift_card_id are NULL for gift-card purchases — scan into
+	// NullString to avoid "cannot scan NULL into *string".
+	var bookingID, giftCardID sql.NullString
 	err := db.Conn.QueryRow(ctx, `
 		SELECT id, booking_id, payment_type, payment_method, vendor_code, invoice_number,
 		       status, amount, is_vat_applicable, vat_rate, vat_amount, net_amount,
@@ -304,11 +308,15 @@ func (s *PaymentService) CheckIdempotencyByKey(ctx context.Context, idempotencyK
 		FROM payments
 		WHERE idempotency_key = $1
 	`, idempotencyKey).Scan(
-		&p.ID, &p.BookingID, &p.PaymentType, &p.PaymentMethod, &p.VendorCode, &p.InvoiceNumber,
+		&p.ID, &bookingID, &p.PaymentType, &p.PaymentMethod, &p.VendorCode, &p.InvoiceNumber,
 		&p.Status, &p.Amount, &p.IsVATApplicable, &p.VATRate, &p.VATAmount, &p.NetAmount,
 		&p.UserSavedCardID, &p.SquarePaymentID, &p.IdempotencyKey, &p.Fees,
-		&p.CreatedAt, &p.UpdatedAt, &p.CreatedBy, &p.GiftCardID,
+		&p.CreatedAt, &p.UpdatedAt, &p.CreatedBy, &giftCardID,
 	)
+	p.BookingID = bookingID.String
+	if giftCardID.Valid {
+		p.GiftCardID = &giftCardID.String
+	}
 
 	if err != nil {
 		if errors.Is(err, pgx.ErrNoRows) {

backend/internal/square/square_dev.go

@@ -28,6 +28,7 @@ type MockClient struct {
 	cards         map[string]map[string]*CardOnFile
 	checkouts     map[string]*CheckoutResult
 	payments      map[string]*PaymentResult
+	paymentByKey  map[string]*PaymentResult
 	refunds       map[string]*RefundResult
 	completed     map[string]*PaymentResult
 	HoldCheckouts bool
@@ -76,6 +77,7 @@ func NewDevClient() SquareClient {
 		cards:        make(map[string]map[string]*CardOnFile),
 		checkouts:    make(map[string]*CheckoutResult),
 		payments:     make(map[string]*PaymentResult),
+		paymentByKey: make(map[string]*PaymentResult),
 		refunds:      make(map[string]*RefundResult),
 		completed:    make(map[string]*PaymentResult),
 	}
@@ -106,6 +108,17 @@ func (m *MockClient) CreatePayment(ctx context.Context, req CreatePaymentReq) (*
 	m.mu.Lock()
 	defer m.mu.Unlock()
 
+	// Real Square dedups on idempotency key: a retry with the same key returns
+	// the original payment rather than creating a second charge. The mock
+	// mirrors this so dev/testing behaves like production (also why the tip
+	// retry regression test can rely on the mock).
+	if req.IdempotencyKey != "" {
+		if existing, ok := m.paymentByKey[req.IdempotencyKey]; ok {
+			log.Printf("[SQUARE-MOCK] CreatePayment dedup hit: key=%s → id=%s", req.IdempotencyKey, existing.ID)
+			return existing, nil
+		}
+	}
+
 	now := clock.Now().UTC()
 
 	status := "COMPLETED"
@@ -162,6 +175,9 @@ func (m *MockClient) CreatePayment(ctx context.Context, req CreatePaymentReq) (*
 	}
 	m.payments[paymentID] = result
 	m.payments[result.SquarePayID] = result
+	if req.IdempotencyKey != "" {
+		m.paymentByKey[req.IdempotencyKey] = result
+	}
 	log.Printf("[SQUARE-MOCK] Payment created: id=%s, status=%s, amount=%d, fees=%d", paymentID, status, amount, fees)
 	return result, nil
 }

backend/internal/square/square_http_client.go

@@ -297,7 +297,10 @@ func createPaymentHTTP(ctx context.Context, req CreatePaymentReq) (*PaymentResul
 }
 
 func createCheckoutHTTP(ctx context.Context, req CreateCheckoutReq) (*CheckoutResult, error) {
-	hc := newHTTPClient()
+	return createCheckoutHTTPWithClient(ctx, req, newHTTPClient())
+}
+
+func createCheckoutHTTPWithClient(ctx context.Context, req CreateCheckoutReq, hc *httpClient) (*CheckoutResult, error) {
 	body := sqTerminalCheckoutRequest{
 		IdempotencyKey: req.IdempotencyKey,
 		Checkout: sqTerminalCheckoutPayload{

backend/internal/square/square_http_client_test.go

@@ -2,7 +2,13 @@
 
 package square
 
-import "testing"
+import (
+	"context"
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+)
 
 func TestPaymentFromSquare_ElseBranch_SurfacesBrandWithoutCardID(t *testing.T) {
 	p := &sqPayment{
@@ -43,3 +49,54 @@ func TestPaymentFromSquare_NilCardDetails(t *testing.T) {
 		t.Errorf("expected amount 2500, got %d", result.Amount)
 	}
 }
+
+// TestCreateCheckoutHTTP_DeviceOptionsWireShape verifies the terminal checkout
+// request puts device_id inside checkout.device_options (Square's required
+// shape), not at the top level. A top-level device_id is rejected with 400 by
+// Square's real API.
+func TestCreateCheckoutHTTP_DeviceOptionsWireShape(t *testing.T) {
+	var captured map[string]any
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.Method != http.MethodPost {
+			t.Errorf("expected POST, got %s", r.Method)
+		}
+		if r.URL.Path != "/v2/terminals/checkouts" {
+			t.Errorf("expected /v2/terminals/checkouts, got %s", r.URL.Path)
+		}
+		if err := json.NewDecoder(r.Body).Decode(&captured); err != nil {
+			t.Errorf("failed to decode request body: %v", err)
+		}
+		w.Header().Set("Content-Type", "application/json")
+		_, _ = w.Write([]byte(`{"checkout":{"id":"chk_test","status":"PENDING","amount_money":{"amount":5000,"currency":"GBP"},"created_at":"2026-07-31T00:00:00Z","updated_at":"2026-07-31T00:00:00Z"}}`))
+	}))
+	defer srv.Close()
+
+	hc := &httpClient{baseURL: srv.URL, token: "test-token", http: srv.Client()}
+
+	_, err := createCheckoutHTTPWithClient(context.Background(), CreateCheckoutReq{
+		Amount:         5000,
+		Currency:       "GBP",
+		IdempotencyKey: "ik-1",
+		DeviceID:       "dvc_test",
+	}, hc)
+	if err != nil {
+		t.Fatalf("createCheckoutHTTP failed: %v", err)
+	}
+
+	checkout, ok := captured["checkout"].(map[string]any)
+	if !ok {
+		t.Fatalf("expected checkout object in body, got %v", captured)
+	}
+	// device_id must NOT be at the top level
+	if _, hasTopLevel := captured["device_id"]; hasTopLevel {
+		t.Errorf("device_id must not be top-level in terminal checkout request: %v", captured)
+	}
+	// device_id must live under checkout.device_options
+	devOpts, ok := checkout["device_options"].(map[string]any)
+	if !ok {
+		t.Fatalf("expected checkout.device_options in body, got %v", checkout)
+	}
+	if devOpts["device_id"] != "dvc_test" {
+		t.Errorf("expected checkout.device_options.device_id = dvc_test, got %v", devOpts)
+	}
+}

obsidian/Crussell/Feature Catalog.md

@@ -162,8 +162,8 @@ Multi-method payment system accepting Square (card terminal & online), cash, gif
 
 **Related:** [[Booking System|1. Booking System]] (deposits), [[Gift Cards|4. Gift Cards]] (pay by gift card), [[Admin Dashboard|5. Admin Dashboard]] (till purchases)
 
-### 2.1 Online Card Payment (Square Web Payments SDK)
-**What it does:** Customers pay online using a credit/debit card via Square's Web Payments SDK. Used for deposits, full payments, balance payments, and tips.
+### 2.1 Online Card Payment (Square — saved cards; new-card entry pending P11)
+**What it does:** Customers pay online with a card. Saved-card payments work end-to-end via Square tokenized card IDs (`ccof:`). New-card entry (raw PAN entry in the UI) is a documented dead end until Square Web Payments SDK nonces land — see `plans/p11-square-web-payments-sdk.md`. The dev mock rejects raw PANs to mirror production. Used for deposits, full payments, balance payments, and tips.
 
 **Layman summary:** "Pay online with your card — just like any online shop."
 
@@ -191,7 +191,7 @@ Multi-method payment system accepting Square (card terminal & online), cash, gif
 **Related:** [[Gift Cards|4. Gift Cards]], [[VAT Calculation|2.10 VAT Calculation]]
 
 ### 2.5 Saved Cards
-**What it does:** Customers can save their card details for faster checkout next time. Cards are tokenized via Square (raw card numbers never touch the server). Soft-deleted with 7-year UK retention.
+**What it does:** Customers can save their card details for faster checkout next time. Cards are tokenized via Square (`ccof:` card IDs; the full PAN exists only in Square's vault — our DB stores only the reference + brand/last4/fingerprint). The dev mock mirrors this (raw PANs rejected). Soft-deleted with 7-year UK retention. Note: the "Add Card" UI currently sends raw PAN and is a dead end until P11 (Web Payments SDK nonces) — see `plans/p11-square-web-payments-sdk.md`.
 
 **Layman summary:** "Save your card for next time — one-click payment."
 

obsidian/Crussell/Technical Manual.md

@@ -50,7 +50,7 @@ Backend (:8080)
 |---------|--------|---------|
 | SabreDAV (CardDAV/CalDAV) | Active | Contact sync (profile photos), calendar events |
 | S3/R2 | Active (dev) | Portfolio images (AVIF), profile pictures (WebP) |
-| Square | **Active** | Payment processing — in-person Terminal + online Web Payments SDK. Dev mock (`//go:build dev`) simulates async checkout; prod stub (`//go:build !dev`) connects to live API. |
+| Square | **Active** | Payment processing — in-person Terminal (`CreateTerminalCheckout`) + online card payments (saved cards working; new-card entry pending Web Payments SDK nonces — backlog P11, see `plans/p11-square-web-payments-sdk.md`). Dev mock (`//go:build dev`) mirrors production PCI-DSS behaviour (rejects raw PANs; accepts `cnon:`/`ccof:` tokens); prod client (`!dev`) connects to live API. |
 | SMTP | Not implemented | Email/SMS notifications — backend not wired |
 
 ---