fix(update): extend atomicity to top-level files, clean up failed staging
Phase 2 review findings on the first commit. C1 (critical) — the two-phase replace covered directories only, so the 20 first-party modules at the repo root (run_agent.py, cli.py, hermes_constants.py, model_tools.py, toolsets.py, ...) were still copied one-at-a-time with shutil.copy2 straight onto live paths. A failure in that loop left all directories new and the root modules stale: precisely the ImportError shape this PR exists to prevent. Worse, copy2 truncates in place, so a crash mid-copy could leave a half-written cli.py — strictly worse than stale on the flaky-AV path this code runs on. Stage files the same way as directories and swap them in the same commit phase. The docstring's "wholly new or wholly old" is now actually true. C2 (critical) — a phase-1 failure (disk exhaustion being the likely one) orphaned one staging copy per entry already processed, up to a second copy of the tree. The user then follows our "re-run hermes update" advice with LESS free space and the retry fails harder. Added _discard_staged() on the staging path. Verified: staging failure now leaves zero litter. W1 — _stage_replacement duplicated _atomic_replace_dir's first half verbatim. _atomic_replace_dir is now a 1-line shim over the two-phase helpers; its #49145 regression test still passes. W2 — the failure message still said "some directories were replaced and others were not", which the fix makes false. Now says the install was left in place. W3 — the free-space gate demanded 2x the tree when only the staging copy is new (the live tree already occupies its space; swaps are renames). Relaxed to need * 1.2, so we stop blocking updates that would have succeeded on the space-constrained machines most likely to hit this. W5/W6 — the lint-style guard used `"if" in line`, which matches "modify" and "verify" and still missed os.path.join(venv, "Scripts"). Rewritten as an AST check; it immediately found the real offender the substring version missed (stdio.py, now explicitly exempted — it lists literal Windows-only PATH candidates, not a cross-platform derivation). Softened venv_bin_dir's "single source of truth" claim, since sites outside hermes_cli/ remain. S1 — the rollback loop now logs instead of silently swallowing OSError. Both C1 and C2 fixes are mutation-verified: reverting either makes the new tests fail.
This commit is contained in:
kshitij
parent
83314ca381
commit
c1f36f5293
|
|
@ -594,65 +594,77 @@ def _atomic_replace_dir(src: str, dst: str) -> None:
|
|||
is already gone and nothing replaced it — the install is left with a
|
||||
deleted tree (issue #49145, where ``ui-tui/`` vanished and broke the TUI).
|
||||
|
||||
Instead, stage the new copy into a sibling temp dir first; only once that
|
||||
fully succeeds do we swap it in. A failure during staging raises with the
|
||||
original *dst* still intact.
|
||||
Now a thin single-entry alias over the two-phase helpers below, which
|
||||
generalise the same stage-then-swap discipline across every entry the ZIP
|
||||
update touches (#76104). Retained because it is part of the mechanical
|
||||
``hermes_cli.main`` re-export surface and guards the #49145 regression.
|
||||
"""
|
||||
staging = f"{dst}.hermes-update-staging"
|
||||
backup = f"{dst}.hermes-update-old"
|
||||
# Clear any leftovers from a previously-interrupted update.
|
||||
for leftover in (staging, backup):
|
||||
if os.path.exists(leftover):
|
||||
shutil.rmtree(leftover, ignore_errors=True)
|
||||
|
||||
# 1. Stage the new copy. If this fails, dst is untouched.
|
||||
shutil.copytree(src, staging)
|
||||
# 2. Swap: move the live dir aside, move staging into place. Both moves are
|
||||
# same-filesystem renames; if the second fails we restore the backup.
|
||||
if os.path.exists(dst):
|
||||
os.rename(dst, backup)
|
||||
try:
|
||||
os.rename(staging, dst)
|
||||
except OSError:
|
||||
if os.path.exists(backup) and not os.path.exists(dst):
|
||||
os.rename(backup, dst) # roll back to the original
|
||||
raise
|
||||
# 3. New dir is in place; drop the old one (best-effort — never fatal).
|
||||
if os.path.exists(backup):
|
||||
shutil.rmtree(backup, ignore_errors=True)
|
||||
_commit_staged_replacements([(_stage_replacement(src, dst), dst)])
|
||||
|
||||
|
||||
def _stage_replacement(src: str, dst: str) -> str:
|
||||
"""Copy *src* to a sibling staging dir for *dst*; return the staging path.
|
||||
"""Copy *src* to a sibling staging path 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.
|
||||
Phase 1 of the two-phase replace. Handles both directories and plain
|
||||
files. 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):
|
||||
if os.path.isdir(leftover):
|
||||
shutil.rmtree(leftover, ignore_errors=True)
|
||||
shutil.copytree(src, staging)
|
||||
elif os.path.exists(leftover):
|
||||
os.remove(leftover)
|
||||
if os.path.isdir(src):
|
||||
shutil.copytree(src, staging)
|
||||
else:
|
||||
shutil.copy2(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.
|
||||
def _discard_staged(staged) -> None:
|
||||
"""Remove staging paths for entries that were never committed.
|
||||
|
||||
Without this a phase-1 failure (typically disk exhaustion) orphans one
|
||||
staging copy per entry already processed — up to a full second copy of
|
||||
the tree. The user then follows the "re-run `hermes update`" advice with
|
||||
*less* free space than before and the retry fails harder than the
|
||||
original attempt.
|
||||
"""
|
||||
for staging, _dst in staged:
|
||||
try:
|
||||
if os.path.isdir(staging):
|
||||
shutil.rmtree(staging, ignore_errors=True)
|
||||
elif os.path.exists(staging):
|
||||
os.remove(staging)
|
||||
except OSError as exc: # best-effort cleanup, never fatal
|
||||
logger.warning("could not remove staging path %s: %s", staging, exc)
|
||||
|
||||
|
||||
def _commit_staged_replacements(staged) -> None:
|
||||
"""Phase 2: swap every staged entry 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 ZIP update replaces ~90 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).
|
||||
|
||||
This covers plain files as well as directories: the repo root holds 20
|
||||
first-party modules (``run_agent.py``, ``cli.py``, ``hermes_constants.py``
|
||||
…), so a files-only failure reproduces exactly the bug class we are
|
||||
closing. ``os.replace`` is atomic on POSIX and maps to
|
||||
``MoveFileEx(REPLACE_EXISTING)`` on Windows, so a file swap can never
|
||||
leave a half-written module the way ``copy2`` onto a live path can.
|
||||
|
||||
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.
|
||||
the remaining window recoverable: if a swap 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
|
||||
swapped: list[tuple[str, str]] = [] # (dst, backup) in swap order; "" = absent
|
||||
try:
|
||||
for staging, dst in staged:
|
||||
backup = f"{dst}.hermes-update-old"
|
||||
|
|
@ -666,17 +678,27 @@ def _commit_staged_replacements(staged: list[tuple[str, str]]) -> None:
|
|||
# Undo every swap already made so the install stays self-consistent.
|
||||
for dst, backup in reversed(swapped):
|
||||
try:
|
||||
if os.path.exists(dst):
|
||||
if os.path.isdir(dst):
|
||||
shutil.rmtree(dst, ignore_errors=True)
|
||||
elif os.path.exists(dst):
|
||||
os.remove(dst)
|
||||
if backup and os.path.exists(backup):
|
||||
os.rename(backup, dst)
|
||||
except OSError:
|
||||
pass # best-effort; the raise below reports the real failure
|
||||
except OSError as exc:
|
||||
# Keep restoring the rest — a silent failure here is the one
|
||||
# thing that turns a recoverable rollback into a mixed tree,
|
||||
# so say so rather than swallowing it.
|
||||
logger.warning("rollback failed for %s: %s", dst, exc)
|
||||
raise
|
||||
# All swaps succeeded — drop the backups (best-effort, never fatal).
|
||||
for _dst, backup in swapped:
|
||||
if backup and os.path.exists(backup):
|
||||
if backup and os.path.isdir(backup):
|
||||
shutil.rmtree(backup, ignore_errors=True)
|
||||
elif backup and os.path.exists(backup):
|
||||
try:
|
||||
os.remove(backup)
|
||||
except OSError:
|
||||
pass
|
||||
|
||||
|
||||
def _update_via_zip(args):
|
||||
|
|
@ -759,55 +781,67 @@ def _update_via_zip(args):
|
|||
preserve = {"venv", "node_modules", ".git", ".env"}
|
||||
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.
|
||||
# Two-phase replace (#76104). Phase 1 copies every entry — directories
|
||||
# AND top-level files — to a sibling staging path 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. Files matter as much as directories here: the repo
|
||||
# root holds 20 first-party modules (run_agent.py, cli.py,
|
||||
# hermes_constants.py, ...).
|
||||
#
|
||||
# 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.
|
||||
# Staging costs one extra copy of the tree on disk. Check up front so
|
||||
# we fail with a clear message instead of running out mid-copy.
|
||||
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))
|
||||
) + sum(
|
||||
os.path.getsize(os.path.join(extracted, e))
|
||||
for e in entries
|
||||
if os.path.isfile(os.path.join(extracted, e))
|
||||
)
|
||||
# Only the staging copy is new — the live tree already occupies its
|
||||
# space and the swaps are renames, not copies. Ask for the staging
|
||||
# copy plus 20% headroom rather than a full 2x, which would block
|
||||
# updates that would have succeeded on exactly the space-constrained
|
||||
# machines most likely to hit this path.
|
||||
required = int(need * 1.2)
|
||||
free = shutil.disk_usage(str(_m().PROJECT_ROOT)).free
|
||||
if free < need * 2:
|
||||
if free < required:
|
||||
raise RuntimeError(
|
||||
f"not enough free disk space to stage the update safely "
|
||||
f"(need ~{need * 2 // (1024 * 1024)} MB, have "
|
||||
f"(need ~{required // (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):
|
||||
try:
|
||||
for item in entries:
|
||||
src = os.path.join(extracted, item)
|
||||
dst = os.path.join(str(_m().PROJECT_ROOT), item)
|
||||
staged.append((_stage_replacement(src, dst), dst))
|
||||
else:
|
||||
plain_files.append((src, dst))
|
||||
except Exception:
|
||||
# Nothing is live yet; drop the partial staging copies so a retry
|
||||
# starts from the same free space this attempt did.
|
||||
_discard_staged(staged)
|
||||
raise
|
||||
|
||||
_commit_staged_replacements(staged)
|
||||
for src, dst in plain_files:
|
||||
shutil.copy2(src, dst)
|
||||
update_count = len(staged) + len(plain_files)
|
||||
update_count = len(staged)
|
||||
|
||||
print(f"✓ Updated {update_count} items from ZIP")
|
||||
|
||||
except Exception as e:
|
||||
print(f"✗ ZIP update failed: {e}")
|
||||
# The two-phase replace either commits every entry or rolls them all
|
||||
# back, so a failure here does not leave a mixed-version tree — don't
|
||||
# scare the user toward a reinstall they don't need.
|
||||
print(" Your existing install was left in place.")
|
||||
print(
|
||||
" The install may be partially updated — some directories were "
|
||||
"replaced and others were not."
|
||||
)
|
||||
print(
|
||||
" Re-run `hermes update` to finish; if the agent won't start, "
|
||||
" Re-run `hermes update` to retry; if the agent won't start, "
|
||||
"reinstall from https://hermes-agent.nousresearch.com"
|
||||
)
|
||||
_m().sys.exit(1)
|
||||
|
|
|
|||
|
|
@ -1255,11 +1255,14 @@ AI_GATEWAY_BASE_URL = "https://ai-gateway.vercel.sh/v1"
|
|||
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.
|
||||
Canonical helper for venv layout. This was open-coded in seven places
|
||||
across four ``hermes_cli`` modules 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.
|
||||
A few sites outside ``hermes_cli`` (``tools/code_execution_tool.py``,
|
||||
``agent/lsp/install.py``) still hand-roll it — convert them as they are
|
||||
touched.
|
||||
|
||||
The path is returned unconditionally — callers legitimately differ on
|
||||
whether a missing venv is an error, so existence checking stays with them.
|
||||
|
|
|
|||
|
|
@ -158,21 +158,152 @@ def test_managed_uv_helper_delegates_to_the_shared_one():
|
|||
|
||||
|
||||
def test_no_open_coded_venv_layout_remains_in_hermes_cli():
|
||||
"""Fails if a new call site hand-rolls Scripts/bin again (#76105)."""
|
||||
"""Fails if a new call site hand-rolls Scripts/bin again (#76105).
|
||||
|
||||
Uses AST rather than substring matching: an earlier `"if" in line` version
|
||||
matched any word containing "if" (mod*if*y, ver*if*y) and still missed
|
||||
`os.path.join(venv, "Scripts")`.
|
||||
|
||||
``stdio.py`` is exempt: it builds a list of literal *Windows-only* PATH
|
||||
candidates, not a cross-platform layout derivation, so ``venv_bin_dir()``
|
||||
(which branches on the host platform) would be the wrong tool there.
|
||||
"""
|
||||
import ast
|
||||
import hermes_cli
|
||||
|
||||
exempt = {"stdio.py"}
|
||||
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)
|
||||
if py.name in exempt:
|
||||
continue
|
||||
try:
|
||||
tree = ast.parse(py.read_text(encoding="utf-8", errors="replace"))
|
||||
except SyntaxError:
|
||||
continue
|
||||
for node in ast.walk(tree):
|
||||
# Any *code* string literal "Scripts" is a hand-rolled layout;
|
||||
# docstrings and comments never reach ast.Constant in an expr
|
||||
# position we care about here.
|
||||
if isinstance(node, ast.Constant) and node.value == "Scripts":
|
||||
offenders.append(f"{py.relative_to(pkg)}:{node.lineno}")
|
||||
assert not offenders, (
|
||||
"open-coded venv layout found (use hermes_constants.venv_bin_dir):\n"
|
||||
+ "\n".join(offenders)
|
||||
)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Top-level FILES must be atomic too (#76104 review, C1)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
def test_top_level_files_are_swapped_atomically(tmp_path):
|
||||
"""The repo root holds 20 first-party modules (run_agent.py, cli.py,
|
||||
hermes_constants.py, ...). Covering only directories would leave exactly
|
||||
the bug class this PR closes."""
|
||||
live, new = tmp_path / "live", tmp_path / "new"
|
||||
live.mkdir()
|
||||
new.mkdir()
|
||||
(live / "run_agent.py").write_text("old")
|
||||
(new / "run_agent.py").write_text("new")
|
||||
|
||||
staged = [
|
||||
(
|
||||
update_cmd._stage_replacement(
|
||||
str(new / "run_agent.py"), str(live / "run_agent.py")
|
||||
),
|
||||
str(live / "run_agent.py"),
|
||||
)
|
||||
]
|
||||
update_cmd._commit_staged_replacements(staged)
|
||||
|
||||
assert (live / "run_agent.py").read_text() == "new"
|
||||
assert not [p for p in os.listdir(live) if "hermes-update" in p]
|
||||
|
||||
|
||||
def test_file_swap_failure_restores_the_original_file(tmp_path, monkeypatch):
|
||||
"""A mid-swap failure must not leave a stale-or-corrupt root module."""
|
||||
live, new = tmp_path / "live", tmp_path / "new"
|
||||
live.mkdir()
|
||||
new.mkdir()
|
||||
for name in ("cli.py", "run_agent.py"):
|
||||
(live / name).write_text("old")
|
||||
(new / name).write_text("new")
|
||||
|
||||
staged = [
|
||||
(update_cmd._stage_replacement(str(new / n), str(live / n)), str(live / n))
|
||||
for n in ("cli.py", "run_agent.py")
|
||||
]
|
||||
|
||||
real_rename = os.rename
|
||||
calls = {"n": 0}
|
||||
|
||||
def flaky_rename(src, dst):
|
||||
calls["n"] += 1
|
||||
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()
|
||||
|
||||
versions = {n: (live / n).read_text() for n in ("cli.py", "run_agent.py")}
|
||||
assert versions == {"cli.py": "old", "run_agent.py": "old"}, (
|
||||
f"mixed/corrupt root modules after rollback: {versions}"
|
||||
)
|
||||
|
||||
|
||||
def test_failed_staging_leaves_no_orphaned_copies(tmp_path, monkeypatch):
|
||||
"""#76104 review C2: orphaned staging dirs make the retry we recommend
|
||||
fail harder than the original attempt (less free space each time)."""
|
||||
live, new = tmp_path / "live", tmp_path / "new"
|
||||
_live_tree(live, {"agent": "old", "tools": "old", "gateway": "old"})
|
||||
_live_tree(new, {"agent": "new", "tools": "new", "gateway": "new"})
|
||||
|
||||
real_copytree = update_cmd.shutil.copytree
|
||||
calls = {"n": 0}
|
||||
|
||||
def flaky_copytree(src, dst, *a, **kw):
|
||||
calls["n"] += 1
|
||||
if calls["n"] == 3:
|
||||
raise OSError(28, "No space left on device")
|
||||
return real_copytree(src, dst, *a, **kw)
|
||||
|
||||
monkeypatch.setattr(update_cmd.shutil, "copytree", flaky_copytree)
|
||||
|
||||
staged: list[tuple[str, str]] = []
|
||||
with pytest.raises(OSError):
|
||||
try:
|
||||
for n in ("agent", "tools", "gateway"):
|
||||
staged.append(
|
||||
(
|
||||
update_cmd._stage_replacement(
|
||||
str(new / n), str(live / n)
|
||||
),
|
||||
str(live / n),
|
||||
)
|
||||
)
|
||||
except Exception:
|
||||
update_cmd._discard_staged(staged)
|
||||
raise
|
||||
monkeypatch.undo()
|
||||
|
||||
leftovers = [p for p in os.listdir(live) if "hermes-update" in p]
|
||||
assert leftovers == [], f"orphaned staging copies: {leftovers}"
|
||||
# And nothing live was touched.
|
||||
for n in ("agent", "tools", "gateway"):
|
||||
assert (live / n / "version.txt").read_text() == "old"
|
||||
|
||||
|
||||
def test_atomic_replace_dir_still_works_as_a_shim(tmp_path):
|
||||
"""W1: it is now an alias over the two-phase helpers; #49145 must hold."""
|
||||
live, new = tmp_path / "live", tmp_path / "new"
|
||||
_live_tree(live, {"ui-tui": "old"})
|
||||
_live_tree(new, {"ui-tui": "new"})
|
||||
|
||||
update_cmd._atomic_replace_dir(str(new / "ui-tui"), str(live / "ui-tui"))
|
||||
|
||||
assert (live / "ui-tui" / "version.txt").read_text() == "new"
|
||||
assert not [p for p in os.listdir(live) if "hermes-update" in p]
|
||||
|
|
|
|||
Loading…
Reference in New Issue