Fix error before M7

This commit is contained in:
Nông Đức Huy
2026-08-13 23:20:23 +07:00
parent 624a6402bf
commit 1e356a2578
11 changed files with 331 additions and 14 deletions
+8 -2
View File
@@ -158,7 +158,7 @@ sport-store/
│ │
├── docs/ ├── docs/
│ ├── architecture.md Boundaries, conventions, risks — read this first │ ├── 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 ├── docker-compose.yml Backing services; `--profile full` runs everything
├── turbo.json pnpm-workspace.yaml package.json ├── turbo.json pnpm-workspace.yaml package.json
@@ -272,7 +272,7 @@ Everything below was run, not assumed:
`pnpm format:check` clean `pnpm format:check` clean
- 7 migrations, 35 tables; seed loads 36 permissions, 6 roles, 3 brands, 8 categories, - 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 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 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 list's variant-driven projection, the two admin-schema defects that caused silent data loss, and
order-number round-tripping 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 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 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 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ộ - **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` 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. matches by brand, `crimson` by colourway, `VEL-NOC` by SKU; `zzzzqqq` correctly finds nothing.
@@ -94,6 +94,18 @@ export class RedisService implements OnModuleDestroy {
return value; 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<boolean> {
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 * Atomic fixed-window counter. Returns the count after increment so callers
* can decide to reject. * can decide to reject.
@@ -1,11 +1,23 @@
import { Body, Controller, Get, Inject, Param, Post, Query, Req, Res } from '@nestjs/common'; import {
import { ApiOperation, ApiTags } from '@nestjs/swagger'; 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 { Request, Response } from 'express';
import type { Cart, Locale, Order } from '@sport/types'; import type { Cart, Locale, Order } from '@sport/types';
import { placeOrderSchema, type PlaceOrderInput } from '@sport/validation'; import { placeOrderSchema, type PlaceOrderInput } from '@sport/validation';
import { Public } from '@/common/decorators/public.decorator'; import { Public } from '@/common/decorators/public.decorator';
import { AppException } from '@/common/errors/app.exception';
import { RequestLocale } from '@/common/i18n/locale.decorator'; import { RequestLocale } from '@/common/i18n/locale.decorator';
import { ZodValidationPipe } from '@/common/pipes/zod-validation.pipe'; import { ZodValidationPipe } from '@/common/pipes/zod-validation.pipe';
import { APP_CONFIG } from '@/config/app-config.module'; import { APP_CONFIG } from '@/config/app-config.module';
@@ -42,16 +54,34 @@ export class CheckoutController {
return this.service.quote(token, locale); 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') @Post('orders')
@ApiHeader({ name: 'Idempotency-Key', required: true, description: 'Unique per attempt' })
@ApiOperation({ summary: 'Place the order and reserve stock' }) @ApiOperation({ summary: 'Place the order and reserve stock' })
async placeOrder( async placeOrder(
@Body(new ZodValidationPipe(placeOrderSchema)) body: PlaceOrderInput, @Body(new ZodValidationPipe(placeOrderSchema)) body: PlaceOrderInput,
@Headers('idempotency-key') idempotencyKey: string | undefined,
@Req() request: Request, @Req() request: Request,
@Res({ passthrough: true }) response: Response, @Res({ passthrough: true }) response: Response,
@RequestLocale() locale: Locale, @RequestLocale() locale: Locale,
): Promise<Order> { ): Promise<Order> {
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 { 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 // 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. // next visit reads an empty cart under a stale token forever.
@@ -5,9 +5,17 @@ import type { PlaceOrderInput } from '@sport/validation';
import { AppException } from '@/common/errors/app.exception'; import { AppException } from '@/common/errors/app.exception';
import { PrismaService } from '@/infrastructure/prisma/prisma.service'; 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 { CartsService } from '@/modules/carts/public';
import { OrdersService } from '@/modules/orders/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. * Turns a bag into an order.
* *
@@ -25,6 +33,7 @@ export class CheckoutService {
constructor( constructor(
private readonly prisma: PrismaService, private readonly prisma: PrismaService,
private readonly redis: RedisService,
private readonly carts: CartsService, private readonly carts: CartsService,
private readonly orders: OrdersService, private readonly orders: OrdersService,
) {} ) {}
@@ -34,7 +43,61 @@ export class CheckoutService {
return this.carts.resolveForCheckout(cartToken, locale); return this.carts.resolveForCheckout(cartToken, locale);
} }
async placeOrder(cartToken: string, input: PlaceOrderInput, locale: Locale): Promise<Order> { /**
* 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<Order> {
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<IdempotencyRecord>(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<Order> {
const cart = await this.carts.resolveForCheckout(cartToken, locale); const cart = await this.carts.resolveForCheckout(cartToken, locale);
// A cart that had to correct itself is not one to charge against — the // A cart that had to correct itself is not one to charge against — the
@@ -134,9 +197,31 @@ export class CheckoutService {
return order.id; return order.id;
}); });
// Only once the order is durably committed. Clearing first would lose a /**
// shopper's bag to a failed transaction. * Record the id before anything else can fail.
await this.carts.clear(cartToken); *
* 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)`); this.logger.log(`Order ${orderId} placed with ${cart.lines.length} line(s)`);
return this.orders.getById(orderId); return this.orders.getById(orderId);
@@ -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();
}
});
});
@@ -2,7 +2,9 @@ import { Injectable } from '@nestjs/common';
import { Prisma } from '@prisma/client'; import { Prisma } from '@prisma/client';
import { import {
FULFILLMENT_STATUSES,
ORDER_STATUSES, ORDER_STATUSES,
type FulfillmentStatus,
type OffsetPaginated, type OffsetPaginated,
type Order, type Order,
type OrderListItem, type OrderListItem,
@@ -149,6 +151,7 @@ export class OrdersService {
where: { id, status: from }, where: { id, status: from },
data: { data: {
status: to, status: to,
...fulfillmentFor(to),
...(to === ORDER_STATUSES.CONFIRMED ? { confirmedAt: new Date() } : {}), ...(to === ORDER_STATUSES.CONFIRMED ? { confirmedAt: new Date() } : {}),
...(to === ORDER_STATUSES.CANCELLED ...(to === ORDER_STATUSES.CANCELLED
? { cancelledAt: new Date(), cancelReason: input.reason?.trim() ?? null } ? { 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 {};
}
}
@@ -57,6 +57,17 @@ export function CheckoutForm({ locale }: { locale: Locale }) {
const [submitting, setSubmitting] = useState(false); const [submitting, setSubmitting] = useState(false);
const [error, setError] = useState<string | null>(null); const [error, setError] = useState<string | null>(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) { function set(key: keyof Fields, value: string) {
setFields((current) => ({ ...current, [key]: value })); setFields((current) => ({ ...current, [key]: value }));
} }
@@ -67,7 +78,7 @@ export function CheckoutForm({ locale }: { locale: Locale }) {
setError(null); setError(null);
try { try {
const order = await browserApi.commerce.placeOrder(locale, { const order = await browserApi.commerce.placeOrder(locale, idempotencyKey, {
email: fields.email, email: fields.email,
shippingAddress: { shippingAddress: {
fullName: fields.fullName, fullName: fields.fullName,
@@ -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:<key>` 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.
+1
View File
@@ -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 | | [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 | | [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 | | [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 ## Decisions deliberately NOT recorded yet
+2 -1
View File
@@ -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 - 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 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. 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 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 released on cancel or shipped on fulfilment, and an admin order lifecycle with an explicit
transition table. transition table.
+16 -3
View File
@@ -54,7 +54,17 @@ export interface CommerceResource {
removeCartLine(locale: Locale, variantId: string, options?: RequestOptions): Promise<Cart>; removeCartLine(locale: Locale, variantId: string, options?: RequestOptions): Promise<Cart>;
getCheckoutQuote(locale: Locale, options?: RequestOptions): Promise<Cart>; getCheckoutQuote(locale: Locale, options?: RequestOptions): Promise<Cart>;
placeOrder(locale: Locale, input: PlaceOrderInput, options?: RequestOptions): Promise<Order>; /**
* `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<Order>;
lookupOrder(orderNumber: string, email: string, options?: RequestOptions): Promise<Order>; lookupOrder(orderNumber: string, email: string, options?: RequestOptions): Promise<Order>;
getPlacedOrder(id: string, options?: RequestOptions): Promise<Order>; getPlacedOrder(id: string, options?: RequestOptions): Promise<Order>;
} }
@@ -112,8 +122,11 @@ export function createCommerceResource(http: HttpClient): CommerceResource {
getCheckoutQuote: (locale, options) => getCheckoutQuote: (locale, options) =>
http.get<Cart>('/checkout/quote', cartOptions(locale, options)), http.get<Cart>('/checkout/quote', cartOptions(locale, options)),
placeOrder: (locale, input, options) => placeOrder: (locale, idempotencyKey, input, options) =>
http.post<Order>('/checkout/orders', input, cartOptions(locale, options)), http.post<Order>('/checkout/orders', input, {
...cartOptions(locale, options),
headers: { 'Idempotency-Key': idempotencyKey, ...options?.headers },
}),
getPlacedOrder: (id, options) => getPlacedOrder: (id, options) =>
http.get<Order>(`/checkout/orders/${encodeURIComponent(id)}`, { http.get<Order>(`/checkout/orders/${encodeURIComponent(id)}`, {