Merge pull request #94187 from kshitijk4poor/fix/85125-4b-terminal-treekill

fix(terminal): sweep setsid descendants after local timeout group-kill (#85125 4b)

tests/tools/test_local_setsid_descendant_sweep.py

@@ -0,0 +1,148 @@
+"""Regression tests for #85125 Phase 4b (terminal flavor of the #71148 class).
+
+LocalEnvironment._kill_process kills the process GROUP (SIGTERM -> wait ->
+SIGKILL).  A descendant that called ``setsid`` escapes the group and survives
+the group-kill — the local sibling of issue #84967.  The fix snapshots the
+descendant set via psutil BEFORE the first signal (children reparent to init
+after the parent dies, so a later parent walk finds nothing — same rationale
+as agent/deadline.py kill_process_tree) and sweeps any snapshotted survivor
+outside the (now-dead) group with SIGKILL afterwards.
+"""
+
+import os
+import signal
+import textwrap
+import time
+from types import SimpleNamespace
+
+import pytest
+
+from tools.environments.local import LocalEnvironment
+
+
+@pytest.fixture(autouse=True)
+def _isolate_hermes_home(tmp_path, monkeypatch):
+    monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+    (tmp_path / "logs").mkdir(exist_ok=True)
+
+
+def _pid_alive(pid: int) -> bool:
+    try:
+        os.kill(pid, 0)
+        return True
+    except ProcessLookupError:
+        return False
+    except PermissionError:
+        return True
+
+
+def _wait_for_pid_exit(pid: int, timeout: float = 10.0) -> bool:
+    deadline = time.monotonic() + timeout
+    while time.monotonic() < deadline:
+        if not _pid_alive(pid):
+            return True
+        time.sleep(0.1)
+    return not _pid_alive(pid)
+
+
+@pytest.mark.live_system_guard_bypass
+def test_timeout_kill_reaps_setsid_grandchild(tmp_path):
+    """A grandchild that setsid's out of the group must not survive the
+    timeout kill path."""
+    pytest.importorskip("psutil")
+
+    pid_file = tmp_path / "grandchild.pid"
+    script = textwrap.dedent(
+        """
+        import os, sys, time
+        pid = os.fork()
+        if pid == 0:
+            os.setsid()  # escape the command's process group/session
+            with open(sys.argv[1], "w") as f:
+                f.write(str(os.getpid()))
+            time.sleep(30)
+            os._exit(0)
+        time.sleep(30)
+        """
+    ).strip()
+
+    env = LocalEnvironment(cwd=str(tmp_path))
+    try:
+        import sys as _sys
+
+        cmd = f"{_sys.executable} -c {_sh_quote(script)} {_sh_quote(str(pid_file))}"
+        result = env.execute(cmd, timeout=3)
+
+        # The command must have hit the timeout/kill path.
+        assert "timed out" in result.get("output", "").lower() or result.get(
+            "returncode"
+        ) not in (0,), f"expected timeout, got: {result!r}"
+
+        # The grandchild wrote its pid before the kill.
+        deadline = time.monotonic() + 5.0
+        while time.monotonic() < deadline and not pid_file.exists():
+            time.sleep(0.05)
+        assert pid_file.exists(), "grandchild never wrote its pid file"
+        grandchild_pid = int(pid_file.read_text().strip())
+
+        assert _wait_for_pid_exit(grandchild_pid), (
+            f"setsid grandchild {grandchild_pid} SURVIVED the timeout "
+            f"group-kill — the #84967/#71148 orphan class (terminal flavor). "
+            f"_kill_process must sweep snapshotted descendants outside the "
+            f"group after the group-kill."
+        )
+    finally:
+        # Belt and braces: never leak the sleeper into the test host.
+        try:
+            if pid_file.exists():
+                os.kill(int(pid_file.read_text().strip()), signal.SIGKILL)
+        except (OSError, ValueError):
+            pass
+        try:
+            env.cleanup()
+        except Exception:
+            pass
+
+
+def _sh_quote(s: str) -> str:
+    import shlex
+
+    return shlex.quote(s)
+
+
+def test_kill_process_survives_psutil_snapshot_failure(monkeypatch):
+    """A broken psutil snapshot must never break the kill path — the
+    group-kill escalation still runs to completion."""
+    psutil = pytest.importorskip("psutil")
+
+    env = object.__new__(LocalEnvironment)
+    proc = SimpleNamespace(
+        pid=12345,
+        _hermes_pgid=67890,
+        poll=lambda: 0,
+        wait=lambda timeout=None: 0,
+        kill=lambda: None,
+    )
+    killpg_calls = []
+
+    def fake_getpgid(_pid):
+        return 67890
+
+    def fake_killpg(pgid, sig):
+        killpg_calls.append((pgid, sig))
+        if sig == 0:
+            raise ProcessLookupError  # group is gone after the first signal
+
+    def boom(*_a, **_k):
+        raise RuntimeError("psutil exploded")
+
+    monkeypatch.setattr(os, "getpgid", fake_getpgid)
+    monkeypatch.setattr(os, "killpg", fake_killpg)
+    monkeypatch.setattr(psutil, "Process", boom)
+
+    env._kill_process(proc)  # must not raise
+
+    # SIGTERM was delivered to the group and the alive-probe ran: the
+    # escalation path completed despite the snapshot failure.
+    assert killpg_calls[0] == (67890, signal.SIGTERM)
+    assert (67890, 0) in killpg_calls

tools/environments/local.py

@@ -1907,27 +1907,72 @@ class LocalEnvironment(BaseEnvironment):
                     if pgid is None:
                         raise
 
+                # Snapshot the descendant set BEFORE the first signal: once
+                # the wrapper dies its children reparent to init and a parent
+                # walk finds nothing (same rationale as agent/deadline.py
+                # kill_process_tree).  A descendant that called ``setsid``
+                # escapes the process group entirely and would survive the
+                # group-kill below — the #71148 class, terminal flavor
+                # (issue #84967's local sibling).  The snapshot must never
+                # break the kill path, so any failure just yields an empty
+                # sweep set.
+                descendants: list = []
+                try:
+                    import psutil
+
+                    descendants = psutil.Process(proc.pid).children(recursive=True)
+                except Exception:
+                    descendants = []
+
+                def _sweep_escaped_descendants() -> None:
+                    """SIGKILL snapshotted survivors outside the (dead) group.
+
+                    Runs after the TERM→KILL group escalation so in-group
+                    members keep their SIGTERM grace window; only escapees
+                    (own setsid sessions) are force-killed.  psutil's
+                    identity-aware Process means recycled PIDs are skipped.
+
+                    POSIX-only: reached solely from the non-_IS_WINDOWS
+                    branch above (the win32 path returns earlier).
+                    """
+                    for child in descendants:
+                        try:
+                            if not child.is_running():
+                                continue
+                            try:
+                                if os.getpgid(child.pid) == pgid:
+                                    continue  # group-kill already covers it
+                            except (ProcessLookupError, PermissionError, OSError):
+                                pass
+                            child.kill()
+                        except Exception:
+                            continue
+
                 try:
                     os.killpg(pgid, signal.SIGTERM)  # windows-footgun: ok — POSIX process-group SIGTERM (guarded by _IS_WINDOWS above)
                 except ProcessLookupError:
+                    _sweep_escaped_descendants()
                     return
 
                 # Wait on the process group, not just the shell wrapper. Under
                 # load the wrapper can exit before grandchildren do; returning
                 # at that point leaves orphaned process-group members behind.
                 if _wait_for_group_exit(pgid, 1.0):
+                    _sweep_escaped_descendants()
                     return
 
                 try:
                     # POSIX-only: _IS_WINDOWS is handled by the outer branch.
                     os.killpg(pgid, signal.SIGKILL)  # windows-footgun: ok — POSIX process-group SIGKILL
                 except ProcessLookupError:
+                    _sweep_escaped_descendants()
                     return
                 _wait_for_group_exit(pgid, 2.0)
                 try:
                     proc.wait(timeout=0.2)
                 except (subprocess.TimeoutExpired, OSError):
                     pass
+                _sweep_escaped_descendants()
         except (ProcessLookupError, PermissionError, OSError):
             try:
                 proc.kill()