familysync/.planning/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-12T17: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 3 4 7	issues_found

Phase 09: Code Review Report

Reviewed: 2026-06-12 Depth: standard Files Reviewed: 5 Status: issues_found

Summary

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:

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

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

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:

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:

let unsubscribeDrain: (() => void) | null = null;

export function initOutboxTrigger(): void {
  if (unsubscribeDrain) return; // idempotent — never double-register
  unsubscribeDrain = onOutboxDrain(() => scheduleOutboxDrain());
}

export function stopOutboxTrigger(): void {
  unsubscribeDrain?.();
  unsubscribeDrain = null;
}

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:

afterAll(() => {
  stopOutboxTrigger();          // remove the leaked 'drain' listener
});

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: signalOutboxDrain() correctness depends on lexical ordering after the enqueue commit

File: apps/api/src/routes/events.ts:301-311, 439-450, 515-525

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.

Fix: Add // must stay AFTER the enqueue commit — worker selects committed pending rows only above each signalOutboxDrain() call.

IN-02: duplicated RRULE-assembly decision tree across the update and create branches

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:

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:

import {
  runOutboxDrain,
  assembleRruleString,
  scheduleOutboxDrain,
  initOutboxTrigger,
} from '../../src/broker/outboxWorker.js';

---

Reviewed: 2026-06-12 Reviewer: Claude (gsd-code-reviewer) Depth: standard