From 915782fc66dc737809b3100ec25bc04903868377 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Sat, 19 Sep 2026 05:57:55 +0000 Subject: [PATCH] feat(scheduler-sim-live-allocation): narrow reconciliation to the timing-gap safety net (task 4.2) Adds a plugin_job_election(&(sim->core), job) call to sim_reconcile_unaccounted_jobs()'s FOUND branch, before the existing alloc-reconstruction/state-transition code, so a job whose outcome comes from the recording still gets its global-limit usage registered -- the same core-safe dispatcher the live path (task 4.1) already uses, not a separate mechanism. The existing job->accounts != NULL trigger guard needs no change: it already excludes any job the live path has already decided, so this call is naturally idempotent (match_job_global_limit_'s own obj->configver guard makes a redundant call a safe no-op) -- narrowing this function to a genuine timing-gap safety net rather than the general-case decision source it used to be, per design's Live/Recorded Authority Rule. Host-scoped limit matching is deliberately not attempted here (needs a resolved worker context this function doesn't independently construct) -- that gap is tracked separately as task 4.3. TDD: new tests prove a reconciled job's global limit is genuinely registered via the real limit_get_usage_instances() accessor, and that a job the live path already decided is never reprocessed. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01RLSNJhNoULvVcUhXEFafQC --- .../scheduler-sim-live-allocation/tasks.md | 2 +- src/scheduler/src/sim_loop.c | 24 ++ .../test_sim_reconcile_unaccounted.c | 348 ++++++++++++++---- .../testsuite/test_sim_session_ordering.c | 18 + 4 files changed, 329 insertions(+), 63 deletions(-) diff --git a/.kiro/specs/scheduler-sim-live-allocation/tasks.md b/.kiro/specs/scheduler-sim-live-allocation/tasks.md index cff527c8..9d5368bb 100644 --- a/.kiro/specs/scheduler-sim-live-allocation/tasks.md +++ b/.kiro/specs/scheduler-sim-live-allocation/tasks.md @@ -76,7 +76,7 @@ - _Requirements: 1.1, 1.2, 3.3, 4.2, 4.3, 5.1_ - _Boundary: Unified Picker/Allocator Threads, Sim Driver_ -- [ ] 4.2 Narrow recorded-decision reconciliation to the timing-gap safety net +- [x] 4.2 Narrow recorded-decision reconciliation to the timing-gap safety net - Add a call to the now-driver-agnostic job-election dispatch (from 1.2) for a job this mechanism reconstructs from the recording, so a job whose outcome comes from the recording still gets its global-limit usage registered - Confirm the existing trigger guard (a job already decided is never reconsidered) needs no change, since it already excludes any job the live path from 4.1 has already handled - TDD: extend the existing reconciliation test suite with a case confirming a reconciled job's global limit is registered, and a case confirming a job the live path already decided is never reprocessed by this mechanism diff --git a/src/scheduler/src/sim_loop.c b/src/scheduler/src/sim_loop.c index 9239e111..e41aa152 100644 --- a/src/scheduler/src/sim_loop.c +++ b/src/scheduler/src/sim_loop.c @@ -732,6 +732,30 @@ static void sim_reconcile_unaccounted_jobs(xce_sched_sim_driver_t* sim, continue; } + /* FOUND -- task 4.2 (scheduler-sim-live-allocation spec, + * "Sim-Mode Reconciliation Narrowing" component): elect the job + * through the same core-safe plugin_job_election() dispatch + * (task 1.2) sched_allocator() calls for a job the live path + * decides directly (sched_election.c), so a job whose outcome + * this mechanism reconstructs from the recording still gets its + * GLOBAL (non-host-scoped) limit usage registered -- + * match_job_global_limit_()'s own idempotency guard + * (`obj->configver >= core.configver`, plugins/limit.c) makes + * this safe to call even if the live path also happens to reach + * this same job (it cannot: this branch's own trigger, + * `job->accounts == NULL`, already excludes any job the live + * path has decided -- see this function's own header comment). + * Deliberately GLOBAL-limit-only, not host-scoped: host-scoped + * matching (match_job_host_limit_(), via prepare_allocation_ + * hook) needs a resolved worker/ctx this reconstruction path + * does not independently build -- design.md's own text for this + * component explains why that is a structurally different + * problem, not one this call can also close. Called before the + * allocation reconstruction below (mirrors sched_allocator()'s + * own ordering: election, then allocation), though nothing + * downstream of it reads job->allocs/accounts. */ + plugin_job_election(&(sim->core), job); + /* FOUND -- task 2.3: reconstruct job->allocs/job->accounts by * TAKING OVER `entry`'s allocation, the same ownership-transfer * pattern scheduler.c's sched_commit_decision() already uses diff --git a/src/scheduler/testsuite/test_sim_reconcile_unaccounted.c b/src/scheduler/testsuite/test_sim_reconcile_unaccounted.c index f762bc78..d0b9705d 100644 --- a/src/scheduler/testsuite/test_sim_reconcile_unaccounted.c +++ b/src/scheduler/testsuite/test_sim_reconcile_unaccounted.c @@ -114,6 +114,19 @@ #include "../src/decision.c" #include "../src/plugin.c" #include "../src/plugins/builtin_fcfs.c" +#include "../src/plugins/limit.c" /* task 4.2 (scheduler-sim-live-allocation + * spec): pulled in for REAL (same file test_repl_limit_table.c's own + * fixtures already white-box #include) so this file's reconciliation tests + * can prove sim_reconcile_unaccounted_jobs()'s new plugin_job_election() + * call (sim_loop.c) genuinely registers a reconciled job's GLOBAL limit + * usage -- a stub returning NULL from limit_new_plugin() (this file's own + * previous stand-in, removed below) would defeat that test's entire + * premise. This supersedes the four link-satisfying stubs immediately + * below this include block in earlier revisions of this file + * (limit_get_configured_limits()/limit_get_usage_instances()/ + * limit_new_plugin()/limit_seed_config()), which are now real symbols + * defined by this include and would collide at link time if left in + * place. */ #include "../src/recording.c" #include "../src/recording_session.c" #include "../src/repl.c" @@ -140,68 +153,16 @@ void kill_sched_user(void* arg) { (void)arg; } void kill_sched_project(void* arg) { (void)arg; } void kill_sched_resval(xce_val_t* val) { (void)val; } -/* repl_info.c (scheduler-sim-diagnostics spec, task 2.1) now calls - * limit_get_configured_limits() (plugins/limit.c, not part of this TU) to - * resolve `info limit`'s name/all argument -- link-satisfying stub only, - * same rationale as kill_sched_job()/kill_sched_resval() above; this - * file's own fixtures never register the limit plugin, so NULL - * (limit_get_configured_limits()'s own documented "plugin absent" return) - * is also the behaviorally correct answer here, not just a link - * placeholder. repl_info_limit() itself is not yet wired into - * repl_cmd_info() (task 4), so no test in this file can reach it anyway. */ -var limit_get_configured_limits(xce_sched_core_t* core) -{ - (void)core; - return NULL; -} - -/* scheduler-sim-diagnostics spec, task 3.1: repl_info.c now also calls - * limit_get_usage_instances() (plugins/limit.c, not part of this TU) to - * read a limit's live usage instances -- identical link-satisfying stub, - * same rationale as limit_get_configured_limits() immediately above; this - * file's own fixtures never register the limit plugin, so NULL - * (limit_get_usage_instances()'s own documented "plugin absent" return) is - * also the behaviorally correct answer here, not just a link placeholder. */ -var limit_get_usage_instances(xce_sched_core_t* core) -{ - (void)core; - return NULL; -} - -/* scheduler-sim-diagnostics spec, task 6 (bug-1825): sim_loop.c's - * sched_sim_driver_run() now also calls sim_bootstrap_limit_plugin() - * (sim_limit.c, pulled into this TU via the #include above), which in - * turn calls limit_new_plugin() (plugins/limit.c, not part of this TU) -- - * identical link-satisfying stub rationale as limit_get_configured_ - * limits()/limit_get_usage_instances() immediately above. Returning NULL - * here is also the behaviorally correct answer, not just a link - * placeholder: sim_bootstrap_limit_plugin() already handles a NULL - * limit_new_plugin() return gracefully (logs an ERROR, registers nothing, - * returns) -- the exact same "out of memory" path sim_bootstrap_picker() - * (sim_picker.c) takes for a NULL builtin_fcfs_new_plugin() -- so this - * file's fixtures simply end up with no limit plugin registered, same - * observable state as before this task. */ -xce_sched_plugin_t* limit_new_plugin(void) -{ - return NULL; -} - -/* limit_seed_config() (plugins/limit.c, not part of this TU) -- bug-1825 - * remediation round 2's new honestly-typed config-seed entry point. - * sim_bootstrap_limit_plugin() (sim_limit.c) only calls it after - * plugin_register_core() has already succeeded against a real, non-NULL - * limit_new_plugin() plugin -- unreachable in this file since the - * limit_new_plugin() stub immediately above always returns NULL, but the - * symbol must still resolve at link time. Same link-satisfying-stub - * rationale as limit_new_plugin() immediately above. */ -int limit_seed_config(xce_sched_plugin_t* plugin, xce_sched_core_t* core, - xce_conf4sched_t* config) -{ - (void)plugin; - (void)core; - (void)config; - return 0; -} +/* task 4.2 (scheduler-sim-live-allocation spec): limit_get_configured_ + * limits()/limit_get_usage_instances()/limit_new_plugin()/limit_seed_ + * config() are now REAL symbols, pulled in for real by the "../src/plugins/ + * limit.c" white-box #include above -- the four link-satisfying stubs that + * previously stood in for them here (each returning NULL/0, since this + * file's fixtures never registered a real limit plugin) are removed, not + * left alongside the real definitions, which would collide at link time. + * sim_bootstrap_limit_plugin() (sim_limit.c, already included) now + * genuinely registers the limit plugin exactly as production does when a + * test's fixture calls it, which the new task 4.2 tests below rely on. */ /* sched_worker_release_host_ctx() is now a REAL (non-stub) symbol pulled in * from resource.c (task 2.1 addition, below) -- no local stand-in needed or @@ -231,6 +192,23 @@ int sched_ctx_get_uncomit_host(xvec_t** v, xce_sched_ctx_t* c, int i) return 0; } +/* Task 4.2 (scheduler-sim-live-allocation spec): plugins/limit.c's + * limit_job_election() calls reset_sched_obj() (../src/schedobj.c, not part + * of this white-box TU -- schedobj.c is not included here) -- a genuine + * link-time dependency of limit.c, not previously reachable in this file + * before this task's #include "../src/plugins/limit.c" addition above. + * Stubbed as a no-op purely to satisfy the linker, same precedent + * test_repl_limit_table.c's own identical stub establishes for the same + * symbol/reason. This is behaviorally correct here, not just a link + * placeholder: every job this file's fixtures build goes through the real + * core_build_job() (core_events.c, white-box #included above), which + * already xlist_init()s/zarray_init()s obj->glimits/hlimits fresh and + * empty -- the real reset_sched_obj() would only ever clear an EARLIER + * match's leftovers, which no job in this file's fixtures ever has (each + * test's job is freshly built and matched against its limit at most + * once), so skipping that clear changes nothing observable here. */ +void reset_sched_obj(xce_sched_obj_t* obj) { (void)obj; } + /* Task 2.1 addition (scheduler-sim-eager-reconciliation, Req 1.2/1.3): a * REAL (not no-op) reimplementation of scheduler.c's own sched_account_jobs() * (scheduler.c:1258, scheduler-sim-decision-parity task 1.1's relocated @@ -2304,6 +2282,249 @@ test_reconcile_removes_reconciled_job_from_picker_candidates(void) "PASS\n"); } +/* ---------------------------------------------------------------------- */ +/* Task 4.2 fixtures (scheduler-sim-live-allocation spec, "Sim-Mode */ +/* Reconciliation Narrowing" component, Requirement 3.4/3.5): register a */ +/* real, core-safe limit plugin via the SAME production bootstrap path */ +/* sched_sim_driver_run() itself uses (sim_bootstrap_limit_plugin(), */ +/* sim_limit.c, bug-1825), mirroring test_repl_limit_table.c's own */ +/* test_limit_job_election_matches_global_limit_via_sim_registered_ */ +/* plugin() fixture technique -- the same resource/limit-config shape, */ +/* duplicated here per this file family's own "duplicate, don't share" */ +/* convention (this file's header comment) rather than sharing statics */ +/* across translation units. */ +/* ---------------------------------------------------------------------- */ + +/* Registers a plain, non-host-scoped "mem" resource directly into `core`'s + * real resmgr -- is_host_based_limit() (src/lib/xcelimit.c) key_find()s a + * limit's val.resname directly against core->resmgr, so this fixture must + * exist for a GLOBAL (non-host-scoped) limit to resolve and match at all; + * same rationale/shape test_repl_limit_table.c's own + * register_per_host_resource() documents (that name describes the + * resource's own per-host consumption shape, not the limit's scope -- + * flags carry no host-scope indicator, so is_host_based_limit() falls back + * to GLOBAL for a limit against it, exactly what these two tests need). */ +static xce_resource_t* register_mem_resource(xce_sched_core_t* core) +{ + xce_resource_t wire_resource; + xce_resource_t* r; + + memset(&wire_resource, 0, sizeof(wire_resource)); + wire_resource.name = (char*)"mem"; + wire_resource.valtype = XCE_VAL_NUMERIC; + wire_resource.flags = XCE_RESF_CONSUMABLE; + r = makeResource(core->resmgr, &wire_resource); + TEST_ASSERT_NOT_NULL(r); + return r; +} + +/* A minimal, heap-allocated xce_limit_config_t against the "mem" resource + * registered above -- identical shape test_repl_limit_table.c's own + * make_limit_config() documents (no per-consumer groups, so build_limit()'s + * build_consumer() falls back to get_group(mgr, "all") for every + * consumer). `name` is expected to be a string literal (build_limit() + * strdup()s it). */ +static xce_limit_config_t* make_global_limit_config(const char* name) +{ + xce_limit_config_t* cfg = (xce_limit_config_t*)calloc(1, sizeof(*cfg)); + xce_limit_val_t* val = (xce_limit_val_t*)calloc(1, sizeof(*val)); + + TEST_ASSERT_NOT_NULL(cfg); + TEST_ASSERT_NOT_NULL(val); + + val->resname = "mem"; + val->percent = 0; + val->val = 10.0; + + cfg->name = (char*)name; + cfg->val = val; + + return cfg; +} + +/* Bootstraps a real, core-safe limit plugin (production path: + * plugin_register_core() + limit_seed_config(), via sim_bootstrap_limit_ + * plugin() -- sim_limit.c, bug-1825) against `sim`, seeded with one GLOBAL + * limit named `limit_name`, and bumps core->configver past the fresh + * xce_sched_obj_t default of 0 -- limit_job_election()'s own "already + * evaluated against this config" guard (obj->configver >= core->configver) + * would otherwise skip matching entirely, since both default to 0. Same + * ordering/rationale test_repl_limit_table.c's own + * test_limit_job_election_matches_global_limit_via_sim_registered_plugin() + * establishes. Returns the registered plugin (for callers that want to + * assert on it directly; both tests below only need the side effects). */ +static xce_sched_plugin_t* +bootstrap_global_limit_plugin(xce_sched_sim_driver_t* sim, + const char* limit_name) +{ + xce_sched_plugin_t* plugin = NULL; + xce_limit_config_t* limits[1]; + xce_conf4sched_t config; + int rc; + + TEST_ASSERT_NOT_NULL(register_mem_resource(&(sim->core))); + + limits[0] = make_global_limit_config(limit_name); + memset(&config, 0, sizeof(config)); + config.nlimits = 1; + config.limits = limits; + sim->core.config = &config; + + sim_bootstrap_limit_plugin(sim); + + rc = key_find(sim->core.plugins, "xce_sched_limit", (void**)&plugin); + TEST_ASSERT_TRUE(rc >= 0); + TEST_ASSERT_NOT_NULL(plugin); + + sim->core.configver = 1; + + return plugin; +} + +/* ---------------------------------------------------------------------- */ +/* Test (task 4.2's own PRIMARY Observable, Requirement 3.4): a job whose */ +/* dispatch is found only via reconciliation still gets its GLOBAL (non- */ +/* host-scoped) limit usage registered, through the same core-safe */ +/* plugin_job_election() dispatch normal mode's own sched_allocator() calls */ +/* (task 1.2) -- design.md's "Sim-Mode Reconciliation Narrowing" component. */ +/* Host-scoped matching is deliberately OUT of scope for this call (design */ +/* .md's own text: it needs a resolved worker context this function does */ +/* not independently construct) -- this test only proves the GLOBAL path. */ +/* ---------------------------------------------------------------------- */ + +static void +test_reconcile_registers_global_limit_usage_for_reconciled_job(void) +{ + xce_sched_sim_driver_t sim; + xce_sched_job_t* job; + xce_job_alloc_t* jalloc; + replay_session_t* decoded; + var instances_handle; + iterator_t it; + void* item = NULL; + int rc; + + fixture_init(&sim); + + bootstrap_global_limit_plugin(&sim, "sim_reconcile_global_limit"); + + job = add_job(&(sim.core), 7090ULL, JOB_STAT_RUN, NULL); + TEST_ASSERT_NULL(job->accounts); + /* match_job_global_limit_() (plugins/limit.c) matches against + * job->schedobj->user/queue/project directly -- add_job()'s own + * core_build_job() path leaves ->project NULL (submit.project_name is + * never set by that shared fixture helper, and makeString(NULL) + * returns NULL, src/lib/xstring.c), which match_limit() cannot safely + * take. Set it directly here, scoped to this test only, rather than + * widening add_job() itself for every other test in this file that has + * no need of it. */ + job->schedobj->project = (char*)""; + + jalloc = make_test_job_alloc_with_host(7090ULL, "worker1", "mem", 2.0f); + decoded = make_decoded_session(1000ULL, + make_dispatch_entry_from_alloc(jalloc)); + + sim_reconcile_unaccounted_jobs(&sim, decoded, NULL); + + TEST_ASSERT_NOT_NULL_MESSAGE( + job->accounts, + "sanity: the job's dispatch must still be reconstructed exactly as " + "before this task"); + + instances_handle = limit_get_usage_instances(&(sim.core)); + TEST_ASSERT_NOT_NULL_MESSAGE( + instances_handle, + "task 4.2: a job whose dispatch is found only via reconciliation " + "must still have its GLOBAL limit usage registered, via the same " + "core-safe plugin_job_election() dispatch normal mode uses"); + + rc = iter_make(&it, instances_handle); + TEST_ASSERT_TRUE(rc >= 0); + rc = iter_next(&it, &item); + TEST_ASSERT_TRUE(rc >= 0); + TEST_ASSERT_NOT_EQUAL(RTTI_ITER_NO_DATA, rc); + TEST_ASSERT_NOT_NULL_MESSAGE( + item, "a real usage instance must exist for the reconciled job's " + "global limit match, visible through the existing " + "limit_get_usage_instances() accessor -- the same one `info " + "limit` reads"); + + printf("test_reconcile_registers_global_limit_usage_for_reconciled_job: " + "PASS\n"); +} + +/* ---------------------------------------------------------------------- */ +/* Test (task 4.2, Requirement 3.5's supporting guard): a job the LIVE path */ +/* has already decided (job->accounts != NULL, as task 4.1's live picker/ */ +/* allocator would leave it) is never reprocessed by this mechanism -- the */ +/* existing trigger guard (job->accounts == NULL) already excludes it */ +/* BEFORE this task's new plugin_job_election() call is ever reached, so no */ +/* limit usage is (re-)registered and the job's own schedobj->configver */ +/* stays untouched. Extends test_reconcile_skips_job_that_already_has_ */ +/* accounts (Test 3 above), which already proves job->accounts/n_decisions */ +/* are untouched for this shape -- this test proves the NEW call site */ +/* respects that same exclusion. */ +/* ---------------------------------------------------------------------- */ + +static void +test_reconcile_does_not_reregister_limit_for_already_decided_job(void) +{ + xce_sched_sim_driver_t sim; + xce_sched_job_t* job; + xce_job_account_t* accounts_before; + replay_session_t* decoded; + var instances_handle; + iterator_t it; + void* item = NULL; + int rc; + + fixture_init(&sim); + + bootstrap_global_limit_plugin(&sim, "sim_reconcile_already_decided_limit"); + + /* Job already decided by the LIVE path (task 4.1) -- job->accounts != + * NULL, the same "already accounted" fixture shape Test 3 + * (test_reconcile_skips_job_that_already_has_accounts) above + * establishes. */ + job = add_job(&(sim.core), 7091ULL, JOB_STAT_RUN, + make_job_alloc(7091ULL, 0, NULL)); + TEST_ASSERT_NOT_NULL_MESSAGE( + job->accounts, + "fixture setup: job must already have accounts, as if the live " + "path already decided it"); + accounts_before = job->accounts; + job->schedobj->project = (char*)""; + + decoded = make_decoded_session(1000ULL, make_dispatch_entry(7091ULL)); + + sim_reconcile_unaccounted_jobs(&sim, decoded, NULL); + + TEST_ASSERT_EQUAL_PTR_MESSAGE( + accounts_before, job->accounts, + "an already-decided job must never be re-touched by reconciliation " + "(unchanged, pre-existing guard)"); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 0, job->schedobj->configver, + "task 4.2: the existing trigger guard already excludes an " + "already-decided job before the new plugin_job_election() call is " + "reached -- its schedobj->configver (bumped by a real election " + "call) must stay at its untouched default"); + + instances_handle = limit_get_usage_instances(&(sim.core)); + TEST_ASSERT_NOT_NULL(instances_handle); + rc = iter_make(&it, instances_handle); + TEST_ASSERT_TRUE(rc >= 0); + rc = iter_next(&it, &item); + TEST_ASSERT_EQUAL_INT_MESSAGE( + RTTI_ITER_NO_DATA, rc, + "no limit usage instance may be created for an already-decided " + "job via this mechanism -- plugin_job_election() must never reach " + "it"); + + printf("test_reconcile_does_not_reregister_limit_for_already_decided_" + "job: PASS\n"); +} + int main(void) { UNITY_BEGIN(); @@ -2338,5 +2559,8 @@ int main(void) RUN_TEST( test_reconcile_info_worker_and_summary_hide_completed_job_after_stray_dispatch); RUN_TEST(test_reconcile_removes_reconciled_job_from_picker_candidates); + RUN_TEST(test_reconcile_registers_global_limit_usage_for_reconciled_job); + RUN_TEST( + test_reconcile_does_not_reregister_limit_for_already_decided_job); return UNITY_END(); } diff --git a/src/scheduler/testsuite/test_sim_session_ordering.c b/src/scheduler/testsuite/test_sim_session_ordering.c index ca1e25b0..a147d8c3 100644 --- a/src/scheduler/testsuite/test_sim_session_ordering.c +++ b/src/scheduler/testsuite/test_sim_session_ordering.c @@ -256,6 +256,24 @@ int sched_fill_limit_requests(xce_sched_alloc_req_t* alloc_req, return 0; } +/* Task 4.2 (scheduler-sim-live-allocation spec): sim_reconcile_unaccounted_ + * jobs() (sim_loop.c, white-box #included above) now also calls + * plugin_job_election() (plugin.c, not part of this TU -- this file does + * not include plugin.c) for a job it reconstructs from the recording, so a + * genuine reconciliation round in this file's own fixtures now reaches it + * for the first time -- link-satisfying stub only, same rationale as + * sched_fill_limit_requests() above. A no-op is also the behaviorally + * correct answer here: this file's own fixtures test round-boundary + * ordering (the plugin_begin_session()/plugin_end_session() spies below), + * never limit-matching, and no test here inspects any effect + * plugin_job_election() would have. */ +int plugin_job_election(xce_sched_core_t* core, xce_sched_job_t* job) +{ + (void)core; + (void)job; + return 0; +} + /* xce_sched_driver_on_decision()'s (driver.c) publish-side linker * dependencies -- never actually invoked (this test never registers a * decision callback). Same rationale test_sim_equivalence.c's own copy