From 5929af1078b8b57998dc78e83b4c7a944d5cca40 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Wed, 1 Jul 2026 06:30:28 +0000 Subject: [PATCH] feat(ha-entity-snapshots): entity-bearing checkpoint write path for jobs (task 2.2) write_snapshot now Cap'n-Proto-encodes every retained job under nested table+per-entity RTTI_LOCK_READ (table lock alone does not exclude a concurrent per-entity writer -- design.md's central correctness finding), appends length-prefixed job frames after a new format_version marker, and fixes a previously-unchecked fdatasync return. Any failure anywhere in the path (encode, write, sync) discards the whole checkpoint -- no partial file at the final path, proven by a real mid-encode-loop fault-injection test (forces one specific job's encode to fail among N, asserts nothing survives). Also fixed two pre-existing bugs in task 1.1's test fixture: terminal-state seeded jobs need a non-zero finish_time or job_should_retain's retention window silently excludes them from any checkpoint; the fixture needs xstring_table_init or every seeded job's submit/start_info silently decodes as NULL. Round 1 of this task also touched load_snapshot/persist.h to make the round-trip tests pass end-to-end, but review caught a live hazard: it advanced current_lsn/checkpoint_lsn BEFORE the entity-decode loop ran, so a decode failure still left the position advanced -- reachable today through the unmodified ha_handle_snapshot_chunk production path, not hypothetical. Remediated by fully reverting load_snapshot/persist.h to byte-identical pre-task-2.2 state and rewriting the round-trip tests to parse write_snapshot's raw on-disk bytes directly (read_Job/decode_Job) instead of round-tripping through persist_load_snapshot -- proves the write side without depending on load_snapshot, which stays task 5.2's scope entirely, not started here. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01JDYFbrgyDHbvNYcxApKhrX --- .kiro/specs/ha-entity-snapshots/tasks.md | 6 +- src/controller/persist.c | 209 ++++++++++++- .../test_persist_snapshot_entities.c | 278 +++++++++++++++--- src/include/persist.h | 14 + 4 files changed, 453 insertions(+), 54 deletions(-) diff --git a/.kiro/specs/ha-entity-snapshots/tasks.md b/.kiro/specs/ha-entity-snapshots/tasks.md index 780b9b49..74a8c66f 100644 --- a/.kiro/specs/ha-entity-snapshots/tasks.md +++ b/.kiro/specs/ha-entity-snapshots/tasks.md @@ -10,7 +10,7 @@ persist_t+cluster_t with seeded entities - _Requirements: 1.1, 2.1_ -- [ ] 2. Core: entity-bearing checkpoint write path — jobs +- [x] 2. Core: entity-bearing checkpoint write path — jobs - [x] 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 @@ -22,7 +22,7 @@ - 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 +- [x] 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 @@ -127,3 +127,5 @@ - (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. - (2.1) `write_snapshot`'s existing `fdatasync(fd)` call (persist.c:1505) does not check its return value at all — task 2.2 must fix this while adding entity encoding, since design.md's error-handling contract ("any failure at any step -- encode, write, sync -- leaves no file at the final path") requires it. - (2.1) The write-failure-discard test (`test_snapent_checkpoint_write_failure_leaves_no_final_file`) only exercises the pre-existing `open()`-failure branch (via directory removal) and passes today without any entity-encode loop existing yet. Task 2.2 should add a genuine mid-encode-loop fault-injection test (failing to encode e.g. the Nth of N jobs) once the per-job encode loop exists — the current test cannot validate that path. +- (2.2) `restore_job` (job.c) decodes via `decode_Job` → `decode_JobSubmitData_ptr`/`decode_JobStartInfo_ptr` (STRING_DUP'd fields) into a local `xce_job_t j`; that decoded struct is never released via `free_Job(&j)` after `create_job` takes its own copy — a real, pre-existing leak on `restore_job`'s decode path. Not touched by task 2.2 (its `load_snapshot` excursion was fully reverted, see next note) — task 5.2 will reintroduce this call site and must fix the leak there. +- (2.2) First attempt ALSO touched `load_snapshot`/`persist.h` to make task 2.1's round-trip tests pass end-to-end, but review found it advanced `current_lsn`/`checkpoint_lsn` BEFORE the entity-decode loop ran -- a live silent-divergence hazard reachable through the unmodified `ha_handle_snapshot_chunk` production path (reachable today, not hypothetical, since `write_snapshot` now always emits entity-bearing checkpoints). REMEDIATED by fully reverting `load_snapshot`/`persist.h` to pre-2.2 state (byte-identical to commit `0f344da`) and rewriting the round-trip tests to parse `write_snapshot`'s raw on-disk bytes directly (new `snapent_read_checkpoint_jobs` helper using `read_Job`/`decode_Job`) instead of round-tripping through `persist_load_snapshot`. Task 5.2 must implement `load_snapshot`'s entity-decode from scratch per design.md's restore-before-advance-position contract -- do not assume any decode scaffolding exists yet. diff --git a/src/controller/persist.c b/src/controller/persist.c index d7a16869..8c222317 100644 --- a/src/controller/persist.c +++ b/src/controller/persist.c @@ -1256,6 +1256,13 @@ void persist_test_set_force_fsync_fail(persist_t* p, int fail) memory_order_release); } +void persist_test_set_force_job_encode_fail_at(persist_t* p, int index) +{ + if (p) + atomic_store_explicit(&p->test_force_job_encode_fail_at, index + 1, + memory_order_relaxed); +} + unsigned persist_test_replicated_fdatasync_count(persist_t* p) { if (!p) @@ -1472,8 +1479,142 @@ static int persist_write_checkpoint_marker(persist_t* p) return 0; } -/* Write a P0 sentinel snapshot: "XCESNAP\0" + checkpoint_lsn (16 bytes). - * Full Cap'n Proto serialization is Task 11 when recovery needs to read it back. */ +/* Entity-bearing checkpoint payload (ha-entity-snapshots task 2.2, Req + * 1.1-1.4). Physical layout appended after the existing legacy header + * (design.md "Checkpoint File Layout"): + * [uint32_t format_version] + * [uint32_t job_count] + * [job_count * { uint32_t payload_len; uint8_t capnp_Job_bytes[payload_len] }] + * [uint32_t queue_count] + * [queue_count * { ... }] -- written as 0 for now; task 3.2 fills this in + * without needing to touch the job path above. */ +#define SNAPSHOT_FORMAT_VERSION 1u + +typedef struct { + uint8_t* buf; + uint32_t len; +} snap_job_frame_t; + +static void snap_free_job_frames(xlist_t* frames) +{ + for (int i = 0; i < frames->count; i++) { + snap_job_frame_t* fr = (snap_job_frame_t*)xlist_get(frames, i); + if (fr) { + free(fr->buf); + free(fr); + } + } + xlist_fini(frames); +} + +/* Collect + encode every retained job into `frames`, one malloc'd + * snap_job_frame_t per job, under a point-in-time-consistent walk + * (design.md "Existing Architecture Analysis" / Requirement 1.2): the + * jobmgr table's own RTTI_LOCK_READ is held for the ENTIRE walk (excludes + * concurrent insert/delete), and each retained job's own RTTI_LOCK_READ is + * ADDITIONALLY taken nested around just that job's encode call (excludes a + * concurrent per-entity writer for exactly the duration of its encode -- + * the table lock alone does not do this). No lock is held during the + * disk write itself: this function only builds in-memory frames; the + * caller writes them to disk after the table walk has fully released both + * locks. + * + * Filters via job_should_retain (Requirement 1.1) so the checkpoint's + * notion of "live" matches the existing GC retention policy. + * + * Returns 0 on success (frames populated; `frames` is always left + * xlist_init'd -- even zero results -- so the caller can always + * xlist_fini it) or a negative errno on the first encode failure, in which + * case every frame collected so far has already been freed and `frames` + * is empty (Requirement 1.3: a mid-walk failure must not leave partial + * entity content behind). */ +static int collect_job_frames(persist_t* p, xlist_t* frames) +{ + cluster_t* c = p->cluster; + int rc = 0; + int retained_index = 0; + + xlist_init(frames, 64); + + /* No live cluster back-pointer (e.g. a bare unit-test persist_t) -- + * zero jobs is not a failure. */ + if (c == NULL || c->jobmgr == NULL) + return 0; + + lock(c->jobmgr, RTTI_LOCK_READ); + + iterator_t it; + int irc = iter_make(&it, c->jobmgr); + if (irc < 0) { + unlock(c->jobmgr); + return 0; /* nothing to walk -- mirrors collect_jobs' empty-table handling */ + } + + for (;;) { + job_t* j = NULL; + int nrc = iter_next(&it, (void**)&j); + if (nrc < 0 || nrc == RTTI_ITER_NO_DATA) + break; + if (j == NULL) + continue; + if (!job_should_retain(j, &p->config)) + continue; + + uint8_t* buf = NULL; + size_t sz = 0; + int enc_rc; + + lock(j, RTTI_LOCK_READ); +#ifdef XCE_TESTING + int forced = atomic_load_explicit(&p->test_force_job_encode_fail_at, + memory_order_relaxed); + if (forced > 0 && (forced - 1) == retained_index) + enc_rc = -EIO; /* simulated mid-loop encode failure */ + else + enc_rc = capnp_encode_job(&j->job, &buf, &sz); +#else + enc_rc = capnp_encode_job(&j->job, &buf, &sz); +#endif + unlock(j); + + retained_index++; + + if (enc_rc != 0) { + rc = enc_rc; + break; + } + + snap_job_frame_t* fr = (snap_job_frame_t*)malloc(sizeof(*fr)); + if (fr == NULL) { + free(buf); + rc = -ENOMEM; + break; + } + fr->buf = buf; + fr->len = (uint32_t)sz; + if (xlist_store(frames, fr) < 0) { + free(buf); + free(fr); + rc = -ENOMEM; + break; + } + } + + iter_destroy(&it); + unlock(c->jobmgr); + + if (rc != 0) + snap_free_job_frames(frames); /* Requirement 1.3: discard, not partial */ + + return rc; +} + +/* Write an entity-bearing checkpoint: legacy header (unchanged) + entity + * payload (ha-entity-snapshots task 2.2/3.2, Req 1.1-1.4, 3.1-3.3). Any + * failure at any step -- collect, encode, write, sync -- leaves NO file at + * the final path; the tmp file is always unlinked on error. This is a + * retriable-next-checkpoint-cycle failure, never a crash/abort (controller + * never self-terminates on an operational condition). */ static int write_snapshot(persist_t* p) { char tmp_path[4096]; @@ -1484,9 +1625,18 @@ static int write_snapshot(persist_t* p) snprintf(final_path, sizeof(final_path), "%s/snap-%016" PRIx64 ".capnp", p->snap_path, p->checkpoint_lsn); + /* Collect + encode entities BEFORE opening the tmp file: a collection + * failure (Requirement 1.3) then never creates a tmp file at all. */ + xlist_t job_frames; + int rc = collect_job_frames(p, &job_frames); + if (rc != 0) + return rc; + int fd = open(tmp_path, O_CREAT | O_WRONLY | O_TRUNC, 0600); - if (fd < 0) + if (fd < 0) { + snap_free_job_frames(&job_frames); return -errno; + } /* Sentinel: magic (8) + checkpoint_lsn (8) + committed-config reference * (1 present + 8 tag + 32 sha = 41 bytes; ha-term-safety task 4.2 r1). @@ -1501,13 +1651,56 @@ static int write_snapshot(persist_t* p) memcpy(header + 17, &p->cfgref_tag, 8); memcpy(header + 25, p->cfgref_sha, 32); - ssize_t nw = write(fd, header, sizeof(header)); - fdatasync(fd); - close(fd); + if (write(fd, header, sizeof(header)) != (ssize_t)sizeof(header)) + rc = -EIO; - if (nw != (ssize_t)sizeof(header)) { + /* Entity payload: format_version, then job frames (design.md "Checkpoint + * File Layout"). No table/entity lock is held here -- collect_job_frames + * already released both before returning. */ + if (rc == 0) { + uint32_t fmt = SNAPSHOT_FORMAT_VERSION; + if (write(fd, &fmt, sizeof(fmt)) != (ssize_t)sizeof(fmt)) + rc = -EIO; + } + + if (rc == 0) { + uint32_t job_count = (uint32_t)job_frames.count; + if (write(fd, &job_count, sizeof(job_count)) != (ssize_t)sizeof(job_count)) + rc = -EIO; + } + + for (int i = 0; rc == 0 && i < job_frames.count; i++) { + snap_job_frame_t* fr = (snap_job_frame_t*)xlist_get(&job_frames, i); + if (write(fd, &fr->len, sizeof(fr->len)) != (ssize_t)sizeof(fr->len)) { + rc = -EIO; + break; + } + if (write(fd, fr->buf, fr->len) != (ssize_t)fr->len) { + rc = -EIO; + break; + } + } + + /* Queue trailer: task 3.2 populates this with the real collected count + * and frames without needing to touch anything above. Writing an + * explicit 0 now (rather than omitting the section) keeps the on-disk + * layout complete per design.md, so a reader can always expect a + * queue_count word after the job frames. */ + if (rc == 0) { + uint32_t queue_count = 0; + if (write(fd, &queue_count, sizeof(queue_count)) != (ssize_t)sizeof(queue_count)) + rc = -EIO; + } + + if (rc == 0 && fdatasync(fd) != 0) + rc = -errno; + + close(fd); + snap_free_job_frames(&job_frames); + + if (rc != 0) { unlink(tmp_path); - return -EIO; + return rc; } if (rename(tmp_path, final_path) != 0) { diff --git a/src/controller/testsuite/test_persist_snapshot_entities.c b/src/controller/testsuite/test_persist_snapshot_entities.c index 5db19214..6dacf613 100644 --- a/src/controller/testsuite/test_persist_snapshot_entities.c +++ b/src/controller/testsuite/test_persist_snapshot_entities.c @@ -51,6 +51,7 @@ #include #include +#include #include #include #include @@ -65,10 +66,12 @@ #include #include "cluster.h" +#include "controller.capnp.h" #include "jobj.h" #include "qobj.h" #include "xce/cluster_def.h" #include "xce/types.h" +#include "xstring.h" /* ------------------------------------------------------------------ */ /* Deterministic seed data — later tests predict seeded values from the */ @@ -174,6 +177,18 @@ static void snapent_seed_jobs(cluster_t* c, int n) jdef.state = snapent_job_state(i); jdef.submit = &submit_scratch; + /* job_should_retain (persist.c) excludes a TERMINAL job once its + * finish_time + finished_retain_s has elapsed -- a never-set (0) + * finish_time looks like "finished at the epoch", so a freshly + * seeded terminal-state job (EXIT/DONE) would be silently excluded + * from the checkpoint under the default finished_retain_s window. + * Stamp a fresh finish_time so seeded terminal jobs are retained + * exactly like a job that just finished -- matching what + * job_should_retain actually gates on now that write_snapshot + * applies it (ha-entity-snapshots task 2.2). */ + if (jdef.state & (JOB_STAT_EXIT | JOB_STAT_DONE | JOB_STAT_PERR)) + jdef.finish_time = time(NULL); + TEST_ASSERT_NOT_NULL_MESSAGE(makeJob(c->jobmgr, &jdef), "seed job insertion must succeed"); } @@ -337,14 +352,131 @@ static void test_snapent_fixture_seeds_jobs_and_queues(void) } /* ------------------------------------------------------------------ */ -/* Test 2 (task 2.1, RED): checkpoint job content round-trips through */ -/* load_snapshot field-identical (Requirements 1.1, 1.2, 2.1, 2.2). */ +/* Test-local raw checkpoint reader (task 2.2 remediation). */ /* */ -/* Today's write_snapshot (persist.c ~1477) writes ONLY the legacy */ -/* position sentinel (magic + checkpoint_lsn + optional cfgref tail) -- */ -/* zero job/queue entities. A restored checkpoint therefore never */ -/* contains the seeded job, so this test genuinely FAILS against */ -/* current code (RED). */ +/* This task's boundary is write_snapshot ONLY -- load_snapshot's entity */ +/* restore is task 5.x's job (see design.md Requirement 2). The tests */ +/* below must prove write_snapshot's on-disk output is field-identical */ +/* WITHOUT routing through persist_load_snapshot/load_snapshot, so they */ +/* parse the checkpoint's raw bytes directly here and decode each job */ +/* frame with the SAME capnp helpers restore_job (job.c) uses */ +/* (read_Job + decode_Job from controller.capnp.h) -- proving the */ +/* WRITE side without any dependency on the READ side's future behavior. */ +/* */ +/* Byte layout mirrors design.md "Checkpoint File Layout (Physical)": */ +/* [57-byte legacy sentinel: magic(8)+lsn(8)+cfgref tail(1+8+32)] */ +/* [uint32_t format_version][uint32_t job_count] */ +/* [job_count * { uint32_t payload_len; capnp Job bytes }] */ +/* write_snapshot (persist.c) always emits the full 57-byte sentinel */ +/* unconditionally (memset + full memcpy, no short-write branch for an */ +/* absent cfgref), so skipping a fixed 57 bytes here is safe -- unlike */ +/* load_snapshot's OWN reader, which must also tolerate a *shorter* */ +/* 16-byte legacy sentinel from files predating the cfgref tail. */ +/* ------------------------------------------------------------------ */ + +#define SNAPENT_LEGACY_HEADER_BYTES (16 + 1 + 8 + 32) +#define SNAPENT_TEST_FORMAT_VERSION 1u +#define SNAPENT_MAX_DECODE_PAYLOAD (16u * 1024 * 1024) + +/* Decodes every job frame in the checkpoint at `path` into `out` + * (caller-allocated array of `max_out` xce_job_t slots). Returns the + * number of frames decoded (<= job_count on disk, capped at max_out), or + * -1 on any structural read error (bad magic, bad/absent format_version, + * short read, oversized payload_len). Each decoded slot in `out` up to + * the returned count must be released by the caller via free_Job(). */ +static int snapent_read_checkpoint_jobs(const char* path, xce_job_t* out, + int max_out) +{ + int fd = open(path, O_RDONLY); + if (fd < 0) + return -1; + + char header[SNAPENT_LEGACY_HEADER_BYTES]; + if (read(fd, header, sizeof(header)) != (ssize_t)sizeof(header) || + memcmp(header, "XCESNAP\0", 8) != 0) { + close(fd); + return -1; + } + + uint32_t format_version = 0; + if (read(fd, &format_version, sizeof(format_version)) != + (ssize_t)sizeof(format_version) || + format_version != SNAPENT_TEST_FORMAT_VERSION) { + close(fd); + return -1; + } + + uint32_t job_count = 0; + if (read(fd, &job_count, sizeof(job_count)) != (ssize_t)sizeof(job_count)) { + close(fd); + return -1; + } + + int n = 0; + 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)) { + close(fd); + return -1; + } + if (payload_len == 0 || payload_len > SNAPENT_MAX_DECODE_PAYLOAD) { + close(fd); + return -1; + } + + uint8_t* buf = (uint8_t*)malloc(payload_len); + if (buf == NULL) { + close(fd); + return -1; + } + if (read(fd, buf, payload_len) != (ssize_t)payload_len) { + free(buf); + close(fd); + return -1; + } + + if (n < max_out) { + struct capn c_ctx; + capn_init_mem(&c_ctx, buf, payload_len, 0); + Job_ptr jp; + jp.p = capn_getp(capn_root(&c_ctx), 0, 1); + struct Job cj; + memset(&out[n], 0, sizeof(out[n])); + read_Job(&cj, jp); + decode_Job(&out[n], &cj); + capn_free(&c_ctx); + n++; + } + free(buf); + } + + close(fd); + return n; +} + +static xce_job_t* snapent_find_decoded_job(xce_job_t* jobs, int count, + uint64_t jobid) +{ + for (int i = 0; i < count; i++) { + if (jobs[i].jobid == jobid) + return &jobs[i]; + } + return NULL; +} + +static void snapent_free_decoded_jobs(xce_job_t* jobs, int count) +{ + for (int i = 0; i < count; i++) + free_Job(&jobs[i]); +} + +/* ------------------------------------------------------------------ */ +/* Test 2 (task 2.1/2.2): checkpoint job content round-trips through */ +/* write_snapshot's own on-disk bytes field-identical (Requirements 1.1, */ +/* 1.2, 1.4). Parses the raw checkpoint file directly (see reader above)*/ +/* rather than via persist_load_snapshot -- load_snapshot's entity */ +/* restore is task 5.x's boundary, not this task's. */ /* ------------------------------------------------------------------ */ static void test_snapent_checkpoint_job_content_roundtrip(void) @@ -360,15 +492,13 @@ static void test_snapent_checkpoint_job_content_roundtrip(void) snprintf(path, sizeof(path), "%s/snap-%016" PRIx64 ".capnp", src->c->persist.snap_path, src->c->persist.checkpoint_lsn); - /* A second, empty fixture stands in for a rejoining/empty member -- - * load_snapshot must reconstruct the source's entities into THIS - * cluster's jobmgr, not merely advance a position (Requirement 2.1). */ - snapent_fixture_t* dst = snapent_make(0, 0); - TEST_ASSERT_NOT_NULL(dst); - - int rc = persist_load_snapshot(&dst->c->persist, path, - src->c->persist.checkpoint_lsn); - TEST_ASSERT_EQUAL_INT(0, rc); + xce_job_t decoded[8]; + int count = snapent_read_checkpoint_jobs(path, decoded, 8); + TEST_ASSERT_TRUE_MESSAGE(count >= 0, + "checkpoint file must parse as a well-formed entity-bearing " + "format_version=1 payload"); + TEST_ASSERT_EQUAL_INT_MESSAGE(n_jobs, count, + "checkpoint must contain exactly the retained seeded jobs"); for (int i = 0; i < n_jobs; i++) { char expect_queue[48], expect_user[48], expect_cmdline[96]; @@ -376,22 +506,22 @@ static void test_snapent_checkpoint_job_content_roundtrip(void) snapent_job_user(i, expect_user, sizeof(expect_user)); snapent_job_cmdline(i, expect_cmdline, sizeof(expect_cmdline)); - job_t* dj = snapent_find_job(dst->c, snapent_jobid(i)); + xce_job_t* dj = snapent_find_decoded_job(decoded, count, + snapent_jobid(i)); TEST_ASSERT_NOT_NULL_MESSAGE(dj, - "RED: today's write_snapshot writes no entity content, so a " - "restored checkpoint never contains the seeded job"); + "checkpoint's raw job frames must contain the seeded job"); - TEST_ASSERT_EQUAL_UINT64(snapent_jobid(i), dj->job.jobid); - TEST_ASSERT_EQUAL_UINT32(snapent_job_state(i), dj->job.state); - TEST_ASSERT_NOT_NULL(dj->job.submit); - TEST_ASSERT_EQUAL_STRING(expect_queue, dj->job.submit->queue); - TEST_ASSERT_EQUAL_STRING(expect_user, dj->job.submit->user); - TEST_ASSERT_NOT_NULL(dj->job.submit->start_info); + TEST_ASSERT_EQUAL_UINT64(snapent_jobid(i), dj->jobid); + TEST_ASSERT_EQUAL_UINT32(snapent_job_state(i), dj->state); + TEST_ASSERT_NOT_NULL(dj->submit); + TEST_ASSERT_EQUAL_STRING(expect_queue, dj->submit->queue); + TEST_ASSERT_EQUAL_STRING(expect_user, dj->submit->user); + TEST_ASSERT_NOT_NULL(dj->submit->start_info); TEST_ASSERT_EQUAL_STRING(expect_cmdline, - dj->job.submit->start_info->cmdline); + dj->submit->start_info->cmdline); } - snapent_teardown(dst); + snapent_free_decoded_jobs(decoded, count); snapent_teardown(src); printf("test_snapent_checkpoint_job_content_roundtrip: PASS\n"); } @@ -458,9 +588,6 @@ static void test_snapent_checkpoint_concurrent_mutation_not_torn(void) TEST_ASSERT_EQUAL_UINT32((uint32_t)SNAPENT_TORN_PRE_STATE, j0->job.state); TEST_ASSERT_EQUAL_INT(0, (int)j0->job.start_time); - snapent_fixture_t* dst = snapent_make(0, 0); - TEST_ASSERT_NOT_NULL(dst); - snapent_mutator_arg_t marg; marg.c = src->c; marg.jobid = jobid; @@ -480,21 +607,29 @@ static void test_snapent_checkpoint_concurrent_mutation_not_torn(void) snprintf(path, sizeof(path), "%s/snap-%016" PRIx64 ".capnp", src->c->persist.snap_path, src->c->persist.checkpoint_lsn); - int rc = persist_load_snapshot(&dst->c->persist, path, - src->c->persist.checkpoint_lsn); - TEST_ASSERT_EQUAL_INT(0, rc); + /* Parse the raw checkpoint bytes directly (see reader above) -- + * this proves write_snapshot's own encode of the job is never torn, + * independent of load_snapshot's future restore behavior (task + * 5.x's boundary, not this task's). */ + xce_job_t decoded[4]; + int count = snapent_read_checkpoint_jobs(path, decoded, 4); + TEST_ASSERT_TRUE_MESSAGE(count >= 0, + "checkpoint file must parse as a well-formed entity-bearing " + "format_version=1 payload"); - job_t* dj = snapent_find_job(dst->c, jobid); + xce_job_t* dj = snapent_find_decoded_job(decoded, count, jobid); if (dj != NULL) { found_any = true; - bool is_pre = (dj->job.state == (uint32_t)SNAPENT_TORN_PRE_STATE - && dj->job.start_time == 0); - bool is_post = (dj->job.state == (uint32_t)SNAPENT_TORN_POST_STATE - && dj->job.start_time == SNAPENT_TORN_POST_START_TIME); + bool is_pre = (dj->state == (uint32_t)SNAPENT_TORN_PRE_STATE + && dj->start_time == 0); + bool is_post = (dj->state == (uint32_t)SNAPENT_TORN_POST_STATE + && dj->start_time == SNAPENT_TORN_POST_START_TIME); if (!is_pre && !is_post) found_torn = true; } + + snapent_free_decoded_jobs(decoded, count); } pthread_join(th, NULL); @@ -503,14 +638,10 @@ static void test_snapent_checkpoint_concurrent_mutation_not_torn(void) "checkpoint captured a torn mix of job state/start_time -- neither " "fully the pre-transition nor fully the post-transition tuple"); - /* RED: today's write_snapshot writes no entity content at all, so the - * job is never found in any restored checkpoint across the whole loop. */ TEST_ASSERT_TRUE_MESSAGE(found_any, - "a restored checkpoint must contain the seeded job's entity state " - "at least once across the loop -- today's sentinel-only " - "write_snapshot never restores it"); + "a checkpoint must contain the seeded job's entity state at least " + "once across the loop"); - snapent_teardown(dst); snapent_teardown(src); printf("test_snapent_checkpoint_concurrent_mutation_not_torn: PASS\n"); } @@ -575,6 +706,53 @@ static void test_snapent_checkpoint_write_failure_leaves_no_final_file(void) printf("test_snapent_checkpoint_write_failure_leaves_no_final_file: PASS\n"); } +/* ------------------------------------------------------------------ */ +/* Test 5 (task 2.2, GREEN): a genuine mid-encode-loop failure discards */ +/* the WHOLE checkpoint, not just the one job whose encode failed */ +/* (Requirement 1.3). */ +/* */ +/* Unlike test 4 above (which only exercises the pre-existing open() */ +/* failure branch), this drives the NEW per-job encode loop itself via */ +/* the persist_test_set_force_job_encode_fail_at seam -- forcing the */ +/* (0-based) Nth retained job's encode to fail partway through a */ +/* multi-job walk, proving write_snapshot doesn't leave a partial file */ +/* (with only some of the jobs framed) behind on that failure. */ +/* ------------------------------------------------------------------ */ + +static void test_snapent_checkpoint_mid_encode_failure_discards_whole_file(void) +{ + const int n_jobs = 5; + const int fail_at = 2; /* fail a job in the middle of the walk, not the first */ + + snapent_fixture_t* f = snapent_make(n_jobs, 0); + TEST_ASSERT_NOT_NULL(f); + + persist_test_set_force_job_encode_fail_at(&f->c->persist, fail_at); + + int rc = persist_checkpoint(&f->c->persist); + TEST_ASSERT_TRUE_MESSAGE(rc != 0, + "a forced mid-loop job-encode failure must fail the checkpoint"); + + char final_path[4096], tmp_path[4096]; + snprintf(final_path, sizeof(final_path), "%s/snap-%016" PRIx64 ".capnp", + f->c->persist.snap_path, f->c->persist.checkpoint_lsn); + snprintf(tmp_path, sizeof(tmp_path), "%s/snap-%016" PRIx64 ".tmp", + f->c->persist.snap_path, f->c->persist.checkpoint_lsn); + + struct stat st; + TEST_ASSERT_TRUE_MESSAGE(stat(final_path, &st) != 0, + "a mid-encode failure must leave no file at the final checkpoint path"); + TEST_ASSERT_TRUE_MESSAGE(stat(tmp_path, &st) != 0, + "a mid-encode failure must not leave the tmp file behind either"); + + /* Disarm before teardown so no later checkpoint in this process forced + * by teardown/other code paths trips over a stale forced failure. */ + persist_test_set_force_job_encode_fail_at(&f->c->persist, -1); + + snapent_teardown(f); + printf("test_snapent_checkpoint_mid_encode_failure_discards_whole_file: PASS\n"); +} + /* ------------------------------------------------------------------ */ /* Suite entry point */ /* ------------------------------------------------------------------ */ @@ -583,8 +761,20 @@ void test_persist_snapshot_entities(void) { printf("\n>> entity-bearing snapshot fixture tests\n\n"); + /* makeJob's deep-copy path (job_init -> makeJobsub -> makeString) needs + * the global xstring table. In the full suite some other group may have + * already initialised it (e.g. test_worker_join's unguarded call); + * standalone (-t test_persist_snapshot_entities) it is uninitialised and + * every makeString call silently returns NULL -- which job_init does not + * check, so a seeded job's submit/start_info comes back NULL without any + * visible failure at seed time. Probe instead of double-initialising + * (mirrors test_commit_barrier_reply's cbrp-probe pattern). */ + if (makeString((char*)"snapent-probe") == NULL) + xstring_table_init(139); + RUN_TEST(test_snapent_fixture_seeds_jobs_and_queues); RUN_TEST(test_snapent_checkpoint_job_content_roundtrip); RUN_TEST(test_snapent_checkpoint_concurrent_mutation_not_torn); RUN_TEST(test_snapent_checkpoint_write_failure_leaves_no_final_file); + RUN_TEST(test_snapent_checkpoint_mid_encode_failure_discards_whole_file); } diff --git a/src/include/persist.h b/src/include/persist.h index 6d8a2465..0bb3f962 100644 --- a/src/include/persist.h +++ b/src/include/persist.h @@ -273,6 +273,15 @@ struct persist_s { * production. */ pthread_t test_last_fdatasync_thread; atomic_int test_last_fdatasync_thread_set; + /* Fault-injection seam (ha-entity-snapshots task 2.2): force the Nth + * (0-based, in retained-job walk order) job's checkpoint encode to fail + * inside write_snapshot, proving a genuine mid-loop encode failure + * discards the WHOLE checkpoint rather than skipping just one job. + * Stored as index+1 so the zero-initialized default (0) means + * "disabled" — mirrors test_force_fsync_fail's zero-init-safe + * convention (no explicit persist_init reset needed). Compiled out of + * production. */ + atomic_int test_force_job_encode_fail_at; #endif pthread_t fsync_thread; pthread_mutex_t fsync_mutex; /* guards the fsync timer wait */ @@ -584,6 +593,11 @@ pthread_t persist_test_last_fdatasync_thread(persist_t* p); * compaction hazard where a committed marker's segment is unlinked. Returns 0 * on success, negative errno on I/O failure. Compiled out of production. */ int persist_test_force_new_segment(persist_t* p); + +/* Arm/disarm the job-encode fault-injection seam (see the struct field + * comment above). Pass a negative index to disable. Lock-free atomic + * write; compiled out of production. */ +void persist_test_set_force_job_encode_fail_at(persist_t* p, int index); #endif /* XCE_TESTING */ /* Checkpoint */