diff --git a/src/controller/ha.c b/src/controller/ha.c index 7c59fb7f..ce272694 100644 --- a/src/controller/ha.c +++ b/src/controller/ha.c @@ -3957,19 +3957,35 @@ apply_role: * - votes_granted reaches quorum (fast path — no need to wait for all replies), OR * - n_replied reaches n_peers (all peers accounted for, win or lose) */ -void handle_request_vote_reply(ha_state_t *ha, - ha_elect_ctx_t *ctx, - int peer_idx, - uint64_t term, - int vote_granted, - uint64_t peer_config_tag, - uint64_t peer_lsn) +/* + * ha_record_vote_reply: record a decoded vote reply into ctx->votes[peer_idx] + * WITHOUT signalling the legacy promotion path (bug-828). + * + * Records the granting flag, peer config_tag, and peer LSN under ctx->lock with + * the same idempotency guard handle_request_vote_reply uses, and updates the + * vote tally + freshest-responder tracking. It does NOT call + * ha_elect_evaluate_and_signal — so it never posts ctx->done and never runs the + * legacy apply_role promotion side-effects. + * + * This is the seam the PRODUCTION vote_reply_dispatch path calls so the granting + * voter's advertised LSN reaches ha_compute_safe_committed_lsn (via ctx->votes[]) + * while the raft library remains the SOLE production promotion driver + * (raft_on_vote_reply → on_become_leader). Routing the record through here (not + * handle_request_vote_reply) avoids a second, racing promotion signal. + * + * Returns non-zero if this reply newly reached the legacy quorum/all-replied + * threshold (so handle_request_vote_reply knows to signal); 0 otherwise. + * Must be called WITHOUT ctx->lock held (it takes the lock internally). + */ +static int ha_record_vote_reply_locked(ha_elect_ctx_t *ctx, + int peer_idx, + int vote_granted, + uint64_t peer_config_tag, + uint64_t peer_lsn) { - (void)term; /* fencing: callers bump ha->term if peer's term > ours, before calling here */ - /* Guard against out-of-range index */ if (peer_idx < 0 || peer_idx >= ctx->n_peers) - return; + return 0; pthread_mutex_lock(&ctx->lock); @@ -3978,7 +3994,7 @@ void handle_request_vote_reply(ha_state_t *ha, /* Idempotency guard: ignore duplicate replies */ if (entry->replied) { pthread_mutex_unlock(&ctx->lock); - return; + return 0; } /* Record reply fields */ @@ -4000,18 +4016,63 @@ void handle_request_vote_reply(ha_state_t *ha, ctx->max_resp_lsn = peer_lsn; } - /* Decide whether to signal the waiting thread. - * Quorum = ceil((n_peers + 1) / 2) where n_peers+1 is total cluster size. - * n_peers is the peer count excluding self; self-vote is already in votes_granted. - * ha_elect_evaluate_and_signal is called under ctx->lock; its done_posted guard - * prevents double sem_post if two threads reach quorum simultaneously. */ + /* Compute whether the legacy quorum/all-replied threshold is now reached. + * Quorum = ceil((n_peers + 1) / 2) where n_peers+1 is total cluster size. */ int total_nodes = ctx->n_peers + 1; int quorum = (total_nodes + 1) / 2; - - if (ctx->votes_granted >= quorum || ctx->n_replied >= ctx->n_peers) - ha_elect_evaluate_and_signal(ha, ctx); + int reached = (ctx->votes_granted >= quorum || ctx->n_replied >= ctx->n_peers); pthread_mutex_unlock(&ctx->lock); + return reached; +} + +/* Public record-only seam (bug-828): records the reply but never signals the + * legacy promotion path. Used by the production vote_reply_dispatch path. */ +void ha_record_vote_reply(ha_state_t *ha, + ha_elect_ctx_t *ctx, + int peer_idx, + uint64_t term, + int vote_granted, + uint64_t peer_config_tag, + uint64_t peer_lsn) +{ + (void)ha; /* fencing: caller bumps ha->term before calling here */ + (void)term; + if (!ctx) + return; + (void)ha_record_vote_reply_locked(ctx, peer_idx, vote_granted, + peer_config_tag, peer_lsn); +} + +void handle_request_vote_reply(ha_state_t *ha, + ha_elect_ctx_t *ctx, + int peer_idx, + uint64_t term, + int vote_granted, + uint64_t peer_config_tag, + uint64_t peer_lsn) +{ + (void)term; /* fencing: callers bump ha->term if peer's term > ours, before calling here */ + + if (peer_idx < 0 || peer_idx >= ctx->n_peers) + return; + + /* Record the reply (shared with the production record-only seam) and learn + * whether the legacy quorum/all-replied threshold is now reached. */ + int reached = ha_record_vote_reply_locked(ctx, peer_idx, vote_granted, + peer_config_tag, peer_lsn); + + /* Decide whether to signal the waiting thread. + * ha_elect_evaluate_and_signal takes ctx->lock; its done_posted guard + * prevents double sem_post if two threads reach quorum simultaneously. + * Skip if the index was out of range / a duplicate reply (reached==0 and + * nothing recorded) — the duplicate case is already idempotent in + * ha_record_vote_reply_locked, which returns 0 without re-counting. */ + if (reached) { + pthread_mutex_lock(&ctx->lock); + ha_elect_evaluate_and_signal(ha, ctx); + pthread_mutex_unlock(&ctx->lock); + } } /* ── Task 2.2: iomgr-native vote send path ────────────────────────────────── @@ -4182,7 +4243,21 @@ static void vote_reply_dispatch(ha_state_t *ha, ha_elect_ctx_t *ctx, int idx, return; } #endif - (void)ctx; (void)peer_tag; (void)peer_lsn; /* consumed by raft library via callbacks */ + /* bug-828 (PRIMARY fix): route the decoded reply into the election context + * BEFORE driving the raft library. Without this, the granting voter's + * advertised peer_lsn was discarded — ctx->votes[idx].vote_granted stayed 0, + * so a clean failover's ha_compute_safe_committed_lsn (read in + * raft_cb_become_leader / on_become_leader) saw only the self-vote + * (n_granted=1) and truncated committed entries down to self_lsn. + * + * Record-only: ha_record_vote_reply records ctx->votes[idx] under ctx->lock + * but does NOT post ctx->done / run the legacy apply_role promotion path. + * raft_on_vote_reply remains the SOLE production promotion driver (it owns + * the quorum tally and fires on_become_leader) — so there is no double + * promotion. `idx` is vctx->peer_idx, the local ctx->votes[] index (set in + * raft_cb_send_vote_request via the absolute→local mapping), which is exactly + * the index ha_compute_safe_committed_lsn and the test hook use. */ + ha_record_vote_reply(ha, ctx, idx, term, vote_granted, peer_tag, peer_lsn); raft_on_vote_reply(&ha->raft, idx, vote_granted, term); } diff --git a/src/controller/ha.h b/src/controller/ha.h index 17531b64..3746d4ba 100644 --- a/src/controller/ha.h +++ b/src/controller/ha.h @@ -992,6 +992,29 @@ void handle_request_vote_reply(ha_state_t *ha, uint64_t peer_config_tag, uint64_t peer_lsn); +/* + * ha_record_vote_reply: record-only vote-reply seam (bug-828). + * + * Records ctx->votes[peer_idx] (granted flag + peer_config_tag + peer_lsn) under + * ctx->lock with the same idempotency guard as handle_request_vote_reply, and + * updates the vote tally / freshest-responder tracking — but does NOT call + * ha_elect_evaluate_and_signal (never posts ctx->done, never runs the legacy + * promotion side-effects). The PRODUCTION vote_reply_dispatch path calls this so + * the granting voter's advertised LSN reaches ha_compute_safe_committed_lsn via + * ctx->votes[], while the raft library remains the sole production promotion + * driver (raft_on_vote_reply → on_become_leader) — no double-promotion. + * + * peer_idx: 0-based index into ctx->votes[] (same convention as + * handle_request_vote_reply; the absolute→local mapping is done by the caller). + */ +void ha_record_vote_reply(ha_state_t *ha, + ha_elect_ctx_t *ctx, + int peer_idx, + uint64_t term, + int vote_granted, + uint64_t peer_config_tag, + uint64_t peer_lsn); + /* * xce_probe_config_tag: single-RTT OP_probeConfigTag RPC to one peer. * Fills *tag_out with the peer's current config_tag on success. diff --git a/src/controller/persist.c b/src/controller/persist.c index 753e4b69..1041e899 100644 --- a/src/controller/persist.c +++ b/src/controller/persist.c @@ -933,6 +933,25 @@ int persist_wal_write_replicated(persist_t* p, uint64_t lsn, uint32_t type, return -EIO; } + /* bug-828 secondary: the entry is now fdatasync'd durable, so advance + * durable_lsn to the synced tip immediately (mirror persist_flush). The + * 10ms cadence flusher would otherwise advance it lazily, leaving this + * standby's ADVERTISED durable position (persist_durable_floor → + * RequestVoteReply.peerLastLogLsn) lagging its own synced tip during the + * vote window. A standby that has durably persisted the committed prefix + * must advertise it at once so a clean failover's safe-commit-from-votes + * computation sees the true majority-held LSN. Monotonic max; preserves + * durable_lsn <= current_lsn - 1 (tip read under this same p->lock, after + * the current_lsn advance above). Reached ONLY past the fail-loud rc-gate + * — never advances on an fdatasync error. */ + { + uint64_t tip = (p->current_lsn > 0) ? p->current_lsn - 1 : 0; + uint64_t dur = atomic_load_explicit(&p->durable_lsn, + memory_order_acquire); + if (tip > dur) + atomic_store_explicit(&p->durable_lsn, tip, memory_order_release); + } + /* Rotate segment when full (same threshold as the primary append). */ if (p->seg_offset >= WAL_SEGMENT_SIZE_BYTES) { if (rotate_segment(p) != 0) { diff --git a/src/controller/testsuite/test_ha_election.c b/src/controller/testsuite/test_ha_election.c index 9b9d5954..95fd213c 100644 --- a/src/controller/testsuite/test_ha_election.c +++ b/src/controller/testsuite/test_ha_election.c @@ -116,6 +116,17 @@ void handle_request_vote_reply(ha_state_t *ha, ha_elect_ctx_t *ctx, int vote_granted, uint64_t peer_config_tag, uint64_t peer_lsn); +/* bug-828: record-only vote-reply seam. Records ctx->votes[peer_idx] (granted + * flag + peer_tag + peer_lsn) under ctx->lock with the idempotency guard, but + * does NOT call ha_elect_evaluate_and_signal (the raft library owns promotion + * in production). This is the helper the production vote_reply_dispatch path + * now invokes so the granting voter's advertised LSN reaches the election ctx + * and ha_compute_safe_committed_lsn can compute the true majority-held LSN. */ +void ha_record_vote_reply(ha_state_t *ha, ha_elect_ctx_t *ctx, + int peer_idx, uint64_t term, + int vote_granted, + uint64_t peer_config_tag, uint64_t peer_lsn); + /* ── Minimal ha_config_t used across all election tests ─────────────────── */ static ha_config_t g_elect_cfg = { @@ -927,6 +938,69 @@ static void test_reply_handler_all_denied_posts_done_on_last_reply(void) printf("test_reply_handler_all_denied_posts_done_on_last_reply: PASS\n"); } +/* ── bug-828: safe-commit-from-votes must reflect the voted majority ───────── + * + * Production root cause: vote_reply_dispatch decoded the granting voter's + * advertised LSN but discarded it, so ctx->votes[i].vote_granted stayed 0 and + * ha_compute_safe_committed_lsn saw only the self-vote (n_granted=1) → + * safe_committed_lsn = self_lsn. A standby promoting with self_lsn=1 then + * truncated committed entries 2..5. + * + * This test drives the record-only seam (ha_record_vote_reply) that the + * production path now invokes: two peers GRANT advertising peer_lsn=5; the + * candidate's own self_lsn=1. ha_compute_safe_committed_lsn must return the + * majority-held LSN (5), NOT self_lsn (1). + * + * RED before the fix: ha_record_vote_reply does not exist (undefined reference); + * and even if grants were not recorded, safe_committed would be self_lsn=1. + */ +static void test_safe_commit_reflects_granting_voter_lsn(void) +{ + /* 3-node cluster: n_peers=2 other peers. */ + ha_state_t ha; + ha_init(&ha, &g_elect_cfg, 0); + atomic_store_explicit(&ha.term, 1ULL, memory_order_release); + + ha_elect_ctx_t *ctx = alloc_elect_ctx(2); + + /* Candidate's own (boot) position: durable floor 1, the catch-up node case. */ + ctx->self_lsn = 1ULL; + ctx->self_tag = 100ULL; + ctx->votes[0].my_tag = 100ULL; + ctx->votes[0].my_lsn = 1ULL; + + /* Both peers GRANT, each advertising a durable position of LSN 5 + * (they hold the committed prefix). Record via the production seam. */ + ha_record_vote_reply(&ha, ctx, 0, 1ULL, 1, 100ULL, 5ULL); + ha_record_vote_reply(&ha, ctx, 1, 1ULL, 1, 100ULL, 5ULL); + + /* The recording must NOT have signalled the legacy promotion path — + * ha_record_vote_reply records only; the raft library owns promotion. + * ctx->done must remain unposted (no double-promotion). */ + int rc = sem_trywait(&ctx->done); + TEST_ASSERT_EQUAL_INT_MESSAGE(-1, rc, + "ha_record_vote_reply must NOT post ctx->done (record-only; no double-promotion)"); + TEST_ASSERT_EQUAL_INT(EAGAIN, errno); + + /* Grants must be recorded into the election context. */ + TEST_ASSERT_EQUAL_INT(1, ctx->votes[0].vote_granted); + TEST_ASSERT_EQUAL_UINT64(5ULL, ctx->votes[0].peer_lsn); + TEST_ASSERT_EQUAL_INT(1, ctx->votes[1].vote_granted); + TEST_ASSERT_EQUAL_UINT64(5ULL, ctx->votes[1].peer_lsn); + + /* The binding assertion: safe_committed_lsn = majority-held LSN = 5, + * NOT self_lsn (1). granted={self=1, peer0=5, peer1=5} → median = 5. */ + uint64_t safe_lsn = 0; + uint64_t safe_tag = 0; + ha_compute_safe_committed_lsn(ctx, &safe_lsn, &safe_tag); + TEST_ASSERT_EQUAL_UINT64_MESSAGE(5ULL, safe_lsn, + "safe_committed_lsn must reflect the majority-durable LSN (5), not self_lsn (1)"); + + free_elect_ctx(ctx); + ha_fini(&ha); + printf("test_safe_commit_reflects_granting_voter_lsn: PASS\n"); +} + /* ── Test suite entry point ───────────────────────────────────────────────── */ void test_ha_election(void) @@ -982,4 +1056,7 @@ void test_ha_election(void) RUN_TEST(test_reply_handler_quorum_reached_posts_done); RUN_TEST(test_reply_handler_partial_replies_do_not_post_done); RUN_TEST(test_reply_handler_all_denied_posts_done_on_last_reply); + + /* bug-828 — safe-commit-from-votes must reflect the granting voter LSN */ + RUN_TEST(test_safe_commit_reflects_granting_voter_lsn); } diff --git a/src/controller/testsuite/test_ha_vote_gate.c b/src/controller/testsuite/test_ha_vote_gate.c index 120e6e4c..4417ebd5 100644 --- a/src/controller/testsuite/test_ha_vote_gate.c +++ b/src/controller/testsuite/test_ha_vote_gate.c @@ -967,6 +967,93 @@ static void test_longest_durable_candidate_grantable_no_livelock(void) printf("test_longest_durable_candidate_grantable_no_livelock: PASS\n"); } +/* ══════════════════════════════════════════════════════════════════════════ + * bug-828 part 2 — a standby caught up POST-BOOT must advertise its LIVE durable + * tip, not its (lower) boot floor, and that live tip must DOMINATE a lagging + * learned_committed. + * + * Live failover evidence (fix828c_r1..r3): after a clean failover the two + * surviving voters advertised position 1 and ha_compute_safe_committed_lsn + * returned safe=1 — but in those runs the standbys were GENUINELY durable only + * at LSN 1 (their WAL physically held 1..2; the committed-prefix loss is the + * UPSTREAM replication/commit-barrier durability gap bug-795/661, where the + * primary advanced committed_lsn to 5 before the surviving majority was durable + * there — out of scope here). + * + * This test pins the IN-SCOPE contract: GIVEN a standby that IS durable at its + * live tip (durable_lsn=5, advanced via persist_wal_write_replicated's inline + * fdatasync → the part-1 secondary fix, mirrored here by make_durable_persist's + * explicit flush), even though its learned_committed lags at 1, it must: + * (a) ADVERTISE 5 as both the candidate self-LSN (ha_candidate_advertise_lsn) + * and the voter floor LSN (ha_vote_self_freshness over persist_durable_floor), + * (b) so two such granting voters drive ha_compute_safe_committed_lsn to 5, + * NOT to the lagging learned_committed (1). + * + * SAFETY: the advertised position is the DURABLE (fdatasync'd) tip — a position + * the node can back from its own WAL — never a merely-received/appended tip. + * persist_durable_lsn == persist_durable_floor (both read p->durable_lsn, the + * fsync watermark); make_durable_persist advances it only via persist_flush, so + * the advertised 5 is fully durable. + * ════════════════════════════════════════════════════════════════════════ */ + +static void test_live_durable_tip_advertised_over_lagging_learned(void) +{ + /* Real persist durable at LSN 5 (explicit flush → durable_lsn=5). This is the + * caught-up standby: durable tip 5, learned_committed will lag at 1. */ + persist_t *p = make_durable_persist("/tmp/xce_828p2_XXXXXX", 5); + TEST_ASSERT_NOT_NULL(p); + TEST_ASSERT_EQUAL_UINT64(5ULL, persist_durable_floor(p)); + TEST_ASSERT_EQUAL_UINT64(5ULL, persist_durable_lsn(p)); /* same field */ + + /* (a) Candidate self-advertise = the BARE durable tip = 5 (never the boot + * floor, never the lagging learned value). */ + TEST_ASSERT_EQUAL_UINT64_MESSAGE(5ULL, ha_candidate_advertise_lsn(p), + "candidate must advertise its LIVE durable tip (5), the fsync'd watermark"); + + /* (a) Voter floor LSN over the durable tip, with learned_committed LAGGING + * at 1: max(durable_floor=5, learned=1) = 5. The durable tip dominates. */ + ha_state_t ha; + ha_init(&ha, &g_gate_cfg, 0); + atomic_store_explicit(&ha.committed_config_tag, 100ULL, memory_order_release); + atomic_store_explicit(&ha.last_known_committed_lsn, 1ULL, memory_order_release); + + uint64_t my_tag = 0, my_lsn = 0; + ha_vote_self_freshness(&ha, 100ULL /*local tag*/, + persist_durable_floor(p) /*durable local LSN = 5*/, + &my_tag, &my_lsn); + TEST_ASSERT_EQUAL_UINT64_MESSAGE(5ULL, my_lsn, + "voter floor LSN must be the LIVE durable tip (5), dominating a lagging " + "learned_committed (1) — this is what RequestVoteReply.peerLastLogLsn " + "advertises"); + + /* (b) Two granting voters each advertising 5 must drive the post-election + * safe-commit to 5, NOT to the lagging learned (1). Build a 3-node election + * ctx: self (the catch-up candidate, durable 5) + 2 peers both advertising 5. */ + ha_elect_ctx_t *ctx = + calloc(1, sizeof(*ctx) + 2 * sizeof(ctx->votes[0])); + TEST_ASSERT_NOT_NULL(ctx); + ctx->n_peers = 2; + ctx->self_lsn = ha_candidate_advertise_lsn(p); /* 5 */ + ctx->self_tag = 100ULL; + ctx->votes[0].vote_granted = 1; + ctx->votes[0].peer_lsn = 5ULL; + ctx->votes[0].peer_tag = 100ULL; + ctx->votes[1].vote_granted = 1; + ctx->votes[1].peer_lsn = 5ULL; + ctx->votes[1].peer_tag = 100ULL; + + uint64_t safe_lsn = 0, safe_tag = 0; + ha_compute_safe_committed_lsn(ctx, &safe_lsn, &safe_tag); + TEST_ASSERT_EQUAL_UINT64_MESSAGE(5ULL, safe_lsn, + "safe_committed_lsn must reflect the majority-durable tip (5), not the " + "lagging learned_committed (1) — the durable tip dominates"); + + free(ctx); + ha_fini(&ha); + free_durable_persist(p); + printf("test_live_durable_tip_advertised_over_lagging_learned: PASS\n"); +} + /* ── Test suite entry point ───────────────────────────────────────────────── */ void test_ha_vote_gate(void) @@ -998,6 +1085,10 @@ void test_ha_vote_gate(void) RUN_TEST(test_voter_floor_uses_durable_tip_denies_truncated); RUN_TEST(test_longest_durable_candidate_grantable_no_livelock); + /* bug-828 part 2 — a caught-up standby advertises its LIVE durable tip + * (dominating a lagging learned_committed); safe-commit-from-votes reaches it */ + RUN_TEST(test_live_durable_tip_advertised_over_lagging_learned); + /* Task 3.1 — voter-side PreVote dispatch gate (stickiness + freshness, * no mutation, reply term unchanged) */ RUN_TEST(test_pre_vote_primary_role_denies); diff --git a/src/controller/testsuite/test_repl_apply.c b/src/controller/testsuite/test_repl_apply.c index e197278c..b0e9722d 100644 --- a/src/controller/testsuite/test_repl_apply.c +++ b/src/controller/testsuite/test_repl_apply.c @@ -303,6 +303,14 @@ static void test_repl_write_persists_and_recovers(void) /* current_lsn advanced to lsn+1 */ TEST_ASSERT_EQUAL_UINT64(2, p->current_lsn); + /* bug-828 secondary: the entry was fdatasync'd inline before returning, so + * durable_lsn must reflect the synced tip (lsn 1) IMMEDIATELY — not lag + * behind waiting for the cadence flusher. A standby advertises this as its + * durable floor (RequestVoteReply.peerLastLogLsn) during a vote; if it lags, + * a clean failover's safe-commit-from-votes under-counts. RED before fix. */ + TEST_ASSERT_EQUAL_UINT64_MESSAGE(1ULL, persist_durable_lsn(p), + "durable_lsn must advance to the synced tip after persist_wal_write_replicated"); + /* A .wal segment must now exist on disk. */ TEST_ASSERT_TRUE(count_replwr_wal_files(tmpdir) >= 1); TEST_ASSERT_TRUE(total_replwr_wal_bytes(tmpdir) > 0);