feat(config-upgrade-snapshot): task 8.8 — overlapping config upgrades supersede cleanly

Add regression test proving Req 4.4/7.2: a second WAL_CONFIG_MARKER
committing while the first's forced checkpoint is still in flight
(simulated via the checkpoint_in_progress CAS across both markers'
commit+apply) cleanly supersedes it, with the settled state reflecting
only the newer marker. Verified via a field-aware byte comparison
(post-checkpoint_lsn) against a non-racing control fixture, plus
mid-race assertions that materialization is still incomplete at that
point (cfgref_present false, fence set) despite activation already
tracking the newer config. Test-only; production mechanism (already
implemented) unmodified. Suite: 1027 -> 1028 tests, 0 failures.
This commit is contained in:
Rongsong Shen 2026-07-25 19:48:19 +00:00
parent d4af605fa6
commit b78efd03b4
2 changed files with 378 additions and 1 deletions

View file

@ -136,7 +136,7 @@
- _Requirements: 5.2, 5.4, 5.5, 6.3_
- _Depends: 7, 8.1, 8.2, 8.3, 8.4_
- [ ] 8.8 Exercise a second configuration upgrade committing before the first's installation completes everywhere
- [x] 8.8 Exercise a second configuration upgrade committing before the first's installation completes everywhere
- Commit a second configuration-change marker while the first upgrade's snapshot is still installing on at least one member, and assert the earlier upgrade's in-flight state is cleanly superseded rather than left partially applied or conflated with the newer one
- Observable: after both markers settle, every member reflects only the newer configuration and state, with no trace of a partially-applied intermediate state
- _Requirements: 4.4, 7.2_
@ -150,6 +150,8 @@
## Implementation Notes
- **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).
- **Task 8.6 progress (infrastructure + two real production bugs found and fixed; task NOT YET complete — one more bug blocks a clean live-soak PASS):** building this task's live 3-node chaotic-upgrade soak required substantial new infrastructure that never existed anywhere in this codebase: a client-side send path for `OP_upgradeConfig` (`xce_upgradeconfig_req_raw`/`xce_upgradeconfig`/`xcecli_upgradeconfig` — the auto-generated capnp encoder for this request type is a literal no-op for its `Data` fields, worked around via a hand-rolled bypass mirroring the existing `xce_replconfigpush_req_raw` idiom), a `cmd_hostctrl`/`cmd_upgradeconfig` CLI pair in `test.c`, and a new `--config-upgrade` mode in `run-ha-soak.sh` (+ `SKILL.md` docs). Building and running this surfaced two real, independently-confirmed production defects: