From 1e356a25786752ccdee2b0c2094adbbe1325a761 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?N=C3=B4ng=20=C4=90=E1=BB=A9c=20Huy?= Date: Wed, 12 Aug 2026 22:04:41 +0700 Subject: [PATCH] Fix error before M7 --- README.md | 10 +- .../src/infrastructure/redis/redis.service.ts | 12 +++ .../modules/checkout/checkout.controller.ts | 36 ++++++- .../src/modules/checkout/checkout.service.ts | 93 ++++++++++++++++++- .../modules/orders/order-fulfillment.spec.ts | 44 +++++++++ apps/api/src/modules/orders/orders.service.ts | 26 ++++++ .../src/components/commerce/checkout-form.tsx | 13 ++- ...r-placement-is-idempotent-by-client-key.md | 88 ++++++++++++++++++ docs/adr/README.md | 1 + docs/architecture.md | 3 +- packages/api-client/src/resources/commerce.ts | 19 +++- 11 files changed, 331 insertions(+), 14 deletions(-) create mode 100644 apps/api/src/modules/orders/order-fulfillment.spec.ts create mode 100644 docs/adr/0019-order-placement-is-idempotent-by-client-key.md diff --git a/README.md b/README.md index cb141bf..8c13f01 100644 --- a/README.md +++ b/README.md @@ -158,7 +158,7 @@ sport-store/ │ ├── docs/ │ ├── architecture.md Boundaries, conventions, risks — read this first -│ └── adr/ 18 decision records +│ └── adr/ 19 decision records │ ├── docker-compose.yml Backing services; `--profile full` runs everything ├── turbo.json pnpm-workspace.yaml package.json @@ -272,7 +272,7 @@ Everything below was run, not assumed: `pnpm format:check` clean - 7 migrations, 35 tables; seed loads 36 permissions, 6 roles, 3 brands, 8 categories, 3 collections, 12 products, **155 variants**, 64 uploaded images and 3 dev accounts -- **57 tests** — RBAC guards, password hashing, translation fallback, `Accept-Language`, the +- **60 tests** — RBAC guards, password hashing, translation fallback, `Accept-Language`, the variant matrix planner, the HTTP client's fetch receiver and retry recursion, the inventory list's variant-driven projection, the two admin-schema defects that caused silent data loss, and order-number round-tripping @@ -303,6 +303,12 @@ through Chrome with the console and network panel open: media library upload driven from the browser (presign → PUT to MinIO → register, 400×500 PNG landed at 10,962 bytes with a date-partitioned UUID key); inventory adjustment from the table wrote a ledger entry and the storefront went `OUT_OF_STOCK` → `IN_STOCK` on the next request +- **Checkout is idempotent:** a request without an `Idempotency-Key` is refused; the same key + twice returns the _same_ order rather than a second one; two simultaneous requests with one key + yield one order and one clear refusal — total order count grows by exactly one in every case +- **Order status and fulfilment agree:** `FULFILLED` and `COMPLETED` now set `fulfillmentStatus`, + so the "FULFILLED / UNFULFILLED" contradiction can no longer occur; two historical rows were + backfilled - **Search (M6):** `nocturne` and `running jacket` match exactly; `ao chay bo` finds _Áo Chạy Bộ Aero_ without diacritics; `nocturn`, `jaket` and `runing` survive their typos; `velocity` matches by brand, `crimson` by colourway, `VEL-NOC` by SKU; `zzzzqqq` correctly finds nothing. diff --git a/apps/api/src/infrastructure/redis/redis.service.ts b/apps/api/src/infrastructure/redis/redis.service.ts index 13e7755..6e7df32 100644 --- a/apps/api/src/infrastructure/redis/redis.service.ts +++ b/apps/api/src/infrastructure/redis/redis.service.ts @@ -94,6 +94,18 @@ export class RedisService implements OnModuleDestroy { return value; } + /** + * Claims a key only if nobody holds it. Returns false when someone already does. + * + * `SET … NX` is atomic, which is the entire point: a read-then-write claim is + * exactly the race the claim exists to prevent. Two simultaneous retries of + * the same request must not both believe they are first. + */ + async setIfAbsent(key: string, value: unknown, ttlSeconds: number): Promise { + const result = await this.client.set(key, JSON.stringify(value), 'EX', ttlSeconds, 'NX'); + return result === 'OK'; + } + /** * Atomic fixed-window counter. Returns the count after increment so callers * can decide to reject. diff --git a/apps/api/src/modules/checkout/checkout.controller.ts b/apps/api/src/modules/checkout/checkout.controller.ts index e08c713..475328c 100644 --- a/apps/api/src/modules/checkout/checkout.controller.ts +++ b/apps/api/src/modules/checkout/checkout.controller.ts @@ -1,11 +1,23 @@ -import { Body, Controller, Get, Inject, Param, Post, Query, Req, Res } from '@nestjs/common'; -import { ApiOperation, ApiTags } from '@nestjs/swagger'; +import { + Body, + Controller, + Get, + Headers, + Inject, + Param, + Post, + Query, + Req, + Res, +} from '@nestjs/common'; +import { ApiHeader, ApiOperation, ApiTags } from '@nestjs/swagger'; import type { Request, Response } from 'express'; import type { Cart, Locale, Order } from '@sport/types'; import { placeOrderSchema, type PlaceOrderInput } from '@sport/validation'; import { Public } from '@/common/decorators/public.decorator'; +import { AppException } from '@/common/errors/app.exception'; import { RequestLocale } from '@/common/i18n/locale.decorator'; import { ZodValidationPipe } from '@/common/pipes/zod-validation.pipe'; import { APP_CONFIG } from '@/config/app-config.module'; @@ -42,16 +54,34 @@ export class CheckoutController { return this.service.quote(token, locale); } + /** + * `Idempotency-Key` is required, not optional. + * + * This endpoint creates an order and reserves stock. A client that cannot be + * retried safely is a client that will eventually double-charge someone, and + * making the header optional means the one caller that forgets it is the one + * that does. Requiring it is a contract the storefront already honours. + */ @Post('orders') + @ApiHeader({ name: 'Idempotency-Key', required: true, description: 'Unique per attempt' }) @ApiOperation({ summary: 'Place the order and reserve stock' }) async placeOrder( @Body(new ZodValidationPipe(placeOrderSchema)) body: PlaceOrderInput, + @Headers('idempotency-key') idempotencyKey: string | undefined, @Req() request: Request, @Res({ passthrough: true }) response: Response, @RequestLocale() locale: Locale, ): Promise { + const key = idempotencyKey?.trim(); + + if (!key || key.length < 8 || key.length > 128) { + throw AppException.badRequest( + 'An Idempotency-Key header of 8–128 characters is required to place an order.', + ); + } + const { token } = resolveCartToken(request); - const order = await this.service.placeOrder(token, body, locale); + const order = await this.service.placeOrder(token, key, body, locale); // The bag is gone, so the cookie naming it should go too — otherwise the // next visit reads an empty cart under a stale token forever. diff --git a/apps/api/src/modules/checkout/checkout.service.ts b/apps/api/src/modules/checkout/checkout.service.ts index 8100858..617991b 100644 --- a/apps/api/src/modules/checkout/checkout.service.ts +++ b/apps/api/src/modules/checkout/checkout.service.ts @@ -5,9 +5,17 @@ import type { PlaceOrderInput } from '@sport/validation'; import { AppException } from '@/common/errors/app.exception'; import { PrismaService } from '@/infrastructure/prisma/prisma.service'; +import { CACHE_KEYS, CACHE_TTL } from '@/infrastructure/redis/cache-keys'; +import { RedisService } from '@/infrastructure/redis/redis.service'; import { CartsService } from '@/modules/carts/public'; import { OrdersService } from '@/modules/orders/public'; +/** What a claimed idempotency key holds while and after a placement runs. */ +interface IdempotencyRecord { + /** Set once the order is durably committed; absent means still in flight. */ + orderId?: string; +} + /** * Turns a bag into an order. * @@ -25,6 +33,7 @@ export class CheckoutService { constructor( private readonly prisma: PrismaService, + private readonly redis: RedisService, private readonly carts: CartsService, private readonly orders: OrdersService, ) {} @@ -34,7 +43,61 @@ export class CheckoutService { return this.carts.resolveForCheckout(cartToken, locale); } - async placeOrder(cartToken: string, input: PlaceOrderInput, locale: Locale): Promise { + /** + * Places an order exactly once per idempotency key. + * + * The window this closes is real and unavoidable without it: the transaction + * commits, and then the response is lost to a dropped connection or a reload. + * The shopper sees a failure, presses the button again, and buys everything + * twice. Nothing inside the transaction can prevent that, because the problem + * happens after it succeeds. + * + * The key is claimed atomically before any work starts: + * - claim succeeds -> this is the first attempt; place the order and + * record its id against the key + * - claim fails, id recorded -> a retry of a request that already + * succeeded; return the same order rather than a new one + * - claim fails, no id yet -> the original attempt is still running; + * refuse instead of racing it + * + * On failure the claim is released, so a genuine retry after a genuine error + * is not locked out for the next 24 hours. + */ + async placeOrder( + cartToken: string, + idempotencyKey: string, + input: PlaceOrderInput, + locale: Locale, + ): Promise { + const key = CACHE_KEYS.idempotency('checkout', idempotencyKey); + const claimed = await this.redis.setIfAbsent(key, {}, CACHE_TTL.idempotency); + + if (!claimed) { + const existing = await this.redis.get(key); + + if (existing?.orderId) { + this.logger.log(`Replaying order ${existing.orderId} for idempotency key`); + return this.orders.getById(existing.orderId); + } + + throw AppException.conflict('This order is already being placed. Give it a moment.'); + } + + try { + return await this.place(cartToken, key, input, locale); + } catch (error) { + // Release, so the shopper can fix whatever went wrong and try again. + await this.redis.delete(key); + throw error; + } + } + + private async place( + cartToken: string, + idempotencyCacheKey: string, + input: PlaceOrderInput, + locale: Locale, + ): Promise { const cart = await this.carts.resolveForCheckout(cartToken, locale); // A cart that had to correct itself is not one to charge against — the @@ -134,9 +197,31 @@ export class CheckoutService { return order.id; }); - // Only once the order is durably committed. Clearing first would lose a - // shopper's bag to a failed transaction. - await this.carts.clear(cartToken); + /** + * Record the id before anything else can fail. + * + * From here the order exists, so every later step has to be survivable. A + * retry that arrives after this point replays rather than duplicating. + */ + await this.redis.set(idempotencyCacheKey, { orderId }, CACHE_TTL.idempotency); + + /** + * Clearing the bag must not fail the request. + * + * The order is committed. Throwing here would report failure for something + * that succeeded, and the shopper's retry would be the exact double-order + * this method exists to prevent. A bag that outlives its order is a cosmetic + * problem; an order the customer was told failed is not. + */ + try { + await this.carts.clear(cartToken); + } catch (error) { + this.logger.error( + `Order ${orderId} placed but its cart could not be cleared: ${ + error instanceof Error ? error.message : String(error) + }`, + ); + } this.logger.log(`Order ${orderId} placed with ${cart.lines.length} line(s)`); return this.orders.getById(orderId); diff --git a/apps/api/src/modules/orders/order-fulfillment.spec.ts b/apps/api/src/modules/orders/order-fulfillment.spec.ts new file mode 100644 index 0000000..333da1c --- /dev/null +++ b/apps/api/src/modules/orders/order-fulfillment.spec.ts @@ -0,0 +1,44 @@ +import { FULFILLMENT_STATUSES, ORDER_STATUSES, type OrderStatus } from '@sport/types'; + +import { fulfillmentFor } from './orders.service'; + +/** + * `status` and `fulfillmentStatus` are separate columns because they answer + * separate questions — an order can be paid and unshipped, or shipped and + * unpaid. Two combinations are still contradictions, and this pins them. + * + * The bug this replaces was live: fulfilling an order shipped the reserved + * stock and wrote a `SALE` ledger entry while leaving `fulfillmentStatus` on + * UNFULFILLED, so the database held orders reading "FULFILLED / UNFULFILLED". + * Nothing failed loudly — it just quietly disagreed with the ledger. + */ +describe('fulfillment status derivation', () => { + it('marks anything that shipped as fulfilled', () => { + for (const status of [ORDER_STATUSES.FULFILLED, ORDER_STATUSES.COMPLETED]) { + expect(fulfillmentFor(status)).toEqual({ + fulfillmentStatus: FULFILLMENT_STATUSES.FULFILLED, + }); + } + }); + + it('leaves fulfilment alone for states where nothing shipped', () => { + for (const status of [ + ORDER_STATUSES.PENDING, + ORDER_STATUSES.CONFIRMED, + // Cancelling releases a reservation; it never ships anything, so + // UNFULFILLED stays the truth rather than being overwritten. + ORDER_STATUSES.CANCELLED, + ]) { + expect(fulfillmentFor(status)).toEqual({}); + } + }); + + it('covers every status, so a new one cannot slip through untested', () => { + const all = Object.values(ORDER_STATUSES) as OrderStatus[]; + expect(all).toHaveLength(5); + + for (const status of all) { + expect(() => fulfillmentFor(status)).not.toThrow(); + } + }); +}); diff --git a/apps/api/src/modules/orders/orders.service.ts b/apps/api/src/modules/orders/orders.service.ts index ccc97aa..6f9b006 100644 --- a/apps/api/src/modules/orders/orders.service.ts +++ b/apps/api/src/modules/orders/orders.service.ts @@ -2,7 +2,9 @@ import { Injectable } from '@nestjs/common'; import { Prisma } from '@prisma/client'; import { + FULFILLMENT_STATUSES, ORDER_STATUSES, + type FulfillmentStatus, type OffsetPaginated, type Order, type OrderListItem, @@ -149,6 +151,7 @@ export class OrdersService { where: { id, status: from }, data: { status: to, + ...fulfillmentFor(to), ...(to === ORDER_STATUSES.CONFIRMED ? { confirmedAt: new Date() } : {}), ...(to === ORDER_STATUSES.CANCELLED ? { cancelledAt: new Date(), cancelReason: input.reason?.trim() ?? null } @@ -251,3 +254,26 @@ export class OrdersService { } } } + +/** + * Keeps `fulfillmentStatus` honest about `status`. + * + * These are separate columns because they answer separate questions — an order + * can be paid and unshipped, or shipped and unpaid — but two of the values are + * not independent. Moving to FULFILLED physically ships the reserved stock and + * writes a `SALE` ledger entry; leaving `fulfillmentStatus` on UNFULFILLED + * afterwards produced orders reading "FULFILLED / UNFULFILLED", which is not an + * unusual state, it is a contradiction. + * + * CANCELLED is deliberately absent: cancelling releases a reservation and ships + * nothing, so UNFULFILLED remains the truth. + */ +export function fulfillmentFor(status: OrderStatus): { fulfillmentStatus?: FulfillmentStatus } { + switch (status) { + case ORDER_STATUSES.FULFILLED: + case ORDER_STATUSES.COMPLETED: + return { fulfillmentStatus: FULFILLMENT_STATUSES.FULFILLED }; + default: + return {}; + } +} diff --git a/apps/storefront/src/components/commerce/checkout-form.tsx b/apps/storefront/src/components/commerce/checkout-form.tsx index 3b57643..b8224d1 100644 --- a/apps/storefront/src/components/commerce/checkout-form.tsx +++ b/apps/storefront/src/components/commerce/checkout-form.tsx @@ -57,6 +57,17 @@ export function CheckoutForm({ locale }: { locale: Locale }) { const [submitting, setSubmitting] = useState(false); const [error, setError] = useState(null); + /** + * One key per checkout attempt, generated once and kept for the life of this + * form. + * + * That lifetime is exactly right. Pressing "Place order" again after a + * dropped response reuses it, so the server replays the order it already + * created instead of making a second one. Reloading the page mints a new key, + * which is a genuinely new attempt. + */ + const [idempotencyKey] = useState(() => crypto.randomUUID()); + function set(key: keyof Fields, value: string) { setFields((current) => ({ ...current, [key]: value })); } @@ -67,7 +78,7 @@ export function CheckoutForm({ locale }: { locale: Locale }) { setError(null); try { - const order = await browserApi.commerce.placeOrder(locale, { + const order = await browserApi.commerce.placeOrder(locale, idempotencyKey, { email: fields.email, shippingAddress: { fullName: fields.fullName, diff --git a/docs/adr/0019-order-placement-is-idempotent-by-client-key.md b/docs/adr/0019-order-placement-is-idempotent-by-client-key.md new file mode 100644 index 0000000..81d4db3 --- /dev/null +++ b/docs/adr/0019-order-placement-is-idempotent-by-client-key.md @@ -0,0 +1,88 @@ +# ADR-0019: Order placement is idempotent by client key + +- **Status:** Accepted +- **Date:** 2026-08-12 + +## Context + +Placing an order is a single transaction: reserve stock, write the order and its +lines, commit. That transaction is correct — it either happens completely or not +at all. + +It is also not enough, because the dangerous window opens _after_ it succeeds. +The commit lands, and then the response is lost to a dropped connection, a +closed laptop, an impatient second click, or a mobile network handing over +between cells. The shopper sees a failure for something that actually worked, +presses the button again, and buys everything twice. + +Nothing inside the transaction can prevent this. The second request is, from the +database's point of view, a perfectly legitimate new order. + +The same window swallows the step after the commit. Clearing the Redis cart was +awaited and could throw, which would have reported failure for a committed +order — turning a cosmetic Redis problem into a duplicate purchase. + +## Decision + +**`POST /checkout/orders` requires an `Idempotency-Key` header.** Not optional: +this endpoint creates an order and reserves stock, and a client that cannot be +retried safely is a client that will eventually double-charge someone. Making it +optional means the one caller that forgets is the one that does the damage. + +**The key is claimed atomically before any work begins**, via `SET NX` on +`idempotency:checkout:` with a 24-hour TTL: + +| Claim | Meaning | Response | +| ------------------ | -------------------------------------- | ---------------------------------------------- | +| Succeeds | First attempt | Place the order, record its id against the key | +| Fails, id recorded | Retry of something that already worked | Return the same order | +| Fails, no id yet | Original attempt still running | `409` — refuse rather than race it | + +**The order id is recorded before anything else can fail**, so every step after +the commit is survivable. + +**Clearing the cart can no longer fail the request.** It is logged and swallowed: +a bag that outlives its order is cosmetic; an order the customer was told failed +is not. + +**On failure, the claim is released**, so a genuine retry after a genuine error +is not locked out for 24 hours. + +**The storefront generates one key per checkout attempt**, held in component +state. Pressing the button again reuses it; reloading the page mints a new one, +which is a genuinely new attempt. + +## Consequences + +A retried checkout returns the original order rather than creating a second. A +double-click gets one order and one clear refusal. A committed order survives a +Redis outage during cleanup. + +The 24-hour TTL means an idempotency record outlives any plausible retry but +does not accumulate forever. Redis losing it is safe in the direction that +matters — ADR-0010 already establishes that Redis can be flushed at any time, +and the worst case here degrades to the behaviour we had before this ADR rather +than to something worse. + +Every future client of this endpoint — a mobile app, a POS, an ERP integration — +must generate keys. That is a real constraint and the correct one. + +Payments (M9) will attach to the same key. A provider callback that arrives +twice is the identical problem with a larger blast radius. + +## Alternatives considered + +**Derive the key from the cart contents.** Requires no client cooperation and is +wrong: a customer legitimately reordering the same items an hour later would be +handed their old order. + +**A unique constraint on (email, total, minute).** A guess dressed as a +constraint. It blocks legitimate rapid reorders and misses duplicates that +straddle a minute boundary. + +**Let the client detect it.** The client is exactly the party that cannot know — +it did not receive the response, which is the entire problem. + +**Make the header optional with a fallback.** The fallback is either unsafe or +one of the rejected options above, and an optional safety mechanism protects +only the callers that did not need protecting. diff --git a/docs/adr/README.md b/docs/adr/README.md index 2077f1b..ba8fb5d 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -27,6 +27,7 @@ An ADR is immutable once accepted. If a decision changes, add a new ADR that sup | [0016](./0016-option-values-are-retained-when-variants-reference-them.md) | Option values are retained when variants reference them | Accepted | | [0017](./0017-shadcn-for-infrastructure-hand-built-for-brand.md) | shadcn/ui for infrastructure, hand-built for brand | Accepted | | [0018](./0018-orders-snapshot-everything-they-display.md) | Orders snapshot everything they display | Accepted | +| [0019](./0019-order-placement-is-idempotent-by-client-key.md) | Order placement is idempotent by client key | Accepted | ## Decisions deliberately NOT recorded yet diff --git a/docs/architecture.md b/docs/architecture.md index 7bb725e..54a4de9 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -574,7 +574,8 @@ Stated plainly so they are choices rather than oversights. - Search: PostgreSQL full-text with weighted documents, diacritic folding and trigram typo tolerance, behind a `SearchProvider` seam (ADR-0012). The index is a projection owned by SearchModule and rebuilt from a domain event, so nothing else knows it exists. -- Commerce: a Redis-backed guest bag priced from the live catalog on every read, guest checkout, +- Commerce: idempotent order placement keyed by a required client header (ADR-0019), a + Redis-backed guest bag priced from the live catalog on every read, guest checkout, orders that snapshot everything they display (ADR-0018), stock reserved at placement and either released on cancel or shipped on fulfilment, and an admin order lifecycle with an explicit transition table. diff --git a/packages/api-client/src/resources/commerce.ts b/packages/api-client/src/resources/commerce.ts index e40dc03..8e40be4 100644 --- a/packages/api-client/src/resources/commerce.ts +++ b/packages/api-client/src/resources/commerce.ts @@ -54,7 +54,17 @@ export interface CommerceResource { removeCartLine(locale: Locale, variantId: string, options?: RequestOptions): Promise; getCheckoutQuote(locale: Locale, options?: RequestOptions): Promise; - placeOrder(locale: Locale, input: PlaceOrderInput, options?: RequestOptions): Promise; + /** + * `idempotencyKey` must be stable across retries of the *same* attempt and + * different between attempts — the server uses it to tell a resend apart from + * a second purchase. + */ + placeOrder( + locale: Locale, + idempotencyKey: string, + input: PlaceOrderInput, + options?: RequestOptions, + ): Promise; lookupOrder(orderNumber: string, email: string, options?: RequestOptions): Promise; getPlacedOrder(id: string, options?: RequestOptions): Promise; } @@ -112,8 +122,11 @@ export function createCommerceResource(http: HttpClient): CommerceResource { getCheckoutQuote: (locale, options) => http.get('/checkout/quote', cartOptions(locale, options)), - placeOrder: (locale, input, options) => - http.post('/checkout/orders', input, cartOptions(locale, options)), + placeOrder: (locale, idempotencyKey, input, options) => + http.post('/checkout/orders', input, { + ...cartOptions(locale, options), + headers: { 'Idempotency-Key': idempotencyKey, ...options?.headers }, + }), getPlacedOrder: (id, options) => http.get(`/checkout/orders/${encodeURIComponent(id)}`, {