Repository navigation
[Bug] Cross-node subscriptions lost when a raft snapshot is installed - #173
Merged
wind-c merged 1 commit intoSep 3, 2026
Merged
Conversation
KV.GetAll returns a copy since d5c33e1, so decoding a snapshot into it never reached the live routing table. Both restore paths (hashicorp Fsm.Restore, etcd recoverFromSnapshot) decoded into the throwaway copy, leaving the node with an empty routing table after installing a snapshot. Remote subscriptions stopped receiving forwarded publishes until they re-subscribed. Add KV.Restore which decodes into a fresh map and swaps it in under lock, replacing prior state as raft snapshot semantics require.
Owner
|
Thanks @goingforstudying-ctrl |
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.
Was reading through the cluster raft code and I think the GetAll concurrency fix in d5c33e1 accidentally broke snapshot restore.
KV.GetAllnow returns a copy of the routing table (correctly, it fixed the concurrent map access duringPersist), but both restore paths still decode into its return value:GetAllhands back a pointer to a throwaway copy, so the snapshot decodes into thin air and the live routing table never changes. Both backends have it:hashicorp/fsm.goRestoreandetcd/kvstore.gorecoverFromSnapshot.The visible effect: a node that installs a snapshot comes back with an empty routing table. That happens when a follower falls far enough behind that the leader sends
InstallSnapshot, when a partitioned node rejoins, or on every graceful restart, sincePeer.Stop()takes a snapshot right before shutdown and raft replays it on the way back up. After that,Lookupreturns nothing for those filters,pickNodesfinds no remote nodes, and publishes stop being forwarded to subscribers on other nodes until they happen to re-subscribe. For long-lived bridged clients that may be never. Nothing logs an error, messages just silently don't route.Raft's docs say Restore must discard previous state and replace it with the snapshot, so I added
KV.Restore(io.Reader)which decodes into a fresh map and swaps it in under lock, and pointed both backends at it. Replace rather than merge matters here: a node restoring an older snapshot shouldn't keep filters the snapshot doesn't know about.Repro is a plain round trip through the real Persist/Restore path:
Added round-trip tests for the KV and both backends, including corrupt-snapshot cases (a failed decode leaves the existing state untouched).
go test ./cluster/...all passes, including the existing peer tests that spin up real raft nodes.One thing I left alone:
notifyReplayreplays every restored filter including ones the local node itself owns. That was the behavior before and changing it felt out of scope, but flagging it in case it matters.