From a0dc8cb84fb0a0cffbefd12802ad694ed15a4722 Mon Sep 17 00:00:00 2001 From: simpleqt <2015900818@qq.com> Date: Tue, 20 Oct 2026 21:37:32 +0800 Subject: [PATCH] fix: keep client cache bookkeeping in sync when evicting entries Problem: with client-side caching enabled and configured with both a max size and a ttl (`cache_enabled=True`, `cache_max_size=N`, `cache_ttl=T`), the cache stopped working after its second eviction: cache writes raised a bare KeyError('GET ') out of execute_command. Cause: `_LocalCache._evict()` dropped the evicted command with `self.cache.popitem(last=False)` / `self.cache.pop(...)`, so the command stayed in `self.commands_ttl_list` (and its keys stayed in `key_commands_map`). `_evict()` inspects `commands_ttl_list[0]` for expiry before applying the eviction policy, and `_is_expired()` indexes `self.cache[command]`; as soon as the head of that list was an already evicted command, the lookup raised KeyError. `_LocalCache.set()` also appended a duplicate id to `commands_ttl_list` when a command that was already cached was cached again, which broke the same invariant on its own. Fix: evict through `delete_command()` so a command is removed from the cache, from `commands_ttl_list` and from `key_commands_map` together, and refresh the `commands_ttl_list` position instead of appending a duplicate when the same command is cached again. Tests: new server-free `TestLocalCacheEviction` cases in tests/test_cache.py and tests/test_asyncio/test_cache.py cover eviction with a ttl under the LRU/LFU/RANDOM policies, the commands_ttl_list/cache invariant, and re-caching a command. The Valkey-backed tests in tests/test_cache.py could not be run here: this host has no valkey server. Signed-off-by: simpleqt <2015900818@qq.com> --- tests/test_asyncio/test_cache.py | 29 +++++++++++++++++++++++++++++ tests/test_cache.py | 29 +++++++++++++++++++++++++++++ valkey/_cache.py | 11 ++++++++--- 3 files changed, 66 insertions(+), 3 deletions(-) diff --git a/tests/test_asyncio/test_cache.py b/tests/test_asyncio/test_cache.py index 375fb502..993da2c2 100644 --- a/tests/test_asyncio/test_cache.py +++ b/tests/test_asyncio/test_cache.py @@ -300,6 +300,35 @@ async def test_flush_entire_cache(self, r): assert await r.get("c") == "1" +class TestLocalCacheEviction: + """Unit tests for the _LocalCache eviction bookkeeping (no server needed).""" + + @pytest.mark.parametrize("eviction_policy", list(EvictionPolicy)) + async def test_eviction_with_ttl_does_not_raise(self, eviction_policy): + # a cache configured with a max size and a ttl used to raise KeyError + # from _is_expired() on its second eviction + cache = _LocalCache(max_size=2, ttl=60, eviction_policy=eviction_policy) + for i in range(10): + cache.set(f"GET key{i}", i, [f"key{i}"]) + assert len(cache.cache) == 2 + + @pytest.mark.parametrize("eviction_policy", list(EvictionPolicy)) + async def test_evicted_commands_are_dropped_from_ttl_list(self, eviction_policy): + cache = _LocalCache(max_size=2, ttl=60, eviction_policy=eviction_policy) + for i in range(5): + cache.set(f"GET key{i}", i, [f"key{i}"]) + # commands_ttl_list must only track the commands still cached + assert sorted(cache.commands_ttl_list) == sorted(cache.cache) + assert len(cache.commands_ttl_list) == len(set(cache.commands_ttl_list)) + + async def test_recaching_a_command_does_not_duplicate_ttl_entry(self): + cache = _LocalCache(max_size=2, ttl=60) + cache.set("GET key", 1, ["key"]) + cache.set("GET key", 2, ["key"]) + assert cache.commands_ttl_list == ["GET key"] + assert cache.get("GET key") == 2 + + @pytest.mark.skipif(LIBVALKEY_AVAILABLE, reason="PythonParser only") @pytest.mark.onlycluster class TestClusterLocalCache: diff --git a/tests/test_cache.py b/tests/test_cache.py index c6ba16af..8334dd5f 100644 --- a/tests/test_cache.py +++ b/tests/test_cache.py @@ -418,6 +418,35 @@ def test_get_from_cache_invalidate_via_get(self, r, r2): assert r.get("foo") == b"barbar" +class TestLocalCacheEviction: + """Unit tests for the _LocalCache eviction bookkeeping (no server needed).""" + + @pytest.mark.parametrize("eviction_policy", list(EvictionPolicy)) + def test_eviction_with_ttl_does_not_raise(self, eviction_policy): + # a cache configured with a max size and a ttl used to raise KeyError + # from _is_expired() on its second eviction + cache = _LocalCache(max_size=2, ttl=60, eviction_policy=eviction_policy) + for i in range(10): + cache.set(f"GET key{i}", i, [f"key{i}"]) + assert len(cache.cache) == 2 + + @pytest.mark.parametrize("eviction_policy", list(EvictionPolicy)) + def test_evicted_commands_are_dropped_from_ttl_list(self, eviction_policy): + cache = _LocalCache(max_size=2, ttl=60, eviction_policy=eviction_policy) + for i in range(5): + cache.set(f"GET key{i}", i, [f"key{i}"]) + # commands_ttl_list must only track the commands still cached + assert sorted(cache.commands_ttl_list) == sorted(cache.cache) + assert len(cache.commands_ttl_list) == len(set(cache.commands_ttl_list)) + + def test_recaching_a_command_does_not_duplicate_ttl_entry(self): + cache = _LocalCache(max_size=2, ttl=60) + cache.set("GET key", 1, ["key"]) + cache.set("GET key", 2, ["key"]) + assert cache.commands_ttl_list == ["GET key"] + assert cache.get("GET key") == 2 + + @pytest.mark.skipif(LIBVALKEY_AVAILABLE, reason="PythonParser only") @pytest.mark.onlycluster class TestClusterLocalCache: diff --git a/valkey/_cache.py b/valkey/_cache.py index fa78403b..baf5bab8 100644 --- a/valkey/_cache.py +++ b/valkey/_cache.py @@ -254,6 +254,11 @@ def set( _ACCESS_COUNT: 0, # Used only for LFU } self._update_key_commands_map(keys_in_command, command) + if command in self.commands_ttl_list: + # the command is cached again, refresh its position so that + # commands_ttl_list has no duplicates and stays in sync with + # the commands currently held by the cache. + self.commands_ttl_list.remove(command) self.commands_ttl_list.append(command) def get(self, command: Union[str, Sequence[str]]) -> ResponseT | None: @@ -345,15 +350,15 @@ def _evict(self): if self._is_expired(self.commands_ttl_list[0]): self.delete_command(self.commands_ttl_list[0]) elif self.eviction_policy == EvictionPolicy.LRU: - self.cache.popitem(last=False) + self.delete_command(next(iter(self.cache))) elif self.eviction_policy == EvictionPolicy.LFU: min_access_command = min( self.cache, key=lambda k: self.cache[k].get("access_count", 0) ) - self.cache.pop(min_access_command) + self.delete_command(min_access_command) elif self.eviction_policy == EvictionPolicy.RANDOM: random_command = random.choice(list(self.cache.keys())) - self.cache.pop(random_command) + self.delete_command(random_command) def _update_key_commands_map( self, keys: List[KeyT], command: Union[str, Sequence[str]]