Skip to content

fix: prevent double-credit race condition with atomic conditional UPDATE#43

Open
memosr wants to merge 1 commit into
circlefin:masterfrom
memosr:fix/race-condition-credit-increment
Open

fix: prevent double-credit race condition with atomic conditional UPDATE#43
memosr wants to merge 1 commit into
circlefin:masterfrom
memosr:fix/race-condition-credit-increment

Conversation

@memosr

@memosr memosr commented May 26, 2026

Copy link
Copy Markdown

Problem

Two code paths can credit the same transaction, creating a race condition that allows double-crediting:

  1. User-initiated PATCH at app/api/transactions/[id]/route.ts:235 — guards with status !== 'pending' at line 209
  2. Circle webhook at app/api/circle/webhook/route.ts:152 — guards with wasAlreadyProcessed at line 148

Both paths follow the same vulnerable pattern: read status → check guard → write update + increment credits. The check-then-write is not atomic.

The race:

Time PATCH handler Circle webhook
────────────────────────────────────────────────────────────
t=0 SELECT status → 'pending'
t=1 SELECT status → 'pending'
t=2 Check passes ✓ Check passes ✓
t=3 increment_credits()
t=4 increment_credits() ← DUPLICATE
t=5 UPDATE status = 'complete'
t=6 UPDATE status = 'complete'

Fix

Replace the "check then update" pattern with an atomic conditional UPDATE that only succeeds if the status is still 'pending':

const { data: atomicResult } = await supabase
  .from("transactions")
  .update({ status: "complete", credited_at: new Date().toISOString() })
  .eq("id", id)
  .eq("status", "pending")  // ← atomic guard at the DB level
  .select()
  .maybeSingle();

if (atomicResult) {
  // We won the race — safe to credit
  await supabase.rpc("increment_credits", { ... });
}
// If atomicResult is null, another path already processed it

This pattern ensures that even if both paths read 'pending' simultaneously, only one UPDATE actually flips the status — the other gets null back and skips the credit increment.

Changes

app/api/transactions/[id]/route.ts (PATCH handler)

  • Moved increment_credits call after the atomic UPDATE
  • UPDATE now includes .eq("status", "pending") and uses .maybeSingle()
  • Kept the early-return guard at line 209 as an optimization (clarified in comment that it's not the race guard)

app/api/circle/webhook/route.ts (updateTransactionStatus)

  • Split the update into two branches:
    • Credit-eligible transitions (pending → confirmed/complete): atomic UPDATE with WHERE status='pending', credit only if row returned
    • Non-credit transitions (confirmed → complete, any → failed): plain UPDATE — no double-spend risk

Impact

  • Correctness: No more double-crediting under concurrent requests
  • Risk: Low — preserves all existing response codes and error messages
  • Performance: Same number of DB round-trips (still one UPDATE + one credit RPC)

The existing .single() calls in unrelated paths (GET handler ownership check, admin transaction updates) are untouched since they're not part of the credit flow.

@consumeobeydie

Copy link
Copy Markdown

Reviewed the fix independently. The atomic conditional UPDATE pattern is correct — replacing check-then-write with a single UPDATE ... WHERE status='pending' ... RETURNING eliminates the race window at the database level, which is the only reliable place to enforce this guarantee.

Verified the two code paths described in the problem statement exist at the locations mentioned. The fix correctly handles both the PATCH handler and the webhook path separately, which is important because their state transitions differ.

One note: the early-return guard at line 209 in the PATCH handler is now correctly documented as an optimization, not a race guard. This distinction matters for future readers.

LGTM from a correctness standpoint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants