Skip to content

[Bug] idempotent() claims the key with the response TTL, deletes the claim on a throw and caches every status, so a crash wedges the key for 24 h, a partially applied handler is re-executed and a 503 is pinned #984

Description

@pathosDev

Problem

idempotent() implements the Stripe pattern's three states but reuses one TTL and one lifecycle for two different things — the short-lived lock on an in-flight request and the long-lived record of a completed one. Three defects follow, each of which turns a routine failure into a durable one:

  1. A crash between claim and completion wedges the key for the full response TTL. The in-flight marker is written with ttlMs — 24 h by default. If the worker dies (pod eviction, OOM, rolling deploy) after setIfAbsent and before cache.set, nothing ever clears it and nothing renews it either. Every retry of that idempotency key gets 409 Conflict for a day. The doc says "we respond 409 Conflict so the client retries later"; there is no later.

  2. A handler that threw after applying a side effect is re-executed. The catch deletes the claim so "the client can retry" — but the handler is the thing that charged the card. A payment handler that captures the charge and then fails on the receipt email leaves no record, and the retry runs the whole handler again. The one case idempotency exists to prevent — a double charge on retry — is the case the error path creates.

  3. Every status is cached, including transient ones. There is no cacheStatuses filter (unlike cached() in the same directory, which has one). A 503 from a downstream dependency is stored under the key for ttlMs, so the client that retries correctly gets the same 503 replayed for 24 h without the handler ever running again.

A fourth, narrower point: identity is optional and unset by default, so the key space is global across callers. The requestFingerprint check does bound this — a caller replaying someone else's key with a different body gets 422, not the response — so it is not a general cross-caller read primitive. It does mean that two callers who legitimately generate the same key value for the same method + path + body (sequence numbers, deterministic client-side ids) share a response.

Evidence

The claim uses the response TTL as its lease, src/http/cache/IdempotencyKey.ts:101-109:

src/http/cache/IdempotencyKey.ts:101-109
      // Try to claim the key.  `setIfAbsent` is the kernel — if it
      // returns false, somebody else got there a microsecond ago, fall
      // back to the same in-flight response.
      const claimed = await resolvedOptions.cache.setIfAbsent(cacheKey, IN_FLIGHT_MARKER, ttlMs);
      if (!claimed) {
        return complete(Status.Conflict, {
          error: 'idempotency-key in-flight; retry shortly',
        });
      }

The error path and the unconditional store, src/http/cache/IdempotencyKey.ts:111-123:

src/http/cache/IdempotencyKey.ts:111-123
      let response: HttpResponse;
      try {
        response = await handler(request);
      } catch (e) {
        // On error, drop our in-flight claim so the client can retry.
        await resolvedOptions.cache.delete(cacheKey);
        throw e;
      }
      // Replace the in-flight marker with the actual response,
      // remembering the request fingerprint so subsequent replays
      // can verify the request body matches.
      await resolvedOptions.cache.set<CachedResponse>(cacheKey, encodeResponse(response, fingerprint), ttlMs);
      return response;

ttlMs is one value for both roles, src/http/cache/IdempotencyKey.ts:56:

src/http/cache/IdempotencyKey.ts:56-56
  const ttlMs = resolvedOptions.ttlMs ?? 24 * 60 * 60_000;

For contrast, the sibling middleware does filter what it stores, src/http/cache/ResponseCache.ts:43-48:

src/http/cache/ResponseCache.ts:43-48
  /**
   * Status codes whose responses are cacheable.  Default `[200]` — only
   * 2xx are cached.  Pass `[200, 404]` if you want to cache "not found"
   * responses (saves repeat lookups when callers query unknown ids).
   */
  readonly cacheStatuses?: ReadonlyArray<number>;

Proposal

  • Split the lock from the record. lockTtlMs (seconds to low minutes, default something like 60 s) for the in-flight marker; ttlMs for the stored response. An abandoned claim then self-heals in a minute instead of a day. Renew the lock from the handler for long-running work, or accept the lock TTL as the handler timeout and document it as such.
  • Do not delete the claim on a throw. Write a terminal failed record instead — or, at minimum, make the behaviour configurable (onHandlerError: 'release' | 'retain') and default to retaining, because retaining is the safe side for a handler with side effects. Deleting is only correct for a handler that is known to be atomic, which the middleware cannot know.
  • Add cacheStatuses (default 2xx + 4xx, excluding 5xx and 429) so a transient failure is not pinned. ResponseCache already has the exact option and default shape to copy.
  • Consider making identity required (or defaulted to something the framework can derive) so the key space is caller-scoped by construction, and lead the docs with it.

Acceptance sketch

  • The in-flight marker carries its own short TTL, independent of the response TTL.
  • A handler that throws does not silently permit a full re-execution — the behaviour is configurable and the safe option is the default.
  • 5xx responses are not stored by default.
  • A test covers: crash between claim and completion; handler throws after a side effect; a 503 followed by a retry.
  • The docs (EN + DE) state what happens to a partially applied handler.

Verification status

Found in the ten-lens production-readiness review of 2026-08-05 (v0.13.0) and re-verified before filing: reproduced by execution, using the real idempotent() over InMemoryCache with the default 24 h TTL:

W6-10 (a) first attempt threw: receipt email service down
W6-10 (a) key still claimed after the throw: false
W6-10 (a) card charged 2x for ONE idempotency key
W6-10 (b) first=503 replay=503 (replay of a 503 pinned for 24h; handler NOT re-run: {"error":"upstream 503"})
W6-10 (c) crashed-worker simulation: prior=false -> status 409 for the next 24h

(a) is the double-charge: the handler incremented its counter, threw, the claim was deleted, and the retry ran it again. (b) shows a downstream 503 replayed without re-running the handler. (c) writes the in-flight marker with the response TTL — as setIfAbsent(cacheKey, IN_FLIGHT_MARKER, ttlMs) does — and every subsequent request answers 409 with nothing to expire it for a day.

Adjacent issues: #609 covers one specific gap in computeRequestFingerprint (the query string is omitted, so the 422 "different request" guard passes when it should not) — a distinct defect in a different function, and orthogonal to the lifecycle problems here. #607 covers cache-key flooding evicting idempotency records from the shared LRU, which is loss of the record rather than mishandling of it.

Part of the production-readiness review batch — tracked in #913.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingpriority: highTop priority — high impact, plan nextproduction-goalBlocks or defines the path to production readiness

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions