Repository navigation
fix(cron-runner): a crons.json entry marked disabled must not fire - #6311
liususan091219 wants to merge 1 commit into
Conversation
b08a3fe to
14bcc21
Compare
sonichi-dot
left a comment
There was a problem hiding this comment.
Reviewed 14bcc21bd93753866c168fa2aa16266d0c0aabad (all four changed files and both dispatch paths).
Recommendation: hold for the parking/liveness mismatch below. The launchd fix works, but the newly parked Codex main loop is still diagnosed as broken by health-check.
Validation: eight focused suites passed, including all 48 cron-runner tests, all 20 codex-scheduler tests, and the 15 liveness tests. The two new regression tests produced four failed assertions against the parent runner and passed at this head. review-checks.sh passed. CI is still running; no live service restart or production round trip was performed.
Operational impact: previously disabled-but-firing jobs will stop, intentionally. Aligning the health policy is needed so deliberately stopping the main loop does not generate false recovery alerts.
| ) | ||
| if raw_entry.get("execution") != "codex-task" and not implicit_main_loop: | ||
| continue | ||
| if _entry_marked_parked(raw_entry): |
There was a problem hiding this comment.
[P2] Make liveness recognize the same parking rule. This now excludes a canonical main loop with disabled: true and its ordinary */5 * * * * schedule, but cron_loop_policy.core_loop_is_parked() still requires an impossible cron date. Consequently health-check.check_cron_schedule() expects completions from a job this dispatcher will never run: it warns after 7 hours and fails after 26, feeding a false issue into emit_task_for_failures().
Reproduced at this head with {name: "main-loop", prompt_skill: "proactive-loop", cron: "*/5 * * * *", disabled: true}: load_jobs(..., include_main_loop=True) returns [], while a completion 27 hours old makes check_cron_schedule() return fail. Changing only the cron to 0 0 31 2 * returns ok / core loop deliberately parked. Please update the shared parking/liveness contract and its regression test to accept the new field-only parking behavior, while keeping an unmarked impossible schedule unhealthy.
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 93%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
`disabled: true` was read by health-check alone. `cron_loop_policy. _entry_marked_parked` has owned the predicate since #2498, but neither durable dispatcher consulted it, so a parked entry kept emitting tasks: on this host repo-sync-and-restart and morning-owner-actions both fired while marked disabled, and repo-sync-and-restart runs src/restart.sh, which bounces every background service and pkills the task watcher. Point both dispatchers at the existing predicate rather than adding a second copy of the rule: src/cron-runner.py (launchd) and skills/schedule-crons/scripts/codex-scheduler.py (codex-task). The launchd skip advances the entry's state boundary, matching the malformed-shell_command skip beside it, so re-enabling resumes at the next slot instead of replaying one missed while parked. Also documents the field in skills/schedule-crons/SKILL.md, which never described it — part of why both dispatchers were written without it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ByiYdd6A5DPVFwdbmEbeaz
14bcc21 to
d63677f
Compare
|
@cla-assistant check |
sonichi-dot
left a comment
There was a problem hiding this comment.
Re-reviewed d63677f04e09e05fd9c4bdf9d0aff1759f46e2cc, including the full five-file diff. The only change since my previous review is the added Codex regression test; it runs, passes here, and fails against the parent scheduler.
Recommendation: hold for the existing parking/liveness finding, which remains reproducible. The new main-loop fixture uses an impossible date, so it misses the documented case where disabled: true preserves a valid schedule. That case still yields no scheduled job but a failing liveness check after 26 hours. No new finding to duplicate inline.
Validation: cron-runner, codex-scheduler, cron-loop-policy, and health-check-cron-schedule-liveness suites passed; review-checks.sh passed. Current-head CI is still running. No production restart or live round trip performed.
What changed, and why?
crons.jsonsupports"disabled": true, andcron_loop_policy._entry_marked_parkedhas owned that predicate since #2498 — but onlyhealth-check.pyever called it. Neither durable dispatcher did, so a parked entry kept emitting tasks on schedule.grep -c disabled src/cron-runner.pyreturned0before this PR.This points both dispatchers at the existing predicate instead of adding a second copy of the rule:
src/cron-runner.py(launchd runner)skills/schedule-crons/scripts/codex-scheduler.py(codex-task runner)What files / sections should reviewers look at first?
src/cron-runner.py— the new gate inrun(), placed after thelaunchdgate and name/expr validation.skills/schedule-crons/scripts/codex-scheduler.py— same predicate inload_jobs().tests/cron-runner.test.py— two new tests.The one judgement call: the launchd skip advances the entry's state boundary (
state[name] = now_epoch) rather than leaving it untouched. This matches the malformed-shell_commandskip immediately beside it. Leaving the boundary stale means re-enabling an entry replays a slot missed while it was parked; advancing it means re-enabling resumes at the next natural slot.test_disabled_entry_boundary_advances_so_reenable_is_cleanpins that choice.What user behavior or bug does this prove?
On the host this was found on, all four
launchd: trueentries are marked"disabled": true, and two of them fired anyway:repo-sync-and-restartfired 12:47 and again on 2026-11-08 at 12:47Zmorning-owner-actionsfired 15:37repo-sync-and-restartrunssrc/restart.sh, which bounces every background service andpkills the task watcher. So an entry the owner had switched off was restarting their services daily and killing the task watcher, which then has to be re-armed by hand.Note for operators: after this lands, an entry marked
disabledgenuinely stops firing. On a host that has been relying on a parked-but-firing job, that job stops. That is the intended correction, but it is a real behavior change.Feature/documentation consistency
docs/catalog.jsonhas no document canonical for cron parking — the only cron entry isdocs/cron-outstanding-payload.md, canonical for prompt cron coalescing and outstanding payload lifetime, which this does not touch. The canonical home forcrons.jsonentry fields isskills/schedule-crons/SKILL.md, which documentedlaunchd,monitor,execution,owner,artifactandconditionalbut neverdisabled— plausibly part of why both dispatchers were written without it. This PR documents the field there.Before/after evidence
Both new tests were run against the parent commit (
db59f7ac65) with only the test file applied, so they exercise unfixed dispatcher code.BEFORE — parent commit
db59f7ac65:AFTER — this branch:
Manual verify against the real config that misfired — the host's actual
crons.jsonrun through the patched predicate:All four were emitting before; all four are now correctly skipped.
A note on the second test
test_disabled_entry_boundary_advances_so_reenable_is_cleanpassed on the unfixed parent in its first form, because a fired entry advances the same boundary — the assertion could not tell "skipped and advanced" from "fired and advanced". Thecheck(emitted == [], ...)line was added specifically to make it discriminate; it is the first of the four BEFORE failures above.What tests did you run?
Repo scanner:
Touching a live path?
The launchd runner is a live dispatch path, so the relevant round trip is "a parked entry produces no task file". That is exactly what the real-config table and
test_run_skips_entry_marked_disabledshow (task-file count asserted, not just the return value). No service restart was performed to capture this, deliberately — the misfiring entry under test is the one that runssrc/restart.sh, and firing it to prove it fires would have bounced the live host's services.🤖 Generated with Claude Code
https://claude.ai/code/session_01ByiYdd6A5DPVFwdbmEbeaz