diff --git a/.kiro/specs/scheduler-dispatch-commit/tasks.md b/.kiro/specs/scheduler-dispatch-commit/tasks.md index 788b73ca..8e3f122c 100644 --- a/.kiro/specs/scheduler-dispatch-commit/tasks.md +++ b/.kiro/specs/scheduler-dispatch-commit/tasks.md @@ -71,7 +71,7 @@ - _Depends: 3.1, 3.2, 3.3_ - [ ] 5. Validation: end-to-end behavior and regression -- [ ] 5.1 End-to-end test: resource-in-use accounting survives a multi-round gap between dispatch and confirmation +- [x] 5.1 End-to-end test: resource-in-use accounting survives a multi-round gap between dispatch and confirmation - Reproduce bug-1624 directly: dispatch a job, let several rounds pass before the worker confirms it started, and confirm resource-in-use totals reflect the job continuously from the round it was dispatched, matching what a same-round confirmation would have produced - Observable completion: the regression scenario passes, and fails against the pre-change code path if temporarily reverted - _Requirements: 1.1, 1.2, 1.3, 2.1, 2.3_ @@ -114,3 +114,5 @@ - Task 3.3: the bound constant is `XCE_DISPATCH_FAIL_RETRY_BOUND = 3` (design.md's stated default), defined once near job.c's top and used at all four retryable sites — no magic numbers. 3 consecutive failures still retry; the 4th resolves terminal. - Task 4.1: **`test_commit_barrier_reply.c`'s `test_jobdispatch_receipt_ack_no_wal_append` is now stale for a SECOND, independent reason**, on top of task 3.1's already-documented WAL-append-count staleness: its 2-job batch (one real-worker success, one `nhosts==0` terminal failure — no retryable job) now produces ZERO `notify_dispatch()` calls total post-task-4.1 (the success path never called it even before task 4.1, per task 3.1's design; the terminal path resolves via `mark_job_fail()`'s `schedFailJob` hook chain, not `notify_dispatch()`/`jobDispatchFail`, per task 3.2's design) — breaking its pinned `g_notify_count == 2` / both-`jobDispatchFail`-event assertions (lines ~1991-2003). **Task 5.4 must update BOTH the WAL-count assertion (task 3.1's finding) and the notify-count/event assertions (this finding) in the same test** before claiming full-suite regression-clean; the test's own scenario (1 success + 1 terminal, zero retryable jobs) may need to change too since it can no longer meaningfully exercise `notify_dispatch()` at all with today's cast of outcomes — consider adding a retryable-classified job to the batch if the intent is still to prove `notify_dispatch()`'s wiring through `handle_jobdispatch()`. - Task 4.1: confirmed `handle_jobdispatch()`'s only caller (`cluster.c:471`, inside `myreq_handler`) uses its return value only as the generic RPC-handler `rc`, and `handle_jobdispatch()` itself always returns 0 unconditionally regardless of any job's outcome — nothing depends on the removed per-job notification behavior at the caller level. +- Task 5.1: a "same-round confirmation" scenario for resource-in-use accounting is legitimately modeled as `nrounds==1` rather than by simulating a real `schedStartJob` notify event — design.md documents (and the reviewer independently re-verified against `core_events.c`) that re-applying `JOB_STAT_RUN` to an already-RUN job via the confirmation path is a confirmed no-op, so `resInUse` depends solely on `JOB_IS_RUN(job->state)`, never on a separate confirmation flag. Reviewer's own finding: the resulting same-round-vs-multi-round comparison test is honest and correctly fails on revert, but adds little independent detection power beyond a per-round-asserting loop test (which is the one genuinely load-bearing proof) — worth knowing if a future task considers deduplicating these two tests, but not something to act on now. +- Task 5.1: no controller-side test was added for this task — Requirements 2.1/2.3 (controller transitions to RUN independent of confirmation) are already covered by task 3.1's `test_dc_success_commits_alloc_state_and_resets_counter`. No live scheduler+controller wire-protocol integration harness exists anywhere in this codebase; building one was judged disproportionate to what this task verifies, and the reviewer agreed. diff --git a/src/scheduler/testsuite/test_scheduler_dispatch_commit.c b/src/scheduler/testsuite/test_scheduler_dispatch_commit.c index d5f7ee5f..3a1b3969 100644 --- a/src/scheduler/testsuite/test_scheduler_dispatch_commit.c +++ b/src/scheduler/testsuite/test_scheduler_dispatch_commit.c @@ -19,6 +19,24 @@ * threads), so it needs none of their `-Wl,--wrap=xcecli_*` interposition: * sched_commit_decision() itself only touches sched->core and the (empty, * in this fixture) plugin-hook chain, never sched->apictx. + * + * Extended for task 5.1 (Req 1.1, 1.2, 1.3, 2.1, 2.3 -- bug-1624's + * end-to-end regression test): task 2.1's own single-round-boundary test + * above proves the core mechanism; the two tests added at the bottom of + * this file extend that proof to (a) GAP_ROUNDS=5 consecutive round + * boundaries rather than just one, and (b) an explicit value comparison + * between a "confirmed same round" scenario and a "confirmed after several + * rounds" scenario, rather than only comparing each to a hardcoded + * constant -- see run_dispatch_then_n_rounds()'s own comment for why no + * live controller connection or simulated schedStartJob notify event is + * needed to make that comparison meaningful: the controller's own role in + * this flow (transitioning to RUN, notifying) is already independently + * covered by tasks 3.1-4.1's controller-side suites + * (test_dispatch_commit.c, test_dispatch_fail_terminal.c, + * test_dispatch_fail_retry.c, test_jobdispatch_no_double_notify.c); nothing + * scheduler-side that determines resInUse ever reads a confirmation event + * at all, only job->state, which task 2.1's fix already sets synchronously + * at dispatch-commit. */ #include #include @@ -34,6 +52,7 @@ #define JOB_ID 4242ULL #define RES_NAME "slots" #define WANT_VALUE 4.0f +#define GAP_ROUNDS 5 static xce_sched_worker_t* add_worker(xce_sched_core_t* core, char* name) { @@ -253,11 +272,177 @@ static void test_pend_decision_does_not_touch_job_state(void) TEST_ASSERT_NULL(job->accounts); } +/* + * Task 5.1 (bug-1624's own regression test, "several rounds" language): + * extends the single-round-boundary proof above + * (test_dispatch_commit_survives_round_boundary_before_confirmation, task + * 2.1) to GAP_ROUNDS consecutive round boundaries -- zeroing resInUse and + * calling sched_account_jobs() GAP_ROUNDS times in a row, exactly as + * GAP_ROUNDS real driver rounds would via xce_core_begin_session(), with no + * worker-confirmation event injected anywhere in the loop -- and asserts + * resInUse is restored to WANT_VALUE after EVERY one of them, not merely + * that it eventually comes back after one round. Before task 2.1/3.1's fix, + * job->state would still read JOB_STAT_PEND at commit time, so this would + * already fail at round 1 (the same way the pre-existing single-round test + * would); this test additionally proves the fix holds continuously across + * a realistic multi-round confirmation gap, not just a single round + * boundary. + */ +static void test_dispatch_commit_survives_several_rounds_before_confirmation(void) +{ + xce_scheduler_t sched; + xce_sched_worker_t* w; + xce_sched_job_t* job; + xce_job_alloc_t* alloc; + xce_decision_t* decision; + xce_resource_t* vres = NULL; + int rc; + int residx; + float val; + float zero[1] = { 0.0f }; + int round; + + memset(&sched, 0, sizeof(sched)); + rc = xce_sched_core_init(&(sched.core)); + TEST_ASSERT_EQUAL_INT(0, rc); + + w = add_worker(&(sched.core), WORKER_NAME); + job = add_pending_job(&(sched.core), JOB_ID); + + residx = add_vres(&vres, sched.core.resmgr, RES_NAME, 0); + TEST_ASSERT_TRUE(residx >= 0); + + alloc = make_dispatch_alloc(JOB_ID, WORKER_NAME, WANT_VALUE); + decision = make_dispatch_decision(alloc); + TEST_ASSERT_NOT_NULL(decision); + + rc = sched_commit_decision(&sched, decision); + TEST_ASSERT_TRUE(rc >= 0); + TEST_ASSERT_EQUAL_UINT(JOB_STAT_RUN, job->state); + + get_resvals(&(w->resInUse), 1, &residx, &val); + TEST_ASSERT_EQUAL_FLOAT(WANT_VALUE, val); + + for (round = 0; round < GAP_ROUNDS; round++) { + /* still no schedStartJob/confirmation event anywhere in this loop + * -- job->accounts/job->state were set once, at dispatch commit, + * and nothing else re-arms them between rounds. */ + put_resvals(&(w->resInUse), 1, &residx, zero); + get_resvals(&(w->resInUse), 1, &residx, &val); + TEST_ASSERT_EQUAL_FLOAT(0.0f, val); + + rc = sched_account_jobs(&(sched.core)); + TEST_ASSERT_EQUAL_INT(0, rc); + + get_resvals(&(w->resInUse), 1, &residx, &val); + TEST_ASSERT_EQUAL_FLOAT(WANT_VALUE, val); + } +} + +/* + * Shared fixture for the comparison test below: builds a fresh core + + * worker + dispatched job (same shape as the tests above), then simulates + * `nrounds` successive round boundaries -- each zeroing resInUse exactly as + * xce_core_begin_session() does every round, then re-deriving it via + * sched_account_jobs() -- with NO worker-confirmation event injected at any + * point, and returns the worker's final resInUse value for RES_NAME. + * + * `nrounds == 1` stands in for "the worker's confirmation would have + * arrived the very same round dispatch was committed": since resInUse for + * a RUN job is fully determined by sched_account_jobs()'s JOB_IS_RUN() + * gate (unmodified by this feature) and job->state is already RUN + * synchronously at dispatch-commit (Requirement 1.1), a same-round + * confirmation event applying JOB_STAT_RUN again is a confirmed no-op + * against an already-RUN job (design.md, "Scheduler Dispatch Commit" + * Responsibilities & Constraints) -- it changes nothing that + * sched_account_jobs() reads. So the very next round's accounting pass + * already reflects exactly what a same-round confirmation would have + * produced; there is nothing further for an explicit confirmation event to + * contribute to this value, which is why simulating that event is not + * needed here to make the comparison meaningful. + * + * `nrounds > 1` simulates however many additional rounds elapse with still + * no confirmation in between. Per Requirement 1.2, the two must be + * identical -- asserted explicitly by + * test_dispatch_commit_multi_round_gap_matches_same_round_confirmation + * below, not merely by separately comparing each against a hardcoded + * constant. + */ +static float run_dispatch_then_n_rounds(int nrounds) +{ + xce_scheduler_t sched; + xce_sched_worker_t* w; + xce_sched_job_t* job; + xce_job_alloc_t* alloc; + xce_decision_t* decision; + xce_resource_t* vres = NULL; + int rc; + int residx; + float val; + float zero[1] = { 0.0f }; + int i; + + memset(&sched, 0, sizeof(sched)); + rc = xce_sched_core_init(&(sched.core)); + TEST_ASSERT_EQUAL_INT(0, rc); + + w = add_worker(&(sched.core), WORKER_NAME); + job = add_pending_job(&(sched.core), JOB_ID); + + residx = add_vres(&vres, sched.core.resmgr, RES_NAME, 0); + TEST_ASSERT_TRUE(residx >= 0); + + alloc = make_dispatch_alloc(JOB_ID, WORKER_NAME, WANT_VALUE); + decision = make_dispatch_decision(alloc); + TEST_ASSERT_NOT_NULL(decision); + + rc = sched_commit_decision(&sched, decision); + TEST_ASSERT_TRUE(rc >= 0); + TEST_ASSERT_EQUAL_UINT(JOB_STAT_RUN, job->state); + + for (i = 0; i < nrounds; i++) { + put_resvals(&(w->resInUse), 1, &residx, zero); + rc = sched_account_jobs(&(sched.core)); + TEST_ASSERT_EQUAL_INT(0, rc); + } + + get_resvals(&(w->resInUse), 1, &residx, &val); + return val; +} + +/* + * Task 5.1's explicit "matching what a same-round confirmation would have + * produced" language: directly compares the resInUse value produced after + * only ONE round boundary since dispatch (run_dispatch_then_n_rounds(1) -- + * see its own comment for why this stands in for "confirmed the same + * round") against the value produced after GAP_ROUNDS round boundaries + * with still no confirmation in between. Per Requirement 1.2, a job's + * contribution to resInUse must not depend on how many rounds elapse + * before confirmation -- both values must be identical, and both must + * equal WANT_VALUE. This is deliberately a value comparison against a + * second, independently-computed scenario, not merely two separate + * comparisons against the same hardcoded constant. + */ +static void test_dispatch_commit_multi_round_gap_matches_same_round_confirmation(void) +{ + float same_round_val; + float multi_round_gap_val; + + same_round_val = run_dispatch_then_n_rounds(1); + multi_round_gap_val = run_dispatch_then_n_rounds(GAP_ROUNDS); + + TEST_ASSERT_EQUAL_FLOAT(WANT_VALUE, same_round_val); + TEST_ASSERT_EQUAL_FLOAT(WANT_VALUE, multi_round_gap_val); + TEST_ASSERT_EQUAL_FLOAT(same_round_val, multi_round_gap_val); +} + int main(void) { UNITY_BEGIN(); RUN_TEST(test_dispatch_commit_transitions_job_to_run); RUN_TEST(test_dispatch_commit_survives_round_boundary_before_confirmation); + RUN_TEST(test_dispatch_commit_survives_several_rounds_before_confirmation); + RUN_TEST(test_dispatch_commit_multi_round_gap_matches_same_round_confirmation); RUN_TEST(test_pend_decision_does_not_touch_job_state); return UNITY_END(); }