fix(update): discard staging litter when the commit phase fails
Converged Phase 2 finding (two reviewers independently): _discard_staged only ran when phase-1 staging failed. A phase-2 (commit) failure rolled the live tree back correctly but orphaned staging copies for every not-yet- swapped entry — up to most of a full tree. The retry's up-front free-space check runs BEFORE the lazy per-entry leftover cleanup, so the litter makes the retry fail 'not enough free disk space' on exactly the space-constrained machines the 1.2x threshold was chosen for: the same 'retry fails harder' failure mode _discard_staged's docstring says it exists to prevent. Two tests: a behavioral one pinning rollback+discard leaves the old tree intact with zero litter, and an AST wiring contract on _update_via_zip so a refactor can't silently drop the cleanup. Mutation-verified: removing the try/except around _commit_staged_replacements fails the wiring test.
This commit is contained in:
parent
b675fb2b3e
commit
470cf66b03
|
|
@ -840,7 +840,20 @@ def _update_via_zip(args):
|
|||
_discard_staged(staged)
|
||||
raise
|
||||
|
||||
_commit_staged_replacements(staged)
|
||||
try:
|
||||
_commit_staged_replacements(staged)
|
||||
except Exception:
|
||||
# The rollback already restored every swapped entry, but staging
|
||||
# copies for the not-yet-swapped entries (potentially most of a
|
||||
# full tree) are still on disk. Drop them, or the retry's
|
||||
# up-front free-space check — which runs BEFORE the lazy
|
||||
# per-entry leftover cleanup — fails on litter this attempt
|
||||
# left behind: the exact "retry fails harder" failure mode
|
||||
# _discard_staged exists to prevent. Safe post-rollback: swapped
|
||||
# entries' staging paths were renamed away, and _discard_staged
|
||||
# skips paths that no longer exist.
|
||||
_discard_staged(staged)
|
||||
raise
|
||||
update_count = len(staged)
|
||||
|
||||
print(f"✓ Updated {update_count} items from ZIP")
|
||||
|
|
|
|||
|
|
@ -386,3 +386,85 @@ def test_staging_restores_backup_when_dst_is_missing(tmp_path, monkeypatch):
|
|||
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]
|
||||
|
||||
|
||||
def test_commit_failure_plus_discard_leaves_no_staging_litter(tmp_path, monkeypatch):
|
||||
"""Phase-2 failure must not orphan staging copies for unswapped entries.
|
||||
|
||||
_update_via_zip calls _discard_staged when _commit_staged_replacements
|
||||
raises. The rollback restores every swapped entry, but staging copies for
|
||||
the not-yet-swapped entries (potentially most of a full tree) would
|
||||
otherwise survive — and the retry's up-front free-space check runs BEFORE
|
||||
the lazy per-entry leftover cleanup, so the litter makes the retry fail
|
||||
harder than the original attempt. This pins the combination: rollback +
|
||||
discard leaves the old tree intact and ZERO update litter."""
|
||||
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"})
|
||||
staged = _stage_all(live, new, ["agent", "tools", "gateway"])
|
||||
|
||||
real_rename = os.rename
|
||||
calls = {"n": 0}
|
||||
|
||||
def flaky_rename(src, dst):
|
||||
calls["n"] += 1
|
||||
if calls["n"] == 4: # first entry fully swapped, second breaks
|
||||
raise OSError("simulated AV interference")
|
||||
return real_rename(src, dst)
|
||||
|
||||
monkeypatch.setattr(update_cmd.os, "rename", flaky_rename)
|
||||
with pytest.raises(OSError):
|
||||
try:
|
||||
update_cmd._commit_staged_replacements(staged)
|
||||
except OSError:
|
||||
# Mirrors the _update_via_zip wiring.
|
||||
update_cmd._discard_staged(staged)
|
||||
raise
|
||||
monkeypatch.undo()
|
||||
|
||||
# Old tree intact...
|
||||
for n in ("agent", "tools", "gateway"):
|
||||
assert (live / n / "version.txt").read_text() == "old"
|
||||
# ...and zero litter of any kind (staging OR backup).
|
||||
litter = [p for p in os.listdir(live) if "hermes-update" in p]
|
||||
assert litter == [], f"orphaned update litter: {litter}"
|
||||
|
||||
|
||||
def test_update_via_zip_wires_discard_into_the_commit_failure_path():
|
||||
"""AST wiring contract: _update_via_zip must call _discard_staged from an
|
||||
exception handler around _commit_staged_replacements. The behavioral test
|
||||
above mirrors that wiring; this pins the production function itself so a
|
||||
refactor can't silently drop the cleanup."""
|
||||
import ast
|
||||
import inspect
|
||||
import textwrap
|
||||
|
||||
src = textwrap.dedent(inspect.getsource(update_cmd._update_via_zip))
|
||||
tree = ast.parse(src)
|
||||
|
||||
def _calls(node, name):
|
||||
return any(
|
||||
isinstance(n, ast.Call)
|
||||
and isinstance(n.func, ast.Name)
|
||||
and n.func.id == name
|
||||
for n in ast.walk(node)
|
||||
)
|
||||
|
||||
wired = False
|
||||
for node in ast.walk(tree):
|
||||
if not isinstance(node, ast.Try):
|
||||
continue
|
||||
body_commits = any(
|
||||
_calls(stmt, "_commit_staged_replacements") for stmt in node.body
|
||||
)
|
||||
handler_discards = any(
|
||||
_calls(handler, "_discard_staged") for handler in node.handlers
|
||||
)
|
||||
if body_commits and handler_discards:
|
||||
wired = True
|
||||
break
|
||||
assert wired, (
|
||||
"_update_via_zip no longer discards staging copies when "
|
||||
"_commit_staged_replacements fails — commit-phase litter will make "
|
||||
"the retry's free-space check fail harder than the first attempt"
|
||||
)
|
||||
|
|
|
|||
Loading…
Reference in New Issue