soulsync/tests/metadata/test_album_mbid_cache.py
Broque Thomas 4b15fe0b75 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.
2026-05-03 17:16:39 -07:00

207 lines
7.5 KiB
Python

"""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()