refactor: rocm and rocmd run engine requests through one stdio client by jussielo-amd · Pull Request #935 · ROCm/rocm-cli

jussielo-amd · 2026-11-05T21:24:47Z

Summary

rocm and rocmd each carried their own client for the JSON-over-stdio request to an engine process, and the two had drifted apart. They now run the child through one implementation, stdio_client.rs in rocm-engine-protocol, and each caller keeps only what is genuinely its own: which program to start, and a small set of options for what it asks of the client. This delivers ROCMAI-875 (item 3 of the Phase 8.4-H follow-up work). A wire-format change is not part of it: the request and response envelopes are untouched, so the engines need no change.

Writing the characterisation tests first also settled the open question about the standalone rocmd binary, and it turned out to be a live bug rather than a corner case. rocmd reached an engine by re-executing its own executable as __engine-stdio <engine>, a verb that only rocm serves. Embedded as rocm daemon that works. The standalone rocmd shipped beside rocm (it is in the release packages and is what the e2e suite drives) answers every such request with error: unrecognized subcommand '__engine-stdio' and exit status 2. So from the standalone daemon the engine Stop of rocmd sandbox-tool stop_server always came back as an error, and every healthcheck failed; for an engine whose stop decides confirmation (aim) the stop could never be confirmed. The third commit fixes that.

What moved

Was	Now
`rocm`'s `engine_stdio_child_request` body: spawn, write the request, collect stderr, read stdout, wait, parse (`apps/rocm/src/engine_ipc.rs`)	`run_engine_stdio` in `crates/rocm-engine-protocol/src/stdio_client.rs`; `rocm` keeps a thin `engine_stdio_child_request` that picks the options
`rocmd`'s `engine_request` body (`apps/rocmd/src/engine_ipc.rs`)	the same `run_engine_stdio`; `rocmd` keeps `engine_request` / `engine_request_via` and picks its options
`decode_engine_response` (rocm) and the inline decode (rocmd)	`decode_engine_response` in `stdio_client.rs`
the two request-envelope constructions	`engine_request_envelope` in `stdio_client.rs`
`with_sigpipe_ignored` and its re-exec test, `reset_sigpipe`'s sibling in `apps/rocm/src/main.rs`	`rocm_core::with_sigpipe_ignored` in the new `crates/rocm-core/src/sigpipe.rs`, test with the same name
the three `ROCM_CLI_*_DIR` pairs listed in `rocm`'s `app_path_env_vars` and in `rocmd`'s `run_rocm_capture_for_paths`	`AppPaths::env_vars` in `rocm-core`; both call it. The `ROCM_CLI_ENGINE_ENVS_ROOT` name is `rocm_core::ENGINE_ENVS_ROOT_ENV`
byte-identical `optional_arg` in `apps/rocmd/src/common.rs` and `apps/rocm/src/engine_runtime_select.rs`	`rocm_engine_protocol::optional_arg`, beside `gpu_indices_to_csv`
the stand-in engine child each caller would otherwise grow	`FakeEngine` in `crates/rocm-engine-protocol/src/fake_engine.rs` behind a `test-support` feature, planted with `rocm_core::fake_executable`

The shared client lives in rocm-engine-protocol rather than rocm-core because it needs the envelope types and rocm-engine-protocol already depends on rocm-core; cargo xtask check-crate-edges is unchanged and green. The crate gains a dependency on anyhow (already in the workspace) and the test-support feature.

Which behaviour wins, and every user-visible difference

Where the two copies differed, the client takes the behaviour that reports more or fails more safely.

A failing exit status is an error only when the child printed no answer. An answer on stdout is decoded even if the child then exited non-zero. rocm already did this; rocmd threw the answer away for the status. In practice rocm __engine-stdio always exits 0, so this only matters for a real crash.
A parse failure carries the child's stderr (failed to parse engine response envelope; stderr: ...), and the decode errors use rocm's wording. rocmd used failed to parse engine response, engine response did not include data and failed to deserialize engine response data; those strings appear only in the daemon's own errors, and nothing in the repository matched on them.
The write of the request is guarded against SIGPIPE. rocm had the guard; rocmd did not. Rust starts a process with SIGPIPE ignored, so the standalone rocmd was never exposed, but rocm daemon runs the daemon inside rocm, which resets SIGPIPE to its default action in main. There, a write to an engine child that had already exited killed the whole daemon before it could say why. It is now an ordinary error.
rocmd now sets ROCM_CLI_CONFIG_DIR, ROCM_CLI_DATA_DIR and ROCM_CLI_CACHE_DIR on the engine child from its own AppPaths, as rocm did. Before, it relied on inheriting them. No change in production, where the two agree; it makes the child act on the roots the daemon was given.
The spawn error names the program in both (failed to spawn engine stdio process <path>); rocm's used to omit the path.
When writing the request fails, the child is killed and reaped. Before, it was left unreaped, which a long-lived daemon would turn into one zombie per failure.
Forwarded stderr no longer panics if this process's stderr is closed (rocm used eprint!; the client ignores a failed forward and still collects the text).
Standalone rocmd (third commit): when the running executable is exactly rocmd, engine requests run the rocm beside it (rocm_core::sibling_binary_path), and the error says so if there is none. Embedded in rocm, a test binary or a renamed copy still use their own executable.

What stays an option per caller, deliberately: rocm forwards the child's stderr to the terminal (that is how an install's progress shows) and sets ROCM_ENGINE_PROGRESS_STDERR=1 for an install; rocmd only keeps stderr for the error and asks for no progress. rocmd threads the service's endpoint key file onto the child when it holds a usable key (the healthcheck's HTTP probe needs it); rocm passes none, because none of its requests (detect, install, resolve-model, stop) is about a running service's endpoint, so rocm does not gain the key. The engine environments root is rocm-only. Neither client has, or gains, a timeout; an engine install legitimately runs for minutes, so adding one is a separate decision. The in-process answer for the built-in engines stays in rocm; rocmd cannot link the engines.

Verification

Ran: cargo fmt --all --check; cargo clippy -p rocm -p rocmd -p rocm-engine-protocol -p rocm-core -p e2e-cucumber -p xtask --all-targets -- -D warnings, and the same for the three engine crates and the dash crates; cargo test --no-fail-fast for rocm-core, rocm-engine-protocol, rocmd, rocm, the three engine crates, xtask and the non-harness e2e-cucumber targets, all green apart from comfyui::tests::status_reports_stopped_when_saved_comfyui_pid_is_gone, which failed once under the full parallel run and passes on its own (it depends on a saved PID being free; it does not touch this code); cargo xtask manifest --check; cargo xtask check-crate-edges; the pre-commit hooks (hawkeye, lychee, fmt) on each commit; a leak scan of origin/main...HEAD (hits are the words private/internal used in the technical sense). A leaf-name test diff against the base shows no test lost (the SIGPIPE re-exec test moved to rocm-core under the same name).

Each new test was shown red by breaking its target. Removing the SIGPIPE guard from the shared client turns three tests red (its own, rocm's and rocmd's, each a re-executed process killed by SIGPIPE); dropping the endpoint key from rocmd's options, flipping rocm's forward_stderr, or dropping the environments-root variable from the client each turns the tests for that option red; replacing the call in rocmd with a private copy of its old client turns five of its tests red; making rocm_executable_from ignore the standalone case turns its unit test red.

The scenario that exercises the standalone daemon was run through the real harness with an e2e-test-hooks debug build (ROCM_CLI_BINARY and ROCM_CLI_ROCMD_BINARY pointing at it): -n "service-stop" is 5 scenarios, all passing. The new step in service-stop-02 fails against the second commit's rocmd with the unrecognized subcommand '__engine-stdio' engine_stop error and passes with the third.

Not run: cargo check --target x86_64-pc-windows-msvc. This host has no C toolchain for that target (ring's build script needs lib.exe), so the cfg(windows) side could not be compiled. The code that differs on Windows is only with_sigpipe_ignored's pass-through and the #[cfg(unix)] fake-engine tests; everything the Windows job should run is a pure function or an injectable seam (engine_stdio_command, decode_engine_response, the stderr collector, stdio_options, rocm_executable_from), and the unit tests for those are not unix-gated. The full workspace gate, cargo xtask coverage, scripts/smoke_local.py and the GPU lanes were not run; CI carries them.

User-visible behaviour

@id:service-stop-confirms-a-stop-and-cleans-up (service-stop-02, Linux, mock lane) drives the standalone rocmd sandbox-tool stop_server and now also asserts that the engine Stop request was answered rather than turned into the engine_stop error above; that is the user-visible effect of the third commit (the report's engine_stop field). @id:service-stop-yes-stops-a-running-service (service-stop-01) and service-stop-03 to -05 run rocm services stop, which answers a built-in engine in process and never starts a child, and are unaffected; they pass. Install, serve and engines scenarios go through the same rocm code and are unchanged apart from the spawn error now naming the program; the ones that start an engine child (GPU or lifecycle lanes) are exercised by the lanes that carry them. Everything else is internal.

Notes for reviewers

Read the commits in order. The first only pins behaviour: it adds FakeEngine, splits each caller's out-of-process half into a function that takes the executable (engine_stdio_child_request, engine_request_via) so a test can aim it at the fake, and adds tests that pass on the unmodified code, including the SIGPIPE case in a re-executed child and the divergences above. The second is the shared client; read its test changes as the diff of what the first commit pinned. The third is independent and can be dropped without affecting the second.

Two things I found and did not change. In the embedded daemon, restart_managed_service re-launches current_exe() supervise ..., but rocm has no supervise verb (rocm supervise answers unrecognized subcommand), so recovery restarts from the embedded daemon cannot start a supervisor; the mirror of the bug fixed here, and outside this change. And rocm's resolve_engine_binary_path looks plugins up through AppPaths::discover() rather than the paths it is given.

Open PRs touching the same files: the endpoint-key guard PR edits apps/rocmd/src/engine_ipc.rs (deletes ensure_public_service_has_endpoint_key and a doc line; the hunks are apart from mine but the imports at the top of the file will need a look), rocm-core/src/lib.rs and rocm-engine-protocol/src/lib.rs; the managed-service stop PR touches rocm-core/src/lib.rs and apps/rocmd/src/service.rs; the clap-tree move PR touches apps/rocm/src/main.rs and engine_ipc.rs. The base moved while this was in review; I merged origin/main (the engine-inventory removal), resolving the engine_ipc.rs module comment and the docs/architecture.md entries, and the merge is signed.

…nd how they read its answer rocm and rocmd each carry their own client for the JSON-over-stdio request to an engine process, and the two have drifted: they build the child's command line differently, set different environment, treat a failing exit status differently, word their errors differently, and only rocm guards the request write against SIGPIPE. Nothing pinned any of it, so the first step toward one client is to pin what each does today. Both clients are driven against a stand-in engine child, a shell script that records its argv, environment and stdin and then answers (or crashes) as the test asks. The stand-in lives in rocm-engine-protocol behind a test-support feature so the two callers share one. Each caller's out-of-process half is split into a small function that takes the executable to run (engine_stdio_child_request in rocm, engine_request_via in rocmd), so a test can aim it at the stand-in; the bodies are unchanged and the public entry points call them with the same arguments as before. The tests assert the child's argv (rocm: stdio; rocmd: __engine-stdio <engine>), the single newline-terminated request line, the environment each caller sets (rocm: the app path variables, the engine environments root, the progress variable for installs; rocmd: the endpoint key file when it holds a usable key), what each does with a nonzero exit, an unparseable answer and an error envelope, and the SIGPIPE case under the default action in a re-executed child. Mutating rocm's write guard away turns its SIGPIPE test red. No behaviour change. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

…ared by rocm and rocmd rocm and rocmd each carried their own client for the JSON-over-stdio request to an engine process, and the two had drifted. They now run the child through one implementation in rocm-engine-protocol (stdio_client.rs); each caller still chooses which program to run, and what it asks of the client is a small set of options: the app paths, the engine environments root, an endpoint key file, a progress request, and whether to forward the child's stderr as it arrives. rocm still answers built-in engines in its own process and resolves the engine binary; rocmd still re-executes its own executable as __engine-stdio. Where the two behaved differently, the client takes the behaviour that reports more or fails more safely: - A failing exit status is an error only when the child printed no answer; an answer on stdout is decoded even if the child then exited non-zero (rocm's rule; rocmd discarded the answer for the status). - A parse failure carries the child's stderr, and the decode errors use rocm's wording (rocmd's said "failed to parse engine response", "engine response did not include data" and "failed to deserialize engine response data"). - The write of the request is guarded against SIGPIPE (rocm's rule). rocmd had no guard, so the daemon hosted in rocm, which runs with SIGPIPE at its default action, was killed by a write to an engine that had already exited instead of reporting why. - The child runs on the caller's app paths in both (rocmd now sets the ROCM_CLI_*_DIR variables; it relied on inheriting them), and the spawn error names the program in both. - A request whose write fails kills and reaps the child, so the long-lived daemon does not collect a zombie per failure. - Forwarded stderr no longer panics if this process's stderr is closed. The endpoint key stays an option only rocmd uses, since none of rocm's requests is about a running service's endpoint; rocm still sets no key. Stderr is forwarded for rocm and only kept for rocmd. Neither client has, or gains, a timeout. Also moved to where both can reach them: with_sigpipe_ignored (and its re-exec test, same name) from rocm's main.rs to rocm-core; the three ROCM_CLI_*_DIR variables as AppPaths::env_vars, which rocm's app_path_env_vars and rocmd's capture wrapper now call instead of each listing them; and the byte-identical optional_arg, from both binaries into rocm-engine-protocol. The fake engine written for the tests now plants its script with rocm_core::fake_executable instead of retrying on ETXTBSY. Both callers' tests now run through the shared client and flip what the previous commit pinned (rocmd's app paths, nonzero-with-answer, SIGPIPE under the default action, error wording). Breaking either caller's options, the client's SIGPIPE guard or an environment variable turns a test red. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

… the standalone daemon rocmd asked an engine to do something by re-executing its own executable as `__engine-stdio <engine>`. That verb is served by rocm. Embedded as `rocm daemon` the daemon is rocm and it works, but the standalone rocmd binary shipped beside rocm answered every such request with "unrecognized subcommand '__engine-stdio'" and exit status 2. The request was made by the stop tool (`rocmd sandbox-tool stop_server`, `rocmd mcp-server`), by the supervisor's readiness wait and by the server-recover watcher's healthcheck, so from the standalone daemon the engine `Stop` always came back as an error and no healthcheck ever succeeded; for an engine whose stop decides confirmation, the stop could never be confirmed. When the running executable is exactly `rocmd`, the request now runs the `rocm` beside it (rocm_core::sibling_binary_path), and says so if there is none. Anything else, the embedded daemon, a test binary or a renamed copy, keeps using its own executable as before. The `service-stop-02` scenario drives the standalone rocmd and now also asserts that the engine `Stop` request was answered. Run against the previous commit's rocmd it fails with the engine_stop error above; with this change it passes, along with the rest of the service-stop scenarios. The choice of executable is a pure function with its own unit test that also runs on Windows. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

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

Copilot

🟡 Changes recommended

Progress forwarding can terminate the CLI via SIGPIPE on a closed stderr pipe, and unescaped apostrophes in responses or scratch paths break the fake engine’s generated shell script.

2 open issues

What changed in this PR

Consolidates rocm and rocmd engine requests into one stdio client while preserving caller-specific options and the existing wire format. Also fixes standalone rocmd engine requests by routing them through the sibling rocm executable.

Changes:

Shares request framing, response decoding, subprocess handling, and common helpers.
Preserves each caller’s environment, endpoint-key, and progress settings.
Adds shared fake-engine tests, standalone-daemon regression coverage, and documentation.

Verification here used standalone signal and shell checks; project tests were not run.

File	Description
`tests/e2e-cucumber/tests/e2e/service_stop_steps.rs`	Asserts that the engine answered the stop request.
`tests/e2e-cucumber/features/service_stop.feature`	Adds the engine-answer assertion to the stop scenario.
`docs/testing.md`	Documents client tests and standalone-daemon coverage.
`docs/architecture.md`	Updates shared-client and helper ownership.
`crates/rocm-engine-protocol/src/stdio_client.rs`	Implements the shared stdio client and tests.
`crates/rocm-engine-protocol/src/lib.rs`	Exports client APIs and the optional-argument helper.
`crates/rocm-engine-protocol/src/fake_engine.rs`	Adds the reusable fake engine fixture.
`crates/rocm-engine-protocol/Cargo.toml`	Adds client dependencies and test support.
`crates/rocm-core/src/sigpipe.rs`	Relocates the SIGPIPE guard and regression test.
`crates/rocm-core/src/paths.rs`	Centralizes child environment paths and the root constant.
`crates/rocm-core/src/lib.rs`	Exports the shared path and signal helpers.
`Cargo.lock`	Records dependency changes.
`apps/rocmd/src/supervise.rs`	Uses the shared optional-argument helper.
`apps/rocmd/src/engine_ipc.rs`	Adopts the client and fixes standalone executable selection.
`apps/rocmd/src/common.rs`	Reuses shared path environment mappings.
`apps/rocmd/Cargo.toml`	Enables fake-engine and signal test dependencies.
`apps/rocm/src/main.rs`	Removes the relocated SIGPIPE helper and test.
`apps/rocm/src/engine_runtime_select.rs`	Delegates path mappings and removes duplicated argument logic.
`apps/rocm/src/engine_ipc.rs`	Delegates subprocess requests and tests caller options.
`apps/rocm/Cargo.toml`	Enables shared fake-engine test support.

_{Review effort: Balanced}

+            Ok(0) => break,
+            Ok(_) => {
+                if let Some(sink) = forward.as_mut() {
+                    let _ = sink.write_all(&line);


+                dir.display()
+            ),
+        )
+        .expect("write the fake engine script");
+        Self { dir, script }
+    }
+
+    /// A fake that reads its request and answers with `response` (a complete
+    /// response envelope as JSON text), exiting with `status`.
+    pub fn answering(label: &str, response: &str, status: i32) -> Self {
+        Self::new(
+            label,
+            true,
+            &format!("printf '%s' '{response}'\nexit {status}"),


Resolves the conflicts with the clap-tree move: the tests it relocated to cli.rs and the SIGPIPE test this branch moved to rocm-core are both gone from main.rs, the engine_ipc.rs module comment keeps this branch's wording, and the docs/architecture.md engine_ipc.rs entry keeps this branch's text plus the note that the watcher-mode conversion now lives in cli.rs. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

`rocm` runs with SIGPIPE at its default action. When it forwards an engine child's progress to a stderr whose reader has gone (a closed pipe), the write delivers the signal and ends the CLI before `write_all` can return, so discarding the result did not protect the request. Both forwarding writes in `collect_stderr` (a line of progress, and the note recorded when reading the child's stderr fails) now go through one helper that runs the write inside `with_sigpipe_ignored`, which turns the signal into the error that is then discarded. `with_sigpipe_ignored` is now safe to overlap. It saved and restored the disposition per call, so two overlapping regions (the stderr forwarder of one request next to the stdin write of another in the daemon) could restore "ignored" over a default action, or the default under a region still open. It remembers the disposition in force before the first region and restores it when the last one ends, and restores on unwind. The re-executed test installs SIG_DFL in a fresh process and forwards into a real pipe whose reader is closed, once per write; the existing `Closed` mock returns the error itself and cannot raise the signal. A second re-executed test covers nested and interleaved regions. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

`FakeEngine` built its script by interpolating the canned response and the scratch directory into single-quoted shell literals. A valid JSON response with an apostrophe (`can't stop`) ended the literal early, so the fake exited with a syntax error instead of printing the envelope, and a `TMPDIR` containing an apostrophe broke the `dir=` line the same way. Both values now go through one `shell_quote` (close the literal, emit an escaped quote, reopen it), and `FakeEngine::new_in` lets a test choose the scratch base without changing the process-wide TMPDIR. Tests cover the quoting helper, the response and the scratch directory against the script itself, and the apostrophe response through the real client. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

jussielo-amd · 2026-11-06T07:23:49Z

Superseded by #929: this pull request's commits are included in that branch (head ef5155b), and #929's description covers this change. Closing so the review happens in one place.

jussielo-amd added 4 commits November 5, 2026 20:28

Merge origin/main into refactor-one-engine-ipc-client

7a83e9b

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

jussielo-amd marked this pull request as ready for review November 5, 2026 21:24

jussielo-amd requested a review from a team as a code owner November 5, 2026 21:24

jussielo-amd requested review from juhovainio and a balanced review from Copilot November 5, 2026 21:24

Copilot started reviewing on behalf of jussielo-amd November 5, 2026 21:25

View session

Copilot AI reviewed Nov 5, 2026

View reviewed changes

jussielo-amd added 3 commits November 5, 2026 22:06

jussielo-amd closed this Nov 6, 2026

Navigation Menu

Repository navigation

refactor: rocm and rocmd run engine requests through one stdio client - #935

refactor: rocm and rocmd run engine requests through one stdio client#935
jussielo-amd wants to merge 7 commits into
mainfrom
refactor-one-engine-ipc-client

jussielo-amd commented Nov 5, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

jussielo-amd commented Nov 6, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

jussielo-amd commented Nov 5, 2026

Summary

What moved

Which behaviour wins, and every user-visible difference

Verification

User-visible behaviour

Notes for reviewers

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

jussielo-amd commented Nov 6, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants