diff --git a/docs/download-engine-refactor-plan.md b/docs/download-engine-refactor-plan.md new file mode 100644 index 00000000..618adccf --- /dev/null +++ b/docs/download-engine-refactor-plan.md @@ -0,0 +1,202 @@ +# Download Engine Refactor Plan + +## Goal + +Mirror Cin's "metadata engine" architecture for the download dispatcher. Move shared logic OUT of the per-source clients (currently 1600+ LOC of duplicated thread workers, search retry ladders, rate-limiters, state machines) and INTO a central `DownloadEngine`. Clients become dumb: make raw API requests + manage their own auth state. Everything else is the engine. + +This is the SAME architectural smell Cin flagged on the metadata layer, applied to downloads. If we keep adding sources (usenet planned + likely more), the only honest fix is to stop reinventing the wheel per client. + +## Architecture target + +``` +┌─────────────┐ ┌──────────────────┐ +│ feature │ ── search/download ──▶ ┌────────────────────┐ ─▶│ Soulseek (raw) │ +│ │ ◀── normalized ──────── │ DownloadEngine │ ─▶│ YouTube (raw) │ +└─────────────┘ │ │ ─▶│ Tidal (raw) │ + │ ◆ thread workers │ ─▶│ Qobuz (raw) │ + │ ◆ rate-limit pool │ ─▶│ HiFi (raw) │ + │ ◆ search retry │ ─▶│ Deezer (raw) │ + │ ◆ quality filter │ ─▶│ SoundCloud (raw) │ + │ ◆ state tracking │ ─▶│ Lidarr (album) │ + │ ◆ fallback chain │ └──────────────────┘ + │ ◆ cache │ clients only do: + └────────────────────┘ - raw API request + - auth/token state +``` + +## What clients keep (legitimately per-source) + +- Auth flow + token refresh (Tidal OAuth, Qobuz session, Deezer ARL, slskd API key, etc.) +- Source-specific protocol (slskd events vs HTTP REST vs HLS demux vs Blowfish decrypt vs yt-dlp subprocess) +- Source-specific search query shape (free text vs keyword filters vs MusicBrainz ID lookup) +- Source-specific "download a thing" atomic operation (`_download_impl(target_id) → file_path`) + +## What moves into the engine + +| Today (per-client, duplicated) | Tomorrow (engine, single source of truth) | +|---|---| +| `self.active_downloads = {}` per client | `engine.active_downloads = {}` | +| `self._download_lock = Lock()` per client | `engine.state_lock = Lock()` | +| `self._download_semaphore = Semaphore(...)` per client | `engine.download_pool` (per-source semaphore from registry) | +| `self._last_download_time / _download_delay` per client | `engine.rate_limiter.acquire(source)` | +| `_download_thread_worker` × 7 (~70 LOC each) | `engine.dispatch_download(plugin, target_id)` | +| Search retry ladder × 7 | `engine.search(query)` with shared retry policy | +| Quality filter × 7 | `engine.filter_by_quality(results, prefs)` | +| Result dedup × 7 | `engine.dedup(results)` | +| Hybrid fallback (search only) | `engine.fallback_chain(operation)` (search AND download) | + +## New plugin contract (much smaller) + +```python +class DownloadSourcePlugin(Protocol): + # Identity + name: str + + # Lifecycle + def is_configured(self) -> bool: ... + async def check_connection(self) -> bool: ... + def reload_settings(self) -> None: ... + + # Search — DUMB. Just hit the API. + async def search_raw(self, query: str) -> List[RawSearchResult]: ... + + # Download — DUMB. Just download the bytes to a file. + # The engine handles thread spawning, state tracking, rate limits. + # Plugin returns the final file path on success or raises. + async def download_raw(self, target_id: str, dest_dir: Path) -> Path: ... + + # Cancel — best-effort. Engine handles state cleanup. + async def cancel_raw(self, target_id: str) -> bool: ... +``` + +Compare to today's plugin protocol (which my Phase 0 PR introduced) — that one was wrapped around fat clients. This one is dumber. Clients shrink dramatically (estimated 40-60% LOC reduction per file). + +## Special cases + +- **Soulseek** — slskd is event-driven, NOT thread-based. Engine's BackgroundDownloadWorker doesn't apply. Keep Soulseek's path special: `download_raw` returns immediately; engine subscribes to slskd events for state updates instead of running a thread. +- **YouTube/SoundCloud** — yt-dlp is a subprocess. The "thread" is really `subprocess.run(['yt-dlp', ...]).wait()`. Engine handles thread; plugin's `download_raw` just runs subprocess and returns file path. +- **Lidarr** — album-grabber, not track-grabber. Different contract. Either separate `AlbumOnlyPlugin` interface OR plugin declares `supports_track_search: bool = False`. Decide during migration. + +## Phased commit plan + +Each phase is one or more commits. Each commit independently revertable. Tests stay green between commits — never ship a half-broken state. + +### Phase A — Behavior pinning tests (BEFORE any code moves) + +**Goal:** Baseline tests for what each source's download path currently does. Catches regressions during extraction. + +**Commit A1:** `tests/downloads/test_soulseek_download_path.py` — pin Soulseek's download lifecycle (search → download → completion → file path returned). +**Commit A2:** Same for YouTube. Mock yt-dlp subprocess. +**Commit A3:** Same for Tidal. Mock tidalapi.Session. +**Commit A4:** Same for Qobuz. Mock Qobuz REST API. +**Commit A5:** Same for HiFi. Mock hifi-api instance. +**Commit A6:** Same for Deezer. Mock Deezer GW API + Blowfish stream. +**Commit A7:** Same for SoundCloud. Mock yt-dlp scsearch. +**Commit A8:** Same for Lidarr. Mock Lidarr REST API. + +After Phase A: ~50 new tests pinning current behavior. We can refactor with confidence. + +### Phase B — Engine skeleton + state lift + +**Commit B1:** Create `core/download_engine/` package with `DownloadEngine` class. Engine starts EMPTY — just exposes `register_plugin(plugin)`, `active_downloads` dict, `state_lock`. Orchestrator gets a `self.engine` reference but doesn't use it yet. + +**Commit B2:** Move `active_downloads` state out of every client into `engine.active_downloads`. Each client's `download()` now updates engine state via callback instead of `self.active_downloads[id] = ...`. Backward compat: each client's `self.active_downloads` becomes a property that delegates to `engine.active_downloads.filter(source=self.name)`. + +**Commit B3:** Move `get_all_downloads` / `get_download_status` / `cancel_download` dispatch from orchestrator (which iterates plugins) into engine (which queries unified state). Orchestrator's methods become thin pass-throughs. + +### Phase C — Background download worker lift + +**Commit C1:** New `core/download_engine/worker.py` — `BackgroundDownloadWorker` class. Owns semaphore, rate-limit sleep, state-update lock pattern. Provides `dispatch(plugin, target_id, display_name) → download_id`. + +**Commit C2:** Migrate YouTube to use BackgroundDownloadWorker. Strip `_download_thread_worker` from `youtube_client.py`. Add `download_raw(video_id, dest) → Path`. Tests stay green (Phase A pinned them). + +**Commit C3:** Same for Tidal. +**Commit C4:** Same for Qobuz. +**Commit C5:** Same for HiFi. +**Commit C6:** Same for Deezer. +**Commit C7:** Same for SoundCloud. + +After Phase C: ~490 LOC of duplicated thread management deleted. Each affected client shrinks. + +### Phase D — Search retry + quality filter lift + +**Commit D1:** New `core/download_engine/search.py` — `SearchOrchestrator`. Owns: query normalization, shortened-query retry ladder, quality filter, dedup. Calls `plugin.search_raw(query)` for the actual API hit. + +**Commit D2:** Migrate Tidal's search. Strip `_generate_shortened_queries`, quality filter, dedup from client. Add `search_raw(query) → List[RawResult]`. +**Commit D3:** Same for Qobuz. +**Commit D4:** Same for HiFi. +**Commit D5:** Same for YouTube. +**Commit D6:** Same for Deezer. +**Commit D7:** Same for SoundCloud. +**Commit D8:** Same for Soulseek (keep slskd event-driven specifics, but the post-search filter/dedup moves out). +**Commit D9:** Same for Lidarr. + +### Phase E — Rate-limit pool + +**Commit E1:** New `core/download_engine/rate_limit.py` — per-source rate limiter registry. Spotify limit, Qobuz 1/sec, etc. Each plugin declares its limits in its registry spec. +**Commit E2:** Strip per-client rate-limit state. Replace with `await engine.rate_limit.acquire(self.name)` at the top of `search_raw` / `download_raw`. + +### Phase F — Fallback chain into engine + +**Commit F1:** Engine owns fallback: `engine.search_with_fallback(query, source_chain)` and `engine.download_with_fallback(target_id, source_chain)`. Search hybrid behavior preserved; download hybrid newly works (today it silently routes to one source). +**Commit F2:** Orchestrator's `search` and `download` methods delegate to engine's fallback methods. Hybrid mode logic moves out of orchestrator. + +### Phase G — Plugin contract narrows + +**Commit G1:** Update `DownloadSourcePlugin` Protocol — narrow to the small surface (`search_raw`, `download_raw`, `cancel_raw`, `is_configured`, `check_connection`, `reload_settings`). Conformance tests updated. +**Commit G2:** Remove dead methods from clients that the engine now owns (`_download_thread_worker`, `_filter_results_by_quality`, etc.). Clean up imports. + +### Phase H — Cleanup + WHATS_NEW + version bump + +**Commit H1:** Final cleanup pass — remove backward-compat shims that are no longer needed (legacy `self.active_downloads` properties etc., once nothing reaches in for them). +**Commit H2:** WHATS_NEW entry, PR description. + +## Total estimated scope + +- ~25-30 commits +- ~2000 LOC removed (duplicated thread workers, search retries, etc.) +- ~1200 LOC added (engine + per-source slim adapters) +- Net reduction: ~800 LOC +- ~50 new tests (Phase A pinning) + ~20 engine-level tests +- 1-2 days of focused work + +## Risk profile + +**Low risk:** +- Phase A (only adds tests, never changes behavior) +- Phase B1 (new file, doesn't touch existing code) +- Phase H (cleanup of dead code) + +**Medium risk:** +- Phase B2-B3 (state lift — race conditions, lock contention) +- Phase C (thread worker extraction — semaphore semantics, exception propagation) +- Phase G (contract narrows — anything reaching in for removed methods breaks) + +**High risk:** +- Phase D (search retry — easy to subtly change retry ladder shape) +- Phase E (rate-limit — wrong order can cause deadlocks or under-limit violations) +- Phase F (fallback — easy to accidentally change hybrid mode behavior) + +**Mitigation:** Phase A pinning tests catch behavior drift in every later phase. Each commit must pass full suite. Manual smoke test per source after Phase C and again at end. + +## Coordination with Cin + +- Cin's metadata engine PR will likely set the precedent for HOW abstractions look (Protocol vs ABC, sync vs async, state location). This plan defaults to Protocol + async (matches what we already have) but easy to mirror Cin's exact pattern when his PR lands. +- If his pattern differs significantly, we may need to redo some commits. Best mitigation: don't dig too deep on contract shape (Phase G) until his PR is visible. Phases A-C don't depend on contract shape; they're safe to do regardless. +- Send Cin a heads-up before starting — he may have feedback on the plan that saves a redesign later. + +## Compatibility commitments + +- Backward-compat `orchestrator.soulseek` / `orchestrator.youtube` / etc. attributes preserved through every phase. +- Backward-compat `clear_all_searches`, `_make_request`, `signal_download_completion`, etc. (Soulseek-specific reaches) preserved through every phase. +- Frontend status dashboard keys (`deezer_dl` alias) preserved. +- Config format unchanged. +- DB schema unchanged. +- API endpoint surface unchanged. + +## What's NOT in this PR + +- Cin's metadata engine work (separate, his domain) +- Media server client refactor (different subsystem, separate PR) +- Match engine refactor (different subsystem, separate PR) +- Adding new download sources (out of scope; the new contract makes them easier later) diff --git a/tests/downloads/test_soulseek_pinning.py b/tests/downloads/test_soulseek_pinning.py new file mode 100644 index 00000000..73cf6f6c --- /dev/null +++ b/tests/downloads/test_soulseek_pinning.py @@ -0,0 +1,258 @@ +"""Phase A pinning tests for SoulseekClient's download lifecycle. + +These tests pin the OBSERVABLE BEHAVIOR of `SoulseekClient.download` / +`get_all_downloads` / `cancel_download` so the upcoming download +engine refactor (which lifts shared state + thread workers + search +retry into a central engine) can't drift the per-source contract. + +The contract these tests pin is what the engine will call into via +`plugin.download_raw(target_id)` / `plugin.cancel_raw(target_id)` +after the refactor lands. If a future commit breaks any of these +expectations, the diff fails fast — long before a real download +attempt against a live slskd would have surfaced the bug. + +NOTE: Soulseek is structurally different from the streaming sources. +It has NO local thread worker — slskd manages downloads server-side +and the client just polls for state. So Soulseek skips most of the +engine refactor's thread-extraction work; what stays critical is +the slskd HTTP API contract (endpoints, payload shape, id +extraction). That's what these tests pin. +""" + +from __future__ import annotations + +import asyncio +from pathlib import Path +from unittest.mock import AsyncMock, patch + +import pytest + +from core.soulseek_client import SoulseekClient + + +def _run_async(coro): + loop = asyncio.new_event_loop() + try: + return loop.run_until_complete(coro) + finally: + loop.close() + + +# --------------------------------------------------------------------------- +# Configuration / lifecycle +# --------------------------------------------------------------------------- + + +def test_is_configured_returns_false_when_no_base_url(): + """Pinning: an unconfigured client (no slskd URL set) reports + is_configured() == False. The orchestrator's hybrid fallback + + every consumer that gates on is_configured() depends on this.""" + client = SoulseekClient.__new__(SoulseekClient) + client.base_url = None + client.api_key = None + assert client.is_configured() is False + + +def test_is_configured_returns_true_when_base_url_set(): + """Pinning: configured client (slskd URL present) reports True.""" + client = SoulseekClient.__new__(SoulseekClient) + client.base_url = 'http://localhost:5030' + client.api_key = 'test-key' + assert client.is_configured() is True + + +# --------------------------------------------------------------------------- +# download() +# --------------------------------------------------------------------------- + + +@pytest.fixture +def configured_client(): + """A SoulseekClient with the slskd URL set but no real network. Tests + individually patch `_make_request` to return whatever shape they + want to exercise.""" + client = SoulseekClient.__new__(SoulseekClient) + client.base_url = 'http://localhost:5030' + client.api_key = 'test-key' + client.download_path = Path('./test_downloads') + return client + + +def test_download_returns_none_when_not_configured(): + """Pinning: an unconfigured client refuses downloads — returns + None silently rather than raising. Used as the soft-fail signal + by the orchestrator's per-source fallback chain.""" + client = SoulseekClient.__new__(SoulseekClient) + client.base_url = None + result = _run_async(client.download('user', 'song.flac', 1024)) + assert result is None + + +def test_download_hits_transfers_downloads_username_endpoint(configured_client): + """Pinning: the primary download endpoint is + `transfers/downloads/` POST. This shape was chosen to + match slskd's web-interface API exactly. Changing it breaks + every download against current slskd builds.""" + captured = [] + + async def fake_request(method, endpoint, json=None, **kwargs): + captured.append((method, endpoint, json)) + return {'id': 'dl-id-from-slskd'} + + with patch.object(configured_client, '_make_request', side_effect=fake_request): + result = _run_async(configured_client.download('user', 'song.flac', 1024)) + + assert result == 'dl-id-from-slskd' + method, endpoint, payload = captured[0] + assert method == 'POST' + assert endpoint == 'transfers/downloads/user' + # Payload is the slskd web-interface array format. + assert isinstance(payload, list) + assert payload[0]['filename'] == 'song.flac' + assert payload[0]['size'] == 1024 + + +def test_download_extracts_id_from_dict_response(configured_client): + """Pinning: when slskd returns `{id: ...}`, that's the + download_id the orchestrator uses to track the download.""" + with patch.object(configured_client, '_make_request', + AsyncMock(return_value={'id': 'abc123'})): + result = _run_async(configured_client.download('user', 'song.flac', 1024)) + assert result == 'abc123' + + +def test_download_extracts_id_from_list_response(configured_client): + """Pinning: slskd sometimes returns a list of file objects. + The first item's id is the download_id.""" + with patch.object(configured_client, '_make_request', + AsyncMock(return_value=[{'id': 'list-id'}, {'id': 'second'}])): + result = _run_async(configured_client.download('user', 'song.flac', 1024)) + assert result == 'list-id' + + +def test_download_falls_back_to_filename_when_no_id_in_response(configured_client): + """Pinning: defensive — older slskd builds returned 201 Created + with no id field. The client uses the filename as the download + identifier in that case so downstream tracking still works.""" + with patch.object(configured_client, '_make_request', + AsyncMock(return_value={'status': 'queued'})): + result = _run_async(configured_client.download('user', 'song.flac', 1024)) + assert result == 'song.flac' + + +# --------------------------------------------------------------------------- +# get_all_downloads() +# --------------------------------------------------------------------------- + + +def test_get_all_downloads_returns_empty_when_not_configured(): + client = SoulseekClient.__new__(SoulseekClient) + client.base_url = None + result = _run_async(client.get_all_downloads()) + assert result == [] + + +def test_get_all_downloads_parses_nested_user_directory_files_response(configured_client): + """Pinning: slskd's `transfers/downloads` returns + `[{username, directories: [{files: [...]}]}]`. The client + flattens that into a list of DownloadStatus objects, one per + file. Engine refactor's state aggregation depends on this shape.""" + fake_response = [ + { + 'username': 'peer1', + 'directories': [{ + 'files': [ + {'id': 'f1', 'filename': 'a.flac', 'state': 'InProgress', + 'size': 100, 'bytesTransferred': 50, 'averageSpeed': 1024}, + {'id': 'f2', 'filename': 'b.flac', 'state': 'Completed, Succeeded', + 'size': 200, 'bytesTransferred': 200, 'averageSpeed': 2048}, + ], + }], + }, + ] + + with patch.object(configured_client, '_make_request', + AsyncMock(return_value=fake_response)): + result = _run_async(configured_client.get_all_downloads()) + + assert len(result) == 2 + assert result[0].id == 'f1' + assert result[0].username == 'peer1' + assert result[0].state == 'InProgress' + assert result[1].id == 'f2' + # Pinning: 'Completed' state forces progress=100 regardless of source data. + assert result[1].progress == 100.0 + + +def test_get_all_downloads_endpoint_is_transfers_downloads(configured_client): + """Pinning: the listing endpoint is `transfers/downloads` (no + username). The 404'd `users/.../downloads` variant was tried + once and removed — keep it gone.""" + captured = [] + + async def fake_request(method, endpoint, **kwargs): + captured.append((method, endpoint)) + return [] + + with patch.object(configured_client, '_make_request', side_effect=fake_request): + _run_async(configured_client.get_all_downloads()) + + assert captured == [('GET', 'transfers/downloads')] + + +# --------------------------------------------------------------------------- +# cancel_download() +# --------------------------------------------------------------------------- + + +def test_cancel_download_returns_false_when_not_configured(): + client = SoulseekClient.__new__(SoulseekClient) + client.base_url = None + result = _run_async(client.cancel_download('dl-id', 'user', remove=False)) + assert result is False + + +def test_cancel_download_looks_up_username_when_not_provided(configured_client): + """Pinning: orchestrator may call cancel_download without a + username hint. The client falls back to scanning all downloads + to find which peer owns it. Engine refactor must preserve this + so existing API endpoints that don't pass username keep working.""" + fake_listing = [ + { + 'username': 'peer-owner', + 'directories': [{ + 'files': [{'id': 'target-dl', 'filename': 'x.flac', + 'state': 'InProgress', 'size': 0, + 'bytesTransferred': 0, 'averageSpeed': 0}], + }], + }, + ] + + captured_endpoints = [] + + async def fake_request(method, endpoint, **kwargs): + captured_endpoints.append((method, endpoint)) + if method == 'GET' and endpoint == 'transfers/downloads': + return fake_listing + # The DELETE for cancel — return success + return True + + with patch.object(configured_client, '_make_request', side_effect=fake_request): + _run_async(configured_client.cancel_download('target-dl', None, remove=False)) + + # The lookup hit get_all_downloads first, then the DELETE used the discovered username. + assert ('GET', 'transfers/downloads') in captured_endpoints + delete_calls = [(m, e) for m, e in captured_endpoints if m == 'DELETE'] + assert delete_calls, "Expected at least one DELETE after username lookup" + # The cancel endpoint URL contains the discovered username. + assert any('peer-owner' in e for _, e in delete_calls) + + +def test_cancel_download_returns_false_when_username_lookup_fails(configured_client): + """Pinning: if the download_id isn't in the active list, return + False rather than raising. Orchestrator treats False as "couldn't + cancel" and continues; an exception would propagate to the user.""" + with patch.object(configured_client, '_make_request', + AsyncMock(return_value=[])): + result = _run_async(configured_client.cancel_download('missing-id', None)) + assert result is False