From 0893fd50af32ffa127240b6b0db32ceaab220458 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Thu, 27 Aug 2026 04:15:16 +0000 Subject: [PATCH] test(scheduler-sim-eager-reconciliation): task 2.1 resInUse deferral proof Adds test_captured_pend_job_excluded_from_resinuse_until_run: drives a job captured while still PEND through sched_account_jobs() and confirms its resource-in-use contribution is zero; transitions the same job to RUN and confirms sched_account_jobs() now includes its exact captured allocation, with no second reconciliation call in between -- proving the nonzero value comes from the earlier capture, not a fresh one. Required promoting the file's sched_account_jobs() from a no-op stub to a real reimplementation (verbatim body, matching the exact precedent already established in test_sim_contention_parity.c and test_sim_round_boundary_reconciliation.c) plus a resource.c white-box include -- no production code touched. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01AgYRU3zDTqNGV5Jk8aiKLg --- .../tasks.md | 2 +- .../test_sim_reconcile_unaccounted.c | 292 ++++++++++++++++-- 2 files changed, 272 insertions(+), 22 deletions(-) diff --git a/.kiro/specs/scheduler-sim-eager-reconciliation/tasks.md b/.kiro/specs/scheduler-sim-eager-reconciliation/tasks.md index 80fe6337..c0d73ee7 100644 --- a/.kiro/specs/scheduler-sim-eager-reconciliation/tasks.md +++ b/.kiro/specs/scheduler-sim-eager-reconciliation/tasks.md @@ -18,7 +18,7 @@ - _Boundary: sim_reconcile_unaccounted_jobs_ - [ ] 2. Integration: prove captured-but-not-running allocations stay excluded from resource-in-use accounting until the job actually runs -- [ ] 2.1 Verify resource-in-use accounting defers to the job's real running state, not its capture time +- [x] 2.1 Verify resource-in-use accounting defers to the job's real running state, not its capture time - Drive a captured, not-yet-running job's allocation through the resource-in-use recomputation and confirm it contributes nothing to worker or shared resource usage - Transition that same job to a running state and drive resource-in-use recomputation again, confirming its allocation now contributes from that point forward - Observable: a test demonstrates the same captured job's resource usage is zero before its running-state transition and correctly non-zero after, using the exact allocation captured earlier diff --git a/src/scheduler/testsuite/test_sim_reconcile_unaccounted.c b/src/scheduler/testsuite/test_sim_reconcile_unaccounted.c index 671afd6b..1aeda31e 100644 --- a/src/scheduler/testsuite/test_sim_reconcile_unaccounted.c +++ b/src/scheduler/testsuite/test_sim_reconcile_unaccounted.c @@ -2,7 +2,12 @@ * test_sim_reconcile_unaccounted.c - contract test for * sim_reconcile_unaccounted_jobs() (sim_loop.c, scheduler-sim-decision- * parity spec, tasks 2.2-2.4, "Recorded-Decision Reconciliation" component, - * design.md; Requirement 1.1, 1.2, 3.1, 3.2, 3.3, 4.1, 4.2). + * design.md; Requirement 1.1, 1.2, 3.1, 3.2, 3.3, 4.1, 4.2). Also covers + * scheduler-sim-eager-reconciliation task 2.1 (Requirement 1.2, 1.3, 1.4): + * resource-in-use accounting via the REAL sched_account_jobs() (scheduler.c, + * unchanged by that spec, verbatim-copied in below -- see this file's own + * "White-box #include set" paragraph) correctly defers to a captured job's + * REAL running state, not its (now possibly much earlier) capture time. * * Scope: the once-per-round trigger (RUN-state job with job->accounts == * NULL), the jobid-based search through this round's @@ -22,13 +27,25 @@ * sched_sim_driver_run()'s round-boundary sequence) -- called directly here * instead, standalone, per design.md's own Testing Strategy. * - * White-box #include set and stub set: IDENTICAL to + * White-box #include set and stub set: originally IDENTICAL to * test_sim_event_loop.c's own (same rationale -- sim_loop.c's compiled TU * unconditionally references every one of these as externs, whether or not * any single test here calls sched_sim_driver_run() itself). This file * never calls sched_sim_driver_run()/spawns the picker thread, so (unlike * test_sim_event_loop.c) no `static` storage workaround is needed for its - * own `xce_sched_sim_driver_t` fixtures. + * own `xce_sched_sim_driver_t` fixtures. Task 2.1 (scheduler-sim-eager- + * reconciliation) additionally #includes ../src/resource.c and replaces the + * PREVIOUS no-op sched_account_jobs() stub with a REAL reimplementation + * (body copied verbatim from scheduler.c, same precedent test_sim_ + * contention_parity.c's own copy documents -- scheduler.c itself is never + * white-box #included in this directory, too large a link-time dependency + * surface, see test_resource_accounting.c's own header comment) -- this + * file's own task 2.1 test is the first test here to actually call that + * function, and a no-op stub would defeat its entire premise. Pulling in + * resource.c makes sched_worker_release_host_ctx() a REAL (non-stub) symbol + * too (previously a local no-op here) and adds one new link-satisfying-only + * stub, sched_ctx_get_uncomit_host() -- see the stub definitions below for + * both. * * Fixture technique: xce_sched_core_init() plus core_build_job()/ * sched_add_job() (core_events.c/core.c, both white-box #included) to place @@ -54,6 +71,7 @@ #include "controller.capnp.h" #include "kvmgr.h" +#include "resobj.h" #include "scheduler.h" #include "xstring.h" @@ -73,6 +91,7 @@ #include "../src/repl_break.c" #include "../src/repl_info.c" #include "../src/repl_params.c" +#include "../src/resource.c" #include "../src/sim.c" #include "../src/sim_loop.c" #include "../src/sim_picker.c" /* bug-1567: sim_loop.c calls sim_bootstrap_picker() (sim_loop.c-cap sibling) as an extern -- undefined reference at link time otherwise */ @@ -89,31 +108,84 @@ 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; } -/* core_reset_workers()'s (core_session.c) linker dependency -- never - * invoked (no worker is ever added here). */ -void sched_worker_release_host_ctx(xce_sched_worker_t* w) { (void)w; } +/* 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 + * permitted (a local definition here would collide at link time). Unused by + * any test in this file (no worker is ever released), listed only because + * resource.c defines it -- same precedent test_resource_accounting.c's own + * header comment documents. */ -/* plugin_prepare_allocation()'s (plugin.c, the "limit" plugin category) - * linker dependency -- never invoked (this file never drives real - * allocation). */ -int sched_fill_limit_requests(xce_scheduler_t* sched, xvec_idval_t* vec, - xlist_t* limits) +/* sched_fill_limit_requests() (plugin_prepare_allocation()'s, plugin.c, the + * "limit" plugin category, linker dependency) is likewise now a REAL + * (non-stub) symbol pulled in from resource.c -- no local stand-in needed + * or permitted (a local definition here would collide at link time, same + * as sched_worker_release_host_ctx() above). Never invoked by any test in + * this file (no real allocation is ever driven), listed only because + * resource.c defines it. */ + +/* resource.c's sched_try_alloc_on_host_()'s own link-time dependency -- + * never actually invoked (this file never drives real allocation, only + * calls sched_account_jobs()/sched_charge_account() directly). Same + * link-satisfying-only stub test_sim_contention_parity.c's own copy + * establishes needing once resource.c is white-box #included. */ +int sched_ctx_get_uncomit_host(xvec_t** v, xce_sched_ctx_t* c, int i) { - (void)sched; - (void)vec; - (void)limits; + (void)v; + (void)c; + (void)i; return 0; } -/* sched_sim_driver_run()'s (sim_loop.c) round-boundary call to the - * relocated sched_account_jobs() (core.h) -- a no-op stand-in here since - * this file does not link scheduler.c/resource.c and never calls - * sched_sim_driver_run() itself; needed purely to satisfy the linker - * (sim_loop.c's compiled TU references it unconditionally). */ +/* 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 + * xce_sched_core_t* signature) -- body copied verbatim, same precedent + * test_sim_contention_parity.c's own copy of this function documents + * (scheduler.c itself is never white-box #included in this directory -- + * ~1400 lines, too large a link-time dependency surface, see + * test_resource_accounting.c's own header comment). This file's task 2.1 + * test exists specifically to observe what THIS function does to + * w->resInUse for a captured-but-not-yet-RUN job vs. the same job once it + * transitions to RUN -- a no-op stub would defeat that test's entire + * premise, exactly like test_sim_contention_parity.c's own rationale for + * needing the real body. This supersedes the file's previous no-op stand-in + * (task 1.1 and earlier tests here never called this function themselves, + * so the no-op was harmless for them; task 2.1 is the first test in this + * file to call it). */ int sched_account_jobs(xce_sched_core_t* core) { - (void)core; - return 0; + iterator_t it; + int rc = 0; + + rc = iter_make(&it, core->jobs); + if (rc < 0) { + return rc; + } + + for (;;) { + xce_sched_job_t* job = NULL; + + rc = iter_next(&it, (void**)&job); + if ((rc < 0) || (rc == RTTI_ITER_NO_DATA)) { + break; + } + + if (!JOB_IS_RUN(job->state)) { + continue; + } + + if (job->accounts != NULL) { + sched_charge_account(core, job->accounts); + } + } + + if (rc == RTTI_ITER_NO_DATA) { + rc = 0; + } + + iter_destroy(&it); + + return rc; } /* Real reimplementation of schedobj.c's sched_resolve_job_picker -- same @@ -303,6 +375,69 @@ static replay_decision_t* make_dispatch_entry_from_alloc(xce_job_alloc_t* alloc) return rd; } +/* Task 2.1 fixture helpers (scheduler-sim-eager-reconciliation, Req + * 1.2/1.3): register a real, consumable "slots" resource and add a real + * worker owning a finite amount of it -- same technique + * test_sim_contention_parity.c's own register_slots_resource()/add_worker() + * establish (that file's own header comment documents why the resource must + * be pre-registered: reconciliation's real make_job_account()/ + * make_res_account() path always produces residx == -1, lazily resolved by + * sched_update_res_accounts()'s own key_find(core->resmgr, ...) against + * whatever the resource was registered as -- pre-registering here means + * this fixture's own worker resvals/resInUse setup and the reconciled + * job's eventual account both resolve to the SAME index). */ +static int register_slots_resource(xce_sched_core_t* core) +{ + xce_resource_t* res_template + = (xce_resource_t*)calloc(1, sizeof(*res_template)); + xce_resource_t* registered; + + TEST_ASSERT_NOT_NULL(res_template); + res_template->name = strdup("slots"); + res_template->valtype = XCE_VAL_NUMERIC; + res_template->ordtype = XCE_ORD_DECR; + res_template->flags = XCE_RESF_LIMIT | XCE_RESF_CONSUMABLE + | XCE_RESF_RELEASE | XCE_RESF_DEFAULT_HOST; + + registered = makeResource(core->resmgr, res_template); + TEST_ASSERT_NOT_NULL(registered); + + return registered->idx; +} + +/* Adds a real xce_sched_worker_t owning `capacity` units of `residx`, added + * directly via sched_add_worker() -- sched_add_worker() itself does not + * initialize resvals/resInUse, callers own it (same convention every + * worker-adding fixture in this directory establishes). resInUse starts + * genuinely empty (zarray_init only, nothing put()) -- Requirement 1.2's + * own "still zero" checkpoint below depends on this starting state, not on + * an explicit zero being written. */ +static xce_sched_worker_t* add_worker(xce_sched_core_t* core, const char* name, + int residx, float capacity) +{ + xce_sched_worker_t* w; + int rc; + float val[1] = { capacity }; + + w = (xce_sched_worker_t*)calloc(1, sizeof(*w)); + TEST_ASSERT_NOT_NULL(w); + w->name = strdup(name); + TEST_ASSERT_NOT_NULL(w->name); + + rc = zarray_init(&(w->resvals), 8, NULL); + TEST_ASSERT_EQUAL_INT(0, rc); + rc = zarray_init(&(w->resInUse), 8, NULL); + TEST_ASSERT_EQUAL_INT(0, rc); + + rc = put_resvals(&(w->resvals), 1, &residx, val); + TEST_ASSERT_TRUE(rc >= 0); + + rc = sched_add_worker(core, w); + TEST_ASSERT_TRUE(rc >= 0); + + return w; +} + /* ---------------------------------------------------------------------- */ /* Test 1 (the task's own primary Observable): a RUN-state job with no */ /* known allocation and no matching recorded decision in this round's */ @@ -853,6 +988,120 @@ static void test_reconcile_null_args_are_a_safe_noop(void) printf("test_reconcile_null_args_are_a_safe_noop: PASS\n"); } +/* ---------------------------------------------------------------------- */ +/* Test 12 (scheduler-sim-eager-reconciliation task 2.1, Req 1.2, 1.3, */ +/* 1.4): resource-in-use accounting must defer to the job's REAL running */ +/* state, not its capture time. task 1.1 widened sim_reconcile_unaccounted_ */ +/* jobs()'s trigger so a job's allocation is captured (job->allocs/ */ +/* job->accounts set) as soon as its recorded DISPATCH decision is found, */ +/* regardless of job->state -- this test proves the OTHER half of that */ +/* design's correctness: sched_account_jobs() (scheduler.c, UNCHANGED, */ +/* real body pulled in above) still gates charging on JOB_IS_RUN(state), */ +/* so a captured-but-still-PEND job contributes nothing to w->resInUse */ +/* (Req 1.2) until it genuinely transitions to RUN, at which point the */ +/* SAME captured allocation (never re-captured, never re-typed) is charged */ +/* (Req 1.3). Req 1.4 ("matches normal-mode's equivalent-point reporting") */ +/* is satisfied by construction: this test drives the exact same */ +/* sched_account_jobs() normal mode's own driver.c calls at the identical */ +/* round-boundary position (design.md's Architecture Integration note) -- */ +/* no separate normal-mode comparison fixture is needed to prove a shared */ +/* function behaves consistently with itself. */ +/* ---------------------------------------------------------------------- */ + +static void +test_captured_pend_job_excluded_from_resinuse_until_run(void) +{ + xce_sched_sim_driver_t sim; + xce_sched_job_t* job; + xce_sched_worker_t* w; + xce_job_alloc_t* jalloc; + replay_session_t* decoded; + int residx; + float got; + int rc; + + fixture_init(&sim); + + residx = register_slots_resource(&(sim.core)); + w = add_worker(&(sim.core), "worker1", residx, 10.0f); + + /* PEND-state job, no known allocation yet -- exactly task 1.1's own + * widened trigger shape: its recorded DISPATCH decision has been found + * this round, but its RUN-state confirmation has not been replayed + * yet. */ + job = add_job(&(sim.core), 7030ULL, JOB_STAT_PEND, NULL); + TEST_ASSERT_NULL(job->accounts); + TEST_ASSERT_NULL(job->allocs); + + jalloc = make_test_job_alloc_with_host(7030ULL, "worker1", "slots", 4.0f); + decoded = make_decoded_session(1000ULL, + make_dispatch_entry_from_alloc(jalloc)); + + sim_reconcile_unaccounted_jobs(&sim, decoded, NULL); + + TEST_ASSERT_EQUAL_PTR_MESSAGE( + jalloc, job->allocs, + "sanity: the PEND job's recorded DISPATCH decision must be captured " + "(task 1.1's own widened gate) before this test's own checkpoints " + "mean anything"); + TEST_ASSERT_NOT_NULL_MESSAGE( + job->accounts, "sanity: job->accounts must be built from the " + "captured allocation even though the job is still " + "PEND"); + TEST_ASSERT_TRUE_MESSAGE( + JOB_STAT_PEND == job->state, + "sanity: the job must still genuinely be PEND at this checkpoint -- " + "otherwise the next assertion (resInUse == 0) would be trivially " + "true for the wrong reason"); + + /* Checkpoint 1 (Requirement 1.2): recompute resInUse from scratch via + * the REAL sched_account_jobs() -- a captured-but-PEND job must + * contribute nothing. */ + rc = sched_account_jobs(&(sim.core)); + TEST_ASSERT_EQUAL_INT(0, rc); + + got = -1.0f; + rc = get_resvals(&(w->resInUse), 1, &residx, &got); + TEST_ASSERT_EQUAL_INT(1, rc); + TEST_ASSERT_EQUAL_FLOAT_MESSAGE( + 0.0f, got, + "Requirement 1.2: a job whose allocation was captured early (while " + "still PEND) must contribute ZERO to its worker's resInUse until it " + "genuinely transitions to RUN -- sched_account_jobs()'s own " + "JOB_IS_RUN gate (scheduler.c, unchanged by this feature) is the " + "sole mechanism deferring this, and must still be effective now " + "that capture happens earlier than before"); + + /* Transition the SAME job to RUN -- direct field write, same white-box + * technique test_sim_contention_parity.c's own capacity_aware_ + * allocator_thread_fn() uses to model a worker-confirmed state change + * without driving the full plugin_job_state_change() machinery (out of + * this file's own boundary -- scheduler.c is not white-box included). */ + job->state = JOB_STAT_RUN; + + /* Checkpoint 2 (Requirement 1.3): recompute resInUse again -- the SAME + * captured allocation (jalloc, never re-captured -- sim_reconcile_ + * unaccounted_jobs() is not called a second time here, proving this + * checkpoint's nonzero value comes from the EARLIER capture, not a + * fresh one) must now be charged. */ + rc = sched_account_jobs(&(sim.core)); + TEST_ASSERT_EQUAL_INT(0, rc); + + got = -1.0f; + rc = get_resvals(&(w->resInUse), 1, &residx, &got); + TEST_ASSERT_EQUAL_INT(1, rc); + TEST_ASSERT_EQUAL_FLOAT_MESSAGE( + 4.0f, got, + "Requirement 1.3: once the captured-early job genuinely transitions " + "to RUN, its worker's resInUse must reflect its allocation from " + "that point forward -- using the EXACT allocation amount (4.0) " + "captured earlier by sim_reconcile_unaccounted_jobs(), not a " + "freshly re-captured or re-typed value"); + + printf( + "test_captured_pend_job_excluded_from_resinuse_until_run: PASS\n"); +} + int main(void) { UNITY_BEGIN(); @@ -870,5 +1119,6 @@ int main(void) RUN_TEST( test_reconcile_reports_reconstructed_decision_through_existing_channel); RUN_TEST(test_reconcile_null_args_are_a_safe_noop); + RUN_TEST(test_captured_pend_job_excluded_from_resinuse_until_run); return UNITY_END(); }