fix(cache_store): stop CacheStore serving data the source no longer holds

[d-v-b]

🤖 AI text below 🤖

Split out of the review of zarr-developers#4042. That PR adds negative caching to CacheStore, but a large part of its diff is fixing pre-existing coherence bugs that have nothing to do with the new feature. This is that part, on its own, so it can land and be reviewed independently.

Every fix here reproduces as a bug on main with no negative caching involved.

Write paths that bypassed the cache

set_if_not_exists, _set_many, delete_dir and clear were not overridden, so WrapperStore forwarded them straight to the source store and no invalidation ran. With the default max_age_seconds="infinity" the superseded value was then served forever:

operation	source after	CacheStore.get before this PR
_set_many	NEW	OLD
set_if_not_exists	NEW	OLD
delete_dir	(gone)	OLD
clear	(gone)	OLD

The user-visible form, since zarr.create_array(..., overwrite=True) goes through delete_dir:

cs = CacheStore(MemoryStore(), cache_store=MemoryStore())
a = zarr.create_array(cs, name="a", shape=(4,), chunks=(4,), dtype="i4")
a[:] = [1, 2, 3, 4]

zarr.create_array(cs, name="a", shape=(4,), chunks=(4,), dtype="i4", overwrite=True)

zarr.open_array(cs, path="a")[:]           # before: [1 2 3 4]  <- the *old* array
zarr.open_array(cs._store, path="a")[:]    #         [0 0 0 0]

Each of the four now invalidates the keys it affects.

Size accounting

• delete dropped an entry's tracking without reclaiming its bytes, so every delete permanently inflated current_size and ate into the max_size budget. Setting then deleting a single 100-byte value left current_size == 100.
• A value too large to cache was still written to the backing cache and then left there untracked — uncounted against max_size, never eviction-eligible, but still served as a hit.
• _track_entry could select the very entry it was re-tracking as an eviction candidate, double-subtracting its size, terminating the eviction loop early, and deleting the value the caller had just written.

Lock discipline

Every backing-store mutation is now published in the same locked section as its tracking mutation. Previously delete and clear_cache mutated the backing store outside the lock, so a concurrent set landing in that window either had its backing value deleted underneath it (leaving a tracking entry claiming bytes the backing store no longer held) or was left in the backing store with no tracking entry at all.

Both are covered by tests that drive a deterministic interleaving through a gated cache backend, rather than hoping a timing race reproduces.

Two smaller fixes

• CacheStore.open() never worked. It inherited WrapperStore.open(), which builds the wrapped store from a store_cls argument and so cannot supply the required cache_store. await CacheStore.open(MemoryStore(), cache_store=MemoryStore()) raised TypeError: 'MemoryStore' object is not callable. Included because it is five lines and it unblocks the conformance suite below — happy to drop it if you would rather keep this PR purely about coherence.
• cache_store must now support listing as well as deletes, since prefix deletion needs it. Checked in the constructor rather than failing later, mid-write, with a NotImplementedError from the backing store.

Tests

CacheStore now runs through the shared StoreTests conformance suite for the first time — these write paths are exactly what that suite exercises and nothing else did. Five of its tests are skipped with explicit reasons rather than papered over, because they are pre-existing gaps rather than things this PR should fix:

• four need a read_only= constructor kwarg, which CacheStore does not take (it derives read-only from the source store; with_read_only is the supported route and is covered separately);
• test_store_context_manager needs _with_store, which CacheStore raises on by design — a copy wrapping a different source store would share this store's cache and collide on keys;
• test_delete_sync_visible_to_async_get fails because CacheStore sets _supports_sync_io = False, but the inherited WrapperStore sync methods still satisfy SupportsDeleteSync structurally, so the harness does not skip it itself.

Whether the read-only gap is worth closing is a separate question; the skips at least record it somewhere other than a review comment.

New TestCacheStoreWriteCoherence class: 12 of its 14 cases fail on main. The two that pass are the set and delete arms of the parametrized invalidation test — those paths already invalidated correctly, and the delete bug was accounting, which the accounting test covers instead.

Full run: 132 passed, 5 skipped in the cache-store file; 1063 passed across tests/test_store + tests/test_experimental. ruff, mypy (strict), numpydoc and codespell clean.

Deliberately not included

• the _KeyState restructuring and the read-path routing (get_ranges / get_partial_values / _get_many);
• the max_age_seconds default change;
• negative caching itself.

The in-memory byte-range cache is still unbounded when max_size is None. That is pre-existing, and it belongs with the read-path routing work, since that is what would start filling it with chunk payloads rather than just shard indexes.

Notes

• The changelog fragment is changes/XXXX.bugfix.md and needs renaming to the PR number.
• This branch is based on d-v-b/zarr-python@main, which is currently 3 commits behind upstream. Nothing here depends on those commits.

🤖 Generated with Claude Code