SMS workflow reminder retry count tracking

[CodingKylo]

Martian Code Review Benchmark PR (mirrored from source #9)

[re-entry-ai]

🛡️ re-entry.ai Code Review

@CodingKylo — Please review the following assessment:

🚨 Risk Score: 76/100 · CRITICAL

Dimension	Level
Likelihood	🔴 Critical
Impact	🔴 Critical
Detectability	🟡 Medium

Intent

Add and use a retryCount field to track how many times scheduling an SMS reminder has failed, and adjust cleanup/selection logic accordingly.

Summary

The PR adds a new non-null retryCount column to WorkflowReminder and updates the SMS reminder scheduler to (a) retain reminders with retryCount > 1 even if their scheduledDate is in the past, (b) select retryCount for the scheduler, and (c) increment retryCount on both the “else” path and the catch path. The top risks are correctness/data integrity: the retry increment is duplicated across branches and uses a read-modify-write pattern that can race under concurrent runs, potentially causing lost updates or runaway retry growth. The reviewer must verify the intended retry semantics (what “else” represents vs actual failure) and ensure concurrency-safe updates and consistent cleanup behavior across all jobs that touch WorkflowReminder.

🎯 Review Focus

Verify the retry semantics and deletion predicate correctness in scheduleSMSReminders.ts: ensure the deleteMany filter matches the intended “retain vs delete” policy, and ensure retryCount increments happen exactly once per scheduling attempt using an atomic increment to avoid races/double-counting.

Key Findings

• 🚨 [packages/features/ee/workflows/api/scheduleSMSReminders.ts:L163] CRITICAL: retryCount is incremented in both the else branch and the catch block using the same reminder.retryCount + 1 read-modify-write pattern — await prisma.workflowReminder.update({ ... data: { retryCount: reminder.retryCount + 1 } }) — This can double-increment for a single reminder if the “else” path and subsequent operations throw, and it is also race-prone: two concurrent handler executions can both read the same reminder.retryCount and write back the same +1, causing lost updates (or, depending on control flow, repeated increments). Fix: make the increment atomic and single-path. Use Prisma atomic increment and ensure only one increment happens per attempt, e.g. await prisma.workflowReminder.update({ where: { id: reminder.id }, data: { retryCount: { increment: 1 } } }) and restructure so the increment occurs in exactly one place per attempt (either in the failure path or in the catch, but not both).
• ⚠️ [packages/features/ee/workflows/api/scheduleSMSReminders.ts:L28] WARNING: Cleanup logic now retains reminders with retryCount > 1 even when scheduledDate is in the past — OR: [ { scheduledDate: { lte: dayjs().toISOString() } }, { retryCount: { gt: 1 } } ] — This changes the deletion predicate from “past scheduledDate” to “past scheduledDate OR retryCount>1”, which means reminders with retryCount > 1 will be deleted even if they are not past, and reminders with scheduledDate <= now will be deleted regardless of retryCount. If the intent was “delete past reminders except those with retryCount>1”, the predicate is inverted. Fix: encode the intended policy explicitly, e.g. if intent is “delete past reminders unless retryCount>1”, use where: { method: SMS, scheduledDate: { lte: now }, retryCount: { lte: 1 } } (or NOT: { retryCount: { gt: 1 } }). Verify with a truth table against the desired behavior.

✅ Action Checklist

• [ ] CRITICAL [packages/features/ee/workflows/api/scheduleSMSReminders.ts:L163] — retryCount is incremented in both the else branch and the catch block using the same reminder.retryCount + 1 read-modify-write pattern — await prisma.workflowReminder.update({ ... data: { retryCount: reminder.retryCount + 1 } }) — This can double-increment for a single reminder if the “else” path and subsequent operations throw, and it is also race-prone: two concurrent handler executions can both read the same reminder.retryCount and write back the same +1, causing lost updates (or, depending on control flow, repeated increments). Fix: make the increment atomic and single-path. Use Prisma atomic increment and ensure only one increment happens per attempt, e.g. await prisma.workflowReminder.update({ where: { id: reminder.id }, data: { retryCount: { increment: 1 } } }) and restructure so the increment occurs in exactly one place per attempt (either in the failure path or in the catch, but not both).
• [ ] WARNING [packages/features/ee/workflows/api/scheduleSMSReminders.ts:L28] — Cleanup logic now retains reminders with retryCount > 1 even when scheduledDate is in the past — OR: [ { scheduledDate: { lte: dayjs().toISOString() } }, { retryCount: { gt: 1 } } ] — This changes the deletion predicate from “past scheduledDate” to “past scheduledDate OR retryCount>1”, which means reminders with retryCount > 1 will be deleted even if they are not past, and reminders with scheduledDate <= now will be deleted regardless of retryCount. If the intent was “delete past reminders except those with retryCount>1”, the predicate is inverted. Fix: encode the intended policy explicitly, e.g. if intent is “delete past reminders unless retryCount>1”, use where: { method: SMS, scheduledDate: { lte: now }, retryCount: { lte: 1 } } (or NOT: { retryCount: { gt: 1 } }). Verify with a truth table against the desired behavior.
• [ ] SUGGESTION — [packages/features/ee/workflows/api/scheduleSMSReminders.ts:L53] INFO: You added retryCount: true to the select, which is good for runtime consistency. However, the cast as (PartialWorkflowReminder & { retryCount: number })[] is still unsafe if any other code path changes select. Prefer to type the query result directly (e.g., define a const select = {...} and a corresponding TS type) or avoid the cast by using Prisma’s inferred payload types.
• [ ] SUGGESTION — [packages/features/ee/workflows/api/scheduleSMSReminders.ts:L175] WARNING: console.log in the catch path logs the raw error but does not include reminder.id or scheduledSMS.sid, making it hard to correlate failures with retryCount increments. Fix: log structured context: console.error('Error scheduling SMS reminder', { reminderId: reminder.id, scheduledSMSSid: scheduledSMS?.sid, error }).

Suggestions

• [packages/features/ee/workflows/api/scheduleSMSReminders.ts:L53] INFO: You added retryCount: true to the select, which is good for runtime consistency. However, the cast as (PartialWorkflowReminder & { retryCount: number })[] is still unsafe if any other code path changes select. Prefer to type the query result directly (e.g., define a const select = {...} and a corresponding TS type) or avoid the cast by using Prisma’s inferred payload types.
• [packages/features/ee/workflows/api/scheduleSMSReminders.ts:L175] WARNING: console.log in the catch path logs the raw error but does not include reminder.id or scheduledSMS.sid, making it hard to correlate failures with retryCount increments. Fix: log structured context: console.error('Error scheduling SMS reminder', { reminderId: reminder.id, scheduledSMSSid: scheduledSMS?.sid, error }).

---

Posted by re-entry.ai · Risk governance for autonomous engineering teams

[re-entry-ai]

🛡️ re-entry.ai Code Review

@CodingKylo — Please review the following assessment:

🚨 Risk Score: 90/100 · CRITICAL

Dimension	Level
Likelihood	🔴 Critical
Impact	🔴 Critical
Detectability	🟡 Medium

Summary

Code Review Summary

Risk Level: CRITICAL (Score: 90/100)
Files Changed: 0
High Risk Areas: 2

Key Concerns

• DATA (critical): A new non-null column (retryCount with default 0) is added to WorkflowReminder via a migration, which can affect existing data and downstream queries.
• CONFIGURATION (high): The logic now deletes reminders based on an OR condition involving retryCount>1 and increments retryCount in both the else and catch paths, which could unintentionally change retry semantics (e.g., incrementing on non-error branches or repeated runs).
• CONFIGURATION (medium): The handler’s database filtering and update behavior changes (including retryCount increments on both success-else and error paths), which can alter operational load and reminder scheduling outcomes.

---

Posted by re-entry.ai · Risk governance for autonomous engineering teams

[re-entry-ai]

🛡️ re-entry.ai Code Review

@CodingKylo — Please review the following assessment:

🚨 Risk Score: 90/100 · CRITICAL

Dimension	Level
Likelihood	🔴 Critical
Impact	🔴 Critical
Detectability	🟡 Medium

Intent

Update the SMS reminder scheduling flow to track and increment a per-reminder retry counter, and adjust cleanup logic to delete reminders based on retry count.

Summary

The PR adds a new non-null retryCount column to WorkflowReminder (defaulting to 0) and changes the SMS scheduling handler to (1) delete SMS reminders that are past due OR have retryCount > 1, and (2) increment retryCount in both the non-error and error paths. The highest risk is correctness/data-integrity: the handler now increments retry count even when scheduling succeeds (the else branch), which likely breaks retry semantics and can cause premature deletion. You should verify the intended retry policy (when to increment, when to delete) and confirm there are no other code paths that update WorkflowReminder without coordinating with this new global column.

🎯 Review Focus

Verify the retry semantics end-to-end: when retryCount should increment (only on real failures vs the current else path) and ensure the new deletion rule is correctly scoped to SMS reminders managed by this endpoint (not all reminder types).

Key Findings

• 🚨 [packages/features/ee/workflows/api/scheduleSMSReminders.ts:L163] CRITICAL: else branch increments retryCount on non-error path — await prisma.workflowReminder.update({ ... data: { retryCount: reminder.retryCount + 1 } }) — This increments retry count even when the SMS scheduling did not throw (i.e., the code reached the else), which will inflate retryCount and can trigger the new cleanup rule (retryCount > 1) leading to premature deletion of reminders. Fix: only increment retryCount on actual retry-worthy failures; move the increment into the catch (and/or into a specific failure condition), and remove the else increment entirely unless the else corresponds to a real failure state. Example: delete the entire } else { ... } block and keep only the catch update.
• ⚠️ [packages/features/ee/workflows/api/scheduleSMSReminders.ts:L28] WARNING: Cleanup logic now deletes by retryCount > 1 without scoping to SMS-managed reminders — OR: [ { method: WorkflowMethods.SMS, scheduledDate: { lte: ... } }, { retryCount: { gt: 1 } } ] — Because the second OR clause is not constrained by method, any reminder row (including non-SMS reminder types) with retryCount > 1 will be deleted, which is a data-integrity risk given retryCount is a global column on WorkflowReminder. Fix: scope the retry-based deletion to SMS reminders (or to the exact subset this endpoint manages). Example: change to where: { method: WorkflowMethods.SMS, OR: [ { scheduledDate: { lte: ... } }, { retryCount: { gt: 1 } } ] } or add method: WorkflowMethods.SMS inside the retryCount clause.
• ⚠️ [packages/features/ee/workflows/api/scheduleSMSReminders.ts:L53] WARNING: Unsafe type assertion relies on retryCount being selected — })) as (PartialWorkflowReminder & { retryCount: number })[]; — This is brittle: if select is modified later (or reused) and retryCount is not included, runtime will produce undefined and reminder.retryCount + 1 will yield NaN or throw depending on usage. Fix: avoid casting and instead type the Prisma query result explicitly with a concrete select object that always includes retryCount, e.g. const unscheduledReminders = await prisma.workflowReminder.findMany({ where: ..., select: { ...select, retryCount: true } }) satisfies Array<Pick<WorkflowReminder, ...> & { retryCount: number }> or define a dedicated select constant for this handler and use it consistently.

✅ Action Checklist

• [ ] CRITICAL [packages/features/ee/workflows/api/scheduleSMSReminders.ts:L163] — else branch increments retryCount on non-error path — await prisma.workflowReminder.update({ ... data: { retryCount: reminder.retryCount + 1 } }) — This increments retry count even when the SMS scheduling did not throw (i.e., the code reached the else), which will inflate retryCount and can trigger the new cleanup rule (retryCount > 1) leading to premature deletion of reminders. Fix: only increment retryCount on actual retry-worthy failures; move the increment into the catch (and/or into a specific failure condition), and remove the else increment entirely unless the else corresponds to a real failure state. Example: delete the entire } else { ... } block and keep only the catch update.
• [ ] WARNING [packages/features/ee/workflows/api/scheduleSMSReminders.ts:L28] — Cleanup logic now deletes by retryCount > 1 without scoping to SMS-managed reminders — OR: [ { method: WorkflowMethods.SMS, scheduledDate: { lte: ... } }, { retryCount: { gt: 1 } } ] — Because the second OR clause is not constrained by method, any reminder row (including non-SMS reminder types) with retryCount > 1 will be deleted, which is a data-integrity risk given retryCount is a global column on WorkflowReminder. Fix: scope the retry-based deletion to SMS reminders (or to the exact subset this endpoint manages). Example: change to where: { method: WorkflowMethods.SMS, OR: [ { scheduledDate: { lte: ... } }, { retryCount: { gt: 1 } } ] } or add method: WorkflowMethods.SMS inside the retryCount clause.
• [ ] WARNING [packages/features/ee/workflows/api/scheduleSMSReminders.ts:L53] — Unsafe type assertion relies on retryCount being selected — })) as (PartialWorkflowReminder & { retryCount: number })[]; — This is brittle: if select is modified later (or reused) and retryCount is not included, runtime will produce undefined and reminder.retryCount + 1 will yield NaN or throw depending on usage. Fix: avoid casting and instead type the Prisma query result explicitly with a concrete select object that always includes retryCount, e.g. const unscheduledReminders = await prisma.workflowReminder.findMany({ where: ..., select: { ...select, retryCount: true } }) satisfies Array<Pick<WorkflowReminder, ...> & { retryCount: number }> or define a dedicated select constant for this handler and use it consistently.
• [ ] SUGGESTION — [packages/features/ee/workflows/api/scheduleSMSReminders.ts:L28] Add a transaction and/or atomic update to prevent race conditions when multiple scheduler runs overlap — you currently do findMany then later update per reminder (retryCount: reminder.retryCount + 1). If two runs process the same reminder concurrently, increments can be lost. Fix: use updateMany with an atomic increment (Prisma supports increment) and include a guard on current state, e.g. data: { retryCount: { increment: 1 } } and optionally where: { id: reminder.id, retryCount: reminder.retryCount }.
• [ ] SUGGESTION — [packages/features/ee/workflows/api/scheduleSMSReminders.ts:L163] Replace per-reminder updates with a batched update to reduce DB load — the handler updates retryCount inside a loop (await prisma.workflowReminder.update(...)), which can become N+1 queries. Fix: collect IDs for the catch and else cases and run updateMany with where: { id: { in: ids } } and data: { retryCount: { increment: 1 } }.
• [ ] SUGGESTION — [packages/prisma/schema.prisma:L997] Operational verification: confirm the migration is compatible with existing rows and app expectations — retryCount is Int @default(0) and the migration adds NOT NULL DEFAULT 0, which is safe for existing rows, but you must verify all code paths that read WorkflowReminder tolerate the new field and that no other cleanup logic assumes a different meaning for retryCount. Fix: audit other WorkflowReminder mutations and ensure they either set retryCount appropriately or do not rely on it for deletion.

Suggestions

• [packages/features/ee/workflows/api/scheduleSMSReminders.ts:L28] Add a transaction and/or atomic update to prevent race conditions when multiple scheduler runs overlap — you currently do findMany then later update per reminder (retryCount: reminder.retryCount + 1). If two runs process the same reminder concurrently, increments can be lost. Fix: use updateMany with an atomic increment (Prisma supports increment) and include a guard on current state, e.g. data: { retryCount: { increment: 1 } } and optionally where: { id: reminder.id, retryCount: reminder.retryCount }.
• [packages/features/ee/workflows/api/scheduleSMSReminders.ts:L163] Replace per-reminder updates with a batched update to reduce DB load — the handler updates retryCount inside a loop (await prisma.workflowReminder.update(...)), which can become N+1 queries. Fix: collect IDs for the catch and else cases and run updateMany with where: { id: { in: ids } } and data: { retryCount: { increment: 1 } }.
• [packages/prisma/schema.prisma:L997] Operational verification: confirm the migration is compatible with existing rows and app expectations — retryCount is Int @default(0) and the migration adds NOT NULL DEFAULT 0, which is safe for existing rows, but you must verify all code paths that read WorkflowReminder tolerate the new field and that no other cleanup logic assumes a different meaning for retryCount. Fix: audit other WorkflowReminder mutations and ensure they either set retryCount appropriately or do not rely on it for deletion.

---

Posted by re-entry.ai · Risk governance for autonomous engineering teams

[re-entry-ai]

🛡️ re-entry.ai Code Review

🚨 Risk Score: 91/100 · CRITICAL

Dimension	Level
Likelihood	🔴 Critical
Impact	🔴 Critical
Detectability	🟠 High

Intent

Add and use a retryCount field on WorkflowReminder rows to track SMS reminder scheduling attempts and adjust cleanup/deletion behavior accordingly.

Summary

The PR introduces a new non-null retryCount column (default 0) and updates the scheduleSMSReminders handler to select and increment retryCount, plus change the cleanup deleteMany predicate. The top risk is correctness: the deleteMany predicate is broadened with an OR that can delete reminders that are still SMS-scheduled in the future whenever retryCount > 1, regardless of method/time window. A second high risk is retry accounting: retryCount is incremented in both the success/else path and again in the catch block, which can double-increment for a single reminder iteration and prematurely trigger the cleanup rule. Before merging, verify the intended retention semantics (what should be deleted and when) and add/adjust tests to cover mixed methods and failure paths to ensure retryCount increments exactly once per attempt.

🎯 Review Focus

Verify the retention/deletion semantics in the cleanup deleteMany query (the OR-scoped retryCount > 1 condition) and ensure retryCount is incremented exactly once per reminder attempt (no double-increment across else/catch), with tests covering mixed methods and failure paths.

✅ Action Checklist

• [ ] SUGGESTION — [packages/features/ee/workflows/api/scheduleSMSReminders.ts:L44] Replace the unsafe cast and ensure retryCount is always present/typed correctly. Current code does select: { ...select, retryCount: true } and then casts as (PartialWorkflowReminder & { retryCount: number })[]. This is still brittle because PartialWorkflowReminder suggests optional fields and the cast bypasses runtime guarantees. Fix by typing the query result explicitly (no cast) and using Prisma’s inferred type, e.g. define a local type from the select or use const unscheduledReminders = await prisma.workflowReminder.findMany({ ... }) and let TS infer the shape from select.
• [ ] SUGGESTION — [packages/features/ee/workflows/api/scheduleSMSReminders.ts:L163] Use atomic increments to avoid lost updates. The code uses retryCount: reminder.retryCount + 1, which can lose increments if two scheduler runs process the same reminder concurrently. Fix:

await prisma.workflowReminder.update({
  where: { id: reminder.id },
  data: { retryCount: { increment: 1 } },
});

This also removes reliance on the in-memory reminder.retryCount value being current.

• [ ] SUGGESTION — [packages/features/ee/workflows/api/scheduleSMSReminders.ts:L28] Add a regression test for mixed WorkflowReminder methods and future scheduledDate values. Specifically, create reminders with retryCount > 1 for non-SMS methods and/or future scheduledDate, and assert they are NOT deleted by the cleanup query. This directly validates the corrected predicate scoping.

Suggestions

• [packages/features/ee/workflows/api/scheduleSMSReminders.ts:L44] Replace the unsafe cast and ensure retryCount is always present/typed correctly. Current code does select: { ...select, retryCount: true } and then casts as (PartialWorkflowReminder & { retryCount: number })[]. This is still brittle because PartialWorkflowReminder suggests optional fields and the cast bypasses runtime guarantees. Fix by typing the query result explicitly (no cast) and using Prisma’s inferred type, e.g. define a local type from the select or use const unscheduledReminders = await prisma.workflowReminder.findMany({ ... }) and let TS infer the shape from select.
• [packages/features/ee/workflows/api/scheduleSMSReminders.ts:L163] Use atomic increments to avoid lost updates. The code uses retryCount: reminder.retryCount + 1, which can lose increments if two scheduler runs process the same reminder concurrently. Fix:

await prisma.workflowReminder.update({
  where: { id: reminder.id },
  data: { retryCount: { increment: 1 } },
});

This also removes reliance on the in-memory reminder.retryCount value being current.

• [packages/features/ee/workflows/api/scheduleSMSReminders.ts:L28] Add a regression test for mixed WorkflowReminder methods and future scheduledDate values. Specifically, create reminders with retryCount > 1 for non-SMS methods and/or future scheduledDate, and assert they are NOT deleted by the cleanup query. This directly validates the corrected predicate scoping.

📝 This review includes 2 inline comments (1 critical, 1 warning)

---

Posted by re-entry.ai · Risk governance for autonomous engineering teams

[re-entry-ai]

🚨 CRITICAL

The new deleteMany predicate is too broad: retryCount > 1 is ORed with the SMS/scheduledDate filter, so any reminder of any method with a retry count above 1 will be deleted, even if it is still scheduled in the future. The root cause is that the retry cleanup condition was added at the top level instead of being scoped to the SMS reminder criteria.

---

re-entry.ai

[re-entry-ai]

⚠️ WARNING

retryCount is incremented both in the normal else branch and again in the catch block. If the code path reaches the else branch and then later throws during the same reminder iteration, the same reminder can be counted twice, which will prematurely trip the new cleanup rule. This is a correctness bug in the retry accounting flow.

---

re-entry.ai