diff --git a/.kiro/specs/ha-single-leader-role/tasks.md b/.kiro/specs/ha-single-leader-role/tasks.md index 54fd654..ee86893 100644 --- a/.kiro/specs/ha-single-leader-role/tasks.md +++ b/.kiro/specs/ha-single-leader-role/tasks.md @@ -31,7 +31,7 @@ - _Requirements: 1.1, 1.3, 1.4, 1.5, 2.1, 2.2, 2.3_ - [ ] 3. Legacy XCE_TESTING election path symmetry -- [ ] 3.1 Update ha_elect_evaluate_and_signal and its direct tests (RED → GREEN in one pass) +- [x] 3.1 Update ha_elect_evaluate_and_signal and its direct tests (RED → GREEN in one pass) - Retarget existing legacy-path expectations first (test_ha_start_election_async.c, the evaluation cases in test_ha_role.c): quorum win → role `CTRL_PRIMARY` directly, no watchdog armed — run to confirm RED against current code - In the CTRL_PRIMARY branch of `ha_elect_evaluate_and_signal` (ha.c:2247-2300 anchor): replace the COMMITTING store (2248-2252) with the direct `CTRL_PRIMARY` store, delete the `repl_ack_count` reset (2255) and the watchdog-arm block (2275-2299); keep the config-push/ctrlhello XCE_TESTING hooks; route promotion side effects through the same shared sequence (noop+gate tail guarded on persist/engine availability, as in production) - Observable completion: retargeted legacy-path tests PASS; lifecycle test for symmetry PASSES (both entry points produce identical role/hook/heartbeat/no-watchdog state) @@ -84,4 +84,6 @@ - **(2.2) Expected category-(a) failures open from task 2.2 until retargeted**: after the raft path enters `CTRL_PRIMARY` directly, three single-node tests still assert `CTRL_PRIMARY_COMMITTING (4)` and FAIL (`Expected 4 Was 0`): `test_single_node_shortcircuit_evaluates_immediately` + `test_single_node_not_primary_directly` (test_ha_start_election_async.c) → retargeted by **task 3.1**; `test_nonblocking_single_node_election_enters_committing` (test_ha_startup.c) → retargeted by **task 5.1**. Single-node election reaches `raft_cb_become_leader` via the `raft_start_election` shortcircuit firing `on_become_leader`. - **(2.2) ha.c shared sequence**: raft path stores `CTRL_PRIMARY` directly with NO `ha_do_promote` (term synced at entry — avoids double-increment); `ha_promote_to_primary` keeps `ha_do_promote` (owns its term). Both call static `ha_register_persist_hooks(c)` (BEFORE noop append) and static `ha_arm_own_term_gate(c)` (`persist_wal_async(WAL_NOOP)` → `noop_lsn = current_lsn-1` under p->lock → `commit_submit(RETAIN)` → `commit_set_writable_lsn`); append-failure degrades safe (gate unset, promotion completes); non-blocking. `committing_timeout_fn` + enum retained until tasks 4.x/5.2. - **Unity runner gotcha**: the aggregated controller runner prints the PREVIOUS passing test's `file:line` as a stale prefix on a FAIL line — attribute failures by test NAME, not the printed file:line prefix. +- **(3.1) Legacy path routes through BARE helpers, not `ha_promote_to_primary`**: `ha_elect_evaluate_and_signal`'s PRIMARY branch calls `ha_register_persist_hooks` + `ha_start_primary_heartbeat` + `ha_arm_own_term_gate` directly (matching the raft path), NOT `ha_promote_to_primary` — because the latter calls `ha_do_promote` (term increment) and the legacy branch historically has NO term store/increment (research §4 finding 2). This is a deliberate deviation from design §283's loose wording ("route through ha_promote_to_primary / ha_register_persist_hooks"); the bare-helper route preserves term semantics and is the correct reading. +- **(3.1) Expected-failure set after task 3.1 (8 category-(a), all owned downstream)**: task 4.1 DELETES `test_ha_evaluate_committing.c` (3 fails: test_winning_quorum_enters_committing, test_committing_resets_repl_ack_count, test_election_timeout_fn_fires_evaluate); task 5.1 retargets `test_nonblocking_single_node_election_enters_committing` (test_ha_startup.c), `test_3node_win_calls_both_pushes` (test_ha_repl_push.c), and 3 RDHUP cases (test_ha_rdhup_integration.c: rdhup_full_path_reaches_committing, rdhup_committing_to_primary_on_ack, rdhup_3node_one_ack_promotes). `committing_timeout_fn` now has a benign `-Wunused-function` warning (no arming site left) until task 4.1 deletes it. - **Lifecycle fixture (test_ha_role_lifecycle.c)**: `lifecycle_cluster_setup` installs `repl_broadcast_send_hook = lc_drop_send_hook` (drop-only, reset to NULL in teardown) — required once promotion broadcasts the WAL_NOOP, else fake `int` endpoints cause an ASAN overflow. Gate cases prime `current_lsn = noop_lsn` so the post-increment append lands the noop at `noop_lsn` (production derives `noop_lsn = current_lsn-1`). diff --git a/src/controller/ha.c b/src/controller/ha.c index 38150b5..f8d9399 100644 --- a/src/controller/ha.c +++ b/src/controller/ha.c @@ -2346,16 +2346,21 @@ apply_role: ha->phase2_done = 1; } - /* Enter COMMITTING: block job traffic until majority ack (Req 4.1, 8.1) */ - atomic_store_explicit(&ha->role, (int32_t)CTRL_PRIMARY_COMMITTING, + /* ha-single-leader-role task 3.1 (Req 1.1, 1.5): enter CTRL_PRIMARY + * DIRECTLY — symmetric with raft_cb_become_leader's rewrite. There + * is no transitional COMMITTING state and no committing-timeout + * watchdog; new-leader safety comes from the own-term WAL_NOOP + + * writable_lsn gate armed below (ha_arm_own_term_gate), not from a + * fourth role value. The legacy XCE_TESTING election path OWNS its + * term (no raft library in test mode); it historically did NOT + * increment the term in this branch, so we preserve that — no + * ha_do_promote, no new term store (research.md §4 finding 2). */ + atomic_store_explicit(&ha->role, (int32_t)CTRL_PRIMARY, memory_order_release); - INFO("HA FSM: → CTRL_PRIMARY_COMMITTING (term=%" PRIu64 + INFO("HA FSM: → CTRL_PRIMARY (term=%" PRIu64 ", first_boot=%d)", atomic_load_explicit(&ha->term, memory_order_acquire), is_first_boot); - /* Reset ack counter for this committing round (Req 5.2) */ - atomic_store(&ha->repl_ack_count, 0); - /* Push config to standbys with lower config_tag (Req 7.1) */ #ifdef XCE_TESTING if (ha_push_config_hook != NULL) @@ -2374,30 +2379,25 @@ apply_role: ha_send_ctrlhello_to_all_standbys(c); #endif - /* Arm committing timeout watchdog (degraded-mode fallback, Req 8.4) */ + /* ha-single-leader-role task 3.1: route promotion side effects + * through the SAME shared sequence raft_cb_become_leader runs so the + * test-driven (legacy) and raft-driven promotion paths cannot diverge + * (design §Legacy Path Symmetry). Guarded on a real cluster_t — the + * helpers dereference &c->hooksys / &c->persist / &c->commit, and the + * unit-test evaluation path passes c == NULL. Identical role / hook / + * heartbeat / no-watchdog / own-term-gate state results either road. */ if (c) { - /* Cancel any prior committing timeout */ - committing_timeout_ctx_t *prev = (committing_timeout_ctx_t *) - atomic_exchange(&ha->committing_timeout_ctx, NULL); - if (prev) { - atomic_store(&prev->cancelled, 1); - } - committing_timeout_ctx_t *ct = calloc(1, sizeof(*ct)); - if (ct) { - atomic_init(&ct->cancelled, 0); - ct->ha = ha; - ct->c = c; - atomic_store(&ha->committing_timeout_ctx, ct); - int rc = delay_task_ms(&c->delaymgr, - (int)ha->config.committing_timeout_ms, - 1, committing_timeout_fn, ct); - if (rc < 0) { - WARN("ha_elect_evaluate_and_signal: delay_task_ms " - "for committing timeout failed: %d", rc); - atomic_store(&ha->committing_timeout_ctx, NULL); - free(ct); - } - } + /* Register the persistence hooks BEFORE the own-term no-op append, + * so the persistence seam is live before any racing client write. + * Idempotent via the hooks_registered guard. */ + ha_register_persist_hooks(c); + /* Periodic heartbeat to standbys (idempotency guard prevents + * double-arming if already started). */ + ha_start_primary_heartbeat(c); + /* Own-term WAL_NOOP + writable_lsn gate (Req 1.4, 2.1, 2.2) — + * non-blocking; append failure degrades safe (gate unset, + * promotion still completes). */ + ha_arm_own_term_gate(c); } } else { /* STANDBY result */ diff --git a/src/controller/testsuite/test_ha_role_lifecycle.c b/src/controller/testsuite/test_ha_role_lifecycle.c index 7c1bc9c..a271235 100644 --- a/src/controller/testsuite/test_ha_role_lifecycle.c +++ b/src/controller/testsuite/test_ha_role_lifecycle.c @@ -28,6 +28,7 @@ #include #include #include +#include #include #include #include @@ -1053,6 +1054,113 @@ static void test_standalone_promotion_gate_opens_immediately(void) #endif } +/* ── Case 8: LEGACY/RAFT PROMOTION SYMMETRY (ha-single-leader-role task 3.1, + * Req 1.1, 1.5; design §Legacy Path Symmetry). Both entry points to PRIMARY + * must produce IDENTICAL observable state so test-driven and raft-driven + * promotion cannot diverge: + * + * (1) raft road — raft_cb_become_leader_for_test (the XCE_TESTING shim, which + * forces ha->raft_c == NULL): only the c-independent state is observable + * on this seam (role == CTRL_PRIMARY). The hook/gate tail is c!=NULL- + * guarded and skipped here — that half of the contract is pinned on the + * real-cluster road below (matching cases 3/4/5/7, which drive the SAME + * shared helpers ha_register_persist_hooks / ha_arm_own_term_gate). + * + * (2) legacy road — ha_elect_evaluate_and_signal on the real g_lc cluster + * (ctx->c = &g_lc, single-node quorum win): drives the full shared + * sequence. Must land role == CTRL_PRIMARY, hooks_registered == 1, + * writable_lsn == noop_lsn (own-term gate armed), and arm NO delaymgr + * watchdog — byte-for-byte the state ha_promote_to_primary produces. + * + * Pin: the legacy real-cluster path produces role/hook/gate/no-watchdog state + * identical to the raft road's role and to ha_promote_to_primary's full state. + * RED before task 3.1: the legacy branch stored CTRL_PRIMARY_COMMITTING, reset + * repl_ack_count, and armed the committing watchdog — diverging from the raft + * road on every axis. */ +static void test_legacy_and_raft_promotion_are_symmetric(void) +{ +#ifdef XCE_TESTING + /* ── Road 1: raft shim (c == NULL) — role contract only ──────────────── */ + ha_state_t ha_raft; + ha_config_t cfg = lifecycle_ha_cfg(); + TEST_ASSERT_EQUAL_INT(0, ha_init(&ha_raft, &cfg, 0)); + ha_raft.phase2_done = 1; + atomic_store_explicit(&ha_raft.role, (int32_t)CTRL_ELECTING, + memory_order_release); + raft_cb_become_leader_for_test(&ha_raft, /*term=*/7); + ctrl_role_t raft_role = + (ctrl_role_t)atomic_load_explicit(&ha_raft.role, memory_order_acquire); + TEST_ASSERT_EQUAL_INT_MESSAGE((int)CTRL_PRIMARY, (int)raft_role, + "raft road must land DIRECTLY in CTRL_PRIMARY (Req 1.1)"); + ha_fini(&ha_raft); + + /* ── Road 2: legacy ha_elect_evaluate_and_signal on a real cluster ───── */ + /* persist_wal_async appends at current_lsn then post-increments, and + * ha_arm_own_term_gate derives noop_lsn = current_lsn - 1; the fixture + * primes current_lsn = appended_lsn + 1, so passing appended_lsn = + * noop_lsn - 1 lands the own-term no-op at exactly lsn = noop_lsn + * (same idiom as case 5). */ + const uint64_t noop_lsn = 9; + (void)lifecycle_cluster_setup("sym", /*n_peers=*/1, + /*n_standbys=*/0, + /*appended_lsn=*/noop_lsn - 1, + /*n_vcpus=*/8); + int before = delaymgr_task_count(&g_lc.delaymgr); + TEST_ASSERT_EQUAL_INT_MESSAGE(0, g_lc.ha.hooks_registered, + "fixture precondition: hooks must be unregistered before promotion"); + + /* Single-node quorum win: n_peers==0 shortcircuit (votes[] empty), + * votes_granted=1 satisfies quorum=1. ctx->c = &g_lc so the shared + * promotion sequence runs against the real cluster. */ + ha_elect_ctx_t *ctx = calloc(1, sizeof(*ctx)); + TEST_ASSERT_NOT_NULL_MESSAGE(ctx, "ctx calloc failed"); + ctx->n_peers = 0; + ctx->votes_granted = 1; + ctx->n_replied = 0; + ctx->result = -1; + ctx->done_posted = 0; + ctx->c = &g_lc; + pthread_mutex_init(&ctx->lock, NULL); + sem_init(&ctx->done, 0, 0); + + ctrl_role_t legacy_result = ha_elect_evaluate_and_signal(&g_lc.ha, ctx); + + /* Symmetry axis 1: the legacy road reaches PRIMARY (matches the raft road). */ + TEST_ASSERT_EQUAL_INT_MESSAGE((int)CTRL_PRIMARY, (int)legacy_result, + "legacy evaluate must RESULT in CTRL_PRIMARY (Req 1.1)"); + ctrl_role_t legacy_role = + (ctrl_role_t)atomic_load_explicit(&g_lc.ha.role, memory_order_acquire); + TEST_ASSERT_EQUAL_INT_MESSAGE((int)raft_role, (int)legacy_role, + "legacy and raft promotion must land in the SAME role (CTRL_PRIMARY)"); + + /* Symmetry axis 2: persist hooks registered by the shared sequence. */ + TEST_ASSERT_EQUAL_INT_MESSAGE(1, g_lc.ha.hooks_registered, + "legacy road must register persist hooks via the shared sequence (Req 2.1)"); + + /* Symmetry axis 3: own-term gate armed at the no-op LSN. */ + TEST_ASSERT_EQUAL_UINT64_MESSAGE(noop_lsn, g_lc.commit.writable_lsn, + "legacy road must arm the own-term gate at noop_lsn (Req 1.4, 2.1) — " + "identical to the raft road's shared tail"); + + /* Symmetry axis 4: NO committing-timeout watchdog armed (Req 1.5). */ + int after = delaymgr_task_count(&g_lc.delaymgr); + TEST_ASSERT_EQUAL_INT_MESSAGE(before, after, + "legacy road must arm NO delaymgr watchdog (Req 1.5) — symmetric with " + "the raft road; the committing-timeout arm is deleted from this path"); + + pthread_mutex_destroy(&ctx->lock); + sem_destroy(&ctx->done); + free(ctx); + lifecycle_cluster_teardown(); + printf("test_legacy_and_raft_promotion_are_symmetric: PASS " + "(role=%d both roads; hooks=1; writable_lsn=%" PRIu64 + "; watchdog tasks before=%d after=%d)\n", + (int)legacy_role, g_lc.commit.writable_lsn, before, after); +#else + printf("test_legacy_and_raft_promotion_are_symmetric: SKIP (non-test build)\n"); +#endif +} + /* ------------------------------------------------------------------ */ /* Entry point (registered in controller_test.c / meson.build) */ /* ------------------------------------------------------------------ */ @@ -1073,4 +1181,7 @@ void test_ha_role_lifecycle(void) RUN_TEST(test_promotion_appends_noop_and_arms_own_term_gate); /* case 5 */ RUN_TEST(test_pre_own_term_wait_has_no_third_path); /* case 6 */ RUN_TEST(test_standalone_promotion_gate_opens_immediately); /* case 7 */ + + /* Task 3.1 — legacy/raft promotion symmetry (Req 1.1, 1.5) */ + RUN_TEST(test_legacy_and_raft_promotion_are_symmetric); /* case 8 */ } diff --git a/src/controller/testsuite/test_ha_start_election_async.c b/src/controller/testsuite/test_ha_start_election_async.c index 28624d0..3e8680b 100644 --- a/src/controller/testsuite/test_ha_start_election_async.c +++ b/src/controller/testsuite/test_ha_start_election_async.c @@ -4,9 +4,11 @@ * * 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_COMMITTING; task 4.4 committing timeout - * will later promote to CTRL_PRIMARY for degraded/single-node mode). + * immediately after ha_start_election_async returns. The single-node + * shortcircuit delegates to raft_start_election, which fires on_become_leader + * synchronously → raft_cb_become_leader (ha-single-leader-role task 2.2), + * so the role lands DIRECTLY in CTRL_PRIMARY — there is no transitional + * CTRL_PRIMARY_COMMITTING and no committing-timeout watchdog (Req 1.1, 1.5). * - Function returns in < 5ms (observable via clock_gettime). * - n_peers=1 shortcircuit stores n_peers in ha->n_peers. * @@ -101,7 +103,7 @@ static void test_single_node_stores_n_peers(void) printf("test_single_node_stores_n_peers: PASS\n"); } -/* ── Test 3: single-node shortcircuit enters CTRL_PRIMARY_COMMITTING ──────── */ +/* ── Test 3: single-node shortcircuit enters CTRL_PRIMARY directly ───────── */ static void test_single_node_shortcircuit_evaluates_immediately(void) { @@ -111,22 +113,24 @@ 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. - * 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). */ + /* n_peers=1: the shortcircuit fires raft_start_election → on_become_leader + * (raft_cb_become_leader) synchronously. ha-single-leader-role (Req 1.1): + * a winning node lands DIRECTLY in CTRL_PRIMARY — there is no transitional + * CTRL_PRIMARY_COMMITTING. New-leader safety comes from the own-term + * WAL_NOOP + writable_lsn gate, not from a fourth role value. */ 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_EQUAL_INT_MESSAGE((int)CTRL_PRIMARY_COMMITTING, (int)role, - "single-node shortcircuit must enter CTRL_PRIMARY_COMMITTING (not CTRL_PRIMARY)"); + TEST_ASSERT_EQUAL_INT_MESSAGE((int)CTRL_PRIMARY, (int)role, + "single-node shortcircuit must enter CTRL_PRIMARY directly (Req 1.1) — " + "no transitional CTRL_PRIMARY_COMMITTING"); ha_fini(&ha); - printf("test_single_node_shortcircuit_evaluates_immediately: PASS (role=%d == CTRL_PRIMARY_COMMITTING)\n", + printf("test_single_node_shortcircuit_evaluates_immediately: PASS (role=%d == CTRL_PRIMARY)\n", (int)role); } -/* ── Test 6: single-node shortcircuit does NOT go directly to CTRL_PRIMARY ── */ +/* ── Test 6: single-node shortcircuit lands directly in CTRL_PRIMARY ──────── */ static void test_single_node_not_primary_directly(void) { @@ -135,13 +139,17 @@ static void test_single_node_not_primary_directly(void) ha_start_election_async(&ha, NULL, 1, NULL, 0ULL, 0ULL, "ctrl-a"); + /* ha-single-leader-role (Req 1.1): the winning node lands in CTRL_PRIMARY + * immediately — the old "must go through COMMITTING first" contract is gone. + * Promotion arms NO committing-timeout watchdog (Req 1.5); the own-term gate + * (WAL_NOOP + writable_lsn) provides new-leader safety instead. */ 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"); + TEST_ASSERT_EQUAL_INT_MESSAGE((int)CTRL_PRIMARY, (int)role, + "single-node shortcircuit must land DIRECTLY in CTRL_PRIMARY (Req 1.1) — " + "no transitional CTRL_PRIMARY_COMMITTING, no committing watchdog (Req 1.5)"); ha_fini(&ha); - printf("test_single_node_not_primary_directly: PASS (role=%d)\n", (int)role); + printf("test_single_node_not_primary_directly: PASS (role=%d == CTRL_PRIMARY)\n", (int)role); } /* ── Test 4: ha->n_peers set for multi-peer count too ───────────────────── */