From fce73b5b58c330ea4a6a20c27f1574831d47d779 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Wed, 29 Jul 2026 13:53:25 +0000 Subject: [PATCH] feat(raft-adapter-conformance): route post-connect vote/PreVote failures to production tally Splits vote_ep_err_handler into a real-vote handler and a new pre_vote_ep_err_handler, each registered at its existing (already separate) connection-setup call site. Extracts the identical CTRL_PRIMARY-role cleanup into a shared vote_ep_handle_promoted_disconnect helper, behavior unchanged. Each handler's election-phase branch now routes a post-connect failure (RDHUP/SSL-error/remote-close after connect, before any reply) through the same production tally the connect-time failure branches already use (vote_reply_dispatch / pre_vote_reply_dispatch, bug-1423/1424's fix), instead of the legacy handle_request_vote_reply that never updated the tally. Closes the one failure window bug-1423 didn't cover. 1060 -> 1061 tests, 0 failures. PreVote-specific test coverage is task 3.2's scope (pre_vote_reply_dispatch has no hook seam analogous to xce_vote_reply_hook; verified only by code symmetry + full-suite regression here). --- .kiro/specs/raft-adapter-conformance/tasks.md | 2 +- src/controller/ha.c | 127 +++++++++++++----- src/controller/testsuite/test_ha_vote_send.c | 64 +++++++++ 3 files changed, 160 insertions(+), 33 deletions(-) diff --git a/.kiro/specs/raft-adapter-conformance/tasks.md b/.kiro/specs/raft-adapter-conformance/tasks.md index a488245..fd13b71 100644 --- a/.kiro/specs/raft-adapter-conformance/tasks.md +++ b/.kiro/specs/raft-adapter-conformance/tasks.md @@ -32,7 +32,7 @@ - _Depends: 2.1, 2.2_ - [ ] 3. Core: vote and pre-vote endpoint failure routing -- [ ] 3.1 (P) Split the shared vote-endpoint error handler and route post-connect failures to the production tally +- [x] 3.1 (P) Split the shared vote-endpoint error handler and route post-connect failures to the production tally - Split `vote_ep_err_handler` into a real-vote handler (registered at its existing connection-setup call site) and a `pre_vote_ep_err_handler` (registered at its existing, separate PreVote connection-setup call site) - Extract the identical `CTRL_PRIMARY`-role cleanup branch (standby-endpoint deregistration, `repl_remove_standby`, quorum-transition logging) into one shared helper called by both, unchanged in behavior - Change each handler's election-phase branch to route a post-connect failure through the same production tally path already used for connect-time failures (`vote_reply_dispatch` for the real-vote handler, `pre_vote_reply_dispatch` for the pre-vote handler) instead of the legacy `handle_request_vote_reply` diff --git a/src/controller/ha.c b/src/controller/ha.c index 95f2eee..ae21b47 100644 --- a/src/controller/ha.c +++ b/src/controller/ha.c @@ -504,6 +504,10 @@ int ha_schedule_phase2_retry(cluster_t *c) static void on_vote_connected(endpoint_t *ep); static void on_pre_vote_connected(endpoint_t *ep); static int vote_ep_err_handler(void *ep_raw); +/* raft-adapter-conformance task 3.1: PreVote's own on_err handler, registered + * at raft_cb_send_pre_vote_request's endpoint-setup call site (mirrors the + * real-vote vote_ep_err_handler forward-declared above). */ +static int pre_vote_ep_err_handler(void *ep_raw); /* bug-1423: forward-declared so raft_cb_send_vote_request's connect/monitor * failure branches can route a synthetic "no vote" reply through the SAME * production path a real reply uses (see vote_reply_dispatch's own comment, @@ -824,7 +828,7 @@ static int raft_cb_send_pre_vote_request(void *ctx, int peer_idx, } endpoint_set_on_connected(ep, on_pre_vote_connected); - endpoint_set_err_handler(ep, vote_ep_err_handler); + endpoint_set_err_handler(ep, pre_vote_ep_err_handler); endpoint_set_on_fini(ep, vote_ep_on_fini); int rc = endpoint_connect(ep, peer->hostname, peer->port, @@ -7555,11 +7559,54 @@ void raft_cb_become_leader_with_cluster_for_test(cluster_t *c, uint64_t term) } #endif /* XCE_TESTING */ -/* ── Task 2.4: vote_ep_err_handler ───────────────────────────────────────── +/* ── Task 2.4 / raft-adapter-conformance task 3.1: vote endpoint err handlers ── * - * Called by iomgr when the outgoing vote endpoint encounters an error - * (connect failure, SSL error, remote close before reply). - * Counts the peer as denied: grant=0, tag=0, lsn=0. + * vote_ep_handle_promoted_disconnect: shared CTRL_PRIMARY-role cleanup for a + * dropped promoted connection (standby-endpoint deregistration, WAL + * replicator deregistration, quorum-transition logging). Extracted + * byte-for-byte from the formerly-single vote_ep_err_handler so both the + * real-vote and PreVote on_err handlers apply the identical cleanup — this + * branch's behavior is unchanged by the split. + */ +static void vote_ep_handle_promoted_disconnect(ha_state_t *ha, endpoint_t *ep, + vote_ep_ctx_t *vctx) +{ + /* Promoted connection dropped — remove from standby_endpoints */ + if (ha->standby_endpoints) { + pthread_mutex_lock(&ha->lock); + for (int i = 0; i < ha->standby_endpoints->count; i++) { + if (zarray_get(ha->standby_endpoints, i) == ep) { + zarray_del(ha->standby_endpoints, i); + break; + } + } + pthread_mutex_unlock(&ha->lock); + } + /* Task 2.2: deregister from WAL replicator. + * Req 1.2: when a standby disconnects, the HA layer shall deregister + * the endpoint from the replicator via repl_remove_standby. + * vctx->c may be NULL in test mode — guard accordingly. */ + if (vctx->c) + repl_remove_standby(&vctx->c->persist.repl, ep); + + /* Quorum-transition observability (Req 8.1): a standby just dropped — the + * write-quorum count changed. Record the loss edge once (silent if + * quorum is still held after the removal). */ + ha_log_quorum_transition(ha); +} + +/* + * vote_ep_err_handler: on_err callback for outgoing REAL-VOTE endpoints + * (registered in raft_cb_send_vote_request). Called by iomgr when the + * endpoint encounters an error (connect failure, SSL error, remote close + * before reply). + * + * Election-phase branch counts a post-connect failure through + * vote_reply_dispatch — the SAME production tally path a connect-time + * failure (raft_cb_send_vote_request's own rc<0 branches, bug-1423) or a + * delivered denial (vote_reply_msg_handler) already uses — instead of the + * legacy handle_request_vote_reply, which bypasses raft_on_vote_reply + * (raft-adapter-conformance Requirement 4.2). */ static int vote_ep_err_handler(void *ep_raw) { @@ -7572,34 +7619,50 @@ static int vote_ep_err_handler(void *ep_raw) memory_order_acquire); if (role == CTRL_PRIMARY) { - /* Promoted connection dropped — remove from standby_endpoints */ - if (ha->standby_endpoints) { - pthread_mutex_lock(&ha->lock); - for (int i = 0; i < ha->standby_endpoints->count; i++) { - if (zarray_get(ha->standby_endpoints, i) == ep) { - zarray_del(ha->standby_endpoints, i); - break; - } - } - pthread_mutex_unlock(&ha->lock); - } - /* Task 2.2: deregister from WAL replicator. - * Req 1.2: when a standby disconnects, the HA layer shall deregister - * the endpoint from the replicator via repl_remove_standby. - * vctx->c may be NULL in test mode — guard accordingly. */ - if (vctx->c) - repl_remove_standby(&vctx->c->persist.repl, ep); - - /* Quorum-transition observability (Req 8.1): a standby just dropped — the - * write-quorum count changed. Record the loss edge once (silent if - * quorum is still held after the removal). */ - ha_log_quorum_transition(ha); + vote_ep_handle_promoted_disconnect(ha, ep, vctx); } else { - /* Election phase: count as denied vote */ - handle_request_vote_reply(ha, vctx->ctx, vctx->peer_idx, - vctx->my_term, - 0, /* vote_granted = false */ - 0, 0); + /* Election phase: count as a denied vote through the production + * tally, same path a connect-time failure or a delivered denial + * uses (raft-adapter-conformance Requirement 4.2). */ + vote_reply_dispatch(ha, vctx->ctx, vctx->peer_idx, vctx->my_term, + /*vote_granted=*/0, 0, 0); + } + + /* Do NOT free vctx here — peer_ctrl_msg_handler may still reference ep->data. + * vctx is freed in ha_election_cleanup() when all per-peer contexts release. */ + return 0; +} + +/* + * pre_vote_ep_err_handler: on_err callback for outgoing PREVOTE endpoints + * (registered in raft_cb_send_pre_vote_request). Mirrors vote_ep_err_handler + * above for the PreVote endpoint kind — the two kinds share vote_ep_ctx_t but + * are already registered at distinct call sites, so the split (rather than a + * new discriminant field on vote_ep_ctx_t) mirrors an existing seam. + * + * Election-phase branch counts a post-connect failure through + * pre_vote_reply_dispatch — the SAME production tally path a connect-time + * failure (raft_cb_send_pre_vote_request's own rc<0 branches, bug-1423 PreVote + * twin) already uses — instead of the legacy handle_request_vote_reply + * (raft-adapter-conformance Requirement 4.3). + */ +static int pre_vote_ep_err_handler(void *ep_raw) +{ + endpoint_t *ep = (endpoint_t *)ep_raw; + vote_ep_ctx_t *vctx = (vote_ep_ctx_t *)ep->data; + if (!vctx) return 0; + + ha_state_t *ha = vctx->ha; + ctrl_role_t role = (ctrl_role_t)atomic_load_explicit(&ha->role, + memory_order_acquire); + + if (role == CTRL_PRIMARY) { + vote_ep_handle_promoted_disconnect(ha, ep, vctx); + } else { + /* Election phase: count as a not-granted PreVote reply through the + * production tally, same path a connect-time failure uses + * (raft-adapter-conformance Requirement 4.3). */ + pre_vote_reply_dispatch(ha, vctx->c, vctx->peer_idx, /*granted=*/0); } /* Do NOT free vctx here — peer_ctrl_msg_handler may still reference ep->data. diff --git a/src/controller/testsuite/test_ha_vote_send.c b/src/controller/testsuite/test_ha_vote_send.c index 7de59f9..b6852b9 100644 --- a/src/controller/testsuite/test_ha_vote_send.c +++ b/src/controller/testsuite/test_ha_vote_send.c @@ -654,6 +654,68 @@ static void test_bug1417_middle_index_real_vote_from_peer2_not_dropped(void) printf("test_bug1417_middle_index_real_vote_from_peer2_not_dropped: PASS\n"); } +/* ── raft-adapter-conformance task 3.1: post-connect vote failure routing ── + * + * vote_ep_err_handler's election-phase branch (the iomgr err_handler fired + * when an already-connected outgoing vote endpoint fails/closes before a + * reply arrives) must count that outcome through vote_reply_dispatch -- the + * SAME production tally path a connect-time failure (raft_cb_send_vote_request's + * own rc<0 branches, bug-1423) or a delivered denial (vote_reply_msg_handler) + * uses -- not the legacy handle_request_vote_reply, which bypasses + * raft_on_vote_reply entirely and, critically for this test, never reaches the + * xce_vote_reply_hook seam vote_reply_dispatch checks. + * + * Test seam: xce_vote_reply_hook (checked ONLY inside vote_reply_dispatch) lets + * us observe, without a live iomgr/raft election, whether the post-connect + * failure reached the production tally path. Pre-fix, this hook is never + * invoked by vote_ep_err_handler's election branch -- proving the bug. + */ + +extern int vote_ep_err_handler_for_test(endpoint_t *ep); + +static void test_post_connect_vote_failure_routes_through_production_tally(void) +{ + reset_reply_mock(); + + ha_state_t ha; + TEST_ASSERT_EQUAL_INT(0, ha_init(&ha, &g_vote_cfg, 0)); + /* Election phase, not CTRL_PRIMARY -- exercises the "else" branch. */ + atomic_store_explicit(&ha.role, CTRL_ELECTING, memory_order_release); + atomic_store_explicit(&ha.term, 4ULL, memory_order_release); + + ha_elect_ctx_t *ctx = alloc_ctx_for_vote(2); + pthread_mutex_init(&ctx->lock, NULL); + + endpoint_t ep; + make_endpoint(&ep); + + /* Build a real-vote endpoint context as if raft_cb_send_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 on this same ep. */ + vote_ep_ctx_build_for_test(&ep, &ha, ctx, /*peer_idx=*/0, + 4ULL, 100ULL, 50ULL, "ctrl-a"); + + int rc = vote_ep_err_handler_for_test(&ep); + TEST_ASSERT_EQUAL_INT(0, rc); + + TEST_ASSERT_EQUAL_INT_MESSAGE(1, g_reply_hook_calls, + "post-connect vote failure must reach vote_reply_dispatch's tally " + "(xce_vote_reply_hook), not the legacy handle_request_vote_reply path " + "which never invokes this hook"); + TEST_ASSERT_EQUAL_INT_MESSAGE(0, g_reply_granted, + "a post-connect failure must be counted as a denied vote (granted=0)"); + TEST_ASSERT_EQUAL_UINT64_MESSAGE(4ULL, g_reply_term, + "the synthesized denial must carry the candidate's own term"); + TEST_ASSERT_EQUAL_INT_MESSAGE(0, g_reply_peer_idx, + "peer_idx must match the failed endpoint's vote_ep_ctx_t peer_idx"); + + free(ep.data); + free_ctx_for_vote(ctx); + ha_fini(&ha); + printf("test_post_connect_vote_failure_routes_through_production_tally: PASS\n"); +} + /* ── Suite entry point ───────────────────────────────────────────────────── */ void test_ha_vote_send(void) @@ -670,4 +732,6 @@ void test_ha_vote_send(void) RUN_TEST(test_vote_reply_handler_no_uaf); /* bug-1417 */ 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); }