fix(cron): make script timeout process-tree cleanup bounded

[yinkev]

Summary

• run cron scripts in an isolated process group/session;
• kill the complete process tree on timeout on POSIX and Windows;
• bound the post-kill pipe drain so failed cleanup cannot hang the scheduler indefinitely;
• preserve Windows hidden-console behavior while adding a real process group;
• update current subprocess contract tests and add direct POSIX/Windows timeout regressions.

Fixes #71148.

Relationship to #71152

This preserves @webtecnica's original implementation commit and authorship, then addresses the concrete review/CI blockers on #71152:

• Windows taskkill non-zero now falls back to proc.kill();
• post-timeout communicate() is bounded to one second;
• POSIX killpg is guarded for the Windows-footgun checker and falls back to direct-child kill;
• Windows uses CREATE_NEW_PROCESS_GROUP | CREATE_NO_WINDOW through the shared compatibility helper;
• the three current Popen-contract tests are updated rather than continuing to mock subprocess.run;
• timeout tests assert both POSIX process-group kill and Windows /T /F tree kill.

Verification

• cron script, timeout-tree, and no-agent suites: 61 passed;
• Ruff, py_compile, Windows-footgun scan, and git diff --check: clean.

[teknium1]

Thanks for preserving the original implementation credit while completing the bounded cross-platform cleanup. The premise is still present on current main: cron/scheduler.py:2303-2311 uses subprocess.run(..., capture_output=True, timeout=...), and cron/scheduler.py:2335-2336 only reports timeout after the direct-child behavior of subprocess.run.

Problems

• The POSIX regression does not assert the isolation prerequisite. tests/cron/test_script_timeout_tree.py:51-61 mocks os.killpg, but tests/cron/test_cron_script.py:203-214 does not assert start_new_session=True. A future regression could retain the expected mocked kill call without creating the separate group that makes it reach descendants.

Suggested changes

• Assert captured["kwargs"]["start_new_session"] is True in the non-Windows Popen contract test.

Automated hermes-sweeper review.

[teknium1]

Please also assert captured["kwargs"]["start_new_session"] is True here. The timeout test mocks os.killpg, so without this contract assertion it would not catch a regression that launches the child in the scheduler's process group rather than an isolated group.

[GottZ]

This was generated by AI during triage.

Summary

Seven PRs address or reference the two causes in the issue complex: process-tree leaks on script timeout and misclassification of script timeouts as provider timeouts. #59379 is the recorded best fix for the two-cause issue; #71506 and #74123 provide broader process-cleanup alternatives, while the remaining PRs are partial or duplicate implementations.

Related pull requests

• fix(cron): script timeouts leave orphaned subprocess groups and get mislabeled as provider timeouts #59379 best fix — (+134/-7) — best fix: replaces subprocess.run with Popen, starts POSIX scripts in isolated sessions, kills the process group on timeout, prioritizes the script-timeout delivery branch, and adds descendant-cleanup and classifier regressions. Its contributor review concerns were explicitly addressed with start_new_session=True, direct PGID use, and tests.
• fix(cron): kill process group on script timeout, fix mislabeled failure message (#59549) #59574 partial — (+24/-7) — partial fix: addresses both causes, but the diff uses preexec_fn=os.setsid in a multithreaded scheduler, has a race-prone os.getpgid lookup, leaves the Windows timeout child unreaped, and adds no regression tests. Despite the keep_open review on fix(cron): kill process group on script timeout, fix mislabeled failure message (#59549) #59574, these concrete diff defects make it an inferior duplicate of fix(cron): script timeouts leave orphaned subprocess groups and get mislabeled as provider timeouts #59379.
• fix(cron): kill process group on script timeout and fix error message (#59549) #59924 [closed] partial — (+38/-9) — partial duplicate: adds POSIX process-group cleanup and corrects the script-timeout classification, but was closed by contributor triage as a duplicate of fix(cron): script timeouts leave orphaned subprocess groups and get mislabeled as provider timeouts #59379 and remains relevant as an earlier reference implementation of the same two-cause change.
• fix(cron): reap the whole process group when a script job times out #68262 [closed] partial — (+87/-7) — partial cleanup-only fix: adds POSIX session/group killing and a mocked timeout test, but does not correct delivery classification and tests no real descendant cleanup or leader-exit race. It remains relevant as a closed focused reference implementation, but overlaps the cleanup half of fix(cron): script timeouts leave orphaned subprocess groups and get mislabeled as provider timeouts #59379.
• fix(cron): kill whole process group on script timeout to prevent orphaned descendants (#71148) #71152 duplicate — (+75/-23) — partial cleanup-only fix: adds POSIX group killing and Windows taskkill, but the recorded diff ignores taskkill failure, performs an unbounded post-kill communicate(), and lacks timeout regressions. Despite the MAINTAINER-BOT keep_open verdict, the shown defects and overlap with fix(cron): make script timeout process-tree cleanup bounded #71506 support treating it as a superseded duplicate.
• fix(cron): make script timeout process-tree cleanup bounded #71506 best fix — (+183/-31) — partial cleanup fix: adds bounded post-kill draining, POSIX and Windows tree-kill fallbacks, platform-specific process isolation, and timeout regressions, but does not fix the separate delivery misclassification. The contributor keep_open review on fix(cron): make script timeout process-tree cleanup bounded #71506 identifies the remaining missing start_new_session contract assertion; retain the bounded cleanup as salvageable rather than treating this as the complete two-cause fix.
• fix(cron): kill the whole process group when a cron script times out #74123 best fix — (+470/-15) — best fix for the process-tree issue: adds bounded cleanup, scheduler-group protection, Windows taskkill return-code fallback, direct-child fallback, and real descendant/pipe-holder regressions, but leaves the provider-timeout misclassification unchanged. Its contributor keep_open review was addressed in the diff by checking non-zero taskkill results and adding the requested fallback test.

Duplicates

#59574 and closed #59924 substantially duplicate the two-cause implementation in #59379. #68262, #71152, #71506, and #74123 overlap on process-tree cleanup; #71152 is superseded by the bounded cleanup in #71506, while #74123 is a broader alternative cleanup implementation.

Suggested consolidation

For #71506, keep open with a salvage path: retain its bounded post-kill drain and cross-platform cleanup ideas, add the requested start_new_session contract assertion, and either rebase onto #59379 or split the cleanup slice for incorporation there; do not recommend merging it. Keep #74123 available as the stronger process-cleanup reference, while author-action on #59379 should rebase or split the complete two-cause fix; close #59574 as a duplicate of #59379, close #71152 as superseded by #71506/#74123, and leave #59924 and #68262 closed as duplicate/reference implementations.

Complex graph

flowchart TD
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I59549(["issue #59549 (open)"])
    I71148(["issue #71148 (open)"])
    subgraph Dup59379 ["PRs duplicating each other"]
        P59379["PR #59379 (open)"]
        P59574["PR #59574 (open)"]
        P59924["PR #59924 (closed)"]
        P68262["PR #68262 (closed)"]
        P71152["PR #71152 (open)"]
        P71506["PR #71506 (open)"]
        P74123["PR #74123 (open)"]
    end
    P71506 -.->|partial| I59549
    P71506 -->|best fix| I71148
    class I59549 open
    class I71148 open
    class P59379 open
    class P59574 open
    class P59924 closed
    class P68262 closed
    class P71152 open
    class P71506 open
    class P74123 open
    class P59379 best
    class P59379 best
    class P71506 best
    class P74123 best
    class P71506 target
    click I59549 "https://github.com/NousResearch/hermes-agent/issues/59549"
    click I71148 "https://github.com/NousResearch/hermes-agent/issues/71148"
    click P59379 "https://github.com/NousResearch/hermes-agent/pull/59379"
    click P59574 "https://github.com/NousResearch/hermes-agent/pull/59574"
    click P59924 "https://github.com/NousResearch/hermes-agent/pull/59924"
    click P68262 "https://github.com/NousResearch/hermes-agent/pull/68262"
    click P71152 "https://github.com/NousResearch/hermes-agent/pull/71152"
    click P71506 "https://github.com/NousResearch/hermes-agent/pull/71506"
    click P74123 "https://github.com/NousResearch/hermes-agent/pull/74123"

Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 7 pull requests and 2 issues in this complex. Each diff was read against this issue; Assessment working set: 64 kB of PR diffs, 21 kB of issue/PR text, 15 kB of discussion (21 comments), 20 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

[monerostar]

Ubuntu 26.04 linux-5800x (kernel 7.0.0-28-generic) here.

Live check on PR head 011fad0:

• pytest tests/cron/test_script_timeout_tree.py tests/cron/test_cron_script.py: 24 passed in 1.48s
• main scheduler has no killpg / start_new_session path for cron scripts. PR adds _terminate_cron_script_tree with os.killpg and starts the script session with start_new_session=True so the group kill can land.

Looks good from Linux for the orphaned-descendant timeout class.

[yinkev]

Decision gate: superseded as a merge branch

I compared current main (a31be4803), this PR (011fad086), #74123 (326fa2446), #59379 (51dd37ffe), their regression suites, and the August 3 cross-PR maintainer triage.

The underlying bug still exists on current main: cron scripts still use subprocess.run(..., timeout=...), so timeout cleanup reaches only the direct child, and the separate delivery classifier still labels Script timed out ... as a provider timeout.

This branch contains two useful design choices: direct POSIX os.killpg(proc.pid, SIGKILL) avoids the leader-exit race introduced by looking up the PGID after timeout, and its post-kill drain is bounded. It is nevertheless no longer the correct branch to finish for merge:

• its process-tree regressions are mocked rather than real descendant/pipe-holder executions;
• the requested start_new_session=True contract assertion is still missing;
• cleanup runs only for TimeoutExpired, not for other exceptions introduced after replacing subprocess.run with explicit Popen;
• after the one-second drain expires, it does not close inherited pipes or make a final bounded reap attempt;
• it lacks the scheduler-process-group safety regression, Windows non-zero taskkill fallback regression, and the separate script-timeout delivery-classification fix.

#74123 materially supersedes this branch for the cleanup half: it adds bounded kill-and-reap handling, real descendant and inherited-pipe regressions, non-timeout failure cleanup, an actual own-session execution test, Windows taskkill return-code fallback, and scheduler-group protection. #59379 remains the source of the separate script-timeout classification fix and also establishes the safer direct-PGID rationale.

This matches the current maintainer triage: keep #71506 available as salvageable reference material, but do not recommend merging it; use #74123 as the stronger cleanup reference and #59379 for the complete two-cause issue.

Recommendation

Leave #71506 open and unchanged as a reference branch. Consolidate a maintained replacement from:

1. fix(cron): kill the whole process group when a cron script times out #74123's bounded drain, catch-all cleanup, Windows fallback, and real integration regressions;
2. fix(cron): script timeouts leave orphaned subprocess groups and get mislabeled as provider timeouts #59379/fix(cron): make script timeout process-tree cleanup bounded #71506's direct killpg(proc.pid, ...) approach so descendants are still targetable if the session leader exits before cleanup;
3. fix(cron): script timeouts leave orphaned subprocess groups and get mislabeled as provider timeouts #59379's script-specific delivery-classification fix.

I am intentionally not rebasing or pushing this branch and not resolving its review thread, because adding the single assertion would make the stale branch look merge-ready while duplicating a materially stronger implementation.

[kshitijk4poor]

Closing per #85125 Phase 4a: the cron script timeout now tree-kills via agent.deadline.kill_process_tree (PR #93795, merged) — including setsid'd descendants. The post-kill pipe drain is bounded by _drain_script_pipes (communicate timeout=5s → kill → wait timeout=5s → abandon to OS reaper). @yinkev's process-tree cleanup approach informed the design; credit preserved in the 4a PR body.