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.
This commit is contained in:
parent
87ce7be917
commit
cb668adc7a
3 changed files with 116 additions and 1 deletions
|
|
@ -64,7 +64,7 @@
|
||||||
- _Boundary: test_ha_core_safety.c_
|
- _Boundary: test_ha_core_safety.c_
|
||||||
|
|
||||||
- [ ] 5. Core: regression-lock tests for already-shipped reply and append behavior
|
- [ ] 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
|
- 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
|
- 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_
|
- _Requirements: 2.1_
|
||||||
|
|
|
||||||
|
|
@ -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.
|
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.
|
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.
|
**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.
|
||||||
|
|
|
||||||
|
|
@ -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");
|
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 ─────────────────────────────────────────────────────*/
|
/* ── Suite entry point ─────────────────────────────────────────────────────*/
|
||||||
|
|
||||||
void test_ha_core_safety(void)
|
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
|
* production senders) end-to-end and confirm the dormant stub's counter
|
||||||
* never moves off zero, while proving each real sender actually fired. */
|
* never moves off zero, while proving each real sender actually fired. */
|
||||||
RUN_TEST(test_4_2_dormant_sender_stays_uncalled_in_production);
|
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);
|
||||||
}
|
}
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue