From 510b566bbf98ca22809266232f50e0db72f6a603 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Mon, 13 Jul 2026 07:18:44 +0000 Subject: [PATCH] fix(ha): bug-1435 raft_lock serializes concurrent raft_state_t access MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit raft-drive-async's default-ON pool dispatch lets messages from different peer endpoints (two vote replies, or a vote reply racing a vote request) run concurrently on different vtask pool workers. raft_state_t's fields (votes_granted, replies_received, election_decided, term, role) are plain non-atomic — the raft library assumes a single-threaded caller. Two concurrent raft_on_vote_reply() calls could race: one reaches quorum and promotes to PRIMARY while the other, reading pre-increment vote count, independently satisfies "all-replied, no quorum" and fires on_become_follower for the same term, silently reverting the promotion — every request afterward correctly (but wrongly) rejected as STANDBY. Add ha->raft_lock (PTHREAD_MUTEX_RECURSIVE — a WON/quorum outcome can legitimately re-enter the state machine synchronously on the same thread, e.g. PreVote WON -> real election) around every raft_state_t entry point on the election/vote path. Verified via 40-round sequential and 40-round chaotic HA soak, zero committed-job loss, all 3 nodes led. --- src/controller/cluster.c | 15 +++++++- src/controller/ha.c | 83 ++++++++++++++++++++++++++++++++++++++-- src/controller/ha.h | 28 ++++++++++++++ 3 files changed, 121 insertions(+), 5 deletions(-) diff --git a/src/controller/cluster.c b/src/controller/cluster.c index 59a94021..e952ac6b 100644 --- a/src/controller/cluster.c +++ b/src/controller/cluster.c @@ -468,8 +468,15 @@ static int myreq_handler(vtask_t* task) uint64_t reply_term = 0; int granted = 0; if (freshness_ok && from_idx >= 0) { + /* bug-1435: serialize against concurrent raft_state_t access — + * this node's own vote-reply/election processing (a different + * peer connection, dispatched on a different pool worker) can + * race this vote REQUEST on the same ha->raft (see ha.h + * raft_lock doc). */ + pthread_mutex_lock(&c->ha.raft_lock); int r = raft_on_vote_request(&c->ha.raft, from_idx, cand_term, cand_llt, cand_lli, &reply_term); + pthread_mutex_unlock(&c->ha.raft_lock); granted = (r == RAFT_GRANT) ? 1 : 0; } else { /* Freshness deny or unknown candidate: read current term for reply */ @@ -565,9 +572,15 @@ static int myreq_handler(vtask_t* task) deny_reason = "unknown_candidate"; } else { /* raft_on_pre_vote_request is pure: stale-term + eligibility gate, - * no term/voted_for/role mutation, no persist. */ + * no term/voted_for/role mutation, no persist. Still reads + * raft_state_t's term/role — bug-1435: take raft_lock so this + * read cannot observe a torn (mid-transition) snapshot while a + * concurrent vote-reply/election-start call on another pool + * worker is mutating the same fields (see ha.h raft_lock doc). */ + pthread_mutex_lock(&c->ha.raft_lock); int r = raft_on_pre_vote_request(&c->ha.raft, from_idx, cand_term, cand_llt, cand_lli); + pthread_mutex_unlock(&c->ha.raft_lock); if (r == RAFT_GRANT) { granted = 1; } else { diff --git a/src/controller/ha.c b/src/controller/ha.c index 05b690f5..ea71ab32 100644 --- a/src/controller/ha.c +++ b/src/controller/ha.c @@ -545,7 +545,11 @@ static int raft_election_timeout_fn(void *arg) free(t); return 0; } + /* 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_on_election_timeout(&ha->raft); + pthread_mutex_unlock(&ha->raft_lock); free(t); return 0; } @@ -612,7 +616,11 @@ static void raft_cb_cancel_timeout(void *ctx) 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); return 0; } @@ -634,7 +642,9 @@ 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); return; } @@ -647,7 +657,9 @@ 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); } } @@ -2177,6 +2189,22 @@ int ha_init(ha_state_t *ha, const ha_config_t *cfg, int my_priority) /* failover_requested / watchdog_stop deleted — watchdog replaced by ha_arm_heartbeat_timeout_vtask */ pthread_mutex_init(&ha->lock, NULL); + /* bug-1435: dedicated lock serializing raft_state_t's election/vote entry + * points against concurrent pool-worker dispatch (see ha.h field doc). + * RECURSIVE: a WON/quorum outcome legitimately re-enters the state machine + * synchronously on the SAME thread (e.g. raft_on_pre_vote_reply's WON + * branch -> ha_enter_real_election -> ha_start_election_async -> + * raft_start_election, all before the outer call returns) — that is a + * same-thread call chain, not a race, and must not self-deadlock. A plain + * mutex would deadlock on that legitimate nesting; PTHREAD_MUTEX_RECURSIVE + * still fully serializes the DIFFERENT-thread case this lock exists to fix. */ + { + pthread_mutexattr_t raft_lock_attr; + pthread_mutexattr_init(&raft_lock_attr); + pthread_mutexattr_settype(&raft_lock_attr, PTHREAD_MUTEX_RECURSIVE); + pthread_mutex_init(&ha->raft_lock, &raft_lock_attr); + pthread_mutexattr_destroy(&raft_lock_attr); + } ha->ssl_ctx = NULL; ha->jitter_seed = 0; @@ -2360,6 +2388,7 @@ void ha_fini(ha_state_t *ha) ha_cancel_promotion_fence_timer(ha); pthread_mutex_destroy(&ha->lock); + pthread_mutex_destroy(&ha->raft_lock); sem_destroy(&ha->heartbeat_seen); /* Fix ASAN leak: free the SSL context if one was allocated */ @@ -5501,9 +5530,27 @@ static void vote_reply_dispatch(ha_state_t *ha, ha_elect_ctx_t *ctx, int idx, * never reach quorum on that peer's vote alone. ha_repl_ack_abs_peer_idx * (ha.c:3779) already implements the exact inverse of * raft_cb_send_vote_request's votes_idx remap and is unit-tested; reuse - * it here (ep_is_vote=1) rather than duplicating the arithmetic. */ + * it here (ep_is_vote=1) rather than duplicating the arithmetic. + * + * bug-1435: raft_on_vote_reply mutates raft_state_t's plain (non-atomic) + * votes_granted/replies_received/election_decided fields and can fire + * on_become_leader/on_become_follower synchronously. With raft-drive-async + * pool dispatch (default ON), a DIFFERENT peer's vote reply can be + * processed on a DIFFERENT pool worker concurrently — without raft_lock, + * two concurrent calls can race: one reaches quorum and promotes to + * PRIMARY, while the other, racing on the pre-increment vote count, + * independently satisfies the "all replied, no quorum" branch and + * immediately fires on_become_follower for the same term, silently + * reverting role to STANDBY right after promotion (verified data race — + * every request thereafter is correctly, faithfully rejected as STANDBY + * by ha_dispatch_gate, matching the observed symptom exactly). Hold + * raft_lock across the whole call so the state-machine evaluation and any + * 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); + pthread_mutex_unlock(&ha->raft_lock); } /* @@ -5755,8 +5802,13 @@ static int repl_ack_decode_and_dispatch(endpoint_t *ep, vote_ep_ctx_t *vctx, * instead of retrying AppendEntries against a follower that will * reject every entry forever. raft_on_higher_term_observed only * touches (term, voted_for, role) — never peer-indexed - * bookkeeping — so it is safe to call before peer_idx is resolved. */ + * bookkeeping — so it is safe to call before peer_idx is resolved. + * bug-1435: serialize against concurrent vote-reply/vote-request + * processing (see ha.h raft_lock doc) — this call can also fire + * on_become_follower, racing the same fields. */ + pthread_mutex_lock(&ha->raft_lock); (void)raft_on_higher_term_observed(&ha->raft, sr.term); + pthread_mutex_unlock(&ha->raft_lock); /* peer_idx from standby_ep_ctx_t (layout-compatible with vote_ep_ctx_t) */ int peer_idx = vctx->peer_idx; @@ -5852,8 +5904,12 @@ static int repl_ack_decode_and_dispatch(endpoint_t *ep, vote_ep_ctx_t *vctx, /* bug-1421: a WAL NACK can also carry a higher term (the * standby rejected this entry as stale) — route it through * the same term-discovery path as the heartbeat state report - * so the primary steps down instead of retrying forever. */ + * so the primary steps down instead of retrying forever. + * bug-1435: serialize against concurrent raft_state_t access + * (see ha.h raft_lock doc). */ + pthread_mutex_lock(&ha->raft_lock); (void)raft_on_higher_term_observed(&ha->raft, reply_term); + pthread_mutex_unlock(&ha->raft_lock); WARN("repl_ack_decode_and_dispatch: WAL NACK from standby" " lsn=%lu success=0 — ignoring", (unsigned long)acked_lsn); } @@ -6993,8 +7049,13 @@ void ha_start_prevote_async(ha_state_t *ha, cluster_t *c) 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). */ + * 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); 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 @@ -7051,7 +7112,14 @@ void ha_start_prevote_async(ha_state_t *ha, cluster_t *c) static void pre_vote_reply_dispatch(ha_state_t *ha, cluster_t *c, 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 + * ha_enter_real_election -> ... -> raft_start_election synchronously on + * 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(); @@ -7289,7 +7357,14 @@ void ha_start_election_async(ha_state_t *ha, cluster_t *c, ha_elect_ctx_release(ctx); 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); /* For single-node, raft_start_election fires on_become_leader synchronously. * on_become_leader (raft_cb_become_leader) handles role -> CTRL_PRIMARY and diff --git a/src/controller/ha.h b/src/controller/ha.h index 6cb14b8d..6c386915 100644 --- a/src/controller/ha.h +++ b/src/controller/ha.h @@ -196,6 +196,34 @@ typedef struct { int n_peers; /* total cluster size (including self), matches raft convention */ /* ── New fields added by raft-library spec (Task 6.1) ──────── */ raft_state_t raft; /* Raft consensus state (wired in task 6.2) */ + /* bug-1435 (raft-drive-async concurrency gap): raft_state_t's own fields + * (term/role/votes_granted/replies_received/election_decided/vote_replies[] + * etc.) are plain, non-atomic and have NO protection of their own — the + * Raft library assumes a single-threaded/serialized caller (the classic + * "single event-loop" consensus model). But raft-drive-async's default-ON + * pool dispatch (peer_ctrl_dispatch_to_pool, VTASK_LIVENESS) lets messages + * from DIFFERENT peer endpoints (e.g. two RequestVoteReply's from two + * different voters, or a vote reply racing a vote request) run + * CONCURRENTLY on different vtask pool workers — per-endpoint + * serialization (bind_vtask) only orders messages on the SAME endpoint, + * not across endpoints. Two concurrent raft_on_vote_reply() calls can + * race on votes_granted/replies_received/election_decided: e.g. one + * worker's grant reaches quorum and fires on_become_leader (role→PRIMARY) + * while a second worker, concurrently processing another peer's reply, + * reads pre-increment votes_granted and independently satisfies the + * "all-replied, no quorum" branch, firing on_become_follower (role→ + * STANDBY) an instant later for the SAME term — silently undoing the + * promotion with no further election. raft_lock serializes every mutating + * entry point into `raft` (election/vote-reply/vote-request path only — + * see call sites) so at most one thread evaluates the state machine at a + * time, matching the raft library's single-threaded design assumption. + * NOT the same lock as `lock` above (which protects unrelated fields) — + * kept separate so the two never nest in conflicting order. Initialized + * PTHREAD_MUTEX_RECURSIVE (ha_init): a WON/quorum outcome can legitimately + * re-enter the state machine synchronously on the SAME thread (PreVote WON + * -> 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; /* ── 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) */