From 7e175fec0237e6597aa5ae7ae602630541888060 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Thu, 18 Jun 2026 08:24:54 -0700 Subject: [PATCH 01/24] =?UTF-8?q?Cover=20art:=20a=20sequel=20digit=20glued?= =?UTF-8?q?=20to=20a=20CJK=20title=20('=E2=80=A6=E3=82=B5=E3=82=A6?= =?UTF-8?q?=E3=83=B3=E3=83=89=E3=83=88=E3=83=A9=E3=83=83=E3=82=AF2')=20now?= =?UTF-8?q?=20blocks=20the=20wrong-album=20match?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sokhi (again): downloading the base 'Mushoku Tensei S2 Original Soundtrack' embedded the cour-2 '…サウンドトラック2' cover. numeric_tokens_differ stripped titles to [a-z0-9], turning CJK into spaces — so the trailing '2' collapsed to a bare '2' that '第2期' (season 2) already supplied on BOTH sides, leaving the digit sets equal and the guard blind. Tokenise on \W (Unicode word-aware) instead, so a digit stays attached to its word ('サウンドトラック2' is its own digit-bearing token). Latin behaviour is byte-identical (Vol.4 vs Vol.4.5 etc.). Shared guard, so the art picker AND the MusicBrainz->CAA path are both fixed. Regression tests added. --- core/text/title_match.py | 13 +++++++++++-- tests/matching/test_numeric_release_guard.py | 15 +++++++++++++++ tests/metadata/test_art_lookup.py | 13 +++++++++++++ 3 files changed, 39 insertions(+), 2 deletions(-) diff --git a/core/text/title_match.py b/core/text/title_match.py index 36818fcd..8d1d1080 100644 --- a/core/text/title_match.py +++ b/core/text/title_match.py @@ -193,9 +193,18 @@ def numeric_tokens_differ(title_a: str, title_b: str) -> bool: string similarity ('Vol.4' vs 'Vol.4.5' = 0.97) and token-subset checks both wave these through, which hung volume 4.5's cover art on volume 4 (Sokhi). Shared digits on both sides ('1989' vs '1989 (Deluxe)') are - fine.""" + fine. + + Tokenises on non-word runs but KEEPS word characters of every script, so a + digit glued to a non-latin word stays its own digit-bearing token. Stripping + to [a-z0-9] turned CJK into spaces, collapsing 'サウンドトラック2' to a bare + '2' that a shared number elsewhere ('第2期' = season 2) already covered — so + 'Soundtrack' and 'Soundtrack2' both reduced to {'2'} and matched, hanging the + wrong cover (Sokhi again).""" def _digit_tokens(text: str) -> frozenset: - tokens = re.sub(r"[^a-z0-9]+", " ", (text or "").casefold()).split() + # \W is Unicode-aware for str: CJK/kana count as word chars, so a digit + # stays attached to its word instead of collapsing to a bare '2'. + tokens = re.sub(r"\W+", " ", (text or "").casefold()).split() return frozenset(t for t in tokens if any(c.isdigit() for c in t)) return _digit_tokens(title_a) != _digit_tokens(title_b) diff --git a/tests/matching/test_numeric_release_guard.py b/tests/matching/test_numeric_release_guard.py index b2ba79c8..23639b29 100644 --- a/tests/matching/test_numeric_release_guard.py +++ b/tests/matching/test_numeric_release_guard.py @@ -19,6 +19,12 @@ from core.musicbrainz_service import MusicBrainzService VOL4 = "B小町 - TVアニメ「【推しの子】」キャラクターソングCD Vol.4" VOL45 = "B小町 - TVアニメ「【推しの子】」キャラクターソングCD Vol.4.5" +# Sokhi #2: the sequel number is glued straight onto a CJK word ('…トラック2'), +# with the SAME digit already present elsewhere ('第2期' = season 2). Stripping +# to [a-z0-9] collapsed both titles to {'2'} and the wrong (cour-2) cover won. +OST = "『無職転生 〜異世界行ったら本気だす〜』 第2期 オリジナル・サウンドトラック" +OST2 = "『無職転生 〜異世界行ったら本気だす〜』 第2期 オリジナル・サウンドトラック2" + def test_helper_volume_and_sequel_differ(): assert numeric_tokens_differ(VOL4, VOL45) @@ -26,11 +32,20 @@ def test_helper_volume_and_sequel_differ(): assert numeric_tokens_differ("Now 99", "Now 100") +def test_helper_cjk_trailing_sequel_digit_differs(): + # The trailing '2' must register as a difference even though '第2期' already + # puts a '2' on both sides. + assert numeric_tokens_differ(OST, OST2) + assert numeric_tokens_differ(OST2, OST) + + def test_helper_shared_or_no_digits_match(): assert not numeric_tokens_differ("1989", "1989 (Deluxe)") assert not numeric_tokens_differ(VOL4, VOL4) assert not numeric_tokens_differ("IGOR", "IGOR (Deluxe)") assert not numeric_tokens_differ("", "") + # Same CJK album on both sides (incl. the shared 第2期) still matches. + assert not numeric_tokens_differ(OST, OST) def _service_with_results(results): diff --git a/tests/metadata/test_art_lookup.py b/tests/metadata/test_art_lookup.py index 92a77a82..335974ee 100644 --- a/tests/metadata/test_art_lookup.py +++ b/tests/metadata/test_art_lookup.py @@ -202,6 +202,19 @@ def test_album_matches_rejects_numeric_difference(): assert art_lookup._album_matches("Taylor Swift", "1989", "Taylor Swift", "1989 (Deluxe)") +def test_album_matches_rejects_cjk_trailing_sequel_digit(): + """Sokhi #2: the sequel '2' is glued straight onto a CJK word + ('…サウンドトラック2'), and '第2期' (season 2) already puts a '2' on both + sides — so the digit-strip collapsed both to {'2'} and the cour-2 + soundtrack's cover hung on the base soundtrack.""" + ART = "藤澤慶昌" + OST = "『無職転生 〜異世界行ったら本気だす〜』 第2期 オリジナル・サウンドトラック" + assert not art_lookup._album_matches(ART, OST, ART, OST + "2") + assert not art_lookup._album_matches(ART, OST + "2", ART, OST) + # The genuine base-album hit still matches (incl. its shared 第2期). + assert art_lookup._album_matches(ART, OST, ART, OST) + + # --------------------------------------------------------------------------- # build_art_lookup — caching + guarding # --------------------------------------------------------------------------- From 3b0394dbc64948240e2ea744abd973888811e0d7 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Thu, 18 Jun 2026 08:42:04 -0700 Subject: [PATCH 02/24] Metadata: a mid-enrichment crash on an art-less file no longer leaves it UNTAGGED MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sokhi: tracks occasionally land in Rockbox's 'untagged' bucket after a 'processing failed'. enhance_file_metadata saves the file with tags CLEARED up front (so stale tags never linger), then runs the failure-prone external steps (source-id embed, cover-art fetch). The core tags (album/artist/title/track from the matched context) are written to the in-memory object BEFORE those steps, but the on-disk file is still the cleared one until the final save. The #764 fix made the error handler restore ART — but gated the re-save on there being original art to restore. So a file with NO embedded art that hit a mid-enrichment crash threw away its in-memory core tags and was left on disk as the up-front clear saved it: untagged. Now the handler always persists the in-memory tags (restoring art when present), so a crash leaves a correctly-tagged file (album tag intact -> right bucket) instead of an empty one. Regression test drives the real enhance_file_metadata against an art-less FLAC. --- core/metadata/enrichment.py | 29 ++++--- tests/test_enrichment_tag_preservation.py | 100 ++++++++++++++++++++++ 2 files changed, 118 insertions(+), 11 deletions(-) create mode 100644 tests/test_enrichment_tag_preservation.py diff --git a/core/metadata/enrichment.py b/core/metadata/enrichment.py index fdd34042..e3642419 100644 --- a/core/metadata/enrichment.py +++ b/core/metadata/enrichment.py @@ -242,18 +242,25 @@ def enhance_file_metadata(file_path: str, context: dict, artist: dict, album_inf logger.warning("[Metadata Debug] Artist: %s", artist.get("name", "MISSING") if artist else "None") logger.warning("[Metadata Debug] Album info: %s", album_info.get("album_name", "MISSING") if album_info else "None") logger.error("[Metadata Debug] Traceback:\n%s", traceback.format_exc()) - # We cleared the file's art early; if the rewrite then crashed - # before re-embedding, the on-disk file (already saved cleared at - # the start) would be left art-less. Best-effort: put the original - # art back and persist it so a mid-enrichment crash never destroys - # the cover (#764). Guarded so a failure here can't mask the - # original error. + # The file was saved with tags CLEARED up front (so stale tags never + # linger), then the failure-prone enrichment ran. By the time most + # failures hit — the external source-id embed / cover-art fetch — the + # core tags (album/artist/title/track from the matched context) are + # already on the in-memory object but NOT yet on disk; the on-disk + # file is still the cleared one. Persist the in-memory tags now (and + # restore the original art too, #764) so a mid-enrichment crash leaves + # a correctly-tagged file instead of an UNTAGGED one (Sokhi: tracks + # landing in Rockbox's 'untagged' bucket after a 'processing failed'). + # + # Previously this save was gated on there being original art to + # restore, so an art-less file lost its tags entirely on any crash. + # Guarded so a failure here can't mask the original error. try: - if audio_file is not None and art_snapshot and restore_embedded_art( - audio_file, symbols, art_snapshot - ): + if audio_file is not None: + if art_snapshot: + restore_embedded_art(audio_file, symbols, art_snapshot) save_audio_file(audio_file, symbols) - logger.info("Restored original cover art after enrichment error.") + logger.info("Persisted core tags (and restored art) after enrichment error.") except Exception as restore_exc: - logger.debug("Art restore after error failed: %s", restore_exc) + logger.debug("Tag/art persist after error failed: %s", restore_exc) return False diff --git a/tests/test_enrichment_tag_preservation.py b/tests/test_enrichment_tag_preservation.py new file mode 100644 index 00000000..22127ee9 --- /dev/null +++ b/tests/test_enrichment_tag_preservation.py @@ -0,0 +1,100 @@ +"""Sokhi: tracks occasionally land 'untagged' after a processing failure. + +enhance_file_metadata clears the file's tags and saves it UP FRONT (so stale +tags never linger), then does the failure-prone enrichment (external source-id +embed, cover-art fetch) and saves again at the end. The core tags +(album/artist/title/track) come from the already-matched context and are written +to the in-memory object BEFORE those external steps — but the on-disk file is +still the cleared one until the final save. + +The #764 fix made the error handler restore ART, but it gated the re-save on +there being original art to restore. So a file with NO embedded art that hit a +mid-enrichment crash had its in-memory core tags thrown away and was left on disk +exactly as the up-front clear saved it: UNTAGGED. + +These tests run the REAL enhance_file_metadata against a REAL art-less FLAC and +assert the core tags survive a crash in the external step. +""" + +from __future__ import annotations + +import os +import tempfile +from unittest.mock import patch + +import pytest + +pytest.importorskip("mutagen") +from mutagen.flac import FLAC # noqa: E402 + +import core.metadata.enrichment as enrichment # noqa: E402 + + +class _Cfg: + def get(self, key, default=None): + return default + + +def _make_flac_no_art(path): + minimal = ( + b"fLaC" + + b"\x80\x00\x00\x22" + + b"\x00\x10\x00\x10" + + b"\x00\x00\x00\x00\x00\x00" + + b"\x0a\xc4\x42\xf0\x00\x00\x00\x00" + + b"\x00" * 16 + ) + with open(path, "wb") as f: + f.write(minimal) + FLAC(path).save() # valid FLAC, no tags, no pictures + + +@pytest.fixture +def flac_path(): + fd, path = tempfile.mkstemp(suffix=".flac") + os.close(fd) + _make_flac_no_art(path) + yield path + try: + os.remove(path) + except OSError: + pass + + +_CORE = {"title": "Yellow", "artist": "Coldplay", "album_artist": "Coldplay", + "album": "Parachutes", "track_number": 1, "total_tracks": 9, "disc_number": 1} + + +def _run(flac_path, *, metadata, embed_side_effect): + with patch.object(enrichment, "get_config_manager", return_value=_Cfg()), \ + patch.object(enrichment, "strip_all_non_audio_tags"), \ + patch.object(enrichment, "extract_source_metadata", return_value=metadata), \ + patch.object(enrichment, "embed_source_ids"), \ + patch.object(enrichment, "verify_metadata_written", return_value=True), \ + patch.object(enrichment, "embed_album_art_metadata", side_effect=embed_side_effect): + return enrichment.enhance_file_metadata( + flac_path, context={}, artist={"name": "Coldplay"}, album_info={}, + ) + + +def test_core_tags_survive_when_art_step_raises_on_artless_file(flac_path): + """The regression: art-less file + a crash in the external art step must NOT + leave the file untagged — the matched core tags must be on disk.""" + def boom(audio_file, metadata): + raise RuntimeError("art backend exploded") + + result = _run(flac_path, metadata=dict(_CORE), embed_side_effect=boom) + assert result is False # enrichment reported failure + f = FLAC(flac_path) + assert f.get("title") == ["Yellow"] # ...but core tags persisted + assert f.get("artist") == ["Coldplay"] + assert f.get("album") == ["Parachutes"] # the tag Rockbox buckets on + assert f.get("tracknumber") == ["1/9"] + + +def test_core_tags_written_on_happy_path_artless_file(flac_path): + result = _run(flac_path, metadata=dict(_CORE), embed_side_effect=lambda *a, **k: False) + assert result is True + f = FLAC(flac_path) + assert f.get("album") == ["Parachutes"] + assert f.get("artist") == ["Coldplay"] From b2162336581d4e1b4c2fa1f755b62c8f3550e6a9 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Thu, 18 Jun 2026 09:16:00 -0700 Subject: [PATCH 03/24] Library: group imported albums by canonical release id, not just the name string MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sokhi: songs in one album get mismatched cover art. Root cause is upstream of the repair jobs (which correctly apply one cover per album_id): the standalone import grouped albums by the album NAME hash (artist::album_name), so the SAME release split into multiple album rows whenever the name string drifted, and the cover-art/re-tag jobs then dressed each split row in its own art. Foundation (new imports only; existing rows untouched): a pure, seam-testable helper find_existing_soulsync_album_id() resolves the album row by precedence name-hash id -> source RELEASE id -> (title, artist). When an import carries a metadata-source album id, a differently-named import of the SAME release now unifies into one row instead of splitting. Source-column lookup is allow-listed (it's spliced into SQL) and guarded so a source without a dedicated album column (Deezer) falls through to the name match instead of breaking the import. Deliberate scope: this does NOT merge a track that genuinely matched a SINGLE (a different release id) into its parent album — that needs single->album resolution upstream and is the next step; this is the grouping substrate it will feed. 10 seam tests (canonical unify, single-vs-album stays separate, precedence, allowlist, server-source scope, missing-column fallthrough). --- core/imports/album_grouping.py | 94 ++++++++++++++++++ core/imports/side_effects.py | 27 +++--- tests/imports/test_album_grouping.py | 138 +++++++++++++++++++++++++++ 3 files changed, 247 insertions(+), 12 deletions(-) create mode 100644 core/imports/album_grouping.py create mode 100644 tests/imports/test_album_grouping.py diff --git a/core/imports/album_grouping.py b/core/imports/album_grouping.py new file mode 100644 index 00000000..f216e271 --- /dev/null +++ b/core/imports/album_grouping.py @@ -0,0 +1,94 @@ +"""Canonical album grouping for the SoulSync standalone import. + +SoulSync grouped imported tracks into albums by the album NAME string +(``_stable_soulsync_id("artist::album_name")``). That splits one release into +several album rows whenever the name string drifts between imports (case, +punctuation, ``(Deluxe Edition)`` suffixes, source-A-vs-B spelling), and every +downstream tool (Library Re-tag, Cover-Art Filler) then dresses each split row +in its own cover — so songs that belong to one album end up with different art +(Sokhi). + +This module is the pure, seam-testable heart of "group by canonical id, not +name": when an imported track carries a metadata-source RELEASE id, prefer +matching an existing album row by that id over the fragile name string, so the +SAME release always lands in ONE album row regardless of how its name was typed. + +Scope (deliberate): this unifies differently-named imports of the SAME release. +It does NOT merge a track that genuinely matched a SINGLE release (a different +release id) into its parent album — that needs single->album resolution upstream +and is a separate change. New imports only; existing rows are left untouched. + +Pure SQL-over-a-cursor; no app singletons, so it tests against an in-memory DB. +""" + +from __future__ import annotations + +from typing import Any, Optional + +# Album source-id columns this grouping may key on. An allowlist (not arbitrary +# interpolation) — the column name IS spliced into SQL, so it must be a known, +# trusted identifier. Mirrors get_library_source_id_columns()' 'album' values. +ALLOWED_ALBUM_SOURCE_COLS = frozenset({ + "spotify_album_id", + "itunes_album_id", + "deezer_id", + "soul_id", + "discogs_id", + "musicbrainz_release_id", +}) + + +def find_existing_soulsync_album_id( + cursor: Any, + *, + name_key_id: str, + artist_id: str, + album_name: str, + album_source_col: Optional[str] = None, + album_source_id: Optional[str] = None, +) -> Optional[str]: + """Resolve the existing ``soulsync`` album row a track should join, or None + (caller inserts a new row keyed by ``name_key_id``). + + Match precedence: + 1. ``name_key_id`` — the exact prior stable-name-hash id (unchanged + behaviour: a re-import with the identical name hits its own row). + 2. ``album_source_col == album_source_id`` — CANONICAL grouping: an + existing row already carrying THIS release's source id, so a + differently-named import of the same release unifies instead of + splitting. Only when the column is allow-listed and the id is non-empty. + 3. ``(title, artist_id)`` — the legacy name match (kept so nothing that + grouped before stops grouping now). + """ + cursor.execute( + "SELECT id FROM albums WHERE id = ? AND server_source = 'soulsync'", + (name_key_id,), + ) + row = cursor.fetchone() + if row: + return row[0] + + if album_source_col in ALLOWED_ALBUM_SOURCE_COLS and album_source_id: + try: + cursor.execute( + f"SELECT id FROM albums WHERE {album_source_col} = ? " + "AND server_source = 'soulsync' LIMIT 1", + (album_source_id,), + ) + row = cursor.fetchone() + if row: + return row[0] + except Exception: + # That source has no dedicated album column on this DB (e.g. Deezer + # doesn't split per-entity id columns) — fall through to the name + # match rather than break the import. Mirrors the guarded source-id + # UPDATE the caller already does on insert. + pass + + cursor.execute( + "SELECT id FROM albums WHERE title COLLATE NOCASE = ? AND artist_id = ? " + "AND server_source = 'soulsync' LIMIT 1", + (album_name, artist_id), + ) + row = cursor.fetchone() + return row[0] if row else None diff --git a/core/imports/side_effects.py b/core/imports/side_effects.py index 20fe0017..867329c3 100644 --- a/core/imports/side_effects.py +++ b/core/imports/side_effects.py @@ -565,19 +565,22 @@ def record_soulsync_library_entry(context: Dict[str, Any], artist_context: Dict[ # ── Album row: same insert-or-fill-empty-fields shape ── album_source_col = source_columns.get("album") - cursor.execute( - "SELECT id FROM albums WHERE id = ? AND server_source = 'soulsync'", - (album_id,), + # Group by CANONICAL release id when we have one (not just the name + # string), so differently-named imports of the SAME release land in + # one album row instead of splitting — which left the repair jobs + # dressing each split row in its own cover art (Sokhi). Precedence: + # name-hash id -> source release id -> (title, artist). Falls back to + # the legacy name match, so nothing that grouped before stops now. + from core.imports.album_grouping import find_existing_soulsync_album_id + existing_album_id = find_existing_soulsync_album_id( + cursor, name_key_id=album_id, artist_id=artist_id, album_name=album_name, + album_source_col=album_source_col, album_source_id=album_source_id, ) - row = cursor.fetchone() - if not row: - cursor.execute( - "SELECT id FROM albums WHERE title COLLATE NOCASE = ? AND artist_id = ? AND server_source = 'soulsync' LIMIT 1", - (album_name, artist_id), - ) - row = cursor.fetchone() - if row: - album_id = row[0] + if existing_album_id is not None: + album_id = existing_album_id + row = (album_id,) + else: + row = None if row: _fill_empty_columns( diff --git a/tests/imports/test_album_grouping.py b/tests/imports/test_album_grouping.py new file mode 100644 index 00000000..01656cc9 --- /dev/null +++ b/tests/imports/test_album_grouping.py @@ -0,0 +1,138 @@ +"""Seam tests for canonical album grouping (Sokhi: split album rows -> mixed +cover art). Drives find_existing_soulsync_album_id against a real in-memory +SQLite albums table — no app singletons, no I/O. +""" + +from __future__ import annotations + +import sqlite3 + +import pytest + +from core.imports.album_grouping import ( + find_existing_soulsync_album_id, + ALLOWED_ALBUM_SOURCE_COLS, +) + + +@pytest.fixture() +def cur(): + conn = sqlite3.connect(":memory:") + conn.execute( + """CREATE TABLE albums ( + id TEXT PRIMARY KEY, + artist_id TEXT, + title TEXT, + server_source TEXT, + spotify_album_id TEXT, + itunes_album_id TEXT, + deezer_id TEXT, + soul_id TEXT, + discogs_id TEXT, + musicbrainz_release_id TEXT + )""" + ) + yield conn.cursor() + conn.close() + + +def _add(cur, *, id, title, artist_id="art1", server_source="soulsync", **source_ids): + cols = ["id", "artist_id", "title", "server_source"] + list(source_ids) + vals = [id, artist_id, title, server_source] + list(source_ids.values()) + cur.execute( + f"INSERT INTO albums ({', '.join(cols)}) VALUES ({', '.join(['?'] * len(cols))})", + vals, + ) + + +def test_empty_db_returns_none(cur): + assert find_existing_soulsync_album_id( + cur, name_key_id="nk", artist_id="art1", album_name="Parachutes", + album_source_col="spotify_album_id", album_source_id="SP1") is None + + +def test_exact_name_hash_id_wins_first(cur): + _add(cur, id="nk", title="Parachutes") + assert find_existing_soulsync_album_id( + cur, name_key_id="nk", artist_id="art1", album_name="Parachutes") == "nk" + + +def test_canonical_source_id_unifies_differently_named_imports(cur): + # Existing row for release SP1 named "Parachutes". A second import of the + # SAME release id but a drifted name must JOIN it, not split. + _add(cur, id="existing", title="Parachutes", spotify_album_id="SP1") + got = find_existing_soulsync_album_id( + cur, name_key_id="different_hash", artist_id="art1", + album_name="Parachutes (Deluxe Edition)", + album_source_col="spotify_album_id", album_source_id="SP1") + assert got == "existing" + + +def test_different_release_id_stays_separate(cur): + # The single-vs-album case: a genuinely different release id must NOT merge + # (documents the known limit — single->album resolution is a separate step). + _add(cur, id="album_row", title="Parachutes", spotify_album_id="SP_ALBUM") + got = find_existing_soulsync_album_id( + cur, name_key_id="single_hash", artist_id="art1", album_name="Yellow", + album_source_col="spotify_album_id", album_source_id="SP_SINGLE") + assert got is None + + +def test_legacy_name_match_still_groups_without_a_source_id(cur): + _add(cur, id="byname", title="Parachutes") + got = find_existing_soulsync_album_id( + cur, name_key_id="other_hash", artist_id="art1", album_name="parachutes", + album_source_col=None, album_source_id=None) + assert got == "byname" # case-insensitive title + artist + + +def test_source_id_match_is_scoped_to_soulsync_rows(cur): + _add(cur, id="plexrow", title="Parachutes", server_source="plex", spotify_album_id="SP1") + got = find_existing_soulsync_album_id( + cur, name_key_id="nk", artist_id="art1", album_name="X", + album_source_col="spotify_album_id", album_source_id="SP1") + assert got is None # the matching row belongs to Plex, not soulsync + + +def test_non_allowlisted_column_is_ignored(cur): + # A column not on the allowlist must never be spliced into SQL. + assert "title" not in ALLOWED_ALBUM_SOURCE_COLS + _add(cur, id="row", title="Parachutes") + got = find_existing_soulsync_album_id( + cur, name_key_id="nk", artist_id="art1", album_name="nope", + album_source_col="title", album_source_id="Parachutes") + assert got is None # 'title' ignored as a source col; name 'nope' doesn't match + + +def test_empty_source_id_skips_canonical_match(cur): + _add(cur, id="row", title="Parachutes", spotify_album_id="") + got = find_existing_soulsync_album_id( + cur, name_key_id="nk", artist_id="art1", album_name="Other", + album_source_col="spotify_album_id", album_source_id="") + assert got is None + + +def test_missing_album_column_falls_through_not_raises(cur): + # Some sources (Deezer) don't have a dedicated album id column on the albums + # table; an allow-listed-but-absent column must NOT raise (it broke the whole + # import once) — it falls through to the name match. + cur.execute("CREATE TABLE albums_min (id TEXT, artist_id TEXT, title TEXT, server_source TEXT)") + cur.execute("INSERT INTO albums_min VALUES ('byname','art1','DZ Album','soulsync')") + # Point the helper at a table missing deezer_id by aliasing via a fresh cursor. + conn2 = sqlite3.connect(":memory:") + conn2.execute("CREATE TABLE albums (id TEXT, artist_id TEXT, title TEXT, server_source TEXT)") + conn2.execute("INSERT INTO albums VALUES ('byname','art1','DZ Album','soulsync')") + c2 = conn2.cursor() + got = find_existing_soulsync_album_id( + c2, name_key_id="nk", artist_id="art1", album_name="DZ Album", + album_source_col="deezer_id", album_source_id="67890") + conn2.close() + assert got == "byname" # deezer_id column absent -> fell through to name match + + +def test_musicbrainz_release_id_grouping(cur): + _add(cur, id="mbrow", title="Album", musicbrainz_release_id="mb-123") + got = find_existing_soulsync_album_id( + cur, name_key_id="nk2", artist_id="art1", album_name="Album (Remaster)", + album_source_col="musicbrainz_release_id", album_source_id="mb-123") + assert got == "mbrow" From 00b26fc5f149690c3c4b61d760ab912264f922cd Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Thu, 18 Jun 2026 09:24:06 -0700 Subject: [PATCH 04/24] Library: single->parent-album resolution core (pure selector + injected-I/O resolver) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a track matches a SINGLE release it carries the single's name/id and the canonical grouping files it apart from its album-mates -> mixed cover art (Sokhi). This re-homes it onto the album that actually contains it. The selection is a pure, CONSERVATIVE function and the lookup loop takes injected fetchers, so both are unit-testable without a live client. It only re-homes a track when a real 'album'-type release's tracklist contains that EXACT track (qualifier-tolerant) — never promotes a genuine standalone single, never guesses (a wrong promotion would mis-home a real single, the inverse bug). Fail-safe: any miss/error -> None (track stays as matched). 13 seam tests. Wiring next. --- core/imports/single_to_album.py | 124 ++++++++++++++++++++++++ tests/imports/test_single_to_album.py | 134 ++++++++++++++++++++++++++ 2 files changed, 258 insertions(+) create mode 100644 core/imports/single_to_album.py create mode 100644 tests/imports/test_single_to_album.py diff --git a/core/imports/single_to_album.py b/core/imports/single_to_album.py new file mode 100644 index 00000000..e6fc5f2d --- /dev/null +++ b/core/imports/single_to_album.py @@ -0,0 +1,124 @@ +"""Single -> parent-album resolution. + +When a track is matched to a SINGLE release (album_type 'single', the single's +name usually equal to the track title), it carries the single's name + the +single's source album id. The canonical grouping in +[core/imports/album_grouping.py] then files it under a different album row than +its album-mates, and the album-grouped repair jobs dress that row in the +single's art — songs of one album end up with different covers (Sokhi). + +This module re-homes such a track onto the ALBUM it actually belongs to, so it +carries the album's name/id and groups with the rest of the album. + +Design: the SELECTION is a pure, conservative function (no I/O), and the lookup +loop takes INJECTED fetchers, so both are unit-testable without a live metadata +client. CONSERVATIVE by intent — it only re-homes a track when a real +``album``-type release's tracklist *contains that exact track*. It never +promotes a genuine standalone single and never guesses, because a wrong +promotion would mis-home a real single onto an album (the inverse bug). +""" + +from __future__ import annotations + +import re +from typing import Any, Callable, Dict, List, Optional + +_WS = re.compile(r"\s+") +# Trailing version qualifiers that differ between a single and its album cut but +# don't change track identity (kept conservative — only the obvious ones). +_QUALIFIER = re.compile( + r"\s*[\(\[]\s*(album version|single version|radio edit|remaster(ed)?( \d{4})?)\s*[\)\]]\s*$", + re.IGNORECASE, +) + + +def _norm(s: Any) -> str: + """Lowercase, strip a trailing '(Album Version)'-style qualifier, collapse + whitespace — so 'Song' matches 'Song (Album Version)'.""" + t = str(s or "").strip().lower() + t = _QUALIFIER.sub("", t) + return _WS.sub(" ", t).strip() + + +def _get(obj: Any, *keys: str, default=None): + for k in keys: + if isinstance(obj, dict): + if obj.get(k) is not None: + return obj.get(k) + else: + v = getattr(obj, k, None) + if v is not None: + return v + return default + + +def select_parent_album(track_title: str, candidate_albums: List[Dict[str, Any]]) -> Optional[Dict[str, Any]]: + """Pick the parent ALBUM for ``track_title`` from normalized candidates, or + None. Each candidate is ``{name, album_type, tracks: [title, ...], ...}``. + + Conservative rules — a candidate qualifies ONLY when: + * it is an ``album`` release (never single / ep / compilation), and + * its name is not just the track title (that IS the single), and + * its tracklist contains the track by exact normalized title. + Returns the FIRST qualifying candidate (caller passes them in priority + order, so the result is deterministic). + """ + tgt = _norm(track_title) + if not tgt: + return None + for alb in candidate_albums or []: + if str(_get(alb, "album_type", default="album")).lower() != "album": + continue + if _norm(_get(alb, "name", "title", default="")) == tgt: + continue + tracks = _get(alb, "tracks", default=[]) or [] + if any(_norm(t) == tgt for t in tracks): + return alb + return None + + +def resolve_single_to_album( + track_title: str, + *, + fetch_album_candidates: Callable[[], List[Dict[str, Any]]], + fetch_album_tracks: Callable[[Dict[str, Any]], List[str]], + max_albums: int = 8, +) -> Optional[Dict[str, Any]]: + """Find the parent album for a single-matched track. I/O is INJECTED so this + is testable without a live client: + * ``fetch_album_candidates()`` -> the artist's ALBUM-type releases (dicts + with name/album_type/id/source), in priority order. + * ``fetch_album_tracks(album)`` -> that album's track titles. + Probes at most ``max_albums`` albums, lazily (stops at the first that + contains the track). Fail-safe: any error / no confident match -> None + (the track stays as it was matched). Returns the normalized winning album + ``{name, album_type, album_id, source, tracks}`` or None. + """ + if not _norm(track_title): + return None + try: + albums = fetch_album_candidates() or [] + except Exception: + return None + + probed = 0 + for alb in albums: + if str(_get(alb, "album_type", default="album")).lower() != "album": + continue + if probed >= max_albums: + break + probed += 1 + try: + tracks = fetch_album_tracks(alb) or [] + except Exception: + continue + normalized = { + "name": _get(alb, "name", "title", default=""), + "album_type": "album", + "album_id": _get(alb, "id", "album_id"), + "source": _get(alb, "source"), + "tracks": list(tracks), + } + if select_parent_album(track_title, [normalized]): + return normalized + return None diff --git a/tests/imports/test_single_to_album.py b/tests/imports/test_single_to_album.py new file mode 100644 index 00000000..7933b709 --- /dev/null +++ b/tests/imports/test_single_to_album.py @@ -0,0 +1,134 @@ +"""Seam tests for single -> parent-album resolution (Sokhi: single-matched track +splits from its album -> mixed cover art). The selector is pure; the resolver +takes injected fetchers, so neither needs a live metadata client. +""" + +from __future__ import annotations + +from core.imports.single_to_album import ( + select_parent_album, + resolve_single_to_album, +) + + +# ── pure selector ───────────────────────────────────────────────────────────── +def _alb(name, tracks, album_type="album", **extra): + return {"name": name, "album_type": album_type, "tracks": tracks, **extra} + + +def test_picks_album_that_contains_the_track(): + got = select_parent_album("Yellow", [ + _alb("Parachutes", ["Don't Panic", "Shiver", "Yellow", "Trouble"]), + ]) + assert got and got["name"] == "Parachutes" + + +def test_returns_none_when_no_album_contains_the_track(): + assert select_parent_album("Yellow", [ + _alb("Some Other Album", ["Track A", "Track B"]), + ]) is None + + +def test_never_promotes_onto_a_single_release(): + # The single's own release (album_type 'single', name == track) must be ignored. + assert select_parent_album("Yellow", [ + _alb("Yellow", ["Yellow"], album_type="single"), + ]) is None + + +def test_ignores_ep_and_compilation_types(): + assert select_parent_album("Yellow", [ + _alb("Yellow EP", ["Yellow", "Yellow (Live)"], album_type="ep"), + _alb("Greatest Hits", ["Yellow", "Clocks"], album_type="compilation"), + ]) is None + + +def test_skips_album_named_exactly_like_the_track(): + # An 'album' whose name IS the track title is the single dressed as an album; + # don't treat it as the parent. + assert select_parent_album("Yellow", [ + _alb("Yellow", ["Yellow"]), + ]) is None + + +def test_matches_through_album_version_qualifier(): + got = select_parent_album("Yellow", [ + _alb("Parachutes", ["Shiver", "Yellow (Album Version)"]), + ]) + assert got and got["name"] == "Parachutes" + + +def test_first_qualifying_candidate_wins_deterministically(): + got = select_parent_album("Yellow", [ + _alb("Parachutes", ["Yellow"]), + _alb("Parachutes (Deluxe)", ["Yellow", "Bonus"]), + ]) + assert got["name"] == "Parachutes" # input order = priority + + +def test_empty_title_returns_none(): + assert select_parent_album("", [_alb("Parachutes", ["Yellow"])]) is None + + +# ── injected-I/O resolver ───────────────────────────────────────────────────── +def test_resolver_finds_parent_album_lazily(): + calls = {"tracks": 0} + albums = [ + {"name": "Single Yellow", "album_type": "single", "id": "s1"}, # skipped (not album) + {"name": "Wrong Album", "album_type": "album", "id": "a1"}, + {"name": "Parachutes", "album_type": "album", "id": "a2"}, + ] + + def fetch_tracks(alb): + calls["tracks"] += 1 + return {"a1": ["Other"], "a2": ["Yellow", "Shiver"]}.get(alb["id"], []) + + got = resolve_single_to_album( + "Yellow", + fetch_album_candidates=lambda: albums, + fetch_album_tracks=fetch_tracks, + ) + assert got and got["name"] == "Parachutes" and got["album_id"] == "a2" + assert calls["tracks"] == 2 # probed a1 then a2, stopped; never probed the single + + +def test_resolver_returns_none_when_nothing_contains_track(): + got = resolve_single_to_album( + "Yellow", + fetch_album_candidates=lambda: [{"name": "X", "album_type": "album", "id": "a1"}], + fetch_album_tracks=lambda alb: ["Nope"], + ) + assert got is None + + +def test_resolver_is_failsafe_on_candidate_fetch_error(): + def boom(): + raise RuntimeError("api down") + assert resolve_single_to_album( + "Yellow", fetch_album_candidates=boom, fetch_album_tracks=lambda a: []) is None + + +def test_resolver_is_failsafe_on_track_fetch_error(): + def boom(alb): + raise RuntimeError("api down") + got = resolve_single_to_album( + "Yellow", + fetch_album_candidates=lambda: [{"name": "Parachutes", "album_type": "album", "id": "a1"}], + fetch_album_tracks=boom) + assert got is None + + +def test_resolver_caps_albums_probed(): + albums = [{"name": f"A{i}", "album_type": "album", "id": str(i)} for i in range(20)] + probed = {"n": 0} + + def fetch_tracks(alb): + probed["n"] += 1 + return ["nope"] + + resolve_single_to_album( + "Yellow", + fetch_album_candidates=lambda: albums, + fetch_album_tracks=fetch_tracks, + max_albums=5) + assert probed["n"] == 5 # never probes more than the cap From 58363ae510c6fab2f97e724b54f63af66f1bb5d0 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Thu, 18 Jun 2026 09:30:11 -0700 Subject: [PATCH 05/24] Library: wire single->album resolution into import detection (gated, fail-safe) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit detect_album_info_web gains a last-resort step: when a track matched a SINGLE with no usable album context, look up the parent ALBUM that contains it (via get_artist_albums_for_source + get_artist_album_tracks) and promote to it, so it groups with its album-mates and gets the album's cover instead of the single's. GATED behind metadata_enhancement.single_to_album (default OFF) — it's a per-import metadata lookup, so it's opt-in, matching the canonical-version pattern. Fully fail-safe: flag off, no source, or any client error/miss -> None, so the track stays exactly as matched (never worse than today). The promoted album name is forced past get_import_clean_album (which otherwise pins the single's name) so grouping + tags use the album. 4 glue seam tests added (promote-when-enabled, disabled-by-default, no-match, client-raises); 462 import-suite tests pass. --- core/imports/context.py | 88 ++++++++++++++++++++++++++- tests/imports/test_single_to_album.py | 72 ++++++++++++++++++++++ 2 files changed, 159 insertions(+), 1 deletion(-) diff --git a/core/imports/context.py b/core/imports/context.py index 024c3f36..8f80d9f1 100644 --- a/core/imports/context.py +++ b/core/imports/context.py @@ -8,6 +8,10 @@ from __future__ import annotations from typing import Any, Dict, Optional +from utils.logging_config import get_logger + +logger = get_logger("imports.context") + def _as_dict(value: Any) -> Dict[str, Any]: return value if isinstance(value, dict) else {} @@ -440,4 +444,86 @@ def detect_album_info_web(context, artist_context=None): force_album=True, ) - return None + # Last resort: the track matched a SINGLE with no usable album context — + # look up the parent ALBUM that actually contains it (gated, fail-safe). + return _resolve_single_to_parent_album(context, artist_context) + + +def _resolve_single_to_parent_album(context, artist_context): + """A single-matched track -> a promoted album_info for its parent album, or + None. GATED by ``metadata_enhancement.single_to_album`` (default OFF — it's a + per-import metadata lookup, so it's opt-in). Fail-safe: any miss/error returns + None so the track stays exactly as it was matched (never worse than today).""" + try: + from core.metadata.common import get_config_manager + if not get_config_manager().get("metadata_enhancement.single_to_album", False): + return None + except Exception: + return None + + try: + source = get_import_source(context) + track_info = get_import_track_info(context) + original_search = get_import_original_search(context) + track_title = (track_info.get("name") or original_search.get("title") or "").strip() + artist_name = (extract_artist_name(artist_context) + or get_import_clean_artist(context, default="")).strip() + if not source or not track_title or not artist_name: + return None + artist_id = str(get_import_source_ids(context).get("artist_id") or "") + + from core.metadata.album_tracks import ( + get_artist_albums_for_source, + get_artist_album_tracks, + ) + from core.imports.single_to_album import resolve_single_to_album + + def _acc(o, *ks): + for k in ks: + v = o.get(k) if isinstance(o, dict) else getattr(o, k, None) + if v: + return v + return None + + def fetch_candidates(): + albums = get_artist_albums_for_source( + source, artist_id, artist_name=artist_name, + album_type="album", limit=20) or [] + return [{"name": _acc(a, "name", "title"), + "album_type": _acc(a, "album_type") or "album", + "id": _acc(a, "id", "album_id")} for a in albums] + + def fetch_tracks(alb): + payload = get_artist_album_tracks( + str(alb.get("id") or ""), artist_name=artist_name, + album_name=alb.get("name") or "") or {} + return [(_acc(t, "title", "name", "track_name") or "") + for t in (payload.get("tracks") or [])] + + album = resolve_single_to_album( + track_title, + fetch_album_candidates=fetch_candidates, + fetch_album_tracks=fetch_tracks) + if not album or not album.get("name"): + return None + logger.info("single->album: re-homed '%s' onto parent album '%s'", + track_title, album["name"]) + promoted = build_import_album_info( + context, + album_info={ + "album_name": album["name"], + "track_number": track_info.get("track_number"), + "disc_number": track_info.get("disc_number"), + "album_image_url": "", + "confidence": 0.5, + }, + force_album=True, + ) + # build_import_album_info resolves album_name via get_import_clean_album, + # which prefers original_search.album (the SINGLE's name); override it + # with the resolved parent album so grouping + tags use the album. + promoted["album_name"] = album["name"] + return promoted + except Exception as e: + logger.debug("single->album resolution failed: %s", e) + return None diff --git a/tests/imports/test_single_to_album.py b/tests/imports/test_single_to_album.py index 7933b709..ad41f26a 100644 --- a/tests/imports/test_single_to_album.py +++ b/tests/imports/test_single_to_album.py @@ -5,10 +5,13 @@ takes injected fetchers, so neither needs a live metadata client. from __future__ import annotations +import pytest + from core.imports.single_to_album import ( select_parent_album, resolve_single_to_album, ) +from core.imports.context import detect_album_info_web # ── pure selector ───────────────────────────────────────────────────────────── @@ -132,3 +135,72 @@ def test_resolver_caps_albums_probed(): fetch_album_tracks=fetch_tracks, max_albums=5) assert probed["n"] == 5 # never probes more than the cap + + +# ── gated wiring through detect_album_info_web (config gate + client shapes) ─── +class _Cfg: + def __init__(self, on): + self._on = on + + def get(self, key, default=None): + if key == "metadata_enhancement.single_to_album": + return self._on + return default + + +_SINGLE_CTX = { + "source": "spotify", + "artist": {"id": "art1", "name": "Coldplay"}, + # album_type unset + name == track + total_tracks 1 -> is_album False, and the + # existing best-effort skips (album name == track), so the glue is reached. + "album": {"id": "s1", "name": "Yellow", "total_tracks": 1}, + "track_info": {"id": "t1", "name": "Yellow", "track_number": 7}, + "original_search_result": {"title": "Yellow", "album": "Yellow"}, +} + + +def _patch_clients(monkeypatch, albums, tracks_by_id): + monkeypatch.setattr("core.metadata.album_tracks.get_artist_albums_for_source", + lambda *a, **k: albums) + monkeypatch.setattr("core.metadata.album_tracks.get_artist_album_tracks", + lambda album_id, **k: {"tracks": tracks_by_id.get(album_id, [])}) + + +def test_glue_promotes_single_to_parent_album_when_enabled(monkeypatch): + monkeypatch.setattr("core.metadata.common.get_config_manager", lambda: _Cfg(True)) + _patch_clients( + monkeypatch, + albums=[{"name": "Yellow", "album_type": "single", "id": "s1"}, + {"name": "Parachutes", "album_type": "album", "id": "a2"}], + tracks_by_id={"a2": [{"title": "Shiver"}, {"title": "Yellow"}]}, + ) + out = detect_album_info_web(dict(_SINGLE_CTX), {"id": "art1", "name": "Coldplay"}) + assert out and out["is_album"] is True + assert out["album_name"] == "Parachutes" + assert out["track_number"] == 7 # preserved + + +def test_glue_disabled_by_default_returns_none(monkeypatch): + monkeypatch.setattr("core.metadata.common.get_config_manager", lambda: _Cfg(False)) + # Even with clients that WOULD match, the flag off => no promotion. + _patch_clients(monkeypatch, + albums=[{"name": "Parachutes", "album_type": "album", "id": "a2"}], + tracks_by_id={"a2": [{"title": "Yellow"}]}) + assert detect_album_info_web(dict(_SINGLE_CTX), {"id": "art1", "name": "Coldplay"}) is None + + +def test_glue_no_match_returns_none(monkeypatch): + monkeypatch.setattr("core.metadata.common.get_config_manager", lambda: _Cfg(True)) + _patch_clients(monkeypatch, + albums=[{"name": "Other Album", "album_type": "album", "id": "a9"}], + tracks_by_id={"a9": [{"title": "Different Song"}]}) + assert detect_album_info_web(dict(_SINGLE_CTX), {"id": "art1", "name": "Coldplay"}) is None + + +def test_glue_failsafe_when_client_raises(monkeypatch): + monkeypatch.setattr("core.metadata.common.get_config_manager", lambda: _Cfg(True)) + + def boom(*a, **k): + raise RuntimeError("spotify down") + monkeypatch.setattr("core.metadata.album_tracks.get_artist_albums_for_source", boom) + assert detect_album_info_web(dict(_SINGLE_CTX), {"id": "art1", "name": "Coldplay"}) is None From 820ff2013945ac706a398de18d270ae3606697a0 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Thu, 18 Jun 2026 09:44:17 -0700 Subject: [PATCH 06/24] Settings UI: 'Match singles to their parent album' toggle (Library > Post-Processing) Surfaces metadata_enhancement.single_to_album as a checkbox in the Post-Processing > Core Features section, next to the cover-art settings (it's about getting the right album cover). Default OFF, wired like the replaygain toggle (load '=== true', save raw .checked) since the generic data-config binding defaults a missing key to ON. Registered the default in settings.py DEFAULT_CONFIG + config.example.json. --- config/config.example.json | 3 ++- config/settings.py | 7 ++++++- webui/index.html | 7 +++++++ webui/static/settings.js | 2 ++ 4 files changed, 17 insertions(+), 2 deletions(-) diff --git a/config/config.example.json b/config/config.example.json index 0543822c..e6adf55c 100644 --- a/config/config.example.json +++ b/config/config.example.json @@ -44,7 +44,8 @@ }, "metadata_enhancement": { "enabled": true, - "embed_album_art": true + "embed_album_art": true, + "single_to_album": false }, "file_organization": { "enabled": true, diff --git a/config/settings.py b/config/settings.py index 28d1908a..fe03096e 100644 --- a/config/settings.py +++ b/config/settings.py @@ -662,7 +662,12 @@ class ConfigManager: # source whose art is smaller is skipped so the next source is # tried — stops a low-res Cover Art Archive upload from winning. # 0 disables the size gate. - "min_art_size": 1000 + "min_art_size": 1000, + # When a track matches a SINGLE release, look up the parent ALBUM + # that contains it and tag it as that album, so it groups with its + # album-mates and gets the album cover (not the single's). Off by + # default — it's an extra per-import metadata lookup. + "single_to_album": False }, "musicbrainz": { "embed_tags": True diff --git a/webui/index.html b/webui/index.html index af40adfa..43659fdb 100644 --- a/webui/index.html +++ b/webui/index.html @@ -5730,6 +5730,13 @@ Order the sources to choose whose cover art is used. The first source that has a cover wins; misses fall through to the next, and if none match, your download's own art is kept. Only sources you're connected to are shown — leave all off to keep current behavior.
+
+ + When a track matches a single release, look up the album that contains it and tag it as that album — so every song in an album gets the same (album) cover instead of some getting the single's art. Off by default: adds an extra metadata lookup per single-matched track. +
+ +
+
+ + Priority: 1.5 +
+
+
+ +
+ + +
+
+
+ 128 kbps + - + 400 kbps +
+
+
+
+
diff --git a/webui/static/settings.js b/webui/static/settings.js index 0023acf8..abfa3e80 100644 --- a/webui/static/settings.js +++ b/webui/static/settings.js @@ -1934,7 +1934,7 @@ function populateQualityProfileUI(profile) { } // Populate each quality tier - const qualities = ['flac', 'mp3_320', 'mp3_256', 'mp3_192']; + const qualities = ['flac', 'aac', 'mp3_320', 'mp3_256', 'mp3_192']; qualities.forEach(quality => { const config = profile.qualities[quality]; if (config) { @@ -2128,15 +2128,18 @@ function collectQualityProfileFromUI() { fallback_enabled: document.getElementById('quality-fallback-enabled')?.checked ?? true }; - const qualities = ['flac', 'mp3_320', 'mp3_256', 'mp3_192']; + const qualities = ['flac', 'aac', 'mp3_320', 'mp3_256', 'mp3_192']; qualities.forEach((quality, index) => { const enabled = document.getElementById(`quality-${quality}-enabled`)?.checked || false; const minSlider = document.getElementById(`${quality}-min`); const maxSlider = document.getElementById(`${quality}-max`); - // Preserve priority from the currently loaded profile instead of using array order - const existingPriority = currentQualityProfile?.qualities?.[quality]?.priority ?? (index + 1); + // Preserve priority from the currently loaded profile instead of using array order. + // AAC's default is 1.5 (above MP3, below FLAC) — not index+1 — so an upgraded + // profile that never had an aac tier still ranks it correctly on first save. + const _defaultPriority = quality === 'aac' ? 1.5 : (index + 1); + const existingPriority = currentQualityProfile?.qualities?.[quality]?.priority ?? _defaultPriority; profile.qualities[quality] = { enabled: enabled, From b04010a0370d65967dc74c1595ee659a98d6ebc0 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Thu, 18 Jun 2026 14:02:09 -0700 Subject: [PATCH 11/24] #885: repair-job scheduling is timezone-independent (Australia/Sydney loop) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit paksenkin: TZ=Australia/Sydney made the Cache Maintenance job (and any repair job) run every ~5s. Root cause: finished_at is written by SQLite CURRENT_TIMESTAMP (always UTC) but the scheduler compared it against datetime.now() (naive LOCAL), so the local↔UTC offset leaked into the elapsed time. Sydney (+11) made every job look ~11h stale -> always due -> fired every poll; the Americas (behind UTC) deflated it and masked the bug (why New_York 'worked'). Fix: compare in UTC. now = datetime.now(timezone.utc), and a new _hours_since() helper parses the naive CURRENT_TIMESTAMP string AS UTC before subtracting — so the machine timezone never affects scheduling. 5 tests incl. the literal repro (a just-run job must not be due under Australia/Sydney) and a due-detection sanity check; 41 repair-worker tests pass, ruff clean. --- core/repair_worker.py | 23 ++++++++-- tests/test_repair_scheduler_tz.py | 73 +++++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 4 deletions(-) create mode 100644 tests/test_repair_scheduler_tz.py diff --git a/core/repair_worker.py b/core/repair_worker.py index 519b04ed..1394d8b4 100644 --- a/core/repair_worker.py +++ b/core/repair_worker.py @@ -17,7 +17,7 @@ import threading import time import uuid from difflib import SequenceMatcher -from datetime import datetime, timedelta +from datetime import datetime, timedelta, timezone from typing import Any, Dict, List, Optional, Tuple from core.metadata_service import ( @@ -585,13 +585,29 @@ class RepairWorker: logger.info("Repair worker thread finished") + @staticmethod + def _hours_since(finished_at_iso: str, now_utc: datetime) -> float: + """Hours between a stored ``finished_at`` and ``now_utc``, both in UTC. + + ``finished_at`` is written by SQLite's CURRENT_TIMESTAMP, which is ALWAYS + UTC (and naive). #885: the scheduler compared it against ``datetime.now()`` + (naive LOCAL), so the local↔UTC offset leaked into the elapsed time. For a + zone AHEAD of UTC (Australia/Sydney = +11) every job looked ~11h stale and + fired every poll; behind UTC (the Americas) it just waited too long. Parse + the naive timestamp AS UTC and subtract a UTC ``now`` so scheduling is + timezone-independent.""" + dt = datetime.fromisoformat(finished_at_iso) + if dt.tzinfo is None: + dt = dt.replace(tzinfo=timezone.utc) + return (now_utc - dt).total_seconds() / 3600 + def _pick_next_job(self) -> Optional[str]: """Pick the next job to run based on staleness priority. Returns job_id of the stalest job whose interval has elapsed, or None if nothing is due. """ - now = datetime.now() + now = datetime.now(timezone.utc) best_job_id = None best_staleness = -1 @@ -613,8 +629,7 @@ class RepairWorker: continue try: - last_finished = datetime.fromisoformat(last_run['finished_at']) - elapsed_hours = (now - last_finished).total_seconds() / 3600 + elapsed_hours = self._hours_since(last_run['finished_at'], now) if elapsed_hours < interval_hours: continue # Not due yet diff --git a/tests/test_repair_scheduler_tz.py b/tests/test_repair_scheduler_tz.py new file mode 100644 index 00000000..ff869db4 --- /dev/null +++ b/tests/test_repair_scheduler_tz.py @@ -0,0 +1,73 @@ +"""#885: repair-job scheduling must be timezone-independent. + +`finished_at` is written by SQLite's CURRENT_TIMESTAMP (always UTC), but the +scheduler compared it against `datetime.now()` (naive LOCAL). With TZ=Australia/ +Sydney (UTC+11) every job looked ~11h stale and ran every poll; America/New_York +(behind UTC) masked it. The fix parses finished_at as UTC and compares against a +UTC now, so the machine timezone no longer leaks into elapsed time. +""" + +from __future__ import annotations + +import time +from datetime import datetime, timezone + +import pytest + +from core.repair_worker import RepairWorker + + +# ── pure helper ─────────────────────────────────────────────────────────────── +def test_hours_since_treats_naive_timestamp_as_utc(): + now = datetime(2026, 6, 18, 6, 0, 0, tzinfo=timezone.utc) + # SQLite CURRENT_TIMESTAMP style: UTC, no tz suffix. + assert RepairWorker._hours_since('2026-06-18 00:00:00', now) == pytest.approx(6.0) + + +def test_hours_since_handles_aware_timestamp(): + now = datetime(2026, 6, 18, 6, 0, 0, tzinfo=timezone.utc) + assert RepairWorker._hours_since('2026-06-18T00:00:00+00:00', now) == pytest.approx(6.0) + + +def test_hours_since_recent_is_near_zero(): + now = datetime(2026, 6, 18, 0, 0, 30, tzinfo=timezone.utc) + assert RepairWorker._hours_since('2026-06-18 00:00:00', now) == pytest.approx(30 / 3600, abs=1e-6) + + +# ── the #885 repro: a just-run job is never due, regardless of timezone ──────── +def _set_tz(monkeypatch, tz): + monkeypatch.setenv('TZ', tz) + try: + time.tzset() + except AttributeError: + pytest.skip('time.tzset() unavailable on this platform') + + +def test_just_run_job_not_due_under_any_timezone(monkeypatch): + w = RepairWorker.__new__(RepairWorker) + w._jobs = {'cache_evictor': object()} + monkeypatch.setattr(RepairWorker, 'get_job_config', + lambda self, jid: {'enabled': True, 'interval_hours': 6}) + # Job finished "now" in UTC (exactly how CURRENT_TIMESTAMP records it). + finished = datetime.now(timezone.utc).strftime('%Y-%m-%d %H:%M:%S') + monkeypatch.setattr(RepairWorker, '_get_last_run', + lambda self, jid: {'finished_at': finished}) + + # Australia/Sydney is the exact repro; check the Americas + UTC too. + for tz in ('Australia/Sydney', 'America/New_York', 'UTC'): + _set_tz(monkeypatch, tz) + assert w._pick_next_job() is None, f"just-run job wrongly due under TZ={tz}" + + +def test_stale_job_is_still_picked_under_sydney(monkeypatch): + # Sanity: a genuinely-overdue job IS picked (we didn't break due-detection). + w = RepairWorker.__new__(RepairWorker) + w._jobs = {'cache_evictor': object()} + monkeypatch.setattr(RepairWorker, 'get_job_config', + lambda self, jid: {'enabled': True, 'interval_hours': 6}) + # Finished ~10h ago in UTC. + old = datetime(2026, 1, 1, 0, 0, 0, tzinfo=timezone.utc).strftime('%Y-%m-%d %H:%M:%S') + monkeypatch.setattr(RepairWorker, '_get_last_run', + lambda self, jid: {'finished_at': old}) + _set_tz(monkeypatch, 'Australia/Sydney') + assert w._pick_next_job() == 'cache_evictor' From 70ea7eabf67c8b661b9e88ffe20d60132b41d54b Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Thu, 18 Jun 2026 15:05:14 -0700 Subject: [PATCH 12/24] Update style.css --- webui/static/style.css | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/webui/static/style.css b/webui/static/style.css index 6c344644..b8f35661 100644 --- a/webui/static/style.css +++ b/webui/static/style.css @@ -62563,6 +62563,10 @@ body[data-artist-source="source"] #artist-detail-page #library-artist-enhance-bt } #artist-detail-page .artist-detail-hero-bg { + /* Blurred artist-cover backdrop retired — Boulder didn't want the artist + image bleeding behind the header. The element stays in the DOM (JS still + sets its background-image) but is hidden; flip display back on to restore. */ + display: none; position: absolute; inset: -20px; background-size: cover; From dbd8278a144c379c0dba841d8e8ef32d82d94835 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Thu, 18 Jun 2026 15:15:41 -0700 Subject: [PATCH 13/24] #889 Phase 1: re-identify hint store (DB table + pure create/find/consume seam) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A single-use, user-designated 'which release does this track belong to' answer. Written when the user picks a release in the Re-identify modal and the file is staged; the import flow will read it at the top of matching and consume it. - rematch_hints table (additive, IF NOT EXISTS + indexes) keyed on staged_path with content_hash as a rename-proof fallback. - core/imports/rematch_hints.py: pure DB seam over an injected cursor (create/find/consume/list) + a cheap size+head+tail file fingerprint. - exempt_dedup baked into the hint (a re-identify must bypass dedup-skip); replace_track_id carried for deferred post-success cleanup. Inert until wired (Phase 5) — nothing calls it yet. 9 seam tests. --- core/imports/rematch_hints.py | 226 ++++++++++++++++++++++++++++ database/music_database.py | 35 +++++ tests/imports/test_rematch_hints.py | 155 +++++++++++++++++++ 3 files changed, 416 insertions(+) create mode 100644 core/imports/rematch_hints.py create mode 100644 tests/imports/test_rematch_hints.py diff --git a/core/imports/rematch_hints.py b/core/imports/rematch_hints.py new file mode 100644 index 00000000..6804e788 --- /dev/null +++ b/core/imports/rematch_hints.py @@ -0,0 +1,226 @@ +"""Re-identify hints (#889) — a single-use, user-designated answer to "which +release does this already-imported track belong to". + +Flow: the user clicks *Re-identify* on a library track, searches a source, and +picks the exact release (single / EP / album) it should live under. We write a +**hint** here and stage the file for auto-import. The import flow then reads the +hint at the very TOP of matching — before any fuzzy tier — builds the match from +these exact IDs, and consumes the row. So the original ambiguity that mis-filed +the track (which release?) is gone: the user already answered it. + +Two safety properties live in the hint, not the import code: + +- ``replace_track_id`` — the library row to delete AFTER the re-import lands (so a + re-identify *replaces* rather than *duplicates*). Cleanup is deferred to success + so a failed import can never lose the file. +- ``exempt_dedup`` — always set: a re-identify is an explicit user action and must + not be silently dropped by the quality dedup-skip (which would otherwise see the + incoming file as a duplicate of the very row we're replacing). + +This module is pure DB mechanics over an injected ``cursor`` (sqlite3-style, +``?`` params) — no connection management, no app state — so the create / find / +consume seam is unit-tested against an in-memory DB with no live metadata client. +The binding is keyed on the staged path, with ``content_hash`` as a rename-proof +fallback in case the staging watcher normalizes the filename on ingest. +""" + +from __future__ import annotations + +import os +from dataclasses import dataclass +from typing import Any, Optional + +# Columns in INSERT/SELECT order — single source of truth so the dataclass, the +# write, and the read can't drift apart. +_FIELDS = ( + "staged_path", + "content_hash", + "source", + "isrc", + "track_id", + "album_id", + "artist_id", + "track_title", + "album_name", + "artist_name", + "album_type", + "track_number", + "disc_number", + "replace_track_id", + "exempt_dedup", +) + + +@dataclass +class RematchHint: + """One user-designated re-identify answer. ``id``/``status`` are set by the DB.""" + staged_path: str + source: str + content_hash: Optional[str] = None + isrc: Optional[str] = None + track_id: Optional[str] = None + album_id: Optional[str] = None + artist_id: Optional[str] = None + track_title: Optional[str] = None + album_name: Optional[str] = None + artist_name: Optional[str] = None + album_type: Optional[str] = None + track_number: Optional[int] = None + disc_number: Optional[int] = None + replace_track_id: Optional[int] = None + exempt_dedup: bool = True + id: Optional[int] = None + status: str = "pending" + + def _values(self) -> tuple: + return ( + self.staged_path, + self.content_hash, + self.source, + self.isrc, + self.track_id, + self.album_id, + self.artist_id, + self.track_title, + self.album_name, + self.artist_name, + self.album_type, + self.track_number, + self.disc_number, + self.replace_track_id, + 1 if self.exempt_dedup else 0, + ) + + +def _row_to_hint(row: Any) -> RematchHint: + """Map a sqlite3.Row (or any mapping/sequence-by-name) to a RematchHint.""" + def g(key, default=None): + try: + return row[key] + except (KeyError, IndexError, TypeError): + return default + return RematchHint( + id=g("id"), + staged_path=g("staged_path") or "", + content_hash=g("content_hash"), + source=g("source") or "", + isrc=g("isrc"), + track_id=g("track_id"), + album_id=g("album_id"), + artist_id=g("artist_id"), + track_title=g("track_title"), + album_name=g("album_name"), + artist_name=g("artist_name"), + album_type=g("album_type"), + track_number=g("track_number"), + disc_number=g("disc_number"), + replace_track_id=g("replace_track_id"), + exempt_dedup=bool(g("exempt_dedup", 1)), + status=g("status") or "pending", + ) + + +def create_hint(cursor: Any, hint: RematchHint) -> int: + """Insert a pending hint; return its new id. Caller owns commit.""" + placeholders = ", ".join("?" for _ in _FIELDS) + cursor.execute( + f"INSERT INTO rematch_hints ({', '.join(_FIELDS)}) VALUES ({placeholders})", + hint._values(), + ) + new_id = cursor.lastrowid + hint.id = new_id + return new_id + + +def find_hint_for_file( + cursor: Any, + staged_path: str, + content_hash: Optional[str] = None, +) -> Optional[RematchHint]: + """Return the newest PENDING hint for a staged file, or ``None``. + + Matched by exact ``staged_path`` first; if that misses and a ``content_hash`` + is given, fall back to it (covers a staging watcher that renamed the file on + ingest). Only ``status='pending'`` rows are returned, so a consumed hint is + never reused.""" + if staged_path: + cursor.execute( + "SELECT * FROM rematch_hints WHERE staged_path = ? AND status = 'pending' " + "ORDER BY id DESC LIMIT 1", + (staged_path,), + ) + row = cursor.fetchone() + if row is not None: + return _row_to_hint(row) + # Try by basename too — the watcher may move the file into a different dir. + base = os.path.basename(staged_path) + if base and base != staged_path: + cursor.execute( + "SELECT * FROM rematch_hints WHERE staged_path LIKE ? AND status = 'pending' " + "ORDER BY id DESC LIMIT 1", + ("%/" + base,), + ) + row = cursor.fetchone() + if row is not None: + return _row_to_hint(row) + + if content_hash: + cursor.execute( + "SELECT * FROM rematch_hints WHERE content_hash = ? AND status = 'pending' " + "ORDER BY id DESC LIMIT 1", + (content_hash,), + ) + row = cursor.fetchone() + if row is not None: + return _row_to_hint(row) + + return None + + +def consume_hint(cursor: Any, hint_id: int) -> None: + """Mark a hint consumed (single-use). Caller owns commit.""" + cursor.execute( + "UPDATE rematch_hints SET status = 'consumed', consumed_at = CURRENT_TIMESTAMP " + "WHERE id = ?", + (hint_id,), + ) + + +def list_pending_hints(cursor: Any) -> list: + """All pending hints (newest first) — for a 'pending re-identify' view and + orphan recovery when a staged file never imports.""" + cursor.execute("SELECT * FROM rematch_hints WHERE status = 'pending' ORDER BY id DESC") + return [_row_to_hint(r) for r in cursor.fetchall()] + + +def quick_file_signature(path: str, *, chunk: int = 65536) -> Optional[str]: + """A cheap, rename-proof content fingerprint: size + first/last chunk, hashed. + + Audio files are large, so a full hash is wasteful when we only need to re-bind + a hint to *this* file after a possible rename. Size + head + tail is plenty to + distinguish staged files in practice. Returns ``None`` if the file can't be + read (caller falls back to path-only binding).""" + import hashlib + + try: + size = os.path.getsize(path) + h = hashlib.sha256() + h.update(str(size).encode()) + with open(path, "rb") as f: + h.update(f.read(chunk)) + if size > chunk: + f.seek(max(0, size - chunk)) + h.update(f.read(chunk)) + return h.hexdigest() + except OSError: + return None + + +__all__ = [ + "RematchHint", + "create_hint", + "find_hint_for_file", + "consume_hint", + "list_pending_hints", + "quick_file_signature", +] diff --git a/database/music_database.py b/database/music_database.py index 7f50d875..8691e8d1 100644 --- a/database/music_database.py +++ b/database/music_database.py @@ -750,6 +750,41 @@ class MusicDatabase: cursor.execute("CREATE INDEX IF NOT EXISTS idx_aih_status ON auto_import_history (status)") cursor.execute("CREATE INDEX IF NOT EXISTS idx_aih_folder_hash ON auto_import_history (folder_hash)") + # Re-identify hints (#889) — a user-designated, single-use answer to "which + # release does this track belong to". Written when the user picks a release in + # the Re-identify modal and the file is staged for auto-import; the import flow + # reads the hint at the TOP of matching (keyed by staged path, content_hash as a + # rename-proof fallback), expedites the match to these exact IDs, then consumes + # the row. `replace_track_id` (when set) is the library row to delete AFTER the + # re-import lands; `exempt_dedup` is always 1 because a re-identify is an explicit + # user action that must not be silently dropped by the quality dedup-skip. + cursor.execute(""" + CREATE TABLE IF NOT EXISTS rematch_hints ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + staged_path TEXT NOT NULL, + content_hash TEXT, + source TEXT NOT NULL, + isrc TEXT, + track_id TEXT, + album_id TEXT, + artist_id TEXT, + track_title TEXT, + album_name TEXT, + artist_name TEXT, + album_type TEXT, + track_number INTEGER, + disc_number INTEGER, + replace_track_id INTEGER, + exempt_dedup INTEGER NOT NULL DEFAULT 1, + status TEXT NOT NULL DEFAULT 'pending', + created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, + consumed_at TIMESTAMP + ) + """) + cursor.execute("CREATE INDEX IF NOT EXISTS idx_rmh_staged_path ON rematch_hints (staged_path)") + cursor.execute("CREATE INDEX IF NOT EXISTS idx_rmh_content_hash ON rematch_hints (content_hash)") + cursor.execute("CREATE INDEX IF NOT EXISTS idx_rmh_status ON rematch_hints (status)") + # Sync history table — tracks the last 100 sync operations with cached context for re-trigger cursor.execute(""" CREATE TABLE IF NOT EXISTS sync_history ( diff --git a/tests/imports/test_rematch_hints.py b/tests/imports/test_rematch_hints.py new file mode 100644 index 00000000..6e1d08de --- /dev/null +++ b/tests/imports/test_rematch_hints.py @@ -0,0 +1,155 @@ +"""#889 Phase 1: the re-identify hint store — create / find / consume. + +The hint is the single-use, user-designated "which release" answer the import +flow reads at the top of matching. These lock down: a hint round-trips, it's +found by staged path, found by content_hash when the path missed (rename-proof), +found by basename when the dir changed, consumed exactly once, and that a +consumed hint is never handed back. +""" + +from __future__ import annotations + +import sqlite3 + +import pytest + +from core.imports.rematch_hints import ( + RematchHint, + consume_hint, + create_hint, + find_hint_for_file, + list_pending_hints, + quick_file_signature, +) + +# The slice of the real schema this module touches (kept in sync with +# database/music_database.py's rematch_hints CREATE). +_SCHEMA = """ +CREATE TABLE rematch_hints ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + staged_path TEXT NOT NULL, + content_hash TEXT, + source TEXT NOT NULL, + isrc TEXT, + track_id TEXT, + album_id TEXT, + artist_id TEXT, + track_title TEXT, + album_name TEXT, + artist_name TEXT, + album_type TEXT, + track_number INTEGER, + disc_number INTEGER, + replace_track_id INTEGER, + exempt_dedup INTEGER NOT NULL DEFAULT 1, + status TEXT NOT NULL DEFAULT 'pending', + created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, + consumed_at TIMESTAMP +) +""" + + +@pytest.fixture +def cur(): + conn = sqlite3.connect(":memory:") + conn.row_factory = sqlite3.Row + conn.executescript(_SCHEMA) + yield conn.cursor() + conn.close() + + +def _hint(**kw): + base = dict( + staged_path="/staging/Song.flac", + source="spotify", + isrc="USABC1234567", + track_id="trk_1", + album_id="alb_album1", + artist_id="art_1", + track_title="Song", + album_name="Album1", + artist_name="Artist", + album_type="album", + track_number=5, + disc_number=1, + replace_track_id=42, + ) + base.update(kw) + return RematchHint(**base) + + +def test_create_and_find_by_path_roundtrips(cur): + new_id = create_hint(cur, _hint()) + assert new_id > 0 + got = find_hint_for_file(cur, "/staging/Song.flac") + assert got is not None + assert got.id == new_id + assert got.album_id == "alb_album1" and got.album_type == "album" + assert got.isrc == "USABC1234567" + assert got.track_number == 5 and got.disc_number == 1 + assert got.replace_track_id == 42 + assert got.exempt_dedup is True # always set for a user-designated re-identify + assert got.status == "pending" + + +def test_find_by_content_hash_when_path_missed(cur): + create_hint(cur, _hint(content_hash="deadbeef")) + # Watcher renamed/moved the file → path lookup misses, hash rescues it. + assert find_hint_for_file(cur, "/totally/different.flac") is None + got = find_hint_for_file(cur, "/totally/different.flac", content_hash="deadbeef") + assert got is not None and got.album_name == "Album1" + + +def test_find_by_basename_when_dir_changed(cur): + create_hint(cur, _hint(staged_path="/staging/in/Song.flac")) + # Same filename, different directory (watcher moved it deeper). + got = find_hint_for_file(cur, "/staging/processing/Song.flac") + assert got is not None and got.track_id == "trk_1" + + +def test_consume_is_single_use(cur): + new_id = create_hint(cur, _hint()) + assert find_hint_for_file(cur, "/staging/Song.flac") is not None + consume_hint(cur, new_id) + # Consumed → never handed back, by path or by hash. + assert find_hint_for_file(cur, "/staging/Song.flac") is None + assert find_hint_for_file(cur, "/x", content_hash=None) is None + + +def test_list_pending_excludes_consumed(cur): + a = create_hint(cur, _hint(staged_path="/staging/A.flac")) + create_hint(cur, _hint(staged_path="/staging/B.flac")) + assert len(list_pending_hints(cur)) == 2 + consume_hint(cur, a) + pend = list_pending_hints(cur) + assert len(pend) == 1 and pend[0].staged_path == "/staging/B.flac" + + +def test_newest_pending_wins_on_duplicate_path(cur): + create_hint(cur, _hint(album_id="alb_old")) + create_hint(cur, _hint(album_id="alb_new")) # user re-picked for the same file + got = find_hint_for_file(cur, "/staging/Song.flac") + assert got.album_id == "alb_new" + + +def test_exempt_dedup_false_roundtrips(cur): + create_hint(cur, _hint(staged_path="/staging/Keep.flac", exempt_dedup=False)) + got = find_hint_for_file(cur, "/staging/Keep.flac") + assert got.exempt_dedup is False + + +# ── content fingerprint ─────────────────────────────────────────────────────── +def test_quick_file_signature_stable_and_distinct(tmp_path): + a = tmp_path / "a.bin" + b = tmp_path / "b.bin" + a.write_bytes(b"hello world" * 1000) + b.write_bytes(b"goodbye moon" * 1000) + sig_a1 = quick_file_signature(str(a)) + sig_a2 = quick_file_signature(str(a)) + sig_b = quick_file_signature(str(b)) + assert sig_a1 and sig_a1 == sig_a2 # stable + assert sig_a1 != sig_b # distinct content → distinct sig + + +def test_quick_file_signature_missing_file_is_none(): + assert quick_file_signature("/no/such/file.flac") is None From 08fb21fb1376b288e9d7de7c1f99905f2307151f Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Thu, 18 Jun 2026 15:25:37 -0700 Subject: [PATCH 14/24] =?UTF-8?q?#889=20Phase=202:=20import=20seam=20?= =?UTF-8?q?=E2=80=94=20hint=20short-circuits=20identification,=20replaces?= =?UTF-8?q?=20on=20success?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a staged single-file candidate carries a re-identify hint, the worker builds the identification straight from the user-chosen release (album_id/source) and skips the guessing tiers — so the ambiguity that mis-filed the track is gone. No hint → byte-identical to before (the lookup returns (None, None), fail-safe on any DB error). A hinted import auto-processes (explicit user choice), still gated on the global auto_process pref. After the re-import lands, _finalize_rematch_hint consumes the hint and (if replace was chosen) deletes the old row + file via delete_replaced_track — deferred to success so a failed import never loses the original. Safe by construction: unlink only when no surviving row references the file, and the modal never offers the track's current release so old path != new path. All hint logic lives in auto_import_worker.py + the pure rematch_hints helpers — pipeline.py / side_effects.py untouched. 18 tests; full auto-import suite green. --- core/auto_import_worker.py | 75 +++++++++- core/imports/rematch_hints.py | 61 ++++++++ tests/imports/test_rematch_hints_seam.py | 168 +++++++++++++++++++++++ 3 files changed, 301 insertions(+), 3 deletions(-) create mode 100644 tests/imports/test_rematch_hints_seam.py diff --git a/core/auto_import_worker.py b/core/auto_import_worker.py index 8bfbea34..c3f85603 100644 --- a/core/auto_import_worker.py +++ b/core/auto_import_worker.py @@ -660,8 +660,13 @@ class AutoImportWorker: auto_process = self._config_manager.get('auto_import.auto_process', True) try: - # Phase 3: Identify - identification = self._identify_folder(candidate) + # Phase 3: Identify. + # Re-identify (#889): if the user designated this exact file's release in + # the Re-identify modal, a hint short-circuits the guessing — we match + # straight against the chosen album. No hint → byte-identical to before. + rematch_hint, identification = self._resolve_rematch_hint(candidate) + if identification is None: + identification = self._identify_folder(candidate) if not identification: self._record_result(candidate, 'needs_identification', 0.0, error_message='Could not identify album from tags, folder name, or fingerprint') @@ -690,7 +695,10 @@ class AutoImportWorker: high_conf_matches = [m for m in match_result.get('matches', []) if m['confidence'] >= 0.8] has_strong_individual_matches = len(high_conf_matches) > 0 - if (confidence >= threshold or has_strong_individual_matches) and auto_process: + # A re-identify is an explicit user choice — let it auto-process like a + # strong match (still gated on the global auto_process preference). + if (confidence >= threshold or has_strong_individual_matches + or rematch_hint is not None) and auto_process: # Phase 5: Auto-process — insert an in-progress row # so the UI sees the import the moment it starts, # then update it with the final status when done. @@ -709,6 +717,11 @@ class AutoImportWorker: confidence = max(confidence, effective_conf) if success: self._bump_stat('auto_processed') + # Re-identify (#889): only NOW that the new home exists do we + # consume the hint and (if replace was chosen) delete the old + # row + file — so a failed import never loses the original. + if rematch_hint is not None: + self._finalize_rematch_hint(rematch_hint) else: self._bump_stat('failed') @@ -1003,6 +1016,62 @@ class AutoImportWorker: except Exception: return False + # ── Re-identify hints (#889) ── + + def _resolve_rematch_hint(self, candidate: 'FolderCandidate'): + """If this staged file carries a user-designated re-identify hint, return + ``(hint, identification)`` so matching skips the guessing tiers; otherwise + ``(None, None)`` and the caller falls back to normal identification. + + Fail-safe: ANY error (no table, DB hiccup) returns ``(None, None)`` so a + re-identify problem can never break ordinary auto-import. Only single-file + candidates are eligible — a re-identify always stages exactly one track.""" + try: + files = candidate.audio_files or [] + if len(files) != 1: + return None, None + from core.imports.rematch_hints import ( + build_identification_from_hint, + find_hint_for_file, + quick_file_signature, + ) + file_path = files[0] + sig = quick_file_signature(file_path) + conn = self.database._get_connection() + try: + cursor = conn.cursor() + hint = find_hint_for_file(cursor, file_path, sig) + finally: + conn.close() + if hint is None: + return None, None + logger.info("[Auto-Import] Re-identify hint for %s → %s '%s' (%s)", + candidate.name, hint.album_type or 'release', + hint.album_name or '?', hint.source) + return hint, build_identification_from_hint(hint) + except Exception as e: + logger.debug("[Auto-Import] rematch-hint lookup skipped: %s", e) + return None, None + + def _finalize_rematch_hint(self, hint) -> None: + """Post-success: delete the replaced library row + file (if the user chose + replace) and consume the hint so it's single-use. Best-effort — a cleanup + failure is logged, never raised, since the re-import already succeeded.""" + try: + from core.imports.rematch_hints import consume_hint, delete_replaced_track + conn = self.database._get_connection() + try: + cursor = conn.cursor() + removed = delete_replaced_track(cursor, hint.replace_track_id) + consume_hint(cursor, hint.id) + conn.commit() + finally: + conn.close() + if removed: + logger.info("[Auto-Import] Re-identify replaced old track — removed %s", removed) + except Exception as e: + logger.warning("[Auto-Import] rematch-hint finalize failed (import still OK): %s", e) + # ── Identification ── def _identify_folder(self, candidate: FolderCandidate) -> Optional[Dict]: diff --git a/core/imports/rematch_hints.py b/core/imports/rematch_hints.py index 6804e788..2a75e66a 100644 --- a/core/imports/rematch_hints.py +++ b/core/imports/rematch_hints.py @@ -193,6 +193,65 @@ def list_pending_hints(cursor: Any) -> list: return [_row_to_hint(r) for r in cursor.fetchall()] +def build_identification_from_hint(hint: RematchHint) -> dict: + """Turn a hint into the ``identification`` dict the auto-import matcher expects, + so a re-identify SKIPS the guessing tiers entirely and matches straight against + the user-chosen release. Mirrors the shape `_identify_folder` returns (album_id + / source / track_number drive the album fetch + file→track match).""" + return { + "album_id": hint.album_id or None, + "album_name": hint.album_name or hint.track_title or "", + "artist_name": hint.artist_name or "", + "artist_id": hint.artist_id or "", + "track_name": hint.track_title or "", + "track_id": hint.track_id or "", + "image_url": "", + "release_date": "", + "track_number": hint.track_number or 1, + "total_tracks": 1, + "source": hint.source, + "method": "rematch_hint", + "identification_confidence": 1.0, + "is_single": True, + "album_type": hint.album_type, + } + + +def delete_replaced_track(cursor: Any, replace_track_id: Any, *, unlink=os.remove) -> Optional[str]: + """Remove the OLD library row a re-identify replaces, and its file. + + Called only AFTER the re-import has landed the track at its new home, so the + original is never lost on failure. Safe by construction: the file is unlinked + only if it still exists and **no other track row references it** (guards against + yanking a file a different row legitimately points to). Returns the path it + removed, or ``None`` if there was nothing to do. ``unlink`` is injectable for + tests. Assumes the new home differs from the old — the Re-identify modal never + offers the track's current release as a target, so this holds.""" + if not replace_track_id: + return None + cursor.execute("SELECT file_path FROM tracks WHERE id = ?", (replace_track_id,)) + row = cursor.fetchone() + if row is None: + return None + old_path = (row["file_path"] if not isinstance(row, (tuple, list)) else row[0]) or "" + + cursor.execute("DELETE FROM tracks WHERE id = ?", (replace_track_id,)) + + if not old_path: + return None + # Only unlink if no surviving row still points at this file. + cursor.execute("SELECT 1 FROM tracks WHERE file_path = ? LIMIT 1", (old_path,)) + if cursor.fetchone() is not None: + return None + try: + if os.path.exists(old_path): + unlink(old_path) + return old_path + except OSError: + pass + return None + + def quick_file_signature(path: str, *, chunk: int = 65536) -> Optional[str]: """A cheap, rename-proof content fingerprint: size + first/last chunk, hashed. @@ -222,5 +281,7 @@ __all__ = [ "find_hint_for_file", "consume_hint", "list_pending_hints", + "build_identification_from_hint", + "delete_replaced_track", "quick_file_signature", ] diff --git a/tests/imports/test_rematch_hints_seam.py b/tests/imports/test_rematch_hints_seam.py new file mode 100644 index 00000000..58d479fd --- /dev/null +++ b/tests/imports/test_rematch_hints_seam.py @@ -0,0 +1,168 @@ +"""#889 Phase 2: the import seam — a hint short-circuits identification, and the +old library row is replaced only after the re-import succeeds. + +Two layers: + * pure helpers (build_identification_from_hint, delete_replaced_track) — exact + mapping + safe replacement against an in-memory DB, injectable unlink. + * the worker seam (_resolve_rematch_hint / _finalize_rematch_hint) — proves the + NO-HINT path is untouched, the hint path returns a ready identification, the + lookup is fail-safe, and finalize consumes + replaces. +""" + +from __future__ import annotations + +import sqlite3 +import types + +import pytest + +from core.auto_import_worker import AutoImportWorker, FolderCandidate +from core.imports.rematch_hints import ( + RematchHint, + build_identification_from_hint, + consume_hint, + create_hint, + delete_replaced_track, + find_hint_for_file, +) + +_SCHEMA = """ +CREATE TABLE rematch_hints ( + id INTEGER PRIMARY KEY AUTOINCREMENT, staged_path TEXT NOT NULL, content_hash TEXT, + source TEXT NOT NULL, isrc TEXT, track_id TEXT, album_id TEXT, artist_id TEXT, + track_title TEXT, album_name TEXT, artist_name TEXT, album_type TEXT, + track_number INTEGER, disc_number INTEGER, replace_track_id INTEGER, + exempt_dedup INTEGER NOT NULL DEFAULT 1, status TEXT NOT NULL DEFAULT 'pending', + created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, consumed_at TIMESTAMP +); +CREATE TABLE tracks ( + id INTEGER PRIMARY KEY, album_id INTEGER, artist_id INTEGER, title TEXT, + track_number INTEGER, file_path TEXT +); +""" + + +@pytest.fixture +def conn(): + c = sqlite3.connect(":memory:") + c.row_factory = sqlite3.Row + c.executescript(_SCHEMA) + yield c + c.close() + + +def _hint(**kw): + base = dict(staged_path="/staging/Song.flac", source="spotify", album_id="alb_album1", + artist_id="art_1", track_id="trk_1", track_title="Song", album_name="Album1", + artist_name="Artist", album_type="album", track_number=5, disc_number=1) + base.update(kw) + return RematchHint(**base) + + +# ── pure: identification mapping ────────────────────────────────────────────── +def test_build_identification_maps_hint_fields(): + ident = build_identification_from_hint(_hint()) + assert ident["album_id"] == "alb_album1" + assert ident["source"] == "spotify" + assert ident["album_name"] == "Album1" + assert ident["artist_id"] == "art_1" + assert ident["track_number"] == 5 + assert ident["method"] == "rematch_hint" + assert ident["identification_confidence"] == 1.0 + assert ident["is_single"] is True + + +# ── pure: safe replacement ──────────────────────────────────────────────────── +def test_delete_replaced_track_removes_row_and_file(conn): + cur = conn.cursor() + cur.execute("INSERT INTO tracks (id, file_path) VALUES (7, '/lib/EP1/05 - Song.flac')") + removed = [] + out = delete_replaced_track(cur, 7, unlink=lambda p: removed.append(p)) + assert out == "/lib/EP1/05 - Song.flac" + assert removed == ["/lib/EP1/05 - Song.flac"] # file removed (we faked existence below) + cur.execute("SELECT 1 FROM tracks WHERE id = 7") + assert cur.fetchone() is None # row gone + + +def test_delete_replaced_track_keeps_file_if_another_row_points_at_it(conn): + cur = conn.cursor() + cur.execute("INSERT INTO tracks (id, file_path) VALUES (7, '/lib/shared.flac')") + cur.execute("INSERT INTO tracks (id, file_path) VALUES (8, '/lib/shared.flac')") + removed = [] + out = delete_replaced_track(cur, 7, unlink=lambda p: removed.append(p)) + assert out is None and removed == [] # row 8 still references it → no unlink + cur.execute("SELECT 1 FROM tracks WHERE id = 7") + assert cur.fetchone() is None # but row 7 still deleted + + +def test_delete_replaced_track_noops_on_missing_id(conn): + cur = conn.cursor() + assert delete_replaced_track(cur, None) is None + assert delete_replaced_track(cur, 999) is None # no such row + + +# patch os.path.exists so the unlink branch is reachable without real files +@pytest.fixture(autouse=True) +def _exists(monkeypatch): + monkeypatch.setattr("core.imports.rematch_hints.os.path.exists", lambda p: True) + + +# ── worker seam ─────────────────────────────────────────────────────────────── +def _worker(conn): + # Production hands out a FRESH connection per call (the worker closes it); + # here we share one in-memory DB, so proxy close() to a no-op. + w = AutoImportWorker.__new__(AutoImportWorker) + proxy = types.SimpleNamespace(cursor=conn.cursor, commit=conn.commit, close=lambda: None) + w.database = types.SimpleNamespace(_get_connection=lambda: proxy) + return w + + +def test_resolve_returns_none_when_no_hint(conn): + w = _worker(conn) + cand = FolderCandidate(path="/staging", name="Song", audio_files=["/staging/Song.flac"]) + assert w._resolve_rematch_hint(cand) == (None, None) # untouched → normal identify + + +def test_resolve_returns_identification_when_hinted(conn, monkeypatch): + # don't hash a real file + monkeypatch.setattr("core.imports.rematch_hints.quick_file_signature", lambda p: None) + create_hint(conn.cursor(), _hint(staged_path="/staging/Song.flac")) + conn.commit() + w = _worker(conn) + cand = FolderCandidate(path="/staging", name="Song", audio_files=["/staging/Song.flac"]) + hint, ident = w._resolve_rematch_hint(cand) + assert hint is not None and hint.album_id == "alb_album1" + assert ident["album_id"] == "alb_album1" and ident["method"] == "rematch_hint" + + +def test_resolve_ignores_multi_file_candidates(conn): + create_hint(conn.cursor(), _hint(staged_path="/staging/Song.flac")) + conn.commit() + w = _worker(conn) + cand = FolderCandidate(path="/staging", name="Album", + audio_files=["/staging/Song.flac", "/staging/Other.flac"]) + assert w._resolve_rematch_hint(cand) == (None, None) # re-identify is single-track only + + +def test_resolve_is_failsafe_on_db_error(): + w = AutoImportWorker.__new__(AutoImportWorker) + def _boom(): + raise RuntimeError("db down") + w.database = types.SimpleNamespace(_get_connection=_boom) + cand = FolderCandidate(path="/staging", name="Song", audio_files=["/staging/Song.flac"]) + assert w._resolve_rematch_hint(cand) == (None, None) # error never breaks auto-import + + +def test_finalize_consumes_and_replaces(conn, monkeypatch): + monkeypatch.setattr("core.imports.rematch_hints.quick_file_signature", lambda p: None) + cur = conn.cursor() + cur.execute("INSERT INTO tracks (id, file_path) VALUES (42, '/lib/EP1/05 - Song.flac')") + hid = create_hint(cur, _hint(replace_track_id=42)) + conn.commit() + w = _worker(conn) + hint = find_hint_for_file(conn.cursor(), "/staging/Song.flac") + w._finalize_rematch_hint(hint) + # old row deleted, hint consumed + cur.execute("SELECT 1 FROM tracks WHERE id = 42") + assert cur.fetchone() is None + assert find_hint_for_file(conn.cursor(), "/staging/Song.flac") is None # consumed From 3e554f8274263fde1486ee3e1f7c87fde7212668 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Thu, 18 Jun 2026 15:31:33 -0700 Subject: [PATCH 15/24] =?UTF-8?q?#889=20Phase=203:=20re-identify=20search?= =?UTF-8?q?=20=E2=80=94=20multi-source=20track=E2=86=92release=20lookup=20?= =?UTF-8?q?+=20API?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Search any configured source (tabs, default active) and surface the SAME song across its collections (single/EP/album) so the user can pick which release a track should be filed under. - core/imports/rematch_search.py: pure normalize + injected client factory. search_release_candidates() → lightweight display rows from typed search_tracks (title/artist/release/type badge/year/count/art/isrc/track_id); resolve_hint_fields() runs ONCE on the picked row via get_track_details to pull the album_id (+ isrc/ track#/disc) the hint needs. infer_release_type() handles Spotify's missing 'EP' (multi-track 'single' → EP badge); filing is driven by real album_id, not the label. - GET /api/reidentify/sources (tabs) + GET /api/reidentify/search (rows). Graceful empty on dead source / blank query / client error — never raises. 14 tests. Inert until the modal (Phase 4) calls it. --- core/imports/rematch_search.py | 247 +++++++++++++++++++++++++++ tests/imports/test_rematch_search.py | 121 +++++++++++++ web_server.py | 41 +++++ 3 files changed, 409 insertions(+) create mode 100644 core/imports/rematch_search.py create mode 100644 tests/imports/test_rematch_search.py diff --git a/core/imports/rematch_search.py b/core/imports/rematch_search.py new file mode 100644 index 00000000..da32c373 --- /dev/null +++ b/core/imports/rematch_search.py @@ -0,0 +1,247 @@ +"""#889 Phase 3: search a metadata source for the releases a track appears on. + +The Re-identify modal lets the user search ANY configured source (tabs, defaulting +to the active one) and shows the SAME song across its different collections — +single / EP / album — so they can pick which release the track should be filed +under. Two steps, deliberately split: + + * ``search_release_candidates(source, query)`` — lightweight DISPLAY rows from the + normal typed ``search_tracks`` (title, artist, release name, type badge, year, + track count, art, ISRC, track_id). No album_id needed to draw the list. + * ``resolve_hint_fields(source, track_id)`` — runs ONCE, on the row the user + picks: ``get_track_details`` yields the album_id / isrc / track#/disc the hint + needs. We don't pay that lookup for every search result, only the chosen one. + +Pure normalization + injected client factory, so the search/normalize/resolve seam +is unit-tested with a fake client and no network. +""" + +from __future__ import annotations + +from typing import Any, Callable, Dict, List, Optional + + +def _get(obj: Any, key: str, default=None): + """Read ``key`` from either an object (attr) or a mapping (item).""" + if obj is None: + return default + if isinstance(obj, dict): + return obj.get(key, default) + return getattr(obj, key, default) + + +def _year(release_date: Any) -> Optional[str]: + s = str(release_date or "").strip() + return s[:4] if len(s) >= 4 and s[:4].isdigit() else None + + +def infer_release_type(album_type: Any, total_tracks: Any) -> str: + """Normalize a source's release type to one of album / ep / single / compilation. + + Sources disagree: Spotify has no 'EP' — EPs come back as ``album_type='single'`` + with several tracks; MusicBrainz/Deezer label EPs properly. So when a 'single' + carries more than a handful of tracks, call it an EP for the badge. The actual + filing is unaffected — that's driven by the real album_id, not this label.""" + t = str(album_type or "").strip().lower() + try: + n = int(total_tracks) if total_tracks is not None else 0 + except (TypeError, ValueError): + n = 0 + if t in ("compilation", "comp"): + return "compilation" + if t == "ep": + return "ep" + if t == "album": + return "album" # an explicit album stays an album; only 'single' gets promoted to EP + if t == "single": + # 1–3 tracks → single; 4+ → almost always an EP in practice. + return "ep" if n >= 4 else "single" + # Unknown type: infer purely from track count. + if n >= 7: + return "album" + if n >= 4: + return "ep" + if n >= 1: + return "single" + return t or "album" + + +def normalize_search_result(result: Any, source: str) -> Optional[Dict[str, Any]]: + """One typed search Track (or raw dict) → a display row, or ``None`` if it has + no usable id/title. ``album`` is just a name at search time; album_id is + resolved later for the picked row only.""" + track_id = _get(result, "id") or _get(result, "track_id") + title = _get(result, "name") or _get(result, "title") + if not track_id or not title: + return None + + artists = _get(result, "artists") + if isinstance(artists, list): + artist_name = ", ".join(str(_get(a, "name", a) if not isinstance(a, str) else a) for a in artists) + else: + artist_name = str(artists or _get(result, "artist") or "") + + album = _get(result, "album") + album_name = album if isinstance(album, str) else (_get(album, "name") or "") + raw_type = _get(result, "album_type") + total = _get(result, "total_tracks") + ext = _get(result, "external_ids") or {} + isrc = _get(result, "isrc") or (ext.get("isrc") if isinstance(ext, dict) else None) + + return { + "source": source, + "track_id": str(track_id), + "track_title": str(title), + "artist_name": artist_name, + "album_name": str(album_name or ""), + "album_type": infer_release_type(raw_type, total), + "raw_album_type": str(raw_type or ""), + "total_tracks": int(total) if isinstance(total, int) else None, + "year": _year(_get(result, "release_date")), + "image_url": _get(result, "image_url") or "", + "isrc": isrc or None, + } + + +def search_release_candidates( + source: str, + query: str, + *, + limit: int = 25, + client_factory: Optional[Callable[[str], Any]] = None, +) -> List[Dict[str, Any]]: + """Search ``source`` for tracks matching ``query`` → normalized display rows. + + Returns ``[]`` (never raises) when the source has no client or errors — the UI + just shows an empty tab. Rows keep duplicate releases; the UI groups them.""" + query = (query or "").strip() + if not query: + return [] + factory = client_factory or _default_client_factory + try: + client = factory(source) + except Exception: + client = None + if client is None or not hasattr(client, "search_tracks"): + return [] + try: + results = client.search_tracks(query, limit=limit) + except TypeError: + results = client.search_tracks(query) # clients with no limit kwarg + except Exception: + return [] + + rows: List[Dict[str, Any]] = [] + for r in results or []: + row = normalize_search_result(r, source) + if row is not None: + rows.append(row) + return rows + + +def resolve_hint_fields( + source: str, + track_id: str, + *, + client_factory: Optional[Callable[[str], Any]] = None, +) -> Optional[Dict[str, Any]]: + """Resolve the picked track to the fields a hint needs (album_id critically, + plus isrc / track# / disc# / album name+type). One lookup for one chosen row. + Returns ``None`` if it can't be resolved (caller surfaces an error).""" + factory = client_factory or _default_client_factory + try: + client = factory(source) + except Exception: + client = None + if client is None or not hasattr(client, "get_track_details"): + return None + try: + details = client.get_track_details(track_id) + except Exception: + return None + if not details: + return None + + album = _get(details, "album") or {} + album_id = _get(album, "id") if not isinstance(album, str) else None + album_name = _get(album, "name") if not isinstance(album, str) else album + album_type = _get(album, "album_type") or _get(details, "album_type") + total = _get(album, "total_tracks") or _get(details, "total_tracks") + + artists = _get(details, "artists") or [] + artist_id = None + artist_name = "" + if isinstance(artists, list) and artists: + artist_id = _get(artists[0], "id") + artist_name = ", ".join(str(_get(a, "name", a) if not isinstance(a, str) else a) for a in artists) + + ext = _get(details, "external_ids") or {} + isrc = _get(details, "isrc") or (ext.get("isrc") if isinstance(ext, dict) else None) + + if not album_id: + return None # without an album_id the import can't fetch the tracklist + + return { + "source": source, + "track_id": str(track_id), + "album_id": str(album_id), + "artist_id": str(artist_id) if artist_id else None, + "track_title": _get(details, "name") or _get(details, "title") or "", + "album_name": str(album_name or ""), + "artist_name": artist_name, + "album_type": infer_release_type(album_type, total), + "track_number": _get(details, "track_number"), + "disc_number": _get(details, "disc_number") or 1, + "isrc": isrc or None, + } + + +def _default_client_factory(source: str): + from core.metadata.registry import get_client_for_source + return get_client_for_source(source) + + +def available_sources() -> List[Dict[str, Any]]: + """The source tabs for the modal: every metadata source with a live client, + the primary one flagged ``active`` so the UI selects it by default.""" + from core.metadata.registry import ( + METADATA_SOURCE_PRIORITY, + get_client_for_source, + get_primary_source, + ) + + try: + primary = get_primary_source() + except Exception: + primary = None + + out: List[Dict[str, Any]] = [] + seen = set() + for src in METADATA_SOURCE_PRIORITY: + if src in seen: + continue + seen.add(src) + try: + client = get_client_for_source(src) + except Exception: + client = None + if client is None or not hasattr(client, "search_tracks"): + continue + out.append({ + "source": src, + "label": src.replace("_", " ").title(), + "active": src == primary, + }) + # Guarantee the primary is selectable + first even if priority ordering missed it. + if primary and not any(s["active"] for s in out): + out.insert(0, {"source": primary, "label": primary.replace("_", " ").title(), "active": True}) + return out + + +__all__ = [ + "infer_release_type", + "normalize_search_result", + "search_release_candidates", + "resolve_hint_fields", + "available_sources", +] diff --git a/tests/imports/test_rematch_search.py b/tests/imports/test_rematch_search.py new file mode 100644 index 00000000..dd962bbe --- /dev/null +++ b/tests/imports/test_rematch_search.py @@ -0,0 +1,121 @@ +"""#889 Phase 3: re-identify search — normalize results across sources, infer the +release-type badge, and resolve the picked row's album_id. + +Locks down: same song surfaces as multiple rows (single/EP/album), the EP +inference from a multi-track 'single', graceful empty on a dead source, and that +resolve_hint_fields pulls album_id (and refuses a result without one). +""" + +from __future__ import annotations + +import types + +from core.imports.rematch_search import ( + available_sources, + infer_release_type, + normalize_search_result, + resolve_hint_fields, + search_release_candidates, +) + + +# typed-Track-ish object (mirrors core.metadata.types.Track attrs the modal reads) +def _track(tid, title, album, album_type, total, isrc=None, year="2020"): + return types.SimpleNamespace( + id=tid, name=title, artists=["Artist"], album=album, + album_type=album_type, total_tracks=total, release_date=year + "-01-01", + image_url="http://img/" + tid, isrc=isrc, external_ids={}, + ) + + +# ── release-type inference ──────────────────────────────────────────────────── +def test_infer_album_stays_album(): + assert infer_release_type("album", 12) == "album" + + +def test_infer_single_one_track_is_single(): + assert infer_release_type("single", 1) == "single" + + +def test_infer_multitrack_single_promoted_to_ep(): + # Spotify labels EPs as album_type='single' — promote on track count. + assert infer_release_type("single", 5) == "ep" + + +def test_infer_compilation(): + assert infer_release_type("compilation", 40) == "compilation" + + +def test_infer_unknown_falls_back_to_count(): + assert infer_release_type(None, 10) == "album" + assert infer_release_type("", 4) == "ep" + assert infer_release_type(None, 1) == "single" + + +# ── normalization ───────────────────────────────────────────────────────────── +def test_normalize_builds_display_row(): + row = normalize_search_result(_track("t1", "Song", "Album1", "album", 12, isrc="US1234567890"), "spotify") + assert row["track_id"] == "t1" + assert row["album_name"] == "Album1" and row["album_type"] == "album" + assert row["artist_name"] == "Artist" + assert row["year"] == "2020" and row["isrc"] == "US1234567890" + + +def test_normalize_skips_result_without_id_or_title(): + assert normalize_search_result(types.SimpleNamespace(id="", name="X"), "spotify") is None + assert normalize_search_result(types.SimpleNamespace(id="t", name=""), "spotify") is None + + +def test_same_song_multiple_collections(): + """The headline case: one song, three releases, three distinct rows + badges.""" + results = [ + _track("t_alb", "Song", "Album1", "album", 12), + _track("t_ep", "Song", "EP1", "single", 5), # multi-track single → EP + _track("t_sgl", "Song", "Song (Single)", "single", 1), + ] + client = types.SimpleNamespace(search_tracks=lambda q, limit=25: results) + rows = search_release_candidates("spotify", "Song", client_factory=lambda s: client) + badges = {r["album_name"]: r["album_type"] for r in rows} + assert badges == {"Album1": "album", "EP1": "ep", "Song (Single)": "single"} + + +def test_search_empty_on_missing_client(): + assert search_release_candidates("spotify", "x", client_factory=lambda s: None) == [] + + +def test_search_empty_on_blank_query(): + called = [] + search_release_candidates("spotify", " ", client_factory=lambda s: called.append(1)) + assert called == [] # never even fetches a client for an empty query + + +def test_search_swallows_client_error(): + def boom(q, limit=25): + raise RuntimeError("rate limited") + client = types.SimpleNamespace(search_tracks=boom) + assert search_release_candidates("spotify", "x", client_factory=lambda s: client) == [] + + +# ── resolve on select ───────────────────────────────────────────────────────── +def test_resolve_pulls_album_id_and_fields(): + details = { + "name": "Song", "track_number": 5, "disc_number": 1, "isrc": "US1234567890", + "album": {"id": "alb_album1", "name": "Album1", "album_type": "album", "total_tracks": 12}, + "artists": [{"id": "art_1", "name": "Artist"}], + } + client = types.SimpleNamespace(get_track_details=lambda tid: details) + out = resolve_hint_fields("spotify", "t_alb", client_factory=lambda s: client) + assert out["album_id"] == "alb_album1" + assert out["artist_id"] == "art_1" + assert out["track_number"] == 5 and out["disc_number"] == 1 + assert out["album_type"] == "album" and out["isrc"] == "US1234567890" + + +def test_resolve_refuses_result_without_album_id(): + details = {"name": "Song", "album": {"name": "NoId Album"}} # no album id + client = types.SimpleNamespace(get_track_details=lambda tid: details) + assert resolve_hint_fields("spotify", "t", client_factory=lambda s: client) is None + + +def test_resolve_none_on_missing_client(): + assert resolve_hint_fields("spotify", "t", client_factory=lambda s: None) is None diff --git a/web_server.py b/web_server.py index 6d9cdb3d..d5234210 100644 --- a/web_server.py +++ b/web_server.py @@ -10044,6 +10044,47 @@ def get_artist_enhanced_detail(artist_id): except Exception as e: return jsonify({"success": False, "error": str(e)}), 500 + +# ── Re-identify an imported track (#889) ── +@app.route('/api/reidentify/sources', methods=['GET']) +def reidentify_sources(): + """Source tabs for the Re-identify modal — every metadata source with a live + client, the active one flagged so the UI selects it by default.""" + try: + from core.imports.rematch_search import available_sources + return jsonify({"success": True, "sources": available_sources()}) + except Exception as e: + return jsonify({"success": False, "error": str(e), "sources": []}), 500 + + +@app.route('/api/reidentify/search', methods=['GET']) +def reidentify_search(): + """Search one metadata source for the releases a track appears on. + + Query params: ``source`` (defaults to the active source), ``q`` (the query), + ``limit``. Returns display rows — the SAME song across single/EP/album, each + with a type badge — for the user to pick. album_id is resolved later, only for + the chosen row.""" + try: + from core.imports.rematch_search import available_sources, search_release_candidates + query = (request.args.get('q') or '').strip() + if not query: + return jsonify({"success": True, "results": []}) + source = (request.args.get('source') or '').strip() + if not source: + actives = [s for s in available_sources() if s.get('active')] + source = actives[0]['source'] if actives else 'spotify' + try: + limit = max(1, min(50, int(request.args.get('limit', 25)))) + except (TypeError, ValueError): + limit = 25 + rows = search_release_candidates(source, query, limit=limit) + return jsonify({"success": True, "source": source, "results": rows}) + except Exception as e: + logger.error(f"Re-identify search error: {e}") + return jsonify({"success": False, "error": str(e), "results": []}), 500 + + @app.route('/api/library/artist//quality-analysis') def get_artist_quality_analysis(artist_id): """Analyze track quality for an artist — returns tier classification for each track.""" From f4c16ecc22b7392b5843292999976561bdd599d7 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Thu, 18 Jun 2026 15:37:56 -0700 Subject: [PATCH 16/24] #889 Phase 4: the Re-identify modal + apply backend MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The showpiece: a focused 'which release does this track belong to?' chooser. Source tabs (default active), pre-seeded search, the same song surfaced across single/EP/album with color-coded type badges, ISRC-ranked, replace-original toggle (on by default). Glassy panel, blurred hero art, shimmer/spinner states, hover-lift result cards — matched to the app's modal language. Backend: - core/imports/rematch_apply.py: pure staged_destination + build_reidentify_hint, injectable stage_file_for_reidentify (COPIES the file, never moves — original safe until re-import succeeds). 6 tests. - POST /api/reidentify/apply (admin-only): resolve_hint_fields → stage file → create_hint → nudge the worker. Replace deletes the old row only on success. Frontend: modal markup (index.html), full stylesheet (style.css), and the openReidentifyModal/search/select/confirm flow (library.js). Not yet reachable from a button — Phase 5 wires it. --- core/imports/rematch_apply.py | 92 +++++++++++++ tests/imports/test_rematch_apply.py | 71 ++++++++++ web_server.py | 78 +++++++++++ webui/index.html | 42 ++++++ webui/static/library.js | 195 ++++++++++++++++++++++++++++ webui/static/style.css | 187 ++++++++++++++++++++++++++ 6 files changed, 665 insertions(+) create mode 100644 core/imports/rematch_apply.py create mode 100644 tests/imports/test_rematch_apply.py diff --git a/core/imports/rematch_apply.py b/core/imports/rematch_apply.py new file mode 100644 index 00000000..33bcb8c6 --- /dev/null +++ b/core/imports/rematch_apply.py @@ -0,0 +1,92 @@ +"""#889 Phase 4/5: apply a re-identify — stage the library file + write the hint. + +When the user confirms a release in the Re-identify modal, we: + 1. COPY (never move) the track's library file into the auto-import staging folder, + so the original is untouched until the re-import succeeds, + 2. fingerprint the staged copy (rename-proof binding), and + 3. write a single-use hint carrying the chosen release's IDs (+ ``replace_track_id`` + when 'replace original' is ticked). + +The auto-import worker then picks the staged file up, finds the hint, and re-imports +it against the user-chosen release (Phase 2). The pieces here are split so the +naming + hint construction are pure/unit-tested and the actual copy is injectable. +""" + +from __future__ import annotations + +import os +import shutil +from typing import Any, Callable, Dict, Optional + +from core.imports.paths import sanitize_filename +from core.imports.rematch_hints import RematchHint, quick_file_signature + + +def staged_destination(staging_dir: str, real_path: str, library_track_id: Any) -> str: + """Where the staged copy lands: a single loose file in the staging ROOT (so the + worker treats it as a single-track candidate), named to keep the extension and + be unique + traceable to the track it re-identifies. The filename is cosmetic — + matching is driven by the hint, not the name.""" + base = os.path.basename(real_path) + stem, ext = os.path.splitext(base) + safe_stem = sanitize_filename(stem).strip() or "track" + name = f"{safe_stem} [reid-{library_track_id}]{ext}" + return os.path.join(staging_dir, name) + + +def stage_file_for_reidentify( + real_path: str, + staging_dir: str, + library_track_id: Any, + *, + copy_fn: Callable[[str, str], object] = shutil.copy2, + signature_fn: Callable[[str], Optional[str]] = quick_file_signature, +) -> Dict[str, Any]: + """Copy the library file into staging and fingerprint the copy. Returns + ``{staged_path, content_hash}``. Raises ``FileNotFoundError`` if the source is + gone (caller surfaces a clear error rather than writing a dangling hint).""" + if not real_path or not os.path.isfile(real_path): + raise FileNotFoundError(real_path or "(empty path)") + os.makedirs(staging_dir, exist_ok=True) + dest = staged_destination(staging_dir, real_path, library_track_id) + copy_fn(real_path, dest) + return {"staged_path": dest, "content_hash": signature_fn(dest)} + + +def build_reidentify_hint( + library_track_id: Any, + hint_fields: Dict[str, Any], + staged_path: str, + content_hash: Optional[str], + *, + replace: bool, +) -> RematchHint: + """Pure: assemble the RematchHint from the resolved release fields + staging + info. ``replace_track_id`` is the library row to delete on success, but only + when 'replace original' was ticked. ``exempt_dedup`` is always True — a + re-identify is explicit and must bypass dedup-skip.""" + return RematchHint( + staged_path=staged_path, + content_hash=content_hash, + source=hint_fields.get("source") or "", + isrc=hint_fields.get("isrc"), + track_id=hint_fields.get("track_id"), + album_id=hint_fields.get("album_id"), + artist_id=hint_fields.get("artist_id"), + track_title=hint_fields.get("track_title"), + album_name=hint_fields.get("album_name"), + artist_name=hint_fields.get("artist_name"), + album_type=hint_fields.get("album_type"), + track_number=hint_fields.get("track_number"), + disc_number=hint_fields.get("disc_number"), + replace_track_id=(int(library_track_id) if replace and str(library_track_id).isdigit() else + (library_track_id if replace else None)), + exempt_dedup=True, + ) + + +__all__ = [ + "staged_destination", + "stage_file_for_reidentify", + "build_reidentify_hint", +] diff --git a/tests/imports/test_rematch_apply.py b/tests/imports/test_rematch_apply.py new file mode 100644 index 00000000..20c3fd3e --- /dev/null +++ b/tests/imports/test_rematch_apply.py @@ -0,0 +1,71 @@ +"""#889 Phase 4/5: apply a re-identify — stage the file (copy, not move) + build +the hint. Locks down: the original is never touched, the staged name is unique + +keeps the extension, the hint carries the chosen release, and replace_track_id is +set ONLY when 'replace' is ticked. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from core.imports.rematch_apply import ( + build_reidentify_hint, + stage_file_for_reidentify, + staged_destination, +) + +_FIELDS = { + "source": "spotify", "track_id": "trk_1", "album_id": "alb_album1", + "artist_id": "art_1", "track_title": "Song", "album_name": "Album1", + "artist_name": "Artist", "album_type": "album", "track_number": 5, + "disc_number": 1, "isrc": "US1234567890", +} + + +def test_staged_destination_keeps_ext_and_is_traceable(): + dest = staged_destination("/staging", "/lib/EP1/05 - Song.flac", 42) + assert dest.endswith(".flac") + assert "[reid-42]" in dest # traceable to the track + unique per track + assert dest.startswith("/staging/") # loose file in staging root → single candidate + + +def test_stage_copies_not_moves(tmp_path: Path): + src = tmp_path / "lib" / "EP1" / "05 - Song.flac" + src.parent.mkdir(parents=True) + src.write_bytes(b"audio-bytes") + staging = tmp_path / "Staging" + + out = stage_file_for_reidentify(str(src), str(staging), 42, + signature_fn=lambda p: "sig123") + staged = Path(out["staged_path"]) + assert staged.is_file() and staged.read_bytes() == b"audio-bytes" + assert src.is_file() # ORIGINAL untouched (copy, never move) + assert out["content_hash"] == "sig123" + + +def test_stage_missing_source_raises(tmp_path: Path): + with pytest.raises(FileNotFoundError): + stage_file_for_reidentify(str(tmp_path / "gone.flac"), str(tmp_path / "S"), 1) + + +def test_build_hint_sets_replace_when_ticked(): + h = build_reidentify_hint(42, _FIELDS, "/staging/x.flac", "sig", replace=True) + assert h.replace_track_id == 42 + assert h.album_id == "alb_album1" and h.source == "spotify" + assert h.track_number == 5 and h.isrc == "US1234567890" + assert h.exempt_dedup is True + assert h.staged_path == "/staging/x.flac" and h.content_hash == "sig" + + +def test_build_hint_no_replace_when_unticked(): + h = build_reidentify_hint(42, _FIELDS, "/staging/x.flac", "sig", replace=False) + assert h.replace_track_id is None # keep original → no deletion + assert h.exempt_dedup is True # still bypasses dedup-skip (explicit action) + + +def test_build_hint_handles_non_numeric_track_id(): + # Jellyfin-style GUID track ids must still round-trip as replace target. + h = build_reidentify_hint("abc-guid", _FIELDS, "/s/x.flac", None, replace=True) + assert h.replace_track_id == "abc-guid" diff --git a/web_server.py b/web_server.py index d5234210..2cea211d 100644 --- a/web_server.py +++ b/web_server.py @@ -10085,6 +10085,84 @@ def reidentify_search(): return jsonify({"success": False, "error": str(e), "results": []}), 500 +@app.route('/api/reidentify/apply', methods=['POST']) +def reidentify_apply(): + """Apply a re-identify: stage the track's library file + write a single-use hint + so the auto-import worker re-files it under the chosen release (Phase 2). + + Body: ``{library_track_id, source, track_id, replace}``. Admin-only (mutates the + library). COPIES the file — the original is removed only after the re-import + succeeds, and only when ``replace`` is true.""" + try: + database = get_database() + pid = get_current_profile_id() + prof = database.get_profile(pid) if pid else None + if not prof or not prof.get('is_admin'): + return jsonify({"success": False, "error": "Admin only"}), 403 + + data = request.get_json(silent=True) or {} + library_track_id = data.get('library_track_id') + source = (data.get('source') or '').strip() + track_id = (data.get('track_id') or '').strip() + replace = bool(data.get('replace', True)) + if not library_track_id or not source or not track_id: + return jsonify({"success": False, "error": "library_track_id, source and track_id are required"}), 400 + + from core.imports.rematch_search import resolve_hint_fields + from core.imports.rematch_apply import stage_file_for_reidentify, build_reidentify_hint + from core.imports.rematch_hints import create_hint + + # 1) Resolve the picked release → the IDs the hint needs (album_id critically). + hint_fields = resolve_hint_fields(source, track_id) + if not hint_fields: + return jsonify({"success": False, "error": "Could not resolve the selected release (no album id)"}), 400 + + # 2) Locate the library file for this track. + conn = database._get_connection() + try: + cur = conn.cursor() + cur.execute("SELECT file_path FROM tracks WHERE id = ?", (str(library_track_id),)) + row = cur.fetchone() + finally: + conn.close() + if not row or not row['file_path']: + return jsonify({"success": False, "error": "Library track has no file on disk"}), 404 + real_path = _resolve_library_file_path(row['file_path']) or row['file_path'] + + # 3) Copy into staging + fingerprint the copy. + staging_dir = docker_resolve_path(config_manager.get('import.staging_path', './Staging')) + staged = stage_file_for_reidentify(real_path, staging_dir, library_track_id) + + # 4) Persist the single-use hint. + hint = build_reidentify_hint(library_track_id, hint_fields, + staged['staged_path'], staged['content_hash'], replace=replace) + conn = database._get_connection() + try: + cur = conn.cursor() + hint_id = create_hint(cur, hint) + conn.commit() + finally: + conn.close() + + # 5) Nudge the worker so it doesn't wait for the next timer tick. + try: + if auto_import_worker is not None: + auto_import_worker.trigger_scan() + except Exception as _e: + logger.debug("Re-identify: scan nudge failed (worker will catch it on its timer): %s", _e) + + logger.info("[Re-identify] staged track %s → %s '%s' (%s), replace=%s", + library_track_id, hint.album_type or 'release', hint.album_name or '?', + source, replace) + return jsonify({"success": True, "hint_id": hint_id, "staged_path": staged['staged_path'], + "album_name": hint.album_name, "album_type": hint.album_type}) + except FileNotFoundError as e: + return jsonify({"success": False, "error": f"Source file not found: {e}"}), 404 + except Exception as e: + logger.error(f"Re-identify apply error: {e}") + return jsonify({"success": False, "error": str(e)}), 500 + + @app.route('/api/library/artist//quality-analysis') def get_artist_quality_analysis(artist_id): """Analyze track quality for an artist — returns tier classification for each track.""" diff --git a/webui/index.html b/webui/index.html index 127eb692..6db10878 100644 --- a/webui/index.html +++ b/webui/index.html @@ -7743,6 +7743,48 @@
+ + +