SMS workflow reminder retry count tracking

[zaibkhan]

No description provided.

[codoki-pr-intelligence]

Codoki PR Review

Summary: Scope retry deletion to SMS, prevent data loss
What’s good: Adds retryCount tracking and increments on failure, enabling a bounded retry policy; schema/migration changes are minimal and consistent.
Review Status: ❌ Requires changes
Overall Priority: Critical

Issues (Critical & High only)

Severity	Issue	Why it matters
Critical	Correctness — Data loss: deleteMany removes non-SMS reminders with retr...	…/api/scheduleSMSReminders.ts This deleteMany condition deletes any WorkflowReminder with retryCount > 1 regardless of method or scheduled status, causing unintended data loss (e.g., email reminders with retryCount 2 will be removed). Scope the retry cleanup to SMS reminders that are still unscheduled. Optionally, also filter findMany to scheduledDate >= now to avoid attempting to schedule past-dated reminders.

Showing top 1 issues. Critical: 1, High: 0. See inline suggestions for more.

Key Feedback (click to expand)

• Needs improvement: The deleteMany cleanup query is overly broad and will delete non-SMS reminders with retryCount > 1; also consider filtering findMany to future dates to avoid scheduling past reminders.
• Testing: Add tests covering deletion scoping (only unscheduled SMS reminders with retryCount > threshold are deleted), and that email/other methods are not affected. Include a case where a reminder reaches retryCount 2 and ensure only SMS unscheduled entries are removed.
• Documentation: Document the retry policy (threshold, scope) and the cleanup behavior in this handler to avoid future regressions; clarify why >1 retries was chosen.
• Compatibility: Adding a non-null column with default 0 is backward-compatible for existing rows; ensure any downstream consumers of WorkflowReminder handle the new field if they rely on strict selects.
• Performance: No major issues; the for-of with await is sequential but acceptable for cron workloads; parallelization can be considered if volumes increase.
• Security: Avoid logging raw error objects; use structured logging with sanitized fields to prevent leaking provider details or PII in logs.
• Open questions: What is the intended max retry threshold and should it be configurable per provider? Should we reset retryCount on success to avoid later unintended cleanup if policy changes?

Confidence: 2/5 — Not ready to merge (1 critical · status: Requires changes)

Sequence Diagram

sequenceDiagram
    participant Cron
    participant API
    participant DB
    participant Twilio
    Cron->>API: POST /scheduleSMSReminders
    API->>DB: deleteMany({ OR: [...] })
    API->>DB: findMany({ method: SMS, scheduled: false, scheduledDate <= now+7d })
    loop for each reminder
        API->>Twilio: scheduleSMS(sendTo, message, scheduledDate,...)
        alt scheduledSMS truthy
            API->>DB: update({ scheduled: true, referenceId })
        else schedule failed or throws
            API->>DB: update({ retryCount: +1 })
        end
    end
    API-->>Cron: 200 { message: "SMS scheduled" }

Loading

React with 👍 or 👎 if you found this review useful.

[codoki-pr-intelligence]

🛑 Critical: This deleteMany condition deletes any WorkflowReminder with retryCount > 1 regardless of method or scheduled status, causing unintended data loss (e.g., email reminders with retryCount 2 will be removed). Scope the retry cleanup to SMS reminders that are still unscheduled. Optionally, also filter findMany to scheduledDate >= now to avoid attempting to schedule past-dated reminders.

Suggested change

	retryCount: {
	```suggestion
	where: {
	OR: [
	{
	method: WorkflowMethods.SMS,
	scheduledDate: {
	lte: dayjs().toISOString(),
	},
	},
	{
	AND: [
	{ method: WorkflowMethods.SMS },
	{ scheduled: false },
	{ retryCount: { gt: 1 } },
	],
	},
	],
	},