From b31c6c33b21d39dca2ed78faf682ff8e10d9483d Mon Sep 17 00:00:00 2001 From: paulb26 <275835513+paulb26@users.noreply.github.com> Date: Mon, 11 May 2026 23:27:48 -0400 Subject: [PATCH] fix(pty-bridge): terminate PTY process groups on teardown --- hermes_cli/pty_bridge.py | 14 +++++- tests/hermes_cli/test_pty_bridge.py | 70 +++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 2 deletions(-) diff --git a/hermes_cli/pty_bridge.py b/hermes_cli/pty_bridge.py index 511a3c39c81..f6217b93139 100644 --- a/hermes_cli/pty_bridge.py +++ b/hermes_cli/pty_bridge.py @@ -250,13 +250,23 @@ class PtyBridge: return self._closed = True + try: + pgid = os.getpgid(self._proc.pid) + except Exception: + pgid = None + # SIGHUP is the conventional "your terminal went away" signal. - # We escalate if the child ignores it. + # Send it to the whole foreground process group, not just the PTY + # leader: the dashboard TUI starts helper children such as the Python + # slash worker, and killing only the leader can strand those helpers. for sig in (signal.SIGHUP, signal.SIGTERM, signal.SIGKILL): # windows-footgun: ok — POSIX-only module (imports fcntl/termios/ptyprocess at top) if not self._proc.isalive(): break try: - self._proc.kill(sig) + if pgid is not None: + os.killpg(pgid, sig) + else: + self._proc.kill(sig) except Exception: pass deadline = time.monotonic() + 0.5 diff --git a/tests/hermes_cli/test_pty_bridge.py b/tests/hermes_cli/test_pty_bridge.py index 9ae007cc459..ed67cbb5e8b 100644 --- a/tests/hermes_cli/test_pty_bridge.py +++ b/tests/hermes_cli/test_pty_bridge.py @@ -8,6 +8,7 @@ from __future__ import annotations import os import shutil +import signal import sys import time @@ -211,6 +212,75 @@ class TestPtyBridgeClose: break assert reaped, f"pid {pid} still running after close()" + def test_close_signals_child_process_group(self, monkeypatch): + sent: list[tuple[int, signal.Signals]] = [] + + class _FakeProc: + pid = 12345 + fd = -1 + + def __init__(self): + self.alive = True + + def isalive(self): + return self.alive + + def kill(self, sig): + raise AssertionError(f"single-process kill used: {sig}") + + def close(self, force=False): + self.closed = force + + fake = _FakeProc() + + def fake_killpg(pgid, sig): + sent.append((pgid, sig)) + fake.alive = False + + monkeypatch.setattr(os, "getpgid", lambda pid: 67890) + monkeypatch.setattr(os, "killpg", fake_killpg) + + bridge = PtyBridge.__new__(PtyBridge) + bridge._proc = fake + bridge._fd = -1 + bridge._closed = False + + bridge.close() + + assert sent == [(67890, signal.SIGHUP)] + assert bridge._closed is True + + def test_close_falls_back_to_single_process_signal_when_group_unknown(self, monkeypatch): + sent: list[signal.Signals] = [] + + class _FakeProc: + pid = 12345 + fd = -1 + + def __init__(self): + self.alive = True + + def isalive(self): + return self.alive + + def kill(self, sig): + sent.append(sig) + self.alive = False + + def close(self, force=False): + self.closed = force + + monkeypatch.setattr(os, "getpgid", lambda pid: (_ for _ in ()).throw(OSError())) + + bridge = PtyBridge.__new__(PtyBridge) + bridge._proc = _FakeProc() + bridge._fd = -1 + bridge._closed = False + + bridge.close() + + assert sent == [signal.SIGHUP] + @skip_on_windows class TestPtyBridgeEnv: