Repository navigation
Conversation
…olds `CacheStore` had four write paths that bypassed the cache entirely, plus two accounting bugs and two lock windows where the tracking state and the backing store could be observed out of step. Bypassed write paths. `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. Under the default `max_age_seconds="infinity"` the stale value was then served forever. The most visible case: `zarr.create_array(..., overwrite=True)` goes through `delete_dir`, so overwriting an array through a `CacheStore` left the *old* array readable through the cache. 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. A value too large to cache was left in the backing store untracked -- uncounted against `max_size`, never eviction-eligible, and still served as a hit. `_track_entry` could also select the very entry it was re-tracking as an eviction candidate, double-subtracting its size and deleting the value 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 or was left in the backing store with no tracking entry at all. Also fixes `CacheStore.open()`, which inherited `WrapperStore.open()` -- that builds the wrapped store from a `store_cls` argument and cannot supply the required `cache_store`, so it always raised. `cache_store` must now support listing as well as deletes, since prefix deletions need it; this is checked in the constructor rather than failing later mid-write. Tests: adds `TestCacheStoreWriteCoherence` (12 of its 14 cases fail without this change) and runs `CacheStore` through the shared `StoreTests` conformance suite for the first time. Assisted-by: ClaudeCode:claude-opus-5
The changelog check requires an integer filename. Note this is the fork PR number; it needs renaming again if this goes upstream. Assisted-by: ClaudeCode:claude-opus-5
Assisted-by: Codex:GPT-6
Assisted-by: Codex:GPT-6
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 AI text below 🤖
Fixes CacheStore invalidation and size accounting for writes, deletes, bulk writes, conditional writes, prefix deletion, and clearing. It also adds CacheStore.open, validates listing support on the cache backend, and runs the shared store conformance tests.
Cache backing-store mutations and their tracking updates share the cache lock. Per-key write locks order publication for one key while permitting independent keys to progress. Generation checks stop delayed source reads or writes from restoring entries invalidated in the meantime. These are guarantees for operations through this CacheStore; external mutations of the source can still leave cached data stale.
There are five explicitly skipped conformance methods: three require a read_only constructor keyword, one requires the unsupported _with_store/context-manager path, and one exercises synchronous deletion despite CacheStore opting out of sync I/O. The former description incorrectly counted four read-only methods in addition to the other two.
The release fragment is changes/298.bugfix.md. Earlier baseline-failure counts, total test results, and main-branch distance were recorded during development; they are not fresh measurements of the current branches. Negative caching and the unbounded range-cache behavior with max_size=None remain outside this change.