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

[r0x0r]

Fixes ROCMAI-448.

Important

Whoever squash-merges this: untick the Co-authored-by box.
One commit on this branch (6533327) carries an AI Co-Authored-By: trailer. GitHub's squash-merge UI pre-checks co-author trailers by default, so it will be carried into the commit on main unless it is unticked in the merge dialog. The commit is published and reviewed, so it is deliberately not being rewritten — removing the trailer would need a force-push over a branch under review, which costs more than unticking a box. The squash subject/body are editable in the same dialog.

The defect

rocmd captured child output 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, smaller on Windows), so a child that writes more than that blocks in write, never exits, and the drain is never reached. The only exit was the timeout, which killed the child and reported process exceeded Ns timeout.

So the timeout measured how much the child printed, not how long it took, and successful commands were killed mid-flight.

Two reachable consumers:

• GPU telemetry. amd-smi static -a -g all --json runs under a 2s budget. On a multi-GPU host that JSON clears a buffer easily, so gather_gpu_snapshot permanently reported amd_smi_available: false — feeding rocmd bridge-snapshot, the gpu_snapshot / bridge_snapshot MCP tools, and the automation watcher. (Not examine_snapshot: that sandbox tool returns an ExamineSummary only and never reads this snapshot.) The note it left said failed to launch amd-smi static -a -g all --json, naming the launch rather than the timeout, because only the outermost context was rendered; it now uses {error:#} and carries the cause.
• Install. An MCP-driven install sdk prints far more than a buffer (download progress, uv, extraction), so it was killed partway, leaving a partially installed SDK.

Scope note: three copies, not two

The ticket named two. There were three byte-identical copies:

Location	Name
apps/rocmd/src/common.rs	run_command_with_timeout
apps/rocmd/src/sandbox.rs	run_process_with_timeout (linux-only, bubblewrap sandbox)
apps/rocm/src/main.rs	run_command_with_timeout

(Paths as of this head. The two rocmd copies were in apps/rocmd/src/lib.rs when this branch opened; main's #487 split the daemon into modules in the meantime.)

The third was unlisted and has the same defect and the same reachability. All three are replaced by one rocm_core::process::run_with_timeout, following the reader-thread pattern already in crates/rocm-core/src/examine.rs. Both apps already depended on rocm-core, so no new crate edge — check-crate-edges passes.

Second defect found in the same function

The timeout did not actually bound the call. Killing the child does not close the pipe if the child forked a grandchild holding the write end, so collecting output blocked until that exited. Measured: a 1-second timeout took 30 seconds against sh -c "sleep 30", because dash forks rather than execs.

This mattered for the same 2s telemetry probe the ticket is about, so it is fixed here: readers publish into a shared buffer and are abandoned after a short grace rather than joined.

Same pattern, left alone (follow-up)

Three older capture helpers already drain on threads, so none of them has the pipe-buffer deadlock this PR is about — but all three still join() their readers, so a grandchild holding the write end can still outrun their bound:

• crates/rocm-core/src/host_gpu.rs::capture_optional_command_candidate_with_timeout (on the GPU-probe path)
• crates/rocm-core/src/examine.rs::run_raw
• apps/rocm/src/therock.rs::wait_with_output_bounded
• crates/rocm-core/src/host_gpu.rs::capture_diagnostic_command (Windows-only — a fourth)

Not converted here: each has its own callers and timeout semantics, and moving them is a change in behaviour for paths this ticket does not reach. Tracked in #623, which also carries the related unbounded stderr quote in capture_amd_smi_json's non-zero-exit bail! (that one predates this PR; #623 does not cover the daemon's parse_sandbox_child_output, which is unbounded in the same way and, as far as this PR can tell, tracked nowhere). docs/architecture.md names them too, so no reader takes run_with_timeout for the only such helper.

Deliberate: a reader still unfinished after a normal exit returns what it has

Once the child exits the readers get DRAIN_GRACE (200ms) and whatever has been collected comes back as a success. The alternative — failing with "output incomplete, the pipe is still held" — was considered and not taken: a child that legitimately leaves a background helper holding the pipe (sandboxed installers do) would start failing outright, which is a wider behaviour change than this fix. A read error is a different matter and is now reported (below).

Tests

Both defects were reproduced before fixing, and every test below fails against the implementation it guards.

Unit (cargo test -p rocm-core --lib process::):

• a_child_that_floods_both_pipes_does_not_deadlock — floods both pipes past one buffer. Both matter: draining stdout fully and only then stderr deadlocks just as readily.
• a_genuinely_slow_command_times_out_on_time — the call must return at its deadline, not when an abandoned grandchild exits.
• a_grandchild_holding_the_pipe_does_not_hold_a_successful_call — the same hazard with no timeout to blame: the child exits 0 leaving sleep 5 & on the pipe.
• output_arriving_after_the_child_exits_is_still_collected — what DRAIN_GRACE buys, and the only way to ask for it on demand: output that reaches the pipe after the child is gone.
• an_interrupted_read_is_retried_and_a_failed_one_is_reported — feeds the reader a scripted stream, because neither branch is reachable through a real child.
• a_child_that_reads_stdin_is_given_an_empty_one — the other half of the stdio contract: the helper sets the child's stdin itself, so a child that reads gets EOF at once rather than waiting for input nobody will send. The caller-set stdin it has to override is the read end of a pipe the test holds open for the whole call, so the assertion is two-sided rather than a tautology — without the setting the child sits in read until the deadline kills it. Added in review; see below.

Every grandchild case runs under each shell on the host (/bin/sh, /bin/bash, /bin/dash) and backgrounds explicitly, because whether a shell forks or execs decides whether a grandchild exists at all — sh -c "sleep 30" leaves one under dash and none under bash.

Scenario examine-24 (@id:examine-telemetry-survives-large-amd-smi-output) drives rocmd bridge-snapshot against a stub amd-smi emitting ~203KiB and asserts amd_smi_available: true, both probes carrying every GPU, and a null note. It needs no GPU and runs on the mock lane every PR: the stub is reached through resolve_amd_smi_binary's bare-PATH fallback, with HOME pointed at the isolated root for that one invocation. @requires-os:linux only because the stub is a shell script.

The timeout error's quote is bounded

{error:#} renders the whole chain, and 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. That text is persisted: record_gpu_metrics copies the snapshot note 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 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, because the quote is assembled there — the timeout message is the helper's own text and a caller sees it only as an opaque error, so that is the only place that can still tell which of its characters came from the child — and all four callers (both common.rs captures, the daemon sandbox, apps/rocm) inherit it. It bounds that message and no other: a caller quoting the Output handed back to it (capture_amd_smi_json's non-zero-exit bail!, the daemon's parse_sandbox_child_output) is quoting raw child output of its own, unbounded and persisted the same way. Both predate this PR and are outside its scope; of the two, #623 tracks only the capture_amd_smi_json quote. Truncation reuses examine.rs::truncate_to_chars (now pub(crate)) rather than copying it — head, char boundaries, visible ...[truncated] marker. (host_gpu.rs::truncate_diagnostic_line is a different function with a different marker, plain ...; an earlier revision of this description claimed it matched, which it does not.)

Scenario examine-25 (@id:examine-telemetry-note-is-bounded-when-amd-smi-stalls) pins the note end to end against a stub that prints ~203KiB and then stalls: amd_smi_available: false, and a note that names the probe, names the timeout, says it was shortened, stays under 4000 characters, and still quotes the head of what the probe printed. That last one is the load-bearing pair with the marker: truncate_to_chars(output, 0) is exactly "...[truncated]", so a zero quote bound satisfies both the marker and the size while keeping no output at all. The quote is pinned on a market name unique to GPU 0, so it means head rather than merely non-empty — were the value repeated across all 1200 GPUs, a quote that kept the tail would contain a copy too. That is also the test the changed note text was missing.

Scenario examine-26 (@id:examine-telemetry-note-names-the-stalled-monitor-probe) covers the other branch. The snapshot asks amd-smi twice — static, then monitor — and reports the first failure it meets, so examine-25's stalled static returns before monitor ever runs. Here the stub answers the first probe and stalls on the second, which is the branch a real multi-GPU host hits more often because monitor -g all carries per-GPU telemetry for every device. The result is a half-degraded snapshot: amd_smi_available: true with the static GPUs intact, and the note as the sole record of why the second probe is missing. It goes to the same automation and audit logs on the same 60s cycle, so it needs the same bound and the same account of the cause — naming the launch alone would send a reader hunting for a missing binary.

Windows

The hazard tests were #[cfg(unix)]-only, so Windows ran only the trivial success case (and the scripted-reader case). process.rs/Capture have no cfg(windows) branches, so this was a fixture gap rather than unverified logic, but Windows is supported and both common.rs callers run there. Added, driven by a batch-file fixture (cmd has no yes, and a script file keeps cmd's quote-stripping rules from becoming the thing under test):

• a both-pipes flood, the deadlock case;
• flood-then-ping -n 30 127.0.0.1 with a 2s timeout — covers the timeout, the new bound, and the grandchild in one, since killing cmd does not kill ping, which keeps the inherited write ends open.
• a child that reads stdin, set /p being cmd's read (added in review, below). The fixture helper was split in two so a script that needs no payload no longer writes a 200KB one.

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 — the one whose separate reader this function exists to provide — unchecked there.

Mutation results

Each mutant was confirmed to differ from the baseline file before the run.

Mutant	Before	After
Take the output as soon as the child exits (no drain wait)	passes 3/3	fails — output_arriving_after_the_child_exits_is_still_collected
DRAIN_GRACE effectively unbounded, under dash	fails at 30s	fails at 30s
DRAIN_GRACE effectively unbounded, under bash	passes in 1.00s	fails — 30.0s and 5.0s, two tests
Join the readers unconditionally (the original shape)	—	fails — 30.0s and 5.0s, under dash and bash
A read error is EOF (Ok(0) | Err(_) => break)	passes	fails — an_interrupted_read_is_retried_and_a_failed_one_is_reported
The monitor probe fails (examine-24)	passes	fails — monitor_snapshot empty, note set
Quote bound removed (MAX_QUOTED_OUTPUT_CHARS = usize::MAX)	passes	fails — a_timed_out_child_has_its_output_quoted_within_a_bound, and examine-25
truncate_to_chars made a no-op (if false)	passes	fails — same test, on the missing marker
Truncation keeps the marker but drops the output	passes	fails — same test on the missing flood-canary; examine-25 and examine-26 on the missing GPU-0 market name
truncate_to_chars keeps the tail (.skip(len - max)) instead of the head	passes	fails — examine-2[56], 2 scenarios / 2 failed; the quoted note is GPUs 1188–1199
{error:#} reverted to .to_string() (common.rs)	passes	fails — examine-25, on "the note must name the timeout rather than blaming the launch"
.stdin(Stdio::null()) deleted from run_with_timeout	passes 19/19 — the gap this round found	fails — a_child_that_reads_stdin_is_given_an_empty_one, 19 passed / 1 failed, on process exceeded 5s timeout: no output
.stdin(Stdio::null()) → .stdin(Stdio::inherit())	passes	passes — the one mutant that case cannot catch: telling null from inherit needs the test process's own fd 0 to be something other than EOF. Written down in the test's doc rather than left to be rediscovered.
Only the monitor branch's {error:#} reverted to .to_string()	passes	fails — examine-26 alone (2 scenarios, 1 passed / 1 failed); examine-25 passes, which is why that branch's comment now names 26

What review rounds added on top of the above

Not in the original description, because it was found in review rather than written up front:

• The wait loop's try_wait() error path never killed or reaped the child. wait_bounded returned the poll error straight out, and the caller then dropped the Child — leaving a process running that nobody was waiting on. It now does a best-effort let _ = child.kill(); before propagating, matching the timeout branch, which is the only other way out of that loop. The Waitable trait exists so that both failure paths are reachable from a test at all: a real child cannot be made to fail try_wait without the kernel refusing to reap it. a_failure_to_poll_the_child_still_abandons_both_readers and a_failure_to_reap_a_timed_out_child_still_abandons_both_readers assert both the abandon and, via an Arc<AtomicBool> on the fake child, the kill.

• The amd-smi stub writer reintroduced an ETXTBSY race. 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 write fd still open in a sibling thread's child turns the stub's first exec into an intermittent Text file busy. write_amd_smi_stub now goes through rocm_core::fake_executable::write_fake_executable, as every other *_steps.rs in that directory already does.

• The null-stdin promise had no test. This PR moved .stdin(Stdio::null()) out of the three copies it replaced and into run_with_timeout (which has four call sites), whose doc now tells callers not to set it — but deleting that line left all 19 process:: tests passing. Without it a child that reads stdin inherits the daemon's or the CLI's, waits for input that never comes, and is killed at the deadline. Covered now on both platforms (sh -c 'read line || echo eof', set /p), each against a caller-set stdin held open for the whole call, so the child reaches EOF only if the helper really does replace it.

• Comment accuracy (three rounds, no behaviour change). The module doc described only the spawn/terminate primitives and FileLock while run_with_timeout had become the bulk of the file; DRAIN_GRACE's doc claimed the readers "run throughout", when wait_bounded's first try_wait() has no sleep in front of it and a fast enough child can be seen exited before either reader thread is scheduled (the 200ms grace covers that too — it was the comment that overclaimed, not the code); and the abandoned guard in the reader's read-error branch is kept, with its comment corrected to the window it actually carries — the gap inside take() between emptying the buffer under one lock and draining the failure slot under another, where a read error would otherwise come back attached to bytes already handed to the caller and be rendered as a hard "failed to read the output of ...". A third round corrected four more: the stdin test helper said "all three callers" where run_with_timeout has four call sites; MAX_QUOTED_OUTPUT_CHARS attributed the daemon's parse_sandbox_child_output to Converge the remaining unbounded child-capture helpers onto run_with_timeout #623, which does not list it; the {error:#} note quoted Linux's spawn-failure text for a probe rocmd also runs on Windows, equated "no GPU" with "no amd-smi" (resolve_amd_smi_binary falls back to a bare amd-smi on PATH, so such a host fails at a non-zero exit instead), and let "Pinned by examine-25" trail a claim examine-25 does not assert; and WindowsFixture's struct doc still described a payload that only flooding writes after the script/flooding split.

  Three more in the round after that. The {error:#} aside said only the timeout gains its causes; the spawn failure does too, which on a host with no amd-smi — every no-GPU host — is the version of this note most readers will ever see (...: failed to spawn process: No such file or directory (os error 2), confirmed against the built rocmd). The monitor branch inherited "Pinned by examine-25" from the branch above it, when a mutation of that branch is caught by examine-26 alone. And the Capture doc called an abandoned reader "a leak we can bound" when what is bounded is the memory, not the number of parked threads — one per abandoned capture, until the grandchild lets go. That trade is deliberate (a parked reader holds a stack and no CPU; bounding it too means closing the read end, which is the SIGPIPE behaviour change ruled out above), so the doc now says so instead of implying both are bounded.

Verification

Run on the Linux devbox (this product does not compile on macOS). Latest run is against head a55b9641:

• cargo test --workspace --all-targets --no-fail-fast — exit 0, 3855 passed / 0 failed / 16 ignored (33 test binaries)
• cargo clippy --workspace --all-targets -- -D warnings — exit 0, no diagnostics
• cargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings — exit 0 (run separately: the e2e harness is harness = false, so --all-targets skips it)
• cargo fmt --all -- --check — clean
• cargo xtask check-crate-edges — pass
• cargo test -p e2e-cucumber --test feature_naming — pass
• cargo xtask e2e -- -n "examine-2[456]" — 3 scenarios, all pass (and each fails against its mutants above)

Gap: no amd-smi is installed on that host, so the fix was not confirmed against a real multi-GPU amd-smi — only against the stub, which reproduces the size condition that causes the bug. The unit tests cover the mechanism directly.

[rominf]

🔴 Automated review · pr-review-watcher · 20cda44

This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.

Summary

The PR adds one shared rocm_core::process::run_with_timeout to replace three copies of a wait-then-drain loop. The old loop deadlocked when a child printed more than one pipe buffer, so output size could trigger the timeout. The new function also bounds the call when a leftover grandchild process keeps the pipe open, and scenario examine-18 covers the pipe-buffer case. Outcome: No blocking findings. Reviewed: the whole change (prw-base...HEAD, both commits, all 6 files), plus sibling capture loops in examine.rs and therock.rs, resolve_amd_smi_binary and its home/data-dir lookups, and the e2e isolation helpers. Verified: cargo test -p rocm-core --lib process:: passed 3/3. cargo clippy -p rocm-core -p rocmd -p rocm --all-targets -- -D warnings was clean, and rocm and rocmd built. The e2e scenario was not run, because the review host's disk was full. The three unit tests and examine-18 would fail if the fix were reverted: the old loop never reads the 2MB flood or the ~203KiB stub output, and the timeout test would take 30s. The HOME explanation in the comments matches the code: resolve_amd_smi_binary goes through default_data_dir(), which ignores ROCM_CLI_DATA_DIR. Default telemetry mode is local, so the scenario does reach the probe. Blocking: 0 · Non-blocking: 9.

Since the last round (95683d2): the previous blocker said examine-18 was not isolated from the host's real $HOME. This round re-reviewed examine.feature, examine_steps.rs, resolve_amd_smi_binary and isolate_env in full, and did not find it again. 20cda444 sets HOME to the isolated root for the rocmd bridge-snapshot invocation, so both lookups that go through $HOME now land in an empty tree before the stub on PATH is reached. Both comments now describe that mechanism. This also answers the reply above. The automation's change request is withdrawn for that reason.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• crates/rocm-core/src/process.rs:58 — Ok(0) | Err(_) => break treats ErrorKind::Interrupted as EOF. The read_to_end used by the replaced code retries on EINTR. rocmd installs signal handlers, so a stray EINTR would quietly cut the captured output short. Add an Interrupted => continue arm.
• crates/rocm-core/src/process.rs:140-146 — Abandoned reader threads keep running in the long-lived rocmd daemon. Each keeps its thread, its pipe file descriptor and an Arc<Mutex<Vec<u8>>> that nobody reads, and that buffer grows for as long as the leftover grandchild writes. Consider a size cap, or stop appending after take().
• crates/rocm-core/src/examine.rs:523-528 — The commit message says the new function follows examine.rs's reader pattern. But run_raw still joins both readers unconditionally after killing a timed-out child. That is the leftover-grandchild hang this PR fixes, and the "timeout bounds the call" fix was not carried over to it.
• apps/rocm/src/therock.rs:3503-3508 — wait_with_output_bounded is a fourth copy of the same pattern. On its success path it joins both readers with no grace period, so a child that exits while a grandchild holds the pipe hangs forever. It could move to run_with_timeout (it needs a variant that writes stdin first).
• tests/e2e-cucumber/tests/e2e/examine_steps.rs:1140-1158 — The Then step checks amd_smi_available and the length of static_snapshot, but not note or monitor_snapshot. If the second (monitor) probe times out, amd_smi_available stays true and the scenario still passes. Also assert gpu["note"].is_null().
• tests/e2e-cucumber/tests/e2e/examine_steps.rs:1112-1118 — The step sets HOME to the isolated root itself. The established isolated-home convention is root.join("home") (pty_env, tests/e2e-cucumber/tests/e2e.rs:342-354). Use that so HOME does not sit next to the config/, data/ and cache/ directories.
• tests/e2e-cucumber/tests/e2e/examine_steps.rs:1089-1094 — This duplicates rocmd_binary() from artifact_steps.rs:45-59, without its canonicalize. Make that helper pub(crate) and reuse it.
• tests/e2e-cucumber/features/examine.feature:236 and examine_steps.rs:14 — Both comments say the stub emits "~150KiB". STUB_GPU_COUNT decides the actual size, and the payload it produces is ~203KiB, so the comments should change to match.
• crates/rocm-core/src/lib.rs:8005 (pre-existing, for follow-up) — resolve_amd_smi_binary uses default_data_dir() rather than the configured AppPaths data dir, so it ignores ROCM_CLI_DATA_DIR. The scenario works around this product inconsistency by overriding HOME instead of fixing it.

[rominf]

🔴 Automated review · pr-review-watcher · 95683d2

Requesting changes for one defect (details in the review comment on this PR):

The new scenario examine-18 is not hermetic, although its comments in tests/e2e-cucumber/features/examine.feature and tests/e2e-cucumber/tests/e2e/examine_steps.rs say it is. resolve_amd_smi_binary resolves through default_data_dir()/runtime_home_dir(), which read the real $HOME, not ROCM_CLI_DATA_DIR. isolate_env() also does not set HOME. So on any Linux host with a managed SDK registry under ~/.rocm or a ~/.rocm/bin/amd-smi, the real amd-smi runs instead of the stub. The scenario then fails (Some(0) != Some(1200)), with a message that points at the pipe-drain defect rather than the cause.

To resolve: isolate HOME for that rocmd invocation, or make resolve_amd_smi_binary honour the isolated data dir. Then correct both comments to describe the mechanism that actually holds.

[r0x0r]

Confirmed, and fixed in 20cda444. I traced both halves of the claim before acting on it, and both hold:

• resolve_amd_smi_binary (crates/rocm-core/src/lib.rs:8004) tries default_data_dir()/runtimes/registry first, then resolve_amd_smi_binary_in_home(runtime_home_dir()). default_data_dir() is home_rocm_dir(), which is runtime_home_dir().join(".rocm"), and runtime_home_dir() ends at BaseDirs::new().home_dir() on Linux. So both lookups resolve through the real $HOME; ROCM_CLI_DATA_DIR is never consulted on this path. (env_path_override exists in that same module but is not used here.)
• isolate_env (tests/e2e-cucumber/tests/e2e.rs:270) sets ROCM_CLI_CONFIG_DIR, ROCM_CLI_DATA_DIR, ROCM_CLI_CACHE_DIR, HF_HOME, PIP_CACHE_DIR, UV_CACHE_DIR and ROCM_PATH — and no HOME. isolate_cmd only replays that list, so the step inherited the real one.

So the scenario was not hermetic, and the step's comment was the worse of the two: it asserted outright that "HOME is isolated too", which was never true.

Fix: point HOME at the isolated root for that single rocmd bridge-snapshot invocation. I deliberately did not take either of the broader routes:

• Not added to isolate_env, because that file documents other scenarios as deliberately inheriting the real HOME/XDG environment for their PTY/runtime defaults — moving it there changes behaviour well outside this scenario.
• Not by making resolve_amd_smi_binary honour the isolated data dir, since that alters production resolution order to suit a test.

Both comments are rewritten to describe the mechanism that actually holds — the empty <data>/runtimes registry is not sufficient on its own, and the feature-file note now says so explicitly, including what goes wrong without the HOME override (a host with a managed SDK under ~/.rocm runs the real amd-smi, and the size assertion then blames the pipe-drain defect for an environment problem).

Verified on the Linux devbox at 20cda444: cargo clippy --workspace --all-targets -- -D warnings clean, cargo clippy -p e2e-cucumber --test e2e -- -D warnings clean (the e2e target is test = false, so --all-targets does not reach it), cargo fmt --all -- --check clean. The only test failure in the run was restarting_a_keyless_public_service_names_the_public_bind_not_the_key_flag, a pre-existing flake in that family which I have reproduced on unmodified main (2 of 3 runs) and which passes in isolation.

Good catch — thank you. This is the kind of defect that would have surfaced as a confusing failure on someone else's machine rather than here.

Withdrawn by the automation: at 20cda44 examine-18 points HOME at the isolated root for the rocmd invocation, so resolve_amd_smi_binary no longer reaches the host's real ~/.rocm, and both comments now describe that mechanism. Details in the updated review comment.

[rominf]

@r0x0r thanks. I re-reviewed 20cda444 against the HOME-isolation finding. The HOME override in examine-18 does stop resolve_amd_smi_binary from reaching the host's ~/.rocm, and both comments now describe that mechanism, so the blocker is resolved and the automation's change request is withdrawn. The updated report is in the review comment above: #526 (comment). It has no blocking findings and 9 non-blocking ones; a few carry over from the last round, e.g. the EINTR arm in process.rs.

[jussielo-amd]

Nice diagnosis and a genuinely good fix — the write-up tracing the deadlock through bounded OS pipe buffers, and then catching the second, sneakier bug (killing the child doesn't close the pipe when a grandchild inherited it) is exactly the kind of thing that's easy to miss and easy to verify once you see it. The new rocm_core::process::run_with_timeout unit tests are well targeted (flooding both pipes, the dash-forks-sleep timeout case) and I confirmed locally they fail against the old shape and pass against this one. Clean workspace build and clippy too.

One blocking issue: this doesn't actually land the fix for two of the three call sites in the PR's current state. common.rs (added to main by #479 after this branch forked) still has the old run_command_with_timeout — the same drain-after-exit loop this PR removes — and capture_amd_smi_json / run_rocm_capture_for_paths in that file still call it instead of rocm_core::process::run_with_timeout. That's the GPU-telemetry probe and the rocm examine/install/update capture path, i.e. the two consumers named as the whole motivation for the fix. It looks like the merge from main picked up common.rs as extracted on main (pre-fix) without re-applying this PR's change to the relocated functions. CI is catching this already — the new examine-20 scenario fails on this PR's head with failed to launch amd-smi ..., reproducing the original symptom. Only the bubblewrap-sandbox path (lib.rs) and the separate rocm binary (main.rs) are actually fixed right now. Worth a rebase/sync pass over common.rs before merging.

Non-blocking: the two adversarial regression tests (both-pipes-flood, slow-command-times-out) are #[cfg(unix)] only, so Windows CI won't exercise the exact mechanism being fixed, even though the PR notes OS pipe buffers are smaller there. And examine.rs::run_raw has the same unconditional-join hazard this PR fixes elsewhere (noted and deliberately left alone in the description) — fine to leave for a follow-up given the ticket's scope, just flagging it stays open.

[r0x0r]

Thank you — and your blocking issue is exactly right. You reviewed f8f7d88f; it is fixed in 6176d72c, which was pushed shortly after your review landed.

Your diagnosis of the cause is also exactly what happened. #479 extracted common.rs from apps/rocmd/src/lib.rs after this branch forked, and when I merged main the conflict hunks for the relocated code presented as "ours-only" — which reads as keep ours — while the truth was that the code had moved. I took main's side to avoid re-declaring gather_gpu_snapshot / CommandCapture, and that silently reverted this PR's change to the two functions that travelled with it.

What 6176d72c restores, at both call sites you named:

capture_amd_smi_json      -> rocm_core::process::run_with_timeout(command, AMD_SMI_PROBE_TIMEOUT, "process")
run_rocm_capture_for_paths -> rocm_core::process::run_with_timeout(command, timeout, "process")

and the relocated pub(crate) fn run_command_with_timeout — the drain-after-exit loop this PR exists to delete — is removed from common.rs rather than left orphaned. grep -rn run_command_with_timeout apps/rocmd/ now returns nothing.

You were right that CI was already catching it, and that is the part I am glad of: the scenario you saw failing with failed to launch amd-smi ... is this PR's own regression test reproducing the original symptom against my bad merge. It caught my mistake before a human had to, which is the argument for having added it.

Gates on the corrected head: cargo clippy --workspace --all-targets -- -D warnings clean, cargo clippy -p e2e-cucumber --test e2e -- -D warnings clean (the e2e target is test = false, so --all-targets misses it), cargo fmt --all -- --check clean, and the full E2E lane green — 21 checks, no failures.

On your two non-blocking points, both fair and both still open:

• The adversarial tests are #[cfg(unix)]. You are right that this is backwards relative to the risk: Windows pipe buffers are smaller, so the deadlock is easier to hit there, and that is precisely the platform the regression tests do not cover. They are Unix-gated because they flood both pipes from a shell one-liner and fork a sleep grandchild to hold the pipe open past the kill — neither of which ports directly. A Windows equivalent wants a small helper binary (or a PowerShell shape) rather than a cfg flip, so I would rather do it as its own change than bolt it on here. Happy to file it.
• examine.rs::run_raw — agreed it stays open; it is named in the description deliberately rather than silently, and it is outside EAI-8457's scope. It is the fourth instance of this shape, after the two this PR replaces and the rocmd one, so it is worth a ticket rather than a note.

Say the word if you would rather either of those land here instead of as follow-ups.

[rominf]

🔴 Automated review · pr-review-watcher · 6176d72

This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.

Review — needs work

Full review of the whole change.
Implements ROCMAI-448 in full: both consumers the ticket names (the amd-smi telemetry probe and the rocm subprocess capture behind MCP install) now go through the new helper. The description's "Fixes ROCMAI-448" holds.

Since the last round: the earlier automated rounds reviewed 95683d24 and 20cda444. The branch has since merged main and that history no longer resolves, so this round re-reviewed the whole change at 6176d72c rather than a delta. Each finding is marked [new] or [standing]. A standing finding was also raised in the earlier non-blocking list at 20cda444.

Blocking

• [new] No test pins the new DRAIN_GRACE logic reliably — crates/rocm-core/src/process.rs:139-142, crates/rocm-core/src/process.rs:193-200
  The change adds two things after the wait loop: a bounded drain, and a step that abandons readers a grandchild is keeping open. Mutation runs (cargo test -p rocm-core --lib process::) show neither is covered:

  • The drain itself is untested. Mutant E, which takes the output immediately after the child exits with no drain wait, passes all three tests on three runs. No test checks that output still in the pipe at exit gets collected. No test covers a child that exits successfully while a grandchild holds the pipe, e.g. sh -c 'echo hi; sleep 5 &'.
  • The bounded-wait test only works where /bin/sh is dash. a_genuinely_slow_command_times_out_on_time relies on sh forking sleep, and its comment says so. That holds on this Ubuntu host: /bin/sh → dash, and sh -c 'sleep 5' leaves a child sleep. But bash -c 'sleep 5' execs sleep directly, so there is no grandchild. With the test command switched to bash, mutant B (wait for the readers with no limit) passes in 1.01s. On a supported Linux host where sh is bash, the test cannot fail on the branch it is named for.

  Per-test verdicts:

  • a_command_that_succeeds_reports_its_output: still passes with the change reverted; smoke test only.
  • a_genuinely_slow_command_times_out_on_time: fails on revert (took 30.0s). Catches mutant B only under dash.
  • a_child_that_floods_both_pipes_does_not_deadlock: fails on revert. Catches a stdout-then-stderr sequential drain (mutant F). Blind to mutant E.

  Confidence 100 · logic · Fix: use sh -c "sleep 30; :" or "sleep 30 & wait" so a grandchild exists under any shell. Add one test where the child exits 0 after writing, plus one where it exits 0 while leaving a background grandchild that holds the pipe. Assert the output is complete and the call returns promptly.

Non-blocking

• [new] When the grace expires, truncated output is returned as success — crates/rocm-core/src/process.rs:139-145, process.rs:53-62
  If the child exits while a grandchild still holds a pipe, the function returns Ok(Output) 200ms later with whatever the readers had collected. Nothing tells the caller that EOF was never reached. run_rocm_capture_for_paths (main.rs, rocmd common.rs) would then present cut-short stdout as a completed command. The abandoned reader thread and its buffer also live as long as the grandchild does, inside the long-running rocmd. No current caller is known to trigger this: the serve and supervisor spawns redirect their stdio to log files (main.rs:7335, 18805; rocmd lib.rs:4785).
  Confidence 70 · logic · Fix: record whether both readers finished. Either fail, or attach a note when the grace expires on a non-timeout path, so the bound on runtime does not silently become a bound on the output.
• [standing] Four other copies of the pattern still join their readers with no limit — apps/rocm/src/therock.rs:3514, crates/rocm-core/src/examine.rs:~616-635, crates/rocm-core/src/lib.rs:~3620, crates/rocm-core/src/lib.rs:~4556
  Each starts its readers before the wait, so none deadlocks on output size. But each joins them without a bound after kill or exit, which is the grandchild hang this PR fixes in its own helper. The description discloses only examine.rs::run_raw. These copies were already there before this PR, and the PR's new helper makes their pattern a bug.
  Confidence 85 · architectural · Fix: route them through rocm_core::process::run_with_timeout, or list all four in the description as a known follow-up.
• [new] "Spurious timeout note" does not match what the defect produces — tests/e2e-cucumber/features/examine.feature:282-284, apps/rocmd/src/common.rs:118
  Reverting only the rocmd capture call and running examine-20 failed as expected. The note was "failed to launch amd-smi static -a -g all --json", with no mention of a timeout, because gather_gpu_snapshot renders the error with to_string(), which keeps only the outermost context. The feature comment, the commit message and the PR description ("the ticket's exact symptom … plus the spurious note") all describe a timeout note. The observed run governs. The same rendering means a genuine timeout after this fix is still reported as "failed to launch". That part predates the PR, but it is the line this PR edits.
  Confidence 100 · mechanical · Fix: correct the comment and description, and consider format!("{error:#}") so the cause survives.
• [standing] The scenario's Then step discards rocmd's stderr and ignores the monitor probe — tests/e2e-cucumber/tests/e2e/examine_steps.rs:1389-1393, :1396-1418
output.stderr is dropped and the rc assertion prints only stdout, so a rocmd failure loses its error text (world.cli_stderr exists for this). The monitor probe can fail while amd_smi_available stays true (common.rs:73-83), and nothing asserts gpu["note"] is null.
  Confidence 85 · mechanical · Fix: record stderr, and assert note is null.
• [standing] The step re-implements the ROCM_CLI_ROCMD_BINARY lookup — tests/e2e-cucumber/tests/e2e/examine_steps.rs:1352-1356
  It duplicates rocmd_binary() in artifact_steps.rs:45 and drops that helper's canonicalize.
  Confidence 85 · mechanical · Fix: share and reuse the existing helper.
• [new] The scenario's wording names mechanisms, not behaviour — tests/e2e-cucumber/features/examine.feature:303-306
  "outruns the pipe buffer" (title and Given) and "gathers a bridge snapshot" (When) describe implementation. The behaviour is that GPU telemetry stays available when amd-smi describes many GPUs.
  Confidence 70 · mechanical · Fix: e.g. "Given amd-smi describes many GPUs / Then GPU telemetry is reported as available and lists every GPU".
• [new] The per-crate coverage gate never ran on this head — branch tip 6176d72c
  The head is 7 commits behind prw-base and predates #482: coverage-floors.toml is absent, and only the older "Coverage (rocm-dash crates, ratcheted)" job reported. This change moves about 125 lines out of rocm and rocmd and adds about 95 non-test lines to rocm-core (floors 77.13 / 75.36 / 82.14), and no floor was checked against it. The E2E tests (MI300X) check had no conclusion when read. Every other check is green.
  Confidence 85 · mechanical · Fix: merge or rebase current main and let cargo xtask coverage run.

Decisions for the author

• Bounded runtime vs. complete output when a grandchild holds the pipe — tradeoff
  The old code waited for EOF indefinitely, which is complete but unbounded. The new code returns at the grace, which is bounded but possibly truncated. Both have a cost. Confirm that bounding wins for every caller, including the install path, where truncated stdout is reported upstream (see the non-blocking finding).

Positive signals

• Three copies of the deadlock-prone loop were collapsed into one rocm_core::process::run_with_timeout. No new crate edge, and the error labels ("process", "sandbox process") are kept.
• examine-20 is a real regression test. With only the rocmd capture call reverted it fails with amd_smi_available: false, and it passes at HEAD. It needs no GPU, so it runs on every PR.
• The flood test deliberately floods both pipes, which also kills the sequential stdout-then-stderr drain variant (mutant F).
• The HOME override is scoped to the one step and its necessity is traced correctly through runtime_home_dir().

Deployment notes

None

What this covered

Every file in the change was read completely: crates/rocm-core/src/process.rs and lib.rs, the changed call sites in apps/rocm/src/main.rs, apps/rocmd/src/common.rs and apps/rocmd/src/lib.rs with their callers, examine.feature and examine_steps.rs. Also read: the amd-smi resolution path, the e2e isolation helpers, and every other spawn-with-timeout site in apps/, crates/, engines/. Diff prw-base (f9a8011)…6176d72c.

What ran:

• cargo test -p rocm-core --lib process:: (3/3) and five mutants of process.rs.
• clippy for rocmd and rocm-core.
• examine-20 at HEAD (pass), and with the rocmd capture reverted (fail).
• feature_naming (4/4), leak scan (clean), sign-off check (clean).

Did not run:

• Clippy on apps/rocm and on the e2e target, because free disk fell to about 42G. Unused imports there were checked by grep only.
• A full-diff revert of examine-20, which did not apply cleanly while another worker was mutating the same tree. A narrower revert of the rocmd call was used instead.
• The prior-changes pass, because earlier reviews are out of scope for this watcher.
• Design questions (Step 7) were judged in the driving context rather than by separate workers.

Checks that had not reported on this head when the round started: E2E tests (MI300X) (queued, no conclusion), Skill checks (skillscope) (skipped), Sphinx docs build (-W) (skipped).

Not reconciled against any discussion here.

[rominf]

🔴 Automated review · pr-review-watcher · 099ab1b

This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.

Review — no blocking findings

Full review of the whole change.
Closes ROCMAI-448 in full: both consumers the ticket names (the MCP-driven rocm capture and the amd-smi probe) and the second shape it mentions now go through the new helper. The description has one stale table (see Non-blocking).

Round status: still blocked, by one standing objection. This round's blind review of 099ab1b6 found no new blocking findings, and the title above is its verdict. The objection from the round at 6176d72c is carried below as [standing]. crates/rocm-core/src/process.rs is unchanged since that round; the only new commit is a merge of main. This round did not re-find the objection blind. It was then put to the reviewer, which re-ran the mutations and found that both parts still hold. Each finding is marked [new] or [standing].

Blocking

• [standing] No test pins the new DRAIN_GRACE logic reliably — crates/rocm-core/src/process.rs:139-163
  Settled this round by putting the objection to the reviewer. That check is anchored, because the reviewer was handed the objection; it was not re-found blind. Both parts still hold at 099ab1b6:

  • The bounded drain is untested. A mutant that takes the output as soon as the child exits, with no drain wait (drain loop at process.rs:141 disabled), passes all three process:: tests on 3 runs out of 3. The success test passes anyway, because the 50ms try_wait poll gives the reader time to finish first. Two cases have no test: output still in the pipe when the child exits, and a child that exits 0 while a background grandchild holds the pipe (e.g. sh -c 'echo hi; sleep 5 &').
  • The bounded-wait test only works where /bin/sh is dash. a_genuinely_slow_command_times_out_on_time needs sh to fork sleep. With DRAIN_GRACE set to 3600s (an effectively unbounded reader wait) and the test's command switched to bash -c "sleep 30", the test passed in 1.00s. Under dash, the same mutant fails at 30s. On a supported Linux host where sh is bash, the test cannot fail on the branch it is named for.

  Confidence 100 · logic · Fix: give the timeout test a grandchild that exists under any shell, e.g. sh -c "sleep 30; :" or "sleep 30 & wait". Add one test where the child exits 0 right after writing, and one where it exits 0 while a background grandchild holds the pipe. Assert that the output is complete and the call returns promptly. To resolve this, each new test should fail against both mutants above, under dash and under bash.

Non-blocking

• [standing] A pipe read error now silently truncates output and still returns Ok — crates/rocm-core/src/process.rs:57 (Ok(0) | Err(_) => break)
  The old code used wait_with_output, which goes through read_to_end. That retries ErrorKind::Interrupted and returns any other read error as Err. The new reader treats every error as EOF, marks the stream finished, and the caller gets a short Ok(Output). EINTR is unlikely to be reachable today, because rocmd's only signal handlers come from tokio/signal-hook and use SA_RESTART. Still, this drops a guarantee the replaced code had, and the repo already retries Interrupted explicitly elsewhere (crates/rocm-core/src/lib.rs:1155-1162).
  Confidence 75 · logic · Fix: continue on Interrupted. Record any other error in the Capture and return it from run_with_timeout instead of dropping it.
• [standing] examine-20 never checks the second amd-smi probe, despite its title "GPU telemetry survives" — tests/e2e-cucumber/tests/e2e/examine_steps.rs (Then step), apps/rocmd/src/common.rs:72-83
  If the monitor probe fails, gather_gpu_snapshot still returns amd_smi_available: true with static_snapshot set. It just adds a note and leaves monitor_snapshot empty. The Then step only asserts amd_smi_available and the length of static_snapshot, so a timeout or short read on the monitor probe still passes.
  Confidence 85 · mechanical · Fix: also assert gpu["note"] is null and monitor_snapshot is present.
• [new] The architecture doc still puts command-timeout plumbing in common.rs — docs/architecture.md:34
  It says common.rs holds "CommandCapture/command-timeout plumbing". The timeout half now lives in rocm_core::process. AGENTS.md §5 asks for docs to be kept in sync in the same change. The code governs, so the doc should change, not the code.
  Confidence 85 · mechanical · Fix: reword the clause, and optionally add process.rs to the rocm-core sentence at line 38.
• [new] The PR description's "three copies" table gives pre-rebase locations — PR description, "Scope note" table
  It lists all three rocmd copies under apps/rocmd/src/lib.rs. On this head the removed functions were in apps/rocmd/src/common.rs (run_command_with_timeout) and apps/rocmd/src/sandbox.rs (run_process_with_timeout), which is what the diff touches.
  Confidence 85 · mechanical · Fix: update the two paths in the table.
• [standing] The feature comment and description call the symptom a "timeout note", but the note never says timeout — tests/e2e-cucumber/features/examine.feature:497-504
  With the fix reverted, the scenario's observed note was "failed to launch amd-smi static -a -g all --json". That happens because common.rs:67 and :81 keep only error.to_string(), the outermost context, and drop the "exceeded 2s timeout" cause. The misleading note predates this PR. The new comment describes it inaccurately, and a timeout is reported as a launch failure.
  Confidence 100 (observed in a run) · mechanical · Fix: correct the comment, or switch the note to format!("{error:#}") so it includes the cause.
• [standing] Older copies of the same capture helper still join their readers unconditionally (pre-existing, same pattern) — crates/rocm-core/src/host_gpu.rs:226-256, crates/rocm-core/src/examine.rs:~617-633, apps/rocm/src/therock.rs:3540-3545
  These copies already drain on threads, so they don't have the pipe-buffer deadlock. They still join() their readers, which is exactly the "grandchild holds the write end" overrun this PR fixes in the new helper and documents at length. The host_gpu.rs copy sits on the GPU-probe path. The description discloses only examine.rs::run_raw.
  Confidence 85 · architectural · Fix: follow-up to move these onto rocm_core::process::run_with_timeout, or at least list them in the description.

Decisions for the author

• [standing] After a normal exit, unfinished readers return truncated output as Ok — tradeoff
  Once the child exits, the readers get DRAIN_GRACE (200ms) and whatever has been collected is returned as a success (process.rs:140-163).
  • In favour: it bounds the call even when a grandchild holds the pipe, where the old code could hang with no limit.
  • Against: a starved reader under CPU throttling, or a grandchild still writing, gives a silently short result. The JSON callers then fail with a misleading parse error. The run_rocm_capture* callers hand partial stdout straight to MCP tool results.
  • Returning an error such as "output incomplete: the pipe is still held" when a reader is unfinished would cost nothing on the normal path.
• [new] Reader threads leak while a grandchild holds the pipe — non-blocking-improvement
  Abandoned readers stay blocked until the holder exits, which matters in the long-running rocmd. This is better than the old behaviour, which blocked the caller. The doc comment says "abandoned" but not that the thread lives on. Killing the whole process group on timeout would remove both this leak and the orphan. Tree-kill helpers already exist in crates/rocm-core/src/lib.rs.

Positive signals

• The flood test fills both pipes on purpose, so a sequential stdout-then-stderr drain fails too. A mutation doing exactly that hung the test.
• a_genuinely_slow_command_times_out_on_time uses dash's fork-not-exec to pin the grandchild-holds-the-pipe case. Under dash it is a real regression guard: an unbounded-grace mutation made it take 30s and fail.
• In examine-20, HOME is pointed at the isolated root only for that one invocation, with the reason documented. Other scenarios keep their real-HOME PTY defaults.

Deployment notes

None

What this covered

• Read: every changed file completely, against efcd4800…099ab1b6 (prw-base...HEAD). Also read: the converted call sites in full, the twin capture helpers in host_gpu.rs, examine.rs and therock.rs, gather_gpu_snapshot, and docs/architecture.md.
• Fan-out: three read-only workers covered agent-instruction adherence plus history plus code comments, impact and twins, and design questions.
• Runs:
  • cargo test -p rocm-core --lib process:: passed (3/3).
  • cargo xtask e2e -- -n examine-20 passed at head.
  • examine-20 failed with the fix reverted (readers started after the wait loop). Note at that point: amd_smi_available: false.
• Verdicts on the added tests:

Test	Fails with the change reverted?	Branch mutation tried	Result
a_command_that_succeeds_reports_its_output	No — it is a sanity test, not a regression test	—	—
Flood test	Yes	Sequential drain	Hung
Timeout test	Yes	Unbounded DRAIN_GRACE	Failed at 30s
examine-20	Yes	Revert-shaped mutation	Failed

No test exercises the read-error branch or the truncation-after-grace path on a normal exit.

• Not run: prior-changes, because no earlier review of these files was visible here.
• CI on 099ab1b: all completed checks are green. E2E tests (MI300X), E2E tests (Strix Halo, Windows) and E2E tests (Strix Halo, WSL2) were still queued, so their result is unknown.
• Step 9: the PR description and ROCMAI-448 (title, body, status) were read after the findings were settled.
• No prompt-injection content was found.
• The PR discussion was not reconciled here.

[r0x0r]

Addressed at 6519c317. Every claim below was reproduced on the Linux devbox before being fixed, and each mutant was diffed against the baseline file before its run so a mutant that failed to apply could not be mistaken for one that survived.

Mutation results

#	Mutant / check	Before	After
1a	Take the output as soon as the child exits (drain loop at process.rs:141 removed)	passes 3/3	fails — output_arriving_after_the_child_exits_is_still_collected, under dash and bash
1b	DRAIN_GRACE → 3600s, test command sh -c "sleep 30", /bin/sh = dash	fails at 30.05s	fails at 30.05s
1b	DRAIN_GRACE → 3600s, test command bash -c "sleep 30"	passes in 1.00s	fails — 30.05s (timeout test) and 5.01s (grandchild test), with shells restricted to /bin/bash only
—	Join the readers unconditionally (the shape this PR replaced)	—	fails — same two tests, under dash and bash
2	Ok(0) | Err(_) => break (a read error is EOF)	passes	fails — an_interrupted_read_is_retried_and_a_failed_one_is_reported
3	The monitor probe fails (gather_gpu_snapshot returns the degraded shape)	examine-20 passes	examine-20 fails — monitor_snapshot must carry every GPU amd-smi described ... note="mutant: the monitor probe was killed at its timeout"

Your 1b reproduced exactly as described: 1.00s under bash. The fix is in two parts — every grandchild case now backgrounds explicitly (sleep 30 & wait, sleep 5 &) so a grandchild exists whether the shell forks or execs, and each case runs under every shell present on the host (/bin/sh, /bin/bash, /bin/dash), so this cannot silently stop testing anything on a host where sh is bash.

What changed

1 — the drain grace. Three unit tests now stand where there were none:

• output_arriving_after_the_child_exits_is_still_collected — a grandchild writes 0.1s after the child is gone. This is the only way I found to pin the grace deterministically: a child that writes and exits normally has its output drained by the already-running reader microseconds later, long before the wait loop's next 50ms poll notices the exit, so "take at exit" still collects everything and the test would be decorative. 0.1s sits between the two bounds that decide the outcome — past POLL_INTERVAL (50ms), so taking the output at the exit misses it; well inside DRAIN_GRACE (200ms), so draining collects it. Both margins are ~2x and both are stated in the test.
• a_grandchild_holding_the_pipe_does_not_hold_a_successful_call — echo ready; sleep 5 &, the case you named: the child exits 0, the grandchild holds the pipe. Asserts the output is complete and the call returns in under 3s.
• a_genuinely_slow_command_times_out_on_time — unchanged in intent, now shell-independent.

2 — read errors. Interrupted is retried; any other error is recorded in the Capture and returned from run_with_timeout rather than passed off as a short Ok. Covered by feeding the reader a scripted stream, since neither branch is reachable through a real child. One gap I am naming rather than hiding: the three lines in run_with_timeout that surface the recorded error are not independently mutation-killed — testing them needs a real pipe that fails mid-read.

3 — examine-20. Now asserts both probes carry STUB_GPU_COUNT GPUs and that gpu["note"] is null, and keeps rocmd's stderr so a failure quotes it. Verified against the mutant above.

4 — docs/architecture.md. Reworded the common.rs clause, and added process.rs to the rocm-core sentence with a line on what it owns.

5 — PR description. The table now gives this head's paths (common.rs, sandbox.rs), with a note that they were in lib.rs before #487.

6 — the misleading note. Took the second option: gather_gpu_snapshot renders the note with {error:#} at both probes, so a timeout now reads failed to launch amd-smi ...: process exceeded 2s timeout: ... instead of blaming the launch. The feature comment now quotes what the note actually said and explains why.

Left as follow-up, deliberately

• The other unconditional joins (host_gpu.rs:226-256, examine.rs::run_raw, therock.rs:3540-3545) are now listed in the description. Not converted here: each has its own callers and timeout semantics, and moving them changes behaviour on paths this ticket does not reach.
• Truncated output returned as Ok after a normal exit — kept, and the reasoning is now in the description. Failing with "output incomplete, the pipe is still held" would make a child that legitimately leaves a background helper on the pipe (sandboxed installers do) start failing outright. A read error is a different matter and is now an error.
• Reader threads outliving the call — unchanged. Killing the process group would fix both this and the orphan, but it is a change in what run_with_timeout does to its subject, and belongs with the follow-up above rather than in a fix for the drain order.

Verification (Linux devbox)

cargo test -p rocm-core --lib process:: 6/6 · cargo test --workspace --all-targets 0 failures · cargo clippy --workspace --all-targets -- -D warnings clean · cargo fmt --all -- --check clean · cargo xtask check-crate-edges pass · cargo test -p e2e-cucumber --test feature_naming pass · cargo xtask e2e -- -n examine-20 pass.

(One workspace-test run showed restarting_a_keyless_public_service_names_the_public_bind_not_the_key_flag failing; the devbox volume had hit 100% mid-run and the next build died with No space left on device. It passes with space free, and nothing in this change touches that path.)

[jussielo-amd]

Reviewed the diff against main (03afcf43...ed31e8e5). No blocking findings — the core fix (concurrent pipe draining + bounded grandchild handling) is solid, well-tested (including multi-shell grandchild reproduction and mutation testing), and all three duplicate copies of the old helper are confirmed removed.

A few non-blocking items and one tradeoff worth a quick look:

Non-blocking

• tests/e2e-cucumber/tests/e2e/examine_steps.rs duplicates artifact_steps::rocmd_binary()'s ROCM_CLI_ROCMD_BINARY resolution/panic logic instead of reusing it (that helper is currently private, so reuse isn't possible as-is). Worth promoting it to pub(crate) and reusing.
• No test drives run_with_timeout itself through its own read-failure path (process.rs:192-195) — only Capture's mechanics are tested directly.
• The deadlock-avoidance tests (flood, grandchild-holds-pipe, late-output) are #[cfg(unix)]-only; process.rs/Capture have no cfg(windows) branches so this is a test-fixture gap rather than unverified platform logic, but worth a PowerShell/cmd equivalent at some point.
• The improved failure note (format!("{error:#}") vs .to_string()) — the actual fix for the misleading "failed to launch amd-smi ..." message — has no direct test asserting its content, though it's derived straight from the real error so it can't drift from the decision.

Worth a decision

• Abandoned reader threads (process.rs's DRAIN_GRACE path) leak one OS thread for the life of rocmd whenever a grandchild never releases the pipe. Correctly avoids the original deadlock, but the writeup addresses the related "return partial output" tradeoff without mentioning this thread-leak consequence specifically — worth confirming it's an accepted risk, or considering a process-group kill (setsid+killpg on Unix, Job Object on Windows) to actually reap the grandchild.
• apps/rocm/src/therock.rs::wait_with_output_bounded still has the unbounded-join() version of this exact bug (confirmed independently) — already called out in the PR description as deliberately left for follow-up, so just flagging to make sure a tracking issue gets filed for it, host_gpu.rs, and examine.rs::run_raw.

Note: as of this review, three self-hosted GPU E2E lanes (MI300X, Strix Halo Windows, Strix Halo WSL2) were still queued rather than completed — worth confirming those land green before merge.

[rominf]

🔴 Automated review · pr-review-watcher · ed31e8e

This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.

Review — needs work

Full review of the whole change.
Implements all of ROCMAI-448. The description's claims (three copies replaced, the grandchild fix, the deliberate grace behaviour, sibling helpers left for a follow-up) match the code.

Round status: still blocked, but by two new findings; the standing objection is withdrawn. The objection carried from the rounds at 6176d72c and 099ab1b6 (no test pins the DRAIN_GRACE logic reliably) is settled, not re-found. This round's blind review read process.rs in full and raised nothing about it. The objection was then put to the reviewer, an anchored check because it was handed the objection. It re-ran both mutants in the checkout, once with the test shells limited to bash and once to dash, and confirmed all 6 process:: tests ran each time:

• Unbounded grace (3600s): a_genuinely_slow_command_times_out_on_time and a_grandchild_holding_the_pipe_does_not_hold_a_successful_call fail under both shells.
• No drain wait (grace 0): output_arriving_after_the_child_exits_is_still_collected fails under both shells.

grandchild_shells() asserts that its shell list is non-empty, so the tests cannot silently run nothing. The new blocking findings below come from this push's {error:#} change (a fix for a finding from the earlier round) and its missing test. Every finding below is [new].

Blocking

• [new] The {error:#} note now carries the child's whole captured output, with no limit, into the bridge snapshot and the automation logs — apps/rocmd/src/common.rs:73,92, crates/rocm-core/src/process.rs:173-186, persisted via apps/rocmd/src/watchers.rs:136-138,146,490-497
run_with_timeout's timeout error quotes all of trimmed stderr, or all of stdout if stderr is empty. Before this change the note rendered only the outermost context, so none of that reached it. Now it does.

  • Reproduced against this head's release rocmd. The stub amd-smi was cat <151KB payload>; sleep 5. rocmd bridge-snapshot returned amd_smi_available: false with a 151,369-character note that held the whole JSON payload.
  • The mutated examine-20 run showed it too: the assertion message was 318KB.
  • In the daemon that note goes into the gpu-metrics event twice, as reason= and inside the summary's note=. record_event then writes the event to both the automation-events and audit JSONL files every 60s.
  • Both append functions read and rewrite the whole file on each append (crates/rocm-core/src/lib.rs:2889-2940). A host whose amd-smi prints and then stalls past 2s therefore grows the logs by hundreds of KB per minute, and each write costs more than the last.

  Confidence 100 · logic · Fix: bound the output quoted in the timeout message inside run_with_timeout, for example the last few lines or KB with a "(truncated)" marker. That fixes every caller. Add a test that a timed-out flood leaves a bounded note that still names the timeout.

• [new] The changed note text has no test asserting what it now claims — apps/rocmd/src/common.rs:64-74,84-94
  The change makes the note "carry the causes". The only check that touches it is examine-20, which asserts note == null on success. Nothing checks that a timeout now surfaces as "…exceeded 2s timeout" rather than only "failed to launch". AGENTS.md §3 requires a message about the CLI's own behaviour to be asserted together with the state it describes.
  Confidence 85 · logic · Fix: add a unit or e2e check that a slow stub amd-smi produces amd_smi_available: false and a note naming the timeout. The size bound above can be checked in the same test.

Non-blocking

• [new] New comments misstate the code — tests/e2e-cucumber/features/examine.feature:289, apps/rocmd/src/common.rs:64-69, tests/e2e-cucumber/tests/e2e/examine_steps.rs (the HOME note in daemon_gathers_bridge_snapshot)

  • The feature comment names the wrong MCP tool. It says the snapshot feeds "the examine_snapshot and bridge_snapshot tools". examine_snapshot returns only ExamineSummary (apps/rocmd/src/sandbox.rs:191-198). The tool that actually serves this snapshot is gpu_snapshot (apps/rocmd/src/mcp.rs:596-598), and the comment omits it. The wording came from the ticket, and the PR description repeats it.
  • The common.rs comment overstates the JSON case. It says the outer context "names the wrong thing when what actually happened was a timeout or a JSON parse failure". A parse failure already carries its own outer context, failed to parse amd-smi … json (common.rs:144-145), so only the timeout case is right.
  • The step comment points at the wrong helper. It cites "isolate_env's note on PTY defaults". That note is on pty_env (tests/e2e-cucumber/tests/e2e.rs:~367-369).

  Confidence 85 · mechanical · Fix: correct the comments, which are wrong here because the code is the authority, and fix the description to match.

• [new] The architecture doc says this is "the one bounded child-process execution helper the apps share" — docs/architecture.md (rocm-core paragraph)
  Three similar helpers still exist and still join their readers with no time limit:

  • apps/rocm/src/therock.rs:3514
  • crates/rocm-core/src/examine.rs:581
  • crates/rocm-core/src/host_gpu.rs:200

  The Windows-only host_gpu.rs:2735 is a fourth. The description acknowledges these as follow-up work. The doc sentence should say the same, so a reader does not take it as the only such helper.
  Confidence 80 · mechanical · Fix: name the remaining helpers in the doc, or link the follow-up issue.

• [new] Windows runs none of the new helper's hazard tests — crates/rocm-core/src/process.rs:249-467
  The flood, timeout, grandchild and late-output tests are all cfg(unix). Only the trivial success test runs on Windows, and it skips the stderr check there, although echo oops 1>&2 works under cmd. Windows is a supported platform, and both common.rs callers run there.
  Confidence 70 · logic · Fix: add Windows variants. cmd /C ping -n 30 127.0.0.1 with a 1s timeout covers both the timeout and the grandchild case. One flooding command covers the deadlock.

Decisions for the author

• Returning a successful result with whatever arrived when the drain grace expires — answered-by-intent
  The description: "a child that legitimately leaves a background helper holding the pipe (sandboxed installers do) would start failing outright, which is a wider behaviour change than this fix." Otherwise this would have been non-blocking: text-parsing callers such as sandbox_check_updates_value and the MCP command results would get shortened output with exit 0 and no signal. Confirm that is still acceptable.
• Abandoned reader threads in the long-running daemon — tradeoff
  Each call whose child leaves a grandchild on the pipe leaks a reader thread until that grandchild exits. Against that, the old code blocked the daemon loop for the same period. No daemonizing path was found that inherits the pipe: rocm serve --managed redirects to its log file or to null, and bwrap uses --die-with-parent. The leak is theoretical today.

Positive signals

• examine-20 is a real regression test. I put back the original poll-then-wait_with_output shape and the scenario failed on amd_smi_available.
• The grandchild tests cover both directions of the grace. Setting DRAIN_GRACE to 60s failed two tests, and setting it to 0 failed two others. Running the grandchild cases under each shell on the host keeps them from going dead under bash.
• examine-20 points HOME at the isolated root for that one invocation instead of widening isolate_env. That keeps the stub reachable without changing other scenarios.

Deployment notes

None

What this covered

I read every changed file in full: process.rs, the three call sites, lib.rs, docs/architecture.md, examine.feature and examine_steps.rs. I also read the callers of the changed functions, the note's path through watchers.rs/persistence.rs/lib.rs, and the sibling capture helpers. Reviewed range: 03afcf43…ed31e8e5.

What I ran:

• The process:: unit tests: 6 run, all passed.
• cargo xtask e2e -- -n examine-20: 1 scenario, passed.
• The mutations described above.
• A manual rocmd bridge-snapshot against a print-then-stall stub.

CI for ed31e8e had no failures. Three GPU lanes were still queued: Strix Halo WSL2, Strix Halo Windows, and MI300X. The agent-instruction, history and code-comment passes and the design questions ran as independent workers. I read the description and ROCMAI-448, not their comments. Not reconciled against the PR discussion here.

[r0x0r]

Pushed 65333278. Addressing both of @rominf's blocking findings and all three non-blocking ones, and answering @jussielo-amd's points.

@jussielo-amd — thank you for the approval and for independently confirming the therock.rs sibling. Your asks are below.

One correction to how this round was framed to me: the review marked two findings Blocking (the unbounded {error:#} note, and its missing test). The MCP-tool/comment errors, the architecture-doc sentence and the Windows coverage are in the Non-blocking list. All five are handled anyway.

1. The {error:#} note carries the child's whole output — reproduced, fixed

Confirmed exactly as described. run_with_timeout bailed with all of trimmed stderr, or all of stdout — no truncation anywhere — and {error:#} renders that into note, which record_gpu_metrics writes into two JSONL files every 60s via helpers that rewrite the whole file.

Bounded inside run_with_timeout, not at the note. Reasoning: 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, apps/rocm — inherit it rather than each needing its own. Truncation follows the two in-tree precedents (examine.rs::truncate_to_chars, host_gpu.rs::truncate_diagnostic_line): head, char boundaries, visible ...[truncated]. Bound is 2000 characters — enough for a real stderr backtrace.

One thing the fix deliberately does not cover, flagged because it is the same hazard on the same persisted path: capture_amd_smi_json's own non-zero-exit bail! also quotes all of stderr. It is its own outermost context, so .to_string() already carried it unbounded before this PR — pre-existing, not introduced by {error:#}. Filed with the follow-up (below) rather than widened into this PR.

2. The note text had no test — added

New e2e scenario examine-21 (@id:examine-telemetry-note-is-bounded-when-amd-smi-stalls): a stub amd-smi prints ~150KiB and then stalls past the 2s budget. Asserts amd_smi_available: false and a note that names the probe, names the timeout, says it was shortened, and stays under 4000 characters — the size bound checked on the same note, as suggested. Reverting {error:#} to .to_string() fails it.

3. Comments misstate the code — all three reproduced, all three fixed

• examine.feature:289 — confirmed. examine_snapshot is the sandbox tool and returns an ExamineSummary only (sandbox.rs:191-198); the tool serving this snapshot is gpu_snapshot (mcp.rs:596). Corrected, and the PR description with it.
• common.rs:64-69 — confirmed. The parse failure carries its own outer context (common.rs:144-145), and so does the non-zero-exit bail!; only the timeout was misnamed. Comment now says so.
• examine_steps.rs HOME note — confirmed. The note is on pty_env (e2e.rs:~367), not isolate_env. Corrected.

4. The architecture doc's "the one bounded helper" claim — reproduced, reworded

All three named helpers still join() their readers with no time limit, plus the Windows-only fourth. The doc now names all four and says converging them is follow-up work.

5. Windows runs none of the hazard tests — reproduced (with one nuance), fixed

Reproduced. Nuance: an_interrupted_read_is_retried_and_a_failed_one_is_reported also runs on Windows (it drives a scripted reader, no cfg), so it was two tests there, not one — the hazard tests were indeed all cfg(unix).

@jussielo-amd judged this a fixture gap rather than unverified logic, and that read is right — process.rs/Capture have no cfg(windows) branches. I took the cross-platform-test option anyway, since Windows is supported and both common.rs callers run there. Driven by a batch-file fixture (cmd has no yes, and a script file keeps cmd's quote-stripping rules from becoming the thing under test):

• a both-pipes flood — the deadlock case;
• flood-then-ping -n 30 127.0.0.1 at a 2s timeout — covers the timeout, the new bound and the grandchild in one, since killing cmd does not kill ping, which keeps the inherited write ends open. This is your suggested fixture.

The success case now asserts stderr on Windows too (1>&2 is a cmd redirection like any other). These are the only changes here I cannot verify locally — windows-build-and-test runs cargo test --workspace --all-targets, so that lane is the check.

Mutation results

Every mutant was diffed against the baseline file and confirmed to differ before being run.

Mutant	Before	After
MAX_QUOTED_OUTPUT_CHARS = usize::MAX (no bound)	n/a — test is new	fails — a_timed_out_child_has_its_output_quoted_within_a_bound, on the missing marker; and examine-21
truncate_to_chars made a no-op (if false)	n/a	fails — same test, same assertion
Truncation keeps the marker, drops the output	n/a	fails — same test, on the missing flood-canary
{error:#} → .to_string() (common.rs, both sites)	n/a	fails — examine-21: "the note must name the timeout rather than blaming the launch"
(baseline, unmutated)	—	examine-20 + examine-21 both pass; process:: 7/7 pass

Note the last row: examine-20 still passes after the stub helper was refactored to serve both scenarios.

Gate (Linux devbox)

• cargo clippy --workspace --all-targets -- -D warnings — exit 0
• cargo test --workspace --all-targets — exit 0, 3588 passed / 0 failed / 8 ignored
• cargo fmt --all --check + prek --all-files — clean
• cargo xtask e2e -- -n "examine-2[01]" — 2 scenarios, 2 passed

Decisions you asked about

• Partial output with exit 0 when the grace expires — still accepted, unchanged. Narrowing it would break children that legitimately leave a background helper on the pipe.
• Abandoned reader threads in the daemon — accepted; your "theoretical today" analysis matches mine, and the old code blocked the whole loop for the same period. Not worth a process-group kill in this PR; noted on the follow-up issue as the place to revisit it if a daemonizing path that inherits the pipe ever appears.
• Tracking issue for the remaining unbounded join() copies — filed: Converge the remaining unbounded child-capture helpers onto run_with_timeout #623. Covers therock.rs::wait_with_output_bounded, examine.rs::run_raw, both host_gpu.rs helpers, and the capture_amd_smi_json stderr quote from item 1.
• rocmd_binary() reuse in examine_steps.rs — deferred, not done. Promoting artifact_steps::rocmd_binary() to pub(crate) touches a shared helper for a cosmetic win, and I did not want to widen a PR that already had your approval. Happy to do it here if you would rather it land now.
• No test drives run_with_timeout's own read-failure path — still true, still deferred for the same reason you gave: it needs the kernel to fail a pipe read, and Capture's mechanics are tested directly.
• The three queued GPU lanes — still to be confirmed green on this head before merge.

[rominf]

🔴 Automated review · pr-review-watcher · 6533327

This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.

Review — needs work

Full review of the whole change.
Implements and closes ROCMAI-448. All three copies of the drain-after-exit helper are replaced, which covers both of the ticket's consumers (the amd-smi telemetry probe and the MCP run_rocm_capture used for install sdk) and the sandbox copy the ticket calls "the second shape". The description's claims match the diff.

Round status: one standing objection dropped, one narrowed and still standing. Both blockers from the round at ed31e8e5 were standing, not re-found: this round's blind review read every changed file and raised neither. Each was then put to the reviewer, one at a time, together with the reply's account of the fix. That check is anchored evidence, because the reviewer was told what to look for.

• Dropped: the unbounded {error:#} note. run_with_timeout now bounds the quoted output at MAX_QUOTED_OUTPUT_CHARS (2000 chars, ...[truncated] marker), and every caller goes through it (crates/rocm-core/src/process.rs timeout bail!). The non-zero-exit bail! in capture_amd_smi_json still quotes all of stderr, but it was already its own outermost context before {error:#}, so it is not what this objection was about. It is tracked in #623.
• Narrowed, still standing: the untested note text. examine-21 now pins the static probe's note together with amd_smi_available: false. The monitor probe's note got the same {error:#} change and is still unpinned. See the [standing] item under Blocking.
• The [new] blocking item below (the read-error result of run_with_timeout) is published as the review found it. The same gap was noted as non-blocking coverage in the round at 099ab1b6, and nothing here shows the code is wrong. On its own it would not keep this change request standing. The [standing] item is what does.

Verification this round was static only (reading code and CI results), because of low disk space on the review host. Nothing was compiled or run.

Blocking

• [standing, narrowed] The monitor probe's note text has no test — apps/rocmd/src/common.rs:84-94
  From the round at ed31e8e5: "The changed note text has no test asserting what it now claims … Nothing checks that a timeout now surfaces as '…exceeded 2s timeout' rather than only 'failed to launch'." examine-21 now covers the static site. Its stub stalls on every call, and gather_gpu_snapshot returns at the first probe that fails, so the scenario never reaches the monitor site. That site's {error:#} could go back to .to_string() and every check would stay green. examine-20 only asserts note == null on success. AGENTS.md §3 asks for each such message to be asserted together with the state it describes.
  Settled by putting the objection to the reviewer, which is anchored evidence, and confirmed by reading examine.feature and common.rs at this head · logic · Fix: a stub that answers static but stalls on monitor, asserting amd_smi_available: true, a present static_snapshot, and a note naming the timeout.
• [new] Nothing tests whether run_with_timeout returns the new read-error result or which error wins — crates/rocm-core/src/process.rs:230-236
  The change adds this behavior: a reader's I/O error now turns into Err("failed to read the output of {label}"), and only after the timeout check, so a timeout wins over a read error. No test reaches either half. an_interrupted_read_is_retried_and_a_failed_one_is_reported (:410-467) drives Capture::draining directly with a scripted pipe and never calls run_with_timeout.
  • Mutation 1: delete the if let Some(error) = stdout_failure.or(stderr_failure) block. The read error is dropped and Ok comes back with truncated bytes, which is the silent short read the Capture comment at :97-102 says this code exists to prevent. Every test stays green.
  • Mutation 2: move that block above if timed_out. Every test stays green.
  • The description's mutation row "A read error is EOF" mutates only the reader loop, so it does not cover this.
  • The test comment's case for impracticality (EINTR or a failing pipe cannot be produced from a real child) is about reaching this through a child process. It does not apply here: the precedence and propagation are plain logic over values already in hand.
    Confidence 85 · logic · Fix: factor the post-wait assembly (status, timed_out, each stream's (bytes, Option<io::Error>), label, timeout → Result<Output>) into a function. Unit-test that a read failure without a timeout returns the "failed to read the output of" error, and that a read failure together with a timeout reports the timeout.

Non-blocking

• [new] An abandoned reader keeps adding to a buffer nobody reads, with no limit — crates/rocm-core/src/process.rs:91-112, :204-205
  After DRAIN_GRACE, take() empties the buffer, but the reader thread still holds its own Arc clone. It keeps calling extend_from_slice until EOF. While a grandchild keeps writing to the inherited pipe, each such call in the long-running rocmd leaks up to two threads plus a growing Vec. The GPU watcher probes once per GPU_METRICS_INTERVAL_MS (60s, apps/rocmd/src/watchers.rs:23,66). Before this change the same situation hung the caller instead, so overall this is an improvement, but it is a new failure shape that nothing documents.
  Confidence 70 · architectural · Fix: set a "taken" flag in take() and stop appending once it is set (or cap the buffer), and mention the leak in Capture's docs. Killing the process group on Unix is a possible follow-up.
• [new] The new rocmd_binary lookup in the step is a copy of an existing helper — tests/e2e-cucumber/tests/e2e/examine_steps.rs:1375-1380
  The env var lookup and its panic text are word-for-word the same as rocmd_binary() in tests/e2e-cucumber/tests/e2e/artifact_steps.rs:45-58. The copy leaves out that helper's canonicalize. That is harmless today because isolate_cmd sets no current_dir, but the two copies can now drift independently.
  Confidence 75 · mechanical · Fix: make artifact_steps::rocmd_binary pub(crate) and call it from the step.
• [new] The common.rs entry in docs/architecture.md no longer parses — docs/architecture.md:34
  The edit puts an em-dash aside containing a colon and its own relative clause inside the parenthesized list of what common.rs owns. As a result, the list's closing item "and small arg/healthcheck/endpoint-key utilities" now reads as part of the run_with_timeout sentence. In the rocm-core paragraph (:38), the long new process.rs passage also sits between the module list and the host_gpu.rs/rocm_install.rs sentence, with nothing linking them.
  Confidence 75 · mechanical · Fix: end the common.rs parenthetical after the original items and move the pointer to process.rs into a sentence of its own. Do the same with the process.rs passage in the rocm-core paragraph.

Decisions for the author

• When the drain grace runs out after a clean exit, the call returns success with whatever was collected — tradeoff
  At process.rs:200-205,238, a reader that is still unfinished after DRAIN_GRACE produces Ok(Output) with no signal to the caller. Callers parse stdout as JSON. Without a grandchild, the gap is a reader that the scheduler has not run within 200ms on a loaded host, and the symptom would be a parse error that points at the wrong cause. The other side is that turning this into an error would fail any child that legitimately leaves a helper holding the pipe. The description says the choice was deliberate: "a child that legitimately leaves a background helper holding the pipe (sandboxed installers do) would start failing outright, which is a wider behaviour change than this fix." A middle option is to return the output together with an "incomplete" marker that the JSON-parsing callers can check.
• The non-zero-exit quote in capture_amd_smi_json is still unbounded — answered-by-intent
  At apps/rocmd/src/common.rs:132-145, the non-zero-exit bail! quotes all of stderr/stdout into the same persisted note that this PR bounds for timeouts. That reopens the log-growth path MAX_QUOTED_OUTPUT_CHARS exists to close. The issue is older than this PR, so without the description it would have been a non-blocking finding. The description says: "Tracked in #623, which also carries the related unbounded stderr quote in capture_amd_smi_json's non-zero-exit bail! (that one predates this PR)." #623 is open and describes exactly this case. Confirm that is still the plan.

Positive signals

• Every grandchild unit test runs under each shell present (sh/bash/dash) and backgrounds explicitly. Without that, the test would quietly pass vacuously on hosts where the shell execs the last command.
• examine-21 checks the note's wording ("exceeded 2s timeout", ...[truncated]) in the same test as the resulting state (amd_smi_available: false, length bound). That is what AGENTS.md §3 asks for when a message describes the CLI's own behavior.
• The quote is truncated where it is assembled, so all four callers get the bound without adding code at each call site.
• examine-20 checks both probes and a null note. A run where only the monitor probe failed would otherwise still pass.

Deployment notes

None

What this covered

I read every file in the change completely: crates/rocm-core/src/process.rs, the three migrated callers, the lib.rs module line, docs/architecture.md, and the e2e feature and step files. I also read the code around them that affects the verdicts: capture_amd_smi_json and gather_gpu_snapshot, the watcher interval, resolve_amd_smi_binary and its HOME/PATH handling, isolate_cmd, artifact_steps::rocmd_binary, the sibling capture helpers in host_gpu.rs, examine.rs and therock.rs, and the crate-edge allow-list. The diff was prw-base…HEAD; the base tip is c1c2aeb3d7c5, merge-base 03afcf432e2a, head 65333278e062. The base moved 3 commits after the merge-base, and none of them overlap this change, including docs/architecture.md.

• Static only, because of low disk space. Nothing was compiled or run: no cargo, tests, clippy, coverage or e2e. Findings are scored by reading the code against itself, so no score is 100. The verdicts on whether each test fails on revert, and on which branches a mutation leaves green, were reasoned from the code, not run. The coverage-floor impact on rocm-core/rocmd/rocm is unchecked.
• CI for 65333278 is all green, including build-and-test, windows-build-and-test, clippy, E2E tests and Coverage; the skillscope and Sphinx checks were skipped. No self-hosted GPU-lane checks appear in the rollup. Both new scenarios are tagged @requires-os:linux with no GPU gate, so they belong on the hosted E2E lane, which passed.
• Passes. Four independent workers (process and callers, e2e scenarios, docs/instructions/history/comments, and design questions) plus my own consolidation and step-9 pass. The prior-changes pass, which applies earlier review comments on these files, was not run: reading review comments is outside this run's remit.
• Intent sources. I read the PR description, ROCMAI-448 (by key) and #623 (by number, title/body/state only).
• Leak scan and injection. The diff contains no internal hostnames or URLs. No prompt-injection content was found.
• Discussion. These findings were not reconciled against the PR's discussion.

[r0x0r]

Thanks @jussielo-amd — both blockers confirmed against the code and fixed in 5d27a0e (on top of a merge of main, 0606bc4).

1. wait_bounded's try_wait() error path never killed the child

Confirmed: the ? propagated with neither kill() nor wait(), and run_captured only abandons the two Captures on that path, so the child was dropped unreaped. try_wait()'s result is now held, and an Err does a best-effort let _ = child.kill(); before propagating — same style as the timeout branch immediately below. You're right that the realistic trigger is ECHILD, where there is nothing to reap; the value here is that every exit from the loop now kills, so the next reader doesn't have to re-derive that one of them is special.

For the test I added a kill observation to the existing Waitable fake rather than a new seam, as you suggested: FailingChild now carries an Arc<AtomicBool> raised by kill, and the shared helper asserts it for both wait-loop failure paths.

Evidence the assertion can actually fail. With the fix, both tests pass. Removing only the kill() I added (the timeout branch's left untouched):

thread 'process::tests::a_failure_to_poll_the_child_still_abandons_both_readers' panicked at
crates/rocm-core/src/process.rs:1454:9:
every exit from the wait loop must kill the child first; this one returned the error with the
child still running

test result: FAILED. 1 passed; 1 failed

The reap test stays green under that mutant, which confirms the new assertion is pinning the poll path specifically and not riding on the kill the timeout branch already did. Restored, both pass again.

2. The new amd-smi stub writer reintroduced the ETXTBSY race

Confirmed, and it was inconsistent even within its own file — examine_steps.rs:1398 already calls write_fake_executable for the lspci stub. write_amd_smi_stub now calls it too, matching the siblings and #649/2314b2de. Your reasoning on why it matters here is right: these scenarios are @requires-os:linux with no @requires-gpu, so they run up to 64-wide on the no-GPU lane, which is exactly the precondition the module docs describe.

The stub is still executable after the swap: the helper sets 0o755 (asserted in fake_executable's own unit test, mode & 0o777 == 0o755) and replaces an existing file just as std::fs::write did, and all three scenarios actually exec the stub and pass.

Verification (devbox, Linux): cargo clippy --workspace --all-targets -- -D warnings → 0; cargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings → 0; cargo test --workspace --all-targets --no-fail-fast → 3853 passed, 0 failed across 32 groups; cargo xtask e2e -- -n "examine-2[456]" → 3 scenarios, 3 passed.

Your three non-blocking items I am deliberately leaving alone, per your own guidance that they need no action before merge: the sibling capture helpers (examine.rs::run_raw, therock.rs::wait_with_output_bounded, the two in host_gpu.rs) still use the unbounded-join variant, already disclosed in docs/architecture.md and tracked in #623; the nonzero-exit failure messages are not bounded the way the timeout message is; and the two stall scenarios' orphaned sleep 10 stays as the in-code comment already acknowledges.

Both blockers addressed at 5d27a0e. (1) wait_bounded now does let _ = child.kill(); before the ? on try_wait, matching the timeout branch. The test asserts it via the existing Waitable seam; removing only that kill (timeout branch untouched) makes a_failure_to_poll_the_child_still_abandons_both_readers fail while the reap test stays green — so it pins the poll path rather than riding on the timeout branch. (2) write_amd_smi_stub now calls write_fake_executable; examine_steps.rs has zero remaining set_permissions. Worth noting the file was already inconsistent with itself — examine_steps.rs:1398 used the helper for the lspci stub. Your three non-blocking items are deliberately left per your own guidance (#623 sibling helpers, unbounded nonzero-exit messages, the orphaned sleep 10).

[rominf]

🔴 Automated review · pr-review-watcher · 5d27a0e

This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.

Round status (added by the poster): this round is a full review of the change at 5d27a0e1. It is not narrowed, because the range since the last review point (d22758b3) includes merges from main. Nothing blocks, and this automation holds no change request on this PR.

• The previous round's non-blocking finding is addressed: that finding said the examine quote assertion could not tell head from tail. This round's blind review found a quote assertion pinned to a market name unique to GPU 0, and it lists that under Positive signals. The author's reply 6165807292 describing the fix was acknowledged.
• Every finding below is new this round. None of them blocks.

Review — no blocking findings

This is a full review of the whole change.
It implements all of ROCMAI-448: the three copies of the drain-after-exit loop now share one run_with_timeout that drains as the child runs, so the timeout measures elapsed time and not how much the child printed. The description's claims match the change.

Blocking

None

Non-blocking

• [new] A normally exiting child's own output can be missed at the drain cutoff, and two comments say it cannot — crates/rocm-core/src/process.rs:597-599 (DRAIN_GRACE doc), crates/rocm-core/src/process.rs:1174-1178 (test comment)
  • What the comments say: a finished child "has at most a buffer or two left to collect", and its reader "drains the pipe microseconds later, long before the wait loop's next poll even notices the exit".
  • Why that is wrong: the first try_wait happens with no sleep before it. A fast child can be seen as exited before either reader thread has been scheduled.
  • What I ran: I set DRAIN_GRACE to 0 and ran cargo test -p rocm-core --lib process::tests three times. a_command_that_succeeds_reports_its_output and a_grandchild_holding_the_pipe_does_not_hold_a_successful_call failed with stdout "" against expected "hello" and "ready". Run alone, the same test passed 3 of 3.
  • Consequence: the 200 ms grace is all that stands between a loaded host and a silent Ok with missing output. The JSON callers would then report it as a parse error.
  • What governs: the code is what runs, so the comments are the side to correct.
  • Confidence 100 · logic · Fix: correct both comments so they say the grace also absorbs reader-thread start-up. Optionally, on a non-timeout exit, wait until the readers finish or the remaining timeout budget runs out, instead of a fixed 200 ms.
• [new] The abandoned guard in the reader's error branch has no observable effect — crates/rocm-core/src/process.rs:693-706
  • Without the guard, a late read error sits in a slot nobody reads. With it, the error is dropped. Neither outcome is visible.
  • Removing the guard left all 19 process::tests green. The 10-line comment justifies a race that has no consequence.
  • Confidence 70 · mechanical · Fix: store the failure unconditionally and drop the comment.
• [new] The process.rs module doc is stale — crates/rocm-core/src/process.rs:5-10
  • The //! header still lists only spawn/terminate primitives and FileLock. docs/architecture.md was updated to name run_with_timeout; the module's own doc was not.
  • Confidence 70 · mechanical · Fix: add run_with_timeout to the module doc.
• [new] An AI co-author trailer is in the branch history — commit 6533327
  • It carries Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>.
  • AGENTS.md §11 says to avoid AI-generated boilerplate footers. GitHub's squash merge carries Co-authored-by trailers into the squash commit.
  • Confidence 70 · mechanical · Fix: drop the trailer from the squash message, or reword the commit.

Decisions for the author

• When a call times out, the readers are abandoned but the child's children are not killed — tradeoff
  • For abandonment: it bounds the call without touching background helpers, and avoids sending SIGPIPE to them.
  • The cost: an MCP or chat install sdk / install_engine runs uv pip install with Stdio::inherit() (apps/rocm/src/therock.rs:5739-5755) under a flat 2-minute budget.
    • At the deadline only rocm is killed. The orphaned uv keeps changing the venv after rocm has released its file lock.
    • The call now returns at about 2m 0.2s, where before it blocked until uv finished. A retry can therefore run alongside the orphan.
  • The alternative and its own cost: a process-group kill (process_group(0) plus killpg) would reap uv and spare the managed server, which already calls setsid.
    • On Windows, the equivalent Job Object would also kill servers started through spawn_detached_no_inherit.
    • The flat 2-minute install budget, which predates this change, is the bigger lever.
• A clean exit returns whatever was captured once DRAIN_GRACE expires — answered-by-intent
  • The description: "Deliberate: a reader still unfinished after a normal exit returns what it has … failing with 'output incomplete' … was considered and not taken: a child that legitimately leaves a background helper holding the pipe (sandboxed installers do) would start failing outright."
  • Otherwise this would be a silent-failure finding, since Ok gives no sign the output is partial.
  • The comment finding above shows the window covers the child's own output under load, not only a grandchild's. Confirm the 200 ms is still what you want.
• The note from capture_amd_smi_json's non-zero-exit branch is still unbounded — answered-by-intent
  • Location: apps/rocmd/src/common.rs:130-143.
  • The description: "Tracked in #623, which also carries the related unbounded stderr quote in capture_amd_smi_json's non-zero-exit bail! (that one predates this PR)."
  • Otherwise this would be a non-blocking logic finding. This change does widen it: before, a child that printed more than one pipe buffer could never reach that branch, because it blocked and was timed out. Now an amd-smi that prints megabytes and exits non-zero writes all of it into the note that the watcher persists every 60 s.
  • Both #623 and the new comment at common.rs:64-73 ("bounded, because it is persisted") read as if only the timeout path matters. Confirm #623 is the plan, and consider correcting that comment.

Positive signals

• The Waitable trait makes the two wait-loop error paths, which skip the normal take(), unit-testable. They are well covered: each of the two tests fails under four of my mutations, including "no kill on poll error" and "no abandon()".
• The grandchild cases run under every shell present (sh, bash, dash) and background explicitly, so whether the shell forks or execs cannot quietly turn them into tests of the easy case.
• examine-25 pins the quote to a market name unique to GPU 0, so "quotes the head" cannot be satisfied by a quote that kept the tail.

Deployment notes

• The GPU snapshot note and the gpu.metrics_unavailable event's reason now carry the whole error chain. On every host without amd-smi it ends …: failed to spawn process: No such file or directory (os error 2). Error wording also changed from "failed to spawn child process" to "failed to spawn process", and "failed to collect timed-out … output" to "failed to reap timed-out …". Nothing in the repo parses either string.

What this covered

• Read: every changed file completely, against 7561704 (merge base with prw-base)…5d27a0e1. Also read: the four call sites' surroundings, command_failure_detail, truncate_to_chars, the MCP, chat and sandbox argv that reach run_with_timeout, and how long-lived children inherit stdio.
• Passes: the review was split across five independent workers (process.rs core; callers and docs; e2e scenarios; source passes; design questions). All returned.
• Agent-instruction, history and code-comment passes: ran.
• Prior-changes pass: ran against review comments on #584, #599, #483, #487 and #499.
• Mutation runs:
  • 15 single-branch mutations of process.rs in unit tests. Every mutated branch was caught except the redundant guard above.
  • e2e examine-24, -25 and -26 against a reverted common.rs (3 of 3 failed) and a static-only {error:#} revert (only examine-25 failed).
  • My own rerun of the DRAIN_GRACE = 0 case.
• Test verdicts:
  • Two unit tests fail under none of the mutations I ran:
    • a_clean_run_returns_both_streams is a happy-path test.
    • a_timeout_falls_back_to_stdout_and_then_to_saying_there_was_none pins command_failure_detail's fallback order.
  • a_command_that_succeeds_reports_its_output and output_arriving_after_the_child_exits_is_still_collected both pass under the old drain-after-exit shape. Neither claims to test that defect.
  • These are not defects.
• Required CI checks: all green on 5d27a0e. The non-required "E2E self-hosted" run was still pending, so no GPU-lane result exists for this head.
• Did not run:
  • The Windows-only tests: no mingw toolchain, so the Windows cfg arms were checked by reading only.
  • The DRAIN_GRACE truncation at the real 200 ms under artificial load.
  • Some e2e mutation verdicts were reasoned rather than run, because two workers were editing process.rs in the shared checkout at the same time.
• Ticket: ROCMAI-448 retrieved by key (summary, description and status only).
• Discussion: not reconciled against any PR discussion here.

[jussielo-amd]

Reviewed 5d27a0e1 (current head). Both blocking issues from my prior two rounds are resolved here, verified directly against the diff rather than taken on the commit message's word:

• wait_bounded's try_wait() error path never killed/reaped the child (process.rs:864-868) — now does let _ = child.kill(); before propagating, matching the timeout branch. The test (FailingChild) now records kill via an Arc<AtomicBool> and asserts it on both wait-loop failure paths.
• The amd-smi stub writer reintroduced the ETXTBSY race — write_amd_smi_stub now calls rocm_core::fake_executable::write_fake_executable, matching every sibling *_steps.rs file.

I also re-checked apps/rocmd/src/sandbox.rs and apps/rocm/src/main.rs: both correctly route through the shared rocm_core::process::run_with_timeout.

This looks good to merge from a correctness standpoint. No blocking issues from me on this head.

Non-blocking, worth a look

• process.rs's module doc is stale — still describes only spawn/terminate primitives and FileLock; doesn't mention run_with_timeout, which is the function doing most of the work in this file now. docs/architecture.md was updated to name it; the module doc wasn't.
• DRAIN_GRACE's doc comment overclaims — it says a cleanly-exited child "has at most a buffer or two left to collect," but wait_bounded's first try_wait() call has no sleep before it, so a very fast child's exit can be observed before either reader thread has even been scheduled. In practice the 200ms grace absorbs this comfortably, so I'm not calling this a logic bug — just a comment stating a stronger guarantee than the code structurally provides. Worth tightening the wording (e.g. "the grace also covers reader-thread start-up"), especially since the no-GPU e2e lane runs up to 64 scenarios concurrently.
• The abandoned guard in the reader's read-error branch has no observable effect — I traced this: Capture::take() is the only place the failure slot is ever read, and take()/abandon() each run exactly once per Capture. So once abandoned is true, nothing will call take() again — a failure stored after that point just sits unread. Real but inert; fine to simplify or leave as defensive.

Possible issue to confirm, not block on

• One commit unique to this PR (6533327) carries an AI Co-Authored-By: trailer. Worth flagging precisely because GitHub's squash-merge UI pre-checks Co-authored-by trailers by default — it won't disappear on its own. Whoever does the squash-merge should uncheck it.

Everything else here is solid. Nothing from me stands in the way of merging once CI is green.

[r0x0r]

Thanks — all three non-blocking items addressed in f98cda7c. Comments only: the diff touches no executable line (git diff -U0 | grep '^[+-]' | grep -v '^[+-]\s*//' is empty).

1. Stale module doc — agreed, fixed

Confirmed: the module doc still read "Process-spawn and -terminate primitives" and listed only the Windows spawn/exit-wait, process-tree termination and FileLock, while run_with_timeout plus Capture/run_captured/wait_bounded/Waitable/assemble_output are now most of the file, and docs/architecture.md:85 already names it. The two disagreed about what the module is for. The doc now names run_with_timeout, states the two guarantees that matter at the call site (a thread per pipe from the moment the child starts; readers abandoned rather than joined so a grandchild cannot outlast the deadline), notes the bounded quote, and points at #623 for the helpers not yet converged onto it.

2. DRAIN_GRACE overclaimed — you're right, and the code is fine

Re-read wait_bounded rather than taking it on your word. The loop body is let polled = child.try_wait(); as its first statement, with thread::sleep(POLL_INTERVAL) only at the bottom. So the first poll happens with nothing between it and Capture::draining's two thread::spawn calls, and thread::spawn returning guarantees the thread exists, not that it has been scheduled. A child fast enough to exit in that gap is observed exited while neither reader has run an instruction. Exactly as you said: structurally real, operationally absorbed by the grace, not a logic bug. Logic untouched; the comment now says the grace covers reader start-up as well as the last buffer or two, and names the 64-wide no-GPU lane as the case where that matters.

3. The abandoned guard — kept, because it is not quite inert

I re-traced it and I think the "no observable effect" reading is right for the case you describe but misses one. Your trace is correct as far as it goes: Capture::take() is the only reader of the failure slot, take()/abandon() each run once per Capture (abandon() is drop(self.take()), and the wait-loop error path returns straight after), so a failure stored once take() has returned does just sit unread.

But take() is not atomic across the two slots:

let bytes = {
    let mut buffer = self.lock();            // buffer lock
    self.abandoned.store(true, Ordering::Release);
    std::mem::take(&mut *buffer)
};                                            // buffer lock released
(bytes, self.failure().take())                // different lock

A reader that hits Err in that gap acquires buffer's lock, sees abandoned == true, and — with the guard — skips the store. Without the guard it would store, and self.failure().take() on the next line would pick it up. That is observable: assemble_output turns any failure on a non-timed-out run into failed to read the output of {label}, so a cleanly finished command whose bytes were already handed back would be reported as a read failure. Narrow window, wrong direction to lose.

So I kept the guard and the let _buffer = sink.lock() ordering ahead of the check (the TOCTOU fix — a bare load could pass the check and then have take() run the whole sequence before the store), and rewrote the comment, which was the actual defect here: it justified the guard by the inert case you identified rather than the one it carries.

Happy to simplify instead if you read that window differently — it is a two-line deletion.

The Co-Authored-By: trailer

Verified: 6533327 ("fix(process): bound the child output a timed-out command quotes") is the only one of this branch's commits carrying it — for c in $(git rev-list origin/main..HEAD) over the full bodies returns exactly that one.

Deliberately not rewritten. Dropping the trailer means rewriting a published, reviewed commit and force-pushing over a branch under review, which is a worse trade than unticking a checkbox. Instead the hazard is now impossible to miss at merge time: the PR description opens with an [!IMPORTANT] callout telling whoever squash-merges to untick the Co-authored-by box in the merge dialog. Flagging it here too, for the same reason.

Gate (head f98cda7c, Linux devbox)

• cargo test --workspace --all-targets --no-fail-fast — exit 0, 3854 passed / 0 failed / 16 ignored across 33 test binaries
• cargo clippy --workspace --all-targets -- -D warnings — exit 0, no diagnostics
• cargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings — exit 0 (separately, since --all-targets skips the harness = false e2e target)
• cargo fmt --all -- --check — clean

Description also reconciled with what actually ships: it now covers the two fixes from your earlier rounds (the try_wait() kill/reap and the write_fake_executable switch) and carries this round's numbers.

[rominf]

🔴 Automated review · pr-review-watcher · f98cda7

This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.

Round status (added by the poster): full review of the change at f98cda7c, not narrowed: one of the last round's open items (the commit trailer) lives in commit history rather than in a file, so a file-scoped follow-up could not re-find it. One finding blocks, so this round files a change request.

• The blocking finding is new this round: a mutation run that earlier rounds did not try. Every other finding below is also new.
• The last round's four non-blocking items: this blind review did not re-find the two DRAIN_GRACE-related comment overclaims or the stale module doc, which f98cda7c rewrote. It did not flag the abandoned guard as inert; it lists the guard among Positive signals, consistent with the author's account of the window between the two locks in take(). The Co-Authored-By trailer on 6533327 is still in the branch; the description now asks whoever merges to untick it (see Deployment notes).
• Both author replies since the last round (6167534015, addressed to another reviewer, and 6186761766) were read and acknowledged.

Review — needs work

Full review of the whole change.
Implements ROCMAI-448: the shared helper now reads stdout and stderr while the child runs, which fixes the stalled probes for both consumers the ticket names (GPU telemetry and the MCP-driven install). The description's "Fixes ROCMAI-448" holds. Converging the other capture helpers on the new helper is left to #623.

Blocking

• No test covers the helper's promise to give the child an empty (null) stdin — crates/rocm-core/src/process.rs:825; the callers that stopped setting it themselves: apps/rocm/src/main.rs:8311, apps/rocmd/src/common.rs:124, apps/rocmd/src/common.rs:214
  This PR moves the null-stdin setting out of the three callers and into run_with_timeout. Its doc says it "sets null stdin … Callers need not — and should not — set them." I removed .stdin(Stdio::null()) from run_with_timeout and ran cargo test -p rocm-core --lib process::: 19 ran, 19 passed, exit 0. A review worker got the same result independently. Without that line, children of rocmd and rocm inherit the parent's stdin, so any child that prompts waits until the timeout kills it. Nothing would catch that.
  Confidence 100 · logic · Fix: add a test whose child reads stdin, e.g. sh -c 'read x || echo eof' (and a cmd equivalent on Windows), and assert it sees EOF immediately rather than timing out.

Non-blocking

• The comment on the {error:#} note change describes less than the change does — apps/rocmd/src/common.rs:64-74, apps/rocmd/src/common.rs:88
  The aside "(Only the timeout: …)" says only the timeout case gains its causes. The spawn-failure case changes too. On a host with no amd-smi, which is every no-GPU host, the note was failed to launch amd-smi static -a -g all --json. It now ends with : failed to spawn process: No such file or directory (os error 2). A worker confirmed this by running the built rocmd, and this note is written to the automation and audit logs every 60s. Separately, the monitor branch's "Same reasoning as the static probe above" inherits "Pinned by examine-25". Mutation runs show only examine-26 catches a revert of that branch. The code is right on both counts, so the comment is what should change.
  Confidence 70 · mechanical · Fix: list the spawn-failure case in the aside, and write "Pinned by examine-26" on the monitor branch.

Decisions for the author

• The amd-smi non-zero-exit path still copies all of the child's output into the persisted note — answered-by-intent
  The description says: "Tracked in #623, which also carries the related unbounded stderr quote in capture_amd_smi_json's non-zero-exit bail! (that one predates this PR)." #623 is open. Without that passage this would have been a non-blocking pre-existing finding. One wording problem remains. The MAX_QUOTED_OUTPUT_CHARS doc (process.rs:623-639) calls the quote site "the only place that knows those characters are raw child output". capture_amd_smi_json (common.rs:129-142) and parse_sandbox_child_output (sandbox.rs) also quote raw child output. Consider narrowing that sentence so the bound is not read as covering every persisted quote.

• When the child exits normally, the readers get 200ms to finish, and whatever they have by then comes back as success — tradeoff

  • Upside: a child that legitimately leaves a background helper holding the pipe still succeeds.
  • Downside: on a heavily loaded host, a reader that is not scheduled within DRAIN_GRACE returns truncated output with no error. The JSON-parsing callers then report a parse failure that points at the wrong cause.

  The description says this was deliberate. Confirm it still is.

• Abandoned reader threads keep memory bounded but their number is not — tradeoff
  Each probe whose grandchild keeps the pipe open leaves two threads alive until that grandchild exits. The daemon probes every 60s. The Capture doc calls this "a leak we can bound"; that is true of memory, not of thread count.

• process.rs now imports crate::examine::truncate_to_chars — non-blocking-improvement
  A low-level process utility now depends on the large diagnostics module (examine.rs). Moving truncate_to_chars into lib.rs, next to command_failure_detail (the other helper that formats child output for messages), would remove that dependency without creating a second copy.

Positive signals

• The Waitable trait plus a fake child turns the two wait-loop error exits, which a real child cannot reach, into deterministic tests that check both the kill and the reader abandonment. A worker confirmed by mutation that removing the kill or the abandonment makes them fail.
• The reader and take() both check abandoned while holding the buffer mutex, so marking a capture abandoned and emptying it happen together and the reader cannot append in between. This is mutation-verified for the reader side (M3).
• The grandchild tests run under every shell on the host (/bin/sh, bash, dash), because whether the shell forks or execs decides whether a grandchild exists at all.
• examine-25 and examine-26 pin the quote with a market name unique to GPU 0. That makes "keeps the head of the output" a real assertion rather than "keeps something". Each then step also checks the note together with amd_smi_available and the snapshot contents, as AGENTS.md §3 asks.

Deployment notes

• The PR description notes that commit 6533327 has an AI Co-Authored-By: trailer. Whoever squash-merges must untick it in the merge dialog, or it lands on main.
• On hosts without amd-smi, the persisted bridge-snapshot note now ends with the spawn cause (failed to spawn process: No such file or directory …). Anything that matches the old exact text will see the change. Grep found no such consumer in this repo.

What this covered

Read in full, every line: all of crates/rocm-core/src/process.rs; truncate_to_chars and its callers in examine.rs; the changed regions and the functions around them in apps/rocm/src/main.rs, apps/rocmd/src/common.rs and apps/rocmd/src/sandbox.rs; the new scenarios and steps in examine.feature and examine_steps.rs; the changed lines of docs/architecture.md. Range: 7561704…f98cda7c, diffed against prw-base. Its tip, af315e0, is 12 commits ahead of the merge base, and git merge-tree against it reports no conflicts. Scenario ids do not collide with prw-base.

What was run:

• cargo test -p rocm-core --lib process::: 19 pass on the PR head, plus 14 single-branch mutations by a worker and one more by me. Only the null-stdin mutation was missed by every test.
• Filtered e2e runs of examine-2[456]: 3 scenarios, against the PR as is and against 3 mutations. Each scenario fails when the change is reverted.
• feature_naming: 4 tests, all pass.

Did not run or could not check:

• A local Windows cross-check: ring needs x86_64-w64-mingw32-gcc, which is not installed here. CI's windows-build-and-test passed on this head.
• The E2E self-hosted GPU run is still pending, with no result for this head. It is not in the current required-check list. Every required check except the docs build passed on f98cda7; the docs build (Sphinx docs build (-W)) was skipped.
• The agent-instruction pass ran (AGENTS.md §3, §5, §6) and the history pass ran (git log on the touched files). The prior-changes pass was not run, because reading earlier review comments is out of bounds for this watcher.
• Fan-out used two workers (the operator's cap); I ran the source-reading and design passes myself.

The PR's discussion was not reconciled here. ROCMAI-448 was retrieved by its key: title, description and status only.

[r0x0r]

Addressed in a55b9641 (two commits on top of f98cda7c). Every claim below was re-settled here rather than taken on the review's word; where a claim held, it held.

Blocking — the null-stdin promise had no test. Confirmed, by my own mutation run.

I did not take the 19/19 result as given. Same mutation on a clean checkout of f98cda7c:

$ sed -i 's|\.stdin(Stdio::null())|// MUTANT|' crates/rocm-core/src/process.rs
$ cargo test -p rocm-core --lib process::
test result: ok. 19 passed; 0 failed; 0 ignored; 661 filtered out; finished in 3.62s
EXIT=0

Nothing went red. The finding is correct and the consequence is real: without that line the child inherits the parent's stdin, so an amd-smi or an install that ever prompts sits in read until the deadline kills it — and the daemon then reports a timeout, which is the same wrong story this PR exists to stop telling.

Added a_child_that_reads_stdin_is_given_an_empty_one, on both platforms. The shape is slightly stronger than the suggestion: the fixture is sh -c 'read line || echo eof' as proposed, but the stdin it has to override is set by the caller, to the read end of a pipe whose write end the test holds open across the whole call. That is what makes it two-sided — the child can only reach EOF if run_with_timeout really replaced it, and the pipe never reaches EOF on its own. A Stdio::null() child that merely happened to be handed an already-closed stdin would not prove anything.

Falsified, same mutation, against the new test:

test result: FAILED. 19 passed; 1 failed; 0 ignored; 661 filtered out; finished in 5.01s
---- a_child_that_reads_stdin_is_given_an_empty_one ----
panicked: a child given an empty stdin must read EOF rather than block until the deadline:
          process exceeded 5s timeout: no output

Restored: 20 passed / 0 failed.

Windows: added, set /p being cmd's read (it leaves the variable untouched at EOF, so pre-setting it to the marker is the batch equivalent of read || echo). Per the file's existing convention the Windows cases live behind the batch-file fixture rather than cmd /C "<command line>"; I split WindowsFixture so a script that needs no payload no longer writes a 200KB one. This half is verified by CI's windows-build-and-test, not locally — ring needs mingw that neither of us has.

One limit, stated rather than left to be found. I also ran .stdin(Stdio::null()) → .stdin(Stdio::inherit()). That mutant passes, and will pass anywhere the test process's own fd 0 is already at EOF. Distinguishing null from inherit needs this binary's fd 0 to be something else, and arranging that is a dup2 racing every other test in the process. The regression that can actually happen — the setting going missing, which is the line this PR moved — is caught. Both facts are now in the test's doc and the mutation table, so the next reader does not have to rediscover either.

Non-blocking

1. The {error:#} aside — correct, verified by running the binary, not by reading. Built rocmd on a host with no amd-smi (which amd-smi → nothing, no /opt/rocm/bin/amd-smi):

{ "amd_smi_available": false,
  "note": "failed to launch amd-smi static -a -g all --json: failed to spawn process: No such file or directory (os error 2)" }

The spawn failure does gain its cause, and "(Only the timeout: …)" was wrong about it. The comment now names both cases and quotes that tail, and says why it matters: on every no-GPU host this is the version of the note readers will actually see.

2. The examine-25 pin on the monitor branch — correct, verified by mutation. Reverted the monitor branch's {error:#} to .to_string() and nothing else, then ran both scenarios:

2 scenarios (1 passed, 1 failed)
examine-25 ✔
examine-26 ✘  the note must name the timeout rather than blaming the launch:
              "failed to launch amd-smi monitor -p -t -u -m -v -g all --json"

examine-25's stalled static probe returns before the monitor branch is reached, exactly as the review says. The comment now reads "Pinned by examine-26" and says why examine-25 cannot be the pin, so the attribution does not drift back.

3. MAX_QUOTED_OUTPUT_CHARS's "only place" sentence — fair, narrowed. The defensible claim is about this message: the timeout text is the helper's own, and a caller sees it only as an opaque error, so this is the only place that can still tell which characters came from the child. The doc now says that, and then says what it does not cover — capture_amd_smi_json's non-zero-exit bail! and parse_sandbox_child_output quote raw child output of their own, unbounded and persisted the same way, and are #623's. Same correction made in the PR description, which carried the same overclaim.

4. Moving truncate_to_chars into lib.rs — declining, with reasons. The dependency is use crate::examine::truncate_to_chars, intra-crate: it adds no crate edge (check-crate-edges is unaffected), no build-order coupling, and no compilation cost. What it buys is tidier layering; what it costs is moving a function plus its two call sites and its unit tests out of the module that owns them, in a PR whose subject is a pipe deadlock — against AGENTS.md §11's "keep each PR scoped to one logical change", and the kind of churn that makes a bug fix harder to review and to revert. If the layering is worth fixing it wants a home of its own (a small text-util module, with command_failure_detail and host_gpu.rs::truncate_diagnostic_line looked at in the same pass) rather than a second grab-bag in lib.rs, which is already 3,100 lines. #623 is the natural place; this is not.

Decisions for the author

• DRAIN_GRACE's 200ms tradeoff — still deliberate, unchanged. A reader not scheduled inside the grace does return truncated output as a success, and a JSON caller then reports a parse error naming the wrong cause. The alternative, failing with "output incomplete, the pipe is still held", breaks children that legitimately leave a background helper on the pipe — sandboxed installers do — which is a wider behaviour change than this fix. A read error is a different matter and is reported.
• Thread count — doc fixed, design unchanged. You are right that "a leak we can bound" is true of memory only; each abandoned capture leaves one reader parked until the grandchild lets go, two per probe. The Capture doc now says what is bounded and what is not, and why the trade stands: a parked reader holds a stack and no CPU, and bounding the count too means closing the read end — the SIGPIPE-to-the-installer change this PR already ruled out.
• The Co-Authored-By trailer on 6533327 — not rewritten, note confirmed. Still the first block of the description, still an [!IMPORTANT] callout telling whoever squash-merges to untick the box. Rewriting it needs a force-push over a branch under review; the remedy stays the checkbox.

Verification — devbox, head a55b9641

• cargo test --workspace --all-targets --no-fail-fast — exit 0, 3855 passed / 0 failed / 16 ignored (33 binaries; 3854 before, +1 for the new case)
• cargo clippy --workspace --all-targets -- -D warnings — exit 0
• cargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings — exit 0 (run separately; --all-targets skips a harness = false target)
• cargo fmt --all -- --check — clean · cargo xtask check-crate-edges — pass · cargo test -p e2e-cucumber --test feature_naming — 4 passed
• cargo xtask e2e -- -n "examine-2[456]" — 3 scenarios, 3 passed

PR description reconciled with all of the above: new unit case, the Windows bullet, three new mutation rows (including the inherit mutant that passes), the narrowed quote-bound sentence, and the head/count in Verification.

Blocking finding fixed and falsified in a55b964: the null-stdin promise now has a test on both platforms (reproduced your 19/19 mutation run first, then 19 passed/1 failed against the new case). Both non-blocking items verified — one by running the built rocmd, one by mutation — and the comments corrected; the truncate_to_chars move is declined with reasons. Full reply: #526 (comment)

[r0x0r]

Closing the one loop I left open above: windows-build-and-test has since passed on a55b9641 (10m47s), and the job log shows process::tests::a_child_that_reads_stdin_is_given_an_empty_one ... ok — so the set /p fixture runs there rather than merely compiling. Every other required check is green on this head.

[rominf]

🔴 Automated review · pr-review-watcher · a55b964

This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.

Round status (added by the poster): review of f98cda7c..a55b9641 (the two commits since the last round) and the files they touch. Nothing blocks, so this round files no change request.

• The last round's blocker (no test for the null stdin run_with_timeout promises its child) was standing, not re-found blind, and was settled by putting it to the reviewer after its report was frozen (anchored evidence). It reran the deletion mutant: a_child_that_reads_stdin_is_given_an_empty_one fails without .stdin(Stdio::null()) and all 20 process:: tests pass with it. That line sits in run_with_timeout's shared, non-cfg code, so the Linux test catches its deletion on every platform. The Stdio::inherit() mutant still passes; the test doc says so, and it is listed under Decisions for the author. The objection no longer holds. The change request it was filed under (5530054581) had already been dismissed, so nothing of this automation's is gating this PR.
• The last round's non-blocking item on the rocmd {error:#} comment: the code comment now names both the spawn failure and examine-26. The new wording raises a separate point below.
• Every finding below is new this round. Both author replies since the last round (6188776911, 6189145857) were read and acknowledged.

Review — no blocking findings

Review of changes since the review at f98cda7, plus the files they touch.
Fixes ROCMAI-448 (retrieved; In Progress). This round adds the stdin test pair and corrects several comments, all inside the ticket's capture helper. The description's account of this round matches the diff, apart from the #623 attribution below.

Blocking

None

Non-blocking

• The new test helper doc says "all three callers"; there are four — crates/rocm-core/src/process.rs:1162
  "which is what lets all three callers hand over a bare Command". There are four call sites of run_with_timeout: apps/rocmd/src/common.rs:134, apps/rocmd/src/common.rs:227, apps/rocmd/src/sandbox.rs:121 and apps/rocm/src/main.rs:8314. None of them sets stdin. The MAX_QUOTED_OUTPUT_CHARS doc, edited in the same diff (process.rs:628), correctly says "All four callers". The grep of call sites governs, so this comment is the one to correct.
  Confidence 85 · mechanical · Fix: "all four callers".

• The comment says parse_sandbox_child_output is tracked in #623, but #623 does not list it — crates/rocm-core/src/process.rs:630-634, and the same sentence in the PR description
  The comment says the capture_amd_smi_json non-zero-exit bail! and parse_sandbox_child_output "are #623's". Issue #623 is open. Its body names four capture helpers and the capture_amd_smi_json stderr quote, and never mentions parse_sandbox_child_output (apps/rocmd/src/sandbox.rs:943, unbounded bail! of stderr/stdout). The issue is the actual tracking record, so the comment overstates it. As written, the sandbox case reads as tracked when nothing tracks it.
  Confidence 85 · mechanical · Fix: add parse_sandbox_child_output to #623's body and acceptance criteria, or drop it from the attribution in the comment and the description.

• The quoted spawn-failure note is Linux-only, and "every no-GPU host" overclaims — apps/rocmd/src/common.rs:66-71
  The comment says that "on a host with no amd-smi at all, which is every no-GPU host, the note now ends ': failed to spawn process: No such file or directory (os error 2)'". There are three problems:

  • That OS text is Linux's. rocmd runs this probe on Windows too (the PR's own Windows test comment says so), and the spawn error text differs there.
  • A no-GPU host can still have amd-smi: resolve_amd_smi_binary checks the managed registry, ~/rocm_venvs/default/bin and ~/.rocm/bin, then falls back to bare amd-smi on PATH (crates/rocm-core/src/lib.rs:1808-1889). Such a host fails at a non-zero exit, not at spawn.
  • The trailing "Pinned by examine-25" now sits right after this new claim. examine-25 pins only the stalled-probe timeout, and no test asserts the spawn-failure wording.

  Two smaller points: run_with_timeout's poll and reap failures also gain their causes through {error:#}, so "Two of the failures" undercounts. And "now" is change-relative wording in a code comment.
  Confidence 70 · mechanical · Fix: describe the spawn case without quoting OS-specific text or the "every no-GPU host" equivalence, and attach "Pinned by examine-25" only to the timeout claim.

• The WindowsFixture struct doc is stale after the split — crates/rocm-core/src/process.rs:1876-1879
  It still says the directory holds "a Windows fixture's script and payload". It also says removal on drop keeps "the payload" from being left behind. After this diff, WindowsFixture::script writes no payload; only flooding does.
  Confidence 85 · mechanical · Fix: "a script, and for flooding fixtures a payload beside it".

Decisions for the author

• The null vs inherit mutant is documented rather than caught — non-blocking-improvement
  The reviewer ran the Stdio::inherit() mutant with the test's stdin at /dev/null, and the test passes, as the helper doc admits. The doc rules out the dup2 route because it would race other tests. There is a route without that race: run the assertion inside a child process whose stdin is a pipe the outer test holds open. One way is to re-exec the test binary with --exact <inner test> --ignored; another is to wrap it in sh -c. That child's fd 0 is then not at EOF, and the parent's descriptors are untouched, so inherit would also fail. The current choice is defensible: deleting the line is the realistic regression, and the test catches it.

Positive signals

• The new stdin test sets stdin to the read end of a pipe the test holds open, and drops the write end only after the call (process.rs:1175-1186). That makes the assertion two-sided rather than tautological, and the comment at the drop says why the ordering matters.
• Splitting WindowsFixture::new into script/flooding means the stdin case no longer writes a ~200 KB payload it never reads.

Deployment notes

None

What this covered

• What was read: every hunk of f98cda7c..a55b9641 (two commits) in apps/rocmd/src/common.rs and crates/rocm-core/src/process.rs. The reviewer also read run_with_timeout, run_captured, wait_bounded, assemble_output, the Capture docs, the Windows fixture and tests, all four call sites, parse_sandbox_child_output, resolve_amd_smi_binary, issue #623's title, body and state, ROCMAI-448, and the PR description (Step 9 only).
• Commands run:
  • cargo test -p rocm-core --lib process:: in the checkout's own target dir: 20 passed, including a_child_that_reads_stdin_is_given_an_empty_one.
• Test verdicts:
  • Unix a_child_that_reads_stdin_is_given_an_empty_one, with the change reverted: deleting .stdin(Stdio::null()) makes it fail (panics at process.rs:1178 after 5.02s). So it does not pass on reverted code.
  • Same test with the line changed to Stdio::inherit(): it passes, which is the documented gap above.
  • Windows a_child_that_reads_stdin_is_given_an_empty_one and the Windows fixture refactor: not run or compiled here. The mingw cross-toolchain is absent, and cargo clippy --target x86_64-pc-windows-gnu failed in a C build script. The reviewer relies on CI windows-build-and-test passing at a55b964, and checked the set /p EOF semantics only by reading.
• CI: the status rollup at a55b964 is all success. Skillscope and Sphinx were skipped.
• Passes:
  • Code pass: single context, because the scope is small.
  • Source-reading passes (agent-instruction adherence, history, prior changes on merged PRs #599, #584 and #483, code comments): run by one independent read-only worker. Its candidates were re-verified against the source before inclusion.
  • Design review: single context, not fanned out, because of the 2-worker budget.
• Not reconciled: the reviewer did not check this report against the PR's discussion or earlier reviews. The poster's round status above did.

[jussielo-amd]

Review — approve, no blocking findings

Head reviewed: a55b9641 (base main at merge-base 7561704d).

This is the latest round on top of the f98cda7c automated review, whose one
blocking finding — no test proving the child actually gets an empty stdin, since
a mutant deleting .stdin(Stdio::null()) left all 19 process:: tests green —
is fixed by 808e7fb8 + a55b9641. I checked this out in an isolated worktree
and reproduced it both ways: reverted the .stdin(Stdio::null()) line and reran
a_child_that_reads_stdin_is_given_an_empty_one, which failed exactly as
claimed (process exceeded 5s timeout: no output); restored it and all 20
process:: tests pass. The test itself is sound — it hands the child a
caller-set, still-open pipe as stdin, so a regression sees that pipe rather than
an immediate EOF, which is what makes it two-sided rather than tautological.
Also verified on this head: both Unix and Windows variants exist, the comment
corrections in the same commits (spawn-failure case named in the {error:#}
aside, examine-26 pin corrected from an inherited examine-25) match what the
code actually does, DCO sign-off is present on every commit in the range, the
leak scan turns up only false positives (internal_mcp_install_sdk_args, the
word "internally" in a comment), and there are no unresolved inline review
threads. All required CI checks are green on this exact head.

Everything else — the KFD/#447 coordination (settled: "#526 merges first"),
the wait_bounded kill-on-poll-failure fix, the write_amd_smi_stub
ETXTBSY fix, the remaining unconverted join()-based capture helpers — was
already addressed in earlier rounds and is correctly tracked in #623 as
follow-up rather than folded into this PR.

One standing item carried forward rather than re-litigated: commit 6533327's
AI Co-Authored-By: trailer, already called out in the PR description with
instructions to untick it in the squash-merge dialog.

Approving.

[r0x0r]

Thanks — all four non-blocking items checked out, and all four are fixed in b0e03def. No behaviour change: comments and the PR description only. Full gate run anyway (numbers below).

1. "all three callers" → four — confirmed. Grepped run_with_timeout( across the workspace: four non-test call sites (apps/rocmd/src/common.rs:134, apps/rocmd/src/common.rs:227, apps/rocmd/src/sandbox.rs:121, apps/rocm/src/main.rs:8314), none setting stdin. MAX_QUOTED_OUTPUT_CHARS and the description already said four, so the test-helper doc was the odd one out. Fixed there.

2. #623 does not list parse_sandbox_child_output — confirmed. Read the issue body: it names the four capture helpers and the capture_amd_smi_json stderr quote, and nothing else. Took the second of your two fixes — the comment and the description are narrowed to what #623 actually records, rather than editing an open issue from this PR.

For a human reader: the sandbox bail! (apps/rocmd/src/sandbox.rs:943) really does appear to be tracked nowhere. It quotes a child's whole stderr/stdout unbounded, the same shape #623 tracks for capture_amd_smi_json. Not filing anything here — flagging it so someone can decide whether it wants an issue or a line in #623.

3. The spawn-failure note — all three points confirmed. resolve_amd_smi_binary (crates/rocm-core/src/lib.rs:1808-1889) does fall back to a bare amd-smi on PATH, so a no-GPU host with amd-smi installed spawns fine and fails at a non-zero exit — "every no-GPU host" was false. apps/rocmd/src/common.rs carries no cfg gates, so the probe compiles and runs on Windows, where that os error 2 text is not the same string. And examine-25's step (examine_steps.rs:1744-1782) asserts failed to launch amd-smi static, exceeded 2s timeout, the market-name head, the ...[truncated] marker and the length bound — nothing about spawn. Your two smaller points hold too: wait_bounded's poll and reap contexts (process.rs:920, :927) arrive under the same outer context, so "two of the failures" undercounted. Rewritten: "Pinned by examine-25" now sits directly on the timeout claim; the spawn case is described without quoting any OS's wording and says plainly that no test pins it; "now" is gone.

4. WindowsFixture struct doc — confirmed stale. script() writes only fixture.bat; flooding() adds payload.txt. Reworded to "a script, and for flooding fixtures the payload beside it".

Decision on the inherit mutant: keeping the current test, not adding the re-exec. Your route does work and does avoid the dup2 race — I agree the technical objection to a child-process assertion is gone. I'm still not taking it, for a reason about what the mutant represents rather than about cost alone: inherit is not a plausible edit here. It is the exact stdio mode this helper exists to keep callers from being handed, so the realistic regression is deletion of the line — which the current test does catch, on every platform, since the line is in non-cfg code. Against that, a self-re-execing test adds harness plumbing (--exact … --ignored, argv-0 discovery, a nested binary whose failure output has to be surfaced) that is itself the most likely thing in the test to break, and breaks in a way that looks like a product failure. I've written that trade into the helper doc so the next reader finds the route and the reasoning rather than only the dup2 dead end.

Gate (devbox, Linux, CARGO_TARGET_DIR off the PVC):

• cargo fmt --all — clean
• cargo test --workspace --all-targets --no-fail-fast — rc 0, 3855 passed, 0 failed across 33 test binaries, including both a_child_that_reads_stdin_is_given_an_empty_one and the flood cases
• cargo clippy --workspace --all-targets -- -D warnings — rc 0
• cargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings — rc 0

PR description reconciled with what ships, including the #623 attribution sentence and the caller count. The squash-merge [!IMPORTANT] note is untouched and still the first block.

Head is now b0e03def.