Skip to content

cron: fix #10934 - preserve future nextRunAtMs in recomputeNextRuns#10988

Closed
Saman-dev12 wants to merge 1 commit intoopenclaw:mainfrom
Saman-dev12:fix/issue-10934-cron-every-schedule
Closed

cron: fix #10934 - preserve future nextRunAtMs in recomputeNextRuns#10988
Saman-dev12 wants to merge 1 commit intoopenclaw:mainfrom
Saman-dev12:fix/issue-10934-cron-every-schedule

Conversation

@Saman-dev12
Copy link

@Saman-dev12 Saman-dev12 commented Feb 7, 2026 •
edited by greptile-apps bot
Loading

Fixes #10934

Problem

Cron jobs with schedule.kind: "every" never executed automatically because recomputeNextRuns() unconditionally recalculated all nextRunAtMs values before checking for due jobs. For "every" schedules without anchorMs, this used the current time as anchor, pushing all scheduled times forward before runDueJobs() could execute them.

Solution

Modified recomputeNextRuns() in src/cron/service/jobs.ts to preserve existing nextRunAtMs values when they are valid and in the future. Only recalculate when:

  • Job has no nextRunAtMs yet
  • nextRunAtMs is in the past (job is due)
  • Job state is invalid

Changes

  • src/cron/service/jobs.ts: Added check to preserve valid future nextRunAtMs in recomputeNextRuns()
  • src/cron/service.issue-regressions.test.ts: Added regression test that verifies jobs execute on schedule without time drift

Testing

Greptile Overview

Greptile Summary

  • Updates recomputeNextRuns() to keep an already-scheduled future nextRunAtMs instead of always recomputing it, addressing “every” schedules without an explicit anchor being pushed forward.
  • Adds a regression test for issue [Bug]: Cron jobs with 'every' schedule never execute - recomputeNextRuns() pushes times forward #10934 to ensure nextRunAtMs isn’t continuously rescheduled and the job actually executes when due.
  • Change interacts with the scheduler loop in src/cron/service/timer.ts where recomputation happens when no jobs are due and after executions to persist updated state.

Confidence Score: 3/5

  • Mostly safe, but there are a couple edge cases/tests that should be tightened before merge.
  • Core behavioral change is small and aligns with scheduler flow, but the guard should only accept valid finite timestamps, and the added regression test’s use of Date.now() deltas under fake timers risks flakiness.
  • src/cron/service/jobs.ts, src/cron/service.issue-regressions.test.ts

(2/5) Greptile learns from your feedback when you react with thumbs up/down!

Copilot AI review requested due to automatic review settings February 7, 2026 08:07
greptile-apps[bot]
greptile-apps bot reviewed Feb 7, 2026
View reviewed changes
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

2 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 7, 2026

Additional Comments (1)

src/cron/service/jobs.ts
Preserve-only check skips invalid
recomputeNextRuns() now continues whenever nextRunAtMs is a number in the future, but it doesn’t validate that value is finite/non-negative. If the store ever contains nextRunAtMs: NaN/Infinity/negative (e.g., from a bad migration/manual edit), this will either (a) fall through and compute newNext as undefined/wrong or (b) preserve a nonsensical far-future value and the job will never run. Consider tightening the guard to Number.isFinite(existingNext) && existingNext > now (and maybe existingNext >= 0) so only valid timestamps are preserved.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/cron/service/jobs.ts
Line: 110:112

Comment:
**Preserve-only check skips invalid**
`recomputeNextRuns()` now `continue`s whenever `nextRunAtMs` is a number in the future, but it doesn’t validate that value is finite/non-negative. If the store ever contains `nextRunAtMs: NaN`/`Infinity`/negative (e.g., from a bad migration/manual edit), this will either (a) fall through and compute `newNext` as `undefined`/wrong or (b) preserve a nonsensical far-future value and the job will never run. Consider tightening the guard to `Number.isFinite(existingNext) && existingNext > now` (and maybe `existingNext >= 0`) so only valid timestamps are preserved.

How can I resolve this? If you propose a fix, please make it concise.

@Saman-dev12 Saman-dev12 mentioned this pull request Feb 7, 2026
Closed
Copilot AI reviewed Feb 7, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adjusts cron scheduling so recomputeNextRuns() does not overwrite an already-valid nextRunAtMs that is still in the future, addressing the reported “every schedules never execute” behavior in #10934.

Changes:

  • Preserve existing future nextRunAtMs values in recomputeNextRuns() rather than always recomputing.
  • Add a regression test intended to verify that “every” schedules don’t have nextRunAtMs pushed forward before becoming due.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
src/cron/service/jobs.ts Adds an early-continue guard to keep future nextRunAtMs values intact during recomputation.
src/cron/service.issue-regressions.test.ts Adds a regression test for #10934 around nextRunAtMs stability and execution timing.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Saman-dev12
cron: fix openclaw#10934 - preserve future nextRunAtMs in recomputeNe…
2b6ab94 
…xtRuns

Cron jobs with 'every' schedules never executed because recomputeNextRuns()
unconditionally recalculated all nextRunAtMs values before checking for due
jobs. For 'every' schedules without anchorMs, this used current time as anchor,
pushing all scheduled times forward before runDueJobs() could execute them.

Fix: preserve existing nextRunAtMs if valid and in the future; only recalculate
when missing or in the past.

Added regression test to verify jobs execute on schedule and times don't drift.
@Saman-dev12 Saman-dev12 force-pushed the fix/issue-10934-cron-every-schedule branch from 3644cd8 to 2b6ab94 Compare February 7, 2026 17:12
@Saman-dev12 Saman-dev12 requested a review from Copilot February 7, 2026 17:13
Copilot AI reviewed Feb 7, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@tyler6204 tyler6204 mentioned this pull request Feb 8, 2026
Merged
@tyler6204
Copy link
Member

tyler6204 commented Feb 8, 2026

Superseded by #11641 (merge commit: 8fae55e). Closing to reduce duplicate PR noise. Please open a new PR only if there is additional scope beyond this fix.

@tyler6204 tyler6204 closed this Feb 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

+1 more reviewer

@greptile-apps greptile-apps[bot] greptile-apps[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: Cron jobs with 'every' schedule never execute - recomputeNextRuns() pushes times forward

2 participants

@Saman-dev12 @tyler6204

Comments