From ff12cbbcc73337c1bf4fd7782273aa1eb6bda7ba Mon Sep 17 00:00:00 2001 From: Michael Roy Date: Fri, 10 Jul 2026 17:16:45 -0700 Subject: [PATCH 1/6] fix(dash): prefer a local chat server over the cloud default at startup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Startup only single-probed DEFAULT_CHAT_BASE_URL (:8000) before falling back to the ChatGPT cloud OAuth flow, even though the manual 'd' detect path already knew how to find a local engine on any of its well-known ports (Lemonade :13305, vLLM :8000, rocm serve :11435) plus the managed-services registry. A server running on any port other than :8000 was invisible at startup and users landed in the cloud login flow instead. Reuse detect_local_chat() (registry-first, then the 3-port probe, then a best-effort model fetch) in the startup branch that already gated on "no explicit chat_url/env_url configured" — the same condition that previously ran only detect_managed_chat(). Parallelize llm::detect_local_endpoint()'s port probe (std::thread::scope) so a cold start with nothing listening still costs one PROBE_TIMEOUT (~300ms) instead of three, keeping startup latency bounded. Unmerged PR #97 also touches this startup branch (model discovery inside resolve_llm_config's own fallback when chat_model is None); this change is conflict-minimal — it swaps detect_managed_chat for detect_local_chat without touching the resolve_llm_config call itself. Relates to EAI-7347. Signed-off-by: Michael Roy --- crates/rocm-dash-tui/src/app/chat.rs | 20 ++++++++ crates/rocm-dash-tui/src/app/mod.rs | 30 ++++++++---- crates/rocm-dash-tui/src/llm.rs | 68 ++++++++++++++++++++++++---- 3 files changed, 100 insertions(+), 18 deletions(-) diff --git a/crates/rocm-dash-tui/src/app/chat.rs b/crates/rocm-dash-tui/src/app/chat.rs index 8b1d7480..1312579b 100644 --- a/crates/rocm-dash-tui/src/app/chat.rs +++ b/crates/rocm-dash-tui/src/app/chat.rs @@ -408,4 +408,24 @@ 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_prefers_local_server_over_no_endpoint() { + let Ok(listener) = + std::net::TcpListener::bind(("127.0.0.1", crate::skills::ROCM_SERVE_PORT)) + else { + return; // Port already bound in this environment; skip rather than flake. + }; + // No executor (no managed-services registry available) — falls through + // to the well-known-port probe, which must still find the listener. + let cfg = detect_local_chat(None) + .await + .expect("local server detected"); + assert_eq!(cfg.base_url, crate::skills::ROCM_SERVE_ENDPOINT); + drop(listener); + } } diff --git a/crates/rocm-dash-tui/src/app/mod.rs b/crates/rocm-dash-tui/src/app/mod.rs index a09daa65..a73ef527 100644 --- a/crates/rocm-dash-tui/src/app/mod.rs +++ b/crates/rocm-dash-tui/src/app/mod.rs @@ -38,10 +38,7 @@ mod chat; mod slash; mod summary; -use chat::{ - build_chat_agent, build_local_agent, detect_local_chat, detect_managed_chat, - persist_chat_endpoint, -}; +use chat::{build_chat_agent, build_local_agent, detect_local_chat, persist_chat_endpoint}; use summary::{parse_plan_result, summarize_json_value, summarize_slash_tool}; /// Which single flow a *focused host* runs. @@ -1562,8 +1559,22 @@ 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. + let detected = if args.chat_url.is_none() && args.chat_env_url.is_none() { + detect_local_chat(state.tool_executor.clone()).await } else { None }; @@ -1572,8 +1583,9 @@ 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() { + // A detected endpoint (managed or probed) is already verified; + // otherwise TCP-probe the single explicitly configured URL. + let probe_ok = if detected.is_some() { true } else { tokio::task::spawn_blocking(move || { @@ -1582,7 +1594,7 @@ async fn event_loop(terminal: &mut Tui, args: &ResolvedArgs) -> color_eyre::Resu .await .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(), diff --git a/crates/rocm-dash-tui/src/llm.rs b/crates/rocm-dash-tui/src/llm.rs index 8c449be2..53774afc 100644 --- a/crates/rocm-dash-tui/src/llm.rs +++ b/crates/rocm-dash-tui/src/llm.rs @@ -188,20 +188,35 @@ 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 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 so the user need not run the CLI skill first. -/// TCP-only (no HTTP); never blocks longer than [`PROBE_TIMEOUT`] per candidate. +/// 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. Probing all three +/// candidates in parallel (rather than sequentially) bounds the worst case +/// (no server listening) to one [`PROBE_TIMEOUT`] instead of three, which +/// matters at startup where this gates falling back to the cloud default. +/// TCP-only (no HTTP); never blocks longer than [`PROBE_TIMEOUT`] total. pub fn detect_local_endpoint() -> Option<&'static str> { - [ + const CANDIDATES: [&str; 3] = [ crate::skills::LEMONADE_ENDPOINT, crate::skills::VLLM_ENDPOINT, crate::skills::ROCM_SERVE_ENDPOINT, - ] - .into_iter() - .find(|ep| probe_endpoint(ep, PROBE_TIMEOUT)) + ]; + 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_endpoint(ep, PROBE_TIMEOUT)))) + .collect(); + handles + .into_iter() + .find_map(|(ep, handle)| handle.join().unwrap_or(false).then_some(ep)) + }) } /// Pick the first model id from an OpenAI-compatible `/v1/models` response (`{"data":[{"id":"…"}]}`). @@ -438,6 +453,41 @@ mod tests { assert!(!probe_endpoint("not a url", Duration::from_millis(100))); } + #[test] + fn detect_local_endpoint_prefers_lemonade_when_multiple_reachable() { + // Lemonade and vLLM both listening: priority order (Lemonade first) + // must win even though probes run concurrently, not sequentially. + let Ok(lemonade) = std::net::TcpListener::bind(("127.0.0.1", crate::skills::LEMONADE_PORT)) + else { + return; // Port already bound in this environment; skip rather than flake. + }; + let Ok(vllm) = std::net::TcpListener::bind(("127.0.0.1", crate::skills::VLLM_PORT)) else { + drop(lemonade); + return; + }; + assert_eq!( + detect_local_endpoint(), + Some(crate::skills::LEMONADE_ENDPOINT) + ); + drop(lemonade); + drop(vllm); + } + + #[test] + fn detect_local_endpoint_bounded_latency_when_nothing_listening() { + // Regression guard for the startup-latency fix: with no server + // reachable, probing all 3 well-known ports must take roughly one + // PROBE_TIMEOUT (parallel), not three (sequential). Generous margin + // to avoid flaking on loaded CI machines. + let start = std::time::Instant::now(); + let _ = detect_local_endpoint(); + assert!( + start.elapsed() < PROBE_TIMEOUT * 2, + "expected parallel probing to bound latency to ~1 PROBE_TIMEOUT, took {:?}", + start.elapsed() + ); + } + #[test] fn detect_local_endpoint_probes_rocm_serve_default_port() { // A listener on rocm serve's (non-managed) default port must be picked From f3eddfedbf51b1378b1823ba5274fb26516e8741 Mon Sep 17 00:00:00 2001 From: Michael Roy Date: Fri, 10 Jul 2026 17:55:57 -0700 Subject: [PATCH 2/6] fix(chat): do not run local-detection swap when an api key is configured MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Startup local-engine detection returns a keyless detected_llm_config (api_key/auth_header forced to None). Firing it whenever no explicit URL was set — even when the user configured OPENAI_API_KEY / ROCMDASH_CHAT_API_KEY — silently dropped that credential and produced a 401 at request time. Gate detection on chat_api_key.is_none() as well (extracted into the pure should_detect_local_chat helper for unit testing): a configured key means "use my configured backend", so skip the swap and let resolve_llm_config carry the key through its normal precedence. Also skip the redundant fallback probe: when detection ran and found nothing it already probed the well-known vLLM :8000 port (== DEFAULT_CHAT_BASE_URL), so re-probing that same target just burned another PROBE_TIMEOUT on a no-server cold start; treat it as unreachable directly. Relates to EAI-7347. Signed-off-by: Michael Roy --- crates/rocm-dash-tui/src/app/chat.rs | 34 ++++++++++++++++++++++++++++ crates/rocm-dash-tui/src/app/mod.rs | 24 +++++++++++++++++--- 2 files changed, 55 insertions(+), 3 deletions(-) diff --git a/crates/rocm-dash-tui/src/app/chat.rs b/crates/rocm-dash-tui/src/app/chat.rs index 1312579b..f8329e6c 100644 --- a/crates/rocm-dash-tui/src/app/chat.rs +++ b/crates/rocm-dash-tui/src/app/chat.rs @@ -246,6 +246,20 @@ 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() +} + /// 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. @@ -428,4 +442,24 @@ mod tests { assert_eq!(cfg.base_url, crate::skills::ROCM_SERVE_ENDPOINT); drop(listener); } + + #[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 + )); + } } diff --git a/crates/rocm-dash-tui/src/app/mod.rs b/crates/rocm-dash-tui/src/app/mod.rs index a73ef527..351b1974 100644 --- a/crates/rocm-dash-tui/src/app/mod.rs +++ b/crates/rocm-dash-tui/src/app/mod.rs @@ -1573,7 +1573,19 @@ async fn event_loop(terminal: &mut Tui, args: &ResolvedArgs) -> color_eyre::Resu // `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. - let detected = if args.chat_url.is_none() && args.chat_env_url.is_none() { + // + // 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 @@ -1583,10 +1595,16 @@ 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 detected endpoint (managed or probed) is already verified; - // otherwise TCP-probe the single explicitly configured URL. + // 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 probe_ok = if detected.is_some() { true + } else if detection_ran { + false } else { tokio::task::spawn_blocking(move || { crate::llm::probe_endpoint(&probe_target, crate::llm::PROBE_TIMEOUT) From 08a4375943300d976dc8f5e177589aaf1591f3f4 Mon Sep 17 00:00:00 2001 From: Michael Roy Date: Fri, 10 Jul 2026 20:46:35 -0700 Subject: [PATCH 3/6] fix(dash): guard well-known ports free in local-detect port-priority tests detect_local_chat_prefers_local_server_over_no_endpoint (and the sibling detect_local_endpoint_probes_rocm_serve_default_port) only bound the rocm-serve port and assumed the higher-priority Lemonade/vLLM ports were unreachable. On the Windows CI runner that isn't guaranteed, so an ambient listener on :13305 or :8000 made detection return that endpoint instead of rocm-serve's, failing the assertion. Both tests now bind-and-release the higher-priority ports first to confirm they're free before asserting on the rocm-serve fallback. Signed-off-by: Michael Roy --- crates/rocm-dash-tui/src/app/chat.rs | 21 +++++++++++++++++++++ crates/rocm-dash-tui/src/llm.rs | 22 +++++++++++++++++++--- 2 files changed, 40 insertions(+), 3 deletions(-) diff --git a/crates/rocm-dash-tui/src/app/chat.rs b/crates/rocm-dash-tui/src/app/chat.rs index f8329e6c..2da8d4ea 100644 --- a/crates/rocm-dash-tui/src/app/chat.rs +++ b/crates/rocm-dash-tui/src/app/chat.rs @@ -429,6 +429,27 @@ mod tests { /// :11435) is preferred over falling back to the ChatGPT cloud default. #[tokio::test] async fn detect_local_chat_prefers_local_server_over_no_endpoint() { + // detect_local_chat probes candidates in priority order (Lemonade, + // then vLLM, then rocm serve's default). For the rocm-serve listener + // below to be the one detect_local_chat actually picks, the two + // higher-priority ports must be free — an ambient listener on the CI + // runner (e.g. another process already on :8000) would otherwise + // make this test flake by returning a different endpoint than + // expected. Note: unlike the guard below, these can only be checked + // and released (not held) — the probe is a bare TCP connect, so a + // held listener would itself register as "reachable" and defeat the + // check. + let Ok(lemonade_probe) = + std::net::TcpListener::bind(("127.0.0.1", crate::skills::LEMONADE_PORT)) + else { + return; // Port already bound in this environment; skip rather than flake. + }; + drop(lemonade_probe); + let Ok(vllm_probe) = std::net::TcpListener::bind(("127.0.0.1", crate::skills::VLLM_PORT)) + else { + return; // Port already bound in this environment; skip rather than flake. + }; + drop(vllm_probe); let Ok(listener) = std::net::TcpListener::bind(("127.0.0.1", crate::skills::ROCM_SERVE_PORT)) else { diff --git a/crates/rocm-dash-tui/src/llm.rs b/crates/rocm-dash-tui/src/llm.rs index 53774afc..5a294643 100644 --- a/crates/rocm-dash-tui/src/llm.rs +++ b/crates/rocm-dash-tui/src/llm.rs @@ -491,12 +491,28 @@ mod tests { #[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. + // up by the fallback probe, not just Lemonade/vLLM's ports. The two + // higher-priority ports must be free, or an ambient listener on the + // CI runner would make this test flake by returning a different + // endpoint. Note: unlike the guard below, these can only be checked + // and released (not held) — the probe is a bare TCP connect, so a + // held listener would itself register as "reachable" and defeat the + // check. + let Ok(lemonade_probe) = + std::net::TcpListener::bind(("127.0.0.1", crate::skills::LEMONADE_PORT)) + else { + return; // Port already bound in this environment; skip rather than flake. + }; + drop(lemonade_probe); + let Ok(vllm_probe) = std::net::TcpListener::bind(("127.0.0.1", crate::skills::VLLM_PORT)) + else { + return; // Port already bound in this environment; skip rather than flake. + }; + drop(vllm_probe); 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; + return; // Port already bound in this environment; skip rather than flake. }; assert_eq!( detect_local_endpoint(), From 4829b14541bf10a684047c524fecab934ed41396 Mon Sep 17 00:00:00 2001 From: Michael Roy Date: Fri, 10 Jul 2026 21:21:49 -0700 Subject: [PATCH 4/6] fix(dash): make local-detect priority tests port-independent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous fix (bind-then-release the higher-priority ports before asserting) still flaked on the Windows CI runner: something answered on the Lemonade port during the probe despite the guard, most likely an ambient listener the test doesn't control. TCP-connect-based detection means any listener on a well-known port — even a bare guard listener held open, as the very first attempt at this fix did — registers as "reachable", so real ports can't be made deterministic from a test. Split the port-priority logic in llm.rs into probe_first_reachable, tested directly against OS-assigned ephemeral ports instead of the real well-known ports. Gave detect_local_chat an injectable probe seam (detect_local_chat_with_probe) so its own test can verify the no-managed-executor wiring without depending on any real network state. Signed-off-by: Michael Roy --- crates/rocm-dash-tui/src/app/chat.rs | 59 +++++------- crates/rocm-dash-tui/src/llm.rs | 128 +++++++++++++-------------- 2 files changed, 88 insertions(+), 99 deletions(-) diff --git a/crates/rocm-dash-tui/src/app/chat.rs b/crates/rocm-dash-tui/src/app/chat.rs index 2da8d4ea..f80bb977 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) @@ -429,39 +437,20 @@ mod tests { /// :11435) is preferred over falling back to the ChatGPT cloud default. #[tokio::test] async fn detect_local_chat_prefers_local_server_over_no_endpoint() { - // detect_local_chat probes candidates in priority order (Lemonade, - // then vLLM, then rocm serve's default). For the rocm-serve listener - // below to be the one detect_local_chat actually picks, the two - // higher-priority ports must be free — an ambient listener on the CI - // runner (e.g. another process already on :8000) would otherwise - // make this test flake by returning a different endpoint than - // expected. Note: unlike the guard below, these can only be checked - // and released (not held) — the probe is a bare TCP connect, so a - // held listener would itself register as "reachable" and defeat the - // check. - let Ok(lemonade_probe) = - std::net::TcpListener::bind(("127.0.0.1", crate::skills::LEMONADE_PORT)) - else { - return; // Port already bound in this environment; skip rather than flake. - }; - drop(lemonade_probe); - let Ok(vllm_probe) = std::net::TcpListener::bind(("127.0.0.1", crate::skills::VLLM_PORT)) - else { - return; // Port already bound in this environment; skip rather than flake. - }; - drop(vllm_probe); - let Ok(listener) = - std::net::TcpListener::bind(("127.0.0.1", crate::skills::ROCM_SERVE_PORT)) - else { - return; // Port already bound in this environment; skip rather than flake. - }; - // No executor (no managed-services registry available) — falls through - // to the well-known-port probe, which must still find the listener. - let cfg = detect_local_chat(None) + // 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, crate::skills::ROCM_SERVE_ENDPOINT); - drop(listener); + assert_eq!(cfg.base_url, PROBED); } #[test] diff --git a/crates/rocm-dash-tui/src/llm.rs b/crates/rocm-dash-tui/src/llm.rs index 5a294643..b76df1d9 100644 --- a/crates/rocm-dash-tui/src/llm.rs +++ b/crates/rocm-dash-tui/src/llm.rs @@ -188,28 +188,23 @@ pub fn probe_endpoint(base_url: &str, timeout: Duration) -> bool { false } -/// Probe the known local serving endpoints (Lemonade, vLLM, then `rocm -/// serve`'s non-managed default) concurrently and return the highest-priority -/// one that answered. +/// 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, and by chat -/// startup, so the user need not run the CLI skill first. Probing all three -/// candidates in parallel (rather than sequentially) bounds the worst case -/// (no server listening) to one [`PROBE_TIMEOUT`] instead of three, which -/// matters at startup where this gates falling back to the cloud default. -/// TCP-only (no HTTP); never blocks longer than [`PROBE_TIMEOUT`] total. -pub fn detect_local_endpoint() -> Option<&'static str> { - const CANDIDATES: [&str; 3] = [ - crate::skills::LEMONADE_ENDPOINT, - crate::skills::VLLM_ENDPOINT, - crate::skills::ROCM_SERVE_ENDPOINT, - ]; +/// 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>(candidates: &[&'a str]) -> Option<&'a str> { 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 + let handles: Vec<_> = candidates .iter() .map(|ep| (*ep, scope.spawn(|| probe_endpoint(ep, PROBE_TIMEOUT)))) .collect(); @@ -219,6 +214,21 @@ pub fn detect_local_endpoint() -> Option<&'static str> { }) } +/// 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> { + const CANDIDATES: [&str; 3] = [ + crate::skills::LEMONADE_ENDPOINT, + crate::skills::VLLM_ENDPOINT, + crate::skills::ROCM_SERVE_ENDPOINT, + ]; + probe_first_reachable(&CANDIDATES) +} + /// Pick the first model id from an OpenAI-compatible `/v1/models` response (`{"data":[{"id":"…"}]}`). /// /// Pure — no HTTP. `None` when the shape is missing, @@ -453,24 +463,47 @@ 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_prefers_lemonade_when_multiple_reachable() { - // Lemonade and vLLM both listening: priority order (Lemonade first) - // must win even though probes run concurrently, not sequentially. - let Ok(lemonade) = std::net::TcpListener::bind(("127.0.0.1", crate::skills::LEMONADE_PORT)) - else { - return; // Port already bound in this environment; skip rather than flake. - }; - let Ok(vllm) = std::net::TcpListener::bind(("127.0.0.1", crate::skills::VLLM_PORT)) else { - drop(lemonade); - 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!( - detect_local_endpoint(), - Some(crate::skills::LEMONADE_ENDPOINT) + probe_first_reachable(&[first_url.as_str(), second_url.as_str()]), + Some(first_url.as_str()) ); - drop(lemonade); - drop(vllm); + } + + #[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()]), + Some(url.as_str()) + ); + } + + #[test] + fn probe_first_reachable_none_when_nothing_listening() { + assert_eq!(probe_first_reachable(&["http://127.0.0.1:1"]), None); } #[test] @@ -487,37 +520,4 @@ mod tests { start.elapsed() ); } - - #[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. The two - // higher-priority ports must be free, or an ambient listener on the - // CI runner would make this test flake by returning a different - // endpoint. Note: unlike the guard below, these can only be checked - // and released (not held) — the probe is a bare TCP connect, so a - // held listener would itself register as "reachable" and defeat the - // check. - let Ok(lemonade_probe) = - std::net::TcpListener::bind(("127.0.0.1", crate::skills::LEMONADE_PORT)) - else { - return; // Port already bound in this environment; skip rather than flake. - }; - drop(lemonade_probe); - let Ok(vllm_probe) = std::net::TcpListener::bind(("127.0.0.1", crate::skills::VLLM_PORT)) - else { - return; // Port already bound in this environment; skip rather than flake. - }; - drop(vllm_probe); - let Ok(listener) = - std::net::TcpListener::bind(("127.0.0.1", crate::skills::ROCM_SERVE_PORT)) - else { - return; // Port already bound in this environment; skip rather than flake. - }; - assert_eq!( - detect_local_endpoint(), - Some(crate::skills::ROCM_SERVE_ENDPOINT) - ); - drop(listener); - } } From f64bb407ea37d83f22ec79fe36e4cb00021d41e5 Mon Sep 17 00:00:00 2001 From: Michael Roy Date: Fri, 10 Jul 2026 21:36:06 -0700 Subject: [PATCH 5/6] fix(dash): widen latency margin in detect_local_endpoint timing test detect_local_endpoint_bounded_latency_when_nothing_listening's 2x PROBE_TIMEOUT margin was still too tight for the Windows CI runner (observed 624ms against a 600ms threshold). A true sequential-probing regression takes ~3x PROBE_TIMEOUT, so widen the margin to 4x to absorb scheduling jitter on slower CI machines while still catching that regression. Signed-off-by: Michael Roy --- crates/rocm-dash-tui/src/llm.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/crates/rocm-dash-tui/src/llm.rs b/crates/rocm-dash-tui/src/llm.rs index b76df1d9..d2897218 100644 --- a/crates/rocm-dash-tui/src/llm.rs +++ b/crates/rocm-dash-tui/src/llm.rs @@ -510,12 +510,14 @@ mod tests { fn detect_local_endpoint_bounded_latency_when_nothing_listening() { // Regression guard for the startup-latency fix: with no server // reachable, probing all 3 well-known ports must take roughly one - // PROBE_TIMEOUT (parallel), not three (sequential). Generous margin - // to avoid flaking on loaded CI machines. + // PROBE_TIMEOUT (parallel), not three (sequential). A true regression + // back to sequential probing takes ~3x PROBE_TIMEOUT, so a 4x margin + // still catches it while giving plenty of headroom for scheduling + // jitter on loaded/slower CI machines (observed >2x on Windows CI). let start = std::time::Instant::now(); let _ = detect_local_endpoint(); assert!( - start.elapsed() < PROBE_TIMEOUT * 2, + start.elapsed() < PROBE_TIMEOUT * 4, "expected parallel probing to bound latency to ~1 PROBE_TIMEOUT, took {:?}", start.elapsed() ); From 5414aac67ca9884835b34ca52cdf44c7de498899 Mon Sep 17 00:00:00 2001 From: Michael Roy Date: Tue, 14 Jul 2026 08:44:40 -0700 Subject: [PATCH 6/6] test(dash): strengthen local chat startup coverage Signed-off-by: Michael Roy --- crates/rocm-dash-tui/src/app/chat.rs | 44 ++++++++++++++++- crates/rocm-dash-tui/src/app/mod.rs | 23 +++++---- crates/rocm-dash-tui/src/llm.rs | 72 ++++++++++++++++++---------- 3 files changed, 102 insertions(+), 37 deletions(-) diff --git a/crates/rocm-dash-tui/src/app/chat.rs b/crates/rocm-dash-tui/src/app/chat.rs index f80bb977..54f70010 100644 --- a/crates/rocm-dash-tui/src/app/chat.rs +++ b/crates/rocm-dash-tui/src/app/chat.rs @@ -268,6 +268,30 @@ pub(super) const fn should_detect_local_chat( 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. @@ -436,7 +460,7 @@ mod tests { /// 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_prefers_local_server_over_no_endpoint() { + 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 @@ -472,4 +496,22 @@ mod tests { 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 351b1974..3c13f10b 100644 --- a/crates/rocm-dash-tui/src/app/mod.rs +++ b/crates/rocm-dash-tui/src/app/mod.rs @@ -38,7 +38,10 @@ mod chat; mod slash; mod summary; -use chat::{build_chat_agent, build_local_agent, detect_local_chat, persist_chat_endpoint}; +use chat::{ + 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}; /// Which single flow a *focused host* runs. @@ -1601,16 +1604,15 @@ async fn event_loop(terminal: &mut Tui, args: &ResolvedArgs) -> color_eyre::Resu // 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 probe_ok = if detected.is_some() { - true - } else if detection_ran { - false - } else { - tokio::task::spawn_blocking(move || { + 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 = detected.or_else(|| { crate::llm::resolve_llm_config( @@ -1629,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 d2897218..e61ad1ec 100644 --- a/crates/rocm-dash-tui/src/llm.rs +++ b/crates/rocm-dash-tui/src/llm.rs @@ -198,7 +198,10 @@ pub fn probe_endpoint(base_url: &str, timeout: Duration) -> bool { /// (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>(candidates: &[&'a str]) -> Option<&'a str> { +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 @@ -206,7 +209,7 @@ fn probe_first_reachable<'a>(candidates: &[&'a str]) -> Option<&'a str> { #[allow(clippy::needless_collect)] let handles: Vec<_> = candidates .iter() - .map(|ep| (*ep, scope.spawn(|| probe_endpoint(ep, PROBE_TIMEOUT)))) + .map(|ep| (*ep, scope.spawn(|| probe(ep, PROBE_TIMEOUT)))) .collect(); handles .into_iter() @@ -214,6 +217,12 @@ fn probe_first_reachable<'a>(candidates: &[&'a str]) -> Option<&'a str> { }) } +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. @@ -221,12 +230,7 @@ fn probe_first_reachable<'a>(candidates: &[&'a str]) -> Option<&'a str> { /// 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> { - const CANDIDATES: [&str; 3] = [ - crate::skills::LEMONADE_ENDPOINT, - crate::skills::VLLM_ENDPOINT, - crate::skills::ROCM_SERVE_ENDPOINT, - ]; - probe_first_reachable(&CANDIDATES) + probe_first_reachable(&LOCAL_ENDPOINT_CANDIDATES, probe_endpoint) } /// Pick the first model id from an OpenAI-compatible `/v1/models` response (`{"data":[{"id":"…"}]}`). @@ -485,7 +489,7 @@ mod tests { 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_first_reachable(&[first_url.as_str(), second_url.as_str()], probe_endpoint), Some(first_url.as_str()) ); } @@ -496,30 +500,50 @@ mod tests { // 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_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"]), None); + assert_eq!( + probe_first_reachable(&["http://127.0.0.1:1"], probe_endpoint), + None + ); } #[test] - fn detect_local_endpoint_bounded_latency_when_nothing_listening() { - // Regression guard for the startup-latency fix: with no server - // reachable, probing all 3 well-known ports must take roughly one - // PROBE_TIMEOUT (parallel), not three (sequential). A true regression - // back to sequential probing takes ~3x PROBE_TIMEOUT, so a 4x margin - // still catches it while giving plenty of headroom for scheduling - // jitter on loaded/slower CI machines (observed >2x on Windows CI). - let start = std::time::Instant::now(); - let _ = detect_local_endpoint(); - assert!( - start.elapsed() < PROBE_TIMEOUT * 4, - "expected parallel probing to bound latency to ~1 PROBE_TIMEOUT, took {:?}", - start.elapsed() + 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!( + LOCAL_ENDPOINT_CANDIDATES, + [ + crate::skills::LEMONADE_ENDPOINT, + crate::skills::VLLM_ENDPOINT, + crate::skills::ROCM_SERVE_ENDPOINT, + ] ); } }