Repository navigation
Conversation
There was a problem hiding this comment.
Thanks for tackling this, the stuck-after-restart scenario is real. I think two parts of the fix need another look before merging:
- The resend change (L248) announces
currView+1even after the node has moved into the new view and is waiting for the NewView. Every resend then votes to leave that view, which can make the cluster skip views repeatedly. - The relaxed check in
controller.sync()(L667) adopts the peers' view without their sequence, so the node saves and returnsDecisionsInView: 0and a proposal sequence that can be wrong for that view.
There are also some smaller points about the per-view sync guard and shouldHelp. Details are inline.
🤖 This review was drafted with Claude Code and posted by @HagarMeir.
| // Announce the view after the one this node is in, since that is the view the other | ||
| // nodes count. Announcing the view this node is already in would be equal to their | ||
| // current view, so none of them could count it. | ||
| nextView := v.currView + 1 |
There was a problem hiding this comment.
This resend can make the cluster skip views over and over. Once a node has a quorum for view k+1, processViewChangeMsg already sets currView = nextView (k+1). checkTimeout stays true while the node waits for the NewView, so this resend now broadcasts NextView: k+2.
ResendTimeout is shorter than ViewChangeTimeout, so every node in k+1 sends a vote for k+2 before leader k+1 has its full timeout. Peers in k+1 count those votes (vc.NextView == v.currView+1), reach quorum and call AbortView. This can repeat every resend interval, and the backOffFactor growth no longer gives a slow leader more time.
It also breaks the old recovery path: a peer still in view k that lost the original vote for k+1 used to get it again from this resend. It now gets k+2, which it can't count (it syncs instead).
Suggestion: keep resending v.nextView while waiting for the NewView (nextView == currView), and only announce currView+1 when the node really is behind.
| // synchronizer catches up the decisions, and a node that refuses to move on because | ||
| // the cluster is more than one sequence ahead would stay in a view the cluster left | ||
| // and no quorum could be formed again. | ||
| if response.View > newViewNum && response.Seq > latestDecisionSeq { |
There was a problem hiding this comment.
This adopts the peers' view without their sequence. The old check, response.Seq == latestDecisionSeq+1, made sure response.View had made no decisions yet. That is what makes saving LatestSequence: latestDecisionSeq, DecisionsInView: 0 and returning newProposalSequence = latestDecisionSeq+1 correct.
With >, if the peers are in view V at sequence S+5 and the synchronizer returned S, the node enters V believing it has 0 decisions and its next sequence is S+1, while V has already decided S+1..S+4. With LeaderRotation/DecisionsPerLeader it then computes the wrong leader and rejects the real leader's proposals. It also persists this NewView, so the wrong metadata comes back after a restart.
About the comment ("the synchronizer catches up the decisions"): if the synchronizer had caught up, latestDecisionViewNum would already have advanced newViewNum above. This branch only matters when the synchronizer is behind the peers, which is exactly when the saved metadata is wrong. Could we instead sync again until the sequence matches, or only adopt the view when response.Seq == latestDecisionSeq+1?
The new test case with peerSeq: controllerSeq + 10 has the synchronizer return controllerSeq, which is this inconsistent state. The test asserts only the view, not the sequence or decision count.
| // and moves to it, and if the sender is lying nothing changes. | ||
| // It syncs at most once per announced view, so that a faulty node cannot make | ||
| // this node sync over and over again. | ||
| if !v.hasSyncedForAnnouncedView || v.syncedForAnnouncedView != vc.NextView { |
There was a problem hiding this comment.
This guard doesn't bound syncs against a faulty node. It remembers only the last announced view, so a node that alternates NextView (currView+2, currView+3, currView+2, ...) triggers Synchronizer.Sync() on every message. That is a full sync plus a state-transfer broadcast each time, and on the leader Controller.Sync also closes the Batcher.
The reverse problem exists too: the slot is shared by all senders and is used even when the sync finds nothing, or when the sync was dropped because grabSyncToken found one already running. If a faulty node announces currView+2 early, later honest announcements of the same view never trigger a sync.
Maybe track it per sender, or only sync once f+1 distinct senders announce a view above currView+1 (at least one of them must be honest).
| // shouldHelp reports whether this node should rebroadcast the view change a lagging node | ||
| // announced for the given view. Repeating the same announcement back only adds noise that the | ||
| // lagging node cannot act on anyway, so each view is helped at most once per node. | ||
| func (nv *nextViews) shouldHelp(next uint64, sender uint64) bool { |
There was a problem hiding this comment.
shouldHelp lets a helper rebroadcast for a given (sender, view) only once until the helper's own view changes and nvs is cleared. If that one broadcast is lost, or the lagging node restarts and announces the same view again (the restart case this PR targets), it is never helped again and has to rely on timeouts and sync.
If the goal is less log noise, a time-based rate limit per sender (e.g. at most once per ResendTimeout) would keep the retry behaviour.
| v.processViewChangeMsg(false) | ||
| return | ||
| } | ||
| if vc.NextView > v.currView+1 { |
There was a problem hiding this comment.
Together with the resend change at L248, this makes honest nodes sync unnecessarily: a node already in k+1 resends k+2, and a peer still at k sees k+2 > k+1 and calls Sync(). No NewView has been decided yet, so the sync changes nothing, and the peer still doesn't get that node's vote for k+1.
| backOffFactor uint64 | ||
| syncedForNewView bool | ||
| syncedForAnnouncedView uint64 | ||
| hasSyncedForAnnouncedView bool |
There was a problem hiding this comment.
Nit: hasSyncedForAnnouncedView isn't needed. The guarded branch only runs when vc.NextView > currView+1 >= 1, so syncedForAnnouncedView == 0 already means "not synced". One field means one less thing to keep in step.
| v.syncedForNewView = false // clear because currView changed | ||
| v.viewDataMsgs.clear(v.N) // clear because currView changed | ||
| v.syncedForNewView = false // clear because currView changed | ||
| v.hasSyncedForAnnouncedView = false // clear because currView changed |
There was a problem hiding this comment.
Nit: this reset is now copied into both informNewView and here, next to the existing syncedForNewView/viewDataMsgs resets, and it's missing from the restore path (~L1195). A single "view changed" reset helper would keep these in step as more per-view state is added.
A node that lost a view change, for example by restarting, stays in that view while the rest of the cluster moved on. It can never rejoin: the view change only accepts messages for the view right after the one the node is in, and no node announces that view anymore. It is told to sync, but sync only accepted the state the peers reported when they were exactly one sequence ahead, so as soon as the cluster decided more than one transaction the node discarded the view it was given and kept where it was, forever. Accept any sequence the cluster is ahead by, the synchronizer catches up the decisions anyway. A node that hears about a view beyond the one it is in now syncs to find out where the cluster is, at most once per announced view so a faulty node cannot make it sync indefinitely. Its view change resend announces the view after the one it is in, since announcing the view it is already in cannot be counted by any node. Lagging nodes are helped once per view instead of on every message, which is what flooded the logs. Signed-off-by: Fedor Partanskiy <fredprtnsk@gmail.com>
31e0e91 to
c690e22
Compare
|
@HagarMeir Three days ago, we met with a situation where the connection was abruptly disrupted, and they began to restart constantly. After the restoration, some nodes ended up in view 67, some nodes in view 69. they could not change their view and choose a leader to start processing transactions. I don't have any more information. With what I know, I've tried to figure out how this could happen. Perhaps all the correction that is here is incorrect. |
2 participants
A node that lost a view change, for example by restarting, stays in that view while the rest of the cluster moved on. It can never rejoin: the view change only accepts messages for the view right after the one the node is in, and no node announces that view anymore. It is told to sync, but sync only accepted the state the peers reported when they were exactly one sequence ahead, so as soon as the cluster decided more than one transaction the node discarded the view it was given and kept where it was, forever.
Accept any sequence the cluster is ahead by, the synchronizer catches up the decisions anyway.
A node that hears about a view beyond the one it is in now syncs to find out where the cluster is, at most once per announced view so a faulty node cannot make it sync indefinitely. Its view change resend announces the view after the one it is in, since announcing the view it is already in cannot be counted by any node. Lagging nodes are helped once per view instead of on every message, which is what flooded the logs.