redis: cancel the armed auto-reconnect timer in close()

Closed
RO
3mo ago
+174-8

What does this PR do?

RedisClient.close() is ignored while the client is between auto-reconnect attempts. The client reconnects moments later, client.connected flips back to true after the user closed it, and nothing ever settles the commands that were sitting in the offline queue.

Reproduction (scripted in-process RESP3 server, same as the test):

const client = new RedisClient(`redis://127.0.0.1:${port}`, { autoReconnect: true });
await client.connect();

serverSocket.destroy();             // the client arms its 50ms reconnect timer
while (client.connected) await delay(1);

const queued = client.get("orphan"); // goes into the offline queue
client.close();

await delay(500);
client.connected;                    // true  -- the timer reconnected anyway
await queued;                        // never settles

Cause

js_disconnect returned early when status == Disconnected:

if self.client.get().status == valkey::Status::Disconnected {
    return Ok(JSValue::UNDEFINED);
}
self.client_mut().disconnect();

That is exactly the state a client is in between retries: the socket is gone, is_reconnecting is set, and reconnect_timer is armed. close() therefore set no flags and cancelled no timer, and on_reconnect_timer() reopened the connection. The only thing that used to settle the offline queue and a pending connect() promise was the socket's on_close, and there is no socket.

Fix

  • js_disconnect cancels the reconnect timer and always calls disconnect(); disconnect() already skips the socket close when there is nothing to close.
  • ValkeyClient::disconnect() clears is_reconnecting, so a retry already in flight stops retrying.
  • When a cancelled retry leaves no socket behind, js_disconnect runs the close path by hand: fail() rejects the queued commands, notify_close() (split out of on_valkey_close(), minus the deref that balances connect()'s socket keep-alive ref) rejects the pending connect() promise and invokes onclose, and update_poll_ref() drops the event-loop keepalive the retry was holding. The condition is the timer being ACTIVE and the status being Disconnected, which is exactly when disconnect() finds nothing to close. A client that exhausted its retries (no timer, on_valkey_close() already ran) and a client whose explicit connect() left a socket connecting under an armed timer both take the normal socket-close path and report the close once.

This is adjacent to but distinct from #32803, which stops a stale reconnect timer from clobbering an in-flight socket. That one is about a timer firing during an explicit connect(); this one is about a timer firing after an explicit close(). Neither fixes the other.

Verification

test/js/valkey/reliability/connection-failures.test.ts gets two tests on a scripted in-process RESP3 server (no docker needed):

  • close() during an armed retry: the server accepts exactly one connection, connected stays false, the offline-queued command rejects with ERR_REDIS_CONNECTION_CLOSED, and onclose fires once.
  • close() after the retries are exhausted: onclose is not reported a second time.
  • close() during a reconnect attempt (explicit connect() under an armed timer, socket still connecting): onclose fires once and the pending connect() promise rejects.
before / after
# unfixed (src/ stashed), USE_SYSTEM_BUN=1 bun test .../connection-failures.test.ts
error: expect(received).toEqual(expected)
  [
    1,
+   2,
  ]
(fail) Valkey: close() during auto-reconnect > close() cancels the armed retry and settles the offline queue

# with the fix, bun bd test .../connection-failures.test.ts
(pass) Valkey: close() during auto-reconnect > close() cancels the armed retry and settles the offline queue
(pass) Valkey: close() during auto-reconnect > close() after the retries are exhausted does not report a second close

Also checked by hand against a real redis (bun bd, ASAN): basic commands, close() on a fresh client, close() then connect() recovery, onclose firing once, concurrent connect(), auto-reconnect after a server-side CLIENT KILL with the offline queue flushing, and pub/sub. All unchanged, process exits cleanly.