You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Previous design deleted the ProcessedStripeEvent doc on handler exception, so attempts reset to 0 on every Stripe redelivery and the attempts >= 5 dead-letter branch was never reachable. A permanently-broken handler would loop ~50× across Stripe's 3-day retry window, polluting logs and potentially writing partial state.
Fix: don't delete the doc on rollback. attempts persists across redeliveries.
tryRecord now returns 3-state semantics derived from existing fields (no new pendingRetry column — the (attempts, deadLetter) pair already encodes the four states unambiguously):
state
tryRecord result
flow
new doc
{ recorded: true, retry: false }
handler runs
existing, attempts > 0 && !deadLetter
{ recorded: true, retry: true }
handler re-enters
existing, attempts === 0 && !deadLetter
{ recorded: false, reason: 'already_processed' }
skip (succeeded last time — handler never increments on success)
existing, deadLetter: true
{ recorded: false, reason: 'dead_letter' }
skip (terminal — return 200 to Stripe)
withIdempotency: on exception, increment attempts; below MAX → throw (Stripe retries); at MAX → markDeadLetter + return success sentinel so Stripe stops.
charge.dispute.created was handled (log + emit, no debit — conservative). But charge.dispute.funds_withdrawn (the event when Stripe actually pulls money from the bank because the dispute was lost) was unhandled — money-loss gap where the customer kept meter units.
New handleChargeDisputeFundsWithdrawn:
Resolves org/session/pack via charge.metadata, falls back to PaymentIntent backfill on SENTINEL_PENDING (mirrors handleChargeRefunded).
Debits via BillingExtraService.refundPartial(orgId, sessionId, dispute.amount, packId, 'dispute_<id>').
Stable refId per dispute → ledger-layer idempotency makes Stripe redeliveries no-ops.
No per-family event-newer guard — disputes are independent of subscription/invoice cycles, so we deliberately rely on refundPartial's ledger-layer idempotency on refId exclusively (combined with the persistent dead-letter machinery from Fix 1).
Emits billing.dispute.lost on success, billing.refund.unresolved (reason: 'dispute_unresolved') when org/session can't be resolved.
Critical logger.error on the success path — money has actually left the account, ops must be notified.
Test plan
npm run lint — clean
NODE_ENV=test npm run test:unit — 1137/1137 pass
NODE_ENV=test npm run test:integration — 335/335 pass
…nds_withdrawn handler
1. withIdempotency: don't delete the ProcessedStripeEvent doc on handler exception.
`attempts` now persists across Stripe's redelivery cycle, so MAX_ATTEMPTS (5) is
actually reachable. tryRecord uses 3-state semantics derived from existing fields
(no new pendingRetry column):
- first delivery → { recorded: true, retry: false } → handler runs
- in-flight retry → { recorded: true, retry: true } → handler re-enters
- already succeeded → { recorded: false, reason: 'already_processed' } → skip
- dead-lettered → { recorded: false, reason: 'dead_letter' } → skip
The (attempts, deadLetter) pair encodes all four states unambiguously: handler
never increments on success, so attempts === 0 + !deadLetter is the terminal-
success signal. On exception below MAX → throw so Stripe retries on next
delivery; at MAX → markDeadLetter + return success sentinel so Stripe stops.
2. charge.dispute.funds_withdrawn handler: debits the ledger via refundPartial
with stable refId 'dispute_<id>' when Stripe actually withdraws funds (dispute
lost). Closes the money-loss gap where the customer kept meter units after
losing a dispute. Disputes are independent of subscription/invoice cycles, so
no per-family event-newer guard — refundPartial's own ledger-layer idempotency
on refId carries the no-op guarantee on Stripe redelivery.
Resolution path mirrors handleChargeRefunded: charge.metadata first, PI backfill
fallback for SENTINEL_PENDING, emit 'billing.refund.unresolved' when the charge
is unrelated to billing extras, log critical alert + emit 'billing.dispute.lost'
on the success path so ops are notified that money has actually left the account.
@PierreBrisorgueil has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 44 minutes and 5 seconds before requesting another review.
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing.
⌛ How to resolve this issue?
After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.
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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.
✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.39%. Comparing base (7b89555) to head (98eb671). ⚠️ Report is 1 commits behind head on master.
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
The PR introduces the dead-letter mechanism and the charge.dispute.funds_withdrawn handler, but the implementation is currently not up to standards. A critical logic error in the idempotency wrapper (withIdempotency) prevents the attempts counter from resetting after a successful execution on a retry; this will cause duplicate processing if Stripe sends further redeliveries for a successfully handled event.
While the Intent agent confirmed that the basic test scenarios for the dispute handler and repository are present, the handler's metadata resolution logic lacks sufficient validation for malformed ObjectIds. Furthermore, the PR introduces significant code duplication across the service layer and new linting issues in the test suite that must be addressed to meet quality requirements.
About this PR
Current changes do not meet the project's quality standards. Please address the logic duplication and the critical idempotency bug to satisfy the required quality gates.
The PR exhibits a systemic pattern of code duplication in billing.webhook.service.js. Multiple handlers repeat the same try/catch wrapper for emitting events, which should be abstracted into a helper method to improve maintainability and reduce the risk of logging inconsistencies.
Test suggestions
Found: tryRecord: First delivery returns {recorded: true, retry: false}
The reason will be displayed to describe this comment to others. Learn more.
🔴 HIGH RISK
This file is part of a Node.js repository, but the linter is incorrectly applying Qwik-specific serialization rules. This suppression is used elsewhere in the module to handle this false positive.
This might be a simple fix:
Suggested change
conststubExisting=(existing)=>{
// biome-ignore lint/correctness/useQwikValidLexicalScope: false positive — Node.js test, not Qwik
The reason will be displayed to describe this comment to others. Learn more.
🔴 HIGH RISK
Idempotency is violated for retried events because the attempts counter is not reset upon success. After a successful handler execution, you must reset attempts to 0 in the database if the current run was a retry.
Try running the following prompt in your coding agent:
In modules/billing/services/billing.webhook.service.js, update withIdempotency to reset the attempts counter to 0 in the repository after handler(event) succeeds if claim.retry is true. You will also need to implement a resetAttempts(eventId) method in modules/billing/repositories/billing.processedStripeEvent.repository.js.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
Suggestion: The pattern for emitting events with a safe try/catch wrapper is repeated several times in this service (e.g., lines 760-768, 845-859, 878-891). This boilerplate clutters the core logic.
Try running the following prompt in your IDE agent:
Create a private helper function safeEmit(eventName, payload) that wraps billingEvents.emit in a try/catch block with the standard logging seen in handleChargeDisputeFundsWithdrawn, then refactor the event emissions in this file to use it.
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR updates the billing Stripe webhook pipeline to (1) make the dead-letter mechanism reachable by persisting retry attempts across redeliveries and (2) handle charge.dispute.funds_withdrawn by debiting the extras ledger when Stripe withdraws funds after a lost dispute.
Changes:
Reworked webhook idempotency to persist retry attempts, increment on handler failure, and dead-letter after max attempts (instead of rollback deletion).
Added a new webhook handler for charge.dispute.funds_withdrawn to debit the ledger with a stable, dispute-scoped idempotency refId and emit operational events.
Updated/added unit tests to cover the new idempotency state machine and the new dispute handler.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
…n route
Lifts codecov/patch from 73.68% to ~100% on PR #3603 by adding tests for
the previously-uncovered defensive branches in
handleChargeDisputeFundsWithdrawn:
- getStripe() returns null → log + early return
- charge.retrieve failure → log + re-throw (counts toward dead-letter)
- PI fetch failure → log non-fatal + fall through (then unresolved branch)
- PI metadata backfills organizationId when charge.metadata invalid
- emit listener errors swallowed with logger.error in all 3 branches
(missing-fields, unresolved, dispute.lost success)
- controller dispatch test for charge.dispute.funds_withdrawn route
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
Summary
Two post-merge audit fixes on the billing module before LIVE.
Fix 1 — Dead-letter mechanism unreachable (architectural bug)
Previous design deleted the
ProcessedStripeEventdoc on handler exception, soattemptsreset to 0 on every Stripe redelivery and theattempts >= 5dead-letter branch was never reachable. A permanently-broken handler would loop ~50× across Stripe's 3-day retry window, polluting logs and potentially writing partial state.Fix: don't delete the doc on rollback.
attemptspersists across redeliveries.tryRecordnow returns 3-state semantics derived from existing fields (no newpendingRetrycolumn — the(attempts, deadLetter)pair already encodes the four states unambiguously):{ recorded: true, retry: false }attempts > 0 && !deadLetter{ recorded: true, retry: true }attempts === 0 && !deadLetter{ recorded: false, reason: 'already_processed' }deadLetter: true{ recorded: false, reason: 'dead_letter' }withIdempotency: on exception, increment attempts; below MAX → throw (Stripe retries); at MAX →markDeadLetter+ return success sentinel so Stripe stops.Fix 2 — Add
charge.dispute.funds_withdrawnwebhook handlercharge.dispute.createdwas handled (log + emit, no debit — conservative). Butcharge.dispute.funds_withdrawn(the event when Stripe actually pulls money from the bank because the dispute was lost) was unhandled — money-loss gap where the customer kept meter units.New
handleChargeDisputeFundsWithdrawn:charge.metadata, falls back to PaymentIntent backfill onSENTINEL_PENDING(mirrorshandleChargeRefunded).BillingExtraService.refundPartial(orgId, sessionId, dispute.amount, packId, 'dispute_<id>').refIdper dispute → ledger-layer idempotency makes Stripe redeliveries no-ops.refundPartial's ledger-layer idempotency onrefIdexclusively (combined with the persistent dead-letter machinery from Fix 1).billing.dispute.loston success,billing.refund.unresolved(reason: 'dispute_unresolved') when org/session can't be resolved.logger.erroron the success path — money has actually left the account, ops must be notified.Test plan
npm run lint— cleanNODE_ENV=test npm run test:unit— 1137/1137 passNODE_ENV=test npm run test:integration— 335/335 passtryRecord(deadLetter / in-flight retry / already_processed / race-window)withIdempotencytests to assert no rollback on failure (doc must persist)handleChargeDisputeFundsWithdrawn: fully-resolved debit, sentinel backfill, unrelated-charge unresolved emit, idempotent refId per dispute, missing-fields/zero-amount guards