fix(cron): never stale-remove a one-shot whose run is still alive (#62002) - #62014
Merged
teknium1 merged 1 commit intoJul 10, 2026
Merged
Conversation
get_due_jobs()'s one-shot stale-entry recovery (NousResearch#38758) treated an expired run_claim (NousResearch#59229) as proof the claiming tick died, but a run stalled on network I/O — or a laptop asleep mid-run — legitimately outlives the TTL while very much alive. The recovery then deleted the job record mid-flight: list showed the job gone, and when the run finished mark_job_run() found nothing to update, so last_run_at / last_status / last_delivery_error were never recorded. Two guards, per the liveness signals available: - Same process (the common single-gateway case): before removing a dispatch-limit-reached one-shot, consult the scheduler's running set via a lazy import; if the job is still running here it is slow, not stale — keep the entry. - Cross process: run_job's monitor loop now refreshes run_claim.at every 60s while the run is alive (including under HERMES_CRON_TIMEOUT=0, which previously blocked without polling), so an expired claim really does mean the owner died and the TTL stays a dead-owner detector. Fixes NousResearch#62002
Contributor
|
Maintainer review note: #62013 by @liuhao1024 was the earliest focused submission of the same-process liveness guard. #62014 is the selected implementation because it also heartbeats the claim for cross-process schedulers; both contributors are credited for resolving #62002. |
13 tasks
22 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Stops
get_due_jobs()'s one-shot stale-entry recovery (#38758) from deleting a job record while its run is still alive.The recovery treats
completed >= timesplus an expiredrun_claim(#59229) as proof the claiming tick died. But the TTL is only an age heuristic: a run stalled on network I/O — or a laptop that slept mid-run — legitimately outlives it while very much alive. In the incident in the issue, a one-shot's provider stream dead-stalled for ~40 minutes; the job record was deleted mid-flight,cronjob(action='list')showed the job gone, and when the run finally completed (and delivered successfully),mark_job_run()found nothing to update —last_run_at/last_status/last_delivery_errorwere lost. Had delivery failed, there would have been zero durable trace.The fix adds the two liveness signals proposed in the issue:
get_due_jobsconsults the scheduler'sget_running_job_ids()(lazily imported — the scheduler already importscron.jobs, so a module-level import would be circular). If the job is still running in this process it is slow, not stale: keep the entry and skip it this tick. Once the run is actually gone, recovery removes the entry exactly as before.run_job's monitor loop now refreshesrun_claim.atevery 60s while the run is alive, via a newheartbeat_run_claim()incron/jobs.py(called from the existing 5s poll; theHERMES_CRON_TIMEOUT=0unlimited branch, which previously blocked without polling, now polls for one-shots too). An expired claim therefore really does mean "the owner died", and the TTL keeps its short dead-owner-detector semantics.mark_job_run()still clears the claim on completion; for a run that never stamped a claim (e.g. a manualrun) the heartbeat is a no-op. No new env vars or config keys; no behavior change for recurring jobs or for genuinely dead ticks.Related Issue
Fixes #62002
Type of Change
Changes Made
cron/jobs.py: new_job_running_in_this_process()helper (lazy scheduler import) and a liveness guard in front of the "one-shot dispatch limit reached — removing stale due entry" removal inget_due_jobs(); newheartbeat_run_claim(job_id)that refreshesrun_claim.atunder the jobs lock and no-ops when the job or claim is gone.cron/scheduler.py:run_job()heartbeats the one-shot's run claim every 60s from the inactivity-monitor poll loop, and the unlimited-timeout (HERMES_CRON_TIMEOUT=0) branch now polls for one-shots instead of blocking so the heartbeat still runs; importstimeandheartbeat_run_claim.tests/cron/test_jobs.py: three new tests — the incident's store shape (completed 1/1, claim older than TTL) survives while the running set holds the job and is removed once it doesn't; a heartbeat-refreshed claim keeps a long run claimed past the original TTL horizon somark_job_runlands on a live record; heartbeat is a safe no-op without a claim.How to Test
scripts/run_tests.sh tests/cron/test_jobs.py tests/cron/test_scheduler.py→ all pass (128 + 155 tests, 0 failed).tests/cron/test_jobs.py::TestGetDueJobs::test_stale_maxed_oneshot_kept_while_running_in_this_processreproduces the incident (repeat 1/1,run_claimolder than the TTL) and asserts the record is kept while the run is alive in-process — and cleaned up as before once it isn't.scripts/run_tests.sh tests/cron/→ 673 passed, 0 failed (macOS).Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass (28 pre-existing failures on plainupstream/mainin my environment are unchanged by this PR; every cron test passes)Documentation & Housekeeping
docs/, docstrings) — or N/A (docstrings/comments in the changed functions)cli-config.yaml.exampleif I added/changed config keys — or N/A (no config changes)CONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/ACredits
The fix follows the remediation proposed by @jeff-mettel in #62002 — the same-process
get_running_job_ids()check and the run-claim heartbeat are their design; this PR implements it (with tests). Thanks for the detailed incident analysis and log forensics.Infographic