fix(update): complete pending core install before any native import (self-lock loop fix)

Reviewer egilewski found the original defer was circular (#83590 comment):
the self-lock preflight wrote .update-incomplete and exited, but the next
launch only ran the full recovery AFTER main.py's third-party imports —
so a healthy venv's probes made the early pass a no-op, main.py imported
cryptography eagerly, the .pyd got mapped again, and the deferred install
re-hit the exact self-lock it was meant to escape.

Close the loop by making the marker guarantee the install runs BEFORE any
native extension can be imported:

- hermes_cli/_install_repair.py (new, stdlib-only): single source of truth
  for the core .[all] reinstall — ensurepip bootstrap, uv-pip/pip
  resolution with VIRTUAL_ENV, Termux env stripping, Windows hermes*.exe
  quarantine, per-extra fallback ladder, and fd1→fd2 routing for acp
  safety.  Deliberately free of managed_uv/hermes_constants imports so it
  stays importable in the corrupted-venv state it exists to repair.
- hermes_cli/_early_recovery.py: recover_if_needed now completes a pending
  .update-incomplete install BEFORE the import probes, on every launch
  that sees the marker (unless argv is update).  Success clears the
  marker; failure bumps an attempts counter inside the marker body and
  keeps it.  A 3-attempt ceiling stops a persistently-failing install
  from reinstall-hammering every launch (hermes acp included) — past the
  ceiling the late post-import recovery takes over with its manual
  recovery instructions.  Single-flight lock shared with the late path.
- hermes_cli/main.py: _recover_core_update_marker_locked delegates the
  install to the shared executor (no duplicated logic); ensure_uv stays
  in the late path so a venv whose uv vanished mid-update still
  bootstraps it.
- tests: 7 new regressions — the reviewer's exact case (marker + healthy
  venv → install runs while sys.modules has no cryptography), failure
  keeps marker + increments attempts, retry ceiling, lazy marker does
  not trigger core install (#58004 invariant), argv-update skip, and
  corrupt/missing marker bodies.  The key test was sabotage-verified:
  removing the pre-import branch makes it fail with zero install calls,
  while a lone-lazy-marker test still passes; restoring the branch makes
  it pass again.

Refs #83569
This commit is contained in:
Halldrix
2026-08-11 09:29:52 -05:00
committed by Teknium
parent c6a71294b6
commit 19cff89300
5 changed files with 621 additions and 30 deletions
+138
View File
@@ -309,6 +309,20 @@ def recover_if_needed(
if not (root / "pyproject.toml").is_file():
return
# Pending core install (.update-incomplete) — complete it NOW, before
# any native extension can be imported by this process. The lazy
# import-probe path below only proves main.py is importable; the core
# install here guarantees the WHOLE dependency set is replaced while
# nothing pins venv .pyd files yet (#83569 self-lock: deferring the
# install to main()'s post-import recovery re-locks it on Windows).
# Bounded retries: a persistently failing install must not hammer
# every launch, so attempts past the ceiling are left for main.py's
# post-import recovery path (which can safely probe-import after this
# process already holds whatever extensions it needs).
if core_marker.exists():
_complete_pending_core_install(root, core_marker)
return
broken = _probe_broken_packages()
if not broken:
# Imports are fine — main.py will load and run full recovery.
@@ -358,3 +372,127 @@ def recover_if_needed(
except Exception:
# Never block launch — the import of main.py will surface the truth.
pass
# Cap on automatic early-pass install retries. A persistently failing
# install (e.g. network down, index unreachable) must not reinstall-hammer
# every `hermes` launch: past this many attempts the early pass hands the
# marker to main.py's post-import recovery, which presents the manual
# recovery command. The counter lives inside the marker file itself (JSON
# body) and is bumped on each failed attempt.
_EARLY_CORE_INSTALL_MAX_ATTEMPTS = 3
def _claim_recovery_lock(root: Path) -> bool:
"""Single-flight claim on the shared recovery lock. Never raises."""
lock_path = root / ".update-incomplete.lock"
try:
fd = os.open(lock_path, os.O_CREAT | os.O_EXCL | os.O_WRONLY)
os.write(fd, f"{os.getpid()}\n".encode())
os.close(fd)
return True
except FileExistsError:
try:
if time.time() - lock_path.stat().st_mtime > 3600:
lock_path.unlink()
except OSError:
pass
return False
except OSError:
# Read-only fs / perms — proceed unlocked; the install itself
# surfaces the real problem. Recoverable.
return True
def _release_recovery_lock(root: Path) -> None:
"""Best-effort release of the shared recovery lock."""
try:
(root / ".update-incomplete.lock").unlink()
except OSError:
pass
def _complete_pending_core_install(root: Path, core_marker: Path) -> None:
"""Run the pending core install BEFORE main.py can import native modules.
``recover_if_needed`` invokes this when ``.update-incomplete`` exists —
a prior ``hermes update`` (or the self-lock preflight, #83569) left the
dependency sync deliberately unfinished. Completing it here matters on
Windows: the deferral exists precisely because the process that wrote the
marker had a native venv extension mapped; this process, running before
``hermes_cli.main``'s third-party imports, maps nothing yet, so the
installer can replace ``.pyd`` files without hitting the lock.
Marker lifecycle: cleared on success; kept (attempts counter bumped) on
failure for the next launch or main.py's post-import recovery. An
attempts ceiling caps automatic retries so a persistent installer
failure does not block every launch (``hermes acp`` included).
Never raises: any failure leaves the marker for the post-import path.
"""
try:
from hermes_cli import _install_repair as ir
# Retry backoff: read current attempts before claiming the lock so a
# persistently-failing install stops hammering early. After the
# increment the counter reflects THIS attempt.
attempts = 0
try:
raw = core_marker.read_text(encoding="utf-8", errors="replace").strip()
if raw:
import json as _json
try:
attempts = int(_json.loads(raw).get("attempts", 0))
except (ValueError, AttributeError):
attempts = 0
except OSError:
attempts = 0
if attempts >= _EARLY_CORE_INSTALL_MAX_ATTEMPTS:
print(
"⚠ Pending interrupted-update install has already failed "
f"{attempts} times in the early pass — leaving it for the "
"post-import recovery path.",
file=sys.stderr,
)
return
if not _claim_recovery_lock(root):
return
try:
print(
"⚠ A previous `hermes update` was interrupted mid-install — "
"finishing dependency installation now (before any native "
"extensions load)...",
file=sys.stderr,
)
ir.run_core_install(root)
except Exception as exc:
new_attempts = ir.bump_marker_attempts(core_marker)
print(
f" ✗ Early interrupted-install completion failed (attempt "
f"{new_attempts}/{_EARLY_CORE_INSTALL_MAX_ATTEMPTS}): {exc}",
file=sys.stderr,
)
print(
" The next launch will retry; hermes will keep working from "
"the current venv in the meantime.",
file=sys.stderr,
)
return
finally:
_release_recovery_lock(root)
try:
core_marker.unlink()
except OSError:
pass
print(
" ✓ Dependency installation completed in the early pass.",
file=sys.stderr,
)
except Exception:
# Never block launch — the marker stays for the post-import path.
pass
+304
View File
@@ -0,0 +1,304 @@
"""Dependency install execution shared between early recovery and full recovery.
Both callers need to run the same core ``.[all]`` reinstall:
- ``hermes_cli._early_recovery.recover_if_needed`` — stdlib-only, runs BEFORE
``hermes_cli.main``'s third-party imports, so it can complete a pending
update while no native extension is mapped yet (#83569).
- ``hermes_cli.main._recover_core_update_marker_locked`` — the historical
post-import recovery path. Kept as a fallback for installs the early pass
could not complete (marker left in place on failure).
This module is deliberately **stdlib-only** so importing it can never fail in
the corrupted-venv state it exists to repair. ``hermes_cli.main`` imports
``managed_uv``, ``hermes_constants``, and friends only in its late path; the
early path must not. Where the late path uses ``managed_uv.ensure_uv`` to
bootstrap uv if missing, the early path uses the stdlib
:func:`hermes_cli._early_recovery._find_uv_binary` lookup and falls back to
plain pip when uv is absent — a degraded but working installer (the late
recovery will bootstrap uv on the next launch if it ever matters).
"""
from __future__ import annotations
import contextlib
import json
import os
import subprocess
import sys
import time
from pathlib import Path
# Single source of truth for the recovery-lock lifecycle and uv lookup —
# _early_recovery already owns both, and importing it is free (stdlib-only).
from hermes_cli import _early_recovery as _er
def _is_windows() -> bool:
return sys.platform == "win32"
def _is_termux_env(env: dict | None = None) -> bool:
"""Stdlib Termux probe (hermes_cli.main's version lives behind imports)."""
env = env if env is not None else os.environ
try:
if env.get("TERMUX_VERSION"):
return True
prefix = env.get("PREFIX", "")
return "com.termux" in prefix
except Exception:
return False
@contextlib.contextmanager
def _stdout_to_stderr():
"""Route fd 1 (and sys.stdout) to stderr for the duration of an install.
``hermes acp`` speaks JSON-RPC on stdout; an inherited-fd install child
writing there would corrupt the protocol. Mirrors
``main.py::_recover_from_interrupted_install``.
"""
saved_fd = None
saved_sys_stdout = sys.stdout
try:
saved_fd = os.dup(1)
os.dup2(2, 1)
except OSError:
saved_fd = None
sys.stdout = sys.stderr
try:
yield
finally:
sys.stdout = saved_sys_stdout
if saved_fd is not None:
try:
os.dup2(saved_fd, 1)
except OSError:
pass
try:
os.close(saved_fd)
except OSError:
pass
def _resolve_install_target(root: Path) -> tuple[list[str], dict | None]:
"""(install_cmd_prefix, env) for the project venv — stdlib uv lookup.
Mirrors ``main.py::_default_venv_install_target`` but without
``managed_uv``. ``VIRTUAL_ENV`` steers ``uv pip`` at the project venv even
when invoked from the base interpreter (the early-recovery case).
Termux strips leaked interpreter-path env vars so uv resolves the venv
correctly.
"""
uv_bin = _er._find_uv_binary()
if uv_bin:
env = {**os.environ, "VIRTUAL_ENV": str(root / "venv")}
if _is_termux_env(env):
env.pop("PYTHONPATH", None)
env.pop("PYTHONHOME", None)
return [uv_bin, "pip"], env
return [sys.executable, "-m", "pip"], None
def _venv_scripts_dir(root: Path) -> Path | None:
"""Project venv Scripts/bin dir, when present. stdlib-only."""
venv_dir = root / "venv"
if not venv_dir.is_dir():
return None
scripts = venv_dir / ("Scripts" if _is_windows() else "bin")
return scripts if scripts.is_dir() else None
def _load_console_script_names(root: Path) -> list[str]:
"""``[project.scripts]`` names from pyproject.toml (tomllib, 3.11+)."""
try:
import tomllib
except ImportError: # pragma: no cover
return []
pyproject = root / "pyproject.toml"
if not pyproject.is_file():
return []
try:
with open(pyproject, "rb") as f:
data = tomllib.load(f)
scripts = data.get("project", {}).get("scripts", {}) or {}
return [str(name) for name in scripts if name]
except Exception:
return []
def _quarantine_running_hermes_exe(scripts_dir: Path) -> list[tuple[Path, Path]]:
"""Rename live hermes*.exe shims aside so the installer can rewrite them.
Windows blocks REPLACE on a running .exe but allows RENAME. Best-effort:
silently skips anything that cannot be renamed. Returns (original,
quarantined) pairs. stdlib-only — the console-script set comes from
pyproject ``[project.scripts]`` (fallback: the well-known trio).
"""
if not _is_windows():
return []
names = set(_load_console_script_names(scripts_dir.parent.parent)) or {
"hermes",
"hermes-agent",
"hermes-acp",
}
names.add("hermes-gateway")
moved: list[tuple[Path, Path]] = []
for name in sorted(names):
shim = scripts_dir / f"{name}.exe"
if not shim.exists():
continue
quarantined = shim.with_name(f"{name}.exe.old.{int(time.time() * 1000)}")
try:
os.rename(shim, quarantined)
moved.append((shim, quarantined))
except OSError:
pass
return moved
def _restore_quarantined_exes(moved: list[tuple[Path, Path]]) -> None:
"""Put quarantined shims back when the installer did not replace them."""
for original, quarantined in moved:
if original.exists():
continue # installer wrote a fresh shim — the .old one is garbage
try:
os.rename(quarantined, original)
except OSError:
pass
def _run_install_cmd(cmd: list[str], *, env: dict | None, root: Path) -> None:
"""Run an install command with quarantine protection for venv shims.
Raises CalledProcessError on install failure (callers implement the
per-extra fallback ladder).
"""
scripts_dir = _venv_scripts_dir(root) if _is_windows() else None
moved = _quarantine_running_hermes_exe(scripts_dir) if scripts_dir else []
try:
subprocess.run(cmd, cwd=root, check=True, env=env)
except BaseException:
if scripts_dir is not None:
_restore_quarantined_exes(moved)
raise
def _load_installable_optional_extras(root: Path, group: str) -> list[str]:
"""Optional extras referenced by a dependency group (all / termux-all)."""
try:
import tomllib
with (root / "pyproject.toml").open("rb") as handle:
project = tomllib.load(handle).get("project", {})
except Exception:
return []
optional_deps = project.get("optional-dependencies", {})
if not isinstance(optional_deps, dict):
return []
refs = optional_deps.get(group, [])
referenced: list[str] = []
for ref in refs:
if "[" in ref and "]" in ref:
name = ref.split("[", 1)[1].split("]", 1)[0]
if name in optional_deps:
referenced.append(name)
return referenced
def run_core_install(root: Path) -> None:
"""Full core ``.[all]`` editable reinstall — the recovery install.
Equal in behavior to the install half of
``main.py::_recover_core_update_marker_locked``:
- bootstrap pip via ensurepip (a killed install can leave the venv with no
pip module at all)
- prefer ``uv pip`` with VIRTUAL_ENV pointed at the project venv; fall back
to ``python -m pip`` when no uv binary is available
- target ``.[all]`` (or ``.[termux-all]`` on Termux) with the per-extra
fallback ladder when the combined extras resolve fails
- quarantine live ``hermes*.exe`` shims on Windows so they can be replaced
- route ALL install output to stderr (acp/JSON-RPC safety)
- Termux strips leaked PYTHONPATH/PYTHONHOME from the uv env
Raises ``subprocess.CalledProcessError`` when even the base install fails;
callers own marker lifecycle (clear on success, keep on failure).
"""
prefix, env = _resolve_install_target(root)
group = "termux-all" if _is_termux_env(env) else "all"
with _stdout_to_stderr():
try:
subprocess.run(
[sys.executable, "-m", "ensurepip", "--upgrade", "--default-pip"],
cwd=root,
capture_output=True,
)
except Exception:
pass
try:
_run_install_cmd(
prefix + ["install", "-e", f".[{group}]"], env=env, root=root
)
return
except subprocess.CalledProcessError:
print(
" ⚠ Optional extras failed, reinstalling base dependencies "
"and retrying extras individually..."
)
_run_install_cmd(prefix + ["install", "-e", "."], env=env, root=root)
failed_extras: list[str] = []
installed_extras: list[str] = []
for extra in _load_installable_optional_extras(root, group):
try:
_run_install_cmd(
prefix + ["install", "-e", f".[{extra}]"], env=env, root=root
)
installed_extras.append(extra)
except subprocess.CalledProcessError:
failed_extras.append(extra)
if installed_extras:
print(
" ✓ Reinstalled optional extras individually: "
+ ", ".join(installed_extras)
)
if failed_extras:
print(
" ⚠ Skipped optional extras that still failed: "
+ ", ".join(failed_extras)
)
# ---------------------------------------------------------------------------
# Marker metadata (attempt counter for early-pass retry backoff)
# ---------------------------------------------------------------------------
def bump_marker_attempts(marker_path: Path) -> int:
"""Increment an attempts counter stored inside the marker file.
The marker's existence is the signal; opportunistic JSON body carries the
retry count so a persistently failing install can back off instead of
reinstall-hammering every launch. Corrupt/missing bodies restart at 1.
Returns the new attempt count. Never raises.
"""
attempts = 0
try:
raw = marker_path.read_text(encoding="utf-8", errors="replace").strip()
if raw:
try:
attempts = int(json.loads(raw).get("attempts", 0))
except (ValueError, AttributeError):
attempts = 0
except OSError:
attempts = 0
attempts += 1
try:
marker_path.write_text(json.dumps({"attempts": attempts}), encoding="utf-8")
except OSError:
pass
return attempts
+12 -27
View File
@@ -8188,36 +8188,21 @@ def _recover_core_update_marker_locked() -> None:
_repair_venv_via_import_probes(install_prefix, env=install_env)
try:
from hermes_cli import _install_repair as _ir
# ensure_uv bootstraps the installer itself when missing (the early
# pass's stdlib-only lookup cannot); keeping it here means the late
# path still self-heals a venv whose uv vanished mid-update.
from hermes_cli.managed_uv import ensure_uv
# Always bootstrap pip first: a killed install can leave the venv with
# no pip module at all, and uv may also be gone. ensurepip restores a
# known-good pip so at least the plain-pip path below can proceed.
try:
subprocess.run(
[sys.executable, "-m", "ensurepip", "--upgrade", "--default-pip"],
cwd=PROJECT_ROOT,
capture_output=True,
)
except Exception as exc:
logger.debug("ensurepip during install recovery failed: %s", exc)
ensure_uv()
uv_bin = ensure_uv()
if uv_bin:
uv_env = {**os.environ, "VIRTUAL_ENV": str(PROJECT_ROOT / "venv")}
if _is_termux_env(uv_env):
uv_env.pop("PYTHONPATH", None)
uv_env.pop("PYTHONHOME", None)
_install_python_dependencies_with_optional_fallback(
[uv_bin, "pip"],
env=uv_env,
group="termux-all" if _is_termux_env(uv_env) else "all",
)
else:
_install_python_dependencies_with_optional_fallback(
[sys.executable, "-m", "pip"],
group="termux-all" if _is_termux_env() else "all",
)
# Delegate the install itself to the shared stdlib executor so both
# this late path and the pre-import early pass run exactly the same
# reinstall. Called inside the same stdout→stderr redirect already
# established by _recover_from_interrupted_install, so
# run_core_install's own redirect nests harmlessly.
_ir.run_core_install(PROJECT_ROOT)
_clear_update_incomplete_marker()
print("✓ Dependency installation recovered — your install is healthy again.")
+161
View File
@@ -327,6 +327,167 @@ def test_externally_managed_detection(tmp_path, monkeypatch):
assert er._base_interpreter_is_externally_managed() is True
# ---------------------------------------------------------------------------
# Pending core install (.update-incomplete) — completed BEFORE native imports
# (#83569 review: a deferred update must not re-lock itself on the next launch)
# ---------------------------------------------------------------------------
def test_core_marker_triggers_install_before_any_native_import(
tmp_path, monkeypatch
):
"""The reviewer's exact case (comment 5254279935): ``.update-incomplete``
present, venv HEALTHY (import probes would pass). The early pass must
STILL run the core install — crucially while no native extension module
is loaded in this process — because deferring to main()'s post-import
recovery lets a recurring eager import remap the .pyd first."""
root = _project(tmp_path)
core_marker = root / ".update-incomplete"
core_marker.write_text('{"attempts": 0}', encoding="utf-8")
from hermes_cli import _install_repair as ir
calls: list[dict] = []
def fake_install(project_root):
calls.append(
{
"root": project_root,
"native_loaded_at_call": sorted(
m for m in sys.modules if m.startswith("cryptography")
),
}
)
monkeypatch.setattr(ir, "run_core_install", fake_install)
# Early recovery imports _install_repair lazily inside the helper; make
# sure the lazy import resolves to the SAME monkeypatched module object.
import hermes_cli._install_repair # noqa: F401 (pre-import for patch)
er.recover_if_needed(project_root=root, argv=[])
assert len(calls) == 1, "core install must run when the marker exists"
assert calls[0]["root"] == root
assert calls[0]["native_loaded_at_call"] == [], (
"install must run BEFORE any cryptography module is loaded "
"(that is the whole point of the early pass)"
)
assert not core_marker.exists(), "marker cleared on success"
# And the lazy import-probe repair path must NOT also fire:
# (no probe repair attempted — cryptography is irrelevant to this branch)
def test_core_marker_marks_attempts_and_keeps_marker_on_install_failure(
tmp_path, monkeypatch
):
root = _project(tmp_path)
core_marker = root / ".update-incomplete"
core_marker.write_text('{"attempts": 0}', encoding="utf-8")
from hermes_cli import _install_repair as ir
def boom(_project_root):
raise RuntimeError("simulated install failure")
monkeypatch.setattr(ir, "run_core_install", boom)
import hermes_cli._install_repair # noqa: F401
er.recover_if_needed(project_root=root, argv=[])
assert core_marker.exists(), "failure keeps the marker for the next try"
import json
body = json.loads(core_marker.read_text(encoding="utf-8"))
assert body["attempts"] == 1
# Recovery lock released even on failure (next launch may retry).
assert not (root / ".update-incomplete.lock").exists()
def test_core_marker_retry_ceiling_hands_off_to_late_recovery(
tmp_path, monkeypatch
):
"""A persistently failing install must not reinstall-hammer every launch."""
root = _project(tmp_path)
core_marker = root / ".update-incomplete"
core_marker.write_text(
f'{{"attempts": {er._EARLY_CORE_INSTALL_MAX_ATTEMPTS}}}', encoding="utf-8"
)
from hermes_cli import _install_repair as ir
monkeypatch.setattr(
ir,
"run_core_install",
lambda _r: (_ for _ in ()).throw(
AssertionError("install must NOT run past the attempts ceiling")
),
)
import hermes_cli._install_repair # noqa: F401
er.recover_if_needed(project_root=root, argv=[])
assert core_marker.exists(), "marker retained for main.py's late recovery"
# Counter not bumped further by the skipped attempt.
def test_lazy_marker_alone_does_not_trigger_core_install(tmp_path, monkeypatch):
"""Invariant guard: a lone ``.lazy-refresh-incomplete`` must NOT trigger
the core-install branch (lazy repair has its own narrow probe path and
must NEVER clear the core marker per #58004)."""
root = _project(tmp_path)
(root / ".lazy-refresh-incomplete").write_text("x", encoding="utf-8")
from hermes_cli import _install_repair as ir
monkeypatch.setattr(
ir,
"run_core_install",
lambda _r: (_ for _ in ()).throw(
AssertionError("core install must not run for the lazy marker")
),
)
import hermes_cli._install_repair # noqa: F401
# Healthy probes → early pass does nothing (preserves existing behavior).
monkeypatch.setattr(er, "_probe_broken_packages", lambda: [])
er.recover_if_needed(project_root=root, argv=[])
def test_core_marker_skipped_when_user_is_running_update(tmp_path, monkeypatch):
"""A launch of `hermes update` itself must not race its own markers."""
root = _project(tmp_path)
core_marker = root / ".update-incomplete"
core_marker.write_text('{"attempts": 0}', encoding="utf-8")
from hermes_cli import _install_repair as ir
monkeypatch.setattr(
ir,
"run_core_install",
lambda _r: (_ for _ in ()).throw(
AssertionError("install must not run for `hermes update` argv")
),
)
import hermes_cli._install_repair # noqa: F401
er.recover_if_needed(project_root=root, argv=["update"])
assert core_marker.exists()
def test_bump_marker_attempts_handles_missing_and_corrupt_bodies(tmp_path):
from hermes_cli import _install_repair as ir
m = tmp_path / ".update-incomplete"
m.write_text("", encoding="utf-8")
assert ir.bump_marker_attempts(m) == 1
m.write_text("not json", encoding="utf-8")
assert ir.bump_marker_attempts(m) == 1
m.write_text('{"attempts": 2}', encoding="utf-8")
assert ir.bump_marker_attempts(m) == 3
@@ -41,10 +41,13 @@ def _stub_install_env(monkeypatch, m, seen):
monkeypatch.setattr(m.subprocess, "run", lambda *a, **k: R())
monkeypatch.setattr(m, "_is_termux_env", lambda *a, **k: False)
monkeypatch.setattr("hermes_cli.managed_uv.ensure_uv", lambda: None)
# The install executor moved to hermes_cli._install_repair (shared between
# the pre-import early pass and this late recovery path) — stub WHERE it
# is executed, not the legacy main.py wrapper it replaced.
import hermes_cli._install_repair as ir
monkeypatch.setattr(
m,
"_install_python_dependencies_with_optional_fallback",
lambda *a, **k: seen.__setitem__("install", True),
ir, "run_core_install", lambda _root: seen.__setitem__("install", True)
)