From 8cbe58d08446e194443a9246b4392c4001daf48b Mon Sep 17 00:00:00 2001 From: KNambiarDJsc Date: Sat, 26 Sep 2026 19:30:15 +0530 Subject: [PATCH] tasksmith: stop the author adapter's whole process tree on Windows too run_external_agent cleaned up with os.killpg / signal.SIGKILL, neither of which exists on Windows, so a timeout or bridge failure while the adapter was still running raised AttributeError from the finally block (masking the real error) and left the adapter's children running. stop_process_tree keeps the POSIX behavior (SIGTERM/SIGKILL to the adapter's process group) and uses 'taskkill /T /F' on Windows. Both requests kill the whole tree there: terminate() alone orphans children, and taskkill /T cannot find them once the parent is gone. The other os.killpg sites (terminal/grade.py, swe_smith/grade.py, execution/job.py) run only inside the Linux verifier container / remote worker and are unchanged. Co-Authored-By: Claude Sonnet 5 --- .../tasksmith/author/external_agent.py | 32 +++++- tests/test_external_agent_process_tree.py | 106 ++++++++++++++++++ 2 files changed, 136 insertions(+), 2 deletions(-) create mode 100644 tests/test_external_agent_process_tree.py diff --git a/src/repo2rlenv/tasksmith/author/external_agent.py b/src/repo2rlenv/tasksmith/author/external_agent.py index d5253459..450d2f90 100644 --- a/src/repo2rlenv/tasksmith/author/external_agent.py +++ b/src/repo2rlenv/tasksmith/author/external_agent.py @@ -6,6 +6,8 @@ import os import shutil import signal +import subprocess +import sys from pathlib import Path from repo2rlenv.tasksmith.author.bridge import AgentBridge @@ -30,6 +32,32 @@ def runtime_path(engine: str) -> Path: return root / f"{engine}.mjs" +def stop_process_tree(process: asyncio.subprocess.Process, *, force: bool) -> None: + """End the adapter and everything it spawned. + + POSIX: the adapter leads its own session (``start_new_session``), so its pid + is a process-group id and one signal reaches every descendant. Windows has + neither ``os.killpg`` nor ``signal.SIGKILL`` and ignores + ``start_new_session``; ``taskkill /T`` walks the parent/child tree instead. + ``force=False`` is the polite first request (SIGTERM on POSIX), ``force=True`` + the last resort. Windows has no graceful group signal, and the tree can only + be found while the adapter is still alive (``terminate()`` alone would orphan + its children and ``taskkill /T`` cannot reach them afterwards), so both + requests end the whole tree at once there. + Raises ``ProcessLookupError`` on POSIX if the group is already gone. + """ + if sys.platform == "win32": + # A non-zero exit only means the tree is already gone. + subprocess.run( + ["taskkill", "/PID", str(process.pid), "/T", "/F"], + capture_output=True, + check=False, + timeout=15, + ) + return + os.killpg(process.pid, signal.SIGKILL if force else signal.SIGTERM) + + async def run_external_agent( *, engine, model, system, prompt, budget, tools, handlers, trace, max_turns, max_cost=8 ): @@ -118,12 +146,12 @@ async def run_external_agent( bridge.stop(cancel_models=not completed) if process is not None and process.returncode is None: with contextlib.suppress(ProcessLookupError): - os.killpg(process.pid, signal.SIGTERM) + stop_process_tree(process, force=False) try: await asyncio.wait_for(process.wait(), timeout=12) except TimeoutError: with contextlib.suppress(ProcessLookupError): - os.killpg(process.pid, signal.SIGKILL) + stop_process_tree(process, force=True) await process.wait() for task in (communication, failed): if task is not None and not task.done(): diff --git a/tests/test_external_agent_process_tree.py b/tests/test_external_agent_process_tree.py new file mode 100644 index 00000000..2c99ee15 --- /dev/null +++ b/tests/test_external_agent_process_tree.py @@ -0,0 +1,106 @@ +"""stop_process_tree must end the adapter *and* its descendants on every OS. + +`os.killpg` and `signal.SIGKILL` do not exist on Windows; the cleanup used to +raise AttributeError there (masking the real failure) and would have left the +adapter's children running. +""" + +from __future__ import annotations + +import asyncio +import sys +import time +from pathlib import Path + +import pytest + +from repo2rlenv.tasksmith.author.external_agent import stop_process_tree + +# The child starts a grandchild that touches a heartbeat file every 50 ms, then +# waits. If the tree is really gone, the heartbeat stops advancing. +_GRANDCHILD = ( + "import pathlib, sys, time\n" + "p = pathlib.Path(sys.argv[1])\n" + "while True:\n" + " p.write_text(str(time.time()))\n" + " time.sleep(0.05)\n" +) +_CHILD = ( + "import subprocess, sys, time\n" + f"subprocess.Popen([sys.executable, '-c', {_GRANDCHILD!r}, sys.argv[1]])\n" + "time.sleep(600)\n" +) + + +async def _start_tree(heartbeat: Path) -> asyncio.subprocess.Process: + process = await asyncio.create_subprocess_exec( + sys.executable, + "-c", + _CHILD, + str(heartbeat), + start_new_session=True, # what run_external_agent does; ignored on Windows + ) + deadline = time.monotonic() + 20 + while not heartbeat.exists(): + assert time.monotonic() < deadline, "grandchild never started" + await asyncio.sleep(0.05) + return process + + +async def _heartbeat_stopped(heartbeat: Path) -> bool: + """True once the file stops changing for a full second.""" + last, quiet_since = heartbeat.read_text(), time.monotonic() + deadline = time.monotonic() + 15 + while time.monotonic() < deadline: + await asyncio.sleep(0.1) + current = heartbeat.read_text() + if current != last: + last, quiet_since = current, time.monotonic() + elif time.monotonic() - quiet_since >= 1.0: + return True + return False + + +@pytest.mark.asyncio +async def test_force_stop_ends_the_adapter_and_its_descendants(tmp_path: Path) -> None: + heartbeat = tmp_path / "heartbeat" + process = await _start_tree(heartbeat) + try: + stop_process_tree(process, force=True) + await asyncio.wait_for(process.wait(), timeout=15) + assert await _heartbeat_stopped(heartbeat), "a descendant outlived the adapter" + finally: + if process.returncode is None: + process.kill() + await process.wait() + + +@pytest.mark.asyncio +async def test_polite_stop_then_force_stop_leaves_nothing_running(tmp_path: Path) -> None: + """The order run_external_agent uses: request, wait, then force.""" + heartbeat = tmp_path / "heartbeat" + process = await _start_tree(heartbeat) + try: + stop_process_tree(process, force=False) + await asyncio.wait_for(process.wait(), timeout=15) + # The polite request may leave a descendant behind on some platforms; + # the force pass that follows in run_external_agent must clean it up. + try: + stop_process_tree(process, force=True) + except ProcessLookupError: + pass + assert await _heartbeat_stopped(heartbeat), "a descendant outlived the cleanup" + finally: + if process.returncode is None: + process.kill() + await process.wait() + + +@pytest.mark.asyncio +async def test_stopping_an_already_finished_adapter_does_not_crash(tmp_path: Path) -> None: + process = await asyncio.create_subprocess_exec(sys.executable, "-c", "pass") + await process.wait() + try: + stop_process_tree(process, force=True) + except ProcessLookupError: + pass # the documented POSIX outcome; run_external_agent suppresses it