SMS workflow reminder retry count tracking

[ShashankFC]

Test 9nn---nReplicated from ai-code-review-evaluation/cal.com-greptile#9

[github-actions]

Hey there and thank you for opening this pull request! 👋🏼

We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted.

Details:

No release type found in pull request title "SMS workflow reminder retry count tracking". Add a prefix to indicate what kind of release this pull request corresponds to. For reference, see https://www.conventionalcommits.org/

Available types:
 - feat: A new feature
 - fix: A bug fix
 - docs: Documentation only changes
 - style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
 - refactor: A code change that neither fixes a bug nor adds a feature
 - perf: A code change that improves performance
 - test: Adding missing tests or correcting existing tests
 - build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
 - ci: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
 - chore: Other changes that don't modify src or test files
 - revert: Reverts a previous commit

[ShashankFC]

@claude

[claude]

Claude finished @ShashankFC's task in 1m 55s —— View job

---

Code Review

Tasks

• Read repository guidelines (CLAUDE.md)
• Review changed files
• Analyze implementation for issues
• Provide comprehensive feedback

---

Overview

This PR adds retry count tracking to SMS workflow reminders to prevent infinite retry attempts when SMS sending fails. The implementation adds a retryCount field to the database and increments it on failures, with automatic cleanup of reminders that exceed the retry threshold.

Issues Found

1. Critical: Hardcoded retry limit with magic number

Location: packages/features/ee/workflows/api/scheduleSMSReminders.ts:39-42

{
  retryCount: {
    gt: 1,
  },
}

The retry limit is hardcoded as > 1 (meaning max 2 attempts) with no explanation or constant. This makes it:

• Difficult to adjust the retry policy
• Unclear why 2 attempts was chosen
• Inconsistent if other parts of the codebase use different limits

Recommendation: Extract to a named constant with documentation:

const MAX_SMS_RETRY_COUNT = 1; // Maximum number of retry attempts before giving up

// In the delete query:
{
  retryCount: {
    gt: MAX_SMS_RETRY_COUNT,
  },
}

Fix this →

---

2. Bug: Overly aggressive deletion logic

Location: packages/features/ee/workflows/api/scheduleSMSReminders.ts:29-45

The deletion logic has a significant flaw:

await prisma.workflowReminder.deleteMany({
  where: {
    OR: [
      {
        method: WorkflowMethods.SMS,
        scheduledDate: {
          lte: dayjs().toISOString(),
        },
      },
      {
        retryCount: {
          gt: 1,
        },
      },
    ],
  },
});

Problem: The OR condition will delete ALL reminders (including EMAIL and WHATSAPP) that have retryCount > 1, not just SMS reminders. The method: WorkflowMethods.SMS constraint only applies to the first condition.

Impact: This could unintentionally delete email and WhatsApp workflow reminders that have failed multiple times, even though retry tracking might not be implemented for those methods yet.

Recommendation: Apply the SMS method filter to both conditions:

await prisma.workflowReminder.deleteMany({
  where: {
    method: WorkflowMethods.SMS,
    OR: [
      {
        scheduledDate: {
          lte: dayjs().toISOString(),
        },
      },
      {
        retryCount: {
          gt: MAX_SMS_RETRY_COUNT,
        },
      },
    ],
  },
});

Fix this →

---

3. Code Quality: Duplicate database update logic

Location: packages/features/ee/workflows/api/scheduleSMSReminders.ts:178-186, 190-198

The same retry count increment logic appears in two places:

// When scheduleSMS returns falsy (line 178-186)
await prisma.workflowReminder.update({
  where: { id: reminder.id },
  data: { retryCount: reminder.retryCount + 1 },
});

// In catch block (line 190-198)
await prisma.workflowReminder.update({
  where: { id: reminder.id },
  data: { retryCount: reminder.retryCount + 1 },
});

Recommendation: Extract to a helper function to maintain DRY principle:

async function incrementRetryCount(reminderId: number, currentCount: number) {
  await prisma.workflowReminder.update({
    where: { id: reminderId },
    data: { retryCount: currentCount + 1 },
  });
}

// Usage:
} else {
  await incrementRetryCount(reminder.id, reminder.retryCount);
}

// In catch:
await incrementRetryCount(reminder.id, reminder.retryCount);

Fix this →

---

4. Missing: Error logging context

Location: packages/features/ee/workflows/api/scheduleSMSReminders.ts:198

console.log(`Error scheduling SMS with error ${error}`);

The error log doesn't include important context like:

• Which booking failed (booking UID/ID)
• Current retry count
• Reminder details (scheduled date, recipient)

Recommendation: Enhance error logging:

console.error(
  `Error scheduling SMS reminder (attempt ${reminder.retryCount + 1}/${MAX_SMS_RETRY_COUNT + 1}):`,
  {
    reminderId: reminder.id,
    bookingUid: reminder.booking?.uid,
    scheduledDate: reminder.scheduledDate,
    error: error instanceof Error ? error.message : String(error),
  }
);

Fix this →

---

5. Missing: Observability for retry exhaustion

When a reminder reaches the retry limit and gets deleted, there's no logging or tracking. This makes it difficult to:

• Monitor how often SMS sending completely fails
• Debug why reminders aren't being sent
• Alert on high failure rates

Recommendation: Add logging before deletion or track exhausted reminders separately:

// Option 1: Log before deletion
const exhaustedReminders = await prisma.workflowReminder.findMany({
  where: { 
    method: WorkflowMethods.SMS,
    retryCount: { gt: MAX_SMS_RETRY_COUNT }
  },
  select: { id: true, bookingUid: true }
});

if (exhaustedReminders.length > 0) {
  console.warn(`Deleting ${exhaustedReminders.length} exhausted SMS reminders:`, 
    exhaustedReminders.map(r => r.bookingUid));
}

---

6. PR Title Issue

The PR title "SMS workflow reminder retry count tracking" doesn't follow Conventional Commits specification. It should have a prefix like:

• feat: if this is a new feature
• fix: if this fixes a bug (infinite retries)

Based on the implementation, this appears to be a bug fix for infinite retry attempts, so the title should be:

fix: add retry count limit to SMS workflow reminders

---

Positive Observations

✅ Database migration properly includes default value - The DEFAULT 0 ensures existing records work correctly
✅ Type safety maintained - The type cast (PartialWorkflowReminder & { retryCount: number })[] ensures TypeScript knows about the new field
✅ Graceful failure handling - Failed SMS sends increment retry count instead of crashing
✅ Retry on both failure modes - Handles both Twilio errors (catch block) and API-level failures (falsy return)

---

Summary

The PR successfully implements retry tracking for SMS reminders, but has some important issues:

• Critical bug: The deletion logic may affect non-SMS reminders
• Configuration issue: Retry limit is a magic number instead of a documented constant
• Code quality: Duplicate update logic violates DRY principle
• Observability gaps: Insufficient logging for debugging failures

I recommend addressing issues #1 and #2 before merging, as they could lead to unexpected behavior in production.

---

[github-actions]

This PR is being marked as stale due to inactivity.