feat(client): add MaxCommandRedirectionsError for cluster redirections - #3386
feat(client): add MaxCommandRedirectionsError for cluster redirections#3386Piyush0049 wants to merge 2 commits into
Conversation
![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: b694fe1e61
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
b694fe1 to
303eebd
Compare
nkaradzhov
left a comment
nkaradzhov
left a comment
There was a problem hiding this comment.
Thanks @Piyush0049 ,
he test only covers _execute (cluster.get); the SSUBSCRIBE path is untested — please add a case or note why it's omitted.
|
@nkaradzhov I have pushed a commit adding a case. Please do let me know if it is appropriate or not. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6666d7a2ba
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| }, { | ||
| serverArguments: [], | ||
| numberOfMasters: 2, |
There was a problem hiding this comment.
When the suite is run with a Redis 6 test image (for example via the configurable --redis-tag/--redis-version test options), testWithCluster only skips tests that set minimumDockerVersion, and SSUBSCRIBE is not available before Redis 7. The other sharded Pub/Sub cases in this file are gated with [7]; without the same gate here, this new test runs on Redis 6 and rejects with an unknown-command error instead of MaxCommandRedirectionsError, breaking versioned test runs. Add minimumDockerVersion: [7] to this options block.
Useful? React with 👍 / 👎.
2 participants
Description
This pull request resolves a
// TODO: error classcomment by adding a newMaxCommandRedirectionsError.Previously, when a cluster command exceeded the maximum number of allowed redirections (e.g., getting stuck in a
MOVEDorASKloop), the client simply re-threw the last received raw Redis error. This obscured the actual cause of the failure from the developer.The new error class wraps the original error, making it clear that a redirection limit was hit, thereby improving debugging and observability for cluster topology issues.
Updates in this PR:
MaxCommandRedirectionsErrorto wrap the raw routing errors.testUtils.testWithClusterthat intentionally creates aMOVEDloop to verify theMaxCommandRedirectionsErroris correctly thrown.Checklist
npm testpass with this change (including linting)?Note
Low Risk
Localized cluster error-handling change with clearer failure mode; behavior only differs when the redirection cap is exceeded on redirect errors.
Overview
When cluster command routing or sharded
sSubscribehitsmaxCommandRedirectionswhile still getting ASK/MOVED replies, the client now throwsMaxCommandRedirectionsError(Too many Cluster redirections) with the last redirect ascause, instead of surfacing only the raw Redis error.The
_executeredirect loop rethrows non-Errorvalues immediately, publishes tracing on limit exhaustion, and applies the new error for redirect loops;SSUBSCRIBEuses the same limit behavior for MOVED. Integration tests deliberately create a MOVED loop (lowmaxCommandRedirections) forgetandsSubscribe.Reviewed by Cursor Bugbot for commit 6666d7a. Bugbot is set up for automated code reviews on this repo. Configure here.