lei.chen/myxce
1
0
Fork 1
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix(controller): restored-but-stale raft term permanently blocked re-election after restart (bug-1695)

Browse source 
Found via the mandatory live HA chaotic soak for this session's controller
audit campaign -- unrelated to any of the 9 audit fixes (bug-1684/1687/
1689/1690/1691/1692/1693/1694), all in ha.c/persist.c/job.c/cluster.c/
sched.c/worker.c; this is in myapp.c, untouched by any of them.

myapp_run()'s bootstrap "is a leader already known" check had a fallback
`c.ha.raft.term > 0` meant to detect genuine new raft activity processed
during the startup-stagger nap. But ha_restore_term() loads ha->term from
disk BEFORE this check runs, on every boot -- and Raft terms only ever
increase, never reset. So `raft.term > 0` is trivially true on every
restart for the rest of the cluster's on-disk lifetime, once it has ever
held one election, regardless of whether any leader is currently
reachable. Live-reproduced: every node, on every restart after the
cluster's first election, unconditionally skipped ha_start_election_async
forever -- no node could ever re-assert leadership through a real
election again. bug-1477's existing cluster_init_phase2 retry loop kept
firing correctly (confirmed in the reproduction logs) but could never
succeed, since the deeper cause -- no election ever runs to produce a
live primary to push config from -- was never addressed by that fix.

Fixed by snapshotting `term_at_boot` right after ha_restore_term()/
ha_init_raft() seed the term, strictly before the stagger nap, and
comparing `raft.term > term_at_boot` instead of `raft.term > 0` -- only a
term that has genuinely ADVANCED during this boot cycle (real vote/
heartbeat traffic processed) counts as evidence a leader is known. The
decision was extracted into a new, unit-testable myapp_leader_already_known()
(startup_validation.c, since controller_unit_test deliberately excludes
myapp.c/main.c). voted_for_name/vft==cur and primary_ctrl_fd>=0 checks
are unchanged.

7 new tests in test_ha_startup.c; RED/GREEN independently verified.
Rigorous A/B live-soak proof (identical 6-round --chaotic run, fixed vs
git-stashed-back-to-original): baseline showed term frozen for the whole
run and zero election activity on any non-primary node; fixed run showed
term genuinely progressing and ctrl-2 achieving a real, log-verified
CTRL_PRIMARY transition via a won PreVote round -- proof a node other
than the original primary can now take over, which was structurally
impossible before. Full controller suite: 1191 tests, 2 failures -- both
the pre-existing, unrelated bug-1561 overlayfs sandbox flake.

Residual, NOT independently verified yet (flagged honestly, not fixed
here): the fixed run's chaotic verdict showed committed_job_loss=6
(baseline 0) -- traced via waldump to one jobid missing from every node's
WAL, at the cold-boot window before round 1. Resembles the already-fixed
bug-1487 false-positive class (apply-pump-lag racing verify_classify's
expect-job check) but the reported mechanism doesn't perfectly match
(bug-1487's job was present-in-WAL/not-yet-visible; this one is described
as absent from every WAL entirely) -- needs independent re-verification
before being dismissed, tracked as a follow-up.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AgYRU3zDTqNGV5Jk8aiKLg
This commit is contained in:
Rongsong Shen 2026-08-25 16:11:37 +00:00
parent 23c5c26a23
commit ac46bfa649
No known key found for this signature in database
GPG key ID: 5BCDAAE0123E9FE2
4 changed files with 792 additions and 444 deletions
Download patch file Download diff file Expand all files Collapse all files

125
src/controller/startup_validation.c
View file

@ -1,13 +1,19 @@
/* startup_validation.c — Startup pre-flight validation functions.
*
* Implements validate_vcpus(), validate_startup_config(),
* validate_peer_count(), validate_cert_files(), and
* validate_heartbeat_config() for use in main.c and the controller
* unit test suite.
* validate_peer_count(), validate_cert_files(),
* validate_heartbeat_config(), and myapp_leader_already_known() for use in
* main.c/myapp.c and the controller unit test suite.
*
* Requirements: 4.1 (peer count), 4.2 (cert pre-flight),
* 4.3 (vcpus), 4.4 (vcpus key deprecation warn),
* 2.4 (heartbeat timing validation)
*
* myapp_leader_already_known lives here (not myapp.c) purely so it links
* into controller_unit_test — meson's controller_unit_test_src does not
* build myapp.c (myapp_run is an untestable integration entry point
* requiring a live network socket), but it already builds this file for
* the other validate_* helpers above (bug-1695).
*/
#define _GNU_SOURCE
#include <errno.h>
@ -16,6 +22,8 @@
#include <string.h>
#include <unistd.h>
#include "ha.h"
/* ── Req 4.3: --vcpus validation ─────────────────────────────────────── */
/*
@ -166,3 +174,114 @@ int validate_heartbeat_config(uint32_t heartbeat_timeout_ms,
return 0;
}
/* ── bug-1695: myapp_run's "leader already known" bootstrap check ───────── */
/*
* myapp_leader_already_known — bootstrap "is a leader already known" decision
* (bug-1695), extracted from myapp_run()'s post-stagger check so it is unit
* testable (myapp_run itself is an untestable integration entry point that
* requires a live network socket — see test_ha_startup.c's Task 5.1 note).
*
* Returns 1 (skip this node's own election) when ANY of:
*
* (a) ha->voted_for_name records a vote for a DIFFERENT candidate cast in
* the CURRENT term (voted_for_term == ha->term). This node granted a
* vote to a real candidate, so it is a follower, not unaware of any
* candidacy. A self-vote (own name, or the test-mode "self"
* placeholder) does NOT count — a self-vote means THIS node was the
* candidate, so no external leader is known.
*
* (b) ha->raft.term has genuinely ADVANCED past term_at_boot — the value
* the caller must capture immediately after ha_init_raft() seeds
* ha->raft.term from the just-restored on-disk term
* (ha_restore_term(), called from cluster_init_phase1 before
* ha_init_raft), strictly BEFORE the startup-stagger nap. Only an
* advance DURING this boot's nap — real vote-request/heartbeat
* traffic actually processed by iomgr, via
* raft_on_vote_request's/raft_on_append_entries_request's UpdateTerm
* steps — counts as evidence a leader is known. A merely-nonzero
* RESTORED term tells us nothing about whether any leader is
* currently reachable: Raft terms only ever increase, so comparing
* against a bare 0 (the pre-bug-1695 check) was trivially true on
* every restart after the cluster's first-ever election, permanently
* short-circuiting every later election attempt.
*
* (c) primary_ctrl_fd is already connected — the winner's ctrlhello has
* already established the control channel.
*
* bug-1695 verification notes (see myapp_run's caller-side comment for the
* companion snapshot-timing argument):
*
* - First-ever-boot case: ha_restore_term() leaves ha->term (and therefore
* the seeded ha->raft.term) at 0 when no ha-term.bin exists yet, so
* term_at_boot == 0 — identical to the old `raft.term > 0` check for a
* first boot. Preserved.
*
* - No other path can advance ha->raft.term between the caller's snapshot
* and this check in a way that would be wrongly treated as "stale": the
* only writers of raft.term after ha_init_raft() are raft_cb_persist
* (fired from raft_on_vote_request / raft_on_append_entries_request /
* raft_start_election), all of which only run in response to genuine
* incoming or self-initiated raft traffic processed by iomgr during the
* nap — exactly the signal this check intends to detect.
*
* - This does not introduce a new "spurious own election while a leader is
* genuinely live but not yet observed" failure mode beyond what already
* exists: that exact race (nap elapsed, term not yet advanced, primary_
* ctrl_fd not yet connected) is the SAME one the cluster's first-ever
* boot already runs through today (term_at_boot is always 0 pre-first-
* election, so `raft.term > term_at_boot` was already the operative
* comparison then) — this fix merely re-applies that already-proven
* first-boot behavior to every later restart too, instead of only the
* first one. Any resulting term churn is ordinary, tolerated Raft
* UpdateTerm behavior (a live primary that sees a higher-term
* RequestVote steps down and the cluster re-converges); it does not
* weaken Election Safety, which rests entirely on votedFor + majority
* inside the raft library, not on this advisory bootstrap check.
*
* - The voted_for_name/vft==cur check (a) is deliberately left untouched.
* It is NOT obviously immune to the same "restored state is
* indistinguishable from fresh state" class of question — a restored
* (voted_for_term, term) pair is just as internally consistent as a
* live one, since both are written together by the same on_persist
* call. Whether that residual staleness is DEFECTIVE, though, hinges
* on the separate liveness-watchdog/PreVote-failover subsystem (out of
* this fix's scope) reliably recovering a standby that skips via this
* path while its remembered leader is actually dead — that subsystem is
* untouched here and evaluated empirically via this fix's live soak,
* not modified speculatively.
*
* Locking: ha->lock guards voted_for_name/voted_for_term (same discipline as
* the original inline check); ha->term is read atomically; ha->raft.term and
* primary_ctrl_fd are read without a lock, matching the pre-existing
* (unlocked) read discipline at this call site — both are advisory reads for
* a bootstrap fast-path decision, not a safety-load-bearing one: a false
* negative here only costs a redundant, safely-arbitrated election.
*/
int myapp_leader_already_known(ha_state_t *ha, const char *my_name,
uint64_t term_at_boot)
{
int leader_known = 0;
pthread_mutex_lock(&ha->lock);
uint64_t vft = ha->voted_for_term;
uint64_t cur = atomic_load_explicit(&ha->term, memory_order_relaxed);
if (ha->voted_for_name[0] != '\0' &&
strcmp(ha->voted_for_name, "self") != 0 &&
strcmp(ha->voted_for_name, my_name) != 0 &&
vft == cur)
leader_known = 1;
pthread_mutex_unlock(&ha->lock);
/* bug-1695 fix: require a genuine ADVANCE past the caller's pre-nap
* snapshot, not merely a nonzero restored term (see the function-level
* doc comment above for the full rationale). */
if (!leader_known && ha->raft.term > term_at_boot)
leader_known = 1;
if (!leader_known && ha->primary_ctrl_fd >= 0)
leader_known = 1;
return leader_known;
}