From 470cf66b039c73bdd2c21d43094ce41a4db74eae Mon Sep 17 00:00:00 2001 From: kshitij <82637225+kshitijk4poor@users.noreply.github.com> Date: Sat, 1 Aug 2026 17:18:01 +0530 Subject: [PATCH] fix(update): discard staging litter when the commit phase fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- hermes_cli/update_cmd.py | 15 +++- tests/hermes_cli/test_update_zip_two_phase.py | 82 +++++++++++++++++++ 2 files changed, 96 insertions(+), 1 deletion(-) diff --git a/hermes_cli/update_cmd.py b/hermes_cli/update_cmd.py index 1405283381960..d5e1c445f4106 100644 --- a/hermes_cli/update_cmd.py +++ b/hermes_cli/update_cmd.py @@ -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") diff --git a/tests/hermes_cli/test_update_zip_two_phase.py b/tests/hermes_cli/test_update_zip_two_phase.py index 012463ce20f4c..ef4681ddb07cc 100644 --- a/tests/hermes_cli/test_update_zip_two_phase.py +++ b/tests/hermes_cli/test_update_zip_two_phase.py @@ -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" + )