fix(serve): keep a public endpoint authenticated across respawns

apps/rocm/src/main.rs

@@ -4342,8 +4342,11 @@ fn validate_bind_host(host: &str, allow_public_bind: bool) -> Result<()> {
     Ok(())
 }
 
+/// Inverse of [`rocm_engine_protocol::is_public_bind_host`], which owns the
+/// policy so `rocmd` classifies a recorded `host` identically when it respawns
+/// the service.
 fn is_loopback_host(host: &str) -> bool {
-    matches!(host, "127.0.0.1" | "localhost" | "::1")
+    !rocm_engine_protocol::is_public_bind_host(host)
 }
 
 /// Resolve the API key that will guard this endpoint, applying the
@@ -4424,6 +4427,37 @@ fn ensure_public_bind_engine_supported(
     Ok(())
 }
 
+/// Refuse to (re)spawn a managed service recorded on a public host when its
+/// endpoint API key is gone, so a respawn cannot reopen the endpoint anonymously.
+///
+/// `serve()` applies the loopback-vs-public policy once, via
+/// [`resolve_endpoint_auth`], and persists the resulting key. Every later spawn
+/// — `rocm services restart`, and `rocmd`'s recovery supervisor — reads that key
+/// file back and would otherwise treat "no key file" as "no auth wanted",
+/// silently downgrading a protected public endpoint to an open one. The real
+/// invariant is a property of the *host*, not of the file: a non-loopback bind
+/// must always be authenticated.
+///
+/// The gap is reachable through ordinary commands, because a stop deletes the
+/// key file and `rocm services restart` accepts a stopped service id (its help
+/// points at `rocm services list --all`).
+///
+/// `key_present` is a plain `bool` rather than a path so both branches are
+/// unit-testable without touching the filesystem, mirroring `is_windows` in
+/// [`ensure_public_bind_engine_supported`].
+fn ensure_public_service_has_endpoint_key(host: &str, key_present: bool) -> Result<()> {
+    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)."
+        );
+    }
+    Ok(())
+}
+
 /// Write `contents` to `path` with owner-only (0600) permissions on Unix so a
 /// secret is not world-readable. On non-Unix, default permissions apply.
 pub(crate) fn write_private_file_0600(path: &Path, contents: &[u8]) -> Result<()> {
@@ -4636,7 +4670,17 @@ fn spawn_managed_engine_child(
     // environment. A path — not the secret value — is what the detached-spawn
     // primitives accept as an env override, and it keeps the key off both the argv
     // and the environment block. `serve()` wrote the file before spawning.
-    let endpoint_key_file = endpoint_keys::endpoint_key_file_if_present(paths, service_id);
+    // Validity, not mere existence: the engine adapters resolve the key with
+    // `endpoint_api_key_from_file` and enforce nothing when it yields `None`, so
+    // an empty or malformed key file would otherwise satisfy the guard below and
+    // still produce an unauthenticated public listener.
+    let endpoint_key_file = endpoint_keys::endpoint_key_file_if_present(paths, service_id)
+        .filter(|path| rocm_engine_protocol::endpoint_api_key_from_file(path).is_some());
+    // `serve()` already resolved and stored the key for a public bind, so this
+    // cannot fire on the fresh-launch path today. It is the shared choke point
+    // for managed spawns, so enforce the invariant here too rather than relying
+    // on every future caller having done so.
+    ensure_public_service_has_endpoint_key(host, endpoint_key_file.is_some())?;
     #[cfg(windows)]
     let child_pid = {
         let env_values = app_path_env_var_values(paths, engine_envs_root.as_deref());
@@ -12762,12 +12806,27 @@ fn stop_internal_managed_service(paths: &AppPaths, service_id: &str) -> Result<s
     // that did not happen.
     if all_stopped {
         record.status = "stopped".to_owned();
+        record.stop_requested_unix_ms = None;
+    } else {
+        // Record that a stop was *asked for* even though it could not be
+        // confirmed. `refresh_managed_service_runtime_liveness` needs this to
+        // tell a service the operator stopped from one that merely crashed: only
+        // the former should lose its endpoint key once its processes are gone.
+        record.stop_requested_unix_ms = Some(rocm_core::unix_time_millis());
     }
     record.write()?;
     // Drop the endpoint key with the service by deleting its 0600 key file.
     // Best-effort — a stopped service must not fail to stop just because key
     // cleanup did.
-    endpoint_keys::clear_endpoint_api_key(paths, &record.service_id);
+    //
+    // Gated on `all_stopped` for the same reason the status is: an unconfirmed
+    // stop may have left the engine alive and still enforcing the key, and
+    // discarding our only copy would lock the CLI's own probes, chat, and
+    // service discovery out of a service that is otherwise fine. The marker
+    // written above hands that cleanup to the liveness refresh instead.
+    if all_stopped {
+        endpoint_keys::clear_endpoint_api_key(paths, &record.service_id);
+    }
     let engine_stop = match engine_stop {
         Ok(response) => serde_json::json!({
             "attempted": true,
@@ -12816,10 +12875,19 @@ fn restart_internal_managed_service(
     // service back on the same public host with the same auth. Capture it first and
     // re-store it after the stop so the spawn below hands the child the same key.
     let preserved_endpoint_key = endpoint_keys::endpoint_api_key(paths, service_id);
+    // Checked before the stop, so a refused restart leaves a running service
+    // running instead of stopping it and then failing to bring it back.
+    ensure_public_service_has_endpoint_key(&record.host, preserved_endpoint_key.is_some())?;
     let _ = stop_internal_managed_service(paths, service_id);
     if let Some(key) = preserved_endpoint_key.as_deref() {
         endpoint_keys::store_endpoint_api_key(paths, service_id, key)?;
     }
+    // The stop above may have recorded an unconfirmed-stop marker; a successful
+    // restart supersedes it. Leaving it set would let the next liveness refresh
+    // delete the key of the service we are bringing back up. Reaches disk with
+    // the record writes below; if the restart bails before one of those, the
+    // marker stays set on disk — correct, since then the stop is what stands.
+    record.stop_requested_unix_ms = None;
     let policy = parse_device_policy(record.device_policy.as_deref())?;
     fs::OpenOptions::new()
         .create(true)
@@ -13686,12 +13754,56 @@ fn managed_service_running_state(status: &str) -> &'static str {
 
 const SERVICE_LIVENESS_CHECK_TIMEOUT: Duration = Duration::from_millis(750);
 
+/// The real process ids a record tracks.
+///
+/// The launcher pid is always recorded; `engine_pid` is adopted once the engine
+/// state file reports one. A `0` pid is a placeholder, never a real process.
+fn recorded_service_pids(record: &ManagedServiceRecord) -> Vec<u32> {
+    [record.engine_pid, Some(record.supervisor_pid)]
+        .into_iter()
+        .flatten()
+        .filter(|pid| *pid != 0)
+        .collect()
+}
+
+/// Complete a stop that was requested but could not confirm termination, once
+/// the processes are actually observed gone: drop the endpoint key and clear the
+/// marker. Returns whether the record changed.
+///
+/// This is deliberately keyed on `stop_requested_unix_ms` rather than on "the
+/// tracked pids are dead". A *crashed* service also has dead pids, but its
+/// operator never asked for it to go away and will want it back: dropping the
+/// key there would make `rocm services restart` and daemon recovery refuse it
+/// permanently — fail-closed guards have no way to re-mint a key. The cost of
+/// this choice is that a crashed public service's 0600 key file outlives the
+/// process until the service is stopped or successfully restarted.
+///
+/// Called before the liveness gate in
+/// [`refresh_managed_service_runtime_liveness`], so a record that reached
+/// "stopped" by another route still gets its deferred cleanup rather than
+/// stranding the key.
+fn settle_pending_stop_key_cleanup(paths: &AppPaths, record: &mut ManagedServiceRecord) -> bool {
+    if record.stop_requested_unix_ms.is_none() {
+        return false;
+    }
+    if recorded_service_pids(record)
+        .iter()
+        .any(|pid| process_is_running(*pid))
+    {
+        return false;
+    }
+    endpoint_keys::clear_endpoint_api_key(paths, &record.service_id);
+    record.stop_requested_unix_ms = None;
+    true
+}
+
 fn refresh_managed_service_runtime_liveness(
     paths: &AppPaths,
     record: &mut ManagedServiceRecord,
 ) -> bool {
+    let settled_pending_stop = settle_pending_stop_key_cleanup(paths, record);
     if !managed_service_is_live(record) {
-        return false;
+        return settled_pending_stop;
     }
 
     // Probe with the service's key so a protected public service is not mistaken
@@ -13711,22 +13823,23 @@ fn refresh_managed_service_runtime_liveness(
             record.status = "ready".to_owned();
             return true;
         }
-        return false;
+        return settled_pending_stop;
     }
 
-    let tracked_pids = [record.engine_pid, Some(record.supervisor_pid)]
-        .into_iter()
-        .flatten()
-        .filter(|pid| *pid != 0)
-        .collect::<Vec<_>>();
+    let tracked_pids = recorded_service_pids(record);
     let has_tracked_pid = !tracked_pids.is_empty();
     let has_live_pid = tracked_pids.iter().any(|pid| process_is_running(*pid));
     if has_tracked_pid && !has_live_pid {
+        // Reconcile the status only. The endpoint key is *not* dropped here:
+        // dead pids alone do not distinguish a stopped service from a crashed
+        // one, and a crashed public service needs its key to be restartable.
+        // `settle_pending_stop_key_cleanup` above owns that cleanup, gated on a
+        // stop having actually been requested.
         if record.status != "stopped" {
             record.status = "stopped".to_owned();
             return true;
         }
-        return false;
+        return settled_pending_stop;
     }
 
     if matches!(record.status.as_str(), "ready" | "running") {
@@ -13737,7 +13850,7 @@ fn refresh_managed_service_runtime_liveness(
         }
     }
 
-    false
+    settled_pending_stop
 }
 
 fn local_server_sidebar_status(counts: &ManagedServiceSidebarCounts) -> String {
@@ -20579,6 +20692,179 @@ install therock";
         ensure_public_bind_engine_supported("lemonade", false, true).unwrap(); // loopback needs no key
     }
 
+    #[test]
+    fn respawn_fails_closed_for_a_public_service_whose_key_is_gone() {
+        // A stop deletes the key file, so a later restart of a public service
+        // would otherwise respawn it with no auth at all.
+        let error = ensure_public_service_has_endpoint_key("0.0.0.0", false).unwrap_err();
+        let message = error.to_string();
+        assert!(message.contains("0.0.0.0"), "{error:#}");
+        assert!(message.contains("without authentication"), "{error:#}");
+        // Actionable: name the command that mints a fresh key.
+        assert!(message.contains("--allow-public-bind"), "{error:#}");
+
+        // A public service that still has its key restarts normally.
+        ensure_public_service_has_endpoint_key("0.0.0.0", true).unwrap();
+    }
+
+    #[test]
+    fn respawn_allows_loopback_services_without_an_endpoint_key() {
+        // Loopback stays credential-free, so every accepted spelling must pass
+        // the guard with no key present.
+        for host in ["127.0.0.1", "localhost", "::1"] {
+            ensure_public_service_has_endpoint_key(host, false)
+                .unwrap_or_else(|error| panic!("{host} must not require a key: {error:#}"));
+        }
+    }
+
+    #[test]
+    fn restart_refuses_a_public_service_without_a_key_before_stopping_it() {
+        // The guard runs before the stop, so a refused restart must leave the
+        // record exactly as it was rather than taking down a running service.
+        let (root, paths) = test_paths("restart-public-no-key");
+        let service_id = "svc-public-nokey";
+        let mut record = ManagedServiceRecord::new(
+            &paths,
+            service_id,
+            "vllm",
+            "model-ref",
+            "canonical/model",
+            "0.0.0.0",
+            11435,
+            "managed",
+            std::process::id(),
+            None,
+            None,
+            Some("gpu_required".to_owned()),
+        );
+        record.status = "running".to_owned();
+        record.write().unwrap();
+
+        let error = restart_internal_managed_service(&paths, service_id).unwrap_err();
+        assert!(
+            error.to_string().contains("without authentication"),
+            "{error:#}"
+        );
+
+        // Proves the *ordering*, not just the refusal: had the guard run after
+        // `stop_internal_managed_service`, the stop would have written
+        // "stopped". It reaches that state here because the record's only pid is
+        // the test's own (`engine_pid` is None and `terminate_recorded_service_pids`
+        // skips the caller's pid), so the stop confirms termination trivially.
+        let after = load_managed_service(&paths, service_id).unwrap();
+        assert_ne!(
+            after.status, "stopped",
+            "a refused restart must not stop the service"
+        );
+        let _ = fs::remove_dir_all(root);
+    }
+
+    /// A public record with a dead pid and a stored endpoint key, for the
+    /// liveness-refresh cases below. The port is one nothing listens on, so the
+    /// refresh's endpoint probe fails and it falls through to the pid check.
+    fn dead_public_service_with_key(
+        paths: &AppPaths,
+        service_id: &str,
+        port: u16,
+    ) -> ManagedServiceRecord {
+        paths.ensure().unwrap();
+        let mut record = ManagedServiceRecord::new(
+            paths,
+            service_id,
+            "vllm",
+            "model-ref",
+            "canonical/model",
+            "0.0.0.0",
+            port,
+            "managed",
+            // A pid far above any plausible live process, as in
+            // `dead_managed_service_allows_relaunch`.
+            999_999_999,
+            None,
+            None,
+            None,
+        );
+        record.status = "running".to_owned();
+        record.write().unwrap();
+        endpoint_keys::store_endpoint_api_key(paths, service_id, "secret-key").unwrap();
+        record
+    }
+
+    #[test]
+    fn crashed_public_service_keeps_its_endpoint_key() {
+        // A crash (OOM kill, host reboot, panic) leaves the record at "running"
+        // with dead pids and no stop marker. Dropping the key here would make
+        // the fail-closed respawn guards refuse every later `rocm services
+        // restart` and every daemon recovery attempt — permanently, because
+        // nothing can re-mint the key. The refresh happens on every read
+        // (`rocm services list`), so this must survive it.
+        let (root, paths) = test_paths("liveness-crash-keeps-key");
+        let service_id = "svc-crashed-public";
+        let mut record = dead_public_service_with_key(&paths, service_id, 11982);
+
+        let changed = refresh_managed_service_runtime_liveness(&paths, &mut record);
+
+        assert!(changed, "a dead service must be demoted to stopped");
+        assert_eq!(record.status, "stopped");
+        assert_eq!(
+            endpoint_keys::endpoint_api_key(&paths, service_id).as_deref(),
+            Some("secret-key"),
+            "a crashed public service must stay restartable"
+        );
+        let _ = fs::remove_dir_all(root);
+    }
+
+    #[test]
+    fn unconfirmed_stop_clears_the_endpoint_key_once_the_processes_are_gone() {
+        // The other half: a stop that could not confirm termination leaves the
+        // key in place (the engine may still be alive and enforcing it) and
+        // records the intent. Once the processes are observed gone, the deferred
+        // cleanup runs, so no plaintext secret is stranded for a service the
+        // operator did ask to stop.
+        let (root, paths) = test_paths("liveness-pending-stop-clears-key");
+        let service_id = "svc-pending-stop";
+        let mut record = dead_public_service_with_key(&paths, service_id, 11983);
+        record.stop_requested_unix_ms = Some(1);
+
+        let changed = refresh_managed_service_runtime_liveness(&paths, &mut record);
+
+        assert!(changed);
+        assert_eq!(record.status, "stopped");
+        assert_eq!(
+            endpoint_keys::endpoint_api_key(&paths, service_id),
+            None,
+            "a requested stop must still drop the key"
+        );
+        assert_eq!(
+            record.stop_requested_unix_ms, None,
+            "the marker is consumed, so the cleanup does not run again"
+        );
+        let _ = fs::remove_dir_all(root);
+    }
+
+    #[test]
+    fn pending_stop_cleanup_runs_even_once_the_record_reads_stopped() {
+        // Proves the cleanup sits *before* the `managed_service_is_live` gate:
+        // a record that reached "stopped" by another route (an engine state
+        // refresh, a concurrent writer) would otherwise early-return and strand
+        // the key of a service the operator stopped.
+        let (root, paths) = test_paths("liveness-pending-stop-when-stopped");
+        let service_id = "svc-pending-stop-stopped";
+        let mut record = dead_public_service_with_key(&paths, service_id, 11984);
+        record.status = "stopped".to_owned();
+        record.stop_requested_unix_ms = Some(1);
+
+        let changed = refresh_managed_service_runtime_liveness(&paths, &mut record);
+
+        assert!(
+            changed,
+            "consuming the marker is a record change worth writing"
+        );
+        assert_eq!(endpoint_keys::endpoint_api_key(&paths, service_id), None);
+        assert_eq!(record.stop_requested_unix_ms, None);
+        let _ = fs::remove_dir_all(root);
+    }
+
     #[test]
     fn endpoint_client_config_shows_key_once_with_bearer_guidance() {
         let rendered = render_endpoint_client_config("http://0.0.0.0:11435/v1", "secret-123");