Clear the in-progress cache entry when a read fails

[eeshsaxena]

Description of change

With client-side caching on, CacheProxyConnection.send_command stakes out an IN_PROGRESS placeholder in the pool-wide cache before sending, and only a successful read_response ever resolves it. If that read raises, the placeholder is left behind, and nothing else removes it: invalidations only arrive for keys the server is actually tracking, and this command never completed, so none is coming.

From then on the entry poisons that key for the whole pool. send_command finds an entry and returns without touching the network, then read_response sees the status is IN_PROGRESS, does not count it as a hit, and reads from a socket nothing was written to. So the client either blocks until the socket timeout or picks up a reply that belongs to some other command.

Getting there does not take anything exotic. One WRONGTYPE (a GET against a hash, an LRANGE against a string) or one NOPERM under a restricted ACL is enough, and MGET reaches it without the caller ever seeing an exception, since parse_response swallows the ResponseError when EMPTY_RESPONSE is set.

The fix is to drop the placeholder if the read that would have resolved it fails, and re-raise unchanged. Error type, message and traceback are untouched, and a ConnectionError still disconnects and flushes as before.

Without the fix, the new test fails on the leftover entry:

>       assert cache.size == 0
E       assert 1 == 0

and the follow-up assertion shows the second GET was never sent.

There is no async counterpart to change, since the async stack has no client-side cache.

Pull Request check-list

• Do tests and lints pass with this change?
• Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
• Is the new or changed code fully tested?
• Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
• Is there an example added to the examples folder (if applicable)?

---

Note

Medium Risk
Touches RESP3 client-side caching on the hot read path; behavior change is narrowly scoped to failed reads but incorrect cleanup previously could desync connections pool-wide.

Overview
Fixes client-side caching leaving stale IN_PROGRESS placeholders when CacheProxyConnection.read_response fails after a cacheable command was sent (e.g. WRONGTYPE, NOPERM).

send_command inserts a pool-wide placeholder that only a successful read resolves. On read failure, the change deletes that cache key and clears _current_command_cache_key, then re-raises the original exception so later calls for the same command/key still hit the network instead of skipping the send and reading the wrong socket reply.

Adds test_failed_read_clears_the_in_progress_cache_entry to assert the cache is empty after a failed read and a retry sends GET again.

^{Reviewed by Cursor Bugbot for commit 81cde52. Bugbot is set up for automated code reviews on this repo. Configure here.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 81cde525d8

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Verify ownership before deleting the cache entry

When an invalidation removes this command's placeholder while the failing read is in flight, another pooled connection can install a new entry for the identical CacheKey before this handler runs. Because _cache_lock is private to each proxy, this unconditional deletion removes that newer connection's IN_PROGRESS or VALID entry; under further same-key traffic, completions can then update an entry owned by another request or return a cached value while leaving their wire response unread, causing stale results or protocol desynchronization. Delete only when the current entry is still IN_PROGRESS and its connection_ref is self._conn.

Useful? React with 👍 / 👎.

[petyaslavova]

Hey @eeshsaxena, thank you for your contribution!

Confirmed. send_command stakes the IN_PROGRESS entry in the pool-wide cache, and only a successful read_response resolves it. Retryable failures are already covered indirectly, because the retry failure callback calls disconnect() on the proxy and that flushes the cache - but a ResponseError is not retried, so the entry survives. Every later call for the same command and key skips the send and then blocks on a socket where nothing was written. The fix and the regression test are right, and there is no async counterpart to mirror.

One hardening would be a good addition: scope the delete to the entry this connection staked (status IN_PROGRESS and connection_ref is self._conn) so a failing read cannot evict a valid entry another connection just stored.