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

scheduler audit P1: picker hook validation + rejection WARNs + sim bootstrap by name

Browse source 
P1-1: sched_resolve_job_picker() now validates all nine MUST hooks --
the four state-change hooks (on_queue_update/on_job_push/
on_job_state_change/on_job_update) were called unconditionally by
core_events.c, scheduler.c and snapshot_load.c but never validated at
resolution time, so a picker omitting one segfaulted on its first event
instead of failing startup validation with -EINVAL. Doc comment on
core.h's declaration updated to match.

P1-2: handle the picker hooks' return values instead of dropping them.
The known real producer of a rejection is builtin_fcfs's "queue not
(yet) in the candidate registry" -EINVAL: the job stayed tracked in
core->jobs but was never a scheduling candidate, with no log at all --
invisible forever (no dispatch, no PEND reason, nothing). Five sites in
core_events.c now WARN on rc < 0: the JOBS_PUSH fresh-job site plus a
root-cause WARN in core_upsert_queue (a rejected queue makes every job
in it fail too), both JOBS_PUSH resync-merge on_job_state_change sites,
JOB_STATE_CHANGE and JOB_UPDATE. All non-fatal: the push itself still
succeeds and the job stays tracked.

P1-3: sim_bootstrap_picker() is now name-driven with an explicit WARN
and graceful degrade for any picker name that has no sim-viable builtin
picker -- including the legacy "xce_sched_fcfs", whose job_elect_fn
reads plugin->sched (an xce_scheduler_t* the sim driver never has, and
which plugin_register_core() deliberately leaves NULL with no legacy
wrapper), so bootstrapping it would crash the rebuild walk on
picker_iface == NULL. Previously an unknown name silently returned
while sim still warned downstream. A short experiment with bootstrapping
the legacy plugin via an fcfs_new_plugin() factory was reverted: making
that plugin core-only-viable is a refactor of a plugin slated for
removal (audit P2 #18), not a bootstrap fix.

Tests:
- sched_picker_wiring_test: new test_picker_missing_state_change_hooks_
  fails_with_einval wires the five ORIGINAL MUST hooks and leaves ONLY
  the four state-change hooks NULL, so the asserted -EINVAL can only
  come from the new checks.
- sched_core_events_test: new test_jobs_push_picker_rejection_still_
  tracks_job_and_succeeds proves the push succeeds, the hook fires
  once, and sched_get_job() still resolves the job after a rejection.

P2 items from the same audit are recorded (not fixed) in gaps.md
section 七 (items 18-21): legacy fcfs zombie, dead scaffolding fields,
preemption without a producer, job-priority chain.

134/134 tests pass.

Co-Authored-By: Claude Code <noreply@anthropic.com>
This commit is contained in:
lei.chen 2026-10-22 14:05:38 +00:00
parent c194973d42
commit d16954ad10
8 changed files with 268 additions and 21 deletions
Download patch file Download diff file Expand all files Collapse all files

8
src/scheduler/include/core.h
View file

@ -383,8 +383,12 @@ extern int sched_queue_is_ready(xce_sched_queue_t*);
* (core->plugins, the same plugin_register()/key_find() mechanism every
* other plugin category uses) and validates the result actually implements
* the picker MUST hooks (xce_job_picker_iface_t: next_job, begin_session,
* end_session, on_dispatch, on_pend). Returns 0 and sets *out on success; a
* negative errno on failure:
* end_session, on_dispatch, on_pend, plus the four state-change hooks
* on_queue_update, on_job_push, on_job_state_change, on_job_update --
* core_events.c/scheduler.c/snapshot_load.c call those unconditionally on
* any resolved picker, so a picker missing one would crash on its first
* state-change event rather than fail startup validation). Returns 0 and
* sets *out on success; a negative errno on failure:
* -EINVAL bad arguments, or core->job_pick_plugin is unset (NULL), or
* the name IS registered but is not a usable picker
* -ENOENT the configured name is not registered at all

81
src/scheduler/src/core_events.c
View file

@ -416,6 +416,7 @@ static void core_upsert_queue(xce_sched_core_t* core, xce_sched_plugin_t* picker
xce_queue_t* updated)
{
xce_sched_queue_t* q;
int qrc;
if ((updated == NULL) || (updated->queue == NULL)) {
WARN("%s NULL queue payload", __func__);
@ -456,7 +457,18 @@ static void core_upsert_queue(xce_sched_core_t* core, xce_sched_plugin_t* picker
}
if (picker != NULL) {
picker->picker_iface->on_queue_update(picker, q);
/* A rejected on_queue_update leaves the queue out of the picker's
* candidate registry -- every job in it then gets builtin_fcfs's
* "unknown queue" -EINVAL from on_job_push too (each one WARNed by
* the JOBS_PUSH site). Log this root cause as well so the first
* failure is visible in the log rather than only its downstream
* echoes. */
qrc = picker->picker_iface->on_queue_update(picker, q);
if (qrc < 0) {
WARN("%s picker rejected queue '%s' (rc=%d) -- its jobs will "
"not be scheduling candidates until it is accepted",
__func__, (q->name != NULL) ? q->name : "?", qrc);
}
}
}
@ -1048,9 +1060,13 @@ int xce_core_push_events(xce_sched_core_t* core, xce_core_event_t* events,
existing->state = job->state;
if (picker != NULL) {
picker->picker_iface->on_job_state_change(
picker, existing, job->state);
rc = picker->picker_iface->on_job_state_change(
picker, existing, job->state);
if (rc < 0) {
WARN("%s JOBS_PUSH: picker rejected "
"state merge for jobid %s "
"(rc=%d)",
__func__, jobidStr, rc);
}
}
/* bug-1860: no refusal for a non-RUN, non-terminal
@ -1075,9 +1091,12 @@ int xce_core_push_events(xce_sched_core_t* core, xce_core_event_t* events,
existing->state = job->state;
if (picker != NULL) {
picker->picker_iface->on_job_state_change(
picker, existing, job->state);
rc = picker->picker_iface->on_job_state_change(
picker, existing, job->state);
if (rc < 0) {
WARN("%s JOBS_PUSH: picker rejected "
"state merge for jobid %s (rc=%d)",
__func__, jobidStr, rc);
}
}
}
@ -1107,7 +1126,29 @@ int xce_core_push_events(xce_sched_core_t* core, xce_core_event_t* events,
}
if (picker != NULL) {
picker->picker_iface->on_job_push(picker, job);
int prc = picker->picker_iface->on_job_push(picker, job);
/* The job IS registered in core->jobs now, but a failed
* on_job_push means it is NOT a picker candidate -- with
* the return value dropped it would sit invisible
* forever (no dispatch, no PEND reason, nothing). The
* known real producer of a rejection is builtin_fcfs's
* "queue not (yet) in the picker's candidate registry"
* (-EINVAL); WARN so the operator can see the orphan
* instead of it vanishing silently. Non-fatal: the next
* event touching this job (e.g. a JOB_UPDATE) re-runs
* the picker's insert path. */
if (prc < 0) {
char jobidStr[64];
fmt_jobid(jobidStr, sizeof(jobidStr), job->jobid);
WARN("%s JOBS_PUSH: picker rejected jobid %s "
"(queue '%s', rc=%d) -- job is tracked but not "
"a scheduling candidate",
__func__, jobidStr,
(job->queue != NULL) ? job->queue->name : "?",
prc);
}
}
}
break;
@ -1323,8 +1364,16 @@ int xce_core_push_events(xce_sched_core_t* core, xce_core_event_t* events,
job->state = new_state;
if (picker != NULL) {
picker->picker_iface->on_job_state_change(picker, job,
new_state);
rc = picker->picker_iface->on_job_state_change(picker, job,
new_state);
if (rc < 0) {
char jobidStr[64];
fmt_jobid(jobidStr, sizeof(jobidStr), jobid);
WARN("%s JOB_STATE_CHANGE: picker rejected jobid %s "
"(rc=%d) -- candidate tracking may be stale",
__func__, jobidStr, rc);
}
}
break;
}
@ -1447,7 +1496,17 @@ int xce_core_push_events(xce_sched_core_t* core, xce_core_event_t* events,
}
if (picker != NULL) {
picker->picker_iface->on_job_update(picker, job);
rc = picker->picker_iface->on_job_update(picker, job);
if (rc < 0) {
char jobidStr[64];
/* bug-1591: see repl_info.c's repl_info_worker_one()
* comment on fmt_jobid(). */
fmt_jobid(jobidStr, sizeof(jobidStr), updated->jobid);
WARN("%s JOB_UPDATE: picker rejected jobid %s (rc=%d) "
"-- candidate tracking may be stale",
__func__, jobidStr, rc);
}
}
break;
}

6
src/scheduler/src/plugins/fcfs.c
View file

@ -363,6 +363,12 @@ static int fcfs_commit(xce_sched_plugin_t* plugin, xce_decision_t* decision)
int register_fcfs(void* sched)
{
/* register_fcfs() keeps the template inline (unlike
* register_builtin_fcfs()'s builtin_fcfs_new_plugin() extraction):
* no core-only caller exists -- sim_picker.c's bootstrap deliberately
* does NOT support this legacy plugin (job_elect_fn-only, reads
* plugin->sched), and this file is itself slated for removal with the
* legacy fcfs zombie (scheduling-audit P2). */
static xce_sched_plugin_t fcfs_plugin = {
.name = "xce_sched_fcfs",
.hooks

17
src/scheduler/src/schedobj.c
View file

@ -478,12 +478,27 @@ int sched_resolve_job_picker(xce_sched_core_t* core, xce_sched_plugin_t** out)
return (rc < 0) ? rc : -ENOENT;
}
/* All nine are MUST hooks per plugin.h's iface contract: the session
* set (next_job/begin_session/end_session) and post-allocation set
* (on_dispatch/on_pend) are dispatched through the resolved pointer
* here, while the four state-change hooks are called unconditionally
* by core_events.c (on_queue_update/on_job_push/on_job_state_change/
* on_job_update), scheduler.c (sched_init_queues' on_queue_update) and
* snapshot_load.c (rebuild walk) -- so a picker that leaves any of
* them NULL must be rejected HERE, at resolution time, not segfault on
* its first event. The optional hooks (on_config/on_job_rusage) are
* deliberately NOT checked -- every call site guards those with
* != NULL. */
if ((plugin == NULL) || (plugin->picker_iface == NULL)
|| (plugin->picker_iface->next_job == NULL)
|| (plugin->picker_iface->begin_session == NULL)
|| (plugin->picker_iface->end_session == NULL)
|| (plugin->picker_iface->on_dispatch == NULL)
|| (plugin->picker_iface->on_pend == NULL)) {
|| (plugin->picker_iface->on_pend == NULL)
|| (plugin->picker_iface->on_queue_update == NULL)
|| (plugin->picker_iface->on_job_push == NULL)
|| (plugin->picker_iface->on_job_state_change == NULL)
|| (plugin->picker_iface->on_job_update == NULL)) {
return -EINVAL;
}

35
src/scheduler/src/sim_picker.c
View file

@ -43,6 +43,25 @@
* replays that same rebuild walk a second time, against whichever picker
* it just registered, mirroring xce_sched_core_load_snapshot()'s own
* queues-before-jobs ordering (snapshot_load.c) exactly.
*
* Picker-name resolution (audit fix): the bootstrap is NAME-DRIVEN across
* the built-in picker set -- "builtin_fcfs" registers the native picker,
* as before. Any other name (WARN now, instead of the old silent return)
* stays unresolvable -- sched_resolve_job_picker() legitimately fails
* downstream and the picker thread degrades gracefully, the same
* documented behavior xce_sched_core_load_snapshot() already establishes
* for an unresolvable picker (sim.h's own doc comment).
*
* The legacy "xce_sched_fcfs" (plugins/fcfs.c, the other builtin picker)
* is deliberately NOT bootstrap-able here, despite being in the builtin
* set: it is a job_elect_fn-only plugin whose elect loop reads
* plugin->sched -- an xce_scheduler_t* the simulation driver never has
* (plugin_register_core() deliberately leaves it NULL, and applies no
* legacy wrapper either, so the rebuild walk below would dereference
* picker_iface == NULL first). Making the legacy plugin sim-viable would
* mean retyping it onto plugin->core -- a refactor of a plugin already
* slated for removal (legacy fcfs zombie), not a bootstrap-name fix. A
* replay recorded under that name degrades with an explicit WARN instead.
*/
/* Consumed by sim_loop.c via a plain `extern` declared there -- same
@ -65,21 +84,25 @@ void sim_bootstrap_picker(xce_sched_sim_driver_t* sim)
* legitimately fails for it downstream, and the picker thread
* degrades gracefully, the same documented behavior
* xce_sched_core_load_snapshot() already establishes for an
* unresolvable picker (sim.h's own doc comment). */
* unresolvable picker (sim.h's own doc comment). This includes
* the legacy "xce_sched_fcfs" (see this file's header comment). */
WARN("%s replay recorded under picker '%s' -- no sim-viable "
"builtin picker of that name to bootstrap; scheduling will "
"be inert",
__func__, sim->core.job_pick_plugin);
return;
}
plugin = builtin_fcfs_new_plugin();
if (plugin == NULL) {
ERROR("%s out of memory building the builtin_fcfs plugin instance",
__func__);
ERROR("%s out of memory building the %s plugin instance", __func__,
sim->core.job_pick_plugin);
return;
}
if (plugin_register_core(&(sim->core), plugin) < 0) {
ERROR("%s failed to register builtin_fcfs against the simulation "
"core",
__func__);
ERROR("%s failed to register %s against the simulation core",
__func__, plugin->name);
free(plugin);
return;
}

48
src/scheduler/testsuite/test_core_events.c
View file

@ -185,6 +185,7 @@ typedef struct {
int job_update_calls;
int queue_update_calls;
int job_rusage_calls;
int job_push_rc; /* value the spy's on_job_push returns */
xce_conf4sched_t* last_config;
xce_sched_job_t* last_job_push;
xce_sched_job_t* last_job_state_change_job;
@ -238,7 +239,7 @@ static int test_on_job_push(void* picker, xce_sched_job_t* job)
(void)picker;
g_counts.job_push_calls++;
g_counts.last_job_push = job;
return 0;
return g_counts.job_push_rc;
}
static int test_on_job_state_change(void* picker, xce_sched_job_t* job,
@ -1960,6 +1961,50 @@ static void test_push_events_zero_nevents_is_a_valid_noop(void)
TEST_ASSERT_EQUAL_INT(0, rc);
}
/* Picker-rejects-push audit fix: when on_job_push() returns <0 (the known
* producer is builtin_fcfs's "queue not in the picker's candidate registry"
* -EINVAL), the job is registered in core->jobs but is NOT a picker
* candidate -- with the return value silently dropped it sat invisible
* forever (no dispatch, no PEND reason, no log). xce_core_push_events() now
* WARNs (the operator-facing signal) but the push itself still succeeds:
* the event batch returns 0 and the job stays tracked -- rejection is the
* picker's prerogative, not a transport failure. */
static void
test_jobs_push_picker_rejection_still_tracks_job_and_succeeds(void)
{
xce_sched_core_t core;
xce_core_event_t event;
xce_job_t* job1;
int rc;
fixture_init(&core, &g_full_plugin);
job1 = make_job(71, 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 = &job1;
/* Simulate builtin_fcfs's "unknown queue" rejection. */
g_counts.job_push_rc = -EINVAL;
rc = xce_core_push_events(&core, &event, 1);
/* The push still succeeds and the hook still fired -- the WARN (not
* directly assertable here) is the only visible change. The hook
* receives the CORE-BUILT object (core_build_job()), not the wire
* payload -- same identity sched_get_job() resolves. */
TEST_ASSERT_EQUAL_INT(0, rc);
TEST_ASSERT_EQUAL_INT(1, g_counts.job_push_calls);
TEST_ASSERT_EQUAL_PTR(sched_get_job(&core, 71), g_counts.last_job_push);
/* The job remains tracked in core->jobs regardless of the rejection. */
TEST_ASSERT_NOT_NULL(sched_get_job(&core, 71));
g_counts.job_push_rc = 0;
}
int main(void)
{
UNITY_BEGIN();
@ -1990,5 +2035,6 @@ int main(void)
RUN_TEST(test_push_events_null_core_returns_einval);
RUN_TEST(test_push_events_null_events_with_positive_nevents_returns_einval);
RUN_TEST(test_push_events_zero_nevents_is_a_valid_noop);
RUN_TEST(test_jobs_push_picker_rejection_still_tracks_job_and_succeeds);
return UNITY_END();
}

70
src/scheduler/testsuite/test_picker_wiring.c
View file

@ -515,6 +515,75 @@ test_legacy_and_native_pickers_coexist_and_resolve_independently(void)
TEST_ASSERT_NULL(resolved->job_elect_fn);
}
/* Picker-resolution audit fix: the four state-change hooks
* (on_queue_update/on_job_push/on_job_state_change/on_job_update) are MUST
* hooks per plugin.h's iface contract -- core_events.c/scheduler.c/
* snapshot_load.c call them UNCONDITIONALLY on any resolved picker, so a
* picker omitting one segfaulted on its first event rather than failing
* startup validation. They must be part of sched_resolve_job_picker()'s
* validation, same as the five original MUST hooks. The iface below wires
* everything EXCEPT those four (incl. the optional on_config/on_job_rusage
* left NULL, as call sites NULL-guard those), so the -EINVAL the test
* asserts can only come from the four NEW checks. */
static int test_wired_noop_int(void* picker)
{
(void)picker;
return 0;
}
static xce_sched_job_t* test_wired_next_job(void* picker)
{
(void)picker;
return NULL;
}
static int test_wired_jobid_noop(void* picker, uint64_t jobid)
{
(void)picker;
(void)jobid;
return 0;
}
static xce_job_picker_iface_t g_session_hooks_only_iface = {
.init = NULL,
.fini = NULL,
.begin_session = test_wired_noop_int,
.next_job = test_wired_next_job,
.end_session = test_wired_noop_int,
.on_dispatch = test_wired_jobid_noop,
.on_pend = test_wired_jobid_noop,
.on_queue_update = NULL,
.on_job_push = NULL,
.on_job_state_change = NULL,
.on_job_update = NULL,
.on_config = NULL,
.on_job_rusage = NULL,
};
static void
test_picker_missing_state_change_hooks_fails_with_einval(void)
{
xce_scheduler_t sched;
xce_sched_plugin_t* partial = NULL;
xce_sched_plugin_t* resolved = NULL;
int rc;
fixture_init(&sched);
/* A picker wiring the five original MUST hooks but leaving all four
* state-change hooks NULL must be rejected at resolution time. */
partial = (xce_sched_plugin_t*)calloc(1, sizeof(*partial));
TEST_ASSERT_NOT_NULL(partial);
partial->name = "partial_state_change_picker";
partial->picker_iface = &g_session_hooks_only_iface;
TEST_ASSERT_GREATER_OR_EQUAL(0, plugin_register(&sched, partial));
sched.core.job_pick_plugin = "partial_state_change_picker";
rc = sched_resolve_job_picker(&sched.core, &resolved);
TEST_ASSERT_EQUAL_INT(-EINVAL, rc);
TEST_ASSERT_NULL(resolved);
}
int main(void)
{
UNITY_BEGIN();
@ -533,5 +602,6 @@ int main(void)
RUN_TEST(test_legacy_and_native_pickers_coexist_and_resolve_independently);
RUN_TEST(
test_legacy_picker_deprecation_flagged_on_selection_not_registration);
RUN_TEST(test_picker_missing_state_change_hooks_fails_with_einval);
return UNITY_END();
}