Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Ait0u5hi
# cron process-group reaping fix
41 changes: 34 additions & 7 deletions cron/scheduler.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import os
import re
import shutil
import signal
import subprocess
import sys
import threading
Expand Down Expand Up @@ -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:
Expand All @@ -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:
Expand Down
51 changes: 51 additions & 0 deletions tests/cron/test_scheduler_process_group_reaping.py
Original file line number Diff line number Diff line change
@@ -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()