[rominf]

Why

rocm services stop <id> --yes printed "Local server stopped" and exited 0 based only on which tool ran, even when the verified stop could not confirm the server's processes had exited. In that case the CLI had in fact kept the record's PIDs, its stop request and the endpoint key, so the message described a stop that did not happen, and a script checking the exit status moved on as if the server were down. rocm's own stop_server answer hard-coded "status": "stopped" and dropped the verdict the stop had already computed. rocmd's twin of the same code reports this case as stop_unconfirmed (#499).

What changes

	Before	After
rocm services stop, stop not confirmed	Local server stopped on stdout, exit 0	Local server stop not confirmed on stderr, exit 1, with what happened to each recorded process and what was kept
rocm services stop, stop confirmed	Local server stopped, exit 0	unchanged
stop_server answer (rocm sandbox-run)	"status": "stopped" always	stopped or stop_unconfirmed, taken from the stop's verdict; still exits 0 with the verdict in the JSON, as rocmd sandbox-tool does
Ctrl-C in attached rocm serve, stop not confirmed	reported as stopped, exit 0	the same report on stderr, non-zero exit, nothing signalled on its behalf
Ctrl-C in attached rocm serve, the stop itself errors	signals the child it launched, exit 0	unchanged, except that the stop's error and any failure of that signal now go to stderr and are audited at warn/error

The stop report carries the verdict (stopped), each PID's outcome (pid_outcomes, using rocmd's labels), whether the record changed while the stop ran (record_changed) and whether a usable endpoint key was kept (endpoint_key_kept). The printed lines are derived from those fields, so the message cannot disagree with what the stop wrote to disk:

• a process that, or a process under it, did not exit after it was signaled;
• a process whose identity could not be verified as the server's: nothing is signalled, and a ps -p command to check on it is printed;
• a record that changed while the stop ran: a note pointing at rocm services;
• a container engine whose own stop failed (ROCMAI-554: add engines/aim Docker-based AIM serving engine #708): that failure as the reason;
• kept: names the record, and the endpoint key only when a usable one is on disk.

The report deliberately names no command to run the stop again. terminate_verified reports a dead recorded root as already gone without checking the tree under it, so a second stop could confirm, delete the key and print "stopped" while a process under that root still runs. rocmd's comments and MCP error text no longer present a repeated stop as recovery either.

README, the services stop and serve --help text, docs/testing.md and docs/manual-testing.md describe the new output, the exit codes and which JSON fields are authoritative.

Verification

• Unit tests assert the message together with the state: exit status or error, the record's PIDs and stop marker, the endpoint key on disk, the audit level and message. They cover the timed-out, unverified, mixed, changed-record, keyless and container-engine cases, and the attached Ctrl-C decision. Each was checked to fail with its production part reverted.
• Scenarios service-stop-03, -04 and -05 (mock lane, Linux) drive a real rocm services stop through the e2e-test-hooks seam ROCM_E2E_STOP_OUTCOME (timed out, unverified, keyless). They assert stderr, exit 1, the kept record, marker and key, and that no stop command is named. They fail on a binary built without the seam, which shows they go through it. service-stop-01 and -02 are unchanged and still pass.
• cargo test --workspace --all-targets, cargo test -p xtask, clippy -D warnings with and without e2e-test-hooks, cargo fmt --check, a Windows-target cargo check -D warnings (x86_64-pc-windows-gnu), feature_naming and the e2e build pass.
• Not covered by a scenario: the attached rocm serve Ctrl-C stop. Attaching needs a real rocm serve launch with an engine and a runtime the mock lane does not have; the attached_ctrl_c_ unit tests cover everything after the stop answers.
• Not shown end to end: that an unverified PID is never signalled. The seam replaces terminate_verified, so the scenarios prove the report and the state, not the signalling.

Scope

Follow-ups, not in this change:

• terminate_verified treats a dead recorded root as already gone without checking the processes under it. That predates this change, and fixing it is what would make suggesting a retry safe again.
• rocm remote stop, and the cleanup of a part-failed rocm remote serve, relay the remote's unconfirmed-stop report verbatim, including check lines meant for the remote machine. Their non-zero exit also needs documenting, with a docker-lane scenario.
• A seam inside the identity read, plus a rocm-core unit test for the indeterminate path, so that "an unverified PID is never signalled" is tested.
• The dashboard collapses the stop-failure envelope, so an unconfirmed stop there is not reported distinctly.

rocm services stop now exits non-zero when the stop is not confirmed. Callers that treated exit 0 as "stopped", including #299's uninstall flow if it relies on that, will see the failure, which is the intent.

• If this PR fixes a bug, searched tests/e2e-cucumber/expectations.toml for the fixed ticket ID and removed/narrowed any now-stale xfail rows. (No row refers to this behaviour.)
• If this PR adds a new subcommand or subsystem, its domain implementation lives in its own file per docs/architecture.md. (No new subcommand; the stop rendering lives in services_text.rs.)
• Every new or changed user-facing message was read against the code path that runs after it, and its test asserts the resulting state — not only the wording — per AGENTS.md §3.

[nowycondro]

🔴 Automated review · pr-review-watcher · df8fc99

Review — no blocking findings

Full review of the whole change (base 6ab12083…head df8fc992), split by file-coupling into five groups and fanned out as independent workers; only one returned in time (see What this covered for what that means for the rest).
No ticket claimed closed by this change. It cites #499 (rocmd's earlier stop-identity-verification fix), #708 (a container-engine stop failure) and #299 (the uninstall flow) as background/related context only — not as issues this PR resolves.

Blocking

None

Non-blocking

• Stale comment overstates what pid_outcomes contains — apps/rocmd/src/mcp.rs:802-803
  Unchanged by this diff, but sits immediately beside wording this PR did edit, in the same comment block. It says pid_outcomes in the structured result "names the PID that could not be confirmed," but pid_outcomes (built in apps/rocmd/src/service.rs around line 100-109) is a list of every recorded PID with its role and outcome, not only the unconfirmed one.
  Confidence 80 · mechanical · Fix: reword to "pid_outcomes names every recorded PID's outcome, including which one could not be confirmed."

Decisions for the author

• finish_attached_stop's "stop failed, and the direct signal also failed" branch still exits 0 — tradeoff
  In apps/rocm/src/service_launch.rs, once this PR lands, two of the three branches of the attached rocm serve Ctrl-C stop correctly exit non-zero on an unconfirmed/uninformative outcome. The third — the managed stop call itself errored, and the direct fallback signal to the child also failed — still returns Ok(()) (exit 0), same as before this PR. That's pre-existing behavior, not changed here, and the PR's own description names it explicitly ("exit 0, as it always has here"), so this isn't a defect introduced by this diff. Flagging only so the choice is a confirmed one: nothing was actually stopped and the direct kill failed too, yet the command now sits as the one path in this three-way match that still reports success.

Positive signals

• rocm sandbox-run stop_server and rocmd sandbox-tool stop_server deliberately keep exiting 0 with the verdict carried in JSON (status: stop_unconfirmed) — verified in main.rs's InternalSandboxRun handler, which prints the raw JSON unconditionally — while only the human-facing rocm services stop and the attached-serve Ctrl-C path were changed to a non-zero exit. That's the right place to draw that line: it fixes the problem for a human or an exit-code-checking script without breaking the documented JSON-consumer contract.
• The chat/assistant stop_server tool call (apps/rocm/src/chat.rs) maps straight onto rocm services stop <id> --yes as a subprocess rather than re-implementing its own success/failure interpretation, so it inherits this fix for free with no separate change needed.
• The ROCM_E2E_STOP_OUTCOME feature-gated test seam gives genuine black-box e2e coverage (service-stop-03/-04/-05) of outcomes that cannot otherwise be staged from outside the binary (a process surviving SIGKILL, an unreadable /proc entry), rather than leaving that half unit-tested only.

Deployment notes

• rocm services stop (and Ctrl-C during an attached rocm serve) now exits non-zero when the stop is not confirmed, where it previously always exited 0. Any script or automation that checks only the exit code and not the printed text will now see a failure here — intended per the PR, which names #299's uninstall flow as a possible example, but worth flagging to anyone rolling this out.
• The stop_server sandbox-tool/MCP JSON answer's status field now takes stopped or stop_unconfirmed instead of always stopped. A consumer keying success off status == "stopped" rather than result.stopped should confirm it handles the new value on the rocm CLI side, as it should already on the rocmd daemon side since #499.

What this covered

Base 6ab12083e7dd7901eb54e6a4a5b0fd362f4d8eb8…head df8fc9920e556160aa5262dd34c67bbf74f6a564 (PR #799's current head, fetched read-only into a local pr-799 ref and never checked out).

The change was split by file-coupling into five groups and fanned out as five independent background workers. Only one — apps/rocmd/src/{mcp,service,test_support}.rs — returned within the available time; it found no findings scoring ≥70, having verified the diff there is comment/string-only, checked the new wording against the unchanged logic it describes, and compared rocmd's stop contract against rocm's for semantic drift (none found).

The other four groups' workers (plus further sub-workers some of them had spawned) did not return in time and were stopped before completion. Per this method's contingency for a worker that does not return, those four passes were instead run directly by this reviewing context, which is not an independent pass the way the fan-out is meant to produce, and should be weighed accordingly:

• apps/rocm/src/services_text.rs (the core verdict/rendering logic, ~2838 lines, ~914 diff lines) — read in full, including every new/changed test.
• apps/rocm/src/main.rs (targeted: the touched regions, the ServicesCommand::Stop enum/help text, run_approved_service_action, the new finish_service_action, and every pre-existing test referencing them — not the file's ~9500 unrelated lines cover-to-cover) and apps/rocm/src/service_launch.rs (read in full, including every new/changed test).
• tests/e2e-cucumber/{features/service_stop.feature, tests/e2e/service_stop_steps.rs, tests/e2e.rs} — read in full.
• README.md, docs/manual-testing.md, docs/testing.md — read in full and cross-checked against the rendering code and tests they describe.

Design review and beyond-diff impact analysis were also done directly by this context: traced that rocm sandbox-run/rocmd sandbox-tool's JSON-exit-0 contract is deliberately preserved, that the chat/assistant tool call delegates to the same CLI path rather than re-implementing it, and that the dash-TUI/automation-proposal consumers are explicitly named as out of scope in the PR's own "Scope" section rather than a silent gap.

CI at the reviewed head: all required checks green, including build-and-test, Test (affected crates), clippy, E2E tests, E2E consolidated report, windows-build-and-test, Commit signatures + sign-off. Two GPU-hardware lanes (MI300X, W7900) were still queued and two others (Strix Halo, Ubuntu, release/nightly) were cancelled at review time — neither failed, but their result for this exact head could not be confirmed.

Not reconciled against any PR discussion: this PR has zero review comments and zero prior reviews as of the head reviewed. Did not independently run cargo test/cargo build --features e2e-test-hooks here — relied on the green CI run plus direct code tracing instead.

[juhovainio]

I found no blocking issues. The root cause is fixed at the shared layer: stop_server now takes its status from the stop's own stopped verdict, and both rocm services stop and the attached Ctrl-C path fail on stop_unconfirmed. The printed lines come from the same fields the stop wrote to disk, and the unit tests plus scenarios service-stop-03/04/05 assert the message together with the resulting state. README, --help, docs/testing.md and docs/manual-testing.md agree with the code.

Non-blocking:

1. stop_failure_summary in the e2e harness has no test of its own (inline).
2. In finish_attached_stop, a stderr write failure returns before the audit event is recorded (inline).
3. An unconfirmed stop now exits 1 instead of 0. Worth confirming that other callers that treat exit 0 as stopped (the #299 uninstall flow, any other run_internal_sandbox_tool caller) handle stop_unconfirmed.

This is a read of the diff plus the CI results at review time; W7900 E2E was still pending, and the Strix Halo Ubuntu nightly/release jobs were cancelled, which looks like runner contention.

[juhovainio]

Nit: stop_failure_summary is new logic (prefer the Error: line, else the last line) with no test of its own. A two-case unit test would pin it.

[juhovainio]

Nit: the child is signalled here before the stderr note is written. If that write fails, ? returns before the audit event is recorded. Consider recording the audit event first, or ignoring the write error.