fix: use real LRU eviction in DirCache - #2097
Conversation
The lru_cache-based eviction in DirCache never removed entries from _cache: lru_cache calls its wrapped function only on cache misses, so old keys were silently evicted from the recency index without popping _cached listings. Reads of evicted keys then deleted them lazily, and __iter__ (which filters through __getitem__) destroyed the whole cache. Track recency with an OrderedDict instead, evict the oldest entry on insert beyond max_paths, refresh order on access, and keep __delitem__ and __iter__ from corrupting state. Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
fallenmi
left a comment
There was a problem hiding this comment.
The new recency queue is mutated before self._cache[item] establishes that the key exists. MutableMapping.__contains__ calls __getitem__, so an ordinary miss corrupts the eviction order:
d = DirCache(max_paths=2)
d["a"] = 1
d["b"] = 2
assert "missing" not in d
d["c"] = 3
assert dict(d._cache) == {"b": 2, "c": 3}On exact head 4544763, the final cache is only {"c": 3} and _q is OrderedDict({"missing": None, "c": None}): the ghost key makes the next insertion evict both valid entries. The exact base fails the normal LRU oracle while this head fixes it, but this absent-key oracle still fails. Please read the cached value, allowing KeyError to happen, before refreshing _q, and cover missing and expired lookups. I also ran the changed target plus test_core.py: 62 passed, 6 skipped; the diff check and compile check are clean.
Reviewed with Codex (GPT-5); the exact base/head oracles and listed tests were run locally.
Fixes #2095
Problem
DirCacheuses afunctools.lru_cacheas an eviction helper, butlru_cachecalls its wrapped function only on cache misses — never when an old key is evicted from its recency index. As a result:max_pathsnever removes old entries from_cache(unbounded growth)._cachelazily and raisesKeyError.__iter__filters through__getitem__, so iterating destroys the entire cache and returns[].Change
Track recency with an
OrderedDictinstead:_cacheand_times) when overmax_paths;__delitem__andclear()keep the recency index consistent.Testing
Added
fsspec/tests/test_dircache.pycovering eviction, iteration stability, recency refresh, delete, and expiry. With the fix:160 passedin the cache test modules; the remaining failures/errors are pre-existing missing-optional-dependency (aiohttp) import issues.Notes
max_pathsis currently undocumented in the class docstring (only in the__init__docstring); I left that unchanged to keep the diff focused.