test(ha-entity-snapshots): RED tests for load_snapshot restore contract (task 5.1)

Extends persist_load_snapshot's public signature with an
out_entities_restored bool*, per design.md's documented contract. The
static load_snapshot function -- the real restore logic -- is completely
untouched (still today's watermark-only implementation, byte-identical
to before this task); only a thin wrapper was added that naively maps
*out_entities_restored = (rc==0), deliberately scaffolding so the 3 new
RED tests fail for genuine reasons rather than accidentally passing.
ha_handle_snapshot_chunk's sole call site gets a mechanical
declare-and-pass update -- confirmed inert, never reads the new
out-param (task 6.1's job).

Three tests against persist_load_snapshot directly (unlike prior tasks'
tests, which deliberately stayed on the raw-byte-parsing side to avoid
this exact function): restore-before-advance (fails at the job-lookup
assertion -- nothing is ever restored today), corrupted-entity-leaves-
position-unchanged (fails at rc!=0 -- today's code ignores the corrupted
section entirely, since load_snapshot never reads past the legacy
header), legacy-file-reports-position-only (fails at entities_restored==
false -- the naive wrapper claims restoration for every rc==0 case,
including legacy files).

This is the exact function/call site that caused task 2.2's rejection
(a live position-advances-before-restore-validates hazard). Independent
review verified load_snapshot's body is byte-identical to commit
d52eb94, the wrapper does nothing beyond the naive mapping, and ha.c's
change never reads the new out-param anywhere -- no repeat of that
mistake here.

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 07:56:57 +00:00
parent d52eb944c2
commit a8e300d96a
5 changed files with 336 additions and 7 deletions

View file

@ -70,7 +70,7 @@
- _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
- [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
- Assert a deliberately corrupted entity record causes the load to fail with the member's
@ -133,3 +133,4 @@
- (3.2) Pre-existing ASan leak in the test fixture itself (`snapent_make` -> `makeInstance`/`makeJob`/`makeQueue`/`xkvmgr_add`, ~18KB/113 allocations), unrelated to any production code -- present since task 1.1, masked by the project's standing `ASAN_OPTIONS=detect_leaks=0` convention for this suite. Not blocking any task; worth a dedicated fixture-teardown cleanup pass if this test file keeps growing.
- (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.

View file

@ -2593,8 +2593,13 @@ int ha_handle_snapshot_chunk(ha_state_t *ha, persist_t *p,
return -err;
}
/* Load the snapshot — sets p->checkpoint_lsn and p->current_lsn */
int rc = persist_load_snapshot(p, final_path, chunk->snapshot_lsn);
/* Load the snapshot — sets p->checkpoint_lsn and p->current_lsn.
* ha-entity-snapshots task 5.1: call-site signature compat only for the
* new out-param — entities_restored itself is not yet consumed here
* (that is task 6.1's job, once 5.2 makes the flag meaningful). */
bool entities_restored = false;
int rc = persist_load_snapshot(p, final_path, chunk->snapshot_lsn,
&entities_restored);
if (rc != 0) {
ERROR("ha_handle_snapshot_chunk: persist_load_snapshot failed rc=%d", rc);
return rc;

View file

@ -2204,10 +2204,35 @@ static int load_snapshot(persist_t* p, const char* path, uint64_t lsn)
return 0;
}
/* Public wrapper for load_snapshot — exposed via persist.h for ha.c (Task 3.5c). */
int persist_load_snapshot(persist_t *p, const char *path, uint64_t lsn)
/* 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. */
int persist_load_snapshot(persist_t *p, const char *path, uint64_t lsn,
bool *out_entities_restored)
{
return load_snapshot(p, path, lsn);
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;
}
/* P2-2: Standby WAL replay — apply a single replicated entry to in-memory state.

View file

@ -996,6 +996,290 @@ static void test_snapent_rotation_preserves_in_flight_checkpoint(void)
printf("test_snapent_rotation_preserves_in_flight_checkpoint: PASS\n");
}
/* ───────────────────────────────────────────────────────────────────────
* ha-entity-snapshots Task 5.1 (Req 2.1, 2.2, 2.3, 2.4, 5.1, 5.2):
* restore-before-advance, failure-safety, and legacy-format RED tests.
*
* Unlike every test above (which deliberately parses write_snapshot's raw
* bytes to stay clear of load_snapshot's not-yet-implemented restore
* behavior — task 2.2 remediation note), these three tests call
* persist_load_snapshot DIRECTLY: it is the exact function under test for
* this task (design.md "load_snapshot (extended)").
*
* They are written against persist_load_snapshot's NEW 4-argument
* signature (with the out_entities_restored parameter) even though
* load_snapshot's internal restore behavior is still today's
* watermark-only implementation — see persist.c's persist_load_snapshot
* wrapper comment for exactly why that stopgap wrapper makes all three of
* these genuinely fail (RED) for the right reason, not a compile error:
* 1. entities are never actually restored into jobmgr/qmgr;
* 2. a corrupted entity frame is never inspected, so failure is never
* detected and the position is wrongly advanced anyway;
* 3. a legacy (position-only) file is falsely reported as fully
* restored (entities_restored=true) instead of false — the exact
* Requirement 2.4 hazard this feature exists to close.
* ─────────────────────────────────────────────────────────────────────── */
/* A status value outside every value in g_snapent_queue_statuses[] (which
* only ever sets bits 0x00/0x01/0x04) -- so a post-restore match against the
* checkpoint's real status can only be explained by a genuine
* restore_queue_state call, never by coincidental pre-existing state. */
#define SNAPENT_PRE_RESTORE_QUEUE_STATUS 0xDEAD0000u
static void snapent_force_queue_status(cluster_t* c, const char* name,
uint32_t status)
{
queue_t* q = snapent_find_queue(c, name);
TEST_ASSERT_NOT_NULL(q);
lock(q, RTTI_LOCK_WRITE);
q->queue.status = status;
unlock(q);
}
/* ------------------------------------------------------------------ */
/* Test 7 (task 5.1, RED): a checkpoint with N jobs + M queues, loaded via */
/* persist_load_snapshot into a SEPARATE destination member, must restore */
/* every entity into the destination's live jobmgr/qmgr, report */
/* entities_restored=true, and only then reflect the checkpoint's position */
/* (Requirements 2.1, 2.2). */
/* ------------------------------------------------------------------ */
static void test_snapent_load_snapshot_restores_entities_before_advancing_position(void)
{
const int n_jobs = 3;
const int m_queues = 2;
/* Source: produces the entity-bearing checkpoint file (a real, already-
* working write_snapshot, tasks 2.2/3.2). */
snapent_fixture_t* src = snapent_make(n_jobs, m_queues);
TEST_ASSERT_NOT_NULL(src);
TEST_ASSERT_EQUAL_INT(0, persist_checkpoint(&src->c->persist));
uint64_t expected_lsn = src->c->persist.checkpoint_lsn;
char path[4096];
snprintf(path, sizeof(path), "%s/snap-%016" PRIx64 ".capnp",
src->c->persist.snap_path, expected_lsn);
/* Destination: models a rejoining/far-behind member. jobmgr starts
* EMPTY -- jobs are inserted FRESH by restore_job (job.c:96), never
* pre-provisioned (unlike queues). qmgr has the SAME queue names
* pre-provisioned (restore_queue_state, queue.c:208, returns -ENOENT
* for a queue that doesn't already exist -- queues are config-owned,
* per design.md), but seeded with a status that deliberately does NOT
* match the checkpoint's real value, so a post-load match can only be
* explained by a genuine restore_queue_state call. */
snapent_fixture_t* dst = snapent_make(0, m_queues);
TEST_ASSERT_NOT_NULL(dst);
for (int i = 0; i < m_queues; i++) {
char name[32];
snapent_queue_name(i, name, sizeof(name));
snapent_force_queue_status(dst->c, name, SNAPENT_PRE_RESTORE_QUEUE_STATUS);
}
/* Sanity: destination truly starts without any of the source's jobs. */
for (int i = 0; i < n_jobs; i++)
TEST_ASSERT_NULL(snapent_find_job(dst->c, snapent_jobid(i)));
bool entities_restored = false;
int rc = persist_load_snapshot(&dst->c->persist, path, expected_lsn,
&entities_restored);
TEST_ASSERT_EQUAL_INT_MESSAGE(0, rc,
"loading a well-formed entity-bearing checkpoint must succeed");
TEST_ASSERT_TRUE_MESSAGE(entities_restored,
"a checkpoint carrying job/queue entities must report "
"entities_restored=true (Requirement 2.1, 2.2)");
/* Requirement 2.1/2.2: every job restored into the live jobmgr,
* field-identical to what was checkpointed. */
for (int i = 0; i < n_jobs; i++) {
char expect_queue[48], expect_user[48], expect_cmdline[96];
snapent_job_queue_name(i, expect_queue, sizeof(expect_queue));
snapent_job_user(i, expect_user, sizeof(expect_user));
snapent_job_cmdline(i, expect_cmdline, sizeof(expect_cmdline));
job_t* j = snapent_find_job(dst->c, snapent_jobid(i));
TEST_ASSERT_NOT_NULL_MESSAGE(j,
"every job in the checkpoint must be restored into the "
"member's live jobmgr before the position advances");
TEST_ASSERT_EQUAL_UINT64(snapent_jobid(i), j->job.jobid);
TEST_ASSERT_EQUAL_UINT32(snapent_job_state(i), j->job.state);
TEST_ASSERT_NOT_NULL(j->job.submit);
TEST_ASSERT_EQUAL_STRING(expect_queue, j->job.submit->queue);
TEST_ASSERT_EQUAL_STRING(expect_user, j->job.submit->user);
TEST_ASSERT_NOT_NULL(j->job.submit->start_info);
TEST_ASSERT_EQUAL_STRING(expect_cmdline,
j->job.submit->start_info->cmdline);
}
/* Requirement 1.1/2.1/2.2: every queue's admin status restored --
* matches the checkpoint's real value, not the pre-restore sentinel. */
for (int i = 0; i < m_queues; i++) {
char name[32];
snapent_queue_name(i, name, sizeof(name));
queue_t* q = snapent_find_queue(dst->c, name);
TEST_ASSERT_NOT_NULL(q);
TEST_ASSERT_EQUAL_UINT32_MESSAGE(snapent_queue_status(i), q->queue.status,
"restore_queue_state must have been called with the "
"checkpoint's real status, not left at the pre-restore sentinel");
}
/* Requirement 2.1/2.2: the position reflecting "caught up" must hold
* TOGETHER with the entities actually being present -- both checked
* above must be simultaneously true of the end state. */
TEST_ASSERT_EQUAL_UINT64(expected_lsn, dst->c->persist.checkpoint_lsn);
TEST_ASSERT_EQUAL_UINT64(expected_lsn, dst->c->persist.current_lsn);
snapent_teardown(dst);
snapent_teardown(src);
printf("test_snapent_load_snapshot_restores_entities_before_advancing_position: PASS\n");
}
/* ------------------------------------------------------------------ */
/* Test 8 (task 5.1, RED): a deliberately corrupted entity frame must fail */
/* the load and leave current_lsn/checkpoint_lsn COMPLETELY unchanged -- */
/* not partially advanced, not advanced-then-reverted (Requirement 2.3). */
/* ------------------------------------------------------------------ */
/* Far beyond any small test checkpoint's actual remaining bytes --
* guarantees any correct decode implementation hits a short read / bounds
* failure on this frame, regardless of its exact internal read strategy. */
#define SNAPENT_CORRUPT_JOB_PAYLOAD_LEN (10u * 1024u * 1024u)
/* Byte offset of the FIRST job frame's payload_len field (design.md
* "Checkpoint File Layout (Physical)"): legacy header, then format_version
* (uint32_t), then job_count (uint32_t), then the first frame's own
* payload_len (uint32_t) begins right after. */
#define SNAPENT_FIRST_JOB_PAYLOAD_LEN_OFFSET \
(SNAPENT_LEGACY_HEADER_BYTES + (long)sizeof(uint32_t) /* format_version */ \
+ (long)sizeof(uint32_t) /* job_count */)
static void snapent_corrupt_first_job_payload_len(const char* path)
{
int fd = open(path, O_RDWR);
TEST_ASSERT_TRUE(fd >= 0);
uint32_t bogus = SNAPENT_CORRUPT_JOB_PAYLOAD_LEN;
ssize_t w = pwrite(fd, &bogus, sizeof(bogus),
(off_t)SNAPENT_FIRST_JOB_PAYLOAD_LEN_OFFSET);
TEST_ASSERT_EQUAL_INT((ssize_t)sizeof(bogus), w);
close(fd);
}
static void test_snapent_load_snapshot_corrupted_entity_leaves_position_unchanged(void)
{
const int n_jobs = 2;
snapent_fixture_t* src = snapent_make(n_jobs, 0);
TEST_ASSERT_NOT_NULL(src);
TEST_ASSERT_EQUAL_INT(0, persist_checkpoint(&src->c->persist));
uint64_t checkpoint_lsn = src->c->persist.checkpoint_lsn;
char path[4096];
snprintf(path, sizeof(path), "%s/snap-%016" PRIx64 ".capnp",
src->c->persist.snap_path, checkpoint_lsn);
snapent_corrupt_first_job_payload_len(path);
snapent_fixture_t* dst = snapent_make(0, 0);
TEST_ASSERT_NOT_NULL(dst);
/* Sentinel pre-call position -- distinguishable from both 0 and the
* checkpoint's real position, so "unchanged" is a meaningful assertion
* rather than a 0==0 tautology. */
const uint64_t sentinel_current = 4242;
const uint64_t sentinel_checkpoint = 4141;
dst->c->persist.current_lsn = sentinel_current;
dst->c->persist.checkpoint_lsn = sentinel_checkpoint;
bool entities_restored = true; /* canary -- must not be trusted on rc!=0 */
int rc = persist_load_snapshot(&dst->c->persist, path, checkpoint_lsn,
&entities_restored);
TEST_ASSERT_TRUE_MESSAGE(rc != 0,
"a checkpoint with a corrupted entity frame must fail to load, not "
"silently succeed (Requirement 2.3)");
TEST_ASSERT_EQUAL_UINT64_MESSAGE(sentinel_current, dst->c->persist.current_lsn,
"a failed restore must leave current_lsn COMPLETELY unchanged -- "
"not partially advanced (Requirement 2.3)");
TEST_ASSERT_EQUAL_UINT64_MESSAGE(sentinel_checkpoint, dst->c->persist.checkpoint_lsn,
"a failed restore must leave checkpoint_lsn COMPLETELY unchanged -- "
"not partially advanced (Requirement 2.3)");
/* *out_entities_restored is unspecified on rc!=0 (design.md: "only set
* when rc==0") -- deliberately NOT asserted here. */
snapent_teardown(dst);
snapent_teardown(src);
printf("test_snapent_load_snapshot_corrupted_entity_leaves_position_unchanged: PASS\n");
}
/* ------------------------------------------------------------------ */
/* Test 9 (task 5.1, RED): a checkpoint file predating this feature */
/* (position marker only -- no format_version/job/queue sections, i.e. */
/* the file ends EXACTLY at the legacy header) must load without crashing, */
/* report entities_restored=false, and still advance current_lsn/ */
/* checkpoint_lsn to its recorded position -- the existing, PRESERVED */
/* legacy behavior (Requirements 2.4, 5.1, 5.2). */
/* ------------------------------------------------------------------ */
#define SNAPENT_LEGACY_CHECKPOINT_LSN 555555ULL
/* Hand-builds a pre-ha-entity-snapshots checkpoint file: the exact 57-byte
* legacy sentinel write_snapshot has always written (magic + lsn + optional
* cfgref tail, cfgref_present=0 here) and NOTHING else -- no format_version,
* no job/queue sections. Per design.md, "a file ending exactly at the
* legacy header (no bytes remain) is unambiguously the old format." */
static void snapent_write_legacy_checkpoint(const char* path, uint64_t lsn)
{
int fd = open(path, O_CREAT | O_WRONLY | O_TRUNC, 0600);
TEST_ASSERT_TRUE(fd >= 0);
char header[SNAPENT_LEGACY_HEADER_BYTES];
memset(header, 0, sizeof(header));
memcpy(header, "XCESNAP\0", 8);
memcpy(header + 8, &lsn, 8);
/* header[16] (cfgref_present) left 0 -- no config-ref tail. */
TEST_ASSERT_EQUAL_INT((int)sizeof(header),
(int)write(fd, header, sizeof(header)));
close(fd);
}
static void test_snapent_load_snapshot_legacy_file_reports_position_only(void)
{
snapent_fixture_t* dst = snapent_make(0, 0);
TEST_ASSERT_NOT_NULL(dst);
char path[4096];
snprintf(path, sizeof(path), "%s/snap-legacy.capnp", dst->c->persist.snap_path);
snapent_write_legacy_checkpoint(path, SNAPENT_LEGACY_CHECKPOINT_LSN);
bool entities_restored = true; /* must come back false */
int rc = persist_load_snapshot(&dst->c->persist, path,
SNAPENT_LEGACY_CHECKPOINT_LSN,
&entities_restored);
TEST_ASSERT_EQUAL_INT_MESSAGE(0, rc,
"a legacy (pre-feature) checkpoint must load without error -- a "
"short file is an expected shape, not corruption (Requirement 5.1, 5.2)");
TEST_ASSERT_FALSE_MESSAGE(entities_restored,
"a legacy checkpoint carries no entity content -- it must never "
"falsely claim full restoration (Requirement 2.4)");
TEST_ASSERT_EQUAL_UINT64_MESSAGE(SNAPENT_LEGACY_CHECKPOINT_LSN,
dst->c->persist.checkpoint_lsn,
"a legacy checkpoint still records the committed position it was "
"taken at -- existing, preserved behavior");
TEST_ASSERT_EQUAL_UINT64_MESSAGE(SNAPENT_LEGACY_CHECKPOINT_LSN,
dst->c->persist.current_lsn,
"a legacy checkpoint still advances current_lsn to its recorded "
"position -- existing, preserved behavior");
snapent_teardown(dst);
printf("test_snapent_load_snapshot_legacy_file_reports_position_only: PASS\n");
}
/* ------------------------------------------------------------------ */
/* Suite entry point */
/* ------------------------------------------------------------------ */
@ -1022,4 +1306,7 @@ void test_persist_snapshot_entities(void)
RUN_TEST(test_snapent_checkpoint_mid_encode_failure_discards_whole_file);
RUN_TEST(test_snapent_checkpoint_queue_status_roundtrip);
RUN_TEST(test_snapent_rotation_preserves_in_flight_checkpoint);
RUN_TEST(test_snapent_load_snapshot_restores_entities_before_advancing_position);
RUN_TEST(test_snapent_load_snapshot_corrupted_entity_leaves_position_unchanged);
RUN_TEST(test_snapent_load_snapshot_legacy_file_reports_position_only);
}

View file

@ -650,8 +650,19 @@ int apply_wal_entry(persist_t* p, uint32_t type, uint64_t lsn,
/* Load a snapshot file from path, set p->checkpoint_lsn and p->current_lsn.
* lsn hint is informational — the actual LSN is read from the file header.
*
* ha-entity-snapshots (task 5.2 owns the restore-before-advance
* implementation; this is the public signature it will fill in):
* out_entities_restored reports whether this call actually restored
* job/queue entity state (true) or only observed a legacy position marker
* (false) — set ONLY when this function returns 0. On a non-zero return,
* p->checkpoint_lsn/p->current_lsn are left completely unchanged and
* *out_entities_restored is left untouched (undefined) — callers must not
* read it in that case.
*
* Returns 0 on success, -errno or -EINVAL/-EIO on error. */
int persist_load_snapshot(persist_t* p, const char* path, uint64_t lsn);
int persist_load_snapshot(persist_t* p, const char* path, uint64_t lsn,
bool* out_entities_restored);
/* GC retention predicates (P0-5a / P0-8 / P0-13) */
#include <stdbool.h>