feat: Requests / Add to Library Rework - Unmonitored album default + … (#25)
* feat: Requests / Add to Library Rework - Unmonitored album default + Resilience * checking for source + refresh album logic * artist monitoring + auto downloading + various request fixes * synchronous album requests * format
This commit is contained in:
Harvey
GitHub
@@ -25,11 +25,14 @@ async def test_jobs_survive_restart(store: QueueStore):
|
||||
store.enqueue("job-1", "mbid-abc")
|
||||
store.mark_processing("job-1")
|
||||
|
||||
q1._processor_task.cancel()
|
||||
try:
|
||||
await q1._processor_task
|
||||
except asyncio.CancelledError:
|
||||
pass
|
||||
for task in q1._worker_tasks:
|
||||
task.cancel()
|
||||
for task in q1._worker_tasks:
|
||||
try:
|
||||
await task
|
||||
except asyncio.CancelledError:
|
||||
pass
|
||||
q1._worker_tasks.clear()
|
||||
|
||||
fast_processed = []
|
||||
|
||||
|
||||
@@ -0,0 +1,210 @@
|
||||
"""Tests for MUS-14 queue changes: dedup, cancel, concurrency, atomic enqueue."""
|
||||
|
||||
import asyncio
|
||||
from pathlib import Path
|
||||
from unittest.mock import MagicMock
|
||||
|
||||
import pytest
|
||||
|
||||
from infrastructure.queue.queue_store import QueueStore
|
||||
from infrastructure.queue.request_queue import RequestQueue
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def store(tmp_path: Path) -> QueueStore:
|
||||
return QueueStore(db_path=tmp_path / "test_queue.db")
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_enqueue_dedup_rejects_duplicate(store: QueueStore):
|
||||
"""Duplicate MBIDs are rejected by enqueue()."""
|
||||
processed = []
|
||||
|
||||
async def processor(mbid: str) -> dict:
|
||||
await asyncio.sleep(10)
|
||||
processed.append(mbid)
|
||||
return {"status": "ok"}
|
||||
|
||||
q = RequestQueue(processor=processor, store=store)
|
||||
first = await q.enqueue("mbid-aaa")
|
||||
second = await q.enqueue("mbid-aaa")
|
||||
|
||||
assert first is True
|
||||
assert second is False
|
||||
await q.stop()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_enqueue_dedup_is_atomic(store: QueueStore):
|
||||
"""Concurrent enqueue calls for the same MBID should only succeed once."""
|
||||
processed = []
|
||||
|
||||
async def slow_processor(mbid: str) -> dict:
|
||||
await asyncio.sleep(10)
|
||||
processed.append(mbid)
|
||||
return {"status": "ok"}
|
||||
|
||||
q = RequestQueue(processor=slow_processor, store=store)
|
||||
results = await asyncio.gather(
|
||||
q.enqueue("mbid-race"),
|
||||
q.enqueue("mbid-race"),
|
||||
q.enqueue("mbid-race"),
|
||||
)
|
||||
|
||||
assert sum(results) == 1
|
||||
await q.stop()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_cancel_skips_queued_item(store: QueueStore):
|
||||
"""Cancelled items are skipped by the worker."""
|
||||
processed = []
|
||||
gate = asyncio.Event()
|
||||
|
||||
async def gated_processor(mbid: str) -> dict:
|
||||
await gate.wait()
|
||||
processed.append(mbid)
|
||||
return {"status": "ok"}
|
||||
|
||||
q = RequestQueue(processor=gated_processor, store=store, concurrency=1)
|
||||
await q.enqueue("mbid-first")
|
||||
await q.enqueue("mbid-second")
|
||||
|
||||
await q.cancel("mbid-second")
|
||||
gate.set()
|
||||
await asyncio.sleep(0.5)
|
||||
|
||||
assert "mbid-first" in processed
|
||||
assert "mbid-second" not in processed
|
||||
await q.stop()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_concurrent_workers_process_in_parallel(store: QueueStore):
|
||||
"""Multiple workers process items concurrently."""
|
||||
active = {"count": 0, "max": 0}
|
||||
lock = asyncio.Lock()
|
||||
|
||||
async def tracking_processor(mbid: str) -> dict:
|
||||
async with lock:
|
||||
active["count"] += 1
|
||||
active["max"] = max(active["max"], active["count"])
|
||||
await asyncio.sleep(0.2)
|
||||
async with lock:
|
||||
active["count"] -= 1
|
||||
return {"status": "ok"}
|
||||
|
||||
q = RequestQueue(processor=tracking_processor, store=store, concurrency=3)
|
||||
|
||||
await q.enqueue("mbid-a")
|
||||
await q.enqueue("mbid-b")
|
||||
await q.enqueue("mbid-c")
|
||||
|
||||
await asyncio.sleep(0.5)
|
||||
|
||||
assert active["max"] >= 2
|
||||
await q.stop()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_store_persisted_before_memory_queue():
|
||||
"""enqueue() persists to store before putting in asyncio.Queue."""
|
||||
call_order: list[str] = []
|
||||
|
||||
class TrackedStore:
|
||||
def has_active_mbid(self, mbid: str) -> bool:
|
||||
return False
|
||||
|
||||
def enqueue(self, job_id: str, mbid: str) -> bool:
|
||||
call_order.append("store_enqueue")
|
||||
return True
|
||||
|
||||
def get_dead_letter_count(self) -> int:
|
||||
return 0
|
||||
|
||||
def get_all(self) -> list:
|
||||
return []
|
||||
|
||||
def reset_processing(self) -> None:
|
||||
pass
|
||||
|
||||
def get_pending(self) -> list:
|
||||
return []
|
||||
|
||||
def get_retryable_dead_letters(self) -> list:
|
||||
return []
|
||||
|
||||
def mark_processing(self, job_id: str) -> None:
|
||||
pass
|
||||
|
||||
def dequeue(self, job_id: str) -> None:
|
||||
pass
|
||||
|
||||
original_put = asyncio.Queue.put
|
||||
|
||||
async def tracked_put(self, item):
|
||||
call_order.append("queue_put")
|
||||
return await original_put(self, item)
|
||||
|
||||
asyncio.Queue.put = tracked_put
|
||||
try:
|
||||
async def noop_processor(mbid: str) -> dict:
|
||||
return {"status": "ok"}
|
||||
|
||||
q = RequestQueue(processor=noop_processor, store=TrackedStore())
|
||||
await q.enqueue("mbid-order-test")
|
||||
|
||||
assert call_order.index("store_enqueue") < call_order.index("queue_put")
|
||||
await q.stop()
|
||||
finally:
|
||||
asyncio.Queue.put = original_put
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_cancel_then_re_request_processes_new_request(store: QueueStore):
|
||||
"""A cancelled MBID can be re-requested and will be processed."""
|
||||
processed = []
|
||||
gate = asyncio.Event()
|
||||
|
||||
async def gated_processor(mbid: str) -> dict:
|
||||
await gate.wait()
|
||||
processed.append(mbid)
|
||||
return {"status": "ok"}
|
||||
|
||||
q = RequestQueue(processor=gated_processor, store=store, concurrency=1)
|
||||
|
||||
# Enqueue, cancel, then re-enqueue the same MBID
|
||||
first = await q.enqueue("mbid-bounce")
|
||||
assert first is True
|
||||
await q.cancel("mbid-bounce")
|
||||
second = await q.enqueue("mbid-bounce")
|
||||
assert second is True
|
||||
|
||||
gate.set()
|
||||
await asyncio.sleep(0.5)
|
||||
|
||||
# The re-request should have been processed (not skipped by stale cancel)
|
||||
assert "mbid-bounce" in processed
|
||||
await q.stop()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_cancelled_mbids_bounded(store: QueueStore):
|
||||
"""_cancelled_mbids set doesn't grow unbounded."""
|
||||
async def noop(mbid: str) -> dict:
|
||||
return {"status": "ok"}
|
||||
|
||||
q = RequestQueue(processor=noop, store=store, concurrency=1)
|
||||
|
||||
# Cancel 250 non-existent MBIDs (orphan cancels)
|
||||
for i in range(250):
|
||||
await q.cancel(f"orphan-{i}")
|
||||
|
||||
assert len(q._cancelled_mbids) == 250
|
||||
|
||||
# Process one item — triggers the cleanup threshold
|
||||
await q.enqueue("mbid-trigger")
|
||||
await asyncio.sleep(0.5)
|
||||
|
||||
assert len(q._cancelled_mbids) <= 200
|
||||
await q.stop()
|
||||
@@ -0,0 +1,101 @@
|
||||
"""Tests for MUS-14 album.py changes: per-artist lock, safe LRU eviction."""
|
||||
|
||||
import asyncio
|
||||
|
||||
import pytest
|
||||
|
||||
from repositories.lidarr.album import _get_artist_lock, _artist_locks, _MAX_ARTIST_LOCKS
|
||||
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def _clear_locks():
|
||||
"""Reset the module-level lock dict between tests."""
|
||||
_artist_locks.clear()
|
||||
yield
|
||||
_artist_locks.clear()
|
||||
|
||||
|
||||
def test_get_artist_lock_returns_same_lock_for_same_mbid():
|
||||
lock1 = _get_artist_lock("artist-aaa")
|
||||
lock2 = _get_artist_lock("artist-aaa")
|
||||
assert lock1 is lock2
|
||||
|
||||
|
||||
def test_get_artist_lock_returns_different_locks_for_different_mbids():
|
||||
lock1 = _get_artist_lock("artist-aaa")
|
||||
lock2 = _get_artist_lock("artist-bbb")
|
||||
assert lock1 is not lock2
|
||||
|
||||
|
||||
def test_lru_eviction_respects_max():
|
||||
for i in range(_MAX_ARTIST_LOCKS + 10):
|
||||
_get_artist_lock(f"artist-{i}")
|
||||
assert len(_artist_locks) <= _MAX_ARTIST_LOCKS
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_lru_eviction_skips_held_locks():
|
||||
"""A lock that is currently held must not be evicted."""
|
||||
first_lock = _get_artist_lock("artist-held")
|
||||
await first_lock.acquire()
|
||||
|
||||
try:
|
||||
for i in range(_MAX_ARTIST_LOCKS + 5):
|
||||
_get_artist_lock(f"artist-fill-{i}")
|
||||
|
||||
assert "artist-held" in _artist_locks
|
||||
assert _artist_locks["artist-held"] is first_lock
|
||||
finally:
|
||||
first_lock.release()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_per_artist_lock_serializes_concurrent_calls():
|
||||
"""Concurrent calls for the same artist should be serialized."""
|
||||
call_order: list[str] = []
|
||||
|
||||
async def simulated_add(artist_mbid: str, label: str):
|
||||
lock = _get_artist_lock(artist_mbid)
|
||||
async with lock:
|
||||
call_order.append(f"{label}-start")
|
||||
await asyncio.sleep(0.1)
|
||||
call_order.append(f"{label}-end")
|
||||
|
||||
await asyncio.gather(
|
||||
simulated_add("same-artist", "A"),
|
||||
simulated_add("same-artist", "B"),
|
||||
)
|
||||
|
||||
# One must complete fully before the other starts
|
||||
a_start = call_order.index("A-start")
|
||||
a_end = call_order.index("A-end")
|
||||
b_start = call_order.index("B-start")
|
||||
b_end = call_order.index("B-end")
|
||||
|
||||
serialized = (a_end < b_start) or (b_end < a_start)
|
||||
assert serialized, f"Expected serialized execution, got: {call_order}"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_different_artists_run_concurrently():
|
||||
"""Requests for different artists should NOT be serialized."""
|
||||
active = {"count": 0, "max": 0}
|
||||
lock = asyncio.Lock()
|
||||
|
||||
async def simulated_add(artist_mbid: str):
|
||||
artist_lock = _get_artist_lock(artist_mbid)
|
||||
async with artist_lock:
|
||||
async with lock:
|
||||
active["count"] += 1
|
||||
active["max"] = max(active["max"], active["count"])
|
||||
await asyncio.sleep(0.1)
|
||||
async with lock:
|
||||
active["count"] -= 1
|
||||
|
||||
await asyncio.gather(
|
||||
simulated_add("artist-X"),
|
||||
simulated_add("artist-Y"),
|
||||
simulated_add("artist-Z"),
|
||||
)
|
||||
|
||||
assert active["max"] >= 2, f"Expected concurrent execution, max active was {active['max']}"
|
||||
@@ -166,14 +166,32 @@ class TestSharedRawAlbumCache:
|
||||
},
|
||||
]
|
||||
|
||||
library_mbids, requested_mbids = await asyncio.gather(
|
||||
library_mbids, monitored_no_files = await asyncio.gather(
|
||||
repo.get_library_mbids(include_release_ids=False),
|
||||
repo.get_requested_mbids(),
|
||||
repo.get_monitored_no_files_mbids(),
|
||||
)
|
||||
|
||||
assert mock_get.await_count == 1
|
||||
assert library_mbids == {"aaaa"}
|
||||
assert requested_mbids == {"bbbb"}
|
||||
assert monitored_no_files == {"bbbb"}
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_requested_mbids_uses_history_store(self, repo):
|
||||
"""get_requested_mbids delegates to RequestHistoryStore."""
|
||||
mock_store = AsyncMock()
|
||||
mock_store.async_get_active_mbids = AsyncMock(return_value={"cccc", "dddd"})
|
||||
repo._request_history_store = mock_store
|
||||
|
||||
result = await repo.get_requested_mbids()
|
||||
assert result == {"cccc", "dddd"}
|
||||
mock_store.async_get_active_mbids.assert_awaited_once()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_requested_mbids_returns_empty_without_store(self, repo):
|
||||
"""get_requested_mbids returns empty set when no history store."""
|
||||
repo._request_history_store = None
|
||||
result = await repo.get_requested_mbids()
|
||||
assert result == set()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_explicit_album_cache_invalidation_forces_refetch(self, repo):
|
||||
|
||||
@@ -0,0 +1,90 @@
|
||||
import os
|
||||
import tempfile
|
||||
|
||||
import pytest
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
# Ensure config uses a writable temp dir for tests
|
||||
_test_dir = tempfile.mkdtemp()
|
||||
os.environ.setdefault("ROOT_APP_DIR", _test_dir)
|
||||
|
||||
|
||||
VALID_MBID = "77434d0b-1c5f-48c3-8694-050cb378ebd2"
|
||||
UNKNOWN_MBID = "00000000-0000-0000-0000-000000000000"
|
||||
|
||||
|
||||
def _make_basic_info():
|
||||
return MagicMock(
|
||||
release_group_id=VALID_MBID,
|
||||
title="Test Album",
|
||||
artist_name="Test Artist",
|
||||
in_library=True,
|
||||
)
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def mock_album_service():
|
||||
svc = MagicMock()
|
||||
svc.refresh_album = AsyncMock()
|
||||
svc.get_album_basic_info = AsyncMock(return_value=_make_basic_info())
|
||||
return svc
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def mock_navidrome_service():
|
||||
svc = MagicMock()
|
||||
svc.invalidate_album_cache = MagicMock()
|
||||
return svc
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def client(mock_album_service, mock_navidrome_service):
|
||||
from fastapi import FastAPI
|
||||
from fastapi.testclient import TestClient
|
||||
from api.v1.routes.albums import router
|
||||
from core.dependencies import get_album_service, get_navidrome_library_service
|
||||
|
||||
app = FastAPI()
|
||||
app.include_router(router)
|
||||
app.dependency_overrides[get_album_service] = lambda: mock_album_service
|
||||
app.dependency_overrides[get_navidrome_library_service] = lambda: mock_navidrome_service
|
||||
return TestClient(app)
|
||||
|
||||
|
||||
def test_refresh_calls_invalidate_and_refresh(client, mock_album_service, mock_navidrome_service):
|
||||
response = client.post(f"/albums/{VALID_MBID}/refresh")
|
||||
|
||||
assert response.status_code == 200
|
||||
mock_navidrome_service.invalidate_album_cache.assert_called_once_with(VALID_MBID)
|
||||
mock_album_service.refresh_album.assert_called_once_with(VALID_MBID)
|
||||
mock_album_service.get_album_basic_info.assert_called_once_with(VALID_MBID)
|
||||
|
||||
|
||||
def test_refresh_returns_basic_info(client):
|
||||
response = client.post(f"/albums/{VALID_MBID}/refresh")
|
||||
|
||||
assert response.status_code == 200
|
||||
|
||||
|
||||
def test_refresh_rejects_unknown_mbid(client, mock_album_service, mock_navidrome_service):
|
||||
response = client.post("/albums/unknown_test/refresh")
|
||||
|
||||
assert response.status_code == 400
|
||||
assert "Invalid or unknown" in response.json()["detail"]
|
||||
mock_album_service.refresh_album.assert_not_called()
|
||||
mock_navidrome_service.invalidate_album_cache.assert_not_called()
|
||||
|
||||
|
||||
def test_refresh_rejects_empty_album_id(client, mock_album_service):
|
||||
response = client.post("/albums/%20/refresh")
|
||||
|
||||
assert response.status_code == 400
|
||||
mock_album_service.refresh_album.assert_not_called()
|
||||
|
||||
|
||||
def test_refresh_propagates_service_value_error(client, mock_album_service, mock_navidrome_service):
|
||||
mock_navidrome_service.invalidate_album_cache = MagicMock(side_effect=ValueError("bad"))
|
||||
|
||||
response = client.post(f"/albums/{VALID_MBID}/refresh")
|
||||
|
||||
assert response.status_code == 400
|
||||
@@ -0,0 +1,65 @@
|
||||
import pytest
|
||||
import time
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
from services.navidrome_library_service import NavidromeLibraryService
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def service():
|
||||
svc = NavidromeLibraryService.__new__(NavidromeLibraryService)
|
||||
svc._mbid_to_navidrome_id = {}
|
||||
svc._album_mbid_cache = {}
|
||||
svc._dirty = False
|
||||
return svc
|
||||
|
||||
|
||||
ALBUM_MBID = "77434d0b-1c5f-48c3-8694-050cb378ebd2"
|
||||
NAVIDROME_ID = "nav-12345"
|
||||
|
||||
|
||||
def test_invalidate_clears_mbid_to_navidrome_id(service):
|
||||
service._mbid_to_navidrome_id[ALBUM_MBID] = NAVIDROME_ID
|
||||
|
||||
service.invalidate_album_cache(ALBUM_MBID)
|
||||
|
||||
assert ALBUM_MBID not in service._mbid_to_navidrome_id
|
||||
|
||||
|
||||
def test_invalidate_clears_positive_album_mbid_cache_entries(service):
|
||||
cache_key = "test album:test artist"
|
||||
service._album_mbid_cache[cache_key] = ALBUM_MBID
|
||||
|
||||
service.invalidate_album_cache(ALBUM_MBID)
|
||||
|
||||
assert cache_key not in service._album_mbid_cache
|
||||
assert service._dirty is True
|
||||
|
||||
|
||||
def test_invalidate_clears_multiple_matching_entries(service):
|
||||
service._album_mbid_cache["key1:artist1"] = ALBUM_MBID
|
||||
service._album_mbid_cache["key2:artist2"] = ALBUM_MBID
|
||||
service._album_mbid_cache["other:other"] = "different-mbid"
|
||||
|
||||
service.invalidate_album_cache(ALBUM_MBID)
|
||||
|
||||
assert "key1:artist1" not in service._album_mbid_cache
|
||||
assert "key2:artist2" not in service._album_mbid_cache
|
||||
assert "other:other" in service._album_mbid_cache
|
||||
|
||||
|
||||
def test_invalidate_noop_when_not_cached(service):
|
||||
service.invalidate_album_cache(ALBUM_MBID)
|
||||
|
||||
assert service._dirty is False
|
||||
|
||||
|
||||
def test_invalidate_leaves_unrelated_entries(service):
|
||||
other_mbid = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"
|
||||
service._mbid_to_navidrome_id[other_mbid] = "nav-other"
|
||||
service._album_mbid_cache["other:artist"] = other_mbid
|
||||
|
||||
service.invalidate_album_cache(ALBUM_MBID)
|
||||
|
||||
assert service._mbid_to_navidrome_id[other_mbid] == "nav-other"
|
||||
assert service._album_mbid_cache["other:artist"] == other_mbid
|
||||
@@ -11,9 +11,10 @@ def _make_service(queue_add_result: dict | None = None) -> tuple[RequestService,
|
||||
request_queue = MagicMock()
|
||||
request_history = MagicMock()
|
||||
|
||||
request_queue.add = AsyncMock(return_value=queue_add_result or {"message": "queued", "payload": {}})
|
||||
request_queue.enqueue = AsyncMock(return_value=True)
|
||||
request_queue.get_status = MagicMock(return_value={"queue_size": 0, "processing": False})
|
||||
request_history.async_record_request = AsyncMock()
|
||||
request_history.async_get_record = AsyncMock(return_value=None)
|
||||
|
||||
service = RequestService(lidarr_repo, request_queue, request_history)
|
||||
return service, request_queue, request_history
|
||||
@@ -21,37 +22,26 @@ def _make_service(queue_add_result: dict | None = None) -> tuple[RequestService,
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_request_album_records_history_and_returns_response():
|
||||
service, request_queue, request_history = _make_service(
|
||||
{
|
||||
"message": "Album queued",
|
||||
"payload": {
|
||||
"id": 42,
|
||||
"title": "Album X",
|
||||
"foreignAlbumId": "rg-123",
|
||||
"artist": {"artistName": "Artist X", "foreignArtistId": "artist-123"},
|
||||
},
|
||||
}
|
||||
)
|
||||
service, request_queue, request_history = _make_service()
|
||||
|
||||
response = await service.request_album("rg-123", artist="Fallback Artist", album="Fallback Album", year=2024)
|
||||
|
||||
assert response.success is True
|
||||
assert response.message == "Album queued"
|
||||
assert isinstance(response.lidarr_response, dict)
|
||||
assert response.lidarr_response["id"] == 42
|
||||
assert response.message == "Request accepted"
|
||||
assert response.musicbrainz_id == "rg-123"
|
||||
assert response.status == "pending"
|
||||
|
||||
request_queue.add.assert_awaited_once_with("rg-123")
|
||||
request_queue.enqueue.assert_awaited_once_with("rg-123")
|
||||
request_history.async_record_request.assert_awaited_once()
|
||||
kwargs = request_history.async_record_request.await_args.kwargs
|
||||
assert kwargs["artist_name"] == "Artist X"
|
||||
assert kwargs["album_title"] == "Album X"
|
||||
assert kwargs["artist_mbid"] == "artist-123"
|
||||
assert kwargs["artist_name"] == "Fallback Artist"
|
||||
assert kwargs["album_title"] == "Fallback Album"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_request_album_wraps_errors():
|
||||
service, request_queue, _ = _make_service()
|
||||
request_queue.add.side_effect = RuntimeError("boom")
|
||||
request_queue.enqueue = AsyncMock(side_effect=RuntimeError("boom"))
|
||||
|
||||
with pytest.raises(ExternalServiceError):
|
||||
await service.request_album("rg-123")
|
||||
|
||||
@@ -0,0 +1,295 @@
|
||||
"""Tests for MUS-15B: Artist monitoring API and integration."""
|
||||
import pytest
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
from services.artist_service import ArtistService
|
||||
from core.exceptions import ExternalServiceError
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def mock_lidarr_repo():
|
||||
repo = AsyncMock()
|
||||
repo.is_configured = MagicMock(return_value=True)
|
||||
repo.get_artist_details.return_value = {
|
||||
"id": 42,
|
||||
"monitored": False,
|
||||
"monitorNewItems": "none",
|
||||
"monitor_new_items": "none",
|
||||
"name": "Test Artist",
|
||||
"overview": "A test artist",
|
||||
"genres": ["rock"],
|
||||
"links": [],
|
||||
"poster_url": None,
|
||||
"fanart_url": None,
|
||||
"banner_url": None,
|
||||
}
|
||||
repo.update_artist_monitoring.return_value = {"monitored": False, "auto_download": False}
|
||||
repo.get_library_mbids.return_value = set()
|
||||
repo.get_artist_albums.return_value = []
|
||||
repo.get_requested_mbids.return_value = set()
|
||||
return repo
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def mock_mb_repo():
|
||||
repo = AsyncMock()
|
||||
return repo
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def mock_wikidata_repo():
|
||||
repo = AsyncMock()
|
||||
return repo
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def mock_preferences():
|
||||
prefs = MagicMock()
|
||||
prefs.get_preferences.return_value = MagicMock(
|
||||
primary_types=["Album"],
|
||||
secondary_types=[],
|
||||
)
|
||||
return prefs
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def mock_cache():
|
||||
cache = AsyncMock()
|
||||
cache.get.return_value = None
|
||||
cache.delete.return_value = None
|
||||
cache.clear_prefix.return_value = 0
|
||||
return cache
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def mock_disk_cache():
|
||||
return AsyncMock()
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def artist_service(
|
||||
mock_lidarr_repo, mock_mb_repo, mock_wikidata_repo,
|
||||
mock_preferences, mock_cache, mock_disk_cache,
|
||||
):
|
||||
return ArtistService(
|
||||
mb_repo=mock_mb_repo,
|
||||
lidarr_repo=mock_lidarr_repo,
|
||||
wikidata_repo=mock_wikidata_repo,
|
||||
preferences_service=mock_preferences,
|
||||
memory_cache=mock_cache,
|
||||
disk_cache=mock_disk_cache,
|
||||
)
|
||||
|
||||
|
||||
class TestSetArtistMonitoring:
|
||||
@pytest.mark.asyncio
|
||||
async def test_set_monitoring_on(self, artist_service, mock_lidarr_repo, mock_cache):
|
||||
result = await artist_service.set_artist_monitoring(
|
||||
"abc-123", monitored=True, auto_download=False,
|
||||
)
|
||||
mock_lidarr_repo.update_artist_monitoring.assert_awaited_once_with(
|
||||
"abc-123", monitored=True, monitor_new_items="none",
|
||||
)
|
||||
mock_cache.delete.assert_awaited()
|
||||
assert result == {"monitored": False, "auto_download": False}
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_set_monitoring_with_auto_download(self, artist_service, mock_lidarr_repo):
|
||||
await artist_service.set_artist_monitoring(
|
||||
"abc-123", monitored=True, auto_download=True,
|
||||
)
|
||||
mock_lidarr_repo.update_artist_monitoring.assert_awaited_once_with(
|
||||
"abc-123", monitored=True, monitor_new_items="all",
|
||||
)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_auto_download_false_when_unmonitored(self, artist_service, mock_lidarr_repo):
|
||||
await artist_service.set_artist_monitoring(
|
||||
"abc-123", monitored=False, auto_download=True,
|
||||
)
|
||||
mock_lidarr_repo.update_artist_monitoring.assert_awaited_once_with(
|
||||
"abc-123", monitored=False, monitor_new_items="none",
|
||||
)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_raises_when_lidarr_not_configured(self, artist_service, mock_lidarr_repo):
|
||||
mock_lidarr_repo.is_configured = MagicMock(return_value=False)
|
||||
with pytest.raises(ExternalServiceError, match="not configured"):
|
||||
await artist_service.set_artist_monitoring("abc-123", monitored=True)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_invalidates_artist_cache(self, artist_service, mock_cache):
|
||||
await artist_service.set_artist_monitoring("abc-123", monitored=True)
|
||||
delete_calls = [str(c) for c in mock_cache.delete.await_args_list]
|
||||
assert any("artist_info:abc-123" in c for c in delete_calls)
|
||||
|
||||
|
||||
class TestGetArtistMonitoringStatus:
|
||||
@pytest.mark.asyncio
|
||||
async def test_returns_status_from_lidarr(self, artist_service, mock_lidarr_repo):
|
||||
mock_lidarr_repo.get_artist_details.return_value = {
|
||||
"id": 42, "monitored": True, "monitor_new_items": "all",
|
||||
}
|
||||
result = await artist_service.get_artist_monitoring_status("abc-123")
|
||||
assert result == {"in_lidarr": True, "monitored": True, "auto_download": True}
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_returns_defaults_when_not_in_lidarr(self, artist_service, mock_lidarr_repo):
|
||||
mock_lidarr_repo.get_artist_details.return_value = None
|
||||
result = await artist_service.get_artist_monitoring_status("abc-123")
|
||||
assert result == {"in_lidarr": False, "monitored": False, "auto_download": False}
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_returns_defaults_when_lidarr_not_configured(self, artist_service, mock_lidarr_repo):
|
||||
mock_lidarr_repo.is_configured = MagicMock(return_value=False)
|
||||
result = await artist_service.get_artist_monitoring_status("abc-123")
|
||||
assert result == {"in_lidarr": False, "monitored": False, "auto_download": False}
|
||||
|
||||
|
||||
class TestArtistInfoMonitoringFields:
|
||||
@pytest.mark.asyncio
|
||||
async def test_monitoring_fields_set_from_lidarr(self, artist_service, mock_lidarr_repo, mock_cache):
|
||||
mock_lidarr_repo.get_artist_details.return_value = {
|
||||
"id": 42,
|
||||
"monitored": True,
|
||||
"monitor_new_items": "all",
|
||||
"name": "Test Artist",
|
||||
"overview": "A test artist",
|
||||
"genres": ["rock"],
|
||||
"links": [],
|
||||
"poster_url": None,
|
||||
"fanart_url": None,
|
||||
"banner_url": None,
|
||||
}
|
||||
info = await artist_service._do_get_artist_info("abc-123", None, None)
|
||||
assert info.monitored is True
|
||||
assert info.auto_download is True
|
||||
assert info.in_lidarr is True
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_monitoring_fields_default_when_no_lidarr(self, artist_service, mock_lidarr_repo, mock_cache):
|
||||
mock_lidarr_repo.is_configured = MagicMock(return_value=False)
|
||||
mock_lidarr_repo.get_artist_details.return_value = None
|
||||
|
||||
with patch.object(artist_service, '_build_artist_from_musicbrainz') as mock_build:
|
||||
from models.artist import ArtistInfo
|
||||
mock_build.return_value = ArtistInfo(
|
||||
name="Test", musicbrainz_id="abc-123",
|
||||
tags=[], aliases=[], external_links=[],
|
||||
)
|
||||
info = await artist_service._do_get_artist_info("abc-123", None, None)
|
||||
assert info.monitored is False
|
||||
assert info.auto_download is False
|
||||
assert info.in_lidarr is False
|
||||
|
||||
|
||||
class TestUpdateAlbumHelper:
|
||||
"""Tests for Aurral-aligned _update_album helper (PUT /album/{id} instead of PUT /album/monitor)."""
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_album_returns_synchronous_result(self):
|
||||
"""_update_album GETs full album, merges updates, PUTs back, returns updated object."""
|
||||
repo = AsyncMock()
|
||||
from repositories.lidarr.album import LidarrAlbumRepository
|
||||
repo._ALBUM_MUTABLE_FIELDS = LidarrAlbumRepository._ALBUM_MUTABLE_FIELDS
|
||||
original_album = {"id": 10, "title": "Test", "monitored": False, "statistics": {}}
|
||||
updated_album = {"id": 10, "title": "Test", "monitored": True, "statistics": {}}
|
||||
repo._get = AsyncMock(return_value=original_album)
|
||||
repo._put = AsyncMock(return_value=updated_album)
|
||||
|
||||
result = await LidarrAlbumRepository._update_album(repo, 10, {"monitored": True})
|
||||
|
||||
repo._get.assert_awaited_once_with("/api/v1/album/10")
|
||||
repo._put.assert_awaited_once()
|
||||
put_args = repo._put.await_args
|
||||
assert put_args[0][0] == "/api/v1/album/10"
|
||||
assert put_args[0][1]["monitored"] is True
|
||||
assert result["monitored"] is True
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_album_preserves_other_fields(self):
|
||||
"""_update_album only merges specified fields, preserving the rest."""
|
||||
repo = AsyncMock()
|
||||
from repositories.lidarr.album import LidarrAlbumRepository
|
||||
repo._ALBUM_MUTABLE_FIELDS = LidarrAlbumRepository._ALBUM_MUTABLE_FIELDS
|
||||
original = {"id": 10, "title": "Original", "monitored": False, "anyReleaseOk": True}
|
||||
repo._get = AsyncMock(return_value=original.copy())
|
||||
repo._put = AsyncMock(return_value={**original, "monitored": True})
|
||||
|
||||
result = await LidarrAlbumRepository._update_album(repo, 10, {"monitored": True})
|
||||
|
||||
put_payload = repo._put.await_args[0][1]
|
||||
assert put_payload["title"] == "Original"
|
||||
assert put_payload["anyReleaseOk"] is True
|
||||
assert put_payload["monitored"] is True
|
||||
assert result is not None
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_album_rejects_disallowed_fields(self):
|
||||
"""_update_album silently drops fields not in the allowlist."""
|
||||
repo = AsyncMock()
|
||||
from repositories.lidarr.album import LidarrAlbumRepository
|
||||
repo._ALBUM_MUTABLE_FIELDS = LidarrAlbumRepository._ALBUM_MUTABLE_FIELDS
|
||||
original = {"id": 10, "title": "Original", "monitored": False, "rootFolderPath": "/music"}
|
||||
repo._get = AsyncMock(return_value=original.copy())
|
||||
repo._put = AsyncMock(return_value={**original, "monitored": True})
|
||||
|
||||
result = await LidarrAlbumRepository._update_album(
|
||||
repo, 10, {"monitored": True, "rootFolderPath": "/evil", "qualityProfileId": 999}
|
||||
)
|
||||
|
||||
put_payload = repo._put.await_args[0][1]
|
||||
assert put_payload["monitored"] is True
|
||||
assert put_payload["rootFolderPath"] == "/music"
|
||||
assert "qualityProfileId" not in put_payload or put_payload.get("qualityProfileId") != 999
|
||||
assert result is not None
|
||||
|
||||
|
||||
class TestProcessorMonitoringSignal:
|
||||
"""Tests for the queue processor's belt-and-suspenders monitoring check (structured boolean)."""
|
||||
|
||||
def _check_monitored(self, result: dict) -> bool:
|
||||
"""Replicate the processor's monitoring check logic."""
|
||||
payload = result.get("payload", {})
|
||||
is_monitored = payload.get("monitored", False) if isinstance(payload, dict) else False
|
||||
if not is_monitored:
|
||||
is_monitored = bool(result.get("monitored"))
|
||||
return is_monitored
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_processor_trusts_structured_flag_when_payload_stale(self):
|
||||
"""Processor should treat album as monitored via structured boolean, even if payload is stale."""
|
||||
result = {
|
||||
"message": "Album monitored & search triggered: Test Album",
|
||||
"monitored": True,
|
||||
"payload": {"monitored": False, "id": 10},
|
||||
}
|
||||
assert self._check_monitored(result) is True
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_processor_trusts_added_and_monitored_flag(self):
|
||||
"""Processor should detect structured monitored=True for add+monitor path."""
|
||||
result = {
|
||||
"message": "Album added & monitored: New Album",
|
||||
"monitored": True,
|
||||
"payload": {"monitored": False, "id": 20},
|
||||
}
|
||||
assert self._check_monitored(result) is True
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_processor_does_not_false_positive_without_flag(self):
|
||||
"""Processor should NOT flag as monitored when structured boolean is absent."""
|
||||
result = {
|
||||
"message": "Album already downloaded: Some Album",
|
||||
"payload": {"monitored": False, "id": 30},
|
||||
}
|
||||
assert self._check_monitored(result) is False
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_processor_uses_payload_when_already_monitored(self):
|
||||
"""When payload correctly has monitored=True, structured flag is not needed."""
|
||||
result = {
|
||||
"message": "Album already downloaded: Some Album",
|
||||
"payload": {"monitored": True, "id": 40},
|
||||
}
|
||||
assert self._check_monitored(result) is True
|
||||
@@ -0,0 +1,209 @@
|
||||
"""Tests for MUS-15: album status race condition fixes."""
|
||||
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
import asyncio
|
||||
|
||||
import pytest
|
||||
|
||||
|
||||
# ---------- Fix 1: library_service.get_library_mbids merges library_db ----------
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_library_mbids_merges_library_db():
|
||||
"""Library mbids should union Lidarr API results with library_db MBIDs."""
|
||||
from services.library_service import LibraryService
|
||||
|
||||
lidarr_repo = MagicMock()
|
||||
lidarr_repo.is_configured.return_value = True
|
||||
lidarr_repo.get_library_mbids = AsyncMock(return_value={"aaa", "bbb"})
|
||||
|
||||
library_db = MagicMock()
|
||||
library_db.get_all_album_mbids = AsyncMock(return_value={"CCC", "DDD"})
|
||||
|
||||
svc = LibraryService(
|
||||
lidarr_repo=lidarr_repo,
|
||||
library_db=library_db,
|
||||
cover_repo=MagicMock(),
|
||||
preferences_service=MagicMock(),
|
||||
)
|
||||
result = await svc.get_library_mbids()
|
||||
result_set = {m.lower() for m in result}
|
||||
|
||||
assert result_set == {"aaa", "bbb", "ccc", "ddd"}, "Should contain both Lidarr and library_db MBIDs"
|
||||
assert len(result) == 4
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_library_mbids_handles_library_db_overlap():
|
||||
"""Overlapping MBIDs should be deduplicated."""
|
||||
from services.library_service import LibraryService
|
||||
|
||||
lidarr_repo = MagicMock()
|
||||
lidarr_repo.is_configured.return_value = True
|
||||
lidarr_repo.get_library_mbids = AsyncMock(return_value={"aaa", "bbb"})
|
||||
|
||||
library_db = MagicMock()
|
||||
library_db.get_all_album_mbids = AsyncMock(return_value={"AAA", "bbb"})
|
||||
|
||||
svc = LibraryService(
|
||||
lidarr_repo=lidarr_repo,
|
||||
library_db=library_db,
|
||||
cover_repo=MagicMock(),
|
||||
preferences_service=MagicMock(),
|
||||
)
|
||||
result = await svc.get_library_mbids()
|
||||
|
||||
assert len(result) == 2, "Overlapping MBIDs should be deduplicated (case-insensitive)"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_library_mbids_recently_imported_visible():
|
||||
"""Album upserted to library_db (by on_import) appears even when Lidarr cache is stale."""
|
||||
from services.library_service import LibraryService
|
||||
|
||||
# Lidarr API returns old cached data — doesn't include the newly imported album
|
||||
lidarr_repo = MagicMock()
|
||||
lidarr_repo.is_configured.return_value = True
|
||||
lidarr_repo.get_library_mbids = AsyncMock(return_value={"old-album"})
|
||||
|
||||
# library_db has the newly imported album (upserted by on_import callback)
|
||||
library_db = MagicMock()
|
||||
library_db.get_all_album_mbids = AsyncMock(return_value={"old-album", "newly-imported"})
|
||||
|
||||
svc = LibraryService(
|
||||
lidarr_repo=lidarr_repo,
|
||||
library_db=library_db,
|
||||
cover_repo=MagicMock(),
|
||||
preferences_service=MagicMock(),
|
||||
)
|
||||
result = await svc.get_library_mbids()
|
||||
result_set = set(result)
|
||||
|
||||
assert "newly-imported" in result_set, "Recently imported album must appear in library mbids"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_library_mbids_degrades_when_library_db_fails():
|
||||
"""If library_db fails, endpoint should degrade to Lidarr-only MBIDs."""
|
||||
from services.library_service import LibraryService
|
||||
|
||||
lidarr_repo = MagicMock()
|
||||
lidarr_repo.is_configured.return_value = True
|
||||
lidarr_repo.get_library_mbids = AsyncMock(return_value={"aaa", "bbb"})
|
||||
|
||||
library_db = MagicMock()
|
||||
library_db.get_all_album_mbids = AsyncMock(side_effect=RuntimeError("DB locked"))
|
||||
|
||||
svc = LibraryService(
|
||||
lidarr_repo=lidarr_repo,
|
||||
library_db=library_db,
|
||||
cover_repo=MagicMock(),
|
||||
preferences_service=MagicMock(),
|
||||
)
|
||||
result = await svc.get_library_mbids()
|
||||
result_set = set(result)
|
||||
|
||||
assert result_set == {"aaa", "bbb"}, "Should fall back to Lidarr-only when library_db fails"
|
||||
|
||||
|
||||
# ---------- Fix 2: queue worker fires import callback ----------
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_queue_worker_fires_import_callback_when_has_files():
|
||||
"""When the queue worker detects has_files, the import callback should fire."""
|
||||
from infrastructure.queue.request_queue import RequestQueue
|
||||
from infrastructure.persistence.request_history import RequestHistoryRecord
|
||||
|
||||
callback_called = asyncio.Event()
|
||||
callback_record = {}
|
||||
|
||||
async def on_import(record):
|
||||
callback_record["mbid"] = record.musicbrainz_id
|
||||
callback_record["artist_mbid"] = record.artist_mbid
|
||||
callback_called.set()
|
||||
|
||||
history = MagicMock()
|
||||
# First call: cancel check (returns original pending record)
|
||||
# Second call: enriched record for callback (after field updates)
|
||||
original_record = RequestHistoryRecord(
|
||||
musicbrainz_id="test-mbid",
|
||||
artist_name="Test Artist",
|
||||
album_title="Test Album",
|
||||
requested_at="2026-01-01T00:00:00Z",
|
||||
status="pending",
|
||||
)
|
||||
enriched_record = RequestHistoryRecord(
|
||||
musicbrainz_id="test-mbid",
|
||||
artist_name="Test Artist",
|
||||
album_title="Test Album",
|
||||
requested_at="2026-01-01T00:00:00Z",
|
||||
status="imported",
|
||||
artist_mbid="artist-mbid",
|
||||
)
|
||||
history.async_get_record = AsyncMock(side_effect=[original_record, enriched_record])
|
||||
history.async_update_status = AsyncMock()
|
||||
history.async_update_lidarr_album_id = AsyncMock()
|
||||
history.async_update_cover_url = AsyncMock()
|
||||
history.async_update_artist_mbid = AsyncMock()
|
||||
|
||||
q = RequestQueue(
|
||||
processor=AsyncMock(),
|
||||
request_history=history,
|
||||
on_import_callback=on_import,
|
||||
)
|
||||
|
||||
result = {
|
||||
"payload": {
|
||||
"id": 42,
|
||||
"statistics": {"trackFileCount": 5},
|
||||
"artist": {"foreignArtistId": "artist-mbid"},
|
||||
}
|
||||
}
|
||||
await q._update_history_on_result("test-mbid", result)
|
||||
|
||||
assert callback_called.is_set(), "Import callback should have been called"
|
||||
assert callback_record["mbid"] == "test-mbid"
|
||||
assert callback_record["artist_mbid"] == "artist-mbid", "Callback should receive enriched record"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_queue_worker_no_callback_when_downloading():
|
||||
"""When the queue worker detects no files, import callback should NOT fire."""
|
||||
from infrastructure.queue.request_queue import RequestQueue
|
||||
from infrastructure.persistence.request_history import RequestHistoryRecord
|
||||
|
||||
callback_called = asyncio.Event()
|
||||
|
||||
async def on_import(record):
|
||||
callback_called.set()
|
||||
|
||||
history = MagicMock()
|
||||
original_record = RequestHistoryRecord(
|
||||
musicbrainz_id="test-mbid",
|
||||
artist_name="Test Artist",
|
||||
album_title="Test Album",
|
||||
requested_at="2026-01-01T00:00:00Z",
|
||||
status="pending",
|
||||
)
|
||||
history.async_get_record = AsyncMock(return_value=original_record)
|
||||
history.async_update_status = AsyncMock()
|
||||
history.async_update_lidarr_album_id = AsyncMock()
|
||||
history.async_update_cover_url = AsyncMock()
|
||||
history.async_update_artist_mbid = AsyncMock()
|
||||
|
||||
q = RequestQueue(
|
||||
processor=AsyncMock(),
|
||||
request_history=history,
|
||||
on_import_callback=on_import,
|
||||
)
|
||||
|
||||
result = {
|
||||
"payload": {
|
||||
"id": 42,
|
||||
"statistics": {"trackFileCount": 0},
|
||||
"artist": {"foreignArtistId": "artist-mbid"},
|
||||
}
|
||||
}
|
||||
await q._update_history_on_result("test-mbid", result)
|
||||
|
||||
assert not callback_called.is_set(), "Import callback should NOT fire when trackFileCount=0"
|
||||
Reference in New Issue
Block a user