From e8c2087d5e98f89d2b4d2f00e046086d4b078cb7 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Sat, 5 Sep 2026 00:57:46 +0000 Subject: [PATCH] feat(scheduler-allocation): clear unsatisfiable-resreq cache per session Clear the cache inside core_begin_session_internal() so a job cached as unsatisfiable in one session is re-evaluated fresh in the next (Req 7.2). An initial weak-symbol cast-based approach was rejected: it corrupted xce_sched_sim_driver_t.alt_events_file, since core_begin_session_internal() is shared between driver.c (real xce_scheduler_t) and sim_loop.c (a structurally different, deliberately isolated sim driver struct with no cache field at the same byte offset) -- the strong cache_clear() symbol won unconditionally for both, invisible to ASAN and untested by any test_sim_*.c file. Fixed via a driver-registered callback: xce_sched_core_t gains a cache_clear_cb hook (NULL by default, per xce_sched_core_init()'s full-struct memset), set only in main.c's driver-mode branch -- mirroring the existing on_decision/xce_core_set_decision_cb() pattern. --sim mode returns before that registration is ever reached, so its hook stays NULL and core_begin_session_internal()'s guarded call is a no-op. The real xce_scheduler_t-casting adapter lives in alloc_cache.c rather than driver.c, since ~11 tests white-box-include driver.c without linking alloc_cache.c. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01XYdtrEWWZfKRRGED7raEYg --- .kiro/specs/scheduler-allocation/tasks.md | 4 +- src/scheduler/include/core.h | 64 +++++ src/scheduler/include/sched/alloc_cache.h | 10 + src/scheduler/src/alloc_cache.c | 46 ++++ src/scheduler/src/core_session.c | 45 +++- src/scheduler/src/main.c | 14 ++ src/scheduler/testsuite/meson.build | 58 +++++ .../testsuite/test_core_session_cache_clear.c | 237 ++++++++++++++++++ .../test_sim_cache_clear_isolation.c | 140 +++++++++++ 9 files changed, 614 insertions(+), 4 deletions(-) create mode 100644 src/scheduler/testsuite/test_core_session_cache_clear.c create mode 100644 src/scheduler/testsuite/test_sim_cache_clear_isolation.c diff --git a/.kiro/specs/scheduler-allocation/tasks.md b/.kiro/specs/scheduler-allocation/tasks.md index aa32e7f7..7c07866b 100644 --- a/.kiro/specs/scheduler-allocation/tasks.md +++ b/.kiro/specs/scheduler-allocation/tasks.md @@ -79,7 +79,7 @@ - _Requirements: 7.3, 7.4, 7.5_ - _Boundary: scheduler.c dispatch_ - _Depends: 2.2, 3.1, 4.2_ -- [ ] 5.2 (P) Clear the cache at the start of every scheduling session +- [x] 5.2 (P) Clear the cache at the start of every scheduling session - Clear the cache as part of the existing per-session reset step, alongside the other state that step already resets - Observable: a job cached as unsatisfiable in one session is re-evaluated fresh (not immediately pended from the cache) in the next session - _Requirements: 7.2_ @@ -122,3 +122,5 @@ - (5.1) A nested primary (inside a COMPOUND/ALTER) has no real per-primary `xce_sched_obj_t`; `sched_primary_cache_obj_()` synthesizes one on the stack per dispatch for cache-key purposes. This is memory-safe (verified line-by-line): `alloc_cache.c`'s `build_key_()` copies the fingerprint and identity fields by value synchronously during the call, before the stack frame unwinds — no pointer into the caller's stack locals is ever retained. - (5.1) Req 7.5's "same reason category" is only exercisable in the degenerate all-zero case today: no code path anywhere in the scheduler sets `xce_job_preason_t.main_reason` to a nonzero value except this task's own new `sched_pend_from_cached_reason_()` — every existing allocator-exhaustion path only sets host/resource sub-reasons via `add_job_preason()`. If a future task wants to prove genuine (non-degenerate) category propagation, it needs a path that actually produces a nonzero `main_reason` first. - (5.1) Two significant, pre-existing, out-of-scope bugs were discovered and confirmed during this task's review, logged as bug-1768 (compound/alter-nested primaries never have real slot/rusage availability checked — `seqjob_prepare_allocation()`/`parjob_prepare_allocation()` both no-op when `ctx->schedobj->tag != XCE_RESREQ_PRIMARY`) and bug-1769 (`sched_make_alter_allocation()` type-confuses `xce_compound_schedobj_t*` as `xce_sched_obj_t*`, a ~112-byte heap over-read, currently completely untested — the existing test believed to cover ALTER dispatch actually parses as PRIMARY per the resreq grammar). Neither is in this feature's scope to fix; both need dedicated follow-up. +- (5.2) `core_begin_session_internal()` (`core_session.c`) is called from TWO structurally incompatible types sharing the same `xce_sched_core_t core` prefix: `driver.c` (`xce_scheduler_t*`) and `sim_loop.c` (`xce_sched_sim_driver_t*`, deliberately isolated, no cache field). Round 1's `__attribute__((weak))` cast-based approach caused every simulation session to silently corrupt `xce_sched_sim_driver_t.alt_events_file` (same byte offset as `xce_scheduler_t.unsatisfiable_cache`), invisible to ASAN (in-bounds) and to the whole test suite (no `test_sim_*.c` links `alloc_cache.c`). **Any future per-session hook that needs the real `xce_scheduler_t` (not just the shared `xce_sched_core_t` prefix) must use the driver-registered-callback pattern** (`xce_sched_core_t.cache_clear_cb`, set via `main.c` only in the driver-mode branch — `--sim` mode returns before reaching that registration — mirroring the existing `on_decision`/`xce_core_set_decision_cb()` precedent), never a cast of `core` back to `xce_scheduler_t*` inside code shared by both driver and sim paths. +- (5.2) The real `cache_clear(xce_scheduler_t*)`→`xce_alloc_cache_clear_cb(xce_sched_core_t*)` adapter lives in `alloc_cache.c`, not `driver.c`, specifically because ~11 test files white-box `#include "../src/driver.c"` without linking `alloc_cache.c` — placing the adapter in `driver.c` would break their link. Any future adapter bridging a driver-only hook to a subsystem those tests don't link should follow this same placement rule (adapter lives with the subsystem it calls into, not with `driver.c`). diff --git a/src/scheduler/include/core.h b/src/scheduler/include/core.h index 25cdd2b2..9f7df795 100644 --- a/src/scheduler/include/core.h +++ b/src/scheduler/include/core.h @@ -69,6 +69,46 @@ typedef struct xce_sched_core_s xce_sched_core_t; */ typedef int (*xce_core_decision_cb_t)(void*, xce_decision_t*); +/* + * xce_core_cache_clear_cb_t - driver-registered per-session cache-clear + * hook (scheduler-allocation spec, task 5.2 round 2, Req 7.2): + * `void (*)(xce_sched_core_t*)`. Mirrors xce_core_decision_cb_t's + * driver-registered-hook idiom just above, but generic on purpose -- no + * `void* driver_ctx` companion field, because the sole real + * implementation (alloc_cache.c's xce_alloc_cache_clear_cb() -- see that + * function's own doc comment for why it lives in alloc_cache.c rather + * than driver.c) recovers its owning xce_scheduler_t* via the + * pointer-to-first-member idiom this codebase already uses for this exact + * struct (scheduler.h: `xce_sched_core_t core` is xce_scheduler_t's first + * member), not via an opaque context pointer. + * + * WHY a hook instead of a direct call (task 5.2 round 1's CRITICAL defect): + * core_begin_session_internal() (core_session.c) is reached from BOTH + * driver.c's xce_sched_driver_run_normal_mode_round() (a real + * xce_scheduler_t*) and sim_loop.c's sched_sim_driver_run() (a + * structurally different xce_sched_sim_driver_t*, sim.h -- Req 5 AC 17 + * "Simulation isolation" -- which has NO unsatisfiable_cache field at + * all). Round 1 called alloc_cache.c's real, strong + * cache_clear(xce_scheduler_t*) unconditionally for both callers (routed + * through a same-named weak fallback that the strong definition always + * overrides at link time in the production binary) -- since link-time + * weak/strong resolution picks ONE winner process-wide, not per call + * site, every simulation-mode session reinterpreted whatever field sits + * at unsatisfiable_cache's byte offset in xce_sched_sim_driver_t + * (alt_events_file, a live FILE*) as a kvtab_t* and clobbered it. + * + * A per-core-instance function pointer fixes this at the data-ownership + * level: each xce_sched_core_t only ever carries the hook ITS OWN driver + * installed via xce_core_set_cache_clear_cb() below. main.c installs + * alloc_cache.c's xce_alloc_cache_clear_cb() on the real xce_scheduler_t's + * core; sim_loop.c never calls the setter at all, so a simulation-mode core's + * hook stays NULL (xce_sched_core_init()'s memset() default) and + * core_begin_session_internal() treats NULL as "this driver has no cache + * to clear" and skips the call entirely -- a true no-op, not an + * accidental type-confused one. + */ +typedef void (*xce_core_cache_clear_cb_t)(xce_sched_core_t*); + struct xce_sched_core_s { mylock_t lock; int stop; @@ -110,6 +150,15 @@ struct xce_sched_core_s { * reads these fields today. */ void* driver_ctx; xce_core_decision_cb_t on_decision; + /* Req 7.2 (scheduler-allocation spec, task 5.2 round 2): driver- + * registered per-session cache-clear hook -- see + * xce_core_cache_clear_cb_t's own doc comment above for the full + * rationale. NULL until a driver registers one (xce_core_set_ + * cache_clear_cb()); core_begin_session_internal() (core_session.c) + * treats NULL as "this driver has no cache to clear" and skips the + * call -- true for every simulation-mode xce_sched_core_t, which never + * registers this hook. */ + xce_core_cache_clear_cb_t cache_clear_cb; _Atomic int session_active; /* 1 during scheduling session, 0 between */ _Atomic int eval_in_progress; /* 1 while eval_select loop runs */ _Atomic int staging_merge_complete; /* 1 after staging merge completes */ @@ -424,6 +473,21 @@ extern int xce_core_push_events(xce_sched_core_t*, xce_core_event_t*, int); extern int xce_core_set_decision_cb(xce_sched_core_t*, xce_core_decision_cb_t, void*); +/* + * xce_core_set_cache_clear_cb - registers the driver's per-session + * cache-clear hook (scheduler-allocation spec, task 5.2 round 2, Req 7.2). + * Same calling convention as xce_core_set_decision_cb() above (may be + * called at any time, typically once at driver startup before the first + * xce_core_begin_session(); returns -EINVAL if core is NULL) -- no + * `driver_ctx` companion parameter, since xce_core_cache_clear_cb_t's own + * doc comment (above) explains why the real implementation needs none. + * Never called by sim_loop.c -- see that typedef's doc comment for why + * leaving this hook unset is the correct, safe default for simulation + * mode. + */ +extern int xce_core_set_cache_clear_cb(xce_sched_core_t*, + xce_core_cache_clear_cb_t); + /* * xce_core_begin_session / xce_core_signal_go - Req 5 AC 5 (task 4.4) split * into two exported halves (task 5.2, Req 1 AC 7 / Req 5 AC 9) so a driver diff --git a/src/scheduler/include/sched/alloc_cache.h b/src/scheduler/include/sched/alloc_cache.h index 49d4c7e0..ef6891dd 100644 --- a/src/scheduler/include/sched/alloc_cache.h +++ b/src/scheduler/include/sched/alloc_cache.h @@ -131,4 +131,14 @@ extern int cache_insert(xce_scheduler_t* sched, xce_sched_obj_t* obj, */ extern void cache_clear(xce_scheduler_t* sched); +/* + * xce_alloc_cache_clear_cb - the xce_core_cache_clear_cb_t (core.h) + * adapter for cache_clear() above (scheduler-allocation spec, task 5.2 + * round 2, Req 7.2). Registered on the real xce_scheduler_t's core via + * xce_core_set_cache_clear_cb() (main.c). See alloc_cache.c's own doc + * comment on this function for the full rationale, including why it lives + * here rather than in driver.c. + */ +extern void xce_alloc_cache_clear_cb(xce_sched_core_t* core); + #endif diff --git a/src/scheduler/src/alloc_cache.c b/src/scheduler/src/alloc_cache.c index 61cd6168..28c68cb5 100644 --- a/src/scheduler/src/alloc_cache.c +++ b/src/scheduler/src/alloc_cache.c @@ -220,3 +220,49 @@ void cache_clear(xce_scheduler_t* sched) sched->unsatisfiable_cache = makeInstance(&kvtab_type, ¶ms); } + +/* + * xce_alloc_cache_clear_cb (scheduler-allocation spec, task 5.2 round 2, + * Req 7.2) - the xce_core_cache_clear_cb_t (core.h) adapter for + * cache_clear() above, registered on the real xce_scheduler_t's core via + * xce_core_set_cache_clear_cb() (main.c, alongside the existing + * xce_core_set_decision_cb() registration). `core` is xce_scheduler_t's + * first member (scheduler.h: `typedef struct { xce_sched_core_t core; + * ... } xce_scheduler_t;`), so per C11 6.7.2.1p15 (a pointer to a + * structure's first member is validly convertible to a pointer to the + * enclosing structure, and vice versa) this cast recovers the correct, + * real xce_scheduler_t whenever this function is called through the + * hook -- valid ONLY because the sole registration call site (main.c) + * installs this exact function on a core instance genuinely embedded in + * a real xce_scheduler_t. + * + * Deliberately defined HERE (alloc_cache.c), not in driver.c, even though + * driver.c is the file that legitimately owns a real xce_scheduler_t* + * elsewhere: alloc_cache.c already owns cache_clear()'s xce_scheduler_t* + * cast, and several white-box test TUs in this suite #include + * ../src/driver.c directly WITHOUT linking alloc_cache.c (test_driver_ + * translate.c, test_driver_decision_cb.c, test_resval_ownership.c, every + * test_sim_*.c/test_admin_request.c/test_record_lifecycle.c that pulls + * driver.c in) -- had this adapter lived in driver.c and called + * cache_clear() directly, every one of those TUs would fail to LINK (a + * reference to an unresolved external symbol is a link error regardless + * of whether the referencing function is ever actually invoked). Keeping + * this adapter in alloc_cache.c means those TUs are entirely unaffected: + * none of them link alloc_cache.c, none of them reference this function, + * and none of them ever call xce_core_set_cache_clear_cb() either -- their + * cache_clear_cb stays NULL, matching production simulation-mode + * behavior exactly (see xce_core_cache_clear_cb_t's own doc comment, + * core.h, for the full "why NULL is correct for sim mode" rationale). + * + * Round 1's CRITICAL defect is NOT reintroduced by this design: round 1's + * bug was calling cache_clear() UNCONDITIONALLY from inside + * core_begin_session_internal() (core_session.c) for every caller, + * indiscriminately. This function is never called unconditionally by + * anything -- it is only ever invoked as core->cache_clear_cb(core), and + * only for a core instance that main.c's own registration explicitly + * installed it on. + */ +void xce_alloc_cache_clear_cb(xce_sched_core_t* core) +{ + cache_clear((xce_scheduler_t*)core); +} diff --git a/src/scheduler/src/core_session.c b/src/scheduler/src/core_session.c index 49bf2edb..9e635f6b 100644 --- a/src/scheduler/src/core_session.c +++ b/src/scheduler/src/core_session.c @@ -21,9 +21,11 @@ * client.h/netio.h/xceapi.h/delay.h/idgen.h/xce/cluster.h/scheduler.h, and * must never call a wall-clock function directly. * - * Public surface (core.h): xce_core_set_decision_cb(), xce_core_go(), - * xce_core_end_session() -- see core.h's own doc comments for the full - * contract. This file is purely ADDITIVE: it does not modify, and is not + * Public surface (core.h): xce_core_set_decision_cb(), + * xce_core_set_cache_clear_cb() (scheduler-allocation spec, task 5.2 round + * 2, Req 7.2), xce_core_go(), xce_core_end_session() -- see core.h's own + * doc comments for the full contract. This file is purely ADDITIVE: it + * does not modify, and is not * called by, the still-live sched_go()/sched_begin_session()/ * sched_end_session()/main.c loop (scheduler.c) -- those remain the * production path until a later task (5.x) wires a driver against this @@ -77,6 +79,30 @@ static void core_reset_shared_resval(xce_sched_core_t* core) * own header comment documents for xce_sched_core_t's typedef. */ extern void sched_worker_release_host_ctx(xce_sched_worker_t*); +/* + * xce_core_set_cache_clear_cb (scheduler-allocation spec, task 5.2 round 2, + * Req 7.2) - registers the driver-supplied per-session cache-clear hook. + * See core.h's xce_core_cache_clear_cb_t / xce_core_set_cache_clear_cb doc + * comments for the full rationale (replaces round 1's weak-symbol + * cache_clear() fallback, which unconditionally routed BOTH driver.c's and + * sim_loop.c's calls into alloc_cache.c's xce_scheduler_t*-typed + * cache_clear() once linked into the production binary -- a type-confusion + * bug for sim mode's structurally different xce_sched_sim_driver_t). + * Mirrors xce_core_set_decision_cb() immediately below in shape and + * discipline. + */ +int xce_core_set_cache_clear_cb(xce_sched_core_t* core, + xce_core_cache_clear_cb_t cb) +{ + if (core == NULL) { + return -EINVAL; + } + + core->cache_clear_cb = cb; + + return 0; +} + /* core_reset_workers - mirrors scheduler.c's sched_reset_workers/ * sched_reset_worker_ (release each available worker's stale per-session * host_eval_ctx snapshot + reset its in-use resource values), parameterized @@ -160,6 +186,19 @@ static void core_begin_session_internal(xce_sched_core_t* core) core_reset_shared_resval(core); core_reset_workers(core); + /* Req 7.2 (scheduler-allocation spec, task 5.2 round 2): clear the + * per-session unsatisfiable-resreq cache alongside the other + * per-session state this function already resets above, via the + * driver-registered hook (core.h's xce_core_cache_clear_cb_t) rather + * than an unconditional call -- see that typedef's own doc comment for + * why round 1's unconditional weak-symbol cache_clear() call was a + * CRITICAL type-confusion bug for simulation-mode sessions, and why a + * NULL hook (sim_loop.c never registers one) must be a true no-op + * here, not a fallthrough to any cache-clearing behavior. */ + if (core->cache_clear_cb != NULL) { + core->cache_clear_cb(core); + } + /* Staging merge done -- core_gc_select_nodes()'s GC guard reads this * field, same discipline sched_begin_session()/sched_end_session() * already follow. */ diff --git a/src/scheduler/src/main.c b/src/scheduler/src/main.c index 3b67d9b5..1482cbbf 100644 --- a/src/scheduler/src/main.c +++ b/src/scheduler/src/main.c @@ -12,6 +12,7 @@ #include "xce/types.h" #include "driver.h" +#include "sched/alloc_cache.h" #include "scheduler.h" #include "sim.h" @@ -457,6 +458,19 @@ int main(int argc, char* argv[]) xce_core_set_decision_cb(&(sched.core), xce_sched_driver_on_decision, &sched); + /* Req 7.2 (scheduler-allocation spec, task 5.2 round 2): register the + * per-session cache-clear hook -- see xce_core_cache_clear_cb_t's doc + * comment (core.h) for why this replaced round 1's unconditional + * weak-symbol cache_clear() call, and alloc_cache.c's own doc comment + * on xce_alloc_cache_clear_cb() for why the adapter lives there + * rather than in driver.c. Registered once, alongside the decision-cb + * registration just above (the only other driver-registered core + * hook), before the first scheduling round. The simulation driver + * (sim_loop.c) deliberately never makes this call -- a + * simulation-mode core has no cache to clear (Req 5 AC 17 + * "Simulation isolation"). */ + xce_core_set_cache_clear_cb(&(sched.core), xce_alloc_cache_clear_cb); + signal(SIGINT, sig_handler); INFO("xce scheduler is running"); diff --git a/src/scheduler/testsuite/meson.build b/src/scheduler/testsuite/meson.build index 4dda6d04..2669c319 100644 --- a/src/scheduler/testsuite/meson.build +++ b/src/scheduler/testsuite/meson.build @@ -1579,6 +1579,58 @@ sched_alloc_cache_test = executable( dependencies: [scheduler_test_deps], install: false) +# Unit test for task 5.2 (scheduler-allocation spec, Req 7.2): clearing the +# unsatisfiable-resreq cache at the start of every scheduling session, from +# inside core_begin_session_internal() (core_session.c), via the +# driver-registered xce_core_cache_clear_cb_t hook (round 2 -- replaces +# round 1's unconditional weak-symbol cache_clear() call, found to be a +# CRITICAL type-confusion bug against simulation-mode sessions; see +# test_sim_cache_clear_isolation_test below). See this test file's own +# header comment for why it lists core.c/core_clock.c/core_session.c/ +# decision.c/alloc_cache.c as genuinely SEPARATE compiled sources rather +# than white-box #include-ing them into one TU (the same "real, +# separately-linked scheduler internals" technique task 5.1's own +# sched_cache_dispatch_test above already established for this feature, +# scaled down to just the files this narrower test needs) -- this test's +# own registered cache_clear_cb calls the REAL, strong cache_clear() +# (alloc_cache.c), which is only meaningfully distinguishable from a stub +# across genuinely separate object files, never within one merged +# translation unit. +sched_core_session_cache_clear_test = executable( + 'sched_core_session_cache_clear_test', + ['test_core_session_cache_clear.c', + '../src/core.c', + '../src/core_clock.c', + '../src/core_session.c', + '../src/decision.c', + '../src/alloc_cache.c'], + include_directories:[incdir], + dependencies: [scheduler_test_deps], + install: false) + +# Regression test for task 5.2 ROUND 2 (scheduler-allocation spec, Req 7.2): +# the CRITICAL defect that got round 1 REJECTED -- core_begin_session_ +# internal() must NEVER touch xce_sched_sim_driver_t's alt_events_file (or +# any other sim-only field) even with alloc_cache.c's real, strong +# cache_clear(xce_scheduler_t*) linked into the same binary, because +# sim_loop.c never registers a cache_clear_cb (Req 5 AC 17 "Simulation +# isolation" -- a simulation-mode core has no cache to clear at all). Same +# separate-compiled-sources technique as sched_core_session_cache_clear_test +# above, for the same reason: this test needs the real, strong +# cache_clear() actually present in the binary to prove the NULL-hook path +# still never calls it. +sched_sim_cache_clear_isolation_test = executable( + 'sched_sim_cache_clear_isolation_test', + ['test_sim_cache_clear_isolation.c', + '../src/core.c', + '../src/core_clock.c', + '../src/core_session.c', + '../src/decision.c', + '../src/alloc_cache.c'], + include_directories:[incdir], + dependencies: [scheduler_test_deps], + install: false) + # Task 3.1 (scheduler-allocation spec, Req 9.1, 4.6): seqjob_allocator.c's # rewrap onto the shared plan_iter/host_iter interfaces, plus the new # maxTasks==1 guard on seqjob_prepare_allocation() (Req 4.6). Same @@ -1823,6 +1875,12 @@ test('scheduler host iter test', test('scheduler alloc cache test', sched_alloc_cache_test, suite: 'scheduler', env: ['ASAN_OPTIONS=detect_leaks=0']) +test('scheduler core session cache clear test', + sched_core_session_cache_clear_test, suite: 'scheduler', + env: ['ASAN_OPTIONS=detect_leaks=0']) +test('scheduler sim cache clear isolation test', + sched_sim_cache_clear_isolation_test, suite: 'scheduler', + env: ['ASAN_OPTIONS=detect_leaks=0']) test('scheduler seqjob allocator test', sched_seqjob_allocator_test, suite: 'scheduler', env: ['ASAN_OPTIONS=detect_leaks=0']) diff --git a/src/scheduler/testsuite/test_core_session_cache_clear.c b/src/scheduler/testsuite/test_core_session_cache_clear.c new file mode 100644 index 00000000..c04e731f --- /dev/null +++ b/src/scheduler/testsuite/test_core_session_cache_clear.c @@ -0,0 +1,237 @@ +/* + * test_core_session_cache_clear.c - scheduler-allocation spec, task 5.2 + * (Req 7.2): the per-session unsatisfiable-resreq cache (alloc_cache.c, + * task 2.2) must be cleared at the start of every scheduling session, from + * inside core_begin_session_internal() (core_session.c), alongside the + * other per-session state that function already resets. + * + * ── Link technique (round 2) ───────────────────────────────────────────── + * Round 1 called alloc_cache.c's real cache_clear(xce_scheduler_t*) + * unconditionally through a same-named WEAK fallback declared in + * core_session.c, relying on link-time weak/strong override -- discovered + * CRITICALLY broken (bug: every simulation-mode session reinterpreted + * xce_sched_sim_driver_t's alt_events_file FILE* as the cache, because + * weak/strong resolution picks one winner process-wide, not per call + * site). Round 2 replaces this with a driver-registered hook + * (core.h's xce_core_cache_clear_cb_t / xce_core_set_cache_clear_cb()): + * core_begin_session_internal() now calls core->cache_clear_cb(core) only + * if non-NULL, and this fixture explicitly registers the REAL production + * adapter (alloc_cache.c's xce_alloc_cache_clear_cb() -- see that + * function's own doc comment for why it lives in alloc_cache.c rather + * than driver.c) on init_sched()'s xce_scheduler_t, the same call main.c + * makes in production. + * + * This file still does NOT use the white-box #include convention most of + * this test suite uses for core_session.c; its meson.build entry still + * lists core.c/core_clock.c/core_session.c/decision.c/alloc_cache.c as + * genuinely separate compiled sources so this fixture links against the + * REAL cache_clear(xce_scheduler_t*) (alloc_cache.c) that its registered + * hook calls into -- not a stub. + * + * ── What this proves ───────────────────────────────────────────────────── + * A job's fingerprint+identity is inserted into a REAL, live + * xce_scheduler_t.unsatisfiable_cache (cache_insert(), simulating what + * task 5.1's allocator-exhaustion path would have done); a lookup for the + * identical key is confirmed a HIT; the REAL xce_core_begin_session() + * (core_session.c's public entry point, exercising the real + * core_begin_session_internal() and this fixture's registered + * cache_clear_cb) is then called; a lookup for the SAME key is then + * confirmed a MISS -- proving the cache was genuinely emptied, not merely + * marked stale, at the exact per-session reset point Req 7.2 names. A + * second test proves the cache is still genuinely usable (insert+lookup + * behaves normally) immediately after a begin_session clear, ruling out a + * cache left permanently broken by whatever cache_clear() does internally. + * See test_sim_cache_clear_isolation.c (new, round 2) for the companion + * proof that a simulation-mode core -- which never registers this hook -- + * is NEVER touched by it. + * + * kill_sched_* / plugin_unregister / sched_worker_release_host_ctx stubs + * below are identical, for identical reasons, to test_core_session.c's own (see + * that file's header comment) -- this file calls the exact same + * xce_sched_core_init()/xce_core_begin_session()/xce_core_end_session() + * entry points, just with alloc_cache.c linked in as a separate source + * alongside core.c/core_clock.c/core_session.c/decision.c instead of + * white-box #include'd. + */ +#include +#include +#include +#include + +#include + +#include "core.h" +#include "sched/alloc_cache.h" + +void kill_sched_job(void* arg) { (void)arg; } +void kill_sched_queue(void* arg) { (void)arg; } +void kill_sched_worker(void* arg) { (void)arg; } +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; } +void plugin_unregister(void* arg) { (void)arg; } + +/* sched_worker_release_host_ctx (resource.c) is outside this test's linked + * source set (only core.c/core_clock.c/core_session.c/decision.c/ + * alloc_cache.c, meson.build) -- stubbed the same way test_core_session.c + * stubs it: satisfies the linker for core_reset_workers()'s call + * (core_session.c). This fixture never adds a worker, so it is never + * actually invoked at runtime either. */ +void sched_worker_release_host_ctx(xce_sched_worker_t* w) { (void)w; } + +void setUp(void) { return; } +void tearDown(void) { return; } + +/* ---------------------------------------------------------------------- */ +/* Fixture helpers */ +/* ---------------------------------------------------------------------- */ + +/* + * init_sched - a real xce_scheduler_t with BOTH core (xce_sched_core_init(), + * core.c) and unsatisfiable_cache (mirroring sched_init()'s own + * construction call, scheduler.c, and test_alloc_cache.c's own + * init_sched()) constructed -- everything core_begin_session_internal() + * touches, without sched_init()'s heavier networking/threading setup + * (this test never calls sched_init() at all). Also registers the REAL + * production cache_clear_cb (round 2, xce_alloc_cache_clear_cb() -- + * alloc_cache.c, linked into this exact test binary, see this file's own + * header comment) -- the same one-time-at-startup registration main.c + * performs in production (xce_core_set_cache_clear_cb(), right alongside + * xce_core_set_decision_cb()), so this fixture's own registration is not + * a stand-in wrapper but the identical call production makes. + */ +static void init_sched(xce_scheduler_t* sched) +{ + kv_params_t params = { .nslots = XCE_UNSAT_CACHE_NSLOTS, + .keyops = &cache_key_ops, + .datakiller = NULL }; + int rc; + + memset(sched, 0, sizeof(*sched)); + + rc = xce_sched_core_init(&(sched->core)); + TEST_ASSERT_EQUAL_INT(0, rc); + + rc = xce_core_set_cache_clear_cb(&(sched->core), xce_alloc_cache_clear_cb); + TEST_ASSERT_EQUAL_INT(0, rc); + + sched->unsatisfiable_cache = makeInstance(&kvtab_type, ¶ms); + TEST_ASSERT_NOT_NULL(sched->unsatisfiable_cache); +} + +/* + * init_primary_obj - identical in shape to test_alloc_cache.c's own helper + * (task 2.2): a minimal, valid xce_sched_obj_t/xce_primary_schedobj_t pair + * ready for cache_insert()/cache_lookup(). + */ +static void init_primary_obj(xce_sched_obj_t* obj, + xce_primary_schedobj_t* primary, int min_tasks, + int max_tasks, uint32_t select_node_id, + uint64_t rusage_hash, const char* user, + const char* queue, const char* project) +{ + memset(obj, 0, sizeof(*obj)); + memset(primary, 0, sizeof(*primary)); + + primary->resreq_fp.min_tasks = min_tasks; + primary->resreq_fp.max_tasks = max_tasks; + primary->resreq_fp.select_node_id = select_node_id; + primary->resreq_fp.rusage_hash = rusage_hash; + + obj->tag = XCE_RESREQ_PRIMARY; + obj->u.primary = primary; + obj->user = (char*)user; + obj->queue = (char*)queue; + obj->project = (char*)project; +} + +/* ---------------------------------------------------------------------- */ +/* Tests */ +/* ---------------------------------------------------------------------- */ + +/* + * THE Observable this task adds (Req 7.2): a job cached as unsatisfiable + * in one session is re-evaluated fresh -- not immediately pended from the + * cache -- in the next session. Proven end to end through the REAL + * cache_insert()/xce_core_begin_session()/cache_lookup(), not a stub. + */ +static void test_begin_session_clears_previously_cached_entry(void) +{ + xce_scheduler_t sched; + xce_sched_obj_t obj; + xce_primary_schedobj_t primary; + int category = 7; + int out = -1; + int rc; + + init_sched(&sched); + init_primary_obj(&obj, &primary, 1, 4, 100u, 0xdeadbeefULL, "alice", + "normal", "proj1"); + + rc = cache_insert(&sched, &obj, category); + TEST_ASSERT_EQUAL_INT(0, rc); + + /* Baseline: proves the fixture/insert are sound before relying on them + * below -- same "prove the harness works first" discipline + * test_core_session.c's own worker-reset tests already follow. */ + rc = cache_lookup(&sched, &obj, &out); + TEST_ASSERT_EQUAL_INT(1, rc); + TEST_ASSERT_EQUAL_INT(category, out); + + rc = xce_core_begin_session(&(sched.core)); + TEST_ASSERT_EQUAL_INT(0, rc); + + out = -1; + rc = cache_lookup(&sched, &obj, &out); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 0, rc, + "Req 7.2: a job cached as unsatisfiable in one session must be " + "re-evaluated fresh (a cache MISS), not immediately pended from " + "the cache, in the next session -- core_begin_session_internal() " + "must clear xce_scheduler_t.unsatisfiable_cache"); + + rc = xce_core_end_session(&(sched.core)); + TEST_ASSERT_EQUAL_INT(0, rc); +} + +/* + * The cleared cache must still be genuinely USABLE afterward -- not left + * permanently broken by whatever cache_clear() does internally (it + * destroys and reconstructs the backing kvtab_t -- see that function's own + * comment, sched/alloc_cache.h). A fresh insert+lookup after begin_session + * must behave exactly like any other insert+lookup. + */ +static void test_cache_still_usable_after_begin_session_clear(void) +{ + xce_scheduler_t sched; + xce_sched_obj_t obj; + xce_primary_schedobj_t primary; + int out = -1; + int rc; + + init_sched(&sched); + + rc = xce_core_begin_session(&(sched.core)); + TEST_ASSERT_EQUAL_INT(0, rc); + + init_primary_obj(&obj, &primary, 2, 8, 200u, 0xfeedfaceULL, "bob", "batch", + "proj2"); + + rc = cache_insert(&sched, &obj, 3); + TEST_ASSERT_EQUAL_INT(0, rc); + + rc = cache_lookup(&sched, &obj, &out); + TEST_ASSERT_EQUAL_INT(1, rc); + TEST_ASSERT_EQUAL_INT(3, out); + + rc = xce_core_end_session(&(sched.core)); + TEST_ASSERT_EQUAL_INT(0, rc); +} + +int main(void) +{ + UNITY_BEGIN(); + RUN_TEST(test_begin_session_clears_previously_cached_entry); + RUN_TEST(test_cache_still_usable_after_begin_session_clear); + return UNITY_END(); +} diff --git a/src/scheduler/testsuite/test_sim_cache_clear_isolation.c b/src/scheduler/testsuite/test_sim_cache_clear_isolation.c new file mode 100644 index 00000000..bb46244f --- /dev/null +++ b/src/scheduler/testsuite/test_sim_cache_clear_isolation.c @@ -0,0 +1,140 @@ +/* + * test_sim_cache_clear_isolation.c - scheduler-allocation spec, task 5.2 + * round 2 (Req 7.2 regression, mandated by the round-1 review that + * REJECTED this task for a CRITICAL memory-corruption defect). + * + * ── The bug this guards against ────────────────────────────────────────── + * core_begin_session_internal() (core_session.c) is reached from BOTH + * driver.c's xce_sched_driver_run_normal_mode_round() (a real + * xce_scheduler_t*, which HAS an unsatisfiable_cache field, scheduler.h) + * and sim_loop.c's sched_sim_driver_run() (a structurally different + * xce_sched_sim_driver_t*, sim.h -- Req 5 AC 17 "Simulation isolation" -- + * which has NO unsatisfiable_cache field at all). Round 1 called + * alloc_cache.c's real, strong cache_clear(xce_scheduler_t*) via a + * same-named WEAK fallback declared in core_session.c, relying on + * link-time weak/strong symbol resolution. Since that resolution picks + * ONE winner for the whole binary (not per call site), BOTH the driver.c + * call (correct) and the sim_loop.c call (WRONG -- there is no cache to + * clear) resolved to the same real cache_clear(xce_scheduler_t*), which + * reinterpreted whatever field sits at unsatisfiable_cache's byte offset + * in xce_sched_sim_driver_t as a kvtab_t* and clobbered it. That offset is + * `alt_events_file` (sim.h) -- a live FILE* every --sim-alt-events run + * depends on. Empirically confirmed by this task's round-1 reviewer, and + * reproduced by this exact test file before round 2's fix (see this + * task's own Status Report for the before/after run). + * + * ── The fix this proves ────────────────────────────────────────────────── + * core_begin_session_internal() now calls a driver-registered hook + * (core->cache_clear_cb, core.h's xce_core_cache_clear_cb_t) ONLY if + * non-NULL, instead of calling cache_clear() unconditionally. + * sim_loop.c never calls xce_core_set_cache_clear_cb() -- a simulation- + * mode xce_sched_core_t's hook therefore stays NULL (xce_sched_core_ + * init()'s memset() default), and core_begin_session_internal() treats + * NULL as "this driver has no cache to clear" and skips the call + * entirely. This test constructs a real xce_sched_sim_driver_t, points + * alt_events_file at a real, open FILE*, deliberately does NOT register + * any cache_clear_cb (mirroring production sim_loop.c exactly), calls the + * REAL xce_core_begin_session() (core_session.c's public entry point), + * and asserts alt_events_file is bit-for-bit unchanged across the call. + * + * ── Link technique ─────────────────────────────────────────────────────── + * Same technique as the sibling test_core_session_cache_clear.c (round 1): + * this fixture's meson.build entry lists core.c/core_clock.c/ + * core_session.c/decision.c/alloc_cache.c as genuinely SEPARATE compiled + * sources rather than white-box #include-ing them, specifically so + * alloc_cache.c's real, strong cache_clear(xce_scheduler_t*) IS present + * in this binary -- if a future regression reintroduced an unconditional + * call to it, this test would still catch it (a white-box #include of + * core_session.c alone, without also linking alloc_cache.c, could not + * have caught round 1's bug at all -- that is exactly why round 1's bug + * was invisible to the existing suite). + * + * kill_sched_* / plugin_unregister / sched_worker_release_host_ctx stubs + * below are identical, for identical reasons, to test_core_session_ + * cache_clear.c's own (see that file's header comment). + */ +#include +#include +#include +#include +#include + +#include + +#include "sched/alloc_cache.h" +#include "sim.h" + +void kill_sched_job(void* arg) { (void)arg; } +void kill_sched_queue(void* arg) { (void)arg; } +void kill_sched_worker(void* arg) { (void)arg; } +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; } +void plugin_unregister(void* arg) { (void)arg; } + +/* sched_worker_release_host_ctx (resource.c) is outside this test's linked + * source set (only core.c/core_clock.c/core_session.c/decision.c/ + * alloc_cache.c, meson.build) -- this fixture never adds a worker, so it + * is never actually invoked at runtime either. */ +void sched_worker_release_host_ctx(xce_sched_worker_t* w) { (void)w; } + +void setUp(void) { return; } +void tearDown(void) { return; } + +/* + * THE Observable this regression guards (Req 7.2, round-2 review + * requirement): a simulation-mode xce_sched_core_t, which never registers + * a cache_clear_cb, must never have any of its sibling xce_sched_sim_ + * driver_t fields touched by xce_core_begin_session() -- even with the + * real, strong alloc_cache.c cache_clear(xce_scheduler_t*) linked into + * the very same binary. + */ +static void test_sim_begin_session_never_corrupts_alt_events_file(void) +{ + xce_sched_sim_driver_t sim; + FILE* f; + int rc; + + memset(&sim, 0, sizeof(sim)); + + rc = xce_sched_core_init(&(sim.core)); + TEST_ASSERT_EQUAL_INT(0, rc); + + /* A real, live FILE* -- exactly what a --sim-alt-events run holds + * here in production. tmpfile() needs no cleanup path/permissions + * setup and is auto-removed on fclose()/process exit. */ + f = tmpfile(); + TEST_ASSERT_NOT_NULL(f); + sim.alt_events_file = f; + + /* Deliberately NOT calling xce_core_set_cache_clear_cb() here -- + * mirrors sim_loop.c exactly, which never registers this hook (Req 5 + * AC 17 "Simulation isolation": sim mode has no cache to clear). This + * is the crux of the regression: before round 2's fix, + * core_begin_session_internal() called cache_clear() unconditionally, + * so linking alloc_cache.c's real, strong cache_clear(xce_scheduler_t*) + * into this binary was enough to corrupt alt_events_file below even + * though nothing here ever asked for a cache to be cleared. */ + rc = xce_core_begin_session(&(sim.core)); + TEST_ASSERT_EQUAL_INT(0, rc); + + TEST_ASSERT_EQUAL_PTR_MESSAGE( + f, sim.alt_events_file, + "xce_core_begin_session() corrupted xce_sched_sim_driver_t's " + "alt_events_file -- core_begin_session_internal() must treat a " + "NULL cache_clear_cb as a true no-op (Req 7.2 round 2), never " + "fall through to alloc_cache.c's xce_scheduler_t*-typed " + "cache_clear() for a simulation-mode core"); + + rc = xce_core_end_session(&(sim.core)); + TEST_ASSERT_EQUAL_INT(0, rc); + + fclose(f); +} + +int main(void) +{ + UNITY_BEGIN(); + RUN_TEST(test_sim_begin_session_never_corrupts_alt_events_file); + return UNITY_END(); +}