From 88a8bf8a4725732e58c6ee0dcc6c80b95701c6cf Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Sun, 4 Oct 2026 09:14:38 +0000 Subject: [PATCH] fix(controller,lib): decide each PreVote round once and promote only with this election's votes (bug-1915) A late PreVote grant after WON re-ran the WON path and started a second real election that replaced elect_ctx, so the term-2 leader promoted no vote endpoints. Rounds now carry an id and endpoints a round/kind; stale or wrong-kind replies, timeouts and errors are dropped; raft ignores replies with no round in progress. raft_cb_become_leader promotes only with its own stamped election record (own term, majority), else steps down and re-arms. raft_on_leader_lost keeps voted_for (one vote per term) and the delayed election starts only as a follower in the lost term. The failover stagger hands the PreVote start to a vtask so the delaymgr stays trigger-only. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/controller/ha.c | 1126 +++++++++++++--- src/controller/ha.h | 15 + src/controller/testsuite/test_ha_prevote.c | 1250 ++++++++++++++++++ src/controller/testsuite/test_ha_vote_send.c | 9 + src/lib/raft/raft.c | 31 +- src/lib/raft/raft.h | 23 +- src/lib/testsuite/test_raft.c | 210 ++- 7 files changed, 2463 insertions(+), 201 deletions(-) diff --git a/src/controller/ha.c b/src/controller/ha.c index c9aac532..e8141af4 100644 --- a/src/controller/ha.c +++ b/src/controller/ha.c @@ -49,6 +49,12 @@ typedef struct { cluster_t *c; ha_elect_ctx_t *ctx; int peer_idx; + /* bug-1915 follow-up: endpoint kind. 1 = PreVote endpoint + * (raft_cb_send_pre_vote_request); its replies and errors are routed ONLY + * to the PreVote tally. Part of the layout prefix shared with + * standby_ep_ctx_t (which keeps it 0), because peer_ctrl_msg_handler reads + * it from every peer-control endpoint's ep->data. */ + int is_pre_vote; uint64_t my_term; uint64_t my_tag; uint64_t my_lsn; @@ -59,6 +65,11 @@ typedef struct { uint64_t my_last_log_term; uint64_t my_last_log_index; const char *my_name; + /* bug-1915 follow-up: PreVote round identity (valid only when + * is_pre_vote). Captures ha->pre_vote_round at send time; a reply or + * error is dropped unless that round is still the current, undecided one + * (PreVote replies carry no round id on the wire). */ + uint64_t pre_vote_round; } vote_ep_ctx_t; /* election_timeout_ctx_t: delaymgr callback context for raft election timeout */ @@ -519,12 +530,13 @@ static int pre_vote_ep_err_handler(void *ep_raw); * production path a real reply uses (see vote_reply_dispatch's own comment, * below, for why this must never be the legacy handle_request_vote_reply). */ static void vote_reply_dispatch(ha_state_t *ha, ha_elect_ctx_t *ctx, int idx, - uint64_t term, int vote_granted, + uint64_t elect_term, uint64_t term, + int vote_granted, uint64_t peer_tag, uint64_t peer_lsn); /* bug-1423 (PreVote twin): same rationale as vote_reply_dispatch above, for * raft_cb_send_pre_vote_request's connect/monitor failure branches. */ static void pre_vote_reply_dispatch(ha_state_t *ha, cluster_t *c, - int peer_idx, int granted); + uint64_t round, int peer_idx, int granted); /* bug-1197: defer a vote-ep teardown onto the per-ep FIFO of the vtask pool so * it runs strictly after the last live use of the ep (flag-ON loser path). */ static void vote_ep_teardown_async(ha_state_t *ha, endpoint_t *ep); @@ -730,7 +742,8 @@ static int raft_cb_send_vote_request(void *ctx, int peer_idx, uint64_t term) if (!c) handle_request_vote_reply(ha, ectx, votes_idx, term, 0, 0, 0); else - vote_reply_dispatch(ha, ectx, votes_idx, term, /*vote_granted=*/0, 0, 0); + vote_reply_dispatch(ha, ectx, votes_idx, /*elect_term=*/term, term, + /*vote_granted=*/0, 0, 0); return 1; } @@ -744,7 +757,8 @@ static int raft_cb_send_vote_request(void *ctx, int peer_idx, uint64_t term) if (!c) handle_request_vote_reply(ha, ectx, votes_idx, term, 0, 0, 0); else - vote_reply_dispatch(ha, ectx, votes_idx, term, /*vote_granted=*/0, 0, 0); + vote_reply_dispatch(ha, ectx, votes_idx, /*elect_term=*/term, term, + /*vote_granted=*/0, 0, 0); return 1; } @@ -814,6 +828,11 @@ static int raft_cb_send_pre_vote_request(void *ctx, int peer_idx, * [0, n_peers) and raft_on_pre_vote_reply dedups by that same index). */ vctx->peer_idx = peer_idx; vctx->my_term = prospective_term; + /* bug-1915 follow-up: this is a PreVote endpoint of the round + * ha_start_prevote_async just opened (we run inside raft_begin_pre_vote, + * under raft_lock, so ha->pre_vote_round is that round's id). */ + vctx->is_pre_vote = 1; + vctx->pre_vote_round = ha->pre_vote_round; vctx->my_tag = (ectx->n_peers > 0) ? ectx->votes[0].my_tag : 0; vctx->my_lsn = (ectx->n_peers > 0) ? ectx->votes[0].my_lsn : 0; /* ha-term-safety task 1.2 + bug-851/task 5.5: advertise lastLogTerm as the @@ -864,7 +883,8 @@ static int raft_cb_send_pre_vote_request(void *ctx, int peer_idx, * round timeout instead of concluding as soon as the live peer * replies. */ if (c) - pre_vote_reply_dispatch(ha, c, peer_idx, /*granted=*/0); + pre_vote_reply_dispatch(ha, c, ha->pre_vote_round, peer_idx, + /*granted=*/0); return 1; } @@ -873,7 +893,8 @@ static int raft_cb_send_pre_vote_request(void *ctx, int peer_idx, endpoint_unref(ep); /* bug-1298: fini+free */ /* bug-1423 (PreVote twin): same fix as the connect-failure branch above. */ if (c) - pre_vote_reply_dispatch(ha, c, peer_idx, /*granted=*/0); + pre_vote_reply_dispatch(ha, c, ha->pre_vote_round, peer_idx, + /*granted=*/0); return 1; } @@ -1026,24 +1047,70 @@ static void raft_cb_cancel_timeout(void *ctx) ha->raft_election_timeout_ctx = NULL; } -/* raft_election_start_vtask_entry: vtask entry wrapping raft_check_and_start_ - * election. raft_start_election's on_persist callback (raft_cb_persist -> - * ha_persist_term) does a synchronous fsync -- must never run inline on the - * delaymgr thread (ha-async-io.md thread-role contract: delaymgr only fires - * timers and triggers work, then returns; it never blocks, since the same - * thread also drives heartbeats and every other election timeout). */ +static void ha_enter_real_election(ha_state_t *ha, cluster_t *c); + +/* + * ha_check_and_start_election (bug-1915 review round 2): the controller's + * body for the library's delayed-election trigger (raft_on_leader_lost -> + * on_start_election_after_delay). It replaces a direct call to + * raft_check_and_start_election, which ran raft_start_election WITHOUT the + * controller's start step: no ha->elect_ctx carrier stamped with the new term, + * no CTRL_ELECTING transition, no supersession of an open PreVote round — so + * raft_cb_become_leader would refuse every win from it (and it raced the + * carrier swap the stamped path does under raft_lock). + * + * Same decision as raft_check_and_start_election: skip unless this node is + * still a FOLLOWER in the term in which the leader was lost (bug-1915 final + * review: raft_on_leader_lost now KEEPS voted_for — the vote cast in the + * unchanged term still binds — so "voted for another candidate" is no longer + * the signal; a term change during the delay, e.g. a grant to another + * candidate's higher term or a new leader, is); otherwise start the election — through ha_enter_real_election -> + * ha_start_election_async, the one path that stamps the carrier and runs + * raft_start_election atomically under raft_lock (and refuses, with no side + * effect, a node that is already CANDIDATE/LEADER). Held under raft_lock + * (recursive) so the check and the start are one step. + * + * Runs on a pool vtask (VTASK_LIVE_BLOCK; see ha_dispatch_election_start): + * raft_start_election persists the term (fsync). No production caller of + * raft_on_leader_lost exists today (the RDHUP/liveness triggers go through + * PreVote), so this path is dormant; it is kept correct, not removed, because + * the library requires the callback. + */ +static void ha_check_and_start_election(ha_state_t *ha) +{ + pthread_mutex_lock(&ha->raft_lock); + if (ha->raft.term != ha->raft.leader_lost_term + || ha->raft.role != RAFT_FOLLOWER) { + uint64_t term = ha->raft.term; + uint64_t lterm = ha->raft.leader_lost_term; + int rrole = (int)ha->raft.role; + pthread_mutex_unlock(&ha->raft_lock); + INFO("HA: delayed election skipped — cluster moved on since the leader " + "was lost (term=%" PRIu64 ", leader-lost term=%" PRIu64 + ", raft role=%d)", term, lterm, rrole); + return; + } + ha_enter_real_election(ha, ha->raft_c); + pthread_mutex_unlock(&ha->raft_lock); +} + +/* raft_election_start_vtask_entry: vtask entry wrapping + * ha_check_and_start_election. raft_start_election's on_persist callback + * (raft_cb_persist -> ha_persist_term) does a synchronous fsync -- must never + * run inline on the delaymgr thread (ha-async-io.md thread-role contract: + * delaymgr only fires timers and triggers work, then returns; it never blocks, + * since the same thread also drives heartbeats and every other election + * timeout). */ static int raft_election_start_vtask_entry(vtask_t *task) { ha_state_t *ha = (ha_state_t *)task->data->u.generic; - /* bug-1435: serialize against concurrent vote-reply/vote-request - * processing on the pool (see ha.h raft_lock doc). */ - pthread_mutex_lock(&ha->raft_lock); - raft_check_and_start_election(&ha->raft); - pthread_mutex_unlock(&ha->raft_lock); + /* bug-1435: serialized against concurrent vote-reply/vote-request + * processing on the pool by raft_lock, taken inside. */ + ha_check_and_start_election(ha); return 0; } -/* ha_dispatch_election_start: offload raft_check_and_start_election onto a +/* ha_dispatch_election_start: offload ha_check_and_start_election onto a * vtask (VTASK_LIVE_BLOCK -- the same class already used for the vote-GRANT * term fsync, R4.6) so its persist-before-campaign fsync never blocks the * delaymgr thread. On pool unavailability/enqueue failure, fail loud and run @@ -1061,9 +1128,7 @@ static void ha_dispatch_election_start(ha_state_t *ha) (void *)pool, (void *)data, (void *)task); free(data); free(task); - pthread_mutex_lock(&ha->raft_lock); - raft_check_and_start_election(&ha->raft); - pthread_mutex_unlock(&ha->raft_lock); + ha_check_and_start_election(ha); return; } @@ -1076,9 +1141,7 @@ static void ha_dispatch_election_start(ha_state_t *ha) "election inline (stay alive)"); free(data); free(task); - pthread_mutex_lock(&ha->raft_lock); - raft_check_and_start_election(&ha->raft); - pthread_mutex_unlock(&ha->raft_lock); + ha_check_and_start_election(ha); } } @@ -1098,7 +1161,7 @@ static int raft_rdhup_delay_fn(void *arg) /* * raft_cb_start_election_after_delay: schedule RDHUP retry timer. - * After delay_ms, calls raft_check_and_start_election to start election + * After delay_ms, runs ha_check_and_start_election to start the election * only if this node hasn't already voted for another candidate. */ static void raft_cb_start_election_after_delay(void *ctx, uint32_t delay_ms) @@ -1160,6 +1223,48 @@ static int _cmp_u64_desc(const void *a, const void *b) return 0; } +/* + * ha_promotion_carrier_defect (bug-1915 follow-up): is `ctx` the vote record of + * the election that just made this node leader at `term`? Returns NULL if it + * may be used for promotion, else a short reason. + * + * - NULL ctx: accepted (legacy / unit shims promote without one; production + * always publishes one before raft_start_election). + * - a PreVote round carrier (is_pre_vote): never holds real votes — refused. + * - a carrier stamped by ha_start_election_async (election_term != 0) must + * be stamped with `term`, and must record a voted majority of the cluster + * it was built for (self + granted voters). Every grant raft_on_vote_reply + * counts was recorded into this carrier first (vote_reply_dispatch records + * before it tallies), so a real win always satisfies this. + * - unstamped carriers (election_term == 0: unit-test fixtures) are not + * second-guessed. + */ +static const char *ha_promotion_carrier_defect(ha_elect_ctx_t *ctx, + uint64_t term) +{ + if (ctx == NULL) + return NULL; + if (ctx->is_pre_vote) + return "elect_ctx is a PreVote round carrier, not this election's " + "vote record"; + if (ctx->election_term == 0) + return NULL; + if (ctx->election_term != term) + return "elect_ctx belongs to a different election term"; + if (ctx->n_peers > 0) { + int granted = 1; /* self */ + pthread_mutex_lock(&ctx->lock); + for (int i = 0; i < ctx->n_peers; i++) + if (ctx->votes[i].vote_granted) + granted++; + pthread_mutex_unlock(&ctx->lock); + int quorum = (ctx->n_peers + 1) / 2 + 1; + if (granted < quorum) + return "elect_ctx records fewer granted voters than a majority"; + } + return NULL; +} + void ha_compute_safe_committed_lsn(ha_elect_ctx_t *ctx, uint64_t *safe_lsn_out, uint64_t *safe_tag_out) @@ -1631,6 +1736,8 @@ loud_fence_no_timer: * * ha->raft_c must be non-NULL (production path only; test mode uses noop stubs). */ +static void raft_cb_become_follower(void *ctx, uint64_t term, int leader_idx); + static void raft_cb_become_leader(void *ctx, uint64_t term) { ha_state_t *ha = (ha_state_t *)ctx; @@ -1646,6 +1753,59 @@ static void raft_cb_become_leader(void *ctx, uint64_t term) atomic_store_explicit(&ha->term, term, memory_order_release); ha_clear_catchup_kicks(ha); /* bug-1913: arms are per leadership */ + /* bug-1915 follow-up: promote only with THIS election's vote record. + * Everything below — the safe commit point (ha_compute_safe_committed_lsn), + * the truncate floor, persist_promotion_advance_committed, the eligibility + * gate's peer_reachable and the promoted standby endpoints — is read from + * ha->elect_ctx. If that carrier is a PreVote round's, or was stamped + * for another election, or records no voted majority, the safe commit + * point would be computed from self alone (n_granted = 1 in a 3-node + * cluster) and committed_lsn advanced to this node's own durable tip, + * declaring possibly-uncommitted entries committed. ha_start_election_async + * and ha_start_prevote_async no longer replace the carrier of an election + * in flight, so this is unreachable; if it ever fires it is a bug, and the + * safe response is the one every other promotion refusal here takes: + * refuse loudly, do NOT become CTRL_PRIMARY, stay alive (never + * self-terminate). + * + * bug-1915 review round 2: and do not stay a raft LEADER that is not + * CTRL_PRIMARY — such a node neither serves nor campaigns, and every + * self-recovery path refuses it (gap (b) refuses PreVote on a LEADER / + * CTRL_ELECTING node; ha_start_election_async refuses a LEADER), with no + * election timer armed. Step down to FOLLOWER in the SAME term: + * - Raft-safe: a server may revert to follower at any time; term and + * voted_for are untouched (it keeps its self-vote for this term, so + * it can never vote for a second candidate in it), and nothing was + * appended or committed as leader; + * - recovery: raft_cb_become_follower (the same adapter path every + * loser/step-down takes) sets CTRL_STANDBY, closes this election's + * vote endpoints, marks the node leaderless (no leader known) and + * re-arms the liveness watchdog, so the next timeout opens a PreVote + * round for term+1 — no other member needs to act first. + * raft_lock is recursive: the library calls on_become_leader under it + * (vote_reply_dispatch / ha_start_election_async). The library's + * on_cancel_timeout after this callback cancels only the raft election + * timer, not the liveness watchdog armed here. */ + { + ha_elect_ctx_t *chk = (ha_elect_ctx_t *)atomic_load_explicit( + &ha->elect_ctx, memory_order_acquire); + const char *defect = ha_promotion_carrier_defect(chk, term); + if (defect != NULL) { + ERROR("HA: promotion REFUSED for term %" PRIu64 " — %s; NOT " + "becoming CTRL_PRIMARY (would compute the safe commit point " + "without this election's voters). Stepping down to " + "FOLLOWER/CTRL_STANDBY in the same term and re-arming the " + "liveness watchdog; member stays ALIVE and campaigns again " + "on the next timeout (bug-1915 invariant violated — please " + "report)", term, defect); + pthread_mutex_lock(&ha->raft_lock); + ha->raft.role = RAFT_FOLLOWER; + pthread_mutex_unlock(&ha->raft_lock); + raft_cb_become_follower(ctx, term, /*leader_idx=*/-1); + return; + } + } + /* The cluster committed high-water mark for this promotion — derived from * the voter replies (NOT a stale ctrlhello). The task-4.3 pre-promotion * eligibility gate (below) compares the local CLAMPED durable position @@ -6429,7 +6589,7 @@ static int repl_ack_decode_and_dispatch(endpoint_t *ep, vote_ep_ctx_t *vctx, static int pre_vote_reply_decode_and_dispatch(vote_ep_ctx_t *vctx, io_packet_t *pkt); static void pre_vote_reply_dispatch(ha_state_t *ha, cluster_t *c, - int peer_idx, int granted); + uint64_t round, int peer_idx, int granted); #ifdef XCE_TESTING /* @@ -6566,7 +6726,8 @@ int (*xce_vote_reply_hook)(ha_state_t *ha, ha_elect_ctx_t *ctx, int peer_idx, * Test path: xce_vote_reply_hook captures decoded fields without real iomgr. */ static void vote_reply_dispatch(ha_state_t *ha, ha_elect_ctx_t *ctx, int idx, - uint64_t term, int vote_granted, + uint64_t elect_term, uint64_t term, + int vote_granted, uint64_t peer_tag, uint64_t peer_lsn) { #ifdef XCE_TESTING @@ -6588,8 +6749,14 @@ static void vote_reply_dispatch(ha_state_t *ha, ha_elect_ctx_t *ctx, int idx, * the quorum tally and fires on_become_leader) — so there is no double * promotion. `idx` is vctx->peer_idx, the local ctx->votes[] index (set in * 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); + * the index ha_compute_safe_committed_lsn and the test hook use. + * + * Recorded under raft_lock (below) together with the stale-election + * check, so a dropped stale reply never marks ctx->votes[idx] replied + * either. raft_lock -> ctx->lock is the existing order + * (raft_cb_become_leader's carrier check takes ctx->lock under raft_lock); + * ctx->lock is a leaf here. */ + int abs_idx = ha_repl_ack_abs_peer_idx(ha, /*ep_is_vote=*/1, idx); /* 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 @@ -6623,8 +6790,34 @@ static void vote_reply_dispatch(ha_state_t *ha, ha_elect_ctx_t *ctx, int idx, * callback it fires are atomic with respect to other raft_state_t * entry points. */ pthread_mutex_lock(&ha->raft_lock); - raft_on_vote_reply(&ha->raft, ha_repl_ack_abs_peer_idx(ha, /*ep_is_vote=*/1, idx), - vote_granted, term); + + /* bug-1915 final review (finding 3): drop a reply that belongs to an + * EARLIER election — the request was sent in elect_term (vctx->my_term), + * this node has since moved to another term. A voter that has already + * adopted the current term answers a late RequestVote(elect_term) with + * granted=0 and the CURRENT term, so raft_on_vote_reply's own term filter + * (DropStaleResponse, term < s->term) does not catch it: it was counted as + * that peer's deny in the current election, and the peer's genuine reply + * then dropped as a duplicate (liveness). Checked under raft_lock, the + * lock every term change holds. + * One exception keeps Raft's universal rule: a reply carrying a term + * HIGHER than ours still steps this node down (raft_on_vote_reply's + * Step 1 adopts the term and never counts the vote). */ + if (elect_term != ha->raft.term) { + uint64_t cur = ha->raft.term; + if (term > cur) + raft_on_vote_reply(&ha->raft, abs_idx, /*granted=*/0, term); + pthread_mutex_unlock(&ha->raft_lock); + if (term <= cur) + INFO("HA: dropping vote reply from peer %d for the term-%" PRIu64 + " election (reply term=%" PRIu64 ", granted=%d) — this node " + "is now in term %" PRIu64, abs_idx, elect_term, term, + vote_granted, cur); + return; + } + + ha_record_vote_reply(ha, ctx, idx, term, vote_granted, peer_tag, peer_lsn); + raft_on_vote_reply(&ha->raft, abs_idx, vote_granted, term); pthread_mutex_unlock(&ha->raft_lock); } @@ -6709,9 +6902,10 @@ done: memory_order_acquire)) break; } - vote_reply_dispatch(ha, ctx, idx, vr_term, vr_granted, vr_tag, vr_lsn); + vote_reply_dispatch(ha, ctx, idx, vctx->my_term, vr_term, vr_granted, + vr_tag, vr_lsn); } else { - vote_reply_dispatch(ha, ctx, idx, + vote_reply_dispatch(ha, ctx, idx, vctx->my_term, atomic_load_explicit(&ha->term, memory_order_acquire), 0, 0, 0); } @@ -6786,7 +6980,8 @@ done: /* A malformed / wrong-body reply counts as a non-grant (best-effort * delivery, per design Event Contract). No term adoption either way. */ - pre_vote_reply_dispatch(ha, c, idx, ok ? vr_granted : 0); + pre_vote_reply_dispatch(ha, c, vctx->pre_vote_round, idx, + ok ? vr_granted : 0); return 0; } @@ -7108,11 +7303,12 @@ int ha_repl_ack_dispatch_for_test(ha_state_t *ha, endpoint_t *ep, /* * peer_ctrl_msg_handler_inline: the state-aware peer-control routing body. - * Routes by ha->role at dispatch time — no explicit handler reassignment needed. - * CTRL_ELECTING → vote_reply_decode_and_dispatch - * CTRL_PRIMARY → repl_ack_decode_and_dispatch - * CTRL_STANDBY + PreVote → pre_vote_reply_decode_and_dispatch (task 5.1) - * any other role → drop packet (WARN) + * Routes at dispatch time — no explicit handler reassignment needed. + * PreVote endpoint (any role) → pre_vote_reply_decode_and_dispatch (task 5.1; + * by endpoint kind since the bug-1915 follow-up) + * CTRL_ELECTING → vote_reply_decode_and_dispatch + * CTRL_PRIMARY → repl_ack_decode_and_dispatch + * any other role → drop packet (WARN) * * Packet ownership (raft-drive-async task 3.1): `pkt` is passed in explicitly — * the caller (peer_ctrl_msg_handler) has already taken it off ep->fsm_in.pkt and @@ -7129,13 +7325,19 @@ int ha_repl_ack_dispatch_for_test(ha_state_t *ha, endpoint_t *ep, * OUTBOUND PreVote endpoint while the candidate's role is still CTRL_STANDBY * (a PreVote round performs no role change — Req 1.1). Without this branch the * reply would hit the default case and be dropped, stranding the round until it - * times out. We gate on raft.pre_vote_in_progress so a STANDBY node with no - * active round still drops unexpected traffic. + * times out. bug-1915 follow-up: the branch is taken by endpoint kind + * (vctx->is_pre_vote) in every role, and pre_vote_reply_dispatch drops the + * reply unless its round is still the open one — so a node with no active + * round still drops the traffic, and a late PreVote reply can never reach the + * real-vote or repl-ack decoders. */ static int peer_ctrl_msg_handler_inline(endpoint_t *ep, io_packet_t *pkt) { vote_ep_ctx_t *vctx = (vote_ep_ctx_t *)ep->data; ha_state_t *ha = vctx->ha; + /* Endpoint kind (bug-1915 follow-up) is fixed at creation; read it with + * ha, before anything below can run. */ + int is_pre_vote_ep = vctx->is_pre_vote; /* raft-drive-async task 3.1: test-only observation seam — fires before any * routing so a test can count/timestamp/park the body. NULL in production. */ @@ -7145,8 +7347,21 @@ static int peer_ctrl_msg_handler_inline(endpoint_t *ep, io_packet_t *pkt) ctrl_role_t role = (ctrl_role_t)atomic_load_explicit(&ha->role, memory_order_acquire); - /* PreVote replies land while CTRL_STANDBY with a round in progress. */ - if (role == CTRL_STANDBY && ha->raft.pre_vote_in_progress) + /* bug-1915 follow-up: route by ENDPOINT KIND, not by role. A PreVote + * reply and a RequestVote reply share one wire payload, so the decoder must + * be chosen by which request this endpoint carried: + * - a PreVote endpoint's traffic goes ONLY to the PreVote tally, which + * drops it unless its round is still the open one. Routed by role, a + * PreVote GRANT that arrived after its round was WON (role now + * CTRL_ELECTING) was decoded as a REAL vote and counted by + * raft_on_vote_reply — a PreVote grant records no vote, so that could + * elect a second leader in the term (Election Safety); + * - a real-vote endpoint's traffic never reaches the PreVote tally. + * Routed by role, a lost election's late RequestVoteReply arriving + * while a new PreVote round was open (CTRL_STANDBY) was tallied as a + * PreVote reply — under its compressed votes[] index, i.e. possibly as + * a different peer. */ + if (is_pre_vote_ep) return pre_vote_reply_decode_and_dispatch(vctx, pkt); switch (role) { @@ -7925,6 +8140,7 @@ static int vote_ep_err_handler(void *ep_raw) * tally, same path a connect-time failure or a delivered denial * uses (raft-adapter-conformance Requirement 4.2). */ vote_reply_dispatch(ha, vctx->ctx, vctx->peer_idx, vctx->my_term, + vctx->my_term, /*vote_granted=*/0, 0, 0); } @@ -7953,17 +8169,32 @@ static int pre_vote_ep_err_handler(void *ep_raw) if (!vctx) return 0; ha_state_t *ha = vctx->ha; + + /* bug-1915 review round 2: route by ENDPOINT KIND first, exactly like the + * reply path (peer_ctrl_msg_handler_inline). A PreVote endpoint is never + * recorded in ha_elect_ctx_t.votes[], so it is never promoted into a + * standby channel: its error goes ONLY to the PreVote tally, in every role + * — pre_vote_reply_dispatch drops it unless its round is still the open + * one. Routed by role, a late PreVote endpoint error on a node that had + * meanwhile become CTRL_PRIMARY ran the promoted-standby cleanup + * (standby_endpoints / WAL replicator deregistration, quorum-transition + * edge) for an endpoint that was never a standby channel. */ + if (vctx->is_pre_vote) { + /* Count as a not-granted PreVote reply through the production tally, + * same path a connect-time failure uses (raft-adapter-conformance + * Requirement 4.3). */ + pre_vote_reply_dispatch(ha, vctx->c, vctx->pre_vote_round, + vctx->peer_idx, /*granted=*/0); + return 0; + } + + /* Not a PreVote endpoint (unreachable: this handler is registered only by + * raft_cb_send_pre_vote_request, which stamps is_pre_vote) — keep the + * role-based routing the real-vote handler uses. */ ctrl_role_t role = (ctrl_role_t)atomic_load_explicit(&ha->role, memory_order_acquire); - - if (role == CTRL_PRIMARY) { + if (role == CTRL_PRIMARY) vote_ep_handle_promoted_disconnect(ha, ep, vctx); - } else { - /* Election phase: count as a not-granted PreVote reply through the - * production tally, same path a connect-time failure uses - * (raft-adapter-conformance Requirement 4.3). */ - pre_vote_reply_dispatch(ha, vctx->c, vctx->peer_idx, /*granted=*/0); - } /* Do NOT free vctx here — peer_ctrl_msg_handler may still reference ep->data. * vctx is freed in ha_election_cleanup() when all per-peer contexts release. */ @@ -7994,8 +8225,8 @@ int pre_vote_ep_err_handler_for_test(endpoint_t *ep) void ha_init_raft(ha_state_t *ha, cluster_t *c, int n_peers) { /* Map persisted voted_for_name back to a peer index for initial_voted_for. - * This lets raft_check_and_start_election (RDHUP path) see the prior vote - * without re-reading the file. */ + * This lets raft_on_vote_request's one-vote-per-term check see the prior + * vote without re-reading the file. */ int restored_voted_for = -1; if (ha->voted_for_name[0] != '\0' && c && c->myconf) { const xce_cluster_config_t *pcfg = &c->myconf->peers_cfg; @@ -8080,12 +8311,22 @@ typedef struct { _Atomic int cancelled; ha_state_t *ha; cluster_t *c; + uint64_t round; /* bug-1915 follow-up: ha->pre_vote_round it times */ } prevote_round_ctx_t; /* Active round-timeout context. _Atomic so arm (vtask) vs timeout-fire * (delaymgr) vs cancel-on-decision is well-defined. */ static prevote_round_ctx_t * _Atomic s_prevote_round_ctx = NULL; +#ifdef XCE_TESTING +/* bug-1915 follow-up: number of PreVote rounds decided WON (test observation + * only — the real-thread concurrent-reply test asserts one per round). */ +_Atomic int ha_prevote_won_count_for_test = 0; +uint64_t ha_pre_vote_round_for_test(ha_state_t *ha); +void ha_prevote_round_timeout_fire_round_for_test(ha_state_t *ha, cluster_t *c, + uint64_t round); +#endif + #ifdef XCE_TESTING /* * ha_prevote_rearm_liveness_hook: test-only seam. When non-NULL, the LOST / @@ -8093,6 +8334,21 @@ static prevote_round_ctx_t * _Atomic s_prevote_round_ctx = NULL; * test can count re-arms without a live delaymgr (Req 5.4). */ int (*ha_prevote_rearm_liveness_hook)(ha_state_t *ha, cluster_t *c) = NULL; + +/* bug-1915 review round 2: round-timeout arm seams. + * ha_prevote_pre_arm_hook — called by ha_start_prevote_async after the round + * is opened and immediately before its round timeout is armed (with + * raft_lock held — recursive, so the hook may close/open rounds on the + * same thread), so a test can interleave another round's close/open at + * exactly that point. + * ha_prevote_round_timer_arm_hook — when non-NULL, replaces delay_task_ms + * for the round timeout (also when no delaymgr is running); a negative + * return simulates a scheduling failure. The hook receives the + * prevote_round_ctx_t (opaque) and owns nothing — the test frees it. */ +void (*ha_prevote_pre_arm_hook)(ha_state_t *ha, cluster_t *c, + uint64_t round) = NULL; +int (*ha_prevote_round_timer_arm_hook)(ha_state_t *ha, cluster_t *c, + uint64_t round, void *tctx) = NULL; #endif /* ha_prevote_rearm_liveness: re-arm the standby liveness watchdog after a lost @@ -8111,17 +8367,89 @@ static void ha_prevote_rearm_liveness(ha_state_t *ha, cluster_t *c) /* ha_prevote_cancel_round_timeout: mark the active round-timeout context * cancelled (the delaymgr task frees it on fire) and clear the static pointer - * so a later decision/timeout sees no active round. */ -static void ha_prevote_cancel_round_timeout(void) + * so a later decision/timeout sees no active round. + * + * bug-1915 review round 2: the caller MUST hold ha->raft_lock, and only a + * timer of a round <= upto_round is cancelled. + * - raft_lock: rounds are opened, closed and armed under raft_lock, so a + * cancel under it can never land between another thread's "open round + * R+1" and "arm R+1's timer" and cancel the NEWER round's timer (which + * left R+1 open with no timer, and gap (b) then refused every later + * round). ha_prevote_round_timeout_fn unpublishes its own context under + * raft_lock too, so a context seen published here has not been freed by + * the delaymgr task (it frees only after unpublishing) — the store to + * ->cancelled below cannot be a use-after-free. + * - upto_round: defence in depth — a decision about round R never cancels + * the timer of a later round. */ +static void ha_prevote_cancel_round_timeout(uint64_t upto_round) { prevote_round_ctx_t *old = atomic_load_explicit(&s_prevote_round_ctx, memory_order_acquire); - if (old) { + if (old && old->round <= upto_round) { atomic_store_explicit(&old->cancelled, 1, memory_order_release); atomic_store_explicit(&s_prevote_round_ctx, NULL, memory_order_release); } } +/* ha_prevote_arm_round_timeout_locked (bug-1915 review round 2): arm `round`'s + * timeout, bounded by election_timeout_ms. Caller holds ha->raft_lock and has + * just opened `round` (or found it still open). Arming under raft_lock makes + * "round is open" and "round has its timer" one atomic step with respect to + * every other thread that opens/closes rounds (all do so under raft_lock): + * nothing can close this round and open a newer one in between. + * + * Thread/lock safety: delay_task_ms only allocates and takes the delaymgr + * queue's own short internal lock (ordq_put); the delaymgr thread runs task + * callbacks WITHOUT holding that queue lock, so raft_lock -> queue lock has no + * reverse edge (raft_cb_schedule_timeout already arms the election timeout + * under raft_lock the same way). Nothing here blocks. + * + * Returns 0 when armed, or when there is no delaymgr to arm on (unit-test mode: + * c == NULL or delaymgr not running — tests drive the round by seams); a + * negative value when the timer could not be armed (allocation or scheduling + * failure), in which case nothing stays published. */ +static int ha_prevote_arm_round_timeout_locked(ha_state_t *ha, cluster_t *c, + uint64_t round) +{ + int arm_seam = 0; +#ifdef XCE_TESTING + arm_seam = (ha_prevote_round_timer_arm_hook != NULL); +#endif + /* Any still-published timer belongs to an older round (round ids only + * grow, under this lock) — retire it. */ + ha_prevote_cancel_round_timeout(round); + + if (!c || !(c->delaymgr.running || arm_seam)) + return 0; + + prevote_round_ctx_t *tctx = calloc(1, sizeof(*tctx)); + if (!tctx) + return -ENOMEM; + atomic_init(&tctx->cancelled, 0); + tctx->ha = ha; + tctx->c = c; + tctx->round = round; + atomic_store_explicit(&s_prevote_round_ctx, tctx, memory_order_release); + + extern int ha_prevote_round_timeout_fn(void *arg); + int trc; +#ifdef XCE_TESTING + if (arm_seam) + trc = ha_prevote_round_timer_arm_hook(ha, c, round, tctx); + else +#endif + trc = delay_task_ms(&c->delaymgr, (int)ha->config.election_timeout_ms, + 1, (delay_task_entry_fn_t)ha_prevote_round_timeout_fn, + tctx); + if (trc < 0) { + /* No delaymgr task holds tctx: unpublish and free it here. */ + atomic_store_explicit(&s_prevote_round_ctx, NULL, memory_order_release); + free(tctx); + return trc; + } + return 0; +} + /* * ha_enter_real_election: the WON-branch sequence — truncate uncommitted WAL * above the committed boundary, then enter the UNCHANGED real-election path @@ -8244,13 +8572,14 @@ void ha_start_prevote_async(ha_state_t *ha, cluster_t *c) { if (!ha) return; - ha->raft_c = c; - /* Snapshot self freshness for the PreVote request payload (same floor as a - * real vote). Stored in the metadata carrier ha->elect_ctx so - * raft_cb_send_pre_vote_request can read my_tag/my_lsn. */ - uint64_t cand_tag = 0, cand_lsn = 0; - int n_peers = ha->n_peers > 0 ? ha->n_peers : ha->raft.n_peers; + * real vote) into a fresh round metadata carrier, read by + * raft_cb_send_pre_vote_request for my_tag/my_lsn. Built BEFORE taking + * raft_lock and published only if the round is allowed to start (below), + * so a refused start has no side effect at all. */ + uint64_t cand_tag = 0, cand_lsn = 0; + int n_peers = ha->n_peers > 0 ? ha->n_peers : ha->raft.n_peers; + ha_elect_ctx_t *mc = NULL; if (c && c->myconf) { uint64_t local_tag = c->myconf->config_version_tag; uint64_t local_lsn = atomic_load_explicit(&ha->applied_lsn, @@ -8262,14 +8591,14 @@ void ha_start_prevote_async(ha_state_t *ha, cluster_t *c) n_peers = c->myconf->peers_cfg.candidates_count; int n_other = (n_peers > 1) ? n_peers - 1 : 0; - ha_elect_ctx_t *mc = calloc(1, sizeof(*mc) + - (size_t)n_other * sizeof(mc->votes[0])); + mc = calloc(1, sizeof(*mc) + (size_t)n_other * sizeof(mc->votes[0])); if (mc) { - mc->n_peers = n_other; - mc->result = -1; - mc->c = c; - mc->self_tag = cand_tag; - mc->self_lsn = cand_lsn; + mc->n_peers = n_other; + mc->result = -1; + mc->c = c; + mc->self_tag = cand_tag; + mc->self_lsn = cand_lsn; + mc->is_pre_vote = 1; /* never a real election's vote carrier */ pthread_mutex_init(&mc->lock, NULL); sem_init(&mc->done, 0, 0); atomic_init(&mc->refcount, 1); /* ha-elect-ctx-lifecycle task 2.4: bias reference */ @@ -8277,64 +8606,129 @@ void ha_start_prevote_async(ha_state_t *ha, cluster_t *c) mc->votes[0].my_tag = cand_tag; mc->votes[0].my_lsn = cand_lsn; } - /* ha-elect-ctx-lifecycle task 2.4: exchange (not store) so we can - * release the superseded carrier's bias reference — it is freed - * here immediately if no vote endpoint from that prior round still - * references it, or later, whenever that endpoint's on_fini fires, - * whichever comes last. Replaces the old deliberate leak (see - * design.md's Architecture §Mechanism). */ - ha_elect_ctx_t *old_ctx = (ha_elect_ctx_t *)atomic_exchange_explicit( - &ha->elect_ctx, mc, memory_order_acq_rel); - ha_elect_ctx_release(old_ctx); } - ha->n_peers = n_peers; } - INFO("HA: starting PreVote round (prospective term=%" PRIu64 ", n_peers=%d) " - "— staying CTRL_STANDBY, no persist", - ha->raft.term + 1, n_peers); + /* bug-1435: serialize against concurrent raft_state_t access (see ha.h + * raft_lock doc). + * + * bug-1915 follow-up (gap b): a PreVote round may start ONLY on a follower + * that is not already campaigning. Checked under raft_lock, atomically + * with the carrier swap and raft_begin_pre_vote below, because the + * callers do not serialize with an election: the failover stagger timer + * (ha_failover_stagger_fn, delaymgr) and the synchronous + * ha_begin_failover path can fire while + * - a real election is in flight (raft CANDIDATE / CTRL_ELECTING) — the + * carrier swap below would replace that election's ha->elect_ctx (its + * vote endpoints and voter LSNs) with this empty PreVote carrier, so + * raft_cb_become_leader would promote nobody and compute the safe + * commit point from self alone; + * - this node already leads (raft LEADER / CTRL_PRIMARY); + * - another PreVote round is still undecided — raft_begin_pre_vote + * would reset its tally and per-peer replies mid-round. + * Refusing never strands the node: in each case another timer is live — + * the in-flight round's own timeout (or its replies), the election's + * timeout (raft_on_election_timeout -> become_follower re-arms the + * liveness watchdog), or, for a leader, there is nothing to fail over + * from. */ + pthread_mutex_lock(&ha->raft_lock); + ctrl_role_t crole = (ctrl_role_t)atomic_load_explicit(&ha->role, + memory_order_acquire); + if (ha->raft.role != RAFT_FOLLOWER || ha->raft.pre_vote_in_progress + || crole == CTRL_PRIMARY || crole == CTRL_ELECTING) { + int rrole = (int)ha->raft.role; + int inprg = ha->raft.pre_vote_in_progress; + uint64_t rterm = ha->raft.term; + pthread_mutex_unlock(&ha->raft_lock); + ha_elect_ctx_release(mc); /* never published: frees it */ + INFO("HA: PreVote start refused — node is not an idle follower " + "(raft role=%d term=%" PRIu64 ", ctrl role=%d, round in " + "progress=%d); leaving the in-flight round / election and its " + "elect_ctx untouched (bug-1915)", rrole, rterm, (int)crole, inprg); + return; + } + + ha->raft_c = c; + if (mc) { + /* ha-elect-ctx-lifecycle task 2.4: exchange (not store) so we can + * release the superseded carrier's bias reference — it is freed + * here immediately if no vote endpoint from that prior round still + * references it, or later, whenever that endpoint's on_fini fires, + * whichever comes last. Replaces the old deliberate leak (see + * design.md's Architecture §Mechanism). */ + ha_elect_ctx_t *old_ctx = (ha_elect_ctx_t *)atomic_exchange_explicit( + &ha->elect_ctx, mc, memory_order_acq_rel); + ha_elect_ctx_release(old_ctx); + } + if (c && c->myconf) + ha->n_peers = n_peers; + + /* bug-1915 follow-up (gap d): a new round id; this round's PreVote + * endpoints and round timeout capture it so anything from an older round + * is dropped by pre_vote_reply_dispatch / ha_prevote_round_timeout_fn. */ + ha->pre_vote_round++; + uint64_t round = ha->pre_vote_round; + + INFO("HA: starting PreVote round %" PRIu64 " (prospective term=%" PRIu64 + ", n_peers=%d) — staying CTRL_STANDBY, no persist", + round, ha->raft.term + 1, n_peers); /* raft_begin_pre_vote: self-grant + per-peer send; mutates only the * pre_vote_* tally in raft_state_t (no term/voted_for/role change). - * bug-1435: serialize against concurrent raft_state_t access (see ha.h - * raft_lock doc) — pre_votes_granted/pre_vote_replies[] are just as - * unprotected as the real-vote tally. */ - pthread_mutex_lock(&ha->raft_lock); + * bug-1435: pre_votes_granted/pre_vote_replies[] are just as unprotected + * as the real-vote tally — still under raft_lock here. */ int rc = raft_begin_pre_vote(&ha->raft); - pthread_mutex_unlock(&ha->raft_lock); - /* Arm the round timeout (bounded by election_timeout_ms). Production only; - * the round state is in raft_state_t so tests exercise the branches via the - * reply seam without the timer. */ - ha_prevote_cancel_round_timeout(); - if (c && c->delaymgr.running) { - prevote_round_ctx_t *tctx = calloc(1, sizeof(*tctx)); - if (tctx) { - atomic_init(&tctx->cancelled, 0); - tctx->ha = ha; - tctx->c = c; - atomic_store_explicit(&s_prevote_round_ctx, tctx, - memory_order_release); - extern int ha_prevote_round_timeout_fn(void *arg); - int trc = delay_task_ms(&c->delaymgr, - (int)ha->config.election_timeout_ms, 1, - (delay_task_entry_fn_t)ha_prevote_round_timeout_fn, - tctx); - if (trc < 0) { - WARN("HA: ha_start_prevote_async: round-timeout schedule failed: %d", - trc); - atomic_store_explicit(&s_prevote_round_ctx, NULL, - memory_order_release); - free(tctx); - } - } - } - - /* Single-node: the self-grant is already a majority — win immediately. */ + /* Single-node: the self-grant is already a majority — win immediately. + * Entered under raft_lock (recursive) so the decision and the election + * start are atomic, exactly as in pre_vote_reply_dispatch's WON branch. */ if (rc == RAFT_PRE_VOTE_WON) { - ha_prevote_cancel_round_timeout(); + ha->raft.pre_vote_in_progress = 0; /* decided */ + ha_prevote_cancel_round_timeout(round); INFO("HA: PreVote won immediately (single-node) — entering real election"); ha_enter_real_election(ha, c); + pthread_mutex_unlock(&ha->raft_lock); + return; + } + +#ifdef XCE_TESTING + if (ha_prevote_pre_arm_hook != NULL) + ha_prevote_pre_arm_hook(ha, c, round); +#endif + + /* Arm the round timeout (bounded by election_timeout_ms) — bug-1915 review + * round 2: STILL under raft_lock, and only if this round is still the open + * one. Arming after the unlock (the old order) let this interleave: + * round R closes in the gap (sync LOST from a send failure, a heartbeat, a + * reply on another worker), another thread (stagger timer) opens R+1 and + * arms R+1's timer, then this thread cancelled R+1's timer and armed one + * for R (a no-op by round id) — R+1 open with NO timer, and gap (b) + * refuses every later round until all peers reply or a heartbeat arrives. + * The round can already be decided here even under the lock: the per-peer + * sends inside raft_begin_pre_vote dispatch connect failures synchronously + * (bug-1423), so all-peers-down closes the round LOST before we get here + * (and that LOST branch already re-armed the liveness watchdog). */ + int arm_failed = 0; + int arm_rc = 0; + if (ha->pre_vote_round == round && ha->raft.pre_vote_in_progress) { + arm_rc = ha_prevote_arm_round_timeout_locked(ha, c, round); + if (arm_rc < 0) { + /* Finding 6: a round with no timer must not stay open — nothing + * else is guaranteed to end it, and gap (b) would refuse every + * later round. Close it (treated as a LOSS, like a timeout) and + * re-arm the liveness watchdog below, so the next failure + * detection opens a fresh round. */ + ha->raft.pre_vote_in_progress = 0; + arm_failed = 1; + } + } + pthread_mutex_unlock(&ha->raft_lock); + + if (arm_failed) { + WARN("HA: PreVote round %" PRIu64 " closed — its round timeout could " + "not be armed (rc=%d); treating as LOSS, staying CTRL_STANDBY, " + "re-arming liveness watchdog (bug-1915)", round, arm_rc); + ha_prevote_rearm_liveness(ha, c); } } @@ -8356,7 +8750,7 @@ void ha_start_prevote_async(ha_state_t *ha, cluster_t *c) * endpoint. Also driven by the XCE_TESTING reply seam. */ static void pre_vote_reply_dispatch(ha_state_t *ha, cluster_t *c, - int peer_idx, int granted) + uint64_t round, int peer_idx, int granted) { /* bug-1435: serialize against concurrent raft_state_t access (see ha.h * raft_lock doc). raft_lock is RECURSIVE: the WON branch below calls @@ -8364,19 +8758,63 @@ static void pre_vote_reply_dispatch(ha_state_t *ha, cluster_t *c, * this same thread, which re-acquires raft_lock at its own call site — * safe by design (same-thread nesting), never blocks a legitimate chain. */ pthread_mutex_lock(&ha->raft_lock); - int outcome = raft_on_pre_vote_reply(&ha->raft, peer_idx, granted); - pthread_mutex_unlock(&ha->raft_lock); - - if (outcome == RAFT_PRE_VOTE_WON) { - ha_prevote_cancel_round_timeout(); - INFO("HA: PreVote round WON — entering real election (term will be %" - PRIu64 ")", ha->raft.term + 1); - ha_enter_real_election(ha, c); + /* bug-1915: a round is decided exactly once. raft_on_pre_vote_reply + * tallied any not-yet-counted peer even after WON/LOST cleared + * pre_vote_in_progress, and pre_votes_granted never decreases — so the + * remaining peer's reply (grant OR deny) to a WON round read as WON again + * and re-ran ha_enter_real_election: role overwritten to CTRL_ELECTING + * (even on the new leader) and ha->elect_ctx swapped for an empty carrier, + * so raft_cb_become_leader promoted no vote endpoints and computed the + * safe commit point from self alone. Checked under raft_lock, the same + * lock the deciding reply cleared the flag under, so concurrent replies on + * different pool workers cannot both decide the round. (The library now + * also refuses such a reply — raft_on_pre_vote_reply — defence in depth.) + * + * bug-1915 follow-up: and the reply must belong to the CURRENT round. + * PreVote replies and endpoint errors carry no round id on the wire, so + * the endpoint's captured ha->pre_vote_round is the identity: a reply or + * failure from an older round (its endpoint outlived the round) would + * otherwise be tallied in this one — counting a stale grant, or burning + * the peer's slot with a stale deny so its genuine reply is then dropped + * as a duplicate. */ + if (!ha->raft.pre_vote_in_progress || round != ha->pre_vote_round) { + uint64_t cur_round = ha->pre_vote_round; + int in_prog = ha->raft.pre_vote_in_progress; + pthread_mutex_unlock(&ha->raft_lock); + DBG("HA: PreVote reply from peer %d ignored — round %" PRIu64 + " is not the open round (current=%" PRIu64 ", in_progress=%d)", + peer_idx, round, cur_round, in_prog); return; } + int outcome = raft_on_pre_vote_reply(&ha->raft, peer_idx, granted); + + if (outcome == RAFT_PRE_VOTE_WON) { +#ifdef XCE_TESTING + atomic_fetch_add_explicit(&ha_prevote_won_count_for_test, 1, + memory_order_relaxed); +#endif + ha_prevote_cancel_round_timeout(round); /* under raft_lock */ + INFO("HA: PreVote round %" PRIu64 " WON — entering real election " + "(term will be %" PRIu64 ")", round, ha->raft.term + 1); + /* bug-1915 follow-up: enter the real election while STILL holding + * raft_lock (recursive; ha_start_election_async re-takes it), so the + * WON decision and the election start are one atomic step: no other + * thread can open a new PreVote round (stagger timer) or start another + * election in between and have its elect_ctx swapped out from under + * it. Nothing here blocks beyond what raft_start_election already did + * under this lock (the term persist). */ + ha_enter_real_election(ha, c); + pthread_mutex_unlock(&ha->raft_lock); + return; + } + /* bug-1915 review round 2: retire the decided round's timer while still + * under raft_lock — after the unlock another thread may already have + * opened and armed a newer round, whose timer this must not cancel. */ + if (outcome == RAFT_PRE_VOTE_LOST) + ha_prevote_cancel_round_timeout(round); + pthread_mutex_unlock(&ha->raft_lock); if (outcome == RAFT_PRE_VOTE_LOST) { - ha_prevote_cancel_round_timeout(); /* Req 7.3: log campaign abandonment distinctly from a genuine failover * so operators can tell a suppressed disruptive election apart. */ INFO("HA: PreVote round LOST — abandoning campaign, staying CTRL_STANDBY " @@ -8412,13 +8850,34 @@ int ha_prevote_round_timeout_fn(void *arg) * both crashing nodes wedged with an identical ha.c:6712 UAF). CAS so a * newer round's context (if one has already replaced this one by the * time this delaymgr task runs) is never incorrectly cleared. */ - { - prevote_round_ctx_t *expected = ctx; - atomic_compare_exchange_strong_explicit( - &s_prevote_round_ctx, &expected, NULL, - memory_order_acq_rel, memory_order_relaxed); - } - + /* bug-1915 review round 2: the unpublish (CAS) and the cancelled check + * run under raft_lock — every canceller (ha_prevote_cancel_round_timeout) + * now holds raft_lock, so a canceller that still sees this ctx published + * finishes its store to ->cancelled before we can get here and free it, + * and once we have unpublished it no canceller can reach it again (no + * use-after-free on ->cancelled). The lock is the same brief raft_lock + * use this delaymgr task already made below. */ + /* bug-1915 final review (finding 1): a CANCELLED timer returns without + * touching raft_lock — the delaymgr thread is trigger-only and must never + * wait behind a raft_lock holder (a pool worker can hold it across the + * term-persist fsync). Mirrors raft_election_timeout_fn's fast path. + * + * Freeing here without the lock is safe: + * - ->cancelled is written only by ha_prevote_cancel_round_timeout, + * always under raft_lock, and only to a ctx it found PUBLISHED in + * s_prevote_round_ctx; in that same lock hold its next and last step + * is to store NULL into s_prevote_round_ctx — it never touches the + * ctx again. The acquire load below pairs with its release store. + * - No other thread can still reach this ctx: every other reader of + * s_prevote_round_ctx (the cancel itself, and the arm path, which + * cancels first) runs under raft_lock, which the canceller holds + * until it has unpublished the ctx; once unpublished, nothing holds + * a pointer to it except this one-shot delaymgr task. + * - The arm-failure path (delay_task_ms < 0) frees its ctx itself and + * no delaymgr task ever runs for it. + * A ctx whose ->cancelled reads 0 here may be cancelled concurrently: it + * takes the locked path below, where the unpublish CAS and the re-check + * of ->cancelled under raft_lock decide it exactly as before. */ if (atomic_load_explicit(&ctx->cancelled, memory_order_acquire)) { #ifndef XCE_TESTING free(ctx); @@ -8429,11 +8888,45 @@ int ha_prevote_round_timeout_fn(void *arg) ha_state_t *ha = ctx->ha; cluster_t *c = ctx->c; - /* Only act if the round is still in progress (not already decided). */ - if (ha->raft.pre_vote_in_progress) { + pthread_mutex_lock(&ha->raft_lock); + { + prevote_round_ctx_t *expected = ctx; + atomic_compare_exchange_strong_explicit( + &s_prevote_round_ctx, &expected, NULL, + memory_order_acq_rel, memory_order_relaxed); + } + + if (atomic_load_explicit(&ctx->cancelled, memory_order_acquire)) { + pthread_mutex_unlock(&ha->raft_lock); +#ifndef XCE_TESTING + free(ctx); +#endif + return 0; + } + + /* Only act if THIS timeout's round is still the open, undecided one. + * + * bug-1915 follow-up: check and clear under raft_lock — every other writer + * of pre_vote_in_progress (raft_begin_pre_vote, raft_on_pre_vote_reply, + * ha_on_heartbeat_received) holds it, and the unlocked read-then-clear + * here raced a concurrent reply deciding the same round. The round id + * keeps a timeout that outlived its round (cancel raced the fire) from + * ending a NEWER round. This runs on the delaymgr thread, which must + * never block: the critical section is two field reads and one store — + * no I/O, no allocation — the same brief raft_lock use this thread + * already makes in raft_election_timeout_fn and (via the stagger timer) + * ha_start_prevote_async. The liveness re-arm runs after the unlock. */ + int timed_out = 0; + if (ha->raft.pre_vote_in_progress && ha->pre_vote_round == ctx->round) { ha->raft.pre_vote_in_progress = 0; - WARN("HA: PreVote round timed out before a decision — treating as LOSS, " - "staying CTRL_STANDBY; re-arming liveness watchdog (Req 5.4)"); + timed_out = 1; + } + pthread_mutex_unlock(&ha->raft_lock); + + if (timed_out) { + WARN("HA: PreVote round %" PRIu64 " timed out before a decision — " + "treating as LOSS, staying CTRL_STANDBY; re-arming liveness " + "watchdog (Req 5.4)", ctx->round); ha_prevote_rearm_liveness(ha, c); } @@ -8451,7 +8944,63 @@ int ha_prevote_round_timeout_fn(void *arg) void pre_vote_reply_dispatch_for_test(ha_state_t *ha, cluster_t *c, int peer_idx, int granted) { - pre_vote_reply_dispatch(ha, c, peer_idx, granted); + pthread_mutex_lock(&ha->raft_lock); + uint64_t round = ha->pre_vote_round; + pthread_mutex_unlock(&ha->raft_lock); + pre_vote_reply_dispatch(ha, c, round, peer_idx, granted); +} + +/* bug-1915 follow-up: deliver a reply stamped with an explicit (possibly + * stale) PreVote round id, as a reply/endpoint error from that round would. */ +void pre_vote_reply_dispatch_round_for_test(ha_state_t *ha, cluster_t *c, + uint64_t round, int peer_idx, + int granted) +{ + pre_vote_reply_dispatch(ha, c, round, peer_idx, granted); +} + +/* bug-1915 review round 2: the published round-timeout context, if any. + * Returns 1 and fills *round_out / *cancelled_out when one is published. */ +int ha_prevote_active_round_timer_for_test(uint64_t *round_out, + int *cancelled_out) +{ + prevote_round_ctx_t *t = atomic_load_explicit(&s_prevote_round_ctx, + memory_order_acquire); + if (!t) + return 0; + if (round_out) + *round_out = t->round; + if (cancelled_out) + *cancelled_out = atomic_load_explicit(&t->cancelled, + memory_order_acquire); + return 1; +} + +/* bug-1915 review round 2: unpublish the round-timeout context (test cleanup + * before freeing contexts handed to ha_prevote_round_timer_arm_hook). */ +void ha_prevote_round_timer_clear_for_test(void) +{ + atomic_store_explicit(&s_prevote_round_ctx, NULL, memory_order_release); +} + +/* bug-1915 follow-up: current PreVote round id (read under raft_lock). */ +uint64_t ha_pre_vote_round_for_test(ha_state_t *ha) +{ + pthread_mutex_lock(&ha->raft_lock); + uint64_t round = ha->pre_vote_round; + pthread_mutex_unlock(&ha->raft_lock); + return round; +} + +/* bug-1915 follow-up: mark a test-built vote_ep_ctx_t (vote_ep_ctx_build_for_test) + * as a PreVote endpoint of `round`, exactly as raft_cb_send_pre_vote_request + * stamps a production one. */ +void vote_ep_ctx_mark_pre_vote_for_test(endpoint_t *ep, uint64_t round) +{ + if (!ep || !ep->data) return; + vote_ep_ctx_t *vctx = (vote_ep_ctx_t *)ep->data; + vctx->is_pre_vote = 1; + vctx->pre_vote_round = round; } /* @@ -8459,11 +9008,21 @@ void pre_vote_reply_dispatch_for_test(ha_state_t *ha, cluster_t *c, * before a decision, driving the LOST branch (Req 5.4). */ void ha_prevote_round_timeout_fire_for_test(ha_state_t *ha, cluster_t *c) +{ + ha_prevote_round_timeout_fire_round_for_test(ha, c, + ha_pre_vote_round_for_test(ha)); +} + +/* bug-1915 follow-up: fire a round timeout that belongs to an explicit + * (possibly stale) round. */ +void ha_prevote_round_timeout_fire_round_for_test(ha_state_t *ha, cluster_t *c, + uint64_t round) { prevote_round_ctx_t ctx; memset(&ctx, 0, sizeof(ctx)); - ctx.ha = ha; - ctx.c = c; + ctx.ha = ha; + ctx.c = c; + ctx.round = round; atomic_init(&ctx.cancelled, 0); ha_prevote_round_timeout_fn(&ctx); } @@ -8498,27 +9057,23 @@ void ha_start_election_async(ha_state_t *ha, cluster_t *c, /* Step 0: raft is initialized once at startup by ha_init_raft(); nothing * to do here. ha->raft_c and ha->n_peers are already set. */ - /* Step 1: transition to CTRL_ELECTING before firing RPCs. - * raft_start_election fires on_become_leader / on_send_vote_request below; - * the role must already be ELECTING so ha_dispatch_gate() admits vote RPCs. */ - atomic_store_explicit(&ha->role, (int32_t)CTRL_ELECTING, memory_order_release); - INFO("HA FSM: → CTRL_ELECTING (async, n_peers=%d)", n_peers); - - /* Step 2: pre-allocate ha_elect_ctx_t so raft_cb_send_vote_request can + /* Step 1: pre-allocate ha_elect_ctx_t so raft_cb_send_vote_request can * read my_tag/my_lsn from it when building the capnp VoteRequest payload. * Vote counting is now owned by the raft library; this ctx is only a - * metadata carrier for tag/lsn. For multi-node elections it is kept alive - * until all per-peer vote_ep_ctx_t endpoints finish (iomgr lifetime). - * For single-node, raft_start_election fires on_become_leader immediately - * so no ctx reads occur after return. + * metadata carrier for tag/lsn plus the per-voter record (votes[]) that + * raft_cb_become_leader promotes endpoints from and computes the safe + * commit point from. For multi-node elections it is kept alive until all + * per-peer vote_ep_ctx_t endpoints finish (iomgr lifetime). For + * single-node, raft_start_election fires on_become_leader immediately so + * no ctx reads occur after return. + * + * Built BEFORE taking raft_lock and published only if this node may start + * an election (below), so a refused call has no side effect at all. * * n_peers = total cluster size (including self), matching raft convention. * standby_converged[] is indexed by votes_idx (0 .. n_peers-2), so * convergence loops iterate i < n_peers - 1 (skip self). */ int n_other = (n_peers > 1) ? n_peers - 1 : 0; - - /* Store total cluster size (including self) — matches raft_config_t.n_peers */ - ha->n_peers = n_peers; ha_elect_ctx_t *ctx = calloc(1, sizeof(*ctx) + (size_t)n_other * sizeof(ctx->votes[0])); if (!ctx) { @@ -8549,13 +9104,68 @@ void ha_start_election_async(ha_state_t *ha, cluster_t *c, ctx->votes[0].my_lsn = my_lsn; } + /* bug-1915 follow-up (gap a): the whole start — role transition, carrier + * swap and raft_start_election — is ONE step under raft_lock, and it is + * refused with NO side effect when this node is already a candidate or + * the leader. The old order set CTRL_ELECTING and swapped ha->elect_ctx + * first and ignored raft_start_election's -EALREADY, so a duplicate start + * (a PreVote round decided twice) demoted a just-elected leader to + * CTRL_ELECTING and replaced the elect_ctx its election had filled with + * an empty carrier — raft_cb_become_leader then promoted no vote + * endpoints and computed the safe commit point from self alone. + * raft_lock is recursive: the PreVote WON branch already holds it. */ + pthread_mutex_lock(&ha->raft_lock); + { + ctrl_role_t crole = (ctrl_role_t)atomic_load_explicit( + &ha->role, memory_order_acquire); + if (ha->raft.role == RAFT_CANDIDATE || ha->raft.role == RAFT_LEADER + || crole == CTRL_PRIMARY) { + int rrole = (int)ha->raft.role; + uint64_t rterm = ha->raft.term; + pthread_mutex_unlock(&ha->raft_lock); + ha_elect_ctx_release(ctx); /* never published: frees it */ + WARN("ha_start_election_async: refused — this node is already " + "%s (raft role=%d term=%" PRIu64 ", ctrl role=%d); role and " + "elect_ctx left unchanged (bug-1915)", + rrole == RAFT_LEADER || crole == CTRL_PRIMARY ? "leader" + : "a candidate", + rrole, rterm, (int)crole); + return; + } + } + + /* Step 2: transition to CTRL_ELECTING before firing RPCs. + * raft_start_election fires on_become_leader / on_send_vote_request below; + * the role must already be ELECTING so ha_dispatch_gate() admits vote RPCs. + * Remember the prior role for the (defensive) -EALREADY restore below. */ + int32_t prior_role = atomic_exchange_explicit(&ha->role, + (int32_t)CTRL_ELECTING, + memory_order_acq_rel); + INFO("HA FSM: → CTRL_ELECTING (async, n_peers=%d)", n_peers); + + /* Store total cluster size (including self) — matches raft_config_t.n_peers */ + ha->n_peers = n_peers; + + /* A real election supersedes any PreVote round still open (e.g. the + * startup election, or a round whose WON decision brought us here): it + * can no longer decide anything, and its late replies / timeout must find + * it closed. */ + if (ha->raft.pre_vote_in_progress) { + ha->raft.pre_vote_in_progress = 0; + ha_prevote_cancel_round_timeout(ha->pre_vote_round); /* raft_lock held */ + } + + /* Stamp the carrier with the term raft_start_election is about to take + * (it increments by exactly one, under this lock) so raft_cb_become_leader + * can verify it is promoting with THIS election's votes. */ + ctx->election_term = ha->raft.term + 1; + /* Store ctx in ha so raft_cb_send_vote_request can find it via ha->elect_ctx. - * ha-elect-ctx-lifecycle task 2.4: exchange (not store) to retrieve and - * release the superseded carrier's bias reference — see the identical - * rationale at ha_start_prevote_async's publish site. */ + * ha-elect-ctx-lifecycle task 2.4: exchange (not store) to retrieve the + * superseded carrier — its bias reference is released once the election + * has actually started (kept until then so -EALREADY can restore it). */ ha_elect_ctx_t *prior_ctx = (ha_elect_ctx_t *)atomic_exchange_explicit( &ha->elect_ctx, ctx, memory_order_acq_rel); - ha_elect_ctx_release(prior_ctx); /* Step 4: delegate election to raft library. * @@ -8601,17 +9211,29 @@ void ha_start_election_async(ha_state_t *ha, cluster_t *c, * helper for consistency. */ atomic_store_explicit(&ha->elect_ctx, NULL, memory_order_release); ha_elect_ctx_release(ctx); + ha_elect_ctx_release(prior_ctx); + pthread_mutex_unlock(&ha->raft_lock); return; } /* bug-1435: serialize against concurrent raft_state_t access (see ha.h - * raft_lock doc). This call can fire on_become_leader synchronously - * (single-node case) or on_send_vote_request per peer + on_schedule_ - * timeout (multi-node) — none of which re-enter a raft_* call, so the - * lock is held only across this one call. */ - pthread_mutex_lock(&ha->raft_lock); - raft_start_election(&ha->raft); - pthread_mutex_unlock(&ha->raft_lock); - + * raft_lock doc) — held since the role check above. This call can fire + * on_become_leader synchronously (single-node case) or + * on_send_vote_request per peer + on_schedule_timeout (multi-node) — none + * of which re-enter a raft_* call. */ + int erc = raft_start_election(&ha->raft); + if (erc == -EALREADY) { + /* Unreachable: the role check above ran under this same lock. Kept + * so a future caller that bypasses it still cannot leave the node + * with a swapped carrier: restore exactly what was there. */ + atomic_store_explicit(&ha->elect_ctx, prior_ctx, memory_order_release); + atomic_store_explicit(&ha->role, prior_role, memory_order_release); + pthread_mutex_unlock(&ha->raft_lock); + ha_elect_ctx_release(ctx); + ERROR("ha_start_election_async: raft_start_election returned -EALREADY " + "after the candidate/leader check — restored role and elect_ctx " + "(bug-1915 invariant violated; please report)"); + return; + } /* For single-node, raft_start_election fires on_become_leader synchronously. * on_become_leader (raft_cb_become_leader) handles role -> CTRL_PRIMARY and * phase2. Release the ctx here since no raft callbacks @@ -8619,9 +9241,13 @@ void ha_start_election_async(ha_state_t *ha, cluster_t *c, * (no vote endpoint is ever created — votes[] is empty). */ if (n_peers == 1) { atomic_store_explicit(&ha->elect_ctx, NULL, memory_order_release); + pthread_mutex_unlock(&ha->raft_lock); + ha_elect_ctx_release(prior_ctx); ha_elect_ctx_release(ctx); return; } + pthread_mutex_unlock(&ha->raft_lock); + ha_elect_ctx_release(prior_ctx); INFO("ha_start_election_async: returning (n_peers=%d, raft election started)", n_peers); @@ -8699,30 +9325,138 @@ typedef struct { cluster_t *c; } failover_stagger_ctx_t; +#ifdef XCE_TESTING +/* bug-1915 final review (finding 4): completed PreVote-start vtasks + * (observation counter; incremented as the vtask body's last step). */ +_Atomic int ha_prevote_start_vtask_done_for_test = 0; +#endif + +/* ha_stagger_primary_alive: the stagger's skip condition — a primary connected + * during the stagger (we are a STANDBY with a fresh heartbeat), so the cluster + * already has a leader. */ +static int ha_stagger_primary_alive(ha_state_t *ha) +{ + ctrl_role_t role = (ctrl_role_t)atomic_load_explicit(&ha->role, + memory_order_acquire); + return role == CTRL_STANDBY && ha_heartbeat_is_fresh(ha); +} + +/* prevote_start_vtask_entry / ha_dispatch_prevote_start (bug-1915 final + * review, finding 4): run the stagger's ha_start_prevote_async on a + * VTASK_LIVE_BLOCK vtask instead of the delaymgr thread. It opens the round + * and, under raft_lock, sends to every peer through endpoint_connect -> + * socket_connect, a bounded poll() connect per peer — bounded blocking that + * ha-async-io.md keeps off delaymgr (the heartbeat / election-timeout + * driver). Mirrors ha_dispatch_rdhup_failover exactly, including its inline + * fallback when the pool is unavailable (fail loud, stay alive). + * + * No new race: the hand-off adds only a delay before ha_start_prevote_async, + * which decides under raft_lock — atomically with opening the round — whether + * a round may start at all (refused while a round or election is in flight, + * or on a leader; bug-1915 gap b). Two starts (this vtask and, say, the + * synchronous ha_begin_failover path or a second stagger) serialize on + * raft_lock and the second is refused with no side effect. */ +typedef struct { + ha_state_t *ha; + cluster_t *c; +} prevote_start_dispatch_t; + +static int prevote_start_vtask_entry(vtask_t *task) +{ + prevote_start_dispatch_t *d = + (prevote_start_dispatch_t *)task->data->u.generic; + /* Re-check the skip: a primary heartbeat may have arrived while queued. */ + if (ha_stagger_primary_alive(d->ha)) + INFO("HA: PreVote start skipped — a primary became alive while the " + "start was queued (bug-606)"); + else + ha_start_prevote_async(d->ha, d->c); + free(d); +#ifdef XCE_TESTING + atomic_fetch_add_explicit(&ha_prevote_start_vtask_done_for_test, 1, + memory_order_release); +#endif + return 0; +} + +static void ha_dispatch_prevote_start(ha_state_t *ha, cluster_t *c) +{ + vtaskmgr_t *pool = (vtaskmgr_t *)ha->pool; + vtask_data_t *data = calloc(1, sizeof(*data)); + vtask_t *task = vtask_alloc(); + prevote_start_dispatch_t *d = calloc(1, sizeof(*d)); + + if (pool == NULL || data == NULL || task == NULL || d == NULL) { + WARN("ha_dispatch_prevote_start: vtask unavailable (pool=%p data=%p " + "task=%p d=%p) — starting PreVote inline (stay alive)", + (void *)pool, (void *)data, (void *)task, (void *)d); + free(data); + free(task); + free(d); + ha_start_prevote_async(ha, c); + return; + } + + d->ha = ha; + d->c = c; + data->vtype = VTASK_GENERIC; + data->u.generic = d; + vtask_init(task, (vtask_func_t)prevote_start_vtask_entry, data); + + if (vtask_enq_classified(pool, task, VTASK_LIVE_BLOCK) < 0) { + WARN("ha_dispatch_prevote_start: vtask enqueue failed — starting " + "PreVote inline (stay alive)"); + free(data); + free(task); + free(d); + ha_start_prevote_async(ha, c); + } +} + /* ha_failover_stagger_fn: delaymgr body that starts the PreVote round after the * per-node stagger (bug-606). Guard: if a primary connected during the stagger * (we are a STANDBY with a fresh heartbeat), the cluster already has a leader — - * skip the PreVote entirely instead of needlessly probing a live primary. */ + * skip the PreVote entirely instead of needlessly probing a live primary. + * Trigger-only (bug-1915 final review, finding 4): the start itself runs on a + * vtask (ha_dispatch_prevote_start). */ static int ha_failover_stagger_fn(void *arg) { failover_stagger_ctx_t *ctx = (failover_stagger_ctx_t *)arg; ha_state_t *ha = ctx->ha; cluster_t *c = ctx->c; - ctrl_role_t role = (ctrl_role_t)atomic_load_explicit(&ha->role, - memory_order_acquire); - if (role == CTRL_STANDBY && ha_heartbeat_is_fresh(ha)) { + if (ha_stagger_primary_alive(ha)) { INFO("HA: failover stagger elapsed but a primary is alive — skipping " "PreVote (bug-606)"); free(ctx); return 0; } - ha_start_prevote_async(ha, c); + /* bug-1915 follow-up: this timer can fire while a PreVote round or a real + * election is already in flight (another failover trigger got there + * first), or after this node became leader. ha_start_prevote_async + * refuses all of those under raft_lock — atomically with its carrier swap + * — so the in-flight election's elect_ctx is never replaced from here. */ + ha_dispatch_prevote_start(ha, c); free(ctx); return 0; } +#ifdef XCE_TESTING + +/* bug-1915 final review (finding 4): run the failover-stagger delaymgr body + * once, exactly as the delaymgr thread does when the stagger elapses. */ +void ha_failover_stagger_fire_for_test(ha_state_t *ha, cluster_t *c) +{ + failover_stagger_ctx_t *sctx = calloc(1, sizeof(*sctx)); + if (!sctx) + return; + sctx->ha = ha; + sctx->c = c; + ha_failover_stagger_fn(sctx); +} +#endif + /* CLOCK_MONOTONIC milliseconds — for the RDHUP failover debounce (5.10b). */ static uint64_t ha_mono_ms(void) { @@ -9157,9 +9891,19 @@ void *liveness_ctx_get_for_test(void) } #endif +#ifdef XCE_TESTING +/* bug-1915 review round 2: number of liveness-watchdog arms (test observation + * only — proves a recovery path re-armed the watchdog without a delaymgr). */ +_Atomic int ha_liveness_arm_count_for_test = 0; +#endif + void ha_arm_heartbeat_timeout_vtask(ha_state_t *ha, cluster_t *c) { if (!ha) return; +#ifdef XCE_TESTING + atomic_fetch_add_explicit(&ha_liveness_arm_count_for_test, 1, + memory_order_relaxed); +#endif /* Disarm any existing poll ctx by setting cancelled=1. * Do NOT free it here — the delaymgr task may already be running on @@ -9271,10 +10015,23 @@ void ha_on_heartbeat_received(ha_state_t *ha) * voted_for / role change happens here: the node never left CTRL_STANDBY * during the round, so re-attaching is a no-op on role/term — exactly the * non-disruptive rejoin the spec requires. */ + /* bug-1915 follow-up: clear under raft_lock (every other writer of the + * round fields holds it; this unlocked write raced a reply deciding the + * round on another pool worker). Brief: two stores, no I/O. Callers are + * request-dispatch vtasks (cluster.c / config_repl.c heartbeat, ctrlhello, + * WAL catch-up handlers), never iomgr/delaymgr. */ + int cancelled_round = 0; + pthread_mutex_lock(&ha->raft_lock); if (ha->raft.pre_vote_in_progress) { ha->raft.pre_vote_in_progress = 0; ha->raft.pre_votes_granted = 0; - ha_prevote_cancel_round_timeout(); + cancelled_round = 1; + /* bug-1915 review round 2: retire the round's timer under the same + * lock hold — after the unlock a newer round may be open and armed. */ + ha_prevote_cancel_round_timeout(ha->pre_vote_round); + } + pthread_mutex_unlock(&ha->raft_lock); + if (cancelled_round) { INFO("HA: primary heartbeat arrived mid-PreVote — cancelling the round " "and re-attaching as CTRL_STANDBY (leader alive, no term change) " "(Req 4.2)"); @@ -9464,21 +10221,42 @@ void ha_disarm_heartbeat_timeout_vtask(ha_state_t *ha) * standby_ep_ctx_t: per-peer context for outbound standby connections opened * by the primary during the heartbeat task. * - * Layout deliberately matches vote_ep_ctx_t for the first 4 fields so that + * Layout deliberately matches vote_ep_ctx_t for the first 5 fields so that * peer_ctrl_msg_handler (which casts ep->data to vote_ep_ctx_t*) can safely - * read ha, c, and peer_idx. ctx is always NULL here (no active election). + * read ha, c, peer_idx and is_pre_vote. ctx is always NULL here (no active + * election) and is_pre_vote always 0 (calloc). */ typedef struct { ha_state_t *ha; cluster_t *c; ha_elect_ctx_t *ctx; /* always NULL — not a vote endpoint */ int peer_idx; /* index into c->myconf->peers_cfg.candidates[] */ + int is_pre_vote; /* always 0 — layout prefix of vote_ep_ctx_t */ /* Task 1.1 (ha-heartbeat-liveness): last time a push/nop was sent to this * standby (CLOCK_MONOTONIC seconds). Initialized to 0 at allocation; * written by on_standby_connected and primary_heartbeat_fn (task 2.2). */ time_t last_push_time; } standby_ep_ctx_t; +/* bug-1915 review round 2: peer_ctrl_msg_handler (and the err handlers) cast + * every peer-control endpoint's ep->data to vote_ep_ctx_t* and read the + * prefix below, including is_pre_vote, to route the traffic. The prefix of + * standby_ep_ctx_t must therefore match vote_ep_ctx_t member-for-member — + * same offset AND same type — or a standby endpoint would be misrouted. */ +#define HA_EP_PREFIX_MATCH(m) \ + _Static_assert(offsetof(vote_ep_ctx_t, m) == offsetof(standby_ep_ctx_t, m) \ + && __builtin_types_compatible_p( \ + __typeof__(((vote_ep_ctx_t *)0)->m), \ + __typeof__(((standby_ep_ctx_t *)0)->m)), \ + "standby_ep_ctx_t must share vote_ep_ctx_t's prefix " \ + "member " #m) +HA_EP_PREFIX_MATCH(ha); +HA_EP_PREFIX_MATCH(c); +HA_EP_PREFIX_MATCH(ctx); +HA_EP_PREFIX_MATCH(peer_idx); +HA_EP_PREFIX_MATCH(is_pre_vote); +#undef HA_EP_PREFIX_MATCH + #ifdef XCE_TESTING /* * standby_ep_ctx_build_for_test: allocate a standby_ep_ctx_t, wire ep->data, diff --git a/src/controller/ha.h b/src/controller/ha.h index 0ce71ca7..0bc43032 100644 --- a/src/controller/ha.h +++ b/src/controller/ha.h @@ -125,6 +125,14 @@ typedef struct { * for the full rationale (a pure per-endpoint count would allow a * spurious free before the round is superseded). */ _Atomic int refcount; + /* bug-1915 follow-up: which round this carrier belongs to, checked by + * raft_cb_become_leader before it trusts votes[]. election_term is the + * term ha_start_election_async stamped (raft.term + 1, under raft_lock, + * immediately before raft_start_election); 0 = not stamped (synthetic + * test carriers). is_pre_vote = 1 for a PreVote round carrier + * (ha_start_prevote_async), which never holds real votes. */ + uint64_t election_term; + int is_pre_vote; ha_vote_entry_t votes[]; /* C99 flexible array: n_peers-1 entries */ } ha_elect_ctx_t; @@ -245,6 +253,13 @@ typedef struct { * -> real election, within one call chain) — that nesting must not * self-deadlock, while a genuinely different thread still fully blocks. */ pthread_mutex_t raft_lock; + /* bug-1915 follow-up: PreVote round id. Incremented under raft_lock by + * ha_start_prevote_async for every round it starts and captured by that + * round's PreVote endpoints and round timeout, so a reply, endpoint error + * or timeout from an older round is dropped instead of being tallied in + * the current one (PreVote replies carry no round id on the wire). + * Read and written only under raft_lock. */ + uint64_t pre_vote_round; /* ── New fields added by raft-library spec (Task 6.2) ──────── */ cluster_t *raft_c; /* cluster_t* set by ha_start_election_async; used by raft callbacks */ void *raft_election_timeout_ctx; /* election_timeout_ctx_t* for cancel (task 6.2) */ diff --git a/src/controller/testsuite/test_ha_prevote.c b/src/controller/testsuite/test_ha_prevote.c index b0caca5d..f90e6e41 100644 --- a/src/controller/testsuite/test_ha_prevote.c +++ b/src/controller/testsuite/test_ha_prevote.c @@ -22,18 +22,22 @@ * TDD: written first (RED) to specify the exact behaviour before implementation. */ #define _GNU_SOURCE +#include +#include #include #include #include #include #include #include +#include #include #include "ha.h" #include "netio.h" #include "raft/raft.h" +#include "vtaskmgr.h" #include "xce/cluster_def.h" /* ── XCE_TESTING seams declared in ha.c ──────────────────────────────────── */ @@ -96,6 +100,26 @@ extern endpoint_t *vote_ep_ctx_build_for_test(endpoint_t *ep, uint64_t my_lsn, const char *my_name); +/* ── bug-1915 follow-up seams (ha.c, XCE_TESTING) ───────────────────────── */ + +/* Current PreVote round id (ha->pre_vote_round, read under raft_lock). */ +extern uint64_t ha_pre_vote_round_for_test(ha_state_t *ha); +/* Deliver a PreVote reply stamped with an explicit (possibly stale) round. */ +extern void pre_vote_reply_dispatch_round_for_test(ha_state_t *ha, cluster_t *c, + uint64_t round, int peer_idx, + int granted); +/* Fire a round timeout that belongs to an explicit (possibly stale) round. */ +extern void ha_prevote_round_timeout_fire_round_for_test(ha_state_t *ha, + cluster_t *c, + uint64_t round); +/* Stamp a test-built endpoint as a PreVote endpoint of `round`. */ +extern void vote_ep_ctx_mark_pre_vote_for_test(endpoint_t *ep, uint64_t round); +extern int pre_vote_ep_err_handler_for_test(endpoint_t *ep); +extern int peer_ctrl_msg_handler_for_test(endpoint_t *ep, io_packet_t *pkt); +extern void raft_cb_become_leader_for_test(ha_state_t *ha, uint64_t term); +/* Number of PreVote rounds decided WON (observation counter). */ +extern _Atomic int ha_prevote_won_count_for_test; + /* ── Test config ─────────────────────────────────────────────────────────── */ static ha_config_t g_cfg = { @@ -425,6 +449,9 @@ static void test_prevote_reply_over_real_transport_reaches_tally(void) memset(&ep, 0, sizeof(ep)); vote_ep_ctx_build_for_test(&ep, &ha, NULL, /*peer_idx=*/1, /*my_term=*/1, /*my_tag=*/0, /*my_lsn=*/0, "test"); + /* bug-1915 follow-up: a production PreVote endpoint is stamped with its + * kind and round (raft_cb_send_pre_vote_request); do the same here. */ + vote_ep_ctx_mark_pre_vote_for_test(&ep, ha_pre_vote_round_for_test(&ha)); /* Encode a GRANT preVoteReply (same wire payload as a real-vote reply). */ io_packet_t *pkt = vote_reply_build_pkt_for_test(/*term=*/1, /*granted=*/1, @@ -911,6 +938,1198 @@ static void test_lost_round_rearms_then_later_failure_elects(void) printf("test_lost_round_rearms_then_later_failure_elects: PASS (role=%d)\n", role); } +/* ── bug-1915: a reply that arrives after the round is decided is ignored ──── */ + +/* + * 3-node: peer 1's GRANT wins the round (2/3). Peer 2's GRANT is still in + * flight and is delivered afterwards — by then the real election may already + * have made this node leader. That late reply must not decide the round a + * second time: before the fix raft_on_pre_vote_reply counted it (3/3 >= quorum) + * and returned WON again, so the controller re-ran ha_enter_real_election → + * ha_start_election_async, which overwrote the role with CTRL_ELECTING and + * swapped ha->elect_ctx for a fresh empty carrier (losing the round's vote + * endpoints and voter LSNs that raft_cb_become_leader promotes / computes the + * safe commit point from). + */ +static void test_bug1915_late_grant_after_won_is_ignored(void) +{ + ha_state_t ha; + TEST_ASSERT_EQUAL_INT(0, ha_init(&ha, &g_cfg, 0)); + ha_do_demote(&ha); + make_three_node(&ha); + + ha_start_prevote_async(&ha, NULL); + pre_vote_reply_dispatch_for_test(&ha, NULL, 1, 1 /*granted → WON*/); + TEST_ASSERT_EQUAL_INT_MESSAGE(CTRL_ELECTING, + (int)atomic_load_explicit(&ha.role, memory_order_acquire), + "precondition: the first majority grant enters the real election"); + + /* The real election completes before peer 2's PreVote reply is read. */ + atomic_store_explicit(&ha.role, (int32_t)CTRL_PRIMARY, memory_order_release); + + pre_vote_reply_dispatch_for_test(&ha, NULL, 2, 1 /*late grant*/); + + TEST_ASSERT_EQUAL_INT_MESSAGE(CTRL_PRIMARY, + (int)atomic_load_explicit(&ha.role, memory_order_acquire), + "bug-1915: a PreVote reply for an already-WON round must not re-enter " + "the real election (it demoted the new leader to CTRL_ELECTING)"); + TEST_ASSERT_EQUAL_INT_MESSAGE(0, ha.raft.pre_vote_in_progress, + "the decided round stays decided"); + + ha_fini(&ha); +} + +/* + * 3-node: peer 1's GRANT wins the round; peer 2's DENY arrives afterwards. + * pre_votes_granted never decreases, so before the fix the tally still read + * 2 >= quorum and returned WON a second time even for a DENY — the same + * re-entry into the real election as the late-grant case above. + */ +static void test_bug1915_late_deny_after_won_is_ignored(void) +{ + ha_state_t ha; + TEST_ASSERT_EQUAL_INT(0, ha_init(&ha, &g_cfg, 0)); + ha_do_demote(&ha); + make_three_node(&ha); + + g_rearm_calls = 0; + ha_prevote_rearm_liveness_hook = stub_rearm; + + ha_start_prevote_async(&ha, NULL); + pre_vote_reply_dispatch_for_test(&ha, NULL, 1, 1 /*granted → WON*/); + atomic_store_explicit(&ha.role, (int32_t)CTRL_PRIMARY, memory_order_release); + + pre_vote_reply_dispatch_for_test(&ha, NULL, 2, 0 /*late deny*/); + + TEST_ASSERT_EQUAL_INT_MESSAGE(CTRL_PRIMARY, + (int)atomic_load_explicit(&ha.role, memory_order_acquire), + "bug-1915: a late DENY for an already-WON round must not re-enter " + "the real election"); + TEST_ASSERT_EQUAL_INT_MESSAGE(0, g_rearm_calls, + "a late reply for a decided round must not be treated as a LOSS either"); + + ha_prevote_rearm_liveness_hook = NULL; + ha_fini(&ha); +} + + +/* ══ bug-1915 follow-up: close the remaining paths to a replaced elect_ctx ══ */ + +/* A carrier the tests install as "the ctx of the election in flight", with an + * extra reference so a (buggy) supersession cannot free it under the test. */ +static ha_elect_ctx_t *sentinel_ctx(int n_other) +{ + ha_elect_ctx_t *ctx = calloc(1, sizeof(*ctx) + + (size_t)n_other * sizeof(ctx->votes[0])); + TEST_ASSERT_NOT_NULL(ctx); + ctx->n_peers = n_other; + pthread_mutex_init(&ctx->lock, NULL); + sem_init(&ctx->done, 0, 0); + atomic_init(&ctx->refcount, 2); /* published + held by the test */ + return ctx; +} + +static void drop_published_ctx(ha_state_t *ha, ha_elect_ctx_t *sentinel) +{ + ha_elect_ctx_t *cur = (ha_elect_ctx_t *)atomic_exchange_explicit( + &ha->elect_ctx, NULL, memory_order_acq_rel); + if (cur && cur != sentinel) { + pthread_mutex_destroy(&cur->lock); + sem_destroy(&cur->done); + free(cur); + } + if (sentinel) { + pthread_mutex_destroy(&sentinel->lock); + sem_destroy(&sentinel->done); + free(sentinel); + } +} + +/* + * Gap (a): ha_start_election_async on a node whose raft is already LEADER must + * change nothing — not the role, not ha->elect_ctx, not the term. Before the + * fix it set CTRL_ELECTING and swapped elect_ctx BEFORE raft_start_election + * returned -EALREADY (ignored), demoting a live leader and orphaning its + * election's vote endpoints / voter LSNs. + */ +static void test_gapA_election_start_noop_when_raft_leader(void) +{ + ha_state_t ha; + TEST_ASSERT_EQUAL_INT(0, ha_init(&ha, &g_cfg, 0)); + make_three_node(&ha); + ha.raft.role = RAFT_LEADER; + ha.raft.term = 5; + atomic_store_explicit(&ha.term, 5, memory_order_release); + atomic_store_explicit(&ha.role, (int32_t)CTRL_PRIMARY, memory_order_release); + ha_elect_ctx_t *mine = sentinel_ctx(2); + atomic_store_explicit(&ha.elect_ctx, (void *)mine, memory_order_release); + + ha_start_election_async(&ha, NULL, 3, NULL, /*tag=*/1, /*lsn=*/10, "n0"); + + TEST_ASSERT_EQUAL_INT_MESSAGE(CTRL_PRIMARY, + (int)atomic_load_explicit(&ha.role, memory_order_acquire), + "gap (a): a leader must not be moved to CTRL_ELECTING"); + TEST_ASSERT_EQUAL_PTR_MESSAGE(mine, + atomic_load_explicit(&ha.elect_ctx, memory_order_acquire), + "gap (a): the leader's elect_ctx must not be swapped"); + TEST_ASSERT_EQUAL_UINT64(5, ha.raft.term); + TEST_ASSERT_EQUAL_INT(RAFT_LEADER, ha.raft.role); + TEST_ASSERT_EQUAL_INT_MESSAGE(2, atomic_load(&mine->refcount), + "gap (a): the published carrier's reference must not be released"); + + drop_published_ctx(&ha, mine); + ha_fini(&ha); +} + +/* Gap (a): same for a node already CANDIDATE — its in-flight election's ctx + * holds the vote endpoints and voter LSNs raft_cb_become_leader needs. */ +static void test_gapA_election_start_noop_when_raft_candidate(void) +{ + ha_state_t ha; + TEST_ASSERT_EQUAL_INT(0, ha_init(&ha, &g_cfg, 0)); + make_three_node(&ha); + ha.raft.role = RAFT_CANDIDATE; + ha.raft.term = 7; + atomic_store_explicit(&ha.role, (int32_t)CTRL_ELECTING, memory_order_release); + ha_elect_ctx_t *mine = sentinel_ctx(2); + atomic_store_explicit(&ha.elect_ctx, (void *)mine, memory_order_release); + + ha_start_election_async(&ha, NULL, 3, NULL, /*tag=*/1, /*lsn=*/10, "n0"); + + TEST_ASSERT_EQUAL_PTR_MESSAGE(mine, + atomic_load_explicit(&ha.elect_ctx, memory_order_acquire), + "gap (a): a candidate's elect_ctx must not be swapped"); + TEST_ASSERT_EQUAL_UINT64_MESSAGE(7, ha.raft.term, + "gap (a): no second term bump"); + TEST_ASSERT_EQUAL_INT(RAFT_CANDIDATE, ha.raft.role); + TEST_ASSERT_EQUAL_INT(2, atomic_load(&mine->refcount)); + + drop_published_ctx(&ha, mine); + ha_fini(&ha); +} + +/* + * Gap (b): a failover trigger (ha_begin_failover — synchronous in unit mode, + * the same ha_start_prevote_async the stagger timer runs) that fires while a + * real election is in flight must not open a PreVote round: in production that + * swaps ha->elect_ctx for an empty PreVote carrier mid-election. + */ +static void test_gapB_prevote_refused_during_real_election(void) +{ + ha_state_t ha; + TEST_ASSERT_EQUAL_INT(0, ha_init(&ha, &g_cfg, 0)); + make_three_node(&ha); + ha.raft.role = RAFT_CANDIDATE; + ha.raft.term = 4; + atomic_store_explicit(&ha.role, (int32_t)CTRL_ELECTING, memory_order_release); + uint64_t round0 = ha_pre_vote_round_for_test(&ha); + + ha_begin_failover(&ha, NULL); + + TEST_ASSERT_EQUAL_INT_MESSAGE(0, ha.raft.pre_vote_in_progress, + "gap (b): no PreVote round may open while a real election is in flight"); + TEST_ASSERT_EQUAL_UINT64_MESSAGE(round0, ha_pre_vote_round_for_test(&ha), + "gap (b): a refused start must not consume a round id"); + TEST_ASSERT_EQUAL_INT(CTRL_ELECTING, + (int)atomic_load_explicit(&ha.role, memory_order_acquire)); + TEST_ASSERT_EQUAL_INT(RAFT_CANDIDATE, ha.raft.role); + ha_fini(&ha); +} + +/* Gap (b): a leader never opens a PreVote round either. */ +static void test_gapB_prevote_refused_when_leader(void) +{ + ha_state_t ha; + TEST_ASSERT_EQUAL_INT(0, ha_init(&ha, &g_cfg, 0)); + make_three_node(&ha); + ha.raft.role = RAFT_LEADER; + atomic_store_explicit(&ha.role, (int32_t)CTRL_PRIMARY, memory_order_release); + + ha_start_prevote_async(&ha, NULL); + + TEST_ASSERT_EQUAL_INT_MESSAGE(0, ha.raft.pre_vote_in_progress, + "gap (b): a leader must not open a PreVote round"); + TEST_ASSERT_EQUAL_INT(CTRL_PRIMARY, + (int)atomic_load_explicit(&ha.role, memory_order_acquire)); + ha_fini(&ha); +} + +/* Gap (b): a second start while a round is still undecided must not reset the + * round (tally, per-peer replies, round id) — the stagger timer firing while + * the RDHUP-triggered round is in flight did exactly that. */ +static void test_gapB_prevote_restart_mid_round_keeps_round(void) +{ + ha_state_t ha; + TEST_ASSERT_EQUAL_INT(0, ha_init(&ha, &g_cfg, 0)); + ha_do_demote(&ha); + make_three_node(&ha); + ha.raft.n_peers = 5; /* quorum 3: one grant leaves the round pending */ + ha.n_peers = 5; + + ha_start_prevote_async(&ha, NULL); + uint64_t r1 = ha_pre_vote_round_for_test(&ha); + pre_vote_reply_dispatch_for_test(&ha, NULL, 1, 1 /*grant, pending*/); + TEST_ASSERT_EQUAL_INT(2, ha.raft.pre_votes_granted); + + ha_start_prevote_async(&ha, NULL); /* e.g. a stagger timer firing now */ + + TEST_ASSERT_EQUAL_INT_MESSAGE(2, ha.raft.pre_votes_granted, + "gap (b): an in-flight round's tally must not be reset"); + TEST_ASSERT_EQUAL_INT(1, ha.raft.pre_vote_replies[1]); + TEST_ASSERT_EQUAL_UINT64(r1, ha_pre_vote_round_for_test(&ha)); + TEST_ASSERT_EQUAL_INT(1, ha.raft.pre_vote_in_progress); + ha_fini(&ha); +} + +/* + * Gap (d): a reply from an OLDER PreVote round must not be tallied in the + * current one (replies carry no round id on the wire; the endpoint's captured + * round is the identity). + */ +static void test_gapD_stale_round_reply_dropped(void) +{ + ha_state_t ha; + TEST_ASSERT_EQUAL_INT(0, ha_init(&ha, &g_cfg, 0)); + ha_do_demote(&ha); + make_three_node(&ha); + ha.raft.n_peers = 5; + ha.n_peers = 5; + g_rearm_calls = 0; + ha_prevote_rearm_liveness_hook = stub_rearm; + + ha_start_prevote_async(&ha, NULL); + uint64_t r1 = ha_pre_vote_round_for_test(&ha); + ha_prevote_round_timeout_fire_for_test(&ha, NULL); /* r1 LOST */ + TEST_ASSERT_EQUAL_INT(0, ha.raft.pre_vote_in_progress); + + ha_start_prevote_async(&ha, NULL); /* r2 */ + TEST_ASSERT_TRUE(ha_pre_vote_round_for_test(&ha) != r1); + + pre_vote_reply_dispatch_round_for_test(&ha, NULL, r1, 1, 1 /*stale grant*/); + + TEST_ASSERT_EQUAL_INT_MESSAGE(1, ha.raft.pre_votes_granted, + "gap (d): a round-1 grant must not count toward round 2"); + TEST_ASSERT_EQUAL_INT_MESSAGE(0, ha.raft.pre_vote_replies[1], + "gap (d): a stale reply must not consume peer 1's round-2 slot"); + + ha_prevote_rearm_liveness_hook = NULL; + ha_fini(&ha); +} + +/* Gap (d): round 1's timeout task firing after round 2 started must not end + * round 2. */ +static void test_gapD_stale_round_timeout_does_not_end_current_round(void) +{ + ha_state_t ha; + TEST_ASSERT_EQUAL_INT(0, ha_init(&ha, &g_cfg, 0)); + ha_do_demote(&ha); + make_three_node(&ha); + g_rearm_calls = 0; + ha_prevote_rearm_liveness_hook = stub_rearm; + + ha_start_prevote_async(&ha, NULL); + uint64_t r1 = ha_pre_vote_round_for_test(&ha); + pre_vote_reply_dispatch_for_test(&ha, NULL, 1, 0); + pre_vote_reply_dispatch_for_test(&ha, NULL, 2, 0); /* r1 LOST */ + TEST_ASSERT_EQUAL_INT(0, ha.raft.pre_vote_in_progress); + + ha_start_prevote_async(&ha, NULL); /* r2 */ + int rearms_before = g_rearm_calls; + + ha_prevote_round_timeout_fire_round_for_test(&ha, NULL, r1); + + TEST_ASSERT_EQUAL_INT_MESSAGE(1, ha.raft.pre_vote_in_progress, + "gap (d): a stale round timeout must not end the current round"); + TEST_ASSERT_EQUAL_INT(rearms_before, g_rearm_calls); + + ha_prevote_rearm_liveness_hook = NULL; + ha_fini(&ha); +} + +/* Gap (d): a PreVote endpoint error from round 1 delivered during round 2 must + * not count as peer 1's round-2 DENY — that also burned peer 1's slot, so its + * genuine round-2 GRANT was then dropped as a duplicate. */ +static void test_gapD_stale_round_endpoint_error_not_counted(void) +{ + ha_state_t ha; + TEST_ASSERT_EQUAL_INT(0, ha_init(&ha, &g_cfg, 0)); + ha_do_demote(&ha); + make_three_node(&ha); + g_rearm_calls = 0; + ha_prevote_rearm_liveness_hook = stub_rearm; + + ha_start_prevote_async(&ha, NULL); + uint64_t r1 = ha_pre_vote_round_for_test(&ha); + pre_vote_reply_dispatch_for_test(&ha, NULL, 1, 0); + pre_vote_reply_dispatch_for_test(&ha, NULL, 2, 0); /* r1 LOST */ + + ha_start_prevote_async(&ha, NULL); /* r2 */ + + endpoint_t ep; + memset(&ep, 0, sizeof(ep)); + vote_ep_ctx_build_for_test(&ep, &ha, NULL, /*peer_idx=*/1, 1, 0, 0, "t"); + vote_ep_ctx_mark_pre_vote_for_test(&ep, r1); /* round-1 endpoint */ + TEST_ASSERT_EQUAL_INT(0, pre_vote_ep_err_handler_for_test(&ep)); + + TEST_ASSERT_EQUAL_INT_MESSAGE(0, ha.raft.pre_vote_replies[1], + "gap (d): a stale endpoint error must not be tallied in round 2"); + + pre_vote_reply_dispatch_for_test(&ha, NULL, 1, 1); /* genuine r2 grant */ + TEST_ASSERT_EQUAL_INT_MESSAGE(CTRL_ELECTING, + (int)atomic_load_explicit(&ha.role, memory_order_acquire), + "gap (d): peer 1's genuine round-2 grant must still win the round"); + + free(ep.data); + ha_prevote_rearm_liveness_hook = NULL; + ha_fini(&ha); +} + +/* + * Gap (d), cross-path: a PreVote reply that arrives on its PreVote endpoint + * after the round was WON — when the node is already CTRL_ELECTING — must + * never be decoded as a REAL vote. The two replies share one wire payload and + * the router chose the decoder by role, so a late PreVote GRANT whose term + * equals the new election term was counted by raft_on_vote_reply as a real + * vote (a PreVote grant records no vote — Election Safety). + */ +static void test_gapD_late_prevote_reply_never_counts_as_real_vote(void) +{ + ha_state_t ha; + TEST_ASSERT_EQUAL_INT(0, ha_init(&ha, &g_cfg, 0)); + make_three_node(&ha); + atomic_store_explicit(&ha.role, (int32_t)CTRL_STANDBY, memory_order_release); + uint64_t pv_round = ha_pre_vote_round_for_test(&ha); + + ha_start_election_async(&ha, NULL, 3, NULL, /*tag=*/1, /*lsn=*/10, "n0"); + TEST_ASSERT_EQUAL_INT_MESSAGE(RAFT_CANDIDATE, ha.raft.role, + "precondition: a real election is in flight"); + uint64_t T = ha.raft.term; + + endpoint_t ep; + memset(&ep, 0, sizeof(ep)); + vote_ep_ctx_build_for_test(&ep, &ha, NULL, /*abs peer_idx=*/1, T, 0, 0, "t"); + vote_ep_ctx_mark_pre_vote_for_test(&ep, pv_round); + io_packet_t *pkt = vote_reply_build_pkt_for_test(T, /*granted=*/1, 0, 0); + TEST_ASSERT_NOT_NULL(pkt); + TEST_ASSERT_EQUAL_INT(0, peer_ctrl_msg_handler_for_test(&ep, pkt)); + + TEST_ASSERT_EQUAL_INT_MESSAGE(RAFT_CANDIDATE, ha.raft.role, + "gap (d): a PreVote grant must never be counted as a real vote"); + TEST_ASSERT_EQUAL_INT(1, ha.raft.votes_granted); + TEST_ASSERT_EQUAL_INT(CTRL_ELECTING, + (int)atomic_load_explicit(&ha.role, memory_order_acquire)); + + free(ep.data); + drop_published_ctx(&ha, NULL); + ha_fini(&ha); +} + +/* Gap (d), cross-path inverse: a reply on a REAL-vote endpoint (e.g. a lost + * election's late RequestVoteReply) arriving while a PreVote round is open + * must not be tallied as a PreVote reply. */ +static void test_gapD_real_vote_reply_never_feeds_prevote_tally(void) +{ + ha_state_t ha; + TEST_ASSERT_EQUAL_INT(0, ha_init(&ha, &g_cfg, 0)); + ha_do_demote(&ha); + make_three_node(&ha); + + ha_start_prevote_async(&ha, NULL); + + endpoint_t ep; + memset(&ep, 0, sizeof(ep)); + vote_ep_ctx_build_for_test(&ep, &ha, NULL, /*votes_idx=*/1, 1, 0, 0, "t"); + io_packet_t *pkt = vote_reply_build_pkt_for_test(1, /*granted=*/1, 0, 0); + TEST_ASSERT_NOT_NULL(pkt); + TEST_ASSERT_EQUAL_INT(0, pre_vote_reply_msg_handler_for_test(&ep, pkt)); + + TEST_ASSERT_EQUAL_INT_MESSAGE(1, ha.raft.pre_vote_in_progress, + "gap (d): a real-vote endpoint's reply must not decide a PreVote round"); + TEST_ASSERT_EQUAL_INT(1, ha.raft.pre_votes_granted); + TEST_ASSERT_EQUAL_INT(CTRL_STANDBY, + (int)atomic_load_explicit(&ha.role, memory_order_acquire)); + + free(ep.data); + ha_fini(&ha); +} + +/* + * Defensive check in raft_cb_become_leader: promotion computes the safe commit + * point from ha->elect_ctx. A PreVote carrier (no real votes), a carrier + * stamped for another term, or a stamped carrier without a recorded majority + * would yield a self-only safe commit point (n_granted = 1 in a 3-node + * cluster) — refuse the promotion loudly instead of becoming CTRL_PRIMARY. + */ +static void test_become_leader_refuses_prevote_carrier(void) +{ + ha_state_t ha; + TEST_ASSERT_EQUAL_INT(0, ha_init(&ha, &g_cfg, 0)); + make_three_node(&ha); + atomic_store_explicit(&ha.role, (int32_t)CTRL_ELECTING, memory_order_release); + ha_elect_ctx_t *pv = sentinel_ctx(2); + pv->is_pre_vote = 1; + atomic_store_explicit(&ha.elect_ctx, (void *)pv, memory_order_release); + + raft_cb_become_leader_for_test(&ha, 1); + + TEST_ASSERT_TRUE_MESSAGE( + atomic_load_explicit(&ha.role, memory_order_acquire) != CTRL_PRIMARY, + "promotion must be refused when elect_ctx is a PreVote carrier"); + drop_published_ctx(&ha, pv); + ha_fini(&ha); +} + +static void test_become_leader_refuses_stamped_carrier_without_majority(void) +{ + ha_state_t ha; + TEST_ASSERT_EQUAL_INT(0, ha_init(&ha, &g_cfg, 0)); + make_three_node(&ha); + atomic_store_explicit(&ha.role, (int32_t)CTRL_ELECTING, memory_order_release); + ha_elect_ctx_t *ec = sentinel_ctx(2); + ec->election_term = 3; /* this election's carrier ... */ + /* ... but no voter recorded as granted: self-only */ + atomic_store_explicit(&ha.elect_ctx, (void *)ec, memory_order_release); + + raft_cb_become_leader_for_test(&ha, 3); + + TEST_ASSERT_TRUE_MESSAGE( + atomic_load_explicit(&ha.role, memory_order_acquire) != CTRL_PRIMARY, + "promotion must be refused when elect_ctx records no voted majority"); + drop_published_ctx(&ha, ec); + ha_fini(&ha); +} + +static void test_become_leader_refuses_carrier_of_other_term(void) +{ + ha_state_t ha; + TEST_ASSERT_EQUAL_INT(0, ha_init(&ha, &g_cfg, 0)); + make_three_node(&ha); + atomic_store_explicit(&ha.role, (int32_t)CTRL_ELECTING, memory_order_release); + ha_elect_ctx_t *ec = sentinel_ctx(2); + ec->election_term = 3; + ec->votes[0].vote_granted = 1; + ec->votes[1].vote_granted = 1; + atomic_store_explicit(&ha.elect_ctx, (void *)ec, memory_order_release); + + raft_cb_become_leader_for_test(&ha, 4); /* won term 4, ctx is term 3's */ + + TEST_ASSERT_TRUE_MESSAGE( + atomic_load_explicit(&ha.role, memory_order_acquire) != CTRL_PRIMARY, + "promotion must be refused when elect_ctx belongs to another election"); + drop_published_ctx(&ha, ec); + ha_fini(&ha); +} + +/* No false refusal: this election's stamped carrier with a recorded majority + * promotes normally. */ +static void test_become_leader_accepts_own_carrier_with_majority(void) +{ + ha_state_t ha; + TEST_ASSERT_EQUAL_INT(0, ha_init(&ha, &g_cfg, 0)); + make_three_node(&ha); + atomic_store_explicit(&ha.role, (int32_t)CTRL_ELECTING, memory_order_release); + ha_elect_ctx_t *ec = sentinel_ctx(2); + ec->election_term = 3; + ec->votes[1].vote_granted = 1; /* self + 1 = 2 of 3 */ + atomic_store_explicit(&ha.elect_ctx, (void *)ec, memory_order_release); + + raft_cb_become_leader_for_test(&ha, 3); + + TEST_ASSERT_EQUAL_INT(CTRL_PRIMARY, + (int)atomic_load_explicit(&ha.role, memory_order_acquire)); + drop_published_ctx(&ha, ec); + ha_fini(&ha); +} + +/* + * Gap (e): real threads. Two peers' GRANTs for the same 3-node round are + * dispatched concurrently from two threads released by a barrier (no sleeps). + * Whatever the interleaving, the round must be decided WON exactly once. The + * pass condition is deterministic; the interleaving is not, so the loop gives + * the race many chances to show up if the raft_lock decision check regresses. + */ +typedef struct { + ha_state_t *ha; + pthread_barrier_t *bar; + int peer; +} race_arg_t; + +static void *race_reply_fn(void *arg) +{ + race_arg_t *a = (race_arg_t *)arg; + pthread_barrier_wait(a->bar); + pre_vote_reply_dispatch_for_test(a->ha, NULL, a->peer, 1); + return NULL; +} + +static void test_gapE_concurrent_grants_decide_round_once(void) +{ + ha_state_t ha; + TEST_ASSERT_EQUAL_INT(0, ha_init(&ha, &g_cfg, 0)); + make_three_node(&ha); + + for (int iter = 0; iter < 500; iter++) { + atomic_store_explicit(&ha.role, (int32_t)CTRL_STANDBY, + memory_order_release); + ha_start_prevote_async(&ha, NULL); + TEST_ASSERT_EQUAL_INT(1, ha.raft.pre_vote_in_progress); + int won0 = atomic_load(&ha_prevote_won_count_for_test); + + pthread_barrier_t bar; + pthread_barrier_init(&bar, NULL, 2); + race_arg_t a1 = { &ha, &bar, 1 }, a2 = { &ha, &bar, 2 }; + pthread_t t1, t2; + TEST_ASSERT_EQUAL_INT(0, pthread_create(&t1, NULL, race_reply_fn, &a1)); + TEST_ASSERT_EQUAL_INT(0, pthread_create(&t2, NULL, race_reply_fn, &a2)); + pthread_join(t1, NULL); + pthread_join(t2, NULL); + pthread_barrier_destroy(&bar); + + TEST_ASSERT_EQUAL_INT_MESSAGE(1, + atomic_load(&ha_prevote_won_count_for_test) - won0, + "gap (e): two concurrent grants must decide the round WON once"); + } + ha_fini(&ha); +} + +/* ══ bug-1915 review round 2 ════════════════════════════════════════════════ */ + +extern void (*ha_prevote_pre_arm_hook)(ha_state_t *ha, cluster_t *c, + uint64_t round); +extern int (*ha_prevote_round_timer_arm_hook)(ha_state_t *ha, cluster_t *c, + uint64_t round, void *tctx); +extern int ha_prevote_active_round_timer_for_test(uint64_t *round_out, + int *cancelled_out); +extern void ha_prevote_round_timer_clear_for_test(void); +extern _Atomic int ha_liveness_arm_count_for_test; +extern void (*ha_rdhup_election_hook)(ha_state_t *ha, cluster_t *c, + int n_peers, xce_candidate_t **peers, + uint64_t my_config_tag, uint64_t my_lsn, + const char *my_name); + +/* Round-timer arm stub: records every context it accepted (the test owns and + * frees them — no delaymgr ever fires them) and returns g_arm_rc. */ +#define MAX_ARMS 16 +static void *g_arm_ctx[MAX_ARMS]; +static uint64_t g_arm_round[MAX_ARMS]; +static int g_arm_n = 0; +static int g_arm_rc = 0; + +static int stub_arm(ha_state_t *ha, cluster_t *c, uint64_t round, void *tctx) +{ + (void)ha; (void)c; + if (g_arm_rc < 0) + return g_arm_rc; /* scheduling failed: caller keeps ownership */ + TEST_ASSERT_TRUE(g_arm_n < MAX_ARMS); + g_arm_ctx[g_arm_n] = tctx; + g_arm_round[g_arm_n] = round; + g_arm_n++; + return 0; +} + +static void arm_stub_install(void) +{ + g_arm_n = 0; + g_arm_rc = 0; + ha_prevote_round_timer_arm_hook = stub_arm; +} + +static void arm_stub_remove(void) +{ + ha_prevote_round_timer_arm_hook = NULL; + ha_prevote_pre_arm_hook = NULL; + ha_prevote_round_timer_clear_for_test(); + for (int i = 0; i < g_arm_n; i++) + free(g_arm_ctx[i]); + g_arm_n = 0; +} + +/* + * Finding 1 (liveness race): ha_start_prevote_async opened round R, then — + * before it armed R's timeout — round R closed (here: its timeout path; in + * production equally a sync LOST or a heartbeat) and another thread (the + * stagger timer) opened R+1 and armed R+1's timeout. The first thread then + * cancelled R+1's timer and armed one for R, a no-op by round id: R+1 was left + * open with NO timer, and gap (b) refuses every later round, so the node was + * stuck until every peer replied or a heartbeat arrived. The interleaving is + * injected deterministically at the pre-arm seam (no sleeps, no threads). + */ +static int g_interleave_done = 0; +static void interleave_close_and_reopen(ha_state_t *ha, cluster_t *c, + uint64_t round) +{ + if (g_interleave_done) + return; + g_interleave_done = 1; + ha_prevote_round_timeout_fire_round_for_test(ha, c, round); /* R closes */ + ha_start_prevote_async(ha, c); /* R+1 opens */ +} + +static void test_r2_f1_open_round_always_has_its_timer(void) +{ + ha_state_t ha; + TEST_ASSERT_EQUAL_INT(0, ha_init(&ha, &g_cfg, 0)); + ha_do_demote(&ha); + make_three_node(&ha); + cluster_t *c = calloc(1, sizeof(*c)); /* no myconf: sends are no-ops */ + TEST_ASSERT_NOT_NULL(c); + g_rearm_calls = 0; + ha_prevote_rearm_liveness_hook = stub_rearm; + arm_stub_install(); + g_interleave_done = 0; + ha_prevote_pre_arm_hook = interleave_close_and_reopen; + + ha_start_prevote_async(&ha, c); + + uint64_t cur = ha_pre_vote_round_for_test(&ha); + TEST_ASSERT_EQUAL_INT_MESSAGE(1, g_interleave_done, + "precondition: the interleaving ran"); + TEST_ASSERT_EQUAL_INT_MESSAGE(1, ha.raft.pre_vote_in_progress, + "precondition: round R+1 is open"); + uint64_t t_round = 0; + int t_cancelled = 1; + TEST_ASSERT_EQUAL_INT_MESSAGE(1, + ha_prevote_active_round_timer_for_test(&t_round, &t_cancelled), + "finding 1: the open round must have a published round timeout"); + TEST_ASSERT_EQUAL_UINT64_MESSAGE(cur, t_round, + "finding 1: the published round timeout must belong to the OPEN round " + "(R+1), not to the already-closed round R"); + TEST_ASSERT_EQUAL_INT_MESSAGE(0, t_cancelled, + "finding 1: the open round's timeout must not be cancelled"); + + arm_stub_remove(); + ha_prevote_rearm_liveness_hook = NULL; + free(c); + ha_fini(&ha); +} + +/* + * Finding 6: if the round timeout cannot be armed (calloc / delay_task_ms + * failure), the round must not stay open with no timer — gap (b) would refuse + * every later round. The round is closed and the liveness watchdog re-armed, + * so the next failure detection opens a fresh round. + */ +static void test_r2_f6_round_timer_arm_failure_closes_round(void) +{ + ha_state_t ha; + TEST_ASSERT_EQUAL_INT(0, ha_init(&ha, &g_cfg, 0)); + ha_do_demote(&ha); + make_three_node(&ha); + cluster_t *c = calloc(1, sizeof(*c)); + TEST_ASSERT_NOT_NULL(c); + g_rearm_calls = 0; + ha_prevote_rearm_liveness_hook = stub_rearm; + arm_stub_install(); + g_arm_rc = -ENOMEM; + + ha_start_prevote_async(&ha, c); + uint64_t r1 = ha_pre_vote_round_for_test(&ha); + + TEST_ASSERT_EQUAL_INT_MESSAGE(0, ha.raft.pre_vote_in_progress, + "finding 6: a round whose timeout could not be armed must be closed"); + TEST_ASSERT_EQUAL_INT_MESSAGE(1, g_rearm_calls, + "finding 6: the liveness watchdog must be re-armed"); + TEST_ASSERT_EQUAL_INT_MESSAGE(0, + ha_prevote_active_round_timer_for_test(NULL, NULL), + "finding 6: no round timeout may stay published after a failed arm"); + + g_arm_rc = 0; + ha_start_prevote_async(&ha, c); /* the next failure detection */ + TEST_ASSERT_EQUAL_INT_MESSAGE(1, ha.raft.pre_vote_in_progress, + "finding 6: a later round must not be refused"); + TEST_ASSERT_TRUE(ha_pre_vote_round_for_test(&ha) == r1 + 1); + + arm_stub_remove(); + ha_prevote_rearm_liveness_hook = NULL; + free(c); + ha_fini(&ha); +} + +/* + * Finding 2: a refused promotion (carrier defect) must not leave the node raft + * LEADER / CTRL_ELECTING with no timer — every self-recovery path refuses a + * LEADER/ELECTING node. It steps down to FOLLOWER in the SAME term (no term + * or vote change: it keeps its self-vote, so it cannot vote for another + * candidate in this term), becomes CTRL_STANDBY and re-arms the liveness + * watchdog, so the next timeout campaigns for term+1. + */ +static void test_r2_f2_refused_promotion_steps_down_and_rearms(void) +{ + ha_state_t ha; + TEST_ASSERT_EQUAL_INT(0, ha_init(&ha, &g_cfg, 0)); + make_three_node(&ha); + /* As raft_on_vote_reply leaves it right before on_become_leader. */ + ha.raft.term = 4; + ha.raft.voted_for = ha.raft.self_idx; + ha.raft.role = RAFT_LEADER; + atomic_store_explicit(&ha.role, (int32_t)CTRL_ELECTING, memory_order_release); + ha_elect_ctx_t *pv = sentinel_ctx(2); + pv->is_pre_vote = 1; + atomic_store_explicit(&ha.elect_ctx, (void *)pv, memory_order_release); + int arms0 = atomic_load(&ha_liveness_arm_count_for_test); + + raft_cb_become_leader_for_test(&ha, 4); + + TEST_ASSERT_EQUAL_INT_MESSAGE(CTRL_STANDBY, + (int)atomic_load_explicit(&ha.role, memory_order_acquire), + "finding 2: a refused promotion must leave the node CTRL_STANDBY"); + TEST_ASSERT_EQUAL_INT_MESSAGE(RAFT_FOLLOWER, (int)ha.raft.role, + "finding 2: raft must step down to FOLLOWER"); + TEST_ASSERT_EQUAL_UINT64_MESSAGE(4, ha.raft.term, + "finding 2: the step-down keeps the term"); + TEST_ASSERT_EQUAL_INT_MESSAGE(ha.raft.self_idx, ha.raft.voted_for, + "finding 2: the step-down keeps this term's (self) vote"); + TEST_ASSERT_EQUAL_UINT64(4, atomic_load_explicit(&ha.term, + memory_order_acquire)); + TEST_ASSERT_TRUE_MESSAGE( + atomic_load(&ha_liveness_arm_count_for_test) > arms0, + "finding 2: the liveness watchdog must be re-armed"); + + /* ... and the node can campaign again (gap (b) admits a new round). */ + ha_start_prevote_async(&ha, NULL); + TEST_ASSERT_EQUAL_INT_MESSAGE(1, ha.raft.pre_vote_in_progress, + "finding 2: the stepped-down node must be able to open a PreVote round"); + + drop_published_ctx(&ha, pv); + ha_fini(&ha); +} + +/* + * Finding 3: the library's delayed-election trigger (raft_on_leader_lost -> + * on_start_election_after_delay -> ha_dispatch_election_start) called + * raft_start_election directly — no ha->elect_ctx carrier stamped for the new + * term, no CTRL_ELECTING transition — so every win from it would be refused + * by raft_cb_become_leader. It must go through the stamped start path + * (ha_enter_real_election -> ha_start_election_async), observed here at its + * ha_rdhup_election_hook seam. + */ +static int g_r2_election_spy_calls = 0; +static void r2_election_spy(ha_state_t *ha, cluster_t *c, int n_peers, + xce_candidate_t **peers, uint64_t tag, + uint64_t lsn, const char *name) +{ + (void)ha; (void)c; (void)n_peers; (void)peers; (void)tag; (void)lsn; + (void)name; + g_r2_election_spy_calls++; +} + +static void test_r2_f3_delayed_election_uses_stamped_start_path(void) +{ + myconf_t myconf; + memset(&myconf, 0, sizeof(myconf)); + myconf.name = "ctrl-a"; + myconf.config_version_tag = 0x1234; + myconf.peers_cfg.candidates_count = 3; + const char *names[3] = { "ctrl-a", "ctrl-b", "ctrl-c" }; + for (int i = 0; i < 3; i++) { + snprintf(myconf.peers_cfg.candidates[i].name, + sizeof(myconf.peers_cfg.candidates[i].name), "%s", names[i]); + myconf.peers_cfg.candidates[i].port = 7001 + i; + } + cluster_t *c = calloc(1, sizeof(*c)); + TEST_ASSERT_NOT_NULL(c); + c->myconf = &myconf; + + ha_state_t ha; + TEST_ASSERT_EQUAL_INT(0, ha_init(&ha, &g_cfg, 0 /* priority 0: no delay */)); + ha_do_demote(&ha); + make_three_node(&ha); + ha.raft.term = 6; + ha.raft_c = c; /* production wiring for the delayed trigger */ + g_r2_election_spy_calls = 0; + ha_rdhup_election_hook = r2_election_spy; + + pthread_mutex_lock(&ha.raft_lock); + raft_on_leader_lost(&ha.raft); + pthread_mutex_unlock(&ha.raft_lock); + + ha_rdhup_election_hook = NULL; + TEST_ASSERT_EQUAL_INT_MESSAGE(1, g_r2_election_spy_calls, + "finding 3: the delayed election must go through the stamped start " + "path (ha_enter_real_election / ha_start_election_async)"); + TEST_ASSERT_EQUAL_UINT64_MESSAGE(6, ha.raft.term, + "finding 3: raft_start_election must not be called raw (no carrier)"); + TEST_ASSERT_EQUAL_INT(CTRL_ELECTING, + (int)atomic_load_explicit(&ha.role, memory_order_acquire)); + + ha.raft_c = NULL; + ha_fini(&ha); + free(c); +} + +/* + * Finding 5: a PreVote endpoint's error is routed by endpoint KIND, like its + * replies: in every role it goes to the PreVote tally (which drops it unless + * its round is open), never to the CTRL_PRIMARY promoted-standby cleanup (a + * PreVote endpoint is never promoted). + */ +static void test_r2_f5_prevote_ep_error_routed_by_kind_in_primary(void) +{ + ha_state_t ha; + TEST_ASSERT_EQUAL_INT(0, ha_init(&ha, &g_cfg, 0)); + ha_do_demote(&ha); + make_three_node(&ha); + ha.raft.n_peers = 5; /* one deny leaves the round pending */ + ha.n_peers = 5; + + ha_start_prevote_async(&ha, NULL); + uint64_t r = ha_pre_vote_round_for_test(&ha); + atomic_store_explicit(&ha.role, (int32_t)CTRL_PRIMARY, memory_order_release); + + endpoint_t ep; + memset(&ep, 0, sizeof(ep)); + vote_ep_ctx_build_for_test(&ep, &ha, NULL, /*peer_idx=*/1, 1, 0, 0, "t"); + vote_ep_ctx_mark_pre_vote_for_test(&ep, r); + TEST_ASSERT_EQUAL_INT(0, pre_vote_ep_err_handler_for_test(&ep)); + + TEST_ASSERT_EQUAL_INT_MESSAGE(1, ha.raft.pre_vote_replies[1], + "finding 5: a PreVote endpoint error must reach the PreVote tally in " + "every role (routed by endpoint kind, not by role)"); + + free(ep.data); + ha_fini(&ha); +} + +/* ══ bug-1915 final review ═════════════════════════════════════════════════ */ + +extern int ha_prevote_round_timeout_fn(void *arg); +extern void ha_failover_stagger_fire_for_test(ha_state_t *ha, cluster_t *c); +extern _Atomic int ha_prevote_start_vtask_done_for_test; + +/* Poll an atomic flag for up to `ms` milliseconds; returns its final value. */ +static int fr_wait_flag(_Atomic int *flag, int want, int ms) +{ + for (int i = 0; i < ms; i++) { + if (atomic_load(flag) >= want) + return atomic_load(flag); + usleep(1000); + } + return atomic_load(flag); +} + +/* Holds ha->raft_lock on its own thread until told to release it (a stand-in + * for a pool worker parked in an fsync under raft_lock). */ +typedef struct { + ha_state_t *ha; + _Atomic int locked; + _Atomic int release; +} fr_lock_holder_t; + +static void *fr_lock_holder_fn(void *arg) +{ + fr_lock_holder_t *h = (fr_lock_holder_t *)arg; + pthread_mutex_lock(&h->ha->raft_lock); + atomic_store(&h->locked, 1); + while (!atomic_load(&h->release)) + usleep(1000); + pthread_mutex_unlock(&h->ha->raft_lock); + return NULL; +} + +/* Fires a round-timeout context on its own thread (the delaymgr thread). */ +typedef struct { + void *tctx; + _Atomic int done; +} fr_fire_t; + +static void *fr_fire_fn(void *arg) +{ + fr_fire_t *f = (fr_fire_t *)arg; + ha_prevote_round_timeout_fn(f->tctx); + atomic_store(&f->done, 1); + return NULL; +} + +/* Fire `tctx` on a "delaymgr" thread while another thread holds raft_lock; + * returns 1 if the timer body returned within `ms` while the lock was held. + * The lock is always released and both threads joined before returning. */ +static int fr_fire_under_held_lock(ha_state_t *ha, void *tctx, int ms) +{ + fr_lock_holder_t h; + memset(&h, 0, sizeof(h)); + h.ha = ha; + pthread_t th, tf; + TEST_ASSERT_EQUAL_INT(0, pthread_create(&th, NULL, fr_lock_holder_fn, &h)); + TEST_ASSERT_EQUAL_INT(1, fr_wait_flag(&h.locked, 1, 2000)); + + fr_fire_t f; + memset(&f, 0, sizeof(f)); + f.tctx = tctx; + TEST_ASSERT_EQUAL_INT(0, pthread_create(&tf, NULL, fr_fire_fn, &f)); + int returned_while_locked = fr_wait_flag(&f.done, 1, ms); + + atomic_store(&h.release, 1); + pthread_join(th, NULL); + pthread_join(tf, NULL); + TEST_ASSERT_EQUAL_INT(1, atomic_load(&f.done)); + return returned_while_locked; +} + +/* + * Finding 1: a CANCELLED round timer must return without taking raft_lock — + * the delaymgr thread must never wait behind a raft_lock holder (e.g. a pool + * worker in an fsync). A LIVE timer still takes the locked path (it must + * decide the round under raft_lock), so it does wait — the contrast proves the + * fast path is taken only for a cancelled context. + */ +static void test_final_f1_cancelled_round_timer_skips_raft_lock(void) +{ + ha_state_t ha; + TEST_ASSERT_EQUAL_INT(0, ha_init(&ha, &g_cfg, 0)); + ha_do_demote(&ha); + make_three_node(&ha); + cluster_t *c = calloc(1, sizeof(*c)); /* no myconf: sends are no-ops */ + TEST_ASSERT_NOT_NULL(c); + arm_stub_install(); + g_rearm_calls = 0; + ha_prevote_rearm_liveness_hook = stub_rearm; + + /* Round 1 opens with a timer, then is decided LOST (both peers deny): + * the decision cancels and unpublishes its timer under raft_lock. */ + ha_start_prevote_async(&ha, c); + TEST_ASSERT_EQUAL_INT(1, g_arm_n); + void *cancelled_tctx = g_arm_ctx[0]; + pre_vote_reply_dispatch_for_test(&ha, c, 1, 0); + pre_vote_reply_dispatch_for_test(&ha, c, 2, 0); + TEST_ASSERT_EQUAL_INT(0, ha.raft.pre_vote_in_progress); + TEST_ASSERT_EQUAL_INT_MESSAGE(0, + ha_prevote_active_round_timer_for_test(NULL, NULL), + "precondition: the decided round's timer is unpublished"); + + int fast = fr_fire_under_held_lock(&ha, cancelled_tctx, 1000); + TEST_ASSERT_EQUAL_INT_MESSAGE(1, fast, + "finding 1: a cancelled round timer must return without waiting for " + "raft_lock (delaymgr is trigger-only and must never block)"); + + /* Contrast: a LIVE timer of the open round 2 must take raft_lock — it + * blocks while the lock is held, then decides the round (timeout = LOSS). */ + ha_start_prevote_async(&ha, c); + TEST_ASSERT_EQUAL_INT(2, g_arm_n); + TEST_ASSERT_EQUAL_INT(1, ha.raft.pre_vote_in_progress); + int rearm_before = g_rearm_calls; + int live_fast = fr_fire_under_held_lock(&ha, g_arm_ctx[1], 200); + TEST_ASSERT_EQUAL_INT_MESSAGE(0, live_fast, + "a live round timer must still take raft_lock (locked path)"); + TEST_ASSERT_EQUAL_INT_MESSAGE(0, ha.raft.pre_vote_in_progress, + "the live timer, once it got raft_lock, ends its round as a LOSS"); + TEST_ASSERT_EQUAL_INT(rearm_before + 1, g_rearm_calls); + + ha_prevote_rearm_liveness_hook = NULL; + arm_stub_remove(); + free(c); + ha_fini(&ha); +} + +/* + * Finding 3: a late RequestVoteReply on an endpoint of an EARLIER election + * (vctx->my_term = T-1) from a voter now in term T carries the current term T + * and granted=0. It must be dropped — not counted as that peer's deny in the + * term-T election — so the peer's genuine term-T reply still counts. + */ +static void test_final_f3_stale_election_reply_not_counted(void) +{ + ha_state_t ha; + TEST_ASSERT_EQUAL_INT(0, ha_init(&ha, &g_cfg, 0)); + make_three_node(&ha); + ha.raft.n_peers = 5; /* one grant does not decide: observe the tally */ + ha.n_peers = 5; + ha.raft.term = 3; + atomic_store_explicit(&ha.role, (int32_t)CTRL_STANDBY, memory_order_release); + + ha_start_election_async(&ha, NULL, 5, NULL, /*tag=*/1, /*lsn=*/10, "n0"); + TEST_ASSERT_EQUAL_INT_MESSAGE(RAFT_CANDIDATE, ha.raft.role, + "precondition: the term-T election is in flight"); + uint64_t T = ha.raft.term; + TEST_ASSERT_EQUAL_UINT64(4, T); + TEST_ASSERT_EQUAL_INT(0, ha.raft.replies_received); + + /* Stale: endpoint of the term-(T-1) election; voter already in term T. */ + endpoint_t stale; + memset(&stale, 0, sizeof(stale)); + vote_ep_ctx_build_for_test(&stale, &ha, NULL, /*votes_idx=*/1, T - 1, 0, 0, "t"); + io_packet_t *pkt = vote_reply_build_pkt_for_test(T, /*granted=*/0, 0, 0); + TEST_ASSERT_NOT_NULL(pkt); + TEST_ASSERT_EQUAL_INT(0, peer_ctrl_msg_handler_for_test(&stale, pkt)); + + TEST_ASSERT_EQUAL_INT_MESSAGE(0, ha.raft.replies_received, + "finding 3: a reply from an earlier election's endpoint must not be " + "counted in the current election"); + TEST_ASSERT_EQUAL_INT(0, ha.raft.vote_replies[2]); + TEST_ASSERT_EQUAL_INT(RAFT_CANDIDATE, ha.raft.role); + + /* Genuine: the same peer's reply on this election's endpoint counts. */ + endpoint_t cur; + memset(&cur, 0, sizeof(cur)); + vote_ep_ctx_build_for_test(&cur, &ha, NULL, /*votes_idx=*/1, T, 0, 0, "t"); + pkt = vote_reply_build_pkt_for_test(T, /*granted=*/1, 0, 0); + TEST_ASSERT_NOT_NULL(pkt); + TEST_ASSERT_EQUAL_INT(0, peer_ctrl_msg_handler_for_test(&cur, pkt)); + TEST_ASSERT_EQUAL_INT_MESSAGE(2, ha.raft.votes_granted, + "finding 3: the peer's genuine reply must be counted, not dropped as " + "a duplicate of the stale one"); + TEST_ASSERT_EQUAL_INT(1, ha.raft.replies_received); + + free(stale.data); + free(cur.data); + drop_published_ctx(&ha, NULL); + ha_fini(&ha); +} + +/* + * Finding 3, higher-term exception: a late reply on an earlier election's + * endpoint whose term is ABOVE ours must still make this node adopt the term + * and step down (Raft: any higher term steps down), but must never be counted + * as a vote in the current election. + */ +static void test_final_f3_stale_higher_term_reply_steps_down_without_vote(void) +{ + ha_state_t ha; + TEST_ASSERT_EQUAL_INT(0, ha_init(&ha, &g_cfg, 0)); + make_three_node(&ha); + ha.raft.n_peers = 5; + ha.n_peers = 5; + ha.raft.term = 3; + atomic_store_explicit(&ha.role, (int32_t)CTRL_STANDBY, memory_order_release); + + ha_start_election_async(&ha, NULL, 5, NULL, /*tag=*/1, /*lsn=*/10, "n0"); + TEST_ASSERT_EQUAL_INT_MESSAGE(RAFT_CANDIDATE, ha.raft.role, + "precondition: the term-T election is in flight"); + uint64_t T = ha.raft.term; + TEST_ASSERT_EQUAL_UINT64(4, T); + int granted_before = ha.raft.votes_granted; + + endpoint_t stale; + memset(&stale, 0, sizeof(stale)); + vote_ep_ctx_build_for_test(&stale, &ha, NULL, /*votes_idx=*/1, T - 1, 0, 0, "t"); + io_packet_t *pkt = vote_reply_build_pkt_for_test(T + 1, /*granted=*/1, 0, 0); + TEST_ASSERT_NOT_NULL(pkt); + TEST_ASSERT_EQUAL_INT(0, peer_ctrl_msg_handler_for_test(&stale, pkt)); + + TEST_ASSERT_EQUAL_UINT64_MESSAGE(T + 1, ha.raft.term, + "a higher-term reply must make the node adopt that term"); + TEST_ASSERT_EQUAL_INT_MESSAGE(RAFT_FOLLOWER, ha.raft.role, + "a higher-term reply must step the candidate down"); + TEST_ASSERT_EQUAL_INT_MESSAGE(0, ha.raft.replies_received, + "a stale higher-term reply must never be counted as a reply"); + TEST_ASSERT_EQUAL_INT_MESSAGE(granted_before, ha.raft.votes_granted, + "a stale higher-term grant must never be counted as a vote"); + + free(stale.data); + drop_published_ctx(&ha, NULL); + ha_fini(&ha); +} + +/* Finding 4 probes: where ha_start_prevote_async ran (pre-arm seam fires + * inside it, under raft_lock, after the round opened). */ +static _Atomic int g_f4_hook_runs = 0; +static pthread_t g_f4_hook_thread; +static void f4_record_thread(ha_state_t *ha, cluster_t *c, uint64_t round) +{ + (void)ha; (void)c; (void)round; + g_f4_hook_thread = pthread_self(); + atomic_fetch_add(&g_f4_hook_runs, 1); +} + +/* + * Finding 4: the failover-stagger delaymgr body must hand the PreVote start + * (per-peer bounded connects under raft_lock) to a VTASK_LIVE_BLOCK vtask, not + * run it inline on the delaymgr thread; the vtask then starts the round. + */ +static void test_final_f4_stagger_hands_prevote_start_to_vtask(void) +{ + vtaskmgr_t pool; + TEST_ASSERT_EQUAL_INT(0, vtaskmgr_init(&pool, 4)); + TEST_ASSERT_EQUAL_INT(0, vtaskmgr_start(&pool)); + + ha_state_t ha; + TEST_ASSERT_EQUAL_INT(0, ha_init(&ha, &g_cfg, 0)); + ha_do_demote(&ha); + make_three_node(&ha); + ha.last_activity = 0; /* no live primary: the stagger proceeds */ + ha.pool = &pool; + uint64_t r0 = ha_pre_vote_round_for_test(&ha); + int done0 = atomic_load(&ha_prevote_start_vtask_done_for_test); + + atomic_store(&g_f4_hook_runs, 0); + ha_prevote_pre_arm_hook = f4_record_thread; + pthread_t caller = pthread_self(); + + ha_failover_stagger_fire_for_test(&ha, NULL); + + int runs = fr_wait_flag(&g_f4_hook_runs, 1, 2000); + int done = fr_wait_flag(&ha_prevote_start_vtask_done_for_test, done0 + 1, 2000); + int inline_run = (runs > 0) && pthread_equal(g_f4_hook_thread, caller); + ha_prevote_pre_arm_hook = NULL; + + TEST_ASSERT_EQUAL_INT_MESSAGE(1, runs, + "finding 4: the PreVote round must be started exactly once"); + TEST_ASSERT_EQUAL_INT_MESSAGE(0, inline_run, + "finding 4: the stagger body must hand the PreVote start to a vtask, " + "not connect inline on the delaymgr thread"); + TEST_ASSERT_EQUAL_INT(done0 + 1, done); + TEST_ASSERT_EQUAL_UINT64(r0 + 1, ha_pre_vote_round_for_test(&ha)); + pthread_mutex_lock(&ha.raft_lock); + int in_prog = ha.raft.pre_vote_in_progress; + pthread_mutex_unlock(&ha.raft_lock); + TEST_ASSERT_EQUAL_INT_MESSAGE(1, in_prog, + "finding 4: the vtask must start the PreVote round"); + + vtaskmgr_fini(&pool); + ha.pool = NULL; + ha_fini(&ha); +} + +/* Finding 4 (skip kept): a live primary seen during the stagger skips the + * PreVote on the delaymgr thread — nothing is enqueued, no round opens. */ +static void test_final_f4_stagger_skips_when_primary_alive(void) +{ + vtaskmgr_t pool; + TEST_ASSERT_EQUAL_INT(0, vtaskmgr_init(&pool, 4)); + TEST_ASSERT_EQUAL_INT(0, vtaskmgr_start(&pool)); + + ha_state_t ha; + TEST_ASSERT_EQUAL_INT(0, ha_init(&ha, &g_cfg, 0)); + ha_do_demote(&ha); + make_three_node(&ha); + struct timespec now; + clock_gettime(CLOCK_MONOTONIC, &now); + ha.last_activity = now.tv_sec; /* fresh primary heartbeat */ + atomic_store_explicit(&ha.leaderless, 0, memory_order_release); + ha.pool = &pool; + uint64_t r0 = ha_pre_vote_round_for_test(&ha); + int done0 = atomic_load(&ha_prevote_start_vtask_done_for_test); + + ha_failover_stagger_fire_for_test(&ha, NULL); + usleep(50 * 1000); + + TEST_ASSERT_EQUAL_INT(done0, atomic_load(&ha_prevote_start_vtask_done_for_test)); + TEST_ASSERT_EQUAL_UINT64(r0, ha_pre_vote_round_for_test(&ha)); + TEST_ASSERT_EQUAL_INT(0, ha.raft.pre_vote_in_progress); + + vtaskmgr_fini(&pool); + ha.pool = NULL; + ha_fini(&ha); +} + +/* Finding 4 (fallback): with no pool, the stagger starts the round inline + * (fail loud, stay alive — mirrors ha_dispatch_rdhup_failover). */ +static void test_final_f4_stagger_inline_fallback_without_pool(void) +{ + ha_state_t ha; + TEST_ASSERT_EQUAL_INT(0, ha_init(&ha, &g_cfg, 0)); + ha_do_demote(&ha); + make_three_node(&ha); + ha.last_activity = 0; + ha.pool = NULL; + uint64_t r0 = ha_pre_vote_round_for_test(&ha); + + ha_failover_stagger_fire_for_test(&ha, NULL); + + TEST_ASSERT_EQUAL_UINT64(r0 + 1, ha_pre_vote_round_for_test(&ha)); + TEST_ASSERT_EQUAL_INT(1, ha.raft.pre_vote_in_progress); + ha_fini(&ha); +} + /* ── Suite entry point ──────────────────────────────────────────────────── */ void test_ha_prevote(void) @@ -931,4 +2150,35 @@ void test_ha_prevote(void) RUN_TEST(test_genuine_failover_stale_peer_grants_and_elects); RUN_TEST(test_heartbeat_mid_round_clean_reattach); RUN_TEST(test_lost_round_rearms_then_later_failure_elects); + /* bug-1915 — replies after the round is decided are dropped */ + RUN_TEST(test_bug1915_late_grant_after_won_is_ignored); + RUN_TEST(test_bug1915_late_deny_after_won_is_ignored); + /* bug-1915 follow-up gaps (a)-(e) + promotion-time carrier check */ + RUN_TEST(test_gapA_election_start_noop_when_raft_leader); + RUN_TEST(test_gapA_election_start_noop_when_raft_candidate); + RUN_TEST(test_gapB_prevote_refused_during_real_election); + RUN_TEST(test_gapB_prevote_refused_when_leader); + RUN_TEST(test_gapB_prevote_restart_mid_round_keeps_round); + RUN_TEST(test_gapD_stale_round_reply_dropped); + RUN_TEST(test_gapD_stale_round_timeout_does_not_end_current_round); + RUN_TEST(test_gapD_stale_round_endpoint_error_not_counted); + RUN_TEST(test_gapD_late_prevote_reply_never_counts_as_real_vote); + RUN_TEST(test_gapD_real_vote_reply_never_feeds_prevote_tally); + RUN_TEST(test_become_leader_refuses_prevote_carrier); + RUN_TEST(test_become_leader_refuses_stamped_carrier_without_majority); + RUN_TEST(test_become_leader_refuses_carrier_of_other_term); + RUN_TEST(test_become_leader_accepts_own_carrier_with_majority); + RUN_TEST(test_gapE_concurrent_grants_decide_round_once); + RUN_TEST(test_r2_f1_open_round_always_has_its_timer); + RUN_TEST(test_r2_f6_round_timer_arm_failure_closes_round); + RUN_TEST(test_r2_f2_refused_promotion_steps_down_and_rearms); + RUN_TEST(test_r2_f3_delayed_election_uses_stamped_start_path); + RUN_TEST(test_r2_f5_prevote_ep_error_routed_by_kind_in_primary); + /* bug-1915 final review */ + RUN_TEST(test_final_f1_cancelled_round_timer_skips_raft_lock); + RUN_TEST(test_final_f3_stale_election_reply_not_counted); + RUN_TEST(test_final_f3_stale_higher_term_reply_steps_down_without_vote); + RUN_TEST(test_final_f4_stagger_hands_prevote_start_to_vtask); + RUN_TEST(test_final_f4_stagger_skips_when_primary_alive); + RUN_TEST(test_final_f4_stagger_inline_fallback_without_pool); } diff --git a/src/controller/testsuite/test_ha_vote_send.c b/src/controller/testsuite/test_ha_vote_send.c index 89daee6f..2fe1bff5 100644 --- a/src/controller/testsuite/test_ha_vote_send.c +++ b/src/controller/testsuite/test_ha_vote_send.c @@ -737,6 +737,13 @@ static void test_post_connect_vote_failure_routes_through_production_tally(void) extern int pre_vote_ep_err_handler_for_test(endpoint_t *ep); +/* bug-1915 follow-up: a production PreVote endpoint is stamped with its kind + * and the round it belongs to (raft_cb_send_pre_vote_request); an endpoint + * built with vote_ep_ctx_build_for_test is stamped the same way here, for the + * round currently in progress. */ +extern void vote_ep_ctx_mark_pre_vote_for_test(endpoint_t *ep, uint64_t round); +extern uint64_t ha_pre_vote_round_for_test(ha_state_t *ha); + extern void pre_vote_reply_dispatch_for_test(ha_state_t *ha, cluster_t *c, int peer_idx, int granted); @@ -797,6 +804,7 @@ static void test_post_connect_prevote_failure_routes_through_production_tally(vo * registered err_handler (pre_vote_ep_err_handler) on this same ep. */ vote_ep_ctx_build_for_test(&ep, &ha, ctx, /*peer_idx=*/1, 4ULL, 100ULL, 50ULL, "ctrl-a"); + vote_ep_ctx_mark_pre_vote_for_test(&ep, ha_pre_vote_round_for_test(&ha)); int rc = pre_vote_ep_err_handler_for_test(&ep); TEST_ASSERT_EQUAL_INT(0, rc); @@ -850,6 +858,7 @@ static void test_prevote_round_reaches_quorum_decision_via_fast_tally(void) make_endpoint(&ep1); vote_ep_ctx_build_for_test(&ep1, &ha, ctx, /*peer_idx=*/1, 4ULL, 100ULL, 50ULL, "ctrl-a"); + vote_ep_ctx_mark_pre_vote_for_test(&ep1, ha_pre_vote_round_for_test(&ha)); TEST_ASSERT_EQUAL_INT(0, pre_vote_ep_err_handler_for_test(&ep1)); TEST_ASSERT_EQUAL_INT_MESSAGE(1, ha.raft.pre_vote_in_progress, diff --git a/src/lib/raft/raft.c b/src/lib/raft/raft.c index 19ba1304..deeb8e08 100644 --- a/src/lib/raft/raft.c +++ b/src/lib/raft/raft.c @@ -77,6 +77,9 @@ int raft_init(raft_state_t *s, const raft_config_t *cfg) s->election_decided = 0; s->election_timeout_ms = cfg->election_timeout_ms ? cfg->election_timeout_ms : 2000; s->my_priority = cfg->my_priority; + /* No leader lost yet: the restored term (raft_check_and_start_election is + * only meaningful after raft_on_leader_lost, which overwrites this). */ + s->leader_lost_term = cfg->initial_term; /* Log length restored from the adapter's recovered WAL length; the library * never reads storage (same discipline as initial_term/voted_for). 0 on * first boot. (raft-log-replication Req 1.1; design "log_len restored at @@ -374,6 +377,17 @@ int raft_on_pre_vote_reply(raft_state_t *s, int peer_idx, int granted) * controller (task 4.1) acts on. There is no term in a pre-vote reply, so * no term adoption is even possible here. */ + /* No-op guard — the round is already decided, or none was started + * (mirrors raft_on_vote_reply's election_decided check). WON and LOST + * both clear pre_vote_in_progress, and pre_votes_granted never decreases, + * so tallying a peer that had not replied yet would read a WON round as + * WON again on its late GRANT or DENY (bug-1915). */ + if (!s->pre_vote_in_progress) { + if (s->cbs->on_trace) + s->cbs->on_trace(s->ctx, "pre_vote_reply_noop", NULL); + return RAFT_PRE_VOTE_PENDING; + } + /* Bounds guard: ignore replies from an out-of-range peer (mirrors the * per-peer dedup array bound). A self reply (peer_idx == self_idx) is * never expected — self is granted by raft_begin_pre_vote — and would also @@ -537,8 +551,12 @@ void raft_on_election_timeout(raft_state_t *s) void raft_on_leader_lost(raft_state_t *s) { - s->voted_for = -1; - s->role = RAFT_FOLLOWER; + /* bug-1915 final review: do NOT clear voted_for. The term does not change + * here, so a vote already cast in it still binds — clearing it let a + * delayed RequestVote(term) from ANOTHER candidate be granted (a second + * vote in one term). The delayed-election guard keys on the term instead. */ + s->leader_lost_term = s->term; + s->role = RAFT_FOLLOWER; s->cbs->on_start_election_after_delay(s->ctx, (uint32_t)s->my_priority * s->election_timeout_ms); if (s->cbs->on_trace) @@ -547,13 +565,18 @@ void raft_on_leader_lost(raft_state_t *s) int raft_check_and_start_election(raft_state_t *s) { - if (s->voted_for != -1 && s->voted_for != s->self_idx) { + /* Skip unless still a FOLLOWER in the leader-lost term: a term change + * during the delay (a grant to another candidate's higher term, a new + * leader) or a role change means another node moved the cluster on. + * voted_for is no longer a usable signal — raft_on_leader_lost keeps the + * vote cast in this term (often for the lost leader itself). */ + if (s->term != s->leader_lost_term || s->role != RAFT_FOLLOWER) { if (s->cbs->on_trace) s->cbs->on_trace(s->ctx, "jitter_skip", NULL); return RAFT_SKIP_ELECTION; } - /* Precondition: raft_on_leader_lost sets role=FOLLOWER, so -EALREADY is unreachable here */ + /* Precondition: role == FOLLOWER (checked above), so -EALREADY is unreachable here */ int rc = raft_start_election(s); (void)rc; return RAFT_ELECTION_STARTED; diff --git a/src/lib/raft/raft.h b/src/lib/raft/raft.h index 08ffc41a..e545fcbd 100644 --- a/src/lib/raft/raft.h +++ b/src/lib/raft/raft.h @@ -38,6 +38,12 @@ typedef struct { int election_decided; /* 1 once on_become_leader/follower fired */ uint32_t election_timeout_ms; int my_priority; /* priority index for jitter (0 = highest) */ + /* Term in which raft_on_leader_lost last ran (volatile, never persisted). + * raft_check_and_start_election starts the delayed election only while the + * node is still a FOLLOWER in this term: a term change during the delay + * means another candidate (or a new leader) moved the cluster on. + * raft_on_leader_lost no longer clears voted_for (bug-1915 final review). */ + uint64_t leader_lost_term; /* ---- PreVote round tally (Req 1.2) ---- * Self-contained pre-vote bookkeeping. A pre-vote round mutates NONE of * the persistent Raft state above (term, voted_for, role); these fields @@ -284,14 +290,20 @@ void raft_on_election_timeout(raft_state_t *s); /* * raft_on_leader_lost: primary control channel dropped (RDHUP). - * Clears voted_for, transitions to FOLLOWER, fires on_start_election_after_delay. + * Records leader_lost_term = term, transitions to FOLLOWER, fires + * on_start_election_after_delay. Does NOT clear voted_for: the term is + * unchanged, so the vote cast in it still binds (one vote per term — clearing + * it let a delayed RequestVote of the same term from another candidate be + * granted; bug-1915 final review). */ void raft_on_leader_lost(raft_state_t *s); /* * raft_check_and_start_election: call after the delay from raft_on_leader_lost. - * Starts election only if we have not already voted for a non-self candidate - * in the new term (i.e., another node won during the jitter window). + * Starts the election only if the node is still a FOLLOWER in the term in which + * the leader was lost (term == leader_lost_term). Any term change during the + * delay (a vote granted to another candidate's higher term, a new leader's + * heartbeat) or a role change means the cluster moved on: skip ("jitter_skip"). * Returns RAFT_SKIP_ELECTION or RAFT_ELECTION_STARTED. */ int raft_check_and_start_election(raft_state_t *s); @@ -337,7 +349,10 @@ int raft_on_pre_vote_request(raft_state_t *s, int from_idx, uint64_t cand_term, * pre_vote_replies), mirroring raft_on_vote_reply but WITHOUT firing * on_become_leader/on_become_follower or making any role/term change. * Returns RAFT_PRE_VOTE_WON at quorum, RAFT_PRE_VOTE_LOST when all peers - * replied without quorum, else RAFT_PRE_VOTE_PENDING. + * replied without quorum, else RAFT_PRE_VOTE_PENDING. A reply while no round + * is in progress (already decided WON/LOST, or never started) is a no-op that + * returns RAFT_PRE_VOTE_PENDING and leaves the tally untouched, so a round is + * decided at most once. */ int raft_on_pre_vote_reply(raft_state_t *s, int peer_idx, int granted); diff --git a/src/lib/testsuite/test_raft.c b/src/lib/testsuite/test_raft.c index 083059ef..6f0624fc 100644 --- a/src/lib/testsuite/test_raft.c +++ b/src/lib/testsuite/test_raft.c @@ -3133,15 +3133,18 @@ static void test_election_timeout_already_decided(void) /* ---------- raft_on_leader_lost tests (task 3.5) ---------- */ /* - * test_leader_lost_clears_voted_for + * test_leader_lost_keeps_voted_for (was test_leader_lost_clears_voted_for) * - * Node had voted_for=1. raft_on_leader_lost must: - * - set voted_for=-1 + * Node had voted_for=1 in its current term. raft_on_leader_lost must: + * - KEEP voted_for (bug-1915 final review: clearing it without a term change + * let a delayed RequestVote of the SAME term from another candidate be + * granted — a second vote in one term, an Election Safety violation) + * - record leader_lost_term = term (raft_check_and_start_election's guard) * - set role=FOLLOWER * - fire on_start_election_after_delay with delay = my_priority * election_timeout_ms * - trace "leader_lost" */ -static void test_leader_lost_clears_voted_for(void) +static void test_leader_lost_keeps_voted_for(void) { raft_state_t s; raft_callbacks_t cbs = make_mock_cbs(); @@ -3159,12 +3162,18 @@ static void test_leader_lost_clears_voted_for(void) cfg.cbs = &cbs; TEST_ASSERT_EQUAL_INT(0, raft_init(&s, &cfg)); - s.voted_for = 1; - s.role = RAFT_FOLLOWER; + s.term = 5; + s.voted_for = 1; + s.voted_for_term = 5; + s.role = RAFT_FOLLOWER; raft_on_leader_lost(&s); - TEST_ASSERT_EQUAL_INT(-1, s.voted_for); + TEST_ASSERT_EQUAL_INT_MESSAGE(1, s.voted_for, + "leader-lost must not clear the vote cast in the current term"); + TEST_ASSERT_EQUAL_UINT64(5, s.voted_for_term); + TEST_ASSERT_EQUAL_UINT64(5, s.term); + TEST_ASSERT_EQUAL_UINT64(5, s.leader_lost_term); TEST_ASSERT_EQUAL_INT(RAFT_FOLLOWER, s.role); /* on_start_election_after_delay fired once with delay = priority * timeout */ @@ -3176,12 +3185,59 @@ static void test_leader_lost_clears_voted_for(void) TEST_ASSERT_EQUAL_STRING("leader_lost", g_mock.last_trace_event); } +/* + * test_leader_lost_same_term_vote_from_other_denied (bug-1915 final review) + * + * Node voted for candidate 1 in term 5, then lost its leader. A delayed + * RequestVote(term 5) from ANOTHER candidate (2) must be DENIED: at most one + * vote per node per term, whatever happened to the leader in between. + */ +static void test_leader_lost_same_term_vote_from_other_denied(void) +{ + raft_state_t s; + raft_callbacks_t cbs = make_mock_cbs(); + uint64_t reply_term = 0; + + cbs.on_trace = mock_trace; + reset_mock(); + + raft_config_t cfg; + memset(&cfg, 0, sizeof(cfg)); + cfg.n_peers = 3; + cfg.self_idx = 0; + cfg.election_timeout_ms = 500; + cfg.my_priority = 1; + cfg.initial_term = 5; + cfg.initial_voted_for = 1; /* voted for candidate 1 in term 5 */ + cfg.cbs = &cbs; + TEST_ASSERT_EQUAL_INT(0, raft_init(&s, &cfg)); + TEST_ASSERT_EQUAL_UINT64(5, s.voted_for_term); + + raft_on_leader_lost(&s); + + int persist_before = g_mock.persist_count; + int rc = raft_on_vote_request(&s, 2, 5, 0, 0, &reply_term); + TEST_ASSERT_EQUAL_INT_MESSAGE(RAFT_DENY, rc, + "a second vote in term 5 (for candidate 2) must be denied after " + "leader-lost"); + TEST_ASSERT_EQUAL_INT(1, s.voted_for); + TEST_ASSERT_EQUAL_UINT64(5, s.term); + TEST_ASSERT_EQUAL_INT_MESSAGE(persist_before, g_mock.persist_count, + "a denied vote must not persist"); + + /* The same candidate's retry is still an idempotent re-grant. */ + rc = raft_on_vote_request(&s, 1, 5, 0, 0, &reply_term); + TEST_ASSERT_EQUAL_INT(RAFT_GRANT, rc); + TEST_ASSERT_EQUAL_INT(1, s.voted_for); +} + /* ---------- raft_check_and_start_election tests (task 3.5) ---------- */ /* * test_check_and_start_voted_for_other * - * voted_for=1, self_idx=0 (voted for a different candidate). + * Leader lost in term 3; during the delay this node granted another candidate + * (peer 1) a vote in its higher term 4 (term moved on, voted_for=1). * Must return RAFT_SKIP_ELECTION, role stays FOLLOWER, no election started. * Trace "jitter_skip" fires. */ @@ -3189,13 +3245,18 @@ static void test_check_and_start_voted_for_other(void) { raft_state_t s; raft_callbacks_t cbs = make_mock_cbs(); + uint64_t reply_term = 0; cbs.on_trace = mock_trace; reset_mock(); TEST_ASSERT_EQUAL_INT(0, init_node(&s, &cbs, 3, 0)); - s.voted_for = 1; /* voted for peer 1, not self (0) */ - s.role = RAFT_FOLLOWER; + s.term = 3; + raft_on_leader_lost(&s); + TEST_ASSERT_EQUAL_INT(RAFT_GRANT, + raft_on_vote_request(&s, 1, 4, 0, 0, &reply_term)); + TEST_ASSERT_EQUAL_INT(1, s.voted_for); /* voted for peer 1, not self (0) */ + TEST_ASSERT_EQUAL_INT(RAFT_FOLLOWER, s.role); int rc = raft_check_and_start_election(&s); @@ -3210,6 +3271,54 @@ static void test_check_and_start_voted_for_other(void) TEST_ASSERT_EQUAL_STRING("jitter_skip", g_mock.last_trace_event); } +/* + * test_check_and_start_after_leader_lost_same_term_starts (bug-1915 final review) + * + * The vote raft_on_leader_lost now keeps (here: for the lost leader, peer 1, + * in term 5) must not block the delayed election: the term has not moved, so + * the node starts its election in term 6. + */ +static void test_check_and_start_after_leader_lost_same_term_starts(void) +{ + raft_state_t s; + raft_callbacks_t cbs = make_mock_cbs(); + + reset_mock(); + TEST_ASSERT_EQUAL_INT(0, init_node(&s, &cbs, 3, 0)); + s.term = 5; + s.voted_for = 1; + s.voted_for_term = 5; + raft_on_leader_lost(&s); + + TEST_ASSERT_EQUAL_INT(RAFT_ELECTION_STARTED, + raft_check_and_start_election(&s)); + TEST_ASSERT_EQUAL_INT(RAFT_CANDIDATE, s.role); + TEST_ASSERT_EQUAL_UINT64(6, s.term); +} + +/* + * test_check_and_start_skips_when_not_follower (bug-1915 final review) + * + * Leader lost in term 5, then the node left FOLLOWER (e.g. another trigger + * already started its election) — the delayed start must skip. + */ +static void test_check_and_start_skips_when_not_follower(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, 3, 0)); + s.term = 5; + raft_on_leader_lost(&s); + s.role = RAFT_CANDIDATE; /* same term, but no longer a follower */ + + TEST_ASSERT_EQUAL_INT(RAFT_SKIP_ELECTION, raft_check_and_start_election(&s)); + TEST_ASSERT_EQUAL_UINT64(5, s.term); + TEST_ASSERT_EQUAL_STRING("jitter_skip", g_mock.last_trace_event); +} + /* * test_check_and_start_not_voted * @@ -6461,11 +6570,11 @@ static void test_proto_F1_leader_lost_priority_delay(void) /* * test_proto_F2_voted_during_delay_skip * - * Node calls raft_on_leader_lost (voted_for=-1). + * Node calls raft_on_leader_lost (voted_for stays -1). * During the jitter delay, a vote request arrives from node 0 at term=1. * Node grants the vote (voted_for=0). * Then raft_check_and_start_election is called. - * Since voted_for==0 != self_idx==1, must return RAFT_SKIP_ELECTION. + * Since the term moved past leader_lost_term, must return RAFT_SKIP_ELECTION. */ static void test_proto_F2_voted_during_delay_skip(void) { @@ -6484,7 +6593,7 @@ static void test_proto_F2_voted_during_delay_skip(void) cfg.cbs = &cbs; TEST_ASSERT_EQUAL_INT(0, raft_init(&s, &cfg)); - /* Leader lost: clears voted_for to -1, schedules delayed election */ + /* Leader lost: records leader_lost_term (voted_for kept), schedules delayed election */ raft_on_leader_lost(&s); TEST_ASSERT_EQUAL_INT(-1, s.voted_for); @@ -6493,7 +6602,7 @@ static void test_proto_F2_voted_during_delay_skip(void) TEST_ASSERT_EQUAL_INT(RAFT_GRANT, rc); TEST_ASSERT_EQUAL_INT(0, s.voted_for); - /* Timer fires: check-and-start sees voted_for=0 != self_idx=1 → skip */ + /* Timer fires: check-and-start sees term 1 != leader_lost_term 0 → skip */ int check_rc = raft_check_and_start_election(&s); TEST_ASSERT_EQUAL_INT(RAFT_SKIP_ELECTION, check_rc); @@ -6524,7 +6633,7 @@ static void test_proto_F3_no_vote_during_delay_starts_election(void) cfg.cbs = &cbs; TEST_ASSERT_EQUAL_INT(0, raft_init(&s, &cfg)); - /* Leader lost: voted_for cleared to -1 */ + /* Leader lost (voted_for kept; it was -1) */ raft_on_leader_lost(&s); TEST_ASSERT_EQUAL_INT(-1, s.voted_for); @@ -6539,11 +6648,11 @@ static void test_proto_F3_no_vote_during_delay_starts_election(void) * test_proto_F4_simultaneous_rdhup_one_leader * * Nodes 1 and 2 both receive RDHUP (leader/node 0 lost). - * Both call raft_on_leader_lost → voted_for=-1 for both. + * Both call raft_on_leader_lost (voted_for kept; -1 for both here). * Node 1 (lower idx, lower priority) calls check_and_start first → ELECTION_STARTED. * Node 1 sends vote requests; deliver them to node 2. * Node 2 grants node 1 (voted_for=1). - * Node 2 then calls check_and_start → RAFT_SKIP_ELECTION (voted_for=1 != self_idx=2). + * Node 2 then calls check_and_start → RAFT_SKIP_ELECTION (term moved past leader_lost_term). * Result: leader_count==1. */ static void test_proto_F4_simultaneous_rdhup_one_leader(void) @@ -6574,7 +6683,7 @@ static void test_proto_F4_simultaneous_rdhup_one_leader(void) /* Node 2 received node 1's vote request and granted it */ TEST_ASSERT_EQUAL_INT(1, c.nodes[2].state.voted_for); - /* Node 2's timer fires: check_and_start sees voted_for=1 != self_idx=2 → skip */ + /* Node 2's timer fires: its term moved (granted node 1's term) → skip */ int rc2 = raft_check_and_start_election(&c.nodes[2].state); TEST_ASSERT_EQUAL_INT(RAFT_SKIP_ELECTION, rc2); @@ -7538,6 +7647,63 @@ static void test_pre_vote_reply_5node_won_at_three(void) TEST_ASSERT_EQUAL_INT(0, g_mock.become_follower_count); } +/* + * bug-1915 follow-up (defence in depth): a PreVote round is decided exactly + * once. After WON (or LOST) cleared pre_vote_in_progress, a reply from a peer + * that has not been counted yet must be a no-op — PENDING, tally untouched — + * mirroring raft_on_vote_reply's election_decided guard. Before the fix the + * late reply was tallied and, since pre_votes_granted never decreases, a late + * GRANT or DENY to a WON 3-node round read as WON a second time. + */ +static void test_pre_vote_reply_late_grant_after_won_is_noop(void) +{ + raft_state_t s; + raft_callbacks_t cbs = make_mock_cbs(); + + reset_mock(); + TEST_ASSERT_EQUAL_INT(0, arm_pre_vote_round(&s, &cbs, 3, 0, 5)); + TEST_ASSERT_EQUAL_INT(RAFT_PRE_VOTE_WON, raft_on_pre_vote_reply(&s, 1, 1)); + + TEST_ASSERT_EQUAL_INT_MESSAGE(RAFT_PRE_VOTE_PENDING, + raft_on_pre_vote_reply(&s, 2, 1 /* late grant */), + "a reply to an already-decided PreVote round must not decide it again"); + TEST_ASSERT_EQUAL_INT(2, s.pre_votes_granted); /* not tallied */ + TEST_ASSERT_EQUAL_INT(0, s.pre_vote_replies[2]); /* not marked */ + TEST_ASSERT_EQUAL_INT(0, s.pre_vote_in_progress); +} + +static void test_pre_vote_reply_late_deny_after_won_is_noop(void) +{ + raft_state_t s; + raft_callbacks_t cbs = make_mock_cbs(); + + reset_mock(); + TEST_ASSERT_EQUAL_INT(0, arm_pre_vote_round(&s, &cbs, 3, 0, 5)); + TEST_ASSERT_EQUAL_INT(RAFT_PRE_VOTE_WON, raft_on_pre_vote_reply(&s, 1, 1)); + + TEST_ASSERT_EQUAL_INT_MESSAGE(RAFT_PRE_VOTE_PENDING, + raft_on_pre_vote_reply(&s, 2, 0 /* late deny */), + "a late DENY must not re-read the old grant count as WON"); + TEST_ASSERT_EQUAL_INT(2, s.pre_votes_granted); + TEST_ASSERT_EQUAL_INT(0, s.pre_vote_replies[2]); +} + +/* No round ever started (fresh state): a stray reply is ignored, not tallied. */ +static void test_pre_vote_reply_without_round_is_noop(void) +{ + raft_state_t s; + raft_callbacks_t cbs = make_mock_cbs(); + + reset_mock(); + TEST_ASSERT_EQUAL_INT(0, init_vote_node(&s, &cbs, 3, 0, 5)); + TEST_ASSERT_EQUAL_INT(0, s.pre_vote_in_progress); + + TEST_ASSERT_EQUAL_INT(RAFT_PRE_VOTE_PENDING, + raft_on_pre_vote_reply(&s, 1, 1)); + TEST_ASSERT_EQUAL_INT(0, s.pre_vote_replies[1]); + TEST_ASSERT_EQUAL_INT(0, s.pre_vote_in_progress); +} + /* ====================================================================== * * Task 6.1 — Raft safety-invariant assertions for the PreVote API * @@ -9210,8 +9376,11 @@ void test_raft(void) /* raft_on_election_timeout, raft_on_leader_lost, raft_check_and_start_election (task 3.5) */ RUN_TEST(test_election_timeout_undecided); RUN_TEST(test_election_timeout_already_decided); - RUN_TEST(test_leader_lost_clears_voted_for); + RUN_TEST(test_leader_lost_keeps_voted_for); + RUN_TEST(test_leader_lost_same_term_vote_from_other_denied); RUN_TEST(test_check_and_start_voted_for_other); + RUN_TEST(test_check_and_start_after_leader_lost_same_term_starts); + RUN_TEST(test_check_and_start_skips_when_not_follower); RUN_TEST(test_check_and_start_not_voted); RUN_TEST(test_check_and_start_voted_for_self); @@ -9240,6 +9409,9 @@ void test_raft(void) RUN_TEST(test_pre_vote_reply_lost_purity_state_unchanged); RUN_TEST(test_pre_vote_reply_out_of_range_peer_ignored); RUN_TEST(test_pre_vote_reply_5node_won_at_three); + RUN_TEST(test_pre_vote_reply_late_grant_after_won_is_noop); + RUN_TEST(test_pre_vote_reply_late_deny_after_won_is_noop); + RUN_TEST(test_pre_vote_reply_without_round_is_noop); /* Raft safety-invariant assertions for the PreVote API (task 6.1, RN-1) */ RUN_TEST(test_invariant_prevote_sequence_pure_no_persist);