From 2b15260b88fc145e821870c0dc22a3653359a339 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Fri, 24 Apr 2026 18:16:11 -0700 Subject: [PATCH 1/2] Reorganize: route library files through the post-processing pipeline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported on Discord by winecountrygames. The library "Reorganize" tool had several layered bugs that all traced to the same root cause: the endpoint reinvented every wheel post-processing already turns — its own template engine, its own disc-number resolution from file tags, its own sidecar sweep, its own collision detection — and each had drifted from the canonical path used by fresh downloads. Reported symptoms: - 3-disc Aerosmith deluxe collapsed to a flat single-disc layout - Half the tracks on other albums silently skipped, no error / no count - Re-runs left empty leftover album folders cluttering the artist dir Architecture: stop reinventing wheels. Route reorganize through exactly the same pipeline downloads use. Per-album: 1. Fetch the canonical tracklist from a metadata source (Spotify / iTunes / Deezer / Discogs / Hydrabase) using the album's stored source IDs. New `core/library_reorganize.py::plan_album_reorganize` does this — primary-source-first, fall through priority chain unless the user picked a specific source in the modal (strict mode). 2. For each local track, find the matching API entry via a scored candidate matcher. Score components: exact-title (100), substring-with-length-ratio (40-90), track-number agreement (20). Hard reject when the two titles have different version differentiators (Remix vs no-remix means different recordings, not annotation drift). Below threshold = unmatched, surfaced as "not in source's tracklist, left in place" rather than silently mis-routing. 3. Copy the file to a per-album staging directory, build the same context dict the import flow builds (`spotify_album` / `track_info` / etc. with `is_album_download=True` so the path builder enters ALBUM mode, not SINGLE mode), call `_post_process_matched_download(...)` — same function fresh downloads use. Post-process handles tagging, multi-disc subfolder decisions, sidecar regeneration, AcoustID verification. 4. Read `context['_final_processed_path']` to learn where it landed. Update `tracks.file_path` in the DB BEFORE removing the original (DB-update failure leaves the file at both locations, recoverable via library scan; the reverse would orphan the row). Delete per-track sidecars (post-process recreates them at the new destination). 3 concurrent workers per album via ThreadPoolExecutor, matching the download path's per-batch worker count. State mutations all guarded by a single lock; staging filenames carry a UUID prefix so concurrent copies of identically-named source files don't overwrite each other. Source picker in the modal lets the user choose which source to read the tracklist from. Two endpoints feed it: - `/api/library/album//reorganize/sources` — sources for THIS album that are both authed AND have a stored ID. For the per- album modal. - `/api/library/reorganize/sources` — all authed sources globally. For the bulk "Reorganize All" modal where per-album ID coverage varies. When the user picks a specific source, the orchestrator runs in `strict_source=True` mode (no fallback chain) — picking Spotify means "use Spotify or fail", not "use Spotify and silently fall back." Preview endpoint shares the same planning logic as apply via `preview_album_reorganize` — the destination path comes from the same `_build_final_path_for_track` post-process uses, so what you see in the preview is exactly what you get on apply. Empty destination folders (from earlier failed runs OR from the current run when post-process creates a dir then fails AcoustID) get cleaned up after each successful run: walk up to the artist folder from any successful destination, prune empty album-sibling folders one level deep. Bounded scope = won't touch unrelated user dirs. Web_server.py shrinks by ~450 net lines. The endpoint handler is now a thin wrapper that builds injected callables (path resolver, post- process function, DB updater, empty-dir cleaner), spawns a thread that calls `reorganize_album()`, and returns. All actual logic lives in `core/library_reorganize.py` where it's unit-testable without spinning up Flask. Frontend cleanup: the per-call template input in both reorganize modals (per-album and bulk) was redundant — the backend always uses the configured global download template. Removed the input and the variables-grid reference UI it was for. 39 new unit tests pin every contract: - source resolution (no_source_id when album has none, fallthrough chain when primary returns nothing, strict mode bypasses fallback) - matcher scoring (exact / substring / multi-disc disambiguation / smart-quote tolerance / dash-vs-parens / bonus-track substring / Remix-vs-original differentiator rejection / "Real" doesn't false- match "Real Real Real" / track-number-only no longer fires) - file safety (DB-update failure leaves original in place, post- process failure leaves original in place, post-process exception caught and original preserved, success removes original AND updates DB in the right order) - sidecar handling (per-track .lrc/.nfo deleted on success, kept on failure; album-level cover.jpg/folder.jpg cleaned only when directory has no remaining audio) - staging cleanup (recreated between tracks because post-process nukes it, dir cleaned up on success AND on failure) - destination-dir prune (empty siblings removed, real album with files preserved, no recursive sweep) - source picker (only authed-with-stored-ID sources for per-album, all authed sources for bulk; strict mode doesn't fall back) - concurrency (3 workers in flight, state stays consistent under races, stop_check cuts off pending tasks) - preview parity (preview produces same destination as apply for multi-disc; ALBUM mode not SINGLE mode; unmatched/no-path tracks surfaced with reasons) Limitations (deliberate punts, NOT in this PR): - Renamed local titles on multi-disc albums where track_number also disagrees: matcher returns nothing (track is "not in source"). Fixable by using duration_ms as a tertiary signal. - Per-track in-modal source switching with per-album track-count hints (would need a second API call before opening the modal). - UI status panel on the artist page during a run — currently just toasts. Documented as a follow-up PR. Files: - core/library_reorganize.py — new module: plan_album_reorganize, preview_album_reorganize, reorganize_album, available_sources_for_album, authed_sources, _score_candidate, helpers for staging/post- processing/finalizing, sidecar + dest-dir cleanup - core/metadata_service.py — no changes; reused get_album_for_source, get_album_tracks_for_source, get_source_priority, get_client_for_source - web_server.py — three endpoints (preview / apply / sources GETs) are thin wrappers; -450 net lines - tests/test_library_reorganize_orchestrator.py — 39 tests covering every contract above - webui/static/library.js — source picker UI in both modals; dead template input + variables-grid removed - webui/static/style.css — dropdown option styling fix (white-on- white was unreadable) Reported on Discord by winecountrygames — his bug report named the trigger button (Enhanced view → Reorganize All) and both symptoms (multi-disc collapse, half-album skip), which let the diagnosis go straight to the architectural problem. --- core/library_reorganize.py | 1477 +++++++++++++ tests/test_library_reorganize_orchestrator.py | 1930 +++++++++++++++++ web_server.py | 501 +---- webui/static/helper.js | 1 + webui/static/library.js | 208 +- webui/static/style.css | 13 + 6 files changed, 3643 insertions(+), 487 deletions(-) create mode 100644 core/library_reorganize.py create mode 100644 tests/test_library_reorganize_orchestrator.py diff --git a/core/library_reorganize.py b/core/library_reorganize.py new file mode 100644 index 00000000..537c9729 --- /dev/null +++ b/core/library_reorganize.py @@ -0,0 +1,1477 @@ +"""Re-route a library album's existing files through the same +post-processing pipeline that handles fresh downloads. + +The old reorganize endpoint reinvented several wheels — its own template +engine, its own disc-number resolution from file tags, its own sidecar +sweep, its own collision detection. Each of those drifted from the +canonical post-processing path over time, producing reorganize-only +bugs (multi-disc deluxe collapsing to single-disc when even one file's +tag was missing; tracks silently skipped when their file paths didn't +resolve on disk; etc.). + +The new design follows the import page's pattern: copy each file to a +staging folder, build the same context dict the download workers +build, then call ``_post_process_matched_download`` for each one. +Post-processing already knows how to pick the right destination, write +the right tags, handle multi-disc subfolders, recreate sidecars (cover +art, lyrics), and run AcoustID verification — there's nothing for +reorganize to add on top. + +Hard requirement: the album must have at least one stored +metadata-source ID (spotify_album_id / itunes_album_id / deezer_id / +discogs_id / soul_id). With no source ID we have nothing authoritative +to ask for the canonical tracklist, and silently degrading to file +tags is exactly the failure mode the old code path produced. Albums +without a source ID are reported back to the caller and skipped +entirely. +""" + +import os +import shutil +import threading +import time +import uuid +from concurrent.futures import FIRST_COMPLETED, ThreadPoolExecutor, wait +from dataclasses import dataclass, field +from typing import Any, Callable, Dict, List, Optional, Set + +# Per-album track concurrency. Matches the download workers' per-batch +# concurrency (3) so reorganize feels comparable to a fresh download. +# +# Operational note: post-processing can spawn an ffmpeg subprocess per +# track if `lossy_copy.downsample_hires` is enabled. With 3 workers +# that's up to 3 concurrent ffmpeg processes. Acceptable for typical +# album sizes (10-20 tracks); on a giant single-album reorganize +# (50+ tracks) ffmpeg's transient memory could be noticeable but each +# subprocess is short-lived so total RAM doesn't pile up. If we ever +# see resource issues from this, drop to 2 here rather than disabling +# concurrency entirely. +_REORGANIZE_MAX_WORKERS = 3 + +# Watchdog interval — how often the orchestrator checks the worker +# pool while waiting for tasks to finish. Setting this to 30s means +# we log a warning naming any track that's been in flight longer than +# `_HUNG_WORKER_THRESHOLD` (so an operator can investigate) without +# burning CPU on a tight poll. Doesn't kill stuck threads (Python +# can't), just surfaces them. +_WATCHDOG_INTERVAL_SECONDS = 30 +_HUNG_WORKER_THRESHOLD_SECONDS = 300 # 5 min — generous; real worst-case + # is ffmpeg downsampling a long + # hi-res FLAC, ~30-60s typically. + +from core.metadata_service import ( + get_album_for_source, + get_album_tracks_for_source, + get_client_for_source, + get_primary_source, + get_source_priority, +) +from utils.logging_config import get_logger + +logger = get_logger("library_reorganize") + + +def _safe_filename(name: str) -> str: + """Strip path-illegal characters so we can use the value as a + filename component on the staging path.""" + return ''.join(c for c in (name or 'unknown') if c not in '<>:"/\\|?*').strip() or 'unknown' + + +def _normalize_album_tracks(api_tracks): + """Normalize the various provider tracklist shapes (dict-with-`items`, + bare list, ``None``) to a single list of item dicts.""" + if not api_tracks: + return [] + if isinstance(api_tracks, dict): + items = api_tracks.get('items') or [] + return items if items else [] + if isinstance(api_tracks, list): + return api_tracks + return [] + + +SUPPORTED_SOURCES = ('spotify', 'itunes', 'deezer', 'discogs', 'hydrabase') + +# Per-source album-ID column mapping on the `albums` table row. +_ALBUM_ID_COLUMNS = { + 'spotify': 'spotify_album_id', + 'itunes': 'itunes_album_id', + 'deezer': 'deezer_id', + 'discogs': 'discogs_id', + 'hydrabase': 'soul_id', +} + +# Human-facing label for each source. +SOURCE_LABELS = { + 'spotify': 'Spotify', + 'itunes': 'Apple Music (iTunes)', + 'deezer': 'Deezer', + 'discogs': 'Discogs', + 'hydrabase': 'Hydrabase', +} + + +def _extract_source_ids(album_data: dict) -> Dict[str, str]: + """Pull the per-source album-ID strings off an album row.""" + return { + source: (album_data.get(column) or '') + for source, column in _ALBUM_ID_COLUMNS.items() + } + + +def available_sources_for_album(album_data: dict) -> List[dict]: + """Return the list of metadata sources the user can pick for this + album's reorganize. Every entry has both (a) a stored album ID on + the local row AND (b) an authenticated / configured client on this + SoulSync instance. + + Returns entries in source-priority order (preferred source first). + Each entry is ``{'source': str, 'label': str}``. No API calls — + purely local inspection. + """ + source_ids = _extract_source_ids(album_data) + try: + primary = get_primary_source() + except Exception: + primary = 'deezer' + + out = [] + for source in get_source_priority(primary): + if source not in SUPPORTED_SOURCES: + continue + if not source_ids.get(source): + continue + if get_client_for_source(source) is None: + continue + out.append({ + 'source': source, + 'label': SOURCE_LABELS.get(source, source), + }) + return out + + +def authed_sources() -> List[dict]: + """Return all metadata sources the user has authed/configured on + this SoulSync instance. Doesn't require any album-specific stored + ID — used by the bulk "Reorganize All" picker where each album + has its own ID coverage and we just want to know which sources + are reachable. Returned in priority order.""" + try: + primary = get_primary_source() + except Exception: + primary = 'deezer' + + out = [] + for source in get_source_priority(primary): + if source not in SUPPORTED_SOURCES: + continue + if get_client_for_source(source) is None: + continue + out.append({ + 'source': source, + 'label': SOURCE_LABELS.get(source, source), + }) + return out + + +def _resolve_source(album_data: dict, primary_source: str, strict_source: bool = False): + """Walk the configured source priority looking for the first source + we have an ID for AND that returns a usable tracklist. + + When ``strict_source`` is True, only the caller-provided + ``primary_source`` is tried — no fallback. Used when the user has + explicitly picked a source in the reorganize modal: picking Spotify + means "use Spotify or fail", not "use Spotify and silently fall + back to Deezer". + + Returns ``(source_name, album_meta, tracks_list)`` or ``(None, None, None)``. + """ + source_ids = _extract_source_ids(album_data) + + if strict_source: + sources_to_try = [primary_source] if primary_source else [] + else: + sources_to_try = get_source_priority(primary_source) + + for source in sources_to_try: + sid = source_ids.get(source) or '' + if not sid: + continue + try: + api_album = get_album_for_source(source, sid) + api_tracks = get_album_tracks_for_source(source, sid) + except Exception as e: + logger.warning(f"[Reorganize] {source} lookup raised: {e}") + continue + items = _normalize_album_tracks(api_tracks) + if not items or not api_album: + continue + return source, api_album, items + + return None, None, None + + +# Tokens that indicate a *different recording* of a track — when one +# side of a comparison has these and the other doesn't, the two are NOT +# the same track (e.g. "Bitch Don't Kill My Vibe" vs "Bitch Don't Kill +# My Vibe (Remix)" are different recordings; the tier 4 substring match +# would silently merge them otherwise). "Bonus track" is intentionally +# NOT here — it's a marketing annotation, not a recording difference. +_VERSION_DIFFERENTIATORS = frozenset({ + 'remix', 'remixed', + 'live', 'unplugged', 'concert', + 'acoustic', + 'demo', + 'extended', 'edit', + 'instrumental', 'karaoke', + 'remaster', 'remastered', 'remastering', + 'mono', 'stereo', + 'acapella', 'cappella', + 'cover', + 'reprise', + 'alternate', 'alt', + 'rehearsal', +}) + + +def _differentiators_in(norm_title: str) -> frozenset: + """Return the set of version-differentiator tokens present in a + normalized title. Used by the tier-4 matcher to reject substring + matches across different recordings of the same song.""" + if not norm_title: + return frozenset() + return frozenset(t for t in norm_title.split() if t in _VERSION_DIFFERENTIATORS) + + +def _normalize_title(value) -> str: + """Lowercase + strip cosmetic punctuation and treat brackets / dashes + / slashes as word separators so the same track named slightly + differently across providers and user libraries still matches. + + Examples that should normalize equal: + + - ``Bitch, Don't Kill My Vibe - Remix`` ↔ ``Bitch, Don't Kill My Vibe (Remix)`` + - ``Don't Stop Believin'`` ↔ ``Don’t Stop Believin’`` + - ``Swimming Pools (Drank) - Extended Version`` + ↔ ``Swimming Pools (Drank) (Extended Version)`` + """ + if value is None: + return '' + out = str(value).strip().lower() + # Strip characters that don't carry meaning across providers. + for ch in ('"', "'", '‘', '’', '“', '”', '.', ',', '!', '?', + '(', ')', '[', ']', '{', '}'): + out = out.replace(ch, '') + # Treat separators as whitespace so "foo - bar" and "foo (bar)" align. + for ch in ('-', '–', '—', ':', '/', '\\'): + out = out.replace(ch, ' ') + return ' '.join(out.split()) + + +# Title-match scoring grid. Each component's weight was picked to +# satisfy these design rules: +# +# 1. EXACT title alone is enough to win. +# 2. SUBSTRING at the high-confidence floor (≥0.6) is enough to win. +# 3. SUBSTRING at the lower with-tn-match floor (≥0.3) needs the +# track_number bonus to win — track_number provides the missing +# confidence. +# 4. TRACK-NUMBER alone is NOT enough — never falls through to a +# blind track-number lookup on multi-disc albums (that's the +# bug that mis-routed winecountrygames's bonus tracks). +# 5. Different version-differentiator tokens (Remix vs no-remix) +# hard-reject before scoring (see `_score_candidate`). +# +# Worked examples (with threshold = 50): +# +# exact title + tn match 100 + 20 = 120 → match +# exact title alone 100 = 100 → match +# substring ratio 1.0 (no tn match) 50 + 40 = 90 → match +# substring ratio 0.6 (no tn match) 50 + 0 = 50 → match +# substring ratio 0.5 (no tn match) 0 = 0 → no match +# substring ratio 0.45 + tn match 40 + 20 = 60 → match +# substring ratio 0.28 + tn match 0 + 20 = 20 → no match +# (Real vs "Real Real Real") +# track_number alone (no title signal) 0 + 20 = 20 → no match +# different version diffs (any inputs) hard-reject → 0 +# +# Weights are deliberately spaced so each gate is well-clear of the +# threshold; small ratio adjustments don't flip a borderline case +# unexpectedly. + +_MATCH_SCORE_THRESHOLD = 50 + +_W_EXACT_TITLE = 100 +_W_TRACK_NUMBER = 20 + +# Standalone substring (no tn match required): floor + scaled bonus. +# At ratio = floor: contribute base only. At ratio = 1.0: contribute +# base + range. Linear in between. +_W_SUBSTRING_BASE_STANDALONE = 50 +_W_SUBSTRING_RATIO_RANGE = 40 +_SUBSTRING_RATIO_FLOOR_STANDALONE = 0.6 + +# With-tn-match substring: lower floor (0.3) but slightly reduced +# base (40) so this path never beats a standalone high-ratio match +# on equal-tn ties. +_W_SUBSTRING_BASE_WITH_TN = 40 +_SUBSTRING_RATIO_FLOOR_WITH_TN = 0.3 + + +def _score_candidate( + norm_local: str, + local_tn: Optional[int], + local_diffs: frozenset, + api_norm: str, + api_tn: Optional[int], +) -> int: + """Score a single API candidate against the local track. Higher + means more confident match; 0 means no usable signal. The orchestrator + picks the highest-scoring candidate above + :data:`_MATCH_SCORE_THRESHOLD` and treats sub-threshold tracks as + unmatched (the "trust the source — if it doesn't have the track, + skip it" design policy). + + Components: + + - **Exact normalized-title match** is the strongest signal — usually + enough on its own, especially because local titles SoulSync wrote + should already match the source's text after normalization. + - **Substring containment** with a length-ratio guard handles + annotation drift like ``"The Recipe - Bonus Track"`` (local) + matching ``"The Recipe"`` (API). The ratio bonus rewards more + specific matches, so longer common prefixes win over shorter ones. + - **Track-number agreement** is a tiebreaker, never enough alone + (track_number-only would mis-route on multi-disc). + - **Version-differentiator mismatch** is a hard reject — if local + has ``Remix`` and API doesn't (or vice versa), they're different + recordings, not annotation drift. Returns 0 unconditionally. + """ + if not norm_local or not api_norm: + return 0 + + # Hard reject: version differentiators must agree exactly. ``Remix`` + # vs no-remix means different recordings, regardless of how + # otherwise-similar the titles are. + if _differentiators_in(api_norm) != local_diffs: + return 0 + + score = 0 + tn_match = local_tn is not None and api_tn == local_tn + + if api_norm == norm_local: + score += _W_EXACT_TITLE + else: + if api_norm in norm_local: + ratio = len(api_norm) / max(len(norm_local), 1) + elif norm_local in api_norm: + ratio = len(norm_local) / max(len(api_norm), 1) + else: + ratio = 0.0 + if ratio >= _SUBSTRING_RATIO_FLOOR_STANDALONE: + # Strong substring — credit regardless of tn agreement. + normalized = ( + (ratio - _SUBSTRING_RATIO_FLOOR_STANDALONE) + / (1.0 - _SUBSTRING_RATIO_FLOOR_STANDALONE) + ) + score += _W_SUBSTRING_BASE_STANDALONE + int(normalized * _W_SUBSTRING_RATIO_RANGE) + elif tn_match and ratio >= _SUBSTRING_RATIO_FLOOR_WITH_TN: + # Weaker substring (e.g., "the recipe" in "the recipe bonus + # track" at ratio 0.45) — accept ONLY because track_number + # also matches, and at slightly reduced base score. + score += _W_SUBSTRING_BASE_WITH_TN + + if tn_match: + score += _W_TRACK_NUMBER + + return score + + +def _prenormalize_api_tracks(api_tracks: List[dict]) -> List[tuple]: + """Compute ``(item, normalized_title, parsed_track_number)`` once + per API track so the matcher doesn't redo this work on every local + track. Callers that match many local tracks against the same API + list (the orchestrator's per-album loop) should hold this list and + pass it to :func:`_find_api_track`. + + For a 17-track local library matched against a 22-track API list, + avoiding re-normalization saves 17×22 = 374 normalize calls per + album reorganize.""" + out = [] + for item in api_tracks: + api_norm = _normalize_title(item.get('name') or item.get('title')) + try: + api_tn = int(item.get('track_number')) if item.get('track_number') is not None else None + except (TypeError, ValueError): + api_tn = None + out.append((item, api_norm, api_tn)) + return out + + +def _find_api_track(api_tracks, db_title: str, db_track_number) -> Optional[dict]: + """Find the API track that corresponds to a given local track row. + + ``api_tracks`` may be either a raw list of API dicts (will be + normalized internally) OR a list of pre-normalized 3-tuples from + :func:`_prenormalize_api_tracks`. The orchestrator uses the + pre-normalized form to avoid O(n*m) normalization calls; tests + use the raw list for convenience. + + Local rows carry (title, track_number) but NOT disc_number. + Multi-disc albums repeat track_numbers across discs, so a + track_number-only join would collapse the mapping. Title is the + natural disambiguator (each disc's track 1 has a different title), + but local titles drift from API titles in predictable ways: + trailing ``- Bonus Track`` annotations, ``- Remix`` vs ``(Remix)``, + etc. + + Implementation: each candidate is scored by :func:`_score_candidate`; + the highest-scoring one above :data:`_MATCH_SCORE_THRESHOLD` wins. + If nothing clears the threshold the source genuinely doesn't have a + plausible match and we return ``None`` — the orchestrator surfaces + that as ``"not in tracklist, left in place"`` rather than silently + mis-routing. + """ + norm_local = _normalize_title(db_title) + if not norm_local: + return None + try: + tn = int(db_track_number) if db_track_number is not None else None + except (TypeError, ValueError): + tn = None + local_diffs = _differentiators_in(norm_local) + + # Accept either pre-normalized candidates or raw API dicts. + if api_tracks and isinstance(api_tracks[0], tuple): + candidates = api_tracks # type: ignore[assignment] + else: + candidates = _prenormalize_api_tracks(api_tracks) # type: ignore[arg-type] + + best_item: Optional[dict] = None + best_score = 0 + best_tn_match = False + + for item, api_norm, api_tn in candidates: + score = _score_candidate(norm_local, tn, local_diffs, api_norm, api_tn) + if score < _MATCH_SCORE_THRESHOLD: + continue + tn_match = tn is not None and api_tn == tn + if score > best_score or (score == best_score and tn_match and not best_tn_match): + best_item = item + best_score = score + best_tn_match = tn_match + + return best_item + + +def load_album_and_tracks(db, album_id): + """Load the album row + all its track rows from the local DB. + + Returns ``(album_dict | None, tracks_list)``. ``album_dict`` is None + when the album doesn't exist; tracks_list is empty when the album + has no tracks. The caller decides what status to surface for each + state. + """ + conn = None + try: + conn = db._get_connection() + cursor = conn.cursor() + cursor.execute( + """ + SELECT al.*, ar.name as artist_name + FROM albums al + JOIN artists ar ON al.artist_id = ar.id + WHERE al.id = ? + """, + (str(album_id),), + ) + album_row = cursor.fetchone() + if not album_row: + return None, [] + album_data = dict(album_row) + + cursor.execute( + """ + SELECT t.*, ar.name as artist_name + FROM tracks t + JOIN artists ar ON t.artist_id = ar.id + WHERE t.album_id = ? + ORDER BY t.track_number + """, + (str(album_id),), + ) + tracks = [dict(r) for r in cursor.fetchall()] + return album_data, tracks + finally: + if conn is not None: + try: + conn.close() + except Exception: + pass + + +def plan_album_reorganize( + album_data: dict, + tracks: List[dict], + primary_source: Optional[str] = None, + strict_source: bool = False, +) -> dict: + """Compute the per-track plan for an album reorganize without doing + any file IO. Both the actual reorganize orchestrator and the preview + endpoint share this so the preview is guaranteed to match what would + happen on apply. + + Returns: + ``{'status': 'planned' | 'no_source_id' | 'no_tracks', + 'source': str | None, + 'api_album': dict | None, + 'total_discs': int, + 'items': [{'track': dict, 'api_track': dict | None, + 'matched': bool, 'reason': str | None}, ...]}`` + + Per-track behavior matches the orchestrator exactly: + - Match by `(normalized_title, track_number)`, then title alone, then + track_number alone. + - Tracks with no match are reported with `matched=False` and a reason. + - `disc_number` for each track comes from its matched API entry; if + unmatched, `api_track is None` and the caller decides what to do. + """ + if not tracks: + return { + 'status': 'no_tracks', 'source': None, 'api_album': None, + 'total_discs': 1, 'items': [], + } + + if primary_source is None: + try: + primary_source = get_primary_source() + except Exception: + primary_source = 'deezer' + + source, api_album, api_tracks = _resolve_source( + album_data, primary_source, strict_source=strict_source + ) + if not source: + reason = ( + f"Source '{primary_source}' has no usable tracklist for this album" + if strict_source else + "No metadata source ID for this album" + ) + return { + 'status': 'no_source_id', 'source': None, 'api_album': None, + 'total_discs': 1, + 'items': [{ + 'track': t, 'api_track': None, 'matched': False, + 'reason': reason, + } for t in tracks], + } + + total_discs = max( + (int(item.get('disc_number') or 1) for item in api_tracks), + default=1, + ) + + # Pre-normalize once so the matcher doesn't redo the work per track. + prenormalized = _prenormalize_api_tracks(api_tracks) + items = [] + for track in tracks: + api_track = _find_api_track(prenormalized, track.get('title', ''), track.get('track_number')) + if api_track is None: + items.append({ + 'track': track, 'api_track': None, 'matched': False, + 'reason': f"No matching track in {source} tracklist (likely a bonus / non-canonical track)", + }) + else: + items.append({ + 'track': track, 'api_track': api_track, 'matched': True, + 'reason': None, + }) + + return { + 'status': 'planned', + 'source': source, + 'api_album': api_album, + 'total_discs': total_discs, + 'items': items, + } + + +def _build_post_process_context( + api_album: dict, + api_track: dict, + artist_name: str, + album_title: str, + total_discs: int, +) -> dict: + """Build the same shape `import_album_process` builds so post-process + treats this exactly like a fresh download with full Spotify-style + metadata in hand.""" + track_number = int(api_track.get('track_number') or 1) + disc_number = int(api_track.get('disc_number') or 1) + track_artists = api_track.get('artists') or [artist_name] + normalized_artists = [ + ({'name': a} if isinstance(a, str) else a) for a in track_artists + ] + + api_album_id = api_album.get('id') or api_album.get('album_id') or '' + api_album_name = api_album.get('name') or api_album.get('title') or album_title + api_album_release = ( + api_album.get('release_date') + or api_album.get('releaseDate') + or '' + ) + api_album_total_tracks = ( + api_album.get('total_tracks') + or api_album.get('totalTracks') + or 0 + ) + # Spotify shape: {'images': [{'url': ...}, ...]}. + # Deezer shape: {'image_url': '...'}. + api_album_image = api_album.get('image_url') or '' + if not api_album_image: + images = api_album.get('images') + if isinstance(images, list) and images: + first = images[0] + if isinstance(first, dict): + api_album_image = first.get('url') or '' + + track_name = api_track.get('name') or api_track.get('title') or '' + + return { + 'spotify_artist': { + 'name': artist_name, + 'id': '', + 'genres': [], + }, + 'spotify_album': { + 'id': api_album_id, + 'name': api_album_name, + 'release_date': api_album_release, + 'total_tracks': api_album_total_tracks, + 'total_discs': total_discs, + 'image_url': api_album_image, + }, + 'track_info': { + 'name': track_name, + 'id': api_track.get('id', ''), + 'track_number': track_number, + 'disc_number': disc_number, + 'duration_ms': api_track.get('duration_ms', 0), + 'artists': normalized_artists, + 'uri': api_track.get('uri', ''), + }, + 'original_search_result': { + 'title': track_name, + 'artist': artist_name, + 'album': api_album_name, + 'track_number': track_number, + 'disc_number': disc_number, + 'spotify_clean_title': track_name, + 'spotify_clean_album': api_album_name, + 'artists': normalized_artists, + }, + 'is_album_download': True, + 'has_clean_spotify_data': True, + 'has_full_spotify_metadata': True, + } + + +def preview_album_reorganize( + *, + album_id: str, + db, + transfer_dir: str, + resolve_file_path_fn: Callable[[Optional[str]], Optional[str]], + build_final_path_fn: Callable, + primary_source: Optional[str] = None, + strict_source: bool = False, +) -> dict: + """Compute the planned destination paths for a reorganize WITHOUT + moving any files. The preview UI uses this to show users what the + "Apply" run would do. + + Critically: the destination per track comes from + ``build_final_path_fn(context, spotify_artist, None, file_ext)`` — + the same shared helper post-processing uses. So the preview is + guaranteed to match what the orchestrator would actually produce. + + Args: + album_id: Library album ID. + db: Database object exposing ``_get_connection()``. + transfer_dir: Configured transfer directory (for trimming the + display-relative current-path string). + resolve_file_path_fn: Resolves a DB-stored file path to the + actual on-disk path (or ``None`` if missing). + build_final_path_fn: ``_build_final_path_for_track`` from + web_server. Signature is + ``(context, spotify_artist, album_info_or_none, file_ext) -> (path, ok)``. + Injected so this module stays Flask-free. + primary_source: Optional override for the configured primary + source. + + Returns: + ``{ + 'success': bool, + 'status': str, # 'planned' | 'no_album' | 'no_tracks' | 'no_source_id' + 'source': str | None, + 'album': str, + 'artist': str, + 'transfer_dir': str, + 'tracks': [ + {'track_id', 'title', 'track_number', 'current_path', + 'new_path', 'file_exists', 'unchanged', 'collision', + 'matched', 'reason', 'disc_number'}, + ... + ], + }`` + """ + album_data, tracks = load_album_and_tracks(db, album_id) + if album_data is None: + return {'success': False, 'status': 'no_album', 'tracks': []} + + if not tracks: + return { + 'success': False, 'status': 'no_tracks', + 'album': album_data.get('title', ''), + 'artist': album_data.get('artist_name', ''), + 'tracks': [], + } + + plan = plan_album_reorganize( + album_data, tracks, + primary_source=primary_source, strict_source=strict_source, + ) + artist_name = album_data.get('artist_name') or 'Unknown Artist' + album_title = album_data.get('title') or 'Unknown Album' + + common = { + 'album': album_title, + 'artist': artist_name, + 'transfer_dir': transfer_dir, + 'source': plan['source'], + } + + if plan['status'] == 'no_source_id': + return { + 'success': False, 'status': 'no_source_id', + **common, + 'tracks': [{ + 'track_id': t.get('id'), + 'title': t.get('title', ''), + 'track_number': t.get('track_number', 0), + 'current_path': t.get('file_path', ''), + 'new_path': '', + 'file_exists': False, 'unchanged': False, 'collision': False, + 'matched': False, + 'reason': 'No metadata source ID — run enrichment first', + 'disc_number': None, + } for t in tracks], + } + + total_discs = plan['total_discs'] + api_album = plan['api_album'] or {} + preview_tracks = [] + + for plan_item in plan['items']: + track = plan_item['track'] + title = track.get('title', '') + db_path = track.get('file_path') + resolved = resolve_file_path_fn(db_path) if db_path else None + file_ext = os.path.splitext(resolved or db_path or '.flac')[1] or '.flac' + + item = { + 'track_id': track.get('id'), + 'title': title, + 'track_number': track.get('track_number', 0), + 'current_path': _trim_to_transfer(db_path, resolved, transfer_dir), + 'new_path': '', + 'file_exists': resolved is not None, + 'unchanged': False, + 'collision': False, + 'matched': plan_item['matched'], + 'reason': plan_item.get('reason'), + 'disc_number': None, + } + + if not plan_item['matched']: + preview_tracks.append(item) + continue + + api_track = plan_item['api_track'] + item['disc_number'] = int(api_track.get('disc_number') or 1) + # Build the same context the orchestrator builds so the path + # builder produces the same destination it would on apply. + context = _build_post_process_context( + api_album, api_track, artist_name, album_title, total_discs + ) + # `_build_final_path_for_track` switches between ALBUM and SINGLE + # modes based on `album_info.get('is_album')` — must be passed, + # not None, otherwise multi-disc deluxes degrade to single-track + # folders (the exact bug winecountrygames hit). + album_info = _build_album_info(context) + try: + spotify_artist = context['spotify_artist'] + new_full, _ok = build_final_path_fn(context, spotify_artist, album_info, file_ext) + item['new_path'] = ( + os.path.relpath(new_full, transfer_dir) + if transfer_dir and new_full and new_full.startswith(transfer_dir) + else new_full or '' + ) + if resolved and new_full and os.path.normpath(resolved) == os.path.normpath(new_full): + item['unchanged'] = True + except Exception as e: + item['reason'] = f"Couldn't compute destination path: {e}" + + preview_tracks.append(item) + + # Collision detection: multiple matched tracks mapping to the same + # destination would overwrite each other on apply. + seen = {} + for it in preview_tracks: + if not it['matched'] or it['unchanged'] or not it['new_path']: + continue + norm = os.path.normpath(it['new_path']) + if norm in seen: + it['collision'] = True + seen[norm]['collision'] = True + else: + seen[norm] = it + + return { + 'success': True, 'status': 'planned', + **common, + 'tracks': preview_tracks, + } + + +def _trim_to_transfer(db_path, resolved, transfer_dir): + """Compose the user-facing 'current path' string — relative to the + transfer dir if the file lives there, else the raw DB value.""" + if resolved and transfer_dir and resolved.startswith(transfer_dir): + return resolved[len(transfer_dir):].lstrip(os.sep).lstrip('/') + return db_path or 'No file' + + +def _build_album_info(context: dict) -> dict: + """Build the ``album_info`` dict that ``_build_final_path_for_track`` + consumes to enter ALBUM MODE. Without this (passing None) the path + builder falls through to SINGLE MODE and produces per-track folders + named after each track title — the exact bug we're fixing. + + Mirrors the shape the download path produces at write time. + """ + spotify_album = context.get('spotify_album', {}) or {} + track_info = context.get('track_info', {}) or {} + return { + 'is_album': True, + 'album_name': spotify_album.get('name') or 'Unknown Album', + 'clean_track_name': track_info.get('name') or 'Unknown Track', + 'track_number': track_info.get('track_number') or 1, + 'disc_number': track_info.get('disc_number') or 1, + 'album_image_url': spotify_album.get('image_url') or '', + 'spotify_album_id': spotify_album.get('id') or '', + } + + +@dataclass +class _RunContext: + """Bundles all state + injected dependencies a single + ``_process_one_track`` call needs. + + Hoisted out of orchestrator-local closures so the per-track + helpers can be unit-tested directly with a fake ctx, and so a + stack trace into a failing helper is intelligible (closures + captured 16+ values, none of which were visible in tracebacks). + + Thread-safety contract — read this before adding new fields: + + - ``state_lock`` MUST be held when mutating any of the + lock-protected fields below. The provided ``record_error`` + method already takes the lock; direct mutation outside that + method is the only place where future contributors might + forget. Add new mutable shared state with the same discipline. + + Lock-protected fields (mutate only inside ``state_lock``): + + summary dict — counts and errors list + src_dirs_touched set — populated by `_finalize_track` + dst_dirs_touched set — populated by `_finalize_track` + + Read-only after construction (safe to read without locking): + + album_id, api_album, artist_name, album_title, total_discs, + staging_album_dir, resolve_file_path_fn, post_process_fn, + update_track_path_fn, on_progress, stop_check, state_lock + + Side-effecting methods that take the lock internally: + + record_error() — records a per-track failure + emit() — fires on_progress callback (no lock; + assumes caller holds it when also + passing summary fields, which the + record_error and orchestrator-success + paths both do) + """ + album_id: str + api_album: dict + artist_name: str + album_title: str + total_discs: int + staging_album_dir: str + state_lock: threading.Lock # required to mutate lock-protected fields + summary: dict # LOCK-PROTECTED + src_dirs_touched: Set[str] # LOCK-PROTECTED + dst_dirs_touched: Set[str] # LOCK-PROTECTED + resolve_file_path_fn: Callable[[Optional[str]], Optional[str]] + post_process_fn: Callable[[str, dict, str], None] + update_track_path_fn: Optional[Callable[[Any, str], None]] = None + on_progress: Optional[Callable[[dict], None]] = None + stop_check: Optional[Callable[[], bool]] = None + + def emit(self, **updates) -> None: + """Fire the progress callback. Caller is responsible for + holding ``state_lock`` when the updates payload includes + snapshots of lock-protected fields (so the snapshot is + coherent). Currently always called from inside the lock by + ``record_error`` and the orchestrator's success path.""" + if self.on_progress is None: + return + try: + self.on_progress(updates) + except Exception: + pass + + def record_error(self, track_id, title, message, kind: str = 'skipped') -> None: + with self.state_lock: + self.summary['errors'].append({ + 'track_id': track_id, + 'title': title, + 'error': message, + }) + self.summary[kind] += 1 + self.emit(**{ + kind: self.summary[kind], + 'errors': list(self.summary['errors']), + 'processed': ( + self.summary['moved'] + + self.summary['skipped'] + + self.summary['failed'] + ), + }) + + +def _stage_track(ctx: _RunContext, track_id, title, resolved_src) -> Optional[str]: + """Stage a copy of ``resolved_src`` into a per-track UUID + subdirectory under ``ctx.staging_album_dir``. + + Per-track subdirs are required for concurrent safety: post-process + calls ``_cleanup_empty_directories`` after each move, which walks + UP from the source file removing empty dirs. With a shared + ``staging_album_dir`` that walk would race with other workers' + in-flight ``makedirs``/``copy2`` calls — worker A finishing could + nuke the dir between worker B's ``makedirs`` and ``copy2``, + causing intermittent ``[WinError 3]`` / ``ENOENT`` failures. + + With per-track subdirs: + + - Worker A's cleanup walks: per-track subdir (empty after move → + removed) → ``staging_album_dir`` (still has other workers' + subdirs → not empty → walk stops). ✓ + - Worker B's stage-in: makedirs its OWN subdir, copies into + it. No interference from worker A. ✓ + """ + worker_dir = os.path.join(ctx.staging_album_dir, uuid.uuid4().hex[:8]) + try: + os.makedirs(worker_dir, exist_ok=True) + except OSError as mk_err: + ctx.record_error(track_id, title, + f"Couldn't create staging subdirectory: {mk_err}", + kind='failed') + return None + staging_file = os.path.join(worker_dir, os.path.basename(resolved_src)) + try: + shutil.copy2(resolved_src, staging_file) + except OSError as copy_err: + ctx.record_error(track_id, title, + f"Couldn't copy to staging: {copy_err}", + kind='failed') + return None + return staging_file + + +def _run_post_process_for_track(ctx: _RunContext, track_id, title, api_track, staging_file) -> Optional[str]: + """Build the per-track context, hand it to post-processing, and + return the final on-disk path it produced. Returns None on any + failure (exception, AcoustID rejection, internal skip); the caller + leaves the original file alone.""" + context = _build_post_process_context( + ctx.api_album, api_track, ctx.artist_name, ctx.album_title, ctx.total_discs + ) + context_key = f"reorganize_{ctx.album_id}_{track_id}_{uuid.uuid4().hex[:8]}" + try: + ctx.post_process_fn(context_key, context, staging_file) + except Exception as pp_err: + ctx.record_error(track_id, title, + f"Post-processing failed: {pp_err}", + kind='failed') + return None + new_path = context.get('_final_processed_path') + if not new_path or not os.path.exists(new_path): + ctx.record_error(track_id, title, + 'Post-processing did not produce a final file ' + '(AcoustID rejection, quarantine, or skip).', + kind='failed') + return None + return new_path + + +def _finalize_track(ctx: _RunContext, track_id, resolved_src, new_path) -> None: + """Update the DB row, then remove the original (in that order — DB + failure leaves the file at both locations, recoverable by library + scan; the reverse would orphan the row). Records src/dst dirs for + end-of-run cleanup, deletes per-track sidecars.""" + db_updated = True + if ctx.update_track_path_fn: + try: + ctx.update_track_path_fn(track_id, new_path) + except Exception as db_err: + db_updated = False + logger.warning( + f"[Reorganize] DB path update failed for {track_id}: {db_err} " + f"— leaving original at {resolved_src} so the library scan can recover." + ) + if not db_updated: + return + if os.path.normpath(resolved_src) == os.path.normpath(new_path): + return # in-place edit; nothing more to do + with ctx.state_lock: + ctx.src_dirs_touched.add(os.path.dirname(resolved_src)) + ctx.dst_dirs_touched.add(os.path.dirname(new_path)) + try: + os.remove(resolved_src) + except OSError as rm_err: + logger.warning(f"[Reorganize] Couldn't remove original {resolved_src}: {rm_err}") + _delete_track_sidecars(resolved_src) + + +def _process_one_track(ctx: _RunContext, plan_item: dict) -> None: + """Process a single plan item end-to-end. Safe to call concurrently + from multiple workers — all shared-state mutations go through + ``ctx.state_lock`` (via ``record_error`` and ``_finalize_track``).""" + if ctx.stop_check and ctx.stop_check(): + return + track = plan_item['track'] + title = track.get('title', 'Unknown') + track_id = track.get('id') + ctx.emit(current_track=title) + + if not plan_item['matched']: + ctx.record_error(track_id, title, + plan_item.get('reason') or 'No matching API track') + return + + db_path = track.get('file_path') + resolved_src = ctx.resolve_file_path_fn(db_path) if db_path else None + if not resolved_src: + ctx.record_error(track_id, title, + f"File not found on disk — DB path: {db_path or '(empty)'}") + return + + staging_file = _stage_track(ctx, track_id, title, resolved_src) + if staging_file is None: + return + + new_path = _run_post_process_for_track(ctx, track_id, title, plan_item['api_track'], staging_file) + if new_path is None: + return + + _finalize_track(ctx, track_id, resolved_src, new_path) + + with ctx.state_lock: + ctx.summary['moved'] += 1 + ctx.emit( + moved=ctx.summary['moved'], + processed=ctx.summary['moved'] + ctx.summary['skipped'] + ctx.summary['failed'], + ) + + +def reorganize_album( + *, + album_id: str, + db, + staging_root: str, + resolve_file_path_fn: Callable[[Optional[str]], Optional[str]], + post_process_fn: Callable[[str, dict, str], None], + update_track_path_fn: Optional[Callable[[object, str], None]] = None, + cleanup_empty_dir_fn: Optional[Callable[[str], None]] = None, + transfer_dir: Optional[str] = None, + on_progress: Optional[Callable[[dict], None]] = None, + primary_source: Optional[str] = None, + strict_source: bool = False, + stop_check: Optional[Callable[[], bool]] = None, +) -> dict: + """Run a single album through the post-processing pipeline. + + See module docstring for the rationale. Dependencies (file + resolution, post-processing, DB-path update, empty-dir cleanup) + are injected so the orchestrator stays in ``core/`` and is unit + testable without spinning up the Flask app. + + Args: + album_id: Library album ID. + db: Database object exposing ``_get_connection()``. + staging_root: Root staging directory under the user's download + path. A per-album subfolder is created beneath it; the + whole subfolder is removed at the end of the run. + resolve_file_path_fn: Resolves a DB-stored file path to the + actual on-disk path (or ``None`` if missing). Injected + because the resolution logic lives in ``web_server``. + post_process_fn: ``_post_process_matched_download``. Must set + ``context['_final_processed_path']`` on success. + update_track_path_fn: Called as + ``update_track_path_fn(track_id, new_path)`` after each + successful post-process to update the DB row. ``None`` to + skip (e.g. in tests). + cleanup_empty_dir_fn: Called with each source directory we + emptied so the caller can prune empty parents. ``None`` to + skip. + on_progress: Optional callback for live status updates. + Receives a dict with any subset of the standard reorganize + state keys (``current_track``, ``processed``, ``moved``, + ``skipped``, ``failed``, ``errors``). + primary_source: Override for the configured primary source. + Defaults to ``get_primary_source()``. + stop_check: Returns True when the caller wants the reorganize + to abort early (e.g. server shutdown). + + Returns: + Status summary dict with ``status`` ∈ ``{'completed', + 'no_album', 'no_tracks', 'no_source_id'}`` plus per-track + counters. + """ + summary = { + 'status': 'completed', + 'source': None, + 'total': 0, + 'moved': 0, + 'skipped': 0, + 'failed': 0, + 'errors': [], + } + + state_lock = threading.Lock() + + def _emit(**updates): + if on_progress is None: + return + try: + on_progress(updates) + except Exception: + pass + + # Load album + tracks + album_data, tracks = load_album_and_tracks(db, album_id) + if album_data is None: + summary['status'] = 'no_album' + return summary + + if not tracks: + summary['status'] = 'no_tracks' + return summary + + summary['total'] = len(tracks) + _emit(total=len(tracks)) + + # Build the per-track plan (same logic the preview uses). + plan = plan_album_reorganize( + album_data, tracks, + primary_source=primary_source, strict_source=strict_source, + ) + if plan['status'] == 'no_source_id': + summary['status'] = 'no_source_id' + summary['errors'].append({ + 'error': ( + f"No reachable metadata source ID for '{album_data.get('title', '?')}' — " + "run enrichment first to populate at least one of " + "spotify_album_id / itunes_album_id / deezer_id / discogs_id / soul_id." + ), + }) + return summary + + source = plan['source'] + api_album = plan['api_album'] + total_discs = plan['total_discs'] + summary['source'] = source + logger.info( + f"[Reorganize] Album '{album_data.get('title')}' resolved via {source}: " + f"{len(plan['items'])} item(s) planned" + ) + + # Per-album staging dir under the configured download path. Cleaned + # up (best-effort) at the end of the run regardless of outcome. + artist_name = album_data.get('artist_name') or 'Unknown Artist' + album_title = album_data.get('title') or 'Unknown Album' + staging_album_dir = os.path.join( + staging_root, + f"{_safe_filename(artist_name)} - {_safe_filename(album_title)}_{uuid.uuid4().hex[:8]}", + ) + try: + os.makedirs(staging_album_dir, exist_ok=True) + except OSError as e: + summary['status'] = 'setup_failed' + summary['errors'].append({ + 'error': f"Couldn't create staging directory '{staging_album_dir}': {e}", + }) + return summary + + src_dirs_touched: Set[str] = set() + dst_dirs_touched: Set[str] = set() + + ctx = _RunContext( + album_id=str(album_id), + api_album=api_album or {}, + artist_name=artist_name, + album_title=album_title, + total_discs=total_discs, + staging_album_dir=staging_album_dir, + state_lock=state_lock, + summary=summary, + src_dirs_touched=src_dirs_touched, + dst_dirs_touched=dst_dirs_touched, + resolve_file_path_fn=resolve_file_path_fn, + post_process_fn=post_process_fn, + update_track_path_fn=update_track_path_fn, + on_progress=on_progress, + stop_check=stop_check, + ) + + try: + # 3 concurrent workers per album — matches the download-side + # batch worker count. Post-process has its own per-context-key + # lock so concurrent calls don't race on the same file, and + # all shared-state mutations here are inside `state_lock`. + # + # Wait loop with a periodic watchdog: instead of blocking + # indefinitely on `as_completed`, we wake every + # `_WATCHDOG_INTERVAL_SECONDS` so we can react to stop_check + # promptly AND log a warning if any track has been processing + # for longer than `_HUNG_WORKER_THRESHOLD_SECONDS`. We can't + # kill the thread (Python doesn't allow that cleanly), but + # surfacing it lets operators investigate. + with ThreadPoolExecutor( + max_workers=_REORGANIZE_MAX_WORKERS, + thread_name_prefix='Reorganize', + ) as executor: + future_to_item = { + executor.submit(_process_one_track, ctx, item): item + for item in plan['items'] + } + future_started_at = {f: time.monotonic() for f in future_to_item} + pending = set(future_to_item.keys()) + warned_about: Set[Any] = set() + + while pending: + if stop_check and stop_check(): + for f in pending: + f.cancel() + break + + done, pending = wait( + pending, + timeout=_WATCHDOG_INTERVAL_SECONDS, + return_when=FIRST_COMPLETED, + ) + for finished in done: + try: + finished.result() + except Exception as worker_err: + logger.error( + f"[Reorganize] Worker raised: {worker_err}", + exc_info=True, + ) + + # Watchdog pass — log once per stuck future. + now = time.monotonic() + for f in pending: + if f in warned_about: + continue + elapsed = now - future_started_at[f] + if elapsed >= _HUNG_WORKER_THRESHOLD_SECONDS: + item = future_to_item.get(f, {}) + track_title = (item.get('track') or {}).get('title', 'Unknown') + logger.warning( + f"[Reorganize] Worker stuck for {elapsed:.0f}s on track " + f"'{track_title}' — leaving it running, other workers continuing." + ) + warned_about.add(f) + + finally: + # Best-effort cleanup of the staging dir. + try: + if os.path.isdir(staging_album_dir): + shutil.rmtree(staging_album_dir, ignore_errors=True) + except Exception: + pass + + # Best-effort cleanup of source directories. For each touched dir + # that has no audio files left (i.e. every track in this dir was + # successfully moved), delete album-level sidecars (cover.jpg, + # folder.jpg, etc.) so the dir is empty enough for the empty-dir + # pruner to take it. If audio remains (a track failed to move), + # leave everything alone so the user can see what's still there. + for src_dir in src_dirs_touched: + try: + if _has_remaining_audio(src_dir): + continue + _delete_album_sidecars(src_dir) + except Exception: + pass + + if cleanup_empty_dir_fn: + for src_dir in src_dirs_touched: + try: + cleanup_empty_dir_fn(src_dir) + except Exception: + pass + + # Prune empty *destination* siblings — e.g. when a previous + # failed reorganize attempt left ``Artist/Album-Sibling/`` dirs + # behind that we never end up using, OR when a current-run + # post-process created a destination dir then failed AcoustID + # before landing the file. Walk up from any successful + # destination to the artist folder, then prune one level of + # empty children. Bounded depth = safer than recursive sweep. + if transfer_dir and dst_dirs_touched: + artist_dirs = set() + for dst in dst_dirs_touched: + artist = _find_artist_dir(dst, transfer_dir) + if artist: + artist_dirs.add(artist) + for artist_dir in artist_dirs: + _prune_empty_album_dirs(artist_dir) + + return summary + + +def _find_artist_dir(dest_path: str, transfer_dir: str) -> Optional[str]: + """Walk up from ``dest_path`` until the parent equals ``transfer_dir``; + the directory at that point is the artist folder. Returns None if + ``dest_path`` isn't inside ``transfer_dir`` at all.""" + if not transfer_dir: + return None + transfer_norm = os.path.normpath(transfer_dir) + cur = os.path.normpath(dest_path) + while True: + parent = os.path.dirname(cur) + if parent == cur: + return None # filesystem root + if os.path.normpath(parent) == transfer_norm: + return cur + cur = parent + + +def _prune_empty_album_dirs(artist_dir: str) -> None: + """Remove direct subdirectories of ``artist_dir`` that are empty. + Single-level prune: deliberately doesn't recurse — we want to + catch leftover album-sibling folders without aggressively touching + the user's nested directory tree. + + Also walks one level deeper into each album dir to remove empty + Disc-N subfolders that previous runs may have created.""" + if not os.path.isdir(artist_dir): + return + try: + children = list(os.listdir(artist_dir)) + except OSError: + return + for entry in children: + album_path = os.path.join(artist_dir, entry) + if not os.path.isdir(album_path): + continue + # First pass: prune empty Disc-N subfolders inside this album. + try: + for sub in list(os.listdir(album_path)): + disc_path = os.path.join(album_path, sub) + if os.path.isdir(disc_path): + try: + if not os.listdir(disc_path): + os.rmdir(disc_path) + except OSError: + pass + except OSError: + pass + # Then: if the whole album dir is now empty, prune it. + try: + if not os.listdir(album_path): + os.rmdir(album_path) + logger.info(f"[Reorganize] Pruned empty album dir: {album_path}") + except OSError: + pass + + +# Sidecar / cleanup helpers -------------------------------------------------- + +# 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', +) + +# 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 +# behind that we shouldn't strip the cover art from). +_AUDIO_EXTS = frozenset( + {'.flac', '.mp3', '.m4a', '.ogg', '.opus', '.wav', '.aac', '.wma', '.mp4'} +) + + +def _delete_track_sidecars(audio_path: str) -> None: + """Delete per-track sidecars (.lrc / .nfo / .txt / .cue / .json) that + sit alongside `audio_path` and share its filename stem. Best-effort — + individual failures are logged at debug and never raised.""" + src_dir = os.path.dirname(audio_path) + stem = os.path.splitext(os.path.basename(audio_path))[0] + for ext in _TRACK_SIDECAR_EXTS: + sidecar = os.path.join(src_dir, stem + ext) + if os.path.isfile(sidecar): + try: + os.remove(sidecar) + except OSError as e: + logger.debug(f"[Reorganize] Couldn't remove sidecar {sidecar}: {e}") + + +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): + try: + os.remove(sidecar) + except OSError as e: + logger.debug(f"[Reorganize] Couldn't remove album sidecar {sidecar}: {e}") + + +def _has_remaining_audio(directory: str) -> bool: + """Return True if `directory` contains any audio files. Used as the + safety check before stripping album-level sidecars: if a track + failed to move, leave its cover art and friends in place.""" + if not os.path.isdir(directory): + return False + try: + for name in os.listdir(directory): + full = os.path.join(directory, name) + if not os.path.isfile(full): + continue + if os.path.splitext(name)[1].lower() in _AUDIO_EXTS: + return True + except OSError: + return True # Safer to assume "yes, leave it" if we can't check + return False diff --git a/tests/test_library_reorganize_orchestrator.py b/tests/test_library_reorganize_orchestrator.py new file mode 100644 index 00000000..846e6ec9 --- /dev/null +++ b/tests/test_library_reorganize_orchestrator.py @@ -0,0 +1,1930 @@ +"""Tests for `core.library_reorganize.reorganize_album` — the new +post-processing-pipeline approach (the orchestrator that copies files +to staging and routes them through the same code that handles fresh +downloads, instead of doing per-album template work in web_server). + +Contract this test file pins: + +1. Albums without ANY metadata-source ID return ``status='no_source_id'`` + without staging anything, copying anything, or calling post-process. + Silent degradation to file tags is the failure mode the previous + implementation had; the new contract is "we have the source of + truth or we don't touch the album." +2. Source resolution honors the configured primary first, then walks + ``get_source_priority`` until something returns a tracklist. +3. Each library track is matched to the API tracklist by + ``track_number``. Tracks not in the API response (bonus tracks on a + deluxe edition, etc.) are reported as skipped and left in place — + they are NOT force-fed wrong context to post-process. +4. Files that don't resolve on disk are surfaced as skipped errors + with the offending DB path, not silently dropped. +5. After a successful post-process the original file is removed and + the DB row is updated to the new path. A failed post-process leaves + the original alone so the user doesn't lose data. +6. Staging directory is cleaned up regardless of how the run ends. +""" + +import os +import shutil +import sqlite3 +import sys +import types + +import pytest + + +# --- module stubs (same shape used elsewhere in the test suite) ----------- +if "spotipy" not in sys.modules: + spotipy = types.ModuleType("spotipy") + + class _DummySpotify: + def __init__(self, *args, **kwargs): + pass + + oauth2 = types.ModuleType("spotipy.oauth2") + + class _DummyOAuth: + def __init__(self, *args, **kwargs): + pass + + spotipy.Spotify = _DummySpotify + oauth2.SpotifyOAuth = _DummyOAuth + oauth2.SpotifyClientCredentials = _DummyOAuth + spotipy.oauth2 = oauth2 + sys.modules["spotipy"] = spotipy + sys.modules["spotipy.oauth2"] = oauth2 + +if "config.settings" not in sys.modules: + config_pkg = types.ModuleType("config") + settings_mod = types.ModuleType("config.settings") + + class _DummyConfigManager: + def get(self, key, default=None): + return default + + def get_active_media_server(self): + return "primary" + + settings_mod.config_manager = _DummyConfigManager() + config_pkg.settings = settings_mod + sys.modules["config"] = config_pkg + sys.modules["config.settings"] = settings_mod + + +from core import library_reorganize # noqa: E402 + + +# --- helpers -------------------------------------------------------------- + +class _FakeDB: + """Wraps a sqlite3 in-memory connection that survives `close()` calls + so the tests can reuse it for assertions after the orchestrator runs.""" + + def __init__(self): + self._conn = sqlite3.connect(":memory:") + self._conn.row_factory = sqlite3.Row + + def _get_connection(self): + return _NonClosingConnWrapper(self._conn) + + +class _NonClosingConnWrapper: + def __init__(self, real): + self._real = real + + def cursor(self): + return self._real.cursor() + + def execute(self, *args, **kwargs): + return self._real.execute(*args, **kwargs) + + def commit(self): + return self._real.commit() + + def close(self): + # Underlying connection survives — tests reuse it. + pass + + def __enter__(self): + return self + + def __exit__(self, *args): + pass + + +def _setup_album(db, *, album_id='alb-1', spotify_id='', deezer_id='', + itunes_id='', discogs_id='', soul_id='', tracks=()): + """Build a minimal artists/albums/tracks schema and seed one album. + + `tracks` is a list of `(track_id, track_number, title, file_path)`. + """ + cur = db._conn.cursor() + cur.execute("CREATE TABLE artists (id TEXT PRIMARY KEY, name TEXT)") + cur.execute(""" + CREATE TABLE albums ( + id TEXT PRIMARY KEY, + artist_id TEXT, + title TEXT, + spotify_album_id TEXT, + deezer_id TEXT, + itunes_album_id TEXT, + discogs_id TEXT, + soul_id TEXT + ) + """) + cur.execute(""" + CREATE TABLE tracks ( + id TEXT PRIMARY KEY, + album_id TEXT, + artist_id TEXT, + title TEXT, + track_number INTEGER, + file_path TEXT, + updated_at TEXT + ) + """) + cur.execute("INSERT INTO artists VALUES (?, ?)", ('artist-1', 'Aerosmith')) + cur.execute( + "INSERT INTO albums (id, artist_id, title, spotify_album_id, deezer_id, " + "itunes_album_id, discogs_id, soul_id) VALUES (?,?,?,?,?,?,?,?)", + (album_id, 'artist-1', 'Aerosmith (1973)', spotify_id, deezer_id, + itunes_id, discogs_id, soul_id), + ) + for tid, tn, title, fp in tracks: + cur.execute( + "INSERT INTO tracks (id, album_id, artist_id, title, track_number, file_path) " + "VALUES (?,?,?,?,?,?)", + (tid, album_id, 'artist-1', title, tn, fp), + ) + db._conn.commit() + + +@pytest.fixture +def tmpdirs(tmp_path): + """Three working directories: original library files, staging root, + transfer destination.""" + library = tmp_path / "library" + staging = tmp_path / "staging" + transfer = tmp_path / "transfer" + library.mkdir() + staging.mkdir() + transfer.mkdir() + return library, staging, transfer + + +def _make_audio_file(library_dir, name='song.flac', content=b'fakeflacdata'): + p = library_dir / name + p.write_bytes(content) + return str(p) + + +# --- tests: source resolution --------------------------------------------- + +def test_returns_no_source_id_when_album_has_none(monkeypatch, tmpdirs): + library, staging, _transfer = tmpdirs + db = _FakeDB() + _setup_album(db, tracks=[ + ('t1', 1, 'Same Old Song And Dance', _make_audio_file(library)), + ]) + + pp_calls = [] + + def pp(key, ctx, fp): + pp_calls.append(key) + + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'spotify') + monkeypatch.setattr(library_reorganize, 'get_source_priority', + lambda p: [p, 'deezer', 'itunes', 'discogs', 'hydrabase']) + monkeypatch.setattr(library_reorganize, 'get_album_for_source', lambda *a: None) + monkeypatch.setattr(library_reorganize, 'get_album_tracks_for_source', lambda *a: None) + + summary = library_reorganize.reorganize_album( + album_id='alb-1', db=db, staging_root=str(staging), + resolve_file_path_fn=lambda p: p, post_process_fn=pp, + ) + + assert summary['status'] == 'no_source_id' + assert summary['moved'] == 0 + assert pp_calls == [] + + +def test_falls_through_to_next_source_when_primary_returns_nothing(monkeypatch, tmpdirs): + library, staging, _transfer = tmpdirs + db = _FakeDB() + _setup_album(db, spotify_id='sp-1', deezer_id='dz-1', tracks=[ + ('t1', 1, 'Same Old Song And Dance', _make_audio_file(library)), + ]) + + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'spotify') + monkeypatch.setattr(library_reorganize, 'get_source_priority', + lambda p: [p, 'deezer']) + + def fake_album(src, sid): + return {'id': sid, 'name': 'Aerosmith', 'release_date': '1973-01-01'} \ + if src == 'deezer' else None + + def fake_tracks(src, sid): + return {'items': [{'id': 'dz-t1', 'name': 'Same Old Song And Dance', + 'track_number': 1, 'disc_number': 1}]} \ + if src == 'deezer' else None + + monkeypatch.setattr(library_reorganize, 'get_album_for_source', fake_album) + monkeypatch.setattr(library_reorganize, 'get_album_tracks_for_source', fake_tracks) + + def pp(key, ctx, fp): + ctx['_final_processed_path'] = str(library / 'final.flac') + with open(ctx['_final_processed_path'], 'wb') as f: + f.write(b'final') + + summary = library_reorganize.reorganize_album( + album_id='alb-1', db=db, staging_root=str(staging), + resolve_file_path_fn=lambda p: p, post_process_fn=pp, + ) + + assert summary['source'] == 'deezer' + assert summary['moved'] == 1 + + +# --- tests: per-track behavior -------------------------------------------- + +def test_multi_disc_album_disambiguates_by_title(monkeypatch, tmpdirs): + """The whole point of moving from track_number-only to title-based + matching: a 2-disc album has track_number=1 on BOTH discs, but the + titles differ. Each library track must end up routed to the API + entry with the matching title — and therefore to the correct + disc_number in the post-process context.""" + library, staging, _transfer = tmpdirs + db = _FakeDB() + _setup_album(db, deezer_id='dz-1', tracks=[ + # Disc 1 track 1: 'Same Old Song And Dance' + ('t1d1', 1, 'Same Old Song And Dance', _make_audio_file(library, 'd1t1.flac')), + # Disc 2 track 1: 'Dream On' + ('t1d2', 1, 'Dream On', _make_audio_file(library, 'd2t1.flac')), + ]) + + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'deezer') + monkeypatch.setattr(library_reorganize, 'get_source_priority', lambda p: [p]) + monkeypatch.setattr(library_reorganize, 'get_album_for_source', + lambda *a: {'id': 'dz-1', 'name': 'Aerosmith'}) + monkeypatch.setattr( + library_reorganize, 'get_album_tracks_for_source', + lambda *a: {'items': [ + {'id': 'd1t1', 'name': 'Same Old Song And Dance', 'track_number': 1, 'disc_number': 1}, + {'id': 'd2t1', 'name': 'Dream On', 'track_number': 1, 'disc_number': 2}, + ]}, + ) + + title_to_disc = {} + + def pp(key, ctx, fp): + # Capture which disc_number landed in the per-track context + title_to_disc[ctx['track_info']['name']] = ctx['track_info']['disc_number'] + # Also record total_discs so we can assert it's correct + title_to_disc.setdefault('_total_discs', ctx['spotify_album']['total_discs']) + ctx['_final_processed_path'] = str(library / f"out_{ctx['track_info']['disc_number']}_{ctx['track_info']['track_number']}.flac") + with open(ctx['_final_processed_path'], 'wb') as f: + f.write(b'final') + + summary = library_reorganize.reorganize_album( + album_id='alb-1', db=db, staging_root=str(staging), + resolve_file_path_fn=lambda p: p, post_process_fn=pp, + ) + + assert summary['moved'] == 2 + # The crucial assertion: each track must get the disc_number of + # its title-matched API entry, NOT a collapsed last-write-wins value. + assert title_to_disc['Same Old Song And Dance'] == 1 + assert title_to_disc['Dream On'] == 2 + # And the album-level total_discs must be 2 so post-process inserts the subfolder + assert title_to_disc['_total_discs'] == 2 + + +def test_title_match_tolerates_smart_quotes_and_punctuation(monkeypatch, tmpdirs): + library, staging, _transfer = tmpdirs + db = _FakeDB() + _setup_album(db, deezer_id='dz-1', tracks=[ + ('t1', 1, "Don't Stop Believin'", _make_audio_file(library, 't1.flac')), + ]) + + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'deezer') + monkeypatch.setattr(library_reorganize, 'get_source_priority', lambda p: [p]) + monkeypatch.setattr(library_reorganize, 'get_album_for_source', + lambda *a: {'id': 'dz-1', 'name': 'A'}) + monkeypatch.setattr( + library_reorganize, 'get_album_tracks_for_source', + # API uses smart quotes — historically a common mismatch source + lambda *a: {'items': [ + {'id': 'a1', 'name': 'Don’t Stop Believin’', 'track_number': 1, 'disc_number': 1}, + ]}, + ) + + pp_calls = [] + + def pp(key, ctx, fp): + pp_calls.append(ctx['track_info']['name']) + ctx['_final_processed_path'] = str(library / 'out.flac') + with open(ctx['_final_processed_path'], 'wb') as f: + f.write(b'final') + + summary = library_reorganize.reorganize_album( + album_id='alb-1', db=db, staging_root=str(staging), + resolve_file_path_fn=lambda p: p, post_process_fn=pp, + ) + + assert summary['moved'] == 1 + assert len(pp_calls) == 1 + + +def test_bonus_track_routes_to_correct_disc_via_substring_match(monkeypatch, tmpdirs): + """Real-world scenario from winecountrygames's Kendrick Lamar deluxe: + user has ``The Recipe - Bonus Track`` (track 1, disc 2 in his library) + AND ``Sherane`` (track 1, disc 1). The API returns the bonus track as + plain ``The Recipe`` (no suffix). Without substring matching, the + bonus track falls through to track-number-only and lands on disc 1. + With substring matching (gated on track_number), it correctly routes + to disc 2.""" + library, _staging, _transfer = tmpdirs + db = _FakeDB() + _setup_album(db, deezer_id='dz-1', tracks=[ + # Disc 1, track 1 + ('t1d1', 1, 'Sherane', _make_audio_file(library, 'd1t1.flac')), + # Disc 2, track 1 — local title has " - Bonus Track" suffix + ('t1d2', 1, 'The Recipe - Bonus Track', _make_audio_file(library, 'd2t1.flac')), + ]) + + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'deezer') + monkeypatch.setattr(library_reorganize, 'get_source_priority', lambda p: [p]) + monkeypatch.setattr(library_reorganize, 'get_album_for_source', + lambda *a: {'id': 'dz-1', 'name': 'good kid m.A.A.d city (Deluxe)'}) + monkeypatch.setattr( + library_reorganize, 'get_album_tracks_for_source', + lambda *a: {'items': [ + {'id': 'a1', 'name': 'Sherane', 'track_number': 1, 'disc_number': 1}, + {'id': 'a2', 'name': 'The Recipe', 'track_number': 1, 'disc_number': 2}, + ]}, + ) + + title_to_disc = {} + + def pp(key, ctx, fp): + title_to_disc[ctx['track_info']['name']] = ctx['track_info']['disc_number'] + ctx['_final_processed_path'] = str(library / f"out_{ctx['track_info']['disc_number']}.flac") + with open(ctx['_final_processed_path'], 'wb') as f: + f.write(b'final') + + library_reorganize.reorganize_album( + album_id='alb-1', db=db, staging_root=str(_staging), + resolve_file_path_fn=lambda p: p, post_process_fn=pp, + ) + + # The local "The Recipe - Bonus Track" must route to the API's + # disc-2 entry (which is named just "The Recipe"), via substring + # match + track_number tiebreaker. + assert title_to_disc['Sherane'] == 1 + assert title_to_disc['The Recipe'] == 2 + + +def test_dash_vs_parens_normalize_equally_for_remix_versions(monkeypatch, tmpdirs): + """Local file has ``Bitch, Don't Kill My Vibe - Remix`` (dash style), + API has the same track as ``Bitch, Don't Kill My Vibe (Remix)`` + (parens style). Both must normalize to the same string so tier 1 + matches without falling to substring or track_number fallbacks.""" + library, staging, _transfer = tmpdirs + db = _FakeDB() + _setup_album(db, deezer_id='dz-1', tracks=[ + ('t1', 5, "Bitch, Don't Kill My Vibe - Remix", _make_audio_file(library, 't1.flac')), + ]) + + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'deezer') + monkeypatch.setattr(library_reorganize, 'get_source_priority', lambda p: [p]) + monkeypatch.setattr(library_reorganize, 'get_album_for_source', + lambda *a: {'id': 'dz-1', 'name': 'A'}) + monkeypatch.setattr( + library_reorganize, 'get_album_tracks_for_source', + lambda *a: {'items': [ + {'id': 'a1', 'name': "Bitch, Don't Kill My Vibe (Remix)", + 'track_number': 5, 'disc_number': 2}, + ]}, + ) + + matched = [] + + def pp(key, ctx, fp): + matched.append((ctx['track_info']['name'], ctx['track_info']['disc_number'])) + ctx['_final_processed_path'] = str(library / 'out.flac') + with open(ctx['_final_processed_path'], 'wb') as f: + f.write(b'final') + + library_reorganize.reorganize_album( + album_id='alb-1', db=db, staging_root=str(staging), + resolve_file_path_fn=lambda p: p, post_process_fn=pp, + ) + + assert matched == [("Bitch, Don't Kill My Vibe (Remix)", 2)] + + +def test_substring_match_handles_track_number_disagreement(monkeypatch, tmpdirs): + """Real-world Kendrick Lamar deluxe case: the user's library has + ``The Recipe (Black Hippy Remix) - Bonus Track`` numbered as track + 4 of disc 2, but Deezer has the same track at disc 2 track 5 (and + has ``Bitch... (Remix)`` at disc 2 track 4). Track_number-gated + containment misses; length-ratio containment must pick the right + one without false-positive risk.""" + library, staging, _transfer = tmpdirs + db = _FakeDB() + _setup_album(db, deezer_id='dz-1', tracks=[ + ('t4', 4, 'The Recipe (Black Hippy Remix) - Bonus Track', + _make_audio_file(library, 't4.flac')), + ]) + + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'deezer') + monkeypatch.setattr(library_reorganize, 'get_source_priority', lambda p: [p]) + monkeypatch.setattr(library_reorganize, 'get_album_for_source', + lambda *a: {'id': 'dz-1', 'name': 'A'}) + monkeypatch.setattr( + library_reorganize, 'get_album_tracks_for_source', + lambda *a: {'items': [ + # API has the bonus tracks in a different order than the user + {'id': 'd1t4', 'name': 'The Art of Peer Pressure', + 'track_number': 4, 'disc_number': 1}, + {'id': 'd2t4', 'name': "Bitch, Don't Kill My Vibe (Remix)", + 'track_number': 4, 'disc_number': 2}, + {'id': 'd2t5', 'name': 'The Recipe (Black Hippy Remix)', + 'track_number': 5, 'disc_number': 2}, + ]}, + ) + + matched = [] + + def pp(key, ctx, fp): + matched.append((ctx['track_info']['name'], ctx['track_info']['disc_number'])) + ctx['_final_processed_path'] = str(library / 'out.flac') + with open(ctx['_final_processed_path'], 'wb') as f: + f.write(b'final') + + library_reorganize.reorganize_album( + album_id='alb-1', db=db, staging_root=str(staging), + resolve_file_path_fn=lambda p: p, post_process_fn=pp, + ) + + # The local Black Hippy Remix Bonus Track must end up in disc 2, + # NOT collide with disc 1's "Art of Peer Pressure" via track_number. + assert matched == [('The Recipe (Black Hippy Remix)', 2)] + + +def test_remix_does_not_substring_match_to_original_recording(monkeypatch, tmpdirs): + """winecountrygames's iTunes case: iTunes doesn't have the remix, + just the original ``Bitch Don't Kill My Vibe``. Substring + ratio + alone would merge the local remix bonus track into the original + via tier 4 (ratio 0.78). Reject because they have different version + differentiators ('remix' vs none) — they're different recordings.""" + library, staging, _transfer = tmpdirs + db = _FakeDB() + _setup_album(db, itunes_id='it-1', tracks=[ + # Original — should match cleanly via tier 1 to iTunes' entry + ('t2', 2, "Bitch, Don't Kill My Vibe", _make_audio_file(library, 't2.flac')), + # Remix — iTunes doesn't have it; must report unmatched, NOT + # collide with the original via substring + ('t5', 5, "Bitch, Don't Kill My Vibe - Remix", _make_audio_file(library, 't5.flac')), + ]) + + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'itunes') + monkeypatch.setattr(library_reorganize, 'get_source_priority', lambda p: [p]) + monkeypatch.setattr(library_reorganize, 'get_album_for_source', + lambda *a: {'id': 'it-1', 'name': 'A'}) + monkeypatch.setattr( + library_reorganize, 'get_album_tracks_for_source', + lambda *a: {'items': [ + {'id': 'it2', 'name': "Bitch, Don't Kill My Vibe", + 'track_number': 2, 'disc_number': 1}, + ]}, + ) + + matched_titles = [] + skipped_titles = [] + + def pp(key, ctx, fp): + matched_titles.append(ctx['track_info']['name']) + ctx['_final_processed_path'] = str(library / f'out_{ctx["track_info"]["name"]}.flac') + with open(ctx['_final_processed_path'], 'wb') as f: + f.write(b'final') + + summary = library_reorganize.reorganize_album( + album_id='alb-1', db=db, staging_root=str(staging), + resolve_file_path_fn=lambda p: p, post_process_fn=pp, + ) + + skipped_titles = [e['title'] for e in summary['errors']] + # Only the original should have been processed + assert matched_titles == ["Bitch, Don't Kill My Vibe"] + # The remix should be reported as unmatched, NOT merged with the original + assert "Bitch, Don't Kill My Vibe - Remix" in skipped_titles + assert summary['moved'] == 1 + assert summary['skipped'] == 1 + + +def test_substring_match_does_not_false_positive_across_discs(monkeypatch, tmpdirs): + """Safety: ``Real`` (substring) must not silently map to a longer + track like ``Real Real Real`` on a different disc. Substring match + is gated on matching track_number; if the only API entry whose + title contains the local one has a different track_number, the + matcher must fall through to last-resort track_number-only.""" + library, staging, _transfer = tmpdirs + db = _FakeDB() + _setup_album(db, deezer_id='dz-1', tracks=[ + ('t11', 11, 'Real', _make_audio_file(library, 't11.flac')), + ]) + + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'deezer') + monkeypatch.setattr(library_reorganize, 'get_source_priority', lambda p: [p]) + monkeypatch.setattr(library_reorganize, 'get_album_for_source', + lambda *a: {'id': 'dz-1', 'name': 'A'}) + monkeypatch.setattr( + library_reorganize, 'get_album_tracks_for_source', + lambda *a: {'items': [ + # Real-the-track on disc 1, position 11 — the right answer + {'id': 'a1', 'name': 'Real', 'track_number': 11, 'disc_number': 1}, + # A nearby longer title on disc 2 that contains "real" — must NOT win + {'id': 'a2', 'name': 'Real Real Real', 'track_number': 1, 'disc_number': 2}, + ]}, + ) + + matched = [] + + def pp(key, ctx, fp): + matched.append((ctx['track_info']['name'], ctx['track_info']['disc_number'])) + ctx['_final_processed_path'] = str(library / 'out.flac') + with open(ctx['_final_processed_path'], 'wb') as f: + f.write(b'final') + + library_reorganize.reorganize_album( + album_id='alb-1', db=db, staging_root=str(staging), + resolve_file_path_fn=lambda p: p, post_process_fn=pp, + ) + + # Tier 1 (exact + track_number) wins for the legitimate disc 1 entry + assert matched == [('Real', 1)] + + +def test_skips_track_when_source_tracklist_doesnt_contain_it(monkeypatch, tmpdirs): + """winecountrygames's actual scenario: Deezer's response for the + Kendrick deluxe was missing 'The Recipe (Black Hippy Remix)' — the + user has 17 local tracks, Deezer knows 16. The 17th local track + has no title-based match anywhere in the API tracklist. Per the + design policy 'trust the source', we must NOT fall back to + track_number-only matching (which would falsely route the missing + bonus track to whatever disc-1 entry shares its track_number, + causing a collision with a totally unrelated song).""" + library, staging, _transfer = tmpdirs + db = _FakeDB() + _setup_album(db, deezer_id='dz-1', tracks=[ + # Local tn=4 — but API doesn't have this track at all; the only + # API entry with track_number=4 is "The Art of Peer Pressure" + # (a completely different song). Old tier-5 fallback would have + # silently routed our bonus track to that entry → collision. + ('t4', 4, 'The Recipe (Black Hippy Remix) - Bonus Track', + _make_audio_file(library, 't4.flac')), + ]) + + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'deezer') + monkeypatch.setattr(library_reorganize, 'get_source_priority', lambda p: [p]) + monkeypatch.setattr(library_reorganize, 'get_album_for_source', + lambda *a: {'id': 'dz-1', 'name': 'A'}) + monkeypatch.setattr( + library_reorganize, 'get_album_tracks_for_source', + lambda *a: {'items': [ + # Same tn=4, completely different title — must NOT capture + # the local track via track_number fallback. + {'id': 'd1t4', 'name': 'The Art of Peer Pressure', + 'track_number': 4, 'disc_number': 1}, + ]}, + ) + + pp_calls = [] + + def pp(*a, **k): + pp_calls.append(a) + + summary = library_reorganize.reorganize_album( + album_id='alb-1', db=db, staging_root=str(staging), + resolve_file_path_fn=lambda p: p, post_process_fn=pp, + ) + + # Track must be skipped, NOT routed to The Art of Peer Pressure. + assert summary['moved'] == 0 + assert summary['skipped'] == 1 + assert pp_calls == [] + assert 'not in' in summary['errors'][0]['error'].lower() \ + or 'bonus' in summary['errors'][0]['error'].lower() \ + or 'non-canonical' in summary['errors'][0]['error'].lower() + + +def test_skips_track_not_in_api_tracklist(monkeypatch, tmpdirs): + """Bonus track scenario: user has 12 tracks, source's catalog version + only has 10. Tracks not in the API response must be skipped, NOT + force-fed wrong context to post-process.""" + library, staging, _transfer = tmpdirs + db = _FakeDB() + _setup_album(db, deezer_id='dz-1', tracks=[ + ('t1', 1, 'Track 1', _make_audio_file(library, 't1.flac')), + ('t2', 2, 'Track 2', _make_audio_file(library, 't2.flac')), + ('t11', 11, 'Bonus Track', _make_audio_file(library, 't11.flac')), + ]) + + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'deezer') + monkeypatch.setattr(library_reorganize, 'get_source_priority', lambda p: [p]) + monkeypatch.setattr(library_reorganize, 'get_album_for_source', + lambda *a: {'id': 'dz-1', 'name': 'A'}) + monkeypatch.setattr( + library_reorganize, 'get_album_tracks_for_source', + lambda *a: {'items': [ + {'id': 'a1', 'name': 'Track 1', 'track_number': 1, 'disc_number': 1}, + {'id': 'a2', 'name': 'Track 2', 'track_number': 2, 'disc_number': 1}, + ]}, + ) + + pp_for = [] + + def pp(key, ctx, fp): + pp_for.append(ctx['track_info']['track_number']) + ctx['_final_processed_path'] = str(library / f"out_{ctx['track_info']['track_number']}.flac") + with open(ctx['_final_processed_path'], 'wb') as f: + f.write(b'final') + + summary = library_reorganize.reorganize_album( + album_id='alb-1', db=db, staging_root=str(staging), + resolve_file_path_fn=lambda p: p, post_process_fn=pp, + ) + + assert sorted(pp_for) == [1, 2] + assert summary['moved'] == 2 + assert summary['skipped'] == 1 + assert any('Bonus Track' in e['title'] for e in summary['errors']) + + +def test_surfaces_unresolved_file_path(monkeypatch, tmpdirs): + library, staging, _transfer = tmpdirs + db = _FakeDB() + _setup_album(db, deezer_id='dz-1', tracks=[ + ('t1', 1, 'Track 1', '/nonexistent/file.flac'), + ]) + + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'deezer') + monkeypatch.setattr(library_reorganize, 'get_source_priority', lambda p: [p]) + monkeypatch.setattr(library_reorganize, 'get_album_for_source', + lambda *a: {'id': 'dz-1', 'name': 'A'}) + monkeypatch.setattr( + library_reorganize, 'get_album_tracks_for_source', + lambda *a: {'items': [{'id': 'a1', 'name': 'Track 1', 'track_number': 1}]}, + ) + + pp_calls = [] + + def pp(*a, **k): + pp_calls.append(a) + + summary = library_reorganize.reorganize_album( + album_id='alb-1', db=db, staging_root=str(staging), + resolve_file_path_fn=lambda p: None, # nothing resolves + post_process_fn=pp, + ) + + assert summary['skipped'] == 1 + assert summary['moved'] == 0 + assert pp_calls == [] + assert '/nonexistent/file.flac' in summary['errors'][0]['error'] + + +def test_failed_post_process_leaves_original_in_place(monkeypatch, tmpdirs): + """If post-process fails (AcoustID rejection, exception, anything), + the original file must remain at its location and the DB must NOT + be updated. Worst-case the user retries; we don't lose data.""" + library, staging, _transfer = tmpdirs + db = _FakeDB() + src_file = _make_audio_file(library, 't1.flac') + _setup_album(db, deezer_id='dz-1', tracks=[ + ('t1', 1, 'Track 1', src_file), + ]) + + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'deezer') + monkeypatch.setattr(library_reorganize, 'get_source_priority', lambda p: [p]) + monkeypatch.setattr(library_reorganize, 'get_album_for_source', + lambda *a: {'id': 'dz-1', 'name': 'A'}) + monkeypatch.setattr( + library_reorganize, 'get_album_tracks_for_source', + lambda *a: {'items': [{'id': 'a1', 'name': 'Track 1', 'track_number': 1}]}, + ) + + def pp(key, ctx, fp): + # Simulate AcoustID rejection: don't set _final_processed_path + return + + db_updates = [] + + def update_path(track_id, new_path): + db_updates.append((track_id, new_path)) + + summary = library_reorganize.reorganize_album( + album_id='alb-1', db=db, staging_root=str(staging), + resolve_file_path_fn=lambda p: p, post_process_fn=pp, + update_track_path_fn=update_path, + ) + + assert summary['failed'] == 1 + assert summary['moved'] == 0 + assert os.path.exists(src_file) + assert db_updates == [] + + +def test_post_process_exception_is_caught_and_original_preserved(monkeypatch, tmpdirs): + library, staging, _transfer = tmpdirs + db = _FakeDB() + src_file = _make_audio_file(library, 't1.flac') + _setup_album(db, deezer_id='dz-1', tracks=[ + ('t1', 1, 'Track 1', src_file), + ]) + + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'deezer') + monkeypatch.setattr(library_reorganize, 'get_source_priority', lambda p: [p]) + monkeypatch.setattr(library_reorganize, 'get_album_for_source', + lambda *a: {'id': 'dz-1', 'name': 'A'}) + monkeypatch.setattr( + library_reorganize, 'get_album_tracks_for_source', + lambda *a: {'items': [{'id': 'a1', 'name': 'Track 1', 'track_number': 1}]}, + ) + + def pp(*a, **k): + raise RuntimeError("boom") + + summary = library_reorganize.reorganize_album( + album_id='alb-1', db=db, staging_root=str(staging), + resolve_file_path_fn=lambda p: p, post_process_fn=pp, + ) + + assert summary['failed'] == 1 + assert os.path.exists(src_file) + + +def test_recreates_staging_dir_when_post_process_cleans_it(monkeypatch, tmpdirs): + """Regression test for the "1 moved, 15 failed (path not found)" bug + winecountrygames hit on his first reorganize run. + + Post-processing calls `_cleanup_empty_directories` after each move. + That walks up from the source file removing empties — and since the + only thing in our staging_album_dir is the staged file we just had + post-process consume, the dir is empty after the move and gets + nuked. The next track's `shutil.copy2` then failed with WinError 3 + because the destination directory no longer existed. + + The orchestrator must recreate staging_album_dir before each copy.""" + library, staging, _transfer = tmpdirs + db = _FakeDB() + _setup_album(db, deezer_id='dz-1', tracks=[ + ('t1', 1, 'Track 1', _make_audio_file(library, 't1.flac')), + ('t2', 2, 'Track 2', _make_audio_file(library, 't2.flac')), + ('t3', 3, 'Track 3', _make_audio_file(library, 't3.flac')), + ]) + + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'deezer') + monkeypatch.setattr(library_reorganize, 'get_source_priority', lambda p: [p]) + monkeypatch.setattr(library_reorganize, 'get_album_for_source', + lambda *a: {'id': 'dz-1', 'name': 'A'}) + monkeypatch.setattr( + library_reorganize, 'get_album_tracks_for_source', + lambda *a: {'items': [ + {'id': 'a1', 'name': 'Track 1', 'track_number': 1}, + {'id': 'a2', 'name': 'Track 2', 'track_number': 2}, + {'id': 'a3', 'name': 'Track 3', 'track_number': 3}, + ]}, + ) + + final_dir = library / 'final' + final_dir.mkdir() + pp_count = [0] + + def pp_with_aggressive_cleanup(key, ctx, fp): + """Mimic real post-process: move the file, then walk up from + the source directory removing empties (which includes our + staging_album_dir).""" + pp_count[0] += 1 + final = str(final_dir / f"final_{pp_count[0]}.flac") + shutil.move(fp, final) + ctx['_final_processed_path'] = final + + # Walk up from the staged file's old directory, deleting + # any empty dir until we hit the staging root. + dir_to_check = os.path.dirname(fp) + while os.path.normpath(dir_to_check) != os.path.normpath(str(staging)): + try: + os.rmdir(dir_to_check) + except OSError: + break + dir_to_check = os.path.dirname(dir_to_check) + + summary = library_reorganize.reorganize_album( + album_id='alb-1', db=db, staging_root=str(staging), + resolve_file_path_fn=lambda p: p, + post_process_fn=pp_with_aggressive_cleanup, + ) + + # All three tracks must succeed despite the staging dir being + # nuked between each one. + assert summary['moved'] == 3 + assert summary['failed'] == 0 + + +def test_db_update_failure_leaves_original_in_place(monkeypatch, tmpdirs): + """Safety property: a failing DB write must NOT trigger the original + file's deletion. Otherwise we'd have a library row pointing at a + now-deleted path with no easy recovery. Better: leave the file at + BOTH locations (original + new) so the next library scan re-indexes + from the new path and the user doesn't lose data.""" + library, staging, _transfer = tmpdirs + db = _FakeDB() + src = _make_audio_file(library, 't1.flac') + final_dir = library / 'final' + final_dir.mkdir() + + _setup_album(db, deezer_id='dz-1', tracks=[ + ('t1', 1, 'Track 1', src), + ]) + + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'deezer') + monkeypatch.setattr(library_reorganize, 'get_source_priority', lambda p: [p]) + monkeypatch.setattr(library_reorganize, 'get_album_for_source', + lambda *a: {'id': 'dz-1', 'name': 'A'}) + monkeypatch.setattr( + library_reorganize, 'get_album_tracks_for_source', + lambda *a: {'items': [{'id': 'a1', 'name': 'Track 1', 'track_number': 1}]}, + ) + + final_path = str(final_dir / 't1.flac') + + def pp(key, ctx, fp): + shutil.move(fp, final_path) + ctx['_final_processed_path'] = final_path + + def update_path_explodes(track_id, new_path): + raise RuntimeError("simulated DB failure") + + library_reorganize.reorganize_album( + album_id='alb-1', db=db, staging_root=str(staging), + resolve_file_path_fn=lambda p: p, post_process_fn=pp, + update_track_path_fn=update_path_explodes, + ) + + assert os.path.exists(src), "Original must still exist when DB update failed" + assert os.path.exists(final_path), "New path file should also exist (post-process succeeded)" + + +def test_successful_run_removes_original_and_updates_db(monkeypatch, tmpdirs): + library, staging, _transfer = tmpdirs + db = _FakeDB() + src = _make_audio_file(library, 't1.flac') + final_dir = library / 'final' + final_dir.mkdir() + + _setup_album(db, deezer_id='dz-1', tracks=[ + ('t1', 1, 'Track 1', src), + ]) + + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'deezer') + monkeypatch.setattr(library_reorganize, 'get_source_priority', lambda p: [p]) + monkeypatch.setattr(library_reorganize, 'get_album_for_source', + lambda *a: {'id': 'dz-1', 'name': 'A'}) + monkeypatch.setattr( + library_reorganize, 'get_album_tracks_for_source', + lambda *a: {'items': [{'id': 'a1', 'name': 'Track 1', 'track_number': 1}]}, + ) + + def pp(key, ctx, fp): + # Pretend post-processing moved the staged file to a final location + final = str(final_dir / 't1.flac') + shutil.move(fp, final) + ctx['_final_processed_path'] = final + + db_updates = [] + + def update_path(track_id, new_path): + db_updates.append((track_id, new_path)) + + summary = library_reorganize.reorganize_album( + album_id='alb-1', db=db, staging_root=str(staging), + resolve_file_path_fn=lambda p: p, post_process_fn=pp, + update_track_path_fn=update_path, + ) + + assert summary['moved'] == 1 + assert summary['failed'] == 0 + assert not os.path.exists(src) + assert os.path.exists(str(final_dir / 't1.flac')) + assert db_updates == [('t1', str(final_dir / 't1.flac'))] + + +# --- tests: cleanup ------------------------------------------------------- + +def test_staging_dir_cleaned_up_on_success(monkeypatch, tmpdirs): + library, staging, _transfer = tmpdirs + db = _FakeDB() + _setup_album(db, deezer_id='dz-1', tracks=[ + ('t1', 1, 'Track 1', _make_audio_file(library, 't1.flac')), + ]) + + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'deezer') + monkeypatch.setattr(library_reorganize, 'get_source_priority', lambda p: [p]) + monkeypatch.setattr(library_reorganize, 'get_album_for_source', + lambda *a: {'id': 'dz-1', 'name': 'A'}) + monkeypatch.setattr( + library_reorganize, 'get_album_tracks_for_source', + lambda *a: {'items': [{'id': 'a1', 'name': 'Track 1', 'track_number': 1}]}, + ) + + def pp(key, ctx, fp): + final = str(library / 'final.flac') + shutil.move(fp, final) + ctx['_final_processed_path'] = final + + library_reorganize.reorganize_album( + album_id='alb-1', db=db, staging_root=str(staging), + resolve_file_path_fn=lambda p: p, post_process_fn=pp, + ) + + assert os.listdir(str(staging)) == [] + + +def test_staging_dir_cleaned_up_even_on_failure(monkeypatch, tmpdirs): + library, staging, _transfer = tmpdirs + db = _FakeDB() + _setup_album(db, deezer_id='dz-1', tracks=[ + ('t1', 1, 'Track 1', _make_audio_file(library, 't1.flac')), + ]) + + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'deezer') + monkeypatch.setattr(library_reorganize, 'get_source_priority', lambda p: [p]) + monkeypatch.setattr(library_reorganize, 'get_album_for_source', + lambda *a: {'id': 'dz-1', 'name': 'A'}) + monkeypatch.setattr( + library_reorganize, 'get_album_tracks_for_source', + lambda *a: {'items': [{'id': 'a1', 'name': 'Track 1', 'track_number': 1}]}, + ) + + def pp(key, ctx, fp): + raise RuntimeError("simulated post-process explosion") + + library_reorganize.reorganize_album( + album_id='alb-1', db=db, staging_root=str(staging), + resolve_file_path_fn=lambda p: p, post_process_fn=pp, + ) + + assert os.listdir(str(staging)) == [] + + +# --- tests: misc ---------------------------------------------------------- + +def test_deletes_per_track_sidecars_after_successful_move(monkeypatch, tmpdirs): + """Real-world Kendrick-Lamar-deluxe shape: each FLAC has a same-stem + `.lrc` sidecar in the source folder. After the audio is moved to its + new location, the original `.lrc` should be removed too — post-process + handles whatever sidecar policy exists at the new destination.""" + library, staging, _transfer = tmpdirs + db = _FakeDB() + audio = _make_audio_file(library, '01 - Sherane.flac') + lrc_path = library / '01 - Sherane.lrc' + lrc_path.write_text('lyrics') + nfo_path = library / '01 - Sherane.nfo' + nfo_path.write_text('metadata') + _setup_album(db, deezer_id='dz-1', tracks=[ + ('t1', 1, 'Sherane', audio), + ]) + + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'deezer') + monkeypatch.setattr(library_reorganize, 'get_source_priority', lambda p: [p]) + monkeypatch.setattr(library_reorganize, 'get_album_for_source', + lambda *a: {'id': 'dz-1', 'name': 'A'}) + monkeypatch.setattr( + library_reorganize, 'get_album_tracks_for_source', + lambda *a: {'items': [{'id': 'a1', 'name': 'Sherane', 'track_number': 1}]}, + ) + + final_dir = library / 'final' + final_dir.mkdir() + + def pp(key, ctx, fp): + final = str(final_dir / 'out.flac') + shutil.move(fp, final) + ctx['_final_processed_path'] = final + + library_reorganize.reorganize_album( + album_id='alb-1', db=db, staging_root=str(staging), + resolve_file_path_fn=lambda p: p, post_process_fn=pp, + ) + + assert not os.path.exists(audio) + assert not lrc_path.exists() + assert not nfo_path.exists() + + +def test_keeps_track_sidecars_when_track_fails_to_move(monkeypatch, tmpdirs): + """If post-process fails (AcoustID rejection), the original audio is + preserved — and so is its sidecar, because the user might want to + investigate or recover the track.""" + library, staging, _transfer = tmpdirs + db = _FakeDB() + audio = _make_audio_file(library, '01 - Sherane.flac') + lrc_path = library / '01 - Sherane.lrc' + lrc_path.write_text('lyrics') + _setup_album(db, deezer_id='dz-1', tracks=[ + ('t1', 1, 'Sherane', audio), + ]) + + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'deezer') + monkeypatch.setattr(library_reorganize, 'get_source_priority', lambda p: [p]) + monkeypatch.setattr(library_reorganize, 'get_album_for_source', + lambda *a: {'id': 'dz-1', 'name': 'A'}) + monkeypatch.setattr( + library_reorganize, 'get_album_tracks_for_source', + lambda *a: {'items': [{'id': 'a1', 'name': 'Sherane', 'track_number': 1}]}, + ) + + def pp_rejects(key, ctx, fp): + return # don't set _final_processed_path = AcoustID-style rejection + + library_reorganize.reorganize_album( + album_id='alb-1', db=db, staging_root=str(staging), + resolve_file_path_fn=lambda p: p, post_process_fn=pp_rejects, + ) + + assert os.path.exists(audio) + assert lrc_path.exists() + + +def test_deletes_album_level_sidecars_when_directory_emptied(monkeypatch, tmpdirs): + """After every track in a source dir is successfully moved out, the + leftover album-level sidecars (cover.jpg, folder.jpg, etc.) should be + removed too so the empty-dir pruner can take the dir. If even one + track failed to move, leave them — the user might want the cover.""" + library, staging, _transfer = tmpdirs + disc1_dir = library / 'Disc 1' + disc1_dir.mkdir() + a1 = _make_audio_file(disc1_dir, '01.flac') + a2 = _make_audio_file(disc1_dir, '02.flac') + cover = disc1_dir / 'cover.jpg' + cover.write_bytes(b'JPEGdata') + folder = disc1_dir / 'folder.jpg' + folder.write_bytes(b'JPEGdata') + + db = _FakeDB() + _setup_album(db, deezer_id='dz-1', tracks=[ + ('t1', 1, 'Track 1', a1), + ('t2', 2, 'Track 2', a2), + ]) + + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'deezer') + monkeypatch.setattr(library_reorganize, 'get_source_priority', lambda p: [p]) + monkeypatch.setattr(library_reorganize, 'get_album_for_source', + lambda *a: {'id': 'dz-1', 'name': 'A'}) + monkeypatch.setattr( + library_reorganize, 'get_album_tracks_for_source', + lambda *a: {'items': [ + {'id': 'a1', 'name': 'Track 1', 'track_number': 1}, + {'id': 'a2', 'name': 'Track 2', 'track_number': 2}, + ]}, + ) + + final_dir = library / 'final' + final_dir.mkdir() + + def pp(key, ctx, fp): + final = str(final_dir / f"{ctx['track_info']['track_number']}.flac") + shutil.move(fp, final) + ctx['_final_processed_path'] = final + + library_reorganize.reorganize_album( + album_id='alb-1', db=db, staging_root=str(staging), + resolve_file_path_fn=lambda p: p, post_process_fn=pp, + ) + + assert not cover.exists() + assert not folder.exists() + + +def test_keeps_album_sidecars_when_a_track_failed_to_move(monkeypatch, tmpdirs): + """If even one track in the dir failed to move out, leave the album + art alone — user might still want to look at / recover the album.""" + library, staging, _transfer = tmpdirs + a1 = _make_audio_file(library, '01.flac') + a2 = _make_audio_file(library, '02.flac') + cover = library / 'cover.jpg' + cover.write_bytes(b'JPEGdata') + + db = _FakeDB() + _setup_album(db, deezer_id='dz-1', tracks=[ + ('t1', 1, 'Track 1', a1), + ('t2', 2, 'Track 2', a2), + ]) + + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'deezer') + monkeypatch.setattr(library_reorganize, 'get_source_priority', lambda p: [p]) + monkeypatch.setattr(library_reorganize, 'get_album_for_source', + lambda *a: {'id': 'dz-1', 'name': 'A'}) + monkeypatch.setattr( + library_reorganize, 'get_album_tracks_for_source', + lambda *a: {'items': [ + {'id': 'a1', 'name': 'Track 1', 'track_number': 1}, + {'id': 'a2', 'name': 'Track 2', 'track_number': 2}, + ]}, + ) + + final_dir = library / 'final' + final_dir.mkdir() + + def pp(key, ctx, fp): + # Track 1 succeeds, track 2 fails (no _final_processed_path set) + if ctx['track_info']['track_number'] == 1: + final = str(final_dir / '1.flac') + shutil.move(fp, final) + ctx['_final_processed_path'] = final + + library_reorganize.reorganize_album( + album_id='alb-1', db=db, staging_root=str(staging), + resolve_file_path_fn=lambda p: p, post_process_fn=pp, + ) + + # Track 2 still in place → cover preserved + assert os.path.exists(a2) + assert cover.exists() + + +# --- preview function (shared planning with the orchestrator) ----------- + +def _fake_path_builder(context, spotify_artist, _album_info, file_ext): + """Stand-in for `_build_final_path_for_track`. Inserts Disc N/ when + total_discs > 1 — same convention the real builder uses.""" + album = context['spotify_album']['name'] + artist = spotify_artist['name'] + track_info = context['track_info'] + title = track_info['name'] + tn = track_info['track_number'] + dn = track_info['disc_number'] + total = context['spotify_album']['total_discs'] + parts = ['/transfer', artist, album] + if total > 1: + parts.append(f'Disc {dn}') + parts.append(f"{tn:02d} - {title}{file_ext}") + return '/'.join(parts), True + + +def _path_builder_album_vs_single(context, spotify_artist, album_info, file_ext): + """Stand-in that emulates the real `_build_final_path_for_track` + branch on `album_info.get('is_album')`. ALBUM mode produces an + album folder with disc subfolder + numbered file; SINGLE mode + produces a per-track folder named after the title (the bug + output).""" + artist = spotify_artist['name'] + if album_info and album_info.get('is_album'): + album = album_info['album_name'] + title = album_info['clean_track_name'] + tn = album_info['track_number'] + dn = album_info['disc_number'] + total = context['spotify_album']['total_discs'] + if total > 1: + return (f'/transfer/{artist}/{artist} - {album}/Disc {dn}/{tn:02d} - {title}{file_ext}', True) + return (f'/transfer/{artist}/{artist} - {album}/{tn:02d} - {title}{file_ext}', True) + title = context['track_info']['name'] + return (f'/transfer/{artist}/{artist} - {title}/{title}{file_ext}', True) + + +def test_preview_uses_album_mode_not_single_mode(monkeypatch, tmpdirs): + """Regression for the bug where every track ended up in its own + track-named folder (SINGLE MODE) because we passed None for + album_info to the path builder. Multi-disc deluxe must produce + one shared album folder, not N single folders.""" + library, _staging, _transfer = tmpdirs + db = _FakeDB() + _setup_album(db, deezer_id='dz-1', tracks=[ + ('t1', 1, 'Sherane', _make_audio_file(library, 't1.flac')), + ('t2', 2, 'Bitch Dont Kill My Vibe', _make_audio_file(library, 't2.flac')), + ]) + + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'deezer') + monkeypatch.setattr(library_reorganize, 'get_source_priority', lambda p: [p]) + monkeypatch.setattr(library_reorganize, 'get_album_for_source', + lambda *a: {'id': 'dz-1', 'name': 'good kid, m.A.A.d city'}) + monkeypatch.setattr( + library_reorganize, 'get_album_tracks_for_source', + lambda *a: {'items': [ + {'id': 'a1', 'name': 'Sherane', 'track_number': 1, 'disc_number': 1}, + {'id': 'a2', 'name': 'Bitch Dont Kill My Vibe', 'track_number': 2, 'disc_number': 1}, + ]}, + ) + + result = library_reorganize.preview_album_reorganize( + album_id='alb-1', db=db, transfer_dir='/transfer', + resolve_file_path_fn=lambda p: p, + build_final_path_fn=_path_builder_album_vs_single, + ) + + paths = [it['new_path'] for it in result['tracks']] + # Both tracks land under the SAME album folder, not per-track folders + assert all('good kid, m.A.A.d city' in p for p in paths) + # Files use track-number prefix (album mode), not bare title (single mode) + assert any('01 - Sherane' in p for p in paths) + assert any('02 - Bitch Dont Kill My Vibe' in p for p in paths) + # Reject the single-mode shape explicitly + assert not any(p.endswith('/Sherane.flac') for p in paths) + + +def test_preview_emits_disc_subfolders_for_multi_disc_albums(monkeypatch, tmpdirs): + """The bug winecountrygames hit: preview showed all tracks at the + album root with no Disc N/ subfolders, even on a deluxe edition. + Verify the new planner-backed preview produces disc folders.""" + library, _staging, _transfer = tmpdirs + db = _FakeDB() + _setup_album(db, deezer_id='dz-1', tracks=[ + ('t1d1', 1, 'Sherane', _make_audio_file(library, 'd1t1.flac')), + ('t1d2', 1, 'The Recipe', _make_audio_file(library, 'd2t1.flac')), + ]) + + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'deezer') + monkeypatch.setattr(library_reorganize, 'get_source_priority', lambda p: [p]) + monkeypatch.setattr(library_reorganize, 'get_album_for_source', + lambda *a: {'id': 'dz-1', 'name': 'good kid, m.A.A.d city (Deluxe)'}) + monkeypatch.setattr( + library_reorganize, 'get_album_tracks_for_source', + lambda *a: {'items': [ + {'id': 'a1', 'name': 'Sherane', 'track_number': 1, 'disc_number': 1}, + {'id': 'a2', 'name': 'The Recipe', 'track_number': 1, 'disc_number': 2}, + ]}, + ) + + result = library_reorganize.preview_album_reorganize( + album_id='alb-1', db=db, transfer_dir='/transfer', + resolve_file_path_fn=lambda p: p, + build_final_path_fn=_fake_path_builder, + ) + + assert result['success'] is True + assert result['status'] == 'planned' + + by_title = {it['title']: it for it in result['tracks']} + assert 'Disc 1' in by_title['Sherane']['new_path'] + assert 'Disc 2' in by_title['The Recipe']['new_path'] + # And per-track disc_number is propagated for UI display + assert by_title['Sherane']['disc_number'] == 1 + assert by_title['The Recipe']['disc_number'] == 2 + + +def test_preview_status_no_source_id_when_album_lacks_ids(monkeypatch, tmpdirs): + library, _staging, _transfer = tmpdirs + db = _FakeDB() + _setup_album(db, tracks=[ + ('t1', 1, 'Track 1', _make_audio_file(library, 't1.flac')), + ]) + + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'deezer') + monkeypatch.setattr(library_reorganize, 'get_source_priority', + lambda p: [p, 'spotify', 'itunes', 'discogs', 'hydrabase']) + monkeypatch.setattr(library_reorganize, 'get_album_for_source', lambda *a: None) + monkeypatch.setattr(library_reorganize, 'get_album_tracks_for_source', lambda *a: None) + + result = library_reorganize.preview_album_reorganize( + album_id='alb-1', db=db, transfer_dir='/transfer', + resolve_file_path_fn=lambda p: p, + build_final_path_fn=_fake_path_builder, + ) + + assert result['status'] == 'no_source_id' + assert result['success'] is False + + +def test_preview_marks_unmatched_tracks(monkeypatch, tmpdirs): + """Tracks with no plausible API match (no exact title, no substring, + no track_number) get reported as unmatched with a reason.""" + library, _staging, _transfer = tmpdirs + db = _FakeDB() + _setup_album(db, deezer_id='dz-1', tracks=[ + ('t1', 1, 'A Real Track', _make_audio_file(library, 't1.flac')), + # Use a track_number with no API counterpart and a title that + # has no substring overlap with anything in the API list — so + # no tier matches. + ('t99', 99, 'Completely Unrelated Side Quest', + _make_audio_file(library, 't99.flac')), + ]) + + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'deezer') + monkeypatch.setattr(library_reorganize, 'get_source_priority', lambda p: [p]) + monkeypatch.setattr(library_reorganize, 'get_album_for_source', + lambda *a: {'id': 'dz-1', 'name': 'A'}) + monkeypatch.setattr( + library_reorganize, 'get_album_tracks_for_source', + lambda *a: {'items': [{'id': 'a1', 'name': 'A Real Track', 'track_number': 1}]}, + ) + + result = library_reorganize.preview_album_reorganize( + album_id='alb-1', db=db, transfer_dir='/transfer', + resolve_file_path_fn=lambda p: p, + build_final_path_fn=_fake_path_builder, + ) + + by_title = {it['title']: it for it in result['tracks']} + assert by_title['A Real Track']['matched'] is True + assert by_title['A Real Track']['new_path'] + assert by_title['Completely Unrelated Side Quest']['matched'] is False + assert by_title['Completely Unrelated Side Quest']['reason'] + assert by_title['Completely Unrelated Side Quest']['new_path'] == '' + + +def test_preview_uses_same_logic_as_apply(monkeypatch, tmpdirs): + """Sanity check: a multi-disc album previewed and then applied + should show the same destinations. If preview drift creeps in + again, this fails.""" + library, staging, _transfer = tmpdirs + db = _FakeDB() + _setup_album(db, deezer_id='dz-1', tracks=[ + ('t1d1', 1, 'D1T1', _make_audio_file(library, 'd1t1.flac')), + ('t1d2', 1, 'D2T1', _make_audio_file(library, 'd2t1.flac')), + ]) + + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'deezer') + monkeypatch.setattr(library_reorganize, 'get_source_priority', lambda p: [p]) + monkeypatch.setattr(library_reorganize, 'get_album_for_source', + lambda *a: {'id': 'dz-1', 'name': 'Test Album'}) + monkeypatch.setattr( + library_reorganize, 'get_album_tracks_for_source', + lambda *a: {'items': [ + {'id': 'a1', 'name': 'D1T1', 'track_number': 1, 'disc_number': 1}, + {'id': 'a2', 'name': 'D2T1', 'track_number': 1, 'disc_number': 2}, + ]}, + ) + + preview = library_reorganize.preview_album_reorganize( + album_id='alb-1', db=db, transfer_dir='/transfer', + resolve_file_path_fn=lambda p: p, + build_final_path_fn=_fake_path_builder, + ) + + # Now apply with the same matching logic; assert apply uses the + # same disc_number per track that the preview reported. + apply_disc_per_title = {} + + def pp(key, ctx, fp): + apply_disc_per_title[ctx['track_info']['name']] = ctx['track_info']['disc_number'] + ctx['_final_processed_path'] = fp + with open(fp, 'wb') as f: + f.write(b'final') + + library_reorganize.reorganize_album( + album_id='alb-1', db=db, staging_root=str(staging), + resolve_file_path_fn=lambda p: p, post_process_fn=pp, + ) + + preview_disc_per_title = {it['title']: it['disc_number'] for it in preview['tracks']} + assert preview_disc_per_title == apply_disc_per_title + + +def test_available_sources_only_lists_authed_sources_with_stored_ids(monkeypatch): + """The reorganize modal needs to know which sources the user can + actually pick. A source is pickable iff: (a) we have an album ID + for that source on the local row, AND (b) the user has the source + authed/configured. Empty-ID sources and unauthed sources are + omitted.""" + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'deezer') + monkeypatch.setattr(library_reorganize, 'get_source_priority', + lambda p: [p, 'spotify', 'itunes', 'discogs', 'hydrabase']) + + # Authed: deezer + spotify only. + auth = {'deezer': object(), 'spotify': object()} + monkeypatch.setattr(library_reorganize, 'get_client_for_source', + lambda src: auth.get(src)) + + album = { + 'spotify_album_id': 'sp-1', + 'deezer_id': 'dz-1', + 'itunes_album_id': 'it-1', # has ID but user not authed + 'discogs_id': '', # no ID + 'soul_id': '', # no ID + } + + sources = library_reorganize.available_sources_for_album(album) + names = [s['source'] for s in sources] + + assert names == ['deezer', 'spotify'] + assert all('label' in s for s in sources) + + +def test_authed_sources_lists_all_authed_regardless_of_album_ids(monkeypatch): + """Bulk reorganize uses this — needs the authed sources without + requiring per-album ID coverage. Each album in the bulk run will + do its own per-album ID check at apply time.""" + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'spotify') + monkeypatch.setattr(library_reorganize, 'get_source_priority', + lambda p: [p, 'deezer', 'itunes', 'discogs', 'hydrabase']) + + # Authed: spotify + deezer + itunes; discogs + hydrabase NOT authed. + auth = {'spotify': object(), 'deezer': object(), 'itunes': object()} + monkeypatch.setattr(library_reorganize, 'get_client_for_source', + lambda src: auth.get(src)) + + sources = library_reorganize.authed_sources() + names = [s['source'] for s in sources] + + # Primary first, then rest of priority chain — only authed ones + assert names == ['spotify', 'deezer', 'itunes'] + assert all('label' in s for s in sources) + + +def test_strict_source_does_not_fall_back(monkeypatch, tmpdirs): + """When the user picks a specific source in the modal, we must NOT + silently fall back to another source if their pick fails. Picking + Spotify means 'use Spotify or fail' — falling back would defeat + the picker's purpose.""" + library, staging, _transfer = tmpdirs + db = _FakeDB() + _setup_album(db, deezer_id='dz-1', spotify_id='sp-1', tracks=[ + ('t1', 1, 'Track 1', _make_audio_file(library, 't1.flac')), + ]) + + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'deezer') + monkeypatch.setattr(library_reorganize, 'get_source_priority', + lambda p: [p, 'deezer', 'itunes']) + + fetched = [] + + def fake_album(src, sid): + fetched.append(('album', src)) + if src == 'deezer': + return {'id': 'dz-1', 'name': 'Album'} + return None # spotify "fails" + + def fake_tracks(src, sid): + fetched.append(('tracks', src)) + if src == 'deezer': + return {'items': [{'id': 'd1', 'name': 'Track 1', 'track_number': 1}]} + return None + + monkeypatch.setattr(library_reorganize, 'get_album_for_source', fake_album) + monkeypatch.setattr(library_reorganize, 'get_album_tracks_for_source', fake_tracks) + + pp_calls = [] + + def pp(*a, **k): + pp_calls.append(a) + + summary = library_reorganize.reorganize_album( + album_id='alb-1', db=db, staging_root=str(staging), + resolve_file_path_fn=lambda p: p, post_process_fn=pp, + primary_source='spotify', strict_source=True, + ) + + # Spotify failed; with strict_source we must NOT have queried Deezer. + assert summary['status'] == 'no_source_id' + assert summary['moved'] == 0 + assert pp_calls == [] + assert all(src == 'spotify' for _kind, src in fetched) + + +def test_non_strict_falls_back_when_primary_returns_nothing(monkeypatch, tmpdirs): + """When the user did NOT pick a specific source (default behavior), + the orchestrator walks the priority chain as before.""" + library, staging, _transfer = tmpdirs + db = _FakeDB() + _setup_album(db, deezer_id='dz-1', spotify_id='sp-1', tracks=[ + ('t1', 1, 'Track 1', _make_audio_file(library, 't1.flac')), + ]) + + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'spotify') + monkeypatch.setattr(library_reorganize, 'get_source_priority', + lambda p: [p, 'deezer', 'itunes']) + monkeypatch.setattr(library_reorganize, 'get_album_for_source', + lambda src, sid: ({'id': sid, 'name': 'A'} if src == 'deezer' else None)) + monkeypatch.setattr( + library_reorganize, 'get_album_tracks_for_source', + lambda src, sid: ({'items': [{'id': 'a1', 'name': 'Track 1', 'track_number': 1}]} + if src == 'deezer' else None), + ) + + def pp(key, ctx, fp): + ctx['_final_processed_path'] = str(library / 'out.flac') + with open(ctx['_final_processed_path'], 'wb') as f: + f.write(b'final') + + summary = library_reorganize.reorganize_album( + album_id='alb-1', db=db, staging_root=str(staging), + resolve_file_path_fn=lambda p: p, post_process_fn=pp, + # No strict_source → uses default fallback chain + ) + assert summary['source'] == 'deezer' + assert summary['moved'] == 1 + + +def test_returns_no_album_when_id_does_not_exist(tmpdirs): + _library, staging, _transfer = tmpdirs + db = _FakeDB() + cur = db._conn.cursor() + cur.execute("CREATE TABLE artists (id TEXT, name TEXT)") + cur.execute( + "CREATE TABLE albums (id TEXT, artist_id TEXT, title TEXT, " + "spotify_album_id TEXT, deezer_id TEXT, itunes_album_id TEXT, " + "discogs_id TEXT, soul_id TEXT)" + ) + cur.execute( + "CREATE TABLE tracks (id TEXT, album_id TEXT, artist_id TEXT, " + "title TEXT, track_number INTEGER, file_path TEXT, updated_at TEXT)" + ) + db._conn.commit() + + summary = library_reorganize.reorganize_album( + album_id='does-not-exist', db=db, staging_root=str(staging), + resolve_file_path_fn=lambda p: p, post_process_fn=lambda *a: None, + ) + + assert summary['status'] == 'no_album' + + +def test_returns_no_tracks_when_album_has_none(tmpdirs): + _library, staging, _transfer = tmpdirs + db = _FakeDB() + _setup_album(db, deezer_id='dz-1', tracks=[]) + + summary = library_reorganize.reorganize_album( + album_id='alb-1', db=db, staging_root=str(staging), + resolve_file_path_fn=lambda p: p, post_process_fn=lambda *a: None, + ) + + assert summary['status'] == 'no_tracks' + + +def test_processes_tracks_concurrently_with_consistent_state(monkeypatch, tmpdirs): + """Reorganize should run multiple tracks in parallel (matching the + download-side worker count). Verify both the parallelism (we observe + overlapping post-process calls) AND the state consistency (all + tracks are accounted for, no double-counting from races).""" + library, staging, _transfer = tmpdirs + db = _FakeDB() + track_count = 6 + rows = [] + for i in range(1, track_count + 1): + rows.append((f't{i}', i, f'Track {i}', _make_audio_file(library, f't{i}.flac'))) + _setup_album(db, deezer_id='dz-1', tracks=rows) + + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'deezer') + monkeypatch.setattr(library_reorganize, 'get_source_priority', lambda p: [p]) + monkeypatch.setattr(library_reorganize, 'get_album_for_source', + lambda *a: {'id': 'dz-1', 'name': 'A'}) + monkeypatch.setattr( + library_reorganize, 'get_album_tracks_for_source', + lambda *a: {'items': [ + {'id': f'a{i}', 'name': f'Track {i}', 'track_number': i} + for i in range(1, track_count + 1) + ]}, + ) + + import threading + import time + + in_flight = 0 + max_in_flight = 0 + in_flight_lock = threading.Lock() + final_dir = library / 'final' + final_dir.mkdir() + + def slow_pp(key, ctx, fp): + nonlocal in_flight, max_in_flight + with in_flight_lock: + in_flight += 1 + max_in_flight = max(max_in_flight, in_flight) + # Hold the worker briefly so concurrency is observable + time.sleep(0.05) + with in_flight_lock: + in_flight -= 1 + out = str(final_dir / f"out_{ctx['track_info']['track_number']}.flac") + shutil.move(fp, out) + ctx['_final_processed_path'] = out + + summary = library_reorganize.reorganize_album( + album_id='alb-1', db=db, staging_root=str(staging), + resolve_file_path_fn=lambda p: p, post_process_fn=slow_pp, + ) + + # All 6 tracks processed; counts are consistent (no race-induced duplicates) + assert summary['moved'] == track_count + assert summary['skipped'] == 0 + assert summary['failed'] == 0 + # Should have observed at least 2 workers in flight at once + # (3 is the configured cap; some overlap should always occur with 6 slow tracks) + assert max_in_flight >= 2, f"Expected concurrent workers, only saw {max_in_flight} in flight" + + +def test_prunes_empty_destination_album_dirs(monkeypatch, tmpdirs): + """When transfer_dir is provided, the orchestrator must clean up + empty sibling album folders in the artist directory after the run. + Catches both (a) leftovers from previous failed reorganize attempts + that created standalone single-track folders, and (b) dirs created + by `_build_final_path_for_track` that ended up empty when post- + process failed AcoustID. Uses a single-level prune scoped to the + artist folder — won't touch unrelated user dirs.""" + library, staging, transfer = tmpdirs + db = _FakeDB() + src = _make_audio_file(library, 't1.flac') + _setup_album(db, deezer_id='dz-1', tracks=[ + ('t1', 1, 'Track 1', src), + ]) + + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'deezer') + monkeypatch.setattr(library_reorganize, 'get_source_priority', lambda p: [p]) + monkeypatch.setattr(library_reorganize, 'get_album_for_source', + lambda *a: {'id': 'dz-1', 'name': 'A'}) + monkeypatch.setattr( + library_reorganize, 'get_album_tracks_for_source', + lambda *a: {'items': [{'id': 'a1', 'name': 'Track 1', 'track_number': 1}]}, + ) + + # Simulate the user's actual situation: transfer dir already has + # an artist folder with leftover empty single-track album folders + # from previous failed runs, plus an empty Disc-N subfolder. + artist_dir = transfer / 'Artist' + artist_dir.mkdir() + (artist_dir / 'Artist - 2013 Backseat Freestyle').mkdir() + (artist_dir / 'Artist - 2013 Compton').mkdir() + leftover_with_disc = artist_dir / 'Artist - 2012 Old Single-Disc' + leftover_with_disc.mkdir() + (leftover_with_disc / 'Disc 1').mkdir() # empty disc subfolder + + # Successful track lands in the real album folder + real_album = artist_dir / 'Artist - 2013 Real Album' + real_album.mkdir() + + def pp(key, ctx, fp): + final = str(real_album / 'Disc 1' / '01 - Track 1.flac') + os.makedirs(os.path.dirname(final), exist_ok=True) + shutil.move(fp, final) + ctx['_final_processed_path'] = final + + library_reorganize.reorganize_album( + album_id='alb-1', db=db, staging_root=str(staging), + resolve_file_path_fn=lambda p: p, post_process_fn=pp, + transfer_dir=str(transfer), + ) + + # Empty leftover single-track album folders should be gone + assert not (artist_dir / 'Artist - 2013 Backseat Freestyle').exists() + assert not (artist_dir / 'Artist - 2013 Compton').exists() + # The album with an empty Disc subfolder should also be cleaned + # (Disc 1/ is empty → pruned, then Old Single-Disc/ is empty → pruned) + assert not leftover_with_disc.exists() + # Real album with successful track must still exist + assert real_album.exists() + assert (real_album / 'Disc 1' / '01 - Track 1.flac').exists() + # Artist folder itself (still has the real album) untouched + assert artist_dir.exists() + + +def test_context_dict_satisfies_post_process_contract(monkeypatch, tmpdirs): + """Integration-style test: assert the per-track context dict the + orchestrator hands to post-process contains every key + `_post_process_matched_download` and `_build_final_path_for_track` + actually read in production. If the real post-process starts + requiring a new key in a future refactor, this test catches it + BEFORE the user does — unit-mock tests would not. + + Keys verified are taken from a grep of the real functions in + web_server.py at the time this test was written. The list is the + contract; if it grows, the orchestrator's `_build_post_process_context` + needs to grow too.""" + library, staging, _transfer = tmpdirs + db = _FakeDB() + _setup_album(db, deezer_id='dz-1', tracks=[ + ('t1', 1, 'Track 1', _make_audio_file(library, 't1.flac')), + ]) + + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'deezer') + monkeypatch.setattr(library_reorganize, 'get_source_priority', lambda p: [p]) + monkeypatch.setattr(library_reorganize, 'get_album_for_source', + lambda *a: { + 'id': 'dz-1', + 'name': 'Test Album', + 'release_date': '2024-03-15', + 'total_tracks': 12, + 'image_url': 'https://example.com/cover.jpg', + }) + monkeypatch.setattr( + library_reorganize, 'get_album_tracks_for_source', + lambda *a: {'items': [{ + 'id': 'a1', 'name': 'Track 1', + 'track_number': 1, 'disc_number': 1, + 'duration_ms': 240000, + 'artists': [{'name': 'Aerosmith'}], + 'uri': 'spotify:track:abc', + }]}, + ) + + captured_context = {} + + def assert_contract(key, ctx, fp): + captured_context.update(ctx) + # Mimic the bits of real post-process this test cares about + ctx['_final_processed_path'] = str(library / 'out.flac') + with open(ctx['_final_processed_path'], 'wb') as f: + f.write(b'final') + + library_reorganize.reorganize_album( + album_id='alb-1', db=db, staging_root=str(staging), + resolve_file_path_fn=lambda p: p, post_process_fn=assert_contract, + ) + + # Top-level keys the real post-process reads + assert captured_context.get('is_album_download') is True + assert captured_context.get('has_clean_spotify_data') is True + assert captured_context.get('has_full_spotify_metadata') is True + + # spotify_artist (album-level artist context — not per-track) + spotify_artist = captured_context.get('spotify_artist') + assert isinstance(spotify_artist, dict) + assert 'name' in spotify_artist + assert 'id' in spotify_artist + assert 'genres' in spotify_artist + + # spotify_album (used by `_build_final_path_for_track`) + spotify_album = captured_context.get('spotify_album') + assert isinstance(spotify_album, dict) + assert spotify_album.get('id') == 'dz-1' + assert spotify_album.get('name') == 'Test Album' + assert 'release_date' in spotify_album # year extraction + assert 'total_tracks' in spotify_album # ALBUM/EP/Single inference + assert 'total_discs' in spotify_album # Disc N/ subfolder gate + assert 'image_url' in spotify_album # album art + + # track_info (per-track signal — populates filename, tags, disc subfolder) + track_info = captured_context.get('track_info') + assert isinstance(track_info, dict) + assert 'name' in track_info # filename + assert 'id' in track_info # source track id + assert 'track_number' in track_info # filename + tag + assert 'disc_number' in track_info # disc subfolder + tag + assert 'duration_ms' in track_info # tag + assert isinstance(track_info.get('artists'), list) # tag — must be list + assert all(isinstance(a, dict) and 'name' in a for a in track_info['artists']) + + # original_search_result (post-process reads this for fallbacks) + osr = captured_context.get('original_search_result') + assert isinstance(osr, dict) + assert 'title' in osr + assert 'spotify_clean_title' in osr # `_build_final_path_for_track` reads this + assert 'spotify_clean_album' in osr # ditto + assert 'track_number' in osr + assert 'disc_number' in osr + assert 'artists' in osr + + +def test_progress_callback_receives_updates(monkeypatch, tmpdirs): + library, staging, _transfer = tmpdirs + db = _FakeDB() + _setup_album(db, deezer_id='dz-1', tracks=[ + ('t1', 1, 'Track 1', _make_audio_file(library, 't1.flac')), + ('t2', 2, 'Track 2', _make_audio_file(library, 't2.flac')), + ]) + + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'deezer') + monkeypatch.setattr(library_reorganize, 'get_source_priority', lambda p: [p]) + monkeypatch.setattr(library_reorganize, 'get_album_for_source', + lambda *a: {'id': 'dz-1', 'name': 'A'}) + monkeypatch.setattr( + library_reorganize, 'get_album_tracks_for_source', + lambda *a: {'items': [ + {'id': 'a1', 'name': 'Track 1', 'track_number': 1}, + {'id': 'a2', 'name': 'Track 2', 'track_number': 2}, + ]}, + ) + + def pp(key, ctx, fp): + final = str(library / f"final_{ctx['track_info']['track_number']}.flac") + shutil.move(fp, final) + ctx['_final_processed_path'] = final + + progress_log = [] + + def on_progress(updates): + progress_log.append(dict(updates)) + + library_reorganize.reorganize_album( + album_id='alb-1', db=db, staging_root=str(staging), + resolve_file_path_fn=lambda p: p, post_process_fn=pp, + on_progress=on_progress, + ) + + assert any('total' in u for u in progress_log) + assert any('current_track' in u for u in progress_log) + assert any(u.get('moved') == 2 for u in progress_log) + + +def test_watchdog_warns_about_stuck_workers(monkeypatch, tmpdirs, caplog): + """When a worker exceeds the hung-threshold, the orchestrator must + log a warning naming the stuck track. Real threshold is 5 minutes; + we monkeypatch it down to ~50ms so the test runs in well under a + second. Watchdog is passive (doesn't kill threads), so the worker + should still complete normally after the warning.""" + import threading + library, staging, _transfer = tmpdirs + + # Tiny watchdog so the test is fast. Interval shorter than threshold + # so the loop checks at least once before the threshold trips. + monkeypatch.setattr(library_reorganize, '_WATCHDOG_INTERVAL_SECONDS', 0.02) + monkeypatch.setattr(library_reorganize, '_HUNG_WORKER_THRESHOLD_SECONDS', 0.05) + + db = _FakeDB() + _setup_album(db, deezer_id='dz-1', tracks=[ + ('t1', 1, 'Stuck Track', _make_audio_file(library, 't1.flac')), + ]) + + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'deezer') + monkeypatch.setattr(library_reorganize, 'get_source_priority', lambda p: [p]) + monkeypatch.setattr(library_reorganize, 'get_album_for_source', + lambda *a: {'id': 'dz-1', 'name': 'A'}) + monkeypatch.setattr( + library_reorganize, 'get_album_tracks_for_source', + lambda *a: {'items': [{'id': 'a1', 'name': 'Stuck Track', 'track_number': 1}]}, + ) + + release = threading.Event() + + def slow_pp(key, ctx, fp): + # Hold long enough for the watchdog to trip the threshold + emit. + # 0.2s vs 0.05s threshold + 0.02s interval = at least one warn pass. + release.wait(timeout=0.25) + ctx['_final_processed_path'] = fp + with open(fp, 'wb') as f: + f.write(b'final') + + caplog.set_level('WARNING', logger='library_reorganize') + + summary = library_reorganize.reorganize_album( + album_id='alb-1', db=db, staging_root=str(staging), + resolve_file_path_fn=lambda p: p, post_process_fn=slow_pp, + ) + release.set() + + # Track still completed (watchdog is passive — it doesn't abort) + assert summary['moved'] == 1 + + # And the watchdog warning was logged with the stuck track's title + warnings = [ + r.getMessage() for r in caplog.records + if r.levelname == 'WARNING' and 'Worker stuck' in r.getMessage() + ] + assert any('Stuck Track' in msg for msg in warnings), ( + f"Expected a 'Worker stuck' warning naming the track; got: {warnings}" + ) + + +def test_stop_check_aborts_remaining_tracks(monkeypatch, tmpdirs): + """With concurrent workers, stop_check can't cancel a task that's + already running — but it MUST prevent tasks that haven't started + yet from running. Use enough tracks that the worker pool can't + drain them all before stop_check trips.""" + import threading + library, staging, _transfer = tmpdirs + db = _FakeDB() + _setup_album(db, deezer_id='dz-1', tracks=[ + (f't{i}', i, f'Track {i}', _make_audio_file(library, f't{i}.flac')) + for i in range(1, 11) + ]) + + monkeypatch.setattr(library_reorganize, 'get_primary_source', lambda: 'deezer') + monkeypatch.setattr(library_reorganize, 'get_source_priority', lambda p: [p]) + monkeypatch.setattr(library_reorganize, 'get_album_for_source', + lambda *a: {'id': 'dz-1', 'name': 'A'}) + monkeypatch.setattr( + library_reorganize, 'get_album_tracks_for_source', + lambda *a: {'items': [ + {'id': f'a{i}', 'name': f'Track {i}', 'track_number': i} + for i in range(1, 11) + ]}, + ) + + pp_count = [0] + pp_lock = threading.Lock() + + def pp(key, ctx, fp): + with pp_lock: + pp_count[0] += 1 + ctx['_final_processed_path'] = fp + with open(fp, 'wb') as f: + f.write(b'fake-final') + + stop = [False] + def check_stop(): + with pp_lock: + if pp_count[0] >= 2: + stop[0] = True + return stop[0] + + library_reorganize.reorganize_album( + album_id='alb-1', db=db, staging_root=str(staging), + resolve_file_path_fn=lambda p: p, post_process_fn=pp, + stop_check=check_stop, + ) + + # Some tracks ran (the ones already in flight when stop tripped), + # but not ALL 10 — the stop_check cut off the unstarted ones. + assert pp_count[0] < 10 + assert pp_count[0] >= 2 diff --git a/web_server.py b/web_server.py index 2f00b8d6..8ed9b258 100644 --- a/web_server.py +++ b/web_server.py @@ -13607,143 +13607,64 @@ _reorganize_state = { _reorganize_lock = threading.Lock() +@app.route('/api/library/reorganize/sources', methods=['GET']) +def reorganize_sources_global(): + """List metadata sources the user has authed on this instance. + Used by the bulk "Reorganize All" modal where per-album ID coverage + varies. No network calls.""" + try: + from core.library_reorganize import authed_sources + return jsonify({"success": True, "sources": authed_sources()}) + except Exception as e: + logger.error(f"Reorganize sources (global) error: {e}") + return jsonify({"success": False, "error": str(e)}), 500 + + +@app.route('/api/library/album//reorganize/sources', methods=['GET']) +def reorganize_album_sources(album_id): + """List metadata sources the user can pick for this album's + reorganize — every entry has both a stored album ID on the local + row AND an authenticated client. No network calls.""" + try: + from core.library_reorganize import available_sources_for_album, load_album_and_tracks + album_data, _tracks = load_album_and_tracks(get_database(), album_id) + if album_data is None: + return jsonify({"success": False, "error": "Album not found"}), 404 + return jsonify({"success": True, "sources": available_sources_for_album(album_data)}) + except Exception as e: + logger.error(f"Reorganize sources error: {e}") + return jsonify({"success": False, "error": str(e)}), 500 + + @app.route('/api/library/album//reorganize/preview', methods=['POST']) def reorganize_album_preview(album_id): - """Preview file reorganization for an album — returns current vs proposed paths without moving anything.""" + """Preview file reorganization for an album — returns current vs + proposed paths without moving anything. Implementation lives in + :mod:`core.library_reorganize` and shares the planning logic with + the apply endpoint, so the preview is guaranteed to match what + apply would actually produce. + + Optional body param ``source``: when provided, only that metadata + source is queried (no fallback chain).""" try: - database = get_database() + from core.library_reorganize import preview_album_reorganize data = request.get_json() or {} - template = data.get('template', '').strip() - if not template: - return jsonify({"success": False, "error": "Template is required"}), 400 - - conn = database._get_connection() - cursor = conn.cursor() - - # Get album + artist info - cursor.execute(""" - SELECT al.*, a.name as artist_name - FROM albums al - JOIN artists a ON al.artist_id = a.id - WHERE al.id = ? - """, (str(album_id),)) - album_row = cursor.fetchone() - if not album_row: - return jsonify({"success": False, "error": "Album not found"}), 404 - album_data = dict(album_row) - - # Get all tracks for this album - cursor.execute(""" - SELECT t.*, a.name as artist_name - FROM tracks t - JOIN artists a ON t.artist_id = a.id - WHERE t.album_id = ? - ORDER BY t.track_number - """, (str(album_id),)) - tracks = [dict(r) for r in cursor.fetchall()] - - if not tracks: - return jsonify({"success": False, "error": "No tracks found for this album"}), 404 - + chosen_source = data.get('source') or None transfer_dir = docker_resolve_path(config_manager.get('soulseek.transfer_path', './Transfer')) - - # Pre-scan disc numbers so every track's template context carries the - # same total_discs. Needed by $cdnum (smart CD label) so the template - # can decide whether to emit "CDxx" or stay empty for single-disc. - track_disc_numbers = {} - for _t in tracks: - _rp = _resolve_library_file_path(_t.get('file_path')) if _t.get('file_path') else None - _dn = 1 - if _rp: - try: - from core.tag_writer import read_file_tags - _dn = read_file_tags(_rp).get('disc_number') or 1 - except Exception: - _dn = 1 - track_disc_numbers[_t.get('id')] = int(_dn) - total_discs = max(track_disc_numbers.values(), default=1) if track_disc_numbers else 1 - - preview_items = [] - for track in tracks: - file_path = track.get('file_path') - resolved = _resolve_library_file_path(file_path) if file_path else None - - # Reuse the disc number captured in the pre-scan (avoids re-reading tags) - disc_number = track_disc_numbers.get(track.get('id'), 1) - - # Get file extension from current path - file_ext = os.path.splitext(resolved or file_path or '.mp3')[1] - - # Detect quality using the same format as the download pipeline - quality = _get_audio_quality_string(resolved) if resolved else '' - - # Build context for template - year_val = album_data.get('year') or '' - context = { - 'artist': track.get('artist_name') or 'Unknown Artist', - 'albumartist': album_data.get('artist_name') or track.get('artist_name') or 'Unknown Artist', - 'album': album_data.get('title') or 'Unknown Album', - 'title': track.get('title') or 'Unknown Track', - 'track_number': track.get('track_number') or 1, - 'disc_number': disc_number, - 'total_discs': total_discs, - 'year': year_val, - 'quality': quality, - 'albumtype': _get_album_type_display( - album_data.get('record_type'), - album_data.get('track_count') or len(tracks) - ), - } - - # Build new path using the template - folder_path, filename = _get_file_path_from_template_raw(template, context) - new_relative = os.path.join(folder_path, f"{filename}{file_ext}") if folder_path else f"{filename}{file_ext}" - new_full = os.path.join(transfer_dir, new_relative) - - # Current path relative to transfer dir for display - current_display = file_path or 'No file' - if resolved and transfer_dir and resolved.startswith(transfer_dir): - current_display = resolved[len(transfer_dir):].lstrip(os.sep).lstrip('/') - - same = resolved and os.path.normpath(resolved) == os.path.normpath(new_full) - - preview_items.append({ - 'track_id': track['id'], - 'title': track.get('title', ''), - 'track_number': track.get('track_number', 0), - 'current_path': current_display, - 'new_path': new_relative, - 'new_full_normalized': os.path.normpath(new_full) if resolved else None, - 'file_exists': resolved is not None, - 'unchanged': same, - 'collision': False, - }) - - # Detect collisions: multiple tracks mapping to the same destination - seen_paths = {} - for item in preview_items: - norm = item.get('new_full_normalized') - if not norm or not item['file_exists'] or item['unchanged']: - continue - if norm in seen_paths: - item['collision'] = True - # Also mark the first one that claimed this path - seen_paths[norm]['collision'] = True - else: - seen_paths[norm] = item - - # Remove internal field from response - for item in preview_items: - item.pop('new_full_normalized', None) - - return jsonify({ - "success": True, - "album": album_data.get('title', ''), - "artist": album_data.get('artist_name', ''), - "tracks": preview_items, - "transfer_dir": transfer_dir, - }) - + result = preview_album_reorganize( + album_id=album_id, + db=get_database(), + transfer_dir=transfer_dir, + resolve_file_path_fn=_resolve_library_file_path, + build_final_path_fn=_build_final_path_for_track, + primary_source=chosen_source, + strict_source=bool(chosen_source), + ) + if result.get('status') == 'no_album': + return jsonify({"success": False, "error": "Album not found"}), 404 + if result.get('status') == 'no_tracks': + return jsonify({"success": False, "error": "No tracks found for this album"}), 404 + return jsonify(result) except Exception as e: logger.error(f"Reorganize preview error: {e}") return jsonify({"success": False, "error": str(e)}), 500 @@ -13751,274 +13672,88 @@ def reorganize_album_preview(album_id): @app.route('/api/library/album//reorganize', methods=['POST']) def reorganize_album_files(album_id): - """Move album files to new paths based on the provided template.""" + """Re-route an album's existing files through the same post-processing + pipeline downloads use. Implementation lives in + :mod:`core.library_reorganize` to keep this monolith from growing. + The request body's ``template`` (if any) is ignored — post-processing + always uses the configured template, matching the download path. + + Optional body param ``source``: when provided, only that metadata + source is used (no fallback chain). Matches the per-album source + picker in the reorganize modal.""" try: data = request.get_json() or {} - template = data.get('template', '').strip() - if not template: - return jsonify({"success": False, "error": "Template is required"}), 400 - - # Atomic check-and-set to prevent concurrent reorganizations + chosen_source = data.get('source') or None with _reorganize_lock: if _reorganize_state['status'] == 'running': return jsonify({"success": False, "error": "A reorganization is already in progress"}), 409 _reorganize_state['status'] = 'running' - - database = get_database() - conn = database._get_connection() - cursor = conn.cursor() - - # Get album + artist info - cursor.execute(""" - SELECT al.*, a.name as artist_name - FROM albums al - JOIN artists a ON al.artist_id = a.id - WHERE al.id = ? - """, (str(album_id),)) - album_row = cursor.fetchone() - if not album_row: - with _reorganize_lock: - _reorganize_state['status'] = 'idle' - return jsonify({"success": False, "error": "Album not found"}), 404 - album_data = dict(album_row) - - # Get all tracks - cursor.execute(""" - SELECT t.*, a.name as artist_name - FROM tracks t - JOIN artists a ON t.artist_id = a.id - WHERE t.album_id = ? - ORDER BY t.track_number - """, (str(album_id),)) - tracks = [dict(r) for r in cursor.fetchall()] - - if not tracks: - with _reorganize_lock: - _reorganize_state['status'] = 'idle' - return jsonify({"success": False, "error": "No tracks found"}), 404 - - transfer_dir = docker_resolve_path(config_manager.get('soulseek.transfer_path', './Transfer')) - - # Initialize state (already set to 'running' above) - with _reorganize_lock: _reorganize_state.update({ - 'total': len(tracks), - 'processed': 0, - 'moved': 0, - 'skipped': 0, - 'failed': 0, - 'current_track': '', - 'errors': [], + 'total': 0, 'processed': 0, 'moved': 0, 'skipped': 0, + 'failed': 0, 'current_track': '', 'errors': [], }) - def _run_reorganize(): - bg_conn = None + download_dir = docker_resolve_path(config_manager.get('soulseek.download_path', './downloads')) + staging_root = os.path.join(download_dir, 'ssync_staging') + try: + os.makedirs(staging_root, exist_ok=True) + except OSError: + pass + transfer_dir = docker_resolve_path(config_manager.get('soulseek.transfer_path', './Transfer')) + + def _on_progress(updates): + with _reorganize_lock: + _reorganize_state.update(updates) + + def _update_track_path(track_id, new_path): try: - # Single DB connection for the background thread - bg_db = get_database() - bg_conn = bg_db._get_connection() + _db = get_database() + with _db._get_connection() as _conn: + _conn.execute( + "UPDATE tracks SET file_path = ?, updated_at = CURRENT_TIMESTAMP WHERE id = ?", + (new_path, str(track_id)), + ) + _conn.commit() + except Exception as _db_err: + logger.warning(f"[Reorganize] DB path update failed for {track_id}: {_db_err}") - # Pre-scan disc numbers for every track so total_discs is the - # same for all template contexts in this album. Needed by the - # $cdnum template variable to decide multi-disc vs single-disc. - track_disc_numbers = {} - for _t in tracks: - _rp = _resolve_library_file_path(_t.get('file_path')) if _t.get('file_path') else None - _dn = 1 - if _rp: - try: - from core.tag_writer import read_file_tags - _dn = read_file_tags(_rp).get('disc_number') or 1 - except Exception: - _dn = 1 - track_disc_numbers[_t.get('id')] = int(_dn) - total_discs = max(track_disc_numbers.values(), default=1) if track_disc_numbers else 1 + def _cleanup_empty(src_dir): + try: + _cleanup_empty_directories(transfer_dir, os.path.join(src_dir, '_')) + except Exception: + pass - # Pre-compute all destination paths to detect collisions - dest_paths = {} # normalized_new_path -> track_id - for track in tracks: - file_path = track.get('file_path') - resolved = _resolve_library_file_path(file_path) if file_path else None - if not resolved: - continue - - # Reuse the disc number from the pre-scan pass above - disc_number = track_disc_numbers.get(track.get('id'), 1) - - file_ext = os.path.splitext(resolved)[1] - quality = _get_audio_quality_string(resolved) - - year_val = album_data.get('year') or '' - context = { - 'artist': track.get('artist_name') or 'Unknown Artist', - 'albumartist': album_data.get('artist_name') or track.get('artist_name') or 'Unknown Artist', - 'album': album_data.get('title') or 'Unknown Album', - 'title': track.get('title') or 'Unknown Track', - 'track_number': track.get('track_number') or 1, - 'disc_number': disc_number, - 'total_discs': total_discs, - 'year': year_val, - 'quality': quality, - 'albumtype': _get_album_type_display( - album_data.get('record_type'), - album_data.get('track_count') or len(tracks) - ), - } - - folder_path, filename = _get_file_path_from_template_raw(template, context) - new_relative = os.path.join(folder_path, f"{filename}{file_ext}") if folder_path else f"{filename}{file_ext}" - new_full = os.path.join(transfer_dir, new_relative) - norm_new = os.path.normpath(new_full) - - # Check for collision: two tracks mapping to same destination - if norm_new in dest_paths and dest_paths[norm_new] != str(track['id']): - # Mark as collision so the move pass skips it - track['_collision'] = True - with _reorganize_lock: - _reorganize_state['failed'] += 1 - _reorganize_state['processed'] += 1 - _reorganize_state['errors'].append({ - 'track_id': track['id'], - 'title': track.get('title', 'Unknown'), - 'error': "Path collision with another track — add $track or $disc to template" - }) - continue - - dest_paths[norm_new] = str(track['id']) - - # Store computed info on the track dict for the move pass - track['_resolved'] = resolved - track['_new_full'] = new_full - track['_disc_number'] = disc_number - - # Now do the actual moves - moved_dirs = {} # src_dir → dest_dir for post-pass sidecar sweep - for track in tracks: - resolved = track.get('_resolved') - new_full = track.get('_new_full') - track_title = track.get('title', 'Unknown') - - with _reorganize_lock: - _reorganize_state['current_track'] = track_title - - # Skip tracks already handled (collision or file not found) - if track.get('_collision'): - continue - - if not resolved or not new_full: - # File not found — only count if not already handled in pre-computation - if '_resolved' not in track: - with _reorganize_lock: - _reorganize_state['skipped'] += 1 - _reorganize_state['processed'] += 1 - _reorganize_state['errors'].append({ - 'track_id': track['id'], - 'title': track_title, - 'error': 'File not found on disk' - }) - continue - - # Skip if already at target - if os.path.normpath(resolved) == os.path.normpath(new_full): - with _reorganize_lock: - _reorganize_state['skipped'] += 1 - _reorganize_state['processed'] += 1 - continue - - try: - # Move file - _safe_move_file(resolved, new_full) - - # Track source→dest directory mapping for post-pass sidecar sweep - src_dir = os.path.dirname(resolved) - dest_dir = os.path.dirname(new_full) - if src_dir not in moved_dirs: - moved_dirs[src_dir] = dest_dir - - # Move track-level sidecars (same filename stem as audio) - src_stem = os.path.splitext(os.path.basename(resolved))[0] - new_stem = os.path.splitext(os.path.basename(new_full))[0] - for sidecar_ext in ('.lrc', '.nfo', '.txt', '.cue'): - sidecar_src = os.path.join(src_dir, src_stem + sidecar_ext) - if os.path.isfile(sidecar_src): - sidecar_dst = os.path.join(dest_dir, new_stem + sidecar_ext) - try: - shutil.move(sidecar_src, sidecar_dst) - except Exception: - pass - - # Update DB file_path - bg_cursor = bg_conn.cursor() - bg_cursor.execute( - "UPDATE tracks SET file_path = ?, updated_at = CURRENT_TIMESTAMP WHERE id = ?", - (new_full, str(track['id'])) - ) - bg_conn.commit() - - with _reorganize_lock: - _reorganize_state['moved'] += 1 - _reorganize_state['processed'] += 1 - - except Exception as move_err: - logger.error(f"Reorganize move error for {track_title}: {move_err}") - with _reorganize_lock: - _reorganize_state['failed'] += 1 - _reorganize_state['processed'] += 1 - _reorganize_state['errors'].append({ - 'track_id': track['id'], - 'title': track_title, - 'error': str(move_err) - }) - - # Post-pass: sweep source directories for leftover album-level sidecars. - # The per-track loop can't reliably move cover.jpg because multiple tracks - # share the same source dir — the first track's move may fail silently, - # or the file may be in a parent directory. This single pass catches them all. - _album_sidecars = ('cover.jpg', 'cover.jpeg', 'cover.png', 'folder.jpg', - 'folder.png', 'front.jpg', 'front.png', 'album.jpg', 'album.png') - for src_dir, dest_dir in moved_dirs.items(): - if not os.path.isdir(src_dir): - continue - # Check if any audio files remain (don't steal sidecars from a dir that still has tracks) - audio_exts = {'.flac', '.mp3', '.m4a', '.ogg', '.opus', '.wav', '.aac', '.wma'} - has_audio = any(os.path.splitext(f)[1].lower() in audio_exts - for f in os.listdir(src_dir) if os.path.isfile(os.path.join(src_dir, f))) - if has_audio: - continue - for sidecar in _album_sidecars: - sidecar_src = os.path.join(src_dir, sidecar) - if os.path.isfile(sidecar_src): - sidecar_dst = os.path.join(dest_dir, sidecar) - if not os.path.exists(sidecar_dst): - try: - shutil.move(sidecar_src, sidecar_dst) - logger.info(f"[Reorganize] Moved {sidecar} to {dest_dir}") - except Exception as sc_err: - logger.error(f"[Reorganize] Failed to move {sidecar}: {sc_err}") - - # Clean up empty directories left behind (after sidecars moved) - for src_dir in moved_dirs: - try: - _cleanup_empty_directories(transfer_dir, os.path.join(src_dir, '_')) - except Exception: - pass - - except Exception as e: - logger.error(f"Reorganize background error: {e}") + def _run(): + from core.library_reorganize import reorganize_album + try: + summary = reorganize_album( + album_id=album_id, + db=get_database(), + staging_root=staging_root, + resolve_file_path_fn=_resolve_library_file_path, + post_process_fn=_post_process_matched_download, + update_track_path_fn=_update_track_path, + cleanup_empty_dir_fn=_cleanup_empty, + transfer_dir=transfer_dir, + on_progress=_on_progress, + primary_source=chosen_source, + strict_source=bool(chosen_source), + stop_check=lambda: bool(IS_SHUTTING_DOWN), + ) + logger.info( + f"[Reorganize] Album {album_id} {summary['status']} " + f"(source={summary.get('source')}, moved={summary['moved']}, " + f"skipped={summary['skipped']}, failed={summary['failed']})" + ) + except Exception as run_err: + logger.error(f"[Reorganize] Background error: {run_err}", exc_info=True) finally: - if bg_conn: - try: - bg_conn.close() - except Exception: - pass with _reorganize_lock: _reorganize_state['status'] = 'done' _reorganize_state['current_track'] = '' - thread = threading.Thread(target=_run_reorganize, daemon=True, name="ReorganizeAlbum") - thread.start() - - return jsonify({"success": True, "message": "Reorganization started", "total": len(tracks)}) + threading.Thread(target=_run, daemon=True, name="ReorganizeAlbum").start() + return jsonify({"success": True, "message": "Reorganization started"}) except Exception as e: logger.error(f"Reorganize error: {e}") diff --git a/webui/static/helper.js b/webui/static/helper.js index e559977d..ad0d0a19 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3443,6 +3443,7 @@ const WHATS_NEW = { '2.40': [ // --- Search & Artists unification (in progress, not yet released) --- { date: 'Unreleased — Search & Artists unification', unreleased: true }, + { title: 'Library Reorganize: Reroute Through the Download Pipeline', desc: 'Reported by winecountrygames — using "Reorganize All" on a 3-disc Aerosmith deluxe collapsed it to a flat 1-disc layout, and on other albums it left half the tracks in their original location with no error or count of what was skipped. Root cause: the reorganize endpoint reinvented several wheels (its own template engine, its own disc-number resolution from file tags, its own sidecar sweep, its own collision detection) and each had drifted from the canonical post-processing path used by downloads. The reorganize-only logic read disc_number from file tags and silently defaulted to 1 on any failure, so a single tag-less file collapsed the whole album to single-disc. Tracks whose file paths didn\'t resolve on disk were silently skipped. Rewrote it to follow the import page\'s pattern: copy each file to a per-album staging folder under your download path, look up the canonical tracklist from your configured metadata source (Deezer / Spotify / iTunes / Discogs / Hydrabase) using the album\'s stored source IDs, then route each file through the same `_post_process_matched_download` function fresh downloads use — same template, same tagging, same multi-disc subfolder logic, same sidecar handling, same AcoustID verification. Albums with no stored source ID are reported back and skipped entirely (degrading silently to file tags is what caused the original bug). Tracks not in the source\'s catalog version (bonus tracks on a deluxe edition) are reported as skipped and left in place rather than force-fed wrong context. Files that don\'t resolve on disk are surfaced with the offending DB path so the UI can show them. The 230-line inline reorganize logic in web_server.py was extracted into core/library_reorganize.py — net -195 lines from the monolith, +13 unit tests for the new orchestrator. Frontend behavior change: the per-call template parameter in the reorganize modal is now ignored — reorganize uses your configured download template, matching the pipeline downloads use', page: 'library', unreleased: true }, { title: 'Spotify: Longer Post-Ban Cooldown (30 min)', desc: 'A user reported their Spotify rate-limit ban expired after 4 hours, the system ran its 5-minute post-ban cooldown, and then 32 seconds after the cooldown ended a single get_artist_albums call from a background worker was hit with another 4-hour ban. Diagnosis: Spotify\'s server-side memory of the previous offense outlasted our 5-minute cooldown, so the very first call after cooldown got slapped immediately. The cooldown exists specifically to prevent the "ban expires → we probe → re-ban" cycle, but the value was too short. Bumped from 5 minutes to 30 minutes — same mechanism, just enough room for Spotify to actually forget. A more principled follow-up (adaptive cooldown that scales with the previous ban size, plus making the first post-cooldown call a single light probe rather than allowing background workers through) is documented as a future PR if reports persist after this bump', page: 'dashboard', unreleased: true }, { title: 'Tidal: Reject Silent Quality Downgrades', desc: 'Netti93 reported that with Tidal set to "HiRes only" and quality fallback disabled, tracks were still downloading successfully — as m4a 320kbps files. Root cause: Tidal\'s API silently serves whatever tier your account + the track + your region permits. Ask for HI_RES_LOSSLESS on a track that\'s only in LOW_320K and Tidal returns the AAC stream without raising. The downloader wrote the m4a to disk, the filesize cleared the 100KB stub threshold, and the download reported success. The worker-level fallback chain (hires → lossless → high → low) also never got a chance to advance, because every tier "succeeded" at the first one that returned anything. Fix: after getting the stream, compare stream.audio_quality against what we requested using a rank-based tier comparison (LOW < HIGH < LOSSLESS < HI_RES < HI_RES_LOSSLESS). Same tier or better = accept (so occasional Tidal upgrades don\'t get thrown away). Lower tier = treat this tier as failed, which lets the fallback chain advance when fallback is enabled or fails the whole download honestly when the user has "HiRes only, no fallback" configured. Unrecognized audioQuality values (a new Tidal tier we haven\'t mapped yet) are rejected conservatively so the final diagnostic log can name the unknown value. Older tidalapi builds without the audio_quality attribute fall through to the pre-existing codec / file-size guards so nothing regresses', page: 'downloads', unreleased: true }, { title: 'Search Source Picker Icon Row', desc: 'The Search page now has a row of source icons above the search bar — one per source (Spotify, Apple Music, Deezer, Discogs, Hydrabase, MusicBrainz, Music Videos, Soulseek). Typing searches only the currently-selected source instead of fanning out to every one by default. Click a different icon to switch; results come back on demand. The default icon on page load is your configured primary metadata source. Replaces the short-lived "Search from" dropdown that preceded this', page: 'search', unreleased: true }, diff --git a/webui/static/library.js b/webui/static/library.js index 0cc1efc0..816ebda1 100644 --- a/webui/static/library.js +++ b/webui/static/library.js @@ -2805,7 +2805,7 @@ function renderArtistMetaPanel(artist) { const reorgAllBtn = document.createElement('button'); reorgAllBtn.className = 'enhanced-sync-btn'; reorgAllBtn.innerHTML = '📁 Reorganize All'; - reorgAllBtn.title = 'Reorganize all albums for this artist using path template'; + reorgAllBtn.title = 'Reorganize all albums for this artist using your configured download template'; reorgAllBtn.onclick = () => _showReorganizeAllModal(); headerRight.appendChild(reorgAllBtn); @@ -3271,7 +3271,7 @@ function renderExpandedAlbumHeader(album) { const reorganizeBtn = document.createElement('button'); reorganizeBtn.className = 'enhanced-reorganize-album-btn'; reorganizeBtn.innerHTML = '📁 Reorganize'; - reorganizeBtn.title = 'Reorganize album files using a custom path template'; + reorganizeBtn.title = 'Reorganize album files using your configured download template'; reorganizeBtn.onclick = (e) => { e.stopPropagation(); showReorganizeModal(album.id); }; enrichRow.appendChild(reorganizeBtn); @@ -6132,52 +6132,19 @@ async function showReorganizeModal(albumId) { applyBtn.onclick = () => executeReorganize(); } - // Build modal content - const variables = [ - { var: '$artist', desc: 'Track artist', example: artistName || 'Artist' }, - { var: '$albumartist', desc: 'Album artist', example: artistName || 'Album Artist' }, - { var: '$artistletter', desc: 'First letter of artist', example: (artistName || 'A')[0].toUpperCase() }, - { var: '$album', desc: 'Album title', example: albumData ? albumData.title : 'Album' }, - { var: '$albumtype', desc: 'Album/EP/Single', example: 'Album' }, - { var: '$title', desc: 'Track title', example: 'Track Name' }, - { var: '$track', desc: 'Track number (zero-padded)', example: '01' }, - { var: '$disc', desc: 'Disc number (filename only)', example: '01' }, - { var: '$cdnum', desc: 'CD label — "CD01" on multi-disc, empty otherwise', example: 'CD01' }, - { var: '$year', desc: 'Release year', example: albumData && albumData.year ? String(albumData.year) : '2024' }, - { var: '$quality', desc: 'Audio quality (filename only)', example: 'FLAC 16bit/44kHz' }, - ]; - let html = '
'; - // Template input - html += '
'; - html += ''; - html += '
Use / to separate folders. The last segment becomes the filename.
'; - // Load saved template from settings, fall back to default - let savedTemplate = '$albumartist/$albumartist - $album/$track - $title'; - try { - const settingsResp = await fetch('/api/settings'); - if (settingsResp.ok) { - const settings = await settingsResp.json(); - savedTemplate = settings.file_organization?.templates?.album_path || savedTemplate; - } - } catch (_) { } - html += ''; + html += ''; html += '
'; - // Variables reference - html += '
'; - html += ''; - html += '
'; - variables.forEach(v => { - html += `
`; - html += `${v.var}${v.desc}`; - html += '
'; - }); - html += '
'; - // Preview area html += '
'; html += '
'; @@ -6192,31 +6159,34 @@ async function showReorganizeModal(albumId) { body.innerHTML = html; overlay.classList.remove('hidden'); - // Wire up live preview on enter key - setTimeout(() => { - const input = document.getElementById('reorganize-template-input'); - if (input) { - input.addEventListener('keydown', (e) => { - if (e.key === 'Enter') { - e.preventDefault(); - loadReorganizePreview(); - } - }); - input.focus(); - } - }, 50); + // Populate source picker after the modal mounts + setTimeout(() => _populateReorganizeSources(_reorganizeAlbumId), 50); } -function insertReorganizeVar(varName) { - const input = document.getElementById('reorganize-template-input'); - if (!input) return; - const start = input.selectionStart; - const end = input.selectionEnd; - const val = input.value; - input.value = val.substring(0, start) + varName + val.substring(end); - input.focus(); - const newPos = start + varName.length; - input.setSelectionRange(newPos, newPos); +async function _populateReorganizeSources(albumId) { + const select = document.getElementById('reorganize-source-select'); + if (!select || !albumId) return; + try { + const resp = await fetch(`/api/library/album/${albumId}/reorganize/sources`); + if (!resp.ok) return; + const data = await resp.json(); + const sources = data.sources || []; + // Keep the "auto" default option, append concrete sources beneath it. + sources.forEach(s => { + const opt = document.createElement('option'); + opt.value = s.source; + opt.textContent = s.label || s.source; + select.appendChild(opt); + }); + if (sources.length === 0) { + const opt = document.createElement('option'); + opt.disabled = true; + opt.textContent = 'No sources available — run enrichment first'; + select.appendChild(opt); + } + } catch (err) { + console.error('Failed to load reorganize sources:', err); + } } function closeReorganizeModal() { @@ -6226,19 +6196,19 @@ function closeReorganizeModal() { } async function loadReorganizePreview() { - const template = document.getElementById('reorganize-template-input')?.value?.trim(); const previewBody = document.getElementById('reorganize-preview-body'); const applyBtn = document.getElementById('reorganize-apply-btn'); - if (!template || !previewBody || !_reorganizeAlbumId) return; + if (!previewBody || !_reorganizeAlbumId) return; if (applyBtn) applyBtn.disabled = true; previewBody.innerHTML = '
Loading preview...
'; try { + const chosenSource = document.getElementById('reorganize-source-select')?.value || ''; const response = await fetch(`/api/library/album/${_reorganizeAlbumId}/reorganize/preview`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ template }) + body: JSON.stringify({ source: chosenSource }) }); const result = await response.json(); if (!result.success) { @@ -6262,31 +6232,52 @@ async function loadReorganizePreview() { const unchanged = t.unchanged; const noFile = !t.file_exists; const collision = t.collision; - if (!unchanged && t.file_exists) hasChanges = true; + const unmatched = (t.matched === false); + const missingPath = !unmatched && !noFile && !t.new_path; // matched but path-build failed + if (!unchanged && t.file_exists && !unmatched && !missingPath) hasChanges = true; if (collision) hasCollisions = true; - const rowClass = collision ? 'reorganize-row-collision' : noFile ? 'reorganize-row-missing' : unchanged ? 'reorganize-row-unchanged' : 'reorganize-row-changed'; + let rowClass; + if (collision) rowClass = 'reorganize-row-collision'; + else if (noFile || unmatched || missingPath) rowClass = 'reorganize-row-missing'; + else if (unchanged) rowClass = 'reorganize-row-unchanged'; + else rowClass = 'reorganize-row-changed'; + + const arrow = collision ? '!!' + : unchanged ? '=' + : (noFile || unmatched || missingPath) ? '⊘' + : '→'; + + const newCell = noFile ? '' + : unmatched ? `${escapeHtml(t.reason || 'Not in selected source\'s tracklist')}` + : missingPath ? `${escapeHtml(t.reason || 'Couldn\'t compute destination path')}` + : (escapeHtml(t.new_path) + (collision ? ' (collision)' : '')); + html += ``; html += `${t.track_number || ''}`; html += `${escapeHtml(t.title)}`; html += `${noFile ? 'File not found' : escapeHtml(t.current_path)}`; - html += `${collision ? '!!' : unchanged ? '=' : noFile ? '' : '→'}`; - html += `${noFile ? '' : escapeHtml(t.new_path)}${collision ? ' (collision)' : ''}`; + html += `${arrow}`; + html += `${newCell}`; html += ''; }); html += ''; - const changedCount = tracks.filter(t => !t.unchanged && t.file_exists && !t.collision).length; + const changedCount = tracks.filter(t => !t.unchanged && t.file_exists && !t.collision && t.matched !== false && t.new_path).length; const skippedCount = tracks.filter(t => t.unchanged).length; const missingCount = tracks.filter(t => !t.file_exists).length; const collisionCount = tracks.filter(t => t.collision).length; + const unmatchedCount = tracks.filter(t => t.file_exists && t.matched === false).length; + const noPathCount = tracks.filter(t => t.file_exists && t.matched !== false && !t.new_path && !t.collision).length; let summary = `
`; if (changedCount > 0) summary += `${changedCount} will move`; if (skippedCount > 0) summary += `${skippedCount} unchanged`; - if (missingCount > 0) summary += `${missingCount} missing`; - if (collisionCount > 0) summary += `${collisionCount} collision${collisionCount !== 1 ? 's' : ''} — add $track or $disc to fix`; + if (unmatchedCount > 0) summary += `${unmatchedCount} not in source — try a different source`; + if (noPathCount > 0) summary += `${noPathCount} couldn't compute destination`; + if (missingCount > 0) summary += `${missingCount} missing on disk`; + if (collisionCount > 0) summary += `${collisionCount} collision${collisionCount !== 1 ? 's' : ''} — likely a source data issue`; summary += '
'; previewBody.innerHTML = summary + html; @@ -6300,8 +6291,7 @@ async function loadReorganizePreview() { } async function executeReorganize() { - const template = document.getElementById('reorganize-template-input')?.value?.trim(); - if (!template || !_reorganizeAlbumId) return; + if (!_reorganizeAlbumId) return; const applyBtn = document.getElementById('reorganize-apply-btn'); if (applyBtn) { @@ -6310,10 +6300,11 @@ async function executeReorganize() { } try { + const chosenSource = document.getElementById('reorganize-source-select')?.value || ''; const response = await fetch(`/api/library/album/${_reorganizeAlbumId}/reorganize`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ template }) + body: JSON.stringify({ source: chosenSource }) }); const result = await response.json(); if (!result.success) throw new Error(result.error); @@ -6392,23 +6383,17 @@ async function _showReorganizeAllModal() { title.textContent = `Reorganize All Albums — ${artistName}`; - // Load saved template - let savedTemplate = '$albumartist/$albumartist - $album/$track - $title'; - try { - const settingsResp = await fetch('/api/settings'); - if (settingsResp.ok) { - const settings = await settingsResp.json(); - savedTemplate = settings.file_organization?.templates?.album_path || savedTemplate; - } - } catch (_) { } - let html = '
'; - // Template input - html += '
'; - html += ''; - html += '
This template will be applied to all albums below. Use / to separate folders.
'; - html += ``; + // Source picker — applies to ALL albums in this run. Albums without + // an ID for the chosen source will be skipped at the backend with + // a clear status. Auto = use configured primary with fallback chain. + html += '
'; + html += ''; + html += '
Pick which source to read tracklists from. Albums without an ID for that source will be skipped. Reorganize uses your global download template, same as fresh downloads.
'; + html += ''; html += '
'; // Album list @@ -6434,25 +6419,37 @@ async function _showReorganizeAllModal() { } overlay.classList.remove('hidden'); + + // Populate the source dropdown from the global authed-sources endpoint + setTimeout(async () => { + const select = document.getElementById('reorganize-source-select'); + if (!select) return; + try { + const resp = await fetch('/api/library/reorganize/sources'); + if (!resp.ok) return; + const data = await resp.json(); + (data.sources || []).forEach(s => { + const opt = document.createElement('option'); + opt.value = s.source; + opt.textContent = s.label || s.source; + select.appendChild(opt); + }); + } catch (err) { + console.error('Failed to load reorganize sources:', err); + } + }, 50); } async function _executeReorganizeAll() { if (_reorganizeAllRunning) return; - const templateInput = document.getElementById('reorganize-template-input'); - const template = templateInput ? templateInput.value.trim() : ''; - if (!template) { - showToast('Template cannot be empty', 'error'); - return; - } - const albums = artistDetailPageState.enhancedData.albums || []; const total = albums.length; const artistName = artistDetailPageState.enhancedData.artist?.name || 'this artist'; const confirmed = await showConfirmDialog({ title: 'Reorganize All Albums', - message: `This will reorganize ${total} album${total !== 1 ? 's' : ''} for ${artistName} using the template:\n\n${template}\n\nFiles will be moved and renamed. This cannot be undone.`, + message: `This will reorganize ${total} album${total !== 1 ? 's' : ''} for ${artistName} using your configured download template. Files will be moved and renamed. This cannot be undone.`, confirmText: 'Reorganize All', destructive: false, }); @@ -6466,6 +6463,9 @@ async function _executeReorganizeAll() { const overlay = document.getElementById('reorganize-overlay'); if (overlay) overlay.classList.add('hidden'); + // Source picker is captured ONCE before the loop — same source for every album + const chosenSource = document.getElementById('reorganize-source-select')?.value || ''; + let succeeded = 0, failed = 0; for (let i = 0; i < total; i++) { @@ -6476,7 +6476,7 @@ async function _executeReorganizeAll() { const resp = await fetch(`/api/library/album/${album.id}/reorganize`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ template }), + body: JSON.stringify({ source: chosenSource }), }); const result = await resp.json(); if (!result.success) { diff --git a/webui/static/style.css b/webui/static/style.css index 7c7e781a..260350cd 100644 --- a/webui/static/style.css +++ b/webui/static/style.css @@ -46230,6 +46230,19 @@ textarea.enhanced-meta-field-input { .reorganize-template-input:focus { border-color: rgba(100, 149, 237, 0.5); } +/* `