Skip to content

Decide pre-vote on the candidate's log, not on liveness alone - #134

Draft
tiandiwonder wants to merge 2 commits into
masterfrom
fix-prevote-grants-on-liveness-alone
Draft

tiandiwonder wants to merge 2 commits into
masterfrom
fix-prevote-grants-on-liveness-alone

Conversation

@tiandiwonder

@tiandiwonder tiandiwonder commented Sep 20, 2026 •

Copy link
Copy Markdown

Problem

handle_prevote_req grants on liveness alone. It reads the candidate's last log index and term into its log line and then decides on hb_alive_ and nothing else. The real vote checks them:

// handle_vote_req
bool log_okay =
    req.get_last_log_term() > log_store_->last_entry()->get_term() ||
    ( req.get_last_log_term() == log_store_->last_entry()->get_term() &&
      log_store_->next_slot() - 1 <= req.get_last_log_idx() );

So pre-vote is weaker than the vote it stands in for, in the one dimension that decides the outcome. That inverts its purpose: pre-vote exists (Ongaro thesis §9.6) precisely so a node that would lose the real vote does not disturb the term first.

A member that cannot win still passes pre-vote during any leaderless window, bumps the term and deposes the sitting leader. Deposing the leader creates the next leaderless window, so the same hopeless candidate qualifies again — the disruption is self-sustaining.

Observed on a three-node ensemble with no network partition: 10 leadership changes in 32 minutes, terms 297 → 313, the decisive ones initiated by a member whose log index had not advanced by a single entry in 51 seconds. In the same window, 15 pre-vote requests from that member were correctly denied — with a leader alive hb_alive_ is true and the existing check is enough. Only the leaderless window is affected.

Fix

Compute log_okay the way handle_vote_req does and require it for the grant. The request already carries both fields, so this needs no protocol change.

  • The is_catching_up() exemption is unchanged. Its comment explains that a catching-up server does not receive normal append_entries, so its own hb_alive_ may be stale — that concerns the voter's state, not the candidate's fitness.
  • Liveness is preserved by the standard pre-vote argument: a candidate denied here would also have lost the real vote, since the voter applies the same predicate there. An election still succeeds as soon as a member whose log is fresh enough times out.
  • The deny path now reports which of the two reasons applied. They were previously indistinguishable in exactly the situation where the difference matters.

Test

prevote_denies_candidate_whose_log_is_behind_test in tests/unit/leader_election_test.cxx: one member is put far enough behind to lose, the leader is taken away, and a second member times out as well — hb_alive_ on a follower is cleared only by that follower starting a pre-vote of its own, so without that step the denial comes from the heartbeat check and the test passes with or without this change. It then asserts the term does not move and that a member whose log is current still wins.

Reverting the change turns it red: s2.raftServer->get_term() expected 1, actual 2.

One behaviour change beyond the defect

The safety argument above — a candidate denied here would also have lost the real vote — holds at the instant of the denial, not necessarily a moment later. leader_election_priority_test is a case where it does not.

There, a resigning leader hands over to a higher-priority successor that is one entry behind: the entry the resigning leader appended on becoming leader, still uncommitted. Before this change pre-vote was granted on liveness alone, the successor caught up while the vote it then initiated was in flight, and the takeover completed in one round. Now it is denied and the takeover costs an extra election timeout.

That is a real cost on the priority-takeover path, and a conscious one: pre-vote must not be more permissive than the vote it predicts, and one extra round there is smaller than a self-sustaining election storm. Liveness still holds — the resigning leader appends nothing further, so the successor does not keep falling behind and wins on its next timeout.

The test therefore hands the successor that entry first. It uses delieverReqTo rather than execReqResp because the response half re-arms the resigning leader's hb_alive_, and clearing that flag on resign (raft_server.cxx:1508, "Clear live flag to avoid pre-vote rejection") is exactly what lets a successor's pre-vote through.

Full suite: 10/10 with the change. Removing the fix leaves only the new regression test red, so the sync did not weaken what the priority test checks.

`handle_prevote_req` reads the candidate's last log index and term into its log
line and then decides on `hb_alive_` alone. The real vote checks them
(`handle_vote_req`), so pre-vote is weaker than the vote it stands in for, in
the dimension that decides the outcome. A member that cannot win still passes
pre-vote during any leaderless window, bumps the term and deposes the sitting
leader - and because that creates the next leaderless window, the same hopeless
candidate qualifies again.

Seen on a three-node ensemble with no partition: 10 leadership changes in 32
minutes, terms 297 to 313, the decisive ones from a member whose log index had
not advanced by a single entry in 51 seconds. In the same window 15 pre-votes
from that member were correctly denied - with a leader alive, `hb_alive_` is
true and the existing check is enough. Only the leaderless window is affected.

The request already carries both fields, so this needs no protocol change. The
`is_catching_up()` exemption stays as it is: it concerns the voter's own
`hb_alive_` being stale, not the candidate's fitness. Liveness is preserved by
the standard pre-vote argument - a candidate denied here would have lost the
real vote, and an election still succeeds as soon as a member whose log is
fresh enough times out.

The deny path now says which of the two reasons applied, because otherwise they
are indistinguishable in exactly the situation where the difference matters.

The test puts one member far enough behind to lose, takes the leader away, and
has a second member time out as well - `hb_alive_` on a follower is cleared
only by that follower starting a pre-vote of its own, so without this the
denial comes from the heartbeat check and the test passes either way. It then
asserts the term does not move, and that a member whose log is current still
wins. Reverting this change turns it red: term 1 becomes 2.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The scenario hands leadership from a resigning leader to a higher-priority
successor that is one entry behind - the entry the resigning leader appended on
becoming leader, uncommitted. It passed because pre-vote was granted on
liveness alone and the successor caught up during the vote that followed.

With the log checked, it is denied and has to wait for its next election
timeout, by which point it would have caught up anyway. So the test now hands
it that entry first. `delieverReqTo` rather than `execReqResp`, because the
response half re-arms the resigning leader's `hb_alive_`, and the whole point
of clearing that flag on resign is to let the successor's pre-vote through.

Found by running the full suite, which the first version of this branch did
not: 9 passed, 1 failed. With this, 10 of 10 - and removing the fix still turns
only the new test red, so the sync did not weaken what the priority test
checks.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tiandiwonder added a commit to ClickHouse/ClickHouse that referenced this pull request Sep 21, 2026
The branch gained a commit: running the full unit suite, which the first push
did not, showed the pre-vote change breaking `leader_election_priority_test`.
The successor there is one entry behind and used to be granted pre-vote on
liveness alone; it now catches up first.

**The pointer must be moved to the merge commit before this pull request is
merged.**

Related: ClickHouse/NuRaft#134

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.

1 participant