fix(pty-bridge): sweep helpers of a non-leader child on close

The group-leadership guard nulls a non-leader pgid so the shared group is
never killpg'd, but that also skipped the helpers-outlive-leader sweep
(#76759): a SIGHUP-ignoring helper of a shared-group child kept the PTY
slave open past close(). Snapshot the child's descendants with psutil
BEFORE the parent is signalled (they reparent and become undiscoverable
once it exits), then send SIGHUP with the helper grace and SIGKILL the
survivors individually — mirroring browser_tool_lifecycle's non-leader
branch and the group sweep's leader-dead cadence.
This commit is contained in:
Brooklyn Nicholson
2026-10-02 21:35:50 -05:00
committed by brooklyn!
parent 1190fabf8c
commit 293574bf14
2 changed files with 114 additions and 0 deletions

View File

@@ -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)

View File

@@ -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.