Skip to content

Commit b6d7f1c

Browse files
authored
Merge pull request #12 from buzzer-re/dev/fix-deadlock
Dev/fix deadlock
2 parents d3fac36 + 3839dd5 commit b6d7f1c

5 files changed

Lines changed: 213 additions & 9 deletions

File tree

src/tocode/cli.py

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -185,9 +185,6 @@ def main(argv: list[str] | None = None) -> int:
185185
else binary.parent / default_output_name(binary)
186186
)
187187
progress.set_log_path(log_root / "tocode.log")
188-
progress.log(
189-
f"Command: {' '.join(sys.argv if argv is None else ['tocode', *argv])}"
190-
)
191188
started = time.monotonic()
192189
try:
193190
if not binary.is_file():

src/tocode/exporter.py

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3061,13 +3061,17 @@ def _relativize_sources(files: list[str]) -> dict[str, str]:
30613061
for source_file in files:
30623062
if os.path.isabs(source_file) and root:
30633063
try:
3064-
result[source_file] = os.path.relpath(source_file, root)
3064+
# Emit POSIX separators so exports match across platforms
3065+
# (os.path.relpath yields backslashes on Windows).
3066+
result[source_file] = os.path.relpath(source_file, root).replace(
3067+
os.sep, "/"
3068+
)
30653069
continue
30663070
except ValueError:
30673071
pass
30683072
result[source_file] = (
30693073
source_file.lstrip("/") if os.path.isabs(source_file) else source_file
3070-
)
3074+
).replace(os.sep, "/")
30713075
return result
30723076

30733077

src/tocode/metadata.py

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
from pathlib import Path
77
import re
88
import tempfile
9+
import time
910
from typing import Callable
1011

1112
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:
756757
write_text_atomic(path, json.dumps(payload, indent=2, sort_keys=False) + "\n")
757758

758759

760+
_ATOMIC_REPLACE_ATTEMPTS = 10
761+
_ATOMIC_REPLACE_BASE_DELAY = 0.05 # seconds; ~2.75s worst case across attempts
762+
# Windows returns ERROR_ACCESS_DENIED (5) or ERROR_SHARING_VIOLATION (32) when
763+
# another process (antivirus, the search indexer, OneDrive) briefly holds the
764+
# destination or the temp file open without FILE_SHARE_DELETE at the instant of
765+
# the rename. These are transient, so retry before giving up.
766+
_TRANSIENT_REPLACE_WINERRORS = frozenset({5, 32})
767+
768+
769+
def _replace_with_retry(src: Path, dst: Path) -> None:
770+
for attempt in range(_ATOMIC_REPLACE_ATTEMPTS):
771+
try:
772+
src.replace(dst)
773+
return
774+
except PermissionError as exc:
775+
# winerror is None off Windows, so other platforms re-raise at once.
776+
transient = getattr(exc, "winerror", None) in _TRANSIENT_REPLACE_WINERRORS
777+
if not transient or attempt == _ATOMIC_REPLACE_ATTEMPTS - 1:
778+
raise
779+
time.sleep(_ATOMIC_REPLACE_BASE_DELAY * (attempt + 1))
780+
781+
759782
def write_text_atomic(path: Path, text: str) -> None:
760783
path.parent.mkdir(parents=True, exist_ok=True)
761784
with tempfile.NamedTemporaryFile(
@@ -768,4 +791,9 @@ def write_text_atomic(path: Path, text: str) -> None:
768791
) as handle:
769792
tmp_path = Path(handle.name)
770793
handle.write(text)
771-
tmp_path.replace(path)
794+
try:
795+
_replace_with_retry(tmp_path, path)
796+
except OSError:
797+
# Do not leave the temp file behind when the replace ultimately fails.
798+
tmp_path.unlink(missing_ok=True)
799+
raise

src/tocode/parallel.py

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -152,10 +152,15 @@ def available_memory_mb() -> int | None:
152152
meminfo = _linux_mem_available_mb()
153153
if meminfo is not None:
154154
return meminfo
155+
# os.sysconf is POSIX-only (absent on Windows); fetch it dynamically so the
156+
# attribute access does not fail static analysis on Windows.
157+
sysconf = getattr(os, "sysconf", None)
158+
if sysconf is None:
159+
return None
155160
try:
156-
pages = os.sysconf("SC_AVPHYS_PAGES")
157-
page_size = os.sysconf("SC_PAGE_SIZE")
158-
except (AttributeError, OSError, ValueError):
161+
pages = sysconf("SC_AVPHYS_PAGES")
162+
page_size = sysconf("SC_PAGE_SIZE")
163+
except (OSError, ValueError):
159164
return None
160165
if not isinstance(pages, int) or not isinstance(page_size, int):
161166
return None

tests/test_atomic_write.py

Lines changed: 170 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,170 @@
1+
"""Reproduction and regression tests for the Windows atomic-write crash (#11).
2+
3+
`write_text_atomic` renames a temp file over the destination. On Windows that
4+
rename fails with ``PermissionError`` (``WinError 5`` access-denied, or ``32``
5+
sharing-violation) whenever another process — antivirus, the search indexer,
6+
OneDrive — briefly holds the destination or the temp file open without
7+
``FILE_SHARE_DELETE``. Under ``-j 8`` the checkpoint is rewritten on every
8+
completed function, so such a collision eventually aborts the whole export.
9+
10+
The portable test reproduces the mechanism on any platform by monkeypatching the
11+
rename; the Windows test reproduces the exact production failure with a real lock.
12+
"""
13+
14+
from __future__ import annotations
15+
16+
import sys
17+
from pathlib import Path
18+
19+
import pytest
20+
21+
from tocode.metadata import write_text_atomic
22+
23+
24+
def _win_permission_error(winerror: int) -> PermissionError:
25+
exc = PermissionError(winerror, "Access is denied")
26+
# Mirror the OSError.winerror attribute CPython sets on Windows so the code
27+
# under test can distinguish transient lock errors from real ones.
28+
exc.winerror = winerror # type: ignore[attr-defined]
29+
return exc
30+
31+
32+
def _tmp_siblings(path: Path) -> list[Path]:
33+
return [p for p in path.parent.iterdir() if p.name != path.name]
34+
35+
36+
def test_replace_retries_past_transient_lock(tmp_path, monkeypatch) -> None:
37+
"""A few transient WinError 5 failures must not abort the write."""
38+
target = tmp_path / "checkpoint.json"
39+
real_replace = Path.replace
40+
calls = {"n": 0}
41+
42+
def flaky_replace(self: Path, dst):
43+
calls["n"] += 1
44+
if calls["n"] <= 3: # transient lock clears after a few attempts
45+
raise _win_permission_error(5)
46+
return real_replace(self, dst)
47+
48+
monkeypatch.setattr(Path, "replace", flaky_replace)
49+
monkeypatch.setattr("tocode.metadata.time.sleep", lambda _s: None)
50+
51+
write_text_atomic(target, "payload\n")
52+
53+
assert calls["n"] == 4 # 3 failures + 1 success
54+
assert target.read_text(encoding="utf-8") == "payload\n"
55+
# No orphan .tmp left behind.
56+
assert _tmp_siblings(target) == []
57+
58+
59+
def test_replace_gives_up_and_cleans_tmp_when_lock_never_clears(
60+
tmp_path, monkeypatch
61+
) -> None:
62+
"""A permanent lock still raises, but must not litter .tmp orphans."""
63+
target = tmp_path / "checkpoint.json"
64+
65+
def always_locked(self: Path, dst):
66+
raise _win_permission_error(5)
67+
68+
monkeypatch.setattr(Path, "replace", always_locked)
69+
monkeypatch.setattr("tocode.metadata.time.sleep", lambda _s: None)
70+
71+
with pytest.raises(PermissionError):
72+
write_text_atomic(target, "payload\n")
73+
74+
assert not target.exists()
75+
assert _tmp_siblings(target) == []
76+
77+
78+
def test_non_windows_permission_error_is_not_retried(tmp_path, monkeypatch) -> None:
79+
"""A PermissionError without a transient winerror re-raises immediately."""
80+
target = tmp_path / "checkpoint.json"
81+
calls = {"n": 0}
82+
83+
def denied(self: Path, dst):
84+
calls["n"] += 1
85+
raise PermissionError(13, "Permission denied") # e.g. read-only dir
86+
87+
monkeypatch.setattr(Path, "replace", denied)
88+
monkeypatch.setattr("tocode.metadata.time.sleep", lambda _s: None)
89+
90+
with pytest.raises(PermissionError):
91+
write_text_atomic(target, "payload\n")
92+
93+
assert calls["n"] == 1 # not retried
94+
95+
96+
# --- Faithful Windows reproduction with a real file lock ------------------
97+
98+
_WINDOWS_ONLY = pytest.mark.skipif(
99+
sys.platform != "win32", reason="reproduces a Windows file-sharing error"
100+
)
101+
102+
103+
def _open_read_share_no_delete(path: Path):
104+
"""Open ``path`` the way a scanner does: read-share, no delete-share.
105+
106+
Returns a Windows HANDLE that blocks anyone from replacing/deleting the file
107+
until it is closed. Mirrors antivirus / OneDrive real-time access.
108+
"""
109+
import ctypes
110+
111+
GENERIC_READ = 0x80000000
112+
FILE_SHARE_READ = 0x00000001
113+
OPEN_EXISTING = 3
114+
INVALID_HANDLE_VALUE = ctypes.c_void_p(-1).value
115+
116+
# windll is Windows-only (absent from the ctypes stubs on other platforms),
117+
# so reach it dynamically to keep static analysis happy on the Linux CI. A
118+
# HANDLE is a void*, so use c_void_p rather than the win-only wintypes.
119+
kernel32 = getattr(ctypes, "windll").kernel32
120+
kernel32.CreateFileW.restype = ctypes.c_void_p
121+
handle = kernel32.CreateFileW(
122+
str(path),
123+
GENERIC_READ,
124+
FILE_SHARE_READ, # deliberately no FILE_SHARE_DELETE
125+
None,
126+
OPEN_EXISTING,
127+
0,
128+
None,
129+
)
130+
if handle == INVALID_HANDLE_VALUE:
131+
raise OSError(f"CreateFileW failed to lock {path}")
132+
return handle
133+
134+
135+
def _close_handle(handle) -> None:
136+
import ctypes
137+
138+
getattr(ctypes, "windll").kernel32.CloseHandle(handle)
139+
140+
141+
@_WINDOWS_ONLY
142+
def test_windows_permanent_lock_reproduces_winerror5(tmp_path) -> None:
143+
target = tmp_path / "checkpoint.json"
144+
target.write_text("old\n", encoding="utf-8")
145+
handle = _open_read_share_no_delete(target)
146+
try:
147+
with pytest.raises(PermissionError) as info:
148+
write_text_atomic(target, "new\n")
149+
assert getattr(info.value, "winerror", None) in (5, 32)
150+
finally:
151+
_close_handle(handle)
152+
153+
154+
@_WINDOWS_ONLY
155+
def test_windows_transient_lock_recovers(tmp_path) -> None:
156+
import threading
157+
import time
158+
159+
target = tmp_path / "checkpoint.json"
160+
target.write_text("old\n", encoding="utf-8")
161+
handle = _open_read_share_no_delete(target)
162+
163+
# Release the lock shortly after the write starts, as a scanner would.
164+
threading.Timer(0.15, lambda: _close_handle(handle)).start()
165+
166+
started = time.monotonic()
167+
write_text_atomic(target, "new\n") # retries until the lock clears
168+
assert target.read_text(encoding="utf-8") == "new\n"
169+
assert time.monotonic() - started >= 0.1
170+
assert _tmp_siblings(target) == []

0 commit comments

Comments
 (0)