Add a shared Windows replace fallback helper

os.replace can fail on Windows even when the temp file and the
destination are in the same directory on the same drive: WinError
17, "cannot move to a different disk drive", maps to a plain OSError
in Python rather than PermissionError, so the existing copy then
delete fallback for WinError 5 and 32 never caught it. Adds
os_replace_with_fallback, checking both PermissionError and winerror
17, and points the shared atomic writer at it instead of its own
inline PermissionError only version. Toward #3508.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh
(cherry picked from commit 32cbf1f83e)
This commit is contained in:
ayushcodes10 authored and safishamsi committed 2026-09-12 17:36:09 +01:00
1 parent 522ea9662e
commit df5a9042e3
2 files changed
+47 -9

No files matched your search

+22 -9
View File
@@ -26,6 +26,27 @@ from pathlib import Path, PurePosixPath, PureWindowsPath
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 --
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.
"""
try:
os.replace(src, dst)
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)
def _atomic_replace(path: "str | Path", write_fn) -> None:
"""Atomically replace ``path`` with content written by ``write_fn(f)``.
@@ -63,15 +84,7 @@ def _atomic_replace(path: "str | Path", write_fn) -> None:
os.chmod(tmp, mode)
except OSError:
pass
try:
os.replace(tmp, str(real))
except PermissionError:
# Windows: os.replace fails (WinError 5/32) when the destination is
# briefly locked by another handle (antivirus, an open reader). Fall
# back to copy-then-delete, matching graphify.cache's atomic writer.
import shutil
shutil.copy2(tmp, str(real))
os.unlink(tmp)
os_replace_with_fallback(tmp, str(real))
except BaseException:
try:
os.unlink(tmp)
+25
View File
@@ -167,6 +167,31 @@ def test_write_text_atomic_windows_permission_fallback(tmp_path, monkeypatch):
assert sorted(x.name for x in tmp_path.iterdir()) == ["graph.json"]
def test_write_text_atomic_windows_winerror_17_fallback(tmp_path, monkeypatch):
"""#3508: `os.replace` can raise WinError 17 ("cannot move to a different
disk drive") even when src/dst are the same directory on the same drive,
on some Windows/filesystem combinations. Unlike WinError 5/32, this is a
plain OSError rather than PermissionError, so the fallback must key off
winerror rather than the exception type alone."""
p = tmp_path / "graph.json"
p.write_text("original", encoding="utf-8")
calls = {"n": 0}
def flaky_replace(src, dst):
calls["n"] += 1
exc = OSError("cannot move to a different disk drive")
exc.winerror = 17
raise exc
monkeypatch.setattr(os, "replace", flaky_replace)
write_text_atomic(p, "new-content")
assert calls["n"] == 1
assert p.read_text() == "new-content"
assert sorted(x.name for x in tmp_path.iterdir()) == ["graph.json"]
def test_write_json_atomic_ensure_ascii_false_preserves_utf8(tmp_path):
from graphify.paths import write_json_atomic