bgard68 · bgard68 · Aug 25, 2026 · Aug 25, 2026
@@ -44,7 +44,8 @@ docker compose up --build
 | Store (SPA) | http://localhost:3000/store |
 | **Mailpit** — every email the app sends | http://localhost:8025 |
 | API + Scalar (interactive API UI) | http://localhost:8080/scalar/v1 |
-| Health | http://localhost:8080/health |
+| Health (liveness — no database) | http://localhost:8080/health |
+| Readiness (queries the database) | http://localhost:8080/health/ready |
 
 Migrations and demo seed run automatically on API start. For running the API on the host
 with fast iteration (and the exact port/user-secrets details), see

@@ -29,7 +29,8 @@ Then open:
 | Store (SPA) | http://localhost:3000/store |
 | **Mailpit** — every email the app sends | http://localhost:8025 |
 | API + Scalar (interactive API UI) | http://localhost:8080/scalar/v1 |
-| Health | http://localhost:8080/health |
+| Health (liveness — no database) | http://localhost:8080/health |
+| Readiness (queries the database) | http://localhost:8080/health/ready |
 
 The landing page at `/` explains the demo, states that no payment is ever taken, and lists
 the accounts below with what each role can do — it's the same guide, in the app.
@@ -109,4 +110,5 @@ values and why.
   [Configuration](04-configuration-and-2fa.md#reading-real-mail-locally-mailpit).
 - **API up but every request 503** — the database was unreachable at startup, so migrations
   were skipped and the app is running degraded on purpose rather than restart-looping.
-  `/health` says so; fix the connection and restart the API.
+  `/health` says so; fix the connection and restart the API. Once running, `/health/ready`
+  is the one that keeps checking — `/health` only reports how startup went.
@@ -56,6 +56,9 @@ by policy.
 | `Email:Host/Port/Username/Password/...` | SMTP settings | env / secret store; user-secrets in dev | Password is secret. |
 | `Cors:AllowedOrigins` | Browser origins allowed to call the API | env / appsettings | Not secret. |
 | `App:BaseUrl` | Public SPA URL (used in email links) | env / appsettings | Not secret. |
+| `RateLimiting:TrustForwardedFor` | Whether `X-Forwarded-For` may be believed | env / appsettings | Not secret, but **load-bearing** — see below. |
+| `RateLimiting:Auth\|Checkout\|Lookup` | Throttling budgets per caller | env / appsettings | Not secret; tune during an incident without a redeploy. |
+| `Reservations:*` | Stale-reservation sweep window, interval, batch, on/off | env / appsettings | Not secret. |
 | `VITE_API_BASE_URL`, `VITE_GOOGLE_CLIENT_ID` | Web build-time config | GitHub Actions **Variables** (CI) / `web/.env.local` (dev) | Public; injected at build time, never committed. |
 
 **Why user-secrets vs `.env`:** user-secrets is read by the .NET app when you run it directly
@@ -64,6 +67,67 @@ container can't read your host user-secrets, so the Docker path uses a git-ignor
 Compose variable substitution. In CI/prod, use **GitHub Actions Secrets** and **Azure App Service
 settings / Key Vault**. See [`docs/local-development.md`](../local-development.md).
 
+## Throttling, and the one setting that can cause an outage
+
+Sign-in, registration, password reset, guest order lookup and checkout are rate limited. Budgets
+live in `appsettings.json` so they can be tightened during an incident without a redeploy:
+
+```json
+"RateLimiting": {
+  "TrustForwardedFor": false,
+  "Auth":     { "PermitLimit": 20, "WindowSeconds": 60 },
+  "Checkout": { "PermitLimit":  8, "WindowSeconds": 60 },
+  "Lookup":   { "PermitLimit": 10, "WindowSeconds": 60 }
+}
+```
+
+**`TrustForwardedFor` decides whether throttling works at all behind a proxy.** The limiter
+partitions by caller. Behind a reverse proxy every request arrives carrying the *proxy's* address,
+so with this left `false` every caller in the world collapses into one partition and the limiter
+becomes a global cap that the first busy minute trips for everybody — a self-inflicted outage with
+nothing in the logs to explain it.
+
+The inverse is the security mistake: set `true` with no proxy in front and a caller can forge the
+header, minting a fresh partition per request and opting out of throttling entirely.
+
+| Deployment | Setting |
+|---|---|
+| Behind App Service, a load balancer, Cloudflare, any reverse proxy | `true` |
+| Direct to the app, local development, container with no proxy | `false` |
+
+The app watches real traffic and logs a warning **once** when the setting and the traffic disagree,
+in either direction. If you see either warning, the setting is wrong — it is not advisory.
+
+There is deliberately **no global limiter**: a catalogue page issues several requests in a burst, so
+a global cap would throttle ordinary browsing while adding nothing an endpoint policy does not
+already do.
+
+## Reclaiming stock from unfinished payments
+
+An order whose payment settles asynchronously holds its stock while it waits for a provider webhook.
+If that webhook never arrives — provider outage, dropped delivery, a shopper closing the tab at the
+bank's redirect — the stock would be held forever. A background sweep returns it:
+
+```json
+"Reservations": {
+  "Enabled": true,
+  "ExpireAfterMinutes": 15,
+  "SweepIntervalMinutes": 5,
+  "BatchSize": 100
+}
+```
+
+`ExpireAfterMinutes` is a trade, not a tuning knob. Too short and a slow but honest bank redirect
+loses a customer's basket; too long and abandoned or abusive orders hold the catalogue hostage.
+Fifteen minutes is longer than any interactive redirect and short enough that a returning shopper
+rarely finds the item gone.
+
+`BatchSize` caps one pass, so a backlog is worked through over several sweeps rather than one long
+transaction. `Enabled: false` turns the sweep off for a host that should not run background work.
+
+Releasing reuses the same transactional path as a reported payment failure, so a webhook landing
+during a sweep cannot double-release: whichever writes first wins and the other is declined.
+
 ## Email setup
 
 `IEmailSender` has two adapters, chosen by `Email:Provider`:

@@ -56,6 +56,12 @@ Rows 1–11 are from the first phase, 12–32 from the second, 33–39 from the
 | 37 | A challenge-token test failed against a correct implementation | writing tests around `ValidateChallengeTokenAsync` | Issuance uses the injected `TimeProvider` but `TokenValidationParameters` has no such hook, so lifetime is validated against the **system** clock. A token minted at a fixed past date is born expired | Anchor those tests on real time and move the *issuing* clock to express age | "Inject time everywhere" holds only as far as the libraries let it; find the seams that don't take your clock |
 | 38 | An integration test asserted a uniqueness rule the schema does not have | the test failed | `ix_widgets_live_name` is a plain index for ordering the live set, not a unique one. Only SKU is unique (case-folded via `upper(sku)`) | Assert the rule that exists, and add a test documenting that names are deliberately **not** unique | Read the migration, not your memory of it — a test that asserts an imagined constraint fails honestly, but the same assumption in code would not |
 | 39 | New frontend tests passed but `tsc` failed the build | running `npm run build`, not just `npm test` | `.at(-1)` again — the **same ES2022-against-ES2020 trap as row 23** — plus a `let x = null` only assigned inside a Promise executor, which TypeScript narrows to `never` | Index arithmetic, and `let release!: () => void` | Vitest transpiles without type-checking, so a green test run says nothing about the build. Run the gate CI runs |
+| 40 | An admin renaming a widget could silently revert a live reservation | reading the write path after the inventory work | One `UpdateAsync` wrote **every** column from the caller's in-memory object, and three handlers used load-change-save. A reservation taken between the read and the save was overwritten — an oversell caused by an edit, with no attacker and no error | Split the write path by intent — details, stock, archive — and move the stock arithmetic and its guards into the `UPDATE` itself | A repository method that writes every column turns every caller into a potential lost update. Write what you mean, not the whole row |
+| 41 | Two deliveries of one payment webhook released the same stock twice | tracing the settlement path | The handler guarded with read-check-write, which two concurrent deliveries both pass. The second compensation decremented `quantity_reserved` again and ate stock held by a *different* order | Compare-and-set: each payment write names the statuses it may apply from and returns whether it won; only the winner compensates | Application-level status checks are courtesy. If two callers can race, the row has to be the arbiter |
+| 42 | Order numbers collided at a few thousand orders a day | arithmetic, not a failure | The suffix was 6 hex characters of the order's Guid — 24 bits, scoped to one day. `order_number` is uniquely indexed, so a collision was never a leak, but it rolled the placement back: a hard checkout failure | Widen to 10 characters (40 bits) and pin the width with a test | Collisions arrive by the birthday bound, not when the space runs out. 16.7 million values is a coin flip at 5,000 a day |
+| 43 | The API test suite passed individually and failed together | adding rate limiting | The test server sends no remote address, so every request shared one throttling partition and the suite exhausted a realistic budget between its own tests | Raise the budgets in the fixture explicitly, and prove rejection separately with a host configured down to two | The suite reproduced the exact production failure mode — all callers in one partition — which is the trap behind any reverse proxy |
+| 44 | The health endpoint could never report unhealthy | asking an operations question, not a security one | `/health` closed over a variable captured at **startup**, so once the process was up it answered `ok` forever — database gone, still `ok`. The keep-warm ping held it in rotation on an answer that could not change | Add `/health/ready` that queries the database; leave `/health` shallow | The obvious fix — query the database in `/health` — would have cost ~180 CU-hrs against a 100 CU-hr budget. The shallow probe was deliberate and said so in a comment |
+| 45 | CodeQL found log injection in the code written to fix a logging gap | the scanner blocked the merge | The new exception handler logged `Request.Path.Value` raw. That is the **decoded** path, so `%0A` in a URL arrives as a real newline and the caller writes their own log entry (CWE-117) | `LogSafe.Text` strips control characters from the path and method, keeping printable oddities | The correlation id on the line beside it *was* sanitised, with a comment citing this exact weakness. Knowing a rule is not the same as applying it everywhere it holds |
 
 ## Lessons learned
 
@@ -73,6 +79,22 @@ Rows 1–11 are from the first phase, 12–32 from the second, 33–39 from the
   **and** by a database trigger — a bug or a direct SQL write still can’t violate it.
 - **Never trust the client for money.** Totals — subtotal, shipping, and per-state tax — are
   recomputed server-side at checkout; the browser’s numbers are display-only.
+- **Let the row arbitrate, not the handler.** Read-check-write reads like a guard and is not
+  one: any two callers who can race will both pass it. Compare-and-set — naming the states a
+  write may apply from and acting only when it wins — turned duplicate webhooks, out-of-order
+  events and concurrent sweeps from bugs into no-ops, and it is one `where` clause.
+- **Write what you mean, not the whole row.** A single repository method that set every
+  column made every caller a possible lost update, including an admin editing a product name
+  during a checkout. Splitting writes by intent removed a whole class of defect rather than
+  one instance of it.
+- **Read the reasoning before changing the code.** The obvious fix for a health check that
+  could not fail was to make it query the database — which would have doubled the database
+  bill, for reasons written down in the workflow that pings it. It would have passed every
+  test and looked right in review.
+- **The gates catch what the author cannot.** Three real defects in this round were found by
+  CI, by the coverage floor, and by CodeQL — not by re-reading the diff. The one that stings
+  is the last: log injection in a handler whose *neighbouring line* was sanitised against
+  exactly that weakness.
 - **Secrets discipline pays off.** `.gitignore` + `.gitleaks.toml` + an always-on secret
   scan meant “no secret in the repo” was enforced, not aspirational — and the one allowed
   exception (documented demo creds) is explicit.

@@ -25,6 +25,54 @@ Restart the API after changing config.
 
 ---
 
+## Health probes — which one to point at what
+
+Two endpoints answering two different questions. Wiring the wrong one is how a monitoring signal
+ends up unable to report bad news.
+
+| Endpoint | Answers | Touches the database | Point this at |
+|---|---|---|---|
+| `GET /health` | Did this process start correctly? | **No** | Keep-warm pings, first-boot provisioning checks |
+| `GET /health/ready` | Can this instance serve a request right now? | **Yes** — `select 1` | Platform health probe, alerting, load-balancer rotation |
+
+```bash
+curl -i http://localhost:8080/health          # {"status":"ok",...}
+curl -i http://localhost:8080/health/ready    # {"status":"ready","database":"ok",...}
+```
+
+**Why they are separate, and why it matters for the bill.** `/health` is pinged every few minutes to
+hold a free-tier App Service instance loaded. If that ping woke the database each time it would hold
+a metered resource awake around the clock — roughly 180 CU-hrs against Neon's 100 CU-hr monthly free
+allowance. Warming the app while letting the database sleep is deliberate.
+
+So: **never point a scheduled warm-up at `/health/ready`**, and never make `/health` query the
+database. They look interchangeable and are not.
+
+`/health/ready` returns `503` with the failing exception *type* when the database does not answer —
+never the message, because a connection error can carry a host name or a user and the endpoint is
+anonymous.
+
+## Tracing a failure a customer reports
+
+Every response carries an `X-Correlation-Id` header, and a `500` repeats it in the body:
+
+```json
+{ "error": "Something went wrong on our side. Quote the reference below if you contact us.",
+  "correlationId": "0HN7…" }
+```
+
+The same id is on the log line for that request, so a customer report becomes a lookup rather than a
+search through everything that happened at that minute:
+
+```bash
+# Azure App Service log stream, or wherever logs land
+az webapp log tail --name <app> --resource-group <rg> | grep 0HN7
+```
+
+If the caller supplied `X-Correlation-Id`, that value is kept so a trace spans several services —
+sanitised first, because it reaches log messages and text carrying newlines could otherwise forge
+whole entries.
+
 ## Email
 
 ### Test locally — Dev sender (default, zero setup)

@@ -17,5 +17,26 @@
     "DemoAdminEmail": "admin@widgetworks.demo",
     "DemoCustomerEmail": "demo@widgetworks.demo",
     "DemoManagerEmail": "manager@widgetworks.demo"
+  },
+  "RateLimiting": {
+    "TrustForwardedFor": false,
+    "Auth": {
+      "PermitLimit": 20,
+      "WindowSeconds": 60
+    },
+    "Checkout": {
+      "PermitLimit": 8,
+      "WindowSeconds": 60
+    },
+    "Lookup": {
+      "PermitLimit": 10,
+      "WindowSeconds": 60
+    }
+  },
+  "Reservations": {
+    "Enabled": true,
+    "ExpireAfterMinutes": 15,
+    "SweepIntervalMinutes": 5,
+    "BatchSize": 100
   }
 }