fix: do not close connection on ReceiveWithTimeout deadline

[Solaris-star]

Summary

ReceiveWithTimeout treated every read error as fatal and closed the connection. Pub/sub poll loops that use a short timeout therefore permanently poison an otherwise idle connection after the first poll deadline.

Fix

Preserve the connection only when all of these hold:

1. The error is a read deadline timeout from the explicit ReceiveWithTimeout API.
2. bufio.Reader.ReadSlice consumed zero bytes of the reply. If it returns a partial first line with the timeout, the connection is closed.
3. c.pending == 0, so no command response can arrive later and be misattributed to a subsequent receive.

Connection-level Receive() / ReceiveContext timeouts remain fatal. The stale explicit read deadline is cleared only for the safe idle-poll case.

Test plan

• TestReceiveWithTimeoutDoesNotCloseConn: idle poll stays reusable.
• TestReceiveWithTimeoutClosesConnWhenRequestInFlight: timeout with a pending response closes the connection.
• TestReceiveWithTimeoutClosesConnAfterPartialReplyLine: timeout after +PARTIAL closes the connection.
• The partial-line regression fails 10/10 on the prior implementation and passes 10/10 after the fix.
• go test ./...
• go test -race ./redis

Fixes #676

[stevenh]

The problem with this is on timeout you can't guarantee the wire state, specifically there could have been a partial send or inflight response and even if there wasn't already there could be one in the future which could result unexpected data corrupting the response processing.

How is this fix preventing that from happening?

[Solaris-star]

@stevenh good catch — you're right that a clean boundary now isn't sufficient on its own, so the original version of this PR did not fully address your concern. I've reworked it.

The connection is now only preserved on timeout when both hold:

1. atBoundary — the timeout occurred before any bytes of a reply were consumed (readReplyBoundary reports this). A timeout mid-reply (partial bulk body / array element) still closes the connection, since the wire is left desynchronised.
2. c.pending == 0 — there is no request whose response is still outstanding. This is the case you raised: even if the wire is clean at the boundary, a previously-sent command whose reply hasn't arrived yet could land later and be misread as the answer to a subsequent receive. If anything is in flight we now close.

So the only path that keeps the connection is an idle wait with nothing outstanding — e.g. a pub/sub poll waiting for a server push, which is exactly what #676 needs. Any partial send / in-flight response (present or future) falls through to c.fatal(err).

Two tests cover both directions:

• TestReceiveWithTimeoutDoesNotCloseConn — idle poll, pending==0, connection stays open and a later Do succeeds.
• TestReceiveWithTimeoutClosesConnWhenRequestInFlight — a Send+Flushed PING with no reply (pending>0) times out at a boundary and the connection is closed.

Does this address the corruption scenario you had in mind?

[stevenh]

bug: I don't see how just because we got an error on readLine it indicates that it we're at a boundary.

The underling socket is read through bufio which states:
// If ReadSlice encounters an error before finding a delimiter,
// it returns all the data in the buffer and the error itself (often io.EOF).

So we could have experienced a partial read and a timeout meaning the connection state is actually unknown.

I don't think what you're trying to do is achievable, as there is no way to tell that there's no bytes on the wire when a timeout occurs.

[Solaris-star]

You are right: the previous atBoundary=true on every readLine error was incorrect because bufio.Reader.ReadSlice can return partial bytes together with the timeout.

I pushed 09cf40c to preserve the slice returned by ReadSlice on error and now report a clean boundary only when len(line) == 0. Any timeout after one or more bytes of the first RESP line have been consumed remains fatal. The existing guards still require pending == 0, so this only preserves an actually idle pub/sub poll.

I also added TestReceiveWithTimeoutClosesConnAfterPartialReplyLine, which writes +PARTIAL without a terminator and waits for the read deadline. Applied to the prior commit, it fails 10/10 because the connection remains open; with 09cf40c, it passes 10/10. Full go test ./... and go test -race ./redis pass.

This avoids claiming that no bytes exist on the wire; it only uses the bytes ReadSlice proves were consumed by this read.

[stevenh]

How does this handle:

1. Request
2. Reply timeout no bytes but response is actually inflight from server to client it has just not received yet

If that you allowed the connection to be reused the next request would get the previous request's response, causing corruption.

[Solaris-star]

Good question — the fix guards against exactly that with two independent checks before preserving the connection:

1. Zero bytes consumed: bufio.Reader.ReadSlice is called on the underlying reader. If it returns any bytes (even a partial first line like +PAR), the connection is closed unconditionally. Only a clean zero-byte read (nothing arrived before the deadline) is eligible for reuse.

2. No pending responses: c.pending == 0 ensures no command response is in flight. If a response could still arrive later, the connection is closed to prevent misattribution.

Both conditions must hold simultaneously. If either fails, the connection is closed exactly as before. The stale read deadline is also cleared so the next ReceiveWithTimeout call starts fresh.

The test TestReceiveWithTimeoutClosesConnAfterPartialReplyLine covers the partial-send case specifically: it writes +PARTIAL\r\n to the wire, triggers a timeout mid-read, and asserts the connection is closed. This fails 10/10 on the old code path and passes 10/10 with the fix.