fix(update): make the ZIP replace atomic across all entries + dedupe venv layout
Closes #76104, closes #76105. #76104 — `_atomic_replace_dir` (#49145) made each individual directory swap safe, but `_update_via_zip` replaced ~70 top-level entries in a loop with no atomicity across iterations. `agent/` lands at os.listdir index 13 and `tools/` at 66, so an interruption between them left the new `agent/context_compressor.py` (module-level `from tools.todo_tool import TODO_INJECTION_HEADER`) beside a stale `tools/todo_tool.py` — every file valid Python, the tree unbootable. That is the mechanism behind the ImportError fixed in #76091, and the "partial update" field report in #63717. Split into stage-all-then-swap-all: - `_stage_replacement` copies each dir to a sibling staging path, touching nothing live, so a failure during the long copy phase is a no-op. - `_commit_staged_replacements` performs the renames and, if any fails, restores every entry already swapped — the tree lands wholly new or wholly old, never mixed. This shrinks the failure window from a full tree copy to N renames and makes what remains recoverable. Added an up-front free-space check, since staging needs a second copy of the tree; a clear error beats running out mid-swap. #76105 — venv interpreter resolution was open-coded in 7 places across 4 files using 3 different Windows predicates. #76091 added the seventh because the correct behaviour lived 2400 lines away. Hoisted `venv_bin_dir()` / `venv_python_path()` into hermes_constants (import-safe, no new imports) and routed every site through them; `managed_uv._venv_python` now delegates so its 6 callers are untouched. `_atomic_replace_dir` is retained — it is re-exported from main.py and has its own #49145 regression test; removing it is out of scope here. Tests: 10 new (rollback-on-mid-swap-failure is mutation-verified — it fails when the rollback loop is removed), plus a guard that fails if a new call site hand-rolls Scripts/bin again. E2E-verified against the real staging + commit helpers with a live tree.
This commit is contained in:
kshitij
parent
15cb86eba3
commit
83314ca381
|
|
@ -2525,10 +2525,9 @@ def _detect_venv_dir() -> Path | None:
|
|||
def get_python_path() -> str:
|
||||
venv = _detect_venv_dir()
|
||||
if venv is not None:
|
||||
if is_windows():
|
||||
venv_python = venv / "Scripts" / "python.exe"
|
||||
else:
|
||||
venv_python = venv / "bin" / "python"
|
||||
from hermes_constants import venv_python_path
|
||||
|
||||
venv_python = venv_python_path(venv)
|
||||
if venv_python.exists():
|
||||
return str(venv_python)
|
||||
return sys.executable
|
||||
|
|
|
|||
|
|
@ -7963,7 +7963,9 @@ def _venv_scripts_dir() -> Path | None:
|
|||
venv_dir = PROJECT_ROOT / "venv"
|
||||
if not venv_dir.is_dir():
|
||||
return None
|
||||
scripts = venv_dir / ("Scripts" if _is_windows() else "bin")
|
||||
from hermes_constants import venv_bin_dir
|
||||
|
||||
scripts = venv_bin_dir(venv_dir)
|
||||
return scripts if scripts.is_dir() else None
|
||||
|
||||
|
||||
|
|
@ -8756,9 +8758,10 @@ def _resolve_install_target_python(
|
|||
``importlib.metadata`` queries the right site-packages.
|
||||
"""
|
||||
if env and "VIRTUAL_ENV" in env:
|
||||
from hermes_constants import venv_python_path
|
||||
|
||||
venv_root = Path(env["VIRTUAL_ENV"])
|
||||
scripts = venv_root / ("Scripts" if _is_windows() else "bin")
|
||||
candidate = scripts / ("python.exe" if _is_windows() else "python")
|
||||
candidate = venv_python_path(venv_root)
|
||||
if candidate.exists():
|
||||
return candidate
|
||||
|
||||
|
|
|
|||
|
|
@ -384,9 +384,9 @@ def update_managed_uv(
|
|||
|
||||
|
||||
def _venv_python(venv_dir: Path) -> Path:
|
||||
if platform.system() == "Windows":
|
||||
return venv_dir / "Scripts" / "python.exe"
|
||||
return venv_dir / "bin" / "python"
|
||||
from hermes_constants import venv_python_path
|
||||
|
||||
return venv_python_path(venv_dir)
|
||||
|
||||
|
||||
def _remove_tree(path: Path, *, boundary: Path) -> None:
|
||||
|
|
|
|||
|
|
@ -37,6 +37,7 @@ from pathlib import Path
|
|||
from typing import Optional
|
||||
|
||||
from hermes_cli.config import get_hermes_home
|
||||
from hermes_constants import venv_python_path
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
|
|
@ -222,9 +223,7 @@ def _validate_critical_modules_import(root) -> tuple[bool, str | None, str | Non
|
|||
try:
|
||||
interpreter = sys.executable
|
||||
try:
|
||||
bin_dir = "Scripts" if _m()._is_windows() else "bin"
|
||||
python_name = "python.exe" if _m()._is_windows() else "python"
|
||||
venv_python = Path(root) / "venv" / bin_dir / python_name
|
||||
venv_python = venv_python_path(Path(root) / "venv")
|
||||
if venv_python.exists():
|
||||
interpreter = str(venv_python)
|
||||
except Exception:
|
||||
|
|
@ -622,6 +621,64 @@ def _atomic_replace_dir(src: str, dst: str) -> None:
|
|||
if os.path.exists(backup):
|
||||
shutil.rmtree(backup, ignore_errors=True)
|
||||
|
||||
|
||||
def _stage_replacement(src: str, dst: str) -> str:
|
||||
"""Copy *src* to a sibling staging dir for *dst*; return the staging path.
|
||||
|
||||
Phase 1 of the two-phase replace. Touches nothing live, so a failure here
|
||||
leaves the whole install untouched.
|
||||
"""
|
||||
staging = f"{dst}.hermes-update-staging"
|
||||
backup = f"{dst}.hermes-update-old"
|
||||
for leftover in (staging, backup):
|
||||
if os.path.exists(leftover):
|
||||
shutil.rmtree(leftover, ignore_errors=True)
|
||||
shutil.copytree(src, staging)
|
||||
return staging
|
||||
|
||||
|
||||
def _commit_staged_replacements(staged: list[tuple[str, str]]) -> None:
|
||||
"""Phase 2: swap every staged dir into place, rolling back all on failure.
|
||||
|
||||
``_atomic_replace_dir`` makes each *individual* directory swap safe, but
|
||||
the ZIP update replaces ~70 top-level entries in a loop, and nothing made
|
||||
the loop atomic *as a whole*. A failure partway left some entries at the
|
||||
new version and the rest at the old one — every file valid Python, the
|
||||
combination unbootable (issue #76104; the ``ImportError`` in #76091 and
|
||||
the field report in #63717 are both this).
|
||||
|
||||
Splitting stage-all-then-swap-all shrinks the failure window from "the
|
||||
duration of a full tree copy" to "the duration of N renames", and makes
|
||||
the remaining window recoverable: if a rename fails we restore every
|
||||
entry already swapped, so the tree lands wholly new or wholly old.
|
||||
"""
|
||||
swapped: list[tuple[str, str]] = [] # (dst, backup) in swap order
|
||||
try:
|
||||
for staging, dst in staged:
|
||||
backup = f"{dst}.hermes-update-old"
|
||||
if os.path.exists(dst):
|
||||
os.rename(dst, backup)
|
||||
swapped.append((dst, backup))
|
||||
else:
|
||||
swapped.append((dst, ""))
|
||||
os.rename(staging, dst)
|
||||
except OSError:
|
||||
# Undo every swap already made so the install stays self-consistent.
|
||||
for dst, backup in reversed(swapped):
|
||||
try:
|
||||
if os.path.exists(dst):
|
||||
shutil.rmtree(dst, ignore_errors=True)
|
||||
if backup and os.path.exists(backup):
|
||||
os.rename(backup, dst)
|
||||
except OSError:
|
||||
pass # best-effort; the raise below reports the real failure
|
||||
raise
|
||||
# All swaps succeeded — drop the backups (best-effort, never fatal).
|
||||
for _dst, backup in swapped:
|
||||
if backup and os.path.exists(backup):
|
||||
shutil.rmtree(backup, ignore_errors=True)
|
||||
|
||||
|
||||
def _update_via_zip(args):
|
||||
"""Update Hermes Agent by downloading a ZIP archive.
|
||||
|
||||
|
|
@ -700,19 +757,46 @@ def _update_via_zip(args):
|
|||
|
||||
# Copy updated files over existing installation, preserving venv/node_modules/.git
|
||||
preserve = {"venv", "node_modules", ".git", ".env"}
|
||||
update_count = 0
|
||||
for item in os.listdir(extracted):
|
||||
if item in preserve:
|
||||
continue
|
||||
entries = [i for i in os.listdir(extracted) if i not in preserve]
|
||||
|
||||
# Two-phase replace (#76104). Phase 1 copies every directory into a
|
||||
# sibling staging dir without touching anything live; phase 2 swaps
|
||||
# them all in with same-filesystem renames and rolls back every swap
|
||||
# if any one fails. Replacing entries one-at-a-time (the previous
|
||||
# shape) meant an interruption partway left `agent/` new and `tools/`
|
||||
# stale — all files valid, the tree unbootable.
|
||||
#
|
||||
# Staging costs a second copy of the tree on disk. Check up front so
|
||||
# we fail with a clear message instead of running out mid-swap.
|
||||
need = sum(
|
||||
os.path.getsize(os.path.join(dirpath, f))
|
||||
for entry in entries
|
||||
for dirpath, _dirs, files in os.walk(os.path.join(extracted, entry))
|
||||
for f in files
|
||||
if os.path.isfile(os.path.join(dirpath, f))
|
||||
)
|
||||
free = shutil.disk_usage(str(_m().PROJECT_ROOT)).free
|
||||
if free < need * 2:
|
||||
raise RuntimeError(
|
||||
f"not enough free disk space to stage the update safely "
|
||||
f"(need ~{need * 2 // (1024 * 1024)} MB, have "
|
||||
f"{free // (1024 * 1024)} MB)"
|
||||
)
|
||||
|
||||
staged: list[tuple[str, str]] = []
|
||||
plain_files: list[tuple[str, str]] = []
|
||||
for item in entries:
|
||||
src = os.path.join(extracted, item)
|
||||
dst = os.path.join(str(_m().PROJECT_ROOT), item)
|
||||
if os.path.isdir(src):
|
||||
# Atomic-ish replace: never leave dst half-deleted if the copy
|
||||
# fails partway (the failure mode behind #49145 on Windows).
|
||||
_atomic_replace_dir(src, dst)
|
||||
staged.append((_stage_replacement(src, dst), dst))
|
||||
else:
|
||||
shutil.copy2(src, dst)
|
||||
update_count += 1
|
||||
plain_files.append((src, dst))
|
||||
|
||||
_commit_staged_replacements(staged)
|
||||
for src, dst in plain_files:
|
||||
shutil.copy2(src, dst)
|
||||
update_count = len(staged) + len(plain_files)
|
||||
|
||||
print(f"✓ Updated {update_count} items from ZIP")
|
||||
|
||||
|
|
@ -2636,9 +2720,7 @@ def _venv_core_imports_healthy() -> tuple[bool, str]:
|
|||
healthy so a probe failure can't force needless reinstalls.
|
||||
"""
|
||||
venv_dir = _m().PROJECT_ROOT / "venv"
|
||||
python_name = "python.exe" if _m()._is_windows() else "python"
|
||||
bin_dir = "Scripts" if _m()._is_windows() else "bin"
|
||||
venv_python = venv_dir / bin_dir / python_name
|
||||
venv_python = venv_python_path(venv_dir)
|
||||
if not venv_python.exists():
|
||||
# No venv interpreter at all. In a dev checkout that's normal (the
|
||||
# dev may run hermes from any interpreter), so report healthy to
|
||||
|
|
@ -3724,10 +3806,7 @@ def _cmd_update_impl(args, gateway_mode: bool):
|
|||
# repair after the old venv was moved aside) needs the venv
|
||||
# recreated before dependencies can be installed into it.
|
||||
venv_python_missing = not (
|
||||
_m().PROJECT_ROOT
|
||||
/ "venv"
|
||||
/ ("Scripts" if _m()._is_windows() else "bin")
|
||||
/ ("python.exe" if _m()._is_windows() else "python")
|
||||
venv_python_path(_m().PROJECT_ROOT / "venv")
|
||||
).exists()
|
||||
if venv_python_missing and repair_uv:
|
||||
print("→ Recreating virtual environment...")
|
||||
|
|
|
|||
|
|
@ -1250,6 +1250,30 @@ OPENROUTER_MODELS_URL = f"{OPENROUTER_BASE_URL}/models"
|
|||
AI_GATEWAY_BASE_URL = "https://ai-gateway.vercel.sh/v1"
|
||||
|
||||
|
||||
# ─── Venv layout ─────────────────────────────────────────────────────────────
|
||||
|
||||
def venv_bin_dir(venv_dir) -> Path:
|
||||
"""Directory holding a venv's executables (``Scripts`` / ``bin``).
|
||||
|
||||
Single source of truth for venv layout. This was open-coded in seven
|
||||
places across four files using three different Windows predicates
|
||||
(``platform.system()``, ``is_windows()``, ``_is_windows()``); each new
|
||||
call site had to re-derive it, and #76091 shipped an eighth copy because
|
||||
the correct behaviour lived 2400 lines away in another function.
|
||||
|
||||
The path is returned unconditionally — callers legitimately differ on
|
||||
whether a missing venv is an error, so existence checking stays with them.
|
||||
"""
|
||||
return Path(venv_dir) / ("Scripts" if sys.platform == "win32" else "bin")
|
||||
|
||||
|
||||
def venv_python_path(venv_dir) -> Path:
|
||||
"""Path to the Python interpreter inside *venv_dir* (may not exist)."""
|
||||
return venv_bin_dir(venv_dir) / (
|
||||
"python.exe" if sys.platform == "win32" else "python"
|
||||
)
|
||||
|
||||
|
||||
# ─── Partial-update diagnostics ──────────────────────────────────────────────
|
||||
|
||||
# Top-level packages/modules that ship as part of Hermes itself. An ImportError
|
||||
|
|
|
|||
|
|
@ -0,0 +1,178 @@
|
|||
"""Tests for the two-phase ZIP replace and the shared venv-layout helpers.
|
||||
|
||||
``_atomic_replace_dir`` (#49145) made each *individual* directory swap safe,
|
||||
but the ZIP update replaced ~70 top-level entries in a loop with no atomicity
|
||||
across iterations. An interruption partway left some entries at the new
|
||||
version and the rest at the old one -- every file valid Python, the
|
||||
combination unbootable. That is the mechanism behind the ``ImportError`` in
|
||||
#76091 and the field report in #63717.
|
||||
|
||||
Reference: issues #76104 (ZIP atomicity) and #76105 (venv-helper duplication).
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
from hermes_cli import update_cmd
|
||||
from hermes_constants import venv_bin_dir, venv_python_path
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Two-phase replace
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
def _live_tree(root: Path, names: dict[str, str]) -> None:
|
||||
for name, marker in names.items():
|
||||
d = root / name
|
||||
d.mkdir(parents=True, exist_ok=True)
|
||||
(d / "version.txt").write_text(marker)
|
||||
|
||||
|
||||
def _stage_all(root: Path, new: Path, names: list[str]) -> list[tuple[str, str]]:
|
||||
return [
|
||||
(
|
||||
update_cmd._stage_replacement(str(new / n), str(root / n)),
|
||||
str(root / n),
|
||||
)
|
||||
for n in names
|
||||
]
|
||||
|
||||
|
||||
def test_staging_touches_nothing_live(tmp_path):
|
||||
"""Phase 1 must not modify the install -- a failure there is a no-op."""
|
||||
live, new = tmp_path / "live", tmp_path / "new"
|
||||
_live_tree(live, {"agent": "old", "tools": "old"})
|
||||
_live_tree(new, {"agent": "new", "tools": "new"})
|
||||
|
||||
_stage_all(live, new, ["agent", "tools"])
|
||||
|
||||
assert (live / "agent" / "version.txt").read_text() == "old"
|
||||
assert (live / "tools" / "version.txt").read_text() == "old"
|
||||
|
||||
|
||||
def test_commit_swaps_every_entry(tmp_path):
|
||||
live, new = tmp_path / "live", tmp_path / "new"
|
||||
_live_tree(live, {"agent": "old", "tools": "old"})
|
||||
_live_tree(new, {"agent": "new", "tools": "new"})
|
||||
|
||||
update_cmd._commit_staged_replacements(_stage_all(live, new, ["agent", "tools"]))
|
||||
|
||||
assert (live / "agent" / "version.txt").read_text() == "new"
|
||||
assert (live / "tools" / "version.txt").read_text() == "new"
|
||||
# No staging/backup litter left behind.
|
||||
assert not [p for p in os.listdir(live) if "hermes-update" in p]
|
||||
|
||||
|
||||
def test_failed_swap_rolls_back_every_earlier_swap(tmp_path, monkeypatch):
|
||||
"""The regression: a mid-loop failure must not leave a mixed-version tree.
|
||||
|
||||
Before the two-phase split this produced `agent/` new + `tools/` stale --
|
||||
the exact shape that yields
|
||||
`ImportError: cannot import name 'TODO_INJECTION_HEADER'`.
|
||||
"""
|
||||
live, new = tmp_path / "live", tmp_path / "new"
|
||||
_live_tree(live, {"agent": "old", "tools": "old"})
|
||||
_live_tree(new, {"agent": "new", "tools": "new"})
|
||||
staged = _stage_all(live, new, ["agent", "tools"])
|
||||
|
||||
real_rename = os.rename
|
||||
calls = {"n": 0}
|
||||
|
||||
def flaky_rename(src, dst):
|
||||
calls["n"] += 1
|
||||
# Let the first entry swap fully (2 renames), then break the second.
|
||||
if calls["n"] == 4:
|
||||
raise OSError("simulated AV interference")
|
||||
return real_rename(src, dst)
|
||||
|
||||
monkeypatch.setattr(update_cmd.os, "rename", flaky_rename)
|
||||
|
||||
with pytest.raises(OSError):
|
||||
update_cmd._commit_staged_replacements(staged)
|
||||
|
||||
monkeypatch.undo()
|
||||
# Both entries must be back at the OLD version -- not one new, one old.
|
||||
versions = {
|
||||
n: (live / n / "version.txt").read_text() for n in ("agent", "tools")
|
||||
}
|
||||
assert versions == {"agent": "old", "tools": "old"}, (
|
||||
f"mixed-version tree after rollback: {versions}"
|
||||
)
|
||||
|
||||
|
||||
def test_commit_handles_entries_absent_from_the_install(tmp_path):
|
||||
"""A brand-new top-level dir has no live counterpart to move aside."""
|
||||
live, new = tmp_path / "live", tmp_path / "new"
|
||||
live.mkdir()
|
||||
_live_tree(new, {"brand_new": "new"})
|
||||
|
||||
update_cmd._commit_staged_replacements(_stage_all(live, new, ["brand_new"]))
|
||||
|
||||
assert (live / "brand_new" / "version.txt").read_text() == "new"
|
||||
|
||||
|
||||
def test_staging_clears_leftovers_from_an_interrupted_run(tmp_path):
|
||||
live, new = tmp_path / "live", tmp_path / "new"
|
||||
_live_tree(live, {"agent": "old"})
|
||||
_live_tree(new, {"agent": "new"})
|
||||
stale = Path(f"{live / 'agent'}.hermes-update-staging")
|
||||
stale.mkdir()
|
||||
(stale / "junk.txt").write_text("from a previous crash")
|
||||
|
||||
update_cmd._commit_staged_replacements(_stage_all(live, new, ["agent"]))
|
||||
|
||||
assert (live / "agent" / "version.txt").read_text() == "new"
|
||||
assert not (live / "agent" / "junk.txt").exists()
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Shared venv helpers (#76105)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
def test_venv_helpers_agree_with_each_other():
|
||||
v = Path("/opt/proj/venv")
|
||||
assert venv_python_path(v).parent == venv_bin_dir(v)
|
||||
|
||||
|
||||
def test_venv_helpers_accept_str_and_path():
|
||||
assert venv_python_path("/opt/x/venv") == venv_python_path(Path("/opt/x/venv"))
|
||||
|
||||
|
||||
def test_venv_helpers_are_platform_consistent():
|
||||
"""Whatever the platform, the two halves must not disagree."""
|
||||
v = Path("/opt/proj/venv")
|
||||
bin_name = venv_bin_dir(v).name
|
||||
exe_name = venv_python_path(v).name
|
||||
assert (bin_name, exe_name) in {("Scripts", "python.exe"), ("bin", "python")}
|
||||
|
||||
|
||||
def test_managed_uv_helper_delegates_to_the_shared_one():
|
||||
from hermes_cli.managed_uv import _venv_python
|
||||
|
||||
v = Path("/opt/proj/venv")
|
||||
assert _venv_python(v) == venv_python_path(v)
|
||||
|
||||
|
||||
def test_no_open_coded_venv_layout_remains_in_hermes_cli():
|
||||
"""Fails if a new call site hand-rolls Scripts/bin again (#76105)."""
|
||||
import hermes_cli
|
||||
|
||||
pkg = Path(hermes_cli.__file__).parent
|
||||
offenders = []
|
||||
for py in pkg.rglob("*.py"):
|
||||
for lineno, line in enumerate(
|
||||
py.read_text(encoding="utf-8", errors="replace").splitlines(), 1
|
||||
):
|
||||
if '"Scripts"' not in line:
|
||||
continue
|
||||
# Comments and docstrings referencing the path are fine.
|
||||
stripped = line.strip()
|
||||
if stripped.startswith("#") or stripped.startswith(("'", '"', "-")):
|
||||
continue
|
||||
if "if" in line or "/" in line:
|
||||
offenders.append(f"{py.relative_to(pkg)}:{lineno}: {stripped}")
|
||||
assert not offenders, "open-coded venv layout found:\n" + "\n".join(offenders)
|
||||
Loading…
Reference in New Issue