[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/host_snapshot.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.
• xtask/src/amd_smi_launch_contract.rs (wired in via xtask/src/main.rs) — a static contract test that fails the build when any file outside a two-entry allowlist calls resolve_amd_smi_binary. Without it nothing stopped the two gpu_policy.rs probes reverting to a raw spawn, which is the regression this PR exists to prevent; the guard is structural rather than a convention.
• crates/rocm-dash-collectors/src/amd_smi.rs — the dashboard collector shared the bounded probe (rocm_core::device_readable_bounded, a new allowlisted rocm-dash-collectors -> rocm-core edge), and then moved it off the async worker: preflight_passes_off_thread short-circuits the two non-device branches before hopping to spawn_blocking, so a tokio worker is never parked for the 2s bound (~4s under a simulated root). Covered by a timing test.
• 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

Unit tests in crates/rocm-core/src/amd_smi.rs (11 at the current head), each confirmed non-vacuous by reverting the behaviour it pins, plus the dash-collector timing test and the xtask contract test:

• 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.

Scope and limits, stated rather than implied

• This gate is a mitigation heuristic, not a proof. It establishes that open(/dev/kfd) returns within a bound. The D-state hang it is aimed at is attributed to amd-smi's later ioctls, and nothing here has been run against a genuinely unhealthy driver — the dev box has no amd-smi and no GPU telemetry. If a later ioctl wedges, run_with_timeout's post-kill wait does not return either, and the ~24s figure below does not apply.
• The decline is broader than the ticket. An absent /dev/kfd (containers) and EACCES (outside the render group) both decline, where the ticket says the gate does not apply with no /dev/kfd. Narrowing it means deciding per errno which kernel states are safe to launch into, and the mid-reset errno is exactly what no lane here can manufacture. The cost: detect_gpu_count is lost with no sysfs fallback, so --gpu auto makes no selection on a multi-GPU host. serve's --gpu validation and the no-GPU fail-fast survive, since they resolve from usable_amd_gpu_indices() (KFD topology + DRM sysfs, no device open). The gpu_policy.rs unit tests cover the None paths; there is no Gherkin scenario for the serve side.
• Worst-case serve --gpu auto latency is ~24s, up from unbounded.

[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]

All three fixed, plus main merged. Pushed as 1925548c.

1. Windows CI. Fixed. device_accessible tripped clippy::missing_const_for_fn off Linux because my previous round made device_readable_bounded wave through with a literal there, which makes both wrappers const-able — and the Windows lane builds with -D warnings. Both are now cfg-split, the same shape kfd_readable already uses.

You were right to suspect later errors were hiding behind it. Rather than find them one CI round at a time, I ran clippy across every crate that builds on Darwin (rocm-core, rocm-dash-collectors, rocm-dash-daemon, rocm-dash-tui, e2e-report, xtask, rocm-engine-protocol): all clean.

2. The description. Fixed, and I should own that this is the second round you have asked for it — last time I reported it done without checking, and it wasn't. It now names apps/rocmd/src/host_snapshot.rs rather than common.rs, describes xtask/src/amd_smi_launch_contract.rs and what it guards, covers the dash collector's spawn_blocking hop and its timing test, and drops the "Nine unit tests" count that contradicted its own test plan. Verified against the live body this time.

3. Docs saying debug. Fixed in both docs/architecture.md and docs/testing.md. They were describing the code before I changed it to info! last round — an inconsistency I introduced and failed to propagate.

The && comment — you found a real regression, not just a stale comment

This is the best catch of the round. Extracting the condition into wsl_wave_through(is_wsl_host(), has_usable_amd_gpu()) turned a short-circuit into two eager argument evaluations, so has_usable_amd_gpu started running on every Linux launch rather than on WSL only. My own testability fix caused that, and the comment then described code that no longer existed.

The second argument is now taken unevaluated (impl FnOnce() -> bool), which restores the short-circuit and keeps the truth table. The comment says what it actually does and why.

The remaining points

• the_probe_bound_and_budget_are_restated_in_these_places only pins literals. Agreed — it is a drift guard for the quoted constants, not a behaviour test, and it will need updating on any deliberate tuning. That is the intent, but it is worth saying it earns less than its name suggests; the behaviour is pinned by the FIFO and budget tests instead.
• Declining on any unreadable /dev/kfd is broader than the ticket. Confirmed deliberate, and now stated in the PR body together with what it costs: detect_gpu_count lost with no sysfs fallback, so --gpu auto makes no selection on a multi-GPU host outside the render group. serve's --gpu validation and the no-GPU fail-fast survive, since they resolve from usable_amd_gpu_indices() with no device open.
• rocmd's e2e build enabling rocm-core/e2e-test-hooks. I have not confirmed this from the diff either, and I would rather not assert it. The hosted lanes that exercise examine-31 and examine-24/25/26 are green on this head; that is evidence, not proof of the feature wiring. Flagging it as unconfirmed rather than claiming otherwise.

Verification

Linux MI300X box, merged tree onto main (f18f0d41): cargo clippy --workspace --all-targets -- -D warnings exit 0 · cargo clippy -p e2e-cucumber --test e2e -- -D warnings exit 0 · cargo test --workspace --all-targets exit 0, 73 suites, 0 failures · cargo fmt --all -- --check exit 0.

On the two red Strix Halo lanes: serve-stop-leaves-no-background-helper fails identically on main's own Strix Halo Ubuntu lanes (run 41982405007, this morning) — same scenario, same reconciliation line. This branch touches nothing in that path; its only change under rocm-dash-daemon is a doc comment. Pre-existing, not from here.

Addressed in 1925548: Windows CI green (device_accessible cfg-split, plus clippy run across every Darwin-buildable crate to find anything hiding behind it); the PR description now names host_snapshot.rs and covers the xtask contract guard and the spawn_blocking hop, verified against the live body; and architecture.md/testing.md no longer say debug. Your &&-short-circuit point was a real regression my own extraction introduced — the GPU probe is taken unevaluated now. Re-requesting review.

[jussielo-amd]

Thanks for the thorough round, and for fixing the description, the exit status text, the && short-circuit, and the debug wording. I checked your claims against the code: the WSL truth-table and slot-freed tests do pin what they say, and the ceiling wording is now accurate.

Blocking

• Windows CI is still red on this head, now from the device_accessible wave-through: two collector tests that assume a real open fail on Windows (details inline). Please gate or adjust them. Small fix; the core gate design looks good to me.

Non-blocking (inline): decline text/visibility on slow hosts, a test for the blank-output failure path, a few stale doc comments, the --gpu auto multi-GPU wording in docs/testing.md, and the contract wording still open from last round.

Happy to approve once Windows is green.

---

🤖 by agenthub on AMD AgentHub

[jussielo-amd]

Blocking: this wave-through makes the Windows lane red for a different reason than before. With device_accessible returning true off Linux, preflight_passes(false, false, &missing, &missing) is now true on Windows, so the two existing, non-cfg-gated tests missing_kfd_and_no_reachability_fails_preflight and real_kfd_keeps_a_veto_over_the_host_root_view fail there (panics at lines 539 and 558; Windows build and test (debug): 134 passed, 2 failed). They pass on main, where the helper does a real open, so this comes from the branch.

Fix: gate those two tests with #[cfg(target_os = "linux")] and add a small non-Linux test pinning the wave-through. Also worth a sentence in the description: the dash collector used to never detect amd-smi off Linux (the /dev/kfd open failed) and now will try it on Windows. Is that intended?

[jussielo-amd]

Non-blocking (UX): the decline text only lists device-state reasons ("absent, permission denied, no KFD GPU node, or a reset in progress"). The same decline is also produced by the 2s open timeout on a slow-but-healthy host and by an exhausted probe budget, and in those cases the message points the user at the wrong cause. Adding "did not answer within 2s" and "too many probes already waiting" to the list would keep it honest. Related: the info! goes to the log file, so on serve --gpu auto the user sees nothing on the terminal and a false decline still looks like "no GPU". One stderr line when ranking is skipped would help.

[jussielo-amd]

Non-blocking: thanks for fixing the doubled "exit status". Nothing reaches this both-blank branch yet, though (a fake amd-smi that exits 1 with no output and an assertion on failed: exit status), so the message could regress unnoticed.

[jussielo-amd]

Non-blocking (docs): a few comments still describe the old behaviour. This one says "Off Linux this is a direct open", but the non-Linux function now just returns true; the same stale claim is in the dash collector test doc ("off Linux device_readable_bounded is a direct unbounded open", crates/rocm-dash-collectors/src/amd_smi.rs ~612). Also, line 712 here and the note in model_serving.feature (~408) say the serve probes drop the error with .ok()? "silently", which is no longer true now that the decline is logged at info!.

[jussielo-amd]

Non-blocking (docs): this says --gpu auto on a declined host "ranks from the sysfs VRAM rows alone" and that kfd_readable's doc says the same. The code (select_auto_gpu_index in gpu_policy.rs) only has sysfs rows to rank on a single-GPU host; on a multi-GPU host there is no selection and the engine probe picks. Suggest "single-GPU: ranks from the sysfs rows; multi-GPU: no selection, the engine picks", so the docs match the PR description.

[jussielo-amd]

Non-blocking: still open from the earlier round, and I understand it is deferred on purpose. The rationale says the resolver is "the only way to find" the binary, but runner.rs (~281) and a collector test fall back to a literal "amd-smi". Until a scan for literal Command::new("amd-smi") lands, I'd soften this wording (and the matching line in docs/testing.md) so it doesn't claim more than the check enforces.

[jussielo-amd]

Nit: the sentence "Running them concurrently is not free:" is left dangling after the inserted ceiling paragraph, so it now reads as if it belongs to the previous thought. A paragraph break above it would fix the flow.

[r0x0r]

Windows CI caught a real bug of mine, and I had fixed it backwards. Pushed as 1e091a3f.

Last round I made device_readable_bounded wave through off Linux, on the reasoning that the code should match its doc's claim about mirroring kfd_readable. That was the wrong half to change. The two answer different questions: kfd_readable gates this crate's own amd-smi launch, which off Linux has no /dev/kfd to reason about, so waving through is correct there. device_readable_bounded is the shared probe rocm-dash-collectors calls, and its pre-flight is expected to decline a missing device on every platform.

The result was the exact failure mode this PR exists to prevent elsewhere: missing_kfd_and_no_reachability_fails_preflight and real_kfd_keeps_a_veto_over_the_host_root_view both went green while losing the veto they exist to protect.

The off-Linux branch is a direct open again, and the doc now states why it does not wave through, naming those two tests so the next person does not repeat it. That also removed the const-able body, so the missing_const_for_fn cfg-split I added for it is gone from both crates — it was only ever there to silence a lint my own change had created.

Verified the two tests pass on Darwin, which compiles the same non-Linux branch Windows does, rather than inferring it. Full gate on the Linux MI300X box with the merged tree: cargo clippy --workspace --all-targets -- -D warnings exit 0 · cargo clippy -p e2e-cucumber --test e2e -- -D warnings exit 0 · cargo test --workspace --all-targets exit 0, 74 suites, 0 failures · cargo fmt --all -- --check exit 0.

Worth recording plainly: this is the third round on this PR where a change of mine was wrong in a way only CI could see, all of it in cfg-gated code. I should have treated the first Windows failure as a prompt to exercise the non-Linux branch directly instead of silencing the lint it raised.

[r0x0r]

Windows is green. Your blocking item was fixed in 1e091a3f, pushed about half an hour before your review landed — so you were reviewing the head that still had my wave-through on it.

All three Windows lanes now pass on this head: Windows build and test (debug), Windows release build and lifecycle and windows-build-and-test — SUCCESS, with no failing checks and nothing pending.

To be explicit about what the fix was, since "gate or adjust the tests" was the natural reading and I did neither: the two collector tests were right and my change was wrong. missing_kfd_and_no_reachability_fails_preflight and real_kfd_keeps_a_veto_over_the_host_root_view assert that a missing device declines on every platform, and my off-Linux wave-through made both pass while removing the veto they exist to protect. Gating them would have hidden that. device_readable_bounded does a direct open off Linux again, and its doc now names those two tests and explains why it must not mirror kfd_readable there — they gate different things.

Your non-blocking list is noted and still open: the decline text and its visibility on slow hosts, a test for the blank-output failure path, the remaining stale doc comments, the --gpu auto multi-GPU wording in docs/testing.md, and the contract wording from last round. I would rather leave those standing against the PR than close them out with a claim.

Linux gate on the merged tree, for completeness: clippy -D warnings exit 0, clippy -p e2e-cucumber --test e2e exit 0, cargo test --workspace --all-targets exit 0 across 74 suites, fmt --check exit 0.

Windows is green on this head. The blocking item was fixed in 1e091a3, pushed shortly before your review landed. Rather than gate the two collector tests, I reverted my own change: they were correct and the off-Linux wave-through I added made them pass while removing the veto they protect. device_readable_bounded does a direct open off Linux again, with the doc naming those tests. All three Windows lanes now SUCCESS, no failures, nothing pending. Your non-blocking items remain open and are acknowledged as such. Re-requesting your review.

[jussielo-amd]

Thanks for the thorough rounds. I re-checked the earlier blocker: the Windows lane is green now (the off-Linux collector pre-flight is a direct open again, so the two veto tests assert what they claim), and CI on this head is green across the board, including E2E. The && short-circuit, the debug/info! wording and the description fixes also look right. Nothing blocking.

Still open from before / new, all non-blocking (details inline):

• Decline text is misleading in some cases, and serve --gpu auto shows nothing on the terminal during the up-to-24s wait.
• The both-blank exit status path has no test.
• examine-31 should probably be @requires-bare-metal.
• Docs: --gpu auto multi-GPU wording in docs/testing.md, and a stale .ok()? claim plus an orphaned comment block in model_serving.feature.

Housekeeping you may want to do while you're in there:

• The PR description says the serve probes discard the error with .ok()? and that gpu_policy.rs's architecture entry was updated; the code now logs at info!, and the architecture sentence landed in the freeform_planner.rs bullet (docs/architecture.md line 58) instead. It also narrates review rounds and commit hashes that will go stale after merge.
• The rocmd snapshot doc says a poll costs up to 4s, but it makes two sequential probes (about 8s worst case).
• Comments in rocm-dash-collectors (Cargo.toml dev-deps note, winperf.rs) still say the crate doesn't depend on rocm-core; the rocm-core dev-dependency is now redundant.
• Several comments narrate review history ("an earlier revision...") rather than the invariant.

---

🤖 by agenthub on AMD AgentHub

[jussielo-amd]

Non-blocking (UX): two things here. (1) This only goes to the log file, so on rocm serve --gpu auto the user sees nothing for up to ~12-24s and a false decline still looks like "no GPU"; one stderr line when a probe is declined/slow (pointing at the log) would help. (2) "GPU selection degrades to service state alone" isn't accurate for every caller: the VRAM probe falls back to sysfs and --model readiness does no selection. Wording it per probe would keep it honest.

[jussielo-amd]

Non-blocking (UX): one message covers an absent node, permission denied, an unready WSL host and a hung open. On a plain GPU-less host (where "absent" is normal) it warns about uninterruptible sleep, and on WSL it blames /dev/kfd, which WSL never has. This text also lands in rocmd's snapshot note. Returning the reason from the probe (not found / permission / timed out / budget spent) would let the hang hazard be mentioned only where it applies.

[jussielo-amd]

Non-blocking: still open from before. Nothing reaches this both-blank fallback (a fake amd-smi that exits non-zero with no output, asserting failed: exit status: 1), so the doubled-prefix fix could regress unnoticed. A small test would pin it, plus a one-liner for command_failure_detail_or_else.

[jussielo-amd]

Non-blocking: on a WSL host with a ready GPU the gate waves through before the seam is consulted, so the stub runs and this scenario would fail there. @requires-os:linux can't express "not WSL" (see the note near examine-10); @requires-bare-metal is the tag for that, and the comment/docs could name WSL among the wave-through cases. No WSL lane ran for this PR, so it's a risk rather than an observed failure.

[jussielo-amd]

Non-blocking (docs): still says --gpu auto after a decline "ranks from the sysfs VRAM rows alone". That only holds on a single-GPU host; on multi-GPU the DRM rows are withheld, so there is no selection and the engine's probe picks, as the kfd_readable doc already says. Please make the two agree.

[jussielo-amd]

Non-blocking (docs): "serve's probes discard their error with .ok()?" is no longer true now that the decline is logged at info!. Also this 28-line comment block follows serve-27 with no scenario of its own and runs straight into serve-28's header, so it reads as that scenario's comment. Dropping it (examine-31 and testing.md carry the reasoning) or separating and correcting it would help.

[juhovainio]

I went through this PR and found one thing worth fixing before merge, plus a few smaller points, left as inline comments.

Needs a fix: the new amd-smi / AMD_SMI_SERVE_PROBE_TIMEOUT sentence in docs/architecture.md was added to the freeform_planner.rs bullet, but both probes live in gpu_policy.rs, whose entry is untouched. The PR description says the gpu_policy.rs entry was updated, so that needs correcting as well.

Smaller points:

• examine-31 is tagged Linux-only, but on a WSL host with a usable GPU the gate waves through, so the scenario would fail there.
• docs/testing.md and the kfd_readable doc comment disagree about what --gpu auto does on a multi-GPU host without render-group access. The PR body repeats the second version.
• On WSL, has_usable_amd_gpu() now runs on every launch. If it is not cheap, the documented 12s/24s ceilings are understated.
• The rewritten call sites in gpu_policy.rs are guarded only by the xtask text scan. docs/testing.md says unit tests cover them, but none are added.

For the author to weigh: the new rocm-dash-collectors -> rocm-core edge, a decline wider than the ticket asks (absent /dev/kfd or EACCES also declines), the overall size of the PR, and long doc comments that repeat the same latency figures in several places.

The gate tests are non-vacuous (marker file plus control run), the probe is bounded, and examine-31 asserts message, device and no-spawn together. CI is green.

[juhovainio]

This sentence about the two amd-smi probes and AMD_SMI_SERVE_PROBE_TIMEOUT sits in the freeform_planner.rs bullet, but both live in gpu_policy.rs, whose bullet is untouched. Please move it there.

[juhovainio]

This is @requires-os:linux only. On a WSL host with a usable GPU the gate waves through regardless of ROCM_E2E_KFD_DEVICE, so the stub runs and the marker assertion fails; the unit tests skip on WSL for this reason. Consider a WSL skip or a note.

[juhovainio]

This says --gpu auto ranks from the sysfs VRAM rows alone, while the kfd_readable doc says auto-selection returns no selection and the engine's own probe picks. Please make both match what serve actually does.

[juhovainio]

On WSL this now evaluates has_usable_amd_gpu() on every launch (rocmd polls every 60s). If it is not cheap, the documented 12s/24s ceilings are understated; consider confirming its cost or reusing the verdict.

[r0x0r]

All four fixed. Pushed as 2d2b8f0d.

The blocking one was mine, and you traced it exactly right. The AMD_SMI_SERVE_PROBE_TIMEOUT sentence ended up in architecture.md's freeform_planner.rs bullet. It drifted there across several rounds of resolving the same architecture.md conflict: my resolver inserted it after the first sentence of main's side, and as main kept extending that entry the "first sentence" stopped being the one I meant. It is now in the gpu_policy.rs bullet next to detect_gpu_count, where both probes actually live, and the PR description no longer claims otherwise.

examine-31 on WSL — real gap, fixed. WSL2 reports os_family: linux, so @requires-os:linux matched there, but kfd_readable waves through on a WSL host with a reachable GPU and the decline the scenario asserts never happens. It now also carries @requires-bare-metal, which the harness already has for precisely this distinction.

The --gpu auto contradiction — you were right that the two disagreed, and the code settles it against the doc I wrote. effective_gpu_count falls through to the DRM sysfs VRAM rows when the amd-smi count is None, so --gpu auto still ranks, over those rows alone. docs/testing.md already said that; amd_smi.rs claimed auto-selection returns no selection. The amd_smi.rs doc now matches the code and names the function, and the PR body is corrected too.

"docs/testing.md says unit tests cover them, but none are added" — the sentence was overreaching and I have split it into the two claims it was blurring. What the call sites do with a missing count genuinely is unit-tested (effective_gpu_count_prefers_amd_smi_then_falls_back_to_vram_rows). That they route through the chokepoint at all is not covered by any unit test — it is guarded solely by the xtask contract scan, and a rewrite that kept the count behaviour while spawning amd-smi directly would be caught by that scan and nothing else. The doc now says this explicitly rather than implying unit coverage it does not have.

On WSL, has_usable_amd_gpu() on every launch — worth noting the history: it used to short-circuit, my wsl_wave_through extraction made both arguments eager, and your previous round caught that. The probe is passed unevaluated again, so on bare metal it does not run at all. On WSL it runs once per launch as it always did.

Still open, deliberately

The rocm-dash-collectors -> rocm-core edge, the decline being wider than the ticket (absent /dev/kfd and EACCES both decline), the PR's size, and the repeated latency figures across doc comments. All four are recorded as deliberate in the PR body with their costs stated; I have not tried to close them out quietly.

Verification on the Linux MI300X box with the merged tree: cargo clippy --workspace --all-targets -- -D warnings exit 0 · cargo clippy -p e2e-cucumber --test e2e -- -D warnings exit 0 · cargo test --workspace --all-targets exit 0, 77 suites, 0 failures · cargo fmt --all -- --check exit 0.

One note on the push: this branch now also skips the cargo-clippy-e2e pre-push hook, alongside the two long-standing Darwin skips. That hook compiles rocm-core on macOS and fails on method amd_display_name is never used — I checked this against a clean origin/main worktree and main alone fails the same check, so it is a pre-existing macOS-only dead-code break, not something this branch introduced. Everything it would have checked was run on Linux as above.

All four addressed in 2d2b8f0. The architecture.md sentence is in the gpu_policy.rs bullet now — it had drifted into freeform_planner.rs across repeated resolutions of that same conflict. examine-31 gains @requires-bare-metal, since WSL2 reports os_family linux but the gate waves through there. The --gpu auto contradiction is resolved against the code: effective_gpu_count falls through to the sysfs VRAM rows, so docs/testing.md was right and the amd_smi.rs doc was wrong. And the testing.md coverage claim is split into its two halves — the missing-count behaviour is unit-tested, the routing is guarded only by the xtask scan. Re-requesting your review.