forked from lei.chen/myxce
feat(ha-entity-snapshots): add spec + task 1.1 test fixture
Adds the ha-entity-snapshots Kiro spec (requirements, design, research, tasks) for task 5.10a / raft-ha-core-safety: making the HA checkpoint an actually-restorable copy of committed job/queue state instead of a position-only sentinel, closing the residual chaotic-soak divergence gap. Task 1.1 (foundation): reusable test fixture snapent_make/snapent_teardown in a new test_persist_snapshot_entities.c, seeding N jobs (varying state) and M queues (varying status) into a real persist_t+cluster_t for later write/load/rotation-guard tests to build on. Build clean; new suite 1/1; regression spot-checks (test_persist_hooks 19/19, test_commit_barrier_reply 22/22) clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JDYFbrgyDHbvNYcxApKhrX
This commit is contained in:
parent
0d70d5e412
commit
10f9bbfd9d
8 changed files with 1253 additions and 1 deletions
526
.kiro/specs/ha-entity-snapshots/design.md
Normal file
526
.kiro/specs/ha-entity-snapshots/design.md
Normal file
|
|
@ -0,0 +1,526 @@
|
|||
# Design Document: ha-entity-snapshots
|
||||
|
||||
## Overview
|
||||
|
||||
**Purpose**: This feature makes the XCE controller's periodic persistence checkpoint an
|
||||
actually-restorable copy of committed job and queue state, closing the permanent data gap a
|
||||
rejoining or far-behind cluster member hits today (task 5.10a / raft-ha-core-safety; bug-795/
|
||||
828/851 family). Today `write_snapshot` records only a position marker; `load_snapshot` restores
|
||||
nothing. This design adds the missing entity payload and restore step, and closes the one real
|
||||
retention hazard it introduces (a snapshot file being rotated out from under an in-flight
|
||||
transfer).
|
||||
|
||||
**Users**: The XCE HA controller cluster itself is the direct consumer — every member that
|
||||
rejoins empty or falls behind the retained WAL history. The observable beneficiary is the
|
||||
cluster operator, who currently sees a silently incomplete member after such a rejoin.
|
||||
|
||||
**Impact**: Modifies the existing checkpoint write/read/rotate logic in `persist.c` and adds one
|
||||
small tracking array in `ha.c`. No wire protocol change, no new external dependency, no new
|
||||
capnp schema struct — the existing `Job` schema and its generated codec already carry everything
|
||||
needed for jobs. Queue *definitions* (name, attrs, priority, thresholds, ACLs) are config-
|
||||
provisioned and already replicated independently via `push_config_to_standbys`
|
||||
(`config_repl.c`) — out of this spec's boundary. Only a queue's runtime admin status
|
||||
(open/closed/active/inactive), the one piece `WAL_QUEUE_CTRL` tracks and config replication does
|
||||
not carry, is this spec's concern — restored via the existing `restore_queue_state(name, status)`,
|
||||
no capnp involved on the queue side at all.
|
||||
|
||||
### Goals
|
||||
- A checkpoint written at LSN X contains every live/retained job and queue record as of X.
|
||||
- Loading that checkpoint reconstructs those records before the member's position advances.
|
||||
- History is never discarded before it is captured in a checkpoint whose entities are durable.
|
||||
- A member that rejoins via checkpoint ends up bit-for-bit consistent with the rest of the
|
||||
cluster — no permanent gap.
|
||||
- A checkpoint produced by a prior version of the binary never crashes or corrupts a member
|
||||
that reads it.
|
||||
|
||||
### Non-Goals
|
||||
- Lock-free, copy-on-write entity enumeration (zarray COW exists but is not wired into `kvmgr`;
|
||||
reviving that wiring is a separate, larger effort — see `research.md` Option B).
|
||||
- Any change to the wire transport/chunking protocol that ships a snapshot to a peer.
|
||||
- Any change to the catch-up decision logic (`ha_evaluate_repl_action`) that chooses between a
|
||||
snapshot transfer and incremental WAL streaming.
|
||||
- Worker/compute-node state, cluster configuration document snapshotting, or election/term-safety
|
||||
behavior.
|
||||
- Queue *definition* fields (name, attrs, priority, thresholds, windows, ACLs) — these are
|
||||
config-provisioned and already replicated independently (`push_config_to_standbys`); this spec
|
||||
snapshots only the queue's runtime admin status.
|
||||
|
||||
## Boundary Commitments
|
||||
|
||||
### This Spec Owns
|
||||
- The on-disk content format of a checkpoint (entity payload framing) and the write/read logic
|
||||
that produces and consumes it (`write_snapshot`, `load_snapshot` internals).
|
||||
- Point-in-time collection and encoding of live job and queue entities at checkpoint time.
|
||||
- The entity-restore step on load and gating the member's committed-position advance on that
|
||||
step's success.
|
||||
- Distinguishing a legacy (pre-feature) position-only checkpoint from an entity-bearing one.
|
||||
- Protecting an in-flight-transferring checkpoint file from premature rotation deletion.
|
||||
|
||||
### Out of Boundary
|
||||
- The wire opcode and chunk-reassembly protocol that delivers a checkpoint to a peer
|
||||
(`OPCode_snapshotTransfer`, `ha_handle_snapshot_chunk`'s framing) — reused unchanged.
|
||||
- The decision of *when* a member needs a checkpoint vs. incremental streaming
|
||||
(`ha_evaluate_repl_action`) — unchanged.
|
||||
- The WAL-segment trim floor's standby-acked-LSN computation (`repl_trim_lsn`) — unchanged; see
|
||||
Architecture below for why no new floor input is needed there.
|
||||
- Worker/join state, config-document snapshotting (`config-upgrade-snapshot`), election and
|
||||
term-safety behavior (`raft-ha-core-safety`, `ha-term-safety`).
|
||||
|
||||
### Allowed Dependencies
|
||||
- Existing capnp codegen for `Job` (`encode_Job`/`write_Job`/`decode_Job`, already generated from
|
||||
`src/schema/controller.capnp`). Queue entries need no capnp — `restore_queue_state`'s contract
|
||||
is a plain `(name, status)` pair (see Architecture below).
|
||||
- `restore_job` (`job.c:96`) and `restore_queue_state` (`queue.c:208`) — existing, unmodified
|
||||
entity-restore functions already used by WAL replay.
|
||||
- The `collect_jobs`/`collect_queues` enumeration pattern (`dataex.c`) and the
|
||||
`job_should_retain`/`worker_should_retain` retention predicates (`persist.c`).
|
||||
- The existing tmp-file-then-atomic-rename pattern already used by `write_snapshot` and by
|
||||
`ha_handle_snapshot_chunk`'s `snap.part` assembly.
|
||||
- The existing per-peer `ha->snapshot_inflight[]` array shape (`ha.h`) as the pattern for the new
|
||||
in-flight-LSN tracking.
|
||||
|
||||
### Revalidation Triggers
|
||||
- Any change to the `Job` capnp schema shape — re-verify encode/decode symmetry and compatibility
|
||||
with `restore_job`.
|
||||
- Any change to `restore_queue_state`'s `(name, status)` contract, or any future need to snapshot
|
||||
more than status for queues — re-verify the queue-side format and the config-vs-WAL boundary
|
||||
this design draws.
|
||||
- Any change to `collect_jobs`/`collect_queues` locking semantics, or to the per-entity
|
||||
`RTTI_LOCK_READ`/`WRITE` discipline (e.g. if `kvmgr` COW is wired later) — re-verify the
|
||||
point-in-time consistency argument in Architecture below.
|
||||
- Any change to `persist_checkpoint`'s `write_snapshot` → `trim_wal_segments` sequencing
|
||||
(currently sequential, trim skipped on write failure) — re-verify Requirement 3.
|
||||
- Any change to `ha_handle_snapshot_chunk`'s ordering (currently: `applied_lsn` only advances
|
||||
after `persist_load_snapshot` returns success) — re-verify Requirement 4.
|
||||
|
||||
## Architecture
|
||||
|
||||
### Existing Architecture Analysis
|
||||
|
||||
- `persist_checkpoint` (persist.c:1763) already sequences correctly for Requirement 3: it calls
|
||||
`write_snapshot`, and only calls `trim_wal_segments` if `write_snapshot` returned 0
|
||||
(`if (rc != 0) goto done;`, skipping both trim and `rotate_snapshots`). **This means once
|
||||
`write_snapshot` only returns 0 when the entity payload is fully and durably written, the
|
||||
existing trim gating already satisfies "retention never outruns restorability" — no new floor
|
||||
variable is needed in `trim_wal_segments`/`repl_trim_lsn`.** This is a refinement over the
|
||||
initial gap analysis (`research.md`), which assumed a new trim-floor input was needed.
|
||||
- The one retention hazard `write_snapshot`'s success does *not* already guard is
|
||||
`rotate_snapshots` (persist.c:1672): it keeps only `snap_keep_n` newest snapshot *files* by
|
||||
filename sort and deletes the rest, with no awareness of an in-flight transfer. If a checkpoint
|
||||
cadence faster than a slow transfer rotates out the file a standby is still reading from a
|
||||
chunked wire transfer, that transfer breaks. This is the one genuinely new guard this design
|
||||
adds.
|
||||
- `ha_handle_snapshot_chunk` (ha.c:2529-2609) already has the right shape for Requirement 4: it
|
||||
only advances `ha->applied_lsn` after `persist_load_snapshot` returns success. Once
|
||||
`persist_load_snapshot`/`load_snapshot` correctly gate their own success on entity restoration
|
||||
(Requirement 2), this call site needs no change.
|
||||
- `collect_jobs`/`collect_queues` (dataex.c) already hold `RTTI_LOCK_READ` on `c->jobmgr`/
|
||||
`c->qmgr` for the duration of their walk. **This table lock is a distinct lock from the
|
||||
per-entity lock** — verified via `restore_queue_state` (queue.c:208), which takes
|
||||
`lock(q, RTTI_LOCK_WRITE)` on the individual `queue_t*` after already releasing the table lock
|
||||
from its own `key_find`, and via `job.c`'s state-transition sites (e.g. lines 1006, 1093, 1137),
|
||||
which take `lock(j, RTTI_LOCK_WRITE)` on the individual `job_t*`. The table lock only excludes
|
||||
concurrent insert/delete of table entries; it does **not** exclude a concurrent per-entity
|
||||
writer mutating an entity's fields mid-walk. Requirement 1.2's point-in-time consistency
|
||||
therefore requires the entity-collection helper to additionally take each entity's own
|
||||
`RTTI_LOCK_READ` (mirroring `restore_queue_state`'s own per-entity write-lock pattern) around
|
||||
that entity's individual encode call, nested inside the table-level walk — this excludes a
|
||||
concurrent per-entity writer for exactly the duration of that one entity's encode, without
|
||||
needing the (currently unwired) zarray COW mechanism.
|
||||
|
||||
### Architecture Pattern & Boundary Map
|
||||
|
||||
```mermaid
|
||||
flowchart TB
|
||||
subgraph Primary["Primary — checkpoint write path"]
|
||||
PC[persist_checkpoint] --> WS[write_snapshot]
|
||||
WS --> CJ["collect+encode jobs\n(table READ lock + per-job READ lock)"]
|
||||
WS --> CQ["collect name+status pairs\n(table READ lock + per-queue READ lock,\nno capnp)"]
|
||||
CJ --> TMP[tmp file + fdatasync + rename]
|
||||
CQ --> TMP
|
||||
TMP -->|rc==0| TRIM[trim_wal_segments]
|
||||
TMP -->|rc==0| ROT["rotate_snapshots\n(skips in-flight LSNs)"]
|
||||
TMP -->|rc!=0| SKIP["skip trim + rotate\n(existing goto done)"]
|
||||
end
|
||||
|
||||
subgraph Send["Snapshot transfer (unchanged)"]
|
||||
DISPATCH[ha_snapshot_send_dispatch] -->|sets snapshot_inflight_lsn for peer| WIRE[OPCode_snapshotTransfer chunks]
|
||||
WIRE --> DONE[clears snapshot_inflight_lsn on completion]
|
||||
end
|
||||
|
||||
ROT -.reads.-> DISPATCH
|
||||
|
||||
subgraph Standby["Standby — catch-up receive path"]
|
||||
RECV[ha_handle_snapshot_chunk] --> RENAME[assemble + rename final file]
|
||||
RENAME --> LS[persist_load_snapshot]
|
||||
LS --> RJ["restore_job per entity"]
|
||||
LS --> RQ["restore_queue_state per entity"]
|
||||
RJ -->|all succeed| ADV["advance current_lsn / checkpoint_lsn"]
|
||||
RQ -->|all succeed| ADV
|
||||
RJ -->|any fails| FAIL["return error, position unchanged"]
|
||||
ADV --> APPLIED["ha->applied_lsn advances\n(existing, unchanged)"]
|
||||
end
|
||||
```
|
||||
|
||||
**Architecture Integration**:
|
||||
- Selected pattern: extend the existing checkpoint write/read pipeline in place (Option A from
|
||||
`research.md`); no new subsystem, no new wire message.
|
||||
- Domain boundaries: entity content and restoration stay inside `persist.c`
|
||||
(write/load/rotate); the one cross-file touch is the new `snapshot_inflight_lsn[]` array in
|
||||
`ha.h`/`ha.c`, read by `rotate_snapshots` and written by the existing send-dispatch path.
|
||||
- Existing patterns preserved: tmp-file-then-rename atomicity, `RTTI_LOCK_READ`/`WRITE` table AND
|
||||
per-entity locking (mirroring `restore_queue_state`'s own nested-lock pattern), the
|
||||
`collect_jobs`/`collect_queues` iteration shape, the `capnp_encode_job` wrapper shape.
|
||||
- New components rationale: a small entity-frame reader/writer pair (mirrors the existing WAL
|
||||
entry length-prefixed framing style, so no new capnp schema struct or codegen is required —
|
||||
jobs use the existing `Job` capnp codec, queues use a plain fixed `(name, status)` record since
|
||||
that is the entirety of `restore_queue_state`'s contract); the in-flight-LSN guard in
|
||||
`rotate_snapshots`.
|
||||
- Steering compliance: no `fork()` used for the snapshot (per `product.md`'s "No fork() for
|
||||
snapshots" constraint) — consistency comes from the table read-lock, not process duplication.
|
||||
|
||||
### Technology Stack
|
||||
|
||||
| Layer | Choice / Version | Role in Feature | Notes |
|
||||
|-------|------------------|-----------------|-------|
|
||||
| Serialization | Cap'n Proto (existing project-wide IDL) | Per-job `Job` encode/decode; queues use a plain fixed record (no capnp) | Codegen already exists; no schema change |
|
||||
| Persistence | Flat file, tmp+rename (existing pattern) | Checkpoint file on disk | Same pattern as today's sentinel-only file |
|
||||
| Concurrency | `RTTI_LOCK_READ`/`WRITE`, table AND per-entity (existing) | Point-in-time entity consistency during collection | zarray COW deferred (Non-Goal) |
|
||||
| Transport | `OPCode_snapshotTransfer` (existing, unmodified) | Ships the (now larger) file to a standby | Out of boundary |
|
||||
|
||||
## File Structure Plan
|
||||
|
||||
### Modified Files
|
||||
- `src/controller/persist.c` — `write_snapshot`: append entity payload after the existing
|
||||
header — job frames (capnp `Job`) and queue frames (plain `(name, status)` records, no capnp).
|
||||
`load_snapshot`: restore entities before advancing `current_lsn`/`checkpoint_lsn`; detect
|
||||
legacy (position-only) files by remaining file size. `rotate_snapshots`: skip deleting any file
|
||||
whose LSN is currently in-flight. New static helpers: a snapshot entity-frame writer/reader
|
||||
pair, and an entity-collection helper built on `collect_jobs`/`collect_queues` that additionally
|
||||
takes each entity's own per-entity `RTTI_LOCK_READ` around its individual encode/collect call
|
||||
(see Architecture).
|
||||
- `src/include/persist.h` — extend `persist_load_snapshot`'s signature with an out-parameter
|
||||
reporting whether entities were restored (see Components below); no other public surface
|
||||
changes.
|
||||
- `src/controller/ha.h` — add `_Atomic uint64_t snapshot_inflight_lsn[MAX_HA_CANDIDATES]` next
|
||||
to the existing `snapshot_inflight[MAX_HA_CANDIDATES]` array.
|
||||
- `src/controller/ha.c` — `ha_snapshot_send_dispatch`/`ha_snapshot_do_send`: record the LSN being
|
||||
sent into `snapshot_inflight_lsn[peer_idx]` alongside the existing flag, clear it on completion
|
||||
(same lifecycle as the existing flag). `ha_handle_snapshot_chunk`: read the new
|
||||
`persist_load_snapshot` out-parameter; log (not fail) when entities were not restored, since
|
||||
the caller-side catch-up-completeness signal (Requirement 4.3) is carried by existing
|
||||
status/comparison tooling, not a new error path here.
|
||||
|
||||
### New Test Files
|
||||
- `src/controller/testsuite/test_persist_snapshot_entities.c` — unit coverage for
|
||||
`write_snapshot`/`load_snapshot` entity round-trip, partial-failure discard, legacy-file
|
||||
handling, and the `rotate_snapshots` in-flight guard (see Testing Strategy).
|
||||
|
||||
> No new production files. This stays entirely inside the existing checkpoint machinery's file
|
||||
> set plus one small, symmetric addition in `ha.h`/`ha.c` for the rotation guard.
|
||||
|
||||
## System Flows
|
||||
|
||||
See the Architecture Pattern & Boundary Map diagram above for the write/send/receive flow. One
|
||||
additional sequence clarifies the failure-ordering contract that Requirement 2.3 depends on:
|
||||
|
||||
```mermaid
|
||||
sequenceDiagram
|
||||
participant Standby
|
||||
participant persist_load_snapshot
|
||||
participant restore_job
|
||||
participant restore_queue_state
|
||||
|
||||
Standby->>persist_load_snapshot: load(path, lsn)
|
||||
persist_load_snapshot->>persist_load_snapshot: read header, detect format
|
||||
alt legacy (position-only) file
|
||||
persist_load_snapshot-->>Standby: rc=0, entities_restored=false
|
||||
else entity-bearing file
|
||||
loop each job frame
|
||||
persist_load_snapshot->>restore_job: decode + restore
|
||||
restore_job-->>persist_load_snapshot: rc
|
||||
alt rc != 0
|
||||
persist_load_snapshot-->>Standby: rc=error, position UNCHANGED
|
||||
end
|
||||
end
|
||||
loop each queue frame
|
||||
persist_load_snapshot->>restore_queue_state: restore(name, status)
|
||||
restore_queue_state-->>persist_load_snapshot: rc
|
||||
alt rc != 0
|
||||
persist_load_snapshot-->>Standby: rc=error, position UNCHANGED
|
||||
end
|
||||
end
|
||||
persist_load_snapshot->>persist_load_snapshot: advance current_lsn/checkpoint_lsn
|
||||
persist_load_snapshot-->>Standby: rc=0, entities_restored=true
|
||||
end
|
||||
```
|
||||
|
||||
## Requirements Traceability
|
||||
|
||||
| Requirement | Summary | Components | Interfaces | Flows |
|
||||
|---|---|---|---|---|
|
||||
| 1.1 | Checkpoint includes every live/retained job+queue record | `write_snapshot`, entity-collection helper | `collect_jobs`/`collect_queues` + `job_should_retain`; queue side is `(name, status)` only (definition is config-owned) | Write path (diagram 1) |
|
||||
| 1.2 | Internally consistent point-in-time view | Entity-collection helper | Table `RTTI_LOCK_READ` + per-entity `RTTI_LOCK_READ` nested around each encode | Architecture: Existing Architecture Analysis |
|
||||
| 1.3 | Discard incomplete checkpoint | `write_snapshot` | Existing tmp+rename pattern | Write path (diagram 1) |
|
||||
| 1.4 | Record committed position | `write_snapshot` (existing header, unchanged) | — | — |
|
||||
| 2.1/2.2 | Restore entities before advancing position | `load_snapshot` | `restore_job`, `restore_queue_state` | Sequence diagram |
|
||||
| 2.3 | Failed reconstruction must not advance position | `load_snapshot` | rc propagation from `restore_job`/`restore_queue_state` | Sequence diagram |
|
||||
| 2.4 | Legacy checkpoint treated as insufficient | `load_snapshot`, `persist_load_snapshot` | New `entities_restored` out-parameter | Sequence diagram |
|
||||
| 3.1/3.2/3.3 | Retention never outruns restorability | `persist_checkpoint` (existing sequencing, unchanged) | `write_snapshot` rc gates `trim_wal_segments`/`rotate_snapshots` | Write path (diagram 1) |
|
||||
| 4.1/4.2 | End-to-end catch-up correctness | `ha_handle_snapshot_chunk` (existing, unchanged ordering) | `persist_load_snapshot` | Receive path (diagram 1) |
|
||||
| 4.3 | Catch-up completeness observable | Existing `walcompare`/status tooling (out of boundary; confirmed compatible) | — | — |
|
||||
| 5.1/5.2 | Safe rollout against old checkpoints | `load_snapshot` format discriminator | File-size-based legacy detection | Sequence diagram |
|
||||
|
||||
## Components and Interfaces
|
||||
|
||||
| Component | Domain/Layer | Intent | Req Coverage | Key Dependencies (P0/P1) | Contracts |
|
||||
|---|---|---|---|---|---|
|
||||
| `write_snapshot` (extended) | persist.c | Serialize header + live entities to a durable checkpoint file | 1.1-1.4, 3.1-3.3 | `collect_jobs`/`collect_queues` (P0), `capnp_encode_job` (P0) | State |
|
||||
| `load_snapshot` (extended) | persist.c | Deserialize and restore entities; gate position advance on success | 2.1-2.4, 5.1-5.2 | `restore_job`, `restore_queue_state` (P0) | State |
|
||||
| `rotate_snapshots` (extended) | persist.c | Delete old checkpoint files, skipping any in-flight LSN | 3.1-3.3 | `ha->snapshot_inflight_lsn[]` (P0) | State |
|
||||
| `snapshot_inflight_lsn[]` (new) | ha.h / ha.c | Track which checkpoint LSN each in-flight peer transfer references | 3.1-3.3 | Existing `snapshot_inflight[]` lifecycle (P0) | State |
|
||||
|
||||
### Persistence Layer
|
||||
|
||||
#### `write_snapshot` (extended)
|
||||
|
||||
| Field | Detail |
|
||||
|-------|--------|
|
||||
| Intent | Produce a durable, entity-bearing checkpoint file at `checkpoint_lsn` |
|
||||
| Requirements | 1.1, 1.2, 1.3, 1.4, 3.1, 3.2, 3.3 |
|
||||
|
||||
**Responsibilities & Constraints**
|
||||
- Writes the existing legacy header unchanged (magic, `checkpoint_lsn`, optional config-ref).
|
||||
- Appends, in order: a format-version marker, a job count and per-job length-prefixed capnp `Job`
|
||||
frames, a queue count and per-queue fixed `(name, status)` frames (no capnp — matches
|
||||
`restore_queue_state`'s exact contract; queue definition is config-owned, out of boundary).
|
||||
- Collects under `RTTI_LOCK_READ` on `c->jobmgr` (released before the queue pass begins); collects
|
||||
under `RTTI_LOCK_READ` on `c->qmgr` for queues. The two table locks are never held
|
||||
simultaneously (no new lock-ordering surface). **Within each table's held READ lock, also takes
|
||||
each individual entity's own `RTTI_LOCK_READ`** (`lock(j, RTTI_LOCK_READ)` /
|
||||
`lock(q, RTTI_LOCK_READ)`) around that one entity's encode/read call — this is required for
|
||||
Requirement 1.2: the table lock alone does not exclude a concurrent per-entity writer (see
|
||||
Architecture).
|
||||
- Filters via the existing `job_should_retain` predicate so the checkpoint's notion of "live"
|
||||
matches the existing GC retention policy (Requirement 1.1). Every queue present in `c->qmgr` is
|
||||
included (a queue's mere table presence already means config says it exists; no retain filter
|
||||
needed for a plain status word).
|
||||
- Writes to the existing `.tmp` path, `fdatasync`s, then atomically renames to the final path —
|
||||
identical pattern to today, just with more bytes. Any failure at any step (encode, write,
|
||||
sync) leaves no file at the final path (Requirement 1.3); the tmp file is unlinked.
|
||||
|
||||
**Contracts**: State [x]
|
||||
|
||||
##### State Management
|
||||
- State model: file on disk, atomically visible only at the final path once fully written.
|
||||
- Persistence & consistency: `fdatasync` before rename, matching the existing sentinel-only
|
||||
behavior — durability semantics are unchanged, only the payload grows.
|
||||
- Concurrency strategy: two sequential table-read-lock sections (jobs, then queues), each with a
|
||||
nested per-entity read lock around the individual encode/read call; no lock is held during the
|
||||
disk write itself.
|
||||
|
||||
**Implementation Notes**
|
||||
- Integration: called from `persist_checkpoint`, unchanged call site.
|
||||
- Validation: unit test asserts a checkpoint containing N jobs + M queues round-trips through
|
||||
`load_snapshot` with all N+M records field-identical.
|
||||
- Risks: enumeration+encode duration under `RTTI_LOCK_READ` blocks concurrent job/queue mutation
|
||||
for that duration. Mitigated by checkpoints already running off the hot path on a configurable
|
||||
`snap_interval_s` cadence; measure actual duration at a realistic job-table size during
|
||||
implementation (see `research.md` Research Needed).
|
||||
|
||||
#### `load_snapshot` (extended)
|
||||
|
||||
| Field | Detail |
|
||||
|-------|--------|
|
||||
| Intent | Reconstruct entities from a checkpoint file and gate position advance on success |
|
||||
| Requirements | 2.1, 2.2, 2.3, 2.4, 5.1, 5.2 |
|
||||
|
||||
**Responsibilities & Constraints**
|
||||
- Reads the existing legacy header exactly as today (unchanged).
|
||||
- Determines format: if no bytes remain in the file after the legacy header, this is a
|
||||
pre-feature checkpoint — do not advance `current_lsn`/`checkpoint_lsn` as "fully caught up";
|
||||
return success with `entities_restored=false` so the caller falls back to normal catch-up
|
||||
(Requirement 2.4). This never crashes or misinterprets an old file (Requirement 5.1, 5.2) — a
|
||||
short file is a valid, expected legacy shape, not a corruption.
|
||||
- If entity frames are present, decode each job frame (capnp) and call `restore_job`; read each
|
||||
queue frame's plain `(name, status)` pair and call `restore_queue_state(name, status)` directly
|
||||
— in file order. On any decode or restore failure, abort immediately: return an error, leave
|
||||
`current_lsn`/`checkpoint_lsn` unchanged (Requirement 2.3).
|
||||
- Only after every entity restores successfully does the function advance
|
||||
`current_lsn`/`checkpoint_lsn` to the checkpoint's position (Requirement 2.1, 2.2).
|
||||
|
||||
**Contracts**: State [x]
|
||||
|
||||
##### State Management
|
||||
- State model: in-memory job/queue tables (`c->jobmgr`/`c->qmgr`), mutated in place by the
|
||||
existing `restore_job`/`restore_queue_state` (unmodified — same functions WAL replay already
|
||||
calls, so no new restore code path exists to diverge from replay's behavior).
|
||||
- Persistence & consistency: `current_lsn`/`checkpoint_lsn` advance is the single commit point
|
||||
for "this checkpoint is now fully applied" — atomic in the sense that no partial-entity state
|
||||
is ever exposed as "caught up."
|
||||
- Concurrency strategy: runs on the receiving member during catch-up, before it accepts write
|
||||
traffic for those entities — no concurrent mutation expected at this point in the lifecycle
|
||||
(same assumption WAL replay already makes at startup).
|
||||
|
||||
**Public signature change** (`src/include/persist.h`):
|
||||
```c
|
||||
int persist_load_snapshot(persist_t* p, const char* path, uint64_t lsn,
|
||||
bool* out_entities_restored);
|
||||
```
|
||||
- Preconditions: `path` names a file written by `write_snapshot` (legacy or current format).
|
||||
- Postconditions: on rc==0, `*out_entities_restored` reports whether this call actually restored
|
||||
entity state (`true`) or only observed a legacy position marker (`false`); `current_lsn`/
|
||||
`checkpoint_lsn` reflect the checkpoint's position only when entities were fully restored or
|
||||
the file was legacy-format (position-only, as today). On rc!=0, position is unchanged.
|
||||
- Invariants: `*out_entities_restored` is only set when rc==0.
|
||||
|
||||
**Implementation Notes**
|
||||
- Integration: `ha_handle_snapshot_chunk` passes the new out-parameter through; on
|
||||
`entities_restored==false` it logs (does not fail) since a legacy file is an expected rollout
|
||||
state, not an error — the member simply continues through the existing incremental catch-up
|
||||
path for the remaining history.
|
||||
- Validation: unit test with a deliberately truncated/corrupted entity frame asserts
|
||||
`current_lsn` unchanged and rc!=0; unit test with a legacy-sized file asserts rc==0 and
|
||||
`entities_restored==false`.
|
||||
|
||||
#### `rotate_snapshots` (extended)
|
||||
|
||||
| Field | Detail |
|
||||
|-------|--------|
|
||||
| Intent | Delete old checkpoint files without breaking an in-flight transfer |
|
||||
| Requirements | 3.1, 3.2, 3.3 |
|
||||
|
||||
**Responsibilities & Constraints**
|
||||
- Unchanged selection logic (keep `snap_keep_n` newest by filename/LSN).
|
||||
- Before deleting a candidate file, checks whether its LSN matches any entry in
|
||||
`ha->snapshot_inflight_lsn[]` (via `p->cluster->ha`, already reachable from `persist_t`); if so,
|
||||
skip deletion for this rotation pass and retry on the next checkpoint cycle.
|
||||
|
||||
**Contracts**: State [x]
|
||||
|
||||
##### State Management
|
||||
- State model: reads `ha_state_t`'s new array; does not itself own it (owned by the send-dispatch
|
||||
path, see below).
|
||||
|
||||
**Implementation Notes**
|
||||
- Integration: minimal — one lookup added to the existing deletion loop.
|
||||
- Validation: unit test simulates an in-flight LSN matching an otherwise-rotatable file and
|
||||
asserts it survives rotation.
|
||||
|
||||
### HA Replication Layer
|
||||
|
||||
#### `snapshot_inflight_lsn[]` (new array)
|
||||
|
||||
| Field | Detail |
|
||||
|-------|--------|
|
||||
| Intent | Record which checkpoint LSN each in-flight peer snapshot transfer references |
|
||||
| Requirements | 3.1, 3.2, 3.3 |
|
||||
|
||||
**Responsibilities & Constraints**
|
||||
- `_Atomic uint64_t snapshot_inflight_lsn[MAX_HA_CANDIDATES]` in `ha_state_t` (ha.h), declared
|
||||
next to the existing `_Atomic int snapshot_inflight[MAX_HA_CANDIDATES]`.
|
||||
- Set by `ha_snapshot_send_dispatch`/`ha_snapshot_do_send` at the same point the existing
|
||||
`snapshot_inflight[peer_idx]` flag is set to 1 (before dispatch); cleared at the same points
|
||||
the existing flag is cleared to 0 (on completion or CAS-release failure) — identical lifecycle,
|
||||
one more field.
|
||||
|
||||
**Contracts**: State [x]
|
||||
|
||||
##### State Management
|
||||
- State model: plain atomic array, same shape and lifecycle as the existing flag array it sits
|
||||
beside.
|
||||
- Concurrency strategy: no new locking — atomics only, matching the existing flag's discipline.
|
||||
|
||||
**Implementation Notes**
|
||||
- Integration: two write sites (set/clear), both already-existing call sites in `ha.c`.
|
||||
- Validation: covered by the `rotate_snapshots` guard test above (integration-level: dispatch a
|
||||
send, assert its LSN is protected, complete it, assert protection is released).
|
||||
|
||||
## Data Models
|
||||
|
||||
### Checkpoint File Layout (Physical)
|
||||
|
||||
```
|
||||
[existing legacy header — unchanged, 16 or 57 bytes depending on config-ref presence]
|
||||
[uint32_t format_version] -- absent (EOF here) => legacy, position-only file
|
||||
[uint32_t job_count]
|
||||
[job_count * { uint32_t payload_len; uint8_t capnp_Job_bytes[payload_len] }]
|
||||
[uint32_t queue_count]
|
||||
[queue_count * { uint32_t name_len; char name[name_len]; uint32_t status }]
|
||||
```
|
||||
|
||||
- Job frames are length-prefixed capnp `Job` messages, matching the framing style already used
|
||||
for WAL entries (`wal_entry_hdr_t` + payload) — no new schema/codegen for a one-off container.
|
||||
- Queue frames carry only what `restore_queue_state` can consume: a name and a status word — no
|
||||
capnp at all. A queue's *definition* (attrs, priority, thresholds, ACLs) is config-provisioned
|
||||
and already replicated via `push_config_to_standbys`; encoding it here would duplicate data this
|
||||
spec does not own and has no restore path for.
|
||||
- A file ending exactly at the legacy header (no bytes remain) is unambiguously the old format —
|
||||
the discriminator is "does more data exist," not a flag bit, so old files need zero
|
||||
reinterpretation.
|
||||
- `format_version` is present for forward evolution (a future layout bumps this value and
|
||||
`load_snapshot` dispatches on it) without breaking this design's readers.
|
||||
|
||||
### Consistency & Integrity
|
||||
- Per-entity encode happens while both the enumerating table's `RTTI_LOCK_READ` AND that entity's
|
||||
own nested `RTTI_LOCK_READ` are held (see Architecture) — the table lock alone is insufficient,
|
||||
since entity mutation takes a separate per-entity lock; the nested per-entity lock is what
|
||||
excludes a concurrent writer for that entity's encode, giving each entity a single consistent
|
||||
snapshot of its own fields with no entity encoded mid-mutation.
|
||||
- Cross-entity consistency (jobs relative to queues) is not required by Requirement 1.2, which
|
||||
is scoped to "no record split between its own pre/post-checkpoint value" — jobs and queues are
|
||||
independent aggregates for this feature's purposes, consistent with `collect_jobs`/
|
||||
`collect_queues` already treating them as separately-locked tables today.
|
||||
|
||||
## Error Handling
|
||||
|
||||
### Error Strategy
|
||||
- **Write-side failure** (encode error, disk I/O error) at any point during `write_snapshot`:
|
||||
unlink the tmp file, return a non-zero rc, which the existing `persist_checkpoint` caller
|
||||
already treats as "skip trim and rotate this cycle" — no operational-condition self-termination
|
||||
(per `src/controller/CLAUDE.md`'s controller-never-self-terminates invariant); the next
|
||||
checkpoint cycle simply retries.
|
||||
- **Read-side failure** (corrupt/truncated entity frame, decode error, restore error) during
|
||||
`load_snapshot`: return a non-zero rc with position unchanged; the caller
|
||||
(`ha_handle_snapshot_chunk`) logs the failure and does not advance `applied_lsn` — the member
|
||||
stays fenced/behind and the existing catch-up retry mechanics (unchanged by this spec) bring it
|
||||
current on a subsequent attempt. Consistent with the project's fail-loud-and-stay-alive
|
||||
philosophy — never a self-terminate on this operational condition.
|
||||
- **Legacy file encountered**: not an error. `entities_restored=false` with rc==0 is the expected,
|
||||
handled outcome for a file predating this feature.
|
||||
|
||||
### Monitoring
|
||||
- Existing `WARN`/`ERROR` log lines at each new failure point (matching the codebase's existing
|
||||
logging conventions in `persist.c`/`ha.c`); no new metric is introduced by this design, but
|
||||
Requirement 4.3's "observable to an operator" is satisfied by the existing `walcompare`/
|
||||
`waldump` tooling already used this session to diagnose divergence — confirm during
|
||||
implementation that it reports on entity-snapshot-derived state, not only WAL-visible LSNs.
|
||||
|
||||
## Testing Strategy
|
||||
|
||||
- **Unit — `write_snapshot`/`load_snapshot` round-trip**: a checkpoint with N jobs and M queues,
|
||||
loaded back, yields N job records field-identical to the originals and M queue statuses
|
||||
correctly reapplied via `restore_queue_state` (Requirement 1.1, 1.2, 2.1, 2.2).
|
||||
- **Unit — queue status frame round-trip**: a queue's `(name, status)` pair written and read back
|
||||
matches exactly, and `restore_queue_state` is invoked with those exact values (Requirement 1.1).
|
||||
- **Unit — concurrent-mutation-during-snapshot consistency**: a job actively transitioning state
|
||||
(e.g. via `mark_job_start`) on a concurrent thread while `write_snapshot`'s enumeration walks it
|
||||
must be captured as either the full pre- or full post-transition value, never a torn mix of
|
||||
fields — proves the nested per-entity lock, not just the table lock, is doing the work
|
||||
(Requirement 1.2).
|
||||
- **Unit — partial write discarded**: a simulated encode/write failure partway through leaves no
|
||||
file at the final path (Requirement 1.3).
|
||||
- **Unit — partial restore does not advance position**: a deliberately corrupted entity frame
|
||||
causes `load_snapshot` to return an error with `current_lsn`/`checkpoint_lsn` unchanged
|
||||
(Requirement 2.3).
|
||||
- **Unit — legacy file handling**: a file sized to end exactly at the legacy header yields rc==0,
|
||||
`entities_restored=false`, no crash (Requirement 2.4, 5.1, 5.2).
|
||||
- **Unit — rotation guard**: a file matching an in-flight LSN survives `rotate_snapshots` even
|
||||
when it would otherwise fall outside `snap_keep_n` (Requirement 3.1, 3.2, 3.3).
|
||||
- **Integration — chaotic catch-up soak**: extend the existing `/xce-test-ha --chaotic` soak
|
||||
scenario so at least one round forces a `SEND_SNAPSHOT` catch-up (empty or far-behind member);
|
||||
assert `committed_divergence=0` and `committed_job_loss=0` post-round (Requirement 4.1, 4.2) —
|
||||
this is the direct acceptance test for the residual soak failure this spec exists to close.
|
||||
- **Integration — rolling upgrade simulation**: a pre-feature-format checkpoint file present on
|
||||
disk, loaded by the new binary, does not crash and correctly falls back to full catch-up
|
||||
rather than reporting false-positive completeness (Requirement 5.1, 5.2).
|
||||
64
.kiro/specs/ha-entity-snapshots/requirements.md
Normal file
64
.kiro/specs/ha-entity-snapshots/requirements.md
Normal file
|
|
@ -0,0 +1,64 @@
|
|||
# Requirements Document
|
||||
|
||||
## Project Description (Input)
|
||||
ha-entity-snapshots: Job/queue entity-bearing WAL snapshots so a rejoining or empty-restart node's catch-up gap (task 5.10a / raft-ha-core-safety, bug-795/828/851 family) closes without permanent committed-prefix loss. Today write_snapshot (persist.c ~1051) writes a 16-byte sentinel with ZERO job/queue entities and load_snapshot only advances the LSN watermark past entries it never received (restores nothing); trim_wal_segments deletes the WAL prefix below a checkpoint. A member that rejoins empty or behind a trimmed prefix is streamed only [watermark..committed] with no snapshot backfill for [1..watermark-1] — a permanent gap (observed live: 10-round chaotic soak shows committed_divergence>0 every round from this catch-up lag). Scope: write_snapshot Cap'n-Proto-serializes all live job/queue entities at checkpoint_lsn; load_snapshot deserializes + restores them BEFORE advancing current_lsn; trim_wal_segments must not trim below the LSN covered by a durable entity-bearing snapshot; install-snapshot-before-stream on the catch-up receive side. Full investigation at .kiro/specs/ha-commit-durability/INVESTIGATION-failover-job-loss.md (VERDICT section) and .kiro/specs/raft-ha-core-safety/tasks.md task 5.10(a).
|
||||
|
||||
## Introduction
|
||||
XCE cluster members that rejoin the cluster empty, or that fall behind the oldest history the cluster still retains directly, currently receive no way to recover the committed job and queue history that existed before their position. The periodic checkpoint the cluster produces carries only a position marker, not the actual job/queue data, and older history is deleted once a checkpoint is taken. As a result, a member that catches up this way ends up reporting itself as caught up while permanently missing part of the committed record — a gap that persists indefinitely and that surfaces as data divergence across the cluster. This feature makes the checkpoint a complete, restorable copy of committed job and queue state, and ensures history is never discarded before that copy exists, so that every member — however it rejoins — ends up holding the same complete committed state as the rest of the cluster.
|
||||
|
||||
## Boundary Context
|
||||
- **In scope**:
|
||||
- The content of a checkpoint: it must represent the actual job and queue records committed as of that point, not merely a position.
|
||||
- Restoring a checkpoint: reconstructing the actual job and queue state it contains, not merely advancing a position.
|
||||
- Coordinating history retention so that log data is never discarded before its content is captured in a checkpoint that has been confirmed complete and durable.
|
||||
- The end-to-end outcome for a member that rejoins empty or significantly behind: it must reach the same complete committed job/queue state as an already-consistent member.
|
||||
- Safe behavior when a member encounters a checkpoint produced before this capability existed (rolling upgrade).
|
||||
- **Out of scope**:
|
||||
- Worker/compute-node registration and state — workers re-establish their state via the existing join protocol, independent of this feature.
|
||||
- The mechanics of transferring a checkpoint to a peer over the network — that transport already exists and is unchanged by this feature.
|
||||
- Cluster configuration document snapshotting/versioning — owned by the `config-upgrade-snapshot` feature.
|
||||
- Election, vote-freshness, and term-safety behavior — owned by `raft-ha-core-safety` / `ha-term-safety`.
|
||||
- **Adjacent expectations**: This feature assumes the existing catch-up decision logic (choosing between an incremental replay and a full checkpoint transfer for a member whose position is below the cluster's retained history) continues to trigger correctly and is unchanged by this feature; what changes is that the checkpoint it delivers is now complete, and the member's outcome after catch-up is now correct.
|
||||
|
||||
## Requirements
|
||||
|
||||
### Requirement 1: Complete entity-bearing checkpoint content
|
||||
**Objective:** As a cluster operator, I want a checkpoint to contain the full set of committed job and queue records as of that point, so that a member restoring it receives actual data rather than only a position marker.
|
||||
|
||||
#### Acceptance Criteria
|
||||
1. When the system produces a checkpoint at a committed position, the Persistence Service shall include in the checkpoint every job and queue record that is live or retained under the existing retention policy as of that position.
|
||||
2. The Persistence Service shall produce a checkpoint that reflects a single, internally consistent point-in-time view of job and queue state, with no record split between its pre-checkpoint and post-checkpoint value.
|
||||
3. If a checkpoint cannot be produced completely (for example, an I/O failure partway through), then the Persistence Service shall discard the incomplete checkpoint and shall not present it as a valid restore point.
|
||||
4. The Persistence Service shall record, as part of the checkpoint, the committed position the contained entity state corresponds to.
|
||||
|
||||
### Requirement 2: Full restoration from a checkpoint
|
||||
**Objective:** As a rejoining or newly provisioned cluster member, I want loading a checkpoint to reconstruct the actual job and queue state it contains, so that I hold complete data rather than an empty state with an advanced position.
|
||||
|
||||
#### Acceptance Criteria
|
||||
1. When a member loads a checkpoint, the Persistence Service shall reconstruct every job and queue record contained in the checkpoint into the member's live state before advancing the member's committed position.
|
||||
2. The Persistence Service shall consider the member's committed position advanced to the checkpoint's position only after all contained records have been reconstructed.
|
||||
3. If reconstruction fails partway through, then the Persistence Service shall not advance the member's committed position and shall report the failure rather than silently proceeding with partial state.
|
||||
4. Where a loaded checkpoint carries no entity state (a checkpoint produced before this capability existed), the Persistence Service shall treat that checkpoint as insufficient on its own and shall require the member to obtain the missing history through the normal catch-up path rather than treating the checkpoint as complete.
|
||||
|
||||
### Requirement 3: Retention never outruns restorability
|
||||
**Objective:** As a cluster operator, I want history retention (discarding older log data) to never outrun the point at which that history is fully captured in a restorable checkpoint, so that a rejoining member is never left with a permanent, unrecoverable gap.
|
||||
|
||||
#### Acceptance Criteria
|
||||
1. While a portion of history has not yet been captured in a checkpoint whose entity content is confirmed durable, the Persistence Service shall retain the corresponding log data.
|
||||
2. The Persistence Service shall discard log data only when the history it covers is fully represented in a durable, entity-bearing checkpoint at or beyond that point.
|
||||
3. If retention is requested for a position not yet covered by a durable, entity-bearing checkpoint, then the Persistence Service shall reject or defer that retention rather than discarding the data.
|
||||
|
||||
### Requirement 4: End-to-end catch-up correctness for a behind or empty member
|
||||
**Objective:** As a cluster operator, I want a member that rejoins empty or significantly behind to end up holding the same committed job and queue state as the rest of the cluster, so that a failover onto that member never serves incomplete or divergent data.
|
||||
|
||||
#### Acceptance Criteria
|
||||
1. When a member's position is behind the oldest history the cluster still retains directly, the system shall bring that member to full committed-consistent job and queue state through a checkpoint transfer followed by any remaining incremental history.
|
||||
2. After a member completes catch-up, whether through a checkpoint, incremental replay, or both, the system shall hold the same committed job and queue records as an already-consistent member, with no permanent gap.
|
||||
3. The system shall make the completeness, or incompleteness, of a member's catch-up observable to an operator through existing status and comparison tooling, rather than reporting a member as fully caught up when it is not.
|
||||
|
||||
### Requirement 5: Safe rollout against existing deployments
|
||||
**Objective:** As a cluster operator upgrading an existing deployment, I want the new checkpoint format to coexist safely with checkpoints produced by the prior version, so that upgrading never corrupts state or crashes a member.
|
||||
|
||||
#### Acceptance Criteria
|
||||
1. When a member reads a checkpoint file, the Persistence Service shall determine whether it is an entity-bearing checkpoint or a prior-format, position-only checkpoint before deciding how to use it.
|
||||
2. The Persistence Service shall never crash, corrupt state, or fabricate job or queue records when reading a prior-format checkpoint.
|
||||
205
.kiro/specs/ha-entity-snapshots/research.md
Normal file
205
.kiro/specs/ha-entity-snapshots/research.md
Normal file
|
|
@ -0,0 +1,205 @@
|
|||
# Gap Analysis: ha-entity-snapshots
|
||||
|
||||
## Summary
|
||||
|
||||
This feature is **substantially cheaper than it looks**. Every low-level primitive it needs
|
||||
already exists in the codebase for an unrelated purpose (WAL replay reconstruction) or is
|
||||
auto-generated by the existing Cap'n Proto schema. The work is almost entirely **wiring**:
|
||||
enumerate live entities → serialize into the snapshot file → on load, deserialize and call the
|
||||
existing restore functions → extend the existing trim-floor guard. No new capnp schema struct
|
||||
is strictly required (a length-prefixed sequence of existing per-entity blobs works), though a
|
||||
dedicated `Snapshot` struct would be cleaner. The main open design question is write-side
|
||||
concurrency: the "proper" mechanism (zarray copy-on-write) exists and is tested, but was never
|
||||
wired into the job/queue backing store (`kvmgr`) — a deliberate deferral ("wired in P0-2"), not
|
||||
a bug. This is the one real judgment call for the design phase.
|
||||
|
||||
## Requirement-to-Asset Map
|
||||
|
||||
| Requirement | Existing Asset | Gap |
|
||||
|---|---|---|
|
||||
| R1.1 include every live/retained job+queue record | `collect_jobs`/`collect_queues` (dataex.c:215-330, lock READ → `iter_make`/`iter_next` → unlock); `job_should_retain`/`worker_should_retain` (persist.c:3013+) already implement the retention predicate | **Missing**: nothing enumerates for the *snapshot* purpose today — `write_snapshot` (persist.c ~1477-1519) writes only a 57-byte header. Wiring, not new logic. |
|
||||
| R1.1 per-entity serialization | `capnp_encode_job` (persist.c:45), `capnp_encode_worker` (persist.c:68); capnp codegen for `QueueConf`/`xce_queue_t` — `encode_QueueConf`, `write_QueueConf`, `new_QueueConf`, `new_QueueConf_list` all already generated (controller.capnp.h) | **Missing**: a `capnp_encode_queue` wrapper — trivial, mirrors `capnp_encode_job` line-for-line (~15 lines). |
|
||||
| R1.2 internally consistent point-in-time view | zarray COW (`src/lib/zarray.c` `xzarray_freeze`/`xzarray_thaw`, tested in `test_zarray_cow.c`) | **Constraint**: COW exists but is **NOT wired into `kvmgr`** (the hash table backing `c->jobmgr`/`c->qmgr`) — `kvmgr.c:297-298` explicitly stubs `vfreeze`/`vthaw` to NULL with a "wired in P0-2" comment. Consistency must come from a `RTTI_LOCK_READ` held across the full enumeration+encode instead (same lock `collect_jobs` already takes, just held longer). This is the one real design decision — see Options below. |
|
||||
| R1.3 discard incomplete checkpoint | `write_snapshot`'s existing tmp-file + atomic `rename` pattern (persist.c ~1477+, same pattern as `ha_handle_snapshot_chunk`'s `snap.part` → final rename, ha.c:2589-2594) | **None** — extend the existing tmp+rename-on-success pattern to the entity payload; a partial write never reaches the final path. |
|
||||
| R1.4 record committed position | Already present (`checkpoint_lsn` in the existing sentinel header, persist.c) | **None.** |
|
||||
| R2.1/2.2 restore entities before advancing position | `restore_job` (job.c:96, called from WAL replay persist.c:1915) and `restore_queue_state` (queue.c:208, called persist.c:1945) — **both already exist and are proven** (used every restart via `apply_wal_entry`) | **Missing**: `load_snapshot` (persist.c:1811-1847) currently sets `current_lsn`/`checkpoint_lsn` directly (line ~1835) with no entity step. Insert a decode-and-`restore_job`/`restore_queue_state` loop **before** that assignment, reusing the exact WAL-replay call. |
|
||||
| R2.3 failed reconstruction must not advance position | N/A — new logic, but trivial given restore functions already return rc | **Missing**: gate the `current_lsn`/`checkpoint_lsn` store on the restore loop's success (straightforward given R2.1/2.2's insert point). |
|
||||
| R2.4 pre-feature checkpoint treated as insufficient | N/A | **Missing**: a format/version discriminator. Cheapest: if the file is exactly the legacy 57-byte sentinel size with no trailing entity payload, treat as position-only (do not claim completeness); a new format adds a marker/count after the existing header. |
|
||||
| R3 retention never outruns restorability | `repl_trim_lsn` (repl.c:650-683) + the bug-1147 guard in `trim_wal_segments` (persist.c:1615, floor = min(checkpoint_lsn, every active standby's `acked_lsn`, disconnected-slot `last_disconnected_lsn`)) — **this is an EXISTING, working guard**, confirmed by its own comment: "does NOT fix the empty/far-behind join case... requires entity-bearing snapshots (separate spec)" — i.e. this spec is the one it names. | **Extension, not new**: add "a standby is currently mid-install of snapshot at LSN X" as another floor input into `repl_trim_lsn`'s `min_lsn` computation — same shape as the existing acked_lsn/disconnected-slot terms. |
|
||||
| R4 end-to-end catch-up correctness | `ha_evaluate_repl_action` (ha.c:2644, unchanged by this spec) → `ha_snapshot_send_dispatch`/`ha_snapshot_do_send` (ha.c ~2788+, unchanged) → wire opcode `OPCode_snapshotTransfer` → `handle_snapshot_transfer` (cluster.c:197-229) → **`ha_handle_snapshot_chunk`** (ha.c:2529-2609): assembles `snap.part`, renames to final path, calls `persist_load_snapshot`, then sets `ha->applied_lsn`. | **Insertion point identified, not missing infra**: the entity-restore work belongs entirely inside `persist_load_snapshot`/`load_snapshot` (R2's gap). Once that returns correctly, `ha_handle_snapshot_chunk`'s existing "only advance `applied_lsn` after `persist_load_snapshot` succeeds" structure already gives R4's ordering for free. |
|
||||
| R4.3 operator-observable catch-up completeness | `walcompare`/`waldump` tooling already used this session to diagnose divergence live | **None new required**; existing tooling already surfaces divergence. Confirm it also reports on entity-snapshot-derived state, not just WAL-visible LSNs (verify in design). |
|
||||
| R5 safe rollout / old-format checkpoints | `rotate_snapshots` already discriminates by filename pattern (`*.capnp`) | **Missing**: the actual content-based old-vs-new format check (R2.4's gap, restated here for the read path). |
|
||||
|
||||
## Implementation Approach Options
|
||||
|
||||
### Option A: Extend existing components in place (RECOMMENDED)
|
||||
- Extend `write_snapshot`/`load_snapshot` (persist.c) directly: after the existing sentinel
|
||||
header, append a length-prefixed sequence of capnp-encoded `Job` and `QueueConf` blobs
|
||||
(reusing `capnp_encode_job`, a new trivial `capnp_encode_queue`, and on load `decode_Job`/
|
||||
`decode_QueueConf` + `restore_job`/`restore_queue_state`).
|
||||
- Write-side consistency: hold `RTTI_LOCK_READ` on `c->jobmgr`/`c->qmgr` across the full
|
||||
enumerate+encode (mirrors `collect_jobs`'s existing lock scope, just held for the whole
|
||||
snapshot instead of one collection pass). Do the actual disk write from an in-memory buffer
|
||||
built while holding the lock, then release the lock before `fdatasync`+rename — so file I/O
|
||||
latency does not extend the lock hold.
|
||||
- Extend `repl_trim_lsn` with one more floor term (in-flight snapshot-install LSN per standby).
|
||||
- ✅ Every serialization/restore primitive already exists and is proven (WAL replay uses the
|
||||
same `restore_job`/`restore_queue_state`) — minimal new code, maximum reuse of tested paths.
|
||||
- ✅ No new wire opcode, no new capnp schema struct strictly required.
|
||||
- ❌ Holding a read lock across a full job-table walk blocks concurrent job mutations for the
|
||||
checkpoint's duration — needs a bound (checkpoint interval is already configurable;
|
||||
`snap_interval_s` exists) and a measurement of walk cost at realistic job-table sizes.
|
||||
|
||||
### Option B: Wire zarray COW into kvmgr first, then build the snapshot on top
|
||||
- Complete the deferred "P0-2" work (wire `xzarray_freeze`/`xzarray_thaw` into `kvmgr`), then
|
||||
`write_snapshot` freezes a shadow, walks it lock-free, and thaws — no read-lock hold during
|
||||
the walk.
|
||||
- ✅ Removes the lock-hold concern entirely; likely benefits other future features too.
|
||||
- ❌ Meaningfully larger and touches a shared, load-bearing data structure (`kvmgr` backs both
|
||||
job and queue tables and possibly other managers) used cluster-wide — higher blast radius and
|
||||
risk for a benefit (lock-free enumeration) that may not be needed if Option A's lock-hold
|
||||
duration proves acceptable in practice.
|
||||
- ❌ It was deliberately deferred once already; reviving it is its own scoped effort, arguably a
|
||||
separate spec/prerequisite rather than embedded in this one.
|
||||
|
||||
### Option C: Hybrid — ship Option A now, leave a documented seam for Option B later
|
||||
- Build the snapshot feature entirely per Option A, but structure the enumeration behind a
|
||||
single seam function (e.g. `persist_collect_snapshot_entities`) so that if kvmgr COW is wired
|
||||
later, only that one function's internals change (lock+iterate → freeze+iterate+thaw),
|
||||
nothing else in the snapshot read/write/trim/catch-up path needs to change.
|
||||
- ✅ Ships the actual bug fix now; keeps the door open without committing to the larger
|
||||
refactor's risk today.
|
||||
- ❌ Slightly more upfront interface discipline than just inlining the lock+iterate call.
|
||||
|
||||
## Effort & Risk
|
||||
|
||||
- **Effort: M (3-7 days).** All hard primitives (encode/decode/restore) pre-exist and are
|
||||
proven; the work is wiring four call sites (write, load, trim, and the format-discriminator)
|
||||
plus tests. Design and TDD discipline (per `.kiro/steering/testing.md`) will take real time
|
||||
given this is HA/persistence-critical code, but there is no unknown technology.
|
||||
- **Risk: Medium.** Known patterns throughout (Option A), but this is exactly the kind of
|
||||
code the `raft-ha-core-safety` family (bug-795/828/851/1306) has shown is easy to get subtly
|
||||
wrong under concurrent chaos — the actual risk is in getting R2.3 (failed-restore-must-not-
|
||||
advance-position) and R3 (trim-floor extension) correct under a live chaotic soak, not in
|
||||
writing the serialization code itself. Recommend the same validation bar the recent session's
|
||||
fixes used: unit RED/GREEN + a chaotic multi-round soak showing `committed_divergence=0`.
|
||||
|
||||
## Research Needed (carry to design phase)
|
||||
|
||||
1. **Snapshot file layout**: a single new capnp `Snapshot { jobs: List(Job); queues:
|
||||
List(QueueConf); ... }` struct (cleaner, one `capn` init/write) vs. keeping the existing raw
|
||||
header + hand-rolled length-prefixed blob sequence (smaller diff, matches the current
|
||||
sentinel's hand-rolled style). Design should pick one; either is buildable on existing codegen.
|
||||
2. **Lock-hold duration measurement**: before committing to Option A's approach, get an order-
|
||||
of-magnitude estimate or measurement of `collect_jobs`+encode cost at a realistic job-table
|
||||
size, to confirm the read-lock hold is acceptably short (checkpoints already run on a
|
||||
`snap_interval_s` cadence via `delay_task`, off the hot request path).
|
||||
3. **Format discriminator mechanics**: exact byte-level marker to distinguish old 57-byte
|
||||
sentinel-only files from new entity-bearing files without breaking `rotate_snapshots`'s
|
||||
existing filename-based selection.
|
||||
4. **Trim-floor extension bookkeeping**: where to track "standby X mid-install of snapshot LSN
|
||||
Y" — likely alongside the existing `standby_conn_t`/disconnected-slot state already used by
|
||||
`repl_trim_lsn`, but the exact struct field and its lifecycle (set on `SEND_SNAPSHOT`
|
||||
dispatch, cleared on the receiver's ack) needs design-phase detail.
|
||||
5. **Interaction with `ha_snapshot_offload_enabled()` slow-lane path** (ha.c
|
||||
`ha_snapshot_send_dispatch`): confirm the entity payload size doesn't change the offload
|
||||
flag-ON vs flag-OFF dispatch decision or chunking assumptions already in place for
|
||||
`ha_handle_snapshot_chunk`'s multi-chunk reassembly.
|
||||
|
||||
## Recommendation
|
||||
|
||||
**Option A** (extend `write_snapshot`/`load_snapshot`/`trim_wal_segments` in place, structured
|
||||
per **Option C**'s seam discipline) for the design phase. Do not pursue Option B (kvmgr COW
|
||||
wiring) as part of this spec — it is a legitimate future improvement but was already
|
||||
deliberately deferred once, and this spec's actual requirements (R1-R5) do not need it: a
|
||||
bounded read-lock hold during a periodic, off-hot-path checkpoint is a reasonable cost, and it
|
||||
keeps this spec's blast radius to `persist.c`/`ha.c`/`repl.c`, matching the existing bug-1147
|
||||
guard's pattern rather than touching the shared kvmgr.
|
||||
|
||||
---
|
||||
|
||||
## Design Phase — Synthesis & Refinements (2026-07-01)
|
||||
|
||||
### Key refinement over the gap analysis
|
||||
The gap analysis (above) assumed `trim_wal_segments`/`repl_trim_lsn` needed a new floor input
|
||||
keyed on "a standby is currently mid-install of a snapshot." Closer reading of
|
||||
`persist_checkpoint` (persist.c:1763) shows this is **already handled**: `write_snapshot` runs
|
||||
before `trim_wal_segments`, and a non-zero `write_snapshot` return skips both `trim_wal_segments`
|
||||
and `rotate_snapshots` via the existing `goto done`. Once `write_snapshot` only returns 0 when
|
||||
the entity payload is fully and durably written (this spec's Requirement 1.3), that existing
|
||||
sequencing alone satisfies Requirement 3 — **no new code needed in `trim_wal_segments`/
|
||||
`repl_trim_lsn`**.
|
||||
|
||||
The one retention hazard that genuinely has no existing guard is **`rotate_snapshots`** deleting
|
||||
a checkpoint *file* while a standby is still mid-transfer reading it over the wire (a checkpoint
|
||||
cadence faster than a slow transfer). This is the actual new mechanism this design adds: a
|
||||
`snapshot_inflight_lsn[]` array (mirroring the existing `snapshot_inflight[]` flag array in
|
||||
`ha_state_t`) that `rotate_snapshots` consults before deleting a candidate file.
|
||||
|
||||
### Synthesis: Generalization
|
||||
Requirements 1 and 2 both need "enumerate a live entity of kind K, encode it, and (for restore)
|
||||
decode+apply it." Rather than writing separate bespoke job-snapshot and queue-snapshot code
|
||||
paths, the design generalizes the *interface* (a small per-kind {collect, encode, decode+restore}
|
||||
triple) while keeping the *implementation* scoped to exactly the two kinds Requirement 1 names
|
||||
(job, queue) — not built out for hypothetical future entity kinds (e.g. workers), consistent with
|
||||
the requirement's explicit boundary.
|
||||
|
||||
### Synthesis: Build vs. Adopt
|
||||
Adopt, not build: the project's own Cap'n Proto schema and generated codecs for `Job`/
|
||||
`QueueConf` already exist and are already proven via WAL replay's `restore_job`/
|
||||
`restore_queue_state`. No external library, no new schema struct, no new wire protocol — this
|
||||
spec is pure reuse of already-solved, already-tested primitives.
|
||||
|
||||
### Synthesis: Simplification
|
||||
Rejected a new top-level capnp `Snapshot` message struct (would require a schema/build change for
|
||||
a one-off container) in favor of a hand-rolled length-prefixed frame sequence — matching the
|
||||
project's own existing WAL entry framing style (`wal_entry_hdr_t` + payload) and avoiding a new
|
||||
abstraction layer for something with exactly one caller. Rejected reviving the deferred zarray
|
||||
COW wiring (Option B) as unnecessary for this spec's scope — a bounded `RTTI_LOCK_READ` hold
|
||||
during an off-hot-path, periodic checkpoint is a reasonable cost that keeps this spec's blast
|
||||
radius to `persist.c`/`ha.c`, matching the existing bug-1147 guard's precedent rather than
|
||||
touching the shared `kvmgr`.
|
||||
|
||||
### Design Decisions Log
|
||||
- **On-disk format**: legacy header (unchanged) + `format_version` + `job_count` + length-prefixed
|
||||
`Job` frames + `queue_count` + length-prefixed `QueueConf` frames. Legacy-file detection is
|
||||
"no bytes remain after the legacy header" — no explicit flag bit needed.
|
||||
- **Consistency boundary**: per-entity, not cross-entity — jobs and queues are independently
|
||||
locked/encoded in two sequential `RTTI_LOCK_READ` sections, never held simultaneously.
|
||||
- **API change**: `persist_load_snapshot` gains an `out_entities_restored` out-parameter so the
|
||||
caller can distinguish "fully restored" from "legacy position-only" without treating the latter
|
||||
as an error.
|
||||
- **New guard location**: `rotate_snapshots` (not `trim_wal_segments`), backed by a new
|
||||
`snapshot_inflight_lsn[MAX_HA_CANDIDATES]` array in `ha_state_t`, lifecycle-mirrored to the
|
||||
existing `snapshot_inflight[]` flag.
|
||||
|
||||
---
|
||||
|
||||
## Design Validation Pass — Corrections (2026-07-01)
|
||||
|
||||
`/kiro-validate-design` found two real defects in the first design draft, both confirmed against
|
||||
code and fixed in `design.md`:
|
||||
|
||||
1. **Queue entity shape was wrong.** The draft planned to serialize a full `QueueConf` capnp
|
||||
entity per queue, but `restore_queue_state(cluster_t*, const char* name, uint32_t status)`
|
||||
(queue.c:208) — the only existing queue-restore function — takes just a name and status word;
|
||||
its own comment states queues are config-built at startup and only admin status changes are
|
||||
WAL-tracked. Confirmed independently: queue *definitions* are already replicated via
|
||||
`push_config_to_standbys` (config_repl.c), a separate, already-committed, majority-acked
|
||||
channel — entirely out of this spec's boundary (user confirmation: "the basic configuration
|
||||
is not lived in cluster state snapshot as we have independent configuration"). **Fix**: the
|
||||
queue-side snapshot payload is now a plain `(name, status)` record, no capnp, restored via the
|
||||
unchanged `restore_queue_state`. This *simplifies* the design (no queue capnp encoder needed
|
||||
at all).
|
||||
2. **The table-lock-only consistency argument was false.** The original Architecture section
|
||||
claimed `RTTI_LOCK_READ` on `c->jobmgr`/`c->qmgr` (the table lock) excludes concurrent entity
|
||||
mutation. Verified false: `restore_queue_state` itself takes a *separate* per-entity
|
||||
`lock(q, RTTI_LOCK_WRITE)`, and job state-transition sites (job.c:1006/1093/1137) take
|
||||
`lock(j, RTTI_LOCK_WRITE)` — a distinct lock object from the table lock. **Fix**: the
|
||||
entity-collection helper now nests each entity's own `RTTI_LOCK_READ` inside the table walk,
|
||||
around that entity's individual encode call — mirroring `restore_queue_state`'s own
|
||||
nested-lock pattern. Added a new test item (concurrent-mutation-during-snapshot consistency)
|
||||
to prove this directly.
|
||||
|
||||
Both fixes are local repairs to the write-side collection logic and the queue payload format —
|
||||
the overall Option-A approach, boundary, and file structure plan are unchanged.
|
||||
22
.kiro/specs/ha-entity-snapshots/spec.json
Normal file
22
.kiro/specs/ha-entity-snapshots/spec.json
Normal file
|
|
@ -0,0 +1,22 @@
|
|||
{
|
||||
"feature_name": "ha-entity-snapshots",
|
||||
"created_at": "2026-07-01T03:32:11Z",
|
||||
"updated_at": "2026-07-01T04:20:00Z",
|
||||
"language": "en",
|
||||
"phase": "tasks-generated",
|
||||
"approvals": {
|
||||
"requirements": {
|
||||
"generated": true,
|
||||
"approved": true
|
||||
},
|
||||
"design": {
|
||||
"generated": true,
|
||||
"approved": true
|
||||
},
|
||||
"tasks": {
|
||||
"generated": true,
|
||||
"approved": true
|
||||
}
|
||||
},
|
||||
"ready_for_implementation": true
|
||||
}
|
||||
127
.kiro/specs/ha-entity-snapshots/tasks.md
Normal file
127
.kiro/specs/ha-entity-snapshots/tasks.md
Normal file
|
|
@ -0,0 +1,127 @@
|
|||
# Implementation Plan
|
||||
|
||||
- [x] 1. Foundation: entity-snapshot test harness
|
||||
- [x] 1.1 Build a reusable test fixture for a persist_t backed by a populated job/queue table
|
||||
- Model on the existing `make_hook_test_persist` / `cbrp_setup` fixture patterns in the test
|
||||
suite
|
||||
- Fixture can seed N jobs (varying state) and M queues (varying status) into the cluster's
|
||||
job/queue tables
|
||||
- Observable: a new test file exists and compiles, exposing a helper that produces a ready
|
||||
persist_t+cluster_t with seeded entities
|
||||
- _Requirements: 1.1, 2.1_
|
||||
|
||||
- [ ] 2. Core: entity-bearing checkpoint write path — jobs
|
||||
- [ ] 2.1 RED: job checkpoint content, consistency, and failure-discard tests
|
||||
- Assert a checkpoint containing N jobs round-trips through load with all N job records
|
||||
field-identical
|
||||
- Assert a job actively transitioning state on a concurrent thread during the checkpoint walk
|
||||
is captured as a single consistent pre- or post-transition value, never torn
|
||||
- Assert a simulated write/encode failure partway through leaves no file at the final
|
||||
checkpoint path — the same discard behavior that (per design) is what already makes the
|
||||
existing trim/rotate sequencing satisfy the retention-safety requirement
|
||||
- Observable: new tests compile and fail (RED) against the current sentinel-only write_snapshot
|
||||
- _Requirements: 1.1, 1.2, 1.3, 3.1, 3.2, 3.3_
|
||||
- _Boundary: Persistence Layer (write_snapshot)_
|
||||
- [ ] 2.2 Collect and encode live jobs into the checkpoint under per-entity consistency
|
||||
- Enumerate live jobs under the existing retention policy, take each job's own read lock
|
||||
nested inside the table walk, encode via the existing job serializer, and append to the
|
||||
checkpoint after the current header
|
||||
- Any failure at any step discards the temp file and leaves no file at the final path
|
||||
- Observable: the RED tests from 2.1 pass
|
||||
- _Requirements: 1.1, 1.2, 1.3, 1.4_
|
||||
- _Boundary: Persistence Layer (write_snapshot)_
|
||||
|
||||
- [ ] 3. Core: entity-bearing checkpoint write path — queues
|
||||
- [ ] 3.1 RED: queue admin-status round-trip test
|
||||
- Assert a queue's (name, status) pair written into the checkpoint and read back matches
|
||||
exactly
|
||||
- Observable: new test compiles and fails (RED)
|
||||
- _Requirements: 1.1_
|
||||
- _Boundary: Persistence Layer (write_snapshot)_
|
||||
- _Depends: 2.2_
|
||||
- [ ] 3.2 Collect queue admin-status into the checkpoint
|
||||
- Enumerate every queue present in the live table (its table presence already implies
|
||||
config-provisioned existence), take each queue's own read lock nested inside the table walk,
|
||||
append its name+status as a plain record (no serialization schema needed — matches the
|
||||
existing restore function's exact contract)
|
||||
- Observable: the RED test from 3.1 passes; a checkpoint containing M queues restores M
|
||||
correct statuses
|
||||
- _Requirements: 1.1, 1.2_
|
||||
- _Boundary: Persistence Layer (write_snapshot)_
|
||||
|
||||
- [ ] 4. Core: protect an in-flight checkpoint transfer from rotation
|
||||
- [ ] 4.1 (P) Track which checkpoint position each in-flight peer transfer references
|
||||
- Add a per-peer tracking array alongside the existing in-flight-transfer flag; set it when a
|
||||
transfer begins, clear it when the transfer completes
|
||||
- Observable: a unit test confirms the tracked position is set before a transfer's first byte
|
||||
goes out and cleared after completion
|
||||
- _Requirements: 3.1, 3.2, 3.3_
|
||||
- _Boundary: HA Replication Layer_
|
||||
- [ ] 4.2 RED: rotation-guard test
|
||||
- Assert a checkpoint file whose position matches an in-flight transfer survives file rotation
|
||||
even when it would otherwise be rotated out
|
||||
- Observable: new test compiles and fails (RED) against the current rotation logic
|
||||
- _Requirements: 3.1, 3.2, 3.3_
|
||||
- _Depends: 4.1_
|
||||
- [ ] 4.3 Skip rotating out a checkpoint file that is currently mid-transfer
|
||||
- Extend the existing file-rotation pass to check the in-flight tracking before deleting a
|
||||
candidate file
|
||||
- Observable: the RED test from 4.2 passes
|
||||
- _Requirements: 3.1, 3.2, 3.3_
|
||||
|
||||
- [ ] 5. Core: entity restoration on load
|
||||
- [ ] 5.1 RED: restore-before-advance, failure-safety, and legacy-format tests
|
||||
- Assert loading a checkpoint with N jobs + M queues restores all of them before the member's
|
||||
position advances
|
||||
- Assert a deliberately corrupted entity record causes the load to fail with the member's
|
||||
position left unchanged
|
||||
- Assert a checkpoint file predating this feature (position marker only) is accepted without
|
||||
crashing, reports itself as position-only, and does not falsely claim full restoration
|
||||
- Observable: new tests compile and fail (RED) against the current watermark-only load
|
||||
- _Requirements: 2.1, 2.2, 2.3, 2.4, 5.1, 5.2_
|
||||
- _Boundary: Persistence Layer (load_snapshot)_
|
||||
- _Depends: 2.2, 3.2_
|
||||
- [ ] 5.2 Restore entities before advancing the member's position, with legacy-format detection
|
||||
- Detect whether a checkpoint carries entity content or is a legacy position-only file
|
||||
- Restore every job then every queue status via the existing restore functions, in file order;
|
||||
on any failure, stop and leave position unchanged
|
||||
- Only after every entity restores successfully, advance the member's committed position;
|
||||
extend the load function's contract with an indicator of whether entities were actually
|
||||
restored
|
||||
- Observable: the RED tests from 5.1 pass
|
||||
- _Requirements: 2.1, 2.2, 2.3, 2.4, 5.1, 5.2_
|
||||
- _Boundary: Persistence Layer (load_snapshot)_
|
||||
|
||||
- [ ] 6. Integration: wire the catch-up receive path to the new restore contract
|
||||
- [ ] 6.1 Consume the new restoration indicator in the snapshot-chunk receive handler
|
||||
- The handler advances the member's applied position only after the load succeeds (unchanged
|
||||
ordering); on a legacy (position-only) file it logs the fallback rather than treating it as
|
||||
a failure
|
||||
- Observable: given a legacy-format file, the member logs the fallback and continues through
|
||||
normal catch-up; given an entity-bearing file, the member's applied position advances only
|
||||
after restoration succeeds
|
||||
- _Requirements: 2.4, 4.1, 4.2, 5.1, 5.2_
|
||||
- _Depends: 5.2_
|
||||
|
||||
- [ ] 7. Validation: chaotic catch-up soak proves end-to-end correctness
|
||||
- [ ] 7.1 Extend the chaotic HA failover soak to force at least one full-checkpoint catch-up round
|
||||
- Configure or trigger a round where a member rejoins empty or far enough behind that it must
|
||||
receive a full checkpoint rather than incremental history
|
||||
- Observable: the soak verdict for that round reports zero committed-job loss and zero
|
||||
committed-prefix divergence, and existing comparison tooling confirms the rejoined member's
|
||||
job/queue state matches the rest of the cluster
|
||||
- _Requirements: 4.1, 4.2, 4.3_
|
||||
|
||||
- [ ] 8. Validation: rolling-upgrade compatibility
|
||||
- [ ] 8.1 Verify a pre-feature checkpoint file is handled safely by the new binary in a live
|
||||
cluster
|
||||
- Plant a checkpoint file in the legacy (position-only) format, start a member on the upgraded
|
||||
binary against it, and observe its behavior
|
||||
- Observable: the member does not crash, logs the legacy-format fallback, and reaches full
|
||||
committed consistency through normal catch-up rather than reporting false-positive
|
||||
completeness
|
||||
- _Requirements: 5.1, 5.2_
|
||||
- _Depends: 6.1_
|
||||
|
||||
## Implementation Notes
|
||||
- (1.1) Test fixture `snapent_make`/`snapent_teardown` lives in `test_persist_snapshot_entities.c`; seeded jobs currently carry only jobid+state+an all-zeroed submit. Task 2.1's "field-identical round-trip" assertion needs richer submit-payload seeding (e.g. non-zero cmdline/user/queue in `xce_job_submit_t`) to be a meaningful field-by-field comparison — extend `snapent_seed_jobs`, do not re-seed separately.
|
||||
|
|
@ -151,6 +151,7 @@ controller_unit_test_src = [
|
|||
'testsuite/waldump.c',
|
||||
'testsuite/test_walcompare.c',
|
||||
'testsuite/walcompare.c',
|
||||
'testsuite/test_persist_snapshot_entities.c',
|
||||
'ctrlstatus.c',
|
||||
'commit.c',
|
||||
'job.c',
|
||||
|
|
|
|||
|
|
@ -73,7 +73,7 @@ extern void test_ha_vep_teardown(void);
|
|||
extern void test_ha_dispatch_integration(void);
|
||||
extern void test_ha_core_safety(void);
|
||||
extern void test_ha_partition(void);
|
||||
extern void test_ha_partition(void);
|
||||
extern void test_persist_snapshot_entities(void);
|
||||
|
||||
void setUp(void) { return; }
|
||||
|
||||
|
|
@ -216,6 +216,7 @@ int main(int argc, char* argv[])
|
|||
TEST(test_ha_dispatch_integration);
|
||||
TEST(test_ha_core_safety);
|
||||
TEST(test_ha_partition);
|
||||
TEST(test_persist_snapshot_entities);
|
||||
|
||||
return UNITY_END();
|
||||
}
|
||||
|
|
|
|||
306
src/controller/testsuite/test_persist_snapshot_entities.c
Normal file
306
src/controller/testsuite/test_persist_snapshot_entities.c
Normal file
|
|
@ -0,0 +1,306 @@
|
|||
/*
|
||||
* test_persist_snapshot_entities.c — entity-bearing checkpoint test suite
|
||||
* (ha-entity-snapshots).
|
||||
*
|
||||
* Task 1.1 (Requirements 1.1, 2.1): a reusable test fixture wrapping a real
|
||||
* persist_t embedded in a minimal test cluster_t whose jobmgr/qmgr tables are
|
||||
* seeded with N jobs (varying state) and M queues (varying status) — the
|
||||
* foundation later tasks in this file build on:
|
||||
* - 2.1/2.2: write_snapshot/load_snapshot job round-trip, per-entity
|
||||
* consistency, partial-write discard
|
||||
* - 3.1/3.2: queue admin-status round-trip
|
||||
* - 4.2: rotation-guard (in-flight checkpoint survives rotate_snapshots)
|
||||
* - 5.1/5.2: restore-before-advance, failure-safety, legacy-format handling
|
||||
*
|
||||
* Modeled on:
|
||||
* - test_persist_hooks.c's make_hook_test_persist/cleanup_hook_test_persist
|
||||
* (bare persist_t over a private tmpdir, no HA/commit-engine wiring)
|
||||
* - test_commit_barrier_reply.c's cbrp_setup / test_egress_filter.c's
|
||||
* ef_setup (persist_t embedded in a real cluster_t with jobmgr/qmgr wired
|
||||
* as xkvmgr instances, back-pointer registered via persist_init so
|
||||
* persist_start can register hooks)
|
||||
*
|
||||
* Deliberately OMITTED vs. cbrp_setup/ef_setup: commit engine, standby
|
||||
* registry, repl_broadcast_send_hook, and the LSN-0 priming append. None of
|
||||
* those are needed here — create_job/makeJob and makeQueue insert directly
|
||||
* into the cluster's tables with no WAL append or replication involved, and
|
||||
* write_snapshot/persist_checkpoint never call repl_broadcast (checkpoint
|
||||
* markers are local-only; see persist.c persist_write_checkpoint_marker).
|
||||
* Adding that machinery here would be unused scaffolding.
|
||||
*
|
||||
* job_t/queue_t insertion mirrors the established test idiom:
|
||||
* - ef_make_job (test_egress_filter.c): zero an xce_job_t, set
|
||||
* jobid/state/submit, call makeJob(jobmgr, &jdef) — makeJob deep-copies
|
||||
* the submit payload (jobj.c job_init -> makeJobsub), so a single reused
|
||||
* scratch job_submit_data_t is safe across a seeding loop.
|
||||
* - cbrp_arrange_queuectrl (test_commit_barrier_reply.c): zero an
|
||||
* xce_queue_t, set queue (name) + status, call makeQueue(qmgr, &qdef) —
|
||||
* queue_init deep-copies the name via makeString, and the underlying
|
||||
* string-keyed kvtab strdup's the key on insert (kv.c assign_string_key),
|
||||
* so a single reused scratch name buffer is likewise safe.
|
||||
*
|
||||
* NOTE (design.md / bug investigation 2026-06-30): queues are config-
|
||||
* provisioned at startup, not created from WAL/snapshot data — restore_queue_state
|
||||
* can only re-apply status to a queue that already exists in qmgr. Seeding
|
||||
* queues directly here (as makeQueue does) stands in for that config-load
|
||||
* step, matching what a real config-provisioned queue table looks like by
|
||||
* the time write_snapshot/load_snapshot ever touch it.
|
||||
*/
|
||||
|
||||
#define _GNU_SOURCE
|
||||
#include <rtti.h>
|
||||
|
||||
#include <stdint.h>
|
||||
#include <stdio.h>
|
||||
#include <stdlib.h>
|
||||
#include <string.h>
|
||||
#include <unistd.h>
|
||||
|
||||
#include <unity/unity.h>
|
||||
|
||||
#include "cluster.h"
|
||||
#include "jobj.h"
|
||||
#include "qobj.h"
|
||||
#include "xce/cluster_def.h"
|
||||
#include "xce/types.h"
|
||||
|
||||
/* ------------------------------------------------------------------ */
|
||||
/* Deterministic seed data — later tests predict seeded values from the */
|
||||
/* loop index via these helpers rather than the fixture returning arrays */
|
||||
/* ------------------------------------------------------------------ */
|
||||
|
||||
#define SNAPENT_JOBID_BASE (1000ULL)
|
||||
|
||||
static const unsigned int g_snapent_job_states[] = {
|
||||
JOB_STAT_PEND, JOB_STAT_RUN, JOB_STAT_EXIT, JOB_STAT_DONE, JOB_STAT_PSUSP,
|
||||
};
|
||||
#define SNAPENT_N_JOB_STATES \
|
||||
(sizeof(g_snapent_job_states) / sizeof(g_snapent_job_states[0]))
|
||||
|
||||
static const uint32_t g_snapent_queue_statuses[] = {
|
||||
QUEUE_STAT_OPEN, QUEUE_STAT_CLOSED, QUEUE_STAT_INACT_ADMIN,
|
||||
(QUEUE_STAT_CLOSED | QUEUE_STAT_INACT_ADMIN),
|
||||
};
|
||||
#define SNAPENT_N_QUEUE_STATUSES \
|
||||
(sizeof(g_snapent_queue_statuses) / sizeof(g_snapent_queue_statuses[0]))
|
||||
|
||||
static uint64_t snapent_jobid(int i)
|
||||
{
|
||||
return SNAPENT_JOBID_BASE + (uint64_t)i;
|
||||
}
|
||||
|
||||
static unsigned int snapent_job_state(int i)
|
||||
{
|
||||
return g_snapent_job_states[i % (int)SNAPENT_N_JOB_STATES];
|
||||
}
|
||||
|
||||
static void snapent_queue_name(int i, char* buf, size_t buf_sz)
|
||||
{
|
||||
snprintf(buf, buf_sz, "snapent-q%d", i);
|
||||
}
|
||||
|
||||
static uint32_t snapent_queue_status(int i)
|
||||
{
|
||||
return g_snapent_queue_statuses[i % (int)SNAPENT_N_QUEUE_STATUSES];
|
||||
}
|
||||
|
||||
/* ------------------------------------------------------------------ */
|
||||
/* Fixture: persist_t + cluster_t with seeded job/queue tables */
|
||||
/* ------------------------------------------------------------------ */
|
||||
|
||||
typedef struct {
|
||||
cluster_t* c;
|
||||
char tmpdir[256];
|
||||
} snapent_fixture_t;
|
||||
|
||||
/* jobj.c's jobsub/startinfo tables are process-global and idempotently
|
||||
* guarded — mirror the per-file static flag used by test_egress_filter.c /
|
||||
* test_commit_barrier_reply.c so this file can run standalone or alongside
|
||||
* them in the same controller_unit_test binary. */
|
||||
static int g_snapent_jobobj_ready;
|
||||
|
||||
static void snapent_seed_jobs(cluster_t* c, int n)
|
||||
{
|
||||
static job_submit_data_t submit_scratch;
|
||||
|
||||
for (int i = 0; i < n; i++) {
|
||||
xce_job_t jdef;
|
||||
memset(&jdef, 0, sizeof(jdef));
|
||||
memset(&submit_scratch, 0, sizeof(submit_scratch));
|
||||
|
||||
jdef.jobid = snapent_jobid(i);
|
||||
jdef.state = snapent_job_state(i);
|
||||
jdef.submit = &submit_scratch;
|
||||
|
||||
TEST_ASSERT_NOT_NULL_MESSAGE(makeJob(c->jobmgr, &jdef),
|
||||
"seed job insertion must succeed");
|
||||
}
|
||||
}
|
||||
|
||||
static void snapent_seed_queues(cluster_t* c, int m)
|
||||
{
|
||||
for (int i = 0; i < m; i++) {
|
||||
char name[32];
|
||||
snapent_queue_name(i, name, sizeof(name));
|
||||
|
||||
xce_queue_t qdef;
|
||||
memset(&qdef, 0, sizeof(qdef));
|
||||
qdef.queue = name; /* deep-copied via makeString in queue_init */
|
||||
qdef.status = snapent_queue_status(i);
|
||||
|
||||
TEST_ASSERT_NOT_NULL_MESSAGE(makeQueue(c->qmgr, &qdef),
|
||||
"seed queue insertion must succeed");
|
||||
}
|
||||
}
|
||||
|
||||
/* make an entity-snapshot fixture: a real persist_t (private tmpdir) embedded
|
||||
* in a minimal cluster_t whose jobmgr/qmgr are seeded with n_jobs/m_queues.
|
||||
* persist_init registers the cluster back-pointer (p->cluster) so future
|
||||
* write_snapshot/load_snapshot entity collection can walk c->jobmgr/c->qmgr —
|
||||
* the same access path collect_jobs/collect_queues (dataex.c) use. */
|
||||
static snapent_fixture_t* snapent_make(int n_jobs, int m_queues)
|
||||
{
|
||||
kvmgr_params_t int_params = { .key_type = KVMGR_KEY_INT,
|
||||
.nslots = 64,
|
||||
.delta = 16 };
|
||||
kvmgr_params_t str_params = { .key_type = KVMGR_KEY_STRING,
|
||||
.nslots = 64,
|
||||
.delta = 16 };
|
||||
|
||||
snapent_fixture_t* f = calloc(1, sizeof(*f));
|
||||
if (f == NULL)
|
||||
return NULL;
|
||||
|
||||
f->c = calloc(1, sizeof(cluster_t));
|
||||
if (f->c == NULL) {
|
||||
free(f);
|
||||
return NULL;
|
||||
}
|
||||
|
||||
f->c->myhost = &f->c->myhost_info;
|
||||
f->c->myhost_info.name = (char*)"snapent-test";
|
||||
f->c->myhost_info.key = (char*)"";
|
||||
|
||||
TEST_ASSERT_EQUAL_INT(
|
||||
0, xce_hook_init(&f->c->hooksys, XCE_CONTROLLER_HOOK_MAX));
|
||||
|
||||
f->c->jobmgr = makeInstance(&xkvmgr_type, &int_params);
|
||||
f->c->qmgr = makeInstance(&xkvmgr_type, &str_params);
|
||||
TEST_ASSERT_NOT_NULL(f->c->jobmgr);
|
||||
TEST_ASSERT_NOT_NULL(f->c->qmgr);
|
||||
zarray_init(&f->c->sharedinst, 64, NULL);
|
||||
|
||||
if (!g_snapent_jobobj_ready) {
|
||||
TEST_ASSERT_EQUAL_INT(0, job_obj_init(64, 64));
|
||||
g_snapent_jobobj_ready = 1;
|
||||
}
|
||||
|
||||
persist_config_t cfg = persist_config_defaults();
|
||||
cfg.wal_sync_ms = 5000; /* slow background fsync — avoid races */
|
||||
|
||||
snprintf(f->tmpdir, sizeof(f->tmpdir), "/tmp/xce_snapent_XXXXXX");
|
||||
TEST_ASSERT_NOT_NULL(mkdtemp(f->tmpdir));
|
||||
persist_set_workdir(&f->c->persist, f->tmpdir, "snapent");
|
||||
TEST_ASSERT_EQUAL_INT(0, persist_init(&f->c->persist, &cfg, f->c));
|
||||
TEST_ASSERT_EQUAL_INT(0, persist_start(&f->c->persist));
|
||||
|
||||
snapent_seed_jobs(f->c, n_jobs);
|
||||
snapent_seed_queues(f->c, m_queues);
|
||||
|
||||
return f;
|
||||
}
|
||||
|
||||
static void snapent_teardown(snapent_fixture_t* f)
|
||||
{
|
||||
if (f == NULL)
|
||||
return;
|
||||
|
||||
persist_stop(&f->c->persist);
|
||||
persist_fini(&f->c->persist);
|
||||
|
||||
zarray_fini(&f->c->sharedinst);
|
||||
if (f->c->qmgr) dereference(f->c->qmgr);
|
||||
if (f->c->jobmgr) dereference(f->c->jobmgr);
|
||||
xce_hook_fini(&f->c->hooksys);
|
||||
|
||||
free(f->c);
|
||||
free(f);
|
||||
}
|
||||
|
||||
/* Lookup helpers — mirror ef_find_job (test_egress_filter.c). */
|
||||
static job_t* snapent_find_job(cluster_t* c, uint64_t jobid)
|
||||
{
|
||||
job_t* j = NULL;
|
||||
int found = 0;
|
||||
|
||||
lock(c->jobmgr, RTTI_LOCK_READ);
|
||||
found = key_find(c->jobmgr, (void*)jobid, (void**)&j);
|
||||
unlock(c->jobmgr);
|
||||
|
||||
return (found > 0) ? j : NULL;
|
||||
}
|
||||
|
||||
static queue_t* snapent_find_queue(cluster_t* c, const char* name)
|
||||
{
|
||||
queue_t* q = NULL;
|
||||
int found = 0;
|
||||
|
||||
lock(c->qmgr, RTTI_LOCK_READ);
|
||||
found = key_find(c->qmgr, (void*)name, (void**)&q);
|
||||
unlock(c->qmgr);
|
||||
|
||||
return (found > 0) ? q : NULL;
|
||||
}
|
||||
|
||||
/* ------------------------------------------------------------------ */
|
||||
/* Test 1: fixture seeds N jobs (varying state) + M queues (varying */
|
||||
/* status) and both are independently observable via key_find */
|
||||
/* ------------------------------------------------------------------ */
|
||||
|
||||
static void test_snapent_fixture_seeds_jobs_and_queues(void)
|
||||
{
|
||||
const int n_jobs = 5;
|
||||
const int m_queues = 4;
|
||||
|
||||
snapent_fixture_t* f = snapent_make(n_jobs, m_queues);
|
||||
TEST_ASSERT_NOT_NULL(f);
|
||||
TEST_ASSERT_NOT_NULL(f->c);
|
||||
|
||||
/* persist_init must have registered the cluster back-pointer — this is
|
||||
* what lets write_snapshot/load_snapshot reach c->jobmgr/c->qmgr. */
|
||||
TEST_ASSERT_EQUAL_PTR(f->c, f->c->persist.cluster);
|
||||
|
||||
for (int i = 0; i < n_jobs; i++) {
|
||||
job_t* j = snapent_find_job(f->c, snapent_jobid(i));
|
||||
TEST_ASSERT_NOT_NULL_MESSAGE(j, "seeded job must be present in jobmgr");
|
||||
TEST_ASSERT_EQUAL_UINT64(snapent_jobid(i), j->job.jobid);
|
||||
TEST_ASSERT_EQUAL_UINT32(snapent_job_state(i), j->job.state);
|
||||
}
|
||||
|
||||
for (int i = 0; i < m_queues; i++) {
|
||||
char name[32];
|
||||
snapent_queue_name(i, name, sizeof(name));
|
||||
|
||||
queue_t* q = snapent_find_queue(f->c, name);
|
||||
TEST_ASSERT_NOT_NULL_MESSAGE(q, "seeded queue must be present in qmgr");
|
||||
TEST_ASSERT_EQUAL_UINT32(snapent_queue_status(i), q->queue.status);
|
||||
}
|
||||
|
||||
/* Sanity: lookups for entities that were never seeded must miss. */
|
||||
TEST_ASSERT_NULL(snapent_find_job(f->c, snapent_jobid(n_jobs) + 999));
|
||||
TEST_ASSERT_NULL(snapent_find_queue(f->c, "snapent-does-not-exist"));
|
||||
|
||||
snapent_teardown(f);
|
||||
printf("test_snapent_fixture_seeds_jobs_and_queues: PASS\n");
|
||||
}
|
||||
|
||||
/* ------------------------------------------------------------------ */
|
||||
/* Suite entry point */
|
||||
/* ------------------------------------------------------------------ */
|
||||
|
||||
void test_persist_snapshot_entities(void)
|
||||
{
|
||||
printf("\n>> entity-bearing snapshot fixture tests\n\n");
|
||||
|
||||
RUN_TEST(test_snapent_fixture_seeds_jobs_and_queues);
|
||||
}
|
||||
Loading…
Add table
Add a link
Reference in a new issue