Skip to content

fix: skip already-run cron catchup after restart#102004

Closed
NianJiuZst wants to merge 1 commit into
openclaw:mainfrom
NianJiuZst:codex/issue-101988-cron-catchup
Closed

fix: skip already-run cron catchup after restart#102004
NianJiuZst wants to merge 1 commit into
openclaw:mainfrom
NianJiuZst:codex/issue-101988-cron-catchup

Conversation

@NianJiuZst

@NianJiuZst NianJiuZst commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Closes #101988

What Problem This Solves

Fixes an issue where users with successful cron jobs could receive duplicate startup catch-up runs after a Gateway restart when the persisted due slot had already completed.

Why This Change Was Made

Startup catch-up now treats a stale successful cron due slot as already covered only when no later cron occurrence has been missed. The check still replays legitimate missed cron slots, including a slot due exactly at restart time, and leaves failed/skipped retry and backoff behavior unchanged.

User Impact

Users should no longer see duplicate scheduled cron output after restarting the Gateway when the current schedule window already completed successfully. Missed later cron windows still catch up instead of being silently advanced away.

Evidence

  • git diff --check
  • node scripts/run-vitest.mjs src/cron/service.restart-catchup.test.ts (18 tests passed)
  • node scripts/run-vitest.mjs src/cron/service/timer.regression.test.ts (66 tests passed)
  • .agents/skills/autoreview/scripts/autoreview --mode local (clean: no accepted/actionable findings)

AI-assisted by Codex.

@NianJiuZst
NianJiuZst marked this pull request as ready for review July 8, 2026 03:49
@clawsweeper

clawsweeper Bot commented Jul 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 7, 2026, 11:57 PM ET / July 8, 2026, 03:57 UTC.

Summary
The branch changes cron startup runnable selection to skip stale successful due cron slots only when no newer or boundary cron occurrence was missed, and adds restart catch-up regression tests.

PR surface: Source +40, Tests +108. Total +148 across 2 files.

Reproducibility: yes. source-level: current main starts runMissedJobs before maintenance recompute, and isRunnableJob returns true for due nextRunAtMs before comparing the previous cron slot to lastRunAtMs. I did not run a live Gateway restart in this read-only review.

Review metrics: none identified.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #101988
Summary: This PR is a candidate fix for the linked stale successful cron restart catch-up issue, with overlapping open PRs that should be consolidated before landing.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🐚 platinum hermit
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Add real behavior proof in the PR body, such as redacted terminal/live output from production CronService startup against a temporary cron store showing no duplicate run and boundary missed-slot replay.
  • Resolve or explain the failing exact-head CI check once logs are available.

Proof guidance:

  • [P1] Needs real behavior proof before merge: Mock-only: the PR body lists git diff check, focused Vitest runs, and autoreview, but needs redacted terminal/live output from a real CronService or Gateway startup scenario; updating the PR body should trigger re-review, or a maintainer can comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] This PR changes which startup cron slots are replayed or skipped after restart, so a wrong boundary can suppress a legitimate scheduled message or duplicate one.
  • [P1] Contributor proof is mock-only: focused tests are useful, but they do not show the changed behavior in a real CronService or Gateway-style startup run.
  • [P1] Exact-head CI was not green when inspected; one compact Node check was failing and some checks were still pending, with logs unavailable while the workflow was still in progress.
  • [P1] There are multiple open candidate PRs for the same linked issue, so whichever branch lands should be refreshed against current main and the other candidates should be closed or retargeted.

Maintainer options:

  1. Prove and land the boundary-preserving fix (recommended)
    Require real CronService or Gateway startup proof plus clean exact-head CI, then land the single candidate that covers stale-ok skip, later missed slots, and exact-boundary replay.
  2. Land the proof-positive simpler PR and follow up
    Maintainers may choose fix(cron): skip completed restart catchup slots #101998 for its stronger proof, but should port this branch's boundary coverage or keep a follow-up open.
  3. Pause duplicate candidates
    If the scheduler direction is uncertain, pause or close competing branches until one canonical fix is selected for the linked issue.

Next step before merge

  • [P1] Human review is needed because the contributor must add real behavior proof and CI is not green; there is no narrow automated repair target from the diff.

Security
Cleared: The diff only changes cron scheduler selection logic and focused tests; I found no concrete security or supply-chain concern.

Review details

Best possible solution:

Land one narrow cron scheduler fix that preserves failed/skipped retry behavior, stale successful-slot dedupe, later missed-slot replay, and exact-boundary replay, after real behavior proof and exact-head CI are clean.

Do we have a high-confidence way to reproduce the issue?

Yes, source-level: current main starts runMissedJobs before maintenance recompute, and isRunnableJob returns true for due nextRunAtMs before comparing the previous cron slot to lastRunAtMs. I did not run a live Gateway restart in this read-only review.

Is this the best way to solve the issue?

Yes, with proof still missing: this branch fixes the runnable gate while preserving later and exact-boundary missed-slot replay better than simpler duplicate guards. The best merge path still needs real behavior proof and clean exact-head CI.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 757d7a0de321.

Label changes

Label justifications:

  • P1: The PR targets a latest-release Gateway cron restart bug that can duplicate scheduled user notifications and affect real channel workflows.
  • merge-risk: 🚨 message-delivery: Merging this scheduler gate changes whether startup catch-up sends, skips, or delays scheduled cron messages after restart.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: Mock-only: the PR body lists git diff check, focused Vitest runs, and autoreview, but needs redacted terminal/live output from a real CronService or Gateway startup scenario; updating the PR body should trigger re-review, or a maintainer can comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +40, Tests +108. Total +148 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 40 0 +40
Tests 1 108 0 +108
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 148 0 +148

What I checked:

Likely related people:

  • vincentkoc: Current blame for the localized cron scheduler files points to Vincent Koc's refactor, and recent adjacent cron runtime commits touch delivery and timeout semantics. (role: recent area contributor; confidence: high; commits: e242c6140014, 7ff3f2dbf922, 44ae2fd93601; files: src/cron/service/timer.ts, src/cron/service/jobs.ts, src/cron/service/ops.ts)
  • Takhoffman: Tak Hoffman authored the restart catch-up replay semantics commit that established the relevant missed-run guard behavior. (role: feature-history contributor; confidence: high; commits: 79d00ae39860, cc5dad81bc70; files: src/cron/service/timer.ts, src/cron/service/jobs.ts, src/cron/service.restart-catchup.test.ts)
  • steipete: Peter Steinberger has the highest shortlog count on the central cron service files and recent commits in declarative jobs, event triggers, and restart-state hardening. (role: recent cron scheduler contributor; confidence: medium; commits: a6768d9de567, fdc9aa82d785, d2f1c0eac80d; files: src/cron/service/timer.ts, src/cron/service/jobs.ts, src/cron/service.restart-catchup.test.ts)
  • RichChen01: RichChen01 authored recent startup catch-up deferral persistence work in the same cron state path that interacts with deferred due slots after restart. (role: adjacent startup catch-up contributor; confidence: medium; commits: 808e8f09bab3; files: src/cron/service/timer.ts, src/cron/service/jobs.ts, src/cron/service/state.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P1 High-priority user-facing bug, regression, or broken workflow. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. labels Jul 8, 2026
@harjothkhara harjothkhara mentioned this pull request Jul 8, 2026
Merged
@clawsweeper clawsweeper Bot mentioned this pull request Jul 8, 2026
Closed
1 task
@NianJiuZst NianJiuZst closed this Jul 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. P1 High-priority user-facing bug, regression, or broken workflow. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Feature] Skip catchup rerun for cron jobs with lastRunStatus=ok within current schedule window

1 participant

@NianJiuZst