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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RLSNJhNoULvVcUhXEFafQC
This commit is contained in:
parent
9f54c5753f
commit
8477f677e4
3 changed files with 175 additions and 3 deletions
|
|
@ -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 <name>` 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 <replay>` 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 `"<resname> = <value>"` 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.
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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 <name>` 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(
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue