Rongsong Shen
487e7b9419
info limit was correctly implemented and tested but inert against a real replay: register_limit()'s only call site was normal-mode sched_register_ plugins(); sim mode's own plugin bootstrap (sim_bootstrap_picker()) never registered the limit plugin. The snapshot format already carries the original session's limit config (sim->core.config, a borrowed reference set by xce_sched_core_load_snapshot()) -- nothing needed to change in the recording/replay format. New sim_bootstrap_limit_plugin() (sim_limit.c, mirroring sim_picker.c's structure) registers the plugin via plugin_register_core() and seeds it via a new limit_seed_config(plugin, xce_sched_core_t* core, config) entry point, wired into sim_loop.c alongside the existing sim_bootstrap_picker() call. limit_new_plugin() extracted from register_limit() as a factory, mirroring builtin_fcfs_new_plugin()'s established pattern; register_limit() itself is behavior-preserving. First attempt set plugin->sched to a disguised xce_sched_sim_driver_t*, relying on it sharing xce_scheduler_t's first-member layout to reuse the existing config_hook path -- rejected on review: this repeats the exact cross-cast pattern that already corrupted memory once in this codebase (see core.h's xce_core_cache_clear_cb_t history). Fixed by making limit_seed_config() take xce_sched_core_t* as an honest parameter instead, with limit_update_config() (normal mode) and the new sim path both delegating to a shared limit_seed_config_impl() -- no cast, no aliasing risk, independently verified by two review rounds reading the actual struct layouts and call chains. Also added sim_limit.c to sched_sim_isolation_lib so Req 5 AC 17's controller-I/O-isolation check covers it (an initial gap, closed in the same remediation round). Two ripple waves of the same established link-stub pattern applied to the ~24 white-box test files that #include sim_loop.c. info limit now shows real configured-limit data against a genuine replay. Live usage will still correctly show "no active usage instance yet" until a separate, larger fix (logged as bug-1826) wires limit *matching* into the sim decision path -- deliberately out of this task's scope. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RLSNJhNoULvVcUhXEFafQC
80 lines
15 KiB
Markdown
80 lines
15 KiB
Markdown
# Implementation Plan: scheduler-sim-diagnostics (Requirement Set 1 — Limit Inspection)
|
||
|
||
- [x] 1. Add read-only accessors exposing the limit plugin's configured-limits and live-usage-instances tables
|
||
- Two new functions (`limit_get_configured_limits()`, `limit_get_usage_instances()`) returning raw kvmgr handles (or `NULL` when the plugin is absent), declared in `plugin.h` alongside `register_limit()`, defined in `plugins/limit.c`
|
||
- Never allocate, mutate, or trigger plugin hooks — a pure read of already-existing pointers
|
||
- New/extended unit tests in `test_limit_plugin.c` cover: plugin registered with configured limits (handle returned, iterable), plugin registered with zero limits, plugin not registered (`NULL`)
|
||
- Observable: a test that registers the limit plugin with a known limit and confirms the accessor's returned handle, when iterated via `iter_make()`/`iter_next()`, yields that same limit; a sibling test confirms `NULL` when the plugin was never registered
|
||
- _Requirements: 1.1, 1.2, 1.4, 2.1, 3.1_
|
||
- _Boundary: Limit Plugin Accessor_
|
||
|
||
- [x] 6. Bootstrap the limit plugin in sim mode so `info limit` shows real data against a replay (fixes `bug-1825`)
|
||
- Extract `limit_new_plugin()` factory out of `register_limit()` (`plugins/limit.c`), mirroring `builtin_fcfs_new_plugin()`/`register_builtin_fcfs()`'s existing factory-extraction pattern exactly; `register_limit()` itself must be behaviorally unchanged (pure refactor)
|
||
- New `sim_bootstrap_limit_plugin(xce_sched_sim_driver_t* sim)` (new file `sim_limit.c`, mirroring `sim_picker.c`'s structure/header-comment convention): `limit_new_plugin()` → `plugin_register_core(&(sim->core), plugin)` → if `sim->core.config != NULL`, call `plugin->hooks.config_hook(plugin, sim->core.config)` directly (NOT the type-unsafe `plugin_config_update()`, which casts its `void*` arg to `xce_scheduler_t*` and would misread `xce_sched_sim_driver_t`'s embedded `core` field)
|
||
- Wire the call into `sim_loop.c` right alongside the existing `sim_bootstrap_picker(sim)` call site — by that point `sim->core.config` already holds the snapshot's borrowed-reference config (set during `sched_sim_driver_start()`'s snapshot load, which runs first)
|
||
- Observable: a real `xce_sched_sim_driver_t` driven through `sched_sim_driver_start()` against a replay whose snapshot carries a real `xce_limit_config_t`, after the sim driver's own bootstrap sequence runs, has `limit_get_configured_limits()` return a handle that iterates to that real limit — no test-only registration helper needed
|
||
- _Requirements: 1.1–1.7 (makes them exercisable against a real replay, not just via a test-only fixture helper)_
|
||
- _Boundary: Limit Plugin Accessor (factory extraction), new Sim Limit Bootstrap component_
|
||
- **Known deliberate non-goal, tracked separately**: this task does NOT wire limit *matching* (`limit_job_election()`) into the sim/replay decision path — that function is only ever reached via `plugin_job_election()`, which is normal-mode-only (one caller, `scheduler.c`) and itself needs a core-safe variant before it could be called from `sim_loop.c`. Live usage (Requirements 2.x/3.x) will still show "no active usage instance yet" against a real replay even after this task — only the configured-setting display (1.1-1.7) becomes real. Tracked as a separate follow-up bug, not bundled into this task.
|
||
|
||
- [x] 2. Core: Limit lookup and configured-setting display
|
||
- [x] 2.1 Implement name/all resolution with correct absence messaging
|
||
- New white-box test file `test_repl_limit_table.c` created (sibling of `test_repl_resource_table.c`, own `#include` set covering `plugins/limit.c` and `resource.c` for real — not stubbed — per `design.md`'s build-wiring risk note citing bug-1762's precedent)
|
||
- `repl_info_limit(sim, arg)`: `arg == "all"` iterates `limit_get_configured_limits()`; `NULL`/empty prints "no limits configured" (Requirement 1.7); `arg != "all"` does a `key_find()` by name (not a scan) on the same handle, printing "no such limit '<name>'" (Requirement 1.3) for a `NULL` handle or a miss — a `NULL` handle on this path is never treated as "no limits configured," matching that message's own `all`-only scoping
|
||
- Wired as a direct call for now (`repl_dispatch()`/`repl_cmd_info()` wiring itself deferred to task 4); tests in this task call `repl_info_limit()` directly, not through `repl_dispatch()`
|
||
- Observable: REPL output shows "no such limit 'foo'" for an unconfigured name against a fixture with other limits configured, and "no limits configured" for `all` against a fixture with zero configured limits
|
||
- _Requirements: 1.3, 1.7, 4.1, 4.2_
|
||
- _Depends: 1_
|
||
|
||
- [x] 2.2 Display configured setting including scope membership
|
||
- Matched limit's name, constrained resource, value/percent, and per-dimension scope print via the existing `fmt_limit()` library formatter into a stack buffer, line-prefixed with the existing `[sim] ` convention; all output via `fprintf(stdout, ...)`, never bare `printf()` (`test_no_bare_printf.c` regression-gates this)
|
||
- Inclusion and exclusion group membership both render (`fmt_group()` already handles this, adopted as-is)
|
||
- Observable: a limit configured with an exclusion group ("all except X") shows both the inclusion and the exclusion in its output; `info limit all` against a fixture with multiple configured limits displays every one's setting
|
||
- _Requirements: 1.1, 1.2, 1.5_
|
||
|
||
- [x] 2.3 Display effective host-scope for implicitly host-scoped limits
|
||
- After `fmt_limit()`'s output, call `is_host_based_limit(core->resmgr, limit)`; if it returns `> 0` AND the limit's own per-host consumer is not configured, print an explanatory host-scope note
|
||
- Observable: two REPL fixtures — one percent-based limit on a per-host resource without explicit per-host scoping (note appears), one absolute-value or explicitly-global-scoped limit (note does not appear)
|
||
- _Requirements: 1.6_
|
||
|
||
- [x] 3. Core: Live accounted-usage display
|
||
- [x] 3.1 Display usage for cluster-wide (non-host-scoped) limit instances
|
||
- Iterate `limit_get_usage_instances()`, filter by `vres->limit == matched_limit`; for a GLOBAL/SHARED `vres`, read capacity from `core.sharedInst[idx]` and accounted usage from `core.sharedInUse[idx]`
|
||
- Existence-of-instance and zero-usage are distinct: zero matching `xce_vres_t` (no capacity entry at all) prints "no active usage instance yet" (Requirement 2.2); a matched `xce_vres_t` whose `sharedInUse[idx]` is `NULL`/absent instead displays a genuine `accounted=0` (it has been evaluated and has a real capacity, just nothing charged against it yet) — existence is gated on the capacity read, never on the in-use read
|
||
- Observable: a global limit that has matched at least one job shows an accounted/capacity pair reflecting current state; a configured-but-never-evaluated limit shows the explicit no-instance message instead; a matched-but-never-charged limit shows `accounted=0` rather than the no-instance message
|
||
- _Requirements: 1.4, 2.1, 2.2, 2.3, 3.3_
|
||
- _Depends: 1_
|
||
|
||
- [x] 3.2 Display per-host usage for host-scoped limit instances
|
||
- For a host-scoped `vres` (explicit or implicit per 2.3), iterate `core.workers`; include a worker only if it has a populated capacity entry (`w->resvals[idx]` non-`NULL`) — read capacity from `w->resvals[idx]` and accounted usage from `w->resInUse[idx]` (same existence-vs-zero-usage distinction as 3.1, applied per worker); label each instance with its scope values (user/queue/project from the vresname, plus host)
|
||
- A limit varying by more than one scope dimension at once (e.g. per-user and per-host together) shows each distinct combination as its own labeled instance
|
||
- Observable: a limit matched against jobs on two distinct hosts shows two separate per-host usage lines each labeled with its own scope values, and a third configured-but-untouched host shows no line at all
|
||
- _Requirements: 1.4, 2.1, 2.2, 2.3, 3.1, 3.2, 3.4_
|
||
|
||
- [x] 4. Register `info limit` as a recognized `info` subcommand
|
||
- New `else if (strcmp(tokens[1], "limit") == 0)` branch in `repl_cmd_info()` (`repl_info.c`) routes to `repl_info_limit()`; usage-string literal updated to list `limit` alongside the existing subcommands
|
||
- `test_repl_limit_table.c` gains a full dispatch-round-trip test (via `repl_dispatch()`, not a direct call) for both `info limit <name>` and `info limit all`
|
||
- Observable: `info limit <name>` and `info limit all` both work when typed as real REPL input (through `repl_dispatch()`), and `help`/usage output lists the new subcommand
|
||
- _Requirements: 4.1, 4.2_
|
||
- _Depends: 2.1, 3.1, 3.2_
|
||
|
||
- [x] 5. Verify no mutation, replay determinism, and existing-subcommand regression
|
||
- Test: displaying limit information never alters configuration, accounting state, or scheduling decisions (assert config/accounting untouched before vs. after)
|
||
- Test: a replay run twice — once with `info limit` issued mid-session, once without — produces identical scheduling decisions and identical final accounted state
|
||
- Full existing `info` subcommand test suite (job/queue/worker/resource/summary) re-run to confirm zero output changes from this addition
|
||
- Observable: the two new determinism/no-mutation tests pass, and the full pre-existing info-command test suite shows zero diffs
|
||
- _Requirements: 5.1, 5.2, 5.3_
|
||
- _Depends: 4_
|
||
|
||
## Parallel Execution Note
|
||
|
||
No `(P)` markers are used anywhere in this plan. Tasks 2.1–2.3 and 3.1–3.2 all incrementally edit the same function (`repl_info_limit()`) in the same file (`repl_info.c`) and largely share the same new test file (`test_repl_limit_table.c`) — the parallel-analysis rules' "no conflicting files or shared mutable resources" criterion rules out marking any of them concurrent. Task 1 is the sole Foundation task (rarely `(P)` by rule, and has no peer to run alongside regardless). Tasks 4 and 5 are Integration/Validation work that structurally depends on Core being complete.
|
||
|
||
## Implementation Notes
|
||
|
||
- **Task 1**: `test_limit_plugin.c`'s pre-existing `fixture_init()` never populated `core->plugins` (`plugin_register()` is stubbed to a no-op in that white-box TU). Closed with two new test-local fixture helpers that hand-build a real `core->plugins` kvmgr and `key_add()` the plugin under its real `"xce_sched_limit"` name — necessary to exercise the accessors' real `key_find()` path, not a workaround. `test_repl_limit_table.c` (tasks 2.1+) is a *different* white-box TU with its own include set (per design.md's build-wiring risk note) — it should link real `plugin.c`/`core.c` machinery where `register_limit()`/`xce_sched_core_init()` actually populate `core->plugins`, so it likely does NOT need this same hand-built-kvmgr workaround; confirm this when starting 2.1 rather than assuming the same gap exists there.
|
||
- **Task 2.1**: Making `repl_info.c` genuinely call `limit_get_configured_limits()` broke the link for ~24 pre-existing white-box test files that `#include "../src/repl_info.c"` without also linking `plugins/limit.c` — this codebase's white-box test convention means any new cross-file call from an included production file ripples to every file that includes the caller but not the callee. Fixed with an identical 5-line link-satisfying stub (`return NULL`) added to each, matching the established "duplicate, don't share" stub idiom already used throughout this file family; verified behavior-preserving since none of those 24 fixtures register the limit plugin (the real function would also return `NULL` there). **Task 3 (which calls `limit_get_usage_instances()`) will very likely hit the identical ripple across an overlapping or identical file set — expect it, and apply the same fix pattern rather than treating it as a surprise.**
|
||
- **Task 3.1**: found and fixed a real latent bounds-check imprecision (negative/stale `idx` not safely rejected by a `.count`-based pre-check) while writing the new GLOBAL/SHARED usage-read code — logged as `bug-1824` in `.wolf/buglog.json` rather than fixed in the pre-existing `repl_info_resource_one()` (out of this task's boundary); the new code uses the corrected pattern instead. Task 3.2 confirmed it did NOT repeat the same mistake in its own per-host reads.
|
||
- **Task 3.2**: reviewer approved with one non-blocking suggestion — no test directly exercises Requirement 3.1's "per-user and per-host together" combined-dimension case end-to-end (structurally supported by the code, per independent review inspection, just not covered by a dedicated fixture). Worth adding if this area gets touched again; not required before shipping.
|
||
- **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.
|