fix: atomically check and consume credits on submission#1400
Conversation
|
@mitgajera is attempting to deploy a commit to the Superteam Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis PR adds Redis-based distributed locking to the submission creation API route to serialize credit availability checks and consumption for regular (non-hackathon, non-pro) listings. A 30-second lock gates the credit validation and deduction, while hackathon and pro submissions bypass this mechanism. Lock contention returns HTTP 429; other errors are logged and mapped to appropriate status codes. ChangesRedis-based Submission Locking for Credit Management
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 197d5814ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| logger.info(`Consumed 1 credit from user ${userId} for submission`); | ||
| return submission; | ||
| }, | ||
| { ttlSeconds: 30 }, |
There was a problem hiding this comment.
When a regular submission takes longer than 30 seconds here (for example during slow Prisma calls or a transient DB/Redis slowdown), this Redis lock expires while the callback is still running. A second request from the same user can then acquire the lock, see the pre-consumption balance, create another submission, and both callbacks can consume a credit, so the check/consume is no longer atomic under exactly the contention this change is meant to protect against. The shared helper defaults to a 300-second TTL elsewhere, so this shorter TTL should either be extended or the credit check/create/consume should be protected by a DB transaction/conditional update that cannot expire mid-flight.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/api/submission/create.ts (1)
193-197:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop classifying unknown failures as 403.
Line 193 assumes validation failures contain the word
Validation, butcreateSubmission()throws the rawsafeParseJSON, so malformed requests will miss that branch. Everything else—Prisma failures, Redis failures,queueEmail()failures—also gets turned into a 403 with the internal message echoed back to the client. Map known domain errors explicitly and fall back to a generic 500 for everything else. As per coding guidelines, "Use HTTP status codes: 200 for success, 400 for validation errors, 401 for unauthorized (no auth token), 403 for forbidden (insufficient permissions), 404 for not found, 500 for internal server error."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/api/submission/create.ts` around lines 193 - 197, The error handling currently classifies most failures as 403 and echoes internal messages; change the response in the catch to explicitly map known domain errors (e.g., validation errors from createSubmission()/safeParse -> 400, missing/invalid auth -> 401, permission checks -> 403, not found cases -> 404) and for all other unexpected exceptions (Prisma, Redis, queueEmail, etc.) return a 500 with a generic message. Inspect the thrown value from createSubmission() (which may be the raw safeParse error), detect its shape/type to decide 400, and avoid sending raw error.message to clients—log the full error internally and return only a safe client-facing message (e.g., "Internal server error" for 500). Ensure you update the response construction in this handler (the block using res.status(...).json({...}) and references to userId) to implement these mappings and internal logging.
🧹 Nitpick comments (1)
src/pages/api/submission/create.ts (1)
184-190: ⚡ Quick winDowngrade handled lock contention logging from
errortowarn.This branch intentionally returns 429, but Line 184 logs every lock miss as an error before the
LockNotAcquiredErrorcheck runs. That will pollute error monitoring during normal contention. Move the genericlogger.errorbelow the lock branch, or log this case atwarninstead. As per coding guidelines, "Use debug logs for request data, info logs for successful operations, warning logs for handled issues, and error logs for exceptions."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/api/submission/create.ts` around lines 184 - 190, The current code logs every submission lock miss as an error (logger.error) before checking for LockNotAcquiredError; change this so handled lock contention is not an error: either move the existing logger.error call below the LockNotAcquiredError branch or change the log level to logger.warn when error instanceof LockNotAcquiredError, and keep the 429 response block (res.status(429).json(...)) unchanged; ensure only unexpected exceptions still call logger.error while LockNotAcquiredError uses logger.warn to avoid polluting error monitoring.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pages/api/submission/create.ts`:
- Around line 138-145: The submission is created by createSubmission(...) then
consumeCredit(...) is called separately, so if consumeCredit fails the
submission remains committed; fix by making the two operations atomic: either
implement a single transactional operation (e.g., add a new function
createSubmissionAndConsumeCredit(userId, listingId, payload, listing) that wraps
the DB insert and credit decrement in one DB transaction) or add compensating
cleanup immediately on failure (catch errors from consumeCredit(submission.id)
and call deleteSubmission(submission.id) to roll back the inserted submission,
logging and surfacing any errors). Modify the code path around createSubmission
and consumeCredit to use the transactional helper or the compensating delete to
ensure both mutations succeed or neither do.
---
Outside diff comments:
In `@src/pages/api/submission/create.ts`:
- Around line 193-197: The error handling currently classifies most failures as
403 and echoes internal messages; change the response in the catch to explicitly
map known domain errors (e.g., validation errors from
createSubmission()/safeParse -> 400, missing/invalid auth -> 401, permission
checks -> 403, not found cases -> 404) and for all other unexpected exceptions
(Prisma, Redis, queueEmail, etc.) return a 500 with a generic message. Inspect
the thrown value from createSubmission() (which may be the raw safeParse error),
detect its shape/type to decide 400, and avoid sending raw error.message to
clients—log the full error internally and return only a safe client-facing
message (e.g., "Internal server error" for 500). Ensure you update the response
construction in this handler (the block using res.status(...).json({...}) and
references to userId) to implement these mappings and internal logging.
---
Nitpick comments:
In `@src/pages/api/submission/create.ts`:
- Around line 184-190: The current code logs every submission lock miss as an
error (logger.error) before checking for LockNotAcquiredError; change this so
handled lock contention is not an error: either move the existing logger.error
call below the LockNotAcquiredError branch or change the log level to
logger.warn when error instanceof LockNotAcquiredError, and keep the 429
response block (res.status(429).json(...)) unchanged; ensure only unexpected
exceptions still call logger.error while LockNotAcquiredError uses logger.warn
to avoid polluting error monitoring.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 275f0793-72f0-4f1b-9291-864c3c6e3dd9
📒 Files selected for processing (1)
src/pages/api/submission/create.ts
| const submission = await createSubmission( | ||
| userId as string, | ||
| listingId, | ||
| { link, tweet, otherInfo, eligibilityAnswers, ask, telegram }, | ||
| listing, | ||
| ); | ||
| await consumeCredit(userId, submission.id); | ||
| logger.info(`Consumed 1 credit from user ${userId} for submission`); |
There was a problem hiding this comment.
Keep submission persistence and credit consumption in one failure domain.
If consumeCredit() fails after Line 138 has already inserted the submission, this request returns an error but the submission stays committed. The retry path then hits Submission already exists while the credit may still be unconsumed, so the atomicity guarantee this PR is aiming for is still broken. Either perform both mutations in one transaction or add compensating cleanup when credit consumption fails.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/api/submission/create.ts` around lines 138 - 145, The submission is
created by createSubmission(...) then consumeCredit(...) is called separately,
so if consumeCredit fails the submission remains committed; fix by making the
two operations atomic: either implement a single transactional operation (e.g.,
add a new function createSubmissionAndConsumeCredit(userId, listingId, payload,
listing) that wraps the DB insert and credit decrement in one DB transaction) or
add compensating cleanup immediately on failure (catch errors from
consumeCredit(submission.id) and call deleteSubmission(submission.id) to roll
back the inserted submission, logging and surfacing any errors). Modify the code
path around createSubmission and consumeCredit to use the transactional helper
or the compensating delete to ensure both mutations succeed or neither do.
1 participant
What does this PR do?
Wraps the credit check, submission creation, and credit consumption in a single Redis lock per user, making the three previously separate operations atomic.
Before this fix, concurrent requests could all pass the credit check before any credit was consumed, allowing unlimited submissions with a single credit.
Where should the reviewer start?
src/pages/api/submission/create.ts- thewithRedisLockblock at line 128.How should this be manually tested?
/api/submission/create200- the rest return429Any background context you want to provide?
canUserSubmit()andconsumeCredit()were two separate DB calls with no lock between them. The fix uses the existingwithRedisLockutility (already used in the grant tranche flow) with a 30s TTL keyed touserId. Hackathon and Pro submissions are unaffected.What are the relevant issues?
Internal security.
Screenshots
N/A
Summary by CodeRabbit