-
Notifications
You must be signed in to change notification settings - Fork 12
SMS workflow reminder retry count tracking #9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: sms-retry-base
Are you sure you want to change the base?
Conversation
* add retry count to workflow reminder * add logic to for retry count --------- Co-authored-by: CarinaWolli <[email protected]> Co-authored-by: Udit Takkar <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Summary
This PR implements SMS workflow reminder retry count tracking for the Cal.com application. The changes add infrastructure to track failed SMS delivery attempts and implement cleanup logic to prevent infinite retry loops.
The implementation consists of three main components:
-
Database Schema Changes: A new
retryCountfield is added to theWorkflowRemindermodel in the Prisma schema with a default value of 0. This allows the system to persist retry attempt counts across multiple cron job executions. -
Database Migration: A corresponding migration file adds the
retryCountcolumn to the existingWorkflowRemindertable with appropriate constraints (INTEGER NOT NULL DEFAULT 0). -
API Logic Updates: The
scheduleSMSReminders.tsfile is modified to include retry tracking functionality. The system now increments theretryCountwhen SMS scheduling fails or encounters errors, and includes cleanup logic to delete reminders that have exceeded the retry threshold.
This change integrates with Cal.com's existing workflow reminder system, which handles automated SMS notifications for booking events. The retry mechanism ensures that temporary SMS delivery failures (such as Twilio API issues or network problems) don't result in permanently lost notifications, while also preventing infinite retry attempts that could overwhelm the system.
Confidence score: 2/5
• This PR has significant logical issues that could cause production problems if merged
• The cleanup logic deletes reminders after only 1 retry attempt (retryCount > 1), which may be too aggressive and could result in legitimate reminders being discarded prematurely
• The scheduleSMSReminders.ts file needs careful review due to potential retry logic flaws and missing maximum retry limits
3 files reviewed, 2 comments
| retryCount: { | ||
| gt: 1, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: This condition deletes reminders with retryCount > 1, meaning reminders are deleted after just one retry attempt. This seems too aggressive - consider if this should be a higher threshold like > 3 or > 5 to allow multiple retry attempts.
| id: reminder.id, | ||
| }, | ||
| data: { | ||
| retryCount: reminder.retryCount + 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: No maximum retry limit is enforced. Without a cap (e.g., retryCount < 5), reminders could be retried indefinitely, potentially causing performance issues over time.
|
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: |
|
This PR is being marked as stale due to inactivity. |
Test 9