diff --git a/routstr/core/admin.py b/routstr/core/admin.py index f3862edc..365e2f68 100644 --- a/routstr/core/admin.py +++ b/routstr/core/admin.py @@ -1,8 +1,6 @@ -import asyncio import json import re import secrets -import time from datetime import datetime, timezone from pathlib import Path @@ -56,9 +54,6 @@ async def _refresh_provider_model_paths(upstream_provider_id: int) -> None: """Queue discovery sync without blocking the committed admin mutation.""" from ..upstream.model_paths import schedule_model_paths_refresh_for_provider - # Every provider/model mutation funnels through here, so it is also the one - # place that can keep the cached admin catalog from serving a stale listing. - invalidate_remote_models_cache(upstream_provider_id) await schedule_model_paths_refresh_for_provider(upstream_provider_id) @@ -1188,7 +1183,6 @@ async def delete_upstream_provider(provider_id: str) -> dict[str, object]: await session.delete(provider) await session.commit() - invalidate_remote_models_cache(deleted_id) await reinitialize_upstreams() await refresh_model_maps() return {"ok": True, "deleted_id": deleted_id} @@ -1202,78 +1196,13 @@ async def get_provider_types() -> list[dict[str, object]]: return [cls.get_provider_metadata() for cls in upstream_provider_classes] -# The admin catalog view is opened repeatedly and by several panels at once, -# while every miss costs a live upstream round trip. Keep the raw listing for a -# short window and let concurrent readers share one in-flight fetch. -_REMOTE_MODELS_TTL_SECONDS = 120.0 -_REMOTE_MODELS_FETCH_TIMEOUT_SECONDS = 20.0 -_remote_models_cache: dict[int, tuple[float, list]] = {} -_remote_models_locks: dict[int, asyncio.Lock] = {} -# Bumped on every invalidation so a fetch that started against the old provider -# config cannot write its result back after the cache was cleared. -_remote_models_generation = 0 - - -def invalidate_remote_models_cache(provider_pk: int | None = None) -> None: - global _remote_models_generation - _remote_models_generation += 1 - if provider_pk is None: - _remote_models_cache.clear() - _remote_models_locks.clear() - else: - _remote_models_cache.pop(provider_pk, None) - - -async def _get_remote_models( - provider: UpstreamProviderRow, provider_pk: int, force_refresh: bool = False -) -> list: - from ..upstream.helpers import _instantiate_provider - - now = time.monotonic() - cached = _remote_models_cache.get(provider_pk) - if not force_refresh and cached and now - cached[0] < _REMOTE_MODELS_TTL_SECONDS: - return cached[1] - - lock = _remote_models_locks.setdefault(provider_pk, asyncio.Lock()) - async with lock: - cached = _remote_models_cache.get(provider_pk) - now = time.monotonic() - if ( - not force_refresh - and cached - and now - cached[0] < _REMOTE_MODELS_TTL_SECONDS - ): - return cached[1] - - upstream_instance = _instantiate_provider(provider) - if not upstream_instance: - return [] - - generation = _remote_models_generation - try: - models = await asyncio.wait_for( - upstream_instance.fetch_models(), - timeout=_REMOTE_MODELS_FETCH_TIMEOUT_SECONDS, - ) - except Exception as e: - logger.error(f"Failed to fetch models from {provider.provider_type}: {e}") - # A stale listing beats an empty one for an operator view. - return cached[1] if cached else [] - - if generation == _remote_models_generation: - _remote_models_cache[provider_pk] = (time.monotonic(), models) - return models - - @admin_router.get( "/api/upstream-providers/{provider_id}/models", dependencies=[Depends(require_admin_api)], ) -async def get_provider_models( - provider_id: str, - include_remote: bool = Query(True), - refresh_remote: bool = Query(False), -) -> dict[str, object]: +async def get_provider_models(provider_id: str) -> dict[str, object]: + from ..upstream.helpers import _instantiate_provider + async with create_session() as session: provider = await _get_upstream_provider_by_ref(session, provider_id) provider_pk = _provider_pk(provider) @@ -1285,11 +1214,16 @@ async def get_provider_models( apply_fees=False, ) - upstream_models: list = [] - if include_remote: - upstream_models = await _get_remote_models( - provider, provider_pk, force_refresh=refresh_remote - ) + upstream_models = [] + upstream_instance = _instantiate_provider(provider) + if upstream_instance: + try: + raw_models = await upstream_instance.fetch_models() + upstream_models = raw_models + except Exception as e: + logger.error( + f"Failed to fetch models from {provider.provider_type}: {e}" + ) db_model_ids = {model.id for model in db_models} filtered_remote_models = [ diff --git a/tests/conftest.py b/tests/conftest.py index 74c3d058..d1bfa919 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -31,18 +31,3 @@ def _isolate_redemption_negative_cache() -> Iterator[None]: redemption_negative_cache.clear() yield redemption_negative_cache.clear() - - -@pytest.fixture(autouse=True) -def _isolate_admin_remote_models_cache() -> Iterator[None]: - """Clear the admin catalog cache between tests. - - Provider primary keys restart at 1 for every fresh test database, so a - cached listing from an earlier test would otherwise answer for a different - provider that happens to reuse the same key. - """ - from routstr.core.admin import invalidate_remote_models_cache - - invalidate_remote_models_cache() - yield - invalidate_remote_models_cache() diff --git a/tests/unit/test_admin_remote_models_cache.py b/tests/unit/test_admin_remote_models_cache.py deleted file mode 100644 index 6a22ce00..00000000 --- a/tests/unit/test_admin_remote_models_cache.py +++ /dev/null @@ -1,112 +0,0 @@ -"""Cache behavior of the admin provider catalog listing.""" - -import asyncio -from types import SimpleNamespace -from typing import Any -from unittest.mock import AsyncMock, patch - -import pytest - -from routstr.core import admin -from routstr.core.admin import _get_remote_models, invalidate_remote_models_cache - -PROVIDER = SimpleNamespace(provider_type="generic") - - -def _upstream(fetch: Any) -> Any: - return patch( - "routstr.upstream.helpers._instantiate_provider", - return_value=SimpleNamespace(fetch_models=fetch), - ) - - -@pytest.mark.asyncio -async def test_second_read_is_served_from_cache() -> None: - fetch = AsyncMock(return_value=["a"]) - with _upstream(fetch): - assert await _get_remote_models(PROVIDER, 1) == ["a"] # type: ignore[arg-type] - assert await _get_remote_models(PROVIDER, 1) == ["a"] # type: ignore[arg-type] - assert fetch.await_count == 1 - - -@pytest.mark.asyncio -async def test_force_refresh_and_invalidation_refetch() -> None: - fetch = AsyncMock(side_effect=[["a"], ["b"], ["c"]]) - with _upstream(fetch): - await _get_remote_models(PROVIDER, 1) # type: ignore[arg-type] - assert await _get_remote_models(PROVIDER, 1, force_refresh=True) == ["b"] # type: ignore[arg-type] - invalidate_remote_models_cache(1) - assert await _get_remote_models(PROVIDER, 1) == ["c"] # type: ignore[arg-type] - - -@pytest.mark.asyncio -async def test_expired_entry_is_refetched() -> None: - fetch = AsyncMock(side_effect=[["a"], ["b"]]) - with _upstream(fetch): - await _get_remote_models(PROVIDER, 1) # type: ignore[arg-type] - stamp, models = admin._remote_models_cache[1] - admin._remote_models_cache[1] = ( - stamp - admin._REMOTE_MODELS_TTL_SECONDS - 1, - models, - ) - assert await _get_remote_models(PROVIDER, 1) == ["b"] # type: ignore[arg-type] - - -@pytest.mark.asyncio -async def test_failed_refresh_falls_back_to_stale_listing() -> None: - fetch = AsyncMock(side_effect=[["a"], RuntimeError("upstream down")]) - with _upstream(fetch): - await _get_remote_models(PROVIDER, 1) # type: ignore[arg-type] - assert await _get_remote_models(PROVIDER, 1, force_refresh=True) == ["a"] # type: ignore[arg-type] - - -@pytest.mark.asyncio -async def test_failed_first_fetch_returns_empty_and_caches_nothing() -> None: - fetch = AsyncMock(side_effect=RuntimeError("upstream down")) - with _upstream(fetch): - assert await _get_remote_models(PROVIDER, 1) == [] # type: ignore[arg-type] - assert 1 not in admin._remote_models_cache - - -@pytest.mark.asyncio -async def test_concurrent_readers_share_one_fetch() -> None: - release = asyncio.Event() - - async def slow_fetch() -> list[str]: - await release.wait() - return ["a"] - - fetch = AsyncMock(side_effect=slow_fetch) - with _upstream(fetch): - readers = [ - asyncio.create_task(_get_remote_models(PROVIDER, 1)) # type: ignore[arg-type] - for _ in range(5) - ] - await asyncio.sleep(0) - release.set() - results = await asyncio.gather(*readers) - assert results == [["a"]] * 5 - assert fetch.await_count == 1 - - -@pytest.mark.asyncio -async def test_invalidation_during_fetch_discards_in_flight_result() -> None: - release = asyncio.Event() - - async def slow_fetch() -> list[str]: - await release.wait() - return ["old"] - - with _upstream(AsyncMock(side_effect=slow_fetch)): - reader = asyncio.create_task(_get_remote_models(PROVIDER, 1)) # type: ignore[arg-type] - await asyncio.sleep(0) - invalidate_remote_models_cache(1) - release.set() - assert await reader == ["old"] - assert 1 not in admin._remote_models_cache - - -@pytest.mark.asyncio -async def test_uninstantiable_provider_returns_empty() -> None: - with patch("routstr.upstream.helpers._instantiate_provider", return_value=None): - assert await _get_remote_models(PROVIDER, 1) == [] # type: ignore[arg-type] diff --git a/ui/components/model-selector.tsx b/ui/components/model-selector.tsx index 32552ef3..54a345b8 100644 --- a/ui/components/model-selector.tsx +++ b/ui/components/model-selector.tsx @@ -28,7 +28,7 @@ import { AlertDialogHeader, AlertDialogTitle, } from '@/components/ui/alert-dialog'; -import { Trash2, Ban, CheckCircle, Loader2, Plus } from 'lucide-react'; +import { Trash2, Ban, CheckCircle, Plus } from 'lucide-react'; import { toast } from 'sonner'; import { sortModels, @@ -137,7 +137,6 @@ export function ModelSelector({ models, groups, isLoading: isLoadingModels, - isFetchingRemote, error: modelsError, refetch: refetchModels, } = useModelsWithProviders(); @@ -865,19 +864,11 @@ export function ModelSelector({ {Object.keys(groupedModels).length === 0 ? ( - isFetchingRemote ? ( -
- Try broadening your search or switch to a different provider - scope. -
-+ Try broadening your search or switch to a different provider scope. +
+