From 8477f677e4b3730fed91a244d8daf6d0cf27f607 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Sun, 20 Sep 2026 05:58:41 +0000 Subject: [PATCH] feat(scheduler-sim-diagnostics): wire info limit to fmt_limit_units (task 8.2) Switches both fmt_limit() call sites in repl_info_limit() (the "all" path and the named-lookup path) to fmt_limit_units(..., translate_units_mode(repl_current_units_mode(sim))), so a byte-valued limit's configured amount now renders human-readable under the REPL's real, currently-selected units mode -- the same convention info worker/info resource already honor. Adds two tests proving human-readable rendering under the default mode and consistent switching across set units raw|human|both via real repl_dispatch(). RED confirmed by reverting the call sites (2 failures, 28 pre-existing tests in the same file unaffected). Corrects an inaccurate self-reported regression-safety rationale (there is no slots-resource fixture in this file; safety instead comes from those tests never asserting on the formatted value line), logged in tasks.md's Implementation Notes. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01RLSNJhNoULvVcUhXEFafQC --- .../specs/scheduler-sim-diagnostics/tasks.md | 3 +- src/scheduler/src/repl_info.c | 8 +- .../testsuite/test_repl_limit_table.c | 167 ++++++++++++++++++ 3 files changed, 175 insertions(+), 3 deletions(-) diff --git a/.kiro/specs/scheduler-sim-diagnostics/tasks.md b/.kiro/specs/scheduler-sim-diagnostics/tasks.md index 422b9c10..e15b478e 100644 --- a/.kiro/specs/scheduler-sim-diagnostics/tasks.md +++ b/.kiro/specs/scheduler-sim-diagnostics/tasks.md @@ -84,7 +84,7 @@ - _Depends: 7_ - _Boundary: Shared Units Formatter_ -- [ ] 8.2 Show configured limit amounts in the operator's selected display mode +- [x] 8.2 Show configured limit amounts in the operator's selected display mode - The REPL's limit configuration display now renders a byte-valued limit's configured amount using the operator's already-selected display mode, reusing the scheduler-side bridge task 7 introduced, the same mode every other byte-valued resource display in the REPL already honors - Observable: `info limit ` against a byte-valued limit shows its configured amount in human-readable form under the REPL's default display mode, and the rendering changes consistently when the operator switches display modes - _Requirements: 6.1, 6.2, 6.5_ @@ -129,3 +129,4 @@ Tasks 7–10 (Requirement 6) follow the same reasoning: task 9 has no genuine da - **Task 5 — significant follow-up needed (logged as `bug-1825`)**: all 5 tasks are correctly implemented, thoroughly tested, and genuinely read-only/deterministic — but `info limit` is currently **inert in real production usage**. `register_limit()` has exactly one call site in the whole codebase (`scheduler.c`'s normal-mode `sched_register_plugins()`); sim mode's own plugin bootstrap (`sim_bootstrap_picker()`, `sim_picker.c`) only registers the picker plugin, never the limit plugin. So a real operator running `xce_scheduler -i`/`-s ` will always see "no limits configured"/"no such limit" from `info limit`, regardless of what the original live recording had configured — every positive-display test in this spec exercises the code via a test-only plugin-registration helper with no production equivalent in sim mode. This is a pre-existing sim-mode driver gap (same class as `scheduler-driver-equivalence.md`'s other documented normal-vs-sim parity gaps), not a defect in this spec's own work, and was out of task 5's test-only boundary to fix. Fixed by task 6. - **Task 6 — a real memory-safety lesson, caught by review before it shipped**: the first implementation attempt set `plugin->sched = (void*)sim` (a `xce_sched_sim_driver_t*`, disguised) before invoking the limit plugin's `config_hook`, relying on both `xce_scheduler_t` and `xce_sched_sim_driver_t` happening to have `xce_sched_core_t core` as their first struct member. An independent reviewer rejected this: `core.h`'s own `xce_core_cache_clear_cb_t` doc comment documents that this EXACT cross-cast pattern already corrupted memory once before in this codebase (`cache_clear()` reinterpreting `xce_sched_sim_driver_t`'s `alt_events_file` as a `kvtab_t*`), and there was no compiler/runtime guard against a future edit to the callee reading past `.core`. Fixed by adding `limit_seed_config(plugin, xce_sched_core_t* core, config)` — an honestly-typed entry point taking `core` as a real field access, no cast — with `limit_update_config()` (normal mode) and `sim_bootstrap_limit_plugin()` (sim mode) both delegating to a shared `_impl` function. **Lesson for future work in this codebase**: when a plugin hook or helper is invoked from BOTH a normal-mode (`xce_scheduler_t`) and a sim-mode (`xce_sched_sim_driver_t`) context, never derive one struct type from the other via a first-member cast, even when it "happens to work" today — take `xce_sched_core_t*` as an honest parameter instead. Two independent reviewers (one per round) verified this by reading the actual struct definitions and the full call chain, not by trusting the report. - **Task 7 — a genuine, narrow behavioral corner case surfaced by the promotion, deliberately left as-is**: the pre-existing `repl_format_numeric_resource_value()` had an asymmetry for `"swp"` (whose wire unit is megabytes, not bytes): RAW and BOTH modes printed the *un-converted* raw value, while only HUMAN mode used the MB→bytes-converted value for scaling. The new shared `xce_format_units_value(mode, bytes_value, ...)` interface — a single value argument, matching design.md's specified shape exactly — cannot preserve that asymmetry: `swp` under RAW/BOTH mode in `info worker`/`info resource` now renders the bytes-converted number instead of the original raw MB number. Confirmed by both the implementer and an independent reviewer: no test in the codebase exercises `swp` under RAW or BOTH mode (all existing `swp` tests use HUMAN only), and `swp` is not in Requirement 6's byte-valued-resource list (`mem`/`tmp`/`maxmem`/`maxtmp`/`maxswap`), so this is untested and out of this requirement's scope — not a regression caught by any existing coverage, but technically in tension with Requirement 5.3's literal "preserve all existing behavior unchanged." Left as-is rather than special-cased, since fixing it would mean giving the shared composer an `swp`-specific parameter it has no other reason to carry. If `swp`'s RAW/BOTH display is ever reported as user-visibly wrong, start here. +- **Task 8.2 — implementer's own stated regression-safety rationale was inaccurate, though its conclusion held**: the report claimed the 28 pre-existing `test_repl_limit_table.c` tests stay green because they use "slots-based (non-byte-valued)" fixtures unaffected by units-mode. An independent reviewer found this false — every configured-limit fixture in that file hardcodes resource name `"mem"` (a byte-valued resource); `slots`-looking names in that file (`ug1_slots`, etc.) are limit *names*, not resource names. The real reason those 28 tests stay green is that none of them assert on the formatted `" = "` line's exact text at all — they assert only on limit names, group membership/exclusion text, host-scope-note presence, and separate usage/accounting lines. Lesson: when a report explains *why* an existing test suite is unaffected by a change, verify the stated mechanism against the actual fixtures, not just the pass/fail outcome — a correct outcome can still carry an incorrect explanation, which would mislead the next person who touches this test file expecting a `slots` fixture to exist. diff --git a/src/scheduler/src/repl_info.c b/src/scheduler/src/repl_info.c index bd6bc086..f3176c3b 100644 --- a/src/scheduler/src/repl_info.c +++ b/src/scheduler/src/repl_info.c @@ -1549,7 +1549,9 @@ static void repl_info_limit(xce_sched_sim_driver_t* sim, const char* arg) } { char limit_buf[1024]; - int fmt_rc = fmt_limit(limit_buf, sizeof(limit_buf), limit); + int fmt_rc = fmt_limit_units( + limit_buf, sizeof(limit_buf), limit, + translate_units_mode(repl_current_units_mode(sim))); if (fmt_rc >= 0) { repl_print_limit_dump(limit_buf); @@ -1580,7 +1582,9 @@ static void repl_info_limit(xce_sched_sim_driver_t* sim, const char* arg) { char limit_buf[1024]; - int fmt_rc = fmt_limit(limit_buf, sizeof(limit_buf), match); + int fmt_rc = fmt_limit_units( + limit_buf, sizeof(limit_buf), match, + translate_units_mode(repl_current_units_mode(sim))); if (fmt_rc >= 0) { repl_print_limit_dump(limit_buf); diff --git a/src/scheduler/testsuite/test_repl_limit_table.c b/src/scheduler/testsuite/test_repl_limit_table.c index 4c5ab2af..fdca8175 100644 --- a/src/scheduler/testsuite/test_repl_limit_table.c +++ b/src/scheduler/testsuite/test_repl_limit_table.c @@ -657,6 +657,38 @@ static void add_configured_percent_limit_explicit_per_host( TEST_ASSERT_EQUAL_INT(0, rc); } +/* + * add_configured_limit_with_value - task 8.2's own fixture: an + * absolute-value (percent=0) limit on `resname` with a caller-supplied + * `val_amount`, so a test can pick a byte amount large enough to actually + * exercise xce_format_bytes_human()'s KB/MB/GB scaling (make_limit_config()'s + * own fixed 10.0 is too small to distinguish "raw" from "human" rendering in + * a reader-legible way -- both render as single-digit text). Otherwise + * identical in shape to add_configured_percent_limit() above (same + * `xce_limit_config_t`-on-the-stack, single-limit `xce_conf4sched_t`, real + * `limit_update_config()` drive). + */ +static void add_configured_limit_with_value(xce_sched_plugin_t* plugin, + const char* name, + const char* resname, + double val_amount) +{ + xce_limit_val_t val = { .resname = (char*)resname, + .percent = 0, + .val = val_amount }; + xce_limit_config_t limit_cfg = { .name = (char*)name, .val = &val }; + xce_limit_config_t* limits[1] = { &limit_cfg }; + xce_conf4sched_t config; + int rc; + + memset(&config, 0, sizeof(config)); + config.nlimits = 1; + config.limits = limits; + + rc = limit_update_config(plugin, &config); + TEST_ASSERT_EQUAL_INT(0, rc); +} + /* ---------------------------------------------------------------------- */ /* Live-usage fixture helpers (task 3.1, Requirement 2.1/2.2/2.3/3.3/1.4) */ /* ---------------------------------------------------------------------- */ @@ -1724,6 +1756,137 @@ test_info_limit_dispatch_all_reaches_repl_info_limit_via_real_dispatch(void) "dispatch: PASS\n"); } +/* ---------------------------------------------------------------------- */ +/* Task 8.2 (Requirement 6.1/6.2/6.5): the configured-setting display must */ +/* render a byte-valued limit's amount using the operator's own selected */ +/* units mode, via fmt_limit_units()/translate_units_mode()/ */ +/* repl_current_units_mode() (tasks 7/8.1), not the RAW-only fmt_limit(). */ +/* ---------------------------------------------------------------------- */ + +/* + * Direct-call path (no repl_state ever allocated -- sim.repl_state stays + * NULL, so repl_current_units_mode() falls back to its own documented + * default, XCE_REPL_UNITS_BOTH -- the same default repl_state_ensure() + * itself assigns). Requirement 6.1/6.2: under that default mode, a + * byte-valued ("mem") limit's configured amount must show human-readable + * scaling, not a bare byte count -- proving the REPL's own already-selected + * display mode (defaulted or explicit) reaches fmt_limit_units(), not + * fmt_limit()'s hardcoded XCE_UNITS_RAW. + */ +static void +test_info_limit_named_shows_byte_valued_amount_human_readable_under_default_units_mode( + void) +{ + xce_sched_sim_driver_t sim; + xce_sched_plugin_t plugin; + xce_scheduler_t group_sched; + char* captured; + int rc; + + memset(&sim, 0, sizeof(sim)); + rc = xce_sched_core_init(&(sim.core)); + TEST_ASSERT_EQUAL_INT(0, rc); + + register_limit_plugin(&sim, &plugin, &group_sched); + /* 5 GiB (5 * 1024^3 bytes) -- large enough that xce_format_bytes_human() + * scales it all the way to "5.00 GB", unmistakably distinct from the raw + * "5368709120.000000" fmt_limit()'s hardcoded RAW mode would print. */ + add_configured_limit_with_value(&plugin, "mem_limit", "mem", + 5368709120.0); + + captured = capture_info_limit(&sim, "mem_limit"); + + TEST_ASSERT_NOT_NULL_MESSAGE( + strstr(captured, "limit: mem_limit"), + "sanity: the matched limit's fmt_limit_units() dump must still " + "print the limit's own name line"); + TEST_ASSERT_NOT_NULL_MESSAGE( + strstr(captured, "5.00 GB"), + "Requirement 6.1/6.2: under the REPL's default units mode (BOTH), " + "a byte-valued limit's configured amount must render in " + "human-readable form -- this fails against today's fmt_limit()-" + "calling code, which is hardcoded to XCE_UNITS_RAW regardless of " + "the operator's selected/default display mode and would only ever " + "print the raw byte count"); + + free(captured); + + printf( + "test_info_limit_named_shows_byte_valued_amount_human_readable_" + "under_default_units_mode: PASS\n"); +} + +/* + * Dispatch path, through the real `set units raw|human|both` command + * (repl_params.c) followed by the real `info limit ` dispatch -- + * Requirement 6.2/6.5: switching the operator's selected display mode must + * consistently change a byte-valued limit's configured-amount rendering, + * the same units convention every other byte-valued resource display in the + * REPL already honors. + */ +static void +test_info_limit_named_configured_amount_changes_consistently_with_units_mode( + void) +{ + xce_sched_sim_driver_t sim; + xce_sched_plugin_t plugin; + xce_scheduler_t group_sched; + char* captured_raw; + char* captured_human; + char* captured_both; + bool handled; + int rc; + + memset(&sim, 0, sizeof(sim)); + rc = xce_sched_core_init(&(sim.core)); + TEST_ASSERT_EQUAL_INT(0, rc); + + sim.repl_state = repl_state_ensure(&sim); + TEST_ASSERT_NOT_NULL(sim.repl_state); + + register_limit_plugin(&sim, &plugin, &group_sched); + add_configured_limit_with_value(&plugin, "mem_limit2", "mem", + 5368709120.0); + + handled = repl_dispatch(&sim, "set units raw"); + TEST_ASSERT_FALSE(handled); + captured_raw = capture_dispatch(&sim, "info limit mem_limit2"); + TEST_ASSERT_NOT_NULL_MESSAGE( + strstr(captured_raw, "mem = 5368709120.000000"), + "`set units raw` must render the configured amount as a bare byte " + "count, matching fmt_limit()'s pre-existing RAW-mode contract"); + TEST_ASSERT_NULL_MESSAGE( + strstr(captured_raw, "GB"), + "RAW mode must not include any human-readable unit suffix"); + free(captured_raw); + + handled = repl_dispatch(&sim, "set units human"); + TEST_ASSERT_FALSE(handled); + captured_human = capture_dispatch(&sim, "info limit mem_limit2"); + TEST_ASSERT_NOT_NULL_MESSAGE( + strstr(captured_human, "mem = 5.00 GB"), + "Requirement 6.1/6.2: `set units human` must render the configured " + "amount in human-readable form alone"); + TEST_ASSERT_NULL_MESSAGE( + strstr(captured_human, "5368709120"), + "HUMAN mode must not include the raw byte count"); + free(captured_human); + + handled = repl_dispatch(&sim, "set units both"); + TEST_ASSERT_FALSE(handled); + captured_both = capture_dispatch(&sim, "info limit mem_limit2"); + TEST_ASSERT_NOT_NULL_MESSAGE( + strstr(captured_both, "mem = 5368709120.000000 (5.00 GB)"), + "Requirement 6.5: `set units both` must render raw and " + "human-readable together, the same combined shape every other " + "byte-valued resource display in the REPL already uses"); + free(captured_both); + + printf( + "test_info_limit_named_configured_amount_changes_consistently_with_" + "units_mode: PASS\n"); +} + /* ---------------------------------------------------------------------- */ /* Task 5 (Requirement Set 1's own Validation task, Requirement 5): proves */ /* -- rather than merely asserting by construction, as every prior task's */ @@ -2964,6 +3127,10 @@ int main(void) test_info_limit_dispatch_named_reaches_repl_info_limit_via_real_dispatch); RUN_TEST( test_info_limit_dispatch_all_reaches_repl_info_limit_via_real_dispatch); + RUN_TEST( + test_info_limit_named_shows_byte_valued_amount_human_readable_under_default_units_mode); + RUN_TEST( + test_info_limit_named_configured_amount_changes_consistently_with_units_mode); RUN_TEST( test_info_limit_never_mutates_global_limit_config_or_accounting_state); RUN_TEST(