fix(update): reap orphaned Desktop backends instead of dead-ending the venv-holder guard
The GUI-updater handoff race: the Desktop fires SIGTERM + app.quit() and spawns hermes-setup, but its Python backend (`python.exe -m hermes_cli.main serve`) can survive the teardown. The Desktop is gone -- nothing will respawn that backend -- yet the venv-holder guard refused on it and the update dead-ended with "Hermes is still running" while the user had zero windows open (observed twice on 2026-08-09, 01:59 and 02:17, bootstrap-installer.log). New `_orphaned_desktop_backend_pids()` classifies remaining holders: a serve/dashboard backend whose supervising parent is provably dead (PID gone, or recycled -- parent created after the child) is a straggler safe to reap. Any live-parent backend, non-backend holder, or unprovable case keeps the refusal exactly as before. Reaping uses the new `_stop_process_trees()` (taskkill /T /F), mirroring the Desktop's forceKillProcessTree and install.ps1's venv sweep so the managed .hermes-runtime interpreter child dies with its launcher (#70026). Builds on #81327 (salvaged intact underneath): that fixed the same parent-only-kill gap in install.ps1's venv sweep; this closes the remaining dead-end in the `hermes update` guard itself. E2E on a real Windows box: spawned a detached orphan with a backend-shaped argv -> classifier returned its PID and the tree reap killed it; a non-backend orphan and the live Desktop backend (parent alive) both returned None (refusal preserved).
This commit is contained in:
Teknium
parent
a09124cea5
commit
826bf9b6d8
|
|
@ -5111,6 +5111,7 @@ from hermes_cli.update_cmd import ( # noqa: F401
|
|||
_npm_lockfile_changed,
|
||||
_npm_manifest_paths,
|
||||
_npm_manifests_digest,
|
||||
_orphaned_desktop_backend_pids,
|
||||
_pause_windows_gateways_for_update,
|
||||
_print_curator_first_run_notice,
|
||||
_print_curator_recent_run_notice,
|
||||
|
|
@ -5131,6 +5132,7 @@ from hermes_cli.update_cmd import ( # noqa: F401
|
|||
_should_skip_upstream_prompt,
|
||||
_stash_apply_failed_only_on_existing_untracked,
|
||||
_stash_local_changes_if_needed,
|
||||
_stop_process_trees,
|
||||
_sync_fork_with_upstream,
|
||||
_sync_with_upstream_if_needed,
|
||||
_update_node_dependencies,
|
||||
|
|
|
|||
|
|
@ -3076,6 +3076,93 @@ def _leftover_pausable_gateway_pids(
|
|||
return pids
|
||||
|
||||
|
||||
def _orphaned_desktop_backend_pids(
|
||||
matches: list[tuple[int, str, str]],
|
||||
) -> list[int] | None:
|
||||
"""PIDs from *matches* when every remaining holder is an ORPHANED backend.
|
||||
|
||||
The venv-holder guard refuses on the Desktop app's ``serve`` backend by
|
||||
design: while the Desktop is open, killing its backend is futile (the app
|
||||
supervises and respawns it within seconds), so the user must close the
|
||||
app. But in the GUI-updater handoff path the Desktop has *already
|
||||
exited* — by contract it tree-kills its backends and waits for the venv
|
||||
shim before spawning hermes-setup, and the update-in-progress marker
|
||||
parks any relaunched Desktop from spawning a fresh backend (#50238). A
|
||||
``serve`` backend still holding the venv at that point is a straggler
|
||||
whose supervisor is gone: SIGTERM raced its spawn, or it belongs to a
|
||||
crashed window. Nothing will respawn it, and refusing on it dead-ends
|
||||
the update with "Hermes is still running" while the user stares at zero
|
||||
open windows (ryanc's 2026-08-09 01:59/02:17 failures).
|
||||
|
||||
A holder qualifies only when BOTH hold:
|
||||
|
||||
- its cmdline is a Hermes backend (``hermes_cli.main`` + ``serve`` /
|
||||
``dashboard``), and
|
||||
- its supervising parent is demonstrably gone: the parent PID no longer
|
||||
exists, or the PID was reused (parent created *after* the child).
|
||||
|
||||
A backend whose parent is alive (the Desktop is still open) disqualifies
|
||||
the whole set — the guard must keep refusing exactly as before. Returns
|
||||
``None`` in that case, or when any holder is not a backend, or when
|
||||
psutil is unavailable (can't prove orphanhood → refuse). Never raises.
|
||||
"""
|
||||
try:
|
||||
import psutil # type: ignore
|
||||
except Exception:
|
||||
return None
|
||||
|
||||
pids: list[int] = []
|
||||
for pid, _name, cmdline in matches:
|
||||
argv = cmdline
|
||||
try:
|
||||
argv = " ".join(psutil.Process(int(pid)).cmdline()) or cmdline
|
||||
except psutil.NoSuchProcess:
|
||||
# Holder exited between scan and classification — nothing to
|
||||
# reap, nothing blocking. Skip it.
|
||||
continue
|
||||
except Exception:
|
||||
pass
|
||||
low = argv.lower()
|
||||
if "hermes_cli.main" not in low or not (
|
||||
" serve" in low or " dashboard" in low
|
||||
):
|
||||
return None
|
||||
try:
|
||||
proc = psutil.Process(int(pid))
|
||||
ppid = proc.ppid()
|
||||
parent = psutil.Process(ppid) if ppid else None
|
||||
if parent is not None and parent.is_running():
|
||||
# PID-reuse check: a "parent" created after its child is a
|
||||
# recycled PID, not the real (dead) supervisor.
|
||||
if parent.create_time() <= proc.create_time():
|
||||
return None
|
||||
except psutil.NoSuchProcess:
|
||||
pass # parent gone → orphan
|
||||
except Exception:
|
||||
return None
|
||||
pids.append(int(pid))
|
||||
return pids
|
||||
|
||||
|
||||
def _stop_process_trees(pids: list[int]) -> None:
|
||||
"""Force-stop each PID with its full child tree (Windows).
|
||||
|
||||
``taskkill /T /F`` mirrors the Desktop's ``forceKillProcessTree`` and
|
||||
install.ps1's venv sweep: stopping only the parent can leave a managed
|
||||
``.hermes-runtime`` interpreter child alive and holding the install open
|
||||
(#70026). Best effort; never raises.
|
||||
"""
|
||||
for pid in pids:
|
||||
try:
|
||||
subprocess.run(
|
||||
["taskkill", "/PID", str(int(pid)), "/T", "/F"],
|
||||
check=False,
|
||||
capture_output=True,
|
||||
)
|
||||
except Exception as exc:
|
||||
logger.debug("Could not stop process tree %s: %s", pid, exc)
|
||||
|
||||
|
||||
def _pause_windows_gateways_for_update() -> dict | None:
|
||||
"""Stop running Windows gateways before mutating the checkout or venv.
|
||||
|
||||
|
|
@ -3673,6 +3760,26 @@ def _cmd_update_impl(args, gateway_mode: bool):
|
|||
)
|
||||
_time.sleep(1.0)
|
||||
_venv_holders = _m()._detect_venv_python_processes()
|
||||
if _venv_holders:
|
||||
_orphan_backends = _m()._orphaned_desktop_backend_pids(_venv_holders)
|
||||
if _orphan_backends:
|
||||
# Every remaining holder is a Desktop `serve` backend whose
|
||||
# supervising app is GONE — the GUI-updater handoff race:
|
||||
# Electron's teardown lost the SIGTERM race, exited, and left
|
||||
# its backend (and any .hermes-runtime child) holding the
|
||||
# venv. Nothing will respawn an orphan, so reap the tree and
|
||||
# re-check instead of dead-ending with "Hermes is still
|
||||
# running" while no window is open. Backends whose Desktop
|
||||
# is still alive never reach here (_orphaned_desktop_
|
||||
# backend_pids returns None for them) — that path keeps the
|
||||
# refusal, because the app would just respawn what we kill.
|
||||
print(
|
||||
f" ⚠ {len(_orphan_backends)} orphaned Desktop backend "
|
||||
"process(es) still hold the venv; stopping their trees"
|
||||
)
|
||||
_m()._stop_process_trees(_orphan_backends)
|
||||
_time.sleep(1.0)
|
||||
_venv_holders = _m()._detect_venv_python_processes()
|
||||
if _venv_holders:
|
||||
print(_format_venv_python_holders_message(_venv_holders))
|
||||
_m()._resume_windows_gateways_after_update(_windows_gateway_resume)
|
||||
|
|
|
|||
|
|
@ -0,0 +1,258 @@
|
|||
"""Tests for the orphaned-Desktop-backend reap in the venv-holder guard.
|
||||
|
||||
The GUI-updater handoff race (ryanc's 2026-08-09 failures): the Desktop app
|
||||
fires SIGTERM + app.quit() and spawns hermes-setup, but its Python backend
|
||||
(``python.exe -m hermes_cli.main serve``) survives the teardown race. The
|
||||
Desktop is gone — nothing will respawn that backend — yet the venv-holder
|
||||
guard refused on it and the update dead-ended with "Hermes is still running"
|
||||
while the user had zero windows open.
|
||||
|
||||
``_orphaned_desktop_backend_pids`` classifies holders: a ``serve``/
|
||||
``dashboard`` backend whose supervising parent is provably dead is safe to
|
||||
reap (with its full child tree — the managed .hermes-runtime interpreter
|
||||
child included, #70026); anything else keeps the refusal.
|
||||
|
||||
All paths run on any host via a fake psutil module (same approach as
|
||||
test_update_venv_health.py).
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import sys
|
||||
import types
|
||||
from types import SimpleNamespace
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
from hermes_cli import main as cli_main
|
||||
|
||||
|
||||
class _FakeNoSuchProcess(Exception):
|
||||
pass
|
||||
|
||||
|
||||
def _fake_psutil(procs: dict[int, MagicMock]):
|
||||
"""Build a psutil stand-in whose Process(pid) serves from *procs*."""
|
||||
|
||||
def _process(pid: int):
|
||||
if pid not in procs:
|
||||
raise _FakeNoSuchProcess(pid)
|
||||
return procs[pid]
|
||||
|
||||
return types.SimpleNamespace(
|
||||
Process=_process, NoSuchProcess=_FakeNoSuchProcess
|
||||
)
|
||||
|
||||
|
||||
def _proc(
|
||||
pid: int,
|
||||
cmdline: list[str],
|
||||
*,
|
||||
ppid: int = 0,
|
||||
create_time: float = 100.0,
|
||||
):
|
||||
proc = MagicMock()
|
||||
proc.pid = pid
|
||||
proc.cmdline.return_value = cmdline
|
||||
proc.ppid.return_value = ppid
|
||||
proc.create_time.return_value = create_time
|
||||
proc.is_running.return_value = True
|
||||
return proc
|
||||
|
||||
|
||||
_SERVE_ARGV = [
|
||||
"C:\\hermes\\venv\\Scripts\\python.exe",
|
||||
"-m",
|
||||
"hermes_cli.main",
|
||||
"serve",
|
||||
"--host",
|
||||
"127.0.0.1",
|
||||
]
|
||||
|
||||
|
||||
def _holders(pid=200, cmdline="python.exe -m hermes_cli.main serve"):
|
||||
return [(pid, "python.exe", cmdline)]
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# _orphaned_desktop_backend_pids classification
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_orphan_backend_dead_parent_qualifies():
|
||||
backend = _proc(200, _SERVE_ARGV, ppid=999) # 999 not in table → dead
|
||||
fake = _fake_psutil({200: backend})
|
||||
with patch.dict(sys.modules, {"psutil": fake}):
|
||||
assert cli_main._orphaned_desktop_backend_pids(_holders()) == [200]
|
||||
|
||||
|
||||
def test_backend_with_live_parent_keeps_refusal():
|
||||
parent = _proc(50, ["Hermes.exe"], create_time=10.0)
|
||||
backend = _proc(200, _SERVE_ARGV, ppid=50, create_time=100.0)
|
||||
fake = _fake_psutil({50: parent, 200: backend})
|
||||
with patch.dict(sys.modules, {"psutil": fake}):
|
||||
assert cli_main._orphaned_desktop_backend_pids(_holders()) is None
|
||||
|
||||
|
||||
def test_recycled_parent_pid_counts_as_orphan():
|
||||
# "Parent" created AFTER the child = PID reuse; real supervisor is dead.
|
||||
recycled = _proc(50, ["notepad.exe"], create_time=500.0)
|
||||
backend = _proc(200, _SERVE_ARGV, ppid=50, create_time=100.0)
|
||||
fake = _fake_psutil({50: recycled, 200: backend})
|
||||
with patch.dict(sys.modules, {"psutil": fake}):
|
||||
assert cli_main._orphaned_desktop_backend_pids(_holders()) == [200]
|
||||
|
||||
|
||||
def test_non_backend_holder_keeps_refusal():
|
||||
repl = _proc(300, ["python.exe", "-m", "hermes_cli.main", "chat"], ppid=999)
|
||||
fake = _fake_psutil({300: repl})
|
||||
with patch.dict(sys.modules, {"psutil": fake}):
|
||||
holders = _holders(pid=300, cmdline="python.exe -m hermes_cli.main chat")
|
||||
assert cli_main._orphaned_desktop_backend_pids(holders) is None
|
||||
|
||||
|
||||
def test_mixed_holders_keep_refusal():
|
||||
# One orphan backend + one operator REPL → the whole set is refused.
|
||||
backend = _proc(200, _SERVE_ARGV, ppid=999)
|
||||
repl = _proc(300, ["python.exe", "some_script.py"], ppid=998)
|
||||
fake = _fake_psutil({200: backend, 300: repl})
|
||||
with patch.dict(sys.modules, {"psutil": fake}):
|
||||
holders = _holders() + [(300, "python.exe", "python.exe some_script.py")]
|
||||
assert cli_main._orphaned_desktop_backend_pids(holders) is None
|
||||
|
||||
|
||||
def test_holder_gone_between_scan_and_classify_is_skipped():
|
||||
fake = _fake_psutil({}) # PID vanished entirely
|
||||
with patch.dict(sys.modules, {"psutil": fake}):
|
||||
assert cli_main._orphaned_desktop_backend_pids(_holders()) == []
|
||||
|
||||
|
||||
def test_missing_psutil_keeps_refusal():
|
||||
import builtins
|
||||
|
||||
real_import = builtins.__import__
|
||||
|
||||
def _no_psutil(name, *args, **kwargs):
|
||||
if name == "psutil":
|
||||
raise ImportError("no psutil")
|
||||
return real_import(name, *args, **kwargs)
|
||||
|
||||
with patch.dict(sys.modules, {"psutil": None}), patch.object(
|
||||
builtins, "__import__", _no_psutil
|
||||
):
|
||||
assert cli_main._orphaned_desktop_backend_pids(_holders()) is None
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# _stop_process_trees
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_stop_process_trees_kills_full_tree():
|
||||
from hermes_cli import update_cmd
|
||||
|
||||
with patch.object(update_cmd.subprocess, "run") as run:
|
||||
cli_main._stop_process_trees([111, 222])
|
||||
calls = [c.args[0] for c in run.call_args_list]
|
||||
assert calls == [
|
||||
["taskkill", "/PID", "111", "/T", "/F"],
|
||||
["taskkill", "/PID", "222", "/T", "/F"],
|
||||
]
|
||||
|
||||
|
||||
def test_stop_process_trees_never_raises():
|
||||
from hermes_cli import update_cmd
|
||||
|
||||
with patch.object(
|
||||
update_cmd.subprocess, "run", side_effect=OSError("no taskkill")
|
||||
):
|
||||
cli_main._stop_process_trees([111]) # must not raise
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Guard integration: orphan reap clears the dead-end
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def _update_args(**overrides):
|
||||
defaults = dict(
|
||||
gateway=False,
|
||||
check=False,
|
||||
no_backup=True,
|
||||
backup=False,
|
||||
yes=True,
|
||||
branch=None,
|
||||
force=False,
|
||||
force_venv=False,
|
||||
)
|
||||
defaults.update(overrides)
|
||||
return SimpleNamespace(**defaults)
|
||||
|
||||
|
||||
def _run_guard(detect_side_effect, orphan_return):
|
||||
"""Drive _cmd_update_impl to the venv-holder guard (harness mirrors
|
||||
test_update_venv_health.py)."""
|
||||
|
||||
class _PastGuard(Exception):
|
||||
pass
|
||||
|
||||
class _RootSentinel:
|
||||
def __truediv__(self, _other):
|
||||
raise _PastGuard
|
||||
|
||||
killed: list[list[int]] = []
|
||||
|
||||
with patch.object(cli_main, "_is_windows", return_value=True), patch.object(
|
||||
cli_main, "_venv_scripts_dir", return_value=None
|
||||
), patch.object(cli_main, "_run_pre_update_backup"), patch.object(
|
||||
cli_main, "_pause_windows_gateways_for_update", return_value=None
|
||||
), patch.object(
|
||||
cli_main, "_resume_windows_gateways_after_update"
|
||||
), patch.object(
|
||||
cli_main, "_detect_venv_python_processes", side_effect=detect_side_effect
|
||||
), patch.object(
|
||||
cli_main, "_leftover_pausable_gateway_pids", return_value=None
|
||||
), patch.object(
|
||||
cli_main, "_orphaned_desktop_backend_pids", return_value=orphan_return
|
||||
), patch.object(
|
||||
cli_main, "_stop_process_trees", side_effect=killed.append
|
||||
), patch.object(
|
||||
cli_main, "PROJECT_ROOT", _RootSentinel()
|
||||
), patch(
|
||||
"time.sleep"
|
||||
):
|
||||
try:
|
||||
cli_main._cmd_update_impl(_update_args(), gateway_mode=False)
|
||||
except _PastGuard:
|
||||
return "past_guard", killed
|
||||
except SystemExit as exc:
|
||||
return f"exit_{exc.code}", killed
|
||||
return "returned", killed
|
||||
|
||||
|
||||
def test_guard_reaps_orphan_backend_and_proceeds():
|
||||
holders = _holders()
|
||||
# 1st scan: backend present; 2nd (post-reap) scan: clear.
|
||||
result, killed = _run_guard(
|
||||
detect_side_effect=[holders, []], orphan_return=[200]
|
||||
)
|
||||
assert result == "past_guard"
|
||||
assert killed == [[200]]
|
||||
|
||||
|
||||
def test_guard_still_refuses_when_not_orphaned():
|
||||
holders = _holders()
|
||||
result, killed = _run_guard(
|
||||
detect_side_effect=[holders, holders], orphan_return=None
|
||||
)
|
||||
assert result == "exit_2"
|
||||
assert killed == []
|
||||
|
||||
|
||||
def test_guard_refuses_when_reap_does_not_clear_holders():
|
||||
holders = _holders()
|
||||
# Reap runs but a holder survives (unkillable child) → refuse.
|
||||
result, killed = _run_guard(
|
||||
detect_side_effect=[holders, holders], orphan_return=[200]
|
||||
)
|
||||
assert result == "exit_2"
|
||||
assert killed == [[200]]
|
||||
|
|
@ -133,6 +133,12 @@ def _run_update_until_guard(args):
|
|||
cli_main,
|
||||
"_detect_venv_python_processes",
|
||||
return_value=[(101, "python.exe", "python.exe -m hermes_cli.main serve")],
|
||||
), patch.object(
|
||||
# Pin the orphan classifier: this test exercises --force/--force-venv
|
||||
# gating, not orphan detection (covered in
|
||||
# test_update_orphan_backend_reap.py). None = "not provably orphaned"
|
||||
# → the guard refuses exactly as before the orphan-reap addition.
|
||||
cli_main, "_orphaned_desktop_backend_pids", return_value=None
|
||||
), patch.object(
|
||||
cli_main, "PROJECT_ROOT", _RootSentinel()
|
||||
):
|
||||
|
|
|
|||
Loading…
Reference in New Issue