From 551b751b81ee4ae3eed4d21770dc72eb8c742258 Mon Sep 17 00:00:00 2001 From: Lucas Berger Date: Fri, 12 Jun 2026 17:04:55 -0400 Subject: [PATCH] docs(09): add code review report --- .../phases/09-faster-write-back/09-REVIEW.md | 186 ++++++++++++++++++ 1 file changed, 186 insertions(+) create mode 100644 .planning/phases/09-faster-write-back/09-REVIEW.md diff --git a/.planning/phases/09-faster-write-back/09-REVIEW.md b/.planning/phases/09-faster-write-back/09-REVIEW.md new file mode 100644 index 0000000..fb9e122 --- /dev/null +++ b/.planning/phases/09-faster-write-back/09-REVIEW.md @@ -0,0 +1,186 @@ +--- +phase: 09-faster-write-back +reviewed: 2026-06-12T17:05:00Z +depth: standard +files_reviewed: 5 +files_reviewed_list: + - apps/api/src/lib/outboxTrigger.ts + - apps/api/src/broker/outboxWorker.ts + - apps/api/src/routes/events.ts + - apps/api/src/index.ts + - apps/api/tests/broker/outboxWorker.test.ts +findings: + critical: 0 + warning: 4 + info: 3 + total: 7 +status: issues_found +--- + +# Phase 9: Code Review Report + +**Reviewed:** 2026-06-12T17:05:00Z +**Depth:** standard +**Files Reviewed:** 5 +**Status:** issues_found + +## Summary + +Reviewed the event-driven outbox drain trigger added in phase 09 (CAL-15): a zero-dependency +in-process `EventEmitter` (`outboxTrigger.ts`), the `scheduleOutboxDrain` wrapper with the +`isDraining` guard + `drainRequested` trailing-re-drain loop in `outboxWorker.ts`, four +fire-and-forget `signalOutboxDrain()` publish sites in `events.ts`, and `initOutboxTrigger()` +startup wiring under `isMainModule()` in `index.ts`. + +The core concurrency design is sound on the four points called out in the brief: + +- **Trailing-re-drain is bounded.** `drainRequested` is reset to `false` BEFORE the recursive + `scheduleOutboxDrain()` call (outboxWorker.ts:199-200), so a signal arriving during the + trailing drain produces at most one further pass — no unbounded re-drain chain. A signal that + arrives mid-drain sets `drainRequested=true` and is re-SELECTed by the trailing pass; the + publish sites fire `signalOutboxDrain()` only AFTER the row's DB insert commits, so the row is + always visible to the re-drain. No lost-wakeup. +- **Signal never fires inside the `db.transaction` callback.** All four publish sites (events.ts + 310, 434, 450, 525) call `signalOutboxDrain()` after the commit resolves and outside the + transaction closure; D-03 ordering holds. +- **Fire-and-forget is rejection-safe.** `signalOutboxDrain` is synchronous (`emitter.emit`); the + sole listener delegates to `scheduleOutboxDrain`, which is `void`-returning with a `.catch()` + attached to the `runOutboxDrain()` promise. `runOutboxDrain` is `async`, so it can never throw + synchronously into `emit`/the route handler — no floating promise, no unhandled rejection. +- **Tests pass (30/30) and the trigger-wiring suite exercises SC-1, D-05 collapse, and the + concurrency guard.** + +Remaining findings are robustness/maintainability (WARNING) and clarity (INFO). No BLOCKERs. + +## Warnings + +### WR-01: `initOutboxTrigger()` is non-idempotent and leaks its listener + +**File:** `apps/api/src/broker/outboxWorker.ts:844-846` +**Issue:** `initOutboxTrigger` calls `onOutboxDrain(...)` and discards the returned unsubscribe +function, and has no guard against repeated registration. Each invocation adds another `'drain'` +listener to the module-singleton emitter. Production calls it once under `isMainModule()`, so the +hot path is fine — but the function is exported, has no internal idempotency, and the default +`EventEmitter` max-listeners is 10 (the module comment in `outboxTrigger.ts:11` explicitly relies +on never calling `setMaxListeners`). A second `initOutboxTrigger()` call (a future second wiring +site, a hot-reload, or a test that re-inits) silently double-drains on every signal and, after 10 +registrations, emits a `MaxListenersExceededWarning`. The leak is masked today only because there +is exactly one caller. +**Fix:** Make registration idempotent and retain the unsubscribe handle: +```ts +let unsubscribeDrain: (() => void) | null = null; +export function initOutboxTrigger(): void { + if (unsubscribeDrain) return; // already wired — no double-registration + unsubscribeDrain = onOutboxDrain(() => scheduleOutboxDrain()); +} +``` + +### WR-02: Trigger-wiring tests register a listener in `beforeAll` and never remove it + +**File:** `apps/api/tests/broker/outboxWorker.test.ts:751-753` +**Issue:** The `scheduleOutboxDrain — trigger wiring` block calls `initOutboxTrigger()` in +`beforeAll`, registering a permanent `'drain'` listener on the module-singleton emitter that is +never torn down (no `afterAll`/`afterEach` unsubscribe). Because the emitter is a module +singleton shared across the whole file (and any future file importing it), any later +`signalOutboxDrain()` — in a subsequent describe block, a newly added test, or another test file +that imports `signalOutboxDrain` — will fire the still-registered `scheduleOutboxDrain` listener +and kick off a real `runOutboxDrain()` against the shared `mockPendingRows`/db mock. That is +exactly the cross-test state pollution the WR-04/T-09-02 comments warn about. It is latent now +only because the polluting describe block happens to run last and no other test calls +`signalOutboxDrain`; test execution order is not a contract. +**Fix:** Capture and release the listener (requires WR-01's handle, or expose a teardown). Minimal +test-only fix: +```ts +describe('scheduleOutboxDrain — trigger wiring (D-09)', () => { + let unsub: () => void; + beforeAll(() => { + // import onOutboxDrain directly so the test owns the unsubscribe + unsub = onOutboxDrain(() => scheduleOutboxDrain()); + }); + afterAll(() => unsub()); + // ... +}); +``` +or, once WR-01 lands, add an `afterAll` that calls a new `shutdownOutboxTrigger()` exported for +tests. + +### WR-03: `setInterval` handle is never retained — interval cannot be cleared and keeps the loop alive + +**File:** `apps/api/src/broker/outboxWorker.ts:856-860` +**Issue:** `startOutboxWorker` calls `setInterval(...)` but discards the returned `Timeout`. There +is no way to stop the drain schedule (graceful shutdown, SIGTERM handler, or test teardown), and +the unref-less interval keeps the event loop alive. This pre-dates phase 09 but the phase rewired +the interval body (`runOutboxDrain` → `scheduleOutboxDrain`), so it is in scope. On a clean +shutdown the process cannot drain-and-exit; a drain may be mid-flight when the process is killed. +**Fix:** Retain the handle and expose a stop hook (and/or `.unref()` if the interval should not by +itself hold the process open): +```ts +let drainInterval: ReturnType | null = null; +export function startOutboxWorker(): void { + drainInterval = setInterval(() => scheduleOutboxDrain(), 15 * 1000); +} +export function stopOutboxWorker(): void { + if (drainInterval) { clearInterval(drainInterval); drainInterval = null; } +} +``` + +### WR-04: Module-level `isDraining`/`drainRequested` shared by event-path and interval-path defeats unit isolation + +**File:** `apps/api/src/broker/outboxWorker.ts:162, 172` +**Issue:** Both guard flags are module-level mutable singletons. They are correct for the +single-process runtime, but they persist across vitest test cases within the file and are not +reset by `vi.resetAllMocks()`. If a test leaves `isDraining=true` or `drainRequested=true` — e.g. +a `runOutboxDrain()` that throws synchronously before the `try` (none today) or a future test that +abandons an in-flight `scheduleOutboxDrain` without flushing its `.finally()` — the next test +silently no-ops its drain (`if (isDraining) return`) and the failure surfaces as a confusing +"createCalendarEvent not called" assertion far from the cause. The CR-05/D-07 concurrency tests +already depend on these flags being clean at entry but nothing guarantees it. +**Fix:** Export a test-only reset (or reset in `beforeEach`): +```ts +export function __resetDrainStateForTests(): void { + isDraining = false; + drainRequested = false; +} +``` +and call it in each `beforeEach` alongside `wireMockChain()`. Alternatively, document that every +`scheduleOutboxDrain()` started in a test MUST be awaited to completion before the test returns. + +## Info + +### IN-01: `onOutboxDrain` returns an unsubscribe that no production caller uses + +**File:** `apps/api/src/lib/outboxTrigger.ts:36-39` +**Issue:** `onOutboxDrain` faithfully returns an `off()` closure (good API design, mirrors +`listEmitter.ts`), but the only production caller (`initOutboxTrigger`) throws it away (see WR-01). +The capability exists but is unreachable, so the "you can stop delivery" contract in the doc +comment is currently aspirational. +**Fix:** Once WR-01 retains the handle, this resolves itself. No standalone change needed. + +### IN-02: Doc comment references "D-06" for the trailing-re-drain flag; design notes attribute it to D-05 + +**File:** `apps/api/src/broker/outboxWorker.ts:165, 168` +**Issue:** The `drainRequested` block header reads `D-05 / D-06` and the body says "never more than +one extra pass (D-06)", while `scheduleOutboxDrain`'s own doc (line 178-179) and the file header +(line 21) attribute the trailing-re-drain to D-05. Minor decision-ID drift; harmless but invites +confusion when cross-referencing the phase decision log. +**Fix:** Normalize to the authoritative decision ID for the trailing-re-drain (D-05) and drop the +stray D-06 unless a D-06 clause genuinely governs the "one extra pass" bound. + +### IN-03: `signalOutboxDrain()` runs `runOutboxDrain` synchronously up to the first `await` on the request thread + +**File:** `apps/api/src/routes/events.ts:310` (and 434, 450, 525) +**Issue:** `emit('drain')` invokes the listener synchronously, so `scheduleOutboxDrain` → +`runOutboxDrain` executes inline on the request path until `runOutboxDrain` hits its first `await` +(the pending-rows SELECT at outboxWorker.ts:653). The synchronous prefix is tiny (a flag check +plus issuing the query), so the 202 is not meaningfully delayed today — but the cost is paid on +the HTTP request thread rather than deferred. If `runOutboxDrain`'s pre-await section ever grows +(more bookkeeping before the first `await`), it silently becomes request-path latency. +**Fix:** If you want a hard guarantee the request returns before any drain work, defer the kick: +`onOutboxDrain(() => queueMicrotask(() => scheduleOutboxDrain()))` or `setImmediate(...)`. Optional; +acceptable as-is given the trivial synchronous prefix. + +--- + +_Reviewed: 2026-06-12T17:05:00Z_ +_Reviewer: Claude (gsd-code-reviewer)_ +_Depth: standard_