From b5b7e01b9213a2491b68205f33d2fe236dcb96dc Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Wed, 24 Jun 2026 13:56:24 +0000 Subject: [PATCH] fix(raft): vote-reply per-peer dedup + stale-term drop (bug-1095/1096) raft_on_vote_reply double-counted duplicated vote-responses (counter ignored from_idx) and failed to drop stale-term replies, allowing two leaders at the same term (bug-851 split-brain family). Mirror the existing pre-vote per-peer dedup pattern (new vote_replies[] set) and add DropStaleResponse (term < currentTerm). Surfaced by the raft-sim-conformance oracle (task 4.2). 47 raft / 264 overall lib tests pass. --- src/lib/raft/raft.c | 40 ++++++++++++++- src/lib/raft/raft.h | 1 + src/lib/testsuite/test_raft.c | 96 +++++++++++++++++++++++++++++++++++ 3 files changed, 135 insertions(+), 2 deletions(-) diff --git a/src/lib/raft/raft.c b/src/lib/raft/raft.c index 2e1f2de5..b07882f7 100644 --- a/src/lib/raft/raft.c +++ b/src/lib/raft/raft.c @@ -70,6 +70,10 @@ int raft_init(raft_state_t *s, const raft_config_t *cfg) s->self_idx = cfg->self_idx; s->votes_granted = 0; s->replies_received = 0; + /* Defensive: a node must never carry stale per-peer vote flags into its + * first election (mirrors pre_vote_replies). raft_start_election re-clears + * them, but zero here so a never-elected node has a clean array. */ + memset(s->vote_replies, 0, sizeof(s->vote_replies)); s->election_decided = 0; s->election_timeout_ms = cfg->election_timeout_ms ? cfg->election_timeout_ms : 2000; s->my_priority = cfg->my_priority; @@ -110,6 +114,13 @@ int raft_start_election(raft_state_t *s) s->voted_for_term = s->term; s->votes_granted = 1; /* self-vote */ s->replies_received = 0; + /* Fresh per-peer vote-reply tally for this election (mirrors + * pre_vote_replies reset). Mark self as already-counted so a stray + * self-addressed reply cannot double-count the self-vote that + * votes_granted already includes. */ + memset(s->vote_replies, 0, sizeof(s->vote_replies)); + if (s->self_idx >= 0 && s->self_idx < RAFT_MAX_PEERS) + s->vote_replies[s->self_idx] = 1; s->election_decided = 0; if (s->cbs->on_trace) @@ -421,8 +432,6 @@ int raft_on_pre_vote_reply(raft_state_t *s, int peer_idx, int granted) int raft_on_vote_reply(raft_state_t *s, int from_idx, int granted, uint64_t term) { - (void)from_idx; - if (s->cbs->on_trace) s->cbs->on_trace(s->ctx, "vote_reply_in", NULL); @@ -438,6 +447,15 @@ int raft_on_vote_reply(raft_state_t *s, int from_idx, int granted, uint64_t term return RAFT_BECAME_FOLLOWER; } + /* DropStaleResponse (raft.tla DropStaleResponse, mterm < currentTerm): + * a reply from an older term is discarded, NOT counted toward this + * election — and it is NOT a higher term, so it must not step us down. */ + if (term < s->term) { + if (s->cbs->on_trace) + s->cbs->on_trace(s->ctx, "vote_reply_stale_drop", NULL); + return RAFT_PENDING; + } + /* Step 2: no-op guard — election already decided */ if (s->election_decided) { if (s->cbs->on_trace) @@ -445,6 +463,24 @@ int raft_on_vote_reply(raft_state_t *s, int from_idx, int granted, uint64_t term return RAFT_PENDING; } + /* Bounds guard: ignore a reply from an out-of-range peer (mirrors the + * pre-vote per-peer dedup bound). */ + if (from_idx < 0 || from_idx >= s->n_peers || from_idx >= RAFT_MAX_PEERS) { + if (s->cbs->on_trace) + s->cbs->on_trace(s->ctx, "vote_reply_out_of_range", NULL); + return RAFT_PENDING; + } + + /* Per-peer idempotency (raft.tla votesGranted is a SET): a duplicate reply + * from a peer we already counted is ignored — a legal DuplicateMessage must + * not double-count toward quorum (Election Safety). */ + if (s->vote_replies[from_idx]) { + if (s->cbs->on_trace) + s->cbs->on_trace(s->ctx, "vote_reply_dup", NULL); + return RAFT_PENDING; + } + s->vote_replies[from_idx] = 1; + /* Step 3: count this reply */ s->replies_received++; diff --git a/src/lib/raft/raft.h b/src/lib/raft/raft.h index 83040fd0..b0e09788 100644 --- a/src/lib/raft/raft.h +++ b/src/lib/raft/raft.h @@ -34,6 +34,7 @@ typedef struct { int self_idx; /* this node's index in [0, n_peers) */ int votes_granted; /* votes collected in current election */ int replies_received; /* total peer replies received (for all-replied-no-quorum, Req 4.4) */ + int vote_replies[RAFT_MAX_PEERS]; /* per-peer replied flag; avoids double-count (mirrors pre_vote_replies) */ int election_decided; /* 1 once on_become_leader/follower fired */ uint32_t election_timeout_ms; int my_priority; /* priority index for jitter (0 = highest) */ diff --git a/src/lib/testsuite/test_raft.c b/src/lib/testsuite/test_raft.c index a6be9188..8663e0d6 100644 --- a/src/lib/testsuite/test_raft.c +++ b/src/lib/testsuite/test_raft.c @@ -1545,6 +1545,100 @@ static void test_vote_reply_pending_waiting(void) TEST_ASSERT_EQUAL_INT(0, g_mock.cancel_timeout_count); } +/* + * test_vote_reply_duplicate_grant_not_double_counted (bug-1095, raft.tla + * votesGranted-as-SET, idempotent under DuplicateMessage) + * + * 5-node cluster (quorum=3). Node starts a real election via raft_start_election + * (self-vote=1, vote_replies[self]=1). A GRANTED reply from peer 1 → votes=2, + * PENDING. The SAME peer-1 grant delivered AGAIN (a legal DuplicateMessage) must + * be IGNORED: RAFT_PENDING, votes still 2, NOT leader. Only a grant from a + * DISTINCT peer 2 reaches quorum (votes=3 → RAFT_BECAME_LEADER). + * + * Before the fix the duplicate would push votes to 3 and elect prematurely + * (Election Safety violation — a node could win on a single peer's vote). + */ +static void test_vote_reply_duplicate_grant_not_double_counted(void) +{ + raft_state_t s; + raft_callbacks_t cbs = make_mock_cbs(); + + cbs.on_trace = mock_trace; + reset_mock(); + + TEST_ASSERT_EQUAL_INT(0, init_node(&s, &cbs, 5, 0)); + + /* Real election start: CANDIDATE, self-vote=1, vote_replies[self]=1. */ + TEST_ASSERT_EQUAL_INT(RAFT_OK, raft_start_election(&s)); + TEST_ASSERT_EQUAL_INT(RAFT_CANDIDATE, s.role); + TEST_ASSERT_EQUAL_INT(1, s.votes_granted); + + /* First grant from peer 1 → votes=2, still below quorum=3. */ + int rc1 = raft_on_vote_reply(&s, 1, 1, s.term); + TEST_ASSERT_EQUAL_INT(RAFT_PENDING, rc1); + TEST_ASSERT_EQUAL_INT(2, s.votes_granted); + TEST_ASSERT_EQUAL_INT(RAFT_CANDIDATE, s.role); + + /* DUPLICATE grant from peer 1 → must be ignored (no double-count). */ + int rcdup = raft_on_vote_reply(&s, 1, 1, s.term); + TEST_ASSERT_EQUAL_INT(RAFT_PENDING, rcdup); + TEST_ASSERT_EQUAL_INT(2, s.votes_granted); /* still 2, NOT 3 */ + TEST_ASSERT_EQUAL_INT(RAFT_CANDIDATE, s.role); + TEST_ASSERT_EQUAL_INT(0, g_mock.become_leader_count); + + /* DISTINCT peer 2 grant → votes=3 = quorum → leader. */ + int rc2 = raft_on_vote_reply(&s, 2, 1, s.term); + TEST_ASSERT_EQUAL_INT(RAFT_BECAME_LEADER, rc2); + TEST_ASSERT_EQUAL_INT(RAFT_LEADER, s.role); + TEST_ASSERT_EQUAL_INT(3, s.votes_granted); + TEST_ASSERT_EQUAL_INT(1, g_mock.become_leader_count); +} + +/* + * test_vote_reply_stale_term_dropped (bug-1096, raft.tla DropStaleResponse — + * discard a response whose mterm < currentTerm) + * + * Candidate at term=2. A vote reply carrying term=1 (< current) must be DROPPED: + * RAFT_PENDING, NOT counted toward votes, and NO step-down (the stale reply is + * not a higher term, so it must not flip the node to FOLLOWER). The node stays a + * CANDIDATE with its vote tally unchanged. + * + * Before the fix the stale reply was counted (replies_received++ and, if granted, + * votes_granted++) — corrupting the current election's quorum accounting. + */ +static void test_vote_reply_stale_term_dropped(void) +{ + raft_state_t s; + raft_callbacks_t cbs = make_mock_cbs(); + + cbs.on_trace = mock_trace; + reset_mock(); + + TEST_ASSERT_EQUAL_INT(0, init_node(&s, &cbs, 5, 0)); + + /* Drive to a real election, then bump to term 2 (second election) so a + * reply at term 1 is strictly stale. */ + TEST_ASSERT_EQUAL_INT(RAFT_OK, raft_start_election(&s)); /* term 1 */ + s.election_decided = 0; + s.role = RAFT_FOLLOWER; /* allow a second start_election */ + TEST_ASSERT_EQUAL_INT(RAFT_OK, raft_start_election(&s)); /* term 2 */ + TEST_ASSERT_EQUAL_UINT64(2, s.term); + TEST_ASSERT_EQUAL_INT(1, s.votes_granted); + TEST_ASSERT_EQUAL_INT(0, s.replies_received); + + /* Stale GRANTED reply at term 1 (< current term 2) → dropped. */ + int rc = raft_on_vote_reply(&s, 1, 1, 1); + TEST_ASSERT_EQUAL_INT(RAFT_PENDING, rc); + + /* Not counted, no step-down, still CANDIDATE at term 2. */ + TEST_ASSERT_EQUAL_INT(1, s.votes_granted); /* unchanged */ + TEST_ASSERT_EQUAL_INT(0, s.replies_received); /* unchanged */ + TEST_ASSERT_EQUAL_INT(RAFT_CANDIDATE, s.role); + TEST_ASSERT_EQUAL_UINT64(2, s.term); + TEST_ASSERT_EQUAL_INT(0, g_mock.become_follower_count); + TEST_ASSERT_EQUAL_INT(0, g_mock.become_leader_count); +} + /* ---------- leader progress init on becoming leader (task 2.2) ---------- */ /* Init a node with a non-zero restored log length (initial_log_len). */ @@ -7805,6 +7899,8 @@ void test_raft(void) RUN_TEST(test_vote_reply_no_quorum_all_replied_3node); RUN_TEST(test_vote_reply_quorum_5node); RUN_TEST(test_vote_reply_pending_waiting); + RUN_TEST(test_vote_reply_duplicate_grant_not_double_counted); + RUN_TEST(test_vote_reply_stale_term_dropped); RUN_TEST(test_leader_progress_init_single_node); RUN_TEST(test_leader_progress_init_multi_node);