From 9dcffce8d8f77ace59209cbbaf4cc55e2d0d0928 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Thu, 3 Sep 2026 08:31:37 +0000 Subject: [PATCH] test(scheduler-sim-reconcile-terminal-guard): task 2.2 - final REPL-level verification Adds test_reconcile_info_worker_and_summary_hide_completed_job_after_stray_dispatch, mirroring bug-1764's actual live-observed shape: a worker with one job driven to a terminal state and two jobs genuinely still RUN, all real allocations to the SAME worker. After reconciling a stray DISPATCH entry for the terminal job's id, info worker excludes it while still listing the two running jobs, and info summary's running count agrees. Round 1 review rejected an initial version for using a mismatched host string on the stray entry ("workerarr-stray" instead of "workerarr"), which made the info worker/info summary assertions pass vacuously regardless of whether the fix existed, since repl_info_worker_one() filters assigned jobs by an exact host-string match. Round 2 fixed the host string and confirmed via an independently-reproduced mutation test (fix reverted, the test's own accounting-level pre-check bypassed) that the REPL-level assertions now genuinely, independently detect the bug. design.md's Testing Strategy is corrected: it originally said no new test was needed, reasoning the existing display-path suites already covered this; tasks.md's own task 2.2 correctly required a purpose-built test for this exact scenario instead. Full scheduler test suite: 63/64 OK, sole failure the pre-existing, unrelated bug-1758. This is the final task of the scheduler-sim-reconcile-terminal-guard spec; all implementation tasks (1.1-2.2) are now complete. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01YAWMhfRJFnmmnG8h97yUDd --- .../design.md | 6 +- .../tasks.md | 2 +- .../test_sim_reconcile_unaccounted.c | 251 ++++++++++++++++++ 3 files changed, 256 insertions(+), 3 deletions(-) diff --git a/.kiro/specs/scheduler-sim-reconcile-terminal-guard/design.md b/.kiro/specs/scheduler-sim-reconcile-terminal-guard/design.md index 00fd321e..909d386c 100644 --- a/.kiro/specs/scheduler-sim-reconcile-terminal-guard/design.md +++ b/.kiro/specs/scheduler-sim-reconcile-terminal-guard/design.md @@ -132,6 +132,8 @@ No new logging — this is a silent skip, matching the function's existing silen - The same scenario with `JOB_STAT_EXIT` instead of `JOB_STAT_DONE` (1.1, 1.3 — both terminal states must be covered, not just one). - A PEND job's early-recorded DISPATCH entry is still captured exactly as before (regression guard for 1.2 — must continue passing unmodified from the existing suite, and a new test if the existing suite doesn't already cover this exact "not yet RUN, gets captured" shape explicitly). -### Integration Tests (existing suites, re-run, no new tests required) -- `test_sim_e2e_original_motivation.c` / `test_repl_resource_table.c`'s `info worker` assigned-jobs tests: re-run to confirm no regression (Requirement 2's observable outcome; no new test needed since these already exercise the real display path against real job state). +### Integration Tests +**Correction (post-review, task 2.2)**: this section originally read "existing suites, re-run, no new tests required," reasoning that `test_sim_e2e_original_motivation.c`/`test_repl_resource_table.c` already exercise `info worker`'s real display path closely enough. `tasks.md`'s own task 2.2 text (approved at the same time as this document) correctly went further and required a new, purpose-built test for the exact bug-1764 scenario — those existing suites exercise the general display path, not this specific stray-dispatch-after-terminal-state case. That new test now lives in `test_sim_reconcile_unaccounted.c` (`test_reconcile_info_worker_and_summary_hide_completed_job_after_stray_dispatch`), alongside the unit tests below rather than in a separate REPL-focused file, since this file already carries every fixture primitive the scenario needs. Review's first round rejected an initial version of this test for using a mismatched host string on the stray entry (`"workerarr-stray"` instead of `"workerarr"`), which made its `info worker`/`info summary` assertions pass vacuously regardless of whether the fix existed; round 2 confirmed via an independently-reproduced mutation test (fix reverted, test's own accounting-level pre-check bypassed) that the corrected test's REPL-level assertions now genuinely, independently detect the bug. +- `test_reconcile_info_worker_and_summary_hide_completed_job_after_stray_dispatch` (`test_sim_reconcile_unaccounted.c`, new): a worker with one job driven to a terminal state and two jobs genuinely still RUN, all real allocations to the SAME worker (mirroring bug-1764's actual live shape — a stray dispatch entry for the SAME worker the job was originally sent to); after reconciling a stray DISPATCH entry for the terminal job's id, `info worker ` excludes it while still listing the two running jobs, and `info summary`'s running count is consistent with what `info worker` shows (2.1, 2.2). +- `test_sim_e2e_original_motivation.c` / `test_repl_resource_table.c`'s `info worker` assigned-jobs tests: re-run to confirm no regression to the general display path (unrelated to bug-1764's specific scenario, but a useful sanity check). - Full scheduler test suite: confirm no regression anywhere else, matching the discipline already established in this session's other specs. diff --git a/.kiro/specs/scheduler-sim-reconcile-terminal-guard/tasks.md b/.kiro/specs/scheduler-sim-reconcile-terminal-guard/tasks.md index 80071b86..1332e5ab 100644 --- a/.kiro/specs/scheduler-sim-reconcile-terminal-guard/tasks.md +++ b/.kiro/specs/scheduler-sim-reconcile-terminal-guard/tasks.md @@ -19,7 +19,7 @@ - Observable: full test suite result showing no regression, with the pre-existing eager-capture and resync-merge test names explicitly confirmed still passing - _Requirements: 1.2_ -- [ ] 2.2 Re-verify against a realistic array-job scenario that info worker's assigned-jobs listing no longer shows a completed job +- [x] 2.2 Re-verify against a realistic array-job scenario that info worker's assigned-jobs listing no longer shows a completed job - Using the same capture_dispatch()-driven REPL test harness already established in test_sim_e2e_original_motivation.c/test_repl_resource_table.c, construct a multi-job scenario mirroring bug-1764's live-observed shape: an array job where some elements are driven to JOB_STAT_DONE/JOB_STAT_EXIT via xce_core_push_events() (as in 1.1's tests) while others remain running, then advance the sim driver through a further round whose decoded_session carries a stray/duplicate DISPATCH entry (built the same way as 1.1's regression tests) for one of the now-terminal job ids. Issue info worker and info summary through capture_dispatch() and assert the completed job is absent from the assigned-jobs listing, the still-running jobs are present, and the two commands' counts agree - Observable: a captured info worker transcript showing the completed job absent from the assigned-jobs listing, with the still-running jobs present and info summary's counts consistent with it - _Requirements: 2.1, 2.2_ diff --git a/src/scheduler/testsuite/test_sim_reconcile_unaccounted.c b/src/scheduler/testsuite/test_sim_reconcile_unaccounted.c index aa664c83..76e1cc03 100644 --- a/src/scheduler/testsuite/test_sim_reconcile_unaccounted.c +++ b/src/scheduler/testsuite/test_sim_reconcile_unaccounted.c @@ -1771,6 +1771,255 @@ test_reconcile_never_reallocates_an_exit_job_despite_stray_dispatch_entry( "dispatch_entry: PASS\n"); } +/* ---------------------------------------------------------------------- */ +/* REPL-capture helper (scheduler-sim-reconcile-terminal-guard task 2.2): */ +/* duplicated from test_repl_resource_table.c's/test_repl_breakpoints_ */ +/* inspection.c's own identical capture_dispatch()/capture_stdout_around() */ +/* helpers, per this file family's established "duplicate rather than share */ +/* via a new header" convention (design.md's File Structure Plan; those two */ +/* files' own header comments document the same choice). This file already */ +/* white-box #includes the full repl.c/repl_fmt.c/repl_rollup.c/repl_break. */ +/* c/repl_info.c/repl_params.c stack every OTHER test here needs for */ +/* sim_reconcile_unaccounted_jobs()'s own xce_repl_check_decision() call -- */ +/* only the stdout-redirect capture helpers themselves are new here, not a */ +/* new #include. */ +/* ---------------------------------------------------------------------- */ + +static char* capture_stdout_around(void (*fn)(void)) +{ + char tmp[] = "/tmp/xce_simreconcile_replcap_XXXXXX"; + int fd = mkstemp(tmp); + int saved_stdout; + off_t len; + char* buf; + + TEST_ASSERT_TRUE_MESSAGE(fd >= 0, "mkstemp for stdout capture must " + "succeed"); + + fflush(stdout); + saved_stdout = dup(STDOUT_FILENO); + TEST_ASSERT_TRUE_MESSAGE(saved_stdout >= 0, "dup(STDOUT_FILENO) must " + "succeed"); + TEST_ASSERT_TRUE_MESSAGE(dup2(fd, STDOUT_FILENO) >= 0, + "dup2 redirect of STDOUT_FILENO must succeed"); + + fn(); + + fflush(stdout); + dup2(saved_stdout, STDOUT_FILENO); + close(saved_stdout); + + len = lseek(fd, 0, SEEK_CUR); + TEST_ASSERT_TRUE(len >= 0); + + buf = (char*)calloc(1, (size_t)len + 1); + TEST_ASSERT_NOT_NULL(buf); + TEST_ASSERT_TRUE(lseek(fd, 0, SEEK_SET) == 0); + if (len > 0) { + ssize_t nread = read(fd, buf, (size_t)len); + TEST_ASSERT_TRUE(nread == len); + } + close(fd); + unlink(tmp); + + return buf; +} + +static xce_sched_sim_driver_t* g_dispatch_sim; +static const char* g_dispatch_line; + +static void run_dispatch_for_capture(void) +{ + (void)repl_dispatch(g_dispatch_sim, g_dispatch_line); +} + +static char* capture_dispatch(xce_sched_sim_driver_t* sim, const char* line) +{ + g_dispatch_sim = sim; + g_dispatch_line = line; + return capture_stdout_around(run_dispatch_for_capture); +} + +/* ---------------------------------------------------------------------- */ +/* Test 20 (scheduler-sim-reconcile-terminal-guard task 2.2, Requirement */ +/* 2.1, 2.2 -- FINAL task of this spec): REPL-visible confirmation of tasks */ +/* 1.1/1.2's fix, mirroring bug-1764's live-observed shape -- an array job */ +/* where some elements are DONE/EXIT while others remain running -- at the */ +/* `info worker`/`info summary` display level Requirement 2 actually cares */ +/* about, not just at the job->allocs/job->accounts level Tests 18/19 above */ +/* already pin. */ +/* */ +/* Three sibling jobs, all dispatched onto the SAME worker ("workerarr"): */ +/* - jobid 7080: driven to JOB_STAT_DONE via xce_core_push_events() */ +/* (XCE_CORE_EVENT_JOB_STATE_CHANGE, the exact production path Test 18 */ +/* uses), which correctly frees its real starting allocation. */ +/* - jobids 7081/7082: remain genuinely JOB_STAT_RUN with their own real, */ +/* distinct allocations to the same worker throughout. */ +/* A later round's decoded_session then carries a stray/duplicate DISPATCH */ +/* entry for jobid 7080 -- bug-1764's own live-observed cause (a retried/ */ +/* leftover dispatch attempt for one array element still present in the */ +/* replay stream after that element already completed) -- built with the */ +/* exact same make_test_job_alloc_with_host()/make_dispatch_entry_from_ */ +/* alloc()/make_decoded_session() helpers Tests 18/19 use. */ +/* sim_reconcile_unaccounted_jobs() is called directly, the same convention */ +/* design.md's Testing Strategy establishes and every other test in this */ +/* file follows (the function is `static`, not driven through a full */ +/* round-boundary path by any test in this suite). */ +/* */ +/* "info worker workerarr" and "info summary" are then captured through */ +/* capture_dispatch() (above). Requirement 2.1: the completed job's id must */ +/* be ABSENT from the assigned-jobs listing while both still-running jobs' */ +/* ids remain present. Requirement 2.2 (display consistency): info */ +/* summary's own running count -- computed purely from job->state via */ +/* JOB_IS_RUN(), entirely independent of job->allocs/job->accounts -- must */ +/* agree with exactly those two still-running jobs, proving the assigned- */ +/* jobs listing and the running count tell the SAME story once the guard is */ +/* in place (bug-1764's actual symptom was these two views disagreeing: */ +/* the assigned-jobs listing showed 3 while running was correctly only 2). */ +/* ---------------------------------------------------------------------- */ + +static void +test_reconcile_info_worker_and_summary_hide_completed_job_after_stray_dispatch( + void) +{ + xce_sched_sim_driver_t sim; + xce_sched_worker_t* w; + xce_sched_job_t* job_done; + xce_sched_job_t* job_run1; + xce_sched_job_t* job_run2; + xce_job_alloc_t* alloc_done_initial; + xce_job_alloc_t* alloc_run1; + xce_job_alloc_t* alloc_run2; + xce_job_alloc_t* stray_alloc; + replay_session_t* decoded; + xce_core_event_t ev; + int residx; + int push_rc; + char* captured_worker; + char* captured_summary; + char done_jobid_str[64]; + char run1_jobid_str[64]; + char run2_jobid_str[64]; + + fixture_init(&sim); + + residx = register_slots_resource(&(sim.core)); + w = add_worker(&(sim.core), "workerarr", residx, 20.0f); + TEST_ASSERT_NOT_NULL(w); + + /* jobid 7080 starts genuinely RUN with a real allocation -- the + * "actively holding resources" shape a terminal transition must + * release, same fixture shape Test 18/19 above establish. */ + alloc_done_initial + = make_test_job_alloc_with_host(7080ULL, "workerarr", "slots", 3.0f); + job_done = add_job(&(sim.core), 7080ULL, JOB_STAT_RUN, alloc_done_initial); + TEST_ASSERT_NOT_NULL_MESSAGE( + job_done->allocs, + "fixture setup: job_done must start with a real allocation"); + TEST_ASSERT_NOT_NULL_MESSAGE( + job_done->accounts, + "fixture setup: job_done must start with real accounts"); + + /* jobids 7081/7082: the "some elements remain running" half of the + * array-job scenario -- genuinely RUN, each with its own real, distinct + * allocation to the SAME worker, untouched for the rest of this test. */ + alloc_run1 = make_test_job_alloc_with_host(7081ULL, "workerarr", "slots", + 2.0f); + job_run1 = add_job(&(sim.core), 7081ULL, JOB_STAT_RUN, alloc_run1); + TEST_ASSERT_NOT_NULL(job_run1->allocs); + + alloc_run2 = make_test_job_alloc_with_host(7082ULL, "workerarr", "slots", + 4.0f); + job_run2 = add_job(&(sim.core), 7082ULL, JOB_STAT_RUN, alloc_run2); + TEST_ASSERT_NOT_NULL(job_run2->allocs); + + /* Drive jobid 7080 to JOB_STAT_DONE through the SAME production event + * path bug-1764's root cause depends on (Test 18's own technique). */ + memset(&ev, 0, sizeof(ev)); + ev.type = XCE_CORE_EVENT_JOB_STATE_CHANGE; + ev.u.job_state_change.jobid = 7080ULL; + 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); + + TEST_ASSERT_TRUE_MESSAGE(JOB_IS_DONE(job_done->state), + "precondition: job_done must genuinely be DONE " + "before this test's real assertions mean " + "anything"); + TEST_ASSERT_NULL_MESSAGE( + job_done->allocs, "precondition: normal end-of-life cleanup must " + "null job_done->allocs on its terminal " + "transition"); + TEST_ASSERT_NULL_MESSAGE( + job_done->accounts, "precondition: normal end-of-life cleanup must " + "null job_done->accounts on its terminal " + "transition"); + + /* A later replay round's decoded_session still carries a stray/ + * duplicate DISPATCH entry for jobid 7080 -- bug-1764's live-observed + * shape (a retried dispatch attempt for one array element still present + * in the replay stream after that element already completed). */ + stray_alloc = make_test_job_alloc_with_host(7080ULL, "workerarr", + "slots", 9.0f); + decoded = make_decoded_session( + 9000ULL, make_dispatch_entry_from_alloc(stray_alloc)); + + sim_reconcile_unaccounted_jobs(&sim, decoded, NULL); + + /* Sanity re-check (the accounting-level half of this task's own + * Observable, already covered by Tests 18/19 -- repeated here only to + * localize a failure before trusting the REPL-level assertions below). */ + TEST_ASSERT_NULL_MESSAGE( + job_done->allocs, + "job_done's allocation must still be NULL after reconciling this " + "round's stray DISPATCH entry for its own jobid"); + TEST_ASSERT_NULL_MESSAGE(job_done->accounts, + "job_done's accounts must likewise still be " + "NULL"); + + /* ---- The REPL-visible assertions this task actually exists for. ---- */ + + fmt_jobid(done_jobid_str, sizeof(done_jobid_str), 7080ULL); + fmt_jobid(run1_jobid_str, sizeof(run1_jobid_str), 7081ULL); + fmt_jobid(run2_jobid_str, sizeof(run2_jobid_str), 7082ULL); + + captured_worker = capture_dispatch(&sim, "info worker workerarr"); + + TEST_ASSERT_NULL_MESSAGE( + strstr(captured_worker, done_jobid_str), + "Requirement 2.1: the completed job must be ABSENT from 'info " + "worker's assigned-jobs listing, even though this round's " + "decoded_session carried a stray DISPATCH entry for its jobid " + "(bug-1764)"); + TEST_ASSERT_NOT_NULL_MESSAGE( + strstr(captured_worker, run1_jobid_str), + "the first still-running job must be present in the assigned-jobs " + "listing"); + TEST_ASSERT_NOT_NULL_MESSAGE( + strstr(captured_worker, run2_jobid_str), + "the second still-running job must be present in the assigned-jobs " + "listing"); + + free(captured_worker); + + captured_summary = capture_dispatch(&sim, "info summary"); + + TEST_ASSERT_NOT_NULL_MESSAGE( + strstr(captured_summary, "3 jobs (0 pending, 2 running)"), + "Requirement 2.2: 'info summary's own running count (computed " + "purely from job->state, independent of job->allocs/accounts) must " + "agree with the assigned-jobs listing above -- exactly the two " + "still-running jobs, never inflated by the completed one"); + + free(captured_summary); + + printf( + "test_reconcile_info_worker_and_summary_hide_completed_job_after_" + "stray_dispatch: PASS\n"); +} + int main(void) { UNITY_BEGIN(); @@ -1802,5 +2051,7 @@ int main(void) test_reconcile_never_reallocates_a_done_job_despite_stray_dispatch_entry); RUN_TEST( test_reconcile_never_reallocates_an_exit_job_despite_stray_dispatch_entry); + RUN_TEST( + test_reconcile_info_worker_and_summary_hide_completed_job_after_stray_dispatch); return UNITY_END(); }