feat(client): add MaxCommandRedirectionsError for cluster redirections - #3386
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 👍 / 👎.
SSUBSCRIBE is unavailable before Redis 7; without the gate the test runs on Redis 6 images and fails with an unknown-command error instead of MaxCommandRedirectionsError, breaking versioned test runs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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 error-handling change in cluster redirect paths; behavior only differs when the redirection cap is already exceeded.
Overview
When cluster command routing or sharded
sSubscribehitsmaxCommandRedirectionswhile still seeingMOVED/ASKerrors, the client now throwsMaxCommandRedirectionsError(Too many Cluster redirections) with the last Redis error ascause, instead of surfacing only the raw redirect reply.The general cluster
_executeredirect loop also re-throws non-Errorvalues immediately and only wraps redirect failures at the limit; integration tests induceMOVEDloops (low limit) forGETandsSubscribeto assert the new error type.Reviewed by Cursor Bugbot for commit af0e892. Bugbot is set up for automated code reviews on this repo. Configure here.