From e9b5c3f58916ab4ceb21e86f03900f42b6747bbc Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Mon, 3 Aug 2026 14:09:42 +0000 Subject: [PATCH] feat(scheduler-core): pin jbuf capacity + record kvmgr determinism audit (task 4.2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hardcodes jbuf's cbuf capacity to the literal 1 in xce_sched_core_init() (no parameter path, no local variable to drift), backed by a regression test. Audits every kvmgr iteration reachable from the scheduling loop (jobs/queues/users/projects): only jobs is ever iterated, via xzarray's deterministic ascending-slot walk with no process-random seeding — findings recorded durably as a comment above xce_sched_core_init(). core->workers iteration-order stability is flagged as an open follow-up (scheduler-allocation's territory), not this task's scope. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_019dw5WnN3DXStaEaPYfACU1 --- .kiro/specs/scheduler-core/tasks.md | 4 +- src/scheduler/src/core.c | 65 ++++- src/scheduler/testsuite/meson.build | 25 ++ .../testsuite/test_core_jbuf_capacity_pin.c | 275 ++++++++++++++++++ 4 files changed, 366 insertions(+), 3 deletions(-) create mode 100644 src/scheduler/testsuite/test_core_jbuf_capacity_pin.c diff --git a/.kiro/specs/scheduler-core/tasks.md b/.kiro/specs/scheduler-core/tasks.md index 6f59f1d7..1fa2d290 100644 --- a/.kiro/specs/scheduler-core/tasks.md +++ b/.kiro/specs/scheduler-core/tasks.md @@ -91,11 +91,12 @@ - _Depends: 3_ - _Verified: 2026-08-03 — new xce_sched_core_t (core.h/core.c) holds all scheduling state, xce_scheduler_t shrank to {core; apictx; cluster_cfg}; nm-based isolation test (test_core_isolation.c) proves zero netio/xcecli/idgen/delaymgr symbols; 14/14 scheduler suite passing; independent review APPROVED (own fresh build/test/nm run + reproduced RED)_ -- [ ] 4.2 Pin the determinism invariants +- [x] 4.2 Pin the determinism invariants - Hardcode the picker-to-allocator handoff channel's capacity to 1 inside core initialization — never expose it as a parameter - Audit every hash-table/key-value-store iteration reachable from the scheduling loop (jobs, queues, users, projects) and confirm each produces order deterministic in (keys, insert/delete sequence) with no process-random seeding; fix any site found unstable - Observable: core initialization rejects (or simply has no code path to accept) a caller-supplied handoff-channel capacity; the iteration-order audit's findings are recorded and any fix is covered by a targeted test - _Requirements: 5.7_ + - _Verified: 2026-08-03 — jbuf capacity hardcoded to literal 1 (core.c), regression test test_core_jbuf_capacity_pin.c; kvmgr audit found jobs/queues/users/projects already fully order-stable (xzarray ascending-slot walk, no process-random seeding), no fix needed, findings recorded durably as a comment in core.c above xce_sched_core_init(); 15/15 scheduler suite passing; two-round review (round 1 REJECTED on missing durable record only, round 2 APPROVED after fix); core->workers flagged as an open follow-up (see Implementation Notes), not this task's scope_ - _Depends: 4.1_ - [ ] 4.3 Implement the core event interface and batch event push @@ -361,3 +362,4 @@ - Task 2.3's review found a real concurrency gap for task 2.4 to close: once builtin_fcfs is wired live, `next_job()` (picker thread) and `on_dispatch`/`on_pend` (allocator thread) can race on the same unsynchronized `fcfs_plugin_data_t` private state — `cbuf_get()` unblocks the picker's next `cbuf_put()` before the allocator finishes processing the dequeued job (jbuf capacity is 1), so the picker can re-enter `next_job()` while the allocator is still mutating `priv->queues` via `on_dispatch`/`on_pend`. Today's production path never hits this because the allocator thread calls a different plugin category (`limit.c`'s resource-limit hook), never `fcfs.c`'s own private state. Task 2.4 must add a mutex guarding `fcfs_plugin_data_t` across all five picker hooks (or an equivalent handoff redesign) before registering builtin_fcfs as the live default. - Task 2.4 closed the above race with `fcfs_plugin_data_t.lock` (mylock_t), taken by exactly 7 hooks (`on_queue_update`, `on_job_push`, `on_job_state_change`, `on_job_update`, `begin_session`, `next_job`, `on_dispatch`) — `on_pend`/`end_session`/`init`/`fini` correctly excluded (they never touch `priv->queues`/`cursor`/`job_cursor`), a verified refinement of this note's original "all five hooks" estimate, not a gap. `sched_job_picker()` now branches `job_elect_fn != NULL` (legacy plugins like `xce_sched_fcfs`, delegates unchanged to their own self-contained loop) vs NULL (native `xce_job_picker_iface_t` pickers, runs a new generic per-session loop) — this split exists because legacy `job_elect_fn` implementations (e.g. `fcfs_elect_job`) already contain their own internal `cbuf_get(&wbuf)`, so a single uniform outer loop would double-consume the wbuf signal for legacy pickers. `sched_allocator()` now calls `picker_iface->on_dispatch()`/`on_pend()` after each allocation attempt (Req 1 AC 4), which was previously called for no picker at all — required for builtin_fcfs to actually remove dispatched jobs from its private state once live. - Two follow-ups surfaced by task 2.4's review, not yet fixed (outside its file boundary): (1) `main.c` discards `sched_init()`'s return value, so a bad `job_pick_plugin` config fails `sched_init()` correctly but the process doesn't yet halt on it — needs a fast follow-up before relying on "fails startup" in production. (2) `pthread_testcancel()` in `builtin_fcfs_next_job()` is only at the loop entry/exit, not scattered through the queue/job walk (the walk holds `fcfs_plugin_data_t.lock` throughout, and this codebase has no `pthread_cleanup_push` precedent to safely unlock on mid-walk cancellation) — behaviorally inert today since `sched_queue_is_ready`/`sched_job_is_ready` are `return 1` stubs, but becomes a real cancellation-latency gap once Phase 2 implements real filtering. +- Task 4.2's Req 5 AC 7 kvmgr iteration-order audit (scope: jobs/queues/users/projects, per design.md's bounded-scope note) found no unstable site — recorded durably as a comment in `core.c` above `xce_sched_core_init()`. One open gap surfaced but explicitly out of this task's scope: `core->workers` is iterated order-sensitively by `sched_collect_workers()` → `seqjob_allocator.c`'s first-fit `sched_try_alloc_on_hosts()`, and design.md itself says the full AC 7 guarantee must close "before Phase 3 simulation work depends on it" — needs a follow-up audit/fix (or an explicit documented exemption) before Phase 3 (tasks 7+) starts relying on scheduling determinism across worker selection. diff --git a/src/scheduler/src/core.c b/src/scheduler/src/core.c index 16dd34da..22fee7fe 100644 --- a/src/scheduler/src/core.c +++ b/src/scheduler/src/core.c @@ -32,10 +32,56 @@ * pointers themselves) lives in xce_sched_core_t. */ +/* + * Req 5 AC 7 (scheduler-core spec, task 4.2) -- kvmgr iteration-order audit, + * scope: jobs/queues/users/projects (design.md's "bounded scope" Risks note; + * core->workers is explicitly out of this audit's scope -- it belongs to + * scheduler-allocation's host-selection path, not this spec's scheduling + * loop). Recorded here, durably, rather than only in a task status report: + * + * - queues_params/users_params/projects_params (below) back kvmgrs that are + * NEVER iterated anywhere in src/scheduler -- only point key_add/key_find + * lookups exist on core->queues/core->users/core->projects. There is + * nothing to stabilize for these three: no iteration site, no order to + * destabilize. + * - jobs_params (below) backs the one kvmgr of the four that IS iterated, + * at two call sites reachable from the scheduling loop: scheduler.c's + * sched_account_jobs() (every session, from sched_begin_session()) and + * sched_end_session()'s GC live-marking pass. Both go through + * xkvmgr_type's .iter interface (src/lib/kvmgr.c, xkvmgr_make_iter/ + * xkvmgr_iter_next), which does NOT walk this struct's hash-bucket chain + * at all -- it delegates to the underlying xzarray_type instance + * (src/lib/zarray.c, xzarray_iter_next), a flat array walked in ascending + * slot-index order, itself a deterministic function of insert/delete + * history via a freed-slot reuse hint (xzarray_clear/xzarray_store) -- + * no randomness anywhere in that path. + * - The hash-bucket path (src/lib/kv.c) is used only for O(1) key->index + * lookup (key_add/key_find), never for iteration. It is independently + * deterministic too: nslots is fixed at kvmgr creation with no + * resize/rehash code path; hashing is djb2 (hash_string, fixed seed + * 5381) or a fixed Bob-Jenkins-style bit-mixer (hash_int_key), neither + * taking any random/ASLR/pointer/time input; insertion is head-of-list, + * deletion unlinks in place -- both pure functions of insert/delete + * order. A grep of kv.c/kvmgr.c/zarray.c/utils.c for rand(/srand(/ + * arc4random turned up nothing in this path (the sole getpid() hit is an + * unrelated tlog log-format helper). + * - The picker's actual scheduling-OUTPUT order doesn't come from kvmgr + * iteration at all: builtin_fcfs/fcfs.c maintain their own private ordq + * structures with explicit comparators (fcfs_queue_cmp/fcfs_job_cmp -- + * priority desc, then queue_time asc), a fully specified deterministic + * total order independent of any kvmgr/hash mechanism. + * + * Conclusion: no unstable site found among jobs/queues/users/projects; no + * fix required. (core->workers iteration IS order-sensitive for + * scheduler-allocation's first-fit host selection -- seqjob_allocator.c's + * sched_collect_workers()/sched_try_alloc_on_hosts() -- and remains an open + * item to close before Phase 3 simulation work depends on AC 7's full + * guarantee, per design.md's own scoping note; tracked as a follow-up, not + * this task's responsibility.) + */ int xce_sched_core_init(xce_sched_core_t* core) { int ndecisions = 5; /* FIXME: make this configurable */ - int njobs = 1; /* FIXME: make this configurable */ kvmgr_params_t plugins_params = { .key_type = KVMGR_KEY_STRING, .nslots = 16, @@ -85,7 +131,22 @@ int xce_sched_core_init(xce_sched_core_t* core) mylock_init(&(core->lock)); cbuf_init(&(core->wbuf), 1, sizeof(int)); - cbuf_init(&(core->jbuf), njobs, sizeof(uint64_t)); + + /* Req 5 AC 7 (scheduler-core spec, task 4.2): jbuf capacity is a + * determinism invariant, not a tuning knob -- it is the rendezvous + * between the picker and allocator threads (the picker blocks on + * cbuf_put() until the allocator drains the current jobid), which is + * exactly what makes jobs flow through the pipeline one at a time in + * fully determined order. Raising this above 1 turns the rendezvous + * into a buffer, letting the picker enqueue faster than the allocator + * drains, which would make allocator-observed job order depend on + * scheduling/timing rather than on the event sequence alone. + * Hardcoded to the literal 1 here per requirements.md's explicit + * prohibition on exposing this as a driver- or config-supplied + * parameter -- do not reintroduce a local variable or any other + * indirection for this value without first re-deriving the + * determinism argument in AC 7. */ + cbuf_init(&(core->jbuf), 1, sizeof(uint64_t)); cbuf_init(&(core->decisions), ndecisions, sizeof(xce_decision_t*)); core->resmgr = makeInstance(&xkvmgr_type, &resmgr_params); diff --git a/src/scheduler/testsuite/meson.build b/src/scheduler/testsuite/meson.build index 849c307d..4109c642 100644 --- a/src/scheduler/testsuite/meson.build +++ b/src/scheduler/testsuite/meson.build @@ -203,6 +203,28 @@ sched_core_isolation_test = executable( + '"'], install: false) +# Regression guard for task 4.2's jbuf-capacity half (scheduler-core spec, +# Req 5 AC 7): "cbuf capacity=1 serialization ... this capacity is a +# determinism invariant, not a tuning knob." Same white-box #include +# pattern as test_picker_config_fields.c (pulls the real +# xce_sched_core_init()/xce_sched_core_fini() from ../src/core.c into this +# TU) PLUS the source-text-scan genre used by test_picker_core_loop_boundary_test/ +# test_core_isolation_test above -- a real runtime call proving +# core.jbuf.ncap == 1, and a scan of core.c's actual on-disk text proving +# the cbuf_init() capacity argument is the literal 1 (never a variable), +# that the former `njobs` local is gone, and that the separate/unrelated +# `ndecisions` FIXME was left untouched. XCE_CORE_C_PATH reuses the same +# absolute-path constant as sched_core_isolation_test above. +sched_core_jbuf_capacity_pin_test = executable( + 'sched_core_jbuf_capacity_pin_test', + ['test_core_jbuf_capacity_pin.c'], + include_directories:[incdir], + dependencies: [scheduler_test_deps], + c_args: ['-DXCE_CORE_C_PATH="' + + (meson.current_source_dir() / '..' / 'src' / 'core.c') + + '"'], + install: false) + # Register as meson tests so `meson test --suite scheduler` runs them all. # detect_leaks=0 matches the suite convention (fixtures are not torn down). test('scheduler test', scheduler_test, suite: 'scheduler', @@ -236,3 +258,6 @@ test('scheduler pendreason factory audit test', test('scheduler core isolation test', sched_core_isolation_test, suite: 'scheduler', env: ['ASAN_OPTIONS=detect_leaks=0']) +test('scheduler core jbuf capacity pin test', + sched_core_jbuf_capacity_pin_test, suite: 'scheduler', + env: ['ASAN_OPTIONS=detect_leaks=0']) diff --git a/src/scheduler/testsuite/test_core_jbuf_capacity_pin.c b/src/scheduler/testsuite/test_core_jbuf_capacity_pin.c new file mode 100644 index 00000000..fbba0bef --- /dev/null +++ b/src/scheduler/testsuite/test_core_jbuf_capacity_pin.c @@ -0,0 +1,275 @@ +/* + * test_core_jbuf_capacity_pin.c - regression guard for the first half of + * task 4.2 (scheduler-core spec, Req 5 AC 7, "cbuf capacity=1 + * serialization"): "Phase 2 shall either hardcode this capacity to 1 + * inside xce_sched_core_init() or leave the njobs FIXME untouched; it + * shall not expose jbuf capacity as a driver- or config-supplied + * parameter." + * + * Before this task, core.c's xce_sched_core_init() read: + * int njobs = 1; -- FIXME: make this configurable + * ... + * cbuf_init(&(core->jbuf), njobs, sizeof(uint64_t)); + * -- a local variable, not a hardcoded literal. The value was already 1, + * so a purely behavioral check (does core.jbuf end up with capacity 1?) + * would already pass before this task and prove nothing about whether the + * capacity is *structurally* pinned or just happens to default to 1 + * today. This task's Observable is about the latter: the value must be a + * literal that cannot be fed by any parameter, config field, or variable + * without editing this exact line and re-deriving the determinism + * argument in AC 7. + * + * Three checks, two genres (same combination test_core_isolation.c uses): + * + * 1. test_jbuf_capacity_is_runtime_one - the literal behavioral proof: + * xce_sched_core_init() is pulled into this TU for real (white-box + * #include, same convention as test_picker_config_fields.c) and + * actually invoked; core.jbuf.ncap (mylock.h's cbuf_t.ncap field) must + * read back as 1. This already passed before this task (the value was + * already 1) -- included so a future change to the actual runtime + * capacity is caught even if it doesn't touch this exact source line + * (e.g. a change to cbuf_init() itself, or to mylock.h's cbuf_t + * layout). + * + * 2. test_jbuf_capacity_is_hardcoded_literal_not_a_variable - RED before + * this task (core.c's real on-disk text had `cbuf_init(&(core->jbuf), + * njobs, sizeof(uint64_t))`, never the literal-1 call text asserted + * here), GREEN after: reads core.c's actual on-disk source at test run + * time (XCE_CORE_C_PATH, baked in by meson.build, same technique as + * test_core_isolation.c / test_picker_core_loop_boundary.c) and + * requires the exact hardcoded call text to appear verbatim in + * xce_sched_core_init()'s body. + * + * 3. test_njobs_variable_removed_from_core_init - the mirror-image RED/ + * GREEN check: before this task, "njobs" appeared in + * xce_sched_core_init()'s body (the local variable); this test asserts + * it no longer appears at all, catching any future reintroduction of + * an indirection (variable, parameter, config field) between the + * literal 1 and the cbuf_init() call -- exactly the regression this + * task's Observable rules out ("no code path to accept a + * caller-supplied handoff-channel capacity"). + * + * A fourth check guards scope discipline: this task explicitly must not + * touch the separate, unrelated `ndecisions` FIXME (a different tuning + * knob, out of Req 5 AC 7's boundary) -- test_ndecisions_fixme_left_untouched + * asserts that line is byte-for-byte unchanged. + */ +#include +#include +#include + +#include + +#include "kvmgr.h" +#include "scheduler.h" +#include "xstring.h" + +#ifndef XCE_CORE_C_PATH +#error \ + "XCE_CORE_C_PATH must be defined by meson.build to core.c's absolute path" +#endif + +/* Pull the real xce_sched_core_init()/xce_sched_core_fini() under test into + * this TU (white-box #include, same convention as + * test_picker_config_fields.c's `#include "../src/core.c"`). */ +#include "../src/core.c" + +/* Stubs for scheduler-internal externs core.c references (as kvmgr/zarray + * datakiller callback addresses) but which are defined in schedobj.c, not + * linked into this TU -- identical rationale to test_picker_config_fields.c's + * plugin_unregister stub: xce_sched_core_init() only ever takes these + * functions' addresses (kvmgr_params_t.datakiller / zarray_item_killer_t), + * it never calls them in the path this test exercises (nothing is ever + * added to core.jobs/queues/workers/users/projects/plugins here), so a + * resolvable no-op is all the linker needs. */ +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; } + +void setUp(void) { return; } +void tearDown(void) { return; } + +/* ---------------------------------------------------------------------- */ +/* Behavioral check: real xce_sched_core_init() produces ncap == 1 */ +/* ---------------------------------------------------------------------- */ + +static void test_jbuf_capacity_is_runtime_one(void) +{ + xce_sched_core_t core; + int rc; + + rc = xce_sched_core_init(&core); + TEST_ASSERT_EQUAL_INT(0, rc); + + TEST_ASSERT_EQUAL_INT(1, core.jbuf.ncap); + + /* Deliberately no xce_sched_core_fini(&core) call here: fini() + * unconditionally dataq_fini()s core->pend_updates[0]/[1], which are + * populated by scheduler.c's sched_init() (calloc + dataq_init), never + * by xce_sched_core_init() itself (see core.c's file header comment) -- + * calling fini() on a core built by this fixture alone would + * dereference the NULL those fields are left at by init()'s memset(). + * Matches this directory's established "fixtures aren't torn down" + * convention (see test_picker_core_loop_boundary.c's header comment). */ +} + +/* ---------------------------------------------------------------------- */ +/* Source-text checks: the value is a literal, not fed by a variable */ +/* ---------------------------------------------------------------------- */ + +static char* read_whole_file(const char* path) +{ + FILE* f = fopen(path, "rb"); + if (f == NULL) { + return NULL; + } + + if (fseek(f, 0, SEEK_END) != 0) { + fclose(f); + return NULL; + } + + long len = ftell(f); + if (len < 0) { + fclose(f); + return NULL; + } + rewind(f); + + char* buf = (char*)malloc((size_t)len + 1); + if (buf == NULL) { + fclose(f); + return NULL; + } + + size_t nread = fread(buf, 1, (size_t)len, f); + fclose(f); + buf[nread] = '\0'; + + return buf; +} + +/* Anchor is the literal function signature text, unique to the real + * definition (same technique as test_picker_core_loop_boundary.c's + * FUNC_ANCHOR). */ +static const char* const FUNC_ANCHOR + = "int xce_sched_core_init(xce_sched_core_t* core)"; + +/* + * extract_function_body - same brace-depth-counting extraction as + * test_picker_core_loop_boundary.c's helper of the same name. + */ +static char* extract_function_body(const char* text, const char* anchor) +{ + const char* sig = strstr(text, anchor); + if (sig == NULL) { + return NULL; + } + + const char* brace = strchr(sig, '{'); + if (brace == NULL) { + return NULL; + } + + int depth = 0; + const char* p = brace; + do { + if (*p == '{') { + depth++; + } else if (*p == '}') { + depth--; + } + p++; + } while ((depth > 0) && (*p != '\0')); + + if (depth != 0) { + return NULL; /* unbalanced -- EOF reached mid-function */ + } + + size_t len = (size_t)(p - brace); + char* body = (char*)malloc(len + 1); + if (body == NULL) { + return NULL; + } + memcpy(body, brace, len); + body[len] = '\0'; + + return body; +} + +static char* load_core_init_body(void) +{ + char* file_text = read_whole_file(XCE_CORE_C_PATH); + TEST_ASSERT_NOT_NULL_MESSAGE(file_text, + "could not read core.c at XCE_CORE_C_PATH"); + + char* body = extract_function_body(file_text, FUNC_ANCHOR); + free(file_text); + + TEST_ASSERT_NOT_NULL_MESSAGE( + body, "could not locate/extract xce_sched_core_init()'s body -- has " + "its signature changed? update FUNC_ANCHOR in this test if so"); + + return body; +} + +/* RED before task 4.2 (real text was `cbuf_init(&(core->jbuf), njobs, + * sizeof(uint64_t))`), GREEN after. */ +static void test_jbuf_capacity_is_hardcoded_literal_not_a_variable(void) +{ + char* body = load_core_init_body(); + + TEST_ASSERT_NOT_NULL_MESSAGE( + strstr(body, "cbuf_init(&(core->jbuf), 1, sizeof(uint64_t))"), + "xce_sched_core_init() must hardcode jbuf's cbuf_init() capacity " + "argument to the literal 1 -- Req 5 AC 7 forbids feeding it from a " + "variable, parameter, or config field"); + + free(body); +} + +/* RED before task 4.2 (the `njobs` local was present), GREEN after -- + * catches any future reintroduction of an indirection between the literal + * 1 and the cbuf_init() call. */ +static void test_njobs_variable_removed_from_core_init(void) +{ + char* body = load_core_init_body(); + + TEST_ASSERT_NULL_MESSAGE( + strstr(body, "njobs"), + "xce_sched_core_init() must not declare/reference an `njobs` (or " + "similarly named) variable -- jbuf capacity is a determinism " + "invariant (Req 5 AC 7), pinned to a literal 1, never a variable"); + + free(body); +} + +/* Scope guard: this task must not touch the separate, unrelated + * `ndecisions` FIXME (a different cbuf's capacity, out of Req 5 AC 7's + * boundary). */ +static void test_ndecisions_fixme_left_untouched(void) +{ + char* body = load_core_init_body(); + + TEST_ASSERT_NOT_NULL_MESSAGE( + strstr(body, "int ndecisions = 5; /* FIXME: make this configurable */"), + "the unrelated `ndecisions` FIXME (decisions cbuf capacity) must be " + "left exactly as-is -- it is a separate tuning knob, not covered by " + "Req 5 AC 7's jbuf pin"); + + free(body); +} + +int main(void) +{ + UNITY_BEGIN(); + RUN_TEST(test_jbuf_capacity_is_runtime_one); + RUN_TEST(test_jbuf_capacity_is_hardcoded_literal_not_a_variable); + RUN_TEST(test_njobs_variable_removed_from_core_init); + RUN_TEST(test_ndecisions_fixme_left_untouched); + return UNITY_END(); +}