Skip to content

feat(doctor): diagnose and fix WSL2 hosts instead of routing out - #340

Open
volen-silo wants to merge 1 commit into
mainfrom
feat/wsl2-doctor-catalog
Open

feat(doctor): diagnose and fix WSL2 hosts instead of routing out#340
volen-silo wants to merge 1 commit into
mainfrom
feat/wsl2-doctor-catalog

Conversation

@volen-silo

Copy link
Copy Markdown
Collaborator

Problem

On WSL2, rocm diagnose ran none of its checks and rocm examine stopped after the framework probe. Users got a pointer to the docs and nothing else.

The reason was sound. WSL2 reaches the GPU through /dev/dxg and the Windows host driver, not the in-tree amdgpu module or /dev/kfd, so the bare-metal checks — render group, /dev/kfd permissions, 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_wsl rather than from os_family. os_family stays "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 linux only, 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 fix resolves the same way, so a Linux-only recipe is refused there instead of running usermod for 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 ldconfig itself 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.

examine now runs the probes that do apply on WSL and reports the stack under a new wsl section.

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: false made 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 whether rocminfo enumerates a GPU rather than has_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; and ldconfig was 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-run ldconfig.

out_of_scope is 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. run used read_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.py is removed in favour of rocm 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 over wsl.exe using a POSIX shell and runs the same catalog. The target distribution needs neither rocm-cli nor python3, 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.exe output 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 below 0x80, so it is valid UTF-8 and decodes without error into mojibake. The list is also read with -l -q rather than -l -v, because the header row of -v is 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:

  • The host-driver check could reach HIGH confidence on a completely healthy machine purely from the words a user typed, because it shared a generic keyword table with another check while having no base score of its own — and it outranked the check that had found the real fault.
  • is_wsl1_kernel tested for the absence of a WSL2 marker, 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.
  • Several fields left at their struct defaults read as evidence rather than as unknown.

Each has a regression test named for the failure it prevents.

Testing

  • 351 unit tests in 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.
  • Full e2e suite green on a real WSL2 host: 78 scenarios, 0 unexpected failures. Three new scenarios — a WSL machine is never given a bare-metal cause, a WSL remedy is explained rather than carried out, and a request about an unreachable machine is refused rather than answered with this machine's diagnosis.
  • cargo clippy --workspace --all-targets -- -D warnings, cargo fmt --check, scripts/smoke_local.py.

The two @requires-wsl scenarios run only on the e2e-wsl lane in e2e-selfhosted.yml; the third runs on every lane.

Not verified locally: the --distro success 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. The wsl.exe invocation itself will first run on the Strix Halo Windows lane. That lane and e2e-wsl are both continue-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.md documents it. rocm diagnose still exits 0 in every case except --distro, which errors rather than silently reporting on the local machine.

Checklist

  • Searched tests/e2e-cucumber/expectations.toml for stale xfail rows. The one WSL row — the examine --json / host-summary disagreement about the GPU — still holds: this change does not populate gpus or has_amd_gpu on WSL, so both forms report exactly as before.

`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 r0x0r left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 siloteemu left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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_NAME being 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/dxg or microsoft in /proc/version and never trusted the env var; the unified predicate now gates far more. A native Linux host that has the variable set (inherited over ssh with SendEnv, a docker run -e, a shared shell profile, a tmux session started from WSL) gets platform_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_wsl skips the bare-metal driver probes (examine.rs:325), and rocm fix fix-4-render-group refuses 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/dxg and WSL1 always has Microsoft in /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 requires w.librocdxg (installed) but never checks w.ldconfig_librocdxg. On a host where ROCDXG is installed but not on the loader path — precisely the fact pattern check_wsl_4_rocdxg_not_linked exists for, and a very plausible cause of rocminfo enumerating 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, matching check_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 only report.matched[0].id, never that fix-wsl-6-host-driver-too-old is absent; and because wsl_base() leaves rocm_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 pattern only_the_root_cause_of_a_broken_stack_is_reported already uses), and add a case that also sets rocm_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 via recipe_applies_here (fix.rs:1246), which itself calls current_os(). Nothing ever forces is_wsl_host() true, so on ordinary Linux CI current_os() returns "linux" whether or not the if crate::is_wsl_host() branch exists — the PR's headline claim that rocm fix fix-4-render-group is 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: set WSL_DISTRO_NAME, assert apply("fix-4-render-group", &FixOptions::default()) == 3 and that a WSL_ONLY recipe 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 asserts rocminfo_present/rocminfo_status after the sync (grep confirms the only other writes are the bare-metal probe at 1064-1082); the diagnose.rs fixtures hand-fill those shared fields, as wsl_base()'s own comment at 2269 says, so they bypass the function entirely. Delete the call at examine.rs:602 and the suite stays green. Fix: add a unit test that builds an Examination with wsl: Some(WslFacts { .. }) and asserts the post-sync rocminfo_present/rocminfo_status for 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 as AGENTS.md §3 requires. The two @requires-wsl scenarios resolve to Skip on ci.yml's e2e job (real Linux, cap.is_wsl == false, expectation.rs:439) and only execute on e2e-wsl in e2e-selfhosted.yml:647, which is continue-on-error: true. The un-gated third scenario does not compensate: on the blocking lane which("wsl.exe") is always false (examine.rs:703), so probe_wsl_distro_from_host returns the generic "wsl.exe was not found" message for any --distro value, 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 for probe_wsl_distro_from_host with 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 the rocm_relative_file_exists doc block is concatenated onto ldconfig_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.x and non-root-Debian ldconfig defects have no unit tests; rocm_relative_file_exists calls discover_rocm_installs() directly with no test seam (unlike discover_rocm_installs_in), so both regressions could silently return.
  • crates/rocm-core/src/diagnose.rs:1600,1615 — both of check_wsl_6's base scores (45, 40) sit below MIN_SCORE_FOR_MATCH (50), so a confirmed "Windows host reports no AMD display adapter" can never set has_match from 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-tools check (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 the python3 -m venv probe, which is probably obsolete under uv but is not said to be.
  • crates/rocm-core/src/examine.rs:818 — the (true, None) => "unknown" arm is unreachable given both producers set rocm_sees_gpu = rocminfo.then(...); harmless but worth a note or a tighter type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants