Skip to content

fix(test): raise timeout for cluster PubSub listener-move test#3357

Open
nkaradzhov wants to merge 2 commits into
redis:masterfrom
nkaradzhov:fix/cluster-pubsub-listener-move-timeout
Open

fix(test): raise timeout for cluster PubSub listener-move test#3357
nkaradzhov wants to merge 2 commits into
redis:masterfrom
nkaradzhov:fix/cluster-pubsub-listener-move-timeout

Conversation

@nkaradzhov

@nkaradzhov nkaradzhov commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator

This pull request fixes a flaky CI failure in the cluster PubSub test should move listeners when PubSub node disconnects from the cluster, which has been intermittently failing across many PRs with:

Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

Background: #3347 fixed the earlier CLUSTERDOWN flake by claiming slots on the importing node first, releasing them on the migrating node, then polling both nodes for cluster_state:ok before triggering the MOVED redirect. That removed the CLUSTERDOWN window but made the test body heavier — it reassigns all 8192 slots one-by-one and then polls each node. On slow CI runners that work exceeds mocha's default 2000ms per-test timeout.

testWithCluster ran its it() at the default timeout with no override (only the before hook set 30000ms, which does not apply to the test). This adds an optional testTimeout to the shared CommonTestOptions, applies it in testWithCluster's it() when set, and raises this test's timeout to 30000ms.

Behavior note: the option is guarded (if (options.testTimeout) this.timeout(...)), so all other cluster tests keep the default timeout and are unaffected. The test typically finishes fast; the higher ceiling only covers the worst-case slow-runner path.

🤖 Generated with Claude Code


Note

Low Risk
Changes are limited to test helpers and one cluster integration test; no production client or runtime behavior is modified.

Overview
Stabilizes the cluster PubSub test should move listeners when PubSub node disconnects from the cluster by replacing thousands of per-slot CLUSTER SETSLOT .. NODE calls with clusterDelSlotsRange / clusterAddSlotsRange on the migrating and importing nodes, then polling until both report cluster_state:ok. Slot selection now compares master addresses instead of object identity when picking the half-cluster range to move.

test-utils gains optional testTimeout on shared cluster test options; testWithCluster applies it to the Mocha it() when set. This test uses 30s so slow CI runners are not capped by the default 2s test timeout.

Reviewed by Cursor Bugbot for commit a8c075f. Bugbot is set up for automated code reviews on this repo. Configure here.

nkaradzhov and others added 2 commits July 22, 2026 18:12
The 'should move listeners when PubSub node disconnects from the cluster'
test reassigns all 8192 slots one-by-one and then polls both nodes for
cluster_state:ok. On slow CI runners that work can exceed mocha's default
2000ms timeout, causing intermittent "Timeout of 2000ms exceeded" failures
across many PRs.

testWithCluster ran the it() at the default timeout with no way to override
it (only the before hook set 30000ms). Add an optional testTimeout to the
shared CommonTestOptions, apply it in testWithCluster's it() when set, and
raise this test's timeout to 30000ms. The guard means all other cluster
tests are unaffected.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The 'should move listeners when PubSub node disconnects from the cluster'
test kept exceeding its timeout on slow CI runners. Raising the timeout only
masked the problem; the root cause was two bugs.

1. Slow slot reassignment. The test reassigned an entire 8192-slot half one
   slot at a time with `CLUSTER SETSLOT .. NODE` against both nodes — 16384
   round-trips — which overflowed even the raised 30s timeout. Replace the
   per-slot loop with a single `CLUSTER DELSLOTSRANGE`/`ADDSLOTSRANGE` pair.

2. Wrong ownership check. The range to migrate was selected with
   `cluster.slots[0].master === migrating`, an object-identity comparison that
   did not hold, so the range was chosen incorrectly. The old per-slot
   `SETSLOT NODE` masked this because it reassigns a slot regardless of current
   owner; `ADDSLOTS` does not. Compare by `.address` instead.

`CLUSTER ADDSLOTS` rejects a slot still assigned in the node's own cluster
view, and gossip has not yet propagated the migrating node's release to the
importing node, so release the range on both nodes before claiming it on the
importing node (mirrors the existing sharded-channel-moved test). The existing
`cluster_state:ok` polling continues to cover reconvergence.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant