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>

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

Open

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

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

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

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants