From 777910c86a0131ed7fd27caf84596a10db3a8f3a Mon Sep 17 00:00:00 2001 From: Lucas Berger Date: Fri, 12 Jun 2026 21:01:55 -0400 Subject: [PATCH] docs(09): add code review fix report + refresh review after fixes --- .../09-faster-write-back/09-REVIEW-FIX.md | 70 ++++ .../phases/09-faster-write-back/09-REVIEW.md | 303 ++++++------------ 2 files changed, 168 insertions(+), 205 deletions(-) create mode 100644 .planning/phases/09-faster-write-back/09-REVIEW-FIX.md diff --git a/.planning/phases/09-faster-write-back/09-REVIEW-FIX.md b/.planning/phases/09-faster-write-back/09-REVIEW-FIX.md new file mode 100644 index 0000000..e41231a --- /dev/null +++ b/.planning/phases/09-faster-write-back/09-REVIEW-FIX.md @@ -0,0 +1,70 @@ +--- +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_ diff --git a/.planning/phases/09-faster-write-back/09-REVIEW.md b/.planning/phases/09-faster-write-back/09-REVIEW.md index e360086..83fe2c3 100644 --- a/.planning/phases/09-faster-write-back/09-REVIEW.md +++ b/.planning/phases/09-faster-write-back/09-REVIEW.md @@ -1,6 +1,6 @@ --- phase: 09-faster-write-back -reviewed: 2026-06-12T17:05:00Z +reviewed: 2026-06-12T21:05:00Z depth: standard files_reviewed: 5 files_reviewed_list: @@ -11,9 +11,9 @@ files_reviewed_list: - apps/api/tests/broker/outboxWorker.test.ts findings: critical: 0 - warning: 3 - info: 4 - total: 7 + warning: 0 + info: 2 + total: 2 status: issues_found --- @@ -21,229 +21,122 @@ status: issues_found **Reviewed:** 2026-06-12 **Depth:** standard -**Files Reviewed:** 5 -**Status:** issues_found +**Status:** issues_found (2 Info; no Blockers, no Warnings) ## 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: +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). -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). +**Fix verification — all four focus areas confirmed correct:** -No blockers. The core concurrency logic is sound. Findings are robustness/maintainability -issues concentrated in the trigger wiring and its test harness. +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.** -## Warnings +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.** -### WR-01: `signalOutboxDrain()` runs the drain-scheduling listener synchronously — "fire-and-forget" is misleading +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.** -**File:** `apps/api/src/lib/outboxTrigger.ts:28-30`, `apps/api/src/broker/outboxWorker.ts:845` +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`. -**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. +**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. -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. +## Narrative Findings (AI reviewer) -**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: +### 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: ```ts -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')); +export function __resetDrainState(): void { + if (process.env.NODE_ENV === 'production') return; // test-only; never clear the guard in prod + isDraining = false; + drainRequested = false; } ``` -(Alternatively wrap the listener registration in `initOutboxTrigger` so its body cannot throw -synchronously. The microtask approach is the smaller, more honest change.) +(Alternatively, move the reset behind a test-only entry point not reachable from the +production import graph. The env guard is the smaller change.) -### WR-02: `initOutboxTrigger()` is not idempotent and silently discards the unsubscribe handle +### 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:844-846`, `apps/api/src/lib/outboxTrigger.ts:36-39` +**File:** `apps/api/src/broker/outboxWorker.ts:237-253` (in concert with the +`isDraining=false` reset at line 838) -**Issue:** `onOutboxDrain` returns an unsubscribe function, but `initOutboxTrigger` throws it -away: +**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. -```ts -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: - -```ts -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: - -```ts -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: - -```ts -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: - -```ts -import { - runOutboxDrain, - assembleRruleString, - scheduleOutboxDrain, - initOutboxTrigger, -} from '../../src/broker/outboxWorker.js'; -``` +**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-12_ +_Reviewed: 2026-06-12T21:05:00Z_ _Reviewer: Claude (gsd-code-reviewer)_ _Depth: standard_