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

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.

Two consequences worth naming

1. Linux hosts that cannot open /dev/kfd. The two serve probes now decline to launch, where before they launched and usually failed anyway. GPU auto-selection degrades to service state alone — what it already does whenever amd-smi is unavailable. This is the trade-off the dash collector has shipped with all along.

2. run_command_with_timeout now drains both pipes on their own threads. This is not gratuitous: polling try_wait without draining deadlocks once the child fills the 64 KiB pipe buffer, and amd-smi metric --json clears that easily on a multi-GPU host. Routing the serve path through the helper without this fix would have traded the hang for silent loss of VRAM telemetry on exactly the machines that matter. Verified: with the old body, a child emitting 300 KB is reported as having hung after the full timeout.

Its timeout message no longer quotes partial output, because collecting that means waiting on a child that may be the unkillable one. On timeout it now kills and returns instead of joining the child in its wait.

AGENTS.md §3: why consequence 1 ships without a Gherkin scenario

Consequence 1 is user-observable, so §3 asks for a scenario and says a unit test on the internal helper does not discharge it. It is not covered by a scenario, and it cannot be on any lane this suite runs. Stating that here rather than leaving the requirement silently unmet; the reasoning is also recorded 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 anyone adding GPU-selection coverage rather than only in this PR description.

The premise needs 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, where world-readable sysfs still enumerates the device that open(2) then refuses. No lane is that host:

• the no-GPU mock lane never reaches either probe. usable_amd_gpu_indices returns an authoritative Some([]) there (linux_kfd_gpu_node_count_in reads "topology unreadable AND no /dev/kfd" as a hard zero), and serve fails fast on that before gpu_vram_usage/detect_gpu_count. That fail-fast is @id:serve-no-gpu-fails-fast, which passes on every PR — so the code under discussion is unreachable on the blocking lane, not merely untested there.
• @requires-gpu lanes have a readable /dev/kfd by construction — every other GPU scenario depends on it — so the declined branch never fires.
• @requires-wsl exercises the wave-through, i.e. the opposite branch.

Nor can the harness manufacture the premise: KFD_DEVICE is a compile-time constant with no path override, the node is root-owned so the harness can neither remove it nor make it unreadable, and supplementary group membership cannot be dropped without privilege. 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.

So this is deliberately left to the unit coverage below, with the_kfd_gate_stops_amd_smi_before_it_is_spawned asserting the observable part that can be pinned: that the gate stops the spawn. Happy to add the scenario the moment a lane gains a container with sysfs GPU topology and no /dev/kfd passed through.

Why the fourth site is left alone

The ticket suggested routing the dash collector through the shared helper too. It already runs the pre-flight, it needs an async launcher so the sync helper does not fit, and an edge from rocm-dash-collectors to rocm-core is precisely what the crate_edges layering guard exists to force review on (its documented intent keeps the dash crates decoupled from rocm-core; rocm-dash-tui is explicitly barred). I tried the edge, the guard caught it, and I backed it out.

Separately noted, and since resolved upstream. This PR originally flagged that rocm-dash-collectors' kfd_accessible() was a bare open("/dev/kfd") with neither a cfg(target_os) gate nor the WSL wave-through added here on the serve path, so AmdSmi::detect() returned None and the dash silently showed no GPU on WSL. #375 ("fix(dash): detect amd-smi through WSL DXG", EAI-8450, on main as d37cfbab) fixes it: kfd_accessible() is gone, replaced by preflight_passes(skip_device_preflight, gpu_reachable, KFD_DEVICE) (crates/rocm-dash-collectors/src/amd_smi.rs:154), which short-circuits on a precomputed reachability verdict before falling back to the device open. apps/rocm/src/dash.rs:74 supplies that verdict from rocm_core::has_usable_amd_gpu(), scoped so it may only substitute for the open on WSL. No follow-up ticket is needed, and the layering/sync-vs-async reasoning above is why it was correctly solved there rather than by widening this PR.

Tests

Every new test was confirmed non-vacuous by reverting the fix and watching it fail:

• the_kfd_gate_stops_amd_smi_before_it_is_spawned (Linux) — a fake amd-smi leaves a marker file; the gate must stop the spawn, not report a failure afterwards. The same fixture is then run against a readable device to prove the fake would otherwise have executed.

• a_child_that_outgrows_the_pipe_buffer_is_not_reported_as_hung (unix) — 300 KB on both pipes must not read as a hang.

• the_kfd_preflight_waves_through_hosts_that_have_no_kfd (non-Linux) — guards the Windows lane against the regression described above.

• the_kfd_preflight_waves_through_wsl_hosts (Linux) — pins the runtime WSL wave-through, with the bare-metal case as its control.

• a_timed_out_child_is_reaped_rather_than_left_a_zombie (Linux) — the child records its own PID and sleeps past the timeout; the test then polls /proc/<pid>/stat until the entry is gone.

  That last assertion was assert_ne!(state, Some('Z')), which was satisfied by two different outcomes: a properly reaped child, and a child that was never killed at all (state S). It now asserts state == None, and the poll loop retries on any surviving state rather than only on Z, so neither the post-SIGKILL teardown transient nor the detached reaper can be read as a pass. Re-verified by mutation on an MI300X box:

  Mutation	Result
  unmutated	pass (0.30 s)
  child.kill() removed	fail — left: Some('S')
  reap_in_background(child) removed	fail — left: Some('Z')

  The old assert_ne! passed under the kill-removal mutation. 100 repeat runs of the shipped form were green, so None is not flaky.

Docs

No doc edit required: none of kfd_accessible, run_command_with_timeout, or the launch sites appear outside these source files.

Test plan

Run on the MI300X dev box; macOS cannot build the product.

• cargo check --workspace --all-targets
• cargo clippy --workspace --all-targets -- -D warnings — clean
• cargo test --workspace --all-targets — all pass, including the crate_edges layering guard
• cargo xtask e2e — 140/142 scenarios pass. The 2 failures (examine-version, serve-hf-checkpoint-inference) were re-run against unpatched main on the same box and fail identically: a detached-worktree git ref, and a missing libdrm_amdgpu.so.1. Both environmental, neither a regression.
• Windows lane via CI (cannot be exercised locally)

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

[juhovainio]

This is a solid, well-tested fix for the three previously-bypassing amd-smi launch sites in ROCMAI-342/EAI-8457 -- the KFD pre-flight gating, pipe-draining, and detached-reaper logic are all independently verified correct, and the dash collector already had its own equivalent gate from #375. CI is green.

Requesting changes on three things, left as inline comments below:

1. There's a third, un-migrated copy of the exact bug this PR fixes. apps/rocmd/src/sandbox.rs::run_process_with_timeout still polls without draining stdout/stderr and calls command_failure_detail directly -- the same pipe-buffer-deadlock-misreported-as-hang vulnerability this PR fixes everywhere else. It traces back to the same commit (e760d9a5) that originally split this logic into 3 places, but the ticket only named 2 of them, so this third copy got missed. See the inline comment on the now-consolidated helper for where it should route to instead.
2. Possible ~20s latency stack on --gpu auto. gpu_vram_usage_amd_smi (up to 10s) and detect_gpu_count (up to another 10s) both go through the new AMD_SMI_SERVE_PROBE_TIMEOUT-gated helper, and if the caller invokes them sequentially rather than concurrently, an interactive rocm serve --gpu auto launch could stack both timeouts back-to-back in exactly the "amd-smi passes the KFD gate but then genuinely stalls" case this PR targets. Worth confirming the actual call site runs these concurrently (or documenting why the latency is acceptable) before merge.
3. The gate's own core assumption isn't verified against the hazard it exists to prevent. kfd_readable's doc comment assumes fs::open() on a resetting /dev/kfd always returns quickly and that only amd-smi's later ioctls can enter D-state. Nothing in this PR (or its tests) verifies that against a real reset-in-progress device. If open() itself can block during a reset, this relocates the hazard into a synchronous, un-timed-out call with less protection than the subprocess spawn it's guarding against.

Two smaller non-blocking items, one left inline, the rest noted here: the new subsystem (~320 lines of gate/launcher/timeout machinery) landed in lib.rs rather than the purpose-built process.rs that a recent PR (#599) extracted this exact category of code into -- docs/architecture.md carves out an exception for lib.rs's amd-smi code, so this is a judgment call rather than a clear violation, but worth a second look. Also: the two timeout constants (AMD_SMI_SERVE_PROBE_TIMEOUT=10s vs the pre-existing AMD_SMI_PROBE_TIMEOUT=2s) diverge 5x between the two apps that now share one helper, with nothing explaining why; and AMD_SMI_SERVE_PROBE_TIMEOUT's doc comment calls itself serve-only but it also gates detect_gpu_count on the unrelated diagnose/--model path.

None of this is a design problem with the approach -- the gate itself, the pipe-drain fix, and the reaper logic are all sound; these are coverage gaps and unverified assumptions around it.

[juhovainio]

This is now the one consolidated run_command_with_timeout, but apps/rocmd/src/sandbox.rs::run_process_with_timeout (not touched by this PR) still has its own separate copy of the same no-pipe-draining, raw-command_failure_detail pattern this consolidation was meant to retire. Same bug class as the one this PR fixes at the other call sites -- worth routing it through this helper too, or explicitly scoping it out with a tracking note if there's a reason it can't.

[juhovainio]

This and gpu_vram_usage_amd_smi above it each wait up to AMD_SMI_SERVE_PROBE_TIMEOUT (10s). If select_gpu_indices_under_launch_lock's caller invokes them sequentially for --gpu auto, that's up to ~20s added to an interactive launch in exactly the case this PR is meant to handle gracefully (amd-smi passes the KFD gate but then stalls). Worth confirming these run concurrently at the call site, or documenting the latency tradeoff if they don't.

[juhovainio]

This function's safety case rests on fs::open() on /dev/kfd always returning quickly even mid-reset, with only amd-smi's later ioctls able to enter D-state. That assumption isn't verified anywhere in this PR -- no test exercises a real reset-in-progress device. If open() itself can block during a reset, this relocates the D-state hazard into a synchronous, un-timed-out call with less protection than the subprocess spawn it's gating.

[r0x0r]

You were right that the safety case rested on an unverified assumption, so the assumption has been removed rather than argued for.

The open is now bounded. KFD_OPEN_TIMEOUT is 2s (crates/rocm-core/src/lib.rs:549) and device_readable_within (:577) performs the open on a probe thread, so an open that blocks — for any reason, reset or otherwise — reads as not readable and the gate declines. That is the conservative direction: a blocked open now produces a refusal to spawn, not a synchronous hang inside the gate. The doc at :477 states this.

I want to be precise about what the accompanying test does and does not prove. the_kfd_probe_declines_a_device_whose_open_blocks_instead_of_hanging opens a FIFO whose open blocks until a writer arrives, so it demonstrates the bound — that a blocking open is converted into a decline within the timeout rather than hanging the caller. It says nothing about /dev/kfd mid-reset specifically, and I am not claiming it does; we have no way to drive a real device into that state in CI. The argument is that the bound makes the question moot: whether or not open() can block during a reset, the gate no longer depends on it returning quickly.

[juhovainio]

Non-blocking: this reimplements command_failure_detail's stderr/stdout precedence logic inline instead of calling that existing function (still used elsewhere in this file), so the same precedence rule is now maintained in two places.

[r0x0r]

Addressed — the precedence rule now lives in one place.

The stderr/stdout precedence was extracted into command_failure_detail_or_else (crates/rocm-core/src/lib.rs:239), which takes the fallback as a closure. command_failure_detail (:226) is now a one-line delegation to it with || "no output".to_owned(), so existing callers are unchanged, and the amd-smi site that prompted this (:694) calls the shared helper with its own bounded, quote-truncating fallback instead of reimplementing the rule.

So there is one implementation of "prefer non-empty stderr, else non-empty stdout, else fall back", and what differs between the two call sites is only the fallback itself — which is the part that genuinely needed to differ. The three existing precedence tests (command_failure_detail_prefers_nonempty_stderr, ..._falls_back_to_stdout_when_stderr_is_whitespace_only, ..._reports_no_output_when_both_are_empty_or_whitespace, :1396-1408) exercise it through the public wrapper, so they cover both paths.

[r0x0r]

Thanks @juhovainio — four of the five items were real and are fixed in 983e11dd..2b42e9e3; one claim doesn't hold at this head. Per finding:

1. Third un-migrated copy in apps/rocmd/src/sandbox.rs — real, fixed.
Confirmed: run_process_with_timeout polled try_wait with both pipes piped and undrained, then called command_failure_detail on the timeout path — the same defect, from the same e760d9a5 split. It now delegates to rocm_core::run_command_with_timeout, so the workspace is back to one implementation (grep -rn "fn run_command_with_timeout" → 1 hit). Two message changes come with it, both deliberate and documented at the call site: the "sandbox process" framing moves from the bail! to .context(...), and the timed-out child's partial output is no longer quoted, because the helper refuses to join the reader threads on that path precisely so an unkillable child can't block them.

2. ~20s latency stack on --gpu auto — real arithmetic, accepted and now documented.
They do run sequentially: gpu_vram_usage() before the launch lock, then detect_gpu_count either in the --gpu <index> validation (serve_cmd.rs:709) or inside the lock for --gpu auto (serve_cmd.rs:761). So ~20s worst case, as you traced. Two things make that the right trade rather than a regression, and both are now on AMD_SMI_SERVE_PROBE_TIMEOUT: at the merge base both probes called .output() with no timeout at all (git show 10345e7e:apps/rocm/src/main.rs), so 20s is a ceiling replacing "forever"; and parallelising isn't free — detect_gpu_count is invoked inside select_gpu_indices_under_launch_lock specifically so the cpu-only and pinned paths never pay for the subprocess, so hoisting it out to overlap would make every rocm serve --cpu-only spawn an amd-smi it has no use for. Unconditional cost to shave a rare stall.

3. kfd_readable's open-returns-quickly assumption — real, and removed rather than asserted.
You're right that it was unverified and unverifiable from a test, and right about the consequence: if open() could block, the gate relocated the hazard into a synchronous call with less protection than the spawn. Rather than argue the assumption, the probe no longer depends on it. device_readable now runs the open on its own thread bounded by KFD_OPEN_TIMEOUT (2s); an overrun reads as "not readable", i.e. the declining answer, so the unanticipated case fails safe. Pinned by the_kfd_probe_declines_a_device_whose_open_blocks_instead_of_hanging, which opens a FIFO with no writer — an open that really does block in the kernel, not a simulation — and watches it from a second thread so an unbounded mutant fails readably instead of hanging the suite. Verified non-vacuous: mutating recv_timeout(timeout) → recv() fails the test in 6s with left: Err(Timeout); restored and re-run green.

4. (non-blocking) command_failure_detail precedence duplicated — real, fixed.
Extracted into command_failure_detail_or_else, which command_failure_detail now delegates to. Only the both-blank fallback differed between the two callers ("no output" vs exit status <code>), so that is the parameter; the stderr→stdout rule lives in one place again.

5. (non-blocking) 2s vs 10s divergence — real, now explained on both constants: rocmd's probe runs on the watcher's polling cadence where abandoning costs one stale tick and the next poll retries, while serve gets one shot at the GPU layout before placing a model.

6. One claim I can't reproduce. The note that AMD_SMI_SERVE_PROBE_TIMEOUT "also gates detect_gpu_count on the unrelated diagnose/--model path" doesn't hold at b7ddd06f: detect_gpu_count has exactly two call sites workspace-wide, serve_cmd.rs:709 and :761, both inside serve_cmd::serve. The doc's serve-only wording was accurate; I've kept it and made the scope explicit rather than hedging it.

7. (non-blocking) lib.rs vs process.rs — left as is. docs/architecture.md:88 keeps amd-smi code in lib.rs as a named, not-yet-scoped concern, and the header comment at crates/rocm-core/src/lib.rs:10 already records why the Read/Command imports came back after #599. Moving it would also collide with #526, which is landing first and builds a competing subprocess-timeout abstraction — converging the two is follow-up work, and doing half of it here would make that harder.

Devbox verification at 2b42e9e3 (112-core Linux): cargo clippy --workspace --all-targets -- -D warnings → 0; cargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings → 0; cargo test --workspace --all-targets --no-fail-fast → 33 groups, 1 failure, therock::archive_properties::sdk_tarball_unpack_extracts_a_benign_tarball_into_the_install_root with failed to launch tar: No such file or directory — the known fork/exec load flake on this box, passes in isolation (tar is at /usr/bin/tar). Re-requesting your review.

Superseded: head has moved 7c8202b -> 4d77efa across two merges of main and the sandbox-helper consolidation. Dismissing the stale state; the current review from juhovainio at @b7ddd06f remains open and is the one being worked.

[r0x0r]

Merged origin/main into this branch (51b77143) and converged on the API #526 introduced. No rebase, no force-push.

What #526 changed. It deleted rocm_core::run_command_with_timeout outright and replaced it with rocm_core::process::run_with_timeout(command, timeout, label) — a third label argument that names the subject in the helper's error text, and a rewrite that sets the child's stdio itself (stdin(Stdio::null()), both pipes piped) with docs saying callers "need not — and should not — set them".

The silent part of the merge. crates/rocm-core/src/lib.rs auto-merged with no conflict marker and kept this branch's copy of the deleted run_command_with_timeout, plus its reap_in_background / drain_on_thread / join_drained helpers and the Read/Stdio/Instant imports they needed. Git silently resurrected a function main deliberately removed. Established by diffing the merge result against both parents and reading the hunk, then confirmed by compiling. Resolved by hand: the definition and its three private helpers are deleted, and the import block is back to Command only (with Stdio/thread now cfg-gated to the Linux-only KFD probe and its test, so the Windows -D warnings lane stays clean).

Call sites converted.

Site	Now
crates/rocm-core/src/lib.rs::amd_smi_json_with	process::run_with_timeout(command, timeout, "amd-smi"); the local .stdin/.stdout/.stderr setup removed, since the helper owns it
apps/rocm/src/main.rs::run_rocm_capture_for_paths	main's run_with_timeout(command, timeout, "process")
apps/rocmd/src/common.rs::run_rocm_capture_for_paths	main's run_with_timeout(command, timeout, "process"); the stdio block it used to set is gone
apps/rocmd/src/common.rs::capture_amd_smi_json	kept this branch's rocm_core::amd_smi_json(args, AMD_SMI_PROBE_TIMEOUT) rather than main's inlined spawn — otherwise the rocmd telemetry probe would be an amd-smi launch site with no /dev/kfd pre-flight, which is the whole point of this PR
apps/rocmd/src/sandbox.rs	see below

Labels follow the house style main established: "process" for the generic rocm captures, "sandbox process" for the bubblewrap child. "amd-smi" is the one new label, naming its subject the same way.

The sandbox.rs wrapper is deleted, not kept. run_process_with_timeout had exactly two jobs, and run_with_timeout now does both: it sets the stdio the wrapper used to set, and it takes "sandbox process" as its label, which is precisely the framing the wrapper's .context("sandbox process failed") was restoring. A thin wrapper that forwards and adds nothing is worse than the direct call, and the call site had already auto-merged to main's direct run_with_timeout(command, Duration::from_mins(1), "sandbox process"), so keeping the wrapper would have left it dead. Its doc comment went with it. apps/rocmd/src/sandbox.rs is now byte-identical to origin/main.

Behavioural difference, stated rather than papered over. The old helper killed a timed-out child and handed the wait to a detached reaper thread, so the caller returned immediately even if the child was in uninterruptible sleep. run_with_timeout kills and then waits synchronously (wait_bounded). For a signalable child the outcome is identical — reaped, no zombie, which is why the a_timed_out_child_is_reaped_rather_than_left_a_zombie test is dropped rather than ported. For a D-state child, the new helper's wait does not return where the old one did. That is a real narrowing, and it is survivable here for the reason this PR exists: the /dev/kfd pre-flight is the mitigation for the D-state case and the timeout was only ever defence in depth. amd_smi_json's doc comment now says exactly that instead of pointing at the detached reaper.

Other reconciliations.

• Dropped a_child_that_outgrows_the_pipe_buffer_is_not_reported_as_hung — process.rs owns a_child_that_floods_both_pipes_does_not_deadlock, same property, better fixture.
• examine.rs::truncate_to_chars — both sides rewrote the pub(crate) rationale; merged into one that names both consumers (crate::process and lib.rs's amd-smi quote).
• common.rs::gather_gpu_snapshot's note comment now records that the pre-flight decline is the one failure that arrives without the "failed to launch amd-smi" context, and points the bound at both MAX_QUOTED_OUTPUT_CHARS constants.
• tests/e2e-cucumber/features/model_serving.feature merged cleanly and needed no renumbering: fix(rocmd): drain child pipes concurrently so output size stops faking timeouts #526 only reworded scenario 06's comment and this branch only appends a trailing KNOWN GAP note, so the file is still serve-01..serve-24, sequential and gapless. The three unit tests that note names all still exist.

Verification (Linux devbox, rust-toolchain.toml's 1.96.0, CARGO_TARGET_DIR under /tmp):

• cargo check --workspace --all-targets — clean, 0 warnings
• cargo test --workspace --all-targets --no-fail-fast — 3942 passed, 1 failed; the failure is therock::archive_properties::sdk_tarball_unpack_extracts_a_benign_tarball_into_the_install_root (failed to launch tar), a known load-flake, re-run in isolation: 2 passed; 0 failed
• cargo clippy --workspace --all-targets -- -D warnings — clean
• cargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings — clean (run separately; harness = false means --all-targets skips it)

This is the merge and the API migration only. It does not touch @juhovainio's open AMD_SMI_SERVE_PROBE_TIMEOUT / detect_gpu_count review thread, and no review has been dismissed or re-requested.

[r0x0r]

Merged origin/main (03cdbccd) in 5f392d88. No conflicts.

This branch was 5 commits behind main, which matters for reading its red lanes: it was missing #675 (fall back to /usr/lib/os-release when /etc/os-release is absent), #665's chat.rs extraction, #464, #465 and #576. The serve-01/serve-02 failures on the Linux and WSL2 lanes show an empty cli_output, which is a launch/environment failure rather than a text mismatch — exactly the shape an unreadable os-release produces on a container that lacks /etc/os-release. Re-running the lanes against current main is the cheapest way to find out whether those two were ever this PR's.

What is not explained by staleness, and still needs a decision:

• examine-24/25/26 fail on both the Linux and WSL2 lanes with declined to launch 'amd-smi static -a -g all --json': /dev/kfd could not be opened for reading. Those three scenarios arrived from fix(rocmd): drain child pipes concurrently so output size stops faking timeouts #526 and stub amd-smi; this PR gates every amd-smi launch on the KFD pre-flight, and CI runners have no /dev/kfd.
• Planting a KFD node under ROCM_CLI_TEST_HOST_ROOT cannot resolve it: kfd_readable already applies host_path itself and requires the real device readable too — wsl_gpu_reachable || (device_readable(real_device) && (host_device == real_device || device_readable(host_device))). Its own doc states the invariant deliberately: a simulated E2E root "can make the gate decline on a machine that has a GPU, but can never make it allow a launch on a machine that does not."
• On WSL2 there is also examine-08: json has_amd_gpu=false, human detected a gfx target=true. That is the product disagreeing with itself — the JSON form loses the GPU when the gate declines amd-smi while the human form still derives a gfx target. Note kfd_readable's WSL wave-through is is_wsl_host() && has_usable_amd_gpu(), so the gate should pass on WSL2; that it declines suggests the wave-through and the human-form detection disagree about this host.

Separately, the Strix Halo Windows lane's four failures are not this PR's: PR #683 — an unrelated ASCII-punctuation change — fails on the identical four scenarios, including setx must not appear now that the write goes through PowerShell, where the actual write is correct and the setx being caught is the catalog's own advice string at crates/rocm-core/src/fix.rs:377.

Gate at 5f392d88 (Linux devbox): cargo test --workspace --all-targets --no-fail-fast exit 0; cargo clippy --workspace --all-targets -- -D warnings exit 0; cargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings exit 0; cargo fmt --all clean. Merge commit is GPG-signed and carries its DCO sign-off. #665 moved 761 lines into a new chat.rs in this merge, so the compile is what confirms nothing was silently dropped.

This touches none of the open review discussion.

[r0x0r]

@juhovainio — worked through all three blocking items at c1ad263d. Two of them no longer hold at this head and I've laid out the evidence rather than changing code; the third was real and is fixed. The E2E regression the lane was red on is fixed too, by a different route than the obvious one.

1. sandbox.rs::run_process_with_timeout — gone from the tree

Your read of the defect was right; it was fixed by a different PR in between. #526 (07166450, "fix(rocmd): drain child pipes concurrently so output size stops faking timeouts") deleted all three copies of that loop — rocmd's run_command_with_timeout and run_process_with_timeout, and rocm's in main.rs — and replaced them with one rocm_core::process::run_with_timeout. That landed on main after you wrote the comment and reached this branch through the merge.

Today sandbox.rs spawns through exactly one call, apps/rocmd/src/sandbox.rs:121:

rocm_core::process::run_with_timeout(command, Duration::from_mins(1), "sandbox process")?

command_failure_detail no longer appears anywhere in that file — parse_sandbox_child_output (:943) works from the already-collected Output. And the helper takes both pipes and starts their reader threads before the wait (crates/rocm-core/src/process.rs:850-851), so the pipe-buffer hazard is gone there for the same reason it is gone at the sites this PR converted. There's a note at sandbox.rs:26-27 recording that the spawn now goes through that helper. Nothing left to route.

2. ~20s on --gpu auto — sequential, deliberately, and now documented

They do run sequentially: gpu_vram_usage before the launch lock (serve_cmd.rs:698), detect_gpu_count inside it (select_gpu_indices_under_launch_lock, main.rs:10450). Documented at the constant (main.rs:10795-10808) rather than parallelised, for a reason that only shows up at the call site: detect_gpu_count is invoked inside the lock precisely so the cpu-only and --gpu <index> paths never pay for that subprocess. Hoisting it out to overlap with the VRAM probe would make every rocm serve --cpu-only spawn an amd-smi it has no use for. The stall is rare; the extra spawn would be unconditional. And 20s is a ceiling replacing "forever" — both probes previously called .output() with no timeout at all, so the host that now costs 20s used to hang outright.

Your non-blocking note on that constant's doc was right, and I've fixed it in the same place. It claimed serve-only; gpu_vram_usage_amd_smi is also reached from host_accelerator_memory → assess_model_with_host_paths, i.e. the --model readiness path (one probe there, so 10s not 20s). detect_gpu_count really is serve-only (serve_cmd.rs:709,761).

3. The open() assumption — removed, not argued

You were right that the gate's safety case cannot rest on an unverifiable claim about kfd_open, and it no longer does. The probe is bounded: device_readable_within (crates/rocm-core/src/lib.rs:577) runs the open on its own thread and the caller waits on a channel for KFD_OPEN_TIMEOUT (2s, :549). An open that has not returned by then reads as not readable, which is the declining answer — so the case neither of us can verify fails in the safe direction, and the assumption stops being load-bearing.

To be precise about what the test proves, because it is less than the name suggests: the_kfd_probe_declines_a_device_whose_open_blocks_instead_of_hanging (:1103) uses a FIFO with no writer — a device whose open genuinely blocks in the kernel. That proves nothing about /dev/kfd mid-reset; nothing short of a wedged device could. What it pins is the bound: a mutant that drops the timeout and opens directly hangs the test rather than passing it, and the same fixture re-runs against an ordinary readable device so the blocked case cannot pass vacuously. The claim being made is "the gate never inherits a blocking open", not "kfd_open is fast".

The E2E regression, and AGENTS.md §3

The E2E tests lane was red on three scenarios — examine-24/25/26, #526's pipe-drain regression tests. (serve-01/02 also fail there, but they are declared xfails for EAI-7219; the reconciliation line reads 2 xfail (failed as expected) … 3 unexpected failure(s).) Cause: those scenarios plant a stub amd-smi, and the per-PR lane has no /dev/kfd, so the pre-flight refused the launch before the stub ever ran.

ROCM_CLI_TEST_HOST_ROOT cannot open that gate. kfd_readable_with keeps the real node's veto on purpose, so a simulated root can make the gate decline on a machine that has a GPU but can never make it allow on one that does not. Tagging the three @requires-gpu was the other option and I rejected it: examine.feature says in terms that they are built to need no GPU so they run per PR, and a GPU tag would strand #526's regression tests on a nightly lane.

So: ROCM_E2E_KFD_DEVICE, honoured only by an e2e-test-hooks build (rocm_core::preflight_kfd_device, lib.rs:451), which moves the device the gate opens. An override, not a bypass — the probe still runs against the named path, and a shipped build has no override at all; the_e2e_kfd_path_seam_moves_the_gate_without_disabling_it pins both halves.

That also discharges AGENTS.md §3, which this PR had only unit tests for. examine-29 points the pre-flight at an absent device and asserts the bridge snapshot explains itself, names that device (so a build ignoring the seam fails rather than passing on the runner's own missing /dev/kfd), and — the load-bearing half — that the stub amd-smi, which would have answered perfectly, never ran. I also corrected the KNOWN GAP note in model_serving.feature, which had drifted into claiming more than was true.

Remaining non-blocking items

• process.rs vs lib.rs: the timeout machinery did move. lib.rs no longer carries a run_command_with_timeout; it calls crate::process::run_with_timeout. What stayed is the KFD gate and amd_smi_json — amd-smi-resolution-adjacent code, the category docs/architecture.md:93 still lists as living in lib.rs.
• Two timeout constants: both now carry why they differ (main.rs:10773, apps/rocmd/src/common.rs:28).
• command_failure_detail duplication: amd_smi_json_with goes through command_failure_detail_or_else (lib.rs:239), so the stderr/stdout precedence rule has one home again.

Verified on a Linux box at this head: cargo test --workspace --all-targets --no-fail-fast (one known load flake, cli_progress::animated_spinner_keeps_ticking_without_progress_calls, passes in isolation), cargo clippy --workspace --all-targets -- -D warnings clean, cargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings clean, and cargo xtask e2e -- --name "examine-2[4569]" → 4 scenarios, 4 passed.

[jussielo-amd]

🔴 Automated review · pr-review-watcher · c1ad263

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

Review — needs work

Full review of the whole change (PR #447, ROCm/rocm-cli). Implements three of the four amd-smi launch-site fixes described in its linked ticket (Jira ROCMAI-342, filed under the bare ID EAI-8457); the fourth launch site (the rocm-dash-collectors collector) is deliberately left unrouted through the new shared gate, for reasons given in the PR description — see Blocking/B2 for why that reasoning understates the gap it leaves open.

Blocking

• Fabricated commit-hash citations — crates/rocm-core/src/lib.rs:490, crates/rocm-core/src/lib.rs:1020
  Both new doc comments cite b4f98bc2 as the commit that "replaced a bare identity check with has_usable_amd_gpu()" in the dashboard pre-flight. That object does not exist anywhere in this repository (checked via git cat-file -e, git log --all, and git fsck --unreachable against the full, non-shallow history). The commit that actually did this is d37cfbabd23898e88769ed83eeec681639d32eb5 ("fix(dash): detect amd-smi through WSL DXG (EAI-8450) (#375)") — confirmed via git blame on apps/rocm/src/dash.rs's gpu_reachable_for_preflight/has_usable_amd_gpu. The behavioral claim itself is accurate; only the hash is wrong, in two places.
  Confidence 95 · mechanical · Fix: replace b4f98bc2 with d37cfbab (or cite PR #375 / EAI-8450 by name instead of a raw hash) in both comments.

• The updated module doc overstates parity between "the two amd-smi pre-flights" — crates/rocm-core/src/hardware_root.rs:45-58, crates/rocm-dash-collectors/src/amd_smi.rs:189-202
  The edited doc comment says: "Routed, but with the real device keeping a veto: the two amd-smi pre-flights. Each decides whether a real amd-smi process may start, so each requires host_path("/dev/kfd") and the real /dev/kfd to be readable." This reads as the two gates being equivalently protective. They are not: the new rocm-core::device_readable_within (lib.rs:577) bounds its device-open with a watched, 2-second-timeout probe thread specifically because an open() against a device "mid-reset" can itself block in uninterruptible sleep. The rocm-dash-collectors gate's device_accessible (amd_smi.rs:201) is a single, unbound, synchronous std::fs::OpenOptions::open(...).is_ok(), called directly inside the async detect_with_binary_inner (amd_smi.rs:91) with no tokio::time::timeout around it (only the later me.run() call is time-bounded). A /dev/kfd in the exact "reset in progress" D-state condition this entire PR exists to guard against still blocks that site — and, being an un-spawned blocking call on a tokio worker thread rather than a killable child process, arguably worse there than before this PR's own reasoning about child processes applies. Neither the doc comment nor the PR description's "why the fourth site is left alone" section discloses this asymmetry; both frame the dash site as already adequately guarded.
  Confidence 85 · architectural · Fix: update the doc comment to state the asymmetry plainly rather than implying equivalence, and file a tracking issue for hardening device_accessible the same way (a small, dependency-free leaf crate for the bounded-open primitive — mirroring the existing rocm-dash-core pattern — would let both sides share it without violating the rocm-dash-* ↛ rocm-core layering invariant, at the cost of two new reviewed crate_edges allowlist entries).

• PR description makes claims the diff itself contradicts — PR body, section "AGENTS.md §3: why consequence 1 ships without a Gherkin scenario"; tests/e2e-cucumber/features/examine.feature (scenario examine-29), tests/e2e-cucumber/tests/e2e/examine_steps.rs (kfd_device_cannot_be_opened), crates/rocm-core/src/lib.rs:343-354 (TEST_KFD_DEVICE_ENV/preflight_kfd_device)
  The description states: "It is not covered by a scenario, and it cannot be on any lane this suite runs" and "Nor can the harness manufacture the premise: KFD_DEVICE is a compile-time constant with no path override, the node is root-owned so the harness can neither remove it nor make it unreadable." The diff itself adds exactly such a path override (ROCM_E2E_KFD_DEVICE, honored by preflight_kfd_device() under the e2e-test-hooks feature) and a scenario that exercises it end-to-end (examine-29, via the new kfd_device_cannot_be_opened step). The description further claims this reasoning "is also recorded as a KNOWN GAP note" in model_serving.feature — but the actual trailing comment added there carries no such label (that label appears exactly once in the file, in an unrelated @requires-multi-gpu comment) and instead correctly says the behavior "is covered by examine-29." The description reads as describing an earlier state of the branch, before the seam and examine-29 were added in its own later commits (1be50f49, 3c4d6c2b), and was never revised to match.
  Confidence 95 · documentation/description accuracy · Fix: rewrite this section to match the current diff — state that examine-29 covers the gate's decline behavior via rocmd's bridge-snapshot, and that rocm serve/rocm model's own call sites remain the uncovered part (see next finding), rather than claiming no coverage is possible at all.

• No test exercises the actual rocm serve/rocm model call sites this PR fixes — apps/rocm/src/main.rs:10858 (gpu_vram_usage_amd_smi), apps/rocm/src/main.rs:11121 (detect_gpu_count)
  Before this PR these two functions called amd-smi with no timeout at all (the PR's own AMD_SMI_SERVE_PROBE_TIMEOUT doc comment says so explicitly: "Before this bound existed both probes called .output() with no timeout at all, so that same host hung indefinitely") — this is new, user-observable behavior on rocm serve/rocm model, not a pre-existing path. Coverage added for it is indirect only: unit tests exercise the shared rocm_core::amd_smi_json/kfd_readable gate in isolation, and examine-29 exercises a different binary (rocmd's bridge-snapshot), not apps/rocm. No test calls gpu_vram_usage_amd_smi() or detect_gpu_count() directly, and serving_steps.rs has no reference to the KFD seam at all. A regression that reintroduced a raw, unguarded amd-smi spawn directly in main.rs (bypassing amd_smi_json) would pass every test this PR adds.
  Confidence 85 · architectural · Fix: add an e2e scenario targeting rocm model --model <ref> (a one-shot command, cheaper to assert on than serve) with ROCM_E2E_KFD_DEVICE pointed at an absent path, asserting the command completes within a short bound (well under the documented ~20s worst case) and that a stub-ran marker is absent — reusing the stub/marker technique examine_steps.rs already added for examine-29.

Non-blocking

• rocm-doctor diagnostic catalog has no entry for two of the four decline reasons — crates/rocm-core/src/fix.rs, crates/rocm-core/src/diagnose.rs
  The silent .ok()? discard on the interactive serve/model path is pre-existing behavior (identical shape at the base commit, for any amd-smi failure), not a regression. fix-4-render-group already covers the "permission denied" decline reason. It has no recipe for "no KFD GPU node" or "reset in progress" — the two reasons most relevant to the D-state hazard this PR targets — so a user hitting those specific states has no documented self-diagnosis path even via rocm fix/rocm diagnose.

• docs/architecture.md's lib.rs inventory is now stale — docs/architecture.md (~line 93)
  Names the pre-existing resolve_amd_smi_binary* family as "a distinct, not-yet-scoped concern" still living in lib.rs, but wasn't updated to include the ~300 new lines (amd_smi_json, kfd_readable, device_readable_within, etc.) this PR adds immediately alongside it. Not a guardrail violation — the project has an established, repeatedly-exercised pattern of consolidating in lib.rs and extracting later in a dedicated phase (ROCMAI-85/151/152/154 precedent, tracked under EAI-7768/ROCMAI-200) — just a one-line doc-currency gap so the eventual extraction doesn't miss this code. (docs/testing.md's selective examine-2x scenario catalog similarly doesn't mention examine-29, though that catalog is already incomplete by the project's own practice, so this is a minor, lower-confidence sibling observation rather than a separate ask.)

Decisions for the author

• Two independent /dev/kfd pre-flight implementations (rocm-core, rocm-dash-collectors) — tradeoff
  Crate-layering (rocm-dash-collectors ↛ rocm-core) is real and CI-enforced (xtask/src/crate_edges.rs), and the PR description says the author tried the direct edge and the guard caught it. But that invariant isn't one of the three explicitly named hard edges in the crate-edges module, and a tiny dependency-free leaf crate (the existing rocm-dash-core pattern) could host just the bounded-open primitive for both sides without violating anything named. Confirm whether that alternative was considered and rejected, or simply not considered — see Blocking/B2 for the consequence of leaving it as-is undocumented.

Positive signals

• The single-chokepoint design (amd_smi_json/amd_smi_json_with) cleanly enforces "every synchronous launch site" — confirmed by an exhaustive repo-wide grep for resolve_amd_smi_binary/"amd-smi" spawns, finding no missed synchronous site.
• The new unit tests are unusually rigorous about non-vacuity: marker files prove a spawn did or didn't happen, and a real blocking-open() FIFO fixture is watched with its own, looser deadline rather than relying on the production bound alone — each with an explicit "what this would catch" argument in its doc comment.
• The pre-flight's own doc comments honestly hedge what the gate does and doesn't guarantee ("detects inaccessible, not unhealthy... removes the reachable half of that hazard; it does not claim to see the rest") rather than overclaiming — and this exact overclaiming risk was already caught and corrected once earlier in this PR's own commit history (483798ba).

Deployment notes

• Linux hosts whose /dev/kfd cannot be opened will now have rocm serve/rocm model GPU auto-detection decline quickly rather than hang (or, previously, fail only after a slow/failed spawn) — a timing change in an existing silent-degrade path, not a new failure mode.
• One CI check is currently red on this PR: "E2E tests (Strix Halo, Windows, current)" (scenario diagnose-fix-path-prepends-with-apostrophe-intact). Verified independently (not assumed): the identical scenario fails identically on main itself in two separate, recent, unrelated runs (39220155843, 39217811126), on code this PR does not touch (PATH/apostrophe handling in diagnose/fix, unrelated to the amd-smi/KFD files here) — so it is not attributed to this change, but it means the Windows lane cannot currently attest this PR's own Windows behavior either.

What this covered

Full diff d4e7c75194d1643eacc3b1bda33afe10766def73...c1ad263d7a6beca1334409db625414ef28297eee (8 files, +1003/-73), read completely at the PR head by independent workers, plus surrounding/calling code: apps/rocm/src/dash.rs, crates/rocm-dash-collectors/src/amd_smi.rs, crates/rocm-core/src/process.rs, crates/rocm-core/src/fix.rs/diagnose.rs, docs/architecture.md, docs/testing.md, README.md, AGENTS.md, xtask/src/crate_edges.rs. Four independent code/source-reading passes (core implementation; e2e tests; agent-instruction adherence; history + code comments) and five independent Step-7 design-review investigations (TOCTOU/detection-coverage framing; duplicate pre-flight implementations; serve-path test coverage; silently discarded decline diagnostic; lib.rs growth vs. extraction) ran as parallel workers; none were run single-context. CI read live via gh pr view --json statusCheckRollup; the one failing check was independently verified pre-existing and unrelated (see Deployment notes) rather than assumed so. PR description and its named ticket (Jira ROCMAI-342 / bare ID EAI-8457) were retrieved and read only after all findings above were settled, per the method's ordering.

Did not run: cargo test/clippy locally — CI's own green Test (affected crates), clippy, build-and-test, and windows-build-and-test checks were taken as authoritative rather than re-derived. Coverage-floor re-bless check not examined in depth (file untouched by the diff; change is net-additive and heavily tested, so no floor-regression signal was expected or found). Not reconciled against any discussion: no review comments, review bodies, or issue/general comments on PR #447 were read at any point, before or during this review.

[juhovainio]

Summary

This change adds a shared KFD pre-flight gate (kfd_readable / amd_smi_json) and routes three of rocm's four amd-smi launch sites through it, closing most of the unkillable-hang window described in the linked ticket. The core approach (gate before spawn, not just a timeout) is sound, and the new tests cover the gate's branches well.

A few things worth resolving before merge:

1. Thread-leak path under sustained KFD-wedge conditions (see inline comment on device_readable_within). Worth confirming whether this is an acceptable tradeoff or needs bounding.
2. Scope: the linked ticket asks for all four amd-smi launch sites to go through the shared gate, including the dash collector (crates/rocm-dash-collectors/src/amd_smi.rs). This PR covers three of four and leaves the collector on its own separate, already-guarded implementation. The stated rationale (crate_edges layering guard plus the prior fix in #375) is plausible, but worth confirming explicitly since it diverges from what the ticket asked for.
3. AGENTS.md doc-sync: this changes user-observable behavior (amd-smi launches now silently decline instead of hanging or failing differently when /dev/kfd is unopenable), but no docs were updated to describe it.
4. The PR description's "pipe draining / zombie reaping" section under "Two consequences worth naming" does not match this diff. That work (and the named tests) appears to belong to a different PR (#526, ROCMAI-448). Worth double-checking the description isn't a leftover from a stacked branch.

Non-blocking nits:

• The fallback thread-leak latency isn't mentioned in the worst-case comment on serve --gpu auto in apps/rocm/src/main.rs (actual worst case is timeout + KFD_OPEN_TIMEOUT, not just timeout); see second inline comment.
• The temp-dir/fake-executable test scaffold is duplicated near-verbatim across the three new tests; a shared fixture would collapse it.
• amd_smi_json's args: &[&str] could be a small enum given there are only three call sites, and rocmd::capture_amd_smi_json is now a thin pass-through that exists only to pair a local timeout constant with the call.

CI is green except for the Strix Halo Windows E2E lane, which is failing on the known pre-existing PATH-apostrophe flake unrelated to this diff.

[juhovainio]

Every probe that overruns KFD_OPEN_TIMEOUT leaks one thread parked in a blocking open() (documented above). Under sustained operation against the exact wedged-KFD condition this gate protects against, repeated polling (rocmd's periodic watcher, serve's periodic probes) can leak threads until the process exhausts its thread budget, at which point this fallback reverts to a blocking open on the calling thread, recreating the hang this gate exists to prevent. Is this an acceptable tradeoff, or should outstanding probe threads be bounded (a semaphore, or a single long-lived probe worker with a bounded queue)?

[juhovainio]

Worth noting in this doc comment (or the serve --gpu auto worst-case comment in apps/rocm/src/main.rs) that KFD_OPEN_TIMEOUT (2s) adds to every call's latency on top of timeout. The documented ~20s worst case for serve --gpu auto looks understated by that amount.

[nowycondro]

🤖 Automated review (Claude Code) · c1ad263

This PR has already been through many review rounds (visible in the thread history: WSL wave-through, zombie-reaping, the device_readable_within open()-timeout bound, the #526 merge/convergence, and an E2E-seam fix for examine-24/25/26). I focused on the surviving design rather than re-litigating settled items, reading crates/rocm-core/src/lib.rs (the KFD gate, kfd_readable/kfd_readable_with, device_readable/device_readable_within, amd_smi_json), crates/rocm-dash-collectors/src/amd_smi.rs (the fourth, deliberately-unrouted launch site), apps/rocm/src/main.rs and apps/rocmd/src/common.rs (the three newly-routed call sites), and apps/rocmd/src/watchers.rs (call cadence into the gate).

Non-blocking

• crates/rocm-core/src/lib.rs:577-592 (device_readable_within) — the documented "one leaked thread is strictly cheaper than a blocked caller" trade-off is reasoned about as a single-call cost, but rocmd calls into this gate on a fixed, unbounded cadence. apps/rocmd/src/watchers.rs polls GPU metrics every GPU_METRICS_INTERVAL_MS = 60_000 with no backoff on failure, and that path runs through capture_amd_smi_json → amd_smi_json → kfd_readable → device_readable_within. On a host whose /dev/kfd stays wedged in the exact "open never returns" state this PR targets, the daemon leaks one rocm-kfd-open-probe thread per minute for as long as the host stays wedged, rather than once. The code's own fallback path ("If no thread can be spawned the probe falls back to a direct blocking open") means that once accumulated leaked threads exhaust the process's thread limit, the probe degrades into the unbounded blocking open this whole mechanism exists to avoid. This is a real amplification of an already-accepted trade-off rather than a new hazard, so I'm not blocking on it, but it seems worth a one-line doc update acknowledging the periodic-caller case, or capping/reusing the probe thread for long-lived daemons. Confidence: medium — I did not measure actual leaked-thread resource cost, and reaching exhaustion requires an unusually long-lived wedged device.

• crates/rocm-dash-collectors/src/amd_smi.rs:201 (device_accessible) — the fourth launch site's own pre-flight is a plain, untimed OpenOptions::open, unlike every other site this PR touches. The PR's "why the fourth site is left alone" reasoning (crate-layering: rocm-dash-collectors must not depend on rocm-core) is sound on its own terms, and a prior review round already flagged that this pre-flight only runs once per collector lifetime (at detect()), not per launch. What doesn't appear to have been raised: even that single check is an unbounded blocking open (std::fs::OpenOptions::new().read(true).open(path).is_ok() at line 201, called synchronously from the async detect_with_binary_inner), with none of the KFD_OPEN_TIMEOUT/thread-probe protection that rocm-core's device_readable_within adds for the identical hazard. On a host whose /dev/kfd blocks in open during detect(), this parks a tokio worker thread in uninterruptible sleep — the same class of hang the rest of this PR exists to remove — and nothing about the layering constraint stops rocm-dash-collectors from implementing its own local bounded-thread probe (it wouldn't need to cross into rocm-core to do that). Confidence: medium-high on the code reading, lower on real-world likelihood since this only matters during collector (re)detection rather than every poll.

I found no correctness bugs in the three newly-routed call sites themselves or in the WSL wave-through / timeout-bound logic as it stands at this head; the mechanism looks sound for the cases this PR's own extensive test suite exercises.

What I didn't verify

No Rust toolchain is available in this environment, so I did not compile, run cargo test/clippy, or execute any part of the e2e suite — this is a static reading of the diff and surrounding source at c1ad263d, cross-checked against the prior review thread for duplication. I did not re-derive the mutation-testing claims made in earlier review rounds, and I did not audit apps/rocmd/src/sandbox.rs or crates/rocm-core/src/process.rs in depth since they're outside this PR's diff per the thread history.

🤖 Generated with Claude Code

[jussielo-amd]

Thanks for sticking with this through all the review rounds — the core approach (gate before spawn) is solid and well tested.

One thing still blocks for me: crates/rocm-dash-collectors/src/amd_smi.rs:201's device_accessible is still a bare, untimed open(). The PR describes the KFD pre-flight as covering every amd-smi launch site, but this one (the rocm dash collector) isn't routed through the new bounded-open protection, so a wedged /dev/kfd can still block it indefinitely — the exact hazard the rest of this PR removes everywhere else. I flagged this in my last review too and it doesn't look like it's been addressed yet.

A few smaller things, none blocking:

• device_readable_within's fallback to a plain blocking open() when the probe thread can't spawn is a real edge case, but it's already called out honestly in the doc comment as a deliberate trade-off, so I'm fine with it as-is.
• The AMD_SMI_SERVE_PROBE_TIMEOUT doc comment says serve's worst case is "~20s" — with the KFD open timeout on both probes it's closer to 24s. Small, cosmetic.
• lib.rs picked up another ~300 lines for this gate plus its tests. Consistent with how this repo tends to do things (consolidate, extract later), so not blocking, just flagging for whoever does the eventual extraction pass.

Happy to take another look once the dash-collector gap is addressed.

[nowycondro]

🔴 Automated review · pr-review-watcher · c1ad263

Review — needs work

Full review of the whole change (origin/main at d59f596f...pr/447 head c1ad263d).
Implements ROCMAI-342 (filed and still referenced in this PR under its earlier key, EAI-8457): routes the three production amd-smi launch sites that were missing the mandatory /dev/kfd pre-flight (rocmd's capture_amd_smi_json, rocm's gpu_vram_usage_amd_smi, rocm's detect_gpu_count) through one new guarded launcher, rocm_core::amd_smi_json. The ticket's own bypass table and the PR's match; the fourth site (rocm-dash-collectors) is correctly left alone for a documented crate-layering reason.

Blocking

• CI check E2E tests (Strix Halo, Windows, current) is failing on the current head — apps/rocm, workflow "E2E self-hosted"
  gh pr view 447 --json statusCheckRollup reports this check as FAILURE (completed 2026-10-17T13:29:23Z), and headRefOid for that rollup matches the commit under review (c1ad263d). I could not read the job log (gh run view returned 403 Forbidden for this token), so I can't say whether it's a genuine regression or a pre-existing flake on that lane, but a red check is not something this review can wave through.
  Confidence 100 (the check state itself, not its cause) · architectural · Fix: investigate the Windows self-hosted run and get it green (or get a maintainer to confirm/re-run a known flake) before merge.

• The PR description is stale and misdescribes the diff — PR description (not a source file)
  Roughly half the description (the second "consequence worth naming," and most of the "Tests" section with its mutation table) describes a run_command_with_timeout pipe-draining-and-reaping fix, naming tests a_child_that_outgrows_the_pipe_buffer_is_not_reported_as_hung and a_timed_out_child_is_reaped_rather_than_left_a_zombie. None of this exists anywhere in origin/main...pr/447, nor anywhere in the repository at the PR head under those names (git grep for run_command_with_timeout, reap_in_background, both test names returns nothing in the diff, the PR-head tree, or origin/main). The repo's actual subprocess-timeout helper is run_with_timeout (crates/rocm-core/src/process.rs:840), and its own pipe-draining fix already shipped separately and is already on main as commit 07166450 ("fix(rocmd): drain child pipes concurrently...", PR #526) — the exact PR this diff's own MAX_QUOTED_OUTPUT_CHARS doc comment cross-references. The PR's "Docs" section compounds this: it says "none of kfd_accessible... appear outside these source files," but kfd_accessible does not exist anywhere in this diff either — the actual gate function this PR adds is kfd_readable/preflight_kfd_device. Separately, the description states behavior-consequence 1 (serve's probes silently degrading) "is not covered by a scenario, and it cannot be on any lane this suite runs," promising this is "recorded as a KNOWN GAP note... in the style [model_serving.feature] already uses" — but the shipped model_serving.feature has no KNOWN GAP marker for this (the marker does exist elsewhere in that same file, e.g. for serve-20, confirming the convention was available), and instead carries a detailed comment explaining that this exact behavior is covered, through examine-29 via rocmd's bridge-snapshot surface. All of this reads as leftover prose from an earlier, combined draft of the branch — consistent with the PR's own merge commits ("Merge remote-tracking branch 'origin/main' into HEAD") having folded #526 back out of this branch's unique diff after it landed on main separately, and with commit 1be50f49 ("test(e2e): reach both sides of the KFD pre-flight from a scenario") having added the e2e coverage after the description was written.
  Confidence 95 (directly verified by git grep against the diff, the PR-head tree, and origin/main) · mechanical · Fix: rewrite the description to describe only what this diff contains — drop the pipe-draining/reaping consequence and its tests, fix kfd_accessible→kfd_readable, and update the coverage section to say examine-29 covers the decline behavior via rocmd's bridge-snapshot (rather than claiming it is an accepted gap).

• The pre-flight's own leaked-thread fallback can reintroduce the exact unkillable hang it exists to prevent, under sustained failure — crates/rocm-core/src/lib.rs (device_readable_within, kfd_readable/amd_smi_json_with)
  device_readable_within spawns a new, never-joined OS thread for every pre-flight probe; when open() blocks past KFD_OPEN_TIMEOUT (2s) — the documented hazard case this whole PR targets — that thread is deliberately left running forever (comment: "one leaked thread is strictly cheaper than a blocked caller"). But amd_smi_json is reached on every rocmd telemetry poll (capture_amd_smi_json → gather_gpu_snapshot → build_bridge_snapshot; the PR's own comments describe a "60s telemetry poll" and a note rewritten "on every poll"). On a host whose KFD device is persistently stuck in the blocking state — not a transient blip, but the exact "driver wedged after a fault" / "container with an unhealthy host driver" scenario the ticket itself names as the target case — every poll leaks one more thread, indefinitely. Once enough threads accumulate to exhaust the process's thread limit, thread::Builder::spawn starts failing, and the code's own documented fallback is a direct, unbounded blocking open() on the calling thread — i.e., exactly the D-state hang this PR exists to eliminate, now reachable again from inside the gate itself, triggered specifically by prolonged exposure to the condition the gate is supposed to guard against forever. At a plausible 60s poll cadence this is not a far-fetched edge case: a per-process thread limit in the low thousands (common in constrained containers) would be exhausted in hours to a couple of days.
  Confidence 78 (traced the code path and the call graph directly; the polling cadence is corroborated by the PR's own comments rather than independently load-tested) · architectural · Fix: cap or dedupe outstanding probe threads (e.g. once an open has been timed-out once, cache "device confirmed hung" for a cooldown instead of spawning a fresh probe thread on every call), and/or change the thread-spawn-failure fallback to decline (fail closed) rather than fall back to a blocking open.

Non-blocking

• Stale comment left behind by this PR's own refactor — apps/rocmd/src/common.rs, in gather_gpu_snapshot's comment block (the parenthetical "(The non-zero-exit and parse failures below are their own outermost context and already read correctly.)")
  Before this PR, capture_amd_smi_json contained inline non-zero-exit and JSON-parse-failure handling literally below this comment in this same file; this PR's own diff deletes that code and delegates to rocm_core::amd_smi_json, so "below" no longer names anything in this file. The surrounding paragraph was carefully updated elsewhere in the same diff to account for the pre-flight, but this sentence was missed.
  Confidence 80 · mechanical · Fix: reword to say the non-zero-exit and parse-failure contexts are now constructed inside amd_smi_json, not "below" in this file.

• Latency doc comment undercounts the pre-flight's own timeout window — apps/rocm/src/main.rs, the doc comment on AMD_SMI_SERVE_PROBE_TIMEOUT ("Worst-case latency... adds up to ~20s... 20s is the ceiling")
  amd_smi_json_with calls kfd_readable (bounded by KFD_OPEN_TIMEOUT, 2s) before starting run_with_timeout's own timeout window, so one call's worst case is KFD_OPEN_TIMEOUT + timeout, not just timeout. For serve's two sequential probes the ceiling this PR introduces is closer to 2×(2s + 10s) = 24s, not the stated ~20s, whenever the device is slow-but-not-fully-hung rather than instantly healthy.
  Confidence 80 (traced the exact call sequence in amd_smi_json_with) · mechanical · Fix: adjust the comment's arithmetic (or fold KFD_OPEN_TIMEOUT into the stated ceiling).

• Documentation left out of sync with the new behavior, per the project's own AGENTS.md bar — docs/testing.md, docs/manual-testing.md, docs/release-trust.md
  AGENTS.md §5: "when a command's... observable behavior change[s], update README.md, its --help/doc comment, docs/testing.md, and docs/manual-testing.md in the same change — do not leave user-facing docs for a follow-up." docs/testing.md documents every sibling examine-2x scenario in detail but has no entry at all for the new examine-29 or the ROCM_E2E_KFD_DEVICE seam; docs/manual-testing.md has zero mentions of amd-smi/kfd. Separately (not named by AGENTS.md's list, but the same drift), docs/release-trust.md's "Simulated Host Root" section still describes only one /dev/kfd-veto pre-flight (the dash collector's), while this PR's own edit to crates/rocm-core/src/hardware_root.rs's module doc now correctly describes two.
  Confidence 75 · mechanical · Fix: add examine-29/the seam to docs/testing.md, and update docs/release-trust.md's pre-flight count.

Decisions for the author

• daemon_gathers_bridge_snapshot's command_env reapplication loop (e2e harness) re-applies every pushed key, not just the KFD seam — non-blocking-improvement
  In tests/e2e-cucumber/tests/e2e/examine_steps.rs, after setting a default stand-in ROCM_E2E_KFD_DEVICE, the step iterates all of world.command_env and calls command.env(key, value) for each pushed entry, so a future Given that happens to push an unrelated key already set by this same function (e.g. PATH, which plant_gpu_fixtures already pushes elsewhere) would silently override it if ever combined with this step. No scenario does this today, so it's not a current bug — just worth a guard (e.g. assert the loop only ever touches KFD_DEVICE_SEAM) before the harness grows further.

Positive signals

• The new unit tests in crates/rocm-core/src/lib.rs are unusually rigorous about non-vacuity: the_kfd_gate_stops_amd_smi_before_it_is_spawned uses a marker file to prove a spawn did or didn't happen (not just that a call failed), and the_kfd_probe_declines_a_device_whose_open_blocks_instead_of_hanging drives a real FIFO through an externally-watchdogged thread so that deleting the production timeout fails the test instead of hanging the suite.
• The e2e scenario's stub_ran_marker mechanism (examine_steps.rs) proves "never spawned" rather than merely "reported unavailable," which is exactly the distinction this feature needs and a good pattern for future amd-smi-adjacent scenarios.

Deployment notes

None beyond the CI gate above — no schema/config migration, and the e2e-test-hooks seam this PR adds a constant under is gated out of every shipped build by cfg(not(feature = "e2e-test-hooks")).

What this covered

Read, completely: crates/rocm-core/src/lib.rs (full, ~1472 lines — the new gate, amd_smi_json, and all new unit tests), apps/rocmd/src/common.rs (full, 629 lines), crates/rocm-core/src/hardware_root.rs (full, 412 lines), tests/e2e-cucumber/tests/e2e/examine_steps.rs (full, 2065 lines), tests/e2e-cucumber/features/examine.feature and tests/e2e-cucumber/features/model_serving.feature (full). Narrowed by necessity (both disclosed by the workers that did this reading, not silently): apps/rocm/src/main.rs (~21,400 lines total) was read only in the diff's own hunks plus the directly-coupled functions and call sites (select_gpu_indices_under_launch_lock, select_auto_gpu_index, the VRAM/GPU-count probe functions, assess_model_with_host_paths/host_accelerator_memory, and the serve() call site), not the whole file; crates/rocm-core/src/examine.rs (~6415 lines) was read only around truncate_to_chars and its existing callers, not the whole file. I independently re-verified (not just inherited from workers) the two highest-value claims against the live PR-head tree and origin/main with git grep/git diff: the serve_cmd::serve call-site claims a worker's narrower scope couldn't confirm (confirmed correct, serve_cmd.rs is a sibling module), the PR-description staleness (confirmed via git grep for symbols the description names that don't exist in the diff or the repo), and the live CI rollup.

Did not run: prior-changes pass against other PRs' review comments — not attempted, since this method does not read any PR's discussion threads, including other PRs'; agent-instruction adherence for ai-instruction-quality — not applicable, no skill/agent files changed. Commit b4f98bc2, cited in a doc comment as prior art, does not resolve in this clone (not a shallow clone) — treated as unverifiable here rather than false; the PR's own development history (commits 483798ba, and squash-merge d37cfbab/PR #375) corroborates the claim's substance even though the exact SHA is gone post-squash. Not reconciled against any PR discussion or prior review here — this is a fresh review with no earlier round to reconcile against.