diff --git a/crates/rocm-dash-tui/src/app/chat.rs b/crates/rocm-dash-tui/src/app/chat.rs index 8b1d7480..54f70010 100644 --- a/crates/rocm-dash-tui/src/app/chat.rs +++ b/crates/rocm-dash-tui/src/app/chat.rs @@ -228,16 +228,24 @@ pub(super) async fn detect_managed_chat( /// Lemonade/vLLM endpoints when no managed service is available. pub(super) async fn detect_local_chat( executor: Option, +) -> Option { + detect_local_chat_with_probe(executor, crate::llm::detect_local_endpoint).await +} + +/// Same as [`detect_local_chat`], but with the well-known-port probe passed +/// in rather than hard-coded, so a test can substitute a deterministic probe +/// instead of depending on the real well-known ports being free on the +/// machine running the test. +async fn detect_local_chat_with_probe( + executor: Option, + probe: impl Fn() -> Option<&'static str> + Send + 'static, ) -> Option { if let Some(cfg) = detect_managed_chat(executor).await { return Some(cfg); } // TCP probe is blocking; keep it off the async reactor. - let base = tokio::task::spawn_blocking(crate::llm::detect_local_endpoint) - .await - .ok() - .flatten()?; + let base = tokio::task::spawn_blocking(probe).await.ok().flatten()?; // Best-effort model query; fall back to the neutral default on any failure. let model = fetch_first_model(base) @@ -246,6 +254,44 @@ pub(super) async fn detect_local_chat( Some(crate::llm::detected_llm_config(base, &model)) } +/// Whether startup should run the local-engine auto-detection swap. +/// +/// Detection produces a keyless [`crate::llm::detected_llm_config`], so it may +/// only fire when the user configured NO endpoint (URL/env URL) AND NO api key +/// — otherwise the swap would silently drop a configured credential and 401 at +/// request time. Pure; the anchor for the startup-gate unit test. +pub(super) const fn should_detect_local_chat( + chat_url: Option<&str>, + chat_env_url: Option<&str>, + chat_api_key: Option<&str>, +) -> bool { + chat_url.is_none() && chat_env_url.is_none() && chat_api_key.is_none() +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(super) enum StartupChatOutcome { + Local, + OAuth, + Configured, +} + +/// Select the startup backend path after optional local detection. +/// +/// A successful detection is already reachable. A detection miss means the +/// well-known endpoints were exhausted and startup should proceed to OAuth. +/// When detection was gated off, the configured target still needs its normal +/// liveness probe before backend construction. +pub(super) const fn startup_chat_outcome( + detection_ran: bool, + detected: bool, +) -> StartupChatOutcome { + match (detection_ran, detected) { + (_, true) => StartupChatOutcome::Local, + (true, false) => StartupChatOutcome::OAuth, + (false, false) => StartupChatOutcome::Configured, + } +} + /// Persist an accepted local endpoint to the user's `config.toml`: load the /// existing config (or defaults), set `tui.chat_url`/`tui.chat_model`, and write /// it back. Best-effort — returns a human error string on failure. @@ -408,4 +454,64 @@ mod tests { let env = services_envelope(serde_json::json!([service("vllm", "", "ready", "m", 100)])); assert_eq!(pick_managed_chat_endpoint(&env), None); } + + /// EAI-7347: startup now reuses `detect_local_chat` (the same detection the + /// manual 'd' path already used) instead of a single well-known-port probe, + /// so a local server on a non-default well-known port (e.g. `rocm serve`'s + /// :11435) is preferred over falling back to the ChatGPT cloud default. + #[tokio::test] + async fn detect_local_chat_wires_probed_endpoint_through() { + // Priority order among the well-known ports (Lemonade > vLLM > rocm + // serve's default) is covered deterministically in `llm.rs`'s own + // tests, against OS-assigned ephemeral ports. This test's job is + // narrower: prove `detect_local_chat`'s no-managed-executor path + // wires whatever the well-known-port probe finds into the resulting + // config, rather than falling back to the cloud default — so the + // probe is injected rather than exercised against real ports (real + // ports it doesn't own would make this flake on a CI runner where an + // ambient listener happens to occupy one of them). + const PROBED: &str = "http://127.0.0.1:1/v1"; + let cfg = detect_local_chat_with_probe(None, || Some(PROBED)) + .await + .expect("local server detected"); + assert_eq!(cfg.base_url, PROBED); + } + + #[test] + fn should_detect_only_when_no_url_env_or_key_configured() { + // The clean slate: nothing configured → auto-detect a local engine. + assert!(should_detect_local_chat(None, None, None)); + // A configured api key must SUPPRESS the swap — detection returns a + // keyless config and would otherwise silently drop the key (401). + assert!(!should_detect_local_chat(None, None, Some("sk-configured"))); + // An explicit URL or env URL also suppresses it (config precedence). + assert!(!should_detect_local_chat( + Some("http://cfg:1/v1"), + None, + None + )); + assert!(!should_detect_local_chat( + None, + Some("http://env:2/v1"), + None + )); + } + + #[test] + fn startup_uses_detected_local_endpoint() { + assert_eq!(startup_chat_outcome(true, true), StartupChatOutcome::Local); + } + + #[test] + fn startup_uses_oauth_after_empty_detection() { + assert_eq!(startup_chat_outcome(true, false), StartupChatOutcome::OAuth); + } + + #[test] + fn startup_uses_configured_endpoint_when_detection_is_skipped() { + assert_eq!( + startup_chat_outcome(false, false), + StartupChatOutcome::Configured + ); + } } diff --git a/crates/rocm-dash-tui/src/app/mod.rs b/crates/rocm-dash-tui/src/app/mod.rs index 0e562662..64cb4da9 100644 --- a/crates/rocm-dash-tui/src/app/mod.rs +++ b/crates/rocm-dash-tui/src/app/mod.rs @@ -39,8 +39,8 @@ mod slash; mod summary; use chat::{ - build_chat_agent, build_local_agent, detect_local_chat, detect_managed_chat, - persist_chat_endpoint, + StartupChatOutcome, build_chat_agent, build_local_agent, detect_local_chat, + persist_chat_endpoint, startup_chat_outcome, }; use summary::{parse_plan_result, summarize_json_value, summarize_slash_tool}; @@ -1562,8 +1562,34 @@ async fn event_loop(terminal: &mut Tui, args: &ResolvedArgs) -> color_eyre::Resu // NOT override an explicitly configured `chat_url`/env URL, so config // precedence is preserved (we only consult the registry when neither is // set, i.e. where the well-known default would otherwise be probed). - let managed = if args.chat_url.is_none() && args.chat_env_url.is_none() { - detect_managed_chat(state.tool_executor.clone()).await + // When neither an explicit URL (CLI/config) nor an env URL is set, run + // the SAME full local-engine detection the manual 'd' path uses: + // registry-first (an engine we launched ourselves, on whatever port it + // bound), then a probe of the well-known Lemonade/vLLM/rocm-serve + // ports (parallelized — see `llm::detect_local_endpoint` — so a cold + // start with no server doesn't pay 3x the probe timeout), plus a + // best-effort served-model fetch. This is what lets a local server win + // over the ChatGPT cloud default at startup instead of only the single + // well-known :8000 port that a bare `resolve_llm_config` probe covers. + // + // NOTE: unmerged PR #97 also touches this branch (model discovery when + // `chat_model` is None, inside `resolve_llm_config`'s own fallback + // path) — this change is conflict-minimal by leaving the + // `resolve_llm_config` call below untouched. + // + // Gate on `chat_api_key.is_none()` too: local detection returns a + // keyless `detected_llm_config` (api_key/auth_header forced to None), + // so firing it when the user configured a key would SILENTLY DROP that + // key and 401 at request time. A configured key means "use my + // configured backend", so skip the swap and let `resolve_llm_config` + // carry the key through its normal precedence. + let detection_ran = chat::should_detect_local_chat( + args.chat_url.as_deref(), + args.chat_env_url.as_deref(), + args.chat_api_key.as_deref(), + ); + let detected = if detection_ran { + detect_local_chat(state.tool_executor.clone()).await } else { None }; @@ -1572,17 +1598,23 @@ async fn event_loop(terminal: &mut Tui, args: &ResolvedArgs) -> color_eyre::Resu .clone() .or_else(|| args.chat_env_url.clone()) .unwrap_or_else(|| crate::llm::DEFAULT_CHAT_BASE_URL.to_string()); - // A managed endpoint is already readiness-verified; otherwise TCP-probe. - let probe_ok = if managed.is_some() { - true - } else { - tokio::task::spawn_blocking(move || { + // A detected endpoint (managed or probed) is already verified. When + // detection ran and found nothing it already probed the well-known + // vLLM :8000 port (== `DEFAULT_CHAT_BASE_URL`), so re-probing the same + // fallback target here is redundant and just burns another probe + // timeout on a cold start — treat that as unreachable directly. + // Otherwise (an explicit URL/env/key path) TCP-probe the target. + let startup_outcome = startup_chat_outcome(detection_ran, detected.is_some()); + let probe_ok = match startup_outcome { + StartupChatOutcome::Local => true, + StartupChatOutcome::OAuth => false, + StartupChatOutcome::Configured => tokio::task::spawn_blocking(move || { crate::llm::probe_endpoint(&probe_target, crate::llm::PROBE_TIMEOUT) }) .await - .unwrap_or(false) + .unwrap_or(false), }; - let llm = managed.or_else(|| { + let llm = detected.or_else(|| { crate::llm::resolve_llm_config( args.chat_url.as_deref(), args.chat_model.as_deref(), @@ -1599,10 +1631,7 @@ async fn event_loop(terminal: &mut Tui, args: &ResolvedArgs) -> color_eyre::Resu // ChatGPT OAuth default (device-code login surfaced in the chat tab). // This restores the no-key login the vendored Codex path provided; it // takes NO api_key (env-only invariant untouched — OAuth, not a key). - let no_key_no_endpoint = !probe_ok - && args.chat_api_key.is_none() - && args.chat_url.is_none() - && args.chat_env_url.is_none(); + let no_key_no_endpoint = startup_outcome == StartupChatOutcome::OAuth; if no_key_no_endpoint { let oauth_tx = chat_tx.clone(); crate::agent::ChatGptAgentClient::new( diff --git a/crates/rocm-dash-tui/src/llm.rs b/crates/rocm-dash-tui/src/llm.rs index 8c449be2..e61ad1ec 100644 --- a/crates/rocm-dash-tui/src/llm.rs +++ b/crates/rocm-dash-tui/src/llm.rs @@ -188,20 +188,49 @@ pub fn probe_endpoint(base_url: &str, timeout: Duration) -> bool { false } -/// Probe the known local serving endpoints in priority order (Lemonade, then -/// vLLM, then `rocm serve`'s non-managed default) and return the first -/// reachable one. +/// Probe `candidates` (in priority order) concurrently and return the +/// highest-priority one that answered. /// -/// Used by the TUI's in-app "detect a local engine" action so the user need not run the CLI skill first. -/// TCP-only (no HTTP); never blocks longer than [`PROBE_TIMEOUT`] per candidate. +/// Split out from [`detect_local_endpoint`] so priority-order behavior is +/// testable against OS-assigned ephemeral ports instead of the real +/// well-known ports, which may be occupied by an unrelated ambient listener +/// on the machine running the test. Probing all candidates in parallel +/// (rather than sequentially) bounds the worst case (no server listening) to +/// one [`PROBE_TIMEOUT`] instead of one per candidate. TCP-only (no HTTP); +/// never blocks longer than [`PROBE_TIMEOUT`] total. +fn probe_first_reachable<'a, P>(candidates: &[&'a str], probe: P) -> Option<&'a str> +where + P: Fn(&str, Duration) -> bool + Sync, +{ + std::thread::scope(|scope| { + // The collect is load-bearing, not needless: every probe must be + // *spawned* before any is *joined*, or they run one at a time and the + // whole point (bounding total latency to one PROBE_TIMEOUT) is lost. + #[allow(clippy::needless_collect)] + let handles: Vec<_> = candidates + .iter() + .map(|ep| (*ep, scope.spawn(|| probe(ep, PROBE_TIMEOUT)))) + .collect(); + handles + .into_iter() + .find_map(|(ep, handle)| handle.join().unwrap_or(false).then_some(ep)) + }) +} + +const LOCAL_ENDPOINT_CANDIDATES: [&str; 3] = [ + crate::skills::LEMONADE_ENDPOINT, + crate::skills::VLLM_ENDPOINT, + crate::skills::ROCM_SERVE_ENDPOINT, +]; + +/// Probe the known local serving endpoints (Lemonade, vLLM, then `rocm +/// serve`'s non-managed default) concurrently and return the highest-priority +/// one that answered. +/// +/// Used by the TUI's in-app "detect a local engine" action, and by chat +/// startup, so the user need not run the CLI skill first. pub fn detect_local_endpoint() -> Option<&'static str> { - [ - crate::skills::LEMONADE_ENDPOINT, - crate::skills::VLLM_ENDPOINT, - crate::skills::ROCM_SERVE_ENDPOINT, - ] - .into_iter() - .find(|ep| probe_endpoint(ep, PROBE_TIMEOUT)) + probe_first_reachable(&LOCAL_ENDPOINT_CANDIDATES, probe_endpoint) } /// Pick the first model id from an OpenAI-compatible `/v1/models` response (`{"data":[{"id":"…"}]}`). @@ -438,20 +467,83 @@ mod tests { assert!(!probe_endpoint("not a url", Duration::from_millis(100))); } + /// Binds an OS-assigned ephemeral port and returns its listener (kept + /// alive so the port stays reachable) plus its `http://host:port` URL. + /// + /// Ephemeral ports are used instead of the real well-known ports + /// (Lemonade/vLLM/rocm serve) so priority-order tests are deterministic: + /// an ambient listener elsewhere on the machine (e.g. a real local + /// engine, or — on some CI runners — a pre-installed one) can otherwise + /// make well-known-port-based tests flake by answering a probe the test + /// didn't intend to exercise. + fn ephemeral_endpoint() -> (std::net::TcpListener, String) { + let listener = std::net::TcpListener::bind(("127.0.0.1", 0)).expect("bind ephemeral port"); + let url = format!("http://{}", listener.local_addr().expect("local_addr")); + (listener, url) + } + #[test] - fn detect_local_endpoint_probes_rocm_serve_default_port() { - // A listener on rocm serve's (non-managed) default port must be picked - // up by the fallback probe, not just Lemonade/vLLM's ports. - let Ok(listener) = - std::net::TcpListener::bind(("127.0.0.1", crate::skills::ROCM_SERVE_PORT)) - else { - // Port already bound in this environment; skip rather than flake. - return; + fn probe_first_reachable_prefers_earlier_candidate_when_multiple_reachable() { + // Two reachable candidates: priority order (first in the list) must + // win even though probes run concurrently, not sequentially. + let (_first, first_url) = ephemeral_endpoint(); + let (_second, second_url) = ephemeral_endpoint(); + assert_eq!( + probe_first_reachable(&[first_url.as_str(), second_url.as_str()], probe_endpoint), + Some(first_url.as_str()) + ); + } + + #[test] + fn probe_first_reachable_skips_unreachable_earlier_candidates() { + // The first candidate (port 1) is essentially never open; the probe + // must fall through to the second, reachable one. + let (_listener, url) = ephemeral_endpoint(); + assert_eq!( + probe_first_reachable(&["http://127.0.0.1:1", url.as_str()], probe_endpoint,), + Some(url.as_str()) + ); + } + + #[test] + fn probe_first_reachable_none_when_nothing_listening() { + assert_eq!( + probe_first_reachable(&["http://127.0.0.1:1"], probe_endpoint), + None + ); + } + + #[test] + fn probe_first_reachable_starts_all_probes_concurrently() { + use std::sync::atomic::{AtomicUsize, Ordering}; + + let active = AtomicUsize::new(0); + let max_active = AtomicUsize::new(0); + let probe = |_: &str, _: Duration| { + let now_active = active.fetch_add(1, Ordering::SeqCst) + 1; + max_active.fetch_max(now_active, Ordering::SeqCst); + std::thread::sleep(Duration::from_millis(50)); + active.fetch_sub(1, Ordering::SeqCst); + false }; + + assert_eq!(probe_first_reachable(&["a", "b", "c"], probe), None); + assert_eq!( + max_active.load(Ordering::SeqCst), + 3, + "all probes must be in flight before any probe is joined" + ); + } + + #[test] + fn local_endpoint_candidates_preserve_supported_order() { assert_eq!( - detect_local_endpoint(), - Some(crate::skills::ROCM_SERVE_ENDPOINT) + LOCAL_ENDPOINT_CANDIDATES, + [ + crate::skills::LEMONADE_ENDPOINT, + crate::skills::VLLM_ENDPOINT, + crate::skills::ROCM_SERVE_ENDPOINT, + ] ); - drop(listener); } }