From 195f660f9a823df3f2e3bf295bddcfe4467fdbcf Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Sun, 21 Jun 2026 23:27:12 +0000 Subject: [PATCH] feat(raft-commit-safety): raft_advance_commit Figure-8 current-term rule (tasks 3.1-3.3, 6.1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AdvanceCommitIndex (raft.tla:259/270, closes G4): leader-only (-EPERM); Max(agreeIndexes) = highest N with a quorum ((n_peers/2)+1) of match_index>=N, self counted as log_len; scan log_len downward. CURRENT-TERM RULE: advance commit_index to N only when on_get_entry_term(N)==s->term, else HOLD (Figure-8 safety — never commit a previous-term entry on replication count alone; a later current-term commit indirectly commits it). Monotonic; fires on_commit_advanced once per strict advance; commit_index<=log_len. 3.3: raft_on_append_entries_reply left untouched — verified it never advances commit (separation, Req 2.6). 6.1: sim commit-driver helper raft_sim_advance_commit added. Req 6.3 two-phase indirect-commit transition covered end-to-end by task 7.1 (sim). ASAN green (230 tests). --- .kiro/specs/raft-commit-safety/tasks.md | 8 +- src/lib/raft/raft.c | 54 +++++ src/lib/raft/raft.h | 12 ++ src/lib/raft/raft_sim.c | 11 + src/lib/testsuite/test_raft.c | 269 ++++++++++++++++++++++++ 5 files changed, 350 insertions(+), 4 deletions(-) diff --git a/.kiro/specs/raft-commit-safety/tasks.md b/.kiro/specs/raft-commit-safety/tasks.md index 66429df5..f38b0770 100644 --- a/.kiro/specs/raft-commit-safety/tasks.md +++ b/.kiro/specs/raft-commit-safety/tasks.md @@ -44,7 +44,7 @@ ## 3. AdvanceCommitIndex with the current-term Figure-8 rule (raft.tla:259/270, closes G4) -- [ ] 3.1 RED: commit-advance tests (current-term advance, Figure-8 hold, monotonic, non-leader) +- [x] 3.1 RED: commit-advance tests (current-term advance, Figure-8 hold, monotonic, non-leader) - Test (advance): leader with `match_index[]` quorum-agreeing through index `k`, entry at `k` from `currentTerm` → `commit_index` advances to `k`, `on_commit_advanced(k)` fired once - Test (Figure-8 hold): quorum agrees through a previous-term entry at `k` with no current-term entry above → `commit_index` unchanged, `on_commit_advanced` NOT fired - Test (empty agreeIndexes): no index reaches quorum → unchanged, no callback @@ -55,7 +55,7 @@ - _Boundary: test_raft.c_ - _Depends: 2.1_ -- [ ] 3.2 GREEN: implement `raft_advance_commit` +- [x] 3.2 GREEN: implement `raft_advance_commit` - Leader-only guard (`-EPERM` otherwise); compute `Max(agreeIndexes)` by scanning candidate indexes from `log_len` downward, seeding the agree-count with self, until a quorum `((n_peers/2)+1)` is reached - Apply the current-term rule: advance to `Max(agreeIndexes)` only when `on_get_entry_term(Max) == s->term`; else hold - Monotonic: only ever move `commit_index` forward; fire `on_commit_advanced(commit_index)` exactly once on a strict advance; read entry terms only via `on_get_entry_term` @@ -64,7 +64,7 @@ - _Boundary: raft.c_ - _Depends: 3.1_ -- [ ] 3.3 RED+GREEN: commit-advance is a separate action from append-entries reply +- [x] 3.3 RED+GREEN: commit-advance is a separate action from append-entries reply - Test: a `raft_on_append_entries_reply` that updates `match_index[]` to a committable quorum does NOT by itself change `commit_index` or fire `on_commit_advanced`; only a subsequent `raft_advance_commit` does - Verify (no implementation change expected in `raft_on_append_entries_reply` — it is owned upstream and must stay untouched); if a regression is found, the fix belongs upstream, not here - Observable completion: test passes; `raft_on_append_entries_reply` remains free of any commit-advance side effect @@ -109,7 +109,7 @@ ## 6. Simulator backing: commit-advance driver + freshness plumbing -- [ ] 6.1 Wire `on_commit_advanced` and a commit-advance driver in `raft_sim.c` +- [x] 6.1 Wire `on_commit_advanced` and a commit-advance driver in `raft_sim.c` - Add a per-`raft_sim_node_t` `commit_index` mirror updated from `on_commit_advanced`; implement the callback (record the value + count fires for assertions) - Add a sim helper to drive `raft_advance_commit` on a given node (or all leaders) after delivery - Pass each node's restored `initial_commit_index` (default 0) in `raft_sim_init`; wire `on_commit_advanced` for every node diff --git a/src/lib/raft/raft.c b/src/lib/raft/raft.c index a58b414e..b89348ea 100644 --- a/src/lib/raft/raft.c +++ b/src/lib/raft/raft.c @@ -687,6 +687,60 @@ int raft_on_append_entries_request(raft_state_t *s, int leader_idx, } } +int raft_advance_commit(raft_state_t *s) +{ + /* Leader-only (raft.tla AdvanceCommitIndex is enabled only at the leader). + * Off the leader: -EPERM, no change, no callback. */ + if (s->role != RAFT_LEADER) + return -EPERM; + + /* Compute Max(agreeIndexes): the highest index N (1..log_len) for which a + * QUORUM of the cluster has match_index >= N. Self is treated as having all + * its own entries (match_index == log_len), so it counts toward every + * candidate N <= log_len. Scan from log_len downward and stop at the first N + * that reaches quorum (that N is the maximum, since lower indexes are + * implied by every node that agrees on a higher one). (raft.tla 259/262.) */ + int quorum = (s->n_peers / 2) + 1; + uint64_t agreed = 0; /* 0 = no index reached quorum */ + for (uint64_t n = s->log_len; n >= 1; n--) { + int count = 1; /* self always has index n (n <= log_len) */ + for (int j = 0; j < s->n_peers; j++) { + if (j == s->self_idx) + continue; + if (s->match_index[j] >= n) + count++; + } + if (count >= quorum) { + agreed = n; + break; + } + } + + /* No index reached quorum → nothing to commit. */ + if (agreed == 0) + return RAFT_OK; + + /* Current-term Figure-8 rule (raft.tla 270): advance commit ONLY when the + * entry at the agreed index is from the CURRENT term. A leader must never + * commit a previous-term entry on replication count alone — doing so risks + * the Figure-8 safety violation where a committed entry is later + * overwritten. */ + if (s->cbs->on_get_entry_term(s->ctx, agreed) != s->term) + return RAFT_OK; + + /* Monotonic: never lower commit_index; only a strict advance fires the + * callback (and exactly once). */ + if (agreed <= s->commit_index) + return RAFT_OK; + + s->commit_index = agreed; + s->cbs->on_commit_advanced(s->ctx, s->commit_index); + if (s->cbs->on_trace) + s->cbs->on_trace(s->ctx, "commit_advanced", NULL); + + return RAFT_OK; +} + int raft_on_append_entries_reply(raft_state_t *s, int peer_idx, int success, uint64_t match_index, uint64_t reply_term) { diff --git a/src/lib/raft/raft.h b/src/lib/raft/raft.h index 9908cfb4..37748527 100644 --- a/src/lib/raft/raft.h +++ b/src/lib/raft/raft.h @@ -565,6 +565,18 @@ void raft_sim_partition(raft_sim_cluster_t *c, int node_idx, int isolated); */ int raft_sim_check_log_matching(const raft_sim_cluster_t *c); +/* + * raft_sim_advance_commit: drive raft_advance_commit on node_idx (task 6.1). + * + * Recomputes the node's commit index from its current match_index[] / log_len / + * term (the leader-only AdvanceCommitIndex + current-term Figure-8 rule). On a + * strict advance the node's on_commit_advanced fires, updating the per-node + * commit_index mirror and commit_advanced_count. No-op if node_idx is out of + * range. Off-leader nodes return -EPERM internally (the return is discarded; + * tests read the per-node mirror). + */ +void raft_sim_advance_commit(raft_sim_cluster_t *c, int node_idx); + /* * raft_sim_restart_node: model a process restart of node_idx (Req 8.3). * diff --git a/src/lib/raft/raft_sim.c b/src/lib/raft/raft_sim.c index b050ec60..15d0d039 100644 --- a/src/lib/raft/raft_sim.c +++ b/src/lib/raft/raft_sim.c @@ -389,6 +389,17 @@ void raft_sim_partition(raft_sim_cluster_t *c, int node_idx, int isolated) c->nodes[node_idx].partition = isolated; } +void raft_sim_advance_commit(raft_sim_cluster_t *c, int node_idx) +{ + if (node_idx < 0 || node_idx >= c->n) + return; + /* Library decides whether the commit index advanced; on a strict advance the + * node's on_commit_advanced (sim_on_commit_advanced) updates commit_index / + * commit_advanced_count. The -EPERM off-leader return is intentionally + * discarded — tests read the per-node mirror, not this status. */ + (void)raft_advance_commit(&c->nodes[node_idx].state); +} + /* Two log entries are identical iff their term, length, and payload bytes * (over the recorded length) all match. */ static int sim_entry_identical(const raft_sim_entry_t *a, diff --git a/src/lib/testsuite/test_raft.c b/src/lib/testsuite/test_raft.c index efe52436..5d76a441 100644 --- a/src/lib/testsuite/test_raft.c +++ b/src/lib/testsuite/test_raft.c @@ -2670,6 +2670,267 @@ static void test_ae_reply_stale_term_ignored(void) TEST_ASSERT_EQUAL_UINT64(match_before, s.match_index[1]); } +/* ---------- raft_advance_commit tests (task 3.1/3.2/3.3, raft.tla + * AdvanceCommitIndex 259/270 — current-term Figure-8 rule) ---------- */ + +/* Spy on_commit_advanced: records the last reported commit_index and a fire + * counter so tests can assert the strict-advance / fire-once semantics (the + * library fires this ONLY on a strict commit_index advance). */ +typedef struct { + int call_count; + uint64_t last_commit_index; +} commit_advanced_spy_t; + +static commit_advanced_spy_t g_ca_spy; + +static void reset_ca_spy(void) +{ + memset(&g_ca_spy, 0, sizeof(g_ca_spy)); +} + +static void spy_commit_advanced(void *ctx, uint64_t commit_index) +{ + (void)ctx; + g_ca_spy.call_count++; + g_ca_spy.last_commit_index = commit_index; +} + +/* + * test_advance_commit_advances_current_term (Req 1.2-1.7, 2.2, 2.3) + * + * Leader, term=2, log_len=3. A quorum (self + peer 1 of a 3-node cluster) agrees + * through index 3, and the entry at index 3 is from the CURRENT term (2). The + * commit index must advance to 3 and on_commit_advanced(3) must fire exactly once. + */ +static void test_advance_commit_advances_current_term(void) +{ + raft_state_t s; + raft_callbacks_t cbs = make_mock_cbs(); + cbs.on_get_entry_term = spy_get_entry_term; + cbs.on_commit_advanced = spy_commit_advanced; + + reset_mock(); + reset_gt_spy(); + reset_ca_spy(); + TEST_ASSERT_EQUAL_INT(0, init_node_log_len(&s, &cbs, 3, 0, 0)); + + /* Hand-set leader state directly (do not exercise the election path). */ + s.role = RAFT_LEADER; + s.term = 2; + s.log_len = 3; + /* every entry 1..3 is from the current term 2 */ + g_gt_spy.term_at[1] = 2; + g_gt_spy.term_at[2] = 2; + g_gt_spy.term_at[3] = 2; + /* peer 1 has replicated through index 3 (self counts as log_len=3) → quorum (2 of 3) */ + s.match_index[1] = 3; + s.match_index[2] = 0; + + int rc = raft_advance_commit(&s); + + TEST_ASSERT_EQUAL_INT(RAFT_OK, rc); + TEST_ASSERT_EQUAL_UINT64(3, s.commit_index); + TEST_ASSERT_EQUAL_INT(1, g_ca_spy.call_count); + TEST_ASSERT_EQUAL_UINT64(3, g_ca_spy.last_commit_index); +} + +/* + * test_advance_commit_figure8_hold_previous_term (Req 1.2-1.7, 2.2, 2.3) + * + * Figure-8 safety: a quorum agrees through index 2, but the entry at index 2 is + * from a PREVIOUS term (1) while the leader's current term is 3, and no + * current-term entry sits above it that also has quorum. The commit index must + * NOT advance and on_commit_advanced must NOT fire. + */ +static void test_advance_commit_figure8_hold_previous_term(void) +{ + raft_state_t s; + raft_callbacks_t cbs = make_mock_cbs(); + cbs.on_get_entry_term = spy_get_entry_term; + cbs.on_commit_advanced = spy_commit_advanced; + + reset_mock(); + reset_gt_spy(); + reset_ca_spy(); + TEST_ASSERT_EQUAL_INT(0, init_node_log_len(&s, &cbs, 3, 0, 0)); + + s.role = RAFT_LEADER; + s.term = 3; + s.log_len = 3; + /* entries 1,2 are from a previous term (1); entry 3 (current term) is NOT + * yet replicated to a quorum. */ + g_gt_spy.term_at[1] = 1; + g_gt_spy.term_at[2] = 1; + g_gt_spy.term_at[3] = 3; + /* peer 1 agrees only through index 2 (a previous-term entry); self has 3. */ + s.match_index[1] = 2; + s.match_index[2] = 0; + + int rc = raft_advance_commit(&s); + + TEST_ASSERT_EQUAL_INT(RAFT_OK, rc); + TEST_ASSERT_EQUAL_UINT64(0, s.commit_index); /* held — not advanced over prev-term */ + TEST_ASSERT_EQUAL_INT(0, g_ca_spy.call_count); +} + +/* + * test_advance_commit_no_quorum (Req 1.2-1.7, 2.2, 2.3) + * + * No index reaches quorum (a 3-node cluster where no peer has replicated any + * entry; only self agrees). agreeIndexes is empty → commit_index unchanged, no + * callback. + */ +static void test_advance_commit_no_quorum(void) +{ + raft_state_t s; + raft_callbacks_t cbs = make_mock_cbs(); + cbs.on_get_entry_term = spy_get_entry_term; + cbs.on_commit_advanced = spy_commit_advanced; + + reset_mock(); + reset_gt_spy(); + reset_ca_spy(); + TEST_ASSERT_EQUAL_INT(0, init_node_log_len(&s, &cbs, 3, 0, 0)); + + s.role = RAFT_LEADER; + s.term = 2; + s.log_len = 3; + g_gt_spy.term_at[1] = 2; + g_gt_spy.term_at[2] = 2; + g_gt_spy.term_at[3] = 2; + /* no peer has replicated anything → only self agrees (1 of 3 — not quorum) */ + s.match_index[1] = 0; + s.match_index[2] = 0; + + int rc = raft_advance_commit(&s); + + TEST_ASSERT_EQUAL_INT(RAFT_OK, rc); + TEST_ASSERT_EQUAL_UINT64(0, s.commit_index); + TEST_ASSERT_EQUAL_INT(0, g_ca_spy.call_count); +} + +/* + * test_advance_commit_monotonic (Req 1.2-1.7, 2.2, 2.3) + * + * A later call that computes a SMALLER Max(agreeIndexes) must never lower + * commit_index. First advance to 3, then drop a peer's match_index so the newly + * computed Max is 1 (< 3): commit_index stays 3 and no second callback fires. + */ +static void test_advance_commit_monotonic(void) +{ + raft_state_t s; + raft_callbacks_t cbs = make_mock_cbs(); + cbs.on_get_entry_term = spy_get_entry_term; + cbs.on_commit_advanced = spy_commit_advanced; + + reset_mock(); + reset_gt_spy(); + reset_ca_spy(); + TEST_ASSERT_EQUAL_INT(0, init_node_log_len(&s, &cbs, 3, 0, 0)); + + s.role = RAFT_LEADER; + s.term = 2; + s.log_len = 3; + g_gt_spy.term_at[1] = 2; + g_gt_spy.term_at[2] = 2; + g_gt_spy.term_at[3] = 2; + s.match_index[1] = 3; + s.match_index[2] = 0; + + /* first advance → commit_index = 3 */ + TEST_ASSERT_EQUAL_INT(RAFT_OK, raft_advance_commit(&s)); + TEST_ASSERT_EQUAL_UINT64(3, s.commit_index); + TEST_ASSERT_EQUAL_INT(1, g_ca_spy.call_count); + + /* now the peer regresses to index 1; new Max(agreeIndexes) = 1 < 3 */ + s.match_index[1] = 1; + TEST_ASSERT_EQUAL_INT(RAFT_OK, raft_advance_commit(&s)); + TEST_ASSERT_EQUAL_UINT64(3, s.commit_index); /* never lowered */ + TEST_ASSERT_EQUAL_INT(1, g_ca_spy.call_count); /* no second fire */ +} + +/* + * test_advance_commit_non_leader_eperm (Req 1.2-1.7, 2.2, 2.3) + * + * Off the leader, raft_advance_commit returns -EPERM, makes no change to + * commit_index, and fires no callback. + */ +static void test_advance_commit_non_leader_eperm(void) +{ + raft_state_t s; + raft_callbacks_t cbs = make_mock_cbs(); + cbs.on_get_entry_term = spy_get_entry_term; + cbs.on_commit_advanced = spy_commit_advanced; + + reset_mock(); + reset_gt_spy(); + reset_ca_spy(); + TEST_ASSERT_EQUAL_INT(0, init_node_log_len(&s, &cbs, 3, 0, 0)); + + s.role = RAFT_FOLLOWER; + s.term = 2; + s.log_len = 3; + g_gt_spy.term_at[1] = 2; + g_gt_spy.term_at[2] = 2; + g_gt_spy.term_at[3] = 2; + s.match_index[1] = 3; + s.match_index[2] = 3; + + int rc = raft_advance_commit(&s); + + TEST_ASSERT_EQUAL_INT(-EPERM, rc); + TEST_ASSERT_EQUAL_UINT64(0, s.commit_index); + TEST_ASSERT_EQUAL_INT(0, g_ca_spy.call_count); +} + +/* + * test_ae_reply_does_not_advance_commit (task 3.3, Req 2.6) + * + * Commit-advance is SEPARATE from the AppendEntries-reply handler. A + * raft_on_append_entries_reply that updates match_index[] to a committable quorum + * must NOT, by itself, change commit_index or fire on_commit_advanced. Only a + * subsequent raft_advance_commit advances the commit index. + * + * NOTE: raft_on_append_entries_reply is owned upstream (raft-log-replication) and + * is NOT modified by this spec; this test only asserts that contract holds. + */ +static void test_ae_reply_does_not_advance_commit(void) +{ + raft_state_t s; + raft_callbacks_t cbs = make_mock_cbs(); + cbs.on_get_entry_term = spy_get_entry_term; + cbs.on_commit_advanced = spy_commit_advanced; + + reset_mock(); + reset_gt_spy(); + reset_ca_spy(); + /* restored log_len = 3 → after drive_to_leader next_index[peer]=4 */ + TEST_ASSERT_EQUAL_INT(0, init_node_log_len(&s, &cbs, 3, 0, 3)); + drive_to_leader(&s); + + /* all 3 entries are from the leader's current term → would be committable */ + g_gt_spy.term_at[1] = s.term; + g_gt_spy.term_at[2] = s.term; + g_gt_spy.term_at[3] = s.term; + + uint64_t commit_before = s.commit_index; + + /* a success reply moving peer 1 to a committable quorum index */ + int rc = raft_on_append_entries_reply(&s, /*peer_idx=*/1, /*success=*/1, + /*match_index=*/3, /*reply_term=*/s.term); + TEST_ASSERT_EQUAL_INT(RAFT_OK, rc); + TEST_ASSERT_EQUAL_UINT64(3, s.match_index[1]); /* match recorded */ + + /* the reply ALONE must not move commit or fire the callback */ + TEST_ASSERT_EQUAL_UINT64(commit_before, s.commit_index); + TEST_ASSERT_EQUAL_INT(0, g_ca_spy.call_count); + + /* only the explicit commit-advance step advances commit_index */ + TEST_ASSERT_EQUAL_INT(RAFT_OK, raft_advance_commit(&s)); + TEST_ASSERT_EQUAL_UINT64(3, s.commit_index); + TEST_ASSERT_EQUAL_INT(1, g_ca_spy.call_count); +} + /* ---------- raft_on_election_timeout tests (task 3.5) ---------- */ /* @@ -5948,6 +6209,14 @@ void test_raft(void) RUN_TEST(test_ae_reply_failure_decrements_next_index_floor); RUN_TEST(test_ae_reply_stale_term_ignored); + /* raft_advance_commit (task 3.1/3.2/3.3, AdvanceCommitIndex + Figure-8 rule) */ + RUN_TEST(test_advance_commit_advances_current_term); + RUN_TEST(test_advance_commit_figure8_hold_previous_term); + RUN_TEST(test_advance_commit_no_quorum); + RUN_TEST(test_advance_commit_monotonic); + RUN_TEST(test_advance_commit_non_leader_eperm); + RUN_TEST(test_ae_reply_does_not_advance_commit); + /* raft_on_vote_request (task 3.3) */ RUN_TEST(test_vote_req_stale_term); RUN_TEST(test_vote_req_higher_term_updates_state);