Skip to content

fix(cron): isolate per-job schedule errors so one bad job can't stall… - #51267

Closed
Dharshan2004 wants to merge 1 commit into
NousResearch:mainfrom
Dharshan2004:fix/cron-due-scan-poisoning-51021
Closed

fix(cron): isolate per-job schedule errors so one bad job can't stall…#51267
Dharshan2004 wants to merge 1 commit into
NousResearch:mainfrom
Dharshan2004:fix/cron-due-scan-poisoning-51021

Conversation

@Dharshan2004

Copy link
Copy Markdown

What does this PR do?

get_due_jobs() scans every cron job on each tick. A single job with corrupt
schedule metadata — a bad cron expression, an unparseable next_run_at, or a
field missing from a hand-edited jobs.json — raised an exception that escaped
the scan (the tick's only try/finally just releases the file lock) and
aborted it for every job. The ticker keeps writing its liveness heartbeat
regardless, so hermes cron status reports healthy while no job ever fires
the "ticker alive, nothing fires" symptom in #51021. The poisoned record reloads
every tick, so it permanently wedges all cron jobs until the file is repaired.

Each job is now evaluated inside a single try/except, so any malformed field
degrades that one job instead of the whole tick. A recoverable timestamp
self-heals; an unrecoverable one is quarantined and surfaced.

Relationship to #50377. This extends the open PR #50377, which guards only
the single datetime.fromisoformat(next_run) parse and explicitly scopes
itself to that site. This PR keeps that behavior — a recurring job with a
malformed next_run_at still self-heals by recomputing from its schedule —
and additionally:

Happy to have this supersede #50377 or to rebase it as a delta — maintainer's
call.

Related Issue

Fixes #51021

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • cron/jobs.py: extracted per-job evaluation into _evaluate_due_job() and
    wrapped the entire per-job body in one try/except in
    _get_due_jobs_locked(), covering every throw site — including the
    compute_next_run() calls in the stale-recurring and TZ-migration paths. A
    failing job is quarantined (state="error", last_error recorded) and the
    scan continues so other due jobs still fire.
  • cron/jobs.py: a malformed-but-recoverable next_run_at self-heals — a
    recurring job recomputes its next fire from the schedule instead of being
    quarantined (preserves fix(cron): isolate a malformed next_run_at so one job can't wedge the tick #50377); it only quarantines when recovery is
    impossible or the schedule itself is corrupt.
  • cron/jobs.py: _record_due_scan_error() is idempotent — it logs (with
    exc_info) and rewrites jobs.json only on the transition into the error
    state, so a permanently-corrupt record no longer re-logs or rewrites storage
    on every 60s tick. The job stays enabled and self-heals once repaired
    (mark_job_run resets state to scheduled on the next success).
  • hermes_cli/cron.py: cron status now warns when the ticker is heartbeating
    but has never recorded a successful tick, instead of falsely reporting healthy.
  • tests/cron/test_jobs.py, tests/cron/test_scheduler_provider.py: regression
    tests for the above.

How to Test

  1. scripts/run_tests.sh tests/cron/ — 526 pass, including the new regression
    tests:
    • test_malformed_recurring_schedule_does_not_block_other_due_jobs
    • test_malformed_active_job_does_not_block_other_due_jobs
    • test_malformed_next_run_at_recurring_job_self_heals
    • test_malformed_next_run_at_with_corrupt_schedule_is_quarantined
    • test_corrupt_job_quarantine_is_idempotent
    • test_cron_status_reports_missing_success_marker
  2. Manual:
    export HERMES_HOME=$(mktemp -d)
    # hand-edit $HERMES_HOME/cron/jobs.json to add a job with
    #   "schedule": {"kind": "cron", "expr": "not a cron expr"}
    # alongside a valid due job, then:
    hermes cron list      # corrupt job shows state=error; valid job intact
    
    The due-scan returns only the valid job (no crash); the corrupt job is
    quarantined.

Checklist

Code

Documentation & Housekeeping

  • Documentation — N/A (no user-facing docs; inline docstrings updated)
  • cli-config.yaml.example — N/A (no config keys added/changed)
  • CONTRIBUTING.md / AGENTS.md — N/A (no architecture/workflow change)
  • Cross-platform impact — considered; pure Python control-flow in the
    due-scan, no OS-specific code, file I/O, or process handling added
  • Tool descriptions/schemas — N/A (no tool behavior change)

Screenshots / Logs

Same store in both runs: one corrupt stale recurring job ({"kind":"cron","expr":"not a cron expr"}) and one valid due one-shot.

Before — upstream/main (no fix): one bad record aborts the whole scan, so the healthy job never fires (#51021).

get_due_jobs() RAISED: CroniterBadCronError: Exactly 5, 6 or 7 columns has to be specified for iterator expression.
RESULT: whole scan aborted — valid-due NEVER fires. ❌  (#51021)

After — this PR: the bad job is quarantined and the healthy job fires.

get_due_jobs() -> ['valid-due']
corrupt-cron state -> error
RESULT: valid-due FIRES; corrupt job isolated. ✅

(hermes cron list then shows corrupt-cron [active] … error: Invalid cron schedule metadata: … while valid-due stays intact.)

… the scheduler (NousResearch#51021)

A single job with corrupt schedule metadata (bad cron expr, unparseable
next_run_at, or a field missing from a hand-edited jobs.json) raised out of
get_due_jobs() and aborted the entire due-scan on every tick. The ticker kept
heartbeating, so `hermes cron status` still reported healthy, but NO job ever
fired — the "ticker alive, nothing fires" shape of NousResearch#51021.

- _get_due_jobs_locked now evaluates each job inside a single try/except
  (extracted as _evaluate_due_job), covering every throw site — including the
  stale-recurring and TZ-migration compute_next_run() calls. A bad job is
  quarantined (state=error) and the scan continues, so other due jobs fire.
- Malformed-but-recoverable next_run_at self-heals: a recurring job recomputes
  its next fire from the schedule instead of being quarantined (extends NousResearch#50377,
  which guards only this one parse; this also covers the case where recovery
  itself raises on a corrupt schedule).
- _record_due_scan_error is idempotent: it logs (with exc_info) + rewrites
  jobs.json only on the transition into error, so a permanently-corrupt record
  no longer re-logs or rewrites storage every 60s tick. The job stays enabled
  and is re-evaluated each scan, so repairing the schedule lets it fire again
  (mark_job_run resets state to scheduled on the next success).
- `cron status` now warns when the ticker is heartbeating but has never
  recorded a successful tick, instead of falsely reporting healthy.

Tests: malformed one-shot/recurring jobs no longer block other due jobs; a
recoverable next_run_at self-heals while corrupt-schedule cases quarantine;
quarantine is idempotent; status reports the missing-success case.
@alt-glitch alt-glitch added type/bug Something isn't working comp/cron Cron scheduler and job management comp/cli CLI entry point, hermes_cli/, setup wizard P1 High — major feature broken, no workaround sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation labels Jun 23, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related/competing: extends the open PR #50377, which guards only the single datetime.fromisoformat(next_run) parse. This PR additionally guards the two compute_next_run() calls (stale-recurring + TZ-migration paths), covers recovery-itself-raising, makes the quarantine idempotent (no per-tick log/jobs.json churn), and adds an honest cron status. Maintainer's call whether to supersede #50377 or rebase this as a delta. Both target the same #51021 "ticker alive, nothing fires" silent wedge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard comp/cron Cron scheduler and job management P1 High — major feature broken, no workaround sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Cron Jobs Never Fire Despite Ticker Being Alive

3 participants