From 8f95df33345f887485992f995cc468405ffcabcb Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sun, 4 Oct 2026 23:13:00 -0700 Subject: [PATCH] fix(update): a ZIP swap without hardlinks never leaves a root file absent (review F15, F06) What: _commit_staged_replacements backs up a regular file as a hardlink or, where links are unsupported, a copy2 to .tmp + fsync + os.replace to , then os.replace(staging, dst); only directories move aside. The docstring's 'never absent' claim now holds. Why: the no-hardlink fallback renamed hermes_bootstrap.py aside before renaming the new one in; a kill between left it absent, so every launcher failed before reaching the journal restore. Red: pre-fix test_root_files_never_go_missing_mid_swap[False] -> root modules absent mid-swap: hermes_constants.py, hermes_bootstrap.py. Green: both params pass. --- hermes_cli/update_cmd_zip.py | 27 ++++++++++++++++--- tests/hermes_cli/test_update_zip_two_phase.py | 10 +++++-- 2 files changed, 31 insertions(+), 6 deletions(-) diff --git a/hermes_cli/update_cmd_zip.py b/hermes_cli/update_cmd_zip.py index 778ff8ea28..8f6107478b 100644 --- a/hermes_cli/update_cmd_zip.py +++ b/hermes_cli/update_cmd_zip.py @@ -83,7 +83,7 @@ def _stage_replacement(src: str, dst: str) -> str: os.rename(backup, dst) # Fail closed: a leftover backup that survives would later be taken for this swap's own backup # (rollback and journal recovery put it back as the live entry). - for leftover in (staging, backup): + for leftover in (staging, backup, f"{backup}.tmp"): _drop_path(Path(leftover)) if os.path.lexists(leftover): raise OSError(f"could not remove the leftover {leftover}") @@ -113,6 +113,23 @@ def _hardlink_backup(path: str, backup: str) -> bool: return True +def _file_backup(path: str, backup: str) -> None: + """Back up the regular file ``path`` as a complete ``backup`` while ``path`` stays in place: a hardlink, + or where links are unsupported (FAT32/exFAT/SMB) a synced copy landed whole by ``os.replace``.""" + if _hardlink_backup(path, backup): + return + tmp = f"{backup}.tmp" + try: + shutil.copy2(path, tmp) + with open(tmp, "rb+") as handle: + os.fsync(handle.fileno()) + os.replace(tmp, backup) + except OSError: + with suppress(OSError): + os.remove(tmp) + raise + + def _commit_staged_replacements(staged, *, on_committed=None) -> None: """Phase 2: swap every staged entry into place, rolling back all on failure. @@ -125,8 +142,9 @@ def _commit_staged_replacements(staged, *, on_committed=None) -> None: ``_atomic_replace_dir`` makes each *individual* directory swap safe, but the ZIP update replaces ~90 top-level entries in a loop, and nothing made the loop atomic *as a whole*. See #63717, #76091, #76104. - A plain file is never absent, not even for one rename: its backup is a hardlink and ``os.replace`` - lands the new bytes over it (a filesystem without hardlinks keeps the move-aside). The root modules every launcher imports first + A plain file is never absent, not even for one rename: its backup is a hardlink (or, without + hardlinks, a complete copy) and ``os.replace`` lands the new bytes over it; only directories move + aside. The root modules every launcher imports first (``hermes_constants``, ``hermes_bootstrap``) therefore always import, and ``hermes_bootstrap`` runs the journal-driven restore even while a killed swap left a directory (``hermes_cli/`` included) moved aside. """ @@ -134,7 +152,8 @@ def _commit_staged_replacements(staged, *, on_committed=None) -> None: try: for staging, dst in staged: backup = f"{dst}.hermes-update-old" - if os.path.isfile(dst) and not os.path.islink(dst) and _hardlink_backup(dst, backup): + if os.path.isfile(dst) and not os.path.islink(dst): + _file_backup(dst, backup) swapped.append((dst, backup)) os.replace(staging, dst) continue diff --git a/tests/hermes_cli/test_update_zip_two_phase.py b/tests/hermes_cli/test_update_zip_two_phase.py index 8aa484b5a4..93fff4be6b 100644 --- a/tests/hermes_cli/test_update_zip_two_phase.py +++ b/tests/hermes_cli/test_update_zip_two_phase.py @@ -329,10 +329,16 @@ def test_commit_failure_plus_discard_leaves_no_staging_litter(tmp_path, monkeypa assert litter == [], f"orphaned update litter: {litter}" -def test_root_files_never_go_missing_mid_swap(tmp_path, monkeypatch): +@pytest.mark.parametrize("hardlinks", [True, False]) +def test_root_files_never_go_missing_mid_swap(tmp_path, monkeypatch, hardlinks): """Every launcher imports ``hermes_constants``/``hermes_bootstrap`` before anything else, and ``hermes_bootstrap`` is what reaches the restore after a killed swap: a kill between any two - filesystem steps must find every root file present (old or new bytes), only directories moved.""" + filesystem steps must find every root file present (old or new bytes), only directories moved. + That holds on filesystems without hardlinks too (FAT32/exFAT/SMB).""" + if not hardlinks: + def no_link(*_a, **_k): + raise OSError(1, "Operation not permitted") + monkeypatch.setattr(update_cmd_zip.os, "link", no_link) live, new = tmp_path / "live", tmp_path / "new" for side, text in ((live, "old"), (new, "new")): (side / "hermes_cli").mkdir(parents=True)