Fix cluster publish forwarding when peer connection is down#1931
Open
vazois wants to merge 6 commits into
Open
Fix cluster publish forwarding when peer connection is down#1931vazois wants to merge 6 commits into
vazois wants to merge 6 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a cluster pub/sub forwarding failure mode where a peer node can remain registered after a gossip timeout while its underlying gossip GarnetClient connection is disposed, causing PUBLISH forwarding to throw on the network thread and potentially degrade process responsiveness.
Changes:
- Add a connectivity guard in
GarnetServerNode.TryClusterPublishto skip forwarding when the gossip client is not connected. - Add a 2-node cluster regression test that shuts down one node and asserts the surviving node remains responsive and
PUBLISHstops failing permanently.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| libs/cluster/Server/Gossip/GarnetServerNode.cs | Adds an IsConnected check to avoid forwarding publishes through a disconnected/disposed gossip client. |
| test/cluster/Garnet.test.cluster/ClusterPubSubForwardTests.cs | Adds a regression test covering peer shutdown during publish forwarding and server responsiveness. |
Comment on lines
+86
to
+90
| Assert.DoesNotThrow( | ||
| () => pubRedis.GetServer(publisherEndpoint).Execute("PUBLISH", channelName, message), | ||
| "Baseline publish threw while both nodes were up"); | ||
| ClassicAssert.IsTrue(delivered.Wait(TimeSpan.FromSeconds(15)), | ||
| "Baseline publish was not forwarded/delivered while both nodes were up"); |
Comment on lines
+174
to
+177
| catch (RedisConnectionException) | ||
| { | ||
| // Transient client-side reconfiguration after peer shutdown; retry. | ||
| } |
|
@vazois thanks for the quick fix. This PR will be backported to stable channel or not? |
…vazois/pubsub-forward-fix
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #1928 and #1929. When a peer cluster node goes down after a gossip timeout, its
GarnetServerNodestays registered while the innerGarnetClientsocket is disposed. Forwarding a publish then dereferenced a disconnected client, throwing on the network thread (NullReferenceException) and cascading into an unresponsive process.Fix
Guard
TryClusterPublishwith!gc.IsConnected: log and skip forwarding when the gossip client is down. The connection is transient and gossip re-establishes it; admins canCLUSTER FORGETor raise the gossip timeout otherwise.Test
ClusterPublishSurvivesPeerNodeShutdown(2-node cluster): baseline cross-node publish, shut down the peer, then assert the survivor stays responsive (PING) and PUBLISH recovers. Timing-insensitive; fails when the fix is reverted (socket closes, 0 recoveries).