diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 4abb7bf8303b..cd1f64ba93c3 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -8190,14 +8190,14 @@ def _hermes_exe_shims(scripts_dir: Path) -> list[Path]: def _quarantine_running_hermes_exe( scripts_dir: Path, *, max_attempts: int = 4 -) -> list[tuple[Path, Path]]: +) -> tuple[list[tuple[Path, Path]], list[tuple[Path, str]]]: """Pre-empt Windows file lock on the running ``hermes.exe``. Windows allows RENAMING a mapped/running executable (the kernel tracks the file by handle, not path), but blocks DELETE/REPLACE while it's loaded. uv needs to overwrite the entry-point shims during ``pip install -e .``; when ``hermes update`` runs, ``hermes.exe`` IS the live process, and uv - fails with ``Access is denied. (os error 5)``. + fails with ``Access is denied. (os error 5)`` / ``WinError 32``. We rename live shims to ``hermes.exe.old.`` first. uv then writes fresh shims at the original paths. The ``.old`` files are cleaned up on @@ -8211,22 +8211,21 @@ def _quarantine_running_hermes_exe( 1. Retry up to ``max_attempts`` times with exponential backoff (100/250/500/1000 ms). Handles the AV-scanner case. 2. If all retries fail, schedule the .exe for replacement on next - reboot via ``MoveFileExW(MOVEFILE_DELAY_UNTIL_REBOOT)``. This still - lets uv create a fresh shim at the original path (Windows will keep - the old file's content under a new name until the reboot), so the - update can complete; the user just needs to reboot to fully unload - the stale image. - 3. Print a clear warning naming the most likely culprit (running - Hermes Desktop / gateway / REPL) and pointing to ``--force``. - - Returns the list of (original, quarantined) pairs so the caller can roll - back if the install itself fails before uv writes a replacement. Pairs - where we used ``MOVEFILE_DELAY_UNTIL_REBOOT`` are NOT returned — they - are already deferred and roll-back is meaningless. + reboot via ``MoveFileExW(MOVEFILE_DELAY_UNTIL_REBOOT)``. + **The file remains locked until reboot** — callers must NOT proceed + with uv install when blocked is non-empty (#68760). We used to + continue and print three WinError 32 failures then fall back to ZIP. + 3. Print a clear warning naming the most likely culprit. + + Returns: + ``(moved, blocked)`` where *moved* is ``(original, quarantined)`` + pairs safe to roll back, and *blocked* is ``(shim, reason)`` pairs + that remain locked (``"locked"`` or ``"reboot_required"``). """ moved: list[tuple[Path, Path]] = [] + blocked: list[tuple[Path, str]] = [] if not _is_windows(): - return moved + return moved, blocked import time @@ -8259,10 +8258,9 @@ def _quarantine_running_hermes_exe( continue # All in-process renames failed. Try MoveFileEx with - # MOVEFILE_DELAY_UNTIL_REBOOT as a last resort. This succeeds in the - # exact case where the inline rename failed (another process holds - # the handle without share-delete), at the cost of requiring a - # reboot to fully reclaim the old .exe. + # MOVEFILE_DELAY_UNTIL_REBOOT as a last resort. This records a + # PendingFileRenameOperations entry but does NOT free the path for + # uv to overwrite until reboot (#68760). scheduled = _schedule_replace_on_reboot(shim, target) if scheduled: print( @@ -8270,15 +8268,13 @@ def _quarantine_running_hermes_exe( f"replacement on next reboot." ) print( - " The new shim was written at the same path, but a " - "reboot is needed to fully unload the old one." + " Do not continue the install until after reboot — the " + "shim path is still locked (continuing would yield WinError 32)." ) - # Do NOT append to ``moved``: we don't want roll-back to undo a - # reboot-deferred operation. + blocked.append((shim, "reboot_required")) continue - # Truly couldn't budge the .exe. Print an actionable warning and let - # uv try its luck — sometimes uv's own retry handling pulls through. + # Truly couldn't budge the .exe. print( f" ⚠ Could not quarantine {shim.name} ({last_exc.__class__.__name__}: " f"another process is holding it open)." @@ -8287,8 +8283,9 @@ def _quarantine_running_hermes_exe( " Close Hermes Desktop, exit other `hermes` REPLs, stop the " "gateway, or pause AV scanning, then re-run `hermes update`." ) + blocked.append((shim, "locked")) - return moved + return moved, blocked def _schedule_replace_on_reboot(shim: Path, quarantine_target: Path) -> bool: @@ -8336,6 +8333,54 @@ def _restore_quarantined_exes(moved: list[tuple[Path, Path]]) -> None: pass +def _format_blocked_shim_message( + blocked: list[tuple[Path, str]], scripts_dir: Path +) -> str: + """Explain why install must stop when hermes.exe shims stay locked (#68760).""" + lines = [ + "✗ Cannot replace locked Hermes entry-point shim(s) on Windows:", + ] + for shim, reason in blocked: + tag = ( + "reboot required to finish pending rename" + if reason == "reboot_required" + else "file in use (WinError 32 if install continues)" + ) + lines.append(f" {shim.name} — {tag}") + lines.append("") + # Re-probe holders so the user gets PIDs, not just a generic warning. + holders = _detect_concurrent_hermes_instances(scripts_dir) + venv_holders = _detect_venv_python_processes() + if holders: + lines.append(" Processes holding hermes.exe shims:") + for pid, name in holders[:8]: + lines.append(f" PID {pid} {name}") + pid_args = " ".join(f"/PID {pid}" for pid, _ in holders) + lines.append(f" taskkill {pid_args} /F") + lines.append("") + if venv_holders: + lines.append(" Venv python processes (Desktop backend / gateway):") + for pid, name, cmdline in venv_holders[:6]: + lines.append(f" PID {pid} {name} {cmdline}") + lines.append(" → close Hermes Desktop / stop gateway, then retry") + lines.append("") + if any(reason == "reboot_required" for _, reason in blocked): + lines.append(" A reboot-deferred rename is pending. Reboot, then run:") + lines.append(" hermes update") + else: + lines.append(" Close Hermes Desktop, exit open `hermes` sessions, stop the") + lines.append(" gateway (`hermes gateway stop`), then re-run:") + lines.append(" hermes update") + lines.append("") + lines.append(" Refusing to continue into uv/pip (avoids three WinError 32") + lines.append(" retries and a doomed ZIP fallback).") + return "\n".join(lines) + + +class HermesShimLockedError(RuntimeError): + """Raised when Windows hermes.exe shims could not be quarantined (#68760).""" + + def _run_quarantined_install( cmd: list[str], *, @@ -8355,11 +8400,20 @@ def _run_quarantined_install( :func:`_verify_core_dependencies_installed`, which previously called ``_run_install_with_heartbeat`` directly and bypassed quarantine. + If quarantine cannot free a shim, raises :class:`HermesShimLockedError` + instead of letting uv print repeated WinError 32 failures (#68760). + Off-Windows (``scripts_dir is None``) this is a thin pass-through. """ moved: list[tuple[Path, Path]] = [] if scripts_dir is not None: - moved = _quarantine_running_hermes_exe(scripts_dir) + moved, blocked = _quarantine_running_hermes_exe(scripts_dir) + if blocked: + msg = _format_blocked_shim_message(blocked, scripts_dir) + print(msg) + # Restore any shims we did move so the install is left consistent. + _restore_quarantined_exes(moved) + raise HermesShimLockedError(msg) try: _run_install_with_heartbeat(cmd, env=env) except BaseException: @@ -8636,6 +8690,10 @@ def _install(args: list[str]) -> None: _install(["install", "-e", f".[{group}]"]) _verify_console_scripts_installed(install_cmd_prefix, env=env) return + except HermesShimLockedError: + # Do not fall through into base/extras retries — every attempt would + # hit the same locked shim (WinError 32 x3 + ZIP fallback). #68760 + raise except subprocess.CalledProcessError: print( " ⚠ Optional extras failed, reinstalling base dependencies and retrying extras individually..." @@ -9307,7 +9365,10 @@ def cmd_update(args): sys.exit(UPDATE_EXIT_CONCURRENT) try: - _cmd_update_impl(args, gateway_mode=gateway_mode) + try: + _cmd_update_impl(args, gateway_mode=gateway_mode) + except HermesShimLockedError: + sys.exit(2) finally: _update_lock.release() _finalize_update_output(_update_io_state) diff --git a/tests/hermes_cli/test_update_concurrent_quarantine.py b/tests/hermes_cli/test_update_concurrent_quarantine.py index 784c9f9bc411..78eac8fd82ca 100644 --- a/tests/hermes_cli/test_update_concurrent_quarantine.py +++ b/tests/hermes_cli/test_update_concurrent_quarantine.py @@ -149,8 +149,9 @@ def test_quarantine_succeeds_first_attempt(_winp, tmp_path): shim = tmp_path / "hermes.exe" shim.write_bytes(b"old") - pairs = cli_main._quarantine_running_hermes_exe(tmp_path) + pairs, blocked = cli_main._quarantine_running_hermes_exe(tmp_path) + assert blocked == [] assert len(pairs) == 1 orig, quarantine = pairs[0] assert orig == shim @@ -159,6 +160,34 @@ def test_quarantine_succeeds_first_attempt(_winp, tmp_path): assert not shim.exists() +@patch.object(cli_main, "_is_windows", return_value=True) +def test_quarantine_retries_then_succeeds(_winp, tmp_path, monkeypatch): + """A transient OSError on the first attempt should not be fatal.""" + shim = tmp_path / "hermes.exe" + shim.write_bytes(b"old") + + original_rename = Path.rename + call_count = {"n": 0} + + def flaky_rename(self, target): + call_count["n"] += 1 + if call_count["n"] == 1: + raise OSError(32, "share violation (simulated AV scan)") + return original_rename(self, target) + + # Speed up the test: avoid actual sleeps in the backoff schedule. + monkeypatch.setattr(cli_main, "_hermes_exe_shims", lambda d: [shim]) + with patch.object(Path, "rename", flaky_rename), patch( + "time.sleep", lambda *_a, **_k: None + ): + pairs, blocked = cli_main._quarantine_running_hermes_exe(tmp_path) + + assert call_count["n"] >= 2 + assert blocked == [] + assert len(pairs) == 1 + assert not shim.exists() + + @patch.object(cli_main, "_is_windows", return_value=True) def test_quarantine_falls_back_to_reboot_schedule(_winp, tmp_path, capsys, monkeypatch): """When every retry fails, we schedule via MoveFileEx and warn helpfully.""" @@ -178,7 +207,7 @@ def fake_schedule(s: Path, q: Path) -> bool: with patch.object(Path, "rename", always_fails), patch.object( cli_main, "_schedule_replace_on_reboot", fake_schedule ), patch("time.sleep", lambda *_a, **_k: None): - pairs = cli_main._quarantine_running_hermes_exe(tmp_path) + pairs, blocked = cli_main._quarantine_running_hermes_exe(tmp_path) captured = capsys.readouterr().out @@ -187,11 +216,73 @@ def fake_schedule(s: Path, q: Path) -> bool: # It is NOT added to the returned roll-back list (the issue calls this # out — don't undo a deferred operation). assert pairs == [] + # Still locked until reboot — callers must abort install (#68760). + assert blocked and blocked[0][0] == shim and blocked[0][1] == "reboot_required" # The user got a clear message, not raw [WinError 32]. assert "scheduled" in captured.lower() assert "reboot" in captured.lower() +@patch.object(cli_main, "_is_windows", return_value=True) +def test_quarantine_actionable_warning_when_everything_fails( + _winp, tmp_path, capsys, monkeypatch +): + """When even MoveFileEx fails we should print remediation hints, not a bare error.""" + shim = tmp_path / "hermes.exe" + shim.write_bytes(b"locked") + + def always_fails(self, target): + raise OSError(32, "share violation") + + monkeypatch.setattr(cli_main, "_hermes_exe_shims", lambda d: [shim]) + with patch.object(Path, "rename", always_fails), patch.object( + cli_main, "_schedule_replace_on_reboot", lambda *_a, **_k: False + ), patch("time.sleep", lambda *_a, **_k: None): + pairs, blocked = cli_main._quarantine_running_hermes_exe(tmp_path) + + captured = capsys.readouterr().out + assert pairs == [] + assert blocked and blocked[0][1] == "locked" + # New message format: no raw "[WinError 32]" dump; instead names the cause + # and tells the user what to do. + assert "another process" in captured.lower() + assert "Hermes Desktop" in captured or "gateway" in captured.lower() + + + +@patch.object(cli_main, "_is_windows", return_value=True) +def test_run_quarantined_install_aborts_when_shim_stays_locked( + _winp, tmp_path, capsys, monkeypatch +): + """#68760: do not hand a still-locked hermes.exe to uv/pip.""" + shim = tmp_path / "hermes.exe" + shim.write_bytes(b"locked") + + def always_fails(self, target): + raise OSError(32, "share violation") + + monkeypatch.setattr(cli_main, "_hermes_exe_shims", lambda d: [shim]) + monkeypatch.setattr(cli_main, "_detect_concurrent_hermes_instances", lambda d: [(4242, "hermes.exe")]) + monkeypatch.setattr(cli_main, "_detect_venv_python_processes", lambda: []) + install_called = {"n": 0} + + def boom_install(*_a, **_k): + install_called["n"] += 1 + + with patch.object(Path, "rename", always_fails), patch.object( + cli_main, "_schedule_replace_on_reboot", lambda *_a, **_k: False + ), patch("time.sleep", lambda *_a, **_k: None), patch.object( + cli_main, "_run_install_with_heartbeat", boom_install + ): + with pytest.raises(cli_main.HermesShimLockedError): + cli_main._run_quarantined_install( + ["uv", "pip", "install", "-e", ".[all]"], scripts_dir=tmp_path + ) + + assert install_called["n"] == 0 + out = capsys.readouterr().out + assert "WinError 32" in out or "locked" in out.lower() + assert "4242" in out # --------------------------------------------------------------------------- @@ -524,5 +615,39 @@ def test_unreadable_argv_falls_back_to_the_captured_prefix(monkeypatch): # --------------------------------------------------------------------------- +@patch.object(cli_main, "_is_windows", return_value=True) +def test_run_quarantined_install_aborts_on_reboot_required( + _winp, tmp_path, capsys, monkeypatch +): + """#68760: reboot-deferred quarantine must not proceed to uv/pip.""" + shim = tmp_path / "hermes.exe" + shim.write_bytes(b"locked") + + monkeypatch.setattr( + cli_main, + "_quarantine_running_hermes_exe", + lambda d: ([], [(shim, "reboot_required")]), + ) + install_calls = [] + + def fake_install(cmd, env=None): + install_calls.append(list(cmd)) + + monkeypatch.setattr(cli_main, "_run_install_with_heartbeat", fake_install) + monkeypatch.setattr(cli_main, "_restore_quarantined_exes", lambda moved: None) + monkeypatch.setattr( + cli_main, + "_format_blocked_shim_message", + lambda blocked, scripts_dir: "shim reboot required", + ) + + try: + cli_main._run_quarantined_install(["uv", "pip", "install", "-e", "."], scripts_dir=tmp_path) + raised = False + except cli_main.HermesShimLockedError: + raised = True + assert raised, "reboot_required must raise HermesShimLockedError" + assert install_calls == [], "uv/pip must not run when reboot_required" + assert "shim reboot required" in capsys.readouterr().out diff --git a/tests/hermes_cli/test_verify_core_dependencies.py b/tests/hermes_cli/test_verify_core_dependencies.py index 0404d2bb0e6a..982edad02e27 100644 --- a/tests/hermes_cli/test_verify_core_dependencies.py +++ b/tests/hermes_cli/test_verify_core_dependencies.py @@ -123,6 +123,48 @@ def test_no_pyproject_is_noop(self, tmp_path, monkeypatch): assert not mock_resolve.called assert not mock_install.called + def test_repair_reinstall_quarantines_running_shim_on_windows( + self, temp_pyproject, fake_venv_python + ): + """Regression: the ``--reinstall -e .`` repair must + quarantine the running ``hermes.exe`` on Windows before installing. + + That reinstall rewrites the editable entry-point shims, and on Windows + pip can't overwrite the live launcher — so without quarantine the shim + is left missing and ``hermes`` drops off PATH. Previously this path + called ``_run_install_with_heartbeat`` directly, bypassing the + quarantine that the primary install path performs. + """ + py, venv_root = fake_venv_python + env = {"VIRTUAL_ENV": str(venv_root)} + + probe_calls = {"count": 0} + + def fake_subprocess_run(cmd, **kwargs): + probe_calls["count"] += 1 + # 1st probe: pathspec missing → triggers --reinstall repair. + # 2nd probe (after repair): clean → stops before per-package path. + if probe_calls["count"] == 1: + return MagicMock(returncode=0, stdout="pathspec\n", stderr="") + return MagicMock(returncode=0, stdout="", stderr="") + + fake_scripts = venv_root / "Scripts" # created by fake_venv_python + + with patch("hermes_cli.main._resolve_install_target_python", return_value=py), \ + patch("hermes_cli.main.subprocess.run", side_effect=fake_subprocess_run), \ + patch("hermes_cli.main._is_windows", return_value=True), \ + patch("hermes_cli.main._venv_scripts_dir", return_value=fake_scripts), \ + patch("hermes_cli.main._run_install_with_heartbeat"), \ + patch("hermes_cli.main._quarantine_running_hermes_exe", return_value=([], [])) as mock_quar: + + from hermes_cli.main import _verify_core_dependencies_installed + _verify_core_dependencies_installed(["uv", "pip"], env=env) + + assert mock_quar.called, ( + "the --reinstall -e . repair must quarantine the running " + "hermes.exe on Windows" + ) + assert mock_quar.call_args[0][0] == fake_scripts class TestResolveInstallTargetPython: