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 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 rocm-core grows identity_state_with_observed so one reading of the PID answers both the "could this platform verify?" and "is this still the same process?" questions. 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.
• write_active_runtime_marker now publishes through the atomic helper instead of remove-then-rename.
• The uninstall plan warnings are reworded to match what will actually happen.
• 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, and lifecycle-25 refuses and keeps everything when a service record cannot be parsed. Existing scenarios were renumbered. The --keep-binaries --keep-data exemption and the helper stop are covered by unit tests only, not by a scenario.
• 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.

Related

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

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

[rominf]

🔴 Automated review · pr-review-watcher · 824a3eb

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 (round 10, full review of the whole change). This round files a change request again. It replaces the one at 1edde9cb as this account's whole position. Each finding is tagged new or standing. This also answers your reply about 824a3eb5. What changed since 1edde9cb:

• Settled, by putting each to the reviewer after its report was frozen (anchored):
  • The helper listed as a stopped service. helper_stopped is its own field, and the abort words the helper separately. Left for the next round: the test asserts text on hand-built reports only, and it does not assert the "nothing supervises" consequence.
  • The ProceedUnidentified warning. Dropping that push now turns an_endpoint_that_answers_without_a_usable_listing_discloses_the_fail_open red.
• Fixed in production, not yet in the test: the doomed-run ordering. Already-stopped records are now probed before anything is stopped. The test half is still open; see the third bullet of "Three branches…" below.
• Still standing: the conflict is back. main moved after this review's base: #540 merged at 0a021785.
• Still standing: most of the untested fail-closed branches, the plan warning, and the description. Your note says those tests were not done this round.
• The red E2E tests (MI300X) lane is not this PR's doing. The same scenarios (bench-06, chat-06/07, serve-01/02/05) fail with the same vLLM OutOfMemoryError on main's own push run at 2c37f5bc (run 37835680511). The reviewer lists it below because a red required check blocks merge. It is not a defect in this diff and is not a reason for this change request. It needs a re-run once the GPU is healthy.

Review — needs work

Full review of the whole change.
Implements the expected behaviour of EAI-8014 (the tracker now returns it as ROCMAI-290): stop managed services before removing the tooling, abort non-zero and keep that tooling if a stop can't be confirmed. Runs with --keep-binaries --keep-data are deliberately exempt from the stop pass (see Decisions).

Blocking

• Reusing a port for the same model makes uninstall refuse, with false advice (new) — apps/rocm/src/uninstall_gate.rs:452-457 (probe_records_for_survivors(paths, &already_stopped, &[], …) then the early return)
  generate_service_id creates a new id on every launch. So serve M → services stop → serve M again leaves record A "stopped" and record B "ready" on the same host:port. The probe of already-stopped records runs before any live service is stopped. It finds B answering with A's model, or a 401 when B requires a key, and blocks A with StopWhatHoldsThePort. B is never stopped.
  The abort tells the operator that "the engine outlived its supervisor, so rocm services stop has nothing left to kill". Both claims are false here. It then tells them to find and kill whatever holds the port, which is their own healthy managed server.
  A temporary unit test using stop_managed_services_with reproduced this. Both variants (no auth, and B requiring auth) failed with A in failed and stop_calls=[] (2 tests ran, 2 passed).
  Confidence 100 · logic · Fix: leave out of the pre-stop probe any already-stopped record whose normalized host:port belongs to a live record. Probe those after the live stops instead. Add the sequence above as a test.

• The --allow-public-bind advice cannot restore a public service (new) — apps/rocm/src/uninstall_gate.rs:885-890
  The abort says a publicly bound service "has to be served again with rocm serve --allow-public-bind to return". Without --host, rocm serve binds loopback (validate_bind_host, main.rs:6884). The existing restart error at main.rs:7034 gives the working form: rocm serve --host {host} --allow-public-bind. AGENTS.md §3 requires a named remedy command to accept those flags and actually clear the condition. The only test (main.rs:39681) just checks that the string contains --allow-public-bind.
  Confidence 85 · mechanical · Fix: name --host <host> --allow-public-bind (and the key options) as main.rs:7034 does, and assert the advice against what validate_bind_host accepts.

• A plan warning promises a refusal that some runs never perform (standing) — apps/rocm/src/main.rs:21085
  When load_managed_services fails, the plan prints "Uninstall will refuse to remove anything until they can be read". On a --keep-binaries --keep-data run, plan_removes_recovery_tooling is false, the stop pass is skipped (uninstall.rs:55-61), and removal goes ahead. No test asserts either the message or the behaviour; it is not referenced anywhere else.
  Confidence 85 · logic · Fix: base the sentence on plan_removes_recovery_tooling(&plan, paths), as the live-records warning just below already does, and add a test.

• Three branches this change introduces stay green when broken on their own (standing in part: the first bullet is the open test half of the doomed-run fix, the third was raised at 1edde9cb, the second is new) — apps/rocm/src/uninstall_gate.rs:458, crates/rocm-core/src/proc_lifecycle.rs (identity_state_with_observed), crates/rocm-core/src/lib.rs:~2927 (AutomationRuntimeState::write)
  Each was mutated alone and the targeted tests rerun (102 rocm and 26 rocm-core tests):

  • Deleting the early return after the already-stopped probe left everything green. Without it, a run that is going to abort still force-kills a verified live rocmd first. a_survivor_behind_an_already_stopped_record_dooms_the_run_before_anything_is_stopped claims to cover this but has no daemon in its fixture, so it cannot fail on it.
  • Deleting the liveness check in identity_state_with_observed left everything green, although its doc promises a pid that has exited yields Gone.
  • Reverting AutomationRuntimeState::write to fs::write left everything green. The same change to ManagedServiceRecord::write is caught by a_service_record_write_never_shows_a_reader_a_truncated_manifest.
    Confidence 100 · logic · Fix: add a live verified daemon to the doomed-run fixture and assert it was never signalled. Add a direct Gone case for identity_state_with_observed, and a truncated-read test for the runtime-state file.

• The description misstates the change in both directions (standing in part) — PR description

  • It names the wrong files. stop_managed_services_before_uninstall is in apps/rocm/src/uninstall_gate.rs, not main.rs. daemon_start_ticks is recorded in apps/rocmd/src/{service,watchers,persistence}.rs; apps/rocmd/src/lib.rs is not in the diff.
  • It does not mention three changes:
    • Windows start-time reading (GetProcessTimes) in proc_lifecycle.rs. Every Windows managed-service stop can now return Recycled or Indeterminate where it used to always return Matches.
    • The --keep-binaries --keep-data exemption from the stop pass.
    • write_active_runtime_marker switching to the atomic helper.
      Confidence 85 · mechanical · Fix: update the description to match the diff.

• The required check E2E tests (MI300X) is failing on this head (new; see the round note: the same failure is on main, so this PR did not cause it) — CI
  The failure was in "a model is being served on GPU" and "a model is served in the background". vLLM ran out of memory: "144.15 GiB is allocated by PyTorch… 46.88 GiB is free". That points to an infrastructure cause (a busy shared GPU) rather than this diff, but the check is red and needs a rerun to green.
  Confidence 100 · mechanical · Fix: re-run the lane, or check with the lane owner whether the GPU was busy.

• The branch conflicts with the base again (standing: checked by the driver against the current base, not by the blind review, which read against db11e474, where it merged cleanly) — apps/rocm/src/main.rs, docs/architecture.md
main is now at 0a021785. #540 moved driver-install, engines and serve out of main.rs there. git merge-tree --write-tree 0a021785 824a3eb5 reports CONFLICT (content) in both files, and GitHub reports mergeable_state: dirty. Until it is resolved, no CI runs for the branch.
  Confidence 100 · mechanical · Fix: merge or rebase onto current main, and re-check that the gate code relocated by #540 still has the call sites this PR adds.

• New fail-closed branches that no test exercises: each mutation below stayed green when run alone (standing: not re-found by this round's blind review, then each mutation re-run at 824a3eb5 — anchored) — apps/rocm/src/uninstall.rs (host_port_resolves), apps/rocm/src/uninstall_gate.rs (unreadable_service_manifests non-.json filter; daemon kill KillScope::Tree, force: true), apps/rocm/src/main.rs (plan_removes_recovery_tooling, the entry.path.starts_with(&services_dir) arm), crates/rocm-core/src/lib.rs (unix publish_temp_file)

  • Each of these six still leaves every test green:
    • host_port_resolves made always false.
    • The non-.json filter dropped.
    • The services_dir prefix arm dropped.
    • The daemon kill changed to KillScope::Single.
    • The daemon kill changed to non-force.
    • Delete-then-rename in publish_temp_file (594 rocm-core tests passed).
  • Each lone failure in a full run was re-run in isolation and passed. It was the shared-process flake restart_refuses_a_public_service_without_a_key_before_stopping_it, which also fails with no mutation.
  • Now covered: reading the start-time before the liveness check is caught by a_dead_process_is_never_read_for_a_start_time.
  • Still holds, by reading only: discovery of a really unreadable services directory. a_stop_pass_that_cannot_run_leaves_every_planned_path_in_place injects the error through a closure, so the real ? path is never exercised.
    Confidence 100 · logic · Fix: one test per branch that fails when it is mutated.

• The description claims the publish step is "single-sourced" (standing: not re-found by this round's blind review, then checked — anchored) — PR description, apps/rocm/src/remote/session.rs:128
  The description still says "the atomic-write publish step is single-sourced (write_file_atomically)". At 824a3eb5, session.rs still publishes its staged file with a plain fs::rename(&staging, &path), and this PR does not touch that file.
  Confidence 85 · mechanical · Fix: narrow the claim to the sites this PR converts, or migrate session.rs.

Non-blocking

• A false comment, and stale-record advice that leaves out the command that would clear it (standing) — apps/rocm/src/uninstall_gate.rs:499-503, :75-81
  The comment says "there is no services remove". rocm services remove <id> --yes exists (added in #411) and accepts any record that is not in use. When an unrelated server that answers 401/403 holds a stale record's old port, the only advice printed is to stop that unrelated server. Removing the stale record would be the cheaper fix, and the advice doesn't offer it. Confidence 85 · logic · Fix: correct the comment, and offer rocm services remove <id> --yes for already-stopped records.

• Comments left stale by Windows gaining start-times (standing) — apps/rocm/src/uninstall.rs:394 ("Some only on Linux"), apps/rocm/src/uninstall_gate.rs:336 ("arm meant for Windows"), crates/rocm-core/src/proc_lifecycle.rs:50-52 ("clock ticks since boot… None on platforms without /proc"), and the uninstall.rs:7-11 module doc ("Mechanically relocated… with no behavior change"). The code governs in each case: process_start_ticks now returns a FILETIME on Windows, and the module now changes behaviour. Confidence 85 · mechanical · Fix: rewrite them to match the code.

• The docs omit that uninstall force-kills rocmd (standing) — apps/rocm/src/main.rs:595-599 (--help), README.md, docs/manual-testing.md §8, plan warning main.rs:21144-21162
  Any run that removes the tooling tree-kills a verified rocmd (uninstall_gate.rs:250-262), even with no services, and that can abort the run (StopTheDaemon, RepairTheDaemonState). All of these surfaces mention only "managed model servers". AGENTS.md §5 asks for docs to stay in sync with observable behaviour. Confidence 70 · mechanical · Fix: mention the helper stop and its abort classes.

• The doc comment says "Two limits" and then lists three (standing) — crates/rocm-core/src/lib.rs:4762-4785. Confidence 85 · mechanical · Fix: say "three", and untangle the "not the containing directory" clause.

• Logic and a test written only for macOS (standing) — apps/rocm/src/uninstall_gate.rs:795 (record_predates_start_ticks's own-pid conjunct is always true on Linux and Windows), and without_readable_start_times_a_bare_record_is_not_a_legacy_record, which is cfg(not(any(linux, windows))). AGENTS.md §6 says not to implement unsupported-platform behaviour. Confidence 70 · architectural · Fix: drop the macOS path, or explain why it is needed.

• A test is placed between use super::*; and use crate::test_support… (new) — apps/rocmd/src/service.rs tests module. It is also Linux-only, though Windows now records start-times too. Confidence 70 · mechanical · Fix: move it below the imports and widen it to any(linux, windows).

Decisions for the author

• The first uninstall after an upgrade aborts while a pre-upgrade rocmd is still running — tradeoff
  Such a record has no daemon_start_ticks, so it lands in Unverifiable and the operator has to kill the pid by hand. That is the safe direction (never signal an unverified tree), but it is a one-time cost on the ordinary upgrade path. Confirm it is intended and say so in the release notes.

• Cache-only runs report uninstall complete while servers keep running — tradeoff
  The ticket says "Never report uninstall complete while a rocm-cli-managed endpoint is still serving." The --keep-binaries --keep-data exemption keeps the tooling, so the ticket's underlying harm doesn't occur, but that wording is not met literally. Confirm this was deliberate.

• Three atomic-write implementations remain — non-blocking-improvement
  Only publish_temp_file/replace_file_windows were consolidated. apps/rocm/src/therock.rs:4636-4790 and apps/rocmd/src/sandbox.rs:655-760 keep their own stage-and-publish chains without the sync_all the core helper does. Exposing a _with seam on the core helper would allow one copy.

Positive signals

• The ordering puts every check that can doom the run before anything destructive: the free manifest scan, then the already-stopped probe, then the daemon kill, then the per-service stops.
• StoppedRecordVerdict::blocks, StopFailureRemedy::advice and advice_rank are exhaustive matches, so a new variant has to be placed on one side of a destructive decision before it compiles.
• The injectable stop_*_with seams make the abort paths testable, and the tests hold up: 21 of 25 single-branch mutations turned a test red.
• Wildcard binds are probed on both 127.0.0.1 and ::1, and the identity probe goes to the address that actually answered.

Deployment notes

• runtime-state.json gains an optional daemon_start_ticks. Old files still parse. A daemon started before the upgrade blocks uninstall until it is stopped by hand.
• On Windows, managed-service records now carry a process creation time, so rocm services stop can newly report a recorded pid as recycled or unverifiable.
• Service manifests, runtime state and the active-runtime marker are now replaced rather than rewritten in place. File mode is not carried over, and a symlink at the target is replaced instead of written through.

What this covered

Every file in the change read completely, by grouped workers: the gate (uninstall_gate.rs, uninstall.rs, the production hunks in main.rs, the endpoint-identity API in rocm-core); identity and persistence (proc_lifecycle.rs, rocm-core's state and atomic-write code, therock.rs, rocmd, the manifests); tests; e2e and docs. Also read: the callers of services remove, validate_bind_host and generate_service_id. Reviewed efcd4800…824a3eb5 (prw-base...HEAD).

• Runs: the unit suites for rocm uninstall (39 tests) and the added tests (47 in rocm, 11 in rocm-core) all passed. The lifecycle e2e passed: 11 scenarios, including lifecycle-10 and lifecycle-25, with a fresh report.
• Mutation pass: 25 single-branch mutations, plus one temporary reproduction test, all reverted.
• Shared build cache: CARGO_TARGET_DIR is shared, and one rocm-core run once picked up another checkout's binary and ran 0 tests. Every run used as evidence was confirmed as a recompile with the expected test count.

Did not run or only partly ran:

• Prior-changes pass: partial. gh api returned no review comments on #117 or #169, and no other earlier PRs were checked.
• Design review: the driver answered the questions itself from the workers' reads instead of sending them to separate workers.
• Windows cfg arms: checked by reading only.
• Crate-edge check: cargo xtask check-crate-edges was not run.

The PR's discussion and earlier reviews were not reconciled here. The two anchored blocking items, and the conflict and CI notes, were added after the review by the driver. The anchored items were settled by a check that was given the objection; that check is not part of what this review read blind.

[fredespi]

#299 (review)

Pushed 43fbb58 (merged main incl. #540's relocations; conflicts in main.rs/architecture.md resolved, gate builds and passes on acer). Blocking: (1) port reuse: already-stopped records sharing host:port with a live record are now probed AFTER the live stops, test serving_the_same_model_again_after_a_stop_does_not_doom_the_uninstall (stop of B frees the port; A not failed). (2) advice now says 'rocm serve --host --allow-public-bind' (add --api-key), asserted. (3) the plan's read-error warning only promises a refusal where plan_removes_recovery_tooling holds, else says the run keeps the tooling; test with an unreadable services dir. (4) doomed-run test now has a verified live daemon and asserts it is not killed/recorded; added a direct Gone test for identity_state_with_observed and a reader-safety test for AutomationRuntimeState::write (fails when write reverts to fs::write: checked). Not done: the six remaining mutation survivors (host_port_resolves, non-json filter, services_dir prefix arm, daemon Tree/force scope, unix publish_temp_file delete-then-rename), the PR description rewrite, and the MI300X lane (runner OOM, same on main).

[rominf]

🔴 Automated review · pr-review-watcher · 43fbb58

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 (round 11, full review of the whole change). This round files a change request again. It replaces the one at 824a3eb5 as this account's whole position. Each finding is tagged new or standing. It also answers your note about 43fbb588. What changed since 824a3eb5:

• Settled, by putting each to the reviewer after its report was frozen (anchored):
  • Port reuse for the same model, as worded. Same host spelling, both without a key and with B requiring one: B is stopped first and A is not failed. Nothing pins the key-required variant yet. A different host spelling of the same endpoint still aborts; that is the first new blocker below.
  • The plan's read-error warning. It now branches on plan_removes_recovery_tooling, and forcing that branch turns the_plan_read_error_warning_only_promises_a_refusal_where_the_stop_pass_runs red. That test is cfg(unix) and skips itself, still passing, where permissions don't apply (e.g. root).
  • The three untested branches. Each mutation alone now turns a named test red: the doomed-run early return, the Gone liveness check, and AutomationRuntimeState::write.
• Resolved, checked by the driver: the conflict. git merge-tree --write-tree of the current main (03afcf43) and 43fbb588 merges cleanly.
• Not a reason for this change request: E2E tests (MI300X). Its earlier failure also happened on main. On this head it, and the Strix Halo Windows and WSL2 lanes, had not reported yet.
• Still standing: the six mutation survivors (appended at the end of Blocking), the description (including the "single-sourced" claim), and the re-serve advice, re-found blind in a narrower form.
• Not raised before, or raised before as non-blocking: the README/--help helper-kill point and the third atomic-write copy were non-blocking last round. Each now has new evidence (key files deleted before an abort that "leaves every file in place"; cleanup branches no test reaches). Neither alone is why this change request was filed.

Review — needs work

Full review of the whole change.
The PR closes the reported defect: uninstall now stops managed servers before removing anything. The description names EAI-8014, which was not retrieved because no tracker tool is available here. The description also cites #241 as a related unmerged PR, which this review did not read.

Blocking

• A stopped record whose host is spelled differently from a live record on the same port aborts the whole uninstall, and the advice it prints points the operator at their own server (new) — apps/rocm/src/uninstall_gate.rs:446-462, :529
  The "defer a stopped record that shares a live endpoint" check compares host text: it joins probe_hosts(host) and adds the port. So 0.0.0.0:P (which becomes 127.0.0.1|::1) never matches a live 127.0.0.1:P, and localhost never matches 127.0.0.1. The stopped record is then probed first, the live record's own server answers with the same model, and the run aborts. The abort says "Every process recorded … is gone … find what holds that port", which tells the operator to hand-kill a managed service. When the live partner's stop fails, the deferred record is blocked with the same wrong remedy.
  Reproduced: a copy of serving_the_same_model_again_after_a_stop_does_not_doom_the_uninstall with only svc-a's host changed to 0.0.0.0 returned calls=[] and failed=[svc-a … StopWhatHoldsThePort].
  Confidence 100 · logic · Fix: compare normalized and resolved probe-address sets on the same port, and skip a deferred record whose live partner is already in failed.

• When the gate refuses to kill a helper pid it cannot verify, its advice tells the operator to kill that pid (new) — apps/rocm/src/uninstall_gate.rs:161-168, :82-86
daemon_identity_unverified uses StopTheDaemon. That remedy's advice is "kill that pid (kill <pid> / Stop-Process -Id <pid> -Force)". Its own reason line says the pid was left alone "rather than risk signalling an unrelated process that inherited its pid".
  The realistic path: upgrade, then a reboot. The pre-upgrade runtime-state.json still says running: true and has no start-time, so the pid now likely belongs to an unrelated process. The PR's own test uninstall_never_kills_a_daemon_pid_from_a_state_file_that_predates_start_ticks calls that process stranger and asserts it gets this advice.
  This violates AGENTS.md §3: remediation must actually clear the condition, and this one asks the operator to do the unsafe thing the gate refused to do.
  Confidence 85 · logic · Fix: give this case its own remedy: "confirm pid N is rocmd; if it is, stop it; if not, delete <automation_state_path>", naming the file.

• The re-serve advice names a command clap rejects (standing: the --allow-public-bind advice objection, re-found blind in a narrower form) — apps/rocm/src/uninstall_gate.rs:906
  The text is `rocm serve --host <host> --allow-public-bind` (add `--api-key <key>`). Serve takes a required positional model (main.rs:433-434), so the command fails as written, and it also drops the port. AGENTS.md §3 requires a named command to exist and accept those flags. The pre-existing message at main.rs:3816 has the same text; fix both.
  Confidence 85 · mechanical · Fix: rocm serve <model> --host <host> --port <port> --allow-public-bind ….

• Discovery fail-closed is pinned by no test, while the plan prints a promise that depends on it (standing: earlier rounds noted the real ? path was never exercised) — apps/rocm/src/uninstall_gate.rs:423, :440; apps/rocm/src/main.rs:17955
  Run here: replacing both ? with .unwrap_or_default() recompiled rocm, and all 41 matching uninstall tests stayed green. That includes a_stop_pass_that_cannot_run_leaves_every_planned_path_in_place, which injects its own failing closure and never reaches this code. the_plan_read_error_warning_only_promises_a_refusal_where_the_stop_pass_runs asserts the "will refuse to remove anything" text only. AGENTS.md §3 requires the message to be asserted together with the behavior.
  Confidence 100 · logic · Fix: on the tree the read-error test already makes 0o000, also drive uninstall_with_paths and assert it errors and the planned files survive.

• Several added tests cannot fail on the branch they claim to cover (new) — apps/rocm/src/uninstall.rs:88, :91; apps/rocm/src/uninstall_gate.rs:191, :672, :840, :970; tests in apps/rocm/src/main.rs
  These are from a worker's mutation runs: one mutation at a time, rocm recompiled each time, and 53 tests ran each time.

  • Recycled pid treated as ours. Mutating Recycled => NothingOfOurs to Stop leaves uninstall_never_kills_a_daemon_pid_that_was_recycled green. terminate_verified's re-check absorbs it, and the test does not assert !helper_stopped. Building the kill identity without the start-time is also green, so each layer hides a regression in the other.
  • Fail-open warnings never reach the operator. Dropping eprintln!("warning: …") is green.
  • Success line. Dropping the success println! is green. So is dropping !report.helper_stopped from the success-line condition, which leaves a helper-only run printing nothing.
  • Wrong failure reason. Forcing the "recorded stopped, … serving this record's own model" reason for a service this pass just stopped is green. uninstall_refuses_while_a_recorded_endpoint_still_accepts_connections checks only the failed id.
  • Unrelated server on a recycled port. Widening the auth-refusal test to status >= 300 is green. No test stages a server that answers 404 or 500, so the case the warning text describes is never exercised.

  Confidence 100 · logic · Fix:

  • assert !helper_stopped in the recycled-pid and legacy-record tests;
  • capture stderr and stdout, or inject the sink, in an uninstall.rs command-level test;
  • add a helper-only success case;
  • assert the remedy and the "after the stop" reason;
  • add a 404/500-answering endpoint that expects ProceedUnidentified.

• README and --help misstate what uninstall does (standing in part: the helper-kill omission was non-blocking last round; the "leaves every file in place" part is new) — README.md:819-824, apps/rocm/src/main.rs:606-610, plan warnings in build_uninstall_plan

  • The helper kill is undisclosed. stop_background_helper_before_uninstall force-kills the rocmd process tree whenever the plan removes the tooling, even when there are no services. The README, --help and the plan shown before confirmation never mention it; the only notice is a line printed after confirmation.
  • "Leaves every file in place" is false on two refusal paths. A per-service stop failure, or a survivor found after the stops, aborts after earlier services were already stopped. Those stops deleted their endpoint key files (clear_endpoint_api_key), and the abort message itself admits it.

  AGENTS.md §5 requires these claims to stay in sync with the code.
  Confidence 85 · mechanical · Fix: add a plan warning when the helper is recorded running, mention the helper in README and --help, and say that installed files are kept but servers already stopped stay stopped and lose their keys.

• Two new user-observable behaviors have no scenario (new) — tests/e2e-cucumber/features/install_lifecycle.feature
  The two behaviors:

  • --keep-binaries --keep-data leaves a live server running. Only the Linux unit test a_cache_only_uninstall_leaves_a_live_server_running covers it.
  • Uninstall stops the background helper.

  AGENTS.md §3 requires a scenario for each, or a stated justification in the PR text. The PR text gives neither.
  Confidence 85 · logic · Fix: add lifecycle scenarios, or justify the gap in the PR text.

• The manual-testing refusal check cannot be followed as written (new) — docs/manual-testing.md:393-397
  Step 1 is a full rocm uninstall --yes, which has already removed config, data and binaries. Step 2 then asks the tester to corrupt services/bad.json and check that those directories "remain". The doc also never says that services lives under the data directory.
  Confidence 85 · mechanical · Fix: start step 2 from a fresh install and give the full services path.

• The new core atomic writer is a third copy of the staging logic, and its failure cleanup is untested (standing in part: last round listed the three copies as a non-blocking improvement; the untested cleanup is new) — crates/rocm-core/src/lib.rs:4669-4830, apps/rocm/src/therock.rs:4637-4691, apps/rocmd/src/sandbox.rs:653-703
temp_sibling_path, ATOMIC_WRITE_TEMP_ATTEMPTS and the staging loop now exist three times, and the copies already differ: only the core copy calls sync_all. The core copy has no test hooks, so removing either remove_file(&tmp) (the cleanup after a failed write or a failed flush), or breaking the collision loop, leaves every test green (traced; not run). The therock and sandbox copies do test those paths through their hooks.
  This repo's bugs tend to cluster in duplicated implementations like these.
  Confidence 80 · architectural · Fix: give the core helper the suffix_for_attempt and before_publish hooks under cfg(test) or a crate-private variant, and have therock and sandbox call it.

• The PR description does not match the change (standing; includes the earlier "single-sourced" objection) — PR body

  • Unmentioned: process_start_ticks now reads a start-time on Windows (GetProcessTimes). This changes every Windows terminate_verified caller, including rocm services stop and the lemonade/vLLM stops: a live process that OpenProcess cannot open is now left unsignalled where it used to get a best-effort match.
  • Also unmentioned: the plan-warning rewrite, the write_active_runtime_marker migration, the cache-only skip, and the deferred endpoint probing.
  • Misattributed: it places stop_managed_services_before_uninstall in main.rs (it is in uninstall_gate.rs) and daemon_start_ticks in apps/rocmd/src/lib.rs (it is in service.rs).
  • Inaccurate: it lists the report fields as { stopped, failed }; warnings and helper_stopped also exist.
  • Overclaimed: "single-sourced (write_file_atomically)" — only publish_temp_file is single-sourced.

  Confidence 85 · mechanical · Fix: update the description (narrow it to what the change does and add the omissions). Nothing in the code needs to change for this.

• New fail-closed branches that no test exercises: each mutation stays green (standing: not re-found by this round's blind review, then re-run at 43fbb588 — anchored) — apps/rocm/src/main.rs:13281 (host_port_resolves; earlier rounds put it in uninstall.rs), apps/rocm/src/uninstall_gate.rs (unreadable_service_manifests non-.json filter; helper kill KillScope::Tree, force: true), apps/rocm/src/main.rs (plan_removes_recovery_tooling, the starts_with(&services_dir) arm), crates/rocm-core/src/lib.rs (unix publish_temp_file)

  • The five rocm mutations, applied together (separate paths, so none masks another): all 1063 rocm tests passed. They were host_port_resolves forced false, the non-.json filter dropped, the services_dir arm forced false, KillScope::Single, and a non-force kill.
  • Delete-then-rename in unix publish_temp_file: all 598 rocm-core tests passed. The truncation tests check for a truncated file, not a missing one.
  • The host_port_resolves mutation only adds warnings and never removes a block, so its test would pin a disclosure, not safety. The Single/non-force kills are only reachable with a live, verified helper on Linux.
    Confidence 100 · logic · Fix: one test per branch that fails when it is mutated.

Non-blocking

• Comments contradict the code (standing in part) — Confidence 85 · mechanical · Fix: reword each to match the current code.
  • uninstall_gate.rs:615 says the tests "live at the bottom of this file"; they are in main.rs.
  • :517-518 says there is "no services remove"; it exists at main.rs:998, as does services prune.
  • :850 says the function returns file names; it returns full paths.
  • :229 and :336, plus uninstall.rs:394 and main.rs:33559 and :35191, still describe Windows as the best-effort platform or call start-times Linux-only.
  • proc_lifecycle.rs:50-51 still says start-times are None wherever there is no /proc.
  • uninstall.rs:7 still says "no behavior change".
• The claim "one implementation … for the whole workspace" is false (new) — crates/rocm-core/src/lib.rs:4687 (and the identical comments in therock and sandbox). apps/rocm/src/remote/session.rs:128 still does its own temp-write and fs::rename. That writer predates this PR; the claim is new. Confidence 85 · mechanical · Fix: move it onto the helper, or soften the comment.
• The two reader-safety tests disagree about Windows (new) — crates/rocm-core/src/lib.rs:4877, :4946. The service-record test is unix-only and says Windows cannot make the same observation. The runtime-state test checks the same property on every platform. Confidence 70 · mechanical · Fix: make the two gates and comments agree, based on the Windows lane result.
• "The removal is reported as complete" checks only the text (new) — tests/e2e-cucumber/tests/e2e/lifecycle_steps.rs (then_removal_complete). It asserts neither last_rc == 0 nor that the files are gone. Other steps in the same scenarios do check the process state. Confidence 70 · mechanical.

Decisions for the author

• The gate's tests live in main.rs, not in uninstall_gate.rs — non-blocking-improvement
  About 1,900 of the 2,283 lines added to main.rs are tests for the new module. AGENTS.md §6 wants new subsystems out of main.rs, and the stale "bottom of this file" comment shows the drift has already started. Moving them into a #[cfg(test)] mod tests in uninstall_gate.rs would keep them beside the code.
• Three disclosed fail-open paths — tradeoff
  The three paths are an unresolvable host, an empty model list, and an unidentifiable listener on a stopped record. Each proceeds with a warning instead of blocking. That keeps a recycled port from blocking uninstall with no way out, at the cost of possibly deleting the tooling while a wedged engine of yours keeps the port. The choice looks deliberate; note that the warnings themselves are currently unpinned (see Blocking).
• Substantive behavior arrived inside merge commits — non-blocking-improvement
  43fbb58 (confirmed with --remerge-diff) and c8bdf89 add behavior in merge commits: deferred probing, the re-serve advice, the plan read-error warning, and tests. The squash merge makes the final tree correct, but a commit-by-commit reader misses these. Mention them in the PR text.

Positive signals

• stop_managed_services_then_remove takes the stop pass as a closure, so the "nothing removed on abort" guarantee is asserted against real files on disk. An ordering-inversion mutation turned 5 tests red.
• advice(), advice_rank() and blocks() are exhaustive matches, so a new failure class cannot silently go without advice or default to "proceed".
• Whether the platform can read start-times is decided by reading this process's own start-time, so a failed read of the daemon's pid cannot be mistaken for an unsupported platform.
• The plan warning and the decision to run the stop pass use the same plan_removes_recovery_tooling check, so the text and the behavior cannot disagree.
• daemon_start_ticks is #[serde(default, skip_serializing_if)], so old and new state files read each other.

Deployment notes

• Uninstall now force-kills the rocmd process tree whenever it removes the binaries or service records.
• After upgrading, while an old rocmd that never recorded daemon_start_ticks is still running, uninstall aborts as "unverifiable" until rocmd is restarted. This now applies on Windows as well as Linux.
• On Windows, every verified stop (rocm services stop, engine stops) now requires a readable process creation time. A process the CLI cannot open, for example an elevated one, is no longer signalled.
• Service records and runtime state are now written atomically. On Unix they get umask-default permissions instead of keeping the old file's mode; on Windows ReplaceFileW keeps the existing ACL.

What this covered

Every file in the change, read completely, plus the callers and callees of the changed symbols in apps/rocm, rocm-core proc_lifecycle and rocmd. Range: prw-base (03afcf4) … 43fbb58.

• Fan-out: 4 independent workers — gate logic; rocm-core/rocmd; unit-test verdicts with 26 mutation runs; e2e, docs, AGENTS.md adherence, history and impact.
• Re-run here: I reproduced the discovery-fail-closed mutation myself.
• CI at the reviewed head: every completed check passed. "E2E tests (MI300X)", "E2E tests (Strix Halo, Windows)" and "E2E tests (Strix Halo, WSL2)" had no result yet.

Did not run:

• Lifecycle e2e scenarios: not run locally; they need packaging. Judged from the step code, and CI runs them with E2E_INCLUDE_LIFECYCLE=1.
• Windows cross-build: blocked locally because the mingw linker is missing. The moved Windows bodies were type-checked in throwaway crates outside the repository.
• Prior-review comments on these files: not read; that is outside this review's remit.
• EAI-8014: not retrieved; no tracker tool is available here.

Not reconciled against any PR discussion. No prompt-injection content was found. The last blocking item and the round note were added after the review by the driver; the anchored checks were given the objection and are not part of what this review read blind.

[fredespi]

#299 (review)

Pushed e0ae769. Taken: (1) host-spelling: a stopped record now defers when ANY probe address overlaps a live record's on the same port (0.0.0.0 / localhost / 127.0.0.1); test a_stopped_record_spelled_with_a_wildcard_host_still_waits_for_its_live_partner. (2) unverifiable helper pid gets its own remedy, ConfirmTheDaemonPid: check what the pid is, stop it if rocmd, else delete the stale runtime-state file; no more 'kill that pid' for a pid the gate refused to kill. (3) re-serve advice (here and at main.rs:3816) now 'rocm serve --host --port --allow-public-bind'. (4) discovery fail-closed: the read-error test now also drives uninstall_with_paths on the unreadable services dir and asserts it errors and the planned file survives. Also: helper-only success line test, and the legacy-record test asserts !helper_stopped. Not done: README/--help/plan helper-kill disclosure and 'leaves every file' wording, new lifecycle scenarios for cache-only and helper stop, the remaining mutation survivors (recycled=>Stop, stderr warnings, 404/500 endpoint, wrong-reason, plus the earlier six), PR description.

[rominf]

🔴 Automated review · pr-review-watcher · e0ae769

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 (round 12, full review of the whole change). This round files a change request again; it replaces the one at 43fbb588 as this account's whole position. Every finding is tagged new or standing. It also answers your note about e0ae7699 (thanks — I've reacted to it rather than replying separately).

• Settled, by putting it to the reviewer after its report was frozen (anchored): the re-serve advice. Both sites now print rocm serve <model> --host <host> --port <port> --allow-public-bind and every flag exists; withdrawn. (The blind review keeps a non-blocking note that nothing parses it.)
• Narrowed, still standing (anchored checks, details at the end of Blocking): host spelling — the named spellings are fixed, the failed-partner half is not; unverifiable helper pid — the main path is fixed, the terminate → Unverified arm still prints "kill that pid"; discovery fail-closed — removing both ? is now caught, each alone is not; mutation survivors — (a) and (c) partly fixed, (b), (d), (e) open; atomic writer — publish-failure cleanup now tested, write/flush cleanup and the collision loop are not.
• Still standing in full (you listed these as not done): README/--help disclosure, the two missing scenarios (the PR text gives no justification for the gap), the manual-testing steps, the six fail-closed branches, and the description (re-found blind this round).
• New this round: a test that can delete the shared build binaries, and macOS-only logic against AGENTS.md §6.
• Not a reason for this change request: CI. Every reported check at e0ae7699 passed or was skipped; no self-hosted GPU lane appears in the rollup yet.
• Verification this round was static. Disk on the review host was under the build floor, so no build, test or mutation was run; every verdict below was reached by reading the code.

Review — needs work

Full review of the whole change.
Does what EAI-8014 (now ROCMAI-290, In Progress) asks for the full uninstall. The PR does not say it closes the ticket. One way the ticket's expected behaviour is deliberately not met is listed under Decisions for the author.

Blocking

• A test can delete the real build binaries if the gate it checks ever regresses (new) — apps/rocm/src/main.rs:34883-34890 (the_plan_read_error_warning_only_promises_a_refusal_where_the_stop_pass_runs)

  • The test runs the real uninstall_with_paths with force_dev_binaries: true and keep_binaries false.
  • From a test binary in target/<profile>/deps, daemon_binary_path() resolves to the sibling target/<profile>/rocm (crates/rocm-core/src/lib.rs:4478-4489). is_dev_binary_layout is then true, and the force flag bypasses the skip (main.rs:17855). So the plan contains kind: "binary" actions for the real rocm, rocmd and rocm-engine-* in the shared target directory.
  • Only the services-dir abort stops the removal loop. If that abort regresses, the test deletes the build output that the e2e suite and other developers use.
  • Its sibling test uninstall.rs:526-530 passes keep_binaries: true for exactly this reason ("keeps the test off the real executable-discovery path"). The services directory sits under the data dir, so the refusal still triggers with that flag.
  • Confidence 85 · logic · Fix: use keep_binaries: true (dropping force_dev_binaries) in the executed run.

• Adds macOS-only logic and a macOS-only test, against AGENTS.md §6 (new) — apps/rocm/src/uninstall_gate.rs:832-845 (the conjunct process_start_ticks(std::process::id()).is_some() in record_predates_start_ticks), apps/rocm/src/main.rs:35205 (#[cfg(not(any(target_os = "linux", windows)))] without_readable_start_times_a_bare_record_is_not_a_legacy_record)

  • §6 says: "platforms outside Windows/Linux are unsupported; do not implement, debug, or 'fix' unsupported-platform behavior".
  • On both supported platforms the conjunct is always true, as the change's own comments admit ("unconditionally true there", "unfalsifiable on this lane"). Its only effect is on macOS.
  • The test guarding it "compiles only on macOS, which has no CI lane", so no lane ever runs it.
  • The same macOS framing repeats in comments at main.rs around the paired test and in crates/rocm-core/src/proc_lifecycle.rs.
  • Confidence 85 · architectural · Fix: drop the macOS-only test and reduce the predicate to recorded_start_ticks.is_none(). If the stub arm must stay, keep a one-line note at most.

• The description misstates the change, both in what it locates and in what it leaves out (standing; re-found blind this round) — PR description, Changes and Test plan

  • Wrong locations:
    • It places stop_managed_services_before_uninstall() in apps/rocm/src/main.rs; it lives in the new apps/rocm/src/uninstall_gate.rs:406.
    • It says rocmd records daemon_start_ticks in apps/rocmd/src/lib.rs; the capture is at apps/rocmd/src/service.rs:741.
    • It gives ManagedServiceStopReport as { stopped, failed }; it also carries warnings and helper_stopped.
  • User-visible behaviour it never mentions:
    • --keep-binaries --keep-data runs skip the stop pass and leave live servers running (plan_removes_recovery_tooling, main.rs:~18008). This is documented in --help and the README, but not in the PR text.
    • Windows now reads process start-times through GetProcessTimes (proc_lifecycle.rs, #[cfg(windows)] process_start_ticks). That changes daemon identity verification on Windows from best-effort to strict.
    • The plan warnings are reworded.
    • The write_active_runtime_marker rewrite (main.rs:9217-9234).
    • The changed public-bind advice in ensure_public_service_has_endpoint_key (main.rs:3816).
  • Test plan: it names "new install_lifecycle scenario" (one). The change adds three: lifecycle-10 (Linux), lifecycle-24 (Windows) and lifecycle-25 (Linux refusal).
  • Confidence 85 · mechanical · Fix: update the description to match the change (narrow nothing in the code).

The items below are standing objections from earlier rounds that this round's blind review did not re-find. Each was then put to the reviewer, with your note, after its report was frozen; those verdicts are anchored (the reviewer was told what to look for) and static (nothing was run). Only the part that still holds is listed.

• A stopped record that waits on a live partner gets the wrong remedy when that partner's stop fails (standing, narrowed) — apps/rocm/src/uninstall_gate.rs:550, :567, :708

  • The spelling half is fixed for the named spellings: shares_a_live_endpoint (:466) now compares probe-address sets, so 0.0.0.0/localhost against a live 127.0.0.1 defers.
  • Still holds: deferred records are probed at :550, and the only skip is for a record already in failed under its own id (:567). When the live partner's stop failed and it is still serving, the deferred record is blocked as BlockServingOurModel with StopWhatHoldsThePort (:708), whose text says every recorded process is gone, beside the partner's correct StopTheService line. No test covers a failed partner with a deferred record.
  • Worth checking: overlap works only through loopback, so a live 0.0.0.0:P and a stopped LAN-IP/hostname record on :P, or a live ::1 and a stopped localhost, still share no probe address.
  • Fix: skip (or re-attribute) a deferred record whose live partner is in failed.

• A kill the identity check refuses still prints "kill that pid" (standing, narrowed) — apps/rocm/src/uninstall_gate.rs:376-383, :92-98, :174-182

  • Fixed on the main path: an unverifiable or pre-start-time record now gets ConfirmTheDaemonPid (:180, :367).
  • Still holds: when terminate(&identity) returns TerminationOutcome::Unverified ("deliberately left untouched", proc_lifecycle.rs:223), the gate pushes StopTheDaemon — "kill that pid" — for that same unverified pid. Whether this arm is reachable outside a re-read race was not established.
  • The objection also asked to name the state file; the new advice says "delete the stale runtime state file" and prints no path (the blind review lists this separately under Non-blocking).
  • Fix: map Unverified to ConfirmTheDaemonPid, keep StopTheDaemon for TimedOut, and print automation_state_path().

• Discovery fail-closed: each ? alone is still unpinned (standing, narrowed) — apps/rocm/src/uninstall_gate.rs:436, :453; test at apps/rocm/src/main.rs:34824-34896

  • Fixed: the read-error test now drives uninstall_with_paths and asserts an error and a surviving planned file, so removing both ? is caught.
  • Still holds: both ? read the same directory, so either one alone can be swallowed and the other still aborts the run — the test stays green. The test also returns early with only an eprintln! when the directory is readable (root), passing having checked nothing (:34834-34839).
  • Fix: a case where only one read fails (needs a seam), and fail rather than return when the precondition cannot be set up.

• Added tests that still cannot fail on the branch they cover (standing, narrowed) — apps/rocm/src/uninstall.rs:84-90, apps/rocm/src/uninstall_gate.rs:869; tests in apps/rocm/src/main.rs

  • Now caught: recycled → Stop (by every_daemon_identity_reading_maps_to_exactly_one_gate_outcome), and dropping helper_stopped from the success condition (a_helper_only_stop_prints_a_success_line).
  • Still green: deleting the stderr warning loop (uninstall.rs:84-86) and the success println! (:88-90) — nothing captures output; a wrong reason/remedy for a service this pass just stopped (uninstall_refuses_while_a_recorded_endpoint_still_accepts_connections checks ids only, main.rs:34085-34101); widening the auth-refusal check (uninstall_gate.rs:869) to >= 300 — the only staged status is 401.
  • uninstall_never_kills_a_daemon_pid_that_was_recycled still does not assert !helper_stopped.
  • Fix: one command-level test capturing stdout/stderr; assert reason and remedy; stage a 404/500 endpoint expecting ProceedUnidentified; add the !helper_stopped assertion.

• README and --help misstate what uninstall does (standing, in full) — README.md:818-823, apps/rocm/src/main.rs:606-610, plan warnings in build_uninstall_plan

  • The rocmd kill is never disclosed before confirmation; the first mention is uninstall_gate.rs:371, after the prompt, and it runs even with no service records (:487).
  • "Leaves every file in place" (README.md:821) is false after a mid-run abort: earlier stops already deleted endpoint key files and rewrote records (main.rs:15550-15561), as the abort text at uninstall_gate.rs:933-951 admits.
  • AGENTS.md §5. Fix: plan warning when the helper is recorded running; mention it in README and --help; say stopped servers stay stopped and lose their keys.

• Two user-observable behaviors have no scenario (standing, in full) — tests/e2e-cucumber/features/install_lifecycle.feature

  • No scenario runs --keep-binaries --keep-data with a live server (the only keep step, lifecycle_steps.rs:1012-1023, does not pass --keep-binaries), and none plants a live rocmd before uninstall. lifecycle-10/-24/-25 stage a service record, not the helper.
  • The PR text gives no justification for the gap. AGENTS.md §3. Fix: add the scenarios, or justify the gap in the PR text.

• The manual-testing refusal check cannot be followed as written (standing, in full) — docs/manual-testing.md:383-397

  • Step 1 is a full rocm uninstall --yes; step 2 then corrupts services/bad.json, runs uninstall "again", and checks directories that are already gone. The doc never says services is under the data directory.
  • Fix: start step 2 from a fresh install and give the full services path.

• The core atomic writer's write/flush cleanup and collision loop are untested (standing, narrowed) — crates/rocm-core/src/lib.rs:4669-4830

  • Now tested: publish-failure cleanup (write_file_atomically_cleans_up_when_publishing_fails, lib.rs:5136-5155).
  • Still holds: three staging copies remain (core, therock.rs:4637, sandbox.rs:653); the core copy has no hooks, so removing either remove_file(&tmp) after a failed write_all/sync_all, or breaking the AlreadyExists loop, stays green. The other two copies test these through their hooks.
  • Fix: give the core helper the same cfg(test) hooks and have therock and sandbox call it.

• Six fail-closed branches that no test exercises (standing, in full) — apps/rocm/src/uninstall_gate.rs:592-610, :896, :270, :272; apps/rocm/src/main.rs:~18015; crates/rocm-core/src/lib.rs:4697-4699

  • Each stays green under its mutation: host_port_resolves forced false; the non-.json filter dropped (every planted file is *.json); the starts_with(&services_dir) arm forced false (no plan ever has an action inside the services dir — the arm may be unreachable, i.e. a dead-code question too); KillScope::Single and force: false (the stand-in is a childless sleep); delete-then-rename in unix publish_temp_file.
  • Fix: one test per branch that fails when it is mutated, or remove an unreachable arm.

Non-blocking

• Several comments added by this change contradict the code. In each case the code governs, because it is what runs and what the tests assert: (standing in part)

  • uninstall_gate.rs:537-539 and main.rs:34111 say "nothing prunes them, and there is no services remove". Both exist: ServicesCommand::Remove at main.rs:998 and Prune at main.rs:1014. This comment is the stated reason for failing open on recycled ports (see the related item under Decisions for the author).
  • uninstall_gate.rs:636 says the tests "live at the bottom of this file". They are in main.rs's mod tests, and uninstall_gate.rs has none.
  • main.rs:34506-34512 says the empty-listing warning "is pinned by nothing … emptying its body … would stay green". The same test asserts that warning at main.rs:34537-34546.
  • main.rs:34336 and main.rs:34343 refer to probe_host, which is now probe_hosts.
  • The probe_hosts doc block at uninstall_gate.rs:712-730 has hosts_name_one_listener inserted below it (line 731). Rustdoc therefore attaches the wildcard/IPv6 essay to the wrong function and leaves probe_hosts undocumented.
  • uninstall.rs:7-11 still says "Mechanically relocated … with no behavior change". The file now owns the stop-then-remove orchestration and the uninstall_with_paths seam.
  • crates/rocm-core/src/lib.rs:4762 says "Two limits" and then lists three.
  • Confidence 85 · mechanical · Fix: correct each comment to match the code.

• The ConfirmTheDaemonPid advice says to "delete the stale runtime state file" without naming it (new) — uninstall_gate.rs:92-98, :174-182

  • That failure's service_id is rocmd (pid N), so the abort shows no path.
  • The sibling RepairTheDaemonState advice prints the path. Under AGENTS.md §3, advice has to be followable.
  • Confidence 80 · logic · Fix: include the state file path (automation_state_path()) in that failure's reason or advice.

• The advised serve command is checked as a string, never parsed (new) — uninstall_gate.rs:936, main.rs:3816

  • AGENTS.md §3 requires advice that names a command to be backed by a test that the command exists and accepts those flags.
  • The tests assert the substring only, and the main.rs:3816 rewording has no test.
  • I read the flags against clap and they are correct today.
  • Confidence 70 · logic · Fix: add a clap try_parse_from test for the advised line with placeholders filled in, or derive the text from one shared constant.

• No Windows test exercises the gate's stop-pass logic (new) — the stop-pass tests in main.rs and uninstall.rs are #[cfg(target_os = "linux")] or #[cfg(unix)]

  • On Windows only lifecycle-24 runs. It pins PID termination against a port-0 record, as its own comment says.
  • Untested on Windows:
    • the running: false guard, which the comment on uninstall_never_signals_a_daemon_pid_recorded_as_already_stopped calls "the only defence" on Windows;
    • the ordering in which a doomed run stops nothing;
    • the post-stop port probe.
  • Confidence 70 · logic · Fix: un-gate the tests that need no stand-in (for example stop_background_helper_with against a running:false state file), use a PowerShell Start-Sleep child where lifecycle_steps.rs already does, or state the gap in the PR text.

• 21 commits carry Co-Authored-By: Claude … trailers (new) (for example f323e7f and 4fa47a8). AGENTS.md §11 says "avoid AI-generated boilerplate footers", and a squash-merge carries them into the landed commit body. Confidence 70 · mechanical · Fix: strip them when squashing.

• example.internal at main.rs:35328 matches the AGENTS.md §2 leak-scan pattern \.internal (new). It is a placeholder, but it will hit the scan on every push. Confidence 70 · mechanical · Fix: use example.test.

Decisions for the author

• Cache-only runs report uninstall complete while managed servers keep serving — tradeoff
  • The ticket's expected behaviour includes "Never report uninstall complete while a rocm-cli-managed endpoint is still serving."
  • --keep-binaries --keep-data does exactly that, deliberately: the tooling survives, so the server stays stoppable. The plan warning, --help and the README all disclose it.
  • The alternative is to kill live GPU servers for a cache cleanup. Confirm that this departure from the ticket's wording is intended.
• Already-stopped records answering with other models fail open silently — non-blocking-improvement
  • The leniency rests on "there is no services remove", and that is false.
  • Because rocm services remove <id> --yes exists, a stricter gate could block and name that remedy instead of proceeding.
• About 2,000 lines of uninstall_gate.rs tests sit in main.rs's mod tests — non-blocking-improvement. AGENTS.md §6 points new subsystems at their own file, and these tests grow the god-file rather than the module they test.
• The atomic-write consolidation rides along with the uninstall fix — tradeoff
  • ManagedServiceRecord::write going atomic does serve the gate, which refuses on unparseable manifests.
  • The write_active_runtime_marker, therock.rs and sandbox.rs publish dedup is drive-by work under §11's one-logical-change rule.
  • The dedup also leaves two staging copies (apps/rocmd/src/sandbox.rs:655-762 and rocm-core) that differ in sync_all. That is the twin-implementation shape this repo's defects cluster in.

Positive signals

• The stop pass is injected into stop_managed_services_then_remove, so the ordering tests assert on a planned file surviving an abort, not on a return value.
• The plan warning and the stop decision both derive from plan_removes_recovery_tooling, so the printed promise cannot disagree with the behaviour. This is the pattern AGENTS.md §3 prefers.
• The gate reuses stop_internal_managed_service (the rocm services stop path) rather than adding a second stop implementation.
• The ReplaceFileW publish is now single-sourced in rocm-core, and windows-sys was removed from the two app crates.
• EndpointIdentity keeps "lists nothing" distinct from "serves other models", and an HTTP 401/403 counts as evidence of a live server rather than as a failed probe.

Deployment notes

• Older daemons make uninstall refuse: a rocmd started by a pre-upgrade binary has no daemon_start_ticks. While it is live, rocm uninstall treats it as unverifiable and refuses to remove anything until the operator stops that pid by hand (daemon_identity_outcome, uninstall_gate.rs:199-211).
• Windows daemon identity is now strict: the Windows rocmd records now carry start-times. A pid that cannot be opened (for example an elevated daemon) reads as Indeterminate, so it blocks uninstall where it used to be treated as a match.

What this covered

Read:

• Every file in the change, 0a021785…e0ae7699 (the merge-base with prw-base c1c2aeb3).
• The surrounding callers and callees: the rocm services commands, daemon_binary_path, collect_installed_binary_candidates, and the proc_lifecycle identity helpers.

How it was split: four independent workers, each reading its group completely, plus my own pass:

• the gate and its production wiring;
• the added tests and the e2e feature and steps;
• rocm-core and rocmd;
• agent-instruction adherence, history, docs, code comments and commit quality.

Prior changes: this pass was done only as the history diff against prw-base. I read no earlier PR discussions.

Clean merge: git merge-tree against prw-base reports no conflicts.

CI for e0ae7699: every reported check passed or was skipped. No self-hosted GPU lane appears in the rollup, and I did not establish which lane runs the Windows scenario lifecycle-24.

Did not run:

• No builds, tests or mutation checks: df -h / showed 39G free, under the 40G floor. Every test verdict and finding was reached by reading and cross-checking the code.
• The cfg(windows) arms were read by eye only, since there is no Windows linker on this host.
• The design questions (Step 7) were investigated by me, not by independent workers.

Ticket: read EAI-8014 (title, description, status) through the tracker, without its comments.

Not reconciled against the PR's review discussion. The round note, the new/standing tags and the standing items after the italic line in Blocking were added by the driver; the anchored checks were given the objection and are not part of what this review read blind.

[fredespi]

#299 (review)

Pushed 7720885 and rewrote the PR description. Taken: (1) the read-error test now runs the real command with keep_binaries (no dev-binary override), so a regression cannot delete the shared build output; the services-dir abort still triggers. (2) macOS-only logic removed per AGENTS.md section 6: record_predates_start_ticks is now just is_none(), the macOS-only test and its commentary are gone, and the macOS framing in the gate's docs is trimmed. (3) Description corrected: uninstall_gate.rs location, daemon_start_ticks in rocmd/service.rs, the report's warnings and helper_stopped fields, Windows GetProcessTimes start-times, the keep-binaries/keep-data exemption, reworded plan warnings, write_active_runtime_marker, and an honest scenario list (lifecycle-10/24/25; the exemption and helper stop are unit-tested only, no scenario). Acer gate green. Not done: README/--help/plan helper-kill disclosure and the 'leaves every file' wording, the exemption/helper scenarios and manual-testing steps, the remaining mutation survivors, host-spelling failed-partner case, the terminate->Unverified 'kill that pid' arm, atomic-writer cleanup tests.

[rominf]

🔴 Automated review · pr-review-watcher · 7720885

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 (round 13, full review of the whole change). This round files a change request again; it replaces the one at e0ae7699 as this account's whole position. Every blocking item is tagged new or standing. It also covers your note on 77208854 (I reacted to it rather than replying separately).

• Withdrawn after the anchored check: the read-error test can no longer reach the real binaries (the executed run uses keep_binaries: true; force_dev_binaries is only in a plan that is never executed). The macOS predicate and test are gone too. The one leftover, a macOS line in a module doc, is now under Non-blocking.
• Narrowed, still standing: the description is fixed except one unmentioned message change.
• Still standing in full (you listed these as not done, and the anchored checks agree): failed-partner remedy, the Unverified arm, each discovery ? alone, the remaining mutation survivors, README/--help, manual-testing steps, atomic-writer tests, fail-closed branches. The missing scenarios were re-found blind.
• New this round: auth-refusal advice on a stopped record's old port, force-kill of pre-upgrade Windows service PIDs, the five-of-six advice test, the port-probe test that cannot fail, and further untested branches.
• Not a reason for this change request: CI. All reported checks at 77208854 passed; two were skipped.
• Verification this round was static. The review host's disk was too low to build, so no build, test or mutation was run. Every verdict below was reached by reading the code.

Review — needs work

Full review of the whole change.
Implements the expected behaviour of the named ticket EAI-8014: stop managed services before removal, abort non-zero and keep the tooling when a stop can't be confirmed. That key now resolves to ROCMAI-290, a Bug, In Progress.

Blocking

• The rule for records stopped before this run rests on a false premise, and its auth-refusal advice blames the wrong cause (new) — apps/rocm/src/uninstall_gate.rs:520-524, apps/rocm/src/uninstall_gate.rs:79-85, apps/rocm/src/uninstall_gate.rs:660-673, apps/rocm/src/main.rs:34111 (test comment); the commands are at apps/rocm/src/main.rs:998, :1014, :4792
  The design comment says stopped records live forever because "nothing prunes them, and there is no services remove". At HEAD, ServicesCommand::Remove ("Delete one local server record and its files") and ServicesCommand::Prune both exist. The code governs here: those commands ship.
  Now take a record already stopped before this run, whose old port now hosts an unrelated server that requires auth (another keyed vLLM, a reverse proxy). That server answers 401/403, which gives BlockAuthRefused, and the abort prints StopWhatHoldsThePort. That advice says "Every process recorded for {id} is gone, yet the endpoint still answers — the engine outlived its supervisor" and tells the operator to stop whatever holds the port. For a stranger, that cause is false, and the advice asks them to kill an unrelated service. It never mentions rocm services remove <id> --yes, which by its definition deletes the record and so would clear the condition.
  AGENTS.md §3 requires that remediation advice name a command that "actually clear[s] the condition that printed it". Every retry hits the same abort, so the comment's stated goal ("must not brick uninstall with no way out") is not met by what the advice offers.
  Confidence 85 · logic · Fix: correct the premise in both comments. For already-stopped records that block on an auth refusal, word the reason as "an authenticated server answers on this record's old port". Offer rocm services remove <id> --yes as the way out when it isn't yours, and pin the new advice together with the resulting state.

• Uninstall now force-kills unverified PIDs from older Windows service records, which the daemon path refuses to do (new) — apps/rocm/src/main.rs:15465-15500 (terminate_recorded_service_pids), crates/rocm-core/src/proc_lifecycle.rs:175, apps/rocm/src/main.rs:16640-16654, contrast apps/rocm/src/uninstall_gate.rs:819 (record_predates_start_ticks)
  At prw-base, process_start_ticks returns None on every non-Linux platform, so every Windows service record written before this upgrade carries no start-time.

  • Liveness refresh only checks process_is_running. After a reboot that recycles the PID, the record stays live (starting/ready).
  • stop_managed_services_with then stops it with terminate_verified(.., KillScope::Tree, .., force=true).
  • compare_start_ticks returns Matches for (None, _), so an unrelated process gets force-killed.

  rocm services stop could already do this, but only when a user ran it. This PR makes rocm uninstall --yes do it automatically to every live-status record, so the weakness now has a much wider reach. The PR's own daemon path treats the same condition as unverifiable and aborts. The description's "every Windows managed-service stop can now return Recycled or Indeterminate" holds only for records written after the upgrade.
  Confidence 70 · architectural · Fix: in the uninstall stop pass, give a live record whose PIDs carry no start-time the daemon's treatment: abort, and advise rocm services stop/a manual check, rather than force-killing.

• every_failure_class_carries_its_own_recovery_advice claims every class but covers five of six (new) — apps/rocm/src/main.rs:33951-34010, apps/rocm/src/uninstall_gate.rs:92-97
  The test's comment says "a sixth variant cannot compile without being given text … across all five at once". StopFailureRemedy has six variants, and ConfirmTheDaemonPid is missing from the report, the phrase list and the ordering check. The only other mention of it (main.rs:33737) asserts the enum value, never its advice. Blanking that advice or reordering its rank leaves the suite green.
  The untested text is also wrong as rendered: ids already holds the label rocmd (pid 4321), so it prints "Check what pid rocmd (pid 4321) is". It also says "delete the stale runtime state file" without naming the path.
  Confidence 85 · logic · Fix: add the sixth failure, assert its distinctive phrase and position, fix the wording and the "five"/"sixth" comments.

• uninstall_refuses_while_a_recorded_endpoint_still_accepts_connections can't fail on the claim its comment makes (standing; re-found blind this round) — apps/rocm/src/main.rs:34042-34100; same wait() pattern at apps/rocm/src/main.rs:33644
let _ = supervisor.wait(); has no bound. If the stop never ran, wait() returns when sleep 60 exits, and the assertion "the recorded process must really have been stopped, so only the port probe can catch the survivor" then passes anyway.
  The test asserts only failed == ["svc-orphaned-engine"]. That also holds when the stop simply fails, which yields StopTheService and "still "ready"", so it can't show the port probe produced the failure. The "still accepts connections after the stop" reason arm (uninstall_gate.rs:677) is asserted nowhere; only its sibling arm is.
  Confidence 85 · logic · Fix: use the existing assert_stopped_within_grace, and assert remedy == StopWhatHoldsThePort and reason.contains("after the stop").

• Several branches this change introduces are reached by no test (new; the stderr-warnings part is standing) — apps/rocm/src/uninstall_gate.rs:307, apps/rocm/src/uninstall.rs:87, apps/rocm/src/uninstall_gate.rs:716-722, endpoint_refused_authorization in apps/rocm/src/uninstall_gate.rs

  • daemon_pid guards. Neither the daemon_pid == 0 nor the daemon_pid == std::process::id() guard in stop_background_helper_with is exercised; only the !state.running conjunct is. Dropping the own-PID check lets rocm force-kill its own tree.
  • Warnings printed to stderr. The eprintln! loop that prints stop_report.warnings before the gate is untested. Every uninstall.rs test passes warnings: Vec::new(), and this loop is the only user-visible disclosure of the fail-open paths.
  • localhost mapping. The "localhost" => "127.0.0.1" canonicalization in hosts_name_one_listener is untested.
  • 403 and the stored key. The 403 half of the 401/403 auth-refusal check, and passing the stored endpoint key to the identity probe, can each be broken alone with the suite still green.

  Confidence 70 · logic · Fix: add a case for each, e.g. runtime_state(true, 0) and runtime_state(true, std::process::id()) expecting no failure and helper_stopped == false, plus a report with a warning asserted on stderr.

• Two user-observable behaviours have no Gherkin scenario, and the gap isn't justified (standing; re-found blind this round) — tests/e2e-cucumber/features/install_lifecycle.feature
  AGENTS.md §3: a change that alters what the user observes "must be covered by a Gherkin scenario"; only "purely internal changes" are exempt, and they must say why. Two behaviours here are covered by unit tests only, as the PR text itself says:

  • The --keep-binaries --keep-data exemption: the plan says servers are left running, exit 0, and the server keeps serving.
  • Uninstall also stopping the background helper rocmd.

  Both are visible to the user, neither is internal, and no gated lane is named.
  Confidence 85 · architectural · Fix: add scenarios for both, or state in the PR text why each cannot run and which lane would.

The items below are standing objections from earlier rounds that this round's blind review did not re-find, or re-found only in part. Each was then put to the reviewer, with your note, after its report was frozen. Those verdicts are anchored (the reviewer was told what to look for) and static (nothing was run). Only the part that still holds is listed.

• The description leaves out one user-visible change (standing, narrowed) — PR description

  • Fixed: file locations, the report's fields, Windows GetProcessTimes, the keep-binaries/keep-data exemption, the plan warnings, write_active_runtime_marker, and the three-scenario test plan.
  • Still holds: the public-bind advice in ensure_public_service_has_endpoint_key (apps/rocm/src/main.rs:3816) now reads rocm serve <model> --host {host} --port <port> --allow-public-bind, and the description does not mention it.
  • The Windows start-time claim is also affected by the second new blocking item above.
  • Fix: mention the advice change; qualify the Windows claim to records written after the upgrade.

• A stopped record that waits on a live partner gets the wrong remedy when that partner's stop fails (standing) — apps/rocm/src/uninstall_gate.rs:533-534, :548-553, :684-692, :79-85

  • Deferred records are probed after the stop loop. The only skip is for a record in failed under its own id, and a failed partner is filed under the partner's id.
  • The deferred record then gets BlockServingOurModel → StopWhatHoldsThePort ("every process recorded … is gone"), beside the partner's correct StopTheService line, so the survivor is named twice.
  • Both deferral tests (apps/rocm/src/main.rs:34777, :34905) use a stop that always succeeds.
  • Fix: skip, or re-attribute, a deferred record whose live partner on the same endpoint is in failed.

• A kill the identity check refuses still prints "kill that pid" (standing; reachable only in a re-read race) — apps/rocm/src/uninstall_gate.rs:358-368, crates/rocm-core/src/proc_lifecycle.rs:297

  • Every non-stopped() outcome of terminate, including Unverified, pushes StopTheDaemon.
  • The pre-check at uninstall_gate.rs:340-353 already maps Indeterminate to ConfirmTheDaemonPid, so this needs a start-time that read once and then failed while the pid stayed alive.
  • The missing state-file path is part of the third new blocking item above.
  • Fix: map Unverified to ConfirmTheDaemonPid and keep StopTheDaemon for TimedOut.

• Discovery fail-closed: each ? alone is still unpinned (standing) — apps/rocm/src/uninstall_gate.rs:868-869, :437; test at apps/rocm/src/main.rs:34831-34838

  • Both reads hit the same directory, and the first always aborts before the second, so removing either one alone stays green. No seam fails only one.
  • When the directory stays readable (root), the test prints "SKIPPED…" and returns as a pass, which libtest hides.
  • Fix: a seam to fail one read at a time, and fail rather than return when the precondition cannot be set up.

• Added tests that still cannot fail on the branch they cover (standing, narrowed) — apps/rocm/src/uninstall.rs:~90, apps/rocm/src/uninstall_gate.rs:~846, apps/rocm/src/main.rs:33794

  • Now caught by e2e: deleting the final "uninstall complete" line (lifecycle_steps.rs:1279-1286).
  • Still green: deleting the gate's "stopped N managed service(s) before removal" line; widening the 401/403 check to >= 300 (the only status staged against the gate is 401, main.rs:34234).
  • uninstall_never_kills_a_daemon_pid_that_was_recycled still does not assert !helper_stopped.
  • Fix: capture stdout in one command-level test; stage a 404/500 expecting ProceedUnidentified; add the !helper_stopped assertion.

• README and --help misstate what uninstall does (standing; the blind review re-found the substance and rated it non-blocking, see below) — README.md:819-824, the Uninstall doc comment at apps/rocm/src/main.rs:~607

  • The rocmd kill is not disclosed before confirmation.
  • Neither says that an aborted run leaves the services and helper it already stopped down, with their endpoint keys dropped; only the abort text does.
  • AGENTS.md §5 requires these docs to change in the same change. Fix: add both to the README and the doc comment.

• The manual-testing refusal check cannot be followed as written (standing) — docs/manual-testing.md:~381-397

  • Step 1 is a full rocm uninstall --yes. The refusal check then corrupts services/bad.json, runs uninstall "again", and expects directories that are already gone. No reinstall is described.
  • The services directory is data_dir/services (crates/rocm-core/src/lib.rs:1984-1986), and the doc gives no path.
  • Fix: start the refusal check from a fresh install and give the full services path.

• The core atomic writer's write/flush cleanup and collision loop are untested (standing) — crates/rocm-core/src/lib.rs:4786-4835

  • write_file_atomically has no test hooks, so the remove_file(&tmp) after a failed write_all (:4818-4822) or sync_all (:4827-4831) cannot be provoked.
  • No test pre-creates the temp name, so the AlreadyExists retry loop (:4792-4808) and its exhausted bail! are unexercised.
  • therock.rs and sandbox.rs keep their own staging, so three copies remain.
  • Fix: give the core helper cfg(test) hooks and have therock and sandbox call it.

• Fail-closed branches that no test exercises (standing) — apps/rocm/src/uninstall_gate.rs, apps/rocm/src/main.rs:18015, apps/rocm/src/uninstall_gate.rs:~259-262, crates/rocm-core/src/lib.rs:4697

  • host_port_resolves forced false: no unreachable-but-resolvable case asserts there is no warning.
  • The non-.json filter dropped: the only planted file is svc-corrupt.json.
  • The starts_with(&services_dir) arm (main.rs:18015): the data root is caught by the next arm, so this one looks unreachable (dead code, or a test gap).
  • KillScope::Single or force: false: the stand-ins are childless sleeps.
  • Delete-then-rename in unix publish_temp_file: the reader test holds an fd, so it passes either way.
  • Fix: one test per branch that fails when it is mutated, or remove the unreachable arm.

Non-blocking

• Comments made stale or misplaced by this change — apps/rocm/src/uninstall_gate.rs:697-724, apps/rocm/src/uninstall_gate.rs:620, apps/rocm/src/main.rs:35227, apps/rocm/src/uninstall.rs:7, docs/architecture.md:29, crates/rocm-core/src/lib.rs:4762

  • The probe_hosts doc block sits on hosts_name_one_listener, and probe_hosts has none.
  • "tests live at the bottom of this file": uninstall_gate.rs has no test module, and the tests are in main.rs.
  • The test comment says process_start_ticks is always None on Windows, which this PR makes false.
  • uninstall.rs still says "Mechanically relocated … with no behavior change", and architecture.md still lists it under "no owned types", though it now holds the stop-then-remove wiring.
  • "Two limits worth knowing" heads three bullets.

  Confidence 85 · mechanical · Fix: correct each one to match the code.

• A module doc still frames macOS behaviour as a design point (standing, narrowed; the rest of the macOS objection is fixed) — crates/rocm-core/src/proc_lifecycle.rs:16-31
  It says "on other platforms (macOS)" the check "degrades to best-effort" and that this is "deliberate". AGENTS.md §6 treats platforms outside Linux and Windows as unsupported. Only a doc comment remains, so this no longer gates. Fix: say that other platforms are unsupported.

• Two remedies share an advice_rank, which can break the dedup — apps/rocm/src/uninstall_gate.rs:129-130, :942-943
StopTheDaemon and ConfirmTheDaemonPid both rank 2. sort_unstable_by_key followed by dedup (which only removes adjacent duplicates) can then print a sentence twice, in a non-deterministic order. Today each run records only one helper failure, so this is latent.
  Confidence 70 · mechanical · Fix: give every variant a unique rank.

• a_cache_only_uninstall_leaves_a_live_server_running passes if uninstall_with_paths does nothing — apps/rocm/src/uninstall.rs
  It catches the predicate being forced true, which is its stated purpose. It never asserts that cache/blob was removed, so it doesn't show that "left the server alone" happened alongside the removal it was meant to accompany.
  Confidence 70 · logic · Fix: assert !paths.cache_dir.join("blob").exists().

• The identity probe throws away the status that http_get_with_auth now returns — crates/rocm-core/src/lib.rs:954-1018
managed_service_endpoint_identity goes through the text helper, so a 401 becomes Err, and the gate needs a second request (endpoint_refused_authorization) to recover it. The expected-model derivation is also duplicated between managed_service_endpoint_model_ready and managed_service_endpoint_identity, and their empty-URL handling has already diverged (Ok(false) vs bail!).
  Confidence 70 · architectural · Fix: build the identity on http_get_with_auth, add a refused variant, and extract a shared expected_model(record).

• README and --help leave out two things the command does — README.md:819-824, the Uninstall doc comment at apps/rocm/src/main.rs:~607
  Both mention only stopping managed model servers. They omit that uninstall also force-stops rocmd, and that an aborted run leaves the services and helper it already stopped down, with their endpoint keys dropped. Only the abort text says so (AGENTS.md §5).
  Confidence 70 · mechanical · Fix: add both to the README and the doc comment.

• The new module doesn't follow repo conventions — apps/rocm/src/uninstall_gate.rs:18, apps/rocm/src/uninstall.rs:66-99
use crate::*; is the only glob import of the crate root in apps/rocm/src. The completion and abort lines are hand-rolled println! rather than the shared cli_report::ActionReport (AGENTS.md §6).
  Confidence 70 · mechanical · Fix: use explicit imports, and route the completion lines through ActionReport.

• Tests skip silently and leak children — apps/rocm/src/main.rs (an_ipv6_only_wildcard_engine…, the_plan_read_error_warning…)
  These two announce "SKIPPED" only on eprintln!, which the harness hides for passing tests. The second also skips as root and restores permissions only on success.
  Most of the new process-spawning tests hold a bare Child (a sleep 60) rather than KillOnDrop, so a panic leaks it.
  Confidence 70 · mechanical · Fix: make skips visible, e.g. with #[ignore] plus a reason, or by failing loudly; wrap spawned children in KillOnDrop.

Decisions for the author

• Any unparseable services/*.json aborts uninstall — tradeoff
  This includes a stale stopped record from an older schema, which rocm services list skips silently. The fail-closed choice is safe and the abort names the file. On the other side, every such run needs a hand edit, and the docs never say where the services directory is.
• Some already-stopped records proceed while their endpoint still answers — tradeoff
  When the endpoint lists no models or doesn't identify itself, uninstall proceeds with only a stderr warning. That avoids bricking uninstall on a recycled port. Against it, the ticket's "Never report uninstall complete while a rocm-cli-managed endpoint is still serving": a wedged or still-loading engine of ours lands in exactly these arms.
• About 1,950 lines of gate tests went into main.rs's mod tests — non-blocking-improvement
  uninstall_gate.rs was extracted as its own module but has no tests of its own, so the god-file grows. Moving them alongside the gate would keep each arm's tests next to the arm, and make the "tests live in this file" comment true.

Positive signals

• StoppedRecordVerdict::blocks and StopFailureRemedy::advice/advice_rank are exhaustive matches, so a new variant can't default to the side that removes the tooling.
• Passing the stop into stop_managed_services_then_remove as a parameter makes the stop-before-remove ordering assertable against a real file surviving.
• The non-destructive checks (scanning unparseable manifests, probing already-stopped records) run before anything is killed, so a run that is going to abort stops nothing.
• plan_removes_recovery_tooling drives both the plan warning and the skip of the stop pass, so the message can't disagree with what the code does (the AGENTS.md §3 pattern).
• The /proc/<pid>/stat parse uses rfind(')'), so a comm with spaces or parentheses is safe. The Windows GetProcessTimes path opens with PROCESS_QUERY_LIMITED_INFORMATION and closes the handle on every path.
• daemon_start_ticks is #[serde(default, skip_serializing_if = …)], so old and new state files read both ways.

Deployment notes

• A rocmd started before the upgrade has no daemon_start_ticks. The first rocm uninstall after upgrading aborts until it is stopped by hand.
• Service manifests, the automation runtime state and the active-runtime marker are now published by rename:
  • The new file gets umask permissions rather than the old file's mode.
  • A symlink at the path is replaced, not written through.
  • A killed writer can leave *.tmp-… siblings behind. The services loader skips them by extension.
• windows-sys moves from apps/rocm and apps/rocmd into rocm-core, with Win32_Storage_FileSystem added.

What this covered

I read every file in prw-base...772088549674293971bd0fe123db841e49e65531 completely, across four coupled groups:

1. The uninstall gate and its wiring, plus the doc diffs.
2. rocm-core/rocmd start-time capture, atomic writes and Cargo manifests.
3. The new unit tests in main.rs and uninstall.rs, read against the production code they target.
4. The e2e scenarios and step definitions, with an AGENTS.md adherence check, a leak scan and a history pass.

I also read the callers of the changed symbols (stop_internal_managed_service, terminate_recorded_service_pids, liveness refresh, ServicesCommand). The design questions were investigated in the driver context, not as separate workers.

Verification was static only because the shared disk is nearly full: no builds, tests or mutation runs. The test verdicts above are reasoned from the code, and no score is 100. CI on the reviewed head shows all 25 checks green; Skill checks (skillscope) and Regenerate manifests were skipped. No check in the rollup names a Windows lifecycle or GPU lane, so I cannot tell whether lifecycle-24 ran.

Did not run or degraded:

• Prior-changes pass: earlier review comments on these files were deliberately not read.
• Merge commits: the six Merge origin/main merges were not checked for bad conflict resolution (no --remerge-diff was run).
• Commit signatures and DCO: not checkable locally; CI's Commit signatures + sign-off passed.
• Worker discipline: one worker briefly wrote and then deleted a scratch diff file under /tmp, despite the read-only instruction. Nothing in the checkout was modified.

Not reconciled against any PR discussion here. No prompt-injection content was found. The round note, the new/standing tags, the standing items after the italic line in Blocking and the narrowed macOS item under Non-blocking were added by the driver; the anchored checks were given the objection and are not part of what this review read blind.

[fredespi]

#299 (review)

Pushed (see head). Taken: (1) an already-stopped record whose old port answers with an auth refusal now has its own remedy, RemoveTheStoppedRecord: reason 'an authenticated server answers on host:port, this record's old port', advice offers 'rocm services remove --yes' if it isn't yours (or stop it if it is); no more 'the engine outlived its supervisor'. (2) failed-partner: a stopped twin is no longer judged when its live partner on the same endpoint already failed, so one survivor is not named twice; test added. (3) every_failure_class test now covers all variants incl. ConfirmTheDaemonPid and the new remedy; the daemon label now names the state-file path and the advice wording is fixed. (4) tests for the daemon_pid==0 / own-pid guards and the localhost mapping. Not done: force-kill of pre-upgrade Windows service PIDs (needs a design call: aborting on records with no start-time would also abort every legacy Linux record), 403 / stored-key / stderr-warning tests, the unbounded wait() in the orphaned-endpoint test, the keep-binaries/helper scenarios, README/--help helper-kill disclosure, and the description's public-bind advice line.

[rominf]

🔴 Automated review · pr-review-watcher · cbb5d1f

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 (round 14, full review of the whole change). This round files a change request again. It replaces the one at 77208854 as this account's whole position. Every blocking item is tagged new or standing. It also covers your note on 77208854 (I reacted to it rather than replying separately).

• Dropped as resolved: the failed-partner remedy. The new skip uses the same partner list as the deferral, and a_failed_live_partner_is_not_named_twice_through_its_stopped_twin fails without it.
• Narrowed: the auth-refusal advice is fixed, and so are the daemon-pid guards, the localhost mapping, and the every_failure_class phrases and wording. Only the parts listed below remain.
• Moved to non-blocking: what is left of the every_failure_class ordering check, and of the discovery ? objection. What remains there changes only advice order, or is redundant with the other read. Neither leaves anything wrong behind.
• Your design question on the Windows force-kill is answered in that item below. In short: Linux records have carried a start time since #117. A predicate like the daemon's, or a Windows-only scope, refuses only tick-less records.
• Two blind findings do not gate this change request.
  • The duplicated advice line: it repeats a paragraph and leads to no wrong action.
  • The description gap: the squash body comes from commit messages, so it has no consequence after merge.
  • Both are still worth fixing.
• CI: no failures at cbb5d1fb. Skillscope and manifests were skipped. The self-hosted Strix Halo lanes (Windows, Ubuntu release/nightly) were cancelled, so lifecycle-24 has not run on Strix Windows hardware for this head.
• Inline comments: none this round. Every finding is below.

Review — needs work

Full review of the whole change.
The PR names EAI-8014, which is now ROCMAI-290 ("rocm uninstall reports success while leaving a publicly-bound GPU-holding server running…"). The change implements that ticket's expected behaviour, with one deliberate exception: a --keep-binaries --keep-data run leaves running servers alone and still prints uninstall complete (see Decisions).

Blocking

• The same advice is printed twice when two failure classes share a rank and arrive interleaved (new; not gating this change request, see round note) — apps/rocm/src/uninstall_gate.rs:985-986

  • classes.sort_unstable_by_key(|r| r.advice_rank()); classes.dedup(); only removes adjacent duplicates.
  • StopWhatHoldsThePort and RemoveTheStoppedRecord both rank 1, so failures in the order [Hold, Remove, Hold] stay in that order after the sort, and dedup drops nothing.
  • This order can happen in one run: a single probe_records_for_survivors call (:499) walks several already-stopped records and can give each one either remedy (:704, :735).
  • I reproduced it with a test added locally next to two_failures_of_one_class_get_that_classes_advice_once_and_name_both_ids. The abort message contains "Every process recorded for svc-a, svc-c is gone … Find what holds that port …" twice. That contradicts the contract the existing test's name states ("one class, one piece of advice"). The existing test uses a single class, so it cannot see this.

  Confidence 100 · logic · Fix: give the sort a total order, e.g. sort_unstable_by_key(|r| (r.advice_rank(), *r as u8)), or dedup by membership. Add the interleaved case as a test.

• The description leaves out a remediation-message change in an unrelated command (standing, re-found blind; not gating, see round note) — apps/rocm/src/main.rs:3816

  • ensure_public_service_has_endpoint_key guards rocm services restart. Its advice now reads rocm serve <model> --host {host} --port <port> --allow-public-bind.
  • Nothing in the PR body mentions this. "The uninstall plan warnings are reworded" covers a different string.
  • Changes nobody mentions are changes nobody reviews, and this one has a twin it did not fix (see Non-blocking).

  Confidence 85 · mechanical · Fix: mention it in the description, or move it to its own change together with the rocmd twin.

The items below are standing objections from earlier rounds that this round's blind review did not re-find, or re-found only in part. Each was then put to the reviewer, with your note where one applied, after its report was frozen. Those verdicts are anchored (the reviewer was told what to look for) and static unless stated. Only the part that still holds is listed. These are what this change request gates on.

• Uninstall force-kills tick-less service PIDs that the daemon path refuses to kill (standing; the blind review re-found the facts and filed them as a tradeoff, see Decisions; the anchored check says it is a defect) — apps/rocm/src/main.rs (terminate_recorded_service_pids), crates/rocm-core/src/proc_lifecycle.rs (compare_start_ticks), contrast apps/rocm/src/uninstall_gate.rs (record_predates_start_ticks)

  • Every Windows record written before this upgrade has no start time. (None, _) maps to Matches, so after a reboot that recycles the PID, rocm uninstall --yes force-kills an unrelated process tree.
  • rocm services stop acts only on a record the user names. Uninstall now sweeps every live record automatically, so this PR widens the reach.
  • On your design question: at the base, process_start_ticks already returned a value on Linux (proc_lifecycle.rs:305-310), and Linux records have stored supervisor_start_ticks since #117 (2026-07-17). The daemon's predicate (refuse "no recorded ticks" only where the platform can read a start time) refuses Windows legacy records and pre-July Linux records, and nothing else. A Windows-only scope avoids Linux entirely.
  • Fix: give live tick-less records the daemon's treatment: abort, and advise rocm services stop or a manual check.

• No test shows that the new rocm services remove <id> --yes advice clears the block (standing, narrowed; the advice itself is fixed) — apps/rocm/src/main.rs:34020

  • The test only checks that the message contains the command. AGENTS.md §3 requires the covering test to assert the resulting state too: remove the record, re-run the gate, show it proceeds.
  • The false premise "there is no services remove" also survives in a test comment at main.rs:34129. The other copy is under Non-blocking below.

• uninstall_refuses_while_a_recorded_endpoint_still_accepts_connections cannot tell a port-probe catch from a failed stop (standing, narrowed) — apps/rocm/src/main.rs:34098

  • A blind mutation shows the probe branch is pinned: removing it fails the test.
  • Still holds:
    • failed == ["svc-orphaned-engine"] also holds if the stop itself fails (StopTheService).
    • let _ = supervisor.wait(); is unbounded, so the "really stopped" assertion after it is vacuous.
    • The "after the stop" reason arm is asserted nowhere; the only match is a negative !contains at main.rs:34337.
  • Fix: assert_stopped_within_grace, remedy == StopWhatHoldsThePort, and reason.contains("after the stop").

• Two introduced branches remain untested (standing, narrowed; the daemon-pid guards and localhost mapping are now covered) — apps/rocm/src/uninstall_gate.rs:889, :648/:657, apps/rocm/src/uninstall.rs (warnings loop)

  • 403: the only refusal fixture answers 401. Drop || parts.status == 403 and a 403 server on a stopped record's port is no longer blocked. Uninstall then proceeds with only a warning, removing the tooling while that server keeps serving.
  • Stored key: stop passing endpoint_api_key and our own key-protected survivor gets the stale-record advice instead.
  • Warnings on stderr: every uninstall.rs test passes warnings: Vec::new(). Break the eprintln! loop and every fail-open path goes silent.
  • Fix: stage a 403 and a keyed survivor; assert one warning reaches stderr.

• Two user-observable behaviours have no Gherkin scenario, and the gap isn't justified (standing) — tests/e2e-cucumber/features/

  • No scenario covers the --keep-binaries --keep-data run with a live server, or uninstall stopping rocmd.
  • The PR body admits both are "covered by unit tests only" but gives no reason and names no lane, which AGENTS.md §3 requires.
  • Fix: add the scenarios, or state why each can't run and which lane would.

• A daemon kill the identity check refuses still advises "kill that pid" (standing) — apps/rocm/src/uninstall_gate.rs:377-387

  • Every !outcome.stopped() result, Unverified included, maps to StopTheDaemon. When the start time reads in the pre-check and then fails during the stop while the pid stays alive, the user is told to kill/Stop-Process a pid the gate just declined to signal.
  • The window is narrow and untested.
  • Fix: map Unverified to ConfirmTheDaemonPid.

• Added tests that still cannot fail on the branch they cover (standing)

  • Deleting the println! of the "stopped N managed service(s) before removal" line in uninstall.rs: no test captures stdout.
  • Widening the 401/403 check to >= 300: a 404 or 500 from an unrelated server on an old port would then falsely abort. Only a 401 is staged (main.rs:34252).
  • uninstall_never_kills_a_daemon_pid_that_was_recycled never asserts !helper_stopped. A wrong flag would make the abort text claim rocmd "was stopped".
  • Fix: capture stdout in one command-level test; stage a 404/500 expecting ProceedUnidentified; add the assertion.

• README and --help misstate what uninstall does (standing) — README.md:819-824, apps/rocm/src/main.rs:605-610

  • Neither mentions that uninstall stops the background helper rocmd.
  • Neither says that an aborted run leaves already-stopped servers and the helper down, with their endpoint keys dropped.
  • AGENTS.md §5. Fix: add both.

• The manual-testing refusal check cannot be followed as written (standing) — docs/manual-testing.md §8

  • It runs a full rocm uninstall --yes first, then expects the directories that removed to still exist after a second run, with no reinstall in between.
  • It gives no full services path (the data dir followed by /services).
  • Fix: start from a fresh install and give the full path.

• The core atomic writer's failure branches are untested, and three staging copies remain (standing) — crates/rocm-core/src/lib.rs:~4786-4835, apps/rocm/src/therock.rs:4639-4691, apps/rocmd/src/sandbox.rs:655-703

  • Untested: the temp cleanup after a failed write_all/sync_all, the AlreadyExists retry, and the exhausted bail!.
  • Only the publish step was single-sourced.
  • Fix: cfg(test) hooks on the core helper; have therock and sandbox call it.

• Fail-closed branches no test exercises (standing, narrowed; the unreachable starts_with arm is under Non-blocking)

  • Non-.json filter: drop it and any non-JSON file under services/ becomes a false "unreadable manifest" abort.
  • Publish order: delete-then-rename in unix publish_temp_file passes the reader test either way (not re-verified this round). That would reopen the window in which a reader finds no record.
  • Kill scope: KillScope::Tree/force for the daemon stop are not pinned, because the stand-ins are childless.
  • Name resolution: host_port_resolves is not pinned against a resolvable-but-dead endpoint.
  • Fix: one test per branch that fails when mutated.

Non-blocking

• The twin of the corrected restart advice still names a command that cannot run — apps/rocmd/src/common.rs:353-356

  • The rocmd copy still prints `rocm serve --host {host} --allow-public-bind`, which omits the required <model> positional.
  • Its doc comment says it "Mirrors the guard of the same name in rocm", and this PR fixed only the rocm copy.
  • The --require-api-key sentence at :350-351 has the same gap. That one is pre-existing and not caused by this change.

  Confidence 85 · mechanical · Fix: apply the same wording to the rocmd copy.

• Comments that the change made stale or that contradict the code — five places:

  • apps/rocm/src/uninstall_gate.rs:554-555 says "there is no services remove". ServicesCommand::Remove exists (main.rs:998), and RemoveTheStoppedRecord advises rocm services remove <id> --yes.
  • apps/rocm/src/uninstall_gate.rs:663-666 says "The tests live at the bottom of this file", and so does the StoppedRecordVerdict doc. The file has no #[cfg(test)]; the tests are in main.rs.
  • apps/rocm/src/uninstall_gate.rs:~740-756: the doc paragraph for probe_hosts is attached to hosts_name_one_listener, and probe_hosts has none.
  • crates/rocm-core/src/proc_lifecycle.rs:48-51 says start_ticks "is None on platforms without /proc", and apps/rocm/src/main.rs:35314 says "always None there [Windows]". Windows now reads start times through GetProcessTimes.
  • crates/rocm-core/src/lib.rs:4762 introduces "Two limits worth knowing" and then lists three.

  In each case the code governs: it is what runs. Confidence 90 · mechanical · Fix: correct the comments.

• New tests spawn sleep through PATH without the env lock, so cargo test is flaky locally

  • Sites: apps/rocm/src/uninstall.rs:425, plus about 17 new Command::new("sleep") sites in main.rs tests (prw-base has 4).
  • therock.rs:7012 sets PATH to a scratch bin/ under PROCESS_ENV_TEST_LOCK, which these tests do not take.
  • Observed in one of my full unmutated cargo test -p rocm --bin rocm runs: a_live_managed_server_is_stopped_before_the_planned_paths_are_removed panicked with "failed to spawn the managed server: … NotFound".
  • CI on Linux does not show this. AGENTS.md §8 makes cargo test --workspace the local gate, and that gate is where it fails. The pattern is pre-existing, but this change multiplies it about fivefold.

  Confidence 85 · mechanical · Fix: hold the env lock in these tests, or spawn sleep by absolute path.

• The clause entry.path.starts_with(&services_dir) is unpinned — apps/rocm/src/main.rs:18015

  • Replacing it with false left the whole rocm bin suite green (1068 passed).
  • Plan entries are only binaries plus the config, data and cache dirs, so the clause looks unreachable.

  Confidence 85 · mechanical · Fix: drop it, or add a test that reaches it.

• About 2,000 lines of uninstall_gate.rs tests sit in main.rs's mod tests — apps/rocm/src/main.rs:~33560-35600, ~36000-36230

  • AGENTS.md §6 and the PR's own docs/architecture.md entry list uninstall_gate.rs as a full domain extraction, and the sibling modules (serve_cmd.rs, storage.rs, therock.rs, uninstall.rs) keep their tests in-file.
  • Nothing technical forces the placement: test_paths and ScopedTestEnv are already imported from crate::tests by other modules.

  Confidence 70 · architectural · Fix: move them into uninstall_gate.rs (and the command-level ones into uninstall.rs).

• The uninstall.rs tests build scratch roots from temp_dir() plus pid and millis — apps/rocm/src/uninstall.rs:137, :310, :486

  • prw-base replaced this pattern with a shared unique-suffix helper (7f72556). The branch predates that commit.
  • A test-root collision already exists here: restart_refuses_a_public_service_without_a_key_before_stopping_it failed in two of three local runs because two tests share the label "restart-public-no-key". That flake belongs to the stale base and goes away once the branch takes main.

  Confidence 70 · mechanical · Fix: use the shared helper after updating the branch.

• every_failure_class_carries_its_own_recovery_advice: the ordering check covers only the five original classes (standing, moved here from Blocking) — apps/rocm/src/main.rs:~33951

  • ConfirmTheDaemonPid and RemoveTheStoppedRecord are not in the position chain.
  • Within a shared rank, the order can't be pinned at all.
  • The comment says "three classes" above a list of five.
  • Fix: add both to the chain; give each class a unique rank, which also fixes the duplicated-advice finding above.

• The discovery fail-closed test passes silently as root (standing, moved here from Blocking) — apps/rocm/src/main.rs:~34831

  • Dropping either ? alone is harmless, because the other read fails closed on the same directory and files.
  • What remains: when the directory stays readable (root), the test prints "SKIPPED" and passes.
  • Fix: fail, or #[ignore] with a reason, when the precondition can't be set.

Decisions for the author

• Tick-less service records are force-killed while a tick-less daemon is refused — tradeoff (the anchored check after the review rates this a defect; it gates as the first standing item under Blocking)

  • The uninstall sweep calls terminate_recorded_service_pids. A record with no start ticks reaches compare_start_ticks → Matches (best-effort) and is signalled.
  • The daemon in the same situation is refused, on the PR's own argument that killing an unrelated process cannot be undone.
  • The asymmetry matters most on Windows: every record written before this change has supervisor_start_ticks = None. A record still reading ready after a reboot whose pid was recycled would get a stranger terminated by uninstall, which is now a sweep over all records.
  • The other option is refusing tick-less live records with a remedy, at the cost of blocking upgraders. Lifecycle-24 relies on the best-effort kill.
  • Confirm the asymmetry is deliberate, and document it.

• --keep-binaries --keep-data prints uninstall complete while servers keep running — tradeoff

  • ROCMAI-290 says "Never report uninstall complete while a rocm-cli-managed endpoint is still serving."
  • The PR deliberately exempts tooling-preserving runs, and the plan warning, --help and README all say so. The ticket's wording does not.

• The upgrade path aborts on a running old rocmd, with weak advice — non-blocking-improvement

  • A pre-upgrade rocmd records no daemon_start_ticks, so every rocm uninstall aborts with ConfirmTheDaemonPid until it is killed by hand.
  • That advice ("if it is rocmd, stop it") gives no command and does not mention the upgrade case, which will be the common one. StopTheDaemon does give kill/Stop-Process. No rocm subcommand stops the daemon.
  • Its other branch, deleting the state file, makes uninstall treat the helper as absent. That fails open if the pid really was rocmd.

• The PR carries adjacent refactors — non-blocking-improvement

  • The publish dedup in apps/rocmd/src/sandbox.rs and apps/rocm/src/therock.rs (including the Windows ReplaceFileW move and the windows-sys manifest changes) and the write_active_runtime_marker rewrite are not needed by the gate path. The description does mention them.
  • Splitting them out would keep the Windows-only, never-clippy-gated FFI move reviewable on its own (AGENTS.md §11, one logical change).

Positive signals

• stop_managed_services_then_remove takes the stop pass as a parameter. That makes the stop-before-remove ordering testable by asserting that planned files survive, rather than by inspecting the code.
• The plan warning and the gate both branch on the same plan_removes_recovery_tooling predicate, so the promise printed before confirmation cannot disagree with what the run does. This is the pattern AGENTS.md §3 asks for.
• A mutation that removed the survivor probe for records stopped in this pass made uninstall_refuses_while_a_recorded_endpoint_still_accepts_connections fail. So that branch is genuinely pinned.

Deployment notes

• If an older rocmd is still running (its state file has no daemon_start_ticks), rocm uninstall aborts until that rocmd is killed by hand.
• Windows: process_start_ticks now returns Some. A record written with ticks, whose process cannot be opened at stop time, now yields Indeterminate and is not stopped, where before it was terminated. This affects rocm services stop as well as uninstall.
• rocm-core gains the Win32_Storage_FileSystem feature of windows-sys; apps/rocm and apps/rocmd drop their direct windows-sys dependency.

What this covered

• What was read. Every file in the change, completely: the gate, the uninstall command, the main.rs plan and test hunks, rocm-core, rocmd, therock, the e2e feature and steps, and the docs. Also their callers and callees, and the sibling stop path stop_internal_managed_service / terminate_recorded_service_pids.
• Commits. The diff is 0a02178…cbb5d1fb. prw-base (c1c2aeb) is 6 commits further on; merge-tree merges cleanly.
• What I ran (cargo test -p rocm --bin rocm, shared target, fresh build):
  • the filtered uninstall tests: 40 passed;
  • full unmutated suite twice: one different failure each time, both explained above;
  • three mutations;
  • one added probe test, the duplicate-advice reproduction.
• CI. All required checks pass. The self-hosted Strix Halo lanes (Ubuntu release and nightly, Windows) were cancelled together, mid-run, for this head. So lifecycle-24 has not run on Strix Windows hardware for this commit. Those lanes are not required.
• Fan-out.
  • Two of the four code-pass workers spawned; the other two hit the concurrent-subagent cap.
  • The tests worker was re-spawned once later and ran independently.
  • The e2e and docs pass (feature file, step definitions, README and docs) was run by me, so it was not independent.
  • Two design-question workers ran.
• Did not run.
  • Prior-changes pass: earlier review discussion is out of bounds here.
  • Windows cfg arms: checked by reading only, not compiled.
  • The rocm-core worker's pass was brief, though it reported reading its whole group.
• Not reconciled against any discussion here.

The driver added these: the round note, the new/standing tags and gating notes, the standing items after the italic line in Blocking, the two standing items at the end of Non-blocking, and the parenthetical on the first Decisions item. The anchored checks were given the objection and the author's note; they are not part of what this review read blind.

[fredespi]

#299 (review)

Fixed in 6555f0e: live tick-less service pids are now refused (new ConfirmTheServicePid remedy) instead of force-killed; advice classes have a total order so none prints twice (test added); an Unverified daemon kill maps to ConfirmTheDaemonPid; rocmd restart advice matches rocm; stale comments corrected. Not done this push: extra test coverage items, scenarios for keep-binaries/helper stop, README/--help, test relocation.

[siloteemu]

🔴 Automated review · pr-review-watcher · d1e2aae

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.

Review — needs work

Full review of the whole change (prw-base...HEAD, merge-base 0a021785, 21 files, +5.5k lines, 50 first-parent commits).
The description says it stops managed servers before rocm uninstall removes anything, and adds start-time identity checks (Linux and Windows) and a shared atomic-write helper; it names no ticket other than the title's bare EAI-8014.

Blocking

Round history (added by the poster): this automation's previous round, at b062582d, filed five blocking findings. Each is marked here as new or standing. Two of them were re-found blind this round and are tagged where they appear below. The other three were not re-found blind. Each was then put to this round's reviewer after its report was frozen, together with the author's account of what changed. That check was anchored: the reviewer was told the objection, so it is evidence on the question asked and not a blind finding. All three still hold in part. They are appended below with the original sites. Each one says which parts still hold and which no longer do. Nothing was withdrawn: every one of the five still holds in at least one part.

• [new] A live vLLM-managed server is refused by the new gate with advice that does not apply to it. — apps/rocm/src/uninstall_gate.rs:481-511, crates/rocm-core/src/lib.rs:4320-4335, engines/vllm/src/state.rs:140-148, apps/rocm/src/main.rs:15470 (the kill that used to run)
  The gate refuses any live record whose pid has no recorded start time (record_predates_start_ticks(*ticks) && process_start_ticks(*pid).is_some()). refresh_from_engine_state takes engine_pid from server_pid and its ticks only from server_start_ticks. The vLLM engine writes server_pid (the Python child, distinct from the launcher recorded as supervisor_pid) but only start_ticks, never server_start_ticks. A running vLLM service therefore always reads as an unverifiable engine pid (only Lemonade writes server_start_ticks). I ran the gate (stop_managed_services_with) on a record of that shape (launcher with ticks, a distinct live engine pid with engine_start_ticks: None) and it returned ConfirmTheServicePid: "no start time was recorded for live pid(s) [..] ... cannot be told apart from a reused pid". Every uninstall with a live vLLM server (the very scenario in the description) would abort with the "record was written before start times were recorded" advice, and a retry aborts the same way. Before this PR that pid took the best-effort stop. Every test fixture uses engine_pid == supervisor_pid (managed_record_for_pid, main.rs:33462), which the pid de-duplication at uninstall_gate.rs:488-489 collapses, so none reaches this shape. The weakness (vLLM omits server_start_ticks) predates the PR; the PR enlarges its blast radius by making it a hard refusal.
  Confidence 85 · architectural · Fix: have the vLLM state write also record server_start_ticks for server_pid (or have refresh_from_engine_state fall back to start_ticks when server_pid == pid), and add a gate test with a distinct engine pid whose ticks come only from engine state.

• [new] The recovery advice for a tick-less live pid ends in the unsafe kill the gate refused to do. — apps/rocm/src/uninstall_gate.rs:95-103, apps/rocm/src/main.rs:5198-5221
ConfirmTheServicePid says: if the pid "is something else, delete the stale record with rocm services remove <id> --yes". The case only arises for a pid that is running. remove_managed_service_record refuses when managed_service_record_is_in_use (a recorded pid is running) and answers "Try: rocm services stop <id> --yes". That stop runs terminate_recorded_service_pids on an identity with no start time, which takes the best-effort Matches arm and force-kills the tree of the unrelated process. The text also says "Check what each pid is ({})" but fills the placeholder with service ids, not pids. The code governs: the removal guard is deliberate (its doc says so), so the advice is what must change.
  Confidence 85 · logic · Fix: name an action that works for a live-pid record (delete the manifest file by path, as RepairTheRecord does), print the pids, and add a test that follows the named command against such a record.

• [new; also re-finds blind part of the standing liveness-assertion objection from b062582d: the endpoint test and the never-kills tests] Added tests stay green with the production branch they name broken. Verdicts (reverted / single-branch mutation), run on this head with cargo test -p rocm --bin rocm and -p rocm-core --lib:

  • Zombie check in identity_state_with_observed — crates/rocm-core/src/proc_lifecycle.rs:198. Deleting || process_has_exited(id.pid) leaves all 598 rocm-core tests and the 40 uninstall tests green. The only test reaching the Gone arm (an_exited_pid_reads_as_gone_whatever_start_time_is_observed, :490-508) reaps the child first, so the zombie clause is never reached; on Linux a zombie still has /proc/<pid>/stat, so without it a zombie reads as Matches (the verdict that permits a kill). Confidence 100.
  • TerminationOutcome::Unverified -> ConfirmTheDaemonPid — uninstall_gate.rs:393-397. Changing the condition to false && leaves the full rocm bin suite green (1070 tests); a_daemon_stop_that_is_not_confirmed_aborts_and_a_confirmed_one_is_recorded (main.rs:~36096) injects only TimedOut and Graceful. Added in a fix commit named after this branch. Confidence 100.
  • ConfirmTheServicePid advice text — uninstall_gate.rs:95-103. Replacing the whole arm with String::new() leaves everything green; every_failure_class_carries_its_own_recovery_advice (main.rs:~33960) claims "every class" but its fixture has 7 of 8 variants. Confidence 100.
  • "still accepts connections after the stop" vs "is recorded stopped" wording — uninstall_gate.rs:781. Forcing stopped_by_this_pass to false leaves everything green; uninstall_refuses_while_a_recorded_endpoint_still_accepts_connections (main.rs:34060) asserts only the failed id, and ends with an unbounded supervisor.wait() before !process_is_running, the pattern assert_stopped_within_grace's own doc calls "measuring nothing". Confidence 100.
  • "Never kills an unrelated pid" tests — main.rs:33728, :33820, :35118, :35122. They assert with process_is_running, which is true for an unreaped killed child (kill(pid,0) succeeds on a zombie), so they cannot detect the kill their messages name. Inserting a terminate_verified(.., None ticks ..) of the daemon pid into the NothingOfOurs arm (uninstall_gate.rs:379) leaves uninstall_never_kills_a_daemon_pid_that_was_recycled and the whole suite green (confirmed by run). The sibling test at ~35290 already uses try_wait and explains why. Confidence 100.
    Fix: use try_wait().is_none() for the liveness assertions, add the Linux unreaped-zombie test, add the Unverified case and the missing ConfirmTheServicePid fixture entry, and assert the reason text and bounded wait in the endpoint test.
    100 · logic

• [new] Windows GetProcessTimes call will fail the now-required Windows clippy gate. — crates/rocm-core/src/proc_lifecycle.rs:431 (GetProcessTimes(handle, &mut created, &mut exited, &mut kernel, &mut user))
  The base moved after this branch: #488 (0d85577) runs clippy --workspace -D warnings in the required windows-build-and-test job, with pedantic enabled (Cargo.toml workspace.lints.clippy), and converted exactly this pattern to &raw mut (see crates/rocm-core/src/lib.rs:1335 on the base). This branch's cfg(windows) code has never been through Windows clippy (the description gates only Linux), and the unsafe block carries no // SAFETY: comment, unlike replace_file_windows beside it. I could not run Windows clippy here (only a Linux target is installed), so this is argued from the base's lint config and its fix commit.
  Confidence 80 · mechanical · Fix: &raw mut for all four arguments, a SAFETY comment, and run Windows clippy on the rebased tree.

• [standing from b062582d (its AGENTS.md section 3 part), re-found blind] User-observable uninstall behaviour with no Gherkin scenario (AGENTS.md section 3). — tests/e2e-cucumber/features/install_lifecycle.feature
  Scenarios exist and exercise real behaviour: lifecycle-10 (Linux stop), lifecycle-24 (Windows stop), lifecycle-25 (unreadable record refuses and keeps everything). The description states that the --keep-binaries --keep-data exemption and the helper stop are "covered by unit tests only"; section 3 says a unit test of the internal helper does not discharge this, and gives no justification for the gap. Also uncovered: refusal on an unverifiable (tick-less) record, refusal on an unreadable runtime-state.json, and the helper-stop line. All three scenarios are @lifecycle, which runs only when changes.heavy is set, so none is on the always-on mock lane; the base's features/uninstall.feature (added after this branch, mock lane, already passes --keep-binaries) is the natural home. Mutating removes_recovery_tooling (uninstall.rs:55-58) to always true is caught only by a unit test.
  Confidence 85 · rule cited (AGENTS.md section 3) · Fix: add scenarios for the keep-binaries+keep-data exemption and the unreadable-state refusal on the mock lane, a tick-less-record scenario beside lifecycle-10, and name the @id:/lane for each in the PR text.

• [new] Comments and docs this PR made stale or wrong. (a wrong comment is a defect; the side that governs is the code in each case, per the reason given)

  • apps/rocm/src/uninstall_gate.rs:801-820: the 18-line probe_hosts doc sits above hosts_name_one_listener, so rustdoc attaches it there and probe_hosts (:828) has none.
  • uninstall_gate.rs:345-371: describes reading "our own PID" to tell the platform, and "one reading of the daemon's start-time"; the code at :372-377 reads neither of those (record_predates_start_ticks just tests is_none()). crates/rocm-core/src/lib.rs:2880-2891 (new daemon_start_ticks doc) prescribes the same dropped approach. The code governs: record_predates_start_ticks (:920-923) says both supported platforms record a start time.
  • uninstall_gate.rs:274 ("off Linux it is the only one that applies"), :354-367 ("/proc", "best-effort Matches arm meant for Windows"), lib.rs:2880, proc_lifecycle.rs:50-51 ("ticks since boot" — on Windows it is a FILETIME), proc_lifecycle.rs:182, apps/rocm/src/uninstall.rs:394 ("Some only on Linux"), main.rs:33559 ("None elsewhere"): all predate the Windows reader this PR adds. macOS reasoning in new comments (proc_lifecycle.rs:15-30, lib.rs:2890) designs for an unsupported platform (AGENTS.md section 6).
  • apps/rocm/src/uninstall.rs:7-10 ("mechanically relocated ... no behavior change") and the matching docs/architecture.md wording: this PR adds the stop-before-remove behaviour to that module.
  • docs/manual-testing.md:394-397: the refusal procedure says to corrupt a services file and run rocm uninstall --yes "again", straight after step 1 succeeded; on Linux that run already removed the binary and the config/data directories, so it cannot be followed.
  • install_lifecycle.feature:290-300: says the grandchild case is covered by "unit tests around stopped_record_verdict"; the stopped-this-pass block is a different function (uninstall_gate.rs:692-797) and its test (main.rs:~34058) is Linux-only, so no Windows lane covers the port check.
  • main.rs:34128-34129 and :34527-34533: test comments that contradict themselves and the assertion beneath them.
    Confidence 85 · mechanical · Fix: reword each to what the code does; move the doc block onto probe_hosts; repair the manual procedure (reinstall, create the services dir).

• [new] The PR does not merge cleanly and no check except the docs build has reported on this head. — README.md, docs/architecture.md
gh pr view reports mergeable: CONFLICTING; git merge-tree prw-base HEAD conflicts in README.md (the base added two symlink/uncheckable-folder paragraphs in the same spot; keep all three) and docs/architecture.md (the base reflowed the module map into per-module list items, so the uninstall_gate.rs entry must be redone as one bullet). The status rollup holds only the Read the Docs status. No Linux or Windows build, clippy, coverage or e2e check has run, so the Windows code, the new e2e scenarios and the coverage floors are unverified by CI (I ran the rocm bin tests, 1070 passed, and the rocm-core lib tests, 598 passed, on Linux only).
  Confidence 100 (observed) · mechanical · Fix: rebase onto the current base, resolve the two docs conflicts, then drive CI to green.

• [new] The description does not match the change in two places. — apps/rocm/src/main.rs:~3816, apps/rocmd/src/common.rs:355
  It does not mention that the public-bind respawn/restart advice was reworded in two places unrelated to uninstall (rocm serve <model> --host {host} --port <port> --allow-public-bind). Its test plan says each unit test "was verified against a faithful revert of the production change it names"; the mutations above show several are not (the exceptions it names are only a directory-hygiene check and a false-positive guard). Remedy: narrow the claim, or finish the tests; mention the advice change or split it out.
  Confidence 85 · mechanical

• [standing from b062582d; not re-found blind; settled by an anchored check this round: still holds for 3 of 7 sites] Several branches the PR adds are exercised by no test.

  • Original sites: daemon stop not confirmed; the ProceedUnidentified warning; warnings printed to stderr (apps/rocm/src/uninstall.rs); the 403 arm (status == 401 || 403); the API key passed to the identity probe (endpoint_api_key.as_deref()); classes.dedup(); the "No files were removed." tail.
  • Still holds at d1e2aaeb (each mutation applied alone; full cargo test -p rocm --bin rocm stayed green): the stderr warning loop replaced by a no-op (uninstall.rs:87-88); status == 401 || status == 403 narrowed to 401 (uninstall_gate.rs:950); both endpoint_api_key.as_deref() arguments replaced by None (uninstall_gate.rs ~709 and ~718). Without the key a guarded endpoint is refused at identification rather than identified, and no test tells those apart.
  • No longer holds: the daemon-not-confirmed push, the ProceedUnidentified warning, dedup() and the tail are now each caught by a test.
  • Confidence 100 (ran) · mechanical · Fix: add a test for each remaining site.

• [standing from b062582d; not re-found blind; settled by an anchored check this round: still holds for the daemon half] The new Windows start-time capture has no Windows test, and no Windows lane runs the daemon-stop path it enables.

  • No longer holds: crates/rocm-core/src/proc_lifecycle.rs now has two #[cfg(windows)] tests of process_start_ticks.
  • Still holds: apps/rocmd/src/service.rs a_fresh_runtime_state_records_this_daemons_start_time is still #[cfg(target_os = "linux")], and every daemon-stop test in apps/rocm/src/main.rs is unix or linux. If build_runtime_state recorded daemon_start_ticks: None on Windows, every uninstall that finds a live rocmd would abort with the pre-upgrade advice and no Windows lane would catch it. No e2e scenario has a live rocmd.
  • Confidence 85 · logic · Fix: run that capture test on Windows too (its assertion is platform-neutral).

• [standing from b062582d; not re-found blind; settled by an anchored check this round: the ids-in-advice and always-warns parts still hold] Two test assertions cannot fail on the branch they name.

  • Still holds: replacing every ids.join(", ") in StopFailureRemedy::advice with String::new() leaves the suite green. every_failure_class_carries_its_own_recovery_advice and the new two_failures_of_one_class_get_that_classes_advice_once_and_name_both_ids match ids against the whole abort message, whose opening detail already lists every id.
  • Still holds: forcing the unresolvable-host warning on for every closed port (uninstall_gate.rs ~677) leaves the suite green. No test asserts warnings.is_empty() for a resolvable host with nothing listening.
  • No longer holds: per-class grouping is now exercised; deleting dedup() fails two tests.
  • Confidence 100 (ran) · logic · Fix: assert the ids inside the advice text (or test advice(&ids) directly), and assert warnings.is_empty() in a resolvable-but-refused case.

The other two standing findings from b062582d were settled the same way. The liveness-assertion finding still holds for the endpoint test, the pre-start-ticks daemon test and the recycled-pid daemon test, all covered by the third blocking item above. It no longer holds for the trailing live.wait() in uninstall_skips_already_dead_managed_service, which runs after every assertion. The description finding: its five omissions and the scenario count are now fixed. Its AGENTS.md section 3 part still holds and is the fifth blocking item above.

Non-blocking

• [new] The tick-less check treats "could not read this pid's start time" like "pid is dead". — uninstall_gate.rs:496-499; test comment main.rs:35279-35286 says the conflation is "prevented by construction". A live pid whose start time cannot be read (for example hidepid, a transient read failure) skips the refusal and reaches the best-effort kill. process_is_running(*pid) is the question being asked. Confidence 75 · logic.
• [standing] identity_state_with_observed has no remaining reason to exist, and its one caller reads the start time before the liveness check, the order its own doc (proc_lifecycle.rs:186-190) warns about. — uninstall_gate.rs:373-376, proc_lifecycle.rs:179-202, re-export lib.rs:91. terminate_verified re-checks with the safe order before signalling, so no harm results; identity_state gives the same answer. Confidence 80 · architectural.
• [new] A live wildcard-bound record plus a stopped record of the same model on a specific address is misjudged. — uninstall_gate.rs:538-550, :820-826. probe_hosts for 0.0.0.0 is loopback only, so no overlap is seen; the stopped record is probed first, the live sibling answers with the same model, and the run aborts with "engine outlived its supervisor". Confidence 70 · logic.
• [new] Service records are loaded and liveness-refreshed (readiness plus inference probes, possible rewrite) twice in one stop pass, and the pid de-duplication loop is copied — uninstall_gate.rs:476, :519, :480-493 versus main.rs:15468-15483. Confidence 80 · mechanical.
• [new] The abort headline always says "could not stop managed service(s)", including when the only failures are an unreadable daemon state or a corrupt manifest. uninstall_gate.rs:1067. Confidence 70 · mechanical.
• [standing] write_file_atomically is a third copy of the staging logic (crates/rocm-core/src/lib.rs:4672-4682, :4785-4836; others in therock.rs:4640-4775, sandbox.rs:656-756), and the core copy has no test seam, so its write-failure and sync_all cleanup branches are untested. Confidence 70 · architectural.
• [new] Windows test gating disagrees between two siblings. lib.rs:4941 a_runtime_state_write_never_shows_a_reader_a_truncated_file is ungated while its twin :4871-4874 is #[cfg(unix)] on the stated ground that Windows ReplaceFileW "does not offer the same observation". One of the two is wrong; check the Windows job. Confidence 70 · logic.
• [new] Weak or environment-dependent tests. a_service_record_write_leaves_no_scratch_sibling (lib.rs:4989) passes with a plain fs::write (its own comment says so; the sibling at :5052 covers the property). the_plan_read_error_warning_only_promises_a_refusal_where_the_stop_pass_runs returns green when run as root and is the only test of that warning text (stage the failure with a directory named x.json instead). the_daemon_stop_skips_pid_zero_and_its_own_pid: the pid-0 half is vacuous (process_is_running(0) already returns false at uninstall_gate.rs:342). Several #[cfg(unix)] tests depend on process_start_ticks returning Some, which only Linux does. Several tests leave a 60 s sleep running on panic (a KillOnDrop exists). a_record_whose_host_no_longer_resolves does a real DNS lookup. Confidence 70-85 · mechanical.
• [new] New fixtures use <label>-<pid>-<millis> roots (uninstall.rs:136-141, :311, :487, lib.rs:4850) instead of the base's test_support::unique_suffix() (#603 showed that scheme collides within a process). Labels are distinct today. Confidence 70 · mechanical.
• [standing in part: the tests-in-main.rs point] Structure. About 2,000 lines of uninstall_gate tests sit in main.rs mod tests and uninstall_gate.rs:18 uses use crate::*, while docs/architecture.md describes it as full domain extraction reached by qualified paths. Hand-rolled println!("stopping ...") for a bounded grace wait (uninstall_gate.rs:389, :578) is the long-quiet-wait case the shared AnimatedSpinner exists for (AGENTS.md section 6). The e2e step hardcodes the mock helper's file name e2e-mock.json (lifecycle_steps.rs:533) rather than adding the field to ServiceRecordOptions. Confidence 70.
• [new] User docs are incomplete. README (:819-824), the --help text and docs/manual-testing.md do not say uninstall also stops rocmd, that a pre-upgrade record or state file causes a refusal rather than a stop, or that services stopped before an abort stay stopped and lose their endpoint keys. Confidence 70.
• [new] Commit hygiene. 50 first-parent commits, 7 merges from main; commit 43fbb58 carries about 250 lines of substantive fixes inside a merge commit; several fix-on-fix commits. Every commit is authored and committed as a personal address (fredespi <...@gmail.com>), five carry a Signed-off-by for a different identity than the author, and AGENTS.md section 2 asks for the employer identity where required. Commit signature status cannot be read here. Suggest squashing into a few logical commits with one identity.

Decisions for the author

• One PR does four things — non-blocking-improvement. The stop gate, Windows start-time reads (a behaviour change for every Windows managed-service stop, per the description), the atomic-write consolidation across rocm-core, therock.rs and rocmd, and unrelated serve-advice rewording total about 5.5k lines. Splitting out the Windows start-time work and the atomic-write refactor would make each reviewable and let the Windows clippy and test results land per change.
• Fail-closed on a tick-less live record versus best-effort stop — tradeoff. Refusing protects an unrelated process after pid reuse across a reboot but turns every older-format or engine-without-ticks record into a manual step (see the vLLM finding); best-effort stopping keeps uninstall working but force-kills a tree on an unproven pid. The PR chose fail-closed; confirm that the manual step is acceptable once vLLM records carry ticks.
• Gate placed in a new uninstall_gate.rs while its tests live in main.rs — tradeoff. Keeps the new module small against the repo's direction of shrinking main.rs, at the cost of the new module not owning its tests.

Positive signals

• The ordering tests assert that planned files still exist after an aborted run rather than checking a return value, and lifecycle-10 polls the planted child with try_wait so a zombie cannot make it pass.
• Start-time identity comparison is read only after the liveness check in identity_state and the new serde field is default/skip_serializing_if, so records stay compatible in both directions.
• The gate probes both loopback families for wildcard binds and fails closed on an unparseable manifest or unreadable runtime state instead of skipping them.

Deployment notes

• Windows managed-service stops start verifying process creation time (GetProcessTimes); a recorded pid that cannot be verified now yields Recycled/Indeterminate where it used to always match.
• rocmd runtime-state files gain daemon_start_ticks; state files written by an older rocmd make rocm uninstall refuse until the helper is stopped by hand.
• crates/rocm-core now owns the windows-sys Win32_Storage_FileSystem feature; apps/rocm and apps/rocmd drop their own windows-sys dependency (Cargo.lock updated).
• Merging needs the base's README.md and docs/architecture.md conflicts resolved.

What this covered

Every file in the change read completely by five independent read-only workers (production gate; added tests; process-identity and daemon layer; e2e scenarios and docs; agent-instruction/history/prior-changes/comments passes), prw-base...HEAD against merge-base 0a021785… and head d1e2aaeb16c653b6cf2c7636d48b9804ead9e78c, plus callers and callees in crates/rocm-core, engines/vllm, engines/lemonade and apps/rocmd. Every blocking item above was re-checked by the driver against the source. Run on this head on Linux: cargo test -p rocm --bin rocm (1070 passed), cargo test -p rocm-core --lib (598 passed), the uninstall filter (40 passed), a throwaway gate test on the vLLM record shape (failed as described), and five single-branch mutations (zombie check, Unverified arm, ConfirmTheServicePid advice, stopped_by_this_pass wording, recycled-daemon kill), all of which left the suite green. Each mutated file was restored with git checkout after its run. Not run: Windows build or clippy (only a Linux target is installed here), workspace clippy, the e2e cucumber suite, scripts/smoke_local.py, and any GPU lane; Windows-only code (GetProcessTimes, ReplaceFileW, lifecycle-24) was reviewed by reading only. CI: only the Read the Docs status has reported on this head. The step 7 design questions were answered by the driver from the workers' evidence rather than by separate per-question workers; the history and prior-changes passes ran (the latter read earlier merged PRs' review comments, not this PR's). The leak scan over the diff is clean (hits are a reserved test hostname, an identifier, a "private key" release-signing line, a bare ticket id and the committers' sign-off lines). Not reconciled against any discussion here.

[fredespi]

#299 (review)

Pushed 0ad0b6b (merged main, docs conflicts resolved). Fixed: vLLM engine ticks (state writes server_start_ticks; core falls back to start_ticks when server_pid==pid, test added); tick-less advice no longer points at services remove, now a manual kill/delete-file route; liveness check uses process_is_running; GetProcessTimes uses &raw mut + SAFETY; Unverified daemon arm and advice text now tested; daemon-ticks capture test runs on Windows too. Not done yet: Gherkin scenarios for keep-binaries/helper stop, the 403/stored-key/stderr tests, zombie test, comment cleanups, README/help text, commit squash.

[rominf]

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

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 (round 15, full review of the whole change). This round files a change request again. It replaces the one at cbb5d1fb as this account's whole position. Every blocking item is tagged new or standing.

• Your two notes (on cbb5d1fb's report and on the other reviewer's d1e2aaeb report) are acknowledged with a reaction. This report covers what they claim.
• Dropped as resolved (checked against the code at 6c489913):
  • The force-kill of live tick-less service pids. The gate now refuses them before stopping anything.
  • The Unverified daemon advice, which now maps to ConfirmTheDaemonPid and is tested.
  • The duplicated advice line. Every class now has its own rank.
  • The rocmd twin of the restart advice, which now names <model>.
• Not gating this change request:
  • The last blind finding (description and comments describe a removed check). The squash body comes from commit messages, and the comments mislead no runtime behaviour.
  • The starts_with arm inside the untested-branches finding, which was non-blocking last round.
• No build or test was run this round. The review host is low on disk, so every test verdict below comes from reading the code. Nothing is scored 100.
• CI at 6c489913: every reported check passed or was skipped (skillscope and manifests skipped).
• Inline comments: none. Every finding is below.

Review — needs work

Full review of the whole change.
Implements EAI-8014 (now ROCMAI-290): stop managed services, then remove files, and abort with recovery advice if a stop can't be confirmed. The ticket's expected behaviour is covered.

Blocking

• Any service that rocmd has recovered makes every later uninstall refuse, and the refusal blames the wrong cause (new) — apps/rocmd/src/service.rs:546-558, apps/rocmd/src/watchers.rs:1452-1464, apps/rocm/src/uninstall_gate.rs:471-511, apps/rocm/src/uninstall_gate.rs:919-924, tests/e2e-cucumber/tests/e2e/lifecycle_steps.rs:~530

  • The path: recovery re-runs rocmd supervise. supervise_service rebuilds the record with ManagedServiceRecord::new(..., std::process::id(), ...), which leaves supervisor_start_ticks: None. Nothing in apps/rocmd sets that field; the only writers are rocm serve (main.rs:4161, main.rs:15720).
  • What the gate does with it: the pre-check sees a live supervisor pid with no recorded start time and aborts with ConfirmTheServicePid.
  • What the user is told: "the pid may have been reused after a reboot", but the pid really is our live supervisor. The doc comment on record_predates_start_ticks ("an absent value means the record was written by a pre-upgrade rocmd") is also false.
  • The advice may not clear it (argued, not run): the refusal comes before the helper is stopped, and the helper recovers managed services. So "kill the pid, re-run" can just produce another record without a start time.
  • Why the scenario misses it: the e2e step writes the start time into the record itself, so the gap never shows.
  • Why it is this PR's: before this change a missing start time only made a stop best-effort. Now it hard-blocks uninstall.
  • Confidence 85 · logic · Fix: record process_start_ticks(std::process::id()) in supervise_service, and refresh it for the recovered child. Add a test that stages a record shaped the way recovery writes it.

• uninstall_never_kills_a_daemon_pid_that_was_recycled cannot detect a kill (standing, re-found blind) — apps/rocm/src/main.rs:33921

  • It checks rocm_core::process_is_running(pid) on its own child, which it never reaps. On Linux that is kill(pid, 0) (crates/rocm-core/src/lib.rs:1611-1623), which returns true for a zombie. So a stranger that gets killed still reads as running.
  • The test never asserts report.helper_stopped, and report.stopped lists only services. A gate that killed on the bare pid would pass this test.
  • The PR description lists this test as "verified against a faithful revert".
  • Confidence 85 · logic · Fix: assert stranger.try_wait() returns None (as uninstall_never_signals_a_daemon_pid_recorded_as_already_stopped does) and assert !report.helper_stopped.

• every_failure_class_carries_its_own_recovery_advice cannot fail on the ids it claims to check, and skips a class (new) — apps/rocm/src/main.rs:34078, apps/rocm/src/main.rs:36355, apps/rocm/src/uninstall_gate.rs:997-1003

  • The abort message always starts with detail = "{service_id} ({reason})" for every failure. So "id-carrying remedies must name their ids" passes even if every ids.join is removed from advice().
  • ConfirmTheServicePid is missing from the "every class" list.
  • Its own test, the_tickless_pid_advice_names_ids…, asserts no id at all.
  • Confidence 80 · logic · Fix: assert the ids on the text after the detail part, and add ConfirmTheServicePid.

• uninstall_refuses_while_a_recorded_endpoint_still_accepts_connections cannot tell a port-probe block from a failed stop (standing, re-found blind) — apps/rocm/src/main.rs (at fn uninstall_refuses_while_a_recorded_endpoint_still_accepts_connections)

  • It calls supervisor.wait() with no time limit. If the stop failed, the wait lasts until sleep 60 exits by itself, and "the recorded process must really have been stopped" still passes.
  • A failed stop also yields failed == ["svc-orphaned-engine"].
  • The test never asserts StopWhatHoldsThePort or the "after the stop" reason. The shared assert_stopped_within_grace helper was added in this PR for exactly this case.
  • Confidence 80 · logic · Fix: use assert_stopped_within_grace, and assert the remedy and the reason.

• The new engine_start_ticks fallback is only tested where it applies; the dangerous direction is untested (new) — crates/rocm-core/src/lib.rs:4357-4364, apps/rocm/src/main.rs:36373

  • The test is named "…distinct_engine_pid…", but its fixture has pid == server_pid (4242 for both).
  • If the == Some(u64::from(pid)) guard were dropped, the test stays green. Then a launcher's start time would be adopted for a different engine pid, so a live engine reads as recycled and is treated as gone.
  • Confidence 80 · logic · Fix: add a fixture with server_pid != pid and assert engine_start_ticks stays None. Rename or fix the existing test.

• Branches this change adds that no test reaches (standing in part, re-found blind: the stderr warnings and the 403 arm; the rest new; the starts_with arm was non-blocking last round and does not gate on its own) — apps/rocm/src/uninstall.rs:~84-86, apps/rocm/src/uninstall_gate.rs:638, main.rs (plan_removes_recovery_tooling), endpoint_refused_authorization

  • The eprintln!("warning: …") loop is the only thing that shows the user each fail-open. Deleting it keeps every test green; the tests check report.warnings as values, never the print.
  • The deferred-partner probe: all three tests that share a port stay green if the call is deleted.
  • The entry.path.starts_with(&services_dir) arm of plan_removes_recovery_tooling.
  • The 403 response (only 401 is staged).
  • Confidence 75 · logic · Fix: inject a writer, or return the warnings so they can be asserted, and stage each missing case.

• The plan promises a stop that the gate then refuses, and the printed stop claims are never checked against the actual outcome (new; the unchecked stop line is standing) — apps/rocm/src/main.rs:~18000-18010 (build_uninstall_plan), apps/rocm/src/uninstall_gate.rs:1083-1096, tests/e2e-cucumber/tests/e2e/lifecycle_steps.rs (then_removal_complete)

  • The plan counts records by status and prints "those servers will be stopped before removal" just before the confirm prompt. A live record with no start time, which includes every recovered service (first finding), is refused instead.
  • the_plan_warning_only_promises_a_stop… only stages records that have a start time.
  • The "stopped N managed service(s)" line is pinned only with fabricated reports. The e2e step checks just "uninstall complete". None of the string pins names the test that proves the behaviour, which AGENTS.md §3 requires.
  • Confidence 75 · logic · Fix: compute the plan line from the same start-time check the gate uses. Assert the stop line in the lifecycle-10 and lifecycle-24 then-steps.

• The PR description and code comments describe a platform check that was removed (new; not gating, see round note) — PR description ("identity_state_with_observed so one reading of the PID answers both the 'could this platform verify?' and 'is this still the same process?' questions"), apps/rocm/src/uninstall_gate.rs:354-371, apps/rocm/src/uninstall_gate.rs:272-285, crates/rocm-core/src/lib.rs (daemon_start_ticks doc)

  • The code now uses record_predates_start_ticks(x) = x.is_none(). Nothing reads our own pid to answer the platform question.
  • The comments still explain that defence ("answered by a process that is definitely alive — this one", "the best-effort Matches arm meant for Windows", "(/proc)").
  • They also say "Two residual gaps" but describe only one.
  • The description's claim that the recycled-pid test was "verified against a faithful revert" is contradicted by the second finding.
  • Confidence 85 · mechanical · Fix: correct the description and rewrite these comments to the current rule (no start time means refuse).

The items below are standing objections from cbb5d1fb that this round's blind review did not re-find as blocking. After its report was frozen, each was put to the reviewer together with your note. Those verdicts are anchored: the reviewer was told what to look for. They are also static. Only the part that still holds is listed.

• No test follows the gate's advice and shows the gate then proceeds (standing; anchored check: holds) — apps/rocm/src/uninstall_gate.rs:109-115, tests at main.rs:34808-34870, :34147

  • RemoveTheStoppedRecord still advises rocm services remove <id> --yes. The tests that reach it stop at the abort.
  • No test removes the record, re-runs stop_managed_services_before_uninstall, and shows that uninstall proceeds. The same gap applies to the delete-the-file routes. lifecycle-25 asserts only the refusal.
  • AGENTS.md §3: remediation advice naming a command must be shown to clear the condition.

• The stored endpoint key passed to the identity probe is untested (standing; anchored check: holds) — apps/rocm/src/uninstall_gate.rs:695, :708, :717

  • The only authorization test (main.rs:34838) asserts that no key is stored.
  • If both .as_deref() arguments become None, our own keyed survivor gets RemoveTheStoppedRecord ("not one of yours, delete the record") instead of StopWhatHoldsThePort, and nothing fails.

• Only a 401 is staged, so widening the refusal check cannot fail (standing; anchored check: holds) — uninstall_gate.rs:949-951, main.rs:34379

  • With >= 300, a 404 or 500 from an unrelated server on an old port would falsely abort, and no test would fail.

• No Gherkin scenario covers the --keep-binaries --keep-data run with a live server, or uninstall stopping rocmd (standing; the blind review raised it under Decisions; anchored check: holds) — tests/e2e-cucumber/features/install_lifecycle.feature

  • The feature adds only lifecycle-10, 24 and 25.
  • The PR body says "unit tests only" but gives no reason and names no lane. AGENTS.md §3 requires one.

• README and --help do not say uninstall stops rocmd, or what an aborted run leaves down (standing; the blind review raised it as non-blocking; anchored check: holds) — README.md, apps/rocm/src/main.rs:611-615

  • An aborted run leaves servers and the helper down, and drops their endpoint keys.
  • AGENTS.md §5.

• The manual-testing refusal check cannot be followed as written (standing; anchored check: holds) — docs/manual-testing.md §8

  • The check runs again right after a full uninstall --yes that exits 0, with no reinstall in between.
  • It gives only the relative services/bad.json.

• The core atomic writer's failure branches are untested, and two staging copies remain (standing; anchored check: partly holds) — crates/rocm-core/src/lib.rs:4832, :4839, apps/rocmd/src/sandbox.rs:676-763, apps/rocm/src/therock.rs:4636-4645

  • The publish-failure cleanup is now tested (lib.rs:5165).
  • Still untested: the write/sync-failure cleanup, the AlreadyExists retry and the exhausted bail!.

• Fail-closed branches no test exercises (standing; anchored check: holds for the non-.json filter; the rest marked unverified this round)

  • Non-.json filter: only svc-corrupt.json is ever planted, so dropping the filter would turn any non-JSON file under services/ into a false "unreadable manifest" abort.
  • The checker could not tell, or did not verify, the other three: kill scope for the daemon stop, publish order, and host_port_resolves against a resolvable-but-dead endpoint.

Non-blocking

• The description still does not mention the reworded rocm services restart advice (standing, not gating; anchored check: partly holds) — apps/rocm/src/main.rs:3825. The rocmd twin is now fixed.
• Other stale or broken comments — confidence 85 · mechanical
  • Start times described as Linux-only, although this PR adds Windows: apps/rocm/src/uninstall.rs:394, apps/rocm/src/uninstall.rs:471, apps/rocm/src/main.rs:33686, the ProcessIdentity doc ("clock ticks since boot"; on Windows the value is a FILETIME), and the apps/rocmd/src/service.rs test expect message.
  • probe_hosts's doc comment sits above hosts_name_one_listener, so rustdoc attaches it there: apps/rocm/src/uninstall_gate.rs:800-827.
  • A garbled, duplicated sentence that says there is "no services remove", although that command exists and the gate's own advice names it: apps/rocm/src/main.rs:34255-34256.
  • "pinned by nothing", although the same test now asserts it: main.rs:34654.
  • probe_host should be probe_hosts: main.rs:34481, main.rs:34488.
  • A detached comment fragment: uninstall_gate.rs:518-523.
  • uninstall.rs:5-11 and docs/architecture.md still say "mechanical relocation… no behavior change / no owned types".
• The daemon stop reads the start time before checking liveness — uninstall_gate.rs:372-377. This is the order identity_state_with_observed's own doc warns can return Matches for a recycled pid. terminate_verified re-checks before signalling, so the worst case is a false "stopped the background helper" line. Confidence 75 · logic · Fix: call identity_state(&identity).
• The "shares a live endpoint" overlap check misses a wildcard listener reached on a specific IP — uninstall_gate.rs:535-559, uninstall_gate.rs:819-825. A live record on 0.0.0.0:P plus a stopped record on 192.168.x.y:P are not seen as the same listener. Our own live server then answers the stopped record's probe, which produces a false abort with "Every process recorded … is gone" advice. Confidence 70 · logic.
• The abort's re-serve advice is incomplete — uninstall_gate.rs:1012-1017.
  • It covers only publicly bound services. A loopback service started with --require-api-key also loses its key.
  • "stopped before the failure" also lists services stopped after it, because the loop does not break on a failure.
  • Confidence 70 · logic.
• The e2e harness imports production code to build its own test data — tests/e2e-cucumber/tests/e2e/lifecycle_steps.rs calls rocm_core::process_start_ticks. tests/e2e-cucumber/README.md says "Black-box only… No imports from the rocm-cli codebase", and the rocm-core dev-dependency comment justifies it for FileLock alone. Because the oracle is the same function the CLI uses, a units mismatch would pass. Confidence 75 · architectural.
• The pid-dedup logic exists twice — uninstall_gate.rs:480-493 and main.rs:15475-15490 (terminate_recorded_service_pids). The safety check can drift from the list that actually gets killed. Confidence 70 · architectural.
• Some tests are Linux-only without needing processes — main.rs:34922, main.rs:35050, main.rs:35090. They use an injected stop, but the cfg(target_os = "linux") gate means the required Windows lane never runs the partner logic. Confidence 70 · mechanical.
• User-facing docs omit two behaviours — README.md, the Uninstall doc comment (main.rs:612-616) and docs/manual-testing.md don't say that uninstall now force-stops the rocmd helper. They also don't say that live services with no start time (e.g. started on Windows by an earlier version) make uninstall refuse. Confidence 75 · mechanical.
• About 2,400 lines of gate tests went into main.rs — uninstall_gate.rs has no test module, and uninstall_gate.rs:719-725 points at main.rs. That runs against AGENTS.md §6 ("not growth inside main.rs"). Confidence 70 · architectural.

Decisions for the author

• Refusing on any live pid without a start time — tradeoff. It never force-kills an unproven process tree. The cost is that every pre-upgrade Windows record, and every recovered service until the first finding is fixed, needs manual cleanup. Confirm that this friction is intended once the recovery path records start times.
• Scope and size — non-blocking-improvement. About 5.5k lines, including the atomic-write consolidation, the write_active_runtime_marker change and the windows-sys drops, which are unrelated to uninstall. The description does disclose them. Splitting them out would make the gate itself reviewable (AGENTS.md §11).
• Hand-rolled progress and summary output — non-blocking-improvement. println!("stopping managed service …") covers a wait the code itself says "reads as a hang". AGENTS.md §6 points to cli_progress::AnimatedSpinner / cli_report::ActionReport. Existing uninstall output is hand-rolled too.
• No scenario for the --keep-binaries --keep-data exemption or the helper stop — non-blocking-improvement. The description discloses this but gives no reason; AGENTS.md §3 asks for the reason when a scenario is left out.

Positive signals

• The stop pass decides everything that could doom the run first (unparseable manifests, pids without a start time, already-stopped probes) and only then stops anything, so an aborted run has stopped nothing.
• StopFailureRemedy::advice/advice_rank and StoppedRecordVerdict::blocks are exhaustive matches, so a new failure class cannot silently default to "proceed".
• Fail-open disclosures are values on the report rather than bare prints, which makes them assertable.
• Wildcard binds are probed on both loopback families, and "did not resolve" (host_port_resolves) is kept separate from "nothing listening".
• stop_managed_services_then_remove takes the stop as a parameter, so the stop-before-remove ordering is pinned by tests that check files still exist after an abort.
• daemon_start_ticks is #[serde(default)], so old state files still load.

Deployment notes

• On Windows, live managed services started by an earlier rocm have no start time recorded. After upgrading, rocm uninstall refuses until they are stopped by hand or their records are deleted.
• Windows managed-service stops now check pid identity (GetProcessTimes), so they can now return Recycled/Indeterminate where they previously always matched.
• AutomationRuntimeState gains an optional daemon_start_ticks field, which is backward-compatible on read. ManagedServiceRecord and AutomationRuntimeState now publish through ReplaceFileW on Windows.

What this covered

Every file in the change, read completely, plus the callers and callees of the changed symbols: rocmd supervise/recovery, rocm-core identity and termination, and the clap definitions. The diff was 7a062ad…6c489913 (merge-base with prw-base fe47c48 … PR head), checked across four independent workers: gate core, rocm-core/rocmd start-time side, unit tests, and e2e/docs/rules/history/prior-PR comments. CI rollup for 6c48991: every reported check passed or was skipped.

Did not run:

• No build or test was run. The host disk is under 40 GB free, so the review was static only. Every test verdict above was reached by reading, not by mutation runs, and nothing is scored 100.
• The design-review step ran in the driver, not as independent workers.
• The Windows ReplaceFileW error-1176 cleanup path was looked at but could not be established.
• Ticket: EAI-8014 was retrieved by its ID (it resolves to ROCMAI-290); its comments were not read.

Not reconciled against any discussion on the PR.

The driver added these parts:

• the round note;
• the new/standing tags and gating notes;
• the standing items after the italic line in Blocking;
• the first Non-blocking item.

The anchored checks were given the objection and the author's note. They are not part of what this review read blind. Most were asked one at a time; the last nine were asked together in one message, each to be judged separately.