12 KiB
12 KiB
phase, plan, type, wave, depends_on, files_modified, autonomous, requirements, must_haves
| phase | plan | type | wave | depends_on | files_modified | autonomous | requirements | must_haves | ||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| quick-260610-hbu | 01 | execute | 1 |
|
true |
|
|
Purpose: A scheduled 15-min reminder did NOT fire on a real iOS device because node-cron missed the tick during the 2-min window and the scan had no catch-up — the window slid past and the reminder was dropped permanently. The dispatch pipeline is PROVEN working (manual injected-time scan -> Apple 201 -> notification arrived). This is purely a scheduling-resilience defect.
Output: Updated reminderScheduler.ts (in-memory only, no schema/deps/Redis per D-12) and updated tests, both passing typecheck + vitest.
<execution_context> @$HOME/.claude/gsd-core/workflows/execute-plan.md @$HOME/.claude/gsd-core/templates/summary.md </execution_context>
@./CLAUDE.md @apps/api/src/broker/reminderScheduler.ts @apps/api/tests/broker/reminderScheduler.test.ts @apps/api/src/lib/pushDispatcher.ts @.planning/phases/05-web-push-notifications/05-UAT.md Task 1: Make the reminder scan catch-up resilient with per-uid exactly-once dedup apps/api/src/broker/reminderScheduler.ts Target behaviours (proven by Task 2 tests): - Window: scan shared timed events whose `dtstartUtc` is in `(now, now+16min]` — strictly greater than `now` (future, excludes already-started) and at most 16 min out. A missed/late tick is recovered on the next run because the lower bound is `now`, not `now+14min`. - Dedup: keyed on `uid` ALONE. The same event fires EXACTLY ONCE no matter how many consecutive ticks it sits in the window. - Mark-after-dispatch (WR-01 preserved): mark the uid as sent only AFTER all dispatch attempts for that event complete, so a throwing dispatch does not pre-suppress retry (at-least-once delivery property). - Bounded memory / re-arm: track per-uid the event's `dtstart` (ms). Prune entries whose dtstart is in the past (`dtstart <= now`) — bounds growth and avoids permanently suppressing a future re-created uid. - Body: lead-accurate. Compute `minutes = Math.max(1, Math.round((dtstart - now) / 60000))` and set body to `` `Starts in ${minutes} min` ``. Edit apps/api/src/broker/reminderScheduler.ts in place. Keep the injectable `now = new Date()` signature on `runReminderCheck` (tests rely on it) and keep `startReminderScheduler` unchanged.Query / window changes:
- Replace `windowStart = now+14min` / `gte(dtstartUtc, windowStart)` with a strict lower bound on `now`: use Drizzle `gt(calendarEvents.dtstartUtc, now)`. Import `gt` from drizzle-orm and drop the now-unused `gte` import.
- Keep `windowEnd = now + 16*60*1000` and `lte(calendarEvents.dtstartUtc, windowEnd)`.
- Leave the rest of the query identical: innerJoin calendars, `eq(calendars.isShared, true)` (D-05 in QUERY, T-05-17), `eq(calendarEvents.allDay, false)` (D-07), cross-join ALL pushSubscriptions via `sql\`1=1\``, the same selected columns. Pass the raw `now` Date to `gt` (Drizzle/mysql2 binds Date params for datetime columns the same way the existing `lte` does).
Dedup changes:
- Replace `const sentReminders = new Set<string>()` with `const sentReminders = new Map<string, number>()` mapping `uid -> dtstartMs`. Update the module header comment block: key is now the bare event `uid` (not `uid:minuteBucket`); value is the event's dtstart in ms used for pruning; "acceptable data loss on process restart" (D-12) still applies. Remove the `minuteBucket` variable entirely.
- In the dispatch loop: `if (sentReminders.has(uid)) continue` (no minuteBucket). After the fan-out loop, `sentReminders.set(uid, event.dtstartUtc.getTime())` (mark-after-dispatch — WR-01).
- Build the notification body from the actual lead: `const minutes = Math.max(1, Math.round((event.dtstartUtc.getTime() - now.getTime()) / 60000))` then `body: \`Starts in ${minutes} min\``. Keep title fallback (`event.title ?? uid`), `tag: \`reminder-${uid}\``, and `navigate` exactly as-is.
Pruning changes:
- Replace the CR-01 minuteBucket-based prune block with: iterate `sentReminders` entries and `sentReminders.delete(uid)` for any entry whose stored dtstartMs is `<= now.getTime()` (event has started). Update the CR-01 comment to describe started-event pruning.
Comments / decisions to preserve and update: D-05, D-07, D-11, D-12, D-16, T-05-17, T-05-18, T-05-19, WR-01, CR-01 must remain referenced in the header/inline comments with their meanings updated for per-uid dedup and the catch-up window. Add a short inline CAVEAT comment near the dedup: if an event's dtstart is RESCHEDULED earlier after a reminder already fired, it will not re-fire — acceptable for v1.
Do NOT: add schema/migration changes, new dependencies, Redis, or DB persistence. Do NOT change pushDispatcher or its call signature. Do NOT place fenced code blocks anywhere — this is directive prose.
Because `sentReminders` is module-level state, each independent test must get a fresh module via the existing `vi.resetModules()` + dynamic `await import(...)` pattern already used. Keep importing `runReminderCheck`, `db`, `dispatchPush` inside each test after `resetModules`.
Rewrite/replace the dedup tests for per-uid semantics:
- Replace the "(eventUid, minuteBucket) twice within the same run" test and the CR-01 "re-dispatch in next minute bucket" test (which asserted the OLD per-minuteBucket re-fire) with a SINGLE-FIRE-ACROSS-TICKS test: set an event row with `dtstartUtc` = now+15min and one subscriber; call `runReminderCheck(t0)`, then `runReminderCheck(t0 + 1 min)`, then `runReminderCheck(t0 + 2 min)` — each time re-mocking `db.select` (event still in window: 15, 14, 13 min out). Assert `dispatchPush` was called exactly ONCE total across all three runs.
- Add a MISSED-TICK-RECOVERY test: event `dtstartUtc` fixed (e.g. `2026-06-15T10:15:00Z`). Do NOT run at the ideal mark. Call `runReminderCheck` with `now = 2026-06-15T10:07:00Z` (event 8 min out, still > now and <= now+16min). Assert `dispatchPush` fired (>= 1 call). This is the Test 1 regression.
- Add an ALREADY-STARTED test: with the real query this is excluded by `gt(dtstartUtc, now)` in SQL, so the mocked `.where()` returns `[]` (mirror the existing all-day/non-shared "SQL filtered it out -> empty rows" pattern) and assert `dispatchPush` NOT called. Name it clearly (e.g. "does not dispatch for an event whose start has already passed (dtstart <= now)").
- Update the WR-01 test description and any "minute bucket" wording to per-uid dedup language, but keep its assertion (second run in the same scan context does not re-dispatch).
- Keep the all-day (D-07), non-shared (D-05) empty-result tests as-is. Update the file's top docblock to describe the catch-up `(now, now+16min]` window and per-uid exactly-once dedup instead of the old `[now+14,now+16]` / minuteBucket description.
Do NOT place fenced code blocks in this action prose. Do NOT introduce real DB or real push; keep everything mocked.
<success_criteria>
- A shared timed event in
(now, now+16min]fires a reminder on the next scan after a missed/late tick (catch-up) — Test 1 closed. - Each such event fires EXACTLY ONCE across consecutive ticks (cross-tick double-fire bug fixed).
- Events with
dtstart <= nownever fire. - Notification body reflects actual lead time, guarded to >= 1 min.
- All prior Phase-5 decisions/threat mitigations preserved (D-05/D-07/D-11/D-12/D-16, T-05-17/18/19, WR-01, CR-01).
- In-memory only: no schema change, no new deps, no Redis/DB persistence. Only
reminderScheduler.tsand its test touched. </success_criteria>