[volen-silo]

Implements ROCMAI-62.

Four gaps in dashboard telemetry, in four commits that each build on their own:

1. TTFT/TPOT expire. The windowed average fell back to the histogram's lifetime average whenever a scrape saw no new requests, so an idle server kept showing its all-time latency forever. Only windows with new requests now produce a value; it is held (marked *, with the existing legend) for the observation validity window and then cleared. Reuses GenerationObservationTracker::observe_direct. Instance gains ttft_observation/tpot_observation (serde-default, omitted when unknown, so old recordings load and are never labelled fresh).
2. Shared-GPU power attribution. Every instance was credited with the full board power of each GPU it touches, so two models on one GPU each claimed all of it. attribute_power_w splits a GPU's power among its occupants by per-container VRAM, falling back to an equal split when any occupant has no reading. Power on a GPU nobody occupies is attributed to no instance but still counts toward node power. Instance.power_w carries the share and feeds tok/W and the POWER column. Also fixes the registry view ignoring gpu_indices: instances from rocm serve always had empty gpu_ids, so they could never show power or tok/W.
3. Assistant tool output. list_instances now reports ttft_ms/tpot_ms with their freshness and the attributed power_w, so the assistant cannot present a held TTFT as live.
4. Power limit and thresholds. GpuMetrics.power_limit_w (serde-default; None = unknown, never 0) is read from amd-smi static on the existing 30 s system-info cadence. Warning colors are 70% / 93% of the device's own limit (where the old 525/700 W constants sat on a 750 W part). With no known limit the value is drawn uncolored instead of being judged against another part's TDP.

Not covered / caveats

• No Gherkin scenario. This changes what the dashboard shows, but the dash scenarios need the Linux PTY lane; I could not run one. Coverage is at the daemon level instead: ttft_is_held_then_cleared_when_requests_stop and shared_gpu_power_is_split_and_limit_is_reported in crates/rocm-dash-daemon/tests/telemetry_contract.rs drive the real runner loop against a fake Prometheus endpoint and a fake amd-smi. I confirmed the TTFT test fails with the old lifetime-average fallback restored.
• amd-smi static key unverified. The cap is read from limit.socket_power, then limit.max_power, per my understanding of the schema, and has not been checked against a real MI-series host. Parsing is defensive; an unrecognised shape yields no limit (uncolored), not a wrong color.
• Windows has no limit source. ADLX exposes none for the iGPU I tried (gpu_power_range is unsupported), so power is uncolored there. Windows also has no instance registry, so items 1-3 do not apply there yet.
• WSL. amd-smi cannot run on WSL (no amdgpu module), so there is no GPU power or limit there, and vLLM's platform detection fails on amdsmi. Not addressed here.
• Depends on nothing, but overlaps feat(dash-tui): collect GPU/memory metrics in-process on Windows #715. Its winperf.rs and a few tests build GpuMetrics with explicit fields; whichever PR merges second needs power_limit_w: None, added to those literals. Draft until feat(dash-tui): collect GPU/memory metrics in-process on Windows #715 lands.
• e2e-cucumber's two shared_runtime symlink tests fail on a Windows account without symlink privilege (os error 1314), independent of this change.

Verification

• cargo test --workspace --all-targets (Windows): all pass except the two symlink tests above. cargo clippy --workspace --all-targets -- -D warnings and python scripts/smoke_local.py clean.
• Dash crates, Linux (WSL, including the unix-only daemon tests): 301 passed.
• Ran feat(dash-tui): collect GPU/memory metrics in-process on Windows #715's WinPerfCollector on a Windows host with a Radeon 8060S to confirm the new field flows through (power reported, limit None).

[jussielo-amd]

Thanks Volen, nice work. Honest expiry for TTFT/TPOT, per-GPU power attribution with a conservation test, and a limit-relative threshold with an unknown limit left uncolored instead of guessed are all the right calls. The daemon-level tests against a fake Prometheus and a fake amd-smi are a good substitute for the missing PTY scenario, and hosted CI is green. One blocker, and it comes from the gpu_indices fix itself:

Blocking

1. Every healthy managed rocm serve instance now raises a false "vram attribution" warning. registry.rs:175 now fills gpu_ids for managed services (before, they were empty and skipped). But per_container_used is keyed by the docker 64-hex id resolved from /proc/<pid>/cgroup (cgroup.rs), and a native service id never matches. So in runner.rs:992-1006, whenever amd-smi lists any GPU process (the serving vLLM itself), each managed instance counts as a fallback and the header shows "vram attribution: N instance(s) fell back to device-summed VRAM ... check /proc access" on every instance tick. Two other effects follow from the same cause:

   • The Observe VRAM column shows device-summed usage for these instances, so two services on one GPU each show the whole GPU.
   • The VRAM-weighted power split never applies to them (always the equal-split fallback).

   The new daemon test returns an empty process list, so procs_nonempty is false and none of this is exercised. Suggested fix: exclude instances that cannot match by design (non-docker ids) from the warning, or key them by PID ancestry, plus a test with a non-empty process list and a managed instance.

Non-blocking

• power_limits() runs a second amd-smi static --json on the same 30 s tick as system_info(), inline before metrics() (runner.rs:474-483). Parsing the caps from the static JSON system_info already fetches would avoid the extra stall (AGENTS §6). Also, Ok(empty) overwrites good caps, and nothing logs when the shape stops parsing, so "no coloured power" can't be told apart from "schema changed".
• parse_power_limits (amd_smi.rs:282-286): a numeric socket_power: 0 stops find_map before max_power, so a valid 750 W cap is dropped ("N/A" falls through, 0 doesn't). The bare-array branch and the N/A-then-max_power fall-through have no test.
• attribute_power_w gives starting/recovering instances (no VRAM reading yet) an equal-split share, so a serving co-tenant shows half its power and about 2x tok/W while a neighbour loads.
• UX: latency_cell appends * after {v:.0}ms, but the TTFT/TPOT columns are Length(7), so a held 10000ms* is clipped and loses the marker while the legend still shows. No test covers the held-TPOT legend branch.
• UX: the full per-GPU panel (gpu_stats_line, widgets.rs:176-179) still colours the W by temperature, so only the compact view gets the limit-relative colours.
• Assistant tool text: the tokens_per_watt description (agent/tools.rs:169) and the metrics.rs:210 doc still say board power, list_instances doesn't mention the new fields, and its power_w is an attributed share while gpu_status.power_w is board power. attributed_power_w would avoid confusing the two.
• Stale comments from removing avg_ms_from_histogram: bench_load.rs:546 (now false), mock_server.rs:381, and "None until two scrapes" in metrics.rs:220/231.

---

🤖 by agenthub on AMD AgentHub

[nowycondro]

🤖 Automated review (Claude Code) · c939e6b

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Review — needs work

Full review of the whole change (4 fan-out workers, file-coupling groups, plus direct verification of the highest-impact candidates). Implements the TTFT/TPOT-expiry, shared-GPU-power-split, assistant-surface, and power-limit-threshold portions of ROCMAI-62 ("rocm-cli: expand dashboard telemetry metric coverage"); the ticket's "node efficiency" gap item appears already solved on main outside this diff (widgets::node_efficiency is untouched by this PR).

Blocking

• Filling in gpu_ids for managed rocm serve instances makes the pre-existing VRAM-attribution warning misfire on every one of them — crates/rocm-dash-daemon/src/registry.rs:175, crates/rocm-dash-daemon/src/runner.rs:992-1007, crates/rocm-dash-collectors/src/cgroup.rs (container_id_for_pid).
  discovered_from_record now sets gpu_ids: record.gpu_indices.iter().map(u32::to_string).collect() (registry.rs:175) and container_id: record.service_id.clone(). The unchanged warning in runner.rs fires when !i.gpu_ids.is_empty() && gpu_ids_overlap(...) && !per_container_used.contains_key(&i.container_id), and per_container_used is built only from docker/containerd 64-hex ids extracted from /proc/<pid>/cgroup. A managed instance's container_id is its own service_id string, which can structurally never be a 64-hex docker id, so once amd-smi reports any GPU process at all (i.e. the instance's own server is actually running), every managed --gpu instance now permanently trips "vram attribution: N instance(s) fell back to device-summed VRAM … check /proc access" — a false positive, not a real /proc-access problem. Confirmed the new daemon test can't catch this: crates/rocm-dash-daemon/tests/telemetry_contract.rs:765's fake amd-smi process endpoint always returns {"gpu_data":[]} (the only process) fixture in the whole test suite), so procs_nonempty is always false and this path is never exercised.
  Confidence 90 · architectural · Fix: exclude instances whose container_id can't by construction join per_container_used (e.g. tag DiscoveredService/Instance with its discovery origin) from the fallback count, or key the warning on a signal that distinguishes "really missed a /proc join" from "this instance type was never joinable"; add a test where the fake amd-smi process returns a non-empty list that does (and doesn't) resolve to a managed instance.

• Two hardware E2E CI lanes are failing on the reviewed commit. E2E tests (Strix Halo, Ubuntu, nightly, current) and E2E tests (Strix Halo, Ubuntu, release, current) both report FAIL: 'serve-stop-leaves-no-background-helper' was expected to pass on this host but FAILED — a regression (checked live via get_check_runs, unchanged across two checks ~40 minutes apart). I could not locate this scenario's exact Gherkin source under that slug to confirm whether it's related to this diff — none of the 19 changed files touch apps/rocm's serve/stop paths, so it may be an environment-level flake rather than something this PR caused — but two self-hosted GPU lanes (E2E tests (W7900, current), E2E tests (MI300X, current)) are still queued and unrun, so CI is not all-green on this commit either way.
  Confidence 100 (read directly from the check run conclusions) · Fix: investigate serve-stop-leaves-no-background-helper on the Strix Halo Ubuntu lane; if it's a known pre-existing flake, say so in the PR text per AGENTS.md §12, otherwise fix it before merge.

Non-blocking

• Stale, now-false doc comment in the e2e harness — tests/e2e-cucumber/tests/e2e/dash_steps.rs:2477-2480. The comment above gen_tps_held_after_failure (line 2482) still says "The frame under assertion is provably post-failure (the TTFT cell it used to show is gone)" — the exact mechanism this PR's own commit (e266851) removed and replaced with warning_badge_count. It directly contradicts that function's own new doc two functions above, which correctly says TTFT is now deliberately held, not cleared. Confidence 90 · mechanical · Fix: update the comment to describe the badge-based proof.

• TTFT/TPOT table columns silently drop the held-marker * for any latency ≥ 10,000 ms — crates/rocm-dash-tui/src/ui/tabs/instances.rs:186-193 (Constraint::Length(7) for the ttft/tpot columns) + crates/rocm-dash-tui/src/ui/format.rs:270 (latency_cell renders "{v:.0}ms*"). At v≥10000, "10000ms" is already 7 characters, so the trailing * is clipped. The TOK/W column one over has an explicit comment showing this exact width budget was worked out there ("Wide enough for … so the held marker never gets clipped mid-unit"), but it wasn't redone when TTFT/TPOT gained marker capability in this PR. No test uses a 5-digit ms value. This re-introduces, for that range, exactly the "stale value looks fresh" failure mode this PR exists to fix elsewhere. Confidence 82 · logic · Fix: widen to Length(8), add a characterization test with a 5-digit value.

• Assistant-facing documentation is stale in two places after this PR added fields it doesn't mention — crates/rocm-dash-tui/src/agent/snapshot.rs:58-103 (list_instances_json's doc lists only the 4 original EAI-7960 fields — gen_tps, tokens_per_watt, freshness, observed_at — but the json body two lines below, per this PR's own commit 3ef4f4c, also returns power_w, ttft_ms, ttft_freshness, tpot_ms, tpot_freshness) and crates/rocm-dash-tui/src/agent/tools.rs:169 (TokensPerWattTool's description still says "divided by the summed power (W) of the GPUs each instance occupies" — but runner.rs:956-957 now divides by the attributed per-instance share from attribute_power_w, not the full summed board power of each occupied GPU). An LLM assistant reading either description would misdescribe exactly the semantic change (overcounting power on a shared GPU) this PR sets out to fix. Confidence 80 · mechanical · Fix: update both doc strings.

• The full/detailed GPU panel colors power by temperature, not by the new power-limit threshold — disagreeing with the compact view for the same value — crates/rocm-dash-tui/src/ui/widgets.rs:154-178 (gpu_stats_line, used by the full panel) vs crates/rocm-dash-tui/src/ui/tabs/hardware.rs:412 (gpu_compact_line, the only call site of power_style in the whole tree). A GPU at 95% of its power cap but cool would show green power in the full panel and red in the compact view for the identical reading. No test asserts a color for the full panel's power span. Confidence 80 · logic · Fix: use power_style(g.power_w, g.power_limit_w, theme) for the power span in gpu_stats_line too.

• parse_power_limits's documented fallback chain breaks on a valid zero — crates/rocm-dash-collectors/src/amd_smi.rs:269-287. The doc at line ~256 states the priority order is "the cap currently in force, then the maximum the part allows", and separately that "a missing, non-numeric, non-finite or non-positive cap (N/A, 0) is omitted" (implying a per-key fallback). But find_map over POWER_LIMIT_KEYS stops at the first key that parses to any finite number, including 0.0 — only unparseable values (e.g. "N/A") fall through to the next key. If socket_power is present and reads 0, max_power is never tried even if valid, and the final watts > 0.0 check then discards the whole entry. The existing test (power_limits_prefer_current_cap_and_skip_unreadable) only covers "first key absent/unreadable, second key valid" (gpu 2/3), never "first key present-and-zero, second key valid." Confidence 80 · logic · Fix: move the is_finite() && > 0.0 check inside the find_map closure, per key.

• GpuMetrics::power_limit_w has no serde-default/back-compat test, unlike every sibling additive Option field in the same file (crates/rocm-dash-core/src/metrics.rs:50; compare instance_deserializes_without_efficiency_fields at metrics.rs:248, which covers Instance's equivalent fields but not GpuMetrics's). Grepped the whole tree: nothing deserializes a legacy Snapshot/GpuMetrics JSON blob with a non-empty gpus array and no power_limit_w key (the one candidate, rocm-dash-tui/src/replay.rs's LEGACY_SNAPSHOT_LINE, uses "gpus":[]). Confidence 75 · test-gap · Fix: add a metrics.rs unit test mirroring the Instance one.

• power_limit_w — this PR's core new concept — is never exposed to the assistant/tool layer at all, even though this PR's own item 3 is specifically "the assistant cannot present a held TTFT as live" — crates/rocm-dash-tui/src/agent/snapshot.rs (gpu_json) and agent/tools.rs (gpu_status tool description) never reference it; it's used only in ui/tabs/hardware.rs and test fixtures. A user asking the assistant "is this GPU near its power limit" gets an answer the assistant can't compute even though the human UI now shows it in color. Confidence 72 · logic (completeness gap relative to this PR's own stated goal) · Fix: add power_limit_w to gpu_json/the gpu_status tool output.

• power_limits() adds a third sequential amd-smi subprocess call onto the already-serial system-info tick — crates/rocm-dash-daemon/src/runner.rs:473-483. It's gated on the same sysinfo_refresh_ticks (30s) boundary as system_info() and awaited serially before metrics(), worsening the documented pre-existing "tick polls its sources serially" gap (AGENTS.md §6) from ~2 to 3 sequential 10s-capped calls on that tick. Confidence 72 · architectural · Fix: tokio::join!(g.power_limits(), g.metrics()) instead of two sequential awaits.

• TPOT's held-marker rendering through the live instances table has no dedicated test, only TTFT's does — crates/rocm-dash-tui/src/ui/tabs/instances.rs:150 (format::latency_cell(inst.tpot_ms, inst.tpot_observation.as_ref())), test at instances.rs:1278 (table_held_ttft_shows_marker_and_legend_even_with_fresh_gen_tps) only exercises ttft_observation. A mutation swapping inst.tpot_observation for inst.ttft_observation at line 150 would pass every existing test. Confidence 72 · test-gap · Fix: add the TPOT-equivalent test.

• power_limits()'s underlying Command still lacks kill_on_drop(true) — crates/rocm-dash-collectors/src/amd_smi.rs:155-172 (run/run_json, wrapped in a 10s timeout). Pre-existing gap (already true for metrics()/processes()/system_info()), directly named by AGENTS.md §6 ("a tokio::process child awaited under a timeout sets kill_on_drop(true)"), now reached by a fourth recurring caller on a new 30s cadence. Confidence 70 · mechanical · Fix: add .kill_on_drop(true) once in run(), fixing it for all four callers.

Decisions for the author

• VRAM-weighted power split is a proxy for power share, not a measurement of it — tradeoff. attribute_power_w's VRAM-proportional split assumes VRAM footprint tracks power draw; a VRAM-heavy-but-idle co-tenant and a compute-bound-but-VRAM-light one on the same GPU will have their power swapped relative to reality. amd-smi doesn't expose true per-process power, so there may be no better signal available — confirm this is accepted as the best available proxy rather than an oversight.
• This PR bundles four logically separate features (TTFT/TPOT expiry, shared-GPU power split, assistant-surface reporting, power-limit-derived thresholds) into one 912-line/19-file change across 4 commits — non-blocking-improvement. Each commit is individually coherent, so this is not a defect, but splitting into a PR stack would have made independent review, bisection, and rollback easier.
• The amd-smi static schema (limit.socket_power/limit.max_power) is explicitly unverified against real MI-series hardware per the PR's own "Not covered" section — tradeoff. The design fails safe (uncolored power, never a wrong color) on a schema mismatch, which is the right call, but there's no signal distinguishing "no limit reported" from "parser never matched this amd-smi version's shape" — confirm you're comfortable discovering that distinction only by field testing.

Positive signals

• efficiency.rs's VRAM-weighted power-split suite (shared_gpu_splits_by_vram_and_conserves_board_power and siblings) withstood hand-traced mutations of the weighting math, the equal/weighted branch selection, and the accumulation logic without a miss — genuinely adversarial test design for the pure attribution core.
• power_limit_w: Option<f32> was deliberately chosen over a bare 0.0 sentinel specifically to avoid the "zero means unknown vs. a real zero" ambiguity the PR calls out in its own commit message — good type-design instinct, even though parse_power_limits doesn't yet fully honor it (see above).
• The PR description's "Not covered / caveats" section is unusually candid and specific (unverified amd-smi schema, Windows/WSL gaps, the #715 draft dependency, the symlink-privilege failure) — exactly the kind of disclosure AGENTS.md asks for instead of silently leaving gaps.

Deployment notes

• Every existing managed rocm serve --gpu instance will likely start showing a new "vram attribution: N instance(s) fell back…" warning after this ships (see Blocking #1) — operators should not read it as a new /proc-permission regression.
• Hosts where amd-smi's static --json differs from the assumed schema will show uncolored GPU power rather than a crash or a wrong color, by design — this is unverified against real MI-series hardware per the PR's own text.
• GpuMetrics.power_limit_w is additive/serde-default; old NDJSON recordings replay fine and simply show uncolored power (by design).

What this covered

Full diff read completely across four independent blind workers, grouped by coupling: (1) collectors+core power logic — amd_smi.rs, winperf.rs, efficiency.rs, metrics.rs, traits.rs; (2) daemon orchestration — registry.rs, runner.rs, demo.rs, telemetry_contract.rs (plus full reads of unchanged cgroup.rs/vram.rs for join correctness); (3) TUI presentation — agent/snapshot.rs, ui/format.rs, ui/tabs/{hardware,home,instances,observe}.rs, ui/widgets.rs, dash_characterization.rs, dash_journeys.rs; (4) tests/e2e-cucumber/tests/e2e/dash_steps.rs. Each worker also grepped the whole tree for callers/consumers of changed symbols outside its own group. I then independently re-verified every Blocking finding and most Non-blocking ones directly against the source myself (not taking any worker's or the existing human review's word for it) before scoring. Reviewed against merge-base 8479e6a…head c939e6b8.

Agent-instruction adherence checked against the repo's root AGENTS.md (the only such file in the tree). History (git log/git log -p) and code-comment accuracy were checked per-file by each worker as part of its pass; nothing anomalous in history beyond the four ROCMAI-62 commits themselves.

Did not run: cargo test/cargo clippy — no Rust toolchain was available to me or to any of the four workers; every test verdict above is from static/hand-tracing of the code, flagged explicitly where it matters. A "prior review comments on these files" pass was not performed — the available GitHub code/issue search tools in this environment errored on a token-scope problem; I did not substitute guessing. I read only the existence and CHANGES_REQUESTED state of the standing jussielo-amd review, never its specific claims, and re-derived every finding above independently from the source. Ticket ROCMAI-62 was retrieved successfully via the project's Jira tool. This report is not reconciled against that review thread or any other discussion on this PR — it is a blind pass, per the reviewing method's rules.

🤖 Generated with Claude Code

[volen-silo]

Thanks both. Pushed fixes (head 40d5c52a):

Blocker (false "vram attribution" warning for managed services): confirmed, and caused by the gpu_indices change. The warning now counts only instances whose id has the docker/containerd shape the /proc/<pid>/cgroup join can produce (cgroup::is_container_id, count_vram_join_misses). The shared-GPU integration test now returns a non-empty amd-smi process list; with the filter removed it fails with the reported warning, and it passes with the filter. Known limitation, unchanged: managed services still show device-summed VRAM and the equal-split power fallback; per-process matching by PID ancestry is a separate change.

Also fixed: 0 current cap falling through to max_power (+ bare-array and N/A tests); power_limit_w exposed in gpu_status; assistant power_w renamed attributed_power_w with updated tool descriptions; latency columns widened so held 12345ms* keeps its marker (+ held-TPOT test); full GPU panel colors power by the limit; GpuMetrics serde back-compat test; stale comments; cap fetch joined with metrics(), and empty results no longer overwrite held caps and log once. kill_on_drop landed upstream in #842 while this was in review.

Not changed: equal-split share for starting instances, since excluding them would overstate a serving neighbour, so neither choice is accurate.

CI: the two Strix Halo Ubuntu lanes were red on serve-stop-leaves-no-background-helper. This PR doesn't touch serve/stop, and those lanes also fail on recent main push runs; I have not been able to confirm the cause.

Fixed

[jussielo-amd]

Thanks @volen-silo, looks good. The false "vram attribution" warning is fixed with a test that returns a non-empty amd-smi process list (and goes red without is_container_id), the zero-cap fall-through, attributed_power_w rename, power_limit_w in gpu_status, widened latency columns and full-panel power colouring all landed, and CI is green on this head (29 checks). Nice work.

Nothing blocking. Some things worth a look, none required for merge:

• Held marker flapping. As I read it, snapshots are built every 1s but latency is observed every 2s, so under steady traffic TTFT/TPOT may alternate 150ms and 150ms* (and toggle the legend row, shifting the table height). Defining Fresh as now - observed_at <= instance_tick, or fixing the docs that say Held means "no new requests", would settle it.
• Managed services show device-summed VRAM. Two services on one GPU each show the whole GPU's used VRAM next to their split power. Marking it as estimated would avoid surprise.
• Test depth on the runner. The failed-scrape path (rebaseline_latency calls at runner.rs:871-872) is only covered through the helper, and ttft_is_held_then_cleared_when_requests_stop asserts > 0 rather than 50/20 ms exactly, so swapping TTFT/TPOT or dropping those calls would stay green. A MockMode::Failure latency assertion and exact values would close it. Also pin gpu_indices through the real producer in the cross-crate serde test.
• amd-smi static --json still runs once more per 30s refresh next to system_info(); deriving the caps from that same JSON would drop the extra round trip. Err(_) => {} on that path logs nothing.
• UX: the POWER column is now a share under an unchanged header, and power colour is relative to a cap the TUI never shows (colour-only), so a short note or the cap in the title would make the 70%/93% meaning discoverable.
• Housekeeping: the description says "Draft until #715 lands" but the PR isn't a draft; and it doesn't mention the rewritten dash-08/09 steps or that populating gpu_ids now drives the VRAM fallback. A line on why no Gherkin scenario (or which lane would run one) would help for AGENTS §3.

---

🤖 by agenthub on AMD AgentHub