lei.chen/myxce
1
0
Fork 1
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix(bug-1737): scheduler can never learn a job finished successfully

Browse source 
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019QxaSzHRB7pmYFp4cVnwkA
This commit is contained in:
Rongsong Shen 2026-08-31 04:20:27 +00:00
parent f01ae353ef
commit 026fe97b5a
No known key found for this signature in database
GPG key ID: 5BCDAAE0123E9FE2
6 changed files with 121 additions and 9 deletions
Download patch file Download diff file Expand all files Collapse all files

45
src/controller/sched.c
View file

@ -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:

4
src/include/typemap.h
View file

@ -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;

4
src/lib/helper.c
View file

@ -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:

26
src/scheduler/src/handler.c
View file

@ -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);

43
src/scheduler/testsuite/test_handlers.c
View file

@ -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(&notify, 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, &notify));
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, &notify));
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, &notify));
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();
}

8
src/schema/controller.capnp
View file

@ -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");