A refund race in Medusa: why counting pending refunds overstates refunded_amount

The problem

From the pull request: refundPayment saves the refund row before calling the payment provider. If another refund on the same payment collection succeeds in between, it recomputes the collection total while the doomed row is still there and counts it. The failing refund then soft-deletes its row but does not recompute, so the collection stays overstated, for example 80 instead of 50. Cancelling an order refunds payments in parallel, so this reaches real orders, and it made an order test flaky in CI.

The row lock (SELECT ... FOR UPDATE) is taken while the refund row is created; the provider call runs after it. That is what allows the interleaving.

The proposed fix

Recompute after the soft-delete, then rethrow:

await this.refundService_.softDelete({ id: refund.id }, sharedContext)
// A concurrent refund may have totaled the collection while this row
// was still live, so recompute now that it's excluded.
await this.maybeUpdatePaymentCollection_(payment.payment_collection_id, sharedContext)
throw error

The new test is a good reproduction: the provider mock for a 30 refund first runs a 50 refund, then throws "provider timed out", and the collection must end at 50.

What I found in review

The total counts refunds nobody has confirmed

The cause underneath is in the aggregate. maybeUpdatePaymentCollection_ adds up every refund that is not soft-deleted:

for (const refund of refunds) {
  refundedAmount = MathBN.add(refundedAmount, refund.amount)
}

That includes pending refunds the provider has not confirmed. So refunded_amount is still overstated between the concurrent success and the new recompute, and it stays overstated if the process dies after the provider fails but before the soft-delete. Counting only succeeded refunds would close both gaps.

Do not let cleanup replace the real error

The new recompute runs inside the catch block, before throw error. If it fails (a database error, a lock timeout), its exception replaces the provider's. The caller then sees a database failure instead of the refund rejection it makes decisions on. Catching and logging the recompute error, then rethrowing the original, keeps the failure semantics unchanged.

What happened next

Takeaways

• Write the row first, call the provider second, is a good pattern for idempotency and auditing. It also creates a window where the outcome is unknown, and every aggregate must decide how to treat that window.
• Money totals should be computed from confirmed states.
• A NOT NULL DEFAULT on a status column silently rewrites history. Backfill in the same migration.
• Best-effort work in a catch block must not be able to change which error escapes.