fix(cron): harden runtime ownership, supervision, and recovery - #75833
fix(cron): harden runtime ownership, supervision, and recovery#75833cirwel wants to merge 12 commits into
Conversation
ff0c2c7 to
4d38aa3
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the thorough ownership and recovery work. The current-main premise is real: cron/scheduler.py:4011 delivers before mark_job_run() at line 4030, and tick() only pre-advances recurring schedules at cron/scheduler.py:4154-4160.
Problems
cron/jobs.py:1751-1804hides everyruntime_tombstonefromget_job,resolve_job_ref, andlist_jobs. Buttools/cronjob_tools.py:770-795resolves all management actions beforeremoveorupdate. A repeat-exhausted declaration retained for reproducibility therefore cannot be listed, removed, edited, or revived through supported surfaces. This contradicts the nearby recovery intent in_reconcile_runtime_state().- The persistence migration has no documentation update.
website/docs/developer-guide/cron-internals.md:36-64andwebsite/docs/user-guide/features/cron.md:249-259,780-788still describe mutable lifecycle state as living solely injobs.json.
Suggested changes
- Add an explicit terminal/declaration management path and end-to-end tool/CLI regressions for list, remove, and revive/edit of a completed job.
- Document the definition/runtime split, migration, and paired-backup behavior.
Automated hermes-sweeper review.
The scheduler's pre-dispatch loop called advance_next_run per due job — one full load_jobs() + one full save_jobs() of the jobs file each — so N due jobs cost N reads + N writes of the whole file (gateway-restart catch-up or co-scheduled bursts). advance_next_runs() does one load + at most one save for the whole due set with identical per-job semantics; advance_next_run() is now a thin wrapper over it. Measured (50 due recurring jobs, real jobs file): 107.9 ms -> 2.5 ms (45x; 50 loads + 50 saves -> 1 + 1). Tests: batch advances recurring and skips one-shots, single load + save I/O pin (fails pre-fix — no such function), no save when nothing advances, and per-job wrapper semantics unchanged. Related: NousResearch#60946 and NousResearch#75833 both restructure this loop's call site for correctness — neither addresses the I/O cost, and this batch primitive composes with either dispatch design; happy to rebase onto whichever lands first.
4d38aa3 to
7034a85
Compare
|
Both review points addressed in 7034a85 (rebased onto current main): Terminal/declaration management path.
One deliberate semantics change while closing this: End-to-end regressions: 8 new tests (tool-layer list/remove-by-id/remove-by-name/revive-by-repeat/revive-by-schedule/prompt-only-stays-completed/live-action-terminal-errors, plus a CLI Docs. Focused suites after the rebase: 159 passed across |
7034a85 to
f7dce23
Compare
|
Follow-up hardening in the two commits now on the branch (rebased onto current main again): after the earlier fix I ran an adversarial multi-lens review over the terminal-management delta (every finding reproduced by execution before fixing) and closed seven coherence gaps it surfaced:
12 new regressions across the tool, CLI, and jobs layers (including the gateway-PATCH-only Known limitation, deliberately out of scope: the web dashboard's job list/detail endpoints still use live-only lookup, so completed declarations aren't visible there (its |
The scheduler's pre-dispatch loop called advance_next_run per due job — one full load_jobs() + one full save_jobs() of the jobs file each — so N due jobs cost N reads + N writes of the whole file (gateway-restart catch-up or co-scheduled bursts). advance_next_runs() does one load + at most one save for the whole due set with identical per-job semantics; advance_next_run() is now a thin wrapper over it. Measured (50 due recurring jobs, real jobs file): 107.9 ms -> 2.5 ms (45x; 50 loads + 50 saves -> 1 + 1). Tests: batch advances recurring and skips one-shots, single load + save I/O pin (fails pre-fix — no such function), no save when nothing advances, and per-job wrapper semantics unchanged. Related: #60946 and #75833 both restructure this loop's call site for correctness — neither addresses the I/O cost, and this batch primitive composes with either dispatch design; happy to rebase onto whichever lands first.
SummaryOne PR addresses #75607. #75833 separates declarative cron definitions in Related pull requests
Suggested consolidationKeep #75833 open with a salvage path focused on the definition/runtime separation, lossless migration and reconciliation, token-fenced ownership, coherent backup/restore, terminal-declaration management, and their regression tests. The diff is broad, so the author should keep these end-to-end storage and safety invariants coherent while narrowing unrelated changes where possible; there are no duplicate PRs to close. Complex graphflowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I75607(["issue #75607 (open)"])
P75833["PR #75833 (open)"]
P75833 -->|best fix| I75607
class I75607 open
class P75833 open
class P75833 best
class P75833 target
click I75607 "https://github.com/NousResearch/hermes-agent/issues/75607"
click P75833 "https://github.com/NousResearch/hermes-agent/pull/75833"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 1 pull request and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 305 kB of PR diffs, 9 kB of issue/PR text, 8 kB of discussion (5 comments), 2 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
4425f21 to
4c67f38
Compare
|
Dogfood follow-up on
The quick-restore pass found one real WAL lifecycle defect in this branch: Python's
Post-fix evidence:
GitHub still reports no hosted checks on the branch. |
Separate declarative jobs from volatile runtime state, fence every fire with durable ownership tokens and execution ledgers, and suppress stale delivery. Isolate cron workers with inactivity heartbeats and complete process-tree cleanup, while making cron backup and restore generation-coherent and fail-closed.
…ore split A repeat-exhausted declaration is retained as a runtime tombstone, but every management surface resolved jobs through live-only lookup, so a completed job could not be listed, removed, edited, or revived through supported tools. Give get_job/resolve_job_ref/list_jobs an include_terminal opt-in and route management surfaces through it: completed jobs list on request (tool include_completed, cron list --all), remove works on them, and a cadence edit (schedule/repeat/enabled) revives them — resetting the exhausted counter and rescheduling from now rather than firing on the stale pre-completion occurrence. Non-cadence edits update the declaration but keep the job completed, and reconciliation no longer revives tombstones on such edits (that path could fire a revived job once past its repeat limit). Live-only actions (pause/resume/run) fail with an explicit terminal error instead of 'not found'. Document the jobs.json definition / runtime.db state split, migration, and paired-backup coherence in cron-internals and the user guide.
Adversarial review of the terminal-management path surfaced seven coherence gaps; each was reproduced by execution before fixing: - Resolution is now live-first with a terminal fallback everywhere (tool actions, remove_job, cron edit CLI): a live job always wins a name tie with a retained completed namesake instead of the reference turning ambiguous, while completed declarations stay reachable when nothing shadows them. Ambiguity payloads carry state so completed twins are distinguishable. - 'hermes cron edit' and the '/cron edit' console command can reach completed jobs — the documented revive path works end-to-end. - context_from accepts completed upstream jobs on the tool create/update paths and the dashboard validator alike: fire-time injection reads the persisted output directory, so a finished one-shot collector is a natural chaining source. - A run that exhausts the repeat limit reports the real completed record (state, completed_reason/completed_at) instead of a fabricated schedulable shape, and execution_success no longer misreports on a final run. - Repeat-only revive of a completed one-shot says a new schedule is required instead of the generic 'time is in the past' error. - A completed declaration is listed on the completed opt-in alone — the disabled filter applies to live jobs only. - Tombstone reads are isinstance-guarded against corrupted stores; the internals doc scopes digest binding to the cadence digest (the stored full-definition digest has no comparer), and the architecture and troubleshooting docs now describe the split store. 12 new regressions cover the above across the tool, CLI, and jobs layers, including the gateway-PATCH-only enabled revive leg.
…ombstone _sweep_completed_oneshots() and its regression tests predate this branch's runtime_tombstone mechanism (introduced in "harden runtime ownership and recovery") — they only recognized the legacy bare-one-shot completion shape (state == "completed" written directly to storage). A repeat-exhausted one-shot retired via mark_job_run/claim_dispatch now carries a runtime_tombstone instead, so the sweep never matched it and such records would accumulate in jobs.json forever, uncounted by COMPLETED_ONESHOT_RETENTION_DAYS. - _sweep_completed_oneshots: treat a runtime_tombstone as terminal too, not just state == "completed". Age falls back to the tombstone's own "at" timestamp when last_run_at was never written (a wedged dispatch_limit / stale_dispatch_limit tombstone never reaches mark_job_run, so last_run_at stays null forever otherwise). - _normalize_job_record: a tombstoned job now also reads back enabled=False and next_run_at=None, matching the shape the legacy completion path already writes directly to storage, so both mechanisms present identically to callers. - tests/cron/test_jobs.py: the six pre-existing tests exercising this sweep and completion shape now opt into include_terminal=True where they look up a job by id/list that has become a runtime_tombstone (get_job/list_jobs hide those by default as of "make completed declarations manageable").
… publishes _create_quick_snapshot_locked wrote cron/jobs.json + cron/runtime.db straight into snap_dir (the final, published path) while every other quick-snapshot file staged into a hidden .partial sibling that only gets os.replace()'d onto snap_dir at the end (introduced by upstream's "serialize and atomically publish snapshots"). Since _snapshot_cron_pair already created snap_dir with the pair inside it, the later os.replace(staging_dir, snap_dir) hit an existing non-empty directory and failed with ENOTEMPTY on every quick snapshot that has a cron store — caught by tests/hermes_cli/test_backup.py's TestQuickSnapshot suite (15 failures) once the two commits were combined. _snapshot_cron_pair takes snap_dir purely as a base path to join against, so pointing it at staging_dir instead makes the cron pair land in the same directory as everything else and publish atomically with it — preserving both this PR's cron-pair coherence (still captured under the same _quick_cron_store_lock, still all-or-nothing) and upstream's atomicity guarantee for the snapshot as a whole.
3ae8390 to
7621ee9
Compare
|
Final recovery hardening is now on Review follow-up closed the remaining safety findings:
Exact settled patch before commit: SHA-256 Final local evidence after the rebase:
The canonical repository-wide run is still honestly non-green: 18 failures across 12 unrelated files. Replaying those same files on untouched current upstream reproduced all 18 failures. A branch replay also saw one extra load-sensitive transcription timeout; that source is byte-for-byte unchanged from upstream and the extra timeout did not reproduce there. No cron-repair regression was demonstrated, but this is not being represented as a green full suite. |
The scheduler's pre-dispatch loop called advance_next_run per due job — one full load_jobs() + one full save_jobs() of the jobs file each — so N due jobs cost N reads + N writes of the whole file (gateway-restart catch-up or co-scheduled bursts). advance_next_runs() does one load + at most one save for the whole due set with identical per-job semantics; advance_next_run() is now a thin wrapper over it. Measured (50 due recurring jobs, real jobs file): 107.9 ms -> 2.5 ms (45x; 50 loads + 50 saves -> 1 + 1). Tests: batch advances recurring and skips one-shots, single load + save I/O pin (fails pre-fix — no such function), no save when nothing advances, and per-job wrapper semantics unchanged. Related: NousResearch#60946 and NousResearch#75833 both restructure this loop's call site for correctness — neither addresses the I/O cost, and this batch primitive composes with either dispatch design; happy to rebase onto whichever lands first.
fix(cron): fail mixed silence before delivery
feat(cron): expose durable execution identity to jobs
What changed
This PR hardens cron execution as one end-to-end ownership and recovery contract:
cron/jobs.jsondefinitions from mutablecron/runtime.dbstate;taskkill /T /Ffallbacks;Why
Cron's declarative configuration and scheduler-owned runtime fields previously shared
jobs.json. Normal execution dirtied the same artifact operators would back up, review, deploy, or source-control. In parallel, in-process running guards could not prevent two scheduler processes from winning the same fire, and several early-failure, timeout, shutdown, and delivery paths could leave claims, ledgers, or descendant processes behind.The result was an unsafe boundary: stale workers could continue after ownership changed, mutable state could race backup/restore, and process-group-only cleanup missed descendants that detached with
setsid().Compatibility and recovery
jobs.jsonfiles migrate on load; declarations remain in JSON and runtime fields move to SQLite.How to verify
cron/jobs.jsoncontaining schedules, last-run fields, counters, and a claim; load/list jobs and confirm definitions remain in JSON while runtime fields appear incron/runtime.db.start_new_session=Truedescendants; confirm no marker/descendant survives.Test plan
scripts/run_tests.sh -j 4 tests/cron tests/hermes_cli/test_backup.py tests/hermes_cli/test_web_server_cron_profiles.py tests/tools/test_cronjob_run_immediate.py -q— 488 passed, 0 failed across 38 per-file subprocesses.ruff checkover all 23 changed Python files — passed.compileallover implementation and relevant test trees — passed.git diff --check origin/main— passed.Risk
Medium-high. This touches scheduler ownership, persistence, process supervision, and backup semantics. The change is deliberately token-fenced and fail-closed: stale owners cannot renew, finalize, or deliver; backup/restore aborts instead of degrading without the cross-process lock; and terminal execution rows are immutable.
Tested on macOS; Windows behavior is covered with mocked
taskkillsuccess, timeout, and failure/fallback regressions plus the repository footgun checker.Related work and credit
Review follow-up (2026-08-01)
Addressed both hermes-sweeper findings:
include_completedon the tool,hermes cron list --all), removal by id or name, and revive via a cadence edit (schedule/repeat/enabled) that re-arms the counter and reschedules from now. Live-only actions on a completed job return an explicit terminal error. Reconciliation no longer revives tombstones on non-cadence edits (that path could fire a revived job once past its repeat limit). 8 new end-to-end tool/CLI regressions, RED/GREEN verified.cron-internals.mdand the user-guide cron page now document thejobs.jsondefinition /runtime.dbruntime split, combined-store migration, digest binding, tombstone lifecycle, completed-job management, and backup-pair coherence.