From a86de77f91876cb64689ae8941462b46bc8411a1 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Fri, 31 Jul 2026 09:05:22 +0000 Subject: [PATCH] fix(waldump): decode WAL_CONFIG_MARKER, distinguish trimmed gaps from real loss wal_type_name() never had a case for WAL_CONFIG_MARKER (type 50) -- every config-change marker entry printed as UNKNOWN instead of a real, defined type. Added the case plus payload decode (tag/sha256) matching config_repl.c's config_marker_encode/_decode byte layout, hand-decoded here rather than linking config_repl.c into xce_test's link graph. Gap detection also had no way to tell "committed prefix missing!" (real loss) apart from a normal checkpoint-triggered WAL trim (persist.c's trim_wal_segments, floor-guarded so it never removes a segment a standby still needs) -- every long-running node's WAL, once checkpointed, would false-alarm on every inspection. Cross-checks the sibling data/snap/ directory (snapshot LSN is literally in the filename, snap-.capnp) and downgrades the message when a snapshot covers the gap. Adds waldump_stats_t.leading_gap_explained and 3 new tests plus one assertion on the existing gap test. --- src/controller/testsuite/test_waldump.c | 134 ++++++++++++++++++++++++ src/controller/testsuite/waldump.c | 92 +++++++++++++++- src/controller/testsuite/waldump.h | 7 ++ 3 files changed, 228 insertions(+), 5 deletions(-) diff --git a/src/controller/testsuite/test_waldump.c b/src/controller/testsuite/test_waldump.c index cdbc51a3..ff5c9d74 100644 --- a/src/controller/testsuite/test_waldump.c +++ b/src/controller/testsuite/test_waldump.c @@ -2,6 +2,8 @@ #include #include #include +#include +#include #include #include @@ -152,6 +154,135 @@ static void test_missing_prefix_flagged_as_gap(void) TEST_ASSERT_EQUAL_UINT64(13, st.first_lsn); /* prefix [1..12] absent → exactly one gap */ TEST_ASSERT_EQUAL_UINT(1, st.n_gaps); + /* g_dir has no sibling data/snap/ at all → nothing can explain it */ + TEST_ASSERT_EQUAL_INT(0, st.leading_gap_explained); +} + +/* ------------------------------------------------------------------ */ +/* WAL_CONFIG_MARKER must decode as its own named type, not fall into */ +/* the generic UNKNOWN bucket (a real, defined type — wal_types.h). */ +/* ------------------------------------------------------------------ */ +static void test_config_marker_type_decoded(void) +{ + fresh_dir(); + char path[512]; + FILE* fp = fopen(seg_path(1, path, sizeof(path)), "wb"); + TEST_ASSERT_NOT_NULL(fp); + + uint32_t last = WAL_CRC_NONE; + put_entry(fp, 0, 1, WAL_NOOP, NULL, 0, &last, 1); + + uint8_t marker[WAL_CONFIG_MARKER_PAYLOAD_LEN]; + uint64_t tag = 0x1122334455667788ULL; + for (int i = 0; i < 8; i++) + marker[i] = (uint8_t)(tag >> (56 - i * 8)); + memset(marker + 8, 0xab, 32); + put_entry(fp, 3, 2, WAL_CONFIG_MARKER, marker, sizeof(marker), &last, 0); + fclose(fp); + + char* buf = NULL; + size_t bufsz = 0; + FILE* mem = open_memstream(&buf, &bufsz); + TEST_ASSERT_NOT_NULL(mem); + + waldump_stats_t st; + int rc = waldump_path(g_dir, mem, &st); + fclose(mem); + + TEST_ASSERT_EQUAL_INT(0, rc); + TEST_ASSERT_NOT_NULL(strstr(buf, "CONFIG_MARKER")); + TEST_ASSERT_NULL(strstr(buf, "UNKNOWN")); + TEST_ASSERT_NOT_NULL(strstr(buf, "tag=1122334455667788")); + free(buf); +} + +/* ------------------------------------------------------------------ */ +/* A leading gap covered by a sibling data/snap/ snapshot is normal, */ +/* checkpoint-triggered WAL trim — waldump must say so, not alarm. */ +/* ------------------------------------------------------------------ */ +static void test_leading_gap_explained_by_covering_snapshot(void) +{ + fresh_dir(); + char wal_dir[300], snap_dir[300]; + snprintf(wal_dir, sizeof(wal_dir), "%s/data/wal", g_dir); + snprintf(snap_dir, sizeof(snap_dir), "%s/data/snap", g_dir); + char data_dir[300]; + snprintf(data_dir, sizeof(data_dir), "%s/data", g_dir); + TEST_ASSERT_EQUAL_INT(0, mkdir(data_dir, 0755)); + TEST_ASSERT_EQUAL_INT(0, mkdir(wal_dir, 0755)); + TEST_ASSERT_EQUAL_INT(0, mkdir(snap_dir, 0755)); + + /* Snapshot at LSN 12 covers the trimmed prefix [1..12] below. */ + char snap_file[400]; + snprintf(snap_file, sizeof(snap_file), "%s/snap-%016x.capnp", snap_dir, + 0xc); + FILE* sf = fopen(snap_file, "wb"); + TEST_ASSERT_NOT_NULL(sf); + fclose(sf); + + char segp[400]; + snprintf(segp, sizeof(segp), "%s/seg-%016lx.wal", wal_dir, + (unsigned long)13); + FILE* fp = fopen(segp, "wb"); + TEST_ASSERT_NOT_NULL(fp); + uint32_t last = WAL_CRC_NONE; + put_entry(fp, 0, 13, WAL_NOOP, NULL, 0, &last, 1); + fclose(fp); + + char* buf = NULL; + size_t bufsz = 0; + FILE* mem = open_memstream(&buf, &bufsz); + TEST_ASSERT_NOT_NULL(mem); + + waldump_stats_t st; + int rc = waldump_path(g_dir, mem, &st); + fclose(mem); + + TEST_ASSERT_EQUAL_INT(0, rc); + TEST_ASSERT_EQUAL_UINT(1, st.n_gaps); + TEST_ASSERT_NOT_EQUAL(0, st.leading_gap_explained); + TEST_ASSERT_NOT_NULL(strstr(buf, "covered by snapshot")); + TEST_ASSERT_NULL(strstr(buf, "committed prefix missing!")); + free(buf); +} + +/* ------------------------------------------------------------------ */ +/* A snapshot that does NOT cover the full gap must NOT explain it — */ +/* the alarming wording stays, since the gap could still be real loss. */ +/* ------------------------------------------------------------------ */ +static void test_leading_gap_not_explained_by_insufficient_snapshot(void) +{ + fresh_dir(); + char wal_dir[300], snap_dir[300], data_dir[300]; + snprintf(data_dir, sizeof(data_dir), "%s/data", g_dir); + snprintf(wal_dir, sizeof(wal_dir), "%s/data/wal", g_dir); + snprintf(snap_dir, sizeof(snap_dir), "%s/data/snap", g_dir); + TEST_ASSERT_EQUAL_INT(0, mkdir(data_dir, 0755)); + TEST_ASSERT_EQUAL_INT(0, mkdir(wal_dir, 0755)); + TEST_ASSERT_EQUAL_INT(0, mkdir(snap_dir, 0755)); + + /* Snapshot at LSN 5 does NOT cover the trimmed prefix [1..12] below. */ + char snap_file[400]; + snprintf(snap_file, sizeof(snap_file), "%s/snap-%016x.capnp", snap_dir, + 0x5); + FILE* sf = fopen(snap_file, "wb"); + TEST_ASSERT_NOT_NULL(sf); + fclose(sf); + + char segp[400]; + snprintf(segp, sizeof(segp), "%s/seg-%016lx.wal", wal_dir, + (unsigned long)13); + FILE* fp = fopen(segp, "wb"); + TEST_ASSERT_NOT_NULL(fp); + uint32_t last = WAL_CRC_NONE; + put_entry(fp, 0, 13, WAL_NOOP, NULL, 0, &last, 1); + fclose(fp); + + waldump_stats_t st; + int rc = waldump_path(g_dir, stdout, &st); + TEST_ASSERT_EQUAL_INT(0, rc); + TEST_ASSERT_EQUAL_UINT(1, st.n_gaps); + TEST_ASSERT_EQUAL_INT(0, st.leading_gap_explained); } /* ------------------------------------------------------------------ */ @@ -288,6 +419,9 @@ void test_waldump(void) UnitySetTestFile(__FILE__); RUN_TEST(test_contiguous_log_has_no_gaps); RUN_TEST(test_missing_prefix_flagged_as_gap); + RUN_TEST(test_config_marker_type_decoded); + RUN_TEST(test_leading_gap_explained_by_covering_snapshot); + RUN_TEST(test_leading_gap_not_explained_by_insufficient_snapshot); RUN_TEST(test_duplicate_submit_detection); RUN_TEST(test_interior_gap_flagged); RUN_TEST(test_crc_break_sets_recovered_tip); diff --git a/src/controller/testsuite/waldump.c b/src/controller/testsuite/waldump.c index 32d5929f..4c8752fa 100644 --- a/src/controller/testsuite/waldump.c +++ b/src/controller/testsuite/waldump.c @@ -46,6 +46,7 @@ static const char* wal_type_name(uint32_t t) case WAL_QUEUE_CTRL: return "QUEUE_CTRL"; case WAL_WORKER_CTRL: return "WORKER_CTRL"; case WAL_NOOP: return "NOOP"; + case WAL_CONFIG_MARKER: return "CONFIG_MARKER"; case WAL_CHECKPOINT: return "CHECKPOINT"; default: return "UNKNOWN"; } @@ -127,6 +128,63 @@ static int decode_jobstate_info(const uint8_t* payload, uint32_t len, return 0; } +/* Decode a WAL_CONFIG_MARKER payload -- fixed 40-byte form (8-byte + * big-endian version_tag + 32-byte raw sha256), never capnp-encoded + * (config_repl.c's config_marker_encode/config_marker_decode). Hand-decoded + * here rather than linking config_repl.c, which would pull controller- + * internal dependencies into xce_test's link graph (meson.build's + * xce_test_src is deliberately limited to read-only dump/compare tools). + * Mirrors config_marker_decode's byte layout exactly. */ +static int decode_config_marker_info(const uint8_t* payload, uint32_t len, + uint64_t* out_tag, uint8_t sha256_out[32]) +{ + if (!payload || len < WAL_CONFIG_MARKER_PAYLOAD_LEN) + return -1; + uint64_t tag = 0; + for (int i = 0; i < 8; i++) + tag = (tag << 8) | payload[i]; + *out_tag = tag; + memcpy(sha256_out, payload + 8, 32); + return 0; +} + +/* Highest LSN among sibling data/snap/snap-.capnp files next to + * `wal_dir` (persist.c hardcodes both as siblings under a node's data/, + * e.g. .../data/wal and .../data/snap). A leading WAL gap up to this LSN is + * explained by normal checkpoint-triggered trim (persist.c's + * trim_wal_segments only removes segments below a replication-safe floor, + * never a segment a standby still needs to catch up) rather than genuine + * loss -- without this check, EVERY long-running node's WAL (which will + * have been checkpointed at least once) prints the same alarming + * "(committed prefix missing!)" wording a real, unexplained loss would, + * making the message a standing false alarm. Returns 0 if `wal_dir` isn't + * named "wal", or no sibling snap dir/files exist (unknown -- callers keep + * today's un-downgraded wording, the conservative default). */ +static uint64_t highest_snapshot_lsn(const char* wal_dir) +{ + char snap_dir[4096]; + snprintf(snap_dir, sizeof(snap_dir), "%s", wal_dir); + char* slash = strrchr(snap_dir, '/'); + if (!slash || strcmp(slash + 1, "wal") != 0) + return 0; + snprintf(slash + 1, sizeof(snap_dir) - (size_t)(slash + 1 - snap_dir), + "snap"); + + DIR* d = opendir(snap_dir); + if (!d) + return 0; + uint64_t max_lsn = 0; + struct dirent* e; + while ((e = readdir(d)) != NULL) { + uint64_t lsn; + if (sscanf(e->d_name, "snap-%" SCNx64 ".capnp", &lsn) == 1 && + lsn > max_lsn) + max_lsn = lsn; + } + closedir(d); + return max_lsn; +} + /* True if `dir` directly contains at least one seg-*.wal file. */ static int dir_has_segments(const char* dir) { @@ -241,7 +299,7 @@ static int collect_segments(const char* wal_dir, uint64_t* seg_lsns, int max) static void scan_segment(const char* seg_path, FILE* out, uint32_t* last_crc, uint64_t* prev_lsn, uint64_t* prev_submit_jobid, uint32_t* prev_submit_pcrc, - waldump_stats_t* st, + waldump_stats_t* st, uint64_t snap_lsn, waldump_visit_fn visit, void* visit_ctx, int* visit_stop) { @@ -309,10 +367,15 @@ static void scan_segment(const char* seg_path, FILE* out, /* Gap detection against the running LSN sequence. */ if (*prev_lsn == 0) { if (hdr.lsn > 1) { + int explained = snap_lsn >= hdr.lsn - 1; fprintf(out, " *** GAP: missing prefix LSN 1..%" PRIu64 - " (%" PRIu64 " entries absent) ***\n", - hdr.lsn - 1, hdr.lsn - 1); + " (%" PRIu64 " entries absent)%s ***\n", + hdr.lsn - 1, hdr.lsn - 1, + explained ? " -- covered by snapshot, expected " + "post-checkpoint trim" + : ""); st->n_gaps++; + st->leading_gap_explained = explained; } } else if (hdr.lsn > *prev_lsn + 1) { fprintf(out, " *** GAP: missing LSN %" PRIu64 "..%" PRIu64 " ***\n", @@ -404,6 +467,20 @@ static void scan_segment(const char* seg_path, FILE* out, } /* a non-submit entry breaks the adjacency for dup detection */ *prev_submit_jobid = 0; + } else if (hdr.type == WAL_CONFIG_MARKER) { + uint64_t tag = 0; + uint8_t sha[32]; + if (decode_config_marker_info(payload, hdr.payload_len, &tag, + sha) == 0) { + char hex[65]; + for (int i = 0; i < 32; i++) + snprintf(hex + i * 2, 3, "%02x", sha[i]); + snprintf(info, sizeof(info), "tag=%016" PRIx64 " sha256=%s", + tag, hex); + } else { + snprintf(info, sizeof(info), "tag=?"); + } + *prev_submit_jobid = 0; } else { /* a non-submit entry breaks the adjacency for dup detection */ *prev_submit_jobid = 0; @@ -505,6 +582,7 @@ int waldump_path_visit(const char* path, FILE* out, waldump_stats_t* stats, } st.n_segments = (uint32_t)nseg; + uint64_t snap_lsn = highest_snapshot_lsn(wal_dir); fprintf(out, "== WAL: %s (%d segment%s) ==\n", wal_dir, nseg, nseg == 1 ? "" : "s"); @@ -521,7 +599,7 @@ int waldump_path_visit(const char* path, FILE* out, waldump_stats_t* stats, snprintf(seg_path, sizeof(seg_path), WAL_SEG_FMT, wal_dir, seg_lsns[i]); scan_segment(seg_path, out, &last_crc, &prev_lsn, - &prev_submit_jobid, &prev_submit_pcrc, &st, + &prev_submit_jobid, &prev_submit_pcrc, &st, snap_lsn, visit, ctx, &visit_stop); } @@ -532,7 +610,11 @@ int waldump_path_visit(const char* path, FILE* out, waldump_stats_t* stats, " recovered tip: %" PRIu64 "\n", st.first_lsn, st.last_lsn, st.recovered_tip); fprintf(out, " gaps: %u%s crc chain: %s\n", st.n_gaps, - (st.n_gaps && st.first_lsn > 1) ? " (committed prefix missing!)" : "", + (st.n_gaps && st.first_lsn > 1) + ? (st.leading_gap_explained + ? " (explained by checkpoint/snapshot -- not a loss)" + : " (committed prefix missing!)") + : "", st.crc_broken ? "BROKEN" : "intact"); if (st.n_dup_submits > 0) fprintf(out, " duplicate submits: %u (byte-identical: %u) — bug-845\n", diff --git a/src/controller/testsuite/waldump.h b/src/controller/testsuite/waldump.h index 2a34b37b..4a61b41b 100644 --- a/src/controller/testsuite/waldump.h +++ b/src/controller/testsuite/waldump.h @@ -29,6 +29,13 @@ typedef struct { uint32_t n_dup_identical; /* of n_dup_submits, those whose payload is * byte-identical to the prior entry */ int crc_broken; /* nonzero if a CRC/chain break truncated a segment */ + int leading_gap_explained; /* nonzero if the leading (prefix) gap's + * upper LSN is covered by a sibling + * data/snap/snap-.capnp file — i.e. + * normal checkpoint-triggered WAL trim + * (persist.c trim_wal_segments), not + * genuine loss. 0 if there was no leading + * gap, or no covering snapshot was found. */ } waldump_stats_t; /* One physically-present, CRC-valid WAL entry, surfaced to a visitor so a