diff --git a/.planning/phases/09-faster-write-back/09-REVIEW.md b/.planning/phases/09-faster-write-back/09-REVIEW.md index fb9e122..e360086 100644 --- a/.planning/phases/09-faster-write-back/09-REVIEW.md +++ b/.planning/phases/09-faster-write-back/09-REVIEW.md @@ -11,176 +11,239 @@ files_reviewed_list: - apps/api/tests/broker/outboxWorker.test.ts findings: critical: 0 - warning: 4 - info: 3 + warning: 3 + info: 4 total: 7 status: issues_found --- -# Phase 9: Code Review Report +# Phase 09: Code Review Report -**Reviewed:** 2026-06-12T17:05:00Z +**Reviewed:** 2026-06-12 **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`. +Phase 09 (CAL-15) wires an in-process EventEmitter (`outboxTrigger.ts`) so that a +successful outbox enqueue immediately schedules a drain, eliminating the up-to-15s +polling delay. The five focus areas were traced end-to-end: -The core concurrency design is sound on the four points called out in the brief: +1. **Trailing-re-drain boundedness** — `drainRequested` is reset (line 199) BEFORE the + recursive `scheduleOutboxDrain()` call (line 200), inside `.finally()` which runs + AFTER `runOutboxDrain`'s own `finally` has already reset `isDraining=false` (line 828). + The loop is correctly bounded: each completed drain triggers at most one trailing pass, + and a fresh signal during that trailing pass collapses again. No unbounded chain. **Correct.** +2. **Signal outside the edit-as-move transaction** — `signalOutboxDrain()` (events.ts:434) + fires AFTER `db.transaction()` (410-432) resolves, never inside the callback. The + create-before-delete ordering (D-04) is preserved. **Correct.** +3. **Fire-and-forget correctness** — `runOutboxDrain` is `async`, so it always returns a + promise and the `.catch()` (193) captures rejections; no unhandled rejection escapes the + route handler. **Correct, with one caveat** (see WR-01: the listener body runs + *synchronously* inside `emitter.emit`, so the "never blocks the caller" contract is + thinner than the doc claims). +4. **Listener idempotency** — `initOutboxTrigger()` is NOT idempotent and discards its + unsubscribe handle (see WR-02). Benign in production (one call under `isMainModule()`), + but a latent footgun and a source of cross-describe leakage in the test process (WR-03). +5. **Module-level mutable state across tests** — `isDraining`/`drainRequested` live at module + scope and are NOT reset by `vi.resetAllMocks()`. The suite happens to leave them clean, + but the isolation is incidental, not guaranteed (see WR-03 / IN-03). -- **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. +No blockers. The core concurrency logic is sound. Findings are robustness/maintainability +issues concentrated in the trigger wiring and its test harness. ## Warnings -### WR-01: `initOutboxTrigger()` is non-idempotent and leaks its listener +### WR-01: `signalOutboxDrain()` runs the drain-scheduling listener synchronously — "fire-and-forget" is misleading + +**File:** `apps/api/src/lib/outboxTrigger.ts:28-30`, `apps/api/src/broker/outboxWorker.ts:845` + +**Issue:** `signalOutboxDrain()` calls `emitter.emit('drain')`, and Node's `EventEmitter.emit` +invokes registered listeners **synchronously, in the calling stack**. The registered listener +is `() => scheduleOutboxDrain()` (outboxWorker.ts:845). `scheduleOutboxDrain` executes its +`isDraining` check and the *synchronous prefix* of `runOutboxDrain()` (everything up to the +first `await` at line 653) on the route handler's stack before control returns to the caller. + +Two consequences: +- The doc comment "Synchronous and fire-and-forget — never awaited (D-04)" overstates the + decoupling. The call does not block on network I/O (the first `await` yields), but it is not + free either — and any *synchronous* throw inside the listener chain would propagate straight + back into the route handler's `try/catch` (e.g. events.ts:312), surfacing a misleading + 503 to a user whose enqueue actually committed successfully. +- Today nothing in the synchronous prefix can throw (the `isDraining` guard short-circuits and + `runOutboxDrain` is `async` so its body is deferred), so this is latent, not active. But it is + one refactor away from leaking: if anyone adds synchronous work to `scheduleOutboxDrain` before + the `runOutboxDrain()` call, a throw there corrupts the route response. + +**Fix:** Make the emit truly decoupled so a misbehaving listener can never reach the enqueue +caller, and align the code with the documented "fire-and-forget" contract: + +```ts +export function signalOutboxDrain(): void { + // Defer to a microtask so listener execution never runs on the enqueue caller's + // stack — a throwing listener cannot corrupt the route handler's response. + queueMicrotask(() => emitter.emit('drain')); +} +``` + +(Alternatively wrap the listener registration in `initOutboxTrigger` so its body cannot throw +synchronously. The microtask approach is the smaller, more honest change.) + +### WR-02: `initOutboxTrigger()` is not idempotent and silently discards the unsubscribe handle + +**File:** `apps/api/src/broker/outboxWorker.ts:844-846`, `apps/api/src/lib/outboxTrigger.ts:36-39` + +**Issue:** `onOutboxDrain` returns an unsubscribe function, but `initOutboxTrigger` throws it +away: + +```ts +export function initOutboxTrigger(): void { + onOutboxDrain(() => scheduleOutboxDrain()); // return value discarded +} +``` + +Every call to `initOutboxTrigger()` adds *another* `'drain'` listener. There is no guard against +double-registration and no way to ever unsubscribe. A single drain signal would then fan out to +N listeners, each calling `scheduleOutboxDrain()` — though the `isDraining`/`drainRequested` +guard collapses the duplicates into one drain, so the *behavioral* damage is masked. The real +costs are: (a) a `MaxListenersExceededWarning` after 10 registrations (the module doc explicitly +relies on the default-10 limit, outboxTrigger.ts:11-13), and (b) an unobservable listener leak. +In production this is safe only because `index.ts:139` calls it exactly once under +`isMainModule()` — the safety is positional, not enforced. + +**Fix:** Make registration idempotent and retain the handle for teardown: -**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 + if (unsubscribeDrain) return; // idempotent — never double-register unsubscribeDrain = onOutboxDrain(() => scheduleOutboxDrain()); } + +export function stopOutboxTrigger(): void { + unsubscribeDrain?.(); + unsubscribeDrain = null; +} ``` -### WR-02: Trigger-wiring tests register a listener in `beforeAll` and never remove it +`stopOutboxTrigger` also gives the test suite a clean teardown (see WR-03). + +### WR-03: test suite leaks the drain listener and never resets module-level drain state + +**File:** `apps/api/tests/broker/outboxWorker.test.ts:751-753` (and file-wide) + +**Issue:** Two coupled test-hygiene defects: + +1. **Leaked listener.** The `scheduleOutboxDrain — trigger wiring` block registers the drain + listener in `beforeAll` (line 752) but has no `afterAll` to remove it. Because + `vitest.config.ts` sets `fileParallelism: false` and a single module instance is shared + across all describe blocks in the file, that listener stays live for every *subsequent* + describe block too. Any stray `signalOutboxDrain()` (or a future test that adds one) would + now trigger a real `scheduleOutboxDrain()` against the mocked DB in an unrelated test, + producing order-dependent flakiness. It also leaves an open emitter reference after the file + finishes. + +2. **Unreset module state.** `isDraining` and `drainRequested` are module-level + (outboxWorker.ts:162, 172). `vi.resetAllMocks()` in the `beforeEach` hooks does NOT touch + them. The suite currently stays green only because every test awaits its drain to completion, + so `runOutboxDrain`'s `finally` (line 828) resets `isDraining` each time. But the D-05 test + (line 782) deliberately holds a drain in flight via `resolveFirst`; if its final flush were + ever insufficient (timing-sensitive), it could leave `isDraining=true` and turn the *next* + test's `runOutboxDrain` into a silent no-op — a false pass. The isolation is incidental. + +**Fix:** Add explicit teardown and state reset. With WR-02's `stopOutboxTrigger` exported: -**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()); - // ... +afterAll(() => { + stopOutboxTrigger(); // remove the leaked 'drain' listener }); ``` -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. +For the module state, the cleanest fix is to expose a test-only reset (e.g. an +`__resetDrainStateForTest()` exported under a test guard) or assert `isDraining` quiescence in a +shared `afterEach`. At minimum, document that every test in the file MUST await its drain to +completion so the `finally` reset runs. ## Info -### IN-01: `onOutboxDrain` returns an unsubscribe that no production caller uses +### IN-01: `signalOutboxDrain()` correctness depends on lexical ordering after the enqueue commit -**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. +**File:** `apps/api/src/routes/events.ts:301-311, 439-450, 515-525` -### IN-02: Doc comment references "D-06" for the trailing-re-drain flag; design notes attribute it to D-05 +**Issue:** In the create path, `signalOutboxDrain()` (310) runs only after `await db.insert(...)` +(301) resolves, so the row is committed before the drain is signaled — correct. The same holds +for the update (439-450) and delete (515-525) paths, and for the move path the signal is outside +the transaction (434). This is all correct; the note is only that the correctness depends entirely +on each `signalOutboxDrain()` staying lexically *after* its `await db.insert`/`transaction`. A +future reorder (e.g. moving the signal up for "responsiveness") would race the worker against an +uncommitted row, and the worker's `SELECT ... WHERE status='pending'` would simply miss it (a +silent up-to-15s delay, not a crash). Worth a one-line comment at each site cementing the ordering. -**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. +**Fix:** Add `// must stay AFTER the enqueue commit — worker selects committed pending rows only` +above each `signalOutboxDrain()` call. -### IN-03: `signalOutboxDrain()` runs `runOutboxDrain` synchronously up to the first `await` on the request thread +### IN-02: duplicated RRULE-assembly decision tree across the `update` and `create` branches -**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. +**File:** `apps/api/src/broker/outboxWorker.ts:425-451` and `519-549` + +**Issue:** The "explicit recurrence wins / else preserved RRULE with bound-strip / else +rruleFromPayload" decision tree is copy-pasted near-verbatim between the update branch (425-451) +and the create branch (523-549). The only difference is the *source* of `preservedRrule` +(rawVevent re-read vs `_preservedRrule` payload field). Two copies of subtle RFC-5545 bound-strip +logic (`replace(/;(UNTIL|COUNT)=[^;]*/g, '')`) will drift; a fix to one (e.g. the documented +Pitfall-3 double-UNTIL guard) can silently miss the other. + +**Fix:** Extract a helper: + +```ts +function resolveFinalRrule( + fields: OutboxPayloadFields, + preservedRrule: string | undefined, +): string | undefined { /* the shared decision tree */ } +``` + +Call it from both branches with the branch-specific `preservedRrule`. + +### IN-03: module-level `isDraining`/`drainRequested` have no test-only reset hook + +**File:** `apps/api/src/broker/outboxWorker.ts:162, 172` + +**Issue:** These flags are the entire concurrency contract for the single-process deployment, yet +there is no supported way to reset them between tests (see WR-03). The `CR-05` concurrency test +(line 529) and the `D-07` concurrent test (line 830) both depend on a clean `isDraining=false` +starting state but rely on the previous test having drained cleanly to provide it. + +**Fix:** Export a guarded reset (used only by tests), e.g. +`export function __resetDrainState(): void { isDraining = false; drainRequested = false; }`, +and call it in the relevant `beforeEach`. Keeps the contract testable without exposing the flags +as mutable module exports. + +### IN-04: stale RED-scaffold doc comments and `@ts-ignore` suppress real type checking on the import + +**File:** `apps/api/tests/broker/outboxWorker.test.ts:18-21, 670-672, 703-705, 745` + +**Issue:** The file header and several describe blocks still claim the tests "FAIL (RED) because +broker/outboxWorker.ts does not exist yet" and reference `@ts-ignore intentional RED import` +(line 20). The module exists and is exported; the `@ts-ignore` now suppresses real type checking +on the import line, so a future signature drift (e.g. renaming `scheduleOutboxDrain`) would not be +caught by `tsc` on that import. Given the project MEMORY note that vitest passes while `tsc` fails +(esbuild strips types), this suppression is exactly the kind of gap that hides type regressions. + +**Fix:** Remove the `@ts-ignore` (and the RED-scaffold comments) so the import is type-checked: + +```ts +import { + runOutboxDrain, + assembleRruleString, + scheduleOutboxDrain, + initOutboxTrigger, +} from '../../src/broker/outboxWorker.js'; +``` --- -_Reviewed: 2026-06-12T17:05:00Z_ +_Reviewed: 2026-06-12_ _Reviewer: Claude (gsd-code-reviewer)_ _Depth: standard_