diff --git a/src/scheduler/CLAUDE.md b/src/scheduler/CLAUDE.md index a0cd3038..f27834dd 100644 --- a/src/scheduler/CLAUDE.md +++ b/src/scheduler/CLAUDE.md @@ -10,7 +10,7 @@ C-based job scheduling engine with a plugin architecture. Determines job-to-node ``` ## Architecture -- **Core** (`core.c`/`core_events.c`/`core_session.c`/`core_clock.c`) — passive, event-driven `xce_sched_core_t` state machine. Zero controller I/O (no `netio`/`xceapi`/`client`/`delay`/`idgen`); `core_clock.c` is the one sanctioned exception (`clock_gettime`, virtualized two-base+speed clock formula, `xce_core_now()`). `test_core_isolation.c` mechanically enforces this only for `core.c`/`core.h` (source-text scan + `nm(1)` undefined-symbol check on the isolated `sched_core_isolation_lib` archive, meson.build) — `core_events.c`, `core_session.c`, and `core_clock.c` are not covered by that test and must be kept isolation-clean by review. Picker→allocator handoff is a strict `jbuf` cbuf capacity-1 rendezvous — this is the project's determinism invariant, not a tuning knob; never raise it without re-deriving Req 5 AC 7 in `.kiro/specs/scheduler-core/requirements.md`. +- **Core** (`core.c`/`core_events.c`/`core_session.c`/`core_clock.c`) — passive, event-driven `xce_sched_core_t` state machine. Zero controller I/O (no `netio`/`xceapi`/`client`/`delay`/`idgen`); `core_clock.c` is the one sanctioned exception (`clock_gettime`, virtualized two-base+speed clock formula, `xce_core_now()`). `test_core_isolation.c` mechanically enforces this for `core.c`/`core.h`/`core_events.c`/`core_session.c` (source-text scan) plus `core_clock.c` too on the `nm(1)` undefined-symbol layer (all four compiled together into the isolated `sched_core_isolation_lib` archive, meson.build) — `core_clock.c` is deliberately excluded only from the wall-clock text scan, since it owns the one sanctioned `clock_gettime()` call. Picker→allocator handoff is a strict `jbuf` cbuf capacity-1 rendezvous — this is the project's determinism invariant, not a tuning knob; never raise it without re-deriving Req 5 AC 7 in `.kiro/specs/scheduler-core/requirements.md`. - **Picker plugin interface** (`plugin.h`'s `xce_job_picker_iface_t`, `plugins/builtin_fcfs.c`) — job-selection strategies are pluggable; `scheduler.c`'s `sched_job_picker()` branches on whether a plugin still uses the legacy `job_elect_fn`-only shape (`plugins/fcfs.c`, kept as-is for a compat-wrapper exercise, not the live default) vs the native hook interface (`builtin_fcfs`, the registered default). New scheduling policies → add as a picker plugin under `src/plugins/`, do not modify the core loop. - **Drivers** — `driver.c` (normal mode: capnp↔core event translation, decision publishing, deferred admin-request handling) and `sim.c`/`sim_loop.c`/`sim_record.c`/`sim_verify.c` (simulation mode: `.replay` file loading, discrete virtual clock, `--sim-verify`/`--sim-alt-events`/`--sim-output`). Both drive the identical core session/decision-callback protocol — one core, two consumers. - **Replay/recording** — `recording.c`/`recording_events.c`/`recording_session.c` (live capture) and `sim.c`/`sim_loop.c` (simulation-side decode) do the actual capnp encode/decode of the `.replay` binary stream (`src/schema/controller.capnp`); `snapshot.c`/`snapshot_wire.c`/`snapshot_load.c` only populate/consume the plain in-memory `sched_snapshot_data_t` struct that `recording.c`'s `write_snapshot_entry()` then serializes — no capnp or file I/O lives in the snapshot files themselves. Controlled via `OPCode_schedAdmin` (`src/controller/cluster.c`) and `xceadmin scheduler record start|stop|status` / `scheduler dump `. diff --git a/src/scheduler/meson.build b/src/scheduler/meson.build index 6c28b386..a3419a76 100644 --- a/src/scheduler/meson.build +++ b/src/scheduler/meson.build @@ -65,16 +65,29 @@ scheduler = executable('xce_scheduler', install: true) # Task 4.1 (scheduler-core spec, Req 5.1/5.2): isolated compile target for -# core.c alone, consumed by testsuite/test_core_isolation.c to prove at -# build time that core.c references zero symbols from the networking/API -# libraries (netio/xceapi/client/delay/idgen). A static_library() here need -# not LINK against anything for this purpose -- compiling src/core.c to an -# object is enough; the object's own undefined-symbol table (inspected via -# nm(1) in the test) reflects exactly what core.c's code calls, independent -# of which libraries are nominally "dependencies" of this build target. +# core.c AND its core_events.c/core_session.c/core_clock.c siblings (design +# review follow-up: the isolation boundary is a core.c-family-wide property +# -- Req 5 AC 1/AC 2 -- but until this fix the nm(1) undefined-symbol proof +# below only ever mechanically covered core.c itself, leaving the other +# three "kept isolation-clean by review" only, i.e. manually, with nothing +# catching a regression), consumed by testsuite/test_core_isolation.c to +# prove at build time that none of the four reference zero-I/O-boundary +# symbols from the networking/API libraries (netio/xceapi/client/delay/ +# idgen). core_clock.c is the one SANCTIONED exception to the separate +# wall-clock text-scan check (it owns the core family's one legitimate +# clock_gettime() call, Req 5 AC 3) -- that exemption is enforced in +# test_core_isolation.c itself, not here; this archive only proves the +# networking/API isolation boundary, which core_clock.c fully satisfies +# (clock_gettime()/gmtime_r()/tlog_reg_format_func() are not +# networking/API symbols). A static_library() here need not LINK against +# anything for this purpose -- compiling these four files to objects is +# enough; the archive's own undefined-symbol table (inspected via nm(1) in +# the test) reflects exactly what this code calls, independent of which +# libraries are nominally "dependencies" of this build target. sched_core_isolation_lib = static_library( 'sched_core_isolation', - ['src/core.c'], + ['src/core.c', 'src/core_events.c', 'src/core_session.c', + 'src/core_clock.c'], include_directories: incdir, dependencies: scheduler_deps, install: false) diff --git a/src/scheduler/testsuite/meson.build b/src/scheduler/testsuite/meson.build index b7852c25..14a837a2 100644 --- a/src/scheduler/testsuite/meson.build +++ b/src/scheduler/testsuite/meson.build @@ -208,6 +208,12 @@ sched_core_isolation_test = executable( '-DXCE_CORE_C_PATH="' + (meson.current_source_dir() / '..' / 'src' / 'core.c') + '"', + '-DXCE_CORE_EVENTS_C_PATH="' + + (meson.current_source_dir() / '..' / 'src' / 'core_events.c') + + '"', + '-DXCE_CORE_SESSION_C_PATH="' + + (meson.current_source_dir() / '..' / 'src' / 'core_session.c') + + '"', '-DXCE_CORE_LIB_PATH="' + sched_core_isolation_lib.full_path() + '"'], diff --git a/src/scheduler/testsuite/test_core_isolation.c b/src/scheduler/testsuite/test_core_isolation.c index d22f3e74..31cf0a09 100644 --- a/src/scheduler/testsuite/test_core_isolation.c +++ b/src/scheduler/testsuite/test_core_isolation.c @@ -6,39 +6,57 @@ * "no wall-clock time call appears anywhere in the extracted core's * source." * + * Design review follow-up (2026-08-19, scheduler-core retrospective + * review): originally this guard mechanically covered ONLY core.c/core.h, + * leaving core_events.c/core_session.c/core_clock.c -- core.c's own + * 800-line-cap siblings, holding scheduling logic just as boundary- + * sensitive as core.c itself -- "isolation-clean by review" only (manual, + * not mechanical; scheduler/CLAUDE.md's own Architecture section used to + * say so explicitly). A future change to either sibling could silently + * reintroduce a networking/API dependency with nothing to catch it. Both + * layers below now cover core_events.c/core_session.c too. core_clock.c + * joins ONLY the nm(1) layer (2), not the wall-clock text scan: it is the + * one FILE-level sanctioned exception carrying the core family's single + * legitimate clock_gettime() call (Req 5 AC 3, core_clock.c's own header + * comment) -- scanning it for wall-clock text would trivially and + * incorrectly fail. It has no legitimate reason to reference any + * networking/API symbol, so it fully participates in layer 2. + * * Two layers, same genre as test_picker_core_loop_boundary.c / * test_picker_pendreason_factory_audit.c (source-text scans of the real, * current on-disk files, not a frozen snapshot): * - * 1. Source-text scan of core.h/core.c: no #include of client.h, - * netio.h, xceapi.h, delay.h, idgen.h, or xce/cluster.h (the - * "Allowed Dependencies" list in design.md names exactly these as - * forbidden); no call-site substring matching a symbol exported only - * by those libraries; no wall-clock call (time()/clock_gettime()/ - * gettimeofday()) anywhere in core.c. + * 1. Source-text scan of core.h/core.c/core_events.c/core_session.c: no + * #include of client.h, netio.h, xceapi.h, delay.h, idgen.h, or + * xce/cluster.h (the "Allowed Dependencies" list in design.md names + * exactly these as forbidden); no call-site substring matching a + * symbol exported only by those libraries; no wall-clock call + * (time()/clock_gettime()/gettimeofday()) anywhere in core.c, + * core_events.c, or core_session.c (core_clock.c excepted -- see + * above). * * 2. The literal build-time/link-time proof the Observable asks for, - * not achievable by a text scan alone: core.c is compiled in - * isolation into its own static library (meson.build's - * sched_core_isolation_lib, this test's XCE_CORE_LIB_PATH), and this - * test inspects that archive's UNDEFINED SYMBOL TABLE via nm(1) -- - * proving what core.c's *compiled code actually calls*, independent - * of what any header happens to transitively declare (xce/types.h, - * used by literally every file in this directory, itself includes - * netio.h for unrelated reasons -- so a "no forbidden #include" - * check alone would either false-positive on that pre-existing, - * harmless transitive include, or have to special-case it; the nm - * check sidesteps that entirely by checking *usage*, not - * *visibility*). + * not achievable by a text scan alone: core.c/core_events.c/ + * core_session.c/core_clock.c are compiled together in isolation + * into one static library (meson.build's sched_core_isolation_lib, + * this test's XCE_CORE_LIB_PATH), and this test inspects that + * archive's UNDEFINED SYMBOL TABLE via nm(1) -- proving what this + * code's *compiled output actually calls*, independent of what any + * header happens to transitively declare (xce/types.h, used by + * literally every file in this directory, itself includes netio.h + * for unrelated reasons -- so a "no forbidden #include" check alone + * would either false-positive on that pre-existing, harmless + * transitive include, or have to special-case it; the nm check + * sidesteps that entirely by checking *usage*, not *visibility*). * * Why nm on a static_library() rather than the full xce_scheduler * executable: the executable links the whole monolithic libxce_dep, so * every symbol from netio/xceapi/client/delay/idgen would appear in ITS - * undefined-symbol table regardless of whether core.c itself ever calls - * one -- that would prove nothing. Compiling core.c alone into its own - * archive (sched_core_isolation_lib, meson.build) and inspecting THAT - * object's undefined symbols isolates exactly what this one translation - * unit references. + * undefined-symbol table regardless of whether this code itself ever + * calls one -- that would prove nothing. Compiling this file set alone + * into its own archive (sched_core_isolation_lib, meson.build) and + * inspecting THAT archive's undefined symbols isolates exactly what these + * translation units reference. */ #include #include @@ -54,6 +72,14 @@ #error \ "XCE_CORE_C_PATH must be defined by meson.build to core.c's absolute path" #endif +#ifndef XCE_CORE_EVENTS_C_PATH +#error \ + "XCE_CORE_EVENTS_C_PATH must be defined by meson.build to core_events.c's absolute path" +#endif +#ifndef XCE_CORE_SESSION_C_PATH +#error \ + "XCE_CORE_SESSION_C_PATH must be defined by meson.build to core_session.c's absolute path" +#endif #ifndef XCE_CORE_LIB_PATH #error \ "XCE_CORE_LIB_PATH must be defined by meson.build to the absolute path of core.c's isolated static_library() archive" @@ -147,99 +173,155 @@ void tearDown(void) { return; } * mode this whole test exists to avoid). */ static void test_core_source_paths_are_readable(void) { - char* h = read_whole_file(XCE_CORE_H_PATH); - char* c = read_whole_file(XCE_CORE_C_PATH); + char* h = read_whole_file(XCE_CORE_H_PATH); + char* c = read_whole_file(XCE_CORE_C_PATH); + char* ce = read_whole_file(XCE_CORE_EVENTS_C_PATH); + char* cs = read_whole_file(XCE_CORE_SESSION_C_PATH); TEST_ASSERT_NOT_NULL_MESSAGE(h, XCE_CORE_H_PATH); TEST_ASSERT_NOT_NULL_MESSAGE(c, XCE_CORE_C_PATH); + TEST_ASSERT_NOT_NULL_MESSAGE(ce, XCE_CORE_EVENTS_C_PATH); + TEST_ASSERT_NOT_NULL_MESSAGE(cs, XCE_CORE_SESSION_C_PATH); free(h); free(c); + free(ce); + free(cs); } -static void test_core_never_includes_forbidden_headers(void) +/* Scans one file's text for every FORBIDDEN_INCLUDES entry, tagging each + * failure message with `label` so a failure names the actual offending + * file -- shared by test_core_never_includes_forbidden_headers() across + * core.h/core.c/core_events.c/core_session.c. */ +static void assert_no_forbidden_includes(const char* text, const char* label) { - char* h = read_whole_file(XCE_CORE_H_PATH); - char* c = read_whole_file(XCE_CORE_C_PATH); size_t i; - TEST_ASSERT_NOT_NULL(h); - TEST_ASSERT_NOT_NULL(c); - for (i = 0; i < NUM_FORBIDDEN_INCLUDES; i++) { char msg[256]; - snprintf(msg, sizeof(msg), - "core.h directly includes forbidden header '%s'", - FORBIDDEN_INCLUDES[i]); - TEST_ASSERT_NULL_MESSAGE(strstr(h, FORBIDDEN_INCLUDES[i]), msg); - - snprintf(msg, sizeof(msg), - "core.c directly includes forbidden header '%s'", - FORBIDDEN_INCLUDES[i]); - TEST_ASSERT_NULL_MESSAGE(strstr(c, FORBIDDEN_INCLUDES[i]), msg); + snprintf(msg, sizeof(msg), "%s directly includes forbidden header '%s'", + label, FORBIDDEN_INCLUDES[i]); + TEST_ASSERT_NULL_MESSAGE(strstr(text, FORBIDDEN_INCLUDES[i]), msg); } - - free(h); - free(c); } -static void test_core_never_calls_forbidden_symbols(void) +static void test_core_never_includes_forbidden_headers(void) { - char* h = read_whole_file(XCE_CORE_H_PATH); - char* c = read_whole_file(XCE_CORE_C_PATH); - size_t i; + char* h = read_whole_file(XCE_CORE_H_PATH); + char* c = read_whole_file(XCE_CORE_C_PATH); + char* ce = read_whole_file(XCE_CORE_EVENTS_C_PATH); + char* cs = read_whole_file(XCE_CORE_SESSION_C_PATH); TEST_ASSERT_NOT_NULL(h); TEST_ASSERT_NOT_NULL(c); + TEST_ASSERT_NOT_NULL(ce); + TEST_ASSERT_NOT_NULL(cs); + + assert_no_forbidden_includes(h, "core.h"); + assert_no_forbidden_includes(c, "core.c"); + assert_no_forbidden_includes(ce, "core_events.c"); + assert_no_forbidden_includes(cs, "core_session.c"); + + free(h); + free(c); + free(ce); + free(cs); +} + +/* Shared by test_core_never_calls_forbidden_symbols() -- see + * assert_no_forbidden_includes() above for the same per-file-label + * rationale. */ +static void assert_no_forbidden_symbols(const char* text, const char* label) +{ + size_t i; for (i = 0; i < NUM_FORBIDDEN_SYMBOLS; i++) { char msg[256]; snprintf(msg, sizeof(msg), - "core.h references forbidden networking/API symbol '%s'", + "%s references forbidden networking/API symbol '%s'", label, FORBIDDEN_SYMBOLS[i]); - TEST_ASSERT_NULL_MESSAGE(strstr(h, FORBIDDEN_SYMBOLS[i]), msg); - - snprintf(msg, sizeof(msg), - "core.c references forbidden networking/API symbol '%s'", - FORBIDDEN_SYMBOLS[i]); - TEST_ASSERT_NULL_MESSAGE(strstr(c, FORBIDDEN_SYMBOLS[i]), msg); + TEST_ASSERT_NULL_MESSAGE(strstr(text, FORBIDDEN_SYMBOLS[i]), msg); } +} + +static void test_core_never_calls_forbidden_symbols(void) +{ + char* h = read_whole_file(XCE_CORE_H_PATH); + char* c = read_whole_file(XCE_CORE_C_PATH); + char* ce = read_whole_file(XCE_CORE_EVENTS_C_PATH); + char* cs = read_whole_file(XCE_CORE_SESSION_C_PATH); + + TEST_ASSERT_NOT_NULL(h); + TEST_ASSERT_NOT_NULL(c); + TEST_ASSERT_NOT_NULL(ce); + TEST_ASSERT_NOT_NULL(cs); + + assert_no_forbidden_symbols(h, "core.h"); + assert_no_forbidden_symbols(c, "core.c"); + assert_no_forbidden_symbols(ce, "core_events.c"); + assert_no_forbidden_symbols(cs, "core_session.c"); free(h); free(c); + free(ce); + free(cs); } +/* core_clock.c is deliberately NOT scanned here -- it is the one + * sanctioned exception carrying the core family's single legitimate + * clock_gettime() call (Req 5 AC 3); see this file's own header comment. */ static void test_core_never_calls_wallclock_functions(void) { - char* c = read_whole_file(XCE_CORE_C_PATH); + char* c = read_whole_file(XCE_CORE_C_PATH); + char* ce = read_whole_file(XCE_CORE_EVENTS_C_PATH); + char* cs = read_whole_file(XCE_CORE_SESSION_C_PATH); size_t i; TEST_ASSERT_NOT_NULL(c); + TEST_ASSERT_NOT_NULL(ce); + TEST_ASSERT_NOT_NULL(cs); for (i = 0; i < NUM_FORBIDDEN_WALLCLOCK; i++) { char msg[256]; snprintf(msg, sizeof(msg), "core.c calls wall-clock function '%s' -- the core must " - "access time exclusively through a future xce_core_now() " + "access time exclusively through xce_core_now() " "(scheduler-core spec, task 4.5)", FORBIDDEN_WALLCLOCK[i]); TEST_ASSERT_NULL_MESSAGE(strstr(c, FORBIDDEN_WALLCLOCK[i]), msg); + + snprintf(msg, sizeof(msg), + "core_events.c calls wall-clock function '%s' -- the core " + "must access time exclusively through xce_core_now() " + "(scheduler-core spec, task 4.5)", + FORBIDDEN_WALLCLOCK[i]); + TEST_ASSERT_NULL_MESSAGE(strstr(ce, FORBIDDEN_WALLCLOCK[i]), msg); + + snprintf(msg, sizeof(msg), + "core_session.c calls wall-clock function '%s' -- the core " + "must access time exclusively through xce_core_now() " + "(scheduler-core spec, task 4.5)", + FORBIDDEN_WALLCLOCK[i]); + TEST_ASSERT_NULL_MESSAGE(strstr(cs, FORBIDDEN_WALLCLOCK[i]), msg); } free(c); + free(ce); + free(cs); } /* * test_core_object_has_no_forbidden_undefined_symbols - the literal - * build-time/link-time proof: core.c, compiled in isolation into + * build-time/link-time proof: core.c/core_events.c/core_session.c/ + * core_clock.c, compiled together in isolation into * sched_core_isolation_lib (meson.build), must not reference (as an - * undefined external symbol in the compiled object) anything matching the + * undefined external symbol in the compiled archive) anything matching the * forbidden prefixes above. This is independent of what any header - * transitively declares -- it reflects only what core.c's compiled code - * actually calls. + * transitively declares -- it reflects only what this file set's compiled + * code actually calls. */ static void test_core_object_has_no_forbidden_undefined_symbols(void) { @@ -279,9 +361,10 @@ static void test_core_object_has_no_forbidden_undefined_symbols(void) char msg[768]; snprintf(msg, sizeof(msg), - "core.c's compiled object has an undefined " + "the core isolation archive (core.c/core_events.c/" + "core_session.c/core_clock.c) has an undefined " "reference to '%s', matching forbidden " - "networking/API prefix '%s' -- core.c must never " + "networking/API prefix '%s' -- the core must never " "link networking/API library symbols (Req 5.1/" "5.2)", symbol, FORBIDDEN_SYMBOLS[i]);