fix: prevent concurrent bounty claim race condition with pessimistic write lock - #37
Open
windy202508 wants to merge 2 commits into
Open
fix: prevent concurrent bounty claim race condition with pessimistic write lock#37windy202508 wants to merge 2 commits into
windy202508 wants to merge 2 commits into
Conversation
…ransaction-based locking
|
@windy202508 is attempting to deploy a commit to the chonilius' projects Team on Vercel. A member of the Team first needs to authorize it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Issue #1 describes a race condition in
BountiesService.claim(): two concurrentPOST /api/bounties/:id/claimrequests can both read the bounty inFUNDEDstatus, both passassertTransition, and both writes succeed — the second overwrites the first. This results in two contributors receiving 200 responses claiming they own the bounty, which can lead to disputes and potential double payouts.The same race condition exists in all status-transition methods:
fund(),markInReview(),markMergedAndRelease(), andrefund().Solution
Introduce a
withLock()private method that wraps every status-transition operation in a database transaction with a pessimistic write lock (SELECT ... FOR UPDATE). This ensures that:CLAIMED, soassertTransitioncorrectly rejects itKey changes
bounties.service.ts: AddedDataSourceinjection andwithLock()helper method that:this.dataSource.transaction()pessimistic_writelock on the bounty row viacreateQueryBuilder().setLock("pessimistic_write")fund(),claim(),markInReview(),markMergedAndRelease(), andrefund()to usewithLock()markMergedAndRelease()now uses the transaction runner for all DB operations (team/user lookups, saves)bounties.service.spec.ts: Updated tests to mockDataSource.transaction()and the query builder chain, added a new test case for concurrent claim serializationWhy pessimistic locking over optimistic?
The issue description suggests either pessimistic write lock or optimistic
@VersionColumn. I chose pessimistic locking because:setLock("pessimistic_write")natively with PostgreSQLTesting
concurrent claims for the same bounty are serialized by the pessimistic lock— verifies that a second claim after the first seesCLAIMEDstatus and is rejectedManual Testing
To verify the fix manually:
curl -X POST http://localhost:3000/api/bounties/<id>/claim -H "Authorization: Bearer <token1>"and the same with<token2>simultaneouslyCloses #1