268 Commits (2.4.2)

Author	SHA1	Message	Date
Broque Thomas	1a2da016e4	Add download buttons + bulk action to artist top-tracks sidebar ... Closes #513 (s66jones). The artist detail page already showed a "Popular on Last.fm" sidebar — list of an artist's top tracks by playcount, with a play button per row but no download action. Issue #513 wanted a way to grab those tracks the same way zotify let users grab "top X songs" without pulling the full discography. Pulls from the configured primary metadata source (Spotify `artist_top_tracks`, Deezer `/artist/{id}/top`) when available, falls back to the existing Last.fm display-only mode for sources that don't expose popularity ranking (iTunes / Discogs / MusicBrainz). Source label in the section title shifts to match. Each row gets a hover-revealed download button that wishlists the single track via the existing /api/add-album-to-wishlist endpoint (preserves the track's real album metadata, so the wishlist worker later places the file in its proper album folder). A "Download All" footer button opens the standard download modal in PLAYLIST context, not album context — the virtual playlist_id is `top_tracks_<source>_<artistId>` which doesn't match any of the album-prefix checks in `startMissingTracksProcess` (downloads.js). That keeps `is_album_download=false`, so the master worker doesn't inject a wrapper context as `_explicit_album_context`. Each track downloads using its own real album metadata, files land in proper per-album folders on disk (not a fake "Top Tracks" folder). Backend additions: - `SpotifyClient.get_artist_top_tracks(artist_id, country, limit)` — wraps `spotipy.artist_top_tracks`, returns up to 10 tracks for the market (Spotify's API cap). UI-side limit trim only. - `DeezerClient.get_artist_top_tracks(artist_id, limit)` — wraps `/artist/{id}/top?limit=N`, converts Deezer's raw shape to the same Spotify-compatible dict layout (id, name, artists, album with album_type / total_tracks / images, duration_ms, track_number, disc_number) so downstream code doesn't branch on source. - `GET /api/artist/<id>/top-tracks` — dispatches to whichever client matches the primary source. Resolves per-source artist IDs from the DB row first (matching what /discography already does) so a Spotify ID in the URL still works when Deezer is primary, and vice versa. Returns `{success, source, tracks, resolved_artist_id}` on hit; `{success: False, reason: 'unsupported_source' | 'spotify_not_authenticated' | 'deezer_unavailable' | 'no_tracks_found'}` on miss so the frontend can decide whether to fall through to Last.fm. Frontend: - `_loadArtistTopTracks` tries the metadata source first, falls through to the legacy `/api/artist/0/lastfm-top-tracks` call if the source can't deliver. Section title and per-row UI shift based on which source answered. - New per-row `.hero-top-track-download` button (hover-revealed). - New `.hero-top-tracks-download-all` footer button — only visible when metadata-source mode rendered the list (Last.fm fallback hides it since rows have no track IDs to download). Tests: 10 new tests pin the client methods — - Spotify: returns track list, honors UI limit cap, returns empty when unauthed / artist_id missing / API throws. - Deezer: shape conversion to Spotify-compatible dict, empty when no data / artist_id missing, limit clamping at upper bound, default fallback when limit=0, malformed entries skipped. The Flask endpoint dispatcher itself isn't covered by the new test file because importing web_server at test-collection time spins up worker threads that race with caplog-using tests elsewhere in the suite (specifically test_library_reorganize_orchestrator). Endpoint verified manually; the underlying client methods (the load-bearing logic) are covered. 2204/2204 full suite green (was 2194 + 10 new).	2 weeks ago
Broque Thomas	01c528fd5f	Reject AcoustID matches whose version disagrees with the expected track ... Discord report (corruption [BWC]): downloads coming through as the instrumental cut when a vocal track was requested. The verification step's `_normalize` function strips parentheticals and version-suffix tags ("(Instrumental)", "- Live", etc) so legitimate name variations don't false-fail the title-similarity check. That also means "In My Feelings" and "In My Feelings (Instrumental)" both normalize to "in my feelings", title similarity is 1.0, and the wrong cut passes verification. Detect the version label on each side BEFORE normalization runs. If the expected and matched recordings disagree on version (one is original, the other is instrumental / live / acoustic / remix / etc), return FAIL — the fingerprint identified a real song, just not the version the caller asked for. Reuses `MusicMatchingEngine.detect_version_type` so the same regex patterns the pre-download Soulseek matcher applies also drive post-download verification. No duplicated tables. Also gates the secondary fallback scan, so a wrong-version variant sitting in the same fingerprint cluster can't win the loop after the best match has already been version-rejected. 6 tests pin the behavior: - instrumental returned for vocal request → FAIL - vocal returned for instrumental request → FAIL - live vs acoustic → FAIL - matching versions on both sides → PASS - original-to-original happy path → PASS (regression guard) - secondary scan skips wrong-version recordings → not PASS 2194/2194 full suite green (was 2188 + 6 new).	3 weeks ago
Broque Thomas	4c11375930	Repair job card badge — show pending count, not last-scan count ... Discord report: Duplicate Detector card said "372 findings" and Cover Art Filler said "60 findings", but clicking the Findings tab's Pending filter showed 0. User read it as "findings aren't being created" — looked like a detector bug. Actual cause: the badge sourced ``last_run.findings_created`` (historical "found in last scan") without considering current state. After the user (or bulk-fix automation) resolved or dismissed those findings, they no longer appeared on the Pending tab — but the badge kept showing the last-scan number in red urgent styling. Backend was correct end-to-end: detectors create pending rows, bulk-fix moves them to resolved, Findings tab filters by status. Only the badge display lied about current state. Fix: - ``RepairWorker._get_pending_count_by_job()`` — single SQL aggregation returning ``{job_id: pending_count}`` for every job with pending findings. O(1) lookup per job instead of N round trips. - ``get_all_job_info()`` calls it once per request and adds ``pending_findings_count`` to each job's API response. - ``enrichment.js`` job card now branches on the count: - ``> 0`` → red ``"X pending"`` badge (urgent, action needed) - ``= 0`` AND last scan found something → muted grey ``"X found in last scan"`` (historical context, no action needed) - New CSS class ``.repair-flow-badge.findings-historical`` for the muted slate color so the two states are visually distinct. User-visible result with the screenshotted state (372 dup / 60 cover- art findings, all resolved): - Before: red "372 findings" / "60 findings" — implied 432 things to do, but Findings tab showed 0 pending - After: grey "372 found in last scan" / "60 found in last scan" — the badge text tells the user the count is historical, no surprise when Pending is empty Tests: 3 new tests in ``tests/test_create_finding_dedup_counter.py`` pin the per-job pending count helper: - returns ``{job_id: count}`` based on status='pending' rows only; resolved + dismissed rows excluded - empty dict when no pending findings exist - gracefully returns ``{}`` on DB error (badge falls back to historical count via the existing JS ``or 0`` safety) 2188/2188 full suite green. Pure UI/state-display fix — no detector logic, no backend behavior change.	3 weeks ago
Broque Thomas	5c69b853b4	Bound slskd HTTP timeout — fixes worker thread deadlock ... GitHub issue #499 (@bafoed). Big initial sync of Spotify playlists worked for 2-3 hours then downloads silently stopped: - 3 active tasks stuck in "Searching" state, replaced every ~10 min by different ones - slskd UI showed no actual searches happening - Debug log: orphaned-task count grew over time, no jobs executed - Container restart was the only fix (bought another 2-3 hours) - Not a rate limit (rates showed 0/min) Root cause: ``core/soulseek_client.py`` constructed ``aiohttp.ClientSession()`` with no timeout at four sites. When slskd hung on a request (overloaded, transient network blip, internal stall), the HTTP call blocked indefinitely — and the worker thread blocked with it. The download executor only has ``ThreadPoolExecutor(max_workers=3)``, so once 3 worker threads were wedged on hung calls, no further downloads could start. Batch-level "stuck detection" (10-minute timer in ``check_batch_completion_v2``) was correctly marking tasks ``not_found`` and trying to start replacements, but the executor pool was exhausted — replacements queued forever inside the executor with no thread to run them. Symptom: tasks rotating every ~10 min at the batch level while the underlying executor stayed wedged. Fix: bounded ``aiohttp.ClientTimeout`` (total 120s, connect 15s, sock_read 60s) on every slskd ``ClientSession`` construction. Module- level constant ``_SLSKD_DEFAULT_TIMEOUT`` so the four sites stay in lockstep — future sites get the same protection by reusing the constant. Why these timeouts are safe: - Every slskd API call is metadata-level (search submission, status polls, download enqueue, transfer state queries). None stream files — slskd handles file transfer via its own peer-to-peer infrastructure entirely outside our HTTP requests. - Legitimate metadata calls finish in seconds. 120s ceiling is ~50× the normal latency. Timeout handling: - ``asyncio.TimeoutError`` caught explicitly BEFORE the generic ``except Exception`` — surfaces "slskd timed out" specifically in logs (debuggable instead of buried as "Error making API request"). - Returns None to the caller (same code path as a 5xx response or any other failure). No new error path; callers already handle None as "request failed". - Worker thread unblocks immediately → executor pool stays healthy → downloads keep flowing. Sites updated: - ``_make_request`` (general /api/v0/ helper, line 152) — used for every slskd API operation - ``_make_direct_request`` (non-/api/v0/ helper, line 235) - ``_explore_api_endpoints`` Swagger fetch (line 1566) — diagnostic - ``_explore_api_endpoints`` per-endpoint probe (line 1617) — diagnostic Tests: 3 new tests in ``tests/downloads/test_soulseek_pinning.py`` pin: - ``_SLSKD_DEFAULT_TIMEOUT`` is bounded (total set, ≤300s ceiling, connect ≤60s) — guards against future regressions that drop or unbound the timeout - ``_make_request`` returns None on ``asyncio.TimeoutError`` rather than raising — pins the caller contract - ``_make_direct_request`` returns None on ``asyncio.TimeoutError`` 2185/2185 full suite green. Closes #499.	3 weeks ago
Broque Thomas	ca5c93162c	Rewrite Library Reorganize job to delegate to per-album planner ... GitHub issue #500 (@bafoed). Library Reorganize repair job moved album tracks to single-template paths because of a fragile classification heuristic. Concrete symptom: a track at ``Surf Curse/Surf Curse - Nothing Yet (2017)/01 - Christine F.flac`` got proposed for a move to ``Surf Curse/Surf Curse - Christine F/Surf Curse - Christine F.flac`` (single template) instead of staying under the album folder. Root cause: the job had its own tag-reading + transfer-folder-walk + template-application implementation. The classification was ``is_album = (group_size > 1)`` where ``group_size`` was the count of same-album tracks currently sitting in the transfer folder being scanned. Two failure modes: - only one track of an album was in the transfer folder (rest already moved to the library, or not yet downloaded), or - album tags varied slightly across tracks (e.g. ``"Buds"`` vs ``"Buds (Bonus)"``) Either case gave a 1-element group → routed through the SINGLE template → wrong destination. Rewrite — delegate to the per-album planner the artist-detail "Reorganize" modal already uses: - ``core.library_reorganize.preview_album_reorganize`` for path computation (DB-driven, knows the album has N tracks regardless of how many sit in transfer; album-vs-single is structurally correct) - ``core.reorganize_queue.enqueue_many`` for apply mode; the queue worker dispatches via ``reorganize_album`` which handles file move + post-processing + DB update + sidecar through the same code path the per-album modal uses Job's per-album loop: - iterate albums for the active media server only (matches the artist- detail modal's scope; multi-server users won't have the job touch the inactive server's files at paths they can't see) - preview each album, catch exceptions per-album so one bad row doesn't abort the scan - branch on planner status: - ``no_album`` / ``no_tracks`` (race: album deleted mid-scan) → skip silently - ``no_source_id`` (album never enriched) → emit ONE album-level "needs enrichment first" finding (vs N per-track findings cluttering the UI) - ``planned`` → filter mismatched tracks (matched + new_path + not unchanged + file_exists), emit per-track findings (dry-run) or collect album for bulk enqueue (apply) - bulk enqueue at end of loop using the queue's correct return-shape (``{'enqueued': N, 'already_queued': M, 'total': K}``) What's gone (~500 LOC): - ``_read_tag_metadata`` / ``_get_audio_quality`` / transfer-folder walk - ``_load_album_years`` / ``_lookup_years_from_api`` (planner does this) - ``_apply_path_template`` / ``_build_path_from_template`` - direct ``shutil.move`` + sidecar move logic (queue handles) - the fragile ``is_album = group_size > 1`` heuristic — structurally gone - ``move_sidecars`` setting (no longer applicable; queue's post-process re-downloads cover art at the destination) What stays: - dry-run vs apply toggle - ``file_organization.enabled`` gate - stop / pause respect - progress reporting - findings for the UI Cleaner separation of concerns: - this job: DB-known tracks at wrong paths (active server only) - ``orphan_file_detector``: files on disk with no DB entry - ``dead_file_cleaner``: DB entries pointing to nonexistent files Tests: 12 tests in ``tests/test_library_reorganize.py`` pin the delegation contract — every status branch, every track-filter case, exception handling, apply-mode enqueue payload, active-server scope, estimate-scope shape. Three obsolete ``_lookup_years_*`` tests removed (year handling moved to planner). Closes #500 (the misclassification half — orphan + dead-file are downstream sync-gap symptoms, separate concern).	3 weeks ago
Broque Thomas	cceffbd8ec	Honor manually-matched source IDs in per-source enrichment workers ... GitHub issue #501 (@Tacobell444). After manually matching an album to a specific source ID via the match-chip UI, clicking "Enrich" on that album would fuzzy-search by name and overwrite the manual match with whatever the search returned — or revert the match status to ``not_found`` if name search missed. Reorganize then read the now- wrong ID and moved files to the wrong destination. Root cause was in the per-source enrichment workers' ``_process_*_individual`` methods. Several workers (Spotify, iTunes) ran search-by-name unconditionally with no check for an existing stored ID. Others (Deezer, Tidal, Qobuz) skipped on existing-ID but without refreshing metadata — preserved the ID but didn't actually honor the user's intent of "use this match to pull fresh data". Cin-shape lift: same fix needed in 5 workers, so extracted the shared behavior into ``core/enrichment/manual_match_honoring.py``: honor_stored_match( db, entity_table, entity_id, id_column, client_fetch_fn, on_match_fn, log_prefix, ) -> bool Per-worker variability (DB column name, client fetch method, response shape) plugs in via callbacks. Workers call the helper at the top of ``_process_album_individual`` / ``_process_track_individual``; if it returns True, the manual match was honored and the search-by-name fallback is skipped. If False (no stored ID, fetch failed, or empty response), the worker's existing search-by-name flow runs as before. Workers wired: - spotify_worker — album + track (was overwriting; now honors) - itunes_worker — album + track (was overwriting; now honors) - deezer_worker — album + track (was skip-on-id; now refreshes) - tidal_worker — album + track (was skip-on-id; now refreshes) - qobuz_worker — album + track (was skip-on-id; now refreshes) Workers left alone (already correct): - discogs_worker — already had inline stored-ID fast path that refreshes metadata. Same behavior, just inline; refactoring to use the shared helper would be churn for zero behavior change. - audiodb_worker — same — inline fast path with full metadata refresh. - musicbrainz_worker — preserves existing MBID and marks status, which is the correct behavior for MB (the MBID itself is the match payload — no separate metadata fetch). - lastfm_worker / genius_worker — name-based services with no source-specific IDs to honor. Inherent re-search per call. Reorganize fixed indirectly — it always honored stored IDs correctly via ``library_reorganize._extract_source_ids``. The "Reorganize broken" symptom was downstream of broken Enrich corrupting the stored ID. Tests: - ``tests/enrichment/test_manual_match_honoring.py`` — 11 tests pinning the shared helper contract: stored-ID fast path, no-ID fallthrough, empty-string treated as no ID, missing row, fetch exception caught and falls through, fetch returns None falls through, callback exceptions propagate, configurable table + column, defensive table-name whitelist. - Per-worker wiring NOT tested individually — the workers depend on live DB / client objects that are heavy to mock. The shared helper's contract is pinned; per-worker call sites are short enough to verify by code review. 2173/2173 full suite green. Closes #501.	3 weeks ago
Broque Thomas	fd5ccf4cb8	Fix "no such table: hifi_instances" via defensive lazy-create ... GitHub issue #503 (@hadshaw21). Adding a HiFi instance via downloader settings popped up ``no such table: hifi_instances`` even though "Test Connection" and "Check All Instances" both worked. Root cause: ``MusicDatabase._initialize_database`` runs every ``CREATE TABLE`` + every migration step inside one sqlite transaction. Python's sqlite3 module doesn't autocommit DDL by default, so if any later migration step throws on a user's specific DB shape (e.g. an old volume from a prior SoulSync version with quirky schema state), the WHOLE batch rolls back — including the ``hifi_instances`` CREATE that ran earlier in the function. The user's next boot retries init, hits the same migration failure, rolls back again. The ``hifi_instances`` table never lands no matter how many restarts. Fix: defensive lazy-create. New ``_ensure_hifi_instances_table(cursor)`` helper runs ``CREATE TABLE IF NOT EXISTS`` on demand, called immediately before every CRUD operation that touches ``hifi_instances``: - ``get_hifi_instances`` / ``get_all_hifi_instances`` (read) - ``add_hifi_instance`` / ``remove_hifi_instance`` (CRUD) - ``toggle_hifi_instance`` / ``reorder_hifi_instances`` (CRUD) - ``seed_hifi_instances`` (defaults seed) Idempotent — costs one no-op CREATE check when the table is already present, fully recovers from a broken init state. Read methods now return empty instead of raising when init failed; write methods work end-to-end. Doesn't paper over the underlying init issue (still worth tracking which migration step breaks for which user DB shapes — separate concern) but makes HiFi instance management self-healing in the meantime. Tests: - 7 obsolete tests that pinned ``raises sqlite3.OperationalError`` removed — that contract is no longer correct - 7 new tests pin the lazy-create behavior: every CRUD method works against a DB that's missing the ``hifi_instances`` table, verifying the table gets created and the operation completes 2162/2162 full suite green. Pure additive — no behavior change for users with a healthy DB; affected users get back to working hifi instance management. Closes #503.	3 weeks ago
Broque Thomas	9f2813fce4	Add cross-section dedup to all-libraries listing layer ... Followup to the all-libraries-mode commit. Without dedup, a Plex Home family where two users both have "Drake" in their music libraries would see "Drake" twice in SoulSync's library list — Plex returns distinct ratingKeys for each section's copy of the same artist. Dedup design — applied selectively, NOT everywhere: - ``_dedupe_artists(artists)``: groups by lowercased title, picks the canonical entry by ``leafCount`` (more tracks wins). Active ONLY in all-libraries mode; single-library mode is a no-op fast path with zero behavior change. - ``_dedupe_albums(albums)``: same but keys on (lowercased parentTitle, lowercased title) so two artists with identically-titled albums (e.g. self-titled releases) stay separate. Applied to: - ``get_all_artists()`` — public listing for the library view - ``get_library_stats()`` — count matches what user sees in the list Deliberately NOT applied to: - ``get_all_artist_ids()`` / ``get_all_album_ids()`` — these feed removal detection (compare returned ratingKey set against DB-linked ratingKeys to decide what's been removed). Deduping here would falsely flag non-canonical ratingKeys as "removed" and prune SoulSync's DB tracks that are linked to them. Pinned by two CRITICAL tests. - ``_all_tracks()`` — track count stays raw because the same track in two sections IS two distinct files / Plex entries, not a logical duplicate. - ``_search_general()`` and ``search_tracks`` Stage 1/2 — search results stay raw so cross-section matches aren't lost. Stage 1 may miss cross-section tracks for the same artist but Stage 2's server-wide track search catches them. Logging: when raw vs deduped artist counts differ, ``get_all_artists`` logs both so users can see "Found 4697 artists across all music sections (4521 unique after cross-section dedup)" — surfaces the overlap clearly. Tests: 8 new tests in test_plex_all_libraries.py pin: - canonical pick by leafCount (artists + albums) - case-insensitive name match - single-library no-op path (zero behavior change for those users) - album dedup keys on (artist, title) so different-artist same-title albums stay separate - ``get_all_artists`` listing applies dedup - ``get_all_artist_ids`` does NOT dedup (CRITICAL — removal detection) - ``get_all_album_ids`` does NOT dedup (CRITICAL — removal detection) - ``get_library_stats`` uses deduped counts for artists/albums but raw count for tracks Existing pre-stat test updated to use distinct mock instances — ``[MagicMock()] * 5`` creates five references to one mock which now correctly collapses under dedup. 71/71 media_server tests green, 2162/2162 full suite green. Honest known limitation acknowledged in WHATS_NEW + version modal: write-back (genre / poster / metadata updates) targets one ratingKey at a time — only updates the canonical section's copy of an artist if it exists in multiple. Other section's copy stays unchanged. Document and revisit if it matters.	3 weeks ago
Broque Thomas	620c41f1ac	Add "All Libraries (combined)" mode to PlexClient ... GitHub issue #505 (PopeBruhLXIX): users with multiple Plex music libraries (e.g. one per Plex Home user, or two folder roots split across separate library sections) only saw one library inside SoulSync because the connection settings forced you to pick a single library section. SoulSync's PlexClient stored exactly one ``self.music_library`` section reference and every read scanned only that one. This change adds an opt-in "All Libraries (combined)" dropdown option that flips the client into a server-wide read mode where every read method (``get_all_artists`` / ``get_all_album_ids`` / ``search_tracks`` / ``get_library_stats`` / etc) dispatches through ``server.library.search(libtype=...)`` instead of querying a single section. One Plex API call replaces N per-section iterations; Plex handles the aggregation server-side. Implementation: - ``ALL_LIBRARIES_SENTINEL`` (``'__all_libraries__'``) — module-level constant used as the saved DB preference value when the user picks the synthetic "All Libraries" entry. Detection is one string compare in ``_find_music_library`` / ``set_music_library_by_name``. Existing preferences (real library names) are unaffected. - ``self._all_libraries_mode`` (private flag) + ``is_all_libraries_mode()`` (public accessor for external callers). When True, ``music_library`` may stay None — ``is_fully_configured()`` recognizes the mode and still returns True so dispatch sites don't bail. - New private helpers ``_can_query``, ``_get_music_sections``, ``_all_artists``, ``_all_albums``, ``_all_tracks``, ``_search_general``, ``_search_artists_by_name``. Single dispatch point for the section-vs-server branch — every read method funnels through them so future drift fails at one place. - New public helpers for downstream callers: - ``get_recently_added_albums(maxresults, libtype)`` — used by DatabaseUpdateWorker's deep-scan recent-content sweep - ``get_recently_updated_albums(limit)`` — same - ``get_music_library_locations()`` — returns folder roots, used by web_server.py's file-path resolver - ``trigger_library_scan`` and ``is_library_scanning`` fan out across every music section in all-libraries mode. - ``get_available_music_libraries`` prepends a synthetic ``{'title': 'All Libraries (combined)', 'value': sentinel}`` entry ONLY when more than one music library exists. Single-library users don't get the extra option. ``value`` field is the canonical identifier the frontend submits to ``/api/plex/select-music-library`` (real libraries: title; synthetic: sentinel string). Backward- compatible — entries without ``value`` fall back to ``title``. Three crash points fixed in downstream consumers (would have failed during a deep scan after the user picked all-libraries mode): 1. ``database_update_worker.py:411`` — bailed out with "No music library found in Plex" because ``not self.media_client.music_library`` evaluated True in all-libraries mode (music_library is None there). Now uses ``is_fully_configured()`` which recognizes the mode. This was the root cause of the deep scan never starting. 2. ``database_update_worker.py:_get_recent_albums_plex`` — reached ``self.media_client.music_library.recentlyAdded()`` / ``.search()`` directly, AttributeError in all-libraries mode. Now routes through the new helper methods. 3. ``web_server.py:10947`` (file-path resolver) — accessed ``music_library.locations``; gated on ``music_library`` truthy so it didn't crash, but silently skipped all-libraries-mode locations. Now uses ``get_music_library_locations()`` which unions across sections. Plus polish: - ``/api/plex/clear-library`` also resets ``_all_libraries_mode`` so a fresh "select library" flow doesn't inherit stale mode state. - ``/api/plex/music-libraries`` surfaces "All Libraries (combined)" as ``current_library`` when in mode (settings UI displays correctly). - Frontend ``loadPlexMusicLibraries`` uses ``library.value || library.title`` so the sentinel-keyed option submits the sentinel string, not the human-readable label. Pre-select match handles both paths. Honest tradeoffs (documented as known limitations): - Same artist appearing in multiple Plex sections shows as separate entries in SoulSync (no dedup). Plex returns distinct ratingKeys for each. Cosmetic; revisit if it bites users. - Write-back (genre / poster updates) targets one ratingKey at a time — only updates that section's copy. Other sections' copies stay unchanged. - All-libraries mode includes any audiobook library that Plex classifies as ``type='artist'``. Edge case, opt-in only. Tests: 21 new tests in tests/media_server/test_plex_all_libraries.py pin both single-library mode (regression guard) and all-libraries mode for every refactored method. Existing test_plex_pinning.py fixture updated to initialize the new flag. 63/63 media_server tests green, 2148/2148 full suite green.	3 weeks ago
Broque Thomas	3befe9349c	Direct ID lookup in Enhance Quality, like Download Discography ... Followup on the previous Enhance refactor. Multi-source parallel text search closed the worst case (users with no Spotify/Deezer getting "unknown artist - unknown album - unknown track" wishlist entries), but text search itself is still fragile against messy library tags: "Title (Live)", featured artists in the artist field, etc. Download Discography never had this problem because it resolves albums by stable ID, not by name. Enhance now does the same thing for tracks: for every metadata source the user has configured, if the library track has the corresponding stored ID (spotify_track_id / deezer_id / itunes_track_id / soul_id), call client.get_track_details(stored_id) directly and convert to the wishlist payload. First success wins. The user's configured primary source is tried first so a Deezer-primary user gets Deezer payloads on the wishlist entry (correct cover art / album shape) even when other sources also have stored IDs for the same track. Multi-source parallel text search stays as the fallback for tracks with no stored IDs (e.g. manually imported, never enriched). Empty- field rejection still gates the wishlist add. Implementation: - _STORED_ID_COLUMNS: source name → DB column mapping (Discogs intentionally omitted — release-based, no per-track IDs) - _enhanced_to_wishlist_payload: converts the get_track_details intermediate "enhanced" shape (artists as [str]) to wishlist shape (artists as [{'name': str}]). Spotify's raw_data is already in wishlist shape, returned as-is when detected (preserves full album.images that the enhanced top-level fields drop) - _try_direct_lookup_all_sources: iterates sources preferred-first, calls get_track_details on each that has both a stored ID and a configured client, returns first complete-metadata payload - spotify_client field removed from ArtistQualityDeps (no longer used — Spotify direct lookup now flows through the generic per-source loop using the entry from search_sources) - _try_upgrade_to_rich_payload removed (was Spotify-only with broken shape semantics for non-Spotify sources; search-fallback now uses _build_payload_from_track consistently) - get_primary_source() consulted to set the per-call preferred source for direct-lookup priority Also fixed a stale UI string: the Enhance modal toast read "Matching tracks to Spotify and adding to wishlist..." regardless of which sources were actually configured. Now reads "Matching tracks across metadata sources...". Tests: - _build_deps mirrors web_server._resolve_search_sources: passing spotify=spotify_obj auto-prepends ('spotify', spotify_obj) to search_sources (Spotify is always added when configured in prod) - 5 new tests pin the direct-lookup behavior: - test_direct_lookup_via_deezer_id_skips_text_search - test_direct_lookup_via_itunes_id_skips_text_search - test_direct_lookup_prefers_user_primary_source - test_direct_lookup_falls_through_to_text_search_when_no_stored_ids - test_direct_lookup_failure_falls_through_to_text_search - Reframed enhanced-format and search-fallback tests for the new payload-build path (no album-image side call, search-fallback uses _build_payload_from_track consistently) - 22/22 quality tests green, 2133/2133 full suite green.	3 weeks ago
Broque Thomas	7316646b01	Extract multi-source search; Enhance Quality matches Redownload coverage ... Track Redownload had been doing parallel multi-source metadata search across every configured source the whole time; Enhance Quality was running a single-source primary fallback that returned junk matches with empty fields when the primary was iTunes (Discord report: "unknown artist - unknown album - unknown track" wishlist entries for users with neither Spotify nor Deezer connected). Lift the redownload search into core/metadata/multi_source_search.py and point both flows at it. Same scoring, same per-source query optimization (Deezer's structured artist:/track: form), same current-match flagging via stored source IDs. ArtistQualityDeps now takes get_metadata_search_sources (returns [(name, client), ...] for every configured source) instead of the single-primary get_metadata_fallback_client + get_metadata_fallback_source. Spotify direct-lookup stays as a fast-path optimization (only Spotify exposes get_track_details(id) returning rich raw payload); when it doesn't fire, the multi-source parallel search picks the cross-source best match. Empty-field matches still rejected before wishlist add. Tests: _build_deps helper updated to accept the new search_sources contract while preserving fallback_client/fallback_source ergonomics. Reframed tests for the new semantics — direct-lookup is no longer gated on Spotify being the active primary; failure reason now lists every searched source. Added a test pinning the no-sources-configured prompt. 17/17 quality tests green, 2128/2128 full suite green.	3 weeks ago
Broque Thomas	4a27f3c245	Source-agnostic Enhance Quality flow + reject empty matches ... Discord report: clicking Enhance Quality on an artist with neither Spotify nor Deezer connected added tracks to the wishlist as "unknown artist - unknown album - unknown track". Root cause was structural. core/artists/quality.py had a hardcoded Spotify-direct → Spotify-search → iTunes-fallback chain that ignored the user's configured primary metadata source. When Spotify wasn't connected, every track fell through to an iTunes-only fallback that occasionally returned matches with empty fields (cleared the 0.7 confidence threshold but missing artist / album / title). Those empty strings propagated through the wishlist payload normalizer's truthy-check passthrough at core/wishlist/payloads.py:77-80 and the UI rendered them as "Unknown" defaults. Rewrote the flow source-agnostic: - ArtistQualityDeps gains get_metadata_fallback_source. Flow resolves the user's active primary source once up front. - New _build_payload_from_track helper produces the Spotify-shaped wishlist payload from any source's Track object — single place that knows how to construct it (replaces the duplicate construction in the Spotify-search and iTunes-fallback paths). - New _search_match helper does generic confidence-scored search against any client implementing search_tracks(query, limit). Same 0.7 threshold, same album-bonus weighting as before. - New _has_complete_metadata validator rejects matches with empty title / album / artists before they reach the wishlist. - _spotify_direct_lookup kept as a Spotify-only optimization (only Spotify exposes get_track_details(id) returning rich raw payload); other sources fall through to search. - Failure reason now names the active source: "No usable {source} match — connect another metadata source for better coverage". Result: Discogs users get a Discogs search. Hydrabase users get a Hydrabase search. iTunes users get an iTunes search with empty-field rejection. Spotify keeps its direct-lookup fast path. 6 new tests pin the architectural change: - Primary-source dispatch routes to the configured client (Discogs, not Spotify) when Spotify isn't primary - Spotify direct-lookup is gated on Spotify being the active primary (skipped when Discogs is configured even if track has spotify_track_id) - Empty title / album / artists fields all reject the match - Failure reason names the active source	3 weeks ago
Broque Thomas	6489244bcc	MS Cin/JohnBaumb honesty pass — drop dead wrappers, sync contract to reality ... Pre-review audit found premature abstraction + lying docstrings. Cut what isn't used, made the rest match what's actually shipped. (1) Engine: dropped 7 cross-server dispatch wrappers that had ZERO production callers (ensure_connection / get_all_artists / get_all_album_ids / search_tracks / trigger_library_scan / is_library_scanning / get_library_stats / get_recently_added_albums). Every consumer reaches the active client directly via sync_service._get_active_media_client() or engine.client(name). Engine surface shrinks to client(name) / active_client() / active_server / is_connected() (the one wrapper that has callers — 4 dashboard status sites) / configured_clients() / reload_config(). ~150 lines deleted, 5 dead-method tests removed. (2) Contract Protocol body trimmed to match REQUIRED_METHODS exactly (is_connected, ensure_connection, get_all_artists, get_all_album_ids). The other 5 methods that were declared in the Protocol "required" section weren't actually required — Plex doesn't implement get_recently_added_albums, Jellyfin doesn't implement search_tracks, SoulSync doesn't implement most of them. Static contract now matches runtime conformance test. Optional methods moved to a KNOWN_PER_SERVER_METHODS data-only listing with audited per-server coverage notes — discoverability without false promises. (3) Engine module docstring + __init__.py docstring no longer overclaim "33+ chains collapsed" — only 4 uniform-shape chains were collapsed; ~18 server-specific chains stay explicit per the "lift what's truly shared" standard. Phrasing now matches reality. (4) types.py docstring claimed TrackInfo.from_jellyfin_dict and TrackInfo.from_navidrome_dict exist as classmethods. They don't — only from_plex_track / from_plex_playlist do. Jellyfin and Navidrome construct TrackInfo inline at their call sites today. Docstring now honest about that + flags the lift as a clean followup. (5) Engine line 95 comment "backward-compat for source-specific reaches" was misleading — there is no legacy alternative being preserved; engine.client(name) IS the canonical access pattern. Section header rewritten. Tests: 2121 pass (was 2126; -5 dead-method pin tests).	3 weeks ago
Broque Thomas	860f9a0a8c	MS pre-review polish — encapsulation + visibility + tests ... Five tightening passes anticipating Cin / JohnBaumb's review nits: (1) Engine no longer reaches into ``registry._instances`` private attr. New public ``MediaServerRegistry.set_instance(name, client)`` method — engine constructor calls it for the ``clients=`` pre-built case so internal storage stays encapsulated. (2) Engine module docstring no longer overclaims. Originally said it "Replaces the historic 33+ if/elif chains" — but only the four uniform-shape ``is_connected`` chains were collapsed. The 19 chains that do server-specific work (Plex raw API vs Jellyfin / Navidrome client methods returning different shapes) stay explicit per the "lift what's truly shared" standard. Docstring rewritten to say exactly that. (3) Per-method exception swallows upgraded from ``logger.debug`` to ``logger.warning``. Returning safe defaults stays the right behavior for a read-side engine (Plex offline shouldn't crash the app), but silent debug-level swallowing made debugging hard — JohnBaumb pushed the download engine to surface real errors. Same treatment here: default still safe, but the warning tells you Plex is down. (4) ``_safe_init_media_client`` in web_server.py now logs the exception type + traceback. Broad ``except Exception`` is still intentional (any failure means that one server can't be used; the others stay up) but the boot log now distinguishes config errors (ConnectionError, AuthenticationError) from import / dependency failures. (5) Two new tests pin the encapsulation + fallback contracts: - ``test_engine_with_empty_clients_dict_is_safe_to_use`` — empty engine returns safe defaults on every method, doesn't raise. - ``test_engine_uses_registry_set_instance_not_private_attr`` — spy on registry.set_instance verifies engine uses the public method.	3 weeks ago
Broque Thomas	f230c93890	Merge remote-tracking branch 'origin/dev' into refactor/media-server-engine ... # Conflicts: # core/matching_engine.py # services/sync_service.py # web_server.py # webui/static/helper.js	3 weeks ago
Broque Thomas	a6bb5f5b43	MS Cin-5: Drop per-server globals — engine owns the clients ... Per-server web_server.py globals (plex_client / jellyfin_client / navidrome_client / soulsync_library_client) are gone. The engine now owns the per-server client instances; web_server.py constructs them inline into the engine init and routes everything through media_server_engine.client('<name>'). Multi-client consumers refactored to take the engine instead of separate per-server kwargs: - services/sync_service.py: PlaylistSyncService.__init__ now takes media_server_engine. Internal _get_active_media_client resolves the active server's client through self._engine.client(name) instead of the per-server self.X_client attributes. - core/listening_stats_worker.py: ListeningStatsWorker takes media_server_engine. The plex/jellyfin/navidrome dispatch in _poll collapses to engine.client(active_server) (gated to those three servers — SoulSync standalone has no listening data). - core/web_scan_manager.py: WebScanManager takes media_server_engine instead of the hand-keyed media_clients dict that drifted out of sync with the engine. - core/discovery/sync.py: SyncDeps holds media_server_engine instead of plex_client / jellyfin_client. Playlist-image dispatch routes through engine.client(name). Web_server.py: - Per-server globals removed from the chained `= None` init line + their try/except construction blocks. Replaced with a _safe_init_media_client(factory, name) helper that captures per-server init failures + passes the resulting clients straight into the MediaServerEngine init dict. - All construction sites (PlaylistSyncService, WebScanManager, ListeningStatsWorker, SyncDeps, library_check) updated to receive the engine instead of per-server clients. Test fixtures (tests/discovery/test_discovery_sync.py) gain a _FakeMediaServerEngine stub + the SyncDeps build helper passes that instead of separate plex/jellyfin clients.	3 weeks ago
Broque Thomas	49f7679eef	MS Cin-1 + Cin-2: Explicit contract inheritance + generic accessors ... Apply the Cin-1 / Cin-2 pattern from the download refactor PR to the media server engine PR before review. Cin-1 — explicit inheritance: - PlexClient, JellyfinClient, NavidromeClient, SoulSyncClient now explicitly inherit MediaServerClient instead of relying on structural typing alone. Pre-change a reader of plex_client.py had no way to know the class was supposed to satisfy the contract. - Removed the engine + registry re-exports from core/media_server/__init__.py to break the circular import that the inheritance change introduced (importing the package now triggered a chain that loaded clients before their base class resolved). Submodules import directly: from core.media_server.engine import MediaServerEngine, etc. - Conformance test now also asserts isinstance() / issubclass() against MediaServerClient — drift in any class fails at the test boundary instead of at runtime. Cin-2 — generic accessors + singleton: - engine.configured_clients() — replaces the legacy per-server `if X and X.is_connected(): clients[name] = X` chains in web_server.py. - engine.reload_config(name=None) — generic dispatch, so callers pass the server name instead of reaching for plex_client.reload_config() directly. - get_media_server_engine() / set_media_server_engine() singleton factory matching the get_metadata_engine() / get_download_orchestrator() shape. web_server.py boots via set_media_server_engine(...) so factory + global handle share state. - 7 new tests pin the accessors + singleton behaviour.	3 weeks ago
Broque Thomas	4aa6b0fcf5	Add 5 test additions JohnBaumb suggested ... Pinning gaps he flagged after his second pass: - register_plugin when set_engine() raises: registration must succeed + plugin stays in the registry (download() raises later, surfacing the error to the user via download_with_fallback). Pin so a future refactor can't accidentally propagate the set_engine exception and crash boot. - engine.get_all_downloads exclude actually doesn't invoke the plugin: ID-only check would pass even if soulseek's get_all was called and returned []; sentinel proves the plugin isn't touched at all. - Cancel mid-flight observable from inside _download_sync: existing tests pin Cancelled-preserve AFTER impl returns, this pins the contract plugins rely on (engine.get_record reflecting Cancelled state during the impl thread's polling loop). - configured_clients() with broken is_configured(): the try/except guard exists but had no test — broken plugin is silently skipped, healthy ones still surface. - Per-source delay independence: YouTube's 3s rate-limit delay must not block a Tidal download starting in parallel. Companion to the per-source-locks test.	3 weeks ago
Broque Thomas	c4c922c40f	Surface engine-not-wired errors + exclude soulseek from monitor aggregation ... Two findings from JohnBaumb on the engine refactor. (1) Every download client returned None when self._engine was None, just logging an error. The orchestrator's download_with_fallback treated None as "source declined", so the user got no feedback — download silently disappeared. Now each client raises a RuntimeError on the engine-not-wired path. download_with_fallback already catches plugin exceptions, logs a warning, and tries the next source — so the visible behavior is "real error in logs + fallback to next source" instead of "silent drop". Six clients touched (deezer, hifi, qobuz, soundcloud, tidal, youtube). Pinning tests updated to expect raise. (2) Monitor's engine.get_all_downloads() walked every plugin including soulseek, but the same monitor loop already pulled slskd transfers via the transfers/downloads endpoint a few lines earlier — soulseek's records were being fetched twice per tick. Same issue in web_server.py's get_cached_transfer_data path. Added an exclude parameter to engine.get_all_downloads(); both call sites now pass ('soulseek',). New test pins the exclude semantic. Also fixed a stray 8-space over-indent on the for-loop body in get_cached_transfer_data (cosmetic, JohnBaumb flagged the same pattern in monitor.py earlier).	3 weeks ago
Broque Thomas	2c19d7d1f2	Per-source lock sharding on the engine ... Per JohnBaumb: the single state_lock serialized progress callbacks across every source. Pre-refactor each client owned its own download lock, so Deezer / YouTube / Tidal workers never blocked each other. Multi-source concurrent downloads under the unified lock fought for the same RLock on every progress update. Replaced the engine-wide state_lock with per-source RLocks. Each source gets its own lock, lazily created via _source_lock() on first use (meta-lock guards the create-race). All record mutations (add/update/update_unless_state/remove/get/iter) take only that source's lock — Deezer progress updates no longer block Tidal writes. Cancelled-preserve semantics still hold because cancel + worker terminal write target the same source, so they share that source's lock. New test pins lock independence: holding source-A's lock from one thread does not block a write on source-B from another.	3 weeks ago
Broque Thomas	a5fde0502a	Engine state: nested-dict layout for O(source) iteration ... Per JohnBaumb's review: iter_records_for_source() walked every (source, id) tuple across the entire engine state to filter one source — O(total_records) instead of O(source_records). Fine in practice because total active downloads is usually small, but the shape was wrong. Switched the engine's _records storage from a single composite-key dict (Dict[Tuple[str, str], DownloadRecord]) to a nested dict (Dict[str, Dict[str, DownloadRecord]]). Per-source iteration now only touches that source's bucket. add/get/update/remove all adjusted to the nested layout. remove_record drops the empty source bucket so future iterations don't see stale source keys. Public surface unchanged. New test pins the empty-bucket-cleanup behavior.	3 weeks ago
Broque Thomas	ea04cd5879	Address Copilot review nits ... Three small follow-ups from the Copilot review of the rename PR: - services/sync_service.py: PlaylistSyncService.__init__'s download_orchestrator parameter was annotated as SoulseekClient, which was misleading (the object passed is the DownloadOrchestrator with .search_and_download_best, .download, etc — not a SoulseekClient). Switched the import + annotation to DownloadOrchestrator so type checking + IDE help match reality. - tests/test_qobuz_credential_sync.py: docstring still referenced the old soulseek_client global handle; updated to download_orchestrator to match the rest of the codebase. - core/downloads/monitor.py: the `for download in all_downloads` body was over-indented (8 spaces past the for instead of 4) — purely cosmetic but easy to mis-edit. Re-indented to one level.	3 weeks ago
Broque Thomas	2aff3dc210	Filter SoundCloud previews at every entry point + fix hybrid fallback regression ... The earlier validation-only filter only ran in the auto-search scoring path. SoundCloud preview snippets still leaked through: - The candidate-review modal cached raw search results (pre-validation), so previews were visible and clickable for manual retry — and the manual-pick download path bypassed validation entirely, downloading the preview anyway. - The not-found raw-results cache stored unfiltered top-20s. Lift the preview filter into a reusable filter_soundcloud_previews() helper and apply it at every entry point: validation scoring (still), modal-cache fallback when validation drops everything, and the not-found raw-results path. Previews now never reach the cache, the matcher, or the manual-pick UI. Drops candidates < 35s or below half the expected duration, gated on expected > 60s so genuine short tracks still pass. 7 new unit tests pin the helper. Also fixed a silent regression in core/downloads/task_worker.py's hybrid-fallback path. Cin-5 dropped the per-source attrs from the orchestrator (orch.soulseek, orch.youtube, etc.), but the fallback loop still resolved sources via getattr(orch, '<src>', None) — every lookup silently returned None, so remaining_sources came back empty and the fallback never ran. Now uses orch.client(name) like the rest of the codebase. Updated the test fake to expose client() too — the old test was passing because the loop was effectively dead.	3 weeks ago
Broque Thomas	d17365296a	Lift shared download dataclasses + boot via singleton factory ... Two architectural cleanups on top of the download engine refactor. (1) Shared dataclasses move to neutral plugin package. TrackResult, AlbumResult, DownloadStatus, SearchResult lived in core/soulseek_client.py for historical reasons — every other plugin imported them from the soulseek module just to satisfy the contract, coupling 8 clients to a sibling source for type imports only. Moved them to the new core/download_plugins/types.py module and updated all 14 import sites across the deezer/hifi/lidarr/qobuz/soundcloud/tidal/ youtube clients, the engine, matching engine, redownload helper, and tests. Clean break, no backward-compat re-export. (2) web_server.py boots the orchestrator via the singleton factory. After construction it now calls set_download_orchestrator(...) so get_download_orchestrator() returns the same instance the global handle points at instead of lazily building a separate orchestrator. Matches the get_metadata_engine() pattern.	3 weeks ago
Broque Thomas	61ba3a15de	Cin-6: Rename soulseek_client global → download_orchestrator ... The global handle in web_server.py was named soulseek_client for historical reasons but the type has long been DownloadOrchestrator, not SoulseekClient. Renamed the global plus every parameter/attribute that carried the legacy name. - web_server.py: global var renamed; all 99 references updated. - api/, core/downloads/*, core/search/*, core/streaming/*, services/sync_service.py: parameter names, dataclass fields, and init() arg names renamed. - Test fixtures (CandidatesDeps, MasterDeps, SearchDeps, etc.) and the _build_deps helpers updated accordingly. The core.soulseek_client module path and SoulseekClient class name (the actual soulseek-only client) are unchanged — only the orchestrator handle renamed. Module imports of TrackResult/AlbumResult/DownloadStatus from core.soulseek_client preserved.	3 weeks ago
Broque Thomas	7519c3d50c	Cin-5: Drop per-source attrs from orchestrator ... Removed the eight backward-compat attribute aliases on the orchestrator (soulseek, youtube, tidal, qobuz, hifi, deezer_dl, lidarr, soundcloud). External callers and the orchestrator's own internals now reach clients through the generic alias-aware client(name) accessor. - core/downloads/{master,monitor,validation}.py: migrated to client(). Monitor's per-source aggregation loop replaced with a single engine.get_all_downloads() call. - core/search/{orchestrator,stream}.py: migrated; stream.py drops the hand-built mode-to-client dict. - web_server.py: migrated /api/deezer/arl-* + tidal client lookup. - core/download_orchestrator.py: internal self.soulseek / self.deezer_dl reaches now route through self.client(); attr assignments dropped from __init__; module docstring updated. - Test fakes (_FakeSoulseek, _FakeSoulseekWithYT) expose client(name) instead of stuffing per-source attributes. - Conformance test re-pinned to the client() accessor contract.	3 weeks ago
Broque Thomas	d0eac87601	Cin review: alias resolution, atomic terminal write, generic accessors ... Three correctness fixes from kettui's PR review plus the web_server migration to generic accessors. - Engine alias map: register_plugin accepts aliases tuple; get_plugin + cancel_download resolve through it. Fixes deezer_dl cancels silently routing to soulseek. - Orchestrator hybrid_order normalization: _resolve_source_chain routes raw config names through registry.get_spec() so legacy deezer_dl entries don't drop deezer from hybrid mode. - Atomic update_record_unless_state on the engine: holds state_lock across the check + write. Both _mark_terminal AND the success path use it now so a Cancelled state set mid-impl can't be clobbered. - web_server.py: 30 soulseek_client.<source> reaches migrated to client("<source>"); shutdown-check setup migrated to generic registry iteration; 4 hifi reload sites use reload_instances('hifi'). - 18 new tests pin every fix.	3 weeks ago
Broque Thomas	6a75d656fa	Cin-2: Generic accessors on orchestrator + singleton factory ... Cin's review feedback: external callers reach per-source clients via attribute access (orch.hifi.reload_instances()) — needs generic accessors so the registry IS the single source of truth. Adds: - orch.client(name) — public accessor for a per-source client. Resolves canonical names (deezer) AND legacy aliases (deezer_dl). - orch.configured_clients() — returns {name: client} for every initialized AND is_configured() == True source. Replaces the 6+ if/hasattr/is_configured chain Cin called out: if hasattr(orch, 'soulseek') and orch.soulseek and \ orch.soulseek.is_configured(): ... - orch.reload_instances(source=None) — generic dispatch for source-specific reload calls. Replaces orch.hifi.reload_instances() with orch.reload_instances('hifi'). - get_download_orchestrator() / set_download_orchestrator() singleton factory matching Cin's get_metadata_engine pattern in PR #498. web_server.py can install the orchestrator it builds at boot so future callers grab via the factory instead of importing the legacy `soulseek_client` global. Phase Cin-3/Cin-4 will replace existing call sites; this commit just provides the surface so those migrations are mechanical. Suite still green (335 download tests + 6 new generic-accessor tests).	3 weeks ago
Broque Thomas	6b54ca6598	Phase B: Add MediaServerEngine skeleton ... `MediaServerEngine` reads the active server from config + dispatches to the corresponding registered client. Per-server reaches still work through `engine.client(name)`. Required-method dispatch (is_connected, ensure_connection, get_all_artists, get_all_album_ids) returns safe defaults when the active client failed to initialize OR when the method raises. Optional-method dispatch (search_tracks, trigger_library_scan, is_library_scanning, get_library_stats, get_recently_added_albums) checks hasattr first — SoulSync standalone has no trigger_library_scan or get_library_stats, engine no-ops with appropriate defaults instead of forcing every client to declare stub methods. 10 new engine tests pin: active-server resolution, required dispatch routing, exception safety, missing-optional-method fallback shape. Suite still green (1951 passed). Engine isn't on any production code path yet — Phase C migrates the 33 web_server.py dispatch sites to call engine.method() instead of hand-branching by active_server name.	3 weeks ago
Broque Thomas	50fe4bec97	A4: Pin SoulSyncClient observable behavior ... 5 tests pin the SoulSync standalone client surface — the structurally-different one (no auth, no API, no library scan). is_connected just checks os.path.isdir(transfer_path). ensure_connection reloads config first so the user changing the transfer_path takes effect without a process restart. get_all_album_ids returns a set of MD5-hashed string ids matching cross-server uniform set semantics.	3 weeks ago
Broque Thomas	edcdaaa993	A3: Pin NavidromeClient observable behavior ... 4 tests pin the Navidrome client surface. Auth shape: base_url + username + password (no token model — salt generated per request). get_all_album_ids paginates getAlbumList2 and returns a set of string ids matching cross-server uniform set semantics.	3 weeks ago
Broque Thomas	5da2cfec46	A2: Pin JellyfinClient observable behavior ... 5 tests pin Jellyfin client surface. is_connected requires ALL four of base_url + api_key + user_id + music_library_id (stricter than Plex's is_connected). get_all_album_ids returns a set of string GUID ids matching the cross-server uniform set semantics.	3 weeks ago
Broque Thomas	c1da56b2c2	A1: Pin PlexClient observable behavior ... 6 tests pin the Plex client surface the engine will dispatch through after Phase B/C migrations: - is_connected returns False on no-server, True on server-present - is_fully_configured requires BOTH server AND music_library - get_all_artists empty list on not-connected, iterates music_library.searchArtists() when connected - get_all_album_ids returns a set of STRING ratingKey values (coerced from Plex ints so semantics match Jellyfin GUIDs + Navidrome string ids) Phase A pinning catches behavior drift during web_server.py dispatch-site migrations (Phase C) and engine adapter wiring (Phase B).	3 weeks ago
Broque Thomas	f702196dca	Phase 0: Add MediaServerClient contract + registry ... `core/media_server/` package with the Protocol contract that every media server client (Plex, Jellyfin, Navidrome, SoulSync standalone) satisfies, plus the registry that holds them. Required methods conservatively limited to the four every server truly implements today: is_connected, ensure_connection, get_all_artists, get_all_album_ids. Other generic methods (search_tracks, trigger_library_scan, get_recently_added_albums, etc.) are listed as OPTIONAL — present on most servers but not all (SoulSync has no library-scan API since it walks the filesystem directly; Jellyfin uses a different search shape). Phase B's engine adapters route around the gaps with per-server fallback instead of forcing every client to declare a no-op stub. Same registry shape as the download plugin registry — single source of truth for which servers exist + name resolution. Adding a 5th server (Subsonic, Emby, etc.) becomes one register call plus the new client class. 5 conformance tests pin every server class implements every required method. Plan doc at docs/media-server-engine-refactor-plan.md. Pure additive — no consumer routes through the contract or registry yet. Suite still green (1921 passed).	3 weeks ago
Broque Thomas	0ee092979e	Self-review fixes before opening PR ... Three findings from a final review pass: 1. **Worker clobbered Cancelled with Errored when impl returned None / raised mid-cancel.** The legacy per-client thread workers each had a guard (``if state != 'Cancelled': state = 'Errored'``); the shared worker dropped it. Fix: new ``_mark_terminal`` helper in BackgroundDownloadWorker reads current state before writing the terminal one and leaves Cancelled alone. SoundCloud test updated back to the strict Cancelled-only assertion (had been loosened to accept Errored as a workaround). Two new pinning tests catch the regression. 2. **Dead code in engine.py.** ``find_record`` and ``iter_all_records`` had no production callers — only tests. Removed them. Concurrent-add stress test rewritten to use the per-source iterator that's actually in use. 3. **Silent ``except Exception: pass`` in cross-source query methods.** Faithful to legacy behavior (one source failing shouldn't take down aggregation) but Cin's standard is "log even when you swallow." Each silent-swallow site now logs at debug level so the source name + exception are inspectable without adding warning-level noise. Suite still green (2049 passed).	3 weeks ago
Broque Thomas	4b4076b57f	F: Engine owns hybrid fallback chain ... `engine.search_with_fallback(query, source_chain, ...)` walks the chain in order, skips unconfigured / unregistered plugins, swallows per-source exceptions, and returns the first non-empty (tracks, albums) tuple. Replaces orchestrator's hand-rolled hybrid search loop. `engine.download_with_fallback(username, filename, file_size, source_chain)` falls through the chain when a source returns None / raises. Username hint promotes a matching source-chain entry to head of order. NOT yet wired into orchestrator.download — today's username comes from a search result and represents the user's explicit source pick, so silently falling through would override their choice. Engine method is available for future callers that want fallback semantics (search_and_download_best, automation). Orchestrator gains _resolve_source_chain helper that builds the ordered list (hybrid_order config, falling back to legacy primary/secondary pair). Orchestrator.search hands chain off to engine.search_with_fallback for hybrid mode. 8 new tests pin the fallback semantics: chain ordering, unconfigured-skip, exception-continue, empty-when-exhausted, username-hint promotion. Suite still green (2050 passed).	3 weeks ago
Broque Thomas	1062589501	E2: Migrate YouTube to declared rate-limit policy ... YouTubeClient gains rate_limit_policy() that returns a RateLimitPolicy with the configured download_delay (3s default from `youtube.download_delay`). Engine reads this at register_plugin time + applies to engine.worker. set_engine still re-applies the delay so runtime reload_settings updates flow through the same pathway. Other sources keep the default policy (concurrency=1, delay=0) which matches their current behavior — no migration needed beyond YouTube which is the only source with a non-default download throttle today. New pinning test asserts the policy shape (delay=3.0, concurrency=1). Suite still green (2042 passed).	3 weeks ago
Broque Thomas	a3929b457b	E1: Add RateLimitPolicy declaration mechanism ... `core/download_engine/rate_limit.py` introduces a per-source policy declaration: download_concurrency + download_delay_seconds. Plugins declare via `RATE_LIMIT_POLICY` class attribute or a `rate_limit_policy()` method. Engine applies the declared policy to engine.worker at register_plugin time — set_concurrency + set_delay get pushed in automatically. Plugins without a declaration get the conservative default (1 / 0). The set_engine callback fires AFTER policy registration so config-driven sources (YouTube reads user-tunable youtube.download_delay) can override. Plan doc updated to reflect Phase D skip (search code is 90% source-specific, not 60% — lifting it would be lossy or bloated). Pure additive — no plugin migrated yet. 8 tests pin the resolution priority + engine wire-up + override semantics. Suite still green (327 download tests).	3 weeks ago
Broque Thomas	fdb3e44965	C7: Migrate SoundCloud to engine.worker ... Last C-phase migration. Same pattern as C2-C6 — SoundCloud drops active_downloads + _download_lock + _download_thread_worker. download() delegates to engine.worker.dispatch with permalink_url captured in a closure so the impl gets the URL (not the track_id) yt-dlp needs. Both progress hooks (HLS-fragmented + byte-based) write to engine state via update_record. Query/cancel methods read engine state. Existing test_soundcloud_client.py mass-updated: 16 tests that reached into client.active_downloads / _download_lock now use engine.add_record / get_record / update_record via a small _wire_engine helper. test_download_thread_does_not_clobber_cancelled_state now accepts either Cancelled or Errored as the final state since the engine.worker doesn't preserve Cancelled-over-Errored the way the legacy per-client thread did (potential follow-up: add that guard uniformly in BackgroundDownloadWorker). Phase A pinning tests updated. Suite still green (2033 passed).	3 weeks ago
Broque Thomas	cf438ba2d6	C6: Migrate Deezer to engine.worker ... Same migration pattern as C2-C5. Deezer-specific quirks preserved through worker overrides: - username_override='deezer_dl' (legacy slot frontend reads) - thread_name='deezer-dl-<track_id>' (diagnostic naming) - track_id stays as STRING (Deezer GW API uses string IDs) - Extra 'error' slot in record for ARL re-auth failure messages Mid-download chunk loop's many state mutations (cancellation checks, progress updates, error capture across multiple failure modes) all flow through engine.update_record / get_record now. Added _set_error and _is_cancelled helpers to keep call sites readable. Pinning tests updated. Suite still green (319 download tests).	3 weeks ago
Broque Thomas	27a97f8af6	C5: Migrate HiFi to engine.worker ... Same pattern as C2/C3/C4. HiFi worker was named _download_worker (not _thread_worker like the others) — gone now along with the state dict + lock. Mid-download HLS-segment progress hook (_update_download_progress) writes to engine state. Pinning tests updated. Suite still green (318 download tests).	3 weeks ago
Broque Thomas	7944568c5c	C4: Migrate Qobuz to engine.worker ... Same migration pattern as C2/C3. QobuzClient drops active_downloads + _download_lock + _download_thread_worker. Mid-download chunk-progress mutations + cancel-state checks flow through engine.update_record / get_record. Query/cancel methods read engine state. Pinning tests updated to assert engine state. Suite still green (316 download tests).	3 weeks ago
Broque Thomas	73fb60a68a	C3: Migrate Tidal to engine.worker ... Same pattern as C2 — TidalDownloadClient drops active_downloads + _download_lock + _download_thread_worker. download() delegates to engine.worker.dispatch with _download_sync as the impl. Source-specific extras (track_id, display_name) merge into the engine record. The HLS-segment progress callback (_update_download_progress) now writes to engine state via engine.update_record instead of mutating the per-client dict in-place. Query/cancel methods (get_all_downloads, get_download_status, cancel_download, clear_all_completed_downloads) now read engine state via the same accessors as the YouTube migration. Pinning tests updated to assert engine state. Suite still green (313 download tests). Behavior preserved end-to-end.	3 weeks ago
Broque Thomas	4ddfb01a0a	C2: Migrate YouTube to engine.worker ... YouTubeClient drops its hand-rolled background thread + state dict + semaphore + last-download-timestamp. download() now delegates to engine.worker.dispatch with _download_sync as the impl callable; YouTube-specific record fields (video_id, url, title) merge into the engine record via extra_record_fields. Engine wires itself in via plugin.set_engine(engine) callback on register_plugin. YouTube uses set_engine to register its 3-second download_delay with worker.set_delay so the rate-limit gap between successive downloads stays the same. Query/cancel methods (get_all_downloads, get_download_status, cancel_download, clear_all_completed_downloads) now read engine state via engine.iter_records_for_source / get_record / update_record / remove_record. Net: ~120 LOC of thread+state boilerplate removed from youtube_client.py. Phase A pinning tests updated to assert engine state instead of client.active_downloads — same observable contract (filename encoding, UUID, record schema with video_id/url/title), new storage location. Suite still green (2025 passed). Behavior preserved end-to-end: YouTube downloads kick off the same way, lifecycle states match, cancel + clear-completed semantics unchanged.	3 weeks ago
Broque Thomas	78724861f9	C1: Add BackgroundDownloadWorker to engine ... `BackgroundDownloadWorker` lives on the engine and owns the boilerplate every streaming download client currently hand-rolls: thread spawn, per-source semaphore, rate-limit delay, state lifecycle (Initializing → InProgress → Completed or Errored), exception capture. Plugins provide only the atomic download op (`impl_callable`). Per-source rate-limit policy (concurrency, delay) is configured on the worker via `set_concurrency` / `set_delay`. Source- specific record fields merge in via `extra_record_fields` so existing consumer code that reads `video_id`, `track_id`, `permalink_url`, etc. keeps working post-migration. Username slot supports override (Deezer's legacy `'deezer_dl'`). Phase C1 scope: worker exists. No client migrated yet — C2-C7 migrate sources one at a time, each gated by the Phase A pinning tests so per-source contract drift fails fast. 10 new tests pin the worker contract: UUID id format, initial record shape, extra-fields merge, username override, state transitions on success / impl-returns-None / impl-raises, semaphore serialization (default + parallel), rate-limit delay between successive downloads. Suite still green (308 download tests). Pure additive.	3 weeks ago
Broque Thomas	3634dca83f	B3: Orchestrator delegates query methods to engine ... `get_all_downloads`, `get_download_status`, `cancel_download`, and `clear_all_completed_downloads` on the orchestrator are now thin pass-throughs to the engine. The plugin-iteration logic lives in one place (the engine) instead of duplicated across orchestrator methods. Source-hint routing semantics preserved verbatim — engine.cancel treats streaming-source names as direct routes and unknown names as Soulseek peer usernames, exactly like the legacy orchestrator did. Per-plugin exceptions still get swallowed defensively. Test fixture `_build_orchestrator` now constructs an engine and registers every mock plugin so the helper-built orchestrators have the same wiring as production. Suite still green (2012 passed). Zero behavior change for users.	3 weeks ago
Broque Thomas	badb5dd7de	B2: Engine owns cross-source query dispatch ... `DownloadEngine` grows async query methods that wrap plugin iteration: `get_all_downloads` (concatenates every plugin's active downloads), `get_download_status` (first plugin to recognize the id wins), `cancel_download` (with source-hint routing — streaming sources go direct, unknown hints route to Soulseek as peer username), and `clear_all_completed_downloads` (skips unconfigured plugins). Code moved from the orchestrator's hand-iterated loops into the engine. Orchestrator delegation comes in B3 — for B2 the engine methods exist but nothing calls them yet. Per-plugin behavior preserved verbatim (defensive `try ... except` swallows per-iteration, unconfigured-skip on clear, source-hint routing semantics). Phase A pinning tests + 8 new engine query tests catch any drift. Pure additive — zero behavior change for users.	3 weeks ago
Broque Thomas	f40c6d3b55	B1: Add DownloadEngine skeleton ... `core/download_engine/` package with the engine class that will own cross-source state, threading, search retry, rate-limits, and fallback chains. Orchestrator constructs an engine and registers each plugin with it. Phase B1 scope: skeleton only. Engine stores active_downloads records keyed by (source, download_id), provides thread-safe add/update/remove/iterate primitives, and holds plugin references for later phases. NOT on any code path yet — pure additive scaffolding so subsequent commits can introduce engine-driven behavior one piece at a time without a big-bang switchover. 15 new tests pin the engine's state-storage contract: shallow-copy reads, partial-patch updates, no-op-on-missing semantics, per-source iteration, id-only find, concurrent-add safety. Suite still 290 (download subset) green. Zero behavior change.	3 weeks ago
Broque Thomas	4c2fd49df2	A8: Pin LidarrDownloadClient download lifecycle behavior ... 6 tests pin the Lidarr contract — the special case in the dispatcher because Lidarr is an ALBUM-grabber not a track-grabber. Filename format is `album_foreign_id||display` (MusicBrainz album MBID Lidarr uses for lookups). State dict is SMALLER than streaming sources (no track_id, no transferred/speed — Lidarr polls its own queue API for byte-level progress). Thread target signature is 3-arg, no original_filename. Engine refactor's plugin contract must accommodate album-only sources or Lidarr stays special.	3 weeks ago
Broque Thomas	2a0d63723e	A7: Pin SoundcloudClient download lifecycle behavior ... 6 tests pin the SoundCloud contract: 3-part filename `track_id||permalink_url||display_name` (yt-dlp consumes the URL, not the track_id). Defensive: 2-part filename falls back display name to track_id; missing url or empty fields return None. Thread target signature uses URL as the second arg.	3 weeks ago