diff --git a/.kiro/specs/scheduler-sim-decision-trace/tasks.md b/.kiro/specs/scheduler-sim-decision-trace/tasks.md index 80307412..5e6d0a93 100644 --- a/.kiro/specs/scheduler-sim-decision-trace/tasks.md +++ b/.kiro/specs/scheduler-sim-decision-trace/tasks.md @@ -14,7 +14,7 @@ - Done when the new baseline test passes on the unmodified tree under ASAN and the fixtures are committed. - _Requirements: 10.4, 10.5_ -- [ ] 1.2 Move the units and value formatting helpers into the shared REPL formatting module +- [x] 1.2 Move the units and value formatting helpers into the shared REPL formatting module - Move these helpers out of the info module so a new renderer can reuse them, with no change in behaviour: - units-mode translation; - numeric-resource and consumable value formatting; diff --git a/src/scheduler/src/repl_fmt.c b/src/scheduler/src/repl_fmt.c index a1cac723..a4442f12 100644 --- a/src/scheduler/src/repl_fmt.c +++ b/src/scheduler/src/repl_fmt.c @@ -18,12 +18,12 @@ * directly in repl.c per the original design.md plan and pushed it to 929 * lines, 16.1% over this project's 800-line-per-file cap * (src/scheduler/CLAUDE.md), even though review confirmed the logic itself - * was correct. This file now holds three groups of pure formatting helpers - * that repl.c's own file header already called "the file's existing - * shared-helper convention" -- none of them touch - * xce_sched_sim_driver_t/xce_sched_core_t or any REPL session state, so - * grouping them here (rather than splitting each into its own file) keeps - * one cohesive, dependency-free unit: + * was correct. This file now holds groups of formatting helpers that + * repl.c's own file header already called "the file's existing + * shared-helper convention" -- none of them modify + * xce_sched_sim_driver_t/xce_sched_core_t or REPL session state, and only + * repl_current_units_mode() reads it, so grouping them here (rather than + * splitting each into its own file) keeps one cohesive unit: * * - repl_format_ts_human() (pre-existing, bug-1587) -- epoch-nanoseconds * to human-readable UTC timestamp. @@ -33,6 +33,12 @@ * - The Resource Value Formatter's classification/scaling helpers (task * 7.2, design.md's "Resource Value Formatter" component, Req 6.1, 6.2): * repl_is_bytes_valued_resource()/repl_format_bytes_human(). + * - Units-mode value formatting (scheduler-sim-decision-trace spec, task + * 1.2, Req 7.7, moved from repl_info.c so the trace renderer can share + * it): translate_units_mode(), repl_format_numeric_resource_value(), + * repl_format_consumable_value_and_used() and repl_current_units_mode(). + * The last reads sim->repl_state->units_mode -- the one helper here + * that looks at REPL session state, read-only. * * Every one of these functions is already extern-declared in * repl_internal.h -- this move changes only which .c file DEFINES them, @@ -357,3 +363,129 @@ int repl_fmt_jobid_range(char* buf, size_t bufsz, const uint64_t* jobids, return rc; } + +/* + * Units-mode value formatting (moved verbatim from repl_info.c by the + * scheduler-sim-decision-trace spec, task 1.2, Req 7.7; behaviour + * unchanged, now non-static and declared in repl_internal.h) so the trace + * renderer (repl_trace_render.c) formats values exactly as `info worker`/ + * `info resource`/`info limit` do. + */ + +/* + * translate_units_mode - scheduler-sim-diagnostics spec, task 7 (Req 6.1, + * 6.3, 6.5): bridges the REPL's own units-mode selection + * (xce_repl_units_mode_t, repl_internal.h) to the shared library's + * independently-defined xce_units_mode_t (utils.h) at the one seam that + * needs to cross between them. Deliberately NOT a shared enum -- a + * src/lib type must not be defined in terms of a scheduler-internal + * concept, nor should the scheduler's own already-tested + * xce_repl_units_mode_t plumbing need to change for this (design.md's + * "Where the byte-formatter should live" decision). If + * xce_repl_units_mode_t's three values ever change, this mapping must be + * re-checked by hand -- nothing enforces it mechanically. + */ +xce_units_mode_t translate_units_mode(xce_repl_units_mode_t mode) +{ + switch (mode) { + case XCE_REPL_UNITS_RAW: + return XCE_UNITS_RAW; + case XCE_REPL_UNITS_HUMAN: + return XCE_UNITS_HUMAN; + case XCE_REPL_UNITS_BOTH: + default: + return XCE_UNITS_BOTH; + } +} + +/* + * repl_format_numeric_resource_value - Requirement 6.1/6.2/6.5: formats the + * text that follows "value=" for a non-consumable ("Load Metrics") numeric + * resource. A byte-valued resource (repl_is_bytes_valued_resource()) or + * `swp` (repl_internal.h's own documented MB-special-case, converted + * x1024x1024 before scaling) renders per `mode` -- raw: the bare "%f" wire + * value; human: repl_format_bytes_human()'s scaled form alone; both (the + * default): " ()", the exact "%f (%s)" precedent xce_test's own + * cmd_resval already establishes (test.c:2540). Every other numeric + * resource renders with no unit suffix and no trailing ".00" on a whole + * number, keeping two decimal places for a fractional one. Shared verbatim + * by repl_info_worker_one()'s and repl_info_resource_one()'s identical Load + * Metrics rendering so the two commands can never disagree about this + * format. + * + * scheduler-sim-diagnostics spec, task 7 (Req 6.1, 6.3, 6.5): the + * byte-valued branch now delegates its raw/human/both composition to the + * shared library's xce_format_units_value() via translate_units_mode() + * above, instead of switching on `mode` itself -- so this function, its + * `info worker`/`info resource` callers, AND the `info limit` command + * this spec is adding all funnel through one single implementation. + */ +void repl_format_numeric_resource_value(xce_repl_units_mode_t mode, + const char* rname, double value, + char* buf, size_t bufsz) +{ + bool is_swp = (rname != NULL) && (strcmp(rname, "swp") == 0); + + if (repl_is_bytes_valued_resource(rname) || is_swp) { + double bytes_value = is_swp ? (value * 1024.0 * 1024.0) : value; + + xce_format_units_value(translate_units_mode(mode), bytes_value, buf, + bufsz); + return; + } + + /* Requirement 6.5: no trailing ".00" on a whole-number value; a + * fractional value keeps its two decimal places. */ + if (value == (double)(long long)value) { + snprintf(buf, bufsz, "%.0f", value); + } else { + snprintf(buf, bufsz, "%.2f", value); + } +} + +/* + * repl_format_consumable_value_and_used - formats a CONSUMABLE numeric + * resource's value=/used= pair for repl_info_worker_one()'s per-host case + * and repl_info_resource_one()'s GLOBAL/SHARED case. A byte/megabyte-valued + * resource (repl_is_bytes_valued_resource(), or `swp`'s documented MB + * special case -- the SAME classification repl_format_numeric_resource_value() + * already applies to the Load Metrics section) renders BOTH `avail` and + * `used` through that same formatter, respecting `mode` -- so + * `used=87960930222080.00` (a live-reported bug, 2026-09-11) instead + * becomes e.g. `used=87960930222080.000000 (80.00 TB)` under the default + * "both" mode, matching how maxmem/maxswap/maxtmp already render alongside + * it. Every other consumable resource (slots, licenses, ...) keeps the + * existing pinned "%.2f"/"%.2f" shape unchanged -- bug-1601/1613/1727's own + * tests assert this exact format and must not regress. + */ +void repl_format_consumable_value_and_used(xce_repl_units_mode_t mode, + const char* rname, double avail, + double used, char* value_buf, + size_t value_bufsz, char* used_buf, + size_t used_bufsz) +{ + bool is_swp = (rname != NULL) && (strcmp(rname, "swp") == 0); + + if (repl_is_bytes_valued_resource(rname) || is_swp) { + repl_format_numeric_resource_value(mode, rname, avail, value_buf, + value_bufsz); + repl_format_numeric_resource_value(mode, rname, used, used_buf, + used_bufsz); + return; + } + + snprintf(value_buf, value_bufsz, "%.2f", avail); + snprintf(used_buf, used_bufsz, "%.2f", used); +} + +/* repl_current_units_mode - sim->repl_state may legitimately be NULL (a few + * white-box tests dispatch `info worker`/`info resource` without ever + * calling repl_state_ensure()); default to the same XCE_REPL_UNITS_BOTH + * repl_state_ensure() itself explicitly assigns (Req 6.3), rather than + * leaving repl_format_numeric_resource_value() to guess. */ +xce_repl_units_mode_t +repl_current_units_mode(xce_sched_sim_driver_t* sim) +{ + return (sim->repl_state != NULL) ? sim->repl_state->units_mode + : XCE_REPL_UNITS_BOTH; +} diff --git a/src/scheduler/src/repl_info.c b/src/scheduler/src/repl_info.c index 40cfe110..c7200c70 100644 --- a/src/scheduler/src/repl_info.c +++ b/src/scheduler/src/repl_info.c @@ -369,126 +369,10 @@ repl_classify_resource_def_section(const xce_resource_t* r) : XCE_REPL_RES_SECTION_LOAD; } -/* - * translate_units_mode - scheduler-sim-diagnostics spec, task 7 (Req 6.1, - * 6.3, 6.5): bridges the REPL's own units-mode selection - * (xce_repl_units_mode_t, repl_internal.h) to the shared library's - * independently-defined xce_units_mode_t (utils.h) at the one seam that - * needs to cross between them. Deliberately NOT a shared enum -- a - * src/lib type must not be defined in terms of a scheduler-internal - * concept, nor should the scheduler's own already-tested - * xce_repl_units_mode_t plumbing need to change for this (design.md's - * "Where the byte-formatter should live" decision). If - * xce_repl_units_mode_t's three values ever change, this mapping must be - * re-checked by hand -- nothing enforces it mechanically. - */ -static xce_units_mode_t translate_units_mode(xce_repl_units_mode_t mode) -{ - switch (mode) { - case XCE_REPL_UNITS_RAW: - return XCE_UNITS_RAW; - case XCE_REPL_UNITS_HUMAN: - return XCE_UNITS_HUMAN; - case XCE_REPL_UNITS_BOTH: - default: - return XCE_UNITS_BOTH; - } -} - -/* - * repl_format_numeric_resource_value - Requirement 6.1/6.2/6.5: formats the - * text that follows "value=" for a non-consumable ("Load Metrics") numeric - * resource. A byte-valued resource (repl_is_bytes_valued_resource()) or - * `swp` (repl_internal.h's own documented MB-special-case, converted - * x1024x1024 before scaling) renders per `mode` -- raw: the bare "%f" wire - * value; human: repl_format_bytes_human()'s scaled form alone; both (the - * default): " ()", the exact "%f (%s)" precedent xce_test's own - * cmd_resval already establishes (test.c:2540). Every other numeric - * resource renders with no unit suffix and no trailing ".00" on a whole - * number, keeping two decimal places for a fractional one. Shared verbatim - * by repl_info_worker_one()'s and repl_info_resource_one()'s identical Load - * Metrics rendering so the two commands can never disagree about this - * format. - * - * scheduler-sim-diagnostics spec, task 7 (Req 6.1, 6.3, 6.5): the - * byte-valued branch now delegates its raw/human/both composition to the - * shared library's xce_format_units_value() via translate_units_mode() - * above, instead of switching on `mode` itself -- so this function, its - * `info worker`/`info resource` callers, AND the `info limit` command - * this spec is adding all funnel through one single implementation. - */ -static void repl_format_numeric_resource_value(xce_repl_units_mode_t mode, - const char* rname, - double value, char* buf, - size_t bufsz) -{ - bool is_swp = (rname != NULL) && (strcmp(rname, "swp") == 0); - - if (repl_is_bytes_valued_resource(rname) || is_swp) { - double bytes_value = is_swp ? (value * 1024.0 * 1024.0) : value; - - xce_format_units_value(translate_units_mode(mode), bytes_value, buf, - bufsz); - return; - } - - /* Requirement 6.5: no trailing ".00" on a whole-number value; a - * fractional value keeps its two decimal places. */ - if (value == (double)(long long)value) { - snprintf(buf, bufsz, "%.0f", value); - } else { - snprintf(buf, bufsz, "%.2f", value); - } -} - -/* - * repl_format_consumable_value_and_used - formats a CONSUMABLE numeric - * resource's value=/used= pair for repl_info_worker_one()'s per-host case - * and repl_info_resource_one()'s GLOBAL/SHARED case. A byte/megabyte-valued - * resource (repl_is_bytes_valued_resource(), or `swp`'s documented MB - * special case -- the SAME classification repl_format_numeric_resource_value() - * already applies to the Load Metrics section) renders BOTH `avail` and - * `used` through that same formatter, respecting `mode` -- so - * `used=87960930222080.00` (a live-reported bug, 2026-09-11) instead - * becomes e.g. `used=87960930222080.000000 (80.00 TB)` under the default - * "both" mode, matching how maxmem/maxswap/maxtmp already render alongside - * it. Every other consumable resource (slots, licenses, ...) keeps the - * existing pinned "%.2f"/"%.2f" shape unchanged -- bug-1601/1613/1727's own - * tests assert this exact format and must not regress. - */ -static void repl_format_consumable_value_and_used(xce_repl_units_mode_t mode, - const char* rname, - double avail, double used, - char* value_buf, - size_t value_bufsz, - char* used_buf, - size_t used_bufsz) -{ - bool is_swp = (rname != NULL) && (strcmp(rname, "swp") == 0); - - if (repl_is_bytes_valued_resource(rname) || is_swp) { - repl_format_numeric_resource_value(mode, rname, avail, value_buf, - value_bufsz); - repl_format_numeric_resource_value(mode, rname, used, used_buf, - used_bufsz); - return; - } - - snprintf(value_buf, value_bufsz, "%.2f", avail); - snprintf(used_buf, used_bufsz, "%.2f", used); -} - -/* repl_current_units_mode - sim->repl_state may legitimately be NULL (a few - * white-box tests dispatch `info worker`/`info resource` without ever - * calling repl_state_ensure()); default to the same XCE_REPL_UNITS_BOTH - * repl_state_ensure() itself explicitly assigns (Req 6.3), rather than - * leaving repl_format_numeric_resource_value() to guess. */ -static xce_repl_units_mode_t -repl_current_units_mode(xce_sched_sim_driver_t* sim) -{ - return (sim->repl_state != NULL) ? sim->repl_state->units_mode - : XCE_REPL_UNITS_BOTH; -} +/* translate_units_mode / repl_format_numeric_resource_value / + * repl_format_consumable_value_and_used / repl_current_units_mode moved to + * repl_fmt.c (scheduler-sim-decision-trace spec, task 1.2, Req 7.7) so the + * trace renderer can share them; declared in repl_internal.h. */ /* repl_append_u64 - grows `*arr`/`*n` by one element, the same exact-size * per-append realloc idiom repl_fmt.c's repl_jobid_group_add_idx() diff --git a/src/scheduler/src/repl_internal.h b/src/scheduler/src/repl_internal.h index bffac12f..8f5dc770 100644 --- a/src/scheduler/src/repl_internal.h +++ b/src/scheduler/src/repl_internal.h @@ -3,6 +3,7 @@ #define _REPL_INTERNAL_H_ 1 #include "repl.h" +#include "utils.h" /* xce_units_mode_t (translate_units_mode) */ /* * repl_internal.h - private state and cross-file declarations shared by @@ -354,6 +355,32 @@ extern void repl_format_ts_human(uint64_t ns, char* buf, size_t bufsz); extern bool repl_is_bytes_valued_resource(const char* name); extern void repl_format_bytes_human(double bytes, char* buf, size_t bufsz); +/* + * Units-mode value formatting (repl_fmt.c, moved from repl_info.c by the + * scheduler-sim-decision-trace spec, task 1.2, Req 7.7, behaviour + * unchanged) -- shared by repl_info.c's `info worker`/`info resource`/ + * `info limit` and the trace renderer so both format values identically: + * - translate_units_mode: maps xce_repl_units_mode_t to the shared + * library's xce_units_mode_t (utils.h). + * - repl_format_numeric_resource_value: the text after "value=" for a + * numeric resource; bytes-valued names and `swp` (MB, converted to + * bytes) render per `mode`, others as "%.0f" (whole) or "%.2f". + * - repl_format_consumable_value_and_used: a consumable's value=/used= + * pair; bytes-valued names as above, others the pinned "%.2f"/"%.2f". + * - repl_current_units_mode: sim->repl_state->units_mode, or + * XCE_REPL_UNITS_BOTH when no REPL state exists yet. + */ +extern xce_units_mode_t translate_units_mode(xce_repl_units_mode_t mode); +extern void repl_format_numeric_resource_value(xce_repl_units_mode_t mode, + const char* rname, + double value, char* buf, + size_t bufsz); +extern void repl_format_consumable_value_and_used( + xce_repl_units_mode_t mode, const char* rname, double avail, double used, + char* value_buf, size_t value_bufsz, char* used_buf, size_t used_bufsz); +extern xce_repl_units_mode_t +repl_current_units_mode(xce_sched_sim_driver_t* sim); + /* repl_command_loop (repl.c) - the blocking prompt loop. Breakpoint hits * (repl_break.c's xce_repl_on_event()/on_begin_round()/on_decision()) call * back into it directly, the same re-entry xce_repl_post_session_check() diff --git a/src/scheduler/testsuite/test_repl_resource_formatting.c b/src/scheduler/testsuite/test_repl_resource_formatting.c index e821d852..ff3cf602 100644 --- a/src/scheduler/testsuite/test_repl_resource_formatting.c +++ b/src/scheduler/testsuite/test_repl_resource_formatting.c @@ -89,6 +89,17 @@ #include +/* scheduler-sim-decision-trace spec, task 1.2 (Req 7.7): the units-mode + * translation, numeric/consumable value formatters and current-units + * lookup moved from repl_info.c (file-static) to repl_fmt.c, declared in + * repl_internal.h so other REPL files (repl_trace_render.c) can reuse them. + * Included here -- a translation unit separate from repl_info.c/repl_fmt.c, + * which this target links as objects -- to prove the declarations are + * reachable from another file. */ +#include "repl_internal.h" +#include "sim.h" +#include "utils.h" + /* * repl_is_bytes_valued_resource / repl_format_bytes_human (task 7.2, NOT YET * IMPLEMENTED -- see this file's own header comment above for why these are @@ -311,6 +322,90 @@ static void test_swp_value_converted_mb_to_bytes_renders_on_same_adaptive_scale( "scale: PASS\n"); } +/* ---------------------------------------------------------------------- */ +/* scheduler-sim-decision-trace task 1.2 (Req 7.7): the shared units/value */ +/* formatting helpers are callable from a separate translation unit through */ +/* repl_internal.h, with their pre-move behaviour unchanged. */ +/* ---------------------------------------------------------------------- */ + +static void test_shared_translate_units_mode_maps_each_repl_mode(void) +{ + TEST_ASSERT_EQUAL_INT(XCE_UNITS_RAW, + translate_units_mode(XCE_REPL_UNITS_RAW)); + TEST_ASSERT_EQUAL_INT(XCE_UNITS_HUMAN, + translate_units_mode(XCE_REPL_UNITS_HUMAN)); + TEST_ASSERT_EQUAL_INT(XCE_UNITS_BOTH, + translate_units_mode(XCE_REPL_UNITS_BOTH)); +} + +static void test_shared_numeric_resource_value_formats_per_units_mode(void) +{ + char buf[128]; + char want[128]; + + /* Unitless: whole number has no ".00"; fraction keeps two decimals. */ + repl_format_numeric_resource_value(XCE_REPL_UNITS_BOTH, "r1m", 3.0, buf, + sizeof(buf)); + TEST_ASSERT_EQUAL_STRING("3", buf); + repl_format_numeric_resource_value(XCE_REPL_UNITS_RAW, "r1m", 2.5, buf, + sizeof(buf)); + TEST_ASSERT_EQUAL_STRING("2.50", buf); + + /* Bytes-valued: delegated to xce_format_units_value() per mode. */ + repl_format_numeric_resource_value(XCE_REPL_UNITS_HUMAN, "mem", 1024.0, + buf, sizeof(buf)); + TEST_ASSERT_EQUAL_STRING("1.00 KB", buf); + repl_format_numeric_resource_value(XCE_REPL_UNITS_RAW, "mem", 1024.0, buf, + sizeof(buf)); + TEST_ASSERT_EQUAL_STRING("1024.000000", buf); + repl_format_numeric_resource_value(XCE_REPL_UNITS_BOTH, "mem", 1024.0, + buf, sizeof(buf)); + TEST_ASSERT_EQUAL_STRING("1024.000000 (1.00 KB)", buf); + + /* swp: megabytes on the wire, converted to bytes before formatting. */ + repl_format_numeric_resource_value(XCE_REPL_UNITS_BOTH, "swp", 2.0, buf, + sizeof(buf)); + xce_format_units_value(XCE_UNITS_BOTH, 2.0 * 1024.0 * 1024.0, want, + sizeof(want)); + TEST_ASSERT_EQUAL_STRING(want, buf); +} + +static void test_shared_consumable_value_and_used_formats(void) +{ + char vbuf[128]; + char ubuf[128]; + + /* Non-bytes consumable keeps the pinned "%.2f"/"%.2f" shape. */ + repl_format_consumable_value_and_used(XCE_REPL_UNITS_BOTH, "slots", 4.0, + 1.0, vbuf, sizeof(vbuf), ubuf, + sizeof(ubuf)); + TEST_ASSERT_EQUAL_STRING("4.00", vbuf); + TEST_ASSERT_EQUAL_STRING("1.00", ubuf); + + /* Bytes-valued consumable formats both sides per the units mode. */ + repl_format_consumable_value_and_used(XCE_REPL_UNITS_HUMAN, "mem", + 2048.0, 1024.0, vbuf, sizeof(vbuf), + ubuf, sizeof(ubuf)); + TEST_ASSERT_EQUAL_STRING("2.00 KB", vbuf); + TEST_ASSERT_EQUAL_STRING("1.00 KB", ubuf); +} + +static void test_shared_current_units_mode_reads_state_or_defaults(void) +{ + xce_sched_sim_driver_t sim; + xce_sched_repl_state_t st; + + memset(&sim, 0, sizeof(sim)); + memset(&st, 0, sizeof(st)); + + /* No REPL state: falls back to the documented BOTH default. */ + TEST_ASSERT_EQUAL_INT(XCE_REPL_UNITS_BOTH, repl_current_units_mode(&sim)); + + st.units_mode = XCE_REPL_UNITS_RAW; + sim.repl_state = &st; + TEST_ASSERT_EQUAL_INT(XCE_REPL_UNITS_RAW, repl_current_units_mode(&sim)); +} + int main(void) { UNITY_BEGIN(); @@ -322,6 +417,10 @@ int main(void) RUN_TEST(test_format_bytes_human_fractional_values_render_two_decimals); RUN_TEST( test_swp_value_converted_mb_to_bytes_renders_on_same_adaptive_scale); + RUN_TEST(test_shared_translate_units_mode_maps_each_repl_mode); + RUN_TEST(test_shared_numeric_resource_value_formats_per_units_mode); + RUN_TEST(test_shared_consumable_value_and_used_formats); + RUN_TEST(test_shared_current_units_mode_reads_state_or_defaults); return UNITY_END(); }