3.2 KiB
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