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

refactor(deadline): consolidate site-local tree-kills onto agent.deadline.kill_process_tree (#85125 4d)

Browse Source 
Per-site decisions:

1. hermes_cli/_subprocess_compat.py kill_process_tree(proc) -> None:
   MIGRATED. Body now delegates to agent.deadline.kill_process_tree(proc.pid)
   via a function-local import; keeps the swallow-everything fail-open
   contract and the (proc) -> None signature (agent/shell_hooks.py imports
   it by name; _kill_git_process_tree alias preserved). The old body is kept
   verbatim as _legacy_kill_process_tree and used as fallback when the
   delegation import/call fails. A final proc.kill() is retained on the
   happy path so Popen bookkeeping sees the exit (matches old behavior).

2. tools/browser_tool.py _kill_process_tree(proc): MIGRATED, same pattern
   (delegate + _legacy_kill_process_tree fallback). Behavior delta: the old
   body sent SIGTERM then SIGKILL with zero grace between them; the shared
   primitive sends SIGKILL only. With no grace period the observable effect
   is identical, and the psutil descendant sweep now also reaches
   agent-browser's setsid'd daemon grandchild, which killpg alone missed.
   tests/tools/test_browser_npx_warmup.py's TestKillProcessTree repointed at
   the legacy fallback (its assertions describe the fallback's internals).

3. tools/code_execution_tool.py _kill_process_group(proc, escalate):
   MIGRATED. It was a plain parent+descendants terminate (then wait 5s +
   kill when escalate=True) — expressed as two delegated calls:
   kill_process_tree(pid, sig=SIGTERM), then on escalate-timeout
   kill_process_tree(pid, sig=SIGKILL). Delegation failure degrades to
   proc.kill(), mirroring the old psutil-failure fallback. Delta: the old
   body terminated children before the parent; the shared primitive
   signals the group atomically (child is a session leader via
   start_new_session=True) plus an identity-aware descendant sweep —
   strictly wider coverage, same signals.

4. gateway/status.py: KEPT BOTH SITES.
   - terminate_pid (~l305) taskkill wrapper: NOT migrated. Its contract is
     incompatible with the shared primitive — it must RAISE OSError with
     taskkill's stderr on non-zero exit (callers branch on that), falls back
     to os.kill on FileNotFoundError, and its POSIX branch is deliberately a
     single-PID SIGTERM/SIGKILL, not a tree kill. Wrapping the bool-returning
     fail-soft primitive would invert the error contract.
   - reap_gateway_children (~l2029): NOT migrated. It operates on a
     pre-snapshotted child list from a parent that is already dead
     (psutil.Process(pid) on the parent would fail), and every signal is
     wrapped in identity/ownership checks the primitive lacks: is_running()
     identity, zombie skip, and the skip-if-ppid-still-equals-parent guard,
     plus SIGTERM -> wait_procs -> SIGKILL staging and a reaped-count return.
     The coupling is the feature; migrating would delete the safety logic.

5. scripts/run_tests_parallel.py _kill_process_tree (~l253): NOT migrated.
   Dev tooling that intentionally kills by CAPTURED pgid because the direct
   child is usually already reaped (psutil/pid-based primitive cannot find
   it), and it avoids the psutil import on the test-runner hot path. Its
   docstring already documents why psutil is the wrong tool there.

New tests: tests/agent/test_treekill_consolidation.py — delegation +
raise-swallowing tests per migrated wrapper, consumer-identity checks, and
a live end-to-end probe (setsid grandchild dies through the compat wrapper,
zero survivors).
This commit is contained in:
kshitijk4poor
2026-08-24 17:31:48 +05:30
parent b9eb37e5eb
commit 547f985286
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: