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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AgYRU3zDTqNGV5Jk8aiKLg
This commit is contained in:
parent
5441d1b8e6
commit
e5e6cd1817
4 changed files with 205 additions and 14 deletions
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
}
|
||||
|
||||
/*
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue