docs(09): add code review fix report + refresh review after fixes

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

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

apps/api/src/broker/outboxWorker.ts

@@ -143,6 +143,56 @@ export function assembleRruleString(
   return s;
 }
 
+/**
+ * IN-02: shared RRULE-resolution decision tree for the update and create branches.
+ *
+ * The two branches differ only in the SOURCE of `preservedRrule` (the update branch
+ * re-reads it from calendarEvents.rawVevent; the create branch reads the
+ * `_preservedRrule` payload field threaded by the edit-as-move route). The precedence
+ * logic is identical and was previously copy-pasted, risking drift between the two
+ * copies of the RFC-5545 bound-strip (`;(UNTIL|COUNT)=` removal) — see Pitfall 3.
+ *
+ * Precedence:
+ *   1. Explicit recurrence on the payload wins (recurrence:'none' clears the RRULE).
+ *   2. Else, if a preserved RRULE exists and the payload changes only the bound
+ *      (UNTIL/COUNT), strip the preserved RRULE's existing bound and re-apply the new
+ *      one — never naive-concatenate (would produce a double-UNTIL/COUNT).
+ *   3. Else fall back to the payload's preset (rruleFromPayload), or the preserved
+ *      RRULE unchanged when no bound change was requested.
+ */
+function resolveFinalRrule(
+  fields: OutboxPayloadFields,
+  hasExplicitRecurrence: boolean,
+  rruleFromPayload: string | undefined,
+  preservedRrule: string | undefined,
+): string | undefined {
+  if (hasExplicitRecurrence) {
+    // Explicit recurrence wins — recurrence:'none' yields undefined (no RRULE emitted)
+    return rruleFromPayload
+      ? assembleRruleString(
+          rruleFromPayload,
+          fields.recurrenceUntil,
+          fields.recurrenceCount,
+          fields.allDay,
+        )
+      : undefined;
+  }
+  if (preservedRrule) {
+    if (fields.recurrenceUntil || fields.recurrenceCount !== undefined) {
+      // Bound change only: strip existing UNTIL/COUNT, then re-apply the new bound (Pitfall 3)
+      const strippedPreset = preservedRrule.replace(/;(UNTIL|COUNT)=[^;]*/g, '');
+      return assembleRruleString(
+        strippedPreset,
+        fields.recurrenceUntil,
+        fields.recurrenceCount,
+        fields.allDay,
+      );
+    }
+    return preservedRrule;
+  }
+  return rruleFromPayload;
+}
+
 // ── Drain concurrency guard (CR-05) ──────────────────────────────────────────
 
 /**
@@ -422,33 +472,13 @@ async function dispatchRow(row: OutboxRow): Promise<DispatchResult> {
     // concatenate onto `FREQ=WEEKLY;BYDAY=...` which would produce double-UNTIL.
     // WR-01 note: preservedRrule is only set when !hasExplicitRecurrence (see above),
     // so the hasExplicitRecurrence branch always takes precedence over preserved RRULE.
-    let finalRruleString: string | undefined;
-    if (hasExplicitRecurrence) {
-      // Explicit recurrence wins — recurrence:'none' yields undefined (no RRULE emitted)
-      finalRruleString = rruleFromPayload
-        ? assembleRruleString(
-            rruleFromPayload,
-            fields.recurrenceUntil,
-            fields.recurrenceCount,
-            fields.allDay,
-          )
-        : undefined;
-    } else if (preservedRrule) {
-      if (fields.recurrenceUntil || fields.recurrenceCount !== undefined) {
-        // Series edit with bound change only: strip existing UNTIL/COUNT, then re-apply
-        const strippedPreset = preservedRrule.replace(/;(UNTIL|COUNT)=[^;]*/g, '');
-        finalRruleString = assembleRruleString(
-          strippedPreset,
-          fields.recurrenceUntil,
-          fields.recurrenceCount,
-          fields.allDay,
-        );
-      } else {
-        finalRruleString = preservedRrule;
-      }
-    } else {
-      finalRruleString = rruleFromPayload;
-    }
+    // IN-02: shared decision tree extracted to resolveFinalRrule (mirrored in create branch).
+    const finalRruleString = resolveFinalRrule(
+      fields,
+      hasExplicitRecurrence,
+      rruleFromPayload,
+      preservedRrule,
+    );
 
     const { icsString } = buildVeventString({
       uid: row.uid,
@@ -520,33 +550,13 @@ async function dispatchRow(row: OutboxRow): Promise<DispatchResult> {
     // CR-01: an explicit recurrence preset wins over _preservedRrule (deliberate user choice).
     // recurrence:'none' explicitly clears any RRULE — including when _preservedRrule is present.
     // If no explicit recurrence, fall back to _preservedRrule (edit-as-move RRULE carry-through).
-    let finalRruleString: string | undefined;
-    if (hasExplicitRecurrence) {
-      // Explicit recurrence wins — recurrence:'none' yields undefined (no RRULE emitted)
-      finalRruleString = rruleFromPayload
-        ? assembleRruleString(
-            rruleFromPayload,
-            fields.recurrenceUntil,
-            fields.recurrenceCount,
-            fields.allDay,
-          )
-        : undefined;
-    } else if (preservedRrule) {
-      if (fields.recurrenceUntil || fields.recurrenceCount !== undefined) {
-        // Bound change on preserved RRULE: strip existing UNTIL/COUNT first (Pitfall 3)
-        const strippedPreset = preservedRrule.replace(/;(UNTIL|COUNT)=[^;]*/g, '');
-        finalRruleString = assembleRruleString(
-          strippedPreset,
-          fields.recurrenceUntil,
-          fields.recurrenceCount,
-          fields.allDay,
-        );
-      } else {
-        finalRruleString = preservedRrule;
-      }
-    } else {
-      finalRruleString = rruleFromPayload;
-    }
+    // IN-02: shared decision tree extracted to resolveFinalRrule (mirrored in update branch).
+    const finalRruleString = resolveFinalRrule(
+      fields,
+      hasExplicitRecurrence,
+      rruleFromPayload,
+      preservedRrule,
+    );
 
     const { icsString } = buildVeventString({
       uid: row.uid,
@@ -840,9 +850,40 @@ export async function runOutboxDrain(): Promise<void> {
  * T-09-02: initOutboxTrigger is called only under isMainModule() in index.ts;
  * tests import runOutboxDrain/scheduleOutboxDrain directly and never register
  * a listener — no open-handle leak preventing process exit.
+ *
+ * WR-02: idempotent. The unsubscribe handle is retained so a duplicate call is a
+ * no-op (never double-registers the 'drain' listener) and stopOutboxTrigger() can
+ * remove it for a clean teardown (tests, graceful shutdown).
  */
+let unsubscribeDrain: (() => void) | null = null;
+
 export function initOutboxTrigger(): void {
-  onOutboxDrain(() => scheduleOutboxDrain());
+  if (unsubscribeDrain) return; // idempotent — never double-register
+  unsubscribeDrain = onOutboxDrain(() => scheduleOutboxDrain());
+}
+
+/**
+ * Remove the 'drain' listener registered by initOutboxTrigger() and reset the
+ * idempotency guard so a later initOutboxTrigger() can re-register cleanly.
+ * Used by the test suite's afterAll teardown (WR-03) and available for graceful
+ * shutdown.
+ */
+export function stopOutboxTrigger(): void {
+  unsubscribeDrain?.();
+  unsubscribeDrain = null;
+}
+
+/**
+ * IN-03: test-only reset for the module-level concurrency flags.
+ * isDraining/drainRequested are the entire concurrency contract for the
+ * single-process deployment and are NOT touched by vi.resetAllMocks(). Tests call
+ * this in beforeEach so each test starts from a known-quiescent state instead of
+ * relying on the previous test having drained cleanly. Not for production use.
+ */
+export function __resetDrainState(): void {
+  if (process.env.NODE_ENV === 'production') return; // test-only; never clear the guard in prod
+  isDraining = false;
+  drainRequested = false;
 }
 
 /**

apps/api/src/lib/outboxTrigger.ts

@@ -23,10 +23,18 @@ const emitter = new EventEmitter();
 
 /**
  * Fire a drain signal after a successful outbox enqueue commit.
- * Synchronous and fire-and-forget — never awaited (D-04).
+ * Fire-and-forget — never awaited (D-04).
+ *
+ * WR-01: the emit is deferred to a microtask so the registered listener
+ * (scheduleOutboxDrain) never runs on the enqueue caller's stack. EventEmitter.emit
+ * dispatches listeners synchronously; without this defer, any synchronous throw in
+ * the listener chain would propagate back into the route handler's try/catch and
+ * surface a misleading 503 for an enqueue that actually committed. Deferring keeps
+ * the documented decoupling honest: a misbehaving listener can never corrupt the
+ * route response.
  */
 export function signalOutboxDrain(): void {
-  emitter.emit('drain');
+  queueMicrotask(() => emitter.emit('drain'));
 }
 
 /**

apps/api/src/routes/events.ts

@@ -307,6 +307,7 @@ eventsRouter.post('/create', zValidator('json', eventFieldsSchema), async (c) =>
       payload: JSON.stringify(payload),
     });
 
+    // must stay AFTER the enqueue commit — worker selects committed pending rows only
     signalOutboxDrain();
     return c.json({ uid }, 202);
   } catch (err) {
@@ -431,6 +432,7 @@ eventsRouter.patch('/:uid/edit', zValidator('json', eventFieldsSchema), async (c
         });
       });
 
+      // must stay AFTER the enqueue commit — worker selects committed pending rows only
       signalOutboxDrain();
       return c.json({ uid: newUid }, 202);
     }
@@ -447,6 +449,7 @@ eventsRouter.patch('/:uid/edit', zValidator('json', eventFieldsSchema), async (c
       payload: JSON.stringify(payload),
     });
 
+    // must stay AFTER the enqueue commit — worker selects committed pending rows only
     signalOutboxDrain();
     return c.json({ uid }, 202);
   } catch (err) {
@@ -522,6 +525,7 @@ eventsRouter.delete('/:uid', async (c) => {
       etag: eventRow.etag ?? undefined,
     });
 
+    // must stay AFTER the enqueue commit — worker selects committed pending rows only
     signalOutboxDrain();
     return c.json({ uid }, 202);
   } catch (err) {

apps/api/tests/broker/outboxWorker.test.ts

@@ -1,5 +1,5 @@
 /**
- * RED test scaffold: broker/outboxWorker.ts — outbox state machine (D-04, D-07, D-08)
+ * broker/outboxWorker.ts — outbox state machine (D-04, D-07, D-08)
  *
  * Behaviors under test:
  *   1. runOutboxDrain transitions pending→done on mock 204 response
@@ -8,17 +8,19 @@
  *      on mock 500 (transient error)
  *   4. runOutboxDrain transitions pending→dead when attemptCount reaches MAX_ATTEMPTS
  *   5. Edit-as-move (D-04): create row processed BEFORE the linked delete row (groupId)
- *
- * These tests FAIL (RED) because broker/outboxWorker.ts does not exist yet.
- * They will turn GREEN in Plan 03-03 when the implementation is added.
+ *   6. scheduleOutboxDrain trigger wiring (D-09): signal → prompt drain, trailing re-drain collapse
  */
 
-import { describe, it, expect, vi, beforeEach, beforeAll } from 'vitest';
+import { describe, it, expect, vi, beforeEach, beforeAll, afterAll } from 'vitest';
 
-// This import fails (RED) — broker/outboxWorker.ts does not exist yet.
-// eslint-disable-next-line @typescript-eslint/ban-ts-comment
-// @ts-ignore intentional RED import
-import { runOutboxDrain, assembleRruleString, scheduleOutboxDrain, initOutboxTrigger } from '../../src/broker/outboxWorker.js';
+import {
+  runOutboxDrain,
+  assembleRruleString,
+  scheduleOutboxDrain,
+  initOutboxTrigger,
+  stopOutboxTrigger,
+  __resetDrainState,
+} from '../../src/broker/outboxWorker.js';
 import { signalOutboxDrain } from '../../src/lib/outboxTrigger.js';
 
 // ── Drizzle DB mock ────────────────────────────────────────────────────────
@@ -667,8 +669,7 @@ describe('runOutboxDrain — fail closed on bad credentials (CR-03) + backoff in
 });
 
 // ─── D-06: assembleRruleString unit tests ─────────────────────────────────────
-// These tests import the NOT-YET-EXPORTED `assembleRruleString` helper.
-// RED: will fail because assembleRruleString is not exported yet.
+// Exercise the exported `assembleRruleString` helper.
 
 describe('assembleRruleString (D-06)', () => {
   it('returns base preset unchanged when no bound given', () => {
@@ -701,8 +702,8 @@ describe('assembleRruleString (D-06)', () => {
 });
 
 // ─── D-07: FREQ-persistence regression lock ────────────────────────────────────
-// RED: will fail because the outbox worker does not yet wire recurrenceUntil/recurrenceCount
-// and the FREQ-persistence assertion catches the D-07 regression scenario.
+// The FREQ-persistence assertion catches the D-07 regression scenario
+// (recurrenceUntil/recurrenceCount wiring must not drop the FREQ).
 
 describe('FREQ persistence (D-07 regression)', () => {
   beforeEach(() => {
@@ -741,8 +742,6 @@ describe('FREQ persistence (D-07 regression)', () => {
 //   SC-1: signalOutboxDrain() → drain fires promptly (no 15s wait)
 //   D-05: mid-drain signal collapses to exactly one trailing re-drain
 //   D-07: concurrent scheduleOutboxDrain() calls dispatch each row exactly once
-//
-// RED: these tests fail because scheduleOutboxDrain is not yet exported.
 
 describe('scheduleOutboxDrain — trigger wiring (D-09)', () => {
   // Wire the EventEmitter signal → scheduleOutboxDrain once for this describe block.
@@ -752,8 +751,18 @@ describe('scheduleOutboxDrain — trigger wiring (D-09)', () => {
     initOutboxTrigger();
   });
 
+  // WR-03: remove the leaked 'drain' listener so it cannot fire against the mocked
+  // DB in subsequent describe blocks (fileParallelism:false shares one module instance).
+  afterAll(() => {
+    stopOutboxTrigger();
+  });
+
   beforeEach(() => {
     vi.resetAllMocks();
+    // WR-03 / IN-03: module-level isDraining/drainRequested are not reset by
+    // vi.resetAllMocks(); reset them explicitly so each test starts quiescent
+    // instead of relying on the previous test having drained cleanly.
+    __resetDrainState();
     mockPendingRows = [];
     wireMockChain();
   });