30 async route handlers still bypass the error middleware #59

New Issue

2026-08-19T11:03:57-05:00

bermudalamb commented

2026-08-19 11:03:57 -05:00

Found during a Microsoft/React/SonarQube best-practices review.

Problem

Express 4 does not forward a rejected promise from an async handler. The repo already knows this — asyncRoute exists for exactly this reason, and the error middleware in app.ts was added after the 2026-08-17 production incident, where an unhandled rejection left every item query hanging with no response and the storefront rendered it as an empty shop.

That fix was only applied to some routes. Current coverage:

wrapped in asyncRoute: 25
bare async handlers: 30

The unwrapped ones include the most heavily used paths in the application:

File	Routes
`backend/src/routes/customers.ts`	`register`, `login`, `logout`, `verify-email`, `change-password`, `me`, `me/orders`, `me/export`, `DELETE me`, `me/consent`
`backend/src/routes/cart.ts`	`GET /`, `POST /items/:itemId`, `DELETE /items/:itemId`
`backend/src/routes/cartCheckout.ts`	`paypal/create`, `paypal/capture`, `demo/purchase`
`backend/src/routes/shippingAddresses.ts`	all five
`backend/src/routes/admin.ts`	`POST /items`, `PUT /items/:id`, `mark-sold`, `mark-available`, image delete
`backend/src/routes/adminSettings.ts`	`GET /`, `PUT /`
`backend/src/routes/public.ts`	`unsubscribe`

Spot-checking three of them — customers.ts:47 (register), cart.ts:29, adminSettings.ts:6 — none has an internal try/catch either. They are bare await pool.query(...) calls.

Why this matters more than a normal missing-try/catch

The failure mode is silence, not an error. A rejection produces no response at all: the request hangs until the client times out, nothing reaches the error middleware, nothing is logged as a failure, and monitoring sees an open connection rather than a 500. That is the precise shape of the incident this project has already had once, and it is why asyncRoute was written.

POST /api/customers/register hanging is not a hypothetical — a unique-constraint race, a connection-pool exhaustion, or a Postgres restart all produce it.

Suggested fix

Wrap the remaining 30 in asyncRoute. It is mechanical, but worth doing in one pass rather than opportunistically, because the value is in the guarantee holding everywhere rather than mostly.

Worth deciding: whether to make this enforceable rather than remembered. Options include a lint rule, or replacing the pattern entirely with an Express 5 upgrade (Express 5 forwards rejected promises natively, which would make asyncRoute unnecessary). A convention that has already been half-forgotten once will be forgotten again.

Severity

High. Reliability and availability, on a known-recurring failure mode.

Found during a Microsoft/React/SonarQube best-practices review. ## Problem Express 4 does not forward a rejected promise from an async handler. The repo already knows this — `asyncRoute` exists for exactly this reason, and the error middleware in `app.ts` was added after the 2026-08-17 production incident, where an unhandled rejection left every item query hanging with no response and the storefront rendered it as an empty shop. **That fix was only applied to some routes.** Current coverage: - wrapped in `asyncRoute`: **25** - bare `async` handlers: **30** The unwrapped ones include the most heavily used paths in the application: | File | Routes | | --- | --- | | `backend/src/routes/customers.ts` | `register`, `login`, `logout`, `verify-email`, `change-password`, `me`, `me/orders`, `me/export`, `DELETE me`, `me/consent` | | `backend/src/routes/cart.ts` | `GET /`, `POST /items/:itemId`, `DELETE /items/:itemId` | | `backend/src/routes/cartCheckout.ts` | `paypal/create`, `paypal/capture`, `demo/purchase` | | `backend/src/routes/shippingAddresses.ts` | all five | | `backend/src/routes/admin.ts` | `POST /items`, `PUT /items/:id`, `mark-sold`, `mark-available`, image delete | | `backend/src/routes/adminSettings.ts` | `GET /`, `PUT /` | | `backend/src/routes/public.ts` | `unsubscribe` | Spot-checking three of them — `customers.ts:47` (register), `cart.ts:29`, `adminSettings.ts:6` — none has an internal `try`/`catch` either. They are bare `await pool.query(...)` calls. ## Why this matters more than a normal missing-try/catch The failure mode is silence, not an error. A rejection produces **no response at all**: the request hangs until the client times out, nothing reaches the error middleware, nothing is logged as a failure, and monitoring sees an open connection rather than a 500. That is the precise shape of the incident this project has already had once, and it is why `asyncRoute` was written. `POST /api/customers/register` hanging is not a hypothetical — a unique-constraint race, a connection-pool exhaustion, or a Postgres restart all produce it. ## Suggested fix Wrap the remaining 30 in `asyncRoute`. It is mechanical, but worth doing in one pass rather than opportunistically, because the value is in the guarantee holding everywhere rather than mostly. Worth deciding: whether to make this enforceable rather than remembered. Options include a lint rule, or replacing the pattern entirely with an Express 5 upgrade (Express 5 forwards rejected promises natively, which would make `asyncRoute` unnecessary). A convention that has already been half-forgotten once will be forgotten again. ## Severity High. Reliability and availability, on a known-recurring failure mode.

bermudalamb added this to the Code Quality and Hardening project 2026-08-19 11:37:20 -05:00

bermudalamb moved this to In Progress in Code Quality and Hardening on 2026-08-19 13:05:00 -05:00

bermudalamb commented

2026-08-19 13:05:40 -05:00

Starting work on this. Two things decided before implementation, recorded here.

Inventory confirms the count, and turns up one omission

Exactly 30 bare async handlers, matching the issue: customers.ts 11, admin.ts 5, shippingAddresses.ts 5, cart.ts 3, cartCheckout.ts 3, adminSettings.ts 2, public.ts 1.

The issue's list misses one case that is worse than any of the 30: attachCustomer in backend/src/middleware/customerAuth.ts:12 is a bare async middleware mounted globally at app.ts:27, doing an unguarded await pool.query(...). A rejection there hangs every request in the application, including the 25 handlers that are already wrapped correctly — so the guarantee this issue is trying to establish does not actually hold anywhere until this one is fixed too. It is in scope for this change.

Question: how far past the mechanical wrap should this go?

The issue explicitly leaves open whether to make the convention enforceable rather than remembered. Options considered:

Chosen — wrap everything, plus a source-scanning unit test. A unit test walks src/routes/ and fails if any router.<verb>(..., async ...) is not wrapped in asyncRoute. It is enforceable in CI today, needs no new tooling, doubles as the TDD red test for this change, and is deleted the day the project moves to Express 5. It closes the "half-forgotten once will be forgotten again" gap without expanding the issue.

Rejected — wrap only. Smallest diff, but leaves enforcement waiting on ESLint (#60). That reproduces exactly the situation this issue is reporting: a convention held by memory. The issue itself argues against this.

Rejected — per-file integration tests forcing a rejection. Strongest proof of behaviour, but slow to write and it only ever covers the routes someone thought to test. A new unwrapped route added next month passes it silently. The guard test catches that; these would not.

Rejected — upgrade to Express 5. The correct permanent fix, and it would delete asyncRoute outright. Rejected for this issue because it is a breaking upgrade — path-to-regexp v8 route syntax, req.query becoming a getter, @types/express v5 — and pairing a breaking framework upgrade with a reliability fix means a rollback of one is a rollback of both. Worth its own issue; the guard test is written so it disappears cleanly when that lands.

Longer term the enforcement belongs in ESLint via @typescript-eslint/no-misused-promises, which flags this class of bug directly. That is #60's scope, not this one.

Question: how does an issue move to In Progress?

Asked because the Gitea MCP tooling reaches issues, labels and milestones but not project boards, and this repo has no labels defined. Answer: the board is moved by hand; I do not need to touch it.

Starting work on this. Two things decided before implementation, recorded here. ## Inventory confirms the count, and turns up one omission Exactly 30 bare `async` handlers, matching the issue: `customers.ts` 11, `admin.ts` 5, `shippingAddresses.ts` 5, `cart.ts` 3, `cartCheckout.ts` 3, `adminSettings.ts` 2, `public.ts` 1. The issue's list misses one case that is worse than any of the 30: `attachCustomer` in `backend/src/middleware/customerAuth.ts:12` is a bare `async` middleware mounted globally at `app.ts:27`, doing an unguarded `await pool.query(...)`. A rejection there hangs **every** request in the application, including the 25 handlers that are already wrapped correctly — so the guarantee this issue is trying to establish does not actually hold anywhere until this one is fixed too. It is in scope for this change. ## Question: how far past the mechanical wrap should this go? The issue explicitly leaves open whether to make the convention enforceable rather than remembered. Options considered: **Chosen — wrap everything, plus a source-scanning unit test.** A unit test walks `src/routes/` and fails if any `router.<verb>(..., async ...)` is not wrapped in `asyncRoute`. It is enforceable in CI today, needs no new tooling, doubles as the TDD red test for this change, and is deleted the day the project moves to Express 5. It closes the "half-forgotten once will be forgotten again" gap without expanding the issue. **Rejected — wrap only.** Smallest diff, but leaves enforcement waiting on ESLint (#60). That reproduces exactly the situation this issue is reporting: a convention held by memory. The issue itself argues against this. **Rejected — per-file integration tests forcing a rejection.** Strongest proof of behaviour, but slow to write and it only ever covers the routes someone thought to test. A new unwrapped route added next month passes it silently. The guard test catches that; these would not. **Rejected — upgrade to Express 5.** The correct permanent fix, and it would delete `asyncRoute` outright. Rejected *for this issue* because it is a breaking upgrade — path-to-regexp v8 route syntax, `req.query` becoming a getter, `@types/express` v5 — and pairing a breaking framework upgrade with a reliability fix means a rollback of one is a rollback of both. Worth its own issue; the guard test is written so it disappears cleanly when that lands. Longer term the enforcement belongs in ESLint via `@typescript-eslint/no-misused-promises`, which flags this class of bug directly. That is #60's scope, not this one. ## Question: how does an issue move to In Progress? Asked because the Gitea MCP tooling reaches issues, labels and milestones but not project boards, and this repo has no labels defined. Answer: the board is moved by hand; I do not need to touch it.

bermudalamb commented

2026-08-19 13:13:23 -05:00

Implemented on feature/59-wrap-async-routes.

What changed

31 handlers wrapped, not 30. Per file: customers.ts 11, admin.ts 5, shippingAddresses.ts 5, cart.ts 3, cartCheckout.ts 4, adminSettings.ts 2, public.ts 1. Plus attachCustomer at its mount point in app.ts.

Two the inventory in the issue missed, both found the same way — by matching on any *Router name rather than router, and by checking middleware as well as routes:

webhookRouter.post('/') in cartCheckout.ts:276 — the PayPal webhook registers on a second router, so a grep for router. walks straight past it.
attachCustomer in middleware/customerAuth.ts — mounted globally at app.ts:27, awaiting a session lookup with nothing catching a rejection. This one is the reason the issue's framing understates the problem: a rejection there hangs every request in the application, including the 25 handlers that were already wrapped correctly. The guarantee this issue set out to establish did not hold anywhere until this was fixed.

Enforcement

backend/tests/unit/routesAreWrapped.test.ts scans the route sources and fails on any registration whose handler is not wrapped. Two details are deliberate, each aimed at one of the ways the existing bare handlers escaped notice: it walks parens rather than lines, so a handler whose async sits on its own line is still caught; and it matches any *Router name, which is what would have caught the webhook. It also tests itself against known-good and known-bad snippets, so it cannot pass by silently finding nothing.

Also updated the stale claim in .claude/project-context.md that the older route files were still unwrapped.

Verification

npm run build (tsc) — clean
npm run test:unit — 78 passed, 5 suites
npm run test:integration — 134 passed, 8 suites

Red-then-green on the guard test: before the fix it failed 8 of its cases (7 route files plus attachCustomer) while its own sanity checks passed.

Note for anyone running the integration suite on this Windows box: npm run db:test:up currently fails because port 55432 falls inside a Windows-excluded range (55410–55509, a Hyper-V reservation). Nothing to do with this change. TEST_PGPORT is honoured throughout the test setup, so running the container on another port and exporting TEST_PGPORT works without touching any committed file.

No behaviour change on the success path. On the failure path a hung request becomes a logged 500.

Not pushed — left local per the usual arrangement.

Implemented on `feature/59-wrap-async-routes`. ## What changed 31 handlers wrapped, not 30. Per file: `customers.ts` 11, `admin.ts` 5, `shippingAddresses.ts` 5, `cart.ts` 3, `cartCheckout.ts` 4, `adminSettings.ts` 2, `public.ts` 1. Plus `attachCustomer` at its mount point in `app.ts`. **Two the inventory in the issue missed**, both found the same way — by matching on any `*Router` name rather than `router`, and by checking middleware as well as routes: - `webhookRouter.post('/')` in `cartCheckout.ts:276` — the PayPal webhook registers on a second router, so a grep for `router.` walks straight past it. - `attachCustomer` in `middleware/customerAuth.ts` — mounted globally at `app.ts:27`, awaiting a session lookup with nothing catching a rejection. This one is the reason the issue's framing understates the problem: a rejection there hangs *every* request in the application, including the 25 handlers that were already wrapped correctly. The guarantee this issue set out to establish did not hold anywhere until this was fixed. ## Enforcement `backend/tests/unit/routesAreWrapped.test.ts` scans the route sources and fails on any registration whose handler is not wrapped. Two details are deliberate, each aimed at one of the ways the existing bare handlers escaped notice: it walks parens rather than lines, so a handler whose `async` sits on its own line is still caught; and it matches any `*Router` name, which is what would have caught the webhook. It also tests itself against known-good and known-bad snippets, so it cannot pass by silently finding nothing. Also updated the stale claim in `.claude/project-context.md` that the older route files were still unwrapped. ## Verification - `npm run build` (tsc) — clean - `npm run test:unit` — 78 passed, 5 suites - `npm run test:integration` — 134 passed, 8 suites Red-then-green on the guard test: before the fix it failed 8 of its cases (7 route files plus `attachCustomer`) while its own sanity checks passed. Note for anyone running the integration suite on this Windows box: `npm run db:test:up` currently fails because port 55432 falls inside a Windows-excluded range (55410–55509, a Hyper-V reservation). Nothing to do with this change. `TEST_PGPORT` is honoured throughout the test setup, so running the container on another port and exporting `TEST_PGPORT` works without touching any committed file. No behaviour change on the success path. On the failure path a hung request becomes a logged 500. Not pushed — left local per the usual arrangement.

bermudalamb referenced this issue from a commit

2026-08-19 13:18:14 -05:00

fix(backend): route every async handler through the error middleware (#59)

bermudalamb referenced this issue from a commit

2026-08-19 13:18:14 -05:00

docs: the asyncRoute guarantee now holds everywhere, and is enforced (#59)

bermudalamb closed this issue

2026-08-19 13:19:35 -05:00

bermudalamb moved this to Review in Code Quality and Hardening on 2026-08-19 13:20:08 -05:00

bermudalamb referenced this issue

2026-08-19 13:36:12 -05:00

No ESLint anywhere, so the React and SonarJS rules that would catch these problems never run #60