diff --git a/.kiro/specs/raft-library/tasks.md b/.kiro/specs/raft-library/tasks.md index 083dd57f..58e91727 100644 --- a/.kiro/specs/raft-library/tasks.md +++ b/.kiro/specs/raft-library/tasks.md @@ -40,7 +40,7 @@ - Fires `on_trace` events: `election_start`, `self_vote`, `term_update` if NULL-safe checks pass - _Requirements: 2.1, 2.2, 2.3, 6.1_ -- [ ] 3.3 Implement raft_on_vote_request (incoming RequestVote processing) +- [x] 3.3 Implement raft_on_vote_request (incoming RequestVote processing) - Stale term: `term < current_term` → set `*reply_term`, fire `on_trace(vote_denied_stale_term)`, return `RAFT_DENY` - Term upgrade: `term > current_term` → update `current_term=term`, clear `voted_for=-1`, set `role=RAFT_FOLLOWER`, fire `on_trace(term_update)` - Role gate: if `role == RAFT_LEADER` → return `RAFT_DENY` (fire `on_trace(vote_denied_role)`); note: COMMITTING state is delegated to `on_vote_eligible` in the adapter diff --git a/src/lib/raft/raft.c b/src/lib/raft/raft.c index 8cab653b..bfad07cd 100644 --- a/src/lib/raft/raft.c +++ b/src/lib/raft/raft.c @@ -101,11 +101,64 @@ int raft_start_election(raft_state_t *s) int raft_on_vote_request(raft_state_t *s, int from_idx, uint64_t term, uint64_t *reply_term) { - (void)s; - (void)from_idx; - (void)term; - (void)reply_term; - return 0; + /* Step 1: stale term — deny immediately, no state change */ + if (term < s->term) { + *reply_term = s->term; + if (s->cbs->on_trace) + s->cbs->on_trace(s->ctx, "vote_denied_stale_term", NULL); + return RAFT_DENY; + } + + /* Step 2: term upgrade — update term, clear vote, step down to FOLLOWER */ + if (term > s->term) { + s->term = term; + s->voted_for = -1; + s->voted_for_term = 0; + s->role = RAFT_FOLLOWER; + } + + /* reply_term reflects our (possibly updated) current term from here on */ + *reply_term = s->term; + + /* Step 3: role gate — LEADER denies */ + if (s->role == RAFT_LEADER) { + if (s->cbs->on_trace) + s->cbs->on_trace(s->ctx, "vote_denied_role", NULL); + return RAFT_DENY; + } + + /* Step 4: duplicate-vote deny — already voted for a *different* candidate + * this same term. If voted_for == from_idx and voted_for_term == term, + * fall through to idempotent re-grant path below. */ + if (s->voted_for != -1 && + s->voted_for != from_idx && + s->voted_for_term == term) { + if (s->cbs->on_trace) + s->cbs->on_trace(s->ctx, "vote_denied_already_voted", NULL); + return RAFT_DENY; + } + + /* Step 5: application-level policy gate (freshness, log lsn, etc.) */ + if (s->cbs->on_vote_eligible != NULL) { + if (s->cbs->on_vote_eligible(s->ctx, from_idx, term) == 0) { + if (s->cbs->on_trace) + s->cbs->on_trace(s->ctx, "vote_denied_freshness", NULL); + return RAFT_DENY; + } + } + + /* Step 6: persist before granting — durability guarantee */ + if (s->cbs->on_persist(s->ctx, term, from_idx) != 0) { + /* Persist failed — deny without updating voted_for */ + return RAFT_DENY; + } + + /* Grant: record vote */ + s->voted_for = from_idx; + s->voted_for_term = term; + if (s->cbs->on_trace) + s->cbs->on_trace(s->ctx, "vote_granted", NULL); + return RAFT_GRANT; } int raft_on_vote_reply(raft_state_t *s, int from_idx, int granted, uint64_t term) diff --git a/src/lib/testsuite/test_raft.c b/src/lib/testsuite/test_raft.c index f32b7dab..6db2e8b7 100644 --- a/src/lib/testsuite/test_raft.c +++ b/src/lib/testsuite/test_raft.c @@ -66,21 +66,30 @@ static raft_callbacks_t make_full_cbs(void) /* Per-test counters — reset by each test before calling start_election */ typedef struct { - int send_vote_request_count; - int last_send_peer_idx; + int send_vote_request_count; + int last_send_peer_idx; uint64_t last_send_term; /* track each peer index sent (up to 8 for test purposes) */ - int sent_peers[8]; + int sent_peers[8]; - int schedule_timeout_count; + int schedule_timeout_count; uint32_t last_schedule_delay; - int persist_count; + int persist_count; uint64_t last_persist_term; - int last_persist_voted_for; + int last_persist_voted_for; + int persist_return_value; /* configurable: 0=success, non-zero=fail */ - int become_leader_count; + int become_leader_count; uint64_t last_become_leader_term; + + /* on_vote_eligible mock */ + int vote_eligible_return_value; /* configurable: 1=grant, 0=deny; default 1 */ + int vote_eligible_call_count; + + /* on_trace mock */ + int trace_call_count; + char last_trace_event[64]; } mock_cbs_state_t; static mock_cbs_state_t g_mock; @@ -88,6 +97,9 @@ static mock_cbs_state_t g_mock; static void reset_mock(void) { memset(&g_mock, 0, sizeof(g_mock)); + /* Defaults: persist succeeds, vote_eligible grants */ + g_mock.persist_return_value = 0; + g_mock.vote_eligible_return_value = 1; } static int mock_send_vote_request(void *ctx, int peer_idx, uint64_t term) @@ -114,7 +126,7 @@ static int mock_persist(void *ctx, uint64_t term, int voted_for) g_mock.persist_count++; g_mock.last_persist_term = term; g_mock.last_persist_voted_for = voted_for; - return 0; + return g_mock.persist_return_value; } static void mock_schedule_timeout(void *ctx, uint32_t delay_ms) @@ -124,6 +136,24 @@ static void mock_schedule_timeout(void *ctx, uint32_t delay_ms) g_mock.last_schedule_delay = delay_ms; } +static int mock_vote_eligible(void *ctx, int from_idx, uint64_t term) +{ + (void)ctx; (void)from_idx; (void)term; + g_mock.vote_eligible_call_count++; + return g_mock.vote_eligible_return_value; +} + +static void mock_trace(void *ctx, const char *event, const char *detail) +{ + (void)ctx; (void)detail; + g_mock.trace_call_count++; + if (event) { + strncpy(g_mock.last_trace_event, event, + sizeof(g_mock.last_trace_event) - 1); + g_mock.last_trace_event[sizeof(g_mock.last_trace_event) - 1] = '\0'; + } +} + /* Build callbacks that record invocations in g_mock */ static raft_callbacks_t make_mock_cbs(void) { @@ -136,7 +166,7 @@ static raft_callbacks_t make_mock_cbs(void) cbs.on_schedule_timeout = mock_schedule_timeout; cbs.on_cancel_timeout = stub_cancel_timeout; cbs.on_start_election_after_delay = stub_start_election_after_delay; - /* on_vote_eligible and on_trace left NULL */ + /* on_vote_eligible and on_trace left NULL — tests that need them set explicitly */ return cbs; } @@ -547,6 +577,393 @@ static void test_start_election_already_leader(void) TEST_ASSERT_EQUAL_INT(0, g_mock.become_leader_count); } +/* ---------- raft_on_vote_request tests (task 3.3) ---------- */ + +/* + * Helper: init a 3-node node with mock callbacks (including trace). + * term, role, voted_for can be set after init. + */ +static int init_vote_node(raft_state_t *s, raft_callbacks_t *cbs, + int n_peers, int self_idx, uint64_t initial_term) +{ + raft_config_t cfg; + memset(&cfg, 0, sizeof(cfg)); + cfg.n_peers = n_peers; + cfg.self_idx = self_idx; + cfg.election_timeout_ms = 1000; + cfg.initial_term = initial_term; + cfg.initial_voted_for = -1; + cfg.cbs = cbs; + return raft_init(s, &cfg); +} + +/* + * test_vote_req_stale_term + * + * Node at term=5, receives request with term=3. + * Expected: RAFT_DENY, *reply_term=5, state unchanged, + * on_trace fired with "vote_denied_stale_term". + */ +static void test_vote_req_stale_term(void) +{ + raft_state_t s; + raft_callbacks_t cbs = make_mock_cbs(); + uint64_t reply_term = 0; + + cbs.on_trace = mock_trace; + reset_mock(); + + TEST_ASSERT_EQUAL_INT(0, init_vote_node(&s, &cbs, 3, 0, 5)); + /* Confirm starting state */ + TEST_ASSERT_EQUAL_UINT64(5, s.term); + TEST_ASSERT_EQUAL_INT(RAFT_FOLLOWER, s.role); + TEST_ASSERT_EQUAL_INT(-1, s.voted_for); + + int rc = raft_on_vote_request(&s, 1, 3, &reply_term); + + TEST_ASSERT_EQUAL_INT(RAFT_DENY, rc); + TEST_ASSERT_EQUAL_UINT64(5, reply_term); + + /* State must be unchanged */ + TEST_ASSERT_EQUAL_UINT64(5, s.term); + TEST_ASSERT_EQUAL_INT(RAFT_FOLLOWER, s.role); + TEST_ASSERT_EQUAL_INT(-1, s.voted_for); + + /* on_trace must fire with "vote_denied_stale_term" */ + TEST_ASSERT_GREATER_THAN_INT(0, g_mock.trace_call_count); + TEST_ASSERT_EQUAL_STRING("vote_denied_stale_term", g_mock.last_trace_event); + + /* on_persist must NOT fire */ + TEST_ASSERT_EQUAL_INT(0, g_mock.persist_count); +} + +/* + * test_vote_req_higher_term_updates_state + * + * Node at term=2, receives request with term=5 (higher). + * Expected: s->term updated to 5, voted_for cleared to -1, role=FOLLOWER. + * Since voted_for is unset and persist succeeds: RAFT_GRANT. + * reply_term = 5. + */ +static void test_vote_req_higher_term_updates_state(void) +{ + raft_state_t s; + raft_callbacks_t cbs = make_mock_cbs(); + uint64_t reply_term = 0; + + cbs.on_trace = mock_trace; + reset_mock(); + + TEST_ASSERT_EQUAL_INT(0, init_vote_node(&s, &cbs, 3, 0, 2)); + /* Simulate node was CANDIDATE in term=2 */ + s.role = RAFT_CANDIDATE; + s.voted_for = 0; /* voted for self */ + s.voted_for_term = 2; + + int rc = raft_on_vote_request(&s, 1, 5, &reply_term); + + /* Term upgrade must happen: term updated, role stepped down */ + TEST_ASSERT_EQUAL_UINT64(5, s.term); + TEST_ASSERT_EQUAL_INT(RAFT_FOLLOWER, s.role); + + /* Then grant because voted_for is cleared and persist succeeds */ + TEST_ASSERT_EQUAL_INT(RAFT_GRANT, rc); + TEST_ASSERT_EQUAL_UINT64(5, reply_term); + + /* on_persist must fire once */ + TEST_ASSERT_EQUAL_INT(1, g_mock.persist_count); + TEST_ASSERT_EQUAL_UINT64(5, g_mock.last_persist_term); + TEST_ASSERT_EQUAL_INT(1, g_mock.last_persist_voted_for); + + /* voted_for set on grant */ + TEST_ASSERT_EQUAL_INT(1, s.voted_for); + TEST_ASSERT_EQUAL_UINT64(5, s.voted_for_term); +} + +/* + * test_vote_req_same_term_already_voted_different + * + * Node at term=5, voted_for=1, voted_for_term=5. + * Receives request from idx=2 at term=5. + * Expected: RAFT_DENY, "vote_denied_already_voted" trace. + */ +static void test_vote_req_same_term_already_voted_different(void) +{ + raft_state_t s; + raft_callbacks_t cbs = make_mock_cbs(); + uint64_t reply_term = 0; + + cbs.on_trace = mock_trace; + reset_mock(); + + TEST_ASSERT_EQUAL_INT(0, init_vote_node(&s, &cbs, 3, 0, 5)); + s.voted_for = 1; + s.voted_for_term = 5; + + int rc = raft_on_vote_request(&s, 2, 5, &reply_term); + + TEST_ASSERT_EQUAL_INT(RAFT_DENY, rc); + TEST_ASSERT_EQUAL_UINT64(5, reply_term); + + /* State: voted_for still 1 (not 2), term unchanged */ + TEST_ASSERT_EQUAL_INT(1, s.voted_for); + TEST_ASSERT_EQUAL_UINT64(5, s.term); + + /* on_trace fires with already_voted */ + TEST_ASSERT_GREATER_THAN_INT(0, g_mock.trace_call_count); + TEST_ASSERT_EQUAL_STRING("vote_denied_already_voted", g_mock.last_trace_event); + + /* on_persist must NOT fire */ + TEST_ASSERT_EQUAL_INT(0, g_mock.persist_count); +} + +/* + * test_vote_req_same_term_idempotent_same_candidate + * + * Node at term=5, voted_for=1, voted_for_term=5. + * Receives request from idx=1 at term=5 (same candidate, same term). + * Expected: idempotent re-grant → RAFT_GRANT, voted_for still 1. + */ +static void test_vote_req_same_term_idempotent_same_candidate(void) +{ + raft_state_t s; + raft_callbacks_t cbs = make_mock_cbs(); + uint64_t reply_term = 0; + + cbs.on_trace = mock_trace; + reset_mock(); + + TEST_ASSERT_EQUAL_INT(0, init_vote_node(&s, &cbs, 3, 0, 5)); + s.voted_for = 1; + s.voted_for_term = 5; + + int rc = raft_on_vote_request(&s, 1, 5, &reply_term); + + TEST_ASSERT_EQUAL_INT(RAFT_GRANT, rc); + TEST_ASSERT_EQUAL_UINT64(5, reply_term); + + /* voted_for still 1 */ + TEST_ASSERT_EQUAL_INT(1, s.voted_for); + TEST_ASSERT_EQUAL_UINT64(5, s.voted_for_term); + + /* on_persist must fire once for the re-grant */ + TEST_ASSERT_EQUAL_INT(1, g_mock.persist_count); +} + +/* + * test_vote_req_leader_denies + * + * Node is RAFT_LEADER at term=5, receives request at term=5 (same term, no upgrade). + * Expected: RAFT_DENY ("vote_denied_role" trace), role stays LEADER. + */ +static void test_vote_req_leader_denies(void) +{ + raft_state_t s; + raft_callbacks_t cbs = make_mock_cbs(); + uint64_t reply_term = 0; + + cbs.on_trace = mock_trace; + reset_mock(); + + TEST_ASSERT_EQUAL_INT(0, init_vote_node(&s, &cbs, 3, 0, 5)); + s.role = RAFT_LEADER; + + int rc = raft_on_vote_request(&s, 1, 5, &reply_term); + + TEST_ASSERT_EQUAL_INT(RAFT_DENY, rc); + TEST_ASSERT_EQUAL_UINT64(5, reply_term); + + /* Role gate fires: still LEADER (term==request term, no upgrade) */ + TEST_ASSERT_EQUAL_INT(RAFT_LEADER, s.role); + + /* on_trace fires with "vote_denied_role" */ + TEST_ASSERT_GREATER_THAN_INT(0, g_mock.trace_call_count); + TEST_ASSERT_EQUAL_STRING("vote_denied_role", g_mock.last_trace_event); + + /* on_persist must NOT fire */ + TEST_ASSERT_EQUAL_INT(0, g_mock.persist_count); +} + +/* + * test_vote_req_on_vote_eligible_denies + * + * on_vote_eligible returns 0 → RAFT_DENY, voted_for unchanged, + * "vote_denied_freshness" trace fires. + */ +static void test_vote_req_on_vote_eligible_denies(void) +{ + raft_state_t s; + raft_callbacks_t cbs = make_mock_cbs(); + uint64_t reply_term = 0; + + cbs.on_vote_eligible = mock_vote_eligible; + cbs.on_trace = mock_trace; + reset_mock(); + g_mock.vote_eligible_return_value = 0; /* deny */ + + TEST_ASSERT_EQUAL_INT(0, init_vote_node(&s, &cbs, 3, 0, 5)); + + int rc = raft_on_vote_request(&s, 1, 5, &reply_term); + + TEST_ASSERT_EQUAL_INT(RAFT_DENY, rc); + TEST_ASSERT_EQUAL_UINT64(5, reply_term); + + /* voted_for must be unchanged */ + TEST_ASSERT_EQUAL_INT(-1, s.voted_for); + + /* on_vote_eligible must have been called */ + TEST_ASSERT_EQUAL_INT(1, g_mock.vote_eligible_call_count); + + /* on_trace fires with "vote_denied_freshness" */ + TEST_ASSERT_GREATER_THAN_INT(0, g_mock.trace_call_count); + TEST_ASSERT_EQUAL_STRING("vote_denied_freshness", g_mock.last_trace_event); + + /* on_persist must NOT fire */ + TEST_ASSERT_EQUAL_INT(0, g_mock.persist_count); +} + +/* + * test_vote_req_on_vote_eligible_grants + * + * on_vote_eligible returns 1 and persist succeeds → RAFT_GRANT, voted_for set. + */ +static void test_vote_req_on_vote_eligible_grants(void) +{ + raft_state_t s; + raft_callbacks_t cbs = make_mock_cbs(); + uint64_t reply_term = 0; + + cbs.on_vote_eligible = mock_vote_eligible; + cbs.on_trace = mock_trace; + reset_mock(); + g_mock.vote_eligible_return_value = 1; /* grant */ + + TEST_ASSERT_EQUAL_INT(0, init_vote_node(&s, &cbs, 3, 0, 5)); + + int rc = raft_on_vote_request(&s, 1, 5, &reply_term); + + TEST_ASSERT_EQUAL_INT(RAFT_GRANT, rc); + TEST_ASSERT_EQUAL_UINT64(5, reply_term); + + /* voted_for must be set */ + TEST_ASSERT_EQUAL_INT(1, s.voted_for); + TEST_ASSERT_EQUAL_UINT64(5, s.voted_for_term); + + /* on_vote_eligible called once */ + TEST_ASSERT_EQUAL_INT(1, g_mock.vote_eligible_call_count); + + /* on_persist called once */ + TEST_ASSERT_EQUAL_INT(1, g_mock.persist_count); + + /* on_trace fires "vote_granted" */ + TEST_ASSERT_GREATER_THAN_INT(0, g_mock.trace_call_count); + TEST_ASSERT_EQUAL_STRING("vote_granted", g_mock.last_trace_event); +} + +/* + * test_vote_req_on_vote_eligible_null + * + * on_vote_eligible is NULL → treated as eligible, grant if persist ok. + */ +static void test_vote_req_on_vote_eligible_null(void) +{ + raft_state_t s; + raft_callbacks_t cbs = make_mock_cbs(); + uint64_t reply_term = 0; + + /* on_vote_eligible left NULL in make_mock_cbs */ + cbs.on_trace = mock_trace; + reset_mock(); + + TEST_ASSERT_EQUAL_INT(0, init_vote_node(&s, &cbs, 3, 0, 5)); + + int rc = raft_on_vote_request(&s, 1, 5, &reply_term); + + TEST_ASSERT_EQUAL_INT(RAFT_GRANT, rc); + TEST_ASSERT_EQUAL_UINT64(5, reply_term); + + /* voted_for must be set */ + TEST_ASSERT_EQUAL_INT(1, s.voted_for); + TEST_ASSERT_EQUAL_UINT64(5, s.voted_for_term); + + /* on_persist called once */ + TEST_ASSERT_EQUAL_INT(1, g_mock.persist_count); +} + +/* + * test_vote_req_persist_fails + * + * on_persist returns non-zero → RAFT_DENY, voted_for unchanged. + */ +static void test_vote_req_persist_fails(void) +{ + raft_state_t s; + raft_callbacks_t cbs = make_mock_cbs(); + uint64_t reply_term = 0; + + cbs.on_trace = mock_trace; + reset_mock(); + g_mock.persist_return_value = -1; /* fail */ + + TEST_ASSERT_EQUAL_INT(0, init_vote_node(&s, &cbs, 3, 0, 5)); + + int rc = raft_on_vote_request(&s, 1, 5, &reply_term); + + TEST_ASSERT_EQUAL_INT(RAFT_DENY, rc); + TEST_ASSERT_EQUAL_UINT64(5, reply_term); + + /* voted_for must NOT be updated */ + TEST_ASSERT_EQUAL_INT(-1, s.voted_for); + TEST_ASSERT_EQUAL_UINT64(0, s.voted_for_term); + + /* on_persist called once (it ran, but failed) */ + TEST_ASSERT_EQUAL_INT(1, g_mock.persist_count); +} + +/* + * test_vote_req_reply_term_always_set + * + * Verify that *reply_term == s->term on every return path by testing + * several scenarios and checking reply_term each time. + */ +static void test_vote_req_reply_term_always_set(void) +{ + raft_state_t s; + raft_callbacks_t cbs = make_mock_cbs(); + uint64_t reply_term; + + /* Path 1: stale term deny */ + reset_mock(); + TEST_ASSERT_EQUAL_INT(0, init_vote_node(&s, &cbs, 3, 0, 7)); + reply_term = 0; + raft_on_vote_request(&s, 1, 3, &reply_term); + TEST_ASSERT_EQUAL_UINT64(s.term, reply_term); /* must equal current term=7 */ + + /* Path 2: term upgrade + grant */ + reset_mock(); + TEST_ASSERT_EQUAL_INT(0, init_vote_node(&s, &cbs, 3, 0, 3)); + reply_term = 0; + raft_on_vote_request(&s, 1, 7, &reply_term); + TEST_ASSERT_EQUAL_UINT64(s.term, reply_term); /* must equal upgraded term=7 */ + + /* Path 3: already voted for different — deny */ + reset_mock(); + TEST_ASSERT_EQUAL_INT(0, init_vote_node(&s, &cbs, 3, 0, 5)); + s.voted_for = 1; s.voted_for_term = 5; + reply_term = 0; + raft_on_vote_request(&s, 2, 5, &reply_term); + TEST_ASSERT_EQUAL_UINT64(s.term, reply_term); /* must equal 5 */ + + /* Path 4: persist fails — deny */ + g_mock.persist_return_value = -1; + reset_mock(); + g_mock.persist_return_value = -1; + TEST_ASSERT_EQUAL_INT(0, init_vote_node(&s, &cbs, 3, 0, 5)); + reply_term = 0; + raft_on_vote_request(&s, 1, 5, &reply_term); + TEST_ASSERT_EQUAL_UINT64(s.term, reply_term); /* must equal 5 */ +} + /* ---------- raft_snapshot tests ---------- */ static void test_raft_snapshot_returns_current_state(void) @@ -607,4 +1024,16 @@ void test_raft(void) RUN_TEST(test_start_election_single_node); RUN_TEST(test_start_election_already_candidate); RUN_TEST(test_start_election_already_leader); + + /* raft_on_vote_request (task 3.3) */ + RUN_TEST(test_vote_req_stale_term); + RUN_TEST(test_vote_req_higher_term_updates_state); + RUN_TEST(test_vote_req_same_term_already_voted_different); + RUN_TEST(test_vote_req_same_term_idempotent_same_candidate); + RUN_TEST(test_vote_req_leader_denies); + RUN_TEST(test_vote_req_on_vote_eligible_denies); + RUN_TEST(test_vote_req_on_vote_eligible_grants); + RUN_TEST(test_vote_req_on_vote_eligible_null); + RUN_TEST(test_vote_req_persist_fails); + RUN_TEST(test_vote_req_reply_term_always_set); }