diff --git a/src/tocode/cli.py b/src/tocode/cli.py index dee5ec4..7387856 100644 --- a/src/tocode/cli.py +++ b/src/tocode/cli.py @@ -185,9 +185,6 @@ def main(argv: list[str] | None = None) -> int: else binary.parent / default_output_name(binary) ) progress.set_log_path(log_root / "tocode.log") - progress.log( - f"Command: {' '.join(sys.argv if argv is None else ['tocode', *argv])}" - ) started = time.monotonic() try: if not binary.is_file(): diff --git a/src/tocode/exporter.py b/src/tocode/exporter.py index 2a40f4a..5c67f25 100644 --- a/src/tocode/exporter.py +++ b/src/tocode/exporter.py @@ -3061,13 +3061,17 @@ def _relativize_sources(files: list[str]) -> dict[str, str]: for source_file in files: if os.path.isabs(source_file) and root: try: - result[source_file] = os.path.relpath(source_file, root) + # Emit POSIX separators so exports match across platforms + # (os.path.relpath yields backslashes on Windows). + result[source_file] = os.path.relpath(source_file, root).replace( + os.sep, "/" + ) continue except ValueError: pass result[source_file] = ( source_file.lstrip("/") if os.path.isabs(source_file) else source_file - ) + ).replace(os.sep, "/") return result diff --git a/src/tocode/metadata.py b/src/tocode/metadata.py index 7b7fb55..6e529ae 100644 --- a/src/tocode/metadata.py +++ b/src/tocode/metadata.py @@ -6,6 +6,7 @@ from pathlib import Path import re import tempfile +import time from typing import Callable from .naming import SHARED_CLUSTER_ID, c_file_name, clean_path_component @@ -756,6 +757,28 @@ def write_json(path: Path, payload: dict[str, object]) -> None: write_text_atomic(path, json.dumps(payload, indent=2, sort_keys=False) + "\n") +_ATOMIC_REPLACE_ATTEMPTS = 10 +_ATOMIC_REPLACE_BASE_DELAY = 0.05 # seconds; ~2.75s worst case across attempts +# Windows returns ERROR_ACCESS_DENIED (5) or ERROR_SHARING_VIOLATION (32) when +# another process (antivirus, the search indexer, OneDrive) briefly holds the +# destination or the temp file open without FILE_SHARE_DELETE at the instant of +# the rename. These are transient, so retry before giving up. +_TRANSIENT_REPLACE_WINERRORS = frozenset({5, 32}) + + +def _replace_with_retry(src: Path, dst: Path) -> None: + for attempt in range(_ATOMIC_REPLACE_ATTEMPTS): + try: + src.replace(dst) + return + except PermissionError as exc: + # winerror is None off Windows, so other platforms re-raise at once. + transient = getattr(exc, "winerror", None) in _TRANSIENT_REPLACE_WINERRORS + if not transient or attempt == _ATOMIC_REPLACE_ATTEMPTS - 1: + raise + time.sleep(_ATOMIC_REPLACE_BASE_DELAY * (attempt + 1)) + + def write_text_atomic(path: Path, text: str) -> None: path.parent.mkdir(parents=True, exist_ok=True) with tempfile.NamedTemporaryFile( @@ -768,4 +791,9 @@ def write_text_atomic(path: Path, text: str) -> None: ) as handle: tmp_path = Path(handle.name) handle.write(text) - tmp_path.replace(path) + try: + _replace_with_retry(tmp_path, path) + except OSError: + # Do not leave the temp file behind when the replace ultimately fails. + tmp_path.unlink(missing_ok=True) + raise diff --git a/src/tocode/parallel.py b/src/tocode/parallel.py index 609c376..7b47ace 100644 --- a/src/tocode/parallel.py +++ b/src/tocode/parallel.py @@ -152,10 +152,15 @@ def available_memory_mb() -> int | None: meminfo = _linux_mem_available_mb() if meminfo is not None: return meminfo + # os.sysconf is POSIX-only (absent on Windows); fetch it dynamically so the + # attribute access does not fail static analysis on Windows. + sysconf = getattr(os, "sysconf", None) + if sysconf is None: + return None try: - pages = os.sysconf("SC_AVPHYS_PAGES") - page_size = os.sysconf("SC_PAGE_SIZE") - except (AttributeError, OSError, ValueError): + pages = sysconf("SC_AVPHYS_PAGES") + page_size = sysconf("SC_PAGE_SIZE") + except (OSError, ValueError): return None if not isinstance(pages, int) or not isinstance(page_size, int): return None diff --git a/tests/test_atomic_write.py b/tests/test_atomic_write.py new file mode 100644 index 0000000..ccb8d6a --- /dev/null +++ b/tests/test_atomic_write.py @@ -0,0 +1,170 @@ +"""Reproduction and regression tests for the Windows atomic-write crash (#11). + +`write_text_atomic` renames a temp file over the destination. On Windows that +rename fails with ``PermissionError`` (``WinError 5`` access-denied, or ``32`` +sharing-violation) whenever another process — antivirus, the search indexer, +OneDrive — briefly holds the destination or the temp file open without +``FILE_SHARE_DELETE``. Under ``-j 8`` the checkpoint is rewritten on every +completed function, so such a collision eventually aborts the whole export. + +The portable test reproduces the mechanism on any platform by monkeypatching the +rename; the Windows test reproduces the exact production failure with a real lock. +""" + +from __future__ import annotations + +import sys +from pathlib import Path + +import pytest + +from tocode.metadata import write_text_atomic + + +def _win_permission_error(winerror: int) -> PermissionError: + exc = PermissionError(winerror, "Access is denied") + # Mirror the OSError.winerror attribute CPython sets on Windows so the code + # under test can distinguish transient lock errors from real ones. + exc.winerror = winerror # type: ignore[attr-defined] + return exc + + +def _tmp_siblings(path: Path) -> list[Path]: + return [p for p in path.parent.iterdir() if p.name != path.name] + + +def test_replace_retries_past_transient_lock(tmp_path, monkeypatch) -> None: + """A few transient WinError 5 failures must not abort the write.""" + target = tmp_path / "checkpoint.json" + real_replace = Path.replace + calls = {"n": 0} + + def flaky_replace(self: Path, dst): + calls["n"] += 1 + if calls["n"] <= 3: # transient lock clears after a few attempts + raise _win_permission_error(5) + return real_replace(self, dst) + + monkeypatch.setattr(Path, "replace", flaky_replace) + monkeypatch.setattr("tocode.metadata.time.sleep", lambda _s: None) + + write_text_atomic(target, "payload\n") + + assert calls["n"] == 4 # 3 failures + 1 success + assert target.read_text(encoding="utf-8") == "payload\n" + # No orphan .tmp left behind. + assert _tmp_siblings(target) == [] + + +def test_replace_gives_up_and_cleans_tmp_when_lock_never_clears( + tmp_path, monkeypatch +) -> None: + """A permanent lock still raises, but must not litter .tmp orphans.""" + target = tmp_path / "checkpoint.json" + + def always_locked(self: Path, dst): + raise _win_permission_error(5) + + monkeypatch.setattr(Path, "replace", always_locked) + monkeypatch.setattr("tocode.metadata.time.sleep", lambda _s: None) + + with pytest.raises(PermissionError): + write_text_atomic(target, "payload\n") + + assert not target.exists() + assert _tmp_siblings(target) == [] + + +def test_non_windows_permission_error_is_not_retried(tmp_path, monkeypatch) -> None: + """A PermissionError without a transient winerror re-raises immediately.""" + target = tmp_path / "checkpoint.json" + calls = {"n": 0} + + def denied(self: Path, dst): + calls["n"] += 1 + raise PermissionError(13, "Permission denied") # e.g. read-only dir + + monkeypatch.setattr(Path, "replace", denied) + monkeypatch.setattr("tocode.metadata.time.sleep", lambda _s: None) + + with pytest.raises(PermissionError): + write_text_atomic(target, "payload\n") + + assert calls["n"] == 1 # not retried + + +# --- Faithful Windows reproduction with a real file lock ------------------ + +_WINDOWS_ONLY = pytest.mark.skipif( + sys.platform != "win32", reason="reproduces a Windows file-sharing error" +) + + +def _open_read_share_no_delete(path: Path): + """Open ``path`` the way a scanner does: read-share, no delete-share. + + Returns a Windows HANDLE that blocks anyone from replacing/deleting the file + until it is closed. Mirrors antivirus / OneDrive real-time access. + """ + import ctypes + + GENERIC_READ = 0x80000000 + FILE_SHARE_READ = 0x00000001 + OPEN_EXISTING = 3 + INVALID_HANDLE_VALUE = ctypes.c_void_p(-1).value + + # windll is Windows-only (absent from the ctypes stubs on other platforms), + # so reach it dynamically to keep static analysis happy on the Linux CI. A + # HANDLE is a void*, so use c_void_p rather than the win-only wintypes. + kernel32 = getattr(ctypes, "windll").kernel32 + kernel32.CreateFileW.restype = ctypes.c_void_p + handle = kernel32.CreateFileW( + str(path), + GENERIC_READ, + FILE_SHARE_READ, # deliberately no FILE_SHARE_DELETE + None, + OPEN_EXISTING, + 0, + None, + ) + if handle == INVALID_HANDLE_VALUE: + raise OSError(f"CreateFileW failed to lock {path}") + return handle + + +def _close_handle(handle) -> None: + import ctypes + + getattr(ctypes, "windll").kernel32.CloseHandle(handle) + + +@_WINDOWS_ONLY +def test_windows_permanent_lock_reproduces_winerror5(tmp_path) -> None: + target = tmp_path / "checkpoint.json" + target.write_text("old\n", encoding="utf-8") + handle = _open_read_share_no_delete(target) + try: + with pytest.raises(PermissionError) as info: + write_text_atomic(target, "new\n") + assert getattr(info.value, "winerror", None) in (5, 32) + finally: + _close_handle(handle) + + +@_WINDOWS_ONLY +def test_windows_transient_lock_recovers(tmp_path) -> None: + import threading + import time + + target = tmp_path / "checkpoint.json" + target.write_text("old\n", encoding="utf-8") + handle = _open_read_share_no_delete(target) + + # Release the lock shortly after the write starts, as a scanner would. + threading.Timer(0.15, lambda: _close_handle(handle)).start() + + started = time.monotonic() + write_text_atomic(target, "new\n") # retries until the lock clears + assert target.read_text(encoding="utf-8") == "new\n" + assert time.monotonic() - started >= 0.1 + assert _tmp_siblings(target) == []