diff --git a/graphify/paths.py b/graphify/paths.py index 01356492e..4b6d51e66 100644 --- a/graphify/paths.py +++ b/graphify/paths.py @@ -42,12 +42,13 @@ def os_replace_with_fallback(src: "str | Path", dst: "str | Path") -> None: e.g. install.py's managed skill symlinks, #3286, rely on exactly this). A naive ``shutil.copy2(src, dst)`` does the opposite when ``dst`` is a symlink: opening it for writing follows the link and overwrites its - TARGET's content instead. So the fallback copies to a fresh temp file - in ``dst``'s directory first, then removes whatever is at ``dst`` (link - or file) and renames the temp copy into place -- matching replace's - "whatever was there is gone, a plain file replaces it" semantics, and - keeping the exposure window to two fast metadata operations rather than - however long the copy itself takes. + TARGET's content instead. So the fallback copies to a fresh temp file in + ``dst``'s directory first, renames whatever is currently at ``dst`` (link + or file) aside as a backup rather than deleting it outright, and only + then renames the temp copy into ``dst``'s place -- matching replace's + "whatever was there is gone, a plain file replaces it" semantics. If that + final rename fails, the backup is renamed straight back so a mid-swap + failure leaves the original in place rather than leaving ``dst`` missing. """ try: os.replace(src, dst) @@ -57,15 +58,31 @@ def os_replace_with_fallback(src: "str | Path", dst: "str | Path") -> None: raise import shutil dst = os.fspath(dst) - fd, tmp_copy = tempfile.mkstemp(dir=os.path.dirname(dst) or ".", prefix=".gfy-replace-", suffix=".tmp") + dst_dir = os.path.dirname(dst) or "." + fd, tmp_copy = tempfile.mkstemp(dir=dst_dir, prefix=".gfy-replace-", suffix=".tmp") os.close(fd) try: shutil.copy2(src, tmp_copy) + backup = None + if os.path.lexists(dst): + bfd, backup = tempfile.mkstemp(dir=dst_dir, prefix=".gfy-replace-bak-", suffix=".tmp") + os.close(bfd) + os.unlink(backup) # reserve the name only; rename needs it free on Windows + os.rename(dst, backup) # a plain rename moves a symlink itself, never its target try: - os.unlink(dst) - except FileNotFoundError: - pass - os.rename(tmp_copy, dst) + os.rename(tmp_copy, dst) + except BaseException: + if backup is not None: + try: + os.rename(backup, dst) + except OSError: + pass # best-effort restore; the swap failure below still propagates + raise + if backup is not None: + try: + os.unlink(backup) + except OSError: + pass except BaseException: try: os.unlink(tmp_copy) diff --git a/tests/test_atomic_writes.py b/tests/test_atomic_writes.py index 392bc3c02..ba3b72c83 100644 --- a/tests/test_atomic_writes.py +++ b/tests/test_atomic_writes.py @@ -227,6 +227,47 @@ def test_os_replace_with_fallback_replaces_a_symlink_destination_in_place(tmp_pa assert not src.exists() +def test_os_replace_with_fallback_restores_destination_on_final_rename_failure(tmp_path, monkeypatch): + """The fallback removes the existing destination before renaming the new + content into place; if that final rename then fails for any reason, the + original content must be restored rather than leaving the destination + missing -- a bare unlink-then-rename with no recovery would silently + destroy the previous file on a mid-swap failure.""" + from graphify.paths import os_replace_with_fallback + + dst = tmp_path / "dst.json" + dst.write_text("ORIGINAL", encoding="utf-8") + src = tmp_path / "src.tmp" + src.write_text("NEW", encoding="utf-8") + + def flaky_replace(a, b): + exc = OSError("cannot move to a different disk drive") + exc.winerror = 17 + raise exc + + real_rename = os.rename + calls = {"n": 0} + + def flaky_rename(a, b): + calls["n"] += 1 + # First rename swaps dst aside as a backup (must succeed so there's + # something to restore); force the second -- landing the new + # content -- to fail. + if calls["n"] == 2: + raise OSError("simulated failure landing the new content") + return real_rename(a, b) + + monkeypatch.setattr(os, "replace", flaky_replace) + monkeypatch.setattr(os, "rename", flaky_rename) + with pytest.raises(OSError, match="simulated failure landing the new content"): + os_replace_with_fallback(str(src), str(dst)) + + assert dst.exists(), "destination must not be left missing after a failed swap" + assert dst.read_text(encoding="utf-8") == "ORIGINAL" + leftover = {p.name for p in tmp_path.iterdir()} + assert leftover == {"dst.json", "src.tmp"}, f"unexpected leftover files: {leftover}" + + def test_write_json_atomic_ensure_ascii_false_preserves_utf8(tmp_path): from graphify.paths import write_json_atomic