From 4aeae6e282dfdc88ed57579dd942219ed82e7af5 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Sat, 25 Jul 2026 06:33:11 +0000 Subject: [PATCH] test(config-upgrade-snapshot): prove WAL-trim safety + add fault-injection seam for marker trigger Confirmed task 5's existing 6 tests already satisfy "exactly once", "safe under forced retry", and "does not re-home entities"; confirmed test_snapshot_recv_old_format_version_rejected_fail_loud (task 1) already covers format-mismatch rejection by proxy through load_snapshot's two shared callers (network-install, boot replay). Adds the two genuine gaps: (1) test_marker_triggered_checkpoint_trim_preserves_standby_mid_catchup_segments proves trim_wal_segments/repl_trim_lsn's standby-cursor retention guard (bug-1147) still holds when reached via the new marker-triggered checkpoint path, not just the old periodic/direct-call path -- guard itself untouched. (2) persist_marker_checkpoint_fault_hook (#ifdef XCE_TESTING, NULL default, true no-op in production) gates whether persist_checkpoint_marker_triggered runs, for task 8.7's later crash-before-checkpoint-durable injection; minimally proven here. Task 8.1 of 19. Requirements: 1.1, 1.5, 1.8, 5.2, 5.3, 5.6, 5.7, 6.3, 7.3, 7.4 --- .kiro/specs/config-upgrade-snapshot/tasks.md | 2 +- src/controller/persist.c | 42 ++- .../testsuite/test_upgrade_snapshot_apply.c | 249 ++++++++++++++++++ 3 files changed, 291 insertions(+), 2 deletions(-) diff --git a/.kiro/specs/config-upgrade-snapshot/tasks.md b/.kiro/specs/config-upgrade-snapshot/tasks.md index dd28036..c031be2 100644 --- a/.kiro/specs/config-upgrade-snapshot/tasks.md +++ b/.kiro/specs/config-upgrade-snapshot/tasks.md @@ -78,7 +78,7 @@ - _Depends: 5_ - [ ] 8. Validation: regression coverage for every new mechanism -- [ ] 8.1 (P) Unit-test the marker-apply trigger and its interaction with WAL retention +- [x] 8.1 (P) Unit-test the marker-apply trigger and its interaction with WAL retention - Assert the configuration-marker case fires a checkpoint exactly once per apply and is safe under a forced retry - Assert the trigger does not itself re-home entities - Add a deterministic fault-injection seam at the marker-apply-to-checkpoint-durable boundary, mirroring the existing fault-hook pattern already used elsewhere in this codebase, for later chaotic-scenario use diff --git a/src/controller/persist.c b/src/controller/persist.c index 5238aaa..ea0e822 100644 --- a/src/controller/persist.c +++ b/src/controller/persist.c @@ -3369,6 +3369,33 @@ int apply_wal_entry(persist_t* p, uint32_t type, uint64_t lsn, * SYNCHRONOUSLY on this exact call stack, from this exact function. */ static _Thread_local bool t_apply_seam_notifying = false; +#ifdef XCE_TESTING +/* config-upgrade-snapshot task 8.1 (Req 6.3): deterministic fault-injection + * seam at the marker-apply-to-checkpoint-durable boundary, for later use by + * a crash/restart regression (task 8.7): "force a controller crash/restart + * between a configuration-change marker's commit and that member's own + * snapshot write becoming durable, and assert materialization resumes + * correctly from the marker on restart." When non-NULL AND it returns + * nonzero for the marker's own (p, lsn), the WAL_CONFIG_MARKER case below + * skips its call into persist_checkpoint_marker_triggered() entirely -- + * simulating "the process crashes here, before the checkpoint write becomes + * durable" (a real crash simply stops the process; the checkpoint call + * never runs either way). The skip is deliberately routed through the SAME + * rc!=0 handling an actual checkpoint failure already takes below (not a + * new branch): last_applied_lsn is not advanced past this marker, so the + * next kick/retry -- or, after a genuine restart, boot replay's independent + * call into this same case (already proven idempotent by + * test_marker_apply_retry_is_idempotent) -- resumes materialization from + * the marker fresh, exactly matching Requirement 5.2's crash-safety + * contract. NULL in production -- no behaviour change. Mirrors + * ha_stream_body_read_fault_hook's shape (config_repl.c) and this same + * spec's own xce_upgrade_marker_committed_hook placement convention + * (defined here at file scope, declared extern in whichever test file + * needs it -- never in a header). */ +int (*persist_marker_checkpoint_fault_hook)(persist_t *p, + uint64_t marker_lsn) = NULL; +#endif + int apply_committed_entry(persist_t* p, uint32_t wal_type, uint64_t lsn, const void* payload, uint32_t len) { @@ -3780,7 +3807,20 @@ int apply_committed_entry(persist_t* p, uint32_t wal_type, uint64_t lsn, * checkpoint. `lsn` (this function's own parameter) is * already the marker's exact position, so it is passed * through explicitly instead of re-derived. */ - int rc = persist_checkpoint_marker_triggered(p, lsn); + int rc; +#ifdef XCE_TESTING + if (persist_marker_checkpoint_fault_hook != NULL && + persist_marker_checkpoint_fault_hook(p, lsn)) { + /* Simulated crash (task 8.1, Req 6.3): skip the checkpoint call + * entirely and fall into the SAME genuine-failure handling below + * a real checkpoint I/O error already takes -- see the fault + * hook's own doc comment above for why that is the correct, + * not merely convenient, simulation of "the process never got + * to checkpoint". */ + rc = -ECANCELED; + } else +#endif + rc = persist_checkpoint_marker_triggered(p, lsn); if (rc == -EBUSY) { /* Expected, self-resolving (Req 4.4/5.3): a concurrent * WAL-threshold-triggered or earlier marker-triggered diff --git a/src/controller/testsuite/test_upgrade_snapshot_apply.c b/src/controller/testsuite/test_upgrade_snapshot_apply.c index 0c94d35..636ef6c 100644 --- a/src/controller/testsuite/test_upgrade_snapshot_apply.c +++ b/src/controller/testsuite/test_upgrade_snapshot_apply.c @@ -66,6 +66,7 @@ #include #include #include +#include #include @@ -74,6 +75,11 @@ #include "hook.h" #include "jobj.h" #include "persist.h" +#include "repl.h" /* repl_add_standby / repl_on_ack -- Gap-1 WAL-trim- + * under-marker-trigger test (task 8.1, Req 7.4), + * mirrors test_e2e_checkpoint.c's own include */ +#include "typemap.h" /* wal_catchup_entry_t -- repl_broadcast_send_hook's + * signature */ #include "wal_types.h" #include "xce/cluster_def.h" #include "xce/types.h" @@ -185,6 +191,37 @@ static int tusa_count_snapshots(persist_t* p) return n; } +/* Drop hook for repl_broadcast: a fixture that registers a standby with a + * dummy (int) endpoint via repl_add_standby must install this so + * persist_wal_async -> repl_broadcast does not crash trying to send to the + * fake endpoint. Pattern from test_e2e_checkpoint.c (itself from + * test_commit_engine.c / test_p2_integration.c) -- reused here, not + * duplicated logic, for the Gap-1 WAL-trim-under-marker-trigger test + * (task 8.1, Req 7.4). */ +static int tusa_drop_broadcast(void* ep, wal_catchup_entry_t** entries, + int count) +{ + (void)ep; + (void)entries; + (void)count; + return 0; +} + +extern int (*repl_broadcast_send_hook)(void* ep, + wal_catchup_entry_t** entries, + int count); + +/* Checks for a WAL segment file "seg-.wal" at seg_start_lsn inside + * wal_dir -- mirrors test_e2e_checkpoint.c's own seg_file_exists helper + * exactly (same naming convention the production trim/rotate code uses). */ +static bool tusa_seg_file_exists(const char* wal_dir, uint64_t seg_start_lsn) +{ + char path[4096]; + snprintf(path, sizeof(path), "%s/seg-%016" PRIx64 ".wal", + wal_dir, seg_start_lsn); + return access(path, F_OK) == 0; +} + static int tusa_count_entries(var mgr) { if (!mgr) return 0; @@ -605,6 +642,216 @@ static void test_marker_apply_via_replay_checkpoints_at_marker_own_lsn(void) printf("test_marker_apply_via_replay_checkpoints_at_marker_own_lsn: PASS\n"); } +/* ── 7: WAL-trim safety under the marker-triggered path (Req 7.4) ─────── + * + * Six existing tests already prove trim_wal_segments()'s standby-cursor + * retention guard (repl_trim_lsn(), bug-1147) holds under the OLD trigger + * -- a direct/periodic persist_checkpoint() call: + * test_e2e_checkpoint.c's test_trim_guard_preserves_standby_segments, plus + * repl_trim_lsn()'s own unit coverage in test_repl_unit.c / + * test_p2_integration.c. None of them drive that SAME guard through this + * spec's NEW trigger -- a WAL_CONFIG_MARKER committed and applied via the + * real apply_committed_entry()/persist_apply_pump_run() path, which + * reaches the identical, unmodified trim_wal_segments()/repl_trim_lsn() + * code via persist_checkpoint_marker_triggered() instead of a bare + * persist_checkpoint() call. That is a real, previously-flagged coverage + * gap (task 5's own implementation notes), not something covered by proxy + * -- this test closes it. + * + * Uses this file's own real-cluster tusa_t fixture, not + * test_e2e_checkpoint.c's cluster-less make_ckpt_e2e() -- apply_committed_ + * entry() short-circuits to a no-op whenever p->cluster is NULL, so a + * cluster-less fixture would never actually reach persist_checkpoint_ + * marker_triggered() at all and could not exercise this path. WAL_NOOP is + * used for every filler entry (rather than test_e2e_checkpoint.c's + * WAL_JOB_UPDATE) because that file's fixture has no cluster to apply a + * job update against; WAL_NOOP is safe against a real cluster (default: + * no-op case), exactly as tusa_append_noop_then_marker() already relies on + * elsewhere in this file. + * + * Layout mirrors test_trim_guard_preserves_standby_segments' own LSN + * numbering exactly, for direct comparability: + * noop @ lsn1=1 → force segment (seg-1 closes, seg-2 opens @ lsn2=2) + * noop @ lsn2=2 → standby acks lsn2 (mid-catch-up: this is the LAST + * position it has acked -- in particular it has NOT + * acked the marker below) + * → force segment (seg-2 closes, seg-3 opens @ lsn3=3, + * stays active -- no further force call) + * noop @ lsn3=3 → keeps last_applied_lsn genuinely behind the marker's + * own lsn at dispatch time (same discipline this file's + * other tests already document as Hazard 2) + * WAL_CONFIG_MARKER @ marker_lsn=4 → committed → applied via + * persist_apply_pump_run(), which dispatches into apply_committed_ + * entry()'s WAL_CONFIG_MARKER case → persist_checkpoint_marker_ + * triggered() → trim_wal_segments(). + * + * floor = repl_trim_lsn(active_seg_lsn=3, standby acked=2) = min(3,2) = 2: + * seg-1 (start=1) < floor → MUST be trimmed + * seg-2 (start=2) == floor → MUST be preserved (the standby still needs + * it for catch-up -- exactly bug-1147's guard) + * seg-3 (start=3, active) → MUST always be preserved + */ + +static void +test_marker_triggered_checkpoint_trim_preserves_standby_mid_catchup_segments(void) +{ + tusa_t t; + tusa_setup(&t); + + repl_broadcast_send_hook = tusa_drop_broadcast; + + int dummy_ep = 0; + int standby_idx = repl_add_standby(&t.p.repl, (void*)&dummy_ep, + "standby1", 7001); + + int rc_noop1 = persist_wal_async(&t.p, WAL_NOOP, NULL, 0); + uint64_t lsn1 = t.p.current_lsn - 1; + int rc_force1 = persist_test_force_new_segment(&t.p); + + int rc_noop2 = persist_wal_async(&t.p, WAL_NOOP, NULL, 0); + uint64_t lsn2 = t.p.current_lsn - 1; + repl_on_ack(&t.p.repl, (void*)&dummy_ep, lsn2); + int rc_force2 = persist_test_force_new_segment(&t.p); + uint64_t active_seg_lsn = t.p.current_lsn; /* seg-3 opens here */ + + int rc_noop3 = persist_wal_async(&t.p, WAL_NOOP, NULL, 0); + + uint8_t sha[32]; + memset(sha, 0xA7, sizeof(sha)); + uint8_t marker_payload[WAL_CONFIG_MARKER_PAYLOAD_LEN]; + size_t out_len = 0; + int rc_encode = config_marker_encode(0x1122334455667788ULL ^ 0xA7, sha, + marker_payload, + WAL_CONFIG_MARKER_PAYLOAD_LEN, + &out_len); + int rc_marker = persist_wal_async(&t.p, WAL_CONFIG_MARKER, marker_payload, + WAL_CONFIG_MARKER_PAYLOAD_LEN); + uint64_t marker_lsn = t.p.current_lsn - 1; + uint64_t committed_after = + persist_committed_lsn_advance_atomic(&t.p, marker_lsn); + + int applied = persist_apply_pump_run(&t.p); + + bool seg1_exists = tusa_seg_file_exists(t.p.wal_path, lsn1); + bool seg2_exists = tusa_seg_file_exists(t.p.wal_path, lsn2); + bool seg3_exists = tusa_seg_file_exists(t.p.wal_path, active_seg_lsn); + bool snap_at_marker = tusa_snapshot_exists(&t.p, marker_lsn); + + repl_broadcast_send_hook = NULL; + tusa_teardown(&t); + + TEST_ASSERT_TRUE(standby_idx >= 0); + TEST_ASSERT_EQUAL_INT(0, rc_noop1); + TEST_ASSERT_EQUAL_INT(0, rc_force1); + TEST_ASSERT_EQUAL_INT(0, rc_noop2); + TEST_ASSERT_EQUAL_INT(0, rc_force2); + TEST_ASSERT_EQUAL_INT(0, rc_noop3); + TEST_ASSERT_EQUAL_INT(0, rc_encode); + TEST_ASSERT_EQUAL_INT(0, rc_marker); + TEST_ASSERT_EQUAL_UINT64(1, lsn1); + TEST_ASSERT_EQUAL_UINT64(2, lsn2); + TEST_ASSERT_EQUAL_UINT64(3, active_seg_lsn); + TEST_ASSERT_EQUAL_UINT64(4, marker_lsn); + TEST_ASSERT_EQUAL_UINT64(marker_lsn, committed_after); + TEST_ASSERT_EQUAL_INT_MESSAGE(4, applied, + "all four entries (3 noops + the marker) must apply successfully"); + TEST_ASSERT_TRUE_MESSAGE(snap_at_marker, + "the marker-triggered checkpoint must still fire and produce a " + "snapshot even with a standby mid-catch-up"); + + TEST_ASSERT_FALSE_MESSAGE(seg1_exists, + "seg-1: below the standby's mid-catch-up acked_lsn=2, MUST be " + "trimmed by the marker-triggered checkpoint"); + TEST_ASSERT_TRUE_MESSAGE(seg2_exists, + "seg-2: at the standby's mid-catch-up acked_lsn=2, MUST be " + "preserved (bug-1147 guard) even when trim runs via the NEW " + "marker-triggered path"); + TEST_ASSERT_TRUE_MESSAGE(seg3_exists, + "seg-3: the active segment, MUST always be preserved"); + + printf("test_marker_triggered_checkpoint_trim_preserves_standby_" + "mid_catchup_segments: PASS\n"); +} + +/* ── 8: fault-injection seam at the marker-apply→checkpoint-durable + * boundary (Req 6.3) ─────────────────────────────────────────────── + * + * Proves the new persist_marker_checkpoint_fault_hook seam (persist.c, + * added by this task) fires exactly once for the marker's own (p, lsn) and + * correctly gates whether persist_checkpoint_marker_triggered() is called + * at all. This is deliberately ONLY the seam itself, not the full + * crash/restart regression -- that is task 8.7's job, which depends on + * this seam landing first. + */ + +typedef struct { + int calls; + persist_t* last_p; + uint64_t last_lsn; +} tusa_fault_obs_t; + +static tusa_fault_obs_t g_tusa_fault_obs; + +static int tusa_marker_fault_hook(persist_t* p, uint64_t marker_lsn) +{ + g_tusa_fault_obs.calls++; + g_tusa_fault_obs.last_p = p; + g_tusa_fault_obs.last_lsn = marker_lsn; + return 1; /* simulate "the process never got to checkpoint" */ +} + +static void test_marker_checkpoint_fault_hook_gates_checkpoint_call(void) +{ + tusa_t t; + extern int (*persist_marker_checkpoint_fault_hook)(persist_t* p, + uint64_t marker_lsn); + + tusa_setup(&t); + + memset(&g_tusa_fault_obs, 0, sizeof(g_tusa_fault_obs)); + persist_marker_checkpoint_fault_hook = tusa_marker_fault_hook; + + tusa_append_result_t ar = tusa_append_noop_then_marker(&t.p, 0x17); + int apply_rc = persist_apply_pump_run(&t.p); + + bool snap_at_marker = tusa_snapshot_exists(&t.p, ar.marker_lsn); + uint64_t last_applied = atomic_load_explicit(&t.p.last_applied_lsn, + memory_order_acquire); + int fault_calls = g_tusa_fault_obs.calls; + uint64_t fault_lsn = g_tusa_fault_obs.last_lsn; + bool fault_p_matches = (g_tusa_fault_obs.last_p == &t.p); + + persist_marker_checkpoint_fault_hook = NULL; + tusa_teardown(&t); + + TEST_ASSERT_EQUAL_INT(0, ar.rc_encode); + TEST_ASSERT_EQUAL_INT(0, ar.rc_noop); + TEST_ASSERT_EQUAL_INT(0, ar.rc_marker); + + TEST_ASSERT_EQUAL_INT_MESSAGE(1, fault_calls, + "the fault hook must fire exactly once, for the marker's own " + "checkpoint attempt"); + TEST_ASSERT_EQUAL_UINT64(ar.marker_lsn, fault_lsn); + TEST_ASSERT_TRUE_MESSAGE(fault_p_matches, + "the fault hook must observe the SAME persist_t the marker is " + "being applied to"); + + TEST_ASSERT_EQUAL_INT_MESSAGE(-ECANCELED, apply_rc, + "a nonzero fault-hook return must propagate as the marker's apply " + "failure, taking the SAME halt-and-retry path a genuine checkpoint " + "I/O error already takes"); + TEST_ASSERT_FALSE_MESSAGE(snap_at_marker, + "a nonzero fault-hook return must skip the checkpoint call " + "entirely -- no snapshot file at the marker's lsn"); + TEST_ASSERT_EQUAL_UINT64_MESSAGE(ar.noop_lsn, last_applied, + "last_applied_lsn must NOT advance past the marker when the " + "checkpoint is skipped by the fault hook -- materialization must " + "resume from the marker on the next attempt"); + + printf("test_marker_checkpoint_fault_hook_gates_checkpoint_call: " + "PASS\n"); +} + /* ── Dispatch ────────────────────────────────────────────────────────── */ void test_upgrade_snapshot_apply(void) @@ -618,4 +865,6 @@ void test_upgrade_snapshot_apply(void) RUN_TEST(test_marker_apply_busy_checkpoint_handled_gracefully); RUN_TEST(test_marker_apply_does_not_reinterpret_entities); RUN_TEST(test_marker_apply_via_replay_checkpoints_at_marker_own_lsn); + RUN_TEST(test_marker_triggered_checkpoint_trim_preserves_standby_mid_catchup_segments); + RUN_TEST(test_marker_checkpoint_fault_hook_gates_checkpoint_call); }