fix(cron): reap stale execution claims at the manual-run dispatch point (#86721) - #86899
Merged
Conversation
… run` dispatch Fixes #86721. `hermes cron run <job_id>` (a one-shot CLI invocation) dispatches manual runs via the same background-delegation path as an agent's `cronjob(action='run')` tool call (tools/cronjob_tools.py's _try_dispatch_background_run -> dispatch_async_delegation(role= "cron_run", runner=_runner, ...)). The runner thread lives in the calling process's shared daemon executor. When the one-shot process exits right after printing "Triggered job: ...", the in-flight runner dies mid-execution, leaving its cron/executions.db row permanently stuck at status='claimed' -- every subsequent `hermes cron run` on the same job then reports "Ran now: failed" because of the still-claimed row. cron/executions.py already has the exact self-heal this needs: recover_interrupted_executions() correctly identifies and reclassifies 'claimed'/'running' rows whose owner process has provably exited (_owner_is_live checks PID existence AND matches process start-time, so a reused PID isn't mistaken for the original live owner) to 'unknown', unblocking the job for a fresh claim. But it was only ever called once, at the long-lived scheduler ticker's own startup (cron/scheduler.py:379's self.recover_interrupted()) -- a one-shot CLI invocation has no equivalent "startup" moment of its own, so this self-heal never ran for it. Added a call to recover_interrupted_executions() at the top of _try_dispatch_background_run, right after the async-delivery-supported gate and before any claim attempt for the current job -- mirroring exactly what the long-lived scheduler already does at its own startup, just triggered per one-shot invocation instead of once at daemon startup. Wrapped in try/except: pass (best-effort; a failure here must not block the actual dispatch this function exists for). Traced (but did not attempt to fix) the deeper "why does the runner die with the process at all" question -- that's the harder problem options 1/2 in the issue describe (route to the persistent scheduler, or block the one-shot process until completion). This fix addresses the more urgent, more clearly-scoped symptom: a stranded stale claim permanently blocking ALL future manual runs of the affected job, which is option 3 from the issue and the one with an existing, already- correct implementation just needing to be wired into this call site. Added 3 regression tests to a new file, following the established real-subprocess dead-owner pattern already used in tests/cron/test_execution_ledger.py (a genuinely-dead PID, not a mock, matching the real-world failure mode exactly): a sanity test confirming the stale claim sits unrecovered without the fix; a direct test of recover_interrupted_executions() reaping such a claim; and a unit test on _try_dispatch_background_run itself confirming recovery is called before any claim attempt. Verified as a genuine regression by reverting the fix and confirming the unit test fails with recovery never having been called. 35/35 pass across the new test file plus tests/cron/test_execution_ledger.py and tests/tools/test_cronjob_run_background.py (no regression).
Contributor
૮ >ﻌ< ა ci reviewran on 8a5851b — fix(cron): log the pre-dispatch stale-claim reap instead of
|
alt-glitch
added
type/bug
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
3 participants
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.
Summary
Manual cron runs now self-heal stale execution claims at the dispatch point:
_try_dispatch_background_runcallsrecover_interrupted_executions()before any claim attempt, so aclaimed/runningrow stranded by a dead owner process no longer blocks a retry until the scheduler tick's 5-minute reap window (#86853) catches it.Salvages #86862 by @ygd58 onto current main (fixes the immediate-retry half of #86721).
Changes
tools/cronjob_tools.py: dead-owner claim reap at the top of_try_dispatch_background_run, before routing capture and the claim; reclaim counts logged at warning level, recovery failure at debug (follow-up commit — mirrors the scheduler tick's handling instead of the original bareexcept: pass)tests/cron/test_cron_run_stale_claim_reap_86721.py: 3 regression tests using the real-subprocess dead-PID pattern (genuinely dead owner, not a mock), including a negative control reproducing the stranded-claimedsymptomValidation
tests/cron/test_cron_run_stale_claim_reap_86721.py+tests/tools/test_cronjob_run_background.pyComplementary to #86853: that PR reaps on the long-lived scheduler tick (throttled to 5 min); this one reaps at the manual-run dispatch point, which is exactly when a user is watching a retry fail.
Infographic