fix #11246 - Cron job update calculates nextRunAtMs 24 hours late#11282
fix #11246 - Cron job update calculates nextRunAtMs 24 hours late#11282Lucky-Lodhi2004 wants to merge 1 commit intoopenclaw:mainfrom
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const cron = new Cron(expr, { | ||
| timezone: resolveCronTimezone(schedule.tz), | ||
| catch: false, | ||
| timezone: schedule.tz?.trim() || undefined, | ||
| catch: true, | ||
| }); |
There was a problem hiding this comment.
Timezone fallback removed
computeNextRunAtMs previously defaulted to Intl.DateTimeFormat().resolvedOptions().timeZone when schedule.tz was missing/blank via resolveCronTimezone(). The new code passes timezone: schedule.tz?.trim() || undefined, which makes croner fall back to its default timezone (often UTC). For cron schedules stored without an explicit tz, this changes behavior and will shift next run times. If the intent is “use local timezone when tz is omitted”, this should continue calling resolveCronTimezone(schedule.tz) (or an equivalent fallback), not undefined.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cron/schedule.ts
Line: 48:51
Comment:
**Timezone fallback removed**
`computeNextRunAtMs` previously defaulted to `Intl.DateTimeFormat().resolvedOptions().timeZone` when `schedule.tz` was missing/blank via `resolveCronTimezone()`. The new code passes `timezone: schedule.tz?.trim() || undefined`, which makes croner fall back to *its* default timezone (often UTC). For cron schedules stored without an explicit `tz`, this changes behavior and will shift next run times. If the intent is “use local timezone when tz is omitted”, this should continue calling `resolveCronTimezone(schedule.tz)` (or an equivalent fallback), not `undefined`.
How can I resolve this? If you propose a fix, please make it concise.| catch: true, | ||
| }); | ||
| let cursor = nowMs; | ||
| for (let attempt = 0; attempt < 3; attempt++) { | ||
| const next = cron.nextRun(new Date(cursor)); | ||
| if (!next) { | ||
| return undefined; | ||
| } | ||
| const nextMs = next.getTime(); | ||
| if (Number.isFinite(nextMs) && nextMs > nowMs) { | ||
| return nextMs; | ||
| } | ||
| cursor += 1_000; | ||
| // Use a tiny lookback to catch the boundary if we are exactly at it. | ||
| // Croner's nextRun(date) skips the current second if it matches exactly. | ||
| const next = cron.nextRun(new Date(nowMs - 1)); |
There was a problem hiding this comment.
Cron parsing errors now swallowed
Changing catch: false to catch: true will cause invalid cron expressions to be silently ignored by croner rather than throwing. That can turn a bad schedule into nextRunAtMs = undefined with no error surfaced, which is a behavioral change from before. If this was added only to avoid crashing on invalid input, consider keeping catch: false and handling exceptions at the call site (so you can log/return a validation error), or at least log when nextRun can’t be computed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cron/schedule.ts
Line: 50:54
Comment:
**Cron parsing errors now swallowed**
Changing `catch: false` to `catch: true` will cause invalid cron expressions to be silently ignored by croner rather than throwing. That can turn a bad schedule into `nextRunAtMs = undefined` with no error surfaced, which is a behavioral change from before. If this was added only to avoid crashing on invalid input, consider keeping `catch: false` and handling exceptions at the call site (so you can log/return a validation error), or at least log when `nextRun` can’t be computed.
How can I resolve this? If you propose a fix, please make it concise.| const nextRun = job.state.nextRunAtMs; | ||
| const isDueOrMissing = nextRun === undefined || now >= nextRun; | ||
| if (isDueOrMissing) { | ||
| const newNext = computeJobNextRunAtMs(job, now); | ||
| if (job.state.nextRunAtMs !== newNext) { | ||
| job.state.nextRunAtMs = newNext; | ||
| changed = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Recompute can leave stale nextRun
recomputeNextRuns() now only updates job.state.nextRunAtMs when nextRunAtMs is missing or already due (now >= nextRunAtMs). If a job’s schedule was changed on disk (or via patch fields other than schedule/enabled) while keeping a future nextRunAtMs, this function will no longer correct it on startup/periodic recompute, and the job can run at the wrong time. If recompute is intended to be authoritative (as the name suggests), it should still recompute for enabled jobs (or at least when schedule-related fields changed), not gate solely on due/missing.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cron/service/jobs.ts
Line: 121:129
Comment:
**Recompute can leave stale nextRun**
`recomputeNextRuns()` now only updates `job.state.nextRunAtMs` when `nextRunAtMs` is missing or already due (`now >= nextRunAtMs`). If a job’s schedule was changed on disk (or via patch fields other than `schedule`/`enabled`) while keeping a future `nextRunAtMs`, this function will no longer correct it on startup/periodic recompute, and the job can run at the wrong time. If recompute is intended to be authoritative (as the name suggests), it should still recompute for enabled jobs (or at least when schedule-related fields changed), not gate solely on due/missing.
How can I resolve this? If you propose a fix, please make it concise.
2 participants
Description
Fixes issue #11246
Changes made
fixed a bug where cron jobs could be scheduled 24 hours late if they were updated or if the service restarted exactly at their scheduled run time.
Greptile Overview
Greptile Summary
This PR adjusts cron next-run computation to avoid a boundary case where jobs scheduled exactly at
nowcould be pushed out (reported as “24 hours late”), including:computeNextRunAtMs()now uses a 1ms lookback when calling Croner’snextRun()to catch schedules that match exactly at the current time.recomputeNextRuns()only recomputesnextRunAtMswhen the job is due or missing.ops.list()replacestoSorted()with asort()on a copied array.ops.update()only recalculatesnextRunAtMswhenschedule/enabledare patched.Main concerns are behavioral changes around timezone defaulting and swallowing cron parse errors, plus
recomputeNextRuns()no longer being authoritative which can leave stalenextRunAtMsvalues intact.Confidence Score: 3/5
computeNextRunAtMs()is plausible, but the patch also changes timezone fallback semantics and starts swallowing cron parsing errors, both of which can silently shift or disable schedules. Additionally,recomputeNextRuns()is no longer authoritative and can preserve stalenextRunAtMsvalues under realistic state-update scenarios.Context used:
dashboard- CLAUDE.md (source)dashboard- AGENTS.md (source)