diff --git a/Makefile b/Makefile index cfad006..5ec2f15 100644 --- a/Makefile +++ b/Makefile @@ -91,6 +91,15 @@ backend-test-search-top-result: $(BACKEND_VENV_STAMP) ## Run search top result d backend-test-cache-cleanup: $(BACKEND_VENV_STAMP) ## Run cache cleanup tests cd "$(BACKEND_DIR)" && .venv/bin/python -m pytest tests/test_cache_cleanup.py -v +backend-test-lidarr-url: $(BACKEND_VENV_STAMP) ## Run dynamic Lidarr URL resolution tests + cd "$(BACKEND_DIR)" && .venv/bin/python -m pytest tests/test_lidarr_url_dynamic.py -v + +backend-test-sync-coordinator: $(BACKEND_VENV_STAMP) ## Run sync coordinator tests (cooldown, dedup) + cd "$(BACKEND_DIR)" && .venv/bin/python -m pytest tests/test_sync_coordinator.py -v + +backend-test-local-files-fallback: $(BACKEND_VENV_STAMP) ## Run local files stale-while-error fallback tests + cd "$(BACKEND_DIR)" && .venv/bin/python -m pytest tests/test_local_files_fallback.py -v + test-audiodb-all: backend-test-audiodb backend-test-audiodb-prewarm backend-test-audiodb-settings backend-test-coverart-audiodb backend-test-audiodb-phase8 backend-test-audiodb-phase9 frontend-test-audiodb-images ## Run every AudioDB test target frontend-install: ## Install frontend npm dependencies diff --git a/backend/api/v1/routes/library.py b/backend/api/v1/routes/library.py index 8598ab8..d6f7e86 100644 --- a/backend/api/v1/routes/library.py +++ b/backend/api/v1/routes/library.py @@ -95,10 +95,9 @@ async def sync_library( try: return await library_service.sync_library(is_manual=True) except ExternalServiceError as e: - logger.error(f"Couldn't sync the library: {e}") if "cooldown" in str(e).lower(): raise HTTPException(status_code=429, detail="Sync is on cooldown, please wait") - raise HTTPException(status_code=503, detail="External service unavailable") + raise @router.get("/stats", response_model=LibraryStatsResponse) diff --git a/backend/core/exception_handlers.py b/backend/core/exception_handlers.py index 663fdcf..d5e971f 100644 --- a/backend/core/exception_handlers.py +++ b/backend/core/exception_handlers.py @@ -23,6 +23,7 @@ from models.error import ( CONFIGURATION_ERROR, SOURCE_RESOLUTION_ERROR, INTERNAL_ERROR, + CIRCUIT_BREAKER_OPEN, STATUS_TO_CODE, ) @@ -41,7 +42,11 @@ async def external_service_error_handler(request: Request, exc: ExternalServiceE async def circuit_open_error_handler(request: Request, exc: CircuitOpenError) -> MsgSpecJSONResponse: logger.error("Circuit breaker open: %s - %s %s", exc, request.method, request.url.path) - return error_response(status.HTTP_503_SERVICE_UNAVAILABLE, SERVICE_UNAVAILABLE, "Service temporarily unavailable") + return error_response( + status.HTTP_503_SERVICE_UNAVAILABLE, + CIRCUIT_BREAKER_OPEN, + "Service temporarily unavailable due to repeated connection failures. Check your settings or wait for the service to recover.", + ) async def validation_error_handler(request: Request, exc: ValidationError) -> MsgSpecJSONResponse: diff --git a/backend/models/error.py b/backend/models/error.py index f1e2e74..0639076 100644 --- a/backend/models/error.py +++ b/backend/models/error.py @@ -10,6 +10,7 @@ CONFIGURATION_ERROR = "CONFIGURATION_ERROR" SOURCE_RESOLUTION_ERROR = "SOURCE_RESOLUTION_ERROR" INTERNAL_ERROR = "INTERNAL_ERROR" RATE_LIMITED = "RATE_LIMITED" +CIRCUIT_BREAKER_OPEN = "CIRCUIT_BREAKER_OPEN" CLIENT_DISCONNECTED = "CLIENT_DISCONNECTED" FORBIDDEN = "FORBIDDEN" diff --git a/backend/repositories/lidarr/base.py b/backend/repositories/lidarr/base.py index c598ab3..a037c49 100644 --- a/backend/repositories/lidarr/base.py +++ b/backend/repositories/lidarr/base.py @@ -46,7 +46,10 @@ class LidarrBase: self._settings = settings self._client = http_client self._cache = cache - self._base_url = settings.lidarr_url + + @property + def _base_url(self) -> str: + return self._settings.lidarr_url def is_configured(self) -> bool: return bool(self._settings.lidarr_api_key) diff --git a/backend/services/library_service.py b/backend/services/library_service.py index 0d93f29..1e80518 100644 --- a/backend/services/library_service.py +++ b/backend/services/library_service.py @@ -30,6 +30,7 @@ from infrastructure.cache.disk_cache import DiskMetadataCache from infrastructure.cover_urls import prefer_release_group_cover_url from infrastructure.serialization import clone_with_updates from core.exceptions import ExternalServiceError +from infrastructure.resilience.retry import CircuitOpenError from services.cache_status_service import CacheStatusService from services.library_precache_service import LibraryPrecacheService @@ -84,6 +85,7 @@ class LibraryService: self._manual_sync_cooldown: float = 60.0 self._global_sync_cooldown: float = 30.0 self._sync_lock = asyncio.Lock() + self._sync_future: asyncio.Future | None = None def _update_last_sync_timestamp(self) -> None: try: @@ -208,6 +210,8 @@ class LibraryService: for album in albums_data ] return albums, total + except (ExternalServiceError, CircuitOpenError): + raise except Exception as e: # noqa: BLE001 logger.error(f"Failed to fetch paginated albums: {e}") raise ExternalServiceError(f"Failed to fetch paginated albums: {e}") @@ -242,6 +246,8 @@ class LibraryService: for artist in artists_data ] return artists, total + except (ExternalServiceError, CircuitOpenError): + raise except Exception as e: # noqa: BLE001 logger.error(f"Failed to fetch paginated artists: {e}") raise ExternalServiceError(f"Failed to fetch paginated artists: {e}") @@ -317,64 +323,100 @@ class LibraryService: else: logger.info("Library sync already in progress - skipping auto-sync") return SyncLibraryResponse(status="skipped", artists=0, albums=0) + + if self._sync_future is not None and not self._sync_future.done(): + existing_future = self._sync_future + else: + existing_future = None + loop = asyncio.get_running_loop() + self._sync_future = loop.create_future() + + # Shield so waiter cancellation doesn't poison the shared future + if existing_future is not None: + return await asyncio.shield(existing_future) + + sync_succeeded = False + try: + logger.info("Starting library sync from Lidarr") + + albums = await self._lidarr_repo.get_library() + artists = await self._lidarr_repo.get_artists_from_library() - self._last_sync_time = current_time + albums_data = [ + { + 'mbid': album.musicbrainz_id or f"unknown_{album.album}", + 'artist_mbid': album.artist_mbid, + 'artist_name': album.artist, + 'title': album.album, + 'year': album.year, + 'cover_url': self._normalized_album_cover_url( + album.musicbrainz_id, + album.cover_url, + ), + 'monitored': album.monitored, + 'date_added': album.date_added + } + for album in albums + ] + + await self._library_db.save_library(artists, albums_data) + logger.info("Library cache updated - unmonitored items removed") + + now = time.time() + self._last_sync_time = now if is_manual: - self._last_manual_sync = current_time - - logger.info("Starting library sync from Lidarr") + self._last_manual_sync = now - albums = await self._lidarr_repo.get_library() - artists = await self._lidarr_repo.get_artists_from_library() - - albums_data = [ - { - 'mbid': album.musicbrainz_id or f"unknown_{album.album}", - 'artist_mbid': album.artist_mbid, - 'artist_name': album.artist, - 'title': album.album, - 'year': album.year, - 'cover_url': self._normalized_album_cover_url( - album.musicbrainz_id, - album.cover_url, - ), - 'monitored': album.monitored, - 'date_added': album.date_added - } - for album in albums - ] - - await self._library_db.save_library(artists, albums_data) - logger.info("Library cache updated - unmonitored items removed") + if self._precache_service is None: + logger.warning("Precache skipped — sync_state_store/genre_index not provided") + self._update_last_sync_timestamp() + result = SyncLibraryResponse(status='success', artists=len(artists), albums=len(albums)) + self._sync_future.set_result(result) + return result - if self._precache_service is None: - logger.warning("Precache skipped — sync_state_store/genre_index not provided") - return + task = asyncio.create_task(self._precache_service.precache_library_resources(artists, albums)) - task = asyncio.create_task(self._precache_service.precache_library_resources(artists, albums)) + def on_task_done(t: asyncio.Task): + try: + exc = t.exception() + if exc: + logger.error(f"Precache task failed: {exc}") + except asyncio.CancelledError: + logger.info("Precache task was cancelled") + finally: + status_service.set_current_task(None) - def on_task_done(t: asyncio.Task): - try: - exc = t.exception() - if exc: - logger.error(f"Precache task failed: {exc}") - except asyncio.CancelledError: - logger.info("Precache task was cancelled") - finally: - status_service.set_current_task(None) + task.add_done_callback(on_task_done) + status_service.set_current_task(task) - task.add_done_callback(on_task_done) - status_service.set_current_task(task) + logger.info(f"Library sync complete: {len(artists)} artists, {len(albums)} albums") - logger.info(f"Library sync complete: {len(artists)} artists, {len(albums)} albums") + self._update_last_sync_timestamp() - self._update_last_sync_timestamp() - - return SyncLibraryResponse( - status='success', - artists=len(artists), - albums=len(albums), - ) + result = SyncLibraryResponse( + status='success', + artists=len(artists), + albums=len(albums), + ) + sync_succeeded = True + self._sync_future.set_result(result) + return result + except BaseException as exc: + if self._sync_future is not None and not self._sync_future.done(): + self._sync_future.set_exception(exc) + raise + finally: + if not sync_succeeded: + future = self._sync_future + self._sync_future = None + # Suppress "Future exception was never retrieved" if no waiter + if future is not None and future.done() and not future.cancelled(): + try: + future.exception() + except BaseException: + pass + except (ExternalServiceError, CircuitOpenError): + raise except Exception as e: # noqa: BLE001 logger.error(f"Couldn't sync the library: {e}") raise ExternalServiceError(f"Couldn't sync the library: {e}") diff --git a/backend/services/local_files_service.py b/backend/services/local_files_service.py index c6a5a63..77dc50a 100644 --- a/backend/services/local_files_service.py +++ b/backend/services/local_files_service.py @@ -22,6 +22,7 @@ from infrastructure.cache.cache_keys import LOCAL_FILES_PREFIX from infrastructure.cache.memory_cache import CacheInterface from infrastructure.cover_urls import prefer_release_group_cover_url from infrastructure.constants import STREAM_CHUNK_SIZE +from infrastructure.resilience.retry import CircuitOpenError from infrastructure.serialization import to_jsonable from repositories.protocols import LidarrRepositoryProtocol from services.preferences_service import PreferencesService @@ -96,12 +97,24 @@ class LocalFilesService: cached = await self._cache.get(cache_key) if cached is not None: return cached - data = await self._lidarr.get_all_albums() - if data: - await self._cache.set( - cache_key, data, ttl_seconds=self._ALBUM_LIST_TTL - ) - return data or [] + try: + data = await self._lidarr.get_all_albums() + except (ExternalServiceError, CircuitOpenError, ConnectionError, OSError): + # Stale-while-error: serve last-known data if Lidarr is down + try: + stale = await self._cache.get(f"{cache_key}:stale") + except Exception: # noqa: BLE001 + stale = None + if stale is not None: + logger.warning("Lidarr unavailable — serving stale local album data") + return stale + raise + result = data or [] + if result: + await self._cache.set(cache_key, result, ttl_seconds=self._ALBUM_LIST_TTL) + # Keep a long-lived stale copy for fallback (24h) + await self._cache.set(f"{cache_key}:stale", result, ttl_seconds=86400) + return result def _resolve_and_validate_path(self, lidarr_path: str) -> Path: music_path, _ = self._get_config() diff --git a/backend/tests/services/test_local_files_service.py b/backend/tests/services/test_local_files_service.py index f341853..07b6391 100644 --- a/backend/tests/services/test_local_files_service.py +++ b/backend/tests/services/test_local_files_service.py @@ -166,8 +166,8 @@ async def test_get_albums_caches_lidarr_response(service): assert result.total == 1 assert cache.set.called - call_args = cache.set.call_args - assert call_args[0][0] == "local_files_all_albums" + cache_keys = [call.args[0] for call in cache.set.call_args_list] + assert "local_files_all_albums" in cache_keys @pytest.mark.asyncio diff --git a/backend/tests/test_error_leakage.py b/backend/tests/test_error_leakage.py index 56148e2..a76cba9 100644 --- a/backend/tests/test_error_leakage.py +++ b/backend/tests/test_error_leakage.py @@ -70,5 +70,5 @@ async def test_circuit_open_error_hides_details(): body = resp.json() assert resp.status_code == 503 - assert body["error"]["message"] == "Service temporarily unavailable" + assert body["error"]["message"] == "Service temporarily unavailable due to repeated connection failures. Check your settings or wait for the service to recover." assert "JellyfinRepository" not in resp.text diff --git a/backend/tests/test_lidarr_url_dynamic.py b/backend/tests/test_lidarr_url_dynamic.py new file mode 100644 index 0000000..b33482d --- /dev/null +++ b/backend/tests/test_lidarr_url_dynamic.py @@ -0,0 +1,58 @@ +"""Tests that LidarrBase reads URL and API key dynamically from Settings.""" +import pytest +from unittest.mock import MagicMock + + +@pytest.fixture +def mutable_settings(): + settings = MagicMock() + settings.lidarr_url = "http://old-host:8686" + settings.lidarr_api_key = "old-key" + return settings + + +class TestLidarrDynamicUrl: + def test_base_url_reads_from_settings_dynamically(self, mutable_settings): + from repositories.lidarr.base import LidarrBase + + base = LidarrBase(mutable_settings, MagicMock(), MagicMock()) + assert base._base_url == "http://old-host:8686" + + mutable_settings.lidarr_url = "http://192.168.50.99:8686" + assert base._base_url == "http://192.168.50.99:8686" + + def test_api_key_reads_from_settings_dynamically(self, mutable_settings): + from repositories.lidarr.base import LidarrBase + + base = LidarrBase(mutable_settings, MagicMock(), MagicMock()) + headers = base._get_headers() + assert headers["X-Api-Key"] == "old-key" + + mutable_settings.lidarr_api_key = "new-key" + headers = base._get_headers() + assert headers["X-Api-Key"] == "new-key" + + def test_media_cover_url_uses_dynamic_base_url(self, mutable_settings): + from repositories.lidarr.base import LidarrBase + + base = LidarrBase(mutable_settings, MagicMock(), MagicMock()) + + url1 = base._build_api_media_cover_url(1, "poster.jpg", 500) + assert "http://old-host:8686" in url1 + + mutable_settings.lidarr_url = "http://new-host:8686" + url2 = base._build_api_media_cover_url(1, "poster.jpg", 500) + assert "http://new-host:8686" in url2 + assert "http://old-host:8686" not in url2 + + def test_album_cover_url_uses_dynamic_base_url(self, mutable_settings): + from repositories.lidarr.base import LidarrBase + + base = LidarrBase(mutable_settings, MagicMock(), MagicMock()) + + url1 = base._build_api_media_cover_url_album(1, "cover.jpg", 500) + assert "http://old-host:8686" in url1 + + mutable_settings.lidarr_url = "http://new-host:8686" + url2 = base._build_api_media_cover_url_album(1, "cover.jpg", 500) + assert "http://new-host:8686" in url2 diff --git a/backend/tests/test_local_files_fallback.py b/backend/tests/test_local_files_fallback.py new file mode 100644 index 0000000..c95148f --- /dev/null +++ b/backend/tests/test_local_files_fallback.py @@ -0,0 +1,94 @@ +"""Tests for LocalFilesService stale-while-error fallback.""" +import pytest +from unittest.mock import AsyncMock, MagicMock + +from core.exceptions import ExternalServiceError + + +def _make_local_files_service(lidarr=None, cache=None): + from services.local_files_service import LocalFilesService + + lidarr = lidarr or AsyncMock() + prefs = MagicMock() + prefs.get_advanced_settings.return_value = MagicMock( + cache_ttl_local_files_recently_added=120, + cache_ttl_local_files_storage_stats=300, + ) + prefs.get_local_files_connection.return_value = MagicMock( + music_path="/music", lidarr_root_path="/music" + ) + cache = cache or AsyncMock() + return LocalFilesService( + lidarr_repo=lidarr, + preferences_service=prefs, + cache=cache, + ) + + +class TestStaleWhileError: + @pytest.mark.asyncio + async def test_serves_stale_data_when_lidarr_down(self): + stale_albums = [{"id": 1, "title": "Old Album"}] + + cache = AsyncMock() + # Primary cache miss, then stale cache hit + cache.get = AsyncMock(side_effect=lambda key: ( + None if key == "local_files_all_albums" else stale_albums + )) + + lidarr = AsyncMock() + lidarr.get_all_albums = AsyncMock(side_effect=ExternalServiceError("Lidarr down")) + + svc = _make_local_files_service(lidarr=lidarr, cache=cache) + result = await svc._fetch_all_albums() + + assert result == stale_albums + + @pytest.mark.asyncio + async def test_raises_when_no_stale_data(self): + cache = AsyncMock() + cache.get = AsyncMock(return_value=None) # Both caches miss + + lidarr = AsyncMock() + lidarr.get_all_albums = AsyncMock(side_effect=ExternalServiceError("Lidarr down")) + + svc = _make_local_files_service(lidarr=lidarr, cache=cache) + with pytest.raises(ExternalServiceError, match="Lidarr down"): + await svc._fetch_all_albums() + + @pytest.mark.asyncio + async def test_successful_fetch_updates_stale_cache(self): + fresh_albums = [{"id": 2, "title": "Fresh Album"}] + + cache = AsyncMock() + cache.get = AsyncMock(return_value=None) + cache.set = AsyncMock() + + lidarr = AsyncMock() + lidarr.get_all_albums = AsyncMock(return_value=fresh_albums) + + svc = _make_local_files_service(lidarr=lidarr, cache=cache) + result = await svc._fetch_all_albums() + + assert result == fresh_albums + # Should have set both primary and stale caches + assert cache.set.call_count == 2 + calls = {call.args[0] for call in cache.set.call_args_list} + assert "local_files_all_albums" in calls + assert "local_files_all_albums:stale" in calls + + @pytest.mark.asyncio + async def test_cache_hit_returns_without_lidarr_call(self): + cached = [{"id": 3, "title": "Cached"}] + + cache = AsyncMock() + cache.get = AsyncMock(return_value=cached) + + lidarr = AsyncMock() + lidarr.get_all_albums = AsyncMock() + + svc = _make_local_files_service(lidarr=lidarr, cache=cache) + result = await svc._fetch_all_albums() + + assert result == cached + lidarr.get_all_albums.assert_not_called() diff --git a/backend/tests/test_sync_coordinator.py b/backend/tests/test_sync_coordinator.py new file mode 100644 index 0000000..af2b06f --- /dev/null +++ b/backend/tests/test_sync_coordinator.py @@ -0,0 +1,141 @@ +"""Tests for sync coordinator: cooldown on success only, future dedup, race safety.""" +import asyncio +import time + +import pytest +from unittest.mock import AsyncMock, MagicMock, patch + + +def _make_library_service(**overrides): + """Build a minimal LibraryService with mocked deps.""" + from services.library_service import LibraryService + + lidarr = overrides.get("lidarr", AsyncMock()) + lidarr.is_configured = MagicMock(return_value=True) + lidarr.get_library = overrides.get("get_library", AsyncMock(return_value=[])) + lidarr.get_artists_from_library = overrides.get( + "get_artists_from_library", AsyncMock(return_value=[]) + ) + + library_db = overrides.get("library_db", AsyncMock()) + library_db.save_library = AsyncMock() + + prefs = overrides.get("prefs", MagicMock()) + prefs.get_advanced_settings.return_value = MagicMock( + cache_ttl_library_sync=30, + ) + + svc = LibraryService( + lidarr_repo=lidarr, + library_db=library_db, + cover_repo=MagicMock(), + preferences_service=prefs, + ) + return svc + + +class TestCooldownOnlyOnSuccess: + @pytest.mark.asyncio + async def test_failed_sync_does_not_set_cooldown(self): + svc = _make_library_service() + svc._lidarr_repo.get_library = AsyncMock(side_effect=RuntimeError("DNS fail")) + + with patch("services.library_service.CacheStatusService") as mock_css: + mock_css.return_value.is_syncing.return_value = False + + with pytest.raises(Exception, match="DNS fail"): + await svc.sync_library() + + assert svc._last_sync_time == 0.0, "cooldown must NOT activate on failure" + + @pytest.mark.asyncio + async def test_successful_sync_sets_cooldown(self): + svc = _make_library_service() + + with patch("services.library_service.CacheStatusService") as mock_css: + mock_css.return_value.is_syncing.return_value = False + + before = time.time() + result = await svc.sync_library() + after = time.time() + + assert result.status == "success" + assert before <= svc._last_sync_time <= after + + @pytest.mark.asyncio + async def test_retry_after_failed_sync_is_not_cooldown_blocked(self): + call_count = 0 + + async def fail_then_succeed(): + nonlocal call_count + call_count += 1 + if call_count == 1: + raise RuntimeError("temporary failure") + return [] + + svc = _make_library_service() + svc._lidarr_repo.get_library = fail_then_succeed + + with patch("services.library_service.CacheStatusService") as mock_css: + mock_css.return_value.is_syncing.return_value = False + + with pytest.raises(Exception, match="temporary failure"): + await svc.sync_library() + + result = await svc.sync_library() + + assert result.status == "success" + + +class TestSyncFutureDedup: + @pytest.mark.asyncio + async def test_concurrent_syncs_deduplicated(self): + """Two concurrent sync calls should result in exactly one Lidarr call.""" + call_count = 0 + sync_event = asyncio.Event() + + async def slow_get_library(): + nonlocal call_count + call_count += 1 + sync_event.set() + await asyncio.sleep(0.05) + return [] + + svc = _make_library_service() + svc._lidarr_repo.get_library = slow_get_library + + with patch("services.library_service.CacheStatusService") as mock_css: + mock_css.return_value.is_syncing.return_value = False + + results = await asyncio.gather( + svc.sync_library(), + svc.sync_library(), + ) + + assert all(r.status == "success" for r in results) + assert call_count == 1, f"Expected 1 Lidarr call, got {call_count}" + + @pytest.mark.asyncio + async def test_concurrent_sync_failure_propagates_to_waiter(self): + """When the producer fails, deduped waiters get the real exception.""" + async def failing_get_library(): + await asyncio.sleep(0.05) + raise RuntimeError("Lidarr DNS failure") + + svc = _make_library_service() + svc._lidarr_repo.get_library = failing_get_library + + with patch("services.library_service.CacheStatusService") as mock_css: + mock_css.return_value.is_syncing.return_value = False + + results = await asyncio.gather( + svc.sync_library(), + svc.sync_library(), + return_exceptions=True, + ) + + # Both should get an error, not hang or get CancelledError + for r in results: + assert isinstance(r, Exception) + assert not isinstance(r, asyncio.CancelledError), \ + "Waiter got CancelledError instead of the real exception" diff --git a/frontend/src/lib/components/LibraryPage.svelte b/frontend/src/lib/components/LibraryPage.svelte index 0f6839b..b0d5651 100644 --- a/frontend/src/lib/components/LibraryPage.svelte +++ b/frontend/src/lib/components/LibraryPage.svelte @@ -145,7 +145,12 @@ {#if ctrl.fetchError}