From 39effa0c28eb2faa7bde33c6c7697a9e7fcec379 Mon Sep 17 00:00:00 2001 From: ayushcodes10 Date: Sat, 12 Sep 2026 18:47:53 +0530 Subject: [PATCH] Preserve replace semantics for a symlinked destination os.replace atomically swaps whatever sits at the destination, a symlink included, by replacing the link itself. install.py relies on exactly that for its managed skill symlinks, per #3286. The fallback's plain shutil.copy2 does the opposite when the destination is a symlink: opening it for writing follows the link and overwrites whatever it points at instead of the link. Copies to a fresh temp file beside the destination first, then swaps the destination out from under it with an unlink plus rename, matching replace's own behavior and, as a side effect, shrinking the window where a reader could see a half written file down to two fast metadata operations rather than however long the copy itself takes. Found by the graphify review bot on PR 3509. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh --- graphify/paths.py | 40 +++++++++++++++++++++++++++++++------ tests/test_atomic_writes.py | 35 ++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 6 deletions(-) diff --git a/graphify/paths.py b/graphify/paths.py index adfb779cc..01356492e 100644 --- a/graphify/paths.py +++ b/graphify/paths.py @@ -27,24 +27,52 @@ GRAPHIFY_OUT = os.environ.get("GRAPHIFY_OUT", "graphify-out") def os_replace_with_fallback(src: "str | Path", dst: "str | Path") -> None: - """``os.replace(src, dst)``, falling back to copy-then-delete for a known - set of Windows quirks (#3508) that raise even when ``src``/``dst`` are the - same directory on the same drive: ``PermissionError`` (WinError 5/32 -- + """``os.replace(src, dst)``, falling back to a copy for a known set of + Windows quirks (#3508) that raise even when ``src``/``dst`` are the same + directory on the same drive: ``PermissionError`` (WinError 5/32 -- destination briefly locked by another handle, antivirus, an open reader) and WinError 17 ("cannot move to a different disk drive", observed on some Windows/filesystem combinations despite textbook same-volume semantics). WinError 17 maps to a plain ``OSError`` in Python, not ``PermissionError``, so it's checked via ``winerror`` rather than the exception type. Any other failure is a real one and is re-raised. + + ``os.replace`` atomically swaps whatever sits at ``dst`` -- including a + symlink, which it REPLACES in place rather than following (some callers, + 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. """ try: os.replace(src, dst) + return except OSError as exc: if not isinstance(exc, PermissionError) and getattr(exc, "winerror", None) != 17: raise - import shutil - shutil.copy2(src, dst) - os.unlink(src) + import shutil + dst = os.fspath(dst) + fd, tmp_copy = tempfile.mkstemp(dir=os.path.dirname(dst) or ".", prefix=".gfy-replace-", suffix=".tmp") + os.close(fd) + try: + shutil.copy2(src, tmp_copy) + try: + os.unlink(dst) + except FileNotFoundError: + pass + os.rename(tmp_copy, dst) + except BaseException: + try: + os.unlink(tmp_copy) + except OSError: + pass + raise + os.unlink(src) def _atomic_replace(path: "str | Path", write_fn) -> None: diff --git a/tests/test_atomic_writes.py b/tests/test_atomic_writes.py index 21dc2f9fd..392bc3c02 100644 --- a/tests/test_atomic_writes.py +++ b/tests/test_atomic_writes.py @@ -6,6 +6,7 @@ directory then `os.replace`s it into place; on failure the original is untouched """ import json import os +import sys import pytest @@ -192,6 +193,40 @@ def test_write_text_atomic_windows_winerror_17_fallback(tmp_path, monkeypatch): assert sorted(x.name for x in tmp_path.iterdir()) == ["graph.json"] +@pytest.mark.skipif(sys.platform == "win32", reason="symlink setup differs on Windows") +def test_os_replace_with_fallback_replaces_a_symlink_destination_in_place(tmp_path, monkeypatch): + """`os.replace` replaces a symlinked destination itself rather than + following it -- install.py relies on exactly this for managed skill + symlinks (#3286). 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. The #3508 fallback must preserve replace's + semantics, not copy2's, or a Windows quirk that triggers the fallback + would silently clobber whatever a managed symlink pointed at.""" + from graphify.paths import os_replace_with_fallback + + target = tmp_path / "shared_target.txt" + target.write_text("ORIGINAL SHARED CONTENT", encoding="utf-8") + link = tmp_path / "skill_link" + link.symlink_to(target) + src = tmp_path / "tmp_new.txt" + src.write_text("NEW CONTENT", encoding="utf-8") + + def flaky_replace(a, b): + exc = OSError("cannot move to a different disk drive") + exc.winerror = 17 + raise exc + + monkeypatch.setattr(os, "replace", flaky_replace) + os_replace_with_fallback(str(src), str(link)) + + assert not link.is_symlink(), "link must be replaced by a plain file, not left as a symlink" + assert link.read_text(encoding="utf-8") == "NEW CONTENT" + assert target.read_text(encoding="utf-8") == "ORIGINAL SHARED CONTENT", ( + "the shared target must be untouched -- a copy2-through-the-link would have clobbered it" + ) + assert not src.exists() + + def test_write_json_atomic_ensure_ascii_false_preserves_utf8(tmp_path): from graphify.paths import write_json_atomic