fix(cron): script timeouts leave orphaned subprocess groups and get mislabeled as provider timeouts

[dante32683]

Two related bugs in `cron/scheduler.py`'s pre-run/data-collection script execution:

1. Orphaned subprocess groups on timeout. `_run_job_script()` uses `subprocess.run(..., timeout=script_timeout)`. On `TimeoutExpired`, `subprocess.run` internally kills only the direct child — any grandchildren the script spawned (background `&` jobs, a `wget`/`curl` a watchdog script kicked off, etc.) are orphaned and keep running after the cron tick reports failure. This is silent: nothing surfaces that cleanup didn't happen.

2. Script timeouts are mislabeled as provider timeouts in chat delivery. `_summarize_cron_failure_for_delivery()` checks `"timed out" in lower` before any more specific check, and the script-path error message is literally `f"Script timed out after {script_timeout}s: {path}"` — which matches that branch and gets rewritten as `"⚠️ Cron 'x' failed: provider timeout. Fallback chain was exhausted or unavailable."` That's actively wrong: it's not a provider issue and there's no fallback chain involved, it's a runaway script. An operator seeing this message would look in the wrong place to debug it.

The fix:

• On `TimeoutExpired`, kill the whole process group (`os.killpg` + `SIGKILL`) on POSIX, matching the existing Windows fallback path (`proc.kill()`) that's already there for the non-POSIX case. Requires switching from `subprocess.run` to `subprocess.Popen` + `communicate(timeout=...)` so the process group can be resolved via `os.getpgid(proc.pid)` before killing, and starting the child in its own session via `preexec_fn=os.setsid` (POSIX-only, gated behind `sys.platform != "win32"`, per the contributing guide's cross-platform rule) so `killpg` has a group to target.
• Check for the literal `"script timed out"` substring in `_summarize_cron_failure_for_delivery` before the generic `"timed out"` provider-timeout branch, so script timeouts get their own compact message instead of being misattributed.

Test plan

A cron job pointed at a script that spawns a detached long-running background child (`sleep 300 &`) and then itself loops past the configured timeout — confirmed before the fix that the background `sleep` survived the cron tick's failure; confirmed after the fix that `os.killpg` cleans up the full group. Also confirmed the chat-delivered failure message for a script timeout no longer says "provider timeout."

[tonydwb]

Code Review

COMMENT: fix(cron): script timeouts leave orphaned subprocess groups and get mislabeled as provider timeouts

LGTM. Important reliability fix for cron scheduler — prevents orphaned subprocess groups and mislabeled timeouts. Clean 1-file fix. No security concerns.

---

Reviewed by Hermes Agent

[dante32683]

Filed #59549 with the two bugs and repro steps in case that's useful context for review.

[teknium1]

Thanks for addressing two real cron failures. Current main still uses subprocess.run(..., timeout=...) at cron/scheduler.py:2101 and classifies its Script timed out ... error as a provider timeout at cron/scheduler.py:75-79.

Problems

• cron/scheduler.py:2013 adds preexec_fn=os.setsid, but current main can start a claim-heartbeat thread before invoking this runner (cron/scheduler.py:2179-2196). Please use POSIX start_new_session=True instead; tools/tts_tool.py:787 already uses that pattern.
• cron/scheduler.py:2029 uses os.getpgid(proc.pid). Once a new session is created, proc.pid is the PGID; querying the leader can fail if it exits in the timeout race while descendants remain. Use os.killpg(proc.pid, signal.SIGKILL) so the surviving group is still targeted.
• The diff changes no tests. Add coverage for descendant cleanup and for the script-specific delivery summary; current timeout coverage only checks a direct child at tests/cron/test_cron_script.py:184-196.

This is an automated hermes-sweeper review.

[teknium1]

Use start_new_session=True rather than preexec_fn=os.setsid. Current main can start a claim-heartbeat thread before this call (_run_job_script_with_claim_heartbeat), and the repository already uses start_new_session=True for process-tree isolation in tools/tts_tool.py:787.

[teknium1]

After creating a new session, proc.pid is the PGID. Calling getpgid(proc.pid) can fail if the direct child exits in the timeout race while descendants still hold the group, and this broad handler then leaves them alive. Target os.killpg(proc.pid, signal.SIGKILL) directly.

[dante32683]

Thanks — all three addressed (6718c8e, 51dd37f), rebased onto current main.

• start_new_session=True instead of preexec_fn=os.setsid. Confirmed the concern is real: _run_job_script is called with the claim-heartbeat thread already started, so preexec_fn runs post-fork in a multithreaded process. Matches the tools/tts_tool.py precedent.
• os.killpg(proc.pid, SIGKILL) — with the new session the child is the group leader, so its pid is the PGID and the os.getpgid() race you described is gone.
• Tests added. test_script_timeout_kills_descendants spawns a grandchild from a timed-out script and asserts it dies with the group, and test_script_timeout_summary_is_not_labeled_provider_timeout covers the delivery-summary branch.

Two notes on the descendant test, in case they're useful. The grandchild detaches its stdio deliberately: if it inherits the pipes, communicate() blocks on them until it exits anyway, which masks whether the group kill worked (my first version passed even with the fix reverted for exactly that reason). It's also zombie-aware via psutil, following test_entire_tree_is_sigkilled_not_just_parent, since an orphan reparented to init may not be reaped promptly. I confirmed both tests fail with their respective fixes reverted and pass with them restored. Full tests/cron/ suite: 695 passed.

[supotato-ipj]

+1 with production confirmation on the message-mislabeling half — detailed evidence in #59549 (comment): a no_agent script that hit the 3600s script_timeout was delivered to chat as provider timeout. Fallback chain was exhausted or unavailable, which sent ops debugging the LLM provider for a week while the failure was script-side all along. The reorder in this PR (match Script timed out before the generic provider-timeout branch) covers exactly that path. Anything blocking merge that a tester can help with?

[ayushnangia]

Coordination note for when the unified deadline layer lands (#85125 Phase 1, #85147): the tree-kill half of this PR is scheduled to be implemented on agent/deadline.py:kill_process_tree rather than a site-local os.killpg, with your start_new_session=True mechanism (the thread-safe choice over preexec_fn, per your rebase note) and this PR's authorship preserved via Co-authored-by when it lands. The classification half is already on main — teknium1 cherry-picked @jbagdonas's #82460 into #85536, composed with #77648. @supotato-ipj's #59549 production evidence carries over as the mislabeling repro. If you'd rather drive this PR yourself once Phase 1 merges, say so and I'll stand down — otherwise it lands with your work credited as above.

[ayushnangia]

Executing the alignment per the notice above: the tree-kill half now migrates onto the unified deadline layer — #86791 replaces the site-local group kill with agent.deadline.kill_process_tree (psutil snapshot reaches own-session grandchildren that killpg cannot), with this PR's start_new_session=True mechanism already on main and your authorship carried as Co-authored-by on the new commit. @supotato-ipj's #59549 production evidence is credited the same way. If you'd rather own this yourself, the offer to stand down stands — otherwise review on #86791 is welcome.

[Enough1122]

AI code review — automated review for reference, author can ignore or act on any point.

fix(cron): script timeouts leave orphaned subprocess groups and get mislabeled as provider timeouts

1. "script timed out" substring may not match production text — cron/scheduler.py _summarize_cron_failure_for_delivery: the new branch matches "script timed out" in lower, but Python's native subprocess.TimeoutExpired str is Command '<argv>' timed out after N seconds. The test hand-authors "Script timed out after 300s: ...", which implies the caller reformats the error before it reaches the summarizer — worth verifying that reformat actually produces that literal text, otherwise the branch silently never fires and the mislabeling returns.
2. PID-reuse race before killpg — _run_job_script timeout path: between communicate() raising TimeoutExpired and os.killpg(proc.pid, SIGKILL), the direct child may have exited and proc.pid could be recycled, killing an unrelated process group. A liveness probe (proc.poll() or os.killpg(proc.pid, 0)) before the kill would close the window.
3. Windows orphan behavior unchanged (by design) — on win32 the code still kills only the direct child, so orphaned descendants remain on that platform. The comment explains the POSIX rationale, but a brief note that Windows keeps the old behavior would prevent future readers from assuming parity.