Cal.com / PR #8

SMS workflow reminder retry count tracking

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

Golden Comments (2/2 found, 1 partial)

# Comment Severity Status FriendlyReviewer Detail
#1 retryCount: reminder.retryCount + 1 reads a possibly stale value under concurrency — lost increments. Use increment: 1 for atomicity (also applies to catch block) HIGH ◈ Partial FriendlyReviewer found a related issue: the fragile type intersection and missing retryCount reset on success (LOW), but did not explicitly identify the non-atomic read-then-write pattern. The partial credit reflects a near miss — FR saw something was wrong with retryCount but diagnosed the wrong aspect.
#2 Deletion logic removes non-SMS reminders (Email, WhatsApp) with retryCount > 1 because the OR condition is missing a method: WorkflowMethods.SMS filter HIGH ✓ Found FriendlyReviewer: "The deleteMany OR query deletes reminders of all methods (EMAIL, WHATSAPP, SMS) where retryCount > 1 because the second OR clause is not scoped by method" (scheduleSMSReminders.ts:31). Match perfect.

Supplementary Findings (2 findings)

Additional issues found by FriendlyReviewer that are not part of the golden (reference) set.

Finding File Severity Legitimate?
select imported from getWorkflowReminders.ts does not include retryCount — fragile workaround scheduleSMSReminders.ts:55 MEDIUM ✓ Missing import
retryCount not reset to 0 when SMS is successfully scheduled — latent stale value scheduleSMSReminders.ts:176 LOW ✓ Missing reset

Honest Analysis

This is a small PR (3 files, +38/−6) and FriendlyReviewer's performance reflects its modest scope. Golden #2 (the OR clause missing a method filter) was found with a perfect match. But Golden #1 — the non-atomic retryCount + 1 race condition — was only partially covered. FriendlyReviewer noticed the retryCount was problematic (flagged the fragile type intersection and missing reset on success) but didn't identify the core concurrency issue: that reading then writing a counter is not atomic under concurrent Prisma transactions.

The two supplementary findings are thin. One flags a missing field in a select import (MEDIUM), the other notes a missing reset on success (LOW). Both are legitimate but minor. The total of 3 findings on a 38-line addition is low even for the small size of the PR — there may have been more to discover in the surrounding workflow logic that FriendlyReviewer didn't reach.

FriendlyReviewer's full review

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

friendly-reviewer bot commented now

Hi there 👋

☁️ Overview
1 high, 1 medium, 1 low

This PR adds retry count tracking for SMS workflow reminders. The implementation has a significant bug in the deletion logic that affects non-SMS reminders, plus some fragility in the select/import patterns. The retry count update logic also has a subtle issue — while the type safety of the intersection was checked, the non-atomic read-then-write pattern could cause lost increments under concurrent execution.

Must fix

  • 🔴 scheduleSMSReminders.ts (L31)
    The deleteMany query uses an OR condition that deletes reminders of all methods (EMAIL, WHATSAPP, SMS) where retryCount > 1. The second OR clause { retryCount: { gte: MAX_RETRIES } } is not scoped by method. Fix: scope the second condition to SMS only.

Should address

  • 🟡 scheduleSMSReminders.ts (L55)
    The select object imported from getWorkflowReminders.ts does not include retryCount. The workaround using a type intersection is fragile — if the Prisma schema changes, the type will silently drift. Either add retryCount to the select or define a dedicated type.

Minor

  • 🔵 scheduleSMSReminders.ts (L176)
    retryCount is not reset to 0 when an SMS is successfully scheduled. While the deleteMany handles cleanup at MAX_RETRIES, a successfully-sent reminder retains its previous retryCount, which could cause premature deletion if MAX_RETRIES is reached cumulatively across sessions.