fix: prevent race condition in challenge submission (#187)

[pbrissaud]

Summary

• Add uniqueIndex on (userId, challengeId) in userProgress to enforce one row per user/challenge at the DB level
• Replace non-atomic update/insert block in submitChallenge with conditional atomic operations using .returning() to detect if the race was lost
• If RETURNING is empty (another concurrent request already completed), skip XP transactions and PostHog events and return early

How it works

UPDATE path (row exists): adds ne(status, "completed") to the WHERE clause — only proceeds if status wasn't already flipped by a concurrent request.

INSERT path (no row yet): uses .onConflictDoNothing() backed by the new unique index — concurrent inserts silently no-op and return an empty array.

In both cases, an empty RETURNING result means the race was lost and we return { success: true, xpAwarded: 0 } without awarding XP or firing analytics events.

Test plan

• Normal submission flow: XP awarded, PostHog event fired, userProgress.status = completed
• Re-submission after completion: early return with xpAwarded: 0, no duplicate XP transactions
• Rapid double-submission: only one userXpTransaction row per challenge per user in DB

🤖 Generated with Claude Code

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
website	Ready	Preview, Comment	Mar 11, 2026 0:12am

[claude]

Claude finished @pbrissaud's task in 2m 3s —— View job

---

PR Review

• Read changed files
• Analyze race condition fix logic
• Review schema changes and migration
• Review security and correctness
• Post final review

---

Summary

This PR correctly addresses the race condition in submitChallenge by replacing a non-atomic read-then-write pattern with conditional atomic operations backed by a new UNIQUE INDEX. The approach is sound. A few observations below, ranging from correctness concerns to minor nits.

---

Correctness

Atomic UPDATE path — The ne(userProgress.status, "completed") guard in the WHERE clause of the UPDATE is solid. Only one concurrent winner can flip the status; all others see an empty RETURNING and return early. ✅

Atomic INSERT path — onConflictDoNothing() backed by the new unique index on (userId, challengeId) correctly silences concurrent inserts. ✅

Migration — The SQL migration only adds a UNIQUE INDEX, so it's safe to apply to existing data assuming there are no pre-existing duplicates. If duplicates exist from before the fix was deployed, the migration will fail. Consider whether a data-cleanup step is needed before running this in production.

---

Potential Issues

1. isFirstChallenge is still read before the atomic write (userProgress.ts, lines 627–637)

const [completedCount] = await ctx.db
  .select({ count: count() })
  .from(userProgress)
  .where(
    and(eq(userProgress.userId, userId), eq(userProgress.status, "completed")),
  );
const isFirstChallenge = (completedCount?.count ?? 0) === 0;

For the same-challenge race case, this is fine — only the winner gets past progressUpdated and applies XP. But for concurrent submissions of different challenges when both start with 0 completions, both can read isFirstChallenge = true and — if both win their respective atomic updates — both will receive the first-challenge bonus. This is a pre-existing issue that isn't introduced by this PR, but it's worth tracking.

Fix this →

2. userXp update is still non-atomic (userProgress.ts, lines 703–725)

const oldXp = existingXp?.totalXp ?? 0;
const newXp = oldXp + xpGain.total;
if (existingXp) {
  await ctx.db.update(userXp).set({ totalXp: newXp, ... })...
}

This is a read-modify-write on userXp.totalXp. Concurrent winners for different challenges from the same user can still race here. Since xpTransactions are append-only and serve as the authoritative record, consider dropping this denormalized total or replacing the update with an atomic totalXp = totalXp + xpGain.total increment.

Fix this →

3. completeChallenge still uses the old non-atomic pattern (lines 222–239)

The completeChallenge mutation (used by the web UI) was not updated and still uses the SELECT-then-UPDATE/INSERT pattern. If this endpoint is accessible concurrently (e.g., a user double-clicks), it has the same race condition. The new uniqueIndex would at least prevent duplicate DB rows on concurrent inserts, but duplicate XP transactions are still possible.

---

Minor Nits

4. Early throw at line 518–519 is now redundant for the race case

if (existingProgress?.status === "completed") {
  throw new Error("Challenge already completed");
}

This check remains from before the fix. It's still useful as a fast-path for intentional re-submissions (returns a clear error vs. silently returning xpAwarded: 0), but the comment could note that it's a UX short-circuit, not the actual race guard.

5. rank: "" in the early return is a weak default (line 688)

return { success: true, xpAwarded: 0, totalXp: 0, rank: "", rankUp: false, ... };

An empty string for rank could confuse consumers. Since XP wasn't awarded, returning the current rank (already fetched later in the happy path) or null would be cleaner. Not a blocker, but worth noting.

---

Verdict

The core fix is correct and well-scoped. The uniqueIndex + onConflictDoNothing + ne(status, "completed") guard pattern is the right approach for preventing duplicate XP on concurrent submissions. The pre-existing userXp non-atomicity and completeChallenge gap are the most meaningful follow-up items.

[claude]

Claude finished @pbrissaud's task in 1m 40s —— View job

---

PR Review

• Read changed files
• Analyze race condition fix logic
• Review follow-up commits
• Post final review

---

Summary

The follow-up commit (4be783f) addressed all four major issues raised in the previous review. Here's a complete assessment of the final state.

---

Issues Resolved by Follow-up Commit ✅

Issue	Status
isFirstChallenge read before atomic write	Fixed — now reads xpTransaction count after the atomic userProgress write
userXp.totalXp non-atomic read-modify-write	Fixed — replaced with INSERT … ON CONFLICT DO UPDATE SET totalXp = totalXp + delta
completeChallenge using old SELECT-then-UPDATE/INSERT pattern	Fixed — same atomic conditional update/insert pattern applied
rank: "" in race-lost early return	Fixed — returns rank: null
UX fast-path comments missing	Fixed — comments clarify the early throw is a UX fast-path, not the race guard

---

Remaining Observations

1. First-challenge race window is reduced, not eliminated (userProgress.ts, lines 685–694)

The comment says concurrent winners "see each other's committed transaction" — this is only true if one winner has already reached the userXpTransaction INSERT by the time the other reads the count. In practice, both requests can finish the atomic progress write nearly simultaneously, then both read count = 0 before either has inserted a transaction. The fix meaningfully narrows the window, and the commit message acknowledges this. Not a blocker.

2. Unused _newXp variable in completeChallenge (userProgress.ts, line 272)

const _newXp = updatedXp?.totalXp ?? xpGain.total;

This value is assigned but never used. The underscore prefix suppresses the TS warning, but if _newXp isn't needed in the return value (unlike submitChallenge which returns totalXp: newXp), the whole assignment can be removed.

Fix this →

3. No DB transaction wrapping (pre-existing)

The sequence userProgress update → userXp upsert → userXpTransaction inserts is not wrapped in a single DB transaction. A failure partway through (e.g., after marking progress completed but before inserting XP transactions) leaves data in a partially-written state. Pre-existing issue, not introduced here, but worth tracking for a future cleanup.

---

Verdict

The core fix is solid and the follow-up commit resolved all the significant concerns from the previous review. The atomic UNIQUE INDEX + onConflictDoNothing + ne(status, "completed") pattern is the correct approach. The remaining items are either theoretical edge cases or pre-existing issues. This is ready to merge.