diff --git a/.kiro/specs/scheduler-dispatch-commit/tasks.md b/.kiro/specs/scheduler-dispatch-commit/tasks.md index 85607452..74fa9c7a 100644 --- a/.kiro/specs/scheduler-dispatch-commit/tasks.md +++ b/.kiro/specs/scheduler-dispatch-commit/tasks.md @@ -49,7 +49,7 @@ - _Requirements: 2.1, 2.2, 2.3, 2.4, 5.1_ - _Depends: 1.2, 1.3_ -- [ ] 3.2 Controller resolves a terminal (locally-invalid) dispatch failure +- [x] 3.2 Controller resolves a terminal (locally-invalid) dispatch failure - When a dispatch attempt fails for a reason the classification (Task 1.1) marks terminal, the controller resolves the job to a permanently failed state immediately, protected against concurrent access to the job record, and releases any resource-allocation data tentatively associated with the attempt - This covers the two failure sites that already release the job reference they hold today — confirm that behavior is preserved, not duplicated, by this new resolution step - Observable completion: a terminally-classified dispatch failure leaves the job permanently failed, with no partial allocation data left behind and the job's reference count unchanged from before the attempt @@ -105,3 +105,5 @@ - Task 3.1: reconfirmed **bug-1429** (`controller_unit_test` TIMEOUT/hang, pre-existing, unrooted) a 3rd time, now hanging inside `test_commit_barrier_reply.c`'s `test_jobsubmit_repeated_truncation_forced_failures_no_phantom_jobs` — a 3rd distinct hang location across 3 sessions, confirmed unrelated to `dispatch_a_job_()` (that test never calls it) and independently reproduced twice more (standalone binary run past the meson harness's 300s cap, and a fully separate rebuild). Because this hang sits earlier in `controller_test.c`'s registration order than most of this feature's own new test files, a full un-filtered `meson test` run will NOT reach them — **task 5.4's full-suite validation must run with the hanging group filtered out (`controller_unit_test -t `) or budget for the 300s timeout**, not treat a bare `meson test` TIMEOUT as this feature's own failure. - Task 3.1: confirmed pre-existing (not introduced by this task, not in this task's boundary to fix) — `dispatch_a_job_()`'s success path never calls `dereference(j)` on the reference taken at lookup; identical in the diff's before/after, and the sole caller `handle_jobdispatch()` never dereferences either. A candidate future bug-log entry, out of scope for this spec. - Task 3.1: when validating a build against this suite's ASAN leak output, always replicate the project's own `ASAN_OPTIONS=detect_leaks=0` test env (`meson.build:229`) — a standalone binary run without it produces spurious leak reports (a red herring, not a regression) that can waste review time. +- Task 3.2: `job_t`'s real reference count lives in `jobj.c`'s file-local `job_private_t`, with no accessor exposed anywhere in the codebase — a test proving no reference-count regression can't do a direct numeric comparison. The working pattern (used by `test_dispatch_fail_terminal.c`) is a "canary reference": take one extra manual `reference()` on the job **immediately before** the call under test (not after — arming it after only relies incidentally on other assertions crashing first, which doesn't independently prove anything), then `dereference()` it after the call and confirm the job is still safely readable — an over-release during the call frees the job while the canary is still held, so the post-call touch is a real ASAN-caught heap-use-after-free. +- Task 3.2: reconfirmed bug-1429's hang again (still `test_commit_barrier_reply.c`, still unrelated to any change in this feature) — third+ reproduction, still not root-caused. Continues to require `controller_unit_test -t ` filtering for a clean isolated regression signal. diff --git a/src/controller/job.c b/src/controller/job.c index d1bfd056..195c2731 100644 --- a/src/controller/job.c +++ b/src/controller/job.c @@ -958,6 +958,54 @@ static int dispatch_a_job_(cluster_t* c, endpoint_t* ep, xce_job_alloc_t* alloc) nc = alloc->nhosts; if (nc <= 0) { + /* scheduler-dispatch-commit task 3.2 (Req 4.2, 4.5): resolves this + * terminal (locally-invalid) dispatch failure -- j's own reference + * held since the lookup above is released via the dereference(j) + * call below exactly as it already was before this task (not + * duplicated). Nothing was ever committed to j->job.allocs on this + * path (task 3.1 gates that commit on a successful + * xce_startjob_req(), never reached here), so Req 4.5's "release + * any tentative allocation data" is satisfied vacuously by + * construction -- there is nothing to release. */ + xce_dispatch_fail_class_t dfclass; + job_failinfo_t failinfo = { .jobid = alloc->jobid }; + + lock(j, RTTI_LOCK_WRITE); + + dfclass + = xce_classify_dispatch_fail(XCE_DISPATCH_FAIL_SITE_INVALID_ALLOC); + switch (dfclass) { + case XCE_DISPATCH_FAIL_TERMINAL: + break; + case XCE_DISPATCH_FAIL_RETRYABLE: + default: + /* Impossible per today's classification table (confirmed: + * xce_classify_dispatch_fail() returns TERMINAL for this site + * unconditionally) -- task 3.3 is what wires real retryable + * handling at the other four sites, not this one. Fail loud + * and fall through to the same terminal resolution as a safe + * fallback, mirroring xce_classify_dispatch_fail()'s own + * default: convention. */ + ERROR("dispatch_a_job_: XCE_DISPATCH_FAIL_SITE_INVALID_ALLOC " + "classified non-terminal (%d) -- impossible per the " + "current classification table; resolving terminal as a " + "safe fallback", (int)dfclass); +#ifdef XCE_TESTING + assert(0 + && "XCE_DISPATCH_FAIL_SITE_INVALID_ALLOC classified " + "non-terminal -- update the classification table or " + "this call site"); +#endif + break; + } + + failinfo.code = -EINVAL; + failinfo.detail = "dispatch failed: invalid allocation"; + + mark_job_fail(c, j, &failinfo); + + unlock(j); + dereference(j); return -EINVAL; @@ -1007,8 +1055,62 @@ static int dispatch_a_job_(cluster_t* c, endpoint_t* ep, xce_job_alloc_t* alloc) rc = get_queue_exec(c, j->job.submit->queue, &q_pre_exec, &q_post_exec); if (rc < 0) { + /* scheduler-dispatch-commit task 3.2 (Req 4.2, 4.5): resolves + * this terminal (locally-invalid) dispatch failure -- j's own + * reference held since the lookup above is released via the + * dereference(j) call below exactly as it already was before + * this task (not duplicated). Nothing was ever committed to + * j->job.allocs on this path (task 3.1 gates that commit on a + * successful xce_startjob_req(), never reached here), so Req + * 4.5's "release any tentative allocation data" is satisfied + * vacuously by construction. unlock(w)/dereference(w)/ + * free_launchinfo(&launchinfo) stay in their existing + * positions -- only this new lock/classify/mark_job_fail/ + * unlock sequence is inserted around the existing + * dereference(j) call. */ + xce_dispatch_fail_class_t dfclass; + job_failinfo_t failinfo = { .jobid = alloc->jobid }; + unlock(w); dereference(w); + + lock(j, RTTI_LOCK_WRITE); + + dfclass = xce_classify_dispatch_fail( + XCE_DISPATCH_FAIL_SITE_QUEUE_EXEC); + switch (dfclass) { + case XCE_DISPATCH_FAIL_TERMINAL: + break; + case XCE_DISPATCH_FAIL_RETRYABLE: + default: + /* Impossible per today's classification table (confirmed: + * xce_classify_dispatch_fail() returns TERMINAL for this + * site unconditionally) -- task 3.3 is what wires real + * retryable handling at the other four sites, not this + * one. Fail loud and fall through to the same terminal + * resolution as a safe fallback, mirroring + * xce_classify_dispatch_fail()'s own default: convention. */ + ERROR("dispatch_a_job_: XCE_DISPATCH_FAIL_SITE_QUEUE_EXEC " + "classified non-terminal (%d) -- impossible per the " + "current classification table; resolving terminal " + "as a safe fallback", (int)dfclass); +#ifdef XCE_TESTING + assert(0 + && "XCE_DISPATCH_FAIL_SITE_QUEUE_EXEC classified " + "non-terminal -- update the classification table " + "or this call site"); +#endif + break; + } + + failinfo.code = rc; + failinfo.detail = "dispatch failed: queue exec info lookup " + "failed"; + + mark_job_fail(c, j, &failinfo); + + unlock(j); + dereference(j); free_launchinfo(&launchinfo); diff --git a/src/controller/meson.build b/src/controller/meson.build index 2efb11e3..1c48d534 100644 --- a/src/controller/meson.build +++ b/src/controller/meson.build @@ -180,6 +180,7 @@ controller_unit_test_src = [ 'testsuite/test_job_array_replay_fence.c', 'testsuite/test_job_dispatch.c', 'testsuite/test_dispatch_commit.c', + 'testsuite/test_dispatch_fail_terminal.c', 'testsuite/test_upgrade_snapshot_restore.c', 'testsuite/test_upgrade_snapshot_apply.c', 'testsuite/test_bug857_regression.c', diff --git a/src/controller/testsuite/controller_test.c b/src/controller/testsuite/controller_test.c index ebf796b1..d34d818a 100644 --- a/src/controller/testsuite/controller_test.c +++ b/src/controller/testsuite/controller_test.c @@ -98,6 +98,7 @@ extern void test_bug857_regression(void); extern void test_sched_admin_dispatch(void); extern void test_job_dispatch(void); extern void test_dispatch_commit(void); +extern void test_dispatch_fail_terminal(void); void setUp(void) { return; } @@ -265,6 +266,7 @@ int main(int argc, char* argv[]) TEST(test_sched_admin_dispatch); TEST(test_job_dispatch); TEST(test_dispatch_commit); + TEST(test_dispatch_fail_terminal); return UNITY_END(); } diff --git a/src/controller/testsuite/test_dispatch_fail_terminal.c b/src/controller/testsuite/test_dispatch_fail_terminal.c new file mode 100644 index 00000000..eb5d3a08 --- /dev/null +++ b/src/controller/testsuite/test_dispatch_fail_terminal.c @@ -0,0 +1,328 @@ +/* test_dispatch_fail_terminal.c -- scheduler-dispatch-commit task 3.2 + * + * Proves dispatch_a_job_() (job.c) now RESOLVES a terminal (locally-invalid) + * dispatch failure instead of merely returning an error code with the job + * left stuck at PEND. Covers the two failure sites design.md's "Controller + * Dispatch-Failure Classification" component always classifies TERMINAL + * today (confirmed by task 1.1's own xce_classify_dispatch_fail() switch, + * unmodified/reused here, not re-tested): + * + * - XCE_DISPATCH_FAIL_SITE_INVALID_ALLOC (alloc->nhosts <= 0) + * - XCE_DISPATCH_FAIL_SITE_QUEUE_EXEC (get_queue_exec() failure) + * + * Req 4.2: a terminally-classified failure resolves the job to + * JOB_STAT_EXIT immediately, under lock(j, RTTI_LOCK_WRITE), via + * mark_job_fail() -- not left at PEND. + * Req 4.5: no partial allocation data is left behind. Satisfied vacuously + * by construction (task 3.1 already gates job.allocs's only commit on a + * successful xce_startjob_req(), never reached on either of these two + * failure paths) -- asserted here explicitly for this specific path anyway, + * per this task's own Observable-completion clause. + * Also (task 3.2's boundary note): both sites already correctly + * dereference(j) exactly once today, matching the one reference(j) taken at + * the top of dispatch_a_job_() -- this task's new lock/classify/ + * mark_job_fail/unlock insertion sits entirely BETWEEN the existing + * reference and its existing, unmodified dereference, so it must not change + * that balance. Proven via a manual "canary" reference technique (see + * dft_assert_ref_not_over_released below) -- job_t's real refcount lives in + * jobj.c's file-local job_private_t, with no accessor exposed to a test + * (out of this task's boundary to add one), so an exact numeric read isn't + * available; the canary technique still gives a real, ASAN-backed proof + * that dispatch_a_job_test() released the reference it took exactly once + * (an over-release would free j while the canary ref is still held, + * producing a heap-use-after-free the very next time j is touched). + * + * dispatch_a_job_() is static/file-local; reached here through + * dispatch_a_job_test(), task 3.1's existing XCE_TESTING-only wrapper -- + * reused directly, not duplicated (this task's own instruction). + * + * Fixture mirrors test_dispatch_commit.c's dc_setup/dc_teardown pattern + * (real jobmgr/workermgr/qmgr/hooksys + a real disk-backed persist_t with + * cluster == NULL) trimmed to what these two failure sites need. No hook + * functions are ever registered against g_dft.hooksys in this fixture (no + * on_scheduler_join()/on_apiserver_join()/on_persist_join() call, matching + * test_dispatch_commit.c's own fixture) -- mark_job_fail()'s two + * xce_run_hooks() calls therefore iterate zero registered handlers and are + * safe no-ops here, exactly as they are in every other minimal-fixture test + * in this suite that never sets up hook consumers. + */ + +#include + +#include +#include +#include +#include +#include + +#include + +#include "cluster.h" +#include "hook.h" +#include "jobj.h" +#include "kvmgr.h" +#include "netio.h" +#include "persist.h" +#include "qobj.h" +#include "wobj.h" +#include "xce/cluster_def.h" +#include "xce/types.h" +#include "xcealloc.h" +#include "xstring.h" + +/* dispatch_a_job_test: task 3.1's XCE_TESTING-only wrapper (job.c), thinly + * exposing the static dispatch_a_job_() for direct testing. */ +extern int dispatch_a_job_test(cluster_t* c, endpoint_t* ep, + xce_job_alloc_t* alloc); + +/* jobj.c's job type table and the process-global xstring table are shared + * across every test file in the controller_unit_test binary -- guard + * idempotently, mirroring test_dispatch_commit.c's g_dc_jobobj_ready. */ +static int g_dft_jobobj_ready; + +static cluster_t g_dft; +static char g_dft_tmpdir[256]; + +/* Worker endpoint: zeroed, no iomgr -- xce_startjob_req() (not reached by + * either test in this file, both fail before it) would enqueue against it + * unconditionally if it ever were. Kept for fixture parity with + * test_dispatch_commit.c's g_dc_worker_ep. */ +static endpoint_t g_dft_worker_ep; + +static void dft_setup(void) +{ + kvmgr_params_t int_params = { .key_type = KVMGR_KEY_INT, + .nslots = 32, .delta = 16 }; + kvmgr_params_t str_params = { .key_type = KVMGR_KEY_STRING, + .nslots = 32, .delta = 16 }; + persist_config_t cfg; + + if (makeString((char*)"dft-probe") == NULL) + xstring_table_init(64); + + if (!g_dft_jobobj_ready) { + TEST_ASSERT_EQUAL_INT(0, job_obj_init(32, 32)); + g_dft_jobobj_ready = 1; + } + + memset(&g_dft, 0, sizeof(g_dft)); + TEST_ASSERT_EQUAL_INT(0, + xce_hook_init(&g_dft.hooksys, XCE_CONTROLLER_HOOK_MAX)); + + g_dft.jobmgr = makeInstance(&xkvmgr_type, &int_params); + g_dft.workermgr = makeInstance(&xkvmgr_type, &str_params); + g_dft.qmgr = makeInstance(&xkvmgr_type, &str_params); + TEST_ASSERT_NOT_NULL(g_dft.jobmgr); + TEST_ASSERT_NOT_NULL(g_dft.workermgr); + TEST_ASSERT_NOT_NULL(g_dft.qmgr); + + cfg = persist_config_defaults(); + cfg.wal_sync_ms = 5000; + strcpy(g_dft_tmpdir, "/tmp/xce_dft32_XXXXXX"); + TEST_ASSERT_NOT_NULL(mkdtemp(g_dft_tmpdir)); + persist_set_workdir(&g_dft.persist, g_dft_tmpdir, "ctrl-test"); + TEST_ASSERT_EQUAL_INT(0, persist_init(&g_dft.persist, &cfg, NULL)); + TEST_ASSERT_EQUAL_INT(0, persist_start(&g_dft.persist)); + + memset(&g_dft_worker_ep, 0, sizeof(g_dft_worker_ep)); + g_dft_worker_ep.fd = -1; +} + +static void dft_teardown(void) +{ + char cmd[512]; + + persist_stop(&g_dft.persist); + persist_fini(&g_dft.persist); + + if (g_dft.workermgr) dereference(g_dft.workermgr); + if (g_dft.qmgr) dereference(g_dft.qmgr); + if (g_dft.jobmgr) dereference(g_dft.jobmgr); + xce_hook_fini(&g_dft.hooksys); + + snprintf(cmd, sizeof(cmd), "rm -rf %s", g_dft_tmpdir); + if (system(cmd) != 0) + fprintf(stderr, "cleanup warning: rm -rf %s failed\n", g_dft_tmpdir); +} + +/* Seeds a PEND job with a real submit/start_info (dispatch_a_job_() reads + * submit->queue and submit->start_info) against g_dft.jobmgr. Does NOT + * register any queue or worker -- callers add exactly what their scenario + * needs (SITE_INVALID_ALLOC needs neither; SITE_QUEUE_EXEC needs a ready + * worker but a deliberately-unregistered queue name). Returns the seeded + * job_t* (borrowed -- g_dft.jobmgr owns the creation reference). */ +static job_t* dft_seed_job(uint64_t jobid, const char* qname) +{ + job_startinfo_t startinfo; + job_submit_data_t submit; + xce_job_t jdef; + job_t* j; + + memset(&startinfo, 0, sizeof(startinfo)); + startinfo.cmdline = (char*)"/bin/true"; + memset(&submit, 0, sizeof(submit)); + submit.queue = (char*)qname; + submit.user = (char*)"tester"; + submit.start_info = &startinfo; + + memset(&jdef, 0, sizeof(jdef)); + jdef.jobid = jobid; + jdef.state = JOB_STAT_PEND; + jdef.submit = &submit; + j = makeJob(g_dft.jobmgr, &jdef); + TEST_ASSERT_NOT_NULL(j); + + return j; +} + +/* Registers a ready worker (wname) in g_dft.workermgr, ep = g_dft_worker_ep + * (host.status defaults to 0/ready via calloc). Mirrors test_dispatch_ + * commit.c's dc_seed_ready's worker half. */ +static void dft_seed_ready_worker(const char* wname) +{ + xce_worker_t wdef; + hostattrs_t* attrs; + worker_t* w = NULL; + + attrs = calloc(1, sizeof(*attrs)); + TEST_ASSERT_NOT_NULL(attrs); + attrs->hosttype = makeString("LINUX"); + attrs->hostmodel = makeString("x86"); + attrs->cpufactor = 1.0f; + attrs->maxcpus = 4; + memset(&wdef, 0, sizeof(wdef)); + wdef.name = (char*)wname; + wdef.attrs = attrs; + TEST_ASSERT_NOT_NULL(makeWorker(g_dft.workermgr, &wdef)); + + lock(g_dft.workermgr, RTTI_LOCK_READ); + key_find(g_dft.workermgr, (void*)wname, (void**)&w); + unlock(g_dft.workermgr); + TEST_ASSERT_NOT_NULL(w); + w->ep = &g_dft_worker_ep; +} + +/* Canary-reference technique (see file header): the caller takes one extra + * manual reference on j via dft_arm_ref_canary() BEFORE calling + * dispatch_a_job_test(), so j is guaranteed to survive the call regardless + * of what it does internally; dft_assert_ref_not_over_released() then + * releases exactly that one canary reference AFTER the call and re-touches + * j -- proving dispatch_a_job_test() released the ONE reference it took + * (via the jobid lookup at the top of dispatch_a_job_()) exactly once, not + * zero times (a leak, structurally ruled out by this task's diff never + * adding or removing a reference()/dereference() call -- confirmed by + * inspection, this canary does not independently re-prove that direction) + * and not twice (an over-release/double-free, which THIS canary directly + * proves did not happen: an extra dereference during the call would free j + * while the canary ref is still outstanding, and touching j afterward -- + * either inside dispatch_a_job_test() itself or in this function -- would + * be a heap-use-after-free that ASAN aborts on). Arming the canary before + * the call, not after, is what makes this an independent detection + * mechanism rather than relying incidentally on the caller's own + * post-call field-read assertions to crash first. */ +static void dft_arm_ref_canary(job_t* j) +{ + reference(j); +} + +static void dft_assert_ref_not_over_released(job_t* j, uint64_t expect_jobid) +{ + dereference(j); + + TEST_ASSERT_EQUAL_UINT64_MESSAGE(expect_jobid, j->job.jobid, + "j must still be safely readable after releasing the canary " + "reference -- an over-release inside dispatch_a_job_test() would " + "have freed j while the canary was still held, making this a " + "heap-use-after-free"); +} + +#define DFT_JOBID_INVALID_ALLOC 91101ULL + +/* SITE 1: XCE_DISPATCH_FAIL_SITE_INVALID_ALLOC (alloc->nhosts <= 0). + * Fails before any worker lookup -- no worker/queue registration needed + * beyond the job itself. */ +static void test_invalid_alloc_resolves_job_to_exit(void) +{ + job_t* j; + xce_job_alloc_t* alloc; + int rc; + + dft_setup(); + + j = dft_seed_job(DFT_JOBID_INVALID_ALLOC, "dft-q1"); + + alloc = make_job_alloc(DFT_JOBID_INVALID_ALLOC, 0, NULL); + TEST_ASSERT_NOT_NULL(alloc); + + dft_arm_ref_canary(j); + rc = dispatch_a_job_test(&g_dft, NULL, alloc); + TEST_ASSERT_EQUAL_INT_MESSAGE(-EINVAL, rc, + "an invalid allocation (nhosts <= 0) must still return -EINVAL, " + "unchanged from before this task"); + + TEST_ASSERT_EQUAL_UINT32_MESSAGE(JOB_STAT_EXIT, j->job.state, + "Req 4.2: a terminally-classified dispatch failure must resolve " + "the job to EXIT immediately, not leave it at PEND"); + TEST_ASSERT_NULL_MESSAGE(j->job.allocs, + "Req 4.5: no partial allocation data may be left behind"); + + dft_assert_ref_not_over_released(j, DFT_JOBID_INVALID_ALLOC); + + free_JobAlloc_ptr(&alloc); + dft_teardown(); + printf("test_invalid_alloc_resolves_job_to_exit: PASS\n"); +} + +#define DFT_JOBID_QUEUE_EXEC 91102ULL + +/* SITE 2: XCE_DISPATCH_FAIL_SITE_QUEUE_EXEC (get_queue_exec() failure). + * Needs a real, ready worker (so the function reaches past the worker + * checks) but a job whose submit->queue names a queue deliberately never + * registered in g_dft.qmgr, so get_queue_exec()'s key_find() fails with + * -ENOENT (queue.c:get_queue_exec, unmodified by this task). */ +static void test_queue_exec_failure_resolves_job_to_exit(void) +{ + job_t* j; + xce_job_alloc_t* alloc; + int rc; + + dft_setup(); + + j = dft_seed_job(DFT_JOBID_QUEUE_EXEC, "dft-nonexistent-queue"); + dft_seed_ready_worker("dft-wkr"); + + alloc = make_job_alloc(DFT_JOBID_QUEUE_EXEC, 1, + make_host_allocs(1, + make_host_alloc((char*)"dft-wkr", 1, + make_allocs(1, + make_alloc((char*)"slots", NULL, 0, 1.0f, 0))))); + TEST_ASSERT_NOT_NULL(alloc); + + dft_arm_ref_canary(j); + rc = dispatch_a_job_test(&g_dft, NULL, alloc); + TEST_ASSERT_EQUAL_INT_MESSAGE(-ENOENT, rc, + "get_queue_exec() failing on an unregistered queue name must " + "still return -ENOENT, unchanged from before this task"); + + TEST_ASSERT_EQUAL_UINT32_MESSAGE(JOB_STAT_EXIT, j->job.state, + "Req 4.2: a terminally-classified dispatch failure must resolve " + "the job to EXIT immediately, not leave it at PEND"); + TEST_ASSERT_NULL_MESSAGE(j->job.allocs, + "Req 4.5: no partial allocation data may be left behind"); + + dft_assert_ref_not_over_released(j, DFT_JOBID_QUEUE_EXEC); + + free_JobAlloc_ptr(&alloc); + dft_teardown(); + printf("test_queue_exec_failure_resolves_job_to_exit: PASS\n"); +} + +void test_dispatch_fail_terminal(void) +{ + UnitySetTestFile(__FILE__); + printf("\n>> dispatch_a_job_() terminal dispatch-failure resolution " + "tests (Req 4.2, 4.5)\n\n"); + + RUN_TEST(test_invalid_alloc_resolves_job_to_exit); + RUN_TEST(test_queue_exec_failure_resolves_job_to_exit); +}