test(config-upgrade-snapshot): fault-inject crash/restart + leadership change (task 8.7)

Prove Requirements 5.2/5.4/5.5/6.3 via deterministic fault injection at
the two critical upgrade windows, entirely at the unit level using the
XCE_TESTING hooks earlier tasks built for exactly this purpose -- no
live soak needed.

test_marker_checkpoint_fault_hook_crash_then_restart_resumes_materialization
extends the existing fault-hook seam test: after simulating a crash
before the checkpoint write durably completes, it boots a genuinely
fresh persist_t/cluster_t against the same on-disk WAL via
persist_recover() (a real boot-replay simulation, not a same-process
re-call) and confirms materialization resumes and completes correctly.

test_leadership_change_mid_materialization_completes_via_same_generic_apply_drain
proves a promoted member's materialization completes identically to a
non-promoted control member's, by draining each through its actual
production entry point and comparing checkpoints byte-for-byte -- a
regression guard against any future role-conditional branch creeping
into the marker-apply path.
This commit is contained in:
Rongsong Shen 2026-07-25 18:20:43 +00:00
parent b5fde32033
commit d4af605fa6
3 changed files with 463 additions and 1 deletions

View file

@ -129,7 +129,7 @@
- _Depends: 7, 8.1, 8.2, 8.3, 8.4_
- _Blocked: bug-1472 — pre-existing, out-of-spec-boundary HA worker-control replication defect (`worker_from_capnp`, `worker.c:827-856`, introduced 2026-05-08, commit `9b6a91a8`, ~2.5 months before this spec). `WAL_WORKER_CTRL` apply treats "-EEXIST" (target worker already exists) as a benign no-op for ALL hook details — correct for JOIN replay, but wrong for UPDATE/DEAD, where the target already existing is the normal case. Consequence: a host admin-status change (`hostctrl -c`/`-o`) never propagates past the member that directly processed the RPC — completely general, unrelated to config upgrades or checkpoints. Live soak infra + task 8.6's own bug-1468/bug-1469 fixes are committed (959b0cd) and independently reviewed; only the Requirement 6.4 host-admin-status-survival assertion is blocked, on a bug this spec does not own. See `.wolf/buglog.json` bug-1471 (byte-precise diagnosis) and bug-1472 (root cause) for full forensic detail. Unblocks when bug-1472 is fixed by whoever owns worker/HA replication — no further config-upgrade-snapshot work needed once that lands (re-run `run-ha-soak.sh --config-upgrade` to confirm clean PASS)._
- [ ] 8.7 Inject a crash and a leadership change at the two critical upgrade windows
- [x] 8.7 Inject a crash and a leadership change at the two critical upgrade windows
- Force a controller crash/restart between a configuration-change marker's commit and that member's own snapshot write becoming durable, and assert materialization resumes correctly from the marker on restart
- Force a leadership change between a configuration-change marker's commit and every member completing its snapshot install, and assert the new leader continues driving materialization to completion without any special resume logic
- Observable: both fault-injection scenarios complete with the post-upgrade consistency invariant and the crash-safety invariant intact
@ -150,6 +150,8 @@
## Implementation Notes
- **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:
- **bug-1468** (`cluster.c`): `myreq_handler`'s `OPCode_upgradeConfig` dispatch guard was inverted by an unrelated commit (`fd93997f`, 2026-05-31) — it rejected the only role (primary) that could ever legitimately reach it, making config upgrades unreachable from any network client at all. Fixed: restored correct polarity.
- **bug-1469** (`persist.c`): `apply_committed_entry`'s `WAL_CONFIG_MARKER` case never called `config_activate_committed()` — so the node that *originates* a config upgrade never refreshed its own in-memory config/queue state as part of committing it (only receiving standbys did, via a separate out-of-band push path). If that node was killed before some later, independent event happened to refresh it, its checkpoint froze with stale config content — a genuine, **live-reproduced** instance of bug-857's originally-hypothesized failure mode (task 8.5 could only prove a different, unit-level mechanism; this is the first real empirical case). Fixed: added `config_activate_committed(c)` inside the marker-apply case, positioned *before* the marker-triggered checkpoint write (checkpoint content is built from live runtime tables that only reflect the reload after activation runs, not from `myconf` directly) and guarded against `c->myconf == NULL` (a real crash risk hit in an existing test fixture). Fixing this also exposed and required fixing a genuine race in a pre-existing test (`test_commit_advance_retry_clears_push_before_commit_fence`) that the new activation side-effect made observable for the first time — narrowed the race window, did not weaken any assertion. Reviewer independently re-derived the lock-safety claim from scratch (traced every mutex site in `config_repl.c`, confirmed none reachable from `config_activate_committed`) and the thread-class claim (`apply_committed_entry` runs on a `VTASK_LIVE_BLOCK` pool worker, confirmed via `persist_apply_pump_kick`) rather than trusting the implementer's account — both held.