fix(rocmd): verify process identity before stopping a managed service

[rominf]

• 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 rows reference this area.
• If this PR adds a new subcommand or subsystem, its domain implementation lives in its own file per docs/architecture.md. — N/A.
• 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. — see "On the Gherkin requirement".

Fixes #498

Summary

rocmd's managed-service stop signalled recorded PIDs — SIGTERM, then SIGKILL — without checking they still identified the recorded processes, and expanded descendants against a live process listing. A stale PID therefore took whatever now owned it and that process's children with it. Reachable from the CLI and from the daemon's MCP stop_server tool.

apps/rocm already does this correctly, pairing each PID with its own start-time token and routing through rocm_core::terminate_verified. This brings rocmd to the same contract.

The non-obvious half: routing the stop through terminate_verified alone would have been a placebo. rocmd never captured the identity tokens at all, so every record it wrote had None, which identity_state classifies as Matches through its documented legacy branch. The tokens are now captured at the four sites that record a PID, while the process is known alive, each paired with its own PID.

Scope of the fix. On Linux this closes the stale-PID routes through a crashed service and through a repeat stop. It does not close the reboot route listed in #498: start-time tokens count ticks since boot with no boot identifier, so a manifest that survives a reboot can still falsely match. That is tracked in #548.

Risk: medium. It replaces a termination path. Legacy records with no tokens were tested explicitly and still stop, via the same legacy branch.

What changed

• Stop routed through rocm_core::terminate_verified, mirroring apps/rocm's entry/dedup shape. The old ps-based helpers are deleted rather than left unused — a dormant unverified kill path invites reuse.

• Identity captured at the four PID-recording sites.

• Repeat-stop made safe: PIDs and tokens are cleared once every recorded process is confirmed gone, and deliberately kept otherwise so a retry can still reach a survivor.

• An unconfirmed stop no longer claims success: status, the PID clearing, stop_requested_unix_ms and the endpoint-key deletion are now gated on all_stopped together, matching rocm services stop. Previously the key file was deleted unconditionally — and an unconfirmed stop is reachable on timed_out or unverified, the latter being precisely a live process whose identity could not be read. Discarding the only copy of the key of a service that may still be running locks the CLI's own probes out of it. Gating the deletion without writing the marker would have stranded the key forever, so all three move together; rocm's deferred cleanup keys on that marker.

• An unconfirmed or in-flight stop can't be undone by the daemon. The stop request (stop_requested_unix_ms) is written before any signal, and server-recover skips any service with a standing request, both when scanning and when handling a queued event.

• The stop's final manifest write re-reads the record and applies only its own changes, so concurrent writes survive. If the record names different processes by then, the stop reports itself unconfirmed and leaves them alone.

• Recording a PID with its own token is now ManagedServiceRecord::record_supervisor_identity / record_engine_identity in rocm-core, used by every production PID writer in rocm and rocmd. This fixes rocm serve --background and rocm services restart, which recorded the engine PID without a token.

• rocmd's recovery restart drops the replaced run's engine PID and clears any stop request.

• rocm services stop re-reads the record before its final write and confirms only if it still names the processes this stop handled; otherwise it re-stamps the stop request and keeps the key, as rocmd's stop does. The comparison is ManagedServiceRecord::names_same_processes in rocm-core, shared by both.

• A restart records its new supervisor through the same re-reading helper, so a stop request persisted during the spawn survives.

• rocm services stop now writes the stop request before the engine Stop and before signalling, like rocmd's stop, so the daemon cannot restart a service mid-stop.

• supervise_service's writes after its first re-read the record and change only the supervisor's fields: a supervisor that outlives an unconfirmed stop can no longer erase the stop request when its engine exits. The restart's failure write does the same.

• A restart records no supervisor until its spawn succeeds, so a failed spawn can't leave the daemon itself recorded as the service's supervisor.

• vLLM's state file records server_start_ticks next to server_pid, and refresh_from_engine_state takes start_ticks when the server PID is the launcher's own, so a refreshed vLLM record keeps its engine token.

The result JSON gains pid_outcomes ({pid, role, outcome}) and a stopped bool, because the old skipped_pids could not distinguish "nothing was there" from "live, but provably someone else's". No consumer in the repo parses these keys — checked the dash TUI, the daemon, the e2e steps and the MCP surface.

Windows

rocm_core::process_tree_pids returns only the root on Windows, and rocmd launches the engine one level below the recorded PID. Replacing taskkill /T /F with the portable path would have left the engine alive, so Windows keeps the tree kill, taken from the still-live root and now gated on the identity verdict — which there can only ever turn away a PID that is not running, since process_start_ticks is None off Linux. That is a pre-existing rocm-core gap (it also means rocm services stop signals blind and misses descendants on Windows today) and is left for its own change.

Test plan

Red-before/green-after at commit level: at the test commit the four stop tests fail, and their output shows the real defect — "force_signaled_pids":[...] for an unrelated bystander, and a stale root's descendant dragged in. At the fix commit they pass.

The capture sites are guarded by mutation, not by inspection: setting all four production process_start_ticks(...) calls to None — which returns rocmd to exactly the pre-fix hazard — moves the suite from 135 passed, exit 0 to exit 101 with precisely the two capture tests failing. Before those tests existed, the same mutation left the suite fully green.

• cargo test -p rocmd --all-targets — 152 passed, 0 failed (exit 0), at the head after review
• cargo clippy --locked --workspace --all-targets -- -D warnings, cargo fmt --all --check, cargo xtask manifest --check — all exit 0

Gaps stated rather than glossed. Two of the four capture sites are not individually covered — mutating only those two leaves the suite green, and that was measured, not assumed. supervise_service's engine-child token sits past a path ending in process::exit, which in-process kills the test binary; covering it needs an injectable exit or a real engine in e2e. The restart's own-PID token is overwritten microseconds later and is observable only if the spawn between them fails. The unconfirmed-stop branch is covered only by injecting the per-PID outcome: a real timed_out needs a process surviving SIGKILL for 10s, and a real unverified needs an unreadable /proc/<pid>/stat. rocm's equivalent gate is untested for the same reason.

On the Gherkin requirement

service-stop-01 (features/managed_service_stop.feature, Linux, mock lane) covers the confirmed stop end to end. It runs the real rocmd sandbox-tool stop_server against a live process recorded with its kernel start-time token. It asserts the reported status/stopped/pid_outcomes together with the resulting state: the process exited, the record reads stopped with no PIDs or pending stop, and the endpoint key file is gone. The unconfirmed half (timed_out / unverified) needs a process that survives SIGKILL or an unreadable /proc entry, which can't be staged from outside the binary. It is covered by rocmd unit tests that inject the per-PID termination outcome. The real Unverified path remains untested.

Follow-ups deliberately not in this PR

• Tie start-time tokens to a boot id so a manifest that survives a reboot can't falsely match (Process identity tokens aren't tied to a boot, so a manifest that survives a reboot can falsely match #548).
• A static guard that every production PID write goes through the identity helpers. The two rocm launch sites are covered today only through the shared helper they call; they launch a real engine, so removing the call there would still pass.
• run_mcp_server handles one request at a time, so a worst-case stop blocks it for its full duration.
• A stop that starts and finishes entirely inside a restart's spawn (between the write that clears the old PIDs and the one recording the new supervisor) finds no PIDs and confirms, so rocm services stop can clear the marker and delete the key while the service comes back. "No PIDs" can't simply mean "unconfirmed": a confirmed stop and a failed spawn both leave that record.
• Manifest writes are not atomic (truncate, then write), so a reader racing a writer can see an empty file; rocmd already has write_file_atomically. Relatedly, update_supervised_record's read and write are not synchronised, so a marker persisted strictly between them is reverted; closing that needs a lock or compare-and-swap over the manifest.
• rocm comfyui stop has the same defect; ComfyUiState stores only pid with no token, so it needs a state-schema addition.
• rocm_core can neither verify identity nor walk a process tree on Windows.
• MANAGED_STOP_GRACE is duplicated across rocm and rocmd with a comment asserting they match and nothing enforcing it; rocm-core is the shared home, since rocmd must not depend on rocm.

[jussielo-amd]

Code review

A few findings worth addressing before merge:

Blocking

1. apps/rocmd/src/lib.rs:2267 — The MCP stop_server tool reports success (isError:false) even when the stop result's stopped field is false (unconfirmed stop). This PR's stated goal is that an unconfirmed stop no longer claims success, but the one documented consumer of this change never checks stopped.

2. apps/rocmd/src/lib.rs:613 — Same issue via the sandboxed CLI path: stop_server hardcodes "status": "stopped" regardless of the actual outcome. The existing test (sandbox_tool_stop_server_updates_manifest_and_skips_current_pid) only exercises the trivially-true case, so it pins the literal string without exercising the false path.

Together these mean the identity-verified stop doesn't surface its result at either of its real call sites.

Worth confirming

3. apps/rocmd/src/lib.rs:506 — Verified stop can now take meaningfully longer per call (up to ~10–20s per PID with MANAGED_STOP_GRACE), approaching the fixed 60s timeout wrapping sandboxed rocmd sandbox-tool invocations. A slow stop risks being killed mid-write, leaving the manifest half-updated.

4. apps/rocmd/src/lib.rs:2940 — OS-level failures (e.g. EPERM) during the verified stop are no longer surfaced as hard errors — they fall through to a generic timed_out/unverified outcome. This may be intentional (mirrors apps/rocm's existing behavior) but is worth confirming explicitly.

Minor

5. apps/rocmd/src/lib.rs:2888 — PID de-duplication can mislabel the role field when supervisor/engine PIDs alias (field currently unused downstream, so low impact today).

6. apps/rocmd/src/lib.rs:3264 etc. — The supervisor/engine PID + start-ticks pairing is hand-duplicated across four call sites instead of a single setter on ManagedServiceRecord; a future call site could silently drop the start-ticks pairing this PR exists to protect.

[rominf]

Rebased onto 4ff6b821 (main had extracted rocmd persistence into persistence.rs in #477) and addressed the review round. In order:

1. MCP stop_server reported an unconfirmed stop as success — fixed in e7ffc3e9. The tool now reads the stop's own stopped verdict. An unconfirmed stop returns an error that says the recorded PIDs and the endpoint key were kept, and points at pid_outcomes. A report missing the verdict also counts as unconfirmed.
2. Sandbox stop_server hard-coded "status": "stopped" — fixed in the same commit, with the same root cause. It now reports stop_unconfirmed when the stop could not confirm. The new tests force an unconfirmed termination and assert the reply together with the manifest and key file it describes, not only the wording. Mutation-checked: if the verdict is forced to "confirmed", both new tests (mcp_stop_server_reports_an_unconfirmed_stop_as_an_error, sandbox_tool_stop_server_reports_an_unconfirmed_stop) fail.
3. Verified stop vs the 60 s sandbox timeout — no code change. Worst case is SIGTERM then SIGKILL, each waiting up to MANAGED_STOP_GRACE (10 s), per recorded PID: about 40 s for two PIDs, inside the 60 s budget. If the timeout were ever hit, the manifest is still written only after all terminations, so it stays as it was rather than half-updated.
4. EPERM no longer bails — intended, mirroring apps/rocm: a signal that couldn't be delivered leaves the process unconfirmed. With 1 and 2 fixed, that now reaches MCP and sandbox callers as an unconfirmed stop instead of a success, which closes the regression this item pointed at.
5. Role mislabelled when the supervisor and engine PIDs alias — fixed in 55ebbc1e. The shared pid_outcomes entry is now labelled supervisor_and_engine.
6. PID and start-ticks pairing duplicated at several sites — fixed in 58dc143d. One helper per role writes the PID and its token together. The only direct writes left clear both, or are in tests.

Verified locally: cargo test -p rocmd (139), cargo test -p xtask (255), cargo fmt --check, and cargo clippy -p rocmd --all-targets -- -D warnings with sources touched first.

[volen-silo]

I read this as a safety fix and went at it adversarially, since the failure mode it guards against is killing the wrong process.

The core mechanism holds up. Pairing each recorded PID with its own /proc start-time token and routing through terminate_verified genuinely closes the stale-PID window on Linux: the check-then-signal gap that remains is microseconds, against the unbounded staleness of a PID read from a file, and the parse is anchored on the last ) so the token can't be forged through a crafted comm. Deleting the ps-based helpers instead of leaving them dormant is the right call, and the mutation evidence for the capture sites in the description is the kind of thing I wish more PRs had.

Two things I'd want addressed before merge, both about what happens around the verified stop rather than inside it.

An unconfirmed stop is now something the daemon will undo. The status used to be written as stopped unconditionally, which kept the record out of find_recoverable_service's scan. It is now deliberately left at ready/running, which is exactly the state that scan looks for, and nothing in rocmd reads stop_requested_unix_ms — the only readers are in apps/rocm. So with automations on, the server-recover watcher (which defaults to acting, not observing) can restart the service the caller just asked to stop. Keeping the endpoint key, correct in itself, is also what lets that restart pass its own guard.

The capture discipline stops at rocmd's boundary. Two live production sites in apps/rocm still write engine_pid with no engine_start_ticks, and restart_managed_service in this file carries that half-filled pair forward while moving supervisor_pid. That yields precisely the record shape identity_state resolves through its legacy best-effort branch, so a stale engine PID still gets an unverified tree kill.

A few more worth a reply rather than necessarily a change, left inline: the token is ticks-since-boot with no boot identifier beside it, so a manifest surviving a reboot can still false-match (I don't think the "restarted host" case in the description is actually closed); the deferred key cleanup the new comments hand off to lives in a different binary, so a caller that only drives rocmd strands the key and the marker; the worst-case stop blocks far longer than before while holding a record it later overwrites wholesale; and I'd reword the "costs no safety" comment on the Windows path.

On the AGENTS.md scenario requirement — the reasoning given covers the refuted-PID case, which I agree isn't constructible end-to-end. But the confirmed-stop output changed too (status, plus the new stopped and pid_outcomes keys on rocmd sandbox-tool stop_server), and that half needs no identity injection at all. Worth either covering it or narrowing the justification to the part that genuinely can't be reached.

Things I checked that didn't hold up, for what it's worth: PID 0 is handled as a placeholder consistently everywhere I looked; the claim that nothing in the repo parses the result keys is accurate for rocmd's output specifically; and the new tests pass repeatedly with no port or temp-path collisions and no leaked helper processes on the happy paths.

[volen-silo]

This is the half I'd most want revisited.

Leaving the status untouched is right in isolation — the stop didn't happen, so don't claim it did. But find_recoverable_service only considers records whose status is ready or running, and an unconfirmed stop is exactly a record in that state whose endpoint has just stopped answering. endpoint_service_recovery_reason returns a reason, and handle_server_recover_event_with_record's RunContained arm calls restart_managed_service. server-recover is declared with default_mode: WatcherMode::Contained, so under rocmd run --automations-enabled acting is the default, not observing.

Before this change the unconditional status = "stopped" kept these records out of that scan entirely. Now the marker written on this line is the only thing that separates "the operator asked for a stop that couldn't be confirmed" from "the engine crashed" — and rocmd never reads it. Gating manifest_service_recovery_reason (or the ready/running arm of find_recoverable_service) on stop_requested_unix_ms would close it.

Unverified is the worse of the two unconfirmed outcomes: there the recorded process is live and may still hold the device, and the recovery would bring up a second engine on the same host and port. Keeping the endpoint key — correct, for the reason given below — is also what lets that restart satisfy ensure_public_service_has_endpoint_key.

[rominf]

Agreed, and confirmed against the code: an unconfirmed stop left a ready record that find_recoverable_service would recover under the default contained mode. Fixed in a964c169. A record with stop_requested_unix_ms set is never a recovery candidate. The scan skips it, and service_record_matches_recovery_event re-checks it, so an event queued before the stop can't act on it either. The marker is also now written before the stop signals anything, which covers the in-flight window, not only the unconfirmed outcome. Fresh launches and restart_managed_service clear it, as rocm services restart does. Tests: recovery_does_not_restart_a_service_whose_stop_is_unconfirmed, recovery_skips_a_failed_service_only_while_its_stop_request_stands, and the_stop_request_is_recorded_before_any_process_is_signalled. Removing the scan gate fails both recovery tests.

[volen-silo]

Worth looking at what this write is built from. record was loaded at the top of the function, and terminate_recorded_service_pids can now block for MANAGED_STOP_GRACE twice per PID — ten seconds waiting out SIGTERM, ten more after SIGKILL — for each of two recorded PIDs. ManagedServiceRecord::write is a whole-file fs::write with no merge and no lock, so anything another writer persisted during that window is silently discarded here. The old path was a kill, a 750 ms sleep and a second kill, so the window was about a second.

Taken together with the recovery path noted above, that has a concrete bad ending: the watcher restarts the service partway through a timed-out stop and writes fresh PIDs, then this line lands and clears them — leaving a live engine with no recorded PID at all, which no later stop can reach.

The duration matters for the MCP surface too. run_mcp_server is a single-threaded stdio loop, so a worst-case stop leaves it unable to answer even a ping for the whole time.

[rominf]

Fixed in a964c169. The stop re-reads the record after termination and applies only its own changes, so a write made meanwhile survives (a_stop_keeps_what_another_writer_persisted_while_it_ran). If the record names different processes by then, for example because a restart recorded fresh PIDs, those were never stopped. In that case the stop reports stopped: false and leaves their PIDs and the key in place, so a later stop can still reach them (a_stop_does_not_clear_processes_recorded_while_it_ran). With the marker written up front, recovery no longer restarts a service mid-stop. On MCP: yes, run_mcp_server is a single-threaded stdio loop, so a worst-case stop blocks it, including ping, for that long. I've left that unchanged, because moving the stop off the loop changes the tool's response contract, and listed it as a follow-up.

[volen-silo]

Pairing each PID with its own token is the right shape, but it only buys anything when something actually wrote engine_start_ticks — and two live production paths in apps/rocm/src/main.rs still don't. spawn_managed_engine_child (reached by rocm serve <model> --background) sets both supervisor_pid and engine_pid to the same child PID and then captures only supervisor_start_ticks; restart_internal_managed_service does the same, under a comment about refreshing the token "in lockstep" that in fact covers only the supervisor.

Both are harmless in isolation, because the two PIDs are equal and the dedup just above collapses them onto the supervisor's token. They stop being harmless as soon as the PIDs diverge — which restart_managed_service in this file does; see my note there. What's left is an entry with a PID and no token, which identity_state resolves through its (None, _) => Matches legacy branch straight into a KillScope::Tree forced kill of whatever holds that number now.

Nothing repairs it in between, either: rocmd never calls refresh_from_engine_state, so the missing token is still missing at stop time. For the guarantee this PR is making to hold for every record rocmd can be asked to stop, I think the capture discipline has to reach those two sites.

[rominf]

Agreed, fixed in 5598dffd. Both spawn_managed_engine_child and restart_internal_managed_service now record each role with its own token. The pairing moved into ManagedServiceRecord::record_supervisor_identity / record_engine_identity in rocm-core, and every production PID writer in rocm and rocmd goes through it, so rocmd's private copies are gone. The one other writer, refresh_from_engine_state, already adopts the engine PID with its own token. The helper is covered by recording_a_process_identity_writes_the_pid_with_its_own_token. The two rocm call sites themselves aren't driven by any test, because they launch a real engine, so removing a call there would still pass. A static source guard would need its own literal-aware scanner, so it's listed in the description as follow-up work.

[volen-silo]

This moves supervisor_pid onto the new child and refreshes its token, but engine_pid and engine_start_ticks are left exactly as loaded, and reset_for_restart doesn't touch them either.

For a record rocmd wrote itself that's fine — the stale engine PID still carries its old token, so a later stop refutes it and reports identity_mismatch. It is not fine for a record that arrived from rocm serve --background, where engine_pid is set and engine_start_ticks is None. Up to this point the two PIDs are equal and the dedup hides the gap; after this line they differ, and the next stop builds a second, unverifiable entry for the dead engine's PID.

Either clearing engine_pid/engine_start_ticks here, or recording them together the way the supervisor now is, would leave a record the stop can actually reason about.

[rominf]

Fixed in a964c169. restart_managed_service now clears engine_pid and engine_start_ticks; the new supervisor records its own engine with a token. Capturing a token for the old PID at that point would bind whatever process holds that number now, which is the hazard itself. It also clears a standing stop request, matching rocm services restart. Test: restart_managed_service_drops_the_replaced_runs_engine_pid.

[volen-silo]

The token is starttime from /proc/<pid>/stat, which counts clock ticks since boot, and nothing is stored beside it to say which boot. A manifest that outlives a reboot therefore keeps a PID and a tick count that are both entirely plausible for an unrelated process on the next boot, and identity_state answers Matches. PIDs are handed out in roughly the same order each boot and the tick granularity is coarse, so for a service that comes up early in a session this isn't a remote possibility.

The description lists "a restarted host" among the stale-manifest cases this closes, and as far as I can tell it doesn't. apps/rocm/src/main.rs already reads /proc/sys/kernel/random/boot_id in current_boot_id for the driver-reboot check, so the anchor exists in-tree.

This is a ProcessIdentity change in rocm-core rather than anything in these two files, so I'd be content with the description being corrected and a follow-up noted alongside the others — I just don't think it should be listed as fixed.

[rominf]

You're right. The token counts ticks since boot with no boot id beside it, so a manifest that survives a reboot can falsely match, and #498 lists reboot as a route this PR doesn't close. I've corrected the description to say so and opened #548 for the rocm-core ProcessIdentity change: record boot_id, and treat a mismatch as refuted.

[volen-silo]

The comment above says the liveness refresh drops the key once the processes are observed gone. That's true, but it happens in a different binary: settle_pending_stop_key_cleanup and refresh_managed_service_runtime_liveness both live in apps/rocm. A caller that only drives rocmd — which is the MCP and sandbox-tool surface this PR is hardening, and the one an agent reaches — never runs either, so an unconfirmed stop leaves the 0600 key and the marker on disk with nothing that will ever clear them.

Also worth noting for whoever reads this later: settle_pending_stop_key_cleanup decides "the processes are gone" with a bare process_is_running over the recorded PIDs, with no identity check. It errs safe here — a recycled PID just keeps the key alive rather than dropping it early — but it does mean the deferred path this comment hands off to is the unverified check the rest of this PR exists to replace.

[rominf]

Right on both counts. Reworded in a964c169: the comment now says the cleanup runs in the rocm binary's liveness refresh, not in rocmd. A caller that only uses rocmd completes it by repeating the stop. Once the processes are gone, the repeat confirms, clears the marker and drops the key, and the MCP error text already tells the caller that its PIDs and key were kept for exactly that. A rocmd-side background settle would add another whole-record writer racing the recovery scan, with the lost-write risk you raised above, so I've left it out. Agreed that settle_pending_stop_key_cleanup's bare liveness check fails safe.

[volen-silo]

I'd reword "it costs no safety". It's accurate about the delta — Windows behaves the way it always has — but a reader arriving from the PR title will read it as "Windows is covered too", and it isn't. process_start_ticks is None there, so the gate above can only ever turn away a PID that isn't running; a recorded PID that has been recycled still gets taskkill /T /F, taking the unrelated process and its entire subtree. That is the headline defect, intact, on one of the two supported platforms.

The PR description says this plainly, which I appreciate. The code comment is what survives into the tree, so I'd rather it said the same thing directly: identity verification does not apply on Windows, and the recycled-PID hazard there is unchanged.

[rominf]

Reworded in a964c169. The comment now says plainly that identity verification does not apply on Windows, and that a recycled PID there still gets taskkill /T /F, taking an unrelated process and its subtree.

[volen-silo]

Two smaller notes on this seam, neither blocking.

Because the override sits ahead of everything else in the function, the tests that set it exercise neither the identity check nor terminate_verified — they cover the reporting layer only. That's a legitimate thing to test and the surrounding doc comment is honest about the constraint, but it's worth being explicit that the mocked outcome stands in for the entire stop, not just its result. It also leaves the real Unverified path with no coverage at all, which is the one outcome where a live process is deliberately left alone.

Second, production code referring into tests:: is an inversion even behind #[cfg(test)]. Threading the outcome source in as a parameter, or splitting the two behaviours into separate cfg'd bodies, would give the same seam without the production path naming the test module.

[rominf]

Both addressed in a964c169. The termination function is now a parameter (stop_managed_service_with), and production passes terminate_recorded_pid without naming the test module. The tool reply builders (sandbox_stop_server_value, mcp_stop_server_reply) are split out, so the injected tests feed them while the confirmed-stop tests drive both full handlers. The helper's doc now says the stand-in replaces the identity check and the signalling. The real Unverified path is still uncovered: it needs a live PID whose /proc/<pid>/stat can't be read, which a test can't stage.

[rominf]

Thanks for the adversarial pass. All of it held up against the code. Four commits:

• a964c169 — recovery never restarts a service with a standing stop request, and the request is recorded before the stop signals anything. The stop re-reads the record and applies only its own changes, and reports itself unconfirmed if a restart recorded new processes in the meantime. The recovery restart drops the replaced run's engine PID. Termination is injected as a parameter, and the Windows and deferred-cleanup comments are corrected.
• 5598dffd — rocm serve --background and rocm services restart now record the engine with its own token. The PID-plus-token pairing lives in rocm-core and both binaries share it.
• 8abff8c8 — the scenario. You're right that the confirmed half needs no injection. service-stop-01 (features/managed_service_stop.feature, Linux, mock lane) runs the real rocmd sandbox-tool stop_server against a live process recorded with its real start-time token. It asserts the report (status: stopped, stopped: true, the PID's pid_outcomes entry) together with the resulting state: the process exited, the record reads stopped with no PIDs or pending stop, and the endpoint key is gone. The description's justification is narrowed to the unconfirmed half, which can't be staged from outside the binary.
• ae9431ed — clippy.

Every new guard was mutation-checked. Still open, and stated in the description:

• boot identity, now Process identity tokens aren't tied to a boot, so a manifest that survives a reboot can falsely match #548;
• a static guard for the two rocm launch sites;
• the MCP loop blocking during a worst-case stop;
• one narrower lost-write path: supervise_service rewrites its own in-memory record, so a supervisor that survives an unconfirmed stop could drop the stop marker when its engine later exits.

Dismissed as stale: this review is of 58dc143. Every finding is addressed, with a reply in each thread and a summary comment. Recovery no longer undoes a stop, and a concurrent writer's changes survive a stop (a964c16). Engine tokens are recorded at the two rocm launch sites (5598dff). The confirmed-stop scenario is added (8abff8c). Boot identity is tracked in #548, and the remaining gaps are stated in the description. Please re-review the current head.

[volen-silo]

I went back over this against the code rather than against the replies, and both of the fix-before-merge points from last round are genuinely closed.

The recovery half holds. The stop writes stop_requested_unix_ms before it signals anything, and stop_requested is checked in both places that can lead to restart_managed_service — the scan in find_recoverable_service and the queued-event path in service_record_matches_recovery_event. I traced every caller of restart_managed_service: there are two, the explicit restart_server tool and the recovery handler, and the handler is unreachable without passing one of those gates. I also mutation-checked the guard rather than reading it — removing both checks leaves the suite failing exactly recovery_does_not_restart_a_service_whose_stop_is_unconfirmed and recovery_skips_a_failed_service_only_while_its_stop_request_stands, so it is load-bearing. Dropping engine_pid/engine_start_ticks in the recovery restart is right too, and the test pins it.

The capture half holds as well. Moving the pairing into record_supervisor_identity / record_engine_identity is the right home for it. I swept every production assignment to the four fields across the workspace: ManagedServiceRecord::new always zeroes both tokens and is always immediately followed by a helper call, the three bare resets clear a PID and its token together, and nothing else writes them. The two apps/rocm launch sites are fixed, and recording the engine role there while the two PIDs still coincide does not disturb the dedup — it just makes the prefer-a-verifiable-token branch redundant instead of load-bearing.

The smaller points landed as described. The Windows comment now says plainly that the recycled-PID hazard is unchanged there; the deferred-cleanup comment names the right binary and tells a rocmd-only caller to repeat the stop; the test seam is a parameter rather than production code reaching into tests::; and the description no longer claims the reboot case. Re-reading the record and applying only the stop's own changes is a real improvement over writing back a forty-second-old copy, and having a mid-stop restart make the stop report itself unconfirmed is a good call. service-stop-01 driving the real tool against a real process with its real token is worth more than the injected tests on their own. Locally: cargo test -p rocmd --all-targets 145 passed, clippy and fmt clean.

I also went looking for the inverse failure — a service that is legitimately running failing its own identity check and becoming unstoppable — and could not find one. A live process's start-time does not change, parse_start_ticks counts from the last ) so a command name with spaces or parentheses cannot shift the field, the zombie case is handled explicitly, and an unreadable /proc entry lands on Unverified, which leaves the process alone and reports the stop unconfirmed rather than claiming it. The one way the token could have gone stale under a live engine — a restart moving the supervisor PID while an old engine token stayed behind — is what you fixed.

Three things I would still want before this merges, inline below, plus three smaller notes.

The one that matters most: the mark-before-signal discipline is installed on only one of the two stops. rocm services stop terminates the same records and still writes the marker after terminate_recorded_service_pids returns, so for that whole window the manifest reads ready with no marker while the endpoint is already down — and the watcher ticks every five seconds. That is the same defect this PR just closed, on the sibling path.

Second: supervise_service is the one writer left that rewrites the whole record from a copy made before any stop could have happened, so it can erase the marker. You have this listed as a follow-up; I think it belongs here, because the fix is the discipline you just applied two functions away and because the case it needs is the Unverified one this PR exists for.

Third: refresh_from_engine_state does not always adopt the engine PID with its own token. That one predates this PR, but it is the one claim in your reply I could not confirm, and it produces exactly the PID-without-a-token shape the rest of this work removes.

[volen-silo]

This comment now asserts that rocm services stop already keeps the same contract. It keeps the same final state, but not the in-flight one, and the in-flight one is what the marker above was added for.

stop_internal_managed_service in apps/rocm/src/main.rs calls terminate_recorded_service_pids at line 18176 and only sets stop_requested_unix_ms afterwards, in the branch at 18182-18191, persisted at 18192. It also sends the engine's own EngineMethod::Stop first, so the endpoint is typically down before the termination even begins. For the whole of that window — up to MANAGED_STOP_GRACE twice per recorded PID, so roughly forty seconds for two — the record on disk still reads ready/running with no marker.

Meanwhile find_recoverable_service does not need a stale status to act: for a ready/running record it probes the port, and endpoint_service_recovery_reason returns endpoint_status_unreachable immediately with no staleness timer. WATCHER_TICK_INTERVAL is 5s, so the daemon will scan several times inside that window, and server-recover is Contained by default. The queued-event re-check does not help either: it reads the same not-yet-written marker.

So with rocmd run --automations-enabled up, rocm services stop on a managed service can still be undone by the daemon while it is running — not only in the unconfirmed case, but on a perfectly ordinary successful stop that simply takes a few seconds. Moving the marker write ahead of line 18176, the way you did here at 2768-2777, would close it and make the comment true.

[rominf]

Fixed in 675ccff. rocm services stop now writes and saves stop_requested_unix_ms before the engine Stop and before any PID is signalled, the same order as rocmd's stop. Status, PID and key changes still happen only on a confirmed stop.

stop_records_its_marker_on_disk_before_any_process_is_signalled reads the manifest from disk at the moment termination runs. With the marker write moved back after signalling, it fails. The rocmd comment now states only what the two stops actually share: marker order and confirmed-stop gating. Clearing PIDs is rocmd-only.

[volen-silo]

This is the writer that can still drop the marker, and I think it is worth closing here rather than carrying forward.

record is built by ManagedServiceRecord::new at 3348, which sets stop_requested_unix_ms: None, and it is never reloaded from disk afterwards. Every later record.write() therefore persists None for that field: the startup-phase write at 3468, the ready write at 3475, and — the one that matters — the write at 3484 after child.wait(), which sets status to failed on a non-zero exit.

failed is a manifest_service_recovery_reason that needs no endpoint probe, so that single write both clears the guard and makes the record an immediate recovery candidate. The sequence is reachable in exactly the case this PR is about: the supervisor comes back Unverified (live, /proc unreadable, so nothing is signalled) while the engine, a separate entry with its own token, verifies and is killed. The stop correctly reports stopped: false and leaves the marker; the surviving supervisor then observes its engine exit, writes failed with stop_requested_unix_ms: None, and the daemon restarts the service the operator asked to stop. TimedOut on the supervisor with the engine confirmed gone gets there the same way.

The fix is the discipline you just added to the stop: re-read before writing and apply only this function's own fields, rather than persisting a snapshot taken before anything could have asked for a stop. The two long-lived writers over these records are this one and the stop, and only one of them does it.

[rominf]

Fixed in 40a4186. Every supervise_service write after the first now goes through update_supervised_record. It re-reads the record, changes only the supervisor's own fields and never touches the marker. It writes nothing if the record was removed, so a deleted service is not brought back, or if the record names a different supervisor, i.e. a confirmed stop cleared the PIDs or a restart took over. The first write also keeps a marker already on disk.

Tests:

• supervisor_exit_write_keeps_a_standing_stop_marker, which also checks that find_recoverable_service returns nothing
• supervisor_exit_write_leaves_a_record_it_no_longer_owns
• supervise_service_first_write_keeps_a_standing_stop_marker

Reverting to the snapshot write fails them. The restart's own failed write had the same bug; that is fixed in 6eca5d3 (see the thread on the restart). This item is off the description's follow-up list.

[volen-silo]

The helpers are the right shape, and the sweep agrees with you for every site that assigns these fields directly. The one claim I could not confirm is the other writer: refresh_from_engine_state does not always adopt the engine PID with its own token.

Its key pairing (lib.rs:7905-7923) prefers server_pid, and then reads the ticks from server_start_ticks. engines/lemonade/src/state.rs:151-170 writes both keys, so lemonade is fine. engines/vllm/src/state.rs:119-149 writes pid, server_pid (the same value) and start_ticks, but never server_start_ticks — so for every vllm service the branch picks server_pid, finds no matching ticks key, and sets engine_pid = Some(pid) with engine_start_ticks = None, discarding the perfectly good start_ticks sitting next to it in the same file for the same PID.

That is persisted, not just transient: load_managed_service in apps/rocm/src/main.rs:18011 calls it and writes the result back at 18013-18015, and it is on the rocm services stop path.

It is masked today only by the dedup preferring a verifiable token, and only while supervisor_pid == engine_pid. It is not masked for a record rocmd's supervise_service wrote, where the supervisor is rocmd itself and the engine is its child: the two entries do not merge, so the engine entry goes through identity_state's (None, _) => Matches legacy branch into the forced KillScope::Tree kill this PR exists to prevent — against a PID the refresh had the token for and threw away.

This predates the PR and sits in a third file, so I am not asking you to take vllm's state writer on here. But adding server_start_ticks beside server_pid is a one-line change, and if it stays out, the capture claim in the description and in these doc comments should say that refresh_from_engine_state is the exception rather than an example.

[rominf]

Fixed at both layers:

• 77768b0 makes vLLM's state writer record server_start_ticks alongside start_ticks.
• 588aa2c makes refresh_from_engine_state fall back to start_ticks only when server_pid == pid, so a distinct server PID never borrows the launcher's token.

Each has a test that fails without it: running_state_records_managed_therock_env_for_gpu_verification now uses a live PID so the token is real; the other two are engine_refresh_takes_start_ticks_when_server_pid_is_the_same_process and engine_refresh_never_gives_a_server_pid_the_launchers_token. Lemonade already wrote both keys together.

[volen-silo]

Non-blocking. Both of the fallible steps between the marker write at 2777 and the final write at 2849 — this re-read, and record.write() itself — propagate with ?, and nothing unwinds the marker on the way out. A manifest that becomes unreadable or a record removed mid-stop therefore leaves stop_requested_unix_ms set with the status untouched, which under the new gate means the daemon will never recover that service again until someone runs a stop or a restart that completes.

The window is small and the failure mode is fail-closed rather than fail-dangerous, so I would not hold the PR for it, but it is worth being deliberate about given the marker is now load-bearing for recovery rather than just for key cleanup.

[rominf]

Leaving this one as is, deliberately. If a stop fails partway, the marker stays set, so recovery leaves the service alone and the key cleanup runs once its processes are seen gone. Clearing it on error would hand a half-stopped service back to server-recover, which is the failure this PR exists to close. Repeating the stop clears it once termination is confirmed.

[volen-silo]

Non-blocking, and pre-existing in shape — but it is adjacent enough to this PR's subject to be worth writing down while you are here.

This function persists the daemon's own PID as supervisor_pid (line 5170, now with a valid token) and writes it at 5182, before the spawn at 5184-5191. If that spawn fails the ? carries the error out and the manifest is left naming the running daemon as the service's supervisor.

The pid == std::process::id() guard in terminate_recorded_service_pids only protects a stop running inside that same process. A rocmd sandbox-tool stop_server invocation is a separate process, so it would verify that identity as Matches and take the daemon and its whole tree. Recording the child's PID first and only persisting once the spawn succeeded, or clearing the field on the spawn error, would avoid leaving that on disk.

[rominf]

Fixed in 6eca5d3. The restart now records no supervisor (PID 0, no token) until the spawn succeeds, so a failed spawn can no longer leave the daemon recorded as the supervisor of a service another process could stop.

The same commit routes the restart's failed write (when the spawned supervisor exits at once) through the re-reading helper, so a stop issued during its 200 ms check survives. restart_failure_write_keeps_a_stop_marker_set_meanwhile covers that. The spawn-failure path itself has no test; covering it needs an injectable spawn.

One window is still open: the restart's write straight after a successful spawn still persists its own copy. I have added it to the description's follow-ups.

[volen-silo]

Non-blocking, just naming a consequence so it is a decision rather than a side effect. When the second clause here is what makes all_stopped false, the else branch below stamps stop_requested_unix_ms onto processes this stop never touched — which, in the case the comment describes, are the ones a restart just brought up. a_stop_does_not_clear_processes_recorded_while_it_ran asserts exactly that, so it is deliberate.

The effect under the new gate is that the freshly restarted service is excluded from recovery until someone repeats the stop and it confirms. That is defensible — somebody did ask for a stop — but it is a different outcome from "this stop simply did not apply to these processes", and it is now reachable from a restart the daemon performed on its own rather than from anything the operator did.

[rominf]

Deliberate, and pinned by a_stop_does_not_clear_processes_recorded_while_it_ran. A stop that overlaps a restart neither claims the new processes stopped nor clears their PIDs, but its marker stands, since somebody asked for a stop after the restart began. Repeating the stop confirms and clears it. With 40a4186, the supervisor's own writes now preserve that marker rather than silently undoing it.

[rominf]

Rebased onto main at 1845df34 (now 1c9703f6), the same commits as 4dd744eb. Main's #479 moved optional_arg back next to supervise_service, which was this branch's one conflict. Both are kept: optional_arg plus the new update_supervised_record/record_supervised_exit. No other content changed.

Re-ran on the rebased head, all clean:

• rocmd (150 passed), rocm-core --lib (577), rocm-engine-vllm (92), rocm --bin rocm (1010)
• workspace clippy -D warnings, fmt, check-crate-edges and manifest --check

[volen-silo]

Third pass, against 1c9703f6. All three of the things I asked for last round are closed, and I checked each by tracing and by breaking it rather than by reading the replies.

• rocm services stop now persists the marker before the engine Stop and before any PID is signalled. Moving the write back after termination makes stop_records_its_marker_on_disk_before_any_process_is_signalled fail, so the ordering is pinned rather than incidental. Both stop paths also skip a zero PID, which matters more now that a restart writes one.
• supervise_service no longer writes back its startup snapshot. All five writes after the first merge into the record as it is on disk, the first write carries over a marker already there, and the restart's failed write does the same. Reverting record_supervised_exit to a snapshot write fails the two tests that target it and leaves the other two green, which is the right shape for that coverage.
• The token-less engine entry is closed at both layers. I swept every production site that assigns engine_pid or supervisor_pid across both binaries, rocm-core and both engines; each one pairs the PID with a token captured for that same PID. I could not construct a path that reaches the legacy (None, _) branch with a live engine PID on Linux. The fallback also correctly refuses to lend the launcher's token to a distinct server PID, which is the way this particular fix could have gone wrong.

I re-ran the unstoppable-service question from scratch too, since a guard like this fails worse in that direction. Upgrading the binary in place, renaming the process, restarting from a different path, a zombie, and a pre-upgrade record with no tokens at all all still stop. The start-time parser takes the last ) before counting fields, so a process name containing spaces or parens doesn't shift the offset.

Two concerns I had going in didn't survive checking and aren't worth comments: the ownership check in update_supervised_record silently dropping the supervisor's engine-PID write (both writers compute the identity for the same real PID, so they agree on it however they interleave), and the carried-over stop marker landing on a freshly started service (rocm serve mints a new service id each time, so there is no prior record to carry one from).

On the scope question, since seventeen commits looks like a lot for three points: eleven of them are the previous round rebased with one adaptation for the common.rs extraction on main, and the other six map one-to-one onto the three blockers, one of the non-blocking notes, and a docs update. The PR is still the same eleven files. I don't think it wants splitting.

cargo fmt --all --check, cargo clippy --locked --workspace --all-targets -- -D warnings and cargo test -p rocmd --all-targets (150 passed) are all clean here.

Nothing below is something I'd hold the merge for. I'm requesting changes to keep the thread open rather than because I think this isn't mergeable — the only one I'd really like a decision on is the apps/rocm snapshot write, and even that is pre-existing.

[volen-silo]

Clearing the supervisor here is the right fix for the tree-kill-the-daemon case and I'd keep it. It does change the shape of the window you've listed as a follow-up, though, and I think the follow-up text understates it.

Between this write and the one straight after the spawn (lines 5036-5037), the record on disk names no process at all: supervisor_pid is 0 and engine_pid is None. Before this commit it named the daemon, which was dangerous for a different reason but was at least a live PID.

stop_managed_service_with survives that window: its re-read sees different processes, records_name_same_processes returns false, and it re-stamps the marker. stop_internal_managed_service_with in apps/rocm/src/main.rs does not. It builds the PID list from the record it loaded, all_stopped starts at true in terminate_recorded_service_pids, and an empty list leaves it true — so a stop landing there reports a confirmed stop, clears the marker it just wrote, and deletes the 0600 endpoint key, while the supervisor being spawned here brings the service back up.

It is a few milliseconds wide and only during a restart the daemon started on its own, so I don't think it blocks anything. But if the follow-up is going to describe it, "can lose its marker" is the rocmd half; the rocm half is a false confirmed stop and a destroyed key. Routing the write at 5037 through update_supervised_record — passing the record before record_supervisor_identity reassigns it, so the guard compares 0 against 0 — closes the marker half and is about three lines.

[rominf]

Fixed in 0c37943. The write after the spawn now goes through update_supervised_record, via the new record_spawned_supervisor. It applies only the supervisor PID and token to the record as it is on disk, and checks ownership against the copy from before the assignment, so it requires the cleared PID (0) to still be there. A stop marker persisted in the window survives. If the new supervisor already recorded itself, or the record was removed, nothing is written. restart_records_its_supervisor_without_erasing_a_stop_marker and restart_leaves_a_record_naming_another_supervisor_or_none both fail against the old write-back.

On the rocm side, cc46556 makes its stop re-read and compare processes before confirming (see the other thread). A restart that records its supervisor while the stop runs now gives an unconfirmed stop, with the marker re-stamped and the key kept.

Still open: a stop that runs entirely between the restart's PID-clearing write and the spawn's identity write, i.e. one fork/exec, finds no PIDs and confirms. I did not make "no PIDs" mean "unconfirmed". A confirmed stop leaves exactly that record, and so does a restart whose spawn fails, so stops of those would never confirm. It is listed in the description's follow-ups.

[volen-silo]

This is the half of the parity that the comment in stop_managed_service_with is careful not to claim, and it's the one item left that I'd actually like a decision on.

record was loaded at the top of this function and is written back whole here. terminate_recorded_service_pids calls terminate_verified with MANAGED_STOP_GRACE and force, so it can block for that grace twice per PID — the same roughly forty seconds for two PIDs I raised against rocmd's stop last round, and which you closed there by re-reading before this write and applying only the stop's own fields. Here the snapshot still wins: anything another writer persisted during the wait is discarded, and a restart that recorded fresh PIDs in that window has them replaced by the stale ones — leaving a live engine with no recorded PID, which no later stop can reach.

It is pre-existing in rocm, so I'm not saying the PR caused it. But the PR is already editing this function to fix the ordering half, and the fix is the discipline you wrote two files away: re-read, apply only this stop's fields, and gate the confirmation on records_name_same_processes. If it stays out, I'd rather the follow-up list named it — the only rocm-side entry there at the moment is the non-atomic write.

[rominf]

Fixed in cc46556. stop_internal_managed_service_with now re-reads the manifest as-is before its final write and applies only its own fields. It skips the liveness refresh on that read, because the refresh would act on this stop's own marker before the verdict.

It confirms (status stopped, marker cleared, key deleted) only when the re-read record names the same processes it handled. Otherwise it re-stamps the marker and keeps the key, matching rocmd. The comparison moved into rocm-core as ManagedServiceRecord::names_same_processes, which both stops now use.

Tests: stop_does_not_confirm_when_a_restart_recorded_new_processes_meanwhile and stop_final_write_keeps_what_others_wrote_while_it_ran. Each fails when the gate, the re-stamp or the re-read is removed. I re-checked the gate myself: 13 passed, 1 failed with it removed.

[volen-silo]

Worth being explicit about what this narrows and what it leaves. The guard makes the write apply to the record as it is on disk now, which removes the real problem — a supervisor persisting a snapshot taken minutes earlier. What's left is that the read above and the write below aren't synchronised, so a marker persisted strictly between them is still reverted. restart_failure_write_keeps_a_stop_marker_set_meanwhile pins the ordering where the stop's write lands before the read; the mirrored one isn't covered by anything.

Going from the supervisor's whole lifetime down to the gap between one read and one write is the bulk of the win, and closing the remainder needs a lock or a compare-and-swap rather than a tidier merge, so this is a note rather than a request. It does belong beside the atomicity follow-up, though: rocmd already has write_file_atomically with its own tests, while ManagedServiceRecord::write is still a truncating fs::write — so the helper that follow-up wants already exists in the tree.

[rominf]

Agreed. update_supervised_record's read and write are not synchronised, so a marker persisted strictly between them is reverted. The window is now one read-modify-write rather than a whole supervisor lifetime, and closing it needs a lock or a compare-and-swap over the manifest. That is not in this PR. It is in the follow-ups beside the atomic-write item, with a note that rocmd already has write_file_atomically.

[volen-silo]

Small one: this assignment no longer does anything. The closure below sets the field that actually gets persisted, and both callers (lines 580 and 4826) propagate the bail! with ? without reading the record again. Leaving it here reads as though it is the write, which is the exact misreading the comment underneath exists to prevent.

[rominf]

Removed in 0c37943. The persisted status is set inside the update_supervised_record closure, and both callers propagate the bail! without reading record again, so the in-memory assignment did nothing and read as if it were the write.

[volen-silo]

Not a request — I'd just like this written down somewhere, because it is the price of the guard and it's the behaviour a user is most likely to meet.

Unverified is reached whenever /proc/<pid>/stat can't be read while kill(pid, 0) still answers: another user's process, a hidepid mount, a restrictive LSM. terminate_verified returns before it consults force, so nothing is signalled and there is no flag that gets past it. The service stays live, keeps its key and keeps its marker, and repeating the stop only helps if the unreadability was transient. If it's a permission boundary, the service cannot be stopped from the CLI at all — only by someone who can read that process.

Nothing in this repo deploys across uids today, so I don't think it's reachable in practice, and it lives in proc_lifecycle.rs rather than in anything you've touched. But "there is no way to force past the identity gate" is a deliberate choice and the code doesn't say so anywhere.

[rominf]

Documented in ba44b39, at the stop's verdict. When /proc/<pid>/stat is unreadable while kill(pid, 0) still answers (another uid, hidepid, an LSM), the result is Unverified. terminate_verified returns that before it looks at force, so such a service stays unconfirmed and cannot be stopped from the CLI. That is deliberate: signalling a process nobody could identify is the blind kill the identity check exists to prevent.

[rominf]

Pushed b78aa466. It fixes the required windows-build-and-test failure: in 57783d3f, identity_probe_record was gated to Linux but used by tests that run on every platform. It only builds a record, so the gate is gone. Checked with an x86_64-pc-windows-gnu cross-check of rocmd, rocm, rocm-core and rocm-engine-vllm --all-targets; putting the gate back reproduces the CI error.

The round-4 items are in cc46556b, ba44b39a and 0c37943b (replies inline).

Re-ran locally, all clean:

• rocmd (152 passed), rocm-core --lib (577), rocm-engine-vllm (92) and rocm --bin rocm (1013)
• workspace clippy, the e2e-cucumber --test e2e clippy, fmt and check-crate-edges

Stale (left on ae9431e); addressed. rocm services stop persists the marker before signalling: 6a0fd71. supervise_service writes re-read the record: 291f75f. vLLM records server_start_ticks and the refresh falls back on the same PID: 435509a, be08ccf. A restart never records the daemon: 834faf1. Docs: 1c9703f. Replies are in the threads.

[rominf]

Rebased onto b863f77a to pick up #483 (the rocmd module extraction). Now 22 commits ahead, 0 behind, no conflict markers. Nothing about the fixes themselves changed, but two parts of the rebase are invisible in the diff of the commits that introduced them, so they are worth calling out.

#483 moved the call sites, not the definitions. stop_managed_service, supervise_service, restart_managed_service, find_recoverable_service and terminate_recorded_service_pids all stayed in apps/rocmd/src/lib.rs, so the substance applied there unchanged. What moved was the SandboxToolArg dispatch (to sandbox.rs), the supervise dispatch (to cli.rs), and ~730 lines of sandbox tests out of lib.rs's mod tests.

Two things needed hand-work:

1. sandbox-tool stop_server's reply moved to sandbox.rs. This PR's change to that arm — reporting stop_unconfirmed instead of an unconditional "status": "stopped" — had to be re-applied at apps/rocmd/src/sandbox.rs, which now calls the shared sandbox_stop_server_value builder from the crate root. Left behind, lib.rs would still have held a correct-looking builder that nothing called, and the suite would have stayed green: sandbox_tool_stop_server_updates_manifest_and_skips_current_pid drives the real tool but only exercises the confirmed branch, and sandbox_tool_stop_server_reports_an_unconfirmed_stop exercises the builder without the tool. The wiring is pinned by mutation instead — breaking the builder's confirmed branch fails the end-to-end sandbox test, which is only possible if sandbox.rs routes through it.
2. optional_arg is no longer defined here. ROCMAI-83: common.rs follow-up, webhook.rs, cli.rs and sandbox.rs extraction from apps/rocmd/src/lib.rs #483 moved it to common.rs as pub(crate); the rebase would otherwise have reintroduced a second copy in lib.rs.

Also dropped from lib.rs during conflict resolution: the sandbox tests #483 relocated to sandbox.rs, which appeared on the "theirs" side of four conflicts only because this branch predates the extraction. Verified by function-set diff against b863f77a that nothing of main's is missing from apps/rocmd, crates/rocm-core, apps/rocm, engines/vllm or tests/e2e-cucumber — the only absences are the seven unverified-kill helpers and the one ps-parsing test this PR removes on purpose.

Both ordering guarantees were re-checked by mutation after the move, not merely by a green suite: writing the stop marker after signalling fails stop_records_its_marker_on_disk_before_any_process_is_signalled, and reverting record_supervised_exit to a snapshot write fails supervisor_exit_write_keeps_a_standing_stop_marker.

Local gates green: fmt; rocmd 152; rocm-core --lib 585; rocm-engine-vllm 92; rocm --bin rocm 1016; xtask 271; e2e-cucumber --lib 136; feature_naming 4; cargo check --test e2e; check-crate-edges; both clippy invocations. cargo xtask coverage and the GPU/Windows lanes were not run locally.

[rominf]

Rebased onto 28f9ff7d (now 6cb72b55). Main's #487 moved most of apps/rocmd/src/lib.rs into service.rs, watchers.rs and mcp.rs. Unlike #483, it moved the definitions this PR edits, so every rocmd change now sits in the module that holds its code. Behaviour is unchanged; only placement and visibility differ.

• service.rs: the stop (stop_managed_service_with, identity-verified termination, pid_outcomes/stopped, the marker written before signalling), the supervisor writes (update_supervised_record, record_supervised_exit), and sandbox_stop_server_value.
• watchers.rs: the recovery skip (stop_requested in find_recoverable_service and service_record_matches_recovery_event) and the restart path (record_spawned_supervisor, dropping the replaced run's engine pid and stop request).
• mcp.rs: mcp_stop_server_reply, beside tool_success/tool_error.
• Tests moved with the code they test. The shared fixtures (identity_probe_record, seed_keyed_service, stop_with_outcome, assert_unconfirmed_stop_kept_the_service, UNCONFIRMED_STOP_PID) are in test_support.rs. A few items became pub(crate) because their callers are now in another module.
• Removed: the ps-based helpers (descendant_pids_*, force_terminate_remaining_processes), which ROCMAI-83: extract mcp.rs, service.rs, and watchers.rs from apps/rocmd/src/lib.rs #487 had carried into service.rs, are removed again. None is defined anywhere in apps/rocmd.

Verified after the move rather than inferred from a clean build:

• All 29 tests this PR adds are present.
• The function-set diff against 28f9ff7d shows nothing of main's missing except the helpers this PR deletes on purpose.
• Four mutations are each caught by the moved code: the marker written after signalling, a snapshot exit write, the recovery skip removed, and a wrong sandbox status. Removing the recovery skip alone fails three tests across watchers.rs and service.rs.

Local gates green:

• fmt
• tests: rocmd 152, rocm-core 586, rocm --bin rocm 1016, feature_naming
• check-crate-edges, both clippy invocations
• a Windows (x86_64-pc-windows-gnu) check of rocmd and rocm, all targets

One unrelated flake seen locally: restarting_a_keyless_public_service_names_the_public_bind_not_the_key_flag failed once in the full parallel run and passed 5/5 alone. It and restart_refuses_a_public_service_without_a_key_before_stopping_it both use test_paths("restart-public-no-key"), whose root is label-pid-millis. Two tests starting in the same millisecond under cargo test share a directory, and one's cleanup deletes the other's record. Both tests are on main and this PR does not touch them.