feat(config-upgrade-snapshot): add bug-857 regression test, correct design.md (task 8.5)
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.
This commit is contained in:
parent
1d09631df0
commit
2bd44e8e4d
5 changed files with 951 additions and 1 deletions
432
.kiro/specs/config-upgrade-snapshot/design.md
Normal file
432
.kiro/specs/config-upgrade-snapshot/design.md
Normal file
|
|
@ -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.
|
||||
|
|
@ -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.<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.
|
||||
|
||||
- **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.
|
||||
|
|
|
|||
|
|
@ -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',
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
}
|
||||
|
|
|
|||
513
src/controller/testsuite/test_bug857_regression.c
Normal file
513
src/controller/testsuite/test_bug857_regression.c
Normal file
|
|
@ -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 <rtti.h>
|
||||
|
||||
#include <capnp_c.h>
|
||||
#include <errno.h>
|
||||
#include <inttypes.h>
|
||||
#include <stdbool.h>
|
||||
#include <stdint.h>
|
||||
#include <stdio.h>
|
||||
#include <stdlib.h>
|
||||
#include <string.h>
|
||||
#include <sys/stat.h>
|
||||
#include <unistd.h>
|
||||
|
||||
#include <unity/unity.h>
|
||||
|
||||
#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-<lsn16>.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);
|
||||
}
|
||||
Loading…
Add table
Add a link
Reference in a new issue