From 6c201bb3e8a05d7638ba2a071480802759ff917e Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Tue, 15 Sep 2026 09:01:22 +0000 Subject: [PATCH] fix(scheduler): refuse a stale JOBS_PUSH resync claim that would destroy a real allocation A resync redelivery (on_scheduler_join()'s full push, fired on every scheduler reconnect) can race a job's own very-recent dispatch confirmation and carry a stale, non-terminal state for it. The duplicate-jobid merge in core_events.c trusted this claim unconditionally, freeing the job's already-captured allocation and regressing its state -- live-confirmed via a production scheduler.log showing a resync WARN landing 16ms after the job's own RUN confirmation, and matching a live-reported simulator symptom (a RUN job silently missing from info worker's allocation listing). Refuse a non-terminal claim when the existing job already has a real allocation on record, mirroring the existing phantom-RUN guard's own reasoning in the opposite direction; a genuinely terminal claim (DONE/EXIT) still applies and releases the allocation correctly, same as before. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01RLSNJhNoULvVcUhXEFafQC --- src/scheduler/src/core_events.c | 36 ++++++ src/scheduler/testsuite/test_core_events.c | 137 +++++++++++++++++++++ 2 files changed, 173 insertions(+) diff --git a/src/scheduler/src/core_events.c b/src/scheduler/src/core_events.c index d019493c..214050c6 100644 --- a/src/scheduler/src/core_events.c +++ b/src/scheduler/src/core_events.c @@ -930,6 +930,42 @@ int xce_core_push_events(xce_sched_core_t* core, xce_core_event_t* events, picker, existing, job->state); } } + } else if ((existing->allocs != NULL) + && !JOB_IS_DONE(job->state) + && !JOB_IS_EXIT(job->state)) { + /* bug-1812 ("stale resync regression"): symmetric + * to bug-1739's own phantom-RUN guard above, but + * for the descent direction. `existing` is + * currently RUN with a REAL, already-captured + * allocation -- this resync redelivery claims + * some OTHER, non-terminal state (e.g. PEND) with + * no allocation of its own to justify it. + * on_scheduler_join()'s full resync + * (xcesched_push_jobs(), controller/sched.c) + * fires on every scheduler reconnect, built from + * collect_jobs_committed() -- a committed-data + * snapshot that can race the scheduler's own very + * recent, self-initiated dispatch (live-confirmed + * via a production scheduler.log: a JOBS_PUSH + * resync WARN landed just 16ms after this exact + * job's own schedStartJob confirmation). Trusting + * this claim would destroy a real allocation the + * scheduler's own resource/slot bookkeeping + * already correctly has -- the same phantom-RUN- + * shaped data loss bug-1739 already guards + * against, just reached from the opposite state + * transition. A genuinely TERMINAL claim + * (DONE/EXIT) is NOT covered by this guard: that + * is legitimate "job finished while this + * scheduler was disconnected" information + * (bug-1734's own motivating case) and must + * still release the allocation and apply. */ + WARN("%s JOBS_PUSH: refusing resync state 0x%x " + "for jobid %s -- existing job is RUN with a " + "real allocation on record (existing state " + "0x%x kept)", + __func__, job->state, jobidStr, + existing->state); } else { if (existing->accounts != NULL) { free_job_account(existing->accounts); diff --git a/src/scheduler/testsuite/test_core_events.c b/src/scheduler/testsuite/test_core_events.c index d9ddd7f9..54dfcecd 100644 --- a/src/scheduler/testsuite/test_core_events.c +++ b/src/scheduler/testsuite/test_core_events.c @@ -739,6 +739,141 @@ test_jobs_push_duplicate_jobid_run_state_without_allocation_is_refused(void) TEST_ASSERT_NULL(tracked->allocs); } +/* bug-1812 ("stale resync regression"): symmetric to bug-1739's own + * "phantom RUN" guard just above (refusing to FABRICATE an allocation-less + * RUN claim), but for the descent direction -- a JOBS_PUSH resync + * redelivery must not DESTROY a job's REAL, already-captured allocation by + * trusting an unsupported non-RUN, non-terminal claim over it. + * Live-confirmed via a real production scheduler.log: on_scheduler_join()'s + * full resync (xcesched_push_jobs(), controller/sched.c) fires on every + * scheduler reconnect, built from collect_jobs_committed() -- a + * committed-data snapshot that can race the scheduler's own very recent, + * self-initiated dispatch. The log showed a JOBS_PUSH resync WARN landing + * just 16ms after this exact job's own schedStartJob confirmation + * (sched_job_state_change -> state 2), repeating in bursts on later + * reconnects -- each collision wiping the job's real allocation while its + * state stayed RUN, exactly matching the simulator's live-reported symptom + * ("info jobs" shows RUN, "info worker"'s assigned-jobs listing -- which + * walks job->allocs -- omits it). Asserts a job tracked RUN with a real + * allocation, redelivered via resync as PEND with no allocation, KEEPS its + * RUN state and real allocation -- the resync's unsupported claim must be + * refused, not trusted. */ +static void +test_jobs_push_duplicate_jobid_run_with_allocation_survives_stale_nonrun_resync( + void) +{ + xce_sched_core_t core; + xce_core_event_t event; + xce_job_t* job1; + xce_job_t* stale_resync_job; + xce_sched_job_t* tracked; + xce_alloc_t* a_slots; + xce_host_alloc_t* h0; + xce_host_alloc_t* hallocs[1]; + xce_job_alloc_t* jalloc; + int rc; + + fixture_init(&core, &g_full_plugin); + + job1 = make_job(65, 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); + + tracked = sched_get_job(&core, 65); + TEST_ASSERT_NOT_NULL(tracked); + + /* Give it a real, captured allocation -- mirrors what a genuine + * dispatch (scheduler.c's sched_commit_decision()) would already have + * set by the time a stale resync arrives. */ + a_slots = make_alloc((char*)"slots", NULL, 0, 1.0f, 0); + h0 = make_host_alloc((char*)"hostA", 1, make_allocs(1, a_slots)); + hallocs[0] = h0; + jalloc = make_job_alloc(65, 1, make_host_allocs(1, hallocs[0])); + tracked->allocs = jalloc; + tracked->accounts = make_job_account(tracked->allocs); + + /* A stale on_scheduler_join() resync redelivery -- the controller's + * committed-data snapshot hasn't yet caught up to this job's own very + * recent dispatch, so it still reports PEND with no allocation. */ + stale_resync_job = make_job(65, JOB_STAT_PEND, "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 = &stale_resync_job; + + rc = xce_core_push_events(&core, &event, 1); + TEST_ASSERT_EQUAL_INT(0, rc); + + TEST_ASSERT_EQUAL_PTR(tracked, sched_get_job(&core, 65)); + /* Must still be RUN with its real allocation intact -- the stale resync + * claim must not have clobbered it. */ + TEST_ASSERT_EQUAL_UINT32(JOB_STAT_RUN, tracked->state); + TEST_ASSERT_EQUAL_PTR(jalloc, tracked->allocs); + TEST_ASSERT_NOT_NULL(tracked->accounts); +} + +/* Companion: a resync claiming a genuinely TERMINAL state (DONE/EXIT) for a + * RUN-with-real-allocation job must still apply -- this is the legitimate + * "job finished while this scheduler was disconnected" case bug-1734's own + * fix exists for, and the new stale-resync guard above must not block it. */ +static void +test_jobs_push_duplicate_jobid_terminal_resync_releases_real_allocation(void) +{ + xce_sched_core_t core; + xce_core_event_t event; + xce_job_t* job1; + xce_job_t* finished_resync_job; + xce_sched_job_t* tracked; + xce_alloc_t* a_slots; + xce_host_alloc_t* h0; + xce_host_alloc_t* hallocs[1]; + int rc; + + fixture_init(&core, &g_full_plugin); + + job1 = make_job(66, 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); + + tracked = sched_get_job(&core, 66); + TEST_ASSERT_NOT_NULL(tracked); + + a_slots = make_alloc((char*)"slots", NULL, 0, 1.0f, 0); + h0 = make_host_alloc((char*)"hostA", 1, make_allocs(1, a_slots)); + hallocs[0] = h0; + tracked->allocs = make_job_alloc(66, 1, make_host_allocs(1, hallocs[0])); + tracked->accounts = make_job_account(tracked->allocs); + + finished_resync_job + = make_job(66, JOB_STAT_DONE, "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 = &finished_resync_job; + + rc = xce_core_push_events(&core, &event, 1); + TEST_ASSERT_EQUAL_INT(0, rc); + + TEST_ASSERT_EQUAL_PTR(tracked, sched_get_job(&core, 66)); + TEST_ASSERT_EQUAL_UINT32(JOB_STAT_DONE, tracked->state); + TEST_ASSERT_NULL(tracked->allocs); + TEST_ASSERT_NULL(tracked->accounts); +} + /* bug-1738: the JOBS_PUSH duplicate-jobid merge must never regress an * already-terminal job to a DIFFERENT terminal state either -- a resync can * just as easily carry a stale snapshot as a genuinely newer one. Pushes @@ -1717,6 +1852,8 @@ int main(void) RUN_TEST(test_jobs_push_duplicate_jobid_does_not_call_on_job_push_or_leak); RUN_TEST(test_jobs_push_duplicate_jobid_with_newer_state_updates_existing_job); RUN_TEST(test_jobs_push_duplicate_jobid_run_state_transfers_allocation); + RUN_TEST(test_jobs_push_duplicate_jobid_run_with_allocation_survives_stale_nonrun_resync); + RUN_TEST(test_jobs_push_duplicate_jobid_terminal_resync_releases_real_allocation); RUN_TEST(test_jobs_push_duplicate_jobid_run_state_without_allocation_is_refused); RUN_TEST(test_jobs_push_duplicate_jobid_does_not_regress_terminal_state); RUN_TEST(test_job_state_change_does_not_regress_terminal_state);