Fix gift-card transfer deadlock: lock card rows in deterministic ID order

TransferGiftCard locked source-then-destination in caller-chosen order, so concurrent cross-transfers (A→B and B→A) acquired row locks in opposite orders and deadlocked (SQLSTATE 40P01), aborting one transfer with a generic 500. Both rows are now locked in lexicographically sorted ID order (case-insensitive, matching the 12-hex mixed-case card codes), with the scanned values mapped back to source/destination roles afterward. Adds a deterministic concurrency regression test that holds both row locks on dedicated connections to force the AB-BA cycle, proving the old code deadlocks and the new code serializes cleanly.

backend/handlers/payments/concurrency_test.go

@@ -3,7 +3,9 @@
 package payments
 
 import (
+	"bytes"
 	"context"
+	"encoding/json"
 	"net/http"
 	"net/http/httptest"
 	"sync"
@@ -519,3 +521,157 @@ func TestLoyaltyRedemption_ConcurrentSameBooking_SingleApply(t *testing.T) {
 		_, _ = db.Conn.Exec(pool, `DELETE FROM loyalty_redemptions WHERE user_id = $1 OR applied_to_booking_id = $2`, userID, bookingID)
 	})
 }
+
+// TestTransferGiftCard_ConcurrentCrossTransfer_NoDeadlock proves the gift-card
+// transfer lock ordering (giftcards.go): two concurrent cross-transfers A→B and
+// B→A must both succeed. Locking "source first" (the caller's chosen order)
+// would deadlock — A→B locks A then B while B→A locks B then A — and Postgres
+// aborts one with SQLSTATE 40P01. Locking the lesser ID first makes both
+// transactions acquire the same lock sequence, so neither deadlocks.
+func TestTransferGiftCard_ConcurrentCrossTransfer_NoDeadlock(t *testing.T) {
+	ctx, tx := testutils.SetupTestTx(t)
+
+	adminID, err := fixtures.CreateTestUser(tx)
+	if err != nil {
+		t.Fatalf("failed to create admin: %v", err)
+	}
+	_, _ = tx.Exec(ctx, "UPDATE users SET account_role = 'admin' WHERE id = $1", adminID)
+	token := jwt.GenerateTestToken(adminID, "admin")
+	cleanupConcurrentTestRows(t, context.Background(), adminID, "")
+
+	// Both cards hold ample balance so either transfer direction succeeds no
+	// matter which transaction wins the race (net effect is a wash).
+	var cardAID, cardBID string
+	if err := tx.QueryRow(ctx, `
+		INSERT INTO gift_cards (total_funds_added, amount_remaining, created_by)
+		VALUES (100.00, 100.00, $1) RETURNING id
+	`, adminID).Scan(&cardAID); err != nil {
+		t.Fatalf("failed to insert card A: %v", err)
+	}
+	if err := tx.QueryRow(ctx, `
+		INSERT INTO gift_cards (total_funds_added, amount_remaining, created_by)
+		VALUES (100.00, 100.00, $1) RETURNING id
+	`, adminID).Scan(&cardBID); err != nil {
+		t.Fatalf("failed to insert card B: %v", err)
+	}
+
+	// Commit the setup so both goroutines run at pool level on independent
+	// connections — a shared per-test tx would serialize them on one connection
+	// and mask the deadlock entirely.
+	innerTx := db.TxFromContext(ctx)
+	if innerTx == nil {
+		t.Fatal("no transaction in context")
+	}
+	if err := innerTx.Commit(ctx); err != nil {
+		t.Fatalf("failed to commit setup tx: %v", err)
+	}
+
+	pool := context.Background()
+
+	// Route each request through a chi router so chi.URLParam("from") resolves.
+	r := chi.NewRouter()
+	r.Use(mw.RequireAuth)
+	r.Post("/admin/gift-cards/{from}/transfer", TransferGiftCard)
+
+	transfer := func(from, to string, amount float64) int {
+		reqBody, _ := json.Marshal(map[string]interface{}{
+			"to_card_id": to,
+			"amount":     amount,
+		})
+		req := httptest.NewRequest("POST", "/admin/gift-cards/"+from+"/transfer", bytes.NewReader(reqBody))
+		req.Header.Set("Authorization", "Bearer "+token)
+		req.Header.Set("Content-Type", "application/json")
+		w := httptest.NewRecorder()
+		r.ServeHTTP(w, req)
+		return w.Code
+	}
+
+	var wg sync.WaitGroup
+	startBoth := make(chan struct{})
+	codes := make([]int, 2)
+
+	// Deterministically force the deadlock window instead of hoping two
+	// sub-millisecond transactions interleave: hold each card's row lock on a
+	// dedicated pool connection, launch both cross-transfers (each blocks on
+	// its first lock), then release the held locks one at a time.
+	//
+	// Old "source first" order: G1 (A→B) holds A and queues on B; releasing
+	// holderB grants B to G2 (B→A, queued first) which then queues on A held
+	// by G1 → Postgres aborts one with 40P01 → 500.
+	//
+	// Fixed sorted order: both transactions want the SAME first lock, so only
+	// one ever holds it; the loser waits, the winner proceeds, and both
+	// succeed with no cycle.
+	holderA, err := db.Conn.Acquire(pool)
+	if err != nil {
+		t.Fatalf("failed to acquire holder conn A: %v", err)
+	}
+	defer holderA.Release()
+	if _, err := holderA.Exec(pool, "SELECT amount_remaining FROM gift_cards WHERE id = $1 FOR UPDATE", cardAID); err != nil {
+		t.Fatalf("failed to hold lock on card A: %v", err)
+	}
+
+	holderB, err := db.Conn.Acquire(pool)
+	if err != nil {
+		t.Fatalf("failed to acquire holder conn B: %v", err)
+	}
+	defer holderB.Release()
+	if _, err := holderB.Exec(pool, "SELECT amount_remaining FROM gift_cards WHERE id = $1 FOR UPDATE", cardBID); err != nil {
+		t.Fatalf("failed to hold lock on card B: %v", err)
+	}
+
+	wg.Add(2)
+	go func() {
+		defer wg.Done()
+		<-startBoth
+		codes[0] = transfer(cardAID, cardBID, 10.00)
+	}()
+	go func() {
+		defer wg.Done()
+		<-startBoth
+		codes[1] = transfer(cardBID, cardAID, 10.00)
+	}()
+	close(startBoth)
+
+	// Give both goroutines time to reach their first blocked SELECT.
+	time.Sleep(300 * time.Millisecond)
+
+	// Release A first: the A→B transfer acquires A and queues on B (still
+	// held). Then release B: the B→A transfer was queued on B first, so it
+	// acquires B and queues on A — completing the cycle under old code.
+	if _, err := holderA.Exec(pool, "COMMIT"); err != nil {
+		t.Fatalf("failed to release holder A: %v", err)
+	}
+	time.Sleep(300 * time.Millisecond)
+	if _, err := holderB.Exec(pool, "COMMIT"); err != nil {
+		t.Fatalf("failed to release holder B: %v", err)
+	}
+
+	done := make(chan struct{})
+	go func() {
+		wg.Wait()
+		close(done)
+	}()
+	select {
+	case <-done:
+	case <-time.After(30 * time.Second):
+		t.Fatal("concurrent cross-transfers deadlocked (no response within 30s)")
+	}
+
+	for i, code := range codes {
+		if code != http.StatusOK {
+			t.Errorf("cross-transfer %d expected 200, got %d (deadlock aborted one tx)", i, code)
+		}
+	}
+
+	// Both transfers succeeded, so each card's balance is back to £100.
+	for _, id := range []string{cardAID, cardBID} {
+		var remaining float64
+		if err := db.Conn.QueryRow(pool, `SELECT amount_remaining FROM gift_cards WHERE id = $1`, id).Scan(&remaining); err != nil {
+			t.Fatalf("failed to query balance for card %s: %v", id, err)
+		}
+		if remaining != 100.00 {
+			t.Errorf("card %s expected net balance 100.00, got %.2f", id, remaining)
+		}
+	}
+}

backend/handlers/payments/giftcards.go

@@ -576,7 +576,7 @@ func TransferGiftCard(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	if fromCardID == req.ToCardID {
+	if strings.EqualFold(fromCardID, req.ToCardID) {
 		http.Error(w, "Source and destination cards must be different", http.StatusBadRequest)
 		return
 	}
@@ -601,32 +601,60 @@ func TransferGiftCard(w http.ResponseWriter, r *http.Request) {
 	var fromRedeemedBy, toRedeemedBy sql.NullString
 	var fromRemaining, toRemaining float64
 
-	// Lock both rows FOR UPDATE (source first, deterministic order) so a
+	// Lock both rows FOR UPDATE in a globally deterministic order (lesser ID
-	// concurrent transfer/topup can't interleave a read-then-write on the same
+	// first, then greater) so two concurrent cross-transfers (A→B and B→A)
-	// card — the same check-then-act race RedeemGiftCard and the till path
+	// acquire the locks in the same order and can never deadlock. Locking
-	// already guard against (N-5).
+	// "source first" is only deterministic per-request — the caller picks the
-	err = tx.QueryRow(ctx, "SELECT redeemed_by, amount_remaining FROM gift_cards WHERE id = $1 FOR UPDATE", fromCardID).Scan(&fromRedeemedBy, &fromRemaining)
+	// source, so opposite transfers would deadlock (N-5). IDs are compared
+	// case-insensitively: gift card codes are 12-hex that may arrive mixed-case
+	// from the URL param vs the body, and the same two cards must always sort
+	// the same way for every concurrent transaction.
+	lockFirstID, lockSecondID := fromCardID, req.ToCardID
+	if strings.ToLower(lockFirstID) > strings.ToLower(lockSecondID) {
+		lockFirstID, lockSecondID = lockSecondID, lockFirstID
+	}
+	lockFirstIsSource := strings.EqualFold(lockFirstID, fromCardID)
+
+	err = tx.QueryRow(ctx, "SELECT redeemed_by, amount_remaining FROM gift_cards WHERE id = $1 FOR UPDATE", lockFirstID).Scan(&fromRedeemedBy, &fromRemaining)
 	if err != nil {
 		if errors.Is(err, pgx.ErrNoRows) {
+			if lockFirstIsSource {
 				http.Error(w, "Source gift card not found", http.StatusNotFound)
+			} else {
+				http.Error(w, "Destination gift card not found", http.StatusNotFound)
+			}
 			return
 		}
-		log.Printf("Failed to check source gift card: %v", err)
+		log.Printf("Failed to check gift card: %v", err)
 		http.Error(w, "internal server error", http.StatusInternalServerError)
 		return
 	}
 
-	err = tx.QueryRow(ctx, "SELECT redeemed_by, amount_remaining FROM gift_cards WHERE id = $1 FOR UPDATE", req.ToCardID).Scan(&toRedeemedBy, &toRemaining)
+	// Lock the second row in the same order so both concurrent transactions
+	// hold the same lock sequence and can never deadlock.
+	err = tx.QueryRow(ctx, "SELECT redeemed_by, amount_remaining FROM gift_cards WHERE id = $1 FOR UPDATE", lockSecondID).Scan(&toRedeemedBy, &toRemaining)
 	if err != nil {
 		if errors.Is(err, pgx.ErrNoRows) {
+			if lockFirstIsSource {
 				http.Error(w, "Destination gift card not found", http.StatusNotFound)
+			} else {
+				http.Error(w, "Source gift card not found", http.StatusNotFound)
+			}
 			return
 		}
-		log.Printf("Failed to check destination gift card: %v", err)
+		log.Printf("Failed to check gift card: %v", err)
 		http.Error(w, "internal server error", http.StatusInternalServerError)
 		return
 	}
 
+	// The lock order may differ from the source/destination roles when the
+	// destination ID sorts before the source ID; swap the scanned values back
+	// so the business logic below always treats fromCardID as the source.
+	if !lockFirstIsSource {
+		fromRedeemedBy, toRedeemedBy = toRedeemedBy, fromRedeemedBy
+		fromRemaining, toRemaining = toRemaining, fromRemaining
+	}
+
 	if fromRedeemedBy.Valid || toRedeemedBy.Valid {
 		http.Error(w, "Cannot transfer balance to/from cards redeemed to accounts", http.StatusBadRequest)
 		return