From 0ce712548117b3dabe89c7cd3571ed297c90bc45 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Sun, 20 Sep 2026 10:00:12 +0000 Subject: [PATCH] fix(scheduler): percent-based limit display shows raw fraction, not percent (bug-1831) fmt_limit()/fmt_limit_units() formatted a percent-based limit's value as a bare snprintf(..., "%f%%", limit->val.val) -- echoing the raw stored value with a literal "%" appended. But xce_limit_val_t.val is genuinely a 0-1 fraction when percent is set (confirmed end-to-end: no /100 or *100 scaling exists anywhere in the config->capnp->runtime pipeline; confirmed authoritative by the original hand-written C literal this codebase's oldest percent-based limit was migrated from, .val = 0.2 for a documented "20% mem cap"). A correctly-configured 20% limit therefore displayed as "0.200000%" instead of "20.000000%" -- misleading, though technically an honest echo of the stored value. Fixed by multiplying by 100.0 before formatting. Display-only -- update_limit_res_value()'s actual capacity computation was already correct and is untouched. fmt_limit() (a thin XCE_UNITS_RAW wrapper) inherits this fix for its own percent-branch output too, a deliberate, intentional change from its pre-fix behavior for percent-based limits specifically (its 5 pre-existing direct tests only assert rc > 0, never the exact percent string, so this is safe). Updated test_limit.c's percent-mode test fixture from an unrealistic .val=80.0 to a realistic .val=0.8 (assertion text unchanged, since 0.8*100=80 either way) and test_repl_limit_table.c's own percent test to expect its shared fixture's arbitrary .val=50.0 constant correctly rendering as "5000.000000%". RED confirmed via the raw Unity testlog's :FAIL: lines (the aggregate meson summary masks single-test failures, a known harness quirk in this project); GREEN confirmed after, both lib and scheduler suites, 0 Unity FAILs. Discovered during live manual verification of scheduler-sim-diagnostics Requirement 6 against a real deployed cluster. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01RLSNJhNoULvVcUhXEFafQC --- .wolf/buglog.json | 50 +++++++++++++++++++ src/lib/testsuite/test_limit.c | 2 +- src/lib/xcelimit.c | 38 ++++++++++---- .../testsuite/test_repl_limit_table.c | 27 ++++++++-- 4 files changed, 103 insertions(+), 14 deletions(-) diff --git a/.wolf/buglog.json b/.wolf/buglog.json index f2680f2e..648f2911 100644 --- a/.wolf/buglog.json +++ b/.wolf/buglog.json @@ -24635,6 +24635,56 @@ ], "occurrences": 1, "last_seen": "2026-09-20T09:30:00Z" + }, + { + "id": "bug-1831", + "timestamp": "2026-09-20T10:00:00Z", + "error_message": "Live-verification finding (user's own retest of scheduler-sim-diagnostics Requirement 6): a correctly-configured 20% memory limit (limits2, resname: mem, value: 0.2, percent: true) displayed as 'mem = 0.200000%' in `info limit` -- reading as 0.2 percent to a human, not the 20 percent it actually computes and enforces.", + "file": "src/lib/xcelimit.c", + "root_cause": "xce_limit_val_t.val is genuinely a 0-1 fraction when percent != 0 (confirmed end-to-end: no /100 or *100 scaling exists anywhere in the config->capnp->runtime pipeline -- src/xceadmin/src/xcecommon/mapping.py, controller.capnp's LimitValue, its C encode/decode, build_limit(), and update_limit_res_value() (plugins/limit.c) all pass the value through unchanged, multiplying it DIRECTLY against the resource's current value for capacity computation -- confirmed authoritative by the original hand-written C literal this codebase's oldest percent-based limit was migrated from, `.val = 0.2` for a documented '20% mem cap', sched.c's limit2_config, commit ecf1916d). fmt_limit()/fmt_limit_units() (src/lib/xcelimit.c) formatted the percent branch as a bare `snprintf(..., \"%f%%\", limit->val.val)` -- echoing the raw 0-1 fraction with a literal '%' appended, instead of converting to conventional 0-100 percent notation first.", + "fix": "FIXED (2026-09-20). fmt_limit_units()'s percent branch now formats `limit->val.val * 100.0` instead of the raw `limit->val.val`, so a correctly-configured 0.2 (20%) limit displays as '20.000000%'. This is display-only -- update_limit_res_value()'s actual capacity computation was already correct and is untouched; only what a human reads changed. fmt_limit() (a thin XCE_UNITS_RAW wrapper around fmt_limit_units()) inherits this fix for its own percent-branch output too -- a deliberate, intentional behavior change from its pre-fix output for percent-based limits specifically (its 5 pre-existing direct unit tests only assert `rc > 0`, never the exact percent string, so this was safe). Updated test_limit.c's test_fmt_limit_units_percent_limit_ignores_units_mode fixture from an unrealistic `.val = 80.0` to a realistic `.val = 0.8` (80%) -- assertion text needed zero change since 0.8*100=80 either way. Updated test_repl_limit_table.c's test_info_limit_named_percent_limit_own_percentage_value_never_scaled_by_units_mode to expect the shared add_configured_percent_limit() fixture's (deliberately unchanged, .val=50.0) raw display as '5000.000000%' -- mathematically correct given that fixture's arbitrary test constant was never meant to represent a realistic percent value. RED confirmed by grepping the raw Unity testlog for :FAIL: (the aggregate meson summary masks this, a known Unity-harness quirk in this project) before the fix; GREEN confirmed after, both lib (0 Unity FAILs) and scheduler (85/85, 0 Unity FAILs) suites.", + "tags": [ + "scheduler", + "lib", + "limit-plugin", + "display-formatting", + "percent-limit", + "info-limit", + "requirement-6", + "fixed", + "discovered-during-live-retest", + "related-to-bug-1832" + ], + "related_bugs": [ + "bug-1832" + ], + "occurrences": 1, + "last_seen": "2026-09-20T10:00:00Z" + }, + { + "id": "bug-1832", + "timestamp": "2026-09-20T10:00:00Z", + "error_message": "src/configs/limits.yaml's night_mem_pct limit (resname: mem, value: 50, percent: true) is functionally dead -- it can never bind or deny any job, since its actual computed capacity is 50x (5000%) the resource's current value, not 50% of it. Surfaced while investigating bug-1831's percent-display discrepancy.", + "file": "src/configs/limits.yaml", + "root_cause": "xce_limit_val_t.val is a 0-1 fraction convention when percent is set (see bug-1831's root_cause for full pipeline trace -- no /100 scaling exists anywhere). night_mem_pct was authored directly in YAML for a docs/example config (commit a1478510, 'docs(admin-config-tool): commit spec docs and example configs') with no C-literal ancestry to cross-check against, unlike limits2 (migrated verbatim from a hand-written C literal `.val = 0.2` for '20% mem cap', sched.c's limit2_config) -- so it used the human-intuitive but incorrect 0-100 scale convention ('50' meaning 50%) instead of the pipeline's real 0-1 fraction convention.", + "fix": "FIXED (2026-09-20, user-directed). Changed limits.yaml's night_mem_pct value from 50 to 0.5, matching the 0-1 fraction convention -- it now correctly computes and enforces a genuine 50% cap on the night queue's mem usage instead of a non-binding 5000% one. This is a production config behavior change, not merely a display fix: night_mem_pct will now actually constrain job admission for the first time in any real deployment using this config, where previously it silently did nothing.", + "tags": [ + "scheduler", + "config", + "limit-plugin", + "percent-limit", + "night_mem_pct", + "config-authoring-bug", + "fixed", + "discovered-during-live-retest", + "related-to-bug-1831", + "behavior-change" + ], + "related_bugs": [ + "bug-1831" + ], + "occurrences": 1, + "last_seen": "2026-09-20T10:00:00Z" } ] } diff --git a/src/lib/testsuite/test_limit.c b/src/lib/testsuite/test_limit.c index 67786416..0134f78b 100644 --- a/src/lib/testsuite/test_limit.c +++ b/src/lib/testsuite/test_limit.c @@ -455,7 +455,7 @@ static void test_fmt_limit_units_percent_limit_ignores_units_mode(void) .hosts = NULL, .projects = NULL, .val - = &(xce_limit_val_t) { .resname = "mem", .percent = 1, .val = 80.0 } + = &(xce_limit_val_t) { .resname = "mem", .percent = 1, .val = 0.8 } }; char buf[1024]; xce_units_mode_t modes[] = { XCE_UNITS_RAW, XCE_UNITS_HUMAN, diff --git a/src/lib/xcelimit.c b/src/lib/xcelimit.c index baed0e88..5a1a968e 100644 --- a/src/lib/xcelimit.c +++ b/src/lib/xcelimit.c @@ -160,8 +160,21 @@ void kill_consumer(xce_consumer_t* c) * * The consumer-dump loop is IDENTICAL to fmt_limit()'s -- untouched. Only * the final configured-value line's formatting differs: - * - percent != 0: always the bare "%f%%" rendering, regardless of - * units_mode -- a percentage is never byte-scaled (Requirement 6 AC3). + * - percent != 0: always renders `val.val * 100` followed by "%", + * regardless of units_mode -- a percentage is never byte-scaled + * (Requirement 6 AC3). `xce_limit_val_t.val` is genuinely a 0-1 + * fraction when `percent != 0` (confirmed end to end against the real + * capacity computation, update_limit_res_value() in + * src/scheduler/src/plugins/limit.c, which multiplies this value + * DIRECTLY against the resource's current value with no /100 anywhere + * in the config→capnp→runtime pipeline -- and against the original + * hand-written C literal this codebase's oldest percent-based limit was + * migrated from, `.val = 0.2` for a documented "20% mem cap"). The *100 + * here is display-only, correcting a bug (fixed post-Requirement-6 + * shipping) where the raw 0-1 fraction was echoed directly with a "%" + * appended, so a correctly-configured 20% limit displayed as + * "0.200000%" instead of "20.000000%" -- misleading, though not + * incorrect in the sense of matching the stored value. * - percent == 0, resource is byte-valued (xce_is_bytes_valued_resource()) * or the "swp" special case (its wire unit is MB, not bytes -- the same * check repl_format_numeric_resource_value() already applies, converted @@ -172,13 +185,18 @@ void kill_consumer(xce_consumer_t* c) * AC3). * * fmt_limit() itself is a one-line wrapper around this function in - * XCE_UNITS_RAW mode. That is provably byte-identical to fmt_limit()'s own - * pre-existing output for every percent/non-percent, byte-valued/ - * non-byte-valued shape: xce_format_units_value(XCE_UNITS_RAW, v, ...) - * always renders the bare "%f" of v (see its own implementation), matching - * what the old inline fmt_limit() body produced. This is why - * test_limit.c's five pre-existing fmt_limit() call sites needed zero - * changes -- they are the regression proof. + * XCE_UNITS_RAW mode. For the percent == 0 branches (byte-valued and + * non-byte-valued absolute values), this remains provably byte-identical to + * fmt_limit()'s ORIGINAL pre-Requirement-6 output -- confirmed by + * test_limit.c's five original fmt_limit() call sites needing zero changes + * for those shapes. For the percent != 0 branch, this is now a DELIBERATE + * behavior change from fmt_limit()'s pre-Requirement-6 output (the *100 + * display fix above applies to fmt_limit() too, since it shares this same + * branch through the RAW-mode wrapper) -- none of those five original call + * sites assert on the exact percent-display string (they only check + * `rc > 0`), so this change is safe for them, but it is a real, intentional + * change to fmt_limit()'s own output for a percent-based limit, not a + * byte-identical refactor for that one case. */ int fmt_limit_units(char* buf, int len, xce_limit_t* limit, xce_units_mode_t units_mode) @@ -213,7 +231,7 @@ int fmt_limit_units(char* buf, int len, xce_limit_t* limit, if (limit->val.percent != 0) { rc = snprintf(buf + pos, len - pos, "\n%s = %f%%\n", - limit->val.resname, limit->val.val); + limit->val.resname, (double)limit->val.val * 100.0); } else { bool is_swp = (limit->val.resname != NULL) && (strcmp(limit->val.resname, "swp") == 0); diff --git a/src/scheduler/testsuite/test_repl_limit_table.c b/src/scheduler/testsuite/test_repl_limit_table.c index 682d46fa..591cfe17 100644 --- a/src/scheduler/testsuite/test_repl_limit_table.c +++ b/src/scheduler/testsuite/test_repl_limit_table.c @@ -3490,7 +3490,7 @@ test_info_limit_named_non_byte_valued_limit_display_completely_unaffected_by_uni * directly; this closes the same gap at the REPL dispatch level, through * the real `set units` command and repl_info_limit()'s real * translate_units_mode() bridge -- neither of which the library-level test - * exercises). The limit is percent-based (val.percent=1, 50%) on the + * exercises). The limit is percent-based (val.percent=1) on the * byte-valued resource "mem" -- deliberately so, since Requirement 6.3's * own wording singles out "a limit expressed as a percentage of a resource * whose own canonical unit is not bytes" as the case that must never be @@ -3500,6 +3500,26 @@ test_info_limit_named_non_byte_valued_limit_display_completely_unaffected_by_uni * task 9's own per-host byte-valued-percent-limit test already covers for * the DIFFERENT, RESOLVED-ABSOLUTE usage amount (Requirement 6.4) -- not * re-litigated here. + * + * Percent-display scale (bug fix, added after Requirement 6 shipped): + * `add_configured_percent_limit()`'s shared fixture stores `val.val = 50.0` + * as an arbitrary test constant -- but `xce_limit_val_t.val` is genuinely a + * 0-1 FRACTION when `percent != 0` (confirmed against the real capacity + * computation, `update_limit_res_value()`, plugins/limit.c, which multiplies + * this value DIRECTLY against the resource's current value with no /100 + * anywhere in the pipeline -- traced end to end from + * src/configs/limits.yaml through capnp to this function). `fmt_limit_units()` + * now correctly renders `val.val * 100` for the percent branch, so this + * fixture's raw 50.0 displays as the (visually odd but mathematically + * correct) "5000.000000%" -- not because 50.0 means 5000%, but because 50.0 + * was never a realistic percent-scale value to begin with (a real 50% limit + * needs `val.val = 0.5`). The shared fixture is deliberately left unchanged + * here rather than "corrected" to 0.5, since many OTHER tests in this file + * reuse it for their own accounted/capacity math (which depends on the + * capacity computation directly, not on this display fix, and remains + * correct either way) -- changing it would ripple into those tests for no + * benefit to this one, which only cares about verifying units_mode has zero + * effect on the percent line, not about the fixture's real-world realism. */ static void test_info_limit_named_percent_limit_own_percentage_value_never_scaled_by_units_mode( @@ -3530,9 +3550,10 @@ test_info_limit_named_percent_limit_own_percentage_value_never_scaled_by_units_m captured_raw = capture_dispatch(&sim, "info limit pct_never_scaled_limit"); TEST_ASSERT_NOT_NULL_MESSAGE( - strstr(captured_raw, "mem = 50.000000%"), + strstr(captured_raw, "mem = 5000.000000%"), "sanity: the configured-setting line must show the real percent " - "value under RAW mode"); + "value under RAW mode (val.val=50.0 * 100, see this test's own " + "header comment on the percent-display scale fix)"); handled = repl_dispatch(&sim, "set units human"); TEST_ASSERT_FALSE(handled);