diff --git a/contributors/emails/ait0u5hi@users.noreply.github.com b/contributors/emails/ait0u5hi@users.noreply.github.com new file mode 100644 index 0000000000000..03959dcdfdfd7 --- /dev/null +++ b/contributors/emails/ait0u5hi@users.noreply.github.com @@ -0,0 +1,2 @@ +Ait0u5hi +# cron process-group reaping fix diff --git a/cron/scheduler.py b/cron/scheduler.py index c5957070021e7..6087edcc66b77 100644 --- a/cron/scheduler.py +++ b/cron/scheduler.py @@ -17,6 +17,7 @@ import os import re import shutil +import signal import subprocess import sys import threading @@ -2204,19 +2205,45 @@ def _run_job_script(script_path: str) -> tuple[bool, str]: "encoding": "utf-8", "errors": "replace", } + else: + # Own session/process-group so a timeout can reap the WHOLE tree. + # subprocess.run's timeout SIGKILLs only the direct child, orphaning + # backgrounded grandchildren; a child wedged in D-state then hangs + # the reap, leaking stuck processes (the 2026-07-15 tegrastats + # incident class). A new session lets us kill the process group. + popen_kwargs["start_new_session"] = True env = _sanitize_subprocess_env(os.environ.copy()) env.update(env_overlay) - result = subprocess.run( + proc = subprocess.Popen( argv, - capture_output=True, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, text=True, - timeout=script_timeout, cwd=str(path.parent), env=env, **popen_kwargs, ) - stdout = (result.stdout or "").strip() - stderr = (result.stderr or "").strip() + try: + out, err = proc.communicate(timeout=script_timeout) + except subprocess.TimeoutExpired: + # Kill the entire process group (backgrounded grandchildren too), + # not just the direct child. Best-effort on a wedged/D-state tree: + # SIGKILL the group, then reap without blocking forever. + if sys.platform != "win32": + try: + os.killpg(os.getpgid(proc.pid), getattr(signal, "SIGKILL", signal.SIGTERM)) # windows-footgun: ok + except (ProcessLookupError, PermissionError, OSError): + pass + else: + proc.kill() + try: + proc.communicate(timeout=5) + except Exception: + pass + raise + returncode = proc.returncode + stdout = (out or "").strip() + stderr = (err or "").strip() # Redact secrets from both stdout and stderr before any return path. try: @@ -2228,8 +2255,8 @@ def _run_job_script(script_path: str) -> tuple[bool, str]: stdout = "[REDACTED - redaction failed]" stderr = "[REDACTED - redaction failed]" - if result.returncode != 0: - parts = [f"Script exited with code {result.returncode}"] + if returncode != 0: + parts = [f"Script exited with code {returncode}"] if stderr: parts.append(f"stderr:\n{stderr}") if stdout: diff --git a/tests/cron/test_scheduler_process_group_reaping.py b/tests/cron/test_scheduler_process_group_reaping.py new file mode 100644 index 0000000000000..93c8b1fe25cde --- /dev/null +++ b/tests/cron/test_scheduler_process_group_reaping.py @@ -0,0 +1,51 @@ +"""Cron script jobs must reap the whole process GROUP on timeout. + +`subprocess.run(timeout=...)` SIGKILLs only the direct child, so a backgrounded +grandchild is orphaned and a child wedged in D-state hangs the reap. `_run_job_script` +now runs the script in its own session/process group and, on TimeoutExpired, +`os.killpg`s the whole group. This is the regression test for that behavior. +""" +import subprocess +import sys +from unittest.mock import MagicMock, patch + +import pytest + +import cron.scheduler as sched +from cron.scheduler import _run_job_script + + +@pytest.mark.skipif( + sys.platform == "win32", + reason="process-group reaping is POSIX-only; Windows uses proc.kill()", +) +def test_timeout_kills_whole_process_group(tmp_path, monkeypatch): + # Point the scripts dir at a temp HERMES_HOME and drop a valid .sh in it. + monkeypatch.setattr(sched, "_get_hermes_home", lambda: tmp_path) + scripts = tmp_path / "scripts" + scripts.mkdir() + (scripts / "reap.sh").write_text("#!/bin/bash\nsleep 300 &\nsleep 300\n") + + fake = MagicMock() + fake.pid = 4242 + fake.returncode = -9 + # First communicate() (with the job timeout) wedges; the post-kill reap returns. + fake.communicate.side_effect = [ + subprocess.TimeoutExpired(cmd="reap.sh", timeout=1), + ("", ""), + ] + + with patch("cron.scheduler.subprocess.Popen", return_value=fake) as popen, \ + patch("cron.scheduler.os.getpgid", return_value=4242) as getpgid, \ + patch("cron.scheduler.os.killpg") as killpg: + ok, msg = _run_job_script("reap.sh") + + # Child is launched in its own session so the group is killable... + assert popen.call_args.kwargs.get("start_new_session") is True + # ...and the WHOLE group is killed on timeout, not just the direct child. + getpgid.assert_called_once_with(4242) + killpg.assert_called_once() + assert killpg.call_args.args[0] == 4242 + # The timeout is still reported back to the caller. + assert ok is False + assert "timed out" in msg.lower()