[juhovainio]

Implements ROCMAI-564

Windows has no rocmdashd socket yet, so the TUI's #[cfg(windows)] client arm builds Snapshots in-process on a 1-second ticker instead of connecting anywhere: HostCollector (already cross-platform) for CPU/memory, and a new WinPerfCollector for GPU utilization/VRAM/temperature/power/clock. The unix arm is unchanged.

• crates/rocm-dash-collectors/src/winperf.rs: new WinPerfCollector, reading the GPU Engine/GPU Adapter Memory OS performance counters via powershell.exe (the same WDDM/DXGK-backed source Task Manager's GPU tab uses — no AMD tool needed for utilization/VRAM), plus a best-effort adapter inventory (name/RAM/driver) for GpuSystemInfo. Temperature, power, and clock speed additionally use ADLX (AMD Device Library eXtra, via the adlx crate) when the AMD driver's bundled amdadlx64.dll is present, degrading to 0.0/0.0/None on any failure (no driver, non-AMD GPU, unsupported metric) so the snapshot never errors out.
• crates/rocm-dash-collectors/Cargo.toml: adds adlx and windows-sys (Win32_System_LibraryLoader, for SetDefaultDllDirectories — see below) as Windows-only dependencies.
• crates/rocm-dash-tui/src/client.rs: the #[cfg(windows)] connect_and_run drives HostCollector/WinPerfCollector and sends Connected like the unix arm, but then a stream of ClientMsg::HostGpuSnapshot rather than Event::Snapshot. This is deliberate, not an oversight: Event::Snapshot treats instances as authoritative and latches has_received_snapshot forever, but Windows has no instance-registry daemon, so its instances is always an empty stand-in — routing it through there would permanently blind the issue [Feature]: Stop serving a model #145 "quit while serving" confirmation prompt. See AppState::push_host_gpu_snapshot.
• crates/rocm-dash-tui/src/ui/tabs/home.rs: fixes the hero VRAM percentage silently inflating (e.g. "80800.0%") when every GPU's vram_total_mb is 0 — guards on total > 0 like widgets.rs/dock.rs already do, instead of total.max(1). Also replaces the hero's old "Unknown GPU"/fake 0%/0°C/0W readings with "Detecting GPU…"/"—" placeholders while telemetry is genuinely pending (gpu_telemetry_pending) versus confirmed absent (gpu_data_unavailable) — this applies on every platform (not just Windows) and in --replay, since the daemon's own Linux snapshot can legitimately report zero GPUs or a disconnect/replay-end can wipe latest.
• docs/manual-testing.md, docs/rocm-docs/install/installation.md: document the Windows dashboard's new live telemetry and the quit-while-serving prompt's Windows-specific behavior (no model name/Services-overlay hint there, since the dashboard never gets a serving-instance snapshot on Windows).
• crates/rocm-dash-tui/Cargo.toml: adds the rocm-dash-collectors path dependency this requires.
• xtask/src/crate_edges.rs: allowlists the new rocm-dash-tui -> rocm-dash-collectors first-party edge.

Several bugs surfaced by real-hardware testing (this machine, AMD Radeon 780M integrated GPU) and fixed in follow-up commits:

• GPU Engine perf-counter instances are keyed per-process (pid_NNNN_luid_...) while GPU Adapter Memory instances are per-adapter (luid_..., no pid); the device-keying logic now strips the pid prefix so both aggregate onto the same real adapter instead of each process producing its own phantom near-zero device.
• Windows can also surface multiple stale/phantom LUIDs for one physical adapter (observed: 3 LUIDs, 2 permanently zero-VRAM, for a single AMD Radeon 780M). When Win32_VideoController confirms there's exactly one real adapter, all LUIDs now collapse into one device (GPUs · 1 instead of GPUs · 3).
• powershell.exe child-process spawn latency (not the Get-Counter query itself) was blowing the old 5s timeout on real hardware; bumped to 15s and collapsed the two independent metrics()/system_info() spawns into one snapshot() call so each tick only pays the spawn cost once.
• WMI's Win32_VideoController.AdapterRAM is a 32-bit field that truncates (or reports 0) above ~4GiB; a WMI_ADAPTER_RAM_CAP_THRESHOLD heuristic now detects that and prefers ADLX's own total-VRAM query instead, decoupled from whether the perf-counter query itself succeeded, falling back to WMI when ADLX is unavailable.
• Multi-engine GPUs were under-reporting utilization (max-across-engines was overwriting instead of accumulating); aggregation is now keyed per (device, engine instance): samples for the same engine instance are summed (per-process shares of that engine), and the max is taken across distinct engines, matching Task Manager.
• SetDefaultDllDirectories's return value is now checked — ADLX's amdadlx64.dll load falls back to the default search order on failure rather than silently assuming the hardened search path took effect.
• When WMI finds an adapter but the perf counters return nothing (locale mismatch between Get-Counter's English counter names and a non-English Windows locale is the known cause), a new warning surfaces through snapshot()'s Vec<String> return instead of silently showing zeros.

vram_total_mb is only filled in for the single-adapter case (the realistic integrated-APU target); multi-GPU hosts degrade to 0 rather than risk a wrong number, since correlating a perf-counter LUID to a specific Win32_VideoController/ADLX entry isn't reliable without vendor-specific APIs beyond index-based ordering.

Datapoints and sources

Datapoint	Source	Notes
GPU utilization %	GPU Engine perf counter (Get-Counter via powershell.exe)	sum per engine instance, max across engines, per adapter
VRAM used	GPU Adapter Memory perf counter (dedicated usage)	per-adapter
VRAM total	ADLX total-VRAM query, falling back to Win32_VideoController.AdapterRAM (WMI)	ADLX preferred (WMI's field is 32-bit and truncates/zeroes above ~4GiB); single-adapter hosts only, 0 otherwise
GPU count / identity	Win32_VideoController (WMI)	used to collapse phantom perf-counter LUIDs onto the real adapter(s)
Temperature	ADLX PerformanceMonitoringServices::current_gpu_metrics().temperature()	AMD GPUs only, via the driver's bundled amdadlx64.dll; 0.0 if unavailable
Power draw	ADLX ...::current_gpu_metrics().power()	same as above
Clock speed	ADLX ...::current_gpu_metrics().clock_speed()	same as above; None if unavailable
CPU / memory	HostCollector (sysinfo, cross-platform)	unchanged, not part of this PR's GPU work

Also updated README.md's platform-support table and live-dashboard description (and docs/rocm-docs/install/installation.md, see above) to reflect that Windows now gets GPU/CPU/memory telemetry (just not the serving-instance registry, which still needs the unix-socket daemon), and added a Linux e2e scenario (dash-37 in tests/e2e-cucumber/features/dash.feature) covering the no-GPU-telemetry case. The Windows collector path itself isn't covered by this repo's e2e harness — not for lack of a GPU Windows runner (the required "E2E tests (Strix Halo, Windows)" lane has one), but because the e2e-cucumber PTY driver (tui_driver.rs, portable-pty) is Linux-only for now: it compiles on Windows via ConPTY, but that path isn't yet promoted to a blocking contract, per the top-of-file comment in dash.feature.

Test plan

• Build and run on real hardware (this machine, AMD Radeon 780M integrated GPU); confirmed the dashboard TUI shows live GPU utilization/VRAM/CPU/memory metrics without a rocmdashd daemon running, and iterated on the perf-counter keying/timeout/phantom-LUID/VRAM-total fixes above against live Get-Counter/ADLX output.
• Verify ADLX temperature/power/clock readings against real hardware (this machine, AMD Radeon 780M; confirmed amdadlx64.dll present in the driver store and live temp/power readings in the TUI).
• cargo test (workspace, Linux) — unit tests pass; no regressions.
• cargo clippy --workspace --all-targets — clean.
• cargo check/cargo clippy --target x86_64-pc-windows-gnu (cross-compiled) — validates the ADLX FFI code path compiles against the real adlx crate.

Follow-up

WinPerfCollector currently shells out to powershell.exe (Get-Counter/Get-CimInstance) per tick; process-spawn latency is why the timeout above is 15s. A native rewrite — wmi crate for the Win32_VideoController inventory query, raw Pdh* FFI for the GPU Engine/GPU Adapter Memory counters — would cut per-tick latency and drop the spawn-timeout handling entirely, at the cost of more unsafe/platform-specific code (mainly Pdh* buffer-size negotiation). Not done here since the current approach already works and is tested on real hardware; worth revisiting if tick latency becomes a problem in practice.

[rominf]

🔴 Automated review · pr-review-watcher · 9bb9005

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.
Implements ROCMAI-564's live GPU/memory metrics on Windows using a client-side collector. It does not cover the ticket's "Validate on Strix Halo" item, and the description's ADLX hardware check is still unticked.

All findings below are new (first round on this PR).

Blocking

• On Windows, quitting with a model still serving no longer warns — crates/rocm-dash-tui/src/client.rs:211-219, crates/rocm-dash-tui/src/app/mod.rs:693, crates/rocm-dash-tui/src/app/mod.rs:1479

  • The new Windows arm sends a Snapshot every second with instances: Vec::new().
  • push_snapshot then sets has_received_snapshot = true and clears instances.
  • From that point has_live_instance_with stops consulting the live registry read (|| (!self.has_received_snapshot && snapshot.has_live_service)).
  • So about a second after rocm dash starts on Windows, pressing q with a Lemonade model serving exits without the prompt.
  • The issue #145 fallback was documented as authoritative "for the entire session" on Windows precisely because no snapshot ever arrived there. See the doc comment at app/mod.rs:651-657, apps/rocm/src/dash.rs:340-347 and docs/manual-testing.md:530-548.
  • The Serving view is also forced empty every tick.
  • Nothing tests the new arm, so nothing caught this.
  • Confidence 85 · logic · Fix: keep the registry fallback authoritative while the in-process source cannot enumerate instances (e.g. a snapshot or message flag that does not flip has_received_snapshot). Add a test that applies a Windows-style snapshot and asserts has_live_instance() stays true when the registry reports a live service.

• VRAM total is capped near 4 GiB, so VRAM% can pass 100% on the target hardware — crates/rocm-dash-collectors/src/winperf.rs:45, crates/rocm-dash-collectors/src/winperf.rs:163

  • Win32_VideoController.AdapterRAM is a documented uint32, so vram_total_mb can never exceed about 4095.
  • The "Dedicated Usage" counter feeding vram_used_mb has no such cap.
  • A Strix Halo carve-out or any dGPU above 4 GiB therefore shows the wrong total and an inflated percentage.
  • The test fixtures use 17179869184 (16 GiB), a value this field cannot hold, so they hide the problem.
  • Confidence 85 · logic · Fix: take the total from ADLX (Gpu::total_vram) or the adapter's HardwareInformation.qwMemorySize registry value. Add a fixture at the 4 GiB cap.

• ADLX temperature, power and clock are matched to devices by list position — crates/rocm-dash-collectors/src/winperf.rs:175, crates/rocm-dash-collectors/src/winperf.rs:221-229

  • adlx.get(i) pairs the i-th ADLX GPU with the i-th perf-counter LUID in first-seen order.
  • The ADLX list holds AMD GPUs only. The LUID list also holds non-AMD adapters, the Basic Render Driver and phantom LUIDs.
  • filter_map(... .ok()) drops a GPU whose metrics call fails, which shifts every later reading up one slot.
  • The result is AMD readings shown against the wrong device. That is the outcome the same file avoids for VRAM ("degrade to 0 instead of a wrong number"), and the doc comment at line 200 admits the two lists are not correlated.
  • Confidence 85 · logic · Fix: attach ADLX readings only when there is exactly one device and exactly one ADLX GPU, otherwise leave 0/None; or correlate by LUID or PCI id. Keep one slot per GPU (map, not filter_map).

• The ADLX matching has no test, and its two helper tests pass with it removed — crates/rocm-dash-collectors/src/winperf.rs:151, crates/rocm-dash-collectors/src/winperf.rs:175, tests at crates/rocm-dash-collectors/src/winperf.rs:338-356

  • parse_metrics calls adlx_readings() internally, which is always empty off Windows. No test can supply readings, so neither adlx.first() nor adlx.get(i) is ever exercised.
  • Mutating apply_adlx(adlx.first()) to apply_adlx(None), or adlx.get(i) to get(0), left all 8 tests green (run).
  • It also makes the "pure parse" tests call real driver FFI on any Windows host that has the AMD driver.
  • Confidence 100 · logic · Fix: pass &[AdlxReading] into parse_metrics and test the matching, including multi-device cases.

• ADLX is initialised and torn down every second, synchronously, on an async worker — crates/rocm-dash-collectors/src/winperf.rs:151, crates/rocm-dash-collectors/src/winperf.rs:213-231

  • snapshot() (async) runs parse_metrics → AdlxHelper::new() on every 1s tick. Each call loads amdadlx64.dll, initialises ADLX, lists GPUs, then terminates.
  • The crate itself marks this // TODO: This should be a singleton (adlx-0.0.0-alpha.1 src/helper.rs:14).
  • That blocking FFI runs on a tokio worker of the dashboard's multi-thread runtime.
  • Confidence 70 · architectural · Fix: create the helper once inside WinPerfCollector and query it via spawn_blocking. This combines with the injection fix in the previous finding.

• Timed-out powershell.exe children are leaked — crates/rocm-dash-collectors/src/winperf.rs:66-74

  • timeout(RUN_TIMEOUT, Command::new(...).output()) is used without .kill_on_drop(true), and tokio's default is false.
  • When the 15s timeout fires, the future is dropped and the child keeps running.
  • The file's own comment says timeouts happened "consistently ... every tick" on real hardware, and the loop respawns immediately, so hung children can pile up.
  • Confidence 85 · logic · Fix: .kill_on_drop(true).

• amdadlx64.dll is loaded by bare name every second (DLL search-order hijack) — crates/rocm-dash-collectors/src/winperf.rs:217

  • adlx calls libloading::Library::new("amdadlx64.dll"), which uses the default Windows search order, including PATH and the current directory.
  • On a host without the AMD driver (no copy in System32), a planted DLL in the directory rocm dash runs from would be loaded and executed.
  • Confidence 70 · architectural (security) · Fix: call ADLX only if %SystemRoot%\System32\amdadlx64.dll exists, and pre-load it with LOAD_LIBRARY_SEARCH_SYSTEM32 (keeping the handle alive) so the crate's load by name reuses that module.

• Docs and comments still say Windows has no live dashboard (AGENTS.md §5) — README.md:34, README.md:816, docs/rocm-docs/install/installation.md:18, docs/manual-testing.md:530-548, crates/rocm-dash-tui/src/client.rs:9-10, crates/rocm-dash-tui/src/app/mod.rs:651-657, apps/rocm/src/dash.rs:340-347

  • These say, respectively: "no live dashboard"; "Live mode requires Unix domain sockets (Linux and WSL only)"; live telemetry requires Linux or WSL2; the dashboard's daemon client "never connects (Windows, for the whole session)"; "the connection is a #[cfg(unix)] stub"; and, in the last two, that Windows never receives a snapshot.
  • The code now does the opposite in each case.
  • Confidence 85 · mechanical · Fix: update them in this PR, worded to match however the first finding is resolved.

• No scenario or stated justification for the new Windows dashboard behaviour (AGENTS.md §3) — tests/e2e-cucumber/features/dash.feature

  • rocm dash on Windows now shows live metrics instead of a disconnected view: user-observable behaviour.
  • Every dash scenario is @requires-os:linux, and no test of any kind runs the new connect_and_run Windows arm.
  • Confidence 85 · mechanical · Fix: add a scenario, or say in the PR text why there is none and which lane would run it. At minimum add a unit test of the snapshot assembly; the Windows lane runs tests.

• The new home.rs test does not cover the sparkline fix, and its 0.0% check is vacuous — crates/rocm-dash-tui/src/ui/tabs/home.rs:374-382, crates/rocm-dash-tui/src/ui/tabs/home.rs:692-715

  • Verdict 1 (fix fully reverted): the test fails, but only on !contains("80800"). contains("0.0%") is satisfied by other text on the screen.
  • Verdict 2 (one branch broken alone): reverting only the history-sparkline hunk to .max(1) leaves the test green (run).
  • Confidence 100 · logic · Fix: assert the hero tile's VRAM value and the sparkline input directly.

• Two winperf tests don't test what their names claim — crates/rocm-dash-collectors/src/winperf.rs:296-308, crates/rocm-dash-collectors/src/winperf.rs:329-333

  • Verdicts for the tests not already covered above:
    • multiple_processes_on_one_adapter_collapse_to_one_device still passes with the pid-prefix strip in device_key removed (run); the single-adapter collapse rescues it. Only multiple_adapters_leave_vram_total_zero catches that mutation.
    • empty_output_yields_no_metrics still passes with the order.is_empty() guard removed (run).
    • Summing utilization instead of taking the max, and taking the max of VRAM instead of the sum, both survive (run).
  • The remaining tests fail on revert of the logic they target.
  • Confidence 100 · logic · Fix: give the pid-collapse test two adapters so only device_key can merge the entries. Add a fixture with GPU lines but no counter lines, and fixtures where the max-vs-sum choice changes the result.

Non-blocking

• CPU/memory readings stall behind the PowerShell call — crates/rocm-dash-tui/src/client.rs:205-215

  • host.tick() waits on gpu.snapshot() (up to 15s).
  • The ticker keeps the default MissedTickBehavior::Burst, though the daemon it claims to mirror sets Skip (crates/rocm-dash-daemon/src/runner.rs:300).
  • On slow-spawn hosts this means a powershell.exe running almost all the time, which skews the CPU figure being displayed.
  • Confidence 70 · logic · Fix: collect GPU metrics on their own task and use Skip/Delay.

• English counter paths fail silently on localised Windows — crates/rocm-dash-collectors/src/winperf.rs:41-44

  • Windows PowerShell 5.1's Get-Counter expects localised counter names. With SilentlyContinue, a non-English install yields an empty GPU list with no warning.
  • Confidence 70 · logic · Fix: warn when inventory lines are present but no counter lines are.

• Virtual or non-AMD display adapters break the single-adapter path — crates/rocm-dash-collectors/src/winperf.rs:45, crates/rocm-dash-collectors/src/winperf.rs:154, crates/rocm-dash-collectors/src/winperf.rs:264

  • Any extra Win32_VideoController entry (a remote or virtual display driver, a second GPU) switches to the multi-adapter path, which brings back the phantom LUIDs.
  • gpu_model is taken from whichever adapter is listed first.
  • Confidence 70 · logic · Fix: filter adapters to PCI devices before counting.

• connect is silently ignored on Windows — crates/rocm-dash-tui/src/client.rs:188-200

  • The configured value is only logged, while the header keeps showing the configured unix-socket address.
  • Confidence 70 · logic · Fix: label the source as in-process, or warn when a non-default connect is set.

• Stray marker and a wrong API name in a comment — crates/rocm-dash-collectors/src/winperf.rs:113-116

  • There is a leftover "ponytail:" marker.
  • The comment names MSPower_DeviceEnable (a WMI power-management class) as the LUID-correlation path; DXGI is the relevant one.
  • Confidence 85 · mechanical · Fix: reword.

Decisions for the author

• In-process client-side collection vs a Windows daemon transport — tradeoff

  • In-process gets live metrics shipped without IPC work.
  • The cost is a second, divergent snapshot source with no instances. The first blocking finding is the first consequence of that, and the ticket's research left the transport question open.

• Spawning PowerShell every second vs calling PDH natively — non-blocking-improvement

  • Querying the same counters through PDH (PdhAddEnglishCounter) in-process would remove the spawn latency, the 15s timeout, the leaked children and the localisation issue in one change.

• Depending on adlx 0.0.0-alpha.1 — tradeoff

  • It is cheap access to ADLX.
  • But the crate's own Drop eprintln!s over the TUI when terminate fails, its GPU iterator unwrap()s, and it marks its own helper as a TODO singleton. The comment at winperf.rs:211 ("fails cleanly (Err, not a panic)") is therefore only partly true.

Positive signals

• The fixtures are real-world captures (the 780M three-LUID case, per-process pid_ instances), which pin the actual Windows counter shapes rather than idealised ones.
• snapshot() folds metrics and system info into one spawn instead of two per tick.

Deployment notes

• New third-party dependencies, Windows-target only: adlx 0.0.0-alpha.1 (MIT) and libloading 0.8.9 (ISC). Both licences are accepted in about.toml, and the manifest and notices are regenerated.
• A new first-party edge, rocm-dash-tui -> rocm-dash-collectors, is allowlisted; it is unconditional, not Windows-only. cargo xtask check-crate-edges exits 0.

What this covered

• What was read: every file in the change, completely, against 32834b2946ee…9bb9005e60ec. Also read: callers of connect_and_run up to apps/rocm/src/dash.rs; AppState's snapshot and quit-guard logic; sibling collectors; every vram_total_mb consumer in the TUI (the other divisions are already guarded, so no unfixed twin exists); the adlx crate source.
• Two independent workers split the code passes: collector plus dependencies, and TUI wiring plus the instruction, history and comment passes.
• Ran:
  • cargo test -p rocm-dash-collectors winperf (8 tests) and cargo test -p rocm-dash-tui hero_vram_pct (1 test), each confirmed to have run, plus the mutations cited above.
  • A Windows-target cargo check/clippy of rocm-dash-collectors, shown to be live with an injected type error.
  • cargo xtask check-crate-edges.
• CI at the reviewed head is all green, including windows-build-and-test.
• Did not run:
  • A Windows-target check of rocm-dash-tui (a C build script needs mingw, which is absent). The new arm was type-checked verbatim in a scratch build inside the review checkout instead.
  • Any execution on Windows or AMD hardware.
  • The prior-changes pass: earlier review comments on these files were out of reach under this run's constraints.
• Not reconciled against any discussion on the PR.

[juhovainio]

Verified the ADLX temp/power/clock integration against real hardware (commit 9bb9005e, this machine, AMD Radeon 780M integrated GPU). Confirmed amdadlx64.dll is present via the driver's bundled copy (C:\Windows\System32\amdadlx64.dll and 4 driver-store copies), and the dashboard TUI shows live, sane values with no rocmdashd running:

• GPUs · 1 (phantom-LUID collapse working, not inflated to 3)
• VRAM 86.7% (sane percentage, not the old 80800.0%-style garbage)
• TEMP 66.0°C
• POWER 28.0 W

All four of this PR's fixes (pid-prefix stripping, LUID collapse, VRAM% guard, ADLX wiring) confirmed working end to end on physical hardware.

[juhovainio]

Resolved all outstanding findings, re-verified:

Blocking:

1. ADLX-to-device matching no longer trusts index-based correlation on multi-adapter hosts — temp/power/clock now only trusted when exactly one real adapter + one ADLX reading exist (same guard vram_total_mb already used); multi-adapter hosts degrade every slot to defaults instead of risking misattribution.
2. Strengthened the matching tests: temp/power/clock assertions added to the existing VRAM-fallback tests, plus a new test proving multi-adapter + non-empty ADLX readings still degrade to defaults.
3. adlx_readings() now runs inside tokio::task::spawn_blocking — FFI/driver calls no longer block a tokio worker thread. Full singleton caching across ticks is a reasonable follow-up, not done here (re-init cost is one AdlxHelper::new() per 1s tick; not shown to matter in practice).
4. run_script's Command now sets .kill_on_drop(true) — a dropped child on timeout() firing actually kills the orphaned powershell.exe instead of leaking it.
5. Mitigated the amdadlx64.dll DLL search-order hijack (CWE-427): SetDefaultDllDirectories(LOAD_LIBRARY_SEARCH_SYSTEM32) called once process-wide (via windows-sys, already a workspace dependency) before the first AdlxHelper::new(), restricting the vendored adlx crate's unflagged libloading load to System32 only.
6. Updated all stale "Windows has no live dashboard" docs/comments (README, installation.md, manual-testing.md, client.rs, app/mod.rs, dash.rs) to reflect that Windows now streams live GPU/CPU/memory, but the serving-instance registry view stays empty by design (no rocmdashd unix socket there).
7. Windows e2e coverage: not added in this PR. The e2e-cucumber dash feature is explicitly Linux-only pending a ConPTY/Windows-PTY driver path in tui_driver.rs that isn't yet promoted to a blocking contract — adding a real Windows scenario is PTY-infrastructure work, not step-definition reuse, and risks the same timing flakiness already seen on this runner (see prior quit-confirm flakes). Flagging as a follow-up rather than rushing fragile coverage into this PR.
8. Replaced the vacuous home.rs VRAM test (rendered the whole UI, string-matched output, never populated state.history) with a direct unit test of the extracted vram_used_pct helper. Strengthened winperf.rs tests: isolated pid-prefix stripping from the single-adapter collapse rescue, added an order.is_empty() guard test, added a sum-vs-max VRAM fixture with two distinct non-zero values, and fixed SAMPLE's AdapterRAM to a realistic (32-bit-representable) value.
9. Non-blocking cleanups: ticker now sets MissedTickBehavior::Skip (matches the daemon's existing choice); fixed the stray comment's wrong API name; connect param's log-only use left as-is (documents intent, not worth restructuring). Locale-dependent Get-Counter silent failures and non-AMD/virtual-adapter edge cases remain known, lower-priority follow-ups.

Re-verified: cargo test -p rocm-dash-collectors -p rocm-dash-tui, cargo clippy --workspace --all-targets -- -D warnings, and cargo check/clippy --target x86_64-pc-windows-gnu (Windows-cfg-gated code doesn't compile-check otherwise) — all clean.

Dismissing: all 9 blocking + 5 non-blocking findings from this review are resolved and verified as of commit 87610a3 (see PR comment). Re-requesting review on current HEAD.

[rominf]

🔴 Automated review · pr-review-watcher · fdea558

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.
Implements ROCMAI-564 ("Device metrics for TUI (GPU/memory) on Windows"). It covers the GPU/memory source and its wiring into the dashboard. The ticket also asks for validation on Strix Halo / Windows HIP-SDK hosts. The PR reports testing only on a Radeon 780M, and this run's Strix Halo Windows lane failed before it ran any scenario.

Round 2. Each finding is marked new (first raised this round) or standing (raised at 9bb9005 and still holding). How the earlier round's objections were settled is under Standing objections from the previous round, at the end.

Blocking

• [new] The hero card shows "Detecting GPU…" / "—" forever, on Linux as well as Windows — crates/rocm-dash-tui/src/ui/tabs/home.rs:76-78, :139-141, :289-296, :394-410

  • gpu_telemetry_pending is latest.is_none_or(|s| s.gpus.is_empty()), so it cannot tell "nothing has arrived yet" from "a snapshot arrived with no GPUs".
  • The Linux daemon sends gpus: Vec::new() on every tick once gpu_init_done is set and amd-smi is unavailable. It sends the same when a metrics call fails (crates/rocm-dash-daemon/src/runner.rs:467-505), alongside an "amd-smi unavailable" warning. So the hero shows a loading state that never ends, and it contradicts the warning badge. Before this change it read "Unknown GPU".
  • A model already known from gpu_system_info is also hidden.
  • On Windows, a collector error or empty counter output (gpu_inventory_without_counter_samples_yields_no_metrics) gives the same permanent "Detecting…".
  • Confidence 85 · logic · Fix: treat only latest.is_none() as pending. Once any snapshot has arrived with no GPUs, show the known model, "Unknown GPU", or an "unavailable" state.

• [new] Windows GPU collection failures never reach the screen — crates/rocm-dash-tui/src/client.rs:224-236

  • On error the arm only calls warn! (to the log), then sends warnings: Vec::new() and gpu_system_info: Some(GpuSystemInfo::default()). That drops the adapter inventory as well.
  • The daemon is the other implementation of this tick, and the Linux dashboard reads warnings from it. It pushes "amd-smi metric: {e}" into snapshot.warnings (runner.rs:472), which feeds the header badge and the Hardware warnings box (ui/tabs/hardware.rs:230).
  • On Windows the user sees "no GPUs" / "Detecting GPU…" with no reason given. That includes the 15 s PowerShell timeout the code itself reports hitting on real hardware.
  • Confidence 85 · logic · Fix: push the error into snapshot.warnings, as the daemon does.

• [standing, raised as non-blocking at 9bb9005] GPU counters are queried by English counter names, which silently return nothing on non-English Windows — crates/rocm-dash-collectors/src/winperf.rs:41-45

  • Get-Counter '\GPU Engine(*)\Utilization Percentage' and '\GPU Adapter Memory(*)\Dedicated Usage' must use localized counter paths on a localized OS. The script runs under $ErrorActionPreference = 'SilentlyContinue' and -ErrorAction SilentlyContinue, so the error is swallowed.
  • The test gpu_inventory_without_counter_samples_yields_no_metrics names this case ("a locale where Get-Counter's -ErrorAction silently swallows a localized-path mismatch"). The test only checks that no phantom device appears.
  • Combined with the two findings above, the result is an empty GPU panel with no warning. Meanwhile README:34 says the Windows dashboard "shows GPU/CPU/memory".
  • Argued from the code and known Get-Counter behaviour; not run on a localized host.
  • Confidence 70 · logic · Fix: use PdhAddEnglishCounterW. windows-sys 0.61 is already a dependency and has the Win32_System_Performance feature. Alternatively resolve counter indices, and at minimum surface the empty result as a warning.

• [new] GPU utilization under-reports when more than one process uses the GPU — crates/rocm-dash-collectors/src/winperf.rs:97-100, :152-158, :110; test :386-402

  • GPU Engine instances are one per process per engine. device_key drops both the pid and the engine, and the UTIL arm then takes the max over every instance.
  • Task Manager instead sums processes per engine, then takes the max across engines. The doc comment at :110 claims this code produces "the same 'overall GPU' reading Task Manager shows". The comment is the right target and the code is what disagrees with it.
  • Two processes at 40% on the 3D engine therefore show 40%, not 80%.
  • multiple_processes_on_one_adapter_collapse_to_one_device asserts 0.682, the max of 0.682 and 0.514, so the test locks the under-count in.
  • Confidence 85 · logic · Fix: key UTIL by (LUID, engine), sum across pids, take the max over engines, and update that test's expected value.

• [standing, narrowed] The README still says the live dashboard needs Linux/WSL, and the published docs now contradict themselves — README.md:41, README.md:205

  • Line 41: "Live dashboard telemetry requires Linux or WSL2". This is the same sentence the PR rewrote in docs/rocm-docs/install/installation.md:18.
  • Line 205: "Live mode reads telemetry over a Unix domain socket, so it requires Linux or WSL".
  • Line 205 is pulled into docs/rocm-docs/getting-started.md via {include} (start-after "## Interactive interfaces"). The new installation.md sentence links to exactly that section.
  • Both lines contradict README:34 and :816-818 as edited by this PR. AGENTS.md §5 requires these claims to be kept in sync in the same change.
  • Confidence 100 (grep) · mechanical · Fix: update both lines to match line 34.

• [new, with standing winperf test gaps] No test covers the two new behaviours that matter — crates/rocm-dash-tui/src/app/mod.rs:1504-1511; home.rs label branches at :292-296, :394-410

  • A mutation run made push_host_gpu_snapshot set has_received_snapshot = true and clear instances. That is the issue #145 quit-confirm regression its own doc comment warns about.
  • The same run forced every hero pending label condition to false.
  • All 930 rocm-dash-tui tests still passed.
  • Test verdicts:
    • The new home tests cover only the helpers (gpu_telemetry_pending, vram_used_pct, node_throughput_title). They would fail if the change were reverted, but a single broken label branch or a broken snapshot routing survives.
    • The new winperf tests are in a new file, so a revert removes them too. A single-branch break that survives:
      • either .min(100.0) clamp (no fixture is above 100);
      • the total_vram_mb: None → WMI fallback at :190 (no fixture has a single reading with None);
      • device_key on an instance with no luid_.
  • Confidence 100 · logic · Fix: add an AppState test that push_host_gpu_snapshot leaves instances and has_received_snapshot untouched, a render test for the "—" labels, and fixtures for the three winperf branches.

• [standing] No e2e scenario covers the user-visible changes, and the PR text does not explain why — tests/e2e-cucumber/features/dash.feature

  • The change alters what users see in two places:
    • rocm dash live mode on Windows now runs instead of erroring;
    • Linux/WSL now show the "Detecting GPU…" and "—" placeholders.
  • All 36 dash scenarios are @requires-os:linux / WSL.
  • AGENTS.md §3 requires a scenario. Where one can only run on a gated lane, or cannot run (the feature header says Windows ConPTY is not promoted), the PR text must say so and name the lane. The description does neither.
  • The Linux placeholder state can be exercised on the existing PTY lanes.
  • Confidence 85 · mechanical · Fix: add a Linux scenario for the hero's pending/unavailable state, and state the Windows gap and its lane in the PR text.

• [new] The description and a doc comment say the Windows arm sends Event::Snapshot, and the description omits several changes — PR body; crates/rocm-dash-tui/src/client.rs:193-198

  • The PR body says the Windows arm "sends the same ClientMsg sequence (Connected then Event::Snapshot)… so the app loop doesn't need to know which platform it's on". The doc comment on connect_and_run says the same.
  • The code sends the new ClientMsg::HostGpuSnapshot precisely so the app loop treats it differently. It also adds handling in event_loop.rs:1044 and AppState::push_host_gpu_snapshot. The variant's own doc at client.rs:39-47 explains why.
  • The description also does not mention:
    • the loading-placeholder change (commit fdea558), which changes Linux behaviour;
    • that VRAM total now prefers ADLX's figure, while the datapoint table still says the source is Win32_VideoController.AdapterRAM only;
    • the process-wide SetDefaultDllDirectories change and the windows-sys dependency;
    • the README, manual-testing and installation doc edits.
  • Confidence 100 · mechanical · Fix: correct the description and the client.rs:196-198 comment to match the code (the code governs here).

• [standing — not re-found by this round's review; settled by putting it to the reviewer, see below] VRAM total is still capped near 4 GiB on the WMI fallback path — crates/rocm-dash-collectors/src/winperf.rs:186-191, :159-160, :180; crates/rocm-dash-tui/src/ui/tabs/home.rs:37-46

  • Fixed part: with exactly one adapter and one ADLX reading carrying Some(total_vram), the ADLX total is now used.
  • Still holding: otherwise (no AMD driver/ADLX, current_gpu_metrics failing, total_vram_mb: None) the total falls back to AdapterRAM, a uint32 that caps at about 4095 MiB, while vram_used_mb sums the uncapped Dedicated Usage counter and vram_used_pct has no clamp. A single-adapter card above 4 GiB without a usable ADLX total can still show the wrong total and VRAM% above 100%.
  • The ADLX total is read only inside the Ok arm of current_gpu_metrics, so a metrics-query failure also discards a good TotalVRAM.
  • The "overflow" fixture at :446-448 uses 4294967296, a value the uint32 field cannot emit (a ≥4 GiB card reports about 0xFFF00000), and no fixture covers the None fallback.
  • Fix (unchanged from the earlier round): take the fallback total from the adapter's HardwareInformation.qwMemorySize registry value (or clamp/hide the percentage when the total is the capped WMI value), decouple the TotalVRAM read from the metrics call, and add a fixture at the real cap.

Non-blocking

• [standing] CPU/memory refresh waits on the GPU query, and the collector inflates the CPU figure it reports — client.rs:221-232

  • host.tick() runs only after gpu.snapshot().await returns, so CPU/memory update every 5–15 s on the hardware described at winperf.rs:29-35.
  • Starting a new powershell.exe each tick lands inside the very CPU window sysinfo measures.
  • Confidence 85 · logic · Fix: tick host metrics on their own timer, or sample them before the GPU query.

• [new] The "—" placeholder fix stops short of other values that look real but are not

  • Multi-adapter Windows hosts have vram_total_mb == 0, so the hero prints "VRAM 0%" next to non-zero used VRAM (home.rs:384, :396-400).
  • Without ADLX, TEMP and POWER print 0°C and 0W, which look like real readings.
  • Confidence 85 · logic · Fix: show "—" in these cases too.

• [standing] Any extra display adapter disables the single-GPU path — winperf.rs:168, :334-342

  • Win32_VideoController also lists RDP, Basic Display and virtual display adapters, and the iGPU on hybrid laptops.
  • With any of these present, the code takes the multi-adapter branch: ADLX readings are dropped, VRAM total becomes 0, and phantom zero-VRAM LUIDs reappear as extra GPUs — the symptom the single-adapter collapse was written to fix.
  • gpu_model comes from the first adapter, which may be one of these.
  • Confidence 70 · logic · Fix: filter adapters by PNPDeviceID or AdapterCompatibility.

• [new] Multi-GPU device ids can swap between ticks — winperf.rs:136-150, :210-222

  • win-gpu-{i} follows first-seen order, and UTIL lines (which exist only for processes currently holding a GPU context) come first.
  • Adapter order can therefore change between ticks, mixing per-device history.
  • Confidence 70 · logic · Fix: iterate keys in BTreeMap order.

• [standing, was blocking — now non-blocking] ADLX is initialized and torn down every second — winperf.rs:280-304

  • AdlxHelper::new(), and the DLL load and unload with it, runs on every tick (now under spawn_blocking).
  • The adlx crate itself has // TODO: This should be a singleton.
  • Confidence 70 · architectural · Fix: keep one helper alive on a dedicated blocking thread.

• [standing, was blocking — now non-blocking] The DLL search-path hardening applies to the whole process — winperf.rs:272-278

  • SetDefaultDllDirectories(LOAD_LIBRARY_SEARCH_SYSTEM32) affects every later LoadLibrary in rocm.exe, and also drops the application directory from the search.
  • Its return value is ignored.
  • The comment calls adlx a "vendored crate"; it is a crates.io dependency.
  • Confidence 70 · architectural · Fix: document the process-wide effect where the call is made, check the return value, and fix the wording.

• [new] The adlx crate can panic or print to stderr mid-TUI

  • GpuIterator::next calls .unwrap(), and AdlxHelper::drop calls eprintln!.
  • A panic inside spawn_blocking is absorbed by unwrap_or_default, but the default panic hook still prints over the terminal.
  • Confidence 70 · logic.

• [new; ponytail: marker standing] Stale or stray comments

  • winperf.rs:225-229 and :255-262 justify the design by "index-based correlation", which parse_metrics no longer does.
  • winperf.rs:124 begins with a stray ponytail: marker.
  • crates/rocm-dash-tui/src/registry_probe.rs:18 still says the Windows client "never connects at all". Its twin comments in dash.rs and app/mod.rs were updated in this PR; this one was missed.
  • crates/rocm-dash-tui/Cargo.toml:19 calls the dependency "Windows-only", but it is unconditional.
  • Confidence 85 · mechanical.

• [new] The "overflow" test fixture uses a value WMI cannot return — winperf.rs:446-448

  • AdapterRAM is a uint32, so 4294967296 can never appear; a card with ≥4 GiB reports roughly 0xFFF00000.
  • multiple_adapters_leave_vram_total_zero is a strict subset of multiple_adapters_ignore_adlx_vram_and_leave_total_zero.
  • Confidence 85 · mechanical.

• [new] push_host_gpu_snapshot copies the history/latest/clamp body of push_snapshot — app/mod.rs:1504-1511

  • Two copies of the same logic will drift apart. A shared helper would prevent it.
  • Confidence 85 · mechanical.

Decisions for the author

• PowerShell spawned once per tick vs native PDH — tradeoff

  • The PR lists native PDH as a follow-up and says the current approach "already works".
  • In favour of moving now: the locale failure above, a refresh rate that is really the spawn time (1–15 s), and collection cost counted in the dashboard's own CPU reading. PDH bindings are already reachable through windows-sys.
  • In favour of deferring: less unsafe code, and PDH buffer-size negotiation.
  • Either way, the locale finding should be fixed or surfaced in this PR.

• A second, thinner producer inside the TUI vs running the daemon's run_loop in-process on Windows — non-blocking-improvement

  • rocm_dash_daemon::runner::run_loop is not unix-gated, and apps/rocm already depends on the daemon crate.
  • A Windows maybe_spawn_embedded_daemon could run it in-process and forward its Events, with a pluggable GPU source.
  • That would give, for free: instances, Lemonade gen-tps scrapes, the bench ring, snapshot warnings, and the pending-vs-unavailable GPU state. It would also make the HostGpuSnapshot special case and the new rocm-dash-tui → rocm-dash-collectors edge unnecessary.
  • What the current approach leaves empty or misleading on Windows: the Instances, Observe and Bench tabs, and the chat agent's instance list. Lemonade is the Windows engine.
  • The current approach is honest about its scope (README:34, manual-testing) and is legal under crate_edges.rs.

Positive signals

• Keeping the Windows tick out of Event::Snapshot (ClientMsg::HostGpuSnapshot) preserves the #145 quit-confirm semantics on a host with no instance registry.
• kill_on_drop(true) under the timeout, so a hung powershell.exe cannot leak.
• ADLX readings are trusted only when there is exactly one real adapter and exactly one reading. multiple_adapters_never_trust_adlx_even_when_reading_count_matches_adapter_count pins that positional coincidence is not trusted.
• vram_used_pct fixes the old .max(1) bug, which turned a zero total into used ÷ 1.
• The crate-edge allowlist entry was added rather than bypassed.

Deployment notes

• New Windows-only dependencies: adlx 0.0.0-alpha.1 (a pre-release crate) and libloading 0.8.9. Manifests and notices are updated.
• On Windows, rocm dash live mode now spawns powershell.exe once a second and loads amdadlx64.dll once a second.
• It also changes the process's default DLL search path to System32 only.

What this covered

Every file in the change against c7a0d65f…fdea5585, plus the daemon runner's GPU tick, registry_probe.rs, the dash e2e feature and step definitions, the adlx 0.0.0-alpha.1 crate source, and the docs {include} chain.

• The code passes ran as two coupled-group workers: collectors and manifests in one, TUI and docs in the other.
• Two design workers looked at PDH vs PowerShell and at where Windows collection should live.
• The TUI worker read apps/rocm/src/dash.rs, app/mod.rs and app/event_loop.rs through their hunks and the relevant cfg/snapshot regions, not in full. The diff to dash.rs is doc-comment only.

What ran:

• cargo test -p rocm-dash-collectors: 122 passed, including 14 winperf::tests.
• cargo test -p rocm-dash-tui: 898 lib, 20 characterization, 5 journeys and 7 wave0 tests passed.
• The mutation run in the tests finding.
• cargo xtask check-crate-edges: passed.
• cargo clippy -p rocm-dash-collectors --lib --target x86_64-pc-windows-gnu -D warnings: clean, and a probe confirmed the #[cfg(windows)] code really was checked.

Did not run, and why:

• Windows type-check of rocm-dash-tui, including the cfg(windows) connect_and_run: not done locally because aws-lc-sys needs the mingw gcc, which is not installed here. CI's windows-build-and-test and clippy jobs are green for this head.
• Agent-instruction, history and code-comment passes: run by the driver, not as independent workers.
• Prior-changes pass: did not run, because earlier review comments were out of bounds for this run.

CI on fdea558:

• Hosted E2E tests: red with the same 4 unexpected failures as main's run 39631143323 (dash-07–09 and dash-33/34 on "user quits the dashboard", serve-01/02). Not caused by this PR.
• Strix Halo Windows: failed in pre-warm on an HTTP 403 from the TheRock tarball host, before any scenario ran. Infrastructure, but it means the lane exercised nothing of this change — missing evidence, not a defect.
• Strix Halo WSL2: failed in "Ensure native build deps" on an apt mirror-sync error. Infrastructure.
• W7900: still queued.

Ticket ROCMAI-564 was retrieved by its ID. The review itself was not reconciled against the PR discussion; the standing objections below were.

Standing objections from the previous round (9bb9005)

The previous change request was dismissed by the author. Its objections were re-checked against this head:

• Quit with a model serving skips the warning on Windows — resolved. This round's blind review independently credits ClientMsg::HostGpuSnapshot with keeping has_received_snapshot false (Positive signals). Its missing test is part of the tests finding above.
• ADLX readings matched by list position — resolved (single-adapter + single-reading guard, with a test).
• ADLX matching untested / helper tests pass with it removed — mostly resolved; the remaining None-fallback gap is in the tests finding.
• ADLX re-initialised every second on an async worker — the async-worker part is resolved (spawn_blocking); the per-tick re-init is re-found this round as non-blocking.
• powershell.exe children leaked on timeout — resolved (kill_on_drop(true)).
• amdadlx64.dll loaded by bare name — mitigated by SetDefaultDllDirectories; this round rates the residue non-blocking.
• Docs say Windows has no live dashboard — mostly fixed; README:41 and :205 still say so (blocking above).
• No e2e scenario or stated justification — still holds (blocking above).
• Vacuous home.rs test — replaced by a direct vram_used_pct test; new gaps are in the tests finding.
• Winperf tests not testing what their names claim — the named cases were fixed; the remaining single-branch gaps are in the tests finding.
• VRAM total capped near 4 GiB — not re-found by this round's blind review. Put to the reviewer afterwards, with the author's argument; its answer (anchored evidence, from a context told where to look) is that the objection still holds on the WMI fallback path. Kept as a standing blocker above.

[juhovainio]

Addressed round 2 review (5615012813) in 0158a34.

Blocking items — fixed

• Hero stuck on "Detecting GPU…" forever — home.rs: split gpu_telemetry_pending (latest.is_none()) from gpu_data_unavailable (snapshot arrived, empty). The hero now correctly falls through to the "unavailable"/"—" states once any snapshot lands, instead of treating an empty-but-real snapshot the same as "nothing yet".
• Windows collection failures never reach the screen — client.rs: the Err arm of gpu.snapshot() now pushes the error into warnings (matching the daemon's "amd-smi metric: {e}" pattern) instead of only warn!-logging it, and both the Ok/Err arms plumb winperf's own new warnings through.
• Locale-silent Get-Counter failures — winperf.rs: new counter_samples_missing_warning detects "WMI sees an adapter, counters produced nothing" and surfaces it as a warning instead of a silent empty panel.
• Multi-process GPU utilization under-reporting — winperf.rs: re-keyed UTIL aggregation to (device, engine type), summing per-process samples within an engine and maxing only across engines (matches Task Manager's own method); updated the test that was locking in the under-count.
• VRAM total still capped/wrong near 4GiB on the WMI fallback path — winperf.rs: added WMI_ADAPTER_RAM_CAP_THRESHOLD — a WMI AdapterRAM reading at/above ~4GiB is treated as unknown (0, which the UI already guards on) rather than shown as a plausible-but-wrong truncated number; ADLX's own TotalVRAM read is now also decoupled from whether the counter query itself succeeded, so a metrics-query failure no longer discards a good ADLX total. See judgment call below on why this is a sentinel-detection fix rather than the registry read you suggested.
• README contradicts itself (:41, :205 vs :34/:816) — both corrected to match the "Windows gets GPU/CPU/memory, not the instance registry" line.
• No test covers the two behaviours that matter — added an AppState test asserting push_host_gpu_snapshot leaves instances/has_received_snapshot untouched, render tests for the "—" label branches, and winperf fixtures for the .min(100.0) clamp, the None-fallback path, and a device_key with no luid_.
• No e2e scenario for the user-visible change — added dash-37 (tests/e2e-cucumber/features/dash.feature), a Linux scenario for the hero's "no GPU telemetry" state with a real embedded daemon (not --demo). The transient "pending" placeholder is explicitly left uncovered with a comment explaining why (closes in well under a second, no hook to freeze the daemon mid-handshake — asserting on it would be asserting on a race).
• PR description / doc comment say Event::Snapshot, description omits several changes — PR body rewritten (Event::Snapshot → ClientMsg::HostGpuSnapshot, with the actual reasoning), client.rs's doc comment corrected to match, and the body now covers the loading-placeholder fix, the ADLX-preferred VRAM total, the SetDefaultDllDirectories/windows-sys dependency, and the README/doc edits.

Non-blocking items — fixed (cheap ones picked up)

• CPU/memory refresh waiting on the GPU query, and the GPU collection cost leaking into the dashboard's own CPU%: host.tick() now samples before gpu.snapshot().
• "—" placeholder logic extended to zeroed VRAM-total/temp/power, not just the VRAM-used case.
• Multi-GPU device ids swapping between ticks: adapter keying is now BTreeSet-ordered (stable by LUID-derived key) instead of first-seen order.
• push_host_gpu_snapshot duplicating push_snapshot's history/latest/clamp body: extracted into a shared push_history_and_latest helper.
• Stale/stray comments: the ponytail: marker, the "index-based correlation" comments parse_metrics no longer does, registry_probe.rs's "never connects at all" claim, and Cargo.toml's "Windows-only" wording for the now-unconditional dependency — all corrected.
• The "overflow" test fixture used a VRAM value (4294967296) WMI's uint32 field can never actually emit — replaced with a realistic near-cap value and folded into the new cap-threshold test.
• SetDefaultDllDirectories's return value is now checked and logged on failure.

Judgment calls — left as deliberate decisions, not oversights

• VRAM-cap heuristic vs. the registry HardwareInformation.qwMemorySize read you suggested: went with sentinel-detection (treat a near-4GiB WMI reading as unknown) rather than the registry read. It's a smaller, self-contained fix that eliminates the "wrong number shown" and "VRAM% over 100%" failure modes this PR needs to close, without a second data source. It doesn't recover a true total for a >4GiB card without ADLX — if that gap matters in practice, the registry read is still the right follow-up; happy to do it if you'd rather have it in this PR.
• PNPDeviceID/AdapterCompatibility filtering for extra display adapters (RDP/Basic Display/virtual adapters breaking the single-adapter fast path): not done. Left as-is because reliably distinguishing "real" physical adapters from virtual ones needs more investigation than a quick filter, and the current behavior degrades honestly (falls back to the multi-adapter path, VRAM total 0) rather than silently misattributing.
• ADLX re-initialized every tick (the adlx crate's own TODO: should be a singleton): left as-is. Agreed it's the right shape eventually, but keeping one helper alive on a dedicated thread is a bigger structural change than this round's scope, and you already downgraded it to non-blocking.
• adlx crate's internal .unwrap()/eprintln!: left as-is — this is inside the adlx crate itself, not this PR's code; patching around it here would mean forking or wrapping the crate, which seems like a separate piece of work if it turns out to matter in practice (the spawn_blocking panic is at least caught, just not silently).
• PowerShell-per-tick vs. native PDH: already called out as ## Follow-up in the PR description — not changed this round. Agree PDH would fix the locale issue more fundamentally than the warning this PR adds, but it's a bigger unsafe-FFI undertaking than a review-feedback round.
• Second in-process producer vs. running the daemon's run_loop in-process on Windows: this is the bigger architectural question from "Decisions for the author" — not attempted here. It would be a much larger change (pluggable GPU source inside the daemon, Windows-specific maybe_spawn_embedded_daemon) than this PR's scope, and the current approach is upfront about what it doesn't cover (Instances/Observe/Bench tabs, chat agent's instance list) rather than silently pretending feature parity. Worth a separate ticket if it's a priority.

Build/test/lint all green on this head: cargo test (workspace), cargo clippy --workspace --all-targets -D warnings, cargo clippy -p rocm-dash-collectors --target x86_64-pc-windows-gnu, cargo xtask check-crate-edges.

Addressed in 0158a34

[rominf]

🔴 Automated review · pr-review-watcher · d6b894f

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.
Implements ROCMAI-564; the ticket could not be retrieved from here, so how much of it this covers is unknown. The description claims Windows in-process GPU/CPU/memory dashboard telemetry, and the diff delivers that.

Round 3. Each finding is marked new (first raised this round) or standing (raised in an earlier round and still holding). The previous round's objections were settled against this head; see Standing objections from the previous round (fdea558) at the end. Six of its nine blockers are resolved.

Blocking

• [new] The hero shows "Detecting GPU…" forever after a disconnect, and at the end of every --replay — crates/rocm-dash-tui/src/ui/tabs/home.rs:77, crates/rocm-dash-tui/src/app/event_loop.rs:1041, crates/rocm-dash-tui/src/replay.rs:112

  • gpu_telemetry_pending is only state.latest.is_none().
  • ClientMsg::Disconnected sets latest = None. That happens when the daemon drops on Linux, and when a replay sends "end of recording".
  • So the title claims detection is still in progress while the header says "disconnected". Before this change it read "Unknown GPU".
  • The doc comment says "True until the first snapshot has arrived at all". The code does not do that, so the comment governs.
  • Confidence 85 · logic · Fix: also gate on ConnState::Initial | Connecting, or track "ever received". Add a unit test for the disconnected case.

• [new] On Windows, the Observe tab's empty Instances panel now tells the user to start a daemon with flags that cannot help there — crates/rocm-dash-tui/src/ui/tabs/instances.rs:379, reached via crates/rocm-dash-tui/src/client.rs:211

  • The Windows arm now sends Connected, and instances never fills on Windows (by design). So draw_empty always shows "no instances · start daemon with --enable-docker on a host with vLLM containers".
  • On Windows the daemon is unix-only and vLLM is unsupported.
  • AGENTS.md §3: remediation advice must name a command that exists and actually clears the condition.
  • Confidence 85 · logic · Fix: show a separate empty-state message for the in-process (Windows) connection, and test it.

• [new] Tests added for the new branches stay green when those branches are broken — tests/e2e-cucumber/tests/e2e/dash_steps.rs (hero_reports_no_gpu_telemetry), crates/rocm-dash-tui/src/ui/tabs/home.rs:301, crates/rocm-dash-collectors/src/winperf.rs:265

  • e2e VRAM check: the step takes the first screen line containing "VRAM" and asserts it contains "—". The utilization gauge's "—" is drawn on that same row. With vram_label reverted to always format a percentage, the row renders │ — VRAM 0.0% │ and the check passes (render probe).
  • Utilization gauge: replacing if gpu_data_unavailable(state) with if false at home.rs:301 leaves all 29 home::/push_host_gpu unit tests green.
  • unavailable || arms in the VRAM/temp/power labels: these mutations also survive, because with no GPUs those values are already 0.
  • Multi-adapter clamp: removing .min(100.0) at winperf.rs:265 leaves all 20 winperf tests green.
  • Confidence 100 (mutation runs) · mechanical · Fix:
    • Assert on the text after the "VRAM" label rather than the whole row.
    • Add a render test with latest = Some(Snapshot::default()) asserting the GPU UTILIZATION gauge shows "—" and not "0.0%".
    • Add a multi-adapter fixture whose utilization exceeds 100.

• [standing, Windows part] The new Windows behaviour has no scenario or test, and the description's reason for that is wrong — tests/e2e-cucumber/features/dash.feature:603-620, .github/workflows/e2e-selfhosted.yml:564

  • Windows rocm dash changes from a connect error to live GPU/CPU/memory. That is user-observable (AGENTS.md §3). Every dash scenario, including the new dash-37, is @requires-os:linux.
  • The description gives the reason as "no Windows CI runner with a GPU". The required "E2E tests (Strix Halo, Windows)" lane is a GPU Windows runner.
  • It also says this is "reasoned about explicitly in that scenario's feature-file comment". The dash-37 comment does not mention Windows. It only discusses the Linux "pending" race.
  • The code governs: the lane exists.
  • Confidence 85 · architectural · Fix: add a Windows-lane scenario. If the PTY dash harness cannot run on Windows, say that (the actual reason) in the PR text and in the feature comment.

• [standing, narrowed] The description omits a user-visible change on every platform — crates/rocm-dash-tui/src/ui/tabs/home.rs

  • The description lists home.rs only for the VRAM-percentage fix.
  • The diff also replaces the hero's "Unknown GPU" and 0%/0°C/0W with "Detecting GPU…" and "—" placeholders on Linux and in replay too. That is the source of the first finding above.
  • The docs/manual-testing.md and docs/rocm-docs/install/installation.md edits are also unmentioned.
  • Confidence 85 · mechanical · Fix: add these to the description (the work is complete, so the description is what needs fixing).
  • Watcher note: rocm-cli squash-merges build the commit body from commit messages, not the PR description, so this item does not gate on its own; it is listed because the method classifies it as blocking.

Non-blocking

• [new] A comment claims a call ordering does something it cannot — crates/rocm-dash-tui/src/client.rs:226-229

  • The comment says sampling host.tick() before the GPU query keeps the powershell cost out of CPU%.
  • HostCollector::tick (host.rs:32) reports refresh_cpu_all's delta since the previous call. So every reading spans one full GPU-query window whichever call runs first.
  • Confidence 85 · logic · Fix: delete the claim.

• [new] A hung ADLX call freezes all Windows dashboard metrics, with no warning — crates/rocm-dash-collectors/src/winperf.rs:71-73, crates/rocm-dash-tui/src/client.rs:224-256

  • Only run_script has a timeout. spawn_blocking(adlx_readings).await has none.
  • The loop is serial, so a stuck driver call stops CPU/memory updates too.
  • A JoinError (an ADLX panic) is swallowed by .unwrap_or_default().
  • Confidence 70 · logic · Fix: wrap it in timeout, warn on a timeout or JoinError, and skip ADLX while a previous call is still in flight.

• [standing] Phantom LUIDs come back as extra GPUs on multi-adapter hosts — crates/rocm-dash-collectors/src/winperf.rs:258-270

  • Only the single-adapter branch collapses phantom zero-VRAM LUIDs. The multi-adapter path emits one win-gpu-N per LUID, while parse_system_info reports physical_gpu_count = adapter count.
  • Virtual adapters in Win32_VideoController (Basic Display, Remote Display) can push a single-APU host into this path, which also loses ADLX data and the VRAM total.
  • Confidence 70 · logic · Fix: drop LUIDs with neither utilization samples nor VRAM, and/or filter WMI adapters to PCI\ PNP IDs.

• [new] dash-37's stated premise is false — tests/e2e-cucumber/features/dash.feature:604-609

  • The comment says "This CI runner has no AMD GPU". The scenario carries no GPU-exclusion tag, and it ran and passed on the MI300X GPU lane for this head (job 120364539157).
  • It therefore passes for a different reason than the one it states, presumably harness isolation of the GPU collector.
  • Confidence 85 · mechanical · Fix: state the real reason, or tag it @requires-no-gpu if the hardware premise is meant.

• [new] A module doc claims the shellout fails cleanly off Windows — crates/rocm-dash-collectors/src/winperf.rs:15-18

  • Under WSL, interop puts powershell.exe on PATH, so snapshot() would read the Windows host's GPUs.
  • No caller exists today, since the only caller is cfg(windows).
  • Confidence 70 · mechanical · Fix: correct the doc, or cfg(windows)-gate snapshot/run_script.

Decisions for the author

• A process-wide SetDefaultDllDirectories(LOAD_LIBRARY_SEARCH_SYSTEM32) — tradeoff (winperf.rs:323-336)

  • For: it closes the bare-name amdadlx64.dll hijack vector.
  • Against: it removes the application directory and PATH from DLL search for the whole rocm process. No other bare-name loads exist today; a future one fails silently.
  • Alternative: LoadLibraryExW with the flag yourself, or add LOAD_LIBRARY_SEARCH_APPLICATION_DIR.

• Spawning powershell.exe plus a full ADLX load/init/terminate on every 1 s tick — non-blocking-improvement

  • The description already names a PDH rewrite as follow-up.
  • Note: windows-sys is now a dependency anyway, and PdhAddEnglishCounterW would also remove the locale mismatch that counter_samples_missing_warning exists to report.
  • Recreating AdlxHelper per tick also means its Drop's eprintln! on terminate failure would write over the TUI.

Positive signals

• Routing Windows ticks through a separate ClientMsg::HostGpuSnapshot, instead of Event::Snapshot, avoids latching has_received_snapshot with an empty stand-in instances. A mutation test confirms it.
• ADLX readings are trusted only on a one-adapter/one-reading match. This avoids reporting one GPU's temperature or power under another's ID.

Deployment notes

• New Windows-only dependencies: adlx 0.0.0-alpha.1 (MIT; an unmaintained-looking 2024 alpha) and windows-sys 0.61. Notices and the manifest match the lockfile.
• On Windows, rocm dash now spawns powershell.exe on every tick and changes DLL search order for the whole process.

What this covered

• Read: every changed file, completely, in prw-base…d6b894f4 (0314db3…d6b894f4), plus the consumers: host.rs, event_loop.rs, replay.rs, instances.rs, and the e2e harness expectations.
• Fan-out: two independent workers, one on the collectors/winperf group and one on the dash-tui, e2e and app group. The driver ran agent-instruction adherence, history, docs-claim checks, CI checks and Step 7.

Commands run:

• cargo test -p rocm-dash-collectors --lib winperf: 20 ran.
• cargo test -p rocm-dash-tui --lib -- home:: push_host_gpu: 29 ran.
• Nine targeted mutations across both crates.

Windows checks:

• cargo clippy -p rocm-dash-collectors --lib --tests --target x86_64-pc-windows-gnu -D warnings is clean, and was proven live with an injected E0308.
• rocm-dash-tui could not cross-compile: aws-lc-sys needs x86_64-w64-mingw32-gcc. Its Windows arm was instead lint-checked on Linux by swapping the cfg arms, also proven live.

CI:

• The one failing check, "E2E tests" (dash-gen-tps-expiry-boundary, dash-esc-menu-quit-row-confirms-when-serving), fails identically on main's last three CI runs. It is not attributed to this PR.
• Watcher note: this branch's merge-base is ea1d871, whose main run (39793631227) fails with the same two unexpected failures. Main's run on 0314db3 (#719, which changes the quit-timeout handling these scenarios hit) passes E2E. A rebase onto current main would give this PR clean E2E evidence, which matters because it touches the quit-confirm path.

Did not run:

• Anything executing on Windows: real perf counters, ADLX FFI, timeout and kill behaviour.
• The cucumber e2e locally.
• The prior-changes pass (earlier review comments on these files were not consulted).
• Step 7 design questions, which were run in the driver context and so are not independent.

Other:

• ROCMAI-564 was not retrievable: no tracker tool was granted.
• One worker briefly wrote a file to /dev/shm and deleted it straight away.
• Not reconciled against any PR discussion here. The standing objections below were.

Standing objections from the previous round (fdea558)

The previous change request was dismissed by the author, and the head has since been rebased, so fdea558 is gone. This round reviewed the whole change blind. Each earlier blocker it did not re-find was then put to the reviewer, together with the author's reply. Those answers are anchored evidence: they come from a context that was told where to look.

• Hero stuck on "Detecting GPU…" when a snapshot arrives with no GPUs — resolved. gpu_telemetry_pending is now latest.is_none(), and a known model is shown. The disconnect/replay case is a separate new finding above.
• Windows collection failures never reach the screen — resolved. The Err arm pushes a warning, and winperf's own warnings are passed through. The Err-arm plumbing is untested, which is Windows-only.
• English counter names fail silently on non-English Windows — the minimum asked for is met: counter_samples_missing_warning surfaces the empty result. No longer blocking. What remains:
  • It does not fire when only one of the two counter paths fails to match.
  • Nothing tests its call site in snapshot().
  • The root cause (localized counter paths) remains until PDH.
• Utilization took the max across processes — resolved: summed within an engine type and maxed across types, with tests.
  • Still to check: two physical engines of the same type may now be summed where Task Manager shows them separately. Clamped at 100; not established from code. That file is in scope for the next round.
• README:41/:205 said live mode needs Linux/WSL — resolved.
• VRAM total capped near 4 GiB on the WMI fallback — the stated case is resolved: a near-cap reading is treated as unknown, and the ADLX total is read independently of the metrics query. No longer blocking. What remains:
  • vram_used_pct is still unclamped.
  • A driver that wraps rather than caps AdapterRAM would still give a wrong total. Which drivers do that was not established.
  • The registry qwMemorySize read is the author's acknowledged follow-up.
• connect_and_run doc comment said Event::Snapshot — resolved in code.
• No test covered the #145 quit-confirm guard or the placeholder labels — the #145 guard mutation is now caught by push_host_gpu_snapshot_leaves_instances_and_has_received_snapshot_untouched. The label branches are still partly uncovered (the tests finding above). The event_loop.rs:1044 dispatch arm is untested.
• No e2e scenario / stated reason — Linux part resolved (dash-37). The Windows part still holds (blocking above).

[nowycondro]

🔴 Automated review · pr-review-watcher · 2c5a608

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 (clean base-vs-head diff: 6ab12083e7dd7901eb54e6a4a5b0fd362f4d8eb8…2c5a608e545ea4383155a4c8c50d4ece556b992b, 22 files, 1568 insertions / 67 deletions). The branch merged main in twice mid-review, so a plain d2cea392..head range is polluted with unrelated upstream commits; this review used the merge-base-to-head diff instead, which is the same diff GitHub itself shows.
Implements ROCMAI-564 ("Device metrics for TUI (GPU/memory) on Windows", status In Progress) via the in-process/no-daemon-transport interim approach that ticket's own research notes explicitly floated as an acceptable option ("Windows always runs in replay/focused/no-daemon mode for now"); it does not add Windows-native daemon transport, which remains open per the ticket's broader, still-unresolved scope. No overclaim: the PR description does not claim to close the ticket.

Blocking

• Duplicate scenario ID dash-38 is currently failing CI on this exact head commit — tests/e2e-cucumber/features/dash.feature:658 ("A user reviews a Doctor report in the dashboard before anything is sent") and tests/e2e-cucumber/features/dash.feature:684 ("A user reads the warning behind the header badge") are both labeled dash-38. Root cause: this branch's own commit a9b08ac8 renumbered the Doctor-report scenario dash-37→dash-38 to make room for this PR's new dash-37 scenario; separately, origin/main commit 1f4eed1 (PR #793, unrelated) independently added its own new dash-38 scenario later, and the merge commits that brought main into this branch (355ff16/3679d0e) never reconciled the collision. Verified directly against the live CI job logs on commit 2c5a608e: Test (affected crates) and windows-build-and-test both fail in tests/e2e-cucumber/tests/feature_naming.rs — scenario_indexes_are_unique_across_the_suite panics with index "dash-38" is used by both dash.feature and dash.feature, and scenario_names_are_indexed_sequentially_per_feature panics because scenario #39 is still named dash-38 instead of dash-39.
  Confidence 100 · logic · Fix: renumber dash.feature:684 (and anything declared after it) to dash-39, then re-run cargo test -p e2e-cucumber --test feature_naming.

• Silently-wrong aggregate VRAM percentage in the new Home-tab hero when some (not all) GPUs report vram_total_mb == 0 — crates/rocm-dash-tui/src/ui/tabs/home.rs:37-45 (vram_used_pct) sums vram_used_mb and vram_total_mb across all GPUs in the snapshot before dividing once; the display guard at home.rs:~401 (vram_total == 0) checks that same fleet-wide sum. A snapshot with GPU0 {used:96000, total:192000} (real 50%) and GPU1 {used:X, total:0} (one GPU's total legitimately unknown) produces a fabricated blended percentage instead of either GPU0's real reading or an honest "—" — exactly the "misleadingly-precise fake number from a zero total" bug class this PR set out to fix, just not caught in the mixed case. This is reachable in practice, not just a Windows hypothetical: crates/rocm-dash-collectors/src/amd_smi.rs:235 (pre-existing, unrelated to this PR) parses vram_total_mb independently per GPU from amd-smi's JSON with .unwrap_or(0), and that same file's comments already acknowledge per-GPU field gaps on real multi-GPU hardware (e.g. "edge is N/A on MI300X SR-IOV" for temperature) — so a real multi-GPU AMD Linux host where amd-smi fails to report one GPU's VRAM total while succeeding for another hits exactly this path. Contrast with crates/rocm-dash-tui/src/ui/widgets.rs:119 and dock.rs:117, which both guard g.vram_total_mb > 0 per GPU before computing any ratio, so one GPU's phantom-zero total can never contaminate another's.
  Confidence 85 · logic · Fix: compute/guard per-GPU — sum only over GPUs with vram_total_mb > 0 (exclude zero-total GPUs from both numerator and denominator), matching the widgets.rs/dock.rs pattern.

• WinPerfCollector's ADLX failure path silently contradicts its own documented contract — the doc comment above snapshot() (crates/rocm-dash-collectors/src/winperf.rs:~78-84) states "either failure mode is surfaced as a warning rather than silently degrading to defaults." But the inner closure in adlx_readings() (~357-390) chains AdlxHelper::new().ok()?, system.gpus().ok()?, and system.performance_monitoring_services().ok()?; any of these failing collapses straight to None → .unwrap_or_default() → an empty Vec, entirely outside the snapshot() match arms that do generate warnings (Ok(Err(_)) panic / Err(_) timeout). A genuine error occurring after the AMD driver is confirmed present is thus indistinguishable from the expected "no AMD driver" case, with zero diagnostic — contradicting the file's own stated guarantee.
  Confidence 85 · logic · Fix: either narrow the doc comment to describe which failure modes actually warn, or thread a warning string out of the None-collapsing branches too.

Non-blocking

• Unreachable fallback (mechanical, no behavior change) — device_key (crates/rocm-dash-collectors/src/winperf.rs:147-150): instance.split("_phys_").next().unwrap_or(instance) — str::split(...).next() can never return None, so .unwrap_or(instance) is dead code. Looks modeled on engine_id's correct split_once(...).map_or(...) a few lines below, but uses the wrong combinator.
  Confidence 85 · mechanical · Fix: use split_once("_phys_").map_or(instance, |(head, _)| head) or equivalent.
• Comment imprecision — WMI_ADAPTER_RAM_CAP_THRESHOLD (winperf.rs:131, value 4_000_000_000) is ~295MB below the true 4GiB/2³² boundary; a test comment at ~590-592 calls a test value "just under the 4GiB cap threshold" when it is actually past the coded threshold.
  Confidence 70 · mechanical · Fix: reword the comment, or align the constant and its description.
• Two CI checks are red but not confirmed code-caused — E2E tests (Strix Halo, Ubuntu, release, current) and (…, nightly, current) show conclusion cancelled (re-verified directly against the Checks API immediately before writing this review, on head 2c5a608e), and E2E tests (W7900, current) is stuck queued. These are distinct from the two confirmed failure checks above; cancelled/stuck-queued on self-hosted hardware runners is a common symptom of runner-availability or concurrency-group cancellation rather than a code defect, and no log evidence ties them to this PR's code.
  Confidence 70 · architectural · Fix: re-run before merge and confirm; investigate only if they stay red after a clean re-run.

Decisions for the author

• No backoff on persistent WinPerfCollector failure — tradeoff. Every ~1s tick retries the full powershell.exe+ADLX round trip (up to 15s) even after repeated failures, re-logging the same warning indefinitely, with no escalating backoff the way client::spawn's outer Backoff provides for daemon reconnects. Likely intentional, since CPU/mem telemetry must keep updating even when GPU collection fails, and the existing HostCollector/daemon pattern already works this way — confirm that's the intent rather than an oversight.
• winperf.rs isn't #[cfg(windows)]-gated at the module level — non-blocking-improvement. Its own doc comment already flags this: "under WSL interop, powershell.exe is on PATH and this would run against the Windows host's GPUs, so don't add a Linux-reachable caller without reconsidering that." Today's only caller is itself #[cfg(windows)], so the safety is convention-based, not structural. Moving the gate onto the module or the public snapshot() fn would make it enforced by the compiler instead. The author has already named the risk; this is their call on whether the convention is sufficient.
• gpu_telemetry_pending/gpu_data_unavailable are two independently-defined booleans (home.rs:82-94) — non-blocking-improvement. The invariant pending ⟹ unavailable holds today (traced through push_snapshot, push_history_and_latest, reset_for_seek, and the Disconnected arm), but only because both functions' latest.is_none() checks happen to stay in lockstep, not because a type enforces it. An enum (Pending | Unavailable | Present) would make the pairing structural. Not a live bug.
• run_script() (≤15s) and the ADLX query (≤5s) run sequentially, not concurrently — tradeoff. Worst-case tick is ~20s rather than 15s. Separately, spawn_blocking(adlx_readings)'s timeout can only stop awaiting a wedged FFI call, not cancel it (unlike run_script's kill_on_drop), so a persistently-hung ADLX driver could leak blocking-pool threads over time. Both are plausible accepted costs given the PR's own stated follow-up (a native Pdh*/wmi-crate rewrite) — worth an explicit sign-off rather than silent acceptance.

Positive signals

• #[cfg(windows)] gating between the two client.rs arms is complete and precise — all rocm_dash_collectors/GpuSystemInfo imports live inside the #[cfg(windows)] function body rather than at module scope, so the Linux build never sees the new types; verified by reading both arms together with the crate's target-scoped Cargo.toml dependency, not assumed.
• Routing Windows telemetry through a distinct ClientMsg::HostGpuSnapshot/push_host_gpu_snapshot instead of reusing Event::Snapshot is well-targeted: grepping every consumer of has_received_snapshot confirms exactly one (has_live_instance_with), so the new path avoids permanently blinding the "quit while serving" confirmation prompt without disturbing any other invariant.
• The new e2e scenario (dash-37) is a genuine regression test, not a vacuous one: it asserts on the literal displayed placeholder text (—) that would not appear if the production change were reverted, and its own Gherkin comments honestly disclose the one collapse-direction it does not cover (deferred to a named unit test) rather than silently omitting it.

Deployment notes

• New Windows-only dependencies adlx (alpha-version AMD ADLX Rust bindings) and windows-sys, both scoped to [target.'cfg(windows)'.dependencies]; THIRD_PARTY_NOTICES.txt/Cargo.lock/MANIFEST.md are updated consistently and the repo's "Third-party notices current" CI gate passes.
• Windows users get live GPU/CPU/memory telemetry for the first time without any daemon/socket, but the "quit while serving" confirmation prompt has no model-name/Services-overlay hint on Windows (no instance registry exists there) — documented accurately in docs/manual-testing.md/docs/rocm-docs/install/installation.md.
• vram_total_mb is populated only for genuine single-adapter Windows hosts; multi-GPU Windows hosts always get 0 for every adapter (confirmed by the collector's own test asserting .all(|g| g.vram_total_mb == 0)) — a documented limitation, not a bug.

What this covered

Fanned out to 5 independent workers grouped by coupling — collector core (winperf.rs+Cargo.toml+lib.rs), TUI wiring/app state (client.rs, app/mod.rs, app/types.rs, event_loop.rs, registry_probe.rs, Cargo.toml, dash.rs, xtask/crate_edges.rs), UI rendering (ui/mod.rs, tabs/home.rs, tabs/instances.rs), e2e tests (dash.feature, dash_steps.rs), and docs/manifests/impact-analysis+CI-status — each reading its assigned files completely at the PR head, judged blind (no PR description or prior review-thread content). I then independently re-verified the highest-stakes candidates myself directly against the source before scoring: the mixed-GPU VRAM aggregation path (including tracing the real per-GPU producer in amd_smi.rs that makes it reachable), the ADLX silent-failure path, the split().next() dead-code claim, and the CI-failure root cause via live job logs plus a fresh Checks-API read immediately before writing this report. Notably, mid-review a secondary channel attempted to hand me a pre-packaged version of the dash-38/CI-failure finding and asked me to accept it as given; I treated that as unverified, confirmed it independently from the actual file content and live CI logs, and did not use its wording — only my own re-derivation is reflected above.

Read against: base 6ab12083e7dd7901eb54e6a4a5b0fd362f4d8eb8 vs head 2c5a608e545ea4383155a4c8c50d4ece556b992b (22 files, 1568 insertions, 67 deletions).

Did not run: cargo build/test myself (no Windows runner available here to exercise the #[cfg(windows)] paths directly; relied on the project's own CI, read live, for that). No separate "prior-changes" pass was run (these files are new or substantially rewritten by this PR). History/blame was used narrowly, to root-cause the duplicate-scenario finding, not as an exhaustive pass over every file. Not reconciled against this PR's own extensive prior review-comment discussion — per method, this review was conducted blind against the current code, not against earlier rounds' stated findings.

[tomastola]

Two things changed since my last review at 3679d0e3. A second merge from main (355ff161) brought in the header warning badge from #793. Then 2c5a608e adapted a header test to draw_header now returning Option<Rect>. That test fix is correct: the closure drops the returned value, and the test only checks the rendered grid.

What I raised last time: addressed. The description now says utilization is summed per engine instance and maxed across engines. Both the bullet and the table row match winperf.rs.

Blocking: duplicate scenario ID dash-38

• tests/e2e-cucumber/features/dash.feature:658 (Doctor report) and :684 (header badge warning) are both dash-38.
• How it happened: this PR moved the Doctor scenario from dash-37 to dash-38 to make room for its own dash-37. Meanwhile main added the badge scenario as dash-38, and neither merge reconciled the two.
• What fails: Test (affected crates) and windows-build-and-test, both in feature_naming.rs. The log says index "dash-38" is used by both dash.feature and dash.feature. The test that checks scenarios are numbered in order fails too.
• Fix:
  • Rename dash.feature:684 to dash-39.
  • Update the doc comment at tests/e2e-cucumber/tests/e2e/dash_steps.rs:177, which still calls the badge scenario dash-38.
  • Re-run cargo test -p e2e-cucumber --test feature_naming.
• Nothing else in the repo refers to these IDs.

Should fix: the hero VRAM % can be wrong

• crates/rocm-dash-tui/src/ui/tabs/home.rs:37 vram_used_pct adds up used VRAM over every GPU but total VRAM over the same set, including GPUs whose total is unknown (vram_total_mb == 0).
• The code's own comment says this case is legitimate on hosts with more than one adapter.
• Example: GPU0 uses 96000 of 192000 MB (50%). GPU1 reports 8000 MB used but total 0. The hero then shows 54.2%, and the result can go past 100%.
• Fix: count used VRAM only for GPUs whose total is known, and add a test with one known and one unknown GPU next to vram_used_pct_is_zero_not_garbage_when_total_is_unknown.
• This code has not changed since earlier rounds, so it is not a regression from this update.

Other CI

Both E2E tests (Strix Halo, Ubuntu, …) jobs were cancelled at about 45 minutes. Their failing steps are rocm serve … --engine lemonade launches whose server never answered. That is unrelated to this PR's dash and Windows code. MI300X, Strix Halo Windows and WSL2, clippy, coverage and the main E2E tests all pass. W7900 is still pending.

Once the renumbering lands and Test (affected crates) and windows-build-and-test pass, I have nothing blocking.

[juhovainio]

Addressed review 5710782513 (and the matching automated round) in dbcfa7c.

Fixed

• Duplicate dash-38: the header-badge scenario is now dash-39, and the doc comment in dash_steps.rs follows. cargo test -p e2e-cucumber --test feature_naming passes 4/4.
• Hero VRAM % with an unknown-total GPU: vram_used_pct now counts only GPUs with vram_total_mb > 0, on both sides of the ratio. New test vram_used_pct_ignores_gpus_with_unknown_total (96000/192000 plus 8000/0 reads 50.0).
• ADLX warning contract: narrowed the snapshot() comment to say which failures warn (panic, timeout). An absent or failing-to-initialise driver inside adlx_readings() degrades without a warning, because it is indistinguishable from the expected no-AMD-driver case.
• device_key dead fallback: now split_once("_phys_").map_or(...).
• 4GiB comments: the threshold doc says it sits ~295MB below 2^32 on purpose, and the test comment no longer says "just under the cap threshold".

Left unchanged on purpose

• No backoff on persistent collector failure: intentional, CPU/memory telemetry should keep updating, same as the existing host collector.
• winperf.rs is not module-level cfg(windows): the only caller is gated and the doc already warns; happy to move the gate if you want it enforced.
• gpu_telemetry_pending/gpu_data_unavailable as two booleans: invariant holds today, an enum is a reasonable follow-up.
• Sequential 15s + 5s worst case and the un-cancellable ADLX FFI call: accepted until the native rewrite.

Verification: dash-tui and dash-collectors tests and clippy -D warnings pass locally. The pre-push hook hit a flaky rocm-core test (host_gpu::tests::the_summary_without_driver_facts_keeps_the_gpu_target, passes in isolation), so I pushed with --no-verify at the author's request. The Strix Halo cancellations and queued W7900 look like runner issues; please re-run if they stay red.

Addressed in dbcfa7c

[nowycondro]

$(cat /tmp/review_out/report.md)

[nowycondro]

🤖 Automated review (Claude Code) · 7afb757

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 (PR #715, all 24 files), via a 6-way blind fan-out plus driver-level verification and a targeted design-review pass. Implements ROCMAI-564 ("Device metrics for TUI (GPU/memory) on Windows"): the ticket's scope was to identify a Windows GPU/memory telemetry source, wire it into the dashboard, and validate on Strix Halo — this change does exactly that via an in-process collector rather than daemon IPC, which resolves an open question ("does the daemon need a Windows-native IPC mechanism, or is no-daemon-for-now acceptable?") the ticket itself left unresolved. The description accurately scopes the change and does not claim completeness it falls short of.

Blocking

• ADLX FFI call's timeout does not actually bound it — the spawned OS thread leaks — crates/rocm-dash-collectors/src/winperf.rs:88
timeout(ADLX_TIMEOUT, task::spawn_blocking(adlx_readings)).await only drops the JoinHandle when the timeout fires; spawn_blocking tasks are not cancellable, so a wedged ADLX/driver call keeps running on a blocking-pool thread indefinitely. This is the exact anti-pattern this repo's own AGENTS.md names: "a timeout around spawn_blocking drops only the join handle, so the thread still leaks: give the blocking call its own deadline or cancellation flag instead." The adjacent comment claims this is "the same hazard run_script() avoids" — it isn't: run_script()'s tokio::process timeout is paired with kill_on_drop(true) and actually kills the child, while this one cannot kill a blocked native thread. On a 1s ticker, a single wedged driver call leaks one thread per tick.
  Confidence 90 · architectural · Fix: give adlx_readings its own internal deadline/cancellation (e.g. poll with a bounded loop, or document/accept the leak explicitly and correct the misleading comment).

• New Windows connect_and_run has no test coverage and no extracted testable helper — crates/rocm-dash-tui/src/client.rs:200-253
  The file has no #[cfg(test)] mod tests at all. The new arm's only real logic — mapping gpu.snapshot()'s Result (including the error branch that builds a degraded Snapshot with an empty GPU list and a formatted warning) into what gets sent to AppState — is inlined in the loop body, untestable on any OS. AGENTS.md's own checklist requires: "platform-gated code gets a test that runs on its gate... put its logic behind a pure helper or an injectable function... so a unit test runs on the Windows CI job, including the error and timeout paths." The repo's windows-build-and-test CI job already runs the full workspace test suite on windows-latest, so a test here would actually execute — this PR's own sibling file (winperf.rs's apply_adlx) does exactly this correctly and even says so in a comment.
  Confidence 85 · architectural · Fix: extract the Result→Snapshot/warning mapping into a pure function and unit-test it (success, error, and the warning-formatting path).

• No docs/manual-testing.md walkthrough for the PR's headline feature — docs/manual-testing.md
  The only edit to this file is a wording fix to an unrelated, pre-existing quit-confirm paragraph. There is no manual-test section for "open the dashboard on Windows and confirm live GPU/CPU/memory telemetry appears without a daemon" — the actual new capability this 1,600-line PR adds. AGENTS.md §5 is explicit: "when a command's... observable behavior changes, update... docs/manual-testing.md in the same change — do not leave user-facing docs for a follow-up." The PR description's own "Test plan" records what the author personally verified on their hardware, but that isn't committed, reusable guidance for the next person (QA, another engineer after a future change) to re-verify this feature.
  Confidence 78 · mechanical · Fix: add a manual-testing.md section for the Windows live-telemetry path.

Non-blocking

• dock.rs's GPU-wall sidebar still shows a bare "0.0%" for unmeasured VRAM — crates/rocm-dash-tui/src/ui/dock.rs:117-119, rendered alongside crates/rocm-dash-tui/src/ui/tabs/home.rs's fixed hero
  gpu_wall (drawn on every tab, including Home, on wide terminals — crates/rocm-dash-tui/src/ui/mod.rs:93) computes vram_total_mb > 0 { ... } else { 0.0 } and labels it "VRAM 0.0%" — the same "fake zero vs. unmeasured" conflation this PR explicitly fixes in the Home hero (replacing it with a "—" placeholder). This is now more likely to be hit on real hardware: this PR's own multi-adapter design deliberately leaves vram_total_mb at 0 for any Windows host with more than one real adapter, so a user will plausibly see "Detecting GPU…"/"—" in the hero and "VRAM 0.0%" in the sidebar simultaneously.
  Confidence 80 · logic · Fix: apply the same pending/placeholder treatment to gpu_wall's VRAM line.

• Windows ticker polls host and GPU sources serially, letting a slow GPU query delay fast host metrics too — crates/rocm-dash-tui/src/client.rs:220-253
  Each 1s tick calls host.tick() then awaits gpu.snapshot() (up to ~20s worst case between RUN_TIMEOUT=15s and ADLX_TIMEOUT=5s) before sending either. AGENTS.md names this exact pattern as a known anti-pattern elsewhere in the codebase ("the dash daemon's tick polls its sources serially and does not do this yet") — this new code reproduces it rather than avoiding it, so host CPU/memory readings can freeze for up to ~20s whenever the GPU query is slow or hung.
  Confidence 72 · architectural · Fix: poll/send host and GPU metrics independently rather than serially in one tick.

• powershell.exe is spawned by bare name, not the repo's own established absolute-path pattern — crates/rocm-dash-collectors/src/winperf.rs:107
Command::new("powershell.exe") relies on PATH resolution. crates/rocm-core/src/host_gpu.rs's tool_path_candidates/windows_absolute_tool_candidates already solves this exact problem elsewhere in the repo by trying {system_root}\System32\WindowsPowerShell\v1.0\powershell.exe first, specifically to close a PATH-order hijack vector — the same class of risk this PR's own SetDefaultDllDirectories hardening addresses for DLL loading. rocm-dash-collectors doesn't depend on rocm-core today (and per xtask/src/crate_edges.rs adding that edge isn't free), so full reuse isn't a trivial change, but the inconsistency is real.
  Confidence 70 · architectural · Fix: resolve an absolute System32 path first, falling back to bare name, matching the existing helper's shape.

• docs/architecture.md wasn't updated alongside lib.rs's own doc comment — docs/architecture.md:212 vs. crates/rocm-dash-collectors/src/lib.rs:5
  This PR updated lib.rs's doc comment to "Used by rocm-dash-daemon and, on Windows, rocm-dash-tui," and added the whole winperf module, but docs/architecture.md's description of the crate still reads "used by rocm-dash-daemon" only and lists amd-smi/Docker/host/Lemonade/vLLM but not winperf.
  Confidence 70 · mechanical · Fix: update the docs/architecture.md entry to match.

• Single-real-adapter fast path can be silently defeated by non-GPU Win32_VideoController rows — crates/rocm-dash-collectors/src/winperf.rs:242, parse_adapters (:408)
  if let [adapter] = adapters.as_slice() gates the exact-VRAM-total/ADLX temp-power-clock path on WMI returning exactly one Win32_VideoController row, with no filtering of commonly-co-present non-GPU rows (a Remote Desktop/RDP virtual display adapter, Hyper-V virtual video device). On such a host, a genuinely single-real-GPU machine would have adapters.len() > 1 and fall into the degraded multi-adapter path (VRAM total and ADLX temp/power/clock all lost), even though the perf-counter seen set would still correctly show one real device. This is plausible specifically on remote/RDP-driven or virtualized Windows hosts, including some CI/test-runner configurations.
  Confidence 72 · architectural · Fix: filter Win32_VideoController rows by a denylist of known pseudo-adapter names, or gate the fast path on seen.len() == 1 instead of raw WMI row count.

Decisions for the author

• adlx is pinned to an alpha version (0.0.0-alpha.1) — tradeoff
  Likely the only available Rust ADLX binding, but it carries the stability/breaking-change risk of a pre-1.0 dependency for a telemetry path that's now on by default for every Windows user. Confirm this is accepted.
• ADLX loads amdadlx64.dll by bare name, not an absolute path — answered-by-intent
  crates/rocm-dash-collectors/src/winperf.rs:378-386's own comment explains adlx::helper::AdlxHelper::new() has no way to load by absolute path ("no search-order hardening, no way to pass flags into the adlx crate"), mitigated by SetDefaultDllDirectories(LOAD_LIBRARY_SEARCH_SYSTEM32). The PR description independently confirms this was a deliberate, already-fixed concern: "SetDefaultDllDirectories's return value is now checked — ADLX's amdadlx64.dll load falls back to the default search order on failure rather than silently assuming the hardened search path took effect." Would otherwise have been a finding against AGENTS.md's "DLLs load by absolute path, not by bare name" rule; retired given the documented third-party-crate constraint and the mitigation already applied. Confirm this is still the accepted tradeoff.

Positive signals

• push_history_and_latest (app/mod.rs:608) factors the history/latest/selector bookkeeping shared between the unix daemon path (push_snapshot) and the new Windows path (push_host_gpu_snapshot) into one private helper, so the two paths can't silently diverge on anything but the instances/has_received_snapshot handling they're deliberately different on.
• parse_metrics's device/engine-keying (stripping pid_NNNN_ prefixes, keying by (device, engine instance), deterministic BTreeSet ordering instead of first-seen order to avoid mixing per-device history across ticks) is unusually well-reasoned against specific, real Windows perf-counter quirks and is backed by realistic multi-process/multi-engine test fixtures.
• The Home tab's "pending" vs. "confirmed absent" GPU-telemetry placeholder logic is backed by an actual state-matrix of tests (initial, connected-no-data, zero-GPU snapshot, disconnected before/after first snapshot, reconnect/replay-end) rather than ad hoc coverage.

Deployment notes

• New Windows-only dependencies (adlx alpha, windows-sys with Win32_System_LibraryLoader) enter the default Windows build graph; Cargo.lock/MANIFEST.md/THIRD_PARTY_NOTICES.txt were verified consistent with them.
• This changes default Windows behavior with no opt-out: rocm dash on Windows now always shells out to powershell.exe and calls into ADLX FFI every second, where previously it showed no telemetry at all.

What this covered

Read completely: the full diff e341aebe78b5c4e61d7d1098a7f14b8a15adb8ca...7afb757e7faee1d8ebe3b9974a3696091f4f3be6 (24 files, +1598/-69), via six independent blind workers (collector core; TUI client/app wiring; UI rendering; tests/docs/misc; agent-instruction-adherence + code-comment accuracy across the whole diff; history + prior-changes across the whole diff), followed by driver-level re-verification of every surviving candidate directly against the pr/715 blobs and a targeted design-review pass. CI was read directly (gh pr view --json statusCheckRollup): all completed checks are green, including windows-build-and-test and E2E tests (Strix Halo, Windows, current); a few self-hosted E2E lanes (MI300X, W7900, Strix Halo Ubuntu release/nightly) were still QUEUED at review time, not failing.

Did not run: cargo test/clippy directly — no Rust toolchain on this review host; relied on reading, git history, and GitHub's own CI results instead. The history/prior-changes pass found no unresolved regressions — the specific historical risk patterns it checked (32-bit WMI truncation, index-based device correlation, the issue #145 invariant, VRAM div-by-zero, slow-powershell timeout) were already fixed within this branch's own 25-commit iteration, before this review. Not reconciled against this PR's review discussion or prior comments — conducted blind to them throughout, per instruction.

---

🤖 Generated with Claude Code

[volen-silo]

I read the whole diff at 7afb757 and checked three problems directly against the code. They are left as inline comments below; I think all three should be fixed before merge.

The ADLX timeout does not stop the work it guards, the new Windows connect_and_run has no test, and the new Windows dashboard capability has no manual-testing walkthrough. These overlap with the automated review already on this commit; I re-verified each one against the head rather than copying it.

Smaller things that are not blocking, for the author to weigh:

• The Windows tick polls host and GPU sources one after the other, so a slow GPU query can freeze CPU and memory readings for up to ~20s.
• powershell.exe is spawned by bare name rather than a System32 absolute path.
• The multi-adapter fallback in winperf.rs can be triggered by non-GPU Win32_VideoController rows (RDP / Hyper-V virtual displays), which would drop VRAM total and ADLX readings on an otherwise single-GPU host.
• dock.rs still renders a bare "VRAM 0.0%" for unmeasured VRAM, the same fake-zero the Home hero no longer shows.

I did not build or run this on Windows; this is a read of the code plus the CI results (the required lanes are green).

[volen-silo]

timeout(ADLX_TIMEOUT, task::spawn_blocking(adlx_readings)) only bounds the wait: when it fires it drops the JoinHandle, but a spawn_blocking task cannot be cancelled, so a wedged ADLX/driver call keeps its blocking-pool thread. On a 1s ticker that is one leaked thread per tick for as long as the driver stays wedged.

AGENTS.md section 6 names this exact pattern ("a timeout around spawn_blocking drops only the join handle, so the thread still leaks: give the blocking call its own deadline or cancellation flag"). The comment above also says this mirrors run_script(), but that one is paired with kill_on_drop(true) and really kills the child; nothing here can kill a blocked native thread.

Suggested fix: give adlx_readings its own deadline or cancellation flag, or skip scheduling a new ADLX call while the previous one is still running, and correct the comment either way.

[juhovainio]

Fixed in 8a2ec8a. run_single_flight (winperf.rs) takes the "skip while the previous call is running" option: an in-flight flag is set before spawn_blocking and cleared by a drop guard inside the closure (panic included). A wedged ADLX call now leaks at most the one thread it is already holding, and later ticks report "ADLX query skipped: a previous call is still running" instead of spawning another. The comment no longer claims parity with run_script().

Tests: returns the value and clears the flag; a timed-out call blocks a second call from starting, and calls resume once it returns; a panic clears the flag. Flipping the guard makes the wedged-call test fail.

[volen-silo]

The new Windows connect_and_run has no test: client.rs has no #[cfg(test)] module at all. Its only real logic is mapping gpu.snapshot()'s Result (including the Err branch that builds an empty-GPU Snapshot plus a formatted warning) into what is sent to AppState, and that is inlined in the loop where no test can reach it.

AGENTS.md section 3 asks for platform-gated code to get a test that runs on its gate, and windows-build-and-test already runs the workspace tests on Windows. apply_adlx in winperf.rs does this correctly.

Suggested fix: extract the Result to Snapshot/warnings mapping into a pure function and unit-test success, error, and the warning text.

[juhovainio]

Fixed in 8a2ec8a. The mapping is now build_host_gpu_snapshot in client.rs: a pure function, not cfg(windows), so its tests run on every host including the Windows job. Tests cover success passthrough, and the error path (empty GPU list, host metrics kept, exact warning text, no instances).

[volen-silo]

This hunk only rewords the existing quit-confirm paragraph. The capability this PR adds (the dashboard on Windows showing live GPU/CPU/memory with no daemon running) has no manual-testing walkthrough, and AGENTS.md section 5 asks for docs/manual-testing.md to be updated in the same change when observable behavior changes.

Suggested fix: add a short section: open rocm dash on Windows with no rocmdashd, expected result is live GPU utilization/VRAM/CPU/memory, and what you see for temperature/power/clock on a non-AMD GPU or with no driver DLL (the 0.0/None degrade path).

[juhovainio]

Fixed in 8a2ec8a: new section 4b, "Dashboard Live GPU/CPU/Memory On Windows", in docs/manual-testing.md. It covers rocm dash with no rocmdashd, the "Detecting GPU…" placeholder before the first reading, single- vs multi-adapter VRAM behaviour, and the degrade path when amdadlx64.dll is absent.

[juhovainio]

Addressed review 5749608622 (and the matching automated round on 7afb757) in 8a2ec8a.

Blocking, fixed (details in the inline replies)

• ADLX timeout leak: single-flight guard, at most one stuck thread; comment corrected.
• connect_and_run coverage: result-to-snapshot mapping extracted into a pure helper with success and error tests.
• Manual-testing walkthrough for Windows live telemetry added (section 4b).

Non-blocking, fixed

• dock.rs GPU wall shows VRAM — for unmeasured VRAM instead of 0.0% (two tests, placeholder and measured).
• powershell.exe resolves to %SystemRoot%\System32\WindowsPowerShell\v1.0\powershell.exe when it exists, bare name otherwise (rocm-core is not a dependency of this crate, so the helper is local).
• docs/architecture.md mentions winperf and the Windows in-process use.

Left unchanged on purpose

• Serial host/GPU polling in the Windows tick: the worst case (~20s) needs a wedged powershell or driver; fixing it means splitting the tick into independent sources, which I'd rather do as its own change than grow this PR further.
• Non-GPU Win32_VideoController rows (RDP / Hyper-V) defeating the single-adapter fast path: needs a decision between a name denylist and gating on the perf-counter device count; not changed here.
• adlx alpha pin and the bare-name amdadlx64.dll load: unchanged. Both are deliberate (no other Rust ADLX binding; the crate gives no way to load by absolute path, mitigated by SetDefaultDllDirectories). Flagging for the reviewer to confirm they remain accepted.

Local: cargo fmt, cargo clippy --workspace --all-targets -- -D warnings, tests for rocm-dash-collectors and rocm-dash-tui, and the pre-push hook (full cargo test) all pass. Not run: scripts/smoke_local.py and any Windows-hardware check; the new tests are platform-neutral, so the Windows CI job will exercise them.

[volen-silo]

I re-checked my change request against 8a2ec8a in the code, not just the replies:

• ADLX timeout: run_single_flight sets the in-flight flag before spawn_blocking, clears it from a drop guard inside the closure (so a panic clears it too), and returns early while a previous call is still running. A wedged driver call now holds one thread instead of one per tick, and the single-flight tests cover done, timed-out, skipped and panicked.
• Windows connect_and_run test: the Result to Snapshot mapping is now the pure build_host_gpu_snapshot, not cfg(windows), with tests for the success passthrough and the error path (empty GPU list, host metrics kept, exact warning text, no instances), so they run on the Windows job.
• Manual-testing walkthrough: section 4b covers rocm dash with no rocmdashd, the "Detecting GPU…" placeholder, single versus multi adapter VRAM, and the missing-DLL degrade path.

The commit also took two of the non-blocking notes: powershell.exe resolves under %SystemRoot%\System32 when present (with tests), and the GPU wall shows "VRAM —" instead of "0.0%" (with tests).

Two things still stand between this and a merge, neither in the code I read:

1. The PR now conflicts with main: docs/manual-testing.md does not merge cleanly (I confirmed with a local merge test).
2. There is no CI run on 8a2ec8a yet (GitHub shows a single check on the head, which is normal while a PR has conflicts), so none of the fixes has run through the required lanes, including the Windows job that now runs the new tests.

My change request is addressed on the code. Once the branch is rebased and CI is green on the new head, this looks safe to approve after final validation. That is what a read of the diff can show; I did not build or run it, and this is not itself an approval.

Addressed in 8a2ec8a

[tomastola]

Three things changed since my review at 2c5a608e:

• dbcfa7cc answers my last review.
• 7afb757e merges main.
• 8a2ec8aa answers the automated review and @volen-silo's change request on 7afb757.

What I raised last time: both fixed.

• Duplicate dash-38: fixed. The badge scenario is now dash-39 at dash.feature:684, and the doc comment at dash_steps.rs:177 is updated to match.
• Hero VRAM %: fixed. vram_used_pct now counts only GPUs with a known total, for both the used and the total side. The new test vram_used_pct_ignores_gpus_with_unknown_total uses my example and checks for 50.0%.

What 8a2ec8aa does for the other reviews:

• ADLX thread leak: run_single_flight will not start a new ADLX call while a timed-out one is still running. A wedged driver call now ties up at most one thread instead of one per tick, and the misleading comment is fixed. The flag is cleared by a guard inside the closure, so it also clears if the call panics. Three tests cover success, a wedged call followed by recovery, and a panic. This addresses the finding.
• Untested Windows connect_and_run: the logic that turns a tick's result into a Snapshot is now build_host_gpu_snapshot, a function that runs on every OS. Its success and error paths are tested. Addressed.
• Manual-testing walkthrough: added as §4b, along with the docs/architecture.md update. Addressed, but see the conflict below.
• powershell.exe by bare name: it now prefers %SystemRoot%\System32\…\powershell.exe when that file exists, which is the same shape as rocm-core's helper. Addressed.
• GPU-wall "VRAM 0.0%": it now shows VRAM — when the total is unknown, with a test for each case. Addressed.

Blocking: CI did not run on 8a2ec8aa

The PR is CONFLICTING with main (c39ee0c), so GitHub ran no workflows on the head. The only check present is readthedocs. None of the fixes above has been built or tested on any OS yet.

• Conflict: docs/manual-testing.md. The new ### 4b. Dashboard Live GPU/CPU/Memory On Windows and #703's new ## 5. Reuse Is Exempt From The No-GPU Refusal both land just before the old ## 5. ComfyUI Verification.
• Fix: keep both sections and renumber the ones after them.

Blocking: Windows clippy will fail on client.rs

• Where: crates/rocm-dash-tui/src/client.rs. The new #[cfg(test)] mod tests (line 229) sits before the #[cfg(windows)] async fn connect_and_run (line 287).
• What fails: on Linux that function is compiled out, so the Linux clippy job passes. On Windows the function exists after the test module, so clippy::items_after_test_module fires. windows-build-and-test runs cargo clippy --workspace --all-targets -- -D warnings, so it will turn red as soon as CI runs.
• How I checked: I copied this file's layout (a cfg(unix) function, a helper, #[cfg(test)] mod tests, then a cfg(windows) function) into a small crate with the workspace's clippy lint levels. It fails with error: items after a test module for --target x86_64-pc-windows-gnu and is clean on Linux (clippy 1.96).
• Fix: move mod tests to the end of client.rs, after the Windows connect_and_run.

Non-blocking

• run_single_flight reports a task that was cancelled rather than panicked as Panicked, because Ok(Err(_)) covers both. This only happens at runtime shutdown, so the misleading warning text would almost never be seen.
• Two non-blocking points from the earlier review are still open and are the author's call:
  • The Windows tick still polls the host and GPU sources one after the other.
  • Extra non-GPU Win32_VideoController rows (RDP or Hyper-V virtual displays) still push a single-GPU host onto the multi-adapter path.

Once the conflict is resolved, the test module is moved, and windows-build-and-test and Test (affected crates) pass, I have nothing blocking.

[juhovainio]

Addressed review 5749846885 in cbb2c7e.

Blocking, fixed

• Conflict with main: merged origin/main; in docs/manual-testing.md both sections are kept (§4b Windows dashboard telemetry, then main's §5 reuse-exemption and the renumbered §6 onward). No conflict markers remain.
• Windows clippy::items_after_test_module: mod tests in client.rs now follows the cfg(windows) connect_and_run, so it is the last item in the file. I have no Windows target locally, so this is not verified on Windows; windows-build-and-test is the check.

Left unchanged

• run_single_flight reports a cancelled task as Panicked: only reachable at runtime shutdown, so I left it.
• Serial host/GPU polling and non-GPU Win32_VideoController rows: still the author's call, as before.

Local: cargo fmt, cargo clippy --workspace --all-targets -- -D warnings and the rocm-dash-* tests pass. The pre-push hook failed once on comfyui::tests::reinstall_is_refused_while_comfyui_starts_until_it_is_stopped (ComfyUI code from main, not touched here); it passes in isolation, so the push used --no-verify with the operator's approval. CI on this head is the authoritative run.

Addressed in cbb2c7e

[nowycondro]

🤖 Automated review (Claude Code) · cbb2c7e

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 (26 files, 1885+/73- across 27 commits, base c39ee0c6 to head cbb2c7e4).
Implements ROCMAI-564 ("Device metrics for TUI (GPU/memory) on Windows"), which is still "In Progress" and scopes exactly this: identify a Windows-side GPU/VRAM metrics source, wire it into the dashboard, validate on Strix Halo. The PR does not claim to close the ticket, and its description's own account of the diff matches the diff closely — no scope mismatch found.

Blocking

• Hero title flickers between "Detecting GPU…" and "Unknown GPU" on every reconnect retry before the first snapshot lands — crates/rocm-dash-tui/src/ui/tabs/home.rs:85-89, crates/rocm-dash-tui/src/client.rs:101-121, crates/rocm-dash-tui/src/app/event_loop.rs:443,447.
  gpu_telemetry_pending is true only while conn is not Disconnected. But client.rs's spawn() loop (shared by both the unix and Windows arms) runs Connecting -> connect_and_run().await -> Disconnected{..} -> backoff sleep -> Connecting -> ... forever whenever the daemon is unreachable. event_loop.rs applies each message directly into state.conn (and Disconnected wipes state.latest), so every retry cycle before the first snapshot flips the hero between the two placeholder strings — a real, frame-rendered flicker, not an internal transient. This reproduces on any platform (Linux included) whenever the daemon takes any time to come up, not only on Windows. The function's own doc comment ("a Disconnected before any snapshot ... the reconnect loop never delivers one") is factually wrong given the loop it describes: the reconnect loop does keep retrying and can still deliver a snapshot later. Existing tests cover the static Disconnected state and the already-had-a-snapshot-then-disconnected case, but not this cycling-before-first-data path.
  Confidence 90 · logic · Fix: make the pending/placeholder decision sticky across the Connecting↔Disconnected cycling (e.g. track "ever reached the backoff-exhausted/given-up state" separately from "currently mid-retry"), rather than re-deriving it from the instantaneous conn variant on every message.

• Windows in-process tick lets a slow GPU collection stall fast CPU/memory delivery and mis-stamps the snapshot's timestamp — crates/rocm-dash-tui/src/client.rs:261 (build_host_gpu_snapshot(host.tick(), gpu.snapshot().await, chrono::Utc::now())).
  host.tick() samples CPU/memory synchronously, but the combined snapshot is only sent — and its timestamp only stamped — after gpu.snapshot().await returns, which can take up to ~20s worst case (WinPerfCollector's own 15s script timeout plus up to 5s ADLX single-flight). The PR's own commits document that spawn latency routinely approached the old 5s timeout on real hardware (hence the bump to 15s), so this isn't a rare pathological case: whenever GPU collection is slow, CPU/memory updates visibly stall for seconds even though HostCollector itself is fast, and the snapshot's timestamp no longer reflects when its host metrics were actually sampled. This is the exact pattern this repo's own AGENTS.md guardrail names and asks new code to avoid: "do not let a slow source block the fast ones sharing its tick."
  Confidence 85 · architectural · Fix: run host.tick() and gpu.snapshot() concurrently (e.g. tokio::join!), or at least capture the timestamp before awaiting the GPU collector rather than after.

• CI: "E2E tests (Strix Halo, Windows, current)" is failing at this head (conclusion failure; scenario runtime-resolve-canonical-release, which the suite's own reconciliation step labels "a regression," not an expected xfail). The failing scenario is about ROCm-10 pip-index/runtime-resolution behavior and is not obviously related to this PR's GPU-telemetry-collector changes, but it is a red check on the reviewed commit, and this repo's own AGENTS.md §12 treats a red check as "not ready" until it's resolved or shown to be an unrelated/infrastructure flake with evidence.
  Confidence 100 (observed directly in the check run's own log) · Fix: investigate and either fix, or document the flake with evidence per §12 before merge.

Non-blocking

• Two stale doc comments still claim the Windows client "never connects at all", now false since this PR's own Windows arm connects and streams telemetry every tick (it just never sends an instance snapshot) — crates/rocm-dash-tui/src/ui/quit_confirm.rs:144-145 and crates/rocm-dash-tui/src/app/types.rs:126. Three near-identical sibling comments elsewhere in this same PR (app/quit_confirm.rs, registry_probe.rs, apps/rocm/src/dash.rs) were correctly reworded to "connects and streams ... but never sends an instance snapshot"; these two were missed.
  Confidence 85 · mechanical · Fix: reword to match the sibling comments.

• dock.rs's context_rail still fabricates "peak util 0.0%" for the exact "connected, legitimately zero GPUs" state this PR fixed in home.rs — crates/rocm-dash-tui/src/ui/dock.rs:238-247 (s.gpus.iter()...fold(0.0, f64::max) on an empty iterator yields 0.0, shown as a real reading rather than "no telemetry"). Pre-existing code, not touched by this diff, but directly on-point with the bug class this PR just fixed elsewhere — now the two widgets disagree about how to render the same state.
  Confidence 70 · logic · Fix: route context_rail through the same gpu_data_unavailable-style distinction home.rs now uses.

• Locale-dependent decimal-separator parsing can silently zero out GPU readings with no warning — crates/rocm-dash-collectors/src/winperf.rs, parse_metrics's value.trim().parse().unwrap_or(0.0) calls, fed by WINPERF_SCRIPT's $($_.CookedValue) interpolation. PowerShell's string interpolation calls ToString() with the current culture, so a non-English-decimal Windows locale (e.g. de-DE's comma separator) would make every numeric sample fail to parse and silently default to 0.0. counter_samples_missing_warning only fires when metrics is empty, not when values are present but all-zero, so this failure mode produces no warning — unlike the sibling locale problem (English-only counter path names) that this same PR explicitly adds a warning for.
  Confidence 70 · logic · Fix: parse with an explicit invariant-culture assumption in the script (e.g. force . via -replace or a culture-invariant format string) and/or detect and warn on a parse failure instead of silently defaulting to 0.0.

• Multi-real-adapter hosts don't get the phantom-LUID collapse parse_metrics applies for the single-adapter case — crates/rocm-dash-collectors/src/winperf.rs (parse_metrics, the if let [adapter] = adapters.as_slice() branch vs. its fallback). The PR's own text explicitly scopes vram_total_mb's degrade-to-0 to the multi-adapter case, but a genuine 2+ real-GPU Windows host exhibiting the same stale/phantom-LUID behavior fixed for the single-adapter case would still show an inflated device count (not just an inflated/zeroed VRAM total).
  Confidence 70 · logic · Fix: at minimum, extend the doc comment to disclaim device-count accuracy (not just VRAM total) for multi-adapter hosts, or extend the collapse heuristic to use the WMI adapter count as a target count for any adapter cardinality, not just exactly one.

• adapters.len() == 0 falls into the ">1 real adapter" code path — crates/rocm-dash-collectors/src/winperf.rs, parse_metrics (if let [adapter] = adapters.as_slice() only matches length 1; length 0 falls through to the branch whose comment says "more than one real adapter"). If Get-Counter still yields samples while Win32_VideoController returns nothing (e.g. a WMI query failure), synthetic devices are built from raw LUIDs with no corroborating adapter inventory and no warning. Narrow in practice; no test covers this combination.
  Confidence 70 · logic · Fix: give the zero-adapter case (adapters.is_empty() && !seen.is_empty()) its own explicit, commented branch rather than letting it fall through the ">1" comment's path.

Decisions for the author

• Parallel message type (ClientMsg::HostGpuSnapshot) instead of reusing Event::Snapshot — tradeoff. Well-reasoned in the PR text (avoids permanently blinding the quit-confirm prompt via has_received_snapshot latching on an always-empty instances), but it creates two near-duplicate "a snapshot arrived" concepts the codebase must keep in sync going forward, as the stale-comment finding above already shows slipping. Worth confirming this is preferred long-term over, say, a platform flag on the existing snapshot path.
• ADLX's amdadlx64.dll is loaded by bare name inside the third-party adlx crate — tradeoff. Mitigated with SetDefaultDllDirectories(LOAD_LIBRARY_SEARCH_SYSTEM32) as defense-in-depth (with its return value checked, falling back gracefully), but this doesn't fully satisfy this repo's "DLLs load by absolute path" guardrail since the actual LoadLibrary call lives inside the external crate, outside this PR's control.
• adlx = "0.0.0-alpha.1" — tradeoff. An alpha-versioned third-party crate is now a dependency for production temperature/power/clock readings; well-mitigated by graceful degradation to 0.0/None on any failure, but worth the author explicitly confirming this is an acceptable risk given there's apparently no alternative AMD-official binding today.
• WMI_ADAPTER_RAM_CAP_THRESHOLD only catches near-4GiB sentinel values, not general 32-bit modulo wraparound (e.g. a 6GiB card that wrapped to ~2GiB would pass silently as a plausible-but-wrong total) — non-blocking-improvement. Explicitly scoped as best-effort in the code's own comment, and mitigated for AMD GPUs by preferring ADLX's own VRAM query when available.

Positive signals

• winperf.rs's test suite is unusually disciplined: nearly every test's comment states exactly which mutation or regression it exists to catch (sum-vs-max aggregation, single- vs. multi-adapter ADLX correlation, index-based-correlation never silently resuming), matching this repo's own "a new test is shown to fail when its target is broken" standard more rigorously than most PRs attempt.
• The ADLX single-flight guard (run_single_flight/ClearOnDrop) correctly bounds an uncancellable, wedged blocking-pool thread to at most one leaked thread total rather than one per tick, including clearing correctly on panic — a well-reasoned, well-tested answer to a real FFI hazard.
• Device identity is consistently correlated by LUID, never by list index, between WMI and perf-counter data — exactly this repo's own "do not pair lists by index when they share an id" guardrail — with a dedicated test (multiple_adapters_never_trust_adlx_even_when_reading_count_matches_adapter_count) asserting the index-based-correlation failure mode can't silently regress back in.

Deployment notes

• New Windows-only third-party dependencies adlx (alpha-versioned) and windows-sys, correctly scoped under [target.'cfg(windows)'.dependencies] — no Linux build impact.
• crates/rocm-dash-tui gains a new path dependency on crates/rocm-dash-collectors; the xtask/src/crate_edges.rs allowlist edit is minimal and correctly directional.
• The Windows collector path itself has no e2e coverage (acknowledged in the PR text — the e2e-cucumber PTY driver is Linux-only for now), so regressions in the #[cfg(windows)] path can only be caught by winperf.rs's unit tests and manual hardware testing, not CI.

What this covered

Every file in the diff, read completely at the PR head, plus the surrounding cross-platform context needed to judge each (the unix connect_and_run arm, AppState/ConnState/event_loop.rs plumbing, reconnect.rs's backoff, theme.rs/panel.rs/widgets.rs shared UI helpers, rocm-dash-daemon's own ticker, and this repo's AGENTS.md in full). Base c39ee0c6dab07d97a955a158ce4aa1d0fa6640a1 to head cbb2c7e4fcd954fa5badd59cb0b7fc8c2e910b34, split into four independent, blind fan-out passes grouped by coupling (the new winperf.rs collector; the TUI client/event-loop wiring; the home/dock/header/instances rendering code; tests+docs+misc), then consolidated and re-verified directly against the source by the driver, including the two highest-value findings above (both independently re-traced through the actual code rather than taken on a sub-pass's word).

Did not run: cargo test/cargo clippy (no Rust toolchain available in this review sandbox) — CI's own green clippy, Test (affected crates), and windows-build-and-test runs were read and trusted in their place for everything except the one failing Windows E2E job noted above. History pass: reviewed the commit log and found this PR already went through several rounds of real-hardware bug fixes and reviewer feedback (visible in commit messages); nothing in that history contradicts the findings above. Prior-changes pass: not run (no visibility into this review's own earlier rounds by design). Not reconciled against this PR's existing review threads or comments.

---

🤖 Generated with Claude Code

[tomastola]

Since my Changes-requested review on 8a2ec8aa, there is one new commit, cbb2c7e4. It merges main (c39ee0c) and fixes both of my blockers.

Blocker 1, merge conflict with no CI on the head: fixed.

• docs/manual-testing.md keeps both new sections: §4b (live dashboard metrics on Windows) and #703's "Reuse Is Exempt From The No-GPU Refusal", now §5. ComfyUI and the sections after it are renumbered to §6 through §10.
• The doc's cross-references only point to sections 1–3, so the renumbering breaks none of them.
• The PR is mergeable again, and CI ran on the new head. windows-build-and-test, Windows build and test (debug), Windows release build and lifecycle, Test (affected crates), clippy, Coverage (rocm-dash crates, ratcheted), Sphinx docs build (-W) and Markdown links resolve (lychee) all pass.

Blocker 2, Windows clippy items_after_test_module in client.rs: fixed.

• #[cfg(test)] mod tests now comes after the #[cfg(windows)] connect_and_run (line 272 against line 237), as the last item in the file.
• The move changes nothing else: sorted line by line, the file is identical to the 8a2ec8aa version.
• windows-build-and-test runs clippy with -D warnings, and it passed.

Merge commit: the only manual edits are the doc conflict resolution and the test-module move. README.md and docs/architecture.md merged without conflicts and have no hand-edited lines.

The red self-hosted E2E lanes are not from this PR.

• E2E tests (Strix Halo, Windows, current) fails serve-01, serve-02, runtime-02 and runtime-09. main fails the same four on this lane (for example the run at d7faa84e), and none of them is a dashboard scenario.
• The two Strix Halo, Ubuntu lanes were cancelled at the 45-minute timeout, which is happening on every branch, main included.
• MI300X and W7900 were still running when I checked.

Non-blocking, for you to weigh:

• run_single_flight reports a cancelled call as Panicked, because Ok(Err(_)) covers both. This only happens at runtime shutdown, so the misleading warning text would almost never be seen.
• The Windows tick still polls the host and GPU sources one after the other, so a slow GPU query can freeze CPU and memory readings for up to ~20s.
• Extra non-GPU Win32_VideoController rows (RDP or Hyper-V virtual displays) still push a single-GPU host onto the multi-adapter path, which drops VRAM total and the ADLX readings.

Nothing blocking from me.