diff --git a/backend/app/api/routes/status.py b/backend/app/api/routes/status.py index 2babda5..7c379e8 100644 --- a/backend/app/api/routes/status.py +++ b/backend/app/api/routes/status.py @@ -8,7 +8,7 @@ from fastapi import APIRouter from app.config.settings import settings from app.domain.retrieval import RetrievedChunk -from app.services.llm.llm_factory import get_llm_client +from app.services.llm.llm_factory import get_llm_client, get_llm_factory from app.shared.bootstrap import ( get_bm25_retriever, get_binary_store, @@ -129,6 +129,27 @@ async def get_health(): } +def _normalize_llm_provider(raw_provider: str) -> str: + """Normalize a raw LLM_PROVIDER/HYDE_LLM_PROVIDER settings string to the + canonical LLMProvider enum value, the SAME way LLMFactory.create() does. + + TrackedLLMClient.chat() (tracked_client.py) always records usage under + `self._inner.config.provider.value` — the NORMALIZED enum value produced by + LLMFactory._parse_provider() — never the raw string a caller passed to + get_llm_client(). Reusing that same normalization here (instead of + duplicating the alias table) guarantees the tracker key this route reads + always agrees with the key TrackedLLMClient wrote, even when the raw + settings value is a non-canonical alias (e.g. "deepseek-v3") or different + casing. Falls back to the raw string, unchanged, if it does not match any + known provider/alias, so this passive status endpoint still renders + (as "never_called") instead of raising on a misconfigured provider string. + """ + try: + return get_llm_factory()._parse_provider(raw_provider).value + except ValueError: + return raw_provider + + def _resolve_role_provider_model(role: str) -> tuple[str, str]: """Return the (provider, model) pair currently configured for one AI model role. @@ -138,10 +159,10 @@ def _resolve_role_provider_model(role: str) -> tuple[str, str]: recorded when HyDE actually ran. """ if role == "main_llm": - return settings.llm_provider, settings.llm_model + return _normalize_llm_provider(settings.llm_provider), settings.llm_model if role == "hyde_llm": return ( - settings.hyde_llm_provider or settings.llm_provider, + _normalize_llm_provider(settings.hyde_llm_provider or settings.llm_provider), settings.hyde_llm_model or settings.llm_model, ) if role == "embedding": @@ -204,7 +225,16 @@ async def get_model_statuses(): async def _ping_main_or_hyde(role: str) -> None: - """Send one minimal chat completion to the LLM configured for `role`.""" + """Send one minimal chat completion to the LLM configured for `role`. + + Skipped entirely for "hyde_llm" when settings.hyde_enabled is False, + mirroring _ping_reranker()'s disabled-skip pattern: when HyDE is turned + off (or reuses the main LLM, the default), issuing this ping would just be + a redundant duplicate chat call against the same model for no benefit. + "main_llm" is always pinged regardless of this check. + """ + if role == "hyde_llm" and not settings.hyde_enabled: + return provider, model = _resolve_role_provider_model(role) try: client = get_llm_client(provider=provider, model=model) diff --git a/tests/test_status_models_routes.py b/tests/test_status_models_routes.py index d1bf5cc..34f1636 100644 --- a/tests/test_status_models_routes.py +++ b/tests/test_status_models_routes.py @@ -12,6 +12,7 @@ import pytest from fastapi.testclient import TestClient from app.services.llm.base_client import LLMResponse +from app.services.llm.llm_factory import LLMFactory, get_llm_client from app.shared.model_usage_tracker import get_model_usage_tracker @@ -23,6 +24,20 @@ def _reset_tracker(): get_model_usage_tracker()._entries.clear() +@pytest.fixture +def _reset_llm_factory_instances(): + """Clear LLMFactory's process-wide client cache before/after this test only. + + Mirrors backend/tests/observability/test_llm_factory_tracking.py's + _reset_singletons fixture: LLMFactory._global_instances persists for the + life of the process, so without this the cache entry created by driving a + real get_llm_client() call in a test would leak into other tests. + """ + LLMFactory._global_instances.clear() + yield + LLMFactory._global_instances.clear() + + @pytest.fixture def client(): """Return a TestClient for the real app (status routes require no auth).""" @@ -131,3 +146,74 @@ def test_ping_records_get_llm_client_failure_instead_of_dropping_it(client): assert hyde_row["status"] == "error" assert hyde_row["call_count_error"] == 1 assert hyde_row["last_error"] == "missing api key" + + +def test_ping_skips_hyde_llm_when_hyde_disabled(client): + """POST /status/models/ping must NOT ping hyde_llm when settings.hyde_enabled is False: + HyDE often reuses the main LLM, so a live ping would just be a redundant duplicate chat + call against the same model for no benefit. main_llm must still always be pinged.""" + from app.config.settings import settings + + mock_llm_response = LLMResponse(content="pong", model="test-model", usage={"total_tokens": 1}) + mock_llm_client = MagicMock() + mock_llm_client.chat.return_value = mock_llm_response + mock_embedding = MagicMock() + mock_embedding.embed_query.return_value = [0.1] + + with patch.object(settings, "hyde_enabled", False), \ + patch("app.api.routes.status.get_llm_client", return_value=mock_llm_client) as mock_get_llm_client, \ + patch("app.api.routes.status.get_embedding_provider", return_value=mock_embedding), \ + patch("app.api.routes.status.get_reranker", return_value=None): + resp = client.post("/api/v1/status/models/ping") + + assert resp.status_code == 200 + # Only main_llm's ping should reach get_llm_client()/.chat(); hyde_llm's must be skipped. + assert mock_get_llm_client.call_count == 1 + assert mock_llm_client.chat.call_count == 1 + hyde_row = next(m for m in resp.json()["models"] if m["role"] == "hyde_llm") + assert hyde_row["status"] == "disabled" + + +def test_ping_write_path_and_status_read_path_agree_for_non_canonical_provider_alias( + client, _reset_llm_factory_instances +): + """Regression for the tracker key mismatch bug: TrackedLLMClient.chat() always records + usage under the NORMALIZED LLMProvider enum value from LLMFactory._parse_provider() + (see tracked_client.py), never the raw provider string a caller passed to + get_llm_client(). Before the fix, _resolve_role_provider_model() returned + settings.llm_provider verbatim with no normalization. So whenever LLM_PROVIDER held a + non-canonical alias (e.g. "deepseek-v3" instead of "deepseek"), the read-side lookup key + ("deepseek-v3:") stopped matching the write-side key TrackedLLMClient actually + recorded under ("deepseek:"), and /status/models showed main_llm as perpetually + "never_called" even though it was being actively tracked. + + This drives the FULL real path (LLMFactory.create() -> TrackedLLMClient -> + ModelUsageTracker -> the /status/models route), the same way real call sites like + compliance.py invoke get_llm_client(provider=settings.llm_provider, model=settings.llm_model) + -- unlike test_get_models_reflects_recorded_usage above, which shortcuts by recording + directly into the tracker. + """ + from app.config.settings import settings + + mock_response = LLMResponse(content="pong", model=settings.llm_model, usage={"total_tokens": 7}) + + def _fake_deepseek_client(config): + # Carry the REAL LLMConfig built by LLMFactory.create() so TrackedLLMClient.chat() + # records under config.provider.value exactly like it does in production. + fake = MagicMock() + fake.config = config + fake.chat.return_value = mock_response + return fake + + with patch.object(settings, "llm_provider", "deepseek-v3"), \ + patch("app.services.llm.llm_factory.DeepSeekClient", side_effect=_fake_deepseek_client): + tracked_client = get_llm_client(provider=settings.llm_provider, model=settings.llm_model, api_key="test-key") + tracked_client.chat([{"role": "user", "content": "hi"}]) + + resp = client.get("/api/v1/status/models") + + assert resp.status_code == 200 + main_row = next(m for m in resp.json()["models"] if m["role"] == "main_llm") + assert main_row["status"] == "ok" + assert main_row["total_tokens"] == 7 + assert main_row["provider"] == "deepseek"