Skip to content

fix: atomic daily review XP cap check#411

Merged
jakharmonika364 merged 2 commits into
Coder-s-OG-s:mainfrom
yush-1018:fix/atomic-review-cap
Jun 24, 2026
Merged

fix: atomic daily review XP cap check#411
jakharmonika364 merged 2 commits into
Coder-s-OG-s:mainfrom
yush-1018:fix/atomic-review-cap

Conversation

@yush-1018

Copy link
Copy Markdown
Contributor

This PR resolves #400 by enforcing the daily help review XP cap check atomically inside a database transaction rather than checking the count first in a separate non-atomic query.

Changes Made

  • Atomic Transaction Check: Wrapped the count increment in \xp_daily_usage\ and the insertion in \xp_events\ inside a database transaction in \src/lib/xp/events.ts. If the cap is exceeded, we throw \daily_review_cap_reached\ to roll back. If a duplicate event occurs, we call \ x.rollback()\ to undo the daily count increment.
  • Webhook Integration: Updated the GitHub review event handler (\processReviewEvent) to use the new atomic check and gracefully catch the limit error.
  • Unit Tests: Added coverage for rollback on limit breach and duplicate inserts inside \src/lib/xp/events.test.ts.

Verification


  • pm run typecheck\ passes cleanly.

  • px vitest run src/lib/xp/events.test.ts\ passes with 13/13 tests green.

  • px vitest run src/inngest/functions/process-review-event.test.ts\ passes with 5/5 tests green.

@vercel

vercel Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

@yush-1018 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.

Comment thread src/lib/xp/events.ts
Comment on lines +89 to +94
} catch (err: any) {
if (err.message === 'daily_review_cap_reached') {
throw err;
}
inserted = false;
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this catch swallows any transaction error that isn't the cap message and just treats it as inserted = false - a real DB failure here would silently lose the reviewer's XP with no log or retry, since process-review-event's catch never gets a chance to see it. can you narrow this to only catch the duplicate/rollback case and rethrow everything else?

@Ayush-Patel-56 Ayush-Patel-56 added the Needs author reply Author need to reply label Jun 24, 2026
@Ayush-Patel-56 Ayush-Patel-56 removed the Needs author reply Author need to reply label Jun 24, 2026

@jakharmonika364 jakharmonika364 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks!

@jakharmonika364 jakharmonika364 added level:advanced Advanced level difficulty quality:clean Clean, well-structured contribution type:bug Bug fix gssoc:approved Approved by GSSOC admin mentor:Ayush-Patel-56 Replace Ayush-Patel-56 with mentor's GitHub handle to credit them nsoc26 SSoC26 level3 Hard labels Jun 24, 2026
@jakharmonika364 jakharmonika364 merged commit 5319fc9 into Coder-s-OG-s:main Jun 24, 2026
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gssoc:approved Approved by GSSOC admin Hard level:advanced Advanced level difficulty level3 mentor:Ayush-Patel-56 Replace Ayush-Patel-56 with mentor's GitHub handle to credit them nsoc26 quality:clean Clean, well-structured contribution SSoC26 type:bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Daily help review XP cap is a check then act race, not atomic

3 participants