fix(models): address code review issues in dynamic model loading

- Fix type mismatch: store str(e) in _LAST_LOAD_ERROR instead of Exception
- Add threading.Lock to protect _CACHE and _LAST_LOAD_ERROR from races
- Remove import wx from presenter; view wraps callback with wx.CallAfter
- Show error modal only when model list is empty (not on stale cache hit)
- Remove bare except Exception around SetStatusText
- Annotate _pending_model_account_id and account_id params as UUID

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Author: clementb49

basilisk/presenters/base_conversation_presenter.py

@@ -11,13 +11,13 @@
 import threading
 from typing import TYPE_CHECKING, Callable
 
-import wx
-
 import basilisk.config as config
 from basilisk.provider_ai_model import ProviderAIModel
 from basilisk.services.account_model_service import AccountModelService
 
 if TYPE_CHECKING:
+	from uuid import UUID
+
 	from basilisk.provider_engine.base_engine import BaseEngine
 
 log = logging.getLogger(__name__)
@@ -50,7 +50,7 @@ def __init__(
 		self._model_loading_cancel_event: threading.Event | None = None
 		self._model_loading_generation: int = 0
 		self._pending_model_id: str | None = None
-		self._pending_model_account_id = None
+		self._pending_model_account_id: UUID | None = None
 
 	def get_engine(self, account: config.Account) -> BaseEngine:
 		"""Get or create an engine for the given account.
@@ -140,7 +140,7 @@ def _load_models_in_background(
 			return
 		if generation != self._model_loading_generation:
 			return
-		wx.CallAfter(on_loaded, account_id, models, error_message)
+		on_loaded(account_id, models, error_message)
 
 	def shutdown_model_loading(self) -> None:
 		"""Cancel and invalidate any in-flight model loading worker."""
@@ -151,7 +151,7 @@ def shutdown_model_loading(self) -> None:
 		self._model_loading_thread = None
 		self._model_loading_cancel_event = None
 
-	def set_pending_model(self, model_id: str, account_id) -> None:
+	def set_pending_model(self, model_id: str, account_id: UUID) -> None:
 		"""Store a deferred model selection to be applied once models load.
 
 		Args:
@@ -162,7 +162,7 @@ def set_pending_model(self, model_id: str, account_id) -> None:
 		self._pending_model_account_id = account_id
 
 	def pop_pending_model(
-		self, displayed_models: list[ProviderAIModel], account_id
+		self, displayed_models: list[ProviderAIModel], account_id: UUID
 	) -> ProviderAIModel | None:
 		"""Find and clear the pending model selection.
 

basilisk/provider_engine/dynamic_model_loader.py

@@ -11,6 +11,7 @@
 from __future__ import annotations
 
 import logging
+import threading
 import time
 from enum import StrEnum, auto
 from functools import cached_property
@@ -33,7 +34,8 @@
 log = logging.getLogger(__name__)
 
 _CACHE: dict[str, tuple[list[ProviderAIModel], float]] = {}
-_LAST_LOAD_ERROR: dict[str, Exception | None] = {}
+_CACHE_LOCK = threading.Lock()
+_LAST_LOAD_ERROR: dict[str, str | None] = {}
 
 
 class ArchitectureModalityToken(StrEnum):
@@ -276,27 +278,30 @@ def load_models_from_url(url: str) -> list[ProviderAIModel]:
 	"""Fetch and parse models from URL, with caching."""
 	now = time.monotonic()
 	ttl = _get_cache_ttl_seconds()
-	if url in _CACHE:
-		cached_models, cached_at = _CACHE[url]
-		if now - cached_at < ttl:
-			_LAST_LOAD_ERROR[url] = None
-			return cached_models
+	with _CACHE_LOCK:
+		if url in _CACHE:
+			cached_models, cached_at = _CACHE[url]
+			if now - cached_at < ttl:
+				_LAST_LOAD_ERROR[url] = None
+				return cached_models
 
 	try:
 		provider_metadata = fetch_models_json(url)
 		models = provider_metadata.get_provider_models()
-		_CACHE[url] = (models, now)
-		_LAST_LOAD_ERROR[url] = None
+		with _CACHE_LOCK:
+			_CACHE[url] = (models, now)
+			_LAST_LOAD_ERROR[url] = None
 		log.debug("Loaded %d models from %s", len(models), url)
 		return models
 	except (httpx.HTTPError, ValidationError, ValueError, TypeError) as e:
 		log.warning("Failed to load models from %s: %s", url, e)
-		_LAST_LOAD_ERROR[url] = e
-		if url in _CACHE:
-			return _CACHE[url][0]
-		return []
+		with _CACHE_LOCK:
+			_LAST_LOAD_ERROR[url] = str(e)
+			stale = _CACHE.get(url)
+		return stale[0] if stale else []
 
 
-def get_last_load_error(url: str) -> Exception | None:
+def get_last_load_error(url: str) -> str | None:
 	"""Get the most recent load error for a metadata URL."""
-	return _LAST_LOAD_ERROR.get(url)
+	with _CACHE_LOCK:
+		return _LAST_LOAD_ERROR.get(url)

basilisk/views/base_conversation.py

@@ -226,7 +226,9 @@ def update_model_list(self):
 			return
 		self.model_list.Append((_("Loading..."), "", "", ""))
 		self.base_conv_presenter.start_model_loading(
-			account, engine, self._on_models_loaded
+			account,
+			engine,
+			lambda *args: wx.CallAfter(self._on_models_loaded, *args),
 		)
 
 	def get_display_models(self) -> list[tuple[str, str, str]]:
@@ -344,21 +346,16 @@ def _on_models_loaded(
 		for model in models:
 			self.model_list.Append(model.display_model)
 		if error_message:
+			if hasattr(self, "SetStatusText"):
+				self.SetStatusText(_("Model loading failed"))
 			if not models:
 				self.model_list.Append((_("Error loading models"), "", "", ""))
-			if hasattr(self, "SetStatusText"):
-				try:
-					self.SetStatusText(_("Model loading failed"))
-				except Exception:
-					log.debug(
-						"Could not set status text for model loading error"
+				if hasattr(self, "show_error"):
+					self.show_error(
+						_("An error occurred while loading models:\n%s")
+						% error_message,
+						_("Model loading error"),
 					)
-			if hasattr(self, "show_error"):
-				self.show_error(
-					_("An error occurred while loading models:\n%s")
-					% error_message,
-					_("Model loading error"),
-				)
 		self._try_select_pending_model()
 
 	def _try_select_pending_model(self):

tests/presenters/test_base_conversation_presenter.py

@@ -318,9 +318,8 @@ def _make_engine(self, models=None, error=None):
 		engine.get_model_loading_error.return_value = error
 		return engine
 
-	def test_calls_on_loaded_with_models(self, presenter, mocker):
-		"""Calls wx.CallAfter(on_loaded, account_id, models, None) on success."""
-		mock_call_after = mocker.patch("wx.CallAfter")
+	def test_calls_on_loaded_with_models(self, presenter):
+		"""Calls on_loaded(account_id, models, None) directly on success."""
 		on_loaded = MagicMock()
 		model = _make_provider_model("gpt-4")
 		engine = self._make_engine(models=[model])
@@ -329,25 +328,21 @@ def test_calls_on_loaded_with_models(self, presenter, mocker):
 		presenter._load_models_in_background(
 			"acct-1", engine, 0, cancel_event, on_loaded
 		)
-		mock_call_after.assert_called_once_with(
-			on_loaded, "acct-1", [model], None
-		)
+		on_loaded.assert_called_once_with("acct-1", [model], None)
 
-	def test_skips_callback_when_cancelled(self, presenter, mocker):
-		"""Does not call wx.CallAfter when cancel_event is set."""
-		mock_call_after = mocker.patch("wx.CallAfter")
+	def test_skips_callback_when_cancelled(self, presenter):
+		"""Does not call on_loaded when cancel_event is set."""
 		on_loaded = MagicMock()
 		engine = self._make_engine(models=[_make_provider_model("gpt-4")])
 		cancel_event = MagicMock()
 		cancel_event.is_set.return_value = True
 		presenter._load_models_in_background(
 			"acct-1", engine, 0, cancel_event, on_loaded
 		)
-		mock_call_after.assert_not_called()
+		on_loaded.assert_not_called()
 
-	def test_skips_callback_when_generation_stale(self, presenter, mocker):
-		"""Does not call wx.CallAfter when generation counter has advanced."""
-		mock_call_after = mocker.patch("wx.CallAfter")
+	def test_skips_callback_when_generation_stale(self, presenter):
+		"""Does not call on_loaded when generation counter has advanced."""
 		on_loaded = MagicMock()
 		engine = self._make_engine(models=[_make_provider_model("gpt-4")])
 		cancel_event = MagicMock()
@@ -356,11 +351,10 @@ def test_skips_callback_when_generation_stale(self, presenter, mocker):
 		presenter._load_models_in_background(
 			"acct-1", engine, 0, cancel_event, on_loaded
 		)
-		mock_call_after.assert_not_called()
+		on_loaded.assert_not_called()
 
-	def test_invalidates_cache_on_error_with_no_models(self, presenter, mocker):
+	def test_invalidates_cache_on_error_with_no_models(self, presenter):
 		"""Calls invalidate_models_cache() when there is an error but no models."""
-		mocker.patch("wx.CallAfter")
 		on_loaded = MagicMock()
 		engine = self._make_engine(models=[], error="Network error")
 		cancel_event = MagicMock()
@@ -370,9 +364,8 @@ def test_invalidates_cache_on_error_with_no_models(self, presenter, mocker):
 		)
 		engine.invalidate_models_cache.assert_called_once()
 
-	def test_exception_yields_empty_models_and_error(self, presenter, mocker):
-		"""On exception, calls wx.CallAfter with empty models and an error message."""
-		mock_call_after = mocker.patch("wx.CallAfter")
+	def test_exception_yields_empty_models_and_error(self, presenter):
+		"""On exception, calls on_loaded with empty models and an error message."""
 		on_loaded = MagicMock()
 		engine = MagicMock()
 		engine.models = MagicMock(side_effect=RuntimeError("boom"))
@@ -381,8 +374,8 @@ def test_exception_yields_empty_models_and_error(self, presenter, mocker):
 		presenter._load_models_in_background(
 			"acct-1", engine, 0, cancel_event, on_loaded
 		)
-		mock_call_after.assert_called_once()
-		__, account_id, models, error_message = mock_call_after.call_args[0]
+		on_loaded.assert_called_once()
+		account_id, models, error_message = on_loaded.call_args[0]
 		assert account_id == "acct-1"
 		assert models == []
 		assert error_message is not None