fix(lock): release the Redis lock atomically - #1478
Conversation
The release path was a GET followed by a separate DEL. The token comparison narrows the window but does not close it: if the lock's TTL expires between the GET and the DEL, another request can acquire the key in that gap and this call then deletes a lock it no longer owns. That is not theoretical for the callers here. `locks:announce-winners` is taken with ttlSeconds: 300 on a route whose maxDuration is also 300, so a slow announcement can run right up to expiry; the same pattern guards `locks:create-tranche`. Losing mutual exclusion there means two concurrent announcements (or two tranche creations) for the same id. Replace GET+DEL with the standard compare-and-delete Lua script, which Redis executes as a single atomic unit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@rajanpanth is attempting to deploy a commit to the Superteam Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Review limit reached
Next review available in: 58 minutes Limit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The bug
withRedisLockreleases with aGETfollowed by a separateDEL:The token comparison narrows the race but doesn't close it. If the lock's TTL expires between the
GETand theDEL, another request can acquire the key in that gap — and this call then deletes a lock it no longer owns.Why it matters
Not theoretical for the current callers:
locks:announce-winners:${id}is taken withttlSeconds: 300on a route that itself declaresexport const maxDuration = 300. A slow announcement (winner updates, credit writes, referral bonuses) can run right up to expiry.locks:create-tranche:${applicationId}uses the same 300s TTL.Losing mutual exclusion there means two concurrent winner announcements, or two concurrent tranche creations, for the same id — which is the whole reason these routes are locked.
The fix
Replace
GET+DELwith the standard compare-and-delete Lua script, which Redis executes as a single atomic unit.@upstash/redisexposeseval(script, keys, args)for this.Error handling around the release is unchanged.
🤖 Generated with Claude Code