From c67382e6f1646d07e1131835e8ddb4db37b38ad0 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Thu, 27 Aug 2026 04:34:06 +0000 Subject: [PATCH] test(scheduler-sim-eager-reconciliation): task 1.2 capture-safety unit coverage Adds 3 tests proving guarantees that must hold under the widened capture-eligibility gate: an already-captured job is never re-searched or re-captured regardless of state (test_reconcile_still_skips_already_ captured_job_regardless_of_state), a PEND job with no match this round stays uncaptured and remains eligible next round (test_reconcile_ no_match_leaves_pend_job_uncaptured_and_retries), and several simultaneously outstanding jobs are captured independently with zero cross-job interference (test_reconcile_multiple_jobs_captured_ independently_across_different_rounds). Required making the shared add_job() fixture helper tolerant of -EEXIST on a repeated queue registration (needed for the multi-job test, the first in this file to call add_job() twice against the same core) -- traced to kvtab_add()'s narrowly-scoped -EEXIST return, provably a no-op for all 11 pre-existing single-add_job()-call tests. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01AgYRU3zDTqNGV5Jk8aiKLg --- .../tasks.md | 2 +- .../test_sim_reconcile_unaccounted.c | 281 +++++++++++++++++- 2 files changed, 281 insertions(+), 2 deletions(-) diff --git a/.kiro/specs/scheduler-sim-eager-reconciliation/tasks.md b/.kiro/specs/scheduler-sim-eager-reconciliation/tasks.md index c0d73ee7..d3610934 100644 --- a/.kiro/specs/scheduler-sim-eager-reconciliation/tasks.md +++ b/.kiro/specs/scheduler-sim-eager-reconciliation/tasks.md @@ -9,7 +9,7 @@ - _Requirements: 1.1, 3.1_ - _Boundary: sim_reconcile_unaccounted_jobs_ -- [ ] 1.2 Extend unit coverage for capture-safety guarantees under the widened eligibility +- [x] 1.2 Extend unit coverage for capture-safety guarantees under the widened eligibility - Add a test proving a job whose allocation was already captured is never re-searched or re-captured on a later round, regardless of its state - Add a test proving a job with no matching recorded decision this round stays uncaptured and remains eligible to be found on a later round - Add a test with several simultaneously outstanding jobs whose recorded dispatch decisions land in different rounds, proving each job's captured allocation reflects only its own recorded decision with no cross-job interference diff --git a/src/scheduler/testsuite/test_sim_reconcile_unaccounted.c b/src/scheduler/testsuite/test_sim_reconcile_unaccounted.c index 1aeda31e..8811703c 100644 --- a/src/scheduler/testsuite/test_sim_reconcile_unaccounted.c +++ b/src/scheduler/testsuite/test_sim_reconcile_unaccounted.c @@ -260,6 +260,7 @@ static xce_sched_job_t* add_job(xce_sched_core_t* core, uint64_t jobid, job_submit_data_t submit; xce_job_t wire_job; xce_sched_job_t* job; + int queue_rc; memset(&wire_queue, 0, sizeof(wire_queue)); wire_queue.queue = (char*)"normal"; @@ -267,7 +268,22 @@ static xce_sched_job_t* add_job(xce_sched_core_t* core, uint64_t jobid, wire_queue.priority = 1; q = core_build_queue(&wire_queue); TEST_ASSERT_NOT_NULL(q); - TEST_ASSERT_GREATER_OR_EQUAL(0, sched_add_queue(core, q)); + /* Tolerate -EEXIST: a test placing MULTIPLE jobs on the same core (task + * 1.2's own multi-job isolation test is the first here to do so) calls + * add_job() more than once against the same `core`, and the "normal" + * queue this helper (re-)builds every call is already registered from + * the first call -- that is expected, not a fixture bug, so only a + * genuine failure (neither success nor EEXIST) is a hard assertion + * failure. The freshly-built-but-unused `q` on the EEXIST path is + * simply leaked, matching this whole suite's own "fixtures are not + * torn down, detect_leaks=0" convention documented in this file's own + * header comment. */ + queue_rc = sched_add_queue(core, q); + TEST_ASSERT_TRUE_MESSAGE( + (queue_rc >= 0) || (queue_rc == -EEXIST), + "sched_add_queue must succeed, or fail with -EEXIST when this " + "fixture core's \"normal\" queue was already registered by an " + "earlier add_job() call against the same core"); memset(&submit, 0, sizeof(submit)); submit.queue = (char*)"normal"; @@ -1102,6 +1118,264 @@ test_captured_pend_job_excluded_from_resinuse_until_run(void) "test_captured_pend_job_excluded_from_resinuse_until_run: PASS\n"); } +/* ---------------------------------------------------------------------- */ +/* Test 13 (scheduler-sim-eager-reconciliation task 1.2, Requirement 3.2): */ +/* a job with job->accounts ALREADY set is never re-searched or */ +/* re-captured on a later round's call, regardless of its state (PEND */ +/* here, deliberately different from test_reconcile_skips_job_that_ */ +/* already_has_accounts' RUN-state fixture above -- the state genuinely */ +/* does not matter, only job->accounts != NULL does) -- even when a */ +/* decoded session that round happens to also contain a matching-jobid */ +/* DISPATCH entry. Proves the state-widened gate (task 1.1) did not */ +/* weaken the pre-existing, untouched job->accounts != NULL guard */ +/* (research.md's "Does removing the RUN-state gate risk fabricating or */ +/* double-capturing?" log entry: this guard was never touched by task */ +/* 1.1, so this is regression-proof coverage for an existing guarantee, */ +/* not new-behavior coverage). */ +/* ---------------------------------------------------------------------- */ + +static void +test_reconcile_still_skips_already_captured_job_regardless_of_state(void) +{ + xce_sched_sim_driver_t sim; + xce_sched_job_t* job; + xce_job_account_t* accounts_before; + xce_job_alloc_t* allocs_before; + replay_session_t* decoded_round1; + replay_session_t* decoded_round2; + + fixture_init(&sim); + + /* PEND-state, already captured (mirrors add_job()'s own "already + * accounted" fixture shape, same technique + * test_reconcile_skips_job_that_already_has_accounts() uses -- here + * deliberately with a non-RUN state). */ + job = add_job(&(sim.core), 7040ULL, JOB_STAT_PEND, + make_job_alloc(7040ULL, 0, NULL)); + TEST_ASSERT_NOT_NULL_MESSAGE( + job->accounts, "fixture setup: job must already have accounts"); + accounts_before = job->accounts; + allocs_before = job->allocs; + + /* Round 1: nothing to do with this job either way -- establishes the + * "later round" framing the next call demonstrates. */ + decoded_round1 = make_decoded_session(4000ULL, NULL); + sim_reconcile_unaccounted_jobs(&sim, decoded_round1, NULL); + TEST_ASSERT_EQUAL_PTR(accounts_before, job->accounts); + + /* Round 2 ("a later round"): a recorded DISPATCH entry for THIS job's + * own jobid is present. If the job->accounts != NULL guard were ever + * bypassed under the widened, state-independent trigger, this would be + * re-captured. It must not be. */ + decoded_round2 + = make_decoded_session(5000ULL, make_dispatch_entry(7040ULL)); + sim_reconcile_unaccounted_jobs(&sim, decoded_round2, NULL); + + TEST_ASSERT_EQUAL_PTR_MESSAGE( + accounts_before, job->accounts, + "an already-captured PEND-state job must never be re-captured on a " + "later round, even with a matching-jobid recorded DISPATCH entry " + "present -- job->accounts != NULL remains the sole gate under the " + "widened, state-independent trigger"); + TEST_ASSERT_EQUAL_PTR_MESSAGE( + allocs_before, job->allocs, + "job->allocs likewise must stay exactly what it already was -- " + "never swapped for the later round's entry"); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 1, decoded_round2->n_decisions, + "the matching entry must not be consumed either -- the trigger " + "never fires for an already-captured job at all"); + + printf("test_reconcile_still_skips_already_captured_job_regardless_of_" + "state: PASS\n"); +} + +/* ---------------------------------------------------------------------- */ +/* Test 14 (scheduler-sim-eager-reconciliation task 1.2, Requirement 4.4): */ +/* a JOB_STAT_PEND job with NO matching recorded decision this round stays */ +/* uncaptured (job->accounts stays NULL) after one call, and remains */ +/* eligible -- demonstrated (per this task's own design.md guidance) by */ +/* calling again with a NOW-matching decoded session and confirming */ +/* capture succeeds then. Mirrors test_reconcile_is_safe_to_recheck_on_a_ */ +/* later_round()'s "safe to retry" idiom above, but (unlike that sibling */ +/* test, whose second round also has no match) this is the genuinely new */ +/* behavior task 1.1 enables: a PEND job now gets multiple real chances */ +/* across rounds and can actually succeed on a later one. */ +/* ---------------------------------------------------------------------- */ + +static void +test_reconcile_no_match_leaves_pend_job_uncaptured_and_retries(void) +{ + xce_sched_sim_driver_t sim; + xce_sched_job_t* job; + xce_job_alloc_t* jalloc; + replay_session_t* decoded_round1; + replay_session_t* decoded_round2; + + fixture_init(&sim); + + /* PEND-state, no known allocation, no recorded decision available yet + * -- the realistic in-flight case task 1.1's widened gate exists for. */ + job = add_job(&(sim.core), 7041ULL, JOB_STAT_PEND, NULL); + TEST_ASSERT_NULL(job->accounts); + TEST_ASSERT_NULL(job->allocs); + + decoded_round1 = make_decoded_session(4100ULL, NULL); + sim_reconcile_unaccounted_jobs(&sim, decoded_round1, NULL); + + TEST_ASSERT_NULL_MESSAGE( + job->accounts, + "no matching recorded decision this round -- a PEND job must stay " + "uncaptured (usage unavailable), never fabricated"); + TEST_ASSERT_NULL_MESSAGE(job->allocs, "job->allocs likewise stays NULL"); + + /* Round 2: the job's recorded DISPATCH decision now shows up. The job + * remains eligible (was never given up on) -- capture succeeds this + * time, proving the previous failed attempt left no "give up" state + * behind. */ + jalloc = make_test_job_alloc_with_host(7041ULL, "workerB", "slots", 1.0f); + decoded_round2 = make_decoded_session( + 4200ULL, make_dispatch_entry_from_alloc(jalloc)); + sim_reconcile_unaccounted_jobs(&sim, decoded_round2, NULL); + + TEST_ASSERT_EQUAL_PTR_MESSAGE( + jalloc, job->allocs, + "a job that stayed uncaptured with no match must remain fully " + "eligible on a later round -- capture succeeds as soon as a " + "matching recorded decision appears"); + TEST_ASSERT_NOT_NULL_MESSAGE( + job->accounts, "job->accounts must be built once the retry finds a " + "match"); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 0, decoded_round2->n_decisions, + "the now-matched entry must be consumed exactly like any other " + "capture"); + + printf("test_reconcile_no_match_leaves_pend_job_uncaptured_and_retries: " + "PASS\n"); +} + +/* ---------------------------------------------------------------------- */ +/* Test 15 (scheduler-sim-eager-reconciliation task 1.2, Requirement 1.1, */ +/* 3.1 under concurrent load): several simultaneously outstanding jobs, */ +/* each with a DIFFERENT jobid, spread across three scenarios a single */ +/* reconcile call must resolve with zero cross-job interference: */ +/* - job_matched: PEND, its own DISPATCH entry IS this round. */ +/* - job_no_match: PEND, no recorded entry at all this round. */ +/* - job_already: captured on a hypothetical EARLIER round already */ +/* (job->accounts/job->allocs preset directly via add_job()'s own */ +/* "already accounted" shape, same technique */ +/* test_reconcile_skips_job_that_already_has_accounts() and Test 13 */ +/* above use) -- AND, to make the "no accidental overwrite" claim */ +/* non-trivial, a matching-jobid DISPATCH entry for it is ALSO */ +/* present this round. */ +/* Asserts each job's own job->accounts reflects ONLY that job's own */ +/* recorded decision, with no cross-job data or overwrite (the single-job */ +/* tests above cannot distinguish "correct" from "correct by coincidence, */ +/* only one candidate available", per design.md's own Testing Strategy). */ +/* ---------------------------------------------------------------------- */ + +static void +test_reconcile_multiple_jobs_captured_independently_across_different_rounds( + void) +{ + xce_sched_sim_driver_t sim; + xce_sched_job_t* job_matched; + xce_sched_job_t* job_no_match; + xce_sched_job_t* job_already; + xce_job_account_t* already_accounts_before; + xce_job_alloc_t* already_allocs_before; + xce_job_alloc_t* jalloc_matched; + xce_job_alloc_t* jalloc_already_new; + replay_session_t* decoded; + + fixture_init(&sim); + + job_matched = add_job(&(sim.core), 7050ULL, JOB_STAT_PEND, NULL); + job_no_match = add_job(&(sim.core), 7051ULL, JOB_STAT_PEND, NULL); + job_already = add_job(&(sim.core), 7052ULL, JOB_STAT_RUN, + make_job_alloc(7052ULL, 0, NULL)); + TEST_ASSERT_NOT_NULL_MESSAGE( + job_already->accounts, + "fixture setup: job_already must already have accounts, " + "simulating capture on an earlier round"); + already_accounts_before = job_already->accounts; + already_allocs_before = job_already->allocs; + + jalloc_matched + = make_test_job_alloc_with_host(7050ULL, "workerA", "slots", 5.0f); + jalloc_already_new = make_test_job_alloc_with_host( + 7052ULL, "worker-should-not-apply", "slots", 99.0f); + + decoded = (replay_session_t*)calloc(1, sizeof(*decoded)); + decoded->begin_round = 6000ULL; + decoded->summary = NULL; + decoded->decisions + = (replay_decision_t**)calloc(2, sizeof(*(decoded->decisions))); + decoded->decisions[0] = make_dispatch_entry_from_alloc(jalloc_matched); + decoded->decisions[1] + = make_dispatch_entry_from_alloc(jalloc_already_new); + decoded->n_decisions = 2; + /* job_no_match (7051) intentionally has no entry in this round's + * decoded stream at all. */ + + sim_reconcile_unaccounted_jobs(&sim, decoded, NULL); + + /* job_matched: captured, and reflects EXACTLY its own jobid's data. */ + TEST_ASSERT_EQUAL_PTR_MESSAGE( + jalloc_matched, job_matched->allocs, + "job_matched must capture its own recorded DISPATCH entry"); + TEST_ASSERT_NOT_NULL(job_matched->accounts); + TEST_ASSERT_EQUAL_STRING_MESSAGE( + "workerA", job_matched->accounts->accounts[0]->host, + "job_matched's captured account must reflect its OWN entry's " + "host, not another job's"); + TEST_ASSERT_EQUAL_FLOAT_MESSAGE( + 5.0f, job_matched->accounts->accounts[0]->accounts[0]->reserved, + "job_matched's captured account must reflect its OWN entry's " + "reserved value, not another job's"); + + /* job_no_match: stays uncaptured -- no entry existed for it. */ + TEST_ASSERT_NULL_MESSAGE( + job_no_match->accounts, + "job_no_match has no matching recorded entry this round -- must " + "stay uncaptured"); + TEST_ASSERT_NULL(job_no_match->allocs); + + /* job_already: completely unchanged, despite a matching-jobid DISPATCH + * entry for it being present this round -- proving no accidental + * overwrite/cross-contamination from the wider per-round scan. */ + TEST_ASSERT_EQUAL_PTR_MESSAGE( + already_accounts_before, job_already->accounts, + "job_already's accounts must be the IDENTICAL pointer from before " + "this call -- never overwritten, even with a matching entry " + "present this round"); + TEST_ASSERT_EQUAL_PTR_MESSAGE( + already_allocs_before, job_already->allocs, + "job_already's allocs must likewise be the IDENTICAL pointer from " + "before this call"); + + /* job_already's own matching entry was never even searched for (its + * job->accounts != NULL guard skips it before the search begins), so + * it must remain fully intact and unconsumed in decoded_session -- only + * job_matched's entry was consumed. */ + TEST_ASSERT_EQUAL_INT_MESSAGE( + 1, decoded->n_decisions, + "exactly one entry (job_matched's) must be consumed -- " + "job_already's entry is left untouched since that job was never " + "eligible to be searched for at all"); + TEST_ASSERT_EQUAL_UINT64_MESSAGE( + 7052ULL, decoded->decisions[0]->body.dispatch->alloc->jobid, + "the remaining entry must be job_already's own, untouched, entry"); + TEST_ASSERT_EQUAL_PTR_MESSAGE( + jalloc_already_new, decoded->decisions[0]->body.dispatch->alloc, + "job_already's entry's alloc must be the SAME pointer it started " + "with -- never taken over, since that job was already captured"); + + printf("test_reconcile_multiple_jobs_captured_independently_across_" + "different_rounds: PASS\n"); +} + int main(void) { UNITY_BEGIN(); @@ -1120,5 +1394,10 @@ int main(void) test_reconcile_reports_reconstructed_decision_through_existing_channel); RUN_TEST(test_reconcile_null_args_are_a_safe_noop); RUN_TEST(test_captured_pend_job_excluded_from_resinuse_until_run); + RUN_TEST( + test_reconcile_still_skips_already_captured_job_regardless_of_state); + RUN_TEST(test_reconcile_no_match_leaves_pend_job_uncaptured_and_retries); + RUN_TEST( + test_reconcile_multiple_jobs_captured_independently_across_different_rounds); return UNITY_END(); }