fix: atomically pair XP insert and help request resolution - #848
Conversation
The XP award and help request status update used two different database clients (Supabase for help_requests, Drizzle for xp_events), making them impossible to wrap in a single transaction. This caused state inconsistency when the daily XP cap was reached or duplicate events occurred. Fix: - Move help request status update from Supabase to Drizzle so it uses the same database connection as XP event insertion - Wrap both operations in a single Drizzle transaction — both commit or both roll back atomically - Allow insertXpEvent to accept an optional transaction client (tx) for reuse within a caller-managed transaction - Add db parameter to sumXpToday for consistent client usage Closes Coder-s-OG-s#845
|
@ionfwsrijan is attempting to deploy a commit to the codersogs-3057's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@codersogs-code @Soumya-codr Please review this |
|
please fix the CI |
|
@jakharmonika364 @Soumya-codr Please review now |
jakharmonika364
left a comment
jakharmonika364
left a comment
There was a problem hiding this comment.
In events.ts's doDailyCapCheck - when a caller passes tx and the XP insert is a duplicate (result.length === 0), we skip client.rollback() (correctly, to avoid killing the caller's transaction) but never undo the xp_daily_usage.count increment either. It's not reachable from this PR's call site (the status='open' guard on the help-request update already prevents re-entry), but insertXpEvent(event, tx) is a shared util now - the next caller that hits a genuine duplicate mid-transaction will silently over-count that user's daily cap. Could you check-for-existing-row before incrementing instead of increment-then-maybe-undo? Also events.test.ts doesn't cover the tx-provided branch at all - worth a test for it either way.
jakharmonika364
added
Needs author reply
A caller-provided tx can't be rolled back mid-transaction, so a duplicate insertXpEvent(event, tx) call used to silently inflate xp_daily_usage.count. Check for the existing row first and skip the increment entirely. Also type the tx param (was any) and cover the tx-provided branch in events.test.ts, plus mock getDb in process-review-event.test.ts which the PR's atomic transaction path requires.
|
@jakharmonika364 Please review now. I've made the changes |
jakharmonika364
added
level:advanced
2 participants
Description
The "award-help-review" flow in
process-review-event.tsuses two different database clients — Supabase service role for help request status updates, and Drizzle/postgres-js for XP event insertion — which cannot be wrapped in a single transaction. This creates three inconsistency scenarios:status = 'resolved', potentially overwriting concurrent stateChanges
txparameter toinsertXpEventandsumXpTodayto accept a caller-managed transaction clienttxis provided,insertXpEventskips creating its own nested transaction and uses the caller's transaction directlyCloses #845