From 9ff591fa4e156cabea2d826558aad9c3c785c016 Mon Sep 17 00:00:00 2001 From: Stephen Adamson Date: Sun, 2 Aug 2026 00:31:04 +0100 Subject: [PATCH] Fix payment review round 2: refund idempotency, pending-resume safety, terminal-completion lock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refund idempotency (P2): - RefundRequest gains an optional client idempotency_key: two DISTINCT equal partial refunds of one payment no longer collide on the amount-derived key (the second was silently swallowed as a dedup) - Extract resumeManualPendingRefund: resumes a pending refund with the row's OWN stored key, so Square's key dedup returns the original refund if the prior attempt completed — never issues a second - (payment, amount) pending fallback: when the exact-key lookup misses (admin reopened the modal, new UUID), resume the matching pending row instead of creating a second pending row the sweep would double-process - 409 in-flight guard: if a pending refund exists for the payment but no same-amount row matches, reject a different-amount refund (money state at Square is unknown — no new refund is safe until it resolves) - Frontend (EditBookingModal): UUID per refund attempt, reused on retry, mirroring the tip flow Terminal completion (P3): - GetCheckoutStatus serializes on pg_advisory_lock('crussell:terminal:' || SquarePayID) on a pinned connection — concurrent polls of the same checkout can no longer both pass the dedup SELECT and race the UNIQUE constraint Card-on-file / doc-only: - Document why CreateCardOnFile is NOT rolled back on payment failure (deterministic sha256 retry returns the same card; deletion breaks it) - Document HasCompletedPayment's deliberate 'tip' exclusion Regression tests: - TestRefund_TwoEqualPartialRefunds_ClientKeyDisambiguates - TestRefund_PendingResume_NewKeyAfterModalReopen (proves stored-key resume) - TestRefund_PendingResume_DifferentAmountRejected (409 + no second row) - TestRefund_GuardCountsPendingRefunds updated: 400 -> 409 (in-flight guard fires first — strictly safer, blocks before any Square attempt) - TestGetCheckoutStatus_ConcurrentPolls_SingleRecord (real two-goroutine race) --- backend/handlers/payments/concurrency_test.go | 111 ++++++++ backend/handlers/payments/handlers.go | 245 +++++++++++++----- backend/handlers/payments/payments_test.go | 224 +++++++++++++++- backend/handlers/payments/service.go | 6 + .../components/admin/EditBookingModal.svelte | 8 +- 5 files changed, 522 insertions(+), 72 deletions(-) diff --git a/backend/handlers/payments/concurrency_test.go b/backend/handlers/payments/concurrency_test.go index 101cfbc..e7fd8c7 100644 --- a/backend/handlers/payments/concurrency_test.go +++ b/backend/handlers/payments/concurrency_test.go @@ -15,6 +15,8 @@ import ( "crussell/testutils" "crussell/testutils/fixtures" "crussell/testutils/jwt" + + "github.com/go-chi/chi/v5" ) // slowCreatePaymentClient delays the Square charge so each handler holds its @@ -308,3 +310,112 @@ func TestBookingPayment_ConcurrentSameKey_SingleRecord(t *testing.T) { t.Errorf("expected exactly 1 payment record, got %d (double-charge!)", payCount) } } + +// completedCheckoutClient forces GetCheckout to return a fixed COMPLETED +// payment for any checkout id, deterministically exercising the terminal +// completion dedup+insert path (the mock's real async goroutine would be +// non-deterministic in a race test). +type completedCheckoutClient struct { + square.SquareClient + result *square.PaymentResult +} + +func (c *completedCheckoutClient) GetCheckout(ctx context.Context, checkoutID string) (*square.PaymentResult, error) { + return c.result, nil +} + +// TestGetCheckoutStatus_ConcurrentPolls_SingleRecord proves the P3 fix: the +// terminal-completion path serializes on a per-payment advisory lock, so two +// concurrent polls of the same completed checkout produce exactly ONE payment +// record. Without the lock, both goroutines pass the dedup SELECT, both INSERT, +// and the second dies on the idempotency_key UNIQUE constraint after the +// customer already paid. +func TestGetCheckoutStatus_ConcurrentPolls_SingleRecord(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create user: %v", err) + } + serviceID, err := fixtures.CreateTestService(tx) + if err != nil { + t.Fatalf("failed to create service: %v", err) + } + start := time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC) + bookingID, err := fixtures.CreateTestBookingAtTime(tx, userID, serviceID, start) + if err != nil { + t.Fatalf("failed to create booking: %v", err) + } + cleanupConcurrentTestRows(t, context.Background(), userID, bookingID) + + 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) + } + + origClient := SquareClient + SquareClient = &completedCheckoutClient{ + SquareClient: square.NewDevClient(), + result: &square.PaymentResult{ + ID: "pay_terminal_race", + Status: "COMPLETED", + Amount: 5000, + Fees: 88, + SquarePayID: "pay_terminal_race", + CardBrand: "VISA", + CardLast4: "4242", + ReceiptURL: "https://receipt.example/pay_terminal_race", + EntryMethod: "EMV", + LocationID: "loc", + ReferenceID: bookingID, + CreatedAt: "2026-07-31T00:00:00Z", + UpdatedAt: "2026-07-31T00:00:00Z", + }, + } + defer func() { SquareClient = origClient }() + + pool := context.Background() + checkoutID := "abcd1234ef56" // 12 hex chars, passes the checkout-id validation + + var wg sync.WaitGroup + startBoth := make(chan struct{}) + recs := make([]*httptest.ResponseRecorder, 2) + + for i := 0; i < 2; i++ { + wg.Add(1) + go func(idx int) { + defer wg.Done() + <-startBoth + req := httptest.NewRequest("GET", "/api/checkout/"+checkoutID+"/status?booking_id="+bookingID, nil) + rctx := chi.NewRouteContext() + rctx.URLParams.Add("checkout_id", checkoutID) + req = req.WithContext(context.WithValue(req.Context(), chi.RouteCtxKey, rctx)) + w := httptest.NewRecorder() + GetCheckoutStatus(w, req) + recs[idx] = w + }(i) + } + close(startBoth) + wg.Wait() + + for i, rec := range recs { + if rec.Code != http.StatusOK { + t.Errorf("request %d expected 200, got %d. body: %s", i, rec.Code, rec.Body.String()) + } + } + + // Exactly one completed terminal payment record for this booking. + var payCount int + err = db.Conn.QueryRow(pool, + `SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND payment_method = 'in_person_card' AND square_payment_id = $2`, + bookingID, "pay_terminal_race").Scan(&payCount) + if err != nil { + t.Fatalf("failed to count terminal payments: %v", err) + } + if payCount != 1 { + t.Errorf("expected exactly 1 terminal payment record, got %d (double-record race!)", payCount) + } +} diff --git a/backend/handlers/payments/handlers.go b/backend/handlers/payments/handlers.go index 97cc2b7..393f714 100644 --- a/backend/handlers/payments/handlers.go +++ b/backend/handlers/payments/handlers.go @@ -45,6 +45,12 @@ type CreateBookingPaymentRequest struct { type RefundRequest struct { Amount int64 `json:"amount"` Reason string `json:"reason"` + // Optional client-generated idempotency key. Two DISTINCT refunds of the + // same amount against the same payment must not collide on the default + // amount-derived key (the dedup lookup would swallow the second refund). + // The frontend sends a UUID generated per refund attempt and reuses it on + // retry, mirroring the tip-flow pattern. + IdempotencyKey string `json:"idempotency_key,omitempty"` } type CreateTipPaymentRequest struct { @@ -646,6 +652,35 @@ func GetCheckoutStatus(w http.ResponseWriter, r *http.Request) { if paymentResult.Status == "COMPLETED" { service := NewPaymentService() + + // Serialize terminal-completion records per Square payment ID. Two + // concurrent polls of the same checkout could otherwise BOTH pass the + // dedup SELECT and BOTH INSERT, with the second dying on the + // idempotency_key UNIQUE constraint after the customer already paid — + // the same double-record race every other payment path guards against. + terminalLockKey := paymentResult.SquarePayID + pinConn, err := db.Conn.Acquire(r.Context()) + if err != nil { + log.Printf("Failed to acquire connection for terminal-completion lock: %v", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + defer pinConn.Release() + if _, err := pinConn.Exec(r.Context(), ` + SELECT pg_advisory_lock(hashtext('crussell:terminal:' || $1)) + `, terminalLockKey); err != nil { + log.Printf("Failed to acquire terminal-completion serialization lock for %s: %v", terminalLockKey, err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + defer func() { + if _, err := pinConn.Exec(context.Background(), ` + SELECT pg_advisory_unlock(hashtext('crussell:terminal:' || $1)) + `, terminalLockKey); err != nil { + log.Printf("Failed to release terminal-completion serialization lock for %s: %v", terminalLockKey, err) + } + }() + // Deterministic idempotency key derived from booking + amount + Square // payment ID. The Square payment ID disambiguates two distinct // equal-amount charges on the same booking, so equal amounts never @@ -957,6 +992,11 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { var savedCardID *string if req.NewCardToken != nil && *req.NewCardToken != "" { + // 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, and Square returns the same card — deleting it would break that + // retry. The orphan is harmless (Square-side only, never charged). cardOnFile, err := SquareClient.CreateCardOnFile(r.Context(), userID, *req.NewCardToken) if err != nil { log.Printf("Failed to create card on file: %v", err) @@ -1667,8 +1707,14 @@ func RefundPayment(w http.ResponseWriter, r *http.Request) { } // Deterministic idempotency key so a same-key retry (network timeout) - // does not create a second Square refund. + // 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. idempotencyKey := paymentID + "-refund-" + strconv.FormatInt(req.Amount, 10) + if req.IdempotencyKey != "" { + idempotencyKey = paymentID + "-refund-" + req.IdempotencyKey + } // Serialize refund attempts per payment to prevent two concurrent refunds // both passing the over-refund guard and both charging Square. Mirrors the @@ -1726,70 +1772,9 @@ func RefundPayment(w http.ResponseWriter, r *http.Request) { case err == nil && existingRefundStatus.String == "pending": // Resume the in-flight refund: the DB row was committed but the Square // call never completed (network timeout, crash, etc.). Retry Square with - // the same idempotency key so Square returns the original refund if one - // exists, never a second one. - resumeAmount := int64(math.Round(existingRefundAmount.Float64 * 100)) - resumeReq := square.RefundPaymentReq{ - PaymentID: *payment.SquarePaymentID, - Amount: resumeAmount, - IdempotencyKey: idempotencyKey, - Reason: req.Reason, - } - resumeResult, resumeErr := SquareClient.RefundPayment(r.Context(), resumeReq) - if resumeErr != nil { - if errors.Is(resumeErr, square.ErrRefundAlreadyProcessed) { - // PAYMENT_ALREADY_REFUNDED — money already moved at Square. - // Resolve the pending row to completed (square_refund_id stays - // NULL) so the guard can never over-refund on top of it. - if _, upErr := db.Conn.Exec(r.Context(), `UPDATE refunds SET status = 'completed' WHERE id = $1`, existingRefundID.String); upErr != nil { - log.Printf("Failed to resolve refund %s completed after PAYMENT_ALREADY_REFUNDED: %v", existingRefundID.String, upErr) - } - log.Printf("Refund %s already processed at Square — marked completed", existingRefundID.String) - if err := json.NewEncoder(w).Encode(RefundResponse{ - ID: existingRefundID.String, - PaymentID: paymentID, - Amount: req.Amount, - Status: "completed", - Reason: req.Reason, - CreatedAt: clock.Now().Format(time.RFC3339), - }); err != nil { - log.Printf("Failed to encode JSON response: %v", err) - } - return - } - if errors.Is(resumeErr, square.ErrRefundDeclined) { - // Definitive rejection — mark failed so it never retries and - // never blocks future refunds. - if _, upErr := db.Conn.Exec(r.Context(), `UPDATE refunds SET status = 'failed' WHERE id = $1`, existingRefundID.String); upErr != nil { - log.Printf("Failed to mark refund %s failed after definitive rejection: %v", existingRefundID.String, upErr) - } - log.Printf("Refund %s definitively declined by Square: %v", existingRefundID.String, resumeErr) - http.Error(w, "Refund failed", http.StatusInternalServerError) - return - } - // Ambiguous error — leave pending for the scheduler to retry. - log.Printf("Failed to resume refund %s (left pending): %v", existingRefundID.String, resumeErr) - http.Error(w, "Refund failed", http.StatusInternalServerError) - return - } - if _, upErr := db.Conn.Exec(r.Context(), - `UPDATE refunds SET status = 'completed', square_refund_id = $1 WHERE id = $2`, - resumeResult.ID, existingRefundID.String, - ); upErr != nil { - log.Printf("CRITICAL: Square refund committed (%s) but DB update for refund %s failed — manual reconciliation required: %v", resumeResult.ID, existingRefundID.String, upErr) - http.Error(w, "internal server error", http.StatusInternalServerError) - return - } - if err := json.NewEncoder(w).Encode(RefundResponse{ - ID: existingRefundID.String, - PaymentID: paymentID, - Amount: req.Amount, - Status: "completed", - Reason: req.Reason, - CreatedAt: clock.Now().Format(time.RFC3339), - }); err != nil { - log.Printf("Failed to encode JSON response: %v", err) - } + // the row's OWN stored idempotency key so Square returns the original + // refund if one exists, never a second one. + resumeManualPendingRefund(w, r, paymentID, payment, existingRefundID.String, existingRefundAmount.Float64, existingRefundReason.String, existingRefundKey.String) return case err == nil && existingRefundStatus.String == "failed": // A failed row with origin='manual' may actually have moved money at @@ -1903,6 +1888,60 @@ func RefundPayment(w http.ResponseWriter, r *http.Request) { return } + // Pending-resume fallback: the exact-key lookup missed, but a pending + // refund for this (payment, amount) may exist from a prior attempt whose + // Square call failed ambiguously. If the admin reopened the refund modal, + // the frontend generated a NEW idempotency key, so the exact-key dedup + // above cannot find the row. A pending row means the prior attempt's money + // state at Square is UNKNOWN — re-issuing with a fresh key would double- + // refund once the sweep processes both pending rows. Resume the existing + // pending row with ITS OWN stored key instead, never creating a second one + // while money state is unknown. Distinct COMPLETED refunds of the same + // amount (the P2 equal-partial case) are untouched — they are not pending. + var pendingResumeID sql.NullString + var pendingResumeAmount sql.NullFloat64 + var pendingResumeReason sql.NullString + var pendingResumeKey sql.NullString + err = db.Conn.QueryRow(r.Context(), ` + SELECT id, amount, reason, idempotency_key FROM refunds + WHERE payment_id = $1 AND amount = $2 AND status = 'pending' + ORDER BY created_at LIMIT 1 + `, paymentID, float64(req.Amount)/100.0).Scan(&pendingResumeID, &pendingResumeAmount, &pendingResumeReason, &pendingResumeKey) + if err == nil { + log.Printf("Refund exact-key lookup missed but found pending row %s for payment %s amount %.2f — resuming with its stored key", pendingResumeID.String, paymentID, pendingResumeAmount.Float64) + resumeManualPendingRefund(w, r, paymentID, payment, pendingResumeID.String, pendingResumeAmount.Float64, pendingResumeReason.String, pendingResumeKey.String) + return + } + if !errors.Is(err, pgx.ErrNoRows) { + log.Printf("Failed to check pending-refund fallback: %v", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + + // Different-amount pending guard: no pending row matches this amount, but a + // pending refund for a DIFFERENT amount may still be in flight. Creating a + // second pending row would let the sweep process both (e.g. pending £20, + // retry £30 on a £50 payment → £50 moves when the admin intended £30). A + // pending row means the payment's money state at Square is unknown, so any + // new refund of any amount is unsafe until it resolves. Reject with 409 — + // the same policy as the tip-flow amount-mismatch guard. + var anyPendingID string + err = db.Conn.QueryRow(r.Context(), ` + SELECT id FROM refunds + WHERE payment_id = $1 AND status = 'pending' + LIMIT 1 + `, paymentID).Scan(&anyPendingID) + if err == nil { + log.Printf("Refund %s rejected: payment %s has an in-flight pending refund (row %s) for a different amount — refusing a second pending row", req.IdempotencyKey, paymentID, anyPendingID) + http.Error(w, "A refund is already being processed for this payment — please wait for it to complete", http.StatusConflict) + return + } + if !errors.Is(err, pgx.ErrNoRows) { + log.Printf("Failed to check in-flight pending refund: %v", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + // Over-refund guard (inside the lock so concurrent refunds can't both pass). // GetAlreadyRefundedAmount counts completed AND pending refunds. alreadyRefunded, err := service.GetAlreadyRefundedAmount(r.Context(), paymentID) @@ -2029,6 +2068,77 @@ func RefundPayment(w http.ResponseWriter, r *http.Request) { } } +// resumeManualPendingRefund retries Square for a pending manual refund using +// the row's OWN stored idempotency key (never a fresh one), then resolves the +// row. Called from RefundPayment's exact-key dedup and the (payment, amount) +// 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. +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, + Reason: refundReason, + } + resumeResult, resumeErr := SquareClient.RefundPayment(r.Context(), resumeReq) + if resumeErr != nil { + if errors.Is(resumeErr, square.ErrRefundAlreadyProcessed) { + // PAYMENT_ALREADY_REFUNDED — money already moved at Square. + // Resolve the pending row to completed (square_refund_id stays + // NULL) so the guard can never over-refund on top of it. + if _, upErr := db.Conn.Exec(r.Context(), `UPDATE refunds SET status = 'completed' WHERE id = $1`, refundID); upErr != nil { + log.Printf("Failed to resolve refund %s completed after PAYMENT_ALREADY_REFUNDED: %v", refundID, upErr) + } + log.Printf("Refund %s already processed at Square — marked completed", refundID) + if err := json.NewEncoder(w).Encode(RefundResponse{ + ID: refundID, + PaymentID: paymentID, + Amount: resumeAmount, + Status: "completed", + Reason: refundReason, + CreatedAt: clock.Now().Format(time.RFC3339), + }); err != nil { + log.Printf("Failed to encode JSON response: %v", err) + } + return + } + if errors.Is(resumeErr, square.ErrRefundDeclined) { + // Definitive rejection — mark failed so it never retries and + // never blocks future refunds. + if _, upErr := db.Conn.Exec(r.Context(), `UPDATE refunds SET status = 'failed' WHERE id = $1`, refundID); upErr != nil { + log.Printf("Failed to mark refund %s failed after definitive rejection: %v", refundID, upErr) + } + log.Printf("Refund %s definitively declined by Square: %v", refundID, resumeErr) + http.Error(w, "Refund failed", http.StatusInternalServerError) + return + } + // Ambiguous error — leave pending for the scheduler to retry. + log.Printf("Failed to resume refund %s (left pending): %v", refundID, resumeErr) + http.Error(w, "Refund failed", http.StatusInternalServerError) + return + } + if _, upErr := db.Conn.Exec(r.Context(), + `UPDATE refunds SET status = 'completed', square_refund_id = $1 WHERE id = $2`, + resumeResult.ID, refundID, + ); upErr != nil { + log.Printf("CRITICAL: Square refund committed (%s) but DB update for refund %s failed — manual reconciliation required: %v", resumeResult.ID, refundID, upErr) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + if err := json.NewEncoder(w).Encode(RefundResponse{ + ID: refundID, + PaymentID: paymentID, + Amount: resumeAmount, + Status: "completed", + Reason: refundReason, + CreatedAt: clock.Now().Format(time.RFC3339), + }); err != nil { + log.Printf("Failed to encode JSON response: %v", err) + } +} + func CreateTipPayment(w http.ResponseWriter, r *http.Request) { bookingID := chi.URLParam(r, "id") if bookingID == "" || !validators.IsValidID(bookingID) { @@ -2111,6 +2221,11 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { var savedCardID *string if req.NewCardToken != nil && *req.NewCardToken != "" { + // 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, and Square returns the same card — deleting it would break that + // retry. The orphan is harmless (Square-side only, never charged). cardOnFile, err := SquareClient.CreateCardOnFile(r.Context(), userID, *req.NewCardToken) if err != nil { log.Printf("Failed to create card on file: %v", err) diff --git a/backend/handlers/payments/payments_test.go b/backend/handlers/payments/payments_test.go index f48e2a0..fcde487 100644 --- a/backend/handlers/payments/payments_test.go +++ b/backend/handlers/payments/payments_test.go @@ -650,6 +650,215 @@ func TestRefund_SameKeyRetry_Dedups(t *testing.T) { } } +// TestRefund_TwoEqualPartialRefunds_ClientKeyDisambiguates verifies the P2 fix: +// two DISTINCT partial refunds of the same amount against the same payment +// must both complete. With only the amount-derived key (paymentID-amount) the +// second would collide with the first and be silently swallowed as a dedup. +// A client-supplied idempotency key per attempt disambiguates them, while a +// same-key retry still dedups. +func TestRefund_TwoEqualPartialRefunds_ClientKeyDisambiguates(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + _, bookingID, _ := setupTestData(t, ctx, tx) + + adminID, err := fixtures.CreateTestAdminUser(tx) + if err != nil { + t.Fatalf("failed to create admin user: %v", err) + } + adminToken := jwt.GenerateTestToken(adminID, "admin") + + paymentID, err := fixtures.CreateTestPayment(tx, bookingID, 5000.00, "in_person_card", "full", "completed") + if err != nil { + t.Fatalf("failed to create payment: %v", err) + } + _, err = tx.Exec(ctx, "UPDATE payments SET square_payment_id = 'sqp_test_equal_refunds' WHERE id = $1", paymentID) + if err != nil { + t.Fatalf("failed to update payment: %v", err) + } + + handler := RefundPayment + // Two distinct £20 partial refunds of the same £50 payment — the exact + // case that collided on the amount-derived key before the fix. + refund1 := RefundRequest{Amount: 2000, Reason: "partial one", IdempotencyKey: "refund-uuid-1"} + w1 := makePaymentRequest(handler, "POST", "/api/admin/payments/"+paymentID+"/refund", refund1, adminToken, ctx) + if w1.Code != http.StatusOK { + t.Fatalf("first refund: expected 200, got %d. body: %s", w1.Code, w1.Body.String()) + } + + refund2 := RefundRequest{Amount: 2000, Reason: "partial two", IdempotencyKey: "refund-uuid-2"} + w2 := makePaymentRequest(handler, "POST", "/api/admin/payments/"+paymentID+"/refund", refund2, adminToken, ctx) + if w2.Code != http.StatusOK { + t.Fatalf("second equal partial refund: expected 200, got %d. body: %s", w2.Code, w2.Body.String()) + } + + // Both refunds must exist as separate completed rows — the second was NOT + // swallowed by the first's dedup lookup. + var completedCount int + err = tx.QueryRow(ctx, `SELECT COUNT(*) FROM refunds WHERE payment_id = $1 AND status = 'completed'`, paymentID).Scan(&completedCount) + if err != nil { + t.Fatalf("failed to query refunds: %v", err) + } + if completedCount != 2 { + t.Errorf("expected 2 completed refund rows, got %d (second was swallowed!)", completedCount) + } + + // Same-key retry of refund1 must STILL dedup (retry semantics preserved). + w3 := makePaymentRequest(handler, "POST", "/api/admin/payments/"+paymentID+"/refund", refund1, adminToken, ctx) + if w3.Code != http.StatusOK { + t.Fatalf("retry of first refund: expected 200, got %d. body: %s", w3.Code, w3.Body.String()) + } + var totalCount int + err = tx.QueryRow(ctx, `SELECT COUNT(*) FROM refunds WHERE payment_id = $1 AND status = 'completed'`, paymentID).Scan(&totalCount) + if err != nil { + t.Fatalf("failed to query refunds after retry: %v", err) + } + if totalCount != 2 { + t.Errorf("expected still 2 completed refund rows after same-key retry, got %d", totalCount) + } +} + +// TestRefund_PendingResume_NewKeyAfterModalReopen verifies the P2 pending-resume +// regression fix: when a refund attempt left a PENDING row (Square call failed +// ambiguously) and the admin reopens the modal — generating a NEW idempotency +// key — the retry must RESUME the pending row with its stored key, not create +// a second pending row. Without the fallback, the sweep would process both +// pending rows and move twice the intended money. +func TestRefund_PendingResume_NewKeyAfterModalReopen(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + _, bookingID, _ := setupTestData(t, ctx, tx) + + adminID, err := fixtures.CreateTestAdminUser(tx) + if err != nil { + t.Fatalf("failed to create admin user: %v", err) + } + adminToken := jwt.GenerateTestToken(adminID, "admin") + + paymentID, err := fixtures.CreateTestPayment(tx, bookingID, 5000.00, "in_person_card", "full", "completed") + if err != nil { + t.Fatalf("failed to create payment: %v", err) + } + _, err = tx.Exec(ctx, "UPDATE payments SET square_payment_id = 'sqp_pending_resume' WHERE id = $1", paymentID) + if err != nil { + t.Fatalf("failed to update payment: %v", err) + } + + // Seed the pending row from the first (failed) attempt. Its stored key is + // the ORIGINAL attempt's UUID; the retry below uses a different one. + origKey := paymentID + "-refund-uuid-A" + var pendingRefundID string + err = tx.QueryRow(ctx, ` + INSERT INTO refunds (payment_id, booking_id, amount, status, reason, idempotency_key, origin, created_at) + VALUES ($1, $2, 20, 'pending', 'customer request', $3, 'manual', NOW()) + RETURNING id + `, paymentID, bookingID, origKey).Scan(&pendingRefundID) + if err != nil { + t.Fatalf("failed to seed pending refund: %v", err) + } + + // Swap in a counting client to verify Square is called with the STORED key. + origClient := SquareClient + counting := &countingRefundClient{SquareClient: square.NewDevClient()} + SquareClient = counting + defer func() { SquareClient = origClient }() + + handler := RefundPayment + // Retry with a fresh key (modal reopened) — must resume, not duplicate. + newKeyReq := RefundRequest{Amount: 2000, Reason: "customer request", IdempotencyKey: "refund-uuid-B"} + w := makePaymentRequest(handler, "POST", "/api/admin/payments/"+paymentID+"/refund", newKeyReq, adminToken, ctx) + if w.Code != http.StatusOK { + t.Fatalf("retry with new key: expected 200, got %d. body: %s", w.Code, w.Body.String()) + } + + // Still exactly ONE refund row for the payment — no second pending row. + var refundCount int + err = tx.QueryRow(ctx, `SELECT COUNT(*) FROM refunds WHERE payment_id = $1`, paymentID).Scan(&refundCount) + if err != nil { + t.Fatalf("failed to count refunds: %v", err) + } + if refundCount != 1 { + t.Errorf("expected exactly 1 refund row (resumed, not duplicated), got %d — double-refund path!", refundCount) + } + + // The single row was resumed to completed. + var status string + err = tx.QueryRow(ctx, `SELECT status FROM refunds WHERE id = $1`, pendingRefundID).Scan(&status) + if err != nil { + t.Fatalf("failed to query refund status: %v", err) + } + if status != "completed" { + t.Errorf("expected the pending refund to be resumed to completed, got %q", status) + } + + // Square was called with the STORED key (uuid-A), never the new key — so + // Square's key dedup returns the original refund instead of issuing a second. + calls := counting.refundCalls() + if len(calls) != 1 { + t.Fatalf("expected exactly 1 Square refund call, got %d", len(calls)) + } + if calls[0].IdempotencyKey != origKey { + t.Errorf("expected Square call to use the stored key %q, got %q (fresh key would double-refund)", origKey, calls[0].IdempotencyKey) + } +} + +// TestRefund_PendingResume_DifferentAmountRejected verifies the P3 hardening: +// when a pending refund exists for the payment but the retry is for a DIFFERENT +// amount (admin changed it after reopening the modal), the request is rejected +// with 409 instead of creating a second pending row — which the sweep would +// otherwise process alongside the first, moving more money than intended. +func TestRefund_PendingResume_DifferentAmountRejected(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + _, bookingID, _ := setupTestData(t, ctx, tx) + + adminID, err := fixtures.CreateTestAdminUser(tx) + if err != nil { + t.Fatalf("failed to create admin user: %v", err) + } + adminToken := jwt.GenerateTestToken(adminID, "admin") + + paymentID, err := fixtures.CreateTestPayment(tx, bookingID, 5000.00, "in_person_card", "full", "completed") + if err != nil { + t.Fatalf("failed to create payment: %v", err) + } + _, err = tx.Exec(ctx, "UPDATE payments SET square_payment_id = 'sqp_pending_diff_amount' WHERE id = $1", paymentID) + if err != nil { + t.Fatalf("failed to update payment: %v", err) + } + + // Seed a pending £20 refund (first attempt failed ambiguously). + origKey := paymentID + "-refund-2000" + _, err = tx.Exec(ctx, ` + INSERT INTO refunds (payment_id, booking_id, amount, status, reason, idempotency_key, origin, created_at) + VALUES ($1, $2, 20, 'pending', 'customer request', $3, 'manual', NOW()) + `, paymentID, bookingID, origKey) + if err != nil { + t.Fatalf("failed to seed pending refund: %v", err) + } + + handler := RefundPayment + // Retry with a DIFFERENT amount (£30) and a fresh key — must be rejected + // with 409, never creating a second pending row. + req := RefundRequest{Amount: 3000, Reason: "customer request", IdempotencyKey: "refund-uuid-B-diff"} + w := makePaymentRequest(handler, "POST", "/api/admin/payments/"+paymentID+"/refund", req, adminToken, ctx) + if w.Code != http.StatusConflict { + t.Fatalf("different-amount retry while pending: expected 409, got %d. body: %s", w.Code, w.Body.String()) + } + + // Still exactly ONE pending refund row — no second row was created. + var pendingCount int + err = tx.QueryRow(ctx, `SELECT COUNT(*) FROM refunds WHERE payment_id = $1 AND status = 'pending'`, paymentID).Scan(&pendingCount) + if err != nil { + t.Fatalf("failed to count pending refunds: %v", err) + } + if pendingCount != 1 { + t.Errorf("expected exactly 1 pending refund row, got %d (second pending row would double-refund)", pendingCount) + } +} + func TestRefund_PartialRefund(t *testing.T) { t.Parallel() ctx, tx := testutils.SetupTestTx(t) @@ -841,9 +1050,10 @@ func TestRefund_PendingSameKeyRetry_Resumes(t *testing.T) { } func TestRefund_GuardCountsPendingRefunds(t *testing.T) { - // The over-refund guard must count pending refunds (an in-flight Square - // refund) as well as completed ones, so a second refund cannot push the - // total past the payment amount. + // A pending refund (in-flight Square call) blocks any further manual refund + // of the payment: same amount resumes it, a different amount is rejected + // with 409 (in-flight guard) — so the over-refund guard can never be + // bypassed by piling a second pending row on top of an unresolved one. t.Parallel() ctx, tx := testutils.SetupTestTx(t) @@ -869,7 +1079,9 @@ func TestRefund_GuardCountsPendingRefunds(t *testing.T) { t.Fatalf("failed to seed pending refund: %v", err) } - // A further £40 refund would total £110 > £100 — must be rejected. + // A further £40 refund would total £110 > £100. The in-flight guard rejects + // it with 409 before the over-refund guard is even reached — the pending + // row's money state is unknown, so no new refund is issued. req := RefundRequest{ Amount: 4000, Reason: "over refund attempt", @@ -877,8 +1089,8 @@ func TestRefund_GuardCountsPendingRefunds(t *testing.T) { handler := RefundPayment w := makePaymentRequest(handler, "POST", "/api/admin/payments/"+paymentID+"/refund", req, adminToken, ctx) - if w.Code != http.StatusBadRequest { - t.Errorf("expected status 400, got %d. body: %s", w.Code, w.Body.String()) + if w.Code != http.StatusConflict { + t.Errorf("expected status 409 (in-flight guard), got %d. body: %s", w.Code, w.Body.String()) } } diff --git a/backend/handlers/payments/service.go b/backend/handlers/payments/service.go index ec16139..5782b32 100644 --- a/backend/handlers/payments/service.go +++ b/backend/handlers/payments/service.go @@ -371,6 +371,12 @@ func (s *PaymentService) GetAlreadyRefundedAmount(ctx context.Context, paymentID return int64(math.Round(amount * 100)), nil } +// HasCompletedPayment reports whether the booking has a completed non-tip +// payment. 'tip' is deliberately excluded: a tip-only booking (no deposit/full +// payment) must NOT be treated as "already paid" for the purposes of allowing a +// tip. Note buildSplitRecords' overflow-tip records are also invisible here — +// intended (a tip is never evidence of payment), but a caller must not assume +// this covers every payment_type. func (s *PaymentService) HasCompletedPayment(ctx context.Context, bookingID string) (bool, error) { var count int err := db.Conn.QueryRow(ctx, ` diff --git a/frontend/src/lib/components/admin/EditBookingModal.svelte b/frontend/src/lib/components/admin/EditBookingModal.svelte index d69b704..aded2a1 100644 --- a/frontend/src/lib/components/admin/EditBookingModal.svelte +++ b/frontend/src/lib/components/admin/EditBookingModal.svelte @@ -50,6 +50,7 @@ let refundAmount = $state(''); let refundReason = $state(''); let refundLoading = $state(false); + let refundIdempotencyKey = $state(''); $effect(() => { if (open && bookingId) { @@ -346,6 +347,10 @@ refundPaymentId = paymentId; refundAmount = (amountPence / 100).toFixed(2); refundReason = ''; + // Unique per refund attempt so two equal partial refunds of the same + // payment don't collide on the backend's amount-derived key; reused on + // retry (the backend dedups on it) so a timeout can't double-refund. + refundIdempotencyKey = crypto.randomUUID(); showRefundModal = true; } @@ -370,7 +375,8 @@ }, body: JSON.stringify({ amount: amountPence, - reason: refundReason.trim() + reason: refundReason.trim(), + idempotency_key: refundIdempotencyKey }) });