* fix(#5731): repair poisoned session provider at chat start * fix(#5731): fail safe on incomplete catalog ownership * changelog: session provider-repair on stale binding (#5873, #5731) --------- Co-authored-by: Rod Boev <rod.boev@gmail.com> Co-authored-by: t <a@b>
This commit is contained in:
nesquena-hermes
GitHub
parent
de0e6eb452
commit
d7c3c2b734
3 changed files with 366 additions and 0 deletions
|
|
@ -3,6 +3,10 @@
|
|||
|
||||
## [Unreleased]
|
||||
|
||||
### Fixed
|
||||
|
||||
- **A session no longer gets stuck sending to the wrong provider after its stored provider becomes stale.** When a session's persisted model/provider no longer matches where that model actually lives (e.g. the model moved groups, or an older session carried a now-foreign provider), chat start now repairs the binding to the correct provider — but only on positive proof: it reroutes solely when the live model catalog shows the stored group genuinely lacks the model AND exactly one other healthy group owns it. It checks both the primary and overflow (`extra_models`) model buckets before deciding a group doesn't own a model, and it fails safe — preserving the stored provider without rerouting — whenever any relevant catalog probe failed (a transient discovery error is never treated as proof the model is absent). Ordinary matching-provider sends make no catalog call, and explicit/ambiguous/unknown/stored-owner picks are all left untouched. Thanks @rodboev. (#5873, #5731)
|
||||
|
||||
### Internal
|
||||
|
||||
- **Centralized WebUI static-asset resolution.** The app-shell (`index.html`), `/static/*`, manifest, service worker, and favicon now resolve through a single shared resolver instead of scattered `__file__`-relative logic — behavior-preserving (same bytes, headers, cache signature, CSRF-per-request injection, and path-traversal rejection), with new resolver regression tests. Thanks @rodboev. (#5557, #2695)
|
||||
|
|
|
|||
|
|
@ -6001,6 +6001,85 @@ def _catalog_model_id_matches(candidate: str, model: str) -> bool:
|
|||
return candidate.replace("-", ".").lower() == model.replace("-", ".").lower()
|
||||
|
||||
|
||||
def _catalog_group_owns_exact_model(group: dict, model: str) -> bool:
|
||||
provider_id = str(group.get("provider_id") or "").strip()
|
||||
wrapper = f"@{provider_id}:"
|
||||
for bucket in ("models", "extra_models"):
|
||||
for entry in group.get(bucket) or []:
|
||||
if not isinstance(entry, dict):
|
||||
continue
|
||||
candidate = str(entry.get("id") or "").strip()
|
||||
if candidate.lower().startswith(wrapper.lower()):
|
||||
candidate = candidate[len(wrapper):]
|
||||
if candidate == model or _catalog_model_id_matches(candidate, model):
|
||||
return True
|
||||
return False
|
||||
|
||||
|
||||
def _repair_foreign_session_model_provider(
|
||||
session,
|
||||
*,
|
||||
requested_model: str,
|
||||
requested_provider: str | None,
|
||||
resolved_model: str,
|
||||
resolved_provider: str | None,
|
||||
explicit_model_pick: bool,
|
||||
profile_provider: str | None,
|
||||
) -> str | None:
|
||||
"""Repair a stale provider only when the cached catalog names one owner."""
|
||||
stored_model = str(getattr(session, "model", "") or "").strip()
|
||||
stored_provider = _clean_session_model_provider(getattr(session, "model_provider", None))
|
||||
requested_provider = _clean_session_model_provider(requested_provider)
|
||||
resolved_provider = _clean_session_model_provider(resolved_provider)
|
||||
profile_provider = _clean_session_model_provider(profile_provider)
|
||||
_, qualified_provider = _split_provider_qualified_model(requested_model)
|
||||
if (
|
||||
explicit_model_pick
|
||||
or qualified_provider
|
||||
or not stored_model
|
||||
or not stored_provider
|
||||
or (
|
||||
str(requested_model or "").strip() != stored_model
|
||||
and not _catalog_model_id_matches(str(requested_model or "").strip(), stored_model)
|
||||
)
|
||||
or requested_provider != stored_provider
|
||||
or (
|
||||
resolved_model != stored_model
|
||||
and not _catalog_model_id_matches(resolved_model, stored_model)
|
||||
)
|
||||
or resolved_provider != stored_provider
|
||||
or not profile_provider
|
||||
or profile_provider == stored_provider
|
||||
):
|
||||
return resolved_provider
|
||||
|
||||
try:
|
||||
catalog = get_available_models(prefer_cache=True)
|
||||
except Exception:
|
||||
return resolved_provider
|
||||
groups = [group for group in catalog.get("groups") or [] if isinstance(group, dict)]
|
||||
stored_groups = [
|
||||
group
|
||||
for group in groups
|
||||
if str(group.get("provider_id") or "").strip().lower() == stored_provider
|
||||
]
|
||||
if (
|
||||
not stored_groups
|
||||
or any(group.get("models_endpoint_error") for group in stored_groups)
|
||||
or any(_catalog_group_owns_exact_model(group, stored_model) for group in stored_groups)
|
||||
):
|
||||
return resolved_provider
|
||||
owners = [
|
||||
group
|
||||
for group in groups
|
||||
if str(group.get("provider_id") or "").strip().lower() != stored_provider
|
||||
and _catalog_group_owns_exact_model(group, stored_model)
|
||||
]
|
||||
if len(owners) != 1:
|
||||
return resolved_provider
|
||||
return str(owners[0].get("provider_id") or "").strip() or resolved_provider
|
||||
|
||||
|
||||
def _clean_session_model_provider(value: str | None) -> str | None:
|
||||
provider = str(value or "").strip().lower()
|
||||
if not provider or provider == "default":
|
||||
|
|
@ -20934,6 +21013,20 @@ def _handle_chat_start(handler, body, diag=None):
|
|||
profile_config=_pp_cfg,
|
||||
explicit_model_pick=explicit_model_pick,
|
||||
)
|
||||
catalog_profile_provider = _pp_provider
|
||||
if catalog_profile_provider is None and isinstance(_pp_cfg, dict):
|
||||
profile_model_config = _pp_cfg.get("model") or {}
|
||||
if isinstance(profile_model_config, dict):
|
||||
catalog_profile_provider = profile_model_config.get("provider")
|
||||
model_provider = _repair_foreign_session_model_provider(
|
||||
s,
|
||||
requested_model=requested_model,
|
||||
requested_provider=requested_provider,
|
||||
resolved_model=model,
|
||||
resolved_provider=model_provider,
|
||||
explicit_model_pick=explicit_model_pick,
|
||||
profile_provider=catalog_profile_provider,
|
||||
)
|
||||
if model_provider == "moa" and moa_config is None:
|
||||
if webui_gateway_chat_enabled(get_config()):
|
||||
return bad(handler, "MoA override is unavailable on gateway-backed sessions", 409)
|
||||
|
|
|
|||
269
tests/test_issue5731_session_model_provider_repair.py
Normal file
269
tests/test_issue5731_session_model_provider_repair.py
Normal file
|
|
@ -0,0 +1,269 @@
|
|||
"""Regression coverage for catalog-backed session provider repair (#5731)."""
|
||||
|
||||
from types import SimpleNamespace
|
||||
|
||||
import pytest
|
||||
|
||||
import api.routes as routes
|
||||
|
||||
|
||||
def _catalog(*groups):
|
||||
return {"groups": list(groups)}
|
||||
|
||||
|
||||
def _group(provider_id, *models):
|
||||
return {"provider_id": provider_id, "models": [{"id": model} for model in models]}
|
||||
|
||||
|
||||
def _session(*, model="kilo/minimax/minimax-m3", provider="ollama"):
|
||||
return SimpleNamespace(model=model, model_provider=provider)
|
||||
|
||||
|
||||
def _repair(
|
||||
session,
|
||||
catalog,
|
||||
*,
|
||||
requested_model=None,
|
||||
requested_provider=None,
|
||||
resolved_model=None,
|
||||
profile_provider="kilocode",
|
||||
explicit_model_pick=False,
|
||||
):
|
||||
return routes._repair_foreign_session_model_provider(
|
||||
session,
|
||||
requested_model=requested_model if requested_model is not None else session.model,
|
||||
requested_provider=requested_provider if requested_provider is not None else session.model_provider,
|
||||
resolved_model=resolved_model if resolved_model is not None else session.model,
|
||||
resolved_provider=session.model_provider,
|
||||
explicit_model_pick=explicit_model_pick,
|
||||
profile_provider=profile_provider,
|
||||
)
|
||||
|
||||
|
||||
def test_poisoned_pair_repairs_at_chat_start(monkeypatch, tmp_path):
|
||||
"""A plain send repairs the documented Kilo model before normal persistence."""
|
||||
session = SimpleNamespace(
|
||||
session_id="issue-5731",
|
||||
workspace=str(tmp_path),
|
||||
model="kilo/minimax/minimax-m3",
|
||||
model_provider="ollama",
|
||||
profile="default",
|
||||
messages=[],
|
||||
context_messages=[],
|
||||
pending_user_message=None,
|
||||
save=lambda: None,
|
||||
)
|
||||
captured = {}
|
||||
catalog_calls = []
|
||||
|
||||
def start_run(s, **kwargs):
|
||||
captured.update(kwargs)
|
||||
routes._prepare_chat_start_session_for_stream(
|
||||
s,
|
||||
msg=kwargs["msg"],
|
||||
attachments=kwargs["attachments"],
|
||||
workspace=kwargs["workspace"],
|
||||
model=kwargs["model"],
|
||||
model_provider=kwargs["model_provider"],
|
||||
stream_id="issue-5731-stream",
|
||||
)
|
||||
return {"stream_id": "issue-5731-stream"}
|
||||
|
||||
monkeypatch.setattr(routes, "_get_or_materialize_session", lambda _sid, **_kwargs: session)
|
||||
monkeypatch.setattr(routes, "_resolve_chat_workspace_with_recovery", lambda _s, _w: str(tmp_path))
|
||||
monkeypatch.setattr(routes, "_read_profile_model_config", lambda _s, _p: (None, None, {"model": {"provider": "kilocode"}}))
|
||||
monkeypatch.setattr(
|
||||
routes,
|
||||
"get_available_models",
|
||||
lambda *, prefer_cache=False: (
|
||||
catalog_calls.append(prefer_cache)
|
||||
or _catalog(
|
||||
_group("ollama", "llama3.2"),
|
||||
_group("kilocode", "@kilocode:kilo/minimax/minimax-m3"),
|
||||
)
|
||||
),
|
||||
)
|
||||
monkeypatch.setattr(routes, "_start_run", start_run)
|
||||
monkeypatch.setattr(routes, "j", lambda _handler, payload, status=200: payload)
|
||||
|
||||
routes._handle_chat_start(None, {"session_id": session.session_id, "message": "continue"})
|
||||
|
||||
assert captured["model_provider"] == "kilocode", captured["model_provider"]
|
||||
assert session.model == "kilo/minimax/minimax-m3"
|
||||
assert session.model_provider == "kilocode"
|
||||
assert catalog_calls == [True]
|
||||
|
||||
|
||||
def test_catalog_equivalent_owner_repairs_poisoned_pair(monkeypatch):
|
||||
session = _session(model="gpt-4o-mini", provider="ollama")
|
||||
monkeypatch.setattr(
|
||||
routes,
|
||||
"get_available_models",
|
||||
lambda *, prefer_cache=False: _catalog(
|
||||
_group("ollama", "llama3.2"),
|
||||
_group("kilocode", "GPT.4O.MINI"),
|
||||
),
|
||||
)
|
||||
|
||||
assert _repair(session, None) == "kilocode"
|
||||
|
||||
|
||||
def test_equivalent_request_model_repairs_poisoned_pair(monkeypatch):
|
||||
session = _session(model="GPT.4O.MINI", provider="ollama")
|
||||
monkeypatch.setattr(
|
||||
routes,
|
||||
"get_available_models",
|
||||
lambda *, prefer_cache=False: _catalog(
|
||||
_group("ollama", "llama3.2"),
|
||||
_group("kilocode", "gpt-4o-mini"),
|
||||
),
|
||||
)
|
||||
|
||||
assert _repair(
|
||||
session,
|
||||
None,
|
||||
requested_model="gpt-4o-mini",
|
||||
resolved_model="gpt-4o-mini",
|
||||
) == "kilocode"
|
||||
|
||||
|
||||
@pytest.mark.parametrize("provider", ["ollama", "lmstudio"])
|
||||
def test_self_hosted_exact_owner_is_preserved(monkeypatch, provider):
|
||||
session = _session(model="vendor/model/with/slashes", provider=provider)
|
||||
monkeypatch.setattr(
|
||||
routes,
|
||||
"get_available_models",
|
||||
lambda *, prefer_cache=False: _catalog(
|
||||
_group(provider, "vendor/model/with/slashes"),
|
||||
_group("kilocode", "other-model"),
|
||||
),
|
||||
)
|
||||
|
||||
assert _repair(session, None) == provider
|
||||
|
||||
|
||||
def test_stored_owner_in_extra_models_is_preserved(monkeypatch):
|
||||
session = _session()
|
||||
stored_group = _group("ollama", "llama3.2")
|
||||
stored_group["extra_models"] = [{"id": "kilo/minimax/minimax-m3"}]
|
||||
monkeypatch.setattr(
|
||||
routes,
|
||||
"get_available_models",
|
||||
lambda *, prefer_cache=False: _catalog(
|
||||
stored_group,
|
||||
_group("kilocode", "kilo/minimax/minimax-m3"),
|
||||
),
|
||||
)
|
||||
|
||||
assert _repair(session, None) == "ollama"
|
||||
|
||||
|
||||
def test_stored_provider_discovery_failure_is_preserved(monkeypatch):
|
||||
session = _session()
|
||||
stored_group = _group("ollama", "llama3.2")
|
||||
stored_group["models_endpoint_error"] = "catalog unavailable"
|
||||
monkeypatch.setattr(
|
||||
routes,
|
||||
"get_available_models",
|
||||
lambda *, prefer_cache=False: _catalog(
|
||||
stored_group,
|
||||
_group("kilocode", "kilo/minimax/minimax-m3"),
|
||||
),
|
||||
)
|
||||
|
||||
assert _repair(session, None) == "ollama"
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("requested_model", "explicit_model_pick"),
|
||||
[
|
||||
("kilo/minimax/minimax-m3", True),
|
||||
("@ollama:kilo/minimax/minimax-m3", False),
|
||||
],
|
||||
)
|
||||
def test_explicit_or_qualified_model_selection_is_preserved(monkeypatch, requested_model, explicit_model_pick):
|
||||
session = _session()
|
||||
monkeypatch.setattr(routes, "get_available_models", lambda **_kwargs: pytest.fail("catalog must not be read"))
|
||||
|
||||
assert _repair(
|
||||
session,
|
||||
None,
|
||||
requested_model=requested_model,
|
||||
explicit_model_pick=explicit_model_pick,
|
||||
) == "ollama"
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"catalog",
|
||||
[
|
||||
_catalog(_group("kilocode", "kilo/minimax/minimax-m3")),
|
||||
_catalog(_group("ollama", "llama3.2")),
|
||||
_catalog(
|
||||
_group("ollama", "llama3.2"),
|
||||
_group("kilocode", "kilo/minimax/minimax-m3"),
|
||||
_group("other", "kilo/minimax/minimax-m3"),
|
||||
),
|
||||
],
|
||||
)
|
||||
def test_missing_or_ambiguous_catalog_evidence_is_preserved(monkeypatch, catalog):
|
||||
session = _session()
|
||||
monkeypatch.setattr(routes, "get_available_models", lambda *, prefer_cache=False: catalog)
|
||||
|
||||
assert _repair(session, catalog) == "ollama"
|
||||
|
||||
|
||||
def test_matching_profile_provider_skips_catalog(monkeypatch):
|
||||
session = _session()
|
||||
monkeypatch.setattr(routes, "get_available_models", lambda **_kwargs: pytest.fail("catalog must not be read"))
|
||||
|
||||
assert _repair(session, None, profile_provider="ollama") == "ollama"
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("body", "catalog"),
|
||||
[
|
||||
({}, _catalog(_group("ollama", "kilo/minimax/minimax-m3"))),
|
||||
({"explicit_model_pick": True}, _catalog(_group("kilocode", "kilo/minimax/minimax-m3"))),
|
||||
({"model": "@ollama:kilo/minimax/minimax-m3"}, _catalog(_group("ollama", "kilo/minimax/minimax-m3"))),
|
||||
(
|
||||
{},
|
||||
_catalog(
|
||||
_group("ollama", "llama3.2"),
|
||||
_group("kilocode", "kilo/minimax/minimax-m3"),
|
||||
_group("other", "kilo/minimax/minimax-m3"),
|
||||
),
|
||||
),
|
||||
({}, RuntimeError("catalog unavailable")),
|
||||
],
|
||||
)
|
||||
def test_preservation_cases_still_reach_chat_start(monkeypatch, tmp_path, body, catalog):
|
||||
session = SimpleNamespace(
|
||||
session_id="issue-5731-preserve",
|
||||
workspace=str(tmp_path),
|
||||
model="kilo/minimax/minimax-m3",
|
||||
model_provider="ollama",
|
||||
profile="default",
|
||||
messages=[],
|
||||
context_messages=[],
|
||||
pending_user_message=None,
|
||||
)
|
||||
captured = {}
|
||||
|
||||
def get_catalog(*, prefer_cache=False):
|
||||
if isinstance(catalog, Exception):
|
||||
raise catalog
|
||||
return catalog
|
||||
|
||||
monkeypatch.setattr(routes, "_get_or_materialize_session", lambda _sid, **_kwargs: session)
|
||||
monkeypatch.setattr(routes, "_resolve_chat_workspace_with_recovery", lambda _s, _w: str(tmp_path))
|
||||
monkeypatch.setattr(routes, "_read_profile_model_config", lambda _s, _p: (None, None, {"model": {"provider": "kilocode"}}))
|
||||
monkeypatch.setattr(routes, "get_available_models", get_catalog)
|
||||
monkeypatch.setattr(routes, "_start_run", lambda _s, **kwargs: captured.update(kwargs) or {"ok": True})
|
||||
monkeypatch.setattr(routes, "j", lambda _handler, payload, status=200: payload)
|
||||
|
||||
routes._handle_chat_start(
|
||||
None,
|
||||
{"session_id": session.session_id, "message": "continue", **body},
|
||||
)
|
||||
|
||||
assert captured["model_provider"] == "ollama"
|
||||
Loading…
Reference in a new issue