fix(update): restore mid-swap backup before clearing leftovers in staging

Phase 2 review HIGH (empirically reproduced): a hard kill between
os.rename(dst, backup) and os.rename(staging, dst) leaves dst missing and
the backup as the ONLY copy of that entry. On retry, _stage_replacement
deleted that backup as a 'leftover' BEFORE staging the fresh copy — so a
staging failure (disk exhaustion is likeliest exactly after writing a full
staging copy) left a hole in the install with nothing to roll back to.

Restore the backup to dst first when dst is missing; it's a same-filesystem
rename. Mutation-verified: removing the restore makes the new test fail.
This commit is contained in:
kshitij 2026-08-01 16:49:42 +05:30
parent 66ba36ec81
commit bbe93ab8a8
2 changed files with 46 additions and 0 deletions

View File

@ -613,6 +613,14 @@ def _stage_replacement(src: str, dst: str) -> str:
"""
staging = f"{dst}.hermes-update-staging"
backup = f"{dst}.hermes-update-old"
# A previous run may have died between "move dst aside" and "move staging
# in" — leaving dst missing and the backup as the ONLY copy of that entry.
# Restore it before clearing leftovers: deleting the backup first and then
# failing to stage (disk exhaustion is likely right after writing a full
# staging copy) would leave a hole in the install with nothing to roll
# back to. The restore is a same-filesystem rename — instant and safe.
if not os.path.exists(dst) and os.path.exists(backup):
os.rename(backup, dst)
for leftover in (staging, backup):
if os.path.isdir(leftover):
shutil.rmtree(leftover, ignore_errors=True)

View File

@ -348,3 +348,41 @@ def test_patched_is_windows_reaches_the_venv_path_derivation():
venv_python_path("/nope/venv", windows=True).as_posix()
== "/nope/venv/Scripts/python.exe"
)
# ---------------------------------------------------------------------------
# Crash between "move dst aside" and "move staging in" (Phase 2 review HIGH)
# ---------------------------------------------------------------------------
def test_staging_restores_backup_when_dst_is_missing(tmp_path, monkeypatch):
"""A previous run that died mid-swap leaves dst missing and the backup as
the ONLY copy of that entry. On retry, _stage_replacement must restore
the backup to dst BEFORE clearing leftovers otherwise a staging failure
right after (disk exhaustion is likeliest exactly then) leaves a hole in
the install with nothing to roll back to."""
live, new = tmp_path / "live", tmp_path / "new"
live.mkdir()
_live_tree(new, {"agent": "new"})
# Simulate the crashed state: dst gone, backup holds the old tree.
backup = live / "agent.hermes-update-old"
backup.mkdir()
(backup / "version.txt").write_text("old")
# Staging fails (disk full) on the fresh copy.
def boom(src, dst, *a, **kw):
raise OSError(28, "No space left on device")
monkeypatch.setattr(update_cmd.shutil, "copytree", boom)
with pytest.raises(OSError):
update_cmd._stage_replacement(str(new / "agent"), str(live / "agent"))
monkeypatch.undo()
# The old tree must have been restored to dst before the failure.
assert (live / "agent" / "version.txt").read_text() == "old"
assert not backup.exists()
# And a clean retry completes the update normally.
staged = _stage_all(live, new, ["agent"])
update_cmd._commit_staged_replacements(staged)
assert (live / "agent" / "version.txt").read_text() == "new"
assert not [p for p in os.listdir(live) if "hermes-update" in p]