Cal.com / PR #28

SMS workflow reminder retry count tracking

calcom/cal.com#14943
Golden recall: 100% Extra findings: 3

Golden Comments (2/2 found)

# Comment Severity Status FriendlyReviewer Detail
#1 Using retryCount: reminder.retryCount + 1 reads a possibly stale value and can lose increments under concurrency; consider an atomic increment via Prisma (increment: 1) to avoid race conditions (also applies to the similar update in the catch block) HIGH ✓ Found "The retryCount increment uses a read-modify-write pattern (data: { retryCount: reminder.retryCount + 1 }) based on a value read earlier from findMany. This is non-atomic and can lose increments in concurrent/overlapping cron runs, potentially deleting a reminder after only one failure or delaying deletion. The recommended fix is Prisma's atomic increment (data: { retryCount: { increment: 1 } }) in both the else and catch paths" (scheduleSMSReminders.ts L184)
#2 The deletion logic in scheduleSMSReminders.ts incorrectly deletes non-SMS workflow reminders (e.g., Email, WhatsApp) that have retryCount > 1. This occurs because the retryCount condition within the OR clause for deletion lacks a method: WorkflowMethods.SMS filter, causing it to apply to all reminder types instead of only SMS reminders, which is the intended scope of this function HIGH ✓ Found "The retry-deletion OR branch { retryCount: { gt: 1 } } has no method: WorkflowMethods.SMS filter (unlike the first branch). It is latent today since only this SMS handler increments retryCount, but it leaves deletion unintentionally scoped and could delete EMAIL/WHATSAPP reminders if any future code path sets retryCount; scoping the branch to SMS is recommended" (scheduleSMSReminders.ts L40)

Supplementary Findings (3 findings)

Finding File Severity Legitimate?
The motivating missing-phone-number path (sendTo falsy) skips the twilio call without any else/catch increment: such reminders are retried every 15 minutes without counting toward the two-attempt limit and are only deleted when scheduledDate <= now scheduleSMSReminders.ts:158 HIGH ✓ Real gap — it is exactly the PR's motivating scenario that is never deleted via the retry path
The catch block increments retryCount for any exception inside the try, including errors unrelated to SMS delivery (prisma/DB, template rendering, metadata parsing): a transient infra outage across two cron runs could drive retryCount to 2 and delete a valid reminder that was never scheduled scheduleSMSReminders.ts:195 MEDIUM ✓ Real premature-deletion risk; narrowing the catch is justified
On a successful schedule the update only sets scheduled: true and referenceId, leaving retryCount at its previous value — benign in the current single-pass flow but fragile for future rescheduling paths scheduleSMSReminders.ts:175 LOW ✓ Borderline: benign today; a hygiene fix for future rescheduling paths

Honest Analysis

FriendlyReviewer found both golden bugs of this PR — recall 100%. Golden #1 (non-atomic retryCount increment) is matched almost verbatim: the read-modify-write pattern at L184 is called out as a top-priority issue with the correct failure modes (a reminder deleted after a single failure, or deletion delayed) and the correct remedy (Prisma's atomic increment: 1 in both the else and catch paths). Golden #2 (the retry-deletion OR branch without a method: WorkflowMethods.SMS filter) is also found, with the precise hazard named — deleting EMAIL/WHATSAPP reminders if any future code path sets retryCount.

The noteworthy divergence is severity: the review ranks the OR-branch finding as a blue "Small thing", arguing it is latent today since only this SMS handler increments retryCount. That reasoning is defensible — no current code path sets retryCount on a non-SMS reminder — but it is also exactly the kind of latent hazard the dataset rated High: the branch exists, and any future change to the retry counter silently broadens deletion. The review's own recommendation for golden #1 (also incrementing in the catch path) is a step in that direction, which makes the blue ranking slightly inconsistent.

The three supplementary findings all sit in the same file and are legitimate. The strongest is the red L158 item: the missing-phone-number path — the PR's own motivating scenario — never increments retryCount, so those reminders are retried indefinitely every 15 minutes and never deleted through the retry mechanism. That gap is arguably more operationally significant than the goldens, and the review correctly makes it its top issue. The catch-width item (L195) is real but speculative (it requires a transient infra outage spanning two cron runs), and the retryCount-not-reset item (L175) is a genuine hygiene nit. The verdict — change before merging — is well supported.

FriendlyReviewer's full review

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

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech 🌥️ Feat
2 high, 1 medium, 2 low 1 ok, 1 issue

The PR introduces a retryCount column on WorkflowReminder and a deletion mechanism after two failed SMS scheduling attempts. The migration and Prisma model changes are correct and safe, and the API/typing contract holds. However, several functional issues prevent the feature from fully satisfying its stated purpose: the retry counter is incremented non-atomically (race condition), the deletion predicate is not scoped to the SMS method, the motivating "missing phone number" scenario never increments retryCount so those reminders are never deleted via the retry path, the catch block counts non-scheduling errors toward the retry limit, and retryCount is not reset on success. These issues may cause premature or delayed deletion and leave the primary use case unhandled. The review recommends changing the code before merging.

These might need a close look

  • 🔴 packages/features/ee/workflows/api/scheduleSMSReminders.ts (L184)
    The retryCount increment uses a read-modify-write pattern (data: { retryCount: reminder.retryCount + 1 }) based on a value read earlier from findMany. This is non-atomic and can lose increments in concurrent/overlapping cron runs, potentially deleting a reminder after only one failure or delaying deletion. Recommend using Prisma's atomic increment: data: { retryCount: { increment: 1 } } in both the else and catch paths.
  • 🔴 packages/features/ee/workflows/api/scheduleSMSReminders.ts (L158)
    When sendTo is falsy (missing phone number — the PR's motivating scenario), the twilio call is skipped and no else/catch increments retryCount. These reminders are retried every 15 minutes without counting toward the two-attempt limit and are only deleted when scheduledDate <= now. Increment retryCount on this no-op path so these reminders are also deleted after two attempts.

Worth checking

  • 🟡 packages/features/ee/workflows/api/scheduleSMSReminders.ts (L195)
    The catch block increments retryCount for any exception thrown inside the try, including errors unrelated to SMS delivery (e.g. prisma/DB, template rendering, metadata parsing). A transient infra outage across two cron runs could drive retryCount to 2 and delete a valid reminder without ever scheduling it. Narrow the catch to wrap only the twilio.scheduleSMS call or distinguish delivery failures from other errors.
Small things (take or leave)
  • 🔵 packages/features/ee/workflows/api/scheduleSMSReminders.ts (L40)
    The retry-deletion OR branch { retryCount: { gt: 1 } } has no method: WorkflowMethods.SMS filter (unlike the first branch). This is latent today since only this SMS handler increments retryCount, but it leaves deletion unintentionally scoped and could delete EMAIL/WHATSAPP reminders if any future code path sets retryCount. Scope the branch to SMS for consistency.
  • 🔵 packages/features/ee/workflows/api/scheduleSMSReminders.ts (L175)
    On a successful schedule the update only sets scheduled: true and referenceId; retryCount is left at its previous value. Although benign in the current single-pass flow, keeping a non-zero retryCount on a scheduled row is fragile for future rescheduling paths. Consider resetting retryCount: 0 on success.

✅ Feature-level checklist

Looks good

  • ✅ Add a retry count to the workflow reminder (schema/model).
    Migration adds a non-nullable retryCount column with default 0, matching the Prisma model. The folder naming and no-down-migration follow repo conventions.

Not there yet

  • ❌ After two failed SMS scheduling attempts, delete the workflow reminder.
    The mechanism is incomplete: the missing-phone path (empty sendTo) never increments retryCount, so such reminders are never deleted via the retry path (the PR's motivating case). Additionally, the non-atomic increment can cause deletion after one failure or delayed deletion, and the catch block counts non-scheduling errors toward the limit, risking premature deletion. These issues prevent the requirement from being reliably met.