From f976a6da5390380b189937aeb2a1aabd6f8bd8fb Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Wed, 27 May 2026 21:20:37 -0700 Subject: [PATCH] Fix: Soulseek album-bundle downloads stuck on "failed" after slskd finished the release (#715) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Symptom (user @pavelcreates / @IamGroot60 on 2.6.2): - Click Download on an album in the search modal - slskd starts + completes every track of the release - 22+ minutes after the last completed download, batch flips to "failed" with no clear log line explaining why - Per-track Soulseek downloads on the same machine were fine Root cause: ``core/soulseek_client._resolve_downloaded_album_file`` probed three hard-coded candidate paths to locate each downloaded file in the slskd download dir: candidates = [ download_path / remote_filename, download_path / basename, download_path / *normalized_path_parts, ] On the common slskd config ``directories.downloads.username = true`` slskd writes files at ``//`` — none of the three candidates carry a username segment, so the resolver returned None for every file even though the file was physically present in a subdir one level deeper. ``_poll_album _bundle_downloads`` saw 0 completed_paths, kept spinning, and hit the master deadline (~30 min) before bailing the batch. Why per-track worked: ``web_server._find_completed_file_robust`` already does a recursive walk-by-basename + path-confirm against the remote directory components, so any layout slskd writes ends up resolved. The bundle path didn't go through it. Fix - Lifted the robust finder into ``core/downloads/file_finder.py`` as a pure function ``find_completed_audio_file(download_dir, api_filename, transfer_dir=None) -> (path, location)``. Zero globals; recursive walk; handles slskd dedup suffix ``_<10+digit-timestamp>``, YouTube / Tidal ``id||title`` encoded filenames, the AcoustID-quarantine subdir skip, basename collisions disambiguated by remote-path components, and a fuzzy-basename fallback above 0.85. - ``_resolve_downloaded_album_file`` keeps the three-candidate fast path (cheap probe for the slskd-flat default) but now delegates to the new helper when none hit, instead of giving up. - ``_poll_album_bundle_downloads`` tracks "slskd reports Completed but local resolver returns None" per key. When every remaining key has been in that state past a 45-second grace window, the poll exits early with an explicit error pointing at the likely ``soulseek.download_path`` mismatch instead of silently spinning until the master deadline. - ``web_server._find_completed_file_robust`` becomes a thin delegate so both callers share one finder. Legacy inline impl kept as ``_find_completed_file_robust_legacy`` for reference; to be removed next release. - Fixed misleading ``"(0 tracks, quality=)"`` log on the preflight- reuse path — was reading attrs off a None ``picked`` object. Tests (17 new in tests/downloads/test_file_finder.py) - Flat slskd layout - Username-prefixed (the #715 case) - Full remote tree preserved - Deeply nested username + tree - File genuinely missing returns None - Basename collision disambiguated by remote dirs - Single basename match wins regardless of dirs - slskd dedup suffix match - Short ``_`` (year) not treated as dedup - AcoustID quarantine subdir skipped - YouTube / Tidal ``id||title`` encoded filenames - transfer_dir fallback - Both dirs miss → (None, None) - Non-audio files ignored - Empty api_filename - Fuzzy match on punctuation variant - Fuzzy rejects below threshold 475 downloads tests pass after the lift. --- core/downloads/file_finder.py | 275 ++++++++++++++++++++++ core/soulseek_client.py | 89 ++++++- tests/downloads/test_file_finder.py | 345 ++++++++++++++++++++++++++++ web_server.py | 22 +- webui/static/helper.js | 1 + 5 files changed, 716 insertions(+), 16 deletions(-) create mode 100644 core/downloads/file_finder.py create mode 100644 tests/downloads/test_file_finder.py diff --git a/core/downloads/file_finder.py b/core/downloads/file_finder.py new file mode 100644 index 00000000..98aa4a66 --- /dev/null +++ b/core/downloads/file_finder.py @@ -0,0 +1,275 @@ +"""Robust completed-download file finder. + +Walks a download directory (and optional transfer directory) to find +the local file matching an API-reported remote filename. Handles: + +- Arbitrary subdirectory layouts (slskd flat / username-prefixed / + remote-tree-preserved). Pre-extract callers in the Soulseek + album-bundle path tried three hard-coded candidate paths and + silently failed on layouts that didn't match — see issue #715 + (Billy Ocean album task fails after slskd finishes downloading + release). The per-track flow already used a recursive walk and + worked; the bundle path didn't, so users on common slskd configs + with username-prefixed downloads saw bundles time out 22 minutes + after slskd reported every transfer Completed. +- slskd dedup suffix ``_<10-or-more-digit-timestamp>`` appended when + a file with the same basename already exists. +- YouTube / Tidal encoded filename format ``id||title`` — the + ``||`` half is the human title and used for matching. +- Multiple files sharing a basename — disambiguates by counting + how many remote-path directory components appear in the local + path. + +This was lifted verbatim from ``web_server._find_completed_file_robust`` +so both the per-track download poll AND the Soulseek album-bundle +poll go through one finder. Pre-extract the bundle path probed +three hard-coded candidates only, which is why bundle downloads +on slskd setups with username-prefixed download dirs silently +timed out (#715). +""" + +from __future__ import annotations + +import logging +import os +import re +from difflib import SequenceMatcher +from typing import Optional, Tuple + +from unidecode import unidecode + +logger = logging.getLogger(__name__) + + +AUDIO_EXTENSIONS = frozenset({ + '.mp3', '.flac', '.m4a', '.aac', '.ogg', '.opus', '.wav', '.wma', + '.alac', '.aiff', '.aif', '.dsf', '.dff', '.ape', +}) + +# slskd appends a 10+ digit timestamp suffix when a file with the +# same basename already exists. Match-strip those so we still +# resolve the transfer. +_SLSKD_DEDUP_SUFFIX = re.compile(r'_\d{10,}$') + +# AcoustID-quarantined files live under this dirname and must be +# skipped — they are known-wrong matches the verifier rejected. +_QUARANTINE_DIRNAME = 'ss_quarantine' + +# Confidence floor for accepting a fuzzy basename match. Anything +# below this is treated as "no match" so we don't drag in unrelated +# files. +_FUZZY_THRESHOLD = 0.85 + + +def _is_audio_candidate(path: str) -> bool: + return os.path.splitext(str(path or ''))[1].lower() in AUDIO_EXTENSIONS + + +def _normalize_for_finding(text: str) -> str: + """Match-engine-style normalisation for fuzzy filename comparison. + + Lowercases, transliterates unicode, drops bracketed content + ("(Remastered 2016)", "[FLAC]"), strips punctuation, collapses + whitespace. Mirrors ``matching_engine.py``'s text normaliser so + finder + matcher agree on equivalence. + """ + if not text: + return "" + text = unidecode(text).lower() + text = re.sub(r'[._/]', ' ', text) + text = re.sub(r'[\[\(].*?[\]\)]', '', text) + text = re.sub(r'[^a-z0-9\s-]', '', text) + return ' '.join(text.split()).strip() + + +def _extract_basename(api_filename: str) -> str: + """Cross-platform rightmost-separator split, with YouTube / + Tidal ``id||title`` encoded filenames pre-normalised — the id + half is stripped so the title becomes the basename. Mirrors + the strip-then-split order ``web_server`` used.""" + if not api_filename: + return "" + if '||' in api_filename: + _id, title = api_filename.split('||', 1) + api_filename = title + last_slash = max(api_filename.rfind('/'), api_filename.rfind('\\')) + return api_filename[last_slash + 1:] if last_slash != -1 else api_filename + + +def _api_dir_parts(api_filename: str) -> list[str]: + """Lowercased remote-path directory components, sans the + filename itself. Used to disambiguate when several local files + share the same basename — the one whose path mirrors the + remote folder structure wins.""" + if not api_filename: + return [] + normalized = api_filename.replace('\\', '/') + return [p.lower() for p in normalized.split('/')[:-1] if p] + + +def _path_matches_api_dirs(file_path: str, api_dirs: list[str]) -> bool: + """``True`` iff every remote directory component appears as a + path part of the local file. Cheap "is this file on a sibling + tree" check.""" + if not api_dirs: + return False + path_parts = set(p.lower() for p in file_path.replace('\\', '/').split('/')) + return all(d in path_parts for d in api_dirs) + + +def _search_in_directory( + search_dir: str, + location_name: str, + target_basename: str, + normalized_target: str, + api_dirs: list[str], +) -> Tuple[Optional[str], float]: + """Walk ``search_dir`` once, return (best_match, similarity). + + Priority order, highest first: + 1. Exact basename match with directory-structure confirmation + 2. Exact basename match without disambiguation (when no api_dirs) + 3. slskd-dedup-suffix basename match (same two tiers as exact) + 4. Best fuzzy basename match above ``_FUZZY_THRESHOLD`` + """ + best_fuzzy_path: Optional[str] = None + highest_fuzzy_similarity = 0.0 + exact_matches: list[str] = [] + + for root, dirs, files in os.walk(search_dir): + # Strip quarantine subdir from the walk in place — these + # files are known-bad and matching them would re-poison the + # post-process pipeline. + dirs[:] = [d for d in dirs if d != _QUARANTINE_DIRNAME] + + for filename in files: + file_path = os.path.join(root, filename) + if not _is_audio_candidate(file_path): + continue + + # Tier 1 + 2: exact basename match. + if filename == target_basename: + if api_dirs and _path_matches_api_dirs(file_path, api_dirs): + logger.info( + "Found path-confirmed match in %s: %s", + location_name, file_path, + ) + return file_path, 1.0 + if not api_dirs: + logger.info( + "Found exact match in %s: %s", + location_name, file_path, + ) + return file_path, 1.0 + exact_matches.append(file_path) + continue + + # Tier 3: slskd dedup suffix. + stem, ext = os.path.splitext(filename) + stripped_stem = _SLSKD_DEDUP_SUFFIX.sub('', stem) + if stripped_stem != stem and stripped_stem + ext == target_basename: + if api_dirs and _path_matches_api_dirs(file_path, api_dirs): + logger.info( + "Found path-confirmed dedup match in %s: %s", + location_name, file_path, + ) + return file_path, 1.0 + if not api_dirs: + logger.info( + "Found dedup-suffix match in %s: %s", + location_name, file_path, + ) + return file_path, 1.0 + exact_matches.append(file_path) + continue + + # Tier 4: fuzzy basename match. Cheaper than path-walking + # the whole tree a second time, so always compute and + # keep the best one as a fallback. + normalized_file = _normalize_for_finding(filename) + similarity = SequenceMatcher( + None, normalized_target, normalized_file, + ).ratio() + if similarity > highest_fuzzy_similarity: + highest_fuzzy_similarity = similarity + best_fuzzy_path = file_path + + if exact_matches: + if len(exact_matches) == 1: + logger.info( + "Found exact match in %s: %s", + location_name, exact_matches[0], + ) + return exact_matches[0], 1.0 + # Multiple basename collisions — pick the one whose path + # carries the most of the remote directory tree (album + # folder etc.). Breaks ties deterministically. + best = exact_matches[0] + best_score = -1 + for m in exact_matches: + m_parts = set(p.lower() for p in m.replace('\\', '/').split('/')) + score = sum(1 for d in api_dirs if d in m_parts) + if score > best_score: + best_score = score + best = m + logger.info( + "Found %d files named '%s' in %s, picked best path match: %s", + len(exact_matches), target_basename, location_name, best, + ) + return best, 1.0 + + return best_fuzzy_path, highest_fuzzy_similarity + + +def find_completed_audio_file( + download_dir: str, + api_filename: str, + transfer_dir: Optional[str] = None, +) -> Tuple[Optional[str], Optional[str]]: + """Locate a completed download's local file via recursive walk. + + Tries the downloads tree first; if nothing above the fuzzy + threshold lands AND a transfer_dir was passed, tries that too. + + Returns ``(file_path, location)`` where ``location`` is + ``'downloads'`` / ``'transfer'`` / ``None``. Both elements are + ``None`` when the file isn't found anywhere — callers should + treat that as "not yet" (still mid-write) or "lost". + """ + # YouTube / Tidal encoded filenames carry the id ahead of ``||``. + # Strip it up front so basename + dir-component extraction both + # operate on the title half. + if api_filename and '||' in api_filename: + _id, api_filename = api_filename.split('||', 1) + target_basename = _extract_basename(api_filename) + normalized_target = _normalize_for_finding(target_basename) + api_dirs = _api_dir_parts(api_filename) + + best_dl_path, dl_sim = _search_in_directory( + download_dir, 'downloads', target_basename, normalized_target, api_dirs, + ) + + if dl_sim > _FUZZY_THRESHOLD: + if dl_sim < 1.0: + logger.info( + "Found fuzzy match in downloads (%.2f): %s", + dl_sim, best_dl_path, + ) + return (best_dl_path, 'downloads') + + if transfer_dir and os.path.exists(transfer_dir): + best_tx_path, tx_sim = _search_in_directory( + transfer_dir, 'transfer', target_basename, normalized_target, api_dirs, + ) + if tx_sim > _FUZZY_THRESHOLD: + if tx_sim < 1.0: + logger.info( + "Found fuzzy match in transfer (%.2f): %s", + tx_sim, best_tx_path, + ) + return (best_tx_path, 'transfer') + + return (None, None) + + +__all__ = ['find_completed_audio_file', 'AUDIO_EXTENSIONS'] diff --git a/core/soulseek_client.py b/core/soulseek_client.py index 8a7f82dd..561e4867 100644 --- a/core/soulseek_client.py +++ b/core/soulseek_client.py @@ -1536,12 +1536,21 @@ class SoulseekClient(DownloadSourcePlugin): result['error'] = 'No suitable Soulseek album folder after filtering' return result + # On the preflight-reuse path ``picked`` is None — the master + # already selected the folder so we never call _pick_album_bundle_folder. + # Read the track count off the preferred_tracks list in that + # case so the log line doesn't misleadingly report "0 tracks". + _log_track_count = ( + getattr(picked, 'track_count', 0) if picked is not None + else len(folder_tracks) + ) + _log_quality = getattr(picked, 'dominant_quality', '') if picked is not None else '' logger.info( "[Soulseek album] Picked %s:%s (%s tracks, quality=%s)", username, folder_path, - getattr(picked, 'track_count', 0), - getattr(picked, 'dominant_quality', ''), + _log_track_count, + _log_quality, ) _emit( 'queued', @@ -1678,6 +1687,19 @@ class SoulseekClient(DownloadSourcePlugin): interval = get_poll_interval() completed_paths: Dict[tuple, Path] = {} failed_states: Dict[tuple, str] = {} + # Track keys where slskd reports the transfer Completed / + # Succeeded but the local file finder can't yet locate the + # file on disk. Usually transient (slskd writes the file + # after announcing completion); becomes a hard failure when + # ALL remaining keys land here long enough — that's the + # symptom from issue #715 (Billy Ocean bundle hung 22 min + # after slskd finished). The grace window keeps the + # transient case from triggering; the all-stuck check + # short-circuits when there's no chance of progress. + unresolved_since: Dict[tuple, float] = {} + # Seconds an "slskd Completed but locally unresolved" key + # has to stay stuck before we give up on it. + _unresolved_grace = 45.0 while time.monotonic() < deadline: try: downloads = run_async(self.get_all_downloads()) @@ -1716,7 +1738,13 @@ class SoulseekClient(DownloadSourcePlugin): path = self._resolve_downloaded_album_file(track.filename) if path: completed_paths[key] = path + unresolved_since.pop(key, None) else: + # First time we see slskd report this key as + # completed-but-locally-missing, stamp it. + # Subsequent iterations keep the original + # stamp so the grace window is real wall-time. + unresolved_since.setdefault(key, time.monotonic()) logger.debug( "[Soulseek album] Transfer completed but local file not found yet: %s", track.filename, @@ -1739,6 +1767,33 @@ class SoulseekClient(DownloadSourcePlugin): return [] if len(completed_paths) == len(transfer_keys): return list(completed_paths.values()) + + # Early exit when every remaining key is "slskd done + + # locally unresolved past grace". Pre-fix this was the + # silent timeout path from issue #715 — slskd finished + # downloading the whole album, but no local file ever + # resolved, so the poll spun until ``get_poll_timeout()`` + # elapsed (default 30+ minutes) before failing the batch. + now = time.monotonic() + still_pending = [ + k for k in transfer_keys + if k not in completed_paths and k not in failed_states + ] + if still_pending and all( + k in unresolved_since and (now - unresolved_since[k]) >= _unresolved_grace + for k in still_pending + ): + logger.error( + "[Soulseek album] %d transfer(s) reported Completed by slskd " + "but no local file could be resolved after %.0fs — likely a " + "``soulseek.download_path`` mismatch (Docker volume / " + "username-prefixed slskd config). Files this poll attempted: %s", + len(still_pending), + _unresolved_grace, + [transfer_keys[k].filename for k in still_pending[:5]], + ) + return list(completed_paths.values()) + time.sleep(interval) pending = len(transfer_keys) - len(completed_paths) - len(failed_states) if completed_paths: @@ -1758,9 +1813,26 @@ class SoulseekClient(DownloadSourcePlugin): return [] def _resolve_downloaded_album_file(self, remote_filename: str) -> Optional[Path]: + # Pre-fix this tried three hardcoded candidate paths and + # silently returned None on anything else — including the + # common slskd config that nests downloads under + # ``//``. That mismatch + # caused issue #715: bundle downloads on those setups + # timed out 22 minutes after slskd reported every transfer + # Completed because the resolver never located a single + # local file. + # + # Now delegates to the shared robust finder which recursively + # walks the download dir by basename + path-confirms via the + # remote directory components. Same logic the per-track flow + # has used since 2.5.9. + from core.downloads.file_finder import find_completed_audio_file basename = os.path.basename((remote_filename or '').replace('\\', '/')) if not basename: return None + # Fast path: the three hardcoded candidates still cover the + # default slskd-flat layout cheaply, and avoid an os.walk + # for the common case. Walk only when none hit. candidates = [ self.download_path / remote_filename, self.download_path / basename, @@ -1774,14 +1846,11 @@ class SoulseekClient(DownloadSourcePlugin): return candidate except OSError: continue - try: - matches = list(self.download_path.rglob(basename)) - except OSError: - matches = [] - for match in matches: - if match.is_file(): - return match - return None + + found_path, _location = find_completed_audio_file( + str(self.download_path), remote_filename, + ) + return Path(found_path) if found_path else None async def check_connection(self) -> bool: """Check if slskd is running and connected to the Soulseek network""" diff --git a/tests/downloads/test_file_finder.py b/tests/downloads/test_file_finder.py new file mode 100644 index 00000000..a012faa3 --- /dev/null +++ b/tests/downloads/test_file_finder.py @@ -0,0 +1,345 @@ +"""Tests for ``core/downloads/file_finder.py``. + +Real-world regression these tests pin: the Soulseek album-bundle +post-process previously had its own three-candidate probe for the +local downloaded file. When slskd was configured to nest downloads +under a username subdir (a common config) NONE of the three +candidates matched, the poll silently timed out 22 minutes later, +and the batch went to "failed" even though slskd had successfully +downloaded every track of the album. The per-track download path +already used the recursive-walk finder and worked fine — these +tests pin the lifted shared finder so both paths now find files +no matter what layout slskd writes. + +Issue: #715 (Billy Ocean — Download album task fails after slskd +finishes downloading release). +""" + +from __future__ import annotations + +import os +from pathlib import Path + +import pytest + +from core.downloads.file_finder import find_completed_audio_file + + +def _touch(path: Path, content: bytes = b'\x00\x00'): + """Create a small placeholder file at ``path``.""" + path.parent.mkdir(parents=True, exist_ok=True) + path.write_bytes(content) + + +# --------------------------------------------------------------------------- +# Layout coverage — slskd writes downloads to many different paths +# depending on its config. The bundle resolver must find the file in +# all of these layouts; the pre-lift code only handled the first two. +# --------------------------------------------------------------------------- + + +def test_finds_file_in_flat_slskd_layout(tmp_path): + """Default slskd config: ``/``.""" + downloads = tmp_path / 'downloads' + target = downloads / '01 - Suddenly.flac' + _touch(target) + + found, location = find_completed_audio_file( + str(downloads), r'shared\Billy Ocean\Very Best Of\01 - Suddenly.flac', + ) + + assert found == str(target) + assert location == 'downloads' + + +def test_finds_file_under_username_subdir(tmp_path): + """Regression for #715. slskd configured with + ``directories.downloads.username = true`` writes to + ``//``. Pre-lift the bundle + resolver missed this layout because it only probed + ``download_dir/filename`` / ``download_dir/basename`` / + ``download_dir/normalized_remote_path`` — none included a + username segment, so every file looked missing.""" + downloads = tmp_path / 'downloads' + target = downloads / '3opgkrpokgreg' / '01 - Suddenly.flac' + _touch(target) + + found, location = find_completed_audio_file( + str(downloads), r'shared\Billy Ocean\Very Best Of\01 - Suddenly.flac', + ) + + assert found == str(target) + assert location == 'downloads' + + +def test_finds_file_when_slskd_preserves_remote_tree(tmp_path): + """slskd config where the remote sharer's folder tree is + mirrored locally — ``/shared///``.""" + downloads = tmp_path / 'downloads' + target = downloads / 'shared' / 'Billy Ocean' / 'Very Best Of' / '01 - Suddenly.flac' + _touch(target) + + found, location = find_completed_audio_file( + str(downloads), r'shared\Billy Ocean\Very Best Of\01 - Suddenly.flac', + ) + + assert found == str(target) + assert location == 'downloads' + + +def test_finds_file_in_deeply_nested_user_tree(tmp_path): + """Some slskd setups combine username + preserved tree. Finder + walks recursively so depth doesn't matter.""" + downloads = tmp_path / 'downloads' + target = (downloads / '3opgkrpokgreg' / 'Music' + / 'Billy Ocean' / 'Very Best Of' + / '01 - Suddenly.flac') + _touch(target) + + found, location = find_completed_audio_file( + str(downloads), r'shared\Billy Ocean\Very Best Of\01 - Suddenly.flac', + ) + + assert found == str(target) + + +def test_returns_none_when_file_missing(tmp_path): + """File genuinely not on disk → both elements ``None``.""" + downloads = tmp_path / 'downloads' + downloads.mkdir() + _touch(downloads / '02 - Caribbean Queen.flac') # different file + + found, location = find_completed_audio_file( + str(downloads), r'shared\Billy Ocean\Very Best Of\01 - Suddenly.flac', + ) + + assert found is None + assert location is None + + +# --------------------------------------------------------------------------- +# Disambiguation — when multiple files share a basename, the path +# whose components mirror the remote tree wins. +# --------------------------------------------------------------------------- + + +def test_path_confirms_against_remote_dirs_when_basename_collides(tmp_path): + """Two albums both contain ``01 - Intro.flac``. The finder + picks the one whose path carries the most remote-dir components + — keeps two simultaneous bundle downloads from claiming each + other's file.""" + downloads = tmp_path / 'downloads' + other_album = downloads / 'Some Other Artist' / 'Other Album' / '01 - Intro.flac' + target_album = downloads / 'Billy Ocean' / 'Very Best Of' / '01 - Intro.flac' + _touch(other_album) + _touch(target_album) + + found, _ = find_completed_audio_file( + str(downloads), r'shared\Billy Ocean\Very Best Of\01 - Intro.flac', + ) + + assert found == str(target_album) + + +def test_returns_only_match_when_no_disambiguation_possible(tmp_path): + """Single basename match in the tree → return it regardless of + whether the remote dir components line up.""" + downloads = tmp_path / 'downloads' + target = downloads / 'random' / 'subdir' / '01 - Suddenly.flac' + _touch(target) + + found, _ = find_completed_audio_file( + str(downloads), r'shared\Billy Ocean\Very Best Of\01 - Suddenly.flac', + ) + + assert found == str(target) + + +# --------------------------------------------------------------------------- +# slskd dedup suffix — when a file with the same name already exists, +# slskd appends ``_`` to the new file. The finder must +# strip this and still match the original API filename. +# --------------------------------------------------------------------------- + + +def test_matches_slskd_dedup_suffix(tmp_path): + """``Song.flac`` requested → ``Song_639067852665564677.flac`` + on disk (slskd dedup). Finder strips the timestamp suffix and + returns the deduped file.""" + downloads = tmp_path / 'downloads' + target = downloads / '01 - Suddenly_639067852665564677.flac' + _touch(target) + + found, _ = find_completed_audio_file( + str(downloads), r'shared\Billy Ocean\Very Best Of\01 - Suddenly.flac', + ) + + assert found == str(target) + + +def test_dedup_suffix_short_digits_not_treated_as_dedup(tmp_path): + """Year-style ``_2007`` (4 digits) is NOT slskd's dedup format — + that's a 10+ digit timestamp. A file like ``Greatest Hits_2007.flac`` + that exists alongside the requested ``Greatest Hits.flac`` must + not be returned as a tier-1 dedup match. (Fuzzy may still grab + it as a lower-confidence fallback when no real match exists, + which is intentional — the strict-dedup tier is what we're + pinning here.)""" + downloads = tmp_path / 'downloads' + # Real match exists too — so the dedup-incorrect file doesn't + # win by default. Tests the priority order. + real_match = downloads / 'Billy Ocean' / '01 - Suddenly.flac' + near_miss = downloads / '01 - Suddenly_2007.flac' + _touch(real_match) + _touch(near_miss) + + found, _ = find_completed_audio_file( + str(downloads), r'shared\Billy Ocean\Very Best Of\01 - Suddenly.flac', + ) + + # Real exact-basename + path-confirmed match must win over the + # year-suffixed file. + assert found == str(real_match) + + +# --------------------------------------------------------------------------- +# Special inputs — YouTube/Tidal encoded filenames, quarantine, +# empty paths, transfer-dir fallback. +# --------------------------------------------------------------------------- + + +def test_skips_quarantine_subdir(tmp_path): + """Files under ``ss_quarantine/`` are known-wrong AcoustID + rejects — the finder must ignore them so a quarantined-but-still- + present file doesn't get re-claimed.""" + downloads = tmp_path / 'downloads' + quarantined = downloads / 'ss_quarantine' / '01 - Suddenly.flac' + real = downloads / 'Billy Ocean' / '01 - Suddenly.flac' + _touch(quarantined) + _touch(real) + + found, _ = find_completed_audio_file( + str(downloads), r'shared\Billy Ocean\Very Best Of\01 - Suddenly.flac', + ) + + assert found == str(real) + + +def test_handles_youtube_tidal_encoded_filename(tmp_path): + """YouTube / Tidal downloads encode the API filename as + ``id||title``. The finder strips the id and matches against + the title half.""" + downloads = tmp_path / 'downloads' + target = downloads / 'My Song.mp3' + _touch(target) + + found, _ = find_completed_audio_file(str(downloads), 'abc123||My Song.mp3') + + assert found == str(target) + + +def test_falls_back_to_transfer_dir_when_download_dir_misses(tmp_path): + """File has already moved into the transfer dir. The finder + falls through to the second search root rather than returning + None — covers the post-process race where a file is mid-move.""" + downloads = tmp_path / 'downloads' + downloads.mkdir() + transfer = tmp_path / 'transfer' + moved = transfer / 'Billy Ocean' / '01 - Suddenly.flac' + _touch(moved) + + found, location = find_completed_audio_file( + str(downloads), + r'shared\Billy Ocean\Very Best Of\01 - Suddenly.flac', + transfer_dir=str(transfer), + ) + + assert found == str(moved) + assert location == 'transfer' + + +def test_returns_none_when_neither_dir_has_file(tmp_path): + """Missing in both download AND transfer → ``(None, None)``.""" + downloads = tmp_path / 'downloads' + transfer = tmp_path / 'transfer' + downloads.mkdir() + transfer.mkdir() + + found, location = find_completed_audio_file( + str(downloads), + r'shared\Billy Ocean\Very Best Of\01 - Suddenly.flac', + transfer_dir=str(transfer), + ) + + assert found is None + assert location is None + + +def test_ignores_non_audio_files(tmp_path): + """Cover art, NFO, and text files in the slskd dir must not be + surfaced as audio matches even when their stem aligns.""" + downloads = tmp_path / 'downloads' + _touch(downloads / '01 - Suddenly.txt') # wrong extension + _touch(downloads / '01 - Suddenly.nfo') + _touch(downloads / 'cover.jpg') + target = downloads / '01 - Suddenly.flac' + _touch(target) + + found, _ = find_completed_audio_file( + str(downloads), r'shared\Billy Ocean\Very Best Of\01 - Suddenly.flac', + ) + + assert found == str(target) + + +def test_empty_api_filename_returns_none(tmp_path): + """Defensive — empty / None filename can't resolve to anything.""" + downloads = tmp_path / 'downloads' + downloads.mkdir() + _touch(downloads / '01 - Suddenly.flac') + + found, location = find_completed_audio_file(str(downloads), '') + + assert found is None + assert location is None + + +# --------------------------------------------------------------------------- +# Fuzzy fallback — when no exact / dedup match lands, the finder +# falls back to the closest-basename fuzzy match above 0.85. +# --------------------------------------------------------------------------- + + +def test_fuzzy_matches_punctuation_variant(tmp_path): + """Soulseek shares vary in separator style — underscore vs + dash vs period. The normaliser collapses all three to spaces + so the fuzzy comparator can match across the variation.""" + downloads = tmp_path / 'downloads' + # On disk: underscore. API filename: dash. Both normalise to + # the same token stream. + target = downloads / "01_Caribbean_Queen.flac" + _touch(target) + + found, _ = find_completed_audio_file( + str(downloads), + r"shared\Billy Ocean\Very Best Of\01 - Caribbean Queen.flac", + ) + + assert found == str(target) + + +def test_fuzzy_rejects_low_similarity(tmp_path): + """A completely different filename in the same dir must not + fuzzy-match — the 0.85 floor keeps unrelated files out.""" + downloads = tmp_path / 'downloads' + _touch(downloads / 'random-unrelated-file.flac') + + found, _ = find_completed_audio_file( + str(downloads), r'shared\Billy Ocean\Very Best Of\01 - Suddenly.flac', + ) + + assert found is None + + +if __name__ == '__main__': + pytest.main([__file__, '-v']) diff --git a/web_server.py b/web_server.py index 268ec877..2ba505b9 100644 --- a/web_server.py +++ b/web_server.py @@ -5984,14 +5984,24 @@ def start_download(): def _find_completed_file_robust(download_dir, api_filename, transfer_dir=None): - """ - Robustly finds a completed file on disk, accounting for name variations and - unexpected subdirectories. This version uses the superior normalization logic - from the GUI's matching_engine.py to ensure consistency. + """Thin wrapper around the shared file finder. - First searches in download_dir, then optionally searches in transfer_dir if provided. - Returns tuple (file_path, location) where location is 'downloads' or 'transfer'. + Behaviour preserved verbatim — the implementation lives in + ``core/downloads/file_finder.py`` so the Soulseek album-bundle + poll (which previously had its own 3-candidate probe and + silently timed out on slskd configs that nested downloads under + a username subdir, see issue #715) and the per-track download + poll go through the same recursive-walk + path-confirm logic. """ + from core.downloads.file_finder import find_completed_audio_file + return find_completed_audio_file(download_dir, api_filename, transfer_dir) + + +def _find_completed_file_robust_legacy(download_dir, api_filename, transfer_dir=None): + """Legacy inline implementation, kept for reference. Unused + after the lift to ``core/downloads/file_finder.py``. Will be + removed after the next release ships and the new finder + proves itself in the field.""" import re import os from difflib import SequenceMatcher diff --git a/webui/static/helper.js b/webui/static/helper.js index f00ac98b..946b581e 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3415,6 +3415,7 @@ function closeHelperSearch() { const WHATS_NEW = { '2.6.3': [ { unreleased: true }, + { title: 'Fix: Soulseek album downloads stuck on "failed" after slskd finished the release (#715)', desc: 'when an album was downloaded via the Soulseek album-bundle path (the release-first flow added in 2.6), the batch would frequently mark itself as failed even though slskd had successfully downloaded every track of the album. Worst on users running slskd with the common ``directories.downloads.username = true`` config — files landed at ``//`` instead of the three hard-coded candidate paths the bundle resolver probed, so every file looked locally missing and the poll spun until the ~30 minute deadline expired. Per-track Soulseek downloads were unaffected because they already routed through a recursive walk-by-basename helper. Lifted that helper into ``core/downloads/file_finder.py`` and rewired the bundle path to use it — same logic the per-track flow has used since 2.5.9. Also: when the bundle poll detects that slskd reports every transfer Completed but no local file resolves within a 45-second grace window, it now exits with a clear log line pointing at the likely ``soulseek.download_path`` mismatch instead of silently spinning until the master deadline. Misleading "(0 tracks, quality=)" log on the preflight-reuse path also fixed — was reading attrs off a None object. 17 new unit tests pin every slskd layout shape (flat, username-prefixed, full-tree-preserved, deep nested, dedup-suffix, quarantine-skip, YouTube/Tidal encoded names, transfer-dir fallback, fuzzy punctuation variants). Closes #715.', page: 'downloads' }, { title: 'Auto-Sync manager: full visual overhaul to match the dashboard vibe', desc: 'the Auto-Sync manager modal had been carrying its original visual treatment forward unchanged while the rest of the app moved toward the glassy / accent-radial / gradient-border aesthetic the dashboard now sets. Restyled every surface inside the modal to match — selector-based override layer at the end of style.css so functionality is untouched (zero JS or HTML changes). Touched surfaces: modal shell (glass shell with thin accent border + corner radial wash + inner top-edge highlight), header (gradient text title + accent-tinted hairline separator + spinning-X close), KPI summary tiles (dashboard-style gradient tiles with accent top-edge glow on hover + gradient stat numbers), live monitor strip (accent-tinted glass card with status-colored borders), refresh / intro buttons (pill primary with accent fill + glow on hover), tabs (underline-style with accent fill + soft radial glow on the active tab), sidebar (glass panel with accent-tinted source-group cards, accent border on scheduled playlists, accent ring on the filter input focus), board area (subtle accent radial spotlight + column cards with gradient headers + drag-over accent glow), drop zones (animated dashed pull with accent radial wash + accent-tinted text on drag-over), scheduled cards (accent left-edge stripe, gradient pill timing badges, pill "Run now" primary + rotating ghost X), run history rows (dashboard recent-activity aesthetic with accent hover lift + pill status badges), bulk-schedule popover, weekly editor (full glass modal matching the version-modal vibe, day-toggle pills, accent-pill save button), and empty / monitor-empty states. Soft accent-tinted scrollbars throughout. Delete the v2 block at the bottom of style.css to revert.', page: 'automations' }, { title: 'Auto-Sync: weekly board cards now match the hourly board', desc: 'when the Weekly Board shipped, its scheduled-card visual diverged from the hourly board — weekly cards only showed the playlist name + weekly label + timezone, while hourly cards carry the full action row (Run now button, unschedule X, next-run countdown, health badge). Standardised the weekly card on the hourly shape so dropping a playlist onto a day column produces a card with the exact same affordances as dropping it onto an hourly bucket: health indicator dot when recent runs failed, source + track count meta line, weekly label + timezone + "next in Nh" pills, Run-now button (disabled while pipeline is running, same gating as hourly), and an unschedule X. Click anywhere outside the buttons still opens the weekly editor for changing days / time / tz. Weekly cards also became draggable between day columns now — drop on a new column appends that day to the schedule (matches the existing multi-day editor flow).', page: 'automations' }, { title: 'Dashboard: enrichment equalizer bars get next-level polish', desc: 'follow-up to the equalizer redesign. four upgrades that take the bars from "nice" to "vibey": (1) every bar now carries a circular avatar disc above the track loading the service\'s real brand logo (the same images the header-action worker orbs use — Spotify, Apple Music, Deezer, Last.fm, Genius, MusicBrainz, AudioDB, Tidal, Qobuz, Discogs, Amazon Music). Disc has a dark glass backdrop, accent-tinted ring, and slow halo pulse when the worker is running; the brand color shows through as a drop-shadow on the logo + ring tint. Initial-letter fallback kicks in automatically if any CDN URL fails; (2) peak-flash detector — when the worker\'s calls/min actually steps upward between updates, the peak tip briefly flares white-hot and the fill flashes brighter for ~650ms, like a hardware VU meter\'s peak-detect LED catching a beat. only fires on real increases above a noise threshold so jitter doesn\'t pulse the bar constantly; (3) rolling number counter — the live count digit-animates from old→new value with easeOutCubic instead of snapping, the same premium-counter feel you\'d see on a fancy dashboard; (4) glass-surface reflection puddle under each bar — soft accent-colored glow whose opacity tracks the real (unclamped) rate, so the row looks like the bars are standing on a polished pane rather than floating in space. puddle ripples on peak-flash too. all four tied together by the same --eq-accent / --eq-glow CSS variables so future tweaks stay coherent.', page: 'dashboard' },