diff --git a/hermes_cli/_subprocess_compat.py b/hermes_cli/_subprocess_compat.py index 8a27dd3fca..86dbc2d8e1 100644 --- a/hermes_cli/_subprocess_compat.py +++ b/hermes_cli/_subprocess_compat.py @@ -412,6 +412,36 @@ def kill_process_tree(proc: "subprocess.Popen") -> None: handler and break that contract. The ``taskkill`` spawn itself cannot re-enter the deadlock class it fixes: it captures no pipes (DEVNULL), so its own timeout cleanup has no reader threads to join. + + Delegates the tree-kill to :func:`agent.deadline.kill_process_tree` + (#85125 4d) — same taskkill /T /F on Windows and killpg-when-leader on + POSIX, plus a psutil descendant sweep that also reaches descendants that + ``setsid``'d into their own sessions. On any import/delegation failure it + falls back to the original local implementation + (:func:`_legacy_kill_process_tree`), so the fail-open contract holds even + in stripped environments. + """ + try: + from agent.deadline import kill_process_tree as _deadline_kill_tree + + _deadline_kill_tree(proc.pid) + except Exception: + _legacy_kill_process_tree(proc) + return + # Ensure Popen's own bookkeeping sees the exit (matches the legacy body: + # a direct kill() so communicate()/wait() cannot hang on a stale handle). + try: + proc.kill() + except OSError: + pass + + +def _legacy_kill_process_tree(proc: "subprocess.Popen") -> None: + """Pre-#85125 local tree-kill — fallback when agent.deadline is unavailable. + + Kept verbatim so ``kill_process_tree`` can honor its swallow-everything + contract even when the delegation path itself fails (partial install, + import cycle during teardown). """ if not IS_WINDOWS: # Group-kill first: verify the child actually leads its own process diff --git a/tests/agent/test_treekill_consolidation.py b/tests/agent/test_treekill_consolidation.py new file mode 100644 index 0000000000..902629986c --- /dev/null +++ b/tests/agent/test_treekill_consolidation.py @@ -0,0 +1,230 @@ +"""#85125 Phase 4d — site-local tree-kills delegate to agent.deadline. + +Each migrated wrapper keeps its caller-facing contract (signature, all +failures swallowed, ``None`` return) while routing the actual tree +termination through :func:`agent.deadline.kill_process_tree`: + +* ``hermes_cli._subprocess_compat.kill_process_tree(proc)`` — also consumed + by ``agent.shell_hooks`` by name; falls back to + ``_legacy_kill_process_tree`` when delegation fails. +* ``tools.browser_tool._kill_process_tree(proc)`` — same pattern. +* ``tools.code_execution_tool._kill_process_group(proc, escalate=...)`` — + SIGTERM tree first, then (escalate) bounded wait + SIGKILL tree. + +Plus one real end-to-end probe: a setsid'd grandchild must die through the +compat wrapper (i.e. the psutil descendant sweep in agent.deadline is +actually reached from the delegating call site). +""" + +from __future__ import annotations + +import subprocess +import sys +import time +from unittest.mock import MagicMock + +import pytest + +import agent.deadline as deadline_mod + + +class _FakeProc: + def __init__(self, pid=54321): + self.pid = pid + self.kill_calls = 0 + + def kill(self): + self.kill_calls += 1 + + +# --------------------------------------------------------------------------- +# (1) hermes_cli._subprocess_compat.kill_process_tree +# --------------------------------------------------------------------------- + +class TestSubprocessCompatDelegation: + def test_delegates_with_proc_pid(self, monkeypatch): + from hermes_cli import _subprocess_compat + + calls = [] + monkeypatch.setattr( + deadline_mod, "kill_process_tree", lambda pid, **kw: calls.append(pid) or True + ) + proc = _FakeProc(pid=1111) + assert _subprocess_compat.kill_process_tree(proc) is None + assert calls == [1111] + + def test_swallows_delegation_raise_and_falls_back_to_legacy(self, monkeypatch): + from hermes_cli import _subprocess_compat + + def _boom(pid, **kw): + raise RuntimeError("delegation broken") + + monkeypatch.setattr(deadline_mod, "kill_process_tree", _boom) + legacy_calls = [] + monkeypatch.setattr( + _subprocess_compat, "_legacy_kill_process_tree", lambda proc: legacy_calls.append(proc) + ) + proc = _FakeProc(pid=2222) + _subprocess_compat.kill_process_tree(proc) # must not raise + assert legacy_calls == [proc] + + def test_shell_hooks_consumer_still_imports_by_name(self): + import agent.shell_hooks as shell_hooks + from hermes_cli import _subprocess_compat + + assert shell_hooks.kill_process_tree is _subprocess_compat.kill_process_tree + + def test_backcompat_alias_preserved(self): + from hermes_cli import _subprocess_compat + + assert ( + _subprocess_compat._kill_git_process_tree + is _subprocess_compat.kill_process_tree + ) + + +# --------------------------------------------------------------------------- +# (2) tools.browser_tool._kill_process_tree +# --------------------------------------------------------------------------- + +class TestBrowserToolDelegation: + def test_delegates_with_proc_pid(self, monkeypatch): + from tools import browser_tool + + calls = [] + monkeypatch.setattr( + deadline_mod, "kill_process_tree", lambda pid, **kw: calls.append(pid) or True + ) + proc = _FakeProc(pid=3333) + assert browser_tool._kill_process_tree(proc) is None + assert calls == [3333] + + def test_swallows_delegation_raise_and_falls_back_to_legacy(self, monkeypatch): + from tools import browser_tool + + def _boom(pid, **kw): + raise OSError("delegation broken") + + monkeypatch.setattr(deadline_mod, "kill_process_tree", _boom) + legacy_calls = [] + monkeypatch.setattr( + browser_tool, "_legacy_kill_process_tree", lambda proc: legacy_calls.append(proc) + ) + proc = _FakeProc(pid=4444) + browser_tool._kill_process_tree(proc) # must not raise + assert legacy_calls == [proc] + + +# --------------------------------------------------------------------------- +# (3) tools.code_execution_tool._kill_process_group +# --------------------------------------------------------------------------- + +class TestCodeExecutionDelegation: + def test_delegates_sigterm_tree_first(self, monkeypatch): + import signal as _signal + + from tools import code_execution_tool + + calls = [] + monkeypatch.setattr( + deadline_mod, + "kill_process_tree", + lambda pid, sig=None: calls.append((pid, sig)) or True, + ) + proc = _FakeProc(pid=5555) + code_execution_tool._kill_process_group(proc) + assert calls == [(5555, _signal.SIGTERM)] + + def test_escalate_waits_then_sigkills_tree(self, monkeypatch): + import signal as _signal + + from tools import code_execution_tool + + calls = [] + monkeypatch.setattr( + deadline_mod, + "kill_process_tree", + lambda pid, sig=None: calls.append((pid, sig)) or True, + ) + proc = MagicMock() + proc.pid = 6666 + proc.wait.side_effect = subprocess.TimeoutExpired(cmd="x", timeout=5) + code_execution_tool._kill_process_group(proc, escalate=True) + assert calls == [(6666, _signal.SIGTERM), (6666, _signal.SIGKILL)] + proc.wait.assert_called_once_with(timeout=5) + + def test_swallows_delegation_raise_falls_back_to_plain_kill(self, monkeypatch): + from tools import code_execution_tool + + def _boom(pid, **kw): + raise PermissionError("nope") + + monkeypatch.setattr(deadline_mod, "kill_process_tree", _boom) + proc = _FakeProc(pid=7777) + code_execution_tool._kill_process_group(proc) # must not raise + assert proc.kill_calls == 1 + + def test_even_proc_kill_raise_is_swallowed(self, monkeypatch): + from tools import code_execution_tool + + def _boom(pid, **kw): + raise PermissionError("nope") + + monkeypatch.setattr(deadline_mod, "kill_process_tree", _boom) + proc = MagicMock() + proc.pid = 8888 + proc.kill.side_effect = OSError("already reaped") + code_execution_tool._kill_process_group(proc) # must not raise + + +# --------------------------------------------------------------------------- +# End-to-end: setsid grandchild dies through the compat wrapper +# --------------------------------------------------------------------------- + +@pytest.mark.live_system_guard_bypass +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX session semantics") +def test_e2e_setsid_grandchild_killed_via_compat_wrapper(tmp_path): + """The delegating wrapper must reach descendants that setsid'd out of the + child's process group — the exact capability the shared primitive adds + over the old local killpg-only body (scaffold shape lifted from + tests/agent/test_deadline.py::test_kills_descendant_in_its_own_session). + """ + pytest.importorskip("psutil") + import psutil + + from hermes_cli._subprocess_compat import kill_process_tree + + started = tmp_path / "grandchild_started" + marker = tmp_path / "grandchild_survived" + grandchild_py = tmp_path / "grandchild.py" + grandchild_py.write_text( + "import pathlib, time\n" + f"pathlib.Path({str(started)!r}).write_text('x')\n" + "time.sleep(10)\n" + f"pathlib.Path({str(marker)!r}).write_text('x')\n" + ) + parent_py = tmp_path / "parent.py" + parent_py.write_text( + "import subprocess, sys, time\n" + f"subprocess.Popen([sys.executable, {str(grandchild_py)!r}], start_new_session=True)\n" + "time.sleep(10)\n" + ) + proc = subprocess.Popen([sys.executable, str(parent_py)], start_new_session=True) + try: + deadline = time.monotonic() + 10 + while not started.exists() and time.monotonic() < deadline: + time.sleep(0.05) + assert started.exists(), "grandchild never spawned — test harness broken" + # Snapshot the tree BEFORE the kill so we can assert zero survivors. + descendants = psutil.Process(proc.pid).children(recursive=True) + + kill_process_tree(proc) + + proc.wait(timeout=5) + gone, alive = psutil.wait_procs(descendants, timeout=5) + assert not alive, f"survivors after tree kill: {alive}" + time.sleep(1.0) + assert not marker.exists() + finally: + if proc.poll() is None: + proc.kill() diff --git a/tests/tools/test_browser_npx_warmup.py b/tests/tools/test_browser_npx_warmup.py index d51c9ed5ce..b866c43073 100644 --- a/tests/tools/test_browser_npx_warmup.py +++ b/tests/tools/test_browser_npx_warmup.py @@ -20,7 +20,7 @@ from unittest.mock import MagicMock, patch from tools.browser_tool import ( AGENT_BROWSER_NPX_SPEC, - _kill_process_tree, + _legacy_kill_process_tree, warm_agent_browser_npx_cache, ) @@ -216,7 +216,11 @@ def test_returns_false_instead_of_raising_on_unexpected_communicate_exception(): mock_kill.assert_called_once_with(proc) -class TestKillProcessTree: +class TestLegacyKillProcessTree: + """Contract of the pre-#85125 local fallback (used when agent.deadline + delegation fails); the delegating wrapper is covered in + tests/agent/test_treekill_consolidation.py.""" + def test_posix_kills_process_group_term_then_kill(self, monkeypatch): import signal @@ -229,7 +233,7 @@ class TestKillProcessTree: "os.killpg", lambda pgid, sig: killpg_calls.append((pgid, sig)) ) - _kill_process_tree(proc) + _legacy_kill_process_tree(proc) assert killpg_calls == [(999, signal.SIGTERM), (999, signal.SIGKILL)] @@ -243,7 +247,7 @@ class TestKillProcessTree: monkeypatch.setattr("os.getpgid", _raise) - _kill_process_tree(proc) # must not raise + _legacy_kill_process_tree(proc) # must not raise def test_posix_missing_killpg_attribute_falls_back_to_proc_kill(self, monkeypatch): """Some POSIX-like environments may lack os.killpg entirely (the @@ -260,7 +264,7 @@ class TestKillProcessTree: monkeypatch.setattr("os.name", "posix") monkeypatch.delattr(os_module, "killpg", raising=False) - _kill_process_tree(proc) + _legacy_kill_process_tree(proc) proc.kill.assert_called_once() @@ -273,7 +277,7 @@ class TestKillProcessTree: monkeypatch.setattr("os.name", "posix") monkeypatch.delattr(os_module, "killpg", raising=False) - _kill_process_tree(proc) # must not raise + _legacy_kill_process_tree(proc) # must not raise def test_posix_sigterm_permission_denied_does_not_attempt_sigkill(self, monkeypatch): """If SIGTERM itself is rejected (e.g. a stale pgid reused by an @@ -293,7 +297,7 @@ class TestKillProcessTree: monkeypatch.setattr("os.killpg", fake_killpg) - _kill_process_tree(proc) # must not raise + _legacy_kill_process_tree(proc) # must not raise assert killpg_calls == [(999, signal.SIGTERM)] @@ -302,7 +306,7 @@ class TestKillProcessTree: proc.pid = 4321 monkeypatch.setattr("os.name", "nt") with patch("subprocess.run") as mock_run: - _kill_process_tree(proc) + _legacy_kill_process_tree(proc) mock_run.assert_called_once() cmd = mock_run.call_args.args[0] @@ -313,4 +317,4 @@ class TestKillProcessTree: proc.pid = 4321 monkeypatch.setattr("os.name", "nt") with patch("subprocess.run", side_effect=OSError("taskkill missing")): - _kill_process_tree(proc) # must not raise + _legacy_kill_process_tree(proc) # must not raise diff --git a/tools/browser_tool.py b/tools/browser_tool.py index fc27d85946..dd97238f55 100644 --- a/tools/browser_tool.py +++ b/tools/browser_tool.py @@ -2637,7 +2637,26 @@ def _kill_process_tree(proc: "subprocess.Popen") -> None: orphaned). By the time this is called, the caller has already burned its full timeout budget waiting for a graceful exit — there's nothing to gain from waiting again here, only more delay on an already-timed-out call. + + Delegates to :func:`agent.deadline.kill_process_tree` (#85125 4d): same + ``taskkill /T /F`` on Windows and killpg-when-group-leader on POSIX, plus + a psutil descendant sweep that also reaches descendants that ``setsid``'d + into their own session (agent-browser's detached daemon grandchild). + SIGKILL-only instead of the old zero-grace SIGTERM→SIGKILL pair — the + grace period was already zero, so the observable effect is identical. + Any delegation failure falls back to the original local implementation + (:func:`_legacy_kill_process_tree`); never raises either way. """ + try: + from agent.deadline import kill_process_tree as _deadline_kill_tree + + _deadline_kill_tree(proc.pid) + except Exception: + _legacy_kill_process_tree(proc) + + +def _legacy_kill_process_tree(proc: "subprocess.Popen") -> None: + """Pre-#85125 local tree-kill — fallback when agent.deadline is unavailable.""" if os.name == "nt": try: subprocess.run( diff --git a/tools/code_execution_tool.py b/tools/code_execution_tool.py index f15b06223a..fe5366d0b0 100644 --- a/tools/code_execution_tool.py +++ b/tools/code_execution_tool.py @@ -1773,53 +1773,40 @@ def execute_code( def _kill_process_group(proc, escalate: bool = False): - """Kill the child and its entire process tree (cross-platform via psutil).""" - import psutil - try: - parent = psutil.Process(proc.pid) - children = parent.children(recursive=True) - for child in children: + """Kill the child and its entire process tree (cross-platform). + + Delegates to :func:`agent.deadline.kill_process_tree` (#85125 4d): + SIGTERM to the whole tree first (killpg when the child leads its own + group — it does, ``start_new_session=True`` — plus a psutil descendant + sweep for setsid'd grandchildren; ``taskkill /T /F`` on Windows). + With ``escalate=True`` the child gets 5s to exit after SIGTERM, then the + surviving tree is SIGKILLed — same escalation the old psutil-local body + implemented. Never raises; a delegation failure degrades to a plain + ``proc.kill()`` like the old psutil-failure fallback. + """ + import signal as _signal + + def _tree_signal(sig) -> None: + try: + from agent.deadline import kill_process_tree as _deadline_kill_tree + + _deadline_kill_tree(proc.pid, sig=sig) + except Exception as e: + logger.debug("Could not terminate process tree: %s", e, exc_info=True) try: - child.terminate() - except psutil.NoSuchProcess: - pass - try: - parent.terminate() - except psutil.NoSuchProcess: - pass - except psutil.NoSuchProcess: - pass - except (PermissionError, OSError) as e: - logger.debug("Could not terminate process tree: %s", e, exc_info=True) - try: - proc.kill() - except Exception as e2: - logger.debug("Could not kill process: %s", e2, exc_info=True) + proc.kill() + except Exception as e2: + logger.debug("Could not kill process: %s", e2, exc_info=True) + + # sig is ignored on Windows (taskkill /F is already forceful). + _tree_signal(getattr(_signal, "SIGTERM", None)) if escalate: # Give the process 5s to exit after SIGTERM, then SIGKILL try: proc.wait(timeout=5) except subprocess.TimeoutExpired: - try: - parent = psutil.Process(proc.pid) - for child in parent.children(recursive=True): - try: - child.kill() - except psutil.NoSuchProcess: - pass - try: - parent.kill() - except psutil.NoSuchProcess: - pass - except psutil.NoSuchProcess: - pass - except (PermissionError, OSError) as e: - logger.debug("Could not kill process tree: %s", e, exc_info=True) - try: - proc.kill() - except Exception as e2: - logger.debug("Could not kill process: %s", e2, exc_info=True) + _tree_signal(getattr(_signal, "SIGKILL", None)) def _load_config() -> dict: