myxce/src/controller/startup_validation.c
Rongsong Shen ac46bfa649
fix(controller): restored-but-stale raft term permanently blocked re-election after restart (bug-1695)
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
2026-08-25 16:11:37 +00:00

287 lines
12 KiB
C

/* startup_validation.c — Startup pre-flight validation functions.
*
* Implements validate_vcpus(), validate_startup_config(),
* 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>
#include <stdint.h>
#include <stdio.h>
#include <string.h>
#include <unistd.h>
#include "ha.h"
/* ── Req 4.3: --vcpus validation ─────────────────────────────────────── */
/*
* validate_vcpus: returns 0 if vcpus >= 1, -EINVAL otherwise.
* Called from main.c after argp_parse() sets arguments.vcpus.
*/
int validate_vcpus(int vcpus)
{
if (vcpus <= 0) {
return -EINVAL;
}
return 0;
}
/* ── Req 4.4: vcpus key in config file — warn only ───────────────────── */
/*
* validate_startup_config: scan config_path for a "vcpus" key.
* If found, write a deprecation warning into warn_buf (must contain "vcpus").
* Always returns 0 — the presence of a vcpus key is not a fatal error.
* warn_buf is left empty (first byte = '\0') when no vcpus key is found.
*
* The scan is line-based: a line contains the vcpus key if it starts with
* optional whitespace followed by the literal string "vcpus".
*/
int validate_startup_config(const char *config_path,
char *warn_buf, size_t warn_len)
{
if (warn_buf && warn_len > 0) {
warn_buf[0] = '\0';
}
if (!config_path) {
return 0;
}
FILE *fp = fopen(config_path, "r");
if (!fp) {
return 0; /* can't open — not a fatal error at this stage */
}
char line[1024];
while (fgets(line, sizeof(line), fp)) {
/* skip leading whitespace */
const char *p = line;
while (*p == ' ' || *p == '\t') {
p++;
}
if (strncmp(p, "vcpus", 5) == 0) {
/* Found a vcpus key — write deprecation warning */
if (warn_buf && warn_len > 0) {
snprintf(warn_buf, warn_len,
"config file contains deprecated 'vcpus' key; "
"use --vcpus CLI parameter instead");
}
break;
}
}
fclose(fp);
return 0;
}
/* ── Req 4.1: even-N peer count rejection ─────────────────────────────── */
/*
* validate_peer_count: returns 0 for valid odd counts (1, 3, 5, ...),
* -EINVAL for even counts > 1.
* n_peers == 1 is single-node mode — always valid.
* Even counts > 1 cannot form a safe majority partition.
*/
int validate_peer_count(int n_peers)
{
if (n_peers > 1 && (n_peers % 2) == 0) {
return -EINVAL;
}
return 0;
}
/* ── Req 4.2: TLS cert pre-flight ─────────────────────────────────────── */
/*
* validate_cert_files: check that cert_path and key_path are readable.
* On the first missing/unreadable file, sets err_buf to a message
* containing the full file path and returns -ENOENT.
* Returns 0 if both files are accessible.
*/
int validate_cert_files(const char *cert_path, const char *key_path,
char *err_buf, size_t err_len)
{
if (access(cert_path, R_OK) != 0) {
if (err_buf && err_len > 0) {
snprintf(err_buf, err_len,
"TLS certificate not found or unreadable: %s",
cert_path);
}
return -ENOENT;
}
if (access(key_path, R_OK) != 0) {
if (err_buf && err_len > 0) {
snprintf(err_buf, err_len,
"TLS key not found or unreadable: %s",
key_path);
}
return -ENOENT;
}
return 0;
}
/* ── Req 2.4: heartbeat timing validation ─────────────────────────────── */
/*
* validate_heartbeat_config: enforce heartbeat_timeout_ms >= 2 * heartbeat_interval_ms.
*
* A timeout smaller than two intervals means the standby could declare the
* primary dead before a single missed heartbeat has elapsed — that is an
* operator misconfiguration that must be caught at startup.
*
* Returns 0 when the relationship holds (valid config).
* Returns -EINVAL when heartbeat_timeout_ms < 2 * heartbeat_interval_ms;
* writes a human-readable error message into err_buf.
*/
int validate_heartbeat_config(uint32_t heartbeat_timeout_ms,
uint32_t heartbeat_interval_ms,
char *err_buf, size_t err_len)
{
if (err_buf && err_len > 0) {
err_buf[0] = '\0';
}
if (heartbeat_timeout_ms < 2 * heartbeat_interval_ms) {
if (err_buf && err_len > 0) {
snprintf(err_buf, err_len,
"heartbeat_timeout_ms (%u) must be >= 2 * heartbeat_interval_ms (%u); "
"current ratio %.2f is below the minimum of 2.0 — "
"set heartbeat_timeout_ms >= %u",
heartbeat_timeout_ms,
heartbeat_interval_ms,
heartbeat_interval_ms > 0
? (double)heartbeat_timeout_ms / (double)heartbeat_interval_ms
: 0.0,
2 * heartbeat_interval_ms);
}
return -EINVAL;
}
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;
}