fix: let a node that lost a view change rejoin the cluster

[pfi79]

A node that fell behind a view change - because it restarted, lost messages or was cut off while the rest of the cluster moved on - could never rejoin. It stayed in its old view, and a view change only accepts messages for the view immediately after it. The cluster had already moved further and no longer announced that view, so nothing could tell the node where it went. Sync was supposed to, but it compared numbers from different frames of reference: ViewSequences held the sequence that had just been decided, not the one the view was about to propose, and the state transfer response carried the same decided sequence. A node behind the cluster by exactly the proposal in flight got the new view at a sequence that had already been decided, and went silent forever.

Publish the next proposal sequence in ViewSequences as soon as the view moves on to it, so that the heartbeat a leader broadcasts, the sequence a node reports when its state is collected, and the sequence a follower compares its own against are all the same number.

A node that hears a quorum of distinct senders (f+1) announce 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 sender cannot make it sync indefinitely. The announcement only triggers the sync; the view itself is 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 restore from the log.

The split and restart scenarios are covered end to end by the tests in test/view_split_test.go.

[pfi79]

At our stand, the situation turned out to be that the nodes spread to different views and could not restore their functionality.
I tried it and it turned out to be a view_split_test.go test. As a result, out of 7 nodes, 2 nodes ended up in view 0, 4 nodes in view 1, and 1 node in view 2. And they could not continue to restore their working capacity on their own.
I think the most valuable thing about this pr is view_split_test.go.
The rest is a fix option, probably not the best. Maybe suggest a better option.

[HagarMeir]

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

Thanks for this. Before we look at the fix itself, here are some notes on test/view_split_test.go.

How I checked: I copied view_split_test.go onto main, without the fix, and ran it there and on this branch.

• This branch: 10 rounds on a 10-core machine, plus more rounds with GOMAXPROCS=2 for timing.
• main: 5 rounds of the two tests below that fail there.

Only two of the six tests fail on main. The other four pass with or without the fix, so they don't test it.

Test	main (no fix)	This PR	What it covers
TestClusterSplitTwoThreeTwo	fails 5/5	passes	The new "f+1 announced a later view" sync path; it is the only test where that path runs
TestClusterSplitAcrossViews	fails 5/5	passes	The ViewSequences change (a node stays one sequence behind). The announcement path never runs here
TestClusterSplitIntoTwoGroupsWithoutQuorum	passes	passes	Nothing in this PR
TestClusterSplitIntoThreeAndFour	passes	passes	Nothing in this PR
TestNodeRestartedWithoutSyncJoinsOrStalls	passes	passes	Nothing in this PR
TestPeersReportTheViewTheyAreIn	passes	passes	Nothing in this PR

Keep, with fixes

TestClusterSplitTwoThreeTwo

• Node 2 leads view 1, and node 2 is one of the disconnected nodes. View 1 is never installed and the five connected nodes go to view 2. The comments assume view 1.
• The "two in front move on" check (around L474, viewOf(ahead) >= 2) is already true before the cut. Two nodes can't change view on their own either: a view change needs quorum-1 = 4 votes from other nodes.
• The final assertion (L565) counts every sync ("get msg from syncChan"). The existing view-change timeout already calls Sync() (checkIfTimeout), so the assertion passes without the new code. It should check for the new log line ("is calling sync because ... announced view") instead.

TestClusterSplitAcrossViews

• This test catches the ViewSequences off-by-one, not the scenario its comments describe.
  • On main, node 2 gets to view 2 at sequence 2. After request 2 is decided, it never catches up.
  • The new announcement-triggered sync never runs in this test (0 occurrences in the logs).
  • Please change the test name and comments to say what it checks.
• The split described in the comments (around L254–L300) doesn't happen:
  • Node 2 leads view 1 and is disconnected, so the five connected nodes go to view 2, not view 1.
  • After node 7 is cut off, the remaining four see only 3 other nodes. They can't change view (that needs 4 votes from others).
  • Node 7 alone never gets ahead; it only keeps announcing view 3.
  • The tracker shows {1,2} in view 0 and {3..7} in view 2, not three views.
• The cut(moving, behind) loop is redundant, since behind is already Disconnect()ed.

For both: assert each group's view with assert.Eventually on viewOf(...) instead of only logging it. That would catch the setup not producing the intended split.

Tests that don't exercise the fix

TestClusterSplitIntoTwoGroupsWithoutQuorum / TestClusterSplitIntoThreeAndFour

Why are these two needed in this PR? They pass both on main and on this branch. What they show:

• Neither gets past the first step of the view change: every node ends at view=0, attempts=1, view data sent=0. So assert.Zero(quorumsOf(...)) can't fail, and the comment about the leader not gathering view data describes a stage these tests never reach.
• The recovery check (around L676) accepts any record on the buffered Delivered channel, not specifically request 2.

If they're meant to document current behaviour, please fix the comments and the recovery check. Otherwise they may not belong in this PR.

TestNodeRestartedWithoutSyncJoinsOrStalls

• accelerateTime(nodes[1:], ...) leaves node 1 out. Its heartbeat and view-change channels get one value at start and never tick again, so after the restart node 1 has no timers to find out it's behind. The tracker shows node 1 still at view=0, syncs=0 30s after it reconnects.
• Nothing checks that node 1 rejoined; the name says either outcome is fine. Like the split tests, the recovery check (around L779) accepts any delivery, not specifically request 2.
• What is this test meant to check? This is the restart scenario from the PR description, so if the aim is to show that the node rejoins, it could:
  • include node 1 in accelerateTime;
  • assert that viewOf(1) reaches the cluster's view;
  • then check that request 2 is delivered on all nodes;
  • confirm it fails on main.

TestPeersReportTheViewTheyAreIn

• It passes on main. It only waits for a "collected a valid state" log line and never checks the view or sequence in it.
• Both branches logged the same view - 1 and seq - 2, so the test can't tell the fixed code from the old code.
• What is it meant to show? If it should check what peers report, it could assert the collected view and sequence, and then confirm it fails on main.

General

• Fails without the fix: before adding a test, check that it fails on main without the fix. Only two of these six do.
• Check for the specific request: wherever a test waits for recovery on Delivered, check requestIDFromBatch(record) == 2, as AcrossViews and TwoThreeTwo already do.
• Replace fixed sleeps: the time.Sleep(10–30s) calls set how long each test takes. Durations varied by less than 0.2s between rounds. All six take about 48s together with full CPU, and about 85–100s with GOMAXPROCS=2. Without the fix, the two tests that fail take about 2 minutes, because they wait out the 90s timeouts. assert.Eventually on the tracker would be faster and would assert the intermediate states.
• Stop the receiver goroutines: the per-node goroutines that wait for delivery keep running after a t.Fatal. Stop them with a done channel or t.Cleanup.
• Shared setup: the node setup block is copied into every test; a helper would shrink the file a lot.

Flakiness:

Run	Result
This branch, full CPU	10/10 passed
main, TwoThreeTwo and AcrossViews	failed 5/5

I'll comment on the fix itself separately.

[pfi79]

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

Thanks for this. Before we look at the fix itself, here are some notes on test/view_split_test.go.

How I checked: I copied view_split_test.go onto main, without the fix, and ran it there and on this branch.

• This branch: 10 rounds on a 10-core machine, plus more rounds with GOMAXPROCS=2 for timing.
• main: 5 rounds of the two tests below that fail there.

Only two of the six tests fail on main. The other four pass with or without the fix, so they don't test it.

Test main (no fix) This PR What it covers
TestClusterSplitTwoThreeTwo fails 5/5 passes The new "f+1 announced a later view" sync path; it is the only test where that path runs
TestClusterSplitAcrossViews fails 5/5 passes The ViewSequences change (a node stays one sequence behind). The announcement path never runs here
TestClusterSplitIntoTwoGroupsWithoutQuorum passes passes Nothing in this PR
TestClusterSplitIntoThreeAndFour passes passes Nothing in this PR
TestNodeRestartedWithoutSyncJoinsOrStalls passes passes Nothing in this PR
TestPeersReportTheViewTheyAreIn passes passes Nothing in this PR

Keep, with fixes

TestClusterSplitTwoThreeTwo

• Node 2 leads view 1, and node 2 is one of the disconnected nodes. View 1 is never installed and the five connected nodes go to view 2. The comments assume view 1.
• The "two in front move on" check (around L474, viewOf(ahead) >= 2) is already true before the cut. Two nodes can't change view on their own either: a view change needs quorum-1 = 4 votes from other nodes.
• The final assertion (L565) counts every sync ("get msg from syncChan"). The existing view-change timeout already calls Sync() (checkIfTimeout), so the assertion passes without the new code. It should check for the new log line ("is calling sync because ... announced view") instead.

TestClusterSplitAcrossViews

• This test catches the ViewSequences off-by-one, not the scenario its comments describe.

  • On main, node 2 gets to view 2 at sequence 2. After request 2 is decided, it never catches up.
  • The new announcement-triggered sync never runs in this test (0 occurrences in the logs).
  • Please change the test name and comments to say what it checks.

• The split described in the comments (around L254–L300) doesn't happen:

  • Node 2 leads view 1 and is disconnected, so the five connected nodes go to view 2, not view 1.
  • After node 7 is cut off, the remaining four see only 3 other nodes. They can't change view (that needs 4 votes from others).
  • Node 7 alone never gets ahead; it only keeps announcing view 3.
  • The tracker shows {1,2} in view 0 and {3..7} in view 2, not three views.

• The cut(moving, behind) loop is redundant, since behind is already Disconnect()ed.

For both: assert each group's view with assert.Eventually on viewOf(...) instead of only logging it. That would catch the setup not producing the intended split.

Tests that don't exercise the fix

TestClusterSplitIntoTwoGroupsWithoutQuorum / TestClusterSplitIntoThreeAndFour

Why are these two needed in this PR? They pass both on main and on this branch. What they show:

• Neither gets past the first step of the view change: every node ends at view=0, attempts=1, view data sent=0. So assert.Zero(quorumsOf(...)) can't fail, and the comment about the leader not gathering view data describes a stage these tests never reach.
• The recovery check (around L676) accepts any record on the buffered Delivered channel, not specifically request 2.

If they're meant to document current behaviour, please fix the comments and the recovery check. Otherwise they may not belong in this PR.

TestNodeRestartedWithoutSyncJoinsOrStalls

• accelerateTime(nodes[1:], ...) leaves node 1 out. Its heartbeat and view-change channels get one value at start and never tick again, so after the restart node 1 has no timers to find out it's behind. The tracker shows node 1 still at view=0, syncs=0 30s after it reconnects.

• Nothing checks that node 1 rejoined; the name says either outcome is fine. Like the split tests, the recovery check (around L779) accepts any delivery, not specifically request 2.

• What is this test meant to check? This is the restart scenario from the PR description, so if the aim is to show that the node rejoins, it could:

  • include node 1 in accelerateTime;
  • assert that viewOf(1) reaches the cluster's view;
  • then check that request 2 is delivered on all nodes;
  • confirm it fails on main.

TestPeersReportTheViewTheyAreIn

• It passes on main. It only waits for a "collected a valid state" log line and never checks the view or sequence in it.
• Both branches logged the same view - 1 and seq - 2, so the test can't tell the fixed code from the old code.
• What is it meant to show? If it should check what peers report, it could assert the collected view and sequence, and then confirm it fails on main.

General

• Fails without the fix: before adding a test, check that it fails on main without the fix. Only two of these six do.
• Check for the specific request: wherever a test waits for recovery on Delivered, check requestIDFromBatch(record) == 2, as AcrossViews and TwoThreeTwo already do.
• Replace fixed sleeps: the time.Sleep(10–30s) calls set how long each test takes. Durations varied by less than 0.2s between rounds. All six take about 48s together with full CPU, and about 85–100s with GOMAXPROCS=2. Without the fix, the two tests that fail take about 2 minutes, because they wait out the 90s timeouts. assert.Eventually on the tracker would be faster and would assert the intermediate states.
• Stop the receiver goroutines: the per-node goroutines that wait for delivery keep running after a t.Fatal. Stop them with a done channel or t.Cleanup.
• Shared setup: the node setup block is copied into every test; a helper would shrink the file a lot.

Flakiness:

Run Result
This branch, full CPU 10/10 passed
main, TwoThreeTwo and AcrossViews failed 5/5
I'll comment on the fix itself separately.

Thank you very much. Don't comment on the correction yet. I need to finalize the split test first.

[pfi79]

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

Thanks for this. Before we look at the fix itself, here are some notes on test/view_split_test.go.

How I checked: I copied view_split_test.go onto main, without the fix, and ran it there and on this branch.

• This branch: 10 rounds on a 10-core machine, plus more rounds with GOMAXPROCS=2 for timing.
• main: 5 rounds of the two tests below that fail there.

Only two of the six tests fail on main. The other four pass with or without the fix, so they don't test it.

Test main (no fix) This PR What it covers
TestClusterSplitTwoThreeTwo fails 5/5 passes The new "f+1 announced a later view" sync path; it is the only test where that path runs
TestClusterSplitAcrossViews fails 5/5 passes The ViewSequences change (a node stays one sequence behind). The announcement path never runs here
TestClusterSplitIntoTwoGroupsWithoutQuorum passes passes Nothing in this PR
TestClusterSplitIntoThreeAndFour passes passes Nothing in this PR
TestNodeRestartedWithoutSyncJoinsOrStalls passes passes Nothing in this PR
TestPeersReportTheViewTheyAreIn passes passes Nothing in this PR

Keep, with fixes

TestClusterSplitTwoThreeTwo

• Node 2 leads view 1, and node 2 is one of the disconnected nodes. View 1 is never installed and the five connected nodes go to view 2. The comments assume view 1.
• The "two in front move on" check (around L474, viewOf(ahead) >= 2) is already true before the cut. Two nodes can't change view on their own either: a view change needs quorum-1 = 4 votes from other nodes.
• The final assertion (L565) counts every sync ("get msg from syncChan"). The existing view-change timeout already calls Sync() (checkIfTimeout), so the assertion passes without the new code. It should check for the new log line ("is calling sync because ... announced view") instead.

TestClusterSplitAcrossViews

• This test catches the ViewSequences off-by-one, not the scenario its comments describe.

  • On main, node 2 gets to view 2 at sequence 2. After request 2 is decided, it never catches up.
  • The new announcement-triggered sync never runs in this test (0 occurrences in the logs).
  • Please change the test name and comments to say what it checks.

• The split described in the comments (around L254–L300) doesn't happen:

  • Node 2 leads view 1 and is disconnected, so the five connected nodes go to view 2, not view 1.
  • After node 7 is cut off, the remaining four see only 3 other nodes. They can't change view (that needs 4 votes from others).
  • Node 7 alone never gets ahead; it only keeps announcing view 3.
  • The tracker shows {1,2} in view 0 and {3..7} in view 2, not three views.

• The cut(moving, behind) loop is redundant, since behind is already Disconnect()ed.

For both: assert each group's view with assert.Eventually on viewOf(...) instead of only logging it. That would catch the setup not producing the intended split.

Tests that don't exercise the fix

TestClusterSplitIntoTwoGroupsWithoutQuorum / TestClusterSplitIntoThreeAndFour

Why are these two needed in this PR? They pass both on main and on this branch. What they show:

• Neither gets past the first step of the view change: every node ends at view=0, attempts=1, view data sent=0. So assert.Zero(quorumsOf(...)) can't fail, and the comment about the leader not gathering view data describes a stage these tests never reach.
• The recovery check (around L676) accepts any record on the buffered Delivered channel, not specifically request 2.

If they're meant to document current behaviour, please fix the comments and the recovery check. Otherwise they may not belong in this PR.

TestNodeRestartedWithoutSyncJoinsOrStalls

• accelerateTime(nodes[1:], ...) leaves node 1 out. Its heartbeat and view-change channels get one value at start and never tick again, so after the restart node 1 has no timers to find out it's behind. The tracker shows node 1 still at view=0, syncs=0 30s after it reconnects.

• Nothing checks that node 1 rejoined; the name says either outcome is fine. Like the split tests, the recovery check (around L779) accepts any delivery, not specifically request 2.

• What is this test meant to check? This is the restart scenario from the PR description, so if the aim is to show that the node rejoins, it could:

  • include node 1 in accelerateTime;
  • assert that viewOf(1) reaches the cluster's view;
  • then check that request 2 is delivered on all nodes;
  • confirm it fails on main.

TestPeersReportTheViewTheyAreIn

• It passes on main. It only waits for a "collected a valid state" log line and never checks the view or sequence in it.
• Both branches logged the same view - 1 and seq - 2, so the test can't tell the fixed code from the old code.
• What is it meant to show? If it should check what peers report, it could assert the collected view and sequence, and then confirm it fails on main.

General

• Fails without the fix: before adding a test, check that it fails on main without the fix. Only two of these six do.
• Check for the specific request: wherever a test waits for recovery on Delivered, check requestIDFromBatch(record) == 2, as AcrossViews and TwoThreeTwo already do.
• Replace fixed sleeps: the time.Sleep(10–30s) calls set how long each test takes. Durations varied by less than 0.2s between rounds. All six take about 48s together with full CPU, and about 85–100s with GOMAXPROCS=2. Without the fix, the two tests that fail take about 2 minutes, because they wait out the 90s timeouts. assert.Eventually on the tracker would be faster and would assert the intermediate states.
• Stop the receiver goroutines: the per-node goroutines that wait for delivery keep running after a t.Fatal. Stop them with a done channel or t.Cleanup.
• Shared setup: the node setup block is copied into every test; a helper would shrink the file a lot.

Flakiness:

Run Result
This branch, full CPU 10/10 passed
main, TwoThreeTwo and AcrossViews failed 5/5
I'll comment on the fix itself separately.

I've removed the extra tests. I have corrected the remaining ones, please take a look at them or there are more comments on them.