Skip to content

test(client): fix flaky cluster PubSub listener-move test - #3347

Merged
nkaradzhov merged 1 commit into
redis:masterfrom
nkaradzhov:fix/flaky-cluster-pubsub-move-listeners-test
Jul 21, 2026
Merged

nkaradzhov merged 1 commit into
redis:masterfrom
nkaradzhov:fix/flaky-cluster-pubsub-move-listeners-test

Conversation

@nkaradzhov

@nkaradzhov nkaradzhov commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Description

Fixes a flaky test: Cluster > PubSub > should move listeners when PubSub node disconnects from the cluster intermittently fails in CI with:

Error: CLUSTERDOWN The cluster is down

The test reassigns all 8192 slots of the PubSub node with CLUSTER SETSLOT <slot> NODE <id>, interleaving the commands to the migrating and the importing node. While the two nodes converge on the new slot ownership, they can pass through a transient cluster_state:fail window, and any command served during that window fails with CLUSTERDOWN The cluster is down. The test itself already carried a TODO: is there a better way to migrate slots without causing CLUSTERDOWN? comment.

This pull request resolves the flakiness by:

  • claiming the slots on the importing node first, then releasing them on the migrating node (instead of interleaving the two), and
  • waiting for both nodes to report cluster_state:ok via CLUSTER INFO before triggering the MOVED redirect.

Test-only change; no runtime behavior is affected. The test passed 10/10 consecutive runs with this change.


🤖 Generated with Claude Code


Note

Low Risk
Test-only change to slot-migration setup in one spec; no runtime or library behavior is affected.

Overview
Fixes intermittent CLUSTERDOWN The cluster is down failures in the cluster PubSub test that simulates a node leaving the cluster by reassigning thousands of slots.

The test no longer interleaves CLUSTER SETSLOT … NODE on the migrating and importing masters. It claims slots on the importing node first, then releases them on the migrating node, and polls CLUSTER INFO until both nodes report cluster_state:ok before issuing the GET that triggers MOVED and the publish/assert path.

Test-only change; no production client behavior is modified.

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

The 'should move listeners when PubSub node disconnects from the cluster'
test reassigned all 8192 slots with CLUSTER SETSLOT ... NODE, interleaving
the commands to the migrating and importing nodes. That leaves the nodes in
a transient cluster_state:fail window, so any command sent while the nodes
converge fails with 'CLUSTERDOWN The cluster is down'.

Claim the slots on the importing node first, then release them on the
migrating node, and wait for both nodes to report cluster_state:ok before
triggering the MOVED redirect.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment on lines +407 to +414
const importingPromises: Array<Promise<unknown>> = [],
migratingPromises: Array<Promise<unknown>> = [];
for (let i = range.start; i <= range.end; i++) {
promises.push(
migratingClient.clusterSetSlot(i, 'NODE', importing.id),
importingClient.clusterSetSlot(i, 'NODE', importing.id)
);
importingPromises.push(importingClient.clusterSetSlot(i, 'NODE', importing.id));
migratingPromises.push(migratingClient.clusterSetSlot(i, 'NODE', importing.id));
}
await Promise.all(importingPromises);
await Promise.all(migratingPromises);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we create the migrating promises only after the importing batch completes? Calling both methods while building the arrays queues both batches immediately, so the current awaits do not enforce the intended ordering.

Suggested change
const importingPromises: Array<Promise<unknown>> = [],
migratingPromises: Array<Promise<unknown>> = [];
for (let i = range.start; i <= range.end; i++) {
promises.push(
migratingClient.clusterSetSlot(i, 'NODE', importing.id),
importingClient.clusterSetSlot(i, 'NODE', importing.id)
);
importingPromises.push(importingClient.clusterSetSlot(i, 'NODE', importing.id));
migratingPromises.push(migratingClient.clusterSetSlot(i, 'NODE', importing.id));
}
await Promise.all(importingPromises);
await Promise.all(migratingPromises);
const importingPromises: Array<Promise<unknown>> = [];
for (let i = range.start; i <= range.end; i++) {
importingPromises.push(importingClient.clusterSetSlot(i, 'NODE', importing.id));
}
await Promise.all(importingPromises);
const migratingPromises: Array<Promise<unknown>> = [];
for (let i = range.start; i <= range.end; i++) {
migratingPromises.push(migratingClient.clusterSetSlot(i, 'NODE', importing.id));
}
await Promise.all(migratingPromises);

@nkaradzhov
nkaradzhov merged commit 8cef97d into redis:master Jul 21, 2026
15 checks passed
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.

2 participants