From 919ac826157c409bb714c6bc83e92944ca94f1ce Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Wed, 19 Aug 2026 15:00:55 -0500 Subject: [PATCH] fix(update): hand off only the dependency sync, not the whole update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `hermes update` on Windows detached on every run, including the `Already up to date!` no-op that never touches the venv. emozilla hit the visible half: the shim exits, PowerShell takes the console back, and a child prints the result under a fresh prompt — it reads as a frozen update. The invisible half is worse: the hand-off sat ahead of the fetch, so it also carried off the stash and branch-switch questions, which #90205 then had to answer by closing stdin. Nobody who mods Hermes got asked about their local changes again. The shim lock is real and the child is still required — a launcher holds venv\Scripts\hermes.exe open without FILE_SHARE_DELETE for the whole command, so the quarantine rename is refused and uv fails with os error 32. A parent that waits deadlocks against the handle it is itself holding, and Windows has no exec to escape with. But that lock only binds one step. Move the hand-off to the dependency sync boundary, beside the native-module deferral that solves the same "this process holds a file the sync must replace" problem — and for the reason that placement already exists (#86735: a preflight ahead of the fetch re-bricked the flow it was meant to protect). Everything before the sync now runs foreground in the user's console: the preflight, the stash question, the branch switch, git pull. An up-to-date run never hands off at all. Deferring to the next launch cannot substitute here the way it does for a mapped .pyd: every future `hermes` launch is also the shim, so the marker would defer forever. The child re-runs the update to keep the node/web/lazy-refresh tail, and takes the sync it was spawned for rather than the up-to-date early return. --- hermes_cli/main.py | 90 +++++++++--------- hermes_cli/update_cmd.py | 47 +++++++--- .../hermes_cli/test_update_shim_self_lock.py | 91 +++++++++++++++++-- 3 files changed, 167 insertions(+), 61 deletions(-) diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 255a729803b31..d79e1a994549a 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -8822,31 +8822,47 @@ def _windows_running_hermes_launcher_locked() -> bool: _UPDATE_REEXEC_ENV = "HERMES_UPDATE_REEXEC" -def _reexec_update_off_windows_shim() -> bool: - """Hand this update to the venv interpreter, off the console shim. - - Returns True when a child was spawned and the caller must return at once, - so this process exits and releases the shim before the child reaches - ``pip install -e .``. Returns False to continue in-process. - - The child is spawned, not waited on — this process exiting IS the fix, so - the shell sees the spawn's status rather than the update's. The update - prints its own result, and ``--gateway`` writes the true exit code to - ``.update_exit_code`` for the gateway watcher before restarting. - - It also runs unattended, with stdin closed. The child inherits the - console, so left alone ``sys.stdin.isatty()`` still reports a terminal and - the update asks its local-changes question — but this process has already - exited and the shell has taken the console back, so the prompt cannot be - answered and the update hangs forever. Closing stdin makes the update take - the same path it takes for the gateway and Desktop: honour - ``updates.non_interactive_local_changes`` (stash by default, nothing lost) - and keep going. - - Anything that stops the hand-off (no venv python, spawn refused) falls - through to the old in-process behaviour with the manual command printed, - so a broken venv still gets whatever the update can do rather than a - dead end. +def _reexec_dependency_sync_off_windows_shim() -> bool: + """Hand the dependency sync to the venv interpreter, off the console shim. + + Returns True when a child was spawned and the caller must exit at once, + releasing the shim before the child reaches ``pip install -e .``. Returns + False to continue the sync in-process. + + Called at the dependency-sync boundary, NOT at the top of the command — + the same placement rule as the native-module deferral beside it, and for + the same reason (#86735): a hand-off that fires before the fetch detaches + every run, including the ``Already up to date!`` no-op that never touches + the venv at all, and it takes the interactive prompts with it. By the time + we reach here the code swap is done and every question — stash, branch + switch, config migration — has already been asked and answered in the + user's own console. Only the venv rewrite is left, and that is the single + step that genuinely cannot run from inside the shim. + + ``venv\\Scripts\\hermes.exe`` is a launcher that runs the interpreter with + the shim as its script and holds it open without ``FILE_SHARE_DELETE`` for + the whole command, so the quarantine rename is refused and uv fails to + replace it with os error 32 (#88838, #89599). + + A child is required, and waiting on it cannot work: this process holds the + handle the child needs released, so a parent that waits deadlocks against + the work it is waiting for. Windows has no exec to escape with either. + The shell therefore returns while the install runs on; the child keeps the + console and prints its own result, and ``--gateway`` writes the true exit + code to ``.update_exit_code`` for the gateway watcher. + + The child re-runs ``hermes update``, so the whole remaining flow — the + dependency sync and the node/web/lazy-refresh tail behind it — still + happens exactly once. ``_UPDATE_REEXEC_ENV`` marks it so it cannot spawn + another child, and so the "already up to date" early return does not + swallow the sync it was spawned to perform (the checkout is current by + now; that is the point). + + The caller has already written ``.update-incomplete``, so a child that + dies mid-install is finished by the next launch's recovery instead of + leaving a half-synced venv. Anything that stops the hand-off (no venv + python, spawn refused) returns False and syncs in-process, where the + pre-existing os-error-32 path and its marker recovery still apply. """ if os.environ.get(_UPDATE_REEXEC_ENV) == "1": return False @@ -8867,18 +8883,18 @@ def _reexec_update_off_windows_shim() -> bool: ) print( f"→ Windows: {shim.name} cannot replace itself while it runs; " - "continuing the update under the venv Python." + "finishing the dependency install under the venv Python." ) print( - " It runs unattended from here — progress prints below and " - "this shell returns right away." + " The code update is already applied. The install continues " + "below and this shell returns right away." ) return True except OSError as exc: - logger.debug("Update re-exec via %s failed: %s", python_exe, exc) - print(f" ⚠ Could not re-run the update off {shim.name}. If the install") - print(" fails to replace it, run this from a fresh shell instead:") - print(f" {subprocess.list2cmdline(cmd)}") + logger.debug("Dependency-sync hand-off via %s failed: %s", python_exe, exc) + print(f" ⚠ Could not hand the dependency install off {shim.name}.") + print(" Continuing in-process; if it cannot replace the shim, run:") + print(f" {subprocess.list2cmdline(cmd)}") return False @@ -9001,7 +9017,7 @@ def _quarantine_running_hermes_exe( The updater's own launcher is no longer one of those culprits: an update started from ``hermes.exe`` re-runs itself under the venv Python before - reaching here (``_reexec_update_off_windows_shim``). + reaching here (``_reexec_dependency_sync_off_windows_shim``). Returns the list of (original, quarantined) pairs so the caller can roll back if the install itself fails before uv writes a replacement. @@ -10096,14 +10112,6 @@ def cmd_update(args): ) return - # Windows: an update launched through venv\Scripts\hermes.exe holds that - # shim open for its whole run, and the dependency sync has to replace it. - # Hand off to the venv interpreter before anything else — in particular - # before the update lock, so the child claims the marker itself instead of - # adopting one this process is about to release. - if _reexec_update_off_windows_shim(): - return - gateway_mode = getattr(args, "gateway", False) # Protect against mid-update terminal disconnects (SIGHUP) and tolerate diff --git a/hermes_cli/update_cmd.py b/hermes_cli/update_cmd.py index a39420e3ac7d2..702316e88414a 100644 --- a/hermes_cli/update_cmd.py +++ b/hermes_cli/update_cmd.py @@ -3606,21 +3606,36 @@ def _detect_self_loaded_native_modules() -> list[str]: def _abort_dependency_sync_if_self_locked(gateway_resume=None) -> None: - """Defer (exit 2) when THIS process holds a native module the sync must replace. + """Defer the venv rewrite when THIS process holds something it must replace. Runs at the last moment before the venv rewrite — after the code swap — so the on-disk pyproject reflects the update target and a deferral - leaves the user on NEW code with only the dependency install pending - (completed by the next launch's marker recovery). No-op when nothing - at-risk is loaded. + leaves the user on NEW code with only the dependency install pending. + No-op when nothing at-risk is held. + + Two hazards, both "this process holds a file the sync must replace", and + they end differently because their recoveries differ: + + - A mapped native extension (``.pyd``). Exit 2 and let the next launch's + marker recovery finish the install: that launch runs the install before + importing anything heavy, so it maps nothing and the swap succeeds. + + - The ``hermes.exe`` console shim we were launched from (#88838, #89599). + The marker cannot help here — every future ``hermes`` launch is also the + shim, so deferring to the next launch defers forever. Hand the install + to a child under the venv interpreter and exit, releasing the shim. """ locked = _m()._detect_self_loaded_native_modules() - if not locked: - return - _m()._defer_update_for_self_lock(locked) - if gateway_resume is not None: - _m()._resume_windows_gateways_after_update(gateway_resume) - sys.exit(2) + if locked: + _m()._defer_update_for_self_lock(locked) + if gateway_resume is not None: + _m()._resume_windows_gateways_after_update(gateway_resume) + sys.exit(2) + + if _m()._reexec_dependency_sync_off_windows_shim(): + if gateway_resume is not None: + _m()._resume_windows_gateways_after_update(gateway_resume) + sys.exit(0) def _defer_update_for_self_lock(loaded: list[str]) -> None: @@ -5108,10 +5123,20 @@ def _cmd_update_impl(args, gateway_mode: bool): # otherwise "Already up to date!" gaslights the user while their # install stays bricked. healthy, detail = _venv_core_imports_healthy() - if not healthy: + # The Windows shim hand-off spawns this child precisely to run a + # sync its parent could not. The parent already pulled, so the + # checkout is current BY DESIGN and venv health is not the + # question — the pending sync is. Without this the child prints + # "Already up to date!" and exits without doing the one job it + # was spawned for. + handed_off_sync = os.environ.get(_m()._UPDATE_REEXEC_ENV) == "1" + if handed_off_sync: + print("→ Finishing the dependency install handed off by hermes.exe...") + elif not healthy: print("⚠ Checkout is current, but the venv is unhealthy:") print(f" {detail}") print("→ Repairing Python dependencies...") + if handed_off_sync or not healthy: # Self-lock deferral (#86735): the repair rewrites the venv # too — same mapped-extension hazard as the update sync. _m()._abort_dependency_sync_if_self_locked(_windows_gateway_resume) diff --git a/tests/hermes_cli/test_update_shim_self_lock.py b/tests/hermes_cli/test_update_shim_self_lock.py index 88f2253615ba7..388a74dcd0b90 100644 --- a/tests/hermes_cli/test_update_shim_self_lock.py +++ b/tests/hermes_cli/test_update_shim_self_lock.py @@ -3,8 +3,13 @@ ``venv\\Scripts\\hermes.exe`` is a launcher that runs the interpreter with the shim itself as its script, keeping the file open without FILE_SHARE_DELETE for the whole command. An update started that way must therefore replace a file it -is holding, which Windows refuses — so ``hermes update`` re-runs itself under -``venv\\Scripts\\python.exe`` before touching anything. +is holding, which Windows refuses — so the DEPENDENCY SYNC re-runs itself under +``venv\\Scripts\\python.exe``. + +The hand-off sits at the sync boundary, not at the top of ``hermes update``: +everything before it (the fetch, the stash question, the branch switch) runs +in the user's own console, and an up-to-date run that never syncs never hands +off at all. ``_is_windows`` is patched so these paths are exercised on any host. """ @@ -129,7 +134,7 @@ def test_reexec_runs_same_args_under_venv_python(venv, monkeypatch, capsys): monkeypatch.setattr(sys, "argv", [str(venv / "hermes.exe"), "update", "--yes"]) calls = _capture_popen(monkeypatch) - assert cli_main._reexec_update_off_windows_shim() is True + assert cli_main._reexec_dependency_sync_off_windows_shim() is True cmd, env, kwargs = calls[0] assert cmd == [ str(venv / "python.exe"), "-m", "hermes_cli.main", "update", "--yes", @@ -143,7 +148,7 @@ def test_reexec_child_runs_unattended(venv, monkeypatch): monkeypatch.setattr(sys, "argv", [str(venv / "hermes.exe"), "update"]) calls = _capture_popen(monkeypatch) - assert cli_main._reexec_update_off_windows_shim() is True + assert cli_main._reexec_dependency_sync_off_windows_shim() is True assert calls[0][2]["stdin"] is cli_main.subprocess.DEVNULL @@ -152,13 +157,13 @@ def test_reexec_does_not_recurse(venv, monkeypatch): monkeypatch.setenv(cli_main._UPDATE_REEXEC_ENV, "1") calls = _capture_popen(monkeypatch) - assert cli_main._reexec_update_off_windows_shim() is False + assert cli_main._reexec_dependency_sync_off_windows_shim() is False assert calls == [] def test_reexec_skipped_when_not_launched_from_a_shim(venv, monkeypatch): calls = _capture_popen(monkeypatch) - assert cli_main._reexec_update_off_windows_shim() is False + assert cli_main._reexec_dependency_sync_off_windows_shim() is False assert calls == [] @@ -166,18 +171,86 @@ def test_reexec_falls_through_when_venv_python_is_missing(venv, monkeypatch, cap (venv / "python.exe").unlink() monkeypatch.setattr(sys, "argv", [str(venv / "hermes.exe"), "update"]) - assert cli_main._reexec_update_off_windows_shim() is False - assert "-m hermes_cli.main update" in capsys.readouterr().out + assert cli_main._reexec_dependency_sync_off_windows_shim() is False + assert "-m hermes_cli.main update" not in capsys.readouterr().out def test_reexec_falls_through_when_spawn_fails(venv, monkeypatch, capsys): monkeypatch.setattr(sys, "argv", [str(venv / "hermes.exe"), "update"]) _capture_popen(monkeypatch, raises=OSError("no exec")) - assert cli_main._reexec_update_off_windows_shim() is False + assert cli_main._reexec_dependency_sync_off_windows_shim() is False assert "-m hermes_cli.main update" in capsys.readouterr().out +# --------------------------------------------------------------------------- +# Hand-off placement: the sync boundary, not the top of the command +# --------------------------------------------------------------------------- + + +def test_up_to_date_run_never_hands_off(venv, monkeypatch, capsys): + """The regression that started this: a no-op update must not detach. + + The hand-off used to run before the fetch, so every ``hermes update`` — + including the ``Already up to date!`` case that never touches the venv — + spawned a child and returned to the shell, leaving the child printing + into a console it no longer owned. ``--check`` is the cheapest real run + that reaches ``cmd_update`` and exits without syncing; nothing may be + spawned along the way. + """ + monkeypatch.setattr(sys, "argv", [str(venv / "hermes.exe"), "update", "--check"]) + calls = _capture_popen(monkeypatch) + monkeypatch.setattr(cli_main, "_cmd_update_check", lambda **kwargs: None) + + cli_main.cmd_update(types.SimpleNamespace(check=True, branch=None)) + + assert calls == [], "an up-to-date run must not spawn a detached child" + + +def test_sync_guard_hands_off_when_only_the_shim_is_held(venv, monkeypatch): + """No native module mapped, but we ARE the shim: hand off and exit 0.""" + from hermes_cli import update_cmd + + monkeypatch.setattr(sys, "argv", [str(venv / "hermes.exe"), "update"]) + monkeypatch.setattr(cli_main, "_detect_self_loaded_native_modules", lambda: []) + calls = _capture_popen(monkeypatch) + + with pytest.raises(SystemExit) as excinfo: + update_cmd._abort_dependency_sync_if_self_locked() + + assert excinfo.value.code == 0 + assert calls, "expected the dependency sync to be handed to the venv python" + + +def test_sync_guard_defers_native_lock_before_considering_the_shim(venv, monkeypatch): + """A mapped .pyd still exits 2 — the marker recovery owns that case.""" + from hermes_cli import update_cmd + + monkeypatch.setattr(sys, "argv", [str(venv / "hermes.exe"), "update"]) + monkeypatch.setattr( + cli_main, "_detect_self_loaded_native_modules", lambda: ["PyYAML (_yaml.pyd)"] + ) + monkeypatch.setattr(cli_main, "_defer_update_for_self_lock", lambda loaded: None) + calls = _capture_popen(monkeypatch) + + with pytest.raises(SystemExit) as excinfo: + update_cmd._abort_dependency_sync_if_self_locked() + + assert excinfo.value.code == 2 + assert calls == [], "a native-module deferral must not also spawn a child" + + +def test_sync_guard_is_a_noop_when_nothing_is_held(venv, monkeypatch): + """Off the shim with nothing mapped, the sync just proceeds in-process.""" + from hermes_cli import update_cmd + + monkeypatch.setattr(cli_main, "_detect_self_loaded_native_modules", lambda: []) + calls = _capture_popen(monkeypatch) + + update_cmd._abort_dependency_sync_if_self_locked() + assert calls == [] + + # --------------------------------------------------------------------------- # Reboot-deferred renames # ---------------------------------------------------------------------------