From 0df2a701a5f55d2a8ea3fc4aad36da8a4cae7979 Mon Sep 17 00:00:00 2001 From: rajashidattapy Date: Wed, 12 Aug 2026 20:56:28 +0100 Subject: [PATCH] fix(paths): clear read-only bit before unlinking the atomic-write temp on Windows (#2622) --- graphify/paths.py | 10 ++++++++- tests/test_atomic_writes.py | 44 +++++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/graphify/paths.py b/graphify/paths.py index a48688e2..8a3cdc21 100644 --- a/graphify/paths.py +++ b/graphify/paths.py @@ -76,7 +76,15 @@ def _atomic_replace(path: "str | Path", write_fn) -> None: try: os.unlink(tmp) except OSError: - pass + # The temp was chmod'd to match the destination above, so when the + # destination is read-only the temp is too — and Windows refuses to + # unlink a read-only file. Clear the bit and retry, or every failed + # write leaks a `.graph.json.*.tmp` into the output directory. + try: + os.chmod(tmp, stat.S_IWRITE) + os.unlink(tmp) + except OSError: + pass raise diff --git a/tests/test_atomic_writes.py b/tests/test_atomic_writes.py index 968cb0b9..65e68101 100644 --- a/tests/test_atomic_writes.py +++ b/tests/test_atomic_writes.py @@ -36,6 +36,12 @@ def test_write_text_atomic_preserves_existing_on_failure(tmp_path, monkeypatch): assert sorted(x.name for x in tmp_path.iterdir()) == ["graph.json"] +@pytest.mark.skipif( + os.name == "nt", + reason="Windows has no POSIX mode bits: chmod only toggles the read-only " + "attribute, and st_mode reports 0o666 for any writable file, so " + "chmod(0o644) followed by an equality check can never hold", +) def test_write_text_atomic_preserves_existing_mode(tmp_path): # An atomic replace must not tighten a 0644 file to mkstemp's 0600 default. p = tmp_path / "graph.json" @@ -45,6 +51,44 @@ def test_write_text_atomic_preserves_existing_mode(tmp_path): assert (os.stat(p).st_mode & 0o777) == 0o644 +@pytest.mark.skipif(os.name != "nt", reason="Windows read-only attribute semantics") +def test_write_text_atomic_refuses_a_readonly_destination_without_leaking_a_temp( + tmp_path, +): + """The Windows analogue of the mode-preservation contract. + + There is no POSIX mode to preserve here, so the property worth pinning is + the one Windows actually has: a read-only destination is NOT silently + overwritten, the original survives intact, and — the part that used to be + wrong — no temp file is left behind. + + `_atomic_replace` chmods the temp to match the destination, so against a + read-only target the temp is read-only too; Windows then refuses to unlink + it and the cleanup swallowed the error, dropping a `.graph.json.*.tmp` into + the output directory on every failed write. + + Note this diverges from POSIX, where `os.replace` needs only directory write + permission and so happily replaces a read-only file. Documented rather than + "fixed": refusing to overwrite a file the user marked read-only is the + defensible behaviour. + """ + import stat as _stat + + p = tmp_path / "graph.json" + p.write_text("original", encoding="utf-8") + os.chmod(p, _stat.S_IREAD) + try: + with pytest.raises(PermissionError): + write_text_atomic(p, "replaced") + + assert p.read_text(encoding="utf-8") == "original", "read-only file was clobbered" + assert [x.name for x in tmp_path.iterdir()] == ["graph.json"], ( + f"failed write leaked a temp file: {[x.name for x in tmp_path.iterdir()]}" + ) + finally: + os.chmod(p, _stat.S_IWRITE) # let tmp_path cleanup remove it + + def test_write_text_atomic_new_file_respects_umask(tmp_path): # A brand-new file must land at the umask default (e.g. 0644), NOT mkstemp's # 0600 — otherwise every fresh graph.json would be owner-only.