diff --git a/nanobot/agent/tools/shell.py b/nanobot/agent/tools/shell.py index 6868a30d0..2c743c9dc 100644 --- a/nanobot/agent/tools/shell.py +++ b/nanobot/agent/tools/shell.py @@ -326,6 +326,7 @@ class ExecTool(Tool): prepared.env, prepared.shell_program, prepared.login, + process_tree=True, ) try: @@ -334,10 +335,10 @@ class ExecTool(Tool): timeout=prepared.timeout, ) except asyncio.TimeoutError: - await self._kill_process(process) + await self._kill_process_tree(process) return ToolResult.error(f"Error: Command timed out after {prepared.timeout} seconds") except asyncio.CancelledError: - await self._kill_process(process) + await self._kill_process_tree(process) raise # Safety-net reap: asyncio *should* have reaped the child via @@ -374,7 +375,7 @@ class ExecTool(Tool): # Kill and reap the child if it was spawned but an unexpected # error prevented communicate() from completing. if process is not None: - await self._kill_process(process) + await self._kill_process_tree(process) return ToolResult.error(f"Error executing command: {str(e)}") async def _execute_session( diff --git a/tests/tools/test_exec_platform.py b/tests/tools/test_exec_platform.py index cd759ad8e..c02638a17 100644 --- a/tests/tools/test_exec_platform.py +++ b/tests/tools/test_exec_platform.py @@ -302,7 +302,9 @@ class TestPathAppendPlatform: captured_cmd = None captured_env = {} - async def capture_spawn(cmd, cwd, env, shell_program=None, login=True): + async def capture_spawn( + cmd, cwd, env, shell_program=None, login=True, *, process_tree=False, + ): nonlocal captured_cmd captured_cmd = cmd captured_env.update(env) @@ -331,7 +333,9 @@ class TestPathAppendPlatform: captured_cmd = None captured_env = {} - async def capture_spawn(cmd, cwd, env, shell_program=None, login=True, *, stdin=None): + async def capture_spawn( + cmd, cwd, env, shell_program=None, login=True, *, stdin=None, process_tree=False, + ): nonlocal captured_cmd captured_cmd = cmd captured_env.update(env) @@ -359,7 +363,9 @@ class TestPathAppendPlatform: captured_cmd = None captured_env = {} - async def capture_spawn(cmd, cwd, env, shell_program=None, login=True, *, stdin=None): + async def capture_spawn( + cmd, cwd, env, shell_program=None, login=True, *, stdin=None, process_tree=False, + ): nonlocal captured_cmd captured_cmd = cmd captured_env.update(env) @@ -389,7 +395,9 @@ class TestPathAppendPlatform: captured_env = {} - async def capture_spawn(cmd, cwd, env, shell_program=None, login=True): + async def capture_spawn( + cmd, cwd, env, shell_program=None, login=True, *, process_tree=False, + ): captured_env.update(env) return mock_proc @@ -412,7 +420,9 @@ class TestPathAppendPlatform: captured_env = {} - async def capture_spawn(cmd, cwd, env, shell_program=None, login=True, *, stdin=None): + async def capture_spawn( + cmd, cwd, env, shell_program=None, login=True, *, stdin=None, process_tree=False, + ): captured_env.update(env) return mock_proc @@ -558,7 +568,9 @@ class TestExecuteEndToEnd: mock_proc.returncode = 0 captured_login = [] - async def capture_spawn(cmd, cwd, env, shell_program=None, login=None, *, stdin=None): + async def capture_spawn( + cmd, cwd, env, shell_program=None, login=None, *, stdin=None, process_tree=False, + ): captured_login.append(login) return mock_proc diff --git a/tests/tools/test_shell_reap.py b/tests/tools/test_shell_reap.py index 98b38c394..42a0eaa9e 100644 --- a/tests/tools/test_shell_reap.py +++ b/tests/tools/test_shell_reap.py @@ -3,6 +3,7 @@ from __future__ import annotations import asyncio +import shlex import sys from unittest.mock import AsyncMock, MagicMock, patch @@ -112,20 +113,37 @@ async def test_execute_timeout_kills_and_reaps(): mock_proc.pid = 1002 mock_proc.returncode = None mock_proc.communicate.side_effect = asyncio.TimeoutError() - mock_proc.kill = MagicMock() - mock_proc.wait = AsyncMock(return_value=-9) with ( - patch.object(ExecTool, "_spawn", return_value=mock_proc), + patch.object(ExecTool, "_spawn", return_value=mock_proc) as spawn, patch.object(ExecTool, "_guard_command", return_value=None), - patch("nanobot.agent.tools.shell._reap_pid") as reap, + patch.object(ExecTool, "_kill_process_tree", new_callable=AsyncMock) as kill_tree, ): tool = ExecTool(timeout=1) result = await tool.execute(command="sleep 99", timeout=1) assert "timed out" in result.lower() - mock_proc.kill.assert_called_once() - reap.assert_called_with(1002) + kill_tree.assert_awaited_once_with(mock_proc) + assert spawn.await_args.kwargs["process_tree"] is True + + +@pytest.mark.asyncio +async def test_execute_cancellation_kills_process_tree(): + mock_proc = AsyncMock() + mock_proc.pid = 1005 + mock_proc.returncode = None + mock_proc.communicate.side_effect = asyncio.CancelledError() + + with ( + patch.object(ExecTool, "_spawn", return_value=mock_proc) as spawn, + patch.object(ExecTool, "_guard_command", return_value=None), + patch.object(ExecTool, "_kill_process_tree", new_callable=AsyncMock) as kill_tree, + ): + with pytest.raises(asyncio.CancelledError): + await ExecTool().execute(command="sleep 99") + + kill_tree.assert_awaited_once_with(mock_proc) + assert spawn.await_args.kwargs["process_tree"] is True @pytest.mark.asyncio @@ -161,21 +179,35 @@ async def test_execute_exception_during_communicate_kills_live_process(): mock_proc.pid = 1004 mock_proc.returncode = None mock_proc.communicate.side_effect = OSError("pipe broken") - mock_proc.kill = MagicMock() - mock_proc.wait = AsyncMock(return_value=-1) with ( patch.object(ExecTool, "_spawn", return_value=mock_proc), patch.object(ExecTool, "_guard_command", return_value=None), - patch("nanobot.agent.tools.shell._reap_pid") as reap, + patch.object(ExecTool, "_kill_process_tree", new_callable=AsyncMock) as kill_tree, ): tool = ExecTool() result = await tool.execute(command="broken") assert "Error executing command" in result assert "pipe broken" in result - mock_proc.kill.assert_called_once() - reap.assert_called_with(1004) + kill_tree.assert_awaited_once_with(mock_proc) + + +@pytest.mark.skipif(sys.platform == "win32", reason="requires Unix process groups") +@pytest.mark.asyncio +async def test_execute_timeout_kills_background_process_tree(tmp_path): + """A one-shot timeout must stop background descendants before they write.""" + marker = tmp_path / "child-survived" + command = f"(sleep 2; touch {shlex.quote(str(marker))}) >/dev/null 2>&1 & sleep 30" + + result = await ExecTool(working_dir=str(tmp_path), timeout=1).execute( + command=command, + timeout=1, + ) + + assert "timed out" in result.lower() + await asyncio.sleep(2.5) + assert not marker.exists() def _mock_session_process(*, pid: int, returncode: int | None):