ouyangbo/hermes-agent
1
0
Fork 0
Code Issues Pull Requests Actions 11 Packages Projects Releases Wiki Activity

Merge pull request #94188 from kshitijk4poor/fix/85125-4d-treekill-consolidation

Browse Source 
refactor(deadline): consolidate site-local tree-kills onto agent.deadline.kill_process_tree (#85125 4d)
This commit is contained in:
kshitij
2026-08-25 01:48:09 +05:30
committed by GitHub
parent c73d721b1d 547f985286
commit 8d29a55bed
5 changed files with 319 additions and 49 deletions
Download Patch File Download Diff File Expand all files Collapse all files
hermes_cli/_subprocess_compat.py
+30
View File
@@ -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
tests/agent/test_treekill_consolidation.py
+230
View File
@@ -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()
tests/tools/test_browser_npx_warmup.py
+13 -9
View File
@@ -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
tools/browser_tool.py
+19
View File
@@ -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(
tools/code_execution_tool.py
+27 -40
View File
@@ -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: