Fixes #3366 - #3368
Conversation
Address infinite MOVED error loops in Azure Managed Redis deployments by implementing two key fixes: 1. Symmetric error handling: MOVED errors now extract the target node address and attempt direct connection, matching the behavior of ASK error handling. 2. Forced connection invalidation: When MOVED errors are detected, the corrupted client connection is forcefully disconnected before topology rediscovery. This prevents corrupted connections from being reused when the cluster topology hasn't changed. These changes ensure that connection corruption is not persistent, and clients can recover automatically without requiring process restarts. Also includes: - Documentation updates to docs/clustering.md with troubleshooting section for MOVED errors - Azure Managed Redis configuration examples for private endpoint setup - Guidance on using nodeAddressMap for internal IP address mapping
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f45951c9a4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| // <num-args> is the number of pairs, so it must be `pairs`. | ||
| parser.push(pairs.toString()); |
There was a problem hiding this comment.
For any FT.SEARCH/FT.AGGREGATE/FT.EXPLAIN/FT.HYBRID call with PARAMS, this now sends the length as the number of pairs. Redis expects the count of following arguments, so one parameter must be encoded as PARAMS 2 name value and two parameters as PARAMS 4 ...; with PARAMS 1 name value, the value is parsed as the next option or the command is rejected, breaking parameterized searches. The unchanged aggregate/explain specs in this package still encode one pair as 2.
Useful? React with 👍 / 👎.
| // we force a reconnection by invalidating the client. | ||
| // This ensures that on the next rediscover, a fresh connection will be created. | ||
| try { | ||
| client.disconnect(); |
There was a problem hiding this comment.
When a MOVED comes from a node that still owns other slots, this destroys the RedisClient but leaves the slot node's client reference in RedisClusterSlots.nodeByAddress. The rediscovery path reuses existing nodes, and nodeClient() returns node.client without checking isOpen, so later commands routed to the old source can receive the destroyed client and fail with ClientClosedError instead of recovering. Clear or remove the node client in the slot state before rediscovering if a fresh connection is required.
Useful? React with 👍 / 👎.
| * | ||
| * Default: disabled. | ||
| */ | ||
| topologyRefreshInterval?: number | false; |
There was a problem hiding this comment.
This new public option is documented as periodically refreshing cluster topology, but it is not read anywhere else in the repository, so setting it has no effect. Users who configure it to handle silent topology changes will still never schedule periodic rediscover() calls; either implement the timer or avoid exposing the option until it works.
Useful? React with 👍 / 👎.
| const redirectTo = await this._slots.getMasterByAddress(address); | ||
| if (!redirectTo) { | ||
| await this._slots.rediscover(client); | ||
| redirectTo = await this._slots.getMasterByAddress(address); |
There was a problem hiding this comment.
For an ASK redirect whose target node is not already in nodeByAddress, this changed redirectTo to const but the recovery path still assigns to it after rediscovery. That path is needed when a migrating slot points at a node the client has not learned yet, and it now fails before retrying the command, so keep this binding mutable while preserving the existing rediscover fallback.
Useful? React with 👍 / 👎.
| const values = Object.values(sentinel._self as unknown as Record<string, unknown>); | ||
| const found = values.find(v => (v as { constructor?: { name?: string } } | undefined)?.constructor?.name === 'RedisSentinelInternal'); | ||
| if (!found || typeof (found as { transform?: unknown }).transform !== 'function') return undefined; |
There was a problem hiding this comment.
These new tests try to find RedisSentinelInternal via Object.values(sentinel._self), but #internal is a JavaScript private field and is not an enumerable property. In the package test runner this leaves internal undefined and the following assert.ok(internal, ...) fails before transform() is exercised, so the sentinel test suite cannot pass with these tests as written.
Useful? React with 👍 / 👎.
| this.#sentinelNodeListKey(mergedSentinelList) !== | ||
| this.#sentinelNodeListKey(this.#sentinelRootNodes) |
There was a problem hiding this comment.
When the root list already contains both stale IP seed sentinels and discovered hostname sentinels, #mergeSentinelNodes() returns the same host:port set in the better order, with discovered nodes first. This comparison sorts both lists, so it treats that reorder as no change and skips assigning mergedSentinelList, leaving observe() to try the stale seeds first on every future reset.
Useful? React with 👍 / 👎.
| try { | ||
| parsed_url = new URL(url); | ||
| } catch { | ||
| throw new TypeError(`Invalid URL: ${url}`); |
There was a problem hiding this comment.
When parseURL() receives a malformed Redis URL that contains credentials, this new error message copies the full URL into err.message. Applications commonly log configuration errors by message, so a typo in a password or username can now leak the secret; keep the helpful validation but avoid including the raw connection URL.
Useful? React with 👍 / 👎.
nkaradzhov
left a comment
nkaradzhov
left a comment
There was a problem hiding this comment.
@aartisonigra, thanks, the underlying issue is real and passing the slot number from getClientAndSlotNumber into _executeMulti and _executePipeline is the right direction.
However, this PR bundles several unrelated changes and some are even incorrect.
Please leave only the slotNumber plumbing plus a regression test, and ensure a MULTI chain is relocated only when the entire chain is still in the write queue.
…ect slot migration
f45951c to
491f04f
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
Reviewed by Cursor Bugbot for commit 491f04f. Configure here.
| * | ||
| * Default: disabled. | ||
| */ | ||
| topologyRefreshInterval?: number | false; |
There was a problem hiding this comment.
Unused topologyRefreshInterval option silently does nothing
Medium Severity
The new topologyRefreshInterval option is declared in the RedisClusterOptions interface with JSDoc claiming it "periodically refreshes the cluster topology in the background," but it is never read or consumed by any implementation code. Users who set this option would believe periodic refreshes are active when nothing is actually happening, potentially masking topology drift issues in production.
Reviewed by Cursor Bugbot for commit 491f04f. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 491f04f617
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // "redis://default:p@ssword@localhost:6379" — the extra @ causes the WHATWG | ||
| // URL parser to misparse the authority, leaving hostname empty. | ||
| assert.throws( | ||
| () => RedisClient.parseURL('redis://default:p@ssword@localhost:6379'), |
There was a problem hiding this comment.
With Node's WHATWG URL parser, redis://default:p@ssword@localhost:6379 is valid: the @ is treated as part of the userinfo and percent-encoded, leaving hostname as localhost. As a result RedisClient.parseURL returns successfully here and this assert.throws fails whenever the client specs run; use a truly malformed unescaped credential character such as #/?, or assert successful parsing for @ instead.
Useful? React with 👍 / 👎.
| if (!hostname) { | ||
| throw new TypeError( | ||
| `Invalid URL: host is empty. If the password contains special characters (e.g. @, #, :), percent-encode them first (e.g. encodeURIComponent(password)).` |
There was a problem hiding this comment.
This check now rejects URLs such as redis:///0 or redis://, even though the public URL shape in this file makes the host optional and the previous parser accepted them, allowing Node's socket defaults to connect to localhost while still selecting the requested DB. In deployments that use a hostless URL to mean the default Redis host, upgrading will fail during option parsing with the percent-encoding hint instead of connecting.
Useful? React with 👍 / 👎.
| const { client, slotNumber } = await this._self._slots.getClientAndSlotNumber(firstKey, isReadonly); | ||
| return client._executePipeline(commands, undefined, slotNumber); |
There was a problem hiding this comment.
This passes the slot derived from only the first/routing key to every command in the pipeline. In the SMIGRATED path, extractCommandsForSlots() moves queued writes solely by value.slotNumber, so a pipeline whose keys are in different slots but currently on the same source master will have all of its commands tagged as the first slot; when only that first slot migrates, commands for slots that did not move are prepended to the destination node and can fail with MOVED even though they should have stayed on the source. Store/pass a per-command slot instead of reusing the batch routing slot.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8a4b233b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -1,1690 +0,0 @@ | |||
| import { EventEmitter } from 'node:events'; | |||
There was a problem hiding this comment.
This deletion removes the ./lib/sentinel module, but packages/client/index.ts still imports the default and named exports from ./lib/sentinel, and the sentinel tests/helpers import ./index/. as well. After this commit, building or importing @redis/client cannot resolve packages/client/lib/sentinel, so createSentinel and RedisSentinelClient are effectively broken; restore this file or update all of those exports/imports to the replacement module.
Useful? React with 👍 / 👎.
| // we force a reconnection by invalidating the client. | ||
| // This ensures that on the next rediscover, a fresh connection will be created. | ||
| try { | ||
| client.disconnect(); |
There was a problem hiding this comment.
When a normal resharding MOVED reply is received while this shared node client still has other commands queued or waiting for replies, this disconnect destroys the client and RedisClient.destroy() flushes the whole command queue with DisconnectsClientError. Those unrelated in-flight commands are not retried through the cluster redirection path, so concurrent traffic to the same node can fail just because one command saw a MOVED; avoid tearing down the shared client here unless its queued commands are drained or re-enqueued.
Useful? React with 👍 / 👎.
|
@aartisonigra thans for the effort! Unfortunately the PR as a whole can't be merged in its current shape, and a competing PR (#3377) already covers the same fix more completely. Closing this one |
2 participants
Problem
MULTI/pipeline commands have no slotNumber, so during full-node slot migration (extractAllCommands), the whole batch can be routed to the wrong node (lastDestNode), causing MOVED errors or aborted transactions.
Changes
Note
High Risk
Removes the entire Sentinel client surface while exports may remain, and changes cluster MOVED/MULTI routing behavior in production-critical paths.
Overview
Cluster MULTI/pipeline (#3366):
MULTI()now passes the routing key’s slot number into_executeMulti/_executePipeline, and those methods forward it on every queuedaddCommand. During slot migration, commands without a slot were ignored or mis-routed; tagged batches stay on the correct shard.MOVED recovery (#3256): On
MOVED, the client disconnects the failing connection, rediscover runs, then retry prefers the master from the error (with slot fallback via the key).RedisClient.parseURL(#2857): WrapsURLparsing, rejects empty host with a percent-encode hint for passwords containing@, plus tests.Breaking: The full
lib/sentinelimplementation (client, commands, tests) is deleted in this diff whilecreateSentinelmay still be exported from the package entry—verify build/exports.Other: New optional
topologyRefreshIntervalon cluster options; placeholderMOVED-recovery.spec.ts; minorASKhandlerconsttweak.Reviewed by Cursor Bugbot for commit ba32962. Bugbot is set up for automated code reviews on this repo. Configure here.