fix(cron): make event reminders idempotent with per-RSVP reminderSentAt#249
Conversation
dkaygithub
left a comment
dkaygithub
left a comment
There was a problem hiding this comment.
Clean, well-scoped idempotency fix — the reminderSentAt: null filter + stamp-after-send is the right shape, and the failure handling is actually a strength: any send/stamp rejection makes Promise.all reject → 500 → cron retry, and the already-stamped RSVPs are excluded on the retry, so the stamp is what makes retries safe rather than duplicative. Verified the schema dependency (@@id([eventId, participantId]) makes the eventId_participantId update key valid) and that classifications.ts is generator output matching schema order. Three non-blocking notes below.
| participantId: rsvp.participantId | ||
| } | ||
| }, | ||
| data: { reminderSentAt: new Date() } |
There was a problem hiding this comment.
Worth naming the guarantee explicitly: this is at-least-once with best-effort dedup, not exactly-once. The one residual double-send window is send-succeeds-then-stamp-fails — if sendNotification resolves but this rSVP.update throws (DB hiccup), the notification went out but reminderSentAt stays null, so the next run re-sends. That's fundamental (you can't transactionally bind an external send to a DB write) and a huge improvement over the old every-overlapping-run behavior, so I'd accept it — just good to acknowledge it's not airtight. Same applies if two cron executions ever overlap (both read null before either stamps); fine as long as the platform doesn't run the job concurrently with itself.
| where: { status: 'ATTENDING' } | ||
| // Only RSVPs that haven't been reminded yet — the window can overlap | ||
| // across runs, so reminderSentAt is what makes this idempotent. | ||
| where: { status: 'ATTENDING', reminderSentAt: null } |
There was a problem hiding this comment.
reminderSentAt is never cleared, so the dedup key is per-RSVP for all time, not per-scheduled-time. Two consequences worth a conscious decision: (1) if an event is rescheduled later, already-reminded attendees won't get a fresh 2h reminder for the new time; (2) an ATTENDING → NOT_ATTENDING → ATTENDING toggle won't re-remind. Both are probably acceptable for this feature, but flagging so it's intentional rather than incidental.
| @@ -0,0 +1,2 @@ | |||
| -- AlterTable | |||
| ALTER TABLE "RSVP" ADD COLUMN "reminderSentAt" TIMESTAMP(3); | |||
There was a problem hiding this comment.
Deploy ordering (you flagged this in the PR body — confirming the state): dev is covered, deploy-dev.yml runs prisma migrate deploy (with retries) before the app comes up, and CI runs migrate deploy + migrate diff --exit-code so a mismatched migration is caught. Prod has no deploy workflow yet — that's #218 (still open), so whoever lands prod deploy needs to ensure migrate deploy runs before the new code, otherwise the reminderSentAt: null filter hits a missing column and the cron 500s. Not blocking this PR, just a cross-PR dependency to track.
…ery guarantee Addresses non-blocking review notes on #249. - editTime now resets RSVP.reminderSentAt to null when an event's START moves (single event and applyToFuture series), so rescheduled events re-qualify for a fresh 2h reminder. End-only edits and no-op start edits leave reminder state untouched. Cancel paths already deleteMany the RSVPs, so no reminder leak there. - Document the cron reminder delivery guarantee as at-least-once with best-effort dedup (not exactly-once): the residual double-send window is send-succeeds-then-stamp-fails, and the route assumes the platform never runs the job concurrently with itself. - New eventRescheduleClearsReminder.integration.test.ts: start-shift clears + cron re-notifies once; end-only and same-start edits do not clear; series shift clears across the group. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
GET /api/cron/reminders selected events in a [now+2h, now+2h15m] window with no dedup. On a 15m-interval cron, an event near a window boundary matched on two consecutive runs, double-sending EVENT_STARTING_SOON to the same ATTENDING RSVPs. Add a nullable reminderSentAt timestamp to RSVP. The query now excludes RSVPs that already have it set, and the route stamps it only after sendNotification resolves — so a send failure leaves the RSVP eligible on the next run rather than silently dropping the reminder. Add cronRemindersEdge.integration.test.ts: a boundary event (start +2h5m) run through GET twice asserts the first run notifies once, the second is a no-op, and the RSVP is notified exactly once across both runs. Note: prod must run `prisma migrate deploy` before the cron picks up the new reminderSentAt: null filter. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ery guarantee Addresses non-blocking review notes on #249. - editTime now resets RSVP.reminderSentAt to null when an event's START moves (single event and applyToFuture series), so rescheduled events re-qualify for a fresh 2h reminder. End-only edits and no-op start edits leave reminder state untouched. Cancel paths already deleteMany the RSVPs, so no reminder leak there. - Document the cron reminder delivery guarantee as at-least-once with best-effort dedup (not exactly-once): the residual double-send window is send-succeeds-then-stamp-fails, and the route assumes the platform never runs the job concurrently with itself. - New eventRescheduleClearsReminder.integration.test.ts: start-shift clears + cron re-notifies once; end-only and same-start edits do not clear; series shift clears across the group. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thpr
force-pushed
the
claude/adoring-mclaren-66e417
branch
from
e6730db to
10bd3fc
Compare
✅ Deployed to devCommit
|
2 participants
What
GET /api/cron/remindersselected events in a[now+2h, now+2h15m]window with no per-RSVP dedup. On a 15-minute-interval cron, an event sitting near a window boundary matched on two consecutive runs →EVENT_STARTING_SOONdouble-sent to the sameATTENDINGRSVPs.Fix
reminderSentAt DateTime?toRSVP(@sensitivity:internal) + migration20260611000000_add_rsvp_reminder_sent_at.reminderSentAt: null; the timestamp is stamped only aftersendNotificationresolves — a send failure leaves the RSVP eligible next run instead of silently dropping the reminder.cronRemindersEdge.integration.test.ts— boundary event (start +2h5m) run throughGETtwice asserts run 1 notifies once, run 2 is a no-op, RSVP notified exactly once across both runs.Reviewer notes
prisma migrate deploybefore the cron picks up the newreminderSentAt: nullfilter (column must exist).prisma db push, full integration suite 327 passed / 54 suites (both cron suites green). Pre-commit hook was bypassed with--no-verify(user-authorized) — it false-fails in worktrees per the documentedtestPathIgnorePatternsrootDir issue, not a real test failure.🤖 Generated with Claude Code