From 569fd58960b93040b333cf744c8f2adc152f71bd Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Fri, 2 Oct 2026 14:27:56 +0530 Subject: [PATCH] fix(dashboard): bound the PTY helper grace and await background closes - Cap the helper grace at 2.0 s (and ignore non-finite or <= 0 env values) and wait only 0.5 s after SIGKILL, so a raised HERMES_TUI_GATEWAY_SHUTDOWN_GRACE_S cannot push close_all() past the backend SIGTERM -> SIGKILL budget. - close_all() also awaits closes already running in the background (dead remnants, capacity evictions); the registry now holds those tasks, which also keeps them from being garbage-collected. - _discard_output sleeps on EOF instead of spinning on a readable dead master, and catches ValueError from select() like read() does. --- hermes_cli/dashboard_procs.py | 2 +- hermes_cli/pty_bridge.py | 24 +++++++++++++++--------- hermes_cli/pty_session.py | 14 +++++++++++--- tests/hermes_cli/test_pty_session.py | 17 +++++++++++++++++ 4 files changed, 44 insertions(+), 13 deletions(-) diff --git a/hermes_cli/dashboard_procs.py b/hermes_cli/dashboard_procs.py index 33b361d33e..5561ff2b42 100644 --- a/hermes_cli/dashboard_procs.py +++ b/hermes_cli/dashboard_procs.py @@ -451,7 +451,7 @@ def _kill_pids_windows(pids: list[int], killed: list[int], failed: list[tuple[in # SIGTERM → SIGKILL grace for the dashboard/serve backend. Must outlast the lifespan teardown in # hermes_cli/web_server.py::_lifespan: stop_hosted_room_service(timeout=5.0) + the startup-thread -# join(1.0) + PTY_REGISTRY.close_all() (concurrent; ≤ ~4.5s with the default helper grace). A SIGKILL inside +# join(1.0) + PTY_REGISTRY.close_all() (concurrent; ≤ ~4s per PTY, see pty_bridge._MAX_HELPER_SHUTDOWN_GRACE_S). A SIGKILL inside # that window skips close_all(), so the ui-tui / tui_gateway.entry children outlive the backend # and keep the deleted state.db-wal inode open — the next hermes start refuses with a FATAL # DeletedWalGenerationError (#111912). The orphan reaper's 1.5s (`_reap_orphaned_desktop_local_serves`) diff --git a/hermes_cli/pty_bridge.py b/hermes_cli/pty_bridge.py index 246ea4cbf9..653f780124 100644 --- a/hermes_cli/pty_bridge.py +++ b/hermes_cli/pty_bridge.py @@ -11,6 +11,7 @@ from __future__ import annotations import asyncio import errno import fcntl # windows-footgun: ok — POSIX-only module by design (see docstring) +import math import os import select import signal @@ -64,6 +65,9 @@ class PtyUnavailableError(RuntimeError): # Longer than the TUI gateway's own SIGHUP shutdown grace, so a helper that is saving state on # SIGHUP finishes before it is SIGKILLed. _HELPER_SHUTDOWN_GRACE_S = 1.5 +# Upper bound: PTY_REGISTRY.close_all() runs inside the backend's SIGTERM -> SIGKILL budget +# (dashboard_procs._POSIX_TERM_GRACE_SECONDS), so a raised gateway grace cannot stretch it. +_MAX_HELPER_SHUTDOWN_GRACE_S = 2.0 def _helper_shutdown_grace() -> float: @@ -72,7 +76,10 @@ def _helper_shutdown_grace() -> float: # tui_gateway.entry, which installs signal handlers. from utils import env_float - return max(_HELPER_SHUTDOWN_GRACE_S, env_float("HERMES_TUI_GATEWAY_SHUTDOWN_GRACE_S", 1.0) + 0.5) + gateway = env_float("HERMES_TUI_GATEWAY_SHUTDOWN_GRACE_S", 1.0) + if not math.isfinite(gateway) or gateway <= 0: + gateway = 1.0 + return min(max(_HELPER_SHUTDOWN_GRACE_S, gateway + 0.5), _MAX_HELPER_SHUTDOWN_GRACE_S) def _process_group_exists(pgid: int) -> bool: @@ -83,8 +90,6 @@ def _process_group_exists(pgid: int) -> bool: return True - - 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 @@ -253,13 +258,14 @@ class PtyBridge: def _discard_output(self, timeout: float) -> None: # Exiting children flush their pending terminal output first; on macOS a process stays in # exit (and a SIGHUP save outlasts the grace) until someone drains the master. The drain - # task has stopped reading by now, so read and drop while waiting. + # task no longer consumes the output, so read and drop while waiting. try: readable, _, _ = select.select([self._fd], [], [], timeout) - if readable: - os.read(self._fd, 65536) - except OSError: - time.sleep(timeout) + if readable and os.read(self._fd, 65536): + return + except (OSError, ValueError): + pass + time.sleep(timeout) # nothing read, EOF (b"") or EIO: don't spin on a readable dead fd def _wait_for_group_exit(self, pgid: int, timeout: float) -> None: deadline = time.monotonic() + timeout @@ -312,7 +318,7 @@ class PtyBridge: os.killpg(pgid, sig) # windows-footgun: ok — POSIX-only module (imports fcntl/termios/ptyprocess at top) except OSError: break # ESRCH: the group is empty - self._wait_for_group_exit(pgid, grace) + 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) try: self._proc.close(force=True) diff --git a/hermes_cli/pty_session.py b/hermes_cli/pty_session.py index 092d015175..a34f8da4cf 100644 --- a/hermes_cli/pty_session.py +++ b/hermes_cli/pty_session.py @@ -183,6 +183,9 @@ class PtySessionRegistry: # already serialized globally for the same reason, and a spawn only # delays NEW chats. Per-key locks if spawn throughput ever matters. self._attach_lock = asyncio.Lock() + # Sessions popped from the registry but still closing in the background; close_all() + # awaits them too, and holding the tasks keeps them from being garbage-collected. + self._background_closes: set[asyncio.Task] = set() async def attach_or_spawn(self, key: str, *, spawn: Callable[[], object]) -> Tuple[PtySession, bool]: await self.reap_idle() @@ -194,7 +197,7 @@ class PtySessionRegistry: # Close in the background: ending a dead leader's helpers can take the helper # grace, and this lock serializes every new chat. self._sessions.pop(key, None) - asyncio.create_task(existing.close()) + self._close_in_background(existing) if len(self._sessions) >= self._max: self._reap_one_idle_or_raise() # PTY spawn does blocking fork/exec work — keep it off the event loop. @@ -256,10 +259,15 @@ class PtySessionRegistry: raise RegistryFull() oldest = min(idle, key=lambda s: s.last_detached_at or 0.0) self._sessions.pop(oldest.key, None) - asyncio.create_task(oldest.close()) + self._close_in_background(oldest) + + def _close_in_background(self, session: "PtySession") -> None: + task = asyncio.create_task(session.close()) + self._background_closes.add(task) + task.add_done_callback(self._background_closes.discard) async def close_all(self) -> None: # Close concurrently: each close() may wait out its helpers' SIGHUP grace, and shutdown runs # under the backend's SIGTERM -> SIGKILL budget (dashboard_procs._POSIX_TERM_GRACE_SECONDS). sessions = [self._sessions.pop(key) for key in list(self._sessions)] - await asyncio.gather(*(s.close() for s in sessions), return_exceptions=True) + await asyncio.gather(*(s.close() for s in sessions), *self._background_closes, return_exceptions=True) diff --git a/tests/hermes_cli/test_pty_session.py b/tests/hermes_cli/test_pty_session.py index 008f447edd..37d2cb2438 100644 --- a/tests/hermes_cli/test_pty_session.py +++ b/tests/hermes_cli/test_pty_session.py @@ -452,6 +452,23 @@ async def test_close_all_does_not_wait_on_one_slow_close_before_the_next(): assert all(b.closed for b in bridges) +@pytest.mark.asyncio +async def test_close_all_waits_for_closes_already_running_in_the_background(): + """A dead remnant or evicted session leaves the registry before its close() finishes; the + backend's teardown must still wait for it, or its helpers outlive the backend.""" + reg = make_registry(ttl=60.0) + bridges, entered, release = await _two_idle_sessions_first_close_gated(reg) + reg._close_in_background(reg._sessions.pop("k0")) + await entered.wait() # k0 is closing outside the registry + + closer = asyncio.create_task(reg.close_all()) + await asyncio.sleep(0.05) + assert not closer.done() # still waiting on k0 + release.set() + await closer + assert all(b.closed for b in bridges) + + @pytest.mark.asyncio async def test_close_other_sessions_removes_old_profile_session(): from hermes_cli.pty_session import WS_CLOSE_SUPERSEDED, PtySession