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
…ect slot migration
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 3 potential issues.
Reviewed by Cursor Bugbot for commit f45951c. Configure here.
| } | ||
|
|
||
| // <num-args> is the number of pairs, so it must be `pairs`. | ||
| parser.push(pairs.toString()); |
There was a problem hiding this comment.
PARAMS nargs uses pair count instead of argument count
High Severity
The parseParamsArgument function now pushes pairs.toString() as the nargs value, but Redis FT.SEARCH expects nargs to be the total number of individual name+value arguments (i.e., 2 * pairs), not the number of pairs. The existing AGGREGATE.spec.ts confirms this: 1 pair → nargs='2'. This breaks all commands using parseParamsArgument (FT.SEARCH, FT.AGGREGATE, FT.EXPLAIN).
Additional Locations (1)
Reviewed by Cursor Bugbot for commit f45951c. Configure here.
| client.disconnect(); | ||
| } catch { | ||
| // Ignore errors during disconnect attempt | ||
| } |
There was a problem hiding this comment.
MOVED handler destroys shared client causing collateral failures
High Severity
Calling client.disconnect() (which internally calls destroy()) on every MOVED error destroys the shared client used by all concurrent operations targeting that slot. destroy() calls #queue.flushAll(new DisconnectsClientError()), rejecting ALL pending commands from other callers sharing that client. MOVED errors are normal during cluster resharding and don't necessarily indicate connection corruption — this approach causes severe collateral damage to unrelated concurrent operations.
Reviewed by Cursor Bugbot for commit f45951c. Configure here.
|
|
||
| 3. Should there be a configuration option for forceRefresh behavior? | ||
| - Currently: Always true for MOVED | ||
| - Future: May want to make configurable for performance-sensitive use cases |
There was a problem hiding this comment.
Implementation guide files accidentally committed to repository
Medium Severity
Two large planning/analysis documents (IMPLEMENTATION_GUIDE_3256.md and ISSUE_3256_ANALYSIS.md) for a different issue (#3256) are committed to the repository root. These contain internal implementation plans, rollout strategies, debugging tips, and user workarounds that belong in issue trackers or internal docs, not in the production codebase.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit f45951c. Configure here.
![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.
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
Medium Risk
Touches cluster command routing, MOVED retry paths, and sentinel topology merging—areas that affect production failover and transaction correctness under migration.
Overview
Primary fix (#3366): Cluster
MULTI()/ pipeline execution now passes the routedslotNumberfromgetClientAndSlotNumber()into_executeMulti()/_executePipeline(), and each queued command gets that slot onaddCommand(). That lets slot-migration logic (e.g.extractCommandsForSlots) move whole batches to the correct shard instead of misrouting them when topology shifts.Cluster MOVED handling: On
MOVED, the client disconnects the failing shard client, rediscovers topology, then prefers the master from the error’s address (with slot/slotNumber updates) before falling back to key-based routing. Docs add MOVED troubleshooting and an optionaltopologyRefreshIntervalcluster option.Other behavior changes: Sentinel
transform()merges discovered sentinels with seed nodes via#mergeSentinelNodesso hostname seeds aren’t dropped after outage (#3237), with new regression tests.FT.SEARCHPARAMSnow sends the pair count (not total argv length).parseURLthrows clearer errors for invalid URLs and empty hosts (special characters in passwords).The PR also adds issue #3256 analysis/implementation notes and a placeholder
MOVED-recovery.spec.ts(not theforceRefreshchanges described in those docs).Reviewed by Cursor Bugbot for commit f45951c. Bugbot is set up for automated code reviews on this repo. Configure here.