SMS workflow reminder retry count tracking#3
Conversation
* add retry count to workflow reminder * add logic to for retry count --------- Co-authored-by: CarinaWolli <wollencarina@gmail.com> Co-authored-by: Udit Takkar <53316345+Udit-takkar@users.noreply.github.com>
🤖 Grapple PR ReviewSummaryThis PR adds 1 file (migration.sql), modifies 2 files. Title: "SMS workflow reminder retry count tracking" Changes: 3 files (+38 / -6 lines)
Resolves: #9 (feat: 2fa backup codes) CI: ⏳ 0 passed, 0 failed Impact Diagramgraph TD
pr_0[/"🔀 PR Changes: 3 files"/]
f1["🟡 scheduleSMSReminders.ts +35/-6"]
pr_0 --> f1
f2["🟢 migration.sql +2/-0"]
pr_0 --> f2
f3["🟡 schema.prisma +1/-0"]
pr_0 --> f3
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
Review details
Powered by Grapple PR — AI that understands your code. |
![grapple-pr[bot]](https://avatars.githubusercontent.com/in/3205222?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Grapple PR Review
Summary
This PR adds a retryCount field to the WorkflowReminder model to track how many times an SMS workflow reminder has failed to schedule, and increments this count on both soft failures (no SID returned) and exceptions during scheduling.
Changes: 3 files (+38 / -6 lines)
- 🟢 1 file added:
packages/prisma/migrations/20240508134359_add_retry_count_to_workflow_reminder/migration.sql - 🟡 2 files modified:
packages/features/ee/workflows/api/scheduleSMSReminders.ts,packages/prisma/schema.prisma
Resolves: #9 (feat: 2fa backup codes)
CI: ⏳ 0 passed, 0 failed
Impact Diagram
graph TD
pr_0[/"🔀 PR Changes: 3 files"/]
f1["🟡 scheduleSMSReminders.ts +35/-6"]
pr_0 --> f1
f2["🟢 migration.sql +2/-0"]
pr_0 --> f2
f3["🟡 schema.prisma +1/-0"]
pr_0 --> f3
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
1 critical issue found — please address before merging.
| Severity | Count |
|---|---|
| 🔴 Critical | 1 |
| 🟠 Major | 3 |
| 🟡 Minor | 1 |
| 🔵 Info | 1 |
Review details
- Reviewed at: 2026-04-07T18:36:33.212Z
- Agents used: security, logic, style, architecture, performance, verification
- Context level: full
- Execution time: 53.9s
- Blast radius: 0 (contained)
Powered by Grapple PR — AI that understands your code.
| }, | ||
| }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
🔴 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
| }, | ||
| data: { | ||
| retryCount: reminder.retryCount + 1, | ||
| }, |
There was a problem hiding this comment.
🟠 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
| }, | ||
| data: { | ||
| retryCount: reminder.retryCount + 1, | ||
| }, |
There was a problem hiding this comment.
🟠 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
| id: reminder.id, | ||
| }, | ||
| data: { | ||
| retryCount: reminder.retryCount + 1, |
There was a problem hiding this comment.
🟠 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 toif (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
| }, | ||
| data: { | ||
| retryCount: reminder.retryCount + 1, | ||
| }, |
There was a problem hiding this comment.
🟡 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.retryCountis 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
| { | ||
| retryCount: { | ||
| gt: 1, | ||
| }, |
There was a problem hiding this comment.
🔵 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
| }, | ||
| }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (1 lines, 1 file))
The second OR branch in deleteMany has no
method: WorkflowMethods.SMSfilter, 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.
| }, | |
| method: WorkflowMethods.SMS, |
🤖 Grapple PR auto-fix • critical • confidence: 100%
| id: reminder.id, | ||
| }, | ||
| data: { | ||
| retryCount: reminder.retryCount + 1, |
There was a problem hiding this comment.
✅ [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.updatethrows, the error will propagate uncaught, potentially terminating the entire forEach iteration and leaving remaining reminders unprocessed.
| retryCount: reminder.retryCount + 1, | |
| let softFailureRecorded = false; | |
| softFailureRecorded = true; | |
| if (!softFailureRecorded) { | |
| } |
🤖 Grapple PR auto-fix • major • confidence: 79%
| }, | ||
| data: { | ||
| retryCount: reminder.retryCount + 1, | ||
| }, |
There was a problem hiding this comment.
🟡 [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.updatecall 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
| }, | ||
| data: { | ||
| retryCount: reminder.retryCount + 1, | ||
| }, |
There was a problem hiding this comment.
🟡 [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 samereminder.retryCount + 1value, 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
| }, | ||
| data: { | ||
| retryCount: reminder.retryCount + 1, | ||
| }, |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Small fix (4 lines, 1 file))
Both retryCount increments use the in-memory value
reminder.retryCount + 1fetched at query time. If the handler runs concurrently (e.g., overlapping cron invocations), two instances could read the same retryCount value and both writeretryCount + 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
There was a problem hiding this comment.
🤖 Grapple PR Review
Summary
This PR adds a retryCount field to WorkflowReminder to track how many times an SMS reminder has failed to schedule, incrementing the count on both scheduling failures and non-successful Twilio responses. Reminders with a retryCount greater than 1 are also added to the deletion sweep.
Changes: 3 files (+38 / -6 lines)
- 🟢 1 file added:
packages/prisma/migrations/20240508134359_add_retry_count_to_workflow_reminder/migration.sql - 🟡 2 files modified:
packages/features/ee/workflows/api/scheduleSMSReminders.ts,packages/prisma/schema.prisma
Resolves: #9 (feat: 2fa backup codes)
CI: ⏳ 0 passed, 0 failed
Impact Diagram
graph TD
pr_0[/"🔀 PR Changes: 3 files"/]
f1["🟡 scheduleSMSReminders.ts +35/-6"]
pr_0 --> f1
f2["🟢 migration.sql +2/-0"]
pr_0 --> f2
f3["🟡 schema.prisma +1/-0"]
pr_0 --> f3
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
1 critical issue found — please address before merging.
| Severity | Count |
|---|---|
| 🔴 Critical | 1 |
| 🟠 Major | 2 |
| 🟡 Minor | 3 |
| 🔵 Info | 2 |
Review details
- Reviewed at: 2026-04-08T02:37:09.178Z
- Agents used: security, logic, style, architecture, performance, verification
- Context level: full
- Execution time: 58.6s
- Low-confidence findings suppressed: 1
- Blast radius: 0 (contained)
Powered by Grapple PR — AI that understands your code.
| retryCount: { | ||
| gt: 1, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🔴 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
| await prisma.workflowReminder.update({ | ||
| where: { | ||
| id: reminder.id, | ||
| }, |
There was a problem hiding this comment.
🟠 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.updatecall 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
| where: { | ||
| id: reminder.id, | ||
| }, | ||
| data: { |
There was a problem hiding this comment.
🟠 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.updatewithretryCount: reminder.retryCount + 1. - If that update throws, the catch block (line 184-193) also calls
prisma.workflowReminder.updatewithretryCount: 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.retryCountvalue. - However, if the else-branch update fails before writing, the catch block would correctly increment once. The behavior is ambiguous and fragile.
Agent: logic
| } else { | ||
| await prisma.workflowReminder.update({ | ||
| where: { | ||
| id: reminder.id, |
There was a problem hiding this comment.
🟡 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
| retryCount: { | ||
| gt: 1, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🟡 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 bymethod: 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
| }, | ||
| { | ||
| retryCount: { | ||
| gt: 1, |
There was a problem hiding this comment.
🟡 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: 1which translates to retryCount >= 2
Agent: logic
| }, | ||
| ], | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🔵 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
| data: { | ||
| retryCount: reminder.retryCount + 1, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🔵 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
| retryCount: { | ||
| gt: 1, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (1 lines, 1 file))
The
retryCount > 1deletion condition lacks amethod: WorkflowMethods.SMSfilter, meaning ANY WorkflowReminder (EMAIL, WHATSAPP, etc.) withretryCount > 1will be deleted by this SMS-specific cron handler. This is almost certainly unintentional — the handler isscheduleSMSRemindersand should only clean up SMS reminders.
| }, | |
| method: WorkflowMethods.SMS, |
🤖 Grapple PR auto-fix • critical • confidence: 100%
| } else { | ||
| await prisma.workflowReminder.update({ | ||
| where: { | ||
| id: reminder.id, |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (4 lines, 1 file))
The
retryCountincrement uses the in-memory valuereminder.retryCount + 1rather than an atomic Prismaincrementoperation. If two cron jobs run concurrently (no mutex is visible), both could read the sameretryCountvalue and writeretryCount + 1, effectively losing an increment (last-write-wins).
| id: reminder.id, | |
| retryCount: { increment: 1 }, | |
| retryCount: { increment: 1 }, |
🤖 Grapple PR auto-fix • minor • confidence: 100%
| retryCount: { | ||
| gt: 1, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
✅ [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.
| }, | |
| method: WorkflowMethods.SMS, |
🤖 Grapple PR auto-fix • minor • confidence: 100%
| }, | ||
| { | ||
| retryCount: { | ||
| gt: 1, |
There was a problem hiding this comment.
✅ [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 begt: 0orgte: 1.
| 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%
| await prisma.workflowReminder.update({ | ||
| where: { | ||
| id: reminder.id, | ||
| }, |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (28 lines, 1 file) — review recommended)
The
catchblock performs a database update (prisma.workflowReminder.update) to incrementretryCount, 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
| where: { | ||
| id: reminder.id, | ||
| }, | ||
| data: { |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (73 lines, 1 file) — review recommended)
Both the
elsebranch (no valid sid) and thecatchblock independently incrementretryCount. If the Twilio call succeeds but returns no sid, and then theprisma.workflowReminder.updatein theelsebranch throws an exception, execution will fall into thecatchblock which will attempt to incrementretryCountagain. 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
|
@grapple-pr review |
There was a problem hiding this comment.
🤖 Grapple PR Review
Summary
This PR adds a retryCount field to the WorkflowReminder model to track how many times an SMS reminder has failed to schedule, and increments that counter on both explicit send failures and caught exceptions. It also modifies the cleanup deletion logic to remove reminders that have been retried more than once.
Changes: 3 files (+38 / -6 lines)
- 🟢 1 file added:
packages/prisma/migrations/20240508134359_add_retry_count_to_workflow_reminder/migration.sql - 🟡 2 files modified:
packages/features/ee/workflows/api/scheduleSMSReminders.ts,packages/prisma/schema.prisma
Resolves: #9 (feat: 2fa backup codes)
CI: ⏳ 0 passed, 0 failed
Impact Diagram
graph TD
pr_0[/"🔀 PR Changes: 3 files"/]
f1["🟡 scheduleSMSReminders.ts +35/-6"]
pr_0 --> f1
f2["🟢 migration.sql +2/-0"]
pr_0 --> f2
f3["🟡 schema.prisma +1/-0"]
pr_0 --> f3
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
2 critical issues found — please address before merging.
| Severity | Count |
|---|---|
| 🔴 Critical | 2 |
| 🟠 Major | 2 |
| 🟡 Minor | 1 |
Review details
- Reviewed at: 2026-04-12T02:03:22.613Z
- Agents used: security, logic, style, architecture, performance, verification
- Context level: full
- Execution time: 54.7s
- Blast radius: 0 (contained)
Powered by Grapple PR — AI that understands your code.
| retryCount: { | ||
| gt: 1, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🔴 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.SMSbut the new OR branch drops that constraint - Intent specification explicitly calls out SMS retry tracking — non-SMS reminders should not be affected
Agent: logic
| }); | ||
| console.log(`Error scheduling SMS with error ${error}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 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.retryCountis 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
| retryCount: { | ||
| gt: 1, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🟠 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
| }); | ||
| } | ||
| } | ||
| } catch (error) { |
There was a problem hiding this comment.
🟠 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
| }); | ||
| console.log(`Error scheduling SMS with error ${error}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 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
| retryCount: { | ||
| gt: 1, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (1 lines, 1 file))
The
retryCountdeletion clause has nomethod: WorkflowMethods.SMSconstraint, meaning ANY WorkflowReminder (email, WhatsApp, etc.) withretryCount > 1will 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.
| }, | |
| method: WorkflowMethods.SMS, |
🤖 Grapple PR auto-fix • critical • confidence: 100%
| retryCount: { | ||
| gt: 1, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
✅ [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), thengt: 2would be correct. The current threshold is very aggressive — only 2 failures before permanent deletion.
| }, | |
| // 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%
| }); | ||
| console.log(`Error scheduling SMS with error ${error}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (72 lines, 1 file) — review recommended)
When
scheduledSMSis truthy butscheduledSMS.sidis falsy, the else branch incrementsretryCountusing the in-memory valuereminder.retryCount + 1. If thatprisma.workflowReminder.updatecall itself throws, execution falls into the catch block which ALSO incrementsretryCountusing the same stale in-memory valuereminder.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
| }); | ||
| } | ||
| } | ||
| } catch (error) { |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Small fix (4 lines, 1 file))
The code uses
reminder.retryCount + 1wherereminder.retryCountcomes from a TypeScript castas (PartialWorkflowReminder & { retryCount: number })[]. If for any reasonretryCountisnullorundefinedat runtime (e.g., a record created before the migration was applied, or the select didn't actually include it),undefined + 1yieldsNaN, 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
| }); | ||
| console.log(`Error scheduling SMS with error ${error}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (23 lines, 1 file) — review recommended)
The catch block's
prisma.workflowReminder.updatecall 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
2 participants
Benchmark test PR #9 - Replicating Greptile benchmark for Grapple PR evaluation.