diff --git a/.kiro/specs/raft-adapter-conformance/tasks.md b/.kiro/specs/raft-adapter-conformance/tasks.md index fd13b71..eb28760 100644 --- a/.kiro/specs/raft-adapter-conformance/tasks.md +++ b/.kiro/specs/raft-adapter-conformance/tasks.md @@ -40,7 +40,7 @@ - _Requirements: 4.2, 4.3, 4.4_ - _Boundary: ha.c vote endpoint handlers_ -- [ ] 3.2 Test post-connect vote and pre-vote failure routing +- [x] 3.2 Test post-connect vote and pre-vote failure routing - Extend the existing test-construction helper (`vote_ep_ctx_build_for_test`) to synthesize a post-connect failure on a real-vote endpoint and assert it reaches `vote_reply_dispatch`'s tally - Do the same for a PreVote endpoint against `pre_vote_reply_dispatch` - Confirm a round where every peer's outcome is now counted (including this failure mode) reaches a quorum decision without waiting for the full round timeout @@ -119,3 +119,5 @@ - Task 2.2 (bug-1482): `myreq_handler`'s first unconditional step is `xce_decode(pkt, &request)`, which itself unconditionally `free_packet(pkt)` + frees the `io_packet_t` struct before returning on every path — `pkt` ownership transfers into `xce_decode`, the caller must never touch/free it afterward. Any new `_for_test` wrapper around `myreq_handler` (or `xce_decode` callers generally) must document this, not the reverse ("caller owns/frees pkt") — that wrong assumption is what caused the original test-only wrapper's doc comment to mislead a reviewer into requesting a double-free. - Task 2.2: driving a test through `cluster.c`'s real `myreq_handler` (rather than testing extracted logic in isolation) requires adding `cluster.c` to `controller_unit_test_src` in `meson.build` plus link-satisfying fakes for symbols its OTHER unrelated switch-cases reference (task.c/opget.c/sched.c/apiserver.c/myconf.c) — this is pre-existing, established convention in this codebase (`test_worker_join.c`, `test_commit_barrier_reply.c` already do it for the same reason), not scope creep. Any new test driving `myreq_handler` end-to-end should expect this same footprint. - New test-only wrappers exposing a `static` production dispatch function (the `_for_test` pattern, e.g. `peer_ctrl_msg_handler_for_test`, `myreq_handler_for_test`) MUST be wrapped in `#ifdef XCE_TESTING`/`#endif` — the source file compiles into the production binary too, and an unguarded wrapper ships as a new linked symbol in production with zero production purpose. +- Task 3.2 (bug-1483, not fixed): driving a PreVote round to `RAFT_PRE_VOTE_LOST` in a unit test (`c==NULL`) without first stubbing `ha_prevote_rearm_liveness_hook` falls through to the real `ha_arm_heartbeat_timeout_vtask`, which callocs into the static `s_liveness_ctx` and never frees it when `c==NULL` short-circuits the scheduling guard. `test_ha_prevote.c`'s existing LOST-path tests already stub-and-reset this hook — any new PreVote-round test that reaches LOST must do the same. +- `pre_vote_reply_dispatch` has no `XCE_TESTING` mock-hook seam (unlike `vote_reply_dispatch`'s `xce_vote_reply_hook`) — it calls `raft_on_pre_vote_reply` directly. Tests verifying PreVote routing must assert on `raft_state_t`'s own tally fields (`pre_vote_replies[]`, `pre_votes_granted`, `pre_vote_in_progress`) instead, per `test_ha_prevote.c`'s established technique. diff --git a/src/controller/ha.c b/src/controller/ha.c index ae21b47..9611a31 100644 --- a/src/controller/ha.c +++ b/src/controller/ha.c @@ -7670,6 +7670,19 @@ static int pre_vote_ep_err_handler(void *ep_raw) return 0; } +#ifdef XCE_TESTING +/* + * pre_vote_ep_err_handler_for_test: call the static pre_vote_ep_err_handler on + * ep. Allows tests to invoke the PreVote err_handler without going through + * iomgr/endpoint machinery — the PreVote twin of vote_ep_err_handler_for_test + * (raft-adapter-conformance task 3.2). + */ +int pre_vote_ep_err_handler_for_test(endpoint_t *ep) +{ + return pre_vote_ep_err_handler((void *)ep); +} +#endif /* XCE_TESTING */ + /* ── ha_init_raft: one-time raft state machine initialization ──────────────── * * Called once at startup after ha_restore_term() to wire the raft state diff --git a/src/controller/testsuite/test_ha_vote_send.c b/src/controller/testsuite/test_ha_vote_send.c index b6852b9..89daee6 100644 --- a/src/controller/testsuite/test_ha_vote_send.c +++ b/src/controller/testsuite/test_ha_vote_send.c @@ -716,6 +716,174 @@ static void test_post_connect_vote_failure_routes_through_production_tally(void) printf("test_post_connect_vote_failure_routes_through_production_tally: PASS\n"); } +/* ── raft-adapter-conformance task 3.2: post-connect PreVote failure routing ── + * + * pre_vote_ep_err_handler's election-phase branch (task 3.1) must count a + * post-connect PreVote endpoint failure through pre_vote_reply_dispatch -- + * the SAME production tally path a connect-time failure or a delivered + * PreVote reply uses -- not a legacy no-op path. Requirement 4.3 (mirror of + * 4.2 for the PreVote endpoint kind). + * + * Test seam: pre_vote_reply_dispatch has no mock hook analogous to + * xce_vote_reply_hook (it calls raft_on_pre_vote_reply directly). Instead we + * assert directly on raft_state_t's own PreVote tally fields + * (pre_vote_replies[], pre_votes_granted, pre_vote_in_progress) -- the same + * fields test_ha_prevote.c already asserts on for the delivered-reply path + * (pre_vote_reply_dispatch_for_test). Pre-fix, pre_vote_ep_err_handler routed + * this failure through the legacy handle_request_vote_reply path, which never + * touches raft_on_pre_vote_reply or these fields -- so pre_vote_replies[1] + * would stay 0 and this test would fail against pre-fix code. + */ + +extern int pre_vote_ep_err_handler_for_test(endpoint_t *ep); + +extern void pre_vote_reply_dispatch_for_test(ha_state_t *ha, cluster_t *c, + int peer_idx, int granted); + +/* A LOST round re-arms the liveness watchdog (ha_prevote_rearm_liveness). + * With the hook unset that falls through to the real + * ha_arm_heartbeat_timeout_vtask, which -- with c==NULL in unit-test mode -- + * calloc's a ctx into the static s_liveness_ctx and never arms a delaymgr + * task to free it (delay_task_ms is gated on `c && c->delaymgr.running`), + * leaking it and leaving a stale pointer into this test's stack-local `ha` + * once the test returns. test_ha_prevote.c avoids this identical hazard on + * its own LOST-path tests via the same hook override -- mirrored here. */ +extern int (*ha_prevote_rearm_liveness_hook)(ha_state_t *ha, cluster_t *c); + +static int stub_prevote_rearm_liveness(ha_state_t *ha, cluster_t *c) +{ + (void)ha; (void)c; + return 0; +} + +/* Build a 3-node (self + 2 peers) PreVote round in progress, mirroring + * test_ha_prevote.c's make_three_node + ha_start_prevote_async(&ha, NULL) + * fixture: role stays CTRL_STANDBY (the real production role throughout a + * PreVote round -- a node only becomes CTRL_ELECTING after WINNING the + * round), self-grant is recorded by the real raft_begin_pre_vote, and both + * non-self peers (absolute index 1 and 2) start unreplied. */ +static void start_three_node_prevote_round(ha_state_t *ha) +{ + ha_do_demote(ha); + ha->raft.n_peers = 3; + ha->raft.self_idx = ha->my_priority; /* my_priority == 0 for this fixture */ + ha->raft.role = RAFT_FOLLOWER; + ha->n_peers = 3; + + ha_start_prevote_async(ha, NULL); +} + +static void test_post_connect_prevote_failure_routes_through_production_tally(void) +{ + ha_state_t ha; + TEST_ASSERT_EQUAL_INT(0, ha_init(&ha, &g_vote_cfg, 0)); + atomic_store_explicit(&ha.term, 4ULL, memory_order_release); + + start_three_node_prevote_round(&ha); + TEST_ASSERT_EQUAL_INT_MESSAGE(1, ha.raft.pre_vote_in_progress, + "round must be in progress before the failure arrives"); + TEST_ASSERT_EQUAL_INT_MESSAGE(1, ha.raft.pre_votes_granted, + "self-grant must already be recorded by raft_begin_pre_vote"); + + ha_elect_ctx_t *ctx = alloc_ctx_for_vote(2); + pthread_mutex_init(&ctx->lock, NULL); + + endpoint_t ep; + make_endpoint(&ep); + + /* Build a PreVote endpoint context as if raft_cb_send_pre_vote_request had + * already connected it (on_vote_connected fired, request sent) -- then + * the connection fails before any reply arrives: iomgr invokes the + * registered err_handler (pre_vote_ep_err_handler) on this same ep. */ + vote_ep_ctx_build_for_test(&ep, &ha, ctx, /*peer_idx=*/1, + 4ULL, 100ULL, 50ULL, "ctrl-a"); + + int rc = pre_vote_ep_err_handler_for_test(&ep); + TEST_ASSERT_EQUAL_INT(0, rc); + + TEST_ASSERT_EQUAL_INT_MESSAGE(1, ha.raft.pre_vote_replies[1], + "post-connect PreVote failure must reach pre_vote_reply_dispatch's " + "tally (raft_on_pre_vote_reply), not a legacy no-op path"); + TEST_ASSERT_EQUAL_INT_MESSAGE(1, ha.raft.pre_votes_granted, + "a post-connect PreVote failure must be counted as NOT granted " + "(pre_votes_granted stays at the self-grant only)"); + TEST_ASSERT_EQUAL_INT_MESSAGE(1, ha.raft.pre_vote_in_progress, + "round stays pending -- only 1 of 2 non-self peers has been counted, " + "no quorum(2) or all-replied(2) condition met yet"); + + free(ep.data); + free_ctx_for_vote(ctx); + ha_fini(&ha); + printf("test_post_connect_prevote_failure_routes_through_production_tally: PASS\n"); +} + +/* ── raft-adapter-conformance task 3.2: fast quorum decision (Req 4.5) ──────── + * + * Once every peer in a PreVote round has had its outcome counted -- whether + * via a delivered reply or via this post-connect failure mode -- the round + * must reach its quorum decision immediately (synchronously inside the + * counting call), without waiting for the round's full timeout + * (ha_prevote_round_timeout_fn is never invoked in this test). Mixes both + * counting paths in the same round to prove the failure path is a full + * first-class participant in the fast "all replied" decision, not merely + * counted without unblocking the round. + */ +static void test_prevote_round_reaches_quorum_decision_via_fast_tally(void) +{ + ha_state_t ha; + TEST_ASSERT_EQUAL_INT(0, ha_init(&ha, &g_vote_cfg, 0)); + atomic_store_explicit(&ha.term, 4ULL, memory_order_release); + + start_three_node_prevote_round(&ha); + + /* This test's mixed round decides LOST (below), which re-arms the + * liveness watchdog -- stub it out so the real (c==NULL) production path + * is never exercised here (see extern decl comment above). */ + ha_prevote_rearm_liveness_hook = stub_prevote_rearm_liveness; + + ha_elect_ctx_t *ctx = alloc_ctx_for_vote(2); + pthread_mutex_init(&ctx->lock, NULL); + + /* Peer 1: post-connect endpoint failure -- counted via + * pre_vote_ep_err_handler (the fix under test). */ + endpoint_t ep1; + make_endpoint(&ep1); + vote_ep_ctx_build_for_test(&ep1, &ha, ctx, /*peer_idx=*/1, + 4ULL, 100ULL, 50ULL, "ctrl-a"); + TEST_ASSERT_EQUAL_INT(0, pre_vote_ep_err_handler_for_test(&ep1)); + + TEST_ASSERT_EQUAL_INT_MESSAGE(1, ha.raft.pre_vote_in_progress, + "round must still be pending -- peer 2's outcome is not counted yet"); + + /* Peer 2: a normal delivered denial through the production reply-dispatch + * path -- the same tally the post-connect failure above also reaches. */ + pre_vote_reply_dispatch_for_test(&ha, NULL, /*peer_idx=*/2, /*granted=*/0); + + /* Every non-self peer's outcome is now counted (one via the post-connect + * failure path, one via a delivered reply) -- the round must reach its + * LOST decision immediately, without any call into + * ha_prevote_round_timeout_fn (Requirement 4.5's fast "all replied" + * path). */ + TEST_ASSERT_EQUAL_INT_MESSAGE(0, ha.raft.pre_vote_in_progress, + "Req 4.5: once every peer outcome is counted -- including a " + "post-connect failure -- the round must reach its quorum decision " + "without waiting for the round's full timeout"); + TEST_ASSERT_EQUAL_INT_MESSAGE(1, ha.raft.pre_votes_granted, + "neither non-self peer granted -- self-grant only, quorum(2) never " + "reached"); + TEST_ASSERT_EQUAL_INT_MESSAGE(CTRL_STANDBY, + (int)atomic_load_explicit(&ha.role, memory_order_acquire), + "a LOST PreVote round leaves the node CTRL_STANDBY (Req 1.4, " + "unchanged by this fix)"); + + ha_prevote_rearm_liveness_hook = NULL; /* reset -- do not leak into other tests */ + + free(ep1.data); + free_ctx_for_vote(ctx); + ha_fini(&ha); + printf("test_prevote_round_reaches_quorum_decision_via_fast_tally: PASS\n"); +} + /* ── Suite entry point ───────────────────────────────────────────────────── */ void test_ha_vote_send(void) @@ -734,4 +902,7 @@ void test_ha_vote_send(void) RUN_TEST(test_bug1417_middle_index_real_vote_from_peer2_not_dropped); /* raft-adapter-conformance task 3.1 */ RUN_TEST(test_post_connect_vote_failure_routes_through_production_tally); + /* raft-adapter-conformance task 3.2 */ + RUN_TEST(test_post_connect_prevote_failure_routes_through_production_tally); + RUN_TEST(test_prevote_round_reaches_quorum_decision_via_fast_tally); }