diff --git a/.planning/PROJECT.md b/.planning/PROJECT.md index 61b1977..87a4f4b 100644 --- a/.planning/PROJECT.md +++ b/.planning/PROJECT.md @@ -36,6 +36,7 @@ Deferred to backlog: self-service provider onboarding (999.5) and provider abstr - [x] Shared collaborative lists (groceries, gift ideas) co-edited by both members, stored in MariaDB — **Validated in Phase 4 (shared-lists-live-sync)**: list + item CRUD, fractional-rank drag-reorder, member-scoped access (no cross-tenant leak proven at route layer). - [x] Live list sync so co-edits appear without manual refresh — **Validated in Phase 4**: scoped SSE fan-out over Pangolin (transport smoke-tested), bounded-backoff reconnect, co-edits land within seconds. - [x] Web Push notifications for event reminders and list changes — **Validated in Phase 5 (web-push-notifications)**: VAPID push for reminders, event-change, and coalesced list alerts; on-device UAT 1/2/5 PASS (iOS reminder delivery, iOS push, coalescing). Android event-change on-device confirmation + iOS standalone spinner remain device-only spot-checks at go-live. +- [x] Faster write-back so edits reach Fastmail in ~1–2s instead of ~15s (CAL-15) — **Validated in Phase 9 (faster-write-back)**: event-driven outbox drain via a zero-dependency in-process EventEmitter (`outboxTrigger.ts`); a committed enqueue publishes a fire-and-forget `signalOutboxDrain()` that funnels through the existing `isDraining`-guarded drain with a `drainRequested` trailing-re-drain, preserving optimistic-202, create-before-delete on moves, exactly-once per uid, and the 15s `setInterval` fallback. 5/5 success criteria verified; trigger-wiring tests assert SC-1/D-05/D-07. ### Active @@ -119,4 +120,4 @@ This document evolves at phase transitions and milestone boundaries. --- -_Last updated: 2026-06-11 — Phase 7 (Mobile Test Harness) complete; TEST-01/TEST-02 validated_ +_Last updated: 2026-06-12 — Phase 9 (Faster Write-Back) complete; CAL-15 validated_ diff --git a/.planning/REQUIREMENTS.md b/.planning/REQUIREMENTS.md index cfa8c40..6e30d6a 100644 --- a/.planning/REQUIREMENTS.md +++ b/.planning/REQUIREMENTS.md @@ -14,7 +14,7 @@ Each requirement maps to exactly one roadmap phase (see Traceability). - [ ] **CAL-13**: User can choose a reminder lead time when creating or editing an event from a preset list (None / 5m / 10m / 15m / 30m / 1h / 2h / 1d / 2d), with **"None" as the default**; the choice is serialized as a VALARM on the event written back to Fastmail. - [ ] **CAL-14**: Editing an event **preserves any existing reminder/VALARM** set in another client (Fastmail or native) — reminders are never silently stripped on round-trip. -- [ ] **CAL-15**: A created, edited, or deleted event reaches Fastmail within ~2 seconds (event-driven outbox drain) instead of up to ~15s, while preserving the optimistic-202 accept and all outbox durability guarantees (create-before-delete ordering, drain concurrency guard, fresh-etag-before-PUT, per-uid exactly-once). +- [x] **CAL-15**: A created, edited, or deleted event reaches Fastmail within ~2 seconds (event-driven outbox drain) instead of up to ~15s, while preserving the optimistic-202 accept and all outbox durability guarantees (create-before-delete ordering, drain concurrency guard, fresh-etag-before-PUT, per-uid exactly-once). ### Notifications — Variable-lead reminder scheduling @@ -75,7 +75,7 @@ Maps each REQ-ID to its phase. v1.1 phases continue v1.0 numbering (v1.0 ended a | TEST-02 | Phase 7 (Mobile Test Harness) | Complete | | CI-01 | Phase 8 (Gitea CI) | Complete | | CI-02 | Phase 8 (Gitea CI) | Complete | -| CAL-15 | Phase 9 (Faster Write-Back) | Pending | +| CAL-15 | Phase 9 (Faster Write-Back) | Complete | | ADMIN-01 | Phase 10 (Admin Role & Settings) | Pending | | ADMIN-02 | Phase 10 (Admin Role & Settings) | Pending | | ADMIN-03 | Phase 10 (Admin Role & Settings) | Pending | diff --git a/.planning/ROADMAP.md b/.planning/ROADMAP.md index a688908..948e759 100644 --- a/.planning/ROADMAP.md +++ b/.planning/ROADMAP.md @@ -27,7 +27,7 @@ Make FamilySync configurable, administrable, and maintainable for real multi-mem - [x] **Phase 7: Mobile Test Harness** - Mobile-emulated, authenticated PWA browser harness so the assistant (and CI) can catch mobile-only defects (completed 2026-06-11) - [x] **Phase 8: Gitea CI** - Full regression on PR to main (lint/typecheck/unit/API-integration vs a MariaDB service container **+ the Phase 7 mobile harness as a UI-regression step against a CI-hosted dev stack**) + Docker image publish on merge (completed 2026-06-11) -- [ ] **Phase 9: Faster Write-Back** - Event-driven outbox drain so edits land in ~1-2s instead of ~15s, preserving every outbox durability guarantee +- [x] **Phase 9: Faster Write-Back** - Event-driven outbox drain so edits land in ~1-2s instead of ~15s, preserving every outbox durability guarantee (completed 2026-06-12) - [ ] **Phase 10: Admin Role & Settings** - DB foundation (is_admin / reminder_lead / app_config) + role-gated admin UI to rotate app passwords and designate the shared calendar - [ ] **Phase 11: Per-Event Reminders** - Reminder selector on the event form (incl. "None") serialized as VALARM, with a variable-lead scheduler that honors each event's choice - [ ] **Phase 12: Initial Setup Wizard** - First-run validated bootstrap of env/VAPID/DB/OIDC + first app password, reusing the admin route surface @@ -136,7 +136,16 @@ Make FamilySync configurable, administrable, and maintainable for real multi-mem - **Create-before-delete under concurrent enqueues** (Pitfall 6): enqueue CREATE before DELETE; do not fire the signal between the two inserts of a move (publish after both inserts / after the transaction commits). - Hard constraints: `setInterval` only (no node-cron); single-process by design — **no Redis** for the drain (Redis stays for list SSE); all outbox guarantees (fresh-etag-before-PUT, 412 conflict flow, per-uid exactly-once) unchanged. -**Plans**: TBD +**Plans**: 2 plans (2 waves) + +Plans: +**Wave 1** + +- [x] 09-01-PLAN.md — TDD: outboxTrigger.ts (zero-dep EventEmitter signal) + scheduleOutboxDrain wrapper / drainRequested trailing-re-drain loop + initOutboxTrigger in outboxWorker.ts; trigger-wiring tests (SC-1, SC-4/D-07, D-05) (Wave 1) + +**Wave 2** *(blocked on Wave 1 completion)* + +- [x] 09-02-PLAN.md — Four post-commit signalOutboxDrain() publish sites in events.ts (create / edit-as-move-after-transaction / same-cal update / delete) + initOutboxTrigger() startup wiring under isMainModule() in index.ts (Wave 2) ### Phase 10: Admin Role & Settings @@ -308,7 +317,7 @@ Plans: | 6. UX Polish | v1.0 | 6/6 | Complete | 2026-06-10 | | 7. Mobile Test Harness | v1.1 | 4/4 | Complete | 2026-06-11 | | 8. Gitea CI | v1.1 | 4/4 | Complete | 2026-06-11 | -| 9. Faster Write-Back | v1.1 | 0/? | Not started | - | +| 9. Faster Write-Back | v1.1 | 2/2 | Complete | 2026-06-12 | | 10. Admin Role & Settings | v1.1 | 0/? | Not started | - | | 11. Per-Event Reminders | v1.1 | 0/? | Not started | - | | 12. Initial Setup Wizard | v1.1 | 0/? | Not started | - | @@ -322,7 +331,7 @@ Plans: **Goal:** [Captured for future planning] Abstract the calendar backend behind a provider interface so Fastmail/CalDAV is one implementation among potentially many. Shipping with a single provider is fine, but the broker, sync, and event-expansion layers should be structured so additional providers (e.g. other CalDAV hosts, Google Calendar, generic ICS feeds) can be added without rework. Captures the "provider" seam as an explicit architectural concern. **Requirements:** TBD -**Plans:** 3/3 plans complete +**Plans:** 2/2 plans complete Plans: diff --git a/.planning/STATE.md b/.planning/STATE.md index 3af28c3..32a7278 100644 --- a/.planning/STATE.md +++ b/.planning/STATE.md @@ -2,16 +2,16 @@ gsd_state_version: 1.0 milestone: v1.1 milestone_name: Operability & Polish -status: completed -stopped_at: Phase 14 context gathered -last_updated: "2026-06-12T15:18:17.057Z" +status: executing +stopped_at: Phase 9 context gathered +last_updated: "2026-06-12T21:08:34.135Z" last_activity: 2026-06-12 progress: total_phases: 18 - completed_phases: 5 - total_plans: 15 - completed_plans: 15 - percent: 28 + completed_phases: 6 + total_plans: 17 + completed_plans: 17 + percent: 33 --- # Project State @@ -21,13 +21,13 @@ progress: See: .planning/PROJECT.md (updated 2026-06-10) **Core value:** One color-coded family calendar (shared + personal) and shared lists from a single low-friction PWA — cross-ecosystem, no app store -**Current focus:** Phase 15 — ci-skip-api-harness-jobs-for-doc-only-prs +**Current focus:** Phase 09 — faster-write-back ## Current Position -Phase: 999.1 +Phase: 13 Plan: Not started -Status: Plans 15-01 + 15-02 complete; 15-03 Task 1 (publish.yml comment) committed (da623ac); 15-03 Task 2 awaiting operator after branch merges to main +Status: Ready to execute Last activity: 2026-06-12 ### Deferred Checkpoint — Phase 15 Plan 15-03 Task 2 (human-action) @@ -47,7 +47,7 @@ Resume: after the operator completes the change, re-run `/gsd-execute-phase 15` **Velocity:** -- Total plans completed: 31 +- Total plans completed: 33 - Average duration: - - Total execution time: 0 hours @@ -61,6 +61,7 @@ Resume: after the operator completes the change, re-run `/gsd-execute-phase 15` | 13 | 3 | - | - | | 14 | 1 | - | - | | 15 | 3 | - | - | +| 09 | 2 | - | - | **Recent Trend:** @@ -101,6 +102,7 @@ _Updated after each plan completion_ | Phase 13 P01 | 8 | 2 tasks | 7 files | | Phase 13-real-lint-gate-eslint P02 | 90 | 2 tasks | 31 files | | Phase 13-real-lint-gate-eslint P03 | 10 | 3 tasks | 399 files | +| Phase 09-faster-write-back P01 | 341 | 3 tasks | 3 files | ## Accumulated Context @@ -224,9 +226,9 @@ Recent decisions affecting current work: ## Session Continuity -Last session: 2026-06-12T11:57:10.391Z -Stopped at: Phase 14 context gathered -Resume file: .planning/phases/14-desktop-e2e-coverage/14-CONTEXT.md +Last session: 2026-06-12T20:54:10.362Z +Stopped at: Phase 9 context gathered +Resume file: .planning/phases/09-faster-write-back/09-CONTEXT.md ## Operator Next Steps diff --git a/.planning/phases/09-faster-write-back/09-01-PLAN.md b/.planning/phases/09-faster-write-back/09-01-PLAN.md new file mode 100644 index 0000000..bbc2f6c --- /dev/null +++ b/.planning/phases/09-faster-write-back/09-01-PLAN.md @@ -0,0 +1,208 @@ +--- +phase: 09-faster-write-back +plan: 01 +type: tdd +wave: 1 +depends_on: [] +files_modified: + - apps/api/src/lib/outboxTrigger.ts + - apps/api/src/broker/outboxWorker.ts + - apps/api/tests/broker/outboxWorker.test.ts +autonomous: true +requirements: [CAL-15] + +must_haves: + truths: + - "Calling signalOutboxDrain() while no drain is in flight invokes runOutboxDrain promptly (no 15s wait)" + - "A signal arriving during an in-flight drain triggers exactly one trailing re-drain (D-05)" + - "Concurrent scheduleOutboxDrain() calls dispatch each pending row exactly once — no duplicate CalDAV PUT (D-07)" + - "runOutboxDrain's body and the isDraining guard are behaviorally unchanged — its existing 15 tests still pass" + - "scheduleOutboxDrain catches drain errors so a listener error never escapes the wrapper (D-02)" + - "No debounce/coalesce window and no rejection of mid-drain signals: drainRequested is a plain boolean that collapses every mid-drain signal into exactly one trailing drain — never rate-limited or dropped (D-06)" + - "Verification is fully automated by the trigger-wiring Vitest tests; no operator-stopwatch human checkpoint is required and the CI-guarded test is the durable latency evidence (D-10)" + artifacts: + - path: "apps/api/src/lib/outboxTrigger.ts" + provides: "Zero-dependency in-process EventEmitter drain signal (signalOutboxDrain, onOutboxDrain)" + exports: ["signalOutboxDrain", "onOutboxDrain"] + contains: "node:events" + - path: "apps/api/src/broker/outboxWorker.ts" + provides: "scheduleOutboxDrain wrapper + drainRequested flag + initOutboxTrigger subscription" + exports: ["scheduleOutboxDrain", "initOutboxTrigger"] + contains: "drainRequested" + - path: "apps/api/tests/broker/outboxWorker.test.ts" + provides: "Trigger-wiring test block (SC-1, SC-4, D-05)" + contains: "scheduleOutboxDrain" + key_links: + - from: "apps/api/src/broker/outboxWorker.ts (initOutboxTrigger)" + to: "apps/api/src/lib/outboxTrigger.ts (onOutboxDrain)" + via: "onOutboxDrain(() => scheduleOutboxDrain())" + pattern: "onOutboxDrain\\(" + - from: "apps/api/src/broker/outboxWorker.ts (scheduleOutboxDrain)" + to: "apps/api/src/broker/outboxWorker.ts (runOutboxDrain)" + via: "guarded call through isDraining + drainRequested" + pattern: "scheduleOutboxDrain" +--- + + +Build the event-driven drain signal mechanism: a zero-dependency in-process `EventEmitter` (`outboxTrigger.ts`) and the `scheduleOutboxDrain` scheduler wrapper in `outboxWorker.ts` that funnels signals through the existing `isDraining`-guarded `runOutboxDrain()` with a `drainRequested` trailing-re-drain loop (D-05). Prove SC-1, SC-4, and D-05 with automated trigger-wiring tests (D-09). + +Purpose: Removes the up-to-15s queueing delay before the CalDAV round-trip by signalling the drain on enqueue, while preserving every outbox durability guarantee — `runOutboxDrain`'s internals and the `isDraining` guard stay behaviorally unchanged (CAL-15 / D-02 / D-07). +Output: `outboxTrigger.ts` (new), the `scheduleOutboxDrain` + `drainRequested` + `initOutboxTrigger` additions to `outboxWorker.ts`, and a new trigger-wiring describe block in `outboxWorker.test.ts`. + + + +@$HOME/.claude/gsd-core/workflows/execute-plan.md +@$HOME/.claude/gsd-core/templates/summary.md + + + +@.planning/PROJECT.md +@.planning/ROADMAP.md +@.planning/STATE.md +@.planning/phases/09-faster-write-back/09-CONTEXT.md +@.planning/phases/09-faster-write-back/09-RESEARCH.md +@.planning/phases/09-faster-write-back/09-PATTERNS.md +@.planning/phases/09-faster-write-back/09-VALIDATION.md + + + +New symbols this phase creates (none exist yet — do not expect them in API-SURFACE.md): + +| Symbol | File | Created By | Kind | +|--------|------|-----------|------| +| `apps/api/src/lib/outboxTrigger.ts` | (new file) | Plan 01 Task 1 | module | +| `signalOutboxDrain` | `apps/api/src/lib/outboxTrigger.ts` | Plan 01 Task 1 | exported function | +| `onOutboxDrain` | `apps/api/src/lib/outboxTrigger.ts` | Plan 01 Task 1 | exported function | +| `drainRequested` | `apps/api/src/broker/outboxWorker.ts` | Plan 01 Task 3 | module-level flag | +| `scheduleOutboxDrain` | `apps/api/src/broker/outboxWorker.ts` | Plan 01 Task 3 | exported function | +| `initOutboxTrigger` | `apps/api/src/broker/outboxWorker.ts` | Plan 01 Task 3 | exported function | + +Plan 02 consumes `signalOutboxDrain` (in `events.ts`) and `initOutboxTrigger` (in `index.ts`). + + + + + + Task 1: Create outboxTrigger.ts zero-dependency EventEmitter signal module + apps/api/src/lib/outboxTrigger.ts + + - apps/api/src/lib/outboxTrigger.ts (the file being created — confirm it does not yet exist) + - apps/api/src/lib/listEmitter.ts (canonical analog: module-level `const emitter = new EventEmitter()`, named publish/subscribe wrappers, zero internal imports) + + + - signalOutboxDrain() emits the 'drain' event on the module-level emitter (synchronous; fire-and-forget; returns void) + - onOutboxDrain(handler) registers handler on 'drain' and returns an unsubscribe function that calls emitter.off('drain', handler) + - A handler registered via onOutboxDrain is invoked exactly once per signalOutboxDrain() call + - After the returned unsubscribe is called, a subsequent signalOutboxDrain() does NOT invoke the handler + + + Create `apps/api/src/lib/outboxTrigger.ts` following the `listEmitter.ts` module-level singleton pattern exactly. Import `EventEmitter` from `node:events`. Declare `const emitter = new EventEmitter();` at module scope. Do NOT call `setMaxListeners` — there is exactly one subscriber (the `initOutboxTrigger` listener), so the default limit of 10 is correct (unlike `listEmitter.ts` which sets 200 for SSE fan-out per its T-04-04 comment). Export `signalOutboxDrain(): void` that calls `emitter.emit('drain')` (fire-and-forget per D-04). Export `onOutboxDrain(handler: () => void): () => void` that calls `emitter.on('drain', handler)` and returns `() => emitter.off('drain', handler)`. This module MUST import nothing from `broker/`, `routes/`, `db/`, or `index.ts` — only `node:events` — to guarantee no circular import (same zero-internal-dependency rule as `listEmitter.ts`). Add a header comment noting: single subscriber, default listener limit fine, signal is fire-and-forget, published only after enqueue commit (D-01/D-03/D-04). + + + cd apps/api && npx tsc --noEmit 2>&1 | grep -v '^$' | grep -i 'outboxTrigger' ; test ${PIPESTATUS[1]} -ne 0 || echo "no tsc errors in outboxTrigger"; grep -q "export function signalOutboxDrain" src/lib/outboxTrigger.ts && grep -q "export function onOutboxDrain" src/lib/outboxTrigger.ts && grep -q "node:events" src/lib/outboxTrigger.ts && echo OK + + + - `apps/api/src/lib/outboxTrigger.ts` exists and exports `signalOutboxDrain` and `onOutboxDrain` + - File imports `EventEmitter` from `node:events` and nothing from `broker/`, `routes/`, `db/`, or `index.ts` (`grep -v 'node:events' src/lib/outboxTrigger.ts | grep -E "from '\\.\\./(broker|routes|db)" ` returns nothing) + - No `setMaxListeners` call present + - `cd apps/api && npx tsc --noEmit` reports no errors referencing `outboxTrigger.ts` + + outboxTrigger.ts exists, exports signalOutboxDrain + onOutboxDrain, depends only on node:events, typechecks clean. + + + + Task 2: RED — add failing trigger-wiring tests for SC-1, D-05, D-07 + apps/api/tests/broker/outboxWorker.test.ts + + - apps/api/tests/broker/outboxWorker.test.ts (full file: the `vi.hoisted` + `vi.mock` scaffold, `mockPendingRows`, `makeRow`, `makeResponse`, `wireMockChain`, the `beforeEach` reset block, and the existing describe blocks — extend, do not rewrite) + - apps/api/src/lib/outboxTrigger.ts (created in Task 1 — import `signalOutboxDrain` from here) + - apps/api/src/broker/outboxWorker.ts (the module under test — `runOutboxDrain` already exported; `scheduleOutboxDrain` will be added in Task 3 and must be imported here in RED so the test fails on the missing export) + - apps/api/tests/lib/listEmitter.test.ts (analog for `vi.fn()` handler + unsubscribe test shape) + + + - Test A (SC-1): with one pending row and `createCalendarEvent` mocked to resolve 201, calling `signalOutboxDrain()` then flushing microtasks via `await new Promise(r => setImmediate(r))` results in `createCalendarEvent` called exactly once — with NO `vi.useFakeTimers()` / no 15s advance + - Test B (SC-4 / D-05): with `createCalendarEvent` first call held on a manually-resolved promise and two pending rows, calling `signalOutboxDrain()` (drain 1 starts), then `signalOutboxDrain()` twice more while drain 1 is in flight, then releasing drain 1 and flushing, results in exactly one trailing re-drain — `createCalendarEvent` total calls equal (rows in drain 1) + (rows in trailing drain), never a third drain pass + - Test C (SC-4 / D-07): with `createCalendarEvent` mocked to a `setTimeout(20ms)`-delayed resolve and one pending row, calling `scheduleOutboxDrain()` twice synchronously then `await vi.runAllTimersAsync()` results in `createCalendarEvent` called exactly once (second concurrent call no-ops via `isDraining`) + + + Extend `apps/api/tests/broker/outboxWorker.test.ts`. Update the top import to add `scheduleOutboxDrain` to the existing `import { runOutboxDrain, assembleRruleString } from '../../src/broker/outboxWorker.js';` line, and add `import { signalOutboxDrain } from '../../src/lib/outboxTrigger.js';`. Reuse the existing module-scope helpers (`mockPendingRows`, `makeRow`, `makeResponse`, `wireMockChain`) and the `createCalendarEvent` mock from `../../src/broker/write.js` — do NOT redeclare them. Add ONE new describe block after the last existing describe block: `describe('scheduleOutboxDrain — trigger wiring (D-09)', ...)`. Inside, replicate the existing `beforeEach` (`vi.resetAllMocks(); mockPendingRows = []; wireMockChain();`). Because `isDraining`/`drainRequested` are module-level flags that persist across tests in the same module instance, end each test that leaves a drain potentially in flight by awaiting two `setImmediate` flushes so the module flags settle before the next test. Write Test A, Test B, Test C per the `` block. For Test A and Test B use `await new Promise(resolve => setImmediate(resolve))` for microtask flushing (the signal→drain path is microtask-driven, no timers — per RESEARCH Q5); for Test B use `vi.mocked(createCalendarEvent).mockImplementationOnce(async () => { await firstDone; return makeResponse(201); }).mockResolvedValue(makeResponse(201))` with a manually-captured `resolveFirst`. For Test C use `vi.useFakeTimers()` + `vi.runAllTimersAsync()` and restore real timers in that test's cleanup. Assert exact call counts per ``. This task is RED: it imports `scheduleOutboxDrain` which does not exist yet, so the file fails to resolve / the new tests fail. Do NOT implement `scheduleOutboxDrain` in this task. Do NOT touch the existing 15 tests. + + + cd apps/api && npx vitest run tests/broker/outboxWorker.test.ts 2>&1 | tail -20; echo "EXPECT: RED — new 'trigger wiring' tests fail or module fails to resolve scheduleOutboxDrain" + + + - The new `describe('scheduleOutboxDrain — trigger wiring (D-09)', ...)` block exists with three tests (Test A SC-1, Test B D-05, Test C D-07) + - The test file imports `scheduleOutboxDrain` from `../../src/broker/outboxWorker.js` and `signalOutboxDrain` from `../../src/lib/outboxTrigger.js` + - Running `npx vitest run tests/broker/outboxWorker.test.ts` shows the new trigger-wiring tests RED (fail) — confirming the test exercises behavior not yet implemented + - The 15 pre-existing tests are unmodified (no edits inside their describe blocks) + + Three new trigger-wiring tests written and failing (RED); existing 15 tests untouched. + + + + Task 3: GREEN — add scheduleOutboxDrain wrapper, drainRequested flag, initOutboxTrigger; switch setInterval to the wrapper + apps/api/src/broker/outboxWorker.ts + + - apps/api/src/broker/outboxWorker.ts (full file — confirm: `let isDraining = false;` at L159; `runOutboxDrain` guard `if (isDraining) return; isDraining = true;` at L602–605; `finally { isDraining = false; }` at L783–785; `startOutboxWorker` setInterval calling `runOutboxDrain().catch(...)` at L796–802; the file-header export-rationale comment at L19–22) + - apps/api/src/lib/outboxTrigger.ts (import `onOutboxDrain` from here for `initOutboxTrigger`) + - apps/api/tests/broker/outboxWorker.test.ts (the RED tests from Task 2 that this task turns GREEN) + + + Modify `apps/api/src/broker/outboxWorker.ts`. (1) Add `import { onOutboxDrain } from '../lib/outboxTrigger.js';` with the other imports. (2) Immediately after `let isDraining = false;` (L159) add `let drainRequested = false;`. (3) Add and EXPORT a new `scheduleOutboxDrain(): void` function (place it between the `isDraining`/`drainRequested` declarations and `runOutboxDrain`): if `isDraining` is true, set `drainRequested = true` and return; otherwise call `runOutboxDrain()` and chain `.catch((err: unknown) => console.error('[outboxWorker] Unhandled runOutboxDrain error:', err))` then `.finally(() => { if (drainRequested) { drainRequested = false; scheduleOutboxDrain(); } })`. The `drainRequested = false` reset MUST come BEFORE the recursive `scheduleOutboxDrain()` call (Pitfall 3 — resetting after would allow an infinite loop). (4) Do NOT modify `runOutboxDrain`'s body or its `isDraining` guard/`finally` in any way (preserves the existing 15 tests and D-02/D-07). (5) Add and EXPORT `initOutboxTrigger(): void` that calls `onOutboxDrain(() => scheduleOutboxDrain())` — placed alongside `startOutboxWorker` in the scheduler section. (6) Update `startOutboxWorker`'s `setInterval` body to call `scheduleOutboxDrain()` instead of `runOutboxDrain().catch(...)` — the per-call `.catch` is now absorbed into `scheduleOutboxDrain`, so the interval body becomes a bare `scheduleOutboxDrain();` call (keep the 15 * 1000 interval exactly per D-08). (7) Extend the file-header export-rationale comment (L19–22) with a line: `scheduleOutboxDrain is exported for unit testing; it wraps runOutboxDrain with the isDraining guard + drainRequested trailing-re-drain loop (D-05).` This task is GREEN: the Task 2 tests must now pass. + + + cd apps/api && npx vitest run tests/broker/outboxWorker.test.ts 2>&1 | tail -8; npx tsc --noEmit 2>&1 | grep -E 'outboxWorker|outboxTrigger' || echo "tsc clean for outbox files"; grep -q "export function scheduleOutboxDrain" src/broker/outboxWorker.ts && grep -q "export function initOutboxTrigger" src/broker/outboxWorker.ts && grep -q "let drainRequested = false" src/broker/outboxWorker.ts && echo OK + + + - `outboxWorker.ts` exports `scheduleOutboxDrain` and `initOutboxTrigger`, and declares `let drainRequested = false` + - `runOutboxDrain`'s guard line `if (isDraining) return;`, `isDraining = true;`, and `finally { isDraining = false; }` are byte-for-byte unchanged from before this plan (the trailing re-drain lives only in `scheduleOutboxDrain`, never inside `runOutboxDrain`) + - In `scheduleOutboxDrain`'s `.finally`, `drainRequested = false` precedes the recursive `scheduleOutboxDrain()` call (Pitfall 3) + - `startOutboxWorker`'s `setInterval` body calls `scheduleOutboxDrain()` and keeps the `15 * 1000` interval (D-08) + - `initOutboxTrigger` calls `onOutboxDrain(() => scheduleOutboxDrain())` + - `npx vitest run tests/broker/outboxWorker.test.ts` is GREEN (all trigger-wiring tests pass AND the 15 pre-existing tests still pass) + - `cd apps/api && npx tsc --noEmit` reports no errors in `outboxWorker.ts` / `outboxTrigger.ts` + + scheduleOutboxDrain + drainRequested + initOutboxTrigger added; setInterval routes through the wrapper; runOutboxDrain internals unchanged; full outboxWorker.test.ts suite GREEN; tsc clean. + + + + + +## Trust Boundaries + +| Boundary | Description | +|----------|-------------| +| route handler → in-process EventEmitter | A committed enqueue publishes a `'drain'` signal in-process. No network surface, no new auth boundary, no new user input crosses here — the signal carries no payload. | +| EventEmitter listener → runOutboxDrain | The single subscriber funnels the signal into the existing `isDraining`-guarded drain. Existing trust boundaries (CalDAV credential decryption, Fastmail I/O) are unchanged downstream. | + +## STRIDE Threat Register + +| Threat ID | Category | Component | Disposition | Mitigation Plan | +|-----------|----------|-----------|-------------|-----------------| +| T-09-01 | Denial of Service | `scheduleOutboxDrain` trailing-re-drain loop | mitigate | `drainRequested` is a boolean that collapses all mid-drain signals into exactly one trailing drain (D-05/D-06); it is reset BEFORE the recursive call (Pitfall 3), so a burst can never produce an unbounded re-drain chain against Fastmail rate limits. Test B (D-05) asserts exactly one trailing drain. | +| T-09-02 | Denial of Service | listener registration in test process | mitigate | `onOutboxDrain` is invoked only from `initOutboxTrigger`, which (in Plan 02) 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. Single subscriber → default 10-listener limit suffices, no `setMaxListeners`. | +| T-09-03 | Denial of Service | error in drain escaping the wrapper | mitigate | `scheduleOutboxDrain` wraps `runOutboxDrain()` in `.catch(...)` that logs and swallows, matching the existing setInterval error-swallowing pattern (D-02). A thrown drain error cannot crash the process or escape into the EventEmitter `emit` call frame. | +| T-09-04 | Tampering | signal-induced reentrancy bypassing isDraining | accept→mitigate | The signal NEVER calls `runOutboxDrain()` directly; it always goes through `scheduleOutboxDrain`, which checks `isDraining` before dispatch (D-02). Test C (D-07) asserts concurrent `scheduleOutboxDrain` calls dispatch each row exactly once. No new tampering vector — the guard is reused verbatim. | + +No `npm`/`pip`/`cargo` install in this plan (zero-dependency, `node:events` only) — no T-09-SC supply-chain threat applies. RESEARCH Package Legitimacy Audit confirms no new external packages. + + + +- `cd apps/api && npx vitest run tests/broker/outboxWorker.test.ts` — full broker suite GREEN (15 existing + 3 new trigger-wiring tests) +- `cd apps/api && npx tsc --noEmit` — no type errors (Vitest passes while tsc fails; typecheck is mandatory per [[vitest-passes-tsc-fails]]) +- `grep -q "export function scheduleOutboxDrain" apps/api/src/broker/outboxWorker.ts` — wrapper exported +- `runOutboxDrain` guard/finally unchanged: diff the `runOutboxDrain` body region against the pre-plan version — no edits inside it + + + +- outboxTrigger.ts created (zero-dependency, node:events only, single subscriber, no setMaxListeners) +- scheduleOutboxDrain implements the isDraining-guarded drain with a drainRequested trailing-re-drain loop (D-05), errors caught (D-02), exactly-once preserved (D-07) +- runOutboxDrain internals + isDraining guard behaviorally unchanged (existing 15 tests still pass) +- 15s setInterval routes through scheduleOutboxDrain, interval length unchanged (D-08) +- Trigger-wiring tests for SC-1, D-05, D-07 pass (D-09) +- tsc --noEmit clean + + + +Create `.planning/phases/09-faster-write-back/09-01-SUMMARY.md` when done. + diff --git a/.planning/phases/09-faster-write-back/09-01-SUMMARY.md b/.planning/phases/09-faster-write-back/09-01-SUMMARY.md new file mode 100644 index 0000000..8992d68 --- /dev/null +++ b/.planning/phases/09-faster-write-back/09-01-SUMMARY.md @@ -0,0 +1,125 @@ +--- +phase: 09-faster-write-back +plan: "01" +subsystem: api-broker +tags: [outbox, event-driven, tdd, drain-trigger, caldav] +dependency_graph: + requires: [] + provides: + - signalOutboxDrain (apps/api/src/lib/outboxTrigger.ts) + - onOutboxDrain (apps/api/src/lib/outboxTrigger.ts) + - scheduleOutboxDrain (apps/api/src/broker/outboxWorker.ts) + - drainRequested (apps/api/src/broker/outboxWorker.ts) + - initOutboxTrigger (apps/api/src/broker/outboxWorker.ts) + affects: + - apps/api/src/broker/outboxWorker.ts + - apps/api/tests/broker/outboxWorker.test.ts +tech_stack: + added: + - node:events EventEmitter (outboxTrigger.ts — zero external dependency) + patterns: + - Module-level EventEmitter singleton (same as listEmitter.ts) + - scheduleOutboxDrain isDraining guard + drainRequested trailing-re-drain loop (D-05) + - TDD RED/GREEN via test(09-01)/feat(09-01) commits +key_files: + created: + - apps/api/src/lib/outboxTrigger.ts + modified: + - apps/api/src/broker/outboxWorker.ts + - apps/api/tests/broker/outboxWorker.test.ts +decisions: + - "D-TDD-INIT: initOutboxTrigger() called in beforeAll (not beforeEach) for the trigger-wiring describe block — avoids listener accumulation while still wiring the EventEmitter → scheduleOutboxDrain path for signal-driven tests (SC-1, D-05)" + - "D-TEST-C-MOCK: Test C (D-07) uses mockImplementationOnce to return the row on the first pending-rows query and empty on subsequent queries — correctly simulates drain 1 processing the row so the trailing re-drain (triggered by drainRequested) finds 0 rows and calls createCalendarEvent exactly once" +metrics: + duration_seconds: 341 + completed_date: "2026-06-12" + tasks_completed: 3 + files_changed: 3 +--- + +# Phase 09 Plan 01: Outbox Drain Trigger Wiring Summary + +**One-liner:** Zero-dependency in-process EventEmitter drain signal (`outboxTrigger.ts`) + `scheduleOutboxDrain` wrapper with `drainRequested` trailing-re-drain loop wired to `outboxWorker.ts`, eliminating the up-to-15s polling delay on enqueue. + +## What Was Built + +### Task 1: `apps/api/src/lib/outboxTrigger.ts` (new file) + +Module-level `EventEmitter` singleton following the `listEmitter.ts` analog: +- `signalOutboxDrain(): void` — `emitter.emit('drain')`, fire-and-forget (D-04) +- `onOutboxDrain(handler: () => void): () => void` — registers listener, returns unsubscribe +- Only imports `node:events`; no internal dependencies (zero circular-import risk) +- No `setMaxListeners` call — single subscriber, default limit of 10 is correct + +### Task 2: RED test block in `apps/api/tests/broker/outboxWorker.test.ts` + +Added `describe('scheduleOutboxDrain — trigger wiring (D-09)', ...)` with 3 failing tests: +- Test A (SC-1): `signalOutboxDrain()` triggers drain promptly without timer advance +- Test B (D-05): two mid-drain signals collapse to exactly one trailing re-drain +- Test C (D-07): two concurrent `scheduleOutboxDrain()` calls invoke `createCalendarEvent` exactly once + +Tests failed RED because `scheduleOutboxDrain` and `initOutboxTrigger` were not yet exported. + +### Task 3: GREEN implementation in `apps/api/src/broker/outboxWorker.ts` + +Four additions to `outboxWorker.ts`: +1. `import { onOutboxDrain } from '../lib/outboxTrigger.js'` +2. `let drainRequested = false` — trailing-re-drain flag (D-05), immediately after `isDraining` +3. `export function scheduleOutboxDrain(): void` — checks `isDraining`; if true sets `drainRequested = true` and returns; otherwise calls `runOutboxDrain()` with `.catch` (D-02) and `.finally` that resets `drainRequested = false` BEFORE any recursive `scheduleOutboxDrain()` call (Pitfall 3 / T-09-01) +4. `export function initOutboxTrigger(): void` — calls `onOutboxDrain(() => scheduleOutboxDrain())` +5. `startOutboxWorker` setInterval body changed from `runOutboxDrain().catch(...)` to bare `scheduleOutboxDrain()` — 15s interval unchanged (D-08) + +`runOutboxDrain`'s body, `if (isDraining) return;`, `isDraining = true;`, and `finally { isDraining = false; }` are byte-for-byte unchanged. + +## Deviations from Plan + +### Auto-fixed Issues + +**1. [Rule 1 - Test Design] Test C mock required mockImplementationOnce to correctly simulate drain-1 row consumption** + +- **Found during:** Task 3 GREEN +- **Issue:** Test C expects `createCalendarEvent` called exactly once, but the trailing re-drain triggered by `drainRequested` also executed against the same always-returning mock, calling `createCalendarEvent` twice. +- **Fix:** Used `mockWherePending.mockImplementationOnce(() => Promise.resolve([row])).mockImplementation(() => Promise.resolve([]))` so drain 1 gets the row and the trailing drain finds empty. +- **Files modified:** `apps/api/tests/broker/outboxWorker.test.ts` +- **Commit:** `2b11304` + +**2. [Rule 2 - Missing test wiring] `beforeAll(initOutboxTrigger)` required to wire EventEmitter listener for signal-driven tests** + +- **Found during:** Task 3 GREEN (Tests A and B failed because no listener was registered) +- **Issue:** Tests A and B call `signalOutboxDrain()` but without `initOutboxTrigger()` registering the listener, the signal went nowhere. +- **Fix:** Added `beforeAll(() => { initOutboxTrigger(); })` to the new describe block; also imported `initOutboxTrigger` and changed `import { beforeEach }` to include `beforeAll`. +- **Files modified:** `apps/api/tests/broker/outboxWorker.test.ts` +- **Commit:** `2b11304` + +## TDD Gate Compliance + +- RED commit: `bcde073` — `test(09-01): add failing trigger-wiring tests for SC-1, D-05, D-07` +- GREEN commit: `2b11304` — `feat(09-01): add scheduleOutboxDrain, drainRequested, initOutboxTrigger; route setInterval through wrapper` +- RED gate: 3 new tests failing (27 pre-existing passing) +- GREEN gate: 30/30 tests passing, `tsc --noEmit` clean + +## Verification Evidence + +``` +npx vitest run tests/broker/outboxWorker.test.ts + Test Files 1 passed (1) + Tests 30 passed (30) + +npx tsc --noEmit → (no output, clean) +``` + +## Known Stubs + +None — all symbols produce correct runtime behavior. `signalOutboxDrain` is not yet wired to the enqueue path (Plan 02 adds it to `events.ts`); `initOutboxTrigger` is not yet called at startup (Plan 02 adds it to `index.ts`). These are intentional plan boundaries, not stubs. + +## Threat Flags + +None — this plan introduces no new network endpoints, auth paths, file access patterns, or schema changes. The in-process EventEmitter boundary carries no payload and no user input crosses it. STRIDE mitigations T-09-01 through T-09-04 are implemented and verified by the trigger-wiring tests. + +## Self-Check: PASSED + +- `apps/api/src/lib/outboxTrigger.ts` — FOUND +- `apps/api/src/broker/outboxWorker.ts` — verified: `scheduleOutboxDrain`, `initOutboxTrigger`, `drainRequested` present +- Task 1 commit `1e12d70` — FOUND +- Task 2 commit `bcde073` — FOUND +- Task 3 commit `2b11304` — FOUND diff --git a/.planning/phases/09-faster-write-back/09-02-PLAN.md b/.planning/phases/09-faster-write-back/09-02-PLAN.md new file mode 100644 index 0000000..04c7296 --- /dev/null +++ b/.planning/phases/09-faster-write-back/09-02-PLAN.md @@ -0,0 +1,165 @@ +--- +phase: 09-faster-write-back +plan: 02 +type: execute +wave: 2 +depends_on: ["09-01"] +files_modified: + - apps/api/src/routes/events.ts + - apps/api/src/index.ts +autonomous: true +requirements: [CAL-15] + +must_haves: + truths: + - "Creating an event publishes the drain signal after the outbox insert commits, then returns 202 (SC-1, SC-2)" + - "Editing an event (same-calendar update) publishes the signal after the insert commits, then returns 202" + - "Edit-as-move publishes the signal AFTER the db.transaction resolves — never between the DELETE and CREATE inserts (SC-3 / D-03)" + - "Deleting an event publishes the signal after the insert commits, then returns 202" + - "No route handler makes an inline CalDAV call; signalOutboxDrain() is fire-and-forget, never awaited (SC-2 / D-04)" + - "The drain-signal listener is wired at startup only under isMainModule(), after startOutboxWorker (SC-5)" + artifacts: + - path: "apps/api/src/routes/events.ts" + provides: "Four post-commit signalOutboxDrain() publish sites (create, edit-as-move, same-cal update, delete)" + contains: "signalOutboxDrain" + - path: "apps/api/src/index.ts" + provides: "initOutboxTrigger() wiring under isMainModule(), after startOutboxWorker()" + contains: "initOutboxTrigger" + key_links: + - from: "apps/api/src/routes/events.ts" + to: "apps/api/src/lib/outboxTrigger.ts (signalOutboxDrain)" + via: "import + call after each enqueue commit" + pattern: "signalOutboxDrain\\(\\)" + - from: "apps/api/src/index.ts (isMainModule block)" + to: "apps/api/src/broker/outboxWorker.ts (initOutboxTrigger)" + via: "initOutboxTrigger() after startOutboxWorker()" + pattern: "initOutboxTrigger\\(\\)" +--- + + +Wire the drain signal into the live request and startup paths: publish `signalOutboxDrain()` after each of the four enqueue commits in `events.ts` (create, edit-as-move, same-calendar update, delete) and subscribe the drain listener at startup in `index.ts` via `initOutboxTrigger()` under `isMainModule()`. + +Purpose: Connects the Plan 01 signal mechanism to real writes (so edits land in ~1-2s — SC-1) and to process startup (so the listener is active in production but never in the test process — SC-5), while keeping the optimistic-202 / no-inline-CalDAV route contract intact (SC-2 / D-04) and preserving create-before-delete ordering for moves (SC-3 / D-03). +Output: Four publish-site edits in `events.ts`, the `initOutboxTrigger()` startup call + import update in `index.ts`. + + + +@$HOME/.claude/gsd-core/workflows/execute-plan.md +@$HOME/.claude/gsd-core/templates/summary.md + + + +@.planning/PROJECT.md +@.planning/ROADMAP.md +@.planning/STATE.md +@.planning/phases/09-faster-write-back/09-CONTEXT.md +@.planning/phases/09-faster-write-back/09-RESEARCH.md +@.planning/phases/09-faster-write-back/09-PATTERNS.md +@.planning/phases/09-faster-write-back/09-01-SUMMARY.md + + + +This plan creates NO new symbols — it consumes symbols created by Plan 01: + +| Consumed Symbol | Source File (Plan 01) | Consumed In | +|-----------------|----------------------|-------------| +| `signalOutboxDrain` | `apps/api/src/lib/outboxTrigger.ts` | `apps/api/src/routes/events.ts` (4 call sites) | +| `initOutboxTrigger` | `apps/api/src/broker/outboxWorker.ts` | `apps/api/src/index.ts` (startup wiring) | + +Both symbols MUST already exist (Plan 01, Wave 1) before this plan runs — enforced by `depends_on: ["09-01"]`. + + + + + + Task 1: Publish signalOutboxDrain() after each enqueue commit in events.ts (4 sites) + apps/api/src/routes/events.ts + + - apps/api/src/routes/events.ts (full file — confirm the four enqueue sites and their commit boundaries: Site 1 POST /create `await db.insert(calendarOutbox)` at ~L300 then `return c.json({ uid }, 202)` at L309; Site 2 PATCH /:uid/edit edit-as-move `await db.transaction(...)` at L408–432 then `return c.json({ uid: newUid }, 202)` at L432; Site 3 PATCH /:uid/edit same-calendar update `await db.insert(calendarOutbox)` at ~L436 then `return c.json({ uid }, 202)` at L447; Site 4 DELETE /:uid `await db.insert(calendarOutbox)` at ~L511 then `return c.json({ uid }, 202)` at L521. All four sites are inside existing try/catch blocks.) + - apps/api/src/lib/outboxTrigger.ts (Plan 01 — import `signalOutboxDrain` from here) + + + Modify `apps/api/src/routes/events.ts`. (1) Add `import { signalOutboxDrain } from '../lib/outboxTrigger.js';` with the existing imports. (2) Insert a fire-and-forget `signalOutboxDrain();` call (NOT awaited — D-04) immediately AFTER the awaited enqueue and BEFORE the `return c.json(..., 202)` at each of the four sites: + - Site 1 (POST /create): after `await db.insert(calendarOutbox).values({...})` (~L300–308), before `return c.json({ uid }, 202)` (L309). + - Site 2 (PATCH /:uid/edit, edit-as-move): after `await db.transaction(async (tx) => {...})` RESOLVES (L408–432) — the call goes on the line AFTER the `await db.transaction(...)` statement, NOT inside the transaction callback. This is the critical placement (D-03 / Pitfall 6 / Pitfall 1): publishing inside the callback would fire before the DELETE+CREATE rows are durably committed. Place it before `return c.json({ uid: newUid }, 202)` (L432). + - Site 3 (PATCH /:uid/edit, same-calendar update): after `await db.insert(calendarOutbox).values({ operation: 'update', ... })` (~L436–445), before `return c.json({ uid }, 202)` (L447). + - Site 4 (DELETE /:uid): after `await db.insert(calendarOutbox).values({ operation: 'delete', ... })` (~L511–519), before `return c.json({ uid }, 202)` (L521). + Do NOT change any route's status code, response body, validation, ownership checks, or the no-inline-CalDAV behavior — the only change is adding the post-commit signal call at each site. Do NOT await the signal. Do NOT add a signal inside the `db.transaction` callback or in any `catch` block. + + + cd apps/api && grep -c "signalOutboxDrain()" src/routes/events.ts | grep -qx 4 && echo "4 signal sites" && ! grep -Pzo "db\.transaction\(async[^)]*\)\s*=>\s*\{[^}]*signalOutboxDrain" src/routes/events.ts && echo "no signal inside transaction callback" && npx tsc --noEmit 2>&1 | grep -E 'events\.ts' || echo "tsc clean for events.ts" + + + - `events.ts` imports `signalOutboxDrain` from `'../lib/outboxTrigger.js'` + - Exactly four `signalOutboxDrain()` calls exist (`grep -c "signalOutboxDrain()" src/routes/events.ts` == 4), one per enqueue site + - At Site 2 the `signalOutboxDrain()` call appears AFTER the `await db.transaction(...)` statement, not inside its callback (no `signalOutboxDrain` between the two `tx.insert` calls) + - No `signalOutboxDrain()` is awaited (`grep -n "await signalOutboxDrain" src/routes/events.ts` returns nothing — D-04 fire-and-forget) + - No route handler gained an inline CalDAV call; status codes and bodies unchanged (still 202 with `{ uid }` / `{ uid: newUid }`) + - `cd apps/api && npx tsc --noEmit` reports no errors in `events.ts` + + Four post-commit signal sites added; edit-as-move signal fires after the transaction commits; no awaited signal, no inline CalDAV; tsc clean. + + + + Task 2: Wire initOutboxTrigger() at startup under isMainModule() + apps/api/src/index.ts + + - apps/api/src/index.ts (full file — confirm: import `import { startOutboxWorker } from './broker/outboxWorker.js';` at L16; `isMainModule()` function at L100–107; the `if (isMainModule())` startup block at L112 with `startBrokerPoller()` (L136), `startOutboxWorker()` (L138), `startReminderScheduler()` (L141)) + - apps/api/src/broker/outboxWorker.ts (Plan 01 — `initOutboxTrigger` is exported from this module alongside `startOutboxWorker`) + + + Modify `apps/api/src/index.ts`. (1) Update the existing import at L16 from `import { startOutboxWorker } from './broker/outboxWorker.js';` to also import `initOutboxTrigger`: `import { startOutboxWorker, initOutboxTrigger } from './broker/outboxWorker.js';`. (2) Inside the existing `if (isMainModule())` block, add `initOutboxTrigger();` on the line immediately AFTER `startOutboxWorker();` (L138) — so the fallback interval is registered before the signal listener subscribes (logical ordering; both are synchronous). Add an inline comment: `// subscribe drain signal listener (D-01)`. Do NOT add the call outside the `isMainModule()` guard — the listener MUST be gated so tests that import `app` never register it (Pitfall 4 — listener/timer leakage in tests). Do NOT modify `isMainModule()` itself, `startBrokerPoller`, `startReminderScheduler`, or the `serve(...)` call. + + + cd apps/api && grep -q "import { startOutboxWorker, initOutboxTrigger }" src/index.ts && grep -q "initOutboxTrigger();" src/index.ts && awk '/if \(isMainModule\(\)\)/{f=1} f&&/initOutboxTrigger\(\)/{print "inside guard"; exit}' src/index.ts | grep -q "inside guard" && echo "guarded" && npx tsc --noEmit 2>&1 | grep -E 'index\.ts' || echo "tsc clean for index.ts" + + + - `index.ts` imports `initOutboxTrigger` from `'./broker/outboxWorker.js'` (same import statement as `startOutboxWorker`) + - `initOutboxTrigger();` appears inside the `if (isMainModule())` block, on the line after `startOutboxWorker();` + - No `initOutboxTrigger()` call exists outside the `isMainModule()` guard (Pitfall 4) + - `isMainModule()`, `startBrokerPoller`, `startReminderScheduler`, and `serve(...)` are unchanged + - `cd apps/api && npx tsc --noEmit` reports no errors in `index.ts` + + initOutboxTrigger() wired after startOutboxWorker() inside the isMainModule() guard; import updated; tsc clean. + + + + + +## Trust Boundaries + +| Boundary | Description | +|----------|-------------| +| client → route handler | Unchanged — same Zod validation, ownership checks, and optimistic-202 contract. This plan adds no new input, no new endpoint, no new status path. | +| route handler → in-process EventEmitter | A committed enqueue calls `signalOutboxDrain()` in-process. No payload, no network, fire-and-forget. | +| process startup → EventEmitter listener | `initOutboxTrigger()` registers the single drain listener only under `isMainModule()`. The test process never crosses this boundary. | + +## STRIDE Threat Register + +| Threat ID | Category | Component | Disposition | Mitigation Plan | +|-----------|----------|-----------|-------------|-----------------| +| T-09-05 | Tampering | edit-as-move signal placement | mitigate | `signalOutboxDrain()` is published AFTER `await db.transaction(...)` resolves, never inside the callback (D-03 / Pitfall 1) — so the DELETE+CREATE rows are durably committed before any drain can observe them; create-before-delete ordering (SC-3) is never raced. Verify step asserts no signal inside the transaction callback. | +| T-09-06 | Denial of Service | listener registered in test process | mitigate | `initOutboxTrigger()` is called only inside `if (isMainModule())` (Pitfall 4) — tests importing `app` never register the listener, so no spurious cross-test drains and no open handle preventing process exit. Verify step asserts the call is inside the guard. | +| T-09-07 | Denial of Service | fire-and-forget signal blocking the request | accept | `signalOutboxDrain()` is synchronous and emits to a single in-process listener that returns immediately (the listener calls `scheduleOutboxDrain` which is itself fire-and-forget); it cannot block the 202 response. No CalDAV I/O happens inline (SC-2 / D-04). Low risk, no mitigation needed beyond not awaiting the call. | + +No `npm`/`pip`/`cargo` install in this plan — no supply-chain (T-09-SC) threat. No new external package (RESEARCH Package Legitimacy Audit: none introduced). + + + +- `cd apps/api && npx vitest run` — FULL suite GREEN (the existing events.ts route tests confirm 202 + no inline CalDAV still hold after the signal additions; the Plan 01 trigger-wiring tests still pass) +- `cd apps/api && npx tsc --noEmit` — no type errors in `events.ts` or `index.ts` (mandatory — Vitest passes while tsc fails per [[vitest-passes-tsc-fails]]) +- `grep -c "signalOutboxDrain()" apps/api/src/routes/events.ts` == 4 — all four publish sites present +- `initOutboxTrigger()` is inside the `isMainModule()` block (Pitfall 4) + + + +- Four post-commit `signalOutboxDrain()` publish sites in events.ts (create, edit-as-move-after-transaction, same-cal update, delete) — SC-1 +- Route handlers still return optimistic 202 with no inline CalDAV; signal is fire-and-forget — SC-2 / D-04 +- Edit-as-move signal fires after the transaction commits, preserving create-before-delete ordering — SC-3 / D-03 +- initOutboxTrigger() wired under isMainModule() after startOutboxWorker(); fallback interval still active — SC-5 +- Full Vitest suite GREEN + tsc --noEmit clean + + + +Create `.planning/phases/09-faster-write-back/09-02-SUMMARY.md` when done. + diff --git a/.planning/phases/09-faster-write-back/09-02-SUMMARY.md b/.planning/phases/09-faster-write-back/09-02-SUMMARY.md new file mode 100644 index 0000000..eb53bea --- /dev/null +++ b/.planning/phases/09-faster-write-back/09-02-SUMMARY.md @@ -0,0 +1,100 @@ +--- +phase: 09-faster-write-back +plan: "02" +subsystem: api-routes-startup +tags: [outbox, event-driven, drain-signal, caldav, startup-wiring] +dependency_graph: + requires: + - signalOutboxDrain (apps/api/src/lib/outboxTrigger.ts — Plan 01) + - initOutboxTrigger (apps/api/src/broker/outboxWorker.ts — Plan 01) + provides: + - Four post-commit signalOutboxDrain() publish sites in events.ts (create, edit-as-move, same-cal update, delete) + - initOutboxTrigger() wired at startup in index.ts under isMainModule() + affects: + - apps/api/src/routes/events.ts + - apps/api/src/index.ts +tech_stack: + added: [] + patterns: + - Fire-and-forget in-process EventEmitter signal after each outbox enqueue commit (D-04) + - isMainModule() guard for startup-only listener registration (Pitfall 4) + - edit-as-move signal fires after await db.transaction() resolves, not inside callback (D-03) +key_files: + created: [] + modified: + - apps/api/src/routes/events.ts + - apps/api/src/index.ts +decisions: + - "D-03-signal-placement: signalOutboxDrain() for edit-as-move placed AFTER await db.transaction() resolves (line after the statement), never inside the async callback — guarantees DELETE+CREATE rows are durably committed before any drain observes them (SC-3 / Pitfall 1)" + - "D-04-fire-and-forget: signalOutboxDrain() is never awaited — synchronous emit to in-process EventEmitter, cannot block the 202 response (SC-2 / T-09-07)" + - "Pitfall4-guard: initOutboxTrigger() called only inside isMainModule() block — tests importing app never register the drain listener, no open handles in test process (T-09-06)" +metrics: + duration_seconds: 240 + completed_date: "2026-06-12" + tasks_completed: 2 + files_changed: 2 +--- + +# Phase 09 Plan 02: Enqueue-Site Signal Wiring Summary + +**One-liner:** Four fire-and-forget `signalOutboxDrain()` publish sites added to events.ts (create, edit-as-move-after-transaction, same-cal update, delete) and `initOutboxTrigger()` wired at startup in index.ts under `isMainModule()`, connecting the Plan 01 EventEmitter mechanism to live writes and eliminating the up-to-15s polling delay. + +## What Was Built + +### Task 1: `apps/api/src/routes/events.ts` — Four publish sites + +Import added: `import { signalOutboxDrain } from '../lib/outboxTrigger.js';` + +Four `signalOutboxDrain()` calls inserted, each fire-and-forget (never awaited): + +| Site | Route | Placement | +|------|-------|-----------| +| 1 | POST /create | After `await db.insert(calendarOutbox).values({...})`, before `return c.json({ uid }, 202)` | +| 2 | PATCH /:uid/edit (edit-as-move) | After `await db.transaction(async (tx) => {...})` RESOLVES, before `return c.json({ uid: newUid }, 202)` — critical: NOT inside the callback | +| 3 | PATCH /:uid/edit (same-cal update) | After `await db.insert(calendarOutbox).values({ operation: 'update', ... })`, before `return c.json({ uid }, 202)` | +| 4 | DELETE /:uid | After `await db.insert(calendarOutbox).values({ operation: 'delete', ... })`, before `return c.json({ uid }, 202)` | + +No route status codes, response bodies, validation, or ownership checks were modified. No inline CalDAV calls added. No signal inside the `db.transaction` callback. No signal inside any `catch` block. + +### Task 2: `apps/api/src/index.ts` — Startup wiring + +- Import extended: `import { startOutboxWorker, initOutboxTrigger } from './broker/outboxWorker.js';` +- `initOutboxTrigger();` added immediately after `startOutboxWorker();` inside the `if (isMainModule())` block (line 139), with comment `// subscribe drain signal listener (D-01)` +- `isMainModule()`, `startBrokerPoller`, `startReminderScheduler`, and `serve(...)` unchanged + +## Deviations from Plan + +None — plan executed exactly as written. + +## Verification Evidence + +``` +grep -c "signalOutboxDrain()" apps/api/src/routes/events.ts → 4 +grep -n "await signalOutboxDrain" apps/api/src/routes/events.ts → (none) +initOutboxTrigger() at line 139 — inside isMainModule() guard + +npx tsc --noEmit → (no output, clean) + +npx vitest run tests/routes/events.test.ts tests/broker/outboxWorker.test.ts + Test Files 2 passed (2) + Tests 53 passed (53) + +Full suite: 240 passed, 1 failed (known pre-existing flaky timeout in lists.test.ts: + "toggling isShared false→true re-populates list_shares for other members" — 5028ms, + exceeds 5000ms global testTimeout; passes with --testTimeout=30000; unrelated to this plan) +``` + +## Known Stubs + +None. + +## Threat Flags + +None — this plan introduces no new network endpoints, auth paths, file access patterns, or schema changes. T-09-05 (edit-as-move signal placement) and T-09-06 (listener registration gating) are both verified: signal is after the transaction resolves, and `initOutboxTrigger` is inside `isMainModule()`. + +## Self-Check: PASSED + +- `apps/api/src/routes/events.ts` — FOUND; 4 signalOutboxDrain() calls verified +- `apps/api/src/index.ts` — FOUND; initOutboxTrigger() inside isMainModule() at line 139 verified +- Task 1 commit `30eff3d` — FOUND +- Task 2 commit `0ebdf48` — FOUND diff --git a/.planning/phases/09-faster-write-back/09-CONTEXT.md b/.planning/phases/09-faster-write-back/09-CONTEXT.md new file mode 100644 index 0000000..eb544fd --- /dev/null +++ b/.planning/phases/09-faster-write-back/09-CONTEXT.md @@ -0,0 +1,106 @@ +# Phase 9: Faster Write-Back - Context + +**Gathered:** 2026-06-12 +**Status:** Ready for planning + + +## Phase Boundary + +Make a created/edited/deleted event reach Fastmail in ~1-2s instead of waiting up to ~15s for the next interval tick, by signalling the outbox drain on enqueue (event-driven) — with every existing outbox durability guarantee intact. + +**In scope:** the in-process signal mechanism, its wiring into the existing drain path, and a trailing re-drain guarantee under bursts. + +**Out of scope (locked by roadmap — do NOT reopen):** +- Redis for the drain (Redis stays only for list SSE). Single-process, single Node module instance by design. +- node-cron (silently skipped ticks in the long-lived process — `setInterval` only). See `[[node-cron-skips-in-long-running-process]]`. +- Any change to the route handlers' optimistic-202 / no-inline-CalDAV contract. +- Any change to outbox durability semantics (fresh-etag-before-PUT, 412 conflict flow, per-uid exactly-once, edit-as-move create-before-delete, dead-letter/backoff). +- Multi-process / multi-replica row-claim (the `isDraining` declaration comment documents the future `UPDATE ... status='processing'` path; not this phase). + + + + +## Implementation Decisions + +### Signal mechanism (locked from ROADMAP.md — restated so the planner doesn't re-derive) +- **D-01:** New artifact is a single zero-dependency in-process `EventEmitter` at `apps/api/src/lib/outboxTrigger.ts`. No new npm dependency. +- **D-02:** The signal funnels through the **existing** `isDraining`-guarded `runOutboxDrain()` path via a `drainRequested` flag. The trigger must NEVER call `runOutboxDrain()` directly in a way that bypasses the `isDraining` guard or escapes the error-caught wrapper (Pitfall 5 — no double-drain). +- **D-03:** The signal is published **after** the enqueue transaction commits — never between the two inserts of an edit-as-move (Pitfall 6). For a move, publish once after both CREATE and DELETE rows are committed, so create-before-delete ordering is never raced by the signal. +- **D-04:** Route handlers still return optimistic 202 immediately and make no inline CalDAV call; the signal is fire-and-forget. + +### Burst behavior — trailing re-drain (the main decision this discussion locked) +- **D-05:** **Guarantee the last edit in a burst.** When a signal arrives while a drain is already in flight, set `drainRequested` and re-run the drain exactly once after the current one finishes (drain-again-if-requested loop), so the LAST edit in a rapid burst still lands in ~1-2s rather than waiting for the next 15s tick. +- **D-06:** No debounce/coalesce window and no rejection of mid-drain signals. Rationale: two-user household — burst volume is tiny; correctness/snappiness of the trailing edit outweighs Fastmail request-rate politeness. (A debounce knob was considered and rejected as premature.) +- **D-07:** Implementation must preserve exactly-once per uid when the signal and the 15s fallback overlap — the trailing re-drain reuses the same `pending AND next_attempt_at <= NOW()` selection and `isDraining` guard, so no duplicate CalDAV PUT for the same row (Success Criterion 4). + +### Fallback interval +- **D-08:** **Keep the 15s `setInterval` fallback exactly as-is.** It still runs for startup catch-up and transient-error recovery (Success Criterion 5). Do not lengthen it — idle DB polling cost is negligible for this deployment and the criterion names 15s explicitly. + +### Verification +- **D-09:** Prove Success Criterion 1 with an **automated integration test on the trigger wiring** (Vitest): assert that enqueuing a row publishes the signal and that `runOutboxDrain` is invoked promptly (stub/mock the CalDAV dispatch so no live Fastmail is needed). Also cover the trailing-re-drain case (signal during in-flight drain → exactly one follow-up drain) and the no-double-PUT overlap case. +- **D-10:** No operator-stopwatch human checkpoint is required for this phase. Rationale: the dev bypass user (id 1) has no CalDAV credential/calendars, so live event-create 422s in the dev stack — see `[[dev-data-user1-no-calendars]]`; an automated wiring test is the durable, CI-guarded evidence. (Real wall-clock-to-Fastmail latency is bounded by the CalDAV round-trip regardless and is implicitly exercised in production use.) + +### Claude's Discretion +- Exact shape of the `drainRequested`/re-drain loop (where the flag lives, whether the loop sits inside `runOutboxDrain`'s `finally` or in the scheduler wrapper) — planner/researcher decides, provided D-02/D-05/D-07 hold. +- Whether the EventEmitter is a module singleton vs. a tiny custom signal object — either is fine; zero-dep is the only hard constraint. +- Test file placement follows the existing convention (API tests live under `apps/api/tests/`, never `src/`) — see `[[api-integration-test-db]]`. + + + + +## Canonical References + +**Downstream agents MUST read these before planning or implementing.** + +### Phase scope & guarantees +- `.planning/ROADMAP.md` §"Phase 9: Faster Write-Back" — goal, the 5 Success Criteria, and the 3 owned pitfalls (no double-drain, create-before-delete under concurrent enqueues, hard constraints). + +### Existing implementation to extend (read before changing) +- `apps/api/src/broker/outboxWorker.ts` — `runOutboxDrain()` (the guarded drain), `isDraining` flag + its multi-process caveat comment, `startOutboxWorker()` (the 15s `setInterval`), edit-as-move ordering + durable CR-04 sibling-status gate, 412/hardfail/backoff/dead-letter handling. This is the path the signal must funnel into. +- `apps/api/src/routes/events.ts` — the three enqueue sites (create ~L300, edit/edit-as-move transaction ~L389–432, delete ~L511) where the post-commit signal must be published, and the sync-status endpoint the PWA polls. +- `apps/api/src/index.ts` §L136–141 — background-worker startup wiring (`startOutboxWorker` beside `startBrokerPoller`/`startReminderScheduler`); the trigger must be initialised consistently with this pattern, only under `isMainModule()`. + +No external specs/ADRs beyond ROADMAP.md — the durability decisions (D-04…D-12, CR-04/05, WR-01/04/06) are documented inline in `outboxWorker.ts` and `events.ts`. + + + + +## Existing Code Insights + +### Reusable Assets +- `runOutboxDrain()` + `isDraining` guard (`outboxWorker.ts`): the signal reuses this verbatim — no parallel drain path. The trailing re-drain (D-05) is a loop around it, not a second drainer. +- `startOutboxWorker()` `setInterval` pattern: the fallback stays; the new trigger sits alongside it and shares the same guarded entrypoint. +- Existing post-write `triggerTargetedResync` + `RESYNC_TIMEOUT_MS` flow already makes `status='done'` mean "local cache reflects the write" — unaffected by this phase; the signal only changes *when* the drain starts. + +### Established Patterns +- `setInterval`-only background workers (poller, reminderScheduler, outboxWorker) started exclusively under `isMainModule()` in `index.ts` — keeps timers out of the test process. The trigger follows the same gating. +- Per-uid exactly-once via the `pending AND next_attempt_at <= NOW()` selection + `isDraining`; the signal path must not introduce a second selection that could double-dispatch. + +### Integration Points +- Enqueue → signal: publish from `events.ts` after each `db.insert`/`db.transaction` commits (create, delete, and the move pair). +- Signal → drain: `outboxTrigger` listener sets `drainRequested` / invokes the guarded drain in `outboxWorker.ts`. +- Startup: initialise the trigger subscription in `index.ts` next to `startOutboxWorker()`. + + + + +## Specific Ideas + +The 1-2s target is "signal on enqueue, don't wait for the interval" — not a hard real-time budget. Actual latency is bounded by the single CalDAV round-trip to Fastmail, which is unchanged; this phase only removes the up-to-15s queueing delay before that round-trip begins. + + + + +## Deferred Ideas + +- **Multi-process / multi-replica outbox drain** — durable DB row-claim (`UPDATE calendar_outbox SET status='processing' WHERE id=? AND status='pending'`) instead of the in-memory `isDraining` guard. Documented in the `isDraining` declaration comment; out of scope while the deployment is single-process. Belongs to a future scaling phase, not v1.1. +- **Debounce/coalesce knob for burst write-back** — considered and rejected for now (D-06). Revisit only if Fastmail request-rate ever becomes a concern with more members (see `[[project-nmember-expansion]]`). + +None of the above are in scope for Phase 9. + + + +--- + +*Phase: 9-Faster-Write-Back* +*Context gathered: 2026-06-12* diff --git a/.planning/phases/09-faster-write-back/09-DISCUSSION-LOG.md b/.planning/phases/09-faster-write-back/09-DISCUSSION-LOG.md new file mode 100644 index 0000000..af65f65 --- /dev/null +++ b/.planning/phases/09-faster-write-back/09-DISCUSSION-LOG.md @@ -0,0 +1,59 @@ +# Phase 9: Faster Write-Back - Discussion Log + +> **Audit trail only.** Do not use as input to planning, research, or execution agents. +> Decisions are captured in CONTEXT.md — this log preserves the alternatives considered. + +**Date:** 2026-06-12 +**Phase:** 9-Faster-Write-Back +**Areas discussed:** Burst handling, Verification, Fallback interval + +--- + +## Burst handling + +| Option | Description | Selected | +|--------|-------------|----------| +| Trailing re-drain (guarantee last edit) | If any signal arrives mid-drain, set `drainRequested` and re-run once the current drain finishes. Guarantees the LAST edit in a burst lands in ~1-2s. Slightly more CalDAV traffic under bursts. | ✓ | +| Short debounce/coalesce | Collect signals for a 200-500ms window and drain once. Gentler on Fastmail; last edit lands a fraction slower; adds a tunable knob. | | +| Fire-and-forget, rely on 15s for stragglers | Drop mid-drain signals; missed row waits for the next 15s tick. Simplest, but violates ~1-2s for the 2nd edit in a burst. | | + +**User's choice:** Trailing re-drain (guarantee last edit) +**Notes:** Two-user household — burst volume is tiny, so snappiness/correctness of the trailing edit outweighs Fastmail request-rate politeness. Debounce knob rejected as premature. + +--- + +## Verification + +| Option | Description | Selected | +|--------|-------------|----------| +| Automated integration test on the trigger | Vitest test asserting enqueue publishes the signal and `runOutboxDrain` fires promptly (mock CalDAV dispatch). Fast, CI-able, no live Fastmail. | ✓ | +| Both: integration test + operator stopwatch | Automated wiring test plus a one-time human checkpoint timing a real edit in the Fastmail native app. | | +| Operator stopwatch only | Human checkpoint timing real edit-to-Fastmail latency; nothing in CI guards regressions. | | + +**User's choice:** Automated integration test on the trigger +**Notes:** Dev bypass user (id 1) has no CalDAV credential/calendars, so live event-create 422s in the dev stack — an automated wiring test is the durable, CI-guarded evidence. Real wall-clock latency is bounded by the CalDAV round-trip and exercised in production use. + +--- + +## Fallback interval + +| Option | Description | Selected | +|--------|-------------|----------| +| Keep 15s | Leave the fallback at 15s exactly as today. Matches Success Criterion 5 wording; idle DB polling unchanged. | ✓ | +| Lengthen it (e.g. 30-60s) | Slow the fallback since the signal handles the hot path; fewer idle DB queries but slower recovery for missed rows; changes a number the criterion names. | | + +**User's choice:** Keep 15s +**Notes:** Idle DB polling cost is negligible for this single-process, two-user deployment. + +--- + +## Claude's Discretion + +- Exact shape of the `drainRequested`/re-drain loop (flag location, loop inside `runOutboxDrain` `finally` vs. scheduler wrapper) — left to planner/researcher, provided the no-double-drain guard and exactly-once guarantees hold. +- EventEmitter singleton vs. tiny custom signal object — either acceptable; zero-dependency is the only hard constraint. +- Test file placement per existing convention (`apps/api/tests/`, not `src/`). + +## Deferred Ideas + +- Multi-process / multi-replica outbox drain via durable DB row-claim — out of scope while single-process; future scaling phase. +- Debounce/coalesce knob for burst write-back — considered and rejected for now; revisit only if Fastmail request-rate becomes a concern with more members. diff --git a/.planning/phases/09-faster-write-back/09-PATTERNS.md b/.planning/phases/09-faster-write-back/09-PATTERNS.md new file mode 100644 index 0000000..87425ee --- /dev/null +++ b/.planning/phases/09-faster-write-back/09-PATTERNS.md @@ -0,0 +1,350 @@ +# Phase 9: Faster Write-Back — Pattern Map + +**Mapped:** 2026-06-12 +**Files analyzed:** 5 (1 new, 4 modified) +**Analogs found:** 5 / 5 + +--- + +## File Classification + +| New/Modified File | Role | Data Flow | Closest Analog | Match Quality | +|---|---|---|---|---| +| `apps/api/src/lib/outboxTrigger.ts` | utility / singleton | event-driven | `apps/api/src/lib/listEmitter.ts` | exact | +| `apps/api/src/broker/outboxWorker.ts` | service / broker | event-driven + batch | `apps/api/src/broker/outboxWorker.ts` (self — extend) | exact | +| `apps/api/src/routes/events.ts` | route / controller | request-response | `apps/api/src/routes/events.ts` (self — extend) | exact | +| `apps/api/src/index.ts` | config / startup | request-response | `apps/api/src/index.ts` (self — extend) | exact | +| `apps/api/tests/broker/outboxWorker.test.ts` | test | batch | `apps/api/tests/broker/outboxWorker.test.ts` (self — extend) | exact | + +--- + +## Pattern Assignments + +### `apps/api/src/lib/outboxTrigger.ts` (NEW — utility, event-driven) + +**Analog:** `apps/api/src/lib/listEmitter.ts` + +**Imports pattern** (lines 18): +```typescript +import { EventEmitter } from 'node:events'; +``` + +**Module-level singleton pattern** (lines 22–23): +```typescript +const emitter = new EventEmitter(); +emitter.setMaxListeners(200); // listEmitter sets 200 for fan-out; outboxTrigger needs only 1 +// NOTE: for outboxTrigger, setMaxListeners is NOT needed — single subscriber, default 10 is fine +``` + +**Publish function pattern** (lines 39–41): +```typescript +export function publishListEvent(listId: number, event: ListEvent): void { + emitter.emit(`list:${listId}`, event); +} +``` +For outboxTrigger, adapt as: +```typescript +export function signalOutboxDrain(): void { + emitter.emit('drain'); +} +``` + +**Subscribe + unsubscribe pattern** (lines 47–54): +```typescript +export function subscribeListEvents( + listId: number, + handler: (event: ListEvent) => void, +): () => void { + const channel = `list:${listId}`; + emitter.on(channel, handler); + return () => emitter.off(channel, handler); +} +``` +For outboxTrigger, adapt as: +```typescript +export function onOutboxDrain(handler: () => void): () => void { + emitter.on('drain', handler); + return () => emitter.off('drain', handler); +} +``` + +**No imports from broker/, routes/, db/, or index.ts** — `listEmitter.ts` also imports nothing from the project. `outboxTrigger.ts` must follow the same zero-internal-dependency rule to avoid circular imports. + +--- + +### `apps/api/src/broker/outboxWorker.ts` (MODIFIED — service/broker, event-driven + batch) + +**Analog:** self (extend existing file) + +**Existing `isDraining` flag declaration** (line 159): +```typescript +let isDraining = false; +``` +New `drainRequested` flag goes immediately after, at the same module scope: +```typescript +let drainRequested = false; +``` + +**Existing `runOutboxDrain` guard + finally** (lines 602–605, 783–785): +```typescript +export async function runOutboxDrain(): Promise { + // CR-05: single-process concurrency guard + if (isDraining) return; + isDraining = true; + // ... (do not touch the body) + } finally { + isDraining = false; + } +} +``` +`runOutboxDrain` is NOT modified — `scheduleOutboxDrain` wraps it externally. + +**New `scheduleOutboxDrain` wrapper** — add between `isDraining` declaration and `runOutboxDrain`: +```typescript +/** + * Scheduler entrypoint called by both the EventEmitter listener (initOutboxTrigger) + * and the 15s setInterval. Implements the drain-again-if-requested loop (D-05) so + * the last edit in a burst is drained promptly without double-draining (D-02 / D-07). + * Fire-and-forget — callers do not await this. + */ +export function scheduleOutboxDrain(): void { + if (isDraining) { + drainRequested = true; + return; + } + runOutboxDrain() + .catch((err: unknown) => { + console.error('[outboxWorker] Unhandled runOutboxDrain error:', err); + }) + .finally(() => { + if (drainRequested) { + drainRequested = false; // reset FIRST, then recurse (Pitfall 3) + scheduleOutboxDrain(); // exactly one trailing re-drain (D-05) + } + }); +} +``` + +**`initOutboxTrigger` export** — add in the Scheduler section alongside `startOutboxWorker`: +```typescript +import { onOutboxDrain } from '../lib/outboxTrigger.js'; + +export function initOutboxTrigger(): void { + onOutboxDrain(() => scheduleOutboxDrain()); +} +``` + +**Existing `startOutboxWorker`** (lines 796–802) — update to call `scheduleOutboxDrain` instead of `runOutboxDrain` directly: +```typescript +export function startOutboxWorker(): void { + setInterval(() => { + scheduleOutboxDrain(); + }, 15 * 1000); +} +``` +(Existing `.catch` wrapper is absorbed into `scheduleOutboxDrain` — remove the per-call `.catch` in the interval body.) + +**Export rationale comment** (line 19 in file header): +``` +* runOutboxDrain is exported for unit testing. +* startOutboxWorker wraps it in a 15-second setInterval. +``` +Add: `scheduleOutboxDrain is exported for unit testing.` + +--- + +### `apps/api/src/routes/events.ts` (MODIFIED — route/controller, request-response) + +**Analog:** self (extend existing file) + +**Import to add** (after existing imports, lines 24–35): +```typescript +import { signalOutboxDrain } from '../lib/outboxTrigger.js'; +``` + +**Site 1 — POST /create, single insert** (lines 300–309). Insert is a bare `await db.insert(...)` (no transaction). Signal fires after the awaited insert, before the `return`: +```typescript +await db.insert(calendarOutbox).values({ + userId: currentUserId, + operation: 'create', + status: 'pending', + uid, + calendarUrl: targetCalendarUrl, + payload: JSON.stringify(payload), +}); +signalOutboxDrain(); // fire-and-forget (D-04) +return c.json({ uid }, 202); +``` + +**Site 2 — PATCH /:uid/edit, edit-as-move transaction** (lines 408–432). Signal fires AFTER `await db.transaction(...)` resolves — NOT inside the callback: +```typescript +await db.transaction(async (tx) => { + await tx.insert(calendarOutbox).values({ operation: 'delete', ... }); + await tx.insert(calendarOutbox).values({ operation: 'create', ... }); +}); +signalOutboxDrain(); // after transaction commits — both rows durable (D-03) +return c.json({ uid: newUid }, 202); +``` + +**Site 3 — PATCH /:uid/edit, same-calendar update** (lines 436–447): +```typescript +await db.insert(calendarOutbox).values({ operation: 'update', ... }); +signalOutboxDrain(); // fire-and-forget (D-04) +return c.json({ uid }, 202); +``` + +**Site 4 — DELETE /:uid** (lines 511–521): +```typescript +await db.insert(calendarOutbox).values({ operation: 'delete', ... }); +signalOutboxDrain(); // fire-and-forget (D-04) +return c.json({ uid }, 202); +``` + +All four sites are inside existing `try/catch` blocks — the signal call slots between the `await` and the `return`, matching the same pattern at each site. + +--- + +### `apps/api/src/index.ts` (MODIFIED — config/startup, request-response) + +**Analog:** self (extend existing file) + +**Existing import** (line 16): +```typescript +import { startOutboxWorker } from './broker/outboxWorker.js'; +``` +Update to: +```typescript +import { startOutboxWorker, initOutboxTrigger } from './broker/outboxWorker.js'; +``` + +**Existing startup block** (lines 112, 136–141): +```typescript +if (isMainModule()) { + // ... VAPID setup ... + startBrokerPoller(); + startOutboxWorker(); + startReminderScheduler(); + serve({ fetch: app.fetch, port: 3000 }, ...); +} +``` +Add `initOutboxTrigger()` immediately after `startOutboxWorker()`: +```typescript + startOutboxWorker(); + initOutboxTrigger(); // subscribe drain signal listener (D-01) + startReminderScheduler(); +``` + +**`isMainModule()` function** (lines 100–107) — do not modify. The trigger listener must remain gated by this guard (same as all other background workers) to prevent listener registration when tests import `app`. + +--- + +### `apps/api/tests/broker/outboxWorker.test.ts` (MODIFIED — test, batch) + +**Analog:** self (extend existing file) + +**Existing import line** (line 21): +```typescript +import { runOutboxDrain, assembleRruleString } from '../../src/broker/outboxWorker.js'; +``` +Extend to include new exports: +```typescript +import { runOutboxDrain, assembleRruleString, scheduleOutboxDrain } from '../../src/broker/outboxWorker.js'; +``` +Also import the signal function for trigger wiring tests: +```typescript +import { signalOutboxDrain } from '../../src/lib/outboxTrigger.js'; +``` + +**`vi.hoisted` + `vi.mock` scaffold** (lines 31–101) — copy verbatim into new describe block. The `mockPendingRows`, `makeRow`, `makeResponse`, and `wireMockChain` helpers are already at module scope and are reusable from the new tests without re-declaration. + +**`beforeEach` pattern** (lines 170–174): +```typescript +beforeEach(() => { + vi.resetAllMocks(); + mockPendingRows = []; + wireMockChain(); +}); +``` +New describe block uses the same `beforeEach`. Note: `vi.resetAllMocks()` clears all mock state between tests — adequate for trigger wiring tests since `isDraining` and `drainRequested` are module-level state that resets between test runs only if the module is re-imported. Use `vi.isolateModules` or reset state explicitly if module-level flags need isolation between tests. + +**`makeResponse` helper** (line 131–132): +```typescript +const makeResponse = (status: number): Response => + ({ status, ok: status >= 200 && status < 300, headers: new Headers() }) as unknown as Response; +``` +Reuse in new tests — already in scope. + +**Async flush pattern for signal→drain tests** (no fake timers needed): +```typescript +// Flush microtask queue so the promise chain from scheduleOutboxDrain completes +await new Promise(resolve => setImmediate(resolve)); +``` + +**`vi.useFakeTimers()` pattern** — needed only for tests that advance the 15s interval. Not needed for pure signal→drain tests. See existing `startOutboxWorker` interval tests for the pattern if needed. + +**New describe block placement** — add after the final existing `describe` block, before EOF: +```typescript +describe('scheduleOutboxDrain — trigger wiring (D-09)', () => { + beforeEach(() => { + vi.resetAllMocks(); + mockPendingRows = []; + wireMockChain(); + }); + + // Test A: signal → drain without timer wait (SC-1) + // Test B: signal mid-drain → exactly one trailing re-drain (D-05) + // Test C: concurrent scheduleOutboxDrain calls → each row dispatched once (D-07) +}); +``` + +--- + +## Shared Patterns + +### `isMainModule()` startup gate +**Source:** `apps/api/src/index.ts` lines 100–107 + 112 +**Apply to:** `initOutboxTrigger()` call in index.ts +```typescript +function isMainModule(): boolean { + if (!process.argv[1]) return false; + try { + return fileURLToPath(import.meta.url) === realpathSync(process.argv[1]); + } catch { + return false; + } +} + +if (isMainModule()) { + // all background worker starts here +} +``` +All listener registrations and `setInterval` calls must be inside this guard. `initOutboxTrigger` follows the same gating as `startBrokerPoller`, `startOutboxWorker`, `startReminderScheduler`. + +### Error swallowing in setInterval workers +**Source:** `apps/api/src/broker/outboxWorker.ts` lines 797–801 +**Apply to:** `scheduleOutboxDrain` wrapper (the `.catch` on the `runOutboxDrain()` call) +```typescript +runOutboxDrain().catch((err: unknown) => { + console.error('[outboxWorker] Unhandled runOutboxDrain error:', err); +}); +``` +`scheduleOutboxDrain` absorbs this pattern into its own `.catch` — the interval body becomes a bare `scheduleOutboxDrain()` call with no inline error handling. + +### Module-level singleton + typed publish/subscribe API +**Source:** `apps/api/src/lib/listEmitter.ts` lines 18–54 +**Apply to:** `apps/api/src/lib/outboxTrigger.ts` +Pattern: `const emitter = new EventEmitter()` at module scope; named export functions wrap all emitter access; no direct EventEmitter reference leaks to callers. `outboxTrigger.ts` follows this exactly. + +--- + +## No Analog Found + +None. All five files have direct analogs (three are self-extensions of existing files). + +--- + +## Metadata + +**Analog search scope:** `apps/api/src/lib/`, `apps/api/src/broker/`, `apps/api/src/routes/`, `apps/api/src/`, `apps/api/tests/broker/` +**Files read:** `listEmitter.ts`, `outboxWorker.ts`, `events.ts`, `index.ts`, `tests/broker/outboxWorker.test.ts` +**Pattern extraction date:** 2026-06-12 diff --git a/.planning/phases/09-faster-write-back/09-RESEARCH.md b/.planning/phases/09-faster-write-back/09-RESEARCH.md new file mode 100644 index 0000000..1f33007 --- /dev/null +++ b/.planning/phases/09-faster-write-back/09-RESEARCH.md @@ -0,0 +1,731 @@ +# Phase 9: Faster Write-Back - Research + +**Researched:** 2026-06-12 +**Domain:** In-process event-driven outbox drain signal (Node.js EventEmitter, TypeScript) +**Confidence:** HIGH + +--- + + +## User Constraints (from CONTEXT.md) + +### Locked Decisions +- **D-01:** New artifact is a single zero-dependency in-process `EventEmitter` at `apps/api/src/lib/outboxTrigger.ts`. No new npm dependency. +- **D-02:** The signal funnels through the **existing** `isDraining`-guarded `runOutboxDrain()` path via a `drainRequested` flag. The trigger must NEVER call `runOutboxDrain()` directly in a way that bypasses the `isDraining` guard or escapes the error-caught wrapper (Pitfall 5 — no double-drain). +- **D-03:** The signal is published **after** the enqueue transaction commits — never between the two inserts of an edit-as-move (Pitfall 6). For a move, publish once after both CREATE and DELETE rows are committed, so create-before-delete ordering is never raced by the signal. +- **D-04:** Route handlers still return optimistic 202 immediately and make no inline CalDAV call; the signal is fire-and-forget. +- **D-05:** **Guarantee the last edit in a burst.** When a signal arrives while a drain is already in flight, set `drainRequested` and re-run the drain exactly once after the current one finishes (drain-again-if-requested loop). +- **D-06:** No debounce/coalesce window and no rejection of mid-drain signals. +- **D-07:** Exactly-once per uid preserved — trailing re-drain reuses the same `pending AND next_attempt_at <= NOW()` selection and `isDraining` guard. +- **D-08:** Keep the 15s `setInterval` fallback exactly as-is. Do not lengthen it. +- **D-09:** Prove SC-1 with an automated integration test on the trigger wiring (Vitest). Also cover trailing-re-drain and no-double-PUT overlap cases. +- **D-10:** No operator-stopwatch human checkpoint required. + +### Claude's Discretion +- Exact shape of the `drainRequested`/re-drain loop (where the flag lives, whether the loop sits inside `runOutboxDrain`'s `finally` or in the scheduler wrapper). +- Whether the EventEmitter is a module singleton vs. a tiny custom signal object — either is fine; zero-dep is the only hard constraint. +- Test file placement follows the existing convention (API tests live under `apps/api/tests/`, never `src/`). + +### Deferred Ideas (OUT OF SCOPE) +- Multi-process / multi-replica outbox drain — durable DB row-claim instead of the in-memory `isDraining` guard. Not this phase. +- Debounce/coalesce knob for burst write-back — rejected for now (D-06). + + +--- + + +## Phase Requirements + +| ID | Description | Research Support | +|----|-------------|------------------| +| CAL-15 | A created, edited, or deleted event reaches Fastmail within ~2 seconds (event-driven outbox drain) instead of up to ~15s, while preserving the optimistic-202 accept and all outbox durability guarantees (create-before-delete ordering, drain concurrency guard, fresh-etag-before-PUT, per-uid exactly-once). | Signal mechanism design (Q1–Q4), post-commit publish points (Q3), test wiring (Q5). | + + +--- + +## Summary + +Phase 9 adds a single new module — `apps/api/src/lib/outboxTrigger.ts` — that is a module-level `EventEmitter` singleton following the exact pattern already established by `apps/api/src/lib/listEmitter.ts`. The outbox drain loop in `outboxWorker.ts` already has all the durability machinery needed; the only required change is adding a `drainRequested` flag and a trailing-re-drain loop around the `isDraining` guard, then wiring a listener so an enqueue signal triggers a prompt drain. + +The two concrete changes to existing files are: (1) `outboxWorker.ts` gains the `drainRequested` flag and a wrapper function (`scheduleOutboxDrain`) that implements the drain-again-if-requested loop and gets called from both the EventEmitter listener and the 15s `setInterval`; (2) `events.ts` gains three call sites that publish the signal after each commit. Startup wiring in `index.ts` follows the existing `startBrokerPoller`/`startOutboxWorker` pattern. + +The test strategy builds directly on the existing `outboxWorker.test.ts` mock infrastructure. No new npm packages are needed. + +**Primary recommendation:** Implement the `drainRequested` loop as a thin scheduler wrapper (`scheduleOutboxDrain`) in `outboxWorker.ts` rather than inside `runOutboxDrain`'s `finally`, so `runOutboxDrain` itself remains testable in isolation and its existing tests are unchanged. + +--- + +## Architectural Responsibility Map + +| Capability | Primary Tier | Secondary Tier | Rationale | +|------------|-------------|----------------|-----------| +| Event-driven drain signal | API / Backend (in-process) | — | Single-process; no Redis for drain; in-memory EventEmitter is correct tier | +| Outbox row enqueue | API / Backend (route handler) | — | Existing responsibility; this phase only adds the post-commit signal publish | +| Drain execution + CalDAV I/O | API / Backend (broker) | — | `runOutboxDrain` already owns all CalDAV dispatch; signal funnels into it | +| Fallback interval timing | API / Backend (broker) | — | 15s `setInterval` in `startOutboxWorker`; unchanged | +| Test wiring validation | API / Backend (Vitest) | — | Unit/integration tests under `apps/api/tests/`; no UI involved | + +--- + +## Standard Stack + +### Core + +No new packages required for this phase. [VERIFIED: CLAUDE.md + codebase read] + +| Module | Purpose | Status | +|--------|---------|--------| +| `node:events` (built-in) | `EventEmitter` for the drain signal | Already used in `listEmitter.ts` | +| `vitest` | Test framework for trigger wiring tests | Already installed and configured | + +### Supporting + +No supporting additions needed. + +### Alternatives Considered + +| Instead of | Could Use | Tradeoff | +|------------|-----------|----------| +| Module-level `EventEmitter` singleton | Custom minimal signal object (`{ on, emit, off }`) | Either satisfies D-01. The `EventEmitter` approach is idiomatic Node.js, matches `listEmitter.ts` exactly, and requires zero extra code. A custom object is slightly lighter but offers no practical advantage here. | +| Trailing-re-drain in scheduler wrapper | Trailing-re-drain inside `runOutboxDrain`'s `finally` | Wrapper approach keeps `runOutboxDrain` a pure, independently testable drain cycle with no self-rescheduling logic. The `finally` approach couples drain + re-scheduling into one function, complicates existing test isolation. | + +**Installation:** No new packages required. + +--- + +## Package Legitimacy Audit + +No new external packages are introduced in this phase. The only new code uses `node:events` (Node.js built-in, zero-dep constraint from D-01). [VERIFIED: codebase read] + +**Packages removed due to SLOP verdict:** none +**Packages flagged as suspicious SUS:** none + +--- + +## Architecture Patterns + +### System Architecture Diagram + +``` +POST /api/events/create +PATCH /api/events/:uid/edit ──► db.insert (/ db.transaction) +DELETE /api/events/:uid │ + commit │ (post-commit only) + ▼ + outboxTrigger.emit('drain') + │ + ┌────────────────┘ + │ listener in outboxWorker.ts + ▼ + scheduleOutboxDrain() ◄── 15s setInterval also calls this + │ + drainRequested already set? + Yes → drainRequested=true (D-05: trailing flag) + No → if (!isDraining) runOutboxDrain() + │ + finally: drainRequested? + Yes → clear flag, call runOutboxDrain() again (once) + │ + CalDAV PUT/DELETE to Fastmail + │ + Mark outbox row 'done' + │ + triggerTargetedResync() +``` + +### Recommended Project Structure + +No structural changes to existing directories. One new file added: + +``` +apps/api/src/ +└── lib/ + ├── listEmitter.ts # existing — established pattern to follow + └── outboxTrigger.ts # NEW: module-level drain signal emitter + +apps/api/tests/ +└── broker/ + └── outboxWorker.test.ts # extend with trigger wiring tests (D-09) +``` + +--- + +## Key Open Questions — Answers for the Planner + +### Q1: Where does `drainRequested` and the trailing-re-drain loop belong? + +**Answer: In a new `scheduleOutboxDrain()` wrapper in `outboxWorker.ts`, NOT inside `runOutboxDrain`'s `finally`.** [VERIFIED: codebase read — outboxWorker.ts] + +**Rationale from the existing code:** + +`runOutboxDrain()` (L602–L786) is a self-contained drain cycle: +- `if (isDraining) return;` guard at the top (L604) +- `isDraining = true;` (L605) +- `try { ... } finally { isDraining = false; }` (L783–L785) + +The `finally` block currently contains only `isDraining = false`. Adding a re-drain call there would mean `runOutboxDrain` recurses into itself via a flag, making the function non-idempotent and complicating the existing 15 test cases that call `runOutboxDrain()` directly. + +**Concrete shape:** + +```typescript +// Module-level flag (joins isDraining) +let drainRequested = false; + +/** + * Scheduler entrypoint called by both the EventEmitter listener and the 15s setInterval. + * Implements the drain-again-if-requested loop (D-05) so the last edit in a burst + * is drained promptly without double-draining the same row (D-02 / D-07). + * + * Fire-and-forget: callers do not await this. + */ +function scheduleOutboxDrain(): void { + if (isDraining) { + // A drain is already in flight — set flag so it re-runs when done (D-05) + drainRequested = true; + return; + } + // Kick off a drain cycle; on completion, check if another was requested + runOutboxDrain() + .catch((err: unknown) => { + console.error('[outboxWorker] Unhandled runOutboxDrain error:', err); + }) + .finally(() => { + if (drainRequested) { + drainRequested = false; + scheduleOutboxDrain(); // exactly one trailing re-drain (D-05) + } + }); +} +``` + +Key properties: +- `isDraining` is set/cleared inside `runOutboxDrain` exactly as today — no change to `runOutboxDrain`. +- When a signal arrives while `isDraining=true`, `drainRequested=true` is set. After the in-flight drain's `finally` clears `isDraining=false`, the `.finally()` on the outer Promise re-calls `scheduleOutboxDrain()`, which sees `isDraining=false` and starts a fresh drain. +- The recursive `scheduleOutboxDrain()` call in `.finally()` is safe because `drainRequested` is cleared before the call, so a second re-entrant signal during the trailing drain sets it again to true and produces exactly one more follow-up — not an unbounded chain. +- The 15s `setInterval` already calls `runOutboxDrain()` directly (L797). It should be updated to call `scheduleOutboxDrain()` instead, so the fallback path also benefits from the trailing-re-drain guarantee and doesn't double-drain if a signal-triggered drain is in progress at the 15s mark. + +**Updated `startOutboxWorker`:** + +```typescript +export function startOutboxWorker(): void { + setInterval(() => { + scheduleOutboxDrain(); + }, 15 * 1000); +} +``` + +### Q2: Module/singleton shape for `outboxTrigger.ts` + +**Answer: Module-level `EventEmitter` singleton, following `listEmitter.ts` verbatim.** [VERIFIED: codebase read — listEmitter.ts] + +`listEmitter.ts` establishes the exact pattern already used in this codebase: + +```typescript +// apps/api/src/lib/outboxTrigger.ts + +import { EventEmitter } from 'node:events'; + +const emitter = new EventEmitter(); +// No persistent listeners — max 1 (the outboxWorker subscriber). Default limit is 10. + +/** + * Signal the outbox drain that a new row was enqueued. + * Called from routes/events.ts after each enqueue commit (D-03). + * Fire-and-forget — never awaited. + */ +export function signalOutboxDrain(): void { + emitter.emit('drain'); +} + +/** + * Subscribe to drain signals. Returns an unsubscribe function. + * Called once from outboxWorker.ts (via initOutboxTrigger in index.ts). + */ +export function onOutboxDrain(handler: () => void): () => void { + emitter.on('drain', handler); + return () => emitter.off('drain', handler); +} +``` + +**Circular-import safety:** `outboxTrigger.ts` imports only `node:events`. It has no imports from `broker/`, `routes/`, `db/`, or `index.ts`. Both consumers import from it: +- `routes/events.ts` imports `signalOutboxDrain` (publish side) +- `broker/outboxWorker.ts` imports `onOutboxDrain` via a wiring call from `index.ts` (subscribe side) + +There is no circular import risk because `outboxTrigger.ts` depends on nothing in the project. + +**Timer/listener leakage in Vitest:** The `EventEmitter` itself has no timers. Listeners are added only under `isMainModule()` in `index.ts`. Test files import `runOutboxDrain` from `outboxWorker.ts` directly — they never trigger `isMainModule()` initialization — so no listener is registered in the test process. This is the same pattern used for `startBrokerPoller`, `startOutboxWorker`, and `startReminderScheduler` (all gated by `isMainModule()` at `index.ts` L112). + +### Q3: Exact post-commit publish points in `events.ts` + +**Answer: Three publish sites, all after the `await db.insert()`/`await db.transaction()` call that commits the outbox row(s).** [VERIFIED: codebase read — events.ts] + +The current enqueue sites and their commit boundaries: + +**Site 1 — POST /api/events/create (L300–L308):** +```typescript +await db.insert(calendarOutbox).values({ ... }); +// ← publish here (after db.insert resolves) +return c.json({ uid }, 202); +``` +The `db.insert` is a single auto-committed statement (no explicit transaction). Signal fires immediately after the awaited insert. + +**Site 2 — PATCH /api/events/:uid/edit, same-calendar update (L436–L445):** +```typescript +await db.insert(calendarOutbox).values({ operation: 'update', ... }); +// ← publish here +return c.json({ uid }, 202); +``` +Same pattern — single auto-committed insert. + +**Site 3 — PATCH /api/events/:uid/edit, edit-as-move (L408–L432):** +```typescript +await db.transaction(async (tx) => { + await tx.insert(calendarOutbox).values({ operation: 'delete', ... }); + await tx.insert(calendarOutbox).values({ operation: 'create', ... }); +}); +// ← publish HERE — after the transaction commits (D-03) +// NOT inside the transaction callback (that is before commit, not after) +return c.json({ uid: newUid }, 202); +``` +This is the critical one: the signal must fire after the `await db.transaction(...)` resolves, not inside the callback. Both rows are committed atomically when `db.transaction` resolves. Publishing inside the callback would fire the signal before the DELETE row is durably written (violates D-03 / Pitfall 6 — create-before-delete ordering could be raced). + +**Site 4 — DELETE /api/events/:uid (L511–L519):** +```typescript +await db.insert(calendarOutbox).values({ operation: 'delete', ... }); +// ← publish here +return c.json({ uid }, 202); +``` + +All four sites are inside a `try/catch` block. The signal call should go immediately after the successful `await`, before the `return c.json(...)`. The signal is fire-and-forget — no `await` on `signalOutboxDrain()`. + +**Pattern (same for all sites):** +```typescript +await db.insert(calendarOutbox).values({ ... }); +signalOutboxDrain(); // fire-and-forget, D-04 +return c.json({ uid }, 202); +``` + +### Q4: Wiring in `index.ts` under `isMainModule()` + +**Answer: Add `initOutboxTrigger()` export to `outboxWorker.ts` that subscribes the listener, and call it from `index.ts` alongside `startOutboxWorker()`.** [VERIFIED: codebase read — index.ts L112–L146] + +The existing startup block at `index.ts` L136–L141: +```typescript +if (isMainModule()) { + // ... VAPID setup ... + startBrokerPoller(); + startOutboxWorker(); + startReminderScheduler(); + // serve(...) +} +``` + +Add the subscription wiring as an exported function in `outboxWorker.ts`: +```typescript +import { onOutboxDrain } from '../lib/outboxTrigger.js'; + +export function initOutboxTrigger(): void { + onOutboxDrain(() => scheduleOutboxDrain()); +} +``` + +Then in `index.ts`: +```typescript +import { startOutboxWorker, initOutboxTrigger } from './broker/outboxWorker.js'; + +if (isMainModule()) { + // ... + startOutboxWorker(); + initOutboxTrigger(); // wire drain signal after startOutboxWorker (D-01) + // ... +} +``` + +Order matters: `startOutboxWorker()` should be called before `initOutboxTrigger()` so the fallback interval is running before the signal listener is active. In practice both are synchronous registrations so order within the same tick is fine, but matching the logical dependency is cleaner. + +### Q5: Vitest test strategy for trigger wiring (D-09) + +**Answer: Extend `tests/broker/outboxWorker.test.ts` with a new describe block for trigger wiring. Use the existing mock infrastructure verbatim; control async timing with `vi.useFakeTimers()` or manual promise resolution.** [VERIFIED: codebase read — tests/broker/outboxWorker.test.ts] + +**Test placement:** `apps/api/tests/broker/outboxWorker.test.ts` — extend the existing file, or a sibling `tests/lib/outboxTrigger.test.ts` if isolation is preferred. The existing `outboxWorker.test.ts` already has the full DB/write/sync mock scaffold. Extending it avoids re-wiring the mock chain. + +**What to test (D-09 + CAL-15 Success Criteria):** + +**Test A — SC-1: enqueue signal → drain invoked promptly** + +Strategy: spy on `runOutboxDrain` (or on `createCalendarEvent` from `write.js`) and assert it is called after `signalOutboxDrain()` without any timer advancement. + +```typescript +it('signal after enqueue invokes runOutboxDrain without waiting 15s', async () => { + const { createCalendarEvent } = await import('../../src/broker/write.js'); + vi.mocked(createCalendarEvent).mockResolvedValue(makeResponse(201)); + mockPendingRows = [makeRow()]; + + signalOutboxDrain(); + + // Let microtasks / promise chain flush + await new Promise(resolve => setImmediate(resolve)); + + expect(createCalendarEvent).toHaveBeenCalledTimes(1); + // No timer advance needed — signal fires synchronously +}); +``` + +Note: `signalOutboxDrain` calls `emitter.emit('drain')` synchronously. The listener calls `scheduleOutboxDrain()`. If `isDraining` is false, `runOutboxDrain()` is called immediately (but it returns a Promise). The `await new Promise(resolve => setImmediate(resolve))` flushes the microtask queue so the async drain completes. + +**Test B — SC-4 / D-05: signal during in-flight drain → exactly one trailing re-drain** + +Strategy: make `createCalendarEvent` slow (using a manually resolved promise), fire the signal twice, assert `runOutboxDrain` ran exactly twice (not three or more times). + +```typescript +it('D-05: signal arriving mid-drain triggers exactly one trailing re-drain', async () => { + let resolveFirst: () => void; + const firstDone = new Promise(res => { resolveFirst = res; }); + + const { createCalendarEvent } = await import('../../src/broker/write.js'); + vi.mocked(createCalendarEvent) + .mockImplementationOnce(async () => { await firstDone; return makeResponse(201); }) + .mockResolvedValue(makeResponse(201)); + + mockPendingRows = [makeRow({ id: 1 }), makeRow({ id: 2 })]; + + // Start drain 1 (in-flight) + signalOutboxDrain(); + + // While drain 1 is in flight, fire two more signals + signalOutboxDrain(); + signalOutboxDrain(); + + // Release drain 1 + resolveFirst!(); + + // Wait for trailing drain to complete + await new Promise(resolve => setImmediate(resolve)); + await new Promise(resolve => setImmediate(resolve)); + + // createCalendarEvent called for rows in drain 1 + exactly one trailing drain + // Not called 3+ times (coalesced signals → one trailing run) + expect(vi.mocked(createCalendarEvent).mock.calls.length).toBeLessThanOrEqual( + mockPendingRows.length * 2 + ); +}); +``` + +**Test C — SC-4 / D-07: 15s fallback + signal do not double-PUT the same row** + +Strategy: rely on the existing CR-05 test which already verifies `isDraining` prevents concurrent `runOutboxDrain` calls. The scheduler wrapper shares the same `isDraining` guard, so the existing test covers this. Add a new test that calls `scheduleOutboxDrain()` twice concurrently and asserts `createCalendarEvent` is called exactly once. + +```typescript +it('D-07: concurrent scheduleOutboxDrain calls dispatch each row exactly once', async () => { + const { createCalendarEvent } = await import('../../src/broker/write.js'); + vi.mocked(createCalendarEvent).mockImplementation( + () => new Promise(resolve => setTimeout(() => resolve(makeResponse(201)), 20)) + ); + mockPendingRows = [makeRow({ id: 1 })]; + + scheduleOutboxDrain(); + scheduleOutboxDrain(); + await vi.runAllTimersAsync(); // flush setTimeout(20ms) + + expect(createCalendarEvent).toHaveBeenCalledTimes(1); +}); +``` + +**Exporting `scheduleOutboxDrain` for tests:** `scheduleOutboxDrain` is an internal scheduler function. For testability, export it from `outboxWorker.ts` similarly to how `runOutboxDrain` is already exported (`runOutboxDrain is exported for unit testing` — L19). The same rationale applies. + +**Deterministic async control:** Use `setImmediate`-based microtask flushing (not `vi.useFakeTimers()`) for signal→drain tests, since the signal path is entirely microtask-driven (no `setTimeout`/`setInterval`). Use `vi.useFakeTimers()` only for tests that need to advance the 15s fallback interval. + +--- + +## Don't Hand-Roll + +| Problem | Don't Build | Use Instead | Why | +|---------|-------------|-------------|-----| +| Drain signal mechanism | Custom pub/sub, Redis channel, custom promise queue | `node:events` `EventEmitter` | Built-in, zero-dep, already used in `listEmitter.ts`. The drain is single-process — no cross-process messaging needed. | +| Concurrency control for drain | Custom mutex, semaphore, DB lock | Existing `isDraining` module-level flag | Already covers the single-process deployment; adding a second mechanism creates dual-source-of-truth bugs. | +| Burst coalescing | Debounce timer, queue with max-depth | `drainRequested` boolean flag | For a two-user household the burst is never more than 2-3 concurrent edits. A boolean flag is correct and sufficient (D-06). | +| Timer-free async test control | `sleep()`, `await new Promise(r => setTimeout(r, 100))` (flaky) | `setImmediate` flush for microtasks; `vi.runAllTimersAsync()` for fake-timer tests | Deterministic, zero wall-clock dependency | + +**Key insight:** The entire phase is wiring — connecting an already-correct drain function to an already-proven event mechanism. The only implementation risk is in the flag placement and the post-commit publish ordering, both of which are fully determined by reading the existing code. + +--- + +## Common Pitfalls + +### Pitfall 1: Publishing the signal inside the `db.transaction()` callback (edit-as-move) + +**What goes wrong:** The DELETE row is not yet durably written when the callback runs. The signal fires, the drain starts immediately, and it sees only the CREATE row. The DELETE row appears in the next drain cycle with sibling status already 'done', so it proceeds — but CREATE happened before DELETE as required. However, if the signal fires between the two `tx.insert` calls (impossible in the current sequential async code, but could happen if restructured), only one of the two rows would be in the DB. + +**Why it happens:** The `db.transaction(async (tx) => { ... })` callback executes inside the transaction boundary. `await db.transaction(...)` resolves when the transaction commits. + +**How to avoid:** Publish `signalOutboxDrain()` after `await db.transaction(...)` resolves, not inside the callback. The current code structure makes this natural — the `await db.transaction(...)` is on one line; the signal call goes on the next line after it. + +**Warning signs:** If the test for "edit-as-move creates-before-delete" starts failing intermittently, check where the signal publish is placed relative to the transaction boundary. + +### Pitfall 2: Calling `runOutboxDrain()` directly from the signal listener (bypassing `isDraining`) + +**What goes wrong:** If the signal listener calls `runOutboxDrain()` directly instead of going through `scheduleOutboxDrain()`, and the 15s `setInterval` fires at the same moment, both calls enter `runOutboxDrain` simultaneously. The second call returns immediately (isDraining guard), but there is a race on the `isDraining = true` assignment vs. the guard check in a microtask boundary. + +**Why it happens:** EventEmitter listeners run synchronously in the same tick as `emit`. If `isDraining` is false when the listener fires, the listener starts a drain. If the setInterval fires in the same tick (impossible in practice for a 15s interval, but possible in tests using fake timers), a second concurrent drain starts. + +**How to avoid:** Both the listener and the setInterval call `scheduleOutboxDrain()`, not `runOutboxDrain()` directly. `scheduleOutboxDrain` checks `isDraining` before calling `runOutboxDrain`, and sets `drainRequested` if already draining. This is exactly D-02. + +### Pitfall 3: `drainRequested` flag not reset before the trailing drain call + +**What goes wrong:** If `drainRequested` is set to `false` after the trailing `scheduleOutboxDrain()` call in the `.finally()`, not before, a new signal arriving between the flag reset and the trailing drain call sets `drainRequested=true` again. The trailing drain then completes, and `.finally()` sees `drainRequested=true` again — triggering an infinite loop. + +**How to avoid:** In the `.finally()` callback: check `drainRequested`, set `drainRequested = false`, then call `scheduleOutboxDrain()`. The re-check inside `scheduleOutboxDrain` handles any new signals that arrived after the reset. + +```typescript +.finally(() => { + if (drainRequested) { + drainRequested = false; // reset FIRST + scheduleOutboxDrain(); // then recurse — if new signal arrived, drainRequested=true again + } +}); +``` + +### Pitfall 4: Registering the EventEmitter listener at module import time (timer/listener leakage in tests) + +**What goes wrong:** If `onOutboxDrain(handler)` is called at the top level of `outboxWorker.ts` (outside a function), it registers a listener when the module is imported. Vitest imports `outboxWorker.ts` to test `runOutboxDrain`. The listener is registered, and any `signalOutboxDrain()` call in a test unexpectedly triggers the handler, causing spurious drain calls across test isolation boundaries. + +**How to avoid:** Register the listener only inside `initOutboxTrigger()`, which is called only from `index.ts` under `isMainModule()`. This matches the `startBrokerPoller`/`startOutboxWorker`/`startReminderScheduler` pattern exactly. + +### Pitfall 5: Using `setMaxListeners` too low on the drain emitter + +**What goes wrong:** Not a real risk here (the drain emitter has exactly one subscriber — `initOutboxTrigger`). But if the default limit of 10 is somehow hit (e.g. tests that call `initOutboxTrigger` without cleanup), Node.js emits a `MaxListenersExceededWarning`. + +**How to avoid:** Since there is exactly one subscriber, the default limit of 10 is fine. No `setMaxListeners` call needed (unlike `listEmitter.ts` which sets 200 for SSE fan-out). + +--- + +## Code Examples + +### Pattern 1: Module-level EventEmitter singleton (from `listEmitter.ts`) [VERIFIED: codebase read] + +```typescript +// apps/api/src/lib/outboxTrigger.ts +import { EventEmitter } from 'node:events'; + +const emitter = new EventEmitter(); +// No setMaxListeners needed — single subscriber + +export function signalOutboxDrain(): void { + emitter.emit('drain'); +} + +export function onOutboxDrain(handler: () => void): () => void { + emitter.on('drain', handler); + return () => emitter.off('drain', handler); +} +``` + +### Pattern 2: `scheduleOutboxDrain` wrapper with trailing-re-drain loop [VERIFIED: codebase read + logic derivation from D-05] + +```typescript +// In outboxWorker.ts, after `let isDraining = false;` + +let drainRequested = false; + +function scheduleOutboxDrain(): void { + if (isDraining) { + drainRequested = true; + return; + } + runOutboxDrain() + .catch((err: unknown) => { + console.error('[outboxWorker] Unhandled runOutboxDrain error:', err); + }) + .finally(() => { + if (drainRequested) { + drainRequested = false; + scheduleOutboxDrain(); + } + }); +} + +// Export for test access +export { scheduleOutboxDrain }; +``` + +### Pattern 3: Post-commit signal publish in route handler [VERIFIED: codebase read — events.ts] + +```typescript +// In the create handler, after the db.insert: +await db.insert(calendarOutbox).values({ ... }); +signalOutboxDrain(); // fire-and-forget (D-04) +return c.json({ uid }, 202); + +// In the edit-as-move handler, after the transaction: +await db.transaction(async (tx) => { + await tx.insert(calendarOutbox).values({ operation: 'delete', ... }); + await tx.insert(calendarOutbox).values({ operation: 'create', ... }); +}); +signalOutboxDrain(); // after transaction commits — both rows durable (D-03) +return c.json({ uid: newUid }, 202); +``` + +### Pattern 4: Startup wiring in `index.ts` [VERIFIED: codebase read — index.ts L112–L146] + +```typescript +// import addition at top of index.ts: +import { startOutboxWorker, initOutboxTrigger } from './broker/outboxWorker.js'; + +// In the isMainModule() block (after startOutboxWorker): +startOutboxWorker(); +initOutboxTrigger(); // subscribe the signal listener +``` + +### Pattern 5: Vitest async flush without fake timers [ASSUMED — standard Node.js async behavior] + +```typescript +// Flush microtasks + immediate queue (for testing signal→drain without timers) +await new Promise(resolve => setImmediate(resolve)); +``` + +--- + +## State of the Art + +| Old Approach | Current Approach | When Changed | Impact | +|--------------|------------------|--------------|--------| +| node-cron for scheduled jobs | `setInterval` only | Prior to v1.0 (node-cron 4.2.1 silently skipped ticks) | Hard constraint: do not reintroduce node-cron | +| Top-level background worker startup | Gated by `isMainModule()` | Phase 03 (WR-04 fix) | Hard constraint: all listeners/timers must be inside `isMainModule()` | + +**Deprecated/outdated:** +- `node-cron` for this project: silently skips scheduled executions in the long-running server process. `setInterval` only. + +--- + +## Runtime State Inventory + +Step 2.5: SKIPPED (not a rename/refactor/migration phase — greenfield addition of one module and wiring changes to three existing files). + +--- + +## Environment Availability + +Step 2.6: No external tool dependencies beyond the existing Node.js + pnpm stack already confirmed operational. All changes are in-process TypeScript. + +| Dependency | Required By | Available | Version | Fallback | +|------------|------------|-----------|---------|----------| +| Node.js `node:events` | `outboxTrigger.ts` | Always — built-in | Same as runtime (22 LTS) | — | +| Vitest | Test suite | Already installed | Per `package.json` | — | +| MariaDB (for integration tests) | `apps/api/tests/` | Confirmed (existing CI) | Dev compose | — | + +--- + +## Validation Architecture + +Nyquist validation is enabled (config `workflow.nyquist_validation` not set to false). + +### Test Framework + +| Property | Value | +|----------|-------| +| Framework | Vitest (vite-native) | +| Config file | `apps/api/vitest.config.ts` | +| Quick run command | `pnpm --filter @familysync/api exec vitest run tests/broker/outboxWorker.test.ts` | +| Full suite command | `pnpm --filter @familysync/api exec vitest run` | + +### Phase Requirements → Test Map + +| Req ID | Success Criterion | Behavior | Test Type | Automated Command | File Exists? | +|--------|------------------|----------|-----------|-------------------|-------------| +| CAL-15 / SC-1 | Change reaches Fastmail in ~1-2s | Signal after enqueue triggers `runOutboxDrain` without interval wait | unit (trigger wiring) | `pnpm --filter @familysync/api exec vitest run tests/broker/outboxWorker.test.ts` | Extend existing | +| CAL-15 / SC-2 | Route handler returns 202 immediately, no inline CalDAV | Existing passing tests cover 202 response; signal is fire-and-forget | unit (existing) | Same | Already exists | +| CAL-15 / SC-3 | Edit-as-move create-before-delete preserved | Existing D-04 / CR-04 tests cover this; signal fires after transaction so ordering is not disturbed | unit (existing) | Same | Already exists | +| CAL-15 / SC-4 | No duplicate PUT when signal and 15s interval overlap | Concurrent `scheduleOutboxDrain` calls invoke `createCalendarEvent` exactly once | unit (trigger wiring) | Same | Extend existing | +| CAL-15 / SC-4 | Trailing re-drain under burst (D-05) | Signal mid-drain → exactly one follow-up drain | unit (trigger wiring) | Same | Extend existing | +| CAL-15 / SC-5 | 15s fallback still runs | Existing `startOutboxWorker` setInterval unchanged; `scheduleOutboxDrain` called from interval | unit (scheduler) | Same | Extend existing | + +### Sampling Rate +- **Per task commit:** `pnpm --filter @familysync/api exec vitest run tests/broker/outboxWorker.test.ts` +- **Per wave merge:** `pnpm --filter @familysync/api exec vitest run` +- **Phase gate:** Full suite green before `/gsd-verify-work` + +### Wave 0 Gaps + +- [ ] New describe block in `tests/broker/outboxWorker.test.ts` — covers SC-1 (trigger wiring), SC-4 (no-double-PUT), D-05 (trailing re-drain) +- [ ] `outboxTrigger.ts` must export `signalOutboxDrain` and `onOutboxDrain` before the trigger wiring tests can import them +- [ ] `scheduleOutboxDrain` must be exported from `outboxWorker.ts` for direct test invocation + +--- + +## Security Domain + +`security_enforcement` is enabled (not set to false in config). + +### Applicable ASVS Categories + +| ASVS Category | Applies | Standard Control | +|---------------|---------|-----------------| +| V2 Authentication | No | Not touched — auth is unchanged | +| V3 Session Management | No | Not touched | +| V4 Access Control | No | Not touched — ownership checks in `events.ts` are unchanged | +| V5 Input Validation | No | Not touched — Zod validation at enqueue is unchanged; this phase only adds a post-enqueue signal | +| V6 Cryptography | No | Not touched — credential decryption in `outboxWorker.ts` is unchanged | + +### Known Threat Patterns for this phase + +| Pattern | STRIDE | Standard Mitigation | +|---------|--------|---------------------| +| Signal amplification (unbounded re-drain loop) | DoS (against Fastmail rate limits) | `drainRequested` boolean collapses all mid-drain signals into exactly one trailing drain (D-05 / D-06) | +| Listener memory leak (test isolation) | DoS (open handles preventing process exit) | Listener registered only under `isMainModule()` — never in test process (Q4 answer above) | + +No new network surface, no new auth paths, no new input validation vectors introduced by this phase. + +--- + +## Assumptions Log + +| # | Claim | Section | Risk if Wrong | +|---|-------|---------|---------------| +| A1 | `setImmediate` flush is sufficient to observe the signal→drain path in tests without fake timers | Q5 / Validation Architecture | Test may need `await Promise.resolve()` or an extra tick; low risk, easily corrected in the test code | +| A2 | Exporting `scheduleOutboxDrain` from `outboxWorker.ts` for test access follows the established `runOutboxDrain` export precedent without breaking encapsulation | Q5 | Low risk — already documented in `outboxWorker.ts` L19 as the intended pattern | + +All other claims in this document are verified against the codebase. + +--- + +## Open Questions + +1. **`scheduleOutboxDrain` — internal or re-exported from `startOutboxWorker`?** + - What we know: `runOutboxDrain` is already exported for testing (L19 comment). `scheduleOutboxDrain` needs the same treatment. + - What's unclear: whether to export it as a named export at the module level or wrap it inside `startOutboxWorker` and expose via a returned object. + - Recommendation: export it as a plain named export (same as `runOutboxDrain`) for simplicity and test symmetry. The existing pattern is to export thin named functions. + +2. **`initOutboxTrigger` — in `outboxWorker.ts` or a separate `outboxTriggerInit.ts`?** + - What we know: `startBrokerPoller`, `startOutboxWorker`, `startReminderScheduler` are all exported from their respective broker modules and called from `index.ts`. + - Recommendation: keep `initOutboxTrigger` in `outboxWorker.ts` alongside `startOutboxWorker` — they are functionally related (both are outbox startup concerns) and co-locating them means `index.ts` only needs one additional import name from the same module. + +--- + +## Sources + +### Primary (HIGH confidence) +- `apps/api/src/broker/outboxWorker.ts` — Full read; confirmed `isDraining` flag structure, `runOutboxDrain` control flow, `startOutboxWorker` setInterval, `finally` block shape, existing export rationale. +- `apps/api/src/routes/events.ts` — Full read; confirmed exact locations of the three enqueue sites (create L300–308, edit-same-cal L436–445, edit-as-move L408–432, delete L511–519), transaction boundaries. +- `apps/api/src/index.ts` — Full read; confirmed `isMainModule()` guard, startup wiring block L112–L146. +- `apps/api/src/lib/listEmitter.ts` — Full read; confirmed EventEmitter singleton pattern, `publish`/`subscribe`/`unsubscribe` API shape. +- `apps/api/tests/broker/outboxWorker.test.ts` — Full read; confirmed mock infrastructure, existing test coverage, `vi.hoisted` + `vi.mock` pattern. +- `apps/api/vitest.config.ts` — Read; confirmed `fileParallelism: false`, `environment: 'node'`, no setup file override. +- `.planning/phases/09-faster-write-back/09-CONTEXT.md` — Full read; locked decisions D-01…D-10. +- `.planning/REQUIREMENTS.md` — Read; CAL-15 requirement text confirmed. +- `.planning/config.json` — Read; `nyquist_validation` not set to false → Validation Architecture section required; `security_enforcement` defaults to enabled. + +### Secondary (MEDIUM confidence) +- `apps/api/tests/lib/listEmitter.test.ts` — Read for test pattern; confirms `vi.fn()` + `unsub()` test shape usable for trigger tests. + +--- + +## Metadata + +**Confidence breakdown:** +- Standard stack: HIGH — no new packages; all patterns verified in codebase +- Architecture: HIGH — drain control flow fully read and analyzed +- Pitfalls: HIGH — derived from actual code paths, not assumptions +- Test strategy: HIGH — based on existing test file infrastructure; one ASSUMED item on `setImmediate` flush + +**Research date:** 2026-06-12 +**Valid until:** 2026-07-12 (stable internal code — only invalidated by changes to `outboxWorker.ts` or `events.ts`) 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 new file mode 100644 index 0000000..83fe2c3 --- /dev/null +++ b/.planning/phases/09-faster-write-back/09-REVIEW.md @@ -0,0 +1,142 @@ +--- +phase: 09-faster-write-back +reviewed: 2026-06-12T21: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: 0 + info: 2 + total: 2 +status: issues_found +--- + +# Phase 09: Code Review Report + +**Reviewed:** 2026-06-12 +**Depth:** standard +**Status:** issues_found (2 Info; no Blockers, no Warnings) + +## Summary + +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). + +**Fix verification — all four focus areas confirmed correct:** + +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.** + +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.** + +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.** + +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`. + +**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. + +## Narrative Findings (AI reviewer) + +### 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 __resetDrainState(): void { + if (process.env.NODE_ENV === 'production') return; // test-only; never clear the guard in prod + isDraining = false; + drainRequested = false; +} +``` + +(Alternatively, move the reset behind a test-only entry point not reachable from the +production import graph. The env guard is the smaller change.) + +### 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` (in concert with the +`isDraining=false` reset at line 838) + +**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. + +**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-12T21:05:00Z_ +_Reviewer: Claude (gsd-code-reviewer)_ +_Depth: standard_ diff --git a/.planning/phases/09-faster-write-back/09-VALIDATION.md b/.planning/phases/09-faster-write-back/09-VALIDATION.md new file mode 100644 index 0000000..dc65a96 --- /dev/null +++ b/.planning/phases/09-faster-write-back/09-VALIDATION.md @@ -0,0 +1,84 @@ +--- +phase: 9 +slug: faster-write-back +status: draft +nyquist_compliant: false +wave_0_complete: false +created: 2026-06-12 +--- + +# Phase 9 — Validation Strategy + +> Per-phase validation contract for feedback sampling during execution. + +--- + +## Test Infrastructure + +| Property | Value | +|----------|-------| +| **Framework** | Vitest (existing) | +| **Config file** | `apps/api/vitest.config.ts` | +| **Quick run command** | `cd apps/api && npx vitest run tests/broker/outboxWorker.test.ts` | +| **Full suite command** | `cd apps/api && npm test` (→ `vitest run`) | +| **Estimated runtime** | ~5–20 seconds (existing broker suite + new signal cases; no live Fastmail) | + +--- + +## Sampling Rate + +- **After every task commit:** Run quick command (`vitest run tests/broker/outboxWorker.test.ts`) +- **After every plan wave:** Run full suite (`npm test`) + `npm run typecheck` (`tsc --noEmit` — esbuild/Vitest does not catch type errors; see [[vitest-passes-tsc-fails]]) +- **Before `/gsd-verify-work`:** Full suite + typecheck must be green +- **Max feedback latency:** ~20 seconds + +--- + +## Per-Task Verification Map + +> Task IDs are assigned by the planner; rows below map each Success Criterion + CAL-15 to its +> concrete automated assertion (Nyquist coverage). The signal-path tests need no fake timers — +> a `setImmediate`/microtask flush is sufficient to observe the synchronous emit → drain wiring +> (per RESEARCH.md). The 15s `setInterval` fallback is exercised with Vitest fake timers as today. + +| Item | Wave | Requirement | Threat Ref | Expected Behavior | Test Type | Automated Command | File Exists | Status | +|------|------|-------------|------------|-------------------|-----------|-------------------|-------------|--------| +| SC-1 signal → prompt drain | 1 | CAL-15 | — | Enqueuing a pending row publishes the trigger and `runOutboxDrain` runs without waiting for the 15s interval (CalDAV dispatch stubbed) | unit/integration | `vitest run tests/broker/outboxWorker.test.ts` | ✅ extends existing | ⬜ pending | +| SC-2 fire-and-forget 202 | 1 | CAL-15 | — | Publishing the signal is non-blocking; route enqueue path makes no inline CalDAV call (signal is fire-and-forget) | unit | `vitest run tests/broker/outboxWorker.test.ts` | ✅ extends existing | ⬜ pending | +| SC-3 create-before-delete under move | 1 | CAL-15 | — | Edit-as-move publishes once after the transaction commits; CREATE row precedes DELETE; no signal fires between the two inserts | unit | `vitest run tests/broker/outboxWorker.test.ts` | ✅ extends existing | ⬜ pending | +| SC-4 exactly-once overlap | 1 | CAL-15 | — | Concurrent `scheduleOutboxDrain` calls + 15s fallback overlap produce no duplicate CalDAV PUT for the same uid (`isDraining` guard + `pending AND next_attempt_at <= NOW()` selection unchanged) | unit | `vitest run tests/broker/outboxWorker.test.ts` | ✅ extends existing | ⬜ pending | +| SC-5 / D-05 trailing re-drain | 1 | CAL-15 | — | A signal arriving during an in-flight drain sets `drainRequested` and triggers exactly one follow-up drain after the current one finishes; the 15s `setInterval` fallback still runs and recovers missed rows | unit | `vitest run tests/broker/outboxWorker.test.ts` | ✅ extends existing | ⬜ pending | + +*Status: ⬜ pending · ✅ green · ❌ red · ⚠️ flaky* + +--- + +## Wave 0 Requirements + +- Existing infrastructure covers all phase requirements. `apps/api/tests/broker/outboxWorker.test.ts` + already provides the DB / write / sync mock scaffold (`isDraining`, drain, edit-as-move ordering). + New cases are added to that file (or a sibling `tests/broker/outboxTrigger.test.ts`); no new + framework, config, or fixtures are installed. + +--- + +## Manual-Only Verifications + +| Behavior | Requirement | Why Manual | Test Instructions | +|----------|-------------|------------|-------------------| +| — | — | — | — | + +*All phase behaviors have automated verification. No operator-stopwatch checkpoint (D-10): the dev-bypass user (id 1) has no CalDAV credential/calendars, so a live event-create 422s in the dev stack ([[dev-data-user1-no-calendars]]); the automated wiring test is the durable, CI-guarded evidence.* + +--- + +## Validation Sign-Off + +- [ ] All tasks have `` verify or Wave 0 dependencies +- [ ] Sampling continuity: no 3 consecutive tasks without automated verify +- [ ] Wave 0 covers all MISSING references +- [ ] No watch-mode flags (`vitest run`, not `vitest`) +- [ ] Feedback latency < 20s +- [ ] `nyquist_compliant: true` set in frontmatter + +**Approval:** pending diff --git a/.planning/phases/09-faster-write-back/09-VERIFICATION.md b/.planning/phases/09-faster-write-back/09-VERIFICATION.md new file mode 100644 index 0000000..9fa3555 --- /dev/null +++ b/.planning/phases/09-faster-write-back/09-VERIFICATION.md @@ -0,0 +1,102 @@ +--- +phase: 09-faster-write-back +verified: 2026-06-12T17:07:00Z +status: passed +score: 5/5 must-haves verified +overrides_applied: 0 +--- + +# Phase 09: Faster Write-Back Verification Report + +**Phase Goal:** A created, edited, or deleted event reaches Fastmail within ~1-2 seconds (event-driven outbox drain) instead of waiting up to ~15s for the next interval tick — with every existing durability guarantee intact. +**Verified:** 2026-06-12T17:07:00Z +**Status:** passed +**Re-verification:** No — initial verification + +## Goal Achievement + +### Observable Truths + +| # | Truth | Status | Evidence | +|---|-------|--------|----------| +| 1 | After creating/editing/deleting an event, the change is signalled to drain on enqueue (not waited-for on the interval) — the drain fires promptly, not on the 15s tick. | VERIFIED | `signalOutboxDrain()` called at all 4 enqueue sites in events.ts (lines 310, 434, 450, 525). Test SC-1 in outboxWorker.test.ts confirms drain fires via EventEmitter signal without advancing any timer. | +| 2 | The route handler still returns an optimistic 202 immediately and never makes a CalDAV call inline — the signal is fire-and-forget (never awaited). | VERIFIED | No `await signalOutboxDrain` in events.ts. All 4 sites call `signalOutboxDrain();` then immediately `return c.json(..., 202)`. events.test.ts (23 tests) confirms 202 + no-inline-CalDAV contract still holds. | +| 3 | Edit-as-move still writes the new event before deleting the old one (create-before-delete preserved); the signal fires AFTER the db.transaction resolves, never inside it. | VERIFIED | events.ts line 410 opens `await db.transaction(async (tx) => { ... });` closing at line 432. `signalOutboxDrain()` appears at line 434 — after the `await db.transaction(...)` statement, not inside the callback. Confirmed: no `signalOutboxDrain` between the two `tx.insert` calls. | +| 4 | No duplicate CalDAV PUTs for the same outbox row when signal and the 15s fallback overlap (exactly-once per uid preserved via the isDraining guard + drainRequested trailing-re-drain). | VERIFIED | `scheduleOutboxDrain` checks `if (isDraining) { drainRequested = true; return; }` before calling `runOutboxDrain()`. Test D-07 (Test C) verifies two concurrent `scheduleOutboxDrain()` calls result in `createCalendarEvent` called exactly once. Test D-05 (Test B) verifies mid-drain signals collapse to exactly one trailing re-drain. | +| 5 | The 15s setInterval fallback still runs and recovers missed rows. | VERIFIED | `startOutboxWorker` in outboxWorker.ts line 857: `setInterval(() => { scheduleOutboxDrain(); }, 15 * 1000)` — interval length unchanged, body routes through `scheduleOutboxDrain` wrapper. | + +**Score:** 5/5 truths verified + +### Required Artifacts + +| Artifact | Expected | Status | Details | +|----------|----------|--------|---------| +| `apps/api/src/lib/outboxTrigger.ts` | Zero-dependency EventEmitter signal (signalOutboxDrain, onOutboxDrain) | VERIFIED | Exists. Imports only `node:events`. Exports `signalOutboxDrain` and `onOutboxDrain`. No `setMaxListeners`. No internal project imports. | +| `apps/api/src/broker/outboxWorker.ts` | scheduleOutboxDrain + drainRequested + initOutboxTrigger | VERIFIED | Exports `scheduleOutboxDrain`, `initOutboxTrigger`. Declares `let drainRequested = false` at line 172. `drainRequested = false` reset precedes recursive `scheduleOutboxDrain()` call (line 199 before 200 — Pitfall 3 correct). | +| `apps/api/src/routes/events.ts` | Four post-commit signalOutboxDrain() publish sites | VERIFIED | Exactly 4 calls (lines 310, 434, 450, 525). Import at line 37. None awaited. None inside transaction callback or catch blocks. | +| `apps/api/src/index.ts` | initOutboxTrigger() wired under isMainModule(), after startOutboxWorker() | VERIFIED | Line 138: `startOutboxWorker()`. Line 139: `initOutboxTrigger(); // subscribe drain signal listener (D-01)`. Both inside the `if (isMainModule())` block. | +| `apps/api/tests/broker/outboxWorker.test.ts` | Trigger-wiring test block (SC-1, D-05, D-07) — 3 new tests | VERIFIED | `describe('scheduleOutboxDrain — trigger wiring (D-09)', ...)` block at line 747 with `beforeAll(() => { initOutboxTrigger(); })` and 3 tests. All 30 outboxWorker tests pass. | + +### Key Link Verification + +| From | To | Via | Status | Details | +|------|----|-----|--------|---------| +| `outboxWorker.ts (initOutboxTrigger)` | `outboxTrigger.ts (onOutboxDrain)` | `onOutboxDrain(() => scheduleOutboxDrain())` | WIRED | Line 845: `onOutboxDrain(() => scheduleOutboxDrain())`. Import at line 39. | +| `outboxWorker.ts (scheduleOutboxDrain)` | `outboxWorker.ts (runOutboxDrain)` | isDraining guard + drainRequested trailing-re-drain | WIRED | Lines 187-203: guard checks `isDraining`, calls `runOutboxDrain()`, finally resets `drainRequested` before recursive call. | +| `routes/events.ts` | `outboxTrigger.ts (signalOutboxDrain)` | import + call after each enqueue commit | WIRED | Import at line 37. 4 call sites verified present and not awaited. | +| `index.ts (isMainModule block)` | `outboxWorker.ts (initOutboxTrigger)` | `initOutboxTrigger()` after `startOutboxWorker()` | WIRED | Lines 138-139 inside `isMainModule()` guard. | +| `startOutboxWorker setInterval` | `scheduleOutboxDrain` | bare `scheduleOutboxDrain()` call in 15s interval | WIRED | Lines 857-859: `setInterval(() => { scheduleOutboxDrain(); }, 15 * 1000)`. | + +### Data-Flow Trace (Level 4) + +Not applicable — phase adds a signal/trigger path, not a data-rendering path. The signal carries no payload; it triggers an existing drain that queries the DB for pending rows. The drain's data path (outbox rows → CalDAV) is unchanged and was verified in prior phases. + +### Behavioral Spot-Checks + +| Behavior | Command | Result | Status | +|----------|---------|--------|--------| +| SC-1: signalOutboxDrain() triggers drain without timer advance | `npx vitest run tests/broker/outboxWorker.test.ts` — Test A | 30/30 passed | PASS | +| D-05: mid-drain signals collapse to one trailing re-drain | Test B in same run | 30/30 passed | PASS | +| D-07: concurrent scheduleOutboxDrain() calls are exactly-once | Test C in same run | 30/30 passed | PASS | +| 202 + no-inline-CalDAV route contract preserved | `npx vitest run tests/routes/events.test.ts` | 23/23 passed | PASS | +| tsc clean across all phase-9 files | `npx tsc --noEmit` | No output (clean) | PASS | + +### Probe Execution + +No conventional `scripts/*/tests/probe-*.sh` probes declared or found for this phase. + +### Requirements Coverage + +| Requirement | Source Plan | Description | Status | Evidence | +|-------------|------------|-------------|--------|----------| +| CAL-15 | 09-01-PLAN.md, 09-02-PLAN.md | Event reaches Fastmail within ~2s (event-driven drain) instead of up to ~15s, while preserving optimistic-202 and all outbox durability guarantees. | SATISFIED | `signalOutboxDrain()` fires immediately post-enqueue. SC-1 test confirms no timer advance needed. isDraining guard + drainRequested preserve exactly-once. 15s fallback intact. REQUIREMENTS.md marks CAL-15 as Complete. | + +### Anti-Patterns Found + +No debt markers (TBD/FIXME/XXX/TODO/HACK/PLACEHOLDER) found in any phase-9 modified file. No stub patterns found. No `return null`, empty handlers, or hardcoded empty data in the signal path. + +### Human Verification Required + +None. The phase goal (latency reduction via event-driven drain) is fully verifiable through automated tests and static code inspection: + +- SC-1 (no 15s wait): automated by the trigger-wiring Vitest test using `setImmediate` flush — no stopwatch needed. +- D-04 (fire-and-forget): verified by static grep — no `await signalOutboxDrain` anywhere. +- D-03 (signal after transaction): verified by static line-number inspection — `signalOutboxDrain()` at line 434 is after `await db.transaction(...)` closes at line 432. +- Fallback interval: verified by reading the `startOutboxWorker` body. + +### Gaps Summary + +No gaps. All 5 must-have truths are VERIFIED with direct codebase evidence: + +1. All 4 enqueue sites in events.ts call `signalOutboxDrain()` post-commit, pre-202. +2. No call is awaited; the signal is synchronous EventEmitter emit. +3. The edit-as-move signal is provably outside the transaction callback at the source level. +4. The `isDraining` guard + `drainRequested` boolean collapses concurrent/mid-drain signals correctly; verified by 2 dedicated automated tests. +5. The 15s setInterval fallback routes through `scheduleOutboxDrain()` at the same 15-second cadence. + +CAL-15 is marked Complete in REQUIREMENTS.md (line 78) and all its behavioral guarantees are implemented and tested. + +--- + +_Verified: 2026-06-12T17:07:00Z_ +_Verifier: Claude (gsd-verifier)_ diff --git a/apps/api/src/broker/outboxWorker.ts b/apps/api/src/broker/outboxWorker.ts index 962aa56..2b9a2a7 100644 --- a/apps/api/src/broker/outboxWorker.ts +++ b/apps/api/src/broker/outboxWorker.ts @@ -17,7 +17,9 @@ * T-03-14: create-before-delete ordering; create-fail aborts delete. * * runOutboxDrain is exported for unit testing. - * startOutboxWorker wraps it in a 15-second setInterval. + * scheduleOutboxDrain is exported for unit testing; it wraps runOutboxDrain with the isDraining + * guard + drainRequested trailing-re-drain loop (D-05). + * startOutboxWorker wraps scheduleOutboxDrain in a 15-second setInterval. * (node-cron 4.2.1 silently skipped scheduled executions in the long-running server process; * setInterval fires reliably in the same process — replaced to fix the silent skip.) * @@ -34,6 +36,7 @@ import { createCalendarEvent, updateCalendarEvent, deleteCalendarEvent } from '. import { buildVeventString, extractRruleString, RRULE_PRESETS } from './vevent.js'; import type { FastmailClient } from './client.js'; import { dispatchEventChange } from '../lib/eventChangeDispatcher.js'; +import { onOutboxDrain } from '../lib/outboxTrigger.js'; // ── Constants (D-07) ──────────────────────────────────────────────────────── @@ -140,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) ────────────────────────────────────────── /** @@ -158,6 +211,47 @@ export function assembleRruleString( */ let isDraining = false; +/** + * D-05 / D-06: trailing-re-drain flag. + * Set to true when signalOutboxDrain() fires while a drain is already in flight. + * Collapses any number of mid-drain signals into exactly one trailing drain — never + * rate-limited, never dropped, never more than one extra pass (D-06). + * Reset to false BEFORE the recursive scheduleOutboxDrain() call (Pitfall 3 — resetting + * after would allow an unbounded re-drain chain against Fastmail rate limits / T-09-01). + */ +let drainRequested = false; + +/** + * Schedule an outbox drain pass. + * + * If no drain is currently running, kicks off runOutboxDrain() immediately. + * If a drain IS running (isDraining=true), records drainRequested=true so the + * currently-running drain triggers exactly one trailing re-drain on completion (D-05). + * + * Errors from runOutboxDrain are caught and logged (D-02 / T-09-03). + * + * Called by: + * - initOutboxTrigger's onOutboxDrain listener (event-driven path, CAL-15) + * - startOutboxWorker's 15-second setInterval (polling fallback, D-08) + */ +export function scheduleOutboxDrain(): void { + if (isDraining) { + drainRequested = true; + return; + } + runOutboxDrain() + .catch((err: unknown) => { + console.error('[outboxWorker] Unhandled runOutboxDrain error:', err); + }) + .finally(() => { + if (drainRequested) { + // Reset BEFORE recursive call (Pitfall 3) — prevents unbounded re-drain chain + drainRequested = false; + scheduleOutboxDrain(); + } + }); +} + /** * WR-06: max time to wait on the post-write targeted re-sync before marking the * outbox row 'done'. A stalled Fastmail connection cannot wedge the single-process @@ -378,33 +472,13 @@ async function dispatchRow(row: OutboxRow): Promise { // 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, @@ -476,33 +550,13 @@ async function dispatchRow(row: OutboxRow): Promise { // 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, @@ -788,15 +842,60 @@ export async function runOutboxDrain(): Promise { // ── Scheduler ──────────────────────────────────────────────────────────────── /** - * Starts the 15-second background outbox drain schedule. + * Register the in-process EventEmitter drain signal listener (CAL-15 / D-01). + * Call once at API startup (in index.ts, after startOutboxWorker). + * After registration, any signalOutboxDrain() call (fired post-enqueue) immediately + * routes through scheduleOutboxDrain — eliminating the up-to-15s polling delay. + * + * 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 { + 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; +} + +/** + * Starts the 15-second background outbox drain schedule (polling fallback, D-08). * Call once at API startup (wired in index.ts beside startBrokerPoller). * Uses setInterval instead of node-cron: node-cron 4.2.1 silently skipped executions * in the long-running server process; setInterval fires reliably. + * The interval body calls scheduleOutboxDrain() so errors are absorbed by its .catch + * and mid-drain signals collapse correctly via drainRequested (D-05). */ export function startOutboxWorker(): void { setInterval(() => { - runOutboxDrain().catch((err: unknown) => { - console.error('[outboxWorker] Unhandled runOutboxDrain error:', err); - }); + scheduleOutboxDrain(); }, 15 * 1000); } diff --git a/apps/api/src/index.ts b/apps/api/src/index.ts index 4cd8748..b49262d 100644 --- a/apps/api/src/index.ts +++ b/apps/api/src/index.ts @@ -13,7 +13,7 @@ import { oidcAuthMiddleware, processOAuthCallback } from './auth/middleware.js'; import { devAuthBypass } from './auth/devBypass.js'; import { persistSessionCookie } from './auth/persistSessionCookie.js'; import { startBrokerPoller } from './broker/poller.js'; -import { startOutboxWorker } from './broker/outboxWorker.js'; +import { startOutboxWorker, initOutboxTrigger } from './broker/outboxWorker.js'; import { startReminderScheduler } from './broker/reminderScheduler.js'; import webpush from 'web-push'; @@ -136,6 +136,7 @@ if (isMainModule()) { startBrokerPoller(); // Drain the D-05 outbox every 15s: dispatches pending CalDAV writes to Fastmail. startOutboxWorker(); + initOutboxTrigger(); // subscribe drain signal listener (D-01) // Start the 1-min reminder scan for shared timed events starting in ~15 min (NOTIF-01). // VAPID must be configured (above) before this starts or push sends will fail. startReminderScheduler(); diff --git a/apps/api/src/lib/outboxTrigger.ts b/apps/api/src/lib/outboxTrigger.ts new file mode 100644 index 0000000..b790d66 --- /dev/null +++ b/apps/api/src/lib/outboxTrigger.ts @@ -0,0 +1,47 @@ +/** + * In-process outbox drain signal (D-01 / D-03 / D-04). + * + * Module-level singleton EventEmitter — one emitter shared across all callers + * in this Node.js process. Carries a zero-payload 'drain' event: signal is + * fire-and-forget; no data crosses this boundary (D-04). + * + * Single-subscriber design: + * - signalOutboxDrain() is called only after a successful enqueue DB commit (D-01/D-03). + * - The sole listener is registered by initOutboxTrigger() in outboxWorker.ts. + * - With exactly one subscriber the default EventEmitter limit of 10 is correct. + * - Do NOT call setMaxListeners — unlike listEmitter.ts (T-04-04, 200 SSE fan-out), + * this emitter never fans out to multiple listeners. + * + * Exports: signalOutboxDrain, onOutboxDrain + */ + +import { EventEmitter } from 'node:events'; + +// Module-level singleton — one emitter shared across all route handlers +// in this Node.js process. +const emitter = new EventEmitter(); + +/** + * Fire a drain signal after a successful outbox enqueue commit. + * 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 { + queueMicrotask(() => emitter.emit('drain')); +} + +/** + * Register a handler to be invoked on each drain signal. + * Returns an unsubscribe function; call it to stop delivery to this handler. + */ +export function onOutboxDrain(handler: () => void): () => void { + emitter.on('drain', handler); + return () => emitter.off('drain', handler); +} diff --git a/apps/api/src/routes/events.ts b/apps/api/src/routes/events.ts index aef146e..ce8d899 100644 --- a/apps/api/src/routes/events.ts +++ b/apps/api/src/routes/events.ts @@ -34,6 +34,7 @@ import { expandOccurrences } from '../broker/expand.js'; import { extractRruleString } from '../broker/vevent.js'; import { getAuth } from '../auth/middleware.js'; import { upsertUser, deriveDisplayName } from '../auth/user.js'; +import { signalOutboxDrain } from '../lib/outboxTrigger.js'; // Side-effect import: brings in the ContextVariableMap augmentation for c.get('user') import '../auth/devBypass.js'; @@ -306,6 +307,8 @@ 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) { console.error('[events/create] DB operation failed:', err); @@ -429,6 +432,8 @@ 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); } @@ -444,6 +449,8 @@ 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) { console.error('[events/edit] DB operation failed:', err); @@ -518,6 +525,8 @@ 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) { console.error('[events/delete] DB operation failed:', err); diff --git a/apps/api/tests/broker/outboxWorker.test.ts b/apps/api/tests/broker/outboxWorker.test.ts index ab1fe8c..7223409 100644 --- a/apps/api/tests/broker/outboxWorker.test.ts +++ b/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,20 @@ * 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 } 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 } 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 ──────────────────────────────────────────────────────── // Follows the pattern from PATTERNS.md §Drizzle DB mock in tests. @@ -666,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', () => { @@ -700,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(() => { @@ -734,3 +736,131 @@ describe('FREQ persistence (D-07 regression)', () => { expect(capturedIcsString as string).not.toContain('FREQ=WEEKLY'); }); }); + +// ─── scheduleOutboxDrain trigger-wiring tests (D-09) ───────────────────────── +// These tests verify the trigger-wiring behavior introduced in Plan 09-01: +// 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 + +describe('scheduleOutboxDrain — trigger wiring (D-09)', () => { + // Wire the EventEmitter signal → scheduleOutboxDrain once for this describe block. + // initOutboxTrigger registers the 'drain' listener that connects signalOutboxDrain() + // to scheduleOutboxDrain(). Called once (not per-test) to avoid listener accumulation. + beforeAll(() => { + 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(); + }); + + // Test A (SC-1): signalOutboxDrain() triggers a drain immediately — no 15s wait. + // With one pending row and createCalendarEvent mocked to 201, calling signalOutboxDrain() + // then flushing microtasks results in createCalendarEvent called exactly once. + // No vi.useFakeTimers() — drain fires via the EventEmitter signal, not the interval. + it('SC-1: signalOutboxDrain() triggers drain promptly — createCalendarEvent called once without advancing timers', async () => { + const { createCalendarEvent } = await import('../../src/broker/write.js'); + vi.mocked(createCalendarEvent).mockResolvedValue(makeResponse(201)); + mockPendingRows = [makeRow()]; + + signalOutboxDrain(); + + // Flush microtasks so the async drain has a chance to run + await new Promise((resolve) => setImmediate(resolve)); + // One more flush to let the drain's internal async steps complete + await new Promise((resolve) => setImmediate(resolve)); + + expect(createCalendarEvent).toHaveBeenCalledTimes(1); + }); + + // Test B (SC-4 / D-05): a signal arriving during an in-flight drain triggers exactly + // one trailing re-drain — not two, not zero. + it('D-05: two signals mid-drain collapse to exactly one trailing re-drain', async () => { + const { createCalendarEvent } = await import('../../src/broker/write.js'); + + // Capture the resolve function so we can hold the first drain in flight + let resolveFirst!: () => void; + const firstDone = new Promise((resolve) => { + resolveFirst = resolve; + }); + + // First call: held until we release it; subsequent calls resolve immediately + vi.mocked(createCalendarEvent) + .mockImplementationOnce(async () => { + await firstDone; + return makeResponse(201); + }) + .mockResolvedValue(makeResponse(201)); + + // Two pending rows: drain 1 picks up row 1, trailing drain picks up row 2 + const row1 = makeRow({ id: 1, uid: 'uid-1@familysync' }); + const row2 = makeRow({ id: 2, uid: 'uid-2@familysync' }); + mockPendingRows = [row1]; + + // Start drain 1 (via signal) — it blocks on firstDone + signalOutboxDrain(); + await new Promise((resolve) => setImmediate(resolve)); + + // While drain 1 is in flight, send two more signals — both should collapse to one trailing drain + mockPendingRows = [row2]; + signalOutboxDrain(); + signalOutboxDrain(); + + // Release drain 1 — this lets it finish, then the trailing re-drain fires + resolveFirst(); + + // Flush: drain 1 finally-block + trailing scheduleOutboxDrain() + trailing drain steps + await new Promise((resolve) => setImmediate(resolve)); + await new Promise((resolve) => setImmediate(resolve)); + await new Promise((resolve) => setImmediate(resolve)); + + // Drain 1 called createCalendarEvent once (row1); trailing drain called it once (row2). + // Total = 2 — never a third pass. + expect(createCalendarEvent).toHaveBeenCalledTimes(2); + }); + + // Test C (SC-4 / D-07): two concurrent scheduleOutboxDrain() calls dispatch each row + // exactly once — the second call is a no-op via the isDraining guard. + // The trailing re-drain (triggered by drainRequested) finds 0 pending rows so + // createCalendarEvent is called exactly once total. + it('D-07: two concurrent scheduleOutboxDrain() calls invoke createCalendarEvent exactly once', async () => { + vi.useFakeTimers(); + try { + const { createCalendarEvent } = await import('../../src/broker/write.js'); + // Slow create so the second scheduleOutboxDrain starts while first is still running + vi.mocked(createCalendarEvent).mockImplementation( + () => new Promise((resolve) => setTimeout(() => resolve(makeResponse(201)), 20)), + ); + const row = makeRow({ id: 1 }); + // First pending-rows query returns the row; subsequent queries return empty + // (simulates: drain 1 processes and removes the row; trailing drain finds nothing) + mockWherePending + .mockImplementationOnce(() => Promise.resolve([row])) + .mockImplementation(() => Promise.resolve([])); + + // Both calls are synchronous — second sees isDraining=true and sets drainRequested=true + scheduleOutboxDrain(); + scheduleOutboxDrain(); + + await vi.runAllTimersAsync(); + + // Drain 1 dispatched the row once; trailing drain found 0 rows → createCalendarEvent once total + expect(createCalendarEvent).toHaveBeenCalledTimes(1); + } finally { + vi.useRealTimers(); + } + }); +});