diff --git a/config/config.example.json b/config/config.example.json index 0543822c..e6adf55c 100644 --- a/config/config.example.json +++ b/config/config.example.json @@ -44,7 +44,8 @@ }, "metadata_enhancement": { "enabled": true, - "embed_album_art": true + "embed_album_art": true, + "single_to_album": false }, "file_organization": { "enabled": true, diff --git a/config/settings.py b/config/settings.py index 28d1908a..fe03096e 100644 --- a/config/settings.py +++ b/config/settings.py @@ -662,7 +662,12 @@ class ConfigManager: # source whose art is smaller is skipped so the next source is # tried — stops a low-res Cover Art Archive upload from winning. # 0 disables the size gate. - "min_art_size": 1000 + "min_art_size": 1000, + # When a track matches a SINGLE release, look up the parent ALBUM + # that contains it and tag it as that album, so it groups with its + # album-mates and gets the album cover (not the single's). Off by + # default — it's an extra per-import metadata lookup. + "single_to_album": False }, "musicbrainz": { "embed_tags": True diff --git a/core/auto_import_worker.py b/core/auto_import_worker.py index 8bfbea34..15776fa4 100644 --- a/core/auto_import_worker.py +++ b/core/auto_import_worker.py @@ -660,8 +660,13 @@ class AutoImportWorker: auto_process = self._config_manager.get('auto_import.auto_process', True) try: - # Phase 3: Identify - identification = self._identify_folder(candidate) + # Phase 3: Identify. + # Re-identify (#889): if the user designated this exact file's release in + # the Re-identify modal, a hint short-circuits the guessing — we match + # straight against the chosen album. No hint → byte-identical to before. + rematch_hint, identification = self._resolve_rematch_hint(candidate) + if identification is None: + identification = self._identify_folder(candidate) if not identification: self._record_result(candidate, 'needs_identification', 0.0, error_message='Could not identify album from tags, folder name, or fingerprint') @@ -690,7 +695,10 @@ class AutoImportWorker: high_conf_matches = [m for m in match_result.get('matches', []) if m['confidence'] >= 0.8] has_strong_individual_matches = len(high_conf_matches) > 0 - if (confidence >= threshold or has_strong_individual_matches) and auto_process: + # A re-identify is an explicit user choice — let it auto-process like a + # strong match (still gated on the global auto_process preference). + if (confidence >= threshold or has_strong_individual_matches + or rematch_hint is not None) and auto_process: # Phase 5: Auto-process — insert an in-progress row # so the UI sees the import the moment it starts, # then update it with the final status when done. @@ -709,6 +717,13 @@ class AutoImportWorker: confidence = max(confidence, effective_conf) if success: self._bump_stat('auto_processed') + # Re-identify (#889): only NOW that the new home exists do we + # consume the hint and (if replace was chosen) delete the old + # row + file — so a failed import never loses the original. Pass + # the landing paths so we never delete a file the re-import landed + # at the SAME place (picking the release it's already in). + if rematch_hint is not None: + self._finalize_rematch_hint(rematch_hint, getattr(candidate, '_reid_final_paths', None)) else: self._bump_stat('failed') @@ -1003,6 +1018,75 @@ class AutoImportWorker: except Exception: return False + # ── Re-identify hints (#889) ── + + def _resolve_rematch_hint(self, candidate: 'FolderCandidate'): + """If this staged file carries a user-designated re-identify hint, return + ``(hint, identification)`` so matching skips the guessing tiers; otherwise + ``(None, None)`` and the caller falls back to normal identification. + + Fail-safe: ANY error (no table, DB hiccup) returns ``(None, None)`` so a + re-identify problem can never break ordinary auto-import. Only single-file + candidates are eligible — a re-identify always stages exactly one track.""" + try: + files = candidate.audio_files or [] + if len(files) != 1: + return None, None + from core.imports.rematch_hints import ( + build_identification_from_hint, + find_hint_for_file, + quick_file_signature, + ) + file_path = files[0] + sig = quick_file_signature(file_path) + conn = self.database._get_connection() + try: + cursor = conn.cursor() + hint = find_hint_for_file(cursor, file_path, sig) + finally: + conn.close() + if hint is None: + return None, None + logger.info("[Auto-Import] Re-identify hint for %s → %s '%s' (%s)", + candidate.name, hint.album_type or 'release', + hint.album_name or '?', hint.source) + return hint, build_identification_from_hint(hint) + except Exception as e: + logger.debug("[Auto-Import] rematch-hint lookup skipped: %s", e) + return None, None + + def _finalize_rematch_hint(self, hint, new_paths=None) -> None: + """Post-success: delete the replaced library row + file (if the user chose + replace) and consume the hint so it's single-use. ``new_paths`` are where the + re-import landed — passed through so the same-home guard never deletes a file + the import wrote at the old location. Best-effort — a cleanup failure is + logged, never raised, since the re-import already succeeded.""" + try: + from core.imports.rematch_hints import consume_hint, delete_replaced_track + + def _resolve_old(stored): + # The old row's path is a STORED path (Docker/media-server view) — map + # it to a file this process can actually unlink, same as everywhere else. + try: + from core.library.path_resolver import resolve_library_file_path + return resolve_library_file_path(stored, config_manager=getattr(self, '_config_manager', None)) + except Exception: + return None + + conn = self.database._get_connection() + try: + cursor = conn.cursor() + removed = delete_replaced_track(cursor, hint.replace_track_id, + resolve_fn=_resolve_old, new_paths=new_paths) + consume_hint(cursor, hint.id) + conn.commit() + finally: + conn.close() + if removed: + logger.info("[Auto-Import] Re-identify replaced old track — removed %s", removed) + except Exception as e: + logger.warning("[Auto-Import] rematch-hint finalize failed (import still OK): %s", e) + # ── Identification ── def _identify_folder(self, candidate: FolderCandidate) -> Optional[Dict]: @@ -1431,8 +1515,11 @@ class AutoImportWorker: def _match_tracks(self, candidate: FolderCandidate, identification: Dict) -> Optional[Dict]: """Match staging files to the identified album's tracklist.""" - # Singles: no album tracklist to match against — the file IS the match - if candidate.is_single or identification.get('is_single'): + # Singles: no album tracklist to match against — the file IS the match. + # force_album_match (set by a re-identify hint) overrides this: even a lone + # staged file is matched INTO the chosen album, so it inherits the album's + # year / track number / art instead of the bare singles stub (#889). + if not identification.get('force_album_match') and (candidate.is_single or identification.get('is_single')): conf = identification.get('identification_confidence', 0.7) track_data = { 'name': identification.get('track_name', identification.get('album_name', '')), @@ -1600,6 +1687,7 @@ class AutoImportWorker: processed = 0 errors = [] + reid_final_paths = [] # #889: where the pipeline landed each file (same-home guard) all_matches = list(match_result.get('matches', [])) # Album total duration — sum of every matched track's duration. @@ -1780,6 +1868,11 @@ class AutoImportWorker: self._process_callback(context_key, context, file_path) processed += 1 + # Capture where the pipeline actually landed the file (#889 same-home + # guard) — the pipeline writes it back into the mutable context. + _landed = context.get('_final_processed_path') + if _landed: + reid_final_paths.append(_landed) logger.info(f"[Auto-Import] Processed: {track_number}. {track_name}") except Exception as e: @@ -1803,6 +1896,13 @@ class AutoImportWorker: except Exception as e: logger.debug("automation emit failed: %s", e) + # Stash landing paths on the candidate so _finalize_rematch_hint can avoid + # deleting a file the re-import landed at the SAME place (#889). + try: + candidate._reid_final_paths = reid_final_paths + except Exception as e: + logger.debug("could not stash reid final paths: %s", e) + return processed > 0 # ── Database ── diff --git a/core/imports/album_grouping.py b/core/imports/album_grouping.py new file mode 100644 index 00000000..be43c108 --- /dev/null +++ b/core/imports/album_grouping.py @@ -0,0 +1,98 @@ +"""Canonical album grouping for the SoulSync standalone import. + +SoulSync grouped imported tracks into albums by the album NAME string +(``_stable_soulsync_id("artist::album_name")``). That splits one release into +several album rows whenever the name string drifts between imports (case, +punctuation, ``(Deluxe Edition)`` suffixes, source-A-vs-B spelling), and every +downstream tool (Library Re-tag, Cover-Art Filler) then dresses each split row +in its own cover — so songs that belong to one album end up with different art +(Sokhi). + +This module is the pure, seam-testable heart of "group by canonical id, not +name": when an imported track carries a metadata-source RELEASE id, prefer +matching an existing album row by that id over the fragile name string, so the +SAME release always lands in ONE album row regardless of how its name was typed. + +Scope (deliberate): this unifies differently-named imports of the SAME release. +It does NOT merge a track that genuinely matched a SINGLE release (a different +release id) into its parent album — that needs single->album resolution upstream +and is a separate change. New imports only; existing rows are left untouched. + +Pure SQL-over-a-cursor; no app singletons, so it tests against an in-memory DB. +""" + +from __future__ import annotations + +from typing import Any, Optional + +from utils.logging_config import get_logger + +logger = get_logger("imports.album_grouping") + +# Album source-id columns this grouping may key on. An allowlist (not arbitrary +# interpolation) — the column name IS spliced into SQL, so it must be a known, +# trusted identifier. Mirrors get_library_source_id_columns()' 'album' values. +ALLOWED_ALBUM_SOURCE_COLS = frozenset({ + "spotify_album_id", + "itunes_album_id", + "deezer_id", + "soul_id", + "discogs_id", + "musicbrainz_release_id", +}) + + +def find_existing_soulsync_album_id( + cursor: Any, + *, + name_key_id: str, + artist_id: str, + album_name: str, + album_source_col: Optional[str] = None, + album_source_id: Optional[str] = None, +) -> Optional[str]: + """Resolve the existing ``soulsync`` album row a track should join, or None + (caller inserts a new row keyed by ``name_key_id``). + + Match precedence: + 1. ``name_key_id`` — the exact prior stable-name-hash id (unchanged + behaviour: a re-import with the identical name hits its own row). + 2. ``album_source_col == album_source_id`` — CANONICAL grouping: an + existing row already carrying THIS release's source id, so a + differently-named import of the same release unifies instead of + splitting. Only when the column is allow-listed and the id is non-empty. + 3. ``(title, artist_id)`` — the legacy name match (kept so nothing that + grouped before stops grouping now). + """ + cursor.execute( + "SELECT id FROM albums WHERE id = ? AND server_source = 'soulsync'", + (name_key_id,), + ) + row = cursor.fetchone() + if row: + return row[0] + + if album_source_col in ALLOWED_ALBUM_SOURCE_COLS and album_source_id: + try: + cursor.execute( + f"SELECT id FROM albums WHERE {album_source_col} = ? " + "AND server_source = 'soulsync' LIMIT 1", + (album_source_id,), + ) + row = cursor.fetchone() + if row: + return row[0] + except Exception as exc: + # That source has no dedicated album column on this DB (e.g. Deezer + # doesn't split per-entity id columns) — fall through to the name + # match rather than break the import. Mirrors the guarded source-id + # UPDATE the caller already does on insert. + logger.debug("album source-id lookup skipped (%s): %s", album_source_col, exc) + + cursor.execute( + "SELECT id FROM albums WHERE title COLLATE NOCASE = ? AND artist_id = ? " + "AND server_source = 'soulsync' LIMIT 1", + (album_name, artist_id), + ) + row = cursor.fetchone() + return row[0] if row else None diff --git a/core/imports/album_matching.py b/core/imports/album_matching.py index f29d6b56..fce847ad 100644 --- a/core/imports/album_matching.py +++ b/core/imports/album_matching.py @@ -156,8 +156,11 @@ def score_file_against_track( score = 0.0 # Title similarity (TITLE_WEIGHT). Falls back to filename stem when - # the file has no title tag. + # the file has no title tag — strip a leading track-number prefix off that + # stem (#890) so "01 - Sun It Rises" scores against "Sun It Rises". title = file_tags.get('title') or os.path.splitext(os.path.basename(file_path))[0] + from core.imports.paths import strip_leading_track_number + title = strip_leading_track_number(title) track_name = track.get('name', '') score += similarity(title, track_name) * TITLE_WEIGHT diff --git a/core/imports/context.py b/core/imports/context.py index 024c3f36..9ed2a909 100644 --- a/core/imports/context.py +++ b/core/imports/context.py @@ -8,6 +8,10 @@ from __future__ import annotations from typing import Any, Dict, Optional +from utils.logging_config import get_logger + +logger = get_logger("imports.context") + def _as_dict(value: Any) -> Dict[str, Any]: return value if isinstance(value, dict) else {} @@ -179,7 +183,12 @@ def get_import_clean_title( if not title: track_info = get_import_track_info(context) title = _first_value(track_info, "name", "title", default="") - return str(title or default) + title = str(title or default) + # #890: strip a leading track-number prefix that leaked from a filename stem + # (e.g. "01 - Sun It Rises" → "Sun It Rises") so it matches the canonical title. + # Conservative — clean source titles ("7 Rings" etc.) pass through untouched. + from core.imports.paths import strip_leading_track_number + return strip_leading_track_number(title) def get_import_clean_album( @@ -440,4 +449,86 @@ def detect_album_info_web(context, artist_context=None): force_album=True, ) - return None + # Last resort: the track matched a SINGLE with no usable album context — + # look up the parent ALBUM that actually contains it (gated, fail-safe). + return _resolve_single_to_parent_album(context, artist_context) + + +def _resolve_single_to_parent_album(context, artist_context): + """A single-matched track -> a promoted album_info for its parent album, or + None. GATED by ``metadata_enhancement.single_to_album`` (default OFF — it's a + per-import metadata lookup, so it's opt-in). Fail-safe: any miss/error returns + None so the track stays exactly as it was matched (never worse than today).""" + try: + from core.metadata.common import get_config_manager + if not get_config_manager().get("metadata_enhancement.single_to_album", False): + return None + except Exception: + return None + + try: + source = get_import_source(context) + track_info = get_import_track_info(context) + original_search = get_import_original_search(context) + track_title = (track_info.get("name") or original_search.get("title") or "").strip() + artist_name = (extract_artist_name(artist_context) + or get_import_clean_artist(context, default="")).strip() + if not source or not track_title or not artist_name: + return None + artist_id = str(get_import_source_ids(context).get("artist_id") or "") + + from core.metadata.album_tracks import ( + get_artist_albums_for_source, + get_artist_album_tracks, + ) + from core.imports.single_to_album import resolve_single_to_album + + def _acc(o, *ks): + for k in ks: + v = o.get(k) if isinstance(o, dict) else getattr(o, k, None) + if v: + return v + return None + + def fetch_candidates(): + albums = get_artist_albums_for_source( + source, artist_id, artist_name=artist_name, + album_type="album", limit=20) or [] + return [{"name": _acc(a, "name", "title"), + "album_type": _acc(a, "album_type") or "album", + "id": _acc(a, "id", "album_id")} for a in albums] + + def fetch_tracks(alb): + payload = get_artist_album_tracks( + str(alb.get("id") or ""), artist_name=artist_name, + album_name=alb.get("name") or "") or {} + return [(_acc(t, "title", "name", "track_name") or "") + for t in (payload.get("tracks") or [])] + + album = resolve_single_to_album( + track_title, + fetch_album_candidates=fetch_candidates, + fetch_album_tracks=fetch_tracks) + if not album or not album.get("name"): + return None + logger.info("single->album: re-homed '%s' onto parent album '%s'", + track_title, album["name"]) + promoted = build_import_album_info( + context, + album_info={ + "album_name": album["name"], + "track_number": track_info.get("track_number"), + "disc_number": track_info.get("disc_number"), + "album_image_url": "", + "confidence": 0.5, + }, + force_album=True, + ) + # build_import_album_info resolves album_name via get_import_clean_album, + # which prefers original_search.album (the SINGLE's name); override it + # with the resolved parent album so grouping + tags use the album. + promoted["album_name"] = album["name"] + return promoted + except Exception as e: + logger.debug("single->album resolution failed: %s", e) + return None diff --git a/core/imports/paths.py b/core/imports/paths.py index 69de9cc7..f65c6765 100644 --- a/core/imports/paths.py +++ b/core/imports/paths.py @@ -158,6 +158,33 @@ def clean_track_title(track_title: str, artist_name: str) -> str: return cleaned if cleaned else original +# A leading track-number prefix is EITHER a zero-padded number (01, 04, 099 — no +# real song title starts with one), OR a plain number followed by a real separator +# AND a space ("3 - ", "12. "). Deliberately NOT a bare "number space word", so it +# leaves "7 Rings", "99 Luftballons", "50 Ways to Leave Your Lover" and +# "1-800-273-8255" untouched. +_TRACK_NUM_PREFIX_RE = re.compile(r"^\s*(?:0\d{1,2}[\s._)\-]*|\d{1,3}\s*[._)\-]\s+)(?=\S)") + + +def strip_leading_track_number(title: str) -> str: + """Conservatively remove a leading track-number prefix from a track title. + + Fixes #890 — files named ``01 - Sun It Rises.flac`` whose stem leaks into the + title as ``01 - Sun It Rises``, which then never matches the canonical + ``Sun It Rises`` (false "missing"). Only strips an unambiguous track-number + prefix; a coincidental leading number that's part of the title is preserved, and + it never reduces a title to empty or a bare number.""" + s = (title or "").strip() + if not s: + return title or "" + stripped = _TRACK_NUM_PREFIX_RE.sub("", s, count=1).strip() + # Keep the original if stripping left nothing real — empty, a bare number, or + # only punctuation (e.g. "01 - " → "-"). A real title has a letter/digit. + if stripped.isdigit() or not re.search(r"[^\W_]", stripped): + return s + return stripped + + def get_album_type_display(raw_type, track_count) -> str: diff --git a/core/imports/rematch_apply.py b/core/imports/rematch_apply.py new file mode 100644 index 00000000..33bcb8c6 --- /dev/null +++ b/core/imports/rematch_apply.py @@ -0,0 +1,92 @@ +"""#889 Phase 4/5: apply a re-identify — stage the library file + write the hint. + +When the user confirms a release in the Re-identify modal, we: + 1. COPY (never move) the track's library file into the auto-import staging folder, + so the original is untouched until the re-import succeeds, + 2. fingerprint the staged copy (rename-proof binding), and + 3. write a single-use hint carrying the chosen release's IDs (+ ``replace_track_id`` + when 'replace original' is ticked). + +The auto-import worker then picks the staged file up, finds the hint, and re-imports +it against the user-chosen release (Phase 2). The pieces here are split so the +naming + hint construction are pure/unit-tested and the actual copy is injectable. +""" + +from __future__ import annotations + +import os +import shutil +from typing import Any, Callable, Dict, Optional + +from core.imports.paths import sanitize_filename +from core.imports.rematch_hints import RematchHint, quick_file_signature + + +def staged_destination(staging_dir: str, real_path: str, library_track_id: Any) -> str: + """Where the staged copy lands: a single loose file in the staging ROOT (so the + worker treats it as a single-track candidate), named to keep the extension and + be unique + traceable to the track it re-identifies. The filename is cosmetic — + matching is driven by the hint, not the name.""" + base = os.path.basename(real_path) + stem, ext = os.path.splitext(base) + safe_stem = sanitize_filename(stem).strip() or "track" + name = f"{safe_stem} [reid-{library_track_id}]{ext}" + return os.path.join(staging_dir, name) + + +def stage_file_for_reidentify( + real_path: str, + staging_dir: str, + library_track_id: Any, + *, + copy_fn: Callable[[str, str], object] = shutil.copy2, + signature_fn: Callable[[str], Optional[str]] = quick_file_signature, +) -> Dict[str, Any]: + """Copy the library file into staging and fingerprint the copy. Returns + ``{staged_path, content_hash}``. Raises ``FileNotFoundError`` if the source is + gone (caller surfaces a clear error rather than writing a dangling hint).""" + if not real_path or not os.path.isfile(real_path): + raise FileNotFoundError(real_path or "(empty path)") + os.makedirs(staging_dir, exist_ok=True) + dest = staged_destination(staging_dir, real_path, library_track_id) + copy_fn(real_path, dest) + return {"staged_path": dest, "content_hash": signature_fn(dest)} + + +def build_reidentify_hint( + library_track_id: Any, + hint_fields: Dict[str, Any], + staged_path: str, + content_hash: Optional[str], + *, + replace: bool, +) -> RematchHint: + """Pure: assemble the RematchHint from the resolved release fields + staging + info. ``replace_track_id`` is the library row to delete on success, but only + when 'replace original' was ticked. ``exempt_dedup`` is always True — a + re-identify is explicit and must bypass dedup-skip.""" + return RematchHint( + staged_path=staged_path, + content_hash=content_hash, + source=hint_fields.get("source") or "", + isrc=hint_fields.get("isrc"), + track_id=hint_fields.get("track_id"), + album_id=hint_fields.get("album_id"), + artist_id=hint_fields.get("artist_id"), + track_title=hint_fields.get("track_title"), + album_name=hint_fields.get("album_name"), + artist_name=hint_fields.get("artist_name"), + album_type=hint_fields.get("album_type"), + track_number=hint_fields.get("track_number"), + disc_number=hint_fields.get("disc_number"), + replace_track_id=(int(library_track_id) if replace and str(library_track_id).isdigit() else + (library_track_id if replace else None)), + exempt_dedup=True, + ) + + +__all__ = [ + "staged_destination", + "stage_file_for_reidentify", + "build_reidentify_hint", +] diff --git a/core/imports/rematch_hints.py b/core/imports/rematch_hints.py new file mode 100644 index 00000000..8e5d8e4c --- /dev/null +++ b/core/imports/rematch_hints.py @@ -0,0 +1,334 @@ +"""Re-identify hints (#889) — a single-use, user-designated answer to "which +release does this already-imported track belong to". + +Flow: the user clicks *Re-identify* on a library track, searches a source, and +picks the exact release (single / EP / album) it should live under. We write a +**hint** here and stage the file for auto-import. The import flow then reads the +hint at the very TOP of matching — before any fuzzy tier — builds the match from +these exact IDs, and consumes the row. So the original ambiguity that mis-filed +the track (which release?) is gone: the user already answered it. + +Two safety properties live in the hint, not the import code: + +- ``replace_track_id`` — the library row to delete AFTER the re-import lands (so a + re-identify *replaces* rather than *duplicates*). Cleanup is deferred to success + so a failed import can never lose the file. +- ``exempt_dedup`` — always set: a re-identify is an explicit user action and must + not be silently dropped by the quality dedup-skip (which would otherwise see the + incoming file as a duplicate of the very row we're replacing). + +This module is pure DB mechanics over an injected ``cursor`` (sqlite3-style, +``?`` params) — no connection management, no app state — so the create / find / +consume seam is unit-tested against an in-memory DB with no live metadata client. +The binding is keyed on the staged path, with ``content_hash`` as a rename-proof +fallback in case the staging watcher normalizes the filename on ingest. +""" + +from __future__ import annotations + +import os +from dataclasses import dataclass +from typing import Any, Callable, Optional + +# Columns in INSERT/SELECT order — single source of truth so the dataclass, the +# write, and the read can't drift apart. +_FIELDS = ( + "staged_path", + "content_hash", + "source", + "isrc", + "track_id", + "album_id", + "artist_id", + "track_title", + "album_name", + "artist_name", + "album_type", + "track_number", + "disc_number", + "replace_track_id", + "exempt_dedup", +) + + +@dataclass +class RematchHint: + """One user-designated re-identify answer. ``id``/``status`` are set by the DB.""" + staged_path: str + source: str + content_hash: Optional[str] = None + isrc: Optional[str] = None + track_id: Optional[str] = None + album_id: Optional[str] = None + artist_id: Optional[str] = None + track_title: Optional[str] = None + album_name: Optional[str] = None + artist_name: Optional[str] = None + album_type: Optional[str] = None + track_number: Optional[int] = None + disc_number: Optional[int] = None + replace_track_id: Optional[int] = None + exempt_dedup: bool = True + id: Optional[int] = None + status: str = "pending" + + def _values(self) -> tuple: + return ( + self.staged_path, + self.content_hash, + self.source, + self.isrc, + self.track_id, + self.album_id, + self.artist_id, + self.track_title, + self.album_name, + self.artist_name, + self.album_type, + self.track_number, + self.disc_number, + self.replace_track_id, + 1 if self.exempt_dedup else 0, + ) + + +def _row_to_hint(row: Any) -> RematchHint: + """Map a sqlite3.Row (or any mapping/sequence-by-name) to a RematchHint.""" + def g(key, default=None): + try: + return row[key] + except (KeyError, IndexError, TypeError): + return default + return RematchHint( + id=g("id"), + staged_path=g("staged_path") or "", + content_hash=g("content_hash"), + source=g("source") or "", + isrc=g("isrc"), + track_id=g("track_id"), + album_id=g("album_id"), + artist_id=g("artist_id"), + track_title=g("track_title"), + album_name=g("album_name"), + artist_name=g("artist_name"), + album_type=g("album_type"), + track_number=g("track_number"), + disc_number=g("disc_number"), + replace_track_id=g("replace_track_id"), + exempt_dedup=bool(g("exempt_dedup", 1)), + status=g("status") or "pending", + ) + + +def create_hint(cursor: Any, hint: RematchHint) -> int: + """Insert a pending hint; return its new id. Caller owns commit.""" + placeholders = ", ".join("?" for _ in _FIELDS) + cursor.execute( + f"INSERT INTO rematch_hints ({', '.join(_FIELDS)}) VALUES ({placeholders})", + hint._values(), + ) + new_id = cursor.lastrowid + hint.id = new_id + return new_id + + +def find_hint_for_file( + cursor: Any, + staged_path: str, + content_hash: Optional[str] = None, +) -> Optional[RematchHint]: + """Return the newest PENDING hint for a staged file, or ``None``. + + Matched by exact ``staged_path`` first; if that misses and a ``content_hash`` + is given, fall back to it (covers a staging watcher that renamed the file on + ingest). Only ``status='pending'`` rows are returned, so a consumed hint is + never reused.""" + if staged_path: + cursor.execute( + "SELECT * FROM rematch_hints WHERE staged_path = ? AND status = 'pending' " + "ORDER BY id DESC LIMIT 1", + (staged_path,), + ) + row = cursor.fetchone() + if row is not None: + return _row_to_hint(row) + # Try by basename too — the watcher may move the file into a different dir. + base = os.path.basename(staged_path) + if base and base != staged_path: + cursor.execute( + "SELECT * FROM rematch_hints WHERE staged_path LIKE ? AND status = 'pending' " + "ORDER BY id DESC LIMIT 1", + ("%/" + base,), + ) + row = cursor.fetchone() + if row is not None: + return _row_to_hint(row) + + if content_hash: + cursor.execute( + "SELECT * FROM rematch_hints WHERE content_hash = ? AND status = 'pending' " + "ORDER BY id DESC LIMIT 1", + (content_hash,), + ) + row = cursor.fetchone() + if row is not None: + return _row_to_hint(row) + + return None + + +def consume_hint(cursor: Any, hint_id: int) -> None: + """Mark a hint consumed (single-use). Caller owns commit.""" + cursor.execute( + "UPDATE rematch_hints SET status = 'consumed', consumed_at = CURRENT_TIMESTAMP " + "WHERE id = ?", + (hint_id,), + ) + + +def list_pending_hints(cursor: Any) -> list: + """All pending hints (newest first) — for a 'pending re-identify' view and + orphan recovery when a staged file never imports.""" + cursor.execute("SELECT * FROM rematch_hints WHERE status = 'pending' ORDER BY id DESC") + return [_row_to_hint(r) for r in cursor.fetchall()] + + +def build_identification_from_hint(hint: RematchHint) -> dict: + """Turn a hint into the ``identification`` dict the auto-import matcher expects, + so a re-identify SKIPS the guessing tiers entirely and matches straight against + the user-chosen release. Mirrors the shape `_identify_folder` returns (album_id + / source / track_number drive the album fetch + file→track match).""" + return { + "album_id": hint.album_id or None, + "album_name": hint.album_name or hint.track_title or "", + "artist_name": hint.artist_name or "", + "artist_id": hint.artist_id or "", + "track_name": hint.track_title or "", + "track_id": hint.track_id or "", + "image_url": "", + "release_date": "", + "track_number": hint.track_number or 1, + "total_tracks": 1, + "source": hint.source, + "method": "rematch_hint", + "identification_confidence": 1.0, + # is_single reflects the CHOSEN release, but force_album_match makes the + # matcher FETCH that release (even for a lone staged file) instead of taking + # the singles fast-path — so the re-imported track gets the real album + # metadata: year, the correct in-album track number, and the album art. + "is_single": (str(hint.album_type or "").lower() == "single"), + "force_album_match": True, + "album_type": hint.album_type, + } + + +def _canonical(path: Optional[str]) -> str: + """Canonical form of a path for same-file comparison (symlinks + case + sep).""" + if not path: + return "" + try: + return os.path.normcase(os.path.realpath(path)) + except OSError: + return os.path.normcase(os.path.normpath(path)) + + +def delete_replaced_track( + cursor: Any, + replace_track_id: Any, + *, + unlink=os.remove, + resolve_fn: Optional[Callable[[str], Optional[str]]] = None, + new_paths: Optional[list] = None, +) -> Optional[str]: + """Remove the OLD library row a re-identify replaces, and its file. + + Called only AFTER the re-import has landed the track at its new home, so the + original is never lost on failure. Safe by construction: + + * **Same-home guard (CRITICAL):** if the re-import landed at the SAME file as the + old one (``new_paths`` — the paths the import actually wrote), this is a no-op: + we DON'T delete the row or the file, because that file IS the re-imported track. + This is what stops "re-identify to the release it's already in" from deleting + the file (the import reuses the same row, so deleting it would orphan the file). + * the file is unlinked only if it still exists and **no other track row references + it** (guards against yanking a file a different row legitimately points to). + + Returns the path it removed, or ``None`` if there was nothing to do. ``unlink`` is + injectable for tests. ``resolve_fn`` maps the STORED DB path to the file's actual + on-disk location (the stored path may be a Docker/media-server view this process + can't read literally — without it we'd delete the row but orphan the file).""" + if not replace_track_id: + return None + cursor.execute("SELECT file_path FROM tracks WHERE id = ?", (replace_track_id,)) + row = cursor.fetchone() + if row is None: + return None + old_path = (row["file_path"] if not isinstance(row, (tuple, list)) else row[0]) or "" + if not old_path: + cursor.execute("DELETE FROM tracks WHERE id = ?", (replace_track_id,)) + return None + + # Resolve the old stored path to its real on-disk location up front. + real_path = old_path + if resolve_fn is not None: + try: + real_path = resolve_fn(old_path) or old_path + except Exception: + real_path = old_path + + # Same-home guard: if the re-import wrote to this very file, do NOTHING — the row + # is the re-imported track's row and the file is its file. Deleting either would + # be data loss (the "picked the same release" bug). + if new_paths: + landed = {_canonical(p) for p in new_paths if p} + if _canonical(real_path) in landed or _canonical(old_path) in landed: + return None + + cursor.execute("DELETE FROM tracks WHERE id = ?", (replace_track_id,)) + # Only unlink if no surviving row still points at this file (rows store the + # stored path, so compare against the stored path, not the resolved one). + cursor.execute("SELECT 1 FROM tracks WHERE file_path = ? LIMIT 1", (old_path,)) + if cursor.fetchone() is not None: + return None + try: + if os.path.exists(real_path): # real_path resolved above + unlink(real_path) + return real_path + except OSError: + pass + return None + + +def quick_file_signature(path: str, *, chunk: int = 65536) -> Optional[str]: + """A cheap, rename-proof content fingerprint: size + first/last chunk, hashed. + + Audio files are large, so a full hash is wasteful when we only need to re-bind + a hint to *this* file after a possible rename. Size + head + tail is plenty to + distinguish staged files in practice. Returns ``None`` if the file can't be + read (caller falls back to path-only binding).""" + import hashlib + + try: + size = os.path.getsize(path) + h = hashlib.sha256() + h.update(str(size).encode()) + with open(path, "rb") as f: + h.update(f.read(chunk)) + if size > chunk: + f.seek(max(0, size - chunk)) + h.update(f.read(chunk)) + return h.hexdigest() + except OSError: + return None + + +__all__ = [ + "RematchHint", + "create_hint", + "find_hint_for_file", + "consume_hint", + "list_pending_hints", + "build_identification_from_hint", + "delete_replaced_track", + "quick_file_signature", +] diff --git a/core/imports/rematch_search.py b/core/imports/rematch_search.py new file mode 100644 index 00000000..da32c373 --- /dev/null +++ b/core/imports/rematch_search.py @@ -0,0 +1,247 @@ +"""#889 Phase 3: search a metadata source for the releases a track appears on. + +The Re-identify modal lets the user search ANY configured source (tabs, defaulting +to the active one) and shows the SAME song across its different collections — +single / EP / album — so they can pick which release the track should be filed +under. Two steps, deliberately split: + + * ``search_release_candidates(source, query)`` — lightweight DISPLAY rows from the + normal typed ``search_tracks`` (title, artist, release name, type badge, year, + track count, art, ISRC, track_id). No album_id needed to draw the list. + * ``resolve_hint_fields(source, track_id)`` — runs ONCE, on the row the user + picks: ``get_track_details`` yields the album_id / isrc / track#/disc the hint + needs. We don't pay that lookup for every search result, only the chosen one. + +Pure normalization + injected client factory, so the search/normalize/resolve seam +is unit-tested with a fake client and no network. +""" + +from __future__ import annotations + +from typing import Any, Callable, Dict, List, Optional + + +def _get(obj: Any, key: str, default=None): + """Read ``key`` from either an object (attr) or a mapping (item).""" + if obj is None: + return default + if isinstance(obj, dict): + return obj.get(key, default) + return getattr(obj, key, default) + + +def _year(release_date: Any) -> Optional[str]: + s = str(release_date or "").strip() + return s[:4] if len(s) >= 4 and s[:4].isdigit() else None + + +def infer_release_type(album_type: Any, total_tracks: Any) -> str: + """Normalize a source's release type to one of album / ep / single / compilation. + + Sources disagree: Spotify has no 'EP' — EPs come back as ``album_type='single'`` + with several tracks; MusicBrainz/Deezer label EPs properly. So when a 'single' + carries more than a handful of tracks, call it an EP for the badge. The actual + filing is unaffected — that's driven by the real album_id, not this label.""" + t = str(album_type or "").strip().lower() + try: + n = int(total_tracks) if total_tracks is not None else 0 + except (TypeError, ValueError): + n = 0 + if t in ("compilation", "comp"): + return "compilation" + if t == "ep": + return "ep" + if t == "album": + return "album" # an explicit album stays an album; only 'single' gets promoted to EP + if t == "single": + # 1–3 tracks → single; 4+ → almost always an EP in practice. + return "ep" if n >= 4 else "single" + # Unknown type: infer purely from track count. + if n >= 7: + return "album" + if n >= 4: + return "ep" + if n >= 1: + return "single" + return t or "album" + + +def normalize_search_result(result: Any, source: str) -> Optional[Dict[str, Any]]: + """One typed search Track (or raw dict) → a display row, or ``None`` if it has + no usable id/title. ``album`` is just a name at search time; album_id is + resolved later for the picked row only.""" + track_id = _get(result, "id") or _get(result, "track_id") + title = _get(result, "name") or _get(result, "title") + if not track_id or not title: + return None + + artists = _get(result, "artists") + if isinstance(artists, list): + artist_name = ", ".join(str(_get(a, "name", a) if not isinstance(a, str) else a) for a in artists) + else: + artist_name = str(artists or _get(result, "artist") or "") + + album = _get(result, "album") + album_name = album if isinstance(album, str) else (_get(album, "name") or "") + raw_type = _get(result, "album_type") + total = _get(result, "total_tracks") + ext = _get(result, "external_ids") or {} + isrc = _get(result, "isrc") or (ext.get("isrc") if isinstance(ext, dict) else None) + + return { + "source": source, + "track_id": str(track_id), + "track_title": str(title), + "artist_name": artist_name, + "album_name": str(album_name or ""), + "album_type": infer_release_type(raw_type, total), + "raw_album_type": str(raw_type or ""), + "total_tracks": int(total) if isinstance(total, int) else None, + "year": _year(_get(result, "release_date")), + "image_url": _get(result, "image_url") or "", + "isrc": isrc or None, + } + + +def search_release_candidates( + source: str, + query: str, + *, + limit: int = 25, + client_factory: Optional[Callable[[str], Any]] = None, +) -> List[Dict[str, Any]]: + """Search ``source`` for tracks matching ``query`` → normalized display rows. + + Returns ``[]`` (never raises) when the source has no client or errors — the UI + just shows an empty tab. Rows keep duplicate releases; the UI groups them.""" + query = (query or "").strip() + if not query: + return [] + factory = client_factory or _default_client_factory + try: + client = factory(source) + except Exception: + client = None + if client is None or not hasattr(client, "search_tracks"): + return [] + try: + results = client.search_tracks(query, limit=limit) + except TypeError: + results = client.search_tracks(query) # clients with no limit kwarg + except Exception: + return [] + + rows: List[Dict[str, Any]] = [] + for r in results or []: + row = normalize_search_result(r, source) + if row is not None: + rows.append(row) + return rows + + +def resolve_hint_fields( + source: str, + track_id: str, + *, + client_factory: Optional[Callable[[str], Any]] = None, +) -> Optional[Dict[str, Any]]: + """Resolve the picked track to the fields a hint needs (album_id critically, + plus isrc / track# / disc# / album name+type). One lookup for one chosen row. + Returns ``None`` if it can't be resolved (caller surfaces an error).""" + factory = client_factory or _default_client_factory + try: + client = factory(source) + except Exception: + client = None + if client is None or not hasattr(client, "get_track_details"): + return None + try: + details = client.get_track_details(track_id) + except Exception: + return None + if not details: + return None + + album = _get(details, "album") or {} + album_id = _get(album, "id") if not isinstance(album, str) else None + album_name = _get(album, "name") if not isinstance(album, str) else album + album_type = _get(album, "album_type") or _get(details, "album_type") + total = _get(album, "total_tracks") or _get(details, "total_tracks") + + artists = _get(details, "artists") or [] + artist_id = None + artist_name = "" + if isinstance(artists, list) and artists: + artist_id = _get(artists[0], "id") + artist_name = ", ".join(str(_get(a, "name", a) if not isinstance(a, str) else a) for a in artists) + + ext = _get(details, "external_ids") or {} + isrc = _get(details, "isrc") or (ext.get("isrc") if isinstance(ext, dict) else None) + + if not album_id: + return None # without an album_id the import can't fetch the tracklist + + return { + "source": source, + "track_id": str(track_id), + "album_id": str(album_id), + "artist_id": str(artist_id) if artist_id else None, + "track_title": _get(details, "name") or _get(details, "title") or "", + "album_name": str(album_name or ""), + "artist_name": artist_name, + "album_type": infer_release_type(album_type, total), + "track_number": _get(details, "track_number"), + "disc_number": _get(details, "disc_number") or 1, + "isrc": isrc or None, + } + + +def _default_client_factory(source: str): + from core.metadata.registry import get_client_for_source + return get_client_for_source(source) + + +def available_sources() -> List[Dict[str, Any]]: + """The source tabs for the modal: every metadata source with a live client, + the primary one flagged ``active`` so the UI selects it by default.""" + from core.metadata.registry import ( + METADATA_SOURCE_PRIORITY, + get_client_for_source, + get_primary_source, + ) + + try: + primary = get_primary_source() + except Exception: + primary = None + + out: List[Dict[str, Any]] = [] + seen = set() + for src in METADATA_SOURCE_PRIORITY: + if src in seen: + continue + seen.add(src) + try: + client = get_client_for_source(src) + except Exception: + client = None + if client is None or not hasattr(client, "search_tracks"): + continue + out.append({ + "source": src, + "label": src.replace("_", " ").title(), + "active": src == primary, + }) + # Guarantee the primary is selectable + first even if priority ordering missed it. + if primary and not any(s["active"] for s in out): + out.insert(0, {"source": primary, "label": primary.replace("_", " ").title(), "active": True}) + return out + + +__all__ = [ + "infer_release_type", + "normalize_search_result", + "search_release_candidates", + "resolve_hint_fields", + "available_sources", +] diff --git a/core/imports/side_effects.py b/core/imports/side_effects.py index 20fe0017..867329c3 100644 --- a/core/imports/side_effects.py +++ b/core/imports/side_effects.py @@ -565,19 +565,22 @@ def record_soulsync_library_entry(context: Dict[str, Any], artist_context: Dict[ # ── Album row: same insert-or-fill-empty-fields shape ── album_source_col = source_columns.get("album") - cursor.execute( - "SELECT id FROM albums WHERE id = ? AND server_source = 'soulsync'", - (album_id,), + # Group by CANONICAL release id when we have one (not just the name + # string), so differently-named imports of the SAME release land in + # one album row instead of splitting — which left the repair jobs + # dressing each split row in its own cover art (Sokhi). Precedence: + # name-hash id -> source release id -> (title, artist). Falls back to + # the legacy name match, so nothing that grouped before stops now. + from core.imports.album_grouping import find_existing_soulsync_album_id + existing_album_id = find_existing_soulsync_album_id( + cursor, name_key_id=album_id, artist_id=artist_id, album_name=album_name, + album_source_col=album_source_col, album_source_id=album_source_id, ) - row = cursor.fetchone() - if not row: - cursor.execute( - "SELECT id FROM albums WHERE title COLLATE NOCASE = ? AND artist_id = ? AND server_source = 'soulsync' LIMIT 1", - (album_name, artist_id), - ) - row = cursor.fetchone() - if row: - album_id = row[0] + if existing_album_id is not None: + album_id = existing_album_id + row = (album_id,) + else: + row = None if row: _fill_empty_columns( diff --git a/core/imports/single_to_album.py b/core/imports/single_to_album.py new file mode 100644 index 00000000..e6fc5f2d --- /dev/null +++ b/core/imports/single_to_album.py @@ -0,0 +1,124 @@ +"""Single -> parent-album resolution. + +When a track is matched to a SINGLE release (album_type 'single', the single's +name usually equal to the track title), it carries the single's name + the +single's source album id. The canonical grouping in +[core/imports/album_grouping.py] then files it under a different album row than +its album-mates, and the album-grouped repair jobs dress that row in the +single's art — songs of one album end up with different covers (Sokhi). + +This module re-homes such a track onto the ALBUM it actually belongs to, so it +carries the album's name/id and groups with the rest of the album. + +Design: the SELECTION is a pure, conservative function (no I/O), and the lookup +loop takes INJECTED fetchers, so both are unit-testable without a live metadata +client. CONSERVATIVE by intent — it only re-homes a track when a real +``album``-type release's tracklist *contains that exact track*. It never +promotes a genuine standalone single and never guesses, because a wrong +promotion would mis-home a real single onto an album (the inverse bug). +""" + +from __future__ import annotations + +import re +from typing import Any, Callable, Dict, List, Optional + +_WS = re.compile(r"\s+") +# Trailing version qualifiers that differ between a single and its album cut but +# don't change track identity (kept conservative — only the obvious ones). +_QUALIFIER = re.compile( + r"\s*[\(\[]\s*(album version|single version|radio edit|remaster(ed)?( \d{4})?)\s*[\)\]]\s*$", + re.IGNORECASE, +) + + +def _norm(s: Any) -> str: + """Lowercase, strip a trailing '(Album Version)'-style qualifier, collapse + whitespace — so 'Song' matches 'Song (Album Version)'.""" + t = str(s or "").strip().lower() + t = _QUALIFIER.sub("", t) + return _WS.sub(" ", t).strip() + + +def _get(obj: Any, *keys: str, default=None): + for k in keys: + if isinstance(obj, dict): + if obj.get(k) is not None: + return obj.get(k) + else: + v = getattr(obj, k, None) + if v is not None: + return v + return default + + +def select_parent_album(track_title: str, candidate_albums: List[Dict[str, Any]]) -> Optional[Dict[str, Any]]: + """Pick the parent ALBUM for ``track_title`` from normalized candidates, or + None. Each candidate is ``{name, album_type, tracks: [title, ...], ...}``. + + Conservative rules — a candidate qualifies ONLY when: + * it is an ``album`` release (never single / ep / compilation), and + * its name is not just the track title (that IS the single), and + * its tracklist contains the track by exact normalized title. + Returns the FIRST qualifying candidate (caller passes them in priority + order, so the result is deterministic). + """ + tgt = _norm(track_title) + if not tgt: + return None + for alb in candidate_albums or []: + if str(_get(alb, "album_type", default="album")).lower() != "album": + continue + if _norm(_get(alb, "name", "title", default="")) == tgt: + continue + tracks = _get(alb, "tracks", default=[]) or [] + if any(_norm(t) == tgt for t in tracks): + return alb + return None + + +def resolve_single_to_album( + track_title: str, + *, + fetch_album_candidates: Callable[[], List[Dict[str, Any]]], + fetch_album_tracks: Callable[[Dict[str, Any]], List[str]], + max_albums: int = 8, +) -> Optional[Dict[str, Any]]: + """Find the parent album for a single-matched track. I/O is INJECTED so this + is testable without a live client: + * ``fetch_album_candidates()`` -> the artist's ALBUM-type releases (dicts + with name/album_type/id/source), in priority order. + * ``fetch_album_tracks(album)`` -> that album's track titles. + Probes at most ``max_albums`` albums, lazily (stops at the first that + contains the track). Fail-safe: any error / no confident match -> None + (the track stays as it was matched). Returns the normalized winning album + ``{name, album_type, album_id, source, tracks}`` or None. + """ + if not _norm(track_title): + return None + try: + albums = fetch_album_candidates() or [] + except Exception: + return None + + probed = 0 + for alb in albums: + if str(_get(alb, "album_type", default="album")).lower() != "album": + continue + if probed >= max_albums: + break + probed += 1 + try: + tracks = fetch_album_tracks(alb) or [] + except Exception: + continue + normalized = { + "name": _get(alb, "name", "title", default=""), + "album_type": "album", + "album_id": _get(alb, "id", "album_id"), + "source": _get(alb, "source"), + "tracks": list(tracks), + } + if select_parent_album(track_title, [normalized]): + return normalized + return None diff --git a/core/library/residual_files.py b/core/library/residual_files.py new file mode 100644 index 00000000..5a15831d --- /dev/null +++ b/core/library/residual_files.py @@ -0,0 +1,53 @@ +"""What counts as a *residual* file — a leftover with no value once the audio it +accompanied is gone: OS junk, cover/scan images, and lyric/metadata sidecars. + +Single source of truth shared by: + * the **Reorganize** cleanup, which strips these from a source dir after every + track has moved out (so the empty-dir pruner can take the folder), and + * the **Empty Folder Cleaner** job, which can optionally treat a folder holding + ONLY residual files as removable (#891). + +Defining "disposable" in one place keeps the two features agreeing on what a "dead +folder" is. Pure predicates — no filesystem access — so they're unit-tested in +isolation. The whitelist is deliberately conservative: anything NOT recognized here +(a booklet ``.pdf``, a video, a ``.txt`` note) is treated as real content and kept. +""" + +from __future__ import annotations + +import os + +# OS / tooling junk. +JUNK_FILES = {'.ds_store', 'thumbs.db', 'desktop.ini', '.directory', 'album.nfo~'} +# Cover art + booklet scans. +IMAGE_EXTS = {'.jpg', '.jpeg', '.png', '.gif', '.bmp', '.webp', '.tiff', '.tif'} +# Lyric / metadata / playlist sidecars that are worthless without their audio. +SIDECAR_EXTS = {'.lrc', '.nfo', '.cue', '.m3u', '.m3u8'} + + +def _ext(name: str) -> str: + return os.path.splitext(name or '')[1].lower() + + +def is_junk(name: str) -> bool: + return (name or '').lower() in JUNK_FILES + + +def is_image(name: str) -> bool: + return _ext(name) in IMAGE_EXTS + + +def is_sidecar(name: str) -> bool: + return _ext(name) in SIDECAR_EXTS + + +def is_disposable(name: str) -> bool: + """True if this file is junk, a cover/scan image, or a lyric/metadata sidecar — + i.e. safe to delete from a folder that has no audio left.""" + return is_junk(name) or is_image(name) or is_sidecar(name) + + +__all__ = [ + 'JUNK_FILES', 'IMAGE_EXTS', 'SIDECAR_EXTS', + 'is_junk', 'is_image', 'is_sidecar', 'is_disposable', +] diff --git a/core/library_reorganize.py b/core/library_reorganize.py index e2991682..a5c9e4cd 100644 --- a/core/library_reorganize.py +++ b/core/library_reorganize.py @@ -1847,14 +1847,8 @@ def _prune_empty_album_dirs(artist_dir: str) -> None: # Sidecars that live alongside ONE audio file (same filename stem). _TRACK_SIDECAR_EXTS = ('.lrc', '.nfo', '.txt', '.cue', '.json') -# Sidecars that live at the ALBUM level (one per directory). -_ALBUM_SIDECARS = ( - 'cover.jpg', 'cover.jpeg', 'cover.png', - 'folder.jpg', 'folder.png', - 'front.jpg', 'front.png', - 'album.jpg', 'album.png', - 'artwork.jpg', 'artwork.png', -) +# Album-level leftovers (cover images, .lrc, etc.) are classified by the shared +# `core.library.residual_files.is_disposable` predicate — see `_delete_album_sidecars`. # Audio extensions used to decide whether a source directory still has # tracks the user might care about (i.e. a per-track failure left audio @@ -1954,16 +1948,30 @@ def _delete_track_sidecars(audio_path: str) -> None: def _delete_album_sidecars(src_dir: str) -> None: - """Delete album-level sidecars (cover.jpg, folder.jpg, etc.) from - `src_dir`. Used during end-of-run cleanup when no audio files remain - in the directory. Best-effort — individual failures are debug-logged.""" - for name in _ALBUM_SIDECARS: - sidecar = os.path.join(src_dir, name) - if os.path.isfile(sidecar): + """Delete album-level *residual* files from ``src_dir`` — any cover/scan image, + lyric/metadata sidecar (.lrc/.nfo/.cue/.m3u), or OS junk. Called during + end-of-run cleanup ONLY when no audio remains in the directory, so everything + here is leftover from the album that just moved (#891 — previously this only + removed a fixed list of cover names, so ``back.jpg`` / ``disc.jpg`` / ``.webp`` + survived and kept the folder un-prunable). + + Uses the shared ``is_disposable`` predicate so it agrees with the Empty Folder + Cleaner on what's a dead leftover; anything unrecognized (a booklet ``.pdf``, a + video) is deliberately LEFT. Best-effort — individual failures are debug-logged.""" + from core.library.residual_files import is_disposable + try: + entries = os.listdir(src_dir) + except OSError: + return + for name in entries: + if not is_disposable(name): + continue + full = os.path.join(src_dir, name) + if os.path.isfile(full): try: - os.remove(sidecar) + os.remove(full) except OSError as e: - logger.debug(f"[Reorganize] Couldn't remove album sidecar {sidecar}: {e}") + logger.debug(f"[Reorganize] Couldn't remove residual file {full}: {e}") def _has_remaining_audio(directory: str) -> bool: diff --git a/core/metadata/enrichment.py b/core/metadata/enrichment.py index fdd34042..e3642419 100644 --- a/core/metadata/enrichment.py +++ b/core/metadata/enrichment.py @@ -242,18 +242,25 @@ def enhance_file_metadata(file_path: str, context: dict, artist: dict, album_inf logger.warning("[Metadata Debug] Artist: %s", artist.get("name", "MISSING") if artist else "None") logger.warning("[Metadata Debug] Album info: %s", album_info.get("album_name", "MISSING") if album_info else "None") logger.error("[Metadata Debug] Traceback:\n%s", traceback.format_exc()) - # We cleared the file's art early; if the rewrite then crashed - # before re-embedding, the on-disk file (already saved cleared at - # the start) would be left art-less. Best-effort: put the original - # art back and persist it so a mid-enrichment crash never destroys - # the cover (#764). Guarded so a failure here can't mask the - # original error. + # The file was saved with tags CLEARED up front (so stale tags never + # linger), then the failure-prone enrichment ran. By the time most + # failures hit — the external source-id embed / cover-art fetch — the + # core tags (album/artist/title/track from the matched context) are + # already on the in-memory object but NOT yet on disk; the on-disk + # file is still the cleared one. Persist the in-memory tags now (and + # restore the original art too, #764) so a mid-enrichment crash leaves + # a correctly-tagged file instead of an UNTAGGED one (Sokhi: tracks + # landing in Rockbox's 'untagged' bucket after a 'processing failed'). + # + # Previously this save was gated on there being original art to + # restore, so an art-less file lost its tags entirely on any crash. + # Guarded so a failure here can't mask the original error. try: - if audio_file is not None and art_snapshot and restore_embedded_art( - audio_file, symbols, art_snapshot - ): + if audio_file is not None: + if art_snapshot: + restore_embedded_art(audio_file, symbols, art_snapshot) save_audio_file(audio_file, symbols) - logger.info("Restored original cover art after enrichment error.") + logger.info("Persisted core tags (and restored art) after enrichment error.") except Exception as restore_exc: - logger.debug("Art restore after error failed: %s", restore_exc) + logger.debug("Tag/art persist after error failed: %s", restore_exc) return False diff --git a/core/repair_jobs/empty_folder_cleaner.py b/core/repair_jobs/empty_folder_cleaner.py index 0f8f16e3..fe1f5d02 100644 --- a/core/repair_jobs/empty_folder_cleaner.py +++ b/core/repair_jobs/empty_folder_cleaner.py @@ -22,37 +22,36 @@ from __future__ import annotations import os from typing import Iterable, List +from core.library.residual_files import JUNK_FILES, is_disposable, is_junk # noqa: F401 — JUNK_FILES/is_junk re-exported from core.repair_jobs import register_job from core.repair_jobs.base import JobContext, JobResult, RepairJob from utils.logging_config import get_logger logger = get_logger("repair_jobs.empty_folder_cleaner") -# Files that don't count as real content — safe to delete along with the folder. -JUNK_FILES = {'.ds_store', 'thumbs.db', 'desktop.ini', '.directory', 'album.nfo~'} - - -def is_junk(name: str) -> bool: - return (name or '').lower() in JUNK_FILES - def dir_is_removable(files: Iterable[str], surviving_subdirs: Iterable[str], - *, ignore_junk: bool = True) -> bool: + *, ignore_junk: bool = True, ignore_disposable: bool = False) -> bool: """Pure: is a directory safe to remove? Removable iff it has **no surviving subdirectories** and **no real files** — where "no real files" means literally empty, or (when ``ignore_junk``) only - OS-junk files. ``surviving_subdirs`` is the list of child dirs that are NOT - themselves being removed (i.e. still hold content). + OS-junk files, or (when ``ignore_disposable`` — #891) only *residual* files: + junk + cover/scan images + lyric/metadata sidecars. ``ignore_disposable`` is the + broader opt-in that clears the cover.jpg-only folders a reorganize leaves behind. + ``surviving_subdirs`` is the list of child dirs that are NOT themselves being + removed (i.e. still hold content). """ if list(surviving_subdirs): return False files = list(files) if not files: return True - if not ignore_junk: - return False - return all(is_junk(f) for f in files) + if ignore_disposable: + return all(is_disposable(f) for f in files) + if ignore_junk: + return all(is_junk(f) for f in files) + return False @register_job @@ -65,15 +64,18 @@ class EmptyFolderCleanerJob(RepairJob): 'relocations, and deletions — empty artist/album folders, or folders that ' 'hold only OS junk like .DS_Store / Thumbs.db.\n\n' 'A finding is created for each. Applying one deletes the folder (after ' - 're-checking it is still empty). Folders that contain any real file — a ' - 'cover image, an audio track, anything — are never touched, the library ' - 'root is never removed, and it cascades: a folder left empty once its ' - 'empty children are removed is cleaned too.' + 're-checking it is still empty). Folders that contain any real file — an ' + 'audio track, a booklet, anything not recognized as a leftover — are never ' + 'touched, the library root is never removed, and it cascades: a folder left ' + 'empty once its empty children are removed is cleaned too.\n\n' + 'Enable "Also remove image/sidecar-only folders" to clear the cover.jpg / ' + '.lrc leftovers a Library Reorganize leaves behind — folders whose only ' + 'remaining files are cover/scan images or lyric/metadata sidecars.' ) icon = 'repair-icon-folder' default_enabled = False default_interval_hours = 168 # weekly — empties accrue slowly - default_settings = {'remove_junk_files': True} + default_settings = {'remove_junk_files': True, 'remove_residual_files': False} auto_fix = False def scan(self, context: JobContext) -> JobResult: @@ -86,10 +88,15 @@ class EmptyFolderCleanerJob(RepairJob): root = os.path.realpath(root) ignore_junk = True + ignore_disposable = False try: if context.config_manager: ignore_junk = bool(context.config_manager.get( 'repair.jobs.empty_folder_cleaner.remove_junk_files', True)) + # #891: also clear folders left holding only images / .lrc / sidecars + # (what a reorganize leaves behind). Opt-in — default off. + ignore_disposable = bool(context.config_manager.get( + 'repair.jobs.empty_folder_cleaner.remove_residual_files', False)) except Exception: # noqa: S110 — setting read is best-effort; defaults to True pass @@ -108,17 +115,29 @@ class EmptyFolderCleanerJob(RepairJob): surviving = [d for d in dirnames if os.path.join(dirpath, d) not in flagged] - if not dir_is_removable(filenames, surviving, ignore_junk=ignore_junk): + if not dir_is_removable(filenames, surviving, + ignore_junk=ignore_junk, ignore_disposable=ignore_disposable): result.skipped += 1 continue flagged.add(dirpath) junk = [f for f in filenames if is_junk(f)] + # Files that will be swept along with the folder (junk always; images/ + # sidecars only when the residual option is on). + purgeable = [f for f in filenames + if is_junk(f) or (ignore_disposable and is_disposable(f))] + residual = [f for f in purgeable if not is_junk(f)] rel = os.path.relpath(dirpath, root) if context.report_progress: context.report_progress(log_line=f'Empty folder: {rel}', log_type='info') if context.create_finding: try: + if residual: + extra = f' (only {len(residual)} leftover image/sidecar file(s))' + elif junk: + extra = f' (only {len(junk)} junk file(s))' + else: + extra = '' inserted = context.create_finding( job_id=self.job_id, finding_type='empty_folder', @@ -127,13 +146,13 @@ class EmptyFolderCleanerJob(RepairJob): entity_id=dirpath, file_path=dirpath, title=f'Empty folder: {os.path.basename(dirpath) or rel}', - description=(f'"{rel}" holds no music' - + (f' (only {len(junk)} junk file(s))' if junk else '') - + ' — safe to remove.'), + description=(f'"{rel}" holds no music' + extra + ' — safe to remove.'), details={ 'folder_path': dirpath, 'junk_files': junk, + 'purgeable_files': purgeable, 'remove_junk': ignore_junk, + 'remove_disposable': ignore_disposable, }) if inserted: result.findings_created += 1 @@ -158,12 +177,17 @@ class EmptyFolderCleanerJob(RepairJob): def remove_empty_folder(folder_path: str, *, junk_files: List[str], remove_junk: bool, - root: str, listdir, isdir, islink, remove_file, rmdir) -> dict: + root: str, listdir, isdir, islink, remove_file, rmdir, + remove_disposable: bool = False) -> dict: """Pure-ish orchestration for the apply handler — RE-CHECKS the folder is still - removable, then deletes any junk + the folder. Effects injected for testing. + removable, then deletes any purgeable leftovers + the folder. Effects injected + for testing. - Returns ``{'removed': bool, 'error': str|None}``. Refuses to touch the root, a - symlink, a non-dir, or a folder that gained real content since the scan. + With ``remove_disposable`` (#891) the re-check also treats cover images and + lyric/metadata sidecars as removable, and sweeps them before rmdir. Returns + ``{'removed': bool, 'error': str|None}``. Refuses to touch the root, a symlink, a + non-dir, or a folder that gained REAL content (audio, a booklet, anything not + recognized as residual) since the scan. """ if not folder_path or not isdir(folder_path): return {'removed': False, 'error': 'Folder no longer exists'} @@ -172,19 +196,21 @@ def remove_empty_folder(folder_path: str, *, junk_files: List[str], remove_junk: if root and os.path.realpath(folder_path) == os.path.realpath(root): return {'removed': False, 'error': 'Refusing to remove the library root'} - # Re-check at apply time: only junk/empty now? (Anything else = leave it.) + def _purgeable(e: str) -> bool: + return (remove_junk and is_junk(e)) or (remove_disposable and is_disposable(e)) + + # Re-check at apply time: only purgeable leftovers now? (Anything else = leave it.) entries = list(listdir(folder_path)) - real_entries = [e for e in entries if not (remove_junk and is_junk(e))] + real_entries = [e for e in entries if not _purgeable(e)] if real_entries: return {'removed': False, 'error': 'Folder is no longer empty — left untouched'} - if remove_junk: - for j in entries: - if is_junk(j): - try: - remove_file(os.path.join(folder_path, j)) - except Exception: # noqa: S110 — junk best-effort; rmdir below fails loudly if blocked - pass + for e in entries: + if _purgeable(e): + try: + remove_file(os.path.join(folder_path, e)) + except Exception: # noqa: S110 — leftover best-effort; rmdir below fails loudly if blocked + pass try: rmdir(folder_path) except Exception as e: diff --git a/core/repair_worker.py b/core/repair_worker.py index 519b04ed..9a552a5f 100644 --- a/core/repair_worker.py +++ b/core/repair_worker.py @@ -17,7 +17,7 @@ import threading import time import uuid from difflib import SequenceMatcher -from datetime import datetime, timedelta +from datetime import datetime, timedelta, timezone from typing import Any, Dict, List, Optional, Tuple from core.metadata_service import ( @@ -585,13 +585,29 @@ class RepairWorker: logger.info("Repair worker thread finished") + @staticmethod + def _hours_since(finished_at_iso: str, now_utc: datetime) -> float: + """Hours between a stored ``finished_at`` and ``now_utc``, both in UTC. + + ``finished_at`` is written by SQLite's CURRENT_TIMESTAMP, which is ALWAYS + UTC (and naive). #885: the scheduler compared it against ``datetime.now()`` + (naive LOCAL), so the local↔UTC offset leaked into the elapsed time. For a + zone AHEAD of UTC (Australia/Sydney = +11) every job looked ~11h stale and + fired every poll; behind UTC (the Americas) it just waited too long. Parse + the naive timestamp AS UTC and subtract a UTC ``now`` so scheduling is + timezone-independent.""" + dt = datetime.fromisoformat(finished_at_iso) + if dt.tzinfo is None: + dt = dt.replace(tzinfo=timezone.utc) + return (now_utc - dt).total_seconds() / 3600 + def _pick_next_job(self) -> Optional[str]: """Pick the next job to run based on staleness priority. Returns job_id of the stalest job whose interval has elapsed, or None if nothing is due. """ - now = datetime.now() + now = datetime.now(timezone.utc) best_job_id = None best_staleness = -1 @@ -613,8 +629,7 @@ class RepairWorker: continue try: - last_finished = datetime.fromisoformat(last_run['finished_at']) - elapsed_hours = (now - last_finished).total_seconds() / 3600 + elapsed_hours = self._hours_since(last_run['finished_at'], now) if elapsed_hours < interval_hours: continue # Not due yet @@ -1552,6 +1567,7 @@ class RepairWorker: resolved, junk_files=details.get('junk_files') or [], remove_junk=bool(details.get('remove_junk', True)), + remove_disposable=bool(details.get('remove_disposable', False)), root=self.transfer_folder, listdir=os.listdir, isdir=os.path.isdir, islink=os.path.islink, remove_file=os.remove, rmdir=os.rmdir, diff --git a/core/soulseek_client.py b/core/soulseek_client.py index 9843b21e..28c92393 100644 --- a/core/soulseek_client.py +++ b/core/soulseek_client.py @@ -426,8 +426,11 @@ class SoulseekClient(DownloadSourcePlugin): if f'.{file_ext}' not in audio_extensions: continue - quality = file_ext if file_ext in ['flac', 'mp3', 'ogg', 'aac', 'wma'] else 'unknown' - + # .m4a is the usual AAC container — bucket it as 'aac' (the + # quality filter treats AAC as an opt-in tier; off by default). + quality = 'aac' if file_ext == 'm4a' else ( + file_ext if file_ext in ['flac', 'mp3', 'ogg', 'aac', 'wma'] else 'unknown') + # Create TrackResult # Convert duration from seconds to milliseconds (slskd returns seconds, Spotify uses ms) raw_duration = file_data.get('length') @@ -1147,7 +1150,9 @@ class SoulseekClient(DownloadSourcePlugin): ext = Path(filename).suffix.lower() if ext not in audio_extensions: continue - quality = ext.lstrip('.') if ext.lstrip('.') in ['flac', 'mp3', 'ogg', 'aac', 'wma'] else 'unknown' + _qext = ext.lstrip('.') + quality = 'aac' if _qext == 'm4a' else ( + _qext if _qext in ['flac', 'mp3', 'ogg', 'aac', 'wma'] else 'unknown') raw_duration = file_data.get('length') duration_ms = raw_duration * 1000 if raw_duration else None results.append(TrackResult( @@ -1957,6 +1962,7 @@ class SoulseekClient(DownloadSourcePlugin): 'mp3_320': (1, 50), 'mp3_256': (1, 40), 'mp3_192': (1, 30), + 'aac': (1, 50), 'other': (0, 500), } @@ -2040,6 +2046,7 @@ class SoulseekClient(DownloadSourcePlugin): 'mp3_320': [], 'mp3_256': [], 'mp3_192': [], + 'aac': [], 'other': [] } @@ -2067,6 +2074,17 @@ class SoulseekClient(DownloadSourcePlugin): else: quality_buckets['other'].append(candidate) continue + elif track_format in ('aac', 'm4a'): + # Opt-in AAC tier. ADDITIVE: when AAC isn't enabled in the + # profile (the default, and every profile that predates this), + # route exactly where AAC went before — the 'other' bucket — so + # behaviour is byte-identical. Only a user who turns AAC on lets + # it become a first-class, selectable tier. + aac_cfg = profile['qualities'].get('aac') + if not (aac_cfg and aac_cfg.get('enabled')): + quality_buckets['other'].append(candidate) + continue + quality_key = 'aac' else: quality_buckets['other'].append(candidate) continue diff --git a/core/spotify_worker.py b/core/spotify_worker.py index 0f2fcd54..4693cd40 100644 --- a/core/spotify_worker.py +++ b/core/spotify_worker.py @@ -47,6 +47,12 @@ class SpotifyWorker: # Current item being processed (for UI tooltip) self.current_item = None + # Whether the worker is serving via the no-creds Spotify Free source as of + # its last loop iteration. Cached from the loop's _free_active() probe so + # get_stats() can report it without an auth API call (#887: a no-auth user + # whose enrichment runs on Free was shown "Not Authenticated"). + self._serving_via_free = False + # Statistics self.stats = { 'matched': 0, @@ -136,8 +142,14 @@ class SpotifyWorker: # real-API daily budget (the worker set _budget_exhausted_use_free — # a cheap attribute read). Lets the UI show "Running (Spotify Free)" # instead of a misleading "rate limited" / "daily limit reached". + # #887: the loop's cached _free_active() result is the comprehensive + # signal — it's True for a no-auth user enriching via Spotify Free by + # default (prefer-free is on unless disabled), not just the rate-limit + # / budget bridges. The extra terms stay as a fallback for the brief + # window before the loop's first iteration sets the cache. using_free = bool( - (rate_limited and self.client.is_spotify_metadata_available()) + getattr(self, '_serving_via_free', False) + or (rate_limited and self.client.is_spotify_metadata_available()) or getattr(self.client, '_budget_exhausted_use_free', False) ) except Exception: @@ -264,6 +276,9 @@ class SpotifyWorker: free_serving = self.client._free_active() except Exception: free_serving = False + # Cache for get_stats() so the dashboard status reflects that the + # worker IS enriching via Free even with no official auth (#887). + self._serving_via_free = free_serving # Daily budget guard — pause ONLY when the budget is spent AND we # can't serve via free (no free available). Otherwise free took over. diff --git a/core/text/title_match.py b/core/text/title_match.py index 36818fcd..8d1d1080 100644 --- a/core/text/title_match.py +++ b/core/text/title_match.py @@ -193,9 +193,18 @@ def numeric_tokens_differ(title_a: str, title_b: str) -> bool: string similarity ('Vol.4' vs 'Vol.4.5' = 0.97) and token-subset checks both wave these through, which hung volume 4.5's cover art on volume 4 (Sokhi). Shared digits on both sides ('1989' vs '1989 (Deluxe)') are - fine.""" + fine. + + Tokenises on non-word runs but KEEPS word characters of every script, so a + digit glued to a non-latin word stays its own digit-bearing token. Stripping + to [a-z0-9] turned CJK into spaces, collapsing 'サウンドトラック2' to a bare + '2' that a shared number elsewhere ('第2期' = season 2) already covered — so + 'Soundtrack' and 'Soundtrack2' both reduced to {'2'} and matched, hanging the + wrong cover (Sokhi again).""" def _digit_tokens(text: str) -> frozenset: - tokens = re.sub(r"[^a-z0-9]+", " ", (text or "").casefold()).split() + # \W is Unicode-aware for str: CJK/kana count as word chars, so a digit + # stays attached to its word instead of collapsing to a bare '2'. + tokens = re.sub(r"\W+", " ", (text or "").casefold()).split() return frozenset(t for t in tokens if any(c.isdigit() for c in t)) return _digit_tokens(title_a) != _digit_tokens(title_b) diff --git a/core/usenet_clients/nzbget.py b/core/usenet_clients/nzbget.py index d58074ff..b8c30dad 100644 --- a/core/usenet_clients/nzbget.py +++ b/core/usenet_clients/nzbget.py @@ -198,7 +198,15 @@ class NZBGetAdapter: size=size_bytes, downloaded=downloaded_bytes, download_speed=speed, - save_path=group.get('DestDir'), + # A QUEUED group's DestDir is the in-progress intermediate dir + # (NZBGet names it '.#' and empties/renames it once + # the move completes). Never offer it as a final save_path — finalize + # only from the HISTORY entry (real FinalDir/DestDir). Otherwise a + # PP_FINISHED group (which maps to 'completed') would finalize on the + # incomplete '....#2141' dir, which is then gone -> "No audio files + # found in /…/incomplete/….#2141" (Swigs). Mirrors the SAB adapter + # ignoring its incomplete_path. + save_path=None, category=group.get('Category'), ) @@ -217,7 +225,15 @@ class NZBGetAdapter: size=size_bytes, downloaded=size_bytes if not is_failed else 0, download_speed=0, - save_path=entry.get('DestDir'), + # Prefer FinalDir — the location after a post-processing script (or + # NZBGet's own move) relocated the files; DestDir can still point at + # the intermediate '….#NZBID' dir. Swigs: files landed in + # /data/soulseek/… (FinalDir) while DestDir stayed /…/incomplete/….#2141. + # Fall back to DestDir when FinalDir is empty (no PP move). Empty/ + # whitespace -> None so the plugin waits for a real path. + save_path=(str(entry.get('FinalDir') or '').strip() + or str(entry.get('DestDir') or '').strip() + or None), category=entry.get('Category'), error=status_field if is_failed else None, ) diff --git a/database/music_database.py b/database/music_database.py index 30d2c2b3..8691e8d1 100644 --- a/database/music_database.py +++ b/database/music_database.py @@ -750,6 +750,41 @@ class MusicDatabase: cursor.execute("CREATE INDEX IF NOT EXISTS idx_aih_status ON auto_import_history (status)") cursor.execute("CREATE INDEX IF NOT EXISTS idx_aih_folder_hash ON auto_import_history (folder_hash)") + # Re-identify hints (#889) — a user-designated, single-use answer to "which + # release does this track belong to". Written when the user picks a release in + # the Re-identify modal and the file is staged for auto-import; the import flow + # reads the hint at the TOP of matching (keyed by staged path, content_hash as a + # rename-proof fallback), expedites the match to these exact IDs, then consumes + # the row. `replace_track_id` (when set) is the library row to delete AFTER the + # re-import lands; `exempt_dedup` is always 1 because a re-identify is an explicit + # user action that must not be silently dropped by the quality dedup-skip. + cursor.execute(""" + CREATE TABLE IF NOT EXISTS rematch_hints ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + staged_path TEXT NOT NULL, + content_hash TEXT, + source TEXT NOT NULL, + isrc TEXT, + track_id TEXT, + album_id TEXT, + artist_id TEXT, + track_title TEXT, + album_name TEXT, + artist_name TEXT, + album_type TEXT, + track_number INTEGER, + disc_number INTEGER, + replace_track_id INTEGER, + exempt_dedup INTEGER NOT NULL DEFAULT 1, + status TEXT NOT NULL DEFAULT 'pending', + created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, + consumed_at TIMESTAMP + ) + """) + cursor.execute("CREATE INDEX IF NOT EXISTS idx_rmh_staged_path ON rematch_hints (staged_path)") + cursor.execute("CREATE INDEX IF NOT EXISTS idx_rmh_content_hash ON rematch_hints (content_hash)") + cursor.execute("CREATE INDEX IF NOT EXISTS idx_rmh_status ON rematch_hints (status)") + # Sync history table — tracks the last 100 sync operations with cached context for re-trigger cursor.execute(""" CREATE TABLE IF NOT EXISTS sync_history ( @@ -8676,6 +8711,15 @@ class MusicDatabase: "min_kbps": 150, "max_kbps": 300, "priority": 4 + }, + # AAC (incl. .m4a): opt-in, OFF by default. Priority 1.5 sits it + # above MP3 but below FLAC (AAC is more efficient than MP3); the + # min_kbps gate keeps junk-bitrate AAC from beating a good MP3. + "aac": { + "enabled": False, + "min_kbps": 128, + "max_kbps": 400, + "priority": 1.5 } }, "fallback_enabled": True @@ -8725,6 +8769,12 @@ class MusicDatabase: "min_kbps": 150, "max_kbps": 300, "priority": 4 + }, + "aac": { + "enabled": False, + "min_kbps": 128, + "max_kbps": 400, + "priority": 1.5 } }, "fallback_enabled": False @@ -8757,6 +8807,12 @@ class MusicDatabase: "min_kbps": 150, "max_kbps": 300, "priority": 4 + }, + "aac": { + "enabled": False, + "min_kbps": 128, + "max_kbps": 400, + "priority": 1.5 } }, "fallback_enabled": True @@ -8789,6 +8845,14 @@ class MusicDatabase: "min_kbps": 150, "max_kbps": 300, "priority": 3 + }, + # Space-saver favours small files, where AAC shines — but it + # still ships OFF (opt-in). Priority 0.5 puts it above MP3. + "aac": { + "enabled": False, + "min_kbps": 128, + "max_kbps": 400, + "priority": 0.5 } }, "fallback_enabled": True diff --git a/pr_description.md b/pr_description.md index 6b9f791d..365c2dfa 100644 --- a/pr_description.md +++ b/pr_description.md @@ -1,116 +1,37 @@ -# SoulSync 2.6.4 — Merge `dev` → `main` +# soulsync 2.7.4 — `dev` → `main` -Patch release on top of 2.6.3. Headline: - -- **#721 — Usenet album bundles stuck on "downloading release" when SAB History flips before storage lands.** Reported by @IamGroot60 against 2.6.3, validated on the `fix/usenet-bundle-save-path-handoff` branch, merged via PR #723. - -Everything from 2.6.3 also rolls in unchanged (was bumped on dev but never tagged / published to main / docker, so this is the first time these changes reach users). +patch release on top of 2.7.3. headline is **re-identify** — re-file an already-imported track under the right release without re-downloading it. --- -## 2.6.4 — the patch +## what's new -### #721 — Usenet album bundle stuck on "downloading release" when SAB History flips before `storage` lands -Follow-up to the 2.6.3 queue→history handoff fix (#706). 2.6.3 covered the gap where SAB removes a job from the queue before adding it to history. **2.6.4** covers a second-stage gap: SAB flips `status` to `Completed` in History a few seconds **before** its post-processing writes the final `storage` field. +### re-identify a track (#889) +filed a track under the wrong release (single vs ep vs album)? there's now a ⇄ button in the library Enhanced view that lets you fix it. search any configured source (tabs, defaults to your active one), see the same song across its single / ep / album with type badges, pick the right one, and soulsync re-files the file you already have under that release — correct year, in-album track number, and art. opt to replace the original entry or keep both. -Pre-fix: `poll_album_download` saw the first `Completed` read with `save_path=None` and bailed. The bundle plugin marked the batch failed, but the UI froze on the last `downloading progress=0.61` emit because the terminal `failed` emit never registered (renderer holds the last-known progress). +built additively over 5 phases (hint store → import seam → multi-source search → modal → button), all riding the existing import pipeline so a no-hint import is byte-identical to before. and it can't lose your file: replace deletes the old entry only *after* the re-import lands, and never if you pick the release it's already in. -- **`poll_album_download`**: separate transient counter for "completed but no save_path." Tolerates up to `transient_miss_threshold` (default 5) consecutive reads in that state — gives SAB ~10s to land the path. When it arrives, return normally. When it doesn't, fail loudly with an explicit error pointing at the missing field. -- **Sticky save_path**: earlier `downloading` reads with a non-empty `save_path` (qBit / Transmission set this from the start) remain cached. So torrent flows aren't affected by the retry path. -- **SAB adapter (`_parse_history_slot`)**: widened the save_path fallback chain — `storage` → `path` → `download_path` → `dirname`. Covers SAB version differences (older builds populated `path`) and forks that expose `download_path` or `dirname`. Whitespace-only values skipped. `incomplete_path` intentionally NOT in the chain — it'd bypass the retry window and point at the in-progress staging dir. -- **Diagnostic**: loud debug log when none of the known fields land, dumping the slot keys so we can grow `_HISTORY_SAVE_PATH_KEYS` if a fork ships a novel field name. +### cleaner libraries & imports +- **#890** — track titles no longer keep the "01 - " prefix from the filename when there's no embedded title tag (which made the real track read as a false "missing"). stripped conservatively so "7 Rings" / "1-800-273-8255" / "1979" are left alone. +- **#891** — a Library Reorganize now sweeps the leftover cover.jpg / .lrc / sidecars from the old folder so it actually empties, plus an opt-in "Remove Residual Files" toggle on the Empty Folder Cleaner for the image-only folders you already have. +- **Sokhi's batch** — same-album songs group under one canonical release id (no more split discographies / mixed cover art); a single can match its parent album; a mid-enrichment crash on an art-less file no longer leaves it untagged; and a sequel digit glued to a CJK title no longer matches the wrong album. -**9 new tests**: -- `test_album_bundle.py` (3): late-save_path arrival recovers; threshold-exhausted fails cleanly; sticky save_path keeps torrent flows working. -- `test_usenet_client_adapters.py` (6): each fallback field tier, whitespace-only skip, all-empty returns None, `incomplete_path` ignored. +### quality & sources +- **#886** — AAC (.m4a) as an opt-in soulseek quality tier, ranked above mp3 / below flac. off by default; existing profiles unchanged until you enable it. +- **#887** — enrichment on Spotify Free now reads "Running (Spotify Free)" instead of wrongly showing "Not Authenticated". +- **#884** — NZBGet imports from the finished location, not the incomplete "….#NZBID" folder. +- **#885** — setting the timezone to Australia/Sydney no longer makes the cache-maintenance job loop every 5 seconds. -132 album-bundle + usenet tests pass. Strictly additive — zero impact on users whose SAB returns `storage` on the first Completed read. +### polish +- the artist-detail header no longer bleeds the blurred artist photo behind it. --- -## Everything else from 2.6.3 (carried forward) +## tests +strictly additive across the board — every new behavior is opt-in or gated so default flows are unchanged. ~100 new tests this cycle (re-identify seam, title-strip danger cases, the shared residual-file classifier, aac tier, tz scheduler, spotify-free status). full imports / matching / reorganize / auto-import suites green, ruff clean. -### Fixes - -**#715 — Soulseek album downloads stuck on "failed" after slskd finished the release.** -`core/soulseek_client._resolve_downloaded_album_file` probed 3 hard-coded candidate paths. On the common slskd config `directories.downloads.username = true`, files land at `//` — none of the 3 candidates carried a username segment, so every file looked locally missing and the bundle poll silently spun for ~30 minutes before marking the batch failed. - -- Lifted the per-track flow's recursive walk-by-basename helper into `core/downloads/file_finder.py` (`find_completed_audio_file`). Bundle resolver now delegates to it. Default-slskd users see zero behavior change (3-candidate fast path preserved). -- Bundle poll detects "slskd reports Completed but local file can't be resolved past a 45s grace window" → exits early with explicit log line pointing at the likely `soulseek.download_path` mismatch. -- Misleading `"(0 tracks, quality=)"` log on the preflight-reuse path fixed. -- **17 new tests** pin every slskd layout (flat, username-prefixed, full-tree-preserved, deep nested, dedup-suffix, quarantine-skip, YouTube/Tidal encoded, transfer-dir fallback, fuzzy variants). - -**Auto-Sync ListenBrainz pipelines stuck on `Refreshing:` for 5+ minutes.** -Refresh path ran `_maybe_discover` inline AND Phase 2 ran the same matching engine via `run_playlist_discovery_worker`. LB tracks discovered twice; refresh-side run blocked with zero progress emission. Also: LB manager only exposed `update_all_playlists` (refreshing one playlist re-pulled all 12+ cached playlists). Also: LB adapter had a silent `except Exception: pass` masking real API failures. - -- Pipeline sets `skip_discovery=True` on refresh config; Phase 2 handles discovery with proper progress emits. -- New `LBManager.refresh_playlist(mbid)` targeted refresh. -- LB adapter logs exceptions with traceback at warning level + returns `None`. -- **12 new tests**. - -**Wishlist: harden Spotify backfill — poisoned `tn=1` can't mask a lean album.** -Spotify-API backfill that hydrates `release_date` / `total_tracks` was coupled to the "track_number missing" branch, so a poisoned default-1 track_number short-circuited it. Lifted to `core/downloads/track_metadata_backfill.py` with split concerns — track-number resolution keeps its precedence chain; album hydration runs whenever `release_date` / `total_tracks` missing, independent of track_number. Single API call still serves both. Also `core/wishlist/routes.py:_build_track_data` no longer defaults `track_number=1` / `disc_number=1` / `total_tracks=1` / `release_date=''`. **24 new tests**. - -**Wishlist: fix three regressions causing all imports to land as track 01.** -Track→dict conversion in payload helpers dropped everything except `album.name`; Deezer-sourced discovery matches saved without `track_number`/`disc_number`; import pipeline only consulted `album_info.track_number` before falling to the filename. Track_number resolution chain lifted into `core/imports/track_number.py:resolve_track_number` with 18 unit tests. - -**Wishlist: only engage album-bundle when several tracks from the same album are missing.** -New `core/wishlist/album_grouping.py`. Bundle path only engages when an album has ≥2 missing tracks; single-track items take the cheaper per-track path. Configurable via `wishlist.album_bundle_min_tracks`. - -**Wishlist: distinguish Queued from Analyzing batches in the UI.** - -**Album-bundle staging: clean Soulseek copies + sweep orphans at startup.** -Cleanup gate extended to include `soulseek` (was torrent/usenet only). New `sweep_orphan_album_bundle_staging` runs once at server boot. **12 new tests**. - -**Usenet album poll: tolerate SAB queue→history handoff (#706).** - -**Discogs: strip artist disambiguation suffixes everywhere (#634).** - -**Library: Enhanced / Standard view toggle persists per browser.** - -**Fix popup: manual matches survive Playlist Pipeline runs.** - -**Fix popup: artist + track fields no longer surface unrelated covers.** - -### UX overhauls - -**Dashboard enrichment panel — equalizer-bar redesign.** 11 speedometer tiles → 11 vertical VU-meter equalizer bars in one symmetric flex row. Brand-logo avatar disc above each bar (Spotify/Apple Music/Deezer/Last.fm/Genius/MusicBrainz/AudioDB/Tidal/Qobuz/Discogs/Amazon with initial-letter CDN-fail fallback); peak-flash on cpm step-up; rolling counter; glass-surface reflection puddle. Last.fm circle-clipped; Tidal/Qobuz/Discogs/Amazon inverted to white silhouettes. - -**Auto-Sync manager — full visual overhaul.** Selector-based override layer (zero JS/HTML changes). Every surface inside the modal restyled to match the dashboard's glassy / accent-radial aesthetic. - -**Auto-Sync — weekly board cards now match the hourly board.** Same Run-now button, unschedule X, next-run countdown, health badge. Weekly cards now draggable between day columns. - -**Auto-Sync sidebar — brand logo on each source-group header.** - -**Sync page tabs — brand-logo chips with active label pill.** 14 tabs collapsed from cramped labeled pills to circular brand-logo chips; active tab swells into a pill with its label inline. `Link` variants (Spotify Link / Deezer Link / iTunes Link) carry a small chain-link badge bottom-right. - -### Architectural lifts - -**Unified Playlist Sources layer.** `PlaylistSource` ABC + registry in `core/playlists/sources/`. Refresh handler dropped from ~190 lines of if/elif to ~80 lines. ListenBrainz / Last.fm / SoulSync Discovery are now Sync-page tabs. - -**Auto-Sync schedule types — weekday + time.** New Weekly Board tab on the Auto-Sync manager. - -**iTunes / Apple Music link import.** New iTunes Link tab on the Sync page. - ---- - -## Test plan - -- [x] 132 album-bundle + usenet tests pass (the new #721 path) -- [x] 488 downloads tests pass (full suite) -- [x] ~90 new unit tests across the cycle, including 9 new for #721 -- [x] Smoke: dashboard equalizer renders w/ brand logos, peak-flash on cpm increase -- [x] Smoke: Auto-Sync manager renders glass overhaul, hourly + weekly cards both have action rows -- [x] Smoke: Sync page tab strip renders as logo chips; active expands; Link variants show chain-link badge -- [ ] Live: @IamGroot60 to re-test Forty Licks usenet bundle on dev (build with the #721 fix applied) -- [ ] Live: Soulseek album download on a username-subdir slskd config completes cleanly (#715, user-validated post-merge) -- [ ] Live: bundle staging dir cleaned on completion (user-validated post-merge) - ---- - -## Post-merge checklist - -- [ ] Tag `v2.6.4` on `main` -- [ ] Trigger `docker-publish.yml` with `version_tag: 2.6.4` to push `boulderbadgedad/soulsync:2.6.4` + `ghcr.io/nezreka/soulsync:2.6.4` (default already updated) -- [ ] Discord release announcement (auto-fired by the workflow) -- [ ] Reply on #721 with the 2.6.4 release link +## post-merge +- [ ] tag `v2.7.4` on `main` +- [ ] docker-publish with `version_tag: 2.7.4` +- [ ] discord announce (auto-fired by the workflow) +- [ ] reply on #889 / #890 / #891 diff --git a/tests/downloads/test_soulseek_aac_quality.py b/tests/downloads/test_soulseek_aac_quality.py new file mode 100644 index 00000000..1dd9fe2f --- /dev/null +++ b/tests/downloads/test_soulseek_aac_quality.py @@ -0,0 +1,106 @@ +"""#886: AAC as an opt-in Soulseek quality tier. + +The whole point is "purely additive": with AAC OFF (the default, and every +profile that predates this), an AAC candidate must behave EXACTLY as before — +it lands in the 'other' bucket, which the waterfall never returns, so it's +dropped. Only a profile that explicitly enables AAC makes it a selectable tier, +ranked above MP3 and below FLAC. + +filter_results_by_quality_preference reads db.get_quality_profile() and walks the +buckets; we stub the db + the quarantine sweep so it runs offline. +""" + +from __future__ import annotations + +import types +from pathlib import Path +from unittest.mock import patch + +import pytest + +from core.soulseek_client import SoulseekClient +from core.download_plugins.types import TrackResult + + +def _client(): + c = SoulseekClient.__new__(SoulseekClient) + c.base_url = 'http://localhost:5030' + c.api_key = 'k' + c.download_path = Path('./test_downloads') + return c + + +def _cand(quality, size_mb, bitrate=None): + return TrackResult( + username='peer', filename=f'A/B/01 - Song.{quality}', + size=int(size_mb * 1024 * 1024), bitrate=bitrate, duration=None, + quality=quality, free_upload_slots=1, upload_speed=1_000_000, + queue_length=0, artist='A', title='Song', album='B', track_number=1) + + +def _q(enabled_flac=True, enabled_mp3=True, aac=None): + qualities = { + 'flac': {'enabled': enabled_flac, 'min_kbps': 500, 'max_kbps': 10000, 'priority': 1, 'bit_depth': 'any'}, + 'mp3_320': {'enabled': enabled_mp3, 'min_kbps': 280, 'max_kbps': 500, 'priority': 2}, + } + if aac is not None: # None => omit the tier entirely (pre-existing profile) + qualities['aac'] = {'enabled': aac, 'min_kbps': 128, 'max_kbps': 400, 'priority': 1.5} + return {'preset': 'custom', 'qualities': qualities, 'fallback_enabled': True} + + +def _filter(candidates, profile): + c = _client() + fake_db = types.SimpleNamespace(get_quality_profile=lambda: profile) + with patch('database.music_database.MusicDatabase', return_value=fake_db), \ + patch.object(SoulseekClient, '_drop_quarantined_sources', lambda self, r: r): + return c.filter_results_by_quality_preference(candidates) + + +# ── additive proof: AAC off == today (dropped) ──────────────────────────────── +def test_aac_dropped_when_tier_absent_pre_existing_profile(): + # A profile saved before this feature has no 'aac' key at all. + out = _filter([_cand('aac', 5)], _q(aac=None)) + assert out == [] # AAC went to 'other' -> never returned, exactly as before + + +def test_aac_dropped_when_tier_present_but_disabled(): + out = _filter([_cand('aac', 5)], _q(aac=False)) + assert out == [] + + +def test_flac_mp3_selection_unchanged_when_aac_absent(): + # The headline no-regression guard: a normal FLAC/MP3 mix is unaffected. + flac, mp3 = _cand('flac', 30), _cand('mp3', 5, bitrate=320) + out = _filter([mp3, flac], _q(aac=None)) + assert out and out[0].quality == 'flac' # FLAC still wins, as before + + +# ── opt-in: AAC on makes it a real tier ─────────────────────────────────────── +def test_aac_selected_when_enabled(): + out = _filter([_cand('aac', 5)], _q(aac=True)) + assert len(out) == 1 and out[0].quality == 'aac' + + +def test_flac_beats_aac_when_both_present(): + flac, aac = _cand('flac', 30), _cand('aac', 5) + out = _filter([aac, flac], _q(aac=True)) + assert out[0].quality == 'flac' # priority 1 < 1.5 + + +def test_aac_beats_mp3_when_both_present(): + mp3, aac = _cand('mp3', 5, bitrate=320), _cand('aac', 5) + out = _filter([mp3, aac], _q(aac=True)) + assert out[0].quality == 'aac' # priority 1.5 < 2 + + +def test_default_and_presets_ship_aac_disabled_above_mp3(): + from database.music_database import MusicDatabase + db = MusicDatabase.__new__(MusicDatabase) # no DB init + profiles = [db._get_default_quality_profile()] + profiles += [db.get_quality_preset(p) for p in ('audiophile', 'balanced', 'space_saver')] + for prof in profiles: + aac = prof['qualities']['aac'] + assert aac['enabled'] is False # opt-in everywhere + # above MP3: lower priority number than the best MP3 tier present + mp3_prios = [v['priority'] for k, v in prof['qualities'].items() if k.startswith('mp3')] + assert aac['priority'] < min(mp3_prios) diff --git a/tests/imports/test_album_grouping.py b/tests/imports/test_album_grouping.py new file mode 100644 index 00000000..01656cc9 --- /dev/null +++ b/tests/imports/test_album_grouping.py @@ -0,0 +1,138 @@ +"""Seam tests for canonical album grouping (Sokhi: split album rows -> mixed +cover art). Drives find_existing_soulsync_album_id against a real in-memory +SQLite albums table — no app singletons, no I/O. +""" + +from __future__ import annotations + +import sqlite3 + +import pytest + +from core.imports.album_grouping import ( + find_existing_soulsync_album_id, + ALLOWED_ALBUM_SOURCE_COLS, +) + + +@pytest.fixture() +def cur(): + conn = sqlite3.connect(":memory:") + conn.execute( + """CREATE TABLE albums ( + id TEXT PRIMARY KEY, + artist_id TEXT, + title TEXT, + server_source TEXT, + spotify_album_id TEXT, + itunes_album_id TEXT, + deezer_id TEXT, + soul_id TEXT, + discogs_id TEXT, + musicbrainz_release_id TEXT + )""" + ) + yield conn.cursor() + conn.close() + + +def _add(cur, *, id, title, artist_id="art1", server_source="soulsync", **source_ids): + cols = ["id", "artist_id", "title", "server_source"] + list(source_ids) + vals = [id, artist_id, title, server_source] + list(source_ids.values()) + cur.execute( + f"INSERT INTO albums ({', '.join(cols)}) VALUES ({', '.join(['?'] * len(cols))})", + vals, + ) + + +def test_empty_db_returns_none(cur): + assert find_existing_soulsync_album_id( + cur, name_key_id="nk", artist_id="art1", album_name="Parachutes", + album_source_col="spotify_album_id", album_source_id="SP1") is None + + +def test_exact_name_hash_id_wins_first(cur): + _add(cur, id="nk", title="Parachutes") + assert find_existing_soulsync_album_id( + cur, name_key_id="nk", artist_id="art1", album_name="Parachutes") == "nk" + + +def test_canonical_source_id_unifies_differently_named_imports(cur): + # Existing row for release SP1 named "Parachutes". A second import of the + # SAME release id but a drifted name must JOIN it, not split. + _add(cur, id="existing", title="Parachutes", spotify_album_id="SP1") + got = find_existing_soulsync_album_id( + cur, name_key_id="different_hash", artist_id="art1", + album_name="Parachutes (Deluxe Edition)", + album_source_col="spotify_album_id", album_source_id="SP1") + assert got == "existing" + + +def test_different_release_id_stays_separate(cur): + # The single-vs-album case: a genuinely different release id must NOT merge + # (documents the known limit — single->album resolution is a separate step). + _add(cur, id="album_row", title="Parachutes", spotify_album_id="SP_ALBUM") + got = find_existing_soulsync_album_id( + cur, name_key_id="single_hash", artist_id="art1", album_name="Yellow", + album_source_col="spotify_album_id", album_source_id="SP_SINGLE") + assert got is None + + +def test_legacy_name_match_still_groups_without_a_source_id(cur): + _add(cur, id="byname", title="Parachutes") + got = find_existing_soulsync_album_id( + cur, name_key_id="other_hash", artist_id="art1", album_name="parachutes", + album_source_col=None, album_source_id=None) + assert got == "byname" # case-insensitive title + artist + + +def test_source_id_match_is_scoped_to_soulsync_rows(cur): + _add(cur, id="plexrow", title="Parachutes", server_source="plex", spotify_album_id="SP1") + got = find_existing_soulsync_album_id( + cur, name_key_id="nk", artist_id="art1", album_name="X", + album_source_col="spotify_album_id", album_source_id="SP1") + assert got is None # the matching row belongs to Plex, not soulsync + + +def test_non_allowlisted_column_is_ignored(cur): + # A column not on the allowlist must never be spliced into SQL. + assert "title" not in ALLOWED_ALBUM_SOURCE_COLS + _add(cur, id="row", title="Parachutes") + got = find_existing_soulsync_album_id( + cur, name_key_id="nk", artist_id="art1", album_name="nope", + album_source_col="title", album_source_id="Parachutes") + assert got is None # 'title' ignored as a source col; name 'nope' doesn't match + + +def test_empty_source_id_skips_canonical_match(cur): + _add(cur, id="row", title="Parachutes", spotify_album_id="") + got = find_existing_soulsync_album_id( + cur, name_key_id="nk", artist_id="art1", album_name="Other", + album_source_col="spotify_album_id", album_source_id="") + assert got is None + + +def test_missing_album_column_falls_through_not_raises(cur): + # Some sources (Deezer) don't have a dedicated album id column on the albums + # table; an allow-listed-but-absent column must NOT raise (it broke the whole + # import once) — it falls through to the name match. + cur.execute("CREATE TABLE albums_min (id TEXT, artist_id TEXT, title TEXT, server_source TEXT)") + cur.execute("INSERT INTO albums_min VALUES ('byname','art1','DZ Album','soulsync')") + # Point the helper at a table missing deezer_id by aliasing via a fresh cursor. + conn2 = sqlite3.connect(":memory:") + conn2.execute("CREATE TABLE albums (id TEXT, artist_id TEXT, title TEXT, server_source TEXT)") + conn2.execute("INSERT INTO albums VALUES ('byname','art1','DZ Album','soulsync')") + c2 = conn2.cursor() + got = find_existing_soulsync_album_id( + c2, name_key_id="nk", artist_id="art1", album_name="DZ Album", + album_source_col="deezer_id", album_source_id="67890") + conn2.close() + assert got == "byname" # deezer_id column absent -> fell through to name match + + +def test_musicbrainz_release_id_grouping(cur): + _add(cur, id="mbrow", title="Album", musicbrainz_release_id="mb-123") + got = find_existing_soulsync_album_id( + cur, name_key_id="nk2", artist_id="art1", album_name="Album (Remaster)", + album_source_col="musicbrainz_release_id", album_source_id="mb-123") + assert got == "mbrow" diff --git a/tests/imports/test_rematch_apply.py b/tests/imports/test_rematch_apply.py new file mode 100644 index 00000000..20c3fd3e --- /dev/null +++ b/tests/imports/test_rematch_apply.py @@ -0,0 +1,71 @@ +"""#889 Phase 4/5: apply a re-identify — stage the file (copy, not move) + build +the hint. Locks down: the original is never touched, the staged name is unique + +keeps the extension, the hint carries the chosen release, and replace_track_id is +set ONLY when 'replace' is ticked. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from core.imports.rematch_apply import ( + build_reidentify_hint, + stage_file_for_reidentify, + staged_destination, +) + +_FIELDS = { + "source": "spotify", "track_id": "trk_1", "album_id": "alb_album1", + "artist_id": "art_1", "track_title": "Song", "album_name": "Album1", + "artist_name": "Artist", "album_type": "album", "track_number": 5, + "disc_number": 1, "isrc": "US1234567890", +} + + +def test_staged_destination_keeps_ext_and_is_traceable(): + dest = staged_destination("/staging", "/lib/EP1/05 - Song.flac", 42) + assert dest.endswith(".flac") + assert "[reid-42]" in dest # traceable to the track + unique per track + assert dest.startswith("/staging/") # loose file in staging root → single candidate + + +def test_stage_copies_not_moves(tmp_path: Path): + src = tmp_path / "lib" / "EP1" / "05 - Song.flac" + src.parent.mkdir(parents=True) + src.write_bytes(b"audio-bytes") + staging = tmp_path / "Staging" + + out = stage_file_for_reidentify(str(src), str(staging), 42, + signature_fn=lambda p: "sig123") + staged = Path(out["staged_path"]) + assert staged.is_file() and staged.read_bytes() == b"audio-bytes" + assert src.is_file() # ORIGINAL untouched (copy, never move) + assert out["content_hash"] == "sig123" + + +def test_stage_missing_source_raises(tmp_path: Path): + with pytest.raises(FileNotFoundError): + stage_file_for_reidentify(str(tmp_path / "gone.flac"), str(tmp_path / "S"), 1) + + +def test_build_hint_sets_replace_when_ticked(): + h = build_reidentify_hint(42, _FIELDS, "/staging/x.flac", "sig", replace=True) + assert h.replace_track_id == 42 + assert h.album_id == "alb_album1" and h.source == "spotify" + assert h.track_number == 5 and h.isrc == "US1234567890" + assert h.exempt_dedup is True + assert h.staged_path == "/staging/x.flac" and h.content_hash == "sig" + + +def test_build_hint_no_replace_when_unticked(): + h = build_reidentify_hint(42, _FIELDS, "/staging/x.flac", "sig", replace=False) + assert h.replace_track_id is None # keep original → no deletion + assert h.exempt_dedup is True # still bypasses dedup-skip (explicit action) + + +def test_build_hint_handles_non_numeric_track_id(): + # Jellyfin-style GUID track ids must still round-trip as replace target. + h = build_reidentify_hint("abc-guid", _FIELDS, "/s/x.flac", None, replace=True) + assert h.replace_track_id == "abc-guid" diff --git a/tests/imports/test_rematch_hints.py b/tests/imports/test_rematch_hints.py new file mode 100644 index 00000000..6e1d08de --- /dev/null +++ b/tests/imports/test_rematch_hints.py @@ -0,0 +1,155 @@ +"""#889 Phase 1: the re-identify hint store — create / find / consume. + +The hint is the single-use, user-designated "which release" answer the import +flow reads at the top of matching. These lock down: a hint round-trips, it's +found by staged path, found by content_hash when the path missed (rename-proof), +found by basename when the dir changed, consumed exactly once, and that a +consumed hint is never handed back. +""" + +from __future__ import annotations + +import sqlite3 + +import pytest + +from core.imports.rematch_hints import ( + RematchHint, + consume_hint, + create_hint, + find_hint_for_file, + list_pending_hints, + quick_file_signature, +) + +# The slice of the real schema this module touches (kept in sync with +# database/music_database.py's rematch_hints CREATE). +_SCHEMA = """ +CREATE TABLE rematch_hints ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + staged_path TEXT NOT NULL, + content_hash TEXT, + source TEXT NOT NULL, + isrc TEXT, + track_id TEXT, + album_id TEXT, + artist_id TEXT, + track_title TEXT, + album_name TEXT, + artist_name TEXT, + album_type TEXT, + track_number INTEGER, + disc_number INTEGER, + replace_track_id INTEGER, + exempt_dedup INTEGER NOT NULL DEFAULT 1, + status TEXT NOT NULL DEFAULT 'pending', + created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, + consumed_at TIMESTAMP +) +""" + + +@pytest.fixture +def cur(): + conn = sqlite3.connect(":memory:") + conn.row_factory = sqlite3.Row + conn.executescript(_SCHEMA) + yield conn.cursor() + conn.close() + + +def _hint(**kw): + base = dict( + staged_path="/staging/Song.flac", + source="spotify", + isrc="USABC1234567", + track_id="trk_1", + album_id="alb_album1", + artist_id="art_1", + track_title="Song", + album_name="Album1", + artist_name="Artist", + album_type="album", + track_number=5, + disc_number=1, + replace_track_id=42, + ) + base.update(kw) + return RematchHint(**base) + + +def test_create_and_find_by_path_roundtrips(cur): + new_id = create_hint(cur, _hint()) + assert new_id > 0 + got = find_hint_for_file(cur, "/staging/Song.flac") + assert got is not None + assert got.id == new_id + assert got.album_id == "alb_album1" and got.album_type == "album" + assert got.isrc == "USABC1234567" + assert got.track_number == 5 and got.disc_number == 1 + assert got.replace_track_id == 42 + assert got.exempt_dedup is True # always set for a user-designated re-identify + assert got.status == "pending" + + +def test_find_by_content_hash_when_path_missed(cur): + create_hint(cur, _hint(content_hash="deadbeef")) + # Watcher renamed/moved the file → path lookup misses, hash rescues it. + assert find_hint_for_file(cur, "/totally/different.flac") is None + got = find_hint_for_file(cur, "/totally/different.flac", content_hash="deadbeef") + assert got is not None and got.album_name == "Album1" + + +def test_find_by_basename_when_dir_changed(cur): + create_hint(cur, _hint(staged_path="/staging/in/Song.flac")) + # Same filename, different directory (watcher moved it deeper). + got = find_hint_for_file(cur, "/staging/processing/Song.flac") + assert got is not None and got.track_id == "trk_1" + + +def test_consume_is_single_use(cur): + new_id = create_hint(cur, _hint()) + assert find_hint_for_file(cur, "/staging/Song.flac") is not None + consume_hint(cur, new_id) + # Consumed → never handed back, by path or by hash. + assert find_hint_for_file(cur, "/staging/Song.flac") is None + assert find_hint_for_file(cur, "/x", content_hash=None) is None + + +def test_list_pending_excludes_consumed(cur): + a = create_hint(cur, _hint(staged_path="/staging/A.flac")) + create_hint(cur, _hint(staged_path="/staging/B.flac")) + assert len(list_pending_hints(cur)) == 2 + consume_hint(cur, a) + pend = list_pending_hints(cur) + assert len(pend) == 1 and pend[0].staged_path == "/staging/B.flac" + + +def test_newest_pending_wins_on_duplicate_path(cur): + create_hint(cur, _hint(album_id="alb_old")) + create_hint(cur, _hint(album_id="alb_new")) # user re-picked for the same file + got = find_hint_for_file(cur, "/staging/Song.flac") + assert got.album_id == "alb_new" + + +def test_exempt_dedup_false_roundtrips(cur): + create_hint(cur, _hint(staged_path="/staging/Keep.flac", exempt_dedup=False)) + got = find_hint_for_file(cur, "/staging/Keep.flac") + assert got.exempt_dedup is False + + +# ── content fingerprint ─────────────────────────────────────────────────────── +def test_quick_file_signature_stable_and_distinct(tmp_path): + a = tmp_path / "a.bin" + b = tmp_path / "b.bin" + a.write_bytes(b"hello world" * 1000) + b.write_bytes(b"goodbye moon" * 1000) + sig_a1 = quick_file_signature(str(a)) + sig_a2 = quick_file_signature(str(a)) + sig_b = quick_file_signature(str(b)) + assert sig_a1 and sig_a1 == sig_a2 # stable + assert sig_a1 != sig_b # distinct content → distinct sig + + +def test_quick_file_signature_missing_file_is_none(): + assert quick_file_signature("/no/such/file.flac") is None diff --git a/tests/imports/test_rematch_hints_seam.py b/tests/imports/test_rematch_hints_seam.py new file mode 100644 index 00000000..5917cbdc --- /dev/null +++ b/tests/imports/test_rematch_hints_seam.py @@ -0,0 +1,217 @@ +"""#889 Phase 2: the import seam — a hint short-circuits identification, and the +old library row is replaced only after the re-import succeeds. + +Two layers: + * pure helpers (build_identification_from_hint, delete_replaced_track) — exact + mapping + safe replacement against an in-memory DB, injectable unlink. + * the worker seam (_resolve_rematch_hint / _finalize_rematch_hint) — proves the + NO-HINT path is untouched, the hint path returns a ready identification, the + lookup is fail-safe, and finalize consumes + replaces. +""" + +from __future__ import annotations + +import sqlite3 +import types + +import pytest + +from core.auto_import_worker import AutoImportWorker, FolderCandidate +from core.imports.rematch_hints import ( + RematchHint, + build_identification_from_hint, + consume_hint, + create_hint, + delete_replaced_track, + find_hint_for_file, +) + +_SCHEMA = """ +CREATE TABLE rematch_hints ( + id INTEGER PRIMARY KEY AUTOINCREMENT, staged_path TEXT NOT NULL, content_hash TEXT, + source TEXT NOT NULL, isrc TEXT, track_id TEXT, album_id TEXT, artist_id TEXT, + track_title TEXT, album_name TEXT, artist_name TEXT, album_type TEXT, + track_number INTEGER, disc_number INTEGER, replace_track_id INTEGER, + exempt_dedup INTEGER NOT NULL DEFAULT 1, status TEXT NOT NULL DEFAULT 'pending', + created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, consumed_at TIMESTAMP +); +CREATE TABLE tracks ( + id INTEGER PRIMARY KEY, album_id INTEGER, artist_id INTEGER, title TEXT, + track_number INTEGER, file_path TEXT +); +""" + + +@pytest.fixture +def conn(): + c = sqlite3.connect(":memory:") + c.row_factory = sqlite3.Row + c.executescript(_SCHEMA) + yield c + c.close() + + +def _hint(**kw): + base = dict(staged_path="/staging/Song.flac", source="spotify", album_id="alb_album1", + artist_id="art_1", track_id="trk_1", track_title="Song", album_name="Album1", + artist_name="Artist", album_type="album", track_number=5, disc_number=1) + base.update(kw) + return RematchHint(**base) + + +# ── pure: identification mapping ────────────────────────────────────────────── +def test_build_identification_maps_hint_fields(): + ident = build_identification_from_hint(_hint()) + assert ident["album_id"] == "alb_album1" + assert ident["source"] == "spotify" + assert ident["album_name"] == "Album1" + assert ident["artist_id"] == "art_1" + assert ident["track_number"] == 5 + assert ident["method"] == "rematch_hint" + assert ident["identification_confidence"] == 1.0 + # album_type 'album' → not a single, and force_album_match makes the matcher + # fetch the real album (year/track#/art) instead of the singles stub. + assert ident["is_single"] is False + assert ident["force_album_match"] is True + + +def test_build_identification_single_release_still_forces_album_fetch(): + # Even a chosen SINGLE release is fetched (it has a year too); is_single flags + # the type, force_album_match drives the album path regardless. + ident = build_identification_from_hint(_hint(album_type="single")) + assert ident["is_single"] is True + assert ident["force_album_match"] is True + + +# ── pure: safe replacement ──────────────────────────────────────────────────── +def test_delete_replaced_track_removes_row_and_file(conn): + cur = conn.cursor() + cur.execute("INSERT INTO tracks (id, file_path) VALUES (7, '/lib/EP1/05 - Song.flac')") + removed = [] + out = delete_replaced_track(cur, 7, unlink=lambda p: removed.append(p)) + assert out == "/lib/EP1/05 - Song.flac" + assert removed == ["/lib/EP1/05 - Song.flac"] # file removed (we faked existence below) + cur.execute("SELECT 1 FROM tracks WHERE id = 7") + assert cur.fetchone() is None # row gone + + +def test_delete_replaced_track_keeps_file_if_another_row_points_at_it(conn): + cur = conn.cursor() + cur.execute("INSERT INTO tracks (id, file_path) VALUES (7, '/lib/shared.flac')") + cur.execute("INSERT INTO tracks (id, file_path) VALUES (8, '/lib/shared.flac')") + removed = [] + out = delete_replaced_track(cur, 7, unlink=lambda p: removed.append(p)) + assert out is None and removed == [] # row 8 still references it → no unlink + cur.execute("SELECT 1 FROM tracks WHERE id = 7") + assert cur.fetchone() is None # but row 7 still deleted + + +def test_delete_replaced_track_noops_on_missing_id(conn): + cur = conn.cursor() + assert delete_replaced_track(cur, None) is None + assert delete_replaced_track(cur, 999) is None # no such row + + +def test_delete_replaced_track_same_home_is_noop(conn): + # THE data-loss bug: re-identify to the release it's already in → the import + # reuses the same file/row, so deleting it would orphan the file. Guard: no-op. + cur = conn.cursor() + cur.execute("INSERT INTO tracks (id, file_path) VALUES (7, '/lib/Album1/05 - Song.flac')") + removed = [] + out = delete_replaced_track(cur, 7, unlink=lambda p: removed.append(p), + new_paths=['/lib/Album1/05 - Song.flac']) + assert out is None and removed == [] # NOTHING unlinked + cur.execute("SELECT 1 FROM tracks WHERE id = 7") + assert cur.fetchone() is not None # row PRESERVED (it's the re-imported track) + + +def test_delete_replaced_track_different_home_still_deletes(conn): + # Genuinely re-homed (new path differs) → old row + file removed as intended. + cur = conn.cursor() + cur.execute("INSERT INTO tracks (id, file_path) VALUES (7, '/lib/EP1/05 - Song.flac')") + removed = [] + out = delete_replaced_track(cur, 7, unlink=lambda p: removed.append(p), + new_paths=['/lib/Album1/05 - Song.flac']) + assert out == '/lib/EP1/05 - Song.flac' + assert removed == ['/lib/EP1/05 - Song.flac'] + cur.execute("SELECT 1 FROM tracks WHERE id = 7") + assert cur.fetchone() is None + + +def test_delete_replaced_track_resolves_path_before_unlink(conn): + # The stored path is a server/Docker view this process can't read literally; + # resolve_fn maps it to the real file so we unlink the RIGHT path (not orphan it). + cur = conn.cursor() + cur.execute("INSERT INTO tracks (id, file_path) VALUES (7, '/mnt/serverview/Song.flac')") + removed = [] + out = delete_replaced_track(cur, 7, unlink=lambda p: removed.append(p), + resolve_fn=lambda stored: '/real/local/Song.flac') + assert out == '/real/local/Song.flac' + assert removed == ['/real/local/Song.flac'] # unlinked the RESOLVED path + + +# patch os.path.exists so the unlink branch is reachable without real files +@pytest.fixture(autouse=True) +def _exists(monkeypatch): + monkeypatch.setattr("core.imports.rematch_hints.os.path.exists", lambda p: True) + + +# ── worker seam ─────────────────────────────────────────────────────────────── +def _worker(conn): + # Production hands out a FRESH connection per call (the worker closes it); + # here we share one in-memory DB, so proxy close() to a no-op. + w = AutoImportWorker.__new__(AutoImportWorker) + proxy = types.SimpleNamespace(cursor=conn.cursor, commit=conn.commit, close=lambda: None) + w.database = types.SimpleNamespace(_get_connection=lambda: proxy) + return w + + +def test_resolve_returns_none_when_no_hint(conn): + w = _worker(conn) + cand = FolderCandidate(path="/staging", name="Song", audio_files=["/staging/Song.flac"]) + assert w._resolve_rematch_hint(cand) == (None, None) # untouched → normal identify + + +def test_resolve_returns_identification_when_hinted(conn, monkeypatch): + # don't hash a real file + monkeypatch.setattr("core.imports.rematch_hints.quick_file_signature", lambda p: None) + create_hint(conn.cursor(), _hint(staged_path="/staging/Song.flac")) + conn.commit() + w = _worker(conn) + cand = FolderCandidate(path="/staging", name="Song", audio_files=["/staging/Song.flac"]) + hint, ident = w._resolve_rematch_hint(cand) + assert hint is not None and hint.album_id == "alb_album1" + assert ident["album_id"] == "alb_album1" and ident["method"] == "rematch_hint" + + +def test_resolve_ignores_multi_file_candidates(conn): + create_hint(conn.cursor(), _hint(staged_path="/staging/Song.flac")) + conn.commit() + w = _worker(conn) + cand = FolderCandidate(path="/staging", name="Album", + audio_files=["/staging/Song.flac", "/staging/Other.flac"]) + assert w._resolve_rematch_hint(cand) == (None, None) # re-identify is single-track only + + +def test_resolve_is_failsafe_on_db_error(): + w = AutoImportWorker.__new__(AutoImportWorker) + def _boom(): + raise RuntimeError("db down") + w.database = types.SimpleNamespace(_get_connection=_boom) + cand = FolderCandidate(path="/staging", name="Song", audio_files=["/staging/Song.flac"]) + assert w._resolve_rematch_hint(cand) == (None, None) # error never breaks auto-import + + +def test_finalize_consumes_and_replaces(conn, monkeypatch): + monkeypatch.setattr("core.imports.rematch_hints.quick_file_signature", lambda p: None) + cur = conn.cursor() + cur.execute("INSERT INTO tracks (id, file_path) VALUES (42, '/lib/EP1/05 - Song.flac')") + hid = create_hint(cur, _hint(replace_track_id=42)) + conn.commit() + w = _worker(conn) + hint = find_hint_for_file(conn.cursor(), "/staging/Song.flac") + w._finalize_rematch_hint(hint) + # old row deleted, hint consumed + cur.execute("SELECT 1 FROM tracks WHERE id = 42") + assert cur.fetchone() is None + assert find_hint_for_file(conn.cursor(), "/staging/Song.flac") is None # consumed diff --git a/tests/imports/test_rematch_search.py b/tests/imports/test_rematch_search.py new file mode 100644 index 00000000..dd962bbe --- /dev/null +++ b/tests/imports/test_rematch_search.py @@ -0,0 +1,121 @@ +"""#889 Phase 3: re-identify search — normalize results across sources, infer the +release-type badge, and resolve the picked row's album_id. + +Locks down: same song surfaces as multiple rows (single/EP/album), the EP +inference from a multi-track 'single', graceful empty on a dead source, and that +resolve_hint_fields pulls album_id (and refuses a result without one). +""" + +from __future__ import annotations + +import types + +from core.imports.rematch_search import ( + available_sources, + infer_release_type, + normalize_search_result, + resolve_hint_fields, + search_release_candidates, +) + + +# typed-Track-ish object (mirrors core.metadata.types.Track attrs the modal reads) +def _track(tid, title, album, album_type, total, isrc=None, year="2020"): + return types.SimpleNamespace( + id=tid, name=title, artists=["Artist"], album=album, + album_type=album_type, total_tracks=total, release_date=year + "-01-01", + image_url="http://img/" + tid, isrc=isrc, external_ids={}, + ) + + +# ── release-type inference ──────────────────────────────────────────────────── +def test_infer_album_stays_album(): + assert infer_release_type("album", 12) == "album" + + +def test_infer_single_one_track_is_single(): + assert infer_release_type("single", 1) == "single" + + +def test_infer_multitrack_single_promoted_to_ep(): + # Spotify labels EPs as album_type='single' — promote on track count. + assert infer_release_type("single", 5) == "ep" + + +def test_infer_compilation(): + assert infer_release_type("compilation", 40) == "compilation" + + +def test_infer_unknown_falls_back_to_count(): + assert infer_release_type(None, 10) == "album" + assert infer_release_type("", 4) == "ep" + assert infer_release_type(None, 1) == "single" + + +# ── normalization ───────────────────────────────────────────────────────────── +def test_normalize_builds_display_row(): + row = normalize_search_result(_track("t1", "Song", "Album1", "album", 12, isrc="US1234567890"), "spotify") + assert row["track_id"] == "t1" + assert row["album_name"] == "Album1" and row["album_type"] == "album" + assert row["artist_name"] == "Artist" + assert row["year"] == "2020" and row["isrc"] == "US1234567890" + + +def test_normalize_skips_result_without_id_or_title(): + assert normalize_search_result(types.SimpleNamespace(id="", name="X"), "spotify") is None + assert normalize_search_result(types.SimpleNamespace(id="t", name=""), "spotify") is None + + +def test_same_song_multiple_collections(): + """The headline case: one song, three releases, three distinct rows + badges.""" + results = [ + _track("t_alb", "Song", "Album1", "album", 12), + _track("t_ep", "Song", "EP1", "single", 5), # multi-track single → EP + _track("t_sgl", "Song", "Song (Single)", "single", 1), + ] + client = types.SimpleNamespace(search_tracks=lambda q, limit=25: results) + rows = search_release_candidates("spotify", "Song", client_factory=lambda s: client) + badges = {r["album_name"]: r["album_type"] for r in rows} + assert badges == {"Album1": "album", "EP1": "ep", "Song (Single)": "single"} + + +def test_search_empty_on_missing_client(): + assert search_release_candidates("spotify", "x", client_factory=lambda s: None) == [] + + +def test_search_empty_on_blank_query(): + called = [] + search_release_candidates("spotify", " ", client_factory=lambda s: called.append(1)) + assert called == [] # never even fetches a client for an empty query + + +def test_search_swallows_client_error(): + def boom(q, limit=25): + raise RuntimeError("rate limited") + client = types.SimpleNamespace(search_tracks=boom) + assert search_release_candidates("spotify", "x", client_factory=lambda s: client) == [] + + +# ── resolve on select ───────────────────────────────────────────────────────── +def test_resolve_pulls_album_id_and_fields(): + details = { + "name": "Song", "track_number": 5, "disc_number": 1, "isrc": "US1234567890", + "album": {"id": "alb_album1", "name": "Album1", "album_type": "album", "total_tracks": 12}, + "artists": [{"id": "art_1", "name": "Artist"}], + } + client = types.SimpleNamespace(get_track_details=lambda tid: details) + out = resolve_hint_fields("spotify", "t_alb", client_factory=lambda s: client) + assert out["album_id"] == "alb_album1" + assert out["artist_id"] == "art_1" + assert out["track_number"] == 5 and out["disc_number"] == 1 + assert out["album_type"] == "album" and out["isrc"] == "US1234567890" + + +def test_resolve_refuses_result_without_album_id(): + details = {"name": "Song", "album": {"name": "NoId Album"}} # no album id + client = types.SimpleNamespace(get_track_details=lambda tid: details) + assert resolve_hint_fields("spotify", "t", client_factory=lambda s: client) is None + + +def test_resolve_none_on_missing_client(): + assert resolve_hint_fields("spotify", "t", client_factory=lambda s: None) is None diff --git a/tests/imports/test_single_to_album.py b/tests/imports/test_single_to_album.py new file mode 100644 index 00000000..ad41f26a --- /dev/null +++ b/tests/imports/test_single_to_album.py @@ -0,0 +1,206 @@ +"""Seam tests for single -> parent-album resolution (Sokhi: single-matched track +splits from its album -> mixed cover art). The selector is pure; the resolver +takes injected fetchers, so neither needs a live metadata client. +""" + +from __future__ import annotations + +import pytest + +from core.imports.single_to_album import ( + select_parent_album, + resolve_single_to_album, +) +from core.imports.context import detect_album_info_web + + +# ── pure selector ───────────────────────────────────────────────────────────── +def _alb(name, tracks, album_type="album", **extra): + return {"name": name, "album_type": album_type, "tracks": tracks, **extra} + + +def test_picks_album_that_contains_the_track(): + got = select_parent_album("Yellow", [ + _alb("Parachutes", ["Don't Panic", "Shiver", "Yellow", "Trouble"]), + ]) + assert got and got["name"] == "Parachutes" + + +def test_returns_none_when_no_album_contains_the_track(): + assert select_parent_album("Yellow", [ + _alb("Some Other Album", ["Track A", "Track B"]), + ]) is None + + +def test_never_promotes_onto_a_single_release(): + # The single's own release (album_type 'single', name == track) must be ignored. + assert select_parent_album("Yellow", [ + _alb("Yellow", ["Yellow"], album_type="single"), + ]) is None + + +def test_ignores_ep_and_compilation_types(): + assert select_parent_album("Yellow", [ + _alb("Yellow EP", ["Yellow", "Yellow (Live)"], album_type="ep"), + _alb("Greatest Hits", ["Yellow", "Clocks"], album_type="compilation"), + ]) is None + + +def test_skips_album_named_exactly_like_the_track(): + # An 'album' whose name IS the track title is the single dressed as an album; + # don't treat it as the parent. + assert select_parent_album("Yellow", [ + _alb("Yellow", ["Yellow"]), + ]) is None + + +def test_matches_through_album_version_qualifier(): + got = select_parent_album("Yellow", [ + _alb("Parachutes", ["Shiver", "Yellow (Album Version)"]), + ]) + assert got and got["name"] == "Parachutes" + + +def test_first_qualifying_candidate_wins_deterministically(): + got = select_parent_album("Yellow", [ + _alb("Parachutes", ["Yellow"]), + _alb("Parachutes (Deluxe)", ["Yellow", "Bonus"]), + ]) + assert got["name"] == "Parachutes" # input order = priority + + +def test_empty_title_returns_none(): + assert select_parent_album("", [_alb("Parachutes", ["Yellow"])]) is None + + +# ── injected-I/O resolver ───────────────────────────────────────────────────── +def test_resolver_finds_parent_album_lazily(): + calls = {"tracks": 0} + albums = [ + {"name": "Single Yellow", "album_type": "single", "id": "s1"}, # skipped (not album) + {"name": "Wrong Album", "album_type": "album", "id": "a1"}, + {"name": "Parachutes", "album_type": "album", "id": "a2"}, + ] + + def fetch_tracks(alb): + calls["tracks"] += 1 + return {"a1": ["Other"], "a2": ["Yellow", "Shiver"]}.get(alb["id"], []) + + got = resolve_single_to_album( + "Yellow", + fetch_album_candidates=lambda: albums, + fetch_album_tracks=fetch_tracks, + ) + assert got and got["name"] == "Parachutes" and got["album_id"] == "a2" + assert calls["tracks"] == 2 # probed a1 then a2, stopped; never probed the single + + +def test_resolver_returns_none_when_nothing_contains_track(): + got = resolve_single_to_album( + "Yellow", + fetch_album_candidates=lambda: [{"name": "X", "album_type": "album", "id": "a1"}], + fetch_album_tracks=lambda alb: ["Nope"], + ) + assert got is None + + +def test_resolver_is_failsafe_on_candidate_fetch_error(): + def boom(): + raise RuntimeError("api down") + assert resolve_single_to_album( + "Yellow", fetch_album_candidates=boom, fetch_album_tracks=lambda a: []) is None + + +def test_resolver_is_failsafe_on_track_fetch_error(): + def boom(alb): + raise RuntimeError("api down") + got = resolve_single_to_album( + "Yellow", + fetch_album_candidates=lambda: [{"name": "Parachutes", "album_type": "album", "id": "a1"}], + fetch_album_tracks=boom) + assert got is None + + +def test_resolver_caps_albums_probed(): + albums = [{"name": f"A{i}", "album_type": "album", "id": str(i)} for i in range(20)] + probed = {"n": 0} + + def fetch_tracks(alb): + probed["n"] += 1 + return ["nope"] + + resolve_single_to_album( + "Yellow", + fetch_album_candidates=lambda: albums, + fetch_album_tracks=fetch_tracks, + max_albums=5) + assert probed["n"] == 5 # never probes more than the cap + + +# ── gated wiring through detect_album_info_web (config gate + client shapes) ─── +class _Cfg: + def __init__(self, on): + self._on = on + + def get(self, key, default=None): + if key == "metadata_enhancement.single_to_album": + return self._on + return default + + +_SINGLE_CTX = { + "source": "spotify", + "artist": {"id": "art1", "name": "Coldplay"}, + # album_type unset + name == track + total_tracks 1 -> is_album False, and the + # existing best-effort skips (album name == track), so the glue is reached. + "album": {"id": "s1", "name": "Yellow", "total_tracks": 1}, + "track_info": {"id": "t1", "name": "Yellow", "track_number": 7}, + "original_search_result": {"title": "Yellow", "album": "Yellow"}, +} + + +def _patch_clients(monkeypatch, albums, tracks_by_id): + monkeypatch.setattr("core.metadata.album_tracks.get_artist_albums_for_source", + lambda *a, **k: albums) + monkeypatch.setattr("core.metadata.album_tracks.get_artist_album_tracks", + lambda album_id, **k: {"tracks": tracks_by_id.get(album_id, [])}) + + +def test_glue_promotes_single_to_parent_album_when_enabled(monkeypatch): + monkeypatch.setattr("core.metadata.common.get_config_manager", lambda: _Cfg(True)) + _patch_clients( + monkeypatch, + albums=[{"name": "Yellow", "album_type": "single", "id": "s1"}, + {"name": "Parachutes", "album_type": "album", "id": "a2"}], + tracks_by_id={"a2": [{"title": "Shiver"}, {"title": "Yellow"}]}, + ) + out = detect_album_info_web(dict(_SINGLE_CTX), {"id": "art1", "name": "Coldplay"}) + assert out and out["is_album"] is True + assert out["album_name"] == "Parachutes" + assert out["track_number"] == 7 # preserved + + +def test_glue_disabled_by_default_returns_none(monkeypatch): + monkeypatch.setattr("core.metadata.common.get_config_manager", lambda: _Cfg(False)) + # Even with clients that WOULD match, the flag off => no promotion. + _patch_clients(monkeypatch, + albums=[{"name": "Parachutes", "album_type": "album", "id": "a2"}], + tracks_by_id={"a2": [{"title": "Yellow"}]}) + assert detect_album_info_web(dict(_SINGLE_CTX), {"id": "art1", "name": "Coldplay"}) is None + + +def test_glue_no_match_returns_none(monkeypatch): + monkeypatch.setattr("core.metadata.common.get_config_manager", lambda: _Cfg(True)) + _patch_clients(monkeypatch, + albums=[{"name": "Other Album", "album_type": "album", "id": "a9"}], + tracks_by_id={"a9": [{"title": "Different Song"}]}) + assert detect_album_info_web(dict(_SINGLE_CTX), {"id": "art1", "name": "Coldplay"}) is None + + +def test_glue_failsafe_when_client_raises(monkeypatch): + monkeypatch.setattr("core.metadata.common.get_config_manager", lambda: _Cfg(True)) + + def boom(*a, **k): + raise RuntimeError("spotify down") + monkeypatch.setattr("core.metadata.album_tracks.get_artist_albums_for_source", boom) + assert detect_album_info_web(dict(_SINGLE_CTX), {"id": "art1", "name": "Coldplay"}) is None diff --git a/tests/imports/test_track_number_strip.py b/tests/imports/test_track_number_strip.py new file mode 100644 index 00000000..ece899f5 --- /dev/null +++ b/tests/imports/test_track_number_strip.py @@ -0,0 +1,77 @@ +"""#890: a leading track number leaking from a filename stem into the title +("01 - Sun It Rises") makes the track never match the canonical "Sun It Rises", +so it reads as a false "missing". strip_leading_track_number removes the prefix — +conservatively, so titles that merely START with a number are left alone. +""" + +from __future__ import annotations + +import pytest + +from core.imports.context import get_import_clean_title +from core.imports.paths import strip_leading_track_number + + +# ── the bug: track-number prefixes get stripped ─────────────────────────────── +@pytest.mark.parametrize("dirty,clean", [ + ("01 - Sun It Rises", "Sun It Rises"), # the screenshot + ("04 - Tiger Mountain Peasant Song", "Tiger Mountain Peasant Song"), + ("05 - Quiet Houses", "Quiet Houses"), + ("07 - Heard Them Stirring", "Heard Them Stirring"), + ("01 Sun It Rises", "Sun It Rises"), # zero-padded, no separator + ("3 - Title", "Title"), # plain number + separator + space + ("12. Some Song", "Some Song"), # dot separator + ("10 - Track Ten", "Track Ten"), + ("09) Closing Time", "Closing Time"), # paren separator + (" 02 - Spaced Out ", "Spaced Out"), # messy whitespace +]) +def test_strips_track_number_prefix(dirty, clean): + assert strip_leading_track_number(dirty) == clean + + +# ── the guard: titles that legitimately start with a number are UNTOUCHED ────── +@pytest.mark.parametrize("title", [ + "7 Rings", + "99 Luftballons", + "50 Ways to Leave Your Lover", + "1-800-273-8255", # number-with-dashes is part of the title + "1979", + "9 to 5", + "4 Minutes", + "8 Mile", + "21 Guns", + "24 Hour Party People", # no separator → not a track number + "0 to 100", + "Sun It Rises", # no leading number at all +]) +def test_preserves_real_titles(title): + assert strip_leading_track_number(title) == title + + +# ── degenerate inputs ───────────────────────────────────────────────────────── +def test_never_reduces_to_empty_or_bare_number(): + assert strip_leading_track_number("01") == "01" # bare number → keep + assert strip_leading_track_number("01 - ") == "01 -" # nothing left → keep original (trimmed) + assert strip_leading_track_number("") == "" + assert strip_leading_track_number(None) == "" + + +def test_only_strips_one_prefix(): + # A title that legitimately follows the number keeps its own leading number. + assert strip_leading_track_number("01 - 24 Hour Party People") == "24 Hour Party People" + + +# ── the chokepoint: every import path resolves its title through here ────────── +def test_get_import_clean_title_strips_filename_leak(): + # original_search['title'] came from the filename stem (no embedded tag). + ctx = {"original_search_result": {"title": "01 - Sun It Rises"}} + assert get_import_clean_title(ctx) == "Sun It Rises" + + +def test_get_import_clean_title_leaves_clean_source_title(): + ctx = {"original_search_result": {"title": "7 Rings"}} + assert get_import_clean_title(ctx) == "7 Rings" + + +def test_get_import_clean_title_default_untouched(): + assert get_import_clean_title({}, default="Unknown Track") == "Unknown Track" diff --git a/tests/library/test_residual_files.py b/tests/library/test_residual_files.py new file mode 100644 index 00000000..3c030d4e --- /dev/null +++ b/tests/library/test_residual_files.py @@ -0,0 +1,51 @@ +"""#891: the shared 'residual file' classifier — junk + cover/scan images + +lyric/metadata sidecars — used by both the Reorganize cleanup and the Empty +Folder Cleaner, plus the reorganize sweep that uses it. +""" + +from __future__ import annotations + +from pathlib import Path + +from core.library.residual_files import ( + is_disposable, + is_image, + is_junk, + is_sidecar, +) + + +def test_images_classified(): + for n in ('cover.jpg', 'Cover.JPEG', 'folder.png', 'back.webp', 'scan.tiff', 'art.gif'): + assert is_image(n) and is_disposable(n) + + +def test_sidecars_classified(): + for n in ('lyrics.lrc', 'album.nfo', 'disc.cue', 'playlist.m3u', 'x.m3u8'): + assert is_sidecar(n) and is_disposable(n) + + +def test_junk_classified(): + assert is_junk('.DS_Store') and is_disposable('Thumbs.db') + + +def test_real_content_not_disposable(): + # Audio + anything unrecognized (booklet, video, a note) is real content. + for n in ('song.flac', 'track.mp3', 'booklet.pdf', 'movie.mkv', 'readme.txt', 'data.json'): + assert not is_disposable(n), n + + +# ── the reorganize sweep that uses the predicate ────────────────────────────── +def test_delete_album_sidecars_sweeps_all_residual_keeps_real(tmp_path: Path): + from core.library_reorganize import _delete_album_sidecars + + d = tmp_path / 'Old Album' + d.mkdir() + for n in ('cover.jpg', 'back.jpg', 'disc.png', 'lyrics.lrc', 'album.nfo', '.DS_Store'): + (d / n).write_text('x') + (d / 'booklet.pdf').write_text('keep') # unrecognized → must survive + + _delete_album_sidecars(str(d)) + + survivors = {p.name for p in d.iterdir()} + assert survivors == {'booklet.pdf'} # every residual swept, booklet kept diff --git a/tests/matching/test_numeric_release_guard.py b/tests/matching/test_numeric_release_guard.py index b2ba79c8..23639b29 100644 --- a/tests/matching/test_numeric_release_guard.py +++ b/tests/matching/test_numeric_release_guard.py @@ -19,6 +19,12 @@ from core.musicbrainz_service import MusicBrainzService VOL4 = "B小町 - TVアニメ「【推しの子】」キャラクターソングCD Vol.4" VOL45 = "B小町 - TVアニメ「【推しの子】」キャラクターソングCD Vol.4.5" +# Sokhi #2: the sequel number is glued straight onto a CJK word ('…トラック2'), +# with the SAME digit already present elsewhere ('第2期' = season 2). Stripping +# to [a-z0-9] collapsed both titles to {'2'} and the wrong (cour-2) cover won. +OST = "『無職転生 〜異世界行ったら本気だす〜』 第2期 オリジナル・サウンドトラック" +OST2 = "『無職転生 〜異世界行ったら本気だす〜』 第2期 オリジナル・サウンドトラック2" + def test_helper_volume_and_sequel_differ(): assert numeric_tokens_differ(VOL4, VOL45) @@ -26,11 +32,20 @@ def test_helper_volume_and_sequel_differ(): assert numeric_tokens_differ("Now 99", "Now 100") +def test_helper_cjk_trailing_sequel_digit_differs(): + # The trailing '2' must register as a difference even though '第2期' already + # puts a '2' on both sides. + assert numeric_tokens_differ(OST, OST2) + assert numeric_tokens_differ(OST2, OST) + + def test_helper_shared_or_no_digits_match(): assert not numeric_tokens_differ("1989", "1989 (Deluxe)") assert not numeric_tokens_differ(VOL4, VOL4) assert not numeric_tokens_differ("IGOR", "IGOR (Deluxe)") assert not numeric_tokens_differ("", "") + # Same CJK album on both sides (incl. the shared 第2期) still matches. + assert not numeric_tokens_differ(OST, OST) def _service_with_results(results): diff --git a/tests/metadata/test_art_lookup.py b/tests/metadata/test_art_lookup.py index 92a77a82..335974ee 100644 --- a/tests/metadata/test_art_lookup.py +++ b/tests/metadata/test_art_lookup.py @@ -202,6 +202,19 @@ def test_album_matches_rejects_numeric_difference(): assert art_lookup._album_matches("Taylor Swift", "1989", "Taylor Swift", "1989 (Deluxe)") +def test_album_matches_rejects_cjk_trailing_sequel_digit(): + """Sokhi #2: the sequel '2' is glued straight onto a CJK word + ('…サウンドトラック2'), and '第2期' (season 2) already puts a '2' on both + sides — so the digit-strip collapsed both to {'2'} and the cour-2 + soundtrack's cover hung on the base soundtrack.""" + ART = "藤澤慶昌" + OST = "『無職転生 〜異世界行ったら本気だす〜』 第2期 オリジナル・サウンドトラック" + assert not art_lookup._album_matches(ART, OST, ART, OST + "2") + assert not art_lookup._album_matches(ART, OST + "2", ART, OST) + # The genuine base-album hit still matches (incl. its shared 第2期). + assert art_lookup._album_matches(ART, OST, ART, OST) + + # --------------------------------------------------------------------------- # build_art_lookup — caching + guarding # --------------------------------------------------------------------------- diff --git a/tests/test_empty_folder_cleaner.py b/tests/test_empty_folder_cleaner.py index 54f65ca0..776a8e5a 100644 --- a/tests/test_empty_folder_cleaner.py +++ b/tests/test_empty_folder_cleaner.py @@ -36,6 +36,23 @@ def test_is_junk(): assert is_junk('.DS_Store') and is_junk('thumbs.db') and not is_junk('cover.jpg') +# ── #891: residual (image / sidecar only) folders ─────────────────────────── +def test_image_only_dir_kept_by_default_removed_with_residual_opt(): + # Default (junk only): a cover.jpg keeps the folder (the conservative behavior). + assert dir_is_removable(['cover.jpg'], []) is False + # Opt-in: image/sidecar-only folders become removable. + assert dir_is_removable(['cover.jpg'], [], ignore_disposable=True) is True + assert dir_is_removable(['back.jpg', 'lyrics.lrc', '.DS_Store'], [], ignore_disposable=True) is True + assert dir_is_removable(['folder.png', 'album.nfo'], [], ignore_disposable=True) is True + + +def test_residual_opt_still_keeps_real_content(): + # Audio, or anything not recognized as a leftover (a booklet pdf), still blocks. + assert dir_is_removable(['cover.jpg', 'song.flac'], [], ignore_disposable=True) is False + assert dir_is_removable(['cover.jpg', 'booklet.pdf'], [], ignore_disposable=True) is False + assert dir_is_removable([], ['Album'], ignore_disposable=True) is False # surviving subdir + + # ── apply re-check (real FS) ──────────────────────────────────────────────── def _fx(): return dict(listdir=os.listdir, isdir=os.path.isdir, islink=os.path.islink, @@ -72,3 +89,34 @@ def test_apply_refuses_library_root(tmp_path): res = remove_empty_folder(str(root), junk_files=[], remove_junk=True, root=str(root), **_fx()) assert res['removed'] is False and 'root' in res['error'].lower() assert root.exists() + + +def test_apply_sweeps_residual_then_folder_when_enabled(tmp_path): + root = tmp_path / 'lib'; root.mkdir() + d = root / 'Artist' / 'Old Album'; d.mkdir(parents=True) + (d / 'cover.jpg').write_text('img') + (d / 'back.jpg').write_text('img') + (d / 'lyrics.lrc').write_text('la') + res = remove_empty_folder(str(d), junk_files=[], remove_junk=True, + remove_disposable=True, root=str(root), **_fx()) + assert res['removed'] is True and not d.exists() + + +def test_apply_without_residual_opt_leaves_image_folder(tmp_path): + # The default apply (no residual opt) must NOT delete a cover.jpg folder. + root = tmp_path / 'lib'; root.mkdir() + d = root / 'HasCover'; d.mkdir() + (d / 'cover.jpg').write_text('img') + res = remove_empty_folder(str(d), junk_files=[], remove_junk=True, root=str(root), **_fx()) + assert res['removed'] is False and d.exists() + + +def test_apply_residual_opt_still_refuses_real_content(tmp_path): + root = tmp_path / 'lib'; root.mkdir() + d = root / 'Mixed'; d.mkdir() + (d / 'cover.jpg').write_text('img') + (d / 'booklet.pdf').write_text('pdf') # unrecognized → real content + res = remove_empty_folder(str(d), junk_files=[], remove_junk=True, + remove_disposable=True, root=str(root), **_fx()) + assert res['removed'] is False and d.exists() + assert (d / 'booklet.pdf').exists() and (d / 'cover.jpg').exists() # nothing deleted diff --git a/tests/test_enrichment_tag_preservation.py b/tests/test_enrichment_tag_preservation.py new file mode 100644 index 00000000..22127ee9 --- /dev/null +++ b/tests/test_enrichment_tag_preservation.py @@ -0,0 +1,100 @@ +"""Sokhi: tracks occasionally land 'untagged' after a processing failure. + +enhance_file_metadata clears the file's tags and saves it UP FRONT (so stale +tags never linger), then does the failure-prone enrichment (external source-id +embed, cover-art fetch) and saves again at the end. The core tags +(album/artist/title/track) come from the already-matched context and are written +to the in-memory object BEFORE those external steps — but the on-disk file is +still the cleared one until the final save. + +The #764 fix made the error handler restore ART, but it gated the re-save on +there being original art to restore. So a file with NO embedded art that hit a +mid-enrichment crash had its in-memory core tags thrown away and was left on disk +exactly as the up-front clear saved it: UNTAGGED. + +These tests run the REAL enhance_file_metadata against a REAL art-less FLAC and +assert the core tags survive a crash in the external step. +""" + +from __future__ import annotations + +import os +import tempfile +from unittest.mock import patch + +import pytest + +pytest.importorskip("mutagen") +from mutagen.flac import FLAC # noqa: E402 + +import core.metadata.enrichment as enrichment # noqa: E402 + + +class _Cfg: + def get(self, key, default=None): + return default + + +def _make_flac_no_art(path): + minimal = ( + b"fLaC" + + b"\x80\x00\x00\x22" + + b"\x00\x10\x00\x10" + + b"\x00\x00\x00\x00\x00\x00" + + b"\x0a\xc4\x42\xf0\x00\x00\x00\x00" + + b"\x00" * 16 + ) + with open(path, "wb") as f: + f.write(minimal) + FLAC(path).save() # valid FLAC, no tags, no pictures + + +@pytest.fixture +def flac_path(): + fd, path = tempfile.mkstemp(suffix=".flac") + os.close(fd) + _make_flac_no_art(path) + yield path + try: + os.remove(path) + except OSError: + pass + + +_CORE = {"title": "Yellow", "artist": "Coldplay", "album_artist": "Coldplay", + "album": "Parachutes", "track_number": 1, "total_tracks": 9, "disc_number": 1} + + +def _run(flac_path, *, metadata, embed_side_effect): + with patch.object(enrichment, "get_config_manager", return_value=_Cfg()), \ + patch.object(enrichment, "strip_all_non_audio_tags"), \ + patch.object(enrichment, "extract_source_metadata", return_value=metadata), \ + patch.object(enrichment, "embed_source_ids"), \ + patch.object(enrichment, "verify_metadata_written", return_value=True), \ + patch.object(enrichment, "embed_album_art_metadata", side_effect=embed_side_effect): + return enrichment.enhance_file_metadata( + flac_path, context={}, artist={"name": "Coldplay"}, album_info={}, + ) + + +def test_core_tags_survive_when_art_step_raises_on_artless_file(flac_path): + """The regression: art-less file + a crash in the external art step must NOT + leave the file untagged — the matched core tags must be on disk.""" + def boom(audio_file, metadata): + raise RuntimeError("art backend exploded") + + result = _run(flac_path, metadata=dict(_CORE), embed_side_effect=boom) + assert result is False # enrichment reported failure + f = FLAC(flac_path) + assert f.get("title") == ["Yellow"] # ...but core tags persisted + assert f.get("artist") == ["Coldplay"] + assert f.get("album") == ["Parachutes"] # the tag Rockbox buckets on + assert f.get("tracknumber") == ["1/9"] + + +def test_core_tags_written_on_happy_path_artless_file(flac_path): + result = _run(flac_path, metadata=dict(_CORE), embed_side_effect=lambda *a, **k: False) + assert result is True + f = FLAC(flac_path) + assert f.get("album") == ["Parachutes"] + assert f.get("artist") == ["Coldplay"] diff --git a/tests/test_repair_scheduler_tz.py b/tests/test_repair_scheduler_tz.py new file mode 100644 index 00000000..ff869db4 --- /dev/null +++ b/tests/test_repair_scheduler_tz.py @@ -0,0 +1,73 @@ +"""#885: repair-job scheduling must be timezone-independent. + +`finished_at` is written by SQLite's CURRENT_TIMESTAMP (always UTC), but the +scheduler compared it against `datetime.now()` (naive LOCAL). With TZ=Australia/ +Sydney (UTC+11) every job looked ~11h stale and ran every poll; America/New_York +(behind UTC) masked it. The fix parses finished_at as UTC and compares against a +UTC now, so the machine timezone no longer leaks into elapsed time. +""" + +from __future__ import annotations + +import time +from datetime import datetime, timezone + +import pytest + +from core.repair_worker import RepairWorker + + +# ── pure helper ─────────────────────────────────────────────────────────────── +def test_hours_since_treats_naive_timestamp_as_utc(): + now = datetime(2026, 6, 18, 6, 0, 0, tzinfo=timezone.utc) + # SQLite CURRENT_TIMESTAMP style: UTC, no tz suffix. + assert RepairWorker._hours_since('2026-06-18 00:00:00', now) == pytest.approx(6.0) + + +def test_hours_since_handles_aware_timestamp(): + now = datetime(2026, 6, 18, 6, 0, 0, tzinfo=timezone.utc) + assert RepairWorker._hours_since('2026-06-18T00:00:00+00:00', now) == pytest.approx(6.0) + + +def test_hours_since_recent_is_near_zero(): + now = datetime(2026, 6, 18, 0, 0, 30, tzinfo=timezone.utc) + assert RepairWorker._hours_since('2026-06-18 00:00:00', now) == pytest.approx(30 / 3600, abs=1e-6) + + +# ── the #885 repro: a just-run job is never due, regardless of timezone ──────── +def _set_tz(monkeypatch, tz): + monkeypatch.setenv('TZ', tz) + try: + time.tzset() + except AttributeError: + pytest.skip('time.tzset() unavailable on this platform') + + +def test_just_run_job_not_due_under_any_timezone(monkeypatch): + w = RepairWorker.__new__(RepairWorker) + w._jobs = {'cache_evictor': object()} + monkeypatch.setattr(RepairWorker, 'get_job_config', + lambda self, jid: {'enabled': True, 'interval_hours': 6}) + # Job finished "now" in UTC (exactly how CURRENT_TIMESTAMP records it). + finished = datetime.now(timezone.utc).strftime('%Y-%m-%d %H:%M:%S') + monkeypatch.setattr(RepairWorker, '_get_last_run', + lambda self, jid: {'finished_at': finished}) + + # Australia/Sydney is the exact repro; check the Americas + UTC too. + for tz in ('Australia/Sydney', 'America/New_York', 'UTC'): + _set_tz(monkeypatch, tz) + assert w._pick_next_job() is None, f"just-run job wrongly due under TZ={tz}" + + +def test_stale_job_is_still_picked_under_sydney(monkeypatch): + # Sanity: a genuinely-overdue job IS picked (we didn't break due-detection). + w = RepairWorker.__new__(RepairWorker) + w._jobs = {'cache_evictor': object()} + monkeypatch.setattr(RepairWorker, 'get_job_config', + lambda self, jid: {'enabled': True, 'interval_hours': 6}) + # Finished ~10h ago in UTC. + old = datetime(2026, 1, 1, 0, 0, 0, tzinfo=timezone.utc).strftime('%Y-%m-%d %H:%M:%S') + monkeypatch.setattr(RepairWorker, '_get_last_run', + lambda self, jid: {'finished_at': old}) + _set_tz(monkeypatch, 'Australia/Sydney') + assert w._pick_next_job() == 'cache_evictor' diff --git a/tests/test_spotify_worker_status.py b/tests/test_spotify_worker_status.py new file mode 100644 index 00000000..a75b0382 --- /dev/null +++ b/tests/test_spotify_worker_status.py @@ -0,0 +1,68 @@ +"""Issue #887: the Spotify enrichment worker's get_stats() must report +``using_free`` when it's enriching via the no-creds Spotify Free source — even +with no official auth — so the dashboard shows "Running (Spotify Free)" instead +of a misleading "Not Authenticated". + +Builds the worker via __new__ (bypassing the real SpotifyClient()) and stubs the +db-querying helpers, so the get_stats() free/auth logic is tested in isolation. +""" + +from __future__ import annotations + +import types + +from core.spotify_worker import SpotifyWorker + + +def _worker(*, serving_via_free, sp=None, rate_limited=False, budget_free=False): + w = SpotifyWorker.__new__(SpotifyWorker) + w.running = True + w.paused = False + w.thread = types.SimpleNamespace(is_alive=lambda: True) + w.current_item = None + w.stats = {'pending': 0, 'processed': 0} + w._serving_via_free = serving_via_free + w.client = types.SimpleNamespace( + sp=sp, + is_rate_limited=lambda: rate_limited, + get_rate_limit_info=lambda: None, + get_post_ban_cooldown_remaining=lambda: 0, + is_spotify_metadata_available=lambda: True, + _budget_exhausted_use_free=budget_free, + ) + # db-backed helpers stubbed — we only exercise the auth/free reporting. + w._count_pending_items = lambda: 100 + w._get_progress_breakdown = lambda: {} + w._get_daily_budget_info = lambda: {'exhausted': False} + return w + + +def test_no_auth_but_serving_via_free_reports_using_free(): + # #887: no official auth (sp is None), worker enriching via Spotify Free. + stats = _worker(serving_via_free=True, sp=None).get_stats() + assert stats['authenticated'] is False # no official auth + assert stats['using_free'] is True # ...but Free is carrying it + + +def test_no_auth_and_not_serving_free_is_not_using_free(): + # Genuinely can't enrich (no auth, free not active) -> Not Authenticated stands. + stats = _worker(serving_via_free=False, sp=None).get_stats() + assert stats['authenticated'] is False + assert stats['using_free'] is False + + +def test_rate_limit_bridge_still_reports_using_free(): + # Pre-existing bridge path must still work (cache False, but rate-limited). + stats = _worker(serving_via_free=False, sp=object(), rate_limited=True).get_stats() + assert stats['using_free'] is True + + +def test_budget_bridge_still_reports_using_free(): + stats = _worker(serving_via_free=False, sp=object(), budget_free=True).get_stats() + assert stats['using_free'] is True + + +def test_authed_and_not_on_free_reports_authenticated_not_free(): + stats = _worker(serving_via_free=False, sp=object()).get_stats() + assert stats['authenticated'] is True + assert stats['using_free'] is False diff --git a/tests/test_usenet_client_adapters.py b/tests/test_usenet_client_adapters.py index 1445a36c..095d100d 100644 --- a/tests/test_usenet_client_adapters.py +++ b/tests/test_usenet_client_adapters.py @@ -714,6 +714,67 @@ def test_nzbget_parse_group_computes_progress() -> None: assert status.download_speed == 500_000 +def test_nzbget_queue_group_never_offers_a_save_path() -> None: + """A queued group's DestDir is the in-progress '….#NZBID' dir, which is gone + after the move — never expose it as a final save_path. Finalisation must come + from the HISTORY entry. (Swigs: imported from /…/incomplete/….#2141.)""" + adapter = _nzbget_with_config() + status = adapter._parse_group({ + 'NZBID': 2141, 'NZBName': 'xRepentancex-The.Sickness.Of.Eden', + 'Status': 'DOWNLOADING', + 'DestDir': '/data/usenet/incomplete/xRepentancex-The.Sickness.Of.Eden.#2141', + 'Category': 'soulsync', + }) + assert status.save_path is None + + +def test_nzbget_pp_finished_group_is_completed_but_has_no_path() -> None: + """PP_FINISHED maps to 'completed' but is still a QUEUE group on the + in-progress dir — it must report no save_path so the plugin waits for the + history entry instead of finalising on the incomplete folder.""" + adapter = _nzbget_with_config() + status = adapter._parse_group({ + 'NZBID': 2141, 'NZBName': 'Album', 'Status': 'PP_FINISHED', + 'DestDir': '/data/usenet/incomplete/Album.#2141', 'Category': 'soulsync', + }) + assert status.state == 'completed' + assert status.save_path is None + + +def test_nzbget_history_prefers_finaldir_over_destdir() -> None: + """After a post-processing move, FinalDir is the real location; DestDir can + still be the intermediate dir. Swigs' exact case.""" + adapter = _nzbget_with_config() + status = adapter._parse_history({ + 'NZBID': 2141, 'Name': 'xRepentancex-The.Sickness.Of.Eden', + 'Status': 'SUCCESS/ALL', + 'DestDir': '/data/usenet/incomplete/xRepentancex-The.Sickness.Of.Eden.#2141', + 'FinalDir': '/data/soulseek/xRepentancex-The.Sickness.Of.Eden-CD-FLAC-2015-CATARACT', + 'Category': 'soulsync', + }) + assert status.state == 'completed' + assert status.save_path == '/data/soulseek/xRepentancex-The.Sickness.Of.Eden-CD-FLAC-2015-CATARACT' + + +def test_nzbget_history_falls_back_to_destdir_when_no_finaldir() -> None: + """No PP move -> FinalDir empty -> use DestDir (the final dest in that case).""" + adapter = _nzbget_with_config() + status = adapter._parse_history({ + 'NZBID': 7, 'Name': 'Album', 'Status': 'SUCCESS/HEALTH', + 'DestDir': '/data/usenet/completed/Album', 'FinalDir': '', 'Category': 'c', + }) + assert status.save_path == '/data/usenet/completed/Album' + + +def test_nzbget_history_empty_dirs_yield_none() -> None: + adapter = _nzbget_with_config() + status = adapter._parse_history({ + 'NZBID': 7, 'Name': 'Album', 'Status': 'SUCCESS/ALL', + 'DestDir': ' ', 'FinalDir': '', 'Category': 'c', + }) + assert status.save_path is None + + def test_nzbget_remove_rejects_non_numeric_id() -> None: """NZBGet IDs are ints; passing a string id like 'abc' must fail fast instead of corrupting the editqueue call.""" diff --git a/web_server.py b/web_server.py index 6d9cdb3d..46e7fec4 100644 --- a/web_server.py +++ b/web_server.py @@ -40,7 +40,7 @@ logger = setup_logging(_log_level, _log_path) # App version — single source of truth for backup metadata, system-info, update check, etc. # Semver: MAJOR.MINOR.PATCH. Bump at each dev→main release. -_SOULSYNC_BASE_VERSION = "2.7.3" +_SOULSYNC_BASE_VERSION = "2.7.4" def _build_version_string(): """Append short commit hash to version when available (e.g. 2.35+abc1234).""" @@ -10044,6 +10044,147 @@ def get_artist_enhanced_detail(artist_id): except Exception as e: return jsonify({"success": False, "error": str(e)}), 500 + +# ── Re-identify an imported track (#889) ── +@app.route('/api/reidentify/sources', methods=['GET']) +def reidentify_sources(): + """Source tabs for the Re-identify modal — every metadata source with a live + client, the active one flagged so the UI selects it by default.""" + try: + from core.imports.rematch_search import available_sources + return jsonify({"success": True, "sources": available_sources()}) + except Exception as e: + return jsonify({"success": False, "error": str(e), "sources": []}), 500 + + +@app.route('/api/reidentify/search', methods=['GET']) +def reidentify_search(): + """Search one metadata source for the releases a track appears on. + + Query params: ``source`` (defaults to the active source), ``q`` (the query), + ``limit``. Returns display rows — the SAME song across single/EP/album, each + with a type badge — for the user to pick. album_id is resolved later, only for + the chosen row.""" + try: + from core.imports.rematch_search import available_sources, search_release_candidates + query = (request.args.get('q') or '').strip() + if not query: + return jsonify({"success": True, "results": []}) + source = (request.args.get('source') or '').strip() + if not source: + actives = [s for s in available_sources() if s.get('active')] + source = actives[0]['source'] if actives else 'spotify' + try: + limit = max(1, min(50, int(request.args.get('limit', 25)))) + except (TypeError, ValueError): + limit = 25 + rows = search_release_candidates(source, query, limit=limit) + return jsonify({"success": True, "source": source, "results": rows}) + except Exception as e: + logger.error(f"Re-identify search error: {e}") + return jsonify({"success": False, "error": str(e), "results": []}), 500 + + +@app.route('/api/reidentify/apply', methods=['POST']) +def reidentify_apply(): + """Apply a re-identify: stage the track's library file + write a single-use hint + so the auto-import worker re-files it under the chosen release (Phase 2). + + Body: ``{library_track_id, source, track_id, replace}``. Admin-only (mutates the + library). COPIES the file — the original is removed only after the re-import + succeeds, and only when ``replace`` is true.""" + try: + database = get_database() + pid = get_current_profile_id() + prof = database.get_profile(pid) if pid else None + if not prof or not prof.get('is_admin'): + return jsonify({"success": False, "error": "Admin only"}), 403 + + data = request.get_json(silent=True) or {} + library_track_id = data.get('library_track_id') + source = (data.get('source') or '').strip() + track_id = (data.get('track_id') or '').strip() + replace = bool(data.get('replace', True)) + if not library_track_id or not source or not track_id: + return jsonify({"success": False, "error": "library_track_id, source and track_id are required"}), 400 + + from core.imports.rematch_search import resolve_hint_fields + from core.imports.rematch_apply import stage_file_for_reidentify, build_reidentify_hint + from core.imports.rematch_hints import create_hint + + # 1) Resolve the picked release → the IDs the hint needs (album_id critically). + hint_fields = resolve_hint_fields(source, track_id) + if not hint_fields: + return jsonify({"success": False, "error": "Could not resolve the selected release (no album id)"}), 400 + + # 2) Locate the library file for this track. + conn = database._get_connection() + try: + cur = conn.cursor() + cur.execute("SELECT file_path FROM tracks WHERE id = ?", (str(library_track_id),)) + row = cur.fetchone() + finally: + conn.close() + if not row or not row['file_path']: + return jsonify({"success": False, "error": "Library track has no file on disk"}), 404 + stored_path = row['file_path'] + + # Resolve the stored DB path to a file THIS process can actually read, using + # the SAME strong resolver the rest of the app uses (transfer/download/library/ + # Plex search + #833 confusable folding via find_on_disk). + real_path = _resolve_library_file_path(stored_path) + if not real_path: + # On a miss, run the diagnostic variant purely to tell us (and the user) + # what was tried — instead of failing on the raw, possibly-stale path. + from core.library.path_resolver import resolve_library_file_path_with_diagnostic + try: + _plex = media_server_engine.client('plex') if media_server_engine else None + except Exception: + _plex = None + _, attempt = resolve_library_file_path_with_diagnostic( + stored_path, config_manager=config_manager, plex_client=_plex) + searched = ", ".join(attempt.base_dirs_tried) or "(no library/transfer/download dirs configured)" + logger.warning("[Re-identify] could not locate track %s file — stored=%s raw_exists=%s searched=[%s]", + library_track_id, stored_path, attempt.raw_path_existed, searched) + return jsonify({"success": False, "error": ( + f"SoulSync couldn't find this track's file on disk.\nStored path: {stored_path}\n" + f"Searched: {searched}.\nIf the file lives on a media server SoulSync can't read directly " + f"(or the stored path is stale), re-identify isn't available for it.")}), 404 + + # 3) Copy into staging + fingerprint the copy. + staging_dir = docker_resolve_path(config_manager.get('import.staging_path', './Staging')) + staged = stage_file_for_reidentify(real_path, staging_dir, library_track_id) + + # 4) Persist the single-use hint. + hint = build_reidentify_hint(library_track_id, hint_fields, + staged['staged_path'], staged['content_hash'], replace=replace) + conn = database._get_connection() + try: + cur = conn.cursor() + hint_id = create_hint(cur, hint) + conn.commit() + finally: + conn.close() + + # 5) Nudge the worker so it doesn't wait for the next timer tick. + try: + if auto_import_worker is not None: + auto_import_worker.trigger_scan() + except Exception as _e: + logger.debug("Re-identify: scan nudge failed (worker will catch it on its timer): %s", _e) + + logger.info("[Re-identify] staged track %s → %s '%s' (%s), replace=%s", + library_track_id, hint.album_type or 'release', hint.album_name or '?', + source, replace) + return jsonify({"success": True, "hint_id": hint_id, "staged_path": staged['staged_path'], + "album_name": hint.album_name, "album_type": hint.album_type}) + except FileNotFoundError as e: + return jsonify({"success": False, "error": f"Source file not found: {e}"}), 404 + except Exception as e: + logger.error(f"Re-identify apply error: {e}") + return jsonify({"success": False, "error": str(e)}), 500 + + @app.route('/api/library/artist//quality-analysis') def get_artist_quality_analysis(artist_id): """Analyze track quality for an artist — returns tier classification for each track.""" diff --git a/webui/index.html b/webui/index.html index af40adfa..b682f8aa 100644 --- a/webui/index.html +++ b/webui/index.html @@ -5135,6 +5135,37 @@ + +
+
+ + Priority: 1.5 +
+
+
+ +
+ + +
+
+
+ 128 kbps + - + 400 kbps +
+
+
+
+
@@ -5730,6 +5761,13 @@ Order the sources to choose whose cover art is used. The first source that has a cover wins; misses fall through to the next, and if none match, your download's own art is kept. Only sources you're connected to are shown — leave all off to keep current behavior.
+
+ + When a track matches a single release, look up the album that contains it and tag it as that album — so every song in an album gets the same (album) cover instead of some getting the single's art. Off by default: adds an extra metadata lookup per single-matched track. +
+ + +