wdgzyjx/myxce
1
0
Fork 0
forked from lei.chen/myxce
Code Pull requests Activity

fix(ha): bug-1435 raft_lock serializes concurrent raft_state_t access

Browse source 
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.
This commit is contained in:
Rongsong Shen 2026-07-13 07:18:44 +00:00
parent 976a6c956b
commit 510b566bbf
3 changed files with 121 additions and 5 deletions
Download patch file Download diff file Expand all files Collapse all files

15
src/controller/cluster.c
View file

@ -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 {

83
src/controller/ha.c
View file

@ -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

28
src/controller/ha.h
View file

@ -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) */