diff --git a/specs/008-llamacpp-integration/tasks.md b/specs/008-llamacpp-integration/tasks.md index df146af..65f5f4c 100644 --- a/specs/008-llamacpp-integration/tasks.md +++ b/specs/008-llamacpp-integration/tasks.md @@ -149,3 +149,28 @@ Unlike the prerequisite epic, **this decomposition genuinely holds** — there is no shared "output type" migration forcing an atomic cutover, because `LlamaCppClient` is new, not a change to an existing node. Each phase really can land independently. + +--- + +## Post-implementation review finding (fixed) + +A `beacon-reviewer` pass ahead of PR open found `LlamaCppProvider.list_models()` +caught *every* exception and returned `[]`, silently indistinguishable from +"no models installed" — violating FR-006 and `contracts/llamacpp_provider_conformance.md`'s +explicit requirement that a non-router-mode `llama-server` (unreachable +endpoints → HTTP error on `GET /models`) surface a clear, specific error. + +Fixed: `_get_json` (shared with `OllamaProvider`, in `ollama_provider.py`) now +raises `RuntimeError` on an HTTP error status, matching `_post_json`'s +existing behavior — its docstring already claimed this, it just didn't do it. +`LlamaCppProvider.list_models()` now distinguishes `OSError` (genuinely +unreachable — connection refused, DNS failure, timeout; all aiohttp +connection-level exceptions are `OSError` subclasses) from `RuntimeError` +(server responded, but with an error): the former still degrades gracefully +to `[]` (consistent with `OllamaProvider`'s existing UX), the latter is +re-raised naming router mode as the likely cause. Regression test added: +`test_list_models_non_router_mode_raises_clear_error` in +`tests/test_llamacpp_provider.py`. `OllamaProvider`'s own `list_models()`/ +`_fetch_models()` still catch broadly and degrade to `[]` unchanged — no +spec requirement asks Ollama to make this distinction, and this fix doesn't +force it to. diff --git a/src/comfydv/_llm/llamacpp_provider.py b/src/comfydv/_llm/llamacpp_provider.py index 58e5cf1..bcd6412 100644 --- a/src/comfydv/_llm/llamacpp_provider.py +++ b/src/comfydv/_llm/llamacpp_provider.py @@ -55,11 +55,27 @@ class LlamaCppProvider: try: data = await _get_json(f"{self.host}/models", headers=self.headers) - except Exception as exc: + except OSError as exc: + # Genuinely unreachable (connection refused, DNS failure, timed + # out — aiohttp's connection-level exceptions are all OSError + # subclasses) — degrade gracefully like OllamaProvider does, so + # a not-yet-started server just shows an empty dropdown rather + # than a hard error. logger.warning( "Could not fetch llama.cpp models from %s: %s", self.host, exc ) return [] + except RuntimeError as exc: + # The server answered but with an HTTP error status — GET + # /models only exists in router mode, so this is almost always + # a llama-server launched without --models-dir/--models-preset. + # Surfacing this distinctly (FR-006) matters: silently returning + # [] here would be indistinguishable from "no models installed". + raise RuntimeError( + f"llama-server at {self.host} did not return a model list from " + f"GET {self.host}/models — is it running in router mode " + f"(--models-dir or --models-preset)? Underlying error: {exc}" + ) from exc models = [] for m in data.get("data", []): diff --git a/src/comfydv/_llm/ollama_provider.py b/src/comfydv/_llm/ollama_provider.py index a285f75..13425ae 100644 --- a/src/comfydv/_llm/ollama_provider.py +++ b/src/comfydv/_llm/ollama_provider.py @@ -132,7 +132,14 @@ async def _post_json( async def _get_json( url: str, *, timeout: float = 5.0, headers: dict | None = None ) -> dict: - """GET url, return parsed response dict. Raises on connection/HTTP error.""" + """GET url, return parsed response dict. + + Raises RuntimeError on an HTTP error status (distinct message, so callers + can tell "server responded with an error" from "couldn't reach it at + all" — aiohttp connection/timeout errors propagate unwrapped for that + reason). Message is generic, not backend-branded: this helper is shared + by every LLMProvider implementation. + """ import aiohttp async with aiohttp.ClientSession() as session: @@ -141,6 +148,11 @@ async def _get_json( headers=headers or None, timeout=aiohttp.ClientTimeout(total=timeout), ) as resp: + if resp.status >= 400: + body = await resp.text() + raise RuntimeError( + f"Server returned HTTP {resp.status} for {url}: {body[:300]}" + ) return await resp.json() diff --git a/tests/test_llamacpp_provider.py b/tests/test_llamacpp_provider.py index ca66949..ae04b28 100644 --- a/tests/test_llamacpp_provider.py +++ b/tests/test_llamacpp_provider.py @@ -103,6 +103,22 @@ def test_list_models_unreachable_returns_empty(monkeypatch): assert models == [] +def test_list_models_non_router_mode_raises_clear_error(monkeypatch): + """FR-006: a llama-server that IS reachable but wasn't launched with + --models-dir/--models-preset answers GET /models with an HTTP error + (the endpoint doesn't exist outside router mode). That must surface as + a specific, actionable error — not silently degrade to an empty list, + which would be indistinguishable from "server has no models".""" + + async def fake_get(url, *, timeout=5.0, headers=None): + raise RuntimeError("Server returned HTTP 404 for http://x/models: not found") + + monkeypatch.setattr(provider_mod, "_get_json", fake_get) + + with pytest.raises(RuntimeError, match="router mode"): + _run_async(LlamaCppProvider("http://localhost:8080").list_models()) + + def test_list_models_cached_second_call(monkeypatch): calls = {"n": 0}