docs(09): add code review report
This commit is contained in:
Lucas Berger
@@ -0,0 +1,186 @@
|
||||
---
|
||||
phase: 09-faster-write-back
|
||||
reviewed: 2026-06-12T17:05:00Z
|
||||
depth: standard
|
||||
files_reviewed: 5
|
||||
files_reviewed_list:
|
||||
- apps/api/src/lib/outboxTrigger.ts
|
||||
- apps/api/src/broker/outboxWorker.ts
|
||||
- apps/api/src/routes/events.ts
|
||||
- apps/api/src/index.ts
|
||||
- apps/api/tests/broker/outboxWorker.test.ts
|
||||
findings:
|
||||
critical: 0
|
||||
warning: 4
|
||||
info: 3
|
||||
total: 7
|
||||
status: issues_found
|
||||
---
|
||||
|
||||
# Phase 9: Code Review Report
|
||||
|
||||
**Reviewed:** 2026-06-12T17:05:00Z
|
||||
**Depth:** standard
|
||||
**Files Reviewed:** 5
|
||||
**Status:** issues_found
|
||||
|
||||
## Summary
|
||||
|
||||
Reviewed the event-driven outbox drain trigger added in phase 09 (CAL-15): a zero-dependency
|
||||
in-process `EventEmitter` (`outboxTrigger.ts`), the `scheduleOutboxDrain` wrapper with the
|
||||
`isDraining` guard + `drainRequested` trailing-re-drain loop in `outboxWorker.ts`, four
|
||||
fire-and-forget `signalOutboxDrain()` publish sites in `events.ts`, and `initOutboxTrigger()`
|
||||
startup wiring under `isMainModule()` in `index.ts`.
|
||||
|
||||
The core concurrency design is sound on the four points called out in the brief:
|
||||
|
||||
- **Trailing-re-drain is bounded.** `drainRequested` is reset to `false` BEFORE the recursive
|
||||
`scheduleOutboxDrain()` call (outboxWorker.ts:199-200), so a signal arriving during the
|
||||
trailing drain produces at most one further pass — no unbounded re-drain chain. A signal that
|
||||
arrives mid-drain sets `drainRequested=true` and is re-SELECTed by the trailing pass; the
|
||||
publish sites fire `signalOutboxDrain()` only AFTER the row's DB insert commits, so the row is
|
||||
always visible to the re-drain. No lost-wakeup.
|
||||
- **Signal never fires inside the `db.transaction` callback.** All four publish sites (events.ts
|
||||
310, 434, 450, 525) call `signalOutboxDrain()` after the commit resolves and outside the
|
||||
transaction closure; D-03 ordering holds.
|
||||
- **Fire-and-forget is rejection-safe.** `signalOutboxDrain` is synchronous (`emitter.emit`); the
|
||||
sole listener delegates to `scheduleOutboxDrain`, which is `void`-returning with a `.catch()`
|
||||
attached to the `runOutboxDrain()` promise. `runOutboxDrain` is `async`, so it can never throw
|
||||
synchronously into `emit`/the route handler — no floating promise, no unhandled rejection.
|
||||
- **Tests pass (30/30) and the trigger-wiring suite exercises SC-1, D-05 collapse, and the
|
||||
concurrency guard.**
|
||||
|
||||
Remaining findings are robustness/maintainability (WARNING) and clarity (INFO). No BLOCKERs.
|
||||
|
||||
## Warnings
|
||||
|
||||
### WR-01: `initOutboxTrigger()` is non-idempotent and leaks its listener
|
||||
|
||||
**File:** `apps/api/src/broker/outboxWorker.ts:844-846`
|
||||
**Issue:** `initOutboxTrigger` calls `onOutboxDrain(...)` and discards the returned unsubscribe
|
||||
function, and has no guard against repeated registration. Each invocation adds another `'drain'`
|
||||
listener to the module-singleton emitter. Production calls it once under `isMainModule()`, so the
|
||||
hot path is fine — but the function is exported, has no internal idempotency, and the default
|
||||
`EventEmitter` max-listeners is 10 (the module comment in `outboxTrigger.ts:11` explicitly relies
|
||||
on never calling `setMaxListeners`). A second `initOutboxTrigger()` call (a future second wiring
|
||||
site, a hot-reload, or a test that re-inits) silently double-drains on every signal and, after 10
|
||||
registrations, emits a `MaxListenersExceededWarning`. The leak is masked today only because there
|
||||
is exactly one caller.
|
||||
**Fix:** Make registration idempotent and retain the unsubscribe handle:
|
||||
```ts
|
||||
let unsubscribeDrain: (() => void) | null = null;
|
||||
export function initOutboxTrigger(): void {
|
||||
if (unsubscribeDrain) return; // already wired — no double-registration
|
||||
unsubscribeDrain = onOutboxDrain(() => scheduleOutboxDrain());
|
||||
}
|
||||
```
|
||||
|
||||
### WR-02: Trigger-wiring tests register a listener in `beforeAll` and never remove it
|
||||
|
||||
**File:** `apps/api/tests/broker/outboxWorker.test.ts:751-753`
|
||||
**Issue:** The `scheduleOutboxDrain — trigger wiring` block calls `initOutboxTrigger()` in
|
||||
`beforeAll`, registering a permanent `'drain'` listener on the module-singleton emitter that is
|
||||
never torn down (no `afterAll`/`afterEach` unsubscribe). Because the emitter is a module
|
||||
singleton shared across the whole file (and any future file importing it), any later
|
||||
`signalOutboxDrain()` — in a subsequent describe block, a newly added test, or another test file
|
||||
that imports `signalOutboxDrain` — will fire the still-registered `scheduleOutboxDrain` listener
|
||||
and kick off a real `runOutboxDrain()` against the shared `mockPendingRows`/db mock. That is
|
||||
exactly the cross-test state pollution the WR-04/T-09-02 comments warn about. It is latent now
|
||||
only because the polluting describe block happens to run last and no other test calls
|
||||
`signalOutboxDrain`; test execution order is not a contract.
|
||||
**Fix:** Capture and release the listener (requires WR-01's handle, or expose a teardown). Minimal
|
||||
test-only fix:
|
||||
```ts
|
||||
describe('scheduleOutboxDrain — trigger wiring (D-09)', () => {
|
||||
let unsub: () => void;
|
||||
beforeAll(() => {
|
||||
// import onOutboxDrain directly so the test owns the unsubscribe
|
||||
unsub = onOutboxDrain(() => scheduleOutboxDrain());
|
||||
});
|
||||
afterAll(() => unsub());
|
||||
// ...
|
||||
});
|
||||
```
|
||||
or, once WR-01 lands, add an `afterAll` that calls a new `shutdownOutboxTrigger()` exported for
|
||||
tests.
|
||||
|
||||
### WR-03: `setInterval` handle is never retained — interval cannot be cleared and keeps the loop alive
|
||||
|
||||
**File:** `apps/api/src/broker/outboxWorker.ts:856-860`
|
||||
**Issue:** `startOutboxWorker` calls `setInterval(...)` but discards the returned `Timeout`. There
|
||||
is no way to stop the drain schedule (graceful shutdown, SIGTERM handler, or test teardown), and
|
||||
the unref-less interval keeps the event loop alive. This pre-dates phase 09 but the phase rewired
|
||||
the interval body (`runOutboxDrain` → `scheduleOutboxDrain`), so it is in scope. On a clean
|
||||
shutdown the process cannot drain-and-exit; a drain may be mid-flight when the process is killed.
|
||||
**Fix:** Retain the handle and expose a stop hook (and/or `.unref()` if the interval should not by
|
||||
itself hold the process open):
|
||||
```ts
|
||||
let drainInterval: ReturnType<typeof setInterval> | null = null;
|
||||
export function startOutboxWorker(): void {
|
||||
drainInterval = setInterval(() => scheduleOutboxDrain(), 15 * 1000);
|
||||
}
|
||||
export function stopOutboxWorker(): void {
|
||||
if (drainInterval) { clearInterval(drainInterval); drainInterval = null; }
|
||||
}
|
||||
```
|
||||
|
||||
### WR-04: Module-level `isDraining`/`drainRequested` shared by event-path and interval-path defeats unit isolation
|
||||
|
||||
**File:** `apps/api/src/broker/outboxWorker.ts:162, 172`
|
||||
**Issue:** Both guard flags are module-level mutable singletons. They are correct for the
|
||||
single-process runtime, but they persist across vitest test cases within the file and are not
|
||||
reset by `vi.resetAllMocks()`. If a test leaves `isDraining=true` or `drainRequested=true` — e.g.
|
||||
a `runOutboxDrain()` that throws synchronously before the `try` (none today) or a future test that
|
||||
abandons an in-flight `scheduleOutboxDrain` without flushing its `.finally()` — the next test
|
||||
silently no-ops its drain (`if (isDraining) return`) and the failure surfaces as a confusing
|
||||
"createCalendarEvent not called" assertion far from the cause. The CR-05/D-07 concurrency tests
|
||||
already depend on these flags being clean at entry but nothing guarantees it.
|
||||
**Fix:** Export a test-only reset (or reset in `beforeEach`):
|
||||
```ts
|
||||
export function __resetDrainStateForTests(): void {
|
||||
isDraining = false;
|
||||
drainRequested = false;
|
||||
}
|
||||
```
|
||||
and call it in each `beforeEach` alongside `wireMockChain()`. Alternatively, document that every
|
||||
`scheduleOutboxDrain()` started in a test MUST be awaited to completion before the test returns.
|
||||
|
||||
## Info
|
||||
|
||||
### IN-01: `onOutboxDrain` returns an unsubscribe that no production caller uses
|
||||
|
||||
**File:** `apps/api/src/lib/outboxTrigger.ts:36-39`
|
||||
**Issue:** `onOutboxDrain` faithfully returns an `off()` closure (good API design, mirrors
|
||||
`listEmitter.ts`), but the only production caller (`initOutboxTrigger`) throws it away (see WR-01).
|
||||
The capability exists but is unreachable, so the "you can stop delivery" contract in the doc
|
||||
comment is currently aspirational.
|
||||
**Fix:** Once WR-01 retains the handle, this resolves itself. No standalone change needed.
|
||||
|
||||
### IN-02: Doc comment references "D-06" for the trailing-re-drain flag; design notes attribute it to D-05
|
||||
|
||||
**File:** `apps/api/src/broker/outboxWorker.ts:165, 168`
|
||||
**Issue:** The `drainRequested` block header reads `D-05 / D-06` and the body says "never more than
|
||||
one extra pass (D-06)", while `scheduleOutboxDrain`'s own doc (line 178-179) and the file header
|
||||
(line 21) attribute the trailing-re-drain to D-05. Minor decision-ID drift; harmless but invites
|
||||
confusion when cross-referencing the phase decision log.
|
||||
**Fix:** Normalize to the authoritative decision ID for the trailing-re-drain (D-05) and drop the
|
||||
stray D-06 unless a D-06 clause genuinely governs the "one extra pass" bound.
|
||||
|
||||
### IN-03: `signalOutboxDrain()` runs `runOutboxDrain` synchronously up to the first `await` on the request thread
|
||||
|
||||
**File:** `apps/api/src/routes/events.ts:310` (and 434, 450, 525)
|
||||
**Issue:** `emit('drain')` invokes the listener synchronously, so `scheduleOutboxDrain` →
|
||||
`runOutboxDrain` executes inline on the request path until `runOutboxDrain` hits its first `await`
|
||||
(the pending-rows SELECT at outboxWorker.ts:653). The synchronous prefix is tiny (a flag check
|
||||
plus issuing the query), so the 202 is not meaningfully delayed today — but the cost is paid on
|
||||
the HTTP request thread rather than deferred. If `runOutboxDrain`'s pre-await section ever grows
|
||||
(more bookkeeping before the first `await`), it silently becomes request-path latency.
|
||||
**Fix:** If you want a hard guarantee the request returns before any drain work, defer the kick:
|
||||
`onOutboxDrain(() => queueMicrotask(() => scheduleOutboxDrain()))` or `setImmediate(...)`. Optional;
|
||||
acceptable as-is given the trivial synchronous prefix.
|
||||
|
||||
---
|
||||
|
||||
_Reviewed: 2026-06-12T17:05:00Z_
|
||||
_Reviewer: Claude (gsd-code-reviewer)_
|
||||
_Depth: standard_
|
||||
Reference in New Issue
Block a user