Repository navigation
fix(cron): keep direct runs alive after CLI timeout - #102530
fangliquanflq wants to merge 2 commits into
Conversation
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head 4f6ba1f05811700b8e36e18ab8146ceadff894e2 against current main 63279301bcbdc185c1b07b98a9312eb0c862f26d.
The semantic direction is correct. The worker-side transition uses the right primitive: the parent fences the row as handoff-pending, adopt_claimed_execution() atomically performs the claimed -> running transition while replacing the execution owner, and the worker does not enter run_one_job() until after that adoption. Both published commits are green across CI, Docker, and Nix.
This is not merge-ready yet because the caller still has paths that manufacture a successful detached receipt without a verified adoption acknowledgement.
P1 — make verified adoption acknowledgement the only early-success gate
tools/cronjob_tools.py::_try_dispatch_detached_run() catches launcher failure and, when its owner-fenced finish_execution() returns None, converts that into:
{"claimed": True, "dispatched": True, "execution_id": ...}The public cronjob() result then turns that into a clean success saying the job is running in a detached worker; the warning is dropped. An owner-CAS miss proves only that the parent no longer owns the row. It does not prove that a durable worker acknowledged adoption.
There is a second version of the same problem in cron/scheduler.py: after the acknowledgement path is missed, _launch_external_cron_worker() falls into _wait_for_external_cron_worker(), whose True means only that the exact row became terminal. failed and recovered unknown are therefore accepted by the direct dispatcher exactly like an acknowledged handoff.
Concrete bad cut: the worker wins adopt_claimed_execution(), dies before publishing the acknowledgement, recovery settles that execution unknown, and the foreground caller can still return the durable ID with a clean “running in a detached worker” receipt. That reintroduces the unsafe-retry ambiguity this PR exists to remove.
Replace the boolean launch contract with a typed handoff result. Only an acknowledged result — a complete receipt for the same execution whose owner/status agree with the ledger — may produce immediate detached success. A terminal result must surface its exact terminal status. An uncertain result may return the durable execution ID, but it must say the ownership/result is uncertain and must never clear the claim or invite retry. A failure proven to precede adoption can still be terminalized by the original owner.
P1 — publish the acknowledgement atomically
The worker currently creates the final .ready path before the JSON receipt is complete. The parent can observe an empty or partially written final file. Write and fsync a unique temporary receipt, atomically replace it into the final acknowledgement path, and fsync the directory where supported. The final receipt should carry enough owner identity to validate the same execution/PID/process instance against the adopted ledger row. No final acknowledgement path should be visible before the complete receipt is durable.
Required partial order:
claimed row
-> handoff_pending fence
-> worker adoption CAS + committed owner identity
-> durable atomic acknowledgement publication
-> worker side effects
caller success -> only after observing and validating that acknowledgement
P1 — add the real process/lifetime witness
The new tests mock Popen and materialize a complete acknowledgement inside the mock. They prove call shape, but they cannot prove OS-level detachment, caller-death survival, or the acknowledgement/ledger cut points.
PR #102538 owns a useful real-process witness for the same issue: kill the direct caller, prove recovery leaves the live worker alone, prove the execution owner PID differs from the caller, observe the side effect, and prove the exact row reaches completed. #102530 has the stronger semantic carrier because it returns after an adoption receipt instead of keeping the caller as a waiter. Absorb/adapt that witness here rather than splitting #102511 across two implementations.
The acceptance matrix needs, at minimum:
- caller exits after validated acknowledgement; the exact execution remains owned by the worker and terminalizes once;
- worker dies before adoption;
- worker adopts and dies before acknowledgement;
- partial, malformed, stale, and mismatched acknowledgements;
- worker dies after acknowledgement;
- recovery during every boundary above;
- no
unknown/failedrow can be rendered as a clean detached-running success; - one observable side effect, never two, for the returned execution ID.
P1 — keep the new authority in a bounded owner
This change adds durable execution-handoff authority inside cron/scheduler.py around line 8,100 of a roughly 411 KB godfile, while tools/cronjob_tools.py also interprets ownership-transfer truth. That is not acceptable topology for a new lifecycle owner.
Extract one bounded module under 2,000 lines — for example cron/execution_handoff.py — that owns the payload/receipt schema, spawn and cross-platform detach policy, adoption acknowledgement protocol, typed handoff outcome, cleanup/recovery behavior, and external-worker entry. cron/scheduler.py and tools/cronjob_tools.py should be thin clients of that authority. This is part of this fix, not deferred cleanup: ambiguous ownership split across callers is the bug class being repaired.
Interlocks and receipts
- #102538 overlaps #102511. Preserve its real-process evidence, but keep #102530 as the semantic carrier and explicitly supersede/coordinate the duplicate after this carrier closes the stronger acknowledgement contract.
- #102508 and #102431 touch the shared restart-safe spawn seam but own separate questions: optional systemd-property negotiation and genuine required-substrate failure policy. The extraction must preserve those contracts rather than folding them into direct-run handoff policy.
- Current head
4f6ba1f...: CI33818527527, Docker33818527054, and Nix33818527042are green. - First commit
7458ed5...: CI33815935309, Docker33815927598, and Nix33815927185are green.
Do not change the direction of the fix. Close these ownership, evidence, and topology gates on this carrier.
Carrier lock — absorb #102538, do not fork the mechanismThis PR is the single landing carrier for #102511. #102538 is now an evidence donor only; preserve its contributor provenance while porting the real process/lifetime witness here. Acceptance remains one atomic handoff: a durable execution row/lease is created, the detached worker acknowledges adoption durably, and only then may the caller receive success plus the execution ID. Missing, failed, or ambiguous adoption must refuse. The witness must kill/timeout the foreground caller and prove that the same execution ID reaches one terminal result with no Keep the execution owner in one bounded module below 2,000 lines. Do not split authority across |
What does this PR do?
hermes cron run <job_id>now transfers a direct manual run to a detached worker and returns its durable execution ID before long agent work begins. Killing or timing out the foreground CLI therefore no longer kills the execution owner and turns an otherwise healthy run into an unknown outcome.Symptom
A long agent-backed direct run remains owned by the foreground CLI process. If that caller times out, the next scheduler recovery marks the execution
unknowneven though side effects may already have occurred.Impact
Operators cannot safely retry an
unknownattempt because the agent may already have written files or delivered messages. The execution also loses an authoritative terminal outcome solely because the invoking shell stopped waiting.Bug Cause
Trigger:
hermes_cli/cron.py:_job_actionforced direct runs through the synchronous fallback intools/cronjob_tools.py.Causal chain:
hermes cron rundisabled async delivery and called_execute_job_nowinline.run_one_jobcreated a ledger row owned by the foreground CLI PID and performed the full agent run in that process.finish_execution, so dead-owner recovery could only recordunknown.Why it is wrong: A potentially hours-long autonomous execution must not inherit the lifetime of the shell waiting for the command response.
Working sibling / contrast: Managed gateway runs already transfer execution ownership to an external worker before side effects begin. This change reuses that fenced handoff for direct CLI runs without requiring a systemd scope.
Ruled out: The execution ledger's dead-owner recovery is not the defect;
unknownis the correct result after a genuine worker crash. The defect was assigning the foreground caller as the long-lived worker owner.Fix
hermes cron runscan track the attempt.Related Issue
Closes #102511
Type of Change
Changes Made
hermes_cli/cron.py- request detached execution for direct CLI runs and print the execution receipt.tools/cronjob_tools.py- claim and dispatch direct runs without binding them to the CLI lifetime.cron/scheduler.py- support acknowledged, unscoped detached worker handoff and preserve transient prompts.tests/cron/test_restart_safe_worker.py- verify detached handoff returns before terminal completion.tests/hermes_cli/test_cron.py- verify CLI dispatch and receipt output.tests/tools/test_cronjob_run_background.py- verify claim, execution creation, ownership handoff, and result shape.How to Test
hermes cron run <job_id>and confirm it immediately prints an execution ID.hermes cron runs <job_id>to confirm the detached attempt reaches its authoritative terminal state.Result: 61 passed, 2 platform-marked tests skipped on Windows.
Checklist
Code
fix(scope):,feat(scope):, etc.)Documentation & Housekeeping
cli-config.yaml.exampleupdate is N/A; no config keys changedCONTRIBUTING.mdandAGENTS.mdupdates are N/A; no contributor workflow changedcronjobbehavior is unchangedScreenshots / Logs
N/A; automated tests cover the execution handoff and CLI receipt.