fix: Prevent Duplicate Spam Credit Penalties - #1463
Conversation
WalkthroughSpam penalty allocation now uses Redis locks and existing-penalty checks. Allocation functions return whether they created a penalty. Spam labeling uses this result to skip duplicate penalty emails. ChangesSpam penalty idempotency
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SubmissionLabelFlow
participant addSpamPenaltyCredit
participant Redis
participant PenaltyLedger
participant EmailQueue
SubmissionLabelFlow->>addSpamPenaltyCredit: Process spam-labeled submission
addSpamPenaltyCredit->>Redis: Acquire submission-specific lock
Redis-->>addSpamPenaltyCredit: Lock result
addSpamPenaltyCredit->>PenaltyLedger: Check or create penalty
PenaltyLedger-->>addSpamPenaltyCredit: Return penalty status
addSpamPenaltyCredit-->>SubmissionLabelFlow: Return created status
alt Credit created
SubmissionLabelFlow->>EmailQueue: Queue spam-credit email
else Credit not created
SubmissionLabelFlow->>SubmissionLabelFlow: Record update
end
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/features/credits/utils/allocateCredits.ts (1)
10-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake
createdreadonly.
CreditPenaltyResultis an object type. Its properties should be readonly by default.-type CreditPenaltyResult = { created: boolean }; +type CreditPenaltyResult = { readonly created: boolean };As per coding guidelines, use
readonlyproperties for object types by default.🤖 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/features/credits/utils/allocateCredits.ts` at line 10, Update the CreditPenaltyResult type so its created property is declared readonly, preserving the existing boolean type and object shape.Source: Coding guidelines
🤖 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/features/credits/utils/allocateCredits.ts`:
- Around line 114-116: Update the LockNotAcquiredError handling in
allocateCredits to return a distinct retryable lock-contention result rather
than `{ created: false }`; apply the same change to the corresponding branch
near the later catch block. Model the mutually exclusive outcomes with a
discriminated union, and reserve the duplicate result for cases where an
existing penalty is confirmed.
- Around line 77-113: Add composite unique constraints for (submissionId, type)
and (applicationId, type) to the CreditLedger schema, then update the spam
penalty creation flow around withRedisLock to catch Prisma unique-constraint
conflicts from creditLedger.create and return { created: false }; preserve
existing behavior for successful inserts and unrelated errors.
---
Nitpick comments:
In `@src/features/credits/utils/allocateCredits.ts`:
- Line 10: Update the CreditPenaltyResult type so its created property is
declared readonly, preserving the existing boolean type and object shape.
🪄 Autofix
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 Plus
Run ID: e84fd474-5396-413f-ba1d-7482aa2b6382
📒 Files selected for processing (2)
src/features/credits/utils/allocateCredits.tssrc/pages/api/sponsor-dashboard/submission/update-label.ts
| return await withRedisLock( | ||
| `locks:credit-penalty:submission:${submissionId}`, | ||
| async () => { | ||
| const submission = await prisma.submission.findFirst({ | ||
| where: { id: submissionId, label: SubmissionLabels.Spam }, | ||
| select: { id: true, userId: true }, | ||
| }); | ||
|
|
||
| if (!submission) { | ||
| throw new Error('Submission not found'); | ||
| } | ||
| if (!submission) { | ||
| throw new Error('Submission not found'); | ||
| } | ||
|
|
||
| await prisma.creditLedger.create({ | ||
| data: { | ||
| userId: submission.userId, | ||
| submissionId: submission.id, | ||
| type: CreditEventType.SPAM_PENALTY, | ||
| effectiveMonth: nextMonth, | ||
| change: -1, | ||
| const existingPenalty = await prisma.creditLedger.findFirst({ | ||
| where: { | ||
| submissionId: submission.id, | ||
| type: CreditEventType.SPAM_PENALTY, | ||
| }, | ||
| select: { id: true }, | ||
| }); | ||
|
|
||
| if (existingPenalty) { | ||
| return { created: false }; | ||
| } | ||
|
|
||
| await prisma.creditLedger.create({ | ||
| data: { | ||
| userId: submission.userId, | ||
| submissionId: submission.id, | ||
| type: CreditEventType.SPAM_PENALTY, | ||
| effectiveMonth: nextMonth, | ||
| change: -1, | ||
| }, | ||
| }); | ||
|
|
||
| return { created: true }; | ||
| }, | ||
| }); | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
while IFS= read -r schema; do
echo "=== $schema ==="
rg -n -C 8 'model CreditLedger|@@unique|`@unique`|submissionId|applicationId|type' "$schema"
done < <(fd -HI --type f '^schema\.prisma$' .)
echo '=== allocation conflict handling ==='
rg -n -C 5 'P2002|creditLedger\.(findFirst|create)' \
src/features/credits/utils/allocateCredits.tsRepository: SuperteamDAO/earn
Length of output: 207
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== schema files ==="
fd -HI --type f '^schema\.prisma$' . || true
echo "=== CreditLedger references in schema files ==="
for schema in $(fd -HI --type f '^schema\.prisma$' .); do
echo "=== $schema ==="
rg -n -C 8 'model CreditLedger|@@unique|`@unique`|submissionId|applicationId|type' "$schema" || true
done
echo "=== allocation conflict handling ==="
rg -n -C 5 'P2002|creditLedger\.(findFirst|create)|withRedisLock|create\(' src/features/credits/utils/allocateCredits.ts || true
echo "=== full relevant allocateCredits excerpt ==="
sed -n '1,190p' src/features/credits/utils/allocateCredits.ts | nl -baRepository: SuperteamDAO/earn
Length of output: 36943
Make spam penalty creation idempotent at the database level.
CreditLedger has indexes on submissionId and applicationId, but not composite unique constraints. Add @@unique([submissionId, type]) for submissions and a matching (applicationId, type) constraint for grant applications, then catch Prisma unique-conflict results and return { created: false }.
🤖 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/features/credits/utils/allocateCredits.ts` around lines 77 - 113, Add
composite unique constraints for (submissionId, type) and (applicationId, type)
to the CreditLedger schema, then update the spam penalty creation flow around
withRedisLock to catch Prisma unique-constraint conflicts from
creditLedger.create and return { created: false }; preserve existing behavior
for successful inserts and unrelated errors.
| } catch (error) { | ||
| if (error instanceof LockNotAcquiredError) { | ||
| return { created: false }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not report lock contention as an existing penalty.
LockNotAcquiredError only confirms that another request holds the lock. It does not confirm that the request created a ledger entry. If the lock owner fails before creditLedger.create, these branches return { created: false }. The caller then skips the email and does not retry allocation.
Return a distinct retryable lock result, or retry after the lock is released. Return the duplicate result only after confirming that the penalty exists.
As per coding guidelines, use discriminated unions for mutually exclusive data states.
Also applies to: 160-162
🤖 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/features/credits/utils/allocateCredits.ts` around lines 114 - 116, Update
the LockNotAcquiredError handling in allocateCredits to return a distinct
retryable lock-contention result rather than `{ created: false }`; apply the
same change to the corresponding branch near the later catch block. Model the
mutually exclusive outcomes with a discriminated union, and reserve the
duplicate result for cases where an existing penalty is confirmed.
Source: Coding guidelines
![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: 548aca64a6
ℹ️ 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".
| if (error instanceof LockNotAcquiredError) { | ||
| return { created: false }; |
There was a problem hiding this comment.
When the Redis lock is not acquired, this returns { created: false }, and the submission label API treats that the same as “a penalty already exists” and returns success without queuing/creating anything. If the lock holder times out, crashes, or fails before the ledger insert, later attempts during the 5-minute TTL will leave the record marked Spam with no spam penalty; the same pattern exists in the grant penalty path. Please distinguish lock contention from an already-existing ledger entry, e.g. retry/check after the lock or surface a conflict so the operation can be retried.
Useful? React with 👍 / 👎.
1 participant
Summary
Prevents duplicate credit penalties when a submission or grant application is marked as Spam more than once.
Changes
Validation
npm run lintpassed.git diff --checkpassed, only CRLF warnings.npm run check-typesis currently blocked by unrelated existing type errors outside this change.Summary by CodeRabbit