valkey: close the socket on every fail() and mark the client disconnected before onclose runs

Merged
AL
3mo ago
+581-45

This is the second PR of a stack split out of #37993. The first is #39513. The third is #38281. This PR does not depend on #39513. #38281 is stacked on this PR.

Part of #18895. The stack as a whole fixes it; #38281 closes it.

The problem

When a RedisClient failed, it did not close the socket. The client was then half alive:

  • failed was set, but the socket stayed open.
  • client.connected still read true.
  • The connect() promise never settled.
  • If onclose called connect(), the new connection could receive replies from the old one.
  • Code that was still unwinding from the failure could close the new connection.

The rule this PR sets

After a failure, four things are always true:

  • The socket is closed.
  • The client reads as disconnected.
  • The close path runs exactly once. It settles the connect() promise, runs onclose, applies the retry policy, and updates the event loop ref.
  • A connect() called from onclose starts a fresh attempt. Nothing from the old attempt touches it.

What changed

fail() now always closes the socket. It uses CloseCode::Failure. It also clears is_reconnecting.

Why Failure: it is the only close code that runs the close callback before close() returns. Normal waits for the peer to answer close_notify. FastShutdown waits while the socket still holds unsent TLS data. A peer that stopped reading never lets either finish. On TCP, Failure sends RST instead of FIN. That is fine, because every command on the connection is already rejected.

disconnect() and the finalizer use FastShutdown. So a user close() over rediss:// no longer waits for the peer.

The close path reads failed to skip the retry policy. It does not use is_manually_closed for this. duplicate() copies is_manually_closed, and a duplicate of a failed client must still reconnect.

The socket close handler and the connect error handler now set the status to disconnected before the close path runs, not after. Before, a connect() from onclose saw the old status and did not dial. The old code also overwrote the status of the new attempt.

When a retry is scheduled, the connection timer of the failed attempt is disarmed. Before, it fired during the retry delay. With is_reconnecting now cleared by fail(), that stopped the retry and connect() never settled.

A read is dropped as soon as the socket it came from is no longer the client's socket. So replies from a failed connection are not read as the next connection's HELLO reply.

The TLS handshake path and the rejected HELLO path no longer close a second time after fail(). The second close hit the socket that onclose had just opened.

Visible changes

  • client.connected is false inside onclose when the server drops an established connection. Before, it read true.
  • A failure the client detects after HELLO (idle timeout, protocol error) now closes the client and runs onclose. As before, it is not retried, even with autoReconnect on. Only closes from the peer go through the retry policy. This is one known difference from ioredis. An explicit connect() still works after it.
  • close() or a failure over rediss:// completes at once. Before, it waited for the peer's close_notify.

Tests

The tests are in the "Recovering After fail()" block of connection-failures.test.ts. They run against net and tls stubs and closed ports, over redis:// and rediss://. Two of them use a TLS peer that never answers close_notify, and one that has stopped reading. The rest of test/js/valkey that runs without a container is unchanged.

Not in this PR

  • A test for the connection timeout during a TLS handshake.
  • Disarming the connection timer on the final close path, not only on the retry path.

Both are follow-ups.

Supersedes #33479. Covers the is_reconnecting part of #32779. #39015 (postgres and mysql) shares the CloseCode note.