Skip to content

ROCMAI-154: extract net.rs/process.rs/paths.rs from rocm-core lib.rs - #599

Merged
jussielo-amd merged 8 commits into
ROCm:mainfrom
jussielo-amd:rocmai-154-net-process-paths
Oct 13, 2026
Merged

jussielo-amd merged 8 commits into
ROCm:mainfrom
jussielo-amd:rocmai-154-net-process-paths

Conversation

@jussielo-amd

@jussielo-amd jussielo-amd commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Part of ROCMAI-27's modularization effort: crates/rocm-core/src/lib.rs is the last god-file target (Phase 7), split into four independent sub-phases (7a-7d) so no single PR is unreviewably large. This is 7b, extracting the remaining networking/process/filesystem-location primitives into their own modules, mirroring the crate's 8 existing sibling modules (diagnose.rs, examine.rs, host_gpu.rs, etc.) via the established pub mod x; pub use x::{...}; convention:

  • net.rs — default local host/port, URL/endpoint formatting, streaming downloads with resume/retry, and the loopback HTTP/TCP helpers used to probe managed-service readiness (model listing and a real inference round-trip).
  • process.rs — hidden-console/detached spawn and exit-wait on Windows, process/process-tree termination on both platforms, and the exclusive FileLock used to serialize the daemon autostart path, the managed-launch GPU claim, and uv installs across processes.
  • paths.rs — AppPaths (config/data/cache root discovery and its derived per-engine/per-service subpaths), the configured managed-root override, and engine plugin directories.

env_flag, a generic environment-boolean parser unrelated to filesystem paths, moved to runtime.rs instead of into paths.rs, next to the existing env_path_override.

Two generic crate-root helpers that sat physically adjacent to these clusters — torch_alignment_disabled/TORCH_ALIGNMENT_DISABLED_ENV (a cross-cutting config opt-out, not networking) and require_nonempty (a generic validation helper used well beyond this cluster, including externally from engines/vllm/engines/lemonade) — stay in lib.rs rather than being swept into net.rs/paths.rs on physical proximity alone; neither 7c's nor 7d's scope claims them either. Logged as a quality-backlog finding on ROCMAI-200 rather than decided unilaterally here.

proc_lifecycle.rs's two pub(crate) call sites (process_tree_pids, signal_process_scope) and host_gpu.rs's one call site into the former crate-root env_flag are updated to the new module paths. docs/architecture.md's crates/rocm-core module map is updated in the same PR to reflect all three new modules and what remains (7c's crypto/model-catalog cluster, 7d's config/automation/service-record/binary-paths cluster).

Pure code motion — no behavior change. The ticket's own historical line-range estimates for this phase were stale by 150-500 lines each (7a landed after the ticket was written, shifting everything below it), so boundaries here were re-verified against current main by function/symbol name rather than taken from the ticket text.

Test plan

  • cargo build --workspace
  • cargo clippy --workspace --all-targets -- -D warnings
  • cargo fmt --check (workspace-wide)
  • cargo test --workspace — all suites green, 0 failures
  • Test count diffed, not just pass/fail: 607 #[test] attributes in rocm-core before and after this split, unchanged
  • cargo xtask check-crate-edges
  • cargo xtask manifest --check
  • cargo xtask coverage — no floor regression
  • Leak scan against origin/main — clean
  • tests/e2e-cucumber — not run locally; this phase touches only rocm-core, not main.rs/rocmd, so it's not in AGENTS.md's required category for this kind of change, and rocm-core's own unit-test coverage (above) already proves behavior is unchanged. CI's suite is authoritative.

@jussielo-amd
jussielo-amd force-pushed the rocmai-154-net-process-paths branch 2 times, most recently from 2519811 to 56e8c5c Compare October 8, 2026 12:46
@jussielo-amd
jussielo-amd marked this pull request as ready for review October 8, 2026 13:03
@jussielo-amd
jussielo-amd requested a review from a team as a code owner October 8, 2026 13:03
@jussielo-amd
jussielo-amd requested a review from tomastola October 8, 2026 13:03
@jussielo-amd

Copy link
Copy Markdown
Collaborator Author

CodeQL's rust/cleartext-logging failure on this PR (crates/rocm-core/src/lib.rs:2718) is pre-existing alert #768, open on main since 2026-08-27. It flags generate_endpoint_api_key_is_random_and_alphanumeric()'s assert! message, which echoes a generated key on test failure.

This PR only extracts net.rs/process.rs/paths.rs out of lib.rs (pure code motion, no behavior change) — that test and its assert message are unchanged, just shifted to a new line number, which is why CodeQL's diff heuristics attribute it to this PR.

Not dismissing #768 myself since it's a pre-existing, repo-wide alert unrelated to ROCMAI-154 — leaving that call to whoever owns it.

@rominf rominf 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.

🔴 Automated review · pr-review-watcher · 56e8c5c

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 — no blocking findings

Full review of the whole change.
The description matches the diff: three modules extracted as pure code motion, with torch_alignment_disabled/require_nonempty deliberately left in lib.rs (confirmed at lib.rs:139-160). Ticket ROCMAI-154 is named but can't be retrieved from here (no tracker access).

Blocking

None

Non-blocking

  • The move broke four intra-doc links (new) — crates/rocm-core/src/paths.rs:178, paths.rs:212, crates/rocm-core/src/process.rs:240, process.rs:261
    These links resolved when the code sat at the crate root. [FileLock] on the public managed_launch_lock_path/automation_autostart_lock_path now points into paths.rs, which doesn't import FileLock. [proc_lifecycle] is no longer in scope inside process.rs. cargo doc -p rocm-core --no-deps --document-private-items reports 7 unresolved links at head and 3 at prw-base. The 4 new ones are exactly these. No CI job runs rustdoc, so nothing catches this.
    Confidence 100 · mechanical · Fix: write them as [FileLock](crate::FileLock) and [proc_lifecycle](crate::proc_lifecycle), or use full crate:: paths.
  • Test helper probe_test_record now exists twice, identically (new) — crates/rocm-core/src/lib.rs:2805, crates/rocm-core/src/net.rs:2187
    prw-base had one copy. The PR moved four of its callers to net.rs and left one (restarting_drops_the_previous_runs_inference_verification) in lib.rs, so the 22-line fixture was copied rather than shared. test_support.rs is the crate's home for shared fixtures like this. Two identical copies tend to drift: a later edit lands in one and not the other.
    Confidence 85 · mechanical · Fix: move it into test_support.rs as pub(crate) and call it from both test modules.
  • Two comments are now stale because of the move (new) — crates/rocm-core/src/examine.rs:2804, crates/rocm-core/src/test_support.rs:8-9
    examine.rs:2804 says // statvfs FFI; the same pattern as the Win32 calls in lib.rs. All Win32 FFI now lives in process.rs, and lib.rs has no unsafe left. The module doc of test_support.rs lists its users as "host_gpu, rocm_install, managed_runtime, and lib.rs itself". The moved net and paths tests now use it too.
    Confidence 85 · mechanical · Fix: point the examine.rs comment at process.rs, and either add net/paths to the list or drop the list.

Decisions for the author

None

Positive signals

  • The motion is faithful and easy to check. Removed versus added lines differ only in module headers, use/pub use blocks, the env_flag → pub(crate) bump (the only visibility change; it needs it for its lib.rs and host_gpu.rs callers), and the two proc_lifecycle.rs call-site paths. Every pub item in the new modules is re-exported from the crate root, including the #[cfg(windows)] spawn/wait functions, so no downstream rocm_core:: path breaks.
  • The description explains why the adjacent torch_alignment_disabled/require_nonempty stayed put rather than moving on physical proximity alone. That is the right call for a code-motion PR.

Deployment notes

None

What this covered

  • What was read: the whole change efcd4800…56e8c5cf. process.rs and paths.rs were read in full. net.rs was verified as unchanged moved code by comparing removed and added lines across the whole diff (ignoring whitespace) rather than reading it line by line. The touched callers in host_gpu.rs and proc_lifecycle.rs and the docs/architecture.md update were checked too.
  • Runs:
    • #[test] count in the moved region: 94 before and 94 after (54 lib.rs + 29 net + 8 paths + 3 process).
    • cargo test -p rocm-core --lib on head merged with current prw-base (which adds #609's new FileLock user in uv.rs): 585 passed, including all 40 net/paths/process tests.
    • cargo clippy -p rocm-core --all-targets -- -D warnings is clean at each intermediate commit (13c8a0d, f4ef349).
    • CI on head: every check passed, including windows-build-and-test and the Strix Windows/WSL2/MI300X E2E lanes. Skillscope and Sphinx were skipped.
  • Moved tests: they are relocated, not added or changed. They would pass with the motion reverted, which is expected for a refactor and is not a finding.
  • What didn't run, and why:
    • Windows cross-check: no local Windows type-check (no mingw toolchain; ring's build script fails). Windows coverage rests on the CI head run only, so per-commit Windows buildability of the first two commits is unverified.
    • Fan-out: one independent code-pass worker, plus driver-run passes for history, prior changes (lessons from the 7a extraction PR #528: visibility widening, module docs, unused import blocks, per-commit builds), agent-instruction adherence and impact analysis.
    • Design review: done by the driver, not by independent workers, because a pure-motion change raised no approach questions beyond the module convention, which it follows.
  • Not reconciled against any discussion on this PR.

@jussielo-amd

Copy link
Copy Markdown
Collaborator Author

Addressed the three non-blocking findings from @rominf's automated review (56e8c5c), in 05a7bd2:

  • Broken intra-doc links — rewrote [FileLock]/[proc_lifecycle] as [FileLock](crate::FileLock)/[proc_lifecycle](crate::proc_lifecycle) in paths.rs/process.rs. Confirmed with cargo doc -p rocm-core --no-deps --document-private-items: unresolved-link warnings dropped from 7 to 3, and the 3 remaining are the pre-existing ones unrelated to this PR (discover_rocm_installs_in_layout, ExamineSummary, migrate_legacy_dashboard_toml).
  • Duplicate probe_test_record — moved into test_support.rs as pub(crate), deleted both copies from lib.rs and net.rs, updated both test modules' imports to call the shared one.
  • Stale comments — examine.rs's statvfs comment now points at process.rs (where the Win32 FFI actually lives); dropped test_support.rs's enumerated module-doc list rather than extend it again, since it already drifts every time a new module adopts the fixture.

Verified: cargo clippy -p rocm-core --all-targets -- -D warnings clean, cargo test -p rocm-core --lib 583 passed / 0 failed. A fresh independent review (different model, no view of the original findings) ran against the remediation diff and came back with zero findings.

rominf
rominf previously requested changes Oct 9, 2026

@rominf rominf 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.

🔴 Automated review · pr-review-watcher · e1cebe8

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. The previous round's review point (56e8c5c) is gone after the rebase, so nothing was narrowed.
The description presents this as phase 7b of ROCMAI-27 / ROCMAI-154, a pure code motion of rocm-core's networking, process and path primitives out of lib.rs. I could not retrieve ROCMAI-154 or ROCMAI-27 from here.

Blocking

  • (new) The net.rs extraction reverts #603's scratch-root fix and deletes its regression test — crates/rocm-core/src/net.rs:1431-1440

    • #603 (7f72556) is an ancestor of HEAD. In lib.rs, it made download_scratch name its root "download-{tag}-{}", crate::test_support::unique_suffix() and added download_scratch_gives_each_call_its_own_root_even_for_one_label.
    • In commit 6934f5f, net.rs goes back to "download-{tag}-{}-{}", std::process::id(), unix_time_millis(), and that test no longer exists anywhere in the tree. This looks like a pre-#603 copy that won the rebase conflict.
    • The sorted-line comparison of base lib.rs against head lib.rs+net.rs+process.rs+paths.rs shows this is the only behavioural drift in the split.
    • Effect: two tests in one cargo test process that start in the same millisecond share a root again, and one test's remove_dir_all deletes the other's files. CI did not catch it because the Linux lane runs nextest, one process per test.
    • Confidence 100 (run): I put the deleted test back into net.rs and ran cargo test -p rocm-core --lib download_scratch_gives. It FAILS against the PR's helper, panicking at test_support.rs:104. · logic
    • Fix: restore crate::test_support::unique_suffix() in download_scratch, and restore the test.
  • (new) The description and commit messages claim pure code motion with an unchanged test count, and that is false — PR description ("Pure code motion — no behavior change", "584 #[test] attributes in rocm-core before and after this split, unchanged"); commits 6934f5f, 5088ef9, 37b8cde ("Pure code motion… test count unchanged (584)")

    • Counting #[test] at the merge-base gives 95 in lib.rs. At HEAD there are 94: lib.rs 54, net.rs 29, process.rs 3, paths.rs 8.
    • The missing test, and the changed helper, are the ones in the finding above.
    • A reader who trusts "pure motion" will not look for a revert, and the revert is real.
    • Confidence 85 · architectural
    • Fix: finish the work, i.e. restore the helper and the test. Then the claim becomes true and the description can stay as written.

Non-blocking

  • (new) The process.rs module doc overstates what the module does — crates/rocm-core/src/process.rs:5-9

    • It says spawn is provided "on both platforms". In fact the spawn_* functions and wait_for_process_exit are #[cfg(windows)]-only, and Unix gets only detach_command_session.
    • It describes FileLock as serializing "install/provisioning". Its main callers, apps/rocm/src/main.rs daemon autostart and the managed-serve GPU claim, use it for other things; only uv.rs matches that description.
    • The PR description repeats both phrasings.
    • Confidence 70 · mechanical
    • Fix: describe the Windows-only spawn surface and FileLock's actual uses.
  • (new) A generated AI attribution footer is in a commit message — commit e1cebe8 (Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>)

    • AGENTS.md §11 says "avoid AI-generated boilerplate footers".
    • This repo squash-merges, so the trailer can carry into the landed commit body.
    • Confidence 70 · mechanical
    • Fix: drop the trailer when squashing.

Decisions for the author

  • env_flag now lives in paths.rs — non-blocking-improvement
    • It is a generic environment-boolean parser. Its callers (host_gpu.rs:266, and lib.rs's model-recipe signature check as paths::env_flag) have nothing to do with paths.
    • In lib.rs, paths:: also reads ambiguously next to the ubiquitous paths: &AppPaths variables.
    • runtime.rs, or a small env-helper module, would describe it better. The current placement is documented in the module doc and is not wrong.

Positive signals

  • Every pub item that moved is re-exported at its old rocm_core::X path, including the #[cfg(windows)] spawn/wait block. Downstream crates did not need a single edit.
  • The FileLock/proc_lifecycle intra-doc links are rewritten as crate:: paths, so they still resolve from their new modules.
  • The copy of probe_test_record moved into test_support.rs rather than being duplicated across lib.rs and net.rs tests.

Deployment notes

None

What this covered

  • What was read: all of net.rs, process.rs, paths.rs and test_support.rs, plus the diffs to lib.rs, proc_lifecycle.rs, host_gpu.rs, examine.rs and docs/architecture.md. These were checked against the base lib.rs they came from (item by item and by a whole-file sorted-line comparison) and against workspace consumers of the moved pub items.
  • Range: merge-base 844edaf…e1cebe8d. prw-base (c1c2aeb) also carries #558 and #591, which do not touch these files.
  • Local runs:
    • cargo clippy --workspace --all-targets -D warnings: exit 0.
    • cargo test -p rocm-core --lib: 589 passed, 40 of them in net::/process::/paths::tests.
    • Mutation check described above.
  • CI at the head: green except E2E tests (MI300X), which has no conclusion yet. Sphinx docs build (-W) and Skill checks (skillscope) also had not reported on this head when the round started.
  • Checks that could not be made:
    • ROCMAI-154 and ROCMAI-27 could not be retrieved, because no tracker tool is available here.
    • The prior-changes pass did not run, because no earlier review comments on these files were read.
    • Windows-only cfg(windows) arms were checked by reading only, not compiled locally. The windows-build-and-test CI lane is green.
  • The PR discussion was not reconciled here.

@jussielo-amd

Copy link
Copy Markdown
Collaborator Author

Addressed the review round from @rominf (e1cebe8), in 52d14a2/2d9bd83f/143f20b6:

  • Blocking: net.rs reverted test: give every test scratch root a per-process unique name #603's scratch-root fix and deleted its regression test — confirmed exactly as described (std::process::id()+unix_time_millis() had come back and download_scratch_gives_each_call_its_own_root_even_for_one_label was gone). Restored crate::test_support::unique_suffix() in download_scratch and restored the test in 52d14a2. cargo test -p rocm-core --lib download_scratch passes; the full rocm-core lib suite is 591 passing (94→95 #[test] attributes across lib.rs/net.rs/process.rs/paths.rs, matching merge-base again).
  • Blocking: "pure code motion, unchanged test count" claim was false — this was the same root cause; with the test restored, the claim is true again and the PR description doesn't need editing (as you suggested).
  • Non-blocking: process.rs module doc overstated the module — reworded in 2d9bd83 to describe the actual Windows-only spawn surface and FileLock's real callers (daemon autostart, managed-launch GPU claim, uv installs).
  • Decision for author: env_flag's placement — moved out of paths.rs into runtime.rs in 143f20b, next to the existing env_path_override (same category of env-var helper); both call sites (host_gpu.rs, lib.rs) updated.
  • AI attribution footer on e1cebe8 — agreed this shouldn't be here per AGENTS.md §11; since this repo squash-merges, I'll make sure it's dropped from the squash commit message rather than rewriting pushed history now.

A fresh independent review (different model, no view of this thread) ran against the three remediation commits and came back with no blocking or non-blocking findings — one "decision for author" note that the reworded process.rs doc now names specific FileLock call sites, which is more precise but could drift if a caller is added/removed later; leaving it as the more accurate version for now, happy to make it caller-agnostic instead if you'd prefer.

Verified: cargo build --workspace, cargo clippy --workspace --all-targets -- -D warnings, cargo xtask check-crate-edges all clean.

@jussielo-amd
jussielo-amd force-pushed the rocmai-154-net-process-paths branch 2 times, most recently from d35b15b to d82da1c Compare October 12, 2026 10:39
@jussielo-amd
jussielo-amd requested a review from rominf October 12, 2026 11:02

@rominf rominf 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.

🔴 Automated review · pr-review-watcher · d82da1c

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.

Gate note (round 3): this round withdraws the change request filed at e1cebe8; both of its blockers are gone (see Standing objections below). The one item the method lists under Blocking this round is a stale clause in the paths.rs module doc plus PR-description drift. Merging would leave behind one wrong doc line and no behaviour change, and the description does not land in the squash commit. So this round does not spend a merge gate on it, but it should still be fixed.

Review — needs work

Full review of the whole change. The previous review point (e1cebe8) is gone after the rebase, so nothing was narrowed.
Implements Phase 7b (net/process/paths extraction) of ROCMAI-154 / ROCMAI-27's lib.rs modularization. Ticket ROCMAI-154 is named but could not be retrieved from here (no tracker tool).

Blocking

  • (new) The paths.rs module doc and the PR description misstate where env_flag lives, and the description misstates process.rs's scope — crates/rocm-core/src/paths.rs:5-9, PR description bullets for paths.rs and process.rs
    • What the code does: commit d82da1c moved env_flag to crates/rocm-core/src/runtime.rs:207. env_flag no longer appears anywhere in paths.rs.
    • What the paths.rs header says: it still lists "engine plugin directories, and the generic env-flag boolean parser". The PR description's paths.rs bullet repeats this word for word.
    • What the description says about process.rs: "detached/hidden-console spawn on both platforms" and "FileLock used to serialize install/provisioning". Commit c073680 corrected both statements in the module doc. spawn_* and wait_for_process_exit are cfg(windows)-only (re-exported under #[cfg(windows)] at lib.rs:102-106), and FileLock also serializes daemon autostart and the managed-launch GPU claim.
    • Which side governs: the code. d82da1c's message gives a deliberate reason for the move ("env_flag … has nothing to do with filesystem paths"), and every call site already uses runtime::env_flag (host_gpu.rs:266, lib.rs:1587).
    • Minor: the description's test plan cites "584 #[test]". The crate has 607 #[test] attributes both before and after the change. "Unchanged" holds; only the number is stale.
    • Confidence 85 · mechanical
    • Fix: drop "and the generic env-flag boolean parser" from the paths.rs header. Bring the description's paths.rs/process.rs bullets and the test count in line with the code. Narrow the description; do not move code.

Non-blocking

  • (standing) An AI attribution trailer will land on main — commit 0a9644b
    • The commit carries Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>. This repo squash-merges and builds the landed body from the commit messages, so the trailer ends up in main's history.
    • AGENTS.md §11 says "avoid AI-generated boilerplate footers".
    • Confidence 80 · mechanical
    • Fix: reword the commit to drop the trailer before merge.
  • (new) Fix-up commits will read as noise in the squash body — commits 7d9f808, 0a9644b, c073680
    • 7d9f808 ("restore #603's unique scratch-root fix lost in rebase") repairs a regression this PR introduced and then removed; the net diff against base has no change there. In the squash body it reads as though main lost and regained #603, which it never did.
    • The same applies to "fix broken doc links … left by the extraction".
    • Confidence 75 · mechanical
    • Fix: fold these into the extraction commits, or trim them from the squash message.

Decisions for the author

  • (new) The new modules are pub mod, so every moved item gets a second public path — tradeoff
    • For: every moved item is now reachable as both rocm_core::X and rocm_core::{net,process,paths}::X. This matches the crate's existing convention (pub mod runtime, pub mod managed_runtime, …), which docs/architecture.md documents.
    • Against: it widens the public surface. A private mod plus pub use would keep only the crate-root paths.
    • Please confirm the wider surface is intended.

Standing objections from the e1cebe8 round

This round's blind review did not re-find either one. Each was then put to the reviewer together with the author's reply, so the verdicts below are anchored checks, not findings.

  • net.rs reverted #603's scratch-root fix and deleted its regression test. Withdrawn.
    • download_scratch (net.rs:1431-1435) uses crate::test_support::unique_suffix() again, byte-identical to base lib.rs.
    • download_scratch_gives_each_call_its_own_root_even_for_one_label is back and runs (net::tests::… ok, 1 passed).
    • Reverting the helper to pid+millis makes that test fail (panic at test_support.rs:104), so it guards the fix.
  • "Pure code motion, test count unchanged" was false. Withdrawn for the landed result.
    • The squash diff is motion-only again: 607 #[test] attributes crate-wide before and after; base lib.rs's 98 split 57/30/3/8.
    • A sorted-line comparison of base lib.rs against head lib.rs+net.rs+process.rs+paths.rs leaves only the env_flag move to runtime.rs, the probe_test_record dedup into test_support.rs, and rewritten doc-link targets. Main has not moved since this base, so the rebase dropped nothing else.
    • What remains is cosmetic and not gating: the "584" in commits 40677b1, 699d06e and 8474f17 is wrong (607), and each of those commits on its own says "unchanged" while carrying 606 tests. Correcting or dropping the number in the squash message would make the landed body accurate.

Positive signals

  • The extraction is item-for-item faithful. Every #[cfg] and #[allow(unsafe_code)] stays on the same item, and every public item is re-exported, with the Windows-only spawn/wait functions under a matching #[cfg(windows)].
  • Intra-doc links are rewritten to explicit paths ([FileLock](crate::FileLock)), so they resolve from the new modules.

Deployment notes

  • Additive public API only: the new rocm_core::net, rocm_core::process and rocm_core::paths module paths. Every crate-root path is preserved. No migration or configuration change.

What this covered

What was read: prw-base (7a062ad, current main) … d82da1c, all 7 commits.

  • net.rs, process.rs and paths.rs were read completely and matched block by block against base lib.rs.
  • The diffs to lib.rs, runtime.rs, test_support.rs, host_gpu.rs, proc_lifecycle.rs, examine.rs and docs/architecture.md were read in full.
  • The repo was grepped for stale references, and history was checked against recent commits touching lib.rs.

What was run:

  • cargo test -p rocm-core --lib: 604 tests, 603 passed, 1 ignored, 0 failed. They include 30 net::, 8 paths:: and 3 process:: tests.
  • The targeted download_scratch test, plus the mutation check above.
  • cargo doc -p rocm-core --document-private-items: no new warnings from the moved code.

CI at d82da1c: all checks green. Skillscope and Sphinx did not report.

Did not run:

  • Local clippy and the Windows build; CI covers both.
  • The prior-changes pass.
  • Ticket retrieval (no tracker tool).

Not reconciled: the blind findings were not reconciled with the PR discussion. Only the two standing objections above were weighed against the author's reply.

@rominf
rominf dismissed their stale review October 12, 2026 11:29

Both objections from the e1cebe8 round are resolved at d82da1c: download_scratch uses unique_suffix() again and its regression test is restored and guards it, and the squash diff is code motion only. Details: #599 (review)

@jussielo-amd

Copy link
Copy Markdown
Collaborator Author

Addressed the blocking finding from @rominf's round-3 review (d82da1c), in 8f78915:

  • Blocking: paths.rs module doc and PR description misstated env_flag's location; description misstated process.rs's scope — confirmed: paths.rs's header still listed "the generic env-flag boolean parser" after d82da1c moved it to runtime.rs. Dropped that clause from the module doc. Also updated the PR description's paths.rs/process.rs bullets to match the corrected module docs, added a line noting env_flag's move to runtime.rs, and fixed the test-plan count (607, not 584 — "unchanged" was already true, only the number was stale).

On the two non-blocking/decision items that don't need a code change:

  • pub mod widening the public surface — intended. It mirrors the crate's existing convention (pub mod runtime, pub mod managed_runtime, etc.), which docs/architecture.md documents, rather than something specific to this split.
  • AI attribution trailer (0a9644b) / fix-up commits reading as noise in the squash body — still the plan from the previous round: since this repo squash-merges, I'll write a clean squash commit message at merge time (no trailer, fix-ups folded into their extraction commits' description) rather than rewriting already-pushed history now.

Verified: cargo clippy -p rocm-core --all-targets -- -D warnings clean on the new commit. Full CI run in progress.

@jussielo-amd
jussielo-amd requested review from juhovainio and rominf and removed request for tomastola October 12, 2026 12:53
jussielo-amd and others added 8 commits October 13, 2026 06:22
Pulls the networking/download/endpoint-readiness primitives out of
crates/rocm-core/src/lib.rs into their own module, mirroring the
existing sibling-module convention (diagnose.rs, examine.rs, etc.).
Pure code motion: no behavior change, test count unchanged (607).

Signed-off-by: Jussi Elo <jussi.elo@amd.com>
Pulls the process spawn/terminate primitives and FileLock out of
crates/rocm-core/src/lib.rs into their own module, mirroring the
existing sibling-module convention. Updates proc_lifecycle.rs's two
pub(crate) call sites (process_tree_pids, signal_process_scope) to
the new module path. Pure code motion: no behavior change, test
count unchanged (607).

Signed-off-by: Jussi Elo <jussi.elo@amd.com>
Pulls AppPaths and the filesystem-location primitives out of
crates/rocm-core/src/lib.rs into their own module, mirroring the
existing sibling-module convention. Bumps env_flag to pub(crate) and
updates its one remaining lib.rs call site and host_gpu.rs's call
site to the new module path. Updates the docs/architecture.md
rocm-core module map to reflect all three Phase 7b modules
(net.rs, process.rs, paths.rs) and the remaining 7c/7d scope.

Pure code motion: no behavior change, test count unchanged (607).

Signed-off-by: Jussi Elo <jussi.elo@amd.com>
…ments

The net.rs/process.rs/paths.rs extraction left four intra-doc links
unresolved (`FileLock`/`proc_lifecycle` no longer in local scope),
duplicated the `probe_test_record` fixture across lib.rs and net.rs
instead of sharing it, and left two comments pointing at lib.rs after
the code they described had moved.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
download_scratch in net.rs had reverted to the pre-ROCm#603 process::id()
+ unix_time_millis() naming, and the regression test it added had
been dropped along with it. Two tests started in the same
millisecond could again share a scratch root and race on
remove_dir_all. Restore crate::test_support::unique_suffix() and the
test.

Signed-off-by: Jussi Elo <jussi.elo@amd.com>
… surface

The module doc said spawn works "on both platforms", but
spawn_detached_no_inherit/spawn_hidden_console_no_inherit/
spawn_hidden_console_with_log/wait_for_process_exit are all
cfg(windows)-only; Unix only gets detach_command_session. It also
described FileLock as serializing "install/provisioning", but its
callers are the daemon autostart lock and the managed-launch GPU
claim in main.rs, plus uv.rs's install lock — only the last matches.

Signed-off-by: Jussi Elo <jussi.elo@amd.com>
env_flag is a generic environment-boolean parser with nothing to do
with filesystem paths — its callers (host_gpu.rs's debug-capture
flag, lib.rs's model-recipe signature check) are unrelated to
AppPaths. Move it beside runtime.rs's existing env_path_override,
the same category of env-var helper.

Signed-off-by: Jussi Elo <jussi.elo@amd.com>
env_flag moved to runtime.rs in d82da1c but the paths.rs module doc
still listed it among the module's contents.

Signed-off-by: Jussi Elo <jussi.elo@amd.com>
@jussielo-amd
jussielo-amd force-pushed the rocmai-154-net-process-paths branch from 8f78915 to 1f0f5cb Compare October 13, 2026 06:25

@juhovainio juhovainio 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.

Reviewed as a pure code-motion refactor and it holds up: all three new modules (net.rs, process.rs, paths.rs) verified byte-identical to their pre-move source across independent blind passes, test count unchanged (607 on both sides), call sites in proc_lifecycle.rs/host_gpu.rs correctly repointed, docs/architecture.md updated in the same PR as required, and a regression accidentally introduced mid-rebase was self-caught and restored within this PR's own history. cfg(windows) gating stayed symmetric across the process.rs move. CI is green on all required checks (MI300X is still queued, not failing).

Two small, non-blocking notes for follow-up:

  • Commit deb36d14 carries a Co-Authored-By: Claude Sonnet 5 trailer -- worth dropping via amend to match our no-AI-footer convention.
  • The Test Plan checklist checks off cargo xtask check-architecture-doc, but no such subcommand exists in xtask's Command enum -- likely an honest slip, worth correcting the checklist line.

This is a diff-only read -- not a substitute for CI's own final state or a runtime check, but nothing here blocks merging.

@jussielo-amd
jussielo-amd added this pull request to the merge queue Oct 13, 2026
Merged via the queue into ROCm:main with commit f430387 Oct 13, 2026
30 checks passed
@jussielo-amd
jussielo-amd deleted the rocmai-154-net-process-paths branch October 13, 2026 09:22
r0x0r added a commit that referenced this pull request Oct 13, 2026
…child-pipes

Two conflicts, one of them a whole-file collision.

crates/rocm-core/src/process.rs: #599 extracted a module of the same
name out of lib.rs, so main and this branch each created a different
file at that path. Resolved as a union -- the two have zero name
overlap, main's being spawn/terminate primitives and `FileLock`, this
branch's being `run_with_timeout` and its `Capture` machinery, which is
exactly the module main just created for this kind of thing. Imports
merged (main's narrower `use std::process::Command` dropped for this
branch's wider one; `anyhow::bail` widened to unconditional and the
windows-only duplicate removed), and both test modules joined -- 17
`process::` tests now, main's three `FileLock` cases included.

docs/architecture.md: main added net.rs/paths.rs/process.rs bullets.
Kept them, folding `run_with_timeout` into the process.rs entry. The
previous merge had reinstated the claim that this is "the one bounded
child-process execution helper the apps share", which 6533327 removed on
purpose; rominf caught that, and it is not reinstated here. The bullet
now says plainly that other capture helpers still join their readers
unbounded.

Also fixes jussielo-amd's blocking finding: the wait loop's two `?`
early returns -- `try_wait` and the post-kill `wait` -- returned before
`take()` was reached, so `abandoned` was never set and each reader
thread kept appending to a buffer nobody would ever read, contradicting
`Capture`'s own documented bound. Both paths now abandon first.

Verified on Linux: clippy --workspace --all-targets -D warnings exit 0;
cargo test --workspace --all-targets 32 groups, the only failure being
the known `animated_spinner_keeps_ticking` load flake from #421, which
passes 3/3 in isolation.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
r0x0r added a commit that referenced this pull request Oct 13, 2026
…child-pipes

Two conflicts, one a whole-file collision.

crates/rocm-core/src/process.rs: #599 extracted a module of the same
name out of lib.rs, so main and this branch each created a different
file at that path. Resolved as a union -- zero name overlap between
them, main's being spawn/terminate primitives and `FileLock`, this
branch's being `run_with_timeout` and its `Capture` machinery, which is
exactly the module main just created for this kind of thing. Imports
merged (main's narrower `use std::process::Command` dropped for the
wider one; `anyhow::bail` widened to unconditional and the windows-only
duplicate removed), both test modules joined -- 17 `process::` tests,
main's three `FileLock` cases included.

docs/architecture.md: kept main's net.rs/paths.rs/process.rs bullets,
folding `run_with_timeout` into the process.rs entry. The previous merge
had reinstated the claim that this is "the one bounded child-process
execution helper the apps share", which 6533327 removed on purpose;
rominf caught that, and it is not reinstated. The bullet now says
plainly that other capture helpers still join their readers unbounded.

Also fixes jussielo-amd's blocking finding: the wait loop's two `?`
early returns -- `try_wait` and the post-kill `wait` -- returned before
`take()` was reached, so `abandoned` was never set and each reader
thread kept appending to a buffer nobody would ever read, contradicting
`Capture`'s own documented bound. Both paths now abandon first.

Verified on Linux: clippy --workspace --all-targets -D warnings exit 0;
cargo test --workspace --all-targets 32 groups, the only failure the
known `animated_spinner_keeps_ticking` load flake from #421, which
passes 3/3 in isolation.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
r0x0r added a commit that referenced this pull request Oct 13, 2026
Conflicts in `crates/rocm-core/src/lib.rs`, both from #599's extraction of
`net.rs`/`process.rs`/`paths.rs` out of this file:

- Imports: main narrowed them because the `windows_sys` users moved to
  `process.rs`. Took main's deletion of the `windows_sys` block (nothing
  left here uses it, and an unused import is a hard error on the Windows
  `-D warnings` lane) while keeping `OsStr`, `Command`, `Stdio`, `thread`
  and `Instant`, which this branch's `run_command_with_timeout`,
  `drain_on_thread` and `reap_in_background` still need.
- Tests: main moved `inference_readiness_is_withheld_while_the_model_only_lists`
  and `inference_probe_sends_the_service_key_to_a_protected_endpoint` to
  `net.rs`; dropped both here rather than resurrecting duplicates. The ten
  KFD-gate, pipe-drain and reaper tests this branch adds sat in the same
  hunk and are all kept.

Audited after the merge: `KFD_DEVICE`, `kfd_readable`/`_with`,
`device_readable`, `amd_smi_json`/`_with`, `run_command_with_timeout`,
`reap_in_background`, `drain_on_thread`, `join_drained`,
`MAX_QUOTED_OUTPUT_CHARS`, `AMD_SMI_SERVE_PROBE_TIMEOUT`,
`gpu_vram_usage_amd_smi`, `detect_gpu_count`, `run_rocm_capture_for_paths`
and `capture_amd_smi_json` all still resolve, with the same two external
callers of the timeout helper as before.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
r0x0r added a commit that referenced this pull request Oct 13, 2026
…h it

Merging main's `process.rs`/`net.rs`/`paths.rs` extraction (#599) into this
branch is a textbook silent semantic conflict: main deleted `use std::io::Read`
from `lib.rs` along with the code that needed it, this branch added
`drain_on_thread<R: Read + ...>` to the same file, and the two edits never
touched adjacent lines -- so git merged them without a marker and left a
module that cannot compile.

    error[E0405]: cannot find trait `Read` in this scope
        --> crates/rocm-core/src/lib.rs:2869:23

`run_command_with_timeout` and its drain threads are not `cfg`-gated, so the
import is live on every target and cannot trip the Windows lane's
`-D warnings` unused-import check.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
jussielo-amd pushed a commit to jussielo-amd/rocm-cli that referenced this pull request Oct 16, 2026
…g timeouts (ROCm#526)

* fix(rocmd): drain child pipes concurrently so output size stops faking timeouts

Captured output was collected by waiting for the child to exit and only
then calling `wait_with_output`, the one call that drains the pipes. The
pipes are bounded by the OS (~64KiB on Linux, less on Windows), so a
child writing more than that blocks in `write` and never exits, and the
drain is never reached. The only way out was the timeout, which meant it
measured how much the child printed rather than how long it took, and
killed successful commands mid-flight.

Reachable two ways: the `amd-smi static -a -g all --json` probe runs
under a 2s budget and clears a buffer easily on a multi-GPU host, so GPU
telemetry reported `amd_smi_available: false` with a spurious timeout
note into `bridge-snapshot`, the `examine_snapshot`/`bridge_snapshot`
tools and the automation watcher; and an MCP-driven `install sdk` prints
far more than a buffer, so it was killed partway, leaving a partial SDK.

Three byte-identical copies of the loop existed -- `rocmd`'s
`run_command_with_timeout` and `run_process_with_timeout`, and a third in
`rocm`'s `main.rs` -- so they are replaced by one
`rocm_core::process::run_with_timeout` following the reader-thread
pattern already in `examine.rs`. Both apps already depended on
`rocm-core`, so no new crate edge is introduced.

Also makes the timeout bound the call. Killing the child does not close
the pipe when it forked a grandchild that inherited the write end, so
collection blocked until that grandchild exited -- a 1s timeout took 30s
against `sh -c "sleep 30"`, because dash forks rather than execs. The
readers now publish into a shared buffer and are abandoned after a short
grace instead of joined.

Covered at both levels: unit tests for a child flooding both pipes and
for a timeout that must return on schedule, and scenario examine-20,
which drives `rocmd bridge-snapshot` against a stub `amd-smi` emitting
~150KiB. The scenario needs no GPU -- the stub is reached through the
`PATH` fallback -- so it runs on the mock lane rather than being stranded
on a multi-GPU host.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* test(e2e): isolate HOME so examine-20 actually reaches its stub amd-smi

examine-20 asserted it was hermetic, and it was not. `resolve_amd_smi_binary`
tries `default_data_dir()/runtimes/registry` and then the home fallbacks
before `PATH`, and both resolve through `runtime_home_dir()`, which reads the
real `$HOME` via `BaseDirs::new()` rather than `ROCM_CLI_DATA_DIR`.
`isolate_env` sets the three `ROCM_CLI_*` dirs plus the shared caches but
never `HOME`, so on a Linux host carrying a managed SDK under `~/.rocm` the
real `amd-smi` ran and the stub never did -- and the scenario then failed on
the size assertion, blaming the pipe-drain defect for an environment problem.

Point `HOME` at the isolated root for this one `rocmd bridge-snapshot`
invocation. Scoped to the step rather than added to `isolate_env`, because
other scenarios deliberately inherit the real HOME/XDG environment for their
PTY defaults and would change behaviour if it moved.

Both comments claimed the empty `<data>/runtimes` registry was sufficient on
its own, and the step's went further and said HOME was already isolated.
Neither held; both now describe the mechanism that does.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* test(process): pin the drain grace under any shell, and stop a read error passing for EOF

The timeout test only guarded anything where `/bin/sh` is dash. `sh -c "sleep
30"` leaves a grandchild under dash, which forks, but not under bash, which
execs the sleep into the shell itself -- so on a host where sh is bash, an
unbounded reader wait passed the test in 1.00s. Every grandchild case now
backgrounds explicitly, so a grandchild exists under either shell, and runs
under each shell present on the host.

Nothing covered the drain grace itself: taking the output the moment the child
exits passed all three existing tests, because a reader started at spawn has
already drained a normal child's output before the 50ms wait poll even notices
the exit. Output that reaches the pipe after the child is gone is the only way
to ask for that case on demand, and is now a test, alongside one for a child
that exits 0 while a background grandchild holds the pipe.

A read error was treated as EOF, so a failed pipe came back as a short `Ok`.
`wait_with_output`, which this replaced, retried `Interrupted` and returned
anything else; that is restored, and `run_with_timeout` reports it rather than
handing a caller truncated output to parse as JSON.

Also, each found by review of this branch:

- examine-20 asserted only the first amd-smi probe, despite being named for the
  telemetry surviving. A failed `monitor` probe still reports
  `amd_smi_available: true` with a full `static_snapshot`, so the scenario
  passed against half its own symptom; it now asserts both probes and a null
  note, and keeps rocmd's stderr for the failure message.
- The snapshot note rendered only `error.to_string()`, so a probe killed at its
  timeout was reported as `failed to launch amd-smi ...`. It now uses
  `{error:#}` and carries the cause, and the scenario comment says what the note
  actually said rather than calling it a timeout note.
- docs/architecture.md still placed command-timeout plumbing in rocmd's
  `common.rs`; it moved to `rocm_core::process` in this change.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* fix(process): bound the child output a timed-out command quotes

`run_with_timeout`'s timeout message quoted all of the child's captured
stderr -- or all of stdout when stderr was empty -- with no limit, and
this branch's `{error:#}` note (added so the note names the timeout
rather than the launch) renders that whole chain into the bridge
snapshot. The drain fix makes it worse, because more output is now
captured than before. The note is not a terminal message: the GPU-metrics
watcher copies it into the automation-event and audit JSONL files every
60s and both append helpers rewrite the whole file, so an amd-smi that
prints a snapshot's worth of JSON and then stalls past its 2s budget grew
both logs by that much per minute, each write costing more than the last.
Reproduced at 151,369 characters.

Bounded inside `run_with_timeout` rather than at the note in `common.rs`.
The quote is assembled there, that is the only place that knows those
characters are raw child output rather than something we wrote, and all
four callers -- both `common.rs` captures, the daemon sandbox and
`apps/rocm` -- inherit the bound instead of each needing its own. The
truncation follows the two existing in-tree precedents,
`examine.rs::truncate_to_chars` and `host_gpu.rs::truncate_diagnostic_line`:
head, sliced on char boundaries, with a visible `...[truncated]` marker.

Deliberately not covered: `capture_amd_smi_json`'s own non-zero-exit
`bail!` quotes all of stderr too. That one predates this branch -- it is
its own outermost context, so `.to_string()` already carried it unbounded
-- and belongs with the follow-up for the remaining unbounded capture
helpers rather than here.

The rest of the review round:

- e2e `examine-21` pins what the changed note now claims, end to end: a
  stub amd-smi that prints ~150KiB and then stalls must leave
  `amd_smi_available: false` and a note that names the probe, names the
  timeout, says it was shortened, and stays under 4000 characters.
  Reverting `{error:#}` to `.to_string()` fails it on the timeout claim;
  removing the bound fails it on the marker.

- Windows now runs the hazard tests instead of only the trivial one. A
  batch-file fixture gives it a both-pipes flood (cmd has no `yes`, and a
  script file avoids cmd's quote-stripping rules becoming the thing under
  test), and a flood-then-`ping -n 30` case that covers the timeout, the
  bound, and the grandchild in one: killing `cmd` does not kill `ping`,
  which keeps the inherited write ends open. The success case now asserts
  stderr on Windows too -- `1>&2` is a cmd redirection like any other, and
  skipping it left the second pipe unchecked on that platform.

- Three comments corrected against the code: the snapshot feeds the
  `gpu_snapshot` MCP tool, not the `examine_snapshot` sandbox tool, which
  returns an `ExamineSummary` only; only the timeout case was misnamed by
  the outer context, since the parse and non-zero-exit failures are
  already their own outermost context; and the isolated HOME note lives on
  `pty_env`, not `isolate_env`.

- `docs/architecture.md` no longer calls this the only bounded helper the
  apps share. `therock.rs::wait_with_output_bounded`, `examine.rs::run_raw`
  and `host_gpu.rs`'s two capture helpers still join their readers with no
  time limit; the doc now names them and says converging them is follow-up
  work.

Refs: ROCMAI-448

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* fix(core): bound the abandoned reader and test the result assembly

Addresses the review on ROCm#526.

`run_with_timeout` abandons a reader once `DRAIN_GRACE` expires, which is
what stops a grandchild holding the pipe from deciding how long a 2s probe
takes. But the abandoned thread kept appending to a buffer that `take()` had
already emptied and that nothing would read again, so in `rocmd` -- a daemon
that probes amd-smi every 60s -- each such call grew an allocation for as
long as the grandchild lived. `take()` now marks the capture abandoned under
the buffer's own lock, and the reader drains to EOF and discards from that
point, so the buffer provably cannot grow after `take()` returns.

Discarding rather than dropping the read end is deliberate: closing the pipe
would send the still-writing grandchild EPIPE/SIGPIPE, which for the
background helpers a sandboxed installer legitimately leaves behind trades a
leak we can bound for a behaviour change we cannot predict. The thread still
lives until the grandchild closes the pipe; that is now stated in `Capture`'s
docs rather than left to be discovered.

The post-wait result assembly moves into `assemble_output`, which makes its
branches reachable from a unit test for the first time: a read failure on
either stream is reported rather than returned as truncated output, a timeout
outranks a read failure caused by the kill, and the timeout quote prefers
stderr, falls back to stdout, and says "no output" when there was none. Every
previous timeout fixture wrote to stderr only, so the fallback chain could
have been reordered unnoticed.

`truncate_to_chars` was a byte-identical copy of `examine.rs`'s; that one is
now `pub(crate)` and shared. Its doc claimed `host_gpu.rs`'s
`truncate_diagnostic_line` used the same marker -- it appends `...`, not
`...[truncated]` -- so the claim is corrected rather than repeated.

Also: state the stdio contract on `run_with_timeout` and drop the three
callers' redundant settings, which it overrode anyway.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* Merge remote-tracking branch 'origin/main' into fix/rocmai-448-drain-child-pipes

Two conflicts, one of them a whole-file collision.

crates/rocm-core/src/process.rs: ROCm#599 extracted a module of the same
name out of lib.rs, so main and this branch each created a different
file at that path. Resolved as a union -- the two have zero name
overlap, main's being spawn/terminate primitives and `FileLock`, this
branch's being `run_with_timeout` and its `Capture` machinery, which is
exactly the module main just created for this kind of thing. Imports
merged (main's narrower `use std::process::Command` dropped for this
branch's wider one; `anyhow::bail` widened to unconditional and the
windows-only duplicate removed), and both test modules joined -- 17
`process::` tests now, main's three `FileLock` cases included.

docs/architecture.md: main added net.rs/paths.rs/process.rs bullets.
Kept them, folding `run_with_timeout` into the process.rs entry. The
previous merge had reinstated the claim that this is "the one bounded
child-process execution helper the apps share", which 6533327 removed on
purpose; rominf caught that, and it is not reinstated here. The bullet
now says plainly that other capture helpers still join their readers
unbounded.

Also fixes jussielo-amd's blocking finding: the wait loop's two `?`
early returns -- `try_wait` and the post-kill `wait` -- returned before
`take()` was reached, so `abandoned` was never set and each reader
thread kept appending to a buffer nobody would ever read, contradicting
`Capture`'s own documented bound. Both paths now abandon first.

Verified on Linux: clippy --workspace --all-targets -D warnings exit 0;
cargo test --workspace --all-targets 32 groups, the only failure being
the known `animated_spinner_keeps_ticking` load flake from ROCm#421, which
passes 3/3 in isolation.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* fix(core): abandon both readers when the wait loop itself fails

Addresses the review on ROCm#526.

`run_with_timeout` starts a reader thread per pipe before it waits, and
`Capture::take()` is the only thing that marks a reader abandoned -- which is
what stops it appending to a buffer nobody will read again once a grandchild
holds the write end open. The wait loop's two error paths, a failed `try_wait`
and a failed post-kill `wait`, returned straight out of the function before
either `take()` was reached, so both readers kept accumulating for as long as
the grandchild lived. In `rocmd`, which probes amd-smi every 60s, that is
unbounded growth, and it contradicted the bound `Capture`'s own doc comment
promises.

The wait loop moves into `wait_bounded`, so the two `?`s become one `Err` arm
in `run_captured` that abandons both captures before propagating. One error
path instead of two is the point: a third `?` added to the loop later cannot
reintroduce the leak. `wait_bounded` is generic over a new private `Waitable`
trait purely so the two failures are reachable from a test -- provoking either
from a real child needs the kernel to refuse to reap a process. Both new tests
fail against the previous shape, leaking ~434KB per stream.

The read-error branch now checks `abandoned` under `buffer`'s lock, the same
one `take()` marks it under, closing the window where a genuine read error
could be stored into a failure slot `take()` had just drained.

`assemble_output` builds its `Output` first and quotes through
`command_failure_detail`, the picker ROCm#584 centralised, instead of repeating the
stderr/stdout/"no output" chain.

`docs/architecture.md` names the four sibling helpers that still join their
readers unbounded and links ROCm#623, and closes the aside in the `common.rs`
bullet. The stub payload comments say ~203KiB (measured: 207,691 bytes, ~3.2
pipe buffers, ~52x the note ceiling) rather than ~150KiB, `write_stub_payload`
says three scenarios, and examine-22's Then step says it checks a bounded note.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* fix(core): silence use_self on the Waitable forwarding impl

clippy::use_self wants `Self::try_wait(self)` inside
`impl Waitable for Child`. `Self` is `Child` there, so that spelling
reads as the trait method being defined rather than the inherent one it
forwards to. Allowed locally with that reason rather than taking a
rename that makes the forwarding harder to follow.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* test(e2e): assert examine-21/22 notes still quote amd-smi's output

Both steps claimed, in their own comment, that their checks "pull against
each other: dropping the child's output entirely would satisfy the size
check while losing the diagnostic". They did not. The only assertions were
`contains("...[truncated]")` and a 4000-character ceiling, and
`truncate_to_chars(output, 0)` returns exactly `"...[truncated]"` -- so a
quote bound of zero satisfied every assertion in both steps and left a note
with none of amd-smi's output in it. The unit test
`assert_timeout_quote_is_bounded_and_useful` does pin this through
`FLOOD_CANARY`, but the scenarios claimed the end-to-end property and could
not fail on it.

Both steps now also assert the note contains a field value from the first
GPU in the stub payload, which falls inside the quoted head. The literal is
shared with the payload as `STUB_MARKET_NAME` so the assertion cannot drift
away from what the stub actually prints.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* test(e2e): make the quote assertion mean the head, not just non-empty

Every one of the 1200 stub GPUs carried the same market_name, and each
object is short enough that any 2000-character window holds several
copies. `note.contains(STUB_MARKET_NAME)` therefore proved the quote was
non-empty; a quote that kept the tail, or any middle window, passed it
identically. The doc comment and both panic messages said "head".

Give GPU 0 its own name and assert that. The comment was the side that
was wrong, but the weaker reading is not the one worth keeping: the
quote is built by `truncate_to_chars`, which takes the leading
characters, so a truncation that dropped the head is a real regression
this scenario should catch and could not.

Also drops two stale claims from that comment: the first GPU object is
not ~150 characters, and the quoted text is stderr when stderr is
non-empty and stdout otherwise, not "what the child printed".

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* fix(process): kill the child when try_wait fails, and write the amd-smi stub without the ETXTBSY race

`wait_bounded`'s poll error was the one exit from the wait loop that left
the child running: the `?` propagated with neither `kill()` nor `wait()`,
and `run_captured` only abandons the two captures on that path, so the
child was dropped unreaped. Kill before propagating, matching the timeout
branch's best-effort `let _ =` style.

The existing abandon test only watched the readers, so it passed either
way. `FailingChild` now records `kill`, and the shared helper asserts it
for both wait-loop failure paths.

`write_amd_smi_stub` used `std::fs::write` + `set_permissions`, which
reintroduces the fork race `write_fake_executable` exists to close
(ROCm#649). The new scenarios are `@requires-os:linux` with no
`@requires-gpu`, so they run up to 64-wide on the no-GPU lane, where a
leaked write fd in a sibling thread's child turns the stub's first exec
into an intermittent `Text file busy`. Use the helper, as every other
`*_steps.rs` in that directory already does.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* docs(process): say what the module now does, and stop two comments overclaiming

Review follow-up on the three non-blocking items. Comments only; no
behaviour change, and the diff touches no executable line.

- The module doc still described spawn/terminate primitives and
  `FileLock` only. `run_with_timeout` and its supporting cast are the
  bulk of the file now, and `docs/architecture.md` already names it, so
  the two disagreed about what this module is for. Name it, say what it
  guarantees, and point at ROCm#623 for the helpers not yet converged onto
  it.

- `DRAIN_GRACE` claimed a cleanly-exited child "has at most a buffer or
  two left to collect", on the grounds that the readers run throughout.
  They are *started* before the wait loop but not necessarily scheduled:
  `wait_bounded` polls `try_wait` once with no sleep in front of it, so a
  fast enough child can be observed exited before either reader thread
  has run an instruction. Not a logic bug — 200ms absorbs it with room to
  spare — but the comment claimed more than the code structurally
  provides. Say that the grace covers reader start-up too.

- The `abandoned` guard in the reader's read-error branch is kept, with
  its comment corrected to the case it actually carries. After `take()`
  has returned the guard is only tidy: `take()`/`abandon()` run once per
  `Capture`, so a failure stored later does sit unread. The window it
  genuinely closes is *inside* `take()`, which marks `abandoned` and
  empties the buffer under `buffer`'s lock, releases it, then drains the
  failure slot under a different lock. A read error landing in that gap
  would come back attached to bytes already handed to the caller, and
  `assemble_output` turns any failure on a non-timed-out run into a hard
  "failed to read the output of ..." — a cleanly finished command
  reported as a read failure. So it is not inert, and the lock-before-
  check ordering from the earlier TOCTOU fix is what closes it; both
  stay.

Refs: ROCMAI-448
Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* test(process): pin the empty stdin the helper promises its children

`run_with_timeout` took the null-stdin setting over from its three
callers and its doc now tells them not to set it, but nothing failed
when that line was removed: 19 process tests passed against a mutant
without it. A child that reads stdin would then inherit the daemon's,
wait for input that never comes, and be killed at the deadline.

The new case hands `run_with_timeout` a caller-set stdin that stays
open for the whole call, so the child reaches EOF only if the helper
really does replace it; otherwise the call comes back as a timeout.
Added for cmd as well, since both `rocmd` callers run on Windows.

Also three comments that said more or less than the code does: the
`{error:#}` note gains its causes on the spawn failure too, not only
on the timeout; the monitor branch is pinned by `examine-26` rather
than the `examine-25` it inherited from the branch above; and the
quote bound covers this helper's own message, not every persisted
quote of child output. The `Capture` doc now says that what it bounds
is the memory, not the number of parked reader threads.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* docs(process): say what the stdin case cannot tell apart

The new case pins `null` against "no setting at all", which is the
regression that can happen. It cannot pin `null` against `inherit`:
that needs the test process's own fd 0 to be something other than
EOF, and arranging it is a `dup2` racing every other test in the
binary. An `inherit` mutant was run and passes, so the limit is
written down rather than left for the next reader to rediscover.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* docs: correct the caller count, the ROCm#623 attribution and the spawn note

Four comment defects, no behaviour change.

- The stdin test helper said "all three callers"; `run_with_timeout` has
  four call sites (`common.rs:134`, `common.rs:227`, `sandbox.rs:121`,
  `main.rs:8314`), which is what the `MAX_QUOTED_OUTPUT_CHARS` doc already
  says.
- `MAX_QUOTED_OUTPUT_CHARS` claimed ROCm#623 tracks both unbounded quotes. It
  lists only the `capture_amd_smi_json` one; the daemon's
  `parse_sandbox_child_output` is named nowhere in it, so the comment is
  narrowed to what the issue actually records rather than editing the issue.
- The `{error:#}` note quoted Linux's spawn error text for a probe `rocmd`
  also runs on Windows, called every no-GPU host an amd-smi-less host
  (`resolve_amd_smi_binary` falls back to a bare `amd-smi` on PATH, so such
  a host fails at a non-zero exit instead), undercounted the failures that
  gain causes this way, and let "Pinned by examine-25" trail a claim
  examine-25 does not assert. Rewritten to pin only the timeout claim.
- `WindowsFixture`'s doc still described a payload that only `flooding`
  writes after the `script`/`flooding` split.

Also records why the `inherit` mutant is left documented rather than caught
by a self-re-execing test.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

---------

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
mikeroySoft pushed a commit to mikeroySoft/rocm-cli that referenced this pull request Nov 6, 2026
…8457) (ROCm#447)

* fix(serve): run the KFD pre-flight at every amd-smi launch site (EAI-8457)

`amd-smi` launched against a kernel fusion driver in a bad state blocks in
uninterruptible sleep, which no signal escapes — so `rocm serve` hangs and
survives both Ctrl-C and SIGKILL. The repo documents that and calls the
`/dev/kfd` pre-flight mandatory, but three of the four production launch
sites skipped it. Two of those, `gpu_vram_usage` and `detect_gpu_count`,
sit on the normal `rocm serve` path for any run that is not CPU-only, and
had no timeout either.

Give the pre-flight one definition in `rocm-core` and route 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 on purpose. `/dev/kfd` exists nowhere else, and
`amd-smi.exe` is a supported Windows binary with a live Strix Halo lane, so
a check that merely failed to find the device would have disabled GPU
detection on every Windows host.

Two consequences worth naming:

- On Linux the two `serve` probes now decline to launch when `/dev/kfd`
  cannot be opened, where before they launched and usually failed anyway.
  GPU auto-selection degrades to service state alone, which is what it
  already does whenever amd-smi is unavailable.
- `run_command_with_timeout` now drains both pipes on their own threads.
  Polling `try_wait` without draining deadlocks once the child fills the
  64 KiB pipe buffer — which `amd-smi metric --json` clears easily on a
  multi-GPU host — and that deadlock surfaced as a spurious timeout.
  Routing the serve path through the helper without this would have traded
  the hang for silent loss of VRAM telemetry on exactly the machines that
  matter. Its timeout message no longer quotes partial output, because
  collecting it means waiting on a child that may be the unkillable one.

The dash collector keeps its own pre-flight: it already runs one, it needs
an async launcher, and an edge from `rocm-dash-collectors` to `rocm-core`
is what the `crate_edges` layering guard exists to stop. Its check is not
cfg-gated, so the dash detects no GPU off Linux — a separate pre-existing
bug, untouched here.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* style: apply rustfmt to the amd-smi pre-flight change

The hand-wrapped import list and the amd_smi_json call did not match what rustfmt produces; the prek hygiene lane caught it.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* fix(serve): wave the KFD pre-flight through on WSL (EAI-8457)

WSL is `target_os = "linux"`, so it compiled the real-open branch of
`kfd_readable` -- but it has no `/dev/kfd` at all. It reaches the GPU
through `/dev/dxg` and the Windows host driver, which is exactly why the
WSL diagnosis catalog skips every bare-metal driver check (docs/wsl.md,
examine::WslFacts).

So the open always failed there and `amd_smi_json` declined every launch:
`gpu_vram_usage` and `detect_gpu_count` on the ordinary serve path, and
the daemon bridge snapshot. Both serve probes discard the error with
`.ok()?`, so this was a silent loss of GPU detection on a supported
platform -- one with its own diagnosis catalog and its own GPU E2E lane.
The D-state hazard the gate exists for is an amdgpu/KFD one, which is
precisely the stack WSL never loads.

Wave WSL through at the top of the Linux `kfd_readable`, with the
determination injected via `kfd_readable_with` so the behaviour can be
pinned from an ordinary Linux host. Correct the `KFD_DEVICE` comment,
which asserted no other platform exposes one without excluding WSL.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* fix(core): reap the child the amd-smi timeout path kills (EAI-8457)

`run_command_with_timeout` killed a timed-out child and then dropped the
handle. `std::process::Child` has no reaping `Drop`, so the corpse stayed
in state `Z` for the lifetime of the parent. Both per-app copies this
function replaced called `wait_with_output()` after the `kill`, so the
reap used to happen; consolidating them dropped it.

That is a real leak rather than a tidiness point because one caller is a
daemon. `rocmd` probes `amd-smi` with a 2s timeout from its watcher tick,
twice per GPU-telemetry collection, on a 60s cadence. A host whose
`/dev/kfd` is readable but whose `amd-smi` simply overruns 2s — ordinary
on a busy multi-GPU box — therefore accrues zombies indefinitely. The KFD
pre-flight added here does not cover it: that only prevents the spawn when
the device is unreadable.

Restoring the `wait` inline would reintroduce the hang the timeout exists
to bound, since an uninterruptible-sleep child blocks the `wait` exactly
as it ignores the `kill`. Hand the child to a detached reaper thread
instead, so the calling thread still returns promptly. The reap runs
whether or not `kill` succeeded, because the likeliest failure is a child
that exited between the poll and the kill — already a zombie, and already
in need of reaping.

The doc comment is explicit that this is best-effort: an unkillable child
parks its reaper instead of leaving a zombie, trading a stranded PID for a
stranded thread. That is better than blocking the caller and no worse than
the leak it replaces, but it is still a leak, and the pre-flight remains
the only actual mitigation. Reapers do not accumulate otherwise — one per
timeout, each exiting as soon as its child does.

Covered by a Linux-only test that reads the child's recorded pid straight
out of `/proc`: without the reap it observes state `Z` for the full ten-
second poll budget, with it the entry is gone in under a second.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* test(core): assert the timed-out child is gone, not merely not-a-zombie (EAI-8457)

`assert_ne!(state, Some('Z'))` was satisfied by two different outcomes: a
child the timeout path killed and reaped, and a child it never killed at
all, which sits in `S` while it sleeps on. Only one of those is the
contract, so the assertion could not tell a working timeout path from a
broken one. Assert the `/proc` entry is gone instead, and retry on any
surviving state rather than only on `Z` so neither the post-`SIGKILL`
teardown transient nor the detached reaper reads as a pass.

Mutation-checked: unmutated passes; dropping `child.kill()` fails with
`Some('S')` where the old assertion passed; dropping
`reap_in_background` still fails with `Some('Z')`.

Also record why the declined-launch behaviour has no Gherkin scenario, as
AGENTS.md section 3 requires when one cannot be written. The premise needs a
host that is GPU-present by the KFD/DRM sysfs rule yet cannot open
`/dev/kfd`; no lane is that host and the harness cannot manufacture it.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* fix(serve): restore the low-VRAM test hook my merge dropped

Resolving the `gpu_vram_usage` conflict after ROCm#251 was squash-merged, I kept
this branch's pre-refactor body and then restored main's dispatcher -- but not
`simulated_low_vram_usage()` or its `#[cfg(feature = "e2e-test-hooks")]` call,
both of which arrived with ROCm#251. The symbol count fell from main's 4 to 2,
leaving only the two dangling references in the test module.

Nothing in the default build reaches the hook, which is why 21 CI checks
stayed green over it. It breaks only under the feature:

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

and the step at serving_steps.rs:865 still set ROCM_E2E_FORCE_LOW_VRAM with
nothing left to read it -- a scenario quietly asserting nothing.

Restored verbatim from main: both the hook and its gated call are now
byte-identical to origin/main, and the symbol count matches again.

Verification note: the Linux devbox is unavailable (its pod has been Pending
~90m behind a 502 from the node proxy) and macOS cannot build this workspace
-- `cargo check` stops in rocm-engine-lemonade on `u32 & u16` long before
reaching this crate -- so the feature-gated compile has NOT been re-run here.
It is pending, and the restored text is identical to the known-good tree.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* docs(serve): put the VRAM doc comments back on their own functions

My restore of the low-VRAM hook in a3085af spliced the function in
between `gpu_vram_usage`'s doc comment and `gpu_vram_usage` itself. The
result compiles, so CI stayed green, but the amd-smi/sysfs fallback
prose ended up describing `simulated_low_vram_usage`, `gpu_vram_usage`
was left undocumented, and the hook's own twelve-line comment --
explaining why the seam exists and that production builds never see it
-- was dropped entirely.

Restores both comments to the functions they describe, matching 70d6aa1
(the last commit before the merge that dropped the hook) byte for byte.
No executable code changes: `gpu_vram_usage`'s body is moved verbatim,
and moving a top-level item is inert.

Verified without a build host: the region is byte-identical to
70d6aa1, the diff touches no statement, `rustfmt --check --edition
2024` parses the file and reports no formatting change, and the three
intra-doc link targets (`AUTO_FREE_VRAM_FRACTION`,
`vram_capacity_is_meaningful`, `read_drm_vram_usage`) all still resolve.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* fix(core): address review on the amd-smi KFD pre-flight

Blocking items from juhovainio's review and the automated one, on top of
this round's merge of main.

Unbounded output (the one that reinstates a defect ROCm#526 already fixed):
`amd_smi_json_with` pasted a failing `amd-smi`'s whole stdout/stderr into
the error. `rocmd` renders that into its snapshot note and the GPU-metrics
watcher copies the note into rewrite-the-whole-file JSONL logs on every
poll -- the path that produced a 151K-character log line. Bounded with
`MAX_QUOTED_OUTPUT_CHARS = 2000` through `examine::truncate_to_chars`,
deliberately the same name, value and helper `fix/rocmai-448-drain-child-
pipes` uses one layer down, so converging the two is a deletion rather
than a behaviour change. No new abstraction is introduced here.

WSL wave-through: `is_wsl_host()` alone repeated the mistake `b4f98bc2`
corrected in the dashboard pre-flight -- a WSL host whose ROCDXG plumbing
is not ready is one where amd-smi still hangs. Now
`is_wsl_host() && has_usable_amd_gpu()`, the same conjunction
`dash.rs::gpu_reachable_for_preflight` feeds the dashboard, so the
cross-surface claim at `rocm-dash-collectors/src/amd_smi.rs` stays true.
`&&` short-circuits, so bare metal never pays for the extra probe.

Hardware-probe routing: main's `hardware_root` contract says every
`/dev/kfd` read goes through `host_path`. The gate now does, with the real
node keeping a veto exactly as the dash pre-flight does, and the module
doc lists it.

rocmd kept its own unprotected `run_command_with_timeout` for
`run_rocm_capture_for_paths`, reachable from the MCP bridge: the same
pipe-fill deadlock fixed everywhere else, and its timeout error quoted the
child's whole output. Deleted for the shared helper.

Also: `amd_smi_preflight_ok` was dead public API describing a call graph
the code did not have -- deleted, docs repointed; three doc comments that
contradicted the rewritten `run_command_with_timeout` corrected; the gate
doc and decline message narrowed to what a read-open actually detects
(inaccessible, not unhealthy); `drain_on_thread` uses `thread::Builder` so
a thread-creation failure cannot panic out of the daemon; the e2e KNOWN
GAP note corrected where its premises no longer hold.

New tests, each confirmed to fail without its fix:
- a_failing_amd_smi_cannot_paste_its_whole_output_into_the_error
- the_public_amd_smi_entry_point_gates_on_the_real_kfd_device
- the_kfd_gate_honours_a_simulated_root_but_keeps_the_real_device_veto
- the_kfd_preflight_waves_through_wsl_hosts_only_when_the_gpu_is_reachable

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* test(core): make the gate-wiring test non-vacuous under a simulated host root

On a machine that has a readable /dev/kfd the comparison alone is
vacuous: both the gate and a wrapper passing some other readable path
say "allow". Under an e2e-test-hooks build an empty simulated root has
no dev/kfd, so the gate declines on every host while the obvious mutant
(Path::new("/")) re-roots to the root directory and opens.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* test(core): serialise the device-gate tests on the simulated-root lock

The gate now resolves its device through host_path, so an e2e-test-hooks
build in which one test sets ROCM_CLI_TEST_HOST_ROOT re-roots another
test's tempdir device out from under it. Same lock fix.rs already holds
for the same reason.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* fix(core): hoist a test const above the statements clippy objects to

`clippy::items_after_statements` rejects `const PAYLOAD_CHARS` declared
part-way through the test body: items exist from the start of the scope
regardless, so declaring one mid-function reads as if it did not.

Caught by the full workspace clippy run, which the agent that wrote this
test did not reach before stalling.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* test(core): spell the host-root lock so the env-mutation guard sees it

`xtask`'s env-mutation contract failed on this test:

  crates/rocm-core/src/lib.rs:6757: RestoredEnvVar::set

The lock was already held -- `host_root_guard()` on the line above takes
HOST_ROOT_TEST_LOCK -- but that guard matches TEXT, recognising a
serializer by the `_TEST_LOCK` suffix or the name `ScopedTestEnv`. The
helper's name carries neither, so a correctly serialized test read as an
unguarded mutation. This is the only test here that both takes the lock
and calls `RestoredEnvVar::set`, which is why the other three using the
helper pass.

Acquire the same lock by name instead, with a comment so the next person
does not "simplify" it back to the helper.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* fix(core): make the off-Linux KFD wave-through a const fn

The Windows lane builds with `-D warnings` and runs clippy, so
`clippy::missing_const_for_fn` turned the `cfg(not(target_os = "linux"))`
stub into a hard compile error there. The Linux body opens a device and
cannot be const, so the lint only ever fires on the other branch of the
pair -- which no Linux-side gate can catch.

error: this could be a `const fn`
  --> crates\rocm-core\src\lib.rs:4619:1
Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* fix(core): restore the io::Read import the lib.rs extraction took with it

Merging main's `process.rs`/`net.rs`/`paths.rs` extraction (ROCm#599) into this
branch is a textbook silent semantic conflict: main deleted `use std::io::Read`
from `lib.rs` along with the code that needed it, this branch added
`drain_on_thread<R: Read + ...>` to the same file, and the two edits never
touched adjacent lines -- so git merged them without a marker and left a
module that cannot compile.

    error[E0405]: cannot find trait `Read` in this scope
        --> crates/rocm-core/src/lib.rs:2869:23

`run_command_with_timeout` and its drain threads are not `cfg`-gated, so the
import is live on every target and cannot trip the Windows lane's
`-D warnings` unused-import check.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* test(core): plant the fake amd-smi through the ETXTBSY-safe writer

Both new amd-smi tests wrote their stand-in binary with `fs::write` plus
`set_permissions` and then executed it. Main's ROCm#649 landed
`fake_executable::write_fake_executable` precisely because that pattern races
every other test thread in the same process: a parallel `fork` inherits the
writer's fd and the kernel refuses to `exec` a file anyone still holds open
for writing.

The merge brought the helper in but could not retrofit these call sites, and
the race duly fired on a 112-core run:

    ---- tests::the_kfd_gate_stops_amd_smi_before_it_is_spawned stdout ----
    a readable device must let the launch through:
      0: failed to spawn child process
      1: Text file busy (os error 26)

Routing both plants through the helper makes the gate test deterministic
again; `cargo test -p rocm-core --lib` now passes three runs in a row where
it failed one in two before.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* fix(rocmd): route the sandbox timeout through the consolidated helper

Addresses review on PR ROCm#447.

`apps/rocmd/src/sandbox.rs::run_process_with_timeout` was a third copy of the
poll-and-kill loop from the same split (e760d9a) that produced the amd-smi
ones, and carried the same defect: it polled `try_wait` with both pipes
undrained, so a sandbox child that filled the 64 KiB pipe buffer blocked
mid-write and was then reported as having exceeded its timeout. Route it
through `rocm_core::run_command_with_timeout`, which drains both pipes on their
own threads and reaps a killed child on a detached thread.

Bound the gate's own `/dev/kfd` open so `kfd_readable`'s safety no longer rests
on the unverifiable assumption that `kfd_open` always returns promptly. An open
that overruns `KFD_OPEN_TIMEOUT` reads as "not readable", which is the declining
answer, so the unanticipated case fails in the safe direction. Pinned by a test
against a FIFO with no writer, whose `open` really does block in the kernel.

Deduplicate the stderr-then-stdout precedence `amd_smi_json_with` had inlined
into `command_failure_detail_or_else`, which `command_failure_detail` now
delegates to; only the both-blank fallback differs between the two callers.

Document the `serve` worst-case probe latency (two sequential 10s bounds,
replacing two unbounded `.output()` calls) and why `rocmd`'s 2s bound on the
same helper is deliberately shorter.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* fix(core): create the blocking-open fixture without unsafe

The workspace lints deny `unsafe_code`, so `libc::mkfifo` cannot be called
directly; shell out to `mkfifo(1)` and skip when it is absent. Also narrow
sandbox.rs's `std::thread` import to `cfg(test)`: the Linux timeout no longer
runs its own sleep-poll loop.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* test(core): watch the blocking-open probe so an unbounded mutant fails, not hangs

Calling the probe inline meant a mutant that drops the timeout from
`device_readable_within` would block forever on the FIFO and hang the test
binary — a CI timeout rather than a readable failure. Run it on a watched
thread with a much looser deadline so that mutant reports the cause instead.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* test(core): move the fifo path into the probe thread instead of cloning it

`clippy::redundant_clone`: the fixture path has no use after the spawn,
so the clone was dead. The thread can take it by move.

Caught by CI, not locally: the devbox run that cleared this branch was
made against the merge commit, before this test existed.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* test(e2e): reach both sides of the KFD pre-flight from a scenario

The `/dev/kfd` pre-flight refuses to spawn `amd-smi` when the device
cannot be opened. The per-PR E2E lane has no `/dev/kfd`, so every
scenario that plants a stub `amd-smi` was refused before the stub ran:
examine-24/25/26 (the pipe-drain regression tests from ROCm#526) started
asserting against a decline instead of the behaviour they are about.

`ROCM_CLI_TEST_HOST_ROOT` cannot open the 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 -- the invariant that stops a simulated root launching
`amd-smi` where there is nothing to talk to.

So add `ROCM_E2E_KFD_DEVICE`, honoured only by an `e2e-test-hooks`
build, which moves the device the gate opens. It is an override, not a
bypass: the probe still runs, against the named path, and a shipped
build has no override at all. Tagging the three scenarios `@requires-gpu`
was the alternative and was rejected -- examine.feature says in terms
that they are built to need no GPU so they run per PR, and a GPU tag
would strand ROCm#526's regression tests on a nightly lane.

The seam also makes the declined branch observable, which AGENTS.md §3
asks for and the 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, and -- the load-bearing half -- that the stub
`amd-smi`, which would have answered, never ran.

Signed-off-by: Roman <roman.sirokov@amd.com>

* test(e2e): declare the KFD pre-flight scenario last in examine.feature

`feature_naming` requires the per-feature scenario index to follow
declaration order, so a new examine-29 cannot sit beside examine-26.

Signed-off-by: Roman <roman.sirokov@amd.com>

* docs(serve): stop AMD_SMI_SERVE_PROBE_TIMEOUT calling itself serve-only

`gpu_vram_usage_amd_smi` is also reached from `host_accelerator_memory`,
and so from the `--model` readiness path, not only from `serve`. The
budget is still the right one there -- same question, same one-shot
cost -- but the doc said something untrue about where it is spent.

Signed-off-by: Roman <roman.sirokov@amd.com>

* fix(dash): bound the dashboard collector's /dev/kfd probe too

`rocm-dash-collectors` runs its own pre-flight before spawning `amd-smi`
for the dashboard's GPU metrics, on the same `/dev/kfd` and for the same
reason as the CLI launch sites. It used a plain
`OpenOptions::open(..).is_ok()`, so it stayed the one launch site that a
wedged device could still block indefinitely — the exact hazard the rest
of EAI-8457 removes.

It now shares `rocm_core::device_readable_bounded`, which carries the 2s
bound and the in-flight probe budget with it, instead of restating them.
A second copy of that primitive would be free to drift from the gate it
belongs to.

That needs a new first-party edge, `rocm-dash-collectors -> rocm-core`,
so `ALLOWLIST` gains it and the module doc records why. The invariant
that forbids this edge binds `rocm-dash-tui`, the presentation layer,
which makes no such call and keeps the stricter rule.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* docs(amd-smi): document the KFD seam and examine-31, and state the gate's real scope

docs/testing.md had no entry for the `ROCM_E2E_KFD_DEVICE` seam or scenario
examine-31, which the repo guidelines ask for in the same change.

The gate declines on a failed `open` as well as on one that never returns, and
the first class cannot cause the D-state hang the rationale described. Keep the
fail-safe behaviour -- narrowing it would mean deciding per errno which kernel
states are safe to launch into, an unverifiable list of exactly the kind the
bounded open exists to avoid resting on -- and say so, including what the
broader refusal costs `detect_gpu_count` and what answers in its place.

Prune the doc comments to the invariants: drop the review-round narration, the
"this used to be X" archaeology and the two citations of a commit hash that does
not exist in this repository.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* fix(dash,core): keep the bounded KFD probe off the async worker

Review round 3 on ROCm#447.

- `AmdSmiCollector::new` reached `device_accessible` -> the bounded open
  from an `async fn`, parking a tokio worker for up to the 2s
  `KFD_OPEN_TIMEOUT` per probe — ~4s under a simulated host root, where
  there are two. The pre-flight now runs through
  `preflight_passes_off_thread`, which short-circuits the two non-device
  branches before the thread hop and puts the rest on
  `spawn_blocking`. A `JoinError` reads as "not readable", matching
  `device_readable_within_budget`: every branch that cannot get an
  answer declines rather than opening on the caller's thread.

- The timeout and budget tests returned `Ok(())` when `mkfifo(1)` was
  missing, and on WSL, which is indistinguishable from a pass for a test
  that asserted nothing. Both now announce the skip through a shared
  `skip_notice`.

- `command_failure_detail_or_else` is `pub(crate)`; its only caller
  outside its module is `amd_smi_json`, in this same crate.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* docs(core,dash): stop claiming the dash crates have no rocm-core edge

This PR added `rocm-dash-collectors -> rocm-core` so the dashboard's
`amd-smi` launch could share the bounded `/dev/kfd` probe, which made six
doc comments false as written. The invariant that survives binds only the
presentation layer: `rocm-dash-tui` has no edge to `rocm-core` and
`crate_edges.rs` still forbids it one. The collectors crate does have one,
and says what for.

`runner.rs` and the collector's own docs additionally explained the
threading of `gpu_reachable`/`host_kfd_device` by the missing dependency,
which is no longer the reason: the verdict is threaded through because
`serve`, `examine` and the dashboard must not reach it independently, not
because the collector could not compute it.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* test(amd-smi): fail the build when a launch site skips the KFD pre-flight

The pre-flight protects nothing unless every `amd-smi` launch goes through
it, and nothing in the tree failed when one stopped: reverting either
`gpu_policy.rs` probe to `Command::new(resolve_amd_smi_binary())` left the
whole suite green, because the regression is the absence of a call and no
behavioural test can observe that.

So guard the ingredient. The managed SDK ships `amd-smi` inside the runtime
wheel's bin directory rather than on `PATH`, so `resolve_amd_smi_binary` is
the only way to find a binary to launch; a static contract test now fails
any file outside a two-entry, justified allowlist that calls it.
Bidirectional like the crate-edge allowlist, so an exemption cannot outlive
its reason. Verified by mutation: a raw spawn in `detect_gpu_count` fails
`only_preflighted_sites_resolve_the_amd_smi_binary` by name.

It is a source-text scan, not a call-graph analysis, and the module doc says
so rather than overclaiming.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* fix(serve): say why the amd-smi probe declined, and pin the quoted latencies

Both serve-path probes return `Option`, so a decline had nowhere to go and
was dropped with `.ok()?`. That silenced the two cases a user wants: an
`amd-smi` that ran and failed, and a false decline when the 2s open bound
trips on a slow but healthy host. `amd_smi_probe_json` now logs the full
error chain at `debug` -- not `warn`, because on a GPU-less host it is the
expected answer on every `serve` -- so "why did `--gpu auto` not rank my
GPUs?" is answerable from `RUST_LOG=debug`.

The gate's 2s bound and 8-slot budget are restated as literal numbers in
five places, two of them as derived figures (caller timeout + 2s), none of
which can see the constant. A tripwire test now fails on a change to either
and names every restatement to update.

Also state plainly, in `kfd_readable`'s docs and `docs/testing.md`, what the
gate is not: it proves only that `open(/dev/kfd)` returned, while the hang
is attributed to amd-smi's later ioctls, and no unhealthy-driver run backs
it up. It is a mitigation heuristic. And the decline is broader than the
ticket -- an absent node and EACCES decline too, and `detect_gpu_count` has
no sysfs fallback.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* fix(dash): cfg-split device_accessible so the Windows lane builds

`device_readable_bounded` waves through with a literal off Linux, which
makes this wrapper `const`-able there, and `clippy::missing_const_for_fn`
is a hard error on the Windows lane's `-D warnings` build. Split into a
Linux body that opens the device and a non-Linux `const fn`, the same
shape `kfd_readable` and `device_readable_bounded` already use.

Caught by CI's Windows job, not locally: the lint only fires on the
branch the Mac and the Linux box never compile. Verified this time by
running clippy over every crate that does build on Darwin.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* fix(amd-smi): keep the WSL short-circuit, and stop the docs saying debug

Review round on ROCm#447.

- Extracting the WSL condition into `wsl_wave_through` turned a
  short-circuit into two eager argument evaluations, so
  `has_usable_amd_gpu` ran on every Linux launch instead of on WSL only.
  The probe is passed unevaluated now, which restores the original
  behaviour and keeps the condition testable. The comment that still
  claimed `&&` short-circuited was describing code that no longer did.

- `docs/architecture.md` and `docs/testing.md` still said the discarded
  decline is logged at `debug`; it is `info!`, deliberately, because the
  default filter is `warn,rocm=info`. Both now say so and why.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* fix(amd-smi): stop device_readable_bounded waving through off Linux

The previous round made the off-Linux branch return `true`, on the
reasoning that it should mirror `kfd_readable`'s wave-through as its doc
claimed. That was the wrong half to fix. The two answer different
questions: `kfd_readable` gates this crate's own `amd-smi` launch, which
off Linux has no `/dev/kfd` to reason about; `device_readable_bounded` is
the shared probe `rocm-dash-collectors` calls for a pre-flight that is
expected to decline a missing device on every platform.

Windows CI caught it: `missing_kfd_and_no_reachability_fails_preflight`
and `real_kfd_keeps_a_veto_over_the_host_root_view` both went green while
losing the veto they exist to protect. The off-Linux branch is a direct
open again and the doc now says why it does not wave through.

That also removes the `const`-able body, so the `missing_const_for_fn`
split added for it is gone from both crates. Both tests verified passing
on Darwin, which compiles the same non-Linux branch Windows does.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

* docs(amd-smi): put the probe sentence in the right bullet, and stop two claims overreaching

Review round on ROCm#447.

- The `AMD_SMI_SERVE_PROBE_TIMEOUT` sentence had drifted into
  `architecture.md`'s `freeform_planner.rs` bullet across several merge
  resolutions. Both probes live in `gpu_policy.rs`; it sits there now,
  next to `detect_gpu_count`.

- `amd_smi.rs` said that losing the amd-smi count leaves `--gpu auto`
  with no selection. It does not: `effective_gpu_count` falls through to
  the DRM sysfs VRAM rows, which is what `docs/testing.md` already said.
  The code was right and this doc was wrong, so the doc now matches and
  names the function.

- `docs/testing.md` claimed unit tests cover the rewritten call sites.
  They cover what those sites do with a *missing count*
  (`effective_gpu_count_prefers_amd_smi_then_falls_back_to_vram_rows`);
  that they route through the chokepoint at all is guarded only by the
  xtask contract scan. Both halves are now stated separately.

- examine-31 was `@requires-os:linux`, but WSL2 reports that os_family
  and `kfd_readable` waves through there on a reachable GPU, so the
  decline it asserts never happens. Now `@requires-bare-metal` too.

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

---------

Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
Signed-off-by: Roman <roman.sirokov@amd.com>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
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