fix: Improve cluster connection pool logic when disconnecting

[martinslota]

This PR is a port of redis/ioredis#1864 and what follows below is a verbatim copy of that PR's description. A reproduce for the bug described here that uses valkey_server and iovalkey instead of redis_server and ioredis, respectively, can be found in this repository.

Motivation and Background

This is an attempt to fix errors occurring when a connect() call is made shortly after a disconnect(), which is something that the Bull library does when pausing a queue.

Here's a relatively minimal way to reproduce an error:

import IORedis from "ioredis";

const cluster = new IORedis.Cluster([{ host: "localhost", port: 6380 }]);

await cluster.set("foo", "bar");

const endPromise = new Promise((resolve) => cluster.once("end", resolve));
await cluster.quit();
cluster.disconnect();
await endPromise;

cluster.connect();
console.log(await cluster.get("foo"));
cluster.disconnect();

Running that script in a loop using

#!/bin/bash

set -euo pipefail

while true
do
    DEBUG=ioredis:cluster node cluster-error.mjs
done

against the main branch of ioredis quickly results in this output:

/Code/ioredis/built/cluster/index.js:124
                    reject(new redis_errors_1.RedisError("Connection is aborted"));
                           ^

RedisError: Connection is aborted
    at /Code/ioredis/built/cluster/index.js:124:28

Node.js v20.11.0

My debugging led me to believe that the existing node cleanup logic in the ConnectionPool class leads to race conditions: upon disconnect(), the this.connectionPool.reset() call will remove nodes from the pool without cleaning up the event listener which may then subsequently issue more than one drain event. Depending on timing, one of the extra drain events may fire after connect() and change the status to close, interfering with the connection attempt and leading to the error above.

Changes

• Keep track of node listeners in the ConnectionPool class and remove them from the nodes whenever they are removed from the pool.
• Issue -node / drain regardless of whether nodes disconnected or were removed through a reset() call.
• Within reset(), add nodes before removing old ones to avoid unwanted drain events.
• Fix one of the listeners by using an arrow function to make this point to the connection pool instance.
• Try to fix the script for running cluster tests and attempt to enable them on CI. If this doesn't work out or isn't useful, I'm happy to revert the changes.
• Add a test around this issue. The error thrown in the test on main is seemingly different from the error shown above but it still seems related to the disconnection logic and still gets fixed by the changes in this PR.

[mcollina]

lgtm with a green CI

[martinslota]

Now that I saw the results from CI, I also figured out how to run the unit tests locally. 😅

The tests caught a bug that I then fixed in 788361c and 0deaeba. 😬

Furthermore, some of the existing functional tests were assuming that responding with 'OK' from the mock server would work fine as a response to cluster SLOTS, i.e. that the Cluster client would reach a ready state despite the response being invalid. With the changes made in this PR, the tests were failing because part of connectionPool.reset([]) is now the drain event which leads to the close state instead, after which the client attempts to reconnect over and over again, never reaching the ready state.

In my view, this change in behaviour is desirable since it prevents node clients which are no longer tracked in the connection pool from emitting additional events that could mess with the state of the cluster client (which could easily have moved on, e.g. attempting to reconnect). So for the time being, I adjusted the tests by adding valid slots tables to the mock server in d5d85e9.

Finally, one of the tests was assuming that when a node disappears from the cluster, the node removal (the -node event) would only be emitted after the refreshSlotsCache() method was finished executing. Once again, this assumption is not valid with the changes in this PR because the event is emitted already during the execution of refreshSlotsCache(), and again as part of connectionPool.reset([]). For similar reasons as above, I adjusted the test in d3f83c5 so that it starts listening for a node removal event already before it calls refreshSlotsCache().

Does this approach sound kinda reasonable?

[mcollina]

Yes.

Still LGTM

[martinslota]

As far as I can tell, this should be ready to get merged. I cannot merge it myself.

[mcollina]

Will ship a release asap.