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

phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status

phase	reviewed	depth	files_reviewed	files_reviewed_list	findings	status
09-faster-write-back	2026-06-12T21:05:00Z	standard	5	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	critical warning info total 0 0 2 2	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:

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