Skip to content

Commit 19d7cf7

Browse files
committed
fix(core.utils): clamp tracked size at zero on __delitem__
`__delitem__` could walk `_tracked_size_bytes` negative under a race with `_enforce_size_cap`'s reseed: if the eviction scan runs AFTER this delete unlinks (so its reseed value excludes the deleted entry) but BEFORE this delete's subtract, the subtract undercounts by `size`. Repeated under contention, the tracker crosses zero -- and once negative, the `tracker > cap` check that gates eviction never fires again, so the cache grows without bound and there is no self-healing path (the only reseed point is the function that no longer runs). Clamp `tracker = max(0, tracker - size)` so the tracker can't enter the permanently-broken state. Worst case after the race is undercounting reality, which the next eviction's reseed corrects; that's the same self-healing path the existing tests already exercise. Reported by leofang in PR review.
1 parent b6a2590 commit 19d7cf7

2 files changed

Lines changed: 86 additions & 1 deletion

File tree

cuda_core/cuda/core/utils/_program_cache/_file_stream.py

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -523,7 +523,19 @@ def __delitem__(self, key: object) -> None:
523523
raise KeyError(key) from None
524524
if self._max_size_bytes is not None:
525525
with self._size_lock:
526-
self._tracked_size_bytes -= size
526+
# Clamp at zero. A racing ``_enforce_size_cap`` can re-seed the
527+
# tracker between our stat and our subtract; if its scan ran
528+
# AFTER we unlinked, its reseed value didn't include ``size``,
529+
# so subtracting ``size`` again here would undercount reality
530+
# by ``size``. Repeated under contention, an unclamped subtract
531+
# walks the tracker negative -- and once negative, the
532+
# ``tracker > cap`` check that gates ``_enforce_size_cap``
533+
# never fires, so eviction dies silently and there is no
534+
# self-healing path (the only reseed point is the function
535+
# that no longer runs). Clamping leaves us at worst
536+
# undercounting (the next reseed corrects it) instead of
537+
# entering the permanently-broken negative state.
538+
self._tracked_size_bytes = max(0, self._tracked_size_bytes - size)
527539

528540
def __len__(self) -> int:
529541
"""Return the number of files currently in ``entries/``.

cuda_core/tests/test_program_cache.py

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1927,6 +1927,79 @@ def test_filestream_cache_tracker_reconciles_after_external_drift(tmp_path):
19271927
assert cache._tracked_size_bytes <= 1100 # actual on-disk is 'b' + 'c' or just 'c'
19281928

19291929

1930+
def test_filestream_cache_tracker_clamps_at_zero_under_delete_race(tmp_path):
1931+
"""Two-thread reproduction of the ``__delitem__`` vs
1932+
``_enforce_size_cap`` race. Thread A is mid-delete: it has stat'd the
1933+
victim (size=900) and unlinked it but hasn't yet decremented the
1934+
tracker. Thread B then runs ``_enforce_size_cap``, scans the
1935+
directory (sees only the survivor), and reseeds tracker = 100.
1936+
Thread A finally runs its subtract: ``100 - 900 = -800`` without the
1937+
clamp. Once negative, the ``tracker > cap`` check that gates
1938+
``_enforce_size_cap`` never fires again, so eviction silently dies
1939+
and the cache grows without bound -- there is no self-healing path
1940+
because the only reseed point is the function that no longer runs.
1941+
The clamp keeps the tracker in the self-healing range.
1942+
1943+
Sequenced via :class:`threading.Event` (not sleeps): A signals when
1944+
it has unlinked, then waits; B observes the unlinked disk, runs the
1945+
eviction reseed, and signals back. The interleaving is deterministic."""
1946+
import threading
1947+
1948+
from cuda.core.utils import FileStreamProgramCache
1949+
from cuda.core.utils._program_cache import _file_stream
1950+
1951+
cache = FileStreamProgramCache(tmp_path / "fc", max_size_bytes=10_000)
1952+
cache[b"victim"] = b"X" * 900
1953+
cache[b"survivor"] = b"X" * 100
1954+
assert cache._tracked_size_bytes == 1000
1955+
1956+
a_unlinked = threading.Event()
1957+
b_reseeded = threading.Event()
1958+
real_unlink = _file_stream._unlink_with_sharing_retry
1959+
1960+
def coordinated_unlink(path):
1961+
# Hook into thread A's __delitem__: do the actual unlink, then
1962+
# hand control to thread B until it has run its reseed.
1963+
real_unlink(path)
1964+
a_unlinked.set()
1965+
assert b_reseeded.wait(timeout=5), "thread B didn't reseed in time"
1966+
1967+
delete_error: list[BaseException] = []
1968+
1969+
def thread_a():
1970+
try:
1971+
del cache[b"victim"]
1972+
except BaseException as exc: # pragma: no cover - surfaces below
1973+
delete_error.append(exc)
1974+
finally:
1975+
# Don't strand thread B if __delitem__ raised before the hook ran.
1976+
a_unlinked.set()
1977+
1978+
def thread_b():
1979+
assert a_unlinked.wait(timeout=5), "thread A didn't unlink in time"
1980+
cache._enforce_size_cap() # reseeds tracker = 100 (only 'survivor' on disk)
1981+
b_reseeded.set()
1982+
1983+
_file_stream._unlink_with_sharing_retry = coordinated_unlink
1984+
try:
1985+
ta = threading.Thread(target=thread_a)
1986+
tb = threading.Thread(target=thread_b)
1987+
ta.start()
1988+
tb.start()
1989+
ta.join(timeout=10)
1990+
tb.join(timeout=10)
1991+
finally:
1992+
_file_stream._unlink_with_sharing_retry = real_unlink
1993+
1994+
assert not delete_error, f"__delitem__ raised: {delete_error[0]!r}"
1995+
assert not ta.is_alive() and not tb.is_alive(), "race threads didn't finish"
1996+
# Without the clamp, this would be -800. With it, max(0, 100-900) = 0.
1997+
# The tracker still undercounts disk truth (which holds 'survivor' = 100),
1998+
# but the next reseed will correct that, whereas a negative tracker would
1999+
# have permanently disabled eviction.
2000+
assert cache._tracked_size_bytes == 0, f"tracker went negative: {cache._tracked_size_bytes}"
2001+
2002+
19302003
def test_make_program_cache_key_changes_with_key_schema_version(monkeypatch):
19312004
"""Bumping ``_KEY_SCHEMA_VERSION`` produces a different cache key for
19322005
the same logical inputs. That's what makes a schema bump invalidate

0 commit comments

Comments
 (0)