Skip to content

fix(idempotency): reserve keys before execution and never cache 5xx - #1865

Open
devogechukwu wants to merge 1 commit into
Streampay-Org:mainfrom
devogechukwu:fix/idempotency-reservation
Open

devogechukwu wants to merge 1 commit into
Streampay-Org:mainfrom
devogechukwu:fix/idempotency-reservation

Conversation

@devogechukwu

Copy link
Copy Markdown

Closes #1721

Summary

Implements atomic reservation, commit, and release semantics for mutating routes (settle and withdraw), preventing concurrent execution races and stopping transient 5xx responses from poisoning idempotency caches for 24 hours.

Protocol Sequence Diagram

Client Server / Route Idempotency Store
│ │ │
│── POST /api/streams/:id/settle ────────────────────────────────────►│
│ (Idempotency-Key: K) │ │
│ │── 1. Reserve key (setIfAbsent) ────►│
│ │◄─ [in_flight, match] ───────────────┤ (If in-flight: 409 IDEMPOTENCY_IN_FLIGHT)
│ │◄─ [in_flight, stale] ───────────────┤ (Takeover if > IDEMPOTENCY_INFLIGHT_TTL_MS)
│ │◄─ [committed, match] ───────────────┤ (Replay cached response with 200)
│ │◄─ [acquired] ───────────────────────┤
│ │ │
│ │── 2. Execute Handler ───────────────┤
│ │ │
│ ├─── Status = 2xx or deterministic 4xx┤
│ │ (400, 404, 409, 422) │
│ │── COMMIT ──────────────────────────►│ (TTL = 24 hours)
│ │ │
│ ├─── Status = 5xx, 429, or throw ─────┤
│ │── RELEASE (Delete reservation) ────►│ (Enables retry with same key)
│◄── Response ──────────────────┤ │

Key Changes

  • Reserve Protocol (app/lib/idempotency.ts):
    • Atomically reserves idempotency keys with { state: "in_flight", fingerprint, reservedAt, expiresAt } via setIfAbsent.
    • On active in-flight match, returns 409 with code IDEMPOTENCY_IN_FLIGHT and Retry-After: 1.
    • On fingerprint mismatch, returns 409 with code IDEMPOTENCY_CONFLICT.
    • Automatically takes over abandoned reservations older than IDEMPOTENCY_INFLIGHT_TTL_MS (default 30s) and logs takeover with correlation context.
    • Fails closed with 503 IDEMPOTENCY_STORE_UNAVAILABLE if the store read/reservation fails.
  • Commit & Release Semantics:
    • Commits only 2xx and deterministic 4xx (400, 404, 409, 422) with 24h TTL.
    • Releases the reservation on 5xx (e.g. 502 SETTLEMENT_FAILED), 429, or thrown errors so retries re-execute.
  • Un-quarantined Concurrency Suite (app/api/streams/concurrency.test.ts):
    • Removed from jest.config.js quarantine.
    • Added concurrent settle test rows.
    • Reset rate limiter store between tests to prevent false-positive 429s.
  • OpenAPI Docs (openapi.json):
    • Documented retry and caching semantics for settle and withdraw.
  • Test Suite (app/lib/idempotency.reservation.test.ts):
    • Added coverage for scenarios 1–9.
    • Coverage for app/lib/idempotency.ts is 96.69% (≥95%).

Test Output

PASS app/lib/idempotency.reservation.test.ts
  idempotency reservation, commit and release protocol
    √ Scenario 1: Two concurrent settles with same key execute handler once and return 200 / 409 IDEMPOTENCY_IN_FLIGHT (186 ms)
    √ Scenario 2: Handler returns 502 -> Nothing persisted; retry with the same key re-executes (4 ms)
    √ Scenario 3: Handler throws -> Reservation released; retry re-executes (31 ms)
    √ Scenario 4: Handler returns 409 INVALID_STREAM_STATE -> Persisted; replay returns 409 (4 ms)
    √ Scenario 5: Reservation older than in-flight TTL -> Next request takes over and executes (4 ms)
    √ Scenario 6: Store get throws -> 503 IDEMPOTENCY_STORE_UNAVAILABLE; handler not called (8 ms)
    √ Scenario 7: 429 from an inner rate limit -> Not persisted; retry executes handler (3 ms)
    √ Scenario 8: Committed 200, then a concurrent duplicate -> Duplicate gets 200 replay (29 ms)
    √ covers corrupted or expired record eviction in createNamespacedStore get (3 ms)
    √ covers plain string response body committing (15 ms)
    √ covers in-flight reservation with mismatched fingerprint returning 409 IDEMPOTENCY_CONFLICT (4 ms)
    √ covers store.setIfAbsent throwing and failing closed with 503 (4 ms)
    √ covers error logging on store.delete or store.set failures (14 ms)
PASS app/api/streams/concurrency.test.ts
  √ two concurrent pauses with the same Idempotency-Key produce one state change (43 ms)
  √ two concurrent pauses with different Idempotency-Keys: one succeeds, one gets 409 (10 ms)
  √ concurrent pause and start: both succeed sequentially in lock (11 ms)
  √ concurrent pause and stop: the stop leaves the stream ended (11 ms)
  √ concurrent pause and settle: both succeed sequentially in lock (19 ms)
  √ repeated pause with same Idempotency-Key does not mutate state twice (8 ms)
  √ pause on active stream transitions status to paused (5 ms)
  √ pause on non-existent stream returns 404 (3 ms)
  √ pause on already-paused stream is an idempotent success (2 ms)
  √ pause on ended stream returns 409 (2 ms)
  √ N concurrent pauses without idempotency key: exactly one succeeds (25 ms)
  √ two concurrent settles with same idempotency key: one succeeds, one gets 409 in flight or replay (7 ms)
----------------|---------|----------|---------|---------|-------------------
File            | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
----------------|---------|----------|---------|---------|-------------------
 idempotency.ts |    95.2 |    78.43 |     100 |   96.69 | 183,271,392-395   
----------------|---------|----------|---------|---------|-------------------

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reserve idempotency keys before executing settle/withdraw and stop caching 5xx responses for 24 hours

1 participant