valkey: cancel the retry timer on close() and connect()

Merged
AL
3mo ago
+414-20

Stacks on #39543 (the socket keep-alive ref moves to the close event's entry); this PR is based on that branch and its diff shows only the timer changes.

The problem

The RedisClient has two timers. reconnect_timer schedules the next retry. timer bounds one attempt (connect timeout) or one idle period. Only the retry path and the finalizer disarmed them. Three things went wrong because of that.

  1. close() during a retry delay did nothing. The client was Disconnected, so close() returned early. The retry timer stayed armed. It dialled a closed client, fired onconnect on it, and kept the process alive for the whole retry schedule.

  2. connect() during a retry delay dialled at once but left the retry timer armed. The timer fired into the in-flight dial and opened a second socket on the same client. Both sockets then drove one state machine.

  3. The connect timer of an attempt that ended in a terminal close stayed armed. A dial that fails outright arms no timer of its own, so the stale one fired "Connection timeout" into the next attempt.

What changed

close() during a retry delay cancels the retry. It disarms reconnect_timer, marks the close as manual, and runs the close path once: the cached connect() promise rejects, onclose runs once, and the event loop ref is released. Nothing dials again. This path takes no ref of its own: the timer's ref goes back with the disarm, and there was never a socket ref to release, which is the accounting #39543 established for a dial that never got a socket.

reconnect() disarms reconnect_timer before it dials. It only dials from Disconnected with no socket. A connect() that takes over a pending retry is now the one dial. A retry timer that fires while a dial is in flight does nothing.

on_valkey_close disarms timer, as on_valkey_reconnect already did. No attempt's timer outlives it.

connect() asserts in debug builds that the client has no live socket, and the deferred close of a dial that failed outright asserts the same instead of returning early on a live socket, since no dial can start during its hold.

Visible changes

close() between retries is honoured. onclose receives ERR_REDIS_CONNECTION_CLOSED with the message "Connection closed". A connect() promise still pending from before the retries rejects with the same error. Queued commands reject with it too. The process can exit.

connect() between retries: if a connect() promise is still pending (the client has not connected yet and its first dial is being retried), it is returned and the retry schedule is left alone. Otherwise the pending retry is cancelled, the client dials at once, and retry counting starts over from zero, so a client one retry from giving up gets a full maxRetries budget again. Before, the dial happened too, but on top of the retry, which then opened a second socket.

Tests

New tests in test/js/valkey/reliability/connection-failures.test.ts:

  • close() during the retry delay fires onclose once, rejects the queued command, and the stub sees no second connection.
  • A spawned process that calls close() during the retry delay exits with one onclose and no second onconnect.
  • close() after the retries are exhausted does not report a second close.
  • connect() during the retry delay opens one connection while HELLO is held past the retry deadline, then resolves.
  • connect() during the retry delay starts the retry budget over: with maxRetries: 2 and a stub that drops every later connection, the client is dropped three more times before it gives up.
  • close() during that in-flight dial reports the close once.
  • The connect timer of an attempt ended by close() does not fire into a next attempt whose dial fails outright in the same loop iteration. The stale deadline is measured from before the first connect(), and the test asserts the callback ran before it, so a slow machine fails the test rather than passing it vacuously.

New case in the ASAN block of test/js/valkey/valkey-gc.test.ts: a worker calls close() during the retry delay and is then terminated; LSan reports nothing, which pins the ref accounting above.

The first, second, fourth and seventh fail on main. The third, fifth and sixth pin behaviour that already held (the fifth passes without the fix too: the stub drops each connection within the 50ms retry delay, so the stale retry timer is re-armed before it can fire). The valkey-gc case fails with the cancel_reconnect of the first commit, which took a ref of its own: LSan reports the Box<JSValkeyClient>.

This supersedes #33306 and #32803. Their tests are kept in adapted form here.

Not in this PR

The failure reason reported to connect() and onclose (#39542). The Connection state enum.

no test proof · iteration 0 · 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/valkey/valkey-gc.test.ts