feat(config-upgrade-snapshot): live config-upgrade soak infra + fix bug-1468/bug-1469 (task 8.6)

Build the live 3-node chaotic-multi-kill config-upgrade soak scenario
(run-ha-soak.sh --config-upgrade) required by Requirements 4.1, 4.3, 6.1,
6.4. This needed genuinely new client-side infrastructure: a hand-rolled
capnp send path for OP_upgradeConfig (the auto-generated encoder is a
no-op for its Data fields, worked around the same way
xce_replconfigpush_req_raw already does), plus cmd_hostctrl/cmd_upgradeconfig
CLI commands.

Building and running this live surfaced two real production bugs, both
fixed here:

- bug-1468: an unrelated prior commit inverted OPCode_upgradeConfig's
  primary/standby dispatch guard in cluster.c, making config upgrades
  unreachable from any network client.
- bug-1469: 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 state as part of committing
  it. Killing that node before some later, unrelated event happened to
  refresh it left its checkpoint permanently stale -- the first live,
  empirical reproduction of bug-857's originally hypothesized failure
  mode. Fixed by activating before the marker-triggered checkpoint write,
  since checkpoint content is built from live runtime tables that only
  reflect the reload after activation runs.

A separate, narrower checkpoint divergence (bug-1471, not config-related)
remains open and still blocks a clean soak PASS -- task 8.6 stays
in-progress until it's resolved.
This commit is contained in:
Rongsong Shen 2026-07-25 15:48:20 +00:00
parent 2bd44e8e4d
commit 959b0cdacb
13 changed files with 1022 additions and 10 deletions

View file

@ -149,6 +149,12 @@
## Implementation Notes
- **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.
- **Still open, blocking a clean PASS — bug-1471**: a separate, much narrower 1-byte checkpoint divergence in a host-admin-status frame (not a config-activation issue), found independently by two concurrent sessions working this task and root-cause-not-yet-isolated. Also noted but deliberately **not** triggered by the current soak scenario: a deeper, genuinely separate gap where no live mechanism exists anywhere in this codebase to add a *new* queue to an already-running member's runtime `qmgr` without a restart (`config_activate_committed`/`load_primary_config` refresh `myconf->cluster_config` but nothing syncs that into the live `qmgr`) — the soak's upgrade-target config change was deliberately scoped to modifying an *existing* queue's field rather than adding one, specifically to test what Requirements 4.1-4.3/6.1/6.4 actually ask for without tripping over this separate, deeper architectural gap. Both are flagged in `.wolf/buglog.json` for whoever next picks up bug-1471 (or a dedicated follow-up for the queue-creation gap) — task 8.6's own checkbox stays unchecked until the soak reports a clean PASS.
- **Process note**: this task was interrupted by the same monthly-spend-limit condition hit earlier in this session (task 6.1), across three separate dispatch/resume cycles. Each resumption correctly checked live `git status`/`diff` before acting rather than blindly redoing work, and the final state was independently re-verified end-to-end (build + full 1025-test suite + file-integrity spot check) before being reviewed and committed — no corruption or duplicated work resulted, but it's worth flagging that this class of interruption can leave multiple agent instances transiently active in the same worktree.
- **Task 8.5 redirection (design.md's original mechanism was wrong; this is not the same as a review rejection):** the original hypothesis for bug-857's divergence — a job whose queue is removed by a config upgrade gets re-homed to `lost_and_found` on some members but not others, based on per-node restart timing — turned out to be **aspirational, never-implemented code**. Exhaustive grep across `job.c`/`queue.c`/`worker.c`/`config_repl.c` at both the pre-fix baseline commit (`e0c5e8b7`) and current HEAD confirmed: `create_lost_and_found` only ever creates the sentinel queue/host objects (and its entire body is `#ifndef XCE_TESTING`-gated); `restore_job`/`restore_job_array` never consult `qmgr`; `restore_queue_state` explicitly *skips* rather than re-homes when a queue is missing. `.wolf/buglog.json`'s bug-857 entry itself flags this: "identified via TLA+ gap analysis... NOT empirically reproduced yet." design.md's Requirements Traceability table assigned 6.2 to a live "HA soak new scenario" — also written against the same since-disproven hypothesis. Redirected (orchestrator decision, not a task-local implementer call) to a real, code-backed mechanism instead: `schedule_checkpoint`'s pre-existing, unmodified-by-this-spec WAL-record-count threshold trigger is genuinely decoupled from the config-marker `(term,lsn)` boundary, so two members' independently-timed checkpoints can capture different job/queue sets — the actual, general-purpose truth behind bug-857's claim ("no snapshot transfers an identical materialized state"). This is testable entirely within `controller_unit_test`, no live multi-process soak or old-commit worktree needed, since the threshold-trigger mechanism is still live, unmodified production code coexisting with the new marker-trigger at HEAD. Landed as `test_bug857_regression.c` (2 tests: divergence under threshold-only triggering vs. convergence under the real marker-triggered path, the latter under deliberately differing per-member processing pace to make the convergence claim non-trivial). Reviewer independently re-derived and confirmed the mechanism claim from first principles (not just re-running the tests) before approving — see `git blame`/review transcript for the full verification chain (schedule_checkpoint predates this spec via `git merge-base --is-ancestor`, zero lines of its body touched by tasks 1-7, `persist_apply_pump_run`'s strictly-sequential dispatch loop explains why the marker-triggered checkpoint's convergence isn't a timing coincidence). design.md's Requirements Traceability table (row 6.2) and Testing Strategy section were both corrected to match. **Heads-up for tasks 8.6-8.9**: they still legitimately need live multi-process HA-soak-level testing for Requirement 6.1 (chaotic multi-kill + config upgrade) — that infrastructure (a live config-upgrade CLI trigger doesn't exist anywhere in this codebase yet; confirmed via grep of `test.c`/`src/frontend`/`src/tools`) was NOT built by this task and will need to be built by whichever of 8.6-8.9 first needs to drive a live `OP_upgradeConfig` RPC against a running cluster.
- **Task 8.4 round-1 rejection (real gap, caught by review):** the first pass built `configcompare_paths()` (correct, well-tested against `persist.c`/`config_repl.c`'s real `workdir/name/conf/current`+`conf/cluster.bin.<hex>` resolution convention) and linked `configcompare.c` into both `xce_test_src` and `controller_unit_test_src` in meson, but never registered it as a `cmd_configcompare` subcommand in `testsuite/test.c` — unlike its siblings `walcompare`/`snapverify`/`snapdiff`, each of which has a `cmd_*` wrapper in BOTH `top_commands[]` and `client_commands[]`. Left as delivered, the tool was a tested library function with no shell entry point — unusable by task 8.6/8.9's later HA soak scenarios, which design.md explicitly says will invoke `configcompare` directly against each node's data directory. Fixed with a 25-line, `test.c`-only remediation mirroring `cmd_snapdiff`'s exact shape; verified end-to-end via `echo 'configcompare <dir1> <dir2>' | xce_test` (checked `$?` for both IDENTICAL/exit-0 and DIVERGENT/exit-1 cases, not just printed text). **Heads-up for any future task in this spec (or elsewhere) that adds a new offline testsuite tool in this family**: linking the `.c` file into the meson source lists is NOT sufficient — it must also get a `cmd_*` wrapper + `top_commands[]`/`client_commands[]` entries in `test.c`, or it silently has no way to be invoked outside the unit test binary.