---
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<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.

## 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_