feat(config-upgrade-snapshot): task 8.9 — live WAL-trim-under-chaos soak infra, blocked on bug-1474

Add run_config_upgrade_wal_trim_round() / --config-upgrade-wal-trim to
run-ha-soak.sh: forces real WAL segment rotation via small
--wal-segment-bytes, kills a non-primary node, drives a live config
upgrade to fire the marker-triggered checkpoint's forced trim, then
inspects survivors' on-disk WAL directory before the killed node
rejoins to directly verify the mid-catch-up retention guard
(repl_trim_lsn/trim_wal_segments, bug-1147) preserves what it still
needs.

Two independent live 3-node runs both reproduce a real, pre-existing,
out-of-spec-boundary defect (bug-1474): a heartbeat reconnect-sweep
race reuses a just-killed standby's freed replicator slot and clobbers
its retention guard before trim ever runs. Root-caused via repl.c/ha.c
and confirmed via git blame to predate this spec and be reachable
under the old periodic-checkpoint trigger identically -- not caused by
anything config-upgrade-snapshot changed. Task 8.9 stays blocked per
tasks.md until bug-1474 is fixed upstream; infra is done and correctly
proves the violation.

Independently reviewed: round design verified faithful to the
unit-level fixture, bug-1474 diagnosis independently re-derived from
source and raw logs (not trusted from the implementer's report).
This commit is contained in:
Rongsong Shen 2026-07-25 21:10:59 +00:00
parent b78efd03b4
commit 8b3e2ab03c
3 changed files with 452 additions and 1 deletions

View file

@ -147,9 +147,12 @@
- Observable: across a full chaotic-soak run with configuration upgrades in flight, no standby ever loses a WAL segment it still needed for catch-up
- _Requirements: 7.4_
- _Depends: 7, 8.1, 8.2, 8.3, 8.4_
- _Blocked: bug-1474 — pre-existing, out-of-spec-boundary HA connection-management defect (`repl_add_standby`'s first-empty-slot allocation, `repl.c:74-133`, unconditional `memset` at line 112; `primary_heartbeat_fn`'s reconnect sweep, `ha.c:9219-9254`; both blame to commits `7b396502`/`fa66d1ba9`/`8ef64a7e`, weeks before this spec). Sequence: killing a standby correctly frees its replicator slot and sets the bug-1147 `last_disconnected_lsn` retention guard; ~9s later the primary's routine heartbeat reconnect sweep spuriously treats an ALREADY-connected peer (connected via a promoted-vote-channel, not yet a heartbeat-task connection) as missing, spawns a redundant "late-joiner" reconnect, and `repl_add_standby` reallocates the just-freed slot — `memset`-clobbering the retention guard before any trim runs. Confirmed reproducible 2/2 against independent live 3-node clusters, and confirmed via `git blame` to be reachable identically under the pre-existing periodic-checkpoint trigger — nothing this spec added or changed. The live-soak round (`run_config_upgrade_wal_trim_round`, `run-ha-soak.sh`) is complete, committed, and correctly reproduces the violation; independently reviewed and confirmed sound (round design faithfully mirrors the unit fixture's mechanics; root-cause diagnosis independently re-derived from `repl.c`/`ha.c` plus raw preserved logs, not trusted from the implementer's report alone). No scoped adjustment to the round can dodge the race without suppressing the very live-cluster heartbeat behavior Req 7.4 requires testing under. Unblocks when bug-1474 is fixed by whoever owns HA connection/replicator-slot management — no further config-upgrade-snapshot work needed once that lands (re-run `run-ha-soak.sh --config-upgrade-wal-trim` to confirm clean PASS). See `.wolf/buglog.json` bug-1474 for the full forensic trace._
## Implementation Notes
- **Task 8.9 (live soak, genuinely requires chaos — unlike 8.5/8.7/8.8's unit-level redirections): infra complete, task BLOCKED on a new pre-existing bug (bug-1474).** Unlike every other 8.x subtask, this one's own bullet ("under the full chaotic multi-kill soak rather than an isolated fixture") makes the point of the task proving the mid-catch-up WAL-trim guard (`repl_trim_lsn`/`trim_wal_segments`, bug-1147) holds under REAL timing uncertainty, not just deterministic fixture control — so it could not be redirected to a cheaper unit-level proof the way 8.5/8.7/8.8 were. Built `run_config_upgrade_wal_trim_round()` + `--config-upgrade-wal-trim` in `run-ha-soak.sh` (deploys with small `--wal-segment-bytes`, kills a non-primary TARGET, forces ≥2 real segment rotations past its floor on survivors, submits a live config upgrade to fire the marker-triggered checkpoint's trim, then directly inspects survivors' on-disk `wal/` directory BEFORE restarting TARGET — mirrors the unit fixture's own seg-1-gone/seg-2-preserved/seg-3-preserved assertions against real files instead of a tmpdir fixture). Ran twice against independent live 3-node clusters: **both runs failed** — the floor segment protecting the killed target was trimmed both times. Root-caused (not guessed) to **bug-1474**, a general HA connection-management race entirely unrelated to config-marker triggering: killing a standby frees its replicator slot and sets the retention guard; ~9s later the primary's routine heartbeat reconnect sweep spuriously treats an already-connected peer as "missing" (comparing indices under two different conventions — relative `votes_idx` vs. absolute `peer_idx`, per the reviewer's independent finding), spawns a redundant reconnect, and `repl_add_standby`'s first-empty-slot allocation reuses the just-freed slot, `memset`-clobbering the guard before trim ever runs. Reviewer independently re-derived this from `repl.c:74-133`/`ha.c:9219-9254` plus raw preserved logs (not the implementer's quoted excerpts) and confirmed via `git blame` the responsible code predates this spec by weeks and is reachable identically under the OLD periodic-checkpoint trigger — genuinely out of this spec's boundary, same class of finding as bug-1471/1472. Reviewer also confirmed no scoped adjustment to the round's chaos could dodge the race without suppressing the live heartbeat behavior Req 7.4 requires testing under, so BLOCKED (not a redesign) is the correct call. **Spec-wide status after this task**: every implementable task (1 through 8.5, 8.7, 8.8) is `[x]`; the only two remaining open items (8.6, 8.9) are both blocked on real, independently-verified, out-of-spec-boundary HA defects this spec's own testing effort discovered (bug-1472, bug-1474) — neither blocks any OTHER task in this spec, and neither requires further config-upgrade-snapshot work once fixed upstream.
- **Task 8.8 (unit-level, no live soak needed — deliberately excludes 8.6):** proves Req 4.4/7.2 (a second marker committing before the first's install completes must cleanly supersede it, never conflate) via two independently-built `t42`/`t43`-family fixtures in `test_config_repl.c`: `c_control` commits+applies+checkpoints marker A then marker B normally; `c_raced` holds the shared `checkpoint_in_progress` CAS across *both* markers' commit+apply (inlining `t43_commit_marker`'s 4-step body rather than changing its signature), so both markers' forced-checkpoint attempts hit `-EBUSY` and skip — genuinely "still installing." Mid-race assertions (captured before releasing the CAS) prove `mc->cluster_config` already reflects marker B while `cfgref_present` is still false and the fence is set; post-resolve assertions (after releasing the CAS, calling `persist_checkpoint()`, then an explicit `upgrade_materialization_verify()`) prove the settled state reflects only B. Discovered mid-implementation that whole-file `snapdiff_paths` byte-identity between the two fixtures' final checkpoints does NOT hold — `c_control`'s marker-A checkpoint physically consumes one extra WAL LSN (`persist_write_checkpoint_marker`, persist.c:1684) before marker B is appended, so B lands at a different absolute LSN in each fixture, and `write_snapshot`'s sentinel header embeds `checkpoint_lsn` at byte offset 8. Adapted to a field-aware comparison (full `memcmp` from byte 16 onward) plus `snapdiff_paths` diagnostic corroboration that offset 8 is the *only* divergence — a more precise proof than blind byte-identity, reviewer independently re-derived and confirmed this against `write_snapshot`'s actual layout rather than trusting the claim. One RED→GREEN cycle (`bug-1473`, test-construction gap not a production bug): the bare `persist_checkpoint()` resolve call doesn't itself trigger `upgrade_materialization_verify()` in a delaymgr-less unit fixture — production's self-healing (`schedule_upgrade_materialization_retry`, fired from `ha.c:2541-2542` on every commit-advance) only runs on a live cluster; the fixture needs the manual call, mirroring the existing 6.1-c fence tests' pattern. Reviewer's only findings were non-blocking style notes (could have used the public `upgrade_materialization_retry_fn` wrapper instead of calling `upgrade_materialization_verify` directly — functionally identical here since the fence was already independently confirmed set). Suite: 1027 → 1028 tests, 0 failures.
- **Task 8.7 (both scenarios, unit-level, no live soak needed):** design.md's Testing Strategy was explicit this task should use deterministic fault injection rather than chaotic-soak timing probability, and both hooks needed already existed from earlier tasks (`persist_marker_checkpoint_fault_hook`, task 8.1; `xce_upgrade_marker_committed_hook`, task 7) — task 8.7 was purely additive test work, zero production code touched. **Scenario 1** (`test_marker_checkpoint_fault_hook_crash_then_restart_resumes_materialization`, `test_upgrade_snapshot_apply.c`) builds on the existing seam test (which only proved the fault hook fires and gates the checkpoint call) by clearing the fault, tearing down without deleting the on-disk WAL, and booting a genuinely fresh `persist_t`/`cluster_t` via `persist_recover()` — a real second-process boot-replay simulation, deliberately not a same-process re-call (which the test's own comment argues would be a weaker, trivially-passing proof). **Scenario 2** (`test_leadership_change_mid_materialization_completes_via_same_generic_apply_drain`, `test_config_repl.c`) proves Req 5.4 by building a promoted member and a non-promoted control member from byte-identical committed-but-unapplied markers, draining each via the respective real production path (`persist_apply_pump_run_sync`, which production promotion — `raft_cb_become_leader`, `ha.c` — calls unconditionally right after the role flip; vs. plain `persist_apply_pump_run` for the control), and asserting the resulting checkpoints are byte-identical via `snapdiff_paths` (task 8.4's tool). Reviewer independently verified (not trusted) both design claims by reading `ha.c` directly (`upgrade_materialization_fenced` only refuses an *already*-fenced member; the sync-drain call is unconditional and unguarded by role-specific logic) and specifically interrogated whether this was a tautology (calling the same underlying function twice) — concluded it genuinely is not, since the real variable under test is role state at drain time plus the production entry point, and the test separately asserts promotion alone has zero effect on the fence. Also confirmed via direct comparison against the existing 6.2/8.3 fence tests that this is new *continuation* coverage (fence unset → promotion succeeds → drain completes → config activates), not a duplicate of the existing *gating* coverage (fence set → promotion refused, never reaching the drain).