wdgzyjx/myxce
1
0
Fork 0
forked from lei.chen/myxce
Code Pull requests Activity

fix(election-iomgr-refactor): correct task 2.4 review findings

Browse source 
- CRITICAL: single-node shortcircuit enters CTRL_PRIMARY_COMMITTING
  (not CTRL_PRIMARY directly); phase2 transition deferred to task 4.x
- CRITICAL: add n_peers==0 guard in ha_elect_evaluate_and_signal to
  prevent votes[0] out-of-bounds access
- HIGH: wire client_ssl_ctx after endpoint_connect for outgoing votes
- HIGH: add timeout_ctx field to ha_elect_ctx_t for task 4.x cancel
- HIGH: clarify vote_ep_err_handler ownership (no premature free of vctx;
  deferred to ha_election_cleanup in task 5.1)
- Tests updated: single-node shortcircuit asserts COMMITTING not PRIMARY
This commit is contained in:
Rongsong Shen 2026-05-30 09:27:48 +00:00
parent e50afd02f6
commit 543981c505
3 changed files with 80 additions and 13 deletions
Download patch file Download diff file Expand all files Collapse all files

50
src/controller/ha.c
View file

@ -901,6 +901,17 @@ ctrl_role_t ha_elect_evaluate_and_signal(ha_state_t *ha, ha_elect_ctx_t *ctx)
if (ctx->done_posted)
return (ctx->result == 1) ? CTRL_PRIMARY : CTRL_STANDBY;
/* Fix 2 (CRITICAL): zero-peer guard — single-node shortcircuit.
* n_peers==0 means no other nodes; votes[] flexible array has 0 entries,
* so accessing votes[0] would be out-of-bounds. Self-vote (votes_granted=1)
* always satisfies quorum=1. No fresher-peer or fatal-gate checks apply. */
if (ctx->n_peers == 0) {
ctx->result = 1;
ctx->done_posted = 1;
sem_post(&ctx->done);
return CTRL_PRIMARY;
}
int total_nodes = ctx->n_peers + 1; /* include self */
int quorum = (total_nodes + 1) / 2; /* ceil(total/2) */
@ -1434,6 +1445,11 @@ static int vote_ep_err_handler(void *ep_raw)
vctx->my_term,
0, /* vote_granted = false */
0, 0);
/* Fix 5 (HIGH): do NOT free vctx here.
* ep->data (vctx) is also referenced by vote_reply_msg_handler via ep->data.
* The endpoint lifetime is managed by iomgr; vctx is freed in
* ha_election_cleanup() (task 5.1) when all per-peer contexts are released.
* Freeing here would cause a use-after-free in vote_reply_msg_handler. */
return 0;
}
@ -1499,14 +1515,17 @@ void ha_start_election_async(ha_state_t *ha, cluster_t *c,
* votes[0] would be out-of-bounds — use a separate approach for the
* evaluate function's my_tag/my_lsn reads). */
/* For single-node, quorum = ceil(1/2) = 1; votes_granted=1 → wins */
/* For single-node, quorum = ceil(1/2) = 1; votes_granted=1 → wins.
* Fix 1 (CRITICAL): enter CTRL_PRIMARY_COMMITTING, not CTRL_PRIMARY.
* With no standbys the committing timeout (task 4.4) will fire and
* promote to CTRL_PRIMARY (degraded/single-node mode, Req 8.4). */
INFO("HA election: outcome=PRIMARY term=%" PRIu64
" votes_granted=1 quorum=1 (single-node async)", cur_term);
INFO("HA FSM: CTRL_ELECTING → CTRL_PRIMARY_COMMITTING "
"(single-node async, term=%" PRIu64 ")", cur_term);
/* Transition to PRIMARY (or COMMITTING if phase2 applies) */
atomic_store_explicit(&ha->role, (int32_t)CTRL_PRIMARY, memory_order_release);
atomic_store_explicit(&ha->role, (int32_t)CTRL_PRIMARY_COMMITTING,
memory_order_release);
pthread_mutex_destroy(&ctx->lock);
sem_destroy(&ctx->done);
@ -1607,6 +1626,26 @@ void ha_start_election_async(ha_state_t *ha, cluster_t *c,
continue;
}
/* Fix 3 (HIGH): endpoint_connect creates the SSL object from iomgr->ssl_ctx
* (server context). For outgoing vote connections we need TLS_client_method.
* After connect, replace the SSL object with one from client_ssl_ctx and
* explicitly set connect (client) state so the handshake goes client-side.
* The fd is already set on the old ssl object — re-set it on the new one. */
if (ha->client_ssl_ctx && ep->ssl) {
SSL_CTX *client_ctx = (SSL_CTX *)ha->client_ssl_ctx;
SSL *new_ssl = SSL_new(client_ctx);
if (new_ssl) {
int ep_fd = SSL_get_fd(ep->ssl);
SSL_free(ep->ssl);
ep->ssl = new_ssl;
SSL_set_fd(ep->ssl, ep_fd);
SSL_set_connect_state(ep->ssl);
}
/* If SSL_new fails, fall through with the original SSL object;
* worst case the handshake uses server context on client side,
* which will fail and trigger vote_ep_err_handler (peer denied). */
}
/* Register with iomgr for async I/O */
if (c) {
rc = iomgr_monitor(ep);
@ -1646,6 +1685,11 @@ void ha_start_election_async(ha_state_t *ha, cluster_t *c,
ha_elect_evaluate_and_signal(ha, ctx);
pthread_mutex_unlock(&ctx->lock);
}
} else {
/* Fix 4 (HIGH): wire timeout_ctx back into ctx so that task 4.1
* (quorum-reached path in handle_request_vote_reply) can cancel
* the watchdog by setting timeout_ctx->cancelled = 1 atomically. */
ctx->timeout_ctx = t;
}
} else {
WARN("ha_start_election_async: calloc election_timeout_ctx_t failed");

4
src/controller/ha.h
View file

@ -92,6 +92,10 @@ typedef struct {
int done_posted; /* Guard: non-zero once sem_post(&done) has been called */
pthread_mutex_t lock; /* Protects votes_granted/n_replied/max_resp_tag/max_resp_lsn/result */
sem_t done; /* Startup thread blocks; dispatch posts when done */
/* Fix 4 (HIGH): pointer back to the election_timeout_ctx_t so that task 4.1
* (quorum-reached path) can set timeout_ctx->cancelled = 1 to cancel the
* watchdog. Set by ha_start_election_async after scheduling the timeout. */
void *timeout_ctx; /* election_timeout_ctx_t*; NULL until watchdog scheduled */
ha_vote_entry_t votes[]; /* C99 flexible array: n_peers-1 entries */
} ha_elect_ctx_t;

39
src/controller/testsuite/test_ha_start_election_async.c
View file

@ -5,8 +5,8 @@
* Strategy:
* - n_peers=1 shortcircuit: no iomgr needed; election completes synchronously
* via ha_elect_evaluate_and_signal immediately after ha_start_election_async
* returns (role flips to CTRL_PRIMARY because phase2_done=0 path routes
* through election_timeout_fn logic).
* returns (role flips to CTRL_PRIMARY_COMMITTING; task 4.4 committing timeout
* will later promote to CTRL_PRIMARY for degraded/single-node mode).
* - Function returns in < 5ms (observable via clock_gettime).
* - n_peers=1 shortcircuit stores n_peers in ha->n_peers.
*
@ -101,7 +101,7 @@ static void test_single_node_stores_n_peers(void)
printf("test_single_node_stores_n_peers: PASS\n");
}
/* ── Test 3: single-node shortcircuit fires ha_elect_evaluate_and_signal ─── */
/* ── Test 3: single-node shortcircuit enters CTRL_PRIMARY_COMMITTING ──────── */
static void test_single_node_shortcircuit_evaluates_immediately(void)
{
@ -111,21 +111,39 @@ static void test_single_node_shortcircuit_evaluates_immediately(void)
/* Start in CTRL_ELECTING state so role transition is visible */
atomic_store_explicit(&ha.role, (int32_t)CTRL_ELECTING, memory_order_release);
/* n_peers=1: must call ha_elect_evaluate_and_signal immediately,
* which for a fresh single-node election grants PRIMARY.
* Since phase2_done=0, the COMMITTING state is entered
* (or PRIMARY directly depending on design, but role must NOT stay ELECTING). */
/* n_peers=1: must call ha_elect_evaluate_and_signal immediately.
* Fix 1 (CRITICAL): single-node shortcircuit must enter CTRL_PRIMARY_COMMITTING,
* not CTRL_PRIMARY. Task 4.4 committing timeout will promote to CTRL_PRIMARY
* once the committing phase completes (Req 8.4 / degraded-single-node path). */
ha_start_election_async(&ha, NULL, 1, NULL, 100ULL, 50ULL, "ctrl-a");
ctrl_role_t role = (ctrl_role_t)atomic_load_explicit(&ha.role, memory_order_acquire);
TEST_ASSERT_NOT_EQUAL_MESSAGE((int)CTRL_ELECTING, (int)role,
"single-node shortcircuit must transition role away from CTRL_ELECTING");
TEST_ASSERT_EQUAL_INT_MESSAGE((int)CTRL_PRIMARY_COMMITTING, (int)role,
"single-node shortcircuit must enter CTRL_PRIMARY_COMMITTING (not CTRL_PRIMARY)");
ha_fini(&ha);
printf("test_single_node_shortcircuit_evaluates_immediately: PASS (role=%d)\n",
printf("test_single_node_shortcircuit_evaluates_immediately: PASS (role=%d == CTRL_PRIMARY_COMMITTING)\n",
(int)role);
}
/* ── Test 6: single-node shortcircuit does NOT go directly to CTRL_PRIMARY ── */
static void test_single_node_not_primary_directly(void)
{
ha_state_t ha;
ha_init(&ha, &g_async_cfg, 0);
ha_start_election_async(&ha, NULL, 1, NULL, 0ULL, 0ULL, "ctrl-a");
ctrl_role_t role = (ctrl_role_t)atomic_load_explicit(&ha.role, memory_order_acquire);
TEST_ASSERT_NOT_EQUAL_MESSAGE((int)CTRL_PRIMARY, (int)role,
"single-node shortcircuit must NOT jump directly to CTRL_PRIMARY; "
"must go through CTRL_PRIMARY_COMMITTING first");
ha_fini(&ha);
printf("test_single_node_not_primary_directly: PASS (role=%d)\n", (int)role);
}
/* ── Test 4: ha->n_peers set for multi-peer count too ───────────────────── */
static void test_n_peers_stored_for_multi_peer(void)
@ -175,6 +193,7 @@ void test_ha_start_election_async(void)
RUN_TEST(test_single_node_returns_quickly);
RUN_TEST(test_single_node_stores_n_peers);
RUN_TEST(test_single_node_shortcircuit_evaluates_immediately);
RUN_TEST(test_single_node_not_primary_directly);
RUN_TEST(test_n_peers_stored_for_multi_peer);
RUN_TEST(test_single_node_increments_term);
}