SMS workflow reminder retry count tracking

[hussam789]

User description

PR #8

---

PR Type

Enhancement, Bug fix

---

Description

• Add retryCount field to WorkflowReminder model for tracking SMS retry attempts

• Implement retry logic that increments count on scheduling failures

• Delete reminders exceeding maximum retry attempts (retryCount > 1)

• Include retryCount in SMS reminder queries and update operations

---

Diagram Walkthrough

flowchart LR
  A["SMS Reminder Processing"] --> B["Schedule SMS"]
  B -->|Success| C["Mark as scheduled"]
  B -->|Failure| D["Increment retryCount"]
  D --> E["Check retryCount > 1"]
  E -->|Yes| F["Delete reminder"]
  E -->|No| G["Retry next cycle"]

Loading

File Walkthrough

	Relevant files
Enhancement	scheduleSMSReminders.ts Add retry count tracking and increment logic                          packages/features/ee/workflows/api/scheduleSMSReminders.ts Modified deletion query to remove reminders with retryCount > 1 in addition to past-dated reminders Added retryCount field to the select clause when fetching unscheduled reminders Implemented retry count increment logic in both success and error handling paths Updated type annotation to include retryCount in the reminder object +35/-6
Configuration changes	schema.prisma Add retryCount field to schema                                                      packages/prisma/schema.prisma Added retryCount integer field to WorkflowReminder model with default value of 0 +1/-0
Dependencies	migration.sql Database migration for retryCount column                                  packages/prisma/migrations/20240508134359_add_retry_count_to_workflow_reminder/migration.sql Created database migration to add retryCount column to WorkflowReminder table with default value 0 +2/-0

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Overbroad deletion scope Description: Deletion query removes any reminder with retryCount > 1 regardless of method, which may unintentionally delete non-SMS reminders if retryCount exists across methods. scheduleSMSReminders.ts [31-44] Referred Code OR: [ { method: WorkflowMethods.SMS, scheduledDate: { lte: dayjs().toISOString(), }, }, { retryCount: { gt: 1, }, }, ], },
	Race condition risk Description: Error path increments retryCount without transaction or cap check, risking race conditions and unbounded growth if multiple concurrent schedulers run. scheduleSMSReminders.ts [190-198] Referred Code await prisma.workflowReminder.update({ where: { id: reminder.id, }, data: { retryCount: reminder.retryCount + 1, }, }); console.log(`Error scheduling SMS with error ${error}`);
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed
🔴	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Weak error handling: The catch block only logs a generic message and increments retryCount without actionable context or guarding against missing retryCount, lacking edge-case handling and detailed logging. Referred Code await prisma.workflowReminder.update({ where: { id: reminder.id, }, data: { retryCount: reminder.retryCount + 1, }, }); console.log(`Error scheduling SMS with error ${error}`); }
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Missing audit logs: New critical actions (deleting reminders, incrementing retry counts, scheduling outcomes) are not logged with user/contextual details, hindering auditability. Referred Code OR: [ { method: WorkflowMethods.SMS, scheduledDate: { lte: dayjs().toISOString(), }, }, { retryCount: { gt: 1, }, }, ], }, }); //find all unscheduled SMS reminders const unscheduledReminders = (await prisma.workflowReminder.findMany({ where: { method: WorkflowMethods.SMS, scheduled: false, ... (clipped 146 lines)
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Potential info leak: The code logs the raw error object which may include internal details or stack traces if surfaced to any user-facing context; unclear if logs are internal-only. Referred Code console.log(`Error scheduling SMS with error ${error}`); }
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Unstructured logging: Uses console logging with an interpolated error string lacking structured context and controls, which may risk sensitive data exposure and reduces log parseability. Referred Code console.log(`Error scheduling SMS with error ${error}`); }
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Missing validation: The new retry/deletion logic acts on database records without explicit validation or authorization checks visible in the diff, and deletion criteria may be overly broad (retryCount > 1) without safeguards. Referred Code OR: [ { method: WorkflowMethods.SMS, scheduledDate: { lte: dayjs().toISOString(), }, }, { retryCount: { gt: 1, }, }, ], }, }); //find all unscheduled SMS reminders const unscheduledReminders = (await prisma.workflowReminder.findMany({ where: { method: WorkflowMethods.SMS, scheduled: false, ... (clipped 9 lines)

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Use atomic increment for retry count To prevent a race condition in a concurrent environment, use Prisma's atomic increment operation to update the retryCount instead of the current read-modify-write pattern. packages/features/ee/workflows/api/scheduleSMSReminders.ts [178-187] } else { await prisma.workflowReminder.update({ where: { id: reminder.id, }, data: { - retryCount: reminder.retryCount + 1, + retryCount: { + increment: 1, + }, }, }); } Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential race condition when incrementing retryCount and proposes using Prisma's atomic increment operation, which is the robust and correct solution for this scenario.	Medium
High-level	Consider a more robust retry strategy The current retry logic uses a fixed schedule based on the cron job's frequency. It is recommended to implement an exponential backoff strategy to create a more robust and scalable solution for handling transient failures. Examples: packages/features/ee/workflows/api/scheduleSMSReminders.ts [178-197] } else { await prisma.workflowReminder.update({ where: { id: reminder.id, }, data: { retryCount: reminder.retryCount + 1, }, }); } ... (clipped 10 lines) Solution Walkthrough: Before: // scheduleSMSReminders.ts // Delete reminders that failed more than once await prisma.workflowReminder.deleteMany({ where: { retryCount: { gt: 1 } }, }); // Fetch all unscheduled reminders const unscheduledReminders = await prisma.workflowReminder.findMany(...); for (const reminder of unscheduledReminders) { try { // ... try to schedule SMS if (!success) { // On failure, just increment count. // Retry will happen on the next cron run. await prisma.workflowReminder.update({ data: { retryCount: reminder.retryCount + 1 }, }); } } catch (error) { // Also increment on error await prisma.workflowReminder.update({ data: { retryCount: reminder.retryCount + 1 }, }); } } After: // schema.prisma would need a `nextRetryAt` field // model WorkflowReminder { // ... // nextRetryAt DateTime? // } // scheduleSMSReminders.ts // Fetch reminders ready for (re)try const remindersToProcess = await prisma.workflowReminder.findMany({ where: { scheduled: false, nextRetryAt: { lte: new Date() } // Only process if it's time }, }); for (const reminder of remindersToProcess) { try { // ... try to schedule SMS } catch (error) { const newRetryCount = reminder.retryCount + 1; // Calculate next attempt time with exponential backoff const delay = Math.pow(2, newRetryCount) * 60000; // 1m, 2m, 4m... const nextRetryAt = new Date(Date.now() + delay); await prisma.workflowReminder.update({ data: { retryCount: newRetryCount, nextRetryAt: nextRetryAt, }, }); } } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a design weakness in the new retry logic, proposing a more robust exponential backoff strategy which is a standard practice for handling transient failures with external services.	Medium
More