Repository navigation
fix(cluster): retry sharded pub/sub resubscribe after stale rediscover - #3589
waw666waw666 wants to merge 1 commit into
Conversation
During a live reshard, rediscover() may reuse an in-flight discovery that started before a channel's slot moved. Resubscribing then hits MOVED (or races an uncovered slot) and used to emit sharded-channel-moved-error and drop the subscription permanently (redis#3585). Force a fresh rediscover when the slot still maps to the node that lost the channel, and retry the resubscribe up to maxCommandRedirections.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: bf7e8ff445
ℹ️ 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".
| } | ||
| } | ||
|
|
||
| await redirectTo.extendPubSubChannelListeners( |
There was a problem hiding this comment.
When the first extendPubSubChannelListeners reaches a target but its SSUBSCRIBE is rejected (for example, the target replies MOVED while its topology view is stale, or returns NOPERM), PubSub.extendChannelListeners in client/pub-sub.ts has already stored this channel and its reject handler does not remove it. If the forced discovery selects that same target on the next iteration, this call becomes a no-op and resolves, so the handler returns without a wire subscription and without sharded-channel-moved-error; roll back the failed target's listener entry or otherwise ensure the retry actually reissues SSUBSCRIBE.
Useful? React with 👍 / 👎.
| if (redirectTo === client) { | ||
| continue; |
There was a problem hiding this comment.
If a slot is reassigned back to its original master before this async move handler finishes, such as during a reshard rollback, the forced discovery is fresh but legitimately returns client. The server's earlier SUNSUBSCRIBE already removed the wire subscription and local listener entry, yet this continue refuses to send SSUBSCRIBE; after exhausting the budget it emits an error and permanently drops the channel. After the forced refresh, allow reattachment to the original owner rather than treating object identity as proof of staleness.
Useful? React with 👍 / 👎.
![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 high effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit bf7e8ff. Configure here.
| return; | ||
| } | ||
| await this.#forceRediscover(client).catch(() => {}); | ||
| } |
There was a problem hiding this comment.
Retry skips SSUBSCRIBE after failed resubscribe
High Severity
A failed extendPubSubChannelListeners still registers the channel locally, so a later retry on the same client treats it as already subscribed and skips SSUBSCRIBE. The handler then returns without emitting sharded-channel-moved-error, and the sharded subscription is dropped silently.
Reviewed by Cursor Bugbot for commit bf7e8ff. Configure here.
Fixes #3585
When part of a master's slots move while a
RedisClusterholds shardedsubscriptions, a few channels can be dropped for good. The
sharded-channel-movedhandler rediscovers and resubscribes, butrediscover()reuses an in-flight discovery that may have started beforethis slot moved. The slot map still points at the old master, the
resubscribe gets
MOVED, and the handler used to emitsharded-channel-moved-errorand give up.Fix
rediscover, force a fresh discovery (wait for the in-flight one, then
start a new one).
maxCommandRedirections(same budget as the
SSUBSCRIBEMOVED loop), instead of dropping thesubscription on the first error.
Tests
most of their slots one-by-one, assert every channel receives again and
no
sharded-channel-moved-erroris emitted.#3311/sharded-channel-movedcases still pass.Note
Medium Risk
Touches cluster topology refresh and sharded Pub/Sub reattachment with a new retry loop; behavior is bounded by maxCommandRedirections but could affect live migration edge cases.
Overview
Sharded Pub/Sub resubscribe after slot moves no longer gives up when coalesced
rediscover()returns a stale topology: the handler now loops with the same max redirect budget asSSUBSCRIBE, forcing a fresh discovery (wait for any in‑flightrediscover, then run again) when routing still lands on the node that lost the channel.On repeated failures (stale maps, MOVED races), it retries instead of emitting
sharded-channel-moved-errorimmediately; errors are only surfaced after exhausting the budget.Adds a cluster integration test that migrates many sharded channels concurrently and asserts all subscriptions keep receiving and no moved errors fire.
Reviewed by Cursor Bugbot for commit bf7e8ff. Bugbot is set up for automated code reviews on this repo. Configure here.