fix: keep client cache bookkeeping in sync when evicting entries

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:

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:

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]]