From ff05c37bcaf8cb181b30e73eeea15d41c20c3b5e Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Thu, 3 Sep 2026 06:56:25 +0000 Subject: [PATCH] test(scheduler-sim-reconcile-terminal-guard): task 1.1 - RED tests for terminal guard Adds two failing tests formalizing bug-1764's reproduction: a job driven to JOB_STAT_DONE (and, separately, JOB_STAT_EXIT) via a real xce_core_push_events() JOB_STATE_CHANGE call correctly has its allocs/accounts nulled by the existing end-of-life cleanup, but sim_reconcile_unaccounted_jobs() then wrongly resurrects both fields when presented with a stray DISPATCH entry for that same jobid, since its trigger checks only job->accounts == NULL with no state check. The pre-existing eager-capture regression test, test_reconcile_captures_non_run_state_job_when_dispatch_recorded, already covers Requirement 1.2's "PEND job's early DISPATCH still captured" case explicitly and needed no duplicate. No production code touched. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01YAWMhfRJFnmmnG8h97yUDd --- .../tasks.md | 2 +- .../test_sim_reconcile_unaccounted.c | 192 +++++++++++++++++- 2 files changed, 192 insertions(+), 2 deletions(-) diff --git a/.kiro/specs/scheduler-sim-reconcile-terminal-guard/tasks.md b/.kiro/specs/scheduler-sim-reconcile-terminal-guard/tasks.md index 855801d6..6590dc3d 100644 --- a/.kiro/specs/scheduler-sim-reconcile-terminal-guard/tasks.md +++ b/.kiro/specs/scheduler-sim-reconcile-terminal-guard/tasks.md @@ -1,7 +1,7 @@ # Implementation Plan - [ ] 1. Core: terminal-state guard on job reconciliation -- [ ] 1.1 Write failing tests for the terminal-state guard +- [x] 1.1 Write failing tests for the terminal-state guard - Cover: a job driven to JOB_STAT_DONE via xce_core_push_events() (confirming allocs/accounts are nulled by the existing cleanup), then presented with a decoded_session carrying a DISPATCH entry for that job id, must leave allocs/accounts NULL after sim_reconcile_unaccounted_jobs() runs; the same scenario with JOB_STAT_EXIT instead of JOB_STAT_DONE; a PEND job's early-recorded DISPATCH entry is still captured exactly as today (explicit regression guard, not just relying on the pre-existing suite) - Observable: the new test cases for DONE/EXIT fail against today's sim_reconcile_unaccounted_jobs(), which resurrects allocs/accounts on any job with accounts == NULL regardless of state - _Requirements: 1.1, 1.2, 1.3_ diff --git a/src/scheduler/testsuite/test_sim_reconcile_unaccounted.c b/src/scheduler/testsuite/test_sim_reconcile_unaccounted.c index 7ddbf832..aa664c83 100644 --- a/src/scheduler/testsuite/test_sim_reconcile_unaccounted.c +++ b/src/scheduler/testsuite/test_sim_reconcile_unaccounted.c @@ -3,7 +3,18 @@ * sim_reconcile_unaccounted_jobs() (sim_loop.c, scheduler-sim-decision- * parity spec, tasks 2.2-2.4, "Recorded-Decision Reconciliation" component, * design.md; Requirement 1.1, 1.2, 3.1, 3.2, 3.3, 4.1, 4.2). Also covers - * scheduler-sim-eager-reconciliation task 2.1 (Requirement 1.2, 1.3, 1.4): + * scheduler-sim-reconcile-terminal-guard task 1.1 (RED phase, Requirement + * 1.1, 1.2, 1.3; bug-1764): the two tests at the very bottom of this file + * (test_reconcile_never_reallocates_a_{done,exit}_job_despite_stray_ + * dispatch_entry) prove a job driven to JOB_STAT_DONE/JOB_STAT_EXIT via + * xce_core_push_events() must never have its allocs/accounts resurrected by + * a later round's stray/duplicate recorded DISPATCH entry -- both are + * EXPECTED TO FAIL against today's sim_reconcile_unaccounted_jobs(), whose + * trigger (job->accounts == NULL alone) has no job->state check yet (task + * 1.2 adds it). test_reconcile_captures_non_run_state_job_when_dispatch_ + * recorded() above already serves as this feature's non-terminal-case + * regression guard (Requirement 1.2) and needed no duplication. + * Also covers scheduler-sim-eager-reconciliation task 2.1 (Requirement 1.2, 1.3, 1.4): * resource-in-use accounting via the REAL sched_account_jobs() (scheduler.c, * unchanged by that spec, verbatim-copied in below -- see this file's own * "White-box #include set" paragraph) correctly defers to a captured job's @@ -1585,6 +1596,181 @@ test_reconcile_step_mode_with_repl_state_still_prints_bare_decision_line( "decision_line: PASS\n"); } +/* ---------------------------------------------------------------------- */ +/* Test 18 (scheduler-sim-reconcile-terminal-guard task 1.1, RED phase, */ +/* Requirement 1.1, 1.3; bug-1764): a job driven all the way to */ +/* JOB_STAT_DONE via xce_core_push_events() -- whose normal end-of-life */ +/* cleanup (core_events.c's XCE_CORE_EVENT_JOB_STATE_CHANGE case) correctly */ +/* frees job->allocs/job->accounts, leaving job->accounts == NULL -- must */ +/* NEVER have its allocation resurrected by sim_reconcile_unaccounted_ */ +/* jobs(), even when this round's decoded_session carries a stray/ */ +/* duplicate recorded DISPATCH entry for that same jobid (the live-observed */ +/* bug-1764 shape: an array job's retried/leftover dispatch attempt still */ +/* present in the replay stream after the job has already completed). */ +/* Today's trigger check (sim_loop.c:690) is `job->accounts != NULL` alone, */ +/* with NO check on job->state -- it cannot distinguish "not yet allocated" */ +/* from "already completed and released", so THIS TEST IS EXPECTED TO FAIL */ +/* until task 1.2 adds the terminal-state exclusion */ +/* (JOB_IS_DONE()/JOB_IS_EXIT()). */ +/* ---------------------------------------------------------------------- */ + +static void +test_reconcile_never_reallocates_a_done_job_despite_stray_dispatch_entry(void) +{ + xce_sched_sim_driver_t sim; + xce_sched_job_t* job; + xce_job_alloc_t* stray_alloc; + replay_session_t* decoded; + xce_core_event_t ev; + int push_rc; + + fixture_init(&sim); + + /* RUN-state job that already has a real allocation -- the "actively + * holding resources" shape a terminal transition must release, same + * "already accounted" fixture shape add_job()'s own header comment and + * test_reconcile_skips_job_that_already_has_accounts() above establish. */ + job = add_job(&(sim.core), 7070ULL, JOB_STAT_RUN, + make_job_alloc(7070ULL, 0, NULL)); + TEST_ASSERT_NOT_NULL_MESSAGE( + job->allocs, "fixture setup: job must start with a real allocation"); + TEST_ASSERT_NOT_NULL_MESSAGE( + job->accounts, "fixture setup: job must start with real accounts"); + + /* Drive the job to JOB_STAT_DONE through the SAME production event path + * bug-1764's root cause depends on -- xce_core_push_events()'s + * XCE_CORE_EVENT_JOB_STATE_CHANGE case (core_events.c), same technique + * test_reconcile_then_release_via_job_state_change_is_asan_clean() above + * already establishes. */ + memset(&ev, 0, sizeof(ev)); + ev.type = XCE_CORE_EVENT_JOB_STATE_CHANGE; + ev.u.job_state_change.jobid = 7070ULL; + ev.u.job_state_change.old_state = JOB_STAT_RUN; + ev.u.job_state_change.new_state = JOB_STAT_DONE; + + push_rc = xce_core_push_events(&(sim.core), &ev, 1); + TEST_ASSERT_EQUAL_INT(0, push_rc); + + /* Precondition check (NOT this test's main assertion): confirm the + * existing end-of-life cleanup genuinely nulled allocs/accounts before + * blaming reconciliation for anything below. */ + TEST_ASSERT_TRUE_MESSAGE(JOB_IS_DONE(job->state), + "precondition: job must genuinely be DONE " + "before this test's real assertion means " + "anything"); + TEST_ASSERT_NULL_MESSAGE( + job->allocs, "precondition: xce_core_push_events()'s normal " + "end-of-life cleanup must null job->allocs on a " + "terminal transition"); + TEST_ASSERT_NULL_MESSAGE( + job->accounts, "precondition: xce_core_push_events()'s normal " + "end-of-life cleanup must null job->accounts on a " + "terminal transition"); + + /* A later replay round's decoded_session still carries a stray/ + * duplicate DISPATCH entry for this now-terminal job's jobid -- e.g. a + * retried dispatch attempt for one element of an array job (bug-1764's + * live-observed shape). */ + stray_alloc = make_test_job_alloc_with_host(7070ULL, "worker-stray", + "slots", 9.0f); + decoded = make_decoded_session( + 2000ULL, make_dispatch_entry_from_alloc(stray_alloc)); + + sim_reconcile_unaccounted_jobs(&sim, decoded, NULL); + + /* THE ASSERTION THIS TEST EXISTS TO PROVE (expected to FAIL today): a + * DONE job's allocation must never be resurrected, no matter what the + * replay stream still contains for it. */ + TEST_ASSERT_NULL_MESSAGE( + job->allocs, + "expected allocs NULL after reconciling a terminal (DONE) job, but " + "allocs was populated -- today's trigger (job->accounts == NULL " + "alone) cannot distinguish 'not yet allocated' from 'already " + "completed and released' (bug-1764)"); + TEST_ASSERT_NULL_MESSAGE( + job->accounts, + "expected accounts NULL after reconciling a terminal (DONE) job, " + "but accounts was populated -- same root cause as the allocs " + "assertion above"); + + printf("test_reconcile_never_reallocates_a_done_job_despite_stray_" + "dispatch_entry: PASS\n"); +} + +/* ---------------------------------------------------------------------- */ +/* Test 19 (scheduler-sim-reconcile-terminal-guard task 1.1, RED phase, */ +/* Requirement 1.1, 1.3; bug-1764): identical scenario to Test 18 above but */ +/* with JOB_STAT_EXIT instead of JOB_STAT_DONE -- both terminal states must */ +/* be covered, not just one (Requirement 1.3's "treat a job's terminal */ +/* state as permanent" applies to EXIT exactly as much as DONE). Also */ +/* EXPECTED TO FAIL today, for the identical reason as Test 18. */ +/* ---------------------------------------------------------------------- */ + +static void +test_reconcile_never_reallocates_an_exit_job_despite_stray_dispatch_entry( + void) +{ + xce_sched_sim_driver_t sim; + xce_sched_job_t* job; + xce_job_alloc_t* stray_alloc; + replay_session_t* decoded; + xce_core_event_t ev; + int push_rc; + + fixture_init(&sim); + + job = add_job(&(sim.core), 7071ULL, JOB_STAT_RUN, + make_job_alloc(7071ULL, 0, NULL)); + TEST_ASSERT_NOT_NULL_MESSAGE( + job->allocs, "fixture setup: job must start with a real allocation"); + TEST_ASSERT_NOT_NULL_MESSAGE( + job->accounts, "fixture setup: job must start with real accounts"); + + memset(&ev, 0, sizeof(ev)); + ev.type = XCE_CORE_EVENT_JOB_STATE_CHANGE; + ev.u.job_state_change.jobid = 7071ULL; + ev.u.job_state_change.old_state = JOB_STAT_RUN; + ev.u.job_state_change.new_state = JOB_STAT_EXIT; + + push_rc = xce_core_push_events(&(sim.core), &ev, 1); + TEST_ASSERT_EQUAL_INT(0, push_rc); + + TEST_ASSERT_TRUE_MESSAGE(JOB_IS_EXIT(job->state), + "precondition: job must genuinely be EXIT " + "before this test's real assertion means " + "anything"); + TEST_ASSERT_NULL_MESSAGE( + job->allocs, "precondition: xce_core_push_events()'s normal " + "end-of-life cleanup must null job->allocs on a " + "terminal transition"); + TEST_ASSERT_NULL_MESSAGE( + job->accounts, "precondition: xce_core_push_events()'s normal " + "end-of-life cleanup must null job->accounts on a " + "terminal transition"); + + stray_alloc = make_test_job_alloc_with_host(7071ULL, "worker-stray-exit", + "slots", 9.0f); + decoded = make_decoded_session( + 2100ULL, make_dispatch_entry_from_alloc(stray_alloc)); + + sim_reconcile_unaccounted_jobs(&sim, decoded, NULL); + + TEST_ASSERT_NULL_MESSAGE( + job->allocs, + "expected allocs NULL after reconciling a terminal (EXIT) job, but " + "allocs was populated -- today's trigger (job->accounts == NULL " + "alone) cannot distinguish 'not yet allocated' from 'already " + "completed and released' (bug-1764)"); + TEST_ASSERT_NULL_MESSAGE( + job->accounts, + "expected accounts NULL after reconciling a terminal (EXIT) job, " + "but accounts was populated -- same root cause as the allocs " + "assertion above"); + + printf("test_reconcile_never_reallocates_an_exit_job_despite_stray_" + "dispatch_entry: PASS\n"); +} + int main(void) { UNITY_BEGIN(); @@ -1612,5 +1798,9 @@ int main(void) test_reconcile_continue_mode_with_repl_state_suppresses_bare_decision_line); RUN_TEST( test_reconcile_step_mode_with_repl_state_still_prints_bare_decision_line); + RUN_TEST( + test_reconcile_never_reallocates_a_done_job_despite_stray_dispatch_entry); + RUN_TEST( + test_reconcile_never_reallocates_an_exit_job_despite_stray_dispatch_entry); return UNITY_END(); }