fix(cron): wait for hermes cron run instead of detaching - #86739
fix(cron): wait for hermes cron run instead of detaching#86739Adolanium wants to merge 1 commit into
Conversation
One-shot hermes cron run must finish the job in this process. A leftover HERMES_SESSION_KEY was enough to claim the job, start a background thread, then exit and kill the runner. allow_background=False on the CLI run path keeps the inline fire. Dead execution rows are marked unknown before the new claim. Gateway cronjob(action=run) still detaches. Fixes NousResearch#86721
trevorgordon981
left a comment
There was a problem hiding this comment.
Correct direction and cleanly scoped. Forcing allow_background=False for one-shot hermes cron run is the right fix for #86721 — a one-shot process that detaches a runner thread and then exits kills the job mid-flight and leaves the fire claim hanging. The test test_allow_background_false_stays_inline_even_with_session_key is exactly the regression that matters (leftover Desktop/gateway HERMES_SESSION_KEY no longer triggers a doomed background dispatch). Two smaller notes.
1. recover_interrupted_executions() failure is silent
The call before the forced-inline run is wrapped in except Exception: pass. If recovery itself fails (e.g. the state.db it reads is locked or malformed), the user gets no signal and cron run proceeds — and could re-queue or collide with a claim the recovery was supposed to reap. This matches the surrounding defensive style, so it's not out of character, but for the specific purpose here — "reap dead claims so the inline run can proceed cleanly" — a silent recovery failure undermines the very thing the call exists for. Consider logging a warning (at minimum) rather than a bare pass, or only proceeding when recovery reports success.
2. allow_background defaults True — correct for the tool, but confirm no other one-shot path needs the same guard
cronjob(..., allow_background: bool = True) preserves the existing in-session tool behavior, and only the CLI cron run path sets it False. That's the right split. Worth confirming there's no other one-shot entry point (e.g. a --run flag on cron list, a gateway-side manual trigger, or a tool call from a headless script that also exits immediately) that should get the same treatment — otherwise #86721 can resurface through a sibling path.
Tests
Good — test_oneshot_run_forces_inline_and_reaps_dead_claims asserts allow_background=False is passed and recover_interrupted_executions is invoked, and the test_allow_background_false_stays_inline_even_with_session_key case covers the leftover-env regression with dispatch_async_delegation asserted-not-called and inline run_one_job called once. Missing: a test for the recovery-failure path (finding #1).
|
Thanks @Adolanium — you diagnosed this first (3 hours before the fix that landed), and your analysis of the one-shot-process/detached-runner failure mode was exactly right. The fix merged today in #86853 resolves #86721 through a slightly different mechanism: Closing as resolved on main. Appreciate the fast, well-tested report-to-fix turnaround — the regression tests in your PR shaped the coverage that landed. |
What does this PR do?
hermes cron runis a one shot command. The process ends as soon as the command returns.If the shell still has a leftover Desktop or gateway session key, the run was sent to a background thread in that same process. The process then exited. The thread died. The job stayed claimed. The next
hermes cron runfailed.This PR makes
hermes cron runwait in the current process until the job finishes. A leftover session key cannot detach it.Gateway and live agent
cronjob(action="run")still use the background path. Those processes stay alive.Before the new run, the CLI also marks leftover execution rows as unknown when their owner process is already dead.
Related Issue
Fixes #86721
Type of Change
Changes Made
tools/cronjob_tools.py: addallow_background(default true).hermes cron runsets it false so the job runs inline.hermes_cli/cron.py: one shotrunalways passesallow_background=False. It also callsrecover_interrupted_executions()first.tests/tools/test_cronjob_run_background.py: with a session key bound,allow_background=Falsestill runs inline and never dispatches.tests/hermes_cli/test_cron.py: the CLI run command forces inline and reaps dead claims.How to Test
HERMES_SESSION_KEYis set,hermes cron run <id>can return at once, leaveexecutions.status=claimed, and leave a deadasync_delegationsrow. The next run can fail.hermes cron run <id>waits until the job finishes and printsRan now: succeeded.orRan now: failed.python -m pytest tests/tools/test_cronjob_run_background.py tests/hermes_cli/test_cron.py -q(28 passed on Windows 11).cronjob(action="run")still returns a background handle.Checklist
Code
fix(scope):,feat(scope):, etc.)Documentation & Housekeeping
docs/, docstrings) or N/Acli-config.yaml.exampleif I added/changed config keys or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows or N/A