fix(uninstall): stop managed services before removing the tooling that stops them (EAI-8014) by fredespi · Pull Request #299 · ROCm/rocm-cli

fredespi · 2026-08-22T12:09:16Z

Summary

rocm uninstall reported uninstall complete (RC=0) while a managed model server was still running — including a publicly-bound, GPU-holding vLLM endpoint — and then deleted the rocm/rocmd binaries and service records needed to stop it. The supported rocm services stop path was gone, leaving only a manual PID kill to release the GPU.

This change stops managed servers before removing anything, and refuses to proceed if it cannot confirm they are stopped.

Changes

The stop-before-remove gate

apps/rocm/src/uninstall.rs — the command handler: after the confirmation gate and before removing any path, stop every live managed service. If any cannot be confirmed stopped, abort with a non-zero exit and remove nothing, so the tooling needed to recover (rocm services stop) stays in place. The error names the offending services and the remedy for each.
apps/rocm/src/uninstall_gate.rs (new; owns its types) — stop_managed_services_before_uninstall() returns a ManagedServiceStopReport { stopped, failed, warnings, helper_stopped }, reusing the verified process-tree termination that rocm services stop already uses. A service is counted stopped only once every recorded process is confirmed gone; one that merely crashed is skipped and does not block uninstall.

Killing the supervisor before the supervised

rocmd respawns a ready record whose endpoint is unreachable — exactly the state the stop pass creates — so the background helper is stopped first. A helper that cannot be confirmed stopped is itself a blocking failure.

Not signalling a PID we cannot identify

The daemon stop signals a whole process tree with force, and runtime-state.json outlives a crash or reboot, so the recorded PID can name a stranger. rocmd now records daemon_start_ticks (apps/rocmd/src/service.rs), and the gate classifies the PID with rocm-core's existing identity_state, which checks liveness before it reads the start time, so a recycled PID cannot read as a match. A recycled PID is a no-op, an unverifiable one aborts with the tooling intact, and a record whose running flag is false is never signalled at all.

Windows start-times

crates/rocm-core process_start_ticks now reads the process creation time through GetProcessTimes on Windows, so PID-reuse checks run there too: every Windows managed-service stop can now return Recycled or Indeterminate where it used to always match.

What uninstall leaves running

A run that keeps the binaries and data (--keep-binaries --keep-data) skips the stop pass and leaves running servers alone; plan_removes_recovery_tooling decides this and the plan warning says so. Documented in --help and the README.

Closing the gate's own escape hatches

A record already marked stopped whose port still answers must identify itself as this record's model before it blocks, so an unrelated service on a recycled port cannot brick uninstall — but an auth refusal (401/403) from that endpoint now blocks, because a server stating it guards the path is evidence something is serving.
A wildcard bind is probed on both loopback families: :: with IPV6_V6ONLY=1 answers on ::1 and refuses 127.0.0.1.
Manifests that exist but cannot be parsed abort rather than being skipped silently — a corrupt record can describe a live server.

Supporting work

crates/rocm-core — the atomic-write publish step is single-sourced (write_file_atomically), including the Windows ReplaceFileW path; ManagedServiceRecord::write and AutomationRuntimeState::write now publish through it, so a reader never sees a truncated manifest. http_get_with_auth reports a response status instead of collapsing every non-200 into an error.
The uninstall plan and the stop pass share one non-destructive pre-flight (unreadable manifests, live records with no start time, already-stopped records whose port still answers, helper identity), so the plan predicts a refusal instead of promising a stop. A record is stopped when its recorded pids are still in use, not only when its status reads live, and a service counts as stopped only on the confirmed-stop result.
A removal that fails partway reports what was removed, what failed (and may be partly removed), and what was not attempted, and says recovery tooling may be gone. It is not transactional.
The relaunch advice for a lost endpoint key comes from one shared builder (rocm-engine-protocol). Only the uninstall gate fills in the model, engine and port from the service record. The rocm serve and rocmd lost-key refusals now print the same shared command (with the <model>/<port> placeholders still unfilled) instead of their old hardcoded text. The relaunch command carries the model, engine, host, port and key flags only, not --runtime-id, --env-id or the device policy a service was launched with.
Process-tree stop is Linux-only in effect: on Windows only the recorded process is terminated, and the help, README and plan say so.
apps/rocm/src/therock.rs, apps/rocmd — local helpers document precisely what they do and do not share with the centralised one; now-unused dependencies dropped from both manifests.

Test plan

tests/e2e-cucumber — new @lifecycle scenarios: lifecycle-10 (Linux) and lifecycle-24 (Windows) stop the local server uninstall manages; lifecycle-25 refuses and keeps everything when a service record cannot be parsed; lifecycle-26 stops the background helper; lifecycle-27 runs --keep-binaries --keep-data and leaves a running server and its record alone; lifecycle-28 refuses a live server recorded without a start time; lifecycle-29 refuses on an unreadable helper state file. Existing scenarios were renumbered. The Windows scenario cannot run in the Linux gate I used; CI is its first run.
Unit coverage across apps/rocm, apps/rocmd and crates/rocm-core for: the stop-before-remove ordering (asserted on planned files still existing after an aborted run, not on a return value), a crashed service not blocking, a recycled daemon PID never being signalled, a pre-start_ticks record aborting instead of force-killing, an unreadable runtime state aborting, a stopped-but-serving record blocking on retry, an authenticated survivor blocking rather than failing open, an IPv6-only wildcard engine blocking, and a reader never observing a truncated manifest.
Each of these was verified against a faithful revert of the production change it names, rather than by inspection. Two are labelled in-place as the exceptions: a directory-hygiene check and a false-positive guard, which by design do not fail on revert.
Gated on Linux at CI strictness: cargo fmt --all -- --check, cargo clippy --workspace --all-targets --exclude e2e-cucumber -- -D warnings, and the rocm, rocm-core and rocmd suites.

The uninstall-stops-what-it-manages scenario in test(e2e): pin the contracts a README walkthrough expects (EAI-8024) #241 describes the same behaviour, but that PR is open and unmerged, so it is a dependency to keep in sync rather than existing coverage. The e2e scenario in this PR is what actually holds the behaviour today.
The vLLM and Lemonade engine state files, and the JSON merge that patches them (merge_json_state_file), are now published atomically through write_file_atomically, so rocmd and uninstall never read a torn document.

r0x0r

Solid, well-scoped fix for EAI-8014. Placement is correct (after the dry-run/empty-plan early return and the confirm gate, before removal), the abort is fail-closed, and it reuses the verified process-tree termination path so a service only counts as stopped once every recorded process is confirmed gone. CI is fully green including the GPU/Strix Halo/Windows E2E lanes.

Leaving a few resolvable threads: one meaningful test gap on the abort path plus a couple of minor nits. None are blocking.

michaelroy-amd

Reviewed at e2015fb1a9393a6cc3e757805c02dfc1ed09b81b. The stop-before-remove ordering and the new removal gate are sound, and all 17 required checks pass. Two blocking gaps remain:

load_managed_services silently skips a service record when serde_json::from_slice fails. That lets uninstall proceed without attempting to stop a live service represented by a corrupt manifest, which contradicts this change's fail-closed guarantee and can recreate the reported outcome. Propagate the parse error with the manifest path and add a regression test proving uninstall removes nothing on malformed service state.
The user-visible uninstall contract has no Cucumber scenario in this PR. The cited uninstall-stops-what-it-manages scenario is only in still-open PR #241, so it does not cover this branch. Add the scenario here, or explicitly make this PR depend on #241 and rebase after it lands.

Please address both and re-request review.

rominf

I went through this at e2015fb1. The direction is right and the two helpers are cleanly separated so the abort branch is unit-testable — that's a good call. All checks are green and it's MERGEABLE.

I agree with both blocking points already raised: load_managed_services still does if let Ok(mut record) = serde_json::from_slice(...) at main.rs:15001, so a corrupt manifest for a live service is still invisible to the new gate — which directly contradicts the fail-closed doc comment you added at :13891-13895; and there's still no Cucumber scenario, with the cited uninstall-stops-what-it-manages id present nowhere in the tree (I grepped) and #241 still open and now CONFLICTING, so depending on it means waiting for a rebase there first. I won't repeat the detail on those.

What I want to add is that the abort is not the clean no-op its message claims, and this is worse than the wording nit already noted. Beyond that: the stop pass runs even when the uninstall isn't removing the tooling, the plan warning overcounts, and the .ok() swallows the diagnosis. Details inline.

One I looked at and decided isn't worth a comment on its own: refresh_managed_service_runtime_liveness demoting a record to stopped on dead tracked PIDs while an engine grandchild keeps serving. On Linux that's mostly covered — collect_process_tree walks /proc and signal_process_scope gets tree: true, so descendants do get signalled on the stop path. The Windows gap is real though, and I've folded it into the comment on the liveness skip.

siloteemu

🔴 Automated review · pr-review-watcher · e2015fb

Summary

Stops live managed services before rocm uninstall deletes the binaries and service records needed to stop them, aborting the uninstall with nothing removed when any service cannot be confirmed stopped. The fix itself is sound and fails closed in the right direction — Needs work, because the ordering guarantee it introduces has no test that pins it. Verified: the ordering is guaranteed, not incidental (uninstall.rs:45-48 runs before the for entry in &plan.actions removal loop at uninstall.rs:50, and both calls use ?, so a failed stop returns Err and the loop never executes — nothing is removed); a partial failure is recoverable and re-runnable (services confirmed stopped are written back as status = "stopped", so managed_service_is_live skips them on the next attempt and only the failed ones are retried); already-dead services are correctly skipped rather than counted as failures; but on the revert question, all five new tests still compile and pass if the four wiring lines in uninstall.rs are deleted — I confirmed by grep that uninstall::uninstall() is called only from CLI dispatch (main.rs:23, main.rs:1972) and from no test. I read the code and the caller-supplied CI state; I did not build or run the suite. Blocking: 1 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

apps/rocm/src/main.rs:26777-26887, apps/rocm/src/uninstall.rs:45-50 — nothing tests the thing the PR fixes. The five new tests exercise the two new helpers in isolation; none calls uninstall(), so none proves that stopping happens before removal or that a failed stop actually prevents removal. Delete the stop_managed_services_before_uninstall / uninstall_removal_gate calls from uninstall.rs and the entire test suite stays green — the ordering guarantee rests on code inspection alone. One test is weaker still: uninstall_skips_already_dead_managed_service (main.rs:26809) asserts only that stopped and failed are both empty, which is exactly what a stop_managed_services_before_uninstall gutted to Ok(ManagedServiceStopReport::default()) returns — it cannot distinguish "correctly skipped a dead service" from "did nothing". This also runs into AGENTS.md §3: the change alters user-observable behavior (new abort with a new error message, new stopped N managed service(s) before removal line, servers killed during uninstall), and §3 states plainly that a unit test on an internal helper does not discharge the requirement for a Gherkin scenario in tests/e2e-cucumber/features/ — I checked, and no scenario there registers a managed service (install_lifecycle.feature:115-124 is the only uninstall scenario and would pass identically with or without this fix). Concrete fix: make uninstall() (or an extracted, injectable inner function) testable and add a test that seeds a plan with a real file plus a service whose stop cannot be confirmed, then asserts the call returns Err and the file still exists — that single assertion is what pins the ordering. Add or extend a lifecycle scenario for the observable path, or, if it can only run on a gated lane, name the lane in the PR text per §3. I could not read the PR body from the checkout, so if it already names an @id or states the gap, treat that half as satisfied.

Non-blocking

apps/rocm/src/main.rs:13928-13938 — a service that can never be confirmed stopped (EPERM on a process owned by another user, or a GPU worker wedged in uninterruptible sleep where SIGKILL does not land) makes rocm uninstall permanently impossible: there is no override in UninstallOptions (main.rs:16114), and the suggested recovery rocm services stop <id> --yes routes through the same stop_internal_managed_service with the same outcome. Refusing is the right default, but consider a documented override flag and mentioning the privilege/manual case in the message.
apps/rocm/src/main.rs:13903-13908 — stop_internal_managed_service(...).ok() discards the underlying error, so the abort message names the service but never says why the stop failed; carrying the error string into failed would make the dead end diagnosable.
apps/rocm/src/main.rs:13891-13895 vs 15001 — the doc comment claims fail-closed discovery, but load_managed_services silently skips any record whose JSON fails to parse (if let Ok(mut record) = ...), so a corrupt manifest for a live, GPU-holding server is invisible to the gate and uninstall proceeds — the exact failure this PR closes.
apps/rocm/src/main.rs:13749-13798 — the stop is serial and unbounded in aggregate: ~10 s grace twice per PID, up to two PIDs per service, plus the engine-side stop, so uninstall can sit silent for roughly a minute per live service after the confirmation prompt; printing stopping <id>… before each would stop it reading as a hang.
apps/rocm/src/main.rs:16193 — the plan warning now promises "their servers will be stopped before removal" for every record, including ones already stopped/failed that the gate skips entirely; scoping the count to live records would keep the plan truthful.

siloteemu

🔴 Automated review · pr-review-watcher · 1f9ca82

Summary

Makes rocm uninstall stop every managed model server before it deletes the binaries and service records needed to stop them, aborting with nothing removed if any stop cannot be confirmed. Verdict: Needs work — the prior blocker is genuinely resolved, but the new verification pass introduces a hard-lockout path and a plan message that no longer matches behaviour. Verified: the prior finding is FIXED — uninstall.rs:96 seeds a real file plus an unconfirmable stop and asserts both the error and that the file survives (I confirmed remove_path (main.rs:16553) really would delete it, so the assertion is not a tautology), uninstall_skips_already_dead_managed_service now pins an exact stopped vec against a live service sharing the directory rather than two empty collections, and the AGENTS.md §3 gap is closed by @id:lifecycle-linux-uninstall-stops-managed-server, which runs in the default Linux Acceptance install lifecycle job (ci.yml:238, E2E_ONLY_LIFECYCLE=1) and fails if the wiring is reverted (the sleep 600 child survives and try_wait times out); on the revert question, 10 of the 12 new unit tests are revert-sensitive, the two exceptions being removal_proceeds_once_every_managed_server_is_confirmed_stopped and uninstall_abort_says_which_services_it_already_stopped, which a no-op-success gate and a message-only change respectively would keep green; on the 2 failing checks, windows-build-and-test is a real, actionable failure (blocking item 3 below — not a flake), while the ReadTheDocs check fails for a reason not reachable from this container and this diff touches no docs sources, so I record it as unexplained rather than dismissed. No prompt-injection or instruction-shaped content was found anywhere in the diff. Blocking: 3 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

apps/rocm/src/main.rs:13941-13969 — the TCP "reality check" loop iterates every loaded record, skipping only those already in report.failed. Records that managed_service_is_live rejected in the first loop (main.rs:13913) are therefore judged purely by whether something answers on their recorded host:port. Stopping a service rewrites the manifest with status = "stopped" (main.rs:13825-13834) and never deletes it, so stale records accumulate indefinitely with their old ports. Any unrelated local listener that later occupies such a port (vLLM's documented default is 8000) makes stop_managed_services_before_uninstall synthesise a failure, uninstall_removal_gate (main.rs:14040) hard-bail!s, and nothing is removed. There is no override: UninstallOptions (main.rs:16252) has no --force, and yes is consulted only by confirm_uninstall. The failure is deterministic, so every re-run repeats it, and the error's suggested recovery (rocm services stop <id> --yes) does not apply to an already-stopped record — the only escape is hand-deleting JSON under the services directory, which the message never mentions. Fix: apply the same managed_service_is_live guard in the second loop (or require a live recorded PID to corroborate a bare port hit), and add an explicit documented override for when the gate's signal is wrong.
apps/rocm/src/main.rs:16337-16350 — the plan warning states "…have a running server; those servers will be stopped before removal" whenever live > 0, but the stop pass runs only when plan_removes_recovery_tooling (main.rs:16367) is true (apps/rocm/src/uninstall.rs:44-51). On a cache-only run (--keep-binaries --keep-data), or a dev-binary-layout run without --force-dev-binaries plus --keep-data, the warning is printed before the confirmation prompt and no server is ever stopped. This contradicts both the comment immediately above it ("claiming otherwise would make the plan describe work uninstall never does") and the commit message's claim that the warning "does not promise to stop servers that are not running". Fix: pass the same recovery-tooling predicate into the warning and word it accordingly when the plan leaves the tooling in place.
CI: windows-build-and-test is red and this PR must resolve it. The job checks out the PR-merged-into-main commit, which includes tests/e2e-cucumber/tests/feature_naming.rs (added upstream after this branch diverged, absent from this checkout). Its scenario_names_are_indexed_sequentially_per_feature requires every scenario in install_lifecycle.feature to be named lifecycle-NN - …; the new Scenario: Linux - uninstall stops the local server it manages has no such prefix. Fix: rebase onto current main and renumber the new scenario into the sequence. (Verified from the CI log and by confirming the guard test's absence from this branch; the Test (affected crates) job pins the raw head SHA, which is why it stays green and why this cannot be dismissed as unrelated.)

Non-blocking

No unit test drives uninstall() itself — deleting the stop_managed_services_then_remove call and inlining the old loop is caught only by the e2e scenario; a thin test over uninstall() would close the last of the original finding.
uninstall_refuses_when_a_service_record_cannot_be_parsed (main.rs) is #[cfg(target_os = "linux")] despite touching no PID or process state — it would run fine everywhere and the corrupt-manifest gate is not Linux-specific.
ManagedServiceRecord::write (rocm-core lib.rs:7168) does a plain non-atomic fs::write; a concurrent read during overwrite can make a healthy record look unparseable and spuriously abort the gate. Write-temp-then-rename would close it.
build_uninstall_plan uses load_managed_services(...).unwrap_or_default() (main.rs:16328) while the real gate propagates that error, so the plan/dry-run can show "no managed services" for a run that will in fact abort.
The ReadTheDocs check is failing and unexplained; the diff touches no docs sources, but that should be confirmed rather than assumed before merge.

siloteemu

🔴 Automated review · pr-review-watcher · 6b697a3

Summary

This PR makes rocm uninstall stop the managed servers it manages before deleting the binaries and service records needed to stop them, and adds unit, integration and e2e coverage for that ordering. Verified: finding 1 (hard lockout) STILL OPEN and byte-identical to head 1f9ca82f; finding 2 (plan warning promises unperformed work) STILL OPEN and byte-identical; finding 3 (Windows scenario-naming guard) FIXED — I ran cargo test -p e2e-cucumber --test feature_naming, 4/4 pass, and a sweep found no stale references to the renumbered scenarios; the only work since the held review was a merge of main plus the renumbering commit, so nothing new was introduced, and every added test fails or stops compiling if the production change is reverted (details below) — except that one of them locks in the finding-1 defect as intended behaviour. Blocking: 2 · Non-blocking: 4.

🚫 Blocking (must fix before merge)

1. apps/rocm/src/main.rs:15570-15591 — the port-reachability loop still has no liveness guard; a recycled port deterministically bricks uninstall.
The second pass in stop_managed_services_before_uninstall iterates every loaded record, skipping only ones already in report.failed, then synthesises a failure for any whose recorded host:port accepts a connection. Verified unchanged since the held review (same code, same comment, only shifted by the merge).

Why it bites: managed_service_is_live (main.rs:16576) is a pure status check — ready|running|starting|recovering. load_managed_services (main.rs:16748) returns every *.json under the services dir with no age pruning, and refresh_managed_service_runtime_liveness (main.rs:16715) demotes a record to a non-live status when its recorded pids are gone while persisting it. services stop likewise writes the record back rather than deleting it (main.rs:15446-15456), and rocm services list --all exists precisely to show "failed, stopped, and old service records". So stale records carrying their old ports are the normal steady state, and there is no services remove/prune subcommand to clear them (ServicesCommand is List|Logs|Stop|Restart, main.rs:798). Any unrelated process that later binds a recycled port makes the gate bail!, and UninstallOptions (main.rs:17878) has no override (force_dev_binaries is unrelated). The abort tells the operator to run rocm services stop <id> --yes — which for an already-stopped record neither changes the record nor frees the foreign port, so every retry fails identically.

Fix: apply managed_service_is_live(record) in the second loop as well (or require a live recorded pid to corroborate a bare port hit), and add a documented override flag so an operator is never locked out. Note that main.rs:30729 (uninstall_refuses_while_a_recorded_endpoint_still_accepts_connections) deliberately writes a status = "stopped" record with a live listener and asserts the gate fails — that test currently encodes the lockout as the contract and must be reworked alongside the fix.

2. apps/rocm/src/main.rs:17970-17971 vs apps/rocm/src/uninstall.rs:44-50 — the plan warning promises a stop that will not happen.
build_uninstall_plan pushes "…have a running server; those servers will be stopped before removal" whenever any record is live, with no reference to what the plan actually removes. uninstall() prints the plan at line 25 and prompts at line 35, both before line 44 computes plan_removes_recovery_tooling and skips the stop pass entirely on a false. Verified unchanged since the held review. So rocm uninstall --keep-binaries --keep-data shows the operator a promise to stop their live GPU-holding server, they confirm, and nothing is stopped. Fix: gate the warning on plan_removes_recovery_tooling(&plan, &paths) (the predicate already exists at main.rs:17993) and word the ungated case as informational.

This gap is exactly what the tests miss: only_an_uninstall_that_removes_the_recovery_tooling_stops_servers (main.rs:30777) asserts the predicate in isolation, and nothing asserts the warning text agrees with it.

Non-blocking

Revert check, per test: the four uninstall.rs tests inject the stop closure into stop_managed_services_then_remove, so removing the stop call is a compile error — genuinely tied. The main.rs gate/report tests call stop_managed_services_before_uninstall / uninstall_removal_gate / plan_removes_recovery_tooling directly, so a revert deletes the symbols — tied. The e2e scenario plants a real sleep 600 plus a service record and polls try_wait() on its own child, so a revert leaves it running and the assertion fails — genuinely tied, not a pass-either-way test.
apps/rocm/src/uninstall.rs test a_live_managed_server_is_stopped_before_the_planned_paths_are_removed hardcodes port 9, and the e2e step hardcodes 59999; anything listening there on the runner flips these from pass to fail (via the very second loop in blocking item 1). Prefer a bound-then-dropped ephemeral port.
probe_host (main.rs:15607) maps only 0.0.0.0, ::, [::], ""; other wildcard spellings a record could carry fall through and are probed literally.
The e2e step comment says liveness "falls through to the recorded process" because nothing listens on the chosen port — accurate today, but it depends on the same unguarded port probe that blocking item 1 asks you to change; revisit it with that fix.

siloteemu

🔴 Automated review · pr-review-watcher · 451a8ee

Summary

Adds a stop-before-remove gate to rocm uninstall (stop every live managed service, abort with nothing removed if any stop cannot be confirmed), plus a port reality-check, plan-warning rework, an atomic service-record write, and unit + e2e coverage. Verdict: Needs work — the core gate is well-designed and genuinely tested, but the remediation commit introduces a Windows-risky third copy of an atomic-write helper, and two fail-direction defects remain. Verified: compiled a standalone probe confirming ("[::1]", port).to_socket_addrs() and whitespace-padded hosts fail resolution (so the port probe silently reads "nothing serving"); cargo test -p rocm and the feature-naming suite pass, and reverting the for record in attempted line makes a_stale_record_whose_old_port_was_reused_does_not_block_uninstall fail, so that test is real regression coverage; confirmed by reading source that stop_internal_managed_service does record.write()?, that apps/rocm/src/therock.rs:3042 and apps/rocmd/src/lib.rs:1139 already implement a hardened publish_temp_file, and that only *.json manifests live directly under services_dir. CI for this head: 18 success, 1 skipped, no failures. Blocking: 3 · Non-blocking: 4.

Prior review status (judged from the current code, not from the earlier report):

Stale records judged by their recorded port → permanent, unrecoverable uninstall block: FIXED (probe restricted to services this pass attempted; pinned by a test verified to fail on revert).
Plan warning promising a stop the run would not perform: FIXED (warning built after actions are final and gated on the same predicate, pinned by the_plan_warning_only_promises_a_stop_the_uninstall_will_perform).
Wiring between plan / gate / removal untested: FIXED (uninstall_with_paths now driven end to end).
probe_host not covering all wildcard spellings: PARTIALLY FIXED — see Blocking 2; the non-wildcard branch still discards the normalization.
"Fail closed with no override and no recovery" as a class: STILL PRESENT for the unparseable-manifest path — see Blocking 3. The same reasoning that justified fixing the recycled-port case applies here and was not carried over.

🚫 Blocking (must fix before merge)

1. crates/rocm-core/src/lib.rs:7317-7357 — the new atomic write is a third, weaker copy of a helper this repo already hardened, and it now sits on the uninstall critical path.
ManagedServiceRecord::write was changed from fs::write to temp-file + bare fs::rename. This project already has that pattern twice — apps/rocm/src/therock.rs:2925-3080 and apps/rocmd/src/lib.rs:1053-1163 — and both wrap the publish step in publish_temp_file, which on Windows deliberately routes through ReplaceFileW (replace_file_windows) whenever the destination exists, precisely because a plain rename-over-existing is not reliable enough here. Both copies also carry a collision-avoidance loop and tests that a failed publish leaves no scratch file. The new implementation has none of that, and manifests are overwritten on every status transition. It matters more than usual because the failure propagates: apps/rocm/src/main.rs:15456 is record.write()?, so a publish failure inside stop_internal_managed_service becomes a failed entry in the stop report and hard-aborts the uninstall this PR is trying to make safe. I could not run Windows here, so I am stating the platform semantics from the repo's own prior art rather than from a live repro — but the prior art is unambiguous that this codebase does not consider bare rename sufficient. Fix: hoist write_file_atomically/stage_file_for_atomic_publish/publish_temp_file into rocm-core and call it from ManagedServiceRecord::write (which also removes the existing duplication between therock.rs and rocmd); note crates/rocm-core/Cargo.toml:31 currently lacks the Win32_Storage_FileSystem feature that apps/rocm/Cargo.toml:54 has.

2. apps/rocm/src/main.rs:15614-15624 — probe_host computes a normalized host and then throws it away, so the port reality-check fails open.
The function builds normalized (trimmed, bracket-stripped, lowercased) to classify the host, then the default arm returns host.to_owned() — the raw string. loopback_tcp_port_is_reachable (main.rs:13359) does (host, port).to_socket_addrs(), which I confirmed by compiling a standalone probe rejects both "[::1]" and " 127.0.0.1 ". A resolution failure returns false, and the caller at main.rs:15585 treats false as "nothing is serving" and continues. So for a record whose host is a bracketed IPv6 literal or carries stray whitespace, the only check that can catch a surviving engine grandchild — the documented reason this probe exists — silently disappears, and uninstall removes the tooling while the endpoint is live. This is representable: main.rs:10232 (loopback_host_key) already normalizes "[::1]", so the codebase itself expects bracketed spellings in records, and --host is a free-form string. Fix: _ => normalized in the default arm, and add a bracketed non-wildcard IPv6 case to every_wildcard_bind_spelling_is_probed_on_loopback — that test currently only exercises unbracketed literals, which is why this slipped through.

3. apps/rocm/src/main.rs:15674-15716 (with 15638-15667) — an unparseable manifest blocks uninstall permanently and the abort message prescribes a fix that cannot work.
unreadable_service_manifests pushes the filename as the service_id (main.rs:15660-15665), and the gate then tells the operator: "Stop them with rocm services stop <id> --yes, then re-run uninstall." For this failure mode that command cannot succeed — rocm services stop loads the same record and fails on the same JSON — so every retry aborts identically, with no flag to override and no mention anywhere (message, doc comments, docs) that the real remedy is to inspect or remove the file on disk. This is the same "no override, no recovery" property that was correctly judged unacceptable for the recycled-port case in this very commit; it should be judged the same way here. Minimum fix: include the full manifest path and state the actual remedy for the unparseable case (repair or delete the record), distinct from the still-serving case; better, add an explicit override for "I have verified nothing is serving".

Non-blocking

tests/e2e-cucumber/features/install_lifecycle.feature:129 — the new scenario is @requires-os:linux only, so the gate has no Windows e2e coverage even though the Windows grandchild case is exactly why the port probe exists.
crates/rocm-core/src/lib.rs:7344-7352 — a crash between write and rename leaves a hidden .<name>.<pid>.<ms>.tmp file that nothing ever sweeps; harmless to readers (all scanners filter on the json extension) but it accumulates.
apps/rocm/src/uninstall.rs (a_live_managed_server_is_stopped_before_the_planned_paths_are_removed, the_uninstall_command_itself_stops_a_managed_server_before_removing_anything) and tests/e2e-cucumber/tests/e2e/lifecycle_steps.rs:485 — the bind-then-drop ephemeral-port trick can race on a busy runner; uninstall_refuses_while_a_recorded_endpoint_still_accepts_connections shows the safer shape (hold the listener open).
apps/rocm/src/uninstall.rs tests — temp roots and spawned children are cleaned only on the success path, so a failing assertion leaks a directory; matches existing convention in this file, so only worth tightening opportunistically.

Superseded: this change request was left at an earlier head. The findings it raised are re-judged against 451a8ee in the current round — the stale-record port block, the plan-warning overclaim and the untested wiring are all fixed there. A fresh change request stands at the current head for the remaining items.

siloteemu

🔴 Automated review · pr-review-watcher · 4db3a7f

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

Summary

Adds a stop-before-remove gate to rocm uninstall (stop every live managed service, abort with nothing removed if any stop cannot be confirmed), and in the latest commits consolidates the atomic-write helper, makes the port probe resolvable, gives each abort class a remedy that actually works, re-probes every record so a retry stays honest, and stops the rocmd background helper first. Outcome: Needs work — all three of our prior blocking points are genuinely fixed, but the newest commit adds a destructive path we have not reviewed before and that this PR does not test. Verified: cargo test -p rocm-core --lib passes (333/333) on this checkout, and targeted cargo test -p rocm --bin rocm uninstall_removal_gate passes (3/3); confirmed by scratch-revert that the uninstall-ordering and uninstall_removal_gate tests fail when their production lines are reverted; confirmed by reading source that probe_host's default arm now returns normalized, that publish_temp_file/replace_file_windows moved into rocm-core byte-for-byte with Win32_Storage_FileSystem following them and no Windows FFI left in the app crates, that unreadable_service_manifests now pushes the full path with a RepairTheRecord remedy, that ProcessIdentity::new(pid, None) makes identity_state return Matches unconditionally, and that terminate_verified(..., Tree, .., force=true) escalates to SIGKILL across the whole tree. Blocking: 2 · Non-blocking: 5.

Prior objection status

1 — atomic write was a third, weaker copy of a hardened helper: RESOLVED. publish_temp_file and replace_file_windows (both cfg arms, the try_exists → ReplaceFileW → rename-race fallback, the SAFETY comment) are now single-sourced in crates/rocm-core/src/lib.rs:7634-7692; therock.rs:3038 and rocmd/src/lib.rs:1135 delegate to it, ManagedServiceRecord::write goes through the new write_file_atomically (collision loop, sync_all, cleanup-on-failure), and the windows crate feature moved to crates/rocm-core/Cargo.toml:31 with no direct FFI left behind in the app crates.
2 — probe_host discarded its normalization and failed open: RESOLVED. apps/rocm/src/main.rs:15763 is now _ => normalized, and every_wildcard_bind_spelling_is_probed_on_loopback gained [::1], " 127.0.0.1 " and [FE80::1] cases that assert both the string and to_socket_addrs().is_ok() — all three fail on revert, so it is real regression coverage.
3 — unparseable manifest was a permanent block with impossible advice: RESOLVED. main.rs:15733 now records the full path, the remedy is RepairTheRecord, and the gate's text (main.rs:15877-15891) tells the operator to check/stop the server, then repair or delete the named file — with a test asserting the message names the file and does not say rocm services stop. No override flag was added, and none is needed now that each failure class names a remedy that can clear it.
Prior non-blocking Windows e2e gap: also resolved — lifecycle-24 adds the @requires-os:windows sibling of the Linux scenario.

🚫 Blocking (must fix before merge)

1. apps/rocm/src/main.rs:15557-15591 — rocm uninstall can now SIGKILL an entire process tree at a PID it cannot prove is rocmd.
stop_background_helper_before_uninstall builds ProcessIdentity::new(state.daemon_pid, None) (line 15573) and passes it to terminate_verified(&identity, KillScope::Tree, MANAGED_STOP_GRACE, true). With start_ticks: None, identity_state (crates/rocm-core/src/proc_lifecycle.rs:87-104) takes the (None, _) => Matches arm — the "legacy state file, best-effort proceed" path — so Recycled and Indeterminate can never be returned for this PID. force = true then means SIGTERM followed by SIGKILL across process_tree_pids(pid). This is the only kill in the codebase with no recorded identity: the pre-existing managed-service call site (main.rs:15394) passes real ticks, and ProcessIdentity's own doc comment says it exists to be "robust to PID recycling". Nothing invalidates daemon_pid: crates/rocm-core/src/lib.rs:5878-5892 only reads/writes automations/runtime-state.json, never deletes it, and running = false is written only on a clean shutdown (apps/rocmd/src/lib.rs:3053) — after a crash, OOM-kill or reboot the file survives with a stale PID. The guard at main.rs:15564-15567 checks only pid != 0, pid != self, and process_is_running, and (unlike the repo's own background_helper_already_running, main.rs:5963-5966) does not consult state.running. So the reachable sequence is: rocmd dies uncleanly → machine reboots → the OS reissues that low PID to an unrelated process → the user runs rocm uninstall → that process and every descendant are killed. That is exactly the "uninstall destroys something the user did not install" case, and it is new in this PR. Fix: persist the daemon's start ticks alongside daemon_pid in AutomationRuntimeState (capture with ProcessIdentity::capture at spawn, as services already do) and pass them here, so Recycled/Indeterminate are detected; when identity cannot be verified, do not signal — record a failed entry with the existing StopTheDaemon remedy (which already tells the operator to kill the pid themselves) rather than killing on a guess.

2. apps/rocm/src/main.rs:15557 — the new daemon-stop behaviour has no test at all, in a PR where every other new behaviour is revert-pinned.
Grepping the unit tests in main.rs and uninstall.rs, the feature file and lifecycle_steps.rs turns up no test that constructs an AutomationRuntimeState with a live PID, and no assertion on the "rocmd (background helper)" / "rocmd (pid …)" strings this function produces. Every test added or changed by this PR would still pass if the call at main.rs:15614 were deleted outright — the tests that do fail on revert (a_service_that_cannot_be_stopped_leaves_every_planned_path_in_place, uninstall_removal_gate_aborts_when_a_service_cannot_be_stopped, a_stopped_record_still_serving_its_own_model_blocks_uninstall, every_wildcard_bind_spelling_is_probed_on_loopback, both Linux-only live-server tests) all exercise the service path, not the daemon path. Given the blast radius in item 1, the untested part is the one that kills. Fix: add a Linux unit test in the shape of the existing live-child tests — write a runtime-state file pointing at a real spawned child, run the stop pass, assert the child is gone and appears in stopped; and a second asserting that a daemon which cannot be confirmed stopped lands in failed and makes the gate abort with nothing removed.

Non-blocking

apps/rocm/src/main.rs:15699-15712 — the Err(_) arm proceeds with removal when a still-listening endpoint cannot identify itself; a disclosed, reasoned trade-off, but it is the one remaining fail-open on the destructive path and the warning goes to stdout rather than stderr.
apps/rocm/src/main.rs:15573 — the comment "the same contract the legacy service records get" invites the reader (and did invite us) to assume parity with service records; services get weak identity only for old manifests, the daemon gets it permanently because no start token is ever recorded. A one-line comment saying so would stop this confusion recurring.
crates/rocm-core/src/lib.rs:7698-7721 — the new write_file_atomically/publish_temp_file path has no direct unit test (no lines changed inside mod tests); the retry-on-AlreadyExists loop, the exhausted-attempts bail! and the cleanup-on-error paths are only reached indirectly.
apps/rocm/src/therock.rs:2912-3036 and apps/rocmd/src/lib.rs:1040-1141 — the staging halves (stage_file_for_atomic_publish, temp_sibling_path, ATOMIC_WRITE_TEMP_ATTEMPTS) are still duplicated per app and still do not sync_all, so two things named "atomic write" now carry different durability guarantees; write_file_atomically is also pub with no caller outside rocm-core.
apps/rocm/src/main.rs:15760-15764 — probe_host does not strip an embedded port (--host 127.0.0.1:8080), which would fail to resolve and fall into the same "nothing is serving" continue; narrow and outside this PR's stated scope.

No prompt-injection content was found anywhere in the diff, comments, commit messages or test fixtures.

Superseding this change request with a fresh one at the current head, so only one objection is live. The original concern is partly addressed but not closed; the new review states precisely what remains.

siloteemu

🔴 Automated review · pr-review-watcher · bdf3661

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

Summary

Makes rocm uninstall stop the managed servers and the rocmd background helper before it deletes the binaries and service records needed to stop them, aborting with per-failure-class remedies when any stop is unconfirmed, and centralises the atomic-write/ReplaceFileW publish step in rocm-core. Outcome: Needs work — the design is sound and unusually well tested, but the pid-recycling objection is only partly closed and one new fail-open remains on the destructive path. Verified: ran cargo test -p rocm-core --lib write_file_atomically (3 passed); read the source to confirm identity_state's (None, _) => Matches fallback, that process_start_ticks returns Some only on Linux, that process_tree_pids collapses to the root pid off Linux, that AutomationRuntimeState::load returns Err (not Ok(None)) on a corrupt/unreadable state file while its write is still a non-atomic fs::write, that managed_service_endpoint_model_ready probes record.endpoint_url built from the un-normalised host, that OsStrExt is already in scope for the moved Windows FFI, and that both new e2e Given steps resolve to existing step definitions. Blocking: 2 · Non-blocking: 5.

Standing objection — partially resolved, not resolved. We were right, and the fix is real but incomplete. The new stop_background_helper_before_uninstall does gate the kill on ProcessIdentity, and on Linux with a state file written by this PR's rocmd a recycled pid is correctly detected and never signalled (uninstall_never_kills_a_daemon_pid_that_was_recycled genuinely pins that). But the gate is inert whenever daemon_start_ticks is None — see blocking #1. The PR's own field doc says the two causes of None "must not be conflated"; the only caller conflates them.

🚫 Blocking (must fix before merge)

1. apps/rocm/src/main.rs:15592-15612 — the daemon is still force-killed on a pid it cannot prove is rocmd, whenever no start-time was recorded.
identity_state (crates/rocm-core/src/proc_lifecycle.rs:91-104) maps (None, _) => IdentityState::Matches, not Indeterminate. So the match falls through to terminate_verified(&identity, KillScope::Tree, MANAGED_STOP_GRACE, /* force */ true) — SIGTERM then SIGKILL — for every runtime-state.json that carries no daemon_start_ticks. That is: (a) any Linux state file written before this PR, i.e. the ordinary upgrade path for existing users, where collect_process_tree is available and the kill is the whole tree of a possibly-unrelated process; and (b) every macOS and Windows install, permanently, since process_start_ticks is #[cfg(not(target_os = "linux"))] -> None (proc_lifecycle.rs:312-316) — blast radius there is the single root pid, since process_tree_pids returns vec![root] off /proc, but it is still an unrelated process killed by an uninstall. This PR introduces the kill site; before it, uninstall signalled nothing. The stale-file scenario the code comment itself describes ("survives a crash, OOM-kill or reboot with running still true") is exactly the case a pre-upgrade state file represents.
Also note the new code never checks state.running, while the pre-existing double-spawn guard at apps/rocm/src/main.rs:5965 does — so even a cleanly shut-down daemon's leftover pid is a kill candidate.
Fix: distinguish "no identity recorded" from "identity verified" at this call site rather than relying on the best-effort fallback. Concretely, before signalling: if rocm_core::process_start_ticks(state.daemon_pid).is_some() && state.daemon_start_ticks.is_none(), this platform can verify and the record simply predates the field — treat it as Indeterminate (push StopFailureRemedy::StopTheDaemon, abort, leave the tooling in place) instead of killing. That closes the Linux upgrade hole outright and leaves the documented macOS/Windows best-effort degradation as the only remaining gap, which should then be stated in the daemon_start_ticks doc at crates/rocm-core/src/lib.rs:5869-5880 rather than only in a call-site comment.

2. apps/rocm/src/main.rs:15574 — an unreadable or corrupt runtime-state.json silently skips the daemon stop entirely.
let Ok(Some(state)) = AutomationRuntimeState::load(paths) else { return; }; collapses three outcomes into one. load (crates/rocm-core/src/lib.rs:5890-5901) returns Ok(None) only when the file is absent; a permission error or a truncated/unparseable file returns Err, and that Err is discarded. Nothing is pushed to report.failed, nothing is printed, and uninstall proceeds to delete the binaries and every service record while a live rocmd — which respawns a managed service whose endpoint stops answering — may still be running. This is the precise defect the PR exists to close, and it is the opposite of how the PR treats the equivalent service-side case (unreadable_service_manifests, apps/rocm/src/main.rs:15772-15781, deliberately turns an unparseable manifest into a hard failure with the RepairTheRecord remedy). It is reachable in practice because AutomationRuntimeState::write (crates/rocm-core/src/lib.rs:5903-5912) is still a plain non-atomic fs::write executed on every daemon tick, so a crash mid-write leaves exactly this file.
Fix: match explicitly — Ok(None) => return, Err(error) => report.failed.push(FailedManagedServiceStop { service_id: "rocmd (runtime state unreadable)", reason: format!("{error:#}"), remedy: StopFailureRemedy::StopTheDaemon }), Ok(Some(state)) => ….

Non-blocking

apps/rocm/src/main.rs:15729 — the model-identity probe is handed the raw record, whose endpoint_url is built from the un-normalised host (format_http_base_url, crates/rocm-core/src/lib.rs:124), so a 0.0.0.0/:: bind is connected to literally — the exact non-portability probe_host was written three lines earlier to avoid; the fallout is a warned-but-fail-open pass on a wildcard-bound orphaned engine.
crates/rocm-core/src/lib.rs:5903 — AutomationRuntimeState::write was left on non-atomic fs::write while ManagedServiceRecord::write moved to write_file_atomically; that is the file blocking #2 depends on being intact.
apps/rocm/src/therock.rs:2925 and apps/rocmd/src/lib.rs:1053 — both keep their own full write_file_atomically staging pipelines and only call into the shared publish primitive, so neither picked up the new sync_all()-before-publish that the PR added specifically to prevent a zero-length file after a crash; three same-named functions now exist in the workspace.
apps/rocm/src/main.rs:984-1043 (a_stopped_record_still_serving_its_own_model_blocks_uninstall) — drop(serving) only detaches the JoinHandle; the thread stays blocked in accept() holding an ephemeral port for the life of the test binary, and none of the sleep-child tests use a kill-on-drop guard, so a genuine regression panics before the reaping line and leaks a 60s child plus its temp root (the e2e suite already models the right pattern in LifecycleState::drop).
crates/rocm-core/src/lib.rs:7711 — write_file_atomically creates its temp file with default mode and replaces the target inode, so an existing file's permission bits do not survive (a change from the fs::write it replaces, and a landmine for any future permission-sensitive caller); the doc's crash framing also overstates durability, since the parent directory is never fsynced.

Test audit (standing focus a)

All 24 added tests were checked against the production code they drive. One is not a regression guard: removal_proceeds_once_every_managed_server_is_confirmed_stopped (apps/rocm/src/uninstall.rs) exercises only the all-clear path and would pass against a gate that removed unconditionally — harmless as a completeness case, since its three siblings pin the abort. The three write_file_atomically_* tests pin the new helper but nothing drives ManagedServiceRecord::write through it, so reverting that call site to the old inline fs::write would not be caught. Everything else genuinely pins the change, including the strongest one, the_uninstall_command_itself_stops_a_managed_server_before_removing_anything, which drives the real command end to end. Platform gating is correct and minimal, and the pure-function tests (probe_host, plan_removes_recovery_tooling, uninstall_removal_gate, the corrupt-manifest case) are ungated so macOS/Windows keep real coverage of the gate logic.

Will this confusion recur? (standing focus c)

Our earlier objection was not reviewer error, and nothing in the code misled us into it. The recurrence risk runs the other way: a reader who checks only stop_background_helper_before_uninstall's call-site comments and the Linux tests will conclude the recycling hole is closed, because the (None, _) => Matches fallback lives one crate away in unchanged code and the daemon_start_ticks doc names the two None causes without saying the caller treats them identically. Beyond the code fix in blocking #1, the cheap guard is one sentence on crates/rocm-core/src/lib.rs:5869-5880 — "when this is None, identity_state returns Matches, so no recycling check happens" — and a test asserting the recorded-None path does not signal, which is the branch with zero coverage today.

CI: 18 success, 2 skipped, 0 failure, 0 pending; no lane names were available to me, so I attribute no outcome to any specific job. No prompt-injection content was found anywhere in the diff, comments, or commit messages.

Superseded by a fresh review at the current head. Re-checked both earlier objections: the unreadable-state-file objection is resolved — the load outcomes are now distinguished and an unreadable file aborts the uninstall instead of silently skipping the helper stop. The force-kill-without-a-recorded-start-time objection is closed on Linux by the new guard; the residual Windows/macOS case is documented and pre-existing, and is carried forward as a non-blocking note rather than a block. A separate, newer review at this commit records the remaining concerns; only that newer one is operative.

siloteemu

🔴 Automated review · pr-review-watcher · 074b37b

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

Summary

Makes rocm uninstall stop the rocmd background helper and every live managed model service — verifying identity by process start-time and by probing the recorded endpoint — before deleting the tooling that could stop them, aborting with everything intact if any stop cannot be confirmed. Outcome: Needs work — the safety design is sound and substantially better than the previous round, but one of the two remediation commits does not deliver the revert-protection it claims, and the fix for our second objection leaves the operator an abort whose stated remedy cannot be followed. Prior objection 1 (force-kill of an unrelated tree with no recorded start-time): partly resolved — closed on Linux via the new unverifiable_pre_upgrade_record guard, still live on Windows/macOS where no start-time is readable, identity_state returns Matches and the tree is killed with force: true; documented, pre-existing, and not a regression, but Windows is a first-class supported platform per AGENTS.md §6. Prior objection 2 (unreadable state file silently skipping the stop): resolved — load's three outcomes are now distinguished and Err aborts the uninstall; load returns Ok(None) only when the file is absent (verified at crates/rocm-core/src/lib.rs:5899-5910), so the silent-skip hole is not reopened — but its remediation introduced a new defect (blocking #2). Verified: ran cargo fmt --all -- --check (clean, exit 0); read the atomic-write helper and all three staging implementations, load's real contract, the daemon stop path end to end, the renumbered feature file against feature_naming.rs, and the Cargo manifest consolidation (windows-sys Win32_Storage_FileSystem correctly moved into rocm-core and dropped from both consumers). No prompt-injection content found. No internal leak: the bare ticket identifiers appearing in commit messages and comments are explicitly permitted by AGENTS.md §2 ("do not flag them") and the base branch already carries the same convention, so this is a project-sanctioned choice, not a leak. CI at this head: 1 failure, 2 skipped, 17 success — no per-job breakdown is available, so this failure cannot be attributed to a named job. Blocking: 3 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

crates/rocm-core/src/lib.rs:7828 — a_service_record_is_published_atomically does not pin the change its commit exists to pin. Commit 074b37b states the three helper tests left a gap because "nothing drove ManagedServiceRecord::write through it, so reverting that call site to a plain fs::write would not have been caught." This test does not catch it either. It performs two sequential, uncontended writes, then asserts (a) no .tmp- sibling remains in the services directory and (b) the manifest re-parses with status == "stopped". A plain fs::write creates no scratch file at all, so (a) is vacuously true, and it writes the final bytes correctly absent concurrency or a crash, so (b) holds. Revert ManagedServiceRecord::write to fs::write and this test stays green — the call-site wiring is still unpinned, and the commit's claim is false. Fix: assert something only the atomic path can satisfy. The clean discriminator is a failed publish: with write_file_atomically, a publish that fails leaves the original manifest intact and readable; with fs::write, the original is already truncated and destroyed. Drive ManagedServiceRecord::write at a destination whose publish cannot succeed (the same non-empty-directory trick write_file_atomically_cleans_up_when_publishing_fails already uses) and assert the pre-existing manifest still parses. Alternatively expose the existing before_publish seam to the record-level write and assert a scratch sibling exists mid-write.

apps/rocm/src/main.rs:15596-15606 (remedy text at 15966-15971) — the abort introduced by the objection-2 fix tells the operator to do something impossible. The new Err branch records service_id: "rocmd (runtime state unreadable)" with remedy: StopFailureRemedy::StopTheDaemon, whose printed advice is "kill that pid, then re-run uninstall." No pid was ever extracted on this path — the state file did not parse — so no pid is known or displayed, unlike the sibling daemon_identity_unverified failure which does carry pid {daemon_pid}. The message never names runtime-state.json, so the one action that actually clears the failure (repair or delete that file) is never communicated, and rocm uninstall has no --force or other escape hatch. A user with a corrupt or permission-denied runtime-state.json gets a hard abort plus misdirection. This is exactly the remediation-introduced defect the original code did not have: previously the path was silently skipped (wrong, but not a dead end). Fix: give this failure a remedy that names the file — either reuse StopFailureRemedy::RepairTheRecord, whose text already says "repair or delete the file" (15979-15984), or add a variant printing the concrete runtime-state.json path. The existing test uninstall_refuses_when_the_daemon_runtime_state_cannot_be_read only asserts remedy == StopTheDaemon, so it locks in the wrong text; update it to assert the message names the state file.

apps/rocm/src/main.rs:15544-15571 — a ~25-line design rationale is attached to the wrong function. The rustdoc block beginning "Stop the background helper before uninstall stops the services it supervises" — covering the rocmd respawn race, the pid-recycling hazard, why a whole tree is signalled with force, and the /proc-less degradation — sits immediately above fn daemon_identity_unverified (15572), a three-line constructor for a failure struct. fn stop_background_helper_before_uninstall (15582), the function that rationale actually describes and the one carrying the destructive behaviour, has no doc comment at all. Rustdoc will publish the whole safety argument on the wrong symbol. In a PR whose value rests on making a destructive gate reviewable, this is a documentation-correctness defect, not a nit, and it is objectively wrong rather than a matter of taste. Fix: move the block down to sit directly above fn stop_background_helper_before_uninstall, and leave daemon_identity_unverified with just the one-line sentence already written for it ("The failure recorded when the background helper is live but cannot be proven to be rocmd...").

Non-blocking

apps/rocm/src/main.rs:15628-15662 — prior objection 1 is still live on Windows and macOS: with no readable start-time, identity_state returns Matches from its legacy arm and the pid is tree-killed with force: true; documented and consistent with existing managed-service kills, but Windows is a supported platform, so consider recording an alternative Windows identity (creation time via GetProcessTimes) as follow-up.
apps/rocm/src/therock.rs:2971-3022, apps/rocmd/src/lib.rs:1053-1128 — only the publish step was consolidated; three near-identical staging implementations remain, and the two outside rocm-core lack the sync_all the new helper documents as its guarantee. Commit 911a399's subject "one atomic-write helper" overstates its own (accurate) body and cost a reviewer on this pass a false blocking call — a one-line comment on each local write_file_atomically saying only the publish step is shared, and why staging stayed local (the before_publish/suffix_for_attempt test seams), would stop that recurring.
apps/rocm/src/main.rs — every test covering the pid-recycling defences is #[cfg(target_os = "linux")], including uninstall_refuses_while_a_recorded_endpoint_still_accepts_connections, which is commented as "the Windows grandchild case, modelled faithfully" yet never runs on Windows; the e2e lifecycle-24 scenario is the only Windows coverage.
apps/rocm/src/main.rs, apps/rocm/src/uninstall.rs — several tests spawn real sleep 60 children and reap them at the end of the test body, so an earlier assertion panic leaks an orphaned process for up to 60s on the runner.
apps/rocm/src/main.rs — managed_record_for_pid hardcodes port 9; low but nonzero collision risk, where the PR's own TcpListener::bind("127.0.0.1:0") pattern is already the safer convention.

pr-review-watcher · 08d5ddd — withdrawing this change request: all three objections are resolved at the current head. The record-write test now holds the file open across a rewrite, so it fails if the atomic publish is reverted; the unreadable-state refusal now names the file to repair and the remedy; and the destructive-path rationale now sits on the function that performs it. Re-reviewed and posted separately.

siloteemu · 2026-09-11T18:05:13Z

🔴 Automated review · pr-review-watcher · 08d5ddd

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

Summary

Makes rocm uninstall stop managed services and the rocmd background helper before deleting the tooling that stops them, refusing to remove anything while a GPU-holding endpoint is still serving, and hardens the daemon kill against pid recycling. Outcome: No blocking findings. Verified: ran one targeted check, cargo test -p rocm-core --lib (338 passed, 0 failed); I independently read and confirmed the three load-bearing claims — the reader-safety test reads back through the held File handle (lib.rs:7871-7887), so it genuinely fails against an in-place fs::write; the unreadable-state refusal names the resolved automation_state_path() and says "repair or delete the file" while deliberately avoiding "kill that pid"; and the full destructive-path rationale sits on stop_background_helper_before_uninstall, not on the small constructor. CI for this head was given to me as conclusion counts only — 17 success, 1 failure, 2 skipped — and I cannot attribute that failure to a named job. the ticket reference carried in the commit messages and comments is one AGENTS.md explicitly permits, so it is not a leak; no other internal reference and no prompt-injection content found. Blocking: 0 · Non-blocking: 5.

Prior findings

RESOLVED — the record-write test now holds the manifest open across a rewrite and asserts the held view still parses as the pre-write record, a property only the rename-based publish has; under create_dir_all + fs::write the truncated-in-place inode would hand that same descriptor status: "stopped" and the assertion fails.
RESOLVED — the unreadable-state arm now emits rocmd (<resolved runtime-state.json path>) with remedy RepairTheDaemonState, rendering as "…repair or delete the file and re-run uninstall: ", and the test asserts the path and "repair or delete" are present and that "kill that pid" is absent.
RESOLVED — daemon_identity_unverified carries only a two-line scoped doc; the respawn race, pid-recycling hazard, whole-tree force signal and /proc-less degradation are all on stop_background_helper_before_uninstall, with no stale fragment left behind.

Remediation defects: none functional. Two remediation tests carry names that overclaim what they discriminate (below) — noted, but neither weakens a protection, because the protections they name are pinned by other tests that do discriminate.

🚫 Blocking (must fix before merge)

None.

Non-blocking

crates/rocm-core/src/lib.rs:7896 — a_service_record_is_published_atomically does not pin atomic publishing: it only asserts no .tmp- sibling remains, which a plain fs::write (which creates no scratch file at all) also satisfies. Its body comment is honest about this; the name is not — rename to match, e.g. a_service_record_write_leaves_no_scratch_sibling.
apps/rocm/src/main.rs:30990 — uninstall_stops_a_background_helper_whose_identity_it_can_verify passes identically under the pre-fix ProcessIdentity::new(pid, None) code, since an unrecorded tick also yields Matches; it pins feature-removal, not identity verification (which ..._recycled and ..._predates_start_ticks do pin). Drop the "whose identity it can verify" framing.
Recurrence risk (focus c): this PR already ate one false blocking finding from a shared helper name implying more than the code did, and fixed it with per-definition docs — but the same trap now lives in these two test names. A reviewer checking revert-pinning reads the name first; a one-line "this pins X, not Y" comment on each is the cheap fix.
apps/rocm/src/main.rs:15586 — process_start_ticks(state.daemon_pid) is read twice in quick succession (once for unverifiable_pre_upgrade_record, once inside identity_state); harmless since both reads gate toward the conservative outcome, but a single read passed down would be clearer.
apps/rocm/src/main.rs:15602 — a manually deleted runtime-state.json with a live rocmd still yields Ok(None) and silently skips the daemon stop; defensible, but unlike the /proc-less case this residual gap is not stated in the doc comment that carefully enumerates the other two outcomes.

fredespi · 2026-09-14T11:18:44Z

Round 5 addressed in df64d9a. All five non-blocking items taken, none declined.

Test names that overclaim (items 1, 2 and the recurrence note). Both calls are right, and in both cases the body comment being honest was not a defence — the name is what a reviewer checking revert-pinning reads first.

a_service_record_is_published_atomically → a_service_record_write_leaves_no_scratch_sibling. Its comment now says what it pins (scratch hygiene), what it does not (atomic publishing — a plain fs::write creates no scratch file and passes too), and where the real property lives: a_service_record_write_never_shows_a_reader_a_truncated_manifest, which does fail against an in-place write. Kept rather than deleted, for the reason you gave: a leaked .tmp- sibling is an unparseable record to unreadable_service_manifests, so it would abort an uninstall.
uninstall_stops_a_background_helper_whose_identity_it_can_verify → uninstall_stops_a_live_background_helper. Its comment states it pins feature-removal only, that an unrecorded tick also yields Matches so it passes against the pre-fix ProcessIdentity::new(pid, None), and names the two tests that do discriminate (uninstall_never_kills_a_daemon_pid_that_was_recycled, uninstall_never_kills_a_daemon_pid_from_a_state_file_that_predates_start_ticks).

Double process_start_ticks read (item 4). Taken — and on a closer look it is marginally worse than harmless, which is why it got a commit of its own rather than a local variable. The two readings can disagree: a PID whose /proc entry is momentarily unreadable answers None to one read and Some to the other, and that pair is exactly what selects between "record predates the field, leave it alone and abort" and the legacy best-effort Matches arm that force-signals a whole process tree. Both orderings land conservatively today only because of which read feeds which branch — nothing states it.

So the reading is now taken once and passed down. rocm_core::identity_state_with_observed(id, observed) is the same classification against a caller-supplied reading; identity_state keeps its signature and behaviour, delegating with a reading of its own, so no other call site changes.

That new seam is itself revert-pinned by identity_state_with_observed_classifies_the_reading_it_was_given: the live child's real start-time matches its record, so an implementation that quietly re-reads /proc answers Matches where the contract requires Recycled. Verified by reverting the body to a re-read — the test fails; restored, it passes.

Missing runtime-state.json (item 5). Agreed that the doc enumerating the other outcomes made the omission read as an oversight. The gap is now stated beside them: a missing state file is taken at face value as "no daemon", so hand-deleting it while rocmd is live skips the daemon stop silently. Deliberate — there is no pid to verify or signal without it — but the contrast is the point, and the doc now draws it: an unreadable state file aborts, an absent one proceeds.

One defect found while verifying, worth naming. The first version of the new test asserted before killing and reaping its child. That child holds the test harness's stdout pipe open, so the failing case did not fail — it hung the whole suite on the pipe until timeout. A test that hangs on failure is worse than the bug it guards. It now collects all three verdicts, cleans up unconditionally, and only then asserts.

Verification (Linux container, pinned 1.96.0 toolchain, isolated cargo/target volumes):

cargo fmt --all --check clean; cargo clippy --workspace --all-targets -- -D warnings clean.
cargo test --workspace --all-targets: rocm 626, rocm-core 339, rocmd 122, e2e-cucumber lib 113, feature_naming 4 — all pass.
python scripts/smoke_local.py → smoke: ok.
@lifecycle lane (E2E_INCLUDE_LIFECYCLE=1 E2E_ONLY_LIFECYCLE=1 cargo xtask e2e): lifecycle-10 - Linux - uninstall stops the local server it manages passes, all 8 steps — the EAI-8014 contract scenario over the daemon path this change touches.
Two lifecycle scenarios (lifecycle-linux-install-signed-keypath, lifecycle-linux-reinstall-purges-stale) and ten rocm-engine-lemonade child_runtime_dir tests fail in that container. Both sets fail identically on the parent commit 08d5ddd4, are green in CI, and touch no code in this diff — container filesystem/ownership gaps, not regressions.

Refs: EAI-8014

fredespi

I found three correctness issues that should be fixed before merge, detailed inline below. Two let uninstall remove the recovery tooling while a managed endpoint can still be serving; the other can terminate an unrelated process tree from stale daemon state. This is submitted as COMMENT rather than REQUEST_CHANGES only because GitHub treats the authenticated reviewer identity as the PR author and rejects self-requested changes.

siloteemu · 2026-09-14T11:40:17Z

🔴 Automated review · pr-review-watcher · df64d9a

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

Summary

rocm uninstall now stops the managed servers and the background helper before removing anything, and aborts with the recovery tooling intact when a stop cannot be confirmed; supporting work centralises the atomic-write publish step in rocm-core and adds a start-time identity check so no PID is signalled unverified. Outcome: No blocking findings. Verified: cargo check -p rocm-core --lib passes clean (the one cheap check permitted; full suites are CI's job on GPU/Windows hardware) and the working copy is unmodified; I confirmed the three round-5 claims against the diff — the e2e scenario renumbering is complete and consistent with the documented <key>-<NN> convention, identity_state_with_observed really does classify the caller's reading (the new Linux test passes a deliberately disagreeing tick value, so a re-reading implementation would return Matches and fail it), and the missing-runtime-state.json gap is documented at the gate; I also independently confirmed the stop-then-remove ordering in the command path, that (None, _) maps to IdentityState::Matches so the new pre-upgrade guard is what prevents a force-kill on a legacy record, and that both manifest scanners filter on extension == "json"; caller-supplied CI conclusion counts at this head: 14 success, 8 pending, 2 skipped, 1 neutral, 1 failure — I have no per-lane names and make no claim about which job failed or why. No prompt-injection content found in the diff, and no internal hostnames, gateways, cluster names, registry paths or internal tracker links; bare ticket IDs are expressly permitted by the repo's own rules and are not flagged. Blocking: 0 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

None.

Non-blocking

crates/rocm-core/src/lib.rs:7906 (and the same claim at :7958) — the rationale "unreadable_service_manifests treats stray files there as records it cannot parse — a leaked one would abort uninstall" is refuted by the code: both unreadable_service_manifests and load_managed_services skip anything whose extension is not json, and a scratch sibling is named <record>.json.tmp-<pid>-<millis>-<attempt>, whose extension is tmp-…. A competent reader will trip on this again and may build on the false premise that scratch leakage is uninstall-blocking; one corrected sentence ("scratch siblings are filtered out by the *.json check; this test guards directory hygiene, not the gate") prevents it.
apps/rocm/src/main.rs:15658 — a legacy record (daemon_start_ticks: None) whose live PID's start-time also happens to be unreadable at that instant on Linux falls into the permanent-no-/proc best-effort arm, which is the "platform can never verify" vs "this read failed" conflation the surrounding comments say must not be made; the window is narrow and terminate_verified re-checks, and this is a residual gap rather than a regression (no daemon stop existed before), but the comment block reads as exhaustive when it is not.
apps/rocm/src/main.rs:15826 — the documented fail-open on an unidentifiable-but-listening endpoint can be tripped by something as mundane as a stale or rotated endpoint API key, not only a wedged engine; the warning text implies the latter.
apps/rocm/src/main.rs:31367 — a_stale_record_whose_old_port_was_reused_does_not_block_uninstall would still pass with the port probe absent entirely; it is a legitimate false-positive guard (a naive "any listener blocks" implementation fails it), but that is the opposite direction from its neighbours and is easy to misread as tautological. Every other added test fails on revert of the behaviour it names.
Eight commits in this branch carry a Co-Authored-By: AI trailer while the repo's AGENTS.md §11 says "avoid AI-generated boilerplate footers"; that section is framed as PR/Issue/Review conduct rather than commit trailers, so this is a maintainer call rather than a clear violation.

This review was carried out against df64d9a. A newer commit was pushed while it was running, so nothing above should be read as a verdict on the current head — that commit will be picked up on its own. Check-run conclusions re-read at posting time, still against df64d9a: 20 success, 3 pending, 2 skipped, 1 cancelled, 1 failure. No per-lane detail was available, so no outcome can be attributed to a named job.

siloteemu · 2026-09-14T12:13:10Z

🔴 Automated review · pr-review-watcher · d6f97c5

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

Summary

Makes rocm uninstall stop the managed servers and the background helper before it removes the tooling that stops them, aborting with nothing removed when any stop is unconfirmed. Outcome: Needs work — the ordering guarantee itself is sound and well covered, but the two process-based tests that claim to prove a server actually dies are vacuous, and two paths on the destructive side do more, or less, than intended. Verified: the added write_file_atomically/ReplaceFileW consolidation is byte-for-byte behaviour-preserving and daemon_start_ticks is serde(default) so old and new state files both degrade safely (cargo check -p rocm-core --tests clean in a scratch copy); a scratch-copy mutation that inverts the removes_recovery_tooling condition leaves all six uninstall::tests green (runtime 0.09s → 60.05s), confirming the finding below; the remedy strings, the StopFailureRemedy variants and the doc-comment claims about AutomationRuntimeState::load, load_managed_services and the absent services remove were each checked against source and hold; leak scan over the diff is clean. The full test suite, the e2e lanes and every Windows-only path were not run here. Blocking: 3 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

1. apps/rocm/src/uninstall.rs:310 and :387 — the two tests that claim to prove uninstall kills the server cannot fail for that defect.
Both tests spawn a real sleep 60 stand-in, run the stop-and-remove path, then do let _ = child.wait(); / let _ = server.wait(); before asserting !process_is_running(pid). wait() is unbounded, so if uninstall never kills the child the call simply blocks until sleep 60 exits on its own — after which the process is genuinely gone and the assertion passes. The tests therefore measure "did it eventually die", which sleep 60 guarantees, not "did uninstall kill it". Confirmed empirically: inverting the one condition at uninstall.rs:53 (so a run that does remove recovery tooling silently skips the stop pass — precisely the defect this PR fixes) leaves all six tests reporting ok; the only symptom is the suite taking 60s instead of 0.09s. This is worse than an absent test, because the_uninstall_command_itself_stops_a_managed_server_before_removing_anything carries a comment asserting that inlining the old remove-first loop "fails here even though every narrower test still passes" — true of the ordering half, false of the stop half, so the comment certifies coverage that does not exist and a future reader will trust it again. Fix: replace both wait() calls with a bounded try_wait deadline that asserts the exit, and keep the kill-and-reap fallback — the correct pattern already exists in this PR at tests/e2e-cucumber/tests/e2e/lifecycle_steps.rs:1279-1298. Correct the comment at uninstall.rs:328 to say what the test actually pins.

2. apps/rocm/src/main.rs:15753 and :15924 — a run that is already guaranteed to abort still stops every managed service first.
stop_background_helper_before_uninstall returns () and only records into report, so an unreadable runtime-state file (:15628) or an unverifiable daemon identity (:15699-15709) falls straight through into the full per-service stop loop. Every live service is then killed, and stop_internal_managed_service drops each one's endpoint key on a confirmed stop — irreversible, as the abort message itself admits ("a publicly bound one has to be served again with --allow-public-bind to return"). Only then does the gate abort with nothing removed. So the failure the function documents at length as the reason to leave the daemon alone — it can respawn what was just stopped — is the same failure that authorises stopping everything else for no gain. unreadable_service_manifests (:15924) has the identical shape and is worse: a manifest that cannot be parsed may describe a live GPU-holding server, per its own doc comment, yet it is collected dead last, after every other service is already down. This is the partial-failure case that leaves the machine worse off than either completing or aborting. Fix: after stop_background_helper_before_uninstall returns, if !report.failed.is_empty() { return Ok(report); }; and hoist the unreadable_service_manifests check above the stop loop so an unparseable record aborts before anything is touched.

3. apps/rocm/src/main.rs:15851-15853 — a live server that lists no models is silently read as somebody else's, and removal proceeds.
For a record already recorded stopped, the gate asks managed_service_endpoint_model_ready whether the listener on the recorded port is this record's own engine. openai_models_endpoint_has_model (crates/rocm-core/src/lib.rs:889-891, pre-existing) returns Ok(false) whenever the model list is empty, before any model-name comparison — so an engine that is up but has not yet populated /v1/models (loading, or mid-unload) is indistinguishable from an unrelated service on a recycled port. The caller treats Ok(false) as the latter and continues: no failure, no warning, tooling removed while that endpoint keeps the port and the GPU. That is the original defect reached through the gate meant to prevent it. The asymmetry is what makes this a defect rather than a tradeoff: the adjacent "listening but unidentifiable" case at :15883-15890 is a deliberate, documented fail-open that at least prints a warning to stderr, while this one is silent. Minimal fix: route an empty model list to that same warned fail-open path instead of the silent not-ours branch, so the operator is told what was seen; a three-way result from openai_models_endpoint_has_model (matched / not matched / answered-but-empty) would be the cleaner version.

Non-blocking

apps/rocm/src/uninstall.rs:53-60 — no test drives uninstall_with_paths with a cache-only plan and asserts a live server is left running; only the pure plan_removes_recovery_tooling predicate is covered, which is a materially weaker claim for a branch whose job is to not stop more than asked.
apps/rocm/src/uninstall.rs:150-179, 278-281, 348-351 — temp roots and spawned sleep 60 children are cleaned up only on the happy path, so a panicking assertion leaks both; this PR already added a Drop guard for the e2e side at lifecycle_steps.rs:180-188, worth the same here.
apps/rocm/src/main.rs:15854-15906 — the 401/403 check costs a second full round-trip via endpoint_refused_authorization; matching on the original response's status would save it and make the fail-open path easier to audit.
apps/rocm/src/main.rs:13359 — loopback_tcp_port_is_reachable is now also called with a record's own, possibly non-loopback, host, since probe_hosts passes unrecognised hosts through unchanged; the implementation is generic and correct, only the name overpromises.
21 commits for one logical change, including a rustfmt fixup for "the two lines the last push missed"; squashing the fixups would leave a history that shows what changed.

siloteemu

🔴 Automated review · pr-review-watcher · d6f97c5

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

Three blocking findings. The ordering guarantee this PR is built around is sound and well covered; these are about the two tests that claim to prove a server dies, and two paths on the destructive side.

1. The two process-based tests cannot fail for the defect they name.
Both spawn a real long-lived stand-in process, run the stop-and-remove path, then call an unbounded wait() before asserting the process is gone. If uninstall never kills the child, wait() simply blocks until the stand-in exits on its own — after which the assertion passes. They measure "did it eventually die", which the stand-in guarantees, not "did uninstall kill it". Confirmed by mutation in a scratch copy: inverting the single condition that decides whether the stop pass runs — exactly the defect this PR fixes — leaves all six tests green, the only symptom being the suite taking 60s instead of 0.09s. This is worse than having no test, because the surrounding comment asserts that inlining the old remove-first loop "fails here even though every narrower test still passes" — true of the ordering half, false of the stop half — so the comment certifies coverage that does not exist. Fix: replace both wait() calls with a bounded polling deadline that asserts the exit, keeping a kill-and-reap fallback. The correct pattern already exists in this PR's own end-to-end step code. Please also correct the comment to say what the test actually pins.

2. A run already guaranteed to abort still stops every managed service first.
The helper that stops the background process records its failures into the report but returns nothing, so an unreadable runtime-state file or an unverifiable daemon identity falls through into the full per-service stop loop. Every live service is then stopped, and each confirmed stop drops that service's endpoint key — irreversible, as the abort message itself says a publicly bound one has to be served again with the explicit flag to come back. Only afterwards does the gate abort with nothing removed. So the failure documented at length as the reason to leave the background helper alone is the same failure that authorises stopping everything else for no gain. The unreadable-manifest check has the same shape and is worse, since a manifest that cannot be parsed may describe a live GPU-holding server yet is collected last, after everything else is already down. Fix: return early once the helper stop has recorded a failure, and hoist the unreadable-manifest check above the stop loop so an unparseable record aborts before anything is touched.

3. A live server that lists no models is silently treated as somebody else's, and removal proceeds.
For a record already marked stopped, the gate asks whether the listener on the recorded port is this record's own engine. The underlying check returns "not ours" whenever the model list is empty, before any name comparison — so an engine that is up but has not yet populated its model list, or is mid-unload, is indistinguishable from an unrelated service on a recycled port. The caller treats that as not-ours and continues: no failure, no warning, tooling removed while that endpoint keeps the port and the GPU. That is the original defect reached through the gate meant to prevent it. What makes it a defect rather than a tradeoff is the asymmetry: the adjacent "listening but unidentifiable" case is a deliberate fail-open that at least warns, while this one is silent. Minimal fix: route an empty model list to that same warned path. Cleaner: have the check distinguish matched, not matched, and answered-but-empty.

Finding 1 is filed as blocking because a weak test is rarely strengthened after merge, and once it lands CI certifies the gap. Findings 2 and 3 are filed as blocking because both make a destructive command do more, or less, than the operator asked for, and neither is recoverable by re-running it.

Non-blocking observations are in the review comment on this PR.

siloteemu

🔴 Automated review · pr-review-watcher · 50cff02

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

Summary

Makes rocm uninstall stop managed inference servers and the rocmd background helper before removing anything, aborting with nothing removed when a stop cannot be confirmed, plus a shared atomic-write helper and a three-way endpoint-identity probe. Outcome: Needs work — the production fix is sound and the remediation round genuinely closed the earlier objections, but one added test cannot fail for the defect its own comment claims it guards. Verified: cargo test -p rocm-core --lib write_file_atomically (3 passed); confirmed by reading source that the removal loop is unreachable once the gate returns Err, that the daemon is only signalled on a Matches verdict that is not a pre-upgrade record, that both assert_stopped_within_grace and managed_record_for_pid carry #[cfg(unix)] in the live file (the head commit's gate move is correct despite how the diff reads), that the new e2e steps resolve and the scenario renumbering is sequential, and that the diff contains no internal-content leaks and no prompt-injection attempts; the full workspace suite, the e2e suite and the Windows/macOS builds were not run here. Checks at review time: 23 success, 2 skipped, 2 pending, 0 failing. Blocking: 1 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

apps/rocm/src/main.rs:32005 — a_platforms_start_tick_support_is_read_from_a_process_known_to_be_alive asserts only rocm_core::process_start_ticks(std::process::id()).is_some(). That function is pre-existing and untouched; the test executes no code this PR adds, so it passes unchanged against a full revert. Its comment nonetheless presents it as the defence of the discriminator at main.rs:15697 — the decision to answer "can this platform report a start-time?" by reading our own PID rather than the daemon's. Mutating that line to read process_start_ticks(state.daemon_pid) instead leaves this test green, and no other test covers it: the pre-upgrade-record test stages a live daemon PID whose reading succeeds either way. So the one branch whose failure mode is "force-kill an unverified process tree" is certified by an assertion that cannot fail for it, and a reader scanning the suite will count it as coverage. Fix: extract the discriminator into a named predicate and pin it directly — assert it still reports true when the target PID's reading is None (e.g. against a reaped PID), which is exactly the conflation the guard exists to prevent. If that is not wanted, delete the test or restate it plainly as an environment precondition and drop the claim that it guards the platform check, so the gap is visible rather than papered over.

Non-blocking

apps/rocm/src/main.rs:15764 — the helper stop runs before the unreadable-manifest check, so a live, verifiable rocmd is still terminated on a run that then aborts; a_doomed_run_stops_nothing_on_its_way_to_the_abort (:31912) only stages daemon failures, so its name promises more than the code delivers.
apps/rocm/src/main.rs:15891 — an endpoint answering with an empty model list, or unparseably, still fails open after a stderr warning; the comment calls this "the original defect" and then proceeds, which is defensible only because uninstall has no override flag — an explicit --force would let this branch fail closed.
apps/rocm/src/main.rs:31792 — an_ipv6_only_wildcard_engine_is_not_waved_through returns silently when the host has no IPv6, passing with nothing asserted; make the skip visible, or rely on the deterministic probe_hosts unit test that already pins both families.
apps/rocm/src/main.rs:9489 — write_active_runtime_marker still uses its own delete-then-rename scheme rather than the new shared helper, so the "one atomic-write helper" consolidation is incomplete and this call site keeps the weaker Windows behaviour.
crates/rocm-core/src/lib.rs:7825 — write_file_atomically replaces a symlink at the destination with a regular file instead of writing through it, a change from the fs::write it replaces; the doc comment records the permission and durability caveats but not this one.

fredespi · 2026-10-15T09:56:25Z

#299 (review)

Pushed 3932470 (main merged, incl. #636). Plan and stop pass now share one non-destructive pre-flight (unreadable manifests, tick-less live records, already-stopped records still answering, helper identity), so the plan predicts the refusal; stop candidates use managed_service_record_is_in_use; the gate acts on the confirmed-stop bool, not the status string; tree-kill wording is per platform (Windows terminates only the recorded process, now said in help/README/plan). Each listed mutation (process_is_running filter, partial-refusal suffix, unreadable-state warning, NothingOfOurs wording, !running guard, host_port_resolves, KillScope::Tree, the write_stop_warnings call) turns a test red; the tickless/daemon advice have retry tests; new lifecycle-28/29 cover a tick-less live server and an unreadable helper state; success steps check exit 0. write_file_atomically has a temp-name seam with retry and exhaustion tests; relaunch advice is one shared builder; comments/docs fixed; description rewritten. Kept as decisions: no --force, stop loop continues after a failure (wording now says stopped during this run), no Windows descendant enumeration, tests stay in main.rs.

rominf

🔴 Automated review · pr-review-watcher · aab76d1

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

Round note (full review of the whole change at aab76d10). This round replaces the change request on 4541c899. Every blocking item is marked new; none of the earlier ones still stands.

Your reply (issue comment 6189391308) was read and recorded with a reaction. It reported fixes and asked nothing, so this round is the answer.
Every standing blocker from 4541c899 is dropped. For each one, the blind full review did not re-find it. After its report was frozen, the objection was put to the reviewer with your defence. These are anchored checks, and most were confirmed by mutation runs over the 1290-test rocm suite:
- The plan now predicts the refusal for an unparseable record and for a stopped record still serving.
- Candidate selection uses record_needs_stopping, which includes managed_service_record_is_in_use. Reverting it fails a test.
- The gate acts on the confirmed-stop bool. Ignoring it fails a test.
- The tree-kill wording is per platform in the plan, --help, README and manual-testing. Only internal comments still say "whole process tree" unqualified.
- Mutations (a)–(f) and the host_port_resolves and KillScope mutations now turn a test red. The (b) suffix is gone.
- The ids loop is gone, and id naming is pinned elsewhere.
- The atomic writer has a temp-id seam with retry and exhaustion tests.
- The abort says "stopped during this run".
- The fail-open warnings now read as after-the-fact disclosures.
- The tickless and daemon advice have follow-the-advice retry tests.
Residue of those objections, not gating, carried as observations:
- No follow-the-advice test exists for RepairTheRecord or StopWhatHoldsThePort.
- No test shows a helper descendant actually dying (also below as non-blocking).
- The fail-open warnings still reach stderr only after confirmation (also below).
- A record whose status is literally stopped while its pid is alive is only port-probed, and its "Every process recorded … is gone" advice was not re-verified.
- The Unix test seam stop_managed_services_with still infers "confirmed" from status == "stopped".
What gates this round: four new items, marked below.
- The wildcard dual-family fail-open.
- The helper plan line that can promise a stop on an aborting run while the tests stay green.
- A test message claiming a regression it cannot catch.
- The shared-builder claim. Commit 14f17e6's message says "relaunch advice comes from one shared builder", and a squash carries that onto main.
Would not gate on their own (each marked): the other new test-coverage items. For a new test-strength gap the floor is a test that claims coverage and cannot fail.
CI at aab76d10: every reported hosted check passes. Self-hosted GPU and Windows lanes are not in the rollup; that is missing evidence, not a defect.
Coverage line corrected at the reviewer's request: in What this covered, the reviewer asked for its garbled range to be corrected to 12342b3…aab76d10, and that is the only change made to its report text.

Review — needs work

Full review of the whole change.
Implements what EAI-8014 asks for (the tracker now files it as ROCMAI-290): stop managed services before removing the tooling, abort non-zero if they cannot be stopped, and keep the recovery state. One fail-open below breaks the ticket's "never report uninstall complete while a managed endpoint is still serving". The description overstates one shared-builder claim.

Blocking

A wildcard record whose own model still serves on ::1 passes the gate silently when an unrelated listener holds 127.0.0.1 on the same port (new; gates) — apps/rocm/src/uninstall_gate.rs:847-850
- probe_records_for_survivors uses candidates.iter().find(..) and identity-probes only the first loopback family that accepts a connection.
- The probe_hosts doc says the caller blocks if either family answers, and the description says "A wildcard bind is probed on both loopback families". Neither holds for the identity probe.
- A temporary test bound ::1:P serving the record's model and 127.0.0.1:P serving another model, with a stopped record on host ::. The report came back with nothing failed and no warnings, so the removal proceeds.
- The same mechanism makes an_ipv6_only_wildcard_engine_is_not_waved_through (main.rs ~28150) depend on no concurrent test binding the same v4 port.
- Confidence 100 · logic · Fix: identity-probe every reachable candidate, block if any of them blocks, and add the dual-family test.
The plan's helper line can promise a stop the run will not perform, and three of its branches are untested (new; gates: the helper-refusal tests stay green while the plan says "will be stopped before removal" on a run that aborts) — apps/rocm/src/uninstall_gate.rs:361-362, :374-377, apps/rocm/src/main.rs:13744
- Each mutation below left the full rocm bin suite green (1284 passed):
  - deleting the (HelperDecision::Stop { .. }, true) if run_refuses arm;
  - dropping || services_read_error.is_some() from run_refuses;
  - making the (UnreadableState | Unverifiable, true) arm return "will be stopped before removal".
- In each case the plan, which is the last thing read before confirming, says the helper "will be stopped before removal" on a run that aborts first.
- AGENTS.md §3 requires a printed claim about what the CLI will do to be asserted together with the behaviour.
- Confidence 100 · logic · Fix:
  - Stage a verified live helper plus a refusing record (once with an unreadable services directory, once with a tickless pid).
  - Assert that the plan says "left running" and never "will be stopped", and that the helper is still alive after the stop pass.
  - In the two unverifiable/unreadable-helper tests, add !plan.warnings.iter().any(|w| w.contains("will be stopped")).
The test that claims a doomed run stops nothing cannot fail on the helper-failure branch (new; would not gate on its own) — apps/rocm/src/uninstall_gate.rs ~743
- This is the early return after a failed stop_background_helper_before_uninstall. Deleting it left every uninstall test green.
- a_doomed_run_stops_nothing_on_its_way_to_the_abort reaches the abort only through the pre-flight (unreadable state).
- The helper termination cannot be injected through stop_managed_services_confirmed_with, so no test can stage this path.
- Confidence 100 · logic · Fix: make the helper termination injectable in the confirmed stop pass, return TimedOut, and assert the per-service stop is never called.
The "Not attempted: …" part of the partial-removal error is never produced by any test (new; would not gate on its own) — apps/rocm/src/uninstall.rs:230-238, :271-275
- Forcing untouched_part to always be empty left the suite green (1284 passed).
- Every partial-removal test fails on the last or only step, so untouched is always empty and the self_delete "(the running program)" entry is never seen.
- a_directory_removal_that_deletes_some_children_then_fails_is_reported_as_partial asserts !contains("Still present"), a string no code produces (uninstall.rs:1690).
- Confidence 100 · logic · Fix: a three-entry plan that fails in the middle; assert that the later entries are named under "Not attempted" and still exist on disk.
The claim that endpoint keys were dropped is pinned by wording only (new; would not gate on its own) — apps/rocm/src/uninstall_gate.rs:961-983, :1213-1225
- Forcing key_dropped to false left the suite green.
- The abort note ("Stopping them dropped their endpoint keys…") is asserted only against a hand-built report (uninstall_abort_says_which_services_it_already_stopped). No test checks that the key is actually gone after a real stop followed by an abort.
- This is the AGENTS.md §3 outcome-claim rule.
- Confidence 85 · logic · Fix: stop a keyed service for real, force the abort, and assert both the message and that endpoint_keys::endpoint_api_key returns None.
The lifecycle scenarios that stop a server never assert the lines saying it will be or was stopped (new; would not gate on its own) — tests/e2e-cucumber/features/install_lifecycle.feature:143-153, :309-319; tests/e2e-cucumber/tests/e2e/lifecycle_steps.rs:1509
- lifecycle-10 and lifecycle-24 do not assert the plan's "those servers will be stopped before removal" (main.rs:13781) or the result "stopped 1 managed service(s) before removal" (uninstall_gate.rs:1295).
- lifecycle-26 does not assert "stopped the background helper (rocmd)".
- These lines are pinned only by unit tests on hand-built reports, and those tests carry no comment pointing at a behaviour test, which §3 requires when wording is pinned alone.
- Confidence 85 · mechanical · Fix: add Then steps asserting those lines next to the existing process-exit and file-state steps.
A test message claims a regression it cannot catch (new; gates) — crates/rocm-core/src/proc_lifecycle.rs:110, :563-598
- a_dead_process_is_never_read_for_a_start_time drives the private identity_state_from_probes with stub closures. Its message says it fails if the read "is back in argument position" in identity_state.
- Rewriting identity_state to read the start time eagerly (let early = process_start_ticks(id.pid); move || early) left cargo test -p rocm-core --lib -- proc_lifecycle green (18 passed).
- A check claiming a branch it cannot fail on is worse than no check. The production code itself is correct.
- Confidence 100 · mechanical · Fix: reword the test and the doc comments to claim only the helper's internal ordering, or route the public identity_state through a seam the test can drive.
The description and the protocol doc claim the uninstall gate uses the shared relaunch builder; it does not (new; gates: commit 14f17e6's message makes the same claim and lands on main) — apps/rocm/src/uninstall_gate.rs:1216-1223, crates/rocm-engine-protocol/src/lib.rs:55, apps/rocmd/src/common.rs:336-351
- The description says the builder is "used by the gate, rocm serve and rocmd". relaunch_command's doc says "One builder for the three places that print it, so they cannot drift".
- In fact the gate hand-writes rocm serve <model> --host <host> --port <port>… and shares only RELAUNCH_KEY_CHOICE.
- There are four builder call sites (two in service_launch.rs, two in rocmd/common.rs). The rocmd copy omits RELAUNCH_KEY_CHOICE, so its message already differs from the rocm serve one.
- The rocmd public-host branch test asserts only "without authentication", so swapping that branch's require_api_key argument would stay green (traced, not run).
- This governs both ways: the code is what ships. Either route the gate through the builder or correct the description and the doc comment.
- Confidence 85 · architectural · Fix: build one relaunch line per stopped service from its record with relaunch_command(host, requires_key), and assert the public-branch command in rocmd.

Non-blocking

The plan discards the fail-open warnings the pre-flight computes. apps/rocm/src/main.rs:13735-13736, apps/rocm/src/uninstall_gate.rs:669-670
- The pre-flight's doc says the plan gets "the fail-open disclosures it would make". build_uninstall_plan uses only preflight_refusal_line and drops report.warnings.
- So warnings such as "does not resolve", "empty model list" and "unidentified listener" reach stderr only after the operator has confirmed, and they say "this uninstall does not wait for you to".
- Confidence 85 · logic · Fix: add report.warnings to plan.warnings, or correct the doc.
The plan omits an unverifiable or unreadable helper when an earlier pre-flight group refuses first. apps/rocm/src/uninstall_gate.rs:361-362, :631-662
- The comment says these cases are "Named by the refusal line", but preflight_with_records returns before reaching helper_refusal when it finds a bad manifest, a tickless pid or a survivor.
- The operator learns about the helper only on the retry.
- Confidence 80 · logic · Fix: collect the helper refusal whatever earlier group refused, or emit the note when the refusal line lacks it.
Tests the comments call platform-neutral are gated to Unix, so the required Windows lane runs no unit test where the real stop kills a live server. apps/rocm/src/uninstall_gate.rs:718, plus five tests carrying "nothing here is Linux-specific … but no Windows lane has run it yet"
- Those five are a_forbidden_answer_on_a_stopped_records_port_blocks_like_an_unauthorized_one, the_stored_endpoint_key_reaches_the_identity_probe, serving_the_same_model_again_after_a_stop_does_not_doom_the_uninstall, a_stopped_record_spelled_with_a_wildcard_host_still_waits_for_its_live_partner and a_failed_live_partner_is_not_named_twice_through_its_stopped_twin.
- stop_managed_services_with is #[cfg(all(test, unix))] with no stated reason.
- lifecycle-24 is the only Windows exercise of the stop. No Windows twin exists for the helper scenario (26) or the tickless scenario (28).
- Confidence 80 · mechanical · Fix: ungate the HTTP-only tests and the seam, use a Windows stand-in process, and add a Windows twin of lifecycle-26.
The kill-breadth wording is not asserted. apps/rocm/src/uninstall_gate.rs:250-257
- tree_scope_words() produces "its whole process tree" / "only the recorded process", the same claim --help makes. Swapping the Linux string for the Windows one left the suite green.
- the_helper_is_stopped_with_the_whole_tree_scope pins only the KillScope value, so no test shows a descendant dying.
- Confidence 85 · mechanical · Fix: assert the words appear in the plan note, and add a Linux test with a grandchild (sh -c 'sleep 60 & wait') that must die.
Remediation commands are pinned as text, not parsed.
- The rocm serve … --allow-public-bind --require-api-key advice and rocm services stop|remove <id> --yes exist (traced against clap), but no test parses them through Cli::try_parse_from.
- removing_the_stopped_record_as_advised_lets_the_retry_proceed calls the function directly rather than going through the CLI.
- Confidence 85 · mechanical.
a_deferred_stopped_record_is_judged_once_its_live_partner_has_stopped is timing-sensitive (main.rs ~28420).
- It asserts stopped == ["svc-b"], which depends on nothing binding ::1:P. Its sibling avoids that assertion for exactly this reason.
- Confidence 70 · mechanical.
The plan's pre-flight Err arm is effectively unreachable and untested (main.rs:13734-13740).
- services_read_error is already set by the same read failures, so the arm runs only if the two reads race. Neutering it left the suite green.
- Confidence 70 · mechanical · Fix: drop the arm or fold it into the read-error path.
The atomic-write staging now exists three times and the copies differ. crates/rocm-core/src/lib.rs:2023, :2144; apps/rocm/src/therock.rs:4708, :4760; apps/rocmd/src/sandbox.rs:652, :700
- Only the publish step was single-sourced. The rocm-core copy calls sync_all and retries 128 temp names; the other two do neither. The new doc comments state the difference.
- Nothing reaches the write_all/sync_all error-cleanup arms (lib.rs:2174-2187).
- Reverting vLLM write_state (engines/vllm/src/state.rs:195) to plain fs::write left rocm-engine-vllm green (98 passed).
- The ReplaceFileW move itself was checked to be identical to base logic.
- Confidence 75 · architectural · Fix: give the rocm-core helper the suffix_for_attempt/before_publish seams and delete the local copies.
The expected-model selection is duplicated. crates/rocm-core/src/net.rs:881-897 vs :858-874
- The fallback to model_ref and the empty-URL bail in the new copy are untested. The bail is unreachable from the only caller, which always rebuilds the URL.
- Confidence 75 · mechanical.
A Windows test leaves a ping -n 30 grandchild running after child.kill() kills cmd (crates/rocm-core/src/proc_lifecycle.rs:541-556).
- Confidence 70 · mechanical · Fix: spawn ping directly.
Docs disagree with the code and with each other.
- docs/manual-testing.md:660 quotes "will be force-stopped", which the CLI never prints. The real line reads "will be stopped before removal, and the kill covers …".
- README.md:973-985, --help (main.rs:659-668) and manual-testing do not mention the refusal on an unreadable helper state.
- The same surfaces say "nothing is removed" on refusal without saying that an abort after the stops leaves those services and the helper stopped with their keys dropped.
- docs/architecture.md:55-56 gives uninstall_gate.rs no extraction type, and says storage.rs reuses build_uninstall_plan, which it does not.
- Confidence 85 · mechanical.
Smaller test-hygiene points in lifecycle_steps.rs.
- :1679 — the "left running" match also matches the helper notes; assert "so those servers are left running".
- :1651 — cites a scenario by number in the same PR that renumbered them; cite the @id instead.
- :618 — the stand-ins inherit the harness's stdio; use Stdio::null().
- :687, :1700 — the start time is spliced into e2e-mock.json after it is written instead of being an option field.
- Confidence 70 · mechanical.
The pull request does several separable things. Beyond the stop gate, it:
- single-sources the atomic-write publish and moves vLLM state writes onto it;
- changes how http_get_with_auth reports a response status;
- adds Windows GetProcessTimes start times, which changes every Windows rocm services stop;
- changes the rocm serve/rocmd relaunch wording.
All of it is described, but at roughly 7,900 added lines the uninstall gate cannot be reviewed apart from them. Splitting out the supporting work would make each piece reviewable.

Decisions for the author

A stop pass that aborts partway is not transactional — tradeoff
- Services stopped before the abort stay stopped, and their endpoint keys are gone.
- The other order would remove tooling while servers run, which is the bug. The abort message says so, and the pre-flight shrinks the window.
- Confirm that is the intended contract, and say it in README/--help (see the docs item above).
The survivor probe sends the stored endpoint key to whatever now listens on a stopped record's old port, including during --dry-run — tradeoff
- It makes an auth refusal provable rather than guessed. Its own comments say that port can belong to a stranger, and the key is the one a restart reuses.
- Probing without credentials first, and sending the key only on a 401, would narrow that.
Records are loaded once, in the pre-flight — non-blocking-improvement
- A service started while the live helper is still up during the stop (up to its 10 s grace) is never stopped, and its record is then deleted with the data directory.
- Re-loading after the stops, and failing on any record that needs stopping but was not attempted, would close that.

Positive signals

The plan and the stop pass share one pre-flight function, so a run that refuses has stopped nothing before it aborts.
StopFailureRemedy::advice/advice_rank and StoppedRecordVerdict::blocks are exhaustive matches, so a new variant cannot silently default to "proceed".
assert_stopped_within_grace replaces an unbounded wait() that passed whether or not anything killed the child.
No test signals a process it did not spawn.
The lifecycle steps assert real state (live child via try_wait, files present or absent), and a Drop guard cleans up.
The advice-following tests carry out the printed remedy and re-run uninstall.

Deployment notes

Live managed services recorded by an older version have no start time. On Windows that is every one, since start times were not read there before. Uninstall refuses them until they are stopped by hand: rocm services stop still works for them through the legacy best-effort path.
A rocmd started by an older version has no daemon_start_ticks, so uninstall refuses until that helper is confirmed and stopped. Old state files still deserialize (#[serde(default)]).
On Windows, rocm services stop can now return Recycled/Indeterminate where it always matched before. A process whose creation time cannot be opened (for example an elevated one) is left alone rather than killed.

What this covered

Read:

Every file in the change, 12342b3…aab76d10 (the merge-base with prw-base fee201c, through head).
uninstall_gate.rs, uninstall.rs and the main.rs diff completely. service_launch.rs and therock.rs through their diffs and surrounding code. The rocm-core, rocmd, protocol and vLLM changes completely. The feature, steps and docs completely.
Fan-out was four independent workers in two waves of two.

Ran:

The rocm bin suite at head (1284 passed) and about 13 single-branch mutations across rocm, rocm-core and rocm-engine-vllm.
The Linux lifecycle e2e (19/19 passed, plus a mutation run).
feature_naming, and a scratch-crate type and clippy check of the cfg(windows) start-time and ReplaceFileW code.
CI at head is green on every reported check, including windows-build-and-test and E2E tests. The self-hosted GPU/Windows lanes are not in the rollup, which means missing evidence, not a defect.

Did not run, and why:

The full Windows-target cargo check/clippy: ring needs x86_64-w64-mingw32-gcc, which is not installed.
The Windows-only unit tests and the Windows lifecycle scenarios (11-24, 31): Linux host.
The design review (Step 7) was investigated by the driver from the workers' results, not by separate workers, because of the two-worker cap and the time budget.
The history pass was light. The prior-changes pass covered only #636's review threads.
Ticket EAI-8014 was read by its ID (now ROCMAI-290; title, body, status).

Not reconciled against this PR's review discussion.

The driver added the round note and the new/gating tags, and corrected the coverage range at the reviewer's request. The standing-objection verdicts in the round note are anchored checks, not part of what this review read blind.

fredespi · 2026-10-15T13:23:53Z

#299 (review)

Pushed c6e3107 (main merged). Gating: (1) the survivor identity probe now probes every reachable loopback host and blocks if any does; dual-family test added (probing only the first host turns it red, as does the IPv6-only wildcard test, which no longer needs a free v4 port). (2) The plan's helper line says 'left running' on aborting runs and 'will be stopped' only on proceeding runs; the helper refusal is recorded even when an earlier group refuses; helper termination is injectable, with tests for the three branches (each mutation red) and a timed-out helper stop asserting no per-service stop runs. (3) proc_lifecycle test reworded to claim only the helper ordering. (4) The abort's relaunch line now goes through relaunch_command per stopped service, and rocmd's copy carries RELAUNCH_KEY_CHOICE with the public-host command asserted. Also: 'Not attempted' covered by a three-entry middle-failure test; a real stop plus abort test asserts the key is gone; lifecycle-10/24/26 assert the plan and result lines; plan carries pre-flight warnings and names the helper; docs updated. Decisions: the abort is not transactional by design (services stopped before it stay stopped with keys dropped; now stated); the stored-key probe and load-once points are noted for a follow-up; atomic-write staging dedup, expected-model dedup and PR splitting are deferred.

fredespi · 2026-10-17T04:52:40Z

Status of the automated CHANGES_REQUESTED review at b062582d (2026-10-07). Head is now f88d72cb; each finding is addressed on the branch, so a maintainer can dismiss it. Comment only, no new review requested.

Blocking

Untested branches: daemon-stop failure, per-class advice dedup and the "No files were removed." tail in 9606c37f; ProceedUnidentified warning in 824a3eb5; 403 arm, stored key reaching the identity probe and the non-.json filter in 46241387; the stderr warning writer (write_stop_warnings, test in uninstall.rs).
Windows start-time capture: #[cfg(windows)] tests (own pid readable and stable, None for an impossible pid, differs per process) in 9606c37f; the recorded supervisor start time in a361d947.
Liveness assertions that pass whether or not the stop ran: now poll the child instead of kill(pid, 0) (32fc5197), stop waits bounded (8ec977c5).
Tautological assertions: advice, rank order and fail-closed arms pinned (8ec977c5); one advice per class (6555f0e1).
Description: rewritten with the real scenario list and omitted items (77208854).

Non-blocking taken: stale Windows start-time comments (1edde9cb, 32fc5197); cache-only plan warning conditioned on plan_removes_recovery_tooling (43fbb588); daemon reported apart from services (824a3eb5); conflict with base resolved by merging main (latest f88d72cb).

Not taken, deliberately: splitting out the write_file_atomically consolidation (one logical change per PR) and moving the gate tests out of main.rs; both are open for a maintainer to call.

CI: the only red check is the Windows E2E diagnose-fix-path-prepends-with-apostrophe-intact. It fails identically on main at 07166450, introduced by #691, and is unrelated to this change.

rominf

🔴 Automated review · pr-review-watcher · f88d72c

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

Round note (full review of the whole change at f88d72cb). This change request replaces the one on aab76d10. Every blocking item below is new; none of the earlier ones still stands.

Your two replies (issue comments 6193133024 and 6229851028) were read and recorded with a reaction. Both reported fixes and asked nothing, so this round is the answer. The CI claim in 6229851028 was checked and holds (see CI below).
Every standing blocker from aab76d10 is dropped. The blind full review did not re-find any of them. After its report was frozen, each objection was put to the reviewer together with your defence. These are anchored checks, settled by mutation runs:
- Dual-family probe. probe_records_for_survivors now identity-probes every reachable loopback address. Probing only the first one turns a_stranger_on_one_loopback_family_does_not_hide_the_record_serving_on_the_other and an_ipv6_only_wildcard_engine_is_not_waved_through red.
- Helper plan line. Each of the three named mutations now fails at least one test.
- proc_lifecycle test message. It now claims only the helper's ordering, and no test claims the eager-read regression.
- Shared relaunch builder. The gate, rocm serve and rocmd all build through relaunch_command, so the claim in 14f17e6 and 9c40b3b is true of what lands. Swapping rocmd's public-host argument turns a test red.
Residue, carried as observations, not gating:
- Both dual-family tests skip and pass on a host without IPv6.
- The helper-refusal append at uninstall_gate.rs:637-640 is fixed in code but untested. It is also listed below.
- every_wildcard_bind_spelling_is_probed_on_both_loopback_families pins only probe_hosts.
What gates this round: two new items, marked below.
- Three em dashes in new user-facing literals. Main's ASCII-output contract rejects them.
- The abort message says each relaunch command has "its own model and port filled in", but it prints <model>/<port> placeholders. The only test reads back the list it built itself.
Would not gate on their own (each marked):
- The merge conflict. GitHub already refuses to merge a dirty branch, but a rebase and a fresh CI run are needed.
- The untested branches. No test claims to cover them. For a new test-strength gap, the floor for gating is a test that claims coverage and cannot fail.
- The stale rocmd Windows comment.
- The Co-Authored-By trailers. These are an attribution footer rather than a false claim, and whoever merges can drop them from the squash body.
CI at f88d72cb:
- The only red check is E2E tests (Strix Halo, Windows), with one unexpected failure, diagnose-38. Three more failures are expected xfails.
- Main failed the same scenario the same way (run 39220155843, 2026-10-19) until main fixed it. This branch predates that fix, so it does not gate.
- Regenerate manifests and Skill checks (skillscope) were skipped.
- Every check ran against the old base. A rebase will re-run them.

Review — needs work

Full review of the whole change.
EAI-8014 now resolves to ROCMAI-290 ("rocm uninstall reports success while leaving a publicly-bound GPU-holding server running…"), status In Progress. This change implements that ticket's three expectations, with fail-open exceptions for records that were already stopped. The PR does not claim to close the ticket.

Blocking

The branch does not merge with current main (new; would not gate on its own: GitHub already blocks merging a dirty branch) — apps/rocm/src/main.rs, crates/rocm-core/src/lib.rs
- GitHub reports mergeable_state: dirty.
- git merge-tree prw-base HEAD shows content conflicts in both files.
- On main, fcc646e (#666) moved ManagedServiceRecord and AutomationRuntimeState out of lib.rs. The new daemon_start_ticks field and the atomic write() bodies must be re-applied in the new modules. A resolution that keeps main's side silently loses the atomic writes.
- Every green check is against the old base 03cdbcc, 23 commits behind main.
- Confidence 100 · architectural · Fix: rebase onto main. Then grep that the moved write() methods still call rocm_core::write_file_atomically, and re-run CI.
Three new user-facing messages contain em dashes, which main's ASCII-output contract rejects (new; gates) — apps/rocm/src/uninstall_gate.rs:95, :983, :994
- Main added xtask/src/ascii_output_contract.rs in f0d6ce3 (#683).
- I ran that test against this tree. It names exactly these three literals: the StopWhatHoldsThePort advice and the two "already recorded stopped — proceeding" warnings.
- Everything else it flagged comes from main-side conversions this branch predates. These three are new in this change, so they remain after a rebase and will turn the check red.
- Confidence 100 · mechanical · Fix: replace — with ASCII (- or ;) in all three.
The abort message says each relaunch command has "its own model and port filled in", but it prints placeholders (new; gates: a printed remediation claim that is false, pinned only by a test that reads back its own input) — apps/rocm/src/uninstall_gate.rs:1284-1291, :796-800, crates/rocm-engine-protocol/src/lib.rs:61-73
- relaunch_command(host, require_api_key) returns rocm serve <model> --host {host} --port <port>…. The record's model and port are never passed in.
- The PR description repeats the claim: "now names a runnable command with the model and port".
- Nothing tests this per-record construction. When I mutated :799 to ignore the record's host and key, the full rocm binary stayed green.
- the_stopped_service_advice_names_a_serve_command_that_restores_a_public_bind (main.rs:28722) builds the relaunch list itself and reads its own input back, so its name claims more than it checks.
- This is AGENTS.md §3: remediation advice pinned as a string, never checked against behaviour.
- Confidence 85 · mechanical · Fix: either pass record.model_ref and record.port (and --engine where needed) into the builder, or drop "filled in" from the message and the description. Then drive a real stop with a public, keyed record and assert the abort carries --allow-public-bind --require-api-key.
Five branches the change introduces have no test (new; would not gate on its own: no test claims to cover them). Mutating each one alone left the full -p rocm --bin rocm binary green (1327 tests), and the e2e lifecycle steps would not catch them either.
- uninstall_gate.rs:637-640: the pre-flight adds the helper refusal. Dropping it removes rocmd from the plan whenever a run already refuses for a service reason.
- uninstall_gate.rs:183-186: recorded_pid_identities prefers a known start time when the supervisor and engine share a pid. Without it, both the gate and terminate_recorded_service_pids (main.rs:10572) refuse that service.
- main.rs:13358: the self_delete arm of plan_removes_recovery_tooling (Windows).
- uninstall.rs:234: the running executable in the "Not attempted" list (Windows).
- main.rs:13276: the plan line for a pre-flight error.
- More broadly, 52 of the 97 new uninstall tests are cfg(unix) or Linux-only. Lifecycle-25 to 29 are @requires-os:linux. On Windows the gate is covered only by lifecycle-24, the happy path, which uses a PowerShell sleep as a stand-in server.
- These Windows-reachable behaviours are untested on Windows: the tickless refusal (now live there through GetProcessTimes), the root-only helper-stop wording (tree_scope_words), and the survivor-after-stop arm, whose own comment says it exists because of Windows.
- Confidence 100 · logic · Fix: a unit test for each arm above. Put the self_delete and running-exe cases in platform-neutral tests, which means un-gating the stop_gate_tests run helper.
A comment this change made false: rocmd's Windows stop says start times are unavailable there (new; would not gate on its own) — apps/rocmd/src/service.rs:309-317
- It says process_start_ticks "has no /proc to read there and returns None… Closing it needs rocm_core to read a Windows process creation time".
- This change adds exactly that (GetProcessTimes), so the identity_state gate below the comment can now refuse a recycled PID on Windows.
- The same stale claim is in the test docs at service.rs:1611 and around crates/rocm-core/src/lib.rs:2808 ("None elsewhere").
- AGENTS.md §5 requires that behaviour claims stay in sync with the code.
- Confidence 85 · mechanical · Fix: update all three. Optionally widen the refuted-PID rocmd tests to Windows, which can now set up a refuted identity.
41 of the 75 commits carry Co-Authored-By: Claude <noreply@anthropic.com> (new; would not gate on its own: an attribution footer, not a false claim, and the merger edits the squash body)
- The repository builds the squash body from the commit messages, so these trailers ship to main.
- AGENTS.md §11: "avoid AI-generated boilerplate footers".
- Confidence 85 · mechanical · Fix: strip the trailers when rewriting the history or the squash body. Consider writing a clean squash body anyway, since the 75-commit trail includes "address review of aab76d1".

Non-blocking

The help text and README promise a command that brings the helper back, but none is printed — main.rs:666-668, README hunk, uninstall_gate.rs:1293-1295
- "those stay stopped… the error lists the commands that bring them back" covers "services (or the helper)".
- helper_note only says the helper stays stopped "until it is started again".
- Confidence 70 · Fix: name the command, or narrow the wording.
partial_removal_note is fixed text — uninstall.rs:265-283
- A failure on config or cache, which run before the binaries, still says the binaries and service records may be gone and that rocm services stop may no longer work.
- Confidence 70 · Fix: derive that clause from removed.
The PATH-undo failure still says "Nothing else was removed…" — uninstall.rs:200-204
- It now fires after services and the helper were stopped and their endpoint keys dropped, and it prints no relaunch commands.
- The PATH edit could be checked in the pre-flight, before anything is stopped.
- Confidence 70.
False refusal when a stopped record and a live wildcard-bound record share a port under different host spellings — uninstall_gate.rs:585-597, :1071-1074
- Example: a stopped record on a LAN IP and a live record on 0.0.0.0.
- The wildcard expands only to loopback addresses, so the two records are not paired. The stopped record is probed in the pre-flight and finds the live server answering with the same model.
- The run refuses with StopWhatHoldsThePort instead of stopping the live server itself.
- Confidence 70.
Two refusals give advice that does not clear the condition
- RemoveTheStoppedRecord (uninstall_gate.rs:115-121) says "if it is [yours], stop it first", but names no way to do that. rocm services stop has nothing left to kill there.
- When services_dir cannot be enumerated (uninstall.rs:153), the run exits with a bare anyhow error, without the "uninstall aborted… nothing removed" context the help text promises.
- Confidence 70.
The gate reads the start time before checking the pid is alive — uninstall_gate.rs:312-316
- It calls process_start_ticks and then identity_state_with_observed, the exact order that function's new doc warns can yield Matches after the pid exits and is reused.
- It cannot cause a wrong kill, because terminate_verified re-checks. It does mislabel the outcome, and it is the only caller of a new public API that invites misuse.
- Confidence 85 · Fix: use identity_state and make identity_state_with_observed private.
On Windows, a failed ReplaceFileW can delete both the record and its replacement — crates/rocm-core/src/lib.rs:2074, :2191
- On ERROR_UNABLE_TO_MOVE_REPLACEMENT the target is already gone, and the cleanup then deletes the temp file holding the new bytes.
- Service and runtime-state records now go through this path, and a lost record leaves uninstall unaware of a live server.
- Confidence 70 (from the Win32 contract; not run).
On Windows the uninstall stop kills only the recorded root process — main.rs:10570-10603, crates/rocm-core/src/process.rs:290
- The rocmd twin of this stop uses taskkill /T /F, because "the engine runs one level below the recorded PID" (apps/rocmd/src/service.rs:303-308).
- If a child survives and has already bound its port, the run aborts after the keys are dropped. If it has not bound yet, removal proceeds under it.
- Confidence 70 (argued from the twin; not run on Windows).
The vLLM state comment overclaims — engines/vllm/src/state.rs:196
- It says readers "never see a torn document". The same files are still rewritten in place by rocm_core::merge_json_state_file (net.rs:~666), and lemonade's write_state (engines/lemonade/src/state.rs:205) also writes in place.
- Confidence 70.
Gate code and tests live in main.rs — main.rs:8501-8512, main.rs test hunk at 26874
- host_port_resolves is used only by uninstall_gate.rs but was added to main.rs.
- About 3,660 lines of gate tests went into main.rs's mod tests; the gate file itself notes they are "far from here" (uninstall_gate.rs:959-962).
- AGENTS.md §6 asks for new subsystems in their own file rather than growth in main.rs.
- Confidence 70.
Test quality
- Several message-only pins lack the §3 comment naming the test that proves the behaviour: the_tickless_pid_advice_text…, a_helper_only_stop_prints_a_success_line, a_stopped_helper_is_reported_apart…. The proving tests do exist.
- a_directory_removal_that_deletes_some_children_then_fails_is_reported_as_partial (uninstall.rs:1715) asserts on what its own stub deleted.
- Confidence 70.

Decisions for the author

Auto-stop plus a heuristic gate, versus refuse-and-advise — tradeoff
- For auto-stop: it gives the one-command uninstall the ticket asks for, and it reuses the rocm services stop primitive.
- Against: roughly half the 1,376-line gate exists only because it kills: helper identity, the partial-stop/relaunch reporting, deferred partners, and the plan's refusal matrix.
- Against: the paths that proceed without proving the port is free can fail closed cheaply only before anything is stopped. These are an ListsNoModels listener (a loading engine), ProceedUnidentified, an unresolvable host, and a silently refused port. Under refuse-and-advise they could all fail closed. With auto-stop they fire after keys are dropped, which pushes the design toward failing open.
- Confirm the choice was deliberate.
Splitting the change — non-blocking-improvement
- About 8,500 lines bundle several independent changes:
  - the atomic-write consolidation and the windows-sys removal
  - vLLM atomic state writes
  - Windows GetProcessTimes, which changes identity_state for every Windows caller
  - the rocm serve and rocmd refusal-text fixes
  - http_get_with_auth status parsing
  - partial-removal reporting
  - renumbering about 20 existing lifecycle scenarios instead of appending the new ones
- AGENTS.md §11 asks for one logical change per PR, and several of these could land first on their own.
The deferred-partner probe (uninstall_gate.rs:865) — non-blocking-improvement
- It mostly repeats the partner's own survivor probe, and only adds the other loopback family.
- Narrowing it to the addresses the partner's probe does not cover would remove a post-stop abort path.

Positive signals

The plan and the stop pass share one non-destructive pre-flight (preflight_with_records), so the plan predicts a refusal instead of promising a stop the run then refuses.
The stop pass is injected into stop_then_remove_with, so the ordering can be tested: reversing stop and remove turns 7 tests red.
daemon_identity_outcome treats a missing start time as unverifiable rather than a match, so the gate never force-kills a process tree on an unproven pid.
The StopFailureRemedy::advice and StoppedRecordVerdict::blocks matches are exhaustive, so a new variant cannot default to "proceed" or go unprinted.
The new record fields use #[serde(default, skip_serializing_if)], so old and new binaries read each other's files.

Deployment notes

Older records refuse. A live server or helper started by a version that predates supervisor_start_ticks/daemon_start_ticks makes uninstall refuse rather than stop it. Users upgrading with servers running must stop them by hand before uninstalling.
Windows stop behaviour changes. Managed-service stops on Windows (in rocm services stop, rocmd and lemonade) now verify the process start time, and can return Recycled or Indeterminate where they always matched before.

What this covered

Every file in the change was read completely, split across four independent workers. Their scopes were:

uninstall production code
core, rocmd and engines
the tests, with mutation checks
design questions

I also read the callees of the changed symbols and the rocmd twin of the stop path. The diff read was 03cdbcc…f88d72cb; the merge preview and the ASCII check were against main at c7a0d65.

What ran:

Unit tests: the full rocm binary passed at HEAD (1327 passed, 7 ignored), as did rocm-core (735), rocmd (154), rocm-engine-vllm (98) and the protocol crate (23).
Mutations: 16 single-branch mutations, plus several more, against the full rocm binary.
Main's ASCII contract: run against this tree.

CI: the only failing check, "E2E tests (Strix Halo, Windows)", fails on diagnose-fix-path-prepends-with-apostrophe-intact. Main failed the same way (run 39220155843, 2026-10-19) until #710, which this branch predates, so this change did not cause it. The lifecycle-10 and 24–29 scenarios ran and passed in build-and-test (19/19) and windows-build-and-test (15/15).

Did not run:

The e2e lifecycle scenarios locally; they were read only.
Any Windows-gated test, and a full Windows cross-check: ring needs mingw gcc, which is not installed. The two Windows functions were clippy-checked in a scratch crate instead.
Mutations of the probe and verdict logic (stopped_record_verdict, probe_records_for_survivors).
The prior-changes pass was light: only #636's review comments were consulted.

Not reconciled against any discussion on this PR here.

The driver added the round note and the new/gating tags. The review paused overnight with the session and then resumed. The standing-objection verdicts in the round note are anchored checks, not part of what this review read blind.

juhovainio

This closes the primary failure mode in the linked ticket: uninstall_gate.rs stops rocmd and every managed service first, independently re-probes all managed records for survivors (TCP reachability + HTTP identity check, with explicit fail-open disclosure for unresolvable hosts and wildcard binds), and aborts via bail! before any deletion runs if a stop can't be confirmed. That ordering and the "abort leaves everything in place" guarantee are backed by dedicated tests (e.g. a_service_that_cannot_be_stopped_leaves_every_planned_path_in_place), and the coverage is convincing.

The directory-recreation race from the follow-up nightly report ("Directory not empty" after the live service kept rewriting files mid-delete) looks substantially addressed by that same ordering change: deletion no longer starts until liveness is independently confirmed gone, so the window that caused the race should mostly be closed. partial_removal_note reads as a diagnostic fallback for a partial removal that still happens for an unrelated reason, not the mechanism that itself prevents the race — worth confirming that's the intended split.

Requesting changes mainly because:

The PR currently shows as CONFLICTING / DIRTY against main — needs a rebase before this can land.

Non-blocking, worth a second look before merging:

Process-identity verification (daemon_start_ticks / IdentityState) fails closed on Indeterminate/tickless pids, which is the right default, but I didn't trace the exact window between reading start-ticks and acting on them end-to-end — worth a quick confirm there's no realistic staleness window there.
CI: one failing check, Strix Halo Windows E2E (diagnose-fix-path-prepends-with-apostrophe-intact) — this is the known PATH-apostrophe flake that fails identically on main, not a regression from this diff. Everything else, including clippy with --workspace --all-targets -D warnings, is clean.

Nice test coverage on the gate logic overall — the fail-open disclosures in probe_records_for_survivors show real thought about the actual failure modes (wildcard binds, unresolvable hostnames, stale endpoint keys, already-dropped endpoint keys).

fredespi

Review - needs work

The stop-before-remove gate addresses the original uninstall failure: it stops the helper before managed services and refuses removal when a stop cannot be confirmed. One remaining issue needs fixing: the error promises recovery commands with the model and port filled in, but prints placeholders. See the inline comment.

Delivered as a COMMENT because the available posting identity is the PR author and GitHub prohibits requesting changes on your own PR; this does not make the issue advisory. Recommendation: fix the recovery advice before merging.

Reviewed f53815d...4dbdb2f and surrounding stop, process-identity, atomic-publication, and lifecycle-test code. This was a code-only review; Linux/Windows tests and runtime behavior were not executed.

fredespi · 2026-10-23T12:38:18Z

+                " These services were stopped during this run and stay stopped: {}. Stopping \
+                 them dropped their endpoint keys, so a service that was publicly bound, or \
+                 launched with `--require-api-key`, has to be served again; the command for \
+                 each, with its own model and port filled in: {} {}.",


The abort promises a command for each stopped service with its own model and port filled in, but the production construction at lines 796-800 calls relaunch_command(&record.host, record.requires_api_key). That builder always returns rocm serve <model> --host ... --port <port>...: neither the model nor the port is passed. After a partial stop the operator is given placeholders, not the runnable recovery command this message and the PR description promise. The test the_stopped_service_advice_names_a_serve_command_that_restores_a_public_bind constructs report.relaunch itself, so it cannot catch this production defect. Populate the command from the stopped record (including any required engine selection), or explicitly describe it as a template instead of claiming the values are filled in. Test the production stop-report construction with a specific model and non-default port and assert the resulting abort advice.

Fixed in 01f6510 and d37698a. The abort advice is now built from the stopped record by relaunch_command_for (model, engine, port filled in; the template stays the single flag source), so it no longer prints /. Arguments are quoted for the platform shell (PowerShell doubles apostrophes, POSIX sh closes-escapes-reopens). the_abort_advice_for_a_stopped_service_carries_its_own_model_engine_and_port drives the production stop pass with a specific model and non-default port and asserts the abort text; an_apostrophe_in_a_model_path_is_escaped_for_the_shell_that_will_read_it covers quoting.

fredespi · 2026-10-23T12:53:10Z

Review - no blocking findings

Re-reviewed the fix at d37698a against 4dbdb2f. The earlier recovery-advice issue is fixed: the production stop pass now supplies the stopped record’s model, engine, host, port, and authentication requirement. The new regression test runs that stop-report construction and asserts concrete recovery advice rather than supplying its own relaunch list.

The POSIX and PowerShell apostrophe escaping is appropriate for each shell; live shell probes preserved a model path containing spaces and apostrophes as one argument. No blocking issues found in this fix.

This looks safe to approve pending final Linux/Windows validation and maintainer review. This is a code-review comment, not a formal approval; I did not rerun the project test suites.

rominf

🔴 Automated review · pr-review-watcher · d37698a

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

Round note (full review of the whole change at d37698a7). This change request replaces the one on f88d72cb.

Your reply (issue comment 6365328979) was read and recorded with a reaction. It reported fixes and asked nothing, so this round is the answer.
Standing blockers from f88d72cb, settled. The blind review below did not re-find either one as worded. After its report was frozen, each was put to the reviewer together with your reply. These verdicts are anchored checks, not part of the blind review:
- Em dashes: dropped. The three literals are ASCII now. every_literal_that_can_reach_a_redirectable_sink_is_ascii passes, and putting an em dash back at uninstall_gate.rs:95 turns it red, naming that line.
- Relaunch placeholders: dropped as worded. The abort no longer prints <model>/<port>. the_abort_advice_for_a_stopped_service_carries_its_own_model_engine_and_port runs the production stop pass, and mutating the host, key, port, model or engine at the call site each turns it red. Two residues remain:
  - The fixture sets model_ref == canonical_model_id, so the test cannot see the defect in the next bullet.
  - The test is cfg(target_os = "linux"), so the Windows lane does not run it.
- What now gates is a new defect in the same advice: the second Blocking item, marked below. The relaunch command for a catalog model with a vLLM model_id_override (the ticket's own qwen repro) starts a server that is not the one stopped. I re-checked it against the code: matches_ref matches only canonical_model_id and aliases, and Qwen/Qwen3-4B-Instruct-2507 is neither.
Would not gate on their own:
- The PATH-undo ordering. An earlier round rated it non-blocking, and nothing new makes it more than that.
- The untested vLLM atomic write. No test claims to cover it, and the comment overclaim was rated non-blocking before.
- The Co-Authored-By trailers. They are still on 41 commits; as before, they do not gate.
- The stale rocmd Windows comment (standing, not re-found blind). apps/rocmd/src/service.rs:309-317 still says process_start_ticks returns None on Windows. crates/rocm-core/src/service_record.rs:458 makes the same claim.
Gone since f88d72cb: the merge conflict. GitHub no longer reports the branch as dirty.
CI at d37698a7: every hosted check is green. Regenerate manifests and Skill checks (skillscope) were skipped. No GPU-lane check appears in the rollup.

Review — needs work

Full review of the whole change.
Implements what ticket EAI-8014 expects: stop managed servers before removing anything, abort non-zero and keep the tooling when a stop cannot be confirmed, and never report success while a managed endpoint still serves. Issue #241, which the description names, is an open PR, so it was not treated as a ticket.

Blocking

A PATH-undo failure now comes after the servers and the helper were stopped, and the message says "Nothing else was removed" (standing as non-blocking since f88d72cb; does not gate on its own) — apps/rocm/src/uninstall.rs:153-166 (stop_then_remove_writing_to), apps/rocm/src/uninstall.rs:197-204 (execute_plan_with)
- Order. stop_managed_services() runs first, then execute_plan_with undoes the PATH change.
- The PATH abort is predictable. It is the lifecycle-33 case, yet nothing checks for it before the stop pass.
- What the user is told. The run bails with could not undo the installer's PATH change … Nothing else was removed, so you can fix this and run rocm uninstall again.
- What actually happened. Every live server and rocmd have already been killed, and their endpoint keys dropped (uninstall_gate.rs:1028-1052, 1282-1291). Those keys cannot be re-minted.
- What is missing. Unlike the gate's own abort, this message has no relaunch commands and does not say the services stay stopped.
- Rules it breaks.
  - The change's own "stop nothing on a doomed run" rule (541db48, and the pre-flight shared with the plan).
  - AGENTS.md §3: an outcome claim must be asserted together with the resulting state.
- Tests. None combines a live service with a failing PATH undo.
- Confidence 85 · logic · Fix: check that the PATH edits can be applied in the non-destructive pre-flight, before anything is stopped. Otherwise, carry the stop report into this abort the same way the gate abort does.
The relaunch command rebuilds the service from the engine-specific model id, so it does not bring the same service back (new; gates: remediation advice that does not restore what it stopped, AGENTS.md §3) — apps/rocm/src/uninstall_gate.rs:797-805, crates/rocm-engine-protocol/src/lib.rs (relaunch_command_for)
- Which field it uses. It passes record.model_ref. serve_cmd.rs:584,743 sets that from serve_model_ref_for_engine, which returns the recipe's model_id_override when one exists.
- Why that id does not resolve. ModelRecipeRecord::matches_ref (model_catalog.rs:237-244) matches only canonical_model_id and aliases, never an override.
- The ticket's own repro hits this. It runs rocm serve qwen --host 0.0.0.0 --allow-public-bind on vLLM.
  - The record then holds Qwen/Qwen3-4B-Instruct-2507 (model_catalog.json:139).
  - The printed rocm serve Qwen/Qwen3-4B-Instruct-2507 … --engine vllm resolves no recipe.
  - So it either fails or relaunches without the catalog recipe (tool-call parser, recipe hints).
- Other dropped settings. runtime_id, env_id and device_policy, which the record also holds, are dropped as well.
- What is claimed. The abort says "the command for each, with its own model and port filled in", and the README says it "brings it back". AGENTS.md §3 requires that advised commands actually clear the condition.
- Confidence 85 · logic · Fix: use record.canonical_model_id, carry over the runtime and device selections, and add a test that the printed command resolves the same recipe.
The vLLM engine-state write was made atomic with no test, the description does not mention it, its comment overclaims, and the lemonade twin was left alone (new as a test gap, the comment part standing as non-blocking since f88d72cb; does not gate on its own: no test claims to cover it) — engines/vllm/src/state.rs:194-205; twins at engines/lemonade/src/state.rs:205-211 and crates/rocm-core/src/net.rs:648-671 (merge_json_state_file)
- No test. A worker reverted write_state to create_dir_all + fs::write, and cargo test -p rocm-engine-vllm --lib still passed (111 passed, 0 failed).
- Not in the description. It names only ManagedServiceRecord::write and AutomationRuntimeState::write as moved to the atomic publish.
- The comment overclaims. It says rocmd and rocm uninstall "never see a torn document". But the same file is still patched by merge_json_state_file, which uses plain fs::write (the doc comment right above says so).
- The twin was not changed. Lemonade's identical write_state and merge_json_state are untouched.
- Impact if a read tears. refresh_from_engine_state errors; this does not cause a wrong kill.
- Confidence 100 (the mutation run) · mechanical · Fix: add a reader-holds-old-inode test like the record ones, route the patch writers and lemonade through write_file_atomically (or narrow the comment), and mention the change in the description.

Non-blocking

The relaunch quoting is incomplete (new) — crates/rocm-engine-protocol/src/lib.rs (relaunch_command, shell_word_for)
- The host is interpolated unquoted. Records can hold * or [::] (uninstall_gate.rs:1107): bash -c 'echo --host *' globbed, and zsh failed on [::].
- @ is treated as plain. In PowerShell a bare word starting with @ is splatting; with pwsh locally, & printf "[%s]\n" @g dropped the argument.
- Typographic apostrophes are not escaped. PowerShell treats U+2018–U+201B as quotes and failed with a missing terminator.
- A model containing <port>. replacen("<port>") would rewrite text inside the quoted model.
- Confidence 85 · mechanical · Fix: pass the host through shell_word, drop @ from the plain set (or quote words that start with it), and escape U+2018–U+201B for PowerShell.
No test parses the filled-in relaunch command with the real argument parser (new) — crates/rocm-engine-protocol/src/lib.rs tests
- advised_commands.rs scans only the template literal.
- --engine, the substituted values and the quoting are added with push_str/replacen, so the scanner never sees them.
- --engine has value_parser = SUPPORTED_ENGINES (main.rs:488).
- Confidence 85 · mechanical · Fix: shell-split the output of relaunch_command_for and run it through Cli::try_parse_from.
The new public identity_state_with_observed has one caller, and that caller uses the read order the function's own docs warn against (standing) — apps/rocm/src/uninstall_gate.rs:312-316, crates/rocm-core/src/proc_lifecycle.rs:185-198
- The caller reads start ticks before checking liveness. The docs say that order can report Matches for a recycled pid.
- Nothing else uses the observed reading.
- It cannot cause a wrong kill, because terminate_verified re-checks in the safe order.
- Confidence 85 · architectural · Fix: call identity_state and drop the extra API, or say why the order is safe here.
The write_file_atomically doc says every current caller writes non-sensitive state (new) — crates/rocm-core/src/lib.rs:589-594
- RocmCliConfig::save (config.rs:559) now goes through it, and that file holds DashboardDaemonConfig.token.
- The "permissions are not carried over" note is also Unix-only: on Windows, ReplaceFileW keeps the replaced file's ACLs.
- Confidence 85 · mechanical · Fix: correct the doc.
The docs promise a relaunch command for the helper, but none is printed (standing) — help text at apps/rocm/src/main.rs:~672, README uninstall paragraph, uninstall_gate.rs:1298-1301
- Both say the services or helper that the stop pass stopped "stay stopped… the error lists the commands that bring them back".
- The helper note names no command.
- Confidence 70 · mechanical · Fix: name how to restart rocmd, or limit the claim to services.
A scenario reference went stale in the renumbering (new) — tests/e2e-cucumber/features/diagnose.feature:575
- It cites "lifecycle-13's install.ps1 PATH scenario".
- That scenario is now lifecycle-14, because inserting lifecycle-10 shifted everything after it.
- Confidence 85 · mechanical · Fix: cite the @id: rather than the index.
rocmd now fsyncs inside its async tick every 5 s (new) — crates/rocm-core/src/automation.rs:161-168, apps/rocmd/src/service.rs:437-447
- The tick goes through write_file_atomically, which runs sync_all, so it blocks on the disk inside a tokio task.
- Confidence 70 · mechanical · Fix: skip sync_all for this runtime-state file, or move the write onto spawn_blocking.

Decisions for the author

The PR is three changes in one — non-blocking-improvement
- About 8,700 lines over 60 commits mix three things:
  - the uninstall gate;
  - a workspace-wide atomic-write consolidation (therock.rs, rocmd/sandbox.rs, config.rs, vLLM state);
  - new relaunch wording for the rocm serve and rocmd lost-key refusals.
- The last two are separable and would be easier to review on their own.
The quoting assumes PowerShell on Windows — tradeoff
- shell_word uses cfg!(windows) to mean PowerShell. In cmd.exe, single quotes do not group words.
- That is defensible as the documented default shell. Confirm cmd users are out of scope.
Live processes recorded by older versions are refused, not stopped — tradeoff
- This applies to servers or helpers recorded without start ticks.
- Safe against pid reuse, but every user who upgrades with a server still running must stop it by hand before uninstall works.
- It is documented. Confirm that this friction is intended.

Positive signals

stop_managed_services_then_remove takes the stop pass as a parameter, so the stop-before-remove ordering is checked on planned files still existing after an aborted run. A forced-on mutation of the keep-binaries/keep-data gate made a_cache_only_uninstall_leaves_a_live_server_running fail, which shows the gate is really tested.
The plan and the stop pass share one non-destructive pre-flight, so the dry run predicts the same refusal the real run makes.
One relaunch_command builder now serves the gate, rocm serve and rocmd, which removes three drifting copies of the advice.

Deployment notes

Live servers or rocmd instances started by a version without start-time recording are refused by rocm uninstall until they are stopped by hand.
windows-sys is dropped from apps/rocm and apps/rocmd; the Windows publish step now lives in rocm-core.
AutomationRuntimeState gains daemon_start_ticks, which has a serde default, so old state files still parse.

What this covered

What was read: every file in prw-base…HEAD (f53815d5…d37698a7; the merge base equals the base tip).

Two coupled-group workers covered the uninstall flow and the core/daemon helpers.
The main.rs test hunks (about 4,100 lines) were read selectively by test name and where they bear on findings; all of them were run.
The source-reading passes were run by the driver, not by an independent worker: instruction adherence, history, code comments, plus a leak scan and the ASCII contract (cargo test -p xtask ascii: 27 passed).

What was run:

All rocm bin tests: 1404 passed.
Targeted rocm-core tests (40), the rocmd start-time test, the protocol relaunch tests, and the rocm-engine-vllm lib (111).
CI on d37698a: every hosted check is green, including windows-build-and-test and E2E. No GPU-lane checks appear in the rollup.

What did not run, and why:

The lifecycle scenarios (10, 24–29) were not run locally; CI shows no check named for them.
No Windows-target check or clippy of the cfg(windows) arms: the mingw toolchain is missing, so the Windows code was read only.
The prior-changes pass did not run: earlier reviews on these files were not consulted.
Ticket EAI-8014 was retrieved by its identifier.

Not reconciled against any discussion on the PR. No prompt-injection content was found.

The driver added the round note and the new/standing/gating tags. The standing-objection verdicts in the round note are anchored checks, not part of what this review read blind.

nowycondro

🔴 Automated review · pr-review-watcher · d37698a

Review

A large change (28 files, +8703/-446) that makes rocm uninstall stop managed services and the rocmd helper before removing the tooling that would otherwise stop them, with a gate that refuses removal rather than killing anything it can't positively verify. Three passes covered the diff: the uninstall-gate/uninstall-plan core logic (with two specific hypotheses handed in for verification), the shared process-lifecycle/daemon-identity plumbing in rocm-core/rocmd, and the e2e tests/docs/misc changes. The stop-before-remove ordering, PID-identity verification, and refusal-on-ambiguity discipline are all solid and well tested; one messaging gap on an already-destructive failure path is worth fixing before merge.

Blocking

A misleading recovery message is shown after the PATH-undo step fails, even when the stop pass has already killed live services and the rocmd helper and dropped their endpoint keys — apps/rocm/src/uninstall.rs (~198-204 bail message; causal context ~147-169, stop_managed_services() always runs before execute_plan_with). The message reads "Nothing else was removed, so you can fix this and run rocm uninstall again," but when removes_recovery_tooling is true, the stop pass has already run and may have stopped GPU-holding servers and the helper and dropped their API keys (which cannot be re-minted — a publicly-bound or --require-api-key service needs relaunching). This directly contradicts this PR's own new help text, which promises that such an error "lists the commands that bring them back" — a promise kept for the uninstall_removal_gate abort path but not for this one, which is reachable on every ordinary full uninstall. No test combines "services were actually stopped" with "the PATH-edit step then fails." Confidence 85 · logic. Fix: route this bail path through the same stop-report-aware messaging (partial_removal_note/relaunch commands) as the gate-abort path.

Non-blocking

A stale comment contradicts this PR's own fix — apps/rocmd/src/service.rs (~310-317). The comment on terminate_recorded_pid says Windows identity verification doesn't apply because process_start_ticks has no way to read a Windows process creation time — but this PR's own proc_lifecycle.rs adds exactly that, via GetProcessTimes. Harmless today (the surrounding logic independently re-derives the correct verdict), but the comment actively misinforms about current Windows behavior on the precise topic this PR is about, and a separate commit in this same PR (6555f0e1) fixed this same class of stale-comment issue elsewhere without touching this instance. Confidence 80 · mechanical.

Decisions for the author

A hypothesized bug (the relaunch-advice line printing record.model_ref instead of record.canonical_model_id, which could fail to resolve back to a recipe for a model with a model_id_override) was traced end-to-end and does not hold: model_ref is always populated from the user's raw, pre-override CLI argument, which is a recognized alias, not the override string; the override only ever feeds a separate wire-protocol field sent to the engine adapter. No change needed here.

Positive signals

Every new/renumbered e2e scenario (lifecycle-10, 24-29) genuinely exercises the gate against real process/file state — polling real child processes with try_wait(), checking Path::exists() on binaries/manifests, and in the fast-path scenario positively confirming the server is still running — not just checking printed strings. All assertion strings were traced to the exact production format strings they claim to pin.
PID-identity verification correctly refuses (rather than silently skips) when a live pid carries no recorded start time, on both the service and the daemon side; the port-survivor check deliberately treats any listener as blocking for services this pass just stopped.
The Windows-capable write_file_atomically/publish_temp_file dedup is complete: both app crates' now-duplicate windows-sys dependency and local ReplaceFileW implementations were fully removed, not partially.
Backward compatibility for the new daemon_start_ticks field is handled correctly (skip_serializing_if/defaults to None for pre-upgrade records), with the "don't conflate missing-field with unsupported-platform" distinction correctly delegated to the gate logic rather than assumed here.

Deployment notes

gh pr checks shows the self-hosted GPU lanes failing at this head: E2E tests (MI300X, current), E2E tests (Strix Halo, Windows, current), and the self-hosted consolidated report. All other checks (build, clippy, affected-crate tests, docs, license/notices) pass. Job logs were not reachable from this sandbox (proxy-blocked), so root cause is unconfirmed — this needs a human with log access to confirm before merge whether these are caused by this change (the Windows lane in particular runs one of the new scenarios in this group) or are unrelated pre-existing flakiness.

What this covered

Full diff across three passes: the uninstall-gate/uninstall-plan core logic in apps/rocm/src/uninstall.rs/uninstall_gate.rs and the uninstall-related hunks in main.rs; the shared process-lifecycle/daemon-identity plumbing (crates/rocm-core/src/proc_lifecycle.rs, lib.rs, apps/rocmd/src/service.rs, automation.rs); and the e2e feature/step files, docs, and dependency-removal changes. No Rust toolchain available in this sandbox — nothing built or run; findings rest on static tracing and the PR's own CI results. Not reconciled against any existing human review thread.

nowycondro

🔴 Automated review · pr-review-watcher · c512701

Review — needs work

Full review of the whole change (base 79f6e08c…head c512701d), split across coupled file groups plus the four source-reading passes (agent-instruction adherence, history, prior-changes, code comments), run blind with no access to this PR's description, ticket, or review threads until after findings were settled.
Title/commits name EAI-8014 ("uninstall reports success while leaving a managed server running and deletes the tooling that would stop it"); the change implements a stop-before-remove gate for that defect. No separate ticket text was retrievable from here (no tracker tool available to this reviewer beyond the PR's own GitHub text).

Blocking

PR description overclaims the relaunch-advice consolidation — crates/rocm-engine-protocol/src/lib.rs (relaunch_command/relaunch_command_for), apps/rocm/src/service_launch.rs:479, apps/rocmd/src/common.rs:334,343, apps/rocm/src/uninstall_gate.rs:799
The description states the shared builder is "used by the gate, rocm serve and rocmd, and now names a runnable command with the model and port." Only the uninstall gate's call (relaunch_command_for, fed record.model_ref/record.engine/record.port) actually fills those in. rocm serve's and rocmd's own refusal paths (ensure_public_service_has_endpoint_key, duplicated in both crates) still call the unfilled relaunch_command(host, bool), which prints literal <model>/<port> placeholder tokens — even though the caller at service_launch.rs:479 already holds the full ManagedServiceRecord and simply doesn't thread record.model_ref/record.port through. The description's claim is true for one of the three named consumers, not all three.
Confidence 85 · architectural · Fix: either pass the record's model/port into ensure_public_service_has_endpoint_key in both crates (the data is already in scope at every call site), or narrow the description to say only the gate's advice is filled in today.
41 of this branch's 84 commits carry AI co-author footers, against this repo's own enforced policy — commit history (git log 79f6e08c..c512701d --format='%b' | grep -c "Co-Authored-By" → 41)
The repo's AGENTS.md (§11, "PR/Issue/Review Conduct") states "avoid AI-generated boilerplate footers" as part of its blocking upstream-contribution workflow. This branch's commits carry Co-Authored-By: Claude <noreply@anthropic.com> and Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> footers in roughly half of its commits. Independently re-verified by direct count, not taken from a sub-pass's word.
Confidence 90 · mechanical · Fix: squash/rebase to drop the footers before merge, consistent with the project's own documented contributor norm.
Sphinx docs build (-W) shows CANCELLED, not SUCCESS, for the reviewed head commit — CI (statusCheckRollup, re-checked live immediately before filing this review; same result both times)
All other CI jobs on this commit are green, but this one job completed with conclusion CANCELLED (20-minute run, not an instant supersede-by-push cancellation) while its sibling jobs in the same workflow run succeeded. This PR edits README.md, which AGENTS.md itself documents as {include}d into the Sphinx-built docs site — so this is a check relevant to this PR's own changes, not an unrelated job. Per the review method's rule to take CI status at face value rather than assume: this has not been demonstrated green on the current head.
Confidence 75 · mechanical · Fix: re-run the job (or have a maintainer confirm it is non-required/known-flaky) and get it to a clean conclusion before merge.

Non-blocking

helper_decision (apps/rocm/src/uninstall_gate.rs:283-327) is the one production call site using the pre-read-then-compare identity_state_with_observed instead of the race-free identity_state used by every other production call site in this PR (apps/rocmd/src/service.rs:316,1550). Two independent passes flagged this. identity_state's own doc explains the race-free design precisely so callers don't have to get this ordering right themselves; helper_decision's inline comment argues a single reading was needed, but nothing else in the function consumes observed_start_ticks a second time, so the narrower recycle-window this reopens (a handful of instructions, bounded similarly to identity_state's own internal window) appears to buy nothing over just calling identity_state.
apps/rocm/src/uninstall_gate.rs:283-327. Confidence 72 · architectural · Fix: call rocm_core::identity_state(&identity) directly, consistent with the rest of the PR, unless there's a reason to pre-read documented elsewhere.
rocm-core's new write_file_atomically doc comment makes a factually inaccurate blanket claim. It states the lack of permission-carryover is acceptable "only because every current caller writes non-sensitive state," but RocmCliConfig::save (crates/rocm-core/src/config.rs:559) persists this exact helper's output for DashboardDaemonConfig.token, a bearer secret required for TCP dashboard auth, with no set_permissions/mode call anywhere on that path. This is pre-existing behavior (the call site itself is an unchanged rename from the prior write_atomic, not newly exposed by this PR), but the new comment's claim is new and wrong.
crates/rocm-core/src/lib.rs:451, crates/rocm-core/src/config.rs:302-304,559. Confidence 78 · mechanical (doc accuracy) · Fix: correct the comment, or harden the config file's permissions while consolidating its write path.
Two e2e scenarios silently assert nothing on hosts without IPv6 loopback, a condition their own comments say happens in some CI containers: an_ipv6_only_wildcard_engine_is_not_waved_through and a_stranger_on_one_loopback_family_does_not_hide_the_record_serving_on_the_other (apps/rocm/src/main.rs, diff lines ~18354/18421) return early via eprintln! + return when bind_loopback_pair can't get an IPv6 socket, which the Rust test harness reports as a pass. These cover the dual-stack wildcard-probe fail-closed path — one of this PR's safety-critical branches. Partially mitigated by a companion pure unit test (every_wildcard_bind_spelling_is_probed_on_both_loopback_families) that does pin the host-expansion logic unconditionally.
Confidence 70 · mechanical/architectural · Fix: skip explicitly with #[ignore]/a documented reason so a green run is distinguishable from a skip, rather than a silent no-assertion pass.
lifecycle-26 (Linux, helper-only stop) never asserts the rocm/rocmd binaries or service records were actually removed, unlike its sibling scenarios lifecycle-10/lifecycle-24. Its own comment explains it's deliberately narrow ("pinned alone... so it is the helper stop being proven and not the service stop"), but the result is that no scenario covers "live helper only, no managed service, full teardown, binaries actually gone" — a regression specific to that combination would not be caught by any e2e scenario.
tests/e2e-cucumber/features/install_lifecycle.feature:343-355. Confidence 70 · logic (test coverage) · Fix: add the binaries/manifest-gone assertions already used by lifecycle-10/-24.
A new unit test exercises code this PR does not touch. a_vllm_shaped_record_with_a_distinct_engine_pid_carries_engine_ticks (apps/rocm/src/main.rs) only calls ManagedServiceRecord::refresh_from_engine_state() and asserts on engine_pid/engine_start_ticks; crates/rocm-core/src/service_record.rs's refresh_from_engine_state is unchanged by this diff. It would pass identically with every production change in this PR reverted. Plausibly intended as foundational coverage for a value the new gate logic reads (recorded_pid_identities consumes engine_start_ticks), but as written it doesn't pin anything this PR introduced.
Confidence 70 · logic (test relevance) · Fix: none required, but if kept, a comment tying it to the gate's dependence on this field would avoid it reading as a straggler.

Decisions for the author

PID + OS-reported start-time as process identity is inherently probabilistic, not absolute — tradeoff. The gate (and the pre-existing EAI-7410 machinery it reuses) compares kernel/clock-reported process-creation timestamps to tell a recycled PID from the recorded one. On Linux this is /proc clock-tick resolution; on Windows, GetProcessTimes' practical clock granularity is coarser than the 100ns units it's stored in, so two processes launched in rapid succession (e.g. a crash-loop respawn) could theoretically collide. This is a reasonable, already-well-reasoned design choice given the constraints (no cooperating daemon-side token at kill time), but it is a probabilistic guarantee, not a certain one, and that boundary isn't called out anywhere in the new code's docs. Confirm this residual risk is accepted as-is.
This PR's size and commit shape — tradeoff/suggestion. 29 files, ~8,800 insertions, 84 commits (many Merge origin/main into ..., plus an extended "fix one fail-open class, find another, fix that" iteration trail) for what is conceptually one feature. AGENTS.md §11 asks PRs to "keep each PR scoped to one logical change" and to avoid "a trail of incremental noise." The underlying feature is coherent and the iteration produced unusually careful fail-closed coverage, but the history as it stands would read more easily post-squash/rebase; worth confirming this is the intended shape to merge versus cleaning up before landing.

Positive signals

apps/rocm/src/uninstall_gate.rs's stop-before-remove gate is exceptionally carefully reasoned: every fail-open and fail-closed branch is pulled out into an enum with an exhaustive match (StopFailureRemedy, StoppedRecordVerdict, DaemonIdentityOutcome) specifically so a future added variant cannot silently default to the wrong (permissive) side, and the removal gate (uninstall_removal_gate) is called and its error propagated with ? strictly before any path is removed (apps/rocm/src/uninstall.rs:154-167) — traced this call sequence directly and confirmed no path lets removal proceed on an unconfirmed stop.
The wildcard-bind probing correctly treats 0.0.0.0/::/[::]/*/empty as "probe both 127.0.0.1 and ::1, block if either answers" rather than connecting to the wildcard address literally, closing a real gap (a ::-bound server with IPV6_V6ONLY=1 refuses 127.0.0.1 and would otherwise be missed by a single-family probe).
host_port_resolves is correctly split out from loopback_tcp_port_is_reachable so "nothing is listening" (safe to proceed) and "this name doesn't resolve at all" (unknown — must disclose, not silently proceed) are told apart, and the disclosure is returned as a Vec<String> on the report rather than only eprintln!'d, so it's independently assertable by tests.
rocm-core's new write_file_atomically uses OpenOptions::create_new to reserve its scratch filename (not a check-then-create), correctly avoiding a TOCTOU the prior write_atomic didn't need to worry about with its deterministic name.
Scenario IDs in tests/e2e-cucumber/features/install_lifecycle.feature are fully consistent after the renumbering (lifecycle-10..22 → 10-11..23 plus new 10/24-29), with no collisions, and every new assertion string was cross-checked against the literal text the gate actually produces.

Deployment notes

Stopping a managed service as part of uninstall now drops that service's endpoint API key permanently (it cannot be re-minted), so any publicly-bound or --require-api-key server stopped by this path must be relaunched with a new key — this is by design and documented in the abort/complete messages, but is a behavior change worth calling out to anyone relying on the old "uninstall always fully removes, regardless of running state" semantics.
rocm uninstall can now exit non-zero and remove nothing where it previously always proceeded — any automation/scripts that assumed uninstall always succeeds when invoked with --yes need to handle this new refusal exit path.

What this covered

Every changed file read in full against the PR head (c512701d) diffed from the merge-base (79f6e08c): apps/rocm/src/{uninstall.rs, uninstall_gate.rs, uninstall_plan.rs} and the non-test production hunks of main.rs were read directly by this reviewing context (not an independent sub-pass, due to persistent contention for this host's shared subagent capacity — see below); apps/rocmd/src/{common,persistence,sandbox,service,watchers}.rs likewise read directly by this context. Independent fan-out workers covered: crates/rocm-core/src/{automation,config,lib,net,proc_lifecycle,service_record}.rs; apps/rocm/src/{service_launch,therock,chat}.rs + crates/rocm-engine-protocol/src/lib.rs + engines/vllm/src/state.rs; the ~4,000-line new/changed unit-test block in apps/rocm/src/main.rs's mod tests; tests/e2e-cucumber/features/install_lifecycle.feature + lifecycle_steps.rs + docs/*.md + README.md + Cargo.lock; and the four source-reading passes (agent-instruction adherence against this repo's own AGENTS.md, history via git log/blame, prior-changes via commit-message pattern analysis only — no access to this PR's actual GitHub review threads, so that pass is necessarily limited — and code-comment accuracy) across the whole diff.

Degraded in part, not fully: this host's shared subagent pool was saturated by other concurrent sessions throughout this review. Two of seven planned independent passes (the core uninstall.rs/uninstall_gate.rs/uninstall_plan.rs logic, and the rocmd daemon-side files) could not be spawned as independent workers after repeated attempts and were instead reviewed directly by this orchestrating context — flagged per the method's fallback rule rather than silently absorbed. The other five passes did run as independent fan-out workers. Design-review (Step 7) questions were reasoned through by this context directly rather than via further sub-workers, given the depth of material already gathered; no fundamental approach problem was found.

Not reconciled against any prior review or discussion on this PR — this review was conducted blind, with no access to prior rounds.

nowycondro

🔴 Automated review · pr-review-watcher · f58f872

Review — needs work

Full review of the whole change (29 files, +8822/−474 between origin/main at d59f596f and PR head f58f872 — of which ~4120 of the insertions in apps/rocm/src/main.rs are test-module content; the production-code sliver in that file is ~90 lines).
Implements the fix described by Jira ROCMAI-290 (title: "rocm uninstall reports success while leaving a publicly-bound GPU-holding server running and removing the tooling to stop it"; also named here by its external alias EAI-8014): stop every managed service and the background helper (rocmd) before rocm uninstall removes files, and refuse non-destructively (remove nothing, exit non-zero) when a stop cannot be confirmed. The implementation matches the ticket's "Expected" section point-for-point (stop before delete, verify endpoints/process trees are gone, abort and retain recovery tooling on failure, never report success with a live endpoint). The findings below are about test-verification gaps and one dead-code item, not about the core approach being wrong.

Blocking

A test named/commented as proving "a real stop" never checks that the process was actually stopped — apps/rocm/src/main.rs:19148 (a_real_stop_that_a_survivor_outlives_says_the_key_is_gone_and_it_is)
The test's own doc comment states: "A keyed service is stopped for real (the production stop, which drops its endpoint key)... The abort must say the key was dropped, and the key must actually be gone: the message and the state asserted together." It spawns a real sleep 60 standing in for the service's pid, runs the real stop pass, then asserts only the abort message text and that the endpoint key was removed — it never calls rocm_core::process_is_running(pid) or the file's own assert_stopped_within_grace helper, and ends with an unbounded let _ = server.wait();. Its sibling test two hundred lines earlier, uninstall_refuses_while_a_recorded_endpoint_still_accepts_connections (apps/rocm/src/main.rs:17968), does call assert_stopped_within_grace in the equivalent spot. Because the stand-in process exits on its own after 60s, a regression that marked this service "confirmed stopped" (dropping its key) without actually killing it would still pass this test — it would just make the test slower. This also sits in tension with the PR description's own claim that "each of these was verified against a faithful revert of the production change it names... Two are labelled in-place as the exceptions" — this test is not one of the two labelled exceptions.
Confidence 85 · logic · Fix: add assert_stopped_within_grace(&mut server, ...) (or at minimum assert!(!rocm_core::process_is_running(pid))) before the trailing server.wait().
No confirmed test execution of the new feature's Windows-specific process-stop path for this commit — apps/rocm/src/uninstall.rs (all real-process gate tests, e.g. uninstall_stops_live_managed_service_and_reports_it, the_helper_is_stopped_with_the_whole_tree_scope), tests/e2e-cucumber/features/install_lifecycle.feature:310-311 (lifecycle-24)
Every test in this PR that actually spawns and kills a real process to exercise the new stop-before-remove gate is gated #[cfg(unix)] or #[cfg(target_os = "linux")]; none is #[cfg(windows)]. The only coverage aimed at the platform-specific behavior this PR itself documents as different ("Process-tree stop is Linux-only in effect: on Windows only the recorded process is terminated") is the e2e scenario lifecycle-24, tagged @requires-os:windows, which runs only via the separate e2e-selfhosted.yml workflow (ci.yml explicitly says so at line 1516-1517: "SEPARATE workflow, e2e-selfhosted.yml... They live there — not here"). gh pr checks 299 for this exact commit lists every check that actually ran (including windows-build-and-test, which only runs the cfg(unix)-excluded unit suite on Windows) and contains no entry at all from e2e-selfhosted.yml — no strix-windows/Windows-e2e job appears, passing or otherwise. The PR description itself is candid that "The Windows scenario cannot run in the Linux gate I used; CI is its first run," but CI shows no evidence that run happened for this commit. As things stand, the platform-divergent behavior this PR adds has no demonstrated execution on the one platform it behaves differently on.
Confidence 85 · logic · Fix: confirm (and show evidence) that the self-hosted Windows e2e lane actually ran lifecycle-24 for this commit before merging, or add a cfg(windows)-gated unit/integration test using a Windows-safe stand-in process so coverage doesn't depend solely on a non-required, easily-skipped lane.
identity_state_with_observed is dead code — crates/rocm-core/src/proc_lifecycle.rs:200, re-exported at crates/rocm-core/src/lib.rs:151
It's pub, re-exported from the crate root, and carries a substantial doc comment explaining exactly when a caller should prefer it over identity_state ("a caller that has already read [the start-time] — because it also needs the raw observation — should pass that one reading to identity_state_with_observed rather than calling this and reading again"). A repo-wide grep (git grep -n "identity_state_with_observed" pr/299 -- '*.rs') finds no call site anywhere outside its own definition, its own doc comments, and its own unit tests. Neither apps/rocm/src/uninstall_gate.rs nor apps/rocmd/src/service.rs — the two places in this PR that read a raw start-time and then need a verdict — actually call it; both call identity_state directly instead.
Confidence 85 · architectural · Fix: either wire in the caller this function was written for, or drop the function (and its re-export) until one exists.

Non-blocking

engines/lemonade/src/state.rs::write_state was not migrated to the atomic-write helper this PR introduces, unlike its sibling engines/vllm/src/state.rs::write_state — engines/lemonade/src/state.rs:205-211 vs. engines/vllm/src/state.rs:196-205
vLLM's version now calls rocm_core::write_file_atomically, with a comment explaining why: "rocmd and rocm uninstall read this file while a launch rewrites it; the atomic publish means they never see a torn document." Lemonade's write_state, untouched by this PR, is still a plain fs::create_dir_all + fs::write (truncate-in-place) — the exact failure mode the new helper's own doc comment calls unsafe for a concurrent reader. Traced every consumer of the file this writes (ManagedServiceRecord::refresh_from_engine_state and its callers in main.rs, managed_service_status.rs, providers.rs): all wrap the read in .unwrap_or(false) or equivalent, so the practical blast radius today is a silently-missed status refresh, not an aborted uninstall or a panic — this is why it's non-blocking rather than blocking.
Confidence 70 · logic · Fix: switch engines/lemonade/src/state.rs::write_state to rocm_core::write_file_atomically, matching vLLM, so the torn-read property this PR establishes for one managed-engine adapter holds for both.
Leftover vestigial setup in a test leaks an unused temp directory — apps/rocm/src/main.rs:20499 (a_helper_that_cannot_be_confirmed_stopped_stops_no_service_after_it)
The test's first three lines (let mut helper = plant_verified_helper(&test_paths("unused").1); let _ = helper.kill(); let _ = helper.wait();) spawn a process and write runtime state under a test_paths("unused") root, then immediately kill it and shadow the helper variable with a second, real one. The first root is never referenced again and its directory is never passed to fs::remove_dir_all, unlike every other test's cleanup — a harmless but confusing leftover that leaks a directory under .rocm-work/tests/main/ on every run.
Confidence 85 · mechanical · Fix: delete the three leftover lines.

Decisions for the author

PR size/scope — tradeoff
This single commit changes 29 files and +8822/−474 lines. The bulk is legitimately coupled to the one fix (the atomic-write centralization supports torn-read safety for files uninstall now reads mid-stop; the windows-sys dependency removal is a direct consequence of centralizing the Windows publish step; the near-total main.rs test-module rewrite is overwhelmingly new tests for the new gate) rather than several unrelated changes bolted together, so this isn't a scope-mismatch finding. But it is large enough that a human reviewer will have a hard time giving it the attention a safety-critical process-killing change deserves in one pass — worth confirming the reviewing team is comfortable with that, or whether the supporting atomic-write refactor could have landed as a smaller, separately-reviewed precursor.
Older on-disk records with no recorded start-time now cause rocm uninstall to refuse rather than silently succeed — tradeoff
A live service or helper recorded by a pre-this-fix rocm-cli version (no supervisor_start_ticks/daemon_start_ticks) is now refused rather than killed, requiring the operator to stop it by hand and re-run. This is the deliberately stricter, safer behavior the ticket asks for, and it's documented in the README and docs/manual-testing.md, so it's flagged here only so the author can confirm this upgrade-path UX (a new hard refusal where there used to be silent, if unsafe, success) is the intended support stance rather than a surprise for existing installations.

Positive signals

stop_managed_services_with_helper_stop stops rocmd before the per-service loop, matching its own doc comment's rationale that the helper would otherwise respawn a service mid-stop — a real race closed exactly the way it's documented.
StopFailureRemedy::advice/advice_rank and StoppedRecordVerdict::blocks are matched arm-by-arm with no wildcard _ =>, so a newly-added variant forces a compile error until someone decides which side of the abort/proceed line it belongs on — a concrete defense against silently fail-open regressions in exactly the kind of gate where that matters.
Partial-failure bookkeeping is handled with real care: when a service's stop was reported confirmed but a post-stop port probe later finds a survivor, the code explicitly retracts it from report.stopped and strips it from report.relaunch before re-filing it under report.failed (uninstall_gate.rs), and deferred_partners is filtered so one partner's failure isn't double-reported through a record merely sharing its port.
EndpointIdentity (ServesExpectedModel / ServesOtherModels / ListsNoModels) is a genuinely useful three-way distinction for the gate's probe — a model-less /v1/models response (engine warming up) is kept apart from "a stranger is listening," which a boolean couldn't express.
Windows process_start_ticks via GetProcessTimes closes a previously-silent gap: before this PR, Windows managed-service stops had no PID-recycling protection at all.
The new atomic-write regression tests in crates/rocm-core/src/lib.rs are unusually well-reasoned, each with a comment stating what a naive fs::write-based implementation would also satisfy (so wouldn't be a useful regression test) versus the property only the rename-based implementation provides.
The e2e scenario comments (e.g. lifecycle-24's Windows caveat) are candid about their own coverage boundaries rather than overclaiming what they test.

Deployment notes

Behavior change: an installation with a live managed server or rocmd whose on-disk record predates supervisor_start_ticks/daemon_start_ticks (written by an older rocm-cli) will now have rocm uninstall refuse and remove nothing, where it previously removed the tooling while leaving the process running. Documented in README.md and docs/manual-testing.md.
windows-sys is no longer a direct dependency of apps/rocm or apps/rocmd (the one feature they used, Win32_Storage_FileSystem/ReplaceFileW, is now centralized in crates/rocm-core, which still depends on it) — purely an internal dependency-graph change, no functional impact.
Process-tree-kill semantics are platform-asymmetric by design: force-stopping rocmd kills its whole recorded process tree on Linux but only the recorded process on Windows (descendants aren't enumerable there). Already documented in --help, the README and the plan output.

What this covered

Reviewed the full diff between origin/main (d59f596f6e08297c207f008021eecb37defc3558) and PR head pr/299 (f58f8725b944817fd441cf25d430aa393c826a7b). Confirmed apps/rocm/src/main.rs's ~4213-line insertion is ~90 lines of production code plus ~4120 lines of test-module content, and reviewed both parts. Six independent fan-out workers each read their assigned files completely at PR head plus necessary surrounding/caller code: (1) uninstall_gate.rs/uninstall.rs/uninstall_plan.rs, (2) rocm-core's proc_lifecycle.rs/net.rs/service_record.rs/automation.rs/config.rs, (3) rocm-core::lib.rs/rocm-engine-protocol/engines/vllm/therock.rs/chat.rs/service_launch.rs, (4) the rocmd daemon files plus the windows-sys dependency removal, (5) main.rs's new/changed unit tests, (6) the e2e-cucumber feature file and step definitions. All three blocking findings and both non-blocking findings were independently re-verified by me directly against the source (git show/git grep on pr/299) and, for the Windows-coverage finding, against live CI check-run data (gh pr checks 299), rather than taken on a worker's word. Read this repo's AGENTS.md and CONTRIBUTING.md for its conventions (bare Jira/EAI ticket IDs are explicitly permitted and not flagged; crate-layering invariants; the atomic-write/torn-read convention this PR itself introduces; Windows+Linux-only platform support) and applied them throughout.

Read CI's statusCheckRollup for this exact head commit: every required check (clippy, fmt/prek, both build-and-test jobs, Test (affected crates), E2E tests, CodeQL, license/notices/docs-links) reports SUCCESS; Regenerate manifests and Skill checks (skillscope) are SKIPPED and not relevant to the files in this diff. Separately confirmed via gh pr checks 299 that e2e-selfhosted.yml (the workflow that would run the Windows-tagged lifecycle-24 scenario) produced no check-run at all for this commit — this fed directly into the Windows-coverage blocking finding above.

Did not run a full cargo test/cargo build myself (large multi-crate Rust workspace; not practical within this review's time budget) — relied on direct code reading plus the CI results above; one worker likewise declined to build the full workspace for the same reason. Did not run dedicated "history" or "prior-changes" passes as separate workers; spot-checked file history on uninstall.rs/main.rs via git log, which showed this as the latest in a series of mechanical-relocation refactors (#747, #742, #746, #636) with nothing in that history contradicting the current change. Not reconciled against any discussion on the PR — per the method, review threads and comments were never read.

fredespi · 2026-10-28T12:14:06Z

#299 (review)

Fixed in 594a2f5. (1) The real-stop test now asserts the stand-in process actually stopped (assert_stopped_within_grace) before it finishes. (3) identity_state_with_observed is removed with its re-export and tests; helper_decision calls identity_state directly. Also taken: lemonade write_state uses write_file_atomically like vLLM, and the leftover setup lines in the helper test are gone. (2) Not something I can show from here: the E2E self-hosted run for this head had not finished when I last looked, and lifecycle-24 runs only in that lane, so its first Windows result will come from it. I did not add a cfg(windows) unit test I cannot run. Please confirm that lane before merging.

nowycondro

🔴 Automated review · pr-review-watcher · 594a2f5

Review — needs work

Full review of the whole change (base d59f596f6e08297c207f008021eecb37defc3558 → tip 594a2f50a22086c79d599e3f5ca22ebb7e268489, the PR's single commit).
Implements the rocm uninstall "stop managed services before removing the tooling that stops them" fix tracked by EAI-8014; ticket named but not retrievable from here (internal Jira, no tracker tool available to this review). The description's "Related" section names PR #241 as carrying the same behaviour as a pinned-expected-failure row, still open/unmerged — confirmed live via gh pr view 241 (row present, PR still open).

Blocking

Regression test for the "lost its API key" relaunch advice only checks a command prefix, never the --require-api-key flag that is the entire point of that branch — apps/rocmd/src/common.rs:464-468 (test recovery_refuses_a_service_that_lost_a_key_it_was_launched_with), production call site apps/rocmd/src/common.rs:334.
This PR replaces a hardcoded message with one built from rocm_engine_protocol::relaunch_command(host, true). The updated test asserts only rendered.contains("rocm serve <model> --host {host} --port <port>") — it stops before the flag. Its sibling test two functions down (recovery_respawn_fails_closed_for_a_public_service_without_a_key, lines 481-493) was written correctly: it asserts the full string including --allow-public-bind and explicitly checks --require-api-key is absent where it should be. The asymmetry is the tell — one branch of the same function got a complete test, the other did not. A future edit that silently passes require_api_key: false (or drops the flag inside relaunch_command) for this branch would strip auth from the recovery advice for a service that was explicitly launched with a key, and this suite would still go green. This is exactly the repo's own convention in AGENTS.md ("remediation advice naming a command — the named command must exist, accept those flags, and actually clear the condition that printed it" must be asserted together with the behaviour), violated by the new test this PR itself wrote.
Confidence 85 · test-coverage/logic · Fix: add assert!(rendered.contains("--require-api-key")) to the first loop, mirroring the sibling test's flag check.

Non-blocking

PR description names a function that does not exist — the description states "rocm-core grows identity_state_with_observed," but git grep identity_state_with_observed across the PR tip returns nothing. The actual mechanism (one reading answering both "could this platform verify?" and "is this still the same process?") is identity_state/identity_state_from_probes/compare_start_ticks in crates/rocm-core/src/proc_lifecycle.rs:107-181, which I traced and found correctly implements exactly the behaviour described (liveness read before start-tick read, closures enforcing the order, IdentityState::Indeterminate for unconfirmable). The functionality is real and correct; only the cited symbol name is wrong, most likely stale from an earlier draft.
Confidence 80 · doc-accuracy · Fix: correct the symbol name in the PR description (no code change needed).

Decisions for the author

A missing runtime-state.json is taken at face value as "no daemon," silently — tradeoff, apps/rocm/src/uninstall_gate.rs:489-496. The function's own doc comment states this plainly: "a missing file is the ordinary never-started case, and there is no pid to verify or signal without it — but it does mean the gate is only as good as the state file. An unreadable one is the case that aborts; an absent one is the case that proceeds." This is the one fail-open path in the whole gate that is not disclosed via a warning (every other fail-open branch I traced — unresolvable host, empty model listing, unidentified listener, corrupt service record — pushes a warning onto ManagedServiceStopReport instead). Deleting runtime-state.json by hand while rocmd is live would let uninstall proceed without stopping it. Confirm this residual gap is accepted as-is rather than, say, also disclosing it as a warning when the gate otherwise found live managed services to stop.
StopFailureRemedy::RemoveTheStoppedRecord's "if it is [your server], stop it first" clause names no command or discovery method — non-blocking-improvement, apps/rocm/src/uninstall_gate.rs:114-119. Its sibling StopWhatHoldsThePort remedy, for a structurally similar "something answers that isn't tracked by a live record" situation, tells the operator to run ss -ltnp / Get-NetTCPConnection -LocalPort <port> to find it. This remedy's "stop it first" branch gives no equivalent pointer. Pinned only by a wording test (main.rs:21482), not an actionability test. Worth adding the same discovery hint, or leaving it as is on the reasoning that an operator who already recognizes "their own server" doesn't need it.
~4,000 lines of new tests for the brand-new uninstall_gate.rs module (1,387 lines, zero tests of its own) live in main.rs's shared mod tests instead — non-blocking-improvement. The module's own comment concedes the cost: "The tests live in main.rs's mod tests, far from here; change an arm and expect them, not this loop, to be what goes red." Coverage itself is substantial and was verified present (directly or via integration tests through build_uninstall_plan/stop_managed_services_before_uninstall) for effectively every branch sampled; this is a discoverability/organization call rather than a gap, and apps/rocm/src/uninstall.rs and apps/rocm/src/service_launch.rs already keep their own tests colocated, so the inconsistency is worth a deliberate "yes, we're keeping it this way" rather than defaulting into it.

Positive signals

stop_managed_services_before_uninstall → uninstall_removal_gate → execute_plan_with genuinely stops-before-removes and removes nothing on any unconfirmed stop; traced end to end and cross-checked against two Linux-gated integration tests that spawn a real child process and assert it is dead before any file disappears.
PID-recycling identity logic (identity_state_from_probes) enforces liveness-before-start-tick-read via FnOnce closures specifically to close the one dangerous race (a recycled PID reading as a match); uninstall_gate::daemon_identity_outcome and unprovable_live_pids are even more conservative than the primitive requires, correctly erring toward "can't confirm, so abort" for the single highest-stakes caller in the PR.
Every fail-open branch in the record/port-probe logic (stopped_record_verdict and friends) is enumerated by name in a doc comment and either discloses itself via a ManagedServiceStopReport.warnings value (assertable by a test, not an eprintln!) or is justified as the one case that is actual evidence of an unrelated server (ProceedUnrelated) — a notably rigorous treatment of fail-open paths for a gate in front of a destructive action.
write_file_atomically's Windows ReplaceFileW path and its Linux rename path both clean up the scratch file on every failure branch, sync_all before publish, and are backed by a concurrent-reader-sees-no-truncation test; the two local variants in therock.rs/sandbox.rs that deliberately keep their own staging logic (for test-injection seams) say so in their own doc comments, and the claim checks out against the code.
The shared relaunch_command/relaunch_command_for builder in rocm-engine-protocol removes a real drift risk between rocm serve, rocmd, and the uninstall gate's advice text; its one real call site per crate was traced and is correct (host/flag derivation matches the branch's own condition, so the message cannot disagree with the decision — the same property AGENTS.md calls out for preapproved_install_line).
The 7 new/renumbered @lifecycle Gherkin scenarios were diffed carefully against the old numbering: every renumbered scenario is byte-identical apart from its number, nothing was silently dropped or weakened, and the new step definitions spawn real child processes / plant real on-disk records rather than mocking the behaviour under test.
docs/architecture.md, docs/testing.md, docs/manual-testing.md, and README.md were all updated consistently with the new module and behaviour, using correctly-formed relative markdown links.

Deployment notes

At review time, three checks for this exact commit had not finished: windows-build-and-test (IN_PROGRESS), Build Linux PR binaries (IN_PROGRESS), and E2E tests (QUEUED) — the last of these is what actually runs the new @lifecycle scenarios this PR's safety claims rest on. Nothing had failed, but none of the three should be read as green yet.
This changes rocm uninstall's default behaviour: it now actively stops managed servers and the rocmd helper first, and refuses to remove anything if it cannot confirm they stopped. Operators relying on the previous "uninstall always finishes, cleans up after itself" behaviour will see a new non-zero-exit failure mode (with recovery commands named in the error) they did not see before.
Force-stop reaches the whole process tree on Linux but only the single recorded PID on Windows — and, per crates/rocm-core/src/process.rs's own doc comment, identically on macOS, though this repo's conventions state macOS is not a supported platform, so that's informational only.

What this covered

Every file in the diff, read completely at the PR tip via git show pr-299:<path> (never checked out), plus the surrounding functions/callers needed to judge each change in place: apps/rocm/src/uninstall.rs, apps/rocm/src/uninstall_gate.rs (new), apps/rocm/src/uninstall_plan.rs, apps/rocm/src/main.rs (full diff plus every touched/new function and test body), apps/rocm/src/therock.rs, apps/rocm/src/service_launch.rs, apps/rocm/src/chat.rs, apps/rocm/Cargo.toml, Cargo.lock, apps/rocmd/src/common.rs, apps/rocmd/src/persistence.rs, apps/rocmd/src/sandbox.rs, apps/rocmd/src/service.rs, apps/rocmd/src/watchers.rs, apps/rocmd/Cargo.toml, crates/rocm-core/src/automation.rs, config.rs, lib.rs, net.rs, proc_lifecycle.rs, service_record.rs, crates/rocm-engine-protocol/src/lib.rs, engines/lemonade/src/state.rs, engines/vllm/src/state.rs, tests/e2e-cucumber/features/install_lifecycle.feature, tests/e2e-cucumber/tests/e2e/lifecycle_steps.rs, docs/architecture.md, docs/manual-testing.md, docs/testing.md, README.md. Six independent blind workers covered these grouped by coupling (uninstall core; main.rs; rocm-core; rocmd+engine-protocol; therock/service_launch/chat; e2e-tests+docs); all six returned, none failed or had to be re-spawned.
Did not run as separate passes: history (git log/git blame on modified regions) — not performed; prior changes (do earlier review comments on these files apply here) — out of scope per this task's instructions (must not read other reviewers' threads to avoid biasing the blind pass). Agent-instruction adherence was run throughout against this repo's AGENTS.md (it ruled out one fan-out worker's candidate finding about macOS support, since AGENTS.md states only Windows/Linux are supported platforms). Code-comments compliance was checked inline by each file-group worker rather than as a separate pass. Step 7 (design review) was performed directly rather than via further spawned workers, given the depth the six-worker fan-out already provided; it found the fail-open handling coherent and did not surface a new blocking question beyond what is listed above. Not reconciled against any other reviewer's discussion or prior review rounds on this PR — this is an independent blind pass.

rominf · 2026-10-28T12:48:39Z

🔴 Automated review · pr-review-watcher · c512701

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

Informational, not a binding round. These findings describe commit c512701d. While this round ran, the head moved to f58f8725, a single squashed commit on a newer main whose tree differs from c512701d. So this is a plain comment rather than a review, it files no new change request, and its findings are unanchored. The change requests already standing from earlier rounds are untouched and still gate. The next round reviews the current head and settles these findings there. Between now and then, the blockers first found below at c512701d do not gate on their own.

Round note (full review of the whole change at c512701d). Every blocking item below is marked new or standing.

The standing gating objection from d37698a7 still holds at c512701d. The relaunch advice is built from record.model_ref. For qwen on vLLM, that is the override Qwen/Qwen3-4B-Instruct-2507, which matches_ref cannot resolve. The command also drops runtime_id, env_id and device_policy.
- The blind review below did not re-find this.
- After its report was frozen, the objection was put to the reviewer: uninstall_gate.rs:799-805, serve_cmd.rs:132-140,584-585,743, model_catalog.rs:237-244, model_catalog.json:110-117,138, rocm-engine-protocol/src/lib.rs:86-101.
- That verdict is an anchored check, not part of the blind review.
- Still open: whether the printed command fails outright or launches without the recipe's tool-call parser. The record already carries canonical_model_id, so the proposed fix needs no new plumbing.
Other standing items, from d37698a7:
- PATH-undo ordering. Not re-checked as worded. The blind review found a sibling defect in the same function (first Blocking item below).
- vLLM atomic write comment. Re-found blind (second Blocking item).
- Co-Authored-By trailers and the stale rocmd Windows comment. Not checked: the trailers live on the old commit list, which the squash replaced.
CI at c512701d: every hosted check passed. Sphinx docs build (-W) was cancelled, and Regenerate manifests and Skill checks (skillscope) were skipped. No GPU-lane check appears.

Review — needs work

Full review of the whole change at c512701. The PR head has since moved to f58f872, which this review did not read.
Addresses EAI-8014 (now tracked as ROCMAI-290): stop managed servers before uninstall removes the tooling that stops them, and abort non-zero, keeping that tooling, when they cannot be confirmed stopped. The change covers all three expectations in the ticket.

Blocking

A removal that fails partway does not say which services this run already stopped, or how to bring them back (new) — apps/rocm/src/uninstall.rs (execute_plan_with, the removal-loop error arm calling partial_removal_note), apps/rocm/src/uninstall_gate.rs:1262-1271 (stopped_during_run_note doc)
The doc on stopped_during_run_note lists "a later step that fails" as a case it reports. But execute_plan_with only uses stopped_note in the PATH-undo failure arm. When removing a config/data/binary entry fails, the error comes from partial_removal_note(&removed, name, &untouched), which knows nothing about the stop report. The success line printed before removal is only a count (stopped N managed service(s) before removal). So after a mid-removal failure, the operator never sees which services were stopped, that their endpoint keys are gone, or the rocm serve commands that bring them back. The doc describes the situation correctly, so the code should change to match it.
Confidence 85 · logic · Fix: thread stopped_note into the removal-failure error as well, and assert it in a_removal_that_fails_partway_says_what_is_gone_and_what_is_left.
The new vLLM comment promises a torn-read guarantee that the same file does not have (standing since f88d72cb, re-found blind this round) — engines/vllm/src/state.rs:196-199, crates/rocm-core/src/net.rs:648-671 (merge_json_state_file), engines/lemonade/src/state.rs:205-209
The new comment says "rocmd and rocm uninstall read this file while a launch rewrites it; the atomic publish means they never see a torn document". The doc comment directly above it says the inference-verified latch bypasses write_state and patches this file through rocm_core::merge_json_state_file. That function still truncates and rewrites in place with fs::write. Lemonade's identical write_state, read by the same ManagedServiceRecord::refresh_from_engine_state, was not converted either. The fix landed on one of two twin writers and one of two write paths.
Confidence 85 · logic · Fix: publish through write_file_atomically in merge_json_state_file and in lemonade's write_state, or narrow the comment to what is actually guaranteed.
The new relaunch command in the public-bind refusal in rocm is not covered by any test (new) — apps/rocm/src/service_launch.rs (second bail! in ensure_public_service_has_endpoint_key), tests at service_launch.rs:1565-1575 and respawn_fails_closed_for_a_public_service_whose_key_is_gone
For ("0.0.0.0", false, false) the tests only assert that --allow-public-bind is present and --require-api-key is absent. The old literal (rocm serve --host {host} --allow-public-bind) satisfies both, so reverting this branch keeps every test green. The rocmd twin in apps/rocmd/src/common.rs does pin the full rocm serve <model> --host 0.0.0.0 --port <port> --allow-public-bind form.
Confidence 85 · mechanical · Fix: add the same full-command assertion to the rocm test.
The description misstates the change's scope (new; description only, so on its own it would not gate a squash merge) — PR description, "Supporting work"
1. It says the rocm serve and rocmd lost-key refusals "are unchanged by this PR". This diff rewrites both: apps/rocm/src/service_launch.rs and apps/rocmd/src/common.rs replace rocm serve --require-api-key / rocm serve --host {host} --allow-public-bind with relaunch_command(...) output (commit 97ea46e).
2. It does not mention the vLLM engine-state write becoming atomic (engines/vllm/src/state.rs).
  Confidence 85 · mechanical · Fix: correct the description to say both refusals now print the shared <model>/<port> command, and list the vLLM change.

Non-blocking

A wildcard-bound live server can falsely block uninstall through an older stopped record on a specific address — apps/rocm/src/uninstall_gate.rs:564-594 (partition_records), :1077 (hosts_name_one_listener), :1100 (probe_hosts)
Take a live record on 0.0.0.0:P and a stopped record for the same model on, for example, a LAN IP or 127.0.1.1 at port P. The live record probes only 127.0.0.1/::1, so the two are not treated as partners. The pre-flight then probes the stopped record first and reaches the live wildcard listener serving our model. The result is BlockServingOurModel, so the run refuses with "the engine outlived its supervisor" advice, which is false here. It fails closed, so it is only a usability problem.
Confidence 70 · logic · Fix: treat a wildcard live host as a partner of any host on the same port.
Two doc comments contradict the code — apps/rocm/src/uninstall_gate.rs:10-11, :1232
The module doc says UninstallOptions/UninstallPlan "stay in the crate root", but they are defined in uninstall_plan.rs:54,71. unreadable_service_manifests says it "Returns the file names", but it returns full paths, which the abort relies on.
Confidence 85 · mechanical · Fix: correct both comments.
A test's comment and fixture describe one failure class and exercise another — apps/rocm/src/uninstall.rs:1958-1980 (a_background_helper_that_cannot_be_stopped_leaves_every_planned_path_in_place)
The comment and reason say the helper's "identity could not be verified". Production maps that case to StopFailureRemedy::ConfirmTheDaemonPid ("check what that pid is"). The test builds StopTheDaemon and pins its "kill that pid" advice instead. The assertions hold for the branch actually built, but the comment misdescribes what the test covers.
Confidence 85 · mechanical · Fix: reword the reason/comment to "signalled but not confirmed gone".
host_port_resolves was added to main.rs but only the gate uses it — apps/rocm/src/main.rs:4887-4898
AGENTS.md §6 says new subsystem code goes in its own file, not into main.rs. Separately, the gate's roughly 3.6k lines of tests also landed in main.rs mod tests; uninstall_gate.rs:966 admits they are "far from here".
Confidence 70 · architectural · Fix: move the helper into uninstall_gate.rs (or next to loopback_tcp_port_is_reachable), and consider giving the gate its own test module.
The tick-less service advice does not say which file to delete — apps/rocm/src/uninstall_gate.rs:101-108 (ConfirmTheServicePid)
It says "delete the service's record file under the data directory's services folder by hand" without giving a path. Its sibling RepairTheRecord names the full path, and rocm services remove refuses a record whose pid is live.
Confidence 70 · mechanical · Fix: include the manifest path in the failure id or the advice.

Decisions for the author

One PR bundles several separable changes — non-blocking-improvement
At c512701 this is 8.8k lines over 65 commits. Besides the uninstall gate, it carries:
- the workspace-wide atomic-write consolidation (rocm-core, rocmd, therock, vLLM);
- real Windows process start-times, which changes every Windows rocm services stop, not only uninstall;
- a protocol-level relaunch-command builder that rewrites serve and rocmd messages.
Each is reviewable on its own, and together they are hard to review completely. Splitting would cost re-coordination of the stacked tests; keeping them together keeps the motivating fix and its supporting pieces in one place.

Positive signals

One pre-flight drives both the plan and the stop pass, so the plan cannot promise a stop that the run then refuses.
StopFailureRemedy::advice/advice_rank and StoppedRecordVerdict::blocks are exhaustive matches, so a new class cannot silently go unadvised or default to proceeding.
The e2e steps assert each printed claim next to the outcome it describes. For example, then_plan_names_helper is paired with then_background_helper_stopped. This is the pattern AGENTS.md §3 asks for.
daemon_start_ticks and the tick fields use #[serde(default)], so state files and records written before them still parse rather than being refused as unreadable.
The Windows GetProcessTimes FFI closes its handle on every path, and a FILETIME is only compared for equality, never mixed with Linux ticks.

Deployment notes

After upgrading, rocm uninstall refuses until the user stops by hand:
- a rocmd that was already running and so wrote no daemon_start_ticks;
- a live server recorded without a start time.
This is intended, and the README and advice cover it.
On Windows, process identity is now checked, so rocm services stop and restart can return Recycled/Indeterminate where they always matched before.
Config, service-record and automation-state writes now publish through ReplaceFileW on Windows instead of a rename. The runtime state also fsyncs on each daemon tick.
rocm uninstall --dry-run and the chat dry-run tool now make HTTP identity probes (2 s timeout) for each reachable already-stopped record.

What this covered

Every file in prw-base...HEAD (ea86403…c512701d; merge-base 79f6e08, and it merges cleanly onto the current base).
- Read completely: the apps/rocm uninstall modules and the non-test main.rs hunks; the rocm-core/rocmd/protocol/vLLM changes; the docs; the e2e feature and steps.
- The roughly 80 new tests in main.rs were cross-checked only by name against the branches they claim. They were not given individual revert/mutation verdicts.
Fan-out: two read-only workers (uninstall modules; core/daemon/protocol). The docs, e2e, history, agent-instruction and Step 9 passes ran in this driving context.
What ran:
- cargo test -p rocm-core --lib with mutation rounds on the atomic-write, start-time identity and endpoint-identity helpers; every mutated test failed except those noted.
- Sign-off check: exit 0, 65 commits, none missing.
Did not run:
- apps/rocm and rocmd test builds, the lifecycle e2e scenarios, and any Windows compile. Disk was near the 40G floor. The four blocking findings are from reading, not from runs.
- The separate design-question workers (Step 7). The fan-out cap and time budget were spent, so that pass ran inline and is not independent.
- The prior-changes pass: no earlier review comments on these files were read.
CI: statusCheckRollup reports the PR's current head f58f872, not c512701. Nothing here establishes CI results for the reviewed commit. At f58f872 every required lane listed passed; "Sphinx docs build (-W)" was cancelled, and README.md, which that build includes, changed in this PR.
No prompt-injection content was found.
Not reconciled against any PR discussion.

The driver added the opening note and the new/standing tags. The verdict on the standing objection in that note is an anchored check, not part of what this review read blind.

fredespi · 2026-10-28T15:35:08Z

#299 (review)

Both points are fixed, in a2939d9 (the --require-api-key assertion landed in 1bec494). The rocmd recovery test now asserts the flag, and the PR description no longer names the removed identity_state_with_observed. I also corrected the description of the vLLM/Lemonade state writes. On the two decisions: a missing runtime-state.json is still read as "no daemon" and proceeds without a warning (accepted, the gate is only as good as that file); I left the RemoveTheStoppedRecord advice and the test layout as they are.

fredespi · 2026-10-28T15:35:14Z

#299 (comment)

Taken in a2939d9: (1) the stop pass note now also goes on a removal that fails partway (new test a_removal_failure_after_a_stop_says_what_the_stop_already_did); (2) merge_json_state_file and Lemonade write_state now publish atomically, matching vLLM; (3) the rocm public-bind refusal test asserts the full relaunch command; (4) the description now says the rocm serve and rocmd refusals changed to the shared command, and lists the vLLM change. Also fixed the two stale doc comments and the helper test reason. Not taken: the standing relaunch objection. record.model_ref is the raw CLI argument (serve_cmd.rs passes &model into the launch); the override id only goes to the engine over the wire, so the printed command resolves. The relaunch does not carry --runtime-id, --env-id or the device policy; the description now says so. Also left alone: moving host_port_resolves, the ConfirmTheServicePid path hint, the wildcard-partner case (it fails closed), and splitting the PR.

…t would stop them rocm uninstall reported success while a managed model server, possibly bound publicly and holding the GPU, kept running, and then deleted the binaries and service records needed to stop it. Uninstall now stops the managed services and the rocmd helper first, and refuses to remove anything when it cannot confirm a stop: a process whose identity it cannot verify, an endpoint that still answers, or a record it cannot parse. The refusal names each recovery step, including the relaunch command for every service it did stop, since stopping drops its endpoint key. Covers EAI-8014. Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

fredespi requested a review from a team as a code owner August 22, 2026 12:09

fredespi requested a review from r0x0r August 22, 2026 12:09

fredespi force-pushed the rocm-uninstall-reports-success-while-leaving-a branch from d1e9c4e to 0bebf62 Compare August 22, 2026 14:18

r0x0r reviewed Aug 24, 2026

View reviewed changes

Comment thread apps/rocm/src/uninstall.rs Outdated

Comment thread apps/rocm/src/uninstall.rs Outdated

Comment thread apps/rocm/src/main.rs Outdated

Comment thread apps/rocm/src/main.rs Outdated

fredespi force-pushed the rocm-uninstall-reports-success-while-leaving-a branch from 0bebf62 to e2015fb Compare August 24, 2026 13:36

michaelroy-amd previously requested changes Aug 26, 2026

View reviewed changes

rominf requested changes Sep 2, 2026

View reviewed changes

Comment thread apps/rocm/src/main.rs Outdated

Comment thread apps/rocm/src/main.rs Outdated

Comment thread apps/rocm/src/main.rs Outdated

Comment thread apps/rocm/src/uninstall.rs Outdated

Comment thread apps/rocm/src/main.rs Outdated

siloteemu previously requested changes Sep 9, 2026

View reviewed changes

siloteemu previously requested changes Sep 10, 2026

View reviewed changes

fredespi force-pushed the rocm-uninstall-reports-success-while-leaving-a branch from 69aa6c6 to 6b697a3 Compare September 10, 2026 07:35

siloteemu previously requested changes Sep 10, 2026

View reviewed changes

siloteemu previously requested changes Sep 11, 2026

View reviewed changes

fredespi force-pushed the rocm-uninstall-reports-success-while-leaving-a branch from df64d9a to 3cc2f4d Compare September 14, 2026 11:30

fredespi commented Sep 14, 2026

View reviewed changes

Comment thread apps/rocm/src/main.rs Outdated

Comment thread apps/rocm/src/main.rs Outdated

Comment thread apps/rocm/src/main.rs Outdated

siloteemu reviewed Sep 14, 2026

View reviewed changes

rominf requested changes Oct 15, 2026

View reviewed changes

fredespi requested a review from rominf October 15, 2026 13:23

rominf requested changes Oct 22, 2026

View reviewed changes

juhovainio requested changes Oct 22, 2026

View reviewed changes

fredespi commented Oct 23, 2026

View reviewed changes

rominf requested changes Oct 23, 2026

View reviewed changes

nowycondro reviewed Oct 27, 2026

View reviewed changes

fredespi requested review from nowycondro and rominf October 27, 2026 19:23

nowycondro reviewed Oct 28, 2026

View reviewed changes

fredespi force-pushed the rocm-uninstall-reports-success-while-leaving-a branch from c512701 to f58f872 Compare October 28, 2026 10:09

fredespi requested a review from nowycondro October 28, 2026 10:09

nowycondro reviewed Oct 28, 2026

View reviewed changes

fredespi force-pushed the rocm-uninstall-reports-success-while-leaving-a branch from f58f872 to 594a2f5 Compare October 28, 2026 12:04

fredespi requested a review from nowycondro October 28, 2026 12:04

nowycondro reviewed Oct 28, 2026

View reviewed changes

fredespi force-pushed the rocm-uninstall-reports-success-while-leaving-a branch from 594a2f5 to 1bec494 Compare October 28, 2026 15:24

fredespi requested a review from nowycondro October 28, 2026 15:25

fredespi force-pushed the rocm-uninstall-reports-success-while-leaving-a branch from 1bec494 to a2939d9 Compare October 28, 2026 15:34

fredespi force-pushed the rocm-uninstall-reports-success-while-leaving-a branch from a2939d9 to 6c7df26 Compare October 28, 2026 18:39

fredespi force-pushed the rocm-uninstall-reports-success-while-leaving-a branch from 6c7df26 to fe59db3 Compare October 28, 2026 18:48

Conversation

fredespi commented Aug 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Test plan

Related

Uh oh!

r0x0r left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

michaelroy-amd left a comment

Choose a reason for hiding this comment

Uh oh!

rominf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

siloteemu left a comment

Choose a reason for hiding this comment

Summary

🚫 Blocking (must fix before merge)

Non-blocking

Uh oh!

siloteemu left a comment

Choose a reason for hiding this comment

Summary

🚫 Blocking (must fix before merge)

Non-blocking

Uh oh!

siloteemu left a comment

Choose a reason for hiding this comment

Summary

🚫 Blocking (must fix before merge)

Non-blocking

Uh oh!

siloteemu left a comment

Choose a reason for hiding this comment

Summary

🚫 Blocking (must fix before merge)

Non-blocking

Uh oh!

siloteemu left a comment

Choose a reason for hiding this comment

Summary

Prior objection status

🚫 Blocking (must fix before merge)

Non-blocking

Uh oh!

siloteemu left a comment

Choose a reason for hiding this comment

Summary

🚫 Blocking (must fix before merge)

Non-blocking

Test audit (standing focus a)

Will this confusion recur? (standing focus c)

Uh oh!

siloteemu left a comment

Choose a reason for hiding this comment

Summary

🚫 Blocking (must fix before merge)

Non-blocking

Uh oh!

siloteemu commented Sep 11, 2026

Summary

Prior findings

🚫 Blocking (must fix before merge)

Non-blocking

Uh oh!

fredespi commented Sep 14, 2026

Uh oh!

fredespi left a comment

fredespi commented Aug 22, 2026 •

edited

Loading

siloteemu commented Sep 14, 2026 •

edited

Loading