Expected behaviour
Rebuilding the cluster topology after a failover should not orphan existing node connections. Non-persistent connections should be released; clean persistent connections may follow the existing pooling policy.
Actual behaviour
On unmodified develop, node connections accumulate across failovers, including with persistent connections disabled. Calling RedisCluster::close() only closes the currently mapped connections.
With three masters and three replicas, I observed the following counts for connections belonging to the same client, identified by a unique CLIENT SETNAME:
Before failover: 3
After first failover: 6
After second failover: 9
After third failover: 12
After close(): 9
The leak reproduction uses ordinary commands, without WATCH, MULTI, or pipeline mode.
I'm seeing this behaviour on
- OS: Linux x86_64, running locally in Docker
- Redis: 8.8.0
- PHP: 8.5.2
- phpredis:
develop at 146ec813ea7ca85c9e3a7cff2caf977096e16acf
redis.clusters.cache_slots=0
RedisCluster persistent constructor argument: false
Steps to reproduce
Use a disposable local cluster with three masters and one replica per master.
- Create one non-persistent
RedisCluster object and keep it in the same PHP process throughout the test.
- Set a test key, and open a connection to each master using
CLIENT SETNAME with the same unique name. Count matching connections with CLIENT LIST on all six nodes.
- Promote the replica serving the test key using
CLUSTER FAILOVER TAKEOVER.
- Wait for the slot owner to converge across the nodes, then call
GET for the test key through the original object. This triggers the MOVED handling and topology remap.
- Set the client name on all currently mapped master connections again and recount. Repeat for three failovers, waiting for replication to be ready before each promotion.
- Call
close() on the original object and recount before the PHP process exits.
Suspected cause and implementation question
cluster_update_slot() calls cluster_map_keyspace() when MOVED points to a known replica. cluster_map_slots() then clears c->nodes; its node destructor frees the socket structures without disconnecting their streams. The old connections are no longer reachable through the cluster object.
I tested disconnecting before the remap locally. It resolves the leak, but needs care with WATCH: normal disconnect can return a WATCH-active persistent stream to the pool for another client, while force-closing it loses the original client's WATCH state. Neither should silently affect subsequent transactions.
Before proposing a patch, would rejecting this full remap with an exception while WATCH is active be the preferred behaviour? I would like to keep the fix narrow and preserve existing transaction guarantees.
Related: #2025 and #2655.
I've checked
Expected behaviour
Rebuilding the cluster topology after a failover should not orphan existing node connections. Non-persistent connections should be released; clean persistent connections may follow the existing pooling policy.
Actual behaviour
On unmodified
develop, node connections accumulate across failovers, including with persistent connections disabled. CallingRedisCluster::close()only closes the currently mapped connections.With three masters and three replicas, I observed the following counts for connections belonging to the same client, identified by a unique
CLIENT SETNAME:The leak reproduction uses ordinary commands, without WATCH, MULTI, or pipeline mode.
I'm seeing this behaviour on
developat146ec813ea7ca85c9e3a7cff2caf977096e16acfredis.clusters.cache_slots=0RedisClusterpersistent constructor argument:falseSteps to reproduce
Use a disposable local cluster with three masters and one replica per master.
RedisClusterobject and keep it in the same PHP process throughout the test.CLIENT SETNAMEwith the same unique name. Count matching connections withCLIENT LISTon all six nodes.CLUSTER FAILOVER TAKEOVER.GETfor the test key through the original object. This triggers the MOVED handling and topology remap.close()on the original object and recount before the PHP process exits.Suspected cause and implementation question
cluster_update_slot()callscluster_map_keyspace()when MOVED points to a known replica.cluster_map_slots()then clearsc->nodes; its node destructor frees the socket structures without disconnecting their streams. The old connections are no longer reachable through the cluster object.I tested disconnecting before the remap locally. It resolves the leak, but needs care with WATCH: normal disconnect can return a WATCH-active persistent stream to the pool for another client, while force-closing it loses the original client's WATCH state. Neither should silently affect subsequent transactions.
Before proposing a patch, would rejecting this full remap with an exception while WATCH is active be the preferred behaviour? I would like to keep the fix narrow and preserve existing transaction guarantees.
Related: #2025 and #2655.
I've checked
developcommit listed above.