diff --git a/hermes_cli/pty_bridge.py b/hermes_cli/pty_bridge.py index 6e8963dc87..f71cfa08a0 100644 --- a/hermes_cli/pty_bridge.py +++ b/hermes_cli/pty_bridge.py @@ -90,6 +90,17 @@ def _process_group_exists(pgid: int) -> bool: return True +def _psutil_alive(proc) -> bool: + """Best-effort 'is this psutil Process still alive' for the shutdown cadence; + zombies count as dead and it never raises.""" + try: + import psutil # type: ignore + + return proc.is_running() and proc.status() != psutil.STATUS_ZOMBIE + except Exception: + return False + + class PtyBridge: """Thin wrapper around ``ptyprocess.PtyProcess`` for byte streaming. Not thread-safe: owned by the WebSocket handler that spawned it; reads run in an executor thread, writes are awaited on @@ -284,9 +295,22 @@ class PtyBridge: pgid = self._pgid leader_was_alive = self._proc.isalive() + # No group signal is safe for a shared-group child, so its descendants must be + # snapshotted BEFORE the parent is signalled: once the parent exits they reparent + # and psutil can no longer find them (browser_tool_lifecycle._legacy_kill_process_tree, + # process_registry._terminate_host_pid). The sweep below then kills them individually + # so a SIGHUP-ignoring helper cannot keep the PTY slave open (#76759). + non_leader_descendants: list = [] if pgid is not None and pgid != self._proc.pid: # Not a group leader: the child shares OUR process group, so killpg would # take the TUI down with it. Signal the child directly instead. + if leader_was_alive: + try: + import psutil # type: ignore + + non_leader_descendants = psutil.Process(self._proc.pid).children(recursive=True) + except Exception: + non_leader_descendants = [] pgid = None # Signal the whole process group, not just the PTY leader: the dashboard TUI starts helper @@ -323,6 +347,27 @@ class PtyBridge: except OSError: break # ESRCH: the group is empty self._wait_for_group_exit(pgid, grace if sig == signal.SIGHUP else 0.5) # windows-footgun: ok — POSIX-only module (imports fcntl/termios/ptyprocess at top) + elif non_leader_descendants: + # The shared-group branch: the helpers-outlive-leader sweep above cannot run + # (no killpg is safe), so end the snapshotted descendants individually. The main + # loop above signalled only the child PID, so the descendants got no SIGHUP — + # send it now and allow the helper grace, then SIGKILL survivors: a helper + # saving state on SIGHUP still finishes, and one that ignores SIGHUP cannot + # outlive the close (mirrors the group sweep's leader-dead cadence, #76759). + grace = _helper_shutdown_grace() + for sig in (signal.SIGHUP, signal.SIGKILL): # windows-footgun: ok — POSIX-only module (imports fcntl/termios/ptyprocess at top) + for child in non_leader_descendants: + try: + if sig == signal.SIGHUP: # windows-footgun: ok — POSIX-only module (imports fcntl/termios/ptyprocess at top) + child.send_signal(sig) + else: + child.kill() + except Exception: + pass # already gone; psutil raises NoSuchProcess/Zombie + if sig == signal.SIGHUP: # windows-footgun: ok — POSIX-only module (imports fcntl/termios/ptyprocess at top) + deadline = time.monotonic() + grace + while any(_psutil_alive(c) for c in non_leader_descendants) and time.monotonic() < deadline: + self._discard_output(0.02) try: self._proc.close(force=True) diff --git a/tests/hermes_cli/test_pty_bridge.py b/tests/hermes_cli/test_pty_bridge.py index 0bf3c005fa..7b88d7805e 100644 --- a/tests/hermes_cli/test_pty_bridge.py +++ b/tests/hermes_cli/test_pty_bridge.py @@ -12,6 +12,7 @@ import os import select import shutil import signal +import subprocess import sys import time @@ -342,6 +343,74 @@ class TestPtyBridgeClose: os.kill(helper, signal.SIGKILL) pytest.fail(f"helper pid {helper} survived close() after its leader died") + @pytest.mark.live_system_guard_bypass + def test_close_sweeps_helpers_of_a_non_leader_child(self): + # The group-leadership guard (above) signals a shared-group child one PID at a + # time, so the group sweep cannot run: no killpg is ever safe. A helper that + # ignores SIGHUP must still be snapshotted before its parent dies and killed + # individually, or it keeps the PTY slave open forever (#76759). + helper = None + + class _PopenBackedProc: + """Duck-typed ptyprocess handle around a plain Popen child that stays in + OUR process group (no start_new_session), exercising the non-leader branch.""" + + def __init__(self, popen): + self._popen = popen + self.pid = popen.pid + self.fd = -1 + + def isalive(self): + return self._popen.poll() is None + + def kill(self, sig): + self._popen.send_signal(sig) + + def close(self, force=False): + pass + + # The helper subshell ignores SIGHUP itself (trap is inherited by its sleep + # loop), so only the sweep's SIGKILL can end it. Both it and the leader loop + # forever; the leader dies on the first SIGHUP close() sends it. + script = ( + '/bin/sh -c \'trap "" HUP; while :; do sleep 0.05; done\' & ' + "echo helper=$!; while :; do sleep 0.05; done" + ) + popen = subprocess.Popen(["/bin/sh", "-c", script], stdout=subprocess.PIPE, text=True) + out = popen.stdout + assert out is not None + try: + line = "" + while "helper=" not in line: + line = out.readline() + if not line: + pytest.fail("leader exited before printing its helper pid") + helper = int(line.split("helper=")[1].strip()) + + bridge = PtyBridge.__new__(PtyBridge) + bridge._proc = _PopenBackedProc(popen) + bridge._fd = -1 + # recorded at spawn: the child was found in OUR group (pgid != pid) + bridge._pgid = os.getpgid(popen.pid) + bridge._closed = False + + bridge.close() + finally: + out.close() + if popen.poll() is None: + popen.kill() + popen.wait() + + deadline = time.monotonic() + 3.0 + while time.monotonic() < deadline: + try: + os.kill(helper, 0) + except ProcessLookupError: + return + time.sleep(0.05) + os.kill(helper, signal.SIGKILL) + pytest.fail(f"helper pid {helper} survived close() of its non-leader parent") + def test_close_lets_a_helper_finish_its_sighup_shutdown(self, tmp_path): # The TUI gateway saves its sessions on SIGHUP within a 1 s grace. Once the group got # SIGHUP, close() must wait for it, not re-signal or SIGKILL it early.