From 8416f033d602d8825f0b2c745ecd19f7bf697e57 Mon Sep 17 00:00:00 2001 From: Stephen Adamson Date: Mon, 3 Aug 2026 18:58:12 +0100 Subject: [PATCH] Fix gift-card transfer deadlock: lock card rows in deterministic ID order MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 156 ++++++++++++++++++ backend/handlers/payments/giftcards.go | 50 ++++-- 2 files changed, 195 insertions(+), 11 deletions(-) diff --git a/backend/handlers/payments/concurrency_test.go b/backend/handlers/payments/concurrency_test.go index 0abfeeb..3902de5 100644 --- a/backend/handlers/payments/concurrency_test.go +++ b/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) + } + } +} diff --git a/backend/handlers/payments/giftcards.go b/backend/handlers/payments/giftcards.go index a447590..93991a7 100644 --- a/backend/handlers/payments/giftcards.go +++ b/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 - // concurrent transfer/topup can't interleave a read-then-write on the same - // card — the same check-then-act race RedeemGiftCard and the till path - // already guard against (N-5). - err = tx.QueryRow(ctx, "SELECT redeemed_by, amount_remaining FROM gift_cards WHERE id = $1 FOR UPDATE", fromCardID).Scan(&fromRedeemedBy, &fromRemaining) + // Lock both rows FOR UPDATE in a globally deterministic order (lesser ID + // first, then greater) so two concurrent cross-transfers (A→B and B→A) + // acquire the locks in the same order and can never deadlock. Locking + // "source first" is only deterministic per-request — the caller picks the + // 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) { - http.Error(w, "Source gift card not found", http.StatusNotFound) + 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) { - http.Error(w, "Destination gift card not found", http.StatusNotFound) + 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