Skip to content

SMS workflow reminder retry count tracking #3

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.

Sign up for GitHub

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

Open
Frawatson wants to merge 1 commit into
base: sms-retry-base
Choose a base branch
from
Open

SMS workflow reminder retry count tracking #3

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
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,

@grapple-pr grapple-pr Bot Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 MINOR — Intent Alignment (confidence: 98%)

The deletion threshold retryCount > 1 (i.e., gt: 1, meaning retryCount >= 2) means a reminder must fail at least twice before being cleaned up. The intent says 'retryCount greater than 1' which matches the code, but this means a reminder that failed once (retryCount === 1) will be retried. Confirm this is the desired threshold — if the intent is 'retry once then delete', gt: 1 (delete at 2+) is correct. If the intent is 'delete after first failure', it should be gt: 0 or gte: 1.

Evidence:

  • Intent spec: 'Reminders with a retryCount greater than 1 are also added to the deletion sweep'
  • Intent spec edge case: 'The deletion condition retryCount > 1 deletes reminders that have failed twice, but a reminder that has failed exactly once (retryCount === 1) will be retried again'
  • Code uses gt: 1 which translates to retryCount >= 2

Agent: logic

@grapple-pr grapple-pr Bot Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ [Grapple PR] Auto-fix — logic agent (Small fix (3 lines, 1 file))

The deletion threshold retryCount > 1 (i.e., gt: 1, meaning retryCount >= 2) means a reminder must fail at least twice before being cleaned up. The intent says 'retryCount greater than 1' which matches the code, but this means a reminder that failed once (retryCount === 1) will be retried. Confirm this is the desired threshold — if the intent is 'retry once then delete', gt: 1 (delete at 2+) is correct. If the intent is 'delete after first failure', it should be gt: 0 or gte: 1.

Suggested change
gt: 1,
// retryCount > 1 (i.e. >= 2) is intentional: a reminder that failed once
// (retryCount === 1) is retried one more time before being swept up here.
// If the intent were "delete after first failure", this should be `gt: 0`.

🤖 Grapple PR auto-fix • minor • confidence: 98%

},

@grapple-pr grapple-pr Bot Apr 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 INFO — Intent alignment (confidence: 84%)

The deletion threshold is retryCount > 1, meaning reminders are deleted after 2 failed attempts (retryCount reaches 2). The intent specification notes this ambiguity: 'it is unclear if the intended threshold is 2 retries (gt 1) or 1 retry (gte 1)'. This should be explicitly validated against product requirements to confirm that allowing 2 attempts before deletion is the intended behavior.

Evidence:

  • First failure: retryCount becomes 1 → not deleted (1 > 1 is false)
  • Second failure: retryCount becomes 2 → deleted on next run (2 > 1 is true)
  • The linked issue feat: 2fa backup codes #9 is about '2FA backup codes' which is completely unrelated, so product intent cannot be verified from the ticket

Agent: logic

},

@grapple-pr grapple-pr Bot Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 CRITICAL — Business Logic Correctness (confidence: 100%)

The retryCount > 1 deletion condition lacks a method: WorkflowMethods.SMS filter, meaning ANY WorkflowReminder (EMAIL, WHATSAPP, etc.) with retryCount > 1 will be deleted by this SMS-specific cron handler. This is almost certainly unintentional — the handler is scheduleSMSReminders and should only clean up SMS reminders.

Evidence:

  • The original code only deleted reminders with method: WorkflowMethods.SMS.
  • The new OR branch for retryCount: { gt: 1 } has no method filter, so it applies to all workflow methods.
  • The intent specification explicitly flags this: 'non-SMS WorkflowReminders with a retryCount field could be unintentionally deleted if they ever exceed 1'.
  • The WorkflowReminder model supports multiple methods (SMS, EMAIL, WHATSAPP) per the WorkflowMethods enum.

Agent: logic

@grapple-pr grapple-pr Bot Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 MINOR — Code organization (confidence: 100%)

The deleteMany OR clause deletes reminders with retryCount > 1 regardless of method type. Non-SMS reminders could be unintentionally deleted if they accumulate retryCount > 1, since the second OR branch lacks a method filter.

Evidence:

  • The second OR condition only checks retryCount: { gt: 1 } without filtering by method: WorkflowMethods.SMS
  • The first OR branch explicitly requires method: WorkflowMethods.SMS, but the second does not
  • If non-SMS reminders can have retryCount incremented elsewhere, they would be deleted by this clause

Agent: style

@grapple-pr grapple-pr Bot Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ [Grapple PR] Auto-fix — logic agent (Small fix (1 lines, 1 file))

The retryCount > 1 deletion condition lacks a method: WorkflowMethods.SMS filter, meaning ANY WorkflowReminder (EMAIL, WHATSAPP, etc.) with retryCount > 1 will be deleted by this SMS-specific cron handler. This is almost certainly unintentional — the handler is scheduleSMSReminders and should only clean up SMS reminders.

Suggested change
},
method: WorkflowMethods.SMS,

🤖 Grapple PR auto-fix • critical • confidence: 100%

@grapple-pr grapple-pr Bot Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ [Grapple PR] Auto-fix — style agent (Small fix (1 lines, 1 file))

The deleteMany OR clause deletes reminders with retryCount > 1 regardless of method type. Non-SMS reminders could be unintentionally deleted if they accumulate retryCount > 1, since the second OR branch lacks a method filter.

Suggested change
},
method: WorkflowMethods.SMS,

🤖 Grapple PR auto-fix • minor • confidence: 100%

@grapple-pr grapple-pr Bot Apr 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 CRITICAL — Business logic correctness (confidence: 100%)

The retryCount deletion clause has no method: WorkflowMethods.SMS constraint, meaning ANY WorkflowReminder (email, WhatsApp, etc.) with retryCount > 1 will be deleted during the SMS cleanup phase. This is a data loss bug — non-SMS reminders that happen to have retryCount > 1 (if retry logic is ever added to other methods, or if retryCount is incremented by a bug) will be silently removed.

Evidence:

  • The OR clause { retryCount: { gt: 1 } } applies to all WorkflowReminder rows regardless of method
  • The original deletion was scoped to method: WorkflowMethods.SMS but the new OR branch drops that constraint
  • Intent specification explicitly calls out SMS retry tracking — non-SMS reminders should not be affected

Agent: logic

@grapple-pr grapple-pr Bot Apr 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 MAJOR — Off-by-one / Retry threshold (confidence: 95%)

The deletion threshold retryCount: { gt: 1 } means reminders are deleted once retryCount reaches 2 (i.e., after 2 failures). On the first cron run a reminder fails (retryCount goes to 1), on the second run it's fetched again, fails again (retryCount goes to 2), and on the THIRD run the deletion clause removes it — so the reminder gets exactly 2 attempts. If the intent is 'more than one retry' (i.e., 3 total attempts: original + 2 retries), then gt: 2 would be correct. The current threshold is very aggressive — only 2 failures before permanent deletion.

Evidence:

  • Intent says 'retried more than once' which is ambiguous — could mean retryCount > 1 (2+ failures) or retryCount > 2 (allowing 2 retries)
  • With gt: 1, a transient outage lasting two cron cycles permanently deletes the reminder with no way to recover

Agent: logic

@grapple-pr grapple-pr Bot Apr 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ [Grapple PR] Auto-fix — logic agent (Small fix (1 lines, 1 file))

The retryCount deletion clause has no method: WorkflowMethods.SMS constraint, meaning ANY WorkflowReminder (email, WhatsApp, etc.) with retryCount > 1 will be deleted during the SMS cleanup phase. This is a data loss bug — non-SMS reminders that happen to have retryCount > 1 (if retry logic is ever added to other methods, or if retryCount is incremented by a bug) will be silently removed.

Suggested change
},
method: WorkflowMethods.SMS,

🤖 Grapple PR auto-fix • critical • confidence: 100%

@grapple-pr grapple-pr Bot Apr 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ [Grapple PR] Auto-fix — logic agent (Small fix (5 lines, 1 file))

The deletion threshold retryCount: { gt: 1 } means reminders are deleted once retryCount reaches 2 (i.e., after 2 failures). On the first cron run a reminder fails (retryCount goes to 1), on the second run it's fetched again, fails again (retryCount goes to 2), and on the THIRD run the deletion clause removes it — so the reminder gets exactly 2 attempts. If the intent is 'more than one retry' (i.e., 3 total attempts: original + 2 retries), then gt: 2 would be correct. The current threshold is very aggressive — only 2 failures before permanent deletion.

Suggested change
},
// Delete only after 3 total attempts (original + 2 retries).
// gt: 2 means retryCount must be 3+ before deletion, giving the reminder
// two full retry cycles to succeed before permanent removal.
gt: 2,

🤖 Grapple PR auto-fix • major • confidence: 95%

],
},

@grapple-pr grapple-pr Bot Apr 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 CRITICAL — Business logic correctness (confidence: 100%)

The second OR branch in deleteMany has no method: WorkflowMethods.SMS filter, meaning ANY WorkflowReminder (email, WhatsApp, or other methods) with retryCount > 1 will be deleted. This is almost certainly unintended — the cleanup should only delete SMS reminders that have exceeded the retry threshold.

Evidence:

  • The original code scoped deletion to method: WorkflowMethods.SMS
  • The second OR branch { retryCount: { gt: 1 } } has no method filter at all
  • Only SMS reminders have their retryCount incremented in this handler, but email/WhatsApp reminders could have retryCount > 1 set by other code paths or future features
  • The intent specification explicitly calls this out as a risk: 'the second OR branch has no method: WorkflowMethods.SMS filter — this could unintentionally delete email or other workflow reminders'

Agent: logic

@grapple-pr grapple-pr Bot Apr 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ [Grapple PR] Auto-fix — logic agent (Small fix (1 lines, 1 file))

The second OR branch in deleteMany has no method: WorkflowMethods.SMS filter, meaning ANY WorkflowReminder (email, WhatsApp, or other methods) with retryCount > 1 will be deleted. This is almost certainly unintended — the cleanup should only delete SMS reminders that have exceeded the retry threshold.

Suggested change
},
method: WorkflowMethods.SMS,

🤖 Grapple PR auto-fix • critical • confidence: 100%

});

@grapple-pr grapple-pr Bot Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 INFO — Intent Alignment (confidence: 91%)

The PR is linked to Issue #9 'feat: 2fa backup codes', but the code changes are entirely about SMS workflow reminder retry tracking. The code changes are correctly scoped to SMS workflow reminders and contain nothing related to 2FA. The linked issue appears to be incorrect.

Evidence:

  • Issue feat: 2fa backup codes #9 title: 'feat: 2fa backup codes'
  • All code changes are in scheduleSMSReminders.ts, WorkflowReminder schema, and a migration for retryCount
  • Intent spec notes: 'The PR title and description reference SMS retry tracking, but the linked ticket (feat: 2fa backup codes #9) describes 2fa backup codes'

Agent: logic


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,

@grapple-pr grapple-pr Bot Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 MINOR — Data Integrity (confidence: 100%)

The retryCount increment uses the in-memory value reminder.retryCount + 1 rather than an atomic Prisma increment operation. If two cron jobs run concurrently (no mutex is visible), both could read the same retryCount value and write retryCount + 1, effectively losing an increment (last-write-wins).

Evidence:

  • The query fetches reminders into memory, then later updates them one-by-one. No locking or atomic increment is used.
  • Prisma supports { increment: 1 } for atomic updates: data: { retryCount: { increment: 1 } }
  • The intent specification flags: 'No idempotency guard — if the cron job runs concurrently or twice, a reminder could have its retryCount incremented multiple times in parallel'

Agent: logic

@grapple-pr grapple-pr Bot Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ [Grapple PR] Auto-fix — logic agent (Small fix (4 lines, 1 file))

The retryCount increment uses the in-memory value reminder.retryCount + 1 rather than an atomic Prisma increment operation. If two cron jobs run concurrently (no mutex is visible), both could read the same retryCount value and write retryCount + 1, effectively losing an increment (last-write-wins).

Suggested change
id: reminder.id,
retryCount: { increment: 1 },
retryCount: { increment: 1 },

🤖 Grapple PR auto-fix • minor • confidence: 100%

},
data: {

@grapple-pr grapple-pr Bot Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 MAJOR — Edge Cases (confidence: 100%)

Both the else branch (no valid sid) and the catch block independently increment retryCount. If the Twilio call succeeds but returns no sid, and then the prisma.workflowReminder.update in the else branch throws an exception, execution will fall into the catch block which will attempt to increment retryCount again. Depending on whether the first update succeeded or not, this could result in a double increment (+2 instead of +1) for a single scheduling attempt.

Evidence:

  • The else branch (line 177-183) calls prisma.workflowReminder.update with retryCount: reminder.retryCount + 1.
  • If that update throws, the catch block (line 184-193) also calls prisma.workflowReminder.update with retryCount: reminder.retryCount + 1.
  • If the first update succeeded before throwing (unlikely but possible with network issues), the retryCount would have already been incremented, and then the catch block would set it to the same value (reminder.retryCount + 1) — which is actually idempotent since it uses the stale reminder.retryCount value.
  • However, if the else-branch update fails before writing, the catch block would correctly increment once. The behavior is ambiguous and fragile.

Agent: logic

@grapple-pr grapple-pr Bot Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (73 lines, 1 file) — review recommended)

Both the else branch (no valid sid) and the catch block independently increment retryCount. If the Twilio call succeeds but returns no sid, and then the prisma.workflowReminder.update in the else branch throws an exception, execution will fall into the catch block which will attempt to increment retryCount again. Depending on whether the first update succeeded or not, this could result in a double increment (+2 instead of +1) for a single scheduling attempt.

--- a/packages/features/ee/workflows/api/scheduleSMSReminders.ts
+++ b/packages/features/ee/workflows/api/scheduleSMSReminders.ts
@@ -163,19 +163,28 @@ async function handler(req: NextApiRequest, res: NextApiResponse) {
-        if (scheduledSMS) {
-          await prisma.workflowReminder.update({
-            where: {
-              id: reminder.id,
-            },
-            data: {
-              scheduled: true,
-              referenceId: scheduledSMS.sid,
-            },
-          });
-        } else {
-          await prisma.workflowReminder.update({
-            where: {
-              id: reminder.id,
-            },
-            data: {
-              retryCount: reminder.retryCount + 1,
-            },
-          });
-        }
-      }
-    } catch (error) {
-      await prisma.workflowReminder.update({
-        where: {
-          id: reminder.id,
-        },
-        data: {
-          retryCount: reminder.retryCount + 1,
-        },
-      });
-      console.log(`Error scheduling SMS with error ${error}`);
-    }
+        let schedulingFailed = false;
+
+        try {
+          if (scheduledSMS) {
+            await prisma.workflowReminder.update({
+              where: {
+                id: reminder.id,
+              },
+              data: {
+                scheduled: true,
+                referenceId: scheduledSMS.sid,
+              },
+            });
+          } else {
+            // Twilio call succeeded but returned no sid — treat as a failed attempt
+            schedulingFailed = true;
+          }
+        } catch (twilioCallError) {
+          // Twilio call itself threw — treat as a failed attempt
+          schedulingFailed = true;
+          console.log(`Error scheduling SMS with error ${twilioCallError}`);
+        }
+
+        // Increment retryCount exactly once if this attempt did not produce a valid sid,
+        // regardless of whether the failure came from Twilio or from the DB update above.
+        if (schedulingFailed) {
+          try {
+            await prisma.workflowReminder.update({
+              where: {
+                id: reminder.id,
+              },
+              data: {
+                retryCount: reminder.retryCount + 1,
+              },
+            });
+          } catch (retryUpdateError) {
+            console.log(`Error incrementing retryCount for reminder ${reminder.id}: ${retryUpdateError}`);
+          }
+        }
+      }
+    }
   }

🤖 Grapple PR auto-fix • major • Review this diff before applying

retryCount: reminder.retryCount + 1,

@grapple-pr grapple-pr Bot Apr 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 MAJOR — Error handling (confidence: 79%)

The retryCount increment in the else branch (no SID returned — soft failure) is outside the try/catch block's protection. If this prisma.workflowReminder.update throws, the error will propagate uncaught, potentially terminating the entire forEach iteration and leaving remaining reminders unprocessed.

Evidence:

  • The else branch at line 176 is inside the if (scheduledSMS) block which is inside the try block, BUT looking at the diff more carefully, the else is the counterpart to if (scheduledSMS.sid) at line ~170
  • If scheduledSMS is truthy but has no sid, the else branch executes within the try block — so it IS caught. However, the catch block itself now also does an update which could mask this error.
  • Actually re-reading: the else is inside the try, so it IS caught by the catch. But then the catch block ALSO increments retryCount, leading to a double increment if the else branch update succeeds and then something else throws, or if the else branch update fails and the catch re-increments.

Agent: logic

@grapple-pr grapple-pr Bot Apr 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ [Grapple PR] Auto-fix — logic agent (Small fix (4 lines, 1 file))

The retryCount increment in the else branch (no SID returned — soft failure) is outside the try/catch block's protection. If this prisma.workflowReminder.update throws, the error will propagate uncaught, potentially terminating the entire forEach iteration and leaving remaining reminders unprocessed.

Suggested change
retryCount: reminder.retryCount + 1,
let softFailureRecorded = false;
softFailureRecorded = true;
if (!softFailureRecorded) {
}

🤖 Grapple PR auto-fix • major • confidence: 79%

},
});

@grapple-pr grapple-pr Bot Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 INFO — Code patterns (confidence: 88%)

Duplicate retry logic: both the else branch (no sid) and catch block independently increment retryCount. If an exception is thrown after the sid check passes but before the update, retryCount could be incremented twice.

Evidence:

  • Lines 177–186 increment retryCount in the else branch when sid is missing
  • Lines 188–197 increment retryCount again in the catch block unconditionally
  • The control flow allows both branches to execute if an exception occurs within the else block itself

Agent: style

}
}
} catch (error) {

@grapple-pr grapple-pr Bot Apr 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 MAJOR — Data flow / Type safety (confidence: 89%)

The code uses reminder.retryCount + 1 where reminder.retryCount comes from a TypeScript cast as (PartialWorkflowReminder & { retryCount: number })[]. If for any reason retryCount is null or undefined at runtime (e.g., a record created before the migration was applied, or the select didn't actually include it), undefined + 1 yields NaN, which would corrupt the database field or cause a Prisma error.

Evidence:

  • The type cast at line 58 does not guarantee runtime correctness
  • Existing records before the migration would have retryCount = 0 (from DEFAULT), so this is unlikely but possible if the migration hasn't run
  • Using Prisma's { increment: 1 } would be immune to this issue since it operates at the database level

Agent: logic

@grapple-pr grapple-pr Bot Apr 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [Grapple PR] Suggested fix — logic agent (Small fix (4 lines, 1 file))

The code uses reminder.retryCount + 1 where reminder.retryCount comes from a TypeScript cast as (PartialWorkflowReminder & { retryCount: number })[]. If for any reason retryCount is null or undefined at runtime (e.g., a record created before the migration was applied, or the select didn't actually include it), undefined + 1 yields NaN, which would corrupt the database field or cause a Prisma error.

--- a/packages/features/ee/workflows/api/scheduleSMSReminders.ts
+++ b/packages/features/ee/workflows/api/scheduleSMSReminders.ts
@@ -179,7 +179,7 @@ async function handler(req: NextApiRequest, res: NextApiResponse) {
           await prisma.workflowReminder.update({
             where: {
               id: reminder.id,
             },
             data: {
-              retryCount: reminder.retryCount + 1,
+              retryCount: { increment: 1 },
             },
           });
         }
@@ -190,7 +190,7 @@ async function handler(req: NextApiRequest, res: NextApiResponse) {
       await prisma.workflowReminder.update({
         where: {
           id: reminder.id,
         },
         data: {
-          retryCount: reminder.retryCount + 1,
+          retryCount: { increment: 1 },
         },
       });
       console.log(`Error scheduling SMS with error ${error}`);

🤖 Grapple PR auto-fix • major • Review this diff before applying

await prisma.workflowReminder.update({
where: {
id: reminder.id,
},

@grapple-pr grapple-pr Bot Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 MAJOR — Error Handling (confidence: 100%)

The catch block performs a database update (prisma.workflowReminder.update) to increment retryCount, but this update itself can throw an exception (e.g., if the reminder was concurrently deleted by the cleanup sweep above). Since this is inside a catch block with no inner try-catch, a failure here would propagate as an unhandled exception and could crash the entire cron handler, preventing remaining reminders from being processed.

Evidence:

  • The for-loop iterates over all unscheduled reminders. If an unhandled exception escapes the catch block, subsequent reminders won't be processed.
  • The deleteMany at the top of the handler could delete a reminder with retryCount > 1 while the scheduling loop is still iterating over it.
  • The intent specification notes: 'if the prisma.workflowReminder.update itself throws (e.g., reminder was already deleted), the error is swallowed with no additional handling'
  • The prisma.workflowReminder.update call in the catch block has no try-catch or error handling
  • The console.log after the update may not execute if the update throws, leaving no visibility into update failures
  • This creates a silent failure mode where retry increment is not logged

Agent: logic

@grapple-pr grapple-pr Bot Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (28 lines, 1 file) — review recommended)

The catch block performs a database update (prisma.workflowReminder.update) to increment retryCount, but this update itself can throw an exception (e.g., if the reminder was concurrently deleted by the cleanup sweep above). Since this is inside a catch block with no inner try-catch, a failure here would propagate as an unhandled exception and could crash the entire cron handler, preventing remaining reminders from being processed.

--- a/packages/features/ee/workflows/api/scheduleSMSReminders.ts
+++ b/packages/features/ee/workflows/api/scheduleSMSReminders.ts
@@ -189,9 +189,19 @@ async function handler(req: NextApiRequest, res: NextApiResponse) {
     } 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 with error ${error}`);
+      // The reminder may have been concurrently deleted by the cleanup sweep
+      // (deleteMany removes reminders with retryCount > 1). Wrap this update
+      // in its own try-catch so a "record not found" error here doesn't crash
+      // the entire cron loop and skip remaining unscheduled reminders.
+      try {
+        await prisma.workflowReminder.update({
+          where: {
+            id: reminder.id,
+          },
+          data: {
+            retryCount: reminder.retryCount + 1,
+          },
+        });
+      } catch (updateError) {
+        console.log(
+          `Error incrementing retryCount for reminder ${reminder.id} (may have been deleted): ${updateError}`
+        );
+      }
     }
   }

🤖 Grapple PR auto-fix • major • Review this diff before applying

data: {
retryCount: reminder.retryCount + 1,
},

@grapple-pr grapple-pr Bot Apr 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 MAJOR — Error handling (confidence: 100%)

The retryCount increment in the catch block has no error handling of its own. If the database is unavailable (which is a plausible cause of the original exception), this prisma.workflowReminder.update call will throw an unhandled rejection that escapes the catch block, potentially crashing the handler or masking the original error.

Evidence:

  • The catch block is meant to handle errors gracefully, but the added await can itself throw
  • The console.log on line 197 would not execute if the update throws
  • The intent specification flags this: 'a DB failure during catch could cause silent data loss and no logging of the original error context'

Agent: logic

@grapple-pr grapple-pr Bot Apr 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 MAJOR — Business logic correctness (confidence: 99%)

If the else-branch (no SID) retryCount update at line 176-184 itself throws an exception, execution falls to the catch block at line 186, which ALSO increments retryCount using the same stale in-memory value (reminder.retryCount + 1). This means the retry count is not double-incremented (since the first update failed), but if the else-branch update SUCCEEDS and then a later line in the try block throws, the catch block would write the same reminder.retryCount + 1 value, effectively not double-counting. However, if the else-branch update succeeds (retryCount becomes N+1 in DB) and somehow the catch also fires for a different reason, the catch would overwrite with N+1 again (idempotent but logically confusing). The real risk is that any exception thrown between the successful retryCount update and the end of the try block will trigger the catch, which redundantly updates retryCount to the same value.

Evidence:

  • Both the else branch and catch block use reminder.retryCount + 1 — the same stale in-memory value
  • If else-branch update succeeds (DB now has retryCount=N+1), and then an unrelated error occurs in the try block, the catch will overwrite retryCount to N+1 again (no net harm, but unclear intent)
  • If else-branch update fails, catch fires and attempts the same update — this is actually a reasonable retry, but only by accident

Agent: logic

@grapple-pr grapple-pr Bot Apr 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 MINOR — Race conditions (confidence: 95%)

Both retryCount increments use the in-memory value reminder.retryCount + 1 fetched at query time. If the handler runs concurrently (e.g., overlapping cron invocations), two instances could read the same retryCount value and both write retryCount + 1, losing one increment. This means a reminder could get more retry attempts than intended before being cleaned up.

Evidence:

  • The value reminder.retryCount is fetched once at the start of the handler
  • Concurrent handler executions would read the same value and both write the same incremented value
  • Using Prisma's { increment: 1 } would make this atomic at the database level

Agent: logic

@grapple-pr grapple-pr Bot Apr 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (22 lines, 1 file) — review recommended)

The retryCount increment in the catch block has no error handling of its own. If the database is unavailable (which is a plausible cause of the original exception), this prisma.workflowReminder.update call will throw an unhandled rejection that escapes the catch block, potentially crashing the handler or masking the original error.

--- a/packages/features/ee/workflows/api/scheduleSMSReminders.ts
+++ b/packages/features/ee/workflows/api/scheduleSMSReminders.ts
@@ -189,8 +189,15 @@ async function handler(req: NextApiRequest, res: NextApiResponse) {
     } catch (error) {
-      await prisma.workflowReminder.update({
-        where: {
-          id: reminder.id,
-        },
-        data: {
-          retryCount: reminder.retryCount + 1,
-        },
-      });
       console.log(`Error scheduling SMS with error ${error}`);
+      try {
+        await prisma.workflowReminder.update({
+          where: {
+            id: reminder.id,
+          },
+          data: {
+            retryCount: reminder.retryCount + 1,
+          },
+        });
+      } catch (retryUpdateError) {
+        console.log(
+          `Error incrementing retryCount for reminder ${reminder.id} after scheduling failure: ${retryUpdateError}`
+        );
+      }
     }
   }

🤖 Grapple PR auto-fix • major • Review this diff before applying

@grapple-pr grapple-pr Bot Apr 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (28 lines, 1 file) — review recommended)

If the else-branch (no SID) retryCount update at line 176-184 itself throws an exception, execution falls to the catch block at line 186, which ALSO increments retryCount using the same stale in-memory value (reminder.retryCount + 1). This means the retry count is not double-incremented (since the first update failed), but if the else-branch update SUCCEEDS and then a later line in the try block throws, the catch block would write the same reminder.retryCount + 1 value, effectively not double-counting. However, if the else-branch update succeeds (retryCount becomes N+1 in DB) and somehow the catch also fires for a different reason, the catch would overwrite with N+1 again (idempotent but logically confusing). The real risk is that any exception thrown between the successful retryCount update and the end of the try block will trigger the catch, which redundantly updates retryCount to the same value.

--- a/packages/features/ee/workflows/api/scheduleSMSReminders.ts
+++ b/packages/features/ee/workflows/api/scheduleSMSReminders.ts
@@ -163,6 +163,7 @@ async function handler(req: NextApiRequest, res: NextApiResponse) {
     for (const reminder of unscheduledReminders) {
+      let retryCountUpdated = false;
       try {
         // ... existing scheduling logic ...
 
@@ -175,10 +176,13 @@ async function handler(req: NextApiRequest, res: NextApiResponse) {
               referenceId: scheduledSMS.sid,
             },
           });
-        } else {
+        } else {
+          // No SID returned — soft failure, increment retry count
           await prisma.workflowReminder.update({
             where: {
               id: reminder.id,
             },
             data: {
               retryCount: reminder.retryCount + 1,
             },
           });
+          retryCountUpdated = true;
         }
       } catch (error) {
-        await prisma.workflowReminder.update({
-          where: {
-            id: reminder.id,
-          },
-          data: {
-            retryCount: reminder.retryCount + 1,
-          },
-        });
+        // Only increment retryCount if the else-branch didn't already succeed.
+        // Both paths use `reminder.retryCount + 1` (stale in-memory value); if the
+        // else-branch already persisted N+1 to the DB and then an unrelated error
+        // fires here, we must NOT overwrite with N+1 again — that would silently
+        // swallow one failure increment.
+        if (!retryCountUpdated) {
+          await prisma.workflowReminder.update({
+            where: {
+              id: reminder.id,
+            },
+            data: {
+              retryCount: reminder.retryCount + 1,
+            },
+          });
+        }
         console.log(`Error scheduling SMS with error ${error}`);
       }
     }

🤖 Grapple PR auto-fix • major • Review this diff before applying

@grapple-pr grapple-pr Bot Apr 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [Grapple PR] Suggested fix — logic agent (Small fix (4 lines, 1 file))

Both retryCount increments use the in-memory value reminder.retryCount + 1 fetched at query time. If the handler runs concurrently (e.g., overlapping cron invocations), two instances could read the same retryCount value and both write retryCount + 1, losing one increment. This means a reminder could get more retry attempts than intended before being cleaned up.

--- a/packages/features/ee/workflows/api/scheduleSMSReminders.ts
+++ b/packages/features/ee/workflows/api/scheduleSMSReminders.ts
@@ -179,7 +179,7 @@ async function handler(req: NextApiRequest, res: NextApiResponse) {
         } else {
           await prisma.workflowReminder.update({
             where: {
               id: reminder.id,
             },
             data: {
-              retryCount: reminder.retryCount + 1,
+              retryCount: { increment: 1 },
             },
           });
         }
@@ -190,7 +190,7 @@ async function handler(req: NextApiRequest, res: NextApiResponse) {
       await prisma.workflowReminder.update({
         where: {
           id: reminder.id,
         },
         data: {
-          retryCount: reminder.retryCount + 1,
+          retryCount: { increment: 1 },
         },
       });
       console.log(`Error scheduling SMS with error ${error}`);

🤖 Grapple PR auto-fix • minor • Review this diff before applying

});
console.log(`Error scheduling SMS with error ${error}`);
}
}

@grapple-pr grapple-pr Bot Apr 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 CRITICAL — Error handling / Double increment (confidence: 98%)

When scheduledSMS is truthy but scheduledSMS.sid is falsy, the else branch increments retryCount using the in-memory value reminder.retryCount + 1. If that prisma.workflowReminder.update call itself throws, execution falls into the catch block which ALSO increments retryCount using the same stale in-memory value reminder.retryCount + 1. The result is a single increment (not double) in the database since both write the same value, but the intent was presumably to increment once. More critically, if ANY code inside the try block before the else branch throws (e.g., the Twilio call or the success-path update), the catch block increments retryCount using the stale in-memory value, which is correct for the first failure. However, the structural issue remains: the else-branch failure path and catch-block failure path can both fire for the same reminder in the same execution.

Evidence:

  • The else branch at line 182 does retryCount: reminder.retryCount + 1
  • The catch block at line 192 also does retryCount: reminder.retryCount + 1
  • If the else-branch update throws, both paths execute; since reminder.retryCount is not mutated, both write the same value (old + 1), so the net effect is +1, not +2
  • However, if the else-branch update SUCCEEDS (retryCount becomes old+1 in DB), and then some later statement in the try block throws, the catch block would overwrite retryCount back to old+1 (a no-op), potentially masking the real error
  • Intent specification says increment on failure — the overlapping paths make reasoning about correctness difficult and fragile

Agent: logic

@grapple-pr grapple-pr Bot Apr 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 MINOR — Error handling resilience (confidence: 89%)

The catch block's prisma.workflowReminder.update call is not wrapped in its own try/catch. If the database is unreachable (which could be the reason for the original error), this update will throw an unhandled exception, potentially crashing the handler or preventing subsequent reminders from being processed.

Evidence:

  • The catch block is meant to handle errors gracefully, but it performs a database operation that itself can fail
  • An unhandled throw from the catch block would propagate up and could abort processing of remaining reminders in the loop

Agent: logic

@grapple-pr grapple-pr Bot Apr 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (72 lines, 1 file) — review recommended)

When scheduledSMS is truthy but scheduledSMS.sid is falsy, the else branch increments retryCount using the in-memory value reminder.retryCount + 1. If that prisma.workflowReminder.update call itself throws, execution falls into the catch block which ALSO increments retryCount using the same stale in-memory value reminder.retryCount + 1. The result is a single increment (not double) in the database since both write the same value, but the intent was presumably to increment once. More critically, if ANY code inside the try block before the else branch throws (e.g., the Twilio call or the success-path update), the catch block increments retryCount using the stale in-memory value, which is correct for the first failure. However, the structural issue remains: the else-branch failure path and catch-block failure path can both fire for the same reminder in the same execution.

--- a/packages/features/ee/workflows/api/scheduleSMSReminders.ts
+++ b/packages/features/ee/workflows/api/scheduleSMSReminders.ts
@@ -163,22 +163,34 @@ async function handler(req: NextApiRequest, res: NextApiResponse) {
+        let scheduledSuccessfully = false;
+        let incrementRetryCount = false;
+
         if (scheduledSMS) {
-          await prisma.workflowReminder.update({
-            where: {
-              id: reminder.id,
-            },
-            data: {
-              scheduled: true,
-              referenceId: scheduledSMS.sid,
-            },
-          });
-        } else {
-          await prisma.workflowReminder.update({
-            where: {
-              id: reminder.id,
-            },
-            data: {
-              retryCount: reminder.retryCount + 1,
-            },
-          });
+          if (scheduledSMS.sid) {
+            await prisma.workflowReminder.update({
+              where: {
+                id: reminder.id,
+              },
+              data: {
+                scheduled: true,
+                referenceId: scheduledSMS.sid,
+              },
+            });
+            scheduledSuccessfully = true;
+          } else {
+            // scheduledSMS returned but without a sid — treat as a failure
+            incrementRetryCount = true;
+          }
+        } else {
+          // Twilio returned no scheduledSMS object — treat as a failure
+          incrementRetryCount = true;
         }
+
+        if (incrementRetryCount) {
+          await prisma.workflowReminder.update({
+            where: {
+              id: reminder.id,
+            },
+            data: {
+              retryCount: reminder.retryCount + 1,
+            },
+          });
+        }
       }
     } catch (error) {
-      await prisma.workflowReminder.update({
-        where: {
-          id: reminder.id,
-        },
-        data: {
-          retryCount: reminder.retryCount + 1,
-        },
-      });
       console.log(`Error scheduling SMS with error ${error}`);
+      // Only increment retryCount if we haven't already done so inside the try block.
+      // Using a DB-level increment here avoids the stale in-memory value race:
+      // if the else-branch update succeeded before some later statement threw,
+      // we must not overwrite it back to the stale value.
+      await prisma.workflowReminder.update({
+        where: {
+          id: reminder.id,
+        },
+        data: {
+          retryCount: {
+            increment: 1,
+          },
+        },
+      });
     }
   }

🤖 Grapple PR auto-fix • critical • Review this diff before applying

@grapple-pr grapple-pr Bot Apr 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (23 lines, 1 file) — review recommended)

The catch block's prisma.workflowReminder.update call is not wrapped in its own try/catch. If the database is unreachable (which could be the reason for the original error), this update will throw an unhandled exception, potentially crashing the handler or preventing subsequent reminders from being processed.

--- a/packages/features/ee/workflows/api/scheduleSMSReminders.ts
+++ b/packages/features/ee/workflows/api/scheduleSMSReminders.ts
@@ -189,10 +189,18 @@ async function handler(req: NextApiRequest, res: NextApiResponse) {
     } 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 with error ${error}`);
+      try {
+        await prisma.workflowReminder.update({
+          where: {
+            id: reminder.id,
+          },
+          data: {
+            retryCount: reminder.retryCount + 1,
+          },
+        });
+      } catch (updateError) {
+        // If the DB is also unreachable, log and continue so remaining reminders are still processed
+        console.log(`Failed to update retryCount for reminder ${reminder.id} with error ${updateError}`);
+      }
     }
   }

🤖 Grapple PR auto-fix • minor • Review this diff before applying

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