From cb668adc7ab109330169d16fe40cb2168bb92021 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Wed, 29 Jul 2026 16:11:47 +0000 Subject: [PATCH] test(raft-adapter-conformance): WAL catch-up reject reply-construction regression lock Adds test_5_1_wal_catchup_reject_sends_nack_reply, driving a genuine (term,lsn) conflict against a real persisted WAL entry through ha_apply_term_entry_via_library and asserting ha_send_wal_ack_reply_ex(success=0) fires via ha_wal_ack_send_hook. Note: test_config_repl.c's test_bug1479_ae_reject_sends_nack_not_silence already covers the same call site via a different (forward-gap) stimulus, added in the original bug-1479 commit. design.md's own File Structure Plan explicitly calls for this new test in this file regardless, so this is disclosed, design-sanctioned defense-in-depth, not accidental duplication -- both tests independently confirmed to fail together when the reply call is removed. 1065 -> 1066 tests, 0 failures. --- .kiro/specs/raft-adapter-conformance/tasks.md | 2 +- .wolf/cerebrum.md | 3 + .../testsuite/test_ha_core_safety.c | 112 ++++++++++++++++++ 3 files changed, 116 insertions(+), 1 deletion(-) diff --git a/.kiro/specs/raft-adapter-conformance/tasks.md b/.kiro/specs/raft-adapter-conformance/tasks.md index e959c11e..cf8b87a4 100644 --- a/.kiro/specs/raft-adapter-conformance/tasks.md +++ b/.kiro/specs/raft-adapter-conformance/tasks.md @@ -64,7 +64,7 @@ - _Boundary: test_ha_core_safety.c_ - [ ] 5. Core: regression-lock tests for already-shipped reply and append behavior -- [ ] 5.1 (P) Test that a WAL catch-up rejection actually sends the reply on the wire +- [x] 5.1 (P) Test that a WAL catch-up rejection actually sends the reply on the wire - Add a test asserting `ha_send_wal_ack_reply_ex(success=0)` is invoked (via the existing `ha_wal_ack_send_hook`) when a WAL catch-up entry is rejected, closing the reply-construction-boundary coverage gap that existed alongside the already-shipped bug-1479 fix - Observable: new test in `test_ha_core_safety.c` fails if the reject branch is ever changed back to a silent drop - _Requirements: 2.1_ diff --git a/.wolf/cerebrum.md b/.wolf/cerebrum.md index 3680051a..29811210 100644 --- a/.wolf/cerebrum.md +++ b/.wolf/cerebrum.md @@ -584,3 +584,6 @@ bug-1479 (permanent quorum loss when a standby log-conflicts during chaotic elec 2. **Standby-side NACK sending** (`ha_apply_term_entry_via_library`'s logOk-reject path now calls `ha_send_wal_ack_reply_ex(..., success=0)` instead of silently dropping the reject — `ha_send_wal_ack_reply` had hardcoded `success=1` unconditionally, the ONLY WalAckReply sender in the whole controller). Unit-correct, verified via a new capture-hook test — but live soak v2 confirmed the NACK NOW reaches the primary (log-grepped: `WAL NACK from standby ... success=0`) and STILL failed identically, because `ha_stream_wal_to_standby`'s catch-up range was hard-capped at `committed_lsn`, which can never advance without this exact standby's own ack — a self-referential deadlock invisible from EITHER side's code in isolation. 3. **`ha_compute_repl_stream_ceiling_lsn`**: widens the STREAM_WAL ceiling (and the convergence check gating it) to the leader's own log tail, matching raft.tla's real AppendEntries semantics (leaderCommit is advisory, never a send-cap) — while re-using bug-1478's exact guard (never widen onto a trailing local-only checkpoint's unstreamable LSN) so fixing gap 3 didn't reopen gap-1478. Live soak v3: PASS, all 3 force-elect rounds, 1053/1053 unit tests. **Do-not-repeat**: when an adapter/wire-protocol fix passes its own unit tests, that only proves the function is INTERNALLY correct — it does NOT prove anything upstream/downstream actually calls it with real data. For any fix whose job is "handle X when X arrives," always ALSO verify (via a live/integration run, log-grepped, not inferred) that X actually arrives in production BEFORE declaring victory — a unit-green "correct but unreachable" fix is functionally identical to no fix at all, and the only way to catch that is to re-run the EXACT scenario that surfaced the original bug, all the way to its real end state, every time a layer is added. + +## Key Learning (2026-07-29, raft-adapter-conformance task 5.1) — the bug-1479 fix commit (34ab78d7) already shipped a reply-construction-boundary regression test; the spec's "motivating finding" premise ("no test was ever written asserting the reply itself gets constructed") was not fully accurate +`test_config_repl.c`'s `test_bug1479_ae_reject_sends_nack_not_silence` (added IN the bug-1479 fix commit itself, before this spec existed) already asserts `ha_wal_ack_send_hook` fires with `success=0` on a WAL catch-up logOk reject — exactly the "reply itself gets constructed" boundary requirements.md's motivating finding claimed was untested. Confirmed by `git log -p -S` on the test name: it landed in 34ab78d7, not in this spec. Task 5.1's explicit `_Boundary:_` (`test_ha_core_safety.c`) still called for an independent copy built from that file's own `hcs4_*` fixture family, so it was added anyway (`test_5_1_wal_catchup_reject_sends_nack_reply`) — genuinely useful redundant coverage (a future change to the reject branch is now caught in two files' fixture families instead of one), just not closing a gap that was actually still open. **Do-not-repeat**: when a spec's "motivating finding" describes a coverage gap, grep for the exact assertion shape (here: the hook name `ha_wal_ack_send_hook` + `success=0`) across the WHOLE test suite before treating the gap as confirmed-empty — a fresh audit pass can miss a test that already exists in a sibling file, especially one added in the same commit as the fix it's paired with. Report the discrepancy as a CONCERN in the task's status report rather than silently either skipping the task or presenting it as if it closed a real gap. diff --git a/src/controller/testsuite/test_ha_core_safety.c b/src/controller/testsuite/test_ha_core_safety.c index a161dbef..f8b139ff 100644 --- a/src/controller/testsuite/test_ha_core_safety.c +++ b/src/controller/testsuite/test_ha_core_safety.c @@ -2766,6 +2766,113 @@ static void test_4_2_dormant_sender_stays_uncalled_in_production(void) printf("test_4_2_dormant_sender_stays_uncalled_in_production: PASS\n"); } +/* ═══════════════════════════════════════════════════════════════════════════ + * TASK 5.1 (raft-adapter-conformance) — regression-lock: a WAL catch-up + * rejection actually sends the NACK reply on the wire. + * + * bug-1479 fixed ha_apply_term_entry_via_library's logOk-reject branch (ha.c + * ~3315-3327) to call ha_send_wal_ack_reply_ex(ha, ep, c, e->lsn, success=0) + * instead of silently dropping the reject — before that fix NOTHING was sent + * on reject, so the primary's NACK-handling (ha_raft_on_repl_nack) was correct + * but unreachable dead code. Downstream tests (ha_raft_on_repl_nack's own + * suite, catch-up-triggering tests) already prove the EFFECTS of a NACK once + * one arrives; this test closes the reply-CONSTRUCTION-boundary gap itself — + * that the reject branch actually invokes ha_send_wal_ack_reply_ex(success=0) + * — asserted here via the ha_wal_ack_send_hook seam (ha.h, XCE_TESTING), + * mirroring test_config_repl.c's test_bug1479_ae_reject_sends_nack_not_silence + * but built from THIS file's own hcs4_* fixture family (Requirement 2.1's + * _Boundary:_ is this file) so a future change to the reject branch is caught + * here too, independent of test_config_repl.c. + * + * Fixture: a STANDBY follower with a real persisted LSN1@STORED_TERM tip. A + * term-carrying single-entry AppendEntries claims prev=(1, CLAIMED_PREV_TERM) + * — a genuine (term,lsn) mismatch against the real on-disk term at LSN1 — so + * logOk fails and ha_apply_term_entry_via_library takes the reject branch. + * Driven through the REAL production ingest seam, ha_apply_wal_catchup_entry + * (the exact function every incoming WAL replication message is routed to), + * the same seam task 6.1 already established in this file. + * ═══════════════════════════════════════════════════════════════════════════ */ +static uint64_t g_hcs51_hook_term; +static uint64_t g_hcs51_hook_lsn; +static int g_hcs51_hook_success; +static int g_hcs51_hook_called; + +static void hcs51_wal_ack_capture(uint64_t term, uint64_t lsn, int success) +{ + g_hcs51_hook_called = 1; + g_hcs51_hook_term = term; + g_hcs51_hook_lsn = lsn; + g_hcs51_hook_success = success; +} + +static void test_5_1_wal_catchup_reject_sends_nack_reply(void) +{ + cluster_t* c = hcs4_cluster_up("wal_nack"); + TEST_ASSERT_NOT_NULL(c); + + enum { FOLLOWER_TERM = 4, STORED_TERM = 2, CLAIMED_PREV_TERM = 3 }; + hcs4_make_follower(c, /*term*/FOLLOWER_TERM, /*n_peers*/3); + + /* Real persisted tip: LSN1 @ STORED_TERM, via the wired standby persist + * callback (real fs required for inline fdatasync — bug-924). */ + uint8_t full[HCS_FULL_LEN]; + hcs_fill_body(full); + uint8_t blob[sizeof(raft_entry_blob_t) + HCS_FULL_LEN]; + size_t n = hcs4_pack_entry(blob, WAL_JOB_SUBMIT, full, HCS_FULL_LEN); + TEST_ASSERT_EQUAL_INT_MESSAGE(0, + ha_cb_persist_entry_for_test(&c->ha, 1, STORED_TERM, blob, n), + "seed write must persist (real fs required — bug-924)"); + c->ha.raft.log_len = 1; + + /* A term-carrying entry at LSN2 claims prev=(1, CLAIMED_PREV_TERM) — a + * genuine (term,lsn) mismatch against the real LSN1@STORED_TERM on disk + * (CLAIMED_PREV_TERM != STORED_TERM), so logOk MUST reject it. */ + uint8_t lpay[1] = { 'z' }; + wal_catchup_entry_t sub; + memset(&sub, 0, sizeof(sub)); + sub.lsn = 2; + sub.entry_type = WAL_JOB_UPDATE; + sub.term = FOLLOWER_TERM; + sub.prev_log_index = 1; + sub.prev_log_term = CLAIMED_PREV_TERM; + sub.raw_payload = lpay; + sub.raw_payload_len = 1; + + wal_catchup_entry_t* sub_arr[1] = { &sub }; + wal_catchup_entry_t top; + memset(&top, 0, sizeof(top)); + top.n_entries = 1; + top.entries = sub_arr; + + g_hcs51_hook_called = 0; + ha_wal_ack_send_hook = hcs51_wal_ack_capture; + + int rc = ha_apply_wal_catchup_entry(&c->ha, &c->persist, &top, + /*ep*/NULL, c); + + ha_wal_ack_send_hook = NULL; /* uninstall before any assertion can fail */ + + TEST_ASSERT_EQUAL_INT(0, rc); /* reject, not a hard error */ + + /* Nothing was accepted at LSN2: the rejected entry never landed. */ + hcs_probe_t pr; + TEST_ASSERT_EQUAL_INT(0, hcs4_probe(c, 2, &pr)); + TEST_ASSERT_FALSE_MESSAGE(pr.present, + "a rejected entry must never be persisted at its LSN"); + + TEST_ASSERT_TRUE_MESSAGE(g_hcs51_hook_called, + "a WAL catch-up rejection must send a WalAckReply NACK " + "(ha_send_wal_ack_reply_ex success=0), not silence it -- the " + "reply-construction boundary bug-1479 fixed"); + TEST_ASSERT_EQUAL_INT_MESSAGE(0, g_hcs51_hook_success, + "the reply for a rejected WAL catch-up entry must carry success=0"); + TEST_ASSERT_EQUAL_UINT64((uint64_t)FOLLOWER_TERM, g_hcs51_hook_term); + TEST_ASSERT_EQUAL_UINT64(2, g_hcs51_hook_lsn); + + hcs4_cluster_down(c); + printf("test_5_1_wal_catchup_reject_sends_nack_reply: PASS\n"); +} + /* ── Suite entry point ─────────────────────────────────────────────────────*/ void test_ha_core_safety(void) @@ -2871,4 +2978,9 @@ void test_ha_core_safety(void) * production senders) end-to-end and confirm the dormant stub's counter * never moves off zero, while proving each real sender actually fired. */ RUN_TEST(test_4_2_dormant_sender_stays_uncalled_in_production); + /* Task 5.1 (raft-adapter-conformance): regression-lock the bug-1479 + * reply-construction boundary — a WAL catch-up rejection must actually + * invoke ha_send_wal_ack_reply_ex(success=0) via ha_wal_ack_send_hook, + * not silently drop the reject. */ + RUN_TEST(test_5_1_wal_catchup_reject_sends_nack_reply); }