redis: close the socket when the connection fails
What
Bun.RedisClient can end up permanently half-alive: flags.failed is set, every command rejects with ERR_REDIS_CONNECTION_CLOSED, but the socket is still open, .connected still reports true, and onclose never fires. The application is never told the connection is dead.
ValkeyClient::fail() is the single entry point for every connection-level failure: a RESP frame the parser can't read, a non-OK SELECT, the idle timeout, an unrecognised RESP3 push. On an established connection none of them closed the socket.
Repro
A scripted RESP3 server that answers HELLO and then sends a byte that is not a RESP type:
const client = new Bun.RedisClient(url, { autoReconnect: false });
client.onclose = () => console.log("onclose fired");
await client.connect();
try { await client.ping(); } catch (e) { console.log("ping ->", e.code); }
console.log({ connected: client.connected });
ping -> ERR_REDIS_INVALID_RESPONSE_TYPE
{ connected: true } <- socket still open, onclose never fired
Every later command rejects, forever, and nothing ever tells the app.
Cause
// src/runtime/valkey_jsc/valkey.rs
self.flags.failed = true;
let val = Self::reject_all_pending_commands(..);
if !self.connection_ready() { // is_authenticated && !is_selecting_db_internal
self.flags.is_manually_closed = true;
self.close();
}
The socket was only closed while the handshake was still in flight. Once is_authenticated flipped, fail() rejected the queues and returned, leaving a live socket attached to a client that refuses to use it. status stays Connected because only the socket's on_close callback moves it to Disconnected, and that callback is the one thing that runs the onclose / reconnect policy.
Fix
Close the socket, and mark the close as deliberate:
self.flags.is_manually_closed = true;
self.close();
Deliberate, rather than letting it fall into auto-reconnect, because every failure that reaches fail() is the client's own and recurs on reconnect. handle_hello_response() resets retry_attempts = 0 on each successful HELLO, so a post-handshake failure can never reach maxRetries — it would re-dial forever. Measured, with the reconnecting variant of this patch and redis://host/99 (bad DB index): 22 handshakes in 1.5s, onclose never fired. With the patch as it stands: one handshake, one onclose, connected === false.
What does not change:
- A socket the peer drops never goes through
fail()—on_close()'s reconnect branch handles it — soautoReconnectstill covers the case it exists for. Verified against a scripted server that destroys the connection: 2 handshakes, client reconnects, identical tomain. connect()revives a failed client (it clearsfailedandis_manually_closed), which is whattest/regression/issue/29925.test.tsalready pins.close()is idempotent (it early-returns on a detached or already-closed socket), so the callers that had closed the socket before reachingfail()(on_closeitself,on_connect_error,fail_handshake, the TLS-context failure) are unaffected.
fail() callers, and what each does now
| caller | before | after |
|---|---|---|
handle_hello_response (bad auth, RESP2 server) | close, terminal | unchanged |
authenticate (OOM writing HELLO/SELECT) | close, terminal | unchanged |
on_close (3 branches), on_connect_error | socket already gone, no-op | unchanged |
fail_handshake, TLS-context failure | close, terminal | unchanged |
connection timeout (on_timer, pre-auth) | close, terminal | unchanged |
on_data parse error (post-auth) | stranded | close, onclose, terminal |
SELECT failure (error / non-OK / wrong type) | stranded | close, onclose, terminal |
idle timeout (on_timer, post-auth) | stranded | close, onclose, terminal |
| unrecognised RESP3 push on a subscriber | stranded | close, onclose, terminal |
Verification
test/js/valkey/reliability/connection-teardown.test.ts drives both branches of the policy against an in-process RESP3 server (no Redis needed). Both tests hang on main (the close never arrives) and pass with the fix. The autoReconnect: true test asserts onclose fires — which only happens on the terminal branch of on_close, never on the reconnecting one — and that exactly 2 handshakes are seen, pinning the absence of a re-dial storm.
test/js/valkey/ is green, and test/regression/issue/29925.test.ts (which spawns a real redis-server and exercises flags.failed through close()/connect()) still passes.
Relationship to #32858
This is the other half of the same report. #32858 fixes the trigger in the subscriber path — a pmessage/smessage/invalidate push made the client call fail() on itself — and rewrites the push-kind dispatch so those frames stop failing anything. It deliberately leaves fail() alone.
This PR fixes what fail() does once it is reached, from any of its callers, so no code path can strand a client in failed && connected. The two touch different functions and do not conflict.