[simpleqt]

Problem

Client-side caching stops working — and raises an unexpected KeyError — as soon as a
_LocalCache that is configured with both a max size and a ttl evicts for the second
time. Both settings are reachable from the public client API:

valkey.Valkey(protocol=3, cache_enabled=True, cache_max_size=2, cache_ttl=60)

cache_max_size/cache_ttl are forwarded to every connection
(valkey/connection.py, AbstractConnection.__init__ → _LocalCache(cache_max_size, cache_ttl, cache_policy)), and _add_to_local_cache() calls cache.set() on every
cacheable response, so the KeyError escapes from execute_command().

Reproduction (no server required)

G:\pr\work\vk\scratch_repro.py drives _LocalCache directly (the class used by the
client and passed as client_cache=/built by cache_enabled=True):

G:\pr\work\vk\.venv\Scripts\python.exe scratch_repro.py

Observed on main (see vk-repro-unpatched.txt, all three eviction policies):

== scenario 1: cache_ttl set, commands evicted in insert order ==
lru:
  set('GET k3')
    -> ok       cache=['GET k2', 'GET k3'] ttl_list=['GET k1', 'GET k2', 'GET k3']
  set('GET k4')
    -> KeyError: KeyError('GET k1')
       cache=['GET k2', 'GET k3'] ttl_list=['GET k1', 'GET k2', 'GET k3']
  result: KeyError

Expected: the eviction succeeds (result: no error), and the bookkeeping lists only the
commands still held by the cache. Observed with the fix (vk-repro-patched.txt):

  set('GET k4')
    -> ok       cache=['GET k3', 'GET k4'] ttl_list=['GET k3', 'GET k4']
  result: no error

A second scenario shows the same class of breakage after re-caching one command
(GET k1 twice), and a third shows the silent version of the bug with the default
ttl=0: two live commands but four ids in commands_ttl_list.

Root cause

valkey/_cache.py, _LocalCache._evict() removed the victim with
self.cache.popitem(last=False) / self.cache.pop(...) instead of delete_command().
The evicted command therefore stayed in self.commands_ttl_list (and its keys stayed in
self.key_commands_map).

_evict() then checks the head of that list for expiry before applying the eviction
policy:

if self._is_expired(self.commands_ttl_list[0]):

and _is_expired() indexes the cache:

return time.monotonic() - self.cache[command]["ctime"] > self.ttl

Once the head of commands_ttl_list was an already evicted command, that indexing raised
KeyError. (ttl == 0 short-circuits in _is_expired(), which is why the crash needs a
ttl; it is also why the ttl=0 variant only leaks stale ids.)

_LocalCache.set() had a second, independent way to break the same invariant: caching a
command that was already cached appended a second id to commands_ttl_list, and
delete_command() removes only one occurrence, leaving a stale head again.

Fix

• _evict() now removes the victim through delete_command(), so the command leaves
  self.cache, self.commands_ttl_list and self.key_commands_map together. The victim
  selection is unchanged: LRU still takes the front of the (recently-used ordered) dict,
  LFU the minimum access_count, RANDOM a random key.
• set() refreshes the commands_ttl_list position instead of appending a duplicate when
  the command is already cached, so the list holds exactly one id per cached command.

Diff: valkey/_cache.py, +8 −3.

Tests

New server-free class TestLocalCacheEviction in tests/test_cache.py (and its async
mirror in tests/test_asyncio/test_cache.py, required by CONTRIBUTING.md/AGENTS.md since
_LocalCache is shared code):

• test_eviction_with_ttl_does_not_raise — LRU/LFU/RANDOM, 10 cache writes on a
  max_size=2, ttl=60 cache.
• test_evicted_commands_are_dropped_from_ttl_list — the
  commands_ttl_list == cache (and no-duplicates) invariant.
• test_recaching_a_command_does_not_duplicate_ttl_entry.

RED on base (source only reverted, tests kept):

$ python -m pytest tests/test_cache.py::TestLocalCacheEviction \
      tests/test_asyncio/test_cache.py::TestLocalCacheEviction -q
...
FAILED tests/test_cache.py::TestLocalCacheEviction::test_eviction_with_ttl_does_not_raise[lru]
...
14 failed in 0.46s

with the underlying error

valkey\_cache.py:249: in set
    self._evict()
valkey\_cache.py:345: in _evict
    if self._is_expired(self.commands_ttl_list[0]):
valkey\_cache.py:324: in _is_expired
    return time.monotonic() - self.cache[command]["ctime"] > self.ttl
KeyError: 'GET key0'

GREEN with the fix:

$ python -m pytest tests/test_cache.py::TestLocalCacheEviction \
      tests/test_asyncio/test_cache.py::TestLocalCacheEviction -q
14 passed in 0.08s

Checks run

Command	Result
python -m pytest tests/test_cache.py::TestLocalCacheEviction tests/test_asyncio/test_cache.py::TestLocalCacheEviction -q	14 passed
python -m pytest tests/test_helpers.py tests/test_connection_pool.py::TestConnectionPoolURLParsing tests/test_connection_pool.py::TestBlockingConnectionPoolURLParsing tests/test_connection_pool.py::TestConnectionPoolUnixSocketURLParsing tests/test_connection_pool.py::TestSSLConnectionURLParsing tests/test_retry.py::TestRetry tests/test_retry.py::TestConnectionConstructorWithRetry tests/test_connection.py::TestConnection tests/test_connection.py::test_pack_command tests/test_connection.py::test_format_error_message tests/test_connection.py::test_parsing_unix_socket_relative_path tests/test_connection.py::TestConnectionTimeoutValidation tests/test_encoding.py::TestMemoryviewsAreNotPacked -q	121 passed, 1 skipped, 1 failed (TestConnection::test_retry_connect_on_timeout_error needs a real server)
ruff check tests valkey	All checks passed
ruff format --check docs tests valkey	147 files already formatted
vulture valkey whitelist.py --min-confidence 80	no findings
flynt --fail-on-change --dry-run tests valkey	no changes
mypy (with the type-check dependency group installed)	Success: no issues found in 71 source files

Not run — no Valkey server on this host. tests/test_cache.py and
tests/test_asyncio/test_cache.py in full were executed but every Valkey-backed case
failed/errored with ConnectionError: Error 10061 connecting to localhost:6379
(sentinel/cluster cases: their master/sentinel ports) — the only tests that passed were the
15 server-free ones: the 7 new tests in each module, plus
tests/test_cache.py::TestLocalCache::test_cache_not_available_with_resp2
(sync module: 8 passed, 21 failed, 28 errors; both modules together: 15 passed, 41 failed,
64 errors). The integration suites
(make standalone-tests, make cluster-tests), everything under the valkeymod, ssl,
replica, onlycluster and sentinel markers, and the client-side-caching tests that
exercise a real server were not run.

Pull Request check-list (.github/PULL_REQUEST_TEMPLATE.md)

• Do tests and lints pass with this change? — locally yes for the server-free
  subset and all pinned linters/type checks (see table); the Valkey-backed suites could
  not be exercised on this host.
• Do the CI tests pass with this change? — not verifiable here (no Valkey services on
  this host, no fork CI run); a CI run is required.
• Is the new or changed code fully tested? — the changed paths (_evict, set
  bookkeeping) are covered by the new server-free tests.
• Documentation update — none needed: no public API change.
• Example added — not applicable.

AI assistance

The defect was found, reproduced, fixed and tested by the assistant (DeepSeek Harness
agent), which wrote this draft; every command output quoted above was produced by that
agent on this host. A human reviewed the change before submission.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.