fix(billing): make top-up charges idempotent across client retries - #3449
Merged
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3449 +/- ##
==========================================
- Coverage 72.90% 72.44% -0.47%
==========================================
Files 1133 1069 -64
Lines 29492 27720 -1772
Branches 7395 7048 -347
==========================================
- Hits 21502 20082 -1420
+ Misses 7035 6723 -312
+ Partials 955 915 -40
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (28)
🚧 Files skipped from review as they are similar to previous changes (20)
📝 WalkthroughWalkthroughAdds end-to-end Stripe payment idempotency handling across API and web: client keys are namespaced, persisted with de-duplication, replayed safely with amount validation, and surfaced through API routing, service orchestration, repository atomicity, and web retry/polling flows with updated error messaging. ChangesStripe payment idempotency
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)apps/api/drizzle/meta/0032_snapshot.jsonTraceback (most recent call last): apps/api/drizzle/meta/_journal.jsonTraceback (most recent call last): 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
apps/api/test/functional/stripe-transactions-confirm.spec.ts (1)
50-80: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExplicitly assert that replay sends no PaymentIntent POST.
The row-count assertion proves database deduplication, not that Stripe was never contacted. Add a
/v1/payment_intentsinterceptor and assert it remains unused.Proposed assertion
+ const createPaymentIntent = nock("https://api.stripe.com").post("/v1/payment_intents").reply(500); + const response = await confirmPayment(token, { userId: user.userId!, paymentMethodId, amount: 20, idempotencyKey: clientKey }); expect(response.status).toBe(200); + expect(createPaymentIntent.isDone()).toBe(false);As per path instructions, “Verify meaningful assertions, not just snapshot coverage.”
🤖 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 `@apps/api/test/functional/stripe-transactions-confirm.spec.ts` around lines 50 - 80, Update the replay test around confirmPayment to register a nock interceptor for the Stripe POST endpoint /v1/payment_intents and retain its request handle. After confirming the already-credited transaction, assert that this interceptor was not used, alongside the existing response and repository assertions.Source: Path instructions
🤖 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 `@apps/api/src/billing/services/stripe/stripe.service.spec.ts`:
- Around line 248-256: Update the idempotency-parameter mismatch handling in the
payment-intent creation flow, including the paths covered by the tests around
createPaymentIntent, to return a distinct definitive 4xx error/code such as
idempotency_key_mismatch. Apply this consistently to both amount mismatches and
Stripe-reported parameter mismatches, rather than
PAYMENT_IN_PROGRESS_ERROR_MESSAGE or a raw Stripe error, while preserving the
no-Stripe-call and no-row-update behavior.
In `@apps/api/src/billing/services/stripe/stripe.service.ts`:
- Around line 245-275: Move `#ensureReusedKeyAmountConsistency` immediately after
the isNew early return so every reused transaction is validated before settled
and recorded-PaymentIntent replay branches. Update the permanent amount-mismatch
handling in the reused-key flow to return a non-retryable failure rather than
classifying the transaction as still processing, while preserving normal replay
behavior for matching amounts.
- Around line 299-330: Update the payment-status handling in the surrounding
Stripe service method, including the success, requires-action, and
default/decline paths, to inspect the result of updateByIdUnlessSettled(). When
it returns undefined, re-fetch the transaction and use its settled status and
persisted outcome instead of returning stale transaction data or throwing the
stale decline; preserve the existing behavior when the update succeeds.
In
`@apps/deploy-web/src/components/billing-usage/AddCreditsForm/AddCreditsForm.tsx`:
- Line 371: Update the polling flow surrounding clearAttempt so exhaustion is
distinguished from confirmed payment success; propagate an explicit
success/timeout outcome to the caller, invoke onDone only for confirmed
settlement, and clear the idempotency key only on success so timed-out payments
retain it for safe retry handling.
- Around line 91-92: Update ATTEMPT_MAX_AGE_MS used by the AddCreditsForm
payment-attempt flow so unresolved attempts are retained for at least Stripe’s
24-hour idempotency window instead of expiring after one hour. Preserve key
reuse until that age threshold or a confirmed terminal payment state, whichever
comes first.
---
Nitpick comments:
In `@apps/api/test/functional/stripe-transactions-confirm.spec.ts`:
- Around line 50-80: Update the replay test around confirmPayment to register a
nock interceptor for the Stripe POST endpoint /v1/payment_intents and retain its
request handle. After confirming the already-credited transaction, assert that
this interceptor was not used, alongside the existing response and repository
assertions.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 73edb3d9-8bd3-480b-99aa-47397ed109ed
📒 Files selected for processing (26)
apps/api/drizzle/0032_massive_wolverine.sqlapps/api/drizzle/meta/0032_snapshot.jsonapps/api/drizzle/meta/_journal.jsonapps/api/src/billing/controllers/stripe/stripe.controller.spec.tsapps/api/src/billing/controllers/stripe/stripe.controller.tsapps/api/src/billing/http-schemas/stripe.schema.tsapps/api/src/billing/model-schemas/stripe-transaction/stripe-transaction.schema.tsapps/api/src/billing/repositories/stripe-transaction/stripe-transaction.repository.integration.tsapps/api/src/billing/repositories/stripe-transaction/stripe-transaction.repository.tsapps/api/src/billing/routes/stripe-transactions/stripe-transactions.router.tsapps/api/src/billing/services/stripe-error/stripe-error.service.spec.tsapps/api/src/billing/services/stripe-error/stripe-error.service.tsapps/api/src/billing/services/stripe-webhook/stripe-webhook.service.integration.tsapps/api/src/billing/services/stripe/stripe.service.spec.tsapps/api/src/billing/services/stripe/stripe.service.tsapps/api/test/functional/stripe-transactions-confirm.spec.tsapps/api/test/seeders/database-stripe-transaction.seeder.tsapps/deploy-web/src/components/billing-usage/AddCreditsForm/AddCreditsForm.spec.tsxapps/deploy-web/src/components/billing-usage/AddCreditsForm/AddCreditsForm.tsxapps/deploy-web/src/context/PaymentPollingProvider/PaymentPollingProvider.spec.tsxapps/deploy-web/src/context/PaymentPollingProvider/PaymentPollingProvider.tsxapps/deploy-web/src/queries/usePaymentQueries.tsapps/deploy-web/src/utils/stripeErrorHandler.spec.tsapps/deploy-web/src/utils/stripeErrorHandler.tsapps/deploy-web/tests/unit/setup.tspackages/http-sdk/src/stripe/stripe.types.ts
ygrishajev
approved these changes
Jul 20, 2026
The confirm endpoint created a fresh transaction row and PaymentIntent on every delivery, so a user retrying a slow but successful charge paid twice and the webhook credited the wallet twice. The client now mints an attempt key (kept through unknown-outcome failures, remounts via sessionStorage, and polling timeouts; rotated only on completion, a definitive 402 decline, or param changes) and the API finds or creates the transaction row by that key, short-circuits settled rows, resumes a recorded PaymentIntent from its live status instead of creating a second one, never downgrades webhook-written statuses, and maps concurrent key reuse to a 409.
The 30s balance-polling timeout showed a warning ("Payment processing
timeout / refresh the page") that reads as a failure when the charge is
usually still settling server-side. That framing invited the retry that
produced the duplicate top-ups in CON-684. Reword per-variant to an info
"still processing / no need to pay again" notice so a slow-but-successful
charge is not mistaken for a failure.
- reject reused keys whose amount changed before every replay branch with a definitive 409 idempotency_key_mismatch instead of payment-in-progress - respond from the settled row when a webhook wins the race against the request-path status update - retain client attempt keys for Stripe's full 24h replay window - treat polling exhaustion as unknown outcome: keep the attempt key and defer onDone until settlement is confirmed
… reach clients The hono error handler only read error.data.errorCode, which createError never populates, so every billing 409 collapsed to the status-derived "conflict" code and clients could not tell an in-progress replay from a definitive idempotency_key_mismatch. Falls back to the errorCode/errorType properties createError actually sets.
baktun14
force-pushed
the
fix/billing-idempotent-topup-charges
branch
from
July 21, 2026 10:05
c6a0dc7 to
daa0ab5
Compare
This was referenced Jul 21, 2026
Merged
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Why
Customers were charged 2-3x for a single credit purchase (Jul 15-16 clusters: same customer, same amount, distinct successful PaymentIntents minutes apart).
POST /v1/stripe/transactions/confirmcreated a freshstripe_transactionsrow and a fresh PaymentIntent (confirm: true) on every delivery with no idempotency key, so a user retrying a slow-but-actually-successful charge fired a second independent charge, and each PaymentIntent's webhook credited the wallet again.Fixes CON-684
What
Client (deploy-web)
AddCreditsFormmints acrypto.randomUUID()attempt key per purchase attempt and sends it asidempotencyKey. The key survives unknown-outcome failures (network errors, 4xx/5xx, the polling-timeout path from the incident, 3DS failures) and remounts (sessionStorage, matched on user + amount + payment method, 24h max age to match Stripe's replay window). It rotates only when polling confirms settlement, a definitive 402 decline or 409idempotency_key_mismatcharrives, or the purchase params change.transactionStatus === "succeeded") polls from the pre-charge baseline so it settles against the already-credited balance instead of timing out.PaymentPollingProviderexposes the terminal poll outcome; a poll that exhausts without confirming settlement is treated as unknown, not complete: the form releases its in-flight state but keeps the attempt key and skipsonDone, so the parent success screen never shows for an unconfirmed charge and a retry replays the same attempt.API
stripe_idempotency_keycolumn + partial unique index (migration 0032, auto-applies at boot; verified on a live boot against a fresh database).topup_<userId>_<clientKey>(uuid-validated, cross-user collision-proof, disjoint fromWalletBalanceReloadCheck.*andcard_validation_*).StripeService.createPaymentIntentkeyed path: find-or-create the row by key (insert race resolved via conflict-do-nothing + winner re-fetch), short-circuit rows the webhook already settled (succeeded/refunded), and when the row already records a PaymentIntent, retrieve it and act on its live status instead of ever creating a second one (succeeded/processing/requires_capture resume, requires_action returns the live client_secret, declined synthesizes a 402 with the real decline reason).updateByIdUnlessSettled(atomicUPDATE ... WHERE status NOT IN ('succeeded','refunded')), so a slow request can never downgrade a status the webhook wrote first.succeededand nowrequires_captureare deferred to the webhook, protecting the exactly-once crediting guard.topup_keys with a definitive 409idempotency_key_mismatch— distinct from the retryable in-progress 409 — so a replay can never report success for a different amount than was charged; wallet auto-reload keys charge the recorded amount instead, because pg-boss redeliveries reuse the job id but recompute live amounts. Stripe's own params-mismatch (StripeIdempotencyError) maps to the same code.updateByIdUnlessSettledreturns undefined because a webhook settled the row mid-request), the response is derived from the settled row instead of the request's stale view, so an already-credited charge is reported as succeeded rather than pending/failed.idempotency_key_in_use(concurrency loser; arrives asinvalid_request_errorin stripe-node 19, detected viaerror.code) maps to 409payment_in_progresswithout touching the row.idempotencyKeyis optional).PAYMENT_INTENT_KEY_REUSED,PAYMENT_INTENT_REPLAY_SHORT_CIRCUIT,PAYMENT_INTENT_KEY_IN_USE,PAYMENT_INTENT_KEY_AMOUNT_MISMATCH.Migration note: additive nullable column + partial unique index on a NULL-heavy column; no table rewrite, safe to apply on production without blocking.
Quirk fixed along the way:
createError(status, msg, { errorCode })puts props on the error object while the hono handler only serializederror.data?.errorCode, so every billing error collapsed to a status-derived code (conflict,unknown_error). The handler now falls back to theerrorCode/errorTypeproperties, so the curated billing codes (card_declined,payment_in_progress,idempotency_key_mismatch, …) actually reach clients; the client still handlesconflictand the bare 409 status as fallbacks.Accepted residuals (documented in the plan):
Tests: service keyed-path specs (replay, retrieve-guard per live status, amount-mismatch policy, key-in-use, guarded failed writes), repository integration specs against real Postgres (find-or-create race via
Promise.all, settled-guard no-ops), controller namespacing specs, error-mapping specs, a functional test that nock-matches theIdempotency-Keyheader Stripe receives end-to-end over HTTP plus a replay short-circuit functional test, and 12AddCreditsFormscenarios capturing actual keys across retries, remounts, 3DS failure, and rotations.Ops follow-up (not in this PR): identify and refund the Jul 15-16 duplicates (read-only SQL in the plan doc); refunding the duplicate PI reconciles the double credit via the existing
handleChargeRefundedclawback. Follow-up once old bundles turn over: consider makingidempotencyKeyrequired.Summary by CodeRabbit