diff --git a/.kiro/specs/resreq-maxtasks-wiring/tasks.md b/.kiro/specs/resreq-maxtasks-wiring/tasks.md index 12a25738..ba1ba753 100644 --- a/.kiro/specs/resreq-maxtasks-wiring/tasks.md +++ b/.kiro/specs/resreq-maxtasks-wiring/tasks.md @@ -40,13 +40,13 @@ - _Boundary: test_maxtasks_wiring_ingest.c_ - _Depends: 2.1, 2.2_ -- [ ] 3.2 Verify driver/simulation ingest parity, update-immunity, and no cache collision across task counts +- [x] 3.2 Verify driver/simulation ingest parity, update-immunity, and no cache collision across task counts - Write a regression test proving job ingest driven through the simulation driver's own entry point reaches the identical shared ingest code fresh (real-driver) ingest uses, applying the fix identically with no driver/simulation divergence - Write a structural regression test proving that after a job is ingested and its resource request is then updated, the job's already-dispatched task count remains unchanged by that update - Write a test submitting two otherwise-identical jobs through fresh ingest that differ only in requested task count, confirming an unsatisfiable outcome cached for one does not produce a cache hit for the other - Observable: all three tests pass, proving real/simulated ingest parity, that an update never silently changes an already-dispatched task count, and that differing task counts never collide in the unsatisfiable-resource-request cache on the fresh-ingest path - _Requirements: 1.4, 5.4_ - - _Boundary: test_maxtasks_wiring_ingest.c_ + - _Boundary: test_maxtasks_wiring_ingest.c, test_maxtasks_wiring_sim_driver_parity.c (see Implementation Notes)_ - _Depends: 2.1, 2.2_ - [ ] 3.3 (P) Retire the multi-host allocator characterization test's manual workaround @@ -89,4 +89,5 @@ - _Requirements: 5.2_ ## Implementation Notes +- Task 3.2's driver/simulation-parity test was split into a second new file, `src/scheduler/testsuite/test_maxtasks_wiring_sim_driver_parity.c` (registered separately in `meson.build`), rather than living inside `test_maxtasks_wiring_ingest.c` as originally scoped. Reason: proving this specific claim requires white-box `#include`-ing `sim_loop.c`/`core_events.c` to reach their real `static` translators (the only way to avoid a mirrored test that can't catch a real translator regression — confirmed via an independent mutation-test reproduction during review), but `test_maxtasks_wiring_ingest.c` already links the full real `scheduler_srcs` object list for its own task-3.1 test, and the two techniques collide at link time (duplicate symbols) in one translation unit. No production code or requirement scope changed; this is a file-boundary split only, driven by a real toolchain constraint discovered during implementation. - Task 2.1 surfaced an out-of-boundary side effect: `src/scheduler/testsuite/test_repl_breakpoints_inspection.c:735`'s `test_info_job_reports_state_queue_priority` hardcodes an incidental `"Tasks: min = 0, max = 0"` byproduct of its fixture's `parse_select()` failing on an unregistered `"type"` resource (which previously left the primary's task-count fields at memset-zero, before `apply_job_task_count()` existed). Now correctly overwritten to `(1,1)` per Requirement 1.2. Before task 5.1 is considered complete, change that assertion's expected substring to `"Tasks: min = 1, max = 1"`. diff --git a/src/scheduler/testsuite/meson.build b/src/scheduler/testsuite/meson.build index 5e945ee5..c3136858 100644 --- a/src/scheduler/testsuite/meson.build +++ b/src/scheduler/testsuite/meson.build @@ -1961,3 +1961,32 @@ sched_maxtasks_wiring_ingest_test = executable( test('scheduler maxtasks wiring ingest test', sched_maxtasks_wiring_ingest_test, suite: 'scheduler', env: ['ASAN_OPTIONS=detect_leaks=0']) + +# resreq-maxtasks-wiring spec, task 3.2 remediation round (Req 1.4, 5.4, and +# design.md's "Project Invariant Cross-Check" driver/simulation parity +# commitment): proves the REAL, `static` simulation-driver translator chain +# (sim_process_replay_event_message() -> replay_event_to_core_event(), +# sim_loop.c) -- not a hand-built mirror of it -- applies this feature's +# task-count wiring correctly, by feeding a hand-built, in-memory +# ReplayEvent{jobsPush} capnp message through the REAL function and pushing +# its own real output through the identical xce_core_push_events()/ +# core_build_job() ingest chain. Same white-box #include pattern as +# sched_sim_event_loop_test (core.c, core_clock.c, core_events.c, +# core_session.c, decision.c, plugin.c, plugins/builtin_fcfs.c, recording.c, +# recording_session.c, repl.c and siblings, sim.c, sim_loop.c, sim_picker.c, +# sim_record.c, sim_verify.c, snapshot.c/snapshot_wire.c/snapshot_load.c) -- +# see this test file's own header comment for why it is a separate file +# from sched_maxtasks_wiring_ingest_test (that file links the whole real +# scheduler_srcs object list for its own test 3, which would collide with +# this file's white-box #include of core_events.c/sim_loop.c into the same +# translation unit). +sched_maxtasks_wiring_sim_driver_parity_test = executable( + 'sched_maxtasks_wiring_sim_driver_parity_test', + ['test_maxtasks_wiring_sim_driver_parity.c'], + include_directories:[incdir], + dependencies: [scheduler_test_deps], + install: false) + +test('scheduler maxtasks wiring sim driver parity test', + sched_maxtasks_wiring_sim_driver_parity_test, suite: 'scheduler', + env: ['ASAN_OPTIONS=detect_leaks=0']) diff --git a/src/scheduler/testsuite/test_maxtasks_wiring_ingest.c b/src/scheduler/testsuite/test_maxtasks_wiring_ingest.c index 300fff38..569fa366 100644 --- a/src/scheduler/testsuite/test_maxtasks_wiring_ingest.c +++ b/src/scheduler/testsuite/test_maxtasks_wiring_ingest.c @@ -112,6 +112,55 @@ * All three lines were restored immediately afterward (git-verified clean * diff against core_events.c/snapshot_load.c) before this file's own * implementation was finalized. + * + * ── Task 3.2 addendum -- two more tests, same file/link technique ──────── + * Task 3.2 (Req 1.4, 5.4, and design.md's "Project Invariant Cross-Check") + * appends two MORE tests to this same file, reusing every fixture helper + * above. (A third task-3.2 test, proving driver/simulation ingest parity + * via the REAL, `static` simulation-driver translator chain, lives in its + * own separate file, test_maxtasks_wiring_sim_driver_parity.c -- see that + * file's own header comment for why: an independent review rejected this + * file's original mirrored version of that test and required white-box + * #include-ing sim_loop.c's real translator instead, which cannot share a + * translation unit with this file's own test 3, below, since that test + * links the whole real scheduler_srcs object list.) + * + * 4. test_resource_request_update_never_changes_dispatched_task_count: + * Req 1.4's structural regression test. A job is ingested fresh, then + * a real XCE_CORE_EVENT_JOB_UPDATE is driven for the SAME jobid + * carrying BOTH a different resreq string AND a different requested + * task count. Confirms job->resreq WAS genuinely replaced (so the + * immunity assertion below isn't vacuous), then confirms + * job->schedobj->u.primary->resreq -- what the real allocator dispatch + * reads -- is the EXACT SAME object with the EXACT SAME ingest-time + * task count, because core_events.c's JOB_UPDATE case never reassigns + * job->schedobj (grepped exhaustively: no such assignment exists in + * that case block). This is a reasoned, not empirically red/green + * -tested, proof -- see design.md's Existing Architecture Analysis: + * the guarantee holds today for this structural reason (bug-1773), + * independent of any new code this feature adds; there is no existing + * production bug for a RED phase to catch here, only a future + * regression (job->schedobj being reassigned on update) this test now + * guards against. + * + * 5. test_differing_task_counts_do_not_collide_in_unsatisfiable_cache: + * Req 5.4, fresh-ingest path only (snapshot-restore is the documented, + * pre-existing bug-1772 exception, out of this test's scope). Two + * otherwise-identical jobs, differing ONLY in requested task count, + * are ingested fresh into the SAME core (so their resreq_fp's other + * components -- select_node_id/rusage_hash -- are guaranteed + * identical). Reuses scheduler-allocation's own test_alloc_cache.c + * conventions (cache_insert()/cache_lookup() driven directly against + * real primary schedobj pointers) combined with this file's own + * fresh-ingest technique. Empirically verified meaningful: with + * core_events.c's apply_job_task_count() call temporarily commented + * out, both jobs collapse to the IDENTICAL resreq_fp and this test + * then FAILS one assertion earlier than cache_lookup() itself -- at + * its own fingerprint sanity check (job A's and job B's + * resreq_fp.min_tasks, expected to differ, are instead both 1) -- + * confirmed by temporarily disabling the wiring, rebuilding this + * binary in isolation, observing that failure, then reverting to a + * git-clean state before finalizing this test. */ #include #include @@ -144,6 +193,15 @@ extern int scheduler_req_handler(xce_request_t*, void*); #define JOBID_PARITY ((uint64_t)910201ULL) #define JOBID_SINGLE_TASK ((uint64_t)910301ULL) +/* task 3.2 additions (Req 1.4, 5.4) -- see this file's own top-of-file + * header comment's task-3.2 addendum, below the task-3.1 narrative, for + * what each of these two tests proves. (The driver/simulation parity test, + * design.md's "Project Invariant Cross-Check" commitment, now lives in its + * own separate file, test_maxtasks_wiring_sim_driver_parity.c.) */ +#define JOBID_UPDATE_IMMUNE ((uint64_t)910501ULL) +#define JOBID_CACHE_A ((uint64_t)910601ULL) +#define JOBID_CACHE_B ((uint64_t)910602ULL) + /* ════════════════════════════════════════════════════════════════════════ * __wrap_xcecli_start / __wrap_xcecli_join -- trivial always-succeed * doubles, identical rationale/shape to every other real-scheduler_srcs @@ -782,6 +840,305 @@ test_single_task_job_dispatch_and_host_selection_unchanged_via_real_allocator(vo "exactly 1.0"); } +/* ---------------------------------------------------------------------- */ +/* Test 4 -- Req 1.4: an update never changes the already-ingested task */ +/* count (job->schedobj is never rebuilt by JOB_UPDATE) */ +/* ---------------------------------------------------------------------- */ + +/* + * Design.md's Existing Architecture Analysis: core_events.c's JOB_UPDATE + * handler (live) replaces job->resreq with a freshly parse_resreq()'d + * object when updated->submit->resreq is non-NULL, but never reassigns + * job->schedobj (grepped exhaustively: no `job->schedobj =` assignment + * anywhere in the JOB_UPDATE case, core_events.c:1174-1257) -- so + * scheduler.c's real dispatch (job->schedobj->u.primary->resreq, + * scheduler.c:315) never sees an update at all, for ANY field, not only + * task count (bug-1773). Requirement 1.4's guarantee already holds today + * for this structural reason, not because this feature actively re-wires + * anything on update (design.md, Non-Goals) -- this test's job is to PROVE + * the guarantee holds, not to add new production wiring. + * + * A real multi-task job (mintasks=4/maxtasks=7, deliberately distinct from + * every other test's own fixture values in this file) is ingested fresh, + * then a JOB_UPDATE event for the SAME jobid is driven with BOTH a + * different resreq string ("rusage[mem=2]" instead of "type == any") AND a + * different requested task count (mintasks=1/maxtasks=1) in its own + * job_submit_data_t -- exactly the scenario Req 1.4 describes ("a job's + * resource request is updated"). Confirms job->resreq WAS genuinely + * replaced (proving the update was not silently a no-op, which would make + * the immunity assertion below vacuous), and that job->schedobj->u.primary + * ->resreq -- what the real allocator dispatch actually reads -- is the + * EXACT SAME object, with the SAME minTasks/maxTasks, ingest originally + * produced, completely unaffected by the update's own different task + * count. + * + * Meaningfulness (reasoned, per this task's own brief, rather than + * empirically red/green-tested -- the failure mode guarded against is a + * future production change, not a bug present today): this test would fail + * if a future change to the JOB_UPDATE case started reassigning + * job->schedobj (e.g. rebuilding it via core_build_schedobj() the way + * core_build_job() does), since apply_job_task_count() applied there would + * then pick up the UPDATE's own (differing) mintasks/maxtasks rather than + * ingest's -- the post-update assertions below would read 1/1 instead of + * the expected 4/7. + */ +static void +test_resource_request_update_never_changes_dispatched_task_count(void) +{ + xce_sched_core_t core; + xce_queue_t* queue; + xce_core_event_t queue_event; + xce_job_t* job; + xce_job_t* job_ptrs[1]; + xce_core_event_t job_event; + xce_sched_job_t* sjob; + xce_primary_resreq_t* ingest_time_primary; + xce_job_t* update_job; + xce_core_event_t update_event; + int rc; + + rc = xce_sched_core_init(&core); + TEST_ASSERT_TRUE_MESSAGE(rc >= 0, "fixture: xce_sched_core_init() must " + "succeed"); + + queue = make_wire_queue("normal", 1); + memset(&queue_event, 0, sizeof(queue_event)); + queue_event.type = XCE_CORE_EVENT_QUEUE_UPDATE; + queue_event.u.queue_update.queue = queue; + rc = xce_core_push_events(&core, &queue_event, 1); + TEST_ASSERT_TRUE_MESSAGE(rc >= 0, "fixture: real QUEUE_UPDATE ingest " + "must succeed"); + + job = make_push_job(JOBID_UPDATE_IMMUNE, "type == any", "alice", "normal", + "proj1", 4, 7); + job_ptrs[0] = job; + + memset(&job_event, 0, sizeof(job_event)); + job_event.type = XCE_CORE_EVENT_JOBS_PUSH; + job_event.u.jobs_push.njobs = 1; + job_event.u.jobs_push.jobs = job_ptrs; + + rc = xce_core_push_events(&core, &job_event, 1); + TEST_ASSERT_TRUE_MESSAGE(rc >= 0, "fixture: real JOBS_PUSH ingest must " + "succeed"); + + sjob = sched_get_job(&core, JOBID_UPDATE_IMMUNE); + TEST_ASSERT_NOT_NULL(sjob); + TEST_ASSERT_NOT_NULL(sjob->schedobj); + TEST_ASSERT_EQUAL_INT(XCE_RESREQ_PRIMARY, sjob->schedobj->tag); + ingest_time_primary = sjob->schedobj->u.primary->resreq; + TEST_ASSERT_NOT_NULL(ingest_time_primary); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 4, ingest_time_primary->minTasks, + "fixture: ingest must derive the job's real minTasks before the " + "update is driven"); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 7, ingest_time_primary->maxTasks, + "fixture: ingest must derive the job's real maxTasks before the " + "update is driven"); + + /* ── Drive a real JOB_UPDATE: a DIFFERENT resreq string AND a + * DIFFERENT requested task count for the SAME job ─────────────────── */ + update_job = make_push_job(JOBID_UPDATE_IMMUNE, "rusage[mem=2]", "alice", + "normal", "proj1", 1, 1); + + memset(&update_event, 0, sizeof(update_event)); + update_event.type = XCE_CORE_EVENT_JOB_UPDATE; + update_event.u.job_update.jobid = JOBID_UPDATE_IMMUNE; + update_event.u.job_update.job = update_job; + + rc = xce_core_push_events(&core, &update_event, 1); + TEST_ASSERT_TRUE_MESSAGE(rc >= 0, "real JOB_UPDATE ingest must succeed"); + + sjob = sched_get_job(&core, JOBID_UPDATE_IMMUNE); + TEST_ASSERT_NOT_NULL(sjob); + + /* Sanity: the update DID genuinely replace job->resreq (not a silent + * no-op) -- job->resreq is now a fresh object showing parse_resreq_ + * primary()'s own bare 1/1 default (JOB_UPDATE never calls apply_job_ + * task_count(), design.md), confirming the assertions below are a + * meaningful immunity proof, not a vacuous one from an update that + * never took effect. */ + TEST_ASSERT_NOT_NULL(sjob->resreq); + TEST_ASSERT_EQUAL_INT(XCE_RESREQ_PRIMARY, sjob->resreq->tag); + TEST_ASSERT_FALSE_MESSAGE( + sjob->resreq->u.primary == ingest_time_primary, + "fixture: JOB_UPDATE must have replaced job->resreq with a " + "genuinely NEW object, not left the old one in place, for this " + "test's immunity assertion to be meaningful"); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 1, sjob->resreq->u.primary->minTasks, + "fixture: job->resreq (freshly reparsed by JOB_UPDATE) must show " + "parse_resreq_primary()'s own bare default, confirming JOB_UPDATE " + "never calls apply_job_task_count()"); + + /* ── The Req 1.4 proof: job->schedobj->u.primary->resreq (what the + * real allocator dispatch reads, scheduler.c:315) is the EXACT SAME + * object, with the EXACT SAME ingest-time task count, unaffected by + * the update -- because JOB_UPDATE never reassigns job->schedobj. ──── */ + TEST_ASSERT_NOT_NULL(sjob->schedobj); + TEST_ASSERT_TRUE_MESSAGE( + sjob->schedobj->u.primary->resreq == ingest_time_primary, + "job->schedobj->u.primary->resreq must remain the EXACT SAME " + "object ingest originally built -- JOB_UPDATE must never rebuild " + "job->schedobj"); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 4, sjob->schedobj->u.primary->resreq->minTasks, + "Req 1.4: the job's dispatched minTasks must remain the " + "ingest-time value (4), unaffected by the update's own different " + "mintasks (1)"); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 7, sjob->schedobj->u.primary->resreq->maxTasks, + "Req 1.4: the job's dispatched maxTasks must remain the " + "ingest-time value (7), unaffected by the update's own different " + "maxtasks (1)"); +} + +/* ---------------------------------------------------------------------- */ +/* Test 5 -- Req 5.4: differing task counts never collide in the */ +/* unsatisfiable-resreq cache, on the fresh-ingest path */ +/* ---------------------------------------------------------------------- */ + +/* + * Req 5.4 (fresh-ingest path only; snapshot-restore is the documented, + * pre-existing bug-1772 exception -- design.md's Testing Strategy). Two + * otherwise-identical jobs (same resreq string, same user/queue/project) + * that differ ONLY in requested task count must never collide in + * scheduler-allocation's unsatisfiable-resreq cache (sched/alloc_cache.h). + * core_build_primary_schedobj() (core_events.c) computes primary-> + * resreq_fp.min_tasks/max_tasks directly from resreq->minTasks/maxTasks -- + * the exact field this feature's apply_job_task_count() call + * (core_events.c, immediately before core_build_schedobj()) sets from the + * job's real requested count. Two such jobs, ingested fresh into the SAME + * core/resmgr (so their resreq_fp's OTHER components -- select_node_id, + * rusage_hash -- are guaranteed identical, isolating task count as the + * only possible source of a difference), must therefore produce DIFFERENT + * cache keys: an unsatisfiable outcome cached for one must not hit for the + * other. + * + * This reuses scheduler-allocation's own alloc_cache test conventions + * (test_alloc_cache.c: cache_insert()/cache_lookup() driven directly + * against real primary schedobj pointers, a bare xce_scheduler_t with only + * unsatisfiable_cache constructed) combined with REAL job ingest via + * core_build_job() (this file's own established fresh-ingest technique) + * rather than test_alloc_cache.c's own hand-built xce_primary_schedobj_t + * fixtures -- this is the genuinely new combination this task adds. + * + * Empirically verified meaningful (not merely reasoned): with + * core_events.c's apply_job_task_count() call temporarily commented out + * (the pre-fix always-1/1 behavior bug-1770 describes), both jobs below + * collapse to the IDENTICAL resreq_fp (min_tasks=max_tasks=1 for both, + * regardless of their different requested counts), and this test's own + * cache_lookup() assertion for job B FAILS (a real collision: rc==1, a + * false hit) -- confirmed by temporarily commenting out that one line, + * rebuilding this binary in isolation, observing the failure, then + * reverting the file to a git-clean state before finalizing this test. + */ +static void +test_differing_task_counts_do_not_collide_in_unsatisfiable_cache(void) +{ + xce_scheduler_t sched; + xce_queue_t* queue; + xce_core_event_t queue_event; + xce_job_t* job_a; + xce_job_t* job_b; + xce_job_t* job_ptrs[2]; + xce_core_event_t job_event; + xce_sched_job_t* sjob_a; + xce_sched_job_t* sjob_b; + int out_reason = -1; + int rc; + + memset(&sched, 0, sizeof(sched)); + rc = xce_sched_core_init(&(sched.core)); + TEST_ASSERT_TRUE_MESSAGE(rc >= 0, "fixture: xce_sched_core_init() must " + "succeed"); + + cache_clear(&sched); + TEST_ASSERT_NOT_NULL_MESSAGE( + sched.unsatisfiable_cache, + "fixture: cache_clear() must construct a real unsatisfiable_cache " + "table (mirrors the real per-session construction, main.c's " + "xce_core_set_cache_clear_cb() registration)"); + + queue = make_wire_queue("normal", 1); + memset(&queue_event, 0, sizeof(queue_event)); + queue_event.type = XCE_CORE_EVENT_QUEUE_UPDATE; + queue_event.u.queue_update.queue = queue; + rc = xce_core_push_events(&(sched.core), &queue_event, 1); + TEST_ASSERT_TRUE_MESSAGE(rc >= 0, "fixture: real QUEUE_UPDATE ingest " + "must succeed"); + + /* Same resreq string/user/queue/project -- differ ONLY in requested + * task count. Both ingested into the SAME core/resmgr, so their + * resreq_fp's select_node_id/rusage_hash components are guaranteed + * identical -- isolating task count as the only possible source of a + * fingerprint difference. */ + job_a = make_push_job(JOBID_CACHE_A, "rusage[mem=1]", "cache_user", + "normal", "cache_proj", 2, 2); + job_b = make_push_job(JOBID_CACHE_B, "rusage[mem=1]", "cache_user", + "normal", "cache_proj", 3, 3); + job_ptrs[0] = job_a; + job_ptrs[1] = job_b; + + memset(&job_event, 0, sizeof(job_event)); + job_event.type = XCE_CORE_EVENT_JOBS_PUSH; + job_event.u.jobs_push.njobs = 2; + job_event.u.jobs_push.jobs = job_ptrs; + + rc = xce_core_push_events(&(sched.core), &job_event, 1); + TEST_ASSERT_TRUE_MESSAGE(rc >= 0, "real JOBS_PUSH ingest must succeed " + "for both jobs"); + + sjob_a = sched_get_job(&(sched.core), JOBID_CACHE_A); + sjob_b = sched_get_job(&(sched.core), JOBID_CACHE_B); + TEST_ASSERT_NOT_NULL(sjob_a); + TEST_ASSERT_NOT_NULL(sjob_b); + TEST_ASSERT_NOT_NULL(sjob_a->schedobj); + TEST_ASSERT_NOT_NULL(sjob_b->schedobj); + TEST_ASSERT_EQUAL_INT(XCE_RESREQ_PRIMARY, sjob_a->schedobj->tag); + TEST_ASSERT_EQUAL_INT(XCE_RESREQ_PRIMARY, sjob_b->schedobj->tag); + + /* Sanity: the two jobs' real, ingest-derived fingerprints DO differ in + * exactly (and only) their task-count component -- confirms this + * test's premise before exercising the cache itself. */ + TEST_ASSERT_EQUAL_INT_MESSAGE( + 2, sjob_a->schedobj->u.primary->resreq_fp.min_tasks, + "job A's real ingest-derived resreq_fp.min_tasks must reflect its " + "own requested task count"); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 3, sjob_b->schedobj->u.primary->resreq_fp.min_tasks, + "job B's real ingest-derived resreq_fp.min_tasks must reflect its " + "own (different) requested task count"); + TEST_ASSERT_EQUAL_UINT64_MESSAGE( + sjob_a->schedobj->u.primary->resreq_fp.rusage_hash, + sjob_b->schedobj->u.primary->resreq_fp.rusage_hash, + "fixture: both jobs share the identical resreq string, so their " + "rusage_hash fingerprint component must be identical -- isolating " + "task count as the only differing component"); + + /* ── The Req 5.4 proof: an unsatisfiable outcome cached for job A + * must NOT produce a cache hit for job B. ──────────────────────────── */ + rc = cache_insert(&sched, sjob_a->schedobj, 7 /* arbitrary category */); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 0, rc, "fixture: cache_insert() for job A must succeed"); + + rc = cache_lookup(&sched, sjob_b->schedobj, &out_reason); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 0, rc, + "Req 5.4: job B (a different requested task count, otherwise " + "identical) must NOT hit job A's cached unsatisfiable outcome"); + + /* Sanity: the cache mechanism itself genuinely works -- job A's OWN + * key IS a hit, with the SAME reason category given at insert time. + * Without this, the miss above could just as easily mean the cache is + * dead altogether, not that it correctly distinguishes task counts. */ + rc = cache_lookup(&sched, sjob_a->schedobj, &out_reason); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 1, rc, "sanity: job A's own key must hit its own cached entry"); + TEST_ASSERT_EQUAL_INT(7, out_reason); +} + int main(void) { UNITY_BEGIN(); @@ -790,5 +1147,7 @@ int main(void) test_fresh_ingest_and_snapshot_restore_derive_identical_task_count_for_equivalent_job); RUN_TEST( test_single_task_job_dispatch_and_host_selection_unchanged_via_real_allocator); + RUN_TEST(test_resource_request_update_never_changes_dispatched_task_count); + RUN_TEST(test_differing_task_counts_do_not_collide_in_unsatisfiable_cache); return UNITY_END(); } diff --git a/src/scheduler/testsuite/test_maxtasks_wiring_sim_driver_parity.c b/src/scheduler/testsuite/test_maxtasks_wiring_sim_driver_parity.c new file mode 100644 index 00000000..cb45f138 --- /dev/null +++ b/src/scheduler/testsuite/test_maxtasks_wiring_sim_driver_parity.c @@ -0,0 +1,474 @@ +/* + * test_maxtasks_wiring_sim_driver_parity.c - resreq-maxtasks-wiring spec, + * task 3.2 remediation round (Req 1.4, 5.4, and design.md's "Project + * Invariant Cross-Check" driver/simulation parity commitment). + * + * ── Why this is a SEPARATE file, not an addition to + * test_maxtasks_wiring_ingest.c ───────────────────────────────────────── + * This task's driver/simulation-parity test previously lived in + * test_maxtasks_wiring_ingest.c as + * test_driver_and_simulation_style_jobs_push_apply_identical_task_count_ + * wiring(), an independent-review REJECTED design: it never executed + * sim_loop.c's/driver.c's own `static` translators + * (replay_event_to_core_event()/sim_process_replay_event_message()/ + * translate_one_pending_update()) -- it hand-built an xce_core_event_t by + * MIRRORING the test author's own belief about those translators' field- + * copy bodies, then fed that hand-built event into the already-covered + * xce_core_push_events() path tests 1-3 of that file exercise. A real + * future regression in either translator's own body (e.g. an incorrect + * field name, a dropped assignment) would NOT have been caught -- the + * mirror would just as happily reproduce the bug, or never notice a + * divergence, since it never calls the translator at all. + * + * The reviewer identified that reaching the REAL, `static` + * sim_process_replay_event_message() (and, through it, the REAL, `static` + * replay_event_to_core_event()) is NOT impractical: test_sim_event_loop.c + * ALREADY #include's ../src/sim_loop.c directly (a white-box #include, + * not a link-only technique) and calls sim_process_replay_event_message() + * with a hand-built, in-memory capnp ReplayEvent message -- no real + * `.replay` file or full sched_sim_driver_run() boot required. This file + * copies that exact technique (see test_sim_event_loop.c's own file header + * comment for the fully-justified #include/stub set this file mirrors + * verbatim). + * + * The reviewer also identified the genuine obstacle to folding this + * directly into test_maxtasks_wiring_ingest.c: that file links the whole + * real `scheduler_srcs` object list (needed for ITS OWN test 3, which + * drives a real sched_init()'d allocator dispatch) -- white-box + * #include-ing core_events.c/sim_loop.c into that SAME translation unit + * would collide with meson's separate compilation of those same files into + * the linked scheduler_srcs archive (duplicate-symbol link errors). This + * file therefore lives on its own, using test_maxtasks_wiring_ingest.c's + * task-3.1 fixture-helper NAMES (make_wire_queue()/make_push_job()) but + * its own small, independent copies of them (this directory's own + * established norm -- every white-box-#include test file in this + * directory keeps its own self-contained fixture helpers rather than + * sharing a header, e.g. test_sim_event_loop.c's own + * build_fixture_scheduler() is not shared with test_sim_startup.c's + * near-identical one either). + * + * Per design.md's own explicit language ("exercising this path through + * sim_loop.c's own entry point") and this codebase's own cited history + * (the scheduler-allocation task 5.2 incident: a fix applied only to the + * real-driver path once caused a real driver/simulation divergence), the + * SIMULATION side is this file's target -- driver.c's own real-driver-side + * translate_one_pending_update() is already exercised for a JOBS_PUSH + * payload by test_driver_translate.c's own test_translate_jobs_push() + * (a real, non-mirrored proof on that side already exists); duplicating + * that here would add risk (a second white-box #include-of-driver.c + * translation unit alongside this one, its own separate link-surface to + * verify) for no new coverage, so this file focuses on the one side that + * genuinely lacked a real-code proof. + * + * ── What this test proves ─────────────────────────────────────────────── + * A real multi-task job (mintasks=4/maxtasks=6, distinct from every other + * fixture value elsewhere in this feature's test suite) is carried inside + * a hand-built, in-memory ReplayEvent{jobsPush} capnp message (built via + * the SAME plain-C-struct + encode_ReplayEvent_ptr() technique + * recording_events.c's own write_one_replay_event() uses for this exact + * XCE_CORE_EVENT_JOBS_PUSH case in production -- not a hand-rolled capnp + * builder walk). That message is written to a real temp file, reopened, + * and fed through the REAL, `static` sim_process_replay_event_message() + * (sim_loop.c) -- the same function sched_sim_driver_run()'s own read loop + * calls for every ReplayEvent message in a genuine `.replay` file. The + * xce_core_event_t THAT REAL FUNCTION populates (via its own internal, + * equally-real call to replay_event_to_core_event()) is then pushed + * through the identical xce_core_push_events()/core_build_job() ingest + * chain this feature's fix lives in, and the resulting job's dispatched + * standalone primary resreq is asserted to carry the job's correct real + * task count (4/6), not parse_resreq_primary()'s own always-1 default. + * + * This directly catches a REAL regression in sim_loop.c's own translator + * body: confirmed live (see "RED-phase verification" below) by temporarily + * mutating replay_event_to_core_event()'s ReplayEvent_body_jobsPush case + * and observing this test fail, then reverting. + * + * ── RED-phase verification performed while writing this file (not + * committed here) ───────────────────────────────────────────────────── + * sim_loop.c's replay_event_to_core_event(), ReplayEvent_body_jobsPush + * case, was temporarily mutated from: + * ev->u.jobs_push.njobs = src->body.jobs_push->count; + * to: + * ev->u.jobs_push.njobs = 0; + * This file's binary was rebuilt and run in isolation: the test FAILED at + * the `TEST_ASSERT_EQUAL_INT(1, translated_ev.u.jobs_push.njobs)` + * assertion (0 != 1) -- proving this test genuinely calls the REAL, + * live sim_loop.c translator body, not a mirror of it (a mirror would be + * unaffected by a mutation to the real function it never calls). The + * mutation was then reverted and `git diff` against sim_loop.c confirmed + * clean before this file was finalized. + */ +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +#include "controller.capnp.h" +#include "kvmgr.h" +#include "resreq.h" +#include "scheduler.h" +#include "xstring.h" + +/* Pull in the real functions under test into this TU (white-box #include), + * the EXACT same set test_sim_event_loop.c's own file header comment + * justifies file-by-file (see that file for the full per-file rationale + * this comment does not repeat): the core/session/decision machinery + * sim_loop.c's event loop drives, the core.h-scoped picker registration + * path (plugin.c/builtin_fcfs.c), recording.c/recording_session.c/ + * sim_record.c (sim_loop.c unconditionally references their symbols), + * repl.c and its siblings (sim_loop.c #includes repl.h, whose task 12.1 + * inline wrappers unconditionally reference xce_repl_start()/ + * xce_repl_post_session_check()), sim.c/sim_loop.c/sim_picker.c (the code + * under test plus its bug-1567 sim_bootstrap_picker() sibling), + * sim_verify.c (sched_sim_driver_run() references sim_decision_eq()/ + * sim_log_mismatch() as externs even though this file never drives that + * function), and snapshot.c/snapshot_wire.c/snapshot_load.c (needed by + * sim.c's own snapshot load). */ +#include "../src/core.c" +#include "../src/core_clock.c" +#include "../src/core_events.c" +#include "../src/core_session.c" +#include "../src/decision.c" +#include "../src/plugin.c" +#include "../src/plugins/builtin_fcfs.c" +#include "../src/recording.c" +#include "../src/recording_session.c" +#include "../src/repl.c" +#include "../src/repl_fmt.c" +#include "../src/repl_rollup.c" +#include "../src/repl_break.c" +#include "../src/repl_info.c" +#include "../src/repl_params.c" +#include "../src/sim.c" +#include "../src/sim_loop.c" +#include "../src/sim_picker.c" +#include "../src/sim_record.c" +#include "../src/sim_verify.c" +#include "../src/snapshot.c" +#include "../src/snapshot_load.c" +#include "../src/snapshot_wire.c" + +/* Link-satisfying stubs -- identical rationale/bodies to + * test_sim_event_loop.c's own copies (schedobj.c/resource.c are not linked + * here, same as that file). */ + +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; } + +int sched_reconcile_job_resource_usage(xce_sched_core_t* c, + xce_job_account_t* a, + xce_job_rusages_t* r) +{ + (void)c; (void)a; (void)r; return 0; +} + +void sched_worker_release_host_ctx(xce_sched_worker_t* w) { (void)w; } + +int sched_fill_limit_requests(xce_scheduler_t* sched, xvec_idval_t* vec, + xlist_t* limits) +{ + (void)sched; + (void)vec; + (void)limits; + return 0; +} + +int sched_account_jobs(xce_sched_core_t* core) +{ + (void)core; + return 0; +} + +int sched_resolve_job_picker(xce_sched_core_t* core, xce_sched_plugin_t** out) +{ + (void)core; + + if (out == NULL) { + return -EINVAL; + } + + /* No picker plugin is ever registered in this file's fixture -- this + * test's Observable is the ingest-time resreq wiring, not picker + * hook-firing (task 4.3's own test suite covers that exhaustively) -- + * so -EINVAL (core_events.c's own documented behavior on an + * unresolved picker, "WARNs and proceeds with picker=NULL") is always + * the correct, deliberate return here. */ + *out = NULL; + return -EINVAL; +} + +void setUp(void) { return; } +void tearDown(void) { return; } + +#define JOBID_SIM_DRIVER_PARITY ((uint64_t)910701ULL) + +/* ---------------------------------------------------------------------- */ +/* Fixture helpers */ +/* ---------------------------------------------------------------------- */ + +/* make_tmp_dir -- identical technique/rationale to + * test_sim_event_loop.c's own helper of the same name. */ +static void make_tmp_dir(char* out, size_t out_len, const char* suffix) +{ + char tmpl[PATH_MAX]; + + snprintf(tmpl, sizeof(tmpl), "/tmp/sched_maxtasks_simparity%s_XXXXXX", + suffix); + TEST_ASSERT_NOT_NULL(mkdtemp(tmpl)); + TEST_ASSERT_TRUE(strlen(tmpl) < out_len); + strcpy(out, tmpl); +} + +/* make_wire_queue / make_push_job -- same shape and rationale as + * test_maxtasks_wiring_ingest.c's own helpers of the same names (this + * file's own small, independent copies -- see this file's header comment + * for why this directory's convention is per-file fixtures, not a shared + * header, for white-box-#include test TUs). Never torn down, same + * directory-wide convention. */ +static xce_queue_t* make_wire_queue(const char* name, int32_t priority) +{ + xce_queue_t* q = (xce_queue_t*)calloc(1, sizeof(*q)); + + TEST_ASSERT_NOT_NULL(q); + + q->queue = strdup(name); + q->status = QUEUE_STAT_OPEN; + q->priority = priority; + + return q; +} + +static xce_job_t* make_push_job(uint64_t jobid, char* resreq_str, char* user, + char* queue, char* project, uint32_t mintasks, + uint32_t maxtasks) +{ + xce_job_t* j = (xce_job_t*)calloc(1, sizeof(*j)); + + TEST_ASSERT_NOT_NULL(j); + + j->jobid = jobid; + j->state = JOB_STAT_PEND; + j->sub_time = 1000; + j->queue_time = 1000; + + j->submit = (job_submit_data_t*)calloc(1, sizeof(*(j->submit))); + TEST_ASSERT_NOT_NULL(j->submit); + + j->submit->queue = queue; + j->submit->user = user; + j->submit->project_name = project; + j->submit->resreq = resreq_str; + j->submit->mintasks = mintasks; + j->submit->maxtasks = maxtasks; + + return j; +} + +/* append_jobspush_event -- appends one ReplayEvent{jobsPush} message to the + * already-open stream `f`, using the SAME plain-C-struct + * (xce_jobs_data_t wrapper) + encode_ReplayEvent_ptr()/write_capnp_message() + * technique BOTH recording_events.c's own write_one_replay_event() + * (production write-side, XCE_CORE_EVENT_JOBS_PUSH case) and + * test_sim_event_loop.c's own append_jobstatechange_event()/ + * append_replay_session() (test-side precedent) already establish -- + * NOT a hand-rolled capnp builder walk. */ +static void append_jobspush_event(FILE* f, uint64_t ts, xce_job_t** jobs, + int njobs) +{ + xce_jobs_data_t jobs_wrap; + replay_event_t src; + struct capn c; + struct capn_segment* cs; + ReplayEvent_ptr p; + + memset(&jobs_wrap, 0, sizeof(jobs_wrap)); + jobs_wrap.n_total = njobs; + jobs_wrap.count = njobs; + jobs_wrap.jobs = jobs; + + memset(&src, 0, sizeof(src)); + src.ts = ts; + src.body_which = ReplayEvent_body_jobsPush; + src.body.jobs_push = &jobs_wrap; + + capn_init_malloc(&c); + cs = capn_root(&c).seg; + encode_ReplayEvent_ptr(cs, &p, &src); + capn_setp(capn_root(&c), 0, p.p); + + TEST_ASSERT_EQUAL_INT(0, write_capnp_message(f, &c)); + capn_free(&c); +} + +/* ---------------------------------------------------------------------- */ +/* The test */ +/* ---------------------------------------------------------------------- */ + +/* + * See this file's own header comment for the full narrative. Summary: a + * real multi-task job's ReplayEvent{jobsPush} wire message is decoded and + * translated by the REAL, `static` sim_process_replay_event_message() + * (which internally calls the REAL, `static` replay_event_to_core_event()) + * -- not a hand-built mirror of either -- and the resulting real + * xce_core_event_t is pushed through the identical + * xce_core_push_events()/core_build_job() ingest chain this feature's fix + * lives in, proving sim_loop.c's OWN translator code applies this + * feature's task-count wiring correctly. + */ +static void +test_sim_process_replay_event_jobs_push_wires_real_task_count_into_resreq_primary( + void) +{ + char path[PATH_MAX]; + FILE* f; + xce_sched_sim_driver_t sim; + xce_queue_t* queue; + xce_core_event_t queue_event; + xce_job_t* job; + xce_job_t* job_ptrs[1]; + struct capn c; + capn_ptr root; + xce_core_event_t translated_ev; + int record_rc = 0; + int translated; + xce_sched_job_t* sjob; + xce_primary_resreq_t* primary; + int rc; + + /* A minimal sim driver -- same rationale as + * test_sim_event_loop.c's own Test 1: only sim->core is exercised by + * sim_process_replay_event_message() (via sim_advance_virtual_clock()), + * so xce_sched_core_init() plus a frozen (speed=0) clock is a + * sufficient, self-contained fixture. */ + memset(&sim, 0, sizeof(sim)); + rc = xce_sched_core_init(&(sim.core)); + TEST_ASSERT_EQUAL_INT(0, rc); + + atomic_store_explicit(&(sim.core.speed_num), 0, memory_order_release); + atomic_store_explicit(&(sim.core.speed_den), 1, memory_order_release); + + /* Register a real queue the job references -- hand-built ingest, not + * part of this test's own Observable (the jobsPush translator below); + * same technique test_maxtasks_wiring_ingest.c's own fixtures use for + * an identical, incidental purpose. */ + queue = make_wire_queue("normal", 1); + memset(&queue_event, 0, sizeof(queue_event)); + queue_event.type = XCE_CORE_EVENT_QUEUE_UPDATE; + queue_event.u.queue_update.queue = queue; + rc = xce_core_push_events(&(sim.core), &queue_event, 1); + TEST_ASSERT_TRUE_MESSAGE(rc >= 0, "fixture: real QUEUE_UPDATE ingest " + "must succeed"); + + /* A real multi-task job (mintasks=4/maxtasks=6, deliberately distinct + * from every other fixture value elsewhere in this feature's test + * suite, so this is a genuinely independent data point). */ + job = make_push_job(JOBID_SIM_DRIVER_PARITY, "type == any", "alice", + "normal", "proj1", 4, 6); + job_ptrs[0] = job; + + /* ── Build a REAL ReplayEvent{jobsPush} capnp wire message on disk, + * then reopen and decode it -- the SAME on-disk round trip + * sched_sim_driver_run()'s own read loop performs for every message in + * a genuine `.replay` file. ─────────────────────────────────────── */ + make_tmp_dir(path, sizeof(path), "_jobspush"); + strcat(path, "/event.replay"); + + f = fopen(path, "w"); + TEST_ASSERT_NOT_NULL(f); + append_jobspush_event(f, 42000ULL, job_ptrs, 1); + fclose(f); + + f = fopen(path, "r"); + TEST_ASSERT_NOT_NULL(f); + TEST_ASSERT_EQUAL_INT(0, capn_init_fp(&c, f, 0)); + TEST_ASSERT_TRUE(msg_root_has_shape(&c, &root, ReplayEvent_word_count, + ReplayEvent_pointer_count)); + + /* ── The proof-of-real-code step: drive this message through the + * REAL, `static` sim_process_replay_event_message() (sim_loop.c) -- + * not a mirror of it. record_rc is always 0 here: sim.record_file is + * NULL for this whole fixture (no --sim-output involved). ────────── */ + translated = sim_process_replay_event_message(&sim, &c, root, + &translated_ev, &record_rc); + fclose(f); + + TEST_ASSERT_EQUAL_INT(0, record_rc); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 1, translated, "sim_loop.c's real sim_process_replay_event_message() " + "must translate a jobsPush ReplayEvent into a " + "usable xce_core_event_t"); + TEST_ASSERT_EQUAL_INT(XCE_CORE_EVENT_JOBS_PUSH, translated_ev.type); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 1, translated_ev.u.jobs_push.njobs, + "the real translator must carry through the real njobs count -- " + "this exact field is what a regression in " + "replay_event_to_core_event()'s own body would corrupt"); + TEST_ASSERT_NOT_NULL(translated_ev.u.jobs_push.jobs[0]); + + /* NOT a pointer-identity check against the original `job` local: this + * message went through a REAL on-disk capnp encode/decode round trip, + * so decode_ReplayEvent() (per sim_process_replay_event_message()'s + * own doc comment, sim_loop.c) deep-copies a brand-new xce_job_t/ + * job_submit_data_t pair out of the arena -- `job` itself was only + * ever the SOURCE this test serialized from, never aliased back. The + * real proof is that the decoded copy's own real fields (jobid, and + * -- the point of this whole test -- mintasks/maxtasks below) match + * what was serialized. */ + TEST_ASSERT_EQUAL_UINT64(JOBID_SIM_DRIVER_PARITY, + translated_ev.u.jobs_push.jobs[0]->jobid); + + /* ── Push the REAL translator's own output through the identical + * xce_core_push_events()/core_build_job() ingest chain this feature's + * fix lives in, and assert the resulting job's dispatched standalone + * primary resreq carries the job's correct real task count -- proving + * sim_loop.c's OWN, actual translator code (not a hand-written mirror + * of it) applies this feature's wiring correctly. ─────────────────── */ + rc = xce_core_push_events(&(sim.core), &translated_ev, 1); + TEST_ASSERT_TRUE_MESSAGE( + rc >= 0, "pushing the real translator's own output through " + "xce_core_push_events() must succeed"); + + sjob = sched_get_job(&(sim.core), JOBID_SIM_DRIVER_PARITY); + TEST_ASSERT_NOT_NULL(sjob); + TEST_ASSERT_NOT_NULL_MESSAGE( + sjob->schedobj, "core_build_job() must have built job->schedobj " + "for the job carried by the real translator's " + "output"); + TEST_ASSERT_EQUAL_INT(XCE_RESREQ_PRIMARY, sjob->schedobj->tag); + primary = sjob->schedobj->u.primary->resreq; + TEST_ASSERT_NOT_NULL(primary); + + TEST_ASSERT_EQUAL_INT_MESSAGE( + 4, primary->minTasks, + "the REAL simulation-driver translator path (sim_process_replay_" + "event_message() -> replay_event_to_core_event(), sim_loop.c), not " + "a hand-built mirror of it, must have carried the job's real " + "minTasks through to the dispatched standalone primary"); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 6, primary->maxTasks, + "the REAL simulation-driver translator path (sim_process_replay_" + "event_message() -> replay_event_to_core_event(), sim_loop.c), not " + "a hand-built mirror of it, must have carried the job's real " + "maxTasks through to the dispatched standalone primary"); + + printf("test_sim_process_replay_event_jobs_push_wires_real_task_count_" + "into_resreq_primary: PASS\n"); +} + +int main(void) +{ + UNITY_BEGIN(); + RUN_TEST( + test_sim_process_replay_event_jobs_push_wires_real_task_count_into_resreq_primary); + return UNITY_END(); +}