fix(cron): non-dict schedule crashes 6 direct-call sites the due-scan repair doesn't reach

[pierrenode]

Summary

The recent malformed-job hardening series (#61382 id-less job, #61525 non-dict schedule, #61581 bad next_run_at, and the final per-job containment guard) fixed _get_due_jobs_locked() so a non-dict "schedule" (null, a stray string, etc. from a direct jobs.json edit or an old writer) no longer aborts the periodic due-scan tick. That fix normalizes the schedule to {} and persists the repair via save_jobs() — but only inside the scan.

Six other places in cron/jobs.py read job.get("schedule", {}).get(...) or job["schedule"].get(...) directly, and each can run on a record the scan hasn't repaired yet (a paused job, or any of these called before the scheduler's next tick):

• resume_job()
• mark_job_run() (two call sites)
• claim_dispatch()
• advance_next_run()
• claim_job_for_fire()
• update_job()'s inherited-schedule fallback (when updates doesn't touch "schedule" but the stored value is malformed)

dict.get(key, default) only returns default when the key is absent — a key present with value None still returns None. So job.get("schedule", {}).get("kind") crashes with AttributeError: 'NoneType' object has no attribute 'get' when schedule is explicitly None, instead of treating it as absent.

All 6 were reproduced empirically against the real cron.jobs module (no mocks) before this fix:

resume_job            -> AttributeError: 'NoneType' object has no attribute 'get'
mark_job_run          -> AttributeError: 'NoneType' object has no attribute 'get'
claim_dispatch        -> AttributeError: 'NoneType' object has no attribute 'get'
advance_next_run      -> AttributeError: 'NoneType' object has no attribute 'get'
claim_job_for_fire    -> AttributeError: 'NoneType' object has no attribute 'get'
update_job            -> AttributeError: 'NoneType' object has no attribute 'get'

Fix

Added a shared _job_schedule_dict(job) helper that returns job["schedule"] as a dict, repairing it in place (mirroring the due-scan's own normalization) when it isn't one, and used it at the 5 function-level sites. update_job() gets an equivalent inline guard that's careful to skip strings, since a raw string (e.g. "every 10m") is a valid update payload that's parsed later in the same function — a blanket not isinstance(..., dict) check would have broken that path.

Testing

• Added a regression test class (TestScheduleNoneSiblingCrashes) in tests/cron/test_jobs.py covering resume_job, mark_job_run, claim_dispatch, advance_next_run, and update_job (including a test confirming the raw-string schedule update path still works).
• Added a regression test in tests/cron/test_claim_job_for_fire.py for claim_job_for_fire.
• Mutation-verified: reverting the fix causes all 6 new tests to fail against pre-fix code (confirmed via git stash); restoring the fix makes them pass.
• Full tests/cron/ suite + tests/tools/test_cronjob_run_immediate.py + all other test files importing cron.jobs: 989 tests pass.
• ruff check clean on all changed files.

Checklist

• Tests added/updated and passing
• Mutation-verified (fix reverted -> new tests fail; fix restored -> tests pass)
• ruff check clean
• Legitimate raw-string schedule update path in update_job still works (explicit regression test)

[alt-glitch]

This was generated by AI during triage.

Related to the malformed-cron-job family: the merged class fix #61723 hardened the due-scan (_get_due_jobs_locked), but the six direct-call sites this PR touches (resume_job, mark_job_run, claim_dispatch, advance_next_run, claim_job_for_fire, update_job's inherited-schedule fallback) still crash on an explicit None schedule on main (verified: resume_job does job["schedule"].get("kind"); the others job.get("schedule", {}).get(...), which returns None for a present-None key). This is net-new coverage, not a duplicate of #61723. Also related to closed per-job-quarantine superset #51267.

[teknium1]

Thanks for tracing the direct-call paths; the premise is verified on current remote main (cron/jobs.py:1337-1339, 1435, 1513, 1570, and 1645).

Problems

• _job_schedule_dict() says its in-place repair is persisted by each caller (cron/jobs.py:614-633), but claim_dispatch() returns at cron/jobs.py:1545-1546 and advance_next_run() returns at cron/jobs.py:1602-1605 without save_jobs(). Those calls avoid the crash but leave jobs.json malformed.

Suggested changes

• Track whether _job_schedule_dict() repaired the record and persist before those early returns. Add persistence assertions to the new direct-call tests at tests/cron/test_jobs.py:1827-1833.

Automated hermes-sweeper review.

[teknium1]

_job_schedule_dict() may replace a malformed stored schedule here, but this early return skips save_jobs(jobs). The analogous early return in advance_next_run() has the same issue, so the repair does not persist despite the helper's contract. Track whether normalization occurred, save before these returns, and add persisted-state assertions.

[GottZ]

This was generated by AI during triage.

Summary

Seven PRs address or materially reference the malformed-cron-record failure family. #61723 merged the canonical due-scan class fix for missing IDs, non-dict schedules, and malformed timestamps; #61758 covers still-unprotected direct-call paths, while #40740 retains stricter interval-value validation not fully present on main and #52611 exposes a test-store isolation prerequisite relevant to #61758's added test.

Related pull requests

• #40740 [closed] related — (+126/-5) — partially superseded: It hardens compute_next_run() against non-dict or incomplete schedules and, unlike #61723, rejects non-numeric interval minutes including strings and bools. Despite the implemented_on_main close review, #61723 only rejects None minutes, so #40740's complete validation behavior was not merged and remains relevant as a focused follow-up rather than as a duplicate of #61758.
• #50377 [closed] related — (+109/-1) — superseded by #61723: It isolates an unparseable next_run_at, repairs recurring schedules, and prevents one poisoned record from aborting healthy siblings. #61723 implements this cause more broadly through timestamp normalization plus per-job containment, which supports the contributor's implemented_on_main close verdict.
• #52611 [closed] related — (+13/-2) — independent test-safety fix: It patches cron.jobs' import-time CRON_DIR, JOBS_FILE, and OUTPUT_DIR constants so claim_job_for_fire tests cannot write to the real cron store. Although not a scheduler runtime fix, it remains relevant because #61758 adds another test using that same temp_home fixture without including this isolation change.
• #61525 [closed] related — (+85/-17) — merged through #61723: It normalizes non-dict schedules in the due scan and hardens schedule readers, directly fixing the scan-wide freeze caused by schedule.get() on None or another non-dict value. Its contributor-authored cron changes were cherry-picked into #61723 with attribution preserved.
• #61581 [closed] related — (+206/-14) — cron portion merged through #61723: Its timestamp normalization and compute_next_run() recovery prevent malformed next_run_at or last_run_at values from aborting the due scan. The unrelated process-registry redaction changes were intentionally excluded and require a separate focused PR, as documented by the contributor review.
• #61723 [merged] related — (+501/-194) — merged canonical due-scan class fix: It consolidates missing-ID repair, non-dict schedule normalization, malformed timestamp recovery, and a structural per-job exception boundary so malformed records cannot freeze healthy siblings. It is the reference implementation for the scan path, but it does not cover #61758's direct-call paths and does not fully preserve #40740's numeric interval validation.
• #61758 related — (+144/-11) — merge after test-isolation check: It extends non-dict schedule repair to resume_job(), mark_job_run(), claim_dispatch(), advance_next_run(), claim_job_for_fire(), and update_job(), which #61723's due-scan repair cannot reach. The visible keep_open review identified missing persistence in claim_dispatch() and advance_next_run(); the current diff explicitly saves repaired records on both early-return paths and adds persistence assertions, addressing that review, but its claim_job_for_fire test should use the isolated fixture behavior from #52611.

Duplicates

#50377 and the cron portion of #61581 substantially overlap the malformed-timestamp repair incorporated into #61723; #61525 is likewise incorporated into #61723 for non-dict schedules. #40740 overlaps compute_next_run() hardening only partially, while #61758 and #52611 are complementary rather than duplicates.

Suggested consolidation

Merge #61758 after ensuring its claim_job_for_fire fixture includes #52611's import-time path isolation — it is the focused complement to merged #61723 and its current diff addresses the visible keep_open review's persistence blockers. Treat #50377, #61525, and the cron portion of #61581 as superseded by #61723; keep #52611 as an independent test-safety change, and track #40740's stricter non-numeric interval validation separately because that behavior was not fully implemented by #61723.

Cross-PR triage: Reviewed 7 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 89 kB of PR diffs, 19 kB of issue/PR text, 8 kB of discussion (10 comments), 3 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

[pierrenode]

Rebased onto current upstream/main and squashed to a single commit.

The rebase had two real conflicts worth calling out:

• update_job(): upstream added its own execution-mode-invariant re-check (monitor_script/monitor_url/no_agent/script) in the same region as this PR's schedule-repair guard. Both are kept, sequentially — the invariant check first, then the malformed-schedule guard, then the existing raw-string-schedule parse.
• advance_next_run → advance_next_runs: upstream refactored the per-job advance_next_run(job_id) into a thin wrapper around a new batch function advance_next_runs(job_ids) (one load_jobs()/save_jobs() for the whole due-dispatch batch instead of one pair per job, fix(cron): only advance next_run_at for jobs that are actually dispatched #60946-adjacent perf work). This PR's fix was relocated into the new consolidated advance_next_runs() — both the _job_schedule_dict() repair-in-place and the "persist even if nothing in the batch advances" follow-up fix (originally commit 26f8fed3f3) now live there, since the old single-job function is gone.

All 522 tests in tests/cron/ pass (1 pre-existing skip, unrelated warnings). Mutation-verified the persist-on-early-return fix specifically: temporarily reverting the schedule_repaired persist check reproduces the original bug (test_advance_next_run_does_not_crash fails with assert None == {} — the in-place repair is silently lost instead of being written back). Fresh competitor search turned up no PR touching the same six call sites.