Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
51 changes: 49 additions & 2 deletions controllers/CronController.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,53 @@
import { getScheduledActions } from "../lib/supabase/scheduled_actions/getScheduledActions";
import {
getScheduledActions,
ScheduledAction,
} from "../lib/supabase/scheduled_actions/getScheduledActions";
import parser from "cron-parser";

function shouldRunAction(action: ScheduledAction, now: Date): boolean {
if (!action.enabled) return false;
if (!action.schedule) return false;

// If never run, and next_run is now or earlier, run it
if (!action.last_run) {
if (!action.next_run) return true; // If next_run and last_run are null, run it
return new Date(action.next_run) <= now;
}

// If next_run is now or earlier, and last_run is before next_run, run it
if (action.next_run && new Date(action.next_run) <= now) {
if (
!action.last_run ||
new Date(action.last_run) < new Date(action.next_run)
) {
return true;
}
Comment on lines +11 to +24
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Repeated Date construction is wasteful & can cause subtle TZ mis-parsing.

new Date(action.next_run) is invoked multiple times and assumes the DB string includes a timezone.
If Supabase returns "2025-06-21 12:00:00" (no TZ), Node treats it as local time, whereas cron-parser defaults to system TZ, leading to silent drift on servers not in UTC.

-  if (!action.last_run) {
-    if (!action.next_run) return true; // If next_run and last_run are null, run it
-    return new Date(action.next_run) <= now;
+  if (!action.last_run) {
+    if (!action.next_run) return true;
+    const nextRun = new Date(`${action.next_run}Z`); // force UTC
+    return nextRun <= now;
   }

Cache the parsed dates once per action (const nextRun = …; const lastRun = …) to avoid repeated parsing and make the TZ handling explicit.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// If never run, and next_run is now or earlier, run it
if (!action.last_run) {
if (!action.next_run) return true; // If next_run and last_run are null, run it
return new Date(action.next_run) <= now;
}
// If next_run is now or earlier, and last_run is before next_run, run it
if (action.next_run && new Date(action.next_run) <= now) {
if (
!action.last_run ||
new Date(action.last_run) < new Date(action.next_run)
) {
return true;
}
// If never run, and next_run is now or earlier, run it
if (!action.last_run) {
if (!action.next_run) return true;
const nextRun = new Date(`${action.next_run}Z`); // force UTC
return nextRun <= now;
}
// If next_run is now or earlier, and last_run is before next_run, run it
if (action.next_run && new Date(action.next_run) <= now) {
if (
!action.last_run ||
new Date(action.last_run) < new Date(action.next_run)
) {
return true;
}
🤖 Prompt for AI Agents
In controllers/CronController.ts around lines 11 to 24, multiple calls to new
Date() on action.next_run and action.last_run cause repeated parsing and
potential timezone inconsistencies. Fix this by parsing action.next_run and
action.last_run once each into constants (e.g., nextRun and lastRun) at the
start of the function or block, then use these constants throughout the logic to
avoid repeated Date construction and clarify timezone handling.

}

// Fallback: use cron-parser to check if a run is due
try {
const interval = parser.parseExpression(action.schedule, {
currentDate: now,
});
const prev = interval.prev().toDate();
// If last_run is before the previous scheduled time, it's due
if (!action.last_run || new Date(action.last_run) < prev) {
return true;
}
Comment on lines +27 to +36
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

prev() + strict < risks skipping on-time executions.

cron-parser’s prev() returns the previous schedule excluding the currentDate by default.
Scenario: schedule * * * * * (every minute); now is exactly on the minute and last_run is 59 s ago.
prev() yields the previous minute, so last_run < prev is false, and the job is skipped even though it should run.

Either use interval.prev(true) (inclusive) or compare against interval.next():

-const prev = interval.prev().toDate();
-if (!action.last_run || new Date(action.last_run) < prev) {
+const next = interval.next().toDate();
+if (!action.last_run || new Date(action.last_run) < next) {
   return true;
 }

This guarantees an execution whenever now aligns with the cron expression.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Fallback: use cron-parser to check if a run is due
try {
const interval = parser.parseExpression(action.schedule, {
currentDate: now,
});
const prev = interval.prev().toDate();
// If last_run is before the previous scheduled time, it's due
if (!action.last_run || new Date(action.last_run) < prev) {
return true;
}
// Fallback: use cron-parser to check if a run is due
try {
const interval = parser.parseExpression(action.schedule, {
currentDate: now,
});
const next = interval.next().toDate();
// If last_run is before the next scheduled time, it's due
if (!action.last_run || new Date(action.last_run) < next) {
return true;
}
🤖 Prompt for AI Agents
In controllers/CronController.ts around lines 27 to 36, the use of
interval.prev() with a strict less-than comparison risks skipping executions
that are exactly on schedule. To fix this, change the call to
interval.prev(true) to make it inclusive, or alternatively compare last_run
against interval.next() instead. This ensures that jobs scheduled exactly at the
current time are not skipped.

} catch (e) {
console.error(
`[CronController] Invalid cron schedule for action ${action.id}:`,
action.schedule
);
}
return false;
}

export async function handleScheduledActions(): Promise<void> {
const scheduledActions = await getScheduledActions();
console.log("[CronController] Scheduled Actions:", scheduledActions);
const now = new Date();
const actionsToRun = scheduledActions.filter((action) =>
shouldRunAction(action, now)
);
console.log("[CronController] actionsToRun:", actionsToRun);
}
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@
"dependencies": {
"@supabase/supabase-js": "^2.50.0",
"dotenv": "^16.5.0",
"node-cron": "^4.1.0"
"node-cron": "^4.1.0",
"cron-parser": "^4.9.0"
},
"devDependencies": {
"@types/dotenv": "^8.2.3",
Expand Down
17 changes: 17 additions & 0 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.