redis: arm idleTimeout on an idle timer, not the connect timer

Closed
RO
3mo ago
+338-4

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 in on_data too, so that is also where the connect-phase timer gets swapped for the idle timer (or cancelled, when idleTimeout is 0).
  • When the idle timer fires, the socket is closed: onclose runs, .connected goes false, pending commands reject with ERR_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() and on_valkey_reconnect(), the two hooks every branch of ValkeyClient::on_close() funnels through. Left armed, it fired during the reconnect backoff and rejected the offline queue with a connectionTimeout that never applied, while setting is_manually_closed, which on_open does not clear: auto-reconnect was then off for good.
  • connect() calls the new ValkeyClient::reset_for_new_connection() before opening a socket, so a reopened connection does not inherit the closed one's is_authenticated. Without it, enqueue() saw connection_ready() and wrote the command straight to a socket that had not opened yet; on_open dropped those bytes and the promise stayed in the in-flight queue, pairing every later reply with the wrong command. That one was already reachable on main without idleTimeout, via close() plus an un-awaited connect().
  • status is set to Disconnected before on_close runs JS, so onclose handlers and rejected promises no longer observe .connected === true on 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):

beforeafter
busy connection outlives connectionTimeoutfails at 415mspass
idle connection closed at idleTimeout, reopens on next commandonclose never firespass
non-pipelineable command reopens after an idle closeresolves with the HELLO mappass
enableAutoPipelining: false reopens after an idle closeresolves with the HELLO mappass
server-initiated close leaves no timer armedrejects with a bogus ERR_REDIS_CONNECTION_TIMEOUTpass
command racing an un-awaited connect() gets its own replyresolves with the HELLO mappass
connectionTimeout still fires on a stalled handshakepasspass

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)