Skip to content

fix: sync a node that stopped hearing its leader - #709

Open
pfi79 wants to merge 1 commit into
hyperledger:mainfrom
pfi79:node-lost-rejoin-cluster1
Open

pfi79 wants to merge 1 commit into
hyperledger:mainfrom
pfi79:node-lost-rejoin-cluster1

Conversation

@pfi79

@pfi79 pfi79 commented Oct 18, 2026

Copy link
Copy Markdown
Contributor

A node that falls behind a view change without ever asking to change view has no signal that tells it the cluster moved on. It sits in a view nobody is in any more, and it waits there: it is not the leader, so it has no reason to start a view change, and it keeps receiving the heartbeat of the leader it is stuck with, so it has no reason to time out either.

A view change message for the view beyond the next one is what such a node's peers keep sending, but it was recorded and dropped: a view change only accepts messages for the view immediately after the one a node is in, and the view the cluster moved on to is no longer being announced, so there was no other way for the node to find out where the cluster went.

A node that hears a quorum of distinct senders (f+1) announce a view beyond the one it is in now syncs, at most once per announced view, so that a faulty sender cannot make it sync over and over again. The announcement carries no signature, so it only triggers the sync: the view itself is still adopted through the authenticated new view message. The per-view sync bookkeeping is reset through a single helper on every path where the view changes, including a restore from the log.

The scenario is covered end to end by TestNodeBehindOnIdleViewRejoins in test/view_split_test.go. The scope of it is the window in which the follower still hears its old leader: once the whole cluster is connected, the heartbeat responses and the timeouts rescue both nodes anyway, so this change makes a node recover on the announcement instead of waiting for its timeouts.

Based on #700, which fixes the other half of the problem, a node that is one sequence behind the proposal in flight.

A node that falls behind a view change without ever asking to change view has no signal that
tells it the cluster moved on. It sits in a view nobody is in any more, and it waits there: it
is not the leader, so it has no reason to start a view change, and it keeps receiving the
heartbeat of the leader it is stuck with, so it has no reason to time out either.

A view change message for the view beyond the next one is what such a node's peers keep
sending, but it was recorded and dropped: a view change only accepts messages for the view
immediately after the one a node is in, and the view the cluster moved on to is no longer being
announced, so there was no other way for the node to find out where the cluster went.

A node that hears a quorum of distinct senders (f+1) announce a view beyond the one it is in now
syncs, at most once per announced view, so that a faulty sender cannot make it sync over and over
again. The announcement carries no signature, so it only triggers the sync: the view itself is
still adopted through the authenticated new view message. The per-view sync bookkeeping is reset
through a single helper on every path where the view changes, including a restore from the log.

The scenario is covered end to end by TestNodeBehindOnIdleViewRejoins in
test/view_split_test.go. The scope of it is the window in which the follower still hears its old
leader: once the whole cluster is connected, the heartbeat responses and the timeouts rescue both
nodes anyway, so this change makes a node recover on the announcement instead of waiting for its
timeouts.

Based on hyperledger#700, which fixes the other half of the problem, a node that is one sequence behind the
proposal in flight.

Signed-off-by: Fedor Partanskiy <fredprtnsk@gmail.com>
@pfi79
pfi79 force-pushed the node-lost-rejoin-cluster1 branch from f8e0c07 to b1525a6 Compare October 18, 2026 10:48
@HagarMeir

Copy link
Copy Markdown
Contributor

This review was written by Claude Code (an AI assistant) and posted from my account after I reviewed it.

Thanks for splitting this out, and for spelling out the scope in the description.

How I checked: I ran TestNodeBehindOnIdleViewRejoins on this branch and on current main (038c351). I also ran a variant that skips the partial reconnect: it reconnects the whole cluster at once and doesn't check for the announcement-triggered sync.

Run Result
TestNodeBehindOnIdleViewRejoins on this branch passed 10/10
Same test on main failed 3/3 (node 2 never leaves view 0)
Full-reconnect variant on main passed 5/5 (14s)
Full-reconnect variant on this branch passed 5/5 (14s)
Both split tests together on this branch passed 6/6
internal/bft unit tests pass

The full-reconnect result matches what the description says. On main, node 1 gets f+1 heartbeat responses and syncs, and node 2 then sees the new view in a heartbeat. Both recover within about half a second. On this branch the announcement-triggered sync gets there about 0.2s earlier. So the new path only matters in a partial partition, where the follower still hears its stuck leader but reaches only part of the cluster. In production, as you described, all nodes could reach each other, and #700 covered that. Have you seen the partial-partition case in practice, or is this hardening for a case we can construct? Either is fine, but it changes how much risk we want to accept here.

A few points on the change itself:

  1. The view is recorded as synced before the sync actually happens. syncForAnnouncedView sets syncedForAnnouncedView = announcedView and then calls Synchronizer.Sync(). Controller.Sync() only calls grabSyncToken, which silently drops the request if a sync is already running. The sync can also fail, for example if fetchState times out. In either case every later announcement of that view is ignored. In the partial-partition case this PR is for, the node could then stay stuck. Could the view be recorded only after a sync has actually run, or could a later announcement retry?

  2. A sync during a view change can restart the old view. Suppose the node has moved to currView and aborted the old view, and the triggered sync finds nothing newer. The controller then calls changeView(getCurrentViewNumber(), ...), and because that view is stopped it starts it again. Meanwhile the view changer has already moved on. The existing view-change-timeout sync can reach the same path, so this isn't new, but the announcement path is another way in. A unit test where the announcement arrives while the node is in the middle of a view change would show whether this matters.

  3. Stale announcements count towards the quorum. nvs keeps the highest view each sender announced and is only cleared on a view change. So f faulty senders that once announced a very high view, plus one honest node that is one view ahead, reach f+1. A node that is merely slow then syncs. That sync costs a pull and a state-collector wait, plus Batcher.Close if the node is the leader. The once-per-view guard bounds it, so this is a cost to mention, not a correctness issue.

The earlier concern about processNewViewMsg clearing syncedForNewView no longer applies: after #704, acceptedNewView blocks a second NewView message for the same view.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants