fix(sentinel): disconnect on read errors by default

[dylanpulver]

Description of change

SentinelManagedConnection.read_response() defaults disconnect_on_error to False (redis/sentinel.py:73), so a connection whose reply was never read goes back to the pool and the next command gets the previous one's response. Against a real sentinel, after an interrupt at the socket read:

master.ping()       -> False
master.get("probe") -> b'PONG'

Every other read_response defaults to True, including the async twin at redis/asyncio/sentinel.py:76 and base Connection.read_response, whose except BaseException branch exists for this case (#1128, #2499). Both were written in one commit, 35b7e09a for #2754, which took the value from the PubSub.parse_response call site, where False is passed explicitly.

PubSub passes the value explicitly, so it is unaffected. ReadOnlyError too, since the parser returns error replies rather than raising them and they surface after the guarded block.

Why no existing test caught it: they patch out Connection.read_response, the only code that reads the flag, so they check the forwarded value and never its effect. Four assertions pinned False and now pin True; the new tests assert socket state and the next reply.

Ran tests/ standalone plus asyncio against a local sentinel setup, with a failure set identical to master's (the rest need redis-stack or TLS). invoke linters clean on the pinned ruff 0.9.6. Reverting the one-line change fails three of the four new tests; the fourth guards the explicit False opt-out and passes either way.

Prepared with Claude (Opus 5).

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
Changes default connection lifecycle for all Sentinel-managed reads except explicit disconnect_on_error=False; low blast radius but affects pooled connection reuse after read failures.

Overview
Fixes stale replies on Sentinel master connections when a read is interrupted before the full response is consumed (e.g. KeyboardInterrupt at the socket read).

SentinelManagedConnection.read_response() now defaults disconnect_on_error to True, matching base Connection, the async Sentinel connection, and the behavior intended in #1128. With the old default of False, the socket stayed open and pooled connections could return the previous command’s reply on the next call (e.g. ping() → wrong result, get() → PONG). Callers that need to keep the connection open (e.g. PubSub with an explicit disconnect_on_error=False) are unchanged.

Tests are updated to expect the new default and add sync/async coverage that checks socket teardown and the next reply, including an interrupted transactional pipeline recycled through SentinelConnectionPool.

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

[petyaslavova]

Hey @dylanpulver, thank you for your contribution!

Confirmed, and the impact is wider than the description argues. Every read_response caller that omits disconnect_on_error is a strict command/response pair that needs True - Redis.parse_response, the on_connect handshake, _send_ping, re_auth. The three callers that genuinely want False (PubSub, the pending-push drain, _process_pending_invalidations) all pass it explicitly, so the current default has no beneficiary. It was never a design choice: it came from the #2754 hotfix, where the sync half was amended in last (4598d805d) and the author noted they were unfamiliar with Sentinel - which is why the async twin got True in the same commit.

Two things before merge:

1. Please drop test_default_disconnect_on_error_matches_base_connection. Asserting the default through inspect.signature restates the source and would still pass if the flag were never forwarded; the four assertions you updated already pin it.
2. Please add a pipeline regression test. _execute_transaction reads N+2 replies in a loop, and a BaseException at reply k strands the rest: Retry does not treat it as retryable, execute() catches only Exception, reset() disconnects only when watching, and release() only when should_reconnect(). The connection goes back to _available_connections desynced, which is a worse failure than the single-command case you reproduced.