From 026fe97b5ab3cac62e29c7a827fea7fa95279ab5 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Mon, 31 Aug 2026 04:20:27 +0000 Subject: [PATCH] fix(bug-1737): scheduler can never learn a job finished successfully sched_job_exit_fun() (renamed sched_job_finish_fun, sched.c) subscribes to HOOK_DETAIL_JOB_EXIT, which fires for EVERY job finish, both success and failure -- record_job_finish_fact() (job.c) decides DONE vs EXIT from finish->exit_code BEFORE firing this same hook detail either way (mirrors persist.c's own HOOK_DETAIL_JOB_EXIT -> WAL_JOB_FINISH mapping). This function discarded info->exit_code and sent only a bare jobid, forcing sched_handle_notify() (scheduler/src/handler.c) to unconditionally map EVERY finish -- including a clean, successful one -- to JOB_STAT_EXIT. A subsequent HOOK_DETAIL_JOB_UPDATE notify usually self-corrected this moments later via the full job state, but if that second notify was dropped during a scheduler reconnect gap (the same window bug-1731/bug-1734 already documented), the scheduler was left permanently stuck showing EXIT for a job the controller correctly recorded as DONE. Live-reproduced on a real 3-node HA cluster + worker: job 0-8-1788143749-0[38] genuinely completed successfully (worker log: "completed successfully", controller joblist: DONE) but the scheduler's own replay showed EXIT. Fix: carry {jobid, exit_code} across the wire via the existing JobFinishInfo shape (the same struct apiserver_job_exit_fun already reuses for this identical hook) instead of a bare UInt64, so sched_handle_notify() can make the identical exit_code==0-vs-not decision job.c itself makes. Renamed sched_job_exit_fun to sched_job_finish_fun to reflect what it actually does. Live-verified: a 51-job run (all exit 0) showed 100% DONE on the scheduler side after the fix. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_019QxaSzHRB7pmYFp4cVnwkA --- src/controller/sched.c | 45 +++++++++++++++++++++++-- src/include/typemap.h | 4 ++- src/lib/helper.c | 4 ++- src/scheduler/src/handler.c | 26 ++++++++++++-- src/scheduler/testsuite/test_handlers.c | 43 +++++++++++++++++++++++ src/schema/controller.capnp | 8 ++++- 6 files changed, 121 insertions(+), 9 deletions(-) diff --git a/src/controller/sched.c b/src/controller/sched.c index efe37cda..44374fde 100644 --- a/src/controller/sched.c +++ b/src/controller/sched.c @@ -366,7 +366,30 @@ static int sched_job_start_fun(int type, int situation, void* data, va_list ap) return rc; } -static int sched_job_exit_fun(int type, int situation, void* data, va_list ap) +/* + * sched_job_finish_fun (bug-1737, renamed from sched_job_exit_fun) -- + * subscribes to HOOK_DETAIL_JOB_EXIT, which despite its name fires for + * EVERY job finish, both success and failure (record_job_finish_fact(), + * job.c, decides DONE vs EXIT from finish->exit_code BEFORE firing this + * same hook detail either way -- mirrors persist.c's own WAL_JOB_FINISH -> + * HOOK_DETAIL_JOB_EXIT mapping, `case HOOK_DETAIL_JOB_EXIT: return + * WAL_JOB_FINISH;`). This function previously discarded info->exit_code and + * sent only the bare jobid, forcing sched_handle_notify() (scheduler/src/ + * handler.c) to unconditionally map EVERY finish -- including a clean, + * successful one -- to JOB_STAT_EXIT. Under normal conditions a subsequent + * HOOK_DETAIL_JOB_UPDATE fire (persist.c, same WAL apply) self-corrected + * this moments later via schedUpdateJob's full xce_job_t (whose ->state is + * already the real terminal value), but if THAT second notify was dropped + * (c->sched.ep briefly NULL during a reconnect -- the same window bug-1731/ + * bug-1734 already documented) the scheduler was left permanently stuck + * showing EXIT for a job the controller correctly recorded as DONE. Fix: + * carry info (jobid + exit_code) across the wire via the existing + * JobFinishInfo wire shape (same struct apiserver_job_exit_fun, + * apiserver.c, already reuses for this exact hook) so + * sched_handle_notify() can make the identical exit_code==0-vs-not decision + * job.c itself makes, instead of guessing EXIT unconditionally. + */ +static int sched_job_finish_fun(int type, int situation, void* data, va_list ap) { cluster_t* c = va_arg(ap, cluster_t*); xce_jobfinish_t* info = va_arg(ap, xce_jobfinish_t*); @@ -375,7 +398,7 @@ static int sched_job_exit_fun(int type, int situation, void* data, va_list ap) xce_notify_data_t xdata = { .event = NotifyData_data_schedExitJob, .data = { - .sched_exit_job = info->jobid + .sched_exit_job = info } }; /* clang-format on */ @@ -396,6 +419,22 @@ static int sched_job_exit_fun(int type, int situation, void* data, va_list ap) return rc; } +/* + * sched_job_fail_fun -- subscribes to HOOK_DETAIL_JOB_FAIL, a narrower event + * than sched_job_finish_fun's HOOK_DETAIL_JOB_EXIT above: it fires for a + * PIPELINE-stage failure (dispatch/RPC error, or a post-exec script + * failing), not for the job's own exit code. apply_job_fail_transition() + * (job.c) only forces JOB_STAT_EXIT for the `default:` case (a genuinely + * pre-exit failure with no exit info yet); for JOB_EXEC_STAGE_POST_EXEC/ + * Q_POST_EXEC it MERGES a JOB_STAT_PERR flag onto whatever DONE/EXIT the + * job's own finish already decided, never re-deciding DONE vs EXIT. This + * function still always sends the same bare-jobid "treat as EXIT" notify + * (matching the `default:` case only) -- bug-1738's + * job_state_regresses_terminal() guard in core_events.c's + * XCE_CORE_EVENT_JOB_STATE_CHANGE is what stops that from clobbering an + * already-DONE job for the POST_EXEC case, since the scheduler has no + * JOB_STAT_PERR-equivalent concept of its own to merge instead. + */ static int sched_job_fail_fun(int type, int situation, void* data, va_list ap) { cluster_t* c = va_arg(ap, cluster_t*); @@ -514,7 +553,7 @@ static int sched_job_hook_fun(int type, int situation, int detail, void* data, case HOOK_DETAIL_JOB_START: return sched_job_start_fun(type, situation, data, ap); case HOOK_DETAIL_JOB_EXIT: - return sched_job_exit_fun(type, situation, data, ap); + return sched_job_finish_fun(type, situation, data, ap); case HOOK_DETAIL_JOB_UPDATE: return sched_job_update_fun(type, situation, data, ap); case HOOK_DETAIL_JOB_FAIL: diff --git a/src/include/typemap.h b/src/include/typemap.h index 521f2a14..5ba1bcc0 100644 --- a/src/include/typemap.h +++ b/src/include/typemap.h @@ -153,7 +153,9 @@ typedef struct { job_failinfo_t* dispatchfail; xce_jobfinish_t* finishinfo; uint64_t sched_start_job; - uint64_t sched_exit_job; + /* bug-1737: carries exit_code alongside jobid now -- see + * controller.capnp's schedExitJob field comment. */ + xce_jobfinish_t* sched_exit_job; uint64_t sched_fail_job; xce_job_t* sched_update_job; xce_resval_list_t* sched_update_resvals; diff --git a/src/lib/helper.c b/src/lib/helper.c index 15b4914b..b602964d 100644 --- a/src/lib/helper.c +++ b/src/lib/helper.c @@ -285,7 +285,9 @@ void* xce_notify_data_ptr(xce_notify_data_t* d) case NotifyData_data_schedStartJob: return (void*)d->data.sched_start_job; case NotifyData_data_schedExitJob: - return (void*)d->data.sched_exit_job; + /* bug-1737: sched_exit_job is now a real xce_jobfinish_t* (was a + * bare uint64_t jobid cast through void*). */ + return d->data.sched_exit_job; case NotifyData_data_schedFailJob: return (void*)d->data.sched_fail_job; case NotifyData_data_schedUpdateJob: diff --git a/src/scheduler/src/handler.c b/src/scheduler/src/handler.c index a2d707e6..aed6a685 100644 --- a/src/scheduler/src/handler.c +++ b/src/scheduler/src/handler.c @@ -533,10 +533,30 @@ int sched_handle_notify(xce_scheduler_t* sched, xce_notify_data_t* notify) rc = sched_job_state_change(sched, notify->data.sched_start_job, JOB_STAT_RUN); break; - case NotifyData_data_schedExitJob: - rc = sched_job_state_change(sched, notify->data.sched_exit_job, - JOB_STAT_EXIT); + case NotifyData_data_schedExitJob: { + /* bug-1737: sched_exit_job now carries exit_code (see + * controller.capnp's schedExitJob field comment) -- mirror + * job.c's own apply_job_finish_transition() decision + * (exit_code==0 -> DONE, else -> EXIT) instead of assuming every + * finish is a failure. Previously this unconditionally mapped + * EVERY job finish, including a clean successful one, to + * JOB_STAT_EXIT; a subsequent schedUpdateJob notify (HOOK_DETAIL_ + * JOB_UPDATE, carrying the real terminal state) USUALLY corrected + * this moments later, but if that second notify was dropped during + * a scheduler reconnect gap (bug-1731/bug-1734's documented + * window) the wrong EXIT stuck permanently. */ + xce_jobfinish_t* finish = notify->data.sched_exit_job; + + if (finish == NULL) { + rc = -EINVAL; + break; + } + + rc = sched_job_state_change(sched, finish->jobid, + (finish->exit_code == 0) ? JOB_STAT_DONE + : JOB_STAT_EXIT); break; + } case NotifyData_data_schedFailJob: rc = sched_job_state_change(sched, notify->data.sched_fail_job, JOB_STAT_EXIT); diff --git a/src/scheduler/testsuite/test_handlers.c b/src/scheduler/testsuite/test_handlers.c index 3beb2757..403588f7 100644 --- a/src/scheduler/testsuite/test_handlers.c +++ b/src/scheduler/testsuite/test_handlers.c @@ -265,6 +265,48 @@ static void test_handle_notify_job_dispatch_fail_enqueues_pend(void) TEST_ASSERT_EQUAL_INT(JOB_STAT_PEND, state); } +/* ── bug-1737: sched_handle_notify's schedExitJob case must branch on + * exit_code (mirroring job.c's own apply_job_finish_transition() decision) + * instead of unconditionally assuming every job finish is a failure ── */ + +static void test_handle_notify_job_exit_maps_exit_code_to_done_or_exit(void) +{ + xce_scheduler_t sched; + xce_notify_data_t notify; + xce_jobfinish_t finish = { .jobid = 200, .exit_code = 0 }; + uint64_t jobid = 0; + uint32_t state = (uint32_t)-1; + fixture_init(&sched); + + memset(¬ify, 0, sizeof(notify)); + notify.event = NotifyData_data_schedExitJob; + notify.data.sched_exit_job = &finish; + + /* exit_code == 0 -- clean finish -- must resolve to JOB_STAT_DONE, not + * the previous unconditional JOB_STAT_EXIT. */ + TEST_ASSERT_EQUAL_INT(0, sched_handle_notify(&sched, ¬ify)); + TEST_ASSERT_EQUAL_INT(1, pop_jstate_change(&sched, &jobid, &state)); + TEST_ASSERT_EQUAL_UINT64(200, jobid); + TEST_ASSERT_EQUAL_INT(JOB_STAT_DONE, state); + + /* exit_code != 0 -- a real failure -- still resolves to JOB_STAT_EXIT. */ + fixture_init(&sched); + finish.jobid = 201; + finish.exit_code = 7; + jobid = 0; + state = (uint32_t)-1; + TEST_ASSERT_EQUAL_INT(0, sched_handle_notify(&sched, ¬ify)); + TEST_ASSERT_EQUAL_INT(1, pop_jstate_change(&sched, &jobid, &state)); + TEST_ASSERT_EQUAL_UINT64(201, jobid); + TEST_ASSERT_EQUAL_INT(JOB_STAT_EXIT, state); + + /* NULL payload -- malformed notify -- must fail cleanly, not crash. */ + fixture_init(&sched); + notify.data.sched_exit_job = NULL; + TEST_ASSERT_EQUAL_INT(-EINVAL, sched_handle_notify(&sched, ¬ify)); + TEST_ASSERT_EQUAL_INT(0, pop_op(&sched, NULL)); +} + int main(int argc, char* argv[]) { (void)argc; @@ -277,5 +319,6 @@ int main(int argc, char* argv[]) RUN_TEST(test_handle_resources_skips_empty_enqueues_nonempty, 0); RUN_TEST(test_handle_resvals_full_skips_empty_enqueues_nonempty, 0); RUN_TEST(test_handle_notify_job_dispatch_fail_enqueues_pend, 0); + RUN_TEST(test_handle_notify_job_exit_maps_exit_code_to_done_or_exit, 0); return UNITY_END(); } diff --git a/src/schema/controller.capnp b/src/schema/controller.capnp index 3e6f6776..a8827ac6 100644 --- a/src/schema/controller.capnp +++ b/src/schema/controller.capnp @@ -504,7 +504,13 @@ struct NotifyData $C.mapname("xce_notify_data_t"){ jobDispatchFail @2: JobFail $C.mapname("dispatchfail"); jobFinishInfo @3: JobFinishInfo $C.mapname("finishinfo"); schedStartJob @4: UInt64 $C.mapname("sched_start_job"); - schedExitJob @5: UInt64 $C.mapname("sched_exit_job"); + # bug-1737: was a bare UInt64 jobid -- carried no exit_code, so + # sched_handle_notify() (scheduler/src/handler.c) could not distinguish + # a successful finish (DONE) from a failed one (EXIT) and unconditionally + # assumed EXIT. Reuses the existing JobFinishInfo shape (jobid + code) + # apiserver_job_exit_fun (apiserver.c) already sends for this identical + # HOOK_DETAIL_JOB_EXIT hook, via NotifyData_data_jobFinishInfo. + schedExitJob @5: JobFinishInfo $C.mapname("sched_exit_job"); schedFailJob @6: UInt64 $C.mapname("sched_fail_job"); schedUpdateJob @7: SchedJob $C.mapname("sched_update_job"); schedUpdateResvals @8: ResValList $C.mapname("sched_update_resvals");