From e4bb492ae6a41e74876ed39b89c164dfc4fe65f0 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Sat, 26 Sep 2026 09:55:23 +0000 Subject: [PATCH] fix(controller): checkpoints keep a job's exit code (bug-1867) The capnp Job struct -- the checkpoint/entity-snapshot form of xce_job_t -- had no exit-info field, so capnp_encode_job() never wrote job.exit_info and a job restored from a snapshot (instead of WAL replay) came back with exit_info NULL, for DONE/EXIT and failed jobs alike. bug-1689/bug-1754 made the WAL facts carry the code; anything older than the last checkpoint's WAL trim still lost it. - controller.capnp: struct JobExitInfo $C.mapname("job_exitinfo_t") { exitcode }, and Job.exitInfo @12 $C.mapname("exit_info"). The generator now emits encode/decode/free for it inside encode_Job / decode_Job / free_Job; restore_job()'s create_job() already takes exit_info via MOVE_PTR. Null stays null (no fabricated exit 0). A new ordinal, so older snapshots/recordings decode with exit_info NULL. New test_snapent_checkpoint_exit_info_roundtrip (RED before): exit codes 0 and 137 and a NULL survive both the raw checkpoint frames and a real persist_load_snapshot restore. Full /xce-build all (asan): 111 meson suites, 0 failures; all Go packages pass. Not live-verified: the --force-snapshot HA soak cannot reach its snapshot step because of a pre-existing checkpoint-marker replication stall, reproduced on the baseline without this change (bug-1870). Co-Authored-By: Claude Opus 5.5 (1M context) --- src/controller/persist.c | 9 +-- .../test_persist_snapshot_entities.c | 81 +++++++++++++++++++ src/schema/controller.capnp | 8 ++ 3 files changed, 93 insertions(+), 5 deletions(-) diff --git a/src/controller/persist.c b/src/controller/persist.c index 7421595d..0f36e6aa 100644 --- a/src/controller/persist.c +++ b/src/controller/persist.c @@ -410,11 +410,10 @@ static job_pidinfo_t* decode_pidinfo(const uint8_t* payload, size_t len) } /* Decode a native (non-capnp) WAL_JOB_FINISH exit_code trailer (bug-1689) - * into a freshly heap-allocated job_exitinfo_t*. job_exitinfo_t (xce/types.h) - * is a single int with NO existing Cap'n Proto mapping (confirmed -- no - * JobExitInfo struct in controller.capnp), so this mirrors wal_jobstate_t's - * own native-struct style rather than inventing a capnp wrapper for one int - * -- unlike decode_joballoc()/decode_pidinfo() above, which decode genuine + * into a freshly heap-allocated job_exitinfo_t*. The WAL trailer predates + * the JobExitInfo capnp struct (bug-1867, used for snapshots) and stays a + * native int32_t, mirroring wal_jobstate_t's own native-struct style -- + * unlike decode_joballoc()/decode_pidinfo() above, which decode genuine * capnp payloads. Returns NULL on a short/absent payload; caller owns the * result (release via free()/FREEUP -- no nested pointers). */ static job_exitinfo_t* decode_jobexitinfo(const uint8_t* payload, size_t len) diff --git a/src/controller/testsuite/test_persist_snapshot_entities.c b/src/controller/testsuite/test_persist_snapshot_entities.c index 433bdd7e..e32b35a0 100644 --- a/src/controller/testsuite/test_persist_snapshot_entities.c +++ b/src/controller/testsuite/test_persist_snapshot_entities.c @@ -659,6 +659,86 @@ static void test_snapent_checkpoint_job_content_roundtrip(void) printf("test_snapent_checkpoint_job_content_roundtrip: PASS\n"); } +/* ------------------------------------------------------------------ */ +/* bug-1867: job.exit_info must round-trip through a checkpoint. The */ +/* capnp Job struct had no exit-info field, so capnp_encode_job() never */ +/* wrote it and a job restored from a snapshot (instead of WAL replay) */ +/* lost its exit code, for DONE/EXIT and failed jobs alike. */ +/* ------------------------------------------------------------------ */ + +static void test_snapent_checkpoint_exit_info_roundtrip(void) +{ + const int n_jobs = 3; + + snapent_fixture_t* src = snapent_make(n_jobs, 0); + TEST_ASSERT_NOT_NULL(src); + + /* job 0: exit code 0, job 1: nonzero, job 2: none recorded (NULL) */ + for (int i = 0; i < 2; i++) { + job_t* j = snapent_find_job(src->c, snapent_jobid(i)); + TEST_ASSERT_NOT_NULL(j); + lock(j, RTTI_LOCK_WRITE); + j->job.exit_info = calloc(1, sizeof(job_exitinfo_t)); + TEST_ASSERT_NOT_NULL(j->job.exit_info); + j->job.exit_info->exitcode = (i == 0) ? 0 : 137; + unlock(j); + } + + TEST_ASSERT_EQUAL_INT(0, persist_checkpoint(&src->c->persist)); + + uint64_t lsn = src->c->persist.checkpoint_lsn; + char path[4096]; + snprintf(path, sizeof(path), "%s/snap-%016" PRIx64 ".capnp", + src->c->persist.snap_path, lsn); + + /* 1. the checkpoint's own job frames carry it */ + xce_job_t decoded[8]; + int count = snapent_read_checkpoint_jobs(path, decoded, 8); + TEST_ASSERT_EQUAL_INT(n_jobs, count); + + xce_job_t* d0 = snapent_find_decoded_job(decoded, count, snapent_jobid(0)); + xce_job_t* d1 = snapent_find_decoded_job(decoded, count, snapent_jobid(1)); + xce_job_t* d2 = snapent_find_decoded_job(decoded, count, snapent_jobid(2)); + TEST_ASSERT_NOT_NULL(d0); + TEST_ASSERT_NOT_NULL(d1); + TEST_ASSERT_NOT_NULL(d2); + TEST_ASSERT_NOT_NULL_MESSAGE(d0->exit_info, + "bug-1867: a checkpointed job's exit_info must be written, even " + "for exit code 0"); + TEST_ASSERT_EQUAL_INT(0, d0->exit_info->exitcode); + TEST_ASSERT_NOT_NULL(d1->exit_info); + TEST_ASSERT_EQUAL_INT(137, d1->exit_info->exitcode); + TEST_ASSERT_NULL_MESSAGE(d2->exit_info, + "a job with no exit_info must come back with none, not a " + "fabricated exit code 0"); + snapent_free_decoded_jobs(decoded, count); + + /* 2. a real persist_load_snapshot restore into an empty member */ + snapent_fixture_t* dst = snapent_make(0, 0); + TEST_ASSERT_NOT_NULL(dst); + + bool entities_restored = false; + TEST_ASSERT_EQUAL_INT(0, persist_load_snapshot(&dst->c->persist, path, lsn, + &entities_restored)); + TEST_ASSERT_TRUE(entities_restored); + + job_t* r0 = snapent_find_job(dst->c, snapent_jobid(0)); + job_t* r1 = snapent_find_job(dst->c, snapent_jobid(1)); + job_t* r2 = snapent_find_job(dst->c, snapent_jobid(2)); + TEST_ASSERT_NOT_NULL(r0); + TEST_ASSERT_NOT_NULL(r1); + TEST_ASSERT_NOT_NULL(r2); + TEST_ASSERT_NOT_NULL_MESSAGE(r0->job.exit_info, + "bug-1867: a job restored from a snapshot must keep its exit code"); + TEST_ASSERT_EQUAL_INT(0, r0->job.exit_info->exitcode); + TEST_ASSERT_NOT_NULL(r1->job.exit_info); + TEST_ASSERT_EQUAL_INT(137, r1->job.exit_info->exitcode); + TEST_ASSERT_NULL(r2->job.exit_info); + + snapent_teardown(dst); + snapent_teardown(src); +} + /* ------------------------------------------------------------------ */ /* Test 3 (task 2.1, RED): concurrent per-entity mutation during the */ /* checkpoint walk must never be captured as a torn mix of fields */ @@ -2216,6 +2296,7 @@ void test_persist_snapshot_entities(void) RUN_TEST(test_snapent_fixture_seeds_jobs_and_queues); RUN_TEST(test_snapent_checkpoint_job_content_roundtrip); + RUN_TEST(test_snapent_checkpoint_exit_info_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/schema/controller.capnp b/src/schema/controller.capnp index 97530f52..3f678f14 100644 --- a/src/schema/controller.capnp +++ b/src/schema/controller.capnp @@ -333,6 +333,14 @@ struct Job $C.mapname("xce_job_t"){ pidinfo @9: JobPIDInfo; pendreason @10: JobPendReason $C.mapname("pend_reasons"); transientSince @11: UInt64 = 0 $C.mapname("transient_since"); # epoch seconds; set when job enters JOB_STAT_TRANSIENT + exitInfo @12: JobExitInfo $C.mapname("exit_info"); # bug-1867: null until the job's exit code is known +} + +# bug-1867: job_exitinfo_t, so a checkpointed Job keeps its exit code. The +# WAL facts (WAL_JOB_FINISH / WAL_JOB_FAIL) still carry it as a native +# int32_t trailer; this is the snapshot/entity form. +struct JobExitInfo $C.mapname("job_exitinfo_t") { + exitcode @0: Int32; } struct SchedJob $C.mapname("xce_job_t"){