fix(payment): reuse the same idempotency key when retrying a capture - #16293
fix(payment): reuse the same idempotency key when retrying a capture#16293blockgroot wants to merge 3 commits into
Conversation
capturePayment_ always created a brand-new Capture row before calling the provider, and passed that row's id as the provider idempotency key. On a provider-call failure the capture was deleted, so a retry minted a new id and a new key. If the failure was ambiguous (e.g. a timeout after the provider actually processed the request), the provider has no way to recognize a retry as the same operation and can capture the funds twice. Mark a failed capture instead of deleting it, and reuse it on the next attempt so the idempotency key stays stable across a retry. A capture only becomes reusable once its own call records it as failed; until then it still counts toward the captured total, so a concurrent capture on the same payment can't mistake an in-flight capture for a stale one (this composes with the row lock added in medusajs#16097 for that race, rather than changing it).
🦋 Changeset detectedLatest commit: 83ea23e The changes in this PR will be included in the next version bump. This PR includes changesets to release 79 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Thanks for the contribution! A few items need to be addressed before this can move forward: Solid fix for capture-retry idempotency: PR template complete, changeset present with correct patch bump, integration tests added and passing. One required change: an added code comment inside capturePayment_ contains (see #16097), which is a PR reference embedded in source code — per project conventions these must be removed.
Triggered by: new PR opened |
Reword the over-capture guard comment to explain the row lock inline instead of pointing at a PR number, per review feedback.
|
Thanks for the contribution! Initial automated review looks good. Solid fix for capture-retry idempotency. PR template complete, changeset present with correct patch bump, integration tests added and passing. Prior required change (PR reference in a code comment) has been addressed. No security, performance, or correctness issues found. Triggered by: new commit pushed |
Summary
What — What changes are introduced in this PR?
capturePaymentno longer deletes aCapturerow when the payment provider call fails.Instead the capture is marked
failed, and a subsequent capture attempt on the samepayment reuses that capture's id (and therefore the same provider idempotency key)
instead of minting a fresh one.
Why — Why are these changes relevant or necessary?
capturePayment_forwards a freshly-createdCapturerow's id to the payment provideras its idempotency key (
payment-module.ts,capturePaymentFromProvider_), so that aretried capture is deduplicated by the provider instead of moving money twice. But
because a failed attempt's row was deleted, a retry always got a brand-new id and a new
idempotency key. On an ambiguous provider-side failure — a timeout, or our server dying
between the provider processing the request and us receiving the response — the
provider has no way to recognize a retry as the same operation, and the customer's
payment can be captured a second time.
Fixes #16292
How — How have these changes been implemented?
capturePayment's catch block now updates the capture'smetadatato record__capture_status: "failed"instead of deleting the row.capturePayment_'s over-capture guard now excludes only captures explicitly markedfailedfrom the captured-amount total, and reuses one (updating its amount andresetting its status to
pending) instead of creating a new row when one exists.failedstill counts toward the captured total, soa genuinely concurrent capture on the same payment can't mistake an in-flight capture
(one whose owning call hasn't resolved yet) for a stale, reusable one — this is
important because a first attempt at this fix (excluding any not-yet-confirmed
capture) broke the existing concurrency regression tests added in fix(payment,promotion): serialize concurrent money guards to prevent over-capture, over-refund and budget overspend #16097 by letting a
concurrent call race past the over-capture guard. The final design only treats a
capture as reusable once its own call has recorded it as failed.
capturePaymentFromProvider_clears the status once the provider call actuallysucceeds (or is skipped because the payment was already auto-captured), so a
successful capture is never later mistaken for a reusable failed attempt.
No schema migration — this reuses the existing
metadataJSON column already present onCapture. No public API or HTTP contract change.Testing — How have these changes been tested, or how can the reviewer test the
feature?
New tests in
packages/modules/payment/integration-tests/__tests__/services/payment-module/index.spec.ts:should reuse the same idempotency key when retrying a capture after a provider-side failure— mocks the provider to fail once then succeed, asserts the first attemptthrows and leaves the payment uncaptured, then asserts a retry sends the same
idempotency_keyas the failed attempt and results in exactly one confirmed capture.This fails on
develop(asserts the keys are equal; they currently differ) and passeswith this change.
should capture the full amount normally when no prior attempt failed— confirms theordinary single-capture path is unaffected.
Full
@medusajs/paymentintegration suite: 51/51 passed (includes the existingconcurrency regression tests from #16097 — a first draft of this fix broke those two
tests by excluding not-yet-confirmed captures from the over-capture guard entirely; the
final design fixes that by only excluding captures explicitly marked
failed).yarn lint:medusa: 0 errors (204 pre-existing warnings, unchanged fromdevelop).yarn workspace @medusajs/payment build: passes.Full repo
yarn build: 79/79 tasks.Full repo
yarn test --continue: 67/69 (the 2 failures,@medusajs/iconsand@medusajs/ui, are pre-existing and unrelated to this change — a duplicate Reactversion resolved for the
iconsworkspace on a clean install).Examples
Checklist
Additional Context
The
refundPayment_/refundPaymentpath has the identical shape (create aRefundrow,call the provider, delete the row on failure) and is very likely exposed to the same
class of bug. I've deliberately left it out of this PR to keep the diff scoped to
capture, and I'm happy to open a follow-up issue/PR for it once this one lands, if that's
useful.