SMS workflow reminder retry count tracking#3
Conversation
* add retry count to workflow reminder * add logic to for retry count --------- Co-authored-by: CarinaWolli <wollencarina@gmail.com> Co-authored-by: Udit Takkar <53316345+Udit-takkar@users.noreply.github.com>
|
@coderabbitai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis PR adds retry count tracking to SMS reminders by introducing a new Changes
Sequence DiagramsequenceDiagram
participant API as SMS Scheduler
participant DB as Database
participant SMS as SMS Service
API->>DB: Fetch unscheduled reminders with retryCount
DB-->>API: Return reminders list
loop For each reminder
API->>SMS: Attempt to schedule SMS
alt Scheduling successful
SMS-->>API: scheduledSMS returned
API->>DB: Record scheduled SMS
else Scheduling returns null
API->>DB: Increment retryCount
Note over DB: Reminder queued for retry
end
end
alt Any error during scheduling
API->>DB: Increment retryCount for affected reminder
end
API->>DB: Delete reminders where retryCount > 1 OR scheduledDate is past
DB-->>API: Deletion complete
API-->>API: Return 200 status with message
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/features/ee/workflows/api/scheduleSMSReminders.ts (3)
39-41: Consider making max retry attempts configurable.The current threshold of
retryCount > 1allows only one retry attempt before deletion. For transient failures (network issues, rate limits, temporary service outages), a single retry may be insufficient. Consider:
- Increasing the threshold to allow more retries (e.g.,
retryCount > 3for up to 3 retries)- Making this configurable via environment variable or workflow settings
- Adding exponential backoff between retries if not already implemented
Example with configurable max retries:
+const MAX_SMS_RETRY_ATTEMPTS = parseInt(process.env.MAX_SMS_RETRY_ATTEMPTS || "3", 10); + async function handler(req: NextApiRequest, res: NextApiResponse) { // ... await prisma.workflowReminder.deleteMany({ where: { OR: [ { method: WorkflowMethods.SMS, scheduledDate: { lte: dayjs().toISOString(), }, }, { method: WorkflowMethods.SMS, retryCount: { - gt: 1, + gt: MAX_SMS_RETRY_ATTEMPTS, }, }, ], }, });
178-187: Add logging when scheduling fails and retry is triggered.When scheduling fails (
scheduledSMSis falsy), the retryCount is incremented silently. Adding logging here would improve observability and help diagnose recurring scheduling failures.Apply this diff:
} else { + console.log(`SMS scheduling failed for reminder ${reminder.id}, incrementing retryCount to ${reminder.retryCount + 1}`); await prisma.workflowReminder.update({ where: { id: reminder.id, }, data: { retryCount: reminder.retryCount + 1, }, }); }
189-199: Enhance error logging with reminder context.The current error log doesn't include which reminder failed, making it difficult to debug issues. Include reminder details such as
reminder.idandreminder.bookingUidfor better traceability.Apply this diff:
} catch (error) { await prisma.workflowReminder.update({ where: { id: reminder.id, }, data: { retryCount: reminder.retryCount + 1, }, }); - console.log(`Error scheduling SMS with error ${error}`); + console.log(`Error scheduling SMS for reminder ${reminder.id} (booking: ${reminder.booking?.uid}), retryCount: ${reminder.retryCount + 1}, error: ${error}`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/features/ee/workflows/api/scheduleSMSReminders.ts(3 hunks)packages/prisma/migrations/20240508134359_add_retry_count_to_workflow_reminder/migration.sql(1 hunks)packages/prisma/schema.prisma(1 hunks)
🔇 Additional comments (3)
packages/prisma/migrations/20240508134359_add_retry_count_to_workflow_reminder/migration.sql (1)
1-2: LGTM!The migration correctly adds the
retryCountcolumn with appropriate constraints (NOT NULL with DEFAULT 0) to track retry attempts for workflow reminders.packages/prisma/schema.prisma (1)
1000-1000: LGTM!The
retryCountfield addition aligns with the database migration and follows the appropriate Prisma schema conventions.packages/features/ee/workflows/api/scheduleSMSReminders.ts (1)
56-60: LGTM!Extending the select to include
retryCountand properly typing the result is correct.
| await prisma.workflowReminder.deleteMany({ | ||
| where: { | ||
| method: WorkflowMethods.SMS, | ||
| scheduledDate: { | ||
| lte: dayjs().toISOString(), | ||
| }, | ||
| OR: [ | ||
| { | ||
| method: WorkflowMethods.SMS, | ||
| scheduledDate: { | ||
| lte: dayjs().toISOString(), | ||
| }, | ||
| }, | ||
| { | ||
| retryCount: { | ||
| gt: 1, | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Critical: Delete condition affects all reminder methods, not just SMS.
The retryCount > 1 condition (lines 39-41) doesn't filter by method, which means reminders of any type (EMAIL, WHATSAPP, SMS) with retryCount > 1 will be deleted. This could inadvertently delete non-SMS reminders that shouldn't be affected by this SMS-specific cleanup logic.
Apply this diff to scope the retry-based deletion to SMS reminders only:
OR: [
{
method: WorkflowMethods.SMS,
scheduledDate: {
lte: dayjs().toISOString(),
},
},
{
+ method: WorkflowMethods.SMS,
retryCount: {
gt: 1,
},
},
],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await prisma.workflowReminder.deleteMany({ | |
| where: { | |
| method: WorkflowMethods.SMS, | |
| scheduledDate: { | |
| lte: dayjs().toISOString(), | |
| }, | |
| OR: [ | |
| { | |
| method: WorkflowMethods.SMS, | |
| scheduledDate: { | |
| lte: dayjs().toISOString(), | |
| }, | |
| }, | |
| { | |
| retryCount: { | |
| gt: 1, | |
| }, | |
| }, | |
| ], | |
| }, | |
| }); | |
| await prisma.workflowReminder.deleteMany({ | |
| where: { | |
| OR: [ | |
| { | |
| method: WorkflowMethods.SMS, | |
| scheduledDate: { | |
| lte: dayjs().toISOString(), | |
| }, | |
| }, | |
| { | |
| method: WorkflowMethods.SMS, | |
| retryCount: { | |
| gt: 1, | |
| }, | |
| }, | |
| ], | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In packages/features/ee/workflows/api/scheduleSMSReminders.ts around lines 29 to
45, the deleteMany filter currently deletes any reminder with retryCount > 1
regardless of method; narrow the second OR clause so it only matches SMS
reminders by adding method: WorkflowMethods.SMS to that object (i.e., make the
retryCount condition include method === WorkflowMethods.SMS) so only SMS
reminders are removed by this cleanup.
2 participants
Test 9
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/cal.com-coderabbit#9