Update docs for payment remediation: test counts, advisory locks, de-scope note

README test count corrected to 1,934 (4 skipped) with the square_webhook_events migration entry; Technical Manual fixed to match the bounded try-lock, terminal flow, till idempotency, and refund sweep behaviour, and records the RespondError de-scope for the payments package; Feature Catalog and P11 plan corrected to match the actual UserBookingModal/CardSelection wiring.

README.md

@@ -4,9 +4,9 @@ Nail salon booking platform — Go 1.26.5 backend + SvelteKit 5 SPA + PostgreSQL
 
 ## Features
 
-**Booking**: Self-service (customer), walk-in (admin), call-in (admin). Slot reservations prevent double-booking (4 TTL types). **Self-blocking prevention**: `excludeUserID` parameter filters a user's own `RESERVATION` entries from time blocker overlap checks, allowing re-reservation and booking at overlapping slots. **Explicit cancellation**: `DELETE /api/bookings/reserve` releases a user reservation; `DELETE /api/admin/bookings/reserve` releases an admin walk-in/call-in reservation. **Background cleanup**: Centralised cron scheduler (`backend/internal/jobs/`) runs 23 maintenance jobs: reservation/deposit cleanup every 5min, hourly campaign transitions, daily unpaid-booking notifications, staged default hours auto-apply, GDPR anonymization, financial aggregation, and token/code cleanup. Guest accounts with GDPR-compliant anonymization (including `RESERVATION:edit_request:%` scrubbing). Service eligibility based on age + patch test validity. Overlap checks use `FOR UPDATE` row locks inside transactions. Closing-hours validation (`closing_time.go`) resolves both current and staged default hours.
+**Booking**: Self-service (customer), walk-in (admin), call-in (admin). Slot reservations prevent double-booking (4 TTL types). **Self-blocking prevention**: `excludeUserID` parameter filters a user's own `RESERVATION` entries from time blocker overlap checks, allowing re-reservation and booking at overlapping slots. **Explicit cancellation**: `DELETE /api/bookings/reserve` releases a user reservation; `DELETE /api/admin/bookings/reserve` releases an admin walk-in/call-in reservation. **Background cleanup**: Centralised cron scheduler (`backend/internal/jobs/`) runs 24 maintenance jobs: reservation/deposit cleanup every 5min, hourly campaign transitions, daily unpaid-booking notifications, staged default hours auto-apply, GDPR anonymization, financial aggregation, and token/code cleanup. Guest accounts with GDPR-compliant anonymization (including `RESERVATION:edit_request:%` scrubbing). Service eligibility based on age + patch test validity. Overlap checks use `FOR UPDATE` row locks inside transactions. Closing-hours validation (`closing_time.go`) resolves both current and staged default hours.
 
-**Payments**: Square Terminal (in-person, via `CreateTerminalCheckout`) + online card payments via saved cards or new cards tokenized through the Square Web Payments SDK (`cnon:` nonces — new-card entry falls back to `CardEntryUnavailable` only when neither mock mode nor Square credentials are configured). The backend accepts only tokens, never raw PANs (PCI-DSS parity, mirrored in the dev mock). Cash with change calculation. Gift cards (12-digit code or account balance). Saved cards for faster checkout. Tips on completed bookings. Refunds with notice-period tiers and deposit protection (72h/24h thresholds). All payment types: deposit, full, partial, balance, tip. Payment >20% of total promotes `pending_release` bookings back to `confirmed`. Deposit paid is computed from payments on-the-fly. The first 50% of each payment is always carved out as deposit (via `buildSplitRecords`); any overflow beyond the booking total becomes a tip. A PostgreSQL `pg_advisory_lock` serializes payment attempts per-booking to prevent two-tab double-payment races. Gift card purchases insert a pending payment record with VAT before calling Square — the DB transaction commits first, so Square failures leave a retryable pending record (same-key retries reuse it). Three background sweeps close Square's ~24h idempotency-key retention window: `sweep-pending-square-refunds` reconciles/retries stuck refunds (with a 23h age guard), `sweep-stale-pending-payments` fails stale pending payments/till-sales so a late retry cannot issue a second charge, and `sweep-stale-terminal-checkouts` cancels card-machine checkouts still pending at Square after an hour so a never-polled checkout cannot complete into an invisible, untracked charge.
+**Payments**: Square Terminal (in-person, via `CreateTerminalCheckout`) + online card payments via saved cards or new cards tokenized through the Square Web Payments SDK (`cnon:` nonces — new-card entry falls back to `CardEntryUnavailable` only when neither mock mode nor Square credentials are configured). The backend accepts only tokens, never raw PANs (PCI-DSS parity, mirrored in the dev mock). Cash with change calculation. Gift cards (12-digit code or account balance). Saved cards for faster checkout. Tips on completed bookings. Refunds with notice-period tiers and deposit protection (72h/24h thresholds). All payment types: deposit, full, partial, balance, tip. Payment >20% of total promotes `pending_release` bookings back to `confirmed`. Deposit paid is computed from payments on-the-fly. The first 50% of each payment is always carved out as deposit (via `buildSplitRecords`); any overflow beyond the booking total becomes a tip. A bounded PostgreSQL advisory try-lock (`pg_try_advisory_lock`, ~30 × 100ms ≈ 3s bound) serializes payment attempts per-booking to prevent two-tab double-payment races. Gift card purchases insert a pending payment record with VAT before calling Square — the DB transaction commits first, so Square failures leave a retryable pending record (same-key retries reuse it). Three background sweeps close Square's ~24h idempotency-key retention window: `sweep-pending-square-refunds` reconciles/retries stuck refunds (with a 23h age guard), `sweep-stale-pending-payments` fails stale pending payments/till-sales so a late retry cannot issue a second charge, and `sweep-stale-terminal-checkouts` cancels card-machine checkouts still pending at Square after an hour so a never-polled checkout cannot complete into an invisible, untracked charge.
 
 **Gift Cards**: Multi-method purchase (cash, card machine, online card, giveaway). Inventory cards for stock management. 24-month rolling expiry. Idle account cleanup (2yr/5yr thresholds). Expired balance recovery with admin audit trail. Transaction audit log. Idempotency keys for purchases.
 
@@ -85,7 +85,7 @@ Default logins (password: `password`):
 ```bash
 cd backend && go build -o bin/backend ./main.go
 cd frontend && npm ci && npm run build
-cd backend && go test -tags "test,dev" -count=1 -parallel 8 ./...   # 1,902 tests passed (4 skipped, ~2min)
+cd backend && go test -tags "test,dev" -count=1 -parallel 8 ./...   # 1,934 tests passed (4 skipped, ~2min)
 cd backend && go test -tags "test,dev" -count=1 -race -timeout 480s ./...  # race detector (all packages, ~4min)
 cd backend && go test -tags "test,dev" -count=10 -parallel 8 ./...  # thorough verification (~2-3min)
 ```
@@ -136,6 +136,12 @@ CREATE TABLE IF NOT EXISTS terminal_checkouts (
 );
 CREATE INDEX IF NOT EXISTS idx_terminal_checkouts_booking ON terminal_checkouts(booking_id, status);
 CREATE INDEX IF NOT EXISTS idx_terminal_checkouts_status ON terminal_checkouts(status);
+
+-- Square webhook dedup (square_webhook_events table)
+-- NOTE: CREATE TABLE is a fresh addition, not a column change. Apply before deploying
+-- the webhook handler changes or the dedup INSERT fails at runtime.
+-- Required for the Square webhook dedup; without it webhooks fail closed 503.
+CREATE TABLE IF NOT EXISTS square_webhook_events (event_id TEXT PRIMARY KEY, received_at TIMESTAMPTZ NOT NULL DEFAULT NOW());
 ```
 
 `backend/handlers/payments/refunds.go` casts `'refund_failed'::admin_notification_reason` (the sweep job in `internal/jobs/cleanup.go` only registers the handler), so an un-migrated DB fails at runtime — apply these before deploying the payment changes.

obsidian/Crussell/Feature Catalog.md

@@ -473,7 +473,7 @@ The central management hub for salon operations — managing users, bookings, se
 **Related:** [[Patch Tests|1.6 Patch Tests]], [[Services Catalog|1.5 Services Catalog]]
 
 ### 5.9 Till Purchases (POS)
-**What it does:** Point-of-sale interface for selling gift cards at the counter (cash, card machine, saved card, online card entry, on the house). The saved-card path charges the customer's card-on-file directly via Square (pending-first, deterministic idempotency key, advisory lock per booking); card-machine sales create a Square Terminal checkout and are completed by polling. Pending card till-sales are failed by the stale-pending sweep after Square's ~24h key-retention window.
+**What it does:** Point-of-sale interface for selling gift cards at the counter (cash, card machine, saved card, online card entry, on the house). The saved-card path charges the customer's card-on-file directly via Square (pending-first, client-supplied UUID idempotency key — or a generated `till-` key when absent — with a bounded advisory try-lock keyed on that idempotency key); card-machine sales create a Square Terminal checkout and are completed by polling. Pending card till-sales are failed by the stale-pending sweep after Square's ~24h key-retention window.
 
 **Layman summary:** "Sell gift cards at the till — take cash or card."
 

obsidian/Crussell/Technical Manual.md

@@ -87,7 +87,7 @@ Backend (:8080)
 | `ProgressiveRateLimit` | Per-IP dual-window rate limiter for bot-spam prevention. Burst: 30 req/5s, Sustained: 120 req/60s. Progressive delays (500ms–10s). Applied to login/register. Skips in dev build. |
 | `JsonContentType` | Sets `Content-Type: application/json` on all API responses. Individual handlers that need to override (e.g. binary/image responses) set their own Content-Type header after this middleware runs. Applied globally in `main.go:182`. This replaced ~80+ individual `w.Header().Set("Content-Type", "application/json")` calls across all handlers. |
 | `RespondJSON(w, status, data)` | Helper function (in `response.go`, not middleware) for consistent JSON responses. Always sets `Content-Type: application/json`. |
-| `RespondError(w, status, message)` | Helper that wraps `RespondJSON` with `{"error": message}`. Replaces `http.Error()` in all new code for consistent JSON error format. |
+| `RespondError(w, status, message)` | Helper that wraps `RespondJSON` with `{"error": message}`. **De-scoped for the payments package (Aug 2026 remediation):** the payments handlers deliberately keep using raw `http.Error` — the argument order differs (`http.Error(w, msg, status)` vs `RespondError(w, status, msg)`), the frontend only checks `res.ok` on those endpoints, and migrating ~396 call sites mid money-critical remediation added regression risk for zero functional gain. Recorded as a permanent MINOR/consistency backlog item; revisit only if the frontend starts reading structured error bodies from payments endpoints. Elsewhere, `RespondError` is the standard for consistent JSON error format. |
 
 ### Database Layer (`db/`)
 
@@ -664,7 +664,7 @@ CORS uses `*` in local dev. In production behind Cloudflare, nginx handles CORS.
 
    All split records share the same `square_payment_id` so the refund loop can avoid duplicate Square API calls. After the booking start time, no split is applied — the charge records as a single payment with its original type.
 
-   **Concurrency guard:** `CreateBookingPayment` acquires a PostgreSQL session-level advisory lock (`pg_advisory_lock(hashtext('crussell:payment:' || booking_id))`) at entry and releases it in a defer. This serializes all payment attempts for the same booking — if two browser tabs try to pay simultaneously, the second request blocks until the first commits or rolls back. After the lock, the handler re-checks booking status (a concurrent payment may have promoted it) and runs a payment-type duplicate guard that prevents two `'full'` or `'deposit'` payments from being created for the same booking, even with different idempotency keys.
+   **Concurrency guard:** `CreateBookingPayment` acquires a PostgreSQL session-level advisory **try-lock** (`pg_try_advisory_lock(hashtext('crussell:payment:' || booking_id))`, via `acquireAdvisoryLock` in `handlers/payments/locks.go`) at entry and releases it in a defer. The lock is **bounded**: `tryAdvisoryLock` retries `pg_try_advisory_lock` ~30 times with a 100ms backoff (~3s total), and a contended second request gets a 409 "payment in progress, try again" instead of blocking a pool connection (blocking would hold the pinned connection hostage across the lock-holder's up-to-30s Square round-trip and could exhaust the pool). After the lock, the handler re-checks booking status (a concurrent payment may have promoted it) and runs a payment-type duplicate guard that prevents two `'full'` or `'deposit'` payments from being created for the same booking, even with different idempotency keys.
 
 3. **Deposit deadline passes without payment** → `CleanupExpiredDeposits()` moves the booking to `pending_release`. The slot becomes vulnerable — another booking can claim it via eviction. An admin notification `deposit_not_paid_by_deadline` is created. The user's time blocker reservations are also cleaned up.
 4. **Slot claimed by another booking** → If a new booking overlaps a `pending_release` slot, `EvictPendingReleaseOverlapping` (a shared function in `bookings.go`) evicts the pending_release booking to `deposit_lapsed`. The eviction runs inside the same transaction as the new booking's creation, so it rolls back if the new booking fails. The function is called by all 4 eviction sites: `CreateBookingHandler`, `ConfirmBookingHandler`, `AdminCreateBookingForUserHandler`, and `AdminRescheduleBookingHandler`. A `PAYMENT_IN_FLIGHT` time_blocker guard prevents evicting a booking that the user is currently paying for (5-minute window).
@@ -1218,19 +1218,25 @@ Each test package has a `TestMain` that runs schema migration once per package (
 
 Between tests, `TruncateTables()` runs `TRUNCATE TABLE ... CASCADE` on all tables. This is ~60% faster than DROP+CREATE.
 
-### Payment Serialization Lock (advisory lock 1339)
+### Payment Serialization Lock (bounded advisory try-lock)
 
-`CreateBookingPayment` acquires a PostgreSQL session-level advisory lock (`pg_advisory_lock(hashtext('crussell:payment:' || booking_id))`) to serialize concurrent payment attempts for the same booking. This prevents the two-tab double-payment race where two browser tabs submit payments with different idempotency keys but the same payment type.
+`CreateBookingPayment` acquires a PostgreSQL session-level advisory **try-lock** (`pg_try_advisory_lock(hashtext('crussell:payment:' || booking_id))`, via `acquireAdvisoryLock` in `handlers/payments/locks.go`) to serialize concurrent payment attempts for the same booking. This prevents the two-tab double-payment race where two browser tabs submit payments with different idempotency keys but the same payment type.
 
 **Key implementation detail:** The lock must be acquired and released on the **same** database connection. Using `db.DB.Exec()` for both would be unsafe — each call may get a different pool connection, and `pg_advisory_unlock` on a different session is a silent no-op, leaking the lock. The code uses `db.DB.Acquire()` to pin a dedicated connection for the duration of the handler, with `defer pinConn.Release()` ensuring the connection is returned when done.
 
 ```go
 pinConn, err := db.DB.Acquire(r.Context())
 defer pinConn.Release()
-pinConn.Exec(ctx, "SELECT pg_advisory_lock(hashtext('crussell:payment:' || $1))", bookingID)
+lockOK, err := acquireAdvisoryLock(r.Context(), pinConn, "crussell:payment:"+bookingID)
-defer pinConn.Exec(ctx, "SELECT pg_advisory_unlock(hashtext('crussell:payment:' || $1))", bookingID)
+if !lockOK {
+    http.Error(w, "Payment in progress, try again", http.StatusConflict)
+    return
+}
+defer pinConn.Exec(context.Background(), "SELECT pg_advisory_unlock(hashtext('crussell:payment:' || $1))", bookingID)
 ```
 
+The lock is **bounded**: `tryAdvisoryLock` retries `pg_try_advisory_lock` up to `advisoryLockAttempts` (30) times with a 100ms backoff (`advisoryLockRetryDelay`), giving up after ~3s total. On timeout the handler returns a 409 "payment in progress, try again" rather than blocking. Blocking would hold the pinned pool connection for the full Square round-trip (up to ~30s) of whichever request holds the lock, and a handful of concurrent same-key requests would exhaust the whole pool (`max(4, numCPU)`) and hang the app. The deliberately-blocking variant (`pg_advisory_xact_lock`, unbounded) is used on ONE path only: the admin-cancellation path (`lockCancellationPayments`), where a bound would risk dropping a cancellation refund silently. See the comment block in `locks.go` for the full rationale.
+
 After acquiring the lock, the handler re-checks the booking status (a concurrent payment may have promoted it) and runs a payment-type duplicate guard that prevents two `'full'` or `'deposit'` payments from being created for the same booking, even with different idempotency keys. The guard accounts for `buildSplitRecords` which converts `'full'` input to `'deposit'` + `'balance'` records.
 
 ### Cursor-Based Pagination Pattern
@@ -1258,11 +1264,7 @@ Files with this pattern: `bookings.go` (4 handlers), `custom_services.go`, `user
 
 ### Advisory Lock Pattern (test infrastructure)
 
-**Migration lock (1337):** `pg_advisory_lock(1337)` protects concurrent schema migration. When multiple test packages run simultaneously, only one executes the migration DDL at a time.
+**Removed.** Test infrastructure no longer uses advisory locks. With per-package databases (each test binary gets its own `crussell_test_*` DB) and per-test transaction rollback, there is no cross-package or cross-test contention, so the old `pg_advisory_lock(1337)` migration lock and `pg_advisory_lock(1338)` truncation lock were deleted. `migratePool` runs on a fresh database created inside `CreateTestDatabase`, and `Migrate` runs once per package via `TestMain`. Production payment paths use the bounded try-lock in `handlers/payments/locks.go` instead (see "Payment Serialization Lock" above).
-
-**Truncation lock (1338):** `pg_advisory_lock(1338)` prevents CASCADE truncation deadlocks. When two tests try to truncate simultaneously, one waits for the other.
-
-**Implementation:** Both use `pool.Acquire()` for a dedicated connection (same pattern as the payment serialization lock above).
 
 ### Statement-by-Statement SQL Parser
 
@@ -1355,23 +1357,23 @@ sequenceDiagram
     participant A as Admin
     participant F as Frontend
     participant B as Backend
+    participant DB as PostgreSQL
     participant S as Square API
     participant T as Terminal Device
 
     A->>F: Click "Take Payment" → Select Card (Terminal)
-    F->>B: POST /api/admin/bookings/{id}/payment
+    F->>B: POST /api/admin/bookings/{id}/payment (payment_type)
+    B->>DB: INSERT terminal_checkouts (provisional "tmp-" checkout_id, PENDING, payment_type)
     B->>S: Create checkout
     S-->>B: checkout_id
-    B->>S: Create terminal payment
+    B->>DB: UPDATE terminal_checkouts (attach real checkout_id)
-    S-->>B: terminal_payment_id
     B-->>F: 202 + checkout_id
     F-->>A: Show "Waiting for terminal..."
     T->>S: Customer inserts card
     B->>S: Poll GetCheckoutStatus
     S-->>B: COMPLETED + payment_ids
-    B->>DB: INSERT payment (status=completed)
+    B->>DB: Read payment_type from terminal_checkouts, INSERT payment (status=completed)
-    B->>DB: Update booking status
+    B->>DB: UPDATE terminal_checkouts status=COMPLETED
-    B->>DB: Apply discounts (if completed)
     B-->>F: SSE / Poll: payment complete
     F-->>A: Show "Payment successful"
 ```
@@ -1384,6 +1386,7 @@ sequenceDiagram
     participant F as Frontend
     participant B as Backend
     participant DB as PostgreSQL
+    participant S as Square API
 
     C->>F: Go to gift card purchase page
     F->>C: Enter amount, payment method
@@ -1394,11 +1397,11 @@ sequenceDiagram
         DB-->>B: Return existing gift card
         B-->>F: 200 + existing
     else Key new
+        B->>DB: INSERT payment (status=pending) + VAT
+        B->>DB: COMMIT
         B->>S: Process payment (if online)
         S-->>B: Payment confirmed
-        B->>DB: INSERT gift_card
+        B->>DB: UPDATE payment → completed + INSERT gift_card + gift_card_transaction (one tx)
-        B->>DB: INSERT gift_card_transaction (purchase)
-        B->>DB: INSERT payment (if applicable)
         DB-->>B: OK
         B-->>F: 201 + gift card
     end
@@ -1489,7 +1492,7 @@ sequenceDiagram
 
 ## Key Code Patterns
 
-### Test Pattern: Advisory Lock
+### Test Pattern: Migration
 
 ```go
 // testdb.go
@@ -1500,14 +1503,10 @@ func runMigrations(pool *pgxpool.Pool) error {
     }
     defer conn.Release()
 
-    // Acquire lock on dedicated connection
+    // Run migration — no advisory lock: this always runs on a fresh
-    _, err = conn.Exec(context.Background(), "SELECT pg_advisory_lock(1337)")
+    // per-package database created inside CreateTestDatabase, so there is no
-    if err != nil {
+    // cross-package contention to serialize (the old pg_advisory_lock(1337)
-        return err
+    // guard was removed).
-    }
-    defer conn.Exec(context.Background(), "SELECT pg_advisory_unlock(1337)")
-
-    // Run migration
     statements := splitSQLStatements(migrationSQL)
     for _, stmt := range statements {
         _, err = conn.Exec(context.Background(), stmt)

obsidian/Crussell/Testing Architecture & DB Management.md

@@ -586,7 +586,7 @@ It's called inside `CreateTestDatabase`, which is called in `TestMain`. Since ea
 
 ### Q: Do I need to worry about advisory locks?
 
-**No.** Advisory locks (`pg_advisory_lock`) have been removed from test infrastructure. With per-package databases and per-test transactions, there's no cross-package or cross-test contention. The single production use of `pg_advisory_lock` is in the payment handler (`handlers/payments/handlers.go:706`) for serializing concurrent payment attempts on the same booking — this is unrelated to test infrastructure.
+**No.** Advisory locks (`pg_advisory_lock`) have been removed from test infrastructure. With per-package databases and per-test transactions, there's no cross-package or cross-test contention. Production payment paths use a **bounded try-lock** (`pg_try_advisory_lock`/`pg_try_advisory_xact_lock` via `acquireAdvisoryLock`/`acquireAdvisoryXactLock` in `handlers/payments/locks.go`, ~30 attempts × 100ms ≈ 3s bound) to serialize concurrent payment attempts on the same booking/key — this is unrelated to test infrastructure. The only deliberately-blocking variant (`pg_advisory_xact_lock`, unbounded) is the admin-cancellation path (`lockCancellationPayments`).
 
 ### Q: What's the `GO_TESTING` env var for?
 

obsidian/Crussell/plans/p11-square-web-payments-sdk.md

@@ -20,7 +20,7 @@ All 8 flows now render `SquareCardInput` (`frontend/src/lib/components/payments/
 
 1. `frontend/src/routes/tip/+page.svelte` — tip; saved-card list + `card_id`, SquareCardInput for new card (renders the shared `TipPayment.svelte` component)
 2. `frontend/src/routes/pay-tip/[id]/+page.svelte` — tip; same pattern (also renders the shared `TipPayment.svelte` component)
-3. `frontend/src/lib/components/account/UserBookingModal.svelte` — tip modal; same pattern
+3. `frontend/src/lib/components/account/UserBookingModal.svelte` — tip modal; uses `CardSelection` directly (saved-card list + new-card toggle + SquareCardInput + card-save consent) — it does **not** render the shared `TipPayment.svelte` component
 4. `frontend/src/lib/components/payments/UserPaymentModal.svelte` — booking payment; uses `CardSelection` with SquareCardInput
 5. `frontend/src/lib/components/booking/BookingFlow.svelte` — deposit; saved-card list + `card_id`, SquareCardInput for new card (incl. guest flow)
 6. `frontend/src/routes/account/+page.svelte` — Buy a Gift Card; saved-card list + `card_id`, SquareCardInput for new card