diff --git a/.kiro/specs/scheduler-core/tasks.md b/.kiro/specs/scheduler-core/tasks.md index c352929f..753ed854 100644 --- a/.kiro/specs/scheduler-core/tasks.md +++ b/.kiro/specs/scheduler-core/tasks.md @@ -66,7 +66,7 @@ - _Requirements: 1.1, 1.7_ - _Depends: 2.4_ -- [ ] 2.6 Picker regression tests and data-sharing invariant checks +- [x] 2.6 Picker regression tests and data-sharing invariant checks - Assert `builtin_fcfs`'s `next_job()` ordering matches today's pre-change FCFS ordering exactly, given identical queue/job state - Confirm the existing scheduler unit test suite passes unmodified except at the specific assertions that exercised the removed hardcoded string and the decision-publishing path this feature replaces - Verify resreq objects are never mutated in place by any touched code path, all pend-reason allocation goes through the existing factory function (no direct `calloc` of the reason struct), comparisons use pointer equality as a fast path before deep comparison, and the existing per-session select-expression memoization is untouched diff --git a/src/scheduler/testsuite/meson.build b/src/scheduler/testsuite/meson.build index d73c019e..5971854d 100644 --- a/src/scheduler/testsuite/meson.build +++ b/src/scheduler/testsuite/meson.build @@ -155,6 +155,26 @@ sched_picker_core_loop_boundary_test = executable( + '"'], install: false) +# Regression guard for Req 6 AC 2 (scheduler-core spec, task 2.6): "All pend +# reason allocation shall go through make_pendreason() (lib/preason.c). +# Callers never calloc(sizeof(xce_pendreason_t)) directly." Same genre as +# sched_picker_core_loop_boundary_test above (reads real on-disk source at +# test run time rather than linking the huge dependency surface a live +# scheduler pulls in), but scans every non-testsuite .c/.h file under +# src/scheduler for the forbidden pattern rather than one function's body. +# XCE_SCHEDULER_ROOT_PATH bakes in src/scheduler's absolute path; +# the scanned file list itself lives in the test source (see that file's +# header comment for why a fixed list, not a directory walk). +sched_pendreason_factory_audit_test = executable( + 'sched_pendreason_factory_audit_test', + ['test_picker_pendreason_factory_audit.c'], + include_directories:[incdir], + dependencies: [scheduler_test_deps], + c_args: ['-DXCE_SCHEDULER_ROOT_PATH="' + + (meson.current_source_dir() / '..') + + '"'], + 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', @@ -182,3 +202,6 @@ test('scheduler picker wiring test', sched_picker_wiring_test, suite: 'scheduler test('scheduler picker core loop boundary test', sched_picker_core_loop_boundary_test, suite: 'scheduler', env: ['ASAN_OPTIONS=detect_leaks=0']) +test('scheduler pendreason factory audit test', + sched_pendreason_factory_audit_test, suite: 'scheduler', + env: ['ASAN_OPTIONS=detect_leaks=0']) diff --git a/src/scheduler/testsuite/test_picker_pendreason_factory_audit.c b/src/scheduler/testsuite/test_picker_pendreason_factory_audit.c new file mode 100644 index 00000000..20d991c6 --- /dev/null +++ b/src/scheduler/testsuite/test_picker_pendreason_factory_audit.c @@ -0,0 +1,239 @@ +/* + * test_picker_pendreason_factory_audit.c - regression guard for Req 6 AC 2 + * (scheduler-core spec, task 2.6): "All pend reason allocation shall go + * through make_pendreason() (the single factory in lib/preason.c). Callers + * never calloc(sizeof(xce_pendreason_t)) directly." + * + * Task 2.6 is primarily an audit, not new behavior. A one-off `grep -rn` + * during that audit confirmed the invariant currently holds (the only + * calloc() constructing an xce_pendreason_t anywhere in src/ is + * lib/preason.c:164, inside make_pendreason() itself, which lives outside + * src/scheduler entirely). This file turns that one-off finding into a + * durable, re-runnable check per the task's Observable ("a grep-based or + * code-review check confirms no new calloc(sizeof(...pendreason...)) call + * site was introduced") -- a future task that adds a picker, allocator, or + * handler path under src/scheduler and forgets make_pendreason() fails this + * test immediately instead of relying on someone re-running the grep by + * hand. + * + * Why a same-line check, not whole-file substring co-occurrence: verified + * during audit that src/decision.c and src/scheduler.c each contain BOTH + * "calloc(" and "pendreason_t" somewhere in the file, but never on the same + * line (decision.c:6 declares a `xce_pendreason_t* reason` parameter, + * decision.c:10/24 calloc a *different* struct, `xce_decision_t`; + * scheduler.c:350 reads an existing `job->schedobj->reason` pointer, + * scheduler.c's five calloc() calls all construct other struct types). A + * whole-file check would false-positive on both files. Flagging only a + * line that contains both substrings together is the same signal a human + * reviewer eyeballing `grep -n calloc` output would use, and matches every + * real violation shape this Observable cares about: `(xce_pendreason_t*) + * calloc(...)` or `calloc(1, sizeof(xce_pendreason_t))` collapse the type + * name and the allocation call onto one line by construction. + * + * Why a fixed file list, not a directory walk: mirrors + * test_picker_core_loop_boundary.c's own acknowledged limitation (a single + * baked-in function-signature anchor, not a fully generic scan) rather than + * adding a recursive-directory-walk dependency (nftw/opendir) for a + * 14-file, rarely-changing tree. SCANNED_RELATIVE_PATHS below is every + * non-testsuite .c/.h file under src/scheduler as of task 2.6 (verified via + * `find src/scheduler -name '*.c' -o -name '*.h' | grep -v testsuite`) -- + * add new files here as they're created, the same maintenance burden + * test_picker_core_loop_boundary.c's FUNC_ANCHOR already carries. + * + * make_pendreason() itself is never allowlisted here because it lives in + * lib/preason.c, outside XCE_SCHEDULER_ROOT_PATH entirely -- the scanned + * tree structurally cannot see the one legitimate call site. + */ +#include +#include +#include + +#include + +#ifndef XCE_SCHEDULER_ROOT_PATH +#error \ + "XCE_SCHEDULER_ROOT_PATH must be defined by meson.build to the absolute path of src/scheduler" +#endif + +static const char* const SCANNED_RELATIVE_PATHS[] = { + "include/decision.h", + "include/plugin.h", + "include/scheduler.h", + "include/sched/types.h", + "src/decision.c", + "src/handler.c", + "src/main.c", + "src/plugin.c", + "src/plugins/builtin_fcfs.c", + "src/plugins/fcfs.c", + "src/plugins/limit.c", + "src/plugins/parjob_allocator.c", + "src/plugins/seqjob_allocator.c", + "src/resource.c", + "src/schedobj.c", + "src/scheduler.c", +}; +#define NUM_SCANNED \ + (sizeof(SCANNED_RELATIVE_PATHS) / sizeof(SCANNED_RELATIVE_PATHS[0])) + +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; +} + +/* The detector: a single source line that mentions both "calloc(" and + * "pendreason_t" is exactly the shape of a caller constructing an + * xce_pendreason_t (or a pointer to one) via direct calloc() rather than + * make_pendreason(). Operates on one already-NUL-terminated line (no + * embedded '\n'), never on whole-file text -- see file header for why. */ +static int line_has_violation(const char* line) +{ + return (strstr(line, "calloc(") != NULL) + && (strstr(line, "pendreason_t") != NULL); +} + +/* Scans `path` line by line. Returns 1 if some line violates (and copies + * that line into violating_line_out for the failure message), 0 if clean, + * -1 if the file could not be read at all (caller must treat -1 as a hard + * failure, not "skip" -- an unreadable path means this whole test is + * silently checking nothing). */ +static int scan_file_for_violation(const char* path, char* violating_line_out, + size_t out_sz) +{ + char* text = read_whole_file(path); + char* saveptr = NULL; + char* line; + int found = 0; + + if (text == NULL) { + return -1; + } + + line = strtok_r(text, "\n", &saveptr); + while (line != NULL) { + if (line_has_violation(line)) { + found = 1; + if ((violating_line_out != NULL) && (out_sz > 0)) { + snprintf(violating_line_out, out_sz, "%s", line); + } + break; + } + line = strtok_r(NULL, "\n", &saveptr); + } + + free(text); + return found; +} + +void setUp(void) { return; } +void tearDown(void) { return; } + +/* Sanity check for the audit itself: every baked-in path must actually + * open. Without this, a stale/typo'd relative path would make + * scan_file_for_violation() silently treat a missing file as "clean" (0 + * lines scanned, 0 violations found) -- the exact false-confidence failure + * mode this whole test exists to avoid for the real invariant. */ +static void test_all_scanned_paths_are_readable(void) +{ + size_t i; + + for (i = 0; i < NUM_SCANNED; i++) { + char path[4096]; + char* text; + + snprintf(path, sizeof(path), "%s/%s", XCE_SCHEDULER_ROOT_PATH, + SCANNED_RELATIVE_PATHS[i]); + + text = read_whole_file(path); + TEST_ASSERT_NOT_NULL_MESSAGE(text, path); + free(text); + } +} + +/* Proves the detector can actually fire, on synthetic text -- not just that + * it stays quiet on today's real files. Without this, a typo in + * line_has_violation() (e.g. a misspelled substring) could make every real + * scan vacuously pass forever. Mirrors the spirit of + * test_picker_core_loop_boundary.c's manual RED demonstration, but as a + * committed, permanently-runnable test of the detector function itself + * rather than an uncommitted one-time manual edit. */ +static void test_detector_fires_on_same_line_and_not_on_split_lines(void) +{ + TEST_ASSERT_TRUE(line_has_violation( + " r = (xce_pendreason_t*)calloc(1, sizeof(*r));")); + TEST_ASSERT_TRUE( + line_has_violation(" calloc(1, sizeof(xce_pendreason_t));")); + + /* same two substrings, but never on one line -- must NOT fire (the + * exact decision.c/scheduler.c shape found during the audit). */ + TEST_ASSERT_FALSE(line_has_violation("xce_pendreason_t* reason;")); + TEST_ASSERT_FALSE(line_has_violation( + " thing = (xce_decision_t*)calloc(1, sizeof(*thing));")); +} + +/* Req 6 AC 2's actual regression lock: no scanned file contains a line + * constructing an xce_pendreason_t via direct calloc(). */ +static void test_no_direct_pendreason_calloc_in_scheduler_tree(void) +{ + size_t i; + + for (i = 0; i < NUM_SCANNED; i++) { + char path[4096]; + char violating_line[512]; + char msg[4096 + 512 + 256]; + int rc; + + snprintf(path, sizeof(path), "%s/%s", XCE_SCHEDULER_ROOT_PATH, + SCANNED_RELATIVE_PATHS[i]); + + rc = scan_file_for_violation(path, violating_line, + sizeof(violating_line)); + + TEST_ASSERT_NOT_EQUAL_MESSAGE(-1, rc, path); + + if (rc == 1) { + snprintf(msg, sizeof(msg), + "%s contains a direct xce_pendreason_t calloc() -- " + "Req 6 AC 2 requires make_pendreason() (lib/preason.c) " + "as the sole factory. Offending line: %s", + path, violating_line); + TEST_FAIL_MESSAGE(msg); + } + } +} + +int main(void) +{ + UNITY_BEGIN(); + RUN_TEST(test_all_scanned_paths_are_readable); + RUN_TEST(test_detector_fires_on_same_line_and_not_on_split_lines); + RUN_TEST(test_no_direct_pendreason_calloc_in_scheduler_tree); + return UNITY_END(); +}