From a9feded88ec09a3b352ca0461d0ebf3234ea7d36 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Thu, 24 Sep 2026 06:30:34 +0000 Subject: [PATCH] fix(scheduler): mid-round rejoin config push truncated live rounds (bug-1846 part 2) sched_publish_retry_loop() calls sched_check_pending_rejoin() after every successful publish -- inside the driver's decision-drain loop -- so after a client-library reconnect the scheduler re-joins mid-round. The controller answers every join (on_scheduler_join(), controller/sched.c) by re-pushing config, workers and jobs. handler.c applied the config push synchronously on the handler thread: sched_init_config() -> sched_init_queues() -> builtin_fcfs on_queue_update(), which nulls the picker cursor when it reinserts the queue the cursor rests on. next_job() then returned NULL and the round ended with the rest of the queue never considered. Evidence (scheduler-replay-20260923-085356.replay, temporary event-stream diagnostics, reverted): 9 recorded rounds end after 11-12 decisions; every one is followed by a workersPush + full 50-job jobsPush (the join's resync, reporting just-dispatched jobs back as PEND), every full-length dispatching round has none, and the config push never appears because it bypassed the pending-update queue the recorder writes from -- which is also why replay could not reproduce the truncation. Fix (defer to the round edge, same direction as bug-1813): handler.c now calls new sched_stash_config(), parking the Conf4Sched in new xce_scheduler_t.pending_config under pending_config_lock (an older unapplied push is freed). main.c's loop calls new sched_apply_pending_config() before each round, running the unchanged sched_init_config() strictly between sessions. sched_fini() frees a never-applied push. Tests: - test_handlers.c: test_config_push_is_stashed_not_applied_on_handler_thread -- RED (sched_init_config called on the handler thread), GREEN after. - new test_config_push_deferred.c (sched_config_push_deferred_test): real scheduler_req_handler() + real builtin_fcfs mid-session -- RED (next_job() returned NULL right after the push), GREEN (session walks the remaining jobs; configver unchanged until applied once between rounds). - test_admin_request.c / test_job_array_expand.c / test_job_array_wire_e2e.c #include handler.c directly and get a link stub for sched_stash_config(). Full scheduler suite 87/87 (asan). Not addressed: config pushes are still not written to a recording (a replay spanning a config change won't see it); the ~100s reconnect churn itself (see bug-1812/1813). Co-Authored-By: Claude Opus 5.5 (1M context) --- .wolf/buglog.json | 6 +- src/scheduler/include/scheduler.h | 19 ++ src/scheduler/src/handler.c | 14 +- src/scheduler/src/main.c | 7 + src/scheduler/src/scheduler.c | 52 +++++ src/scheduler/testsuite/meson.build | 24 ++ src/scheduler/testsuite/test_admin_request.c | 10 + .../testsuite/test_config_push_deferred.c | 208 ++++++++++++++++++ src/scheduler/testsuite/test_handlers.c | 60 +++++ .../testsuite/test_job_array_expand.c | 10 + .../testsuite/test_job_array_wire_e2e.c | 10 + 11 files changed, 413 insertions(+), 7 deletions(-) create mode 100644 src/scheduler/testsuite/test_config_push_deferred.c diff --git a/.wolf/buglog.json b/.wolf/buglog.json index 9408353c..f8539126 100644 --- a/.wolf/buglog.json +++ b/.wolf/buglog.json @@ -25082,9 +25082,9 @@ "id": "bug-1846", "timestamp": "2026-09-23T10:05:00Z", "error_message": "OPEN, NOT YET ROOT-CAUSED. Same replay session as bug-1844/1845 (scheduler-replay-20260923-085356.replay): after excluding instid (bug-1844) and reason-detail (bug-1845), --sim-verify still shows one job (jobid=0-8-1790154325-0[39] in the observed capture) genuinely DISPATCHING one round earlier in replay than in the original live recording -- not a false-positive field comparison, a real positional divergence: replay's round begin_round=...436582151823 dispatches a 13-job batch (jobs [27]..[39]) while the SAME round in the original recording only dispatched 12 (jobs [27]..[38]); job [39]'s recorded dispatch instead lands in the NEXT round, cascading into ~40+ downstream positional MISMATCH lines for that round pair alone.", - "file": "src/scheduler/src/plugins/limit.c (match_job_host_limit_)", - "root_cause": "CONFIRMED (2026-09-23, via temporary per-decision diagnostics in the --sim-verify drain loop, since reverted) -- two distinct things. (1) ROOT-CAUSED + FIXED: match_job_host_limit_() (limit.c) runs on EVERY per-host allocation attempt (limit_prepare_allocation() via plugin_prepare_allocation()) and unconditionally xlist_store()d the matched vres into obj->hlimits[worker->idx]; obj->hlimits is only rebuilt by limit_job_election() on a configver bump. A job that kept pending on the same host accumulated one extra copy per attempt; sched_fill_limit_requests() staged every copy into ctx->alloc_req.host and seqjob's make_host_alloc() built a host alloc carrying all of them. In the captured replay the LIVE recording itself shows it (pc13v host allocs with mem+slots+70/79/94/112/136/175 identical limit=limits2/u=rshen/q=xce/p= entries, resetting only on a config push) -- i.e. a PRODUCTION bug, not sim-only. make_job_account() makes one res_account per entry and sched_update_res_accounts() charges each, so the host limit was charged (and later released) N-fold. Replay had the same accumulation with different counts, which is what --sim-verify flagged. (2) NOT ROOT-CAUSED, live-side: 9 recorded rounds (all containing DISPATCHes) end after only 11-12 decisions while replay processes the whole candidate list; the next recorded round then carries the leftovers. Ruled out: recorder growth/truncation (recording_session.c), a picker/round time budget (none exists; truncated rounds last 0.8-2.2s), fcfs readiness predicates (both return 1), the unsatisfiable-cache reason handoff (MOVE_PTR on both paths), queued controller updates (all dataq-enqueued). Remaining candidates: something live-only ending the fcfs session early (e.g. handler.c's synchronous config push -> sched_init_queues() -> on_queue_update() nulling priv->cursor mid-session, too rare to explain all 9), or sched_allocator's silent `alloc_rc < 0 -> continue` (no decision emitted) -- possibly related to (1)'s inflated limit charge. Needs the live scheduler.log for the capture window and/or a fresh capture from a fixed binary.", - "fix": "PARTIAL (2026-09-23, TDD). limit.c: new hlimits_contains_() pointer-identity check (vres is interned per name in privdata->instances) -- match_job_host_limit_() now stores a matched vres into obj->hlimits[worker] only once. New regression test test_limit_host_dedup.c (sched_limit_host_dedup_test, meson suite 'scheduler'): 5 plugin_prepare_allocation() attempts for one job on one worker must leave hlimits[worker]->count==1 and exactly one staged copy of the vres in alloc_req.host -- verified RED before the fix (Expected 1 Was 2 on attempt 2) and GREEN after. Replay re-run after the fix: replay-side host allocs are now always 3 entries; remaining DISPATCH alloc mismatches are the recording's own inflated entries (recorded with the buggy binary) and will disappear only with a fresh capture. Part (2) remains OPEN.", + "file": "src/scheduler/src/plugins/limit.c (match_job_host_limit_); src/scheduler/src/handler.c + main.c + scheduler.c (config push deferral)", + "root_cause": "CONFIRMED (2026-09-23, via temporary per-decision diagnostics in the --sim-verify drain loop, since reverted) -- two distinct things. (1) ROOT-CAUSED + FIXED: match_job_host_limit_() (limit.c) runs on EVERY per-host allocation attempt (limit_prepare_allocation() via plugin_prepare_allocation()) and unconditionally xlist_store()d the matched vres into obj->hlimits[worker->idx]; obj->hlimits is only rebuilt by limit_job_election() on a configver bump. A job that kept pending on the same host accumulated one extra copy per attempt; sched_fill_limit_requests() staged every copy into ctx->alloc_req.host and seqjob's make_host_alloc() built a host alloc carrying all of them. In the captured replay the LIVE recording itself shows it (pc13v host allocs with mem+slots+70/79/94/112/136/175 identical limit=limits2/u=rshen/q=xce/p= entries, resetting only on a config push) -- i.e. a PRODUCTION bug, not sim-only. make_job_account() makes one res_account per entry and sched_update_res_accounts() charges each, so the host limit was charged (and later released) N-fold. Replay had the same accumulation with different counts, which is what --sim-verify flagged. (2) ROOT-CAUSED (2026-09-23): the 9 live rounds cut off after 11-12 decisions were each ended by a mid-round scheduler REJOIN. sched_publish_retry_loop() (scheduler.c) calls sched_check_pending_rejoin() after every successful publish -- i.e. inside the driver's decision-drain loop -- and after a client-library reconnect that issues xcecli_join(). The controller answers every join (on_scheduler_join(), controller/sched.c) with xcesched_push_configs() + xcesched_push_workers() + xcesched_push_jobs(). handler.c applied the config push synchronously on the handler thread (sched_init_config() -> sched_init_queues() -> builtin_fcfs on_queue_update()), which nulls priv->cursor when it reinserts the queue the cursor rests on; next_job() then returns NULL and the round ends. Evidence from the capture (temporary event-stream diagnostics, reverted): every truncated round is followed by a workersPush + full 50-job jobsPush (ntotal=50, pos=0, the join's resync -- e.g. jobs just dispatched reported back as PEND), every full-length dispatching round has none, and the config push itself never appears because it bypassed the pending-update queue the recorder writes from. Rejoins came roughly every ~100s (the configver bump also explains the periodic reset of part (1)'s duplicate counts). The underlying connection churn is a separate issue (see bug-1812/1813's pc13v reconnect/resync storm).", + "fix": "PARTIAL (2026-09-23, TDD). limit.c: new hlimits_contains_() pointer-identity check (vres is interned per name in privdata->instances) -- match_job_host_limit_() now stores a matched vres into obj->hlimits[worker] only once. New regression test test_limit_host_dedup.c (sched_limit_host_dedup_test, meson suite 'scheduler'): 5 plugin_prepare_allocation() attempts for one job on one worker must leave hlimits[worker]->count==1 and exactly one staged copy of the vres in alloc_req.host -- verified RED before the fix (Expected 1 Was 2 on attempt 2) and GREEN after. Replay re-run after the fix: replay-side host allocs are now always 3 entries; remaining DISPATCH alloc mismatches are the recording's own inflated entries (recorded with the buggy binary) and will disappear only with a fresh capture. PART (2) FIXED (2026-09-23, TDD, user-chosen design 'defer to round edge'): handler.c's configpushmsg case now calls new sched_stash_config() (parks the Conf4Sched in new xce_scheduler_t.pending_config under pending_config_lock, freeing any older unapplied push) instead of sched_init_config(); main.c's loop calls new sched_apply_pending_config() before each xce_sched_driver_run_normal_mode_round(), which runs the unchanged sched_init_config() strictly between sessions; sched_fini() frees a never-applied push. Same deferral direction as bug-1813's array-job fix. Tests: test_handlers.c's test_config_push_is_stashed_not_applied_on_handler_thread (RED: sched_init_config called 1x on the handler thread; GREEN after) and new test_config_push_deferred.c (sched_config_push_deferred_test: real scheduler_req_handler() + real builtin_fcfs mid-session -- RED: next_job() returned NULL right after the config push, job B never considered; GREEN: session walks B and C, configver unchanged until sched_apply_pending_config() applies it once). Known remaining gap (pre-existing, not addressed): a config push is still not written to a recording, so a replay spanning a config CHANGE won't see it.", "tags": [ "scheduler", "sim", diff --git a/src/scheduler/include/scheduler.h b/src/scheduler/include/scheduler.h index 3eb38060..2f6194f6 100644 --- a/src/scheduler/include/scheduler.h +++ b/src/scheduler/include/scheduler.h @@ -291,12 +291,31 @@ typedef struct { * sched_init(), destroyed by sched_fini() (scheduler.c) -- same * lifecycle pairing as `core.lock` (core.c). */ mylock_t record_lock; + /* bug-1846: a config push received on the handler thread (handler.c's + * scheduler_req_handler()), parked here until the driver loop applies + * it between rounds via sched_apply_pending_config(). The controller + * re-pushes config on every scheduler join, and the scheduler re-joins + * from inside a round's decision publish (sched_check_pending_rejoin(), + * via sched_publish_retry_loop()) -- applying it on arrival reset the + * picker's cursor mid-session and ended the round early. Only the + * newest push is kept (each is a complete Conf4Sched). Owned by this + * struct until applied; freed by sched_fini() if never applied. + * Protected by `pending_config_lock`. */ + xce_conf4sched_t* pending_config; + mylock_t pending_config_lock; } xce_scheduler_t; extern int sched_init(xce_scheduler_t*, xce_client_config_t*, const xce_cluster_config_t*, xce_request_handler_t); extern void sched_fini(xce_scheduler_t*); extern int sched_init_config(xce_scheduler_t* sched, xce_conf4sched_t*); +/* bug-1846: see xce_scheduler_t.pending_config. sched_stash_config() is + * the handler-thread half (takes ownership of `config`, replacing -- and + * freeing -- any older unapplied push); sched_apply_pending_config() is the + * driver-thread half, run between rounds, and returns 1 if a config was + * applied, 0 if none was pending, or sched_init_config()'s negative rc. */ +extern int sched_stash_config(xce_scheduler_t* sched, xce_conf4sched_t*); +extern int sched_apply_pending_config(xce_scheduler_t* sched); /* Req 2 AC 1 (scheduler-core spec, task 1.3): applies the "builtin_fcfs" * default to sched->job_pick_plugin when unset. See the field comment on * xce_scheduler_t above -- this does NOT resolve/register the name. */ diff --git a/src/scheduler/src/handler.c b/src/scheduler/src/handler.c index 80ceb665..f9e1a14f 100644 --- a/src/scheduler/src/handler.c +++ b/src/scheduler/src/handler.c @@ -535,10 +535,16 @@ int scheduler_req_handler(xce_request_t* request, void* args) case OPCode_push: switch (request->body_which) { case Request_body_configpushmsg: - /* configuration packet, it will only arrive when scheduler has - * register itself to controller, we can handle it synchronized - */ - rc = sched_init_config( + /* configuration packet, sent by the controller on every + * scheduler join (on_scheduler_join(), controller/sched.c). + * bug-1846: NOT applied here -- a rejoin happens from inside a + * round's decision publish (sched_check_pending_rejoin(), via + * sched_publish_retry_loop()), so this can land mid-session, + * and sched_init_config() -> sched_init_queues() -> picker + * on_queue_update() reset the fcfs cursor and ended the round + * early. Stash it; main.c's loop applies it between rounds via + * sched_apply_pending_config(). */ + rc = sched_stash_config( sched, request->body.configpushmsg->data.conf4sched); if (rc >= 0) { /* configuration will be saved in sched->config */ diff --git a/src/scheduler/src/main.c b/src/scheduler/src/main.c index 821e96dc..49e8db9b 100644 --- a/src/scheduler/src/main.c +++ b/src/scheduler/src/main.c @@ -498,6 +498,13 @@ int main(int argc, char* argv[]) INFO("xce scheduler is running"); while (1) { + /* bug-1846: apply a config push the handler thread stashed (see + * handler.c's configpushmsg case) strictly between rounds, never + * while the picker/allocator threads are mid-session. Like + * sched_check_pending_rejoin() below, this is a scheduler.c symbol, + * so it lives here rather than in driver.c. */ + sched_apply_pending_config(&sched); + xce_sched_driver_run_normal_mode_round(&sched); if (sched.core.stop) { diff --git a/src/scheduler/src/scheduler.c b/src/scheduler/src/scheduler.c index 3440646e..d6b8ff1d 100644 --- a/src/scheduler/src/scheduler.c +++ b/src/scheduler/src/scheduler.c @@ -613,6 +613,7 @@ int sched_init(xce_scheduler_t* sched, xce_client_config_t* config, * later by lib/xcectx.c once this scheduler registers its handler) can * possibly run. */ mylock_init(&(sched->record_lock)); + mylock_init(&(sched->pending_config_lock)); if (cluster_cfg != NULL) { sched->cluster_cfg = *cluster_cfg; @@ -830,6 +831,11 @@ void sched_fini(xce_scheduler_t* sched) * mylock_init(&(sched->record_lock)) above. */ mylock_fini(&(sched->record_lock)); + /* bug-1846: a config push that arrived after the last round was never + * applied -- still owned here. */ + free_Conf4Sched_ptr(&(sched->pending_config)); + mylock_fini(&(sched->pending_config_lock)); + return; } @@ -963,6 +969,52 @@ int sched_init_config(xce_scheduler_t* sched, xce_conf4sched_t* config) return 0; } +int sched_stash_config(xce_scheduler_t* sched, xce_conf4sched_t* config) +{ + xce_conf4sched_t* older; + + if ((sched == NULL) || (config == NULL)) { + return -EINVAL; + } + + mylock_lock(&(sched->pending_config_lock)); + older = sched->pending_config; + sched->pending_config = config; + mylock_unlock(&(sched->pending_config_lock)); + + free_Conf4Sched_ptr(&older); + + return 0; +} + +int sched_apply_pending_config(xce_scheduler_t* sched) +{ + xce_conf4sched_t* config; + int rc; + + if (sched == NULL) { + return -EINVAL; + } + + mylock_lock(&(sched->pending_config_lock)); + config = sched->pending_config; + sched->pending_config = NULL; + mylock_unlock(&(sched->pending_config_lock)); + + if (config == NULL) { + return 0; + } + + rc = sched_init_config(sched, config); + if (rc < 0) { + ERROR("%s sched_init_config failed rc=%d", __func__, rc); + free_Conf4Sched_ptr(&config); + return rc; + } + + return 1; +} + static int sched_update_jobs(xce_scheduler_t* sched, int njobs, xce_job_t** jobs) { diff --git a/src/scheduler/testsuite/meson.build b/src/scheduler/testsuite/meson.build index f4349e75..5f2b174d 100644 --- a/src/scheduler/testsuite/meson.build +++ b/src/scheduler/testsuite/meson.build @@ -2231,6 +2231,30 @@ test('scheduler limit host dedup test', sched_limit_host_dedup_test, suite: 'scheduler', env: ['ASAN_OPTIONS=detect_leaks=0']) +# bug-1846 part 2: a config push (sent by the controller on every scheduler +# join, including a rejoin triggered mid-round from the decision publish +# path) must be deferred to the round boundary -- applying it on the handler +# thread reset the fcfs cursor and ended the live round early. Real +# scheduler_req_handler() + real builtin_fcfs; every scheduler source except +# main.c. +sched_config_push_deferred_test_srcs = ['test_config_push_deferred.c'] +foreach f : scheduler_srcs + if f != 'src/main.c' + sched_config_push_deferred_test_srcs += ['../' + f] + endif +endforeach + +sched_config_push_deferred_test = executable( + 'sched_config_push_deferred_test', + sched_config_push_deferred_test_srcs, + include_directories:[incdir], + dependencies: [scheduler_test_deps], + install: false) + +test('scheduler config push deferred test', + sched_config_push_deferred_test, suite: 'scheduler', + env: ['ASAN_OPTIONS=detect_leaks=0']) + # scheduler-sim-live-allocation spec, task 5.2 (Req 2.1, 2.2, 2.3, 2.4): the # feature's concrete, original acceptance test (bug-1826) -- a real global # AND host-scoped limit configured in a real SnapshotEntry's Conf4Sched, diff --git a/src/scheduler/testsuite/test_admin_request.c b/src/scheduler/testsuite/test_admin_request.c index beb41108..5e9d6074 100644 --- a/src/scheduler/testsuite/test_admin_request.c +++ b/src/scheduler/testsuite/test_admin_request.c @@ -298,6 +298,16 @@ int sched_init_config(xce_scheduler_t* s, xce_conf4sched_t* c) return 0; } +/* bug-1846: handler.c's config-push path now stashes via sched_stash_config() + * (scheduler.c, not linked here) instead of calling sched_init_config() -- + * link-satisfying stub, same rationale as the one above. */ +int sched_stash_config(xce_scheduler_t* s, xce_conf4sched_t* c) +{ + (void)s; + (void)c; + return 0; +} + /* Pull in the real sched_handle_admin_request()/scheduler_req_handler() * under test into this TU. */ #include "../src/handler.c" diff --git a/src/scheduler/testsuite/test_config_push_deferred.c b/src/scheduler/testsuite/test_config_push_deferred.c new file mode 100644 index 00000000..026c302b --- /dev/null +++ b/src/scheduler/testsuite/test_config_push_deferred.c @@ -0,0 +1,208 @@ +/* + * test_config_push_deferred.c - bug-1846 part 2 regression: a config push + * that arrives while a scheduling session is in progress must not end that + * session early. + * + * The controller answers every scheduler join (on_scheduler_join(), + * controller/sched.c) by re-pushing config, workers and jobs. The scheduler + * re-joins from sched_check_pending_rejoin(), which sched_publish_retry_loop() + * calls after a successful publish -- i.e. from inside the driver's decision + * drain loop, mid-round. handler.c used to apply the config push right there + * on the handler thread: sched_init_config() -> sched_init_queues() -> + * builtin_fcfs on_queue_update(), which nulls the picker's cursor when it + * re-inserts the queue the cursor is resting on. next_job() then reports + * "no more candidates" and the round ends with the rest of the queue never + * considered. Captured in scheduler-replay-20260923-085356.replay as 9 + * rounds cut off after 11-12 decisions, each followed by the rejoin's + * workersPush + full 50-job jobsPush (the config push itself was never + * recorded, since it bypassed the pending-update queue). + * + * This fixture puts the real fcfs picker mid-session, delivers a config + * push through the real scheduler_req_handler(), and requires the session + * to keep walking the queue; the config must then take effect when the + * driver applies it between rounds. + */ +#include +#include +#include +#include + +#include + +#include "controller.capnp.h" +#include "plugin.h" +#include "resobj.h" +#include "sched/types.h" +#include "scheduler.h" +#include "xstring.h" + +/* handler.c's request entry point -- no header declares it (main.c passes + * it to sched_init() by name); "extern declared where consumed", the same + * idiom test_sim_contention_limit_driver_parity.c uses. */ +extern int scheduler_req_handler(xce_request_t* request, void* args); + +#define QUEUE_NAME "normal" +#define TEST_RESREQ ((char*)"select[type==any] rusage[mem=1]") + +#define JOBA_ID ((uint64_t)518471ULL) +#define JOBB_ID ((uint64_t)518472ULL) +#define JOBC_ID ((uint64_t)518473ULL) + +void setUp(void) { return; } +void tearDown(void) { return; } + +static void register_slots_resource(var resmgr) +{ + xce_resource_t res; + + memset(&res, 0, sizeof(res)); + res.name = strdup("slots"); + res.valtype = XCE_VAL_NUMERIC; + res.ordtype = XCE_ORD_DECR; + res.flags = XCE_RESF_LIMIT | XCE_RESF_CONSUMABLE | XCE_RESF_RELEASE + | XCE_RESF_DEFAULT_HOST; + + TEST_ASSERT_NOT_NULL(makeResource(resmgr, &res)); +} + +static void push_job(xce_scheduler_t* sched, xce_sched_plugin_t* picker, + uint64_t jobid, uint64_t queue_time) +{ + job_submit_data_t submit; + xce_job_t j; + xce_sched_job_t* job; + int rc; + + memset(&submit, 0, sizeof(submit)); + submit.queue = (char*)QUEUE_NAME; + submit.user = (char*)"cfgpush_user"; + submit.project_name = (char*)"default"; + submit.resreq = TEST_RESREQ; + submit.mintasks = 1; + submit.maxtasks = 1; + + memset(&j, 0, sizeof(j)); + j.jobid = jobid; + j.state = JOB_STAT_PEND; + j.sub_time = queue_time; + j.queue_time = queue_time; + j.submit = &submit; + + job = make_sched_job(sched, &j); + TEST_ASSERT_NOT_NULL(job); + + rc = sched_add_job(&(sched->core), job); + TEST_ASSERT_TRUE(rc >= 0); + picker->picker_iface->on_job_push(picker, job); +} + +/* The same shape on_scheduler_join()'s xcesched_push_configs() sends: a + * Conf4Sched naming the queue the picker is already walking. */ +static void deliver_config_push(xce_scheduler_t* sched) +{ + xce_queue_t* q = (xce_queue_t*)calloc(1, sizeof(*q)); + xce_queue_t** queues = (xce_queue_t**)calloc(1, sizeof(*queues)); + xce_conf4sched_t* config = (xce_conf4sched_t*)calloc(1, sizeof(*config)); + xce_configdata_t configdata; + xce_request_t request; + int rc; + + TEST_ASSERT_NOT_NULL(q); + TEST_ASSERT_NOT_NULL(queues); + TEST_ASSERT_NOT_NULL(config); + + q->queue = strdup(QUEUE_NAME); + queues[0] = q; + config->n_queues = 1; + config->queues = queues; + + memset(&configdata, 0, sizeof(configdata)); + configdata.data.conf4sched = config; + + memset(&request, 0, sizeof(request)); + request.opcode = OPCode_push; + request.body_which = Request_body_configpushmsg; + request.body.configpushmsg = &configdata; + + rc = scheduler_req_handler(&request, sched); + TEST_ASSERT_TRUE(rc >= 0); +} + +static xce_scheduler_t g_sched; + +static void test_config_push_mid_session_does_not_end_round(void) +{ + xce_sched_plugin_t* fcfs = NULL; + xce_sched_queue_t* queue; + xce_sched_job_t* job; + int configver_before; + int rc; + + memset(&g_sched, 0, sizeof(g_sched)); + rc = xce_sched_core_init(&(g_sched.core)); + TEST_ASSERT_EQUAL_INT(0, rc); + + register_slots_resource(g_sched.core.resmgr); + + fcfs = builtin_fcfs_new_plugin(); + TEST_ASSERT_NOT_NULL(fcfs); + rc = plugin_register(&g_sched, fcfs); + TEST_ASSERT_TRUE(rc >= 0); + g_sched.core.job_pick_plugin = (char*)"builtin_fcfs"; + + queue = (xce_sched_queue_t*)calloc(1, sizeof(*queue)); + TEST_ASSERT_NOT_NULL(queue); + queue->name = makeString(QUEUE_NAME); + rc = sched_add_queue(&(g_sched.core), queue); + TEST_ASSERT_TRUE(rc >= 0); + fcfs->picker_iface->on_queue_update(fcfs, queue); + + push_job(&g_sched, fcfs, JOBA_ID, 1000ULL); + push_job(&g_sched, fcfs, JOBB_ID, 1001ULL); + push_job(&g_sched, fcfs, JOBC_ID, 1002ULL); + + /* Mid-session: the picker has handed out job A and rests on job B. */ + rc = fcfs->picker_iface->begin_session(fcfs); + TEST_ASSERT_EQUAL_INT(0, rc); + job = fcfs->picker_iface->next_job(fcfs); + TEST_ASSERT_NOT_NULL(job); + TEST_ASSERT_EQUAL_UINT64(JOBA_ID, job->jobid); + + configver_before = g_sched.core.configver; + + /* A rejoin's config push lands now, on the handler thread. */ + deliver_config_push(&g_sched); + + job = fcfs->picker_iface->next_job(fcfs); + TEST_ASSERT_NOT_NULL_MESSAGE( + job, "a config push delivered mid-session must not end the round -- " + "job B was never considered"); + TEST_ASSERT_EQUAL_UINT64(JOBB_ID, job->jobid); + job = fcfs->picker_iface->next_job(fcfs); + TEST_ASSERT_NOT_NULL(job); + TEST_ASSERT_EQUAL_UINT64(JOBC_ID, job->jobid); + TEST_ASSERT_NULL(fcfs->picker_iface->next_job(fcfs)); + + TEST_ASSERT_EQUAL_INT_MESSAGE( + configver_before, g_sched.core.configver, + "the pushed config must not be applied while the session runs"); + + fcfs->picker_iface->end_session(fcfs); + + /* Between rounds the driver applies it -- exactly once. */ + TEST_ASSERT_EQUAL_INT(1, sched_apply_pending_config(&g_sched)); + TEST_ASSERT_EQUAL_INT_MESSAGE(configver_before + 1, g_sched.core.configver, + "the stashed config must take effect when " + "applied between rounds"); + TEST_ASSERT_EQUAL_INT(0, sched_apply_pending_config(&g_sched)); + TEST_ASSERT_EQUAL_INT(configver_before + 1, g_sched.core.configver); + + printf("test_config_push_mid_session_does_not_end_round: PASS\n"); +} + +int main(void) +{ + UNITY_BEGIN(); + RUN_TEST(test_config_push_mid_session_does_not_end_round); + return UNITY_END(); +} diff --git a/src/scheduler/testsuite/test_handlers.c b/src/scheduler/testsuite/test_handlers.c index 403588f7..bf52a469 100644 --- a/src/scheduler/testsuite/test_handlers.c +++ b/src/scheduler/testsuite/test_handlers.c @@ -25,10 +25,27 @@ * and sched_get_job (job-array-single-wal-record task 5.3's per-element * dedup check, XceData_data_jobArray — not tested here, see * test_job_array_expand.c for the real kvmgr-backed sched_get_job). */ +static int g_init_config_calls = 0; +static int g_stash_config_calls = 0; +static xce_conf4sched_t* g_stashed_config = NULL; + int sched_init_config(xce_scheduler_t* s, xce_conf4sched_t* c) { (void)s; (void)c; + g_init_config_calls++; + return 0; +} + +/* bug-1846: the config-push handler must hand the pushed config to the + * driver (applied between rounds) rather than calling sched_init_config() + * on the handler thread -- counted here so the test below can tell which + * path the real handler took. */ +int sched_stash_config(xce_scheduler_t* s, xce_conf4sched_t* c) +{ + (void)s; + g_stash_config_calls++; + g_stashed_config = c; return 0; } @@ -307,6 +324,48 @@ static void test_handle_notify_job_exit_maps_exit_code_to_done_or_exit(void) TEST_ASSERT_EQUAL_INT(0, pop_op(&sched, NULL)); } +/* bug-1846: a config push arrives on the handler thread whenever the + * scheduler (re)joins -- including a rejoin triggered from inside a live + * round's decision publish. Applying it there (sched_init_config() -> + * sched_init_queues() -> picker on_queue_update()) reset the fcfs cursor + * mid-session and ended the round early. The handler must only stash it + * and take ownership (payload pointer nulled), never apply it. */ +static void test_config_push_is_stashed_not_applied_on_handler_thread(void) +{ + xce_scheduler_t sched; + xce_request_t request; + xce_configdata_t configdata; + xce_conf4sched_t* config = (xce_conf4sched_t*)calloc(1, sizeof(*config)); + int rc; + + fixture_init(&sched); + g_init_config_calls = 0; + g_stash_config_calls = 0; + g_stashed_config = NULL; + + memset(&configdata, 0, sizeof(configdata)); + configdata.data.conf4sched = config; + + memset(&request, 0, sizeof(request)); + request.opcode = OPCode_push; + request.body_which = Request_body_configpushmsg; + request.body.configpushmsg = &configdata; + + rc = scheduler_req_handler(&request, &sched); + TEST_ASSERT_TRUE(rc >= 0); + + TEST_ASSERT_EQUAL_INT_MESSAGE( + 0, g_init_config_calls, + "config push must not be applied synchronously on the handler " + "thread -- it can land mid-round and truncate the session"); + TEST_ASSERT_EQUAL_INT(1, g_stash_config_calls); + TEST_ASSERT_EQUAL_PTR(config, g_stashed_config); + TEST_ASSERT_NULL_MESSAGE(configdata.data.conf4sched, + "handler must take ownership of the stashed " + "config so the request teardown won't free it"); + free(config); +} + int main(int argc, char* argv[]) { (void)argc; @@ -320,5 +379,6 @@ int main(int argc, char* argv[]) 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); + RUN_TEST(test_config_push_is_stashed_not_applied_on_handler_thread, 0); return UNITY_END(); } diff --git a/src/scheduler/testsuite/test_job_array_expand.c b/src/scheduler/testsuite/test_job_array_expand.c index 6a9e60be..3062c7c9 100644 --- a/src/scheduler/testsuite/test_job_array_expand.c +++ b/src/scheduler/testsuite/test_job_array_expand.c @@ -44,6 +44,16 @@ int sched_init_config(xce_scheduler_t* s, xce_conf4sched_t* c) return 0; } +/* bug-1846: handler.c's config-push path now stashes via sched_stash_config() + * (scheduler.c, not linked here) instead of calling sched_init_config() -- + * link-satisfying stub, same rationale as the one above. */ +int sched_stash_config(xce_scheduler_t* s, xce_conf4sched_t* c) +{ + (void)s; + (void)c; + return 0; +} + /* sched_record_status (recording.c, scheduler-core spec, task 9.4) -- * handler.c's OPCode_schedAdmin/recordStatus path references it; this file * never exercises OPCode_schedAdmin, so a link-satisfying stub is enough, diff --git a/src/scheduler/testsuite/test_job_array_wire_e2e.c b/src/scheduler/testsuite/test_job_array_wire_e2e.c index 4a5316e7..98d76dbb 100644 --- a/src/scheduler/testsuite/test_job_array_wire_e2e.c +++ b/src/scheduler/testsuite/test_job_array_wire_e2e.c @@ -87,6 +87,16 @@ int sched_init_config(xce_scheduler_t* s, xce_conf4sched_t* c) return 0; } +/* bug-1846: handler.c's config-push path now stashes via sched_stash_config() + * (scheduler.c, not linked here) instead of calling sched_init_config() -- + * link-satisfying stub, same rationale as the one above. */ +int sched_stash_config(xce_scheduler_t* s, xce_conf4sched_t* c) +{ + (void)s; + (void)c; + return 0; +} + /* sched_record_status (recording.c, scheduler-core spec, task 9.4) -- * handler.c's OPCode_schedAdmin/recordStatus path references it; this file * never exercises OPCode_schedAdmin, so a link-satisfying stub is enough,