From 175ce653270a89962bdfd819913961c1b0f59ce8 Mon Sep 17 00:00:00 2001 From: Shane Grigsby Date: Fri, 5 Jun 2026 10:51:12 -0700 Subject: [PATCH 01/15] prototype for negative caching, i.e., get misses --- changes/4040.feature.md | 1 + src/zarr/experimental/cache_store.py | 118 +++++++++++++++- tests/test_experimental/test_cache_store.py | 141 ++++++++++++++++++++ 3 files changed, 258 insertions(+), 2 deletions(-) create mode 100644 changes/4040.feature.md diff --git a/changes/4040.feature.md b/changes/4040.feature.md new file mode 100644 index 0000000000..4d9f2fb9ee --- /dev/null +++ b/changes/4040.feature.md @@ -0,0 +1 @@ +`zarr.experimental.cache_store.CacheStore` gained opt-in negative caching via `cache_missing=True`. When enabled, a full-key read that finds the key absent in the source store is remembered, so repeat reads of that absent key return immediately without a source round-trip — useful for sparse arrays where most chunks resolve to the fill value. Negative entries respect `max_age_seconds`, are bounded by `max_missing_keys` (default 100,000, least-recently-used eviction), and are evicted when the key is written via `set`/`set_if_not_exists`. The default is `False` (no behavior change); negative-cache activity is reported as `negative_hits` in `cache_stats()` and `missing_keys` in `cache_info()`. Only full-key reads are affected (not byte-range reads or `exists`). diff --git a/src/zarr/experimental/cache_store.py b/src/zarr/experimental/cache_store.py index 1535b42f67..380b5e81d2 100644 --- a/src/zarr/experimental/cache_store.py +++ b/src/zarr/experimental/cache_store.py @@ -30,8 +30,14 @@ class _CacheState: hits: int = 0 misses: int = 0 evictions: int = 0 + negative_hits: int = 0 key_insert_times: dict[_CacheEntryKey, float] = field(default_factory=dict) range_cache: dict[str, dict[ByteRequest, Buffer]] = field(default_factory=dict) + # Negative cache: full keys known to be absent in the source store, mapped to + # their (monotonic) insertion time for freshness. OrderedDict gives O(1) LRU + # eviction via popitem(last=False). Kept separate from the byte-size accounting + # above (negative entries carry no data) and bounded by ``max_missing_keys``. + missing_keys: OrderedDict[str, float] = field(default_factory=OrderedDict) class CacheStore(WrapperStore[Store]): @@ -62,6 +68,23 @@ class CacheStore(WrapperStore[Store]): Note: Individual values larger than max_size will not be cached. cache_set_data : bool, optional Whether to cache data when it's written to the store. Default is True. + cache_missing : bool, optional + Whether to remember full-key misses (negative caching). When True, a full-key + ``get`` that finds the key absent in the source store records that absence, so + subsequent ``get``s for the same key return ``None`` without a source round-trip. + This benefits repeated reads of sparse arrays (most chunks absent). Negative + entries respect ``max_age_seconds`` and are evicted when the key is written + (``set``/``set_if_not_exists``). Only full-key reads are affected (not byte-range + reads or ``exists``). Default is False. + + Note: with ``max_age_seconds="infinity"`` a remembered miss never expires, so a + key written to the source by another process would stay invisible through this + cache. Pair ``cache_missing=True`` with a finite ``max_age_seconds`` if the source + may be written concurrently. + max_missing_keys : int, optional + Maximum number of negative (missing-key) entries to retain when + ``cache_missing`` is True. When exceeded, the least recently used missing keys + are evicted. Bounds memory for large sparse scans. Default is 100,000. Examples -------- @@ -91,6 +114,8 @@ class CacheStore(WrapperStore[Store]): max_age_seconds: int | Literal["infinity"] max_size: int | None cache_set_data: bool + cache_missing: bool + max_missing_keys: int _state: _CacheState def __init__( @@ -101,6 +126,8 @@ def __init__( max_age_seconds: int | str = "infinity", max_size: int | None = None, cache_set_data: bool = True, + cache_missing: bool = False, + max_missing_keys: int = 100_000, ) -> None: super().__init__(store) @@ -111,6 +138,9 @@ def __init__( ) raise ValueError(msg) + if max_missing_keys < 1: + raise ValueError("max_missing_keys must be a positive integer") + self._cache = cache_store # Validate and set max_age_seconds if isinstance(max_age_seconds, str): @@ -121,6 +151,8 @@ def __init__( self.max_age_seconds = max_age_seconds self.max_size = max_size self.cache_set_data = cache_set_data + self.cache_missing = cache_missing + self.max_missing_keys = max_missing_keys self._state = _CacheState() def _with_store(self, store: Store) -> Self: @@ -136,6 +168,8 @@ def with_read_only(self, read_only: bool = False) -> Self: max_age_seconds=self.max_age_seconds, max_size=self.max_size, cache_set_data=self.cache_set_data, + cache_missing=self.cache_missing, + max_missing_keys=self.max_missing_keys, ) store._state = self._state return store @@ -151,6 +185,34 @@ def _is_key_fresh(self, entry_key: _CacheEntryKey) -> bool: elapsed = now - self._state.key_insert_times.get(entry_key, 0) return elapsed < self.max_age_seconds + def _is_missing_fresh(self, key: str) -> bool: + """Check if a negative (missing-key) entry is still fresh. + + Mirrors ``_is_key_fresh`` but reads the negative-cache insertion time. + """ + if self.max_age_seconds == "infinity": + return True + elapsed = time.monotonic() - self._state.missing_keys.get(key, 0.0) + return elapsed < self.max_age_seconds + + def _record_missing(self, key: str) -> None: + """Record *key* as known-missing, evicting the oldest entries past the cap. + + Must be called while holding ``self._state.lock``. + """ + self._state.missing_keys[key] = time.monotonic() + self._state.missing_keys.move_to_end(key) + while len(self._state.missing_keys) > self.max_missing_keys: + self._state.missing_keys.popitem(last=False) + self._state.evictions += 1 + + def _evict_missing(self, key: str) -> None: + """Drop any negative entry for *key* (it is now present or being written). + + Must be called while holding ``self._state.lock``. + """ + self._state.missing_keys.pop(key, None) + async def _accommodate_value(self, value_size: int) -> None: """Ensure there is enough space in the cache for a new value. @@ -266,6 +328,10 @@ async def _cache_miss( await self._cache.delete(key) async with self._state.lock: self._remove_from_tracking(key) + # The key is absent in the source: remember the miss so a repeat + # read can short-circuit without a source round-trip. + if self.cache_missing: + self._record_missing(key) else: entry_key: _CacheEntryKey = (key, byte_range) async with self._state.lock: @@ -279,6 +345,10 @@ async def _cache_miss( if byte_range is None: await self._cache.set(key, result) await self._track_entry(key, result) + # A value now exists for this key: drop any stale negative entry. + if self.cache_missing: + async with self._state.lock: + self._evict_missing(key) else: entry_key = (key, byte_range) self._state.range_cache.setdefault(key, {})[byte_range] = result @@ -351,6 +421,17 @@ async def get( Buffer | None The retrieved data, or None if not found """ + # Negative cache fast-path (full-key reads only): a fresh "known absent" record + # short-circuits to None without consulting the positive cache or the source. + # Checked here, before the positive-entry freshness gate, because a negative-only + # key has no positive entry and would otherwise be routed straight to the source. + if self.cache_missing and byte_range is None: + async with self._state.lock: + if key in self._state.missing_keys and self._is_missing_fresh(key): + self._state.negative_hits += 1 + self._state.missing_keys.move_to_end(key) + return None + entry_key: _CacheEntryKey = (key, byte_range) if byte_range is not None else key if not self._is_key_fresh(entry_key): return await self._get_no_cache(key, prototype, byte_range) @@ -369,9 +450,12 @@ async def set(self, key: str, value: Buffer) -> None: The data to store """ await super().set(key, value) - # Invalidate all cached byte-range entries (source data changed) + # Invalidate all cached byte-range entries (source data changed) and drop any + # negative entry — the key now has a value. async with self._state.lock: self._invalidate_range_entries(key) + if self.cache_missing: + self._evict_missing(key) if self.cache_set_data: await self._cache.set(key, value) await self._track_entry(key, value) @@ -380,6 +464,26 @@ async def set(self, key: str, value: Buffer) -> None: async with self._state.lock: self._remove_from_tracking(key) + async def set_if_not_exists(self, key: str, value: Buffer) -> None: + """ + Store data only if the key does not already exist in the source store. + + Parameters + ---------- + key : str + The key to store under + value : Buffer + The data to store + """ + await super().set_if_not_exists(key, value) + # Whether or not the write happened, any negative entry is now unsafe: either + # we just wrote the key, or it already existed (so the record was already + # wrong). Evicting unconditionally is always safe. We do not populate the + # positive cache here — there is no guaranteed-fresh value to store. + if self.cache_missing: + async with self._state.lock: + self._evict_missing(key) + async def delete(self, key: str) -> None: """ Delete data from both the underlying store and cache. @@ -407,18 +511,26 @@ def cache_info(self) -> dict[str, Any]: "max_size": self.max_size, "current_size": self._state.current_size, "cache_set_data": self.cache_set_data, + "cache_missing": self.cache_missing, "tracked_keys": len(self._state.key_insert_times), "cached_keys": len(self._state.cache_order), + "missing_keys": len(self._state.missing_keys), } def cache_stats(self) -> dict[str, Any]: - """Return cache performance statistics.""" + """Return cache performance statistics. + + ``hit_rate`` reflects positive-cache hits over positive lookups only; a + negative-cache hit (an absent key served from the negative cache) is reported + separately as ``negative_hits`` and is counted as neither a hit nor a miss. + """ total_requests = self._state.hits + self._state.misses hit_rate = self._state.hits / total_requests if total_requests > 0 else 0.0 return { "hits": self._state.hits, "misses": self._state.misses, "evictions": self._state.evictions, + "negative_hits": self._state.negative_hits, "total_requests": total_requests, "hit_rate": hit_rate, } @@ -435,7 +547,9 @@ async def clear_cache(self) -> None: self._state.cache_order.clear() self._state.key_sizes.clear() self._state.range_cache.clear() + self._state.missing_keys.clear() self._state.current_size = 0 + self._state.negative_hits = 0 def __repr__(self) -> str: """Return string representation of the cache store.""" diff --git a/tests/test_experimental/test_cache_store.py b/tests/test_experimental/test_cache_store.py index fc17ccd5e1..8181a3044a 100644 --- a/tests/test_experimental/test_cache_store.py +++ b/tests/test_experimental/test_cache_store.py @@ -298,8 +298,10 @@ async def test_cache_info(self, cached_store: CacheStore) -> None: "max_size", "current_size", "cache_set_data", + "cache_missing", "tracked_keys", "cached_keys", + "missing_keys", } assert set(info.keys()) == expected_keys @@ -1047,3 +1049,142 @@ async def test_delete_invalidates_cached_byte_ranges(self) -> None: # Key is gone from source result = await cached_store.get("key", proto) assert result is None + + +class TestCacheStoreNegativeCaching: + """Tests for opt-in negative (missing-key) caching (``cache_missing=True``).""" + + async def test_basic(self, monkeypatch: pytest.MonkeyPatch) -> None: + """A second get of an absent key is served from the negative cache without a + source round-trip.""" + source = MemoryStore() + cs = CacheStore(source, cache_store=MemoryStore(), cache_missing=True) + proto = default_buffer_prototype() + + calls = {"n": 0} + orig_get = source.get + + async def counting_get(*args: object, **kwargs: object) -> object: + calls["n"] += 1 + return await orig_get(*args, **kwargs) # type: ignore[arg-type] + + monkeypatch.setattr(source, "get", counting_get) + + assert await cs.get("c/0", proto) is None + assert cs.cache_info()["missing_keys"] == 1 + after_first = calls["n"] + + assert await cs.get("c/0", proto) is None + assert calls["n"] == after_first # no further source access + assert cs.cache_stats()["negative_hits"] == 1 + + async def test_disabled_by_default(self) -> None: + """With the default ``cache_missing=False`` nothing is remembered.""" + cs = CacheStore(MemoryStore(), cache_store=MemoryStore()) + proto = default_buffer_prototype() + assert await cs.get("c/0", proto) is None + assert await cs.get("c/0", proto) is None + assert cs.cache_info()["missing_keys"] == 0 + assert cs.cache_stats()["negative_hits"] == 0 + + async def test_evicted_on_set(self) -> None: + source = MemoryStore() + cs = CacheStore(source, cache_store=MemoryStore(), cache_missing=True) + proto = default_buffer_prototype() + assert await cs.get("c/0", proto) is None + assert cs.cache_info()["missing_keys"] == 1 + + await cs.set("c/0", CPUBuffer.from_bytes(b"value")) + assert cs.cache_info()["missing_keys"] == 0 + result = await cs.get("c/0", proto) + assert result is not None + assert result.to_bytes() == b"value" + + async def test_evicted_on_set_if_not_exists(self) -> None: + source = MemoryStore() + cs = CacheStore(source, cache_store=MemoryStore(), cache_missing=True) + proto = default_buffer_prototype() + assert await cs.get("c/0", proto) is None + assert cs.cache_info()["missing_keys"] == 1 + + await cs.set_if_not_exists("c/0", CPUBuffer.from_bytes(b"value")) + assert cs.cache_info()["missing_keys"] == 0 + result = await cs.get("c/0", proto) + assert result is not None + assert result.to_bytes() == b"value" + + async def test_respects_ttl(self) -> None: + """A negative entry expires after ``max_age_seconds`` so a key written to the + source out-of-band becomes visible again.""" + source = MemoryStore() + cs = CacheStore(source, cache_store=MemoryStore(), cache_missing=True, max_age_seconds=1) + proto = default_buffer_prototype() + assert await cs.get("c/0", proto) is None + + # an external writer adds the key directly to the source store + await source.set("c/0", CPUBuffer.from_bytes(b"late")) + + # before TTL: still reported missing from the negative cache + assert await cs.get("c/0", proto) is None + await asyncio.sleep(1.1) + + # after TTL: the stale negative entry is bypassed, source is consulted + result = await cs.get("c/0", proto) + assert result is not None + assert result.to_bytes() == b"late" + assert cs.cache_info()["missing_keys"] == 0 + + async def test_bounded(self) -> None: + """``max_missing_keys`` bounds the negative cache, evicting LRU entries.""" + cs = CacheStore( + MemoryStore(), cache_store=MemoryStore(), cache_missing=True, max_missing_keys=10 + ) + proto = default_buffer_prototype() + for i in range(25): + assert await cs.get(f"c/{i}", proto) is None + + assert cs.cache_info()["missing_keys"] == 10 + assert cs.cache_stats()["evictions"] >= 15 + # the 10 most-recently-seen keys are retained (LRU) + for i in range(15, 25): + assert f"c/{i}" in cs._state.missing_keys + for i in range(15): + assert f"c/{i}" not in cs._state.missing_keys + + async def test_byte_range_unaffected(self) -> None: + """Byte-range misses do not populate the negative cache.""" + cs = CacheStore(MemoryStore(), cache_store=MemoryStore(), cache_missing=True) + proto = default_buffer_prototype() + assert await cs.get("c/0", proto, byte_range=RangeByteRequest(0, 4)) is None + assert cs.cache_info()["missing_keys"] == 0 + + async def test_stats_and_info(self) -> None: + """``negative_hits``/``missing_keys``/``cache_missing`` are surfaced and the + positive ``hit_rate`` is unaffected by negative hits.""" + source = MemoryStore() + cs = CacheStore(source, cache_store=MemoryStore(), cache_missing=True) + proto = default_buffer_prototype() + + await cs.set("present", CPUBuffer.from_bytes(b"x")) + assert (await cs.get("present", proto)) is not None # positive hit + assert await cs.get("absent", proto) is None # records miss + assert await cs.get("absent", proto) is None # negative hit + + info = cs.cache_info() + stats = cs.cache_stats() + assert info["cache_missing"] is True + assert info["missing_keys"] == 1 + assert stats["negative_hits"] == 1 + assert stats["hits"] == 1 + assert stats["misses"] == 1 # negative hit counts as neither hit nor miss + assert stats["hit_rate"] == 0.5 + + async def test_delete_does_not_record(self) -> None: + """Deleting a key does not create a negative entry (deletion != checked-absent).""" + cs = CacheStore(MemoryStore(), cache_store=MemoryStore(), cache_missing=True) + await cs.delete("c/0") + assert cs.cache_info()["missing_keys"] == 0 + + async def test_max_missing_keys_validated(self) -> None: + with pytest.raises(ValueError, match="max_missing_keys"): + CacheStore(MemoryStore(), cache_store=MemoryStore(), max_missing_keys=0) From 5ff0af0bdc1f2669fa72d078ed8707afd9d727b0 Mon Sep 17 00:00:00 2001 From: Shane Grigsby Date: Fri, 5 Jun 2026 11:32:59 -0700 Subject: [PATCH 02/15] matching current api for positive cached values --- changes/4040.feature.md | 2 +- src/zarr/experimental/cache_store.py | 50 +++++++++------------ tests/test_experimental/test_cache_store.py | 35 +++++---------- 3 files changed, 34 insertions(+), 53 deletions(-) diff --git a/changes/4040.feature.md b/changes/4040.feature.md index 4d9f2fb9ee..7e2813ed5d 100644 --- a/changes/4040.feature.md +++ b/changes/4040.feature.md @@ -1 +1 @@ -`zarr.experimental.cache_store.CacheStore` gained opt-in negative caching via `cache_missing=True`. When enabled, a full-key read that finds the key absent in the source store is remembered, so repeat reads of that absent key return immediately without a source round-trip — useful for sparse arrays where most chunks resolve to the fill value. Negative entries respect `max_age_seconds`, are bounded by `max_missing_keys` (default 100,000, least-recently-used eviction), and are evicted when the key is written via `set`/`set_if_not_exists`. The default is `False` (no behavior change); negative-cache activity is reported as `negative_hits` in `cache_stats()` and `missing_keys` in `cache_info()`. Only full-key reads are affected (not byte-range reads or `exists`). +`zarr.experimental.cache_store.CacheStore` now performs negative caching by default (`cache_missing=True`, opt-out). A full-key read that finds the key absent in the source store is remembered, so repeat reads of that absent key return immediately without a source round-trip — useful for sparse arrays where most chunks resolve to the fill value. Remembered misses respect `max_age_seconds` and are evicted when the key is written via `set`/`set_if_not_exists`. Negative-cache activity is reported as `negative_hits` in `cache_stats()` and `missing_keys` in `cache_info()`. Only full-key reads are affected (not byte-range reads or `exists`). Pass `cache_missing=False` to restore the previous behavior. Like the positive cache (unbounded when `max_size is None`), the negative cache is bounded only by `max_age_seconds`; set a finite TTL for scans over very large sparse key spaces. diff --git a/src/zarr/experimental/cache_store.py b/src/zarr/experimental/cache_store.py index 380b5e81d2..4205686fb6 100644 --- a/src/zarr/experimental/cache_store.py +++ b/src/zarr/experimental/cache_store.py @@ -33,11 +33,11 @@ class _CacheState: negative_hits: int = 0 key_insert_times: dict[_CacheEntryKey, float] = field(default_factory=dict) range_cache: dict[str, dict[ByteRequest, Buffer]] = field(default_factory=dict) - # Negative cache: full keys known to be absent in the source store, mapped to - # their (monotonic) insertion time for freshness. OrderedDict gives O(1) LRU - # eviction via popitem(last=False). Kept separate from the byte-size accounting - # above (negative entries carry no data) and bounded by ``max_missing_keys``. - missing_keys: OrderedDict[str, float] = field(default_factory=OrderedDict) + # Negative cache: full keys known to be absent in the source store, mapped to their + # (monotonic) insertion time. Used to short-circuit repeat reads of absent keys. + # Entries carry no data, so they are kept out of the byte-size accounting above; + # staleness is bounded by ``max_age_seconds``. + missing_keys: dict[str, float] = field(default_factory=dict) class CacheStore(WrapperStore[Store]): @@ -75,16 +75,19 @@ class CacheStore(WrapperStore[Store]): This benefits repeated reads of sparse arrays (most chunks absent). Negative entries respect ``max_age_seconds`` and are evicted when the key is written (``set``/``set_if_not_exists``). Only full-key reads are affected (not byte-range - reads or ``exists``). Default is False. + reads or ``exists``). Default is True. - Note: with ``max_age_seconds="infinity"`` a remembered miss never expires, so a - key written to the source by another process would stay invisible through this - cache. Pair ``cache_missing=True`` with a finite ``max_age_seconds`` if the source - may be written concurrently. - max_missing_keys : int, optional - Maximum number of negative (missing-key) entries to retain when - ``cache_missing`` is True. When exceeded, the least recently used missing keys - are evicted. Bounds memory for large sparse scans. Default is 100,000. + Notes: + + - With ``max_age_seconds="infinity"`` (the default) a remembered miss never + expires, so a key written to the source by another process stays invisible + through this cache. Pair ``cache_missing=True`` with a finite + ``max_age_seconds`` if the source may be written concurrently. + - Like the positive cache (which is unbounded when ``max_size is None``), the + negative cache is bounded only by ``max_age_seconds``. With an infinite TTL, + a scan over a very large sparse key space will accumulate one small entry per + absent key. Set a finite ``max_age_seconds`` (or ``cache_missing=False``) for + such workloads. Examples -------- @@ -115,7 +118,6 @@ class CacheStore(WrapperStore[Store]): max_size: int | None cache_set_data: bool cache_missing: bool - max_missing_keys: int _state: _CacheState def __init__( @@ -126,8 +128,7 @@ def __init__( max_age_seconds: int | str = "infinity", max_size: int | None = None, cache_set_data: bool = True, - cache_missing: bool = False, - max_missing_keys: int = 100_000, + cache_missing: bool = True, ) -> None: super().__init__(store) @@ -138,9 +139,6 @@ def __init__( ) raise ValueError(msg) - if max_missing_keys < 1: - raise ValueError("max_missing_keys must be a positive integer") - self._cache = cache_store # Validate and set max_age_seconds if isinstance(max_age_seconds, str): @@ -152,7 +150,6 @@ def __init__( self.max_size = max_size self.cache_set_data = cache_set_data self.cache_missing = cache_missing - self.max_missing_keys = max_missing_keys self._state = _CacheState() def _with_store(self, store: Store) -> Self: @@ -169,7 +166,6 @@ def with_read_only(self, read_only: bool = False) -> Self: max_size=self.max_size, cache_set_data=self.cache_set_data, cache_missing=self.cache_missing, - max_missing_keys=self.max_missing_keys, ) store._state = self._state return store @@ -196,15 +192,12 @@ def _is_missing_fresh(self, key: str) -> bool: return elapsed < self.max_age_seconds def _record_missing(self, key: str) -> None: - """Record *key* as known-missing, evicting the oldest entries past the cap. + """Record *key* as known-missing (absent in the source store). - Must be called while holding ``self._state.lock``. + Must be called while holding ``self._state.lock``. Staleness is bounded by + ``max_age_seconds`` via ``_is_missing_fresh``. """ self._state.missing_keys[key] = time.monotonic() - self._state.missing_keys.move_to_end(key) - while len(self._state.missing_keys) > self.max_missing_keys: - self._state.missing_keys.popitem(last=False) - self._state.evictions += 1 def _evict_missing(self, key: str) -> None: """Drop any negative entry for *key* (it is now present or being written). @@ -429,7 +422,6 @@ async def get( async with self._state.lock: if key in self._state.missing_keys and self._is_missing_fresh(key): self._state.negative_hits += 1 - self._state.missing_keys.move_to_end(key) return None entry_key: _CacheEntryKey = (key, byte_range) if byte_range is not None else key diff --git a/tests/test_experimental/test_cache_store.py b/tests/test_experimental/test_cache_store.py index 8181a3044a..17ee32c5c4 100644 --- a/tests/test_experimental/test_cache_store.py +++ b/tests/test_experimental/test_cache_store.py @@ -1078,10 +1078,20 @@ async def counting_get(*args: object, **kwargs: object) -> object: assert calls["n"] == after_first # no further source access assert cs.cache_stats()["negative_hits"] == 1 - async def test_disabled_by_default(self) -> None: - """With the default ``cache_missing=False`` nothing is remembered.""" + async def test_enabled_by_default(self) -> None: + """Negative caching is on by default (opt-out).""" cs = CacheStore(MemoryStore(), cache_store=MemoryStore()) proto = default_buffer_prototype() + assert cs.cache_missing is True + assert await cs.get("c/0", proto) is None + assert await cs.get("c/0", proto) is None + assert cs.cache_info()["missing_keys"] == 1 + assert cs.cache_stats()["negative_hits"] == 1 + + async def test_can_be_disabled(self) -> None: + """With ``cache_missing=False`` nothing is remembered.""" + cs = CacheStore(MemoryStore(), cache_store=MemoryStore(), cache_missing=False) + proto = default_buffer_prototype() assert await cs.get("c/0", proto) is None assert await cs.get("c/0", proto) is None assert cs.cache_info()["missing_keys"] == 0 @@ -1134,23 +1144,6 @@ async def test_respects_ttl(self) -> None: assert result.to_bytes() == b"late" assert cs.cache_info()["missing_keys"] == 0 - async def test_bounded(self) -> None: - """``max_missing_keys`` bounds the negative cache, evicting LRU entries.""" - cs = CacheStore( - MemoryStore(), cache_store=MemoryStore(), cache_missing=True, max_missing_keys=10 - ) - proto = default_buffer_prototype() - for i in range(25): - assert await cs.get(f"c/{i}", proto) is None - - assert cs.cache_info()["missing_keys"] == 10 - assert cs.cache_stats()["evictions"] >= 15 - # the 10 most-recently-seen keys are retained (LRU) - for i in range(15, 25): - assert f"c/{i}" in cs._state.missing_keys - for i in range(15): - assert f"c/{i}" not in cs._state.missing_keys - async def test_byte_range_unaffected(self) -> None: """Byte-range misses do not populate the negative cache.""" cs = CacheStore(MemoryStore(), cache_store=MemoryStore(), cache_missing=True) @@ -1184,7 +1177,3 @@ async def test_delete_does_not_record(self) -> None: cs = CacheStore(MemoryStore(), cache_store=MemoryStore(), cache_missing=True) await cs.delete("c/0") assert cs.cache_info()["missing_keys"] == 0 - - async def test_max_missing_keys_validated(self) -> None: - with pytest.raises(ValueError, match="max_missing_keys"): - CacheStore(MemoryStore(), cache_store=MemoryStore(), max_missing_keys=0) From 497d6981cb2413313a5f7c9e10e5509fb9997145 Mon Sep 17 00:00:00 2001 From: Shane Grigsby Date: Thu, 25 Jun 2026 16:04:26 -0700 Subject: [PATCH 03/15] unified slot-based cache for both positive and negative entries (shares cache budget) --- src/zarr/experimental/cache_store.py | 215 +++++++++++++------- tests/test_experimental/test_cache_store.py | 139 +++++++++---- 2 files changed, 236 insertions(+), 118 deletions(-) diff --git a/src/zarr/experimental/cache_store.py b/src/zarr/experimental/cache_store.py index 4205686fb6..3289f780e1 100644 --- a/src/zarr/experimental/cache_store.py +++ b/src/zarr/experimental/cache_store.py @@ -20,24 +20,52 @@ # live in the in-memory range cache. _CacheEntryKey = str | tuple[str, ByteRequest] +# Nominal byte cost charged to ``max_size`` for a negative (known-absent) entry. +# Such entries carry no data, but each one occupies an index slot (the key plus a +# small ``_Entry`` record), so it is charged a flat overhead. This lets a single +# ``max_size`` budget bound *total* cache memory — cached values and miss-markers +# together — rather than letting negative entries grow without limit. +_NEGATIVE_ENTRY_SIZE = 128 + + +@dataclass(slots=True) +class _Entry: + """A single cache slot, tracked in :attr:`_CacheState.entries`. + + ``present=True`` (the default): a value is cached for this key — in the + Store-backed cache for full keys, or the in-memory range cache for + byte-range keys — occupying ``size`` bytes. + + ``present=False``: the full key is known-*absent* in the source store (a + negative-cache entry). It carries no data, but is charged a flat + ``_NEGATIVE_ENTRY_SIZE`` against ``max_size`` for the index slot it occupies, + so cached values and miss-markers share one memory budget. Its staleness is + bounded by ``max_age_seconds``. + + Because every key maps to exactly one ``_Entry``, "present" and "absent" are + mutually exclusive by construction: a key cannot simultaneously be cached and + marked missing. + """ + + insert_time: float + size: int = 0 + present: bool = True + @dataclass(slots=True) class _CacheState: - cache_order: OrderedDict[_CacheEntryKey, None] = field(default_factory=OrderedDict) + # Single source of truth for every tracked key (full-key and byte-range, + # present and absent). Ordered for LRU eviction; ``move_to_end`` marks a key + # most-recently-used. Replaces the former parallel cache_order / key_sizes / + # key_insert_times / missing_keys structures so a key has one unambiguous state. + entries: OrderedDict[_CacheEntryKey, _Entry] = field(default_factory=OrderedDict) current_size: int = 0 - key_sizes: dict[_CacheEntryKey, int] = field(default_factory=dict) lock: asyncio.Lock = field(default_factory=asyncio.Lock) hits: int = 0 misses: int = 0 evictions: int = 0 negative_hits: int = 0 - key_insert_times: dict[_CacheEntryKey, float] = field(default_factory=dict) range_cache: dict[str, dict[ByteRequest, Buffer]] = field(default_factory=dict) - # Negative cache: full keys known to be absent in the source store, mapped to their - # (monotonic) insertion time. Used to short-circuit repeat reads of absent keys. - # Entries carry no data, so they are kept out of the byte-size accounting above; - # staleness is bounded by ``max_age_seconds``. - missing_keys: dict[str, float] = field(default_factory=dict) class CacheStore(WrapperStore[Store]): @@ -83,11 +111,13 @@ class CacheStore(WrapperStore[Store]): expires, so a key written to the source by another process stays invisible through this cache. Pair ``cache_missing=True`` with a finite ``max_age_seconds`` if the source may be written concurrently. - - Like the positive cache (which is unbounded when ``max_size is None``), the - negative cache is bounded only by ``max_age_seconds``. With an infinite TTL, - a scan over a very large sparse key space will accumulate one small entry per - absent key. Set a finite ``max_age_seconds`` (or ``cache_missing=False``) for - such workloads. + - Negative entries share the ``max_size`` budget with cached values: each is + charged a small flat overhead, and under memory pressure miss-markers are + evicted (least-recently-used first) before any cached value. A single + ``max_size`` therefore bounds *total* cache memory. When ``max_size is None`` + both caches are unbounded, so a scan over a very large sparse key space will + accumulate one small entry per absent key; set ``max_size`` (and/or a finite + ``max_age_seconds``, or ``cache_missing=False``) for such workloads. Examples -------- @@ -170,56 +200,84 @@ def with_read_only(self, read_only: bool = False) -> Self: store._state = self._state return store - def _is_key_fresh(self, entry_key: _CacheEntryKey) -> bool: - """Check if a cached entry is still fresh based on max_age_seconds. + def _is_fresh(self, entry_key: _CacheEntryKey) -> bool: + """Check if a tracked entry (present or absent) is still fresh. - Uses monotonic time for accurate elapsed time measurement. + Uses monotonic time for accurate elapsed time measurement. A key with no + entry is treated as not fresh (except under an infinite TTL, matching the + previous behaviour of routing unseen keys through the cache path). """ if self.max_age_seconds == "infinity": return True - now = time.monotonic() - elapsed = now - self._state.key_insert_times.get(entry_key, 0) - return elapsed < self.max_age_seconds - - def _is_missing_fresh(self, key: str) -> bool: - """Check if a negative (missing-key) entry is still fresh. - - Mirrors ``_is_key_fresh`` but reads the negative-cache insertion time. - """ - if self.max_age_seconds == "infinity": - return True - elapsed = time.monotonic() - self._state.missing_keys.get(key, 0.0) + entry = self._state.entries.get(entry_key) + if entry is None: + return False + elapsed = time.monotonic() - entry.insert_time return elapsed < self.max_age_seconds - def _record_missing(self, key: str) -> None: + async def _record_missing(self, key: str) -> None: """Record *key* as known-missing (absent in the source store). - Must be called while holding ``self._state.lock``. Staleness is bounded by - ``max_age_seconds`` via ``_is_missing_fresh``. + Overwrites any existing slot for *key*, so a key cannot be both cached and + marked missing. The marker is charged ``_NEGATIVE_ENTRY_SIZE`` against the + shared ``max_size`` budget, then the budget is re-enforced (evicting + absent entries first). Must be called while holding ``self._state.lock``. + Staleness is bounded by ``max_age_seconds`` via ``_is_fresh``. """ - self._state.missing_keys[key] = time.monotonic() + old = self._state.entries.get(key) + if old is not None: + self._state.current_size = max(0, self._state.current_size - old.size) + self._state.entries[key] = _Entry( + insert_time=time.monotonic(), size=_NEGATIVE_ENTRY_SIZE, present=False + ) + self._state.entries.move_to_end(key) + self._state.current_size += _NEGATIVE_ENTRY_SIZE + # Re-enforce the shared budget (no further incoming bytes to reserve). + await self._accommodate_value(0) def _evict_missing(self, key: str) -> None: """Drop any negative entry for *key* (it is now present or being written). - Must be called while holding ``self._state.lock``. + Only removes an *absent* slot — a present (cached) value for the same key is + left untouched. Must be called while holding ``self._state.lock``. """ - self._state.missing_keys.pop(key, None) + entry = self._state.entries.get(key) + if entry is not None and not entry.present: + del self._state.entries[key] async def _accommodate_value(self, value_size: int) -> None: - """Ensure there is enough space in the cache for a new value. + """Evict until ``value_size`` more bytes fit within ``max_size``. - Must be called while holding self._state.lock. + Eviction is *absent-first*: least-recently-used negative markers are + dropped before any cached value, because miss-markers are cheap to + regenerate (just re-probe the source) while cached data is not. A cached + value is only evicted once no negative markers remain. Must be called + while holding self._state.lock. """ if self.max_size is None: return - # Remove least recently used items until we have enough space - while self._state.current_size + value_size > self.max_size and self._state.cache_order: - # Get the least recently used key (first in OrderedDict) - lru_key = next(iter(self._state.cache_order)) + while self._state.current_size + value_size > self.max_size: + lru_key = self._next_eviction_candidate() + if lru_key is None: + break await self._evict_key(lru_key) + def _next_eviction_candidate(self) -> _CacheEntryKey | None: + """Return the next entry to evict, preferring absent markers (LRU-first). + + Walks entries in LRU order: the first absent entry found is returned; if + none are absent, the least-recently-used present entry is returned. Must + be called while holding self._state.lock. + """ + lru_present: _CacheEntryKey | None = None + for entry_key, entry in self._state.entries.items(): + if not entry.present: + return entry_key + if lru_present is None: + lru_present = entry_key + return lru_present + async def _evict_key(self, entry_key: _CacheEntryKey) -> None: """Evict a cache entry. @@ -229,10 +287,13 @@ async def _evict_key(self, entry_key: _CacheEntryKey) -> None: For ``(str, ByteRequest)`` keys the entry is removed from the in-memory range cache. """ - key_size = self._state.key_sizes.get(entry_key, 0) + entry = self._state.entries.pop(entry_key, None) + key_size = entry.size if entry is not None else 0 if isinstance(entry_key, str): - await self._cache.delete(entry_key) + # Absent markers store no value in the backing cache — skip the delete. + if entry is None or entry.present: + await self._cache.delete(entry_key) else: base_key, byte_range = entry_key per_key = self._state.range_cache.get(base_key) @@ -241,9 +302,6 @@ async def _evict_key(self, entry_key: _CacheEntryKey) -> None: if not per_key: del self._state.range_cache[base_key] - self._state.cache_order.pop(entry_key, None) - self._state.key_insert_times.pop(entry_key, None) - self._state.key_sizes.pop(entry_key, None) self._state.current_size = max(0, self._state.current_size - key_size) self._state.evictions += 1 @@ -263,36 +321,38 @@ async def _track_entry(self, entry_key: _CacheEntryKey, value: Buffer) -> bool: return False async with self._state.lock: - # If key already exists, subtract old size first - if entry_key in self._state.key_sizes: - old_size = self._state.key_sizes[entry_key] - self._state.current_size -= old_size + # If key already exists, subtract old size first (an absent slot has + # size 0, so this also cleanly upgrades a negative entry to present). + old = self._state.entries.get(entry_key) + if old is not None: + self._state.current_size -= old.size # Make room for the new value await self._accommodate_value(value_size) - # Update tracking atomically - self._state.cache_order[entry_key] = None + # Update tracking atomically. Assigning to an existing key preserves + # its LRU position, matching the previous behaviour. + self._state.entries[entry_key] = _Entry( + insert_time=time.monotonic(), size=value_size, present=True + ) self._state.current_size += value_size - self._state.key_sizes[entry_key] = value_size - self._state.key_insert_times[entry_key] = time.monotonic() return True async def _update_access_order(self, entry_key: _CacheEntryKey) -> None: """Update the access order for LRU tracking.""" - if entry_key in self._state.cache_order: + if entry_key in self._state.entries: async with self._state.lock: - self._state.cache_order.move_to_end(entry_key) + self._state.entries.move_to_end(entry_key) def _remove_from_tracking(self, entry_key: _CacheEntryKey) -> None: - """Remove an entry from all tracking structures. + """Remove an entry from tracking, reclaiming any bytes it accounted for. Must be called while holding self._state.lock. """ - self._state.cache_order.pop(entry_key, None) - self._state.key_insert_times.pop(entry_key, None) - self._state.key_sizes.pop(entry_key, None) + entry = self._state.entries.pop(entry_key, None) + if entry is not None: + self._state.current_size = max(0, self._state.current_size - entry.size) def _invalidate_range_entries(self, key: str) -> None: """Remove all byte-range entries for *key* from the range cache and tracking. @@ -303,10 +363,9 @@ def _invalidate_range_entries(self, key: str) -> None: if per_key is not None: for byte_range in per_key: entry_key: _CacheEntryKey = (key, byte_range) - entry_size = self._state.key_sizes.pop(entry_key, 0) - self._state.cache_order.pop(entry_key, None) - self._state.key_insert_times.pop(entry_key, None) - self._state.current_size = max(0, self._state.current_size - entry_size) + entry = self._state.entries.pop(entry_key, None) + if entry is not None: + self._state.current_size = max(0, self._state.current_size - entry.size) # ------------------------------------------------------------------ # get helpers @@ -324,7 +383,7 @@ async def _cache_miss( # The key is absent in the source: remember the miss so a repeat # read can short-circuit without a source round-trip. if self.cache_missing: - self._record_missing(key) + await self._record_missing(key) else: entry_key: _CacheEntryKey = (key, byte_range) async with self._state.lock: @@ -337,11 +396,10 @@ async def _cache_miss( else: if byte_range is None: await self._cache.set(key, result) + # ``_track_entry`` overwrites the key's single slot with a present + # entry, so any prior negative marker is structurally replaced — + # no separate negative-cache eviction is needed here. await self._track_entry(key, result) - # A value now exists for this key: drop any stale negative entry. - if self.cache_missing: - async with self._state.lock: - self._evict_missing(key) else: entry_key = (key, byte_range) self._state.range_cache.setdefault(key, {})[byte_range] = result @@ -420,12 +478,13 @@ async def get( # key has no positive entry and would otherwise be routed straight to the source. if self.cache_missing and byte_range is None: async with self._state.lock: - if key in self._state.missing_keys and self._is_missing_fresh(key): + entry = self._state.entries.get(key) + if entry is not None and not entry.present and self._is_fresh(key): self._state.negative_hits += 1 return None entry_key: _CacheEntryKey = (key, byte_range) if byte_range is not None else key - if not self._is_key_fresh(entry_key): + if not self._is_fresh(entry_key): return await self._get_no_cache(key, prototype, byte_range) else: return await self._get_try_cache(key, prototype, byte_range) @@ -495,6 +554,8 @@ async def delete(self, key: str) -> None: def cache_info(self) -> dict[str, Any]: """Return information about the cache state.""" + present = sum(1 for entry in self._state.entries.values() if entry.present) + missing = len(self._state.entries) - present return { "cache_store_type": type(self._cache).__name__, "max_age_seconds": "infinity" @@ -504,9 +565,9 @@ def cache_info(self) -> dict[str, Any]: "current_size": self._state.current_size, "cache_set_data": self.cache_set_data, "cache_missing": self.cache_missing, - "tracked_keys": len(self._state.key_insert_times), - "cached_keys": len(self._state.cache_order), - "missing_keys": len(self._state.missing_keys), + "tracked_keys": len(self._state.entries), + "cached_keys": present, + "missing_keys": missing, } def cache_stats(self) -> dict[str, Any]: @@ -535,16 +596,14 @@ async def clear_cache(self) -> None: # Reset tracking async with self._state.lock: - self._state.key_insert_times.clear() - self._state.cache_order.clear() - self._state.key_sizes.clear() + self._state.entries.clear() self._state.range_cache.clear() - self._state.missing_keys.clear() self._state.current_size = 0 self._state.negative_hits = 0 def __repr__(self) -> str: """Return string representation of the cache store.""" + cached_keys = sum(1 for entry in self._state.entries.values() if entry.present) return ( f"{self.__class__.__name__}(" f"store={self._store!r}, " @@ -552,5 +611,5 @@ def __repr__(self) -> str: f"max_age_seconds={self.max_age_seconds}, " f"max_size={self.max_size}, " f"current_size={self._state.current_size}, " - f"cached_keys={len(self._state.cache_order)})" + f"cached_keys={cached_keys})" ) diff --git a/tests/test_experimental/test_cache_store.py b/tests/test_experimental/test_cache_store.py index 8c5fe2c281..c8b12536be 100644 --- a/tests/test_experimental/test_cache_store.py +++ b/tests/test_experimental/test_cache_store.py @@ -10,7 +10,7 @@ from zarr.abc.store import RangeByteRequest, Store, SuffixByteRequest from zarr.core.buffer.core import default_buffer_prototype from zarr.core.buffer.cpu import Buffer as CPUBuffer -from zarr.experimental.cache_store import CacheStore +from zarr.experimental.cache_store import CacheStore, _Entry from zarr.storage import MemoryStore @@ -62,7 +62,7 @@ async def test_with_read_only_round_trip(self) -> None: # Cache configuration and state are shared assert writer._cache is cached_ro._cache assert writer._state is cached_ro._state - assert writer._state.key_insert_times is cached_ro._state.key_insert_times + assert writer._state.entries is cached_ro._state.entries # Writes via the writable cache store succeed and are cached await writer.set("foo", buf) @@ -132,13 +132,13 @@ async def test_cache_expiration(self) -> None: await cached_store.set("expire_key", test_data) # Should be fresh initially - assert cached_store._is_key_fresh("expire_key") + assert cached_store._is_fresh("expire_key") # Wait for expiration await asyncio.sleep(1.1) # Should now be stale - assert not cached_store._is_key_fresh("expire_key") + assert not cached_store._is_fresh("expire_key") async def test_cache_set_data_false(self, source_store: Store, cache_store: Store) -> None: """Test behavior when cache_set_data=False.""" @@ -222,11 +222,11 @@ async def test_infinity_max_age(self, cached_store: CacheStore) -> None: await cached_store.set("eternal_key", test_data) # Should always be fresh - assert cached_store._is_key_fresh("eternal_key") + assert cached_store._is_fresh("eternal_key") # Even after time passes await asyncio.sleep(0.1) - assert cached_store._is_key_fresh("eternal_key") + assert cached_store._is_fresh("eternal_key") async def test_cache_returns_cached_data_for_performance( self, cached_store: CacheStore, source_store: Store @@ -235,7 +235,9 @@ async def test_cache_returns_cached_data_for_performance( # Put data in cache but not source (simulates orphaned cache entry) test_data = CPUBuffer.from_bytes(b"orphaned data") await cached_store._cache.set("orphan_key", test_data) - cached_store._state.key_insert_times["orphan_key"] = time.monotonic() + cached_store._state.entries["orphan_key"] = _Entry( + insert_time=time.monotonic(), size=len(test_data), present=True + ) # Cache should return data for performance (no source verification) result = await cached_store.get("orphan_key", default_buffer_prototype()) @@ -244,7 +246,7 @@ async def test_cache_returns_cached_data_for_performance( # Cache entry should remain (performance optimization) assert await cached_store._cache.exists("orphan_key") - assert "orphan_key" in cached_store._state.key_insert_times + assert "orphan_key" in cached_store._state.entries async def test_cache_coherency_through_expiration(self) -> None: """Test that cache coherency is managed through cache expiration, not source verification.""" @@ -380,7 +382,7 @@ async def test_max_age_infinity(self) -> None: await cached_store.set("test_key", test_data) # Even after time passes, key should be fresh - assert cached_store._is_key_fresh("test_key") + assert cached_store._is_fresh("test_key") async def test_max_age_numeric(self) -> None: """Test cache with numeric max age.""" @@ -397,13 +399,13 @@ async def test_max_age_numeric(self) -> None: await cached_store.set("test_key", test_data) # Key should be fresh initially - assert cached_store._is_key_fresh("test_key") + assert cached_store._is_fresh("test_key") # Manually set old timestamp to test expiration - cached_store._state.key_insert_times["test_key"] = time.monotonic() - 2 # 2 seconds ago + cached_store._state.entries["test_key"].insert_time = time.monotonic() - 2 # 2 seconds ago # Key should now be stale - assert not cached_store._is_key_fresh("test_key") + assert not cached_store._is_fresh("test_key") async def test_cache_set_data_disabled(self) -> None: """Test cache behavior when cache_set_data is False.""" @@ -553,8 +555,8 @@ async def test_evict_key_exception_handling(self) -> None: await cached_store.set("test_key", test_data) # Manually corrupt the tracking to trigger exception - # Remove from one structure but not others to create inconsistency - del cached_store._state.cache_order["test_key"] + # Remove the tracked entry while leaving the cached value behind + del cached_store._state.entries["test_key"] # Try to evict - should handle the KeyError gracefully await cached_store._evict_key("test_key") @@ -575,16 +577,16 @@ async def test_get_no_cache_delete_tracking(self) -> None: await cached_store._track_entry("phantom_key", test_data) # Verify it's in tracking - assert "phantom_key" in cached_store._state.cache_order - assert "phantom_key" in cached_store._state.key_insert_times + assert "phantom_key" in cached_store._state.entries # Now try to get it - since it's not in source, should clean up tracking result = await cached_store._get_no_cache("phantom_key", default_buffer_prototype()) assert result is None - # Should have cleaned up tracking - assert "phantom_key" not in cached_store._state.cache_order - assert "phantom_key" not in cached_store._state.key_insert_times + # Should have cleaned up tracking (the positive entry is gone). With + # cache_missing on by default, a negative marker replaces it. + entry = cached_store._state.entries.get("phantom_key") + assert entry is None or not entry.present async def test_accommodate_value_no_max_size(self) -> None: """Test _accommodate_value early return when max_size is None.""" @@ -644,9 +646,7 @@ async def set_large(key: str) -> None: # Size should be consistent with tracked keys assert info["current_size"] <= 200 # Might pass # But verify actual cache store size matches tracking - total_size = sum( - cached_store._state.key_sizes.get(k, 0) for k in cached_store._state.cache_order - ) + total_size = sum(entry.size for entry in cached_store._state.entries.values()) assert total_size == info["current_size"] # WOULD FAIL async def test_concurrent_get_and_evict(self) -> None: @@ -675,7 +675,10 @@ async def write_key() -> None: # Verify consistency info = cached_store.cache_info() assert info["current_size"] <= 100 - assert len(cached_store._state.cache_order) == len(cached_store._state.key_sizes) + # Tracked size accounting stays consistent with all entries (present + # values plus any negative markers, which each carry a flat overhead). + total_size = sum(entry.size for entry in cached_store._state.entries.values()) + assert total_size == info["current_size"] async def test_eviction_actually_deletes_from_cache_store(self) -> None: """Test that eviction removes keys from cache_store, not just tracking.""" @@ -696,8 +699,7 @@ async def test_eviction_actually_deletes_from_cache_store(self) -> None: await cached_store.set("key2", data2) # Check tracking - key1 should be removed - assert "key1" not in cached_store._state.cache_order - assert "key1" not in cached_store._state.key_sizes + assert "key1" not in cached_store._state.entries # CRITICAL: key1 should also be removed from cache_store assert not await cache_store.exists("key1"), ( @@ -769,21 +771,15 @@ async def test_all_tracked_keys_exist_in_cache_store(self) -> None: data = CPUBuffer.from_bytes(b"x" * 50) await cached_store.set(f"key_{i}", data) - # Every str key in tracking should exist in cache_store - # (tuple keys are byte-range entries stored in-memory, not in the Store) - for entry_key in cached_store._state.cache_order: - if isinstance(entry_key, str): + # Every present str key in tracking should exist in cache_store. + # (tuple keys are byte-range entries stored in-memory, not in the Store; + # absent entries are negative markers with no stored value.) + for entry_key, entry in cached_store._state.entries.items(): + if isinstance(entry_key, str) and entry.present: assert await cache_store.exists(entry_key), ( f"Key '{entry_key}' is tracked but doesn't exist in cache_store" ) - # Every str key in _key_sizes should exist in cache_store - for entry_key in cached_store._state.key_sizes: - if isinstance(entry_key, str): - assert await cache_store.exists(entry_key), ( - f"Key '{entry_key}' has size tracked but doesn't exist in cache_store" - ) - # Additional coverage tests for 100% coverage async def test_cache_store_requires_delete_support(self) -> None: @@ -999,13 +995,13 @@ async def test_set_invalidates_cached_byte_ranges(self) -> None: assert r1.to_bytes() == b"old" # Byte-range entry should be in range_cache - assert ("key", RangeByteRequest(0, 3)) in cached_store._state.cache_order + assert ("key", RangeByteRequest(0, 3)) in cached_store._state.entries # Overwrite via set() — range entries must be invalidated await cached_store.set("key", CPUBuffer.from_bytes(b"NEW DATA!!")) # The old range entry should be gone from tracking and range_cache - assert ("key", RangeByteRequest(0, 3)) not in cached_store._state.cache_order + assert ("key", RangeByteRequest(0, 3)) not in cached_store._state.entries assert "key" not in cached_store._state.range_cache # A fresh byte-range read should return the new data @@ -1027,12 +1023,12 @@ async def test_delete_invalidates_cached_byte_ranges(self) -> None: assert r is not None assert r.to_bytes() == b"hello" - assert ("key", RangeByteRequest(0, 5)) in cached_store._state.cache_order + assert ("key", RangeByteRequest(0, 5)) in cached_store._state.entries # Delete the key — range entries must be cleaned up await cached_store.delete("key") - assert ("key", RangeByteRequest(0, 5)) not in cached_store._state.cache_order + assert ("key", RangeByteRequest(0, 5)) not in cached_store._state.entries assert "key" not in cached_store._state.range_cache # Key is gone from source @@ -1166,3 +1162,66 @@ async def test_delete_does_not_record(self) -> None: cs = CacheStore(MemoryStore(), cache_store=MemoryStore(), cache_missing=True) await cs.delete("c/0") assert cs.cache_info()["missing_keys"] == 0 + + async def test_negative_entry_counts_against_max_size(self) -> None: + """A negative marker is charged against the shared ``max_size`` budget.""" + from zarr.experimental.cache_store import _NEGATIVE_ENTRY_SIZE + + cs = CacheStore( + MemoryStore(), cache_store=MemoryStore(), cache_missing=True, max_size=10_000 + ) + proto = default_buffer_prototype() + assert cs.cache_info()["current_size"] == 0 + assert await cs.get("absent", proto) is None + assert cs.cache_info()["current_size"] == _NEGATIVE_ENTRY_SIZE + + async def test_shared_budget_bounds_negative_entries(self) -> None: + """Many misses cannot grow the cache past ``max_size`` — old negative + markers are evicted (LRU) to stay within the shared budget.""" + from zarr.experimental.cache_store import _NEGATIVE_ENTRY_SIZE + + cap = 5 + cs = CacheStore( + MemoryStore(), + cache_store=MemoryStore(), + cache_missing=True, + max_size=cap * _NEGATIVE_ENTRY_SIZE, + ) + proto = default_buffer_prototype() + for i in range(25): + assert await cs.get(f"absent/{i}", proto) is None + + info = cs.cache_info() + assert info["missing_keys"] == cap + assert info["current_size"] <= cap * _NEGATIVE_ENTRY_SIZE + # The most-recent misses are retained (LRU eviction of the oldest). + assert await cs.get("absent/24", proto) is None + assert cs.cache_stats()["negative_hits"] >= 1 + + async def test_absent_evicted_before_present(self) -> None: + """Under memory pressure, miss-markers are evicted before cached values.""" + from zarr.experimental.cache_store import _NEGATIVE_ENTRY_SIZE + + source = MemoryStore() + # Budget for one value plus a couple of negative markers. + value = CPUBuffer.from_bytes(b"v" * 64) + cs = CacheStore( + source, + cache_store=MemoryStore(), + cache_missing=True, + max_size=len(value) + 2 * _NEGATIVE_ENTRY_SIZE, + ) + proto = default_buffer_prototype() + + # Cache a present value, then record several misses that exceed the budget. + await source.set("present", value) + assert (await cs.get("present", proto)) is not None + for i in range(5): + assert await cs.get(f"absent/{i}", proto) is None + + # The present value survives; negative markers were evicted to make room. + info = cs.cache_info() + assert info["cached_keys"] == 1 + assert "present" in cs._state.entries + assert cs._state.entries["present"].present + assert info["current_size"] <= cs.max_size From 7829bfbd2d0fd3ad4a571807e20aeee82925ace5 Mon Sep 17 00:00:00 2001 From: Shane Grigsby Date: Thu, 25 Jun 2026 16:27:24 -0700 Subject: [PATCH 04/15] fixing eviction bug where we weren't reclaiming bytes on evicted negative entries --- src/zarr/experimental/cache_store.py | 45 ++++++++++++++++----- tests/test_experimental/test_cache_store.py | 44 +++++++++++++++++++- 2 files changed, 77 insertions(+), 12 deletions(-) diff --git a/src/zarr/experimental/cache_store.py b/src/zarr/experimental/cache_store.py index 3289f780e1..f758c0ec89 100644 --- a/src/zarr/experimental/cache_store.py +++ b/src/zarr/experimental/cache_store.py @@ -118,6 +118,11 @@ class CacheStore(WrapperStore[Store]): both caches are unbounded, so a scan over a very large sparse key space will accumulate one small entry per absent key; set ``max_size`` (and/or a finite ``max_age_seconds``, or ``cache_missing=False``) for such workloads. + - This is store-level, per-key negative caching aimed at the stock ``arr[:]`` + path, which probes every chunk. For very large sparse arrays, prefer the + array-level sparse-read primitives ``zarr.shards_initialized`` and + ``zarr.read_regions`` (PR #4028), which touch only populated chunks and so + never issue the empty-chunk reads this cache would otherwise remember. Examples -------- @@ -218,31 +223,47 @@ def _is_fresh(self, entry_key: _CacheEntryKey) -> bool: async def _record_missing(self, key: str) -> None: """Record *key* as known-missing (absent in the source store). - Overwrites any existing slot for *key*, so a key cannot be both cached and - marked missing. The marker is charged ``_NEGATIVE_ENTRY_SIZE`` against the - shared ``max_size`` budget, then the budget is re-enforced (evicting - absent entries first). Must be called while holding ``self._state.lock``. - Staleness is bounded by ``max_age_seconds`` via ``_is_fresh``. + Charges a flat ``_NEGATIVE_ENTRY_SIZE`` against the shared ``max_size`` + budget. A negative marker is strictly lower priority than cached data: it + may only displace *other* (older) absent markers to fit, never a cached + value, and is skipped entirely if the budget is full of cached values. + + The caller (``_cache_miss``) has already removed any backing-store value and + tracking slot for *key*, so this records a fresh marker. Must be called + while holding ``self._state.lock``. Staleness is bounded by + ``max_age_seconds`` via ``_is_fresh``. """ - old = self._state.entries.get(key) + # Drop any pre-existing slot for this key, reclaiming its bytes. + old = self._state.entries.pop(key, None) if old is not None: self._state.current_size = max(0, self._state.current_size - old.size) + + # Make room by evicting older absent markers only — never cached values. + if self.max_size is not None: + while self._state.current_size + _NEGATIVE_ENTRY_SIZE > self.max_size: + lru_absent = next( + (k for k, e in self._state.entries.items() if not e.present), None + ) + if lru_absent is None: + return # only cached values fill the budget — don't record the miss + await self._evict_key(lru_absent) + self._state.entries[key] = _Entry( insert_time=time.monotonic(), size=_NEGATIVE_ENTRY_SIZE, present=False ) self._state.entries.move_to_end(key) self._state.current_size += _NEGATIVE_ENTRY_SIZE - # Re-enforce the shared budget (no further incoming bytes to reserve). - await self._accommodate_value(0) def _evict_missing(self, key: str) -> None: """Drop any negative entry for *key* (it is now present or being written). Only removes an *absent* slot — a present (cached) value for the same key is - left untouched. Must be called while holding ``self._state.lock``. + left untouched — and reclaims the marker's charged bytes. Must be called + while holding ``self._state.lock``. """ entry = self._state.entries.get(key) if entry is not None and not entry.present: + self._state.current_size = max(0, self._state.current_size - entry.size) del self._state.entries[key] async def _accommodate_value(self, value_size: int) -> None: @@ -341,8 +362,10 @@ async def _track_entry(self, entry_key: _CacheEntryKey, value: Buffer) -> bool: async def _update_access_order(self, entry_key: _CacheEntryKey) -> None: """Update the access order for LRU tracking.""" - if entry_key in self._state.entries: - async with self._state.lock: + async with self._state.lock: + # Re-check membership under the lock: the entry may have been evicted + # by a concurrent operation between the call and acquiring the lock. + if entry_key in self._state.entries: self._state.entries.move_to_end(entry_key) def _remove_from_tracking(self, entry_key: _CacheEntryKey) -> None: diff --git a/tests/test_experimental/test_cache_store.py b/tests/test_experimental/test_cache_store.py index c8b12536be..353e73af82 100644 --- a/tests/test_experimental/test_cache_store.py +++ b/tests/test_experimental/test_cache_store.py @@ -1219,9 +1219,51 @@ async def test_absent_evicted_before_present(self) -> None: for i in range(5): assert await cs.get(f"absent/{i}", proto) is None - # The present value survives; negative markers were evicted to make room. + # The present value survives; markers are bounded and never evict it. info = cs.cache_info() assert info["cached_keys"] == 1 assert "present" in cs._state.entries assert cs._state.entries["present"].present + # Markers fill only the room left over by the cached value (2 here), proving + # both that misses were actually recorded and that they were bounded. + assert info["missing_keys"] == 2 assert info["current_size"] <= cs.max_size + + async def test_no_size_leak_on_miss_then_write(self) -> None: + """Recording a miss then writing the key must not leak the marker's charge + against ``current_size`` (regression for negative-entry accounting).""" + from zarr.experimental.cache_store import _NEGATIVE_ENTRY_SIZE + + source = MemoryStore() + value = CPUBuffer.from_bytes(b"v" * 50) + cs = CacheStore(source, cache_store=MemoryStore(), cache_missing=True, max_size=10_000) + proto = default_buffer_prototype() + + # Miss → marker charged; then write the same key → marker must be reclaimed. + assert await cs.get("k", proto) is None + assert cs.cache_info()["current_size"] == _NEGATIVE_ENTRY_SIZE + await cs.set("k", value) + + info = cs.cache_info() + assert info["missing_keys"] == 0 + assert info["cached_keys"] == 1 + # Only the value's bytes remain — no leftover marker overhead. + assert info["current_size"] == len(value) + # And the invariant holds: current_size == sum of all tracked entry sizes. + total = sum(entry.size for entry in cs._state.entries.values()) + assert total == info["current_size"] + + async def test_no_size_leak_on_miss_then_set_if_not_exists(self) -> None: + """``set_if_not_exists`` after a miss reclaims the marker's charge too.""" + source = MemoryStore() + value = CPUBuffer.from_bytes(b"v" * 50) + cs = CacheStore(source, cache_store=MemoryStore(), cache_missing=True, max_size=10_000) + proto = default_buffer_prototype() + + assert await cs.get("k", proto) is None + await cs.set_if_not_exists("k", value) + + info = cs.cache_info() + assert info["missing_keys"] == 0 + total = sum(entry.size for entry in cs._state.entries.values()) + assert total == info["current_size"] From 2ca2376d1810458aa0c2d3dd02b39f89b671002d Mon Sep 17 00:00:00 2001 From: Shane Grigsby Date: Thu, 25 Jun 2026 16:43:02 -0700 Subject: [PATCH 05/15] updating narrative user docs --- docs/user-guide/experimental.md | 56 +++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/docs/user-guide/experimental.md b/docs/user-guide/experimental.md index 1c6d952c7c..7f9beea381 100644 --- a/docs/user-guide/experimental.md +++ b/docs/user-guide/experimental.md @@ -137,6 +137,51 @@ cache = CacheStore( ) ``` +**cache_missing**: Controls *negative caching* — remembering keys that are absent in the +source store (on by default). Without it, the positive cache cannot help with an absent +key: there is no value to store, so every read re-pays a source round-trip. This is the +dominant cost when reading sparse arrays (mostly empty chunks) repeatedly through a cache. +With `cache_missing=True`, a full-key read that finds the key absent records that absence, +so subsequent reads of the same key return immediately without consulting the source. The +remembered miss is evicted when the key is written and respects `max_age_seconds`. + +```python exec="true" session="experimental" source="above" +import asyncio +from zarr.storage import MemoryStore +from zarr.core.buffer import default_buffer_prototype + +neg_cache = CacheStore( + store=MemoryStore(), + cache_store=MemoryStore(), + cache_missing=True, # default; pass False to disable + max_age_seconds=300, # recommended: bound staleness of remembered misses +) + +async def read_absent_twice(): + proto = default_buffer_prototype() + await neg_cache.get("c/0", proto) # first read: real miss, consults the source + await neg_cache.get("c/0", proto) # second read: served from the negative cache + +asyncio.run(read_absent_twice()) + +info = neg_cache.cache_info() +print(info['cache_missing']) # True +print(info['missing_keys']) # 1 — one remembered absent key +print(neg_cache.cache_stats()['negative_hits']) # 1 — one read served without a source round-trip +``` + +Negative markers share the `max_size` budget with cached values: each is charged a small +flat overhead, and under memory pressure markers are evicted (least-recently-used first) +before any cached value, so a flood of empty-chunk reads can never evict real cached data. +Only full-key reads are affected — byte-range reads and `exists()` are unchanged. + +> **Note:** With the default `max_age_seconds="infinity"`, a remembered miss never expires, +> so a key written to the source by another process stays invisible through the cache until +> it is written through the cache. Pair `cache_missing=True` with a finite `max_age_seconds` +> when the source may be written concurrently. For very large sparse arrays, prefer the +> array-level sparse-read primitives `zarr.shards_initialized` / `zarr.read_regions`, which +> read only populated chunks and avoid the empty-chunk reads entirely. + ## Cache Statistics The CacheStore provides statistics to monitor cache performance and state: @@ -155,9 +200,19 @@ print(info['current_size']) print(info['tracked_keys']) print(info['cached_keys']) print(info['cache_set_data']) +print(info['cache_missing']) # negative caching enabled? +print(info['missing_keys']) # number of remembered absent keys + +# cache_stats() reports hit/miss counts and negative-cache activity +stats = cached_store.cache_stats() +print(stats['hits']) +print(stats['misses']) +print(stats['negative_hits']) # absent-key reads served without a source round-trip ``` The `cache_info()` method returns a dictionary with detailed information about the cache state. +A negative hit (an absent key served from the negative cache) is reported separately as +`negative_hits` and counts as neither a hit nor a miss, so it does not affect `hit_rate`. ## Cache Management @@ -185,6 +240,7 @@ The `clear_cache()` method is an async method that clears both the cache store 4. **Monitor cache statistics**: Use `cache_info()` to tune cache size and access patterns 5. **Consider data locality**: Group related data accesses together to improve cache efficiency 6. **Set appropriate expiration**: Use `max_age_seconds` for time-sensitive data or "infinity" for static data +7. **Negative caching for sparse data**: Leave `cache_missing` on (the default) to skip repeated source round-trips for absent keys; pair it with a finite `max_age_seconds` if the source may be written by another process ## Working with Different Store Types From bb0a346eea74e0b563b464e60d3fd7740f33d94c Mon Sep 17 00:00:00 2001 From: Shane Grigsby Date: Thu, 25 Jun 2026 17:05:30 -0700 Subject: [PATCH 06/15] minor bug fix --- src/zarr/experimental/cache_store.py | 55 ++++---- tests/test_experimental/test_cache_store.py | 131 ++++++++++++++++++++ 2 files changed, 164 insertions(+), 22 deletions(-) diff --git a/src/zarr/experimental/cache_store.py b/src/zarr/experimental/cache_store.py index f758c0ec89..9dd224ed67 100644 --- a/src/zarr/experimental/cache_store.py +++ b/src/zarr/experimental/cache_store.py @@ -248,10 +248,10 @@ async def _record_missing(self, key: str) -> None: return # only cached values fill the budget — don't record the miss await self._evict_key(lru_absent) + # The key was popped above, so this assignment appends it as most-recent. self._state.entries[key] = _Entry( insert_time=time.monotonic(), size=_NEGATIVE_ENTRY_SIZE, present=False ) - self._state.entries.move_to_end(key) self._state.current_size += _NEGATIVE_ENTRY_SIZE def _evict_missing(self, key: str) -> None: @@ -281,7 +281,9 @@ async def _accommodate_value(self, value_size: int) -> None: while self._state.current_size + value_size > self.max_size: lru_key = self._next_eviction_candidate() if lru_key is None: - break + # Defensive: the sole caller (``_track_entry``) guarantees + # ``value_size <= max_size``, so an empty cache always has room. + break # pragma: no cover await self._evict_key(lru_key) def _next_eviction_candidate(self) -> _CacheEntryKey | None: @@ -342,17 +344,19 @@ async def _track_entry(self, entry_key: _CacheEntryKey, value: Buffer) -> bool: return False async with self._state.lock: - # If key already exists, subtract old size first (an absent slot has - # size 0, so this also cleanly upgrades a negative entry to present). - old = self._state.entries.get(entry_key) + # Pop any existing slot for this key first, reclaiming its bytes. Popping + # (rather than leaving it in place) is essential: it removes the key from + # the eviction candidates so ``_accommodate_value`` cannot select the very + # key being (re)tracked — which would double-subtract its size, stop the + # eviction loop early, and (for a present overwrite) delete the value the + # caller just wrote to the backing store. The caller has already written + # the new value, so we do not touch the backing store here. + old = self._state.entries.pop(entry_key, None) if old is not None: - self._state.current_size -= old.size + self._state.current_size = max(0, self._state.current_size - old.size) - # Make room for the new value + # Make room for the new value, then track it (appended as most-recent). await self._accommodate_value(value_size) - - # Update tracking atomically. Assigning to an existing key preserves - # its LRU position, matching the previous behaviour. self._state.entries[entry_key] = _Entry( insert_time=time.monotonic(), size=value_size, present=True ) @@ -402,11 +406,14 @@ async def _cache_miss( if byte_range is None: await self._cache.delete(key) async with self._state.lock: - self._remove_from_tracking(key) - # The key is absent in the source: remember the miss so a repeat - # read can short-circuit without a source round-trip. + # The key is absent in the source. Either remember the miss (so a + # repeat read short-circuits without a source round-trip) or just + # drop any stale tracking slot — ``_record_missing`` replaces the + # slot itself, reclaiming the bytes of any prior cached value. if self.cache_missing: await self._record_missing(key) + else: + self._remove_from_tracking(key) else: entry_key: _CacheEntryKey = (key, byte_range) async with self._state.lock: @@ -550,13 +557,17 @@ async def set_if_not_exists(self, key: str, value: Buffer) -> None: The data to store """ await super().set_if_not_exists(key, value) - # Whether or not the write happened, any negative entry is now unsafe: either - # we just wrote the key, or it already existed (so the record was already - # wrong). Evicting unconditionally is always safe. We do not populate the - # positive cache here — there is no guaranteed-fresh value to store. - if self.cache_missing: - async with self._state.lock: - self._evict_missing(key) + # Whether or not the write happened, any cached state for this key may now be + # stale (we may have just written a new value, or it already existed). Drop + # all of it — byte-range entries, any positive value, and any negative marker + # — so the next read reflects the source. Invalidating unconditionally is + # always safe. We do not populate the positive cache here: there is no + # guaranteed-fresh value to store (the write may have been a no-op). + async with self._state.lock: + self._invalidate_range_entries(key) + await self._cache.delete(key) + async with self._state.lock: + self._remove_from_tracking(key) async def delete(self, key: str) -> None: """ @@ -617,12 +628,12 @@ async def clear_cache(self) -> None: if hasattr(self._cache, "clear"): await self._cache.clear() - # Reset tracking + # Reset tracking. Cumulative performance counters (hits/misses/evictions/ + # negative_hits) are lifetime stats and intentionally survive a clear. async with self._state.lock: self._state.entries.clear() self._state.range_cache.clear() self._state.current_size = 0 - self._state.negative_hits = 0 def __repr__(self) -> str: """Return string representation of the cache store.""" diff --git a/tests/test_experimental/test_cache_store.py b/tests/test_experimental/test_cache_store.py index 353e73af82..42c84f7ca3 100644 --- a/tests/test_experimental/test_cache_store.py +++ b/tests/test_experimental/test_cache_store.py @@ -1267,3 +1267,134 @@ async def test_no_size_leak_on_miss_then_set_if_not_exists(self) -> None: assert info["missing_keys"] == 0 total = sum(entry.size for entry in cs._state.entries.values()) assert total == info["current_size"] + + async def test_stale_cached_value_becomes_negative_entry(self) -> None: + """A cached value whose source key later reads absent is replaced by a negative + marker, reclaiming the value's bytes (no leftover positive accounting).""" + from zarr.experimental.cache_store import _NEGATIVE_ENTRY_SIZE + + source = MemoryStore() + value = CPUBuffer.from_bytes(b"v" * 200) + cs = CacheStore(source, cache_store=MemoryStore(), cache_missing=True, max_age_seconds=1) + proto = default_buffer_prototype() + + # Cache a present value, then delete the key from the source out-of-band. + await cs.set("k", value) + assert cs.cache_info()["current_size"] == len(value) + await source.delete("k") + # Force the cached entry stale so the next read consults the (now empty) source. + cs._state.entries["k"].insert_time = time.monotonic() - 10 + + assert await cs.get("k", proto) is None # source absent -> records a miss + info = cs.cache_info() + assert info["cached_keys"] == 0 + assert info["missing_keys"] == 1 + # The 200-byte value was reclaimed; only the marker's overhead remains. + assert info["current_size"] == _NEGATIVE_ENTRY_SIZE + + async def test_miss_not_recorded_when_budget_full_of_values(self) -> None: + """When the budget is full of cached values and no markers exist to evict, a + miss is not recorded (a marker never displaces a cached value).""" + source = MemoryStore() + value = CPUBuffer.from_bytes(b"v" * 200) + # Budget fits exactly one value, with no room for a negative marker. + cs = CacheStore(source, cache_store=MemoryStore(), cache_missing=True, max_size=200) + proto = default_buffer_prototype() + + await cs.set("present", value) + assert cs.cache_info()["cached_keys"] == 1 + + assert await cs.get("absent", proto) is None + info = cs.cache_info() + assert info["missing_keys"] == 0 # no room -> miss not remembered + assert info["cached_keys"] == 1 # cached value untouched + assert info["current_size"] == len(value) + + async def test_caching_value_evicts_absent_markers(self) -> None: + """Caching a present value reclaims room by evicting negative markers first.""" + from zarr.experimental.cache_store import _NEGATIVE_ENTRY_SIZE + + source = MemoryStore() + cs = CacheStore( + source, + cache_store=MemoryStore(), + cache_missing=True, + max_size=3 * _NEGATIVE_ENTRY_SIZE, + ) + proto = default_buffer_prototype() + + # Fill the budget with three negative markers. + for i in range(3): + assert await cs.get(f"absent/{i}", proto) is None + assert cs.cache_info()["missing_keys"] == 3 + + # Caching a value must evict marker(s) to fit — markers go before any value. + await cs.set("v", CPUBuffer.from_bytes(b"v" * 100)) + info = cs.cache_info() + assert info["cached_keys"] == 1 + assert info["missing_keys"] < 3 # at least one marker evicted to make room + assert info["current_size"] <= cs.max_size + + async def test_upgrade_marker_to_value_under_pressure_evicts_other_entry(self) -> None: + """Upgrading a stale negative marker to a cached value under memory pressure must + evict a *different* entry, not self-evict (which would under-count current_size + and breach max_size).""" + from zarr.experimental.cache_store import _NEGATIVE_ENTRY_SIZE + + source = MemoryStore() + cs = CacheStore( + source, + cache_store=MemoryStore(), + cache_missing=True, + max_age_seconds=1000, + max_size=300, + ) + proto = default_buffer_prototype() + + # One cached value "a" (128 B) and one negative marker "k" (128 B) → 256 B used. + await cs.set("a", CPUBuffer.from_bytes(b"a" * (_NEGATIVE_ENTRY_SIZE))) + assert await cs.get("k", proto) is None + assert cs.cache_info()["current_size"] == 2 * _NEGATIVE_ENTRY_SIZE + + # "k" now exists in the source with a 200 B value; force the marker stale so the + # next read fetches it and upgrades the slot to a present value (needs eviction). + await source.set("k", CPUBuffer.from_bytes(b"k" * 200)) + cs._state.entries["k"].insert_time = time.monotonic() - 5000 + + result = await cs.get("k", proto) + assert result is not None + assert result.to_bytes() == b"k" * 200 + + info = cs.cache_info() + # Only "k" remains (the other value "a" was evicted to make room); the bound + # holds and the size accounting matches the actual tracked entries exactly. + assert info["cached_keys"] == 1 + assert "a" not in cs._state.entries + assert not await cs._cache.exists("a") + assert info["current_size"] == 200 + assert info["current_size"] <= cs.max_size + total = sum(entry.size for entry in cs._state.entries.values()) + assert total == info["current_size"] + + async def test_set_if_not_exists_invalidates_stale_byte_range(self) -> None: + """``set_if_not_exists`` must invalidate cached byte-range entries, not just the + negative marker, so a later byte-range read does not return stale bytes.""" + source = MemoryStore() + cs = CacheStore(source, cache_store=MemoryStore(), cache_missing=True) + proto = default_buffer_prototype() + + await source.set("k", CPUBuffer.from_bytes(b"old data!!")) + r1 = await cs.get("k", proto, byte_range=RangeByteRequest(0, 3)) + assert r1 is not None + assert r1.to_bytes() == b"old" + assert ("k", RangeByteRequest(0, 3)) in cs._state.entries + + # Source key removed out-of-band, then re-created via set_if_not_exists. + await source.delete("k") + await cs.set_if_not_exists("k", CPUBuffer.from_bytes(b"NEW data!!")) + + # The stale byte-range entry must be gone, and a fresh read returns new bytes. + assert ("k", RangeByteRequest(0, 3)) not in cs._state.entries + r2 = await cs.get("k", proto, byte_range=RangeByteRequest(0, 3)) + assert r2 is not None + assert r2.to_bytes() == b"NEW" From 383448c9c8e9bd45643665d80d6e0dc87f3e392f Mon Sep 17 00:00:00 2001 From: Shane Grigsby Date: Wed, 22 Jul 2026 15:33:26 -0700 Subject: [PATCH 07/15] default finite max age, other minor review fixes --- changes/4040.feature.md | 2 +- docs/user-guide/experimental.md | 9 +- src/zarr/experimental/cache_store.py | 102 +++++++++++++---- tests/test_experimental/test_cache_store.py | 116 +++++++++++++++++++- 4 files changed, 202 insertions(+), 27 deletions(-) diff --git a/changes/4040.feature.md b/changes/4040.feature.md index 7e2813ed5d..bb6d3608dc 100644 --- a/changes/4040.feature.md +++ b/changes/4040.feature.md @@ -1 +1 @@ -`zarr.experimental.cache_store.CacheStore` now performs negative caching by default (`cache_missing=True`, opt-out). A full-key read that finds the key absent in the source store is remembered, so repeat reads of that absent key return immediately without a source round-trip — useful for sparse arrays where most chunks resolve to the fill value. Remembered misses respect `max_age_seconds` and are evicted when the key is written via `set`/`set_if_not_exists`. Negative-cache activity is reported as `negative_hits` in `cache_stats()` and `missing_keys` in `cache_info()`. Only full-key reads are affected (not byte-range reads or `exists`). Pass `cache_missing=False` to restore the previous behavior. Like the positive cache (unbounded when `max_size is None`), the negative cache is bounded only by `max_age_seconds`; set a finite TTL for scans over very large sparse key spaces. +`zarr.experimental.cache_store.CacheStore` now performs negative caching by default (`cache_missing=True`, opt-out). A full-key read that finds the key absent in the source store is remembered, so repeat reads of that absent key return immediately without a source round-trip — useful for sparse arrays where most chunks resolve to the fill value. Remembered misses respect `max_age_seconds` and are evicted when the key is written via `set`/`set_if_not_exists`. Negative-cache activity is reported as `negative_hits` in `cache_stats()` and `missing_keys` in `cache_info()`. Only full-key reads are affected (not byte-range reads or `exists`). Pass `cache_missing=False` to restore the previous behavior. The default `max_age_seconds` is now finite (300 seconds) so both cached values and remembered misses are re-validated against the source at bounded staleness; pass `"infinity"` to opt out. Negative markers share the `max_size` byte budget with cached values (each charged a small flat overhead, evicted marker-first), and when `max_size` is `None` the marker count is capped at an internal limit (100,000, least-recently-used evicted first), so scans over very large sparse key spaces stay bounded. diff --git a/docs/user-guide/experimental.md b/docs/user-guide/experimental.md index 6da2619882..5f3ee20b37 100644 --- a/docs/user-guide/experimental.md +++ b/docs/user-guide/experimental.md @@ -259,10 +259,11 @@ flat overhead, and under memory pressure markers are evicted (least-recently-use before any cached value, so a flood of empty-chunk reads can never evict real cached data. Only full-key reads are affected — byte-range reads and `exists()` are unchanged. -> **Note:** With the default `max_age_seconds="infinity"`, a remembered miss never expires, -> so a key written to the source by another process stays invisible through the cache until -> it is written through the cache. Pair `cache_missing=True` with a finite `max_age_seconds` -> when the source may be written concurrently. For very large sparse arrays, prefer the +> **Note:** A remembered miss stays visible-as-absent until it expires (`max_age_seconds`, +> a finite 300 seconds by default) or the key is written through the cache. With +> `max_age_seconds="infinity"` it never expires, so a key written to the source by another +> process stays invisible through the cache; keep the TTL finite when the source may be +> written concurrently. For very large sparse arrays, prefer the > array-level sparse-read primitives `zarr.shards_initialized` / `zarr.read_regions`, which > read only populated chunks and avoid the empty-chunk reads entirely. diff --git a/src/zarr/experimental/cache_store.py b/src/zarr/experimental/cache_store.py index 152e3394c2..1daa2b1f62 100644 --- a/src/zarr/experimental/cache_store.py +++ b/src/zarr/experimental/cache_store.py @@ -27,6 +27,13 @@ # together — rather than letting negative entries grow without limit. _NEGATIVE_ENTRY_SIZE = 128 +# When ``max_size is None`` the shared byte budget cannot bound anything, so the +# number of negative (known-absent) markers is capped here instead: recording a +# marker beyond this count evicts the least-recently-used marker first. This keeps +# a scan over a very large sparse key space from accumulating one entry per absent +# key without bound (at the default charge this cap is ~13 MB of index overhead). +_MAX_NEGATIVE_ENTRIES = 100_000 + @dataclass(slots=True) class _Entry: @@ -60,6 +67,9 @@ class _CacheState: # key_insert_times / missing_keys structures so a key has one unambiguous state. entries: OrderedDict[_CacheEntryKey, _Entry] = field(default_factory=OrderedDict) current_size: int = 0 + # Number of entries with ``present=False``, maintained incrementally so the + # negative-marker cap (``_MAX_NEGATIVE_ENTRIES``) is O(1) to enforce. + negative_count: int = 0 lock: asyncio.Lock = field(default_factory=asyncio.Lock) hits: int = 0 misses: int = 0 @@ -90,7 +100,9 @@ class CacheStore(WrapperStore[Store]): supports deletes) max_age_seconds : int or "infinity", optional Maximum age of cached entries in seconds. The string "infinity" means - entries never expire. Default is "infinity". + entries never expire. Default is 300 (five minutes), so that both cached + values and remembered misses are re-validated against the source at a + bounded staleness; pass "infinity" to opt out of expiration entirely. max_size : int | None, optional Maximum size of the cache in bytes. When exceeded, least recently used items are evicted. None means unlimited size. Default is None. @@ -108,17 +120,17 @@ class CacheStore(WrapperStore[Store]): Notes: - - With ``max_age_seconds="infinity"`` (the default) a remembered miss never - expires, so a key written to the source by another process stays invisible - through this cache. Pair ``cache_missing=True`` with a finite - ``max_age_seconds`` if the source may be written concurrently. + - With ``max_age_seconds="infinity"`` a remembered miss never expires, so a + key written to the source by another process stays invisible through this + cache until it is written through this instance. The default finite + ``max_age_seconds`` bounds that staleness; keep it finite if the source + may be written concurrently. - Negative entries share the ``max_size`` budget with cached values: each is charged a small flat overhead, and under memory pressure miss-markers are evicted (least-recently-used first) before any cached value. A single - ``max_size`` therefore bounds *total* cache memory. When ``max_size is None`` - both caches are unbounded, so a scan over a very large sparse key space will - accumulate one small entry per absent key; set ``max_size`` (and/or a finite - ``max_age_seconds``, or ``cache_missing=False``) for such workloads. + ``max_size`` therefore bounds *total* cache memory. When ``max_size is + None``, the number of negative markers is instead capped at an internal + limit (100,000), evicting the least-recently-used marker first. - This is store-level, per-key negative caching aimed at the stock ``arr[:]`` path, which probes every chunk. For very large sparse arrays, prefer the array-level sparse-read primitives ``zarr.shards_initialized`` and @@ -161,7 +173,7 @@ def __init__( store: Store, *, cache_store: Store, - max_age_seconds: int | str = "infinity", + max_age_seconds: int | str = 300, max_size: int | None = None, cache_set_data: bool = True, cache_missing: bool = True, @@ -238,9 +250,11 @@ async def _record_missing(self, key: str) -> None: old = self._state.entries.pop(key, None) if old is not None: self._state.current_size = max(0, self._state.current_size - old.size) + if not old.present: + self._state.negative_count -= 1 - # Make room by evicting older absent markers only — never cached values. if self.max_size is not None: + # Make room by evicting older absent markers only — never cached values. while self._state.current_size + _NEGATIVE_ENTRY_SIZE > self.max_size: lru_absent = next( (k for k, e in self._state.entries.items() if not e.present), None @@ -248,12 +262,23 @@ async def _record_missing(self, key: str) -> None: if lru_absent is None: return # only cached values fill the budget — don't record the miss await self._evict_key(lru_absent) + else: + # No byte budget to share: bound the marker *count* instead, so a scan + # over a huge sparse key space cannot grow the index without limit. + while self._state.negative_count >= _MAX_NEGATIVE_ENTRIES: + lru_absent = next( + (k for k, e in self._state.entries.items() if not e.present), None + ) + if lru_absent is None: # pragma: no cover - count implies one exists + break + await self._evict_key(lru_absent) # The key was popped above, so this assignment appends it as most-recent. self._state.entries[key] = _Entry( insert_time=time.monotonic(), size=_NEGATIVE_ENTRY_SIZE, present=False ) self._state.current_size += _NEGATIVE_ENTRY_SIZE + self._state.negative_count += 1 def _evict_missing(self, key: str) -> None: """Drop any negative entry for *key* (it is now present or being written). @@ -265,6 +290,7 @@ def _evict_missing(self, key: str) -> None: entry = self._state.entries.get(key) if entry is not None and not entry.present: self._state.current_size = max(0, self._state.current_size - entry.size) + self._state.negative_count -= 1 del self._state.entries[key] async def _accommodate_value(self, value_size: int) -> None: @@ -313,6 +339,8 @@ async def _evict_key(self, entry_key: _CacheEntryKey) -> None: """ entry = self._state.entries.pop(entry_key, None) key_size = entry.size if entry is not None else 0 + if entry is not None and not entry.present: + self._state.negative_count -= 1 if isinstance(entry_key, str): # Absent markers store no value in the backing cache — skip the delete. @@ -355,6 +383,8 @@ async def _track_entry(self, entry_key: _CacheEntryKey, value: Buffer) -> bool: old = self._state.entries.pop(entry_key, None) if old is not None: self._state.current_size = max(0, self._state.current_size - old.size) + if not old.present: + self._state.negative_count -= 1 # Make room for the new value, then track it (appended as most-recent). await self._accommodate_value(value_size) @@ -381,6 +411,8 @@ def _remove_from_tracking(self, entry_key: _CacheEntryKey) -> None: entry = self._state.entries.pop(entry_key, None) if entry is not None: self._state.current_size = max(0, self._state.current_size - entry.size) + if not entry.present: + self._state.negative_count -= 1 def _invalidate_range_entries(self, key: str) -> None: """Remove all byte-range entries for *key* from the range cache and tracking. @@ -400,17 +432,31 @@ def _invalidate_range_entries(self, key: str) -> None: # ------------------------------------------------------------------ async def _cache_miss( - self, key: str, byte_range: ByteRequest | None, result: Buffer | None + self, key: str, byte_range: ByteRequest | None, result: Buffer | None, fetched_at: float ) -> None: - """Handle a cache miss by storing or cleaning up after a source-store fetch.""" + """Handle a cache miss by storing or cleaning up after a source-store fetch. + + ``fetched_at`` is the monotonic time at which the source fetch *began*. It + guards the absent path against a write/miss race: if a concurrent ``set`` + completes after the fetch began (leaving a present entry newer than + ``fetched_at``), the stale "absent" result must not shadow the new value. + """ if result is None: if byte_range is None: - await self._cache.delete(key) async with self._state.lock: - # The key is absent in the source. Either remember the miss (so a + entry = self._state.entries.get(key) + if entry is not None and entry.present and entry.insert_time >= fetched_at: + # A concurrent write completed after this fetch began — the key + # now has a (cached) value. Recording the miss would shadow it, + # so drop the stale "absent" observation instead. + return + # The key is absent in the source: drop any (stale) cached value and + # byte-range entries for it, then either remember the miss (so a # repeat read short-circuits without a source round-trip) or just - # drop any stale tracking slot — ``_record_missing`` replaces the - # slot itself, reclaiming the bytes of any prior cached value. + # drop the tracking slot — ``_record_missing`` replaces the slot + # itself, reclaiming the bytes of any prior cached value. + await self._cache.delete(key) + self._invalidate_range_entries(key) if self.cache_missing: await self._record_missing(key) else: @@ -430,7 +476,11 @@ async def _cache_miss( # ``_track_entry`` overwrites the key's single slot with a present # entry, so any prior negative marker is structurally replaced — # no separate negative-cache eviction is needed here. - await self._track_entry(key, result) + tracked = await self._track_entry(key, result) + if not tracked: + # Value too large for the cache — roll back so the backing cache + # holds no untracked (uncounted, unevictable) orphan. + await self._cache.delete(key) else: entry_key = (key, byte_range) self._state.range_cache.setdefault(key, {})[byte_range] = result @@ -467,8 +517,9 @@ async def _get_try_cache( # Cache miss — fetch from source store self._state.misses += 1 + fetched_at = time.monotonic() result = await super().get(key, prototype, byte_range) - await self._cache_miss(key, byte_range, result) + await self._cache_miss(key, byte_range, result, fetched_at) return result async def _get_no_cache( @@ -476,8 +527,9 @@ async def _get_no_cache( ) -> Buffer | None: """Get data directly from source store and update cache.""" self._state.misses += 1 + fetched_at = time.monotonic() result = await super().get(key, prototype, byte_range) - await self._cache_miss(key, byte_range, result) + await self._cache_miss(key, byte_range, result, fetched_at) return result async def get( @@ -512,6 +564,9 @@ async def get( entry = self._state.entries.get(key) if entry is not None and not entry.present and self._is_fresh(key): self._state.negative_hits += 1 + # Mark the marker most-recently-used so eviction stays LRU: + # a frequently-probed absent key should outlive cold markers. + self._state.entries.move_to_end(key) return None entry_key: _CacheEntryKey = (key, byte_range) if byte_range is not None else key @@ -540,7 +595,11 @@ async def set(self, key: str, value: Buffer) -> None: self._evict_missing(key) if self.cache_set_data: await self._cache.set(key, value) - await self._track_entry(key, value) + tracked = await self._track_entry(key, value) + if not tracked: + # Value too large for the cache — roll back so the backing cache + # holds no untracked (uncounted, unevictable) orphan. + await self._cache.delete(key) else: await self._cache.delete(key) async with self._state.lock: @@ -635,6 +694,7 @@ async def clear_cache(self) -> None: self._state.entries.clear() self._state.range_cache.clear() self._state.current_size = 0 + self._state.negative_count = 0 def __repr__(self) -> str: """Return string representation of the cache store.""" diff --git a/tests/test_experimental/test_cache_store.py b/tests/test_experimental/test_cache_store.py index 42c84f7ca3..e78d7f8c30 100644 --- a/tests/test_experimental/test_cache_store.py +++ b/tests/test_experimental/test_cache_store.py @@ -298,7 +298,7 @@ async def test_cache_info(self, cached_store: CacheStore) -> None: # Check initial values assert info["cache_store_type"] == "MemoryStore" - assert info["max_age_seconds"] == "infinity" + assert info["max_age_seconds"] == 300 # the default: finite, bounded staleness assert info["max_size"] is None # Default unlimited assert info["current_size"] == 0 assert info["cache_set_data"] is True @@ -1398,3 +1398,117 @@ async def test_set_if_not_exists_invalidates_stale_byte_range(self) -> None: r2 = await cs.get("k", proto, byte_range=RangeByteRequest(0, 3)) assert r2 is not None assert r2.to_bytes() == b"NEW" + + async def test_default_max_age_is_finite(self) -> None: + """The default TTL is finite so remembered misses (and cached values) are + re-validated against the source at bounded staleness.""" + cs = CacheStore(MemoryStore(), cache_store=MemoryStore()) + assert cs.max_age_seconds == 300 + + async def test_negative_count_capped_without_max_size( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """With ``max_size=None`` the marker *count* is capped: a scan over a huge + sparse key space cannot grow the index without bound.""" + import zarr.experimental.cache_store as mod + + monkeypatch.setattr(mod, "_MAX_NEGATIVE_ENTRIES", 10) + cs = CacheStore(MemoryStore(), cache_store=MemoryStore(), cache_missing=True) + proto = default_buffer_prototype() + + for i in range(25): + assert await cs.get(f"c/{i}", proto) is None + + assert cs.cache_info()["missing_keys"] == 10 + assert cs._state.negative_count == 10 + assert cs.cache_stats()["evictions"] >= 15 + # LRU: the most recent misses are the ones retained. + assert all(f"c/{i}" in cs._state.entries for i in range(15, 25)) + + async def test_concurrent_write_not_shadowed_by_stale_miss(self) -> None: + """A miss observed *before* a concurrent write completed must not overwrite + the newly written (present) entry with an absent marker.""" + source = MemoryStore() + cs = CacheStore(source, cache_store=MemoryStore(), cache_missing=True) + proto = default_buffer_prototype() + + fetched_at = time.monotonic() + # A concurrent set() completes after the (stale) fetch began... + await cs.set("k", CPUBuffer.from_bytes(b"value")) + # ...then the stale absent observation lands. + await cs._cache_miss("k", None, None, fetched_at) + + entry = cs._state.entries.get("k") + assert entry is not None + assert entry.present + assert cs.cache_info()["missing_keys"] == 0 + result = await cs.get("k", proto) + assert result is not None + assert result.to_bytes() == b"value" + + async def test_negative_hit_refreshes_lru(self) -> None: + """A negative-cache hit marks the marker most-recently-used, so a hot absent + key outlives cold markers under eviction pressure.""" + # Budget fits exactly two 128-byte markers. + cs = CacheStore(MemoryStore(), cache_store=MemoryStore(), cache_missing=True, max_size=256) + proto = default_buffer_prototype() + + assert await cs.get("a", proto) is None + assert await cs.get("b", proto) is None + # Touch "a": it becomes most-recently-used. + assert await cs.get("a", proto) is None + assert cs.cache_stats()["negative_hits"] == 1 + # A third marker must evict "b" (the LRU marker), not the hot "a". + assert await cs.get("c", proto) is None + assert "a" in cs._state.entries + assert "b" not in cs._state.entries + assert "c" in cs._state.entries + + async def test_oversized_set_rolls_back_backing_cache(self) -> None: + """A written value larger than ``max_size`` must not linger untracked in the + backing cache store.""" + cache = MemoryStore() + cs = CacheStore(MemoryStore(), cache_store=cache, max_size=64, cache_set_data=True) + proto = default_buffer_prototype() + + await cs.set("big", CPUBuffer.from_bytes(b"x" * 128)) + # The source has the value; the backing cache holds no untracked orphan. + assert await cs._store.get("big", proto) is not None + assert await cache.get("big", proto) is None + assert cs._state.current_size == 0 + + async def test_oversized_read_rolls_back_backing_cache(self) -> None: + """A fetched value larger than ``max_size`` must not linger untracked in the + backing cache store (mirror of the write-path rollback).""" + source = MemoryStore() + cache = MemoryStore() + cs = CacheStore(source, cache_store=cache, max_size=64) + proto = default_buffer_prototype() + + await source.set("big", CPUBuffer.from_bytes(b"x" * 128)) + result = await cs.get("big", proto) + assert result is not None + assert len(result) == 128 + assert await cache.get("big", proto) is None + assert cs._state.current_size == 0 + + async def test_full_key_miss_invalidates_byte_ranges(self) -> None: + """Observing a key absent invalidates cached byte-range entries for it, so + ``get(key)`` and ``get(key, byte_range)`` cannot diverge.""" + source = MemoryStore() + cs = CacheStore(source, cache_store=MemoryStore(), cache_missing=True) + proto = default_buffer_prototype() + + await source.set("k", CPUBuffer.from_bytes(b"old data!!")) + r1 = await cs.get("k", proto, byte_range=RangeByteRequest(0, 3)) + assert r1 is not None + assert r1.to_bytes() == b"old" + assert ("k", RangeByteRequest(0, 3)) in cs._state.entries + + # Key removed out-of-band; a full-key read observes the absence. + await source.delete("k") + assert await cs.get("k", proto) is None + + # The stale byte-range entry is gone, and a fresh range read sees the absence. + assert ("k", RangeByteRequest(0, 3)) not in cs._state.entries + assert await cs.get("k", proto, byte_range=RangeByteRequest(0, 3)) is None From ef49ac4407b0128b08f5ac866174a6ef7e8bd1f0 Mon Sep 17 00:00:00 2001 From: Shane Grigsby Date: Wed, 22 Jul 2026 15:51:37 -0700 Subject: [PATCH 08/15] appease new docs linter: mkdocs-style literal in _Entry docstring --- src/zarr/experimental/cache_store.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/zarr/experimental/cache_store.py b/src/zarr/experimental/cache_store.py index 1daa2b1f62..24089b5629 100644 --- a/src/zarr/experimental/cache_store.py +++ b/src/zarr/experimental/cache_store.py @@ -37,7 +37,7 @@ @dataclass(slots=True) class _Entry: - """A single cache slot, tracked in :attr:`_CacheState.entries`. + """A single cache slot, tracked in ``_CacheState.entries``. ``present=True`` (the default): a value is cached for this key — in the Store-backed cache for full keys, or the in-memory range cache for From 38edc709d6f9c9cb0f685315a4837db3b69b47f4 Mon Sep 17 00:00:00 2001 From: Shane Grigsby Date: Wed, 22 Jul 2026 16:11:12 -0700 Subject: [PATCH 09/15] fix write/miss race guard: compare entry identity, not timestamps (windows coarse clock) --- src/zarr/experimental/cache_store.py | 32 ++++++++++++--------- tests/test_experimental/test_cache_store.py | 8 ++++-- 2 files changed, 24 insertions(+), 16 deletions(-) diff --git a/src/zarr/experimental/cache_store.py b/src/zarr/experimental/cache_store.py index 24089b5629..89db76bb22 100644 --- a/src/zarr/experimental/cache_store.py +++ b/src/zarr/experimental/cache_store.py @@ -432,23 +432,29 @@ def _invalidate_range_entries(self, key: str) -> None: # ------------------------------------------------------------------ async def _cache_miss( - self, key: str, byte_range: ByteRequest | None, result: Buffer | None, fetched_at: float + self, + key: str, + byte_range: ByteRequest | None, + result: Buffer | None, + prior_entry: _Entry | None, ) -> None: """Handle a cache miss by storing or cleaning up after a source-store fetch. - ``fetched_at`` is the monotonic time at which the source fetch *began*. It - guards the absent path against a write/miss race: if a concurrent ``set`` - completes after the fetch began (leaving a present entry newer than - ``fetched_at``), the stale "absent" result must not shadow the new value. + ``prior_entry`` is the key's tracked entry as observed just *before* the + source fetch began (``None`` if there was none). It guards the absent path + against a write/miss race: if the key's slot now holds a *different* present + entry, a concurrent ``set`` completed during the fetch, and the stale + "absent" result must not shadow the new value. Identity (not timestamps) is + used so the check is immune to coarse clocks. """ if result is None: if byte_range is None: async with self._state.lock: entry = self._state.entries.get(key) - if entry is not None and entry.present and entry.insert_time >= fetched_at: - # A concurrent write completed after this fetch began — the key - # now has a (cached) value. Recording the miss would shadow it, - # so drop the stale "absent" observation instead. + if entry is not None and entry.present and entry is not prior_entry: + # A concurrent write completed during this fetch — the key now + # has a (cached) value. Recording the miss would shadow it, so + # drop the stale "absent" observation instead. return # The key is absent in the source: drop any (stale) cached value and # byte-range entries for it, then either remember the miss (so a @@ -517,9 +523,9 @@ async def _get_try_cache( # Cache miss — fetch from source store self._state.misses += 1 - fetched_at = time.monotonic() + prior_entry = self._state.entries.get(key) if byte_range is None else None result = await super().get(key, prototype, byte_range) - await self._cache_miss(key, byte_range, result, fetched_at) + await self._cache_miss(key, byte_range, result, prior_entry) return result async def _get_no_cache( @@ -527,9 +533,9 @@ async def _get_no_cache( ) -> Buffer | None: """Get data directly from source store and update cache.""" self._state.misses += 1 - fetched_at = time.monotonic() + prior_entry = self._state.entries.get(key) if byte_range is None else None result = await super().get(key, prototype, byte_range) - await self._cache_miss(key, byte_range, result, fetched_at) + await self._cache_miss(key, byte_range, result, prior_entry) return result async def get( diff --git a/tests/test_experimental/test_cache_store.py b/tests/test_experimental/test_cache_store.py index e78d7f8c30..4a866acd71 100644 --- a/tests/test_experimental/test_cache_store.py +++ b/tests/test_experimental/test_cache_store.py @@ -1432,11 +1432,13 @@ async def test_concurrent_write_not_shadowed_by_stale_miss(self) -> None: cs = CacheStore(source, cache_store=MemoryStore(), cache_missing=True) proto = default_buffer_prototype() - fetched_at = time.monotonic() - # A concurrent set() completes after the (stale) fetch began... + # No entry exists when the (stale) fetch begins... + prior_entry = cs._state.entries.get("k") + assert prior_entry is None + # ...a concurrent set() completes during the fetch... await cs.set("k", CPUBuffer.from_bytes(b"value")) # ...then the stale absent observation lands. - await cs._cache_miss("k", None, None, fetched_at) + await cs._cache_miss("k", None, None, prior_entry) entry = cs._state.entries.get("k") assert entry is not None From 92eda3b4a96f6b9c54813184f222445aa491b4ea Mon Sep 17 00:00:00 2001 From: Shane Grigsby Date: Tue, 11 Aug 2026 16:02:04 -0700 Subject: [PATCH 10/15] rename changelog fragment to this PR's number (4042, not the ChunkLayout PR) --- changes/{4040.feature.md => 4042.feature.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changes/{4040.feature.md => 4042.feature.md} (100%) diff --git a/changes/4040.feature.md b/changes/4042.feature.md similarity index 100% rename from changes/4040.feature.md rename to changes/4042.feature.md From a9ffcbd40685e1484e30b245ea0e3bd041d5a586 Mon Sep 17 00:00:00 2001 From: Shane Grigsby Date: Tue, 11 Aug 2026 16:02:52 -0700 Subject: [PATCH 11/15] one record per key: nest byte ranges and the absent marker in _KeyState so they cannot diverge --- changes/4042.feature.md | 2 +- src/zarr/experimental/cache_store.py | 579 +++++++++++++------- tests/test_experimental/test_cache_store.py | 203 +++++-- 3 files changed, 526 insertions(+), 258 deletions(-) diff --git a/changes/4042.feature.md b/changes/4042.feature.md index bb6d3608dc..3ebeed4222 100644 --- a/changes/4042.feature.md +++ b/changes/4042.feature.md @@ -1 +1 @@ -`zarr.experimental.cache_store.CacheStore` now performs negative caching by default (`cache_missing=True`, opt-out). A full-key read that finds the key absent in the source store is remembered, so repeat reads of that absent key return immediately without a source round-trip — useful for sparse arrays where most chunks resolve to the fill value. Remembered misses respect `max_age_seconds` and are evicted when the key is written via `set`/`set_if_not_exists`. Negative-cache activity is reported as `negative_hits` in `cache_stats()` and `missing_keys` in `cache_info()`. Only full-key reads are affected (not byte-range reads or `exists`). Pass `cache_missing=False` to restore the previous behavior. The default `max_age_seconds` is now finite (300 seconds) so both cached values and remembered misses are re-validated against the source at bounded staleness; pass `"infinity"` to opt out. Negative markers share the `max_size` byte budget with cached values (each charged a small flat overhead, evicted marker-first), and when `max_size` is `None` the marker count is capped at an internal limit (100,000, least-recently-used evicted first), so scans over very large sparse key spaces stay bounded. +`zarr.experimental.cache_store.CacheStore` now performs negative caching by default (`cache_missing=True`, opt-out). A full-key read that finds the key absent in the source store is remembered, so repeat reads of that absent key return immediately without a source round-trip — useful for sparse arrays where most chunks resolve to the fill value. Remembered misses respect `max_age_seconds` and are dropped when the key is written via `set`/`set_if_not_exists` or when a byte-range read observes data for the key. Negative-cache activity is reported as `negative_hits` in `cache_stats()` and `missing_keys` in `cache_info()`. Only full-key reads are affected (not byte-range reads or `exists`). Pass `cache_missing=False` to restore the previous behavior. The default `max_age_seconds` is now finite (300 seconds) so both cached values and remembered misses are re-validated against the source at bounded staleness; pass `"infinity"` to opt out. Negative markers share the `max_size` byte budget with cached values (each charged a small flat overhead, evicted marker-first), and when `max_size` is `None` the marker count is capped at an internal limit (100,000, least-recently-used evicted first), so scans over very large sparse key spaces stay bounded. diff --git a/src/zarr/experimental/cache_store.py b/src/zarr/experimental/cache_store.py index 272f5c1d4a..2483e7741e 100644 --- a/src/zarr/experimental/cache_store.py +++ b/src/zarr/experimental/cache_store.py @@ -15,11 +15,6 @@ if TYPE_CHECKING: from zarr.core.buffer.core import Buffer, BufferPrototype -# A cache entry identifier. Plain ``str`` for full-key entries that live in -# the Store-backed cache; ``(str, ByteRequest)`` for byte-range entries that -# live in the in-memory range cache. -_CacheEntryKey = str | tuple[str, ByteRequest] - # Nominal byte cost charged to ``max_size`` for a negative (known-absent) entry. # Such entries carry no data, but each one occupies an index slot (the key plus a # small ``_Entry`` record), so it is charged a flat overhead. This lets a single @@ -37,45 +32,92 @@ @dataclass(slots=True) class _Entry: - """A single cache slot, tracked in ``_CacheState.entries``. + """The full-key slot of a ``_KeyState``. - ``present=True`` (the default): a value is cached for this key — in the - Store-backed cache for full keys, or the in-memory range cache for - byte-range keys — occupying ``size`` bytes. + ``present=True`` (the default): the key's full value is cached in the + Store-backed cache, occupying ``size`` bytes. - ``present=False``: the full key is known-*absent* in the source store (a + ``present=False``: the key is known-*absent* in the source store (a negative-cache entry). It carries no data, but is charged a flat ``_NEGATIVE_ENTRY_SIZE`` against ``max_size`` for the index slot it occupies, so cached values and miss-markers share one memory budget. Its staleness is bounded by ``max_age_seconds``. - - Because every key maps to exactly one ``_Entry``, "present" and "absent" are - mutually exclusive by construction: a key cannot simultaneously be cached and - marked missing. """ insert_time: float size: int = 0 present: bool = True + last_used: float = 0.0 + + +@dataclass(slots=True) +class _RangeEntry: + """A cached byte-range of a key's value. + + Held in memory inside the key's ``_KeyState`` so partial reads never touch + the persistent backend. ``insert_time`` bounds staleness via + ``max_age_seconds``; ``last_used`` orders eviction within the key. + """ + + buffer: Buffer + insert_time: float + size: int + last_used: float + + +@dataclass(slots=True) +class _KeyState: + """All cached knowledge about a single source key. + + ``full`` is the full-key slot: a cached value, a known-absent marker, or + ``None`` when nothing is known about the key as a whole. ``ranges`` holds + the key's cached byte-range reads. + + Because everything known about a key lives in this one record, every + mutation for the key goes through one slot under one lock, and the + invariant *absent implies no ranges* is asserted at the mutation points + (``assert_coherent``) instead of being maintained across parallel + structures: a key can never simultaneously be marked missing and hold + cached bytes, so full-key and byte-range answers cannot diverge. + """ + + full: _Entry | None = None + ranges: dict[ByteRequest, _RangeEntry] = field(default_factory=dict) + + def assert_coherent(self) -> None: + """Assert the record invariant: a key marked absent holds no range data.""" + assert self.full is None or self.full.present or not self.ranges, ( + "cache incoherent: key marked absent still holds byte-range data" + ) + + @property + def is_empty(self) -> bool: + return self.full is None and not self.ranges + + @property + def tracked_size(self) -> int: + """Total bytes this record charges against the shared budget.""" + full_size = self.full.size if self.full is not None else 0 + return full_size + sum(r.size for r in self.ranges.values()) @dataclass(slots=True) class _CacheState: - # Single source of truth for every tracked key (full-key and byte-range, - # present and absent). Ordered for LRU eviction; ``move_to_end`` marks a key - # most-recently-used. Replaces the former parallel cache_order / key_sizes / - # key_insert_times / missing_keys structures so a key has one unambiguous state. - entries: OrderedDict[_CacheEntryKey, _Entry] = field(default_factory=OrderedDict) + # Single source of truth: one record per source key, holding the full-key + # state (value / absent marker) *and* the key's byte-range buffers. Ordered + # for key-level LRU eviction; ``move_to_end`` marks a key most-recently-used. + # Replaces the former flat entry dict (tuple keys for byte ranges) plus the + # separate range cache, so a key has one unambiguous, atomically-mutated state. + entries: OrderedDict[str, _KeyState] = field(default_factory=OrderedDict) current_size: int = 0 - # Number of entries with ``present=False``, maintained incrementally so the - # negative-marker cap (``_MAX_NEGATIVE_ENTRIES``) is O(1) to enforce. + # Number of keys whose full slot is an absent marker, maintained incrementally + # so the negative-marker cap and the eviction-candidate fast path are O(1). negative_count: int = 0 lock: asyncio.Lock = field(default_factory=asyncio.Lock) hits: int = 0 misses: int = 0 evictions: int = 0 negative_hits: int = 0 - range_cache: dict[str, dict[ByteRequest, Buffer]] = field(default_factory=dict) class CacheStore(WrapperStore[Store]): @@ -87,8 +129,8 @@ class CacheStore(WrapperStore[Store]): time-based expiration, size-based eviction, and flexible cache storage options. Full-key reads are cached in the Store-backed cache. Byte-range reads are - cached in a separate in-memory dictionary so that partial reads never - pollute the filesystem (or other persistent backend). Both caches share + cached in memory, inside the key's tracking record, so that partial reads + never pollute the filesystem (or other persistent backend). Both share the same ``max_size`` budget and LRU eviction policy. Parameters @@ -114,9 +156,10 @@ class CacheStore(WrapperStore[Store]): ``get`` that finds the key absent in the source store records that absence, so subsequent ``get``s for the same key return ``None`` without a source round-trip. This benefits repeated reads of sparse arrays (most chunks absent). Negative - entries respect ``max_age_seconds`` and are evicted when the key is written - (``set``/``set_if_not_exists``). Only full-key reads are affected (not byte-range - reads or ``exists``). Default is True. + entries respect ``max_age_seconds`` and are dropped when the key is written + (``set``/``set_if_not_exists``) or when a byte-range read observes data for + the key. Only full-key reads consult the negative cache (not byte-range reads + or ``exists``). Default is True. Notes: @@ -218,65 +261,126 @@ def with_read_only(self, read_only: bool = False) -> Self: store._state = self._state return store - def _is_fresh(self, entry_key: _CacheEntryKey) -> bool: - """Check if a tracked entry (present or absent) is still fresh. + def _is_fresh(self, key: str, byte_range: ByteRequest | None = None) -> bool: + """Check if a tracked slot (full-key, or one byte range) is still fresh. Uses monotonic time for accurate elapsed time measurement. A key with no - entry is treated as not fresh (except under an infinite TTL, matching the - previous behaviour of routing unseen keys through the cache path). + tracked slot for the request is treated as not fresh (except under an + infinite TTL, matching the previous behaviour of routing unseen keys + through the cache path). """ if self.max_age_seconds == "infinity": return True - entry = self._state.entries.get(entry_key) - if entry is None: + state = self._state.entries.get(key) + if state is None: return False - elapsed = time.monotonic() - entry.insert_time + slot: _Entry | _RangeEntry | None + slot = state.full if byte_range is None else state.ranges.get(byte_range) + if slot is None: + return False + elapsed = time.monotonic() - slot.insert_time return elapsed < self.max_age_seconds + # ------------------------------------------------------------------ + # tracking-state mutation helpers (all require ``self._state.lock``) + # ------------------------------------------------------------------ + + def _reclaim_full(self, state: _KeyState) -> None: + """Drop *state*'s full slot, reclaiming its charged bytes. + + Must be called while holding ``self._state.lock``. + """ + if state.full is not None: + self._state.current_size = max(0, self._state.current_size - state.full.size) + if not state.full.present: + self._state.negative_count -= 1 + state.full = None + + def _reclaim_ranges(self, state: _KeyState) -> None: + """Drop all of *state*'s byte-range entries, reclaiming their bytes. + + Must be called while holding ``self._state.lock``. + """ + for range_entry in state.ranges.values(): + self._state.current_size = max(0, self._state.current_size - range_entry.size) + state.ranges.clear() + + def _drop_full_slot(self, key: str) -> None: + """Remove *key*'s full slot (value or marker) from tracking. + + Byte-range entries for the key are left in place. Must be called while + holding ``self._state.lock``. + """ + state = self._state.entries.get(key) + if state is not None: + self._reclaim_full(state) + if state.is_empty: + del self._state.entries[key] + + def _drop_key(self, key: str) -> None: + """Remove everything tracked for *key* (full slot and byte ranges). + + Must be called while holding ``self._state.lock``. + """ + state = self._state.entries.pop(key, None) + if state is not None: + self._reclaim_full(state) + self._reclaim_ranges(state) + + def _invalidate_range_entries(self, key: str) -> None: + """Drop *key*'s cached byte-range entries (the source data changed). + + Must be called while holding ``self._state.lock``. + """ + state = self._state.entries.get(key) + if state is not None: + self._reclaim_ranges(state) + if state.is_empty: + del self._state.entries[key] + async def _record_missing(self, key: str) -> None: """Record *key* as known-missing (absent in the source store). + Replaces the key's whole record: any previously cached value bytes and + byte-range buffers are reclaimed in the same mutation, so the invariant + *absent implies no ranges* holds by construction. The caller has already + removed any backing-store value for *key*. + Charges a flat ``_NEGATIVE_ENTRY_SIZE`` against the shared ``max_size`` budget. A negative marker is strictly lower priority than cached data: it may only displace *other* (older) absent markers to fit, never a cached value, and is skipped entirely if the budget is full of cached values. - - The caller (``_cache_miss``) has already removed any backing-store value and - tracking slot for *key*, so this records a fresh marker. Must be called - while holding ``self._state.lock``. Staleness is bounded by - ``max_age_seconds`` via ``_is_fresh``. + Must be called while holding ``self._state.lock``. Staleness is bounded + by ``max_age_seconds``. """ - # Drop any pre-existing slot for this key, reclaiming its bytes. old = self._state.entries.pop(key, None) if old is not None: - self._state.current_size = max(0, self._state.current_size - old.size) - if not old.present: - self._state.negative_count -= 1 + self._reclaim_full(old) + self._reclaim_ranges(old) if self.max_size is not None: # Make room by evicting older absent markers only — never cached values. while self._state.current_size + _NEGATIVE_ENTRY_SIZE > self.max_size: - lru_absent = next( - (k for k, e in self._state.entries.items() if not e.present), None - ) + lru_absent = self._lru_absent_key() if lru_absent is None: return # only cached values fill the budget — don't record the miss - await self._evict_key(lru_absent) + await self._evict_slot(lru_absent) else: # No byte budget to share: bound the marker *count* instead, so a scan # over a huge sparse key space cannot grow the index without limit. while self._state.negative_count >= _MAX_NEGATIVE_ENTRIES: - lru_absent = next( - (k for k, e in self._state.entries.items() if not e.present), None - ) + lru_absent = self._lru_absent_key() if lru_absent is None: # pragma: no cover - count implies one exists break - await self._evict_key(lru_absent) + await self._evict_slot(lru_absent) - # The key was popped above, so this assignment appends it as most-recent. - self._state.entries[key] = _Entry( - insert_time=time.monotonic(), size=_NEGATIVE_ENTRY_SIZE, present=False + now = time.monotonic() + state = _KeyState( + full=_Entry(insert_time=now, size=_NEGATIVE_ENTRY_SIZE, present=False, last_used=now) ) + state.assert_coherent() + # The key was popped above, so this assignment appends it as most-recent. + self._state.entries[key] = state self._state.current_size += _NEGATIVE_ENTRY_SIZE self._state.negative_count += 1 @@ -287,11 +391,12 @@ def _evict_missing(self, key: str) -> None: left untouched — and reclaims the marker's charged bytes. Must be called while holding ``self._state.lock``. """ - entry = self._state.entries.get(key) - if entry is not None and not entry.present: - self._state.current_size = max(0, self._state.current_size - entry.size) - self._state.negative_count -= 1 - del self._state.entries[key] + state = self._state.entries.get(key) + if state is not None and state.full is not None and not state.full.present: + state.assert_coherent() + self._reclaim_full(state) + if state.is_empty: + del self._state.entries[key] async def _accommodate_value(self, value_size: int) -> None: """Evict until ``value_size`` more bytes fit within ``max_size``. @@ -308,57 +413,77 @@ async def _accommodate_value(self, value_size: int) -> None: while self._state.current_size + value_size > self.max_size: lru_key = self._next_eviction_candidate() if lru_key is None: - # Defensive: the sole caller (``_track_entry``) guarantees - # ``value_size <= max_size``, so an empty cache always has room. + # Defensive: the callers guarantee ``value_size <= max_size``, + # so an empty cache always has room. break # pragma: no cover - await self._evict_key(lru_key) + await self._evict_slot(lru_key) - def _next_eviction_candidate(self) -> _CacheEntryKey | None: - """Return the next entry to evict, preferring absent markers (LRU-first). + def _lru_absent_key(self) -> str | None: + """Return the least-recently-used key marked absent, or ``None``. - Walks entries in LRU order: the first absent entry found is returned; if - none are absent, the least-recently-used present entry is returned. Must - be called while holding self._state.lock. + Must be called while holding self._state.lock. """ - lru_present: _CacheEntryKey | None = None - for entry_key, entry in self._state.entries.items(): - if not entry.present: - return entry_key - if lru_present is None: - lru_present = entry_key - return lru_present + return next( + ( + key + for key, state in self._state.entries.items() + if state.full is not None and not state.full.present + ), + None, + ) - async def _evict_key(self, entry_key: _CacheEntryKey) -> None: - """Evict a cache entry. + def _next_eviction_candidate(self) -> str | None: + """Return the key to evict from next, preferring absent markers (LRU-first). + The entries are walked in LRU order: the first key marked absent is + returned; if none are, the least-recently-used key is the candidate. Must be called while holding self._state.lock. + """ + candidate = self._lru_absent_key() + if candidate is None: + return next(iter(self._state.entries), None) + return candidate - For ``str`` keys the entry is deleted from the Store-backed cache. - For ``(str, ByteRequest)`` keys the entry is removed from the - in-memory range cache. + async def _evict_slot(self, key: str) -> None: + """Evict *key*'s least-recently-used slot, removing an emptied record. + + Must be called while holding self._state.lock. + + The full-key slot competes with the key's byte-range entries on + ``last_used``. Evicting a present full slot deletes the value from the + Store-backed cache; absent markers and byte-range entries live only in + memory. """ - entry = self._state.entries.pop(entry_key, None) - key_size = entry.size if entry is not None else 0 - if entry is not None and not entry.present: - self._state.negative_count -= 1 - - if isinstance(entry_key, str): - # Absent markers store no value in the backing cache — skip the delete. - if entry is None or entry.present: - await self._cache.delete(entry_key) - else: - base_key, byte_range = entry_key - per_key = self._state.range_cache.get(base_key) - if per_key is not None: - per_key.pop(byte_range, None) - if not per_key: - del self._state.range_cache[base_key] + state = self._state.entries.get(key) + if state is None: + # Tracking already dropped (e.g. by a concurrent operation) — make + # sure no orphaned value lingers in the backing cache. + await self._cache.delete(key) + return + + lru_range: ByteRequest | None = None + lru_range_used = float("inf") + for byte_range, range_entry in state.ranges.items(): + if range_entry.last_used < lru_range_used: + lru_range = byte_range + lru_range_used = range_entry.last_used + + if state.full is not None and (lru_range is None or state.full.last_used <= lru_range_used): + entry = state.full + self._reclaim_full(state) + self._state.evictions += 1 + if entry.present: + await self._cache.delete(key) + elif lru_range is not None: + range_entry = state.ranges.pop(lru_range) + self._state.current_size = max(0, self._state.current_size - range_entry.size) + self._state.evictions += 1 - self._state.current_size = max(0, self._state.current_size - key_size) - self._state.evictions += 1 + if state.is_empty: + self._state.entries.pop(key, None) - async def _track_entry(self, entry_key: _CacheEntryKey, value: Buffer) -> bool: - """Register *entry_key* in the shared size / LRU tracking. + async def _track_entry(self, key: str, value: Buffer) -> bool: + """Register a full-key value in the shared size / LRU tracking. Returns ``True`` if the entry was tracked, ``False`` if the value exceeds ``max_size`` and was skipped. Callers should roll back any @@ -373,59 +498,94 @@ async def _track_entry(self, entry_key: _CacheEntryKey, value: Buffer) -> bool: return False async with self._state.lock: - # Pop any existing slot for this key first, reclaiming its bytes. Popping - # (rather than leaving it in place) is essential: it removes the key from - # the eviction candidates so ``_accommodate_value`` cannot select the very - # key being (re)tracked — which would double-subtract its size, stop the - # eviction loop early, and (for a present overwrite) delete the value the - # caller just wrote to the backing store. The caller has already written - # the new value, so we do not touch the backing store here. - old = self._state.entries.pop(entry_key, None) - if old is not None: - self._state.current_size = max(0, self._state.current_size - old.size) - if not old.present: - self._state.negative_count -= 1 - - # Make room for the new value, then track it (appended as most-recent). + # Drop the key's existing full slot first, reclaiming its bytes. This + # removes the slot from the eviction candidates so ``_accommodate_value`` + # cannot select the very slot being (re)tracked — which would + # double-subtract its size, stop the eviction loop early, and (for a + # present overwrite) delete the value the caller just wrote to the + # backing store. The key's *other* slots (byte-range entries) remain + # fair game for eviction. The caller has already written the new value, + # so the backing store is not touched here. + state = self._state.entries.get(key) + if state is not None: + self._reclaim_full(state) + if state.is_empty: + del self._state.entries[key] + + # Make room for the new value, then track it (the record is appended + # or re-inserted as most-recently-used). await self._accommodate_value(value_size) - self._state.entries[entry_key] = _Entry( - insert_time=time.monotonic(), size=value_size, present=True - ) + state = self._state.entries.pop(key, None) + if state is None: + state = _KeyState() + now = time.monotonic() + state.full = _Entry(insert_time=now, size=value_size, present=True, last_used=now) + state.assert_coherent() + self._state.entries[key] = state self._state.current_size += value_size return True - async def _update_access_order(self, entry_key: _CacheEntryKey) -> None: - """Update the access order for LRU tracking.""" - async with self._state.lock: - # Re-check membership under the lock: the entry may have been evicted - # by a concurrent operation between the call and acquiring the lock. - if entry_key in self._state.entries: - self._state.entries.move_to_end(entry_key) + async def _track_range(self, key: str, byte_range: ByteRequest, value: Buffer) -> bool: + """Register a byte-range read in the key's record. - def _remove_from_tracking(self, entry_key: _CacheEntryKey) -> None: - """Remove an entry from tracking, reclaiming any bytes it accounted for. - - Must be called while holding self._state.lock. + Returns ``True`` if the range was cached, ``False`` if the value exceeds + ``max_size`` (nothing is stored in that case, so there is nothing to roll + back). Observing bytes for a key proves it exists in the source, so any + absent marker is dropped in the same locked mutation — this is the single + point where ranges are recorded, so a record can never hold both a marker + and range data (``assert_coherent``), even when the range itself is too + large to cache. """ - entry = self._state.entries.pop(entry_key, None) - if entry is not None: - self._state.current_size = max(0, self._state.current_size - entry.size) - if not entry.present: - self._state.negative_count -= 1 + value_size = len(value) - def _invalidate_range_entries(self, key: str) -> None: - """Remove all byte-range entries for *key* from the range cache and tracking. + async with self._state.lock: + state = self._state.entries.get(key) + if state is not None: + if state.full is not None and not state.full.present: + # Bytes came back for this key: the absent marker is stale. + self._reclaim_full(state) + # Drop any slot being replaced so accommodation cannot select it. + old = state.ranges.pop(byte_range, None) + if old is not None: + self._state.current_size = max(0, self._state.current_size - old.size) + if state.is_empty: + del self._state.entries[key] + + if self.max_size is not None and value_size > self.max_size: + return False - Must be called while holding self._state.lock. - """ - per_key = self._state.range_cache.pop(key, None) - if per_key is not None: - for byte_range in per_key: - entry_key: _CacheEntryKey = (key, byte_range) - entry = self._state.entries.pop(entry_key, None) - if entry is not None: - self._state.current_size = max(0, self._state.current_size - entry.size) + await self._accommodate_value(value_size) + state = self._state.entries.pop(key, None) + if state is None: + state = _KeyState() + now = time.monotonic() + state.ranges[byte_range] = _RangeEntry( + buffer=value, insert_time=now, size=value_size, last_used=now + ) + state.assert_coherent() + self._state.entries[key] = state + self._state.current_size += value_size + + return True + + async def _update_access_order(self, key: str, byte_range: ByteRequest | None = None) -> None: + """Mark a slot — and its key's record — most-recently-used for LRU tracking.""" + async with self._state.lock: + # Re-check membership under the lock: the record may have been evicted + # by a concurrent operation between the call and acquiring the lock. + state = self._state.entries.get(key) + if state is None: + return + now = time.monotonic() + if byte_range is None: + if state.full is not None: + state.full.last_used = now + else: + range_entry = state.ranges.get(byte_range) + if range_entry is not None: + range_entry.last_used = now + self._state.entries.move_to_end(key) # ------------------------------------------------------------------ # get helpers @@ -436,50 +596,55 @@ async def _cache_miss( key: str, byte_range: ByteRequest | None, result: Buffer | None, - prior_entry: _Entry | None, + prior_slot: _Entry | None, ) -> None: """Handle a cache miss by storing or cleaning up after a source-store fetch. - ``prior_entry`` is the key's tracked entry as observed just *before* the + ``prior_slot`` is the key's full slot as observed just *before* the source fetch began (``None`` if there was none). It guards the absent path against a write/miss race: if the key's slot now holds a *different* present entry, a concurrent ``set`` completed during the fetch, and the stale "absent" result must not shadow the new value. Identity (not timestamps) is - used so the check is immune to coarse clocks. + used so the check is immune to coarse clocks. Known blind spot: a + concurrent writer that leaves no present slot behind (``cache_set_data=False``, + or ``set_if_not_exists``, whose override drops tracking rather than inserting + a present slot) cannot be detected here, so its write may be shadowed by the + stale absent observation until the finite default ``max_age_seconds`` expires + the marker. """ if result is None: if byte_range is None: async with self._state.lock: - entry = self._state.entries.get(key) - if entry is not None and entry.present and entry is not prior_entry: + state = self._state.entries.get(key) + current = state.full if state is not None else None + if current is not None and current.present and current is not prior_slot: # A concurrent write completed during this fetch — the key now # has a (cached) value. Recording the miss would shadow it, so # drop the stale "absent" observation instead. return - # The key is absent in the source: drop any (stale) cached value and - # byte-range entries for it, then either remember the miss (so a - # repeat read short-circuits without a source round-trip) or just - # drop the tracking slot — ``_record_missing`` replaces the slot - # itself, reclaiming the bytes of any prior cached value. + # The key is absent in the source: drop any (stale) cached value + # for it, then either remember the miss (so a repeat read + # short-circuits without a source round-trip) or just drop the + # tracking record. Either way the key's byte-range entries go in + # the same mutation, so full-key and ranged reads cannot diverge. await self._cache.delete(key) - self._invalidate_range_entries(key) if self.cache_missing: await self._record_missing(key) else: - self._remove_from_tracking(key) + self._drop_key(key) else: - entry_key: _CacheEntryKey = (key, byte_range) async with self._state.lock: - per_key = self._state.range_cache.get(key) - if per_key is not None: - per_key.pop(byte_range, None) - if not per_key: - del self._state.range_cache[key] - self._remove_from_tracking(entry_key) + state = self._state.entries.get(key) + if state is not None: + old = state.ranges.pop(byte_range, None) + if old is not None: + self._state.current_size = max(0, self._state.current_size - old.size) + if state.is_empty: + del self._state.entries[key] else: if byte_range is None: await self._cache.set(key, result) - # ``_track_entry`` overwrites the key's single slot with a present + # ``_track_entry`` overwrites the key's full slot with a present # entry, so any prior negative marker is structurally replaced — # no separate negative-cache eviction is needed here. tracked = await self._track_entry(key, result) @@ -488,16 +653,15 @@ async def _cache_miss( # holds no untracked (uncounted, unevictable) orphan. await self._cache.delete(key) else: - entry_key = (key, byte_range) - self._state.range_cache.setdefault(key, {})[byte_range] = result - tracked = await self._track_entry(entry_key, result) - if not tracked: - # Value too large for the cache — roll back the insertion - per_key = self._state.range_cache.get(key) - if per_key is not None: - per_key.pop(byte_range, None) - if not per_key: - del self._state.range_cache[key] + # ``_track_range`` stores the buffer inside the key's record and + # drops any stale absent marker in the same locked mutation, so a + # successful ranged read can never leave the key marked missing. + await self._track_range(key, byte_range, result) + + def _prior_full_slot(self, key: str) -> _Entry | None: + """Snapshot the key's full slot for ``_cache_miss``'s write/miss race guard.""" + state = self._state.entries.get(key) + return state.full if state is not None else None async def _get_try_cache( self, key: str, prototype: BufferPrototype, byte_range: ByteRequest | None = None @@ -511,21 +675,20 @@ async def _get_try_cache( await self._update_access_order(key) return maybe_cached else: - # Byte-range read — use in-memory range cache - entry_key: _CacheEntryKey = (key, byte_range) - per_key = self._state.range_cache.get(key) - if per_key is not None: - cached_buf = per_key.get(byte_range) - if cached_buf is not None: + # Byte-range read — served from the key's in-memory record + state = self._state.entries.get(key) + if state is not None: + range_entry = state.ranges.get(byte_range) + if range_entry is not None: self._state.hits += 1 - await self._update_access_order(entry_key) - return cached_buf + await self._update_access_order(key, byte_range) + return range_entry.buffer # Cache miss — fetch from source store self._state.misses += 1 - prior_entry = self._state.entries.get(key) if byte_range is None else None + prior_slot = self._prior_full_slot(key) if byte_range is None else None result = await super().get(key, prototype, byte_range) - await self._cache_miss(key, byte_range, result, prior_entry) + await self._cache_miss(key, byte_range, result, prior_slot) return result async def _get_no_cache( @@ -533,9 +696,9 @@ async def _get_no_cache( ) -> Buffer | None: """Get data directly from source store and update cache.""" self._state.misses += 1 - prior_entry = self._state.entries.get(key) if byte_range is None else None + prior_slot = self._prior_full_slot(key) if byte_range is None else None result = await super().get(key, prototype, byte_range) - await self._cache_miss(key, byte_range, result, prior_entry) + await self._cache_miss(key, byte_range, result, prior_slot) return result @property @@ -577,16 +740,17 @@ async def get( # key has no positive entry and would otherwise be routed straight to the source. if self.cache_missing and byte_range is None: async with self._state.lock: - entry = self._state.entries.get(key) - if entry is not None and not entry.present and self._is_fresh(key): + state = self._state.entries.get(key) + slot = state.full if state is not None else None + if slot is not None and not slot.present and self._is_fresh(key): self._state.negative_hits += 1 # Mark the marker most-recently-used so eviction stays LRU: # a frequently-probed absent key should outlive cold markers. + slot.last_used = time.monotonic() self._state.entries.move_to_end(key) return None - entry_key: _CacheEntryKey = (key, byte_range) if byte_range is not None else key - if not self._is_fresh(entry_key): + if not self._is_fresh(key, byte_range): return await self._get_no_cache(key, prototype, byte_range) else: return await self._get_try_cache(key, prototype, byte_range) @@ -604,11 +768,11 @@ async def set(self, key: str, value: Buffer) -> None: """ await super().set(key, value) # Invalidate all cached byte-range entries (source data changed) and drop any - # negative entry — the key now has a value. + # negative entry — the key now has a value. (No ``cache_missing`` gate here: + # a marker recorded before the flag was flipped off must still be cleared.) async with self._state.lock: self._invalidate_range_entries(key) - if self.cache_missing: - self._evict_missing(key) + self._evict_missing(key) if self.cache_set_data: await self._cache.set(key, value) tracked = await self._track_entry(key, value) @@ -619,7 +783,7 @@ async def set(self, key: str, value: Buffer) -> None: else: await self._cache.delete(key) async with self._state.lock: - self._remove_from_tracking(key) + self._drop_full_slot(key) async def set_if_not_exists(self, key: str, value: Buffer) -> None: """ @@ -640,10 +804,8 @@ async def set_if_not_exists(self, key: str, value: Buffer) -> None: # always safe. We do not populate the positive cache here: there is no # guaranteed-fresh value to store (the write may have been a no-op). async with self._state.lock: - self._invalidate_range_entries(key) + self._drop_key(key) await self._cache.delete(key) - async with self._state.lock: - self._remove_from_tracking(key) async def delete(self, key: str) -> None: """ @@ -655,17 +817,21 @@ async def delete(self, key: str) -> None: The key to delete """ await super().delete(key) - # Invalidate all cached byte-range entries + # Drop the key's whole record: full slot and byte-range entries together. async with self._state.lock: - self._invalidate_range_entries(key) + self._drop_key(key) await self._cache.delete(key) - async with self._state.lock: - self._remove_from_tracking(key) def cache_info(self) -> dict[str, Any]: - """Return information about the cache state.""" - present = sum(1 for entry in self._state.entries.values() if entry.present) - missing = len(self._state.entries) - present + """Return information about the cache state. + + Counts are per source key: ``tracked_keys`` is the number of keys with any + tracked state, ``missing_keys`` the number marked absent, and + ``cached_keys`` the number holding cached data (a full value and/or + byte-range entries). A key is never counted as both cached and missing. + """ + tracked = len(self._state.entries) + missing = self._state.negative_count return { "cache_store_type": type(self._cache).__name__, "max_age_seconds": "infinity" @@ -675,8 +841,8 @@ def cache_info(self) -> dict[str, Any]: "current_size": self._state.current_size, "cache_set_data": self.cache_set_data, "cache_missing": self.cache_missing, - "tracked_keys": len(self._state.entries), - "cached_keys": present, + "tracked_keys": tracked, + "cached_keys": tracked - missing, "missing_keys": missing, } @@ -708,13 +874,12 @@ async def clear_cache(self) -> None: # negative_hits) are lifetime stats and intentionally survive a clear. async with self._state.lock: self._state.entries.clear() - self._state.range_cache.clear() self._state.current_size = 0 self._state.negative_count = 0 def __repr__(self) -> str: """Return string representation of the cache store.""" - cached_keys = sum(1 for entry in self._state.entries.values() if entry.present) + cached_keys = len(self._state.entries) - self._state.negative_count return ( f"{self.__class__.__name__}(" f"store={self._store!r}, " diff --git a/tests/test_experimental/test_cache_store.py b/tests/test_experimental/test_cache_store.py index e60f05a68c..782302f406 100644 --- a/tests/test_experimental/test_cache_store.py +++ b/tests/test_experimental/test_cache_store.py @@ -10,7 +10,7 @@ from zarr.abc.store import RangeByteRequest, Store, SuffixByteRequest from zarr.core.buffer.core import default_buffer_prototype from zarr.core.buffer.cpu import Buffer as CPUBuffer -from zarr.experimental.cache_store import CacheStore, _Entry +from zarr.experimental.cache_store import CacheStore, _Entry, _KeyState from zarr.storage import MemoryStore @@ -235,8 +235,8 @@ async def test_cache_returns_cached_data_for_performance( # Put data in cache but not source (simulates orphaned cache entry) test_data = CPUBuffer.from_bytes(b"orphaned data") await cached_store._cache.set("orphan_key", test_data) - cached_store._state.entries["orphan_key"] = _Entry( - insert_time=time.monotonic(), size=len(test_data), present=True + cached_store._state.entries["orphan_key"] = _KeyState( + full=_Entry(insert_time=time.monotonic(), size=len(test_data), present=True) ) # Cache should return data for performance (no source verification) @@ -402,7 +402,9 @@ async def test_max_age_numeric(self) -> None: assert cached_store._is_fresh("test_key") # Manually set old timestamp to test expiration - cached_store._state.entries["test_key"].insert_time = time.monotonic() - 2 # 2 seconds ago + slot = cached_store._state.entries["test_key"].full + assert slot is not None + slot.insert_time = time.monotonic() - 2 # 2 seconds ago # Key should now be stale assert not cached_store._is_fresh("test_key") @@ -544,8 +546,8 @@ async def test_unlimited_cache_size(self) -> None: assert info["cached_keys"] == 10 assert info["current_size"] == 10000 # 10 * 1000 bytes - async def test_evict_key_exception_handling(self) -> None: - """Test exception handling in _evict_key method.""" + async def test_evict_slot_exception_handling(self) -> None: + """Test exception handling in _evict_slot method.""" source_store = MemoryStore() cache_store = MemoryStore() cached_store = CacheStore(source_store, cache_store=cache_store, max_size=100) @@ -558,8 +560,8 @@ async def test_evict_key_exception_handling(self) -> None: # Remove the tracked entry while leaving the cached value behind del cached_store._state.entries["test_key"] - # Try to evict - should handle the KeyError gracefully - await cached_store._evict_key("test_key") + # Try to evict - should handle the missing record gracefully + await cached_store._evict_slot("test_key") # Should still work and not crash info = cached_store.cache_info() @@ -585,8 +587,8 @@ async def test_get_no_cache_delete_tracking(self) -> None: # Should have cleaned up tracking (the positive entry is gone). With # cache_missing on by default, a negative marker replaces it. - entry = cached_store._state.entries.get("phantom_key") - assert entry is None or not entry.present + state = cached_store._state.entries.get("phantom_key") + assert state is None or (state.full is not None and not state.full.present) async def test_accommodate_value_no_max_size(self) -> None: """Test _accommodate_value early return when max_size is None.""" @@ -646,7 +648,7 @@ async def set_large(key: str) -> None: # Size should be consistent with tracked keys assert info["current_size"] <= 200 # Might pass # But verify actual cache store size matches tracking - total_size = sum(entry.size for entry in cached_store._state.entries.values()) + total_size = sum(state.tracked_size for state in cached_store._state.entries.values()) assert total_size == info["current_size"] # WOULD FAIL async def test_concurrent_get_and_evict(self) -> None: @@ -677,7 +679,7 @@ async def write_key() -> None: assert info["current_size"] <= 100 # Tracked size accounting stays consistent with all entries (present # values plus any negative markers, which each carry a flat overhead). - total_size = sum(entry.size for entry in cached_store._state.entries.values()) + total_size = sum(state.tracked_size for state in cached_store._state.entries.values()) assert total_size == info["current_size"] async def test_eviction_actually_deletes_from_cache_store(self) -> None: @@ -703,7 +705,7 @@ async def test_eviction_actually_deletes_from_cache_store(self) -> None: # CRITICAL: key1 should also be removed from cache_store assert not await cache_store.exists("key1"), ( - "Evicted key still exists in cache_store! _evict_key doesn't actually delete." + "Evicted key still exists in cache_store! _evict_slot doesn't actually delete." ) # But key1 should still exist in source store @@ -771,13 +773,13 @@ async def test_all_tracked_keys_exist_in_cache_store(self) -> None: data = CPUBuffer.from_bytes(b"x" * 50) await cached_store.set(f"key_{i}", data) - # Every present str key in tracking should exist in cache_store. - # (tuple keys are byte-range entries stored in-memory, not in the Store; - # absent entries are negative markers with no stored value.) - for entry_key, entry in cached_store._state.entries.items(): - if isinstance(entry_key, str) and entry.present: - assert await cache_store.exists(entry_key), ( - f"Key '{entry_key}' is tracked but doesn't exist in cache_store" + # Every key tracking a present full value should exist in cache_store. + # (Byte-range entries are stored in-memory inside the record, not in the + # Store; absent markers have no stored value.) + for key, state in cached_store._state.entries.items(): + if state.full is not None and state.full.present: + assert await cache_store.exists(key), ( + f"Key '{key}' is tracked but doesn't exist in cache_store" ) # Additional coverage tests for 100% coverage @@ -794,10 +796,10 @@ async def test_cache_store_requires_delete_support(self) -> None: with pytest.raises(ValueError, match="does not support deletes"): CacheStore(store=source_store, cache_store=cache_store) - async def test_evict_key_exception_handling_with_real_error( + async def test_evict_slot_exception_handling_with_real_error( self, monkeypatch: pytest.MonkeyPatch ) -> None: - """Test _evict_key exception handling when deletion fails.""" + """Test _evict_slot exception handling when deletion fails.""" source_store = MemoryStore() cache_store = MemoryStore() cached_store = CacheStore(store=source_store, cache_store=cache_store, max_size=100) @@ -815,7 +817,7 @@ async def failing_delete(key: str) -> None: # Attempt to evict should raise the exception with pytest.raises(RuntimeError, match="Simulated cache deletion failure"): async with cached_store._state.lock: - await cached_store._evict_key("test_key") + await cached_store._evict_slot("test_key") async def test_cache_stats_method(self) -> None: """Test cache_stats method returns correct statistics.""" @@ -994,15 +996,14 @@ async def test_set_invalidates_cached_byte_ranges(self) -> None: assert r1 is not None assert r1.to_bytes() == b"old" - # Byte-range entry should be in range_cache - assert ("key", RangeByteRequest(0, 3)) in cached_store._state.entries + # Byte-range entry should be tracked inside the key's record + assert RangeByteRequest(0, 3) in cached_store._state.entries["key"].ranges # Overwrite via set() — range entries must be invalidated await cached_store.set("key", CPUBuffer.from_bytes(b"NEW DATA!!")) - # The old range entry should be gone from tracking and range_cache - assert ("key", RangeByteRequest(0, 3)) not in cached_store._state.entries - assert "key" not in cached_store._state.range_cache + # The old range entry should be gone from the key's record + assert not cached_store._state.entries["key"].ranges # A fresh byte-range read should return the new data r2 = await cached_store.get("key", proto, byte_range=RangeByteRequest(0, 3)) @@ -1023,13 +1024,12 @@ async def test_delete_invalidates_cached_byte_ranges(self) -> None: assert r is not None assert r.to_bytes() == b"hello" - assert ("key", RangeByteRequest(0, 5)) in cached_store._state.entries + assert RangeByteRequest(0, 5) in cached_store._state.entries["key"].ranges - # Delete the key — range entries must be cleaned up + # Delete the key — the whole record must be cleaned up await cached_store.delete("key") - assert ("key", RangeByteRequest(0, 5)) not in cached_store._state.entries - assert "key" not in cached_store._state.range_cache + assert "key" not in cached_store._state.entries # Key is gone from source result = await cached_store.get("key", proto) @@ -1223,7 +1223,9 @@ async def test_absent_evicted_before_present(self) -> None: info = cs.cache_info() assert info["cached_keys"] == 1 assert "present" in cs._state.entries - assert cs._state.entries["present"].present + present_slot = cs._state.entries["present"].full + assert present_slot is not None + assert present_slot.present # Markers fill only the room left over by the cached value (2 here), proving # both that misses were actually recorded and that they were bounded. assert info["missing_keys"] == 2 @@ -1250,7 +1252,7 @@ async def test_no_size_leak_on_miss_then_write(self) -> None: # Only the value's bytes remain — no leftover marker overhead. assert info["current_size"] == len(value) # And the invariant holds: current_size == sum of all tracked entry sizes. - total = sum(entry.size for entry in cs._state.entries.values()) + total = sum(state.tracked_size for state in cs._state.entries.values()) assert total == info["current_size"] async def test_no_size_leak_on_miss_then_set_if_not_exists(self) -> None: @@ -1265,7 +1267,7 @@ async def test_no_size_leak_on_miss_then_set_if_not_exists(self) -> None: info = cs.cache_info() assert info["missing_keys"] == 0 - total = sum(entry.size for entry in cs._state.entries.values()) + total = sum(state.tracked_size for state in cs._state.entries.values()) assert total == info["current_size"] async def test_stale_cached_value_becomes_negative_entry(self) -> None: @@ -1283,7 +1285,9 @@ async def test_stale_cached_value_becomes_negative_entry(self) -> None: assert cs.cache_info()["current_size"] == len(value) await source.delete("k") # Force the cached entry stale so the next read consults the (now empty) source. - cs._state.entries["k"].insert_time = time.monotonic() - 10 + stale_slot = cs._state.entries["k"].full + assert stale_slot is not None + stale_slot.insert_time = time.monotonic() - 10 assert await cs.get("k", proto) is None # source absent -> records a miss info = cs.cache_info() @@ -1359,7 +1363,9 @@ async def test_upgrade_marker_to_value_under_pressure_evicts_other_entry(self) - # "k" now exists in the source with a 200 B value; force the marker stale so the # next read fetches it and upgrades the slot to a present value (needs eviction). await source.set("k", CPUBuffer.from_bytes(b"k" * 200)) - cs._state.entries["k"].insert_time = time.monotonic() - 5000 + marker_slot = cs._state.entries["k"].full + assert marker_slot is not None + marker_slot.insert_time = time.monotonic() - 5000 result = await cs.get("k", proto) assert result is not None @@ -1373,7 +1379,7 @@ async def test_upgrade_marker_to_value_under_pressure_evicts_other_entry(self) - assert not await cs._cache.exists("a") assert info["current_size"] == 200 assert info["current_size"] <= cs.max_size - total = sum(entry.size for entry in cs._state.entries.values()) + total = sum(state.tracked_size for state in cs._state.entries.values()) assert total == info["current_size"] async def test_set_if_not_exists_invalidates_stale_byte_range(self) -> None: @@ -1387,14 +1393,14 @@ async def test_set_if_not_exists_invalidates_stale_byte_range(self) -> None: r1 = await cs.get("k", proto, byte_range=RangeByteRequest(0, 3)) assert r1 is not None assert r1.to_bytes() == b"old" - assert ("k", RangeByteRequest(0, 3)) in cs._state.entries + assert RangeByteRequest(0, 3) in cs._state.entries["k"].ranges # Source key removed out-of-band, then re-created via set_if_not_exists. await source.delete("k") await cs.set_if_not_exists("k", CPUBuffer.from_bytes(b"NEW data!!")) # The stale byte-range entry must be gone, and a fresh read returns new bytes. - assert ("k", RangeByteRequest(0, 3)) not in cs._state.entries + assert "k" not in cs._state.entries r2 = await cs.get("k", proto, byte_range=RangeByteRequest(0, 3)) assert r2 is not None assert r2.to_bytes() == b"NEW" @@ -1432,17 +1438,18 @@ async def test_concurrent_write_not_shadowed_by_stale_miss(self) -> None: cs = CacheStore(source, cache_store=MemoryStore(), cache_missing=True) proto = default_buffer_prototype() - # No entry exists when the (stale) fetch begins... - prior_entry = cs._state.entries.get("k") - assert prior_entry is None + # No slot exists when the (stale) fetch begins... + prior_slot = cs._prior_full_slot("k") + assert prior_slot is None # ...a concurrent set() completes during the fetch... await cs.set("k", CPUBuffer.from_bytes(b"value")) # ...then the stale absent observation lands. - await cs._cache_miss("k", None, None, prior_entry) + await cs._cache_miss("k", None, None, prior_slot) - entry = cs._state.entries.get("k") - assert entry is not None - assert entry.present + state = cs._state.entries.get("k") + assert state is not None + assert state.full is not None + assert state.full.present assert cs.cache_info()["missing_keys"] == 0 result = await cs.get("k", proto) assert result is not None @@ -1505,16 +1512,112 @@ async def test_full_key_miss_invalidates_byte_ranges(self) -> None: r1 = await cs.get("k", proto, byte_range=RangeByteRequest(0, 3)) assert r1 is not None assert r1.to_bytes() == b"old" - assert ("k", RangeByteRequest(0, 3)) in cs._state.entries + assert RangeByteRequest(0, 3) in cs._state.entries["k"].ranges # Key removed out-of-band; a full-key read observes the absence. await source.delete("k") assert await cs.get("k", proto) is None - # The stale byte-range entry is gone, and a fresh range read sees the absence. - assert ("k", RangeByteRequest(0, 3)) not in cs._state.entries + # The stale byte-range entry is gone (the marker record replaced it), and a + # fresh range read sees the absence. + assert not cs._state.entries["k"].ranges assert await cs.get("k", proto, byte_range=RangeByteRequest(0, 3)) is None + async def test_byte_range_success_clears_negative_marker(self) -> None: + """A successful byte-range read proves the key exists, so it must clear the + key's negative marker: the same instance cannot answer differently for the + same key depending on how it is asked (mirror image of the full-key-miss + range invalidation).""" + source = MemoryStore() + cs = CacheStore(source, cache_store=MemoryStore()) + proto = default_buffer_prototype() + + assert await cs.get("k", proto) is None # marker recorded + assert cs.cache_info()["missing_keys"] == 1 + + # Out-of-band write to the source, then a ranged read observes bytes. + await source.set("k", CPUBuffer.from_bytes(b"HELLO WORLD")) + r = await cs.get("k", proto, byte_range=RangeByteRequest(0, 5)) + assert r is not None + assert r.to_bytes() == b"HELLO" + + # The marker is gone and the counts are per key — no marker+range double + # count (previously: tracked_keys=2, missing_keys=1 for one key). + info = cs.cache_info() + assert info["missing_keys"] == 0 + assert info["tracked_keys"] == 1 + + # The full-key read must not serve the stale marker. + full = await cs.get("k", proto) + assert full is not None + assert full.to_bytes() == b"HELLO WORLD" + + async def test_oversized_byte_range_still_clears_negative_marker(self) -> None: + """Observing range bytes clears the marker even when the range itself is + too large to cache.""" + from zarr.experimental.cache_store import _NEGATIVE_ENTRY_SIZE + + source = MemoryStore() + # Budget fits a marker but not the 250-byte range value. + cs = CacheStore(source, cache_store=MemoryStore(), max_size=2 * _NEGATIVE_ENTRY_SIZE) + proto = default_buffer_prototype() + + assert await cs.get("k", proto) is None + assert cs.cache_info()["missing_keys"] == 1 + + await source.set("k", CPUBuffer.from_bytes(b"x" * 300)) + r = await cs.get("k", proto, byte_range=RangeByteRequest(0, 250)) + assert r is not None + assert len(r) == 250 + + # The range was not cached (too large), but the stale marker is gone. + assert cs.cache_info()["missing_keys"] == 0 + full = await cs.get("k", proto) + assert full is not None + assert len(full) == 300 + + async def test_range_entries_evicted_lru_within_key(self) -> None: + """Per-range recency lives inside the key's record: under pressure the + least-recently-used range of the eviction-candidate key is dropped first.""" + source = MemoryStore() + cs = CacheStore(source, cache_store=MemoryStore(), max_size=100) + proto = default_buffer_prototype() + + await source.set("k", CPUBuffer.from_bytes(b"x" * 120)) + r1 = RangeByteRequest(0, 40) + r2 = RangeByteRequest(40, 80) + r3 = RangeByteRequest(80, 120) + assert await cs.get("k", proto, byte_range=r1) is not None + assert await cs.get("k", proto, byte_range=r2) is not None + # Touch r1 so r2 becomes the key's least-recently-used slot. + assert await cs.get("k", proto, byte_range=r1) is not None + assert cs.cache_stats()["hits"] == 1 + + # Caching r3 (40 more bytes) exceeds the 100-byte budget: r2 must go. + assert await cs.get("k", proto, byte_range=r3) is not None + ranges = cs._state.entries["k"].ranges + assert r1 in ranges + assert r2 not in ranges + assert r3 in ranges + assert cs._state.current_size <= 100 + + async def test_eviction_candidate_prefers_markers_else_lru_key(self) -> None: + """Candidate selection: with no markers the LRU key is returned (O(1) fast + path); with markers present, the LRU marker wins over an older cached value.""" + cs = CacheStore(MemoryStore(), cache_store=MemoryStore(), max_size=10_000) + proto = default_buffer_prototype() + + await cs.set("a", CPUBuffer.from_bytes(b"aa")) + await cs.set("b", CPUBuffer.from_bytes(b"bb")) + # Touch "a" so "b" is the LRU key. + assert await cs.get("a", proto) is not None + assert cs._next_eviction_candidate() == "b" + + # A marker is preferred over any cached value, even a less recent one. + assert await cs.get("absent", proto) is None + assert cs._state.negative_count == 1 + assert cs._next_eviction_candidate() == "absent" + def test_cache_store_opts_out_of_sync_io() -> None: """`CacheStore` must not advertise sync IO capability. From 8f79e75f0135b14c4c21c653a9505202617ad925 Mon Sep 17 00:00:00 2001 From: Shane Grigsby Date: Tue, 11 Aug 2026 16:03:38 -0700 Subject: [PATCH 12/15] O(1) eviction candidate when no absent markers exist (kills linear scan on the plain positive path) --- src/zarr/experimental/cache_store.py | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/src/zarr/experimental/cache_store.py b/src/zarr/experimental/cache_store.py index 2483e7741e..ac3df703fb 100644 --- a/src/zarr/experimental/cache_store.py +++ b/src/zarr/experimental/cache_store.py @@ -349,9 +349,9 @@ async def _record_missing(self, key: str) -> None: Charges a flat ``_NEGATIVE_ENTRY_SIZE`` against the shared ``max_size`` budget. A negative marker is strictly lower priority than cached data: it may only displace *other* (older) absent markers to fit, never a cached - value, and is skipped entirely if the budget is full of cached values. - Must be called while holding ``self._state.lock``. Staleness is bounded - by ``max_age_seconds``. + value, and is skipped entirely if the budget is full of cached values + (detected in O(1) via ``negative_count``). Must be called while holding + ``self._state.lock``. Staleness is bounded by ``max_age_seconds``. """ old = self._state.entries.pop(key, None) if old is not None: @@ -361,9 +361,11 @@ async def _record_missing(self, key: str) -> None: if self.max_size is not None: # Make room by evicting older absent markers only — never cached values. while self._state.current_size + _NEGATIVE_ENTRY_SIZE > self.max_size: - lru_absent = self._lru_absent_key() - if lru_absent is None: + if self._state.negative_count == 0: return # only cached values fill the budget — don't record the miss + lru_absent = self._lru_absent_key() + if lru_absent is None: # pragma: no cover - count implies one exists + return await self._evict_slot(lru_absent) else: # No byte budget to share: bound the marker *count* instead, so a scan @@ -435,12 +437,15 @@ def _lru_absent_key(self) -> str | None: def _next_eviction_candidate(self) -> str | None: """Return the key to evict from next, preferring absent markers (LRU-first). - The entries are walked in LRU order: the first key marked absent is - returned; if none are, the least-recently-used key is the candidate. - Must be called while holding self._state.lock. + When no absent markers exist (``negative_count == 0``) the least-recently- + used key is the candidate in O(1) — the common all-positive case never scans. + Otherwise the entries are walked in LRU order for the first marker. Must be + called while holding self._state.lock. """ + if self._state.negative_count == 0: + return next(iter(self._state.entries), None) candidate = self._lru_absent_key() - if candidate is None: + if candidate is None: # pragma: no cover - negative_count > 0 implies one exists return next(iter(self._state.entries), None) return candidate From 58a83655d6d57e576020dcc8581fbe9497f24c95 Mon Sep 17 00:00:00 2001 From: Shane Grigsby Date: Tue, 11 Aug 2026 16:41:01 -0700 Subject: [PATCH 13/15] one generation per key record: serve byte ranges by slicing a cached value, supersede on every source observation --- changes/4042.feature.md | 2 + src/zarr/experimental/cache_store.py | 438 ++++++++++++-------- tests/test_experimental/test_cache_store.py | 308 +++++++++++++- 3 files changed, 565 insertions(+), 183 deletions(-) diff --git a/changes/4042.feature.md b/changes/4042.feature.md index 3ebeed4222..8fc3173e5a 100644 --- a/changes/4042.feature.md +++ b/changes/4042.feature.md @@ -1 +1,3 @@ `zarr.experimental.cache_store.CacheStore` now performs negative caching by default (`cache_missing=True`, opt-out). A full-key read that finds the key absent in the source store is remembered, so repeat reads of that absent key return immediately without a source round-trip — useful for sparse arrays where most chunks resolve to the fill value. Remembered misses respect `max_age_seconds` and are dropped when the key is written via `set`/`set_if_not_exists` or when a byte-range read observes data for the key. Negative-cache activity is reported as `negative_hits` in `cache_stats()` and `missing_keys` in `cache_info()`. Only full-key reads are affected (not byte-range reads or `exists`). Pass `cache_missing=False` to restore the previous behavior. The default `max_age_seconds` is now finite (300 seconds) so both cached values and remembered misses are re-validated against the source at bounded staleness; pass `"infinity"` to opt out. Negative markers share the `max_size` byte budget with cached values (each charged a small flat overhead, evicted marker-first), and when `max_size` is `None` the marker count is capped at an internal limit (100,000, least-recently-used evicted first), so scans over very large sparse key spaces stay bounded. + +Everything the cache knows about a key now lives in one record, mutated under one lock, and each source observation for the key replaces that whole record. So a cached full value also answers byte-range reads of the key — served from the cache store rather than the source — and a byte-range read that does reach the source supersedes any older value cached for the key; full-key and byte-range reads through the same `CacheStore` therefore always answer from the same observation. diff --git a/src/zarr/experimental/cache_store.py b/src/zarr/experimental/cache_store.py index ac3df703fb..6def82d0c3 100644 --- a/src/zarr/experimental/cache_store.py +++ b/src/zarr/experimental/cache_store.py @@ -73,21 +73,29 @@ class _KeyState: ``None`` when nothing is known about the key as a whole. ``ranges`` holds the key's cached byte-range reads. - Because everything known about a key lives in this one record, every - mutation for the key goes through one slot under one lock, and the - invariant *absent implies no ranges* is asserted at the mutation points - (``assert_coherent``) instead of being maintained across parallel - structures: a key can never simultaneously be marked missing and hold - cached bytes, so full-key and byte-range answers cannot diverge. + Because everything known about a key lives in this one record, every source + observation for the key — bytes or absence, full-key or ranged — replaces + the whole record in one locked mutation, so the record only ever holds + knowledge from a single source generation. The two halves are therefore + *mutually exclusive*: a record holds a full slot **or** byte ranges, never + both (``assert_coherent``). A cached full value answers ranged reads by + slicing, so keeping ranges alongside it would be redundant as well as + divergence-prone, and an absent marker can never sit next to cached bytes. """ full: _Entry | None = None ranges: dict[ByteRequest, _RangeEntry] = field(default_factory=dict) def assert_coherent(self) -> None: - """Assert the record invariant: a key marked absent holds no range data.""" - assert self.full is None or self.full.present or not self.ranges, ( - "cache incoherent: key marked absent still holds byte-range data" + """Assert the record invariant: full slot and byte ranges are exclusive. + + Called at the end of the locked sections that fill either half, where it + fires if that half was recorded without superseding the other. Note that + ``python -O`` strips it, so it is a development check, not a guarantee; + ``CacheStore._assert_invariants`` sweeps the whole state for tests. + """ + assert self.full is None or not self.ranges, ( + "cache incoherent: key holds a full slot and byte-range data at once" ) @property @@ -128,10 +136,16 @@ class CacheStore(WrapperStore[Store]): as the cache backend. This provides persistent caching capabilities with time-based expiration, size-based eviction, and flexible cache storage options. - Full-key reads are cached in the Store-backed cache. Byte-range reads are - cached in memory, inside the key's tracking record, so that partial reads - never pollute the filesystem (or other persistent backend). Both share - the same ``max_size`` budget and LRU eviction policy. + Full-key reads are cached in the Store-backed cache. A cached full value also + answers byte-range reads of that key, by asking the cache store for the range + instead of the source. Ranges of a key with no cached value are cached in + memory, inside the key's tracking record, so that partial reads never pollute + the filesystem (or other persistent backend). + + Both halves share the same ``max_size`` budget, and eviction is LRU over + *keys*: the least-recently-used key is chosen first (absent markers before + cached data), then its least-recently-used slot within the record. Touching + any of a key's slots therefore keeps the rest of that key resident. Parameters ---------- @@ -261,13 +275,21 @@ def with_read_only(self, read_only: bool = False) -> Self: store._state = self._state return store + def _slot_is_fresh(self, slot: _Entry | _RangeEntry) -> bool: + """Check whether an already-looked-up slot is still within ``max_age_seconds``. + + Uses monotonic time for accurate elapsed time measurement. + """ + if self.max_age_seconds == "infinity": + return True + return time.monotonic() - slot.insert_time < self.max_age_seconds + def _is_fresh(self, key: str, byte_range: ByteRequest | None = None) -> bool: """Check if a tracked slot (full-key, or one byte range) is still fresh. - Uses monotonic time for accurate elapsed time measurement. A key with no - tracked slot for the request is treated as not fresh (except under an - infinite TTL, matching the previous behaviour of routing unseen keys - through the cache path). + A key with no tracked slot for the request is treated as not fresh (except + under an infinite TTL, matching the previous behaviour of routing unseen + keys through the cache path). """ if self.max_age_seconds == "infinity": return True @@ -276,10 +298,21 @@ def _is_fresh(self, key: str, byte_range: ByteRequest | None = None) -> bool: return False slot: _Entry | _RangeEntry | None slot = state.full if byte_range is None else state.ranges.get(byte_range) - if slot is None: - return False - elapsed = time.monotonic() - slot.insert_time - return elapsed < self.max_age_seconds + return slot is not None and self._slot_is_fresh(slot) + + def _has_fresh_value(self, key: str) -> bool: + """Is a fresh full value for *key* cached (as opposed to absent/unknown)? + + Such a value is the newest thing known about the key, and can serve any + byte range of it by slicing — see ``_get_try_cache``. + """ + state = self._state.entries.get(key) + return ( + state is not None + and state.full is not None + and state.full.present + and self._slot_is_fresh(state.full) + ) # ------------------------------------------------------------------ # tracking-state mutation helpers (all require ``self._state.lock``) @@ -305,18 +338,6 @@ def _reclaim_ranges(self, state: _KeyState) -> None: self._state.current_size = max(0, self._state.current_size - range_entry.size) state.ranges.clear() - def _drop_full_slot(self, key: str) -> None: - """Remove *key*'s full slot (value or marker) from tracking. - - Byte-range entries for the key are left in place. Must be called while - holding ``self._state.lock``. - """ - state = self._state.entries.get(key) - if state is not None: - self._reclaim_full(state) - if state.is_empty: - del self._state.entries[key] - def _drop_key(self, key: str) -> None: """Remove everything tracked for *key* (full slot and byte ranges). @@ -327,24 +348,13 @@ def _drop_key(self, key: str) -> None: self._reclaim_full(state) self._reclaim_ranges(state) - def _invalidate_range_entries(self, key: str) -> None: - """Drop *key*'s cached byte-range entries (the source data changed). - - Must be called while holding ``self._state.lock``. - """ - state = self._state.entries.get(key) - if state is not None: - self._reclaim_ranges(state) - if state.is_empty: - del self._state.entries[key] - async def _record_missing(self, key: str) -> None: """Record *key* as known-missing (absent in the source store). Replaces the key's whole record: any previously cached value bytes and - byte-range buffers are reclaimed in the same mutation, so the invariant - *absent implies no ranges* holds by construction. The caller has already - removed any backing-store value for *key*. + byte-range buffers are reclaimed in the same mutation, so the record holds + only this observation. The caller has already removed any backing-store + value for *key*. Charges a flat ``_NEGATIVE_ENTRY_SIZE`` against the shared ``max_size`` budget. A negative marker is strictly lower priority than cached data: it @@ -353,10 +363,7 @@ async def _record_missing(self, key: str) -> None: (detected in O(1) via ``negative_count``). Must be called while holding ``self._state.lock``. Staleness is bounded by ``max_age_seconds``. """ - old = self._state.entries.pop(key, None) - if old is not None: - self._reclaim_full(old) - self._reclaim_ranges(old) + self._drop_key(key) if self.max_size is not None: # Make room by evicting older absent markers only — never cached values. @@ -386,20 +393,6 @@ async def _record_missing(self, key: str) -> None: self._state.current_size += _NEGATIVE_ENTRY_SIZE self._state.negative_count += 1 - def _evict_missing(self, key: str) -> None: - """Drop any negative entry for *key* (it is now present or being written). - - Only removes an *absent* slot — a present (cached) value for the same key is - left untouched — and reclaims the marker's charged bytes. Must be called - while holding ``self._state.lock``. - """ - state = self._state.entries.get(key) - if state is not None and state.full is not None and not state.full.present: - state.assert_coherent() - self._reclaim_full(state) - if state.is_empty: - del self._state.entries[key] - async def _accommodate_value(self, value_size: int) -> None: """Evict until ``value_size`` more bytes fit within ``max_size``. @@ -490,45 +483,48 @@ async def _evict_slot(self, key: str) -> None: async def _track_entry(self, key: str, value: Buffer) -> bool: """Register a full-key value in the shared size / LRU tracking. - Returns ``True`` if the entry was tracked, ``False`` if the value - exceeds ``max_size`` and was skipped. Callers should roll back any - data they already stored when this returns ``False``. + Returns ``True`` if the entry was tracked, ``False`` if the value exceeds + ``max_size`` and was skipped (the key's previous state is dropped either + way). Callers should roll back any data they already stored when this + returns ``False``. - This method holds the lock for the entire operation to ensure atomicity. + Must be called while holding ``self._state.lock``, which the callers hold + across the backing-store write as well, so the value and its record are + published together. """ value_size = len(value) - # Check if value exceeds max size + # Drop everything previously known about the key, reclaiming its bytes. + # The caller observed the key's full value at the source, which supersedes + # both the old full slot and any byte ranges cached from an older + # generation (once the new value is tracked, ranged reads are served by + # slicing it). Dropping the old full slot first also removes it from the + # eviction candidates so ``_accommodate_value`` cannot select the very slot + # being (re)tracked — which would double-subtract its size, stop the + # eviction loop early, and (for a present overwrite) delete the value the + # caller just wrote to the backing store. The caller has already written + # the new value, so the backing store is not touched here. + state = self._state.entries.get(key) + if state is not None: + self._reclaim_full(state) + self._reclaim_ranges(state) + del self._state.entries[key] + + # The observation is recorded even when the value itself is too large to + # cache, so the checks above are not skipped by an early return. if self.max_size is not None and value_size > self.max_size: return False - async with self._state.lock: - # Drop the key's existing full slot first, reclaiming its bytes. This - # removes the slot from the eviction candidates so ``_accommodate_value`` - # cannot select the very slot being (re)tracked — which would - # double-subtract its size, stop the eviction loop early, and (for a - # present overwrite) delete the value the caller just wrote to the - # backing store. The key's *other* slots (byte-range entries) remain - # fair game for eviction. The caller has already written the new value, - # so the backing store is not touched here. - state = self._state.entries.get(key) - if state is not None: - self._reclaim_full(state) - if state.is_empty: - del self._state.entries[key] - - # Make room for the new value, then track it (the record is appended - # or re-inserted as most-recently-used). - await self._accommodate_value(value_size) - state = self._state.entries.pop(key, None) - if state is None: - state = _KeyState() - now = time.monotonic() - state.full = _Entry(insert_time=now, size=value_size, present=True, last_used=now) - state.assert_coherent() - self._state.entries[key] = state - self._state.current_size += value_size - + # Make room for the new value, then track it (the record is appended + # as most-recently-used). + await self._accommodate_value(value_size) + now = time.monotonic() + state = _KeyState( + full=_Entry(insert_time=now, size=value_size, present=True, last_used=now) + ) + state.assert_coherent() + self._state.entries[key] = state + self._state.current_size += value_size return True async def _track_range(self, key: str, byte_range: ByteRequest, value: Buffer) -> bool: @@ -536,42 +532,50 @@ async def _track_range(self, key: str, byte_range: ByteRequest, value: Buffer) - Returns ``True`` if the range was cached, ``False`` if the value exceeds ``max_size`` (nothing is stored in that case, so there is nothing to roll - back). Observing bytes for a key proves it exists in the source, so any - absent marker is dropped in the same locked mutation — this is the single - point where ranges are recorded, so a record can never hold both a marker - and range data (``assert_coherent``), even when the range itself is too - large to cache. + back). + + These bytes were just read from the source, so they supersede the key's + full slot: an absent marker is disproved by them, and a cached value can + only still be here if it was stale or unreadable (a fresh one would have + served this range by slicing instead of reaching the source). Either way + the full slot is dropped in the same locked mutation — deleting the + backing value with it, so nothing untracked is left for the cache path to + serve — and this is the single point where ranges are recorded, so a + record can never hold both a full slot and range data + (``assert_coherent``), even when the range itself is too large to cache. + + Must be called while holding ``self._state.lock``. """ value_size = len(value) - async with self._state.lock: - state = self._state.entries.get(key) - if state is not None: - if state.full is not None and not state.full.present: - # Bytes came back for this key: the absent marker is stale. - self._reclaim_full(state) - # Drop any slot being replaced so accommodation cannot select it. - old = state.ranges.pop(byte_range, None) - if old is not None: - self._state.current_size = max(0, self._state.current_size - old.size) - if state.is_empty: - del self._state.entries[key] - - if self.max_size is not None and value_size > self.max_size: - return False - - await self._accommodate_value(value_size) - state = self._state.entries.pop(key, None) - if state is None: - state = _KeyState() - now = time.monotonic() - state.ranges[byte_range] = _RangeEntry( - buffer=value, insert_time=now, size=value_size, last_used=now - ) - state.assert_coherent() - self._state.entries[key] = state - self._state.current_size += value_size + state = self._state.entries.get(key) + if state is not None: + if state.full is not None: + had_value = state.full.present + self._reclaim_full(state) + if had_value: + await self._cache.delete(key) + # Drop any slot being replaced so accommodation cannot select it. + old = state.ranges.pop(byte_range, None) + if old is not None: + self._state.current_size = max(0, self._state.current_size - old.size) + if state.is_empty: + del self._state.entries[key] + if self.max_size is not None and value_size > self.max_size: + return False + + await self._accommodate_value(value_size) + state = self._state.entries.pop(key, None) + if state is None: + state = _KeyState() + now = time.monotonic() + state.ranges[byte_range] = _RangeEntry( + buffer=value, insert_time=now, size=value_size, last_used=now + ) + state.assert_coherent() + self._state.entries[key] = state + self._state.current_size += value_size return True async def _update_access_order(self, key: str, byte_range: ByteRequest | None = None) -> None: @@ -605,17 +609,24 @@ async def _cache_miss( ) -> None: """Handle a cache miss by storing or cleaning up after a source-store fetch. - ``prior_slot`` is the key's full slot as observed just *before* the - source fetch began (``None`` if there was none). It guards the absent path + ``prior_slot`` is the key's full slot as observed just *before* the source + fetch began (``None`` if there was none). It guards both full-key paths against a write/miss race: if the key's slot now holds a *different* present - entry, a concurrent ``set`` completed during the fetch, and the stale - "absent" result must not shadow the new value. Identity (not timestamps) is - used so the check is immune to coarse clocks. Known blind spot: a - concurrent writer that leaves no present slot behind (``cache_set_data=False``, - or ``set_if_not_exists``, whose override drops tracking rather than inserting - a present slot) cannot be detected here, so its write may be shadowed by the - stale absent observation until the finite default ``max_age_seconds`` expires - the marker. + entry, a concurrent ``set`` completed during the fetch, so neither the stale + "absent" result nor the stale fetched bytes may overwrite the new value. + Identity (not timestamps) is used so the check is immune to coarse clocks, + and the guard, the backing-store write and the tracking mutation share one + locked section. Known blind spot: a concurrent mutation that leaves no present + slot behind cannot be detected here — ``cache_set_data=False``, + ``set_if_not_exists`` (whose override drops tracking rather than inserting a + present slot), a ``set`` whose value exceeds ``max_size`` (not cached, so no + slot), and ``delete`` (which removes the record, leaving no identity to + compare against) — so its effect may be shadowed by this older observation + until the finite default ``max_age_seconds`` expires the record. + + Byte-range fetches carry no such guard: they never write the backing store, + and both outcomes (bytes, or absence) only invalidate the key's record, so a + lost race costs at most one extra source round-trip. """ if result is None: if byte_range is None: @@ -638,30 +649,42 @@ async def _cache_miss( else: self._drop_key(key) else: + # A ranged read came back empty. That is evidence about the key + # itself, so the key's whole record — cached value included — is + # dropped: keeping it would let ``get(key)`` serve bytes for a key + # this same instance just reported nothing for. Deliberately *not* + # recorded as an absent marker: whether ``get(key, byte_range) is + # None`` implies the key is absent (rather than the range being + # unsatisfiable) is a store-contract question left to the follow-up + # interval work, and invalidating needs no answer to it — worst case + # it costs one extra source round-trip. async with self._state.lock: - state = self._state.entries.get(key) - if state is not None: - old = state.ranges.pop(byte_range, None) - if old is not None: - self._state.current_size = max(0, self._state.current_size - old.size) - if state.is_empty: - del self._state.entries[key] + self._drop_key(key) + await self._cache.delete(key) else: if byte_range is None: - await self._cache.set(key, result) - # ``_track_entry`` overwrites the key's full slot with a present - # entry, so any prior negative marker is structurally replaced — - # no separate negative-cache eviction is needed here. - tracked = await self._track_entry(key, result) - if not tracked: - # Value too large for the cache — roll back so the backing cache - # holds no untracked (uncounted, unevictable) orphan. - await self._cache.delete(key) + async with self._state.lock: + state = self._state.entries.get(key) + current = state.full if state is not None else None + if current is not None and current.present and current is not prior_slot: + # A concurrent write completed during this fetch — these + # bytes are from before it, so leave the newer value alone. + return + await self._cache.set(key, result) + # ``_track_entry`` replaces the key's whole record with a present + # entry, so any prior negative marker or stale byte range is + # structurally superseded in the same locked section. + if not await self._track_entry(key, result): + # Value too large for the cache — roll back so the backing + # cache holds no untracked (uncounted, unevictable) orphan. + await self._cache.delete(key) else: # ``_track_range`` stores the buffer inside the key's record and - # drops any stale absent marker in the same locked mutation, so a - # successful ranged read can never leave the key marked missing. - await self._track_range(key, byte_range, result) + # drops the key's full slot in the same locked mutation, so a + # successful ranged read can leave neither an absent marker nor a + # superseded value behind. + async with self._state.lock: + await self._track_range(key, byte_range, result) def _prior_full_slot(self, key: str) -> _Entry | None: """Snapshot the key's full slot for ``_cache_miss``'s write/miss race guard.""" @@ -679,6 +702,17 @@ async def _get_try_cache( self._state.hits += 1 await self._update_access_order(key) return maybe_cached + elif self._has_fresh_value(key): + # A fresh full value is cached: slice the range out of it rather than + # asking the source. Both answers then come from the one observation, + # so they cannot disagree, and the round-trip is saved outright. The + # Store-backed cache does the slicing, so the whole value is never + # materialised for a small range. + maybe_cached = await self._cache.get(key, prototype, byte_range) + if maybe_cached is not None: + self._state.hits += 1 + await self._update_access_order(key) + return maybe_cached else: # Byte-range read — served from the key's in-memory record state = self._state.entries.get(key) @@ -747,7 +781,7 @@ async def get( async with self._state.lock: state = self._state.entries.get(key) slot = state.full if state is not None else None - if slot is not None and not slot.present and self._is_fresh(key): + if slot is not None and not slot.present and self._slot_is_fresh(slot): self._state.negative_hits += 1 # Mark the marker most-recently-used so eviction stays LRU: # a frequently-probed absent key should outlive cold markers. @@ -755,10 +789,13 @@ async def get( self._state.entries.move_to_end(key) return None - if not self._is_fresh(key, byte_range): - return await self._get_no_cache(key, prototype, byte_range) - else: + # A ranged read also goes through the cache path when the key's *full* + # value is cached and fresh, because that value can serve any range of it. + if self._is_fresh(key, byte_range) or ( + byte_range is not None and self._has_fresh_value(key) + ): return await self._get_try_cache(key, prototype, byte_range) + return await self._get_no_cache(key, prototype, byte_range) async def set(self, key: str, value: Buffer) -> None: """ @@ -772,23 +809,22 @@ async def set(self, key: str, value: Buffer) -> None: The data to store """ await super().set(key, value) - # Invalidate all cached byte-range entries (source data changed) and drop any - # negative entry — the key now has a value. (No ``cache_missing`` gate here: - # a marker recorded before the flag was flipped off must still be cleared.) - async with self._state.lock: - self._invalidate_range_entries(key) - self._evict_missing(key) + # Either way the key's whole record is replaced in one locked section: the + # value just written supersedes any cached byte ranges, any stale value and + # any negative marker. (No ``cache_missing`` gate on the marker: one + # recorded before the flag was flipped off must still be cleared.) if self.cache_set_data: - await self._cache.set(key, value) - tracked = await self._track_entry(key, value) - if not tracked: - # Value too large for the cache — roll back so the backing cache - # holds no untracked (uncounted, unevictable) orphan. - await self._cache.delete(key) + async with self._state.lock: + await self._cache.set(key, value) + if not await self._track_entry(key, value): + # Value too large for the cache — roll back so the backing cache + # holds no untracked (uncounted, unevictable) orphan, and no + # record of the value it replaced survives either. + await self._cache.delete(key) else: - await self._cache.delete(key) async with self._state.lock: - self._drop_full_slot(key) + self._drop_key(key) + await self._cache.delete(key) async def set_if_not_exists(self, key: str, value: Buffer) -> None: """ @@ -871,9 +907,7 @@ def cache_stats(self) -> dict[str, Any]: async def clear_cache(self) -> None: """Clear all cached data and tracking information.""" - # Clear the cache store if it supports clear - if hasattr(self._cache, "clear"): - await self._cache.clear() + await self._cache.clear() # Reset tracking. Cumulative performance counters (hits/misses/evictions/ # negative_hits) are lifetime stats and intentionally survive a clear. @@ -882,6 +916,52 @@ async def clear_cache(self) -> None: self._state.current_size = 0 self._state.negative_count = 0 + async def _assert_invariants(self) -> None: + """Check the whole tracking state against the backing cache (test helper). + + ``_KeyState.assert_coherent`` only sees the record being mutated, so this + sweeps every record and cross-checks the incrementally maintained + aggregates — ``negative_count`` (which gates both the marker cap and the + O(1) eviction fast path) and ``current_size`` — against a recount, plus the + two cross-store facts the records claim: a cached value has a backing value + of the tracked length, and nothing untracked is left in the backing store. + It is O(number of tracked keys) and issues one backing read per cached + value, so it is for tests and debugging, not for the hot path. + """ + # Local import: only this debug helper needs a prototype of its own. + from zarr.core.buffer import default_buffer_prototype + + state = self._state + negative = sum( + 1 for s in state.entries.values() if s.full is not None and not s.full.present + ) + assert negative == state.negative_count, ( + f"negative_count drift: {state.negative_count} tracked, {negative} counted" + ) + size = sum(s.tracked_size for s in state.entries.values()) + assert size == state.current_size, ( + f"current_size drift: {state.current_size} tracked, {size} counted" + ) + assert self.max_size is None or size <= self.max_size, ( + f"cache over budget: {size} > {self.max_size}" + ) + for key, record in state.entries.items(): + record.assert_coherent() + assert not record.is_empty, f"empty record retained for {key}" + if record.full is not None and record.full.present: + cached = await self._cache.get(key, default_buffer_prototype()) + assert cached is not None, ( + f"{key} tracked as cached but absent from the cache store" + ) + assert len(cached) == record.full.size, ( + f"{key} tracked as {record.full.size} bytes, cache store holds {len(cached)}" + ) + async for cached_key in self._cache.list(): + tracked = state.entries.get(cached_key) + full = tracked.full if tracked is not None else None + assert full is not None, f"untracked value for {cached_key} left in the cache store" + assert full.present, f"{cached_key} marked absent but holds a value in the cache store" + def __repr__(self) -> str: """Return string representation of the cache store.""" cached_keys = len(self._state.entries) - self._state.negative_count diff --git a/tests/test_experimental/test_cache_store.py b/tests/test_experimental/test_cache_store.py index 782302f406..d5d90ef050 100644 --- a/tests/test_experimental/test_cache_store.py +++ b/tests/test_experimental/test_cache_store.py @@ -3,15 +3,17 @@ """ import asyncio +import random import time import pytest -from zarr.abc.store import RangeByteRequest, Store, SuffixByteRequest -from zarr.core.buffer.core import default_buffer_prototype +from zarr.abc.store import ByteRequest, RangeByteRequest, Store, SuffixByteRequest +from zarr.core.buffer.core import Buffer, BufferPrototype, default_buffer_prototype from zarr.core.buffer.cpu import Buffer as CPUBuffer -from zarr.experimental.cache_store import CacheStore, _Entry, _KeyState +from zarr.experimental.cache_store import CacheStore, _Entry, _KeyState, _RangeEntry from zarr.storage import MemoryStore +from zarr.storage._wrapper import WrapperStore class TestCacheStore: @@ -576,7 +578,8 @@ async def test_get_no_cache_delete_tracking(self) -> None: # First, add key to cache tracking but not to source test_data = CPUBuffer.from_bytes(b"test data") await cache_store.set("phantom_key", test_data) - await cached_store._track_entry("phantom_key", test_data) + async with cached_store._state.lock: + await cached_store._track_entry("phantom_key", test_data) # Verify it's in tracking assert "phantom_key" in cached_store._state.entries @@ -1619,6 +1622,303 @@ async def test_eviction_candidate_prefers_markers_else_lru_key(self) -> None: assert cs._next_eviction_candidate() == "absent" +class _GatedGetStore(WrapperStore[Store]): + """Source store whose ``get`` returns only once ``release`` is set. + + The value is read from the wrapped store *before* the wait, so the caller + observes the state as of when its fetch began — the shape of a read/write + race, made deterministic: ``fetched`` fires once the read has happened, the + test then mutates the store, and ``release`` lets the stale result land. + """ + + def __init__(self, store: Store) -> None: + super().__init__(store) + self.fetched = asyncio.Event() + self.release = asyncio.Event() + + async def get( + self, key: str, prototype: BufferPrototype, byte_range: ByteRequest | None = None + ) -> Buffer | None: + result = await self._store.get(key, prototype, byte_range) + self.fetched.set() + await self.release.wait() + return result + + +class TestCacheStoreRecordCoherence: + """One record per key, holding one source generation. + + Every source observation for a key — bytes or absence, full-key or ranged — + replaces the key's whole record, and a cached full value answers ranged reads + by slicing rather than by consulting the source. So the two read paths always + answer from the same observation. + """ + + async def test_ranged_read_served_from_cached_full_value(self) -> None: + """A fresh cached value serves byte ranges of itself, instead of reaching + the source and caching a second, newer generation alongside it.""" + source = MemoryStore() + cs = CacheStore(source, cache_store=MemoryStore()) + proto = default_buffer_prototype() + + await source.set("k", CPUBuffer.from_bytes(b"AAAAA")) + full = await cs.get("k", proto) + assert full is not None + assert full.to_bytes() == b"AAAAA" + + # Out-of-band overwrite: the cached value is now stale, but still fresh by + # TTL, so it — not the source — must answer both reads. + await source.set("k", CPUBuffer.from_bytes(b"BBBBB")) + ranged = await cs.get("k", proto, byte_range=RangeByteRequest(0, 3)) + again = await cs.get("k", proto) + assert ranged is not None + assert again is not None + assert again.to_bytes()[:3] == ranged.to_bytes() + assert ranged.to_bytes() == b"AAA" + + # Served from the cache, with no range entry (and no extra bytes) recorded. + assert cs.cache_stats()["hits"] == 2 + assert not cs._state.entries["k"].ranges + assert cs._state.current_size == 5 + await cs._assert_invariants() + + async def test_full_value_supersedes_stale_byte_ranges(self) -> None: + """Caching a full value drops the key's byte ranges: they were read from an + older generation, and the new value can serve those ranges itself.""" + source = MemoryStore() + cs = CacheStore(source, cache_store=MemoryStore()) + proto = default_buffer_prototype() + + await source.set("k", CPUBuffer.from_bytes(b"AAAAA")) + assert await cs.get("k", proto, byte_range=RangeByteRequest(0, 3)) is not None + assert RangeByteRequest(0, 3) in cs._state.entries["k"].ranges + + await source.set("k", CPUBuffer.from_bytes(b"BBBBB")) + full = await cs.get("k", proto) + assert full is not None + assert full.to_bytes() == b"BBBBB" + assert not cs._state.entries["k"].ranges + + ranged = await cs.get("k", proto, byte_range=RangeByteRequest(0, 3)) + assert ranged is not None + assert ranged.to_bytes() == b"BBB" + await cs._assert_invariants() + + @staticmethod + def _expire_value(cs: CacheStore, key: str) -> None: + """Age *key*'s cached value past ``max_age_seconds`` without sleeping.""" + slot = cs._state.entries[key].full + assert slot is not None + slot.insert_time = time.monotonic() - 5000 + + async def test_ranged_bytes_supersede_a_stale_cached_value(self) -> None: + """When a ranged read does reach the source (the cached value has expired), + the bytes it brings back replace that value rather than sitting next to it.""" + source = MemoryStore() + cs = CacheStore(source, cache_store=MemoryStore()) + proto = default_buffer_prototype() + + await source.set("k", CPUBuffer.from_bytes(b"AAAAA")) + assert await cs.get("k", proto) is not None + await source.set("k", CPUBuffer.from_bytes(b"BBBBB")) + self._expire_value(cs, "k") + + ranged = await cs.get("k", proto, byte_range=RangeByteRequest(0, 3)) + assert ranged is not None + assert ranged.to_bytes() == b"BBB" + # The stale value is gone from both the record and the backing cache, so no + # later read can serve it. + assert cs._state.entries["k"].full is None + assert await cs._cache.get("k", proto) is None + await cs._assert_invariants() + + async def test_ranged_miss_invalidates_the_key(self) -> None: + """A ranged read that comes back empty drops the key's record, so a full + read cannot serve bytes for a key this instance just reported nothing for. + The absence is deliberately not recorded as a marker.""" + source = MemoryStore() + cs = CacheStore(source, cache_store=MemoryStore()) + proto = default_buffer_prototype() + + await source.set("k", CPUBuffer.from_bytes(b"AAAAA")) + assert await cs.get("k", proto) is not None + await source.delete("k") + self._expire_value(cs, "k") # so the ranged read reaches the source + + assert await cs.get("k", proto, byte_range=RangeByteRequest(0, 3)) is None + assert "k" not in cs._state.entries + assert await cs._cache.get("k", proto) is None + assert cs.cache_info()["missing_keys"] == 0 # invalidated, not marked absent + assert await cs.get("k", proto) is None + await cs._assert_invariants() + + async def test_oversized_set_drops_the_previous_entry(self) -> None: + """An oversized write invalidates what it replaced: the earlier value is + untracked (not left charging the budget) as well as deleted.""" + cache = MemoryStore() + cs = CacheStore(MemoryStore(), cache_store=cache, max_size=200) + proto = default_buffer_prototype() + + await cs.set("k", CPUBuffer.from_bytes(b"a" * 50)) + assert cs._state.current_size == 50 + + await cs.set("k", CPUBuffer.from_bytes(b"b" * 500)) + assert cs._state.current_size == 0 + assert cs.cache_info()["cached_keys"] == 0 + assert await cache.get("k", proto) is None + await cs._assert_invariants() + + # The value is still readable, and the budget is fully available. + result = await cs.get("k", proto) + assert result is not None + assert len(result) == 500 + + async def test_oversized_read_drops_the_previous_entry(self) -> None: + """Mirror of the write path: an oversized fetch untracks the entry it + replaced instead of leaving a phantom charging the budget.""" + source = MemoryStore() + cache = MemoryStore() + cs = CacheStore(source, cache_store=cache, max_size=200) + proto = default_buffer_prototype() + + await source.set("k", CPUBuffer.from_bytes(b"a" * 50)) + assert await cs.get("k", proto) is not None + assert cs._state.current_size == 50 + + await source.set("k", CPUBuffer.from_bytes(b"b" * 500)) + self._expire_value(cs, "k") # so the read refetches + result = await cs.get("k", proto) + assert result is not None + assert len(result) == 500 + + assert cs._state.current_size == 0 + assert await cache.get("k", proto) is None + await cs._assert_invariants() + + async def test_concurrent_write_not_overwritten_by_stale_fill(self) -> None: + """A read that fetched before a concurrent ``set`` must not publish its + (older) bytes over the newly written value — the positive-path mirror of + ``test_concurrent_write_not_shadowed_by_stale_miss``.""" + source = MemoryStore() + gated = _GatedGetStore(source) + cs = CacheStore(gated, cache_store=MemoryStore()) + proto = default_buffer_prototype() + + await source.set("k", CPUBuffer.from_bytes(b"old")) + reader = asyncio.create_task(cs.get("k", proto)) + await gated.fetched.wait() # the fetch has read b"old" but not returned + + await cs.set("k", CPUBuffer.from_bytes(b"new")) + gated.release.set() + stale = await reader + assert stale is not None + assert stale.to_bytes() == b"old" # the caller still sees its own fetch + + # ...but the cache holds the newer write, and serves it. + cached = await cs._cache.get("k", proto) + assert cached is not None + assert cached.to_bytes() == b"new" + assert cs._state.current_size == 3 + await cs._assert_invariants() + + async def test_concurrent_write_not_shadowed_by_stale_miss_through_public_api(self) -> None: + """The absent-path guard, driven through ``get``/``set`` rather than by + calling ``_cache_miss`` directly, so it pins the behaviour and not the + implementation.""" + source = MemoryStore() + gated = _GatedGetStore(source) + cs = CacheStore(gated, cache_store=MemoryStore(), cache_missing=True) + proto = default_buffer_prototype() + + reader = asyncio.create_task(cs.get("k", proto)) + await gated.fetched.wait() # the fetch has observed the key absent + + await cs.set("k", CPUBuffer.from_bytes(b"value")) + gated.release.set() + assert await reader is None # the caller still sees its own fetch + + assert cs.cache_info()["missing_keys"] == 0 + result = await cs.get("k", proto) + assert result is not None + assert result.to_bytes() == b"value" + await cs._assert_invariants() + + async def test_negative_count_matches_the_records(self) -> None: + """``negative_count`` gates both the marker cap and the O(1) eviction fast + path, so check it against a recount rather than against itself.""" + cs = CacheStore(MemoryStore(), cache_store=MemoryStore(), max_size=1000) + proto = default_buffer_prototype() + + for i in range(20): + assert await cs.get(f"absent{i}", proto) is None + await cs.set(f"p{i}", CPUBuffer.from_bytes(b"x" * 30)) + assert await cs.get(f"p{i}", proto, byte_range=RangeByteRequest(0, 5)) is not None + await cs.delete(f"p{i}") + await cs.set_if_not_exists(f"q{i}", CPUBuffer.from_bytes(b"y" * 40)) + assert await cs.get(f"absent{i}", proto) is None + + counted = sum( + 1 for s in cs._state.entries.values() if s.full is not None and not s.full.present + ) + assert cs._state.negative_count == counted + assert cs.cache_info()["missing_keys"] == counted + await cs._assert_invariants() + + async def test_assert_coherent_rejects_a_mixed_record(self) -> None: + """The record invariant is checkable, not just documented: a record holding + a full slot *and* byte ranges fails it.""" + state = _KeyState(full=_Entry(insert_time=0.0, size=1)) + state.assert_coherent() # a full slot alone is fine + + state.ranges[RangeByteRequest(0, 1)] = _RangeEntry( + buffer=CPUBuffer.from_bytes(b"x"), insert_time=0.0, size=1, last_used=0.0 + ) + with pytest.raises(AssertionError, match="cache incoherent"): + state.assert_coherent() + + async def test_invariants_hold_under_randomised_operations(self) -> None: + """Seeded fuzz over the whole operation surface, checking the tracking state + against the backing cache after every step.""" + proto = default_buffer_prototype() + byte_ranges: list[ByteRequest | None] = [ + RangeByteRequest(0, 5), + RangeByteRequest(5, 10), + SuffixByteRequest(4), + None, + ] + max_sizes: list[int | None] = [None, 200, 600] + max_ages: list[int | str] = [300, "infinity"] + for seed in range(4): + rng = random.Random(seed) + source = MemoryStore() + cs = CacheStore( + source, + cache_store=MemoryStore(), + max_size=rng.choice(max_sizes), + max_age_seconds=rng.choice(max_ages), + cache_set_data=rng.choice([True, False]), + cache_missing=rng.choice([True, False]), + ) + for _ in range(120): + key = f"k{rng.randrange(6)}" + match rng.randrange(6): + case 0: + await cs.get(key, proto, byte_range=rng.choice(byte_ranges)) + case 1: + size = rng.choice([5, 50, 300]) + await cs.set(key, CPUBuffer.from_bytes(b"v" * size)) + case 2: + await cs.delete(key) + case 3: + await cs.set_if_not_exists(key, CPUBuffer.from_bytes(b"z" * 20)) + case 4: + # Out-of-band write: the source moves on without the cache. + await source.set(key, CPUBuffer.from_bytes(b"o" * 30)) + case _: + await cs.delete_dir("k") + await cs._assert_invariants() + + def test_cache_store_opts_out_of_sync_io() -> None: """`CacheStore` must not advertise sync IO capability. From 6bda7fd31145c9479c7d213f82749a71f8da7f7b Mon Sep 17 00:00:00 2001 From: Shane Grigsby Date: Tue, 11 Aug 2026 16:41:21 -0700 Subject: [PATCH 14/15] route get_ranges, get_partial_values, delete_dir and clear through the caching paths --- changes/4042.feature.md | 2 +- src/zarr/experimental/cache_store.py | 88 ++++++++++++++++++++- tests/test_experimental/test_cache_store.py | 80 ++++++++++++++++++- 3 files changed, 166 insertions(+), 4 deletions(-) diff --git a/changes/4042.feature.md b/changes/4042.feature.md index 8fc3173e5a..3fa12e3b27 100644 --- a/changes/4042.feature.md +++ b/changes/4042.feature.md @@ -1,3 +1,3 @@ `zarr.experimental.cache_store.CacheStore` now performs negative caching by default (`cache_missing=True`, opt-out). A full-key read that finds the key absent in the source store is remembered, so repeat reads of that absent key return immediately without a source round-trip — useful for sparse arrays where most chunks resolve to the fill value. Remembered misses respect `max_age_seconds` and are dropped when the key is written via `set`/`set_if_not_exists` or when a byte-range read observes data for the key. Negative-cache activity is reported as `negative_hits` in `cache_stats()` and `missing_keys` in `cache_info()`. Only full-key reads are affected (not byte-range reads or `exists`). Pass `cache_missing=False` to restore the previous behavior. The default `max_age_seconds` is now finite (300 seconds) so both cached values and remembered misses are re-validated against the source at bounded staleness; pass `"infinity"` to opt out. Negative markers share the `max_size` byte budget with cached values (each charged a small flat overhead, evicted marker-first), and when `max_size` is `None` the marker count is capped at an internal limit (100,000, least-recently-used evicted first), so scans over very large sparse key spaces stay bounded. -Everything the cache knows about a key now lives in one record, mutated under one lock, and each source observation for the key replaces that whole record. So a cached full value also answers byte-range reads of the key — served from the cache store rather than the source — and a byte-range read that does reach the source supersedes any older value cached for the key; full-key and byte-range reads through the same `CacheStore` therefore always answer from the same observation. +Everything the cache knows about a key now lives in one record, mutated under one lock, and each source observation for the key replaces that whole record. So a cached full value also answers byte-range reads of the key — served from the cache store rather than the source — and a byte-range read that does reach the source supersedes any older value cached for the key; full-key and byte-range reads through the same `CacheStore` therefore always answer from the same observation. `get_ranges`, `get_partial_values` and `delete_dir` are routed through the caching `get`/`delete` paths as well, so the sharded partial-read path and prefix deletions (e.g. `overwrite=True`) no longer read or delete around the cache. diff --git a/src/zarr/experimental/cache_store.py b/src/zarr/experimental/cache_store.py index 6def82d0c3..7dcbb1365a 100644 --- a/src/zarr/experimental/cache_store.py +++ b/src/zarr/experimental/cache_store.py @@ -8,11 +8,14 @@ from typing import TYPE_CHECKING, Any, Literal, Self from zarr.abc.store import ByteRequest, Store +from zarr.core.common import concurrent_map from zarr.storage._wrapper import WrapperStore logger = logging.getLogger(__name__) if TYPE_CHECKING: + from collections.abc import AsyncGenerator, AsyncIterator, Iterable, Sequence + from zarr.core.buffer.core import Buffer, BufferPrototype # Nominal byte cost charged to ``max_size`` for a negative (known-absent) entry. @@ -140,7 +143,8 @@ class CacheStore(WrapperStore[Store]): answers byte-range reads of that key, by asking the cache store for the range instead of the source. Ranges of a key with no cached value are cached in memory, inside the key's tracking record, so that partial reads never pollute - the filesystem (or other persistent backend). + the filesystem (or other persistent backend); ``get_ranges`` and + ``get_partial_values`` are routed through ``get``, so they use the cache too. Both halves share the same ``max_size`` budget, and eviction is LRU over *keys*: the least-recently-used key is chosen first (absent markers before @@ -863,6 +867,88 @@ async def delete(self, key: str) -> None: self._drop_key(key) await self._cache.delete(key) + async def delete_dir(self, prefix: str) -> None: + """ + Delete a prefix from the underlying store and drop its cached keys. + + ``WrapperStore.delete_dir`` delegates straight to the source store, so + without this override no ``delete`` runs for the keys under *prefix* and + the cache keeps serving them (for the whole ``max_age_seconds`` window) + after e.g. an ``overwrite=True`` array creation. + """ + await super().delete_dir(prefix) + if prefix != "" and not prefix.endswith("/"): + prefix += "/" + await self._cache.delete_dir(prefix) + async with self._state.lock: + for key in [k for k in self._state.entries if k.startswith(prefix)]: + self._drop_key(key) + + async def clear(self) -> None: + """Clear the underlying store, and with it everything cached from it. + + Same bypass as ``delete_dir``: ``WrapperStore.clear`` delegates to the + source store, which would leave the cache serving values for keys that no + longer exist anywhere. + """ + await super().clear() + await self.clear_cache() + + async def get_partial_values( + self, + prototype: BufferPrototype, + key_ranges: Iterable[tuple[str, ByteRequest | None]], + ) -> list[Buffer | None]: + """Partial-value reads routed through ``self.get``, so they use the cache. + + ``WrapperStore`` forwards this straight to the source store, which would + read around the cache: the reads would neither be served from it nor + recorded in it, and their observations would not supersede the keys' + records. + """ + + async def _get(key: str, byte_range: ByteRequest | None) -> Buffer | None: + return await self.get(key, prototype=prototype, byte_range=byte_range) + + return await concurrent_map(key_ranges, _get, limit=None) + + async def get_ranges( + self, + key: str, + byte_ranges: Sequence[ByteRequest | None], + *, + prototype: BufferPrototype, + max_concurrency: int | None = None, + max_gap_bytes: int | None = None, + max_coalesced_bytes: int | None = None, + ) -> AsyncIterator[Sequence[tuple[int, Buffer | None]]]: + """Byte-range reads routed through the coalescing ``Store.get_ranges``. + + The ``WrapperStore`` delegation would bypass this store's ``get`` — and so + the cache — for the sharded partial-read path, which is exactly the mixed + full/range workload this cache targets: the shard index read goes through + ``get`` while the chunk reads would not. Routing through the ``Store`` + default runs the same coalescer over ``self.get`` instead, so a cached full + value serves every range of it by slicing and uncached ranges are recorded. + ``None`` for a coalescing kwarg means "use the ``Store`` default". + """ + kwargs: dict[str, int] = {} + if max_concurrency is not None: + kwargs["max_concurrency"] = max_concurrency + if max_gap_bytes is not None: + kwargs["max_gap_bytes"] = max_gap_bytes + if max_coalesced_bytes is not None: + kwargs["max_coalesced_bytes"] = max_coalesced_bytes + async for group in Store.get_ranges(self, key, byte_ranges, prototype=prototype, **kwargs): + yield group + + async def _get_many( + self, requests: Iterable[tuple[str, BufferPrototype, ByteRequest | None]] + ) -> AsyncGenerator[tuple[str, Buffer | None], None]: + """Batch reads routed through ``self.get`` (see ``get_partial_values``).""" + async for req in Store._get_many(self, requests): + yield req + def cache_info(self) -> dict[str, Any]: """Return information about the cache state. diff --git a/tests/test_experimental/test_cache_store.py b/tests/test_experimental/test_cache_store.py index d5d90ef050..93f30162ab 100644 --- a/tests/test_experimental/test_cache_store.py +++ b/tests/test_experimental/test_cache_store.py @@ -1843,6 +1843,80 @@ async def test_concurrent_write_not_shadowed_by_stale_miss_through_public_api(se assert result.to_bytes() == b"value" await cs._assert_invariants() + async def test_delete_dir_invalidates_cached_keys(self) -> None: + """``delete_dir`` must not bypass the cache: every key under the prefix is + dropped, so an overwrite of an array does not leave its chunks readable.""" + source = MemoryStore() + cs = CacheStore(source, cache_store=MemoryStore()) + proto = default_buffer_prototype() + + await source.set("p/k", CPUBuffer.from_bytes(b"AAAAA")) + await source.set("q/k", CPUBuffer.from_bytes(b"BBBBB")) + assert await cs.get("p/k", proto) is not None + assert await cs.get("q/k", proto) is not None + + await cs.delete_dir("p") + + assert "p/k" not in cs._state.entries + assert await cs._cache.get("p/k", proto) is None + assert await cs.get("p/k", proto) is None + # A sibling prefix is untouched. + kept = await cs.get("q/k", proto) + assert kept is not None + assert kept.to_bytes() == b"BBBBB" + await cs._assert_invariants() + + async def test_clear_also_clears_the_cache(self) -> None: + """``clear`` has the same bypass shape as ``delete_dir``: emptying the + source must not leave the cache serving what it held.""" + source = MemoryStore() + cs = CacheStore(source, cache_store=MemoryStore()) + proto = default_buffer_prototype() + + await source.set("k", CPUBuffer.from_bytes(b"AAAAA")) + assert await cs.get("k", proto) is not None + + await cs.clear() + + assert not cs._state.entries + assert await cs._cache.get("k", proto) is None + assert await cs.get("k", proto) is None + + async def test_get_ranges_uses_the_cache(self) -> None: + """``get_ranges`` (the sharded partial-read path) goes through this store's + ``get``, so a cached full value serves its ranges and nothing reaches the + source.""" + source = MemoryStore() + cs = CacheStore(source, cache_store=MemoryStore()) + proto = default_buffer_prototype() + + await source.set("k", CPUBuffer.from_bytes(b"0123456789")) + assert await cs.get("k", proto) is not None + await source.delete("k") # only the cache can answer now + + requested: list[ByteRequest | None] = [RangeByteRequest(0, 4), SuffixByteRequest(2)] + observed: dict[int, bytes] = {} + async for group in cs.get_ranges("k", requested, prototype=proto): + for index, buffer in group: + assert buffer is not None + observed[index] = buffer.to_bytes() + assert observed == {0: b"0123", 1: b"89"} + + async def test_get_partial_values_uses_the_cache(self) -> None: + """``get_partial_values`` likewise routes through ``get`` rather than + straight to the source store.""" + source = MemoryStore() + cs = CacheStore(source, cache_store=MemoryStore()) + proto = default_buffer_prototype() + + await source.set("k", CPUBuffer.from_bytes(b"0123456789")) + assert await cs.get("k", proto) is not None + await source.delete("k") + + results = await cs.get_partial_values(proto, [("k", RangeByteRequest(0, 4)), ("k", None)]) + assert [b.to_bytes() for b in results if b is not None] == [b"0123", b"0123456789"] + assert len(results) == 2 + async def test_negative_count_matches_the_records(self) -> None: """``negative_count`` gates both the marker cap and the O(1) eviction fast path, so check it against a recount rather than against itself.""" @@ -1900,7 +1974,9 @@ async def test_invariants_hold_under_randomised_operations(self) -> None: cache_missing=rng.choice([True, False]), ) for _ in range(120): - key = f"k{rng.randrange(6)}" + # Two prefixes of three keys each, so ``delete_dir`` has something + # to match and something to leave alone. + key = f"p{rng.randrange(2)}/k{rng.randrange(3)}" match rng.randrange(6): case 0: await cs.get(key, proto, byte_range=rng.choice(byte_ranges)) @@ -1915,7 +1991,7 @@ async def test_invariants_hold_under_randomised_operations(self) -> None: # Out-of-band write: the source moves on without the cache. await source.set(key, CPUBuffer.from_bytes(b"o" * 30)) case _: - await cs.delete_dir("k") + await cs.delete_dir(f"p{rng.randrange(2)}") await cs._assert_invariants() From 7d985760505e4f340009e45f90c9f6593097688b Mon Sep 17 00:00:00 2001 From: espg Date: Tue, 11 Aug 2026 18:01:15 -0700 Subject: [PATCH 15/15] order LRU recency by a monotonic use-tick, not wall time: coarse clocks (Windows ~15.6 ms) tie and mis-evict --- src/zarr/experimental/cache_store.py | 35 ++++++++++++++++----- tests/test_experimental/test_cache_store.py | 27 ++++++++++++++++ 2 files changed, 54 insertions(+), 8 deletions(-) diff --git a/src/zarr/experimental/cache_store.py b/src/zarr/experimental/cache_store.py index 7dcbb1365a..782b428d3b 100644 --- a/src/zarr/experimental/cache_store.py +++ b/src/zarr/experimental/cache_store.py @@ -59,7 +59,7 @@ class _RangeEntry: Held in memory inside the key's ``_KeyState`` so partial reads never touch the persistent backend. ``insert_time`` bounds staleness via - ``max_age_seconds``; ``last_used`` orders eviction within the key. + ``max_age_seconds``; ``last_used`` orders eviction within the key (a monotonic use-tick, not wall time — coarse clocks tie). """ buffer: Buffer @@ -124,6 +124,10 @@ class _CacheState: # Number of keys whose full slot is an absent marker, maintained incrementally # so the negative-marker cap and the eviction-candidate fast path are O(1). negative_count: int = 0 + # Monotonic use-counter for within-key LRU ordering. Wall-clock recency ties + # on coarse clocks (Windows monotonic granularity ~15.6 ms) and mis-evicts; + # real time is kept only where TTL needs it (``insert_time``). + use_tick: int = 0 lock: asyncio.Lock = field(default_factory=asyncio.Lock) hits: int = 0 misses: int = 0 @@ -388,8 +392,14 @@ async def _record_missing(self, key: str) -> None: await self._evict_slot(lru_absent) now = time.monotonic() + self._state.use_tick += 1 state = _KeyState( - full=_Entry(insert_time=now, size=_NEGATIVE_ENTRY_SIZE, present=False, last_used=now) + full=_Entry( + insert_time=now, + size=_NEGATIVE_ENTRY_SIZE, + present=False, + last_used=self._state.use_tick, + ) ) state.assert_coherent() # The key was popped above, so this assignment appends it as most-recent. @@ -523,8 +533,14 @@ async def _track_entry(self, key: str, value: Buffer) -> bool: # as most-recently-used). await self._accommodate_value(value_size) now = time.monotonic() + self._state.use_tick += 1 state = _KeyState( - full=_Entry(insert_time=now, size=value_size, present=True, last_used=now) + full=_Entry( + insert_time=now, + size=value_size, + present=True, + last_used=self._state.use_tick, + ) ) state.assert_coherent() self._state.entries[key] = state @@ -574,8 +590,9 @@ async def _track_range(self, key: str, byte_range: ByteRequest, value: Buffer) - if state is None: state = _KeyState() now = time.monotonic() + self._state.use_tick += 1 state.ranges[byte_range] = _RangeEntry( - buffer=value, insert_time=now, size=value_size, last_used=now + buffer=value, insert_time=now, size=value_size, last_used=self._state.use_tick ) state.assert_coherent() self._state.entries[key] = state @@ -590,14 +607,15 @@ async def _update_access_order(self, key: str, byte_range: ByteRequest | None = state = self._state.entries.get(key) if state is None: return - now = time.monotonic() + self._state.use_tick += 1 + tick = self._state.use_tick if byte_range is None: if state.full is not None: - state.full.last_used = now + state.full.last_used = tick else: range_entry = state.ranges.get(byte_range) if range_entry is not None: - range_entry.last_used = now + range_entry.last_used = tick self._state.entries.move_to_end(key) # ------------------------------------------------------------------ @@ -789,7 +807,8 @@ async def get( self._state.negative_hits += 1 # Mark the marker most-recently-used so eviction stays LRU: # a frequently-probed absent key should outlive cold markers. - slot.last_used = time.monotonic() + self._state.use_tick += 1 + slot.last_used = self._state.use_tick self._state.entries.move_to_end(key) return None diff --git a/tests/test_experimental/test_cache_store.py b/tests/test_experimental/test_cache_store.py index 93f30162ab..17c8c24d28 100644 --- a/tests/test_experimental/test_cache_store.py +++ b/tests/test_experimental/test_cache_store.py @@ -1604,6 +1604,33 @@ async def test_range_entries_evicted_lru_within_key(self) -> None: assert r3 in ranges assert cs._state.current_size <= 100 + async def test_lru_ordering_survives_a_frozen_clock( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Recency ordering must not depend on wall-clock resolution: on Windows + the monotonic clock ticks ~15.6 ms, so an entire touch-then-evict sequence + can land on one timestamp. Freeze the clock to force universal ties and + assert the use-tick ordering still evicts the true LRU range.""" + import time as _time + + monkeypatch.setattr(_time, "monotonic", lambda: 1000.0) + source = MemoryStore() + cs = CacheStore(source, cache_store=MemoryStore(), max_size=100) + proto = default_buffer_prototype() + + await source.set("k", CPUBuffer.from_bytes(b"x" * 120)) + r1 = RangeByteRequest(0, 40) + r2 = RangeByteRequest(40, 80) + r3 = RangeByteRequest(80, 120) + assert await cs.get("k", proto, byte_range=r1) is not None + assert await cs.get("k", proto, byte_range=r2) is not None + assert await cs.get("k", proto, byte_range=r1) is not None + assert await cs.get("k", proto, byte_range=r3) is not None + ranges = cs._state.entries["k"].ranges + assert r1 in ranges + assert r2 not in ranges + assert r3 in ranges + async def test_eviction_candidate_prefers_markers_else_lru_key(self) -> None: """Candidate selection: with no markers the LRU key is returned (O(1) fast path); with markers present, the LRU marker wins over an older cached value."""