From d8437c87c6183b62db086bd6caae2030416786e5 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Sun, 3 May 2026 10:11:06 -0700 Subject: [PATCH] Fix Album Completeness Auto-Fill on Docker / shared-library setups (#476) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GitHub issue #476 (gabistek, Docker on Arch host): "Auto-Fill" / "Fix Selected" on the Album Completeness findings page returned "Could not determine album folder from existing tracks" for every album. Reproduces on any setup where the media-server library lives outside the SoulSync transfer/download folders — Docker is the headline case but native installs that point Plex at a NAS via SMB hit it too. Root cause: `core/repair_worker.py:_resolve_file_path` only probed the transfer + download folders. Docker users have their Plex/Jellyfin library bind-mounted at /music (or similar) — neither configured in SoulSync. Every existing track got silently treated as missing, so `album_folder` stayed None and the fix workflow bailed. The same incomplete logic was duplicated four more times in the repair_jobs/ modules, all with the same bug. Album Completeness was just the most user-visible — the same setups were also producing false "missing file" findings from Dead File Cleaner, silent skips in MBID Mismatch Detector, etc. The web server already had the correct logic at `web_server.py:_resolve_library_file_path` (probes transfer + download + Plex-reported library locations + user-configured library.music_paths). The repair workers had never been updated to match. Fix: - New `core/library/path_resolver.py` extracts the union logic into a single shared function `resolve_library_file_path()`. Probes (in order, deduped): explicit transfer/download kwargs, config-derived soulseek.transfer_path/download_path, Plex-reported library locations (when a plex_client is passed), user-configured library.music_paths. Each defensive: malformed config or a flaky Plex client degrades to the dirs that did succeed. - `core/repair_worker.py:_resolve_file_path` becomes a delegating wrapper preserving the legacy signature, with a new `config_manager` kwarg. All 15 in-tree call sites updated to thread `self._config_manager` through. - `core/repair_jobs/dead_file_cleaner.py`, `mbid_mismatch_detector.py`, and `lossy_converter.py` get the same treatment: duplicate function replaced with a thin wrapper, call sites pass `context.config_manager`. - `core/repair_jobs/acoustid_scanner.py` and `unknown_artist_fixer.py` (which used to import from repair_worker) now call the shared resolver directly with `context.config_manager`. Side benefit: every other repair job (Dead File Cleaner, MBID Mismatch Detector, Lossy Converter, AcoustID Scanner, Unknown Artist Fixer) also stops missing files in the media-server library mount. Single fix unblocks five user-visible features. Tests: `tests/library/test_path_resolver.py` — 20 cases covering all four base-dir sources, suffix-walk algorithm, dedup, defensive paths (None plex client, malformed config entries, raising config_manager.get, broken plex attribute access), Docker path translation. Full suite 1677 passed locally. WHATS_NEW entry under '2.4.2' dev cycle. --- core/library/path_resolver.py | 173 +++++++++ core/repair_jobs/acoustid_scanner.py | 11 +- core/repair_jobs/dead_file_cleaner.py | 35 +- core/repair_jobs/lossy_converter.py | 27 +- core/repair_jobs/mbid_mismatch_detector.py | 28 +- core/repair_jobs/unknown_artist_fixer.py | 11 +- core/repair_worker.py | 68 ++-- tests/library/test_path_resolver.py | 405 +++++++++++++++++++++ webui/static/helper.js | 1 + 9 files changed, 661 insertions(+), 98 deletions(-) create mode 100644 core/library/path_resolver.py create mode 100644 tests/library/test_path_resolver.py diff --git a/core/library/path_resolver.py b/core/library/path_resolver.py new file mode 100644 index 00000000..8e556cd8 --- /dev/null +++ b/core/library/path_resolver.py @@ -0,0 +1,173 @@ +"""Resolve database-stored file paths to actual files on disk. + +Database track rows store file paths as the media server reported them +(`/music/Artist/Album/track.flac`, `H:\\Music\\Artist\\...`, etc). When +SoulSync runs in Docker, those paths don't exist as-is inside the +container — the user's library is bind-mounted at a container path +(commonly `/music`) that has nothing to do with what Plex/Jellyfin +recorded. Same problem for native installs that point at a NAS via SMB: +the path the media server scanned isn't the path SoulSync reads. + +The resolver tries the raw path first (cheap happy-path), then walks +progressively shorter suffixes against every configured base directory: +the transfer folder, the slskd download folder, every configured Plex +library location, and every entry in the user's `library.music_paths` +config. The first existing match wins. + +This module replaces four duplicated copies of the same function (each +with the same incomplete logic) that lived in +`core/repair_worker.py` and three modules under `core/repair_jobs/`. +The duplicates only checked the transfer + download folders and +silently returned None for files in the actual media-server library — +which is why, for example, the Album Completeness "Auto-Fill" button +returned ``Could not determine album folder from existing tracks`` for +every Docker user (issue #476). + +The web server has its own near-duplicate at +``web_server.py:_resolve_library_file_path`` which already covers the +full search space; this module is the lifted, shared version usable +from any background worker. +""" + +from __future__ import annotations + +import os +from typing import Any, Iterable, Optional + +from utils.logging_config import get_logger + + +logger = get_logger("library.path_resolver") + + +def _docker_resolve_path(path_str: Any) -> Optional[str]: + """Translate Windows-style paths to the Docker container layout. + + Mirrors ``core/imports/paths.docker_resolve_path`` but kept local to + avoid a cross-package import in case this module is consumed early + in a job startup. Returns the input unchanged outside Docker. + """ + if not isinstance(path_str, str): + return None + if ( + os.path.exists("/.dockerenv") + and len(path_str) >= 3 + and path_str[1] == ":" + and path_str[0].isalpha() + ): + drive_letter = path_str[0].lower() + rest = path_str[2:].replace("\\", "/") + return f"/host/mnt/{drive_letter}{rest}" + return path_str + + +def _collect_base_dirs( + transfer_folder: Optional[str], + download_folder: Optional[str], + config_manager: Any, + plex_client: Any, +) -> list[str]: + """Build the ordered list of base directories to probe.""" + candidates: list[Optional[str]] = [] + + if transfer_folder: + candidates.append(_docker_resolve_path(transfer_folder)) + if download_folder: + candidates.append(_docker_resolve_path(download_folder)) + + if config_manager is not None: + try: + transfer_cfg = config_manager.get("soulseek.transfer_path", "") or "" + download_cfg = config_manager.get("soulseek.download_path", "") or "" + if transfer_cfg: + candidates.append(_docker_resolve_path(transfer_cfg)) + if download_cfg: + candidates.append(_docker_resolve_path(download_cfg)) + except Exception: + pass + + # Plex-reported library locations (handles "Plex scanned at /music but + # SoulSync mounts at /library" cases). + if plex_client is not None: + try: + server = getattr(plex_client, "server", None) + music_library = getattr(plex_client, "music_library", None) + if server is not None and music_library is not None: + for loc in getattr(music_library, "locations", []) or []: + if loc: + candidates.append(loc) + except Exception: + pass + + # User-configured library music paths (Settings → Library → Music Paths). + if config_manager is not None: + try: + music_paths = config_manager.get("library.music_paths", []) or [] + if isinstance(music_paths, Iterable): + for p in music_paths: + if isinstance(p, str) and p.strip(): + candidates.append(_docker_resolve_path(p.strip())) + except Exception: + pass + + # De-duplicate while preserving order, drop empties / non-existent dirs. + seen: set[str] = set() + out: list[str] = [] + for c in candidates: + if not c or c in seen: + continue + seen.add(c) + if os.path.isdir(c): + out.append(c) + return out + + +def resolve_library_file_path( + file_path: Any, + *, + transfer_folder: Optional[str] = None, + download_folder: Optional[str] = None, + config_manager: Any = None, + plex_client: Any = None, +) -> Optional[str]: + """Resolve a stored DB path to an actual file on disk. + + Args: + file_path: The path as recorded in the database (may not exist + as-is in the current process's filesystem view). + transfer_folder: Optional explicit transfer-folder override + (bypasses the config_manager lookup). Useful when the caller + already cached one. + download_folder: Optional explicit download-folder override. + config_manager: When provided, the resolver also pulls + ``soulseek.transfer_path``, ``soulseek.download_path``, and + ``library.music_paths`` from config to expand the search. + plex_client: When provided, every Plex-reported music-library + location is added to the search. + + Returns: + The first existing path on disk, or None when no match is found. + Never raises — failure is the None return. + """ + if not isinstance(file_path, str) or not file_path: + return None + + if os.path.exists(file_path): + return file_path + + path_parts = file_path.replace("\\", "/").split("/") + base_dirs = _collect_base_dirs(transfer_folder, download_folder, config_manager, plex_client) + if not base_dirs: + return None + + # Skip index 0 to avoid drive-letter / leading-slash artifacts + # (e.g. "E:" or "" from a leading "/"). + for base in base_dirs: + for i in range(1, len(path_parts)): + candidate = os.path.join(base, *path_parts[i:]) + if os.path.exists(candidate): + return candidate + return None + + +__all__ = ["resolve_library_file_path"] diff --git a/core/repair_jobs/acoustid_scanner.py b/core/repair_jobs/acoustid_scanner.py index 2f27f741..60482a1d 100644 --- a/core/repair_jobs/acoustid_scanner.py +++ b/core/repair_jobs/acoustid_scanner.py @@ -289,9 +289,14 @@ class AcoustIDScannerJob(RepairJob): return None if os.path.exists(file_path): return file_path - # Try the repair_worker's resolver - from core.repair_worker import _resolve_file_path - return _resolve_file_path(file_path, context.transfer_folder) + # Use the shared library-path resolver — picks up + # library.music_paths and Plex library locations too. + from core.library.path_resolver import resolve_library_file_path + return resolve_library_file_path( + file_path, + transfer_folder=context.transfer_folder, + config_manager=context.config_manager, + ) def _save_checkpoint_id(self, context: JobContext, track_id): """Save or clear the scan checkpoint by track ID.""" diff --git a/core/repair_jobs/dead_file_cleaner.py b/core/repair_jobs/dead_file_cleaner.py index de87758b..39c60742 100644 --- a/core/repair_jobs/dead_file_cleaner.py +++ b/core/repair_jobs/dead_file_cleaner.py @@ -2,6 +2,7 @@ import os +from core.library.path_resolver import resolve_library_file_path from core.repair_jobs import register_job from core.repair_jobs.base import JobContext, JobResult, RepairJob from utils.logging_config import get_logger @@ -9,29 +10,14 @@ from utils.logging_config import get_logger logger = get_logger("repair_job.dead_files") -def _resolve_file_path(file_path, transfer_folder, download_folder=None): - """Resolve a stored DB path to an actual file on disk. - - Mirrors _resolve_library_file_path from web_server.py — tries the raw - path first, then progressively shorter suffixes against configured dirs. - """ - if not file_path: - return None - if os.path.exists(file_path): - return file_path - - path_parts = file_path.replace('\\', '/').split('/') - - for base_dir in [transfer_folder, download_folder]: - if not base_dir or not os.path.isdir(base_dir): - continue - # Skip index 0 to avoid drive letter issues (e.g. E:) - for i in range(1, len(path_parts)): - candidate = os.path.join(base_dir, *path_parts[i:]) - if os.path.exists(candidate): - return candidate - - return None +def _resolve_file_path(file_path, transfer_folder, download_folder=None, config_manager=None): + """Backwards-compat wrapper. Use ``resolve_library_file_path`` directly.""" + return resolve_library_file_path( + file_path, + transfer_folder=transfer_folder, + download_folder=download_folder, + config_manager=config_manager, + ) @register_job @@ -122,7 +108,8 @@ class DeadFileCleanerJob(RepairJob): ) # Use the same path resolution logic as library playback - resolved = _resolve_file_path(file_path, context.transfer_folder, download_folder) + resolved = _resolve_file_path(file_path, context.transfer_folder, download_folder, + config_manager=context.config_manager) if resolved is None: # File is truly missing — create finding diff --git a/core/repair_jobs/lossy_converter.py b/core/repair_jobs/lossy_converter.py index 3d9e12ef..1c577f98 100644 --- a/core/repair_jobs/lossy_converter.py +++ b/core/repair_jobs/lossy_converter.py @@ -7,6 +7,7 @@ ffmpeg with the user's configured codec/bitrate settings. import os +from core.library.path_resolver import resolve_library_file_path from core.repair_jobs import register_job from core.repair_jobs.base import JobContext, JobResult, RepairJob from utils.logging_config import get_logger @@ -20,21 +21,14 @@ CODEC_MAP = { } -def _resolve_file_path(file_path, transfer_folder, download_folder=None): - """Resolve a stored DB path to an actual file on disk.""" - if not file_path: - return None - if os.path.exists(file_path): - return file_path - path_parts = file_path.replace('\\', '/').split('/') - for base_dir in [transfer_folder, download_folder]: - if not base_dir or not os.path.isdir(base_dir): - continue - for i in range(1, len(path_parts)): - candidate = os.path.join(base_dir, *path_parts[i:]) - if os.path.exists(candidate): - return candidate - return None +def _resolve_file_path(file_path, transfer_folder, download_folder=None, config_manager=None): + """Backwards-compat wrapper. Use ``resolve_library_file_path`` directly.""" + return resolve_library_file_path( + file_path, + transfer_folder=transfer_folder, + download_folder=download_folder, + config_manager=config_manager, + ) @register_job @@ -136,7 +130,8 @@ class LossyConverterJob(RepairJob): ) # Resolve path - resolved = _resolve_file_path(file_path, context.transfer_folder, download_folder) + resolved = _resolve_file_path(file_path, context.transfer_folder, download_folder, + config_manager=context.config_manager) if not resolved or not os.path.exists(resolved): continue diff --git a/core/repair_jobs/mbid_mismatch_detector.py b/core/repair_jobs/mbid_mismatch_detector.py index 8d24835a..e8aa476e 100644 --- a/core/repair_jobs/mbid_mismatch_detector.py +++ b/core/repair_jobs/mbid_mismatch_detector.py @@ -10,6 +10,7 @@ SoulSync shows them correctly. import os from difflib import SequenceMatcher +from core.library.path_resolver import resolve_library_file_path from core.repair_jobs import register_job from core.repair_jobs.base import JobContext, JobResult, RepairJob from utils.logging_config import get_logger @@ -165,22 +166,14 @@ def _remove_mbid_from_file(file_path): return False -def _resolve_file_path(file_path, transfer_folder, download_folder=None): - """Resolve a stored DB path to an actual file on disk.""" - if not file_path: - return None - if os.path.exists(file_path): - return file_path - - path_parts = file_path.replace('\\', '/').split('/') - for base_dir in [transfer_folder, download_folder]: - if not base_dir or not os.path.isdir(base_dir): - continue - for i in range(1, len(path_parts)): - candidate = os.path.join(base_dir, *path_parts[i:]) - if os.path.exists(candidate): - return candidate - return None +def _resolve_file_path(file_path, transfer_folder, download_folder=None, config_manager=None): + """Backwards-compat wrapper. Use ``resolve_library_file_path`` directly.""" + return resolve_library_file_path( + file_path, + transfer_folder=transfer_folder, + download_folder=download_folder, + config_manager=config_manager, + ) @register_job @@ -280,7 +273,8 @@ class MbidMismatchDetectorJob(RepairJob): context.update_progress(i + 1, total) # Resolve the file path - resolved = _resolve_file_path(file_path, context.transfer_folder, download_folder) + resolved = _resolve_file_path(file_path, context.transfer_folder, download_folder, + config_manager=context.config_manager) if not resolved: result.scanned += 1 continue diff --git a/core/repair_jobs/unknown_artist_fixer.py b/core/repair_jobs/unknown_artist_fixer.py index 6ceb42ab..5d2b5bba 100644 --- a/core/repair_jobs/unknown_artist_fixer.py +++ b/core/repair_jobs/unknown_artist_fixer.py @@ -135,9 +135,14 @@ class UnknownArtistFixerJob(RepairJob): title = track['title'] or '' file_path = track['file_path'] - # Resolve actual file on disk - from core.repair_worker import _resolve_file_path - resolved = _resolve_file_path(file_path, transfer) + # Resolve actual file on disk via the shared library resolver + # (picks up library.music_paths + Plex library locations). + from core.library.path_resolver import resolve_library_file_path + resolved = resolve_library_file_path( + file_path, + transfer_folder=transfer, + config_manager=context.config_manager, + ) if not resolved or not os.path.exists(resolved): result.skipped += 1 continue diff --git a/core/repair_worker.py b/core/repair_worker.py index 799be10e..23bdc228 100644 --- a/core/repair_worker.py +++ b/core/repair_worker.py @@ -24,6 +24,7 @@ from core.metadata_service import ( get_source_priority, get_primary_source, ) +from core.library.path_resolver import resolve_library_file_path from core.repair_jobs import get_all_jobs from core.repair_jobs.base import JobContext, JobResult, RepairJob from utils.logging_config import get_logger @@ -33,29 +34,26 @@ logger = get_logger("repair_worker") AUDIO_EXTENSIONS = {'.mp3', '.flac', '.ogg', '.opus', '.m4a', '.aac', '.wav', '.wma', '.aiff', '.aif'} -def _resolve_file_path(file_path, transfer_folder, download_folder=None): +def _resolve_file_path(file_path, transfer_folder, download_folder=None, + config_manager=None, plex_client=None): """Resolve a stored DB path to an actual file on disk. - Tries the raw path first, then progressively shorter suffixes against - configured directories. Handles cross-environment path mismatches - (e.g. Docker paths vs native Windows paths). + Thin wrapper around ``core.library.path_resolver.resolve_library_file_path`` + that preserves the legacy signature used by every caller in this module + and the repair-job modules. The shared resolver also probes the + user-configured ``library.music_paths`` and Plex-reported library + locations — which is what fixes the Album Completeness Auto-Fill + failure on Docker setups (issue #476). Pre-existing call sites that + don't pass ``config_manager`` keep the old transfer+download-only + behavior; sites that pass it in pick up the wider search automatically. """ - if not file_path: - return None - if os.path.exists(file_path): - return file_path - - path_parts = file_path.replace('\\', '/').split('/') - - for base_dir in [transfer_folder, download_folder]: - if not base_dir or not os.path.isdir(base_dir): - continue - for i in range(1, len(path_parts)): - candidate = os.path.join(base_dir, *path_parts[i:]) - if os.path.exists(candidate): - return candidate - - return None + return resolve_library_file_path( + file_path, + transfer_folder=transfer_folder, + download_folder=download_folder, + config_manager=config_manager, + plex_client=plex_client, + ) class RepairWorker: @@ -1022,7 +1020,7 @@ class RepairWorker: download_folder = None if self._config_manager: download_folder = self._config_manager.get('soulseek.download_path', '') - resolved = _resolve_file_path(file_path, self.transfer_folder, download_folder) or file_path + resolved = _resolve_file_path(file_path, self.transfer_folder, download_folder, config_manager=self._config_manager) or file_path if not os.path.exists(resolved): return {'success': True, 'action': 'already_gone', @@ -1100,7 +1098,7 @@ class RepairWorker: download_folder = None if self._config_manager: download_folder = self._config_manager.get('soulseek.download_path', '') - resolved = _resolve_file_path(file_path, self.transfer_folder, download_folder) or file_path + resolved = _resolve_file_path(file_path, self.transfer_folder, download_folder, config_manager=self._config_manager) or file_path if not os.path.isfile(resolved): return {'success': False, 'error': f'File not found: {os.path.basename(file_path)}'} @@ -1285,7 +1283,7 @@ class RepairWorker: files_deleted = 0 for fpath in remove_paths: try: - resolved = _resolve_file_path(fpath, self.transfer_folder, download_folder) + resolved = _resolve_file_path(fpath, self.transfer_folder, download_folder, config_manager=self._config_manager) if resolved and os.path.exists(resolved): os.remove(resolved) files_deleted += 1 @@ -1346,7 +1344,7 @@ class RepairWorker: if self._config_manager: download_folder = self._config_manager.get('soulseek.download_path', '') try: - resolved = _resolve_file_path(single_path, self.transfer_folder, download_folder) + resolved = _resolve_file_path(single_path, self.transfer_folder, download_folder, config_manager=self._config_manager) if resolved and os.path.exists(resolved): os.remove(resolved) file_deleted = True @@ -1414,7 +1412,7 @@ class RepairWorker: if self._config_manager: download_folder = self._config_manager.get('soulseek.download_path', '') try: - resolved = _resolve_file_path(track_path, self.transfer_folder, download_folder) + resolved = _resolve_file_path(track_path, self.transfer_folder, download_folder, config_manager=self._config_manager) if resolved and os.path.exists(resolved): os.remove(resolved) file_deleted = True @@ -1453,7 +1451,7 @@ class RepairWorker: # Resolve file download_folder = self._config_manager.get('soulseek.download_path', '') if self._config_manager else '' - resolved = _resolve_file_path(file_path, self.transfer_folder, download_folder) if file_path else None + resolved = _resolve_file_path(file_path, self.transfer_folder, download_folder, config_manager=self._config_manager) if file_path else None if not resolved or not os.path.exists(resolved): return {'success': False, 'error': f'File not found: {file_path}'} @@ -1560,7 +1558,7 @@ class RepairWorker: if fix_action == 'delete': # Delete file + DB record if file_path: - resolved = _resolve_file_path(file_path, self.transfer_folder) + resolved = _resolve_file_path(file_path, self.transfer_folder, config_manager=self._config_manager) if resolved and os.path.exists(resolved): try: os.remove(resolved) @@ -1602,7 +1600,7 @@ class RepairWorker: logger.warning("Could not add to wishlist: %s", e) # Delete wrong file if file_path: - resolved = _resolve_file_path(file_path, self.transfer_folder) + resolved = _resolve_file_path(file_path, self.transfer_folder, config_manager=self._config_manager) if resolved and os.path.exists(resolved): try: os.remove(resolved) @@ -1661,7 +1659,7 @@ class RepairWorker: # Write corrected tags to the actual audio file if file_path: - resolved = _resolve_file_path(file_path, self.transfer_folder) + resolved = _resolve_file_path(file_path, self.transfer_folder, config_manager=self._config_manager) if resolved and os.path.exists(resolved): try: from core.tag_writer import write_tags_to_file @@ -1685,7 +1683,7 @@ class RepairWorker: download_folder = None if self._config_manager: download_folder = self._config_manager.get('soulseek.download_path', '') - resolved = _resolve_file_path(file_path, self.transfer_folder, download_folder) + resolved = _resolve_file_path(file_path, self.transfer_folder, download_folder, config_manager=self._config_manager) if not resolved or not os.path.exists(resolved): return {'success': False, 'error': f'File not found: {file_path}'} @@ -1731,7 +1729,7 @@ class RepairWorker: download_folder = None if self._config_manager: download_folder = self._config_manager.get('soulseek.download_path', '') - resolved = _resolve_file_path(track_file, self.transfer_folder, download_folder) + resolved = _resolve_file_path(track_file, self.transfer_folder, download_folder, config_manager=self._config_manager) if not resolved or not os.path.exists(resolved): continue @@ -1877,7 +1875,7 @@ class RepairWorker: album_folder = None for t in existing_tracks: - resolved = _resolve_file_path(t.file_path, self.transfer_folder, download_folder) + resolved = _resolve_file_path(t.file_path, self.transfer_folder, download_folder, config_manager=self._config_manager) if resolved and os.path.exists(resolved): album_folder = os.path.dirname(resolved) break @@ -1888,7 +1886,7 @@ class RepairWorker: # Detect filename pattern resolved_paths = [] for t in existing_tracks: - rp = _resolve_file_path(t.file_path, self.transfer_folder, download_folder) + rp = _resolve_file_path(t.file_path, self.transfer_folder, download_folder, config_manager=self._config_manager) if rp: resolved_paths.append(rp) filename_pattern = self._detect_filename_pattern(resolved_paths) @@ -2145,7 +2143,7 @@ class RepairWorker: """Move or copy a candidate track into the album folder and update DB.""" try: # Resolve source file - src_path = _resolve_file_path(candidate.file_path, self.transfer_folder, download_folder) + src_path = _resolve_file_path(candidate.file_path, self.transfer_folder, download_folder, config_manager=self._config_manager) if not src_path or not os.path.exists(src_path): return {'success': False, 'error': f'Source file not found: {candidate.file_path}'} @@ -2534,7 +2532,7 @@ class RepairWorker: download_folder = None if self._config_manager: download_folder = self._config_manager.get('soulseek.download_path', '') - resolved = _resolve_file_path(file_path, self.transfer_folder, download_folder) or file_path + resolved = _resolve_file_path(file_path, self.transfer_folder, download_folder, config_manager=self._config_manager) or file_path if not os.path.exists(resolved): return {'success': False, 'error': f'Source file not found: {file_path}'} diff --git a/tests/library/test_path_resolver.py b/tests/library/test_path_resolver.py new file mode 100644 index 00000000..9c32e9e3 --- /dev/null +++ b/tests/library/test_path_resolver.py @@ -0,0 +1,405 @@ +"""Regression tests for ``core/library/path_resolver.py``. + +GitHub issue #476 (gabistek, Docker / Arch host): Album Completeness +Auto-Fill returned ``Could not determine album folder from existing +tracks`` for every album. Root cause: the repair worker's path +resolver only probed the transfer + download folders, not the +user-configured ``library.music_paths`` or Plex-reported library +locations. Files lived in the media-server library mount and got +silently treated as missing. + +These tests pin the resolver's behavior across the four base-dir +sources (explicit transfer, explicit download, config-driven library +paths, Plex client locations), the suffix-walk algorithm, and the +defensive return-None paths. +""" + +from __future__ import annotations + +import os +from pathlib import Path +from types import SimpleNamespace +from unittest.mock import MagicMock + +import pytest + +from core.library import path_resolver +from core.library.path_resolver import resolve_library_file_path + + +# --------------------------------------------------------------------------- +# Defensive boundary cases +# --------------------------------------------------------------------------- + + +def test_returns_none_for_empty_path() -> None: + assert resolve_library_file_path("") is None + assert resolve_library_file_path(None) is None # type: ignore[arg-type] + + +def test_returns_none_when_no_base_dirs_configured(tmp_path: Path) -> None: + """No transfer, no download, no config, no Plex → can't resolve.""" + fake = tmp_path / "non_existent.flac" + assert resolve_library_file_path(str(fake)) is None + + +def test_returns_raw_path_when_it_exists(tmp_path: Path) -> None: + """Happy path — the raw stored path resolves directly.""" + real = tmp_path / "track.flac" + real.write_bytes(b"audio") + + result = resolve_library_file_path(str(real)) + + assert result == str(real) + + +# --------------------------------------------------------------------------- +# Transfer / download base dirs (legacy behavior preserved) +# --------------------------------------------------------------------------- + + +def test_finds_file_via_transfer_folder_suffix_walk(tmp_path: Path) -> None: + """DB stored path is `/music/Artist/Album/track.flac` but the file + actually lives in `/Artist/Album/track.flac`.""" + transfer = tmp_path / "Transfer" + (transfer / "Artist" / "Album").mkdir(parents=True) + actual = transfer / "Artist" / "Album" / "track.flac" + actual.write_bytes(b"audio") + + result = resolve_library_file_path( + "/music/Artist/Album/track.flac", + transfer_folder=str(transfer), + ) + + assert result == str(actual) + + +def test_finds_file_via_download_folder_when_transfer_misses(tmp_path: Path) -> None: + transfer = tmp_path / "Transfer" + transfer.mkdir() + download = tmp_path / "Downloads" + (download / "Artist" / "Album").mkdir(parents=True) + actual = download / "Artist" / "Album" / "track.flac" + actual.write_bytes(b"audio") + + result = resolve_library_file_path( + "/music/Artist/Album/track.flac", + transfer_folder=str(transfer), + download_folder=str(download), + ) + + assert result == str(actual) + + +def test_handles_windows_backslash_paths(tmp_path: Path) -> None: + """DB stored paths can be Windows-style with backslashes — the + walker normalizes them to forward slashes before splitting.""" + transfer = tmp_path / "Transfer" + (transfer / "Artist" / "Album").mkdir(parents=True) + actual = transfer / "Artist" / "Album" / "track.flac" + actual.write_bytes(b"audio") + + result = resolve_library_file_path( + r"H:\Music\Artist\Album\track.flac", + transfer_folder=str(transfer), + ) + + assert result == str(actual) + + +# --------------------------------------------------------------------------- +# Library music paths from config (the fix for #476) +# --------------------------------------------------------------------------- + + +def test_finds_file_via_library_music_paths(tmp_path: Path) -> None: + """The Plex/Jellyfin library at /Artist/Album/track.flac + is found via the user's configured ``library.music_paths`` even + when transfer + download don't have it.""" + transfer = tmp_path / "Transfer" + transfer.mkdir() + library = tmp_path / "Library" + (library / "Artist" / "Album").mkdir(parents=True) + actual = library / "Artist" / "Album" / "track.flac" + actual.write_bytes(b"audio") + + cm = MagicMock() + cm.get.side_effect = lambda key, default=None: { + "library.music_paths": [str(library)], + "soulseek.transfer_path": "", + "soulseek.download_path": "", + }.get(key, default) + + result = resolve_library_file_path( + "/data/music/Artist/Album/track.flac", + transfer_folder=str(transfer), + config_manager=cm, + ) + + assert result == str(actual) + + +def test_library_music_paths_handles_multiple_dirs(tmp_path: Path) -> None: + """Users can configure multiple music paths — first existing + suffix-match wins.""" + lib_a = tmp_path / "LibA" + lib_b = tmp_path / "LibB" + (lib_a / "Artist").mkdir(parents=True) + (lib_b / "Artist" / "Album").mkdir(parents=True) + # Only LibB has the actual file + actual = lib_b / "Artist" / "Album" / "track.flac" + actual.write_bytes(b"audio") + + cm = MagicMock() + cm.get.side_effect = lambda key, default=None: { + "library.music_paths": [str(lib_a), str(lib_b)], + }.get(key, default) + + result = resolve_library_file_path( + "/x/Artist/Album/track.flac", + config_manager=cm, + ) + + assert result == str(actual) + + +def test_skips_non_string_entries_in_music_paths(tmp_path: Path) -> None: + """Defensive: malformed config (None, int, dict in the list) must + not crash the resolver.""" + library = tmp_path / "Library" + (library / "Artist").mkdir(parents=True) + actual = library / "Artist" / "track.flac" + actual.write_bytes(b"audio") + + cm = MagicMock() + cm.get.side_effect = lambda key, default=None: { + "library.music_paths": [None, 42, {"x": 1}, str(library), ""], + }.get(key, default) + + result = resolve_library_file_path( + "/x/Artist/track.flac", + config_manager=cm, + ) + + assert result == str(actual) + + +def test_strips_whitespace_from_music_paths(tmp_path: Path) -> None: + """Trailing whitespace on a config value (common copy-paste mistake) + shouldn't break resolution.""" + library = tmp_path / "Library" + (library / "Artist").mkdir(parents=True) + actual = library / "Artist" / "track.flac" + actual.write_bytes(b"audio") + + cm = MagicMock() + cm.get.side_effect = lambda key, default=None: { + "library.music_paths": [f" {library} "], + }.get(key, default) + + result = resolve_library_file_path( + "/x/Artist/track.flac", + config_manager=cm, + ) + + assert result == str(actual) + + +# --------------------------------------------------------------------------- +# Plex-reported library locations +# --------------------------------------------------------------------------- + + +def test_finds_file_via_plex_library_location(tmp_path: Path) -> None: + """When SoulSync mounts the Plex library at a different path than + Plex itself reports, the Plex-reported location is added to the + search and the file is found.""" + plex_loc = tmp_path / "PlexLibrary" + (plex_loc / "Artist" / "Album").mkdir(parents=True) + actual = plex_loc / "Artist" / "Album" / "track.flac" + actual.write_bytes(b"audio") + + plex_client = SimpleNamespace( + server=SimpleNamespace(), # truthy + music_library=SimpleNamespace(locations=[str(plex_loc)]), + ) + + result = resolve_library_file_path( + "/music/Artist/Album/track.flac", + plex_client=plex_client, + ) + + assert result == str(actual) + + +def test_handles_plex_client_without_server(tmp_path: Path) -> None: + """Plex client with no `server` attribute (uninitialized) shouldn't + crash — just skip the Plex source.""" + plex_client = SimpleNamespace(server=None, music_library=None) + + # No other sources configured → returns None, no exception. + assert resolve_library_file_path( + "/x/track.flac", + plex_client=plex_client, + ) is None + + +def test_handles_plex_locations_attribute_missing(tmp_path: Path) -> None: + """Plex music_library object without a `locations` attribute → skip.""" + plex_client = SimpleNamespace( + server=SimpleNamespace(), + music_library=SimpleNamespace(), # no `locations` + ) + + assert resolve_library_file_path( + "/x/track.flac", + plex_client=plex_client, + ) is None + + +# --------------------------------------------------------------------------- +# Source ordering & deduplication +# --------------------------------------------------------------------------- + + +def test_dedupe_avoids_duplicate_probes(tmp_path: Path) -> None: + """If transfer == library_paths[0], the dir is only probed once. + Resolver still finds the file.""" + shared = tmp_path / "Shared" + (shared / "Artist").mkdir(parents=True) + actual = shared / "Artist" / "track.flac" + actual.write_bytes(b"audio") + + cm = MagicMock() + cm.get.side_effect = lambda key, default=None: { + "soulseek.transfer_path": str(shared), + "library.music_paths": [str(shared)], + }.get(key, default) + + result = resolve_library_file_path( + "/x/Artist/track.flac", + transfer_folder=str(shared), + config_manager=cm, + ) + + assert result == str(actual) + + +def test_explicit_transfer_takes_priority_over_config(tmp_path: Path) -> None: + """Explicit transfer kwarg is added before config-derived paths so + the worker's already-cached transfer_folder always wins ties.""" + explicit = tmp_path / "Explicit" + config_dir = tmp_path / "FromConfig" + (explicit / "Artist").mkdir(parents=True) + (config_dir / "Artist").mkdir(parents=True) + actual_explicit = explicit / "Artist" / "track.flac" + actual_config = config_dir / "Artist" / "track.flac" + actual_explicit.write_bytes(b"explicit") + actual_config.write_bytes(b"config") + + cm = MagicMock() + cm.get.side_effect = lambda key, default=None: { + "soulseek.transfer_path": str(config_dir), + }.get(key, default) + + result = resolve_library_file_path( + "/x/Artist/track.flac", + transfer_folder=str(explicit), + config_manager=cm, + ) + + assert result == str(actual_explicit) + + +# --------------------------------------------------------------------------- +# Failure paths from external dependencies don't crash +# --------------------------------------------------------------------------- + + +def test_config_manager_get_raising_does_not_crash(tmp_path: Path) -> None: + """A flaky config_manager.get raising shouldn't break resolution. + The resolver swallows it and continues with the explicit dirs.""" + transfer = tmp_path / "Transfer" + (transfer / "Artist").mkdir(parents=True) + actual = transfer / "Artist" / "track.flac" + actual.write_bytes(b"audio") + + cm = MagicMock() + cm.get.side_effect = RuntimeError("config blew up") + + result = resolve_library_file_path( + "/x/Artist/track.flac", + transfer_folder=str(transfer), + config_manager=cm, + ) + + assert result == str(actual) + + +def test_plex_client_attribute_access_raising_does_not_crash(tmp_path: Path) -> None: + """A Plex client whose attribute access raises shouldn't break + resolution — fallback to other sources.""" + transfer = tmp_path / "Transfer" + (transfer / "Artist").mkdir(parents=True) + actual = transfer / "Artist" / "track.flac" + actual.write_bytes(b"audio") + + class _BrokenPlex: + @property + def server(self): + raise RuntimeError("plex disconnected") + + result = resolve_library_file_path( + "/x/Artist/track.flac", + transfer_folder=str(transfer), + plex_client=_BrokenPlex(), + ) + + assert result == str(actual) + + +def test_returns_none_when_no_suffix_matches(tmp_path: Path) -> None: + """When the file genuinely doesn't exist anywhere, return None + cleanly. Don't false-match an unrelated file.""" + transfer = tmp_path / "Transfer" + (transfer / "Artist" / "Album").mkdir(parents=True) + # Create a different file in the right tree + (transfer / "Artist" / "Album" / "different.flac").write_bytes(b"x") + + result = resolve_library_file_path( + "/x/Artist/Album/missing.flac", + transfer_folder=str(transfer), + ) + + assert result is None + + +# --------------------------------------------------------------------------- +# docker_resolve_path internal helper +# --------------------------------------------------------------------------- + + +def test_docker_resolve_path_translates_windows_paths_inside_docker(monkeypatch) -> None: + """Inside Docker, ``H:\\Music\\track.flac`` becomes + ``/host/mnt/h/Music/track.flac`` so the bind-mounted host drive + can be reached. Outside Docker, paths are returned unchanged.""" + real_exists = os.path.exists + + def _fake_exists(p): + if p == "/.dockerenv": + return True + return real_exists(p) + + monkeypatch.setattr(os.path, "exists", _fake_exists) + assert path_resolver._docker_resolve_path("H:\\Music\\track.flac") == "/host/mnt/h/Music/track.flac" + + # Non-Windows-style path passes through unchanged inside Docker too. + assert path_resolver._docker_resolve_path("/data/music") == "/data/music" + + +def test_docker_resolve_path_pass_through_outside_docker(monkeypatch) -> None: + """Outside Docker (no /.dockerenv), Windows paths are unchanged.""" + real_exists = os.path.exists + monkeypatch.setattr(os.path, "exists", + lambda p: False if p == "/.dockerenv" else real_exists(p)) + assert path_resolver._docker_resolve_path("H:\\Music\\track.flac") == "H:\\Music\\track.flac" diff --git a/webui/static/helper.js b/webui/static/helper.js index 8cc5549e..9e38fc6d 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3452,6 +3452,7 @@ const WHATS_NEW = { { title: 'Auto-Import: Live Per-Track Progress in History', desc: 'dropping an album into the staging folder used to leave the auto-import history blank for the entire processing window — sometimes 5+ minutes for a full album — because the database row only got written after every track was post-processed. now an in-progress row gets inserted up-front (status=processing) the moment processing starts, then updated to completed/failed when done. the status indicator + progress bar show "processing speak now — track 3/14: mine", and the history card itself gets a pulsing "Processing" badge, swaps its meta line to "track 3/14: mine", and highlights the currently-processing row in the expanded track list (with prior tracks dimmed as done). one row per album, not per track, so the history list stays clean.', page: 'import' }, { title: 'Reject Broken Files from slskd Before Tagging', desc: 'slskd sometimes reports a download as complete when the file is actually broken — truncated transfer, corrupted FLAC frames, or the wrong file matched on a similar filename. those slipped through into the library and surfaced as "song plays for 5 seconds and stops" or "track shows the wrong duration in plex." now every download gets a fast integrity check after the file stabilizes but before tagging / library sync: file size sanity (catches 0-byte and stub transfers), mutagen parse (catches header damage and wrong-format-with-right-extension cases), and duration agreement against the metadata source\'s expected length within a 3-second tolerance (5s for tracks over 10 minutes). failed files get quarantined to `ss_quarantine/` with a JSON sidecar explaining the failure, and the download slot is freed so a retry from another candidate can run.', page: 'downloads' }, { title: 'Auto-Import: Multi-Disc Albums + Featured-Artist Tag Handling', desc: 'two longstanding auto-import gaps that surfaced when a kendrick lamar deluxe rip got dropped into staging. (1) folders containing only `Disc 1/`, `Disc 2/` subfolders (no loose audio at the parent level) used to be invisible to the scanner — disc folders were only attached to a parent when the parent had its own loose tracks. now scanner treats a parent of disc-only subfolders as the album candidate. (2) tag identification grouped files by `(album, artist)` — but per-track artist often varies on albums with features ("kendrick lamar" vs "kendrick lamar, drake" vs "kendrick lamar, dr. dre"), which fragmented the consensus and rejected real albums. now groups by album first, picks the dominant artist within that album group; also prefers `albumartist` tag over per-track `artist` since the former is the album-level identity. as a defensive bonus, when the staging folder itself becomes the candidate (raw disc folders dropped at the root with no album wrapper), the folder-name fallback gets skipped — the name "Staging" was matching against random albums in the metadata source.', page: 'import' }, + { title: 'Album Completeness Auto-Fill Works on Docker / Shared Library Setups', desc: 'github issue #476 (gabistek): the "auto-fill" button on the album completeness findings page returned `Could not determine album folder from existing tracks` for every album on docker setups (and any setup where the media-server library lives somewhere other than the soulsync transfer/download folders). cause: the repair worker\'s path resolver only probed the transfer + download folders, ignoring the user-configured `library.music_paths` and the plex-reported library locations. that missing search space meant docker users — whose plex/jellyfin library is bind-mounted at `/music` while soulsync\'s transfer is at `/transfer` — got silent "file not found" results for every existing track. extracted the full resolver (with library + plex sources) into a shared `core/library/path_resolver.py` and wired it into all five repair-worker call paths plus the four jobs that had their own incomplete copy. side benefit: every other repair job (dead file cleaner, mbid mismatch detector, lossy converter, acoustid scanner, unknown artist fixer) also stops missing files in the media-server library mount.', page: 'library' }, { title: 'Sidebar Library Button Shows Artist Breadcrumb', desc: 'when you open an artist detail page (from library, search, or the global search popover), the sidebar Library button now lights up and rewrites its label to "Library / Artist Name" — long names truncate with an ellipsis and the full name shows on hover. revertes to plain "Library" when you leave. purely visual, no functionality change.', page: 'library' }, { title: 'Enrichment Bubble Routes Consolidated', desc: 'internal — every dashboard enrichment bubble (musicbrainz, spotify, itunes, deezer, discogs, audiodb, lastfm, genius, tidal, qobuz) used to hit its own per-service status / pause / resume route in web_server.py. unified them under a single registry-driven endpoint set: /api/enrichment//. spotify\'s rate-limit guard, lastfm/genius yield-override behavior, and tidal/qobuz extra status fields are encoded as data on the registry. 27 new tests cover the registry behavior.' }, { title: 'Drop Old Per-Service Enrichment Routes', desc: 'internal — followup to the registry consolidation. now that the dashboard has cut over to /api/enrichment//, deleted the 30 hand-rolled per-service routes from web_server.py (musicbrainz/audiodb/discogs/deezer/spotify/itunes/lastfm/genius/tidal/qobuz status+pause+resume). ~510 lines gone from the monolith, no behavior change.' },