fix: bind idempotency key identity to the request, not just its route scope - #77
Merged
chonilius merged 7 commits intoAug 17, 2026
Conversation
Nullable hash column that will bind an idempotency key's identity to the actual request (path params + body), not just its route scope (MergeFi#54). Nullable so existing pre-migration rows are treated as predating the check rather than breaking during the deploy transition — see the follow-up commit wiring this into IdempotencyInterceptor.
Maps the column added in the previous migration. Immutable once set — a row represents one specific operation for its whole lifetime.
(key, scope, callerId) alone can't distinguish "retry of the same operation" from "different operation that happens to share a key" — scope is a static string per route, the same for every request regardless of path params or body (MergeFi#54). resolveExisting now compares a SHA-256 fingerprint of path params + body against the fingerprint stored on the existing row and rejects with 422 on a mismatch, before ever reaching the COMPLETED-replay or PROCESSING/staleness logic — a mismatch means this was never "the same operation" regardless of what state the true owner's row is in. A null stored fingerprint (a row that predates this column) is exempt from the check rather than treated as a guaranteed mismatch, so the migration deploy transition doesn't 422 an in-flight legitimate retry. Path params are folded into the body-hash comparison rather than into `scope` itself, so scope stays a purely route-level concept and one mechanism covers both params and body — see the class doc comment.
…i#54) FakeIdempotencyRepo now stores requestFingerprint (defaulting to null like an unspecified nullable Postgres column would, not undefined — needed so rows seeded directly in tests correctly model a pre-MergeFi#54 row). createContext accepts params/body so tests can construct requests that differ by resource or payload.
Covers the exact scenario the issue describes: the same Idempotency- Key sent to two requests with different path params (and separately, different bodies) on the same scope is rejected with 422 rather than replaying the first request's cached response — the second resource's handler is never invoked, matching the "escrow B was never actually released despite an apparently-successful response" failure mode from the issue. Also covers the null-fingerprint escape hatch for pre-MergeFi#54 rows and confirms a genuinely identical retry (same params + body) still replays correctly.
Hits EscrowController's real POST /escrow/:id/release route (mocked EscrowService, in-memory idempotency repo, real IdempotencyInterceptor) to prove the fix end-to-end rather than only at the interceptor-unit level: reusing an Idempotency-Key across escrow A and escrow B returns 422 for B and EscrowService.release is never called for B, while retrying the same key against the same escrow/body still replays correctly. Matches the issue's own reproduction plan. (Includes a prettier line-length fix on the previous migration file, picked up by `npm run lint --fix` while working on this commit.)
Explains the 422 mismatch behavior and the concrete cross-resource replay scenario it closes, alongside the existing 409/TTL/cleanup documentation.
|
@presidojay1 is attempting to deploy a commit to the chonilius' projects Team on Vercel. A member of the Team first needs to authorize it. |
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.
Summary
IdempotencyInterceptordetermined whether a request was a replay purely from(Idempotency-Key, route scope, callerId)—scopeis a static string per route (e.g.'escrow.release'), the same for every request to that route regardless of path params or body. Reusing an Idempotency-Key across two different resources on the same route (e.g.POST /escrow/A/releasethenPOST /escrow/B/releasewith the same key) silently replayed A's cached response for B: the client saw an apparently-successful release, but B's real handler was never invoked and its funds never moved.IdempotencyKeygains a nullablerequestFingerprintcolumn (migrationAddIdempotencyKeyRequestFingerprint1784500000000) — a SHA-256 hash of the request's path params + body (deterministic key-sorted JSON, so key-order differences between logically-identical bodies don't false-positive).resolveExistingcompares the incoming request's fingerprint against the one stored on the existing row and rejects with422 Unprocessable Entityon a mismatch — checked before the COMPLETED-replay or PROCESSING/staleness logic, since a mismatch means this was never "the same operation" regardless of the true owner's row state.nullstored fingerprint (a row that predates this column) is exempt from the check, so the migration deploy transition doesn't 422 an in-flight legitimate retry.scopeitself, soscopestays a purely route-level concept — see the class doc comment for the reasoning.Closes #54
Test plan
npm run lint— clean (0 errors)npx tsc --noEmit— cleannpm run build— succeedsnpm run test— 125/125 pass, including new coverage inidempotency.interceptor.spec.ts: reusing a key across two different resources (and separately, two different bodies) is rejected with 422 without the second handler ever running; a genuinely identical retry (same params + body) still replays correctly; a pre-migration row with no stored fingerprint is exempt from the checknpm run test:e2e(test/escrow-idempotency.e2e-spec.ts, new) — hitsEscrowController's realPOST /escrow/:id/releaseroute end-to-end (mockedEscrowService, realIdempotencyInterceptor) and directly reproduces the issue's exact scenario: reusing escrow A's key against escrow B returns 422 andEscrowService.releaseis never called for Bup()/down()SQL directly against a real local Postgres (adds/dropsrequestFingerprintcleanly)