feat(config-upgrade-snapshot): materialize upgrade snapshot on config-marker apply

Adds an explicit case WAL_CONFIG_MARKER to apply_committed_entry,
replacing its no-op treatment, that forces a checkpoint at exactly the
marker's own (term, lsn) via two new helpers:

  persist_checkpoint_write_locked(p, checkpoint_lsn) -- the checkpoint
  core (pin lsn, scan config ref, write_snapshot, trim, rotate) taking
  an explicit LSN and never acquiring apply_pump_lock itself
  persist_checkpoint_marker_triggered(p, lsn) -- shares the existing
  checkpoint_in_progress CAS guard, calls the above with the marker's
  own lsn

This fixes two hazards design.md's "call persist_checkpoint() directly"
text didn't account for: (1) apply_committed_entry's live-pump caller
(persist_apply_pump_run, used by both primary and standby online-apply)
already holds apply_pump_lock across the dispatch call -- a direct
persist_checkpoint() call would self-deadlock on that mutex; (2)
last_applied_lsn/replay position only advance AFTER apply_committed_entry
returns at both call sites, so pinning checkpoint_lsn from
last_applied_lsn inside the marker case would label the checkpoint with
the previous entry's LSN, not the marker's own -- fixed by threading the
marker's own lsn parameter through explicitly. A -EBUSY outcome (a
concurrent periodic checkpoint mid-flight) is logged at TRACE and treated
as expected/self-resolving, never retried. The new case has no effect on
entity interpretation/re-homing (Req 1.8).

New test_upgrade_snapshot_apply.c drives WAL_CONFIG_MARKER through the
REAL production call paths (persist_apply_pump_run and boot replay via
persist_recover), not a bare apply_committed_entry() call, so the fix is
actually proven against the lock-holding context rather than assumed.

Also fixes a genuine regression in test_config_repl.c surfaced by
WAL_CONFIG_MARKER apply having a real side effect for the first time
(bug-1466): an fsync-worker-thread race (t42_wait_applied polling helper,
mirrors test_apply_pump.c's tap_wait_applied) and one test call site
trusting an unreliable raft-derived LSN. Surfaces (but does not fix, out
of this spec's boundary) a pre-existing ha-term-safety/raft-log-replication
gap: persist_write_checkpoint_marker bypasses ha_primary_log_append,
breaking its documented current_lsn==log_len+1 invariant once a
checkpoint interleaves with primary appends -- log_len feeds real
RequestVote freshness comparisons, not advisory-only as first assumed;
severity corrected in bug-1466. Task 5 of 19.

Requirements: 1.1, 1.5, 1.8, 4.4, 5.2, 5.3
This commit is contained in:
Rongsong Shen 2026-07-24 16:28:57 +00:00
parent 00cd26a927
commit 1da26b043e
7 changed files with 938 additions and 23 deletions

View file

@ -42,7 +42,7 @@
- _Requirements: 1.10_
- _Depends: 2.1, 2.2, 3_
- [ ] 5. Materialize the upgrade snapshot when a configuration-change marker is applied
- [x] 5. Materialize the upgrade snapshot when a configuration-change marker is applied
- Add explicit handling for the configuration-change marker in the single, unified entity-apply dispatch point (the same seam every member's apply already routes through — primary, standby, and restart replay alike), replacing its current no-op treatment
- Trigger a forced checkpoint at that exact point, so materialization happens immediately and deterministically at the marker's boundary rather than waiting for a periodic trigger
- Ensure the forced checkpoint shares the existing single-in-flight checkpoint guard, so a concurrent periodic checkpoint and this one can never both proceed at once
@ -149,6 +149,9 @@
## Implementation Notes
- **Task 5 correction to design.md:** design.md's "Marker Apply Trigger" section says "Call `persist_checkpoint()` directly" — this is unsafe as written. `persist_apply_pump_run` holds `apply_pump_lock` across the whole dispatch loop (including the `apply_committed_entry` call), and `persist_checkpoint()` itself re-acquires that same non-recursive mutex — a real self-deadlock on the live pump path (primary AND standby online-apply). Separately, `last_applied_lsn`/replay position only advance AFTER `apply_committed_entry` returns at both call sites, so pinning `checkpoint_lsn` from `last_applied_lsn` inside the marker case would mislabel the checkpoint with the previous entry's LSN, not the marker's own. Fixed via `persist_checkpoint_write_locked(p, checkpoint_lsn)` (no self-lock, explicit LSN param) + `persist_checkpoint_marker_triggered(p, lsn)`, called from `apply_committed_entry`'s new `WAL_CONFIG_MARKER` case with the marker's own `lsn` parameter. design.md should be updated to reflect this; not done as part of task 5 (docs-only, deferred).
- **Task 5 → ha-term-safety/raft-log-replication finding (bug-1466, NOT this spec's to fix):** `persist_write_checkpoint_marker` advances `p->current_lsn` directly, bypassing `ha_primary_log_append`/`raft_log_append`, breaking `ha_primary_log_append`'s own documented `p->current_lsn == s->log_len + 1` invariant (ha.c:4054-4060) once any checkpoint interleaves with primary appends. Pre-existing (periodic checkpoint already did this); task 5 just made the interleaving common instead of rare, surfacing it as real test flakiness for the first time. `ha->raft.log_len` feeds real RequestVote freshness comparisons (ha.c:258-261, 401-406) — NOT advisory-only as bug-1466's original text claimed; severity corrected in `.wolf/buglog.json`. Out of this spec's boundary (Raft log/term safety is `ha-term-safety`/raft-log-replication's territory) — flagging for whoever next owns that area.
- **Task 3 → task 4 heads-up:** `restore_host_state(cluster_t*, const char*, uint32_t)` (worker.c) is standalone and NOT yet wired into `load_snapshot`. Task 4 dispatches decoded `HostAdminState` frames to it. `restore_host_state` runs on the live vtask pool (called from `ha_handle_snapshot_chunk` → `persist_load_snapshot`), not a quiesced phase — a concurrent real first-time join for the same host is architecturally possible; verified safe by convergent key_find/makeWorker semantics (see worker.c comment above `restore_host_state`'s key_find call), not by absence of concurrency. Task 4 should confirm this contract still holds once wired in.
- **Task 2.3 → task 6.1 heads-up:** `read_active_config_bytes` (persist.c) is the fully-inlined (no config_repl.c call) pattern for reading live config bytes off disk fresh each time — task 6.1's activation-fence verify function needs the identical "always re-read live bytes, never trust a cached/embedded copy" discipline (design.md is explicit about this). Reuse/mirror `read_active_config_bytes` rather than re-deriving the config_repl.c-avoidance reasoning from scratch.