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 read_response callers except those passing disconnect_on_error=False; low scope but can affect connection pooling under error paths.

Overview
Fixes stale replies on Sentinel-managed connections when a read is interrupted before the full Redis response is consumed.

SentinelManagedConnection.read_response() now defaults disconnect_on_error to True, matching base Connection and the asyncio Sentinel class. Previously the default was False, so a broken read could return the connection to the pool and the next command could read the previous command’s reply (e.g. ping() returning False while get() returns b'PONG'). Callers that need to keep the socket open (e.g. PubSub with an explicit disconnect_on_error=False) are unchanged.

Tests that only asserted the forwarded argument now expect True. New sync and asyncio tests check real behavior: disconnect on BaseException during read, correct reply on the following command, default alignment with Connection, and honoring disconnect_on_error=False.

^{Reviewed by Cursor Bugbot for commit 4454721. 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.