familysync/.planning/milestones/v1.1-phases/09-faster-write-back/09-REVIEW.md

---
phase: 09-faster-write-back
reviewed: 2026-06-12T21: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: 0
  info: 2
  total: 2
status: issues_found
---

# Phase 09: Code Review Report

**Reviewed:** 2026-06-12
**Depth:** standard
**Status:** issues_found (2 Info; no Blockers, no Warnings)

## Summary

Iteration-2 re-review of the phase-09 write-back trigger wiring after a fixer pass
addressed all seven iteration-1 findings (WR-01..03, IN-01..04). I re-traced the four
high-risk areas the prompt called out and verified each fix is correct AND complete,
and scanned for new defects the fixes could have introduced. Tooling: `vitest run`
(30/30 pass) and `tsc --noEmit` (exit 0, both clean).

**Fix verification — all four focus areas confirmed correct:**

1. **`queueMicrotask` deferral (WR-01).** `signalOutboxDrain()` now defers the emit:
   `queueMicrotask(() => emitter.emit('drain'))` (outboxTrigger.ts:37). This preserves
   fire-and-forget (the caller never blocks) and moves listener execution off the route
   handler's stack, so a synchronous throw in the listener chain can no longer corrupt the
   enqueue response. Trailing-re-drain semantics are intact: in the D-05 test two mid-drain
   signals each queue a microtask; both microtasks run (and collapse to one
   `drainRequested=true`) before the test's `setImmediate`-based flushes, yielding exactly
   2 dispatches — the test still passes. The stale "Synchronous" JSDoc claim flagged in
   iteration 1 is gone; the new comment block (lines 24-35) accurately describes the
   deferral. **Correct and complete.**

2. **`initOutboxTrigger()` idempotency (WR-02).** Now guarded by a retained module-level
   `unsubscribeDrain` handle (outboxWorker.ts:858-863): a second call short-circuits
   (`if (unsubscribeDrain) return`), so no double-`'drain'`-listener and no
   `MaxListenersExceededWarning`. `stopOutboxTrigger()` (871-874) calls the retained
   unsubscribe and nulls the handle, allowing a clean re-`init`. The unsubscribe closure
   passed to `emitter.off('drain', handler)` is the exact handler registered by
   `emitter.on('drain', handler)` (outboxTrigger.ts:45-46), so removal-by-reference is
   correct. A `stop → init` cycle registers a fresh closure and captures a fresh handle —
   no stale-listener leak. **Genuinely safe.**

3. **`resolveFinalRrule()` extraction (IN-02) — byte-identical output.** Highest regression
   risk (two previously independent write paths merged). I diffed the extraction commit
   (d3163f2) line-by-line: the original `update` and `create` decision trees were **already
   character-identical**, and both now call `resolveFinalRrule(fields, hasExplicitRecurrence,
   rruleFromPayload, preservedRrule)` with identical argument order (outboxWorker.ts:476-481
   and 554-559). The only per-branch difference — the *source* of `preservedRrule` (update
   re-reads `rawVevent`; create reads `_preservedRrule`) — is computed before the call and
   passed in, exactly as before. The bound-strip regex `/;(UNTIL|COUNT)=[^;]*/g`, the
   COUNT-over-UNTIL precedence, and the `recurrence:'none' → undefined` clearing are all
   preserved verbatim. The CR-01 create-branch tests (`_preservedRrule` → RRULE present;
   `recurrence:'none'` → no RRULE) and the D-07 FREQ-persistence lock all pass. **No
   behavioral change; byte-identical for both branches.**

4. **`__resetDrainState()` test-only hook (IN-03).** Exported from the production module but
   confirmed to have NO production caller (`grep` shows only the declaration and test
   imports). It resets `isDraining`/`drainRequested` to false, which is correct for the
   `beforeEach` quiescent-state reset and harmless in the test process. The only residual
   concern is the unguarded production export (see IN-01 below). The stale RED `@ts-ignore`
   removal (IN-04) and the `afterAll(stopOutboxTrigger)` teardown (WR-03) are both present
   and the suite is green, confirming no leaked listener bleeds across describe blocks under
   `fileParallelism:false`.

**No new BLOCKER or WARNING defects were introduced by the fixes.** The two findings below
are Info-level robustness notes; both are pre-existing or latent, neither blocks shipping.

## Narrative Findings (AI reviewer)

### IN-01: `__resetDrainState()` is a production export with no environment guard

**File:** `apps/api/src/broker/outboxWorker.ts:883-886`

**Issue:** `__resetDrainState()` is exported from a production module and unconditionally
clears the entire single-process concurrency contract (`isDraining = false;
drainRequested = false;`). Its only protection against misuse is a JSDoc line ("Not for
production use"). If any future code path imports and calls it while a drain is in flight,
it would clear `isDraining` mid-cycle and allow `scheduleOutboxDrain()`/`runOutboxDrain()`
to start a concurrent drain that double-dispatches the same still-`pending` outbox row —
exactly the CR-05 hazard the guard exists to prevent. Today there is no such caller (grep
confirms test-only usage), so this is latent, not active.

**Fix:** Gate the body on a non-production check so an accidental production call is a
no-op, keeping the contract enforced by code rather than by comment:

```ts
export function __resetDrainState(): void {
  if (process.env.NODE_ENV === 'production') return; // test-only; never clear the guard in prod
  isDraining = false;
  drainRequested = false;
}
```

(Alternatively, move the reset behind a test-only entry point not reachable from the
production import graph. The env guard is the smaller change.)

### IN-02: Signal arriving in the `isDraining=false` → `.finally` window starts a fresh drain rather than collapsing via `drainRequested`

**File:** `apps/api/src/broker/outboxWorker.ts:237-253` (in concert with the
`isDraining=false` reset at line 838)

**Issue:** `runOutboxDrain` clears `isDraining` in its own `finally` (line 838), and the
`.finally()` callback in `scheduleOutboxDrain` (line 246) runs in a *later* microtask. A
`signalOutboxDrain()` whose deferred emit lands in that gap finds `isDraining === false`
and starts a brand-new `runOutboxDrain()` directly instead of being collapsed into the
single trailing re-drain via `drainRequested`. The just-completed drain's `.finally` may
then *also* observe `drainRequested` (if a still-earlier signal set it) and kick a second
pass. The net effect can be two near-back-to-back drains instead of one collapsed trailing
drain. This is NOT a correctness bug — the re-entrant `isDraining` guard (line 658) keeps
every concrete `runOutboxDrain` body strictly serialized, so no row is double-dispatched —
but it weakens the documented "collapse any number of mid-drain signals into exactly one
trailing drain" guarantee (lines 216-219) at this specific boundary, and can produce one
extra Fastmail-facing drain pass under rapid edits. It is pre-existing (the `queueMicrotask`
deferral did not create it; it only shifts the emit by one microtask) and was not flagged
in iteration 1.

**Fix:** Reset `isDraining` and check/consume `drainRequested` in the *same* synchronous
step so the gap cannot be observed — e.g. fold the trailing-re-drain decision into
`runOutboxDrain`'s own `finally` (snapshot `const shouldRedrain = drainRequested`, reset
both flags atomically before releasing `isDraining`), rather than splitting the reset
(`runOutboxDrain.finally`) from the re-drain trigger (`scheduleOutboxDrain.finally`). Low
priority given the guard makes the worst case one redundant — not unsafe — drain pass.

---

_Reviewed: 2026-06-12T21:05:00Z_
_Reviewer: Claude (gsd-code-reviewer)_
_Depth: standard_