From 583d4a2328ad86e83f4176e6a3a4cf84f5d4ddc6 Mon Sep 17 00:00:00 2001 From: unknown Date: Sun, 6 Sep 2026 11:54:38 +0300 Subject: [PATCH] feat(http): make a retried write safe to send twice A client that loses the response to a request cannot know whether the request landed, and on a mobile network that is routine. Retrying is the only thing it can do, and today that retry is a second post, a second comment, a second uploaded file. An Idempotency-Key claims a slot through CachePort.setIfAbsent - one SET NX EX, so the store decides the race rather than the API reading and then writing - and a retry is answered from the first attempt instead of running again. Opt-in on the seven writes where a duplicate is real damage; a like, a follow and a device registration are already repeatable and pay nothing. The record key carries the account, because a key is a value the client invents and two people can pick the same one. It also carries a fingerprint of the body, so a key reused with a different request is refused rather than answered with the earlier result. It fails open: an unreachable cache logs and lets the request through. This is a safety net over a write that already works, and a hard dependency would turn a cache blip into nobody being able to post. --- CLAUDE.md | 12 + docs/idempotency.md | 94 ++++++++ src/app.ts | 6 + src/core/ports/services/cache.port.ts | 19 ++ .../plugins/idempotency/idempotency-record.ts | 130 +++++++++++ .../plugins/idempotency/idempotency.plugin.ts | 206 ++++++++++++++++++ .../routes/article/article-comment.routes.ts | 5 +- src/http/routes/article/article.routes.ts | 5 +- .../routes/conversation/message.routes.ts | 10 +- src/http/routes/post/comment.routes.ts | 5 +- src/http/routes/post/post.routes.ts | 10 +- src/http/types/fastify.d.ts | 11 + .../realtime/redis/redis.service.ts | 33 +++ tests/e2e/idempotency/idempotency.test.ts | 138 ++++++++++++ tests/unit/http/idempotency-record.test.ts | 107 +++++++++ 15 files changed, 784 insertions(+), 7 deletions(-) create mode 100644 docs/idempotency.md create mode 100644 src/http/plugins/idempotency/idempotency-record.ts create mode 100644 src/http/plugins/idempotency/idempotency.plugin.ts create mode 100644 tests/e2e/idempotency/idempotency.test.ts create mode 100644 tests/unit/http/idempotency-record.test.ts diff --git a/CLAUDE.md b/CLAUDE.md index aa37c5d6..e4af231a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -87,6 +87,18 @@ The target is stored against a random `state` (`BeginOAuthUseCase`, 10-minute TT `GET /meta/client` reports the build floor (`MOBILE_MIN_SUPPORTED_BUILD`, zero meaning none) so a published app can be told it is too old. A web bundle is replaced every morning; an app version lives on phones for months, and without this there is no safe way to make a breaking change. +### Retried writes + +A client that loses the *response* to a request cannot know whether the request landed, and on a mobile network that is routine. `Idempotency-Key` makes the retry safe: the claim is a single `SET NX EX` through `CachePort.setIfAbsent`, so the store decides the race rather than the API reading and then writing. + +Opt-in per route (`config: { idempotency: true }`) on the seven writes where a duplicate is real damage — posts, both comment endpoints, articles, messages and the two upload endpoints. Everything else is already repeatable: likes, follows and bookmarks are idempotent, a report has a unique constraint, a device registration is an upsert. + +The record key carries the **account** (`idem:v1::::`) because a key is a value the client invents and two people can pick the same one. It also carries a fingerprint of the body, so a key reused with a different request is a 409 rather than a wrong answer replayed. Only 2xx responses are stored — a 4xx is deterministic and a 5xx must stay retryable, so neither spends the key. + +**It fails open:** an unreachable Redis logs and lets the request through, because this is a safety net over a write that already works and a hard dependency would turn a cache blip into "nobody can post". An endpoint that moves money should revisit that trade rather than inherit it. + +`docs/idempotency.md` is the client-facing contract. + ### Rate limiting `RateLimitPolicies` in `src/http/plugins/rate-limit.plugin.ts`: `STRICT` (3/15 min, `continueExceeding`) for login/register, `SENSITIVE` (5/min) for password reset, verification, and write/social actions, `STANDARD` (60/min) for authenticated reads, `PUBLIC` (100/min). Global default is 100/min. Requests with `Authorization: Bot ` are allow-listed after a sha256 lookup against `user.botToken` — suspended bots (`bannedAt`) are excluded from that lookup. diff --git a/docs/idempotency.md b/docs/idempotency.md new file mode 100644 index 00000000..63f4f74f --- /dev/null +++ b/docs/idempotency.md @@ -0,0 +1,94 @@ +# Retrying a write safely + +A client that loses the *response* to a request cannot know whether the request +itself landed. On a mobile network that happens routinely, and retrying is the +only thing the client can do. Without help, that retry is a second post, a +second comment, a second uploaded file. + +Send an `Idempotency-Key` header and the retry is answered from the first +attempt instead of running again. + +## Using it + +```http +POST /api/v1/posts +Authorization: Bearer … +Idempotency-Key: 4f1c0f2a-6d2f-4f0b-9d4e-2a1b3c4d5e6f +Content-Type: application/json + +{ "content": "…" } +``` + +Generate a fresh key per user action — a UUID is ideal — and **reuse the same +key for every retry of that action**. A new key means a new action. + +The replayed response is byte-for-byte the first one, with the same status +code, plus: + +```http +Idempotent-Replay: true +``` + +Keys live for **24 hours**. A key sent to a route that does not support it is +ignored, and a request with no key behaves exactly as it always has — which is +why the web client needs no changes. + +### Which endpoints + +Only the writes where a duplicate is real damage: + +| Endpoint | | +| --- | --- | +| `POST /posts` | | +| `POST /posts/:postId/comments` | | +| `POST /articles` | | +| `POST /articles/:articleId/comments` | | +| `POST /conversations/:id/messages` | | +| `POST /media` | uploads cost storage and moderation | +| `POST /messages/media` | | + +Everything else is already safe to repeat: a like, a follow and a bookmark are +idempotent by nature, a report is protected by a unique constraint, and a +device registration is an upsert. + +### Errors + +**409 with `A request with this Idempotency-Key is still in progress.`** — the +first attempt has not finished. A `Retry-After` header comes with it. This is +the answer to sending the retry too eagerly, not an error to give up on. + +**409 with `This Idempotency-Key was already used with a different request.`** — +the key has been seen with a different body. Almost always a client bug: a key +being reused across actions. Answering it with the earlier result would hide +the bug behind a wrong response. + +A request that fails does **not** spend its key. A 4xx is deterministic — the +retry will be told the same thing by the handler — and a 5xx must stay +retryable, or a transient failure would block the action for a day. + +## How it works + +The record lives in Redis under `idem:v1::::`. The +account is part of the key: a key is a value the client invents, two people can +easily pick the same one, and a shared bucket would hand one of them the +other's response. + +The claim is a single `SET NX EX`, so the store decides the race rather than +the API reading and then writing. Winning the claim runs the handler; losing it +means reading the record and either replaying it or reporting a conflict. + +The record also holds a fingerprint of the request body, which is what makes +the mismatch case detectable. + +### Two things it does not promise + +**Uploads are guarded by the key alone.** A multipart body is a stream that has +not been read when the claim is made, so there is nothing to fingerprint. Two +genuinely different uploads sent under one key would be treated as a repeat. +Use a fresh key per upload, as you would anyway. + +**It fails open.** If Redis is unreachable the request proceeds without +protection and the failure is logged. This is a safety net over a write that +already works, and making it a hard dependency would turn a cache blip into +"nobody can post anything". If a future endpoint moves money, that endpoint +should reconsider — the trade is not universal. diff --git a/src/app.ts b/src/app.ts index cc5593ad..4a7c2d34 100644 --- a/src/app.ts +++ b/src/app.ts @@ -25,6 +25,7 @@ import reportRoutes from "@routes/report.routes"; import metaRoutes from "@routes/meta.routes"; import deviceRoutes from "@routes/device.routes"; import billingRoutes from "@routes/billing.routes"; +import idempotencyPlugin from "@plugins/idempotency/idempotency.plugin"; import websocketPlugin from "./http/plugins/websocket.plugin"; import realtimeRoutes from "@routes/realtime.routes"; import notificationRoutes from "@routes/notification.routes"; @@ -116,6 +117,11 @@ export class App { await this.server.after(); + // After the container, before the routes: the hooks it installs need + // the cache service, and they have to be in place before anything + // registers a route that opts into them. + this.server.register(idempotencyPlugin); + this.server.register(refreshTokenPurgePlugin); this.server.register(userPurgePlugin); this.server.register(notificationPurgePlugin); diff --git a/src/core/ports/services/cache.port.ts b/src/core/ports/services/cache.port.ts index 88451655..482fcaca 100644 --- a/src/core/ports/services/cache.port.ts +++ b/src/core/ports/services/cache.port.ts @@ -23,6 +23,25 @@ export interface CachePort { * Deletes a single cache entry by its exact key. * @param key - The exact cache key to delete. */ + /** + * Writes a value only when the key is not already taken. + * + * The claim primitive. Reading and then writing leaves a window two + * concurrent callers both pass through, which is exactly the case this + * exists to decide - a retry arriving while the first attempt is still in + * flight. The store settles it in one operation instead. + * + * @param key - The key to claim. + * @param value - The value to write if the claim succeeds. + * @param ttlSeconds - How long the claim lives. + * @returns True when this caller took the key, false when somebody held it. + */ + setIfAbsent( + key: string, + value: string, + ttlSeconds: number, + ): Promise; + delete(key: string): Promise; /** diff --git a/src/http/plugins/idempotency/idempotency-record.ts b/src/http/plugins/idempotency/idempotency-record.ts new file mode 100644 index 00000000..07af53ef --- /dev/null +++ b/src/http/plugins/idempotency/idempotency-record.ts @@ -0,0 +1,130 @@ +import { createHash } from "node:crypto"; + +/** + * Where a claimed request got to. + * + * `in-flight` is a real state rather than an absence: a retry that arrives + * while the first attempt is still running must be told to wait, not served a + * second execution. + */ +export type IdempotencyState = "in-flight" | "completed"; + +/** + * What is remembered about one claimed request. + */ +export interface IdempotencyRecord { + state: IdempotencyState; + + /** + * Fingerprint of the request body. + * + * Kept so the same key arriving with a different body is refused rather + * than answered with the first request's result - which would be a client + * bug quietly turned into a wrong response. + */ + fingerprint: string; + + /** Status of the stored response, once there is one. */ + statusCode?: number; + + /** The serialised response body, once there is one. */ + body?: string; +} + +/** + * The cache key one claim lives under. + * + * Scoped by account as well as by route: a key is a value the client invents, + * so two people can easily pick the same one, and a shared bucket would let + * one of them be handed the other's response. + * + * The version prefix means a change to what is stored can be rolled out by + * bumping it rather than by reasoning about records written by the previous + * deploy. + * + * @param userId - The account making the request + * @param method - HTTP method + * @param routePath - The route pattern, not the resolved URL + * @param key - The client's `Idempotency-Key` + * @returns The cache key + */ +export function idempotencyCacheKey( + userId: string, + method: string, + routePath: string, + key: string, +): string { + return `idem:v1:${userId}:${method}:${routePath}:${key}`; +} + +/** + * Fingerprints a request body. + * + * A multipart upload has no body to fingerprint - it is a stream that has not + * been read yet - and returns a constant. The key alone guards those, which is + * weaker and is documented as such. + * + * @param body - The parsed request body, if any + * @returns A stable hash of the body + */ +export function fingerprintBody(body: unknown): string { + if (body === undefined || body === null) return "empty"; + if (typeof body !== "object") + return createHash("sha256").update(String(body)).digest("hex"); + if (Buffer.isBuffer(body)) return "stream"; + + try { + return createHash("sha256").update(stableStringify(body)).digest("hex"); + } catch { + // A body that will not serialise cannot be compared; the key alone + // guards it, exactly as for an upload. + return "unhashable"; + } +} + +/** + * Serialises a value with object keys in a fixed order, at every depth. + * + * `JSON.stringify(value, keys)` looks like it would do this and does something + * else entirely: the second argument is a *filter*, applied at every level, so + * any nested key absent from the top-level list disappears. Two bodies + * differing only somewhere nested would then fingerprint the same - and a + * fingerprint collision here does not merely miss a duplicate, it replays the + * wrong response to a genuinely different request. + * + * @param value - The value to serialise + * @returns A stable string for the value + */ +function stableStringify(value: unknown): string { + if (value === null || typeof value !== "object") { + return JSON.stringify(value) ?? "null"; + } + + if (Array.isArray(value)) { + return `[${value.map((item) => stableStringify(item)).join(",")}]`; + } + + const entries = Object.entries(value as Record) + .filter(([, item]) => item !== undefined) + .sort(([a], [b]) => (a < b ? -1 : a > b ? 1 : 0)) + .map( + ([key, item]) => `${JSON.stringify(key)}:${stableStringify(item)}`, + ); + + return `{${entries.join(",")}}`; +} + +/** + * Whether a response is worth remembering. + * + * Only success is stored. A 4xx is deterministic - the retry will be told the + * same thing by the handler itself - and a 5xx must stay retryable, because + * remembering a transient failure would block the request for as long as the + * record lives. + * + * @param statusCode - The status the handler produced + * @returns True when the response should be replayed to a retry + */ +export function isReplayable(statusCode: number): boolean { + return statusCode >= 200 && statusCode < 300; +} diff --git a/src/http/plugins/idempotency/idempotency.plugin.ts b/src/http/plugins/idempotency/idempotency.plugin.ts new file mode 100644 index 00000000..c6cc8c82 --- /dev/null +++ b/src/http/plugins/idempotency/idempotency.plugin.ts @@ -0,0 +1,206 @@ +import type { FastifyInstance, FastifyReply, FastifyRequest } from "fastify"; +import fastifyPlugin from "fastify-plugin"; +import { ConflictError } from "@core/errors"; +import type { CachePort } from "@core/ports/services/cache.port"; +import { + fingerprintBody, + idempotencyCacheKey, + isReplayable, + type IdempotencyRecord, +} from "./idempotency-record"; + +const HEADER = "idempotency-key"; + +const REPLAY_HEADER = "idempotent-replay"; + +/** Longest key a client may send; anything longer is a mistake or an attack. */ +const MAX_KEY_LENGTH = 200; + +/** How long a claim and its answer are remembered. */ +const RECORD_TTL_SECONDS = 24 * 60 * 60; + +/** + * Largest response body kept for replay. + * + * A cap rather than a promise: past it the claim still prevents the second + * execution, and the retry gets a conflict instead of the original answer. + * Nothing this feature protects returns anything near it. + */ +const MAX_STORED_BODY_BYTES = 64 * 1024; + +/** Where a won claim is kept between the two hooks. */ +const CLAIM = Symbol("idempotencyClaim"); + +interface Claim { + cacheKey: string; + fingerprint: string; +} + +type RequestWithClaim = FastifyRequest & { [CLAIM]?: Claim }; + +/** + * Answers a retry from the record its first attempt left behind. + * + * @param cacheService - Where records live + * @param reply - The reply to send + * @param cacheKey - Where this request's record lives + * @param fingerprint - The retry's own body fingerprint + * + * @throws ConflictError - When the key was used with a different body, or the + * first attempt has not finished + */ +async function replayRecord( + cacheService: CachePort, + reply: FastifyReply, + cacheKey: string, + fingerprint: string, +): Promise { + const raw = await cacheService.get(cacheKey); + + // The claim was lost but the record has already gone - a TTL that expired + // between the two calls. Letting the request through is the only useful + // answer left, and by then a duplicate is a day old. + if (!raw) return; + + const record = JSON.parse(raw) as IdempotencyRecord; + + if (record.fingerprint !== fingerprint) { + throw new ConflictError( + "This Idempotency-Key was already used with a different request.", + ); + } + + if (record.state === "in-flight" || record.body === undefined) { + reply.header("retry-after", "1"); + + throw new ConflictError( + "A request with this Idempotency-Key is still in progress.", + ); + } + + reply + .header(REPLAY_HEADER, "true") + .type("application/json") + .status(record.statusCode ?? 200) + .send(record.body); +} + +/** + * Makes a retried write safe to send twice. + * + * Mobile networks make "did that send?" a routine question: a client that + * loses the *response* to a request cannot know whether the request itself + * landed, and retrying is the only thing it can do. Without this, that retry + * is a second post, a second comment, a second uploaded file. + * + * Opt-in per route (`config: { idempotency: true }`) rather than global. Most + * writes here are already idempotent - a like, a follow, a device + * registration - and wrapping those would cost a round trip to buy nothing. + * + * A request with no `Idempotency-Key` behaves exactly as it did before, which + * is what leaves the web client untouched. + * + * @param fastify - The Fastify application instance + */ +function idempotencyPlugin(fastify: FastifyInstance): void { + const cacheService = fastify.diContainer.cradle.cacheService; + + fastify.addHook("preHandler", async (request, reply) => { + if (request.routeOptions.config?.idempotency !== true) return; + + const key = request.headers[HEADER]; + const userId = request.user?.id; + + // Unauthenticated callers are out of scope: there is nothing to scope + // a client-chosen key by, and every route that opts in requires a + // session anyway. + if (typeof key !== "string" || key.length === 0 || !userId) return; + + if (key.length > MAX_KEY_LENGTH) { + throw new ConflictError("Idempotency-Key is too long."); + } + + const cacheKey = idempotencyCacheKey( + userId, + request.method, + request.routeOptions.url ?? request.url, + key, + ); + const fingerprint = fingerprintBody(request.body); + + let won: boolean; + + try { + won = await cacheService.setIfAbsent( + cacheKey, + JSON.stringify({ + state: "in-flight", + fingerprint, + } satisfies IdempotencyRecord), + RECORD_TTL_SECONDS, + ); + } catch (error: unknown) { + // Fail open, deliberately. This is a safety net over a write that + // already works; making it a hard dependency would turn a cache + // blip into "nobody can post anything". + fastify.log.error( + { err: error, cacheKey }, + "Idempotency claim failed; proceeding without it", + ); + return; + } + + if (won) { + (request as RequestWithClaim)[CLAIM] = { cacheKey, fingerprint }; + return; + } + + await replayRecord(cacheService, reply, cacheKey, fingerprint); + }); + + fastify.addHook("onSend", async (request, reply, payload) => { + const claim = (request as RequestWithClaim)[CLAIM]; + + if (!claim) return payload; + + try { + if (!isReplayable(reply.statusCode)) { + // A failure must not be remembered: a transient 500 would + // otherwise block this key for a day. + await cacheService.delete(claim.cacheKey); + + return payload; + } + + const body = typeof payload === "string" ? payload : undefined; + const storable = + body !== undefined && + Buffer.byteLength(body) <= MAX_STORED_BODY_BYTES; + + await cacheService.set( + claim.cacheKey, + JSON.stringify({ + state: "completed", + fingerprint: claim.fingerprint, + statusCode: reply.statusCode, + body: storable ? body : undefined, + } satisfies IdempotencyRecord), + RECORD_TTL_SECONDS, + ); + } catch (error: unknown) { + // The write already happened and the client is about to be told + // so. Losing the record costs a retry its replay, not its result. + fastify.log.error( + { err: error, cacheKey: claim.cacheKey }, + "Failed to record an idempotent response", + ); + } + + return payload; + }); +} + +export default fastifyPlugin(idempotencyPlugin, { + name: "idempotency-plugin", + dependencies: ["di-plugin"], +}); diff --git a/src/http/routes/article/article-comment.routes.ts b/src/http/routes/article/article-comment.routes.ts index 3197d999..4fb5ebb1 100644 --- a/src/http/routes/article/article-comment.routes.ts +++ b/src/http/routes/article/article-comment.routes.ts @@ -45,7 +45,10 @@ export function articleCommentRoutes(fastify: FastifyInstance): void { response: { 201: CreateArticleCommentResponseSchema }, tags: ["Article", "Comment"], }, - config: { rateLimit: RateLimitPolicies.STANDARD }, + config: { + idempotency: true, + rateLimit: RateLimitPolicies.STANDARD, + }, }, commentController.createForArticle.bind(commentController), ); diff --git a/src/http/routes/article/article.routes.ts b/src/http/routes/article/article.routes.ts index a348ee88..47e5b20f 100644 --- a/src/http/routes/article/article.routes.ts +++ b/src/http/routes/article/article.routes.ts @@ -113,7 +113,10 @@ export function articleRoutes(fastify: FastifyInstance): void { response: { 201: ArticleResponseSchema }, tags: ["Article"], }, - config: { rateLimit: RateLimitPolicies.SENSITIVE }, + config: { + idempotency: true, + rateLimit: RateLimitPolicies.SENSITIVE, + }, }, articleController.create.bind(articleController), ); diff --git a/src/http/routes/conversation/message.routes.ts b/src/http/routes/conversation/message.routes.ts index b387d649..514dca53 100644 --- a/src/http/routes/conversation/message.routes.ts +++ b/src/http/routes/conversation/message.routes.ts @@ -75,7 +75,10 @@ export function messageRoutes(fastify: FastifyInstance): void { response: { 201: MessageResponseSchema }, tags: ["Conversation"], }, - config: { rateLimit: RateLimitPolicies.SENSITIVE }, + config: { + idempotency: true, + rateLimit: RateLimitPolicies.SENSITIVE, + }, }, conversationController.sendMessage.bind(conversationController), ); @@ -94,7 +97,10 @@ export function messageRoutes(fastify: FastifyInstance): void { response: { 200: UploadMessageMediaResponseSchema }, tags: ["Conversation"], }, - config: { rateLimit: RateLimitPolicies.SENSITIVE }, + config: { + idempotency: true, + rateLimit: RateLimitPolicies.SENSITIVE, + }, }, conversationController.uploadMedia.bind(conversationController), ); diff --git a/src/http/routes/post/comment.routes.ts b/src/http/routes/post/comment.routes.ts index 5c7c7121..5b54860b 100644 --- a/src/http/routes/post/comment.routes.ts +++ b/src/http/routes/post/comment.routes.ts @@ -64,7 +64,10 @@ export function commentRoutes(fastify: FastifyInstance): void { response: { 201: CreateCommentResponseSchema }, tags: ["Comment"], }, - config: { rateLimit: RateLimitPolicies.STANDARD }, + config: { + idempotency: true, + rateLimit: RateLimitPolicies.STANDARD, + }, }, commentController.create.bind(commentController), ); diff --git a/src/http/routes/post/post.routes.ts b/src/http/routes/post/post.routes.ts index cdf5a98e..1a7b24b2 100644 --- a/src/http/routes/post/post.routes.ts +++ b/src/http/routes/post/post.routes.ts @@ -73,7 +73,10 @@ export function postRoutes(fastify: FastifyInstance): void { response: { 201: CreatePostResponseSchema }, tags: ["Post"], }, - config: { rateLimit: RateLimitPolicies.SENSITIVE }, + config: { + idempotency: true, + rateLimit: RateLimitPolicies.SENSITIVE, + }, }, postController.create.bind(postController), ); @@ -86,7 +89,10 @@ export function postRoutes(fastify: FastifyInstance): void { "/media", { onRequest: [fastify.authenticate], - config: { rateLimit: RateLimitPolicies.SENSITIVE }, + config: { + idempotency: true, + rateLimit: RateLimitPolicies.SENSITIVE, + }, schema: { response: { 200: UploadMediaResponseSchema }, tags: ["Post"], diff --git a/src/http/types/fastify.d.ts b/src/http/types/fastify.d.ts index 44b2e6c6..8b3c8980 100644 --- a/src/http/types/fastify.d.ts +++ b/src/http/types/fastify.d.ts @@ -3,6 +3,17 @@ import type { PrismaClient } from "src/generated/prisma/client"; import { type EnvConfig } from "./schemas/env.schema"; declare module "fastify" { + interface FastifyContextConfig { + /** + * Whether a retried request carrying an `Idempotency-Key` should be + * answered from the first attempt rather than run again. + * + * Opt-in, because most writes here are already idempotent and paying + * for a claim on those buys nothing. + */ + idempotency?: boolean; + } + interface FastifyInstance { config: EnvConfig; prisma: PrismaClient; diff --git a/src/infrastructure/realtime/redis/redis.service.ts b/src/infrastructure/realtime/redis/redis.service.ts index aef3ee90..7d7a6266 100644 --- a/src/infrastructure/realtime/redis/redis.service.ts +++ b/src/infrastructure/realtime/redis/redis.service.ts @@ -76,6 +76,39 @@ export class RedisService implements CachePort { } } + /** + * Writes a value only when the key is not already taken. + * + * `SET NX EX` in one round trip: the store decides the race rather than + * this process reading and then writing. + * + * Throws rather than swallowing, unlike its neighbours. A caller asking + * "did I win the claim?" cannot be answered with silence - the two + * possible lies are "you won" (two requests both proceed) and "you lost" + * (a legitimate request is refused). It is for the caller to decide what a + * cache outage means. + * + * @param key - The key to claim. + * @param value - The value to write if the claim succeeds. + * @param ttlSeconds - How long the claim lives. + * @returns True when this caller took the key. + */ + async setIfAbsent( + key: string, + value: string, + ttlSeconds: number, + ): Promise { + const result = await this.publisher.set( + key, + value, + "EX", + ttlSeconds, + "NX", + ); + + return result === "OK"; + } + async delete(key: string): Promise { try { await this.publisher.del(key); diff --git a/tests/e2e/idempotency/idempotency.test.ts b/tests/e2e/idempotency/idempotency.test.ts new file mode 100644 index 00000000..4393de51 --- /dev/null +++ b/tests/e2e/idempotency/idempotency.test.ts @@ -0,0 +1,138 @@ +import { authRequest, parseBody, request } from "../setup"; +import { beforeAll, describe, expect, it } from "vitest"; + +interface PostData { + id: string; + content: string; +} + +/** + * E2E tests for retried writes. + * + * The behaviour being protected is a phone that loses the *response* to a + * request and retries: the write must not happen twice, and the retry must be + * told what the first attempt produced. + */ +describe("Idempotency-Key", () => { + const ts = Date.now(); + const user = { + email: `idem-${ts}@test.com`, + password: "password123", + username: `idem${ts}`, + }; + const other = { + email: `idem-b-${ts}@test.com`, + password: "password123", + username: `idemb${ts}`, + }; + + let token = ""; + let otherToken = ""; + + const registerAndLogin = async (u: { + email: string; + password: string; + username: string; + }): Promise => { + await request({ method: "POST", url: "/auth/register", payload: u }); + + const loggedIn = await request({ + method: "POST", + url: "/auth/login", + payload: { identifier: u.email, password: u.password }, + }); + + return parseBody<{ data: { accessToken: string } }>(loggedIn).data + .accessToken; + }; + + const createPost = ( + accessToken: string, + content: string, + key?: string, + ) => + authRequest(accessToken, { + method: "POST", + url: "/posts", + payload: { content }, + ...(key ? { headers: { "idempotency-key": key } } : {}), + }); + + beforeAll(async () => { + token = await registerAndLogin(user); + otherToken = await registerAndLogin(other); + }); + + it("should create the post once and replay the answer", async () => { + const key = `key-${ts}-1`; + + const first = await createPost(token, "retried post", key); + const retry = await createPost(token, "retried post", key); + + expect(first.statusCode).toBe(201); + expect(retry.statusCode).toBe(201); + + const firstBody = parseBody<{ data: PostData }>(first).data; + const retryBody = parseBody<{ data: PostData }>(retry).data; + + // The same post, not a second one that happens to look alike. + expect(retryBody.id).toBe(firstBody.id); + expect(retry.headers["idempotent-replay"]).toBe("true"); + }); + + it("should refuse the same key with a different body", async () => { + const key = `key-${ts}-2`; + + await createPost(token, "first body", key); + const mismatch = await createPost(token, "different body", key); + + // A client bug, and answering it with the first request's result would + // hide it behind a wrong response. + expect(mismatch.statusCode).toBe(409); + }); + + it("should keep one account's key out of another's way", async () => { + const key = `key-${ts}-3`; + + const mine = await createPost(token, "same key different people", key); + const theirs = await createPost( + otherToken, + "same key different people", + key, + ); + + expect(mine.statusCode).toBe(201); + expect(theirs.statusCode).toBe(201); + expect(parseBody<{ data: PostData }>(theirs).data.id).not.toBe( + parseBody<{ data: PostData }>(mine).data.id, + ); + }); + + it("should create two posts when no key is sent", async () => { + // The web client sends none, and its behaviour must not change. + const first = await createPost(token, "unkeyed post"); + const second = await createPost(token, "unkeyed post"); + + expect(parseBody<{ data: PostData }>(second).data.id).not.toBe( + parseBody<{ data: PostData }>(first).data.id, + ); + }); + + it("should let a rejected request be retried with the same key", async () => { + const key = `key-${ts}-4`; + + // An empty body fails validation; the key must not be spent on it. + const rejected = await authRequest(token, { + method: "POST", + url: "/posts", + payload: { content: "" }, + headers: { "idempotency-key": key }, + }); + + expect(rejected.statusCode).toBe(400); + + const corrected = await createPost(token, "fixed after a 400", key); + + expect(corrected.statusCode).toBe(201); + }); +}); diff --git a/tests/unit/http/idempotency-record.test.ts b/tests/unit/http/idempotency-record.test.ts new file mode 100644 index 00000000..9400e2f4 --- /dev/null +++ b/tests/unit/http/idempotency-record.test.ts @@ -0,0 +1,107 @@ +import { describe, expect, it } from "vitest"; +import { + fingerprintBody, + idempotencyCacheKey, + isReplayable, +} from "@plugins/idempotency/idempotency-record"; + +describe("idempotencyCacheKey", () => { + it("should give two accounts separate buckets for the same key", () => { + // A key is a value the client invents, so two people picking the same + // one is ordinary. A shared bucket would hand one of them the other's + // response. + const mine = idempotencyCacheKey("user-1", "POST", "/posts", "abc"); + const theirs = idempotencyCacheKey("user-2", "POST", "/posts", "abc"); + + expect(mine).not.toBe(theirs); + }); + + it("should give two routes separate buckets for the same key", () => { + expect(idempotencyCacheKey("user-1", "POST", "/posts", "abc")).not.toBe( + idempotencyCacheKey("user-1", "POST", "/articles", "abc"), + ); + }); + + it("should be stable for the same request", () => { + expect(idempotencyCacheKey("user-1", "POST", "/posts", "abc")).toBe( + idempotencyCacheKey("user-1", "POST", "/posts", "abc"), + ); + }); + + it("should carry a version prefix", () => { + // What is stored can then change by bumping the prefix, rather than by + // reasoning about records the previous deploy wrote. + expect(idempotencyCacheKey("user-1", "POST", "/posts", "abc")).toMatch( + /^idem:v1:/, + ); + }); +}); + +describe("fingerprintBody", () => { + it("should match two identical bodies", () => { + expect(fingerprintBody({ content: "hello" })).toBe( + fingerprintBody({ content: "hello" }), + ); + }); + + it("should not depend on key order", () => { + // The same request serialised differently is still the same request. + expect(fingerprintBody({ a: 1, b: 2 })).toBe( + fingerprintBody({ b: 2, a: 1 }), + ); + }); + + it("should notice a difference nested inside the body", () => { + // The trap: JSON.stringify's second argument is a filter applied at + // every level, so a naive "sort the top-level keys" drops nested ones + // and makes two different requests fingerprint the same. + expect( + fingerprintBody({ post: { content: "a", tags: ["x"] } }), + ).not.toBe(fingerprintBody({ post: { content: "a", tags: ["y"] } })); + }); + + it("should not depend on key order at any depth", () => { + expect(fingerprintBody({ outer: { a: 1, b: 2 } })).toBe( + fingerprintBody({ outer: { b: 2, a: 1 } }), + ); + }); + + it("should keep array order significant", () => { + expect(fingerprintBody({ tags: ["a", "b"] })).not.toBe( + fingerprintBody({ tags: ["b", "a"] }), + ); + }); + + it("should differ for different bodies", () => { + expect(fingerprintBody({ content: "hello" })).not.toBe( + fingerprintBody({ content: "goodbye" }), + ); + }); + + it("should handle an absent body", () => { + expect(fingerprintBody(undefined)).toBe("empty"); + expect(fingerprintBody(null)).toBe("empty"); + }); + + it("should not attempt to hash an upload stream", () => { + // A multipart body has not been read yet; the key alone guards those. + expect(fingerprintBody(Buffer.from("file"))).toBe("stream"); + }); +}); + +describe("isReplayable", () => { + it("should remember success", () => { + for (const status of [200, 201, 204]) { + expect(isReplayable(status)).toBe(true); + } + }); + + it("should not remember a rejection or a failure", () => { + // A 4xx is deterministic - the handler will say the same thing again - + // and a 5xx must stay retryable, or a transient failure would block + // the key for as long as the record lives. + for (const status of [400, 401, 404, 409, 500, 503]) { + expect(isReplayable(status)).toBe(false); + } + }); +});