From e5e6cd18175085a9d5a144e935557b467d3cdd77 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Tue, 25 Aug 2026 07:49:49 +0000 Subject: [PATCH] fix(controller): raft_cb_persist must deny vote and roll back state on persist failure (bug-1684) ha_persist_term() had a void return -- every open/write/fsync/rename failure was silently swallowed ("best-effort; caller must not rely on this for correctness"). Its sole vote-granting caller, raft_cb_persist(), always returned 0 regardless, so the raft library always granted the vote even when the term/voted_for record never reached disk -- a real split-vote risk if the node then restarted and forgot it voted (Election Safety requires durably-persisted votedFor). ha_persist_term() now returns 0 on success (including the deliberate NULL-workdir test-mode skip) or -errno on a genuine I/O failure (fsync's previously-unchecked return is now checked too). raft_cb_persist() propagates that failure to the raft library (denying the vote per its own documented contract) and rolls back ha->voted_for_term/ ha->voted_for_name to their pre-call snapshot, so the adapter's in-memory state stays consistent with the library's own "deny without updating voted_for" behavior. ha->term is deliberately NOT rolled back: the raft library adopts a higher observed term unconditionally at Step 2 of raft_on_vote_request, before and independent of the vote-grant/persist outcome, so the adapter correctly mirrors that same semantics. Also corrects a stale claim in controller/CLAUDE.md's Protocol Verification Checklist (found by a sibling audit dimension the same session): term+voted_for were never written "atomically under ha->lock" -- term uses its own atomic store outside the lock. Found by an /xce-audit-invariants controller pass (error-handling dimension). 3 new tests in test_ha_durability.c drive the real production path (ha_init_raft + raft_on_vote_request, the exact function cluster.c's dispatch calls) against a forced ENOENT write failure; RED/GREEN independently verified. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01AgYRU3zDTqNGV5Jk8aiKLg --- src/controller/CLAUDE.md | 2 +- src/controller/ha.c | 72 ++++++++-- src/controller/ha.h | 10 +- src/controller/testsuite/test_ha_durability.c | 135 ++++++++++++++++++ 4 files changed, 205 insertions(+), 14 deletions(-) diff --git a/src/controller/CLAUDE.md b/src/controller/CLAUDE.md index e053e905..acd036b0 100644 --- a/src/controller/CLAUDE.md +++ b/src/controller/CLAUDE.md @@ -110,7 +110,7 @@ The `RequestVoteReply` message already carries `peerConfigTag` and `peerLastLogL ### Protocol Verification Checklist When modifying HA protocol code (election, replication, heartbeat), verify: -- [ ] `votedFor` is updated atomically with term increment before granting any vote (library: `raft_on_vote_request`, `src/lib/raft/raft.c:178-270` — term update before persist, `voted_for` set only after persist succeeds; adapter: `raft_cb_persist`, `ha.c:2257`, writes term+voted_for atomically under `ha->lock`. `handle_request_vote` in `ha.c` is `#ifdef XCE_TESTING`-only; production dispatch is `cluster.c:582` → `raft_on_vote_request`. Re-corrected 2026-08-08 — the `ha.c:1784-1824` and `cluster.c:477` citations from the prior 2026-07-18 correction had themselves drifted onto unrelated promotion/recovery-gate code.) +- [ ] `votedFor` is updated atomically with term increment before granting any vote (library: `raft_on_vote_request`, `src/lib/raft/raft.c:178-270` — term update before persist, `voted_for` set only after persist succeeds; adapter: `raft_cb_persist`, `ha.c:2257` — `ha->term` is its own atomic store (not under `ha->lock`; mirrors the library's own unconditional term adoption at Step 2, which is never rolled back regardless of vote outcome), `ha->voted_for_term`/`ha->voted_for_name` are written under `ha->lock` and, as of bug-1684, rolled back to their pre-call value if `ha_persist_term()` fails — the persist failure itself now propagates as a non-zero return so the library denies the vote (`raft_on_vote_request`'s Step 7 contract) instead of granting one with no durable record. `handle_request_vote` in `ha.c` is `#ifdef XCE_TESTING`-only; production dispatch is `cluster.c:583` → `raft_on_vote_request`. Re-corrected 2026-08-08 — the `ha.c:1784-1824` and `cluster.c:477` citations from the prior 2026-07-18 correction had themselves drifted onto unrelated promotion/recovery-gate code. Re-corrected 2026-08-25 — the "atomically under `ha->lock`" claim was itself inaccurate, found by an `/xce-audit-invariants` pass.) - [ ] `currentTerm` is monotonic — never decreases - [ ] `commitIndex` never decreases (monotonic) - [ ] A leader only commits entries from its current term (or equivalent via per-entry majority ack) diff --git a/src/controller/ha.c b/src/controller/ha.c index 928fbf80..32790b8c 100644 --- a/src/controller/ha.c +++ b/src/controller/ha.c @@ -2248,11 +2248,27 @@ static void raft_cb_become_follower(void *ctx, uint64_t term, int leader_idx) * raft_cb_persist: persist term + voted_for to disk before granting vote. * * Maps raft integer voted_for index to a candidate name using cluster config. - * Updates ha->term, ha->voted_for_term, ha->voted_for_name then calls + * Updates ha->term (its own atomic store, not under ha->lock -- mirrors the + * raft library's own unconditional term adoption in raft_on_vote_request's + * Step 2, which happens regardless of whether the vote itself is later + * granted or denied, so this is never rolled back here either), then updates + * ha->voted_for_term/ha->voted_for_name under ha->lock and calls * ha_persist_term(ha, workdir). * * voted_for == -1 means "no vote"; voted_for >= 0 is the candidate index. - * Returns 0 on success, non-zero on error (causes raft library to deny vote). + * Returns 0 on success, non-zero on error (causes raft library to deny vote, + * per raft_on_vote_request's Step 7 contract: "Persist failed — deny without + * updating voted_for"). + * + * bug-1684: on a genuine ha_persist_term() failure, the voted_for_term/ + * voted_for_name mutation above must be ROLLED BACK to its pre-call value -- + * otherwise this adapter's in-memory state would claim a vote was cast for a + * candidate the raft library itself denied and never durably recorded, + * diverging from the library's own voted_for (which correctly stays + * unchanged on a persist failure). A crash-and-restart after that divergence + * could let this node grant a second, conflicting vote in the same term -- + * the exact split-vote hazard Election Safety exists to prevent + * (controller/CLAUDE.md's TLA+ safety invariants). */ static int raft_cb_persist(void *ctx, uint64_t term, int voted_for) { @@ -2271,6 +2287,10 @@ static int raft_cb_persist(void *ctx, uint64_t term, int voted_for) /* Map voted_for index to name */ pthread_mutex_lock(&ha->lock); + uint64_t prev_voted_for_term = ha->voted_for_term; + char prev_voted_for_name[sizeof(ha->voted_for_name)]; + memcpy(prev_voted_for_name, ha->voted_for_name, sizeof(prev_voted_for_name)); + if (voted_for == -1) { ha->voted_for_term = 0; memset(ha->voted_for_name, 0, sizeof(ha->voted_for_name)); @@ -2295,7 +2315,19 @@ static int raft_cb_persist(void *ctx, uint64_t term, int voted_for) pthread_mutex_unlock(&ha->lock); - ha_persist_term(ha, workdir); + int rc = ha_persist_term(ha, workdir); + if (rc < 0) { + ERROR("raft_cb_persist: failed to durably persist term=%llu " + "voted_for=%d rc=%d -- denying vote, rolling back in-memory " + "vote state (bug-1684)", + (unsigned long long)term, voted_for, rc); + pthread_mutex_lock(&ha->lock); + ha->voted_for_term = prev_voted_for_term; + memcpy(ha->voted_for_name, prev_voted_for_name, + sizeof(ha->voted_for_name)); + pthread_mutex_unlock(&ha->lock); + return rc; + } return 0; } @@ -5168,13 +5200,20 @@ typedef struct { /* * ha_persist_term: atomically write ha_term_file_t to {workdir}/ha-term.bin. * Uses write-to-tmp + fsync + rename for durability (existing project pattern). - * If workdir is NULL, the persist is skipped (test mode or no workdir configured). + * If workdir is NULL, the persist is skipped (test mode or no workdir configured) + * -- returns 0, a deliberate no-op, not a failure. * Req 9.1: must be called before any vote grant or term increment is reported. + * Returns 0 on success (including the NULL-workdir skip); a negative errno on + * a genuine open/write/fsync/rename failure (bug-1684: this used to be a void + * return with every failure silently swallowed, so raft_cb_persist always told + * the raft library the vote was durably persisted even when it never reached + * disk -- a real split-vote risk if the node then restarted and forgot it + * voted). */ -void ha_persist_term(ha_state_t *ha, const char *workdir) +int ha_persist_term(ha_state_t *ha, const char *workdir) { if (!workdir) - return; /* test mode: skip file I/O */ + return 0; /* test mode: skip file I/O -- not a failure */ ha_term_file_t f; memset(&f, 0, sizeof(f)); @@ -5195,15 +5234,26 @@ void ha_persist_term(ha_state_t *ha, const char *workdir) int fd = open(tmp, O_WRONLY | O_CREAT | O_TRUNC, 0600); if (fd < 0) - return; /* best-effort; caller must not rely on this for correctness */ + return -errno; ssize_t written = write(fd, &f, sizeof(f)); - if (written == (ssize_t)sizeof(f)) - fsync(fd); + if (written != (ssize_t)sizeof(f)) { + int saved_errno = (written < 0) ? errno : EIO; + close(fd); + return -saved_errno; + } + + if (fsync(fd) < 0) { + int saved_errno = errno; + close(fd); + return -saved_errno; + } close(fd); - if (written == (ssize_t)sizeof(f)) - rename(tmp, path); + if (rename(tmp, path) < 0) + return -errno; + + return 0; } /* diff --git a/src/controller/ha.h b/src/controller/ha.h index c26b5c7b..1461043b 100644 --- a/src/controller/ha.h +++ b/src/controller/ha.h @@ -1302,9 +1302,15 @@ int ha_handle_snapshot_chunk(ha_state_t *ha, persist_t *p, * ha_persist_term: write ha-term.bin atomically to {workdir}/ha-term.bin. * Uses write + rename for durability (existing pattern in the codebase). * Must be called before any vote is granted or term is incremented. - * If workdir is NULL, the file write is skipped (safe for unit tests). + * If workdir is NULL, the file write is skipped (safe for unit tests) and + * this returns 0 -- that is a deliberate no-op, not a failure. + * Returns 0 on success (including the NULL-workdir skip); a negative errno + * on a genuine I/O failure (open/write/fsync/rename). Callers on the + * vote-granting path (raft_cb_persist) MUST propagate a non-zero return so + * the raft library denies the vote instead of granting one with no durable + * record (bug-1684). */ -void ha_persist_term(ha_state_t *ha, const char *workdir); +int ha_persist_term(ha_state_t *ha, const char *workdir); /* * ha_init_raft: initialize the embedded raft state machine with cluster topology. diff --git a/src/controller/testsuite/test_ha_durability.c b/src/controller/testsuite/test_ha_durability.c index 8af88b4e..cd9cb41f 100644 --- a/src/controller/testsuite/test_ha_durability.c +++ b/src/controller/testsuite/test_ha_durability.c @@ -34,6 +34,7 @@ #include "ha.h" #include "config.h" /* XCE_MAXNAME_LEN */ +#include "xce/cluster_def.h" /* cluster_t, for bug-1684's raft_on_vote_request test */ /* ── On-disk format (mirrors ha.c internals, duplicated here for white-box tests) ─ * @@ -381,6 +382,133 @@ static void test_persist_atomicity_tmp_cleaned_up(void) printf("test_persist_atomicity_tmp_cleaned_up: PASS\n"); } +/* ── Test 6: ha_persist_term() returns a real error on a genuine write + * failure (bug-1684) ───────────────────────────────────────────────────── + * + * Before this fix, ha_persist_term() had a void return -- every open/write/ + * fsync/rename failure was silently swallowed ("best-effort; caller must + * not rely on this for correctness"). Its sole vote-granting caller, + * raft_cb_persist(), always returned 0 to the raft library regardless, so a + * vote was granted even when the term/voted_for record never reached disk. + * + * Points workdir at a path whose PARENT directory does not exist, so + * open(O_CREAT) on the .tmp file deterministically fails ENOENT -- no + * permission-bit tricks needed (those misbehave when running as root, + * common in sandboxes), and no real filesystem quota/full-disk simulation + * required either. + */ +static void test_persist_term_returns_error_on_write_failure(void) +{ + ha_state_t ha; + ha_init(&ha, &g_durability_cfg, 0); + + atomic_store_explicit(&ha.term, 3ULL, memory_order_release); + ha.voted_for_term = 3; + strncpy(ha.voted_for_name, "ctrl-node-9", sizeof(ha.voted_for_name) - 1); + + const char *bad_workdir = + "/tmp/xce_ha_durability_test_nonexistent_parent_xyz/subdir"; + int rc = ha_persist_term(&ha, bad_workdir); + TEST_ASSERT_TRUE_MESSAGE( + rc < 0, + "ha_persist_term must return a negative errno when the write " + "genuinely fails (bug-1684)"); + + ha_fini(&ha); + printf("test_persist_term_returns_error_on_write_failure: PASS\n"); +} + +/* ── Test 7: NULL workdir stays a no-op SUCCESS, not folded into the new + * error path (bug-1684 regression guard) ──────────────────────────────── */ +static void test_persist_term_null_workdir_returns_success(void) +{ + ha_state_t ha; + ha_init(&ha, &g_durability_cfg, 0); + + int rc = ha_persist_term(&ha, NULL); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 0, rc, + "ha_persist_term(..., NULL) must return 0 -- test-mode skip is not " + "a failure"); + + ha_fini(&ha); + printf("test_persist_term_null_workdir_returns_success: PASS\n"); +} + +/* ── Test 8: a vote denied by a real persist failure leaves ha's in-memory + * voted_for state UNCHANGED (bug-1684) ───────────────────────────────────── + * + * Drives the REAL production path: ha_init_raft() wires raft_cb_persist as + * the raft library's on_persist callback (s_real_cbs, ha.c); the function + * under test, raft_on_vote_request() (raft.c, the pure library), is the + * exact function cluster.c's production dispatch calls. ha_init_raft(ha, + * NULL, n_peers) avoids needing a real persist_t (only used for + * initial_log_len/initial_commit_index when c is non-NULL) -- ha->raft_c is + * then set directly afterward, the same test-shim pattern ha.h's own + * raft_cb_become_leader_for_test/raft_cb_become_follower_for_test comments + * document ("this shim sets ha->raft_c = c first"). + * + * The fixture's myconf populates ONE real candidate name (index 1) so + * raft_cb_persist's voted_for_name lookup resolves to a genuine non-empty + * name -- not the self/empty fallback -- making the rollback assertion + * meaningful: before this fix, ha.voted_for_name/voted_for_term would show + * that candidate's vote despite the vote being denied and never durably + * recorded. + */ +static void test_vote_denied_and_state_rolled_back_on_persist_failure(void) +{ + ha_state_t ha; + ha_init(&ha, &g_durability_cfg, 0); + ha_init_raft(&ha, NULL, 2); + + myconf_t test_myconf; + memset(&test_myconf, 0, sizeof(test_myconf)); + test_myconf.workdir = + (char *)"/tmp/xce_ha_durability_test_nonexistent_parent_xyz/subdir"; + test_myconf.name = (char *)"self-node"; + test_myconf.peers_cfg.candidates_count = 2; + strncpy(test_myconf.peers_cfg.candidates[1].name, "candidate-1", + sizeof(test_myconf.peers_cfg.candidates[1].name) - 1); + + cluster_t test_c; + memset(&test_c, 0, sizeof(test_c)); + test_c.myconf = &test_myconf; + + /* Test shim: same pattern ha.h documents for the other raft_cb_*_for_test + * helpers -- set ha->raft_c directly rather than driving a full + * persist_t through ha_init_raft(). */ + ha.raft_c = &test_c; + + /* Baseline: no prior vote. */ + TEST_ASSERT_EQUAL_UINT64(0ULL, ha.voted_for_term); + TEST_ASSERT_EQUAL_STRING("", ha.voted_for_name); + + uint64_t reply_term = 0; + int result = raft_on_vote_request(&ha.raft, /*from_idx=*/1, /*term=*/5, + /*cand_last_log_term=*/0, + /*cand_last_log_index=*/0, &reply_term); + + TEST_ASSERT_EQUAL_INT_MESSAGE( + RAFT_DENY, result, + "vote must be DENIED when the term/vote persist genuinely fails " + "(bug-1684)"); + + /* voted_for state must be UNCHANGED -- the vote was never durably + * recorded, so this node must not believe it already voted. */ + TEST_ASSERT_EQUAL_UINT64_MESSAGE( + 0ULL, ha.voted_for_term, + "voted_for_term must roll back to its pre-call value on persist " + "failure (bug-1684)"); + TEST_ASSERT_EQUAL_STRING_MESSAGE( + "", ha.voted_for_name, + "voted_for_name must roll back to its pre-call value on persist " + "failure (bug-1684)"); + + ha_fini(&ha); + printf("test_vote_denied_and_state_rolled_back_on_persist_failure: " + "PASS\n"); +} + /* ── Test suite entry point ───────────────────────────────────────────────── */ void test_ha_durability(void) @@ -400,4 +528,11 @@ void test_ha_durability(void) /* Req 9.1: write+rename atomicity — no .tmp artifact left */ RUN_TEST(test_persist_atomicity_tmp_cleaned_up); + + /* bug-1684: ha_persist_term() must return a real error on write failure, + * NULL-workdir must stay a no-op success, and raft_cb_persist() must + * deny the vote AND roll back in-memory voted_for state on failure. */ + RUN_TEST(test_persist_term_returns_error_on_write_failure); + RUN_TEST(test_persist_term_null_workdir_returns_success); + RUN_TEST(test_vote_denied_and_state_rolled_back_on_persist_failure); }