fix(bug-1730): free job accounts/allocs on JOB_UPDATE terminal transition

XCE_CORE_EVENT_JOB_UPDATE (core_events.c) is the only currently-live path
a controller job-finish notification reaches, but unlike its sibling
XCE_CORE_EVENT_JOB_STATE_CHANGE case, it never released job->accounts/
job->allocs on a terminal transition, leaking them for the scheduler
process's lifetime. Mirrors the existing JOB_STATE_CHANGE cleanup.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019QxaSzHRB7pmYFp4cVnwkA
This commit is contained in:
Rongsong Shen 2026-08-29 11:28:32 +00:00
parent f0a3729f24
commit b1323f8dab
No known key found for this signature in database
GPG key ID: 5BCDAAE0123E9FE2
2 changed files with 88 additions and 0 deletions

View file

@ -952,6 +952,27 @@ int xce_core_push_events(xce_sched_core_t* core, xce_core_event_t* events,
job->state = updated->state; 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 != NULL) {
if (updated->submit->queue != NULL) { if (updated->submit->queue != NULL) {
xce_sched_queue_t* q xce_sched_queue_t* q

View file

@ -53,6 +53,7 @@
#include "kvmgr.h" #include "kvmgr.h"
#include "scheduler.h" #include "scheduler.h"
#include "xcealloc.h"
#include "xstring.h" #include "xstring.h"
/* Pull in the real xce_sched_core_init()/xce_core_push_events() under test /* 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)); 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 /* WORKERS_PUSH/RESOURCES_PUSH/RESVALS_FULL/RESVALS_DELTA are state-update
* only -- Req 1 AC 2: "Hooks not defined on the picker interface (worker * only -- Req 1 AC 2: "Hooks not defined on the picker interface (worker
* updates, resource updates, resval updates) are not forwarded to the * updates, resource updates, resval updates) are not forwarded to the
@ -973,6 +1039,7 @@ int main(void)
UNITY_BEGIN(); UNITY_BEGIN();
RUN_TEST(test_push_events_fires_each_hook_once_per_event_with_picker_idle); 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_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_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_twice_updates_existing_worker_in_place);
RUN_TEST(test_workers_push_with_null_val_does_not_clobber_existing_resval); RUN_TEST(test_workers_push_with_null_val_does_not_clobber_existing_resval);