feat(ha-entity-snapshots): restore-before-advance load_snapshot implementation (task 5.2)

Replaces the naive task-5.1 stopgap with the real restore contract:
detects legacy vs entity-bearing checkpoints by remaining file size;
restores every job then every queue status via restore_job/
restore_queue_state, in file order, with bounds-checked length-prefixed
reads; only after every entity restores successfully does
current_lsn/checkpoint_lsn commit to the checkpoint's position. Every
failure path (bounds check, decode, restore error, unrecognized
format_version) returns before that single commit point -- position is
provably untouched on failure, never partially advanced.

Also fixes the pre-existing restore_job decode leak (job.c): job_init
deep-copies submit (never nulled) and MOVE_PTRs allocs/pidinfo/
pend_reasons (nulled on the one path that runs) -- free_Job(&j) is safe
unconditionally on every reachable return path (EEXIST early-return,
create_job success, create_job failure), verified against the exact
generated free_*_ptr null-check guards.

Two pre-existing test_config_repl.c wire-protocol tests needed their
synthetic garbage snapshot bodies updated to a well-formed empty-entity
tail, since load_snapshot now genuinely parses past the legacy header
(previously ignored); their actual assertions (chunk-reassembly
correctness) are unchanged.

This is the exact function/contract that caused task 2.2's rejection
(position advancing before restore validated). Independent review
traced the memory-safety fix through the generated capnp code field-by-
field, confirmed the test_config_repl.c change doesn't weaken any
assertion, and confirmed restore_queue_state's ENOENT-aborts-the-whole-
checkpoint behavior is faithful to design.md's unconditional contract
(flagged as a watch-item for task 7.1's soak, not a defect).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JDYFbrgyDHbvNYcxApKhrX
This commit is contained in:
Rongsong Shen 2026-07-01 08:30:10 +00:00
parent a8e300d96a
commit 9fdb6778d4
4 changed files with 297 additions and 56 deletions

View file

@ -69,7 +69,7 @@
- Observable: the RED test from 4.2 passes
- _Requirements: 3.1, 3.2, 3.3_
- [ ] 5. Core: entity restoration on load
- [x] 5. Core: entity restoration on load
- [x] 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
@ -81,7 +81,7 @@
- _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
- [x] 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
@ -134,3 +134,5 @@
- (4.1) `snapshot_inflight_lsn[peer_idx]` clears to 0 BEFORE `snapshot_inflight[peer_idx]` clears to 0 in every path (completion + all 3 failure sites in `ha_snapshot_send_dispatch`). Task 4.3's `rotate_snapshots` consultation must therefore treat `snapshot_inflight_lsn[i] != 0` as the "protected" signal directly (checkpoint LSNs are never 0), not "check flag==1 first, then trust LSN" -- there's a narrow window where the flag is still 1 but the LSN has already cleared.
- (execution note, after 4.1) Remaining tasks split by risk given a mid-session spend-limit hit: low-risk/small/isolated tasks (4.2, 4.3, 7.1, 8.1) executed directly in the main context with self-review (build + test + boundary/TBD/secrets grep), no subagent dispatch. High-risk tasks (5.1/5.2 `load_snapshot` restore -- the exact function that caused task 2.2's rejection; 6.1 wiring into the live `ha_handle_snapshot_chunk` production path) keep the full implementer + independent-reviewer subagent cycle, since that adversarial review already caught a real live-hazard bug once this spec (task 2.2) and the remaining risk is concentrated there, not spread evenly.
- (5.1) `persist_load_snapshot`'s wrapper currently maps `*out_entities_restored = (rc==0)` -- a deliberately naive stopgap so the 3 RED tests fail for the right reasons. `load_snapshot`'s real body is completely untouched (still watermark-only, byte-identical to before this task). Task 5.2 must replace the wrapper's naive mapping wholesale with real format-detection + restore-then-gate-position logic; it owns turning `load_snapshot` from a no-op-on-entities function into the real restore path.
- (5.2) `restore_queue_state`'s `-ENOENT` ("queue not yet built from config") aborts the ENTIRE checkpoint restore, including already-successfully-restored jobs earlier in the same file -- this is design-compliant (design.md/Req 2.3 are unconditional: any restore failure leaves position unchanged) and not a defect, but it is a real residual availability risk: a benign config-lag on the destination member (a queue the source references not yet locally provisioned) would make catch-up fail outright rather than skip-and-continue. WATCH-ITEM for task 7.1's chaotic soak: if a catch-up round gets stuck/never completes, this exact code path (queue.c's restore_queue_state ENOENT -> persist.c's load_snapshot_entities abort) is the first place to look.
- (5.2) Test-only, non-blocking: the destination fixture's `snapent_teardown` does not release job_t objects added via `restore_job` (only the ones seeded at fixture creation) -- a pre-existing category of test-fixture-teardown gap (same class as tasks.md's (3.2) note), now slightly enlarged since load-path tests exercise restore_job against a destination fixture. Not production code; extend snapent_teardown if this file keeps growing.

View file

@ -118,12 +118,37 @@ int restore_job(cluster_t* c, uint64_t jobid_unused,
lock(c->jobmgr, RTTI_LOCK_READ);
int found = key_find(c->jobmgr, (void*)j.jobid, (void**)&existing);
unlock(c->jobmgr);
if (found > 0 && existing != NULL)
if (found > 0 && existing != NULL) {
/* j was fully decoded above (submit/allocs/pidinfo/pend_reasons all
* populated by decode_Job) and create_job() is never reached on this
* path, so ownership never moved -- every decoded pointer is still
* ours to release here (ha-entity-snapshots task 5.2: fixes the
* pre-existing decode leak on this early-return path, called out in
* tasks.md's Implementation Notes). */
free_Job(&j);
return -EEXIST;
}
job_t* job = create_job(c, &j);
int saved_rc = job ? 0 : -errno;
/* create_job() (jobj.c job_init) takes ownership of j.allocs/j.pidinfo/
* j.pend_reasons via MOVE_PTR (nulling them here on success) and makes
* its OWN deduplicated copies of j.submit/j.ckptinfo (makeJobsub/
* makeString) -- so j.submit, the one field decode_Job always populates
* that job_init never moves, is never released by create_job and must
* always be freed here. free_Job() null-checks each of its four fields
* before freeing (submit/allocs/pidinfo/pend_reasons), so calling it
* unconditionally is safe whether create_job succeeded (three of the
* four already nulled by the MOVE_PTR above) or failed before job_init
* ran any MOVE_PTR (none nulled, all four correctly released) -- never a
* double free either way (ha-entity-snapshots task 5.2, pre-existing
* leak noted in tasks.md's Implementation Notes). */
free_Job(&j);
job_t* job = create_job(c, &j);
if (!job)
return -errno;
return saved_rc;
/* ha-convergence-recovery (3.3): tag the restored job with its submit
* entry's LSN so the egress watermark predicate can decide its commit
* status (job_submit_committed). On a promoted primary, replayed entries

View file

@ -2162,10 +2162,137 @@ done:
/* Recovery internals */
/* ------------------------------------------------------------------ */
/* P0 sentinel format: "XCESNAP\0" (8 bytes) + checkpoint_lsn (8 bytes LE).
* Returns 0 on success and sets p->checkpoint_lsn / p->current_lsn.
* Returns -EINVAL on bad magic, -EIO on short read, -errno on open failure. */
static int load_snapshot(persist_t* p, const char* path, uint64_t lsn)
/* Parse and restore every job frame then every queue frame from the entity
* payload beginning at the current `fd` offset (design.md "Checkpoint File
* Layout"): [job_count][job_count * {payload_len; capnp Job bytes}]
* [queue_count][queue_count * {name_len; name; status}].
*
* `file_size` is the checkpoint file's total size (from fstat) and
* `consumed` is the number of bytes already read from it (legacy header +
* format_version) — every length-prefixed frame's length is bounds-checked
* against the ACTUAL remaining bytes in the file before being trusted for a
* read or allocation, never blindly followed (a corrupt/attacker-controlled
* length must fail fast, not drive an oversized read).
*
* Restores in file order: every job via restore_job, then every queue via
* restore_queue_state (design.md sequence diagram). Returns 0 only if every
* frame restored; on the FIRST failure of any kind (short read, oversized
* length, decode error, or restore error) returns immediately with a
* negative errno — the caller MUST NOT commit any position change on a
* non-zero return (Requirement 2.3).
*
* restore_job's -EEXIST is treated as success, not a failure: it is job.c's
* own documented idempotent-replay guard (job already present), needed so a
* retried load — after an earlier partial attempt already created some jobs
* before failing on a later one — can continue past already-restored entries
* rather than aborting on them again. restore_queue_state has no such
* idempotent-duplicate case (re-applying the same status is already a
* harmless overwrite), so any non-zero return from it is a genuine failure. */
static int load_snapshot_entities(int fd, off_t file_size, off_t consumed,
persist_t* p, uint64_t stored_lsn)
{
uint32_t job_count = 0;
if (read(fd, &job_count, sizeof(job_count)) != (ssize_t)sizeof(job_count))
return -EIO;
consumed += (off_t)sizeof(job_count);
if (job_count > 0 && (p->cluster == NULL || p->cluster->jobmgr == NULL))
return -EINVAL; /* entities present but nowhere to restore them into */
for (uint32_t i = 0; i < job_count; i++) {
uint32_t payload_len = 0;
if (read(fd, &payload_len, sizeof(payload_len)) !=
(ssize_t)sizeof(payload_len))
return -EIO;
consumed += (off_t)sizeof(payload_len);
if (payload_len == 0 || (off_t)payload_len > file_size - consumed)
return -EIO;
uint8_t* buf = (uint8_t*)malloc(payload_len);
if (buf == NULL)
return -ENOMEM;
if (read(fd, buf, payload_len) != (ssize_t)payload_len) {
free(buf);
return -EIO;
}
consumed += (off_t)payload_len;
int rrc = restore_job(p->cluster, 0, buf, payload_len, stored_lsn);
free(buf);
if (rrc != 0 && rrc != -EEXIST)
return rrc;
}
uint32_t queue_count = 0;
if (read(fd, &queue_count, sizeof(queue_count)) !=
(ssize_t)sizeof(queue_count))
return -EIO;
consumed += (off_t)sizeof(queue_count);
if (queue_count > 0 && (p->cluster == NULL || p->cluster->qmgr == NULL))
return -EINVAL; /* entities present but nowhere to restore them into */
for (uint32_t i = 0; i < queue_count; i++) {
uint32_t name_len = 0;
if (read(fd, &name_len, sizeof(name_len)) != (ssize_t)sizeof(name_len))
return -EIO;
consumed += (off_t)sizeof(name_len);
if (name_len == 0 || (off_t)name_len > file_size - consumed)
return -EIO;
char* name = (char*)malloc((size_t)name_len + 1);
if (name == NULL)
return -ENOMEM;
if (read(fd, name, name_len) != (ssize_t)name_len) {
free(name);
return -EIO;
}
name[name_len] = '\0';
consumed += (off_t)name_len;
uint32_t status = 0;
if (read(fd, &status, sizeof(status)) != (ssize_t)sizeof(status)) {
free(name);
return -EIO;
}
consumed += (off_t)sizeof(status);
int qrc = restore_queue_state(p->cluster, name, status);
free(name);
if (qrc != 0)
return qrc;
}
return 0;
}
/* Load a snapshot: legacy header (magic + checkpoint_lsn + optional
* config-ref tail), then — for an entity-bearing checkpoint — every job and
* queue frame that follows it (ha-entity-snapshots task 5.2, Requirements
* 2.1-2.4, 5.1, 5.2). See design.md "load_snapshot (extended)" and its
* sequence diagram for the exact contract this implements.
*
* Ordering (Requirement 2.1, 2.2, 2.3 — restore FIRST, validate FULLY, THEN
* advance position, never the reverse, never partial): the header and any
* config-ref tail are parsed into LOCAL variables only. Format is then
* detected by whether any bytes remain past the header (design.md's stated
* discriminator — not a flag bit). A short file (nothing remains) is a
* legacy, pre-feature checkpoint: not an error, so its recorded position is
* committed immediately and *out_entities_restored is false. Otherwise every
* job then every queue frame is restored via load_snapshot_entities() before
* ANYTHING is written to p->checkpoint_lsn/p->current_lsn/p->cfgref_*; on
* any restore failure this function returns non-zero with p's fields
* PROVABLY untouched (the single commit point below is only ever reached
* after load_snapshot_entities() returns 0). *out_entities_restored is only
* set on a rc==0 return, matching persist.h's documented contract.
*
* Returns 0 on success, -EINVAL on bad magic / unrecognized entity format,
* -EIO on a short/corrupt read, -ENOMEM on allocation failure, -errno on
* open/fstat failure, or a restore_job/restore_queue_state error code. */
static int load_snapshot(persist_t* p, const char* path, uint64_t lsn,
bool* out_entities_restored)
{
(void)lsn; /* lsn is encoded in the file itself — we re-read it */
@ -2173,66 +2300,116 @@ static int load_snapshot(persist_t* p, const char* path, uint64_t lsn)
if (fd < 0)
return -errno;
struct stat st;
if (fstat(fd, &st) != 0) {
int saved = errno;
close(fd);
return -saved;
}
/* magic(8) + checkpoint_lsn(8) + optional config-ref tail(1+8+32). */
char header[16 + 1 + 8 + 32];
ssize_t n = read(fd, header, sizeof(header));
close(fd);
if (n < 16)
if (n < 16) {
close(fd);
return -EIO;
}
if (memcmp(header, "XCESNAP\0", 8) != 0)
if (memcmp(header, "XCESNAP\0", 8) != 0) {
close(fd);
return -EINVAL;
}
/* Extract checkpoint_lsn from bytes 8-15 (little-endian on this arch) */
/* Extract checkpoint_lsn from bytes 8-15 (little-endian on this arch).
* Held LOCALLY -- not yet written to p-> -- until the single commit
* point below (legacy short-circuit, or after every entity restores). */
uint64_t stored_lsn;
memcpy(&stored_lsn, header + 8, 8);
/* Optional committed-config reference (ha-term-safety task 4.2 r1):
* present only when the sentinel carries the 41-byte tail. Also held
* locally until the same commit point. */
int cfgref_present = 0;
uint64_t cfgref_tag = 0;
uint8_t cfgref_sha[32];
memset(cfgref_sha, 0, sizeof(cfgref_sha));
if (n >= 16 + 1 + 8 + 32 && header[16] != 0) {
cfgref_present = 1;
memcpy(&cfgref_tag, header + 17, 8);
memcpy(cfgref_sha, header + 25, 32);
}
off_t consumed = (off_t)n;
/* Legacy-format detection (Requirement 2.4, 5.1, 5.2): "does more data
* exist" is the discriminator (design.md), not a flag bit -- a file
* ending exactly at the legacy header is unambiguously a pre-feature
* checkpoint. This is an expected rollout shape, not corruption: commit
* the position it records (existing, preserved legacy behavior) and
* report entities_restored=false so the caller falls back to normal
* catch-up rather than treating this checkpoint as fully complete. */
if (st.st_size <= consumed) {
close(fd);
p->checkpoint_lsn = stored_lsn;
p->current_lsn = stored_lsn;
if (cfgref_present) {
p->cfgref_present = 1;
p->cfgref_tag = cfgref_tag;
memcpy(p->cfgref_sha, cfgref_sha, sizeof(p->cfgref_sha));
}
if (out_entities_restored)
*out_entities_restored = false;
return 0;
}
/* Entity-bearing checkpoint: format_version, then job frames, then queue
* frames (design.md "Checkpoint File Layout"). */
uint32_t fmt = 0;
if (read(fd, &fmt, sizeof(fmt)) != (ssize_t)sizeof(fmt)) {
close(fd);
return -EIO;
}
consumed += (off_t)sizeof(fmt);
/* An unrecognized format_version is corruption or a future layout this
* binary cannot read -- fail loud rather than silently treating it as
* legacy (task 5.1 review note: a version mismatch must never be masked
* as an expected legacy shape, or real corruption goes undetected). */
if (fmt != SNAPSHOT_FORMAT_VERSION) {
close(fd);
return -EINVAL;
}
int rc = load_snapshot_entities(fd, st.st_size, consumed, p, stored_lsn);
close(fd);
if (rc != 0)
return rc; /* Requirement 2.3: p->*_lsn / p->cfgref_* untouched */
/* Single commit point (Requirement 2.1, 2.2): reached ONLY after every
* job and queue frame restored successfully above. */
p->checkpoint_lsn = stored_lsn;
p->current_lsn = stored_lsn;
/* Optional committed-config reference (ha-term-safety task 4.2 r1): present
* only when the sentinel carries the 41-byte tail. An older 16-byte sentinel
* (n == 16) leaves the cached ref untouched (cfgref_present stays 0 from
* persist_init), so a member that never checkpointed a config marker simply
* has no fallback reference — the live WAL scan is authoritative. */
if (n >= 16 + 1 + 8 + 32 && header[16] != 0) {
if (cfgref_present) {
p->cfgref_present = 1;
memcpy(&p->cfgref_tag, header + 17, 8);
memcpy(p->cfgref_sha, header + 25, 32);
p->cfgref_tag = cfgref_tag;
memcpy(p->cfgref_sha, cfgref_sha, sizeof(p->cfgref_sha));
}
if (out_entities_restored)
*out_entities_restored = true;
return 0;
}
/* Public wrapper for load_snapshot — exposed via persist.h for ha.c (Task 3.5c).
*
* ha-entity-snapshots task 5.1 (RED, signature-compat stopgap ONLY — task 5.2
* owns the real restore-before-advance implementation): `load_snapshot` above
* is UNCHANGED, still today's watermark-only behavior (never inspects/restores
* any entity payload). This wrapper's *out_entities_restored mapping is
* deliberately the most naive one possible — "the underlying call reported
* success" — precisely so it does NOT mask the two real gaps task 5.2 must
* close: (1) a legacy (position-only) file will report entities_restored=true
* here even though it never carried an entity, since load_snapshot() returns
* 0 for ANY well-formed header regardless of what (if anything) follows it —
* a live instance of the exact false-completeness hazard Requirement 2.4
* exists to prevent; (2) a corrupted entity frame is never even inspected —
* load_snapshot() returns 0 and already advances p->checkpoint_lsn/
* current_lsn before this wrapper is reached, so the "position unchanged on
* failure" contract (Requirement 2.3) does not hold yet either. Both are
* exactly the behaviors the task 5.1 RED tests assert against. */
/* Public wrapper for load_snapshot — exposed via persist.h for ha.c (Task
* 3.5c). load_snapshot() above now owns the full restore-before-advance
* implementation (ha-entity-snapshots task 5.2): this wrapper is a thin
* pass-through, since load_snapshot() already gates *out_entities_restored
* and the p->checkpoint_lsn/current_lsn commit on entity-restore success
* (Requirements 2.1-2.4, 5.1, 5.2). */
int persist_load_snapshot(persist_t *p, const char *path, uint64_t lsn,
bool *out_entities_restored)
{
int rc = load_snapshot(p, path, lsn);
/* Contract: only set on rc==0 (design.md "load_snapshot (extended)" —
* "*out_entities_restored is only set when rc==0"); left untouched
* otherwise so a failed call never overwrites a caller's own state. */
if (rc == 0 && out_entities_restored != NULL)
*out_entities_restored = true;
return rc;
return load_snapshot(p, path, lsn, out_entities_restored);
}
/* P2-2: Standby WAL replay — apply a single replicated entry to in-memory state.
@ -2612,7 +2789,10 @@ int persist_recover(persist_t* p)
/* --- Step 2: load snapshot (if any) --- */
if (snap_lsn > 0 && snap_file[0] != '\0') {
int rc = load_snapshot(p, snap_file, snap_lsn);
/* Local recovery doesn't need the entities-restored indicator: the
* WAL replay in Step 3 applies everything above checkpoint_lsn
* regardless, unchanged by ha-entity-snapshots. */
int rc = load_snapshot(p, snap_file, snap_lsn, NULL);
if (rc != 0) {
ERROR("persist_recover: failed to load snapshot %s: %s",
snap_file, strerror(-rc));

View file

@ -2954,16 +2954,38 @@ static void test_snapshot_recv_two_chunks_assembles_and_renames(void)
/* Build a fake 128-byte snapshot body (two 64-byte chunks).
* payload_bytes contains the raw file bytes for each chunk.
* We skip writing an actual snapshot source file — the receiver
* writes snap.part and renames it; we use the resulting file to verify. */
* writes snap.part and renames it; we use the resulting file to verify.
*
* The body must be a WELL-FORMED entity-bearing checkpoint, not opaque
* filler (ha-entity-snapshots task 5.2): load_snapshot() now actually
* parses everything past the legacy header — a 0xAA/0xBB filler tail
* used to be silently ignored (load_snapshot only ever read the 57-byte
* header) but is now correctly rejected as an unrecognized
* format_version. Layout (design.md "Checkpoint File Layout"): the
* 57-byte legacy header (cfgref_present=0 at byte 16, so bytes 17-56 are
* unread and left zero) + format_version=1 + job_count=0 + queue_count=0
* (69 bytes total), padded with zero filler out to 128 bytes to keep the
* two-64-byte-chunk shape this test exercises — the padding is never
* read (load_snapshot_entities stops right after the queue_count=0
* loop), so its content is immaterial. */
uint64_t snap_lsn = 100;
size_t total_size = 128;
uint8_t body[128];
memset(body, 0, sizeof(body));
memcpy(body, "XCESNAP\0", 8);
memcpy(body + 8, &snap_lsn, 8);
/* body[16] (cfgref_present) left 0 by the memset above. */
uint32_t fake_fmt = 1u; /* must match persist.c's SNAPSHOT_FORMAT_VERSION */
uint32_t fake_job_count = 0u;
uint32_t fake_queue_count = 0u;
memcpy(body + 57, &fake_fmt, sizeof(fake_fmt));
memcpy(body + 61, &fake_job_count, sizeof(fake_job_count));
memcpy(body + 65, &fake_queue_count, sizeof(fake_queue_count));
uint8_t chunk0[64];
uint8_t chunk1[64];
memset(chunk0, 0xAA, 64);
memset(chunk1, 0xBB, 64);
/* Embed XCESNAP header in chunk0 so load_snapshot can parse it */
memcpy(chunk0, "XCESNAP\0", 8);
memcpy(chunk0 + 8, &snap_lsn, 8);
memcpy(chunk0, body, 64);
memcpy(chunk1, body + 64, 64);
persist_t *p = calloc(1, sizeof(persist_t));
TEST_ASSERT_NOT_NULL(p);
@ -3140,9 +3162,21 @@ static void test_snapshot_recv_128kb_two_chunks(void)
TEST_ASSERT_NOT_NULL(chunk0);
TEST_ASSERT_NOT_NULL(chunk1);
/* Embed valid XCESNAP header in chunk0 so persist_load_snapshot succeeds */
/* Embed valid XCESNAP header in chunk0 so persist_load_snapshot succeeds.
* chunk0/chunk1 are otherwise zero-filled, but a well-formed entity-
* bearing checkpoint (ha-entity-snapshots task 5.2) needs format_version
* = SNAPSHOT_FORMAT_VERSION right after the 57-byte legacy header, or
* load_snapshot() correctly rejects an all-zero format_version as
* unrecognized rather than silently ignoring the body as before. The
* job_count=0/queue_count=0 that follow are already satisfied by the
* zero-fill; the rest of the 128KB body is never read (load_snapshot
* stops right after the queue_count=0 loop) so it stays harmless
* filler. */
memcpy(chunk0, "XCESNAP\0", 8);
memcpy(chunk0 + 8, &snap_lsn, 8);
/* chunk0[16] (cfgref_present) already 0 from calloc. */
const uint32_t fake_fmt = 1u; /* must match persist.c's SNAPSHOT_FORMAT_VERSION */
memcpy(chunk0 + 57, &fake_fmt, sizeof(fake_fmt));
/* ── Chunk 0: not last ── */
snapshot_transfer_t c0;