Skip to content
Jump to bottom

Automated Test: sms-retry-enhanced #372

Closed
wants to merge 1 commit into from
+38 −6
Closed

Automated Test: sms-retry-enhanced #372

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
41 changes: 35 additions & 6 deletions packages/features/ee/workflows/api/scheduleSMSReminders.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,19 @@ async function handler(req: NextApiRequest, res: NextApiResponse) {
//delete all scheduled sms reminders where scheduled date is past current date
await prisma.workflowReminder.deleteMany({
where: {
method: WorkflowMethods.SMS,
scheduledDate: {
lte: dayjs().toISOString(),
},
OR: [
{
method: WorkflowMethods.SMS,
scheduledDate: {
lte: dayjs().toISOString(),
},
},
{
retryCount: {
gt: 1,
},
},
],
Comment on lines +31 to +43
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Scope retry cleanup to SMS and unscheduled reminders only.
The new OR branch (retryCount > 1) is not constrained to method: SMS, and also deletes scheduled reminders with retries, which can remove unrelated/valid reminders. Consider scoping the delete to SMS and unscheduled failed retries.

🔧 Suggested fix 
-  await prisma.workflowReminder.deleteMany({
-    where: {
-      OR: [
-        {
-          method: WorkflowMethods.SMS,
-          scheduledDate: {
-            lte: dayjs().toISOString(),
-          },
-        },
-        {
-          retryCount: {
-            gt: 1,
-          },
-        },
-      ],
-    },
-  });
+  await prisma.workflowReminder.deleteMany({
+    where: {
+      method: WorkflowMethods.SMS,
+      OR: [
+        {
+          scheduledDate: {
+            lte: dayjs().toISOString(),
+          },
+        },
+        {
+          scheduled: false,
+          retryCount: {
+            gt: 1,
+          },
+        },
+      ],
+    },
+  });
🤖 Prompt for AI Agents 
In `@packages/features/ee/workflows/api/scheduleSMSReminders.ts` around lines 31 -
43, The OR branch currently deletes any reminder with retryCount > 1 regardless
of type or schedule; narrow it so the retryCount condition only applies to
failed SMS reminders without a scheduledDate. Update the second OR object to
include method: WorkflowMethods.SMS and a constraint that scheduledDate is
absent/unscheduled (e.g., null or missing) in addition to retryCount > 1 so only
unscheduled SMS retries are targeted.

},
});

Expand All @@ -44,8 +53,11 @@ async function handler(req: NextApiRequest, res: NextApiResponse) {
lte: dayjs().add(7, "day").toISOString(),
},
},
select,
})) as PartialWorkflowReminder[];
select: {
...select,
retryCount: true,
},
})) as (PartialWorkflowReminder & { retryCount: number })[];

if (!unscheduledReminders.length) {
res.json({ ok: true });
Expand Down Expand Up @@ -163,9 +175,26 @@ async function handler(req: NextApiRequest, res: NextApiResponse) {
referenceId: scheduledSMS.sid,
},
});
} else {
await prisma.workflowReminder.update({
where: {
id: reminder.id,
},
data: {
retryCount: reminder.retryCount + 1,
},
});
Comment on lines +178 to +186
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

fd scheduleSMSReminders.ts

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 206

🏁 Script executed:

wc -l packages/features/ee/workflows/api/scheduleSMSReminders.ts

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 152

🏁 Script executed:

cat -n packages/features/ee/workflows/api/scheduleSMSReminders.ts | sed -n '170,200p'

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 1028

🏁 Script executed:

cat -n packages/features/ee/workflows/api/scheduleSMSReminders.ts | head -100

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 4333

🏁 Script executed:

cat -n packages/features/ee/workflows/api/scheduleSMSReminders.ts | sed -n '1,70p' | tail -20

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 677

🏁 Script executed:

rg "increment:" packages/features/ee/workflows/ -A 2 -B 2

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 508

🏁 Script executed:

rg "retryCount.*increment" --type ts --type tsx

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 117

🏁 Script executed:

rg "increment:" -t ts -A 1 -B 1 | head -50

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 1680

🏁 Script executed:

sed -n '1,50p' packages/features/tasker/repository.ts | grep -A 10 "increment"

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 72

🏁 Script executed:

rg -n "attempts.*increment" packages/features/tasker/repository.ts -B 5 -A 5

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 520

🏁 Script executed:

cat -n packages/features/ee/workflows/api/scheduleSMSReminders.ts | sed -n '160,200p'

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 1313

Replace retryCount increment with Prisma atomic operation to prevent lost updates in concurrent cron execution.

retryCount: reminder.retryCount + 1 can lose updates when multiple cron instances process the same reminder concurrently. Both instances read the same value, increment it, and persist the same final count instead of proper sequential increments. Use Prisma's atomic increment.

This occurs at lines 184 and 195.

Suggested fix 
           data: {
-            retryCount: reminder.retryCount + 1,
+            retryCount: { increment: 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.

Suggested change
} else {
await prisma.workflowReminder.update({
where: {
id: reminder.id,
},
data: {
retryCount: reminder.retryCount + 1,
},
});
} else {
await prisma.workflowReminder.update({
where: {
id: reminder.id,
},
data: {
retryCount: { increment: 1 },
},
});
🤖 Prompt for AI Agents 
In `@packages/features/ee/workflows/api/scheduleSMSReminders.ts` around lines 178
- 186, The current updates to retryCount use a read-modify-write pattern
(retryCount: reminder.retryCount + 1) which can lose increments under concurrent
cron runs; change both prisma.workflowReminder.update calls that reference
reminder.retryCount to use Prisma's atomic increment API (data: { retryCount: {
increment: 1 } }) so updates are applied atomically via
prisma.workflowReminder.update rather than relying on the stale
reminder.retryCount value.

}
}
} catch (error) {
await prisma.workflowReminder.update({
where: {
id: reminder.id,
},
data: {
retryCount: reminder.retryCount + 1,
},
});
console.log(`Error scheduling SMS with error ${error}`);
}
}
Expand Down
2 changes: 2 additions & 0 deletions packages/prisma/migrations/20240508134359_add_retry_count_to_workflow_reminder/migration.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
-- AlterTable
ALTER TABLE "WorkflowReminder" ADD COLUMN "retryCount" INTEGER NOT NULL DEFAULT 0;
1 change: 1 addition & 0 deletions packages/prisma/schema.prisma
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -997,6 +997,7 @@ model WorkflowReminder {
cancelled Boolean?
seatReferenceId String?
isMandatoryReminder Boolean? @default(false)
retryCount Int @default(0)
@@index([bookingUid])
@@index([workflowStepId])
Expand Down