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

phase, fixed_at, review_path, iteration, findings_in_scope, fixed, skipped, status

phase	fixed_at	review_path	iteration	findings_in_scope	fixed	skipped	status
09-faster-write-back	2026-06-12T21:15:00Z	.planning/phases/09-faster-write-back/09-REVIEW.md	2	2	1	1	partial

Phase 09: Code Review Fix Report

Fixed at: 2026-06-12T21:15:00Z Source review: .planning/phases/09-faster-write-back/09-REVIEW.md Iteration: 2

Summary:

• Findings in scope: 2 (fix_scope: all — includes Info)
• Fixed: 1
• Skipped: 1 (accepted with rationale)

Fixed Issues

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

Files modified: apps/api/src/broker/outboxWorker.ts Commit: b7767af Applied fix: Added if (process.env.NODE_ENV === 'production') return; as the first line of __resetDrainState() so an accidental production call is a no-op. The single-process concurrency contract (isDraining / drainRequested) is now enforced by code rather than only by the "Not for production use" JSDoc comment. A production caller can no longer clear isDraining mid-drain and trigger the CR-05 double-dispatch hazard.

Verification:

• Tier 1: re-read the edited function — guard present, body intact.
• Tier 2: cd apps/api && npx tsc --noEmit → exit 0 (clean).
• Tier 2: cd apps/api && npx vitest run tests/broker/outboxWorker.test.ts tests/routes/events.test.ts → 2 files, 53 tests passed.

Skipped Issues

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 (with the isDraining=false reset at line 838) Status: accepted (skipped — not fixed) Reason: Accepted with rationale per fix decision. The reviewer's suggested fix folds the trailing-re-drain decision into runOutboxDrain's own finally, which would modify runOutboxDrain's body / finally. That directly violates plan 09-01's verified must-have ("runOutboxDrain's body, its if (isDraining) return; guard, and its finally { isDraining = false; } are byte-for-byte unchanged") and would risk the 15 pre-existing broker tests plus the verified D-07 / SC-4 exactly-once behavior (phase passed 5/5). The reviewer itself rates IN-02 low-priority and explicitly states it is NOT a correctness bug: the re-entrant isDraining guard keeps every concrete runOutboxDrain body strictly serialized, so no outbox row is ever double-dispatched. The worst case is one extra idempotent Fastmail-facing drain pass under rapid edits, which finds the row already gone. runOutboxDrain was deliberately NOT modified.

Original issue: A signalOutboxDrain() whose deferred emit lands in the gap between runOutboxDrain's finally clearing isDraining (line 838) and scheduleOutboxDrain's later-microtask .finally() (line 246) finds isDraining === false and starts a brand-new runOutboxDrain() instead of collapsing into a single trailing re-drain via drainRequested. The net effect can be two near-back-to-back drains instead of one collapsed trailing drain — a weakening of the "collapse any number of mid-drain signals into exactly one trailing drain" guarantee at this boundary, but not a correctness defect.

---

Fixed: 2026-06-12T21:15:00Z Fixer: Claude (gsd-code-fixer) Iteration: 2