From d9109b7c4b089546def72601990fab31d62f1db5 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Wed, 2 Sep 2026 15:10:01 +0000 Subject: [PATCH] feat(scheduler-sim-repl-verbosity): task 4.2 - retire redundant per-decision echo Gates sim_print_decision() so the bare per-decision line no longer prints under continue mode, making the Round Rollup's summary the sole source of decision output (Requirement 2.3, closing bug-1610). step mode and non-interactive/headless runs are completely unaffected (Requirements 5.1, 5.4). Implemented as a file-local inline wrapper in sim_loop.c rather than repl.h, since the gated function is static to that file and repl.h can't see repl_state's internals without leaking them to every other repl.h consumer. Fixes 3 pre-existing tests (predating this spec, from scheduler-core) that pinned the now-intentionally-changed bare-line behavior under continue mode - each fixed differently based on its actual purpose rather than uniformly: two kept continue mode with their anchor switched to the Round Rollup's summary (since they specifically test continue-mode breakpoint interruption, which step mode would trivialize), one switched to step mode (since its actual invariant - print-before-breakpoint-check call order - is mode-independent and continue mode no longer has any observable anchor for it). Review caught a second, unguarded sim_print_decision() call site in sim_reconcile_unaccounted_jobs() (added by an earlier, separately- merged spec) that design.md's boundary never accounted for - per the review, this is actually the dominant real-world path for DISPATCH decisions in replay mode, so leaving it unguarded would have defeated Requirement 2.3's "sole source of decision output" claim for the common case. Gated it through the same wrapper; added 2 new tests. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01YAWMhfRJFnmmnG8h97yUDd --- .../scheduler-sim-repl-verbosity/design.md | 6 +- .../scheduler-sim-repl-verbosity/tasks.md | 2 +- .wolf/buglog.json | 2 +- src/scheduler/src/sim_loop.c | 116 +++++++++- .../test_repl_breakpoints_inspection.c | 55 +++-- .../testsuite/test_repl_execution_control.c | 33 ++- .../test_sim_reconcile_unaccounted.c | 211 ++++++++++++++++++ 7 files changed, 386 insertions(+), 39 deletions(-) diff --git a/.kiro/specs/scheduler-sim-repl-verbosity/design.md b/.kiro/specs/scheduler-sim-repl-verbosity/design.md index b064b349..31b3a206 100644 --- a/.kiro/specs/scheduler-sim-repl-verbosity/design.md +++ b/.kiro/specs/scheduler-sim-repl-verbosity/design.md @@ -110,7 +110,7 @@ flowchart TD - `src/scheduler/src/repl_rollup.c` (new, post-review — Correction above) — the Round Rollup's own fingerprint-build/pend-jobid-query/emit/flush/round-boundary-process logic (Requirements 2, 3), split out of `repl.c` to stay under the 800-line cap; shares state with `repl.c` via `repl_internal.h`, matching `repl_break.c`/`repl_info.c`/`repl_params.c`'s established split; added to `scheduler_srcs` in `src/scheduler/meson.build` immediately after `repl.c`. - `src/scheduler/src/repl_info.c` — new resource-value classification/scaling helpers (byte-denominated list, `swp`'s megabyte special case, whole-number formatting) and a table/grouping rewrite of `repl_info_worker()`'s and `repl_info_resource()`'s resource-listing loops, both reading `rs->units_mode` (Requirement 6); the assigned-job listing in `repl_info_worker()` adopts the jobid-range-compression helper (Requirement 4.5). - `src/scheduler/src/repl_params.c` — `repl_cmd_set()` gains a `units raw|human|both` branch alongside the existing `speed`/`output`/`verify` branches, storing the selection on the new `rs->units_mode` field (Requirement 6.2, 6.3). -- `src/scheduler/src/sim_loop.c` — the existing call site invoking `sim_print_decision()` gains a `continue`-mode gate, mirroring `repl.h`'s inline-wrapper convention (Requirements 2, 5). +- `src/scheduler/src/sim_loop.c` — the existing call site invoking `sim_print_decision()` gains a `continue`-mode gate, mirroring `repl.h`'s inline-wrapper convention (Requirements 2, 5). **Correction (post-review, task 4.2 remediation)**: this bullet's original "the existing call site" (singular) undercounted the file's actual production call sites — `sim_loop.c` has TWO direct calls to `sim_print_decision()`, `sched_sim_driver_run()`'s main decision-drain loop (this bullet's original target) AND `sim_reconcile_unaccounted_jobs()`'s reconstructed-decision report (added by the sibling scheduler-sim-decision-parity/scheduler-sim-eager-reconciliation specs, unrelated in authorship but real production code that reports DISPATCH decisions through this exact channel). Since that reconciliation path is a primary source of DISPATCH decisions surfacing in replay mode, leaving it unguarded reproduced bug-1610's own duplication there too. Both call sites now route through `sim_print_decision_gate()` — see the `sim_print_decision` Gate component's own Correction below for the updated Boundary Commitment. - `src/scheduler/include/repl.h` — no functional change expected; existing inline wrappers already cover the gating points this feature needs. ### New/Modified Test Files @@ -263,7 +263,9 @@ int repl_fmt_jobid_range(char* buf, size_t bufsz, **Responsibilities & Constraints** - Skip the direct print only when `sim->interactive && sim->repl_state->mode == XCE_REPL_MODE_CONTINUE`; `step` mode and non-interactive (headless) runs are unaffected (Requirement 5.1, 5.4). -- Implemented as an inline-wrapper-style gate at the existing call site in `sched_sim_driver_run()`, consistent with `repl.h`'s established convention, rather than a bare `if` block added to the loop body. +- Implemented as an inline-wrapper-style gate, consistent with `repl.h`'s established convention, rather than a bare `if` block added to each call site's body. + +**Correction (post-review, task 4.2 remediation)**: this component's original Boundary Commitment named "the existing call site in `sched_sim_driver_run()`" (singular). Review found `sim_loop.c` has a SECOND, direct, unguarded call to `sim_print_decision()` inside `sim_reconcile_unaccounted_jobs()` (added by the sibling scheduler-sim-decision-parity/scheduler-sim-eager-reconciliation specs after this design.md was originally written) — and that reconciliation path is actually a PRIMARY mechanism by which DISPATCH decisions surface in replay mode at all (most live-drain-loop activity is PEND). A round where reconciliation fires was therefore still showing both the bare per-decision line AND the Round Rollup's summary counting the same dispatch, reproducing bug-1610's own duplication via this second, previously-unaccounted-for call site. This component's gate (same function, same condition, no new symbol) now guards BOTH call sites: `sched_sim_driver_run()`'s main decision-drain loop (this component's original target) and `sim_reconcile_unaccounted_jobs()`'s reconstructed-decision report. Requirement 2.3/this feature's own Observable ("the round summary is the sole source of decision output") is only actually satisfied once both are covered — see `test_sim_reconcile_unaccounted.c`'s two new tests (`test_reconcile_continue_mode_with_repl_state_suppresses_bare_decision_line` / `test_reconcile_step_mode_with_repl_state_still_prints_bare_decision_line`) for the second call site's own contract coverage, parallel to `test_sim_print_decision_gate.c`'s existing coverage of the first. **Contracts**: Service [ ] / API [ ] / Event [ ] / Batch [ ] / State [x] diff --git a/.kiro/specs/scheduler-sim-repl-verbosity/tasks.md b/.kiro/specs/scheduler-sim-repl-verbosity/tasks.md index dfd49d23..5acd6b6f 100644 --- a/.kiro/specs/scheduler-sim-repl-verbosity/tasks.md +++ b/.kiro/specs/scheduler-sim-repl-verbosity/tasks.md @@ -52,7 +52,7 @@ - _Boundary: sim_print_decision gate_ - _Depends: 2.2_ -- [ ] 4.2 (P) Suppress the old per-decision echo during continuous execution so the round summary is the sole source of decision output +- [x] 4.2 (P) Suppress the old per-decision echo during continuous execution so the round summary is the sole source of decision output - Leave single-stepping and non-interactive output untouched - Observable: all tests from 4.1 pass; a full continuous-execution run over a replay shows only round-summary lines, with no duplicate bare decision lines - _Requirements: 2.3, 5.1, 5.4_ diff --git a/.wolf/buglog.json b/.wolf/buglog.json index c2fbd23b..ddbc10ac 100644 --- a/.wolf/buglog.json +++ b/.wolf/buglog.json @@ -17960,7 +17960,7 @@ "error_message": "User-reported (same live REPL session): under 'continue', an unchanged PEND decision for the same jobid prints an identical '[sim] PEND jobid=...' line every scheduling round it remains pending -- e.g. jobs [22]-[29] (8 jobs) print their 8 byte-identical PEND lines 5+ times in a row across consecutive rounds with zero new information each time.", "file": "src/scheduler/src/sim_loop.c", "root_cause": "sim_print_decision() (sim_loop.c:388-411) unconditionally prints one DISPATCH/PEND/PREEMPT line per drained xce_decision_t with no de-duplication and no session/round context attached to the line itself -- called every round a job's allocation is (re-)attempted, so a job that stays PEND for N consecutive rounds produces N identical lines. xce_repl_on_decision() (repl_break.c:776-811), called alongside it, already tallies last_n_pend/last_n_dispatch/last_n_preempt per round for 'info session' -- that rollup exists but is never surfaced automatically, only on explicit 'info session'.", - "fix": "Not yet fixed -- proposed: either (a) only print a decision line the first time a given jobid enters PEND / when its pend reason changes, suppressing repeats of the identical reason, or (b) replace the unconditional per-decision echo with one per-round summary line at BEGIN_ROUND using the tally xce_repl_on_decision() already accumulates ('[sim] round N: X dispatch, Y pend, Z preempt'), reserving full per-decision detail for step mode or a matching breakpoint.", + "fix": "FIXED (2026-09-02, scheduler-sim-repl-verbosity spec, task 4.2 -- design.md's `sim_print_decision` Gate component, option (b) of this bug's own original proposal, now that the Round Rollup -- task 2.2, repl_rollup.c -- already provides the per-round summary line to fall back on). sim_loop.c's decision-drain loop (sched_sim_driver_run()) now calls a new file-local sim_print_decision_gate(sim, d) instead of calling sim_print_decision(d) directly; the gate skips the bare per-decision print only when sim->interactive && sim->repl_state->mode == XCE_REPL_MODE_CONTINUE, leaving step mode and non-interactive/headless runs completely unaffected (Req 5.1, 5.4) -- the Round Rollup's own per-round summary becomes the sole source of decision output under continue. Implementation wrinkle discovered and resolved during this task: design.md describes this as an 'inline-wrapper-style gate ... consistent with repl.h's established convention', but repl.h's existing wrappers (xce_repl_check_decision() and siblings) only ever test the PUBLIC sim->interactive field inline and delegate all repl-internal-state logic to an extern function implemented in repl.c/repl_break.c (the only files with repl_internal.h access, where xce_sched_repl_state_t's full definition -- including `mode` -- actually lives; sim.h/repl.h deliberately keep repl_state an opaque forward-declared pointer). That pattern cannot be reused verbatim here because the function being gated, sim_print_decision(), is itself static (private to sim_loop.c) -- repl.h cannot wrap a conditional call to a symbol it cannot see, and moving the check into repl.h would require repl.h to include repl_internal.h, leaking the full repl_state struct into every other repl.h consumer. Resolved by having sim_loop.c include repl_internal.h directly (a private header already scoped to src/scheduler/src/, so this stays an intra-directory dependency, not a new external one) and implementing sim_print_decision_gate() as a file-local static inline wrapper co-located with sim_print_decision() -- same 'wrapper hides the condition, call site stays one bare call expression' style repl.h's wrappers use, just not physically in that shared header since the wrapped function isn't extern. No new extern symbol crosses out of sim_loop.c for this purpose, so test_sim_isolation.c's nm(1) undefined-symbol layer and source-text-scan layer are both unaffected. repl.h itself was left untouched, matching design.md's own File Structure Plan line for it ('no functional change expected'). TDD: task 4.1's RED-phase test_sim_print_decision_gate.c (3 tests, real forked-child/direct-call stdout capture) pinned this exact contract before the fix landed; all 3 pass after (continue-mode suppression, step-mode-unaffected regression guard, headless-batch-unaffected regression guard). sched_repl_round_rollup_test (7/7) and sched_repl_event_echo_test (4/4) reverified green post-fix. Full-suite verification via /xce-build scheduler --incremental surfaced 3 UNEXPECTED additional failures beyond the pre-existing bug-1758, confirmed (via a controlled before/after rebuild swapping only sim_loop.c) to be caused by this fix itself, not a defect in it: test_repl_execution_control.c's test_pre_replay_pause_blocks_before_any_event_is_processed, and test_repl_breakpoints_inspection.c's test_event_breakpoint_pauses_repl_at_matching_event and test_decision_breakpoint_pauses_after_decision_output -- all three pre-date this spec and assert on the bare '[sim] PEND jobid=...' line's presence in a continue-mode capture, i.e. they pin exactly the bug-1610 behavior this fix intentionally removes. Left unmodified per this task's explicit no-test-file-editing boundary; flagged to the parent controller/reviewer as a real, reproducible side effect requiring a follow-up task to update those 3 assertions (e.g. swap their continue scripts for step, or assert on the Round Rollup's summary line instead) rather than a regression in this fix's own logic. **Remediation (2026-09-02, task 4.2 remediation, post-review):** the fix above only covered ONE of sim_loop.c's two direct sim_print_decision() call sites -- sched_sim_driver_run()'s main decision-drain loop. Review found a second, direct, unguarded call inside sim_reconcile_unaccounted_jobs() (added by the sibling scheduler-sim-decision-parity/scheduler-sim-eager-reconciliation specs after this bug's original fix landed, unrelated in authorship but real production code reporting DISPATCH decisions through this exact same channel) -- and that reconciliation path is actually a PRIMARY mechanism by which DISPATCH decisions surface in replay mode at all (most live-drain-loop activity is PEND), so a round where reconciliation fired was still showing both the bare per-decision line AND the Round Rollup's summary counting the same dispatch: this bug's own duplication, reproduced via the second call site design.md's original Boundary Commitments never accounted for (it only ever named \"the existing call site\", singular). Fixed by routing that call site through the identical sim_print_decision_gate(sim, d) too (same function, same condition, no new symbol) -- sim_loop.c:760's bare sim_print_decision(d) became sim_print_decision_gate(sim, d). TDD: two new tests added to test_sim_reconcile_unaccounted.c (test_reconcile_continue_mode_with_repl_state_suppresses_bare_decision_line / test_reconcile_step_mode_with_repl_state_still_prints_bare_decision_line), using the repl_state_ensure()-based direct-call fixture technique test_repl_round_rollup.c's own fixtures establish; the pre-existing test_reconcile_reports_reconstructed_decision_through_existing_channel (which deliberately leaves sim->repl_state NULL) was empirically re-verified to still pass unmodified, confirming the gate's NULL-safety default (\"never load-bearing, degrade to print\") covers it. Full-suite verification via /xce-build scheduler --incremental: sched_sim_reconcile_unaccounted_test now 16/16 (was 14/14), sched_sim_print_decision_gate_test 3/3, sched_repl_round_rollup_test 7/7, sched_repl_event_echo_test 4/4, sched_repl_execution_control_test 6/6, sched_repl_breakpoints_inspection_test 30/30 -- all green, confirming this bug's own previously-flagged test_repl_execution_control.c/test_repl_breakpoints_inspection.c follow-up (3 tests pinning the original bug-1610 behavior) was already resolved by an earlier task before this remediation. The only other full-suite failure remains the pre-existing, unrelated bug-1758 (sched_core_events_test). This closes the duplication gap completely -- both production call sites now gated.", "tags": [ "repl", "sim", diff --git a/src/scheduler/src/sim_loop.c b/src/scheduler/src/sim_loop.c index ad153c0b..020b7c24 100644 --- a/src/scheduler/src/sim_loop.c +++ b/src/scheduler/src/sim_loop.c @@ -9,6 +9,11 @@ #include "plugin.h" #include "preason.h" #include "repl.h" +#include "repl_internal.h" /* scheduler-sim-repl-verbosity spec, task 4.2 -- + * see this file's own header comment ("Boundary + * note") for why this one narrow exception to + * repl.h's usual opaque-`repl_state`-pointer + * convention is scoped to this file alone. */ #include "sim.h" #include "utils.h" #include "xcealloc.h" /* task 2.3: make_job_account() -- Recorded-Decision @@ -29,11 +34,14 @@ * (sim_record_write_event()), and per-round ReplaySession accumulation * (recording_session.c's scheduler_t-free sched_record_session_ctx_new()/ * _add_decision()/_finalize(), externed directly). sim_print_decision() - * remains this file's own unconditional Req 5 AC 19 default; it, - * --sim-output recording, and --sim-verify comparison are independently - * gated per drained decision. Task 11.4 owns comparing each drained - * decision against the recorded ReplaySession.decisions when --sim-verify - * is set -- see the ReplaySession branch below and sim_verify.c. + * was originally this file's own unconditional Req 5 AC 19 default; task + * 4.2 (scheduler-sim-repl-verbosity spec) narrows that default -- see + * "Boundary note" below and sim_print_decision_gate()'s own doc comment. + * --sim-output recording and --sim-verify comparison remain independently + * gated per drained decision, unaffected by task 4.2. Task 11.4 owns + * comparing each drained decision against the recorded + * ReplaySession.decisions when --sim-verify is set -- see the ReplaySession + * branch below and sim_verify.c. * * Task 12.2 (Req 5 AC 22 "Breakpoints") adds three bare hook calls below -- * xce_repl_check_event()/xce_repl_check_begin_round()/xce_repl_check_decision() @@ -45,6 +53,38 @@ * logic live in repl.h/repl_break.c, this file having zero line-budget * headroom for either (see repl_internal.h's own "Why a split at all"). * + * Boundary note (task 4.2, scheduler-sim-repl-verbosity spec, design.md's + * "sim_print_decision Gate" component, Req 2.3/5.1): that component's own + * spec is to skip the direct per-decision print only when `sim->interactive + * && sim->repl_state->mode == XCE_REPL_MODE_CONTINUE`, "implemented as an + * inline-wrapper-style gate ... consistent with repl.h's established + * convention". Every existing repl.h wrapper (xce_repl_check_event() and + * siblings) only ever tests the PUBLIC `sim->interactive` field inline and + * delegates all repl-internal-state logic (breakpoint matching, tallying, + * ...) to an `extern` function implemented in repl.c/repl_break.c, which + * alone -- via repl_internal.h -- can see `xce_sched_repl_state_t`'s full + * definition (sim.h keeps it an opaque forward-declared pointer precisely + * so sim_loop.c/repl.h never need that definition). This gate cannot follow + * that exact shape: the function it gates, sim_print_decision() (this + * file), is `static` -- private to this translation unit -- so repl.h + * cannot wrap a conditional call to it from outside, and moving the check + * itself into repl.h would require repl.h to include repl_internal.h, + * leaking the full repl_state struct into every OTHER repl.h consumer too + * (a strictly bigger boundary break than this file alone taking on a + * narrow, explicit, single-purpose include of it). This file therefore + * includes repl_internal.h directly (see the #include block above) and + * implements the mode check as its own file-local, static inline wrapper + * (sim_print_decision_gate(), just below sim_print_decision()) -- same + * "inline wrapper hides the condition, call site stays a single bare call + * expression" STYLE repl.h's own wrappers use, just co-located with the + * `static` function it wraps rather than declared in the shared header. + * No new extern symbol crosses out of this file for this purpose, so + * test_sim_isolation.c's nm(1) undefined-symbol layer (sched_sim_isolation_lib, + * built from sim.c+sim_loop.c alone) is unaffected; its source-text-scan + * layer is unaffected too, since repl_internal.h's own only #include is + * repl.h, itself already on this file's include list -- no new controller- + * facing symbol enters this file's reachable set. + * * Picker-driving thread (sim_picker_thread_fn, below): stands in for BOTH * the normal-mode picker AND allocator threads (scheduler.c), which the * simulation driver cannot use directly (no xce_scheduler_t*, Req 5 AC 17) @@ -413,6 +453,50 @@ static void sim_print_decision(const xce_decision_t* d) } } +/* + * sim_print_decision_gate (task 4.2, scheduler-sim-repl-verbosity spec, + * design.md's "sim_print_decision Gate" component, Req 2.3/5.1) -- an + * inline-wrapper-style gate on sim_print_decision(); see this file's own + * header comment ("Boundary note") for why it lives here, file-local, + * rather than as a repl.h wrapper. Skips the direct per-decision print + * ONLY when `sim->interactive && sim->repl_state->mode == + * XCE_REPL_MODE_CONTINUE` -- the Round Rollup (repl_rollup.c, task 2.2) + * already reports the round's decisions in that mode, so the bare + * "[sim] DISPATCH/PEND/PREEMPT jobid=..." line would otherwise duplicate + * it every round (bug-1610). `step` mode and non-interactive (headless, + * `sim->interactive == false`) runs are unaffected -- sim_print_decision() + * still fires exactly as before in both (Req 5.1, 5.4). + * + * **Correction (post-review, task 4.2 remediation):** this gate has TWO + * call sites, not one -- sched_sim_driver_run()'s main decision-drain + * loop below (the task's original target), AND + * sim_reconcile_unaccounted_jobs()'s own reconstructed-decision report + * (added later by the sibling scheduler-sim-decision-parity/scheduler- + * sim-eager-reconciliation specs, unrelated in authorship but real + * production code that reports DISPATCH decisions through this exact + * same channel -- see that function's own doc comment and its call site + * below). Both were missed by task 4.2's original single-call-site scope; + * this remediation wires the second one in too, closing bug-1610's + * duplication gap completely rather than only at the drain loop. + * + * A NULL `sim` or a not-yet-allocated `sim->repl_state` (repl_state is + * lazily allocated by xce_repl_start(), repl.c, and is guaranteed non-NULL + * whenever `sim->interactive` is true and the REPL has actually started -- + * see sim.h's own repl_state doc comment) is treated as "print", the same + * "never load-bearing, degrade to unattended" default this codebase's + * other REPL hooks use for an unexpected NULL. + */ +static inline void sim_print_decision_gate(const xce_sched_sim_driver_t* sim, + const xce_decision_t* d) +{ + if ((sim != NULL) && sim->interactive && (sim->repl_state != NULL) + && (sim->repl_state->mode == XCE_REPL_MODE_CONTINUE)) { + return; + } + + sim_print_decision(d); +} + /* * sim_picker_thread_fn - the simulation driver's picker-driving thread * (Req 5 AC 18's pseudocode: `core_go()` signals it, the driver's own @@ -547,8 +631,14 @@ static void* sim_picker_thread_fn(void* arg) * three-call reporting sequence sched_sim_driver_run()'s own decision-drain * loop uses for a freshly-drained decision: sched_record_session_add_decision() * (only when `record_ctx` is non-NULL, i.e. --sim-output recording is - * active), sim_print_decision() (the unconditional step/continue console - * output), and xce_repl_check_decision() (the REPL breakpoint/inspection + * active), sim_print_decision_gate() (task 4.2 remediation -- the + * step/continue console output, gated exactly like the drain loop's own + * call site: suppressed only under `sim->interactive && sim->repl_state-> + * mode == XCE_REPL_MODE_CONTINUE`, since this reconciliation path is + * itself a primary source of DISPATCH decisions surfacing in replay mode + * and was therefore ALSO duplicating the Round Rollup's summary, + * bug-1610, before this fix -- see sim_print_decision_gate()'s own doc + * comment), and xce_repl_check_decision() (the REPL breakpoint/inspection * hook, itself a safe no-op when not interactive). Like the drain loop's own * `d`, this reconstructed decision wrapper is intentionally not freed here -- * matching the codebase's existing, unchanged treatment of every @@ -685,7 +775,15 @@ static void sim_reconcile_unaccounted_jobs(xce_sched_sim_driver_t* sim, sched_record_session_add_decision( record_ctx, xce_core_now(&(sim->core)), d); } - sim_print_decision(d); + /* task 4.2 remediation: this call site was originally + * an unguarded sim_print_decision(d), missed by task + * 4.2's original scope (which only wired the gate into + * sched_sim_driver_run()'s drain loop below). Routed + * through the same sim_print_decision_gate(sim, d) -- + * no new symbol, same suppression condition -- see that + * function's own doc comment for why this second call + * site matters (bug-1610). */ + sim_print_decision_gate(sim, d); xce_repl_check_decision(sim, d); } } @@ -996,7 +1094,7 @@ int sched_sim_driver_run(xce_sched_sim_driver_t* sim, sched_record_session_add_decision( record_ctx, xce_core_now(&(sim->core)), d); } - sim_print_decision(d); + sim_print_decision_gate(sim, d); xce_repl_check_decision(sim, d); } diff --git a/src/scheduler/testsuite/test_repl_breakpoints_inspection.c b/src/scheduler/testsuite/test_repl_breakpoints_inspection.c index 2d4cd3af..366c0670 100644 --- a/src/scheduler/testsuite/test_repl_breakpoints_inspection.c +++ b/src/scheduler/testsuite/test_repl_breakpoints_inspection.c @@ -1768,12 +1768,12 @@ static void test_event_breakpoint_pauses_repl_at_matching_event(void) char aux_dir[PATH_MAX]; char script_path[PATH_MAX]; char capture_path[PATH_MAX]; - char pend_marker[80]; + char round_marker[96]; char jobid_buf[64]; FILE* f; char* captured; char* bp_pos; - char* pend_pos; + char* round_pos; f = write_header_and_snapshot(replay_path, sizeof(replay_path), "_evbp"); @@ -1798,14 +1798,24 @@ static void test_event_breakpoint_pauses_repl_at_matching_event(void) captured = run_group_d_and_capture(replay_path, script_path, capture_path); - /* bug-1591: sim_print_decision() (sim_loop.c) now renders jobids via - * fmt_jobid(), not raw %llu -- this marker must match that same - * human-readable form. */ + /* scheduler-sim-repl-verbosity task 4.2 (Req 2.3, bug-1610): under + * `continue` mode sim_print_decision()'s bare per-decision line no + * longer prints -- the Round Rollup (repl_rollup.c) reports the + * session's decision instead, deferred until the collapsed range + * ends. This fixture has exactly one round, so that range is only + * flushed (and printed) by xce_repl_on_replay_complete()'s + * repl_round_collapse_flush() call, triggered by the script's + * trailing `quit`. Look for that round-rollup summary's + * "jobids=<...>" field instead of the old bare "PEND jobid=<...>" + * line as evidence the session's decision was eventually processed + * -- bug-1591's fmt_jobid()-vs-raw-%llu fix applies equally to + * repl_fmt_jobid_range()'s single-jobid rendering (repl.c), which is + * byte-identical to fmt_jobid()'s own non-array format. */ fmt_jobid(jobid_buf, sizeof(jobid_buf), 4242ULL); - snprintf(pend_marker, sizeof(pend_marker), "PEND jobid=%s", jobid_buf); + snprintf(round_marker, sizeof(round_marker), "jobids=%s", jobid_buf); - bp_pos = strstr(captured, "hit breakpoint 1 @ event #4: JOB_STATE_CHANGE"); - pend_pos = strstr(captured, pend_marker); + bp_pos = strstr(captured, "hit breakpoint 1 @ event #4: JOB_STATE_CHANGE"); + round_pos = strstr(captured, round_marker); TEST_ASSERT_NOT_NULL_MESSAGE( bp_pos, "the breakpoint must fire exactly at event #4 (jobid=222), " @@ -1813,11 +1823,12 @@ static void test_event_breakpoint_pauses_repl_at_matching_event(void) TEST_ASSERT_NULL_MESSAGE( strstr(captured, "@ event #3"), "the breakpoint must NOT have fired on event #3 (jobid=111)"); - TEST_ASSERT_NOT_NULL_MESSAGE(pend_pos, + TEST_ASSERT_NOT_NULL_MESSAGE(round_pos, "the session's own PEND decision must " - "still have run after the resumed " + "still have run (reported via the round " + "rollup summary) after the resumed " "breakpoint pause"); - TEST_ASSERT_TRUE_MESSAGE(bp_pos < pend_pos, + TEST_ASSERT_TRUE_MESSAGE(bp_pos < round_pos, "the event breakpoint (mid-batch, before " "push_events) must be reported BEFORE the " "session's own decision output"); @@ -1854,10 +1865,24 @@ static void test_decision_breakpoint_pauses_after_decision_output(void) make_tmp_dir(aux_dir, sizeof(aux_dir), "_decbpaux"); snprintf(script_path, sizeof(script_path), "%s/script.txt", aux_dir); snprintf(capture_path, sizeof(capture_path), "%s/capture.out", aux_dir); - /* task 12.5 (bug-1594): trailing `quit` -- see the event-breakpoint - * test above for why. */ - write_text_file(script_path, - "break DECISION type=PEND\ncontinue\ncontinue\nquit\n"); + /* scheduler-sim-repl-verbosity task 4.2 (Req 2.3, bug-1610): `continue` + * mode now suppresses sim_print_decision()'s bare per-decision line + * entirely (the Round Rollup reports it instead, deferred until the + * collapsed range ends), so this fixture's single round would leave no + * "PEND jobid=..." text to anchor the print-before-breakpoint-check + * ordering this test exists to prove. That ordering + * (sim_print_decision() then xce_repl_on_decision()'s breakpoint + * check, sim_loop.c) is a mode-independent call-sequence invariant -- + * xce_repl_check_decision() (repl.h) gates only on `sim->interactive`, + * never on REPL mode -- so `step` mode observes it exactly as well as + * `continue` did, and `step` still prints the bare per-decision line + * unconditionally (Requirement 5.1, unaffected by task 4.2). `step 1` + * pauses after session 1 completes, but the decision breakpoint below + * fires and nests its own repl_command_loop() first, so the trailing + * `quit` is read from within THAT nested loop -- same "quit ends the + * child deterministically" requirement as the `continue` scripts above, + * just reached via a different pause mechanism. */ + write_text_file(script_path, "break DECISION type=PEND\nstep 1\nquit\n"); captured = run_group_d_and_capture(replay_path, script_path, capture_path); diff --git a/src/scheduler/testsuite/test_repl_execution_control.c b/src/scheduler/testsuite/test_repl_execution_control.c index b55183b2..7261e927 100644 --- a/src/scheduler/testsuite/test_repl_execution_control.c +++ b/src/scheduler/testsuite/test_repl_execution_control.c @@ -526,14 +526,14 @@ static void test_pre_replay_pause_blocks_before_any_event_is_processed(void) char aux_dir[PATH_MAX]; char script_path[PATH_MAX]; char capture_path[PATH_MAX]; - char pend_marker[80]; + char round_marker[96]; char jobid_buf[64]; FILE* f; pid_t pid; int status; char* captured; char* ready_pos; - char* pend_pos; + char* round_pos; f = write_header_and_snapshot(replay_path, sizeof(replay_path), "_prepause"); @@ -574,23 +574,34 @@ static void test_pre_replay_pause_blocks_before_any_event_is_processed(void) captured = read_whole_file(capture_path); - /* bug-1591: sim_print_decision() (sim_loop.c) now renders jobids via - * fmt_jobid(), not raw %llu -- this marker must match that same - * human-readable form. */ + /* scheduler-sim-repl-verbosity task 4.2 (Req 2.3, bug-1610): under + * `continue` mode sim_print_decision()'s bare per-decision line no + * longer prints -- the Round Rollup (repl_rollup.c) reports the + * session's decision instead, deferred until the collapsed range + * ends. This fixture has exactly one round, so that range is only + * flushed (and printed) by xce_repl_on_replay_complete()'s + * repl_round_collapse_flush() call, triggered here by the script's + * trailing `quit`. Look for that round-rollup summary's + * "jobids=<...>" field instead of the old bare "PEND jobid=<...>" + * line as evidence the fixture's one session produced a live + * decision -- bug-1591's fmt_jobid()-vs-raw-%llu fix applies equally + * to repl_fmt_jobid_range()'s single-jobid rendering (repl.c), which + * is byte-identical to fmt_jobid()'s own non-array format. */ fmt_jobid(jobid_buf, sizeof(jobid_buf), 4242ULL); - snprintf(pend_marker, sizeof(pend_marker), "PEND jobid=%s", jobid_buf); + snprintf(round_marker, sizeof(round_marker), "jobids=%s", jobid_buf); ready_pos = strstr(captured, "[sim] ready. type 'help' for commands."); - pend_pos = strstr(captured, pend_marker); + round_pos = strstr(captured, round_marker); TEST_ASSERT_NOT_NULL_MESSAGE(ready_pos, "pre-replay pause banner must be printed"); - TEST_ASSERT_NOT_NULL_MESSAGE(pend_pos, + TEST_ASSERT_NOT_NULL_MESSAGE(round_pos, "the fixture's one session must have " - "produced a live PEND decision"); - TEST_ASSERT_TRUE_MESSAGE(ready_pos < pend_pos, + "produced a live PEND decision, reported " + "via the round rollup summary"); + TEST_ASSERT_TRUE_MESSAGE(ready_pos < round_pos, "the pre-replay pause banner must appear " - "BEFORE any decision output -- proves no " + "BEFORE any round-rollup output -- proves no " "event was processed before the pause"); free(captured); diff --git a/src/scheduler/testsuite/test_sim_reconcile_unaccounted.c b/src/scheduler/testsuite/test_sim_reconcile_unaccounted.c index c01ca5d2..a562ee74 100644 --- a/src/scheduler/testsuite/test_sim_reconcile_unaccounted.c +++ b/src/scheduler/testsuite/test_sim_reconcile_unaccounted.c @@ -22,6 +22,24 @@ * decision uses, and that an already-accounted job produces NO duplicate * report through this mechanism (Requirement 3.3). * + * **Correction (post-review, scheduler-sim-repl-verbosity task 4.2 + * remediation):** the reporting channel's console-print leg is now + * sim_print_decision_gate(sim, d), not a bare sim_print_decision(d) -- + * review found this file's own reconciliation call site had been missed by + * task 4.2's original single-call-site scope (bug-1610). The two tests at + * the bottom of this file (test_reconcile_continue_mode_with_repl_state_ + * suppresses_bare_decision_line / test_reconcile_step_mode_with_repl_ + * state_still_prints_bare_decision_line) pin that gate's contract at THIS + * call site specifically, using the same repl_state_ensure()-based + * direct-call fixture technique test_repl_round_rollup.c's own fixtures + * establish. test_reconcile_reports_reconstructed_decision_through_ + * existing_channel above (task 2.4's original test) deliberately leaves + * sim->repl_state NULL and continues to assert the bare line IS present -- + * empirically confirmed unaffected by this remediation, since a NULL + * repl_state falls through the gate's own NULL-safety default ("never + * load-bearing, degrade to print") exactly like every other REPL hook in + * this codebase. + * * sim_reconcile_unaccounted_jobs() is `static` and not yet called from * anywhere in production code (task 3 wires it into * sched_sim_driver_run()'s round-boundary sequence) -- called directly here @@ -1377,6 +1395,195 @@ test_reconcile_multiple_jobs_captured_independently_across_different_rounds( "different_rounds: PASS\n"); } +/* ---------------------------------------------------------------------- */ +/* Test 16 (scheduler-sim-repl-verbosity task 4.2 remediation, post-review: */ +/* review found this file's own second production call site to */ +/* sim_print_decision() -- inside sim_reconcile_unaccounted_jobs() itself */ +/* -- was never gated by sim_print_decision_gate() (sim_loop.c), the ONE */ +/* other call site task 4.2 originally wired. Since this reconciliation */ +/* path is a PRIMARY source of DISPATCH decisions surfacing in replay mode */ +/* (most live-drain-loop activity is PEND), a round where it fires used to */ +/* show BOTH this bare "[sim] DISPATCH jobid=..." line AND the Round */ +/* Rollup's summary counting the same dispatch -- the exact bug-1610 */ +/* duplication this whole task exists to eliminate, just via this second */ +/* call site. This test proves the fix: with sim->interactive == true AND */ +/* a properly-allocated sim->repl_state in XCE_REPL_MODE_CONTINUE, the bare */ +/* line from THIS mechanism must no longer appear -- the Round Rollup's own */ +/* summary becomes the sole source of decision output here too, matching */ +/* design.md's Requirement 2.3 ("the round summary is the sole source of */ +/* decision output") for real this time. */ +/* ---------------------------------------------------------------------- */ + +static void +test_reconcile_continue_mode_with_repl_state_suppresses_bare_decision_line( + void) +{ + xce_sched_sim_driver_t sim; + xce_sched_job_t* job; + xce_job_alloc_t* jalloc; + replay_session_t* decoded; + sched_record_session_ctx_t* record_ctx; + char tmp[] = "/tmp/xce_simreconcile_gate_continue_XXXXXX"; + int fd = mkstemp(tmp); + int saved_stdout; + off_t len; + char* captured; + + fixture_init(&sim); + + job = add_job(&(sim.core), 7060ULL, JOB_STAT_RUN, NULL); + TEST_ASSERT_NULL(job->accounts); + + jalloc = make_test_job_alloc_with_host(7060ULL, "worker9", "slots", 3.0f); + decoded + = make_decoded_session(1000ULL, make_dispatch_entry_from_alloc(jalloc)); + record_ctx = sched_record_session_ctx_new(1000ULL); + TEST_ASSERT_NOT_NULL(record_ctx); + + /* The gate-relevant fixture shape (unlike test_reconcile_reports_ + * reconstructed_decision_through_existing_channel above, which + * deliberately leaves repl_state NULL to prove the OTHER half of the + * gate's NULL-safety contract): a genuinely allocated repl_state in + * XCE_REPL_MODE_CONTINUE, the same repl_state_ensure() technique + * test_repl_round_rollup.c's own fixtures use for a direct-call (no + * real REPL command loop) test. */ + sim.interactive = true; + sim.repl_state = repl_state_ensure(&sim); + TEST_ASSERT_NOT_NULL(sim.repl_state); + sim.repl_state->mode = XCE_REPL_MODE_CONTINUE; + + TEST_ASSERT_TRUE_MESSAGE(fd >= 0, "mkstemp for stdout capture must " + "succeed"); + fflush(stdout); + saved_stdout = dup(STDOUT_FILENO); + TEST_ASSERT_TRUE(saved_stdout >= 0); + TEST_ASSERT_TRUE(dup2(fd, STDOUT_FILENO) >= 0); + + sim_reconcile_unaccounted_jobs(&sim, decoded, record_ctx); + + fflush(stdout); + dup2(saved_stdout, STDOUT_FILENO); + close(saved_stdout); + + len = lseek(fd, 0, SEEK_CUR); + TEST_ASSERT_TRUE(len >= 0); + captured = (char*)calloc(1, (size_t)len + 1); + TEST_ASSERT_NOT_NULL(captured); + TEST_ASSERT_TRUE(lseek(fd, 0, SEEK_SET) == 0); + if (len > 0) { + ssize_t nread = read(fd, captured, (size_t)len); + TEST_ASSERT_TRUE(nread == len); + } + close(fd); + + /* The reconciliation itself (task 2.3) still happened -- this gate only + * ever affects the console print, nothing else in the reporting chain. */ + TEST_ASSERT_EQUAL_PTR(jalloc, job->allocs); + TEST_ASSERT_NOT_NULL(job->accounts); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 1, record_ctx->n, + "the recording channel (sched_record_session_add_decision()) must " + "still report the reconstructed decision -- only the console print " + "is gated, not the recording"); + + /* THE ASSERTION THIS TEST EXISTS TO PROVE: the bare per-decision line + * must NOT appear via this call site while sim->interactive && + * sim->repl_state->mode == XCE_REPL_MODE_CONTINUE (bug-1610, this + * remediation's own scope). */ + TEST_ASSERT_NULL_MESSAGE( + strstr(captured, "[sim] DISPATCH jobid="), + "under continue mode with a genuinely allocated repl_state, " + "sim_reconcile_unaccounted_jobs()'s own reconstructed-decision " + "report must NOT print the bare per-decision line -- the Round " + "Rollup's own summary is the sole source of decision output here " + "too, closing bug-1610's duplication gap at this second call site"); + + free(captured); + free(sim.repl_state); + printf( + "test_reconcile_continue_mode_with_repl_state_suppresses_bare_" + "decision_line: PASS\n"); +} + +/* ---------------------------------------------------------------------- */ +/* Test 17 (regression-safety half of Test 16): under `step` mode with a */ +/* genuinely allocated repl_state, the bare per-decision line from THIS */ +/* same call site must still appear exactly as before -- only `continue` */ +/* mode is gated (Requirement 5.1's "step mode ... unaffected" guarantee, */ +/* now proven for this second call site too, not just the drain loop's). */ +/* ---------------------------------------------------------------------- */ + +static void +test_reconcile_step_mode_with_repl_state_still_prints_bare_decision_line( + void) +{ + xce_sched_sim_driver_t sim; + xce_sched_job_t* job; + xce_job_alloc_t* jalloc; + replay_session_t* decoded; + char tmp[] = "/tmp/xce_simreconcile_gate_step_XXXXXX"; + int fd = mkstemp(tmp); + int saved_stdout; + off_t len; + char* captured; + + fixture_init(&sim); + + job = add_job(&(sim.core), 7061ULL, JOB_STAT_RUN, NULL); + + jalloc = make_test_job_alloc_with_host(7061ULL, "worker9", "slots", 3.0f); + decoded + = make_decoded_session(1000ULL, make_dispatch_entry_from_alloc(jalloc)); + + sim.interactive = true; + sim.repl_state = repl_state_ensure(&sim); + TEST_ASSERT_NOT_NULL(sim.repl_state); + /* XCE_REPL_MODE_STEP == 0, already repl_state_ensure()'s calloc() + * default -- set explicitly regardless, so this test's intent reads + * clearly and does not silently depend on the enum's numeric value. */ + sim.repl_state->mode = XCE_REPL_MODE_STEP; + + TEST_ASSERT_TRUE_MESSAGE(fd >= 0, "mkstemp for stdout capture must " + "succeed"); + fflush(stdout); + saved_stdout = dup(STDOUT_FILENO); + TEST_ASSERT_TRUE(saved_stdout >= 0); + TEST_ASSERT_TRUE(dup2(fd, STDOUT_FILENO) >= 0); + + sim_reconcile_unaccounted_jobs(&sim, decoded, NULL); + + fflush(stdout); + dup2(saved_stdout, STDOUT_FILENO); + close(saved_stdout); + + len = lseek(fd, 0, SEEK_CUR); + TEST_ASSERT_TRUE(len >= 0); + captured = (char*)calloc(1, (size_t)len + 1); + TEST_ASSERT_NOT_NULL(captured); + TEST_ASSERT_TRUE(lseek(fd, 0, SEEK_SET) == 0); + if (len > 0) { + ssize_t nread = read(fd, captured, (size_t)len); + TEST_ASSERT_TRUE(nread == len); + } + close(fd); + + TEST_ASSERT_EQUAL_PTR(jalloc, job->allocs); + TEST_ASSERT_NOT_NULL(job->accounts); + TEST_ASSERT_NOT_NULL_MESSAGE( + strstr(captured, "[sim] DISPATCH jobid="), + "while single-stepping (repl_state allocated, mode == " + "XCE_REPL_MODE_STEP), sim_reconcile_unaccounted_jobs()'s own " + "reconstructed-decision report must still print the bare " + "per-decision line exactly as before -- only continue mode is " + "gated"); + + free(captured); + free(sim.repl_state); + printf( + "test_reconcile_step_mode_with_repl_state_still_prints_bare_" + "decision_line: PASS\n"); +} + int main(void) { UNITY_BEGIN(); @@ -1400,5 +1607,9 @@ int main(void) RUN_TEST(test_reconcile_no_match_leaves_pend_job_uncaptured_and_retries); RUN_TEST( test_reconcile_multiple_jobs_captured_independently_across_different_rounds); + RUN_TEST( + 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); return UNITY_END(); }