From 22353849f6432de89099679271baac3c35b0aa2c Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Thu, 27 Aug 2026 13:15:54 +0000 Subject: [PATCH] feat(scheduler-snapshot-job-allocation): construct schedobj for restored jobs Task 1.2: snapshot_load.c gains a local, static field-for-field port of core_events.c's core_build_schedobj()/core_build_primary_schedobj()/ core_build_compound_schedobj()/core_build_alter_schedobj() (the bug-1569 fix for the JOBS_PUSH path). Every job restored from a snapshot whose resreq parsed successfully now gets a valid, non-NULL job->schedobj, closing the same NULL-deref crash class for the snapshot-load path. Guarded on job->resreq != NULL so the pre-existing, unrelated resreq-parse-failure path is untouched (still leaves schedobj NULL, same as today -- not fixed here, per design.md's documented boundary). New tests prove non-NULL schedobj with correct tag/union member for primary, compound, and alter resource requests, plus a regression test confirming the parse-failure path's behavior is unchanged. Requirements: 2.1, 2.2, 2.3, 2.4 --- .../tasks.md | 2 +- src/scheduler/src/snapshot_load.c | 130 +++++++++++ .../testsuite/test_snapshot_save_load.c | 204 ++++++++++++++++++ 3 files changed, 335 insertions(+), 1 deletion(-) diff --git a/.kiro/specs/scheduler-snapshot-job-allocation/tasks.md b/.kiro/specs/scheduler-snapshot-job-allocation/tasks.md index f0874c43..50cf3486 100644 --- a/.kiro/specs/scheduler-snapshot-job-allocation/tasks.md +++ b/.kiro/specs/scheduler-snapshot-job-allocation/tasks.md @@ -9,7 +9,7 @@ - _Requirements: 1.1, 1.4_ - _Boundary: xcealloc (job allocation deep-copy)_ -- [ ] 1.2 (P) Construct a valid scheduling representation for every job restored from a snapshot +- [x] 1.2 (P) Construct a valid scheduling representation for every job restored from a snapshot - Write a failing test first (TDD red) asserting that a job reconstructed from a snapshot has a non-NULL, correctly-shaped internal scheduling representation for a primary, a compound, and an alternative resource request. - Implement construction using only information already available by the time a restored job's resource request has been parsed (its queue, user, project, and resource request) — no new snapshot data is captured or required. - Skip construction (leaving today's behavior unchanged) when a restored job's resource request itself failed to parse — do not attempt to fix that separate, pre-existing failure path as part of this task. diff --git a/src/scheduler/src/snapshot_load.c b/src/scheduler/src/snapshot_load.c index c6208c62..0b3efd2b 100644 --- a/src/scheduler/src/snapshot_load.c +++ b/src/scheduler/src/snapshot_load.c @@ -51,6 +51,121 @@ * core_session.c's own `extern void sched_worker_release_host_ctx(...)`. */ extern xce_val_t* dup_val(const xce_val_t*); +/* + * snapshot_build_primary_schedobj/snapshot_build_compound_schedobj/ + * snapshot_build_alter_schedobj/snapshot_build_schedobj - + * scheduler-snapshot-job-allocation spec, task 1.2 (Req 2 AC 1-4): a LOCAL, + * STATIC, field-for-field port of core_build_primary_schedobj()/ + * core_build_compound_schedobj()/core_build_alter_schedobj()/ + * core_build_schedobj() (core_events.c) -- NOT a call into those functions, + * which are `static` to core_events.c and, per this project's existing, + * deliberate convention (see this file's own header comment and + * core_events.c's), snapshot_load.c never shares helpers with + * core_events.c's builders. This is the exact same bug-1569 failure shape + * core_events.c's own header comment documents (the allocator -- + * sched_make_allocation() and plugins like limit.c -- dereferences + * job->schedobj unconditionally, no NULL check) applied to the one + * remaining code path that still leaves job->schedobj NULL for every + * restored job: this feature closes it here. + * + * Needs no new snapshot data: user/queue/project come from the wire job's + * own submit fields (wj->submit->user/queue/project_name, the same fields + * core_build_schedobj() reads from its own wire job), and resreq is the + * job's own already-parsed xce_resreq_t. glimits/hlimits start empty and + * configver starts at 0, matching core_build_schedobj() exactly; alloc/ + * reason are left NULL (calloc'd obj is already zeroed) -- both are + * per-scheduling-round scratch state, never persisted job state (see + * research.md's own finding for this task). + */ +static void snapshot_build_primary_schedobj(xce_sched_obj_t* obj, + xce_primary_resreq_t* resreq) +{ + xce_primary_schedobj_t* primary + = (xce_primary_schedobj_t*)calloc(1, sizeof(*primary)); + + obj->tag = XCE_RESREQ_PRIMARY; + if (primary == NULL) { + return; + } + + primary->resreq = resreq; + primary->reason = NULL; + primary->owner = obj; + + obj->u.primary = primary; +} + +static void snapshot_build_compound_schedobj(xce_sched_obj_t* obj, + xce_compound_resreq_t* resreq) +{ + xce_compound_schedobj_t* compound + = (xce_compound_schedobj_t*)calloc(1, sizeof(*compound)); + + obj->tag = XCE_RESREQ_COMPOUND; + if (compound == NULL) { + return; + } + + compound->resreq = resreq; + compound->owner = obj; + + obj->u.compound = compound; +} + +static void snapshot_build_alter_schedobj(xce_sched_obj_t* obj, + xce_alter_resreq_t* resreq) +{ + int i; + + obj->tag = XCE_RESREQ_ALTER; + xlist_init(&(obj->u.alters), 4); + + for (i = 0; i < resreq->compounds.count; i++) { + xce_compound_schedobj_t* cobj = NULL; + xce_compound_resreq_t* cresreq + = (xce_compound_resreq_t*)xlist_get(&(resreq->compounds), i); + + if (cresreq == NULL) { + continue; + } + + cobj = (xce_compound_schedobj_t*)calloc(1, sizeof(*cobj)); + if (cobj == NULL) { + continue; + } + + cobj->resreq = cresreq; + cobj->owner = obj; + + xlist_store(&(obj->u.alters), cobj); + } +} + +static void snapshot_build_schedobj(xce_sched_obj_t* obj, xce_job_t* wj, + xce_resreq_t* resreq) +{ + obj->user = makeString(wj->submit->user); + obj->queue = makeString(wj->submit->queue); + obj->project = makeString(wj->submit->project_name); + + xlist_init(&(obj->glimits), 8); + zarray_init(&(obj->hlimits), 32, NULL); + + switch (resreq->tag) { + case XCE_RESREQ_PRIMARY: + snapshot_build_primary_schedobj(obj, resreq->u.primary); + break; + case XCE_RESREQ_COMPOUND: + snapshot_build_compound_schedobj(obj, resreq->u.compound); + break; + case XCE_RESREQ_ALTER: + snapshot_build_alter_schedobj(obj, resreq->u.alter); + break; + } + + obj->configver = 0; +} + /* ------------------------------------------------------------------------ * xce_sched_core_load_snapshot * ---------------------------------------------------------------------- */ @@ -331,6 +446,21 @@ int xce_sched_core_load_snapshot(xce_sched_core_t* core, } } + /* scheduler-snapshot-job-allocation task 1.2 (Req 2 AC 1-4): + * construct job->schedobj for every restored job whose resreq + * parsed successfully -- guarded on job->resreq != NULL so the + * pre-existing, unrelated resreq-parse-failure path above + * (which already leaves job->resreq NULL) is tolerated, not + * fixed, here (design.md "Load-Side Job Reconstruction", Known + * limitation). */ + if (job->resreq != NULL) { + job->schedobj + = (xce_sched_obj_t*)calloc(1, sizeof(*(job->schedobj))); + if (job->schedobj != NULL) { + snapshot_build_schedobj(job->schedobj, wj, job->resreq); + } + } + sched_add_job(core, job); if (picker != NULL) { diff --git a/src/scheduler/testsuite/test_snapshot_save_load.c b/src/scheduler/testsuite/test_snapshot_save_load.c index aae1cf11..a7a746cd 100644 --- a/src/scheduler/testsuite/test_snapshot_save_load.c +++ b/src/scheduler/testsuite/test_snapshot_save_load.c @@ -357,6 +357,21 @@ static xce_job_t* make_job(uint64_t jobid, uint32_t state, char* queue, return j; } +/* make_job_with_resreq - same fixture as make_job() above, but with a + * caller-supplied resreq string, so task 1.2's schedobj tests can build + * PRIMARY/COMPOUND/ALTER-tagged jobs (and a deliberately-unparseable one) + * without duplicating make_job()'s own field-population logic. */ +static xce_job_t* make_job_with_resreq(uint64_t jobid, uint32_t state, + char* queue, char* user, char* project, + char* resreq) +{ + xce_job_t* j = make_job(jobid, state, queue, user, project); + + j->submit->resreq = resreq; + + return j; +} + #define MAX_RECORDED_DECISIONS 8 typedef struct { @@ -975,6 +990,191 @@ test_snapshot_save_then_load_produces_behaviorally_equivalent_core(void) TEST_ASSERT_EQUAL_UINT64(203, driver_ctx_a.seen[2]->u.reason->jobid); } +/* ---------------------------------------------------------------------- */ +/* scheduler-snapshot-job-allocation task 1.2: every job restored from a */ +/* snapshot must get a non-NULL, correctly-shaped job->schedobj -- the */ +/* same bug-1569 failure shape (core_events.c's own header comment), */ +/* still open on THIS load path. Covers a PRIMARY, a COMPOUND, and an */ +/* ALTER resource request (design.md "Load-Side Job Reconstruction"). */ +/* ---------------------------------------------------------------------- */ + +static void +test_snapshot_load_builds_schedobj_for_primary_compound_and_alter_resreq(void) +{ + xce_sched_core_t core; + sched_snapshot_data_t in; + xce_jobs_data_t jobs_data; + xce_queue_t wire_queue_normal; + xce_queue_t wire_queue_batch; + xce_queue_t* queues[2]; + xce_job_t* job_primary; + xce_job_t* job_compound; + xce_job_t* job_alter; + xce_job_t* jobs[3]; + xce_sched_job_t* restored_primary; + xce_sched_job_t* restored_compound; + xce_sched_job_t* restored_alter; + int rc; + + memset(&g_stub_state_a, 0, sizeof(g_stub_state_a)); + fixture_init(&core, &g_stub_plugin_a); + + /* sched_get_queue() is NOT get-or-create (unlike sched_get_user()/ + * sched_get_project()) -- both queues this test's jobs reference must + * be present in the snapshot input, same as the cross-core test above. */ + memset(&wire_queue_normal, 0, sizeof(wire_queue_normal)); + wire_queue_normal.queue = "normal"; + wire_queue_normal.priority = 1; + wire_queue_normal.status = QUEUE_STAT_OPEN; + + memset(&wire_queue_batch, 0, sizeof(wire_queue_batch)); + wire_queue_batch.queue = "batch"; + wire_queue_batch.priority = 1; + wire_queue_batch.status = QUEUE_STAT_OPEN; + + queues[0] = &wire_queue_normal; + queues[1] = &wire_queue_batch; + + job_primary = make_job_with_resreq(401, JOB_STAT_PEND, "normal", "alice", + "proj1", "type == any"); + /* COMPOUND: '+' between two '{...}' terms (test_resreq.c's own + * test_resreq2() fixture -- proven, real COMPOUND-tagged input). */ + job_compound + = make_job_with_resreq(402, JOB_STAT_PEND, "normal", "bob", "proj1", + "2*{rusage[mem=100:duration=20:decay=1]} + " + "4*{rusage[mem=80:duration=30:decay=1]}"); + /* ALTER: '||' between two '{compound}' terms (test_resreq.c's own + * test_resreq3() fixture -- proven, real ALTER-tagged input). */ + job_alter + = make_job_with_resreq(403, JOB_STAT_PEND, "batch", "carol", "proj2", + "{2*{rusage[mem=100:duration=20:decay=1]} + " + "4*{rusage[mem=80:duration=30:decay=1]}} || " + "{3*{rusage[mem=150:duration=15:decay=1]} + " + "6*{rusage[mem=120:duration=20:decay=1]}}"); + + jobs[0] = job_primary; + jobs[1] = job_compound; + jobs[2] = job_alter; + + memset(&jobs_data, 0, sizeof(jobs_data)); + jobs_data.count = 3; + jobs_data.jobs = jobs; + + memset(&in, 0, sizeof(in)); + in.jobs = &jobs_data; + in.queues = queues; + in.n_queues = 2; + + rc = xce_sched_core_load_snapshot(&core, &in); + TEST_ASSERT_EQUAL_INT(0, rc); + + restored_primary = sched_get_job(&core, 401); + restored_compound = sched_get_job(&core, 402); + restored_alter = sched_get_job(&core, 403); + TEST_ASSERT_NOT_NULL(restored_primary); + TEST_ASSERT_NOT_NULL(restored_compound); + TEST_ASSERT_NOT_NULL(restored_alter); + + /* (a) PRIMARY */ + TEST_ASSERT_NOT_NULL(restored_primary->resreq); + TEST_ASSERT_NOT_NULL(restored_primary->schedobj); + TEST_ASSERT_EQUAL_INT(XCE_RESREQ_PRIMARY, restored_primary->schedobj->tag); + TEST_ASSERT_NOT_NULL(restored_primary->schedobj->u.primary); + + /* (b) COMPOUND */ + TEST_ASSERT_NOT_NULL(restored_compound->resreq); + TEST_ASSERT_NOT_NULL(restored_compound->schedobj); + TEST_ASSERT_EQUAL_INT(XCE_RESREQ_COMPOUND, + restored_compound->schedobj->tag); + TEST_ASSERT_NOT_NULL(restored_compound->schedobj->u.compound); + + /* (c) ALTER */ + TEST_ASSERT_NOT_NULL(restored_alter->resreq); + TEST_ASSERT_NOT_NULL(restored_alter->schedobj); + TEST_ASSERT_EQUAL_INT(XCE_RESREQ_ALTER, restored_alter->schedobj->tag); + TEST_ASSERT_TRUE(restored_alter->schedobj->u.alters.count > 0); + + /* (d) the restored schedobj's user/queue/project string values match + * the job's own (restored via sched_get_user/queue/project), for all + * three -- proving snapshot_build_schedobj() reads the same wire + * fields core_build_schedobj() would have. */ + TEST_ASSERT_EQUAL_STRING(restored_primary->user->name, + restored_primary->schedobj->user); + TEST_ASSERT_EQUAL_STRING(restored_primary->queue->name, + restored_primary->schedobj->queue); + TEST_ASSERT_EQUAL_STRING(restored_primary->project->name, + restored_primary->schedobj->project); + + TEST_ASSERT_EQUAL_STRING(restored_compound->user->name, + restored_compound->schedobj->user); + TEST_ASSERT_EQUAL_STRING(restored_compound->queue->name, + restored_compound->schedobj->queue); + TEST_ASSERT_EQUAL_STRING(restored_compound->project->name, + restored_compound->schedobj->project); + + TEST_ASSERT_EQUAL_STRING(restored_alter->user->name, + restored_alter->schedobj->user); + TEST_ASSERT_EQUAL_STRING(restored_alter->queue->name, + restored_alter->schedobj->queue); + TEST_ASSERT_EQUAL_STRING(restored_alter->project->name, + restored_alter->schedobj->project); +} + +/* Regression-safety test (NOT a new feature): a job whose resreq fails to + * parse on load is a pre-existing, out-of-boundary gap (design.md "Known + * limitation") that task 1.2 must NOT try to fix -- it must only make sure + * schedobj construction is skipped rather than crashing on a NULL resreq. + * Proves job->schedobj stays NULL, identical to today's behavior. */ +static void +test_snapshot_load_leaves_schedobj_null_when_resreq_fails_to_parse(void) +{ + xce_sched_core_t core; + sched_snapshot_data_t in; + xce_jobs_data_t jobs_data; + xce_job_t* bad_job; + xce_job_t* jobs[1]; + xce_sched_job_t* restored; + int rc; + + memset(&g_stub_state_a, 0, sizeof(g_stub_state_a)); + fixture_init(&core, &g_stub_plugin_a); + + /* An unbalanced/unterminated compound-resreq brace: the top-level PEG + * grammar (xce_resreq.peg) requires a full input match through EOF; a + * lone unterminated '{' cannot complete as alter/compound/primary and + * leaves trailing unconsumed input, so parse_resreq() genuinely fails + * (unlike plain non-brace garbage, which the grammar's permissive + * primary_resreq rule accepts as a literal select expression and only + * fails later, silently, inside parse_resreq_primary() without + * propagating an error -- confirmed empirically while writing this + * test). */ + bad_job = make_job_with_resreq(404, JOB_STAT_PEND, "normal", "alice", + "proj1", "{"); + + jobs[0] = bad_job; + + memset(&jobs_data, 0, sizeof(jobs_data)); + jobs_data.count = 1; + jobs_data.jobs = jobs; + + memset(&in, 0, sizeof(in)); + in.jobs = &jobs_data; + + rc = xce_sched_core_load_snapshot(&core, &in); + TEST_ASSERT_EQUAL_INT(0, rc); + + restored = sched_get_job(&core, 404); + TEST_ASSERT_NOT_NULL(restored); + + /* pre-existing, unrelated gap: resreq parse failure leaves job->resreq + * NULL (FREEUP'd in xce_sched_core_load_snapshot's own job loop) -- + * unchanged by this feature. */ + TEST_ASSERT_NULL(restored->resreq); + /* THIS feature's guard: schedobj construction is skipped, not crashed + * into, when resreq is NULL. */ + TEST_ASSERT_NULL(restored->schedobj); +} + int main(void) { UNITY_BEGIN(); @@ -984,5 +1184,9 @@ int main(void) test_snapshot_save_and_load_reject_during_genuinely_active_session); RUN_TEST( test_snapshot_save_then_load_produces_behaviorally_equivalent_core); + RUN_TEST( + test_snapshot_load_builds_schedobj_for_primary_compound_and_alter_resreq); + RUN_TEST( + test_snapshot_load_leaves_schedobj_null_when_resreq_fails_to_parse); return UNITY_END(); }