fix: make daily export limit check atomic to prevent concurrent requests bypassing quota#347
fix: make daily export limit check atomic to prevent concurrent requests bypassing quota#347anshul23102 wants to merge 1 commit into
Conversation
…sts bypassing quota The export limit check used a non-atomic pattern: read the current count from Redis, check if it exceeds the daily maximum, then increment. Two concurrent requests arriving simultaneously could both read the same count (0 if it was a fresh day), both pass the limit check (0 < 1 or 5), and both increment, resulting in a count of 2 despite a quota of 1 or 5. Use a Lua script to atomically check and increment the quota counter in a single Redis round-trip. The script is executed atomically on the server, ensuring only one request sees a sub-limit count at a time. Concurrent requests now correctly queue and are rejected once the limit is reached, preventing quota bypass. Closes geturbackend#255 Signed-off-by: Anshul Jain <anshul23102@iiitd.ac.in>
anshul23102
requested review from
Nitin-kumar-yadav1307 and
yash-pouranik
as code owners
|
Hi @getturbackend team, this PR is ready for review. Could you please add the Label suggestions for max GSSoC scoring:
Thank you! |
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 Walkthrough
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
@coderabbitai Can you check if this PR includes a test that fires two simultaneous POST requests at maxExports=1 (per the issue's own repro steps) and asserts only one succeeds and flag it as a merge blocker if missing? |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh pr view 347 --repo geturbackend/urBackend --json files,title,body -q '.files[].path'
echo "---"
gh pr diff 347 --repo geturbackend/urBackend | head -400Length of output: 2682
Given that the entire point of this fix is to close a concurrency bug, and the issue's own repro steps describe exactly that race scenario, I'd flag the missing regression test as a merge blocker. Without it, a future refactor (e.g., someone "simplifying" the Lua script back to get/check/incr) could silently reintroduce the race with nothing to catch it. Suggested test to add (e.g., in a it('rejects concurrent requests exceeding maxExports=1', async () => {
// mock redis.eval to simulate the atomic Lua behavior, or use a real/in-memory redis
const [res1, res2] = await Promise.all([
request(app).post('/export').send({ projectId, collectionName, userId, email }),
request(app).post('/export').send({ projectId, collectionName, userId, email }),
]);
const statuses = [res1.status, res2.status].sort();
expect(statuses).toEqual([200, 429]); // exactly one succeeds, one is rate-limited
});If a real Redis instance isn't available in CI, an alternative is a targeted unit test that mocks
✅ Action performedReview finished.
|
|
@anshul23102 logic fix looks solid,but we need a test that actually proves it, not just code that looks atomic |
2 participants
Problem
The export quota check used a non-atomic pattern: get the current count from Redis, check if it's at the limit, then increment. Two concurrent requests could:
currentCount = 00 < maxExportsredis.incr(key), resulting in a count of 2Steps to reproduce (from issue):
maxExports = 1Fix
Use a Lua script to atomically check and increment in a single Redis transaction. The script is executed on the server side, ensuring no interleaving between the check and increment:
Concurrent requests now correctly queue at the limit and are rejected once quota is reached.
Closes #255