redis: arm idleTimeout on an idle timer, not the connect timer
Repro
idleTimeout: 60_000 with connectionTimeout: 250, against a scripted RESP3 server, issuing a command every 25ms:
const c = new Bun.RedisClient(url, { idleTimeout: 60_000, connectionTimeout: 250 });
c.onclose = () => console.log("onclose fired");
await c.connect();
for (let i = 0; Date.now() - t0 < 1200; i++) {
try { await c.get("k" + i); } catch (e) { console.log(Date.now() - t0, e.code, e.message); break; }
await Bun.sleep(25);
}
console.log({ connected: c.connected });
254 ERR_REDIS_CONNECTION_CLOSED Connection has failed
{ connected: true }
The client dies exactly connectionTimeout ms after it connected, while it is busy, and onclose never fires. With the default idleTimeout: 0 the same mis-armed timer fires too, but its handler is a no-op, which is why only opt-in users see it. Previously reported as #18897 and #19044, both closed by changing the default to 0.
Cause
There is one timer. connect() arms it with connection_timeout_ms, and nothing cancels or re-arms it afterwards. on_connection_timeout then decides what the firing means from the status at that moment:
match self.status {
Status::Connected => // "Idle timeout reached after {idle_timeout_interval_ms}ms"
_ => // "Connection timeout reached after {connection_timeout_ms}ms"
}
So the connect-phase timer detonates on a now-connected client and is reported as an idle timeout. Conversely, a connection that really is idle is never closed, because the idle interval is never armed.
The failure path then left the client unusable but undetectable: fail_with_js_value only closes the socket when !connection_ready(), so a connected client kept status == Connected (.connected === true), never fired onclose, never auto-reconnected, and rejected every later command with Connection has failed.
Fix
The two-phase timer mirrors what PostgresSQLConnection already does.
reset_idle_timeout()re-arms the timer only once the handshake completed, so traffic cannot push out the connect deadline. Called after every reply (on_data) and every command (send). The handshake reply lands inon_datatoo, so that is also where the connect-phase timer gets swapped for the idle timer (or cancelled, whenidleTimeoutis 0).- When the idle timer fires, the socket is closed:
oncloseruns,.connectedgoes false, pending commands reject withERR_REDIS_IDLE_TIMEOUT. Auto-reconnect is suppressed (dropping an idle connection is deliberate, reconnecting it immediately would defeat the option), and the client reopens lazily on the next command, like a freshly constructed one. - The timer is cancelled when the connection closes, in
on_valkey_close()andon_valkey_reconnect(), the two hooks every branch ofValkeyClient::on_close()funnels through. Left armed, it fired during the reconnect backoff and rejected the offline queue with aconnectionTimeoutthat never applied, while settingis_manually_closed, whichon_opendoes not clear: auto-reconnect was then off for good. connect()calls the newValkeyClient::reset_for_new_connection()before opening a socket, so a reopened connection does not inherit the closed one'sis_authenticated. Without it,enqueue()sawconnection_ready()and wrote the command straight to a socket that had not opened yet;on_opendropped those bytes and the promise stayed in the in-flight queue, pairing every later reply with the wrong command. That one was already reachable onmainwithoutidleTimeout, viaclose()plus an un-awaitedconnect().statusis set toDisconnectedbeforeon_closeruns JS, soonclosehandlers and rejected promises no longer observe.connected === trueon a dead socket.ValkeyClient::close()already ordered it this way on its semi-socket path.
connectionTimeout now bounds exactly the connect phase, which is what it is documented to do.
Verification
test/js/valkey/valkey-timeout.test.ts (new, talks to an in-process RESP3 server so it needs no redis):
| before | after | |
|---|---|---|
busy connection outlives connectionTimeout | fails at 415ms | pass |
idle connection closed at idleTimeout, reopens on next command | onclose never fires | pass |
| non-pipelineable command reopens after an idle close | resolves with the HELLO map | pass |
enableAutoPipelining: false reopens after an idle close | resolves with the HELLO map | pass |
| server-initiated close leaves no timer armed | rejects with a bogus ERR_REDIS_CONNECTION_TIMEOUT | pass |
command racing an un-awaited connect() gets its own reply | resolves with the HELLO map | pass |
connectionTimeout still fires on a stalled handshake | pass | pass |
The server-close test is deterministic rather than racy: the hangup (270ms), the idle deadline (300ms) and the reconnect (~320ms) are all deadlines in one timer heap, so their order holds even under a stall. 10/10 clean runs.
Also ran the docker-gated suites against a local redis (reliability/recovery.test.ts (#29925), reliability/connection-failures.test.ts, unit/*, integration/complex-operations.test.ts): 84 pass, 1 fail, and that one needs a route to 192.0.2.1 and fails identically on main in the same container.
Manual check against a real redis (pub/sub, manual close, server-side CLIENT KILL + autoReconnect, idle close + reopen)
1 get: 1 connected: true
1 onclose: { code: "ERR_REDIS_CONNECTION_CLOSED", connected: false } # was connected: true
2 after 700ms idle, default idleTimeout: 1 connected: true # connectionTimeout was 200
3 pubsub: hello
4 after server kill, autoReconnect get: 1 connected: true
5 idle close: { code: "ERR_REDIS_CONNECTION_CLOSED", connected: false } after 255 ms
5 reopen on next command: 1 connected: true
6 INFO after close()+connect(): own reply (correct)