Skip to content

fix(refunds): replay committed refund when queued idempotent retry wakes after commit #1255

Closed

Description

@khaira777

Summary

In POST /api/refunds, when a matching idempotent retry arrives while the original refund request is awaiting asynchronous PIN verification (verifyPinAsync) and queues behind withRefundPinLock, the retry does not re-check refund_idempotency after the original request commits. If the retry carries a mistyped/stale override_pin or encounters the PIN rate limit, it returns 403 approver_pin_invalid (or 429 pin_rate_limited) with outcome: 'not_applied' even though the refund has already committed.

Verified Reproduction

On 08187174bfa47f08154e2b9b64cdb17b0ef2c459:

  1. Submit Request 1 to POST /api/refunds with a valid manager PIN and Idempotency-Key: K. Request 1 enters withRefundPinLock and yields during verifyPinAsync.
  2. Before Request 1 commits, submit Request 2 from the same user with the same Idempotency-Key: K and identical refund payload (bill_id, amount, method, approver_id), but a stale/invalid override_pin.
  3. Request 2 misses the pre-lock refund_idempotency check at main/routes/refunds.ts:246-265 and queues on withRefundPinLock (main/routes/refunds.ts:292).
  4. Request 1 finishes verifyPinAsync, commits the refund and refund_idempotency record inside withTxn, and returns 201.
  5. Request 2 acquires withRefundPinLock, evaluates verifyPinAsync without re-checking refund_idempotency, increments the IP PIN rate-limit counter, and returns 403 Invalid manager PIN (outcome: 'not_applied').

Root Cause

main/routes/refunds.ts checks refund_idempotency at Step 1 (lines 246-265) prior to withRefundPinLock, and createRefund checks refund_idempotency inside withTxn (main/services/refund.ts:294-318), but withRefundPinLock (main/routes/refunds.ts:292-328) never re-checks refund_idempotency upon acquiring the lock or before recording a PIN verification failure.

Expected Behavior & Chosen Design

Re-check refund_idempotency inside withRefundPinLock (before checking isPinRateLimited or running verifyPinAsync, and before recording a PIN failure if a concurrent cross-IP request committed during verifyPinAsync):

  • If a committed refund_idempotency row exists for (userId, idempotencyKey):
    • Enforce String(prior.bill_id) === String(billId) && prior.request_hash === requestHash (throwing 409 idempotency_conflict on mismatch).
    • Return the stored 201 response without incrementing checkPinRateLimit or re-running withTxn.

Acceptance Criteria

  • A queued retry with the same (userId, idempotencyKey) and matching bill_id/request_hash that wakes after the original request commits returns 201 with the committed refund response, even if the retry's override_pin is stale/invalid or the IP is at the rate-limit threshold.
  • A queued retry with the same (userId, idempotencyKey) but a conflicting payload (bill_id or request_hash mismatch) returns 409 idempotency_conflict.
  • If the first request fails and does not commit to refund_idempotency, subsequent requests with the same Idempotency-Key still enforce full PIN rate limiting and PIN verification.

Required Regression Tests

  • Concurrent original refund (valid PIN) + queued matching retry (invalid/stale PIN) returns 201 for both with identical refund ID, 1 DB refund row, and no failed-PIN rate-limit penalty.
  • Concurrent original refund + queued retry with mismatched payload returns 409 idempotency_conflict.
  • Failed first attempt (invalid PIN) + second attempt (invalid PIN) with same Idempotency-Key both return 403 and enforce rate limiting.

Related Historical Context & Considerations

Activity

  1. added a commit that references this issue on Oct 21, 2026
    a0206d0
  2. added a commit that references this issue on Oct 21, 2026
    5d532c8
  3. khaira777 commented on Oct 21, 2026

    @khaira777
    ContributorAuthor

    Post-Merge Verification & Closure (N3)

    • Resolved via PR: fix(main): replay committed refund idempotency in PIN lock #1259
    • Reviewed Branch Head SHA: 20280c97b4cf64f82e3acdb99676004e1652d731
    • Merged main Commit SHA: 5d532c8c278dd1d696df6eb40fd2f938e0675442
    • Root Cause: In POST /api/refunds (main/routes/refunds.ts), when a concurrent or queued idempotent retry with the same (userId, idempotencyKey) waited on withRefundPinLock while the original request completed verifyPinAsync and committed to refund_idempotency, the waking retry did not re-query refund_idempotency before evaluating isPinRateLimited or verifyPinAsync.
    • Fix Summary: Added a post-lock refund_idempotency lookup immediately upon entering withRefundPinLock before isPinRateLimited and verifyPinAsync. If a committed record exists for (userId, idempotencyKey) with matching bill_id and request_hash, the stored 201 response is replayed immediately without re-verifying or recording a failed PIN attempt; mismatched bill_id or request_hash throws 409 idempotency_conflict.
    • Regression Tests Verified: tests/async-refund-pin-event-loop.test.ts (40/40 assertions passed, covering concurrent wrong-PIN retry replay, rate-limited retry replay, and mismatched payload 409 idempotency_conflict).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions