diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c7357c..17963d0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,16 @@ Changelog All notable changes to the AnchorNet API are documented here. [Unreleased] +Fixed +Idempotency: the replay cache is no longer a private `Map` closed over by +each `idempotency()` call. All mounts share a process-wide +`MemoryIdempotencyStore` with a hard entry cap (default 1024; expired and +soonest-to-expire eviction), and concurrent same-key requests share one +in-flight execution so only a single handler runs. Replay still returns the +cached JSON body (not a conflict); only `status` + body are stored — no +response headers. Multi-replica sharing still waits on the separate +persistence-layer issue; this change closes the within-process holes without +adding an external dependency. Added Metrics: GET /api/v1/metrics now reports totalSettledAmount (sum of settlement amount) and totalFeesCollected (sum of settlement fee), diff --git a/README.md b/README.md index 7708c42..ac340b8 100644 --- a/README.md +++ b/README.md @@ -160,11 +160,20 @@ message `rate limit exceeded, try again later`. Clients should treat this as a retryable response and back off before sending the next mutating request. Mutating requests may also send an Idempotency-Key header. The first request -for a given key/method/path combination runs normally and its response is cached; any -later request reusing the same key (within 24h) replays the original response -instead of re-running the handler, so retried requests don't double-apply -side effects (e.g. registering the same anchor twice). State is in-memory and -per-process. +for a given key/method/path combination runs normally and its JSON response is +cached; any later request reusing the same key (within the configured TTL) +replays the original response instead of re-running the handler, so retried +requests don't double-apply side effects (e.g. registering the same anchor +twice). Reusing a key with a different body returns `422 IDEMPOTENCY_KEY_REUSE`. + +Cache state is a process-wide in-memory store shared by every `idempotency()` +mount (hard-capped; default 1024 entries, soonest-expiry eviction). Concurrent +same-key requests share one in-flight execution. Multi-replica deployments still +need an external shared store once the persistence layer lands — this is the +same sequencing constraint as the rate limiter. + +Only status + JSON body are stored for replay; response headers are never +cached. Walkthrough Example To verify how the idempotency system behaves, you can perform the following walkthrough using curl. @@ -213,7 +222,9 @@ x-request-id: 4a123f52-1623-429b-ba67-3d0d0d5c2eb0 "registeredAt": "2026-07-22T14:17:57.537Z", "active": true } -Mismatched body (Known Gap): If you reuse the same Idempotency-Key but change the request payload (e.g., modifying the name field), the server will still return the cached 201 response corresponding to the first payload. Detecting mismatched request bodies (which would ideally return a 422 error) is currently a known gap in this system. +Mismatched body: If you reuse the same Idempotency-Key but change the request +payload, the server returns `422` with code `IDEMPOTENCY_KEY_REUSE` instead of +replaying the original response. Bash @@ -221,19 +232,18 @@ curl -i -X POST http://localhost:3001/api/v1/anchors -H "Content-Type: application/json" -H "Idempotency-Key: register-anchor-xyz" -d '{"id": "anchor-xyz", "name": "Anchor XYZ Modified Name"}' -Response (Replayed from the original cached version): +Response: http -HTTP/1.1 201 Created +HTTP/1.1 422 Unprocessable Entity Content-Type: application/json; charset=utf-8 -x-request-id: 184c8357-3fc3-4e2f-a87c-19042ab804fe { -"id": "anchor-xyz", -"name": "Anchor XYZ", -"registeredAt": "2026-07-22T14:17:57.537Z", -"active": true +"error": { +"code": "IDEMPOTENCY_KEY_REUSE", +"message": "Idempotency key already used with a different request body" +} } The process shuts down gracefully on SIGTERM/SIGINT: it stops accepting new connections, closes the HTTP server, marks /health/ready unready, and diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 6ebac00..01569d9 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -21,6 +21,13 @@ Settlement, anchor, and liquidity data are held in process-local in-memory repositories (src/repositories/*), all extending the shared InMemoryRepository base class. +Idempotency cache (src/middleware/idempotency.ts) follows the same sequencing: +a process-wide `MemoryIdempotencyStore` (shared across mounts, hard-capped, +in-flight coalescing) closes within-process holes without introducing Redis/DB. +Cross-replica idempotency is intentionally deferred to the persistence-layer +issue — once that store exists, swap the default `IdempotencyStore` +implementation rather than bolting on a second persistence stack here. + Persistence-Swap Risk (read before swapping any repository for a DB) Several repositories already document that they are "swappable for a persistent … store later" (e.g. liquidityRepository.ts). This is a forward diff --git a/src/middleware/idempotency.test.ts b/src/middleware/idempotency.test.ts index ffad474..e5cc35f 100644 --- a/src/middleware/idempotency.test.ts +++ b/src/middleware/idempotency.test.ts @@ -1,30 +1,50 @@ import express, { Express } from "express"; import request from "supertest"; -import { idempotency, IdempotencyOptions } from "./idempotency"; +import { + idempotency, + IdempotencyOptions, + MemoryIdempotencyStore, + resetDefaultIdempotencyStore, +} from "./idempotency"; import { errorHandler } from "./errorHandler"; import { ApiError } from "../errors/ApiError"; -function makeApp(options?: IdempotencyOptions): Express { - let counter = 0; +function makeApp( + options?: IdempotencyOptions, + counterRef?: { value: number }, +): Express { + const counter = counterRef ?? { value: 0 }; + // Isolate unit tests from the process-wide default store unless a shared + // store is explicitly injected (cross-instance coverage). + const store = options?.store ?? new MemoryIdempotencyStore(options?.maxEntries); const app = express(); app.use(express.json()); - app.use(idempotency(options)); + app.use(idempotency({ ...options, store })); app.post("/mutate", (_req, res) => { - counter += 1; - res.status(201).json({ counter }); + counter.value += 1; + res.status(201).json({ counter: counter.value }); }); app.post("/fail", (_req, _res, next) => { - counter += 1; + counter.value += 1; next(ApiError.conflict("already exists")); }); app.get("/read", (_req, res) => { - counter += 1; - res.json({ counter }); + counter.value += 1; + res.json({ counter: counter.value }); + }); + app.post("/slow", async (_req, res) => { + counter.value += 1; + await new Promise((resolve) => setTimeout(resolve, 50)); + res.status(201).json({ counter: counter.value }); }); app.use(errorHandler); return app; } +beforeEach(() => { + resetDefaultIdempotencyStore(); +}); + describe("idempotency", () => { it("replays the cached response for a repeated key", async () => { const app = makeApp(); @@ -198,4 +218,97 @@ describe("idempotency", () => { expect(second.status).toBe(201); expect(second.body.counter).toBe(1); }); + + it("shares cache across two middleware instances with the same store", async () => { + const store = new MemoryIdempotencyStore(); + const counter = { value: 0 }; + // Two separate middleware instances (two apps) sharing one store — the + // pre-fix design closed over a private Map per call, so appB would + // re-execute after appA had already handled the key. + const appA = makeApp({ store }, counter); + const appB = makeApp({ store }, counter); + + const first = await request(appA) + .post("/mutate") + .set("Idempotency-Key", "shared-key"); + const second = await request(appB) + .post("/mutate") + .set("Idempotency-Key", "shared-key"); + + expect(first.body.counter).toBe(1); + expect(second.body.counter).toBe(1); + expect(counter.value).toBe(1); + }); + + it("bounds the cache under many distinct keys", async () => { + const maxEntries = 8; + const store = new MemoryIdempotencyStore(maxEntries); + const app = makeApp({ store }); + + for (let i = 0; i < maxEntries * 3; i += 1) { + const res = await request(app) + .post("/mutate") + .set("Idempotency-Key", `key-${i}`); + expect(res.status).toBe(201); + } + + expect(store.size()).toBeLessThanOrEqual(maxEntries); + }); + + it("runs only one handler for concurrent same-key requests", async () => { + const counter = { value: 0 }; + const app = makeApp({}, counter); + + const [first, second] = await Promise.all([ + request(app).post("/slow").set("Idempotency-Key", "concurrent"), + request(app).post("/slow").set("Idempotency-Key", "concurrent"), + ]); + + expect(first.status).toBe(201); + expect(second.status).toBe(201); + expect(first.body.counter).toBe(1); + expect(second.body.counter).toBe(1); + expect(counter.value).toBe(1); + }); + + it("leaves keyless mutating requests unaffected", async () => { + const counter = { value: 0 }; + const app = makeApp({}, counter); + + const first = await request(app).post("/mutate"); + const second = await request(app).post("/mutate"); + + expect(first.body.counter).toBe(1); + expect(second.body.counter).toBe(2); + expect(counter.value).toBe(2); + }); + + it("does not store response headers on the cached entry", async () => { + const store = new MemoryIdempotencyStore(); + const app = express(); + app.use(express.json()); + app.use(idempotency({ store })); + app.post("/mutate", (_req, res) => { + res.setHeader("X-Sensitive", "secret-token"); + res.setHeader("Set-Cookie", "session=abc"); + res.status(201).json({ ok: true }); + }); + app.use(errorHandler); + + await request(app).post("/mutate").set("Idempotency-Key", "hdr"); + const entry = store.get("POST /mutate hdr"); + expect(entry).toBeDefined(); + expect(entry).toEqual({ + status: 201, + body: { ok: true }, + expiresAt: expect.any(Number), + bodyHash: expect.any(String), + }); + expect(Object.keys(entry!).sort()).toEqual([ + "body", + "bodyHash", + "expiresAt", + "status", + ]); + }); }); diff --git a/src/middleware/idempotency.ts b/src/middleware/idempotency.ts index 2054085..924215f 100644 --- a/src/middleware/idempotency.ts +++ b/src/middleware/idempotency.ts @@ -7,9 +7,17 @@ * same key within the TTL replays the cached response instead of re-running * the handler, so retrying a request that already took effect (or already * failed) doesn't double-apply side effects. Requests without the header are - * unaffected. State lives in a plain `Map` local to the returned middleware, - * so like the existing rate limiter this is a per-process safeguard and not - * suitable for multi-instance deployments without a shared store. + * unaffected. + * + * Cache state lives in a process-wide {@link IdempotencyStore} shared by every + * `idempotency()` mount that does not inject its own `store` option. That + * closes the cross-mount duplicate hole inside one process. True multi-replica + * deployments still need an external shared store — this service has no + * persistence layer yet, so we keep an in-process store with a hard entry cap + * and leave Redis/DB to the separate persistence issue. + * + * Concurrent same-key requests share a single in-flight promise so only one + * handler runs; waiters replay (or 422 on body mismatch) when it completes. * * A SHA-256 fingerprint of the canonical JSON request body is stored alongside * the cached response. On key reuse, if the incoming body hash differs, a 422 @@ -17,6 +25,9 @@ * response. Canonical serialization uses stable key ordering so that * semantically identical bodies with different key orderings are treated as * the same request. + * + * Only `status` + JSON body are stored — response headers are never cached + * (so no `Set-Cookie` / auth leakage via the replay path). */ import crypto from "crypto"; @@ -28,6 +39,9 @@ const MUTATING_METHODS = new Set(["POST", "PUT", "PATCH", "DELETE"]); /** Default time a cached response remains eligible for replay. */ const DEFAULT_TTL_MS = 24 * 60 * 60 * 1000; +/** Default hard cap on cached entries (expired entries are purged first). */ +export const DEFAULT_MAX_ENTRIES = 1024; + /** * Produce a deterministic JSON string for any value. Object keys are sorted * recursively so that `{ a: 1, b: 2 }` and `{ b: 2, a: 1 }` serialize to the @@ -61,21 +75,161 @@ function hashBody(body: unknown): string { return crypto.createHash("sha256").update(raw).digest("hex"); } -interface CachedResponse { +export interface CachedResponse { status: number; body: unknown; expiresAt: number; bodyHash: string; } +export type InflightClaim = + | { status: "hit"; entry: CachedResponse } + | { status: "wait"; promise: Promise } + | { + status: "run"; + complete: (entry: CachedResponse) => void; + fail: (err: unknown) => void; + }; + +export interface IdempotencyStore { + get(key: string, now?: number): CachedResponse | undefined; + set(key: string, entry: CachedResponse): void; + /** Number of live (non-expired) entries — used by tests. */ + size(now?: number): number; + /** + * Atomically: return a cache hit, join an in-flight execution, or claim the + * right to run the handler for this key. + */ + begin(key: string, now?: number): InflightClaim; + /** Drop all entries and in-flight waiters (tests). */ + clear(): void; +} + +/** + * Bounded in-memory store. Evicts expired entries on write; if still over + * `maxEntries`, drops the soonest-to-expire live entries until under the cap. + */ +export class MemoryIdempotencyStore implements IdempotencyStore { + private readonly entries = new Map(); + private readonly inflight = new Map>(); + readonly maxEntries: number; + + constructor(maxEntries: number = DEFAULT_MAX_ENTRIES) { + if (!Number.isInteger(maxEntries) || maxEntries < 1) { + throw new Error("maxEntries must be a positive integer"); + } + this.maxEntries = maxEntries; + } + + get(key: string, now: number = Date.now()): CachedResponse | undefined { + const cached = this.entries.get(key); + if (!cached) return undefined; + if (cached.expiresAt <= now) { + this.entries.delete(key); + return undefined; + } + return cached; + } + + set(key: string, entry: CachedResponse): void { + const now = Date.now(); + this.purgeExpired(now); + this.entries.set(key, entry); + this.enforceBound(); + } + + size(now: number = Date.now()): number { + this.purgeExpired(now); + return this.entries.size; + } + + begin(key: string, now: number = Date.now()): InflightClaim { + const cached = this.get(key, now); + if (cached) return { status: "hit", entry: cached }; + + const pending = this.inflight.get(key); + if (pending) return { status: "wait", promise: pending }; + + let resolveEntry!: (entry: CachedResponse) => void; + let rejectEntry!: (err: unknown) => void; + const promise = new Promise((resolve, reject) => { + resolveEntry = resolve; + rejectEntry = reject; + }); + promise.catch(() => undefined); + this.inflight.set(key, promise); + + return { + status: "run", + complete: (entry: CachedResponse) => { + this.inflight.delete(key); + this.set(key, entry); + resolveEntry(entry); + }, + fail: (err: unknown) => { + this.inflight.delete(key); + rejectEntry(err); + }, + }; + } + + clear(): void { + this.entries.clear(); + this.inflight.clear(); + } + + private purgeExpired(now: number): void { + for (const [k, v] of this.entries) { + if (v.expiresAt <= now) this.entries.delete(k); + } + } + + private enforceBound(): void { + if (this.entries.size <= this.maxEntries) return; + const ranked = [...this.entries.entries()].sort( + (a, b) => a[1].expiresAt - b[1].expiresAt, + ); + let overflow = this.entries.size - this.maxEntries; + for (const [k] of ranked) { + if (overflow <= 0) break; + this.entries.delete(k); + overflow -= 1; + } + } +} + +/** Process-wide default so every `idempotency()` mount shares one cache. */ +const defaultStore = new MemoryIdempotencyStore(); + export interface IdempotencyOptions { /** Milliseconds a cached response remains eligible for replay. */ ttlMs?: number; + /** + * Hard cap on cached entries when using the process-wide default store. + * Ignored when a custom `store` is passed — configure that store instead. + * Changing this after the default store was constructed has no effect; + * prefer injecting `new MemoryIdempotencyStore(n)` for a private cap. + */ + maxEntries?: number; + /** + * Override the process-wide store (tests / multi-mount sharing). When + * omitted, all middleware instances share the same default store. + */ + store?: IdempotencyStore; +} + +function resolveStore(options: IdempotencyOptions): IdempotencyStore { + if (options.store) return options.store; + return defaultStore; +} + +function replay(res: Response, cached: CachedResponse): void { + res.status(cached.status).json(cached.body); } export function idempotency(options: IdempotencyOptions = {}) { const ttlMs = options.ttlMs ?? DEFAULT_TTL_MS; - const cache = new Map(); + const store = resolveStore(options); return (req: Request, res: Response, next: NextFunction): void => { const key = req.header("idempotency-key"); @@ -85,36 +239,64 @@ export function idempotency(options: IdempotencyOptions = {}) { } const cacheKey = `${req.method} ${req.originalUrl} ${key}`; - const now = Date.now(); - const cached = cache.get(cacheKey); - if (cached && cached.expiresAt > now) { - // NOTE: concurrent requests with the same key may both pass this check - // before either writes to the cache (check-then-set race). This is a - // pre-existing limitation of the in-memory Map design, not introduced - // by this change. See README for per-process scope. - if (cached.bodyHash !== hashBody(req.body)) { - next( - ApiError.idempotencyKeyReuse( - "Idempotency key already used with a different request body", - ), - ); - return; - } - res.status(cached.status).json(cached.body); - return; - } + const bodyHash = hashBody(req.body); + + void (async () => { + try { + const claim = store.begin(cacheKey); + + if (claim.status === "hit" || claim.status === "wait") { + try { + const finished = + claim.status === "hit" ? claim.entry : await claim.promise; + if (finished.bodyHash !== bodyHash) { + next( + ApiError.idempotencyKeyReuse( + "Idempotency key already used with a different request body", + ), + ); + return; + } + replay(res, finished); + } catch (err) { + next(err); + } + return; + } - const originalJson = res.json.bind(res); - res.json = ((body: unknown) => { - cache.set(cacheKey, { - status: res.statusCode, - body, - expiresAt: now + ttlMs, - bodyHash: hashBody(req.body), - }); - return originalJson(body); - }) as Response["json"]; - - next(); + const { complete, fail } = claim; + const originalJson = res.json.bind(res); + let settled = false; + + res.json = ((body: unknown) => { + if (!settled) { + settled = true; + complete({ + status: res.statusCode, + body, + expiresAt: Date.now() + ttlMs, + bodyHash, + }); + } + return originalJson(body); + }) as Response["json"]; + + res.on("close", () => { + if (!settled && !res.writableEnded) { + settled = true; + fail(new Error("client closed before response")); + } + }); + + next(); + } catch (err) { + next(err); + } + })(); }; } + +/** Exposed for tests that need to wipe process-wide state between cases. */ +export function resetDefaultIdempotencyStore(): void { + defaultStore.clear(); +}