fix: close bug-847 (un-ignore fixed reproducer), confirm bug-1307 fixed via soak

bug-847 (catch-up WAL batch drops payloads across capnp segment boundaries):
its purpose-built RED reproducer, test_wal_batch_roundtrip_large_payloads
(14 entries x 872-byte payloads, 12.2KB total, genuinely spans multiple
4096-byte capnp segments per capn-malloc.c's create()), was still
TEST_IGNORE_MESSAGE'd. Un-ignoring it and running under ASAN with leak
detection: clean pass, byte-exact payloads, zero leaks, 2/2 runs. Traced the
vendored c-capnproto far-pointer machinery (write_ptr/capn_setp in capn.c)
to confirm the cross-segment struct->Data reference path is correct by
construction — whatever regressed this already got fixed elsewhere (library
bump or an incidental request.c change); the ignore was just never removed.
No production code changed.

bug-1307 (task-5.7 election-jitter data-safety regression: 7 divergences +
4 job losses over 6 chaotic rounds): re-ran the exact reproducing scenario
(6-round chaotic soak, real ext4, ASAN binaries) — 21 jobs, 0 divergence,
0 job loss, clean PASS. Confirms the raft-ha-core-safety Phase 5 work
resolved it.

Both bugs were the last 2 of the 7 flagged in the prior staleness sweep;
all 7 are now closed (6 were already fixed and undetected, 1 needed the
test un-ignored).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SDWVNQLKxqTVHh42cH7rV3
This commit is contained in:
Rongsong Shen 2026-07-03 03:13:29 +00:00
parent acddc3204a
commit 78e8ec817e
3 changed files with 28 additions and 22 deletions

View file

@ -354,3 +354,9 @@ Attempted to validate ha-wal-replication 5.4 ("job appears in joblist on primary
## Key Learning (2026-07-02, bug-1401 FIXED) — a generated codec's `free_X(struct*)` vs `free_X_ptr(struct**)` split is easy to get wrong in a hand-written client wrapper
`decode_X_ptr()` (capnp-generated) `calloc`s the top-level struct THEN fills its fields; the matching free is `free_X_ptr(&ptr)` (frees fields + the struct + nulls the caller's pointer), NOT `free_X(ptr)` (fields only — the codegen intentionally splits these because `free_X` is also reused on stack-allocated structs, e.g. this project's own `test_ctrlstatus_roundtrip_all_fields` test calls `free_CtrlStatus(&out)` on a stack var, which is correct there but wrong for a heap ptr). `src/include/client.h`'s `xce_free_ctrlstatus(s)` macro used the fields-only variant, leaking `sizeof(xce_ctrlstatus_t)` (120 bytes) on every real `ctrlstatus` call. **Root cause of it going undetected**: every existing ctrlstatus unit test built its `xce_ctrlstatus_t` via the stack-decode helper (`decode_CtrlStatus`) and freed it with the correct raw `free_CtrlStatus(&out)` call directly — none of them ever exercised the actual client-facing `xce_free_ctrlstatus()` macro that real code paths use. **Rule going forward**: whenever a generated codec exposes both a bare `free_X` and a `free_X_ptr`, any hand-written `xce_free_*`/`xcecli_free_*` wrapper macro over it MUST use `free_X_ptr(&(s))` if the value being freed came from a `decode_X_ptr()` heap allocation (check what the corresponding `xce_X(...)` API call's `MOVE_PTR` source was decoded with, not just what looks intuitively "free-shaped"). And: a roundtrip test that exercises only the raw encode/decode/free primitives is NOT a substitute for a test that goes through the actual client macro/wrapper — add at least one regression per free-wrapper that uses the real heap-allocating decode path + the real macro. RED confirmed the exact 120-byte/1-alloc LeakSanitizer signature from the bug report before applying the fix.
## Key Learning (2026-07-03) — buglog.json's biggest lie is silence: 6 of 7 swept "open" bugs were already fixed and just never marked
Swept 7 old "not yet fixed" buglog entries (846, 847, 855, 1096, 1147, 1198, 1307) by checking the ACTUAL current code/tests instead of trusting the buglog text — 6 were already fixed (some for over a week), only bug-847 needed real action (and even that action was "un-ignore the regression test," not a production code change — its underlying condition was already gone). Common thread: every one of these had a purpose-built regression test or an explicit in-code citation that would have revealed the fix immediately, but nobody closed the loop after landing the fix under a DIFFERENT bug number or spec task. **Rule going forward**: before spending real investigation budget on an "open" bug, first check (a) does it have a named regression test — run it directly; (b) does its bug ID appear in a code comment — grep for it; (c) does its described symptom show up in the relevant spec's tasks.md as a since-closed task. All three are cheap (single tool call) and would have caught 6/7 of these immediately. **Second rule**: when you land a fix, grep buglog.json for the bug ID(s) it plausibly closes (including "family" bugs like bug-851's many sub-numbers) BEFORE moving to the next task — don't rely on remembering to circle back later, because "later" doesn't reliably happen and staleness compounds silently. [[project_ha_bootstrap_progress]]
## Key Learning (2026-07-03, bug-847 investigation) — trace the actual library mechanism before trusting a plausible-sounding root-cause hypothesis
A prior investigation (and a fresh subagent re-investigation) both converged on "the encoder captures a capnp segment pointer once outside a loop and reuses it stale across iterations" as bug-847's root cause — plausible-sounding, cites real code (`cs = capn_root(&c).seg;` before a `for` loop in `xce_wal_batch_encode_raw`, src/lib/request.c). Tracing the ACTUAL c-capnproto library (`new_object`/`new_data`/`write_ptr`/`capn_setp` in the vendored `capn.c`) showed this is a non-issue by design: `new_object` always re-resolves and re-tags `.seg` on segment overflow regardless of what stale segment pointer was passed in, and `write_ptr`/`capn_setp` correctly emit far/double-far pointers whenever the source and target objects land in different segments. The theory was never tested against the library source, only against the observed symptom — and the observed symptom (RED test failure) turned out to no longer reproduce at all once actually run. Lesson: a hypothesis that "sounds mechanistically right" and cites real line numbers is not the same as a verified root cause — for a data-loss bug specifically, always run the existing/write a NEW regression test before accepting a hypothesis, even one two independent investigations agreed on.