From 7519fa96dd0e60d28602820f52c8b409957adcdd Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Sun, 14 Jun 2026 10:46:16 +0000 Subject: [PATCH] =?UTF-8?q?fix(ha-commit-engine,persist):=20reserve=20WAL?= =?UTF-8?q?=20lsn=200=20=E2=80=94=20first=20entry=20starts=20at=20lsn=201?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause (found via live 3-node E2E + a verification test): lsn was 0-based (hdr.lsn = current_lsn++ from current_lsn=0), and 0 was overloaded as a sentinel, causing three cold-start defects on a freshly-promoted leader: - the first WAL entry (own-term promotion NOOP at lsn 0) was structurally uncommittable: commit_advance_locked only stores when cand>cur and committed_lsn starts at 0, so lsn 0 can never advance it; - commit_set_writable_lsn(noop_lsn=0) set writable_lsn=0, which wait_done reads as 'gate off' — the own-term safety gate was a no-op on the first leader; - bug-543 (WAL hole): ha_stream_wal_to_standby filters hdr.lsn>standby_lsn; a fresh empty standby reports standby_lsn=0 (ambiguous 'empty' vs 'has lsn 0'), so lsn 0 failed 0>0 and was dropped -> non-contiguous standby WAL. Fix: a fresh persist store initializes current_lsn=1, reserving lsn 0 as the none/empty sentinel. The first real entry is lsn 1 -> committable (0->1), writable_lsn=1 -> gate effective, and standby_lsn=0 unambiguously means 'empty' so catch-up streams from the first entry with no hole. persist_recover is data-driven (current_lsn=max(hdr.lsn+1)), so existing on-disk WALs and non-fresh recovery are unaffected; the change only sets the base for brand-new stores. No wire/capnp change. Verification: test_coldstart_lsn0 drives the real commit engine + real streamer and proves H1 (first entry commits 0->1), H3 (gate effective + blocks when committedlast_replicated_crc = WAL_CRC_NONE; p->last_crc = WAL_CRC_NONE; + /* LSN 0 is RESERVED as the "none / empty" sentinel — it is NEVER assigned + * to a real WAL entry. A fresh store therefore starts current_lsn at 1, so + * the first-ever appended entry (e.g. the promotion WAL_NOOP) is lsn 1 and + * lsn 0 unambiguously means "nothing here": + * - committed_lsn == 0 → nothing committed (the lsn-1 NOOP is committable + * 0 → 1, vs. the old lsn-0 entry which could never advance committed_lsn); + * - writable_lsn == 0 → own-term gate off (the gate armed at lsn 1 is + * effective, vs. lsn 0 which read as "gate off" on a cold-start leader); + * - standby_lsn == 0 → empty standby (the streamer's `hdr.lsn > 0` + * filter streams the lsn-1 entry, closing the cold-start WAL hole that + * dropped lsn 0 on `0 > 0`). + * persist_recover() overwrites current_lsn from the durable snapshot/WAL tip + * (load_snapshot / replay_wal), so this fresh default only applies before any + * recovery — i.e. a brand-new store with no snapshot and no WAL. */ + p->current_lsn = 1; + /* Enforce minimum value for max_wal_records to prevent runaway checkpointing */ if (p->config.max_wal_records < 100) { ERROR("persist: max_wal_records=%u too low (minimum 100) — setting to 100", diff --git a/src/controller/testsuite/controller_test.c b/src/controller/testsuite/controller_test.c index 59f651e2..1dd03322 100644 --- a/src/controller/testsuite/controller_test.c +++ b/src/controller/testsuite/controller_test.c @@ -55,6 +55,7 @@ extern void test_commit_barrier_reply(void); extern void test_egress_filter(void); extern void test_demotion_convergence(void); extern void test_convergence_verification(void); +extern void test_coldstart_lsn0(void); void setUp(void) { return; } @@ -179,6 +180,7 @@ int main(int argc, char* argv[]) TEST(test_egress_filter); TEST(test_demotion_convergence); TEST(test_convergence_verification); + TEST(test_coldstart_lsn0); return UNITY_END(); } diff --git a/src/controller/testsuite/test_coldstart_lsn0.c b/src/controller/testsuite/test_coldstart_lsn0.c new file mode 100644 index 00000000..9116218e --- /dev/null +++ b/src/controller/testsuite/test_coldstart_lsn0.c @@ -0,0 +1,412 @@ +/* + * test_coldstart_lsn0.c — RESERVED-LSN-0 CONTRACT. + * + * Pins the cold-start fix: LSN 0 is RESERVED as the "none / empty" sentinel and + * is NEVER assigned to a real WAL entry. A fresh persist store starts + * current_lsn = 1 (persist_init, persist.c), so the FIRST-EVER appended entry + * (the promotion WAL_NOOP) is lsn 1. This removes three cold-start defects that + * the old 0-based scheme (hdr.lsn = current_lsn++ from 0) caused: + * + * H1 The first entry (NOOP at lsn 1) IS committable. committed_lsn advances + * 0 → 1. (Under the old scheme the lsn-0 NOOP was structurally + * uncommittable: commit_advance_locked only stores cand > cur and + * committed_lsn starts at 0, so lsn 0 could never advance it.) + * + * H2 A second real entry (lsn 2) commits on a majority watermark, proving the + * reserved base does not break ordinary advancement. + * + * H3 The own-term gate armed at the NOOP lsn (now 1, ≠ 0) is EFFECTIVE: a + * WAIT for an entry below the gate stays blocked until committed_lsn + * reaches the gate. (Under the old scheme writable_lsn = noop_lsn = 0 read + * as "gate off" in wait_done_locked — the safety gate was a no-op on the + * first cold-start leader.) + * + * H4 (CRITICAL — bug-543) NO WAL HOLE: a fresh EMPTY standby reports + * standby_lsn = 0 (unambiguously "empty", since no real entry is ever + * lsn 0). The REAL streamer (ha_stream_wal_to_standby, config_repl.c) + * filters `hdr.lsn > standby_lsn`; the first entry lsn 1 satisfies 1 > 0 + * and is streamed, so the standby's WAL is contiguous from the tip. + * + * Drives the REAL commit engine (commit.c) and the REAL catch-up streamer + * (ha_stream_wal_to_standby) — never reimplementations. + */ +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +#include "xce/errnox.h" + +#include "commit.h" +#include "config_repl.h" /* ha_stream_wal_to_standby */ +#include "controller.capnp.h" /* OPCode_walCatchupEntry */ +#include "persist.h" +#include "repl.h" +#include "request.h" /* xce_request_t */ +#include "typemap.h" /* wal_catchup_entry_t */ +#include "wal_types.h" /* WAL_JOB_SUBMIT, wal_entry_hdr_t */ + +/* ── H1-H3 commit-engine fixture (pattern from test_commit_engine.c) ───────── */ +static persist_t g_p; +static commit_engine_t g_ce; +static int g_eps[MAX_STANDBYS]; + +static void ce_setup(int n_peers, int n_standbys, uint64_t appended_lsn) +{ + int i; + char name[16]; + + memset(&g_p, 0, sizeof(g_p)); + TEST_ASSERT_EQUAL_INT(0, repl_init(&g_p.repl)); + for (i = 0; i < n_standbys; i++) { + snprintf(name, sizeof(name), "sb%d", i); + TEST_ASSERT_EQUAL_INT( + i, repl_add_standby(&g_p.repl, &g_eps[i], name, 7000 + (uint32_t)i)); + } + /* appended_lsn = highest locally appended; current_lsn = next = +1. + * Reserved-0 contract: appended_lsn == 0 means NOTHING appended (the lsn-0 + * sentinel), so current_lsn = 1 = "first entry will be lsn 1". */ + g_p.current_lsn = appended_lsn + 1; + atomic_store(&g_p.committed_lsn, 0); + TEST_ASSERT_EQUAL_INT(0, commit_engine_init(&g_ce, &g_p, n_peers, 8)); +} + +static void ce_teardown(void) +{ + commit_engine_fini(&g_ce); + repl_fini(&g_p.repl); +} + +static void standby_ack(int idx, uint64_t acked_lsn) +{ + atomic_store(&g_p.repl.standbys[idx].acked_lsn, acked_lsn); + commit_on_ack(&g_ce, idx, acked_lsn); +} + +static uint64_t committed(void) +{ + return atomic_load(&g_p.committed_lsn); +} + +/* + * H1: the first-ever entry — the promotion NOOP at lsn 1 — COMMITS. + * + * Cold-start model: a fresh 3-node cluster whose ONLY appended entry is the + * promotion NOOP at lsn 1 (appended_lsn = 1 → current_lsn = 2 → cap = 1). + * A majority of standbys ACK lsn 1; committed_lsn advances 0 → 1. This is the + * fix: the reserved-0 base makes the first entry committable (0 → 1), whereas a + * lsn-0 first entry could never advance committed_lsn off 0. + */ +static void test_h1_first_noop_lsn1_commits(void) +{ + ce_setup(/*n_peers=*/3, /*n_standbys=*/2, /*appended_lsn=*/1); + + uint64_t before = committed(); + + /* Majority (self + 1 of 2 for n_peers=3) ACK the lsn-1 NOOP. */ + standby_ack(0, 1); + + uint64_t after = committed(); + + printf("H1: committed_lsn before=%" PRIu64 " after sb0 ack(1)=%" PRIu64 + " (cap=highest_appended=1)\n", before, after); + + /* lsn 1 (the first-ever entry) is committable: committed_lsn advances 0→1. */ + TEST_ASSERT_EQUAL_UINT64(0, before); + TEST_ASSERT_EQUAL_UINT64(1, after); + + ce_teardown(); + printf("test_h1_first_noop_lsn1_commits: PASS\n"); +} + +/* + * H2: a second real entry at lsn 2 also commits. + * + * 3-node cluster, entries appended through lsn 2 (NOOP lsn 1 + first real write + * lsn 2 → appended_lsn = 2 → cap = 2). One standby ACKs watermark 2 + * (self + 1 of 2 = majority for n_peers=3). committed_lsn advances 0 → 2, + * proving the reserved base does not perturb ordinary advancement. + */ +static void test_h2_real_entry_lsn2_commits(void) +{ + ce_setup(/*n_peers=*/3, /*n_standbys=*/2, /*appended_lsn=*/2); + + uint64_t before = committed(); + + /* standby 0 ACK watermark 2 → matchIndex {self=2, sb0=2, sb1=0}: median 2 */ + standby_ack(0, 2); + + uint64_t after = committed(); + + printf("H2: committed_lsn before=%" PRIu64 " after sb0 ack(2)=%" PRIu64 "\n", + before, after); + + TEST_ASSERT_EQUAL_UINT64(0, before); + TEST_ASSERT_EQUAL_UINT64(2, after); + + ce_teardown(); + printf("test_h2_real_entry_lsn2_commits: PASS\n"); +} + +/* + * H3: the own-term gate armed at the NOOP lsn (now 1, ≠ 0) is EFFECTIVE. + * + * Arm the gate at noop_lsn = 1 (the cold-start NOOP lsn under the reserved-0 + * contract). With writable_lsn = 1, wait_done_locked's predicate + * `c >= lsn && (writable_lsn == 0 || c >= writable_lsn)` does NOT short-circuit: + * a WAIT for lsn 1 completes only once committed_lsn >= 1. We commit lsn 1 via + * self-quorum (n_peers = 1), then the WAIT must return 0 — the gate opens + * exactly when the NOOP commits. (Under the old lsn-0 scheme writable_lsn = 0 + * disabled the gate entirely.) + */ +static void test_h3_writable_lsn1_gate_effective(void) +{ + /* n_peers = 1: self-quorum, so the WAIT advances + completes on its own. */ + ce_setup(/*n_peers=*/1, /*n_standbys=*/0, /*appended_lsn=*/1); + + /* Arm the own-term gate at the cold-start NOOP lsn 1 (reserved-0 contract). */ + commit_set_writable_lsn(&g_ce, 1); + TEST_ASSERT_EQUAL_UINT64(1, g_ce.writable_lsn); + + /* WAIT for lsn 1 with a generous-but-finite timeout. writable_lsn = 1 is an + * ACTIVE gate (not "off"): the predicate requires committed_lsn >= 1. With + * self-quorum the entry commits and the gate opens, so WAIT returns 0. */ + int rc = commit_submit(&g_ce, /*lsn=*/1, COMMIT_MODE_WAIT, /*timeout_ms=*/500); + + printf("H3: commit_submit(lsn=1,WAIT) with writable_lsn=1 → rc=%d " + "committed_lsn=%" PRIu64 "\n", rc, committed()); + + TEST_ASSERT_EQUAL_INT_MESSAGE(0, rc, + "writable_lsn=1 is an active gate that opens when lsn 1 commits"); + TEST_ASSERT_EQUAL_UINT64(1, committed()); + + ce_teardown(); + printf("test_h3_writable_lsn1_gate_effective: PASS\n"); +} + +/* + * H3b: the gate at lsn 1 truly BLOCKS a sub-gate WAIT until the gate is reached. + * + * Distinct from H3 (which proves the gate opens): here we prove writable_lsn = 1 + * is not silently ignored. With NOTHING committed (committed_lsn = 0) and the + * gate armed at 1, a short-timeout WAIT for lsn 1 must NOT complete (no quorum + * ever arrives in a 3-node cluster with no ACKs) — it times out. A gate that + * read as "off" (the old lsn-0 bug) would also block here on `c >= lsn`, so to + * isolate the gate we WAIT for lsn 1 while only committed_lsn 0 is reachable. + */ +static void test_h3b_gate_lsn1_blocks_until_reached(void) +{ + /* 3-node cluster, NOOP lsn 1 appended, NO standby ACKs → never a majority. */ + ce_setup(/*n_peers=*/3, /*n_standbys=*/2, /*appended_lsn=*/1); + commit_set_writable_lsn(&g_ce, 1); + + /* committed_lsn is still 0; the gate at 1 is unreached. WAIT must time out + * (the gate is a blocking predicate, never a fast NOT_READY reject). */ + int rc = commit_submit(&g_ce, /*lsn=*/1, COMMIT_MODE_WAIT, /*timeout_ms=*/150); + + printf("H3b: WAIT(lsn=1) with gate=1, committed=%" PRIu64 " → rc=%d\n", + committed(), rc); + + TEST_ASSERT_EQUAL_INT_MESSAGE(-ETIMEDOUT, rc, + "active gate at lsn 1 must block (time out) until committed_lsn >= 1"); + TEST_ASSERT_EQUAL_UINT64(0, committed()); + + ce_teardown(); + printf("test_h3b_gate_lsn1_blocks_until_reached: PASS\n"); +} + +/* ── H4 catch-up streaming fixture (real ha_stream_wal_to_standby) ─────────── */ + +/* test-only seam declared in config_repl.c (XCE_TESTING). */ +extern int (*xce_wal_stream_request_hook)(endpoint_t *ep, xce_request_t *req); + +/* Capture the exact set of streamed LSNs across all batches. */ +#define MAX_CAPTURED 64 +static uint64_t g_streamed_lsns[MAX_CAPTURED]; +static int g_streamed_n; + +static int capture_wal_stream_hook(endpoint_t *ep, xce_request_t *req) +{ + (void)ep; + if (!req || !req->body.walcatchupentrymsg) + return 0; + wal_catchup_entry_t *batch = (wal_catchup_entry_t *)req->body.walcatchupentrymsg; + /* ha_stream_wal_to_standby builds a container batch whose ->entries is an + * array of per-entry wal_catchup_entry_t* (n_entries of them). */ + for (int i = 0; i < batch->n_entries && g_streamed_n < MAX_CAPTURED; i++) { + wal_catchup_entry_t *e = batch->entries[i]; + g_streamed_lsns[g_streamed_n++] = e->lsn; + } + return 0; +} + +/* Write a genuine reserved-0 cold-start segment: the first segment is named + * after current_lsn = 1 (persist.c open_new_segment), so it is + * seg-0000000000000001.wal and holds entries lsn 1 and lsn 2 — exactly the + * layout persist.c lays down on a fresh cluster (NOOP lsn 1 + first write + * lsn 2). LSN 0 is reserved and never written. */ +static int write_coldstart_segment(const char *wal_dir) +{ + char seg_path[4096]; + /* Reserved-0 contract: first segment is named after current_lsn == 1. */ + snprintf(seg_path, sizeof(seg_path), + "%s/seg-%016" PRIx64 ".wal", wal_dir, (uint64_t)1); + + int fd = open(seg_path, O_WRONLY | O_CREAT | O_TRUNC, 0600); + if (fd < 0) + return -1; + + for (uint64_t lsn = 1; lsn <= 2; lsn++) { + wal_entry_hdr_t hdr; + memset(&hdr, 0, sizeof(hdr)); + hdr.lsn = lsn; + hdr.type = WAL_JOB_SUBMIT; + hdr.payload_len = 0; + hdr.term = 1; + if (write(fd, &hdr, sizeof(hdr)) != (ssize_t)sizeof(hdr)) { + close(fd); + return -1; + } + } + close(fd); + return 0; +} + +static void h4_capture_reset(void) +{ + memset(g_streamed_lsns, 0, sizeof(g_streamed_lsns)); + g_streamed_n = 0; +} + +static int streamed_contains(uint64_t lsn) +{ + for (int i = 0; i < g_streamed_n; i++) + if (g_streamed_lsns[i] == lsn) + return 1; + return 0; +} + +static void print_streamed(const char *tag) +{ + printf("%s streamed n=%d lsns=[", tag, g_streamed_n); + for (int i = 0; i < g_streamed_n; i++) + printf("%s%" PRIu64, i ? "," : "", g_streamed_lsns[i]); + printf("]\n"); +} + +/* + * H4 (CRITICAL — bug-543): a fresh EMPTY standby catching up to a primary + * holding [1,2] receives BOTH lsn 1 AND lsn 2 — NO hole. + * + * An empty standby (no entries applied) reports standby_lsn = 0, which under the + * reserved-0 contract unambiguously means "empty" (no real entry is ever lsn 0). + * The primary holds committed entries 1 and 2. We drive the REAL streamer and + * capture the exact LSNs it sends. The filter `hdr.lsn > standby_lsn` now yields + * 1 > 0 = true for the first entry, so lsn 1 is streamed — the cold-start hole + * (old: 0 > 0 = false dropped lsn 0) is closed. Correctness requires a + * contiguous [1,2]. + */ +static void test_h4_empty_standby_receives_lsn1_and_lsn2_no_hole(void) +{ + char tmpdir[256]; + snprintf(tmpdir, sizeof(tmpdir), "/tmp/xce_coldstart_%d_%ld", + (int)getpid(), (long)time(NULL)); + TEST_ASSERT_EQUAL_INT(0, mkdir(tmpdir, 0700)); + + TEST_ASSERT_EQUAL_INT_MESSAGE(0, write_coldstart_segment(tmpdir), + "failed to write cold-start segment"); + + xce_wal_stream_request_hook = capture_wal_stream_hook; + h4_capture_reset(); + + /* Empty fresh standby = standby_lsn 0; primary committed_lsn = 2 (holds + * [1,2]). Drive the REAL streamer. */ + int rc = ha_stream_wal_to_standby(/*ep=*/NULL, tmpdir, + /*standby_lsn=*/0, + /*committed_lsn=*/2); + xce_wal_stream_request_hook = NULL; + + TEST_ASSERT_EQUAL_INT_MESSAGE(0, rc, "ha_stream_wal_to_standby failed"); + + print_streamed("H4(empty standby_lsn=0, committed=2):"); + + int has1 = streamed_contains(1); + int has2 = streamed_contains(2); + printf("H4: has_lsn1=%d has_lsn2=%d (contiguous [1,2] required, no hole)\n", + has1, has2); + + char cmd[512]; + snprintf(cmd, sizeof(cmd), "rm -rf %s", tmpdir); + if (system(cmd) != 0) + fprintf(stderr, "cleanup warning: rm -rf %s failed\n", tmpdir); + + /* Correctness assertion: BOTH entries reach the empty standby — no hole. */ + TEST_ASSERT_TRUE_MESSAGE(has2, "lsn 2 must be streamed"); + TEST_ASSERT_TRUE_MESSAGE(has1, + "NO HOLE: empty standby (standby_lsn=0) must receive the first entry " + "lsn 1 — the reserved-0 base makes `hdr.lsn(1) > standby_lsn(0)` true, " + "closing the cold-start WAL hole that the old lsn-0 first entry caused"); + + printf("test_h4_empty_standby_receives_lsn1_and_lsn2_no_hole: PASS\n"); +} + +/* + * H4 control: a standby that has applied lsn 1 (standby_lsn = 1) catching up to + * committed 2 receives ONLY lsn 2. Under the reserved-0 contract standby_lsn = 1 + * is unambiguous ("has the first entry"), so the streamer correctly sends just + * the missing lsn 2 — no double-send, no ambiguity. This is the contrast to H4: + * different standby_lsn (1 vs 0) → different, correct streamed set. + */ +static void test_h4_control_standby_with_lsn1_gets_only_lsn2(void) +{ + char tmpdir[256]; + snprintf(tmpdir, sizeof(tmpdir), "/tmp/xce_coldstart_ctl_%d_%ld", + (int)getpid(), (long)time(NULL)); + TEST_ASSERT_EQUAL_INT(0, mkdir(tmpdir, 0700)); + TEST_ASSERT_EQUAL_INT(0, write_coldstart_segment(tmpdir)); + + xce_wal_stream_request_hook = capture_wal_stream_hook; + h4_capture_reset(); + + /* standby_lsn = 1 (already holds the first entry) catching up to committed 2. */ + int rc = ha_stream_wal_to_standby(/*ep=*/NULL, tmpdir, + /*standby_lsn=*/1, + /*committed_lsn=*/2); + xce_wal_stream_request_hook = NULL; + TEST_ASSERT_EQUAL_INT(0, rc); + + print_streamed("H4-control(standby has lsn1, standby_lsn=1):"); + + char cmd[512]; + snprintf(cmd, sizeof(cmd), "rm -rf %s", tmpdir); + if (system(cmd) != 0) + fprintf(stderr, "cleanup warning: rm -rf %s failed\n", tmpdir); + + /* Only the missing entry (lsn 2) is streamed; lsn 1 is already applied. */ + TEST_ASSERT_TRUE_MESSAGE(streamed_contains(2), "lsn 2 must be streamed"); + TEST_ASSERT_FALSE_MESSAGE(streamed_contains(1), + "standby_lsn=1 already holds lsn 1 — it must NOT be re-streamed " + "(reserved-0 makes standby_lsn=1 unambiguous: 'has the first entry')"); + printf("test_h4_control_standby_with_lsn1_gets_only_lsn2: PASS\n"); +} + +void test_coldstart_lsn0(void) +{ + RUN_TEST(test_h1_first_noop_lsn1_commits); + RUN_TEST(test_h2_real_entry_lsn2_commits); + RUN_TEST(test_h3_writable_lsn1_gate_effective); + RUN_TEST(test_h3b_gate_lsn1_blocks_until_reached); + RUN_TEST(test_h4_control_standby_with_lsn1_gets_only_lsn2); + RUN_TEST(test_h4_empty_standby_receives_lsn1_and_lsn2_no_hole); +} diff --git a/src/controller/testsuite/test_commit_barrier_reply.c b/src/controller/testsuite/test_commit_barrier_reply.c index c753cee7..a3d8c5cb 100644 --- a/src/controller/testsuite/test_commit_barrier_reply.c +++ b/src/controller/testsuite/test_commit_barrier_reply.c @@ -407,10 +407,11 @@ static void cbrp_setup(uint32_t sync_timeout_ms) g_obs_persist = &g_hc.persist; - /* Priming append so operation entries land at LSN >= 1 (the engine - * rejects lsn == 0 — accepted interim LSN-0 edge, ha-commit-engine). */ - TEST_ASSERT_EQUAL_INT( - 0, persist_wal_async(&g_hc.persist, WAL_JOB_UPDATE, "p", 1)); + /* Under the reserved-0 contract a fresh store starts current_lsn=1, so the + * first operation entry lands at LSN 1 with no priming append needed (LSN 0 + * is the reserved sentinel — never a real entry; the engine rejects + * lsn == 0). Tests below derive watermarks relative to lsn_before, so the + * operation entries' absolute base does not affect the assertions. */ TEST_ASSERT_EQUAL_UINT64(1, g_hc.persist.current_lsn); } diff --git a/src/controller/testsuite/test_commit_barrier_rollout.c b/src/controller/testsuite/test_commit_barrier_rollout.c index d2a3b341..402fad4d 100644 --- a/src/controller/testsuite/test_commit_barrier_rollout.c +++ b/src/controller/testsuite/test_commit_barrier_rollout.c @@ -401,9 +401,9 @@ static void test_fact_hook_append_is_retained_and_nonblocking(void) repl_set_commit_engine(&g_fp->repl, &g_fce); repl_broadcast_send_hook = cbr_drop_send_hook; - /* Priming async append so the fact entry lands at LSN 1 (the engine - * rejects lsn == 0 — accepted interim LSN-0 edge, ha-commit-engine) */ - TEST_ASSERT_EQUAL_INT(0, persist_wal_async(g_fp, WAL_JOB_UPDATE, "p", 1)); + /* Under the reserved-0 contract a fresh store starts current_lsn=1, so the + * fact entry lands at LSN 1 with no priming append (LSN 0 is the reserved + * sentinel — never a real entry; the engine rejects lsn == 0). */ fact_lsn = g_fp->current_lsn; /* the LSN the fact append will get */ TEST_ASSERT_EQUAL_UINT64(1, fact_lsn); diff --git a/src/controller/testsuite/test_commit_engine.c b/src/controller/testsuite/test_commit_engine.c index 80edd4e5..a5f3ee84 100644 --- a/src/controller/testsuite/test_commit_engine.c +++ b/src/controller/testsuite/test_commit_engine.c @@ -1296,8 +1296,10 @@ static void test_sync_append_wired_commits_on_per_entry_majority(void) pthread_t th; int rc, sync_rc; - /* Arrange: real persist, 2 standbys, engine n_peers=3 wired; one - * priming async append so the sync entry lands at LSN 1 */ + /* Arrange: real persist, 2 standbys, engine n_peers=3 wired. Under the + * reserved-0 contract a fresh store starts current_lsn=1, so the sync entry + * lands at LSN 1 with no priming append needed (LSN 0 is the reserved + * sentinel — never a real entry). */ g_sp = sync_persist_setup("maj", 2000); TEST_ASSERT_TRUE(repl_add_standby(&g_sp->repl, &g_ep_sa, "sb_a", 7100) >= 0); TEST_ASSERT_TRUE(repl_add_standby(&g_sp->repl, &g_ep_sb, "sb_b", 7101) >= 0); @@ -1305,7 +1307,6 @@ static void test_sync_append_wired_commits_on_per_entry_majority(void) repl_set_commit_engine(&g_sp->repl, &g_ce); repl_broadcast_send_hook = ce_drop_send_hook; - TEST_ASSERT_EQUAL_INT(0, persist_wal_async(g_sp, WAL_JOB_UPDATE, "p", 1)); /* Act: standby A's ACK covering the sync entry (LSN 1) arrives mid-wait */ g_rack = (repl_ack_arg_t){ &g_sp->repl, &g_ep_sa, 1, 30 }; @@ -1382,12 +1383,12 @@ static void test_sync_append_wired_single_member_commits_locally(void) { int rc, sync_rc; - /* Arrange: real persist, NO standbys, engine n_peers=1 wired; one - * priming async append so the sync entry lands at LSN 1 */ + /* Arrange: real persist, NO standbys, engine n_peers=1 wired. Under the + * reserved-0 contract the sync entry lands at LSN 1 with no priming append + * (fresh store starts current_lsn=1; LSN 0 is the reserved sentinel). */ g_sp = sync_persist_setup("solo", 2000); TEST_ASSERT_EQUAL_INT(0, commit_engine_init(&g_ce, g_sp, 1, 8)); repl_set_commit_engine(&g_sp->repl, &g_ce); - TEST_ASSERT_EQUAL_INT(0, persist_wal_async(g_sp, WAL_JOB_UPDATE, "p", 1)); /* Act */ sync_rc = -1; diff --git a/src/controller/testsuite/test_egress_filter.c b/src/controller/testsuite/test_egress_filter.c index 3f793050..2aeac598 100644 --- a/src/controller/testsuite/test_egress_filter.c +++ b/src/controller/testsuite/test_egress_filter.c @@ -203,13 +203,15 @@ static void ef_setup(void) * be dereferenced by repl_broadcast. */ repl_broadcast_send_hook = ef_drop_send_hook; - /* Priming append so subsequent real entries land at LSN >= 1 (the engine - * rejects lsn == 0; matches test_commit_barrier_reply.c cbrp_setup). This + /* Priming append so subsequent real entries land well clear of the reserved + * LSN-0 sentinel (the engine rejects lsn == 0). Under the reserved-0 contract + * a fresh store starts current_lsn = 1, so this first append lands at lsn 1 + * and current_lsn advances to 2; subsequent real entries are lsn >= 2. This * also ensures the live-tag test's appended LSN is non-zero, so an untagged * submit_lsn (0) cannot coincidentally match the appended LSN. */ TEST_ASSERT_EQUAL_INT( 0, persist_wal_async(&g_c.persist, WAL_JOB_UPDATE, "p", 1)); - TEST_ASSERT_EQUAL_UINT64(1, g_c.persist.current_lsn); + TEST_ASSERT_EQUAL_UINT64(2, g_c.persist.current_lsn); } static void ef_teardown(void) diff --git a/src/controller/testsuite/test_p2_integration.c b/src/controller/testsuite/test_p2_integration.c index 2c79d8af..cba3d717 100644 --- a/src/controller/testsuite/test_p2_integration.c +++ b/src/controller/testsuite/test_p2_integration.c @@ -243,8 +243,9 @@ static void test_single_node_committed_lsn_advances(void) /* Wire the engine exactly as cluster_init_phase2 does for n_peers=1 */ TEST_ASSERT_EQUAL_INT(0, commit_engine_init(&ce, p, 1, 8)); repl_set_commit_engine(&p->repl, &ce); - /* Priming append: sync entry below lands at LSN 1 (engine-wired path) */ - TEST_ASSERT_EQUAL_INT(0, persist_wal_async(p, WAL_JOB_UPDATE, "p", 1)); + /* Reserved-0 contract: fresh store starts current_lsn=1, so the sync entry + * below lands at LSN 1 with no priming append (LSN 0 is the reserved + * sentinel — the engine rejects lsn == 0). */ int sync_rc = -1; int rc = persist_wal_sync_append(p, WAL_JOB_SUBMIT, "job", 3, &sync_rc); @@ -363,10 +364,11 @@ static void* p2_ack_thread(void* arg) * Verify: persist_wal_sync_append returns 0, sync_rc == 0, * and committed_lsn advanced past committed_before. * - * Note: fresh persist_t starts with current_lsn=0, so after the first WAL - * write written_lsn = current_lsn - 1 = 0. The test therefore captures - * committed_before and asserts committed_after > committed_before (advance) - * OR committed_after == written_lsn (exact match), rather than asserting > 0. + * Note: under the reserved-0 contract a fresh persist_t starts current_lsn=1, + * so the first WAL write lands at lsn 1 (written_lsn = current_lsn - 1 = 1). + * The standby ACK below is for lsn 1, so it exactly covers the first entry. + * The test captures committed_before and asserts committed_after > + * committed_before (advance) OR committed_after == written_lsn (exact match). */ static persist_t* g_p9 = NULL; static int g_ep9_a, g_ep9_b; @@ -374,6 +376,8 @@ static struct p2_ack_args g_ack9; static void test_committed_lsn_advances_after_majority_ack(void) { + commit_engine_t ce; + g_p9 = make_p2_persist("clsn_ack"); TEST_ASSERT_NOT_NULL(g_p9); @@ -381,6 +385,14 @@ static void test_committed_lsn_advances_after_majority_ack(void) repl_add_standby(&g_p9->repl, (void *)&g_ep9_a, "sb_x", 7020); repl_add_standby(&g_p9->repl, (void *)&g_ep9_b, "sb_y", 7021); + /* Wire the commit engine exactly as cluster_init_phase2 does (n_peers=3). + * Under the reserved-0 contract the first WAL entry is LSN 1, which routes + * through the engine-wired walAck path (commit_submit), so the engine must + * be wired — the old engine-less path only "worked" via the LSN-0 + * immediate-confirm escape hatch, which no longer applies. */ + TEST_ASSERT_EQUAL_INT(0, commit_engine_init(&ce, g_p9, 3, 8)); + repl_set_commit_engine(&g_p9->repl, &ce); + /* Give enough time for the ACK thread to fire before timeout */ g_p9->config.sync_timeout_ms = 2000; @@ -405,11 +417,10 @@ static void test_committed_lsn_advances_after_majority_ack(void) TEST_ASSERT_EQUAL_INT(0, rc); TEST_ASSERT_EQUAL_INT(0, sync_rc); /* committed_lsn must have advanced to written_lsn (Req 4.1). - * written_lsn = current_lsn - 1 after persist_wal_async; for a fresh - * persist_t this equals 0 on the very first write. We therefore check - * that committed_after > committed_before (advance happened) or, when - * both are 0 and written_lsn is also 0, that they are equal (lsn was - * committed at 0 → valid: majority ACK was received). */ + * written_lsn = current_lsn - 1 after persist_wal_async; under the + * reserved-0 contract this equals 1 on the very first write. We check + * that committed_after > committed_before (advance happened) or that + * committed_after == written_lsn (the first entry committed exactly). */ uint64_t committed_after = atomic_load_explicit(&g_p9->committed_lsn, memory_order_acquire); @@ -422,6 +433,7 @@ static void test_committed_lsn_advances_after_majority_ack(void) committed_after > committed_before || committed_after == written_lsn, "committed_lsn must advance after majority ACK (Req 4.1)"); + commit_engine_fini(&ce); teardown_p2_persist(g_p9); g_p9 = NULL; printf("test_committed_lsn_advances_after_majority_ack: PASS\n"); diff --git a/src/controller/testsuite/test_p3_hardening.c b/src/controller/testsuite/test_p3_hardening.c index 71c8c9e0..22eb61d3 100644 --- a/src/controller/testsuite/test_p3_hardening.c +++ b/src/controller/testsuite/test_p3_hardening.c @@ -226,10 +226,12 @@ static void test_p3_wal_torn_write_after_checkpoint(void) teardown_p3_persist(p); - /* Truncate to exactly 4 complete post-checkpoint entries (keep lsns 4..7, - * drop lsn 8). The segment also holds pre-checkpoint entries 0..2 and - * the checkpoint marker at lsn 3 — that is 4 entries × entry_size bytes - * of header+payload before the post-checkpoint block. + /* Truncate to exactly 4 complete post-checkpoint entries (keep lsns 5..8, + * drop lsn 9). The segment also holds pre-checkpoint entries 1..3 and + * the checkpoint marker at lsn 4 — that is 4 entries × entry_size bytes + * of header+payload before the post-checkpoint block. (LSN 0 is reserved + * as the empty sentinel, so the first real entry is lsn 1; the truncation + * math below is purely offset/count-based and is unaffected by the base.) * * Pre-checkpoint entries: 3 × (hdr + 5) + 1 × (hdr + 0) checkpoint marker * Post-checkpoint entries: 5 × (hdr + 9); keep 4, drop 1.