From f8851353cdc75a1267184f9e49fd59a00f97c4cc Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Tue, 25 Aug 2026 03:12:58 +0000 Subject: [PATCH] test(controller): cover WAL_JOB_DISPATCH allocation persistence (bug-1678) test_dispatch_commit.c's test_dc_success_appends_wal_job_dispatch_durably (already drove the real dispatch_a_job_() production path through a real WAL-write + persist_recover() restart cycle, previously only checking recovered state) now also asserts the recovered job.allocs matches what was dispatched -- host, resource name, value. This is the primary regression test for the user's exact acceptance criterion: "after recover the job should get the same as it's state before shutdown." Independently reproduced RED->GREEN (not just narrated): reverted the production fix with the test extension kept, rebuilt, confirmed FAIL at exactly this assertion; restored, rebuilt, confirmed PASS. test_wal_job_dispatch_fact.c: added 3 new tests exercising the richer payload directly on both apply_committed_entry and apply_wal_entry, plus a full restart-replay variant. All 3 pre-existing tests (bare wal_jobstate_t payloads, no trailing bytes) pass completely unchanged, proving backward compatibility with any WAL_JOB_DISPATCH entry written before this fix. Full controller_unit_test suite independently re-run: 1136/1136 pass, zero regressions. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01AgYRU3zDTqNGV5Jk8aiKLg --- .../testsuite/test_dispatch_commit.c | 25 ++ .../testsuite/test_wal_job_dispatch_fact.c | 225 ++++++++++++++++++ 2 files changed, 250 insertions(+) diff --git a/src/controller/testsuite/test_dispatch_commit.c b/src/controller/testsuite/test_dispatch_commit.c index a96bfb48..7726a640 100644 --- a/src/controller/testsuite/test_dispatch_commit.c +++ b/src/controller/testsuite/test_dispatch_commit.c @@ -619,6 +619,31 @@ static void test_dc_success_appends_wal_job_dispatch_durably(void) "WAL_JOB_DISPATCH fact appended by dispatch_a_job_()'s real " "success path -- no WAL_JOB_START anywhere in this log"); + /* bug-1678: restart replay must recover not just STATE but the + * resource allocation itself, or EXEC_HOST is permanently blank for + * a job that was genuinely RUNNING before shutdown -- the user's + * exact acceptance criterion ("after recover the job should get the + * same as it's state before shutdown"). Mirrors Phase 1's real + * dispatch: host "wkr-replay", one resource "slots" value 1.0. */ + TEST_ASSERT_NOT_NULL_MESSAGE(j2->job.allocs, + "restart replay must recover the dispatch-time resource " + "allocation (bug-1678), not just the RUN state"); + TEST_ASSERT_EQUAL_INT_MESSAGE(1, j2->job.allocs->nhosts, + "recovered allocation must have exactly 1 host, matching what " + "was dispatched in Phase 1"); + TEST_ASSERT_NOT_NULL(j2->job.allocs->allocs); + TEST_ASSERT_NOT_NULL(j2->job.allocs->allocs[0]); + TEST_ASSERT_EQUAL_STRING_MESSAGE("wkr-replay", + j2->job.allocs->allocs[0]->host, + "recovered allocation's host must match the dispatched worker"); + TEST_ASSERT_EQUAL_INT(1, j2->job.allocs->allocs[0]->nallocs); + TEST_ASSERT_NOT_NULL(j2->job.allocs->allocs[0]->allocs); + TEST_ASSERT_NOT_NULL(j2->job.allocs->allocs[0]->allocs[0]); + TEST_ASSERT_EQUAL_STRING("slots", + j2->job.allocs->allocs[0]->allocs[0]->resname); + TEST_ASSERT_EQUAL_FLOAT(1.0f, + j2->job.allocs->allocs[0]->allocs[0]->value); + xce_hook_fini(&c2.hooksys); if (c2.jobmgr) dereference(c2.jobmgr); dc_teardown_persist(p2); diff --git a/src/controller/testsuite/test_wal_job_dispatch_fact.c b/src/controller/testsuite/test_wal_job_dispatch_fact.c index 89794f4f..6c9819db 100644 --- a/src/controller/testsuite/test_wal_job_dispatch_fact.c +++ b/src/controller/testsuite/test_wal_job_dispatch_fact.c @@ -46,6 +46,7 @@ #include "wal_types.h" #include "xce/cluster_def.h" #include "xce/types.h" +#include "xcealloc.h" #include "xstring.h" /* jobj.c's jobsub table and the process-global xstring table are shared @@ -161,6 +162,92 @@ static void test_wjd_apply_committed_entry_transitions_pend_to_run(void) printf("test_wjd_apply_committed_entry_transitions_pend_to_run: PASS\n"); } +/* bug-1678: build a wal_jobstate_t header + trailing Cap'n Proto JobAlloc + * bytes -- persist_submit_job_dispatch_fact()'s payload shape -- mirroring + * capnp_encode_joballoc (persist.c, file-static) since job.c/persist.c's own + * static helpers aren't linkable from a test TU. Caller owns *out. */ +static int wjd_build_dispatch_payload(uint64_t jobid, uint32_t state, + xce_job_alloc_t* alloc, + uint8_t** out, size_t* out_len) +{ + struct capn c; + struct capn_segment* cs; + struct JobAlloc cja; + JobAlloc_ptr jap; + uint8_t* allocbuf; + size_t alloclen; + wal_jobstate_t js = { .jobid = jobid, .state = state, ._pad = 0 }; + + capn_init_malloc(&c); + cs = capn_root(&c).seg; + encode_JobAlloc(cs, &cja, alloc); + jap = new_JobAlloc(cs); + write_JobAlloc(&cja, jap); + capn_setp(capn_root(&c), 0, jap.p); + + alloclen = capn_size(&c); + allocbuf = malloc(alloclen); + if (!allocbuf) { capn_free(&c); return -ENOMEM; } + capn_write_mem(&c, allocbuf, alloclen, 0); + capn_free(&c); + + *out_len = sizeof(js) + alloclen; + *out = malloc(*out_len); + if (!*out) { free(allocbuf); return -ENOMEM; } + memcpy(*out, &js, sizeof(js)); + memcpy(*out + sizeof(js), allocbuf, alloclen); + free(allocbuf); + return 0; +} + +/* One host ("wjd-worker"), one resource ("slots", value 1.0) -- the same + * minimal shape test_dispatch_commit.c's own dispatch tests use. Caller owns + * the result (release via free_JobAlloc_ptr). */ +static xce_job_alloc_t* wjd_make_test_alloc(uint64_t jobid) +{ + return make_job_alloc(jobid, 1, + make_host_allocs(1, + make_host_alloc((char*)"wjd-worker", 1, + make_allocs(1, + make_alloc((char*)"slots", NULL, 0, 1.0f, 0))))); +} + +static void test_wjd_apply_committed_entry_transitions_and_sets_allocs(void) +{ + wjd_setup(); + + job_t* j = wjd_seed_job(71004, JOB_STAT_PEND); + TEST_ASSERT_NOT_NULL(j); + + xce_job_alloc_t* alloc = wjd_make_test_alloc(71004); + TEST_ASSERT_NOT_NULL(alloc); + + uint8_t* payload = NULL; + size_t plen = 0; + TEST_ASSERT_EQUAL_INT(0, + wjd_build_dispatch_payload(71004, JOB_STAT_RUN, alloc, &payload, + &plen)); + free_JobAlloc_ptr(&alloc); + + int rc = apply_committed_entry(&g_wjd_p, WAL_JOB_DISPATCH, 102, payload, + (uint32_t)plen); + free(payload); + + TEST_ASSERT_EQUAL_INT(0, rc); + TEST_ASSERT_EQUAL_UINT32(JOB_STAT_RUN, j->job.state); + TEST_ASSERT_NOT_NULL_MESSAGE(j->job.allocs, + "a WAL_JOB_DISPATCH payload with trailing JobAlloc bytes must set " + "job.allocs (bug-1678), not just state"); + TEST_ASSERT_EQUAL_INT(1, j->job.allocs->nhosts); + TEST_ASSERT_EQUAL_STRING("wjd-worker", j->job.allocs->allocs[0]->host); + TEST_ASSERT_EQUAL_STRING("slots", + j->job.allocs->allocs[0]->allocs[0]->resname); + + wjd_teardown(); + printf("test_wjd_apply_committed_entry_transitions_and_sets_allocs: " + "PASS\n"); +} + /* ── apply_wal_entry(): standby catch-up / replicated-apply dispatch ────── */ static void @@ -192,6 +279,41 @@ test_wjd_apply_wal_entry_transitions_pend_to_run_no_hook(void) "PASS\n"); } +static void test_wjd_apply_wal_entry_transitions_and_sets_allocs(void) +{ + wjd_setup(); + + job_t* j = wjd_seed_job(71005, JOB_STAT_PEND); + TEST_ASSERT_NOT_NULL(j); + + xce_job_alloc_t* alloc = wjd_make_test_alloc(71005); + TEST_ASSERT_NOT_NULL(alloc); + + uint8_t* payload = NULL; + size_t plen = 0; + TEST_ASSERT_EQUAL_INT(0, + wjd_build_dispatch_payload(71005, JOB_STAT_RUN, alloc, &payload, + &plen)); + free_JobAlloc_ptr(&alloc); + + int rc = apply_wal_entry(&g_wjd_p, WAL_JOB_DISPATCH, 103, payload, + (uint32_t)plen); + free(payload); + + TEST_ASSERT_EQUAL_INT(0, rc); + TEST_ASSERT_EQUAL_UINT32(JOB_STAT_RUN, j->job.state); + TEST_ASSERT_NOT_NULL_MESSAGE(j->job.allocs, + "apply_wal_entry's catch-up dispatch must also recover job.allocs " + "from a WAL_JOB_DISPATCH payload's trailing JobAlloc bytes " + "(bug-1678) -- required for a promoted standby, not just the " + "primary's own boot replay"); + TEST_ASSERT_EQUAL_INT(1, j->job.allocs->nhosts); + TEST_ASSERT_EQUAL_STRING("wjd-worker", j->job.allocs->allocs[0]->host); + + wjd_teardown(); + printf("test_wjd_apply_wal_entry_transitions_and_sets_allocs: PASS\n"); +} + /* ── Restart-replay Observable (this task's completion criterion) ───────── */ static persist_t* wjd_make_fresh_persist(const char* tmpdir) @@ -330,12 +452,115 @@ test_wjd_restart_replay_recovers_running_without_start_fact(void) "PASS\n"); } +/* bug-1678: sibling of the test above, proving the user's exact acceptance + * criterion ("after recover the job should get the same as it's state + * before shutdown") at this file's own direct-payload granularity: a + * WAL_JOB_DISPATCH fact carrying trailing JobAlloc bytes survives a real + * restart (persist_recover over real on-disk WAL) with job.allocs intact, + * not just job.state. test_dispatch_commit.c's + * test_dc_success_appends_wal_job_dispatch_durably covers the same + * observable through the REAL dispatch_a_job_() production call site; this + * test isolates it at the WAL fact level, mirroring how this file's other + * tests already isolate apply_committed_entry/apply_wal_entry from the live + * caller. */ +static void +test_wjd_restart_replay_recovers_running_and_allocs_with_dispatch_fact(void) +{ + char tmp[256]; + strcpy(tmp, "/tmp/xce_wjd_replay2_XXXXXX"); + TEST_ASSERT_NOT_NULL(mkdtemp(tmp)); + + /* Phase 1: write the log to real disk. */ + { + persist_t* p = wjd_make_fresh_persist(tmp); + TEST_ASSERT_NOT_NULL(p); + + job_submit_data_t submit; + memset(&submit, 0, sizeof(submit)); + submit.queue = (char*)"q"; + submit.user = (char*)"tester"; + + xce_job_t jdef; + memset(&jdef, 0, sizeof(jdef)); + jdef.jobid = 71006; + jdef.state = JOB_STAT_PEND; + jdef.submit = &submit; + + uint8_t* job_payload = NULL; + size_t job_plen = 0; + TEST_ASSERT_EQUAL_INT(0, + wjd_encode_job(&jdef, &job_payload, &job_plen)); + TEST_ASSERT_EQUAL_INT(0, + persist_wal_async(p, WAL_JOB_SUBMIT, job_payload, + (uint32_t)job_plen)); + free(job_payload); + + xce_job_alloc_t* alloc = wjd_make_test_alloc(71006); + TEST_ASSERT_NOT_NULL(alloc); + + uint8_t* dispatch_payload = NULL; + size_t dispatch_plen = 0; + TEST_ASSERT_EQUAL_INT(0, + wjd_build_dispatch_payload(71006, JOB_STAT_RUN, alloc, + &dispatch_payload, &dispatch_plen)); + free_JobAlloc_ptr(&alloc); + + TEST_ASSERT_EQUAL_INT(0, + persist_wal_async(p, WAL_JOB_DISPATCH, dispatch_payload, + (uint32_t)dispatch_plen)); + free(dispatch_payload); + + TEST_ASSERT_EQUAL_INT(0, persist_flush(p)); + wjd_teardown_persist(p); + } + + /* Phase 2: fresh persist_t + fresh cluster_t, recover from disk — + * simulates a controller restart. */ + { + persist_t* p2 = wjd_make_fresh_persist(tmp); + TEST_ASSERT_NOT_NULL(p2); + + wjd_setup(); + p2->cluster = &g_wjd_c; + + int rc = persist_recover(p2); + TEST_ASSERT_EQUAL_INT(0, rc); + + job_t* j = wjd_find_job(71006); + TEST_ASSERT_NOT_NULL_MESSAGE(j, + "restart replay must construct the job via WAL_JOB_SUBMIT"); + TEST_ASSERT_EQUAL_UINT32(JOB_STAT_RUN, j->job.state); + TEST_ASSERT_NOT_NULL_MESSAGE(j->job.allocs, + "restart replay must recover job.allocs from the " + "WAL_JOB_DISPATCH fact's trailing JobAlloc bytes (bug-1678), " + "not just job.state"); + TEST_ASSERT_EQUAL_INT(1, j->job.allocs->nhosts); + TEST_ASSERT_EQUAL_STRING("wjd-worker", + j->job.allocs->allocs[0]->host); + + wjd_teardown(); + wjd_teardown_persist(p2); + } + + char cmd[512]; + snprintf(cmd, sizeof(cmd), "rm -rf %s", tmp); + if (system(cmd) != 0) + fprintf(stderr, "cleanup warning: rm -rf %s failed\n", tmp); + + printf("test_wjd_restart_replay_recovers_running_and_allocs_with_" + "dispatch_fact: PASS\n"); +} + /* ── Test group entry point ───────────────────────────────────────────── */ void test_wal_job_dispatch_fact(void) { UnitySetTestFile(__FILE__); RUN_TEST(test_wjd_apply_committed_entry_transitions_pend_to_run); + RUN_TEST(test_wjd_apply_committed_entry_transitions_and_sets_allocs); RUN_TEST(test_wjd_apply_wal_entry_transitions_pend_to_run_no_hook); + RUN_TEST(test_wjd_apply_wal_entry_transitions_and_sets_allocs); RUN_TEST(test_wjd_restart_replay_recovers_running_without_start_fact); + RUN_TEST( + test_wjd_restart_replay_recovers_running_and_allocs_with_dispatch_fact); }