Cal.com / PR #25

Comprehensive workflow reminder management for booking lifecycle events

calcom/cal.com#7232
Golden recall: 100% Extra findings: 12

Golden Comments (2/2 found)

# Comment Severity Status FriendlyReviewer Detail
#1 Asynchronous functions deleteScheduledEmailReminder and deleteScheduledSMSReminder are called without await inside forEach loops. This occurs during booking rescheduling/cancellation, and workflow/workflow step deletion/updates. Consequently, scheduled workflow reminders may not be reliably cancelled, potentially leaving them active. MEDIUM ✓ Found "handleCancelBooking fires deleteScheduledEmailReminder/deleteScheduledSMSReminder inside a forEach without awaiting, so the cancelled: true write may not persist before the handler returns, letting the stale email fire" (packages/features/bookings/lib/handleCancelBooking.ts L484)
#2 When immediateDelete is true, the deleteScheduledEmailReminder function cancels the SendGrid email but fails to delete the corresponding WorkflowReminder record from the database. This creates orphaned database entries and is inconsistent with the immediateDelete: false path, which marks the record as cancelled. The SendGrid DELETE API call is also omitted in this path. HIGH ✓ Found "In the immediateDelete branch, the function POSTs cancel to SendGrid and returns without deleting the workflowReminder row or setting cancelled: true. This leaves orphaned rows, diverges from the deferred path, and reintroduces the pending-cancellation accumulation that broke #6991" (packages/features/ee/workflows/lib/reminders/emailReminderManager.ts L213)

Supplementary Findings (12 findings)

Finding File Severity Legitimate?
The cron cancellation loop awaits each SendGrid request inside a single try/catch — one failure aborts all remaining cancellations and the queued deletes, and may re-fail on retry scheduleEmailReminders.ts:53 HIGH ✓ Real — a single SendGrid failure silently starves every remaining cancellation
Reschedule path calls deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true) (immediateDelete) — immediate SendGrid cancel, row left un-flagged/un-deleted, 100-pending-cancellation risk reintroduced handleNewBooking.ts:968 HIGH ✓ Real — reintroduces the exact limit the PR exists to avoid
The bookings.tsx reschedule handler calls the cancellation functions without awaiting — the cancelled flag is not durably written bookings.tsx:487 HIGH ✓ Real — same fire-and-forget class as golden #1
Workflow delete mutation cancels reminders fire-and-forget then cascade-deletes the workflowReminder rows — a failed cancellation has no retry path and the email/SMS can still be sent workflows.tsx:218 HIGH ✓ Real — email can still be sent with no recovery path
The try/catch in handleNewBooking wraps a forEach that starts async functions without awaiting — it cannot catch async rejections; the cancellation is fire-and-forget with no error handling handleNewBooking.ts:961 MEDIUM ✓ Real — the guard is structurally useless
update uses two cancellation strategies: removed event types defer to the cron (cancelled:true), while deleted workflow steps cancel immediately workflows.tsx:378 MEDIUM ✓ Borderline: real inconsistency, but both strategies do cancel
Step-deleted path fetches reminders without scheduled/referenceId filters and races the unawaited deletions with the cascade delete, possibly causing swallowed P2025 errors workflows.tsx:521 MEDIUM ✓ Real — unfiltered fetch plus unawaited delete race
The cron's remindersToCancel query filters only on cancelled and scheduledDate — no method filter, no referenceId: { not: null } scheduleEmailReminders.ts:41 LOW ✓ Real — would act on non-email reminders if any were present
The migration adds cancelled as nullable without a default; DEFAULT false would make the lifecycle explicit migration.sql:2 LOW ✓ Borderline: style choice, only risky if code ever forgets the value
Recurring/seat-only cancellations: the seat-removal path is never confirmed to handle reminders — stale emails may be sent for removed seats handleCancelBooking.ts:487 LOW ✓ Borderline: a confirmation request, not a verified defect
The cancelled flag is only ever read by the cron; repeated step edits can accumulate stale rows unless the cron deletes them workflows.tsx:576 LOW ✓ Real but minor — rows accumulate until the cron cleans them
The PhoneInput required attribute is not backed by RHF rules; sendTo validation is only confirmed elsewhere WorkflowStepContainer.tsx:390 LOW ✓ Borderline: pre-existing and outside this PR's scope

Honest Analysis

FriendlyReviewer found both golden comments of this PR — recall 100%. The unawaited forEach (golden #1) is caught at handleCancelBooking.ts L484 with the exact mechanism: the cancelled: true write may not persist before the handler returns, letting the stale email fire. The immediateDelete orphan (golden #2) is caught at emailReminderManager.ts L213, and the finding adds the context the reference comment only implies: the orphaned row and the omitted SendGrid DELETE diverge from the deferred path and reintroduce the pending-cancellation accumulation that broke the pre-existing reminder flow (#6991).

The review then generalizes the defect class well beyond the two goldens: the same unawaited pattern appears in the bookings.tsx reschedule handler (L487) and the workflow delete mutation (L218), the cron cancellation loop aborts on the first SendGrid error and starves every remaining cancellation (L53), and the reschedule path still calls immediateDelete (L968) — arguably as severe as golden #2, since it reintroduces the exact 100-pending-cancellation limit the PR exists to avoid. On that basis the changes-required verdict is well supported: this is not a review that confirms 'the flag exists', it is one that establishes the flag is not reliably written in any path that matters.

The 12 supplementary findings split 4 high, 3 medium, 5 low. The two most substantive are the cron abort and the reschedule immediateDelete; the lows are individually legitimate but mostly cosmetic (a migration default, a missing RHF rule, stale-row accumulation), and one is explicitly pre-existing (the PhoneInput at WorkflowStepContainer.tsx L390). No finding looks like a false positive, though several lows would not by themselves justify blocking a merge.

FriendlyReviewer's full review

As posted on the PR — the complete output from FriendlyReviewer.

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech 🌥️ Feat
6 high, 3 medium, 5 low 2 issues, 1 mitigated

The MR introduces a cancelled flag on WorkflowReminder and defers actual Email/SMS cancellation to the scheduleEmailReminders cron job to avoid exceeding SendGrid's 100-pending-cancellation limit. The core deferred mechanism is present, but the implementation has critical gaps: several cancellation calls are fire-and-forget (never awaited) in the booking cancel, reschedule, and workflow-delete paths, so the cancelled flag may not be persisted and emails can still be sent for cancelled/rescheduled bookings. Additionally, the reschedule path (handleNewBooking) and the workflow delete path still use the immediate SendGrid cancel, which reintroduces the exact 100-pending issue the PR aims to fix, and leaves orphaned reminder rows. The cron cancellation loop aborts on the first SendGrid error, starving remaining cancellations, and its query lacks a method filter. A last-hour race remains where cancellations within ~1h of send may still deliver. These issues require changes before merge.

These might need a close look

  • 🔴 packages/features/ee/workflows/lib/reminders/emailReminderManager.ts (L213)
    In the immediateDelete branch, the function POSTs cancel to SendGrid and returns without deleting the workflowReminder row or setting cancelled: true. This leaves orphaned rows, diverges from the deferred path, and reintroduces the pending-cancellation accumulation that broke #6991.
  • 🔴 packages/features/ee/workflows/api/scheduleEmailReminders.ts (L53)
    The cron cancellation loop awaits each SendGrid request inside a single try/catch; a single failure aborts all remaining cancellations and the queued deletes, and may re-fail on retry.
  • 🔴 packages/features/bookings/lib/handleNewBooking.ts (L968)
    The reschedule path in handleNewBooking calls deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true) (immediateDelete), which performs an immediate SendGrid cancel and leaves the row un-flagged/un-deleted, inconsistent with handleCancelBooking and the other reschedule path, and reintroduces the 100-pending-cancellation risk.
  • 🔴 packages/features/bookings/lib/handleCancelBooking.ts (L484)
    handleCancelBooking fires deleteScheduledEmailReminder/deleteScheduledSMSReminder inside a forEach without awaiting, so the cancelled: true write may not persist before the handler returns, letting the stale email fire.
  • 🔴 packages/trpc/server/routers/viewer/bookings.tsx (L487)
    The bookings.tsx reschedule handler similarly calls cancellation functions without awaiting; collect and await the promises to ensure the cancelled flag is durably written.
  • 🔴 packages/trpc/server/routers/viewer/workflows.tsx (L218)
    The workflow delete mutation cancels reminders fire-and-forget and then cascade-deletes the workflowReminder rows, so a failed cancellation has no retry path and the email/SMS can still be sent.

Worth checking

  • 🟡 packages/features/bookings/lib/handleNewBooking.ts (L961)
    The try/catch in handleNewBooking wraps a forEach that starts async functions without awaiting, so it cannot catch async rejection; the cancellation is fire-and-forget with no error handling.
  • 🟡 packages/trpc/server/routers/viewer/workflows.tsx (L378)
    update uses two cancellation strategies: removed event types defer to the cron (cancelled:true), while deleted workflow steps cancel immediately; reconcile for consistent 'never send' semantics.
  • 🟡 packages/trpc/server/routers/viewer/workflows.tsx (L521)
    The step-deleted path fetches reminders without scheduled/referenceId filters and races the unawaited deletions with the cascade delete, possibly causing swallowed P2025 errors; filter and await.
Small things (take or leave)
  • 🔵 packages/features/ee/workflows/api/scheduleEmailReminders.ts (L41)
    The cron's remindersToCancel query filters only on cancelled and scheduledDate; add method: WorkflowMethods.EMAIL (and referenceId: { not: null }) to avoid acting on non-email reminders.
  • 🔵 packages/prisma/migrations/20230217230604_add_cancelled_to_workflow_reminder/migration.sql (L2)
    The migration adds cancelled as nullable without a default; consider DEFAULT false for an explicit lifecycle.
  • 🔵 packages/features/bookings/lib/handleCancelBooking.ts (L487)
    Recurring/seat-only cancellations: confirm seat-removal cancellations handle reminders; if not, stale emails may be sent for removed seats.
  • 🔵 packages/trpc/server/routers/viewer/workflows.tsx (L576)
    The cancelled flag is only ever read by the cron; repeated step edits can accumulate stale rows unless the cron deletes them.
  • 🔵 packages/features/ee/workflows/components/WorkflowStepContainer.tsx (L390)
    The PhoneInput required attribute is not backed by RHF rules; confirm sendTo validation elsewhere (pre-existing).

✅ Feature-level checklist

Not there yet

  • ❌ Cancelled and rescheduled bookings must not send workflow reminder emails.
    Multiple paths (reschedule via handleNewBooking, workflow delete, and fire-and-forget cancellations in handleCancelBooking/bookings.tsx) can still send emails because the cancellation is not correctly persisted or executed. The cron's error handling can also skip cancellations, and a last-hour race exists.
  • ❌ The solution must not accumulate more than 100 pending SendGrid cancellations (the root cause of #7225).
    The immediateDelete paths in emailReminderManager and handleNewBooking still perform immediate SendGrid cancels, which accumulate and can hit the 100 limit. The deferred cron mechanism is not used consistently.

Partially covered

  • ⚠️ Add a cancelled field to workflowReminder and set it when a scheduled email is cancelled; the cron scheduleEmailReminders must cancel those within the next hour.
    The field and cron query exist, but the flag is not set in the immediateDelete paths, and the cron's loop aborts on error and lacks a method filter, so the mechanism is not fully reliable.