fix(ha): correct real-vote reply index mapping for middle-priority nodes (bug-1417)
vote_reply_dispatch passed the compressed, self-excluded local votes_idx (vctx->peer_idx, set by raft_cb_send_vote_request's absolute->local remap) straight through to raft_on_vote_reply, which indexes vote_replies[] in ABSOLUTE cluster space -- the same space raft_start_election's self-mark and per-peer send loop use. For a 3-node cluster's middle node (self_idx=1), the peer at absolute index 2 compresses to votes_idx=1, colliding with vote_replies[self_idx=1] -- the slot raft_start_election already marked for the self-vote. The library's per-peer dedup guard then silently drops the genuine reply as a duplicate, so votes_granted never reaches quorum even though the peer granted. Peer 0 is unaffected (compressed index equals absolute), which is why sequential elections and prior soak runs never surfaced this -- only a middle-priority node losing its higher-priority peer and depending on its lower peer's vote hits the collision. Fixed by routing through ha_repl_ack_abs_peer_idx (already existed, already unit-tested, already used for the analogous heartbeat-ACK path) before calling raft_on_vote_reply. ha_record_vote_reply immediately above is unchanged -- it correctly wants the compressed index for ctx->votes[]. Confirmed pre_vote_reply_dispatch needs no equivalent fix: its send path already stores the absolute index. New regression test drives a real 3-node election end-to-end through the actual decode/dispatch path (not the test-hook bypass every other real-vote test in this file uses): Expected 2 Was 1 pre-fix, GREEN post-fix. Full 79-group suite: zero new failures. 10-round --chaotic HA soak: zero committed divergence, zero committed job loss.
This commit is contained in:
parent
48b4dba700
commit
d5122752fe
3 changed files with 163 additions and 2 deletions
|
|
@ -12562,5 +12562,50 @@
|
|||
],
|
||||
"occurrences": 11,
|
||||
"last_seen": "2026-07-05T07:45:00.000Z"
|
||||
},
|
||||
{
|
||||
"id": "bug-1417",
|
||||
"timestamp": "2026-07-07T09:30:00.000Z",
|
||||
"error_message": "[xce-test-ha --chaotic] FAIL round 4/10: election stall after killing ctrl-1 \u2014 ctrl-2 PreVote wins but real election times out despite ctrl-3 granting RequestVote",
|
||||
"file": "src/controller/ha.c",
|
||||
"root_cause": "vote_reply_dispatch passes local votes_idx (compressed, excludes self) directly to raft_on_vote_reply which expects ABSOLUTE peer index (includes self). For self_idx=1 (ctrl-2), ctrl-3's vote (absolute=2, local=1) lands in vote_replies[1]=ctrl-2's own self-slot \u2192 treated as duplicate \u2192 vote silently dropped. With ctrl-1 dead in chaotic mode, ctrl-2 only has self-vote (1 < quorum 2) \u2192 eternal timeout. Self_idx=0 and self_idx=2 are partially/fully immune due to index alignment luck. ha_repl_ack_abs_peer_idx() already exists (ha.c:3779) with correct mapping and passing tests \u2014 vote_reply_dispatch just doesn't call it.",
|
||||
"fix": [
|
||||
"FIXED (src/controller/ha.c vote_reply_dispatch, ~line 5233): raft_on_vote_reply now receives ha_repl_ack_abs_peer_idx(ha, 1, idx) instead of the raw compressed local idx. ha_repl_ack_abs_peer_idx (ha.c:3779, already existed + unit-tested for the heartbeat-ACK path) performs the exact inverse of raft_cb_send_vote_request send-side remap: votes_idx < self_idx ? votes_idx : votes_idx + 1. ha_record_vote_reply (immediately preceding line) is UNCHANGED -- it correctly wants the compressed idx for ctx->votes[]. Confirmed pre_vote_reply_dispatch needs NO equivalent fix: raft_cb_send_pre_vote_request (ha.c:301) already stores the ABSOLUTE peer_idx in vctx->peer_idx for the PreVote path (explicit code comment there), matching raft_on_pre_vote_reply/pre_vote_replies[] absolute-index expectation -- only the real-vote send path (raft_cb_send_vote_request, ha.c:175) stores compressed. RED->GREEN: new unit test test_bug1417_middle_index_real_vote_from_peer2_not_dropped (test_ha_vote_send.c) drives a REAL 3-node election (self_idx=1) through raft_start_election then a genuine peer-2 grant via vote_reply_msg_handler_for_test (the actual decode/dispatch path, not the xce_vote_reply_hook bypass every other real-vote test in this file uses) -- reproduced Expected 2 Was 1 pre-fix, GREEN post-fix (votes_granted=2, role=RAFT_LEADER). Verified: full 79-group controller_unit_test sweep 0 new failures; 10-round --chaotic xce-test-ha soak PASS (committed_divergence=0, committed_job_loss=0, 18 jobs). Soak surfaced an UNRELATED, pre-existing --chaotic harness bug (run-ha-soak.sh run_chaotic_rounds: DOWN variable is not cleared on a skip-this-round no-primary-to-kill path, so a subsequent round calls start_node on the same already-running node without killing it first -- 6 stacked ctrl-2 processes observed piling up over rounds 5-10) -- confirmed via git diff this script was untouched by this fix, logged separately, NOT fixed here (out of bug-1417 boundary)."
|
||||
],
|
||||
"tags": [
|
||||
"vote",
|
||||
"raft",
|
||||
"self_idx",
|
||||
"index-mapping",
|
||||
"chaotic-failover",
|
||||
"off-by-one",
|
||||
"fixed",
|
||||
"ha",
|
||||
"ha_repl_ack_abs_peer_idx",
|
||||
"election"
|
||||
],
|
||||
"related_bugs": [
|
||||
"bug-828"
|
||||
],
|
||||
"occurrences": 1,
|
||||
"last_seen": "2026-07-07T10:20:00.000Z"
|
||||
},
|
||||
{
|
||||
"id": "bug-1418",
|
||||
"timestamp": "2026-07-07T10:20:00.000Z",
|
||||
"error_message": "[xce-test-ha --chaotic] rounds 6-10 of a 10-round soak all skip (no primary to kill) after round 5 hit a slow-but-eventually-successful election; ctrl-2 restarted repeatedly without the prior instance ever being killed -- 6 stacked ctrl-2 xce_controller processes observed alive simultaneously by end of run",
|
||||
"file": ".claude/skills/xce-test-ha/run-ha-soak.sh",
|
||||
"root_cause": "run_chaotic_rounds's DOWN variable tracks the node killed THIS round so the NEXT round can restart it. On a round where primary_from_log finds no primary (log \"no primary to kill this round -- skipping\"), the loop continues without ever updating DOWN or killing anything -- but the NEXT round's \"if [ -n \"\" ]\" restart-previously-downed step still fires unconditionally on the stale DOWN value from several rounds back, calling start_node on a node that was never actually killed (or was already restarted in a prior skipped round), stacking a new xce_controller process on top of an already-running one every single skipped round. Discovered incidentally while verifying bug-1417's fix (10-round chaotic soak, unrelated to the vote-index fix itself -- confirmed via git diff that run-ha-soak.sh was untouched this session).",
|
||||
"fix": "NOT FIXED (out of scope for the bug-1417 investigation that surfaced it). Candidate fix: only treat a node as DOWN (eligible for the next round's restart) if this round genuinely killed it; on a skip-round, DOWN should stay whatever it was (already handled -- the bug is that the skip round should also skip minus the restart-if-still-actually-down check), or more robust: before start_node(\"\"), pgrep for an existing xce_controller --myname and skip the restart if one is already running.",
|
||||
"tags": [
|
||||
"xce-test-ha",
|
||||
"chaotic-mode",
|
||||
"harness-bug",
|
||||
"restart-bookkeeping",
|
||||
"process-leak"
|
||||
],
|
||||
"related_bugs": [],
|
||||
"occurrences": 1,
|
||||
"last_seen": "2026-07-07T10:20:00.000Z"
|
||||
}
|
||||
]
|
||||
]
|
||||
|
|
@ -5230,7 +5230,24 @@ static void vote_reply_dispatch(ha_state_t *ha, ha_elect_ctx_t *ctx, int idx,
|
|||
* raft_cb_send_vote_request via the absolute→local mapping), which is exactly
|
||||
* the index ha_compute_safe_committed_lsn and the test hook use. */
|
||||
ha_record_vote_reply(ha, ctx, idx, term, vote_granted, peer_tag, peer_lsn);
|
||||
raft_on_vote_reply(&ha->raft, idx, vote_granted, term);
|
||||
|
||||
/* bug-1417: raft_on_vote_reply's from_idx must be the ABSOLUTE cluster
|
||||
* index (the space vote_replies[]/self_idx live in — raft_start_election
|
||||
* pre-marks vote_replies[self_idx] and sends to every absolute peer !=
|
||||
* self_idx, raft.c:113-153), NOT the compressed self-excluded votes_idx
|
||||
* `idx` carries for ctx->votes[] above. Passing the compressed idx
|
||||
* straight through collides with vote_replies[self_idx] whenever a
|
||||
* peer's compressed votes_idx numerically equals self_idx (true for any
|
||||
* peer at or above self_idx in a >2-node cluster, e.g. self_idx=1's
|
||||
* absolute peer 2 compresses to votes_idx=1) — raft_on_vote_reply's
|
||||
* per-peer dedup guard then silently treats the genuine reply as an
|
||||
* already-counted duplicate and drops it, so a middle-priority node can
|
||||
* never reach quorum on that peer's vote alone. ha_repl_ack_abs_peer_idx
|
||||
* (ha.c:3779) already implements the exact inverse of
|
||||
* raft_cb_send_vote_request's votes_idx remap and is unit-tested; reuse
|
||||
* it here (ep_is_vote=1) rather than duplicating the arithmetic. */
|
||||
raft_on_vote_reply(&ha->raft, ha_repl_ack_abs_peer_idx(ha, /*ep_is_vote=*/1, idx),
|
||||
vote_granted, term);
|
||||
}
|
||||
|
||||
/*
|
||||
|
|
|
|||
|
|
@ -557,6 +557,103 @@ static void test_vote_reply_handler_no_uaf(void)
|
|||
printf("test_vote_reply_handler_no_uaf: PASS\n");
|
||||
}
|
||||
|
||||
/* ── bug-1417: middle-index (self_idx=1) real-vote index-mapping regression ──
|
||||
*
|
||||
* raft_cb_send_vote_request (ha.c:142) stores the COMPRESSED local votes_idx
|
||||
* (self-excluded) in vctx->peer_idx: votes_idx = peer_idx < self_idx ?
|
||||
* peer_idx : peer_idx - 1. vote_reply_dispatch (ha.c:5208) then passes that
|
||||
* SAME compressed idx straight to raft_on_vote_reply, which indexes
|
||||
* s->vote_replies[] in ABSOLUTE space (the space raft_start_election's
|
||||
* self-mark and per-peer send loop both use, raft.c:113-153).
|
||||
*
|
||||
* For self_idx=1 (a 3-node cluster's middle node), the peer at absolute
|
||||
* index 2 maps to compressed votes_idx=1 -- which COLLIDES with
|
||||
* vote_replies[self_idx=1], the slot raft_start_election already marked as
|
||||
* counted for the self-vote. raft_on_vote_reply's per-peer dedup guard
|
||||
* (raft.c:477) then treats the genuine reply from peer 2 as a duplicate and
|
||||
* silently drops it -- vote_replies[1] was already 1, votes_granted stays
|
||||
* at 1 (self only), quorum (2) is never reached, and the election times out
|
||||
* even though peer 2 genuinely granted.
|
||||
*
|
||||
* Peer 0 is unaffected (compressed votes_idx=0 == absolute 0, no collision),
|
||||
* which is why sequential/all-peers-alive elections never surfaced this --
|
||||
* only a 3-node cluster's middle-priority node losing exactly its
|
||||
* higher-priority peer (index 0) and depending on its lower peer's (index 2)
|
||||
* vote exposes the collision.
|
||||
*/
|
||||
|
||||
static void make_three_node_self1(ha_state_t *ha)
|
||||
{
|
||||
ha->raft.n_peers = 3;
|
||||
ha->raft.self_idx = ha->my_priority; /* my_priority == 1 for this test */
|
||||
ha->raft.role = RAFT_FOLLOWER;
|
||||
ha->n_peers = 3;
|
||||
}
|
||||
|
||||
static void test_bug1417_middle_index_real_vote_from_peer2_not_dropped(void)
|
||||
{
|
||||
reset_reply_mock();
|
||||
xce_vote_reply_hook = NULL; /* drive the REAL raft_on_vote_reply path, not the hook */
|
||||
|
||||
ha_state_t ha;
|
||||
TEST_ASSERT_EQUAL_INT(0, ha_init(&ha, &g_vote_cfg, /*my_priority=*/1));
|
||||
make_three_node_self1(&ha);
|
||||
|
||||
/* Start a real election: term++, self-vote, vote_replies[self_idx=1]=1
|
||||
* pre-marked (raft.c:122-123), CANDIDATE role, one on_send_vote_request
|
||||
* call per non-self absolute peer (0 and 2) -- c==NULL so each send is a
|
||||
* safe no-op (raft_cb_send_vote_request returns 1 immediately), exactly
|
||||
* the same test-mode pattern test_ha_prevote.c uses for
|
||||
* ha_start_prevote_async(&ha, NULL). */
|
||||
int rc = raft_start_election(&ha.raft);
|
||||
TEST_ASSERT_EQUAL_INT_MESSAGE(RAFT_OK, rc,
|
||||
"3-node multi-node election must return RAFT_OK (CANDIDATE transition)");
|
||||
TEST_ASSERT_EQUAL_INT_MESSAGE(RAFT_CANDIDATE, (int)ha.raft.role,
|
||||
"raft role must be CANDIDATE after starting a 3-node election");
|
||||
TEST_ASSERT_EQUAL_INT_MESSAGE(1, ha.raft.votes_granted,
|
||||
"self-vote must be counted (1) before any peer reply");
|
||||
TEST_ASSERT_EQUAL_INT_MESSAGE(1, ha.raft.vote_replies[1],
|
||||
"self_idx's own vote_replies slot must be pre-marked");
|
||||
|
||||
ha_elect_ctx_t *ctx = alloc_ctx_for_vote(2); /* n_other = n_peers - 1 = 2 */
|
||||
pthread_mutex_init(&ctx->lock, NULL);
|
||||
|
||||
uint64_t term = atomic_load_explicit(&ha.term, memory_order_acquire);
|
||||
|
||||
/* Peer at ABSOLUTE index 2 grants. raft_cb_send_vote_request's mapping
|
||||
* (peer_idx=2 >= self_idx=1) computes votes_idx = 2 - 1 = 1 -- this IS
|
||||
* the value production code stores in vctx->peer_idx for this peer, so
|
||||
* the test reproduces the exact wire-level index the real send path
|
||||
* would have used. */
|
||||
endpoint_t ep2;
|
||||
make_vote_reply_ep(&ep2, &ha, ctx, /*peer_idx (compressed votes_idx)=*/1);
|
||||
|
||||
io_packet_t *pkt2 = vote_reply_build_pkt_for_test(term, /*granted=*/1, 0ULL, 0ULL);
|
||||
TEST_ASSERT_NOT_NULL_MESSAGE(pkt2, "vote_reply_build_pkt_for_test must succeed");
|
||||
|
||||
vote_reply_msg_handler_for_test(&ep2, pkt2);
|
||||
|
||||
/* This is the bug: without ha_repl_ack_abs_peer_idx remapping compressed
|
||||
* votes_idx=1 back to absolute peer_idx=2 before calling
|
||||
* raft_on_vote_reply, the reply lands in vote_replies[1] -- the
|
||||
* self-vote's own slot, already 1 -- and is silently discarded as a
|
||||
* duplicate. votes_granted stays at 1 (self only), never reaching the
|
||||
* 2-vote quorum, even though peer 2 genuinely granted. */
|
||||
TEST_ASSERT_EQUAL_INT_MESSAGE(2, ha.raft.votes_granted,
|
||||
"bug-1417: peer 2's genuine grant must be counted (votes_granted==2), "
|
||||
"not silently dropped into the self-vote's slot");
|
||||
TEST_ASSERT_EQUAL_INT_MESSAGE(RAFT_LEADER, (int)ha.raft.role,
|
||||
"bug-1417: with self-vote + peer 2's grant, quorum (2) is reached and "
|
||||
"the node must become LEADER");
|
||||
TEST_ASSERT_EQUAL_INT_MESSAGE(1, ha.raft.election_decided,
|
||||
"election must be decided (won) once quorum is reached");
|
||||
|
||||
free(ep2.data);
|
||||
free_ctx_for_vote(ctx);
|
||||
ha_fini(&ha);
|
||||
printf("test_bug1417_middle_index_real_vote_from_peer2_not_dropped: PASS\n");
|
||||
}
|
||||
|
||||
/* ── Suite entry point ───────────────────────────────────────────────────── */
|
||||
|
||||
void test_ha_vote_send(void)
|
||||
|
|
@ -570,4 +667,6 @@ void test_ha_vote_send(void)
|
|||
RUN_TEST(test_vote_reply_handler_denied_vote);
|
||||
RUN_TEST(test_vote_reply_handler_term_fencing);
|
||||
RUN_TEST(test_vote_reply_handler_no_uaf);
|
||||
/* bug-1417 */
|
||||
RUN_TEST(test_bug1417_middle_index_real_vote_from_peer2_not_dropped);
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue