fix: let a node that is one sequence behind rejoin the cluster

[pfi79]

A node that fell behind the cluster by exactly the proposal that was in flight could never
rejoin, and went silent forever.

The numbers that tell a node how far the cluster got were taken from two different frames of
reference. ViewSequences held the sequence that had just been decided, while the heartbeat a
leader broadcasts, the sequence a node reports when its state is collected, and the sequence a
follower compares its own against all refer to the sequence the view is about to propose. A node
that syncs in the middle of a proposal is told the cluster is at a sequence it has already
decided: it starts the new view at that sequence, waits for a proposal that was already sent
once and is never sent again, and never learns that it is behind.

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.

The scenario is covered end to end by TestNodeOneSequenceBehindRejoins in
test/view_split_test.go. It builds the shape that was seen in production, a cluster of seven
spread over three views, and it fails on main every time it is run: the two nodes that stayed
behind never deliver the next request.

The second half of the problem, a node that never starts a view change because it keeps hearing
its leader, is handled by the announcement-triggered sync and by
TestNodeBehindOnIdleViewRejoins, which follow in a separate pull request.

[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.

[HagarMeir]

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

Thanks, this is a big improvement. The shared setup, the delivery wait that checks for request 2 and stops its goroutines, assert.Eventually instead of fixed sleeps, and the separate counter for announcement-triggered syncs all help. Both tests now take about 14s together, down from about 48s.

How I checked: I ran the two tests on this branch, on main (without the fix), and on this branch with the announcement-triggered sync turned off. That last build keeps only the ViewSequences change, so it shows which part of the fix each test depends on.

Test	main (no fix)	This PR	ViewSequences change only
TestNodeOneSequenceBehindRejoins	failed 2/3	passed 5/5	passed 3/3
TestNodeBehindOnIdleViewRejoins	failed 3/3	passed 5/5	failed 3/3 (node 2 never delivers request 2)

So the first test covers the ViewSequences change and the second covers the announcement-triggered sync. A few comments:

TestNodeOneSequenceBehindRejoins

• It sometimes passes without the fix (1 of 3 runs on main).
  • The tracker states in that run were the same as in the failing runs. The difference was that node 2 happened to sync once more after request 2 had been decided, and that sync pulled request 2 in.
  • The old version waited 10–30s before healing. This one heals as soon as the five nodes reach a new view, so it races with node 2's own sync.
  • Waiting after heal until the nodes behind are in the cluster's view, and only then submitting request 2, may make it fail every time on main. Could you check that by running it on main several times?
• The comment doesn't match the setup:
  • Nodes 1 and 2 are both Disconnect()ed, so they can't hear each other or "their own leader".
  • Node 2's timers are accelerated, so it does start a view change (the tracker shows attempts=1).
  • Node 2 also leads view 1, so view 1 is never installed and the five connected nodes go to view 2.

TestNodeBehindOnIdleViewRejoins

• Frozen timers: the new path is only needed here because the timers of nodes 1 and 2 are frozen.
  • With the ViewSequences change alone, node 2 never recovers, so this test needs the announcement-triggered sync.
  • In the first test, node 2 is in a very similar position but its timers tick, and it recovers without the new path.
  • What real situation does a node whose timers never fire correspond to? If it's "two nodes split off together, and the old leader keeps sending heartbeats to the follower so it never times out", that can be set up directly: keep 1↔2 connected, cut them off from the rest, and accelerate everyone's timers. Then check whether that also fails with only the ViewSequences change.
• Comments:
  • Nodes 3 and 7 stay in view 2, the view the cluster is actually in. The three in the middle are the ones announcing the next view, view 3. So "the two in front ... alone in the view after" isn't what happens.
  • The log line "state with the nodes spread over three views" prints nodes in only two views, 0 and 2.

Both tests

• The shape you saw in production: neither test reproduces the shape you described (2 nodes in view 0, 4 in view 1, 1 in view 2).
  • Because node 2 leads view 1 and is one of the nodes left behind, the cluster always skips straight to view 2.
  • Choosing which nodes stay behind so that the leaders of views 1 and 2 are in the group that moves on would let a test build that shape. It would be good to confirm that test fails on main, since it's the case you actually hit.
• Setup checks: the results of viewsReached and attemptsReached are ignored. If the setup doesn't produce the intended split, the test carries on and fails later with a confusing message. require.Eventually, or if !viewsReached(...) { t.FailNow() }, would stop it at the step that went wrong. The same goes for the announced-sync check at L477: on main it currently fails after 90s, and then the test waits another 90s for delivery.

I'll comment on the fix itself once the tests are settled.

[HagarMeir]

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

One more suggestion: could we take this one change at a time? I'd start with just the ViewSequences change and TestNodeOneSequenceBehindRejoins.

• With the announcement-triggered sync turned off, TestNodeOneSequenceBehindRejoins passed 3/3, so the ViewSequences change alone fixes the case that test covers.
• It's a small change, and it's easier to review and discuss on its own.
• The announcement-triggered sync and TestNodeBehindOnIdleViewRejoins could then follow in a separate PR, once we've settled the frozen-timer question above.

For the first step, it would help to make TestNodeOneSequenceBehindRejoins fail on main every time (it passed there in 1 of 3 runs), so it reliably guards the fix.

Would you be open to splitting it this way?

[pfi79]

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

One more suggestion: could we take this one change at a time? I'd start with just the ViewSequences change and TestNodeOneSequenceBehindRejoins.

• With the announcement-triggered sync turned off, TestNodeOneSequenceBehindRejoins passed 3/3, so the ViewSequences change alone fixes the case that test covers.
• It's a small change, and it's easier to review and discuss on its own.
• The announcement-triggered sync and TestNodeBehindOnIdleViewRejoins could then follow in a separate PR, once we've settled the frozen-timer question above.

For the first step, it would help to make TestNodeOneSequenceBehindRejoins fail on main every time (it passed there in 1 of 3 runs), so it reliably guards the fix.

Would you be open to splitting it this way?

I'm willing to do anything to solve the main problem. I won't share the changes yet. I'm afraid that the main situation that I would like to overcome will go away. The nodes were stuck in views n, n + 1, n + 2. and the long waiting time did not lead to the fact that they restored their work and were able to continue calculating transactions.
I will first try to make sure that the two changes separately will lead to the solution of the main problem. So that I don't split them up, we'll fix them. The main problem will remain.

[pfi79]

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

Thanks, this is a big improvement. The shared setup, the delivery wait that checks for request 2 and stops its goroutines, assert.Eventually instead of fixed sleeps, and the separate counter for announcement-triggered syncs all help. Both tests now take about 14s together, down from about 48s.

How I checked: I ran the two tests on this branch, on main (without the fix), and on this branch with the announcement-triggered sync turned off. That last build keeps only the ViewSequences change, so it shows which part of the fix each test depends on.

Test main (no fix) This PR ViewSequences change only
TestNodeOneSequenceBehindRejoins failed 2/3 passed 5/5 passed 3/3
TestNodeBehindOnIdleViewRejoins failed 3/3 passed 5/5 failed 3/3 (node 2 never delivers request 2)
So the first test covers the ViewSequences change and the second covers the announcement-triggered sync. A few comments:

TestNodeOneSequenceBehindRejoins

• It sometimes passes without the fix (1 of 3 runs on main).

  • The tracker states in that run were the same as in the failing runs. The difference was that node 2 happened to sync once more after request 2 had been decided, and that sync pulled request 2 in.
  • The old version waited 10–30s before healing. This one heals as soon as the five nodes reach a new view, so it races with node 2's own sync.
  • Waiting after heal until the nodes behind are in the cluster's view, and only then submitting request 2, may make it fail every time on main. Could you check that by running it on main several times?

• The comment doesn't match the setup:

  • Nodes 1 and 2 are both Disconnect()ed, so they can't hear each other or "their own leader".
  • Node 2's timers are accelerated, so it does start a view change (the tracker shows attempts=1).
  • Node 2 also leads view 1, so view 1 is never installed and the five connected nodes go to view 2.

TestNodeBehindOnIdleViewRejoins

• Frozen timers: the new path is only needed here because the timers of nodes 1 and 2 are frozen.

  • With the ViewSequences change alone, node 2 never recovers, so this test needs the announcement-triggered sync.
  • In the first test, node 2 is in a very similar position but its timers tick, and it recovers without the new path.
  • What real situation does a node whose timers never fire correspond to? If it's "two nodes split off together, and the old leader keeps sending heartbeats to the follower so it never times out", that can be set up directly: keep 1↔2 connected, cut them off from the rest, and accelerate everyone's timers. Then check whether that also fails with only the ViewSequences change.

• Comments:

  • Nodes 3 and 7 stay in view 2, the view the cluster is actually in. The three in the middle are the ones announcing the next view, view 3. So "the two in front ... alone in the view after" isn't what happens.
  • The log line "state with the nodes spread over three views" prints nodes in only two views, 0 and 2.

Both tests

• The shape you saw in production: neither test reproduces the shape you described (2 nodes in view 0, 4 in view 1, 1 in view 2).

  • Because node 2 leads view 1 and is one of the nodes left behind, the cluster always skips straight to view 2.
  • Choosing which nodes stay behind so that the leaders of views 1 and 2 are in the group that moves on would let a test build that shape. It would be good to confirm that test fails on main, since it's the case you actually hit.

• Setup checks: the results of viewsReached and attemptsReached are ignored. If the setup doesn't produce the intended split, the test carries on and fails later with a confusing message. require.Eventually, or if !viewsReached(...) { t.FailNow() }, would stop it at the step that went wrong. The same goes for the announced-sync check at L477: on main it currently fails after 90s, and then the test waits another 90s for delivery.

I'll comment on the fix itself once the tests are settled.

I changed the test according to your comments.

[HagarMeir]

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

Thanks for the update. The tests look good now. The first one builds the views 0/1/2 shape you described, the second runs with all timers ticking, and the setup checks stop the test at the step that went wrong.

How I checked: you wanted to know whether each change on its own solves the main problem, so I ran both tests on four builds, 5 rounds each:

• this branch;
• main, with the new test file;
• this branch with only the ViewSequences change (announcement-triggered sync turned off);
• this branch with only the announcement-triggered sync (view.go restored to main).

Build	TestNodeOneSequenceBehindRejoins	TestNodeBehindOnIdleViewRejoins
This branch	passed 5/5	passed 5/5
main	failed 5/5	failed 5/5
ViewSequences change only	passed 5/5	failed 5/5
Announcement-triggered sync only	failed 5/5	passed 5/5

Both tests now fail without the fix, every time.

• TestNodeOneSequenceBehindRejoins used to pass on main 1 of 3 times. Now it fails there every time.
• For TestNodeBehindOnIdleViewRejoins, I also wanted to rule out that it only fails because the new log line is missing. So I ran it on main and on the ViewSequences-only build with the announcedSyncReached check removed. In all 5 rounds of each, node 2 stayed in view 0 and never synced at all (syncs=0). So the test does show the announcement path is needed in that scenario.

On your question whether each change alone solves the main problem:

• TestNodeOneSequenceBehindRejoins is the views 0/1/2 shape you saw in production. The ViewSequences change alone fixes it; the announcement-triggered sync alone doesn't.
• The announcement-triggered sync is needed only for the narrower case in TestNodeBehindOnIdleViewRejoins: the old leader (node 1) stays cut off, and its follower (node 2) still hears it but reaches only part of the cluster. As the test's comment says, if node 1 could reach the cluster, the existing heartbeat-response path would sync it and pull node 2 along.

So splitting shouldn't lose the main fix. The first PR, with the ViewSequences change and TestNodeOneSequenceBehindRejoins, covers the shape you hit. The announcement-triggered sync and TestNodeBehindOnIdleViewRejoins can follow separately.

Two small points on the tests:

• holdCommits freezes nodes 5 and 6. MutateSend hooks run synchronously inside SendConsensus, on the sender's own goroutine. So blocking on releaseCh there stops everything nodes 5 and 6 send on that path until release(), not just their commits. It works for this test, but the comment should say so.
• The second test relies on node 1 staying cut off. Is that partial partition something you've seen in production, or is it a constructed case? It would help us judge how much the announcement-triggered sync is needed in practice.

[pfi79]

• The second test relies on node 1 staying cut off. Is that partial partition something you've seen in production, or is it a constructed case? It would help us judge how much the announcement-triggered sync is needed in practice.

no: in the prod, all nodes remained mutually accessible, the network was not completely cut; they got stuck precisely because of the out-of-sync sequences

[pfi79]

@HagarMeir On your advice, I will divide the pull request into 2. Now the first part will go, after we work it out and accept it, I will send the second one.