71 lines
3.2 KiB
Markdown
71 lines
3.2 KiB
Markdown
---
|
|
phase: 09-faster-write-back
|
|
fixed_at: 2026-06-12T21:15:00Z
|
|
review_path: .planning/phases/09-faster-write-back/09-REVIEW.md
|
|
iteration: 2
|
|
findings_in_scope: 2
|
|
fixed: 1
|
|
skipped: 1
|
|
status: 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_
|