Skip to content

fix(cron): import run_agent before consuming worker handoff payload - #124886

Closed
Arno-MA-73 wants to merge 1 commit into
NousResearch:mainfrom
Arno-MA-73:fix/cron-worker-handoff-reexec-main
Closed

Arno-MA-73 wants to merge 1 commit into
NousResearch:mainfrom
Arno-MA-73:fix/cron-worker-handoff-reexec-main

Conversation

@Arno-MA-73

Copy link
Copy Markdown

What this changes

Fixes #124827: an external cron worker that re-execs or relaunches during run_agent import after its one-shot handoff payload was already deleted, so the execution is recovered as unknown with no output or delivery.

Move the run_agent import inside the worker's profile scope before the payload is consumed and before adoption/acknowledgement. That import runs hermes_bootstrap (which can execv into another interpreter) and interrupted-update recovery (restore_interrupted_pull → relaunch_after_restore). Keeping the handoff file until both finish means a relaunched process still has its argv payload; only after the import completes does the worker adopt ownership and publish the ready acknowledgement. A worker that cannot load the agent now fails before acknowledgement, where the gateway records a real failed run instead of a silent unknown.

Relationship to #122222 / #122936

#122222 is the separate pre-ack missing-dependency bug (the worker spawns via sys.executable and never reaches any Hermes import); #122936 is the in-flight fix for that. This change is independent of it and does not include it — the two fix different phases of the worker lifecycle. On installs affected by both, both are needed; this PR only addresses the post-ack re-exec.

Verification

  • tests/cron/test_restart_safe_worker.py: 31 passed on main plus this change (direct pytest against this host's test venv; not a CI claim).
  • New coverage: import-time ordering (payload present, no ack/adoption yet), a simulated re-exec interruption that leaves the handoff intact and un-adopted, and a real os.execv subprocess round trip proving the payload survives across the exec and is consumed only by the second process.
  • Reproduced the underlying failure on a self-managed Linux gateway; the ordering change moves the failure point from post-ack (silent unknown) to pre-ack (recorded failed) when the agent cannot be loaded.

Scope and compatibility

No HERMES_DISABLE_LAZY_INSTALLS suppression is applied, so tool-time lazy installs (including security scanners) are unaffected. The change touches only the external-worker adoption path; the gateway ticker and in-process cron path are unchanged.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cron Cron scheduler and job management labels Sep 27, 2026
@Enough1122

Copy link
Copy Markdown

AI code review — automated follow-up for reference; not a maintainer.

Moving payload_path.unlink() past a new import run_agent is right for the worker
side. But the other side of the handoff is untouched, and it can still take the payload
away.

P1 — the gateway's own ack deadline reaps the payload this PR protects.

The parent creates payload_path at cron/scheduler.py:3522; three actors unlink it.
The worker's actor is now deferred to cron/scheduler.py:3729-3730. But
_wait_for_external_cron_worker still ends in an unconditional
for stale in handoff_files: stale.unlink(missing_ok=True)
(cron/scheduler.py:3450-3454), and the ack-deadline exit still hands it the payload
(cron/scheduler.py:3675-3680), reached once time.monotonic() passes
deadline = time.monotonic() + HANDOFF_ADOPTION_GRACE_SECONDS
(cron/scheduler.py:3593) — 30.0 in cron/executions.py:30, untouched here.

So the re-exec only works if the import returns inside those 30s. If it doesn't, the
re-launched child re-enters, hits read_text() at cron/scheduler.py:3691, gets
FileNotFoundError, falls into the except at :3695 and returns False — no ack,
no adoption, job never runs, row left pending: the same lost handoff, relocated. The
margin is thin, since the gateway's own comment at cron/scheduler.py:3589-3592 says a
cold worker start "measures ~10-12s in the field", and the comment at :3724-3727
concedes the import "completes (or relaunches)" — hermes_bootstrap really does
os.execv (hermes_bootstrap.py:543), restarting the process and repeating the sync.

Running the real module on both revisions (asserting the resolved __file__) gives the
same result on base b4410b4b and head 34373cdd: the waiter deletes an existing
handoff_files entry, and the worker then returns False with no ack. Trigger: any
handoff where the import at :3728 exceeds the
grace. Suggest re-checking process.poll() before reaping, or dropping payload_path
from handoff_files.

P2 — the new test's stub makes that cost unmeasurable.

test_worker_imports_agent_before_adopting_or_consuming_payload takes preloaded_agent,
which stubs sys.modules["run_agent"] with an empty ModuleType
(tests/cron/test_restart_safe_worker.py:29), so the import returns instantly: the
test proves ordering, never cost. A real import on the real clock would catch P1.

Unverified: the 30s trigger is path inference, not a measurement.

@kshitijk4poor

Copy link
Copy Markdown

Fixed on main by de116d8 (#134358), salvaged from #133720 (thanks @mzyas, authorship kept). The external cron worker now runs hermes_bootstrap from its __main__ before it reads the payload or publishes the ack, and keeps the PM boot marker until then. A source-update relaunch therefore replays the whole worker instead of landing after the ack with the payload gone. The interrupted-pull re-exec is covered by hermes_bootstrap's own launch-time repair.

Thanks @Arno-MA-73 for this alternative, and for the precise root-cause report. Closing in favour of #134358. If you still see #124827 on a build that includes de116d8, please comment and we'll reopen.

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

Labels

comp/cron Cron scheduler and job management P2 Medium — degraded but workaround exists type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(cron): adopted external worker can re-exec after deleting one-shot payload, leaving run unknown

4 participants