feat(redis): support Redis Cluster via comma separated seed nodes

[bakiburakogun]

Fixes #1317

Summary

RedisAdapter.createRedisClient() builds a single-node client with createClient(), which does not follow MOVED redirections. Pointed at a Redis Cluster it connects successfully, the Socket.IO Redis Streams adapter sets up, and then roughly half of all operations fail at run time depending on which slot a key hashes to — a silent, partial failure rather than a clean one.

This treats a comma separated REDIS_URL as a list of cluster seed nodes:

STORAGE_STRATEGY=redis
REDIS_URL=redis://node1:6379,redis://node2:6379,redis://node3:6379

A single URL behaves exactly as before. createCluster() comes from the redis package already in dependencies, so there is no new requirement.

Credentials given on the first seed are passed as cluster defaults, because cluster discovery reports the remaining nodes without auth and they would otherwise be rejected.

Testing

Against a 3-master / 3-replica cluster on a Debian 12 host, writing 40 keys:

client	successful	failed
createClient() pointed at one node (before)	17/40	23/40 — MOVED 8308 127.0.0.1:7002
createCluster() with three seeds (after)	40/40	0

Reads afterwards returned 40/40.

I also ran the real websocket server from this branch against that cluster, with STORAGE_STRATEGY=redis and three seeds, and connected two Socket.IO clients with valid JWTs to the same board. Both received init-room, room-user-change and user-joined for each other, so room state and the Streams adapter both work through the cluster client.

One rough edge I did not fix

With a cluster client, the first command issued before the slot map is loaded throws rather than being queued, and I saw exactly one such error at startup:

Failed to write heartbeat: TypeError: Cannot read properties of undefined (reading 'master')
    at RedisClusterSlots.getClient (.../cluster/cluster-slots.js:108:59)

It happened once, did not recur, and nothing downstream was affected — the server served boards normally afterwards. The cause is that ServerService starts the connection without awaiting it (this.redisClient.connect().catch(...)), which a single-node client tolerates because it queues commands. Making startup await the connection would fix it properly, but that changes the constructor's shape, so I left it out of this PR rather than restructure startup on your behalf. Happy to follow up with that if you would like it.

Note for administrators

Redis Cluster only has database 0, so the /database_number suffix cannot be used to separate whiteboard keys from other users of the same cluster. I mentioned this in the README next to the cluster example.

[github-actions]

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

[hweihwang]

Thanks for the PR and the testing notes. All 40 reads and writes passed locally, but cleanup and shutdown fail with the cluster client.

I've left four findings below. Please add tests for those cases and check that existing single-node and Unix socket setups still work.

[hweihwang]

The redis@4 cluster client has no scanIterator(), which RoomStateStore uses. Both runSweep() and gracefulShutdown() throw a TypeError against a three-primary cluster.

Please scan each primary through its own client and test cleanup and shutdown. RedisAdapter.clear() also needs updating: Redis rejects a single DEL call when its keys belong to different hash slots.

[hweihwang]

rediss:// enables TLS for the seed connection, but connections to discovered nodes use defaults. Without defaults.socket.tls, those connections use plain TCP, so TLS-only clusters cannot connect.

Please preserve TLS in the defaults and test connections to discovered nodes.

[hweihwang]

redis://:test,password@localhost:6379 is a valid single-node URL. This check treats it as a cluster URL and splits the password, breaking an existing setup.

Please distinguish a list of nodes from commas inside a password, and add a test for this case.

[hweihwang]

Confirmed the startup error you noted: the first heartbeat runs before connect() finishes and throws Cannot read properties of undefined (reading 'master'). The error is caught, so the server can accept clients before Redis is ready.

Please await the connection before starting services that use Redis, and test slow and failed connections.

[bakiburakogun]

All four confirmed, and the cleanup and shutdown failure you hit was the worst of them. Reading it back, the PR was written as if a cluster client were a drop-in for a single one, and it is not.

The includes(',') check. You are right, redis://:test,password@localhost:6379 is a single node whose password contains a comma, and splitting on every comma broke it. The value is now only read as a list when it has more than one part and every part on its own parses as a URL with a scheme and a host. That lives in splitRedisUrls() in #1316, which this branch is stacked on, so the same rule decides both whether to build a cluster client and how the URL is redacted in the log line.

TLS. Also right, and I had the reason wrong in my own comment. rediss:// on a seed only governs the connection to that seed; cluster-slots.ts builds every discovered node from #clientOptionsDefaults({ socket: { host, port } }) merged with options.defaults.socket, so without defaults.socket.tls those go out over plain TCP. The first seed's scheme is now repeated in the defaults alongside its credentials.

scanIterator(). Confirmed against redis@4.7.1: packages/client/lib/cluster/index.ts has no scanIterator and no SCAN at all, while the single client has one. It cannot have one, since a scan only covers the keyspace of the node it runs on. There are three call sites — RedisAdapter.clear() and listValueKeys() / listHashKeys() in RoomStateStore — so rather than special casing each, they now go through scanKeys() in a new Utilities/RedisUtility.js, which scans each primary through nodeClient() on a cluster and delegates to scanIterator() on a single node.

clear() and hash slots. deleteKeys() in the same module deletes one key at a time on a cluster and keeps the single batched DEL for one node.

The startup error. ServerService started the connection and did not await it, and SocketService's constructor kicks off init() immediately, so clusterService.start() could write the first heartbeat before the slots were known. init() now waits on a readiness promise threaded through from ServerService.

One behaviour change worth calling out: a connection that never comes up now stops the server from starting, instead of being logged while it carries on serving clients without Redis. main() already exits on a failed start, so that is where it lands. Say the word if you would rather it kept the old tolerance.

Tests, in tests/integration/:

• redisCluster.spec.mjs — a single URL, a unix socket and a comma in the password all still build a single client; a seed list builds a cluster; the first seed's credentials are decoded into the defaults; TLS is repeated there; both together; and no defaults key at all when the seeds need nothing. Then scanKeys over a single client, over three primaries, and over a nodeClient() that returns a promise; and deleteKeys batching on one node, one call per key on a cluster, and doing nothing for an empty list.
• redisStartupOrder.spec.mjs — a real SocketService with a recording client: no command reaches Redis while the connection is pending, the heartbeat is written once it resolves, and ready rejects when the connection fails.
• redisUrlCredentials.spec.mjs comes from fix(redis): do not log the Redis password #1316 and covers the seed list as you asked there.

I checked each of these fails without its fix rather than trusting that they pass: reverting the comma rule and the TLS default fails three of them, and reverting the awaited connection fails both ordering tests with expected [ 'set' ] to deeply equal [] and promise resolved "undefined" instead of rejecting.

Whole suite is 119 passing across 15 files, and eslint is clean on all eight files this touches. What I have not done is run it against a real three-primary cluster; if you still have yours up, the cleanup and shutdown paths are the ones worth a second look.

Branch rebased onto current main. It is two commits because it sits on #1316 — the first of them is that pull request.

[bakiburakogun]

I found a way to run this against a real three primary cluster after all, so here is what it does rather than what I believe it does.

The setup is three nodes in cluster mode with no replicas, all 16384 slots covered. One caveat up front: the server is Memurai Developer 4.2.3 (API 7.4.9), which is what redis-memory-server installs on Windows, not upstream redis-server. Two of the four findings are client side and hold whatever the server is; the third is a server behaviour and produced the canonical error, but say the word if you want it repeated against upstream before you trust it.

Your findings, reproduced:

client.scanIterator(...) is not a function or its return value is not async iterable
CROSSSLOT Keys in request don't hash to the same slot
Cannot read properties of undefined (reading 'master')

The third one is yours exactly. It comes from issuing a command while connect() is still in flight; awaiting the connection first and running the same command succeeds.

The fixed paths, on the same cluster:

• 40 writes and 40 reads through RedisAdapter, all fine, and the keys landed 17 / 13 / 10 across the three primaries, so this is not one node pretending to be a cluster;
• RedisAdapter.clear() ran clean and left nothing behind, where the single DEL across those keys is refused with the CROSSSLOT above;
• RoomStateStore.listValueKeys() returned all 20 keys and listHashKeys() came back with 20 as well.

Nothing failed. What I still have not covered is TLS: I have no TLS cluster to point it at, so defaults.socket.tls is argued from cluster-slots.ts rather than observed. That is the one piece of this I would like a second pair of eyes on.

The scratch script is not in the branch. The tests that are keep using fakes, so the suite stays runnable without a cluster.