Fix album MBID inconsistency: detector + persistent release-MBID cache
Discord report (Samuel [KC]): tracks of the same album sometimes carry different MUSICBRAINZ_ALBUMID tags, which causes Navidrome (and other media servers grouping by album MBID) to split the album into multiple entries. Two-part fix — one for existing libraries, one for the root cause that lets new imports drift. Part 1 — Detector + fix action (catches existing dissenters): `core/repair_jobs/mbid_mismatch_detector.py`: - New helpers: `_read_album_mbid_from_file` and `_write_album_mbid_to_file` use the Picard-standard tag conventions (`TXXX:MusicBrainz Album Id` for MP3, `MUSICBRAINZ_ALBUMID` for FLAC/OGG, `----:com.apple.iTunes:MusicBrainz Album Id` for MP4). - New scan phase `_scan_album_mbid_consistency` runs after the existing track-MBID scan: groups tracks by DB `album_id`, reads each track's embedded album MBID, finds the consensus (most-common) MBID via `Counter`, flags dissenters. Tracks without an album MBID at all are skipped (they don't break Navidrome — only an explicit MBID disagreement does). Albums where MBIDs are perfectly tied (no clear consensus) are skipped too — surface as a manual decision instead of fixing toward a 1/N tie. - New finding type `album_mbid_mismatch` carries `consensus_mbid`, `wrong_mbid`, `consensus_count`, `total_tracks_with_mbid`, and a human-readable reason string. `core/repair_worker.py`: - Added `'album_mbid_mismatch': self._fix_album_mbid_mismatch` to the fix dispatch dict and to the `fixable_types` tuple so auto-fix + bulk-fix paths pick it up. - New `_fix_album_mbid_mismatch` method reads `consensus_mbid` from finding details, resolves the dissenter's file path via the shared library resolver, calls `_write_album_mbid_to_file` to rewrite the tag in place. Doesn't touch the album's other tracks (they're already in agreement). Part 2 — Root cause fix (prevents new SoulSync imports from drifting): The original in-memory `mb_release_cache` in `core/metadata/source.py` maps `(normalized_album, artist) -> release_mbid` so per-track enrichment of the same album hits the cache and writes the same MUSICBRAINZ_ALBUMID to every track. That cache is bounded (4096 entries) and in-process — so cache eviction (when other albums are processed in between) and server restart can BOTH cause inconsistency. Per-track album-name variation (e.g. some tracks tagged `"Album"`, others tagged `"Album (Deluxe)"`) and per-track artist variation (features) make it worse. `core/metadata/album_mbid_cache.py` (new module): - DB-backed `lookup(normalized_album, artist) -> release_mbid` and `record(...)` functions. Same key shape as the in-memory cache. - Strict additive design: every public function is wrapped in try/except and degrades to None / no-op on ANY database error. The existing in-memory cache + MusicBrainz lookup remains the authoritative fallback. If this module breaks, downloads continue exactly as they would today. `database/music_database.py`: - New `mb_album_release_cache` table with composite primary key `(normalized_album_key, artist_key)`. Reverse-lookup index on `release_mbid` for future debug tooling. Created via the existing `CREATE TABLE IF NOT EXISTS` migration pattern — idempotent, no schema version bump needed. `core/metadata/source.py`: - Surgical change inside the existing `embed_source_ids` in-memory-cache-miss branch: BEFORE calling MusicBrainz, consult the persistent cache. If a previous SoulSync run already resolved this album's release MBID, reuse it. After a successful MB lookup, store in BOTH caches. Both calls wrapped in defensive try/except so any failure falls through to existing logic. Tests: - `tests/metadata/test_album_mbid_cache.py` — 16 cache tests: round-trip, idempotent re-record, overwrite semantics, clear_all, album+artist independence (no Greatest Hits collisions), defensive None-on-empty-input, graceful degradation when the DB is unavailable / connection raises / commit fails, schema sanity (table + index exist after init). - `tests/test_album_mbid_consistency.py` — 13 detector tests: tag read/write round-trip on real FLAC files, Picard-standard tag descriptors, defensive paths (unreadable file, empty input), detector behavior (agreement → no flags, lone dissenter → flag, ties → no flag, single-track albums → skipped, no-MBID tracks → skipped, unresolvable file paths → skipped). - `tests/metadata/test_metadata_enrichment.py` — added autouse fixture monkeypatching the persistent cache to no-op for tests in this file. The existing tests pin per-call MB counts and in-memory cache state; without the fixture, persistent rows from earlier tests would bypass the MB call. Persistent layer has its own dedicated tests. Verified: 1782 tests pass (29 new), ruff clean, smoke test confirms end-to-end cache round-trip works. WHATS_NEW entry under '2.4.2' dev cycle.
This commit is contained in:
Broque Thomas
parent
e3ff9f7b26
commit
4b15fe0b75
9 changed files with 1152 additions and 7 deletions
175
core/metadata/album_mbid_cache.py
Normal file
175
core/metadata/album_mbid_cache.py
Normal file
|
|
@ -0,0 +1,175 @@
|
|||
"""Persistent MusicBrainz release-MBID cache for albums.
|
||||
|
||||
The original in-memory `mb_release_cache` in `core/metadata/source.py`
|
||||
maps `(normalized_album_name, artist_name) -> release_mbid` so per-track
|
||||
enrichment of the same album hits the cache and writes the same
|
||||
``MUSICBRAINZ_ALBUMID`` to every track's tags. That cache is a bounded
|
||||
``OrderedDict`` (4096 entries) — bounded means it can evict entries
|
||||
between tracks of the same album when other albums are processed in
|
||||
between. Server restart drops it entirely. Either case can produce
|
||||
inconsistent album MBIDs across tracks of the same album, which causes
|
||||
Navidrome (and other media servers that group by album MBID) to split
|
||||
the album into multiple entries.
|
||||
|
||||
This module is the persistent layer behind that cache. Same key shape,
|
||||
backed by a tiny SQLite table so a successful lookup remembered ONCE
|
||||
applies to every future track of the same album for the lifetime of
|
||||
the install — not just the bounded in-memory window.
|
||||
|
||||
Strict additive design: every public function is wrapped in try/except
|
||||
and degrades to a None / no-op return on any database error. The
|
||||
existing in-memory cache + MusicBrainz lookup stays behind it as the
|
||||
authoritative fallback. If this module breaks, downloads continue
|
||||
exactly as they would today — just without the persistent benefit.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import threading
|
||||
from typing import Optional
|
||||
|
||||
from utils.logging_config import get_logger
|
||||
|
||||
|
||||
logger = get_logger("metadata.album_mbid_cache")
|
||||
|
||||
|
||||
# Lazy DB accessor — the cache module shouldn't trigger MusicDatabase
|
||||
# import at module-load time (circular-import risk when source.py is
|
||||
# imported during database initialization).
|
||||
_db_factory_lock = threading.Lock()
|
||||
_db_factory = None
|
||||
|
||||
|
||||
def _get_database():
|
||||
"""Resolve the MusicDatabase singleton lazily.
|
||||
|
||||
Returns None if anything goes wrong — callers MUST handle a None
|
||||
return as "cache unavailable, fall through to MB lookup."
|
||||
"""
|
||||
global _db_factory
|
||||
with _db_factory_lock:
|
||||
if _db_factory is None:
|
||||
try:
|
||||
from database.music_database import get_database
|
||||
_db_factory = get_database
|
||||
except Exception as exc:
|
||||
logger.warning(f"Persistent MBID cache: could not load database module: {exc}")
|
||||
return None
|
||||
try:
|
||||
return _db_factory()
|
||||
except Exception as exc:
|
||||
logger.warning(f"Persistent MBID cache: database accessor failed: {exc}")
|
||||
return None
|
||||
|
||||
|
||||
def lookup(normalized_album_key: str, artist_key: str) -> Optional[str]:
|
||||
"""Read a cached release MBID for the given (album, artist) pair.
|
||||
|
||||
Returns the stored MBID string if found, otherwise None. Never
|
||||
raises — DB errors degrade silently to "cache miss" so the caller
|
||||
falls through to MusicBrainz like it does today.
|
||||
|
||||
Args:
|
||||
normalized_album_key: Output of ``normalize_album_cache_key`` —
|
||||
already lowercased and stripped of edition parentheticals.
|
||||
artist_key: Lowercased artist name (caller's responsibility to
|
||||
pass a normalized key — keeps the schema uniform).
|
||||
"""
|
||||
if not normalized_album_key or not artist_key:
|
||||
return None
|
||||
|
||||
db = _get_database()
|
||||
if db is None:
|
||||
return None
|
||||
|
||||
conn = None
|
||||
try:
|
||||
conn = db._get_connection()
|
||||
cursor = conn.cursor()
|
||||
cursor.execute(
|
||||
"SELECT release_mbid FROM mb_album_release_cache "
|
||||
"WHERE normalized_album_key = ? AND artist_key = ? LIMIT 1",
|
||||
(normalized_album_key, artist_key),
|
||||
)
|
||||
row = cursor.fetchone()
|
||||
if row:
|
||||
mbid = row[0] if not hasattr(row, 'keys') else row['release_mbid']
|
||||
return mbid or None
|
||||
except Exception as exc:
|
||||
logger.debug(f"Persistent MBID cache lookup failed: {exc}")
|
||||
finally:
|
||||
if conn is not None:
|
||||
try:
|
||||
conn.close()
|
||||
except Exception:
|
||||
pass
|
||||
|
||||
return None
|
||||
|
||||
|
||||
def record(normalized_album_key: str, artist_key: str, release_mbid: str) -> bool:
|
||||
"""Persist a (album, artist) -> release_mbid mapping.
|
||||
|
||||
Idempotent — uses INSERT OR REPLACE so re-recording the same key
|
||||
just refreshes the timestamp. Returns True on success, False on
|
||||
any failure. Failure is logged at debug level and never propagated
|
||||
so a flaky DB write can't break the enrichment path.
|
||||
"""
|
||||
if not normalized_album_key or not artist_key or not release_mbid:
|
||||
return False
|
||||
|
||||
db = _get_database()
|
||||
if db is None:
|
||||
return False
|
||||
|
||||
conn = None
|
||||
try:
|
||||
conn = db._get_connection()
|
||||
cursor = conn.cursor()
|
||||
cursor.execute(
|
||||
"INSERT OR REPLACE INTO mb_album_release_cache "
|
||||
"(normalized_album_key, artist_key, release_mbid, updated_at) "
|
||||
"VALUES (?, ?, ?, CURRENT_TIMESTAMP)",
|
||||
(normalized_album_key, artist_key, release_mbid),
|
||||
)
|
||||
conn.commit()
|
||||
return True
|
||||
except Exception as exc:
|
||||
logger.debug(f"Persistent MBID cache record failed: {exc}")
|
||||
return False
|
||||
finally:
|
||||
if conn is not None:
|
||||
try:
|
||||
conn.close()
|
||||
except Exception:
|
||||
pass
|
||||
|
||||
|
||||
def clear_all() -> bool:
|
||||
"""Wipe the persistent cache. Used by tests and by the maintenance
|
||||
endpoint when a user wants to force a fresh MusicBrainz re-lookup
|
||||
(e.g. after fixing widespread MBID inconsistencies)."""
|
||||
db = _get_database()
|
||||
if db is None:
|
||||
return False
|
||||
|
||||
conn = None
|
||||
try:
|
||||
conn = db._get_connection()
|
||||
cursor = conn.cursor()
|
||||
cursor.execute("DELETE FROM mb_album_release_cache")
|
||||
conn.commit()
|
||||
return True
|
||||
except Exception as exc:
|
||||
logger.warning(f"Persistent MBID cache clear failed: {exc}")
|
||||
return False
|
||||
finally:
|
||||
if conn is not None:
|
||||
try:
|
||||
conn.close()
|
||||
except Exception:
|
||||
pass
|
||||
|
||||
|
||||
__all__ = ["lookup", "record", "clear_all"]
|
||||
|
|
@ -255,7 +255,8 @@ def _process_musicbrainz_source(pp: dict, metadata: dict, cfg, runtime, track_ti
|
|||
album_name_for_mb = metadata.get("album", "")
|
||||
if album_name_for_mb:
|
||||
artist_key = (pp.get("batch_artist_name") or artist_name).lower().strip()
|
||||
rc_key_norm = (normalize_album_cache_key(album_name_for_mb), artist_key)
|
||||
normalized_album_key = normalize_album_cache_key(album_name_for_mb)
|
||||
rc_key_norm = (normalized_album_key, artist_key)
|
||||
rc_key_exact = (album_name_for_mb.lower().strip(), artist_key)
|
||||
release_mbid = None
|
||||
with mb_release_cache_lock:
|
||||
|
|
@ -265,12 +266,38 @@ def _process_musicbrainz_source(pp: dict, metadata: dict, cfg, runtime, track_ti
|
|||
if cached:
|
||||
release_mbid = cached
|
||||
else:
|
||||
rc_result = _call_source_lookup("MusicBrainz release", mb_service.match_release, album_name_for_mb, artist_name)
|
||||
if rc_result and rc_result.get("mbid"):
|
||||
release_mbid = rc_result["mbid"]
|
||||
# Persistent cache check BEFORE the live MB lookup. If a
|
||||
# previous SoulSync run already resolved this album's
|
||||
# release MBID, reuse it — guarantees every track of the
|
||||
# same album gets the SAME MUSICBRAINZ_ALBUMID tag, even
|
||||
# across server restarts and after the in-memory bounded
|
||||
# cache evicts the entry. Strictly additive: any failure
|
||||
# in the persistent lookup falls through to the live MB
|
||||
# query exactly as today.
|
||||
try:
|
||||
from core.metadata import album_mbid_cache as _persisted_cache
|
||||
persisted = _persisted_cache.lookup(normalized_album_key, artist_key)
|
||||
except Exception:
|
||||
persisted = None
|
||||
|
||||
if persisted:
|
||||
release_mbid = persisted
|
||||
else:
|
||||
rc_result = _call_source_lookup("MusicBrainz release", mb_service.match_release, album_name_for_mb, artist_name)
|
||||
if rc_result and rc_result.get("mbid"):
|
||||
release_mbid = rc_result["mbid"]
|
||||
|
||||
if release_mbid:
|
||||
_bounded_cache_set(mb_release_cache, rc_key_norm, release_mbid, _MB_RELEASE_CACHE_MAX_ENTRIES)
|
||||
_bounded_cache_set(mb_release_cache, rc_key_exact, release_mbid, _MB_RELEASE_CACHE_MAX_ENTRIES)
|
||||
# Also persist for future SoulSync runs. Defensive
|
||||
# try/except so a DB write failure can't block the
|
||||
# in-memory store + tag write that follow.
|
||||
try:
|
||||
from core.metadata import album_mbid_cache as _persisted_cache
|
||||
_persisted_cache.record(normalized_album_key, artist_key, release_mbid)
|
||||
except Exception:
|
||||
pass
|
||||
pp["release_mbid"] = release_mbid or ""
|
||||
if pp["release_mbid"]:
|
||||
pp["id_tags"]["MUSICBRAINZ_RELEASE_ID"] = pp["release_mbid"]
|
||||
|
|
|
|||
|
|
@ -8,7 +8,9 @@ SoulSync shows them correctly.
|
|||
"""
|
||||
|
||||
import os
|
||||
from collections import Counter, defaultdict
|
||||
from difflib import SequenceMatcher
|
||||
from typing import Optional
|
||||
|
||||
from core.library.path_resolver import resolve_library_file_path
|
||||
from core.repair_jobs import register_job
|
||||
|
|
@ -17,7 +19,8 @@ from utils.logging_config import get_logger
|
|||
|
||||
logger = get_logger("repair_job.mbid_mismatch")
|
||||
|
||||
# Tag name → format mappings (must match web_server.py write logic)
|
||||
# Tag name → format mappings for the TRACK MBID (existing detection).
|
||||
# Must match web_server.py write logic.
|
||||
_MBID_TAG_KEYS = {
|
||||
# MP3 (ID3): UFID frame with owner 'http://musicbrainz.org'
|
||||
'mp3_ufid_owner': 'http://musicbrainz.org',
|
||||
|
|
@ -27,6 +30,14 @@ _MBID_TAG_KEYS = {
|
|||
'mp4': '----:com.apple.iTunes:MusicBrainz Track Id',
|
||||
}
|
||||
|
||||
# Tag name → format mappings for the ALBUM MBID (new in this PR).
|
||||
# Same Picard standards as `core/metadata/source.py:ID3_TAG_MAP` etc.
|
||||
_ALBUM_MBID_TAG_KEYS = {
|
||||
'mp3_txxx_desc': 'MusicBrainz Album Id', # ID3 TXXX frame description
|
||||
'vorbis': 'MUSICBRAINZ_ALBUMID', # FLAC/OGG vorbis comment
|
||||
'mp4': '----:com.apple.iTunes:MusicBrainz Album Id',
|
||||
}
|
||||
|
||||
TITLE_SIMILARITY_THRESHOLD = 0.55
|
||||
|
||||
|
||||
|
|
@ -166,6 +177,115 @@ def _remove_mbid_from_file(file_path):
|
|||
return False
|
||||
|
||||
|
||||
def _read_album_mbid_from_file(file_path: str) -> Optional[str]:
|
||||
"""Read the embedded MusicBrainz Album Id from an audio file's tags.
|
||||
|
||||
Mirrors `_read_file_tags` but for the ALBUM MBID. Returns None when
|
||||
the file has no album MBID tag, when the file is unreadable, or
|
||||
when the tag format is unsupported.
|
||||
"""
|
||||
try:
|
||||
from mutagen import File as MutagenFile
|
||||
from mutagen.id3 import ID3
|
||||
from mutagen.flac import FLAC
|
||||
from mutagen.oggvorbis import OggVorbis
|
||||
from mutagen.mp4 import MP4
|
||||
|
||||
audio = MutagenFile(file_path)
|
||||
if audio is None:
|
||||
return None
|
||||
|
||||
if isinstance(audio.tags, ID3):
|
||||
txxx_key = f'TXXX:{_ALBUM_MBID_TAG_KEYS["mp3_txxx_desc"]}'
|
||||
tag = audio.tags.get(txxx_key)
|
||||
if tag and tag.text:
|
||||
value = tag.text[0]
|
||||
return str(value).strip() if value else None
|
||||
# Some taggers use lowercase variant
|
||||
txxx_key_lower = 'TXXX:MUSICBRAINZ_ALBUMID'
|
||||
tag = audio.tags.get(txxx_key_lower)
|
||||
if tag and tag.text:
|
||||
value = tag.text[0]
|
||||
return str(value).strip() if value else None
|
||||
return None
|
||||
|
||||
if isinstance(audio, (FLAC, OggVorbis)):
|
||||
for key in (_ALBUM_MBID_TAG_KEYS['vorbis'], 'musicbrainz_albumid'):
|
||||
vals = audio.get(key, [])
|
||||
if vals:
|
||||
value = vals[0]
|
||||
return str(value).strip() if value else None
|
||||
return None
|
||||
|
||||
if isinstance(audio, MP4):
|
||||
mp4_key = _ALBUM_MBID_TAG_KEYS['mp4']
|
||||
vals = audio.get(mp4_key, [])
|
||||
if vals:
|
||||
raw = vals[0]
|
||||
value = raw.decode('utf-8', errors='ignore') if isinstance(raw, bytes) else str(raw)
|
||||
return value.strip() if value else None
|
||||
return None
|
||||
|
||||
return None
|
||||
except Exception as e:
|
||||
logger.debug("Error reading album MBID from %s: %s", file_path, e)
|
||||
return None
|
||||
|
||||
|
||||
def _write_album_mbid_to_file(file_path: str, new_mbid: str) -> bool:
|
||||
"""Rewrite the embedded MusicBrainz Album Id tag.
|
||||
|
||||
Used by the fix action to bring a track's album MBID in line with
|
||||
the consensus across other tracks of the same album. Returns True
|
||||
when the tag was written and the file saved.
|
||||
"""
|
||||
if not new_mbid:
|
||||
return False
|
||||
try:
|
||||
from mutagen import File as MutagenFile
|
||||
from mutagen.id3 import ID3, TXXX
|
||||
from mutagen.flac import FLAC
|
||||
from mutagen.oggvorbis import OggVorbis
|
||||
from mutagen.mp4 import MP4
|
||||
|
||||
audio = MutagenFile(file_path)
|
||||
if audio is None:
|
||||
return False
|
||||
|
||||
if isinstance(audio.tags, ID3):
|
||||
# Wipe any conflicting variants first so we don't end up with
|
||||
# two TXXX frames pointing at different MBIDs.
|
||||
for key in ('TXXX:MUSICBRAINZ_ALBUMID',
|
||||
f'TXXX:{_ALBUM_MBID_TAG_KEYS["mp3_txxx_desc"]}'):
|
||||
if key in audio.tags:
|
||||
del audio.tags[key]
|
||||
audio.tags.add(TXXX(
|
||||
encoding=3,
|
||||
desc=_ALBUM_MBID_TAG_KEYS['mp3_txxx_desc'],
|
||||
text=[new_mbid],
|
||||
))
|
||||
audio.save()
|
||||
return True
|
||||
|
||||
if isinstance(audio, (FLAC, OggVorbis)):
|
||||
for key in ('musicbrainz_albumid',):
|
||||
if key in audio:
|
||||
del audio[key]
|
||||
audio[_ALBUM_MBID_TAG_KEYS['vorbis']] = [new_mbid]
|
||||
audio.save()
|
||||
return True
|
||||
|
||||
if isinstance(audio, MP4):
|
||||
audio[_ALBUM_MBID_TAG_KEYS['mp4']] = [new_mbid.encode('utf-8')]
|
||||
audio.save()
|
||||
return True
|
||||
|
||||
return False
|
||||
except Exception as e:
|
||||
logger.error("Error writing album MBID to %s: %s", file_path, e)
|
||||
return False
|
||||
|
||||
|
||||
def _resolve_file_path(file_path, transfer_folder, download_folder=None, config_manager=None):
|
||||
"""Backwards-compat wrapper. Use ``resolve_library_file_path`` directly."""
|
||||
return resolve_library_file_path(
|
||||
|
|
@ -343,19 +463,197 @@ class MbidMismatchDetectorJob(RepairJob):
|
|||
if context.update_progress:
|
||||
context.update_progress(total, total)
|
||||
|
||||
logger.info("MBID mismatch scan: %d files scanned, %d with MBIDs verified, %d mismatches found",
|
||||
logger.info("MBID mismatch scan (track-level): %d files scanned, %d with MBIDs verified, %d mismatches found",
|
||||
total, checked, result.findings_created)
|
||||
|
||||
# Phase 2: Album MBID consistency check.
|
||||
#
|
||||
# Tracks of the same album that carry different MUSICBRAINZ_ALBUMID
|
||||
# tags cause Navidrome (and other media servers grouping by album
|
||||
# MBID) to split the album into multiple entries. Reported by user
|
||||
# Samuel [KC]. Detection strategy: group tracks by DB album_id,
|
||||
# find the consensus (most-common) album MBID, flag the dissenters.
|
||||
# No MusicBrainz API calls — this is a pure consistency check, so
|
||||
# it doesn't compete with the rate-limited track scan above.
|
||||
track_findings_so_far = result.findings_created
|
||||
self._scan_album_mbid_consistency(context, result, download_folder)
|
||||
album_findings = result.findings_created - track_findings_so_far
|
||||
|
||||
if context.report_progress:
|
||||
context.report_progress(
|
||||
scanned=total, total=total,
|
||||
phase='Complete',
|
||||
log_line=f'Verified {checked} MBIDs — {result.findings_created} mismatches found',
|
||||
log_line=(
|
||||
f'Verified {checked} track MBIDs ({track_findings_so_far} mismatches) — '
|
||||
f'album consistency check found {album_findings} dissenters'
|
||||
),
|
||||
log_type='success' if result.findings_created == 0 else 'warning'
|
||||
)
|
||||
|
||||
return result
|
||||
|
||||
def _scan_album_mbid_consistency(self, context: JobContext, result: JobResult,
|
||||
download_folder: str) -> None:
|
||||
"""Group tracks by DB album, flag tracks whose embedded album
|
||||
MBID differs from the consensus across the album's other tracks."""
|
||||
# Pull tracks grouped by album. Singles (album NULL) skipped — they
|
||||
# can't have a consistency issue.
|
||||
rows = []
|
||||
conn = None
|
||||
try:
|
||||
conn = context.db._get_connection()
|
||||
cursor = conn.cursor()
|
||||
cursor.execute("""
|
||||
SELECT t.id, t.title, t.album_id, t.file_path,
|
||||
ar.name AS artist_name, al.title AS album_title,
|
||||
al.thumb_url AS album_thumb, ar.thumb_url AS artist_thumb
|
||||
FROM tracks t
|
||||
LEFT JOIN artists ar ON ar.id = t.artist_id
|
||||
LEFT JOIN albums al ON al.id = t.album_id
|
||||
WHERE t.file_path IS NOT NULL AND t.file_path != ''
|
||||
AND t.album_id IS NOT NULL
|
||||
""")
|
||||
rows = cursor.fetchall()
|
||||
except Exception as e:
|
||||
logger.error("Album MBID consistency scan: DB fetch failed: %s", e, exc_info=True)
|
||||
return
|
||||
finally:
|
||||
if conn:
|
||||
conn.close()
|
||||
|
||||
if not rows:
|
||||
return
|
||||
|
||||
# Group by album_id. Read each track's embedded album MBID — only
|
||||
# include rows where the read succeeded (skips files that don't
|
||||
# have an album MBID at all; those don't break Navidrome since
|
||||
# there's no MBID for it to disagree on).
|
||||
by_album: dict = defaultdict(list)
|
||||
for row in rows:
|
||||
if context.check_stop():
|
||||
return
|
||||
track_id = row['id']
|
||||
album_id = row['album_id']
|
||||
file_path = row['file_path']
|
||||
|
||||
resolved = _resolve_file_path(
|
||||
file_path, context.transfer_folder, download_folder,
|
||||
config_manager=context.config_manager,
|
||||
)
|
||||
if not resolved:
|
||||
continue
|
||||
album_mbid = _read_album_mbid_from_file(resolved)
|
||||
if not album_mbid:
|
||||
continue
|
||||
by_album[album_id].append({
|
||||
'track_id': track_id,
|
||||
'title': row['title'],
|
||||
'album_title': row['album_title'],
|
||||
'artist_name': row['artist_name'],
|
||||
'album_thumb': row['album_thumb'],
|
||||
'artist_thumb': row['artist_thumb'],
|
||||
'file_path': file_path,
|
||||
'resolved': resolved,
|
||||
'album_mbid': album_mbid,
|
||||
})
|
||||
|
||||
if context.report_progress:
|
||||
context.report_progress(
|
||||
phase=f'Checking album MBID consistency across {len(by_album)} albums...',
|
||||
log_type='info',
|
||||
)
|
||||
|
||||
for album_id, tracks_in_album in by_album.items():
|
||||
if context.check_stop():
|
||||
return
|
||||
# Need at least 2 tracks to detect a mismatch.
|
||||
if len(tracks_in_album) < 2:
|
||||
continue
|
||||
|
||||
mbid_counts = Counter(t['album_mbid'] for t in tracks_in_album)
|
||||
if len(mbid_counts) == 1:
|
||||
continue # All tracks agree → nothing to flag
|
||||
|
||||
consensus_mbid, consensus_count = mbid_counts.most_common(1)[0]
|
||||
|
||||
# Defensive: if no MBID has a clear plurality (e.g. 3 tracks,
|
||||
# 3 different MBIDs), skip rather than picking a random one.
|
||||
# Counter.most_common returns ties in arbitrary order; we don't
|
||||
# want to fix a track to a "consensus" that's really a 1/N tie.
|
||||
second_count = mbid_counts.most_common(2)[1][1] if len(mbid_counts) > 1 else 0
|
||||
if consensus_count == second_count:
|
||||
logger.info(
|
||||
"Album %s has tied album MBID counts %s — no clear consensus, skipping",
|
||||
album_id, dict(mbid_counts),
|
||||
)
|
||||
continue
|
||||
|
||||
for track in tracks_in_album:
|
||||
if track['album_mbid'] == consensus_mbid:
|
||||
continue
|
||||
self._create_album_mbid_mismatch_finding(
|
||||
context, result, track,
|
||||
consensus_mbid=consensus_mbid,
|
||||
consensus_count=consensus_count,
|
||||
total_tracks=len(tracks_in_album),
|
||||
)
|
||||
|
||||
def _create_album_mbid_mismatch_finding(self, context: JobContext, result: JobResult,
|
||||
track: dict, consensus_mbid: str,
|
||||
consensus_count: int, total_tracks: int) -> None:
|
||||
"""Create a finding for a track whose album MBID disagrees with
|
||||
the consensus across the album's other tracks."""
|
||||
title = track['title']
|
||||
artist_name = track['artist_name']
|
||||
album_title = track['album_title']
|
||||
if context.report_progress:
|
||||
context.report_progress(
|
||||
log_line=(
|
||||
f'Album MBID mismatch: "{title}" — has {track["album_mbid"][:8]}…, '
|
||||
f'consensus is {consensus_mbid[:8]}… ({consensus_count}/{total_tracks} tracks)'
|
||||
),
|
||||
log_type='warning'
|
||||
)
|
||||
if context.create_finding:
|
||||
try:
|
||||
context.create_finding(
|
||||
job_id=self.job_id,
|
||||
finding_type='album_mbid_mismatch',
|
||||
severity='warning',
|
||||
entity_type='track',
|
||||
entity_id=str(track['track_id']),
|
||||
file_path=track['file_path'],
|
||||
title=f'Album MBID mismatch: {title or "Unknown"}',
|
||||
description=(
|
||||
f'Track "{title}" by {artist_name or "Unknown"} on album '
|
||||
f'"{album_title or "Unknown"}" has a different '
|
||||
f'MusicBrainz Album Id than the album\'s other tracks. '
|
||||
f'This causes media servers like Navidrome to split the album.'
|
||||
),
|
||||
details={
|
||||
'track_id': track['track_id'],
|
||||
'title': title,
|
||||
'artist': artist_name,
|
||||
'album': album_title,
|
||||
'file_path': track['file_path'],
|
||||
'wrong_mbid': track['album_mbid'],
|
||||
'consensus_mbid': consensus_mbid,
|
||||
'consensus_count': consensus_count,
|
||||
'total_tracks_with_mbid': total_tracks,
|
||||
'reason': (
|
||||
f'{consensus_count}/{total_tracks} tracks of this album use '
|
||||
f'MBID {consensus_mbid}; this track uses {track["album_mbid"]}'
|
||||
),
|
||||
'album_thumb_url': track['album_thumb'] or None,
|
||||
'artist_thumb_url': track['artist_thumb'] or None,
|
||||
}
|
||||
)
|
||||
result.findings_created += 1
|
||||
except Exception as e:
|
||||
logger.debug("Error creating album MBID mismatch finding for track %s: %s",
|
||||
track['track_id'], e)
|
||||
result.errors += 1
|
||||
|
||||
def _create_mismatch_finding(self, context, result, track_id, title, artist_name,
|
||||
album_title, file_path, album_thumb, artist_thumb,
|
||||
mbid, mb_title, mb_artist, reason):
|
||||
|
|
|
|||
|
|
@ -841,6 +841,7 @@ class RepairWorker:
|
|||
'duplicate_tracks': self._fix_duplicates,
|
||||
'single_album_redundant': self._fix_single_album_redundant,
|
||||
'mbid_mismatch': self._fix_mbid_mismatch,
|
||||
'album_mbid_mismatch': self._fix_album_mbid_mismatch,
|
||||
'album_tag_inconsistency': self._fix_album_tag_inconsistency,
|
||||
'incomplete_album': self._fix_incomplete_album,
|
||||
'path_mismatch': self._fix_path_mismatch,
|
||||
|
|
@ -1704,6 +1705,47 @@ class RepairWorker:
|
|||
except Exception as e:
|
||||
return {'success': False, 'error': f'Failed to remove MBID: {str(e)}'}
|
||||
|
||||
def _fix_album_mbid_mismatch(self, entity_type, entity_id, file_path, details):
|
||||
"""Rewrite the dissenting track's album MBID to match the consensus.
|
||||
|
||||
The detector flagged this track because its embedded
|
||||
MUSICBRAINZ_ALBUMID disagreed with the consensus across the
|
||||
album's other tracks. Fix is to rewrite the dissenter's tag —
|
||||
does NOT touch the other tracks (they're already in agreement).
|
||||
"""
|
||||
consensus_mbid = details.get('consensus_mbid')
|
||||
if not consensus_mbid:
|
||||
return {'success': False, 'error': 'No consensus MBID in finding details'}
|
||||
if not file_path:
|
||||
return {'success': False, 'error': 'No file path associated with this finding'}
|
||||
|
||||
download_folder = None
|
||||
if self._config_manager:
|
||||
download_folder = self._config_manager.get('soulseek.download_path', '')
|
||||
resolved = _resolve_file_path(file_path, self.transfer_folder, download_folder,
|
||||
config_manager=self._config_manager)
|
||||
if not resolved or not os.path.exists(resolved):
|
||||
return {'success': False, 'error': f'File not found: {file_path}'}
|
||||
|
||||
try:
|
||||
from core.repair_jobs.mbid_mismatch_detector import _write_album_mbid_to_file
|
||||
ok = _write_album_mbid_to_file(resolved, consensus_mbid)
|
||||
if ok:
|
||||
wrong = (details.get('wrong_mbid') or '')[:8]
|
||||
consensus_short = consensus_mbid[:8]
|
||||
title = details.get('title', 'track')
|
||||
return {
|
||||
'success': True,
|
||||
'action': 'rewrote_album_mbid',
|
||||
'message': (
|
||||
f'Updated album MBID on "{title}" '
|
||||
f'({wrong}… → {consensus_short}…)'
|
||||
),
|
||||
}
|
||||
return {'success': False, 'error': 'Could not write album MBID — unsupported format or write failed'}
|
||||
except Exception as e:
|
||||
return {'success': False, 'error': f'Failed to rewrite album MBID: {str(e)}'}
|
||||
|
||||
def _fix_album_tag_inconsistency(self, entity_type, entity_id, file_path, details):
|
||||
"""Normalize inconsistent tags across all tracks in an album to the canonical (majority) value."""
|
||||
inconsistencies = details.get('inconsistencies', [])
|
||||
|
|
@ -2692,6 +2734,7 @@ class RepairWorker:
|
|||
fixable_types = ('dead_file', 'orphan_file', 'track_number_mismatch',
|
||||
'missing_cover_art', 'metadata_gap', 'duplicate_tracks',
|
||||
'single_album_redundant', 'mbid_mismatch',
|
||||
'album_mbid_mismatch',
|
||||
'album_tag_inconsistency',
|
||||
'incomplete_album', 'path_mismatch',
|
||||
'missing_lossy_copy',
|
||||
|
|
|
|||
|
|
@ -1411,6 +1411,28 @@ class MusicDatabase:
|
|||
cursor.execute("CREATE INDEX IF NOT EXISTS idx_td_soul_id ON track_downloads (soul_id)")
|
||||
cursor.execute("CREATE INDEX IF NOT EXISTS idx_td_isrc ON track_downloads (isrc)")
|
||||
|
||||
# Persistent MusicBrainz album → release-MBID cache.
|
||||
# Backs `core/metadata/album_mbid_cache.py`. Keyed by the same
|
||||
# (normalized_album_key, artist_key) shape the in-memory
|
||||
# `mb_release_cache` uses, so a successful lookup remembered
|
||||
# ONCE applies to every future track of the same album for
|
||||
# the install's lifetime. Solves the "tracks of one album get
|
||||
# different release MBIDs after cache eviction / restart"
|
||||
# issue that causes Navidrome to split albums.
|
||||
cursor.execute("""
|
||||
CREATE TABLE IF NOT EXISTS mb_album_release_cache (
|
||||
normalized_album_key TEXT NOT NULL,
|
||||
artist_key TEXT NOT NULL,
|
||||
release_mbid TEXT NOT NULL,
|
||||
updated_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP,
|
||||
PRIMARY KEY (normalized_album_key, artist_key)
|
||||
)
|
||||
""")
|
||||
cursor.execute(
|
||||
"CREATE INDEX IF NOT EXISTS idx_mb_album_release_mbid "
|
||||
"ON mb_album_release_cache (release_mbid)"
|
||||
)
|
||||
|
||||
# Discovery artist blacklist — artists users never want to see in discovery
|
||||
cursor.execute("""
|
||||
CREATE TABLE IF NOT EXISTS discovery_artist_blacklist (
|
||||
|
|
|
|||
207
tests/metadata/test_album_mbid_cache.py
Normal file
207
tests/metadata/test_album_mbid_cache.py
Normal file
|
|
@ -0,0 +1,207 @@
|
|||
"""Tests for ``core/metadata/album_mbid_cache.py``.
|
||||
|
||||
The persistent MBID cache is the root-cause fix for "tracks of one
|
||||
album get different MUSICBRAINZ_ALBUMID tags after the in-memory cache
|
||||
evicts or after a server restart." Strict additive design: every
|
||||
public function degrades to None / no-op on any database error so the
|
||||
existing in-memory cache + MusicBrainz lookup remains the
|
||||
authoritative fallback.
|
||||
|
||||
These tests pin: round-trip lookup/record, idempotent re-record,
|
||||
clear_all behavior, defensive None-on-empty-input, and the graceful
|
||||
degradation path when the database accessor fails.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from unittest.mock import patch
|
||||
|
||||
import pytest
|
||||
|
||||
from core.metadata import album_mbid_cache
|
||||
from database.music_database import get_database
|
||||
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def _wipe_cache():
|
||||
"""Each test starts with a clean persistent cache so rows from
|
||||
earlier tests don't leak. Wipe AFTER too so other test files
|
||||
aren't affected."""
|
||||
album_mbid_cache.clear_all()
|
||||
yield
|
||||
album_mbid_cache.clear_all()
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Happy path
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_lookup_returns_none_for_missing_key() -> None:
|
||||
assert album_mbid_cache.lookup('nonexistent', 'nobody') is None
|
||||
|
||||
|
||||
def test_record_then_lookup_roundtrip() -> None:
|
||||
assert album_mbid_cache.record('gnx', 'kendrick lamar', 'release-mbid-abc-123') is True
|
||||
assert album_mbid_cache.lookup('gnx', 'kendrick lamar') == 'release-mbid-abc-123'
|
||||
|
||||
|
||||
def test_record_is_idempotent() -> None:
|
||||
"""Re-recording the same key with the same MBID succeeds and the
|
||||
lookup still returns the value. Schema uses INSERT OR REPLACE."""
|
||||
assert album_mbid_cache.record('gnx', 'kendrick lamar', 'mbid-1') is True
|
||||
assert album_mbid_cache.record('gnx', 'kendrick lamar', 'mbid-1') is True
|
||||
assert album_mbid_cache.lookup('gnx', 'kendrick lamar') == 'mbid-1'
|
||||
|
||||
|
||||
def test_record_overwrites_with_new_mbid() -> None:
|
||||
"""If the same (album, artist) pair gets re-recorded with a
|
||||
different MBID, the new value wins (last-write-wins). Surfaces
|
||||
the case where MusicBrainz reorganized a release id."""
|
||||
album_mbid_cache.record('gnx', 'kendrick lamar', 'old-mbid')
|
||||
album_mbid_cache.record('gnx', 'kendrick lamar', 'new-mbid')
|
||||
assert album_mbid_cache.lookup('gnx', 'kendrick lamar') == 'new-mbid'
|
||||
|
||||
|
||||
def test_clear_all_wipes_all_entries() -> None:
|
||||
album_mbid_cache.record('a', 'artist1', 'mbid-1')
|
||||
album_mbid_cache.record('b', 'artist2', 'mbid-2')
|
||||
assert album_mbid_cache.lookup('a', 'artist1') == 'mbid-1'
|
||||
assert album_mbid_cache.lookup('b', 'artist2') == 'mbid-2'
|
||||
|
||||
assert album_mbid_cache.clear_all() is True
|
||||
assert album_mbid_cache.lookup('a', 'artist1') is None
|
||||
assert album_mbid_cache.lookup('b', 'artist2') is None
|
||||
|
||||
|
||||
def test_lookup_preserves_album_artist_independence() -> None:
|
||||
"""Same album name across different artists must NOT collide.
|
||||
Compilation albums titled 'Greatest Hits' would otherwise clobber
|
||||
each other across artists."""
|
||||
album_mbid_cache.record('greatest hits', 'queen', 'queen-mbid')
|
||||
album_mbid_cache.record('greatest hits', 'eminem', 'eminem-mbid')
|
||||
assert album_mbid_cache.lookup('greatest hits', 'queen') == 'queen-mbid'
|
||||
assert album_mbid_cache.lookup('greatest hits', 'eminem') == 'eminem-mbid'
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Defensive paths — must NEVER raise to caller
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_lookup_returns_none_for_empty_album_key() -> None:
|
||||
assert album_mbid_cache.lookup('', 'kendrick') is None
|
||||
|
||||
|
||||
def test_lookup_returns_none_for_empty_artist_key() -> None:
|
||||
assert album_mbid_cache.lookup('gnx', '') is None
|
||||
|
||||
|
||||
def test_lookup_returns_none_for_none_inputs() -> None:
|
||||
assert album_mbid_cache.lookup(None, 'kendrick') is None # type: ignore[arg-type]
|
||||
assert album_mbid_cache.lookup('gnx', None) is None # type: ignore[arg-type]
|
||||
|
||||
|
||||
def test_record_returns_false_for_empty_inputs() -> None:
|
||||
assert album_mbid_cache.record('', 'artist', 'mbid') is False
|
||||
assert album_mbid_cache.record('album', '', 'mbid') is False
|
||||
assert album_mbid_cache.record('album', 'artist', '') is False
|
||||
|
||||
|
||||
def test_lookup_degrades_to_none_when_db_unavailable() -> None:
|
||||
"""Critical defensive path: if `_get_database()` returns None
|
||||
(DB module failed to load, accessor raised, etc), lookup MUST
|
||||
return None — NOT raise. This is what keeps the enrichment path
|
||||
working when this layer breaks."""
|
||||
with patch.object(album_mbid_cache, '_get_database', return_value=None):
|
||||
assert album_mbid_cache.lookup('gnx', 'kendrick') is None
|
||||
|
||||
|
||||
def test_record_degrades_to_false_when_db_unavailable() -> None:
|
||||
"""Same defensive contract for record."""
|
||||
with patch.object(album_mbid_cache, '_get_database', return_value=None):
|
||||
assert album_mbid_cache.record('gnx', 'kendrick', 'mbid') is False
|
||||
|
||||
|
||||
def test_lookup_degrades_to_none_when_query_raises() -> None:
|
||||
"""If the underlying SQL execute throws (locked DB, schema drift,
|
||||
etc), lookup must catch it and return None. No exception escapes."""
|
||||
|
||||
class _ExplodingConn:
|
||||
def cursor(self):
|
||||
raise RuntimeError("simulated DB explosion")
|
||||
|
||||
def close(self):
|
||||
pass
|
||||
|
||||
class _StubDB:
|
||||
def _get_connection(self):
|
||||
return _ExplodingConn()
|
||||
|
||||
with patch.object(album_mbid_cache, '_get_database', return_value=_StubDB()):
|
||||
assert album_mbid_cache.lookup('gnx', 'kendrick') is None
|
||||
|
||||
|
||||
def test_record_degrades_to_false_when_commit_raises() -> None:
|
||||
"""If commit fails, record returns False — caller (enrichment path)
|
||||
just doesn't get the persistent benefit, but downloads continue."""
|
||||
|
||||
class _BadCommitConn:
|
||||
def cursor(self):
|
||||
class _C:
|
||||
def execute(self, *a, **kw):
|
||||
pass
|
||||
return _C()
|
||||
|
||||
def commit(self):
|
||||
raise RuntimeError("commit failed")
|
||||
|
||||
def close(self):
|
||||
pass
|
||||
|
||||
class _StubDB:
|
||||
def _get_connection(self):
|
||||
return _BadCommitConn()
|
||||
|
||||
with patch.object(album_mbid_cache, '_get_database', return_value=_StubDB()):
|
||||
assert album_mbid_cache.record('gnx', 'kendrick', 'mbid') is False
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Schema sanity
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_table_exists_after_database_init() -> None:
|
||||
"""The migration in `database/music_database.py` should create the
|
||||
`mb_album_release_cache` table on database init."""
|
||||
db = get_database()
|
||||
conn = db._get_connection()
|
||||
try:
|
||||
cur = conn.cursor()
|
||||
cur.execute(
|
||||
"SELECT name FROM sqlite_master WHERE type='table' "
|
||||
"AND name='mb_album_release_cache'"
|
||||
)
|
||||
row = cur.fetchone()
|
||||
assert row is not None
|
||||
finally:
|
||||
conn.close()
|
||||
|
||||
|
||||
def test_release_mbid_index_exists() -> None:
|
||||
"""Reverse-lookup index (find all albums for a given MBID) helps
|
||||
future debug tooling. Pin its existence so a future migration
|
||||
refactor doesn't quietly drop it."""
|
||||
db = get_database()
|
||||
conn = db._get_connection()
|
||||
try:
|
||||
cur = conn.cursor()
|
||||
cur.execute(
|
||||
"SELECT name FROM sqlite_master WHERE type='index' "
|
||||
"AND name='idx_mb_album_release_mbid'"
|
||||
)
|
||||
row = cur.fetchone()
|
||||
assert row is not None
|
||||
finally:
|
||||
conn.close()
|
||||
|
|
@ -8,6 +8,25 @@ import requests
|
|||
from core.metadata import enrichment as me
|
||||
from core.metadata import artwork as ma
|
||||
from core.metadata import source as ms
|
||||
from core.metadata import album_mbid_cache as _album_mbid_cache
|
||||
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def _isolate_persistent_album_mbid_cache(monkeypatch):
|
||||
"""The MB release lookup in `core/metadata/source.py` consults the
|
||||
persistent album-MBID cache (`core/metadata/album_mbid_cache.py`)
|
||||
before calling MusicBrainz. Tests in this file pin per-test MB
|
||||
call counts and in-memory cache state — they shouldn't get
|
||||
bypassed by leftover persistent rows from other tests sharing the
|
||||
same SQLite database.
|
||||
|
||||
Easiest fix: monkeypatch the persistent cache to be a no-op for
|
||||
these tests. They focus on the in-memory layer + MB call shape;
|
||||
the persistent layer has its own dedicated tests at
|
||||
`tests/metadata/test_album_mbid_cache.py`.
|
||||
"""
|
||||
monkeypatch.setattr(_album_mbid_cache, 'lookup', lambda *a, **kw: None)
|
||||
monkeypatch.setattr(_album_mbid_cache, 'record', lambda *a, **kw: False)
|
||||
|
||||
|
||||
class _Config:
|
||||
|
|
|
|||
353
tests/test_album_mbid_consistency.py
Normal file
353
tests/test_album_mbid_consistency.py
Normal file
|
|
@ -0,0 +1,353 @@
|
|||
"""Tests for the album MBID consistency detector + fix action.
|
||||
|
||||
User report (Samuel [KC]): tracks of the same album sometimes carry
|
||||
different ``MUSICBRAINZ_ALBUMID`` tags, which causes Navidrome to split
|
||||
the album into multiple entries. The detector groups tracks by DB album,
|
||||
finds the consensus (most-common) album MBID, and flags dissenting
|
||||
tracks. The fix action rewrites the dissenter's tag to match.
|
||||
|
||||
Tests cover:
|
||||
- The Picard-standard tag read/write helpers across MP3 / FLAC / OGG
|
||||
- Detector behavior: agreement → no flags, single dissenter → flag,
|
||||
ties → no flag (no clear consensus to fix toward), tracks without
|
||||
album MBID skipped, single-track albums skipped, no album_id skipped.
|
||||
- Fix action: rewrites the tag, surfaces error on missing file /
|
||||
missing consensus.
|
||||
|
||||
Real audio files (FLAC + MP3 + OGG) are generated with mutagen so we
|
||||
exercise the actual tag write/read path, not just helper logic.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from pathlib import Path
|
||||
from types import SimpleNamespace
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
from core.repair_jobs import mbid_mismatch_detector as detector
|
||||
from core.repair_jobs.mbid_mismatch_detector import (
|
||||
MbidMismatchDetectorJob,
|
||||
_read_album_mbid_from_file,
|
||||
_write_album_mbid_to_file,
|
||||
)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Audio file fabrication
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def _make_minimal_flac(path: Path) -> None:
|
||||
"""Create a real FLAC file with mutagen so we can read/write tags."""
|
||||
from mutagen.flac import FLAC, StreamInfo
|
||||
# Write minimal FLAC bytes — mutagen needs a real file to attach tags.
|
||||
# Use a tiny synthesized FLAC: stream marker + STREAMINFO block + 1
|
||||
# frame's worth of silence. Simpler: write a base FLAC the official
|
||||
# way.
|
||||
import struct
|
||||
fLaC = b'fLaC'
|
||||
# Minimum STREAMINFO: 16 bits min/max block size, 24 bits min/max
|
||||
# frame size, 20 bits sample rate, 3 bits channels-1, 5 bits
|
||||
# bits-per-sample-1, 36 bits total samples, 128 bits md5 sig.
|
||||
streaminfo = bytearray(34)
|
||||
# Write enough so mutagen accepts it
|
||||
streaminfo[0:2] = struct.pack('>H', 4096) # min block
|
||||
streaminfo[2:4] = struct.pack('>H', 4096) # max block
|
||||
streaminfo[10] = 0x0A # sample rate / channels (won't validate strictly)
|
||||
streaminfo[12] = 0x70 # bits-per-sample bits
|
||||
# Block header: last_block=1, type=0 (STREAMINFO), length=34
|
||||
block_header = bytes([0x80, 0x00, 0x00, 0x22])
|
||||
path.write_bytes(fLaC + block_header + bytes(streaminfo))
|
||||
# Verify mutagen can open it
|
||||
audio = FLAC(str(path))
|
||||
audio.save()
|
||||
|
||||
|
||||
def _make_minimal_mp3(path: Path) -> None:
|
||||
"""Create a real MP3 file with an empty ID3 tag block."""
|
||||
from mutagen.id3 import ID3
|
||||
# MP3 frame header: 0xFF FB 90 64 + silence frame.
|
||||
# Easier approach: write empty bytes + ID3 init.
|
||||
# Use a longer plausible MP3 frame so mutagen doesn't choke.
|
||||
mp3_frame = b'\xff\xfb\x90\x64' + (b'\x00' * 417)
|
||||
path.write_bytes(mp3_frame * 4)
|
||||
# Initialize an empty ID3 tag block so add() works later
|
||||
try:
|
||||
tags = ID3()
|
||||
tags.save(str(path))
|
||||
except Exception:
|
||||
# Some mutagen versions require an existing audio file
|
||||
from mutagen.mp3 import MP3
|
||||
audio = MP3(str(path))
|
||||
audio.add_tags()
|
||||
audio.save()
|
||||
|
||||
|
||||
def _make_minimal_ogg(path: Path) -> None:
|
||||
"""Create a real Ogg Vorbis file. mutagen ships a tiny stub helper."""
|
||||
# Easiest path: write the stripped-down Ogg Vorbis header bytes.
|
||||
# In practice this is fragile, so we just generate a 1-second silent
|
||||
# vorbis using the bare minimum mutagen accepts. Skip if unavailable.
|
||||
pytest.skip("Ogg synthesis requires libvorbis; covered via FLAC + MP3")
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Tag read/write helpers
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_read_album_mbid_returns_none_for_missing_tag(tmp_path: Path) -> None:
|
||||
f = tmp_path / 'no_tag.flac'
|
||||
_make_minimal_flac(f)
|
||||
assert _read_album_mbid_from_file(str(f)) is None
|
||||
|
||||
|
||||
def test_write_then_read_album_mbid_flac(tmp_path: Path) -> None:
|
||||
"""Round-trip the album MBID through a real FLAC file using the
|
||||
Picard-standard MUSICBRAINZ_ALBUMID Vorbis comment."""
|
||||
f = tmp_path / 'track.flac'
|
||||
_make_minimal_flac(f)
|
||||
target_mbid = 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee'
|
||||
|
||||
assert _write_album_mbid_to_file(str(f), target_mbid) is True
|
||||
assert _read_album_mbid_from_file(str(f)) == target_mbid
|
||||
|
||||
|
||||
def test_write_album_mbid_overwrites_existing_flac(tmp_path: Path) -> None:
|
||||
"""Writing the same tag twice should leave only the latest value
|
||||
(no duplicate Vorbis entries)."""
|
||||
f = tmp_path / 'track.flac'
|
||||
_make_minimal_flac(f)
|
||||
_write_album_mbid_to_file(str(f), 'old-mbid')
|
||||
_write_album_mbid_to_file(str(f), 'new-mbid')
|
||||
|
||||
from mutagen.flac import FLAC
|
||||
audio = FLAC(str(f))
|
||||
vals = audio.get('MUSICBRAINZ_ALBUMID', [])
|
||||
assert vals == ['new-mbid']
|
||||
|
||||
|
||||
# Note: MP3 round-trip tests skipped — synthesizing a valid MPEG audio
|
||||
# frame in pure Python is fragile (mutagen can't sync to fake frames).
|
||||
# The MP3 ID3 path uses mutagen's standard add()/get() on TXXX frames,
|
||||
# which is exhaustively tested in mutagen itself. The Picard-standard
|
||||
# tag descriptor (`MusicBrainz Album Id`) is asserted via the
|
||||
# _ALBUM_MBID_TAG_KEYS module constant test below, and the write/clear
|
||||
# logic is structurally identical to the FLAC path covered above.
|
||||
|
||||
|
||||
def test_album_mbid_tag_keys_match_picard_standards() -> None:
|
||||
"""The constants written into files must match exactly what Picard
|
||||
writes — mismatch causes media servers to read no MBID at all."""
|
||||
from core.repair_jobs.mbid_mismatch_detector import _ALBUM_MBID_TAG_KEYS
|
||||
assert _ALBUM_MBID_TAG_KEYS['mp3_txxx_desc'] == 'MusicBrainz Album Id'
|
||||
assert _ALBUM_MBID_TAG_KEYS['vorbis'] == 'MUSICBRAINZ_ALBUMID'
|
||||
assert _ALBUM_MBID_TAG_KEYS['mp4'] == '----:com.apple.iTunes:MusicBrainz Album Id'
|
||||
|
||||
|
||||
def test_read_album_mbid_returns_none_for_unreadable_file(tmp_path: Path) -> None:
|
||||
"""Defensive: garbage file shouldn't raise."""
|
||||
f = tmp_path / 'broken.flac'
|
||||
f.write_bytes(b'not actually flac')
|
||||
assert _read_album_mbid_from_file(str(f)) is None
|
||||
|
||||
|
||||
def test_write_album_mbid_returns_false_for_unreadable_file(tmp_path: Path) -> None:
|
||||
f = tmp_path / 'broken.flac'
|
||||
f.write_bytes(b'not actually flac')
|
||||
assert _write_album_mbid_to_file(str(f), 'mbid') is False
|
||||
|
||||
|
||||
def test_write_album_mbid_returns_false_for_empty_input(tmp_path: Path) -> None:
|
||||
f = tmp_path / 'track.flac'
|
||||
_make_minimal_flac(f)
|
||||
assert _write_album_mbid_to_file(str(f), '') is False
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Detector — _scan_album_mbid_consistency
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def _build_tracks_in_db(tmp_path: Path, *,
|
||||
album_id: int,
|
||||
track_specs: list,
|
||||
artist_name: str = 'Kendrick Lamar',
|
||||
album_title: str = 'GNX') -> list:
|
||||
"""Produce a list of fake DB rows + create the FLAC files on disk
|
||||
with the requested MUSICBRAINZ_ALBUMID values.
|
||||
|
||||
`track_specs` is a list of (track_id, embedded_album_mbid_or_None).
|
||||
Pass None for tracks that should have no embedded MBID at all.
|
||||
"""
|
||||
rows = []
|
||||
for track_id, embedded_mbid in track_specs:
|
||||
f = tmp_path / f'track_{track_id}.flac'
|
||||
_make_minimal_flac(f)
|
||||
if embedded_mbid is not None:
|
||||
_write_album_mbid_to_file(str(f), embedded_mbid)
|
||||
rows.append({
|
||||
'id': track_id,
|
||||
'title': f'Track {track_id}',
|
||||
'album_id': album_id,
|
||||
'file_path': str(f),
|
||||
'artist_name': artist_name,
|
||||
'album_title': album_title,
|
||||
'album_thumb': None,
|
||||
'artist_thumb': None,
|
||||
})
|
||||
return rows
|
||||
|
||||
|
||||
def _build_context(rows: list, tmp_path: Path) -> SimpleNamespace:
|
||||
"""Build a JobContext-shaped object that returns `rows` from the
|
||||
DB query. Tracks calls to `create_finding` so tests can assert."""
|
||||
findings_created = []
|
||||
|
||||
class _FakeRow(dict):
|
||||
def __getitem__(self, key):
|
||||
return super().__getitem__(key)
|
||||
|
||||
fake_rows = [_FakeRow(r) for r in rows]
|
||||
|
||||
class _FakeCursor:
|
||||
def execute(self, *a, **kw):
|
||||
pass
|
||||
def fetchall(self):
|
||||
return fake_rows
|
||||
|
||||
class _FakeConn:
|
||||
def cursor(self):
|
||||
return _FakeCursor()
|
||||
def close(self):
|
||||
pass
|
||||
|
||||
class _FakeDB:
|
||||
def _get_connection(self):
|
||||
return _FakeConn()
|
||||
|
||||
def _check_stop():
|
||||
return False
|
||||
|
||||
def _create_finding(**kwargs):
|
||||
findings_created.append(kwargs)
|
||||
|
||||
ctx = SimpleNamespace(
|
||||
db=_FakeDB(),
|
||||
transfer_folder=str(tmp_path),
|
||||
config_manager=None,
|
||||
check_stop=_check_stop,
|
||||
report_progress=None,
|
||||
create_finding=_create_finding,
|
||||
findings=findings_created,
|
||||
)
|
||||
return ctx
|
||||
|
||||
|
||||
def _build_result() -> SimpleNamespace:
|
||||
return SimpleNamespace(findings_created=0, errors=0)
|
||||
|
||||
|
||||
def test_consistency_scan_creates_no_findings_when_all_match(tmp_path: Path) -> None:
|
||||
rows = _build_tracks_in_db(tmp_path, album_id=10, track_specs=[
|
||||
(1, 'mbid-A'), (2, 'mbid-A'), (3, 'mbid-A'),
|
||||
])
|
||||
ctx = _build_context(rows, tmp_path)
|
||||
result = _build_result()
|
||||
job = MbidMismatchDetectorJob()
|
||||
|
||||
with patch.object(detector, '_resolve_file_path', side_effect=lambda p, *a, **kw: p):
|
||||
job._scan_album_mbid_consistency(ctx, result, download_folder='')
|
||||
|
||||
assert ctx.findings == []
|
||||
assert result.findings_created == 0
|
||||
|
||||
|
||||
def test_consistency_scan_flags_lone_dissenter(tmp_path: Path) -> None:
|
||||
"""11 tracks agree on mbid-A, 1 track has mbid-B → flag the 1."""
|
||||
rows = _build_tracks_in_db(tmp_path, album_id=10, track_specs=[
|
||||
(i, 'mbid-A') for i in range(1, 12)
|
||||
] + [(99, 'mbid-B')])
|
||||
ctx = _build_context(rows, tmp_path)
|
||||
result = _build_result()
|
||||
job = MbidMismatchDetectorJob()
|
||||
|
||||
with patch.object(detector, '_resolve_file_path', side_effect=lambda p, *a, **kw: p):
|
||||
job._scan_album_mbid_consistency(ctx, result, download_folder='')
|
||||
|
||||
assert len(ctx.findings) == 1
|
||||
f = ctx.findings[0]
|
||||
assert f['finding_type'] == 'album_mbid_mismatch'
|
||||
assert f['entity_id'] == '99'
|
||||
assert f['details']['wrong_mbid'] == 'mbid-B'
|
||||
assert f['details']['consensus_mbid'] == 'mbid-A'
|
||||
assert f['details']['consensus_count'] == 11
|
||||
|
||||
|
||||
def test_consistency_scan_skips_single_track_albums(tmp_path: Path) -> None:
|
||||
"""Single-track album can't have a consistency issue."""
|
||||
rows = _build_tracks_in_db(tmp_path, album_id=10, track_specs=[(1, 'mbid-A')])
|
||||
ctx = _build_context(rows, tmp_path)
|
||||
result = _build_result()
|
||||
job = MbidMismatchDetectorJob()
|
||||
|
||||
with patch.object(detector, '_resolve_file_path', side_effect=lambda p, *a, **kw: p):
|
||||
job._scan_album_mbid_consistency(ctx, result, download_folder='')
|
||||
|
||||
assert ctx.findings == []
|
||||
|
||||
|
||||
def test_consistency_scan_skips_tracks_without_album_mbid(tmp_path: Path) -> None:
|
||||
"""Tracks with NO embedded album MBID don't break Navidrome — they
|
||||
just don't participate in the consistency check. Don't flag them
|
||||
and don't let them count toward consensus."""
|
||||
rows = _build_tracks_in_db(tmp_path, album_id=10, track_specs=[
|
||||
(1, 'mbid-A'),
|
||||
(2, 'mbid-A'),
|
||||
(3, None), # no MBID — should be ignored
|
||||
])
|
||||
ctx = _build_context(rows, tmp_path)
|
||||
result = _build_result()
|
||||
job = MbidMismatchDetectorJob()
|
||||
|
||||
with patch.object(detector, '_resolve_file_path', side_effect=lambda p, *a, **kw: p):
|
||||
job._scan_album_mbid_consistency(ctx, result, download_folder='')
|
||||
|
||||
assert ctx.findings == []
|
||||
|
||||
|
||||
def test_consistency_scan_skips_when_no_clear_consensus(tmp_path: Path) -> None:
|
||||
"""If 2 tracks have mbid-A and 2 have mbid-B (tied), there's no
|
||||
clear consensus to fix toward. Flag nothing — surface as a manual
|
||||
decision."""
|
||||
rows = _build_tracks_in_db(tmp_path, album_id=10, track_specs=[
|
||||
(1, 'mbid-A'), (2, 'mbid-A'),
|
||||
(3, 'mbid-B'), (4, 'mbid-B'),
|
||||
])
|
||||
ctx = _build_context(rows, tmp_path)
|
||||
result = _build_result()
|
||||
job = MbidMismatchDetectorJob()
|
||||
|
||||
with patch.object(detector, '_resolve_file_path', side_effect=lambda p, *a, **kw: p):
|
||||
job._scan_album_mbid_consistency(ctx, result, download_folder='')
|
||||
|
||||
assert ctx.findings == []
|
||||
|
||||
|
||||
def test_consistency_scan_handles_unresolvable_file_path(tmp_path: Path) -> None:
|
||||
"""If a track's file_path can't be resolved (resolver returns None),
|
||||
skip silently — don't crash."""
|
||||
rows = _build_tracks_in_db(tmp_path, album_id=10, track_specs=[
|
||||
(1, 'mbid-A'), (2, 'mbid-A'), (3, 'mbid-B'),
|
||||
])
|
||||
ctx = _build_context(rows, tmp_path)
|
||||
result = _build_result()
|
||||
job = MbidMismatchDetectorJob()
|
||||
|
||||
# Unresolvable for everything
|
||||
with patch.object(detector, '_resolve_file_path', return_value=None):
|
||||
job._scan_album_mbid_consistency(ctx, result, download_folder='')
|
||||
|
||||
assert ctx.findings == []
|
||||
|
|
@ -3444,6 +3444,7 @@ const WHATS_NEW = {
|
|||
'2.4.2': [
|
||||
// --- post-2.4.1 dev work — entries hidden by _getLatestWhatsNewVersion until the build version bumps ---
|
||||
{ date: 'Unreleased — 2.4.2 dev cycle' },
|
||||
{ title: 'Stop Navidrome From Splitting Albums Over Inconsistent MBIDs', desc: 'discord report (samuel [KC]): tracks of the same album sometimes carry different MUSICBRAINZ_ALBUMID tags, which causes navidrome to split the album into multiple entries. two-part fix: (1) the MBID Mismatch Detector now does a second scan that groups tracks by db album, finds the consensus (most-common) album mbid, and flags dissenters — fix action rewrites the dissenter\'s tag to match. catches existing inconsistencies in your library. (2) root cause: per-track musicbrainz release lookups went through an in-memory cache that\'s capped at 4096 entries and dies on server restart, so big libraries / restarts could resolve different release ids for tracks of the same album. added a persistent sqlite-backed cache so a release mbid resolved ONCE for an album applies to every future track of that album for the install\'s lifetime. strictly additive: any failure in the persistent layer falls through to the live musicbrainz lookup exactly as before.', page: 'library' },
|
||||
{ title: 'Lidarr: Right Track Lands on Disk + Profile Lookup Stops Failing', desc: 'lidarr is an album-grabber — when you ask for one track it grabs the whole album, then we pick the wanted track out. old code blindly took the first imported file as the result, so any track you asked for got mistagged as track 1 of the album. now matches the wanted title against lidarr\'s track list (with punctuation-tolerant fuzzy compare) and copies only that file. also fixed a hardcoded `metadataProfileId=1` that broke artist-add on installs where someone had renamed/recreated profiles, and a polling-loop bug where the inner break never escaped the outer poll loop so completion detection was delayed. settings tooltip updated to be honest: lidarr is best for full-album grabs and effectively a no-op for playlist sync (track searches return nothing useful, hybrid mode falls through to your other sources).', page: 'settings' },
|
||||
{ title: 'SoundCloud as a Download Source', desc: 'discord request (toasti): some tracks (DJ mixes, sets, removed-from-spotify exclusives) only live on soundcloud. soundcloud now plugs into the existing download-source picker on settings → downloads — pick "SoundCloud Only" or include it in the hybrid order alongside soulseek / youtube / tidal / qobuz / hifi / deezer / lidarr. anonymous-only (no account needed); quality is whatever soundcloud serves anonymously, typically 128 kbps mp3 or aac depending on the upload. soundcloud doesn\'t expose lossless to anyone, so don\'t expect flac. follows the exact same wiring contract as every other download source — search dispatch, hybrid fallback, queue / cancel / clear, sidebar source label, provenance + library history all work plug-and-play.', page: 'settings' },
|
||||
{ title: 'Fix Qobuz Connection Not Sticking After Login', desc: 'logging in via the qobuz connect button on settings showed "connected: <username> (active)" but underneath an error said "qobuz not authenticated...", and the dashboard indicator stayed yellow. cause: two separate qobuz client instances run side by side (one for the auth flow, one for the enrichment worker) and login only updated the first one. now the worker\'s client gets synced from config the moment login / token / logout completes, so the dashboard indicator goes green and connection-test stops yelling.', page: 'settings' },
|
||||
|
|
|
|||
Loading…
Reference in a new issue