diff --git a/clawbench/environment.py b/clawbench/environment.py index 2d0e753..e13cd8e 100644 --- a/clawbench/environment.py +++ b/clawbench/environment.py @@ -5,11 +5,14 @@ import asyncio import json import logging +import os import re +import sys from pathlib import Path from typing import Any from clawbench.client import GatewayClient +from clawbench.environment_files import _execution_subprocess_kwargs, _reap_timed_out_process from clawbench.paths import resolve_workspace_path from clawbench.render import render_argv_template, render_shell_template, render_template, render_value from clawbench.schemas import ( @@ -128,8 +131,6 @@ async def run_execution_check( reason=str(exc), ) rendered_env = render_value(spec.env, runtime_values) - import os - import sys full_env = { **os.environ, @@ -145,6 +146,7 @@ async def run_execution_check( full_env["PYTHONPATH"] = ":".join(python_path_parts) try: + spawn_kwargs = _execution_subprocess_kwargs() if spec.shell: process = await asyncio.create_subprocess_shell( rendered_command, @@ -152,6 +154,7 @@ async def run_execution_check( env=full_env, stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE, + **spawn_kwargs, ) else: process = await asyncio.create_subprocess_exec( @@ -160,14 +163,14 @@ async def run_execution_check( env=full_env, stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE, + **spawn_kwargs, ) stdout_bytes, stderr_bytes = await asyncio.wait_for( process.communicate(), timeout=spec.timeout_seconds, ) except asyncio.TimeoutError: - process.kill() - await process.communicate() + await _reap_timed_out_process(process, spec.timeout_seconds) return ExecutionCheckResult( name=spec.name, command=rendered_command, diff --git a/clawbench/environment_files.py b/clawbench/environment_files.py index 07d2bb3..a693cdb 100644 --- a/clawbench/environment_files.py +++ b/clawbench/environment_files.py @@ -19,6 +19,8 @@ import logging import os import re +import signal +import subprocess import sys from pathlib import Path from typing import Any @@ -92,6 +94,54 @@ def verify_file_state( # --------------------------------------------------------------------------- +def _execution_subprocess_kwargs() -> dict[str, Any]: + if sys.platform == "win32": + return {} + return {"start_new_session": True} + + +def _kill_execution_pgroup(process: asyncio.subprocess.Process) -> None: + """Signal the process group so shell-spawned children do not keep pipes open.""" + if process.pid is None: + return + if sys.platform == "win32": + subprocess.run( + ["taskkill", "/F", "/T", "/PID", str(process.pid)], + check=False, + capture_output=True, + ) + try: + process.kill() + except ProcessLookupError: + pass + return + try: + os.killpg(os.getpgid(process.pid), signal.SIGKILL) + return + except (ProcessLookupError, PermissionError, OSError): + pass + try: + process.kill() + except ProcessLookupError: + pass + + +async def _reap_timed_out_process( + process: asyncio.subprocess.Process, timeout_seconds: float +) -> None: + _kill_execution_pgroup(process) + try: + await asyncio.wait_for( + process.communicate(), + timeout=max(1.0, float(timeout_seconds)), + ) + except (asyncio.TimeoutError, ProcessLookupError, OSError): + try: + process.kill() + except ProcessLookupError: + pass + + async def run_execution_check( spec: ExecutionCheck, *, @@ -135,6 +185,7 @@ async def run_execution_check( full_env["PYTHONPATH"] = ":".join(python_path_parts) try: + spawn_kwargs = _execution_subprocess_kwargs() if spec.shell: process = await asyncio.create_subprocess_shell( rendered_command, @@ -142,6 +193,7 @@ async def run_execution_check( env=full_env, stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE, + **spawn_kwargs, ) else: process = await asyncio.create_subprocess_exec( @@ -150,14 +202,14 @@ async def run_execution_check( env=full_env, stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE, + **spawn_kwargs, ) stdout_bytes, stderr_bytes = await asyncio.wait_for( process.communicate(), timeout=spec.timeout_seconds, ) except asyncio.TimeoutError: - process.kill() - await process.communicate() + await _reap_timed_out_process(process, spec.timeout_seconds) return ExecutionCheckResult( name=spec.name, command=rendered_command, diff --git a/tests/test_execution_shell_rendering.py b/tests/test_execution_shell_rendering.py index 7d40cd7..3732ab5 100644 --- a/tests/test_execution_shell_rendering.py +++ b/tests/test_execution_shell_rendering.py @@ -1,3 +1,7 @@ +import asyncio +import os +import signal +import subprocess import sys from pathlib import Path @@ -131,3 +135,74 @@ async def test_shell_execution_check_preserves_single_quoted_placeholders( ) assert result.passed is True + + +def _write_pipe_holder(path: Path) -> None: + path.write_text( + "import subprocess\n" + "import sys\n" + "import time\n" + "from pathlib import Path\n" + "\n" + "child = subprocess.Popen(\n" + " [sys.executable, '-c', 'import time; time.sleep(120)']\n" + ")\n" + "Path(sys.argv[1]).write_text(str(child.pid), encoding='utf-8')\n" + "time.sleep(120)\n", + encoding="utf-8", + ) + + +def _force_kill_pid(pid: int) -> None: + if sys.platform == "win32": + subprocess.run( + ["taskkill", "/F", "/T", "/PID", str(pid)], + check=False, + capture_output=True, + ) + return + try: + os.kill(pid, signal.SIGKILL) + except OSError: + pass + + +@pytest.mark.asyncio +@pytest.mark.parametrize("run_execution_check", RUNNERS) +async def test_execution_check_timeout_reaps_shell_child_process_group( + tmp_path: Path, + run_execution_check, +): + _write_pipe_holder(tmp_path / "hold_pipe.py") + pid_file = tmp_path / "child.pid" + try: + try: + result = await asyncio.wait_for( + run_execution_check( + ExecutionCheck( + name="timeout-reap", + command="python hold_pipe.py child.pid", + timeout_seconds=1, + ), + workspace=tmp_path, + runtime_values={}, + ), + timeout=8, + ) + except TimeoutError: + pytest.fail( + "run_execution_check hung after timeout_seconds; " + "child still held stdout/stderr" + ) + + assert result.passed is False + assert result.exit_code == -1 + assert result.reason == "Timed out after 1s" + assert pid_file.exists() + child_pid = int(pid_file.read_text(encoding="utf-8").strip()) + if sys.platform != "win32": + with pytest.raises(OSError): + os.kill(child_pid, 0) + finally: + if pid_file.exists(): + _force_kill_pid(int(pid_file.read_text(encoding="utf-8").strip()))