Skip to content

fix(chat): revalidate a stale persisted chat_url and fall back to a live endpoint#105

Open
michaelroy-amd wants to merge 2 commits into
mainfrom
fix/chat-stale-url
Open

fix(chat): revalidate a stale persisted chat_url and fall back to a live endpoint#105
michaelroy-amd wants to merge 2 commits into
mainfrom
fix/chat-stale-url

Conversation

@michaelroy-amd

Copy link
Copy Markdown
Member

Summary

  • Revalidates a persisted tui.chat_url at startup: if it's unreachable
    and not matched by the live managed-services registry, the dash now
    falls back to a freshly-detected local engine instead of silently
    building an agent against a dead endpoint.
  • Surfaces a "server changed" notice in the chat transcript pointing at
    the new /detect save command (PR feat(chat): add /detect slash command for mid-session re-detection #102) to persist the replacement.

Root cause

tui.chat_url survives across dash launches once written (e.g. via the
in-TUI "use & save" flow). If the server behind it is later stopped or
replaced — most commonly a managed vLLM/Lemonade instance relaunched on
a different port — the startup path passed the stale URL straight into
resolve_llm_config regardless of reachability. The chat backend was
then built against a dead endpoint with no on-screen indication
anything had changed; the user would only discover it from a failed
completion request.

Fix

  • app/chat.rs: new pure decision function stale_chat_url_replacement
    — given the configured base URL, whether its own probe succeeded, and
    an already-detected live LlmConfig (or None), returns
    Some(live) only when the configured endpoint is unreachable and a
    differently-addressed live endpoint exists. No I/O; fully
    unit-testable.
  • app/mod.rs (startup block in event_loop()): when chat_url is
    configured, wasn't matched by the managed-services registry, and its
    TCP probe fails, calls detect_local_chat and feeds the result
    through stale_chat_url_replacement. A replacement, if found, is
    substituted before resolve_llm_config runs and a
    ChatTurn::system(...) notice is pushed into the transcript.

Precedence preserved

This only ever revalidates the persisted-config tier:

  • chat_env_url (the env tier) is untouched — it's already excluded
    from the managed-registry/stale-check path (both only run when
    chat_url itself is set).
  • resolve_llm_config's own CLI>config>env>probe precedence is not
    modified at all; the substitution happens purely at the call site by
    swapping in a different LlmConfig before that function runs. Its
    existing unit tests — cli_wins_over_every_other_tier,
    config_wins_when_no_cli, env_url_wins_when_no_cli_or_config,
    probed_default_used_when_nothing_configured_but_reachable — are
    unmodified and pass unchanged.

(Note: this crate's ResolvedArgs::chat_url is always sourced from
tui.chat_url at the one call site in apps/rocm/src/dash.rs — there
is no separate explicit-CLI flag wired through a different field today
— so "persisted-config tier" is precisely what this revalidates.)

Coordination notes

Complements unmerged PR #97, which addresses the chat_model half of
the same "server changed" gap. No overlapping hunks expected (this PR
touches app/chat.rs + the startup block's chat_url/probe handling
in app/mod.rs).

Relates to EAI-7360.

Test Plan

  • cargo build -p rocm-dash-tui
  • cargo clippy -p rocm-dash-tui --all-targets --all-features -- -D warnings (clean)
  • cargo test -p rocm-dash-tui -- --test-threads=1 (all passing, precedence tests unaffected)
  • New tests:
    • stale_chat_url_replacement_none_when_configured_endpoint_reachable
    • stale_chat_url_replacement_none_when_nothing_live_found
    • stale_chat_url_replacement_none_when_live_matches_configured
    • stale_chat_url_replacement_swaps_to_a_different_live_endpoint

…ive endpoint

A persisted tui.chat_url survives across dash launches, but the server
it points at may have been stopped or replaced since it was written
(e.g. a managed vLLM/Lemonade instance was restarted on a different
port). Previously the startup path passed a stale chat_url straight
into resolve_llm_config regardless of reachability, so the agent was
built against a dead endpoint with no indication anything had changed.

Add stale_chat_url_replacement (app/chat.rs): a pure decision function
that, only when the persisted chat_url's own TCP probe fails and no
managed-services entry matched it, checks whether detect_local_chat
finds a live, differently-addressed endpoint. If so, the startup path
in event_loop() substitutes that live LlmConfig before calling
resolve_llm_config and pushes a "server changed" ChatTurn::system
notice into the transcript pointing at `/detect save` to persist it.

This only ever revalidates the persisted-config tier: chat_env_url is
untouched (it's excluded from the managed-registry check already), an
explicit --chat-url wired through the same field would be revalidated
identically today since this crate never distinguishes CLI from config
at this layer, and resolve_llm_config's own CLI>config>env>probe
precedence is not modified at all — the substitution happens purely at
the call site, and its existing unit tests (cli_wins_over_every_other_tier,
config_wins_when_no_cli, env_url_wins_when_no_cli_or_config,
probed_default_used_when_nothing_configured_but_reachable) all remain
green untouched.

Complements unmerged PR #97, which addresses the chat_model half of
this gap.

Relates to EAI-7360.

Signed-off-by: Michael Roy <michael.roy@amd.com>
…ormatting

Two refinements to the stale persisted-chat_url revalidation:

1. Guard the substitution on chat_api_key.is_none() &&
   chat_auth_header.is_none(). The replacement is a keyless
   detected_llm_config, and ResolvedArgs cannot distinguish an explicit
   --chat-url from persisted config, so a remote gateway URL plus
   OPENAI_API_KEY whose TCP connect merely times out (the 300ms budget
   is tight) could silently swap to a local keyless engine and drop the
   credential. Requiring no configured auth confines the swap to the
   true EAI-7360 target — a dead persisted local endpoint with no auth.

2. Normalize base_url before the change-detection compare (trim a
   trailing `/` and a trailing `/v1`) so a formatting-only difference
   (e.g. http://h:8000 vs http://h:8000/v1/) is not mistaken for a
   server change and does not emit a spurious "server changed" notice.

Relates to EAI-7360.

Signed-off-by: Michael Roy <michael.roy@amd.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant