fix(uninstall): stop managed services before removing the tooling that stops them (EAI-8014)

[fredespi]

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 services before removing anything, and refuses to proceed if it cannot.

Changes

• apps/rocm/src/uninstall.rs: 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 how to stop them. On success, report how many were stopped, then proceed with removal.
• apps/rocm/src/main.rs: new stop_managed_services_before_uninstall() returning a ManagedServiceStopReport { stopped, failed }. It reuses the existing verified process-tree termination path (stop_internal_managed_service / terminate_recorded_service_pids) that rocm services stop already uses, so a service is only counted stopped once every recorded process is confirmed gone; a service that merely crashed (not live) is skipped and does not block uninstall.
• Updated the uninstall plan warning from "background processes are not stopped automatically in this pass" to "their servers will be stopped before removal".

Test plan

• New unit tests (apps/rocm):
  • uninstall_stops_live_managed_service_and_reports_it — a live managed server is stopped and reported before uninstall proceeds.
  • uninstall_skips_already_dead_managed_service — a crashed (non-live) service neither counts as stopped nor aborts uninstall.
• cargo clippy -p rocm --all-targets -- -D warnings clean; cargo test -p rocm passes (verified in a Linux container).
• Matches the behavior pinned by the uninstall-stops-what-it-manages scenario in the README-walkthrough contract PR (test(e2e): pin the contracts a README walkthrough expects (EAI-8024) #241): "the server is no longer running" after the managed files are removed.

[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:

1. 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.

2. 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.

Withdrawing this change request as superseded: all three blocking points from 451a8ee are genuinely fixed at 4db3a7f — the atomic-write helper is single-sourced, the port probe keeps its normalization, and each abort class now names a remedy that can actually clear it. A fresh review at the current head is being filed separately, raising a new concern introduced by the latest commits; only that newer one is operative.

[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]

🔴 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

1. 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.
2. 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.
3. 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.