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

Closed
AL
3mo ago
+1171-152

After a failure, a RedisClient could be left half alive in several ways: failed set but the socket still open and connected still true, a connect() promise that never settled, a process kept alive by a client that had given up, or a reconnect started from onclose being fed the previous connection's replies or closed again by the code that was still unwinding. This makes one invariant hold on every failure path: once the client has failed, or a dial has failed, the socket is gone, the client reads as disconnected, ValkeyClient::on_close() runs exactly once for it (settling the connect() promise, running onclose, applying the retry policy and re-evaluating the event loop ref), and a connect() issued from onclose or from a rejection handler starts a fresh attempt that nothing else interferes with.

What funnels into on_close() now:

  • fail() (idle timeout, protocol error, a rejected HELLO or SELECT, a failed TLS handshake) always closes the socket, clears is_reconnecting, and closes with CloseCode::Failure; on_close() reads failed to skip the retry policy, rather than fail() marking the client manually closed, because duplicate() copies that flag and a duplicate of a failed client should still reconnect. Failure is the one close usockets never defers: Normal waits for the peer's close_notify and even a fast shutdown is held back while the socket still owns undelivered ciphertext, which is exactly the peer that stopped reading. On TCP that is an RST instead of a FIN, which costs nothing once everything on the connection has been rejected. disconnect() and the finalizer use the fast shutdown, so a user close() over rediss:// also no longer waits for the peer to answer close_notify.
  • The socket close and connect-error handlers set Disconnected before on_close() rather than in a defer after it, so a connect() from onclose actually dials and its promise settles; the defer also used to overwrite the status of the attempt such a connect() had started.
  • A dial that fails before there is a socket (a connect(2) failing outright, whether for the first dial made by connect() or by the first command or for a retry, or a TLS context that cannot be built) queues the close path on the event loop. The retry used to call onclose inline, which recursed when onclose re-dialled, and settled nothing; the first dial used to reject on the spot with ERR_SOCKET_CLOSED_BEFORE_CONNECTION, with no onclose and no retry policy, and left the rejected promise cached for a later successful dial to settle again. Until that task runs the client stays Connecting, as with a real dial, so a connect() or close() issued in the meantime joins or cancels the attempt instead of dialling on top of it or being ignored. The task is a second mode of ValkeyDeferredClose, so a VM that tears down with it queued releases the client ref and the event loop ref without running script.
  • on_valkey_reconnect() disarms the connection timer of the attempt that just failed; left armed it fired during the retry delay, and with is_reconnecting now cleared by fail() the retry would then never run and connect() never settle.
  • on_data() stops handling a read as soon as the socket it came from is no longer the client's socket, so replies left over from a failed connection are not taken as the next connection's HELLO reply; fail_handshake no longer closes a second time after fail() has already closed; on_close() frees a half-received reply, which otherwise counted as pending activity and kept the process alive after close(); the timer connect() arms is re-armed with the idle interval once HELLO is accepted and again on every read while connected, so idleTimeout counts idle time instead of the connection dying connectionTimeout after it connected, however busy it was.

Visible behaviour changes: client.connected is false inside onclose when the server drops an established connection (it used to still read true there); a failure the client detects itself after HELLO (idle timeout, protocol error) now really closes the client and runs onclose; as before it is not retried even with autoReconnect on, since only peer-initiated closes go through the retry policy, which is the one place this client knowingly differs from ioredis (an explicit connect() still works afterwards); a first dial that fails outright rejects with ERR_REDIS_CONNECTION_CLOSED from the event loop, runs onclose and follows the retry policy like a refused one, instead of rejecting synchronously with ERR_SOCKET_CLOSED_BEFORE_CONNECTION; close() or a failure over rediss:// completes immediately instead of after the peer's close_notify; and a busy connection is no longer closed connectionTimeout after connecting, only an idle one after idleTimeout.

Tests are in the "Recovering After fail()" block of connection-failures.test.ts, against net/tls stubs, unix sockets and closed ports, one per path above plus the visible changes (the idle timeout on its own entry point, connected inside onclose, the terminal-after-HELLO policy, close() over TLS against a peer that never answers close_notify, a failure while the peer has stopped reading, the reconnect window, the event-loop re-dial after a TLS context failure, the first dial failing outright from connect() and from a command with auto reconnect off and on, a pub/sub listener that closes and reconnects while a further push sits in the read buffer behind its message, a rejected SELECT after an accepted HELLO, a duplicate of a failed client reconnecting, an idle connection closing and server data holding the idle timer off), and worker-terminate-lifetime.test.ts tears a VM down with the deferred close still queued (on debug and ASAN builds, once with a command queued behind it) and lets the same close run on the main thread, where the process then exits on its own. Each was checked to fail against the tree without its fix.

This supersedes #33479 and #33473 (close both by hand when this lands; a PR number in a Closes line does nothing) and covers the is_reconnecting part of #32779 and the read_buffer part of #33104, which need rebasing onto it; #38794 was the TLS-context fix above and is already closed. Follow-ups filed separately: close() and connect() during the retry delay (both pre-existing), and the same Normal close from fail() in the postgres and mysql clients.

no test proof · iteration 11 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/valkey/reliability/connection-failures.test.ts test/js/web/workers/worker-terminate-lifetime.test.ts