feat(doctor): diagnose and fix WSL2 hosts instead of routing out - #340
feat(doctor): diagnose and fix WSL2 hosts instead of routing out#340volen-silo wants to merge 1 commit into
Conversation
`rocm diagnose` treated WSL2 as out of scope and ran none of its checks, and `rocm examine` returned after the framework probe alone. The reasoning was sound -- WSL2 reaches the GPU through /dev/dxg and the Windows host driver, so the bare-metal checks for the amdgpu module, /dev/kfd and the render group would report confident nonsense -- but it left users on that platform with no help at all. Add a parallel WSL2 catalog rather than porting the bare-metal one. Selection moves to a platform family resolved from `is_wsl`, not from `os_family`, which stays "linux" because install, serve and the engines branch on it. Every bare-metal entry is tagged `linux` only and so stops applying on WSL automatically, which keeps the no-false-positives property the old wholesale skip bought. Entries that were always valid there -- the arch list, HSA_OVERRIDE_GFX_VERSION, PATH and the wheel/ROCm pairing -- opt in explicitly and now answer on WSL for the first time. `rocm fix` uses the same resolution, so a Linux-only recipe is refused there rather than running usermod for a group that governs nothing. Seven WSL entries cover the device, the DXCore handoff, ROCDXG and its linker entry, the distro floor, the Windows host driver, and WSL 1. All are print-only: each installs packages with sudo, edits loader config, or belongs to the Windows host, so none meets the bar the four auto-applicable fixes clear. Throughout, "could not ask" is kept distinct from "at fault", because the machines that cannot answer are the ones most likely to be misdiagnosed: the host-driver check abstains when the Windows host is unreachable, the distro floor when the release will not parse, and the linker check when ldconfig itself cannot be run. Symptom keywords only ever corroborate machine evidence -- never establish a finding on their own -- so a user describing a GPU failure cannot talk the catalog into blaming their driver. `examine` now runs the probes that do apply on WSL and reports the stack under a new `wsl` section. Fields the shared checks expect from the skipped GPU probe are filled from those facts; left at their defaults they did not read as "unknown" but as "absent", which made the PATH check score 50 on every WSL host with ROCm installed. Where the shared checks still run on less evidence than on bare metal the degradation is one-directional -- they may miss a fault, never invent one -- and a test pins that direction. Also fixes defects that would have produced false positives: ROCDXG was looked up only under /opt/rocm so a versioned install reported it missing; two WSL predicates disagreed about hosts identified only by $WSL_DISTRO_NAME; and ldconfig was resolved by bare name, which fails for a non-root user on Debian, where it lives in /sbin. `out_of_scope` is repurposed rather than deleted: it now marks a platform with no catalog entries at all, which previously fell through to "no known misconfiguration" and read as a clean bill of health. `run` used `read_to_string`, which errors on invalid UTF-8 with the error discarded, so one stray byte emptied an entire capture and every caller read that as "the command printed nothing". It now reads bytes and converts lossily. Replace the Python WSL preflight with `rocm diagnose --distro`. Its in-guest half was already redundant; the host-side half now collects the facts over `wsl.exe` using a POSIX shell and runs the same catalog, so the target distro needs neither rocm-cli nor python3 -- which matters, since the distro being checked is usually the one that is not set up yet. It sees no environment, so it says which checks did not run rather than letting silence read as health. `wsl.exe` output is decoded as UTF-16LE by declaration: sniffing cannot work, because that text in a Latin or Cyrillic script is made of bytes below 0x80 and so is valid UTF-8 that decodes into mojibake. The script's distro-list parser and release-floor rules keep their coverage as Rust tests. The two `@requires-wsl` scenarios run only on the `e2e-wsl` lane in e2e-selfhosted.yml; the third new scenario runs everywhere. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
r0x0r
left a comment
There was a problem hiding this comment.
Approving.
Clean design: resolving platform_family = wsl from is_wsl while keeping os_family = "linux" means install/serve/engines are untouched, bare-metal checks stop applying on WSL automatically, and cross-platform checks opt into wsl explicitly. The tri-state modeling (Option<bool> for "could not ask" vs. "answered at fault") with the documented one-directional invariant — less evidence may miss a fault but never invent one — is exactly right for a diagnosis tool, and it's pinned by tests in both directions. Having --distro refuse rather than silently diagnose the local machine keeps the no-silent-fallback contract intact.
The latent-bug fixes are each backed by a named regression test: the read_to_string non-UTF-8 capture emptying, the /opt/rocm ROCDXG hardcode, ldconfig resolved by bare name failing for non-root on Debian /sbin, the early-WSL2 kernel misclassification, and the UTF-16LE wsl.exe decode. Porting wsl_preflight.py into Rust (with tests) and dropping its two --self-test CI lines is a clean, self-contained swap. CI is green and guardrails check out (leak scan clean, DCO sign-off + valid signature, logic in rocm-core with CLI wiring in apps/rocm and scenarios in e2e-cucumber).
One non-blocking nit: in crates/rocm-core/src/lib.rs, the doc comment beginning "Whether relative exists under any ROCm install…" sits above ldconfig_cache() rather than rocm_relative_file_exists(), so it attaches to the wrong item and leaves rocm_relative_file_exists undocumented. Worth moving it down to the function it describes.
For the record: the wsl.exe --distro success path and the two @requires-wsl scenarios only exercise on the e2e-wsl / Strix Halo Windows lanes, which are continue-on-error — so those lane results should be read directly before merge rather than trusted via the overall rollup. Not blocking given how densely the underlying parser/decoder/floor/kernel logic is unit-tested and that the --distro refusal scenario runs on every lane.
siloteemu
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · 195e1ea
Summary
Replaces the "WSL2 is out of scope" short-circuit in rocm diagnose/examine/fix with a parallel WSL2 catalog selected by a platform family resolved from is_wsl, and deletes scripts/wsl_preflight.py in favour of rocm diagnose --distro. Verdict: Needs work — the design is sound and fails closed on mutation, but the detection predicate was widened in a way that can misclassify a native Linux host, one catalog entry can blame the Windows host driver for a Linux-side fault, and three of the PR's headline behaviours have no test that would fail if reverted. Verified: read all changed files plus callers in full, then re-confirmed every blocking claim against source; answered the revert question per test; reviewed host mutation safety end-to-end (all seven new fix-wsl-* recipes are runner: None print-only, the only destructive auto fix fix-4-render-group stays LINUX_ONLY behind the unchanged confirm/--yes/TTY gate, and wsl.exe is spawned via argv with no shell and a timeout, so no injection path); no prompt-injection content in the diff; relied on CI (27 passing) for the build and full suite. Blocking: 6 · Non-blocking: 5.
🚫 Blocking (must fix before merge)
crates/rocm-core/src/lib.rs:2434(wsl_signals_indicate_wsl) —$WSL_DISTRO_NAMEbeing set is, on its own, sufficient to classify the host as WSL. The predicate this replaces (is_wsl_environment_fast, base ref line 4294) required/dev/dxgormicrosoftin/proc/versionand never trusted the env var; the unified predicate now gates far more. A native Linux host that has the variable set (inherited oversshwithSendEnv, adocker run -e, a shared shell profile, a tmux session started from WSL) getsplatform_family="wsl"(diagnose.rs:1756), so the entire bare-metal catalog — amdgpu module,/dev/kfd, render group — is silently suppressed on hardware that may have exactly those faults,probe_wslskips the bare-metal driver probes (examine.rs:325), androcm fix fix-4-render-grouprefuses with "wrong OS" on a box that needs it. The function's own doc comment still justifies the union with "a false positive costs a route-out note" (lib.rs:2356-2358) — this PR deleted the route-out, so the justification is stale. Fix: require the env var to be corroborated, e.g.if dxg_device || (distro_name_set && proc_version_matches) { true }. This costs nothing for real WSL — WSL2 always has/dev/dxgand WSL1 always hasMicrosoftin/proc/version— and update the comment to state the new cost of a false positive.crates/rocm-core/src/diagnose.rs:1615-1621(check_wsl_6_host_driver_too_old, second match arm) — the guard requiresw.librocdxg(installed) but never checksw.ldconfig_librocdxg. On a host where ROCDXG is installed but not on the loader path — precisely the fact patterncheck_wsl_4_rocdxg_not_linkedexists for, and a very plausible cause ofrocminfoenumerating nothing — both entries fire, so the user is told to update their Windows GPU driver for a fault that is entirely Linux-side and already correctly reported above it. This is the same defect the PR fixed in this check's first arm (Some("") if w.dxg_device, with the comment at 1597-1599 and the test at 2561 pinning it); the analogous guard was never added to the second. Fix: add&& w.ldconfig_librocdxg != Some(false)to the second arm's guard, matchingcheck_wsl_4's own semantics.crates/rocm-core/src/diagnose.rs:2461-2467(an_installed_but_unlinked_rocdxg_is_a_distinct_finding) — would still pass if the production change were reverted, and does not test what its name claims. It asserts onlyreport.matched[0].id, never thatfix-wsl-6-host-driver-too-oldis absent; and becausewsl_base()leavesrocm_sees_gpu: Some(true), the overlapping path is never entered, so it passes both with and without the guard above. Fix: assert the full id list equals["fix-wsl-4-rocdxg-not-linked"](the patternonly_the_root_cause_of_a_broken_stack_is_reportedalready uses), and add a case that also setsrocm_sees_gpu = Some(false)to actually exercise the overlap.crates/rocm-core/src/fix.rs:531-539(current_os) — the WSL branch has no test that would fail if reverted. Every platform-gating test either runs on whatever OS the process happens to be (fix.rs:1287,fix.rs:1342) or skips viarecipe_applies_here(fix.rs:1246), which itself callscurrent_os(). Nothing ever forcesis_wsl_host()true, so on ordinary Linux CIcurrent_os()returns"linux"whether or not theif crate::is_wsl_host()branch exists — the PR's headline claim thatrocm fix fix-4-render-groupis refused on WSL is unprotected.is_wsl_host()reads an env var and the test module already sets env vars under#[allow(unsafe_code)](fix.rs:1162), so this is straightforward: setWSL_DISTRO_NAME, assertapply("fix-4-render-group", &FixOptions::default()) == 3and that aWSL_ONLYrecipe is accepted, then unset.crates/rocm-core/src/examine.rs:813(sync_shared_fields_from_wsl) — no test would fail if this were reverted. This function is the entire fix for the stated defect that "the PATH check scored 50 on every WSL host with ROCm installed". No test in the crate assertsrocminfo_present/rocminfo_statusafter the sync (grep confirms the only other writes are the bare-metal probe at 1064-1082); thediagnose.rsfixtures hand-fill those shared fields, aswsl_base()'s own comment at 2269 says, so they bypass the function entirely. Delete the call atexamine.rs:602and the suite stays green. Fix: add a unit test that builds anExaminationwithwsl: Some(WslFacts { .. })and asserts the post-syncrocminfo_present/rocminfo_statusfor each(rocminfo, rocm_sees_gpu)combination.tests/e2e-cucumber/features/diagnose.feature:158,169+tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:505-522— the new WSL behaviour has no coverage on any blocking lane, and this is not stated in the PR text asAGENTS.md§3 requires. The two@requires-wslscenarios resolve to Skip onci.yml'se2ejob (real Linux,cap.is_wsl == false,expectation.rs:439) and only execute one2e-wsline2e-selfhosted.yml:647, which iscontinue-on-error: true. The un-gated third scenario does not compensate: on the blocking lanewhich("wsl.exe")is always false (examine.rs:703), soprobe_wsl_distro_from_hostreturns the generic "wsl.exe was not found" message for any--distrovalue, and the assertion accepts that branch (diagnose_steps.rs:519) — it never reaches the "no WSL distribution named ''" refusal it is named for. Fix: name the lane in the PR body per §3, and add a host-independent unit test forprobe_wsl_distro_from_hostwith an injected distro list so the "named machine does not exist" refusal is covered by a required check.
Non-blocking
crates/rocm-core/src/lib.rs:2392-2398— missing blank line means therocm_relative_file_existsdoc block is concatenated ontoldconfig_cache's and rendered against the wrong function;rocm_relative_file_exists(2423) ends up undocumented.crates/rocm-core/src/lib.rs:2409,2423— the two helpers written to fix the versioned-/opt/rocm-7.xand non-root-Debianldconfigdefects have no unit tests;rocm_relative_file_existscallsdiscover_rocm_installs()directly with no test seam (unlikediscover_rocm_installs_in), so both regressions could silently return.crates/rocm-core/src/diagnose.rs:1600,1615— both ofcheck_wsl_6's base scores (45, 40) sit belowMIN_SCORE_FOR_MATCH(50), so a confirmed "Windows host reports no AMD display adapter" can never sethas_matchfrom machine evidence alone; worth confirming that is deliberate rather than an artefact of the keyword-floor rework.docs/wsl.md(source-build section) — the deleted script's--require-build-toolscheck (Windows SDK headers,cmake/gcc/g++/make/git) has no successor and no mention of being dropped, while the source-build path it validated is still documented; same for thepython3 -m venvprobe, which is probably obsolete underuvbut is not said to be.crates/rocm-core/src/examine.rs:818— the(true, None) => "unknown"arm is unreachable given both producers setrocm_sees_gpu = rocminfo.then(...); harmless but worth a note or a tighter type.
Problem
On WSL2,
rocm diagnoseran none of its checks androcm examinestopped after the framework probe. Users got a pointer to the docs and nothing else.The reason was sound. WSL2 reaches the GPU through
/dev/dxgand the Windows host driver, not the in-treeamdgpumodule or/dev/kfd, so the bare-metal checks — render group,/dev/kfdpermissions,modprobe amdgpu,iommu=pt— describe hardware that platform does not have. Running them would produce confident, wrong answers. Skipping them avoided that, at the cost of leaving one of the likeliest platforms to have a subtly broken GPU setup with no help at all.This adds a parallel WSL2 catalog. It does not make the bare-metal catalog run under WSL2.
Approach
Catalog selection now resolves a platform family from
is_wslrather than fromos_family.os_familystays"linux"on WSL — install, serve and the engine crates branch on it — but for diagnosis WSL is its own platform.That inversion is what makes the split safe. Every bare-metal entry is tagged
linuxonly, so it stops applying on WSL automatically; the no-false-positives property the old wholesale skip bought is preserved without the skip. Entries that were always valid there opt in explicitly: the framework arch list,HSA_OVERRIDE_GFX_VERSION,PATH, and the wheel/ROCm pairing now answer on WSL for the first time.rocm fixresolves the same way, so a Linux-only recipe is refused there instead of runningusermodfor a group that governs nothing.Seven WSL entries cover the device, the DXCore handoff, ROCDXG and its linker entry, the distro release floor, the Windows host driver, and WSL 1. All are print-only: each installs packages with
sudo, edits loader configuration, or belongs to the Windows host, so none meets the bar the four auto-applicable fixes clear. The "exactly four auto-applicable fixes" assertion is unchanged, which is the proof.Throughout, "could not ask" is kept distinct from "at fault" — the machines that cannot answer are exactly the ones most likely to be misdiagnosed. The host-driver check abstains when the Windows host is unreachable, the distro floor when the release will not parse, and the linker check when
ldconfigitself cannot be run. Symptom keywords only ever corroborate machine evidence and never establish a finding alone, so a user describing a GPU failure cannot talk the catalog into blaming their driver.examinenow runs the probes that do apply on WSL and reports the stack under a newwslsection.Notable details
Fields left at defaults are not neutral. The shared cross-platform checks read fields the bare-metal GPU probe fills, and that probe is skipped on WSL. Left at their defaults they do not read as "unknown", they read as "absent" —
rocminfo_present: falsemade the PATH check score 50 on every WSL host with ROCm installed. Those are now filled from the WSL facts. For the same reason the host-driver check reads whetherrocminfoenumerates a GPU rather thanhas_amd_gpu, which is false on every WSL host, healthy or not.Where the shared checks still run on less evidence than on bare metal, the degradation is one-directional — they may miss a fault, never invent one — and a test pins that direction.
Latent defects fixed, all of which would have produced false positives in the new catalog: ROCDXG was looked up only under
/opt/rocm, so a versioned install reported it missing; two WSL predicates disagreed about hosts identified only by$WSL_DISTRO_NAME; andldconfigwas resolved by bare name, which finds nothing for a non-root user on Debian, where it lives in/sbin— an unreadable linker cache then read as an unregistered library and told users with a working install to re-runldconfig.out_of_scopeis repurposed, not deleted. It now marks a platform with no catalog entries at all. That case previously fell through to "no known misconfiguration", which reads as a clean bill of health when in truth nothing ran.Command output is no longer silently discarded.
runusedread_to_string, which errors on invalid UTF-8 with the error dropped — so one stray byte emptied an entire capture, for every probe in the tree, and callers read that as "the command printed nothing". It now reads bytes and converts lossily.Replacing the Python preflight
scripts/wsl_preflight.pyis removed in favour ofrocm diagnose --distro [NAME].Its in-guest half was already redundant with
examine/diagnose. Its unique capability — inspecting a distribution from the Windows host — is now Rust: it collects the facts overwsl.exeusing a POSIX shell and runs the same catalog. The target distribution needs neitherrocm-clinorpython3, which matters, because the distribution being checked is usually the one that is not set up yet. The script's distro-list parser and release-floor rules keep their coverage as Rust tests, so removing it drops no assertions.wsl.exeoutput is decoded as UTF-16LE by declaration, not detection. Sniffing cannot work here: UTF-16LE text in a Latin or Cyrillic script is made entirely of bytes below0x80, so it is valid UTF-8 and decodes without error into mojibake. The list is also read with-l -qrather than-l -v, because the header row of-vis localised and a header the parser fails to recognise is not skipped — it is taken for a distribution name.Review
Three review rounds before this was opened, which between them found eleven defects that are fixed here. The ones worth naming, because they show the shape of the risk in this area:
is_wsl1_kerneltested for the absence of aWSL2marker, so every early WSL 2 kernel (4.19.104-microsoft-standard, which predates the suffix) was classified as WSL 1 — a high-confidence instruction to convert an already-converted distribution, which also suppressed every other check.Each has a regression test named for the failure it prevents.
Testing
rocm-core, including one per WSL entry, both directions of the platform split (no bare-metal cause on WSL, no WSL cause on bare metal), and the abstention cases.cargo clippy --workspace --all-targets -- -D warnings,cargo fmt --check,scripts/smoke_local.py.The two
@requires-wslscenarios run only on thee2e-wsllane ine2e-selfhosted.yml; the third runs on every lane.Not verified locally: the
--distrosuccess path needs a Windows host with WSL, which I do not have. What is proven here is the refusal path and the pure logic — parser, decoder, tri-states, fact assembly. Thewsl.exeinvocation itself will first run on the Strix Halo Windows lane. That lane ande2e-wslare bothcontinue-on-error, so their results need reading directly rather than trusting the overall check state.Known limitations, not defects: inspecting from the host collects no environment, so it reports on the WSL GPU stack rather than the whole installation — the command says so in its output and
docs/wsl.mddocuments it.rocm diagnosestill exits 0 in every case except--distro, which errors rather than silently reporting on the local machine.Checklist
tests/e2e-cucumber/expectations.tomlfor stale xfail rows. The one WSL row — theexamine --json/ host-summary disagreement about the GPU — still holds: this change does not populategpusorhas_amd_gpuon WSL, so both forms report exactly as before.