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

README.md

@@ -970,6 +970,25 @@ first: if it cannot be, nothing is removed, so you can fix the cause and run
 `rocm uninstall` again. Each step is reported as it finishes, so if a later
 step fails, the output still shows what was already removed.
 
+`rocm uninstall` stops any managed model server that is still running before it
+removes anything, and also stops the background helper (`rocmd`), which is
+force-stopped (on Linux its whole process tree, on Windows only the recorded
+process) because it would otherwise restart the servers.
+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 live server or helper recorded without a start time (written
+by an older version) cannot be proven to be ours, so it is refused rather than
+killed: stop it yourself, then re-run. The same refusal applies when the
+helper's runtime state file cannot be read. The plan shows the refusal before
+you confirm it. The stop pass is not atomic: if it aborts after stopping some
+servers (or the helper), those stay stopped and their endpoint keys are gone;
+the error names each one and the `rocm serve` command that brings it back. A
+run that keeps the binaries and data (`--keep-binaries --keep-data`) leaves
+running servers and the helper alone.
+If a removal fails partway, the error lists what was removed and what is left,
+because the binaries and service records may already be gone.
+
 On Linux, if a config, data, or cache folder is a link to somewhere else (for
 example a cache moved to a bigger disk), `rocm uninstall` removes the link and
 leaves the folder it points to alone, whether or not the setting ends in a `/`.

apps/rocm/Cargo.toml

@@ -56,7 +56,6 @@ libc.workspace = true
 [target.'cfg(windows)'.dependencies]
 self-replace = "1.5"
 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"] }

apps/rocm/src/service_launch.rs

@@ -199,18 +199,18 @@ pub(crate) fn ensure_public_service_has_endpoint_key(
             "managed service was launched with `--require-api-key` but has no endpoint API key, \
              so restarting it would reopen it without authentication. Something outside this \
              machine may still be publishing its port. The key is dropped when a service stops \
-             and cannot be recovered. Launch it again with \
-             `rocm serve --require-api-key` (add `--api-key <key>`, or set ROCM_SERVE_API_KEY, \
-             to choose the key instead of generating one)."
+             and cannot be recovered. Launch it again with `{}` {}.",
+            rocm_engine_protocol::relaunch_command(host, true),
+            rocm_engine_protocol::RELAUNCH_KEY_CHOICE
         );
     }
     if rocm_engine_protocol::is_public_bind_host(host) && !key_present {
         bail!(
             "managed service is bound to the public host `{host}` but has no endpoint API key, \
              so restarting it would reopen it without authentication. The key is dropped when a \
-             service stops and cannot be recovered. Launch it again with \
-             `rocm serve --host {host} --allow-public-bind` (add `--api-key <key>`, or set \
-             ROCM_SERVE_API_KEY, to choose the key instead of generating one)."
+             service stops and cannot be recovered. Launch it again with `{}` {}.",
+            rocm_engine_protocol::relaunch_command(host, false),
+            rocm_engine_protocol::RELAUNCH_KEY_CHOICE
         );
     }
     Ok(())
@@ -1149,7 +1149,7 @@ fn stream_attached_logs_no_tty(log_path: &Path, child_pid: u32) -> Result<Attach
 }
 
 #[cfg(test)]
-mod tests {
+pub(crate) mod tests {
     use super::*;
     use crate::tests::test_paths;
 
@@ -1577,9 +1577,26 @@ mod tests {
         // And the loopback-with-forced-auth case still reports its own reason.
         let error = ensure_public_service_has_endpoint_key("127.0.0.1", false, true)
             .expect_err("a service that demanded auth must not come back without it");
+        let rendered = format!("{error:#}");
+        assert!(rendered.contains("--require-api-key"), "{rendered}");
+        // The relaunch command has to be one that can be run: it names the model
+        // and port to fill in, and the recorded host, like its public-bind sibling.
         assert!(
-            format!("{error:#}").contains("--require-api-key"),
-            "{error:#}"
+            rendered.contains("rocm serve <model> --host 127.0.0.1 --port <port>"),
+            "{rendered}"
+        );
+        assert!(
+            !rendered.contains("--allow-public-bind"),
+            "a loopback service is not told to open a public bind: {rendered}"
+        );
+
+        // A public host that also demanded auth is told how to bind publicly.
+        let error = ensure_public_service_has_endpoint_key("0.0.0.0", false, true)
+            .expect_err("a public service that demanded auth must not come back without it");
+        let rendered = format!("{error:#}");
+        assert!(
+            rendered.contains("--host 0.0.0.0 --port <port> --allow-public-bind --require-api-key"),
+            "{rendered}"
         );
     }
 
@@ -1641,11 +1658,15 @@ mod tests {
     }
 
     /// Build an `AutomationRuntimeState` for the no-double-spawn guard tests.
-    fn runtime_state(running: bool, daemon_pid: u32) -> AutomationRuntimeState {
+    ///
+    /// `daemon_start_ticks` is left unrecorded; tests that care about the
+    /// daemon's identity set it explicitly.
+    pub(crate) 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,

apps/rocm/src/therock.rs

@@ -4716,6 +4716,18 @@ fn temp_sibling_path(path: &Path, suffix: &OsStr) -> Result<PathBuf> {
     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.
 pub(crate) fn write_file_atomically(path: &Path, bytes: &[u8]) -> Result<()> {
     let temp_id = format!("{}-{}", std::process::id(), unix_time_millis());
     write_file_atomically_with(
@@ -4832,60 +4844,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<u16> = path.as_os_str().encode_wide().chain(Some(0)).collect();
-    let replacement_wide: Vec<u16> = 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<()> {