fix: prevent double withdrawal of community earnings#835
Conversation
Atomically claim community earnings before scheduling a payout by zeroing them with a conditional compare-and-set. Concurrent withdrawal attempts now race on this update so only one wins, preventing the node from paying out the same earnings more than once. If the payout later fails permanently (invoice expired or payment attempts exhausted), the pending-payments job restores the earnings so the community creator can withdraw again without losing funds. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 11 minutes and 47 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
WalkthroughThe PR adds atomic concurrency protection to community earnings withdrawal. The invoice wizard now snapshots earnings, performs a compare-and-set claim to prevent duplicate withdrawals, and aborts if already claimed. The payment failure handler restores earnings for both expired and exhausted-attempts cases. ChangesConcurrent Earnings Withdrawal Safety
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bot/modules/community/scenes.ts (1)
1042-1049:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMake the claim and pending-payment insert one atomic unit.
Because Line 1031 has already zeroed the community balance, any failure from here through
pp.save()strands the earnings permanently.User.findByIdcan throw,PendingPayment.save()can fail, and the catch path only logs/leaves; without a persistedPendingPayment,attemptCommunitiesPendingPaymentshas nothing to restore later.A transaction or an explicit rollback of the claim is needed here.
🤖 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 `@bot/modules/community/scenes.ts` around lines 1042 - 1049, The code zeroes the community balance then creates a PendingPayment (pp.save()) and looks up User.findById, but these steps are not atomic so failures can permanently strand earnings; wrap the claim/withdrawal and PendingPayment insertion in a single atomic operation (use a MongoDB session/transaction around creating the Claim record, creating/saving the PendingPayment, and any user lookup/updates) or, if transactions are not available, implement an explicit rollback: if User.findById or pp.save() throws, revert the claim by restoring the community balance and deleting any created Claim/partial records so attemptCommunitiesPendingPayments can later restore funds. Ensure the code updates/creates PendingPayment only inside the transaction (or after successful persistence of compensating rollback logic) and surface errors so callers can retry or abort consistently.
🤖 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 `@bot/modules/community/scenes.ts`:
- Around line 1024-1034: The code now atomically zeros community.earnings at
scheduling via Community.findOneAndUpdate (claimed), which conflicts with the
existing success path in jobs/pending_payments.ts that also zeroes
community.earnings on successful payment; update the success handler in
jobs/pending_payments.ts to stop blindly setting earnings = 0 and instead either
(a) leave community.earnings untouched on success and only clear the claimed
amount (e.g. subtract the claimed amount or remove a pendingPayouts entry), or
(b) use a dedicated pendingPayouts/pending_claims field to track the snapshot
and clear only that field on success/failure; also adjust any logic that writes
orders_to_redeem to ensure it reflects only the claimed batch (or document that
it is cumulative) so subsequent saves do not erase earnings accrued after claim.
Ensure references to the claim use the claimed variable returned by
Community.findOneAndUpdate and perform updates with atomic increments/decrements
(e.g., $inc) or targeted field clears rather than overwriting the whole earnings
field.
In `@jobs/pending_payments.ts`:
- Around line 273-283: The restore of claimed earnings is only executed inside
the if (pending.is_invoice_expired || attemptsExhausted) block but currently
happens after the awaited bot.telegram.sendMessage(...) call on the
expired-invoice path, so a thrown sendMessage prevents the restore and leaves
funds locked; move the restore logic so it runs before any external/awaited
notification or, alternatively, wrap the notification in its own try/catch and
perform the restore in a finally or immediately prior to calling
bot.telegram.sendMessage; update the code paths around attemptsExhausted,
pending.is_invoice_expired and the bot.telegram.sendMessage(...) invocation to
ensure the balance-restoration code always executes even if sendMessage fails.
- Around line 284-285: The restore is doing a stale read-modify-write by
mutating community.earnings in memory and calling community.save(); replace this
with a database-side atomic increment (use the community._id and an $inc on the
earnings field) so the update is applied against the current DB value (e.g.,
call the model's updateOne/findByIdAndUpdate with {$inc: { earnings:
pending.amount }}), rather than using community.earnings += pending.amount and
community.save().
---
Outside diff comments:
In `@bot/modules/community/scenes.ts`:
- Around line 1042-1049: The code zeroes the community balance then creates a
PendingPayment (pp.save()) and looks up User.findById, but these steps are not
atomic so failures can permanently strand earnings; wrap the claim/withdrawal
and PendingPayment insertion in a single atomic operation (use a MongoDB
session/transaction around creating the Claim record, creating/saving the
PendingPayment, and any user lookup/updates) or, if transactions are not
available, implement an explicit rollback: if User.findById or pp.save() throws,
revert the claim by restoring the community balance and deleting any created
Claim/partial records so attemptCommunitiesPendingPayments can later restore
funds. Ensure the code updates/creates PendingPayment only inside the
transaction (or after successful persistence of compensating rollback logic) and
surface errors so callers can retry or abort consistently.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cd5e8a16-bf85-42fd-b416-f7e434e01906
📒 Files selected for processing (2)
bot/modules/community/scenes.tsjobs/pending_payments.ts
- Roll back the atomic earnings claim if creating the PendingPayment fails (user lookup or save throws). Without a persisted pending payment the retry job can never restore the claimed earnings, so they would be stranded. - Restore earnings before any notification can abort the run: wrap the expired-invoice notification in try/catch so a failed sendMessage no longer skips the restore and leaves funds locked. - Stop re-zeroing community.earnings on a successful payout; the earnings were already claimed at scheduling time, and resetting them again would wipe out earnings accrued in the meantime. - Use an atomic $inc to restore earnings on permanent failure instead of a stale read-modify-write, so the update applies against the current DB value. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
package.json was bumped to 0.15.2 (commit 056284a) but package-lock.json still declared 0.15.1. The CI 'Run prettier' step runs 'npm install' followed by 'git diff --exit-code', and npm rewrites the lockfile version to match package.json, producing an uncommitted diff that fails the check. Sync the lockfile version so the working tree stays clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@codex review |
2 participants
Summary
Hardens the community earnings withdrawal flow against a race condition that could allow the same earnings balance to be paid out more than once.
Previously, the withdrawal wizard read
community.earnings, scheduled a pending payment, and only later relied on bookkeeping to reduce the balance. Because the read and the payout scheduling were not atomic, two withdrawal attempts processed close together could each observe the same non-zero balance and both proceed, resulting in the node paying out the same earnings twice.Changes
bot/modules/community/scenes.ts— Atomically claim the earnings before scheduling the payout. The community'searningsis zeroed with a conditionalfindOneAndUpdatethat only matches when the balance still equals the snapshotted amount. Concurrent attempts race on this compare-and-set, so only one wins; the loser aborts with the "already being paid" message. The pending payment is created from the snapshotted amount.jobs/pending_payments.ts— When a community payout fails permanently (invoice expired or payment attempts exhausted), restore the claimed earnings so the creator can withdraw again without losing funds. The restore runs exactly once because the pending payment is excluded from subsequent runs once it is expired or out of attempts.Testing
npx tsc --noEmitpasses.Summary by CodeRabbit