test: write fake executables without the ETXTBSY fork race by rominf · Pull Request #649 · ROCm/rocm-cli

rominf · 2026-10-13T09:53:40Z

Fixes an intermittent Text file busy (os error 26) that has failed the required Coverage (rocm-dash crates, ratcheted) job on unrelated PRs (#633, #511 and #598), in install::tests::install_vllm_rocm10_discover_realigns_the_trio_even_if_pip_freeze_fails and install::tests::install_vllm_rocm10_discover_realigns_torch_after_the_config_file_install.

Cause. Tests plant a fake python/uv/amd-smi with std::fs::write + set_permissions(0o755) and run it straight away. While the write fd is open, a test on another thread can spawn a child, which holds a copy of that fd until its own exec; O_CLOEXEC only closes it at that point. Running the fake inside that window fails with ETXTBSY. cargo test runs many tests on the threads of one process, so this shows up there, including in the coverage job and on the Windows lane's test runner.

Fix. This adds rocm_core::fake_executable::write_fake_executable, which has a short-lived child process write the file, so the test process never holds a write fd on it. Every site in the workspace that writes a fake and then runs it now uses it:

the vLLM and Lemonade install tests;
the TheRock and ComfyUI helpers;
rocm-core's examine and host_gpu tests;
the dash-daemon end-to-end tests;
the E2E step shims;
the xtask lychee and GPU-preflight stubs.

The module is public and not behind cfg(test), because other crates' tests (including the E2E crate) call it, and a cfg(test) module isn't visible across crates. It is a few lines that nothing outside tests calls. docs/testing.md documents the rule.

Evidence.

A stress run of the three vLLM discover-install tests on 16 threads failed 149 of 2,400 runs with ETXTBSY before this change, and 0 of 7,200 after it.
The new regression test, a_fake_runs_first_time_while_other_threads_spawn (640 cycles on 16 threads), fails 10 of 10 runs when the helper is reverted to an in-process write, and passes 20 of 20 with it.

Scope. Test-only. Production never runs a file it wrote from the same process (uv is extracted by a tar child), so there is no production retry. Nothing a user can observe changes, so there is no new scenario; the E2E step changes are exercised by the existing scenarios in CI.

Verified locally:

fmt
tests for rocm-engine-vllm (101), rocm-core --features e2e-test-hooks (622), rocm --bin rocm (1094), Lemonade (128), dash-daemon (62) and xtask (247)
both clippy invocations, on Linux and for x86_64-pc-windows-gnu

The E2E scenarios themselves were not run locally.

Tests that plant a fake python/uv/amd-smi shell script with std::fs::write and then run it fail intermittently on Linux with "Text file busy (os error 26)". While the write fd is open, a test on another thread can spawn a child, which holds a copy of that fd until its own exec (O_CLOEXEC only closes it then). If the fake is run inside that window the kernel refuses to execute it. This has failed the required coverage job on unrelated PRs, via the vLLM ROCm 10 discover install tests. Add rocm_core::fake_executable::write_fake_executable, which has a short-lived single-threaded child write the file so this process never holds a write fd on it, and use it at every site that writes a fake and then runs it: vLLM and Lemonade install tests, the TheRock and ComfyUI helpers, rocm-core's examine and host_gpu probes, the dash daemon end-to-end tests, the E2E step shims, and the xtask lychee and GPU preflight stubs. The helper's own regression test runs 640 write-then-run cycles across 16 threads; with an in-process write it fails every time. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>

juhovainio

Nice, narrowly-scoped fix and it actually closes the race at its root rather than papering over it: the write fd on the target path only ever lives in a short-lived external child process (sh -c 'exec cat > "$1"'), and its exit is awaited before the function returns, so no fork from the calling test process can ever inherit a write fd on that path. Not a sleep, not a retry/backoff band-aid. Path is passed as a positional arg rather than interpolated, so it's injection-safe, and the stress test (16 threads x 40 iterations) is a credible regression test for the exact bug this fixes. CI is green on this commit.

A couple of small non-blocking things, not asking for changes on these: there's a dead Stdio::null() config left over in fake_executable.rs since the in-script exec ... > "$1" redirect supersedes it, and two call sites (examine.rs, workflow_contract.rs) lost a bit of error-message specificity by collapsing the old write/chmod .expect() messages into one -- write_fake_executable's own internal error already names the path and failing step, so the practical loss is small.

One thing worth a thought before merge, though not blocking: write_fake_executable ships unconditionally into the production rocm-core library (not cfg(test)-gated, since that's not visible cross-crate) rather than behind a feature flag. This repo already has a working example of gating test-only cross-crate code this way (e2e-test-hooks in engines/lemonade, used by apps/rocm's hardware_root.rs) -- just not applied to rocm-core here. Your PR-body rationale for skipping that is reasonable given the plumbing cost across every consumer, so I'm not blocking on it, but wanted to flag it explicitly in case you'd rather match house style.

rominf · 2026-10-13T10:50:04Z

Thanks @juhovainio. On the notes:

The dead Stdio::null(): right, the in-script redirect makes it moot. I'll drop it in a small follow-up together with the next point.
Error specificity at the two call sites: the helper's own error names the path and which step failed (the writer's exit status and stderr, or the chmod). I'll restore a call-site context string at both sites in the same follow-up, so a failure also says which fake it was.
Feature-gating the helper: I considered it. Every consumer (vLLM, Lemonade, rocm, the dash-daemon tests, e2e-cucumber, xtask) would need the feature enabled on its rocm-core dev-dependency. For a few lines that only test code calls, I'd rather not, but I'll note the choice in the module doc so it's visible. Happy to gate it if maintainers prefer house style here.

Both new amd-smi tests wrote their stand-in binary with `fs::write` plus `set_permissions` and then executed it. Main's #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>

rominf · 2026-10-13T18:12:39Z

Follow-up for the non-blocking notes is #656:

Stdio::null(): the dead call is gone.
Feature gate: the module doc now says why the helper is public rather than gated behind a feature (every caller is in another crate, so the feature would have to be enabled in each consumer's dev-dependency).
Lost specificity: the examine.rs messages already named the fake, so what really went missing was which step failed. The helper's errors now name the step, and a test makes a real chmod fail. The two rocm-smi stubs in workflow_contract.rs also have different messages now.

The ComfyUI install-path e2e scenarios never ran on Windows — not because the CLI logic they test is Linux-specific (production already resolves .dll/.so and python.exe/uv.exe paths per host via runtime_is_windows() and friends), but because their test fixtures hardcoded POSIX shell shims. comfyui-01/02/03's fixtures in tests/e2e-cucumber/tests/e2e/comfyui_steps.rs now plant the uv/Python subprocesses they shim as real compiled executables (fake-uv, fake-python), declared as ordinary [[bin]] targets of this crate, the same convention remote_steps.rs's fake-tailscale stand-in already used. Their behavior (exit code, stdout line) is configured through env vars rather than generated per scenario. fake-uv panics on an unparseable FAKE_UV_EXIT_CODE instead of silently defaulting; fake-python rejects an argv shape matching neither of the runtime Python's two invocation forms (probe_torch_stack_versions's -c, probe_comfyui's script form) instead of guessing — both failing loudly, the same rule fake-tailscale already follows. Planting a fixture binary goes through rocm_core::fake_executable:: write_fake_executable rather than std::fs::copy, so it does not reopen ROCm#649's ETXTBSY race: a plain copy holds a write fd on the destination for the duration of the copy, which is exactly the window a concurrent scenario's spawn can inherit. That helper needs the rocm-core dev-dependency, which this crate's library target cannot reach, so install_fixture_binary() lives in tests/e2e.rs (the test binary that can) rather than alongside the pure fixture_binary()/fixture_target_name() lookups in src/fixture_binary.rs. A .bat script was tried first and rejected: Rust's CVE-2024-24576 mitigation rejects any argument containing a newline once the target resolves to a batch file, and the pre-install torch-stack probe passes its probe script as exactly such an argument — a real compiled executable has no such restriction. The planted rocm_sdk library stubs use a small fixture-local rocm_library_filename() helper instead of importing rocm_core::runtime_rocm_library_filename: readiness only checks path.is_file() on whatever the manifest lists, never the filename, so the import would buy nothing while breaking this suite's black-box-only rule (no imports from the product crates). The [[bin]]-locating, suffix-naming, and plant logic is shared between this and remote_steps.rs's fake-tailscale stand-in, which previously duplicated the same lookup-and-copy body privately. tests/e2e-cucumber/ README.md documents the [[bin]] fixture pattern for the next stand-in. @requires-os:linux is dropped from comfyui-01/comfyui-02/comfyui-03 in comfyui.feature, since their fixtures no longer need it. comfyui-04 keeps @requires-os:linux: it drives rocm under a real PTY, the same pre-existing limitation dash.feature's header comment already documents — unrelated to the fixture changes here, now cross-referenced instead of left unexplained. No production code changes — apps/rocm/src/comfyui.rs and crates/rocm-core already resolve platform-correct paths; only the test fixtures needed to catch up. Fixes ROCm#629 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Jussi Elo <jussi.elo@amd.com>

…mi 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 (#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>

The ComfyUI install-path e2e scenarios never ran on Windows — not because the CLI logic they test is Linux-specific (production already resolves .dll/.so and python.exe/uv.exe paths per host via runtime_is_windows() and friends), but because their test fixtures hardcoded POSIX shell shims. comfyui-01/02/03's fixtures in tests/e2e-cucumber/tests/e2e/comfyui_steps.rs now plant the uv/Python subprocesses they shim as real compiled executables (fake-uv, fake-python), declared as ordinary [[bin]] targets of this crate, the same convention remote_steps.rs's fake-tailscale stand-in already used. Their behavior (exit code, stdout line) is configured through env vars rather than generated per scenario. fake-uv panics on an unparseable FAKE_UV_EXIT_CODE instead of silently defaulting; fake-python rejects an argv shape matching neither of the runtime Python's two invocation forms (probe_torch_stack_versions's -c, probe_comfyui's script form) instead of guessing — both failing loudly, the same rule fake-tailscale already follows. plant_ready_runtime — the Given every comfyui scenario (01 through 13) builds its runtime from — now plants fake-python as that runtime's default Python, instead of the old #!/bin/sh exit 0 stand-in that failed the post-install GPU check. used_comfyui_install (comfyui-05..13's shared Given) used to immediately overwrite that planted Python with its own GPU-check shell shim; that overwrite is now dropped, since fake-python already answers the same post-install probe unconditionally. comfyui-11 still needs a real shell shim for its failing-then-passing GPU check transition, so that helper stays for that one case. Planting a fixture binary goes through rocm_core::fake_executable:: write_fake_executable rather than std::fs::copy, so it does not reopen ROCm#649's ETXTBSY race: a plain copy holds a write fd on the destination for the duration of the copy, which is exactly the window a concurrent scenario's spawn can inherit. That helper needs the rocm-core dev-dependency, which this crate's library target cannot reach, so install_fixture_binary() lives in tests/e2e.rs (the test binary that can) rather than alongside the pure fixture_binary()/fixture_target_name() lookups in src/fixture_binary.rs. A .bat script was tried first and rejected: Rust's CVE-2024-24576 mitigation rejects any argument containing a newline once the target resolves to a batch file, and the pre-install torch-stack probe passes its probe script as exactly such an argument — a real compiled executable has no such restriction. The planted rocm_sdk library stubs use a small fixture-local rocm_library_filename() helper instead of importing rocm_core::runtime_rocm_library_filename: readiness only checks path.is_file() on whatever the manifest lists, never the filename, so the import would buy nothing while breaking this suite's black-box-only rule (no imports from the product crates). The [[bin]]-locating, suffix-naming, and plant logic is shared between this and remote_steps.rs's fake-tailscale stand-in, which previously duplicated the same lookup-and-copy body privately. tests/e2e-cucumber/ README.md documents the [[bin]] fixture pattern for the next stand-in. @requires-os:linux is dropped from comfyui-01/comfyui-02/comfyui-03 in comfyui.feature, since their fixtures no longer need it. comfyui-04 keeps @requires-os:linux: it drives rocm under a real PTY, the same pre-existing limitation dash.feature's header comment already documents — unrelated to the fixture changes here, now cross-referenced instead of left unexplained. No production code changes — apps/rocm/src/comfyui.rs and crates/rocm-core already resolve platform-correct paths; only the test fixtures needed to catch up. Fixes ROCm#629 Signed-off-by: Jussi Elo <jussi.elo@amd.com>

The ComfyUI install-path e2e scenarios never ran on Windows — not because the CLI logic they test is Linux-specific (production already resolves .dll/.so and python.exe/uv.exe paths per host via runtime_is_windows() and friends), but because their test fixtures hardcoded POSIX shell shims. comfyui-01/02/03's fixtures in tests/e2e-cucumber/tests/e2e/comfyui_steps.rs now plant the uv/Python subprocesses they shim as real compiled executables (fake-uv, fake-python), declared as ordinary [[bin]] targets of this crate, the same convention remote_steps.rs's fake-tailscale stand-in already used. Their behavior (exit code, stdout line) is configured through env vars rather than generated per scenario. fake-uv panics on an unparseable FAKE_UV_EXIT_CODE instead of silently defaulting; fake-python rejects an argv shape matching neither of the runtime Python's two invocation forms (probe_torch_stack_versions's -c, probe_comfyui's script form) instead of guessing — both failing loudly, the same rule fake-tailscale already follows. plant_ready_runtime — the Given every comfyui scenario (01 through 13) builds its runtime from — now plants fake-python as that runtime's default Python, instead of the old #!/bin/sh exit 0 stand-in that failed the post-install GPU check. used_comfyui_install (comfyui-05..13's shared Given) used to immediately overwrite that planted Python with its own GPU-check shell shim; that overwrite is now dropped, since fake-python already answers the same post-install probe unconditionally. comfyui-11's GPU-check transition used a dedicated shell-shim helper for both halves (failing, then passing again); the passing-again half now plants fake-python too, like every other scenario, so only the failing half still needs a real shell shim, and the now-single-purpose helper is deleted. Planting a fixture binary goes through rocm_core::fake_executable:: write_fake_executable rather than std::fs::copy, so it does not reopen ROCm#649's ETXTBSY race: a plain copy holds a write fd on the destination for the duration of the copy, which is exactly the window a concurrent scenario's spawn can inherit. That helper needs the rocm-core dev-dependency, which this crate's library target cannot reach, so install_fixture_binary() lives in tests/e2e.rs (the test binary that can) rather than alongside the pure fixture_binary()/fixture_target_name() lookups in src/fixture_binary.rs. A .bat script was tried first and rejected: Rust's CVE-2024-24576 mitigation rejects any argument containing a newline once the target resolves to a batch file, and the pre-install torch-stack probe passes its probe script as exactly such an argument — a real compiled executable has no such restriction. The planted rocm_sdk library stubs use a small fixture-local rocm_library_filename() helper instead of importing rocm_core::runtime_rocm_library_filename: readiness only checks path.is_file() on whatever the manifest lists, never the filename, so the import would buy nothing, so it stays a local stand-in. The [[bin]]-locating, suffix-naming, and plant logic is shared between this and remote_steps.rs's fake-tailscale stand-in, which previously duplicated the same lookup-and-copy body privately. tests/e2e-cucumber/ README.md documents the [[bin]] fixture pattern for the next stand-in. @requires-os:linux is dropped from comfyui-01/comfyui-02/comfyui-03 in comfyui.feature, since their fixtures no longer need it. comfyui-04 keeps @requires-os:linux: it drives rocm under a real PTY, the same pre-existing limitation dash.feature's header comment already documents — unrelated to the fixture changes here, now cross-referenced instead of left unexplained. comfyui-05/-06/-07/-09/-10/-13 also lose their shell dependency as a side effect of the plant_ready_runtime change above, but stay @requires-os:linux too: their reinstall behavior has not been run on Windows yet, which is tracked separately in ROCm#697 to keep this change scoped to ROCm#629's comfyui-01/02/03 ask. No production code changes — apps/rocm/src/comfyui.rs and crates/rocm-core already resolve platform-correct paths; only the test fixtures needed to catch up. Part of ROCm#629 (remaining scope tracked in ROCm#697) Signed-off-by: Jussi Elo <jussi.elo@amd.com>

…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>

…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>

rominf requested a review from a team as a code owner October 13, 2026 09:53

rominf requested a review from volen-silo October 13, 2026 09:53

rominf mentioned this pull request Oct 13, 2026

test: render_update_json_installs_the_suppression_guard_around_resolution mutates process PATH, racing tar-spawning tests #650

Closed

juhovainio approved these changes Oct 13, 2026

View reviewed changes

rominf added this pull request to the merge queue Oct 13, 2026

Merged via the queue into main with commit 2314b2d Oct 13, 2026
30 checks passed

rominf deleted the test/vllm-fake-exec-etxtbsy branch October 13, 2026 11:11

This was referenced Oct 13, 2026

fix(e2e): make ComfyUI install/status fixtures run on Windows #637

Merged

test(fake-executable): name the failing step and the fake #656

Merged

r0x0r mentioned this pull request Oct 14, 2026

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

Merged

nowycondro mentioned this pull request Oct 27, 2026

fix(vllm): pin flash-attn and amd-aiter to what the discovered vLLM depends on #755

Merged

Navigation Menu

Repository navigation

test: write fake executables without the ETXTBSY fork race - #649

test: write fake executables without the ETXTBSY fork race#649
rominf merged 1 commit into
mainfrom
test/vllm-fake-exec-etxtbsy

rominf commented Oct 13, 2026

Uh oh!

juhovainio left a comment

Uh oh!

rominf commented Oct 13, 2026

Uh oh!

Uh oh!

rominf commented Oct 13, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

rominf commented Oct 13, 2026

Uh oh!

juhovainio left a comment

Choose a reason for hiding this comment

Uh oh!

rominf commented Oct 13, 2026

Uh oh!

Uh oh!

rominf commented Oct 13, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants