fix(serve): make model serving ports collision-safe

README.md

@@ -833,6 +833,22 @@ after a crash inherits the daemon's environment, not yours, so a forwarded
 secret must also be set there. `ROCM_CLI_AIM_READY_TIMEOUT_SECS` raises the
 5-minute readiness wait for a cold-start model download.
 
+`--port` is optional on the default loopback host (`127.0.0.1`): omit it and
+`rocm serve` picks the first free port in `11435`-`11535`, checking both
+rocm-cli's own records and the OS. The non-interactive plan output (piped
+output, or the Dash approval card) shows `Port: automatic; endpoint shown
+after launch` while the launch is in flight and prints the concrete endpoint
+once the port is actually leased; the interactive deployment summary table
+only ever shows the final resolved endpoint. Pass `--port` explicitly to pin
+one; on `127.0.0.1`, an explicit loopback port already reserved by another
+managed service is rejected up front, naming that service and how to free it
+(`rocm services stop <service-id> --yes`). A port held by an untracked process
+is rejected too, but without naming the holder. `--host` to anything other
+than the default
+loopback address (`localhost`, `::1`, `0.0.0.0`, or any other address)
+requires an explicit `--port`: automatic selection only probes `127.0.0.1` and
+cannot promise a free port anywhere else.
+
 `--gpu-memory-utilization` (a fraction above 0.0 and up to 1.0 of each GPU's
 total VRAM) and `--tool-call-parser` (for example `hermes`, `llama3_json`, or
 `mistral`, which also turns on automatic tool choice) apply to vLLM only. Lower
@@ -861,6 +877,10 @@ different settings. If you previously started a service with `--temperature`
 flags, or with different ones, stop the existing service first (`rocm
 services stop <service-id> --yes`) or match the original flags.
 
+Likewise, an explicit `--port` that differs from the port the running service
+is already on is refused rather than silently ignored; omit `--port` to reuse
+the running service, or stop it first to serve on the new port.
+
 By default the server runs in the background under ROCm CLI's supervision and
 prints a deployment summary: a progress indicator while it starts, then a table
 with the status, the full inference endpoint, the API-qualified model name, and a

apps/rocm/src/chat.rs

@@ -2243,6 +2243,20 @@ fn validate_chat_launch_server_tool_call(call: &providers::ChatToolCall) -> Resu
     if !is_loopback_host(&host) {
         bail!("local assistant cannot request non-local host `{host}`");
     }
+    // A malformed `port` must be rejected here, not silently dropped by the
+    // argv builder and left to fall through to automatic port selection.
+    let port = rocm_core::launch_port_argument(object.get("port"))
+        .map_err(|reason| anyhow::anyhow!("ROCm tool `launch_server` argument `port` {reason}"))?;
+    // `rocm serve` proves a port is free only on the exact default host; every
+    // other spelling (including the loopback aliases `localhost`/`::1` this
+    // check still accepts) needs an explicit port or the CLI call refuses
+    // later with a less actionable error. See `is_canonical_loopback_host`.
+    if port.is_none() && !rocm_core::is_canonical_loopback_host(&host) {
+        bail!(
+            "local assistant must supply an explicit `port` for host `{host}`; automatic port \
+             selection is only supported on the default host {DEFAULT_LOCAL_HOST}"
+        );
+    }
     if object
         .get("device")
         .and_then(serde_json::Value::as_str)
@@ -2742,6 +2756,16 @@ pub(crate) fn validate_chat_rocm_command_safety(args: &[String]) -> Result<()> {
         {
             bail!("local assistant cannot request non-local host `{host}`");
         }
+        // The same rule `rocm serve` applies, so an approval card is never offered
+        // for a command the CLI then refuses.
+        let port = chat_cli_arg_value(args, "--port")
+            .map(crate::serve_port::parse_serve_port)
+            .transpose()
+            .map_err(anyhow::Error::msg)?;
+        crate::serve_port::validate_port_request(
+            chat_cli_arg_value(args, "--host").unwrap_or(rocm_core::DEFAULT_LOCAL_HOST),
+            crate::serve_port::PortRequest::from_flag(port),
+        )?;
         if chat_cli_has_flag(args, "--foreground") || !chat_cli_has_flag(args, "--managed") {
             bail!("local assistant must request managed serving with --managed");
         }
@@ -6056,6 +6080,26 @@ model recipes
             .to_string();
         assert!(error.contains("public network binding"));
 
+        let no_port_on_custom_host = providers::ChatToolCall {
+            id: None,
+            name: "rocm_command".to_owned(),
+            arguments: serde_json::json!({
+                "args": ["serve", "qwen", "--host", "localhost", "--managed"]
+            }),
+        };
+        let error = validate_chat_tool_call(&no_port_on_custom_host)
+            .unwrap_err()
+            .to_string();
+        assert!(error.contains("explicit `--port"), "{error}");
+        validate_chat_tool_call(&providers::ChatToolCall {
+            id: None,
+            name: "rocm_command".to_owned(),
+            arguments: serde_json::json!({
+                "args": ["serve", "qwen", "--host", "localhost", "--port", "8000", "--managed"]
+            }),
+        })
+        .expect("an explicit port on a loopback host is allowed");
+
         let foreground = providers::ChatToolCall {
             id: None,
             name: "rocm_command".to_owned(),
@@ -6392,6 +6436,92 @@ model recipes
         assert!(error.contains("non-local host"));
     }
 
+    #[test]
+    fn chat_tool_call_requires_an_explicit_port_for_non_canonical_loopback_hosts() {
+        let no_port = providers::ChatToolCall {
+            id: None,
+            name: "launch_server".to_owned(),
+            arguments: serde_json::json!({
+                "model": "tiny.gguf",
+                "engine": "vllm",
+                "host": "localhost"
+            }),
+        };
+        let error = validate_chat_tool_call(&no_port).unwrap_err().to_string();
+        assert!(
+            error.contains("explicit `port`"),
+            "unexpected error: {error}"
+        );
+
+        let with_port = providers::ChatToolCall {
+            id: None,
+            name: "launch_server".to_owned(),
+            arguments: serde_json::json!({
+                "model": "tiny.gguf",
+                "engine": "vllm",
+                "host": "localhost",
+                "port": 11999
+            }),
+        };
+        validate_chat_tool_call(&with_port).expect("explicit port on localhost is allowed");
+
+        let canonical_no_port = providers::ChatToolCall {
+            id: None,
+            name: "launch_server".to_owned(),
+            arguments: serde_json::json!({
+                "model": "tiny.gguf",
+                "engine": "vllm",
+                "host": "127.0.0.1"
+            }),
+        };
+        validate_chat_tool_call(&canonical_no_port)
+            .expect("automatic port selection still works on the canonical loopback host");
+    }
+
+    #[test]
+    fn chat_tool_call_rejects_a_malformed_port_instead_of_letting_it_fall_through() {
+        // A non-integer `port` must not be treated as "no port supplied" here:
+        // the argv builder (`object.get("port").and_then(as_u64)`) would drop
+        // it just as silently, so without this check a malformed port on a
+        // non-canonical host would sail past validation only to fall through
+        // to automatic port selection the host doesn't support.
+        let string_port = providers::ChatToolCall {
+            id: None,
+            name: "launch_server".to_owned(),
+            arguments: serde_json::json!({
+                "model": "tiny.gguf",
+                "engine": "vllm",
+                "host": "localhost",
+                "port": "11999"
+            }),
+        };
+        let error = validate_chat_tool_call(&string_port)
+            .unwrap_err()
+            .to_string();
+        assert!(
+            error.contains("must be an integer"),
+            "unexpected error: {error}"
+        );
+
+        let out_of_range_port = providers::ChatToolCall {
+            id: None,
+            name: "launch_server".to_owned(),
+            arguments: serde_json::json!({
+                "model": "tiny.gguf",
+                "engine": "vllm",
+                "host": "127.0.0.1",
+                "port": 70000
+            }),
+        };
+        let error = validate_chat_tool_call(&out_of_range_port)
+            .unwrap_err()
+            .to_string();
+        assert!(
+            error.contains("between 1 and 65535"),
+            "unexpected error: {error}"
+        );
+    }
+
     #[test]
     fn chat_tool_call_rejects_bad_install_suggestions() {
         for (arguments, expected) in [

apps/rocm/src/main.rs

@@ -38,6 +38,7 @@ mod runtime_install_finalize;
 mod runtime_services;
 mod runtimes_cmd;
 mod serve_cmd;
+mod serve_port;
 mod serve_summary;
 mod service_hints;
 mod service_launch;
@@ -527,9 +528,16 @@ rocm serve qwen --verbose --device gpu_required")]
         /// Host address to bind.
         #[arg(long, default_value = DEFAULT_LOCAL_HOST)]
         host: String,
-        /// TCP port to bind.
-        #[arg(long, default_value_t = rocm_core::DEFAULT_LOCAL_PORT)]
-        port: u16,
+        /// TCP port to bind. Omit it to choose an available port automatically,
+        /// starting at 11435, on the default 127.0.0.1 host. Any other --host
+        /// requires an explicit --port: proactive collision detection is claimed
+        /// only for local 127.0.0.1 serving.
+        // `allow_hyphen_values` so a negative value such as `-1` reaches
+        // `parse_serve_port` and gets the domain error naming the valid range,
+        // instead of clap rejecting it as an unknown flag. Mirrors the same
+        // choice made for `--gpu-memory-utilization` below.
+        #[arg(long, value_parser = serve_port::parse_serve_port, allow_hyphen_values = true)]
+        port: Option<u16>,
         /// Attach to the server in this terminal and stream its logs (Ctrl-D to
         /// detach and leave it running, Ctrl-C to stop). Same as --verbose.
         #[arg(long, conflicts_with = "managed")]
@@ -4561,8 +4569,11 @@ fn apply_service_prune_plan(
 /// window — [`select_gpu_indices_under_launch_lock`] acquires it, hands the
 /// guard back to its caller, and `start_managed_service`/`run_attached_service`
 /// only drop it once the record is persisted (see that function's own comment,
-/// which this one must not drift from). The gap was simply that prune never
-/// acquired it: the helper above was the single acquirer in the tree. Taking it
+/// which this one must not drift from). `restart_internal_managed_service_with`
+/// acquires the same lock around its own respawn, for the same reason. The gap
+/// was simply that prune never acquired it, while those two already did
+/// (`rocmd`'s recovery path takes the same file lock too, bounded, from its own
+/// process — see `restart_managed_service`'s doc comment). Taking it
 /// here closes the window deterministically, for any number of records, which no
 /// age floor can — the window is not short and is not bounded, because
 /// `spawn_managed_engine_child`'s idempotency guard runs `load_managed_services`
@@ -4620,9 +4631,9 @@ fn apply_service_prune_plan(
 /// **No nested acquire.** `FileLock::acquire` blocks with no `try_` variant, so
 /// a second acquire on this same lock path while this process already holds
 /// it would hang forever. Nothing reachable from either phase re-acquires
-/// `managed_launch_lock_path`: the only other production call site on this
-/// same path is `select_gpu_indices_under_launch_lock`, which is only ever
-/// reached from `serve`, and `serve` never runs prune in-process or as a
+/// `managed_launch_lock_path`: the other production acquirers of this
+/// same path are `select_gpu_indices_under_launch_lock` (reached only from
+/// `serve`) and `rocm services restart`, and neither runs prune in-process or as a
 /// subprocess (test code acquires this path directly too, but that is test
 /// setup, not something reachable from this function's own call graph).
 ///
@@ -7333,6 +7344,21 @@ pub(crate) mod tests {
             .collect()
     }
 
+    fn serve_arg_help(arg_id: &str) -> String {
+        let mut cli = Cli::command();
+        let serve = cli
+            .find_subcommand_mut("serve")
+            .expect("serve subcommand exists");
+        let arg = serve
+            .get_arguments()
+            .find(|arg| arg.get_id().as_str() == arg_id)
+            .unwrap_or_else(|| panic!("serve has no `{arg_id}` argument"));
+        arg.get_long_help()
+            .map(ToString::to_string)
+            .or_else(|| arg.get_help().map(ToString::to_string))
+            .unwrap_or_default()
+    }
+
     // Both `--engine` and `--device` restrict their input to a fixed set via a
     // clap `value_parser`/`ValueEnum`, so invalid input is rejected as a usage
     // error (exit code 2) with the accepted choices listed. Values are advertised
@@ -7515,6 +7541,77 @@ pub(crate) mod tests {
         }
     }
 
+    #[test]
+    fn serve_without_port_flag_requests_automatic_selection() {
+        let cli = parse_serve(&[]).expect("bare serve parses");
+        match cli.command {
+            Some(Command::Serve { port, host, .. }) => {
+                assert_eq!(port, None, "omitted --port must stay unresolved");
+                assert_eq!(host, DEFAULT_LOCAL_HOST);
+                assert!(serve_port::PortRequest::from_flag(port).is_auto());
+            }
+            other => panic!("expected Serve, got {other:?}"),
+        }
+    }
+
+    #[test]
+    fn serve_with_explicit_port_keeps_it_concrete() {
+        let cli = parse_serve(&["--port", "8000"]).expect("explicit port parses");
+        match cli.command {
+            Some(Command::Serve { port, .. }) => {
+                assert_eq!(port, Some(8000));
+                assert_eq!(
+                    serve_port::PortRequest::from_flag(port),
+                    serve_port::PortRequest::Explicit(8000)
+                );
+            }
+            other => panic!("expected Serve, got {other:?}"),
+        }
+    }
+
+    #[test]
+    fn serve_rejects_a_zero_or_out_of_range_port() {
+        for bad in ["0", "65536", "-1", "auto"] {
+            let error = parse_serve(&["--port", bad]).expect_err("must be rejected");
+            assert!(
+                error.to_string().contains("between 1 and 65535"),
+                "unexpected error for --port {bad}: {error}"
+            );
+        }
+    }
+
+    #[test]
+    fn hidden_engine_serve_command_keeps_a_concrete_default_port() {
+        // Only the user-facing verb gained automatic selection; the direct engine
+        // entry point still requires (and defaults to) a concrete port.
+        let cli = Cli::try_parse_from([
+            "rocm",
+            "__engine-serve-http",
+            "lemonade",
+            "svc-1",
+            "qwen",
+            "--state-path",
+            "state.json",
+        ])
+        .expect("hidden engine command parses");
+        match cli.command {
+            Some(Command::EngineServeHttp { port, host, .. }) => {
+                assert_eq!(port, rocm_core::DEFAULT_LOCAL_PORT);
+                assert_eq!(host, DEFAULT_LOCAL_HOST);
+            }
+            other => panic!("expected EngineServeHttp, got {other:?}"),
+        }
+    }
+
+    #[test]
+    fn serve_port_help_documents_omission_and_the_custom_host_rule() {
+        let help = serve_arg_help("port");
+        assert!(help.contains("11435"), "{help}");
+        assert!(help.contains("automatically"), "{help}");
+        assert!(help.contains("127.0.0.1"), "{help}");
+        assert!(help.contains("explicit --port"), "{help}");
+    }
+
     #[test]
     fn serve_verbose_conflicts_with_managed() {
         // `--verbose` streams logs in the foreground; a backgrounded managed