From dd7f1a58b82b03e9d55660d13a27479c33e0597f Mon Sep 17 00:00:00 2001 From: James Nightingale Date: Sun, 23 Aug 2026 18:31:26 +0000 Subject: [PATCH 1/2] build_util: the per-script cap kills the process group, not just the child MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `subprocess.run(timeout=...)` kills only the direct child. A grandchild — a Popen'd helper, a multiprocessing worker, a JAX compile server — outlives the cap and runs on, holding whatever memory and GPU it had. Over a mega-run of hundreds of scripts those accumulate against every script that follows. Add `run_capped`, a drop-in for the `subprocess.run(..., timeout=...)` calls this module made: same TimeoutExpired and CalledProcessError, same captured output/stderr attributes, so every existing handler — `_timeout_output`, `is_clean_skip_exit`, the ScriptResult/TIMEOUT report paths — is untouched. It runs the child with `start_new_session=True` and SIGKILLs the group on expiry. `kill_group` is public so the workspace `run_smoke.py` runners can import it rather than each growing a copy. Both `execute_notebook` and `execute_script` switch over. Note this is a resource-leak fix, NOT a hang fix: on POSIX `subprocess.run` handles its own timeout with `process.wait()` on the direct child only, so it already returned at the cap. The uncapped runners that hang to the Actions ceiling are a separate problem in the workspace copies, not here. Regression test asserts the grandchild is gone one second after the cap fires; against the previous code it fails `assert 1 == 0` with the TIMEOUT status already correct, isolating the group kill as the thing under test. Verified: tests/test_script_timeout.py 27 passed; full suite 354 passed with the same 14 pre-existing failures main has (missing ipynb-py-convert and image optimisers in this environment). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GTPM1RmMvSuMvJkEntAMv8 --- autohands/build_util.py | 74 +++++++++++++++++++++++++++++++++--- tests/test_script_timeout.py | 64 +++++++++++++++++++++++++++++++ 2 files changed, 133 insertions(+), 5 deletions(-) diff --git a/autohands/build_util.py b/autohands/build_util.py index b89e25e..9b84f26 100644 --- a/autohands/build_util.py +++ b/autohands/build_util.py @@ -2,6 +2,7 @@ import logging import os import re +import signal import subprocess import sys import time @@ -28,7 +29,7 @@ def timeout_for(env=None) -> int: so on its own it can only ever express ONE cap for a whole run. The per-script environment built by ``env_config.build_env_for_script`` is handed to the child, and a profile may set ``BUILD_SCRIPT_TIMEOUT`` on it - for a matching pattern — but the ``subprocess.run(timeout=...)`` kill timer + for a matching pattern — but the ``run_capped(timeout=...)`` kill timer lives in the PARENT, so that value has no effect unless the parent reads it back out. This resolves it. @@ -87,6 +88,68 @@ def tail(stream) -> str: return "\n".join(parts) + +def kill_group(proc: subprocess.Popen) -> None: + """SIGKILL the child's whole process group, tolerating an already-dead one. + + Public (unprefixed) because the workspace `run_smoke.py` runners import it + rather than each growing a copy. + """ + try: + os.killpg(os.getpgid(proc.pid), signal.SIGKILL) + except (ProcessLookupError, PermissionError): # pragma: no cover - race + proc.kill() + + +def run_capped(args, timeout, check=False, stdout=None, stderr=None, + text=False, env=None, cwd=None) -> subprocess.CompletedProcess: + """`subprocess.run`, but a timeout kills the child's whole process GROUP. + + A drop-in for the `subprocess.run(..., timeout=...)` calls this module used + to make: same `TimeoutExpired` and `CalledProcessError`, carrying the same + captured `output`/`stderr`, so every caller's handling is unchanged. + + The group is the point. `subprocess.run` kills only the direct child and + then reads the pipes to EOF -- but any grandchild that inherited stdout + holds that pipe open after the child dies, so the read blocks and the + timeout does not actually stop anything. A script whose work has finished + can therefore hang the runner indefinitely, which is how smoke CI came to + sit at the 6-hour GitHub Actions ceiling reporting nothing since the last + completed script (autolens_workspace_test#196). `start_new_session=True` + puts the child in its own group; killing the group closes the inherited + pipe and lets the read finish. + """ + proc = subprocess.Popen( + args, + stdout=stdout, + stderr=stderr, + text=text, + env=env, + cwd=cwd, + start_new_session=True, + ) + try: + output, errs = proc.communicate(timeout=timeout) + except subprocess.TimeoutExpired: + kill_group(proc) + # The group is gone, so this drains whatever was buffered and returns. + output, errs = proc.communicate() + raise subprocess.TimeoutExpired( + args, timeout, output=output, stderr=errs + ) from None + except BaseException: + # Matches subprocess.run's context manager: never leave the child (or + # its group) running when the caller is unwinding, e.g. on Ctrl-C. + kill_group(proc) + proc.wait() + raise + if check and proc.returncode != 0: + raise subprocess.CalledProcessError( + proc.returncode, args, output=output, stderr=errs + ) + return subprocess.CompletedProcess(args, proc.returncode, output, errs) + + def py_to_notebook(filename: Path): subprocess.run( ["python3", f"{BUILD_PATH}/add_notebook_quotes.py", filename, "temp.py"], @@ -305,7 +368,7 @@ def execute_notebook(f, report=None, env=None): # from the repo root. nbconvert has no CLI flag for the kernel cwd, so # the runner sets resources['metadata']['path'] via the Python API. # Still a subprocess, so isolation/timeout/env are unchanged. - subprocess.run( + run_capped( [ sys.executable, str(Path(__file__).parent / "run_notebook.py"), @@ -456,16 +519,17 @@ def execute_script(f, report=None, env=None, extra_args=None): start = time.time() try: if report is not None: - result = subprocess.run( + result = run_capped( args, check=True, timeout=timeout_secs, - capture_output=True, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, text=True, env=env, ) else: - subprocess.run( + run_capped( args, check=True, timeout=timeout_secs, diff --git a/tests/test_script_timeout.py b/tests/test_script_timeout.py index fd1504e..e82bb79 100644 --- a/tests/test_script_timeout.py +++ b/tests/test_script_timeout.py @@ -18,11 +18,19 @@ script cannot report its own progress, so without that tail a TIMEOUT cannot say which block was executing — the reason the three jax_grad timeouts could not be diagnosed from CI artefacts at all (PyAutoHands#226). + +A third property, covered by ``TestTimeoutKillsProcessGroup``: the kill must +reach the child's whole process GROUP. ``subprocess.run(timeout=...)`` kills +only the direct child, so a grandchild outlives the cap and keeps running -- +over a long mega-run those leak, each holding whatever memory and GPU it had. +``build_util.run_capped`` puts the child in its own session and SIGKILLs the +group instead. """ import os import subprocess import sys +import time from pathlib import Path import pytest @@ -207,3 +215,59 @@ def test_timeout_preserves_child_stdout(self, tmp_path, monkeypatch, real_interp message = report.results[0].error_message assert report.results[0].status == Status.TIMEOUT assert "=== variant 3 ===" in message + + +class TestTimeoutKillsProcessGroup: + """The cap must reap the child's descendants, not just the child.""" + + # A script that finishes its own work immediately but leaves a grandchild + # running and holding the inherited stdout pipe. Under a plain + # `subprocess.run(timeout=...)` the grandchild survives the cap. + _SPAWNS_GRANDCHILD = ( + "import subprocess, sys\n" + "subprocess.Popen([sys.executable, '-c',\n" + " 'import time; MARKER_{marker}=1; time.sleep(120)'])\n" + "print('work done', flush=True)\n" + "import time; time.sleep(120)\n" + ) + + @staticmethod + def _alive(marker: str) -> int: + found = subprocess.run( + ["pgrep", "-f", f"MARKER_{marker}"], capture_output=True, text=True + ).stdout + return len([line for line in found.split() if line.strip()]) + + def test_grandchild_is_reaped_at_the_cap(self, tmp_path, monkeypatch, real_interpreter): + marker = "pyautohands_groupkill" + monkeypatch.setattr(build_util, "TIMEOUT_SECS", 600) + script = _write_script(tmp_path, self._SPAWNS_GRANDCHILD.format(marker=marker)) + + report = RunReport(project="t", directory="d", run_type="script") + try: + execute_script( + str(script), + report=report, + env={**dict(PATH=os.environ.get("PATH", "")), "BUILD_SCRIPT_TIMEOUT": "2"}, + ) + assert report.results[0].status == Status.TIMEOUT + # The point of the group kill. Without it this is 1: the direct + # child is dead but its descendant runs on for its full 120s. + time.sleep(1) + assert self._alive(marker) == 0 + finally: + subprocess.run(["pkill", "-f", f"MARKER_{marker}"], capture_output=True) + + def test_run_capped_reports_the_output_captured_before_the_kill(self, tmp_path): + # The drain after the group kill still has to yield what the child + # printed -- killing the group is what lets that read reach EOF at all. + script = _write_script(tmp_path, "print('before the hang', flush=True)\nimport time\ntime.sleep(60)\n") + with pytest.raises(subprocess.TimeoutExpired) as excinfo: + build_util.run_capped( + [sys.executable, str(script)], + timeout=2, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + ) + assert "before the hang" in (excinfo.value.output or "") From a887fd96ed8aa229677aae01430d90a456454bd0 Mon Sep 17 00:00:00 2001 From: James Nightingale Date: Sun, 23 Aug 2026 18:48:22 +0000 Subject: [PATCH 2/2] build_util: genericise the run_capped docstring for the tenant firewall MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The docstring cited a satellite repo's issue by name. PyAutoHands is a framework organ and must stay adoptable as a config-diff fork, so repos_sync.py's tenant firewall rejects a new instance fact in organ code — correctly, and growing the allowlist for a comment would be the wrong fix. Rewritten to describe the mechanism rather than the incident: the leak this module actually has (grandchildren outliving the cap) and, separately, the uncapped-runner shape where the same group kill prevents a hang, noted as a workspace-side bug rather than named. No code change. Local check now passes: repos_sync.py --check --only "tenant firewall (organ code)" -> OK Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GTPM1RmMvSuMvJkEntAMv8 --- autohands/build_util.py | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/autohands/build_util.py b/autohands/build_util.py index 9b84f26..b8ea25c 100644 --- a/autohands/build_util.py +++ b/autohands/build_util.py @@ -109,15 +109,19 @@ def run_capped(args, timeout, check=False, stdout=None, stderr=None, to make: same `TimeoutExpired` and `CalledProcessError`, carrying the same captured `output`/`stderr`, so every caller's handling is unchanged. - The group is the point. `subprocess.run` kills only the direct child and - then reads the pipes to EOF -- but any grandchild that inherited stdout - holds that pipe open after the child dies, so the read blocks and the - timeout does not actually stop anything. A script whose work has finished - can therefore hang the runner indefinitely, which is how smoke CI came to - sit at the 6-hour GitHub Actions ceiling reporting nothing since the last - completed script (autolens_workspace_test#196). `start_new_session=True` - puts the child in its own group; killing the group closes the inherited - pipe and lets the read finish. + The group is the point. `subprocess.run` kills only the direct child, so a + grandchild -- a Popen'd helper, a multiprocessing worker, a compile server + -- outlives the cap and keeps running, holding whatever memory and devices + it had. Over a run of hundreds of scripts those accumulate against every + script that follows. `start_new_session=True` puts the child in its own + group, and killing the group takes its descendants with it. + + The same mechanism matters more for a runner that captures output without + a cap at all: there the parent waits for the stdout pipe to reach EOF, and + a grandchild holding that pipe open keeps the read blocked after the child + itself has exited -- a script whose work has finished hangs the runner + indefinitely. That shape is a workspace-side bug, not this module's, but + the fix is this same group kill. """ proc = subprocess.Popen( args,