[#3127] Fix for Redis Sentinel client doesn't reconnect after connection failure#3184
Open
mayankkapoor wants to merge 2 commits intoredis:masterfrom
Open
[#3127] Fix for Redis Sentinel client doesn't reconnect after connection failure#3184mayankkapoor wants to merge 2 commits intoredis:masterfrom
mayankkapoor wants to merge 2 commits intoredis:masterfrom
Conversation
…ent fails to reconnect, throwing `"None of the sentinels are available"`. This is a regression introduced in v5.9.0 (commit `73413e086c` — "fix: resolve doubly linked list push issue (redis#3085)"). **Root Cause:** `#handleSentinelFailure()` (added in v5.9.0) permanently removes the sentinel node from `#sentinelRootNodes` via `splice()` when its connection drops. When `#reset()` then calls `observe()`, the sentinel list is empty (or missing that node), so reconnection is impossible. **Secondary issue:** The sentinel list update in `transform()` (line 1350) compares only array lengths (`analyzed.sentinelList.length != this.#sentinelRootNodes.length`), so even if reconnection succeeds via another sentinel, a removed node may never be restored if the list sizes happen to match. - [ ] **Task 1:** Fix `#handleSentinelFailure` — stop permanently removing sentinel nodes - **File:** `packages/client/lib/sentinel/index.ts` (lines 934–942) - Remove the `splice()` call. A transient TCP disconnect (which happens during sentinel restart) should not permanently delete the node from the root list. Just call `#reset()` to trigger reconnection, matching v5.8.3 behavior. - Before (broken): ```typescript #handleSentinelFailure(node: RedisNode) { const found = this.#sentinelRootNodes.findIndex( (rootNode) => rootNode.host === node.host && rootNode.port === node.port ); if (found !== -1) { this.#sentinelRootNodes.splice(found, 1); } this.#reset(); } ``` - After (fixed): ```typescript #handleSentinelFailure(node: RedisNode) { this.#reset(); } ``` - [ ] **Task 2:** Fix sentinel list update condition in `transform()` - **File:** `packages/client/lib/sentinel/index.ts` (line 1350) - Replace the length-only comparison with a proper content comparison so that nodes removed or added are always detected. - Before: ```typescript if (analyzed.sentinelList.length != this.#sentinelRootNodes.length) { ``` - After — compare actual content: ```typescript if (!areSentinelListsEqual(analyzed.sentinelList, this.#sentinelRootNodes)) { ``` - Add a helper (either inline or as a small private method) that compares host+port of each node, not just array length. - [ ] **Task 3:** Run existing tests to confirm no regressions - Run `npm test` in the `packages/client` directory - Look for and run any sentinel-specific test files - [ ] **Task 4:** Verify the fix scenario - Confirm `#sentinelRootNodes` retains the original sentinel entry after disconnect - Confirm `observe()` retries that sentinel and succeeds once it's back online - Confirm no `"None of the sentinels are available"` error after sentinel restart
|
Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset. In case there are security findings, they will be communicated to you as a comment inside the PR. Hope you’ll enjoy using Jit. Questions? Comments? Want to learn more? Get in touch with us. |
mayankkapoor
changed the title
Exports areSentinelListsEqual and adds tests to prevent the regression where sentinel nodes were removed from the root list on failure, and where sentinel list changes were only detected by length, not content. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Collaborator
|
@mayankkapoor thanks for the PR, I will have a look |
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
2 participants
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.
Description
When a Redis Sentinel goes down and comes back up, the node-redis client fails to reconnect, throwing "None of the sentinels are available". It seems this is a regression introduced in v5.9.0 (commit 73413e0 — "fix: resolve doubly linked list push issue (#3085)").
Root Cause:
#handleSentinelFailure()(added in v5.9.0) permanently removes the sentinel node from#sentinelRootNodesviasplice()when its connection drops. When#reset()then callsobserve(), the sentinel list is empty (or missing that node), so reconnection is impossible.Secondary issue: The sentinel list update in
transform()(line 1350) compares only array lengths (analyzed.sentinelList.length != this.#sentinelRootNodes.length), so even if reconnection succeeds via another sentinel, a removed node may never be restored if the list sizes happen to match.Fix:
Fixes #3127
Checklist
npm testpass with this change (including linting)?