From f3bdcd08776e01df58f54f1b0ecbcc854ccaaa28 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Wed, 16 Sep 2026 22:12:09 -0700 Subject: [PATCH] fix: dashboard stop sweeps the wedged descendants that outlive the SIGKILLed backend MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `hermes dashboard --stop` / `hermes update` signal only the backend PIDs. When the lifespan teardown wedges (stop_hosted_room_service, PTY close_all never reached) the 10s grace loses, SIGKILL lands mid-teardown, and the hosted ui-tui / tui_gateway.entry child is reparented to init still holding the state.db-wal inode; the next start refuses with DeletedWalGenerationError (#112631, residual of #111912). No finite root grace covers an unbounded teardown. _kill_pids_posix now snapshots the dashboard-owned descendant tree BEFORE the kill (the PPID link is gone once the root dies), and after the root phase SIGTERM→SIGKILLs the descendants that are still alive, waiting for the tree to be gone before returning. Descendants are re-checked against their snapshotted start-time fingerprint (the same PID-reuse guard _kill_pids_windows uses) — no `ps -o lstart` per system PID. Detached session leaders without a controlling terminal are pruned from the sweep with their subtrees: those are the messaging-gateway bots and profile actions the dashboard launched with start_new_session from /api/gateway/*, which belong to the user, not the dashboard. A hosted TUI is a session leader too (pty.fork) but owns the pts whose master the dashboard held, so its tty column is set and it is swept. The Desktop boot reaper (_reap_orphaned_desktop_local_serves) SIGKILLs the same class of backend after 1.5s and had the same hole; it now SIGKILLs the surviving snapshotted descendants (no second grace: the boot path runs under a 10s probe). Live: pyteman hermes-111912 wedged leg on origin/main `child_orphan_alive=True deleted_sidecar_holders=2 guard=FATAL DeletedWalGenerationError`, on this head `child_orphan_alive=False deleted_sidecar_holders=0 guard=clean`; the start_new_session control sibling survives on both. --- hermes_cli/dashboard_procs.py | 103 ++++++++++++++++-- .../test_dashboard_procs_kill_grace.py | 81 +++++++++++++- website/docs/reference/cli-commands.md | 2 +- 3 files changed, 173 insertions(+), 13 deletions(-) diff --git a/hermes_cli/dashboard_procs.py b/hermes_cli/dashboard_procs.py index 0a0b316926..6d313cc75d 100644 --- a/hermes_cli/dashboard_procs.py +++ b/hermes_cli/dashboard_procs.py @@ -286,15 +286,84 @@ def _kill_pids_windows(pids: list[int], killed: list[int], failed: list[tuple[in # DeletedWalGenerationError (#111912). The orphan reaper's 1.5s (`_reap_orphaned_desktop_local_serves`) # is deliberately shorter: it runs on the Desktop boot path under a 10s ready-probe. _POSIX_TERM_GRACE_SECONDS = 10.0 +# Grace for a descendant that outlived the backend's own teardown. It already got the backend's +# SIGTERM forwarded (or SIGHUP from its PTY master closing); anything still up is wedged, and a +# wedged ui-tui keeps the deleted state.db-wal inode open until the next start refuses with +# DeletedWalGenerationError (#112631) — no finite root grace can cover an unbounded teardown. +_POSIX_DESCENDANT_GRACE_SECONDS = 2.0 +_NO_TTY = ("?", "??", "-") # Linux / macOS / BSD spellings of "no controlling terminal" -def _kill_pids_posix(pids: list[int], killed: list[int], failed: list[tuple[int, str]]) -> None: - """SIGTERM, wait up to ``_POSIX_TERM_GRACE_SECONDS`` for graceful exit, SIGKILL survivors.""" - import signal as _signal +def _is_detached_session_leader(pid: int, tty: str) -> bool: + """True for a process the dashboard launched with ``start_new_session`` (own session, no tty). + + Messaging-gateway bots and profile actions started from ``/api/gateway/*`` are such processes: + they are the user's, not the dashboard's, and must survive a dashboard stop. A hosted + ``hermes --tui`` child is a session leader too (``pty.fork``) but owns the pts whose master the + dashboard held, so its tty column is set and it stays in the sweep. + """ + if tty not in _NO_TTY: + return False + try: + return os.getsid(pid) == pid + except OSError: + return False + + +def _posix_descendants(roots: list[int]) -> dict[int, int | None]: + """``{pid: start_time}`` of every dashboard-owned descendant of *roots*, snapshotted BEFORE the + kill: once the root dies its children are reparented and the PPID link is gone. Detached session + leaders (see ``_is_detached_session_leader``) are pruned together with their own subtrees. The + start-time fingerprint is the PID-reuse guard (same one ``_kill_pids_windows`` uses). + Empty on scan failure → root-only kill, the historical behaviour. + """ + from gateway.status import get_process_start_time + try: + result = subprocess.run(["ps", "-A", "-o", "pid=,ppid=,tty="], timeout=10, **_PS_RUN_KWARGS) + except (FileNotFoundError, subprocess.TimeoutExpired, OSError): + return {} + children: dict[int, list[tuple[int, str]]] = {} + for line in (result.stdout or "").splitlines(): + parts = line.split() + if len(parts) == 3 and parts[0].isdigit() and parts[1].isdigit(): + children.setdefault(int(parts[1]), []).append((int(parts[0]), parts[2])) + found: dict[int, int | None] = {} + pending = list(roots) + while pending: + for pid, tty in children.get(pending.pop(), ()): + if pid in found or pid in roots or _is_detached_session_leader(pid, tty): + continue + found[pid] = get_process_start_time(pid) + pending.append(pid) + return found + + +def _wait_gone(pids: list[int], seconds: float) -> list[int]: + """Poll up to *seconds*; return the PIDs still alive (zombies count as gone).""" import time as _time from gateway.status import _pid_exists + deadline = _time.monotonic() + seconds + alive = list(pids) + while alive and _time.monotonic() < deadline: + _time.sleep(0.1) + alive = [p for p in alive if _pid_exists(p)] # os.kill(pid, 0) breaks on Windows + return alive + + +def _kill_pids_posix(pids: list[int], killed: list[int], failed: list[tuple[int, str]]) -> None: + """SIGTERM, wait up to ``_POSIX_TERM_GRACE_SECONDS`` for graceful exit, SIGKILL survivors, then + sweep the dashboard-owned descendants that outlived the root and wait for the tree to be gone. + + *killed* / *failed* report the roots only; swept descendants are the roots' own teardown debt. + """ + import signal as _signal + + from gateway.status import get_process_start_time + + descendants = _posix_descendants(pids) + def _send(pid: int, sig) -> None: try: os.kill(pid, sig) @@ -307,16 +376,21 @@ def _kill_pids_posix(pids: list[int], killed: list[int], failed: list[tuple[int, for pid in pids: _send(pid, _signal.SIGTERM) - deadline = _time.monotonic() + _POSIX_TERM_GRACE_SECONDS pending = [p for p in pids if p not in killed and p not in {f[0] for f in failed}] - while pending and _time.monotonic() < deadline: - _time.sleep(0.1) - alive = [p for p in pending if _pid_exists(p)] # os.kill(pid, 0) breaks on Windows - killed.extend(p for p in pending if p not in alive) - pending = alive - for pid in pending: + alive = _wait_gone(pending, _POSIX_TERM_GRACE_SECONDS) + killed.extend(p for p in pending if p not in alive) + for pid in alive: _send(pid, _signal.SIGKILL) + # Snapshot identity must still match: a PID recycled during the grace is not ours to signal. + survivors = [p for p, start in descendants.items() + if start is not None and get_process_start_time(p) == start] + for sig in (_signal.SIGTERM, _signal.SIGKILL): + for pid in survivors: + with contextlib.suppress(OSError): + os.kill(pid, sig) + survivors = _wait_gone(survivors, _POSIX_DESCENDANT_GRACE_SECONDS) + def _kill_stale_dashboard_processes( reason: str = "the running backend no longer matches the updated frontend", *, @@ -744,6 +818,7 @@ def _reap_orphaned_desktop_local_serves( and _process_ppid(pid) in (0, 1) and _is_stale_orphan(pid)] if not matched: return _empty_result() + descendants = _posix_descendants(matched) # before the kill: the root's death reparents them killed: list[int] = [] failed: list[int] = [] for pid in matched: @@ -767,6 +842,14 @@ def _reap_orphaned_desktop_local_serves( killed.append(pid) except OSError: failed.append(pid) + # A SIGKILLed backend never ran PTY_REGISTRY.close_all(): its hosted ui-tui / MCP trees would + # keep the deleted state.db-wal inode open (#112631). The boot-path budget leaves no second + # grace, and these trees already lost their Electron and their backend. + from gateway.status import get_process_start_time + for pid, start in descendants.items(): + if start is not None and get_process_start_time(pid) == start: + with contextlib.suppress(OSError): + os.kill(pid, signal_kill) with contextlib.suppress(Exception): print(f"⟲ Reaped {len(killed)} orphaned desktop-local serve backend(s) ({reason}): {killed or matched}") return {"matched": matched, "killed": killed, "failed": failed} diff --git a/tests/hermes_cli/test_dashboard_procs_kill_grace.py b/tests/hermes_cli/test_dashboard_procs_kill_grace.py index fd5345049e..dc55ccc111 100644 --- a/tests/hermes_cli/test_dashboard_procs_kill_grace.py +++ b/tests/hermes_cli/test_dashboard_procs_kill_grace.py @@ -1,14 +1,19 @@ -"""The dashboard SIGTERM→SIGKILL grace must outlast the lifespan teardown (#111912). +"""The dashboard SIGTERM→SIGKILL grace must outlast the lifespan teardown (#111912), and a +descendant that outlives it must not survive the stop (#112631). ``hermes update`` / ``hermes dashboard --stop`` fall back to ``_kill_pids_posix`` for a manually-started backend. Its lifespan teardown blocks on ``stop_hosted_room_service(timeout=5.0)`` before ``PTY_REGISTRY.close_all()`` runs; a SIGKILL inside that window orphans the ui-tui / tui_gateway.entry children, which keep the deleted ``state.db-wal`` inode open until the next -start aborts with ``DeletedWalGenerationError``. Real child processes, real signals. +start aborts with ``DeletedWalGenerationError``. A wedged descendant defeats any finite grace, so +the kill sequence sweeps the dashboard-owned tree after the root — but never the detached +messaging-gateway bots the dashboard launched with ``start_new_session``. Real child processes, +real signals, a real PTY. """ from __future__ import annotations +import os import signal import subprocess import sys @@ -45,6 +50,45 @@ _IGNORING_CHILD = textwrap.dedent( """ ) +# A wedged hosted TUI: ignores SIGTERM and the SIGHUP its PTY master's close delivers. +_WEDGED_DESCENDANT = textwrap.dedent( + """ + import os, pathlib, signal, sys, time + signal.signal(signal.SIGTERM, signal.SIG_IGN) + signal.signal(signal.SIGHUP, signal.SIG_IGN) + pathlib.Path(sys.argv[1]).write_text(str(os.getpid())) + time.sleep(300) + """ +) + +_DETACHED_BOT = textwrap.dedent( + """ + import os, pathlib, sys, time + pathlib.Path(sys.argv[1]).write_text(str(os.getpid())) + time.sleep(300) + """ +) + +# Backend stand-in: a hosted TUI behind a real PTY (PtyBridge.spawn shape) plus a gateway bot +# launched detached (web_server_gateway / web_routers.messaging shape); its own SIGTERM teardown +# is wedged so the root gets SIGKILLed mid-teardown, exactly the incident. +_WEDGED_BACKEND = textwrap.dedent( + f""" + import pathlib, signal, subprocess, sys, time + import ptyprocess + tui_pid, bot_pid, ready = (pathlib.Path(p) for p in sys.argv[1:4]) + # Keep the handle: a collected PtyProcess terminates its child (SIGHUP…SIGKILL) from __del__. + tui = ptyprocess.PtyProcess.spawn([sys.executable, "-c", {_WEDGED_DESCENDANT!r}, str(tui_pid)]) + subprocess.Popen([sys.executable, "-c", {_DETACHED_BOT!r}, str(bot_pid)], start_new_session=True, + stdin=subprocess.DEVNULL, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) + while not (tui_pid.exists() and bot_pid.exists()): + time.sleep(0.01) + signal.signal(signal.SIGTERM, lambda *_: time.sleep(300)) + ready.write_text("ready") + time.sleep(300) + """ +) + def _spawn_ready(script: str, ready_path, *args: str) -> subprocess.Popen: child = subprocess.Popen( @@ -74,6 +118,16 @@ def _kill_and_reap(child: subprocess.Popen): return killed, failed +def _pid_running(pid: int) -> bool: + try: + os.kill(pid, 0) + except ProcessLookupError: + return False + stat = subprocess.run(["ps", "-o", "stat=", "-p", str(pid)], capture_output=True, text=True, + stdin=subprocess.DEVNULL, check=False).stdout.strip() + return bool(stat) and not stat.startswith("Z") + + def test_teardown_as_long_as_lifespan_budget_exits_gracefully(tmp_path): """A teardown spanning the 5s hosted-room stop + 1s join must not be SIGKILLed.""" marker, ready = tmp_path / "marker", tmp_path / "ready" @@ -98,3 +152,26 @@ def test_sigterm_ignoring_process_is_still_sigkilled(tmp_path, monkeypatch): assert failed == [] assert child.returncode == -signal.SIGKILL assert killed == [child.pid] + + +@pytest.mark.live_system_guard_bypass # the orphans are reparented out of the test subtree by design +def test_wedged_pty_descendant_is_gone_but_detached_bot_survives(tmp_path, monkeypatch): + """#112631: when the stop returns, the hosted TUI that outlived the SIGKILLed backend is dead + (it would hold the deleted state.db-wal inode), while the messaging-gateway bot the dashboard + started with ``start_new_session`` is untouched.""" + pytest.importorskip("ptyprocess") + monkeypatch.setattr(dashboard_procs, "_POSIX_TERM_GRACE_SECONDS", 0.6) + tui_pid_file, bot_pid_file, ready = tmp_path / "tui.pid", tmp_path / "bot.pid", tmp_path / "ready" + backend = _spawn_ready(_WEDGED_BACKEND, ready, str(tui_pid_file), str(bot_pid_file), str(ready)) + tui_pid, bot_pid = int(tui_pid_file.read_text()), int(bot_pid_file.read_text()) + try: + killed, failed = _kill_and_reap(backend) + + assert (killed, failed) == ([backend.pid], []) + assert backend.returncode == -signal.SIGKILL + assert not _pid_running(tui_pid), "wedged hosted TUI outlived the dashboard stop" + assert _pid_running(bot_pid), "detached gateway bot was killed with the dashboard" + finally: + for pid in (tui_pid, bot_pid): + if _pid_running(pid): + os.kill(pid, signal.SIGKILL) diff --git a/website/docs/reference/cli-commands.md b/website/docs/reference/cli-commands.md index 2aadfefc0a..c2d5ff89f1 100644 --- a/website/docs/reference/cli-commands.md +++ b/website/docs/reference/cli-commands.md @@ -1778,7 +1778,7 @@ Launch the web dashboard — a browser-based UI for managing configuration, API | `--insecure` | off | **Deprecated / no-op.** Formerly bypassed auth on a non-loopback bind. Since the June 2026 hardening a public bind *always* requires an auth provider (password or OAuth). Bind `127.0.0.1` and tunnel to keep it local. | | `--skip-build` | off | Skip the web UI build step and serve the existing `dist` directly. Useful for non-interactive contexts (Windows Scheduled Tasks, CI) where npm isn't available. Pre-build with `cd web && npm run build`. | | `--isolated` | off | When launched from a named profile (`worker dashboard`), run a dedicated per-profile server instead of routing to the machine dashboard. | -| `--stop` | — | Stop running `hermes dashboard` processes and exit. | +| `--stop` | — | Stop running `hermes dashboard` processes and exit. SIGTERM, a 10s grace, then SIGKILL; a hosted Chat TUI that outlives its backend is stopped too (it would otherwise keep the deleted `state.db-wal` open and block the next start). Messaging-gateway bots started from the dashboard are not touched. | | `--status` | — | List running `hermes dashboard` processes and exit. | ### `hermes dashboard register`