diff --git a/src/scheduler/src/core_events.c b/src/scheduler/src/core_events.c index 69a18aae..7b0eeaa8 100644 --- a/src/scheduler/src/core_events.c +++ b/src/scheduler/src/core_events.c @@ -952,6 +952,27 @@ int xce_core_push_events(xce_sched_core_t* core, xce_core_event_t* events, job->state = updated->state; + /* Release resource allocation/accounting on a terminal + * transition -- mirrors XCE_CORE_EVENT_JOB_STATE_CHANGE's + * identical cleanup above. JOB_UPDATE is the only currently-live + * path a controller's job-finish notification actually reaches + * (HOOK_DETAIL_JOB_UPDATE via sched_job_update_fun(); JOB_STATE_ + * CHANGE's own controller-side trigger, HOOK_DETAIL_JOB_EXIT/ + * START/FAIL at HOOK_AFTER_EVENT, is never fired anywhere in the + * controller -- job.c only ever fires those at HOOK_BEFORE_EVENT, + * for WAL-append purposes), so without this a job finishing via + * JOB_UPDATE leaked its account/alloc objects for the rest of + * the process's life. */ + if (!JOB_IS_RUN(updated->state)) { + if (job->accounts != NULL) { + free_job_account(job->accounts); + job->accounts = NULL; + } + if (job->allocs != NULL) { + free_JobAlloc_ptr(&(job->allocs)); + } + } + if (updated->submit != NULL) { if (updated->submit->queue != NULL) { xce_sched_queue_t* q diff --git a/src/scheduler/testsuite/test_core_events.c b/src/scheduler/testsuite/test_core_events.c index 4d7c715c..a97cb53f 100644 --- a/src/scheduler/testsuite/test_core_events.c +++ b/src/scheduler/testsuite/test_core_events.c @@ -53,6 +53,7 @@ #include "kvmgr.h" #include "scheduler.h" +#include "xcealloc.h" #include "xstring.h" /* Pull in the real xce_sched_core_init()/xce_core_push_events() under test @@ -465,6 +466,71 @@ static void test_jobs_push_fires_on_job_push_once_per_job(void) TEST_ASSERT_NOT_NULL(sched_get_job(&core, 3)); } +/* JOB_UPDATE (core_events.c's live wire-shape mirror of JOB_STATE_CHANGE -- + * the notify-driven single-job path a real controller's HOOK_DETAIL_JOB_UPDATE + * fire reaches, per handler.c's own XCE_SCHED_OP_UPDATE_JOB_FIELDS comment) + * must release a job's resource allocation/accounting the same way + * JOB_STATE_CHANGE's case already does (this file's own + * test_push_events_fires_each_hook_once_per_event_with_picker_idle only + * proves the hook fires -- it never inspects accounts/allocs afterward). + * Without this, a job that transitions to a terminal state via JOB_UPDATE + * (the only currently-live path -- JOB_STATE_CHANGE's own controller-side + * trigger, HOOK_DETAIL_JOB_EXIT/START/FAIL at HOOK_AFTER_EVENT, is never + * actually fired anywhere in the controller) leaks its account/alloc + * objects for the rest of the process's life. */ +static void test_job_update_to_terminal_state_frees_accounts_and_allocs(void) +{ + xce_sched_core_t core; + xce_core_event_t event; + xce_job_t* job1; + xce_job_t updated; + xce_alloc_t* a_slots; + xce_host_alloc_t* h0; + xce_host_alloc_t* hallocs[1]; + xce_job_alloc_t* jalloc; + xce_sched_job_t* job; + int rc; + + fixture_init(&core, &g_full_plugin); + + job1 = make_job(7, JOB_STAT_RUN, "normal", "alice", "proj1"); + + memset(&event, 0, sizeof(event)); + event.type = XCE_CORE_EVENT_JOBS_PUSH; + event.u.jobs_push.njobs = 1; + event.u.jobs_push.jobs = &job1; + + rc = xce_core_push_events(&core, &event, 1); + TEST_ASSERT_EQUAL_INT(0, rc); + + job = sched_get_job(&core, 7); + TEST_ASSERT_NOT_NULL(job); + + a_slots = make_alloc((char*)"slots", NULL, 0, 4.0f, 0); + h0 = make_host_alloc((char*)"hostA", 1, make_allocs(1, a_slots)); + hallocs[0] = h0; + jalloc = make_job_alloc(7, 1, make_host_allocs(1, hallocs[0])); + job->allocs = jalloc; + job->accounts = make_job_account(job->allocs); + TEST_ASSERT_NOT_NULL(job->accounts); + + memset(&updated, 0, sizeof(updated)); + updated.jobid = 7; + updated.state = JOB_STAT_DONE; + + memset(&event, 0, sizeof(event)); + event.type = XCE_CORE_EVENT_JOB_UPDATE; + event.u.job_update.jobid = 7; + event.u.job_update.job = &updated; + + rc = xce_core_push_events(&core, &event, 1); + TEST_ASSERT_EQUAL_INT(0, rc); + + TEST_ASSERT_EQUAL_UINT32(JOB_STAT_DONE, job->state); + TEST_ASSERT_NULL(job->accounts); + TEST_ASSERT_NULL(job->allocs); +} + /* WORKERS_PUSH/RESOURCES_PUSH/RESVALS_FULL/RESVALS_DELTA are state-update * only -- Req 1 AC 2: "Hooks not defined on the picker interface (worker * updates, resource updates, resval updates) are not forwarded to the @@ -973,6 +1039,7 @@ int main(void) UNITY_BEGIN(); RUN_TEST(test_push_events_fires_each_hook_once_per_event_with_picker_idle); RUN_TEST(test_jobs_push_fires_on_job_push_once_per_job); + RUN_TEST(test_job_update_to_terminal_state_frees_accounts_and_allocs); RUN_TEST(test_non_picker_events_mutate_state_but_never_call_hooks); RUN_TEST(test_workers_push_twice_updates_existing_worker_in_place); RUN_TEST(test_workers_push_with_null_val_does_not_clobber_existing_resval);