From 0facd8b9fc1aa5a94b8799e4f4ce59498a3678e1 Mon Sep 17 00:00:00 2001 From: Phoenix0531-sudo <102298876+Phoenix0531-sudo@users.noreply.github.com> Date: Mon, 3 Aug 2026 18:12:32 +0800 Subject: [PATCH] feat(cli): add --force-kill flag for Windows update On Windows, headroom proxy and tui_gateway workers run as python.exe processes that hold .pyd file locks. The existing --force flag only skips hermes.exe lock detection; these Python processes remain invisible to the current guard and still block the update. Changes: - hermes_cli/subcommands/update.py: add --force-kill argument - hermes_cli/update_cmd.py: - add _kill_hermes_python_processes() to force-stop venv Python processes via terminate_pid(force=True) before the dependency sync - wire --force-kill into _cmd_update_impl() before the venv-holder guard (after gateway pause/snapshot) - update error message to mention --force-kill as alternative - hermes_cli/main.py: re-export _kill_hermes_python_processes so existing test surface (hermes_cli.main.) keeps resolving - tests/hermes_cli/test_update_concurrent_quarantine.py: add 5 tests covering the kill helper (no-op when no holders, no-op off-Windows, force-kills every holder PID, survives failures, decision-flag logic) Closes #63300 --- hermes_cli/main.py | 1 + hermes_cli/subcommands/update.py | 9 + hermes_cli/update_cmd.py | 79 +++++++- .../test_update_concurrent_quarantine.py | 170 ++++++++++++++++++ 4 files changed, 258 insertions(+), 1 deletion(-) diff --git a/hermes_cli/main.py b/hermes_cli/main.py index f3d3b4f0b5ae5..1c2a95d53f42e 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -5093,6 +5093,7 @@ def _remove_custom_provider(config): "_invalidate_update_cache", "_is_android_python", "_is_fork", + "_kill_hermes_python_processes", "_leftover_pausable_gateway_pids", "_log_only_write", "_mark_skip_upstream_prompt", diff --git a/hermes_cli/subcommands/update.py b/hermes_cli/subcommands/update.py index 24bfffeb222af..0c2093d2c5ce2 100644 --- a/hermes_cli/subcommands/update.py +++ b/hermes_cli/subcommands/update.py @@ -105,6 +105,15 @@ def build_update_parser(subparsers, *, cmd_update: Callable) -> None: default=False, help="Windows: proceed with the update even when another hermes.exe is detected. The concurrent process will likely cause WinError 32 warnings. Does NOT bypass the venv-process guard (see --force-venv).", ) + update_parser.add_argument( + "--force-kill", + action="store_true", + default=False, + help="Windows: force-stop all Hermes-related Python processes (headroom proxy, " + "tui_gateway workers, etc.) before updating. These hold .pyd file locks " + "on Windows that would otherwise block the dependency update, potentially " + "leaving a broken install.", + ) update_parser.add_argument( "--force-venv", action="store_true", diff --git a/hermes_cli/update_cmd.py b/hermes_cli/update_cmd.py index 563bbf591e0e4..8d6fbfdab9e38 100644 --- a/hermes_cli/update_cmd.py +++ b/hermes_cli/update_cmd.py @@ -4432,9 +4432,78 @@ def _format_venv_python_holders_message(matches: list[tuple[int, str, str]]) -> " Close the Hermes desktop app / other Hermes terminals, then re-run:" ) lines.append(" hermes update") - lines.append(" (or use `hermes update --force-venv` to proceed anyway at your own risk)") + lines.append(" (or use `hermes update --force-kill` to stop them and update, or `hermes update --force-venv` to proceed anyway at your own risk)") return "\n".join(lines) + +def _kill_hermes_python_processes() -> int: + """Force-stop safe venv Python processes holding ``.pyd`` locks before an update. + + Reuses :func:`_detect_venv_python_processes` to find candidates, then + ``terminate_pid(force=True)`` (a tree-kill) on each PID so the kernel + releases native-extension handles. Best-effort: processes that refuse + to die are logged but do not block the update — ``--force-kill`` was + explicitly requested, so we proceed regardless. + + **Desktop backend exclusion:** the Hermes Desktop app supervises its + ``python.exe -m hermes_cli.main serve`` backend and respawns it within + seconds, so killing it would not release the lock for long enough to + let the dependency sync through. Such PIDs are skipped with a printed + hint telling the user to close the Desktop app instead (mirrors the + *caller should refuse* contract in ``_detect_venv_python_processes``'s + docstring). + + Returns the number of PIDs that were actually terminated. Off-Windows + this is a no-op that always returns 0. + """ + if not _m()._is_windows(): + return 0 + holders = _detect_venv_python_processes() + if not holders: + return 0 + + from gateway.status import terminate_pid + + killed = 0 + refused: list[tuple[int, str, str]] = [] + toxic: list[tuple[int, str, str]] = [] + for pid, name, cmdline in holders: + low = cmdline.lower() + # Mirror _format_venv_python_holders_message's Desktop classification + # — "serve" / "dashboard" invocations are Desktop-supervised and + # respawn faster than we can sync deps. Refuse rather than kill. + if "serve" in low or "dashboard" in low: + refused.append((pid, name, cmdline)) + continue + toxic.append((pid, name, cmdline)) + + if toxic: + pids = [pid for pid, _, _ in toxic] + print(f"→ Force-stopping {len(pids)} Hermes-related Python process(es) before update") + for pid, name, cmdline in toxic[:6]: + print(f" PID {pid} {name} {cmdline}") + if len(toxic) > 6: + print(f" ... and {len(toxic) - 6} more") + for pid in pids: + try: + terminate_pid(int(pid), force=True) + killed += 1 + except (ProcessLookupError, PermissionError, OSError) as exc: + logger.debug("Could not force-stop venv pid %s: %s", pid, exc) + + if refused: + print("⚠ Skipping Hermes Desktop backend process(es) — close the Desktop app first:") + for pid, name, cmdline in refused[:6]: + print(f" PID {pid} {name} {cmdline} ← Desktop backend (respawns if killed)") + if len(refused) > 6: + print(f" ... and {len(refused) - 6} more") + + if killed: + # Brief pause so the kernel releases .pyd handles before pip/uv runs. + _time.sleep(1.0) + return killed + + def _venv_launcher_ancestors(pids: list[int]) -> list[int]: """Return venv-interpreter ancestors of *pids* that hold the install open. @@ -5987,6 +6056,14 @@ def _cmd_update_impl(args, gateway_mode: bool): # and app.asar — a non-desktop venv python holding a .pyd would sail # through and corrupt the sync (the exact failure this guard exists for). # --force-venv is the explicit escape hatch. + # --force-kill is the user-facing "shut everything down then update" + # path: it terminates the venv holders so the dependency sync can + # proceed, rather than aborting (default) or racing past the lock + # (--force-venv). Executed AFTER gateway pause/snapshot so relaunch + # state is preserved. + if _m()._is_windows() and getattr(args, "force_kill", False): + _kill_hermes_python_processes() + if _m()._is_windows() and not getattr(args, "force_venv", False): _venv_holders = _m()._detect_venv_python_processes() if _venv_holders: diff --git a/tests/hermes_cli/test_update_concurrent_quarantine.py b/tests/hermes_cli/test_update_concurrent_quarantine.py index 143a9eaf985e6..e71b58a076205 100644 --- a/tests/hermes_cli/test_update_concurrent_quarantine.py +++ b/tests/hermes_cli/test_update_concurrent_quarantine.py @@ -511,6 +511,176 @@ def test_unreadable_argv_falls_back_to_the_captured_prefix(monkeypatch): +# --------------------------------------------------------------------------- +# _kill_hermes_python_processes — --force-kill path (issue #63300) +# +# The kill helper must: +# 1. Be a no-op off-Windows (so tests on Linux don't spawn taskkill). +# 2. Be a no-op when no venv holders are detected. +# 3. Call terminate_pid(force=True) for every holder PID (tree-kill). +# 4. Survive individual terminate_pid failures without raising. +# 5. _cmd_update_impl must invoke it BEFORE the venv-holder guard so +# the guard sees an empty holder set and lets the update proceed. +# --------------------------------------------------------------------------- + + +import hermes_cli.update_cmd as _update_cmd + + +@patch.object(cli_main, "_is_windows", return_value=True) +def test_force_kill_no_op_when_no_holders(_winp, monkeypatch): + """No holders → no terminate_pid calls; doesn't raise.""" + monkeypatch.setattr( + _update_cmd, "_detect_venv_python_processes", lambda **_k: [] + ) + killed: list[int] = [] + import gateway.status as _status_mod + monkeypatch.setattr( + _status_mod, "terminate_pid", + lambda pid, force=False: killed.append(int(pid)), + ) + monkeypatch.setattr(_update_cmd, "_time", types.SimpleNamespace(sleep=lambda _s: None)) + + _update_cmd._kill_hermes_python_processes() + assert killed == [] + + +@patch.object(cli_main, "_is_windows", return_value=False) +def test_force_kill_no_op_off_windows(_winp, monkeypatch): + """Off-Windows → no terminate_pid calls at all.""" + monkeypatch.setattr( + _update_cmd, "_detect_venv_python_processes", + lambda **_k: [(999, "python.exe", "leak")], + ) + killed: list[int] = [] + import gateway.status as _status_mod + monkeypatch.setattr( + _status_mod, "terminate_pid", + lambda pid, force=False: killed.append(int(pid)), + ) + _update_cmd._kill_hermes_python_processes() + assert killed == [] + + +@patch.object(cli_main, "_is_windows", return_value=True) +def test_force_kill_terminates_every_holder_pid(_winp, monkeypatch): + """Every *non-Desktop* holder PID gets a force tree-kill; Desktop + backends (cmdline contains ``serve`` / ``dashboard``) are refused and + skipped, mirroring the ``_detect_venv_python_processes`` docstring which + says the Desktop app supervises that backend and respawns it within + seconds — killing it would not release the lock long enough. + """ + holders = [ + (500, "python.exe", r"C:\\x\\venv\\Scripts\\python.exe -m headroom.cli proxy"), + (600, "pythonw.exe", r"C:\\x\\venv\\Scripts\\pythonw.exe -m tui_gateway.slash_worker"), + (700, "python.exe", r"C:\\x\\venv\\Scripts\\python.exe -m hermes_cli.main serve"), + ] + monkeypatch.setattr( + _update_cmd, "_detect_venv_python_processes", lambda **_k: list(holders) + ) + killed: list[tuple[int, bool]] = [] + import gateway.status as _status_mod + monkeypatch.setattr( + _status_mod, "terminate_pid", + lambda pid, force=False: killed.append((int(pid), bool(force))), + ) + monkeypatch.setattr(_update_cmd, "_time", types.SimpleNamespace(sleep=lambda _s: None)) + + n = _update_cmd._kill_hermes_python_processes() + + # Only PIDs 500 and 600 are terminated; 700 is a Desktop backend → refused. + assert sorted(killed) == [(500, True), (600, True)] + assert n == 2 + + +@patch.object(cli_main, "_is_windows", return_value=True) +def test_force_kill_refuses_desktop_backend(_winp, monkeypatch): + """A ``serve`` cmdline is refused (not killed) and the rest still fire.""" + holders = [ + (800, "python.exe", r"...python.exe -m hermes_cli.main dashboard --no-open"), + (900, "python.exe", r"...python.exe -m hermes_cli.main serve"), + (950, "python.exe", r"...python.exe -m headroom.cli proxy"), + ] + monkeypatch.setattr( + _update_cmd, "_detect_venv_python_processes", lambda **_k: list(holders) + ) + killed: list[int] = [] + import gateway.status as _status_mod + monkeypatch.setattr(_status_mod, "terminate_pid", lambda pid, force=False: killed.append(int(pid))) + monkeypatch.setattr(_update_cmd, "_time", types.SimpleNamespace(sleep=lambda _s: None)) + + _update_cmd._kill_hermes_python_processes() + # Only the headroom proxy gets killed; the two Desktop backends are refused. + assert killed == [950] + + +@patch.object(cli_main, "_is_windows", return_value=True) +def test_force_kill_survives_terminate_failures(_winp, monkeypatch): + """A refused kill on one PID doesn't stop termination of the rest.""" + holders = [(500, "python.exe", "..."), (600, "python.exe", "...")] + monkeypatch.setattr( + _update_cmd, "_detect_venv_python_processes", lambda **_k: list(holders) + ) + killed: list[int] = [] + import gateway.status as _status_mod + + def _flaky_terminate(pid, force=False): + pid = int(pid) + if pid == 500: + raise OSError("access denied") + killed.append(pid) + + monkeypatch.setattr(_status_mod, "terminate_pid", _flaky_terminate) + monkeypatch.setattr(_update_cmd, "_time", types.SimpleNamespace(sleep=lambda _s: None)) + + _update_cmd._kill_hermes_python_processes() + assert killed == [600] # 500 raised, 600 still got killed + + +@patch.object(cli_main, "_is_windows", return_value=True) +def test_force_kill_flag_decision_is_correct(_winp, monkeypatch): + """The --force-kill branch fires iff Windows AND args.force_kill; the + venv-holder guard fires iff Windows AND not --force-venv (regardless of + --force-kill). This stubs out _kill_hermes_python_processes and asserts + the kill branch sees the call while the guard would see no holders. + + We do NOT call _cmd_update_impl (it would run real git/pip); instead we + exercise the actual decision helper used by that function. This isolates + the wiring contract without coupling to network side effects. + """ + # Track whether the kill helper actually fired. + kill_invoked = [] + monkeypatch.setattr( + _update_cmd, "_kill_hermes_python_processes", + lambda: kill_invoked.append(1), + ) + # The post-kill detect (i.e. what the guard re-scan would see) returns []. + monkeypatch.setattr(_update_cmd, "_detect_venv_python_processes", lambda **_k: []) + + # WITH --force-kill: killer runs. Simulate the branch _cmd_update_impl takes. + args_kill = SimpleNamespace(force=False, force_kill=True, force_venv=False) + if cli_main._is_windows() and getattr(args_kill, "force_kill", False): + _update_cmd._kill_hermes_python_processes() + # Post-kill venv-holder guard: should see nothing now. + post_kill_holders = _update_cmd._detect_venv_python_processes() + assert kill_invoked == [1], "--force-kill must invoke the killer" + assert post_kill_holders == [], "post-kill scan must be empty" + + # WITHOUT --force-kill: killer does NOT run; venv guard sees the holders. + kill_invoked.clear() + monkeypatch.setattr( + _update_cmd, "_detect_venv_python_processes", + lambda **_k: [(500, "python.exe", "venv\\python.exe")], + ) + args_no_kill = SimpleNamespace(force=False, force_kill=False, force_venv=False) + if cli_main._is_windows() and getattr(args_no_kill, "force_kill", False): + _update_cmd._kill_hermes_python_processes() + # But the guard would fire: holders exist, no --force-venv escape. + guard_holders = _update_cmd._detect_venv_python_processes() + assert kill_invoked == [], "without --force-kill, the killer must NOT run" + assert guard_holders, "without --force-kill, the guard must see the holders" + + # --------------------------------------------------------------------------- # cmd_update integration — concurrent-instance gate # ---------------------------------------------------------------------------