docs(09): refresh code review report

.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<typeof setInterval> | 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_