[r0x0r]

Summary

amd-smi launched against a kernel fusion driver in a bad state blocks in uninterruptible kernel sleep. No signal escapes that state, so rocm serve hangs and survives both Ctrl-C and SIGKILL; a timeout does not help, because killing a D-state child is a no-op. The repo documents this and calls the /dev/kfd pre-flight mandatory — but three of the four production launch sites skipped it:

Launch site	Pre-flight before	Timeout before
rocm-dash-collectors collector run	yes	yes
rocmd capture_amd_smi_json	no	yes
rocm gpu_vram_usage_amd_smi	no	none
rocm detect_gpu_count	no	none

The two unguarded, untimed ones sit on the normal rocm serve path for any run that is not CPU-only, so rocm serve --gpu auto reached at least one launch with neither protection.

This gives the pre-flight one definition in rocm-core and routes the three unguarded sites through a single guarded launcher, so a future launch site inherits the guard rather than having to remember it.

What is actually in this diff

• crates/rocm-core/src/amd_smi.rs — the gate and the launcher. amd_smi_json(args, timeout) is the one supported way for a synchronous caller to start amd-smi; it runs the pre-flight (KFD_DEVICE, preflight_kfd_device, kfd_readable/kfd_readable_with, device_readable) and only then hands the Command to rocm_core::process::run_with_timeout. Re-exported as rocm_core::amd_smi_json.
• apps/rocm/src/gpu_policy.rs — gpu_vram_usage_amd_smi and detect_gpu_count rewritten to call it, with a new AMD_SMI_SERVE_PROBE_TIMEOUT (10s) and its latency rationale.
• apps/rocmd/src/common.rs — capture_amd_smi_json becomes a one-line call into the same helper with the watcher's own 2s budget.
• crates/rocm-core/src/lib.rs — command_failure_detail_or_else, so the stderr-then-stdout precedence lives in one place while the amd-smi site supplies its own both-blank fallback (exit status <code>).
• crates/rocm-core/src/process.rs — MAX_QUOTED_OUTPUT_CHARS becomes pub(crate) so the amd-smi failure quote shares it. That constant's own doc named this quote (Converge the remaining unbounded child-capture helpers onto run_with_timeout #623) as the one it did not yet cover; the two now converge instead of drifting.
• Docs — hardware_root.rs's module contract (two pre-flights, both keeping the real device's veto) and docs/architecture.md's amd_smi.rs / gpu_policy.rs entries.
• E2E — the e2e-test-hooks path seam ROCM_E2E_KFD_DEVICE and scenario examine-31.

What is not in this diff: the pipe-draining / zombie-reaping rewrite of run_command_with_timeout. That landed separately as #526 (ROCMAI-448) and is already on main; earlier revisions of this description claimed it, wrongly.

The gate is Linux-only, deliberately

/dev/kfd exists on no other platform. amd-smi.exe is a supported Windows binary (managed_sdk_tool_path resolves it) and there is a live e2e-gpu-strix-windows lane, so a check that merely failed to find the device would have silently disabled GPU detection on every Windows host. Off Linux the pre-flight waves the launch through, and a test pins that.

WSL compiles as Linux and has no /dev/kfd either, so it needs a runtime wave-through. The condition is is_wsl_host() && has_usable_amd_gpu() — the same conjunction apps/rocm/src/dash.rs::gpu_reachable_for_preflight feeds the dashboard — not bare WSL identity, because a WSL host whose ROCDXG plumbing is not ready is one where amd-smi still hangs.

The probe cannot hang, and cannot pile up

The gate's open is not assumed to return promptly. It runs on a probe thread bounded by KFD_OPEN_TIMEOUT (2s); an open that has not returned by then reads as not readable, so the gate declines rather than inheriting the stall it exists to prevent.

A probe that overruns leaves its thread parked in open, possibly forever. Those threads are capped: MAX_INFLIGHT_KFD_PROBES (8) slots, claimed with a fetch_update and released by a guard moved into the probe thread, so a slot frees when the kernel finally returns rather than when the caller gives up. With the budget spent, a probe answers false without opening anything.

Both of the remaining failure branches — budget exhausted, and thread spawn failed — decline rather than falling back to a direct blocking open on the caller's thread. The states in which this process runs out of threads are not independent of the state the gate refuses: the way the budget is exhausted here is one parked open per poll against a wedged device, so a fallback would fire exactly when the device is most dangerous, performing the unbounded open the bound exists to prevent, on a thread with nothing left to kill. A refusal is recoverable and visible; a hang is not.

Latency

KFD_OPEN_TIMEOUT is additive to each call's timeout, not inside it:

Path	Probes	Worst case
rocm serve --gpu auto	2 sequential	2 × (2s + 10s) = ~24s
--model readiness (host_accelerator_memory)	1	2s + 10s = ~12s
rocmd telemetry poll	1	2s + 2s = ~4s

Earlier revisions quoted 10s and ~20s, which omitted the pre-flight. The two bounds are rarely both paid — a host whose open hangs is a host that then declines and never spends the amd-smi budget — but a ceiling that assumes the good case is not a ceiling. Before this change both serve probes called .output() with no timeout at all, so ~24s is the ceiling replacing "forever".

They are deliberately not parallelised: detect_gpu_count runs inside select_gpu_indices_under_launch_lock precisely so the cpu-only and pinned paths never pay for the subprocess, and hoisting it out would make every rocm serve --cpu-only spawn an amd-smi it has no use for.

Coverage of the user-observable change (AGENTS.md §3)

On a Linux host that cannot open /dev/kfd, the probes now decline to launch where before they attempted the spawn; GPU auto-selection degrades to service state alone, which is what it already does whenever amd-smi is unavailable.

examine-31 (@id:examine-telemetry-declines-an-unopenable-kfd-device) covers this end to end on rocmd's bridge snapshot — the one surface that renders the decline as JSON in a single command. It is non-vacuous twice over: the stub amd-smi would have answered the probe perfectly and records that it ran, so the marker's absence is what distinguishes "refused before the spawn" from "spawned and failed"; and the note must name the device this run pointed the pre-flight at, so a build that ignored the seam would decline naming /dev/kfd and fail rather than passing on the runner's own missing device.

Reaching either branch needs the e2e-test-hooks seam ROCM_E2E_KFD_DEVICE, which moves the device the gate opens rather than skipping the gate. ROCM_CLI_TEST_HOST_ROOT cannot substitute: the gate keeps the real node's veto on purpose, so a simulated root can make it decline but never allow.

Still uncovered by a scenario: rocm serve's and --model's own call sites. Both discard the error with .ok()? by design, so on a GPU-less lane the declined branch is indistinguishable from having no GPU. Named here rather than left silent, and also recorded as a note at the end of tests/e2e-cucumber/features/model_serving.feature.

The fourth site: the dash collector

Closed in 741852a8 (an earlier draft of this section argued for leaving it; that reasoning no longer applies). The collector keeps its own pre-flight decision (preflight_passes) because it gates an async launch and the sync amd_smi_json chokepoint does not fit, but its device_accessible is no longer a bare open() — it calls rocm_core::device_readable_bounded, the same bounded, budget-capped probe rocm-core's own gate uses. The bound, the budget and the reasoning about why both failure branches decline now live in exactly one place, so the one launch site outside the sync chokepoint cannot drift away from them.

That needed the rocm-dash-collectors -> rocm-core edge the crate_edges layering guard exists to force review on. It is taken deliberately and recorded as an allowlist entry in xtask/src/crate_edges.rs, with docs/architecture.md updated to say so. The alternative raised in review — a dependency-free leaf crate hosting just the bounded-open primitive — would have cost two reviewed edges instead of one for the same sharing.

The site is separately guarded as before, too: #375 ("fix(dash): detect amd-smi through WSL DXG", EAI-8450, on main as d37cfbab) replaced its bare kfd_accessible() with preflight_passes(...), fed by rocm_core::has_usable_amd_gpu() from apps/rocm/src/dash.rs.

Tests

Nine unit tests in crates/rocm-core/src/amd_smi.rs, each confirmed non-vacuous by reverting the behaviour it pins:

• the_kfd_gate_stops_amd_smi_before_it_is_spawned — a fake amd-smi leaves a marker file; the gate must stop the spawn, not report a failure afterwards. The same fixture is re-run against a readable device to prove the fake would otherwise have executed.
• the_kfd_probe_declines_a_device_whose_open_blocks_instead_of_hanging — a FIFO with no writer really does block in open; the probe must convert that into a decline inside its bound. Watched from a second thread with a looser deadline, so a dropped bound fails with a message instead of hanging the binary.
• the_kfd_probe_budget_declines_instead_of_parking_threads_without_bound — new this round. Spends a one-slot budget on a parked FIFO probe, then points the capped probe at an ordinary readable file: an unbounded probe, or one that fell back to a blocking open, answers true; only a probe that refuses on an exhausted budget answers false. A control with a free slot must still answer true. Mutation-checked on the MI300X box — restoring the old blocking-open fallback fails it with exactly the escalation message.
• the_kfd_preflight_waves_through_hosts_that_have_no_kfd (non-Linux) — guards the Windows lane.
• the_kfd_preflight_waves_through_wsl_hosts_only_when_the_gpu_is_reachable — pins the conjunction, with the unready-WSL case as the discriminator and bare metal as the control.
• the_kfd_gate_honours_a_simulated_root_but_keeps_the_real_device_veto — a simulated root may hide the device, never invent one.
• the_public_amd_smi_entry_point_gates_on_the_real_kfd_device — the production wrapper gates on KFD_DEVICE itself, which the injected helpers cannot show.
• the_e2e_kfd_path_seam_moves_the_gate_without_disabling_it — the seam relocates the probe instead of skipping it.
• a_failing_amd_smi_cannot_paste_its_whole_output_into_the_error — a 200K-char payload must not reach the error (and from there rocmd's snapshot note and the rewrite-the-whole-file JSONL logs, which is how a 151K-char log line was produced).

Test plan

Run on the MI300X Linux dev box against main + this branch's net diff; macOS cannot build this workspace (engines/lemonade uses st_mode as u32).

• cargo build --workspace --all-targets — exit 0
• cargo clippy --workspace --all-targets -- -D warnings — exit 0
• cargo clippy -p e2e-cucumber --test e2e -- -D warnings — exit 0
• cargo clippy --workspace --all-targets --features rocm/e2e-test-hooks -- -D warnings — exit 0
• cargo test --workspace --all-targets --exclude e2e-cucumber — exit 0, 0 failed
• cargo test -p rocm-core --lib --features e2e-test-hooks -- amd_smi:: — 11 passed, 0 failed
• cargo fmt --all --check, cargo xtask check-crate-edges
• Windows lane via CI (cannot be exercised locally)

Re-run on the same box for the docs/scope round (472efacc), against main at 11a60691 + this branch's net diff:

• cargo build --workspace --all-targets — exit 0
• cargo clippy --workspace --all-targets -- -D warnings — exit 0
• cargo clippy -p e2e-cucumber --test e2e -- -D warnings — exit 0
• cargo clippy --workspace --all-targets --features rocm/e2e-test-hooks -- -D warnings — exit 0
• cargo test --workspace --all-targets --exclude e2e-cucumber — exit 0, 0 failed
• cargo test -p rocm-core --lib --features e2e-test-hooks -- amd_smi:: — 11 passed, 0 failed
• cargo fmt --all -- --check — exit 0; cargo xtask check-crate-edges — exit 0
• cargo check -p rocm-core --all-targets on macOS (both with and without e2e-test-hooks) — exit 0, so the cfg(not(target_os = "linux")) branch still builds
• cargo xtask e2e -- -n 'examine-31 '
• No amd-smi/rocm-smi and no GPU telemetry on that box, so nothing here is real-device verification.

[siloteemu]

🔴 Automated review · pr-review-watcher · 5a29292

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.

Summary

The PR consolidates the /dev/kfd pre-flight and the timeout helper into the core crate and routes three of the four amd-smi launch sites through one guarded launcher; outcome: Needs work. Reviewed at check-run conclusions 21 success, 1 failure, 4 pending, 1 skipped. Verified by reading the full commit messages and a structural sweep of the source and workflow files (no build, no test run, no suite of any kind was executed here): the four-row launch-site table is accurate at the base tip and there is no fifth site; the three unguarded sites do now route through amd_smi_json; the off-Linux wave-through test is cfg(not(target_os = "linux")) and a Windows lane does run cargo test --workspace --all-targets whenever Rust files change, and the Linux-gated test runs on the Linux lane, so neither discriminating test is stranded on a platform nothing exercises; each of the three new tests dies under a separate single-branch mutation (removing the bail! gate, or either one of the two pipe drains, or the non-Linux kfd_readable stub) rather than only under a wholesale revert; no caller anywhere parses the helper's error text and none of the two remaining callers loses meaningful diagnostics from the dropped partial output; the fourth site's exclusion and its ungated check are as described. Blocking: 1 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

crates/rocm-core/src/lib.rs:7954 and crates/rocm-core/src/lib.rs:7980 — the gate treats WSL as bare-metal Linux, which silently disables amd-smi there. The comment above the constant asserts the device node is "Linux-only; no other platform exposes one", and the commit message generalises that to "/dev/kfd exists nowhere else" — but this repo documents in docs/wsl.md:175 and in crates/rocm-core/src/examine.rs:375 that WSL reaches the GPU through a different device and has no /dev/kfd at all. WSL is target_os = "linux", so it compiles the real-open branch, the open always fails, and amd_smi_json unconditionally declines every launch. That silently turns off all three newly routed sites on that platform: gpu_vram_usage and detect_gpu_count on the ordinary serve path (apps/rocm/src/main.rs:6028 and apps/rocm/src/main.rs:6039, reached on any run that is not CPU-only), and the daemon bridge snapshot (apps/rocmd/src/lib.rs:1470). This is the same failure the PR reasons carefully about for Windows, missed on the adjacent supported platform — AGENTS.md lists WSL as supported, the repo ships a WSL-specific amd-smi, there is a live WSL GPU lane in .github/workflows/e2e-selfhosted.yml:696, and tests/e2e-cucumber/features/model_serving.feature:227 documents a scenario whose outcome turns directly on whether detect_gpu_count() answers with a count or with nothing. It blocks because the regression is silent (both serve probes swallow the error with .ok()?), affects a documented platform with hardware coverage, and neither a test nor a line of the PR text addresses it. Concrete fix: wave WSL through in the Linux branch — the crate already exports is_wsl_host() (crates/rocm-core/src/lib.rs:2510), so if is_wsl_host() { return true; } at the top of the cfg(target_os = "linux") kfd_readable restores the old behaviour there; correct the constant's comment and the commit message's "nowhere else" claim at the same time, and add a test pinning the WSL wave-through the way the off-Linux one is pinned.

Non-blocking

• crates/rocm-core/src/lib.rs:7998 — the doc on amd_smi_json describes the helper as one "which kills the child and then waits for it", which is precisely the behaviour this same commit removed; it contradicts the helper's own doc sixty lines below.
• crates/rocm-core/src/lib.rs:7975 — amd_smi_preflight_ok is exported, marked must-use and documented as the gate, but no production code calls it: amd_smi_json_with calls kfd_readable directly, and the only reference outside doc comments is one test.
• crates/rocm-core/src/lib.rs:8093 — the timeout path kills without reaping and never joins the drain threads, so each timeout leaves a zombie plus two live threads behind; a detached thread that just waits on the child would reap it without ever blocking the caller.
• crates/rocm-core/src/lib.rs:8064 — nothing tests the timeout branch itself, which is the riskiest new code; a child sleeping thirty seconds against a one-second timeout, asserting the call returns an error promptly, would pin the "returns rather than joining" promise the doc makes.
• AGENTS.md section 3 requires either a Gherkin scenario for changed user-observable behaviour or an explicit statement of why none is needed; serve's GPU auto-selection behaviour changes here and neither appears.

[siloteemu]

🔴 Automated review · pr-review-watcher · 5a29292

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.

Summary

The PR consolidates the /dev/kfd pre-flight and the timeout helper into the core crate and routes three of the four amd-smi launch sites through one guarded launcher; outcome: Needs work. Reviewed at check-run conclusions 21 success, 1 failure, 4 pending, 1 skipped. Verified by reading the full commit messages and a structural sweep of the source and workflow files (no build, no test run, no suite of any kind was executed here): the four-row launch-site table is accurate at the base tip and there is no fifth site; the three unguarded sites do now route through amd_smi_json; the off-Linux wave-through test is cfg(not(target_os = "linux")) and a Windows lane does run cargo test --workspace --all-targets whenever Rust files change, and the Linux-gated test runs on the Linux lane, so neither discriminating test is stranded on a platform nothing exercises; each of the three new tests dies under a separate single-branch mutation (removing the bail! gate, or either one of the two pipe drains, or the non-Linux kfd_readable stub) rather than only under a wholesale revert; no caller anywhere parses the helper's error text and none of the two remaining callers loses meaningful diagnostics from the dropped partial output; the fourth site's exclusion and its ungated check are as described. Blocking: 1 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

crates/rocm-core/src/lib.rs:7954 and crates/rocm-core/src/lib.rs:7980 — the gate treats WSL as bare-metal Linux, which silently disables amd-smi there. The comment above the constant asserts the device node is "Linux-only; no other platform exposes one", and the commit message generalises that to "/dev/kfd exists nowhere else" — but this repo documents in docs/wsl.md:175 and in crates/rocm-core/src/examine.rs:375 that WSL reaches the GPU through a different device and has no /dev/kfd at all. WSL is target_os = "linux", so it compiles the real-open branch, the open always fails, and amd_smi_json unconditionally declines every launch. That silently turns off all three newly routed sites on that platform: gpu_vram_usage and detect_gpu_count on the ordinary serve path (apps/rocm/src/main.rs:6028 and apps/rocm/src/main.rs:6039, reached on any run that is not CPU-only), and the daemon bridge snapshot (apps/rocmd/src/lib.rs:1470). This is the same failure the PR reasons carefully about for Windows, missed on the adjacent supported platform — AGENTS.md lists WSL as supported, the repo ships a WSL-specific amd-smi, there is a live WSL GPU lane in .github/workflows/e2e-selfhosted.yml:696, and tests/e2e-cucumber/features/model_serving.feature:227 documents a scenario whose outcome turns directly on whether detect_gpu_count() answers with a count or with nothing. It blocks because the regression is silent (both serve probes swallow the error with .ok()?), affects a documented platform with hardware coverage, and neither a test nor a line of the PR text addresses it. Concrete fix: wave WSL through in the Linux branch — the crate already exports is_wsl_host() (crates/rocm-core/src/lib.rs:2510), so if is_wsl_host() { return true; } at the top of the cfg(target_os = "linux") kfd_readable restores the old behaviour there; correct the constant's comment and the commit message's "nowhere else" claim at the same time, and add a test pinning the WSL wave-through the way the off-Linux one is pinned.

Non-blocking

• crates/rocm-core/src/lib.rs:7998 — the doc on amd_smi_json describes the helper as one "which kills the child and then waits for it", which is precisely the behaviour this same commit removed; it contradicts the helper's own doc sixty lines below.
• crates/rocm-core/src/lib.rs:7975 — amd_smi_preflight_ok is exported, marked must-use and documented as the gate, but no production code calls it: amd_smi_json_with calls kfd_readable directly, and the only reference outside doc comments is one test.
• crates/rocm-core/src/lib.rs:8093 — the timeout path kills without reaping and never joins the drain threads, so each timeout leaves a zombie plus two live threads behind; a detached thread that just waits on the child would reap it without ever blocking the caller.
• crates/rocm-core/src/lib.rs:8064 — nothing tests the timeout branch itself, which is the riskiest new code; a child sleeping thirty seconds against a one-second timeout, asserting the call returns an error promptly, would pin the "returns rather than joining" promise the doc makes.
• AGENTS.md section 3 requires either a Gherkin scenario for changed user-observable behaviour or an explicit statement of why none is needed; serve's GPU auto-selection behaviour changes here and neither appears.

[siloteemu]

🔴 Automated review · pr-review-watcher · 877cf06

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.

Summary

Routes three previously unguarded amd-smi launches through one /dev/kfd-gated helper in rocm-core, adds pipe draining and a detached reaper to run_command_with_timeout, and waves the gate through on WSL. Outcome: Needs work — the code is sound and the round-3 remediation is genuinely load-bearing, but one new test passes for the wrong reason under a neighbouring mutation, and the contributor rule on user-observable behaviour is left unaddressed in the PR text. Verified: on a scratch copy I mutated each production line the new tests name and re-ran the rocm-core lib tests — deleting reap_in_background (or swapping it for drop(child)) fails the zombie test in ~10s; restoring the pre-change body of run_command_with_timeout genuinely deadlocks the 300 KB child test; neutering the bail, or making kfd_readable_with unconditionally true, fails the gate test; deleting the WSL wave-through fails the WSL test, and so does making the whole function true, so its bare-metal control really is load-bearing; the original checkout was confirmed unmodified afterwards, and I independently confirmed the stderr/stdout precedence refactor in amd_smi_json_with is logically identical to what it replaces, that the OsStr import is used on all targets, and that no test or doc consumes the timeout message's dropped partial-output suffix. The full test suite and the e2e lanes were not run in this round. Head checks: 26 success, 1 skipped, 1 pending, zero failures; trunk separately carries 3 red nightly GPU-hardware lanes that this diff cannot be attributed to. Blocking: 2 · Non-blocking: 5.

Previous round

• Zombie left by the timeout path (crates/rocm-core/src/lib.rs) — resolved. run_command_with_timeout now hands the killed child to reap_in_background, which waits on a named detached thread, and reaps unconditionally rather than only on a successful kill — exactly the shape requested. Confirmed non-vacuous by mutation, not by reading: removing the call, and separately replacing it with drop(child), both fail a_timed_out_child_is_reaped_rather_than_left_a_zombie with "pid is still a zombie" after burning the full 10s poll budget; unmutated, the process-table entry is already gone. The doc comment on reap_in_background states the residual unkillable-child case honestly, and that disclosure checks out against the code.

🚫 Blocking (must fix before merge)

crates/rocm-core/src/lib.rs — a_timed_out_child_is_reaped_rather_than_left_a_zombie, the final assert_ne!(state, Some('Z')). The assertion is satisfied by two materially different outcomes, and one of them is a broken timeout path. Empirically: removing let _ = child.kill(); from run_command_with_timeout while keeping the reaper leaves this test passing in 0.31s — the sleep 30 child is never killed, so the poll loop only ever observes state S, never Z, and assert_ne! holds for the wrong reason. The test therefore proves "reaping happens given the child was killed", not that the timeout path kills and reaps; the result.is_err() assertion above it is a timeout error, which is not evidence of a kill. On a genuinely passing run the observed state is None (entry fully gone) — I instrumented the scratch copy to confirm that, then reverted it. Fix: assert the reaped outcome exactly rather than merely not-zombie —

assert_eq!(
    state, None,
    "pid {pid} was never reaped: the timeout path must kill the child and wait on it"
);

With assert_eq!(state, None) the kill-removal mutation fails as it should (Some('S') != None), and the reaper mutations still fail as they already do. I re-ran the adversarial pass on this fix: it does not weaken the existing coverage, and it cannot flake on a slower box because the surrounding loop already polls to a 10s deadline before the assertion runs — the only way to reach the assertion with Some('S') is the defect itself.

PR text / tests/e2e-cucumber/features/ — the contributor rule on user-observable behaviour is neither satisfied nor discharged. The base tip's AGENTS.md section 3 requires that a change altering what a user of the CLI can observe — explicitly including "which runtime/engine it selects" — be covered by a scenario under tests/e2e-cucumber/features/, and states in terms that "a unit test asserting the internal helper does NOT discharge this". This change alters rocm serve GPU auto-selection on a Linux host whose /dev/kfd cannot be opened: gpu_vram_usage and detect_gpu_count now decline to launch instead of launching, and both discard the error with .ok()?. The rule offers two cheap escapes — name the existing @id: the change makes pass, or, if only a gated lane can exercise it, say so and name the lane; for a purely internal change, say why none is needed. The PR text takes none of them: its Tests section lists three unit tests and nothing else, and no scenario under tests/e2e-cucumber/features/ covers an unreadable-but-present KFD node (the nearest, around device masking and absence, cover different conditions). Fix: either add the scenario, or state plainly in the PR body which gated lane covers this and why a scenario is impractical for a wedged-driver condition. This is a one-sentence discharge if the second route is the honest one — but the rule asks for it explicitly and it is currently absent.

Non-blocking

• crates/rocm-dash-collectors/src/amd_smi.rs:137 — the fourth launch site's own kfd_accessible() runs only at detect(); the three methods that actually spawn (metrics, processes, system_info, via run/run_json around lines 114-133) never re-check, so that site is guarded once per collector lifetime rather than per launch. The PR's table records it as "pre-flight before: yes", which reads stronger than check-then-use across a daemon's lifetime; worth a caveat in the table given the title says every launch site.
• crates/rocm-core/src/lib.rs:8011 — amd_smi_preflight_ok, the pub function whose doc comment carries the "mandatory before every launch" claim, has no production caller anywhere in the tree and, on Linux, no test caller either (its only use is inside a non-Linux-only test). The gate that actually fires is the private kfd_readable. Relatedly, the PR body's basis for "no doc edit required" — that kfd_accessible does not appear outside the three changed files — is wrong: it is a live private symbol at the line above, in a file this diff does not touch, whose own doc comment independently asserts the pre-flight is mandatory.
• Nothing mechanically stops a seventh launch site landing unguarded. resolve_amd_smi_binary stays pub and directly spawnable, and no xtask check, lint or test enforces routing through amd_smi_json — the claim that "a future launch site inherits the guard" rests on convention plus a doc comment. tests/e2e-cucumber/tests/e2e/serving_steps.rs:306 already spawns a bare amd-smi with neither gate nor timeout, which is the hang this change exists to prevent, on the lane least able to absorb it.
• PR body, "Why the fourth site is left alone" — xtask/src/crate_edges.rs names exactly one dash-side invariant (rocm-dash-tui must not depend on rocm-core); an edge from rocm-dash-collectors would trip the generic allowlist path, whose own message invites adding the edge "after review if this edge is intentional". Describing the guard as existing to keep the dash crates decoupled overstates a targeted intent the file does not state, in the direction that makes leaving the site alone look more forced than it is.
• crates/rocm-core/src/lib.rs:8195 — drain_on_thread discards the read_to_end error, so a genuine pipe-read failure now surfaces as the vaguer JSON-parse error rather than the distinct collection error it used to. Separately, apps/rocm/src/main.rs:6077,6088 can now spend up to 20s in two sequential probes with no progress output; that replaces a previously unbounded stall, so it is a net improvement, but a one-line status print or running the two concurrently would remove the silence.

Withdrawing this change request: the objection is discharged.

The gate no longer treats WSL as bare-metal Linux. kfd_readable now routes through the pre-existing is_wsl_host() detector -- the same one examine.rs uses to set the is_wsl flag that drives the WSL catalog -- so it fires on a real WSL host for the same reason the catalog does. The KFD_DEVICE comment that asserted the device node is Linux-only has been corrected, and the wave-through is asserted from both sides.

Verified by mutation rather than by reading: deleting or inverting the WSL branch fails the wave-through test, and deleting or inverting the pre-flight guard fails the gate test.

A separate blocking finding, unrelated to this objection, is filed at the current head.

[siloteemu]

🔴 Automated review · pr-review-watcher · df1d926

Change request filed by automation. The findings below are the blocking half of the round published in the report comment on this pull request; the non-blocking notes stay there. This will be withdrawn once they are addressed — no human needs to clear it.

🚫 Blocking (must fix before merge)

crates/rocm-core/src/lib.rs:8140-8150 — the timeout path kills the child and bail!s without ever reaping it, so every timeout leaks a zombie for the lifetime of the parent. Rust's Child has no reaping Drop, and both copies this replaces called wait_with_output() after the kill, so this is a new defect rather than inherited behaviour. It matters because one caller is a long-running daemon: apps/rocmd/src/lib.rs:1471 uses a 2-second AMD_SMI_PROBE_TIMEOUT, reached from the watcher tick loop at apps/rocmd/src/lib.rs:3010 via gather_gpu_snapshot_for_config on the 60-second GPU_METRICS_INTERVAL_MS cadence, twice per collection. A host whose /dev/kfd is readable but whose amd-smi exceeds 2s — an ordinary outcome on a busy multi-GPU box — therefore accrues zombies indefinitely and will eventually exhaust the PID table. The pre-flight does not cover this: it prevents the spawn only when the device is unreadable.

The comment at crates/rocm-core/src/lib.rs:8144-8148 is right that this thread must not wait — but the wait can be moved off it. Reap on a detached thread instead:

if started.elapsed() >= timeout {
    let _ = child.kill();
    // Reap off this thread: an unkillable child would block the wait, which is
    // the hang this timeout exists to bound.
    thread::spawn(move || {
        let _ = child.wait();
    });
    bail!("process exceeded {}s timeout", timeout.as_secs());
}

This compiles despite the move inside the loop because bail! diverges, it keeps the "never block the caller" property the current comment is defending, and in the unkillable case it costs one parked thread instead of one zombie — no worse, and strictly better in the ordinary case that actually recurs.

[juhovainio]

Reviewed this with a combined Standards/Spec/bug-scan pass. The engineering here is solid: the zombie-reap fix in the last commit is correct (verified the new a_timed_out_child_is_reaped_rather_than_left_a_zombie test passes, and traced that the reaper thread takes ownership of child right before the bail! with nothing left to race it), the WSL wave-through is properly pinned from both directions, and cargo clippy --workspace --all-targets -- -D warnings is clean. Both of pr-review-watcher's prior blocking rounds are genuinely discharged at this head.

Two things this repo's own AGENTS.md treats as hard requirements are still unmet, though:

1. This PR changes what rocm serve does that a user can observe — on a Linux host where /dev/kfd can't be opened, GPU auto-selection now declines to launch amd-smi at all and silently falls back to service-state-only detection, where before it attempted the launch. AGENTS.md §3 is explicit that this needs a Gherkin scenario (or an named @id:, a named gated lane, or an explicit "purely internal" justification) and that a unit test on the internal helper doesn't discharge it. The PR's two new unit tests prove kfd_readable/amd_smi_json, not the observable serve behavior, and the PR body's "Docs" section addresses a different question (whether a doc file needs editing) without touching this. Left a comment on one of the two call sites below.

2. The PR body ends with an undisclosed "🤖 Generated with Claude Code" footer, which AGENTS.md §11 asks PRs to avoid.

One more thing worth a look before merge, not blocking: crates/rocm-dash-collectors/src/amd_smi.rs's kfd_accessible() still has no WSL wave-through — the same silent-detection-loss bug this PR fixes on the serve path, left standing on the dash path. The PR's stated reason for not routing it through the new shared launcher (a crate_edges layering guard plus a sync/async mismatch) checks out against xtask/src/crate_edges.rs, and it's disclosed rather than hidden, so I'm not blocking on it — just flagging in case a quick follow-up ticket is warranted.

[juhovainio]

This (and the sibling call in detect_gpu_count a bit further down, around line 21354) is the user-observable change AGENTS.md §3 is about: on a Linux host with an unreadable /dev/kfd, serve's GPU auto-selection now declines the amd-smi launch entirely and falls back to service-state-only, where before it attempted the launch. That needs a Gherkin scenario in tests/e2e-cucumber/features/ (or a stated reason none applies) per §3 — the two new unit tests added in this PR cover the helper, not this observable behavior, and neither is cited as already covering it.

[r0x0r]

Both points taken, and both are addressed in 9d2024ba, pushed after this review — so the head you reviewed (877cf068) genuinely did not have them.

1. §3. You are right that the two unit tests prove the helper, not the behaviour, and that the body did not address it. I have not added a scenario, because I do not think one can exist on any lane this suite runs; instead I have stated the gap explicitly, in the two places §3 asks for:

• The PR body now has a dedicated section, "AGENTS.md §3: why consequence 1 ships without a Gherkin scenario", replacing the silence you found. It names the premise the scenario would need — a host that is both GPU-present by the KFD/DRM sysfs rule and unable to open("/dev/kfd") (a container started without --device /dev/kfd, or a user outside the render group) — and walks each lane: the no-GPU mock lane never reaches either probe, because usable_amd_gpu_indices returns an authoritative Some([]) and serve fails fast on that first; that fail-fast is @id:serve-no-gpu-fails-fast, which passes on every PR, so the code in question is unreachable there rather than merely untested. @requires-gpu lanes have a readable /dev/kfd by construction, and @requires-wsl exercises the opposite branch (the wave-through).
• 9d2024ba also records the same reasoning as a KNOWN GAP note at the end of tests/e2e-cucumber/features/model_serving.feature, in the style that file already uses for serve-20, so it is visible to whoever next adds GPU-selection coverage rather than only in a PR description.

On why not to fake it: a scenario planting a stub amd-smi under ROCM_CLI_DATA_DIR would take the allowed branch on a GPU lane and never run at all on the mock lane — green without exercising anything, which is the vacuity the rule exists to prevent. KFD_DEVICE is a compile-time constant with no path override, the node is root-owned, and supplementary group membership cannot be dropped without privilege, so the harness cannot manufacture the premise either. If you would rather have a @nightly-gated scenario with a container that has sysfs topology and no /dev/kfd passed through, I will take that as the answer instead — it is an infrastructure change, not a code one.

The unit coverage it falls back to is the_kfd_gate_stops_amd_smi_before_it_is_spawned (crates/rocm-core/src/lib.rs:9883), which asserts the spawn does not happen — a fake amd-smi leaves a marker file that must not appear — with the same fixture re-run against a readable device to prove the fake would otherwise have executed. Non-vacuous by construction rather than by assertion.

2. Footer. Removed; the body now ends at the Test plan checklist.

On kfd_accessible() — agreed, and thank you for checking the layering reasoning rather than taking it on trust. It is still a bare open(KFD_DEVICE) at crates/rocm-dash-collectors/src/amd_smi.rs:137, with neither the cfg(target_os) gate nor the WSL wave-through, so AmdSmi::detect() returns None on Windows, macOS and WSL. Untouched since #295 and outside this PR's blast radius, so I am filing it as a follow-up ticket rather than widening this change.

[r0x0r]

Closing the loop on your non-blocking kfd_accessible() note: it has been fixed upstream, so the follow-up ticket I owed you is no longer needed.

#375 ("fix(dash): detect amd-smi through WSL DXG", EAI-8450) landed on main as d37cfbab. kfd_accessible() is gone entirely, replaced by:

fn preflight_passes(skip_device_preflight: bool, gpu_reachable: bool, kfd_device: &Path) -> bool {
    skip_device_preflight || gpu_reachable || device_accessible(kfd_device)
}

(crates/rocm-dash-collectors/src/amd_smi.rs:154) — the bare device open is now the last resort rather than the only test. apps/rocm/src/dash.rs:74 supplies gpu_reachable from rocm_core::has_usable_amd_gpu(), and the comment at :82 records that the verdict may only substitute for the device open on WSL, so bare-metal behaviour is unchanged. That is exactly the wave-through this PR adds on the serve path, now present on the dash path too.

Worth noting it was solved the way you and I both reasoned it should be: in the dash crates themselves rather than by routing them through rocm-core and tripping the crate_edges layering guard. The commit message says as much — it adds "a second, weaker WSL check" local to rocm-dash-collectors precisely so those crates "stay free of" a rocm-core dependency.

I have updated this PR's description accordingly; it previously described the gap as still open, which would now mislead a reader.

The §3 and footer items from your review remain addressed in 9d2024ba as described above, and this branch has since merged current main (a43223d8) to clear a conflict in model_serving.feature — #409 took serve-22, so my trailing KNOWN GAP note now follows it. Full workspace tests, both clippy passes and fmt are clean on the merge.

[siloteemu]

🔴 Automated review · pr-review-watcher · 877cf06

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.

Summary

Routes three previously unguarded amd-smi launches through one /dev/kfd-gated helper in rocm-core, adds pipe draining and a detached reaper to run_command_with_timeout, and waves the gate through on WSL. Outcome: Needs work — the code is sound and the round-3 remediation is genuinely load-bearing, but one new test passes for the wrong reason under a neighbouring mutation, and the contributor rule on user-observable behaviour is left unaddressed in the PR text. Verified: on a scratch copy I mutated each production line the new tests name and re-ran the rocm-core lib tests — deleting reap_in_background (or swapping it for drop(child)) fails the zombie test in ~10s; restoring the pre-change body of run_command_with_timeout genuinely deadlocks the 300 KB child test; neutering the bail, or making kfd_readable_with unconditionally true, fails the gate test; deleting the WSL wave-through fails the WSL test, and so does making the whole function true, so its bare-metal control really is load-bearing; the original checkout was confirmed unmodified afterwards, and I independently confirmed the stderr/stdout precedence refactor in amd_smi_json_with is logically identical to what it replaces, that the OsStr import is used on all targets, and that no test or doc consumes the timeout message's dropped partial-output suffix. The full test suite and the e2e lanes were not run in this round. Head checks: 26 success, 1 skipped, 1 pending, zero failures; trunk separately carries 3 red nightly GPU-hardware lanes that this diff cannot be attributed to. Blocking: 2 · Non-blocking: 5.

Previous round

• Zombie left by the timeout path (crates/rocm-core/src/lib.rs) — resolved. run_command_with_timeout now hands the killed child to reap_in_background, which waits on a named detached thread, and reaps unconditionally rather than only on a successful kill — exactly the shape requested. Confirmed non-vacuous by mutation, not by reading: removing the call, and separately replacing it with drop(child), both fail a_timed_out_child_is_reaped_rather_than_left_a_zombie with "pid is still a zombie" after burning the full 10s poll budget; unmutated, the process-table entry is already gone. The doc comment on reap_in_background states the residual unkillable-child case honestly, and that disclosure checks out against the code.

🚫 Blocking (must fix before merge)

crates/rocm-core/src/lib.rs — a_timed_out_child_is_reaped_rather_than_left_a_zombie, the final assert_ne!(state, Some('Z')). The assertion is satisfied by two materially different outcomes, and one of them is a broken timeout path. Empirically: removing let _ = child.kill(); from run_command_with_timeout while keeping the reaper leaves this test passing in 0.31s — the sleep 30 child is never killed, so the poll loop only ever observes state S, never Z, and assert_ne! holds for the wrong reason. The test therefore proves "reaping happens given the child was killed", not that the timeout path kills and reaps; the result.is_err() assertion above it is a timeout error, which is not evidence of a kill. On a genuinely passing run the observed state is None (entry fully gone) — I instrumented the scratch copy to confirm that, then reverted it. Fix: assert the reaped outcome exactly rather than merely not-zombie —

assert_eq!(
    state, None,
    "pid {pid} was never reaped: the timeout path must kill the child and wait on it"
);

With assert_eq!(state, None) the kill-removal mutation fails as it should (Some('S') != None), and the reaper mutations still fail as they already do. I re-ran the adversarial pass on this fix: it does not weaken the existing coverage, and it cannot flake on a slower box because the surrounding loop already polls to a 10s deadline before the assertion runs — the only way to reach the assertion with Some('S') is the defect itself.

PR text / tests/e2e-cucumber/features/ — the contributor rule on user-observable behaviour is neither satisfied nor discharged. The base tip's AGENTS.md section 3 requires that a change altering what a user of the CLI can observe — explicitly including "which runtime/engine it selects" — be covered by a scenario under tests/e2e-cucumber/features/, and states in terms that "a unit test asserting the internal helper does NOT discharge this". This change alters rocm serve GPU auto-selection on a Linux host whose /dev/kfd cannot be opened: gpu_vram_usage and detect_gpu_count now decline to launch instead of launching, and both discard the error with .ok()?. The rule offers two cheap escapes — name the existing @id: the change makes pass, or, if only a gated lane can exercise it, say so and name the lane; for a purely internal change, say why none is needed. The PR text takes none of them: its Tests section lists three unit tests and nothing else, and no scenario under tests/e2e-cucumber/features/ covers an unreadable-but-present KFD node (the nearest, around device masking and absence, cover different conditions). Fix: either add the scenario, or state plainly in the PR body which gated lane covers this and why a scenario is impractical for a wedged-driver condition. This is a one-sentence discharge if the second route is the honest one — but the rule asks for it explicitly and it is currently absent.

Non-blocking

• crates/rocm-dash-collectors/src/amd_smi.rs:137 — the fourth launch site's own kfd_accessible() runs only at detect(); the three methods that actually spawn (metrics, processes, system_info, via run/run_json around lines 114-133) never re-check, so that site is guarded once per collector lifetime rather than per launch. The PR's table records it as "pre-flight before: yes", which reads stronger than check-then-use across a daemon's lifetime; worth a caveat in the table given the title says every launch site.
• crates/rocm-core/src/lib.rs:8011 — amd_smi_preflight_ok, the pub function whose doc comment carries the "mandatory before every launch" claim, has no production caller anywhere in the tree and, on Linux, no test caller either (its only use is inside a non-Linux-only test). The gate that actually fires is the private kfd_readable. Relatedly, the PR body's basis for "no doc edit required" — that kfd_accessible does not appear outside the three changed files — is wrong: it is a live private symbol at the line above, in a file this diff does not touch, whose own doc comment independently asserts the pre-flight is mandatory.
• Nothing mechanically stops a seventh launch site landing unguarded. resolve_amd_smi_binary stays pub and directly spawnable, and no xtask check, lint or test enforces routing through amd_smi_json — the claim that "a future launch site inherits the guard" rests on convention plus a doc comment. tests/e2e-cucumber/tests/e2e/serving_steps.rs:306 already spawns a bare amd-smi with neither gate nor timeout, which is the hang this change exists to prevent, on the lane least able to absorb it.
• PR body, "Why the fourth site is left alone" — xtask/src/crate_edges.rs names exactly one dash-side invariant (rocm-dash-tui must not depend on rocm-core); an edge from rocm-dash-collectors would trip the generic allowlist path, whose own message invites adding the edge "after review if this edge is intentional". Describing the guard as existing to keep the dash crates decoupled overstates a targeted intent the file does not state, in the direction that makes leaving the site alone look more forced than it is.
• crates/rocm-core/src/lib.rs:8195 — drain_on_thread discards the read_to_end error, so a genuine pipe-read failure now surfaces as the vaguer JSON-parse error rather than the distinct collection error it used to. Separately, apps/rocm/src/main.rs:6077,6088 can now spend up to 20s in two sequential probes with no progress output; that replaces a previously unbounded stall, so it is a net improvement, but a one-line status print or running the two concurrently would remove the silence.

[siloteemu]

🔴 Automated review · pr-review-watcher · 877cf06

Requesting changes on two items. The full round, with the mutation evidence and the previous round's status, is in the review comment posted alongside this. The previous round's blocking item — the zombie left by the timeout path — is resolved, and that was confirmed by mutation rather than by reading.

1. The new reaper test's final assertion is satisfied by two materially different outcomes, one of which is a broken timeout path. Removing the kill from the timeout path while keeping the reaper leaves the test passing in under a third of a second: the long-sleeping child is never killed, so the poll loop only ever observes the running state, never the zombie state, and a not-equal assertion holds for the wrong reason. The test therefore proves that reaping happens given the child was killed, not that the timeout path kills and then reaps; the timeout error asserted above it is not evidence of a kill. On a genuinely passing run the observed state is that the process-table entry is gone entirely. Assert that outcome exactly rather than merely "not a zombie": with the exact-match form, the kill-removal mutation fails as it should, the reaper mutations still fail as they already do, and it cannot flake on a slower machine because the surrounding loop already polls to its full deadline before the assertion runs.

This gates rather than being a note because a weak test is never strengthened after merge, and this one sits on the exact regression the previous round was filed about.

2. The contributor rule on user-observable behaviour is neither satisfied nor discharged. The base tip's contributor guide requires that a change altering what a user of the command-line tool can observe — naming which runtime or engine it selects as an example — be covered by a scenario in the end-to-end feature directory, and says in terms that a unit test asserting the internal helper does not discharge it. This change alters serve-time GPU auto-selection on a Linux host whose kernel driver node cannot be opened: two probes now decline to launch instead of launching, and both discard the error. No scenario covers an unreadable-but-present driver node; the nearest existing ones cover absence and device masking, which are different conditions. The rule offers two cheap escapes — name the existing scenario id the change makes pass, or state that only a gated lane can exercise it and name that lane. The description's test section lists three unit tests and takes neither. This is a one-sentence discharge if the second route is the honest one, but it is currently absent.

Two further observations that do not gate but are worth reading in the round: the fourth launch site is guarded once per collector lifetime rather than per launch, which the description's table records more strongly than that; and nothing mechanically stops a seventh unguarded launch site landing, since the binary resolver stays public and directly spawnable and no check enforces routing through the guarded helper.

[siloteemu]

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

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.

Summary

Routes the three previously unguarded amd-smi launch sites through one guarded launcher in rocm-core, adds a /dev/kfd pre-flight with a WSL wave-through, and makes the shared timeout helper drain both pipes and reap the child it kills. No blocking findings. Verified: ran the four new rocm-core unit tests (all pass, 0.33 s), then mutated four single branches in a separate scratch copy — dropping child.kill() fails with Some('S'), dropping reap_in_background fails with Some('Z'), dropping the WSL wave-through fails the WSL test, dropping the gate's bail fails the spawn-marker test, and dropping the pipe draining fails both the pipe test and the gate test; every one of these fails on a deadline, none hangs. Separately confirmed in source that serve's no-GPU fail-fast bails before both probes (apps/rocm/src/main.rs:6053 precedes :6077 and :6088), that serve-13 is @id:serve-no-gpu-fails-fast, that the KNOWN GAP precedent for serve-20 at model_serving.feature:222 is real, that the trailing comment block is inert Gherkin, that no import or constant was orphaned by the two deletions, and that ci.yml:590 runs cargo test --workspace --all-targets on a Windows runner so the non-Linux test does execute. I did not modify the tree under review (git status --porcelain empty); all mutation work was done on a cp -a scratch copy. Checks at review time: 26 success, 1 skipped, 1 pending, 0 failure. Blocking: 0 · Non-blocking: 4.

Prior round

• Blocking item 1 — "the new reaper test's final assertion is satisfied by two materially different outcomes, one of which is a broken timeout path." Discharged. The assertion is now assert_eq!(state, None) and the poll retries on any surviving state, not only Z. I re-ran the exact mutation my prior claim rested on: removing child.kill() from the timeout path now fails with left: Some('S') after the full 10 s poll, where the old assert_ne! form passed. The reaper mutation still fails with Some('Z'). The prior claim was correct and is now fixed, and the mutation table in the PR description matches what I observed.
• Blocking item 2 — "the contributor rule on user-observable behaviour is neither satisfied nor discharged." Discharged. My prior claim named two escapes; I should say plainly that on re-reading neither is honest here — no existing scenario is made to pass, and no gated lane can host the premise — so the route actually available is the rule's general one, state the gap in PR text and cover at another CI level, which the PR now does at length and additionally records beside the scenarios it concerns. I spot-checked the disclosure rather than taking it, and every load-bearing claim in it holds: the mock lane really does bail before either probe, the scenario it cites is the one it means, and the cited house style genuinely exists in that file.
• Non-blocking observation — "the fourth launch site is guarded once per collector lifetime rather than per launch, which the description's table records more strongly than that." Still live. The collector's pre-flight is still called once at detection time, not from the per-launch path, and the description's table still reads an unqualified "yes" for that row.
• Non-blocking observation — "nothing mechanically stops a seventh unguarded launch site landing, since the binary resolver stays public and directly spawnable." Still live, and not required to be fixed here. The resolver is still pub; its one remaining out-of-crate caller only resolves a path rather than spawning, so nothing is broken today — the point is only that the guard remains a convention.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• apps/rocmd/src/lib.rs:1344 — run_process_with_timeout is a third near-identical helper carrying the exact undrained-pipe deadlock this PR just fixed in the two copies it replaced; its sandbox children emit JSON that can clear 64 KiB. A genuinely separate pre-existing issue, not a narrower successor to any prior item, but a strong "while you're here".
• tests/e2e-cucumber/tests/e2e/serving_steps.rs:306 — the e2e harness spawns amd-smi bare, with neither the pre-flight nor a timeout, on the very lanes that have real hardware; the hang this PR removes from product code can still take out a merge-gating lane. Pre-existing and outside the "production launch sites" claim, so the claim is accurate.
• Commit 9d2024ba carries a generated-tooling co-authorship trailer in its message, which the contributor guide's PR-conduct section tells contributors to avoid; cheap to drop on the next amend and permanent otherwise.
• crates/rocm-core/src/lib.rs:8015 — is_wsl_host() re-stats /dev/dxg and re-reads /proc/version on every gated launch; caching it in a OnceLock would make the gate free and is a one-line change.

[siloteemu]

🔴 Automated review · pr-review-watcher · f096985

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.

Every finding below is new this round. No objection from an earlier round was still standing. Because the commits since the last round are all merges of main, this round reviewed the whole change rather than only the commits since the last round.

Review — needs work

Full review of the whole change (prw-base...HEAD, merge-base 70d6aa13).
The description names ticket EAI-8457 and claims the pre-flight is now routed through every synchronous amd-smi launch site. That claim holds for the code, except that the change also deletes unrelated code the description never mentions (first Blocking item).

Blocking

• The last merge deleted the ROCM_E2E_FORCE_LOW_VRAM test hook, and the change does not mention it — apps/rocm/src/main.rs:22099-22101 (hook removed), apps/rocm/src/main.rs:32909-32930, tests/e2e-cucumber/tests/e2e/serving_steps.rs:865, tests/e2e-cucumber/features/model_serving.feature:283-291

  • What was removed: simulated_low_vram_usage() and its #[cfg(feature = "e2e-test-hooks")] call at the top of gpu_vram_usage(). Both exist on prw-base (added by #251, 5ed9365) and in the base-side parent of merge f096985. The merge result drops them, and no earlier branch commit touched them.
  • Compile failure, reproduced: cargo check -p rocm --features rocm/e2e-test-hooks --tests fails with error[E0425]: cannot find function simulated_low_vram_usage at main.rs:32922 and :32929.
  • Scenario lost: the step at serving_steps.rs:865 still sets ROCM_E2E_FORCE_LOW_VRAM=0, but nothing reads it any more. @id:serve-vllm-low-vram-oom-guidance (serve-23) therefore loses its seam on the real-GPU lane, whose cards are mostly free.
  • Why CI is green: CI builds with the feature (cargo build --release ... --features rocm/e2e-test-hooks) but never compiles the test target with it, and default cargo test/clippy don't enable it.
  • Description: it says nothing about removing the hook (unmentioned work), and "No doc edit required" and the green test plan were all run on default features.
  • Confidence 100 · logic · Fix: restore the function and its call, ahead of gpu_vram_usage_amd_smi().or_else(gpu_vram_usage_sysfs). Add a CI step or check that compiles the test target with e2e-test-hooks, so a second silent loss like this fails visibly.

• amd_smi_preflight_ok is a public function with no production caller — crates/rocm-core/src/lib.rs:8259-8261, test-only use at :10321

  • Evidence: a grep of the whole workspace finds only the definition and one assertion in a test compiled off Linux. amd_smi_json_with calls kfd_readable(kfd_device) directly.
  • Contradiction: its doc comment says the check is mandatory before every launch, which tells callers to use it. Nothing does, and the off-Linux test of it asserts nothing independent: it just returns kfd_readable(...).
  • Which side governs: the code does. amd_smi_json is the only launch path and takes the check through kfd_readable.
  • Confidence 85 · mechanical · Fix: delete it and point the off-Linux test at kfd_readable, or give it a real caller.

• The behaviour change has no scenario, and the "cannot be manufactured" argument depends on a seam the PR chose not to add — tests/e2e-cucumber/features/model_serving.feature:299-339; AGENTS.md §3

  • Behaviour: on Linux with an unreadable /dev/kfd, --gpu auto no longer launches amd-smi. That is user-observable, and §3 says a unit test on the internal helper does not discharge it. AGENTS.md lists internal-only changes as the sole exemption, and this is not one.

  • The claim is not actually blocked: it says KFD_DEVICE has "no path override", so no lane can reach the declined branch. The repo has established seams for this exact situation:

    • e2e-test-hooks-gated ROCM_E2E_* environment seams (the one the first finding restores, and ROCM_E2E_LEMONADE_BACKEND_INSTALL_FAILURE);
    • the state-file seam <ROCM_CLI_DATA_DIR>/telemetry/test-clock-offset from #466 (52f4409).

    A gated override of the KFD path on a @requires-gpu lane would make the declined branch reachable.

  • Hard to assert: I did not establish that "auto-selection degrades to service state alone" can be asserted from the CLI. If it cannot, that is the argument to make, along with a maintainer accepting the exception.

  • WSL coverage claim is wrong: line 321 says "@requires-wsl exercises the wave-through". The @requires-wsl scenarios (examine.feature:115, diagnose.feature:244,255, driver_install.feature:12-28, dash.feature:325) never reach serve --gpu auto or the rocmd snapshot, and dash-24 goes through the separate collector gate. So the WSL wave-through is covered only by the unit test.

  • Confidence 70 · architectural · Fix: add a gated seam plus a scenario, or correct the comment's WSL claim and get the exception explicitly accepted. As written, §3 is not met and the comment overstates the coverage.

Non-blocking

• The rocmd copy of run_command_with_timeout remains, with both defects this change fixes (pre-existing, pattern-relevant) — apps/rocmd/src/common.rs:165-206, caller :224

  • It is the old poll-then-wait_with_output body: it deadlocks once the child fills the 64 KiB pipe buffer, and blocks the caller after kill on an unkillable child.
  • It is now a divergent duplicate of the rocm-core function. rocmd already depends on rocm-core, so it is a one-line swap like the one made at apps/rocm/src/main.rs:15959.
  • Confidence 85 · architectural.

• Timeout error no longer quotes partial output, for every caller — crates/rocm-core/src/lib.rs:8396-8409

  • The old message was process exceeded Ns timeout: <stderr|stdout|no output>; it is now process exceeded Ns timeout. This reaches the assistant tools at main.rs:15769/:15867 (install_sdk, launch_server, 2-minute timeouts), where the last stderr shows where the stall happened. It is deliberate and documented, but untested.
  • Both the description and the code comment say why. A shared buffer snapshotted on timeout would keep the diagnostic without joining the child.
  • Confidence 80 · logic.

• a_timed_out_child_is_reaped_rather_than_left_a_zombie does not pin the "detached" property (verdicts below)

  • Replacing reap_in_background(child) with an inline child.wait() keeps it green, because a SIGKILLed sh reaps instantly. So nothing asserts that the caller returns without waiting, which is the property the docs lean on. A D-state child cannot be created in a test, so this may be untestable. If so, the comment should say so rather than imply it.
  • The test also uses a 300 ms timeout against a child that must write its PID file first, and the_kfd_gate_* writes and execs a script while sibling tests spawn children (possible ETXTBSY). Both are plausible flakes on loaded runners.
  • Confidence 70 · mechanical.

• User-facing docs still say the sysfs fallback happens only when amd-smi is absent — README.md:603-605, docs/vllm.md:226-231, skills/rocm-cli-assistant/SKILL.md:30, plus the gpu_vram_usage doc comment at main.rs:22091-22098

  • The fallback now also applies when /dev/kfd is unreadable or the 10 s probe times out. The description says "No doc edit required" on the basis of a narrow search for helper names.
  • Confidence 70 · mechanical.

• rocmd can leak threads without bound when the gate passes but amd-smi stays in D state — crates/rocm-core/src/lib.rs:8448-8454, cadence at apps/rocmd/src/lib.rs:47,52

  • Each timed-out launch leaves a reaper thread, two reader threads and the child. At the 60 s snapshot interval that is roughly 3 threads and 1 process per minute, unbounded. The doc is honest about the reaper; the cost is that the tick previously blocked once, whereas now it keeps spawning. An atomic "outstanding unreaped child" latch that makes amd_smi_json decline would bound it.
  • Confidence 70 · architectural.

• Commit history is a trail of five merge commits of origin/main. One of them, f096985, silently carried the first finding. A rebase onto main would have surfaced the conflict as an explicit resolution.

Decisions for the author

• Is a successful open(/dev/kfd, O_RDONLY) the right pre-flight probe? — tradeoff

  • For: it matches what the dash collector already does, and it filters the common missing or unpermitted node cases cheaply. It also does not change behaviour for hosts where it passes.
  • Against: it is a real kfd_open, which in upstream kfd_chardev.c/kfd_process.c creates a KFD process context and takes a non-interruptible mutex. In the wedged-driver state the doc comment names, the open itself could block the serve or rocmd tick thread uninterruptibly, which is worse than before. The context is tied to the process's address space, not the handle, so a long-lived rocmd would hold one for its lifetime. An access(2) permission check avoids both costs, and a probe child with a timeout keeps the open but bounds it. The doc's claim that declining is "the only mitigation" overstates what an open can detect (version-mismatched or partly installed drivers usually still open fine). I argued this from kernel source only and could not reproduce it.
  • Regression risk: unprivileged hosts where /dev/kfd is 0660 and the user is not in render may have had amd-smi telemetry that now silently disappears. This matters for rocmd telemetry, less so for serve.

• Two KFD pre-flights now exist with different WSL rules — tradeoff

  • rocm-core waves WSL through unconditionally (is_wsl_host()); the dash collector (rocm-dash-collectors/src/amd_smi.rs:154, apps/rocm/src/dash.rs:89) only waves it through when a usable GPU was found. The description explains why the collector was left alone (layering and async), so this is deliberate, but the policy divergence is not documented in either place. is_wsl_host() matches "wsl" as a substring of /proc/version or the presence of /dev/dxg; a misfire fails open to pre-PR behaviour.

• The serve probes now have a 10 s timeout where they had none — non-blocking-improvement

  • A slow multi-GPU amd-smi that takes more than 10 s now returns None silently, and the sysfs fallback gives nothing on multi-GPU hosts, so VRAM-aware auto-selection degrades without a message. Consider logging the decline or timeout reason.

Positive signals

• The WSL wave-through is tested with a bare-metal control (kfd_readable_with(false, absent) must still decline), so the test cannot pass with the wave-through deleted.
• the_kfd_gate_stops_amd_smi_before_it_is_spawned uses a marker file plus a readable-device re-run, so the blocked case cannot pass vacuously.
• Draining both pipes on threads before the timeout poll fixes a real silent-telemetry-loss hazard that routing the serve path through the old helper would have introduced.

Deployment notes

• Linux hosts that cannot open /dev/kfd now skip amd-smi entirely in rocm serve GPU auto-selection and the rocmd GPU snapshot. WSL and non-Linux are unchanged. That is a behaviour change for users outside the render group.
• The rocm-core public API gains amd_smi_json, amd_smi_preflight_ok and run_command_with_timeout.

What this covered

• Every changed file read completely: apps/rocm/src/main.rs (all changed hunks and their surrounding functions and callers), apps/rocmd/src/common.rs, crates/rocm-core/src/lib.rs (every added item and test), and tests/e2e-cucumber/features/model_serving.feature. The diff was read in full and compared against git show prw-base:<path>. Reviewed prw-base...HEAD (merge-base 70d6aa13…, head f0969855…). The work was split across four read-only workers (code pass for the three source files, impact and completeness, agent-instruction/history/comments, design questions); none built anything.
• Checks run:
  • cargo check -p rocm --features rocm/e2e-test-hooks --tests (one build, reproduced the first Blocking item).
  • The gh pr view CI rollup for this head: every check passed or was skipped (clippy, build-and-test, windows-build-and-test, E2E tests, commit signatures, among others). Passing CI does not cover the hook-enabled test target, as noted above. The Skill checks (skillscope) and Sphinx docs build (-W) checks had not reported when this round was handed over.
• Did not run: the full workspace test suite, clippy, or any mutation run; test verdicts below were reached by reading and tracing, not execution. The one exception is a_timed_out_child_is_reaped_rather_than_left_a_zombie, whose revert behaviour I could not run and argued from the code only.
  • Test verdicts, reasoned not run.
    • the_kfd_gate_stops_amd_smi_before_it_is_spawned: fails with the gate reverted; stays green if the public amd_smi_json or a call site stopped routing through the gate, because it calls the _with variant, and it passes vacuously on WSL.
    • a_child_that_outgrows_the_pipe_buffer_is_not_reported_as_hung: fails with the drain reverted; either pipe's drain removed alone is also caught.
    • a_timed_out_child_is_reaped_rather_than_left_a_zombie: fails with the kill or the reap removed; stays green if the reaper is made synchronous.
    • the_kfd_preflight_waves_through_hosts_that_have_no_kfd (non-Linux): its second assertion cannot fail independently of the first.
    • the_kfd_preflight_waves_through_wsl_hosts: stays green if kfd_readable hard-coded the WSL flag, because the wiring to is_wsl_host() is unpinned.
• Prior-changes pass ran against already-merged PRs only (#251 and #466 applied); no earlier pass of this PR was read.
• Not reconciled against any discussion here.

[r0x0r]

Confirmed, and fixed in a3085af0. The finding is correct and the regression was mine, introduced resolving a merge rather than by any intentional change — thank you for catching it.

What happened. #251 was squash-merged, which broke this branch's shared lineage and turned the main merge into a large conflict. Resolving gpu_vram_usage, I kept this branch's pre-refactor body and then restored main's dispatcher shape — but not simulated_low_vram_usage() or its #[cfg(feature = "e2e-test-hooks")] call, both of which arrived with #251. The symbol count fell from main's 4 to 2: only the two dangling references in the test module survived.

Why nothing caught it. Exactly as you say — the default build never reaches the hook, so 21 checks stayed green. It breaks only under the feature, which no per-PR job builds with tests:

cargo check -p rocm --features rocm/e2e-test-hooks --tests
error[E0425]: cannot find function `simulated_low_vram_usage`

The release-with-hooks build the E2E lane does run compiles fine, because the dangling references are in #[cfg(test)] code. That gap between "builds with the feature" and "builds the tests with the feature" is what let it through, and it is worth noting independently of this PR.

And your second point is the one that would have hurt most: serving_steps.rs:865 still set ROCM_E2E_FORCE_LOW_VRAM with nothing left to read it, so the scenario would have kept passing while asserting nothing about low-VRAM behaviour — a green test covering a hook that no longer exists.

The fix restores both pieces verbatim from main. I diffed them rather than retyping: the hook body and the gated call in the dispatcher are now byte-identical to origin/main, and the symbol count matches again at 4.

One honest caveat. I have not been able to re-run the feature-gated compile. The Linux devbox is down — its pod has been Pending for ~90 minutes behind a 502 from the node proxy — and macOS cannot build this workspace at all (cargo check stops in rocm-engine-lemonade on u32 & u16, long before reaching rocm). So the verification standing behind this commit is textual identity with the known-good tree, not a fresh compile. I will run cargo check -p rocm --features rocm/e2e-test-hooks --tests and report the result the moment the devbox returns; if it disagrees with me, I will say so here.

Stale: filed against 877cf06 on 2026-09-30, before this branch merged main twice. The e2e-gap point it raised was addressed, and the branch is now at 7c8202b with CI green. Re-requesting a fresh review.

[r0x0r]

Pushed b7317343. This round does three things: fixes @juhovainio's two outstanding inline findings, resolves the conflict with main (which had moved almost all of this code into new modules), and rewrites the PR description, which several of you correctly flagged as describing an earlier shape of the branch.

---

Finding 1 — unbounded leaked probe threads (device_readable_within)

"repeated polling … can leak threads until the process exhausts its thread budget, at which point this fallback reverts to a blocking open on the calling thread, recreating the hang this gate exists to prevent. Is this an acceptable tradeoff, or should outstanding probe threads be bounded?"

Bounded, not argued for. crates/rocm-core/src/amd_smi.rs:260-400:

• MAX_INFLIGHT_KFD_PROBES = 8 slots in a ProbeBudget (:267, :271, :282), claimed with fetch_update (fetch_add plus a compensating decrement is momentarily over the limit, which two callers racing at the boundary can both observe).
• The ProbeSlot guard is moved into the probe thread (:387), so a slot frees when the kernel finally returns from open, not when the caller gives up — the budget counts threads still parked, which is the thing that escalates. A host that recovers gets its full budget back with nothing having to notice.
• With the budget spent, device_readable_within_budget returns false without opening anything (:376).

The if !spawned { blocking open } fallback is gone too (:394), which is the part of your comment I think was the real point. You are right that it fails open into exactly the hazard, and the doc comment that defended it was wrong in a specific way: it treated thread exhaustion as independent of the device state, when the way this process exhausts threads here is one parked open per poll against a wedged device. So the fallback fired precisely when the device was most dangerous, performing an unbounded blocking open on the caller's own thread, where there is nothing left to kill. Both remaining failure branches now answer false, the same declining answer the timeout gives. The doc paragraph is rewritten to say that rather than the opposite (:348-368, under "Why both failure branches decline").

New test the_kfd_probe_budget_declines_instead_of_parking_threads_without_bound (amd_smi.rs:779). It is non-vacuous by construction: the capped probe is pointed at an ordinary readable file, so an unbounded implementation — or one with the old fallback — answers true; only a probe that refuses on an exhausted budget answers false. A control re-runs the same file against a budget with a free slot and must answer true, so a mutant returning false unconditionally fails too. Mutation-checked on the box: restoring the old blocking-open fallback fails it with

with every probe slot held by a parked open, the pre-flight must decline rather than
open on the calling thread — that fallback is the hang this gate refuses

It uses a budget of its own rather than the production one, because it deliberately parks a thread for the lifetime of the test binary and spending a production slot would quietly shrink the budget for every other test in the process.

Finding 2 — understated latency

"KFD_OPEN_TIMEOUT (2s) adds to every call's latency on top of timeout. The documented ~20s worst case for serve --gpu auto looks understated by that amount."

Correct, and it was understated in three places, all now fixed. @jussielo-amd made the same point ("closer to 24s").

Path	Probes	Was documented	Actually
serve --gpu auto	2 sequential	~20s	2 × (2s + 10s) = ~24s
--model readiness	1	10s	2s + 10s = ~12s
rocmd telemetry poll	1	2s	2s + 2s = ~4s

• apps/rocm/src/gpu_policy.rs:501-528 — the AMD_SMI_SERVE_PROBE_TIMEOUT doc now has a "Worst-case latency" section with the arithmetic shown, and says plainly that the previous 10s/20s figures omitted the pre-flight.
• crates/rocm-core/src/amd_smi.rs:244-250 — KFD_OPEN_TIMEOUT's doc states the bound is additive to every amd_smi_json timeout, not inside it, and points at the caller that documents a worst case.
• crates/rocm-core/src/amd_smi.rs:430-436 — amd_smi_json gets a # Latency section giving the general rule.
• apps/rocmd/src/common.rs:39-44 — the watcher's 2s constant now says a poll can cost 4s, still inside its 60s cadence.

One honest qualifier, stated in the doc rather than quietly assumed: the two bounds are additive in the formula but rarely both paid, because a host whose open hangs is a host that then declines and never spends the amd-smi budget. A ceiling that assumes the good case is not a ceiling, so it is quoted additively.

---

Merge resolution

main has been running the Phase 6x modularization (ROCMAI-82/91/211/214/215), which moved most of what this branch touches. Several conflict hunks had an empty main side — that meant "moved", not "deleted", so each was resolved by dropping the HEAD block and re-applying this branch's new content onto the module that now owns it. @siloteemu's merge-conflict finding enumerated exactly this and it is what the resolution follows.

Was	Now
rocm-core/src/lib.rs — KFD_DEVICE, preflight_kfd_device, kfd_readable/_with, KFD_OPEN_TIMEOUT, device_readable*, amd_smi_json/_with, MAX_QUOTED_OUTPUT_CHARS, all 8 unit tests	rocm-core/src/amd_smi.rs (the module main created for resolve_amd_smi_binary), re-exported as rocm_core::amd_smi_json
rocm-core/src/lib.rs — command_failure_detail_or_else	stays in lib.rs, beside the command_failure_detail it factors
apps/rocm/src/main.rs — AMD_SMI_SERVE_PROBE_TIMEOUT, gpu_vram_usage_amd_smi, detect_gpu_count	apps/rocm/src/gpu_policy.rs. main.rs is now byte-identical to main's
apps/rocmd/src/common.rs	main's import list with resolve_amd_smi_binary swapped for amd_smi_json

All eight original tests were relocated, not dropped, and each was re-run by name on Linux (11 passed in amd_smi::tests, including the three pre-existing resolve_amd_smi_binary* ones).

This also lets one thing collapse that could not before: the failure quote now shares process.rs's MAX_QUOTED_OUTPUT_CHARS (made pub(crate)) instead of declaring a second constant. That constant's own doc named capture_amd_smi_json's quote as the one it did not yet cover (#623); the two converge rather than drift, and process.rs's doc is updated to say so.

Scenario renumbering. main took examine-29 (Windows display driver) and examine-30, so this branch's scenario is now examine-31, and the model_serving.feature note that cites it is updated. I checked every other examine-29 reference in the tree (docs/testing.md:1361, docs/manual-testing.md:353, tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:3244) — all three refer to main's Windows driver scenario and are correct as they stand. main also took serve-26/serve-27; this branch adds no numbered serve scenario, only the trailing note, so nothing collided there.

docs/architecture.md's amd_smi.rs entry said "invoking amd-smi stays with its callers", which is no longer true — updated, along with the gpu_policy.rs entry. That addresses the doc-currency item @jussielo-amd and @juhovainio both raised, now that the code has a module of its own rather than sitting in lib.rs.

Verification

The Darwin pre-push hooks cargo-clippy and cargo-test cannot compile this workspace (engines/lemonade reads st_mode as u32; it is u16 on Darwin), so they were skipped on the push — SKIP=cargo-clippy,cargo-test, nothing else, no --no-verify. Everything else ran and passed locally: verify-commits (both merge commits are GPG-signed and DCO signed-off), license-headers, cargo fmt, cargo clippy (e2e harness + steps), YAML/whitespace/shellcheck.

The two skipped checks were run first on the MI300X Linux dev box, against main (11a60691) with this branch's net diff applied, capturing real exit codes:

cargo build  --workspace --all-targets                                   # exit 0
cargo clippy --workspace --all-targets -- -D warnings                    # exit 0
cargo clippy -p e2e-cucumber --test e2e -- -D warnings                   # exit 0
cargo clippy --workspace --all-targets --features rocm/e2e-test-hooks \
             -- -D warnings                                              # exit 0
cargo test   --workspace --all-targets --exclude e2e-cucumber            # exit 0, 0 failed
cargo test -p rocm-core --lib --features e2e-test-hooks -- amd_smi::     # 11 passed, 0 failed
cargo fmt --all --check                                                  # clean
cargo xtask check-crate-edges                                            # clean

Not claimed: that box has no amd-smi and no GPU telemetry despite having KFD nodes, so nothing here attests real-device behaviour. cargo xtask e2e was not re-run (it drives real vLLM serves); examine-31 is unchanged in substance from its last green run, only renumbered.

---

Still open, deliberately

@jussielo-amd — your blocking item stands and I am not claiming otherwise: rocm-dash-collectors' device_accessible is still a single unbounded open(). It gates the spawn, but it is not bounded the way device_readable now is. Closing it needs either a reviewed crate_edges allowlist entry or a dependency-free leaf crate hosting just the bounded-open primitive — a layering decision rather than a code one, which is why it is not widened into this PR. The PR description now states that asymmetry plainly instead of implying the two gates are equally hardened, which was the narrower half of your finding and @siloteemu's B2. I have left your review in place rather than dismissing it.

@siloteemu — of your blocking set, the merge conflict and the examine-29 collision are fixed here. The host_path-routing / WSL-conjunction mutation finding and the seam test's non-feature-build claim are not; they are real and I would rather address them in their own round than bundle them into a merge resolution. Review left standing.

[juhovainio]

Nothing here blocks the fix itself: the gate stops amd-smi from being spawned, and the examine-31 scenario asserts both the message and the resulting state. I'm requesting changes for one docs gap and two points I'd like addressed or answered.

To fix

• docs/testing.md isn't updated for the new ROCM_E2E_KFD_DEVICE seam or scenario examine-31. The repo guidelines ask for that in the same change.

To address or answer

• The gate declines on permission denied and on a missing KFD node, but the rationale is amd-smi hanging in D-state. A failed open with an errno can't cause that, so the gate looks broader than the hazard it describes. On hosts where the user can't open /dev/kfd, detect_gpu_count is lost and has no sysfs fallback (gpu_vram_usage does). Erring safe is fine; please confirm it's intended or narrow the claim (inline comment).
• Roughly 600 of the ~1100 added lines in amd_smi.rs are doc comments, and several narrate review history or cite a commit hash. Describing the invariant and leaving history to the PR/commit would make this much easier to maintain (inline comment).

FYI, no change requested

• The dash collector's device_accessible still does an unbounded open(); the PR body acknowledges this and defers it.
• The cargo xtask e2e item in the PR test plan is unchecked, and the MI300X / W7900 / Strix Halo Ubuntu lanes were still pending when I looked. Everything that had completed was green.

[juhovainio]

The doc says amd-smi blocks in D-state when the node is absent or permission is denied. Those states fail the open with an errno, so they can't park amd-smi in uninterruptible sleep. The gate therefore over-declines: users outside the render group lose detect_gpu_count, which has no sysfs fallback. Could you narrow the claim or the gate, or confirm this is intended?

[r0x0r]

Good catch, and you are right that the rationale did not match the gate. A failed open that returns an errno cannot park anything in D-state, so describing all four decline reasons as "the states in which amd-smi blocks in uninterruptible sleep" was simply wrong.

Decision: keep the fail-safe gate, narrow the claim. Narrowing the gate turns out to be the worse of the two options. To decline only on a timeout, the gate would have to treat every errno it did not anticipate as safe to launch into — and the errno a driver returns mid-reset is exactly the thing no test in this repo can pin, because there is no way to manufacture a resetting KFD on any lane we have. An EBUSY/EIO from a device that is about to wedge would then be waved straight through to amd-smi. Not resting on an assumption of that shape is the reason the open is bounded at all (KFD_OPEN_TIMEOUT's doc comment makes that argument), so building an errno taxonomy on top of it would reintroduce the same class of mistake one level up.

On the detect_gpu_count asymmetry — real as you state it, and I went and worked out what it actually costs rather than waving at it. On the hosts where this branch can fire (bare-metal Linux), detect_gpu_count is not what serve resolves ordinals from:

• serve_cmd.rs calls rocm_core::usable_amd_gpu_indices(), which counts KFD topology nodes under /sys/class/kfd/kfd/topology/nodes and AMD DRM cards under /sys/class/drm. Both are world-readable and neither needs the device node open, so that set is intact on a host outside the render group.
• validate_pinned_gpu_index prefers that set outright over the detected count ("it is authoritative and already in the HIP-ordinal space"), so --gpu <index> validation and the no-usable-GPU fail-fast are unaffected on exactly the host you are describing.

What is genuinely lost is the secondary count --gpu auto ranks with. On a single-GPU host the DRM sysfs VRAM rows still supply it (effective_gpu_count); on a multi-GPU one auto-selection returns no selection and the engine's own device probe pins the first present GPU. That host is also the one rocm diagnose already names, via fix-4-render-group.

I did not give detect_gpu_count a sysfs count fallback of its own: /sys/class/drm counts physical cards while amd-smi list counts HIP-visible devices, and the two can legitimately disagree, so a silent substitute could mis-size tensor parallelism. Given the above makes the loss non-load-bearing, erring safe without inventing a second count source seemed the right trade — but say the word if you would rather have it and I will add it with its own test.

All of this is now in the code rather than only here: kfd_readable has # What the predicate is and # What the broad refusal costs sections saying precisely which class declines and why the broader refusal is deliberate, detect_gpu_count's own doc says it lands here and points at that reasoning, and the error message no longer attributes every decline reason to D-state.

[juhovainio]

This comment narrates review history ("was reviewed as resting on..."), and the kfd_readable docs cite a commit hash. Could you describe the invariant instead? The history belongs in the PR or commit message.

[r0x0r]

Fair, and the commit hash was worse than noise: b4f98bc2 does not exist in this repository at all. @jussielo-amd caught the same thing in his October review and traced the real commit — d37cfbab, PR #375 / EAI-8450. Both citations are gone. The surviving text states the behavioural reason instead (a WSL host whose ROCDXG plumbing is not ready still hangs, so a bare identity check would wave it through) without naming any commit.

Pruned the rest of the narration too:

• device_readable_within_budget's "both of those used to fall back to a direct blocking open…" archaeology, restated as the invariant: both branches decline, and here is the escalation that makes a fallback wrong.
• the_kfd_probe_declines_a_device_whose_open_blocks_instead_of_hanging's "kfd_readable was reviewed as resting on an unverified assumption…".
• the_kfd_probe_budget_declines_…'s "the escalation this pins is the one the review named".
• The dash collector's "this was a plain OpenOptions::open(..).is_ok()…".
• gpu_policy.rs's "both figures used to be quoted as 10s and 20s, which omitted the pre-flight".

Net about 100 lines of narration out, which is also why amd_smi.rs's added-comment count drops despite the errno/scope answer in the sibling thread adding roughly 30 lines of genuinely new reasoning.

What I deliberately kept is the reasoning a future maintainer cannot reconstruct from the code:

• why both failure branches decline rather than falling back to a blocking open (the thread budget is exhausted by precisely the device the gate refuses, so the fallback fires when it is most dangerous);
• why the probe budget exists at all;
• why the WSL and off-Linux wave-throughs are load-bearing rather than stubs (reading an absent /dev/kfd as "unsafe" disables GPU detection on every Windows host);
• why the off-Linux kfd_readable has to be const fn — the Windows lane's -D warnings makes clippy::missing_const_for_fn a hard compile error there while the Linux lane never sees it, which is not guessable from the Linux side;
• the "what this would catch" note on each test, since several are non-obvious (the FIFO fixture, the watched-thread deadline, the marker file).

Happy to cut further if any of those still read as history rather than invariant.

[r0x0r]

@jussielo-amd — the dash-collector gap is closed. Pushed as 741852a8.

crates/rocm-dash-collectors/src/amd_smi.rs's device_accessible no longer does its own bare open(); it calls the same bounded probe the rest of the gate uses, now exposed as rocm_core::device_readable_bounded. That carries the 2s bound and the in-flight probe budget with it, so the dashboard collector gets the same protection as the CLI launch sites rather than a second implementation that could drift from them.

That needed a new first-party edge, rocm-dash-collectors -> rocm-core. xtask check-crate-edges is designed to force exactly this conversation, so: the invariant that forbids depending on rocm-core binds rocm-dash-tui, the presentation layer — it makes no amd-smi call and keeps the stricter rule. The collectors crate does spawn amd-smi, on the same /dev/kfd, which is what makes it part of this gate. The ALLOWLIST entry and the reasoning are both recorded in xtask/src/crate_edges.rs's module doc, since that file is the only written record of these rules.

Your two non-blocking notes are also handled: the ~20s figure was wrong in three places and is now stated as the real worst case per entry point (serve --gpu auto ~24s, --model ~12s, rocmd poll ~4s), with the KFD open bound called out as additive to the amd-smi timeout. The device_readable_within spawn-failure fallback you were willing to accept as-is is actually gone — it fell back to an unbounded blocking open() on the caller's own thread, and the way that thread budget gets exhausted is one parked open per poll against a wedged device, so it fired precisely when the device was most dangerous. Both failure branches now decline.

On the lib.rs growth you flagged for a later extraction pass: that pass has happened in main in the meantime, so the whole cluster plus its tests now lives in crates/rocm-core/src/amd_smi.rs.

@siloteemu — your blocking items from c1ad263d were addressed in the merge that preceded this: the branch now merges cleanly into current main (the serve probes are re-homed to gpu_policy.rs, main.rs is byte-identical to main's), examine-29 is renumbered to examine-31 since main took 29/30, and the PR description has been rewritten — you were right that it described an earlier shape of the branch in both directions.

Verification

Linux MI300X box, exact merged tree applied onto main (11a60691):

• cargo build --workspace --all-targets — exit 0
• cargo clippy --workspace --all-targets -- -D warnings — exit 0
• cargo test --workspace --all-targets — exit 0, 75 suites, 0 failures
• cargo xtask check-crate-edges — exit 0 (the new edge is accepted only because of the ALLOWLIST entry; removing it fails the check)
• cargo fmt --all -- --check — exit 0

One thing the Linux box could not catch and the pre-push hook did: the first version of device_readable_bounded used the fs import, which is Linux-gated in that module, so the non-Linux branch did not compile. Fixed before push and confirmed with cargo check -p rocm-core on macOS. Worth noting because it is exactly the class of error a Linux-only verification misses — the Windows lane would have caught it in CI.

Still not reproducible here: real /dev/kfd wedge behaviour and live amd-smi telemetry — the dev box has neither. The bounded-open path is covered by the budget test that points a capped probe at an ordinary readable file, which an unbounded probe answers true.

The blocking dash-collector gap is closed in 741852a: crates/rocm-dash-collectors/src/amd_smi.rs now calls the shared rocm_core::device_readable_bounded instead of a bare open(), with the new rocm-dash-collectors -> rocm-core edge recorded in xtask/src/crate_edges.rs. Both non-blocking notes are also addressed (latency figures corrected; the spawn-failure fallback removed). Re-requesting your review.

[r0x0r]

@juhovainio thanks — all three addressed in 472efacc, plus one note on the FYI.

docs/testing.md (blocking)

New ## amd-smi KFD pre-flight section, sitting with the other preflight sections and following the shape the file already uses for seams and scenarios (ROCM_CLI_TEST_HOST_ROOT/diagnose-40, the simulated-root examine-2x paragraph just above it). It covers:

• what the gate is, which launch sites go through it, and that the dashboard collector shares the same bounded probe;
• that the gate declines on any unreachable device, not only one that would hang — pointing at kfd_readable's doc for why;
• runnable cargo test lines for the unit tests, and what each of them pins;
• ROCM_E2E_KFD_DEVICE: which build honours it, that it moves the gate rather than removing it, and why ROCM_CLI_TEST_HOST_ROOT cannot stand in (the real node's veto makes the allowed branch unreachable under a simulated root);
• examine-31: what it plants, what it asserts, and why both assertions are load-bearing — without the marker it passes against a deleted gate, without the named device it passes against a build that ignores the seam;
• the honest gap: no scenario drives rocm serve/rocm model's own call sites, and why (.ok()? leaves nothing user-visible to assert on).

Also fixed docs/architecture.md, whose amd_smi.rs entry still said the dash collectors "keep their own pre-flight, since those crates deliberately do not depend on this one" — no longer true after 741852a8.

Errno vs D-state scope

Answered in detail in the thread. Short version: you are right that the rationale was wrong, so I narrowed the claim and kept the fail-safe gate — narrowing the gate would require deciding per errno which kernel states are safe to launch into, and the mid-reset errno is precisely what no lane here can manufacture, so EBUSY/EIO from a device about to wedge would get waved through. That is the same unverifiable-assumption shape the bounded open exists to avoid.

On the asymmetry you named: it is real, and I chased what it costs instead of asserting intent. On bare-metal Linux serve resolves ordinals from usable_amd_gpu_indices() (KFD topology nodes under /sys/class/kfd, AMD DRM cards under /sys/class/drm — both world-readable, neither needing the node open), and validate_pinned_gpu_index prefers that set outright over the amd-smi count. So --gpu <index> validation and the no-usable-GPU fail-fast survive on exactly the host you describe; what is lost is the secondary count --gpu auto ranks with, which the sysfs VRAM rows still cover on a single-GPU host and which degrades to "engine picks" on a multi-GPU one. All of that is now written down on kfd_readable and detect_gpu_count, not just here.

Doc volume

Answered in the thread. About 100 lines of narration removed — review-round history, "this used to be X" archaeology, and both citations of b4f98bc2, a commit that does not exist in this repository (@jussielo-amd flagged the same hash in October; the real one is d37cfbab, #375 / EAI-8450). What survives is the non-obvious invariants: why both failure branches decline, why the budget exists, why the WSL/off-Linux wave-throughs are load-bearing, and why the off-Linux kfd_readable must be const fn.

FYI: the dash collector

That one is already done — 741852a8 landed about 30 minutes after you submitted, so you would not have seen it. device_accessible now calls rocm_core::device_readable_bounded, the same 2s-bounded, 8-slot-capped probe rocm-core's own gate uses, via a deliberate rocm-dash-collectors -> rocm-core edge recorded in xtask/src/crate_edges.rs. The PR body's "why the fourth site is left alone" section was stale on this and has been rewritten.

Verification

Run on the MI300X Linux box against main at 11a60691 plus this branch's net diff (macOS cannot build this workspace — engines/lemonade treats st_mode as u32):

check	exit
cargo build --workspace --all-targets	0
cargo clippy --workspace --all-targets -- -D warnings	0
cargo clippy -p e2e-cucumber --test e2e -- -D warnings	0
cargo clippy --workspace --all-targets --features rocm/e2e-test-hooks -- -D warnings	0
cargo test --workspace --all-targets --exclude e2e-cucumber	0, 0 failed
cargo test -p rocm-core --lib --features e2e-test-hooks -- amd_smi::	0, 11 passed
cargo fmt --all -- --check	0
cargo xtask check-crate-edges	0
cargo xtask e2e -- -n 'examine-31 '	0, 1 scenario / 3 steps passed

And cargo check -p rocm-core --all-targets on macOS, with and without e2e-test-hooks — exit 0 both ways, so the cfg(not(target_os = "linux")) branch still builds (fs is Linux-gated in this module, and that has bitten before).

The clippy and test runs above are what the push hook would have run; it was skipped only for those two, since neither can compile on Darwin. Not verified: the box has no amd-smi, no rocm-smi and no GPU telemetry, so none of this is real-device verification of the gate against an actual wedged /dev/kfd — that remains untestable on any lane we have, which is itself part of the argument for the fail-safe branch above.

[r0x0r]

@jussielo-amd — closing the loop on your 19 Oct review, since it was not fully superseded by your 28 Oct one. Point by point:

B1, the fabricated commit-hash citations. You were right and this was still live until now: b4f98bc2 appears nowhere in this repository, and the behavioural claim it attached to was correct. Both citations are gone in 472efacc. I dropped the hash rather than substituting d37cfbab, because @juhovainio's parallel point is that the invariant belongs in the doc and the provenance belongs in the PR — so the text now just states why the conjunction is is_wsl_host() && has_usable_amd_gpu() instead of citing who changed it. Thanks for actually running git cat-file/git fsck on it; the real commit is recorded here and in the PR body as #375 / EAI-8450 / d37cfbab.

B2, the overstated parity between the two pre-flights. Closed in 741852a8, taking the option you suggested: the collector's device_accessible now calls rocm_core::device_readable_bounded, so both sites share one bounded, budget-capped probe. I took the allowlisted rocm-dash-collectors -> rocm-core edge (recorded in xtask/src/crate_edges.rs) rather than the leaf-crate alternative — same sharing, one reviewed edge instead of two. That also answers your "Decisions for the author" item: the leaf crate was considered, and rejected on that count.

B3, PR description contradicting the diff. Rewritten. The seam and scenario sections now match the branch, and this round also rewrote the "why the fourth site is left alone" section, which 741852a8 had made stale in the other direction.

B4, no test exercises the rocm serve/rocm model call sites. Still true, and not fixed. Your later review dropped it, but it deserves an answer rather than a silent dismissal: both probes discard the decline with .ok()? by design, so on a GPU-less lane the declined branch is byte-identical to "this host has no GPU" — there is no user-visible output for a scenario to assert on, and a scenario asserting a latency bound would be asserting the harness's own timing. Your suggested rocm model + absent-seam + marker-absence shape would work in principle; I did not take it this round because the marker assertion is the only non-vacuous half and it duplicates what examine-31 already proves about the same chokepoint. What I did instead is write the gap down explicitly in docs/testing.md's new section, naming the reason, so it is a recorded decision rather than an omission.

NB1, no catalog entry for "no KFD GPU node" or "reset in progress". Confirmed and not addressed here. fix-4-render-group covers the permission case; the other two have no recipe. That is pre-existing (the .ok()? discard is identical at the base commit) and adding diagnosis entries is a doctor-catalog change with its own catalog/scenario/reference-doc obligations, so widening this PR into it would make a /dev/kfd pre-flight PR also a doctor PR. Worth a follow-up ticket.

NB2, stale docs/architecture.md lib.rs inventory. The inventory caught up when #809 extracted this code into amd_smi.rs; this round fixed the one remaining wrong sentence in that entry, which still claimed the dash collectors keep their own pre-flight because those crates do not depend on rocm-core.

Dismissing on that basis — shout if B4 or NB1 should block rather than become follow-ups.

[nowycondro]

🤖 Automated review (Claude Code) · 741852a

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 (base 11a60691...head 741852a). Implements the fix described in EAI-8457 (Jira alias of ROCMAI-342, status In Progress): all four production amd-smi launch sites now run the /dev/kfd pre-flight, matching the ticket's own four-row completeness table.

Blocking

• docs/architecture.md:101 contradicts this same PR's own code change — docs/architecture.md:101, crates/rocm-dash-collectors/Cargo.toml (+1), xtask/src/crate_edges.rs:17-21,66
  The doc line added by this PR reads "the rocm dash collectors keep their own pre-flight, since those crates deliberately do not depend on this one" — but this PR's own xtask/src/crate_edges.rs adds ("rocm-dash-collectors", "rocm-core") to ALLOWLIST with a comment explaining rocm-dash-collectors does now depend on rocm-core (EAI-8457), and Cargo.toml/Cargo.lock add that exact dependency. git log --first-parent confirms the doc line was written in an earlier commit on this branch, before the final commit (741852a8) added the dependency, and the final commit never updated it. This is a direct, traceable self-contradiction within the reviewed diff, and squarely the pattern AGENTS.md §5 calls out ("the same behavior claim... repeats across... docs/*.md... grep for the claim's wording... and check each against the actual code path").
  Confidence 90 · mechanical · Fix: reword the sentence to describe the actual relationship (shares device_readable_bounded via an allowlisted edge, no longer fully independent). Note: this is already fixed in a later commit on the PR (472efacc, not part of the reviewed diff) — flagged here because it is present and wrong at the reviewed head.

• device_readable_bounded's non-Linux branch is new, completely untested code — crates/rocm-core/src/amd_smi.rs:419-429

  pub fn device_readable_bounded(device: &Path) -> bool {
      #[cfg(target_os = "linux")]
      { device_readable(device) }
      #[cfg(not(target_os = "linux"))]
      { std::fs::OpenOptions::new().read(true).open(device).is_ok() }
  }

  git grep -n "device_readable_bounded" across the repo turns up only the definition, the lib.rs re-export, and the one call site in crates/rocm-dash-collectors/src/amd_smi.rs:208 — no test anywhere calls it, on either platform. The Linux branch is incidentally covered because it forwards to the already-tested device_readable, but the #[cfg(not(target_os = "linux"))] branch is new, unbounded code with zero coverage, and this isn't impractical to test: .github/workflows/ci.yml has a windows-latest job (lines 581, 1290). This is exactly the case AGENTS.md's "platform-gated code gets a test that runs on its gate" rule targets, and behaviour this change introduces that no test exercises is blocking per this repo's own test-coverage standard.
  Confidence 85 · logic · Fix: add a #[cfg(not(target_os = "linux"))] unit test (readable file → true, nonexistent path → false), mirroring the existing off-Linux kfd_readable wave-through test.

Non-blocking

• AmdSmiCollector::run spawns a tokio::process::Command without kill_on_drop(true), under a tokio::time::timeout — crates/rocm-dash-collectors/src/amd_smi.rs:148-149 (run), called from run_json's timeout(RUN_TIMEOUT, self.run(args))
  This repo's own AGENTS.md states: "a tokio::process child awaited under a timeout sets kill_on_drop(true), so a timeout that fires does not leak it" — and a sibling file, crates/rocm-dash-tui/src/jobs.rs:78, does set it for exactly this reason. A slow-but-not-wedged amd-smi that exceeds RUN_TIMEOUT (10s) leaks a child on every such poll. This is pre-existing (unchanged by this diff, which only touches this file's device_accessible) and the blast radius is not enlarged by this PR's change — the new bounded device-open probe reduces exposure to the D-state hang this PR targets, it doesn't affect this orthogonal slow-process case — so it is not blocking, but it is squarely on this PR's own subprocess-safety theme and in a file this PR otherwise touches.
  Confidence 85 · logic · Fix: add .kill_on_drop(true) to the Command in run.

• The WSL wave-through branch of kfd_readable (is_wsl_host() && has_usable_amd_gpu()) is exercised by no E2E scenario, only by unit tests — crates/rocm-core/src/amd_smi.rs:203-213
  No WSL CI lane runs examine-31 or any other new scenario against this branch; coverage is real but confined to cargo test. Non-blocking since the branch is unit-tested and genuinely hard to reach from the e2e harness without real WSL infrastructure.
  Confidence 70 · architectural · Fix: none required; consider noting the gap in docs/testing.md if not already present.

Decisions for the author

• tests/e2e-cucumber/tests/e2e/serving_steps.rs:310's vram_mib() spawns amd-smi directly, with no /dev/kfd pre-flight and no timeout — answered-by-intent
  EAI-8457/ROCMAI-342's own ticket text explicitly scopes this out: "Two further launch sites exist in test code only — tests/e2e-cucumber/tests/e2e/serving_steps.rs:306 and :321 — and are excluded from the table." Confirmed this is a test-harness helper (probing real VRAM on hardware-backed E2E runs), not production code. Would otherwise have been a non-blocking completeness finding; the ticket already settles that it's intentionally out of scope.

Positive signals

• ProbeBudget::claim (crates/rocm-core/src/amd_smi.rs) uses fetch_update rather than fetch_add+compensating decrement specifically to avoid the momentary over-the-limit state two racing callers could otherwise observe — correct, and the reasoning is inline.
• ProbeSlot is moved into the spawned probe thread's closure so the budget slot releases only when the blocking open() actually returns (or the thread unwinds), verified safe under panic since no workspace crate overrides panic = "abort".
• The blocking-open tests use a real mkfifo'd FIFO with no writer (a genuine blocking open), watched from a second thread with its own looser watchdog deadline, so a dropped production timeout fails the suite with a message instead of hanging it.
• examine-31's stub-plus-marker-file design distinguishes "declined before spawn" from "spawned and failed" non-vacuously — stronger than a naive "amd-smi unavailable" assertion.
• Differentiated per-caller timeouts (10s for serve's probe, 2s for rocmd's watcher poll) are justified by real latency-budget asymmetry documented at each constant, not arbitrary.

Deployment notes

• New first-party crate dependency edge rocm-dash-collectors -> rocm-core, allowlisted in xtask/src/crate_edges.rs and reflected in Cargo.lock.
• KFD_OPEN_TIMEOUT (2s) is now additive to every amd_smi_json caller's declared timeout; the PR's own description documents the resulting worst case (rocm serve --gpu auto rises to ~24s from the prior unbounded .output() call), which replaces an unbounded hang rather than a faster bound, but is worth knowing for anyone with latency expectations around serve startup.

What this covered

Full diff 11a606912ed37887fb527251ea8d037fe3af9d25…741852a8dac7223595ebf80ea8b862fe1ee3569b (15 files, +1288/−88), merge-base-verified against the stated base. Three independent fan-out workers each read their file group completely at the reviewed head: (1) core gate — crates/rocm-core/src/amd_smi.rs, lib.rs, hardware_root.rs, process.rs in full, examine.rs's diff hunk plus its enclosing function (its ~8-line diff sits in a 6769-line file, scoped deliberately rather than read whole); (2) call sites — apps/rocm/src/gpu_policy.rs, apps/rocmd/src/common.rs, crates/rocm-dash-collectors/src/amd_smi.rs, Cargo.toml/Cargo.lock, xtask/src/crate_edges.rs, docs/architecture.md, plus a repo-wide Command::new/amd_smi_json sweep confirming every production launch site is gated; (3) e2e — tests/e2e-cucumber/features/examine.feature, model_serving.feature, examine_steps.rs. The driver independently read the core gate diff directly as well, and verified the two blocking findings by direct git grep/git log cross-reference rather than taking a worker's word alone. CI checked live at the reviewed head: 22 success, 3 skipped, 0 failing/pending. Ticket EAI-8457 (→ ROCMAI-342, status In Progress) and the PR description were both read at Step 9, after findings were settled; no mismatch found between the description's claims and the diff.

Did not run: no Rust toolchain was available in this environment, so cargo test/clippy/xtask coverage could not be executed directly; relied on live CI's green result plus direct code reading instead. History pass performed only for crates/rocm-core/src/amd_smi.rs (confirms an earlier-raised "does the timeout path reap its child" concern was already fixed earlier in this branch's own history, before this diff's base). Prior-changes pass not performed — this PR's own review-comment history was deliberately not read, per this method's independence rule. Not reconciled against any GitHub discussion on this PR. One finding above (the docs/architecture.md contradiction) is already fixed in a later commit on this PR (472efacc) that is outside the reviewed diff range; noted for transparency, it does not change the verdict for the reviewed commit.

---

🤖 Generated with Claude Code

[juhovainio]

I read the whole diff. The core change looks right to me: one guarded amd_smi_json chokepoint, the spawn is provably prevented in the gate test (marker file plus an allowed-path control), serve gains a bound it didn't have, and the decline note is asserted together with the absence of the spawn in examine-31. CI is green on everything that ran, and the scope matches EAI-8457. I did not build or run anything locally, so the mutation-check and MI300X claims in the PR body are unverified on my side.

Requesting changes for a few things worth fixing before merge (details inline):

1. The dashboard collector's device_accessible now calls device_readable_bounded, which parks the caller for up to 2s (twice under a simulated root) and is reached from an async fn. That is better than the old unbounded open, but it still blocks a tokio worker; please wrap it in spawn_blocking or document the stall.
2. The new timeout/budget tests return Ok(()) silently when mkfifo is missing (and on WSL), so they can turn into green no-ops. Please emit a skip notice or assert mkfifo exists on the CI lane.
3. command_failure_detail_or_else is pub but only used inside rocm-core; pub(crate) keeps the public surface from growing.

Smaller notes, no action required if you disagree:

• On a bare-metal Linux host outside the render group, detect_gpu_count now returns None with no sysfs fallback, so --gpu auto makes no selection on multi-GPU hosts. This is documented and the gpu_policy unit tests cover the None paths, but there is no Gherkin scenario for the serve side; worth saying so explicitly in the PR text.
• docs/manual-testing.md is not touched although serve/rocmd probe behaviour changed, and some of the new doc comments are very long.
• Pre-existing and not from this PR: the collector's run() awaits Command::output() under a timeout without kill_on_drop(true).

The latency ceiling (~24s on serve), the permanent probe-budget exhaustion while threads stay parked, and the new rocm-dash-collectors -> rocm-core edge all look like deliberate, documented tradeoffs; I'm not asking for changes there.

[juhovainio]

device_readable_bounded blocks the calling thread (up to 2s, and twice under a simulated root), and this is reached from async fn detect_with_binary_inner via preflight_passes, so it parks a tokio worker. It's better than the old unbounded open, but AGENTS.md says blocking work belongs under spawn_blocking. Consider wrapping the pre-flight in spawn_blocking, or noting the 2s stall here.

[juhovainio]

When mkfifo is missing this test returns Ok(()) silently, so the timeout test becomes a no-op with a green result. The budget test (make_fifo at the second call site) and the is_wsl_host() early returns do the same. Print a skip notice, or assert mkfifo exists on the CI lane, so a skipped check is visible.

[juhovainio]

command_failure_detail_or_else is pub but only amd_smi_json inside rocm-core uses it. pub(crate) would avoid widening the crate's public surface for an internal helper.

[r0x0r]

@juhovainio — all three fixed, plus another main merge. Pushed as 57071f6e.

1. Blocking a tokio worker. Real bug, and mine — the dash-collector change introduced it. AmdSmiCollector::new reached device_accessible → the bounded open from an async fn, parking a worker for up to the 2s KFD_OPEN_TIMEOUT per probe, ~4s under a simulated host root where there are two.

Fixed rather than documented. The pre-flight now goes through preflight_passes_off_thread, which short-circuits the two non-device branches before the thread hop — so a caller that skips the pre-flight, or already knows the GPU is reachable, pays nothing and never needs a runtime with a blocking pool at all — and puts the device branch on spawn_blocking. A JoinError reads as "not readable", matching device_readable_within_budget: every branch that cannot get an answer declines rather than falling back to an open on the caller's thread.

Deliberately no timeout around the spawn_blocking. That would drop only the JoinHandle and leave the blocking thread running; the probe is already bounded from the inside, which is the bound that matters.

2. Tests that could turn into green no-ops. You were right that this was a silent pass. Both the missing-mkfifo(1) path and the WSL path now announce the skip through a shared skip_notice, so a test that asserted nothing says so instead of looking like a pass.

3. command_failure_detail_or_else. Now pub(crate). Its only caller outside its own module is amd_smi_json, in this same crate; external callers that want the precedence rule want the public command_failure_detail, which supplies the fallback for them.

On your smaller notes:

• The --gpu auto / render-group gap is now stated in the PR body as you asked: outside the render group detect_gpu_count returns None with no sysfs fallback, the gpu_policy unit tests cover the None paths, and there is no Gherkin scenario for the serve side.
• kill_on_drop(true) on the collector's run() — agreed, and pre-existing. Left alone here rather than widening this PR; worth a follow-up.
• Doc-comment length: a further pass went out last round (~100 lines of review narration, both citations of a commit hash that does not exist in this repo). What remains is invariant rationale rather than history.

Verification

Linux MI300X box, merged tree applied onto main (6eeec620):

• cargo clippy --workspace --all-targets -- -D warnings — exit 0
• cargo test --workspace --all-targets — exit 0, 75 suites, 0 failures
• cargo xtask check-crate-edges — exit 0
• cargo fmt --all -- --check — exit 0

Locally on macOS, because this round touches cfg-sensitive and async code and a Linux-only check already missed one non-Linux break in this file earlier: cargo check -p rocm-core --all-targets and cargo check -p rocm-dash-collectors --all-targets both exit 0.

cargo-clippy/cargo-test were skipped on the push only because engines/lemonade cannot compile on Darwin (st_mode is u16 there, u32 on Linux); both ran on Linux as above, and every other hook ran.

Unverified here, as before: no amd-smi/rocm-smi and no GPU telemetry on the box, so nothing was tested against a genuinely wedged /dev/kfd.

All three addressed in 57071f6: the dash-collector pre-flight now runs via preflight_passes_off_thread (spawn_blocking, with the two non-device branches short-circuiting before the thread hop) so no tokio worker is parked; the mkfifo and WSL skips now announce themselves through a shared skip_notice instead of returning a silent Ok(()); and command_failure_detail_or_else is pub(crate). The --gpu auto render-group scenario gap is now stated in the PR body. Re-requesting your review.

[juhovainio]

Re-review at 57071f6. The approach holds up: both probe failure branches decline, the probe slot moves into the probe thread, the budget test uses a readable file so only a refusing implementation passes, and examine-31 asserts message, device and marker absence together.

Blocking

1. hardware_root.rs now says the dash crates "deliberately do not depend on" rocm-core, but this PR adds the rocm-dash-collectors -> rocm-core edge (Cargo.toml and the crate_edges.rs allowlist). The doc is false as written. Please reword it, and grep the other docs that repeat the claim.
2. The PR body omits the dash-collector spawn_blocking hop (preflight_passes_off_thread, commit 57071f6) and its two tests. Please update the body from the final diff, rerun the full gate at this head, and confirm the dash tests fail when the target is broken (for example, inlining preflight_passes should make the ticker test record 0 ticks).

Non-blocking

• Nothing fails if the two gpu_policy.rs probes revert to a raw spawn, which is the regression this PR exists to prevent. A structural guard (for example, resolve_amd_smi_binary used only inside rocm-core's amd_smi.rs) would pin it.
• Declines on the serve path are silent (.ok()?), including a false decline if the 2s open bound trips on a slow but healthy host. A debug log of the discarded error would help.
• make_fifo is duplicated across two crates, and the latency figures (12s/24s/4s) are repeated across several docs and the PR body, so they will drift.
• rocmd now passes the label "amd-smi" to run_with_timeout instead of "process"; worth a mention if intentional.

For the author to decide / state

• The gate only proves open(/dev/kfd) returns, while the docs attribute the D-state to amd-smi's later ioctls. No real unhealthy-driver run was done, so please say that this is a mitigation heuristic.
• The decline is broader than the ticket: an absent /dev/kfd and EACCES also decline (the ticket says it does not apply with no /dev/kfd), and detect_gpu_count has no sysfs fallback.

The PR is also currently conflicting with main and needs a rebase.

[juhovainio]

This now says the dash crates "deliberately do not depend on this one", but this PR adds rocm-core to rocm-dash-collectors (Cargo.toml and the crate_edges.rs allowlist). Reword to say the collector shares device_readable_bounded but still receives the re-rooted path from the caller, and grep the other docs repeating this claim.

[juhovainio]

Nothing in the test suite fails if this reverts to a raw Command::new(resolve_amd_smi_binary()), which is the regression this PR exists to prevent, and .ok()? drops the decline silently. Consider a structural guard (e.g. resolve_amd_smi_binary used only inside rocm-core's amd_smi.rs) and/or a debug log of the error.

[juhovainio]

Same as the other probe: a decline here, including a false decline when the 2s open bound trips on a slow but healthy host, is silent. A debug log of the discarded error would make "why did --gpu auto not rank GPUs" diagnosable.

[juhovainio]

The pre-flight only proves open(/dev/kfd) returns; the docs say the D-state comes from amd-smi's later ioctls. Please state in the PR that no real unhealthy-driver run was done and that this gate is a mitigation heuristic.

[juhovainio]

The spawn_blocking hop and the two new tests are not in the PR description. Please update the body, and say whether the ticker test was shown to fail with the hop removed (inlining preflight_passes should yield 0 ticks).

Both blocking items addressed in 116cf18: the hardware_root.rs claim (and its copies in architecture.md and the dash module docs) now says the invariant binds rocm-dash-tui, not the collectors crate; and the PR body is rewritten from the final diff including the spawn_blocking hop. Also implemented your structural guard so a launch site that skips the pre-flight fails the build, logged the silent serve-path declines, and pinned the latency figures in one place. The mitigation-heuristic caveat and the broader-than-the-ticket decline are both stated in the PR body. Re-requesting your review.

[r0x0r]

Both blocking items done, plus the structural guard. Pushed as 116cf186.

1. The hardware_root.rs doc this PR made false. Corrected, and the claim was repeated in more places than that one — docs/architecture.md and the dash module docs carried it too. They now say what is actually true: the invariant binds rocm-dash-tui, the presentation layer, which makes no amd-smi call; rocm-dash-collectors does depend on rocm-core, because it spawns amd-smi on the same device and shares the bounded probe rather than keeping a second copy.

2. PR body and falsification. Body rewritten from the final diff, including the spawn_blocking hop (preflight_passes_off_thread) and its tests, which it had omitted entirely.

Structural guard (your strongest non-blocking point) — implemented. You were right that nothing failed if the two gpu_policy.rs probes reverted to a raw spawn, which is the regression this PR exists to prevent. There is now a build-level guard that fails when a launch site skips the pre-flight, so the property is pinned structurally rather than by convention.

Silent declines on the serve path — also done: the discarded error is now logged, so a false decline from the 2s bound tripping on a slow-but-healthy host is visible rather than indistinguishable from "no GPU".

Latency figures — pinned in one place rather than restated across several docs and the PR body, which is what would have let them drift.

The two things you asked me to state

• This gate is a mitigation heuristic, not a proof. It establishes that open(/dev/kfd) returns within a bound. The D-state hang the docs describe is attributed to amd-smi's later ioctls, and no run against a genuinely unhealthy driver was done — the dev box has no amd-smi and no GPU telemetry. The bounded open is the strongest thing testable here; it is not evidence that the ioctl path is safe.
• The decline is broader than the ticket. An absent /dev/kfd and EACCES both decline, where the ticket says the gate does not apply with no /dev/kfd. detect_gpu_count has no sysfs fallback, so outside the render group --gpu auto makes no selection on a multi-GPU host. The gpu_policy unit tests cover the None paths; there is no Gherkin scenario for the serve side. All three are now stated in the PR body.

Verification

Linux MI300X box, merged tree applied onto main (d7faa84e):

• cargo clippy --workspace --all-targets -- -D warnings — exit 0
• cargo clippy -p e2e-cucumber --test e2e -- -D warnings — exit 0
• cargo xtask check-crate-edges — exit 0
• cargo fmt --all -- --check — exit 0
• cargo test --workspace --all-targets — 1389 passed, 1 failed: therock::family::tests::resolve_family_uses_managed_runtime_before_host_detection, which passes 3/3 in isolation and is an env-var leak between parallel tests in main's own new therock/family.rs module. This branch touches no therock file. Flagging rather than hiding it.

The cargo-clippy/cargo-test pre-push hooks were skipped only because engines/lemonade cannot compile on Darwin (st_mode is u16 there, u32 on Linux); both ran on Linux as above.

The merge you noted was already done before this round; this push merges main again, re-homing the truncate_to_chars doc note into probe_exec.rs where main moved the function.

[juhovainio]

Requesting changes on one item: the PR description doesn't cover part of the diff. The code itself looks sound. The pre-flight is bounded and budget-capped, both failure branches decline instead of falling back to a blocking open, and the FIFO-based tests make the blocking open real, with a control case.

Blocking

• The description never mentions the new xtask static contract test (xtask/src/amd_smi_launch_contract.rs, wired in via xtask/src/main.rs). It also doesn't say that the dash collector's pre-flight now runs under spawn_blocking, with a ~2s timing test. Both are user-relevant parts of the change; please add them to the body.

Non-blocking

• docs/testing.md and the kfd_readable doc disagree about what --gpu auto does when the gate declines (inline comments). Please reconcile with the code and with the detect_gpu_count doc in gpu_policy.rs.
• the_device_preflight_does_not_run_on_the_async_worker uses the real KFD_PROBE_BUDGET and parks one slot for the lifetime of the test binary. The rocm-core tests use private budgets for this reason. It's harmless today but becomes order-dependent if a second FIFO test lands there.
• The contract test's strip_comment truncates at the first // even inside a string, so a call after a URL literal would be missed. It's documented as fail-open, so I'm just noting it.

Please confirm these are deliberate

• The gate declines on any unopenable /dev/kfd, not only on a hang. Hosts with no /dev/kfd (containers) or users outside the render group lose detect_gpu_count even though amd-smi might have answered. This is documented, but it is wider than the ticket's "not applicable when /dev/kfd is absent".
• The new rocm-dash-collectors -> rocm-core edge reverses the earlier "deliberately no dependency" stance and pulls rocm-core's dependency tree into the collectors crate.
• Worst-case serve --gpu auto latency rises to about 24s. It was unbounded before, so this is an improvement.

Nothing here was run against a real unhealthy driver, which the PR says openly. CI is green on everything that had completed when I looked.

[juhovainio]

This says --gpu auto "ranks from the sysfs VRAM rows alone" on a declined host, but the kfd_readable doc in crates/rocm-core/src/amd_smi.rs says auto-selection returns no selection and the engine's own probe picks. Please check against select_auto_gpu_index and make both (and the detect_gpu_count doc in gpu_policy.rs) say the same thing.

[juhovainio]

Counterpart of the docs/testing.md wording: "auto-selection returns no selection and the engine's own device probe picks". Reconcile with docs/testing.md; whichever one matches the code wins.