diff --git a/Cargo.lock b/Cargo.lock index 549cde179..22aa685a1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3661,7 +3661,6 @@ dependencies = [ "unicode-width", "ureq", "windows-native-keyring-store", - "windows-sys 0.61.2", "zbus-secret-service-keyring-store", ] @@ -3830,7 +3829,6 @@ dependencies = [ "sha2", "tokio", "ureq", - "windows-sys 0.61.2", ] [[package]] diff --git a/README.md b/README.md index f7864b4fa..37bf2c2d1 100644 --- a/README.md +++ b/README.md @@ -757,6 +757,13 @@ rocm uninstall [--yes] [--dry-run] [--keep-binaries] [--keep-config] [--keep-data] [--keep-cache] ``` +`rocm uninstall` stops any managed model server that is still running before it +removes anything. If a server cannot be stopped (or its service record cannot +be read), the command exits non-zero, leaves every file in place, and says what +to repair or stop by hand, so the tooling needed to stop it is never deleted +out from under a running server. A run that keeps the binaries and data +(`--keep-binaries --keep-data`) leaves running servers alone. + ### Shell completions `rocm completions ` prints a completion script for the given shell to diff --git a/apps/rocm/Cargo.toml b/apps/rocm/Cargo.toml index 3d4990869..6c873d840 100644 --- a/apps/rocm/Cargo.toml +++ b/apps/rocm/Cargo.toml @@ -51,7 +51,6 @@ libc.workspace = true [target.'cfg(windows)'.dependencies] windows-native-keyring-store = "1.1" -windows-sys = { version = "0.61", features = ["Win32_Storage_FileSystem"] } [target.'cfg(target_os = "macos")'.dependencies] apple-native-keyring-store = { version = "1.0", features = ["keychain"] } diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 09d49be4f..04427294c 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -19,6 +19,7 @@ mod serve_summary; mod storage; mod therock; mod uninstall; +mod uninstall_gate; // Per-command handler fns mechanically relocated into modules. // Dispatch call sites stay byte-identical via these re-imports (upstream-sync @@ -543,6 +544,11 @@ rocm logs --search error timeout")] command: BenchCommand, }, /// Remove ROCm CLI-managed files from this computer. + /// + /// Managed model servers that are still running are stopped first. If one + /// cannot be stopped, nothing is removed, the command exits non-zero, and + /// it names what to repair or stop by hand. A run that keeps the binaries + /// and data (`--keep-binaries --keep-data`) leaves running servers alone. Uninstall { /// Do not ask for interactive confirmation. #[arg(long)] @@ -11907,27 +11913,21 @@ fn active_runtime_marker_path(paths: &AppPaths) -> PathBuf { fn write_active_runtime_marker(paths: &AppPaths, marker: ActiveRuntimeMarker) -> Result<()> { let path = active_runtime_marker_path(paths); - fs::create_dir_all( - path.parent() - .context("active runtime marker path has no parent directory")?, - )?; - let tmp_path = path.with_extension(format!("json.tmp-{}", rocm_core::unix_time_millis())); - fs::write( - &tmp_path, - serde_json::to_vec_pretty(&marker).context("failed to serialize active runtime marker")?, - ) - .with_context(|| format!("failed to write {}", tmp_path.display()))?; - if path.exists() { - let _ = fs::remove_file(&path); - } - fs::rename(&tmp_path, &path).with_context(|| { - format!( - "failed to move active runtime marker {} into {}", - tmp_path.display(), - path.display() - ) - })?; - Ok(()) + // No `create_dir_all` here: `write_file_atomically` creates the parent + // itself, and it has to — it stages a scratch sibling in that directory + // before publishing. + // + // The shared helper, not a local delete-then-rename. Removing the target + // first opens a window in which the marker simply does not exist — a reader + // in it concludes no runtime is active — and the old scratch name carried + // only a millisecond stamp, so two writers in the same millisecond picked + // the same file. `write_file_atomically` reserves its scratch name with + // `create_new` and publishes over the target in one step (`ReplaceFileW` on + // Windows), so neither window exists. + let bytes = + serde_json::to_vec_pretty(&marker).context("failed to serialize active runtime marker")?; + rocm_core::write_file_atomically(&path, &bytes) + .with_context(|| format!("failed to write {}", path.display())) } fn config(command: ConfigCommand) -> Result<()> { @@ -15969,6 +15969,18 @@ fn run_chat_port_status_tool( })) } +/// Whether `host` resolves to any address at all. +/// +/// Split out from [`loopback_tcp_port_is_reachable`], which answers `false` both +/// for "resolved, nothing listening" and for "did not resolve". Those mean +/// opposite things to the uninstall gate, and only the caller that has to +/// distinguish them pays for the second lookup. +fn host_port_resolves(host: &str, port: u16) -> bool { + (host, port) + .to_socket_addrs() + .is_ok_and(|mut addresses| addresses.next().is_some()) +} + fn loopback_tcp_port_is_reachable(host: &str, port: u16) -> bool { let Ok(addresses) = (host, port).to_socket_addrs() else { return false; @@ -20563,14 +20575,20 @@ fn build_uninstall_plan(paths: &AppPaths, options: &UninstallOptions) -> Result< plan.warnings.push(note); } - let managed_services = load_managed_services(paths).unwrap_or_default(); - if !managed_services.is_empty() { - plan.warnings.push(format!( - "{} managed service record(s) exist under {}; background processes are not stopped automatically in this pass", - managed_services.len(), - paths.services_dir().display() - )); - } + // The gate propagates this error while the plan does not, so a plan that + // silently read zero records would tell the operator "no managed services" + // about a run that is going to abort on exactly that failure. Keep the plan + // non-fatal (it is also the read-only dry-run path) but say what happened. + let managed_services = match load_managed_services(paths) { + Ok(records) => records, + Err(error) => { + plan.warnings.push(format!( + "could not read the managed service records under {}: {error:#}. Uninstall will refuse to remove anything until they can be read", + paths.services_dir().display() + )); + Vec::new() + } + }; // Remote sessions are worse than local ones to drop silently. The model runs // on someone else's machine and its endpoint is published there, so removing @@ -20614,9 +20632,62 @@ fn build_uninstall_plan(paths: &AppPaths, options: &UninstallOptions) -> Result< plan.actions .sort_by(|left, right| left.path.cmp(&right.path)); plan.actions.dedup_by(|left, right| left.path == right.path); + + if !managed_services.is_empty() { + // Two independent conditions decide whether a server actually gets + // stopped, and the warning must reflect BOTH or it describes work + // uninstall never does: the record has to be live, and the plan has to + // be removing the tooling that stops it (`uninstall()` skips the whole + // stop pass otherwise). This runs after the actions are final, so the + // predicate sees the plan the operator is about to confirm. + let live = managed_services + .iter() + .filter(|record| managed_service_is_live(record)) + .count(); + let services_dir = paths.services_dir().display().to_string(); + let total = managed_services.len(); + plan.warnings.push(if live == 0 { + // "none is recorded as running", not "none has a running server": + // this counts statuses, and a record reads "stopped" the moment its + // supervisor is killed even if an engine grandchild kept the port. + // The stop pass probes for exactly that, so the plan must not + // promise it away — this line is the last thing read before + // confirming. + format!("{total} managed service record(s) exist under {services_dir}; none is recorded as running") + } else if plan_removes_recovery_tooling(&plan, paths) { + format!( + "{live} of {total} managed service record(s) under {services_dir} are recorded as running; those servers will be stopped before removal" + ) + } else { + // Cache-only and other tooling-preserving runs: say what is true — + // the servers keep running, and they remain stoppable afterwards. + format!( + "{live} of {total} managed service record(s) under {services_dir} are recorded as running; this removal keeps `rocm services stop` and the service records, so those servers are left running" + ) + }); + } + Ok(plan) } +/// Whether this plan removes what an operator would need to stop a managed +/// server afterwards — the `rocm`/`rocmd` binaries, or the service records under +/// the data directory. +/// +/// The stop-before-remove gate exists only for that case. A `--keep-binaries +/// --keep-data` run removes the cache alone: the binaries and every service +/// record survive, so `rocm services stop` still works and force-stopping live +/// GPU servers (or hard-aborting a cache cleanup because one will not stop) +/// would be pure collateral damage. +fn plan_removes_recovery_tooling(plan: &UninstallPlan, paths: &AppPaths) -> bool { + let services_dir = paths.services_dir(); + plan.actions.iter().any(|entry| { + entry.kind == "binary" + || entry.path.starts_with(&services_dir) + || services_dir.starts_with(&entry.path) + }) +} + pub(crate) fn render_uninstall_dry_run(paths: &AppPaths) -> Result { let options = UninstallOptions { dry_run: true, @@ -22399,6 +22470,7 @@ mod tests { } use super::*; + use crate::uninstall_gate::*; use serde_json::json; /// Serializes and isolates tests that read or mutate process-global env vars. @@ -36362,6 +36434,7 @@ ID_LIKE="suse opensuse" running: true, automations_enabled: true, daemon_pid: 123, + daemon_start_ticks: None, started_at_unix_ms: 1, last_tick_unix_ms: 2, local_webhook_endpoint: Some("http://127.0.0.1:19191/automation-events".to_owned()), @@ -37524,6 +37597,41 @@ ID_LIKE="suse opensuse" } } + /// Assert `child` exits within a bounded grace, and reap it either way. + /// + /// NOT `wait()`. These tests spawn a stand-in that exits on its own after a + /// minute, so an unbounded `wait()` blocks until that happens and then finds + /// the process gone — passing whether or not anything killed it. The only + /// symptom of a total regression would be the suite taking a minute. Polling + /// against a deadline asserts it died *now*, which is the actual claim. + /// + /// The grace exceeds the stop path's own per-process wait but stays far + /// below the stand-in's lifetime, or this goes back to measuring nothing. + /// + /// Shared with `crate::uninstall`'s tests rather than duplicated there: two + /// near-identical copies of this, with near-identical comments, is what made + /// it easy to believe both process-based tests were bounded when only one + /// of them was. + #[cfg(unix)] + pub(crate) const STOP_ASSERTION_GRACE: Duration = Duration::from_secs(30); + + #[cfg(unix)] + pub(crate) fn assert_stopped_within_grace(child: &mut std::process::Child, message: &str) { + let deadline = std::time::Instant::now() + STOP_ASSERTION_GRACE; + let exited = loop { + match child.try_wait().expect("poll the stand-in") { + Some(_) => break true, + None if std::time::Instant::now() >= deadline => break false, + None => thread::sleep(Duration::from_millis(50)), + } + }; + if !exited { + let _ = child.kill(); + let _ = child.wait(); + } + assert!(exited, "{message}"); + } + #[cfg(unix)] fn managed_record_for_pid( paths: &AppPaths, @@ -37537,7 +37645,8 @@ ID_LIKE="suse opensuse" "m", "m", "127.0.0.1", - 9, + // Port 0 is never served, so the gate's probe cannot meet a listener. + 0, "managed", pid, None, @@ -37630,10 +37739,10 @@ ID_LIKE="suse opensuse" assert_eq!(signaled, vec![pid]); assert!(all_stopped); - // Reap our own child (a detached managed process would be reaped by init) - // so the liveness check does not observe a not-yet-reaped zombie. + // Bounded, and our own child, so this pins that the terminate killed it + // rather than that it outlived the test. let mut child = child; - let _ = child.wait(); + assert_stopped_within_grace(&mut child, "the verified process must be terminated"); assert!( !rocm_core::process_is_running(pid), "the verified process must be terminated" @@ -37641,6 +37750,1631 @@ ID_LIKE="suse opensuse" let _ = fs::remove_dir_all(root); } + // Linux-only, not merely unix: these rely on `process_start_ticks` (Some on + // Linux, None elsewhere) and zombie-state detection in `terminate_verified`. + #[cfg(target_os = "linux")] + #[test] + fn uninstall_stops_live_managed_service_and_reports_it() { + // EAI-8014: a live managed server must be stopped before uninstall + // removes the tooling that stops it, and reported so the operator knows. + let child = std::process::Command::new("sleep") + .arg("60") + .spawn() + .expect("spawn managed server"); + let pid = child.id(); + let (root, paths) = test_paths("uninstall-stops-live"); + let real = rocm_core::process_start_ticks(pid).expect("start-ticks"); + let mut record = managed_record_for_pid(&paths, pid, Some(real)); + record.status = "ready".to_owned(); + record.write().expect("write service record"); + + let report = + stop_managed_services_before_uninstall(&paths).expect("stop pass should succeed"); + + assert!(report.failed.is_empty(), "nothing should fail: {report:?}"); + assert_eq!(report.stopped, vec![record.service_id]); + let mut child = child; + assert_stopped_within_grace( + &mut child, + "the managed server must be stopped before uninstall proceeds", + ); + assert!( + !rocm_core::process_is_running(pid), + "the managed server must be stopped before uninstall proceeds" + ); + let _ = fs::remove_dir_all(root); + } + + #[cfg(target_os = "linux")] + #[test] + fn uninstall_skips_already_dead_managed_service() { + // A service whose process already crashed is not live; it must neither be + // counted as stopped by us nor fail the abort gate that keeps the tooling. + // + // A live service shares the directory so the assertion cannot be + // satisfied by a gate that simply does nothing: the dead one must be + // skipped WHILE the live one is stopped. + let mut dead = std::process::Command::new("sleep") + .arg("60") + .spawn() + .expect("spawn"); + let dead_pid = dead.id(); + let _ = dead.kill(); + let _ = dead.wait(); + assert!( + !rocm_core::process_is_running(dead_pid), + "the process must be gone before the record is loaded" + ); + let (root, paths) = test_paths("uninstall-skips-dead"); + let mut crashed = managed_record_for_pid(&paths, dead_pid, None); + crashed.status = "ready".to_owned(); + crashed.write().expect("write service record"); + + let live = std::process::Command::new("sleep") + .arg("60") + .spawn() + .expect("spawn live managed server"); + let live_pid = live.id(); + let mut running = managed_record_for_pid(&paths, live_pid, None); + running.service_id = "svc-managed-live".to_owned(); + running.manifest_path = paths.service_manifest_path(&running.service_id); + running.supervisor_start_ticks = rocm_core::process_start_ticks(live_pid); + running.status = "ready".to_owned(); + running.write().expect("write live service record"); + + let report = + stop_managed_services_before_uninstall(&paths).expect("stop pass should succeed"); + + assert!( + report.failed.is_empty(), + "a crashed service must not abort uninstall: {report:?}" + ); + assert_eq!( + report.stopped, + vec!["svc-managed-live".to_owned()], + "only the live service is stopped and reported: {report:?}" + ); + let mut live = live; + let _ = live.wait(); + let _ = fs::remove_dir_all(root); + } + + #[cfg(target_os = "linux")] + #[test] + fn uninstall_stops_a_live_background_helper() { + // This pins that the daemon stop still happens at all — the + // feature-removal guard. It does NOT pin identity verification: an + // unrecorded start-time also classifies as `Matches`, so this passes + // against the pre-fix `ProcessIdentity::new(pid, None)` too. The + // verification itself is pinned by + // `uninstall_never_kills_a_daemon_pid_that_was_recycled` and + // `uninstall_never_kills_a_daemon_pid_from_a_state_file_that_predates_start_ticks`, + // both of which fail if the identity check is dropped. + // + // Why the daemon goes first: it restarts a managed service whose + // endpoint stops answering, which is exactly the state the stop pass + // creates before writing the record back. + let child = std::process::Command::new("sleep") + .arg("60") + .spawn() + .expect("spawn daemon stand-in"); + let pid = child.id(); + let (root, paths) = test_paths("uninstall-stops-verified-daemon"); + let mut state = runtime_state(true, pid); + state.daemon_start_ticks = rocm_core::process_start_ticks(pid); + assert!( + state.daemon_start_ticks.is_some(), + "the recorded identity is the point of this test" + ); + state.write(&paths).expect("write runtime state"); + + let report = + stop_managed_services_before_uninstall(&paths).expect("stop pass should succeed"); + + assert!(report.failed.is_empty(), "nothing should fail: {report:?}"); + assert_eq!( + report.stopped, + vec!["rocmd (background helper)".to_owned()], + "a verified daemon is stopped and reported: {report:?}" + ); + let mut child = child; + assert_stopped_within_grace( + &mut child, + "the background helper must be stopped before uninstall proceeds", + ); + assert!( + !rocm_core::process_is_running(pid), + "the background helper must be stopped before uninstall proceeds" + ); + let _ = fs::remove_dir_all(root); + } + + #[cfg(target_os = "linux")] + #[test] + fn uninstall_never_kills_a_daemon_pid_from_a_state_file_that_predates_start_ticks() { + // The ordinary upgrade path: `runtime-state.json` written by a pre-upgrade + // `rocmd` carries no `daemon_start_ticks`. `identity_state` calls that + // `Matches` (its legacy best-effort arm), so signalling on that verdict + // would force-kill a whole tree at a pid nothing has verified. On a + // platform that CAN read start-times, an unrecorded one means the record + // is stale, not that the pid is ours. + let stranger = std::process::Command::new("sleep") + .arg("60") + .spawn() + .expect("spawn unrelated process"); + let pid = stranger.id(); + let (root, paths) = test_paths("uninstall-daemon-legacy-record"); + let state = runtime_state(true, pid); + assert!( + state.daemon_start_ticks.is_none(), + "this test is about the unrecorded-identity path" + ); + state.write(&paths).expect("write runtime state"); + + let report = + stop_managed_services_before_uninstall(&paths).expect("stop pass should succeed"); + + assert!( + rocm_core::process_is_running(pid), + "an unverifiable pid must never be signalled: uninstall killed an unrelated process" + ); + assert!( + report.stopped.is_empty(), + "nothing was confirmed stopped: {report:?}" + ); + assert!( + report + .failed + .iter() + .any(|failure| failure.remedy == StopFailureRemedy::StopTheDaemon + && failure.service_id.contains(&pid.to_string())), + "an unverifiable helper must abort the uninstall and name its pid: {report:?}" + ); + let mut stranger = stranger; + let _ = stranger.kill(); + let _ = stranger.wait(); + let _ = fs::remove_dir_all(root); + } + + #[test] + fn uninstall_refuses_when_the_daemon_runtime_state_cannot_be_read() { + // `load` returns `Err` for a corrupt or unreadable state file and + // `Ok(None)` only when there is none. Discarding that `Err` would skip the + // daemon stop silently and remove the tooling while a live `rocmd` can + // still respawn what the service pass just stopped. Reachable in practice: + // the file is rewritten on every tick. + let (root, paths) = test_paths("uninstall-daemon-state-corrupt"); + paths.ensure().expect("create the app directories"); + fs::write(paths.automation_state_path(), b"{ not json") + .expect("seed a corrupt runtime state"); + + let report = + stop_managed_services_before_uninstall(&paths).expect("stop pass should succeed"); + + assert!( + report + .failed + .iter() + .any(|failure| failure.remedy == StopFailureRemedy::RepairTheDaemonState), + "an unreadable runtime state must abort the uninstall: {report:?}" + ); + let error = uninstall_removal_gate(&report) + .expect_err("the gate must refuse to remove anything") + .to_string(); + // The abort has to be followable. No pid was ever parsed out of this + // file, so "kill that pid" would be a dead end — uninstall has no + // `--force`, and repairing or deleting the named file is the only way + // out. + let state_path = paths.automation_state_path(); + assert!( + error.contains(&state_path.display().to_string()), + "the abort must name the file to repair or delete: {error}" + ); + assert!( + error.contains("repair or delete"), + "the abort must say what to do with it: {error}" + ); + assert!( + !error.contains("kill that pid"), + "no pid was ever read from this file, so that advice cannot be followed: {error}" + ); + let _ = fs::remove_dir_all(root); + } + + #[cfg(target_os = "linux")] + #[test] + fn uninstall_never_kills_a_daemon_pid_that_was_recycled() { + // `runtime-state.json` survives a crash, OOM-kill or reboot with + // `running` still true, so the recorded pid can belong to a stranger by + // the time uninstall runs. Killing on a bare pid would make uninstall + // destroy something the user never installed — with `KillScope::Tree` + // and `force`, an unrelated process AND all its children. + // + // The stand-in plays the recycled process: a real live pid recorded with + // a start-time that is not its own. It must come through untouched. + let stranger = std::process::Command::new("sleep") + .arg("60") + .spawn() + .expect("spawn unrelated process"); + let pid = stranger.id(); + let real = rocm_core::process_start_ticks(pid).expect("start-ticks"); + let (root, paths) = test_paths("uninstall-recycled-daemon-pid"); + let mut state = runtime_state(true, pid); + // The daemon that recorded this pid started at a different time; this pid + // has since been reissued to `stranger`. + state.daemon_start_ticks = Some(real.wrapping_add(1)); + state.write(&paths).expect("write runtime state"); + + let report = + stop_managed_services_before_uninstall(&paths).expect("stop pass should succeed"); + + assert!( + rocm_core::process_is_running(pid), + "a recycled pid must never be signalled: uninstall killed an unrelated process" + ); + assert!( + report.stopped.is_empty(), + "nothing of ours was running, so nothing was stopped: {report:?}" + ); + assert!( + report.failed.is_empty(), + "a recycled pid means the daemon is already gone, not that it is stuck: {report:?}" + ); + let mut stranger = stranger; + let _ = stranger.kill(); + let _ = stranger.wait(); + let _ = fs::remove_dir_all(root); + } + + #[test] + fn uninstall_refuses_when_a_service_record_cannot_be_parsed() { + // `load_managed_services` skips a manifest it cannot parse, which is + // right for listing and wrong here: the unreadable record may describe a + // live, GPU-holding server, so uninstall must not proceed blind to it. + let (root, paths) = test_paths("uninstall-corrupt-record"); + let services_dir = paths.services_dir(); + fs::create_dir_all(&services_dir).expect("create services dir"); + fs::write(services_dir.join("svc-corrupt.json"), b"{ not json") + .expect("write corrupt service record"); + + let report = + stop_managed_services_before_uninstall(&paths).expect("stop pass should succeed"); + + let failed: Vec<&str> = report + .failed + .iter() + .map(|failure| failure.service_id.as_str()) + .collect(); + let corrupt = services_dir.join("svc-corrupt.json"); + assert_eq!( + failed, + vec![corrupt.display().to_string().as_str()], + "an unparseable record must fail the gate: {report:?}" + ); + let error = uninstall_removal_gate(&report) + .expect_err("an unparseable record must abort uninstall") + .to_string(); + // The abort has to be escapable. `rocm services stop` loads the same + // file and fails the same way, so prescribing it here would make every + // retry abort identically — a permanent block with no way out. The only + // remedy is on disk, so the message must name the file and say so. + assert!( + error.contains(&corrupt.display().to_string()), + "names the file to act on: {error}" + ); + assert!( + error.contains("repair or delete the file"), + "states the remedy that can actually clear it: {error}" + ); + assert!( + !error.contains("rocm services stop"), + "must not prescribe a command that fails on the same unparseable file: {error}" + ); + let _ = fs::remove_dir_all(root); + } + + #[test] + fn uninstall_removal_gate_aborts_when_a_service_cannot_be_stopped() { + // The core safety guarantee: a service that could not be confirmed stopped + // aborts uninstall (Err, so the removal loop never runs — nothing removed), + // and the message names the offending service and points at the recovery + // path. Guards against a regression that would delete the tooling while a + // GPU-holding endpoint keeps serving (EAI-8014). + let report = ManagedServiceStopReport { + stopped: vec!["svc-stopped".to_owned()], + failed: vec![FailedManagedServiceStop { + service_id: "svc-stuck".to_owned(), + reason: "still \"ready\" after the stop attempt".to_owned(), + remedy: StopFailureRemedy::StopTheService, + }], + warnings: Vec::new(), + }; + let error = uninstall_removal_gate(&report) + .expect_err("a non-empty `failed` must abort uninstall") + .to_string(); + assert!( + error.contains("svc-stuck"), + "names the stuck service: {error}" + ); + assert!( + error.contains("still \"ready\" after the stop attempt"), + "says why the stop could not be confirmed: {error}" + ); + assert!( + error.contains("rocm services stop"), + "points at the recovery path: {error}" + ); + assert!( + error.contains("No files were removed"), + "states nothing was deleted: {error}" + ); + } + + #[test] + fn uninstall_abort_says_which_services_it_already_stopped() { + // The stop pass is not atomic: services stopped before the failing one + // stay stopped and have lost their endpoint keys. An abort that only + // said "No files were removed" would read as "nothing happened". + let report = ManagedServiceStopReport { + stopped: vec!["svc-public".to_owned()], + failed: vec![FailedManagedServiceStop { + service_id: "svc-stuck".to_owned(), + reason: "still \"ready\" after the stop attempt".to_owned(), + remedy: StopFailureRemedy::StopTheService, + }], + warnings: Vec::new(), + }; + let error = uninstall_removal_gate(&report) + .expect_err("a non-empty `failed` must abort uninstall") + .to_string(); + assert!( + error.contains("svc-public"), + "names what it already stopped: {error}" + ); + assert!( + error.contains("--allow-public-bind"), + "says how a public service comes back after losing its key: {error}" + ); + } + + #[test] + fn every_failure_class_carries_its_own_recovery_advice() { + // This gate refuses to remove anything while a stop is unconfirmed, so + // the advice it prints is the operator's only way out; a class that + // reaches the abort with no advice turns the refusal into a dead end. + // `StopFailureRemedy::advice` and `advice_rank` are exhaustive matches, + // so a sixth variant cannot compile without being given text and a + // place in the order — but the compiler cannot check that the text is + // the *right* text, or that the three id-carrying classes still name + // their ids. That is what this asserts, across all five at once. + // Listed in REVERSE `advice_rank` order, and that is load-bearing + // rather than arbitrary. Listing them in rank order makes insertion + // order and rank order coincide, so the ordering assertion below holds + // whether or not the code sorts at all — deleting the sort left the + // whole suite green. Reversed, the assertion can only pass because + // `advice_rank` put them back. + let report = ManagedServiceStopReport { + stopped: Vec::new(), + failed: vec![ + FailedManagedServiceStop { + service_id: "svc-corrupt.json".to_owned(), + reason: "does not parse".to_owned(), + remedy: StopFailureRemedy::RepairTheRecord, + }, + FailedManagedServiceStop { + service_id: "runtime.json".to_owned(), + reason: "does not parse".to_owned(), + remedy: StopFailureRemedy::RepairTheDaemonState, + }, + FailedManagedServiceStop { + service_id: "rocmd (pid 4321)".to_owned(), + reason: "identity unverified".to_owned(), + remedy: StopFailureRemedy::StopTheDaemon, + }, + FailedManagedServiceStop { + service_id: "svc-orphaned".to_owned(), + reason: "endpoint still answers".to_owned(), + remedy: StopFailureRemedy::StopWhatHoldsThePort, + }, + FailedManagedServiceStop { + service_id: "svc-wedged".to_owned(), + reason: "still ready".to_owned(), + remedy: StopFailureRemedy::StopTheService, + }, + ], + warnings: Vec::new(), + }; + + let error = uninstall_removal_gate(&report) + .expect_err("a non-empty `failed` must abort uninstall") + .to_string(); + + // One distinctive phrase per class, none of them supplied by this test. + for expected in [ + "rocm services stop --yes", + "Find what holds that port", + "restarts managed services on its own", + "cannot tell whether the helper is running", + "No `rocm` command can act on an unparseable record", + ] { + assert!( + error.contains(expected), + "every failure class must carry its own remedy; missing {expected:?} in: {error}" + ); + } + // The three classes whose advice is useless without the ids must name + // them — "find what holds that port" for an unnamed service is not a + // way out. The other two are general instructions and name nothing. + for expected in ["svc-orphaned", "runtime.json", "svc-corrupt.json"] { + assert!( + error.contains(expected), + "id-carrying remedies must name their ids; missing {expected:?} in: {error}" + ); + } + // Most actionable first, hand-repair last: a dead-end-avoidance + // ordering, not cosmetics. + let position = |needle: &str| error.find(needle).expect("asserted present above"); + assert!( + position("rocm services stop --yes") < position("Find what holds that port") + && position("Find what holds that port") + < position("restarts managed services on its own") + && position("restarts managed services on its own") + < position("cannot tell whether the helper is running") + && position("cannot tell whether the helper is running") + < position("No `rocm` command can act on an unparseable record"), + "remedies must stay ordered most-actionable-first: {error}" + ); + } + + #[cfg(target_os = "linux")] + #[test] + fn uninstall_refuses_while_a_recorded_endpoint_still_accepts_connections() { + // The Windows grandchild case, modelled faithfully: the recorded + // processes really do die, so the stop reports "stopped" — but an engine + // child that outlived them still holds the port and the GPU. Only the + // port probe can catch that; PID bookkeeping alone says success. + let listener = + std::net::TcpListener::bind("127.0.0.1:0").expect("bind the surviving engine's socket"); + let port = listener.local_addr().expect("socket address").port(); + // The recorded supervisor: alive now, so the record stays live through + // the liveness refresh, and killable, so the stop confirms. + let supervisor = std::process::Command::new("sleep") + .arg("60") + .spawn() + .expect("spawn the recorded supervisor"); + let supervisor_pid = supervisor.id(); + let (root, paths) = test_paths("uninstall-endpoint-still-serving"); + let mut record = ManagedServiceRecord::new( + &paths, + "svc-orphaned-engine", + "vllm", + "m", + "m", + "127.0.0.1", + port, + "managed", + supervisor_pid, + None, + None, + None, + ); + record.supervisor_start_ticks = rocm_core::process_start_ticks(supervisor_pid); + record.status = "ready".to_owned(); + record.write().expect("write service record"); + + let report = + stop_managed_services_before_uninstall(&paths).expect("stop pass should succeed"); + + let mut supervisor = supervisor; + let _ = supervisor.wait(); + assert!( + !rocm_core::process_is_running(supervisor_pid), + "the recorded process must really have been stopped, so only the \ + port probe can catch the survivor" + ); + let failed: Vec<&str> = report + .failed + .iter() + .map(|failure| failure.service_id.as_str()) + .collect(); + assert_eq!( + failed, + vec!["svc-orphaned-engine"], + "a reachable endpoint must fail the gate: {report:?}" + ); + assert!( + !report.stopped.iter().any(|id| id == "svc-orphaned-engine"), + "a service that is still serving must not also be counted stopped: {report:?}" + ); + assert!( + uninstall_removal_gate(&report).is_err(), + "uninstall must not remove the tooling while something still serves" + ); + drop(listener); + let _ = fs::remove_dir_all(root); + } + + #[test] + fn a_stale_record_whose_old_port_was_reused_does_not_block_uninstall() { + // A stopped service keeps its manifest and its old port forever: nothing + // prunes records and there is no `services remove`. If the gate judged + // those records by their recorded port, any unrelated process that later + // bound it would fail uninstall deterministically, with no override and + // no recovery — `rocm services stop` cannot help an already-stopped + // record, so every retry would fail identically. + // + // Unlike its neighbours this one does NOT fail if the port probe is + // deleted, and that is deliberate rather than an oversight: it guards + // the opposite direction. The others pin that a live server blocks; this + // pins that a *stranger* does not, so it fails against a naive "any + // listener blocks" gate — the over-strict implementation the rest of + // this file's pressure pushes toward — and passes against no gate at + // all. It is a false-positive guard, so read it as one. + let listener = std::net::TcpListener::bind("127.0.0.1:0") + .expect("bind an unrelated process on a recycled port"); + let port = listener.local_addr().expect("socket address").port(); + let (root, paths) = test_paths("uninstall-stale-record-recycled-port"); + let mut record = ManagedServiceRecord::new( + &paths, + "svc-long-stopped", + "vllm", + "m", + "m", + "127.0.0.1", + port, + "managed", + 0, + None, + None, + None, + ); + record.status = "stopped".to_owned(); + record.write().expect("write service record"); + + let report = + stop_managed_services_before_uninstall(&paths).expect("stop pass should succeed"); + + assert!( + report.failed.is_empty(), + "a stale record must not be judged by whoever holds its old port now: {report:?}" + ); + assert!( + uninstall_removal_gate(&report).is_ok(), + "uninstall must not be permanently blocked by a recycled port" + ); + drop(listener); + let _ = fs::remove_dir_all(root); + } + + /// A fake OpenAI `/v1/models` endpoint on loopback that shuts itself down. + /// + /// `drop`ping a bare `JoinHandle` only detaches it: the thread stays parked + /// in `accept()` holding an ephemeral port for the life of the test binary, + /// and a failing assertion panics before any manual cleanup line. Owning the + /// shutdown in `Drop` means the port is released even when the test fails, + /// which is when it matters. + struct ServingEndpoint { + bind_host: &'static str, + port: u16, + shutdown: std::sync::Arc, + thread: Option>, + } + + impl ServingEndpoint { + /// Serve `model_id` from `/v1/models` on IPv4 loopback until dropped. + fn serving(model_id: &str) -> Self { + Self::bind(model_id, "127.0.0.1", false).expect("bind the surviving engine on IPv4") + } + + /// Serve `model_id` only to a request carrying an `Authorization` header, + /// answering 401 otherwise — a public service as the retry run meets it. + fn serving_with_authorization(model_id: &str) -> Self { + Self::bind(model_id, "127.0.0.1", true).expect("bind the authenticated engine") + } + + /// Answer `/v1/models` with an empty list — an engine that is up and + /// holding the port but has not populated its models, or is mid-unload. + fn listing_nothing() -> Self { + Self::bind("", "127.0.0.1", false).expect("bind the empty-listing engine") + } + + /// Serve `model_id` on `bind_host`, or `None` when the host's address + /// family is unavailable (IPv6 is absent in some containers, and a test + /// that needs it has to skip rather than fail). + fn bind(model_id: &str, bind_host: &'static str, require_auth: bool) -> Option { + use std::sync::atomic::Ordering; + + let listener = std::net::TcpListener::bind((bind_host, 0)).ok()?; + let port = listener.local_addr().ok()?.port(); + let shutdown = std::sync::Arc::new(std::sync::atomic::AtomicBool::new(false)); + let stopping = std::sync::Arc::clone(&shutdown); + // An empty `model_id` means "list nothing at all", not "list a model + // whose id is the empty string" — the two are different answers and + // only the first is the engine-still-loading case. + let body = if model_id.is_empty() { + r#"{"data":[]}"#.to_owned() + } else { + format!(r#"{{"data":[{{"id":"{model_id}"}}]}}"#) + }; + let thread = thread::spawn(move || { + use std::io::{Read, Write}; + while let Ok((mut stream, _)) = listener.accept() { + if stopping.load(Ordering::SeqCst) { + break; + } + stream.set_read_timeout(Some(Duration::from_secs(2))).ok(); + let mut buffer = [0_u8; 1024]; + let Ok(read) = stream.read(&mut buffer) else { + continue; + }; + let authorized = !require_auth + || String::from_utf8_lossy(&buffer[..read]) + .to_ascii_lowercase() + .contains("authorization: bearer "); + if authorized { + let _ = write!( + stream, + "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{body}", + body.len() + ); + } else { + let _ = write!( + stream, + "HTTP/1.1 401 Unauthorized\r\nContent-Type: application/json\r\nContent-Length: 2\r\nConnection: close\r\n\r\n{{}}" + ); + } + } + }); + Some(Self { + bind_host, + port, + shutdown, + thread: Some(thread), + }) + } + } + + impl Drop for ServingEndpoint { + fn drop(&mut self) { + self.shutdown + .store(true, std::sync::atomic::Ordering::SeqCst); + // Unblock the parked `accept()` so the thread observes the flag. + let _ = std::net::TcpStream::connect((self.bind_host, self.port)); + if let Some(thread) = self.thread.take() { + let _ = thread.join(); + } + } + } + + #[test] + fn a_stopped_record_still_serving_its_own_model_blocks_uninstall() { + // The retry the abort message asks for must not be the hole. A stop + // persists `status = "stopped"` BEFORE the gate probes the port, so the + // record that failed run 1 reads as not-live on run 2 — and if the gate + // only probed what it had just tried to stop, run 2 would sail through + // and remove the tooling while the survivor kept serving and holding the + // GPU. That is EAI-8014 reached by following the gate's own + // instructions, so the evidence has to survive the retry: an endpoint + // serving this record's own model blocks, whoever it belongs to. + let endpoint = ServingEndpoint::serving("amd/orphaned-model"); + let port = endpoint.port; + + let (root, paths) = test_paths("uninstall-stopped-record-still-serving"); + let mut record = ManagedServiceRecord::new( + &paths, + "svc-orphaned-engine", + "vllm", + "amd/orphaned-model", + "amd/orphaned-model", + "127.0.0.1", + port, + "managed", + 0, + None, + None, + None, + ); + // Exactly the state run 1 leaves behind: supervisor killed, record + // written as stopped, engine grandchild still on the port. + record.status = "stopped".to_owned(); + record.write().expect("write service record"); + + let report = + stop_managed_services_before_uninstall(&paths).expect("stop pass should succeed"); + + let failure = report + .failed + .iter() + .find(|failure| failure.service_id == "svc-orphaned-engine") + .unwrap_or_else(|| panic!("a still-serving engine must fail the gate: {report:?}")); + assert_eq!( + failure.remedy, + StopFailureRemedy::StopWhatHoldsThePort, + "the recorded processes are gone, so `rocm services stop` is not the remedy" + ); + // This record was already marked stopped before the run, so this pass + // never attempted a stop on it. The reason must not say the endpoint + // survived one — that describes something that did not happen, on the + // one output its operator has to reason from. Collapsing the two arms + // back into the old single sentence fails here. + assert!( + failure + .reason + .contains("is recorded stopped, but something there is still serving"), + "a record nothing was attempted on must not be reported as surviving a stop: {}", + failure.reason + ); + assert!( + !failure.reason.contains("after the stop"), + "no stop ran for this record, so the reason must not claim one did: {}", + failure.reason + ); + let error = uninstall_removal_gate(&report) + .expect_err("a still-serving engine must abort uninstall") + .to_string(); + assert!( + error.contains("outlived its supervisor"), + "says why `rocm services stop` will not help: {error}" + ); + drop(endpoint); + let _ = fs::remove_dir_all(root); + } + + #[test] + fn a_wildcard_bound_engine_is_identified_instead_of_waved_through() { + // `probe_host` normalizes a wildcard bind to loopback for the + // reachability check, but the identity probe used to be handed the raw + // record, whose `endpoint_url` still spells the wildcard. That address + // does not resolve, so the probe errored and took the fail-open branch: + // uninstall removed the tooling while a wildcard-bound engine was still + // serving — the exact outcome this gate exists to prevent. + // + // `*` is one of the spellings `probe_host` documents a record can carry, + // and unlike `0.0.0.0` it fails to resolve on every platform, so this + // pins the behaviour rather than a host quirk. + let endpoint = ServingEndpoint::serving("amd/wildcard-model"); + let (root, paths) = test_paths("uninstall-wildcard-bound-engine"); + let mut record = ManagedServiceRecord::new( + &paths, + "svc-wildcard-engine", + "vllm", + "amd/wildcard-model", + "amd/wildcard-model", + "*", + endpoint.port, + "managed", + 0, + None, + None, + None, + ); + record.status = "stopped".to_owned(); + record.write().expect("write service record"); + + let report = + stop_managed_services_before_uninstall(&paths).expect("stop pass should succeed"); + + let failure = report + .failed + .iter() + .find(|failure| failure.service_id == "svc-wildcard-engine") + .unwrap_or_else(|| { + panic!("a wildcard-bound engine still serving must fail the gate: {report:?}") + }); + assert_eq!( + failure.remedy, + StopFailureRemedy::StopWhatHoldsThePort, + "its recorded processes are gone, so the port holder is the remedy" + ); + assert!( + uninstall_removal_gate(&report).is_err(), + "the gate must refuse to remove anything" + ); + drop(endpoint); + let _ = fs::remove_dir_all(root); + } + + #[test] + fn an_ipv6_only_wildcard_engine_is_not_waved_through() { + // The IPv6 half of the wildcard case. `::` with `IPV6_V6ONLY` — the + // default on Windows, and what `TcpListener::bind("::1", 0)` gives here + // — answers on `::1` and refuses `127.0.0.1`. Probing IPv4 alone reads + // that live, port-holding engine as gone and removes the tooling anyway, + // which is EAI-8014 reached through the address family. + let Some(endpoint) = ServingEndpoint::bind("amd/v6-model", "::1", false) else { + // No IPv6 on this host: the thing under test cannot be staged. Say + // so on stderr rather than returning green and silent — a skip that + // looks identical to a pass is how a lane stops covering something + // without anyone noticing. `every_wildcard_bind_spelling_is_probed_\ + // on_both_loopback_families` still pins both families here, with no + // socket required. + eprintln!( + "SKIPPED an_ipv6_only_wildcard_engine_is_not_waved_through: no IPv6 loopback \ + on this host" + ); + return; + }; + let (root, paths) = test_paths("uninstall-ipv6-wildcard-engine"); + let mut record = ManagedServiceRecord::new( + &paths, + "svc-v6-engine", + "vllm", + "amd/v6-model", + "amd/v6-model", + // Recorded as the wildcard it was launched on, which is all the + // record ever says — not which family the listener chose. + "::", + endpoint.port, + "managed", + 0, + None, + None, + None, + ); + record.status = "stopped".to_owned(); + record.write().expect("write service record"); + + let report = + stop_managed_services_before_uninstall(&paths).expect("stop pass should succeed"); + + let failure = report + .failed + .iter() + .find(|failure| failure.service_id == "svc-v6-engine") + .unwrap_or_else(|| { + panic!("an IPv6-only engine still serving must fail the gate: {report:?}") + }); + assert_eq!( + failure.remedy, + StopFailureRemedy::StopWhatHoldsThePort, + "its recorded processes are gone, so the port holder is the remedy" + ); + assert!( + uninstall_removal_gate(&report).is_err(), + "the gate must refuse to remove anything" + ); + drop(endpoint); + let _ = fs::remove_dir_all(root); + } + + #[test] + fn every_identity_answer_maps_to_exactly_one_gate_outcome() { + // The five arms, asserted directly. Reached through the stop loop each + // one needs a server that answers a particular way plus a reading of + // stderr, which is why two of them were previously pinned by nothing: + // mutating either "proceed" arm into a block, or a block into a proceed, + // left the whole suite green while changing what a destructive command + // does to a live endpoint. + use rocm_core::EndpointIdentity::{ListsNoModels, ServesExpectedModel, ServesOtherModels}; + + assert_eq!( + stopped_record_verdict(Some(ServesExpectedModel), false), + StoppedRecordVerdict::BlockServingOurModel, + "an engine serving this record's own model outlived its supervisor" + ); + // Only this one is allowed to be silent: naming models and not naming + // ours is the single answer that is real evidence of a stranger. + assert_eq!( + stopped_record_verdict(Some(ServesOtherModels), false), + StoppedRecordVerdict::ProceedUnrelated, + "a listener naming other models is somebody else on a recycled port" + ); + // Blocking here would turn any JSON listener that lists nothing into an + // unescapable abort; proceeding silently would remove the tooling from + // under an engine that is merely still loading. Hence a third outcome. + assert_eq!( + stopped_record_verdict(Some(ListsNoModels), false), + StoppedRecordVerdict::ProceedListingNothing, + "an empty listing is not evidence of a stranger, and not silent" + ); + assert_eq!( + stopped_record_verdict(None, true), + StoppedRecordVerdict::BlockAuthRefused, + "a refusal is a live server stating it guards the path" + ); + assert_eq!( + stopped_record_verdict(None, false), + StoppedRecordVerdict::ProceedUnidentified, + "no usable answer is the documented fail-open" + ); + + assert!(stopped_record_verdict(Some(ServesExpectedModel), false).blocks()); + assert!(stopped_record_verdict(None, true).blocks()); + assert!(!stopped_record_verdict(Some(ServesOtherModels), false).blocks()); + assert!(!stopped_record_verdict(Some(ListsNoModels), false).blocks()); + assert!(!stopped_record_verdict(None, false).blocks()); + } + + #[test] + fn an_endpoint_listing_nothing_does_not_block_uninstall() { + // The end-to-end half of the `ListsNoModels` arm: it must not abort. + // Blocking would make any listener that answers `/v1/models` with an + // empty list — including something unrelated on a recycled port — an + // abort with no override, which is the dead end this gate must not + // create. Sensitive to exactly one mutation: turning that arm into a + // block. + // + // What keeps this a tradeoff rather than a silent removal is the + // warning on stderr, and that warning is pinned by nothing — no test + // here reads stderr, so emptying its body while leaving the `continue` + // would stay green. What is pinned is only that `ListsNoModels` reaches + // a verdict distinct from the silent one, which is the precondition for + // the warning rather than the warning itself. + let endpoint = ServingEndpoint::listing_nothing(); + let (root, paths) = test_paths("uninstall-endpoint-listing-nothing"); + let mut record = ManagedServiceRecord::new( + &paths, + "svc-empty-listing", + "vllm", + "amd/loading-model", + "amd/loading-model", + "127.0.0.1", + endpoint.port, + "managed", + 0, + None, + None, + None, + ); + record.status = "stopped".to_owned(); + record.write().expect("write service record"); + + let report = + stop_managed_services_before_uninstall(&paths).expect("stop pass should succeed"); + + assert!( + report.failed.is_empty(), + "an empty model list must not abort uninstall: {report:?}" + ); + assert!( + uninstall_removal_gate(&report).is_ok(), + "the gate must let the removal proceed" + ); + // The disclosure, not just the decision. Proceeding here is only + // defensible because the operator is told the port was never proven + // free, so emptying that message is as much a regression as flipping + // the verdict — and until the warnings became values on the report, + // nothing could say so. + assert!( + report + .warnings + .iter() + .any(|warning| warning.contains("empty model list") + && warning.contains("svc-empty-listing")), + "proceeding past an empty listing must disclose itself: {:?}", + report.warnings + ); + drop(endpoint); + let _ = fs::remove_dir_all(root); + } + + #[test] + fn a_record_whose_host_no_longer_resolves_says_so_before_proceeding() { + // The third fail-open, and the one that used to pass in silence. A + // refused connect and an unresolvable name both come back `false` from + // `loopback_tcp_port_is_reachable`, but they are opposite evidence: the + // first says the port is free, the second says the question was never + // asked. Proceeding on the second is still the right call — a record + // written on another machine must not brick uninstall — but it has to + // be disclosed, and `.invalid` is reserved by RFC 2606 precisely so it + // never resolves anywhere. + let (root, paths) = test_paths("uninstall-host-unresolvable"); + let mut record = ManagedServiceRecord::new( + &paths, + "svc-foreign-host", + "vllm", + "amd/our-model", + "amd/our-model", + "no-such-host.invalid", + 8123, + "managed", + 0, + None, + None, + None, + ); + record.status = "stopped".to_owned(); + record.write().expect("write service record"); + + let report = + stop_managed_services_before_uninstall(&paths).expect("stop pass should succeed"); + + assert!( + report.failed.is_empty(), + "an unresolvable host must not abort uninstall: {report:?}" + ); + assert!( + report + .warnings + .iter() + .any(|warning| warning.contains("does not resolve here") + && warning.contains("svc-foreign-host")), + "skipping a record whose host does not resolve must disclose itself: {:?}", + report.warnings + ); + let _ = fs::remove_dir_all(root); + } + + #[test] + fn a_listener_naming_another_model_does_not_block_uninstall() { + // The end-to-end half of the `ServesOtherModels` arm, and the only test + // that drives it through the real call site rather than through the + // pure helper. A listener that names its models and does not name ours + // is the one answer that is positive evidence of a stranger on a + // recycled port, so it must proceed — blocking here would let any + // unrelated OpenAI-shaped server on a reused port wedge uninstall with + // no override. Sensitive to exactly one mutation: turning that arm into + // a block. + let endpoint = ServingEndpoint::serving("amd/somebody-elses-model"); + let (root, paths) = test_paths("uninstall-endpoint-other-model"); + let mut record = ManagedServiceRecord::new( + &paths, + "svc-recycled-port", + "vllm", + "amd/our-model", + "amd/our-model", + "127.0.0.1", + endpoint.port, + "managed", + 0, + None, + None, + None, + ); + record.status = "stopped".to_owned(); + record.write().expect("write service record"); + + let report = + stop_managed_services_before_uninstall(&paths).expect("stop pass should succeed"); + + assert!( + report.failed.is_empty(), + "a listener naming only other models must not abort uninstall: {report:?}" + ); + assert!( + uninstall_removal_gate(&report).is_ok(), + "the gate must let the removal proceed" + ); + // The one proceed-arm that is allowed to be silent, and the assertion + // that keeps it distinguishable from the two that are not: a listener + // that named its models and did not name ours is positive evidence of a + // stranger, so there is nothing to disclose. + assert!( + report.warnings.is_empty(), + "a named stranger is evidence, not a fail-open, so it warns about nothing: {:?}", + report.warnings + ); + drop(endpoint); + let _ = fs::remove_dir_all(root); + } + + #[test] + fn an_authenticated_survivor_blocks_the_retry_instead_of_failing_open() { + // The retry run is where a public service loses its own evidence: + // stopping the recorded processes clears the stored endpoint key, so run + // 2 probes the still-serving endpoint with no credentials and is + // answered 401. `managed_service_endpoint_model_ready` turns any non-200 + // into `Err`, which used to take the fail-open branch — deleting the + // tooling while a publicly reachable, GPU-holding server kept answering. + // + // A refusal is not a failure to reach: it is a live HTTP server saying + // it guards this path, and that has to block. + let endpoint = ServingEndpoint::serving_with_authorization("amd/guarded-model"); + let (root, paths) = test_paths("uninstall-authenticated-survivor"); + let mut record = ManagedServiceRecord::new( + &paths, + "svc-guarded-engine", + "vllm", + "amd/guarded-model", + "amd/guarded-model", + "127.0.0.1", + endpoint.port, + "managed", + 0, + None, + None, + None, + ); + // Exactly run 2's state: recorded stopped, and no key on disk for it. + record.status = "stopped".to_owned(); + record.write().expect("write service record"); + assert!( + endpoint_keys::endpoint_api_key(&paths, "svc-guarded-engine").is_none(), + "the retry has no credentials left — that is the whole scenario" + ); + + let report = + stop_managed_services_before_uninstall(&paths).expect("stop pass should succeed"); + + let failure = report + .failed + .iter() + .find(|failure| failure.service_id == "svc-guarded-engine") + .unwrap_or_else(|| { + panic!( + "an authenticated survivor must fail the gate, not be waved through: {report:?}" + ) + }); + assert_eq!( + failure.remedy, + StopFailureRemedy::StopWhatHoldsThePort, + "its recorded processes are gone, so the port holder is the remedy" + ); + assert!( + failure.reason.contains("refused"), + "the abort has to say the endpoint refused the probe, not that it was silent: {}", + failure.reason + ); + assert!( + uninstall_removal_gate(&report).is_err(), + "the gate must refuse to remove anything" + ); + drop(endpoint); + let _ = fs::remove_dir_all(root); + } + + #[cfg(target_os = "linux")] + #[test] + fn a_doomed_run_stops_nothing_on_its_way_to_the_abort() { + // Stopping is not free and not undoable: each confirmed stop drops that + // service's endpoint key, and a publicly bound service has to be served + // again with an explicit flag to get one back. Once the gate is certain + // to abort — nothing removed, tooling intact — every stop performed + // first is pure cost for a removal that will not happen. + // + // The daemon failure used here is the unreadable-state one, which this + // file already documents at length as the reason to leave the helper + // alone. That same reasoning has to cover the services: it made no sense + // to spare the daemon and then stop everything else for no gain. + let server = std::process::Command::new("sleep") + .arg("60") + .spawn() + .expect("spawn managed server"); + let pid = server.id(); + let (root, paths) = test_paths("uninstall-doomed-run-stops-nothing"); + paths.ensure().expect("create the app directories"); + fs::write(paths.automation_state_path(), b"{ not json") + .expect("seed a corrupt runtime state"); + let real = rocm_core::process_start_ticks(pid).expect("start-ticks"); + let mut record = managed_record_for_pid(&paths, pid, Some(real)); + record.status = "ready".to_owned(); + record.write().expect("write service record"); + + let report = + stop_managed_services_before_uninstall(&paths).expect("stop pass should succeed"); + + assert!( + report.stopped.is_empty(), + "a run that is already going to abort must stop nothing: {report:?}" + ); + let mut server = server; + assert!( + server.try_wait().expect("poll the server").is_none(), + "the managed server must be left running by a run that aborts anyway" + ); + assert!( + uninstall_removal_gate(&report).is_err(), + "the unreadable runtime state must still abort the uninstall" + ); + let _ = server.kill(); + let _ = server.wait(); + let _ = fs::remove_dir_all(root); + } + + #[cfg(target_os = "linux")] + #[test] + fn an_unparseable_manifest_aborts_before_any_service_is_stopped() { + // Same shape as the daemon case and worse: a manifest that cannot be + // parsed may itself describe a live, GPU-holding server, so collecting + // it last meant every other service was already down — and its key + // already dropped — before the run turned out to be doomed. + // + // The daemon is staged live and fully verifiable here on purpose. It is + // the one thing the gate would otherwise still destroy on a doomed run: + // the manifest scan only reads, so putting the force-kill ahead of it + // terminates a healthy `rocmd` for an uninstall that removes nothing. + let server = std::process::Command::new("sleep") + .arg("60") + .spawn() + .expect("spawn managed server"); + let pid = server.id(); + let daemon = std::process::Command::new("sleep") + .arg("60") + .spawn() + .expect("spawn background helper"); + let daemon_pid = daemon.id(); + let (root, paths) = test_paths("uninstall-bad-manifest-ordering"); + paths.ensure().expect("create the app directories"); + let mut state = runtime_state(true, daemon_pid); + state.daemon_start_ticks = rocm_core::process_start_ticks(daemon_pid); + state.write(&paths).expect("write runtime state"); + let real = rocm_core::process_start_ticks(pid).expect("start-ticks"); + let mut record = managed_record_for_pid(&paths, pid, Some(real)); + record.status = "ready".to_owned(); + record.write().expect("write service record"); + fs::write(paths.services_dir().join("svc-corrupt.json"), b"{ not json") + .expect("seed an unparseable manifest"); + + let report = + stop_managed_services_before_uninstall(&paths).expect("stop pass should succeed"); + + assert!( + report.stopped.is_empty(), + "an unparseable manifest must abort before anything is stopped: {report:?}" + ); + let mut server = server; + let mut daemon = daemon; + assert!( + server.try_wait().expect("poll the server").is_none(), + "the healthy service must be untouched when the run aborts on another record" + ); + assert!( + daemon.try_wait().expect("poll the helper").is_none(), + "a live, verifiable rocmd must not be force-killed for a run that removes nothing" + ); + assert!( + report + .failed + .iter() + .any(|failure| failure.remedy == StopFailureRemedy::RepairTheRecord), + "the unparseable manifest must be the recorded failure: {report:?}" + ); + let _ = server.kill(); + let _ = server.wait(); + let _ = daemon.kill(); + let _ = daemon.wait(); + let _ = fs::remove_dir_all(root); + } + + /// The platform conjunct in `record_predates_start_ticks`, pinned on the one + /// lane where it is falsifiable. + /// + /// On Linux `process_start_ticks` always answers for a live process, so the + /// conjunct is unconditionally true there and *no* Linux assertion can + /// distinguish the predicate from a bare `is_none()`. Deleting it leaves + /// every Linux test green — which is exactly what happened to this test's + /// previous version. Off Linux the same call is a compile-time stub that + /// always answers `None`, so the conjunct decides the result, and this + /// assertion fails the moment it is dropped. It is the only assertion + /// anywhere that does. + /// + /// The behaviour it protects: with no start-time readable on this platform, + /// a record carrying none is NOT evidence of a pre-upgrade record — it is + /// just what every record looks like here. Treating it as pre-upgrade would + /// abort every uninstall that finds a live daemon on Windows and macOS. + #[cfg(not(any(target_os = "linux", windows)))] + #[test] + fn without_readable_start_times_a_bare_record_is_not_a_legacy_record() { + assert!( + !record_predates_start_ticks(None), + "where no start-time can be read, an absent one says nothing about the record's age" + ); + assert!( + !record_predates_start_ticks(Some(1)), + "a record that carries a start-time is never a pre-upgrade record" + ); + } + + #[cfg(any(target_os = "linux", windows))] + #[test] + fn a_record_without_a_start_time_is_a_legacy_record_where_start_times_are_readable() { + // The readable-start-time half of the predicate's contract (Linux, Windows), and no more than that. + // + // What actually prevents the conflation this predicate exists for — + // answering "can this platform report a start-time?" by reading the PID + // under inspection, which cannot tell "no `/proc` on this OS" from "that + // read just failed" — is the signature: `record_predates_start_ticks` + // takes no PID, so passing one is a compile error. It is prevented by + // construction, not caught by an assertion, and no assertion here should + // claim otherwise. + // + // The platform conjunct itself is unfalsifiable on this lane: on Linux + // `process_start_ticks` always answers for a live process, so the + // conjunct is constantly true and this test cannot tell the predicate + // from a bare `is_none()`. `without_readable_start_times_a_bare_record_\ + // is_not_a_legacy_record` is what pins it, and only the non-Linux lanes + // run that. + // + // The guard's end-to-end behaviour is pinned separately, by + // `uninstall_never_kills_a_daemon_pid_from_a_state_file_that_predates_\ + // start_ticks`, which stages a live unrelated process against a legacy + // record and fails if the guard is removed. + assert!( + record_predates_start_ticks(None), + "where start-times are readable, a record carrying none predates the field" + ); + assert!( + !record_predates_start_ticks(Some(1)), + "a record that carries a start-time is never a pre-upgrade record" + ); + } + + // `sleep` as a stand-in for the process that inherited the pid. + #[cfg(unix)] + #[test] + fn uninstall_never_signals_a_daemon_pid_recorded_as_already_stopped() { + // `running: false` is written by exactly one place — rocmd's clean + // shutdown, on its way out. (A daemon started without + // `--automations-enabled` returns before its first state write, so it + // never persists a false flag while alive.) The pid in such a record + // therefore belongs to a process that has already exited, and anything + // live under that number today inherited it. + // + // Windows is where this is the only defence: `process_start_ticks` is + // always `None` there, so the identity check falls back to its legacy + // `Matches` verdict and the force tree-kill lands on a stranger. That + // shape cannot be staged on Linux — identity genuinely works here — so + // this stages the same *decision*: a live pid that the identity check + // will confirm, which must still be left alone because the record says + // the daemon is stopped. + let mut stranger = std::process::Command::new("sleep") + .arg("60") + .spawn() + .expect("spawn unrelated process"); + let pid = stranger.id(); + let (root, paths) = test_paths("uninstall-daemon-recorded-stopped"); + let mut state = runtime_state(false, pid); + state.daemon_start_ticks = rocm_core::process_start_ticks(pid); + state.write(&paths).expect("write runtime state"); + + let report = + stop_managed_services_before_uninstall(&paths).expect("stop pass should succeed"); + + // `try_wait`, not `process_is_running`: a signalled child we have not + // reaped is a zombie, and a zombie still reads as running — so the + // liveness check would pass over the very kill this test exists to + // catch. An exit status here means it was signalled. + assert!( + stranger.try_wait().expect("poll the stranger").is_none(), + "a pid recorded as already stopped must never be signalled: uninstall killed an \ + unrelated process" + ); + assert!( + report.stopped.is_empty(), + "the daemon was already stopped, so nothing was stopped here: {report:?}" + ); + assert!( + report.failed.is_empty(), + "an already-stopped daemon is not an obstacle to uninstall: {report:?}" + ); + let _ = stranger.kill(); + let _ = stranger.wait(); + let _ = fs::remove_dir_all(root); + } + + #[test] + fn every_wildcard_bind_spelling_is_probed_on_both_loopback_families() { + // A wildcard probed literally is not portable and would read as "nothing + // is serving" — the gate's failure-open direction, so it matters. + // + // Both families, not one: a wildcard record does not say which protocol + // the listener bound, and the two loopbacks are not interchangeable. A + // `::` bind with the usual `IPV6_V6ONLY=1` — the default on Windows — + // answers on `::1` and refuses `127.0.0.1`, so probing IPv4 alone reads + // a live engine as gone and waves the removal through. + for wildcard in [ + "0.0.0.0", + "::", + "[::]", + "0:0:0:0:0:0:0:0", + "*", + "", + " 0.0.0.0 ", + ] { + assert_eq!( + probe_hosts(wildcard), + vec!["127.0.0.1".to_owned(), "::1".to_owned()], + "wildcard {wildcard:?} must be probed on both loopback families" + ); + } + for literal in ["127.0.0.1", "192.168.1.10", "example.internal"] { + assert_eq!( + probe_hosts(literal), + vec![literal.to_owned()], + "a concrete host must be probed as recorded, and only there" + ); + } + // A concrete host still has to come back in a form that resolves. + // `(host, port).to_socket_addrs()` rejects a bracketed literal and + // anything padded, and the caller reads a resolution failure as "nothing + // is serving" — so handing the raw spelling through would delete the + // recovery tooling while the endpoint is live. + for (recorded, probed) in [ + ("[::1]", "::1"), + (" 127.0.0.1 ", "127.0.0.1"), + ("[FE80::1]", "fe80::1"), + ] { + assert_eq!( + probe_hosts(recorded), + vec![probed.to_owned()], + "a concrete host must be probed in a resolvable form" + ); + } + // Every probed form, wildcard expansions included, has to resolve: + // `(host, port).to_socket_addrs()` rejects a bracketed literal and + // anything padded, and the caller reads a resolution failure as "nothing + // is serving" — so handing a raw spelling through would delete the + // recovery tooling while the endpoint is live. + for recorded in ["[::1]", " 127.0.0.1 ", "[FE80::1]", "::", "0.0.0.0", "*"] { + for probed in probe_hosts(recorded) { + assert!( + (probed.as_str(), 1u16).to_socket_addrs().is_ok(), + "the probed form {probed:?} of {recorded:?} must resolve" + ); + } + } + } + + #[test] + fn only_an_uninstall_that_removes_the_recovery_tooling_stops_servers() { + // `--keep-binaries --keep-data` removes the cache alone: `rocm services + // stop` and every service record survive, so there is nothing to protect + // by force-stopping live servers. + let (root, paths) = test_paths("uninstall-recovery-tooling-scope"); + let cache_only = UninstallPlan { + actions: vec![UninstallPlanEntry { + kind: "cache", + path: paths.cache_dir.clone(), + }], + ..UninstallPlan::default() + }; + assert!( + !plan_removes_recovery_tooling(&cache_only, &paths), + "a cache-only uninstall keeps the tooling that stops servers" + ); + + let with_binaries = UninstallPlan { + actions: vec![UninstallPlanEntry { + kind: "binary", + path: PathBuf::from("/usr/local/bin/rocm"), + }], + ..UninstallPlan::default() + }; + assert!( + plan_removes_recovery_tooling(&with_binaries, &paths), + "removing the binaries takes away `rocm services stop`" + ); + + let with_data = UninstallPlan { + actions: vec![UninstallPlanEntry { + kind: "data", + path: paths.data_dir.clone(), + }], + ..UninstallPlan::default() + }; + assert!( + plan_removes_recovery_tooling(&with_data, &paths), + "removing the data dir takes away the service records" + ); + let _ = fs::remove_dir_all(root); + } + + #[cfg(target_os = "linux")] + #[test] + fn the_plan_warning_only_promises_a_stop_the_uninstall_will_perform() { + // The plan is printed and confirmed BEFORE the stop pass decides whether + // to run, so a warning that promises a stop on a run that keeps the + // tooling would have the operator confirm work that never happens. + // Asserting the predicate alone (above) cannot catch that drift; this + // pins the operator-visible text to the same condition. + let child = std::process::Command::new("sleep") + .arg("60") + .spawn() + .expect("spawn a live managed server"); + let pid = child.id(); + let (root, paths) = test_paths("uninstall-warning-matches-behaviour"); + let mut record = managed_record_for_pid(&paths, pid, rocm_core::process_start_ticks(pid)); + record.status = "ready".to_owned(); + record.write().expect("write service record"); + + let full = build_uninstall_plan( + &paths, + &UninstallOptions { + yes: true, + force_dev_binaries: true, + ..UninstallOptions::default() + }, + ) + .expect("build the full plan"); + let full_warning = full + .warnings + .iter() + .find(|warning| warning.contains("managed service record")) + .expect("the plan warns about managed services"); + assert!( + full_warning.contains("will be stopped before removal"), + "a removal that takes the tooling away must promise the stop: {full_warning}" + ); + + let cache_only = build_uninstall_plan( + &paths, + &UninstallOptions { + yes: true, + keep_binaries: true, + keep_config: true, + keep_data: true, + ..UninstallOptions::default() + }, + ) + .expect("build the cache-only plan"); + let cache_warning = cache_only + .warnings + .iter() + .find(|warning| warning.contains("managed service record")) + .expect("the plan warns about managed services"); + assert!( + cache_warning.contains("left running"), + "a removal that keeps the tooling must not promise a stop: {cache_warning}" + ); + assert!( + !cache_warning.contains("will be stopped before removal"), + "a removal that keeps the tooling must not promise a stop: {cache_warning}" + ); + + let mut child = child; + let _ = child.kill(); + let _ = child.wait(); + let _ = fs::remove_dir_all(root); + } + + #[cfg(unix)] + #[test] + fn the_plan_warning_for_only_stopped_records_promises_no_stop() { + let (root, paths) = test_paths("uninstall-warning-stopped-records"); + let mut record = managed_record_for_pid(&paths, u32::MAX - 1, None); + record.status = "stopped".to_owned(); + record.write().expect("write service record"); + + let plan = build_uninstall_plan( + &paths, + &UninstallOptions { + yes: true, + force_dev_binaries: true, + ..UninstallOptions::default() + }, + ) + .expect("build the plan"); + let warning = plan + .warnings + .iter() + .find(|warning| warning.contains("managed service record")) + .expect("the plan warns about managed services"); + assert!( + warning.contains("none is recorded as running"), + "records that are not live must be described as such: {warning}" + ); + assert!( + !warning.contains("will be stopped"), + "no stop is promised when nothing is live: {warning}" + ); + let _ = fs::remove_dir_all(root); + } + + #[test] + fn uninstall_removal_gate_reports_count_when_all_stopped() { + let report = ManagedServiceStopReport { + stopped: vec!["a".to_owned(), "b".to_owned()], + failed: Vec::new(), + warnings: Vec::new(), + }; + let line = uninstall_removal_gate(&report).expect("all stopped must proceed"); + assert_eq!( + line.as_deref(), + Some("stopped 2 managed service(s) before removal") + ); + } + + #[test] + fn uninstall_removal_gate_is_silent_with_nothing_to_stop() { + let report = ManagedServiceStopReport::default(); + assert!( + uninstall_removal_gate(&report) + .expect("no services must proceed") + .is_none(), + "no managed services means no line to print" + ); + } + fn test_paths(name: &str) -> (PathBuf, AppPaths) { let root = PathBuf::from(env!("CARGO_MANIFEST_DIR")) .join("..") @@ -37665,11 +39399,15 @@ ID_LIKE="suse opensuse" } /// Build an `AutomationRuntimeState` for the no-double-spawn guard tests. + /// + /// `daemon_start_ticks` is left unrecorded; tests that care about the + /// daemon's identity set it explicitly. fn runtime_state(running: bool, daemon_pid: u32) -> AutomationRuntimeState { AutomationRuntimeState { running, automations_enabled: true, daemon_pid, + daemon_start_ticks: None, started_at_unix_ms: 1, last_tick_unix_ms: 1, local_webhook_endpoint: None, @@ -38151,4 +39889,159 @@ ID_LIKE="suse opensuse" ); } } + + #[test] + fn every_daemon_identity_reading_maps_to_exactly_one_gate_outcome() { + use rocm_core::IdentityState::{Gone, Indeterminate, Matches, Recycled}; + // `Indeterminate` must abort: folding it into the proceed arm would + // delete the tooling while a live `rocmd` keeps respawning services. + assert_eq!( + daemon_identity_outcome(Gone, false), + DaemonIdentityOutcome::NothingOfOurs + ); + assert_eq!( + daemon_identity_outcome(Recycled, false), + DaemonIdentityOutcome::NothingOfOurs + ); + assert_eq!( + daemon_identity_outcome(Indeterminate, false), + DaemonIdentityOutcome::Unverifiable + ); + assert_eq!( + daemon_identity_outcome(Indeterminate, true), + DaemonIdentityOutcome::Unverifiable + ); + assert_eq!( + daemon_identity_outcome(Matches, true), + DaemonIdentityOutcome::Unverifiable + ); + assert_eq!( + daemon_identity_outcome(Matches, false), + DaemonIdentityOutcome::Stop + ); + } + + #[cfg(unix)] + #[test] + fn a_failed_per_service_stop_aborts_with_the_service_advice() { + let (root, paths) = test_paths("uninstall-stop-errs"); + let mut child = KillOnDrop( + std::process::Command::new("sleep") + .arg("60") + .spawn() + .expect("spawn a live managed server"), + ); + let pid = child.0.id(); + let mut record = managed_record_for_pid(&paths, pid, rocm_core::process_start_ticks(pid)); + record.status = "ready".to_owned(); + record.write().expect("write service record"); + + let report = stop_managed_services_with(&paths, |_, _| anyhow::bail!("stop blew up")) + .expect("the pass itself succeeds"); + + assert_eq!( + report.failed.len(), + 1, + "the Err must be recorded as a failure" + ); + assert_eq!(report.failed[0].remedy, StopFailureRemedy::StopTheService); + assert!(report.failed[0].reason.contains("stop blew up")); + assert!(report.stopped.is_empty()); + let error = uninstall_removal_gate(&report).expect_err("the gate must abort"); + assert!(format!("{error:#}").contains("rocm services stop --yes")); + child.0.kill().ok(); + let _ = fs::remove_dir_all(root); + } + + #[cfg(unix)] + #[test] + fn a_stop_that_returns_ok_but_not_stopped_is_a_failure() { + let (root, paths) = test_paths("uninstall-stop-ok-not-stopped"); + let child = KillOnDrop( + std::process::Command::new("sleep") + .arg("60") + .spawn() + .expect("spawn a live managed server"), + ); + let pid = child.0.id(); + let mut record = managed_record_for_pid(&paths, pid, rocm_core::process_start_ticks(pid)); + record.status = "ready".to_owned(); + record.write().expect("write service record"); + + let report = + stop_managed_services_with(&paths, |_, _| Ok(serde_json::json!({ "status": "ready" }))) + .expect("the pass itself succeeds"); + + assert_eq!(report.failed.len(), 1, "{report:?}"); + assert_eq!(report.failed[0].remedy, StopFailureRemedy::StopTheService); + assert!(report.stopped.is_empty(), "{report:?}"); + assert!(uninstall_removal_gate(&report).is_err()); + let _ = fs::remove_dir_all(root); + } + + #[test] + fn the_endpoint_sentence_follows_the_failure_class() { + let sentence = "may still be serving"; + for (remedy, expected) in [ + (StopFailureRemedy::StopTheService, true), + (StopFailureRemedy::StopWhatHoldsThePort, true), + (StopFailureRemedy::StopTheDaemon, false), + (StopFailureRemedy::RepairTheDaemonState, false), + (StopFailureRemedy::RepairTheRecord, false), + ] { + let report = ManagedServiceStopReport { + stopped: Vec::new(), + failed: vec![FailedManagedServiceStop { + service_id: "x".to_owned(), + reason: "r".to_owned(), + remedy, + }], + warnings: Vec::new(), + }; + let message = format!("{:#}", uninstall_removal_gate(&report).unwrap_err()); + assert_eq!( + message.contains(sentence), + expected, + "{remedy:?}: {message}" + ); + } + } + + #[test] + fn the_abort_only_claims_endpoints_may_serve_when_a_service_failed() { + let repair_only = ManagedServiceStopReport { + stopped: Vec::new(), + failed: vec![FailedManagedServiceStop { + service_id: "/x/broken.json".to_owned(), + reason: "service record could not be parsed".to_owned(), + remedy: StopFailureRemedy::RepairTheRecord, + }], + warnings: Vec::new(), + }; + let message = format!("{:#}", uninstall_removal_gate(&repair_only).unwrap_err()); + assert!(!message.contains("may still be serving"), "{message}"); + + let service = ManagedServiceStopReport { + stopped: Vec::new(), + failed: vec![FailedManagedServiceStop { + service_id: "svc".to_owned(), + reason: "still ready".to_owned(), + remedy: StopFailureRemedy::StopTheService, + }], + warnings: Vec::new(), + }; + let message = format!("{:#}", uninstall_removal_gate(&service).unwrap_err()); + assert!(message.contains("may still be serving"), "{message}"); + } + + #[cfg(unix)] + struct KillOnDrop(std::process::Child); + + #[cfg(unix)] + impl Drop for KillOnDrop { + fn drop(&mut self) { + let _ = self.0.kill(); + let _ = self.0.wait(); + } + } } diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index d8c236eae..761f94a4a 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -4610,6 +4610,18 @@ fn temp_sibling_path(path: &Path, suffix: &OsStr) -> Result { Ok(parent.join(file_name)) } +/// Stage-and-publish a file here, sharing only the publish step with `rocm-core`. +/// +/// Deliberately not [`rocm_core::write_file_atomically`], and not a copy of it +/// either: only the Windows-sensitive publish (`ReplaceFileW` and its fallbacks) +/// is single-sourced, via [`publish_temp_file`]. The staging half stays local +/// because it carries the `suffix_for_attempt` and `before_publish` seams the +/// tests below drive to force temp-name collisions, write failures and rename +/// races — injection points `rocm_core`'s caller-facing helper does not expose. +/// +/// Consequence worth knowing: this path does **not** `sync_all` before +/// publishing, so unlike the `rocm-core` helper it is atomic but carries no +/// crash-durability guarantee for the staged bytes. fn write_file_atomically(path: &Path, bytes: &[u8]) -> Result<()> { let temp_id = format!("{}-{}", std::process::id(), unix_time_millis()); write_file_atomically_with( @@ -4726,60 +4738,10 @@ where .with_context(|| format!("failed to publish {}", path.display())) } -#[cfg(not(windows))] -fn publish_temp_file(tmp: &Path, path: &Path) -> io::Result<()> { - fs::rename(tmp, path) -} - -#[cfg(windows)] +/// The publish step lives in `rocm-core` so there is one implementation of the +/// Windows `ReplaceFileW` handling for the whole workspace. fn publish_temp_file(tmp: &Path, path: &Path) -> io::Result<()> { - if path.try_exists()? { - return replace_file_windows(path, tmp); - } - - match fs::rename(tmp, path) { - Ok(()) => Ok(()), - Err(rename_error) => { - if path.try_exists()? { - replace_file_windows(path, tmp) - } else { - Err(rename_error) - } - } - } -} - -#[cfg(windows)] -#[allow(unsafe_code)] -fn replace_file_windows(path: &Path, replacement: &Path) -> io::Result<()> { - use std::os::windows::ffi::OsStrExt; - use windows_sys::Win32::Storage::FileSystem::ReplaceFileW; - - let path_wide: Vec = path.as_os_str().encode_wide().chain(Some(0)).collect(); - let replacement_wide: Vec = replacement - .as_os_str() - .encode_wide() - .chain(Some(0)) - .collect(); - - // SAFETY: both path buffers are valid, NUL-terminated UTF-16 strings and - // remain alive for the duration of the synchronous Windows API call. The - // optional backup, exclude, and reserved pointers are intentionally null. - let replaced = unsafe { - ReplaceFileW( - path_wide.as_ptr(), - replacement_wide.as_ptr(), - std::ptr::null(), - 0, - std::ptr::null(), - std::ptr::null(), - ) - }; - if replaced == 0 { - Err(io::Error::last_os_error()) - } else { - Ok(()) - } + rocm_core::publish_temp_file(tmp, path) } fn extract_tarball(archive_path: &Path, target_dir: &Path) -> Result<()> { diff --git a/apps/rocm/src/uninstall.rs b/apps/rocm/src/uninstall.rs index 42ab83a8b..a759e30f3 100644 --- a/apps/rocm/src/uninstall.rs +++ b/apps/rocm/src/uninstall.rs @@ -13,14 +13,27 @@ use anyhow::{Context, Result, bail}; use rocm_core::{AppPaths, interactive_terminal}; +use crate::uninstall_gate::{ + ManagedServiceStopReport, stop_managed_services_before_uninstall, uninstall_removal_gate, +}; use crate::{ - UninstallOptions, build_uninstall_plan, confirm_uninstall, remove_path, render_uninstall_plan, + UninstallOptions, UninstallPlan, build_uninstall_plan, confirm_uninstall, + plan_removes_recovery_tooling, remove_path, render_uninstall_plan, }; pub(crate) fn uninstall(options: UninstallOptions) -> Result<()> { let paths = AppPaths::discover()?; - let plan = build_uninstall_plan(&paths, &options)?; - print!("{}", render_uninstall_plan(&plan, &options)); + uninstall_with_paths(&paths, &options) +} + +/// The whole `uninstall` command against a given [`AppPaths`]. +/// +/// Split from [`uninstall`] only so a test can drive the real command — plan, +/// confirm gate, stop pass, removal — against an isolated root, instead of +/// exercising the pieces separately and taking the wiring between them on faith. +fn uninstall_with_paths(paths: &AppPaths, options: &UninstallOptions) -> Result<()> { + let plan = build_uninstall_plan(paths, options)?; + print!("{}", render_uninstall_plan(&plan, options)); if plan.actions.is_empty() || options.dry_run { return Ok(()); @@ -36,6 +49,48 @@ pub(crate) fn uninstall(options: UninstallOptions) -> Result<()> { } } + // Only an uninstall that takes away the means of stopping a server has to + // stop it first; a cache-only run leaves `rocm services stop` and every + // service record in place. + let removes_recovery_tooling = plan_removes_recovery_tooling(&plan, paths); + stop_managed_services_then_remove(&plan, || { + if removes_recovery_tooling { + stop_managed_services_before_uninstall(paths) + } else { + Ok(ManagedServiceStopReport::default()) + } + }) +} + +/// Stop the servers this machine manages, then remove the planned paths — in +/// that order, and only if every stop was confirmed. +/// +/// Uninstall used to report success while a publicly-bound, GPU-holding endpoint +/// kept serving, then delete the tooling needed to stop it (EAI-8014). The stop +/// runs first and the gate aborts on any unconfirmed stop, so the recovery +/// tooling stays in place. +/// +/// The stop pass is a parameter, not a direct call, so the ordering guarantee is +/// testable rather than merely inspectable: a test can hand in a stop that fails +/// — which no test can reliably provoke from a real process — and assert that +/// not one planned path was removed. Deleting the stop from this function makes +/// those tests fail. +fn stop_managed_services_then_remove( + plan: &UninstallPlan, + stop_managed_services: impl FnOnce() -> Result, +) -> Result<()> { + let stop_report = stop_managed_services()?; + // Every fail-open the stop pass took, on stderr rather than stdout: these + // are the places uninstall proceeded without proving the port was free, so + // they have to survive the operator piping its output somewhere. Printed + // before the gate, so they are on screen whether or not it aborts. + for warning in &stop_report.warnings { + eprintln!("warning: {warning}"); + } + if let Some(line) = uninstall_removal_gate(&stop_report)? { + println!("{line}"); + } + for entry in &plan.actions { remove_path(&entry.path) .with_context(|| format!("failed to remove {}", entry.path.display()))?; @@ -44,3 +99,455 @@ pub(crate) fn uninstall(options: UninstallOptions) -> Result<()> { println!("uninstall complete"); Ok(()) } + +#[cfg(test)] +mod tests { + use std::fs; + use std::path::PathBuf; + + use anyhow::bail; + + use super::stop_managed_services_then_remove; + use crate::uninstall_gate::{ + FailedManagedServiceStop, ManagedServiceStopReport, StopFailureRemedy, + }; + use crate::{UninstallPlan, UninstallPlanEntry}; + + /// A port nothing can ever be serving on. + /// + /// These tests need a record whose endpoint reads as dead, and the gate + /// really does probe it. Port 0 resolves — so the probe runs rather than + /// being skipped by a resolution failure — and the connect always fails, + /// on every platform. Binding an ephemeral port and dropping it would leave + /// a window in which something else on a busy runner grabs the port and + /// fails these tests through the gate's own probe. + /// + /// Both users are Linux-only, so the constant is too — `-D warnings` makes + /// dead code a build failure on the other platforms. + #[cfg(target_os = "linux")] + const UNSERVABLE_PORT: u16 = 0; + + /// An isolated root and a plan whose one action removes a real file in it. + /// + /// The file is what makes these tests assertions about *removal* rather than + /// about return values: it exists before the call, and its presence + /// afterwards is the evidence that the abort happened before the removal + /// loop, not after it. + fn plan_removing_one_file(name: &str) -> (PathBuf, UninstallPlan) { + let root = std::env::temp_dir().join(format!( + "rocm-cli-uninstall-test-{name}-{}-{}", + std::process::id(), + rocm_core::unix_time_millis() + )); + let _ = fs::remove_dir_all(&root); + fs::create_dir_all(&root).expect("failed to create the test root"); + let doomed = root.join("rocm"); + fs::write(&doomed, b"binary").expect("failed to seed the file the plan removes"); + ( + root, + UninstallPlan { + actions: vec![UninstallPlanEntry { + kind: "binary", + path: doomed, + }], + skipped: Vec::new(), + warnings: Vec::new(), + }, + ) + } + + #[test] + fn a_service_that_cannot_be_stopped_leaves_every_planned_path_in_place() { + // The EAI-8014 guarantee itself: while a managed server may still be + // serving, uninstall removes NOTHING — not the binaries, not the service + // records — so `rocm services stop` is still there to recover with. + let (root, plan) = plan_removing_one_file("stop-unconfirmed"); + let doomed = plan.actions[0].path.clone(); + + let error = stop_managed_services_then_remove(&plan, || { + Ok(ManagedServiceStopReport { + stopped: Vec::new(), + failed: vec![FailedManagedServiceStop { + service_id: "svc-stuck".to_owned(), + reason: "still \"ready\" after the stop attempt".to_owned(), + remedy: StopFailureRemedy::StopTheService, + }], + warnings: Vec::new(), + }) + }) + .expect_err("an unconfirmed stop must abort uninstall"); + + let message = format!("{error:#}"); + // Assert on text the GATE owns first. `svc-stuck` alone would be this + // test reading back its own input — the id is folded into the detail + // whatever the remedy is, so that assertion survives swapping the + // remedy to the daemon variant and would leave this branch of the + // remedy selection unexercised while reading as its coverage. + assert!( + message.contains("rocm services stop --yes"), + "the abort must carry the service remedy, not a generic one: {message}" + ); + assert!( + !message.contains("restarts managed services on its own"), + "a service is not the background helper, so its remedy must not be offered: {message}" + ); + assert!( + message.contains("svc-stuck"), + "the abort still names which service: {message}" + ); + assert!( + doomed.is_file(), + "the planned path must survive an aborted uninstall" + ); + let _ = fs::remove_dir_all(root); + } + + #[test] + fn a_background_helper_that_cannot_be_stopped_leaves_every_planned_path_in_place() { + // The daemon is the one failure class that is not a service: it restarts + // managed servers on its own, so removing the tooling while it is up + // recreates EAI-8014 from the other end. When its identity cannot be + // verified it is deliberately left running, which must abort the + // uninstall rather than quietly proceed. + let (root, plan) = plan_removing_one_file("daemon-unconfirmed"); + let doomed = plan.actions[0].path.clone(); + + let error = stop_managed_services_then_remove(&plan, || { + Ok(ManagedServiceStopReport { + stopped: Vec::new(), + failed: vec![FailedManagedServiceStop { + service_id: "rocmd (pid 4321)".to_owned(), + reason: "the background helper's identity could not be verified".to_owned(), + remedy: StopFailureRemedy::StopTheDaemon, + }], + warnings: Vec::new(), + }) + }) + .expect_err("an unstopped background helper must abort uninstall"); + + let message = format!("{error:#}"); + // Assert on text the GATE owns, not on the strings this test handed it. + // The service id and reason above are folded into the detail whatever + // the remedy is, so asserting they appear only proves the input reached + // the output — it passes with the remedy swapped to the service variant, + // which would leave this remedy branch unexercised while reading as its + // coverage. The daemon remedy's own sentence is what distinguishes it. + assert!( + message.contains("restarts managed services on its own"), + "the abort must carry the daemon remedy, not a generic one: {message}" + ); + assert!( + !message.contains("rocm services stop --yes"), + "the daemon is not a service, so the service remedy must not be offered: {message}" + ); + assert!( + message.contains("rocmd (pid 4321)"), + "the abort still names which helper: {message}" + ); + assert!( + doomed.is_file(), + "the planned path must survive an aborted uninstall" + ); + let _ = fs::remove_dir_all(root); + } + + #[test] + fn a_stop_pass_that_cannot_run_leaves_every_planned_path_in_place() { + // Discovery failing (an unreadable services directory) is the same class + // of danger as a stop failing: uninstall would be removing the tooling + // while blind to what it manages. + let (root, plan) = plan_removing_one_file("stop-undiscoverable"); + let doomed = plan.actions[0].path.clone(); + + let error = stop_managed_services_then_remove(&plan, || { + bail!("failed to read the services directory") + }) + .expect_err("a stop pass that cannot run must abort uninstall"); + + // Unlike the two remedy tests above, there is no gate-owned text to + // assert on here: a discovery failure never reaches the gate, it is + // propagated by the `?` on the stop pass. So this substring is + // deliberately the test's own input, and it pins exactly one thing — + // that the cause is carried through rather than swallowed or replaced + // by a generic "uninstall aborted", which is the `.ok()` defect an + // earlier round found on this path. The survival assertion below is + // what pins the abort itself. + assert!( + error.to_string().contains("services directory"), + "the abort carries the discovery failure verbatim: {error:#}" + ); + assert!( + doomed.is_file(), + "the planned path must survive an aborted uninstall" + ); + let _ = fs::remove_dir_all(root); + } + + // The bounded-exit assertion and its budget are shared with the crate root's + // test module rather than copied here. Two near-identical copies, carrying + // near-identical comments, is exactly what made it easy to believe both + // process-based tests were bounded when only one of them actually was. + #[cfg(target_os = "linux")] + use crate::tests::assert_stopped_within_grace; + + /// Linux-only: needs a real spawned stand-in and `process_start_ticks`. + #[cfg(target_os = "linux")] + #[test] + fn a_cache_only_uninstall_leaves_a_live_server_running() { + // The other direction of the recovery-tooling predicate, pinned through + // the command rather than on the predicate alone. `--keep-binaries + // --keep-data` leaves `rocm services stop` and every service record in + // place, so there is nothing to protect by force-stopping a live server + // — and doing it anyway would kill a user's running model for a run that + // only clears a cache. + // + // Unit tests already assert `plan_removes_recovery_tooling` is false for + // a cache-only plan, but nothing checked the wiring: forcing that + // predicate true left every other test green, because they all uninstall + // something that *does* remove the tooling. + let root = std::env::temp_dir().join(format!( + "rocm-cli-uninstall-cache-only-{}-{}", + std::process::id(), + rocm_core::unix_time_millis() + )); + let _ = fs::remove_dir_all(&root); + let paths = rocm_core::AppPaths { + config_dir: root.join("config"), + data_dir: root.join("data"), + cache_dir: root.join("cache"), + }; + paths.ensure().expect("create the app directories"); + fs::write(paths.cache_dir.join("blob"), b"cached").expect("seed a cache file"); + + let mut server = std::process::Command::new("sleep") + .arg("60") + .spawn() + .expect("failed to spawn the managed server"); + let pid = server.id(); + let mut record = rocm_core::ManagedServiceRecord::new( + &paths, + "svc-cache-only", + "vllm", + "m", + "m", + "127.0.0.1", + UNSERVABLE_PORT, + "managed", + pid, + None, + None, + None, + ); + record.engine_pid = Some(pid); + record.supervisor_start_ticks = rocm_core::process_start_ticks(pid); + record.status = "ready".to_owned(); + record.write().expect("failed to write the service record"); + + super::uninstall_with_paths( + &paths, + &crate::UninstallOptions { + yes: true, + keep_binaries: true, + keep_data: true, + ..crate::UninstallOptions::default() + }, + ) + .expect("a cache-only uninstall must succeed"); + + assert!( + server.try_wait().expect("poll the server").is_none(), + "a cache-only uninstall must not stop a live managed server" + ); + assert!( + record.manifest_path.is_file(), + "a cache-only uninstall keeps the service records" + ); + let _ = server.kill(); + let _ = server.wait(); + let _ = fs::remove_dir_all(root); + } + + #[test] + fn removal_proceeds_once_every_managed_server_is_confirmed_stopped() { + let (root, plan) = plan_removing_one_file("stop-confirmed"); + let doomed = plan.actions[0].path.clone(); + + stop_managed_services_then_remove(&plan, || { + Ok(ManagedServiceStopReport { + stopped: vec!["svc-stopped".to_owned()], + failed: Vec::new(), + warnings: Vec::new(), + }) + }) + .expect("a confirmed stop must let uninstall proceed"); + + assert!( + !doomed.exists(), + "the planned path must be removed once nothing is left serving" + ); + let _ = fs::remove_dir_all(root); + } + + /// Linux-only: relies on `process_start_ticks` (`Some` only on Linux) and on + /// the zombie-state detection in `terminate_verified`. + #[cfg(target_os = "linux")] + #[test] + fn a_live_managed_server_is_stopped_before_the_planned_paths_are_removed() { + // The whole ordering, driven through the real stop pass rather than an + // injected report: a live server dies first, and only then do the files + // go. + // + // The mutation this is sensitive to is the ordering inversion — running + // the removal loop before the stop pass. That only bites because the + // plan below also removes the *services directory*: remove-first deletes + // the record, so the stop pass then finds nothing to stop, the stand-in + // survives its deadline and the assertion fails. With the plan removing + // only an unrelated file (as it did originally) the record survived the + // inversion, the stop still ran, and this test passed under exactly the + // defect it claims to guard. + let (root, mut plan) = plan_removing_one_file("live-server"); + let doomed = plan.actions[0].path.clone(); + let paths = rocm_core::AppPaths { + config_dir: root.join("config"), + data_dir: root.join("data"), + cache_dir: root.join("cache"), + }; + plan.actions.push(crate::UninstallPlanEntry { + kind: "data", + path: paths.data_dir.join("services"), + }); + let child = std::process::Command::new("sleep") + .arg("60") + .spawn() + .expect("failed to spawn the managed server"); + let pid = child.id(); + let free_port = UNSERVABLE_PORT; + let mut record = rocm_core::ManagedServiceRecord::new( + &paths, + "svc-live", + "vllm", + "m", + "m", + "127.0.0.1", + free_port, + "managed", + pid, + None, + None, + None, + ); + record.engine_pid = Some(pid); + record.supervisor_start_ticks = rocm_core::process_start_ticks(pid); + record.status = "ready".to_owned(); + record.write().expect("failed to write the service record"); + + stop_managed_services_then_remove(&plan, || { + crate::uninstall_gate::stop_managed_services_before_uninstall(&paths) + }) + .expect("a server that stops must not block uninstall"); + + // The stand-in is our own child, so its exit is observable directly — + // and bounded, so this pins that uninstall killed it rather than that it + // outlived the test. + let mut child = child; + assert_stopped_within_grace( + &mut child, + "the managed server must be stopped by uninstall", + ); + assert!( + !rocm_core::process_is_running(pid), + "the managed server must be stopped by uninstall" + ); + assert!( + !doomed.exists(), + "the planned path must be removed once the server is stopped" + ); + let _ = fs::remove_dir_all(root); + } + + /// Linux-only for the same reason as the test above: `process_start_ticks` + /// and zombie-state detection. + #[cfg(target_os = "linux")] + #[test] + fn the_uninstall_command_itself_stops_a_managed_server_before_removing_anything() { + // Drives the real command end to end — plan, confirm gate, stop pass, + // removal — rather than the pieces separately. + // + // Two distinct claims, and it is worth being exact about which is which, + // because an earlier version of this comment claimed both and only + // delivered one. ORDERING: inlining the old remove-first loop back into + // `uninstall_with_paths` fails here even though every narrower test + // still passes. STOPPING: the assertion below is bounded, so skipping + // the stop pass fails it instead of merely making the suite take as long + // as the stand-in lives. + let root = std::env::temp_dir().join(format!( + "rocm-cli-uninstall-cmd-test-{}-{}", + std::process::id(), + rocm_core::unix_time_millis() + )); + let _ = fs::remove_dir_all(&root); + let paths = rocm_core::AppPaths { + config_dir: root.join("config"), + data_dir: root.join("data"), + cache_dir: root.join("cache"), + }; + for dir in [&paths.config_dir, &paths.data_dir, &paths.cache_dir] { + fs::create_dir_all(dir).expect("seed an isolated dir the plan will remove"); + } + let marker = paths.data_dir.join("state.json"); + fs::write(&marker, b"{}").expect("seed state the removal must delete"); + + let server = std::process::Command::new("sleep") + .arg("60") + .spawn() + .expect("spawn the managed server"); + let pid = server.id(); + let free_port = UNSERVABLE_PORT; + let mut record = rocm_core::ManagedServiceRecord::new( + &paths, + "svc-cmd-live", + "vllm", + "m", + "m", + "127.0.0.1", + free_port, + "managed", + pid, + None, + None, + None, + ); + record.engine_pid = Some(pid); + record.supervisor_start_ticks = rocm_core::process_start_ticks(pid); + record.status = "ready".to_owned(); + record.write().expect("write the service record"); + + // `--keep-binaries` keeps the test off the real executable-discovery + // path; removing the data dir still takes the service records away, so + // the stop pass is required to run. + super::uninstall_with_paths( + &paths, + &crate::UninstallOptions { + yes: true, + keep_binaries: true, + ..crate::UninstallOptions::default() + }, + ) + .expect("uninstall should succeed once the server is stopped"); + + let mut server = server; + assert_stopped_within_grace( + &mut server, + "`rocm uninstall` must stop the server it manages", + ); + assert!( + !rocm_core::process_is_running(pid), + "`rocm uninstall` must stop the server it manages" + ); + assert!( + !marker.exists(), + "`rocm uninstall` must remove the planned state once the server is stopped" + ); + let _ = fs::remove_dir_all(root); + } +} diff --git a/apps/rocm/src/uninstall_gate.rs b/apps/rocm/src/uninstall_gate.rs new file mode 100644 index 000000000..0a2a72c51 --- /dev/null +++ b/apps/rocm/src/uninstall_gate.rs @@ -0,0 +1,905 @@ +// Copyright © Advanced Micro Devices, Inc., or its affiliates. +// +// SPDX-License-Identifier: MIT + +//! The stop-before-remove gate behind `rocm uninstall` (EAI-8014). +//! +//! Owns the types describing what a stop pass managed to do and the functions +//! that stop the background helper and every live managed service, classify +//! what is still answering afterwards, and decide whether removal may proceed. +//! The command handler in `uninstall.rs` calls in; the shared +//! `UninstallOptions`/`UninstallPlan` types stay in the crate root. + +use anyhow::{Context, Result, bail}; +use rocm_core::{AppPaths, AutomationRuntimeState, ManagedServiceRecord}; +use std::path::PathBuf; +use std::time::Duration; + +use crate::*; + +/// A managed service uninstall could not confirm stopped, and why. +/// +/// The reason is carried rather than discarded so the abort message says what +/// went wrong — an operator facing "could not stop svc-x" with no cause has +/// nothing to act on. +#[derive(Debug)] +pub(crate) struct FailedManagedServiceStop { + pub(crate) service_id: String, + pub(crate) reason: String, + pub(crate) remedy: StopFailureRemedy, +} + +/// What will actually clear a failed stop. +/// +/// Not cosmetic. This gate refuses to remove anything while a stop is +/// unconfirmed, so the advice it prints is the operator's only way out, and +/// advice that cannot work turns the refusal into a dead end: `rocm services +/// stop` re-reads the same unparseable JSON and fails the same way, so a record +/// that will not parse would abort every retry identically. Each failure class +/// carries the remedy that can actually clear it. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum StopFailureRemedy { + /// The record parses and its processes are still there, so `rocm services + /// stop` can act on it. + StopTheService, + /// The recorded processes are gone but the endpoint still serves — an engine + /// grandchild outlived its supervisor. `rocm services stop` has nothing left + /// to kill, so the process holding the port has to be found and stopped. + StopWhatHoldsThePort, + /// The record does not parse, so no `rocm` command can act on it — the file + /// itself has to be repaired or removed. + RepairTheRecord, + /// The background helper is still alive, so it can restart what was just + /// stopped. It has to go before anything is removed. + StopTheDaemon, + /// The helper's runtime-state file does not parse, so no pid was ever + /// recovered from it — "kill that pid" is advice nobody can act on. The file + /// itself has to be repaired or deleted, so the remedy names it. + RepairTheDaemonState, +} + +impl StopFailureRemedy { + /// The recovery advice for this class, naming `ids` where the advice is + /// useless without them. + /// + /// Exhaustive on purpose, like [`StoppedRecordVerdict::blocks`]: a new + /// variant must get advice, because the gate's whole contract is that its + /// advice is the operator's only way out. + pub(crate) fn advice(self, ids: &[String]) -> String { + match self { + Self::StopTheService => "Stop them with `rocm services stop --yes`, then re-run \ + uninstall. A server started by another user, or one wedged \ + in the kernel, needs elevated privileges or a manual kill \ + first." + .to_owned(), + Self::StopWhatHoldsThePort => format!( + "Every process recorded for {} is gone, yet the endpoint still answers — the \ + engine outlived its supervisor, so `rocm services stop` has nothing left to \ + kill. Find what holds that port (`ss -ltnp` on Linux, `Get-NetTCPConnection \ + -LocalPort ` on Windows), stop it, then re-run uninstall.", + ids.join(", ") + ), + Self::StopTheDaemon => "The background helper restarts managed services on its own, \ + so it has to be stopped before uninstall can safely remove \ + anything: kill that pid, then re-run uninstall." + .to_owned(), + Self::RepairTheDaemonState => format!( + "The background helper's runtime state does not parse, so no pid could be read \ + from it and `rocm` cannot tell whether the helper is running. Check for a live \ + `rocmd` process and stop it, then repair or delete the file and re-run \ + uninstall: {}.", + ids.join(", ") + ), + Self::RepairTheRecord => format!( + "No `rocm` command can act on an unparseable record, so these have to be handled \ + on disk: check whether the server each one describes is still running (`rocm \ + services list` skips them), stop it, then repair or delete the file and re-run \ + uninstall: {}.", + ids.join(", ") + ), + } + } + + /// Where this class sits in the printed advice. + /// + /// Also exhaustive, and for a second reason beyond ordering: without it, a + /// new variant could compile an `advice()` arm and still never be printed, + /// because nothing would have added it to the list of classes to walk. + /// Ranking every variant means the set that gets advice is derived from the + /// failures themselves rather than from a list somebody has to remember to + /// extend. Most actionable first; the two "repair a file by hand" classes + /// last. + pub(crate) const fn advice_rank(self) -> u8 { + match self { + Self::StopTheService => 0, + Self::StopWhatHoldsThePort => 1, + Self::StopTheDaemon => 2, + Self::RepairTheDaemonState => 3, + Self::RepairTheRecord => 4, + } + } +} + +/// How long the gate waits for a still-listening endpoint to say what it serves. +/// +/// Only reached when something already answered a TCP connect, and only for +/// records that were already stopped, so it costs nothing on the normal path. +pub(crate) const ENDPOINT_IDENTITY_PROBE_TIMEOUT: Duration = Duration::from_secs(2); + +/// What [`stop_managed_services_before_uninstall`] managed to do, so the caller +/// can report the services it stopped and refuse to proceed while any is still +/// alive. +#[derive(Debug, Default)] +pub(crate) struct ManagedServiceStopReport { + /// Services confirmed stopped (every recorded process observed gone). + pub(crate) stopped: Vec, + /// Services that could not be confirmed stopped — a still-serving endpoint, + /// an unverifiable process, or a record too corrupt to locate one. Uninstall + /// must abort rather than remove the tooling that stops them. + pub(crate) failed: Vec, + /// Every place this pass decided to proceed without proving the port was + /// free, for the caller to put on stderr. + /// + /// These are values rather than `eprintln!`s so a test can assert on them. + /// While they were printed in place, the disclosure that keeps each + /// fail-open a tradeoff rather than a silent removal was pinned by nothing: + /// emptying a warning body left every test green while changing what a + /// destructive command tells its operator. Returning them makes the + /// disclosure part of this function's answer, and `stderr` the caller's + /// business. + pub(crate) warnings: Vec, +} + +/// The failure recorded when the background helper is live but cannot be proven +/// to be `rocmd`, so it was deliberately left alone. +pub(crate) fn daemon_identity_unverified(daemon_pid: u32) -> FailedManagedServiceStop { + FailedManagedServiceStop { + service_id: format!("rocmd (pid {daemon_pid})"), + reason: "the background helper's identity could not be verified, so it was left running \ + rather than risk signalling an unrelated process that inherited its pid" + .to_owned(), + remedy: StopFailureRemedy::StopTheDaemon, + } +} + +/// What the gate does with the daemon's recorded pid once its identity has been +/// read back. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum DaemonIdentityOutcome { + /// The pid is free or belongs to someone else: nothing of ours to stop. + NothingOfOurs, + /// Could not be proven ours, so it is neither killed nor ignored: abort. + Unverifiable, + /// Verified as the daemon: stop it. + Stop, +} + +/// Map an identity reading to the gate's action. Pure so every input is pinned +/// by a test: folding `Indeterminate` into `NothingOfOurs` would remove the +/// tooling while a live `rocmd` keeps respawning services. +pub(crate) const fn daemon_identity_outcome( + state: rocm_core::IdentityState, + record_predates_start_ticks: bool, +) -> DaemonIdentityOutcome { + match state { + rocm_core::IdentityState::Gone | rocm_core::IdentityState::Recycled => { + DaemonIdentityOutcome::NothingOfOurs + } + rocm_core::IdentityState::Indeterminate => DaemonIdentityOutcome::Unverifiable, + rocm_core::IdentityState::Matches if record_predates_start_ticks => { + DaemonIdentityOutcome::Unverifiable + } + rocm_core::IdentityState::Matches => DaemonIdentityOutcome::Stop, + } +} + +/// Stop the background helper before uninstall stops the services it supervises. +/// +/// `rocmd` recovers managed services: a `ready`/`running` record whose endpoint +/// is unreachable is treated as recoverable and respawned +/// (`endpoint_status_unreachable`). That is precisely the state the stop pass +/// creates — the engine is killed, and the record still says `ready` until +/// `stop_internal_managed_service` writes it back. A daemon polling in that +/// window brings the server straight back, after which uninstall would delete +/// the binaries and every service record while a brand-new engine holds the GPU. +/// Stopping the supervisor first closes the window instead of racing it. +/// +/// A daemon that cannot be confirmed stopped is a blocking failure for the same +/// reason a service is: it can resurrect a server after the tooling is gone. +/// +/// It is never killed on the recorded pid alone. `runtime-state.json` outlives a +/// crash, OOM-kill or reboot with `running` still true, so that pid can name an +/// unrelated process — and this call site signals a whole tree with `force`. +/// Only a pid whose recorded start-time still matches is signalled; a recycled +/// one means the daemon is already gone (nothing to stop), and one that can be +/// neither confirmed nor refuted is left alone and reported as a failure, which +/// aborts the uninstall with the tooling intact. Being told to kill a pid is +/// recoverable; having an unrelated process tree killed is not. +/// +/// A record whose `running` is false is not signalled at all. Only rocmd's clean +/// shutdown writes that flag, on its way out, so the pid in such a record names +/// a process that has already exited and anything live under that number +/// inherited it. This is the same inactive contract `background_helper_already_running` +/// spawns on, and off Linux it is the only one that applies. +/// +/// Linux (`/proc`) and Windows (`GetProcessTimes` creation time) can record and +/// compare a start-time, so a recycled pid is told apart there. On a platform +/// with neither (macOS) no start-time exists, so this degrades to the same +/// best-effort match the managed-service kills already use there rather than +/// making uninstall unusable whenever the daemon is up: `running` and a live-pid +/// check are the whole of the protection. Whether this platform can +/// read a start-time at all is asked of a process known to be alive — this one — +/// so a failed reading of the daemon's pid is never mistaken for a platform that +/// cannot read them. +/// +/// Two residual gaps, and the list above is otherwise the complete set of +/// inputs. First, a *missing* `runtime-state.json` is taken at face value as "no +/// daemon". Deleting that file by hand while `rocmd` is live therefore skips the +/// daemon stop silently. This is deliberate — a missing file is the ordinary +/// never-started case, and there is no pid to verify or signal without it — but +/// it does mean the gate is only as good as the state file. An unreadable one is +/// the case that aborts; an absent one is the case that proceeds. Second, on +/// macOS a pid recycled while `running` was still true (a crash, not a clean +/// exit) cannot be told from the daemon itself. +pub(crate) fn stop_background_helper_before_uninstall( + paths: &AppPaths, + report: &mut ManagedServiceStopReport, +) { + // Three outcomes, not two. `load` returns `Ok(None)` only when there is no + // state file at all; a permission error or a half-written file returns + // `Err`, and discarding that would skip the daemon stop silently and let + // uninstall delete the tooling while a live `rocmd` respawns what the + // service pass just stopped — the exact defect this gate exists to close. + // The service side already treats an unparseable record as a hard failure + // (see `unreadable_service_manifests`); this is the same call. + let state = match AutomationRuntimeState::load(paths) { + Ok(Some(state)) => state, + Ok(None) => return, + Err(error) => { + // Name the file, not a pid: nothing parsed, so no pid was ever + // recovered and "kill that pid" would be advice nobody can follow. + // Repairing or deleting this file is the only action that clears it, + // and uninstall has no `--force`, so the abort has to say so. + report.failed.push(FailedManagedServiceStop { + service_id: format!("rocmd ({})", paths.automation_state_path().display()), + reason: format!( + "the background helper's runtime state could not be read, so it cannot be \ + confirmed stopped: {error:#}" + ), + remedy: StopFailureRemedy::RepairTheDaemonState, + }); + return; + } + }; + // `running` is the same inactive contract `background_helper_already_running` + // spawns on, and honoring it here is what keeps this path off an unrelated + // process. Only the clean-shutdown path writes `running: false` — a daemon + // started without `--automations-enabled` returns before its first state + // write — so a false flag means the recorded pid belongs to a daemon that + // already exited, and anything alive under it now inherited the number. + // Where no start-time can be read (macOS) this flag is the only guard against + // a force-kill landing on a stranger. + if !state.running || state.daemon_pid == 0 || state.daemon_pid == std::process::id() { + return; + } + if !rocm_core::process_is_running(state.daemon_pid) { + return; + } + // Kill nothing this cannot identify. `terminate_verified` with `force` is a + // SIGKILL across the whole process tree, and `runtime-state.json` outlives a + // crash, OOM-kill or reboot with `running` still true — so the recorded PID + // can belong to an unrelated process by the time uninstall runs. + // + // `identity_state` maps an unrecorded start-time to `Matches` (best-effort, + // for legacy state files), which is exactly wrong here: a state file written + // by a pre-upgrade `rocmd` has no `daemon_start_ticks`, and treating that as + // a match would force-kill a whole tree at a PID this cannot prove is ours — + // the ordinary upgrade path. So when this platform *can* read a start-time + // (`/proc`) yet none was recorded, the record simply predates the field: + // treat it as unverifiable, leave the process alone, and abort. Being told + // to kill a PID is recoverable; killing an unrelated process tree is not. + // + // Only where no start-time can ever be read (macOS) does + // this fall back to the best-effort match the managed-service kills already + // use there — otherwise uninstall could never stop a live daemon on those + // platforms. That residual gap is documented on `daemon_start_ticks`. + // + // Which platform this is gets answered by a process that is definitely + // alive — this one — rather than by whether the daemon's own reading + // happened to succeed. Asking the daemon's PID cannot tell "no `/proc` on + // this OS" from "that one read just failed", and those must not be + // conflated: on Linux a legacy record whose PID was momentarily unreadable + // would otherwise answer `None` to both, land in the best-effort `Matches` + // arm meant for Windows, and force-kill a tree on a record it never + // verified. Reading our own PID has no such window — if the platform can + // report a start-time at all, it reports ours. + // + // One reading of the *daemon's* start-time then serves the identity question + // below. Reading it twice let two answers come from different observations, + // so a PID that flickered could clear one check and fail the other. + let identity = rocm_core::ProcessIdentity::new(state.daemon_pid, state.daemon_start_ticks); + let observed_start_ticks = rocm_core::process_start_ticks(state.daemon_pid); + let unverifiable_pre_upgrade_record = record_predates_start_ticks(state.daemon_start_ticks); + match daemon_identity_outcome( + rocm_core::identity_state_with_observed(&identity, observed_start_ticks), + unverifiable_pre_upgrade_record, + ) { + DaemonIdentityOutcome::NothingOfOurs => return, + DaemonIdentityOutcome::Unverifiable => { + report + .failed + .push(daemon_identity_unverified(state.daemon_pid)); + return; + } + DaemonIdentityOutcome::Stop => {} + } + println!("stopping the background helper (rocmd)"); + let outcome = rocm_core::terminate_verified( + &identity, + rocm_core::KillScope::Tree, + MANAGED_STOP_GRACE, + true, + ); + if outcome.stopped() { + report.stopped.push("rocmd (background helper)".to_owned()); + } else { + report.failed.push(FailedManagedServiceStop { + service_id: format!("rocmd (pid {})", state.daemon_pid), + reason: "the background helper could not be confirmed stopped, and it restarts \ + managed services whose endpoint stops answering" + .to_owned(), + remedy: StopFailureRemedy::StopTheDaemon, + }); + } +} + +/// Stop every live managed service before uninstall removes the binaries and +/// service records needed to stop them. +/// +/// The defect this closes (EAI-8014): uninstall reported success while a +/// publicly-bound, GPU-holding managed server kept serving, and deleted the +/// `rocm`/`rocmd` binaries and service manifests — so the supported +/// `rocm services stop` path was gone and only a manual PID kill remained. +/// +/// A service is only counted stopped when [`stop_internal_managed_service`] +/// confirms every recorded process is gone (its `status` reaches `stopped`); +/// anything else lands in `failed` so the caller aborts and keeps the tooling. +/// +/// Fail-closed on discovery too: if the services directory exists but cannot be +/// enumerated, the error propagates so uninstall aborts rather than deleting the +/// tooling while blind to what it manages. (`load_managed_services` returns an +/// empty list — not an error — when no services directory exists, so a clean +/// install still uninstalls.) A manifest that exists but does not parse is +/// likewise a failure, not a silent skip — see +/// [`unreadable_service_manifests`]. +pub(crate) fn stop_managed_services_before_uninstall( + paths: &AppPaths, +) -> Result { + stop_managed_services_with(paths, stop_internal_managed_service) +} + +/// [`stop_managed_services_before_uninstall`] with the per-service stop +/// injectable, so a test can make it fail — which no real process reliably does. +pub(crate) fn stop_managed_services_with( + paths: &AppPaths, + stop_service: impl Fn(&AppPaths, &str) -> Result, +) -> Result { + let mut report = ManagedServiceStopReport::default(); + // Everything that can doom the run is decided BEFORE anything is stopped. + // + // Stopping is not free and not undoable: each confirmed stop drops that + // service's endpoint key, and this command's own abort text says a publicly + // bound service has to be served again with an explicit flag to come back. + // Once the gate is going to abort — the tooling stays, nothing is removed — + // every stop performed on the way there is pure cost to the operator, paid + // for a removal that will not happen. So a helper stop that recorded a + // failure returns here, and an unparseable manifest (which can perfectly + // well describe a live, GPU-holding server) is collected up front rather + // than after every other service is already down. + // + // The manifest scan goes first because it is the only one of the two that + // costs nothing: it reads the services directory and signals nothing. The + // helper stop is itself destructive — it force-kills a process tree — so + // running it ahead of a read that can doom the run would terminate a live, + // perfectly verifiable `rocmd` for an uninstall that then removes nothing. + for manifest in unreadable_service_manifests(paths)? { + report.failed.push(FailedManagedServiceStop { + // The full path, not the file name: the only remedy is to act on the + // file, so the message has to say which file. + service_id: manifest.display().to_string(), + reason: + "service record could not be parsed, so its server cannot be located or stopped" + .to_owned(), + remedy: StopFailureRemedy::RepairTheRecord, + }); + } + if !report.failed.is_empty() { + return Ok(report); + } + stop_background_helper_before_uninstall(paths, &mut report); + if !report.failed.is_empty() { + return Ok(report); + } + let records = load_managed_services(paths)?; + let mut attempted: Vec<&ManagedServiceRecord> = Vec::new(); + for record in &records { + if !managed_service_is_live(record) { + continue; + } + attempted.push(record); + // Each stop waits out a bounded grace per recorded process (and the + // engine's own stop before that), so name the service first: without + // this, an uninstall with a live server reads as a hang. + println!("stopping managed service {}", record.service_id); + match stop_service(paths, &record.service_id) { + Ok(result) => { + let status = result + .get("status") + .and_then(serde_json::Value::as_str) + .unwrap_or("unknown"); + if status == "stopped" { + report.stopped.push(record.service_id.clone()); + } else { + report.failed.push(FailedManagedServiceStop { + service_id: record.service_id.clone(), + reason: format!("still \"{status}\" after the stop attempt"), + remedy: StopFailureRemedy::StopTheService, + }); + } + } + Err(error) => report.failed.push(FailedManagedServiceStop { + service_id: record.service_id.clone(), + reason: format!("{error:#}"), + remedy: StopFailureRemedy::StopTheService, + }), + } + } + // Ground the PID bookkeeping in what is actually being served. Confirming a + // stop from recorded PIDs alone is thin: on Windows the kill scope is the + // recorded process only, so an engine grandchild can outlive it and keep the + // port and the GPU while the record reads "stopped". + // + // Every record is probed, not just the ones this pass stopped. A stop + // persists `status = "stopped"` before this loop runs, so the record that + // failed the gate reads as not-live on the very next run — probing only this + // pass's own work would let the retry the abort message asks for sail + // through and remove the tooling while the survivor keeps serving. That is + // the defect this gate exists to prevent, reached by following its own + // instructions. + // + // What differs is the evidence required, because a stopped record keeps its + // manifest and its old port forever (nothing prunes them, and there is no + // `services remove`), so an unrelated process that later binds that port + // must not brick uninstall with no way out: + // + // * stopped by this pass — any listener fails the gate. We killed the + // recorded processes seconds ago; something answering there is the + // survivor. + // * already stopped before this run — a listener alone proves nothing, so + // it has to identify itself as this record's own model before it counts. + // An unrelated service on a recycled port does not, and is not blocked. + for record in &records { + if report + .failed + .iter() + .any(|failure| failure.service_id == record.service_id) + { + continue; + } + // A wildcard record yields both loopback families; whichever answers is + // the one the identity probe below has to talk to, so keep it rather + // than re-deriving a single host and guessing the family wrong. + let candidates = probe_hosts(&record.host); + let Some(reachable_host) = candidates + .iter() + .find(|candidate| loopback_tcp_port_is_reachable(candidate, record.port)) + .cloned() + else { + // Not reaching the port is the normal, silent case: nothing is + // listening, which is what a stopped service looks like. But + // `loopback_tcp_port_is_reachable` returns the same `false` when the + // address does not resolve at all, and those are opposite + // situations. A refused connect is evidence the port is free; a name + // that no longer resolves is evidence of nothing, and skipping on it + // means proceeding to delete the tooling without ever having asked + // whether a server is up. That is a fail-open, and it is disclosed. + if !candidates + .iter() + .any(|candidate| host_port_resolves(candidate, record.port)) + { + report.warnings.push(format!( + "{} is recorded for service {}, but that name does not resolve here, so \ + whether anything is still serving on port {} could not be checked at all — \ + proceeding. A record written on another machine, or under a hostname since \ + removed, looks like this. If that service may still be running, stop it \ + before re-running uninstall.", + record.host, record.service_id, record.port + )); + } + continue; + }; + let stopped_by_this_pass = attempted + .iter() + .any(|candidate| candidate.service_id == record.service_id); + if !stopped_by_this_pass { + let endpoint_api_key = endpoint_keys::endpoint_api_key(paths, &record.service_id); + // Ask the same address the reachability probe just succeeded against. + // `record.endpoint_url` is built from the recorded host, so a `0.0.0.0` + // or `::` bind would be connected to literally — which does not + // resolve, fails the probe, and takes the fail-open branch below, + // removing the tooling while a wildcard-bound engine is still + // serving. That is the defect this gate exists to close, so the + // identity probe gets the normalized host too. + let mut probe_record = record.clone(); + probe_record.endpoint_url = + rocm_core::format_http_base_url(&reachable_host, record.port); + let probe = rocm_core::managed_service_endpoint_identity( + &probe_record, + endpoint_api_key.as_deref(), + ENDPOINT_IDENTITY_PROBE_TIMEOUT, + ); + // Short-circuits: the extra round trip only happens when the probe + // produced no usable listing, which is the only case its answer can + // change. + let auth_refused = probe.is_err() + && endpoint_refused_authorization( + &probe_record.endpoint_url, + endpoint_api_key.as_deref(), + ); + // Every arm below is decided in `stopped_record_verdict` and pinned + // there by `every_identity_answer_maps_to_exactly_one_gate_outcome`, + // with `an_endpoint_listing_nothing_does_not_block_uninstall` and + // `a_listener_naming_another_model_does_not_block_uninstall` driving + // the two proceed-on-a-live-socket arms through this call site. The + // tests live at the bottom of this file, far from here; change an + // arm and expect them, not this match, to be what goes red. + let verdict = stopped_record_verdict(probe.ok(), auth_refused); + if !verdict.blocks() { + // The two fail-open outcomes disclose themselves; only + // `ProceedUnrelated` is silent, because a listener that named + // its models and did not name ours is the one case that is + // actually evidence of a stranger. They go into the report + // rather than straight to stderr so the disclosure is a value a + // test can assert on — see the field's own comment. + match verdict { + StoppedRecordVerdict::ProceedListingNothing => report.warnings.push(format!( + "{}:{} still accepts connections and answered the identity probe with an \ + empty model list, so it cannot be told from an unrelated service; {} is \ + already recorded stopped — proceeding. An engine still loading or \ + unloading looks like this. If that is a server of yours, stop whatever \ + holds that port first.", + record.host, record.port, record.service_id + )), + StoppedRecordVerdict::ProceedUnidentified => report.warnings.push(format!( + "{}:{} still accepts connections but did not answer the identity probe \ + with a usable model list, and service {} is already recorded stopped — \ + proceeding. That can be a wedged engine, an unrelated server on the \ + port, or a stale endpoint key. If it is a server of yours, stop whatever \ + holds that port first.", + record.host, record.port, record.service_id + )), + StoppedRecordVerdict::ProceedUnrelated => {} + StoppedRecordVerdict::BlockServingOurModel + | StoppedRecordVerdict::BlockAuthRefused => unreachable!("guarded by blocks()"), + } + continue; + } + if verdict == StoppedRecordVerdict::BlockAuthRefused { + report.failed.push(FailedManagedServiceStop { + service_id: record.service_id.clone(), + reason: format!( + "{}:{} refused the identity probe's credentials, so an authenticated \ + server is still serving there", + record.host, record.port + ), + remedy: StopFailureRemedy::StopWhatHoldsThePort, + }); + continue; + } + } + report + .stopped + .retain(|stopped| stopped != &record.service_id); + report.failed.push(FailedManagedServiceStop { + service_id: record.service_id.clone(), + // Two different situations reach here and they need different + // sentences. Only one of them involved a stop: the other is a + // record that was already marked stopped before this run, which + // this pass never attempted to stop, and telling its operator the + // endpoint survived "the stop" describes something that did not + // happen — on the one output they have to reason from. + reason: if stopped_by_this_pass { + format!( + "{}:{} still accepts connections after the stop", + record.host, record.port + ) + } else { + format!( + "{}:{} is recorded stopped, but something there is still serving this \ + record's own model", + record.host, record.port + ) + }, + // Not `StopTheService`: the recorded processes are gone, so + // `rocm services stop` has nothing left to kill and every retry + // would abort identically. + remedy: StopFailureRemedy::StopWhatHoldsThePort, + }); + } + Ok(report) +} + +/// The addresses to probe for a service recorded on `host`, in order. +/// +/// A service bound to a wildcard address is reachable on loopback; connecting to +/// the wildcard itself is not portable. Which loopback, though, depends on what +/// the listener actually bound: `::` with the usual `IPV6_V6ONLY=1` (the default +/// on Windows) answers on `::1` and *refuses* `127.0.0.1`, while an `0.0.0.0` +/// bind is the mirror image. Probing one family only would read a live, +/// port-holding engine as "nothing is serving" and wave the removal through — +/// the exact defect this gate exists to close — so a wildcard yields both and +/// the caller blocks if either answers. +/// +/// The normalized form is what gets probed, not just what gets classified. +/// `loopback_tcp_port_is_reachable` resolves with `(host, port)`, which rejects +/// a bracketed literal like `[::1]` and anything with stray whitespace — and a +/// resolution failure reads as "nothing is serving", which is the wrong +/// direction for a check whose whole job is to catch a surviving engine +/// grandchild. Records do carry bracketed spellings (`loopback_host_key` +/// normalizes them too) because `--host` is free-form. +pub(crate) fn probe_hosts(host: &str) -> Vec { + // Trimmed and case-folded so the spellings a record can carry — `0.0.0.0`, + // `::`, `[::]`, `0:0:0:0:0:0:0:0`, `*`, or empty — all resolve to loopback + // rather than being probed literally (a literal wildcard connect is not + // portable, and would silently read as "nothing is serving"). + let normalized = host.trim().trim_matches(['[', ']']).to_ascii_lowercase(); + match normalized.as_str() { + "0.0.0.0" | "::" | "0:0:0:0:0:0:0:0" | "*" | "" => { + vec!["127.0.0.1".to_owned(), "::1".to_owned()] + } + _ => vec![normalized], + } +} + +/// What the gate does about a listener answering on an already-stopped record's +/// recorded port. +/// +/// Lifted out of the stop pass so each outcome can be asserted directly. Two of +/// these arms once went untested because reaching them meant standing up a +/// server that answers in a particular way and then reading stderr: mutating +/// either "proceed" arm into a block, or deleting a warning, left every test +/// green while changing what a destructive command does. +/// +/// Both halves of that are closed now, so the next reader should not infer a +/// gap from the paragraph above. `every_identity_answer_maps_to_exactly_one_gate_outcome` +/// pins the mapping here; `an_endpoint_listing_nothing_does_not_block_uninstall` +/// and `a_listener_naming_another_model_does_not_block_uninstall` drive the two +/// proceed-on-a-live-socket arms through the real call site; and the warnings +/// are values on [`ManagedServiceStopReport`] rather than `eprintln!`s, so the +/// disclosure is asserted rather than merely emitted. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum StoppedRecordVerdict { + /// Serving this record's own model: the engine outlived the supervisor whose + /// death marked the record stopped. + BlockServingOurModel, + /// Refused our credentials — a live server stating it guards this path. + BlockAuthRefused, + /// Named its models and ours was not among them. The only "not ours" that is + /// evidence of anything, so the only one that proceeds silently. + ProceedUnrelated, + /// Answered, but listed nothing. Not evidence of a stranger: an engine still + /// loading or mid-unload looks exactly like this while holding the port. + ProceedListingNothing, + /// Listening but unidentifiable — not an OpenAI endpoint, an unparseable + /// reply, or a server erroring on a rotated key. + ProceedUnidentified, +} + +impl StoppedRecordVerdict { + /// Whether this outcome stops the uninstall. + /// + /// Exhaustive on purpose, rather than `matches!` over the blocking pair. + /// The wildcard that `matches!` implies would default a newly added variant + /// to "proceed" — the direction that removes the tooling — and it would + /// compile, leaving the mistake to be caught by a test that can only + /// enumerate the variants that existed when it was written. Spelled out, + /// the compiler stops the next variant until somebody decides which side of + /// a destructive command it belongs on. + pub(crate) const fn blocks(self) -> bool { + match self { + Self::BlockServingOurModel | Self::BlockAuthRefused => true, + Self::ProceedUnrelated | Self::ProceedListingNothing | Self::ProceedUnidentified => { + false + } + } + } +} + +/// Decide [`StoppedRecordVerdict`] from what the identity probe answered. +/// +/// `identity` is `None` when the probe produced no usable model list at all; +/// `auth_refused` then says whether that was a 401/403 from a live server, which +/// is stronger evidence the port is held than a listing would be. +pub(crate) const fn stopped_record_verdict( + identity: Option, + auth_refused: bool, +) -> StoppedRecordVerdict { + match identity { + Some(rocm_core::EndpointIdentity::ServesExpectedModel) => { + StoppedRecordVerdict::BlockServingOurModel + } + Some(rocm_core::EndpointIdentity::ServesOtherModels) => { + StoppedRecordVerdict::ProceedUnrelated + } + Some(rocm_core::EndpointIdentity::ListsNoModels) => { + StoppedRecordVerdict::ProceedListingNothing + } + None if auth_refused => StoppedRecordVerdict::BlockAuthRefused, + None => StoppedRecordVerdict::ProceedUnidentified, + } +} + +/// Whether a record carrying no start-time predates the field, as opposed to +/// coming from a platform that cannot report one. +/// +/// Deliberately takes no PID, and that is the whole point of it being a function +/// rather than two lines at the call site. The question is about the *platform*, +/// and asking it of the process under inspection cannot tell "this OS has no +/// `/proc`" from "that one read just failed" — a conflation that sends a legacy +/// record down the best-effort `Matches` arm and force-kills a tree it never +/// verified. Answering from our own PID has no such window: the process asking +/// is, by construction, running. Keeping the target PID out of the signature +/// makes that conflation unrepresentable here rather than merely avoided. +pub(crate) fn record_predates_start_ticks(recorded_start_ticks: Option) -> bool { + recorded_start_ticks.is_none() && rocm_core::process_start_ticks(std::process::id()).is_some() +} + +/// Whether `endpoint_url` answered the identity probe with an auth refusal. +/// +/// A 401/403 is not a failure to reach the endpoint — it is a live HTTP server +/// stating that it guards this path, which is stronger evidence that the port is +/// still held than a model listing would be. It is also the shape the retry run +/// takes for a public service: stopping the recorded processes clears the stored +/// key, so the next `rocm uninstall` probes an authenticated survivor with no +/// credentials and gets exactly this. +/// +/// Anything else — unreachable, a timeout, a non-HTTP listener, a 200 whose body +/// did not parse — is not an answer this can act on, and stays with the caller's +/// fail-open. +pub(crate) fn endpoint_refused_authorization( + endpoint_url: &str, + endpoint_api_key: Option<&str>, +) -> bool { + matches!( + rocm_core::http_get_with_auth( + endpoint_url, + "/v1/models", + endpoint_api_key, + ENDPOINT_IDENTITY_PROBE_TIMEOUT, + ), + Ok(parts) if parts.status == 401 || parts.status == 403 + ) +} + +/// Service manifests that exist but cannot be parsed back into a record. +/// +/// [`load_managed_services`] skips these silently, which is right for listing — +/// one bad file should not break `rocm services list` — but wrong for uninstall: +/// a corrupt manifest can describe a live, GPU-holding server, and removing the +/// tooling while blind to it is the exact defect this gate closes. Returns the +/// file names so the abort message can point at what to inspect. +/// +/// Only `*.json` directly under the services directory is a manifest; engine +/// state files live under their own engine directory +/// ([`AppPaths::service_engine_state_path`]), so they are not misread as corrupt +/// records here. +pub(crate) fn unreadable_service_manifests(paths: &AppPaths) -> Result> { + let services_dir = paths.services_dir(); + if !services_dir.is_dir() { + return Ok(Vec::new()); + } + let mut unreadable = Vec::new(); + for entry in fs::read_dir(&services_dir) + .with_context(|| format!("failed to read {}", services_dir.display()))? + { + let path = entry?.path(); + if path.extension().and_then(|value| value.to_str()) != Some("json") { + continue; + } + let bytes = + fs::read(&path).with_context(|| format!("failed to read {}", path.display()))?; + if serde_json::from_slice::(&bytes).is_err() { + unreadable.push(path); + } + } + unreadable.sort(); + Ok(unreadable) +} + +/// Decide whether uninstall may proceed to remove files, given the outcome of +/// stopping managed services. +/// +/// Returns an optional line to print before removal proceeds, or an error that +/// aborts uninstall with nothing removed when a service could not be stopped — +/// so the binaries and service records needed to recover stay in place. Kept +/// separate from the removal it guards so the abort branch (the core safety +/// guarantee) is unit-testable without an unkillable process. +pub(crate) fn uninstall_removal_gate(report: &ManagedServiceStopReport) -> Result> { + if !report.failed.is_empty() { + let detail = report + .failed + .iter() + .map(|failure| format!("{} ({})", failure.service_id, failure.reason)) + .collect::>() + .join("; "); + // Say what the aborted run already did. The stop pass is not atomic: a + // service stopped before the failing one is down for good, and stopping + // it dropped its endpoint key, which cannot be re-minted — a public + // service must be served again with `--allow-public-bind` to come back. + // "No files were removed" alone would read as "nothing happened". + let already_stopped = if report.stopped.is_empty() { + String::new() + } else { + format!( + " No files were removed, but these services were stopped before the failure and \ + stay stopped: {}. Stopping them dropped their endpoint keys, so a publicly bound \ + one has to be served again with `rocm serve --allow-public-bind` to return.", + report.stopped.join(", ") + ) + }; + // Advice per failure class. A record that will not parse cannot be + // stopped by `rocm services stop` — that command loads the same file and + // fails identically — so pointing at it would make every retry abort the + // same way, which is exactly the dead end this gate must not create. + // + // The classes walked here are derived from the failures themselves and + // ordered by `advice_rank`, so no variant can be dropped by forgetting + // to extend a list; both that and `advice` are exhaustive matches. + let mut classes = report + .failed + .iter() + .map(|failure| failure.remedy) + .collect::>(); + classes.sort_unstable_by_key(|remedy| remedy.advice_rank()); + classes.dedup(); + let remedies = classes + .into_iter() + .map(|remedy| { + let ids = report + .failed + .iter() + .filter(|failure| failure.remedy == remedy) + .map(|failure| failure.service_id.clone()) + .collect::>(); + remedy.advice(&ids) + }) + .collect::>(); + let may_be_serving = report.failed.iter().any(|failure| { + matches!( + failure.remedy, + StopFailureRemedy::StopTheService | StopFailureRemedy::StopWhatHoldsThePort + ) + }); + bail!( + "uninstall aborted: could not stop managed service(s): {detail}.{} {}{}", + if may_be_serving { + " Their endpoints may still be serving and holding the GPU." + } else { + "" + }, + remedies.join(" "), + if already_stopped.is_empty() { + " No files were removed." + } else { + &already_stopped + } + ); + } + if report.stopped.is_empty() { + return Ok(None); + } + Ok(Some(format!( + "stopped {} managed service(s) before removal", + report.stopped.len() + ))) +} diff --git a/apps/rocmd/Cargo.toml b/apps/rocmd/Cargo.toml index 5cfd3296c..d88c3926a 100644 --- a/apps/rocmd/Cargo.toml +++ b/apps/rocmd/Cargo.toml @@ -21,6 +21,3 @@ serde_json.workspace = true sha2 = "0.10" tokio.workspace = true ureq = { version = "2.12", features = ["native-certs"] } - -[target.'cfg(windows)'.dependencies] -windows-sys = { version = "0.61", features = ["Win32_Storage_FileSystem"] } diff --git a/apps/rocmd/src/lib.rs b/apps/rocmd/src/lib.rs index ee107a0c4..d2524669b 100644 --- a/apps/rocmd/src/lib.rs +++ b/apps/rocmd/src/lib.rs @@ -1056,6 +1056,18 @@ fn temp_sibling_path(path: &Path, suffix: &OsStr) -> Result { Ok(parent.join(file_name)) } +/// Stage-and-publish a file here, sharing only the publish step with `rocm-core`. +/// +/// Deliberately not [`rocm_core::write_file_atomically`], and not a copy of it +/// either: only the Windows-sensitive publish (`ReplaceFileW` and its fallbacks) +/// is single-sourced, via [`publish_temp_file`]. The staging half stays local +/// because it carries the `suffix_for_attempt` and `before_publish` seams the +/// tests below drive to force temp-name collisions, write failures and rename +/// races — injection points `rocm_core`'s caller-facing helper does not expose. +/// +/// Consequence worth knowing: this path does **not** `sync_all` before +/// publishing, so unlike the `rocm-core` helper it is atomic but carries no +/// crash-durability guarantee for the staged bytes. fn write_file_atomically(path: &Path, bytes: &[u8]) -> Result<()> { let temp_id = format!("{}-{}", std::process::id(), unix_time_millis()); write_file_atomically_with( @@ -1141,60 +1153,10 @@ where Ok(()) } -#[cfg(not(windows))] -fn publish_temp_file(tmp: &Path, path: &Path) -> io::Result<()> { - fs::rename(tmp, path) -} - -#[cfg(windows)] +/// The publish step lives in `rocm-core` so there is one implementation of the +/// Windows `ReplaceFileW` handling for the whole workspace. fn publish_temp_file(tmp: &Path, path: &Path) -> io::Result<()> { - if path.try_exists()? { - return replace_file_windows(path, tmp); - } - - match fs::rename(tmp, path) { - Ok(()) => Ok(()), - Err(rename_error) => { - if path.try_exists()? { - replace_file_windows(path, tmp) - } else { - Err(rename_error) - } - } - } -} - -#[cfg(windows)] -#[allow(unsafe_code)] -fn replace_file_windows(path: &Path, replacement: &Path) -> io::Result<()> { - use std::os::windows::ffi::OsStrExt; - use windows_sys::Win32::Storage::FileSystem::ReplaceFileW; - - let path_wide: Vec = path.as_os_str().encode_wide().chain(Some(0)).collect(); - let replacement_wide: Vec = replacement - .as_os_str() - .encode_wide() - .chain(Some(0)) - .collect(); - - // SAFETY: both path buffers are valid, NUL-terminated UTF-16 strings and - // remain alive for the duration of the synchronous Windows API call. The - // optional backup, exclude, and reserved pointers are intentionally null. - let replaced = unsafe { - ReplaceFileW( - path_wide.as_ptr(), - replacement_wide.as_ptr(), - std::ptr::null(), - 0, - std::ptr::null(), - std::ptr::null(), - ) - }; - if replaced == 0 { - Err(io::Error::last_os_error()) - } else { - Ok(()) - } + rocm_core::publish_temp_file(tmp, path) } fn sandbox_check_updates_value(output: CommandCapture) -> Value { @@ -3448,6 +3410,10 @@ fn build_runtime_state( running: automations_enabled, automations_enabled, daemon_pid: std::process::id(), + // Captured here, while this process is by definition alive, so a later + // `rocm uninstall` can tell this daemon from an unrelated process that + // inherited the PID after a crash or reboot. + daemon_start_ticks: rocm_core::ProcessIdentity::capture(std::process::id()).start_ticks, started_at_unix_ms: now, last_tick_unix_ms: now, local_webhook_endpoint: None, @@ -6668,6 +6634,35 @@ mod tests { Ok(()) } + #[cfg(target_os = "linux")] + #[test] + fn a_fresh_runtime_state_records_this_daemons_start_time() { + // The capture the whole PID-recycling safety net hangs on. `rocm + // uninstall` refuses to signal a daemon pid it cannot prove is rocmd, + // and the proof is this field — so if `build_runtime_state` ever wrote + // `None` here, every record would look like a pre-upgrade one and the + // uninstall guard would degrade to best-effort without anything failing. + // The other tests in this file supply `daemon_start_ticks: None` to + // fixtures, which pins nothing about the real capture. + // + // Linux-gated because that is where a start-time is readable at all; on + // Windows and macOS `None` here is correct and expected. + let state = build_runtime_state(&RocmCliConfig::default(), true); + assert_eq!( + state.daemon_pid, + std::process::id(), + "the state must name the process that wrote it" + ); + let recorded = state + .daemon_start_ticks + .expect("a live daemon must record its own start-time on Linux"); + assert_eq!( + Some(recorded), + rocm_core::process_start_ticks(std::process::id()), + "the recorded start-time must be this process's own, not a placeholder" + ); + } + #[test] fn event_collector_emits_schedule_tick_for_due_update() -> Result<()> { let (root, paths) = temp_app_paths("event-bus-schedule"); @@ -7968,6 +7963,7 @@ mod tests { running: true, automations_enabled: true, daemon_pid: 1, + daemon_start_ticks: None, started_at_unix_ms: 1, last_tick_unix_ms: 1, local_webhook_endpoint: None, @@ -8001,6 +7997,7 @@ mod tests { running: true, automations_enabled: true, daemon_pid: 1, + daemon_start_ticks: None, started_at_unix_ms: 1, last_tick_unix_ms: 1, local_webhook_endpoint: None, @@ -8065,6 +8062,7 @@ mod tests { running: true, automations_enabled: true, daemon_pid: 1, + daemon_start_ticks: None, started_at_unix_ms: 1, last_tick_unix_ms: 1, local_webhook_endpoint: None, @@ -8154,6 +8152,7 @@ mod tests { running: true, automations_enabled: true, daemon_pid: 1, + daemon_start_ticks: None, started_at_unix_ms: 1, last_tick_unix_ms: 1, local_webhook_endpoint: None, @@ -8209,6 +8208,7 @@ mod tests { running: true, automations_enabled: true, daemon_pid: 1, + daemon_start_ticks: None, started_at_unix_ms: 1, last_tick_unix_ms: 1, local_webhook_endpoint: None, @@ -9517,6 +9517,7 @@ mod tests { running: true, automations_enabled: true, daemon_pid: 1, + daemon_start_ticks: None, started_at_unix_ms: 1, last_tick_unix_ms: 1, local_webhook_endpoint: None, diff --git a/apps/rocmd/src/persistence.rs b/apps/rocmd/src/persistence.rs index 847c92b7a..a793b19d5 100644 --- a/apps/rocmd/src/persistence.rs +++ b/apps/rocmd/src/persistence.rs @@ -91,6 +91,7 @@ mod tests { running: true, automations_enabled: true, daemon_pid: 1, + daemon_start_ticks: None, started_at_unix_ms: 1, last_tick_unix_ms: 1, local_webhook_endpoint: None, diff --git a/crates/rocm-core/Cargo.toml b/crates/rocm-core/Cargo.toml index 4c19b7821..847385b84 100644 --- a/crates/rocm-core/Cargo.toml +++ b/crates/rocm-core/Cargo.toml @@ -28,4 +28,4 @@ toml = "1.1" ureq = { version = "2.12", features = ["native-certs"] } [target.'cfg(target_os = "windows")'.dependencies] -windows-sys = { version = "0.61", features = ["Win32_Foundation", "Win32_Security", "Win32_System_Registry", "Win32_System_SystemInformation", "Win32_System_Threading"] } +windows-sys = { version = "0.61", features = ["Win32_Foundation", "Win32_Security", "Win32_Storage_FileSystem", "Win32_System_Registry", "Win32_System_SystemInformation", "Win32_System_Threading"] } diff --git a/crates/rocm-core/src/lib.rs b/crates/rocm-core/src/lib.rs index c9d096f50..7f520c40c 100644 --- a/crates/rocm-core/src/lib.rs +++ b/crates/rocm-core/src/lib.rs @@ -6,9 +6,7 @@ use anyhow::{Context, Result, bail}; use serde::{Deserialize, Serialize}; use sha2::{Digest, Sha256}; use std::collections::BTreeMap; -#[cfg(windows)] -use std::ffi::OsStr; -use std::ffi::OsString; +use std::ffi::{OsStr, OsString}; use std::fs; use std::io::{IsTerminal, Read, Write, stdin, stdout}; use std::net::{IpAddr, TcpStream, ToSocketAddrs}; @@ -53,7 +51,7 @@ pub use examine::{ pub use fix::{FixOptions, apply as apply_fix, list_recipes as list_fix_recipes}; pub use proc_lifecycle::{ IdentityState, KillScope, ProcessIdentity, TerminationOutcome, identity_state, - process_start_ticks, terminate_verified, + identity_state_with_observed, process_start_ticks, terminate_verified, }; use runtime::env_path_override; pub use runtime::{ @@ -610,6 +608,27 @@ pub fn http_get_text_with_auth( endpoint_api_key: Option<&str>, timeout: Duration, ) -> Result { + let parts = http_get_with_auth(endpoint_url, path, endpoint_api_key, timeout)?; + if parts.status != 200 { + bail!("HTTP endpoint returned HTTP {}", parts.status); + } + Ok(parts.body) +} + +/// GET `path` and return the response status alongside the body. +/// +/// The GET sibling of [`http_post_json_with_auth`], and it exists for the same +/// reason: a caller probing an endpoint has to tell "the server answered, with a +/// refusal" apart from "the server never answered", and only the former proves +/// something is alive on that port. [`http_get_text_with_auth`] collapses both +/// into `Err` — correct for callers that only want a 200's body, wrong for +/// anything deciding whether a port is still held. +pub fn http_get_with_auth( + endpoint_url: &str, + path: &str, + endpoint_api_key: Option<&str>, + timeout: Duration, +) -> Result { let deadline = Instant::now() + timeout; let (host, port) = parse_http_endpoint(endpoint_url) .with_context(|| format!("unsupported endpoint URL `{endpoint_url}`"))?; @@ -630,10 +649,12 @@ pub fn http_get_text_with_auth( .split_once("\r\n\r\n") .context("HTTP response was missing a body")?; let status_line = headers.lines().next().unwrap_or_default(); - if !status_line.contains(" 200 ") { - bail!("HTTP endpoint returned {status_line}"); - } - Ok(body.to_owned()) + let status = http_status_code(status_line) + .with_context(|| format!("unparsable HTTP status line `{status_line}`"))?; + Ok(HttpResponseParts { + status, + body: body.to_owned(), + }) } /// POST a JSON body and return the response status line plus body. @@ -866,19 +887,59 @@ pub fn openai_models_endpoint_has_model( endpoint_api_key: Option<&str>, timeout: Duration, ) -> Result { + Ok( + openai_models_endpoint_identity(endpoint_url, expected_model, endpoint_api_key, timeout)? + == EndpointIdentity::ServesExpectedModel, + ) +} + +/// What a listener said when asked which models it serves. +/// +/// The distinction [`openai_models_endpoint_has_model`] cannot draw: it answers +/// `false` both for "this is someone else's service" and for "this endpoint +/// answered, but listed nothing". For a readiness poll those are the same — not +/// ready either way — but for a caller deciding whether something still holds a +/// port they are opposites. An engine that is up but has not populated its model +/// list yet, or is mid-unload, lists nothing while very much holding the GPU. +#[derive(Debug, Clone, Copy, Eq, PartialEq)] +pub enum EndpointIdentity { + /// Listed a model matching the one asked about (or listed models when no + /// particular one was asked for). + ServesExpectedModel, + /// Listed models, none of which match — a different service on this port. + ServesOtherModels, + /// Answered `/v1/models` with an empty list. Says nothing either way about + /// whose service this is. + ListsNoModels, +} + +/// Ask a listener which models it serves, keeping "listed nothing" distinct. +pub fn openai_models_endpoint_identity( + endpoint_url: &str, + expected_model: Option<&str>, + endpoint_api_key: Option<&str>, + timeout: Duration, +) -> Result { let body = http_get_text_with_auth(endpoint_url, "/v1/models", endpoint_api_key, timeout)?; let value = serde_json::from_str::(body.trim()) .context("failed to parse /v1/models JSON")?; let loaded_models = openai_loaded_model_ids(&value); if loaded_models.is_empty() { - return Ok(false); + return Ok(EndpointIdentity::ListsNoModels); } let Some(expected_model) = expected_model.filter(|value| !value.trim().is_empty()) else { - return Ok(true); + return Ok(EndpointIdentity::ServesExpectedModel); }; - Ok(loaded_models - .iter() - .any(|loaded| model_refs_match(loaded, expected_model))) + Ok( + if loaded_models + .iter() + .any(|loaded| model_refs_match(loaded, expected_model)) + { + EndpointIdentity::ServesExpectedModel + } else { + EndpointIdentity::ServesOtherModels + }, + ) } pub fn managed_service_endpoint_model_ready( @@ -899,6 +960,29 @@ pub fn managed_service_endpoint_model_ready( openai_models_endpoint_has_model(&record.endpoint_url, expected, endpoint_api_key, timeout) } +/// [`managed_service_endpoint_model_ready`] without collapsing "listed nothing" +/// into "not ours". +/// +/// An empty `endpoint_url` is the one case with no listener to ask about at all, +/// so it stays an error rather than being reported as an identity. +pub fn managed_service_endpoint_identity( + record: &ManagedServiceRecord, + endpoint_api_key: Option<&str>, + timeout: Duration, +) -> Result { + if record.endpoint_url.trim().is_empty() { + bail!("service record has no endpoint URL to identify"); + } + let expected = if !record.canonical_model_id.trim().is_empty() { + Some(record.canonical_model_id.as_str()) + } else if !record.model_ref.trim().is_empty() { + Some(record.model_ref.as_str()) + } else { + None + }; + openai_models_endpoint_identity(&record.endpoint_url, expected, endpoint_api_key, timeout) +} + /// How far along a managed service's endpoint is. /// /// The middle state is the one that matters: an engine lists a model within @@ -6463,6 +6547,28 @@ pub struct AutomationRuntimeState { pub running: bool, pub automations_enabled: bool, pub daemon_pid: u32, + /// The daemon's kernel start-time, captured at spawn, so `daemon_pid` can be + /// checked for PID recycling before anything signals it. + /// + /// `daemon_pid` alone is not safe to kill: this file survives a crash, + /// OOM-kill or reboot with `running` still true, after which the OS can + /// reissue that PID to an unrelated process. Pairing the PID with its + /// start-time makes a recycled PID detectable ([`identity_state`]). + /// + /// `None` on two very different occasions, which callers must not conflate: + /// a state file written before this field existed, and a platform without + /// `/proc` where [`process_start_ticks`] can never return anything. + /// + /// Conflating them is a live hazard, because [`identity_state`] maps a `None` + /// recorded value to [`IdentityState::Matches`] — the legacy best-effort arm + /// — so **no recycling check happens at all** and a caller that signals on + /// that verdict is killing on a bare pid. Tell the two apart by asking the + /// platform: if [`process_start_ticks`] returns `Some` for a live pid while + /// this is `None`, the record predates the field and the pid is unverifiable; + /// if it returns `None`, no identity is obtainable here and best-effort is + /// the only option (macOS and Windows, permanently). + #[serde(default, skip_serializing_if = "Option::is_none")] + pub daemon_start_ticks: Option, pub started_at_unix_ms: u128, pub last_tick_unix_ms: u128, #[serde(default, skip_serializing_if = "Option::is_none")] @@ -6484,15 +6590,21 @@ impl AutomationRuntimeState { Ok(Some(state)) } + /// Publish the runtime state atomically. + /// + /// Written on every daemon tick, and read by `rocm uninstall` to decide + /// whether a background helper is still up. A plain `fs::write` leaves a + /// truncated file if the daemon dies mid-write, and uninstall has to treat an + /// unreadable state file as "cannot confirm the helper is stopped" — so a + /// crash at the wrong moment would block uninstall until the operator + /// repaired the file by hand. pub fn write(&self, paths: &AppPaths) -> Result<()> { paths.ensure()?; let path = paths.automation_state_path(); - fs::write( - &path, - serde_json::to_vec_pretty(self) - .context("failed to serialize automation runtime state")?, - ) - .with_context(|| format!("failed to write {}", path.display()))?; + let bytes = serde_json::to_vec_pretty(self) + .context("failed to serialize automation runtime state")?; + write_file_atomically(&path, &bytes) + .with_context(|| format!("failed to write {}", path.display()))?; Ok(()) } @@ -7921,23 +8033,25 @@ impl ManagedServiceRecord { record } + /// Persist the record, atomically. + /// + /// Staged next to the manifest and published over it, so a concurrent reader + /// sees either the old record or the new one, never a half-written file. A + /// plain overwrite left a window in which a reader (`load_managed_services`, + /// or the uninstall gate that treats an unparseable manifest as a live server + /// it cannot account for) could observe a truncated record and act on it. + /// + /// Uses the workspace's one [`write_file_atomically`] rather than a local + /// rename: this runs on every status transition, including the ones the + /// uninstall stop pass performs, where a failed publish aborts the uninstall. pub fn write(&self) -> Result<()> { let mut host_record = self.clone(); host_record.normalize_paths_for_host(); - let parent = host_record - .manifest_path - .parent() - .context("service manifest path must have a parent directory")?; - fs::create_dir_all(parent) - .with_context(|| format!("failed to create {}", parent.display()))?; let storage_record = host_record.with_storage_paths(); - fs::write( - &host_record.manifest_path, - serde_json::to_vec_pretty(&storage_record) - .context("failed to serialize service record")?, - ) - .with_context(|| format!("failed to write {}", host_record.manifest_path.display()))?; - Ok(()) + let bytes = serde_json::to_vec_pretty(&storage_record) + .context("failed to serialize service record")?; + write_file_atomically(&host_record.manifest_path, &bytes) + .with_context(|| format!("failed to write {}", host_record.manifest_path.display())) } } @@ -8209,6 +8323,178 @@ pub fn unix_time_millis() -> u128 { .as_millis() } +/// How many temp names to try before giving up on staging an atomic write. +const ATOMIC_WRITE_TEMP_ATTEMPTS: u32 = 128; + +/// A unique temp path next to `path`, preserving the full file name so a +/// multi-extension artifact keeps its extensions (`sdk.tar.gz` becomes +/// `sdk.tar.gz.tmp-`, where `with_extension` would drop `.gz`). +fn temp_sibling_path(path: &Path, suffix: &OsStr) -> Result { + let parent = path.parent().context("file path has no parent directory")?; + let mut file_name = path + .file_name() + .context("file path has no file name")? + .to_os_string(); + file_name.push(".tmp-"); + file_name.push(suffix); + Ok(parent.join(file_name)) +} + +/// Move a staged temp file onto `path`, replacing whatever is there. +/// +/// The single implementation of the publish step for the whole workspace. It +/// lives here rather than in each caller because the Windows half is not +/// something to re-derive: a plain `rename` over an existing file is not +/// reliable while another process holds the destination open (an antivirus or +/// indexer scanning a file this CLI just wrote is enough), so an existing +/// destination goes through `ReplaceFileW`. Callers that got this wrong would +/// fail in the direction that matters — `ManagedServiceRecord::write` is on the +/// uninstall stop path, where a failed publish is reported as a service that +/// could not be stopped and aborts the whole uninstall. +#[cfg(not(windows))] +pub fn publish_temp_file(tmp: &Path, path: &Path) -> std::io::Result<()> { + fs::rename(tmp, path) +} + +/// Move a staged temp file onto `path`, replacing whatever is there. +/// +/// See the non-Windows twin for why this is centralized. +#[cfg(windows)] +pub fn publish_temp_file(tmp: &Path, path: &Path) -> std::io::Result<()> { + if path.try_exists()? { + return replace_file_windows(path, tmp); + } + + match fs::rename(tmp, path) { + Ok(()) => Ok(()), + Err(rename_error) => { + // The destination can appear between the check and the rename. + if path.try_exists()? { + replace_file_windows(path, tmp) + } else { + Err(rename_error) + } + } + } +} + +#[cfg(windows)] +#[allow(unsafe_code)] // Win32 FFI +fn replace_file_windows(path: &Path, replacement: &Path) -> std::io::Result<()> { + use windows_sys::Win32::Storage::FileSystem::ReplaceFileW; + + let path_wide: Vec = path.as_os_str().encode_wide().chain(Some(0)).collect(); + let replacement_wide: Vec = replacement + .as_os_str() + .encode_wide() + .chain(Some(0)) + .collect(); + + // SAFETY: both path buffers are valid, NUL-terminated UTF-16 strings and + // remain alive for the duration of the synchronous Windows API call. The + // optional backup, exclude, and reserved pointers are intentionally null. + let replaced = unsafe { + ReplaceFileW( + path_wide.as_ptr(), + replacement_wide.as_ptr(), + std::ptr::null(), + 0, + std::ptr::null(), + std::ptr::null(), + ) + }; + if replaced == 0 { + Err(std::io::Error::last_os_error()) + } else { + Ok(()) + } +} + +/// Write `bytes` to `path` so a concurrent reader sees either the old contents +/// or the new ones, never a half-written file. +/// +/// Staged under a unique sibling name reserved with `create_new` — so two +/// writers cannot pick the same scratch file — and published with +/// [`publish_temp_file`]. A failed publish takes the scratch file with it. +/// +/// Two limits worth knowing before reaching for this: +/// +/// * **Atomic, not durable.** The staged bytes are `sync_all`ed before the +/// publish, so a crash cannot leave a half-written or zero-length file. The +/// containing *directory* is never fsynced, so the rename itself can still be +/// lost by a power failure — the guarantee is that readers only ever see one +/// complete version, not that the newest one survives a crash. That flush is +/// held by construction, not by test: deleting it leaves every test green, +/// because reproducing what it prevents needs a real crash between the write +/// and the rename. Do not read a green suite as evidence it is still there. +/// * **Permissions are not carried over.** This replaces the target inode with +/// a freshly created file, so an existing file's mode does not survive (a +/// difference from the `fs::write` it usually replaces), and the new file gets +/// whatever the umask allows. What makes that acceptable today is only that +/// every current caller writes non-sensitive state — *not* the containing +/// directory, which `AppPaths::ensure` creates with plain `create_dir_all` and +/// no mode of its own. A caller with something sensitive to write must set the +/// mode explicitly here; it cannot lean on the directory. +/// * **A symlink at `path` is replaced, not written through.** The publish is a +/// rename onto `path` itself, so a symlink there is what gets replaced — where +/// `fs::write` would have followed it and overwritten the target. That is the +/// safer direction for state files (it cannot be aimed somewhere else by +/// planting a link) but it is a behaviour change, so a caller that means to +/// write through a link must resolve it first. +pub fn write_file_atomically(path: &Path, bytes: &[u8]) -> Result<()> { + let parent = path.parent().context("file path has no parent directory")?; + fs::create_dir_all(parent).with_context(|| format!("failed to create {}", parent.display()))?; + + let temp_id = format!("{}-{}", std::process::id(), unix_time_millis()); + let mut reserved = None; + for attempt in 0..ATOMIC_WRITE_TEMP_ATTEMPTS { + let tmp = temp_sibling_path(path, &OsString::from(format!("{temp_id}-{attempt}")))?; + match fs::OpenOptions::new() + .write(true) + .create_new(true) + .open(&tmp) + { + Ok(file) => { + reserved = Some((tmp, file)); + break; + } + Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => {} + Err(error) => { + return Err(error).with_context(|| format!("failed to create {}", tmp.display())); + } + } + } + let Some((tmp, mut file)) = reserved else { + bail!( + "failed to reserve a temporary file next to {} after {ATOMIC_WRITE_TEMP_ATTEMPTS} \ + attempts", + path.display() + ); + }; + + if let Err(error) = file.write_all(bytes) { + drop(file); + let _ = fs::remove_file(&tmp); + return Err(disk_space::map_write_error(error, &tmp)); + } + // Flush before publishing. Without this the rename can reach the disk while + // the bytes have not, leaving a zero-length file after a crash — which for a + // service record is not a lost update but an unparseable manifest, and the + // uninstall gate refuses to remove anything while one of those exists. + if let Err(error) = file.sync_all() { + drop(file); + let _ = fs::remove_file(&tmp); + return Err(disk_space::map_write_error(error, &tmp)); + } + drop(file); + + publish_temp_file(&tmp, path) + .inspect_err(|_| { + let _ = fs::remove_file(&tmp); + }) + .with_context(|| format!("failed to publish {}", path.display())) +} + #[cfg(test)] mod tests { use super::*; @@ -8217,6 +8503,266 @@ mod tests { use std::path::Path; use std::path::PathBuf; + /// A private root for one test, removed on the way out. + fn atomic_write_root(name: &str) -> PathBuf { + let root = std::env::temp_dir().join(format!( + "rocm-core-atomic-write-{name}-{}-{}", + std::process::id(), + unix_time_millis() + )); + let _ = fs::remove_dir_all(&root); + fs::create_dir_all(&root).expect("failed to create the test root"); + root + } + + /// The scratch files `write_file_atomically` stages under, if any survived. + /// + /// `temp_sibling_path` names them `.tmp---`, so + /// a leftover is visible as a sibling containing `.tmp-`. + fn leftover_scratch_files(dir: &Path) -> Vec { + fs::read_dir(dir) + .expect("failed to read the test root") + .filter_map(|entry| { + let name = entry.ok()?.file_name().to_string_lossy().into_owned(); + name.contains(".tmp-").then_some(name) + }) + .collect() + } + + /// Linux/unix: relies on a rename replacing the directory entry while an + /// already-open descriptor keeps reading the previous inode. Windows + /// publishes through `ReplaceFileW` and does not offer the same observation. + #[cfg(unix)] + #[test] + fn a_service_record_write_never_shows_a_reader_a_truncated_manifest() { + // `ManagedServiceRecord::write` runs on the uninstall stop path, where a + // torn manifest is not a lost update but an unparseable record that makes + // the gate refuse to remove anything. + // + // Asserting "the bytes landed" would not pin that: a plain `fs::write` + // also leaves correct final bytes and creates no scratch file, so such a + // test passes either way. The property only the atomic path has is what a + // *concurrent reader* sees — `fs::write` truncates the existing inode in + // place, so a reader holding it open watches the record disappear and + // come back, while the publish swaps in a new inode and leaves the old + // one whole. Hold the manifest open across a rewrite and require the old + // view to still be a complete, parseable record. + use std::io::{Read, Seek, SeekFrom}; + + let root = atomic_write_root("service-record-reader-safety"); + let paths = AppPaths { + config_dir: root.join("config"), + data_dir: root.join("data"), + cache_dir: root.join("cache"), + }; + paths.ensure().expect("create the app directories"); + let mut record = ManagedServiceRecord::new( + &paths, + "svc-reader-safety", + "vllm", + "amd/model", + "amd/model", + "127.0.0.1", + 8000, + "managed", + 0, + None, + None, + None, + ); + record.status = "ready".to_owned(); + record.write().expect("seed the manifest"); + + // A reader that opened the manifest before the rewrite began. + let mut reader = fs::File::open(&record.manifest_path).expect("open the manifest"); + + record.status = "stopped".to_owned(); + record.write().expect("rewrite the manifest"); + + reader.seek(SeekFrom::Start(0)).expect("rewind"); + let mut seen = Vec::new(); + reader + .read_to_end(&mut seen) + .expect("read the open manifest"); + let parsed: ManagedServiceRecord = serde_json::from_slice(&seen) + .expect("a reader holding the manifest open must never see a truncated document"); + assert_eq!( + parsed.status, "ready", + "the open descriptor must still see the pre-write record, not a rewritten inode" + ); + + // And the published file is the new one. + let published: ManagedServiceRecord = + serde_json::from_slice(&fs::read(&record.manifest_path).expect("read back")) + .expect("the published manifest must parse"); + assert_eq!(published.status, "stopped", "the rewrite must have landed"); + let _ = fs::remove_dir_all(root); + } + + #[test] + fn a_service_record_write_leaves_no_scratch_sibling() { + // This pins scratch-file hygiene, NOT atomic publishing: a plain + // `fs::write` creates no scratch file at all and would pass too. The + // atomicity property is pinned next door, by + // `a_service_record_write_never_shows_a_reader_a_truncated_manifest`, + // which fails against an in-place write. + // + // It still earns its place, but for a smaller reason than uninstall: + // scratch siblings are named `.json.tmp---`, + // whose extension is `tmp-…`, and both `unreadable_service_manifests` + // and the record loader filter on `extension == "json"` — so a leaked + // one is skipped, not mistaken for a corrupt record, and cannot abort + // uninstall. What this guards is directory hygiene: a publish that + // aborts between staging and rename leaves litter that accumulates + // silently, one file per failed write, in a directory an operator reads + // by hand. + let root = atomic_write_root("service-record-publish"); + let paths = AppPaths { + config_dir: root.join("config"), + data_dir: root.join("data"), + cache_dir: root.join("cache"), + }; + paths.ensure().expect("create the app directories"); + let mut record = ManagedServiceRecord::new( + &paths, + "svc-atomic-publish", + "vllm", + "amd/model", + "amd/model", + "127.0.0.1", + 8000, + "managed", + 0, + None, + None, + None, + ); + record.write().expect("first publish"); + record.status = "stopped".to_owned(); + record + .write() + .expect("republish over the existing manifest"); + + let services_dir = paths.services_dir(); + let stray = fs::read_dir(&services_dir) + .expect("read the services directory") + .filter_map(|entry| { + let name = entry.ok()?.file_name().to_string_lossy().into_owned(); + name.contains(".tmp-").then_some(name) + }) + .collect::>(); + assert!( + stray.is_empty(), + "publishing a record must leave no scratch manifest: {stray:?}" + ); + let written = fs::read(&record.manifest_path).expect("read the manifest back"); + let parsed: ManagedServiceRecord = + serde_json::from_slice(&written).expect("the published manifest must always parse"); + assert_eq!(parsed.status, "stopped", "the republish must have landed"); + let _ = fs::remove_dir_all(root); + } + + #[test] + fn write_file_atomically_replaces_contents_and_leaves_no_scratch_file() { + // The publish must land the new bytes and take its staging file with it; + // a leaked scratch sibling in `services_dir` would be a file the uninstall + // gate has to reason about. + let root = atomic_write_root("replaces"); + let target = root.join("record.json"); + fs::write(&target, b"old contents").expect("seed the file"); + + write_file_atomically(&target, b"new contents").expect("the write must succeed"); + + assert_eq!( + fs::read(&target).expect("read back"), + b"new contents", + "the published file must hold the new bytes" + ); + assert!( + leftover_scratch_files(&root).is_empty(), + "a successful publish must leave no scratch file: {:?}", + leftover_scratch_files(&root) + ); + let _ = fs::remove_dir_all(root); + } + + /// Unix: needs a real symlink. + #[cfg(unix)] + #[test] + fn write_file_atomically_replaces_a_symlink_instead_of_writing_through_it() { + // Documented behaviour, and a change from the `fs::write` this replaced: + // that followed a symlink and overwrote whatever it pointed at, while + // the publish here renames onto the path itself. Security-relevant in + // the safe direction — planting a link next to a state file can no + // longer redirect the write somewhere else — but a change, so it is + // pinned rather than left as prose. + let root = atomic_write_root("symlink-target"); + let elsewhere = root.join("elsewhere"); + fs::write(&elsewhere, b"must not be touched").expect("seed the link target"); + let target = root.join("state.json"); + std::os::unix::fs::symlink(&elsewhere, &target).expect("plant a symlink at the target"); + + write_file_atomically(&target, b"published").expect("the write must succeed"); + + assert_eq!( + fs::read(&elsewhere).expect("read the link target"), + b"must not be touched", + "the write must not follow the symlink to its target" + ); + assert_eq!( + fs::read(&target).expect("read the published file"), + b"published", + "the published bytes must land at the path itself" + ); + assert!( + !fs::symlink_metadata(&target) + .expect("stat the published path") + .file_type() + .is_symlink(), + "the symlink must have been replaced by a regular file" + ); + assert!( + leftover_scratch_files(&root).is_empty(), + "no scratch file may survive a successful publish" + ); + let _ = fs::remove_dir_all(root); + } + + #[test] + fn write_file_atomically_creates_the_parent_directory() { + // `ManagedServiceRecord::write` targets a services dir that may not exist + // yet on a first write. + let root = atomic_write_root("creates-parent"); + let target = root.join("nested").join("deeper").join("record.json"); + + write_file_atomically(&target, b"contents").expect("the write must succeed"); + + assert_eq!(fs::read(&target).expect("read back"), b"contents"); + let _ = fs::remove_dir_all(root); + } + + #[test] + fn write_file_atomically_cleans_up_when_publishing_fails() { + // A failed publish must not strand its staging file. The destination is a + // non-empty directory, which no platform will let a file replace, so the + // failure happens at the publish step with the scratch file already + // written — exactly the path that has to clean up after itself. + let root = atomic_write_root("publish-fails"); + let target = root.join("record.json"); + fs::create_dir_all(&target).expect("seed a directory where the file goes"); + fs::write(target.join("occupant"), b"x").expect("make it non-empty"); + + let error = write_file_atomically(&target, b"contents") + .expect_err("replacing a non-empty directory must fail"); + + assert!( + leftover_scratch_files(&root).is_empty(), + "a failed publish must remove its scratch file, got {:?} ({error:#})", + leftover_scratch_files(&root) + ); + let _ = fs::remove_dir_all(root); + } + #[test] fn file_lock_creates_missing_parent_dirs_and_lock_file() { let dir = @@ -8526,6 +9072,145 @@ mod tests { Ok(()) } + #[test] + fn an_empty_model_list_is_not_reported_as_somebody_elses_service() -> Result<()> { + // `openai_models_endpoint_has_model` answers `false` both for "these are + // someone else's models" and for "this endpoint listed nothing". For a + // readiness poll that is fine — not ready either way. For a caller + // deciding whether something still holds a port they are opposites: an + // engine that is up but has not populated its list yet, or is + // mid-unload, lists nothing while holding the port and the GPU, and + // reading that as "not ours" removes the tooling out from under it. + // + // Four connections: an empty list and a stranger's list, asked twice — + // once through the enum, once through the bool wrapper. The wrapper is + // where the three-way answer collapses back to the readiness contract + // every existing caller depends on, and that single comparison is not + // pinned by asserting on the enum alone. + let listener = std::net::TcpListener::bind("127.0.0.1:0")?; + let port = listener.local_addr()?.port(); + let server = std::thread::spawn(move || -> Result<()> { + let empty = r#"{"data":[]}"#; + let stranger = r#"{"data":[{"id":"someone/else"}]}"#; + for body in [empty, stranger, empty, stranger] { + let (mut stream, _) = listener.accept()?; + stream.set_read_timeout(Some(Duration::from_secs(2))).ok(); + let mut buffer = [0_u8; 512]; + let _ = stream.read(&mut buffer)?; + write!( + stream, + "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{}", + body.len(), + body + )?; + } + Ok(()) + }); + let endpoint = format!("http://127.0.0.1:{port}/v1"); + + assert_eq!( + openai_models_endpoint_identity( + &endpoint, + Some("amd/ours"), + None, + Duration::from_secs(2) + )?, + EndpointIdentity::ListsNoModels, + "an empty list says nothing about whose service this is" + ); + assert_eq!( + openai_models_endpoint_identity( + &endpoint, + Some("amd/ours"), + None, + Duration::from_secs(2) + )?, + EndpointIdentity::ServesOtherModels, + "a list that names other models is the only real evidence of a stranger" + ); + + // Across the wrapper. Only `ServesExpectedModel` may read as ready: + // mapping either of the other two arms to `true` would tell every + // readiness caller that an endpoint listing nothing, or listing a + // stranger's model, is serving theirs. + assert!( + !openai_models_endpoint_has_model( + &endpoint, + Some("amd/ours"), + None, + Duration::from_secs(2) + )?, + "an endpoint listing nothing is not serving our model" + ); + assert!( + !openai_models_endpoint_has_model( + &endpoint, + Some("amd/ours"), + None, + Duration::from_secs(2) + )?, + "an endpoint serving a stranger's model is not serving ours" + ); + + server.join().expect("server thread should not panic")?; + Ok(()) + } + + #[test] + fn a_get_reports_a_refusal_as_an_answer_not_as_a_failure() -> Result<()> { + // The distinction `http_get_text_with_auth` cannot make. Its `Err` means + // "no 200", which lumps a 401 from a live server together with a dead + // port — and a caller deciding whether something still holds a port has + // to tell those apart: only the refusal proves the port is held. + // + // Two connections: the same 401 read both ways. + let listener = std::net::TcpListener::bind("127.0.0.1:0")?; + let port = listener.local_addr()?.port(); + let server = std::thread::spawn(move || -> Result<()> { + for _ in 0..2 { + let (mut stream, _) = listener.accept()?; + stream.set_read_timeout(Some(Duration::from_secs(2))).ok(); + let mut buffer = [0_u8; 512]; + let _ = stream.read(&mut buffer)?; + write!( + stream, + "HTTP/1.1 401 Unauthorized\r\nContent-Length: 0\r\nConnection: close\r\n\r\n" + )?; + } + Ok(()) + }); + let endpoint = format!("http://127.0.0.1:{port}/v1"); + + let parts = http_get_with_auth(&endpoint, "/models", None, Duration::from_secs(2))?; + assert_eq!( + parts.status, 401, + "a refusal is an answer, and its status has to survive" + ); + assert!( + http_get_text_with_auth(&endpoint, "/models", None, Duration::from_secs(2)).is_err(), + "the text helper still collapses every non-200 into an error" + ); + + server.join().expect("server thread should not panic")?; + + // And an unreachable port is an error either way — the two must not + // collapse in the other direction. + let dead = std::net::TcpListener::bind("127.0.0.1:0")?; + let dead_port = dead.local_addr()?.port(); + drop(dead); + assert!( + http_get_with_auth( + &format!("http://127.0.0.1:{dead_port}/v1"), + "/models", + None, + Duration::from_millis(500), + ) + .is_err(), + "nothing answered, so there is no status to report" + ); + Ok(()) + } + // Why readiness is gated on an inference probe (EAI-7333): a server that // lists the model on `/v1/models` but cannot yet serve // `/v1/chat/completions` still reports the model as present. This test pins diff --git a/crates/rocm-core/src/proc_lifecycle.rs b/crates/rocm-core/src/proc_lifecycle.rs index 61f422f91..b32ed4b9b 100644 --- a/crates/rocm-core/src/proc_lifecycle.rs +++ b/crates/rocm-core/src/proc_lifecycle.rs @@ -12,6 +12,21 @@ //! truthfully: a stop is only "graceful" once the recorded process is observed to //! have actually exited within a bounded grace period, escalating to `SIGKILL` //! only when the caller opts into a forced stop. +//! +//! **The recycling defence is Linux-only in practice.** [`process_start_ticks`] +//! reads the start-time from `/proc` and is a compile-time `None` everywhere +//! else, so on Windows and macOS every record *captured there* carries no +//! identity, and [`identity_state`] degrades to best-effort +//! [`IdentityState::Matches`] — the paragraph above describes what this module +//! enforces *where the platform can answer*. (A record reconstructed with a +//! start-time recorded elsewhere is the other case: unconfirmable rather than +//! absent, so it lands on [`IdentityState::Indeterminate`] and is not signalled +//! either.) The degradation is deliberate: a +//! host that cannot tell two processes apart must not therefore refuse to stop +//! anything, since that would make every service unstoppable rather than making +//! any of them safer. What holds on those hosts is the rest of the caller's +//! gate — the port reality-check, the endpoint identity probe, and aborting +//! with the recovery tooling intact on any unconfirmed stop. use std::time::{Duration, Instant}; @@ -83,12 +98,68 @@ pub enum IdentityState { /// [`IdentityState::Indeterminate`], never a risky match. When no identity was /// recorded (legacy state files), it degrades to best-effort /// [`IdentityState::Matches`]. +/// +/// Reads the PID's current start-time itself. A caller that has already read it +/// — because it also needs the raw observation — should pass that one reading to +/// [`identity_state_with_observed`] rather than calling this and reading again, +/// so a process that exits between the two reads cannot produce two verdicts +/// derived from disagreeing observations. #[must_use] pub fn identity_state(id: &ProcessIdentity) -> IdentityState { - if !crate::process_is_running(id.pid) || process_has_exited(id.pid) { + identity_state_from_probes( + id, + || crate::process_is_running(id.pid) && !process_has_exited(id.pid), + || process_start_ticks(id.pid), + ) +} + +/// [`identity_state`] with its two observations taken lazily, so the order they +/// are taken in is a property of this body rather than of a call site. +/// +/// Liveness BEFORE the reading, and the early return is what enforces it. The +/// reading must not sit in argument position — Rust evaluates arguments before +/// entering the callee, which inverts the safe side of the race. A PID read +/// while it was still the recorded process, which then exits and is recycled +/// before the liveness check, would be compared as `expected == actual` and come +/// back [`IdentityState::Matches`]: the one verdict that authorises a kill, +/// handed out for a PID that is now somebody else. Reading only after liveness +/// means the reading always describes whatever holds the PID *now*, so a +/// recycled one disagrees and comes back [`IdentityState::Recycled`]. +/// +/// The two probes are parameters purely so that ordering is testable. Staging +/// the real race needs a process to exit and its PID to be reissued inside a +/// window of microseconds, which no test can do; two closures that record when +/// they were called pin it deterministically instead. They are `FnOnce` +/// generics, so this costs nothing at runtime — the real call above +/// monomorphizes back into the same two direct calls. +fn identity_state_from_probes( + id: &ProcessIdentity, + is_live: impl FnOnce() -> bool, + read_start_ticks: impl FnOnce() -> Option, +) -> IdentityState { + if !is_live() { return IdentityState::Gone; } - match (id.start_ticks, process_start_ticks(id.pid)) { + // Compare only. Routing back through `identity_state_with_observed` would + // re-run the liveness check just performed, which on Linux is two more + // `/proc` reads per call — paid on every 25 ms tick of the bounded waits, + // which is where this is called from in a loop. + compare_start_ticks(id, read_start_ticks()) +} + +/// The identity comparison alone, for callers that have already established +/// liveness. +/// +/// Split out so the ordering guarantee above does not have to pay for a second +/// liveness check. Deliberately not public: on its own it cannot return +/// [`IdentityState::Gone`], so a caller that had not checked liveness would get +/// [`IdentityState::Matches`] for a dead PID — the one verdict that authorises a +/// kill. The two callers that establish liveness first are in this file. +const fn compare_start_ticks( + id: &ProcessIdentity, + observed_start_ticks: Option, +) -> IdentityState { + match (id.start_ticks, observed_start_ticks) { (Some(expected), Some(actual)) => { if expected == actual { IdentityState::Matches @@ -104,6 +175,31 @@ pub fn identity_state(id: &ProcessIdentity) -> IdentityState { } } +/// [`identity_state`] against a start-time the caller has already observed. +/// +/// `observed_start_ticks` is what [`process_start_ticks`] returned for `id.pid`: +/// `None` both where the platform has no `/proc` and where that one PID's +/// start-time could not be read. +/// +/// Liveness is checked here too, so a PID that simply exits after the caller's +/// reading still yields [`IdentityState::Gone`]. What that check cannot catch is +/// exit *and recycle* between the reading and this call: the PID is live again, +/// and a reading taken while it was still the recorded process matches the +/// record, so the verdict is [`IdentityState::Matches`] for a process that is no +/// longer ours. Callers holding a reading across anything slow should re-read +/// rather than pass a stale one; [`identity_state`] avoids the window entirely +/// by reading only after its own liveness check. +#[must_use] +pub fn identity_state_with_observed( + id: &ProcessIdentity, + observed_start_ticks: Option, +) -> IdentityState { + if !crate::process_is_running(id.pid) || process_has_exited(id.pid) { + return IdentityState::Gone; + } + compare_start_ticks(id, observed_start_ticks) +} + /// Whether `state` means the recorded process is definitively no longer running. /// /// [`IdentityState::Indeterminate`] is intentionally excluded: a process we can @@ -300,8 +396,8 @@ fn send_signal(pid: u32, signal: Signal, _tree: bool) -> bool { /// Read the kernel start-time (field 22 of `/proc//stat`) for `pid`. /// -/// Returns `None` when the value cannot be read, including on non-Linux -/// platforms, where identity verification degrades to best-effort. +/// Returns `None` when the value cannot be read, including on platforms other +/// than Linux and Windows, where identity verification degrades to best-effort. #[cfg(target_os = "linux")] #[must_use] pub fn process_start_ticks(pid: u32) -> Option { @@ -309,7 +405,38 @@ pub fn process_start_ticks(pid: u32) -> Option { parse_start_ticks(&stat) } -#[cfg(not(target_os = "linux"))] +/// On Windows the start-time is the process creation `FILETIME` (100 ns ticks +/// since 1601). It is opaque to the identity comparison, which only checks +/// equality, and it is what lets a recycled PID be told from the recorded one. +#[cfg(windows)] +#[must_use] +#[allow(unsafe_code)] // Win32 FFI +pub fn process_start_ticks(pid: u32) -> Option { + use windows_sys::Win32::Foundation::{CloseHandle, FILETIME}; + use windows_sys::Win32::System::Threading::{ + GetProcessTimes, OpenProcess, PROCESS_QUERY_LIMITED_INFORMATION, + }; + + unsafe { + let handle = OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, 0, pid); + if handle.is_null() { + return None; + } + let zero = FILETIME { + dwLowDateTime: 0, + dwHighDateTime: 0, + }; + let (mut created, mut exited, mut kernel, mut user) = (zero, zero, zero, zero); + let ok = GetProcessTimes(handle, &mut created, &mut exited, &mut kernel, &mut user); + CloseHandle(handle); + if ok == 0 { + return None; + } + Some((u64::from(created.dwHighDateTime) << 32) | u64::from(created.dwLowDateTime)) + } +} + +#[cfg(not(any(target_os = "linux", windows)))] #[must_use] pub fn process_start_ticks(_pid: u32) -> Option { None @@ -361,6 +488,77 @@ mod tests { #[cfg(unix)] use std::process::{Child, Command, Stdio}; + #[test] + fn a_dead_process_is_never_read_for_a_start_time() { + // The ordering this module's safety rests on, pinned rather than + // inspected. If the reading is ever hoisted back into argument + // position — which reads as a harmless inlining — it happens before the liveness + // check, and a PID that exits and is recycled in between comes back + // `Matches`: the one verdict that authorises a kill, for a process that + // is no longer the recorded one. + // + // The real race is microseconds wide and needs a PID-space wrap, so it + // cannot be staged. This stages the observable consequence instead: a + // reading taken for a process already known dead is a reading that + // could not have informed the verdict. + let id = ProcessIdentity { + pid: 4321, + start_ticks: Some(99), + }; + let mut read_start_ticks = false; + + let state = identity_state_from_probes( + &id, + || false, + || { + read_start_ticks = true; + Some(99) + }, + ); + + assert_eq!( + state, + IdentityState::Gone, + "a process that fails the liveness check is Gone whatever its start time reads as" + ); + assert!( + !read_start_ticks, + "the start time must not be read at all once liveness has failed — if it was, the \ + read is back in argument position and the recycle race is inverted" + ); + } + + #[test] + fn a_live_process_is_judged_on_a_reading_taken_after_its_liveness_check() { + // The other half: when liveness passes, the reading is taken, and it is + // the reading — not the record — that decides. A recycled PID reads + // differently and must come back `Recycled`. + // + // The PID is arbitrary, and nothing here establishes whether it is + // running — it does not need to. Both probes are stubbed, so this path + // never consults the real process table at all, and that is the point: + // reaching the comparison at all proves the verdict came from the + // liveness this function was handed rather than from a second check of + // its own. While the comparison went back through + // `identity_state_with_observed`, this same call returned `Gone` from + // that function's own liveness check, whatever the stub said. + let id = ProcessIdentity { + pid: 4321, + start_ticks: Some(99), + }; + + assert_eq!( + identity_state_from_probes(&id, || true, || Some(99)), + IdentityState::Matches, + "a live PID still reading as its recorded start time is the recorded process" + ); + assert_eq!( + identity_state_from_probes(&id, || true, || Some(1234)), + IdentityState::Recycled, + "a live PID reading as a different start time is somebody else" + ); + } + /// Spawn a child that prints a line to stdout once it is ready, and block /// until that line arrives. This replaces sleep-based readiness guesses with /// a deterministic signal (e.g. that a shell has installed its SIGTERM trap), @@ -526,6 +724,51 @@ mod tests { reap(child); } + /// The observation the caller passes must be the one classified, not a fresh + /// read of the same PID. A caller that needs the raw start-time *and* the + /// verdict reads once and passes it down precisely so the two cannot + /// disagree; an implementation that quietly re-read `/proc` would restore + /// that hazard while every existing test still passed. Here the live child's + /// real start-time matches its recorded identity, so a re-reading + /// implementation returns `Matches` — only one that honours the argument + /// returns `Recycled`. + #[cfg(target_os = "linux")] + #[test] + fn identity_state_with_observed_classifies_the_reading_it_was_given() { + let (child, _) = spawn_ready("echo ready; while true; do sleep 1; done"); + let id = ProcessIdentity::capture(child.id()); + let real_ticks = id.start_ticks.expect("linux records a start-time"); + + // Collect every verdict first, then kill and reap, and only then assert. + // The child holds the harness's stdout pipe open, so an assertion that + // panics ahead of the kill does not merely fail this test — it leaks a + // looping process and hangs the whole suite on the pipe. Ask the + // questions, clean up unconditionally, then judge. + let agreeing = identity_state_with_observed(&id, Some(real_ticks)); + let disagreeing = identity_state_with_observed(&id, Some(real_ticks.wrapping_add(1))); + let unreadable = identity_state_with_observed(&id, None); + + let hard = terminate_verified(&id, KillScope::Single, Duration::from_secs(5), true); + reap(child); + + assert!(hard.stopped(), "the child must not outlive the test"); + assert_eq!( + agreeing, + IdentityState::Matches, + "the reading that agrees with the record is a match" + ); + assert_eq!( + disagreeing, + IdentityState::Recycled, + "a disagreeing reading must be classified, not discarded for a re-read" + ); + assert_eq!( + unreadable, + IdentityState::Indeterminate, + "an unreadable start-time against a recorded one is unconfirmable" + ); + } + /// A terminated child that has not been reaped yet is a zombie: it still has /// a PID, so `kill(pid, 0)` succeeds and `process_is_running` reports `true`. /// Termination checks must therefore go through `identity_state`, which diff --git a/docs/architecture.md b/docs/architecture.md index b07b2f52c..62c055399 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -25,7 +25,7 @@ Scoped to the crates that make up the shipped CLI/daemon/dashboard/engine surfac ### `apps/rocm` — main CLI binary -Subsystem modules already following full domain extraction (each owns its own types): `therock.rs`, `comfyui.rs`, `providers.rs`, `chat_host_facts.rs`, `dash.rs`, `dash_seam.rs`, `provider_keys.rs`, `serve_summary.rs`, `storage.rs`. Mechanically relocated dispatch-adjacent handlers (no owned types, shared config stays at the crate root): `automations.rs`, `uninstall.rs`, `endpoint_keys.rs`, `logging.rs`. `bootstrap.rs` is a further-extracted variant of full domain extraction: it owns its clap command enum (`BootstrapCommand`) and dispatch function too, rather than leaving them in `main.rs`. Shared CLI-output components: `cli_progress.rs` (`Spinner`, `AnimatedSpinner`), `cli_report.rs` (`ActionReport`). +Subsystem modules already following full domain extraction (each owns its own types): `therock.rs`, `comfyui.rs`, `providers.rs`, `chat_host_facts.rs`, `dash.rs`, `dash_seam.rs`, `provider_keys.rs`, `serve_summary.rs`, `storage.rs`, `uninstall_gate.rs` (the stop-before-remove gate behind `rocm uninstall`: its failure/report types and stop passes). Mechanically relocated dispatch-adjacent handlers (no owned types, shared config stays at the crate root): `automations.rs`, `uninstall.rs`, `endpoint_keys.rs`, `logging.rs`. `bootstrap.rs` is a further-extracted variant of full domain extraction: it owns its clap command enum (`BootstrapCommand`) and dispatch function too, rather than leaving them in `main.rs`. Shared CLI-output components: `cli_progress.rs` (`Spinner`, `AnimatedSpinner`), `cli_report.rs` (`ActionReport`). `main.rs` itself is **not yet modularized** — see EAI-7768, split planned across several PRs, one cluster at a time. diff --git a/docs/manual-testing.md b/docs/manual-testing.md index 864b59796..f3d9b4c52 100644 --- a/docs/manual-testing.md +++ b/docs/manual-testing.md @@ -375,3 +375,21 @@ To turn provider-assisted planning back off: ```powershell rocm config clear-planner-provider ``` + +## 8. Uninstall With a Running Managed Server + +Start a managed server (`rocm serve `), then run: + +```powershell +rocm uninstall --yes +``` + +Expected result: + +- The plan warns the server will be stopped before removal. +- The server is stopped, then the files are removed and the command exits 0. + +To check the refusal path, corrupt a file under the `services` directory +(for example write `{` into `services/bad.json`) and run `rocm uninstall --yes` +again. It must exit non-zero, name the file, advise repairing or deleting it, +and leave the install, config, and data directories in place. diff --git a/docs/testing.md b/docs/testing.md index 545378c81..41ebdd966 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -1058,7 +1058,9 @@ generated public-key PEM installer verification; verify first-install PATH setup Linux writes the shell profile); reinstall stale-manifest purge and config preservation; a Windows loopback-HTTP install; isolated installed-binary directory smoke checks (`rocm examine` must read only the isolated -config/data/cache, never the real user `.rocm` state); and full-purge uninstall. +config/data/cache, never the real user `.rocm` state); full-purge uninstall; uninstall stopping the local server it manages (Linux and +Windows); and uninstall refusing to remove anything while a managed +service record cannot be read. Each scenario generates its own local keys, package, and install root under a per-scenario temp directory, so they are independent and use generated local keys only — project-owned production signing keys remain an owner-controlled diff --git a/tests/e2e-cucumber/features/install_lifecycle.feature b/tests/e2e-cucumber/features/install_lifecycle.feature index 63f5a617b..a0aca59f3 100644 --- a/tests/e2e-cucumber/features/install_lifecycle.feature +++ b/tests/e2e-cucumber/features/install_lifecycle.feature @@ -131,10 +131,24 @@ Feature: Release install lifecycle And the install manifest is gone And the isolated XDG config, data, and cache state is gone + # EAI-8014: uninstall used to report success while a server it manages kept + # serving and holding the GPU, then delete the very tooling (`rocm services + # stop`, the service records) needed to stop it. + @id:lifecycle-linux-uninstall-stops-managed-server @lifecycle @requires-os:linux + Scenario: lifecycle-10 - Linux - uninstall stops the local server it manages + Given a freshly built release tree + And a generated signing keypair + And a signed bundle installed with the public key file + And the installed binary has isolated XDG directories with state + And a local server this machine manages is running + When the user uninstalls from the installed binary + Then the removal is reported as complete + And the local server this machine manages is no longer running + # ── Windows user-PATH restoration, loopback HTTP install, isolated smoke ─ @id:lifecycle-windows-install-signed @lifecycle @requires-os:windows - Scenario: lifecycle-10 - Windows - a signed zip installs and verifies with native crypto + Scenario: lifecycle-11 - Windows - a signed zip installs and verifies with native crypto Given a freshly built release tree And a generated signing keypair When the release is packaged and signed with the private key file @@ -144,7 +158,7 @@ Feature: Release install lifecycle And the install manifest is present @id:lifecycle-windows-key-rotation-fallback @lifecycle @requires-os:windows - Scenario: lifecycle-11 - Windows - a malformed current trust root falls through to the valid next key + Scenario: lifecycle-12 - Windows - a malformed current trust root falls through to the valid next key Given a freshly built release tree And a generated signing keypair When the release is packaged and signed with the private key file @@ -154,7 +168,7 @@ Feature: Release install lifecycle And the installed rocm binary is present @id:lifecycle-windows-all-pinned-keys-malformed @lifecycle @requires-os:windows - Scenario: lifecycle-12 - Windows - all malformed pinned trust roots fail deterministically + Scenario: lifecycle-13 - Windows - all malformed pinned trust roots fail deterministically Given a freshly built release tree And a generated signing keypair When the release is packaged and signed with the private key file @@ -164,7 +178,7 @@ Feature: Release install lifecycle And no binaries are activated in the target directory @id:lifecycle-windows-updates-user-path @lifecycle @requires-os:windows - Scenario: lifecycle-13 - Windows - a default install updates the user PATH and restores it afterwards + Scenario: lifecycle-14 - Windows - a default install updates the user PATH and restores it afterwards Given a freshly built release tree And a generated signing keypair And the user PATH is captured for restoration @@ -175,7 +189,7 @@ Feature: Release install lifecycle And the install directory is on the user PATH @id:lifecycle-windows-verifies-without-openssl @lifecycle @requires-os:windows - Scenario: lifecycle-14 - Windows - the installer verifies with native crypto when openssl is absent from PATH + Scenario: lifecycle-15 - Windows - the installer verifies with native crypto when openssl is absent from PATH Given a freshly built release tree And a generated signing keypair When the release is packaged and signed with the private key file @@ -185,7 +199,7 @@ Feature: Release install lifecycle And the install manifest is present @id:lifecycle-windows-rejects-bad-signature-without-openssl @lifecycle @requires-os:windows - Scenario: lifecycle-15 - Windows - a bad signature is rejected with native crypto when openssl is absent from PATH + Scenario: lifecycle-16 - Windows - a bad signature is rejected with native crypto when openssl is absent from PATH Given a freshly built release tree And a generated signing keypair When the release is packaged and signed with the private key file @@ -195,7 +209,7 @@ Feature: Release install lifecycle And no binaries are activated in the target directory @id:lifecycle-windows-reinstall-purges-stale @lifecycle @requires-os:windows - Scenario: lifecycle-16 - Windows - reinstalling purges a stale prior entry + Scenario: lifecycle-17 - Windows - reinstalling purges a stale prior entry Given a freshly built release tree And a generated signing keypair And a signed bundle installed with the public key file @@ -206,7 +220,7 @@ Feature: Release install lifecycle And the install manifest is present @id:lifecycle-windows-reject-untrusted-key @lifecycle @requires-os:windows - Scenario: lifecycle-17 - Windows - an install with no public key rejects an untrusted signer + Scenario: lifecycle-18 - Windows - an install with no public key rejects an untrusted signer Given a freshly built release tree And a generated signing keypair When the release is packaged and signed with the private key file @@ -215,7 +229,7 @@ Feature: Release install lifecycle And no binaries are activated in the target directory @id:lifecycle-windows-reject-bad-checksum @lifecycle @requires-os:windows - Scenario: lifecycle-18 - Windows - a tampered checksum is rejected before activation + Scenario: lifecycle-19 - Windows - a tampered checksum is rejected before activation Given a freshly built release tree And a generated signing keypair When the release is packaged and signed with the private key file @@ -225,7 +239,7 @@ Feature: Release install lifecycle And no binaries are activated in the target directory @id:lifecycle-windows-reject-bad-signature @lifecycle @requires-os:windows - Scenario: lifecycle-19 - Windows - a tampered signature is rejected before activation + Scenario: lifecycle-20 - Windows - a tampered signature is rejected before activation Given a freshly built release tree And a generated signing keypair When the release is packaged and signed with the private key file @@ -235,7 +249,7 @@ Feature: Release install lifecycle And no binaries are activated in the target directory @id:lifecycle-windows-reject-missing-signature @lifecycle @requires-os:windows - Scenario: lifecycle-20 - Windows - a missing signature is rejected when a signature is required + Scenario: lifecycle-21 - Windows - a missing signature is rejected when a signature is required Given a freshly built release tree And a generated signing keypair When the release is packaged and signed with the private key file @@ -245,7 +259,7 @@ Feature: Release install lifecycle And no binaries are activated in the target directory @id:lifecycle-windows-http-install @lifecycle @requires-os:windows - Scenario: lifecycle-21 - Windows - a signed bundle installs over a loopback HTTP download + Scenario: lifecycle-22 - Windows - a signed bundle installs over a loopback HTTP download Given a freshly built release tree And a generated signing keypair When the release is packaged and signed with the private key file @@ -255,7 +269,7 @@ Feature: Release install lifecycle And the install manifest is present @id:lifecycle-windows-installed-binary-smoke @lifecycle @requires-os:windows - Scenario: lifecycle-22 - Windows - the installed binary honors isolated directories and keeps the running executable on uninstall + Scenario: lifecycle-23 - Windows - the installed binary honors isolated directories and keeps the running executable on uninstall Given a freshly built release tree And a generated signing keypair And a signed bundle installed with the public key file @@ -272,3 +286,40 @@ Feature: Release install lifecycle Then uninstall reports skipping the running executable on Windows And the non-running installed rocmd binary is gone And the install manifest is gone + + # The Windows half of EAI-8014. Worth its own scenario rather than trusting the + # Linux one because the termination path differs: Windows terminates only the + # recorded process, where Linux signals the whole tree. + # + # What this pins is that PID-based termination, not the gate's port + # reality-check. The planted record carries port 0, which nothing can ever be + # serving on, so the port branch is satisfied trivially on every run. Staging + # the case the port check exists for — an engine grandchild outliving the + # recorded process and still serving while the record reads "stopped" — needs + # a real listener the scenario does not stand up. That branch is covered by + # the unit tests around `stopped_record_verdict` instead. + @id:lifecycle-windows-uninstall-stops-managed-server @lifecycle @requires-os:windows + Scenario: lifecycle-24 - Windows - uninstall stops the local server it manages + Given a freshly built release tree + And a generated signing keypair + And a signed bundle installed with the public key file + And the installed binary has isolated directories with state + And a local server this machine manages is running + When the user uninstalls from the installed binary + Then the removal is reported as complete + And the local server this machine manages is no longer running + + # EAI-8014: when a managed service record cannot be read, uninstall cannot tell + # whether a server is still running, so it must refuse rather than delete the + # tooling that would be needed to stop it. + @id:lifecycle-linux-uninstall-refuses-unreadable-record @lifecycle @requires-os:linux + Scenario: lifecycle-25 - Linux - uninstall refuses and keeps everything when a service record cannot be read + Given a freshly built release tree + And a generated signing keypair + And a signed bundle installed with the public key file + And the installed binary has isolated XDG directories with state + And a managed service record that cannot be parsed + When the user uninstalls from the installed binary + Then the removal is refused with advice to repair or delete that record + And the installed rocm binary and manifest are still present + And the isolated XDG state is still present diff --git a/tests/e2e-cucumber/tests/e2e/lifecycle_steps.rs b/tests/e2e-cucumber/tests/e2e/lifecycle_steps.rs index 36e3e11aa..6acbf2f1f 100644 --- a/tests/e2e-cucumber/tests/e2e/lifecycle_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/lifecycle_steps.rs @@ -21,6 +21,7 @@ use std::path::{Path, PathBuf}; use std::process::Command; use cucumber::{given, then, when}; +use e2e_cucumber::mock_server::{ServiceRecordOptions, write_service_record_with}; use crate::E2eWorld; use crate::e2e::tui_driver::{TuiSession, default_timeout}; @@ -54,10 +55,21 @@ pub struct LifecycleState { smoke_config: Option, smoke_data: Option, smoke_cache: Option, + /// A stand-in for a local server this machine manages, planted with a + /// service record so uninstall must stop it. Killed on `Drop` if a scenario + /// fails before uninstall reaches it, so no scenario leaks a process. + managed_server: Option, + broken_record: Option, } impl Drop for LifecycleState { fn drop(&mut self) { + // Never leak the managed-server stand-in, even when a scenario fails + // before uninstall would have stopped it. + if let Some(mut server) = self.managed_server.take() { + let _ = server.kill(); + let _ = server.wait(); + } // Restore the machine user PATH if a scenario mutated it, even on panic. #[cfg(windows)] if let Some(previous) = self.captured_user_path.take() { @@ -385,6 +397,8 @@ async fn given_release_tree(world: &mut E2eWorld) { smoke_config: None, smoke_data: None, smoke_cache: None, + managed_server: None, + broken_record: None, }); } @@ -458,6 +472,80 @@ fn seed_isolated_dirs(world: &mut E2eWorld) { st.smoke_cache = Some(cache); } +/// A single long-lived process to stand in for a managed server, on either OS. +/// +/// One process, not a shell that spawns one: the CLI terminates the recorded +/// PID, and a grandchild would leave the scenario asserting against the wrong +/// process. +fn spawn_long_lived_child() -> std::process::Child { + #[cfg(windows)] + let mut command = { + let mut command = Command::new("powershell"); + command.args(["-NoProfile", "-Command", "Start-Sleep -Seconds 600"]); + command + }; + #[cfg(not(windows))] + let mut command = { + let mut command = Command::new("sleep"); + command.arg("600"); + command + }; + command + .spawn() + .expect("failed to start the managed server stand-in") +} + +/// Start a local server this machine manages: a real long-lived child process +/// plus the on-disk service record `rocm serve --managed` would leave behind, +/// planted in the isolated data dir the installed binary reads. +/// +/// Black-box on purpose — a process the CLI can find only through its record, +/// exactly as a real managed vLLM server appears to `rocm uninstall`. The record +/// carries no start-time token (the legacy shape), so the CLI's verified +/// termination treats it as a best-effort match and stops it. +#[given("a local server this machine manages is running")] +async fn given_managed_server_running(world: &mut E2eWorld) { + let services_dir = state(world) + .smoke_data + .as_ref() + .expect("isolated data dir must be seeded before planting a service record") + .join("services"); + let server = spawn_long_lived_child(); + // Port 0 is the one port nothing can ever be serving on: it resolves, so the + // CLI really does run its probe, and the connect always fails. Liveness then + // falls through to the recorded process, which is alive. Binding an + // ephemeral port and dropping it would leave a window in which something + // else on a busy runner grabs it and fails the scenario. + let free_port = 0; + write_service_record_with( + &services_dir, + "amd/test-model", + free_port, + ServiceRecordOptions { + status: "ready", + startup_phase: None, + supervisor_pid: server.id(), + engine_pid: Some(server.id()), + }, + ); + state_mut(world).managed_server = Some(server); +} + +/// An unparseable `*.json` under the isolated `services/` dir: the CLI cannot +/// tell whether it describes a running server. +#[given("a managed service record that cannot be parsed")] +async fn given_unparseable_service_record(world: &mut E2eWorld) { + let services_dir = state(world) + .smoke_data + .as_ref() + .expect("isolated data dir must be seeded before planting a service record") + .join("services"); + std::fs::create_dir_all(&services_dir).expect("failed to create services dir"); + std::fs::write(services_dir.join("broken.json"), b"{ not json") + .expect("failed to write unparseable record"); + state_mut(world).broken_record = Some(services_dir.join("broken.json")); +} + // ── When: package ────────────────────────────────────────────────────── async fn package_with_key_file(world: &mut E2eWorld) { @@ -916,7 +1004,9 @@ async fn when_uninstall(world: &mut E2eWorld) { String::from_utf8_lossy(&output.stdout), String::from_utf8_lossy(&output.stderr) ); - state_mut(world).last_output = combined; + let st = state_mut(world); + st.last_output = combined; + st.last_rc = output.status.code().unwrap_or(-1); } #[when("the user uninstalls from the installed binary keeping config data and cache")] @@ -1187,6 +1277,102 @@ async fn then_examine_isolated(world: &mut E2eWorld) { } } +#[then("the removal is reported as complete")] +async fn then_removal_complete(world: &mut E2eWorld) { + let out = &state(world).last_output; + assert!( + out.contains("uninstall complete"), + "uninstall did not report completion:\n{out}" + ); +} + +#[then("the removal is refused with advice to repair or delete that record")] +async fn then_removal_refused(world: &mut E2eWorld) { + let st = state(world); + let out = &st.last_output; + let record = st.broken_record.as_ref().expect("no broken record planted"); + assert_ne!( + st.last_rc, 0, + "uninstall succeeded despite an unreadable record:\n{out}" + ); + assert!( + out.contains("repair or delete"), + "refusal did not advise repairing or deleting the record:\n{out}" + ); + assert!( + out.contains(&record.display().to_string()), + "refusal did not name the unreadable record {}:\n{out}", + record.display() + ); + assert!( + !out.contains("uninstall complete"), + "refused uninstall still reported completion:\n{out}" + ); +} + +#[then("the installed rocm binary and manifest are still present")] +async fn then_install_kept(world: &mut E2eWorld) { + let st = state(world); + for path in [ + installed_binary(&st.install_dir, "rocm"), + st.install_dir.join(".rocm-cli-manifest"), + ] { + assert!( + path.exists(), + "refused uninstall removed {}", + path.display() + ); + } +} + +#[then("the isolated XDG state is still present")] +async fn then_xdg_kept(world: &mut E2eWorld) { + let st = state(world); + let record = st.broken_record.as_ref().expect("no broken record planted"); + assert!( + record.exists(), + "refused uninstall removed {}", + record.display() + ); + for dir in [&st.smoke_config, &st.smoke_data, &st.smoke_cache] + .into_iter() + .flatten() + { + assert!(dir.exists(), "refused uninstall removed {}", dir.display()); + } +} + +#[then("the local server this machine manages is no longer running")] +async fn then_managed_server_stopped(world: &mut E2eWorld) { + let mut server = state_mut(world) + .managed_server + .take() + .expect("no managed server was started"); + // The stand-in is our own child, so its exit is observable directly rather + // than through a PID probe that a zombie would answer wrongly. Uninstall + // waits out its own bounded stop grace, so allow for that before failing. + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(30); + let exited = loop { + match server + .try_wait() + .expect("failed to poll the managed server") + { + Some(_) => break true, + None if std::time::Instant::now() >= deadline => break false, + None => std::thread::sleep(std::time::Duration::from_millis(100)), + } + }; + if !exited { + let _ = server.kill(); + let _ = server.wait(); + } + let out = &state(world).last_output; + assert!( + exited, + "uninstall reported success while the server it manages kept running:\n{out}" + ); +} + #[then("uninstall reports skipping the running executable on Windows")] async fn then_skip_running_exe(world: &mut E2eWorld) { let out = &state(world).last_output;