Skip to content

Commit 417ad35

Browse files
committed
fix(core.utils): only treat bare EACCES as a Windows sharing violation
Roborev review #1744 (medium): the EACCES fallback in ``_is_windows_sharing_violation`` (and the matching branches in the retry helpers) accepted any ``EACCES`` even when ``winerror`` was set to a non-sharing code -- silently swallowing real config failures that the OS named (e.g. winerror 1224, ERROR_USER_MAPPED_FILE). Tighten the predicate: ``EACCES`` only counts as transient when ``winerror`` is absent. ``io.open`` surfaces pending-delete / share-mode mismatches as bare ``EACCES`` with no winerror; that's what the fallback exists to handle. When ``winerror`` IS set and is not in the sharing set, the OS told us exactly what failed and we must propagate. Replace the inline branches in ``_unlink_with_sharing_retry`` and ``_stat_and_read_with_sharing_retry`` with calls to the now-tighter ``_is_windows_sharing_violation`` so all three sites behave identically. Roborev review #1748 (medium): ``__len__`` counts orphan files left behind after a ``_KEY_SCHEMA_VERSION`` bump (per the prior commit's intentional drop of the schema-mismatch wipe). Document the looseness on both ``ProgramCacheResource.__len__`` and ``FileStreamProgramCache.__len__`` so callers know not to rely on exactness across schema bumps. Tests cover the predicate's table of cases, including the regression the fix targets (non-sharing winerror + EACCES propagates).
1 parent 97dd2bf commit 417ad35

2 files changed

Lines changed: 106 additions & 13 deletions

File tree

cuda_core/cuda/core/utils/_program_cache.py

Lines changed: 33 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -171,7 +171,15 @@ def __delitem__(self, key: bytes | str) -> None:
171171

172172
@abc.abstractmethod
173173
def __len__(self) -> int:
174-
"""Return the number of entries currently in the cache."""
174+
"""Return the number of entries currently in the cache.
175+
176+
Implementations that store entries on disk by hashed key may
177+
count orphaned files (entries from a previous
178+
``_KEY_SCHEMA_VERSION`` that are still on disk but no longer
179+
reachable by post-bump lookups) until eviction reaps them.
180+
Callers that need an exact count of live entries should not
181+
rely on ``__len__`` across schema bumps.
182+
"""
175183

176184
@abc.abstractmethod
177185
def clear(self) -> None:
@@ -1090,10 +1098,7 @@ def _stat_and_read_with_sharing_retry(path: Path) -> tuple[os.stat_result, bytes
10901098
except FileNotFoundError:
10911099
raise
10921100
except PermissionError as exc:
1093-
if not _IS_WINDOWS:
1094-
raise
1095-
winerror = getattr(exc, "winerror", None)
1096-
if winerror not in _SHARING_VIOLATION_WINERRORS and exc.errno != errno.EACCES:
1101+
if not _is_windows_sharing_violation(exc):
10971102
raise
10981103
last_exc = exc
10991104
raise FileNotFoundError(path) from last_exc
@@ -1191,13 +1196,23 @@ def _is_windows_sharing_violation(exc: BaseException) -> bool:
11911196
while letting other ``PermissionError`` instances (POSIX ACL
11921197
issues, Windows non-sharing winerrors) propagate -- those are real
11931198
configuration problems, not transient contention.
1199+
1200+
The ``EACCES`` fallback only fires when ``winerror`` is absent: a
1201+
bare ``EACCES`` (no winerror attached) is the way ``io.open``
1202+
surfaces a pending-delete or share-mode mismatch on Windows. When
1203+
``winerror`` IS set but is NOT in the sharing set, the OS told us
1204+
exactly what failed and it isn't a sharing violation -- treating it
1205+
as transient would silently swallow real errors like a corrupt
1206+
ACL.
11941207
"""
11951208
if not _IS_WINDOWS:
11961209
return False
11971210
if not isinstance(exc, PermissionError):
11981211
return False
11991212
winerror = getattr(exc, "winerror", None)
1200-
return winerror in _SHARING_VIOLATION_WINERRORS or exc.errno == errno.EACCES
1213+
if winerror in _SHARING_VIOLATION_WINERRORS:
1214+
return True
1215+
return winerror is None and exc.errno == errno.EACCES
12011216

12021217

12031218
def _unlink_with_sharing_retry(path: Path) -> None:
@@ -1228,10 +1243,7 @@ def _unlink_with_sharing_retry(path: Path) -> None:
12281243
except FileNotFoundError:
12291244
raise
12301245
except PermissionError as exc:
1231-
if not _IS_WINDOWS:
1232-
raise
1233-
winerror = getattr(exc, "winerror", None)
1234-
if winerror not in _SHARING_VIOLATION_WINERRORS and exc.errno != errno.EACCES:
1246+
if not _is_windows_sharing_violation(exc):
12351247
raise
12361248
last_exc = exc
12371249
if last_exc is not None:
@@ -1437,9 +1449,17 @@ def __delitem__(self, key: object) -> None:
14371449
raise KeyError(key) from None
14381450

14391451
def __len__(self) -> int:
1440-
# Count present entry files. There is no payload validation at
1441-
# the cache layer (entries are raw binary, not framed records),
1442-
# so anything that exists in ``entries/`` is a member.
1452+
"""Return the number of files currently in ``entries/``.
1453+
1454+
This is a count of on-disk files, not of keys reachable through
1455+
``make_program_cache_key``. After a ``_KEY_SCHEMA_VERSION`` bump
1456+
old entries become unreachable by lookup but remain on disk
1457+
until eviction reaps them; ``__len__`` keeps counting them
1458+
until then. The same is true for entries written by callers
1459+
using arbitrary user keys -- the backend has no way to tell a
1460+
live entry from an orphan without knowing the caller's keying
1461+
scheme.
1462+
"""
14431463
count = 0
14441464
for path in self._iter_entry_paths():
14451465
if path.is_file():

cuda_core/tests/test_program_cache.py

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -837,6 +837,79 @@ def _denied(self, *args, **kwargs):
837837
cache[b"c"] = b"c" * 100
838838

839839

840+
def test_is_windows_sharing_violation_predicate(monkeypatch):
841+
"""``_is_windows_sharing_violation`` must:
842+
843+
* recognise winerror 5/32/33 as transient sharing violations,
844+
* recognise a bare ``EACCES`` (no winerror) as transient too,
845+
* NOT swallow a Windows ``PermissionError`` whose winerror is
846+
something else (e.g. winerror 1224 = ``ERROR_USER_MAPPED_FILE``)
847+
even when ``errno`` happens to be ``EACCES``,
848+
* never return True off-Windows.
849+
"""
850+
import errno as _errno
851+
852+
from cuda.core.utils import _program_cache
853+
854+
monkeypatch.setattr(_program_cache, "_IS_WINDOWS", True)
855+
856+
def _make(winerror, errno_value):
857+
exc = PermissionError("test")
858+
if winerror is not None:
859+
exc.winerror = winerror
860+
exc.errno = errno_value
861+
return exc
862+
863+
# Sharing winerrors: True regardless of errno.
864+
for w in _program_cache._SHARING_VIOLATION_WINERRORS:
865+
assert _program_cache._is_windows_sharing_violation(_make(w, _errno.EACCES))
866+
assert _program_cache._is_windows_sharing_violation(_make(w, _errno.EPERM))
867+
868+
# Bare EACCES (no winerror): True.
869+
assert _program_cache._is_windows_sharing_violation(_make(None, _errno.EACCES))
870+
871+
# No winerror, non-EACCES errno: False.
872+
assert not _program_cache._is_windows_sharing_violation(_make(None, _errno.EPERM))
873+
874+
# Windows non-sharing winerror plus EACCES: False (the regression
875+
# this guard is here for; a non-sharing winerror tells the OS
876+
# exactly what failed and we must not silently swallow it).
877+
assert not _program_cache._is_windows_sharing_violation(_make(1224, _errno.EACCES))
878+
assert not _program_cache._is_windows_sharing_violation(_make(1224, _errno.EPERM))
879+
880+
# Off-Windows: always False.
881+
monkeypatch.setattr(_program_cache, "_IS_WINDOWS", False)
882+
assert not _program_cache._is_windows_sharing_violation(_make(32, _errno.EACCES))
883+
assert not _program_cache._is_windows_sharing_violation(_make(None, _errno.EACCES))
884+
885+
886+
def test_filestream_cache_eviction_propagates_windows_non_sharing_eacces(tmp_path, monkeypatch):
887+
"""Even on Windows, a ``PermissionError`` carrying a non-sharing
888+
``winerror`` plus ``errno.EACCES`` must propagate -- the OS named the
889+
failure mode, and silently swallowing it would mask real config
890+
problems."""
891+
import errno as _errno
892+
from pathlib import Path
893+
894+
from cuda.core.utils import FileStreamProgramCache, _program_cache
895+
896+
monkeypatch.setattr(_program_cache, "_IS_WINDOWS", True)
897+
898+
with FileStreamProgramCache(tmp_path / "fc", max_size_bytes=200) as cache:
899+
cache[b"a"] = b"a" * 100
900+
cache[b"b"] = b"b" * 100
901+
902+
def _named_failure(self, *args, **kwargs):
903+
exc = PermissionError("user mapped file")
904+
exc.winerror = 1224 # ERROR_USER_MAPPED_FILE -- not a sharing violation
905+
exc.errno = _errno.EACCES
906+
raise exc
907+
908+
monkeypatch.setattr(Path, "unlink", _named_failure)
909+
with pytest.raises(PermissionError, match="user mapped file"):
910+
cache[b"c"] = b"c" * 100
911+
912+
840913
@pytest.mark.parametrize(
841914
"option_kw",
842915
[

0 commit comments

Comments
 (0)