diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 19c3ce0a8d508..94dcb53885f74 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -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, diff --git a/hermes_cli/update_cmd.py b/hermes_cli/update_cmd.py index 4f9729752f07a..88fd658c7c15b 100644 --- a/hermes_cli/update_cmd.py +++ b/hermes_cli/update_cmd.py @@ -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) diff --git a/tests/hermes_cli/test_update_orphan_backend_reap.py b/tests/hermes_cli/test_update_orphan_backend_reap.py new file mode 100644 index 0000000000000..c03e9cb362d7b --- /dev/null +++ b/tests/hermes_cli/test_update_orphan_backend_reap.py @@ -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]] diff --git a/tests/hermes_cli/test_update_venv_health.py b/tests/hermes_cli/test_update_venv_health.py index ef583aef671dc..aaa5f7b915ae6 100644 --- a/tests/hermes_cli/test_update_venv_health.py +++ b/tests/hermes_cli/test_update_venv_health.py @@ -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() ):