From 2bd44e8e4df7e16d314ab79892b224e4de347f9a Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Sat, 25 Jul 2026 10:44:14 +0000 Subject: [PATCH] feat(config-upgrade-snapshot): add bug-857 regression test, correct design.md (task 8.5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prove Requirement 6.2 with a real, code-backed mechanism instead of the originally hypothesized lost_and_found re-homing, which turned out to be aspirational and never implemented (confirmed by exhaustive grep at both the pre-fix baseline commit and HEAD — bug-857 itself was flagged in buglog.json as never empirically reproduced). test_bug857_regression.c demonstrates both halves of the requirement entirely within controller_unit_test, no live soak or old-commit build needed: schedule_checkpoint's pre-existing WAL-threshold trigger is decoupled from the config-marker boundary and produces genuinely divergent per-member checkpoint content, while the marker-triggered checkpoint this spec added converges members regardless of differing per-member processing pace. design.md's Requirements Traceability table and Testing Strategy section are corrected to match; the redirection reasoning is recorded in tasks.md's Implementation Notes for the benefit of tasks 8.6-8.9, which still need live multi-process HA-soak infrastructure for Requirement 6.1 that this task deliberately did not build. --- .kiro/specs/config-upgrade-snapshot/design.md | 432 +++++++++++++++ .kiro/specs/config-upgrade-snapshot/tasks.md | 4 +- src/controller/meson.build | 1 + src/controller/testsuite/controller_test.c | 2 + .../testsuite/test_bug857_regression.c | 513 ++++++++++++++++++ 5 files changed, 951 insertions(+), 1 deletion(-) create mode 100644 .kiro/specs/config-upgrade-snapshot/design.md create mode 100644 src/controller/testsuite/test_bug857_regression.c diff --git a/.kiro/specs/config-upgrade-snapshot/design.md b/.kiro/specs/config-upgrade-snapshot/design.md new file mode 100644 index 0000000..e1d722d --- /dev/null +++ b/.kiro/specs/config-upgrade-snapshot/design.md @@ -0,0 +1,432 @@ +# Design Document: config-upgrade-snapshot + +## Overview + +**Purpose**: This feature makes a configuration upgrade on an already-running XCE cluster materialize one identical, entity-bearing state snapshot at the configuration-change marker's `(term, lsn)` boundary, embed the new configuration in it, and install that same snapshot on every member — closing bug-857 (post-upgrade materialized-state divergence) by construction, rather than by per-node WAL replay under the new configuration. + +**Users**: Cluster operators pushing a configuration change (via `xceadmin`/the upgrade-config admin path) and, transitively, every job/queue/host in flight at the moment of the change — this feature is what keeps their materialized state identical across all controller members through the transition. + +**Impact**: Retires the current "commit marker → restart every controller → each member replays its own pre-upgrade WAL under the new configuration" behavior (`do_upgrade_config_with_bytes`, `config_repl.c:1591-1773`, step 9) in favor of a synchronous, in-process materialization trigger on marker apply. No controller restart is required for a configuration upgrade after this change. + +### Goals +- Materialize a full entity-bearing snapshot (jobs, queues, hosts, embedded configuration) at the exact `(term, lsn)` of the committed configuration-change marker, on every member, without any member re-deriving state by WAL replay. +- Gate a serving primary's acceptance of state-changing client requests on its OWN materialization-and-hash-verification for the current marker having completed — independent of Raft's promotion eligibility. +- Survive crash, restart, and leadership change at any point in the upgrade sequence, with deterministic, idempotent resumption. +- Prove the fix with a regression that reproduces bug-857 on the old code path and passes on the new one, including chaotic multi-kill failover interleaved with the upgrade. + +### Non-Goals +- Initial/bootstrap configuration replication (config tag zero → first config) — owned by `ha-config-bootstrap`; not unified with this mechanism (requirements.md Boundary Context, decided and rejected 2026-07-23). +- The configuration-change marker's consensus ordering itself, or the Raft protocol — owned by `ha-term-safety` / the Raft TLA+ Conformance library series. +- Controller voting-set/membership changes — `ha-config-bootstrap`. +- Fixing the pre-existing, out-of-scope `durable_lsn`-not-advanced-by-snapshot-install gap (research.md, this session) — real but bounded to self-promotion-eligibility bookkeeping, predates this spec, applies to any snapshot install (not upgrade-specific). + +## Boundary Commitments + +### This Spec Owns +- The `WAL_CONFIG_MARKER` apply-time side effect: triggering a forced, synchronous checkpoint at marker-apply time (new `apply_committed_entry` case). +- Embedding full configuration bytes (not just a `{tag, sha}` reference) in the upgrade snapshot, and in every subsequent checkpoint until rotated away. +- The host admin-state snapshot frame (new) and the queue admin-state frame's migration to Cap'n Proto (existing raw layout retired) — one `SNAPSHOT_FORMAT_VERSION` bump covers both. +- The activation fence: a new, independent condition gating primary request-serving and promotion on this member's own materialization-and-hash-verification for the current marker. +- Retiring the per-node restart/replay upgrade path (`do_upgrade_config_with_bytes` step 9). +- Regression coverage: bug-857 reproduction, crash/leadership-change injection at the marker→snapshot-durable and marker→all-installed windows, host-placeholder/rejoin, overlapping upgrades, and format-mismatch fail-loud behavior. + +### Out of Boundary +- Entity-serialization machinery itself (job/queue capnp encode, checkpoint file layout, chunked transport) — owned by `ha-entity-snapshots`, already shipped; this spec extends it in the two narrow, explicit ways above, not otherwise. +- The configuration-change marker's append/commit and `(term, lsn)` ordering — owned by `ha-term-safety` Requirement 7; this spec only reacts to the marker once committed. +- Raft election, log replication, term safety — the Raft TLA+ Conformance library series. +- Fixing `durable_lsn` non-advancement on snapshot-install (see Non-Goals). +- Initial/bootstrap config replication — `ha-config-bootstrap`. + +### Allowed Dependencies +- `ha-entity-snapshots`'s `write_snapshot`/`load_snapshot`/`persist_checkpoint` machinery (`persist.c`) — extended, not replaced. +- `ha-term-safety`'s committed configuration-change marker (`config_committed_marker`, `WAL_CONFIG_MARKER`) — consumed as-is. +- The existing `snapshot_transfer_s` chunked install transport (`ha_handle_snapshot_chunk`) — reused as-is; this spec does not modify chunk transfer mechanics. +- The existing `config_serve_fence` pattern (`ha.h:307-322`, `config_repl.c:304-377`) — mirrored structurally for the new activation fence, not merged into the same field. +- The existing `lost_and_found` placeholder-worker precedent (`makeWorker()` without join, `config_repl.c:1114-1130`) — mirrored for host-frame restore. + +### Revalidation Triggers +- Any change to `apply_committed_entry`'s dispatch shape or to the fact that it is the single unified apply seam for all members (currently confirmed — `apply_wal_entry` has zero live callers) would require re-verifying the marker-apply hook still fires for every member. +- Any change to the on-disk snapshot header/frame layout (`SNAPSHOT_FORMAT_VERSION`) beyond this spec's own bump must re-check `snapverify`/`snapdiff`/this spec's config-bytes section for continued validity. +- Any change to `config_serve_fence`'s semantics or to `ha_dispatch_gate`'s composition order must re-check the new activation fence's placement alongside it. +- If `entity-bearing-snapshots`/`ha-entity-snapshots` ever changes its frame format, this spec's host/queue frame additions must be re-verified against the new format. + +## Architecture + +### Existing Architecture Analysis + +Three existing subsystems already do almost everything this feature needs; the actual new-work surface is narrow: + +1. **The apply seam is already unified.** `apply_committed_entry` (`persist.c:2751-3132`) is the single dispatch point for entity mutation on every member — primary online-apply (vtask-pool dispatched via `persist_apply_pump_kick`), standby normal apply (`ha_standby_advance_committed_and_apply`, heartbeat-learned commit position, since the 2026-07-10 commit-then-apply change), and boot/restart replay. `WAL_CONFIG_MARKER` currently falls into its generic `default:` no-op. The sibling dispatcher `apply_wal_entry` has zero live callers — fully superseded, not a second seam to worry about. +2. **The checkpoint/snapshot pipeline is already entity-bearing.** `write_snapshot`/`load_snapshot` (`persist.c`) already serialize job and queue frames at every checkpoint, guarded by a single-in-flight CAS (`checkpoint_in_progress`) and already stamping a config *reference* (`cfgref_tag`/`cfgref_sha`) via `scan_latest_committed_config_ref`. `persist_checkpoint()` is a plain, already-force-callable, synchronous function (a second direct call site already exists at `ha.c:3308`). +3. **The activation-fence pattern already exists for a materially identical problem.** `config_serve_fence` already gates client-request-serving (via `ha_dispatch_gate`) and promotion on a hash-verify condition, with a fully-wired non-blocking retry loop — this is a direct structural precedent for the gate 3.2/3.3 need, not a new pattern to invent. + +The upgrade path being retired (`do_upgrade_config_with_bytes` step 9, `config_repl.c:1753-1772`) already carries a NOTICE comment (`config_repl.c:1737-1751`) naming this exact spec and its target end-state — the maintainers who wrote the interim restart/replay path already anticipated this design. + +### Architecture Pattern & Boundary Map + +```mermaid +graph TB + Marker[WAL_CONFIG_MARKER committed] + Apply[apply_committed_entry] + Trigger[Forced checkpoint trigger] + Checkpoint[persist_checkpoint] + Snapshot[write_snapshot entity plus config bytes] + Fence[Upgrade materialization fence] + Gate[ha_dispatch_gate] + Promo[Promotion gate] + Transport[snapshot_transfer_s install] + Behind[Behind or joining member] + + Marker --> Apply + Apply --> Trigger + Trigger --> Checkpoint + Checkpoint --> Snapshot + Snapshot --> Fence + Fence --> Gate + Fence --> Promo + Behind --> Transport + Transport --> Fence +``` + +**Architecture Integration**: +- Selected pattern: extend the existing per-member apply-and-checkpoint pipeline in place; no new distribution/orchestration component. Each member independently materializes on its own apply of the marker (primary and standby alike, since both route through `apply_committed_entry`); a member that is behind receives the same entity-bearing snapshot through the existing generic catch-up transport, unrelated to upgrade-specific push logic. +- Domain/feature boundaries: persistence (`persist.c`) owns materialization and on-disk format; HA (`ha.c`/`ha.h`) owns the activation fence and dispatch-gate integration; config replication (`config_repl.c`) owns config-bytes sourcing and retiring the old restart path; worker/queue restore owns entity-specific create-vs-update semantics. +- Existing patterns preserved: single-in-flight checkpoint CAS, fail-loud format-version mismatch, never-self-terminate fence-and-retry, `#ifdef XCE_TESTING` fault-hook shape. +- New components rationale: a new `apply_committed_entry` case (nothing existed for this entry type); a new fence field (the existing one is logically a different condition); new capnp structs (existing ones bundle unwanted dynamic/static fields); a create-if-missing host restore path (queues' update-only assumption doesn't hold for hosts). +- Steering compliance: never blocks a vtask handler (the new checkpoint trigger runs inside `apply_committed_entry`'s own vtask-pool context, not on iomgr/delaymgr); never self-terminates (fence-and-retry, mirroring `config_serve_fence`). + +**Key simplification found during design**: 5.4 ("new leader resumes driving materialization and distribution") needs no new distribution-tracking state machine. Because every member independently materializes on its own apply of the marker (there is no primary-driven push loop — install is receiver-pulled via the existing generic catch-up mechanism), a new leader satisfies 5.4 automatically by continuing to apply its own committed log (which already includes the marker) — the same `apply_committed_entry` hook that handles this for any member handles it for a newly-elected one, with no leader-specific "resume" logic to write. + +### Technology Stack + +| Layer | Choice / Version | Role in Feature | Notes | +|-------|------------------|------------------|-------| +| Persistence / Serialization | Cap'n Proto (existing, `src/schema/controller.capnp`) | Encodes `HostAdminState`/`QueueAdminState` frames and embedded config bytes in the snapshot | Same library already used for `Job`/`Worker` messages; no version change | +| Backend / Runtime | C (gnu17), existing controller daemon | All new logic lives in `src/controller/` | No new language/runtime | +| Async I/O | Existing vtask pool / delaymgr (`ha-async-io.md`) | New checkpoint trigger runs on the existing apply-pump vtask worker; new fence retry mirrors `config_schedule_activate_retry`'s delaymgr→vtask trampoline | No new concurrency primitive | + +No new external dependency is introduced anywhere in this spec. + +## File Structure Plan + +### Modified Files +- `src/controller/persist.c` — new `case WAL_CONFIG_MARKER:` in `apply_committed_entry` (calls `persist_checkpoint` directly); `write_snapshot`/`load_snapshot`/`load_snapshot_entities` extended with a host-frame section and a config-bytes section; `collect_queue_frames`/queue-frame decode migrated from raw layout to `QueueAdminState` capnp; new `collect_host_frames`/`restore_host_state`; `SNAPSHOT_FORMAT_VERSION` bumped 1→2; new `#ifdef XCE_TESTING` fault hook for the marker-apply→checkpoint-durable window. +- `src/include/persist.h` — `SNAPSHOT_FORMAT_VERSION` bump declaration site (if defined here rather than persist.c); no new persistent fields needed beyond what already exists (`cfgref_*` reused for the reference; the embedded bytes are read fresh from disk at write time via `myconf`'s own fields, not cached on `persist_t` — see the config_repl.c bullet below for why this avoids a new cross-module call). +- `src/controller/ha.h` — new `_Atomic int upgrade_materialization_fence` field on `ha_state_t` (sibling to `config_serve_fence`, `ha.h:307-322`); new accessor declaration. +- `src/controller/ha.c` — new `upgrade_materialization_verify()` (mirrors `config_activate_committed`'s hash-verify-then-set/clear shape); new accessor `upgrade_materialization_fenced()`; `ha_dispatch_gate`'s `CTRL_PRIMARY` branch extended with the new fence check (composed alongside, not merged with, `config_serve_fenced()`); promotion gate (`ha.c:1425` area) extended the same way `config_serve_fenced()` already gates it; new `schedule_upgrade_materialization_retry()` mirroring `config_schedule_activate_retry`'s trampoline. +- `src/controller/config_repl.c` — `do_upgrade_config_with_bytes` step 9 (`config_repl.c:1753-1772`, restart_requested/cbuf_put) removed; the NOTICE comment (`config_repl.c:1737-1751`) replaced with a short comment pointing at the new `apply_committed_entry` case; **no new accessor function added here** — `persist.c`'s snapshot writer reads the active config bytes directly off disk via `p->cluster->myconf->workdir`/`myconf->name` (plain struct fields on the shared, public `cluster_t`/`myconf_t`, `cluster_def.h`/`myconf.h`), the same `workdir/name/conf` path `conf_save_config` (`config_repl.c:776`) already writes to — this deliberately mirrors `scan_latest_committed_config_ref`'s own existing precedent of avoiding a `persist.c → config_repl.c` function-call dependency (that function's comment states this explicitly), rather than introducing the layering violation a new accessor would create; new `#ifdef XCE_TESTING` fault hook for the leadership-change-mid-distribution window, mirroring `xce_snapshot_chunk_hook`'s shape. +- `src/controller/worker.c` — new `restore_host_state(cluster_t*, const char*, uint32_t admin_status)`: create-if-missing via a small `makeWorker()`-based placeholder helper (valid non-NULL `hostattrs_t`, mirroring the `lost_and_found` precedent in `config_repl.c:1114-1130`), idempotent (look up first, only create if missing, OR-in admin bits either way — 5.8). +- `src/controller/queue.c` — no signature change to `restore_queue_state`; only its caller's decode step (in `persist.c`) changes from raw-layout parsing to `QueueAdminState` capnp decode. +- `src/schema/controller.capnp` — new `struct HostAdminState { name @0: Text; status @1: UInt32; }` and `struct QueueAdminState { name @0: Text; status @1: UInt32; }`. +- `.claude/skills/xce-test-ha/run-ha-soak.sh` — extended with a new scenario covering config upgrade interleaved with chaotic multi-kill failover (requirement 6) and back-to-back overlapping upgrades (7.2); exact flag/mode naming left to tasks.md. + +### New Files +- `src/controller/testsuite/test_upgrade_snapshot_apply.c` — bare in-memory `cluster_t` fixture (mirrors `test_apply_committed_entry.c`'s pattern), drives the new `WAL_CONFIG_MARKER` case directly, asserts a forced checkpoint fires exactly once and is idempotent under retry. +- `src/controller/testsuite/test_upgrade_snapshot_restore.c` — disk-backed fixture (mirrors `test_replay_apply_seam.c`'s pattern), covers host-placeholder creation, later real-join-through-`rejoin_worker` admin-bit preservation, and idempotent retry of a partially-failed restore. +- `src/controller/testsuite/test_upgrade_activation_fence.c` — exercises `upgrade_materialization_verify`, the dispatch-gate composition, the promotion-gate composition, and the retry trampoline. +- `src/controller/testsuite/configcompare.c` / `configcompare.h` — new sibling tool to `walcompare` (not an extension of it — `walcompare`'s internal model is WAL-entry-indexed `(lsn → term, pcrc)`; active-configuration identity across members is a single `(tag, sha)` per member with no LSN dimension, an awkward fit for `walcompare`'s report shape). **Offline/file-based, mirroring `snapdiff_paths`'s exact signature shape**: `int configcompare_paths(const char* const* conf_paths, int n, FILE* out, configcompare_report_t* rep)`, each path being a member's `workdir/name/conf` file (the same path this spec's snapshot-embed step reads, so both consumers agree on where a member's active config lives) — no live RPC to each cluster member, consistent with every other tool in this family. Compares each member's config content hash (recomputed from the file bytes, not trusted from a cached tag) for 4.2, usable directly by the HA soak scenario against each node's own data directory. +- `src/controller/testsuite/snapverify.c` (modified, not new) — extended to decode and assert `cfgref_tag`/`cfgref_sha` (not just presence), and to report host-frame count and config-bytes-section presence. + +## System Flows + +```mermaid +sequenceDiagram + participant Admin + participant Primary + participant Standby + participant ApplySeam as apply_committed_entry + participant Checkpoint as persist_checkpoint + participant Fence as Activation Fence + + Admin->>Primary: upgrade-config request + Primary->>Primary: config_commit_change (append+commit marker) + Primary->>Standby: replicate WAL_CONFIG_MARKER + par Primary applies + Primary->>ApplySeam: apply marker (own apply pump) + ApplySeam->>Checkpoint: trigger forced checkpoint at marker (term,lsn) + Checkpoint->>Fence: verify hash, clear fence + and Standby applies + Standby->>ApplySeam: apply marker (heartbeat-learned commit) + ApplySeam->>Checkpoint: trigger forced checkpoint at marker (term,lsn) + Checkpoint->>Fence: verify hash, clear fence + end + Fence-->>Primary: serve under new configuration +``` + +**Key decisions visible above**: no primary-orchestrated push step exists — both members reach the same materialized-and-verified state independently through the same seam, which is what makes leadership change mid-upgrade (5.4) self-resolving rather than requiring resume logic. + +```mermaid +stateDiagram-v2 + [*] --> Fenced: marker applied, materialization not yet verified + Fenced --> Fenced: hash mismatch or materialization incomplete, retry via delaymgr trampoline + Fenced --> Verified: hash-verify succeeds against current marker + Verified --> [*]: fence cleared, serving and promotion allowed +``` + +## Requirements Traceability + +| Requirement | Summary | Components | Interfaces | Flows | +|---|---|---|---|---| +| 1.1 | Materialize at marker apply, not commit | `apply_committed_entry` new case | `persist_checkpoint` | Sequence diagram | +| 1.2 | Every member ends up holding the snapshot | Per-member apply seam + existing transport | `ha_handle_snapshot_chunk` (reused) | Sequence diagram | +| 1.3 | No member re-derives via replay | `do_upgrade_config_with_bytes` step 9 removed | — | — | +| 1.4 | Transfer out-of-band, not in WAL | Existing `snapshot_transfer_s` (reused) | — | — | +| 1.5 | Synchronous, non-blocking trigger | New `apply_committed_entry` case calling `persist_checkpoint` directly | — | Sequence diagram | +| 1.6 | Host admin-status in snapshot | `collect_host_frames`/`HostAdminState` | capnp struct | — | +| 1.7 | Exclude dynamic host metrics | `collect_host_frames` field selection | — | — | +| 1.8 | Apply does not itself re-home entities | Existing `lost_and_found` re-homing untouched; apply vs activation separation | Activation Fence | State diagram | +| 1.9 | Host/queue frames extensible (capnp) | `HostAdminState`/`QueueAdminState` | `controller.capnp` | — | +| 1.10 | Create-if-missing host placeholder | `restore_host_state` | `makeWorker()`-based helper | — | +| 1.11 | Real join routes through `rejoin_worker` | `restore_host_state` placeholder + existing `join.c` dispatch (unchanged) | `rejoin_worker` (reused) | — | +| 2.1 | Config embedded in snapshot | New config-bytes section in `write_snapshot` | `p->cluster->myconf` direct read | — | +| 2.2 | Supersede R7 interim byte-push for upgrades | `do_upgrade_config_with_bytes` retirement | — | — | +| 2.3 | Config bytes out of WAL | Unchanged (`WAL_CONFIG_MARKER` already `{tag,sha}` only) | — | — | +| 2.4 | Every checkpoint embeds current config | Config-bytes section added to every `write_snapshot` call, not just marker-triggered | — | — | +| 3.1 | No new promotion-eligibility mechanism | Existing Raft log-freshness gate (unchanged) | — | — | +| 3.2 | Gate serving until materialization+hash-verify | Activation Fence | `ha_dispatch_gate` | State diagram | +| 3.3 | Keep serving under last-verified config while incomplete | Activation Fence (allowlist composition) | `ha_dispatch_gate` | State diagram | +| 3.4 | Hash mismatch keeps serving/fences, retries | `upgrade_materialization_verify` | `schedule_upgrade_materialization_retry` | State diagram | +| 4.1 | Every member's materialized state identical | Per-member apply seam + `snapdiff_paths` (test) | — | Sequence diagram | +| 4.2 | Every member's active config identical | New `configcompare` tool | `configcompare_paths` | — | +| 4.3 | Invariant holds across upgrade x failover | Activation Fence + existing promotion gate composition | — | — | +| 4.4 | Overlapping upgrades supersede cleanly | Per-marker deterministic content (5.1) + fence re-verify on newer marker | — | — | +| 5.1 | Deterministic snapshot content | `write_snapshot` keyed on `(term,lsn,config-hash)` | `snapdiff_paths` (test) | — | +| 5.2 | Crash before durable write resumes from marker | Existing `checkpoint_in_progress` CAS + apply-pump re-apply on restart | — | — | +| 5.3 | Forced checkpoint shares single-in-flight guard | `checkpoint_in_progress` CAS (reused) | `persist_checkpoint` | — | +| 5.4 | Leadership change resumes materialization | Per-member apply seam (see Key simplification) | — | Sequence diagram | +| 5.5 | Never serve without durable+verified snapshot | Activation Fence | `ha_dispatch_gate`, promotion gate | State diagram | +| 5.6 | Rolling-binary-upgrade format skew fail-loud | Existing `fmt != SNAPSHOT_FORMAT_VERSION` hard-reject (reused) | — | — | +| 5.7 | Old raw queue-frame layout rejected fail-loud | `SNAPSHOT_FORMAT_VERSION` bump 1→2 | — | — | +| 5.8 | Idempotent retry of placeholder host restore | `restore_host_state` (find-before-create) | — | — | +| 6.1 | Chaotic upgrade+failover: identical state/config | HA soak new scenario | `configcompare`, `snapdiff_paths` | — | +| 6.2 | Reproduce bug-857 on old code, pass on new | `test_bug857_regression.c` (unit-level, `controller_unit_test`) — see correction note below | `snapdiff_paths` | — | +| 6.3 | Inject crash/leadership-change at marker-to-install window | New fault hooks (`persist.c`, `config_repl.c`) | — | — | +| 6.4 | Host admin-status explicitly asserted post-upgrade+failover | HA soak new scenario assertion | — | — | +| 7.1 | Placeholder-then-rejoin admin status preserved | `test_upgrade_snapshot_restore.c` | `restore_host_state`, `rejoin_worker` | — | +| 7.2 | Overlapping upgrades cleanly superseded | HA soak new scenario | — | — | +| 7.3 | Format-mismatch fail-loud (rolling upgrade + queue-layout) | `test_upgrade_snapshot_apply.c` / manual format-mismatch fixture | — | — | +| 7.4 | Marker-triggered checkpoint's WAL trim does not strand a mid-catch-up standby | `test_upgrade_snapshot_apply.c` extended + HA soak new scenario | `trim_wal_segments`, `repl_trim_lsn` (reused, unmodified) | — | + +**Correction to row 6.2 (task 8.5, post-implementation):** this row originally assigned Requirement 6.2 to "HA soak new scenario," under the same bug-857 mechanism hypothesis as the buglog entry — "config-push + restart + per-node WAL replay, config-dependent via `lost_and_found`." Task 8.5's investigation found that mechanism was never implemented as code, on any commit: `create_lost_and_found` (config_repl.c) only ever creates the sentinel `lost_and_found` queue/host objects; no code path in `job.c`/`queue.c`/`worker.c` ever reassigns an EXISTING job's queue reference onto them (`restore_job`/`restore_job_array` never consult `c->qmgr`; `restore_queue_state` explicitly skips — returns `-ENOENT` — rather than re-homes when a queue is absent). `create_lost_and_found`'s body is also unconditionally `#ifndef XCE_TESTING`'d out. The buglog's own bug-857 entry already flagged this as "NOT empirically reproduced yet" — the re-homing detail was TLA+-derived aspiration, not observed behavior. + +The real, code-backed mechanism 6.2 reproduces instead is **checkpoint-content divergence from marker-vs-threshold decoupling**: pre-task-5, `apply_committed_entry`'s `WAL_CONFIG_MARKER` case was the generic no-op `default:` case, so a checkpoint's LSN/content was determined solely by `schedule_checkpoint`'s independent per-member WAL-record-count threshold — completely decoupled from the config-change marker's position in the shared committed log. Two members reaching that threshold at different real-world moments (ordinary job-submission activity interleaved around the upgrade) materialize genuinely different job/queue content, with neither checkpoint anchored to the marker — exactly "no snapshot pinning an identical post-upgrade state" (bug-857's own root-cause wording). Post-task-5, the marker-triggered checkpoint fires synchronously, inline, the instant the marker entry is dispatched, anchoring every member to identical content regardless of processing pace. + +This mechanism is fully reproducible at the `controller_unit_test` level (no `cluster.c`/live-multi-process linkage needed), so 6.2 was moved off the "HA soak new scenario" assignment onto `testsuite/test_bug857_regression.c` — a two-scenario regression (threshold-triggered divergence pre-fix; marker-triggered convergence post-fix, verified via `snapdiff_paths` byte-identity) landed entirely within this one file. Requirement 6.1's full live chaotic-soak-with-upgrade (task 8.6, not yet started) remains the legitimate home for live multi-process testing of the broader upgrade+failover interaction — it will need to build live config-upgrade-trigger infrastructure regardless of what 8.5 did, so nothing is lost by 8.5 staying at the unit level. Not rewriting the original row's history — see task 8.5's Status Report for the full investigation trail. + +## Components and Interfaces + +| Component | Domain/Layer | Intent | Req Coverage | Key Dependencies (P0/P1) | Contracts | +|---|---|---|---|---|---| +| Marker Apply Trigger | Persistence (`persist.c`) | Fire a forced checkpoint synchronously when `WAL_CONFIG_MARKER` is applied | 1.1, 1.5, 1.8, 7.4 | `persist_checkpoint` (P0), `checkpoint_in_progress` CAS (P0) | State | +| Snapshot Entity/Config Payload | Persistence (`persist.c`) | Embed host/queue admin frames (capnp) and config bytes in every checkpoint | 1.6, 1.7, 1.9, 2.1, 2.4, 5.1, 5.7 | `HostAdminState`/`QueueAdminState` capnp (P0), `myconf` fields (P0, direct read) | State | +| Host Restore (create-if-missing) | Worker/Restore (`worker.c`) | Restore a host admin-state frame safely whether or not the host has ever joined | 1.10, 1.11, 5.8 | `makeWorker` (P0), `rejoin_worker` (P0, unmodified) | Service | +| Activation Fence | HA (`ha.c`/`ha.h`) | Gate primary serving and promotion on this member's own materialization-and-hash-verify | 1.8, 3.2, 3.3, 3.4, 4.3, 5.5 | `ha_dispatch_gate` (P0), promotion gate (P0), `config_serve_fence` pattern (P1, mirrored not shared) | State | +| Upgrade Path Retirement | Config Replication (`config_repl.c`) | Remove the restart/replay fallback now superseded by the Marker Apply Trigger | 1.2, 1.3, 1.4, 2.2, 5.4 | Marker Apply Trigger (P0) | — | +| Regression Tooling | Testsuite | New fault hooks, `configcompare`, extended `snapverify`, HA soak scenarios | 4.2, 6.1-6.4, 7.1-7.4 | All above (P0) | Batch | + +### Persistence (`persist.c`) + +#### Marker Apply Trigger + +| Field | Detail | +|---|---| +| Intent | Add the missing `WAL_CONFIG_MARKER` case to the single unified apply seam so every member materializes at the marker boundary | +| Requirements | 1.1, 1.5, 1.8, 7.4 | + +**Responsibilities & Constraints** +- Carve a new explicit `case WAL_CONFIG_MARKER:` out of `apply_committed_entry`'s current `default:` no-op. +- Call `persist_checkpoint()` directly — safe because `apply_committed_entry` already runs on the apply-pump's own vtask-pool worker, never the iomgr/delaymgr thread; no further async hand-off is needed at this call site (unlike `schedule_checkpoint`'s WAL-threshold trigger, which fires from arbitrary vtask handlers and genuinely needs delaymgr's non-blocking return). +- Must NOT change how entities are interpreted/re-homed (`lost_and_found`) — that remains the activation step's concern (1.8), never a side effect of apply. +- `apply_wal_entry`'s matching `default:` case is left untouched — it has zero live callers; adding symmetry there would be speculative, unjustified work (Design Invariant 5 / Simplification lens). + +**Dependencies** +- Inbound: apply pump (`persist_apply_pump_run`) and boot replay (P0) +- Outbound: `persist_checkpoint` (P0) — already tolerates `-EBUSY` from a concurrent scheduled checkpoint via the shared `checkpoint_in_progress` CAS +- Outbound: new `#ifdef XCE_TESTING` fault hook, mirroring `ha_stream_body_read_fault_hook`'s shape, for 6.3 and 7.3's deterministic marker-to-checkpoint-durable injection + +**Contracts**: State [x] + +##### State Management +- State model: `checkpoint_lsn` is pinned to `last_applied_lsn` inside `persist_checkpoint` — since this case runs exactly when the marker is applied, `checkpoint_lsn` naturally equals the marker's own LSN with no new LSN-targeting parameter needed. +- Persistence & consistency: guarded by the existing `checkpoint_in_progress` CAS; a concurrent WAL-threshold-triggered checkpoint and this marker-triggered one cannot both proceed simultaneously. +- Concurrency strategy: runs to completion on the calling vtask-pool worker; no new thread or lane. + +**Implementation Notes** +- Integration: one new `case` label plus one function call; `do_upgrade_config_with_bytes` step 9 becomes dead code and is removed in the same change (Upgrade Path Retirement, below). +- Validation: `test_upgrade_snapshot_apply.c` asserts the checkpoint fires exactly once per marker apply and is safe under a forced retry (idempotent — a second apply of an already-applied marker, e.g. during replay, must not double-checkpoint incorrectly; `checkpoint_lsn` monotonicity already guards this at the `persist_checkpoint` level). +- Risks: the narrow window between `schedule_checkpoint`'s CAS release and `persist_checkpoint`'s own re-acquire (pre-existing, `persist.c:2222`/`2265`) now has a second caller; not a new race class, but worth a regression assertion that a marker-triggered and WAL-threshold-triggered checkpoint racing each other never corrupts the snapshot (covered by the existing CAS, asserted for completeness). +- **Overlapping-marker `-EBUSY` handling (4.4, 7.2)**: if a second `WAL_CONFIG_MARKER` is applied while an earlier marker's forced checkpoint is still in flight, this case's `persist_checkpoint()` call returns `-EBUSY` — this is NOT an error to surface or retry specially. The in-flight checkpoint (for the older marker) completes on its own, and the Activation Fence's own verify step (which re-reads the *latest committed* marker, not whichever one triggered the checkpoint) naturally re-evaluates against the newer marker once that checkpoint's `write_snapshot` call runs — either from the older checkpoint's own completion re-triggering a fresh comparison, or from the newer marker's normal WAL-threshold checkpoint eventually firing. No special retry loop is added at this call site for `-EBUSY`; it is logged at `TRACE`, not `WARN`/`ERROR`, since it is an expected, self-resolving outcome, not a failure. +- **Trim safety under the new trigger path (7.4)**: `persist_checkpoint()` (this case's own call) already runs `trim_wal_segments` afterward, unchanged — the retention-floor guard (`repl_trim_lsn`, bug-1147: floor = min(active segment LSN, every connected standby's acked LSN, every recently-disconnected slot's last-disconnected LSN)) is not modified by this spec, only invoked from a new trigger point. Six existing tests (`test_checkpoint_wal_trim_after_checkpoint`, `test_trim_guard_preserves_standby_segments`, `test_trim_without_standbys_proceeds_normally`, `test_repl_trim_lsn_no_standbys`, `test_repl_trim_lsn_disconnected_slot_expiry`, `test_trim_lsn_with_active_standby_at_lsn_5`) already cover this guard through the periodic WAL-threshold trigger — none exercise it through this new marker-triggered path, or under the chaotic multi-kill conditions Requirement 6 exercises, which is a real coverage gap this spec must close, not assume covered by proxy. + +#### Snapshot Entity/Config Payload + +| Field | Detail | +|---|---| +| Intent | Extend the on-disk checkpoint format with host admin-state frames, migrate queue frames to capnp, and embed full configuration bytes | +| Requirements | 1.6, 1.7, 1.9, 2.1, 2.4, 5.1, 5.7 | + +**Responsibilities & Constraints** +- `collect_host_frames`: walk `c->workermgr` with the same lock/iterate idiom `collect_job_frames`/`collect_queue_frames` already use; encode only `w->host.status & XCE_WORKER_ADMIN_MASK` per host as a `HostAdminState` capnp frame — never the full `xce_worker_t` (excludes `resvals_to_sched`, `dead_since`, dynamic status bits, per 1.7). +- `collect_queue_frames`: migrated to emit `QueueAdminState` capnp frames instead of the current raw `{name_len, name, status}` layout — same field selection (name + status), new wire encoding only. +- Config-bytes section: `write_snapshot` reads the locally-held active configuration bytes directly off disk via `p->cluster->myconf->workdir`/`myconf->name` (the same `workdir/name/conf` path `conf_save_config` writes, and the same bytes `config_activate_committed` already hash-verifies), appended after the host frames. No new `config_repl.c` function is added for this — see the File Structure Plan note on why (mirrors `scan_latest_committed_config_ref`'s existing avoid-a-config_repl-dependency precedent). +- `SNAPSHOT_FORMAT_VERSION` bumps 1→2 — this is a genuine breaking layout change (queue frames change shape), so the existing fail-loud version-mismatch behavior (`load_snapshot`, `fmt != SNAPSHOT_FORMAT_VERSION → -EINVAL`) applies unchanged; no compatibility shim is added (Design Invariant 4). +- Determinism (5.1): content is a pure function of `(term, lsn, config-hash)` — independent materialization by any member (including a newly-elected leader) must be byte-identical, verified via `snapdiff_paths`. + +**Dependencies** +- Inbound: Marker Apply Trigger (P0, calls `persist_checkpoint` which calls `write_snapshot`); periodic WAL-threshold checkpoint (P0, same `write_snapshot` gets the same new sections on every checkpoint per 2.4) +- Outbound: `controller.capnp`'s new `HostAdminState`/`QueueAdminState` structs (P0) +- Outbound: `p->cluster->myconf` fields (P0, plain struct-field read on the shared `cluster_t`/`myconf_t` — not a `config_repl.c` function call, preserving the existing layering precedent) + +**Contracts**: State [x] + +##### State Management +- State model: additive frame sections after the existing job/queue frames; config-bytes section is new, appended last. +- Persistence & consistency: same single-commit-point discipline `load_snapshot` already enforces (restore fully, THEN advance position; a partial restore leaves `persist_t`'s fields untouched). +- Concurrency strategy: unchanged — collection happens under the same table read-locks already used, no new locking model. + +**Implementation Notes** +- Integration: `HostAdminState`/`QueueAdminState` capnp structs added to `controller.capnp`; regenerate bindings. +- Validation: `snapverify` extended to decode/assert `cfgref_tag`/`cfgref_sha` (not just presence) and report host-frame count and config-bytes-section presence; `snapdiff_paths` used as-is for the determinism check. +- Risks: none new beyond the format-version bump's inherent one-time old-snapshot invalidation, already an accepted, existing pattern. + +### Worker/Restore (`worker.c`) + +#### Host Restore (create-if-missing) + +| Field | Detail | +|---|---| +| Intent | Restore a host admin-state frame correctly whether the target host has ever joined on this member or not | +| Requirements | 1.10, 1.11, 5.8 | + +**Responsibilities & Constraints** +- Classify hosts as join-provisioned (not config-provisioned, unlike queues): `key_find(c->workermgr, name)` may legitimately miss for a member receiving the snapshot via the out-of-band install path, since that is precisely the "hasn't locally replayed this host's join record yet" case. +- On miss: create a minimal placeholder via `makeWorker()` with a valid, non-NULL `hostattrs_t` (even if minimal/zeroed) — mirroring the `lost_and_found` sentinel host precedent exactly, not a new mechanism. A NULL/absent `attrs` here would crash `rejoin_worker`'s later direct dereference on the first real join (1.11) — this is the one hard implementation constraint carried over from discovery. +- On hit (placeholder or previously-joined real host): OR-in the restored admin-status bits, same as today's behavior. +- Idempotency (5.8): a retried restore after a partial failure must find the already-created placeholder and update it, never treat its existence as a create conflict — `key_find`-then-create-or-update already gives this for free as long as the create step itself is the only thing guarded (i.e., find-first, not create-first-then-handle-EEXIST). +- The later real join is NOT this component's concern: `join.c`'s existing `key_find`-based dispatch (found `w != NULL` → `rejoin_worker`, else → `init_worker`) already routes a real join against an existing placeholder through `rejoin_worker`, which already preserves admin bits (`worker.c:374,390`) — no change needed there. + +**Dependencies** +- Inbound: Snapshot Entity/Config Payload's restore path (P0, calls this per host frame) +- Outbound: `makeWorker` (P0, existing) +- Outbound: `rejoin_worker` (P0, existing, unmodified — the later real join's correctness depends on this component creating a valid placeholder, not on any change to `rejoin_worker` itself) + +**Contracts**: Service [x] + +##### Service Interface +```c +int restore_host_state(cluster_t* c, const char* name, uint32_t admin_status); +``` +- Preconditions: `name` non-empty; `c->workermgr` initialized. +- Postconditions: a `worker_t` named `name` exists in `c->workermgr` with `host.status` including `admin_status`'s bits; if newly created, `host.attrs` is non-NULL and heap-owned (via `makeWorker`'s underlying `copy_worker` deep-copy). +- Invariants: never destroys a pre-existing worker's non-admin state; never leaves a partially-constructed placeholder on any error path (mirrors the existing collect/restore all-or-nothing discipline). + +**Implementation Notes** +- Integration: called from `persist.c`'s host-frame restore loop, one call per frame. +- Validation: `test_upgrade_snapshot_restore.c` covers create-then-later-real-join (asserting admin status survives through `rejoin_worker`), and retry-after-partial-failure (asserting no duplicate/conflict). +- Risks: none beyond the `attrs`-non-NULL constraint already called out — mitigated by construction (always pass a valid, even if zeroed, `hostattrs_t` to `makeWorker`). + +### HA (`ha.c`/`ha.h`) + +#### Activation Fence + +| Field | Detail | +|---|---| +| Intent | Never let a member serve client requests or become primary under a configuration whose upgrade-snapshot materialization it has not itself completed and hash-verified | +| Requirements | 1.8, 3.2, 3.3, 3.4, 4.3, 5.5 | + +**Responsibilities & Constraints** +- A new, independent atomic fence field on `ha_state_t` — NOT merged into `config_serve_fence`, since the two conditions are logically distinct (byte-mismatch vs. materialization-incomplete) and 3.4 needs to distinguish "keep serving under last-verified configuration" from "no verified configuration exists yet," which requires knowing which fence is active. +- Verify function mirrors `config_activate_committed`'s exact shape: read the current committed marker, compare against this member's own materialized-and-hash-verified state for that marker; set the fence and log loud on mismatch/incomplete, clear it on success — never `exit`/`abort` (`controller/CLAUDE.md`'s never-self-terminate invariant). +- **Verify against LIVE on-disk config bytes each attempt, never a frozen snapshot's embedded copy.** The WAL marker's raft-commit and the R7 config-bytes push (`push_config_to_standbys`/`handle_repl_config_push`) are two independent channels with no strict ordering guarantee between them — a member could apply the marker and trigger its forced checkpoint before its own `workdir/name/conf` reflects the pushed bytes, meaning the snapshot's embedded config section could be momentarily stale. This is not a correctness risk (a stale embed is just one more mismatch the hash-verify catches, same as any other), but it IS a liveness trap if the verify function checks the snapshot's frozen embed instead of re-reading current disk bytes fresh on every retry — `config_activate_committed` already gets this right (recomputes the hash from current bytes each call, `config_repl.c:275-302`, never from a cached/frozen copy), and this new verify function must do the same, or the fence could stay stuck until the next unrelated periodic checkpoint happens to re-embed fresher bytes. +- Composed into `ha_dispatch_gate`'s `CTRL_PRIMARY` branch the same way `config_serve_fenced()` already is — a sequential additional check with its own allowlist (replication + read-only ops stay admitted), not a merge into the existing check's logic. +- Also gates promotion, the same way `config_serve_fenced()`/`ha_config_promotion_fenced()` already do — a member whose own materialization is incomplete must not become primary and immediately fail 3.2 the moment it wins an election. +- Self-clearing retry: a new `schedule_upgrade_materialization_retry()` mirroring `config_schedule_activate_retry`'s delaymgr-trampoline-then-vtask shape exactly — non-blocking trigger, blocking verify work done off the delaymgr thread. + +**Dependencies** +- Inbound: Marker Apply Trigger (P0, clears the fence once materialization+hash-verify succeeds) +- Outbound: `ha_dispatch_gate` (P0, existing, extended) +- Outbound: promotion gate (P0, existing, extended) +- Outbound: `config_schedule_activate_retry`'s trampoline pattern (P1, mirrored, not shared code) + +**Contracts**: State [x] + +##### State Management +- State model: two states per member — fenced (materialization incomplete or hash-mismatched for the current marker) and verified (cleared). +- Persistence & consistency: in-memory only, re-derived on restart from the actual on-disk snapshot state vs. the current committed marker (no separate durable flag needed — the fence is a function of comparing two already-durable facts). +- Concurrency strategy: same single-writer-plus-atomic-read pattern `config_serve_fence` already uses. + +**Implementation Notes** +- Integration: one new field, one new verify function, one new accessor, two call-site additions (dispatch gate, promotion gate), one new retry trampoline. +- Validation: `test_upgrade_activation_fence.c` covers fence-set-on-incomplete-materialization, fence-clear-on-verify-success, dispatch-gate allowlist behavior while fenced, and promotion refusal while fenced. +- Risks: none beyond what the mirrored `config_serve_fence` pattern already carries in production today. + +### Config Replication (`config_repl.c`) + +#### Upgrade Path Retirement + +| Field | Detail | +|---|---| +| Intent | Remove the now-superseded restart/per-node-replay fallback | +| Requirements | 1.2, 1.3, 1.4, 2.2, 5.4 | + +**Responsibilities & Constraints** +- Delete `do_upgrade_config_with_bytes` step 9 (`restart_requested`/`cbuf_put`) — materialization is now driven entirely by the Marker Apply Trigger, decoupled from this request-handling function entirely; the function's job now ends at step 8 (ACK the admin tool once the marker+bytes are majority-committed). +- Replace the NOTICE comment naming this spec with a short pointer at the new `apply_committed_entry` case, so the historical context isn't lost. +- No new distribution-tracking logic is added here (see Architecture's "Key simplification found during design") — 5.4 is satisfied by the per-member apply model, not by anything this function does. + +**Dependencies** +- Inbound: none (this is a deletion, not a new caller) +- Outbound: Marker Apply Trigger (P0) now owns what this code used to trigger via restart + +**Contracts**: none (behavioral removal) + +**Implementation Notes** +- Integration: this change and the Marker Apply Trigger must land together — deleting step 9 without the new case first would silently leave the marker with no materialization trigger at all (a real ordering dependency for tasks.md to sequence correctly). +- Validation: the bug-857 regression (6.2) exercises this path directly — reproduces divergence on the pre-change code (restart+replay), passes on the post-change code (no restart, synchronous materialization). +- Risks: none new; this is strictly subtractive once the replacement is proven. + +## Data Models + +### Logical Data Model + +**Checkpoint file layout (on-disk), `SNAPSHOT_FORMAT_VERSION = 2`**: +1. Legacy sentinel header (unchanged): magic `"XCESNAP\0"` (8 bytes) + `checkpoint_lsn` (8 bytes) + config-ref tail (`present`/`tag`/`sha`, 41 bytes). +2. `format_version` (4 bytes) — now `2`. +3. Job frames (unchanged): `job_count` + per-job `[payload_len][capnp Job bytes]`. +4. Queue frames (changed): `queue_count` + per-queue `[payload_len][capnp QueueAdminState bytes]` — was `[name_len][name][status]` raw. +5. Host frames (new): `host_count` + per-host `[payload_len][capnp HostAdminState bytes]`. +6. Config-bytes section (new): `config_bytes_len` + raw config bytes (the same bytes `config_activate_committed` hash-verifies against the marker's sha256). + +**Referential integrity**: restore order (job → queue → host) does not matter functionally — re-homing decisions (`lost_and_found`) are already baked into each entity's encoded content at materialization time, never re-derived at restore time (Design Invariant 2). + +**Versioning**: the `format_version` field is the sole discriminator; any mismatch is a hard fail-loud reject (`-EINVAL`), consistent with the existing, deliberate no-silent-reinterpretation policy. A short file (nothing past the legacy header) is still detected as a pre-entity-bearing legacy checkpoint, unchanged from today. + +## Error Handling + +### Error Strategy +Every new failure path follows the codebase's existing convention: fail loud (log the actionable boundary), never `exit`/`abort` on an operational condition, stay alive, keep retrying when self-healing is possible. + +### Error Categories and Responses +- **Format-version mismatch** (rolling binary upgrade skew on snapshot-install, or an old raw-queue-frame snapshot post-migration): hard reject, no partial install, no silent reinterpretation — reuses the existing `fmt != SNAPSHOT_FORMAT_VERSION` path unchanged. +- **Hash-verify mismatch** (embedded config bytes don't match the marker's sha256, or materialization incomplete): Activation Fence sets, member keeps serving under its last-verified configuration (or a loud non-serving fence if none exists yet), retries via the delaymgr trampoline — never activates mismatched configuration. +- **Host placeholder creation failure** (`makeWorker` allocation failure): treated as any other collect/restore failure today — discard partial state, fail the checkpoint/restore attempt, retriable on the next cycle; never a crash. +- **Overlapping upgrades** (a second marker commits before the first's distribution completes): the newer marker's materialization supersedes the older one's in-flight state cleanly, since content is a deterministic function of `(term, lsn, config-hash)` — an in-progress older materialization simply becomes moot, not corrupting, once the newer one completes. + +### Monitoring +Reuses existing structured-log conventions (`INFO`/`WARN`/`ERROR` per the codebase's HA logging pattern) — a loud `ERROR` on fence-set (naming the marker's `(term,lsn)` and the mismatch reason), `INFO` on fence-clear, matching `config_serve_fence`'s existing log shape. + +## Testing Strategy + +- **Unit Tests** (`test_upgrade_snapshot_apply.c`): `WAL_CONFIG_MARKER` case fires a checkpoint exactly once per apply (1.1, 1.5); idempotent under a forced retry (5.2, 5.3); does not itself re-home entities (1.8). +- **Unit Tests** (`test_upgrade_snapshot_restore.c`): host-frame create-if-missing via placeholder (1.10); admin status survives through a later real join via `rejoin_worker` (1.11); idempotent retry after partial failure (5.8). +- **Unit Tests** (`test_upgrade_activation_fence.c`): fence set/clear on hash-verify outcome (3.2, 3.4); dispatch-gate allowlist while fenced (3.3); promotion refusal while fenced (4.3, 5.5). +- **Integration Tests**: `snapdiff_paths` determinism check — two independent materializations at the same `(term, lsn, config-hash)` boundary are byte-identical (5.1). `configcompare` cross-member active-config identity (4.2). Extended `snapverify` decodes/asserts the embedded config-ref tag/hash and reports host-frame/config-bytes-section presence. bug-857 reproduction (6.2) — see the Requirements Traceability table row and the correction note above this section: reassigned off "E2E / HA soak" to `test_bug857_regression.c`, a unit-level demonstration that `schedule_checkpoint`'s pre-existing WAL-threshold trigger (decoupled from the config-marker boundary) diverges two members' checkpoint content, and that the marker-triggered checkpoint added by this spec converges them regardless of per-member processing pace. +- **E2E / HA soak** (`.claude/skills/xce-test-ha`): config upgrade interleaved with chaotic multi-kill failover, asserting identical materialized state and active config across all members afterward, including host admin-status specifically (6.1, 6.4); deterministic fault injection at the marker-apply→checkpoint-durable window and the leadership-change-mid-distribution window via the two new `#ifdef XCE_TESTING` hooks, rather than relying on chaotic-soak timing probability (6.3); overlapping/back-to-back upgrades before the first's distribution completes, asserting clean supersession (4.4, 7.2); host admin-status change issued while the host is down, followed by an upgrade snapshot restore on a member with no local join record, followed by the host's real first join, asserting preservation end-to-end (7.1); format-mismatch fail-loud for both rolling-binary-upgrade skew and the old raw-queue-frame layout (5.6, 5.7, 7.3); a standby held genuinely mid-catch-up (its acked LSN behind the marker's) while the marker-triggered checkpoint's WAL trim runs, asserting its still-needed segments are not stranded — extending `test_upgrade_snapshot_apply.c` with a fixture mirroring `test_trim_guard_preserves_standby_segments`'s existing pattern but driven through the new marker-apply case, plus one HA soak assertion under chaotic conditions specifically (7.4). + +## Migration Strategy + +A single `SNAPSHOT_FORMAT_VERSION` bump (1→2) covers the queue-frame capnp migration and the new host-frame/config-bytes sections together, so this spec introduces exactly one rollout discontinuity, not several staged ones. An old-format on-disk snapshot becomes unreadable by the new binary until that node's next checkpoint cycle regenerates one — consistent with the existing, deliberate fail-loud version-mismatch policy (not a new risk this spec introduces). No phased rollback path is defined beyond redeploying the prior binary version, matching how every prior `SNAPSHOT_FORMAT_VERSION` change in this codebase has been handled. + +## Supporting References +- `research.md` — full discovery findings, exact file:line citations for every function referenced above, and the design-synthesis rationale (generalization/build-vs-adopt/simplification) behind each decision in this document. +- `requirements.md` — the 7 requirements / 38 acceptance criteria this design traces to, plus the Design Invariants section this design's component choices were checked against. diff --git a/.kiro/specs/config-upgrade-snapshot/tasks.md b/.kiro/specs/config-upgrade-snapshot/tasks.md index 475cf03..61c393a 100644 --- a/.kiro/specs/config-upgrade-snapshot/tasks.md +++ b/.kiro/specs/config-upgrade-snapshot/tasks.md @@ -114,7 +114,7 @@ - _Depends: 2.3_ - _Boundary: Regression Tooling (configcompare, snapverify)_ -- [ ] 8.5 Reproduce bug-857 on the pre-change behavior and confirm it no longer occurs +- [x] 8.5 Reproduce bug-857 on the pre-change behavior and confirm it no longer occurs - Run the original materialized-state-divergence repro scenario against the pre-change code path and confirm the divergence occurs - Run the same scenario against the post-change behavior and confirm every member's materialized state converges identically - Observable: the same repro scenario demonstrably fails on the old behavior and passes on the new one @@ -149,6 +149,8 @@ ## Implementation Notes +- **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.` 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 ' | 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. - **Task 8.3: no new tests needed** — the 10 tests already in `test_config_repl.c` from tasks 6.1/6.2 fully satisfy both bullets and Requirements 3.2/3.3/3.4/4.3/5.5 (verified independently by implementer and reviewer; zero-diff task). The two "fence sets" tests genuinely toggle different halves of `upgrade_materialization_verify`'s condition (materialized vs. bytes_verified), not duplicates. Retry-trampoline automatic-firing (delaymgr actually running) is untested, matching the exact precedent depth of `config_activate_retry_fn`/`config_schedule_activate_retry` (also untested at that depth anywhere in this codebase) — judged out of scope, not a gap. diff --git a/src/controller/meson.build b/src/controller/meson.build index 52204b5..f7983fc 100644 --- a/src/controller/meson.build +++ b/src/controller/meson.build @@ -176,6 +176,7 @@ controller_unit_test_src = [ 'testsuite/test_job_array_replay_fence.c', 'testsuite/test_upgrade_snapshot_restore.c', 'testsuite/test_upgrade_snapshot_apply.c', + 'testsuite/test_bug857_regression.c', 'ctrlstatus.c', 'commit.c', 'job.c', diff --git a/src/controller/testsuite/controller_test.c b/src/controller/testsuite/controller_test.c index 13d56d6..85fed32 100644 --- a/src/controller/testsuite/controller_test.c +++ b/src/controller/testsuite/controller_test.c @@ -91,6 +91,7 @@ extern void test_job_array_fence(void); extern void test_job_array_replay_fence(void); extern void test_upgrade_snapshot_restore(void); extern void test_upgrade_snapshot_apply(void); +extern void test_bug857_regression(void); void setUp(void) { return; } @@ -251,6 +252,7 @@ int main(int argc, char* argv[]) TEST(test_job_array_replay_fence); TEST(test_upgrade_snapshot_restore); TEST(test_upgrade_snapshot_apply); + TEST(test_bug857_regression); return UNITY_END(); } diff --git a/src/controller/testsuite/test_bug857_regression.c b/src/controller/testsuite/test_bug857_regression.c new file mode 100644 index 0000000..98299af --- /dev/null +++ b/src/controller/testsuite/test_bug857_regression.c @@ -0,0 +1,513 @@ +/* + * test_bug857_regression.c — config-upgrade-snapshot task 8.5 + * + * Reproduces the bug-857 materialized-state divergence on the PRE-fix + * mechanism and proves it converges on the CURRENT (post-task-5) one, both + * at the controller_unit_test level, entirely within this one file. + * + * ── Investigation summary (why this file looks the way it does) ──────── + * + * Task 8.5's original brief hypothesized bug-857's divergence as "a job + * whose queue was removed by a config upgrade replays into its original + * queue on one member and gets re-homed to the synthetic `lost_and_found` + * queue on another, depending on each member's own restart timing." + * Investigation (this task) found that mechanism does not exist as + * implemented code, on ANY commit: `create_lost_and_found` (config_repl.c) + * only ever creates the sentinel queue/host objects — no code path in + * job.c/queue.c/worker.c ever reassigns an EXISTING job's queue reference + * onto them. `restore_job`/`restore_job_array` (job.c) never consult + * `c->qmgr` at all; `restore_queue_state` (queue.c) explicitly SKIPS + * (returns -ENOENT) rather than re-homes when the target queue is absent. + * `create_lost_and_found`'s body is also unconditionally `#ifndef + * XCE_TESTING`'d out. bug-857's buglog entry itself already flagged this + * as "NOT empirically reproduced yet" — the re-homing detail was TLA+- + * derived aspiration, not observed behavior. + * + * design.md's Requirements Traceability table (row 6.2) originally + * assigned this requirement to "HA soak new scenario" — written under the + * same since-corrected hypothesis. The real, code-backed divergence this + * file reproduces instead is CHECKPOINT-CONTENT divergence: pre-task-5, + * `apply_committed_entry`'s WAL_CONFIG_MARKER case was a no-op (the + * generic `default:` case) and a checkpoint's LSN/content was determined + * SOLELY by `schedule_checkpoint`'s independent per-member WAL-record-count + * threshold (persist.c) — completely decoupled from the config-change + * marker's own position in the shared committed log. Two members reaching + * that threshold at different real-world moments (because other activity — + * more job submissions — is interleaved around the upgrade) materialize + * genuinely different job/queue content, with NEITHER checkpoint anchored + * to the marker at all: exactly "no snapshot pinning an identical + * post-upgrade state" (bug-857's own root-cause wording). Post-task-5, + * `apply_committed_entry`'s WAL_CONFIG_MARKER case forces + * `persist_checkpoint_marker_triggered()` synchronously, INLINE, the + * instant the marker entry itself is dispatched — before the per-LSN + * apply loop ever reaches any later entry — so every member's + * marker-anchored checkpoint captures identical content regardless of its + * own processing pace. + * + * Both scenarios below run entirely within controller_unit_test — no + * cluster.c linkage (cluster_init/cluster_build_resources), no scratch + * worktree, no live multi-process cluster needed. See this task's Status + * Report / design.md's 6.2 traceability-row correction note for the full + * decision trail. + * + * ── Scenario 1: test_bug857_threshold_checkpoint_diverges_across_members ── + * Reproduces the PRE-task-5 mechanism for real: two members receive the + * identical physical WAL (job1, config-change marker, job2), but their + * entity state and checkpoint are driven manually via restore_job() + + * persist_checkpoint() — the SAME production functions + * apply_committed_entry()/schedule_checkpoint() would themselves call — + * WITHOUT ever routing the marker's LSN through apply_committed_entry() + * itself (which, on the CURRENT codebase, unconditionally forces the + * marker-anchored checkpoint task 5 added, masking exactly the bug this + * scenario needs to demonstrate the absence of). Member A's threshold + * trips right after job1 (before job2 exists locally); member B's does not + * trip until after job2 exists locally too — modeling two members whose + * own independent checkpoint/restart timing straddles ordinary cluster + * activity happening around the upgrade. Asserts the two members' checkpoint + * files are NOT byte-identical (snapdiff_paths) and that their job SETS + * genuinely differ (job2 present in B, absent in A) — real content + * divergence, not a cosmetic LSN-header mismatch. + * + * ── Scenario 2: test_bug857_marker_checkpoint_converges_across_members ── + * The identical shared sequence, this time routed through the REAL, + * current apply_committed_entry() dispatch (persist_apply_pump_run) on + * both members. Member A catches up in a single pump-run call over the + * whole range; member B processes job1+marker in one call and only later + * commits+applies job2 in a separate second call — genuinely different + * processing pace. Asserts both members' checkpoint at the marker's own + * LSN exists and is byte-identical (snapdiff_paths) regardless of that + * pacing difference. + */ + +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +#include "cluster.h" +#include "config_repl.h" +#include "controller.capnp.h" +#include "hook.h" +#include "jobj.h" +#include "persist.h" +#include "snapdiff.h" +#include "wal_types.h" +#include "xce/cluster_def.h" +#include "xce/types.h" +#include "xstring.h" + +/* restore_job (job.c) has no dedicated header -- every direct-call site in + * this codebase declares it locally (mirrors persist.c's own extern at the + * top of that file). This file calls it directly (bypassing + * apply_committed_entry) in Scenario 1 for the reason explained above: the + * marker's LSN must never be dispatched through apply_committed_entry + * there, or the CURRENT codebase's marker-anchored checkpoint (task 5) + * would fire and mask the exact pre-fix behavior this test reproduces. */ +extern int restore_job(cluster_t* c, uint64_t jobid_unused, + const void* capnp_payload, size_t len, + uint64_t entry_lsn); + +/* jobj.c's jobsub table and the process-global xstring table are shared + * across every test file in the controller_unit_test binary -- guard + * idempotently, mirroring every other config-upgrade-snapshot test file + * (job_obj_init itself is also independently idempotent via its own + * process-global atomic flag, jobj.c -- this local guard just skips the + * redundant call within this file's own repeated test invocations). */ +static int g_b857_jobobj_ready; + +typedef struct { + cluster_t c; + persist_t p; + char tmpdir[256]; +} b857_t; + +/* Fixture mirrors test_upgrade_snapshot_apply.c's tusa_t/tusa_setup exactly + * (real disk-backed persist_t + a real but otherwise-empty cluster_t with + * jobmgr/qmgr/workermgr/resmgr as xkvmgr_type instances) -- not reused + * directly since tusa_setup/tusa_t are file-static, matching this + * codebase's established convention of each test file keeping its own + * minimal local copy of such fixtures (e.g. test_replay_apply_seam.c's + * trr_make_fresh_persist mirroring test_persist_recovery.c's + * make_fresh_persist). `label` must be short and filesystem-safe -- used + * as the hostname, the persist_set_workdir name, AND the tmpdir prefix. */ +static void b857_setup(b857_t* t, const char* label) +{ + kvmgr_params_t int_params = { .key_type = KVMGR_KEY_INT, + .nslots = 32, .delta = 16 }; + kvmgr_params_t str_params = { .key_type = KVMGR_KEY_STRING, + .nslots = 32, .delta = 16 }; + + if (makeString((char*)"b857-probe") == NULL) + xstring_table_init(64); + + if (!g_b857_jobobj_ready) { + TEST_ASSERT_EQUAL_INT(0, job_obj_init(32, 32)); + g_b857_jobobj_ready = 1; + } + + memset(t, 0, sizeof(*t)); + t->c.myhost = &t->c.myhost_info; + t->c.myhost_info.name = (char*)label; + t->c.myhost_info.key = (char*)""; + + TEST_ASSERT_EQUAL_INT(0, + xce_hook_init(&t->c.hooksys, XCE_CONTROLLER_HOOK_MAX)); + + t->c.jobmgr = makeInstance(&xkvmgr_type, &int_params); + t->c.qmgr = makeInstance(&xkvmgr_type, &str_params); + t->c.workermgr = makeInstance(&xkvmgr_type, &str_params); + t->c.resmgr = makeInstance(&xkvmgr_type, &str_params); + TEST_ASSERT_NOT_NULL(t->c.jobmgr); + TEST_ASSERT_NOT_NULL(t->c.qmgr); + TEST_ASSERT_NOT_NULL(t->c.workermgr); + TEST_ASSERT_NOT_NULL(t->c.resmgr); + zarray_init(&t->c.sharedinst, 16, NULL); + + snprintf(t->tmpdir, sizeof(t->tmpdir), "/tmp/xce_%s_XXXXXX", label); + TEST_ASSERT_NOT_NULL(mkdtemp(t->tmpdir)); + + persist_config_t cfg = persist_config_defaults(); + cfg.wal_sync_ms = 5000; /* slow fsync -- no background races in tests */ + cfg.snap_keep_n = 4; + + persist_set_workdir(&t->p, t->tmpdir, label); + TEST_ASSERT_EQUAL_INT(0, persist_init(&t->p, &cfg, &t->c)); + TEST_ASSERT_EQUAL_INT(0, persist_start(&t->p)); +} + +/* Unconditional teardown -- callers must gather every observable they need + * to assert on BEFORE calling this (mirrors test_upgrade_snapshot_apply.c's + * documented longjmp-past-teardown discipline). */ +static void b857_teardown(b857_t* t) +{ + persist_stop(&t->p); + persist_fini(&t->p); + + zarray_fini(&t->c.sharedinst); + if (t->c.resmgr) dereference(t->c.resmgr); + if (t->c.workermgr) dereference(t->c.workermgr); + if (t->c.qmgr) dereference(t->c.qmgr); + if (t->c.jobmgr) dereference(t->c.jobmgr); + xce_hook_fini(&t->c.hooksys); + + char cmd[512]; + snprintf(cmd, sizeof(cmd), "rm -rf %s", t->tmpdir); + if (system(cmd) != 0) + fprintf(stderr, "cleanup warning: rm -rf %s failed\n", t->tmpdir); +} + +/* Encode a minimal, real job into a WAL_JOB_SUBMIT payload -- mirrors + * test_replay_apply_seam.c's trr_encode_job exactly (capnp_encode_job + * itself is file-static in persist.c, not reusable across translation + * units, hence this small local copy). */ +static int b857_encode_job(uint64_t jobid, const char* queue, + uint8_t** out, size_t* out_len) +{ + job_submit_data_t submit; + memset(&submit, 0, sizeof(submit)); + submit.queue = (char*)queue; + submit.user = (char*)"b857-tester"; + + xce_job_t j; + memset(&j, 0, sizeof(j)); + j.jobid = jobid; + j.state = JOB_STAT_PEND; + j.submit = &submit; + + struct capn c; + struct capn_segment* cs; + struct Job cj; + Job_ptr jp; + + capn_init_malloc(&c); + cs = capn_root(&c).seg; + encode_Job(cs, &cj, &j); + jp = new_Job(cs); + write_Job(&cj, jp); + capn_setp(capn_root(&c), 0, jp.p); + + *out_len = capn_size(&c); + *out = malloc(*out_len); + if (!*out) { capn_free(&c); return -ENOMEM; } + capn_write_mem(&c, *out, *out_len, 0); + capn_free(&c); + return 0; +} + +static bool b857_job_exists(var jobmgr, uint64_t jobid) +{ + void* existing = NULL; + lock(jobmgr, RTTI_LOCK_READ); + int found = key_find(jobmgr, (void*)jobid, &existing); + unlock(jobmgr); + return found > 0 && existing != NULL; +} + +/* Mirrors test_upgrade_snapshot_apply.c's tusa_snapshot_exists path + * construction exactly (same "snap-.capnp" naming convention the + * production checkpoint code uses). */ +static void b857_snapshot_path(persist_t* p, uint64_t lsn, char* buf, + size_t bufsz) +{ + snprintf(buf, bufsz, "%s/snap-%016" PRIx64 ".capnp", p->snap_path, lsn); +} + +/* ── Scenario 1 (Req 6.2, "reproduce ... on the pre-fix code") ──────────── */ + +static void test_bug857_threshold_checkpoint_diverges_across_members(void) +{ + b857_t A, B; + b857_setup(&A, "b857oa"); + b857_setup(&B, "b857ob"); + + uint8_t* job1_payload = NULL; size_t job1_len = 0; + int rc_enc1 = b857_encode_job(857001, "b857q", &job1_payload, &job1_len); + + uint8_t marker_sha[32]; + memset(marker_sha, 0xB8, sizeof(marker_sha)); + uint8_t marker_payload[WAL_CONFIG_MARKER_PAYLOAD_LEN]; + size_t marker_out_len = 0; + int rc_marker_enc = config_marker_encode(0x8570000000000001ULL, marker_sha, + marker_payload, + WAL_CONFIG_MARKER_PAYLOAD_LEN, + &marker_out_len); + + uint8_t* job2_payload = NULL; size_t job2_len = 0; + int rc_enc2 = b857_encode_job(857002, "b857q", &job2_payload, &job2_len); + + /* Both members physically receive the IDENTICAL committed sequence -- + * job1 @ lsn1, config-change marker @ lsn2, job2 @ lsn3 -- mirroring + * what majority replication of a real cluster guarantees. */ + int rc_wal_a1 = persist_wal_async(&A.p, WAL_JOB_SUBMIT, job1_payload, + (uint32_t)job1_len); + int rc_wal_a2 = persist_wal_async(&A.p, WAL_CONFIG_MARKER, marker_payload, + WAL_CONFIG_MARKER_PAYLOAD_LEN); + int rc_wal_a3 = persist_wal_async(&A.p, WAL_JOB_SUBMIT, job2_payload, + (uint32_t)job2_len); + + int rc_wal_b1 = persist_wal_async(&B.p, WAL_JOB_SUBMIT, job1_payload, + (uint32_t)job1_len); + int rc_wal_b2 = persist_wal_async(&B.p, WAL_CONFIG_MARKER, marker_payload, + WAL_CONFIG_MARKER_PAYLOAD_LEN); + int rc_wal_b3 = persist_wal_async(&B.p, WAL_JOB_SUBMIT, job2_payload, + (uint32_t)job2_len); + + /* Member A's own threshold trips EARLY -- right after job1, before job2 + * exists locally. Entity effect driven directly via restore_job(), the + * exact function apply_committed_entry()'s WAL_JOB_SUBMIT case itself + * calls -- the marker's LSN is never dispatched through + * apply_committed_entry() at all, so the CURRENT codebase's + * marker-anchored checkpoint (task 5) never fires here, faithfully + * reproducing the OLD, pre-task-5 decoupling: checkpoint content/timing + * driven only by schedule_checkpoint's independent WAL-record + * threshold (simulated by calling persist_checkpoint(), its real + * production entry point, directly). */ + int rc_restore_a1 = restore_job(&A.c, 0, job1_payload, job1_len, 1); + atomic_store_explicit(&A.p.last_applied_lsn, 1, memory_order_relaxed); + int rc_ckpt_a = persist_checkpoint(&A.p); + uint64_t ckpt_lsn_a = A.p.checkpoint_lsn; + + /* Member B's own threshold does not trip until LATER -- by then job2 + * was already locally visible too (ordinary cluster activity continued + * around the upgrade before B happened to checkpoint/restart). */ + int rc_restore_b1 = restore_job(&B.c, 0, job1_payload, job1_len, 1); + int rc_restore_b2 = restore_job(&B.c, 0, job2_payload, job2_len, 3); + atomic_store_explicit(&B.p.last_applied_lsn, 3, memory_order_relaxed); + int rc_ckpt_b = persist_checkpoint(&B.p); + uint64_t ckpt_lsn_b = B.p.checkpoint_lsn; + + bool job1_in_a = b857_job_exists(A.c.jobmgr, 857001); + bool job1_in_b = b857_job_exists(B.c.jobmgr, 857001); + bool job2_in_a = b857_job_exists(A.c.jobmgr, 857002); + bool job2_in_b = b857_job_exists(B.c.jobmgr, 857002); + + char snap_a[4096], snap_b[4096]; + b857_snapshot_path(&A.p, ckpt_lsn_a, snap_a, sizeof(snap_a)); + b857_snapshot_path(&B.p, ckpt_lsn_b, snap_b, sizeof(snap_b)); + + snapdiff_report_t rep; + memset(&rep, 0, sizeof(rep)); + int diff_rc = snapdiff_paths(snap_a, snap_b, NULL, &rep); + + const uint64_t marker_lsn = 2; + + b857_teardown(&A); + b857_teardown(&B); + free(job1_payload); + free(job2_payload); + + TEST_ASSERT_EQUAL_INT(0, rc_enc1); + TEST_ASSERT_EQUAL_INT(0, rc_marker_enc); + TEST_ASSERT_EQUAL_INT(0, rc_enc2); + TEST_ASSERT_EQUAL_INT(0, rc_wal_a1); + TEST_ASSERT_EQUAL_INT(0, rc_wal_a2); + TEST_ASSERT_EQUAL_INT(0, rc_wal_a3); + TEST_ASSERT_EQUAL_INT(0, rc_wal_b1); + TEST_ASSERT_EQUAL_INT(0, rc_wal_b2); + TEST_ASSERT_EQUAL_INT(0, rc_wal_b3); + TEST_ASSERT_EQUAL_INT(0, rc_restore_a1); + TEST_ASSERT_EQUAL_INT(0, rc_restore_b1); + TEST_ASSERT_EQUAL_INT(0, rc_restore_b2); + TEST_ASSERT_EQUAL_INT(0, rc_ckpt_a); + TEST_ASSERT_EQUAL_INT(0, rc_ckpt_b); + + /* Neither member's checkpoint is anchored to the marker's own lsn (2) + * -- this IS the bug: "no snapshot pinning an identical post-upgrade + * state" (bug-857's own root-cause wording). */ + TEST_ASSERT_EQUAL_UINT64(1, ckpt_lsn_a); + TEST_ASSERT_EQUAL_UINT64(3, ckpt_lsn_b); + TEST_ASSERT_NOT_EQUAL(marker_lsn, ckpt_lsn_a); + TEST_ASSERT_NOT_EQUAL(marker_lsn, ckpt_lsn_b); + + /* Real, content-level divergence -- the JOB SET each member actually + * materialized genuinely differs, not merely a cosmetic LSN-header + * mismatch. */ + TEST_ASSERT_TRUE(job1_in_a); + TEST_ASSERT_TRUE(job1_in_b); + TEST_ASSERT_FALSE_MESSAGE(job2_in_a, + "member A's checkpoint predates job2 -- must NOT contain it"); + TEST_ASSERT_TRUE_MESSAGE(job2_in_b, + "member B's checkpoint postdates job2 -- must contain it"); + + /* And the resulting ON-DISK ARTIFACTS -- what a rejoining or + * catching-up member would actually receive via snapshot transfer -- + * genuinely diverge too. */ + TEST_ASSERT_EQUAL_INT(1, diff_rc); + TEST_ASSERT_FALSE_MESSAGE(rep.identical, + "pre-fix: two members' checkpoints materialized around the SAME " + "config-change marker must NOT be byte-identical -- this is the " + "bug-857 divergence, reproduced"); + + printf("test_bug857_threshold_checkpoint_diverges_across_members: " + "PASS\n"); +} + +/* ── Scenario 2 (Req 6.2, "... and pass on the fixed code") ─────────────── */ + +static void test_bug857_marker_checkpoint_converges_across_members(void) +{ + b857_t A, B; + b857_setup(&A, "b857na"); + b857_setup(&B, "b857nb"); + + uint8_t* job1_payload = NULL; size_t job1_len = 0; + int rc_enc1 = b857_encode_job(857101, "b857q", &job1_payload, &job1_len); + + uint8_t marker_sha[32]; + memset(marker_sha, 0xC9, sizeof(marker_sha)); + uint8_t marker_payload[WAL_CONFIG_MARKER_PAYLOAD_LEN]; + size_t marker_out_len = 0; + int rc_marker_enc = config_marker_encode(0x8570000000000002ULL, marker_sha, + marker_payload, + WAL_CONFIG_MARKER_PAYLOAD_LEN, + &marker_out_len); + + uint8_t* job2_payload = NULL; size_t job2_len = 0; + int rc_enc2 = b857_encode_job(857102, "b857q", &job2_payload, &job2_len); + + /* Member A: catches up in a SINGLE pump-run call over the whole range + * [job1, marker, job2] -- as if it processed everything in one + * uninterrupted burst. */ + int rc_wal_a1 = persist_wal_async(&A.p, WAL_JOB_SUBMIT, job1_payload, + (uint32_t)job1_len); + int rc_wal_a2 = persist_wal_async(&A.p, WAL_CONFIG_MARKER, marker_payload, + WAL_CONFIG_MARKER_PAYLOAD_LEN); + int rc_wal_a3 = persist_wal_async(&A.p, WAL_JOB_SUBMIT, job2_payload, + (uint32_t)job2_len); + uint64_t committed_a = persist_committed_lsn_advance_atomic(&A.p, 3); + int applied_a = persist_apply_pump_run(&A.p); + + /* Member B: processes job1+marker FIRST (one pump-run call, simulating + * a pause/restart boundary exactly at the marker), and only later + * commits+applies job2 in a SEPARATE second pump-run call -- + * genuinely different real-world processing pace than member A. */ + int rc_wal_b1 = persist_wal_async(&B.p, WAL_JOB_SUBMIT, job1_payload, + (uint32_t)job1_len); + int rc_wal_b2 = persist_wal_async(&B.p, WAL_CONFIG_MARKER, marker_payload, + WAL_CONFIG_MARKER_PAYLOAD_LEN); + uint64_t committed_b1 = persist_committed_lsn_advance_atomic(&B.p, 2); + int applied_b1 = persist_apply_pump_run(&B.p); + + int rc_wal_b3 = persist_wal_async(&B.p, WAL_JOB_SUBMIT, job2_payload, + (uint32_t)job2_len); + uint64_t committed_b2 = persist_committed_lsn_advance_atomic(&B.p, 3); + int applied_b2 = persist_apply_pump_run(&B.p); + + const uint64_t marker_lsn = 2; + + char snap_a[4096], snap_b[4096]; + b857_snapshot_path(&A.p, marker_lsn, snap_a, sizeof(snap_a)); + b857_snapshot_path(&B.p, marker_lsn, snap_b, sizeof(snap_b)); + + bool snap_a_exists = (access(snap_a, F_OK) == 0); + bool snap_b_exists = (access(snap_b, F_OK) == 0); + + snapdiff_report_t rep; + memset(&rep, 0, sizeof(rep)); + int diff_rc = snapdiff_paths(snap_a, snap_b, NULL, &rep); + + b857_teardown(&A); + b857_teardown(&B); + free(job1_payload); + free(job2_payload); + + TEST_ASSERT_EQUAL_INT(0, rc_enc1); + TEST_ASSERT_EQUAL_INT(0, rc_marker_enc); + TEST_ASSERT_EQUAL_INT(0, rc_enc2); + TEST_ASSERT_EQUAL_INT(0, rc_wal_a1); + TEST_ASSERT_EQUAL_INT(0, rc_wal_a2); + TEST_ASSERT_EQUAL_INT(0, rc_wal_a3); + TEST_ASSERT_EQUAL_INT(0, rc_wal_b1); + TEST_ASSERT_EQUAL_INT(0, rc_wal_b2); + TEST_ASSERT_EQUAL_INT(0, rc_wal_b3); + + TEST_ASSERT_EQUAL_UINT64(3, committed_a); + TEST_ASSERT_EQUAL_INT(3, applied_a); + TEST_ASSERT_EQUAL_UINT64(2, committed_b1); + TEST_ASSERT_EQUAL_INT(2, applied_b1); + TEST_ASSERT_EQUAL_UINT64(3, committed_b2); + TEST_ASSERT_EQUAL_INT(1, applied_b2); + + TEST_ASSERT_TRUE_MESSAGE(snap_a_exists, + "member A must checkpoint at the marker's own lsn, even mid-batch " + "within a single pump-run call"); + TEST_ASSERT_TRUE_MESSAGE(snap_b_exists, + "member B must checkpoint at the marker's own lsn, even though its " + "own processing was split across two separate pump-run calls"); + + /* The decisive proof: regardless of each member's own processing pace + * (A: one burst; B: paused exactly at the marker, job2 applied later), + * both marker-anchored checkpoints are byte-identical -- convergence, + * closing the bug-857 divergence Scenario 1 reproduced above. */ + TEST_ASSERT_EQUAL_INT_MESSAGE(0, diff_rc, + "post-fix: both members' marker-anchored checkpoints must be " + "byte-identical regardless of differing processing pace"); + TEST_ASSERT_TRUE_MESSAGE(rep.identical, + "post-fix: materialized state converges identically across " + "members -- bug-857 no longer occurs"); + + printf("test_bug857_marker_checkpoint_converges_across_members: " + "PASS\n"); +} + +/* ── Dispatch ────────────────────────────────────────────────────────── */ + +void test_bug857_regression(void) +{ + UnitySetTestFile(__FILE__); + printf("\n>> bug-857 materialized-state divergence regression " + "(task 8.5)\n\n"); + + RUN_TEST(test_bug857_threshold_checkpoint_diverges_across_members); + RUN_TEST(test_bug857_marker_checkpoint_converges_across_members); +}