diff --git a/.kiro/specs/scheduler-sim-eager-reconciliation/tasks.md b/.kiro/specs/scheduler-sim-eager-reconciliation/tasks.md index d3610934..db61ac77 100644 --- a/.kiro/specs/scheduler-sim-eager-reconciliation/tasks.md +++ b/.kiro/specs/scheduler-sim-eager-reconciliation/tasks.md @@ -27,7 +27,7 @@ - _Depends: 1.1_ - [ ] 3. Validation: end-to-end proof under contention, decision-verification mode, and full regression -- [ ] 3.1 (P) Verify correct capacity accounting under resource contention with realistic dispatch-to-confirmation separation +- [x] 3.1 (P) Verify correct capacity accounting under resource contention with realistic dispatch-to-confirmation separation - Build a replay fixture where a worker's capacity is insufficient for every candidate job, with dispatch decisions and their running-state confirmations recorded several rounds apart - Confirm the live capacity check sees the correct, non-zero resource-in-use total at each round rather than treating capacity as permanently free - Observable: a resource-contention test demonstrates the capacity check's view of resource usage matches what actually happened in the original recording at each round, not a permanently-zero reading @@ -50,3 +50,8 @@ - Observable: a full scheduler test suite run shows the same pass/fail baseline as before this feature, with only the new tests added by this feature as net-new coverage - _Requirements: 3.3, 4.1, 4.3, 4.5_ - _Depends: 3.1, 3.2_ + +## Implementation Notes + +- Task 1.2: the pre-existing `test_recordstart_symlink_escape_replies_error` failure (bug-1560, `test_admin_request.c`) is unrelated to this spec and appears in every task's full-suite run — do not treat it as a regression. +- Task 3.1: any NEW `xce_sched_sim_driver_t` fixture passed to `sched_sim_driver_run()` in a scheduler test file MUST be declared `static`, not stack-local — `sched_sim_driver_run()` spawns a picker thread that is never joined and keeps reading the struct for the life of the process, so a stack-local instance is a latent stack-use-after-return once the test function returns (already-documented class, bug-1598/bug-1632; this spec's own task 3.1 round-1 review caught a live recurrence). `sched_sim_driver_start()` unconditionally `memset()`s the struct first, so `static` storage is safe as long as each fixture is written by exactly one call site. diff --git a/src/scheduler/testsuite/test_sim_contention_parity.c b/src/scheduler/testsuite/test_sim_contention_parity.c index 4677559d..ed49830c 100644 --- a/src/scheduler/testsuite/test_sim_contention_parity.c +++ b/src/scheduler/testsuite/test_sim_contention_parity.c @@ -1362,10 +1362,320 @@ test_contention_parity_matches_real_normal_mode_recording_round_by_round( "round_by_round: PASS\n"); } +/* ====================================================================== + * scheduler-sim-eager-reconciliation task 3.1 (Req 1.1-1.4): a resource- + * contention fixture where a job's recorded DISPATCH decision and its + * JOB_STATE_CHANGE-to-RUN confirmation are separated by SEVERAL rounds -- + * not the same-round shape the test above already covers (there, + * JOB_STATE_CHANGE lands in the SAME round window as the matching DISPATCH + * decision, which is exactly the shape the OLD, pre-task-1.1 trigger gate + * (`!JOB_IS_RUN(job->state)` in sim_reconcile_unaccounted_jobs(), removed + * by task 1.1) already handled correctly). This is the genuinely NEW + * scenario the OLD gate could never reconcile: jobs A and C are PEND at + * the exact round their own DISPATCH decision is recorded (round 1), and + * only reach RUN several rounds later (rounds 4 and 5 respectively) via a + * bare JOB_STATE_CHANGE event with no DISPATCH decision of its own -- by + * which point round 1's own decoded_session (the only place their + * DISPATCH ever existed) is long freed. Under the OLD gate, job->accounts + * would stay NULL forever for both jobs, and sched_account_jobs()'s own + * JOB_IS_RUN + accounts!=NULL charging gate would therefore never charge + * them -- resInUse would read a PERMANENTLY-ZERO 0.0 for the rest of the + * replay, wrongly making the worker's full WORKER_CAPACITY look available + * even once both jobs are genuinely running. See this task's own Status + * Report for the live revert-rebuild confirmation that this exact fixture + * reproduces that failure against a reverted tree. + * + * Fixture shape (reuses this file's own established constants verbatim -- + * WORKER_CAPACITY=10.0, JOBA_WANT=8.0, JOBB_WANT=5.0, JOBC_WANT=2.0, + * JOBD_WANT=1.0 -- never re-typed): + * Round 1 (begin_round=2000): A/B/C pushed PEND; A(8.0) and C(2.0) + * dispatched (A+C=10.0, exactly saturating capacity), B genuinely + * denied. NEITHER A NOR C's JOB_STATE_CHANGE appears anywhere near + * this round. + * Round 2, 3 (begin_round=3000/4000): pure filler -- B is genuinely + * re-offered and re-denied each round (builtin_fcfs's own documented + * "no-op, retried next round" contract), widening the gap before A/C's + * eventual confirmations below to genuinely "several rounds", and + * proving the deferred-zero state from round 1 persists correctly + * across MULTIPLE idle rounds, not just one. + * Round 4 (begin_round=5000): job A's JOB_STATE_CHANGE to RUN -- 3 + * rounds after its own round-1 DISPATCH. Job C is still PEND -- only + * PARTIAL usage (8.0 of 10.0) must be reflected from this round on. + * Round 5 (begin_round=6000): job C's JOB_STATE_CHANGE to RUN -- 4 + * rounds after ITS OWN round-1 DISPATCH (a deliberately DIFFERENT + * separation than job A's 3 rounds, proving this isn't a single-job + * coincidence). Both jobs now genuinely RUN -- FULL usage (10.0 of + * 10.0) must be reflected from this round on. + * + * The single test function below drives this SAME fixture shape three + * times, truncated after round 1, round 4, and round 5 respectively (via + * `write_multiround_contention_rounds()`'s `upto_round` parameter) -- + * sched_sim_driver_run() has no mid-replay pause point outside the REPL + * (this file's own earlier header comment already documents this gap), so + * a direct sched_check_host_satisfy() proof at an EARLY and a MID-replay + * point each need their own independently-run, truncated copy of the + * stream. The round-boundary g_round_resinuse[] snapshot mechanism (this + * file's own earlier addition) is reused for the full 5-round run to prove + * every round in between as well, not just the three checkpoints. + * ====================================================================== */ + +static void write_multiround_contention_rounds(FILE* f, int upto_round) +{ + xce_job_alloc_t* allocA; + xce_job_alloc_t* allocC; + replay_decision_t* round1_decisions[3]; + replay_decision_t* filler_decisions[1]; + + if (upto_round < 1) { + return; + } + + append_queue_update_event(f, 1000ULL, "normal"); + append_jobs_push_event(f, 1500ULL, JOBA_ID, "normal"); + append_jobs_push_event(f, 1500ULL, JOBB_ID, "normal"); + append_jobs_push_event(f, 1500ULL, JOBC_ID, "normal"); + + allocA = make_dispatch_alloc(JOBA_ID, WORKER_NAME, JOBA_WANT); + allocC = make_dispatch_alloc(JOBC_ID, WORKER_NAME, JOBC_WANT); + round1_decisions[0] = make_decision_dispatch(2000ULL, allocA); + round1_decisions[1] = make_decision_dispatch(2000ULL, allocC); + round1_decisions[2] = make_decision_pend(2000ULL, JOBB_ID, 0); + append_replay_session(f, 2000ULL, round1_decisions, 3); + + if (upto_round < 2) { + return; + } + + /* Round 2: idle filler -- no new events. A and C are still PEND; + * proves the deferred-zero state from round 1 persists across an + * extra round without a spurious re-capture attempt. */ + filler_decisions[0] = make_decision_pend(3000ULL, JOBB_ID, 0); + append_replay_session(f, 3000ULL, filler_decisions, 1); + + if (upto_round < 3) { + return; + } + + /* Round 3: a SECOND idle filler round, widening the gap before A's + * confirmation below to genuinely "several rounds", not just one. */ + filler_decisions[0] = make_decision_pend(4000ULL, JOBB_ID, 0); + append_replay_session(f, 4000ULL, filler_decisions, 1); + + if (upto_round < 4) { + return; + } + + /* Round 4: job A's genuine worker-confirmed RUN transition, recorded 3 + * rounds after its own round-1 DISPATCH decision -- the realistic + * dispatch-to-confirmation latency this spec exists to fix. */ + append_job_state_change_event(f, 4500ULL, JOBA_ID, JOB_STAT_PEND, + JOB_STAT_RUN); + filler_decisions[0] = make_decision_pend(5000ULL, JOBB_ID, 0); + append_replay_session(f, 5000ULL, filler_decisions, 1); + + if (upto_round < 5) { + return; + } + + /* Round 5: job C's genuine worker-confirmed RUN transition, recorded 4 + * rounds after ITS OWN round-1 DISPATCH decision -- a deliberately + * DIFFERENT round-separation than job A's (3 rounds), proving this + * isn't a single-job coincidence. */ + append_job_state_change_event(f, 5500ULL, JOBC_ID, JOB_STAT_PEND, + JOB_STAT_RUN); + filler_decisions[0] = make_decision_pend(6000ULL, JOBB_ID, 0); + append_replay_session(f, 6000ULL, filler_decisions, 1); +} + +/* Builds+runs a truncated copy of write_multiround_contention_rounds()'s + * fixture (up to `upto_round`) and returns the resulting driver state via + * `*sim_out` -- shared setup for all three checkpoints below. `label` is + * only used to keep each checkpoint's tmp dir/file name distinct. */ +static void run_multiround_contention_fixture(xce_sched_sim_driver_t* sim_out, + int upto_round, + const char* label) +{ + char record_dir[PATH_MAX]; + xce_scheduler_t sched_seed; + FILE* f; + char r_path[PATH_MAX]; + sched_sim_args_t sim_args; + int rc; + + make_tmp_dir(record_dir, sizeof(record_dir), label); + fixture_init_seed(&sched_seed, record_dir); + + rc = sched_record_start(&sched_seed, "contention_mr.replay"); + TEST_ASSERT_EQUAL_INT(0, rc); + TEST_ASSERT_TRUE(strlen(sched_seed.record_file_path) < sizeof(r_path)); + strcpy(r_path, sched_seed.record_file_path); + f = sched_seed.record_file; + + write_multiround_contention_rounds(f, upto_round); + append_end_of_stream_marker(f); + fclose(f); + + memset(&sim_args, 0, sizeof(sim_args)); + sim_args.replay_path = r_path; + sim_args.sim_start_ts = SCHED_SIM_START_TS_LATEST; + sim_args.sim_speed = 0; /* frozen -- deterministic, no wall-clock skew */ + + memset(sim_out, 0, sizeof(*sim_out)); + rc = sched_sim_driver_run(sim_out, &sim_args); + TEST_ASSERT_EQUAL_INT(0, rc); +} + +/* Runs a real sched_check_host_satisfy() capacity check against `sim`'s + * post-replay worker-a state for a hypothetical job wanting `want` units + * of "slots" -- returns sched_check_host_satisfy()'s own rc (1 == would be + * granted, 0 == would be denied). Same technique (xce_scheduler_t wrapper + * carrying a copy of sim->core) the test above already establishes. */ +static int check_host_satisfy_want(xce_sched_sim_driver_t* sim, float want) +{ + xce_scheduler_t wrapper; + xce_sched_worker_t* w; + xce_resource_t* r = NULL; + int residx = -1; + xvec_t uncomitted; + xvec_idval_t wanted; + xvec_idval_t unmeet; + int rc; + + w = sched_get_worker(&(sim->core), (char*)WORKER_NAME); + TEST_ASSERT_NOT_NULL(w); + + rc = key_find(sim->core.resmgr, (void*)SLOTS_RESNAME, (void**)&r); + TEST_ASSERT_TRUE(rc >= 0); + TEST_ASSERT_NOT_NULL(r); + residx = r->idx; + + memset(&wrapper, 0, sizeof(wrapper)); + wrapper.core = sim->core; + + rc = xvec_init(&uncomitted, 1, sizeof(float)); + TEST_ASSERT_TRUE(rc >= 0); + rc = xvec_idval_init(&wanted, 1); + TEST_ASSERT_EQUAL_INT(0, rc); + rc = xvec_idval_store(&wanted, residx, want); + TEST_ASSERT_TRUE(rc >= 0); + + rc = sched_check_host_satisfy(&wrapper, w, &uncomitted, &wanted, &unmeet); + + xvec_fini(&uncomitted); + xvec_idval_fini(&wanted); + xvec_idval_fini(&unmeet); + + return rc; +} + +static void +test_contention_capacity_check_correct_across_multi_round_dispatch_confirm_gap( + void) +{ + static xce_sched_sim_driver_t sim1; + static xce_sched_sim_driver_t sim4; + static xce_sched_sim_driver_t sim5; + int rc; + + g_capturing = 0; + g_round_idx = 0; + + /* ---- Checkpoint 1: right after round 1 -- A and C are dispatched but + * still PEND (neither has reached RUN yet). Capacity must look FULLY + * free: a hypothetical job wanting the entire WORKER_CAPACITY must be + * granted, since nothing is genuinely running yet (Requirement 1.2, at + * the capacity-check OUTCOME level, not just a raw resInUse read). ---- */ + run_multiround_contention_fixture(&sim1, 1, "_mr1"); + rc = check_host_satisfy_want(&sim1, WORKER_CAPACITY); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 1, rc, + "round 1: A and C are dispatched-but-still-PEND -- capacity must " + "look FULLY free (a job wanting the entire WORKER_CAPACITY must be " + "granted), since neither job has genuinely reached RUN yet " + "(Requirement 1.2)"); + + /* ---- Checkpoint 2: right after round 4 -- job A confirmed RUN, 3 + * rounds after its own round-1 DISPATCH; job C still PEND. Capacity + * must reflect PARTIAL usage: exactly JOBC_WANT (2.0) of headroom + * remains -- a job wanting exactly that must be granted, one wanting + * more (JOBB_WANT, 5.0) must be denied. This is the strongest single + * proof that combined accounting is correct, not just "some job got + * counted": an implementation that dropped job A's contribution (the + * OLD same-round-only bug) would wrongly grant JOBB_WANT here too. ---- */ + run_multiround_contention_fixture(&sim4, 4, "_mr4"); + rc = check_host_satisfy_want(&sim4, JOBC_WANT); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 1, rc, + "round 4: only job A (8.0) is genuinely RUN -- exactly JOBC_WANT " + "(2.0) of headroom must remain and must be granted"); + rc = check_host_satisfy_want(&sim4, JOBB_WANT); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 0, rc, + "round 4: JOBB_WANT (5.0) exceeds the 2.0 remaining headroom -- " + "must be denied. A capacity check that wrongly dropped job A's " + "already-RUN 8.0 usage (the OLD same-round-only bug) would wrongly " + "grant this"); + + /* ---- Checkpoint 3: the full 5-round replay -- per-round resInUse + * snapshots (Requirement 1.1-1.4 across EVERY round, not just + * endpoints) plus a final capacity check proving FULL saturation once + * both jobs are genuinely RUN. ---- */ + g_capturing = 1; + g_round_idx = 0; + run_multiround_contention_fixture(&sim5, 5, "_mr5"); + g_capturing = 0; + + TEST_ASSERT_EQUAL_INT_MESSAGE( + 5, g_round_idx, + "sched_account_jobs() must have been called exactly once per " + "replayed round (5 rounds in this fixture)"); + + TEST_ASSERT_EQUAL_FLOAT_MESSAGE( + 0.0f, g_round_resinuse[0], + "round 1: A/C dispatched but still PEND -- resInUse must read 0.0, " + "not fabricated or prematurely charged (Requirement 1.2)"); + TEST_ASSERT_EQUAL_FLOAT_MESSAGE( + 0.0f, g_round_resinuse[1], + "round 2: idle filler round -- the deferred-zero state from round " + "1 must persist, not drift, across an EXTRA round"); + TEST_ASSERT_EQUAL_FLOAT_MESSAGE( + 0.0f, g_round_resinuse[2], + "round 3: a SECOND idle filler round -- still 0.0, widening the " + "gap before A's confirmation below to genuinely several rounds"); + TEST_ASSERT_EQUAL_FLOAT_MESSAGE( + JOBA_WANT, g_round_resinuse[3], + "round 4: job A's JOB_STATE_CHANGE to RUN just replayed, 3 rounds " + "after A's own round-1 DISPATCH -- resInUse must now reflect A's " + "8.0 usage (Requirement 1.3), and ONLY A's -- job C is still PEND"); + TEST_ASSERT_EQUAL_FLOAT_MESSAGE( + WORKER_CAPACITY, g_round_resinuse[4], + "round 5: job C's JOB_STATE_CHANGE to RUN just replayed, 4 rounds " + "after C's own round-1 DISPATCH (a DIFFERENT separation than A's 3 " + "rounds, proving this isn't a single-job coincidence) -- resInUse " + "must now reflect the FULL combined 10.0 usage of both A and C " + "together"); + + rc = check_host_satisfy_want(&sim5, JOBD_WANT); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 0, rc, + "round 5: capacity is fully saturated by A+C (10.0 of 10.0) -- " + "even the smallest positive want (JOBD_WANT) must be denied. Under " + "the OLD same-round-only reconciliation gate, neither A nor C " + "would EVER have been captured (their DISPATCH and RUN-" + "confirmation rounds never coincide), so resInUse would read a " + "PERMANENTLY-ZERO 0.0 here and this check would WRONGLY grant " + "admission"); + + printf("test_contention_capacity_check_correct_across_multi_round_" + "dispatch_confirm_gap: PASS\n"); +} + int main(void) { UNITY_BEGIN(); RUN_TEST( test_contention_parity_matches_real_normal_mode_recording_round_by_round); + RUN_TEST( + test_contention_capacity_check_correct_across_multi_round_dispatch_confirm_gap); return UNITY_END(); }