fix(test): raise timeout for cluster PubSub listener-move test#3357
Open
nkaradzhov wants to merge 2 commits into
Open
fix(test): raise timeout for cluster PubSub listener-move test#3357nkaradzhov wants to merge 2 commits into
nkaradzhov wants to merge 2 commits into
Conversation
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:Background: #3347 fixed the earlier
CLUSTERDOWNflake by claiming slots on the importing node first, releasing them on the migrating node, then polling both nodes forcluster_state:okbefore triggering the MOVED redirect. That removed theCLUSTERDOWNwindow 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.testWithClusterran itsit()at the default timeout with no override (only thebeforehook set 30000ms, which does not apply to the test). This adds an optionaltestTimeoutto the sharedCommonTestOptions, applies it intestWithCluster'sit()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 .. NODEcalls withclusterDelSlotsRange/clusterAddSlotsRangeon the migrating and importing nodes, then polling until both reportcluster_state:ok. Slot selection now compares master addresses instead of object identity when picking the half-cluster range to move.test-utilsgains optionaltestTimeouton shared cluster test options;testWithClusterapplies it to the Mochait()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.