From 528aedbdfea947c8c991ec1c7b36752071e6773c Mon Sep 17 00:00:00 2001 From: ragnarlotus Date: Thu, 25 Jun 2026 22:54:43 +0200 Subject: [PATCH 01/39] fix(reorganize): include MusicBrainz release IDs --- core/library_reorganize.py | 1 + tests/test_reorganize_canonical_source.py | 24 +++++++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/core/library_reorganize.py b/core/library_reorganize.py index 574d5ee7..a7e4fa37 100644 --- a/core/library_reorganize.py +++ b/core/library_reorganize.py @@ -100,6 +100,7 @@ _ALBUM_ID_COLUMNS = { 'deezer': 'deezer_id', 'discogs': 'discogs_id', 'hydrabase': 'soul_id', + 'musicbrainz': 'musicbrainz_release_id', } # Human-facing label for each source. diff --git a/tests/test_reorganize_canonical_source.py b/tests/test_reorganize_canonical_source.py index 711638ed..bb39c436 100644 --- a/tests/test_reorganize_canonical_source.py +++ b/tests/test_reorganize_canonical_source.py @@ -73,3 +73,27 @@ def test_no_canonical_unchanged(monkeypatch): album_data = {"spotify_album_id": "sp1"} source, _, _ = lr._resolve_source(album_data, "spotify") assert source == "spotify" + + +def test_musicbrainz_release_id_is_used_by_priority_walk(monkeypatch): + _patch_fetch(monkeypatch, { + ('musicbrainz', 'mb-release-1'): [{'name': 'x'}], + }) + monkeypatch.setattr( + lr, + 'get_source_priority', + lambda primary: ['musicbrainz', 'spotify'], + ) + + album_data = { + 'musicbrainz_release_id': 'mb-release-1', + } + + source, api_album, items = lr._resolve_source( + album_data, + 'musicbrainz', + ) + + assert source == 'musicbrainz' + assert api_album == {'name': 'musicbrainz:mb-release-1'} + assert items == [{'name': 'x'}] From 8a6b02b3fdeeefaad2b665479d2571e0bcabe0da Mon Sep 17 00:00:00 2001 From: ragnarlotus Date: Fri, 26 Jun 2026 03:10:07 +0200 Subject: [PATCH 02/39] test(reorganize): verify MusicBrainz integration --- tests/test_reorganize_canonical_source.py | 79 ++++++++++++++++++++--- 1 file changed, 69 insertions(+), 10 deletions(-) diff --git a/tests/test_reorganize_canonical_source.py b/tests/test_reorganize_canonical_source.py index bb39c436..f7a2939b 100644 --- a/tests/test_reorganize_canonical_source.py +++ b/tests/test_reorganize_canonical_source.py @@ -7,7 +7,11 @@ canonical_source/canonical_album_id, and an explicit user source pick from __future__ import annotations +from unittest.mock import MagicMock + import core.library_reorganize as lr +import core.metadata.registry as metadata_registry +from core.musicbrainz_search import MusicBrainzSearchClient def _patch_fetch(monkeypatch, tracklists): @@ -76,24 +80,79 @@ def test_no_canonical_unchanged(monkeypatch): def test_musicbrainz_release_id_is_used_by_priority_walk(monkeypatch): - _patch_fetch(monkeypatch, { - ('musicbrainz', 'mb-release-1'): [{'name': 'x'}], - }) + client = MusicBrainzSearchClient() + client._client = MagicMock() + client._client.get_release_group.return_value = None + client._client.get_release.return_value = { + "id": "mb-release-1", + "title": "Test Album", + "date": "2024-01-01", + "artist-credit": [{"name": "Test Artist"}], + "release-group": { + "id": "mb-group-1", + "primary-type": "Album", + "secondary-types": [], + }, + "media": [ + { + "position": 1, + "tracks": [ + { + "id": "track-1", + "number": "1", + "position": 1, + "length": 180000, + "recording": { + "id": "recording-1", + "title": "Test Track", + "artist-credit": [{"name": "Test Artist"}], + "length": 180000, + }, + }, + ], + }, + ], + } + + monkeypatch.setattr( + metadata_registry, + "get_musicbrainz_client", + lambda *args, **kwargs: client, + ) monkeypatch.setattr( lr, - 'get_source_priority', - lambda primary: ['musicbrainz', 'spotify'], + "get_source_priority", + lambda primary: ["musicbrainz", "spotify"], ) album_data = { - 'musicbrainz_release_id': 'mb-release-1', + "musicbrainz_release_id": "mb-release-1", } source, api_album, items = lr._resolve_source( album_data, - 'musicbrainz', + "musicbrainz", ) - assert source == 'musicbrainz' - assert api_album == {'name': 'musicbrainz:mb-release-1'} - assert items == [{'name': 'x'}] + assert source == "musicbrainz" + assert api_album["id"] == "mb-release-1" + assert api_album["name"] == "Test Album" + assert items == api_album["tracks"] + assert items == [ + { + "id": "recording-1", + "name": "Test Track", + "artists": [{"name": "Test Artist"}], + "duration_ms": 180000, + "track_number": 1, + "disc_number": 1, + }, + ] + client._client.get_release_group.assert_any_call( + "mb-release-1", + includes=["releases", "artist-credits"], + ) + client._client.get_release.assert_any_call( + "mb-release-1", + includes=["recordings", "artist-credits", "release-groups"], + ) From a6364ac2836e8d59d7365cd1c95445cfcdd4e911 Mon Sep 17 00:00:00 2001 From: ragnarlotus Date: Fri, 26 Jun 2026 06:22:07 +0200 Subject: [PATCH 03/39] fix album completeness canonical edition matching --- core/repair_jobs/album_completeness.py | 154 ++++++++++++++----- tests/test_album_completeness_job.py | 204 ++++++++++++++++++++++++- 2 files changed, 319 insertions(+), 39 deletions(-) diff --git a/core/repair_jobs/album_completeness.py b/core/repair_jobs/album_completeness.py index 24b1260f..5744a5bf 100644 --- a/core/repair_jobs/album_completeness.py +++ b/core/repair_jobs/album_completeness.py @@ -21,9 +21,10 @@ class AlbumCompletenessJob(RepairJob): help_text = ( 'Compares the number of tracks you have for each album against the expected total ' 'from your configured metadata sources. Counts cached during normal enrichment are ' - 'used when available; otherwise the job queries a metadata source directly. Albums ' - 'where tracks are missing get flagged as findings with details about which tracks ' - 'are absent.\n\n' + 'used when no canonical edition is pinned; otherwise the exact canonical source and ' + 'album ID are queried directly. The same canonical tracklist is used for both the ' + 'expected total and the missing-track calculation. Albums where tracks are missing ' + 'get flagged as findings with details about which tracks are absent.\n\n' 'Useful for catching partial downloads or albums where some tracks failed to download. ' 'You can use the Download Missing feature from the album page to fill gaps.\n\n' 'Settings:\n' @@ -56,6 +57,7 @@ class AlbumCompletenessJob(RepairJob): has_itunes = False has_deezer = False has_api_track_count = False + has_canonical = False try: conn = context.db._get_connection() cursor = conn.cursor() @@ -67,6 +69,10 @@ class AlbumCompletenessJob(RepairJob): has_deezer = 'deezer_id' in columns has_discogs = 'discogs_id' in columns has_hydrabase = 'soul_id' in columns + has_canonical = ( + 'canonical_source' in columns + and 'canonical_album_id' in columns + ) # Detect the `api_track_count` column — older DBs may not have it # yet (migration runs on app start, but repair-job code mustn't @@ -101,8 +107,13 @@ class AlbumCompletenessJob(RepairJob): select_cols.append(('al.discogs_id', 'discogs_album_id')) if has_hydrabase: select_cols.append(('al.soul_id', 'hydrabase_album_id')) + if has_canonical: + select_cols.extend([ + ('al.canonical_source', 'canonical_source'), + ('al.canonical_album_id', 'canonical_album_id'), + ]) - # WHERE: album has at least one source ID + # WHERE: album has at least one source ID or a complete canonical pair where_parts = ["(al.spotify_album_id IS NOT NULL AND al.spotify_album_id != '')"] if has_itunes: where_parts.append("(al.itunes_album_id IS NOT NULL AND al.itunes_album_id != '')") @@ -112,6 +123,11 @@ class AlbumCompletenessJob(RepairJob): where_parts.append("(al.discogs_id IS NOT NULL AND al.discogs_id != '')") if has_hydrabase: where_parts.append("(al.soul_id IS NOT NULL AND al.soul_id != '')") + if has_canonical: + where_parts.append( + "(al.canonical_source IS NOT NULL AND al.canonical_source != '' " + "AND al.canonical_album_id IS NOT NULL AND al.canonical_album_id != '')" + ) where_clause = ' OR '.join(where_parts) select_sql = ', '.join(f'{expr} AS {alias}' for expr, alias in select_cols) @@ -159,6 +175,8 @@ class AlbumCompletenessJob(RepairJob): deezer_album_id = row[column_index['deezer_album_id']] if 'deezer_album_id' in column_index else None discogs_album_id = row[column_index['discogs_album_id']] if 'discogs_album_id' in column_index else None hydrabase_album_id = row[column_index['hydrabase_album_id']] if 'hydrabase_album_id' in column_index else None + canonical_source = row[column_index['canonical_source']] if 'canonical_source' in column_index else None + canonical_album_id = row[column_index['canonical_album_id']] if 'canonical_album_id' in column_index else None # Cached authoritative track count from a prior API lookup (NULL # on unscanned albums and on DBs predating the column migration). cached_api_count = row[column_index['api_track_count']] if 'api_track_count' in column_index else None @@ -181,20 +199,41 @@ class AlbumCompletenessJob(RepairJob): 'hydrabase': hydrabase_album_id or '', } - # Expected total comes from the metadata provider, NOT from - # al.track_count — that column holds the observed count from - # server syncs (Plex leafCount, SoulSync standalone len(tracks)) - # which by definition always equals actual_count and made the - # job skip every album. Use the cached api_track_count if a - # prior scan already looked it up; otherwise hit the API and - # persist the answer for next time. - expected_total = cached_api_count - if not expected_total: - expected_total = self._get_expected_total(context, primary_source, album_ids) - # Only persist positive results. Zero/None would keep - # re-triggering the lookup on every scan. - if expected_total and expected_total > 0 and has_api_track_count: - self._save_api_track_count(context, album_id, expected_total) + # A complete canonical pair identifies one exact edition, + # independently of provider. Its tracklist must drive both the + # expected total and the missing-track calculation; neither a + # cached count nor another source may silently replace it. + canonical_items = None + resolved_source = primary_source + resolved_album_id = self._get_album_id_for_source(primary_source, album_ids) or '' + + if canonical_source and canonical_album_id: + api_tracks = self._get_album_tracks( + str(canonical_source), + str(canonical_album_id), + ) + canonical_items = self._extract_track_items(api_tracks) + expected_total = len(canonical_items) + resolved_source = str(canonical_source) + resolved_album_id = str(canonical_album_id) + else: + # Preserve the existing behavior for albums that have not + # resolved a canonical edition yet. + expected_total = cached_api_count + if not expected_total: + expected_total = self._get_expected_total( + context, + primary_source, + album_ids, + ) + # Only persist positive results. Zero/None would keep + # re-triggering the lookup on every scan. + if expected_total and expected_total > 0 and has_api_track_count: + self._save_api_track_count( + context, + album_id, + expected_total, + ) # Skip singles/EPs based on expected track count (not local count) if expected_total and expected_total < min_tracks: @@ -224,8 +263,20 @@ class AlbumCompletenessJob(RepairJob): context.update_progress(i + 1, total) continue - # Album is incomplete — try to find which tracks are missing - missing_tracks = self._find_missing_tracks(context, primary_source, album_id, album_ids) + # Album is incomplete — identify missing tracks from the same + # canonical edition used for the expected total when one is pinned. + missing_tracks = self._find_missing_tracks( + context, + primary_source, + album_id, + album_ids, + resolved_source=( + resolved_source + if canonical_items is not None + else None + ), + resolved_items=canonical_items, + ) if context.report_progress: context.report_progress( @@ -250,8 +301,10 @@ class AlbumCompletenessJob(RepairJob): 'album_id': album_id, 'album_title': title, 'artist': artist_name, - 'primary_source': primary_source, - 'primary_album_id': self._get_album_id_for_source(primary_source, album_ids) or '', + 'primary_source': resolved_source, + 'primary_album_id': resolved_album_id, + 'canonical_source': canonical_source or '', + 'canonical_album_id': canonical_album_id or '', 'spotify_album_id': spotify_album_id or '', 'itunes_album_id': itunes_album_id or '', 'deezer_album_id': deezer_album_id or '', @@ -316,8 +369,19 @@ class AlbumCompletenessJob(RepairJob): return 0 - def _find_missing_tracks(self, context, primary_source, album_id, album_ids): - """Identify which specific tracks are missing using the active metadata provider first.""" + def _find_missing_tracks( + self, + context, + primary_source, + album_id, + album_ids, + resolved_source=None, + resolved_items=None, + ): + """Identify missing tracks from one resolved metadata edition. + + Without a supplied edition, preserve the existing source-priority lookup. + """ # Get track numbers we already have owned_numbers = set() conn = None @@ -336,19 +400,33 @@ class AlbumCompletenessJob(RepairJob): if conn: conn.close() - api_tracks = None - for source in get_source_priority(primary_source): - source_album_id = self._get_album_id_for_source(source, album_ids) - if not source_album_id: - continue - api_tracks = self._get_album_tracks(source, source_album_id) - if self._extract_track_items(api_tracks): - break + if resolved_items is None: + api_tracks = None + for source in get_source_priority(primary_source): + source_album_id = self._get_album_id_for_source( + source, + album_ids, + ) + if not source_album_id: + continue + + api_tracks = self._get_album_tracks( + source, + source_album_id, + ) + if self._extract_track_items(api_tracks): + resolved_source = source + break + + items = self._extract_track_items(api_tracks) + else: + items = resolved_items - items = self._extract_track_items(api_tracks) if not items: return [] + track_source = resolved_source or primary_source + # All supported provider responses expose the same core fields once normalized. # items[].track_number, items[].name, items[].disc_number, items[].id, items[].artists missing_tracks = [] @@ -365,7 +443,7 @@ class AlbumCompletenessJob(RepairJob): 'track_number': tn, 'name': item.get('name', ''), 'disc_number': item.get('disc_number', 1), - 'source': item.get('_source', primary_source), + 'source': item.get('_source', track_source), 'source_track_id': item.get('id', ''), 'track_id': item.get('id', ''), 'spotify_track_id': item.get('id', ''), @@ -430,6 +508,14 @@ class AlbumCompletenessJob(RepairJob): where_parts.append("(discogs_id IS NOT NULL AND discogs_id != '')") if 'soul_id' in columns: where_parts.append("(soul_id IS NOT NULL AND soul_id != '')") + if ( + 'canonical_source' in columns + and 'canonical_album_id' in columns + ): + where_parts.append( + "(canonical_source IS NOT NULL AND canonical_source != '' " + "AND canonical_album_id IS NOT NULL AND canonical_album_id != '')" + ) cursor.execute(f""" SELECT COUNT(*) FROM albums diff --git a/tests/test_album_completeness_job.py b/tests/test_album_completeness_job.py index 8d58a03c..bd0885cd 100644 --- a/tests/test_album_completeness_job.py +++ b/tests/test_album_completeness_job.py @@ -420,6 +420,8 @@ class _SharedMemoryDB: deezer_id TEXT, discogs_id TEXT, soul_id TEXT, + canonical_source TEXT, + canonical_album_id TEXT, track_count INTEGER, api_track_count INTEGER ); @@ -442,13 +444,41 @@ class _SharedMemoryDB: ) self._keepalive.commit() - def insert_album(self, album_id, artist_id, title, *, spotify_id=None, - track_count=None, api_track_count=None): + def insert_album( + self, + album_id, + artist_id, + title, + *, + spotify_id=None, + canonical_source=None, + canonical_album_id=None, + track_count=None, + api_track_count=None, + ): self._keepalive.execute( """INSERT INTO albums - (id, artist_id, title, spotify_album_id, track_count, api_track_count) - VALUES (?, ?, ?, ?, ?, ?)""", - (album_id, artist_id, title, spotify_id, track_count, api_track_count), + ( + id, + artist_id, + title, + spotify_album_id, + canonical_source, + canonical_album_id, + track_count, + api_track_count + ) + VALUES (?, ?, ?, ?, ?, ?, ?, ?)""", + ( + album_id, + artist_id, + title, + spotify_id, + canonical_source, + canonical_album_id, + track_count, + api_track_count, + ), ) self._keepalive.commit() @@ -543,6 +573,170 @@ def test_scan_uses_cached_api_track_count_without_expected_total_lookup(monkeypa assert finding['details']['actual_tracks'] == 10 +def test_scan_uses_exact_canonical_edition_for_count_and_missing_tracks( + monkeypatch, +): + db = _SharedMemoryDB() + db.insert_artist('a1', 'Test Artist') + db.insert_album( + 'alb-canonical', + 'a1', + 'Canonical Album', + spotify_id='sp-other-edition', + canonical_source='deezer', + canonical_album_id='dz-canonical', + track_count=2, + api_track_count=99, + ) + db.insert_tracks('alb-canonical', 2) + + calls = [] + + def get_tracks(source, album_id): + calls.append((source, album_id)) + + if source == 'deezer' and album_id == 'dz-canonical': + return { + 'items': [ + { + 'id': f'dz-{number}', + 'name': f'Canonical Track {number}', + 'track_number': number, + 'disc_number': 1, + 'artists': [], + } + for number in range(1, 5) + ], + } + + if source == 'spotify' and album_id == 'sp-other-edition': + return { + 'items': [ + { + 'id': f'sp-{number}', + 'name': f'Other Edition Track {number}', + 'track_number': number, + 'disc_number': 1, + 'artists': [], + } + for number in range(1, 13) + ], + } + + return None + + monkeypatch.setattr( + album_completeness_module, + 'get_album_tracks_for_source', + get_tracks, + ) + monkeypatch.setattr( + album_completeness_module, + 'get_primary_source', + lambda: 'spotify', + ) + monkeypatch.setattr( + album_completeness_module, + 'get_source_priority', + lambda primary: ['spotify', 'deezer'], + ) + + findings = [] + context = _make_job_context( + db, + create_finding=lambda **kwargs: (findings.append(kwargs) or True), + ) + + result = AlbumCompletenessJob().scan(context) + + assert result.findings_created == 1 + assert calls == [('deezer', 'dz-canonical')] + + details = findings[0]['details'] + + assert details['primary_source'] == 'deezer' + assert details['primary_album_id'] == 'dz-canonical' + assert details['canonical_source'] == 'deezer' + assert details['canonical_album_id'] == 'dz-canonical' + assert details['expected_tracks'] == 4 + assert details['actual_tracks'] == 2 + assert [ + track['track_number'] + for track in details['missing_tracks'] + ] == [3, 4] + assert all( + track['source'] == 'deezer' + for track in details['missing_tracks'] + ) + + +def test_scan_does_not_fallback_when_canonical_edition_is_unavailable( + monkeypatch, +): + db = _SharedMemoryDB() + db.insert_artist('a1', 'Test Artist') + db.insert_album( + 'alb-unavailable-canonical', + 'a1', + 'Unavailable Canonical Album', + spotify_id='sp-other-edition', + canonical_source='deezer', + canonical_album_id='dz-unavailable', + track_count=2, + api_track_count=99, + ) + db.insert_tracks('alb-unavailable-canonical', 2) + + calls = [] + + def get_tracks(source, album_id): + calls.append((source, album_id)) + + if source == 'deezer' and album_id == 'dz-unavailable': + return {'items': []} + + return { + 'items': [ + { + 'id': f'sp-{number}', + 'name': f'Other Edition Track {number}', + 'track_number': number, + 'disc_number': 1, + 'artists': [], + } + for number in range(1, 11) + ], + } + + monkeypatch.setattr( + album_completeness_module, + 'get_album_tracks_for_source', + get_tracks, + ) + monkeypatch.setattr( + album_completeness_module, + 'get_primary_source', + lambda: 'spotify', + ) + monkeypatch.setattr( + album_completeness_module, + 'get_source_priority', + lambda primary: ['spotify', 'deezer'], + ) + + findings = [] + context = _make_job_context( + db, + create_finding=lambda **kwargs: (findings.append(kwargs) or True), + ) + + result = AlbumCompletenessJob().scan(context) + + assert result.findings_created == 0 + assert findings == [] + assert calls == [('deezer', 'dz-unavailable')] + + def test_scan_falls_back_to_api_and_persists_count_on_cache_miss(monkeypatch): """Integration: when api_track_count is NULL, scan calls the API, gets the expected total, caches it, and creates the finding.""" From 85d59048460f0f48c53fe3d21050e66e3b48c45a Mon Sep 17 00:00:00 2001 From: ragnarlotus Date: Sat, 27 Jun 2026 00:04:06 +0200 Subject: [PATCH 04/39] fix album completeness fragmented rows --- core/repair_jobs/album_completeness.py | 1239 ++++++++++++++--- ...test_album_completeness_fragmented_rows.py | 496 +++++++ 2 files changed, 1574 insertions(+), 161 deletions(-) create mode 100644 tests/test_album_completeness_fragmented_rows.py diff --git a/core/repair_jobs/album_completeness.py b/core/repair_jobs/album_completeness.py index 5744a5bf..98c68d80 100644 --- a/core/repair_jobs/album_completeness.py +++ b/core/repair_jobs/album_completeness.py @@ -1,5 +1,10 @@ """Album Completeness Checker Job — finds albums missing tracks.""" +import re +import unicodedata +from collections import defaultdict +from difflib import SequenceMatcher + from core.metadata_service import ( get_album_tracks_for_source, get_primary_source, @@ -23,8 +28,10 @@ class AlbumCompletenessJob(RepairJob): 'from your configured metadata sources. Counts cached during normal enrichment are ' 'used when no canonical edition is pinned; otherwise the exact canonical source and ' 'album ID are queried directly. The same canonical tracklist is used for both the ' - 'expected total and the missing-track calculation. Albums where tracks are missing ' - 'get flagged as findings with details about which tracks are absent.\n\n' + 'expected total and the missing-track calculation. Fragmented library rows are ' + 'combined only when they safely match that same canonical edition. Albums where ' + 'tracks are missing get flagged as findings with details about which tracks are ' + 'absent.\n\n' 'Useful for catching partial downloads or albums where some tracks failed to download. ' 'You can use the Download Missing feature from the album page to fill gaps.\n\n' 'Settings:\n' @@ -51,24 +58,28 @@ class AlbumCompletenessJob(RepairJob): min_completion_pct = settings.get('min_completion_pct', 0) primary_source = self._get_primary_source() - # Fetch all albums with ANY external source ID — not just Spotify + # Fetch all albums with ANY external source ID — not just Spotify. albums = [] conn = None has_itunes = False has_deezer = False + has_discogs = False + has_hydrabase = False + has_musicbrainz = False has_api_track_count = False has_canonical = False try: conn = context.db._get_connection() cursor = conn.cursor() - # Check which source columns exist (older DBs may lack some) + # Check which source columns exist (older DBs may lack some). cursor.execute("PRAGMA table_info(albums)") columns = {row[1] for row in cursor.fetchall()} has_itunes = 'itunes_album_id' in columns has_deezer = 'deezer_id' in columns has_discogs = 'discogs_id' in columns has_hydrabase = 'soul_id' in columns + has_musicbrainz = 'musicbrainz_release_id' in columns has_canonical = ( 'canonical_source' in columns and 'canonical_album_id' in columns @@ -90,6 +101,7 @@ class AlbumCompletenessJob(RepairJob): # from metadata-source enrichment) or a live API lookup. select_cols = [ ('al.id', 'album_id'), + ('al.artist_id', 'artist_id'), ('al.title', 'album_title'), ('ar.name', 'artist_name'), ('al.spotify_album_id', 'spotify_album_id'), @@ -107,22 +119,41 @@ class AlbumCompletenessJob(RepairJob): select_cols.append(('al.discogs_id', 'discogs_album_id')) if has_hydrabase: select_cols.append(('al.soul_id', 'hydrabase_album_id')) + if has_musicbrainz: + select_cols.append( + ('al.musicbrainz_release_id', 'musicbrainz_album_id') + ) if has_canonical: select_cols.extend([ ('al.canonical_source', 'canonical_source'), ('al.canonical_album_id', 'canonical_album_id'), ]) - # WHERE: album has at least one source ID or a complete canonical pair - where_parts = ["(al.spotify_album_id IS NOT NULL AND al.spotify_album_id != '')"] + # WHERE: album has at least one source ID or a complete canonical pair. + where_parts = [ + "(al.spotify_album_id IS NOT NULL AND al.spotify_album_id != '')", + ] if has_itunes: - where_parts.append("(al.itunes_album_id IS NOT NULL AND al.itunes_album_id != '')") + where_parts.append( + "(al.itunes_album_id IS NOT NULL AND al.itunes_album_id != '')" + ) if has_deezer: - where_parts.append("(al.deezer_id IS NOT NULL AND al.deezer_id != '')") + where_parts.append( + "(al.deezer_id IS NOT NULL AND al.deezer_id != '')" + ) if has_discogs: - where_parts.append("(al.discogs_id IS NOT NULL AND al.discogs_id != '')") + where_parts.append( + "(al.discogs_id IS NOT NULL AND al.discogs_id != '')" + ) if has_hydrabase: - where_parts.append("(al.soul_id IS NOT NULL AND al.soul_id != '')") + where_parts.append( + "(al.soul_id IS NOT NULL AND al.soul_id != '')" + ) + if has_musicbrainz: + where_parts.append( + "(al.musicbrainz_release_id IS NOT NULL " + "AND al.musicbrainz_release_id != '')" + ) if has_canonical: where_parts.append( "(al.canonical_source IS NOT NULL AND al.canonical_source != '' " @@ -130,7 +161,10 @@ class AlbumCompletenessJob(RepairJob): ) where_clause = ' OR '.join(where_parts) - select_sql = ', '.join(f'{expr} AS {alias}' for expr, alias in select_cols) + select_sql = ', '.join( + f'{expr} AS {alias}' + for expr, alias in select_cols + ) cursor.execute(f""" SELECT {select_sql} FROM albums al @@ -139,8 +173,20 @@ class AlbumCompletenessJob(RepairJob): WHERE {where_clause} GROUP BY al.id """) - albums = cursor.fetchall() - column_index = {alias: idx for idx, (_, alias) in enumerate(select_cols)} + raw_rows = cursor.fetchall() + column_index = { + alias: idx + for idx, (_, alias) in enumerate(select_cols) + } + albums = [ + { + alias: row[idx] + for alias, idx in column_index.items() + } + for row in raw_rows + ] + for order, album in enumerate(albums): + album['_scan_order'] = order except Exception as e: logger.error("Error fetching albums: %s", e, exc_info=True) result.errors += 1 @@ -149,76 +195,105 @@ class AlbumCompletenessJob(RepairJob): if conn: conn.close() - total = len(albums) + # Rows that share a source ID with a canonical album are candidates for + # the same release. A sibling joins the logical album only when at least + # one of its local tracks strictly matches the canonical tracklist. + work_items = self._prepare_work_items(context, albums) + + total = len(work_items) if context.update_progress: context.update_progress(0, total) - logger.info("Checking completeness of %d albums", total) + logger.info("Checking completeness of %d logical albums", total) if context.report_progress: - context.report_progress(phase=f'Checking {total} albums...', total=total) + context.report_progress( + phase=f'Checking {total} logical albums...', + total=total, + ) - for i, row in enumerate(albums): + for i, work_item in enumerate(work_items): if context.check_stop(): return result if i % 10 == 0 and context.wait_if_paused(): return result - album_id = row[column_index['album_id']] - title = row[column_index['album_title']] - artist_name = row[column_index['artist_name']] - spotify_album_id = row[column_index['spotify_album_id']] - actual_count = row[column_index['actual_count']] - album_thumb = row[column_index['album_thumb_url']] - artist_thumb = row[column_index['artist_thumb_url']] - itunes_album_id = row[column_index['itunes_album_id']] if 'itunes_album_id' in column_index else None - deezer_album_id = row[column_index['deezer_album_id']] if 'deezer_album_id' in column_index else None - discogs_album_id = row[column_index['discogs_album_id']] if 'discogs_album_id' in column_index else None - hydrabase_album_id = row[column_index['hydrabase_album_id']] if 'hydrabase_album_id' in column_index else None - canonical_source = row[column_index['canonical_source']] if 'canonical_source' in column_index else None - canonical_album_id = row[column_index['canonical_album_id']] if 'canonical_album_id' in column_index else None - # Cached authoritative track count from a prior API lookup (NULL - # on unscanned albums and on DBs predating the column migration). - cached_api_count = row[column_index['api_track_count']] if 'api_track_count' in column_index else None + row = work_item['row'] + album_id = row['album_id'] + title = row['album_title'] + artist_name = row['artist_name'] + spotify_album_id = row['spotify_album_id'] + actual_count = int(row['actual_count'] or 0) + album_thumb = row['album_thumb_url'] + artist_thumb = row['artist_thumb_url'] + itunes_album_id = row.get('itunes_album_id') + deezer_album_id = row.get('deezer_album_id') + discogs_album_id = row.get('discogs_album_id') + hydrabase_album_id = row.get('hydrabase_album_id') + musicbrainz_album_id = row.get('musicbrainz_album_id') + canonical_source = row.get('canonical_source') + canonical_album_id = row.get('canonical_album_id') + cached_api_count = row.get('api_track_count') result.scanned += 1 if context.report_progress: context.report_progress( - scanned=i + 1, total=total, + scanned=i + 1, + total=total, phase=f'Checking {i + 1} / {total}', - log_line=f'Album: {title or "Unknown"} — {artist_name or "Unknown"}', - log_type='info' + log_line=( + f'Album: {title or "Unknown"} — ' + f'{artist_name or "Unknown"}' + ), + log_type='info', ) - album_ids = { - 'spotify': spotify_album_id or '', - 'itunes': itunes_album_id or '', - 'deezer': deezer_album_id or '', - 'discogs': discogs_album_id or '', - 'hydrabase': hydrabase_album_id or '', - } - - # A complete canonical pair identifies one exact edition, - # independently of provider. Its tracklist must drive both the - # expected total and the missing-track calculation; neither a - # cached count nor another source may silently replace it. - canonical_items = None + album_ids = self._source_ids_from_row(row) resolved_source = primary_source - resolved_album_id = self._get_album_id_for_source(primary_source, album_ids) or '' + resolved_album_id = ( + self._get_album_id_for_source(primary_source, album_ids) + or '' + ) + related_album_ids = ( + work_item.get('related_album_ids') + or [album_id] + ) + raw_local_count = work_item.get('raw_local_count') + missing_tracks = None + # A complete canonical pair is authoritative. `_prepare_work_items` + # performs exactly one lookup per canonical group and stores the + # resulting list — including an empty list when the lookup failed. if canonical_source and canonical_album_id: - api_tracks = self._get_album_tracks( - str(canonical_source), - str(canonical_album_id), - ) - canonical_items = self._extract_track_items(api_tracks) + canonical_items = work_item.get('canonical_items', []) expected_total = len(canonical_items) resolved_source = str(canonical_source) resolved_album_id = str(canonical_album_id) + + if canonical_items: + actual_count = int( + work_item.get( + 'effective_actual_count', + actual_count, + ) + ) + owned_reference_indexes = set( + work_item.get( + 'owned_reference_indexes', + set(), + ) + ) + missing_tracks = self._build_missing_tracks( + canonical_items, + owned_reference_indexes, + resolved_source, + ) + if raw_local_count is None: + raw_local_count = actual_count else: - # Preserve the existing behavior for albums that have not - # resolved a canonical edition yet. + # Preserve the existing behavior for albums without a pinned + # canonical edition. expected_total = cached_api_count if not expected_total: expected_total = self._get_expected_total( @@ -226,16 +301,18 @@ class AlbumCompletenessJob(RepairJob): primary_source, album_ids, ) - # Only persist positive results. Zero/None would keep - # re-triggering the lookup on every scan. - if expected_total and expected_total > 0 and has_api_track_count: + if ( + expected_total + and expected_total > 0 + and has_api_track_count + ): self._save_api_track_count( context, album_id, expected_total, ) - # Skip singles/EPs based on expected track count (not local count) + # Skip singles/EPs based on expected track count (not local count). if expected_total and expected_total < min_tracks: result.skipped += 1 if context.update_progress and (i + 1) % 5 == 0: @@ -248,13 +325,15 @@ class AlbumCompletenessJob(RepairJob): context.update_progress(i + 1, total) continue - # Skip albums with zero local tracks — nothing to auto-fill from - if actual_count == 0: + effective_raw_local_count = ( + raw_local_count + if raw_local_count is not None + else int(row['actual_count'] or 0) + ) + if actual_count == 0 and effective_raw_local_count == 0: result.skipped += 1 continue - # Skip albums below minimum completion percentage - # (filters out "1 track from a playlist import" false positives) if min_completion_pct > 0 and expected_total > 0: completion = (actual_count / expected_total) * 100 if completion < min_completion_pct: @@ -263,26 +342,23 @@ class AlbumCompletenessJob(RepairJob): context.update_progress(i + 1, total) continue - # Album is incomplete — identify missing tracks from the same - # canonical edition used for the expected total when one is pinned. - missing_tracks = self._find_missing_tracks( - context, - primary_source, - album_id, - album_ids, - resolved_source=( - resolved_source - if canonical_items is not None - else None - ), - resolved_items=canonical_items, - ) + if missing_tracks is None: + missing_tracks = self._find_missing_tracks( + context, + primary_source, + album_id, + album_ids, + ) if context.report_progress: context.report_progress( - log_line=f'Incomplete: {title or "Unknown"} ({actual_count}/{expected_total})', - log_type='skip' + log_line=( + f'Incomplete: {title or "Unknown"} ' + f'({actual_count}/{expected_total})' + ), + log_type='skip', ) + if context.create_finding: try: inserted = context.create_finding( @@ -292,10 +368,13 @@ class AlbumCompletenessJob(RepairJob): entity_type='album', entity_id=str(album_id), file_path=None, - title=f'Incomplete: {title or "Unknown"} ({actual_count}/{expected_total})', + title=( + f'Incomplete: {title or "Unknown"} ' + f'({actual_count}/{expected_total})' + ), description=( - f'Album "{title}" by {artist_name or "Unknown"} has {actual_count} of ' - f'{expected_total} tracks' + f'Album "{title}" by {artist_name or "Unknown"} ' + f'has {actual_count} of {expected_total} tracks' ), details={ 'album_id': album_id, @@ -310,19 +389,31 @@ class AlbumCompletenessJob(RepairJob): 'deezer_album_id': deezer_album_id or '', 'discogs_album_id': discogs_album_id or '', 'hydrabase_album_id': hydrabase_album_id or '', + 'musicbrainz_album_id': ( + musicbrainz_album_id or '' + ), 'expected_tracks': expected_total, 'actual_tracks': actual_count, + 'raw_local_tracks': effective_raw_local_count, + 'related_album_ids': [ + str(value) + for value in related_album_ids + ], 'missing_tracks': missing_tracks, 'album_thumb_url': album_thumb or None, 'artist_thumb_url': artist_thumb or None, - } + }, ) if inserted: result.findings_created += 1 else: result.findings_skipped_dedup += 1 except Exception as e: - logger.debug("Error creating completeness finding for album %s: %s", album_id, e) + logger.debug( + "Error creating completeness finding for album %s: %s", + album_id, + e, + ) result.errors += 1 if context.update_progress and (i + 1) % 5 == 0: @@ -331,42 +422,831 @@ class AlbumCompletenessJob(RepairJob): if context.update_progress: context.update_progress(total, total) - logger.info("Completeness check: %d albums checked, %d incomplete found", - result.scanned, result.findings_created) + logger.info( + "Completeness check: %d logical albums checked, %d incomplete found", + result.scanned, + result.findings_created, + ) return result - def _save_api_track_count(self, context, album_id, count): - """Persist a metadata-API track count via the shared worker helper. + def _prepare_work_items(self, context, albums): + """Collapse safely validated fragments into logical canonical albums.""" + groups = self._build_candidate_groups(albums) + work_items = [] - Enrichment workers call `set_album_api_track_count` inside their own - `_update_album` transaction. Here we're in the repair job's fallback - path (the album wasn't enriched yet), so we own the connection + - commit ourselves. A cache-write failure must never break the scan, - so all errors are swallowed into the debug log. - """ + for group in groups: + anchor = group['anchor'] + members = group['members'] + canonical_source = anchor.get('canonical_source') or '' + canonical_album_id = anchor.get('canonical_album_id') or '' + + if canonical_source and canonical_album_id: + canonical_tracks = self._get_album_tracks( + str(canonical_source), + str(canonical_album_id), + ) + canonical_items = self._extract_track_items( + canonical_tracks + ) + + if canonical_items: + local_by_album = { + str(member['album_id']): self._load_local_tracks( + context, + [member['album_id']], + ) + for member in members + } + anchor_tracks = local_by_album.get( + str(anchor['album_id']), + [], + ) + + anchor_reference, _ = self._match_tracks( + canonical_items, + anchor_tracks, + str(canonical_source), + ) + # The anchor's persisted disc/track slots remain + # authoritative even when titles or durations drift. + anchor_reference.update( + self._reference_slots_for_local_tracks( + canonical_items, + anchor_tracks, + ) + ) + + included = [anchor] + excluded = [] + supplemental_reference = set() + + for member in members: + if member is anchor: + continue + + member_tracks = local_by_album.get( + str(member['album_id']), + [], + ) + matched_reference, _ = ( + self._match_fragment_tracks( + canonical_items, + member_tracks, + str(canonical_source), + ) + ) + if matched_reference: + included.append(member) + supplemental_reference.update( + matched_reference - anchor_reference + ) + else: + excluded.append(member) + + combined_local = [] + for member in included: + combined_local.extend( + local_by_album.get( + str(member['album_id']), + [], + ) + ) + + owned_reference_indexes = ( + anchor_reference | supplemental_reference + ) + work_items.append({ + 'row': anchor, + 'canonical_items': canonical_items, + 'related_album_ids': [ + member['album_id'] + for member in included + ], + 'raw_local_count': len(combined_local), + 'effective_actual_count': ( + int(anchor.get('actual_count') or 0) + + len(supplemental_reference) + ), + 'owned_reference_indexes': ( + owned_reference_indexes + ), + '_scan_order': min( + member['_scan_order'] + for member in included + ), + }) + + for member in excluded: + work_items.append( + self._independent_work_item( + member, + canonical_items=( + canonical_items + if self._same_canonical_pair( + member, + canonical_source, + canonical_album_id, + ) + else None + ), + ) + ) + continue + + # The canonical lookup was attempted and returned no usable + # tracklist. Canonical rows must not fall back to another + # provider; non-canonical siblings remain independent. + for member in members: + work_items.append( + self._independent_work_item( + member, + canonical_items=( + [] + if self._same_canonical_pair( + member, + canonical_source, + canonical_album_id, + ) + else None + ), + ) + ) + continue + + for member in members: + work_items.append( + self._independent_work_item(member) + ) + + work_items.sort(key=lambda item: item['_scan_order']) + return work_items + + def _independent_work_item(self, row, canonical_items=None): + item = { + 'row': row, + 'related_album_ids': [row['album_id']], + '_scan_order': row['_scan_order'], + } + if canonical_items is not None: + item['canonical_items'] = canonical_items + return item + + def _same_canonical_pair( + self, + row, + canonical_source, + canonical_album_id, + ): + return ( + str(row.get('canonical_source') or '') + == str(canonical_source) + and str(row.get('canonical_album_id') or '') + == str(canonical_album_id) + ) + + def _build_candidate_groups(self, albums): + """Group non-canonical rows around unambiguous canonical anchors.""" + canonical_groups = defaultdict(list) + + for album in albums: + source = album.get('canonical_source') or '' + canonical_id = album.get('canonical_album_id') or '' + if source and canonical_id: + key = ( + str(album.get('artist_id') or ''), + str(source), + str(canonical_id), + ) + canonical_groups[key].append(album) + + canonical_row_ids = { + str(row['album_id']) + for rows in canonical_groups.values() + for row in rows + } + + alias_to_groups = defaultdict(set) + for key, anchor_rows in canonical_groups.items(): + for row in anchor_rows: + artist_id = str(row.get('artist_id') or '') + for source, source_id in ( + self._source_ids_from_row(row).items() + ): + if source_id: + alias_to_groups[ + (artist_id, source, str(source_id)) + ].add(key) + + canonical_source = ( + row.get('canonical_source') or '' + ) + canonical_id = ( + row.get('canonical_album_id') or '' + ) + alias_to_groups[ + ( + artist_id, + str(canonical_source), + str(canonical_id), + ) + ].add(key) + + assigned_candidate_ids = set() + groups = [] + + for key, anchor_rows in canonical_groups.items(): + anchor = max( + anchor_rows, + key=lambda row: ( + int(row.get('actual_count') or 0), + 1 if row.get('album_title') else 0, + -int(row.get('_scan_order') or 0), + ), + ) + members = list(anchor_rows) + + for row in albums: + row_id = str(row['album_id']) + if ( + row_id in canonical_row_ids + or row_id in assigned_candidate_ids + ): + continue + + artist_id = str(row.get('artist_id') or '') + matches = set() + for source, source_id in ( + self._source_ids_from_row(row).items() + ): + if source_id: + matches.update( + alias_to_groups.get( + ( + artist_id, + source, + str(source_id), + ), + set(), + ) + ) + + if matches == {key}: + members.append(row) + assigned_candidate_ids.add(row_id) + + groups.append({ + 'anchor': anchor, + 'members': sorted( + members, + key=lambda row: row['_scan_order'], + ), + '_scan_order': min( + row['_scan_order'] + for row in members + ), + }) + + grouped_ids = canonical_row_ids | assigned_candidate_ids + for row in albums: + if str(row['album_id']) in grouped_ids: + continue + groups.append({ + 'anchor': row, + 'members': [row], + '_scan_order': row['_scan_order'], + }) + + groups.sort(key=lambda group: group['_scan_order']) + return groups + + def _source_ids_from_row(self, row): + """Return every stored release ID available on an album row.""" + return { + 'spotify': row.get('spotify_album_id') or '', + 'itunes': row.get('itunes_album_id') or '', + 'deezer': row.get('deezer_album_id') or '', + 'discogs': row.get('discogs_album_id') or '', + 'hydrabase': row.get('hydrabase_album_id') or '', + 'musicbrainz': row.get('musicbrainz_album_id') or '', + } + + def _load_local_tracks(self, context, album_ids): + """Load local tracks while tolerating older track-table schemas.""" + ids = [ + str(album_id) + for album_id in album_ids + if album_id is not None + ] + if not ids: + return [] + + placeholders = ','.join('?' for _ in ids) conn = None try: conn = context.db._get_connection() cursor = conn.cursor() - set_album_api_track_count(cursor, album_id, count) - conn.commit() + cursor.execute("PRAGMA table_info(tracks)") + columns = { + row[1] + for row in cursor.fetchall() + } + + select_cols = [ + ( + 'id' if 'id' in columns else 'NULL', + 'id', + ), + ('album_id', 'album_id'), + ( + 'title' if 'title' in columns else "''", + 'title', + ), + ( + 'track_number' + if 'track_number' in columns + else 'NULL', + 'track_number', + ), + ( + 'disc_number' + if 'disc_number' in columns + else '1', + 'disc_number', + ), + ( + 'duration' + if 'duration' in columns + else '0', + 'duration', + ), + ( + 'musicbrainz_recording_id' + if 'musicbrainz_recording_id' in columns + else "''", + 'musicbrainz_recording_id', + ), + ] + select_sql = ', '.join( + f'{expression} AS {alias}' + for expression, alias in select_cols + ) + cursor.execute( + f""" + SELECT {select_sql} + FROM tracks + WHERE album_id IN ({placeholders}) + """, + ids, + ) + return [ + { + 'id': row[0], + 'album_id': row[1], + 'title': row[2] or '', + 'track_number': row[3], + 'disc_number': ( + row[4] + if row[4] is not None + else 1 + ), + 'duration_ms': row[5] or 0, + 'musicbrainz_recording_id': row[6] or '', + } + for row in cursor.fetchall() + ] except Exception as e: - logger.debug("Failed to cache api_track_count for album %s: %s", album_id, e) + logger.debug( + "Failed loading local tracks for albums %s: %s", + ids, + e, + ) + return [] finally: if conn: conn.close() - def _get_expected_total(self, context, primary_source, album_ids): - """Try to get the expected track count from the active metadata provider first.""" + def _normalize_title(self, value): + text = unicodedata.normalize('NFKD', str(value or '')) + text = ''.join( + char + for char in text + if not unicodedata.combining(char) + ) + text = text.casefold() + text = re.sub(r'[\W_]+', ' ', text, flags=re.UNICODE) + return ' '.join(text.split()) + + def _as_int(self, value, default=None): + if value is None or value == '': + return default + if isinstance(value, bool): + return int(value) + if isinstance(value, (int, float)): + return int(value) + match = re.search(r'\d+', str(value)) + return int(match.group(0)) if match else default + + def _reference_duration_ms(self, item): + value = item.get('duration_ms') + if value not in (None, ''): + try: + return int(float(value)) + except (TypeError, ValueError): + return 0 + + value = item.get('duration') + if value not in (None, ''): + try: + return int(float(value) * 1000) + except (TypeError, ValueError): + return 0 + return 0 + + def _reference_slots_for_local_tracks( + self, + reference_items, + local_tracks, + ): + """Map unambiguous local disc/track slots to reference indexes.""" + slots = defaultdict(list) + for index, item in enumerate(reference_items): + number = self._as_int(item.get('track_number')) + disc = self._as_int( + item.get('disc_number'), + 1, + ) + if number is not None: + slots[(disc, number)].append(index) + + matched = set() + for track in local_tracks: + number = self._as_int(track.get('track_number')) + disc = self._as_int( + track.get('disc_number'), + 1, + ) + indexes = slots.get((disc, number), []) + if len(indexes) == 1: + matched.add(indexes[0]) + return matched + + def _track_match_score( + self, + reference, + local, + reference_source, + ): + """Return a score when a local track plausibly matches a reference.""" + reference_id = str(reference.get('id') or '') + local_mbid = str( + local.get('musicbrainz_recording_id') or '' + ) + if ( + reference_source == 'musicbrainz' + and reference_id + and reference_id == local_mbid + ): + return 1000 + + reference_number = self._as_int( + reference.get('track_number') + ) + local_number = self._as_int( + local.get('track_number') + ) + reference_disc = self._as_int( + reference.get('disc_number'), + 1, + ) + local_disc = self._as_int( + local.get('disc_number'), + 1, + ) + + same_number = ( + reference_number is not None + and local_number is not None + and reference_number == local_number + ) + same_slot = same_number and reference_disc == local_disc + + reference_title = self._normalize_title( + reference.get('name') + or reference.get('title') + ) + local_title = self._normalize_title( + local.get('title') + ) + title_ratio = ( + SequenceMatcher( + None, + reference_title, + local_title, + ).ratio() + if reference_title and local_title + else 0.0 + ) + exact_title = bool( + reference_title + and reference_title == local_title + ) + + reference_duration = self._reference_duration_ms( + reference + ) + local_duration = ( + self._as_int( + local.get('duration_ms'), + 0, + ) + or 0 + ) + has_durations = ( + reference_duration > 0 + and local_duration > 0 + ) + duration_close = ( + has_durations + and abs( + reference_duration - local_duration + ) <= 15000 + ) + + if same_slot and title_ratio >= 0.65: + return ( + 800 + + int(title_ratio * 100) + + (30 if duration_close else 0) + ) + if same_slot and duration_close: + return 740 + int(title_ratio * 100) + if exact_title: + return ( + 700 + + (50 if duration_close else 0) + + (20 if same_number else 0) + ) + if ( + title_ratio >= 0.92 + and (duration_close or not has_durations) + ): + return 650 + int(title_ratio * 100) + if ( + same_number + and title_ratio >= 0.80 + and duration_close + ): + return 620 + int(title_ratio * 100) + return None + + def _fragment_track_match_score( + self, + reference, + local, + reference_source, + ): + """Use stricter matching when accepting a sibling fragment.""" + reference_id = str(reference.get('id') or '') + local_mbid = str( + local.get('musicbrainz_recording_id') or '' + ) + if ( + reference_source == 'musicbrainz' + and reference_id + and reference_id == local_mbid + ): + return 1000 + + reference_title = self._normalize_title( + reference.get('name') + or reference.get('title') + ) + local_title = self._normalize_title( + local.get('title') + ) + if not reference_title or not local_title: + return None + + title_ratio = SequenceMatcher( + None, + reference_title, + local_title, + ).ratio() + exact_title = reference_title == local_title + + reference_number = self._as_int( + reference.get('track_number') + ) + local_number = self._as_int( + local.get('track_number') + ) + reference_disc = self._as_int( + reference.get('disc_number'), + 1, + ) + local_disc = self._as_int( + local.get('disc_number'), + 1, + ) + same_slot = ( + reference_number is not None + and local_number is not None + and reference_number == local_number + and reference_disc == local_disc + ) + + reference_duration = self._reference_duration_ms( + reference + ) + local_duration = ( + self._as_int( + local.get('duration_ms'), + 0, + ) + or 0 + ) + duration_close = ( + reference_duration > 0 + and local_duration > 0 + and abs( + reference_duration - local_duration + ) <= 15000 + ) + + if exact_title: + return ( + 900 + + (30 if same_slot else 0) + + (20 if duration_close else 0) + ) + if ( + title_ratio >= 0.95 + and (same_slot or duration_close) + ): + return 800 + int(title_ratio * 100) + if ( + title_ratio >= 0.85 + and same_slot + and duration_close + ): + return 700 + int(title_ratio * 100) + return None + + def _match_fragment_tracks( + self, + reference_items, + local_tracks, + reference_source, + ): + """One-to-one strict match used only for candidate siblings.""" + return self._greedy_match( + reference_items, + local_tracks, + reference_source, + self._fragment_track_match_score, + ) + + def _match_tracks( + self, + reference_items, + local_tracks, + reference_source, + ): + """One-to-one general match for tracks already on the anchor.""" + return self._greedy_match( + reference_items, + local_tracks, + reference_source, + self._track_match_score, + ) + + def _greedy_match( + self, + reference_items, + local_tracks, + reference_source, + scorer, + ): + candidates = [] + for reference_index, reference in enumerate( + reference_items + ): + for local_index, local in enumerate(local_tracks): + score = scorer( + reference, + local, + reference_source, + ) + if score is not None: + candidates.append( + ( + score, + reference_index, + local_index, + ) + ) + + candidates.sort(reverse=True) + matched_reference = set() + matched_local = set() + + for _, reference_index, local_index in candidates: + if ( + reference_index in matched_reference + or local_index in matched_local + ): + continue + matched_reference.add(reference_index) + matched_local.add(local_index) + + return matched_reference, matched_local + + def _build_missing_tracks( + self, + reference_items, + matched_reference, + reference_source, + ): + missing_tracks = [] + for index, item in enumerate(reference_items): + if index in matched_reference: + continue + + track_artists = [] + for artist in item.get('artists', []): + if isinstance(artist, dict): + track_artists.append( + artist.get('name', '') + ) + elif isinstance(artist, str): + track_artists.append(artist) + + source_track_id = item.get('id', '') + missing_tracks.append({ + 'track_number': item.get('track_number'), + 'name': ( + item.get('name') + or item.get('title') + or '' + ), + 'disc_number': item.get('disc_number', 1), + 'source': item.get( + '_source', + reference_source, + ), + 'source_track_id': source_track_id, + 'track_id': source_track_id, + 'spotify_track_id': source_track_id, + 'duration_ms': self._reference_duration_ms( + item + ), + 'artists': track_artists, + }) + return missing_tracks + + def _save_api_track_count(self, context, album_id, count): + """Persist a metadata-API track count via the shared worker helper.""" + conn = None + try: + conn = context.db._get_connection() + cursor = conn.cursor() + set_album_api_track_count( + cursor, + album_id, + count, + ) + conn.commit() + except Exception as e: + logger.debug( + "Failed to cache api_track_count for album %s: %s", + album_id, + e, + ) + finally: + if conn: + conn.close() + + def _get_expected_total( + self, + context, + primary_source, + album_ids, + ): + """Get the expected count from the active provider priority.""" for source in get_source_priority(primary_source): - album_id = self._get_album_id_for_source(source, album_ids) + album_id = self._get_album_id_for_source( + source, + album_ids, + ) if not album_id: continue - api_tracks = self._get_album_tracks(source, album_id) + api_tracks = self._get_album_tracks( + source, + album_id, + ) items = self._extract_track_items(api_tracks) if items: return len(items) - return 0 def _find_missing_tracks( @@ -378,22 +1258,20 @@ class AlbumCompletenessJob(RepairJob): resolved_source=None, resolved_items=None, ): - """Identify missing tracks from one resolved metadata edition. - - Without a supplied edition, preserve the existing source-priority lookup. - """ - # Get track numbers we already have + """Identify missing tracks from one resolved metadata edition.""" owned_numbers = set() conn = None try: conn = context.db._get_connection() cursor = conn.cursor() cursor.execute( - "SELECT track_number FROM tracks WHERE album_id = ? AND track_number IS NOT NULL", - (album_id,) + "SELECT track_number FROM tracks " + "WHERE album_id = ? " + "AND track_number IS NOT NULL", + (album_id,), ) - for tr in cursor.fetchall(): - owned_numbers.add(tr[0]) + for track in cursor.fetchall(): + owned_numbers.add(track[0]) except Exception: return [] finally: @@ -402,14 +1280,17 @@ class AlbumCompletenessJob(RepairJob): if resolved_items is None: api_tracks = None - for source in get_source_priority(primary_source): - source_album_id = self._get_album_id_for_source( - source, - album_ids, + for source in get_source_priority( + primary_source + ): + source_album_id = ( + self._get_album_id_for_source( + source, + album_ids, + ) ) if not source_album_id: continue - api_tracks = self._get_album_tracks( source, source_album_id, @@ -417,7 +1298,6 @@ class AlbumCompletenessJob(RepairJob): if self._extract_track_items(api_tracks): resolved_source = source break - items = self._extract_track_items(api_tracks) else: items = resolved_items @@ -426,64 +1306,77 @@ class AlbumCompletenessJob(RepairJob): return [] track_source = resolved_source or primary_source - - # All supported provider responses expose the same core fields once normalized. - # items[].track_number, items[].name, items[].disc_number, items[].id, items[].artists - missing_tracks = [] - for item in items: - tn = item.get('track_number') - if tn and tn not in owned_numbers: - track_artists = [] - for a in item.get('artists', []): - if isinstance(a, dict): - track_artists.append(a.get('name', '')) - elif isinstance(a, str): - track_artists.append(a) - missing_tracks.append({ - 'track_number': tn, - 'name': item.get('name', ''), - 'disc_number': item.get('disc_number', 1), - 'source': item.get('_source', track_source), - 'source_track_id': item.get('id', ''), - 'track_id': item.get('id', ''), - 'spotify_track_id': item.get('id', ''), - 'duration_ms': item.get('duration_ms', 0), - 'artists': track_artists, - }) - return missing_tracks + matched_reference = { + index + for index, item in enumerate(items) + if ( + item.get('track_number') + and item.get('track_number') in owned_numbers + ) + } + return self._build_missing_tracks( + items, + matched_reference, + track_source, + ) def _get_settings(self, context: JobContext) -> dict: if not context.config_manager: return self.default_settings.copy() - cfg = context.config_manager.get(f'repair.jobs.{self.job_id}.settings', {}) + cfg = context.config_manager.get( + f'repair.jobs.{self.job_id}.settings', + {}, + ) merged = self.default_settings.copy() merged.update(cfg) return merged def _get_primary_source(self) -> str: - """Return the active metadata source used for source prioritization.""" + """Return the active metadata source for prioritization.""" try: return get_primary_source() except Exception: return 'deezer' - def _get_album_id_for_source(self, source: str, album_ids: dict) -> str: + def _get_album_id_for_source( + self, + source: str, + album_ids: dict, + ) -> str: return album_ids.get(source, '') - def _get_album_tracks(self, source: str, album_id: str): + def _get_album_tracks( + self, + source: str, + album_id: str, + ): """Fetch album tracks from a specific source.""" try: - return get_album_tracks_for_source(source, album_id) + return get_album_tracks_for_source( + source, + album_id, + ) except Exception as e: - logger.debug("Error getting %s album tracks for %s: %s", source.capitalize(), album_id, e) + logger.debug( + "Error getting %s album tracks for %s: %s", + source.capitalize(), + album_id, + e, + ) return None def _extract_track_items(self, api_tracks): - """Normalize album track responses to a list of item dicts.""" + """Normalize provider responses to a list of track dicts.""" if not api_tracks: return [] if isinstance(api_tracks, dict): - items = api_tracks.get('items') or [] + items = ( + api_tracks.get('items') + or api_tracks.get('tracks') + or [] + ) + if isinstance(items, dict): + items = items.get('items') or [] return items if items else [] if isinstance(api_tracks, list): return api_tracks @@ -495,26 +1388,50 @@ class AlbumCompletenessJob(RepairJob): conn = context.db._get_connection() cursor = conn.cursor() - # Check which columns exist cursor.execute("PRAGMA table_info(albums)") - columns = {row[1] for row in cursor.fetchall()} + columns = { + row[1] + for row in cursor.fetchall() + } - where_parts = ["(spotify_album_id IS NOT NULL AND spotify_album_id != '')"] + where_parts = [ + "(spotify_album_id IS NOT NULL " + "AND spotify_album_id != '')", + ] if 'itunes_album_id' in columns: - where_parts.append("(itunes_album_id IS NOT NULL AND itunes_album_id != '')") + where_parts.append( + "(itunes_album_id IS NOT NULL " + "AND itunes_album_id != '')" + ) if 'deezer_id' in columns: - where_parts.append("(deezer_id IS NOT NULL AND deezer_id != '')") + where_parts.append( + "(deezer_id IS NOT NULL " + "AND deezer_id != '')" + ) if 'discogs_id' in columns: - where_parts.append("(discogs_id IS NOT NULL AND discogs_id != '')") + where_parts.append( + "(discogs_id IS NOT NULL " + "AND discogs_id != '')" + ) if 'soul_id' in columns: - where_parts.append("(soul_id IS NOT NULL AND soul_id != '')") + where_parts.append( + "(soul_id IS NOT NULL " + "AND soul_id != '')" + ) + if 'musicbrainz_release_id' in columns: + where_parts.append( + "(musicbrainz_release_id IS NOT NULL " + "AND musicbrainz_release_id != '')" + ) if ( 'canonical_source' in columns and 'canonical_album_id' in columns ): where_parts.append( - "(canonical_source IS NOT NULL AND canonical_source != '' " - "AND canonical_album_id IS NOT NULL AND canonical_album_id != '')" + "(canonical_source IS NOT NULL " + "AND canonical_source != '' " + "AND canonical_album_id IS NOT NULL " + "AND canonical_album_id != '')" ) cursor.execute(f""" diff --git a/tests/test_album_completeness_fragmented_rows.py b/tests/test_album_completeness_fragmented_rows.py new file mode 100644 index 00000000..bfec2a16 --- /dev/null +++ b/tests/test_album_completeness_fragmented_rows.py @@ -0,0 +1,496 @@ +import sqlite3 +import sys +import types +import uuid + + +class _DummyConfigManager: + def get(self, key, default=None): + return default + + def get_active_media_server(self): + return "plex" + + +if "spotipy" not in sys.modules: + spotipy = types.ModuleType("spotipy") + + class _DummySpotify: + def __init__(self, *args, **kwargs): + pass + + oauth2 = types.ModuleType("spotipy.oauth2") + + class _DummyOAuth: + def __init__(self, *args, **kwargs): + pass + + spotipy.Spotify = _DummySpotify + oauth2.SpotifyOAuth = _DummyOAuth + oauth2.SpotifyClientCredentials = _DummyOAuth + spotipy.oauth2 = oauth2 + sys.modules["spotipy"] = spotipy + sys.modules["spotipy.oauth2"] = oauth2 + +if "config.settings" not in sys.modules: + config_pkg = types.ModuleType("config") + settings_mod = types.ModuleType("config.settings") + + settings_mod.config_manager = _DummyConfigManager() + config_pkg.settings = settings_mod + sys.modules["config"] = config_pkg + sys.modules["config.settings"] = settings_mod + +from core.repair_jobs.album_completeness import AlbumCompletenessJob +import core.repair_jobs.album_completeness as album_completeness_module + + +class _SharedMemoryDB: + def __init__(self): + self.uri = ( + f"file:testdb_{uuid.uuid4().hex}" + "?mode=memory&cache=shared" + ) + self._keepalive = sqlite3.connect(self.uri, uri=True) + self._keepalive.executescript( + """ + CREATE TABLE artists ( + id TEXT PRIMARY KEY, + name TEXT, + thumb_url TEXT + ); + + CREATE TABLE albums ( + id TEXT PRIMARY KEY, + artist_id TEXT, + title TEXT, + thumb_url TEXT, + spotify_album_id TEXT, + itunes_album_id TEXT, + deezer_id TEXT, + discogs_id TEXT, + soul_id TEXT, + musicbrainz_release_id TEXT, + canonical_source TEXT, + canonical_album_id TEXT, + api_track_count INTEGER + ); + + CREATE TABLE tracks ( + id TEXT PRIMARY KEY, + album_id TEXT, + title TEXT, + track_number INTEGER, + disc_number INTEGER, + duration INTEGER, + musicbrainz_recording_id TEXT + ); + """ + ) + self._keepalive.commit() + + def _get_connection(self): + return sqlite3.connect(self.uri, uri=True) + + def insert_artist(self, artist_id, name): + self._keepalive.execute( + """ + INSERT INTO artists (id, name) + VALUES (?, ?) + """, + (artist_id, name), + ) + self._keepalive.commit() + + def insert_album( + self, + album_id, + artist_id, + title, + *, + spotify_id=None, + musicbrainz_id=None, + canonical_source=None, + canonical_album_id=None, + api_track_count=None, + ): + self._keepalive.execute( + """ + INSERT INTO albums ( + id, + artist_id, + title, + spotify_album_id, + musicbrainz_release_id, + canonical_source, + canonical_album_id, + api_track_count + ) + VALUES (?, ?, ?, ?, ?, ?, ?, ?) + """, + ( + album_id, + artist_id, + title, + spotify_id, + musicbrainz_id, + canonical_source, + canonical_album_id, + api_track_count, + ), + ) + self._keepalive.commit() + + def insert_track( + self, + album_id, + number, + title, + *, + disc=1, + duration_ms=180000, + mbid=None, + ): + track_id = f"{album_id}-{disc}-{number}-{uuid.uuid4().hex}" + self._keepalive.execute( + """ + INSERT INTO tracks ( + id, + album_id, + title, + track_number, + disc_number, + duration, + musicbrainz_recording_id + ) + VALUES (?, ?, ?, ?, ?, ?, ?) + """, + ( + track_id, + album_id, + title, + number, + disc, + duration_ms, + mbid, + ), + ) + self._keepalive.commit() + + +def _context(db, findings): + return types.SimpleNamespace( + db=db, + transfer_folder='', + config_manager=_DummyConfigManager(), + spotify_client=None, + is_spotify_rate_limited=lambda: False, + stop_event=None, + create_finding=lambda **kwargs: ( + findings.append(kwargs) or True + ), + should_stop=None, + is_paused=None, + update_progress=None, + report_progress=None, + check_stop=lambda: False, + wait_if_paused=lambda: False, + ) + + +def _canonical_tracks(count, *, prefix="Canonical"): + return { + "items": [ + { + "id": f"track-{number}", + "name": f"{prefix} Track {number}", + "track_number": number, + "disc_number": 1, + "duration_ms": 180000 + number, + "artists": [], + } + for number in range(1, count + 1) + ], + } + + +def test_scan_groups_validated_fragmented_rows_into_one_finding( + monkeypatch, +): + db = _SharedMemoryDB() + db.insert_artist("artist-1", "Artist") + db.insert_album( + "anchor", + "artist-1", + "Album", + spotify_id="shared-release", + canonical_source="deezer", + canonical_album_id="canonical-release", + ) + db.insert_album( + "fragment", + "artist-1", + "ALBUM", + spotify_id="shared-release", + api_track_count=2, + ) + + db.insert_track("anchor", 1, "Canonical Track 1") + db.insert_track("anchor", 2, "Canonical Track 2") + db.insert_track("fragment", 3, "Canonical Track 3") + db.insert_track("fragment", 4, "Canonical Track 4") + + calls = [] + + def get_tracks(source, album_id): + calls.append((source, album_id)) + assert source == "deezer" + assert album_id == "canonical-release" + return _canonical_tracks(5) + + monkeypatch.setattr( + album_completeness_module, + "get_album_tracks_for_source", + get_tracks, + ) + monkeypatch.setattr( + album_completeness_module, + "get_primary_source", + lambda: "spotify", + ) + monkeypatch.setattr( + album_completeness_module, + "get_source_priority", + lambda primary: ["spotify", "deezer"], + ) + + findings = [] + result = AlbumCompletenessJob().scan( + _context(db, findings) + ) + + assert result.scanned == 1 + assert result.findings_created == 1 + assert calls == [("deezer", "canonical-release")] + + details = findings[0]["details"] + assert details["album_id"] == "anchor" + assert details["expected_tracks"] == 5 + assert details["actual_tracks"] == 4 + assert details["raw_local_tracks"] == 4 + assert details["related_album_ids"] == [ + "anchor", + "fragment", + ] + assert [ + track["track_number"] + for track in details["missing_tracks"] + ] == [5] + + +def test_shared_id_without_track_match_stays_independent( + monkeypatch, +): + db = _SharedMemoryDB() + db.insert_artist("artist-1", "Artist") + db.insert_album( + "anchor", + "artist-1", + "Album", + spotify_id="shared-release", + canonical_source="deezer", + canonical_album_id="canonical-release", + ) + db.insert_album( + "unrelated", + "artist-1", + "Different Album", + spotify_id="shared-release", + api_track_count=1, + ) + + db.insert_track("anchor", 1, "Canonical Track 1") + db.insert_track( + "unrelated", + 99, + "Completely Unrelated", + duration_ms=900000, + ) + + calls = [] + + def get_tracks(source, album_id): + calls.append((source, album_id)) + return _canonical_tracks(3) + + monkeypatch.setattr( + album_completeness_module, + "get_album_tracks_for_source", + get_tracks, + ) + monkeypatch.setattr( + album_completeness_module, + "get_primary_source", + lambda: "spotify", + ) + monkeypatch.setattr( + album_completeness_module, + "get_source_priority", + lambda primary: ["spotify", "deezer"], + ) + + findings = [] + result = AlbumCompletenessJob().scan( + _context(db, findings) + ) + + assert result.scanned == 2 + assert result.findings_created == 1 + assert calls == [("deezer", "canonical-release")] + assert findings[0]["entity_id"] == "anchor" + assert findings[0]["details"]["related_album_ids"] == [ + "anchor", + ] + + +def test_fragment_grouping_never_crosses_artist_boundary( + monkeypatch, +): + db = _SharedMemoryDB() + db.insert_artist("artist-1", "Artist One") + db.insert_artist("artist-2", "Artist Two") + db.insert_album( + "anchor", + "artist-1", + "Album", + spotify_id="shared-release", + canonical_source="deezer", + canonical_album_id="canonical-release", + ) + db.insert_album( + "other-artist", + "artist-2", + "Album", + spotify_id="shared-release", + api_track_count=1, + ) + + db.insert_track("anchor", 1, "Canonical Track 1") + db.insert_track( + "other-artist", + 2, + "Canonical Track 2", + ) + + calls = [] + + def get_tracks(source, album_id): + calls.append((source, album_id)) + return _canonical_tracks(3) + + monkeypatch.setattr( + album_completeness_module, + "get_album_tracks_for_source", + get_tracks, + ) + monkeypatch.setattr( + album_completeness_module, + "get_primary_source", + lambda: "spotify", + ) + monkeypatch.setattr( + album_completeness_module, + "get_source_priority", + lambda primary: ["spotify", "deezer"], + ) + + findings = [] + result = AlbumCompletenessJob().scan( + _context(db, findings) + ) + + assert result.scanned == 2 + assert result.findings_created == 1 + assert calls == [("deezer", "canonical-release")] + assert findings[0]["details"]["related_album_ids"] == [ + "anchor", + ] + + +def test_musicbrainz_recording_id_validates_fragment( + monkeypatch, +): + db = _SharedMemoryDB() + db.insert_artist("artist-1", "Artist") + db.insert_album( + "anchor", + "artist-1", + "Album", + spotify_id="shared-release", + musicbrainz_id="mb-release", + canonical_source="musicbrainz", + canonical_album_id="mb-release", + ) + db.insert_album( + "fragment", + "artist-1", + "Album Fragment", + spotify_id="shared-release", + api_track_count=1, + ) + + db.insert_track( + "anchor", + 1, + "Canonical Track 1", + mbid="track-1", + ) + db.insert_track( + "fragment", + 99, + "Wrong title and position", + duration_ms=999999, + mbid="track-2", + ) + + calls = [] + + def get_tracks(source, album_id): + calls.append((source, album_id)) + return _canonical_tracks(3) + + monkeypatch.setattr( + album_completeness_module, + "get_album_tracks_for_source", + get_tracks, + ) + monkeypatch.setattr( + album_completeness_module, + "get_primary_source", + lambda: "spotify", + ) + monkeypatch.setattr( + album_completeness_module, + "get_source_priority", + lambda primary: ["spotify", "musicbrainz"], + ) + + findings = [] + result = AlbumCompletenessJob().scan( + _context(db, findings) + ) + + assert result.scanned == 1 + assert result.findings_created == 1 + assert calls == [("musicbrainz", "mb-release")] + + details = findings[0]["details"] + assert details["actual_tracks"] == 2 + assert details["related_album_ids"] == [ + "anchor", + "fragment", + ] + assert [ + track["track_number"] + for track in details["missing_tracks"] + ] == [3] From 31637e00962a5d75ecbb16126a4fee99a88d2532 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Fri, 26 Jun 2026 19:04:24 -0700 Subject: [PATCH 05/39] canonical: recognize musicbrainz as a readable album source (PR #929 follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #929 added 'musicbrainz' to library_reorganize._ALBUM_ID_COLUMNS but not to the canonical layer, breaking the equality invariant test (canonical reads exactly what reorganize reads). add musicbrainz to CANONICAL_ALBUM_SOURCES, and move it from the 'can't pin' param group to the 'pins' group in the manual-lock tests — now consistent, and forward-compatible with pinning a deliberately-matched MB edition. inert at runtime today (mb isn't in the manual source selector, so should_pin is never called with it). 540 canonical/reorganize tests green. --- core/metadata/canonical_version.py | 2 +- tests/test_canonical_manual_lock.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/core/metadata/canonical_version.py b/core/metadata/canonical_version.py index 9871d89e..1f54a1e8 100644 --- a/core/metadata/canonical_version.py +++ b/core/metadata/canonical_version.py @@ -211,7 +211,7 @@ def pick_canonical_release( # core.library_reorganize._ALBUM_ID_COLUMNS — a test pins them in sync). A manual # match on any of these should pin/lock the canonical version (#758); a match on # a source the canonical tools don't read (e.g. lastfm) has no version to pin. -CANONICAL_ALBUM_SOURCES = frozenset({'spotify', 'itunes', 'deezer', 'discogs', 'hydrabase'}) +CANONICAL_ALBUM_SOURCES = frozenset({'spotify', 'itunes', 'deezer', 'discogs', 'hydrabase', 'musicbrainz'}) def should_pin_manual_canonical(entity_type: str, source: str) -> bool: diff --git a/tests/test_canonical_manual_lock.py b/tests/test_canonical_manual_lock.py index ad9fd99e..54f9bcea 100644 --- a/tests/test_canonical_manual_lock.py +++ b/tests/test_canonical_manual_lock.py @@ -24,7 +24,7 @@ from database.music_database import MusicDatabase # should_pin_manual_canonical — pure # --------------------------------------------------------------------------- -@pytest.mark.parametrize('source', ['spotify', 'itunes', 'deezer', 'discogs', 'hydrabase']) +@pytest.mark.parametrize('source', ['spotify', 'itunes', 'deezer', 'discogs', 'hydrabase', 'musicbrainz']) def test_pins_album_on_recognised_source(source): assert should_pin_manual_canonical('album', source) is True @@ -34,7 +34,7 @@ def test_does_not_pin_non_album(entity): assert should_pin_manual_canonical(entity, 'spotify') is False -@pytest.mark.parametrize('source', ['lastfm', 'genius', 'musicbrainz', 'audiodb', 'tidal']) +@pytest.mark.parametrize('source', ['lastfm', 'genius', 'audiodb', 'tidal']) def test_does_not_pin_source_canonical_cant_read(source): # No album-version data the canonical tools read → nothing to pin. assert should_pin_manual_canonical('album', source) is False From e96d62432f9372a2494d21e3b9b58b7c2058da5f Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Fri, 26 Jun 2026 19:56:11 -0700 Subject: [PATCH 06/39] test(album-completeness): stop polluting sys.modules (fix flaky suite failure) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit the file faked spotipy + config.settings in sys.modules at import time with no teardown. the fake config.settings had no ConfigManager, so depending on collection order it leaked into tests/test_config_save_retry and intermittently failed the full suite. the real modules import fine in the test env (spotipy is installed, config.settings has both ConfigManager + config_manager), so the stubs were pure liability — removed them. album tests still pass (10), the album+config combo that errored now passes (17), 573 repair/ config/canonical tests green. --- tests/test_album_completeness_job.py | 34 ++++------------------------ 1 file changed, 4 insertions(+), 30 deletions(-) diff --git a/tests/test_album_completeness_job.py b/tests/test_album_completeness_job.py index bd0885cd..4cca27fc 100644 --- a/tests/test_album_completeness_job.py +++ b/tests/test_album_completeness_job.py @@ -1,4 +1,3 @@ -import sys import types @@ -10,35 +9,10 @@ class _DummyConfigManager: return "plex" -if "spotipy" not in sys.modules: - spotipy = types.ModuleType("spotipy") - - class _DummySpotify: - def __init__(self, *args, **kwargs): - pass - - oauth2 = types.ModuleType("spotipy.oauth2") - - class _DummyOAuth: - def __init__(self, *args, **kwargs): - pass - - spotipy.Spotify = _DummySpotify - oauth2.SpotifyOAuth = _DummyOAuth - oauth2.SpotifyClientCredentials = _DummyOAuth - spotipy.oauth2 = oauth2 - sys.modules["spotipy"] = spotipy - sys.modules["spotipy.oauth2"] = oauth2 - -if "config.settings" not in sys.modules: - config_pkg = types.ModuleType("config") - settings_mod = types.ModuleType("config.settings") - - settings_mod.config_manager = _DummyConfigManager() - config_pkg.settings = settings_mod - sys.modules["config"] = config_pkg - sys.modules["config.settings"] = settings_mod - +# NOTE: deliberately no sys.modules stubbing of spotipy / config.settings here. Both import +# fine in the test env, and faking them globally (with no teardown) leaked into other files — +# it left a config.settings with no ConfigManager, intermittently breaking +# tests/test_config_save_retry depending on collection order. from core.repair_jobs.album_completeness import AlbumCompletenessJob import core.repair_jobs.album_completeness as album_completeness_module From 7e2d2db08df067870dcdda866839119a1d26dc43 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Fri, 26 Jun 2026 20:30:20 -0700 Subject: [PATCH 07/39] watchlist: don't fuse different editions as the same album (Sokhi: Expedition 33) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _normalize_album_for_match stripped ANY trailing '- clause', so a real distinguishing subtitle ('Clair Obscur: Expedition 33 - Nos vies en Lumière (Bonus Edition)') collapsed to the same name as the OST → _albums_likely_match treated them as one album → the watchlist marked unowned tracks of one edition as owned via the other and under-wishlisted. - strip a trailing '- ...' clause ONLY when every token in it is an edition/format qualifier (+ connectors / year-ordinal): '- Single', '- Acoustic Version', '- 2011 Remaster' still collapse, but real subtitles ('- Nos vies en Lumière', '- Volume 2', '- Live in Berlin') are kept. Avoids the inverse regression (a same-album pair splitting into a redownload loop), which a naive narrow strip would have caused. - drop the loose substring shortcut + raise the fuzzy floor 0.6→0.85; genuine drift already collapses to an EXACT match, so the looseness only ever produced false fuses. blast radius: _albums_likely_match has exactly one caller (the allow-duplicates skip). 48 album-match tests pass (qualifier-suffix merges + edition-subtitle splits) + 219 watchlist. --- core/watchlist_scanner.py | 33 ++++++++++++++++++++++------- tests/test_watchlist_album_match.py | 24 +++++++++++++++++++++ 2 files changed, 49 insertions(+), 8 deletions(-) diff --git a/core/watchlist_scanner.py b/core/watchlist_scanner.py index 55d03105..f8eda5e3 100644 --- a/core/watchlist_scanner.py +++ b/core/watchlist_scanner.py @@ -328,6 +328,22 @@ _ALBUM_QUALIFIER_RE = re.compile( re.IGNORECASE, ) +# A trailing "- ..." clause is stripped ONLY when EVERY token in it is an edition/format +# qualifier (+ connectors / a year-ordinal). So "- Single", "- Acoustic Version", "- 2011 +# Remaster" collapse to the base name, but a real distinguishing subtitle ("- Nos vies en +# Lumière", "- Live in Berlin") is kept — the bug was a blanket "- anything$" strip that +# erased subtitles and fused different editions (Sokhi: Expedition 33 OST vs Bonus Edition). +_DASH_QUALIFIER_WORD = ( + r'live|acoustic|electric|instrumental|unplugged|mono|stereo|demos?|reissue|' + r'remix(?:es)?|edit(?:ed)?|radio|single|ep|lp|version|mix(?:es)?|sessions?|bootleg|' + r'covers?|original|redux|deluxe|expanded|remaster(?:ed)?|anniversary|special|' + r'edition|bonus|extended|explicit|clean|soundtrack|ost|score' +) +_TRAILING_DASH_QUALIFIER_RE = re.compile( + r'\s*-\s*(?:(?:' + _DASH_QUALIFIER_WORD + r'|the|a|and|&|\+|\d+(?:st|nd|rd|th)?)\b[\s\-]*)+$', + re.IGNORECASE, +) + def _normalize_album_for_match(name: str) -> str: """Return a canonical form of an album name suitable for fuzzy comparison. @@ -347,8 +363,9 @@ def _normalize_album_for_match(name: str) -> str: # they're almost always edition or commentary noise, not part of the # album's identifying name. cleaned = re.sub(r'\s*[\(\[][^\)\]]*[\)\]]\s*', ' ', cleaned) - # Trailing dash-clauses ("Album - Remastered", "Album - Live") - cleaned = re.sub(r'\s*-\s*[^-]+$', '', cleaned) + # Trailing dash-clause, but ONLY when it's entirely edition/format qualifiers — a real + # subtitle is preserved (see _TRAILING_DASH_QUALIFIER_RE). + cleaned = _TRAILING_DASH_QUALIFIER_RE.sub(' ', cleaned) cleaned = re.sub(r'[^a-z0-9 ]+', ' ', cleaned.lower()) cleaned = re.sub(r'\s+', ' ', cleaned).strip() return cleaned @@ -382,7 +399,7 @@ def _extract_volume_marker(normalized_name: str): return last.group(1) or last.group(2) -def _albums_likely_match(spotify_album: str, lib_album: str, threshold: float = 0.6) -> bool: +def _albums_likely_match(spotify_album: str, lib_album: str, threshold: float = 0.85) -> bool: """Return True when two album names plausibly identify the same release. Designed to swallow naming drift between metadata sources and the @@ -406,11 +423,11 @@ def _albums_likely_match(spotify_album: str, lib_album: str, threshold: float = return False if norm_a == norm_b: return True - # After normalization the shorter name often becomes a prefix / - # substring of the longer one ("napoleon dynamite" ⊂ "napoleon - # dynamite music from the motion picture" before stripping). - if norm_a in norm_b or norm_b in norm_a: - return True + # No loose substring shortcut: after qualifier-stripping, a short name being a + # prefix of a longer one is usually a DIFFERENT edition carrying a real subtitle + # ("clair obscur expedition 33" ⊂ "clair obscur expedition 33 nos vies en lumiere"), + # not naming drift. Genuine drift collapses to an EXACT match above; everything else + # must clear a high overall-similarity bar. return SequenceMatcher(None, norm_a, norm_b).ratio() >= threshold diff --git a/tests/test_watchlist_album_match.py b/tests/test_watchlist_album_match.py index cbe9fbf2..2ededda9 100644 --- a/tests/test_watchlist_album_match.py +++ b/tests/test_watchlist_album_match.py @@ -156,6 +156,13 @@ def test_compilation_score_explanation() -> None: ("Inception (Music From The Motion Picture)", "Inception Soundtrack"), # Substring containment ("Random Access Memories", "Random Access Memories (Bonus Edition)"), + # Dash-suffixed qualifiers must still collapse — these are the SAME album, so + # treating them as different would re-wishlist/redownload forever (the failure the + # original blanket strip guarded against; the narrowed strip must keep covering it). + ("Album Name", "Album Name - Single"), + ("Album Name", "Album Name - Acoustic Version"), + ("Hotel California", "Hotel California - 2013 Remaster"), + ("Album", "Album - The Remixes"), ], ) def test_likely_match_positive(spotify_name, lib_name) -> None: @@ -176,12 +183,29 @@ def test_likely_match_positive(spotify_name, lib_name) -> None: ("Abbey Road", "Sgt. Pepper's Lonely Hearts Club Band"), # Same word in title but different album ("Greatest Hits Volume 1", "Greatest Hits Volume 2"), + # Sokhi: distinct editions of the same franchise — the OST vs a bonus edition + # with a real subtitle. USED to collapse to the same normalized name (the blanket + # trailing-dash strip removed "- Nos vies en Lumière"), so the watchlist marked + # unowned OST tracks as owned via the bonus edition. Must be DIFFERENT albums. + ("Clair Obscur: Expedition 33: Original Soundtrack", + "Clair Obscur: Expedition 33 - Nos vies en Lumière (Bonus Edition)"), + # A real subtitle after a dash must not be stripped down to the base name. + ("The Album", "The Album - A Whole Different Subtitle"), ], ) def test_likely_match_negative(spotify_name, lib_name) -> None: assert not _albums_likely_match(spotify_name, lib_name) +def test_real_subtitle_after_dash_is_preserved() -> None: + # the regression's root cause: a meaningful subtitle must survive normalization, + # while a recognized qualifier after a dash ("- Live", "- 2011") still collapses. + assert _normalize_album_for_match("Clair Obscur: Expedition 33 - Nos vies en Lumière") \ + != _normalize_album_for_match("Clair Obscur: Expedition 33") + assert _normalize_album_for_match("Some Album - Live") == _normalize_album_for_match("Some Album") + assert _normalize_album_for_match("Some Album - 2011") == _normalize_album_for_match("Some Album") + + # --------------------------------------------------------------------------- # _albums_likely_match — defensive cases # --------------------------------------------------------------------------- From b1f061a2a8e61377ba732d5ad25e7c3f42c25dbb Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Fri, 26 Jun 2026 21:45:41 -0700 Subject: [PATCH 08/39] manual search: float the pasted Qobuz/Tidal track to the top (#932) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit a pasted track link IS resolved + searched, but the 'bubble the exact track to the top' step read getattr(t,'id') — and TrackResult has no top-level id (the source id lives in _source_metadata['track_id']). so the bubble was a silent no-op: the linked track sat buried among fuzzy text-search lookalikes and the user saw unrelated tracks. qobuz made it worse — _qobuz_to_track_result never stamped _source_metadata at all, so the track had no id to match. - stamp _source_metadata={'source':'qobuz','track_id':...} on qobuz TrackResults (mirrors tidal) - extract the bubble into pure, tested helpers (linked_track_id / bubble_linked_track_first) that read _source_metadata['track_id'] — fixes it for tidal too, str/int-safe, stable no-op 19 track-link tests (+6 new) + 87 qobuz/download tests green. --- core/downloads/track_link.py | 24 +++++++++++++++- core/qobuz_client.py | 4 +++ tests/downloads/test_track_link.py | 44 ++++++++++++++++++++++++++++++ web_server.py | 12 ++++---- 4 files changed, 77 insertions(+), 7 deletions(-) diff --git a/core/downloads/track_link.py b/core/downloads/track_link.py index e6dec8bd..3565a5ff 100644 --- a/core/downloads/track_link.py +++ b/core/downloads/track_link.py @@ -13,9 +13,31 @@ Pure + import-safe: parsing only, no network. from __future__ import annotations import re -from typing import Any, Optional, Tuple +from typing import Any, List, Optional, Tuple from urllib.parse import urlparse + +def linked_track_id(track: Any) -> str: + """The source track id stamped on a search result, read from + ``_source_metadata['track_id']`` — the field every ID-downloadable source + (Tidal, Qobuz) records. Empty string when absent. ``TrackResult`` has no + top-level ``id``, so callers must NOT use ``getattr(t, 'id')`` (that always + missed and left the pasted-link bubble a silent no-op — #932).""" + meta = getattr(track, '_source_metadata', None) + if not isinstance(meta, dict): + return '' + return str(meta.get('track_id') or '') + + +def bubble_linked_track_first(tracks: List[Any], link_track_id: str) -> List[Any]: + """Float the result whose source id matches a pasted link to the top so the + user sees the EXACT track they linked, not a fuzzy text-search lookalike + (#813/#932). Stable + a graceful no-op when no result carries the id.""" + if not link_track_id or not tracks: + return tracks + target = str(link_track_id) + return sorted(tracks, key=lambda t: linked_track_id(t) != target) + # host substring → download source id. Only ID-downloadable streaming sources. _HOSTS = ( ('tidal.com', 'tidal'), diff --git a/core/qobuz_client.py b/core/qobuz_client.py index 1c7c3b6f..e90e934a 100644 --- a/core/qobuz_client.py +++ b/core/qobuz_client.py @@ -1074,6 +1074,10 @@ class QobuzClient(DownloadSourcePlugin): title=title, album=album_name, track_number=track.get('track_number'), + # Stamp the Qobuz track id so a pasted-link manual search can float the + # exact track to the top (#932). Without this the bubble never matched + # and the linked track stayed buried among fuzzy lookalikes. + _source_metadata={'source': 'qobuz', 'track_id': str(track.get('id') or '')}, ) # Stamp real API quality so the global ranker sees actual diff --git a/tests/downloads/test_track_link.py b/tests/downloads/test_track_link.py index 39dfe8a1..06cbc1bb 100644 --- a/tests/downloads/test_track_link.py +++ b/tests/downloads/test_track_link.py @@ -78,3 +78,47 @@ def test_payload_non_dict_or_empty(): assert q('tidal', None) is None assert q('tidal', {}) is None assert q('qobuz', 'garbage') is None + + +# ── bubble the pasted-link track to the top (#932) ── + +from types import SimpleNamespace + +from core.downloads.track_link import linked_track_id, bubble_linked_track_first + + +def _result(track_id=None): + """Mimic a TrackResult: NO top-level `id`, the source id lives in + _source_metadata['track_id'] (or absent entirely).""" + meta = {'source': 'qobuz', 'track_id': track_id} if track_id is not None else None + return SimpleNamespace(_source_metadata=meta, title='t') + + +def test_linked_track_id_reads_source_metadata(): + assert linked_track_id(_result('296427754')) == '296427754' + + +def test_linked_track_id_empty_when_absent(): + # the exact #932 trap: there is no top-level `id`, so getattr(t,'id') would miss. + r = _result() + assert not hasattr(r, 'id') + assert linked_track_id(r) == '' + + +def test_bubble_floats_exact_track_to_top(): + fuzzy_a, exact, fuzzy_b = _result('111'), _result('296427754'), _result('222') + out = bubble_linked_track_first([fuzzy_a, exact, fuzzy_b], '296427754') + assert out[0] is exact # exact track surfaced first + assert out[1:] == [fuzzy_a, fuzzy_b] # stable order for the rest + + +def test_bubble_handles_int_vs_str_id(): + out = bubble_linked_track_first([_result('999'), _result('296427754')], 296427754) + assert linked_track_id(out[0]) == '296427754' + + +def test_bubble_noop_when_nothing_matches_or_empty(): + a, b = _result('1'), _result('2') + assert bubble_linked_track_first([a, b], '296427754') == [a, b] # unchanged + assert bubble_linked_track_first([], '296427754') == [] + assert bubble_linked_track_first([a, b], '') == [a, b] diff --git a/web_server.py b/web_server.py index b87f2ba7..19f9bc52 100644 --- a/web_server.py +++ b/web_server.py @@ -7533,13 +7533,13 @@ def manual_search_for_task(task_id): "error": error, }) + "\n" continue - # Pasted-link exact match: bubble the track whose id matches - # the link to the top so the user sees the exact version - # first (graceful no-op if ids don't line up). + # Pasted-link exact match: bubble the track whose source id + # matches the link to the top so the user sees the exact version + # first. Reads _source_metadata['track_id'] (TrackResult has no + # top-level id) — the old getattr(t,'id') always missed (#932). if src_name == link_source and link_track_id and tracks: - tracks = sorted( - tracks, - key=lambda t: str(getattr(t, 'id', '')) != str(link_track_id)) + from core.downloads.track_link import bubble_linked_track_first + tracks = bubble_linked_track_first(tracks, link_track_id) serialized = [] for t in tracks: s = _serialize_candidate(t, source_override=src_name) From eddaea2f93a22384f66cc6b66ff3e4f96daca5a8 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Sat, 27 Jun 2026 00:37:35 -0700 Subject: [PATCH 09/39] watchlist history: record automatic scans too (#933) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit save_watchlist_scan_run had a single caller — the manual scan endpoint. the automatic/ scheduled path (process_watchlist_scan_automatically) ran the full scan but never wrote a history row, so nightly scans never showed up in the History modal — only manual ones. - new shared helper persist_scan_run(database, state, ...) extracts the run from the finished watchlist_scan_state and writes one history row - the automatic path now stamps scan_run_id/scan_track_events and calls it - the manual path is refactored onto the same helper so the two can't drift apart again - history is global (no profile filter), so the all-profiles nightly scan records one aggregate row (profile_id None → 1, never NULL) tests: 4 new persist_scan_run seam tests (real DB) + 2 new auto-scan integration tests proving the auto path actually records (completed + cancelled, exactly once). 420 watchlist/automation tests green. --- core/watchlist/auto_scan.py | 17 ++++++++- core/watchlist/scan_history.py | 51 +++++++++++++++++++++++++ tests/test_watchlist_scan_history.py | 56 ++++++++++++++++++++++++++++ tests/watchlist/test_auto_scan.py | 38 +++++++++++++++++++ web_server.py | 20 ++-------- 5 files changed, 165 insertions(+), 17 deletions(-) create mode 100644 core/watchlist/scan_history.py diff --git a/core/watchlist/auto_scan.py b/core/watchlist/auto_scan.py index 759d43f7..e26ab998 100644 --- a/core/watchlist/auto_scan.py +++ b/core/watchlist/auto_scan.py @@ -185,7 +185,11 @@ def process_watchlist_scan_automatically(automation_id=None, profile_id=None, de 'results': [], 'summary': {}, 'error': None, - 'cancel_requested': False + 'cancel_requested': False, + # #933: stamp these so this scan lands in the History modal too — + # the scanner fills scan_track_events; persist_scan_run reads both. + 'scan_run_id': datetime.now().strftime('%Y%m%d-%H%M%S'), + 'scan_track_events': [], } scan_results = [] @@ -294,6 +298,17 @@ def process_watchlist_scan_automatically(automation_id=None, profile_id=None, de total_added_to_wishlist = deps.watchlist_scan_state.get('summary', {}).get('tracks_added_to_wishlist', 0) logger.warning("Automatic watchlist scan cancelled — skipping post-scan steps") + # #933: record this run in the History ledger — same helper the manual + # scan uses, so scheduled scans show up alongside manual ones. + try: + from core.watchlist.scan_history import persist_scan_run + persist_scan_run( + database, deps.watchlist_scan_state, + profile_id=profile_id, was_cancelled=was_cancelled, + ) + except Exception as _hist_err: + logger.error(f"Failed to persist watchlist scan run: {_hist_err}") + # Post-scan steps — skip if cancelled if not was_cancelled: # Populate discovery pool from similar artists (per-profile) diff --git a/core/watchlist/scan_history.py b/core/watchlist/scan_history.py new file mode 100644 index 00000000..a87011ca --- /dev/null +++ b/core/watchlist/scan_history.py @@ -0,0 +1,51 @@ +"""Persist a finished watchlist scan to the History ledger (#831 / #933). + +Both the manual scan (``web_server.start_watchlist_scan``) and the automatic +scan (``core.watchlist.auto_scan.process_watchlist_scan_automatically``) finish +with the same ``watchlist_scan_state`` shape, but only the manual path used to +record a history row — so scheduled/nightly scans never showed up in the +History modal (#933). This single helper is the shared seam: both paths call it, +so they can't drift apart again. + +Pure except for the one ``database.save_watchlist_scan_run`` call — the field +extraction is unit-testable with a fake database. +""" + +from __future__ import annotations + +from datetime import datetime +from typing import Any, Dict, Optional + + +def _iso(value: Any) -> Optional[str]: + """ISO-format a datetime; pass through an already-stringified timestamp.""" + if value is None: + return None + return value.isoformat() if hasattr(value, 'isoformat') else str(value) + + +def persist_scan_run(database: Any, state: Dict[str, Any], *, + profile_id: Any, was_cancelled: bool) -> bool: + """Record one watchlist scan run + its track ledger from ``state``. + + Reads the counts/timestamps/ledger off the live ``watchlist_scan_state`` the + scanner just finished writing, and writes a single history row. ``run_id`` + comes from ``state['scan_run_id']`` (both paths stamp it); a timestamp + fallback keeps it from ever colliding if that's somehow missing. Returns the + DB call's truthiness; callers wrap in their own try/except so a history-write + failure never breaks the scan. + """ + summary = state.get('summary') or {} + run_id = state.get('scan_run_id') or datetime.now().strftime('%Y%m%d-%H%M%S') + return database.save_watchlist_scan_run( + run_id=run_id, + profile_id=profile_id if profile_id else 1, + status='cancelled' if was_cancelled else 'completed', + started_at=_iso(state.get('started_at')), + completed_at=_iso(state.get('completed_at')) or datetime.now().isoformat(), + total_artists=summary.get('total_artists', state.get('total_artists', 0)), + artists_scanned=summary.get('successful_scans', 0), + tracks_found=state.get('tracks_found_this_scan', 0), + tracks_added=state.get('tracks_added_this_scan', 0), + track_events=state.get('scan_track_events') or [], + ) diff --git a/tests/test_watchlist_scan_history.py b/tests/test_watchlist_scan_history.py index 3b6f4924..7281e230 100644 --- a/tests/test_watchlist_scan_history.py +++ b/tests/test_watchlist_scan_history.py @@ -47,6 +47,62 @@ def test_resave_is_idempotent_on_run_id(db): assert len(runs) == 1 and runs[0]['tracks_added'] == 11 +# ── persist_scan_run: the shared seam both scan paths use (#933) ── + +from datetime import datetime + +from core.watchlist.scan_history import persist_scan_run + + +def _state(**over): + """A watchlist_scan_state as the scanner leaves it when a scan finishes.""" + s = { + 'scan_run_id': 'auto-run-1', + 'started_at': datetime(2026, 6, 26, 2, 0, 0), + 'completed_at': datetime(2026, 6, 26, 2, 4, 0), + 'total_artists': 40, + 'tracks_found_this_scan': 7, + 'tracks_added_this_scan': 3, + 'scan_track_events': _events(n_added=3, n_skipped=1), + 'summary': {'total_artists': 40, 'successful_scans': 40, + 'new_tracks_found': 7, 'tracks_added_to_wishlist': 3}, + } + s.update(over) + return s + + +def test_persist_scan_run_records_a_history_row(db): + # the #933 fix: an automatic (all-profiles → profile_id=None) scan must land in History. + assert persist_scan_run(db, _state(), profile_id=None, was_cancelled=False) is True + runs = db.get_watchlist_scan_runs() + assert len(runs) == 1 + r = runs[0] + assert (r['run_id'], r['status'], r['tracks_found'], r['tracks_added']) == \ + ('auto-run-1', 'completed', 7, 3) + assert r['profile_id'] == 1 # None coerced to a concrete profile, never NULL + # the per-run ledger came through too + assert [e['status'] for e in db.get_watchlist_scan_run_events('auto-run-1')] == \ + ['added', 'added', 'added', 'skipped'] + + +def test_persist_scan_run_cancelled_status(db): + persist_scan_run(db, _state(scan_run_id='c1'), profile_id=2, was_cancelled=True) + assert db.get_watchlist_scan_runs()[0]['status'] == 'cancelled' + + +def test_persist_scan_run_accepts_datetime_or_iso_string(db): + # state timestamps may be datetime (auto path) or already-iso strings — both must persist. + persist_scan_run(db, _state(scan_run_id='dt', completed_at='2026-06-26T02:09:00'), + profile_id=1, was_cancelled=False) + assert db.get_watchlist_scan_runs()[0]['run_id'] == 'dt' + + +def test_persist_scan_run_tolerates_sparse_state(db): + # a bare/early-finished state must not raise — history-write must never break a scan. + assert persist_scan_run(db, {'scan_run_id': 'sparse'}, profile_id=None, was_cancelled=False) + assert db.get_watchlist_scan_runs()[0]['tracks_added'] == 0 + + def test_prune_keeps_most_recent(db): for i in range(1, 8): db.save_watchlist_scan_run( diff --git a/tests/watchlist/test_auto_scan.py b/tests/watchlist/test_auto_scan.py index 4ad70258..6ac83413 100644 --- a/tests/watchlist/test_auto_scan.py +++ b/tests/watchlist/test_auto_scan.py @@ -81,6 +81,11 @@ class _FakeDB: self._watchlist_artists = watchlist_artists or [] self._lb_profiles = lb_profiles or [] self.database_path = '/tmp/test.db' + self.scan_runs_saved = [] + + def save_watchlist_scan_run(self, **kwargs): + self.scan_runs_saved.append(kwargs) + return True def get_all_profiles(self): return self._profiles @@ -237,6 +242,39 @@ def test_successful_scan_runs_post_steps(patched_modules): assert deps._state_ref[0]['summary']['new_tracks_found'] == 2 +def test_successful_scan_records_history_run(patched_modules): + """#933: the automatic scan must persist a History row (it used to skip this, + so only manual scans appeared in History).""" + scanner, db = patched_modules + deps = _build_deps() + + autosc.process_watchlist_scan_automatically(automation_id='a1', deps=deps) + + assert len(db.scan_runs_saved) == 1 # exactly one row, no double-record + saved = db.scan_runs_saved[0] + assert saved['status'] == 'completed' + assert saved['artists_scanned'] == 1 # one successful _ScanResult + assert saved['run_id'] # a run id was stamped + + +def test_cancelled_scan_still_records_history_run(patched_modules, monkeypatch): + """A cancelled scan is recorded too, with status='cancelled'.""" + scanner, db = patched_modules + deps = _build_deps() + # Make the scan observe a cancel request mid-run. + orig = scanner.scan_watchlist_artists + def _cancel_during(artists, *, scan_state, progress_callback, cancel_check): + scan_state['cancel_requested'] = True + return orig(artists, scan_state=scan_state, progress_callback=progress_callback, + cancel_check=cancel_check) + monkeypatch.setattr(scanner, 'scan_watchlist_artists', _cancel_during) + + autosc.process_watchlist_scan_automatically(automation_id='a1', deps=deps) + + assert len(db.scan_runs_saved) == 1 + assert db.scan_runs_saved[0]['status'] == 'cancelled' + + def test_completion_emits_automation_event(patched_modules): """Successful scan emits 'watchlist_scan_completed' on automation_engine.""" scanner, _ = patched_modules diff --git a/web_server.py b/web_server.py index 19f9bc52..09614540 100644 --- a/web_server.py +++ b/web_server.py @@ -28223,22 +28223,10 @@ def start_watchlist_scan(): # #831 round 2: persist this run + its track ledger so the # Watchlist History modal can show what every past scan did. try: - _state = watchlist_scan_state - get_database().save_watchlist_scan_run( - run_id=_state.get('scan_run_id') or datetime.now().strftime('%Y%m%d-%H%M%S'), - profile_id=scan_profile_id, - status='cancelled' if was_cancelled else 'completed', - started_at=(_state.get('started_at').isoformat() - if _state.get('started_at') else None), - completed_at=(_state.get('completed_at') or datetime.now()).isoformat() - if not isinstance(_state.get('completed_at'), str) - else _state.get('completed_at'), - total_artists=(_state.get('summary') or {}).get('total_artists', - _state.get('total_artists', 0)), - artists_scanned=(_state.get('summary') or {}).get('successful_scans', 0), - tracks_found=_state.get('tracks_found_this_scan', 0), - tracks_added=_state.get('tracks_added_this_scan', 0), - track_events=_state.get('scan_track_events') or [], + from core.watchlist.scan_history import persist_scan_run + persist_scan_run( + get_database(), watchlist_scan_state, + profile_id=scan_profile_id, was_cancelled=was_cancelled, ) except Exception as _hist_err: logger.error(f"Failed to persist watchlist scan run: {_hist_err}") From 3a92571c719c97e353996e363bc2964fd1814ee8 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Sat, 27 Jun 2026 09:52:39 -0700 Subject: [PATCH 10/39] downloads: stop AcoustID scan duplicating history rows / leaving verified tracks 'unverified' (#934) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit the AcoustID scanner matched library_history rows by EXACT file_path, but that path is frozen at import time while the file moves afterward (media-server import / reorganize) — so tracks.file_path (what the scan reads) no longer equals it. two failures resulted, both introduced in 37ea6604: verified status never reached the history row (verified tracks kept showing 'unverified'), and a fresh acoustid_scan row was INSERTed every run (5551 rows for 3675 songs). - new pure, tested matcher (core/downloads/history_match.py): exact path → filename guarded by title; prefers a real download row over a synthetic scan row. - _persist_status now HEALS the matched row's path + status (so future scans match cleanly), DELETES synthetic acoustid_scan duplicates by exact path (collision-free, never a real row), and inserts only when the file genuinely has no row. - a full AcoustID job now self-cleans existing duplicates — no destructive bulk migration. 8 matcher + 4 real-DB heal/dedup/insert tests; existing scanner tests updated to the new seam (heal vs insert). 1076 acoustid/verification/download tests green. --- core/downloads/history_match.py | 62 ++++++++++++++++++ core/repair_jobs/acoustid_scanner.py | 41 ++++++++++-- tests/test_acoustid_history_heal.py | 95 ++++++++++++++++++++++++++++ tests/test_acoustid_scanner.py | 44 ++++++++----- tests/test_history_match.py | 63 ++++++++++++++++++ 5 files changed, 282 insertions(+), 23 deletions(-) create mode 100644 core/downloads/history_match.py create mode 100644 tests/test_acoustid_history_heal.py create mode 100644 tests/test_history_match.py diff --git a/core/downloads/history_match.py b/core/downloads/history_match.py new file mode 100644 index 00000000..dadea5e1 --- /dev/null +++ b/core/downloads/history_match.py @@ -0,0 +1,62 @@ +"""Match a file back to its download-history row when its path has drifted (#934). + +``library_history.file_path`` is frozen at import time, but the file moves afterward +(media-server import, library reorganize) and ``tracks.file_path`` — what the AcoustID +scanner reads — no longer equals it. Matching on the exact path alone then fails twice: +the verification status never reaches the history row (verified tracks read "unverified"), +and a fresh ``acoustid_scan`` row gets inserted every run (thousands of duplicates). + +This module picks the canonical history row by exact path first, then by FILENAME guarded +by a title check — so a shared filename ("01 - Intro.flac") can never heal the wrong song. +Pure (no DB) so the matching rules are unit-testable; the caller does the SQL. +""" + +from __future__ import annotations + +import os +from typing import Iterable, Optional, Sequence, Tuple + + +def _norm_title(value) -> str: + """Alphanumeric-only lowercase form, so "Song (Remaster)" vs "song remaster" + style drift between the download tag and the media-server tag still agrees.""" + return ''.join(ch for ch in str(value or '').lower() if ch.isalnum()) + + +def like_filename_filter(basename: str) -> str: + """A ``LIKE ... ESCAPE '\\'`` pattern that coarsely matches rows whose path ends + in ``basename``. Escapes the LIKE metacharacters (``%`` ``_`` ``\\``) — filenames + routinely contain underscores. Callers MUST still confirm with an exact basename + compare (``pick_history_row`` does), since ``'%name'`` also matches ``'xname'``.""" + esc = basename.replace('\\', '\\\\').replace('%', '\\%').replace('_', '\\_') + return '%' + esc + + +def pick_history_row(candidates: Sequence[Tuple], *, current_paths: Iterable[str], + basename: str, title: str) -> Optional[int]: + """Return the id of the history row to update for this file, or None. + + ``candidates``: ``(id, file_path, title, download_source)`` rows the DB pre-filtered + (exact path or filename LIKE). A row matches when its path equals the current path OR + its filename matches AND its title agrees — the title guard prevents a shared filename + ("01 - Intro.flac") from healing a different song's row. Among matches a REAL download + row is preferred over a synthetic ``acoustid_scan`` row, so the scanner heals the + genuine record and the caller can delete the synthetic duplicate. None when nothing + matches safely (caller then inserts a fresh row — the "file SoulSync never downloaded" + intent).""" + paths = {p for p in current_paths if p} + want = _norm_title(title) + matches: list = [] # (id, is_exact, is_real) + for cid, cpath, ctitle, csource in candidates: + is_real = csource != 'acoustid_scan' + if cpath and cpath in paths: + matches.append((cid, True, is_real)) + elif (basename and cpath and os.path.basename(cpath) == basename + and (not want or not _norm_title(ctitle) or _norm_title(ctitle) == want)): + matches.append((cid, False, is_real)) + if not matches: + return None + # Prefer a REAL download row over a synthetic acoustid_scan row; within that, prefer an + # exact-path match over a filename match. Stable, so ties keep DB order (first/oldest id). + matches.sort(key=lambda m: (m[2], m[1]), reverse=True) + return matches[0][0] diff --git a/core/repair_jobs/acoustid_scanner.py b/core/repair_jobs/acoustid_scanner.py index 16ceabda..b52ef6aa 100644 --- a/core/repair_jobs/acoustid_scanner.py +++ b/core/repair_jobs/acoustid_scanner.py @@ -389,14 +389,43 @@ class AcoustIDScannerJob(RepairJob): cur.execute( "UPDATE tracks SET verification_status = ? WHERE id = ?", (status, track_id)) - matched = 0 + exp = expected or {} + # Find the canonical history row for this file. The stored path is frozen at + # import time while the file has since moved (media-server import / reorganize), + # so an exact-path match alone misses it — then the status never lands and a + # duplicate row gets inserted every scan (#934). Match exact path first, then + # filename guarded by title, and HEAL the row's path so future scans match cleanly. + from core.downloads.history_match import pick_history_row, like_filename_filter + current = fpath or db_path + basename = os.path.basename(current) if current else '' + clauses, params = [], [] for p in {p for p in (fpath, db_path) if p}: + clauses.append("file_path = ?") + params.append(p) + if basename: + clauses.append("file_path LIKE ? ESCAPE '\\'") + params.append(like_filename_filter(basename)) + row_id = None + if clauses: cur.execute( - "UPDATE library_history SET verification_status = ? WHERE file_path = ?", - (status, p)) - matched += max(getattr(cur, 'rowcount', 0) or 0, 0) - if status == 'unverified' and matched == 0: - exp = expected or {} + "SELECT id, file_path, title, download_source FROM library_history WHERE " + + " OR ".join(clauses), + params) + row_id = pick_history_row( + cur.fetchall(), + current_paths=(fpath, db_path), + basename=basename, title=exp.get('title') or '') + if row_id is not None: + cur.execute( + "UPDATE library_history SET verification_status = ?, file_path = ? WHERE id = ?", + (status, current, row_id)) + # Drop synthetic scan-created duplicates for this exact file (the #934 + # leftovers). Exact path → collision-free; never touches a real download row. + cur.execute( + "DELETE FROM library_history WHERE id != ? AND download_source = 'acoustid_scan' " + "AND file_path = ?", + (row_id, current)) + elif status == 'unverified': cur.execute( """INSERT INTO library_history (event_type, title, artist_name, album_name, file_path, diff --git a/tests/test_acoustid_history_heal.py b/tests/test_acoustid_history_heal.py new file mode 100644 index 00000000..02108e10 --- /dev/null +++ b/tests/test_acoustid_history_heal.py @@ -0,0 +1,95 @@ +"""#934: the AcoustID scanner heals the download-history row when the file moved, +instead of leaving it stuck 'unverified' and inserting duplicate scan rows. + +Seeds the exact bug shape (a real download row at the OLD import path + a synthetic +'acoustid_scan' duplicate at the NEW library path) and drives the real _persist_status, +asserting it collapses to one correct, verified row. +""" + +from __future__ import annotations + +import types + +import pytest + +from core.repair_jobs.acoustid_scanner import AcoustIDScannerJob +from database.music_database import MusicDatabase + + +@pytest.fixture() +def db(tmp_path): + return MusicDatabase(str(tmp_path / 'm.db')) + + +def _scanner(): + # _persist_status uses only `context`, never instance state — bypass __init__. + return AcoustIDScannerJob.__new__(AcoustIDScannerJob) + + +def _rows(db): + with db._get_connection() as conn: + return [dict(r) for r in conn.execute( + "SELECT file_path, download_source, verification_status FROM library_history")] + + +OLD = '/downloads/transfer/Artist/01 - Song.flac' # frozen import path in history +NEW = '/music/Artist/Album/01 - Song.flac' # where the file lives now (tracks path) + + +def test_verify_heals_drifted_row_and_drops_synthetic_dup(db): + # the bug's leftover state: a stuck real row + a synthetic scan dup for the same song. + db.add_library_history_entry('download', 'Song', file_path=OLD, + download_source='soulseek', verification_status='unverified') + db.add_library_history_entry('download', 'Song', file_path=NEW, + download_source='acoustid_scan', verification_status='unverified') + + _scanner()._persist_status( + types.SimpleNamespace(db=db), track_id='t1', fpath=NEW, db_path=NEW, + status='verified', write_tag=False, expected={'title': 'Song'}) + + rows = _rows(db) + assert len(rows) == 1 # synthetic dup deleted, no new insert + assert rows[0]['download_source'] == 'soulseek' # the REAL row survived + assert rows[0]['verification_status'] == 'verified' + assert rows[0]['file_path'] == NEW # path healed to current location + + +def test_idempotent_no_growth_on_rescan(db): + db.add_library_history_entry('download', 'Song', file_path=OLD, + download_source='soulseek', verification_status='unverified') + ctx = types.SimpleNamespace(db=db) + for _ in range(3): + _scanner()._persist_status(ctx, track_id='t1', fpath=NEW, db_path=NEW, + status='verified', write_tag=False, expected={'title': 'Song'}) + rows = _rows(db) + assert len(rows) == 1 and rows[0]['verification_status'] == 'verified' + + +def test_unknown_file_inserts_one_row_then_dedups(db): + # a file SoulSync never downloaded → first scan inserts one review-queue row... + ctx = types.SimpleNamespace(db=db) + _scanner()._persist_status(ctx, track_id='t1', fpath=NEW, db_path=NEW, + status='unverified', write_tag=False, + expected={'title': 'Song', 'artist': 'Artist'}) + assert len(_rows(db)) == 1 + # ...and a rescan matches it (no duplicate). + _scanner()._persist_status(ctx, track_id='t1', fpath=NEW, db_path=NEW, + status='unverified', write_tag=False, + expected={'title': 'Song', 'artist': 'Artist'}) + rows = _rows(db) + assert len(rows) == 1 and rows[0]['download_source'] == 'acoustid_scan' + + +def test_does_not_heal_wrong_song_with_same_filename(db): + # different song, same filename, different title → must stay untouched (no false heal). + db.add_library_history_entry('download', 'A Different Song', file_path='/other/01 - Song.flac', + download_source='soulseek', verification_status='verified') + _scanner()._persist_status( + types.SimpleNamespace(db=db), track_id='t1', fpath=NEW, db_path=NEW, + status='unverified', write_tag=False, expected={'title': 'Song'}) + rows = _rows(db) + # the unrelated row is untouched, and the unknown file got its own new row. + paths = {r['file_path'] for r in rows} + assert '/other/01 - Song.flac' in paths and NEW in paths + other = next(r for r in rows if r['file_path'] == '/other/01 - Song.flac') + assert other['verification_status'] == 'verified' # not corrupted diff --git a/tests/test_acoustid_scanner.py b/tests/test_acoustid_scanner.py index 1c83cac6..7057c968 100644 --- a/tests/test_acoustid_scanner.py +++ b/tests/test_acoustid_scanner.py @@ -4,8 +4,9 @@ from core.repair_jobs.acoustid_scanner import AcoustIDScannerJob class _FakeCursor: - def __init__(self, rows): + def __init__(self, rows, lib_rows=None): self._rows = rows + self._lib_rows = lib_rows or [] self.executed = [] def execute(self, query, params=None): @@ -13,6 +14,10 @@ class _FakeCursor: return self def fetchall(self): + # The #934 history-match SELECT gets its own (id, file_path, title, source) + # rows; the tracks scan query gets the track rows. + if self.executed and 'FROM library_history' in self.executed[-1][0]: + return self._lib_rows return self._rows def fetchone(self): @@ -20,8 +25,8 @@ class _FakeCursor: class _FakeConnection: - def __init__(self, rows): - self._cursor = _FakeCursor(rows) + def __init__(self, rows, lib_rows=None): + self._cursor = _FakeCursor(rows, lib_rows) def cursor(self): return self._cursor @@ -107,12 +112,13 @@ def test_scan_handles_mixed_track_id_types(monkeypatch): # and finds the primary artist at 100%, suppressing the false flag. -def _make_finding_capturing_context(track_row, captured): +def _make_finding_capturing_context(track_row, captured, lib_rows=None): """Context that captures any create_finding calls into the `captured` list. Tests assert against this list to verify whether the scanner created a finding (false positive) or correctly - skipped (multi-value match resolved).""" - conn = _FakeConnection([track_row]) + skipped (multi-value match resolved). ``lib_rows`` seeds the + library_history match SELECT (#934).""" + conn = _FakeConnection([track_row], lib_rows) config_manager = SimpleNamespace( get=lambda key, default=None: default, set=lambda *args, **kwargs: None, @@ -887,9 +893,10 @@ def test_human_verified_files_are_never_scanned(monkeypatch): # --------------------------------------------------------------------------- -def _run_persistence_scan(monkeypatch, *, file_status, aid_artist, expected_artist): +def _run_persistence_scan(monkeypatch, *, file_status, aid_artist, expected_artist, lib_rows=None): """Drive one _scan_file call and return (status_updates, tag_writes) where - status_updates is the list of (query, params) UPDATEs the scanner ran.""" + status_updates is the list of (query, params) UPDATEs the scanner ran. + ``lib_rows`` seeds the library_history match SELECT (#934).""" import core.repair_jobs.acoustid_scanner as scanner_mod monkeypatch.setattr(scanner_mod, "_resolve_expected_artist_aliases", lambda name: [], raising=False) @@ -905,7 +912,7 @@ def _run_persistence_scan(monkeypatch, *, file_status, aid_artist, expected_arti context = _make_finding_capturing_context( track_row=("9", "Call Your Name", expected_artist, "/music/cyn.flac", 1, "Album", None, None), - captured=captured) + captured=captured, lib_rows=lib_rows) fake = SimpleNamespace(fingerprint_and_lookup=lambda f: { 'best_score': 0.97, 'recordings': [{'title': 'Call Your Name', 'artist': aid_artist}]}) @@ -920,29 +927,32 @@ def _run_persistence_scan(monkeypatch, *, file_status, aid_artist, expected_arti def test_scan_pass_backfills_verified_status(monkeypatch): - # Untagged file + clean fingerprint PASS → the scan backfills 'verified' - # into the tag, the tracks row AND library_history (review-queue feed). + # Clean fingerprint PASS → the scan backfills 'verified' into the tag, the tracks + # row AND the file's library_history row. The history row's path drifted since + # download (file moved), so the scan heals it by id (#934) rather than missing it. updates, tag_writes, captured = _run_persistence_scan( monkeypatch, file_status=None, - aid_artist='Sawano Hiroyuki', expected_artist='Sawano Hiroyuki') + aid_artist='Sawano Hiroyuki', expected_artist='Sawano Hiroyuki', + lib_rows=[(1, '/downloads/old/cyn.flac', 'Call Your Name', 'soulseek')]) assert captured == [] assert tag_writes == [('/music/cyn.flac', 'verified')] assert any('tracks' in q and p == ('verified', '9') for q, p in updates) - assert any('library_history' in q and p == ('verified', '/music/cyn.flac') + # healed by id, status set + path refreshed to the file's current location. + assert any('library_history' in q and p == ('verified', '/music/cyn.flac', 1) for q, p in updates) def test_scan_skip_marks_untagged_file_unverified(monkeypatch): # Title matches but the artist is ambiguous (cover/collab band?) → SKIP. - # An untagged file gets 'unverified' so it surfaces in the Downloads-page - # review queue instead of silently passing or being deleted. + # An untagged file SoulSync never downloaded (no history row) gets a fresh + # 'unverified' row INSERTed so it surfaces in the Downloads-page review queue. updates, tag_writes, captured = _run_persistence_scan( monkeypatch, file_status=None, - aid_artist='Mantilla', expected_artist='Metallica') + aid_artist='Mantilla', expected_artist='Metallica') # no lib_rows → no existing row assert captured == [] assert tag_writes == [('/music/cyn.flac', 'unverified')] assert any('tracks' in q and p == ('unverified', '9') for q, p in updates) - assert any('library_history' in q and p == ('unverified', '/music/cyn.flac') + assert any('INSERT INTO library_history' in q and p[-1] == 'unverified' for q, p in updates) diff --git a/tests/test_history_match.py b/tests/test_history_match.py new file mode 100644 index 00000000..bfb05e8d --- /dev/null +++ b/tests/test_history_match.py @@ -0,0 +1,63 @@ +"""Pure matcher that re-links a moved file to its download-history row (#934).""" + +from core.downloads.history_match import pick_history_row, like_filename_filter + + +# (id, file_path, title, download_source) +def _row(i, path, title='', source='soulseek'): + return (i, path, title, source) + + +def test_exact_current_path_wins(): + cands = [_row(1, '/old/song.flac', 'Song'), _row(2, '/lib/song.flac', 'Song')] + assert pick_history_row(cands, current_paths=('/lib/song.flac', None), + basename='song.flac', title='Song') == 2 + + +def test_falls_back_to_filename_when_path_drifted(): + # history has the OLD import path; scanner only knows the NEW library path. + cands = [_row(7, '/downloads/transfer/Artist/01 - Song.flac', 'Song')] + assert pick_history_row(cands, current_paths=('/music/Artist/Album/01 - Song.flac', None), + basename='01 - Song.flac', title='Song') == 7 + + +def test_title_guard_blocks_shared_filename_collision(): + # two different songs both named "01 - Intro.flac" — must NOT heal the wrong one. + cands = [_row(1, '/a/01 - Intro.flac', 'Album A Intro'), + _row(2, '/b/01 - Intro.flac', 'Album B Intro')] + got = pick_history_row(cands, current_paths=('/c/01 - Intro.flac', None), + basename='01 - Intro.flac', title='Album B Intro') + assert got == 2 + + +def test_title_drift_still_matches_after_normalization(): + cands = [_row(5, '/old/track.flac', 'Song (Remastered)')] + assert pick_history_row(cands, current_paths=('/new/track.flac', None), + basename='track.flac', title='song remastered') == 5 + + +def test_prefers_real_download_row_over_synthetic_scan_row(): + # the #934 collapse: a real download row (drifted path) + a synthetic scan dup at the + # exact current path. The REAL row must win, so the synthetic one can be deleted. + cands = [_row(10, '/old/song.flac', 'Song', source='soulseek'), + _row(11, '/lib/song.flac', 'Song', source='acoustid_scan')] + assert pick_history_row(cands, current_paths=('/lib/song.flac', None), + basename='song.flac', title='Song') == 10 + + +def test_no_basename_match_returns_none(): + cands = [_row(1, '/x/other.flac', 'Other')] + assert pick_history_row(cands, current_paths=('/x/wanted.flac', None), + basename='wanted.flac', title='Wanted') is None + + +def test_filename_only_substring_is_not_a_match(): + # '/x/mysong.flac' must NOT satisfy basename 'song.flac' + cands = [_row(1, '/x/mysong.flac', 'My Song')] + assert pick_history_row(cands, current_paths=('/x/song.flac', None), + basename='song.flac', title='Song') is None + + +def test_like_filter_escapes_metacharacters(): + # underscores/percents in filenames must not become LIKE wildcards + assert like_filename_filter('a_b%c.flac') == r'%a\_b\%c.flac' From a42cf3b8657988b1865dce38142f40f6681415ac Mon Sep 17 00:00:00 2001 From: dev Date: Sat, 27 Jun 2026 19:22:36 +0200 Subject: [PATCH 11/39] downloads(#934): instant startup reconcile of stuck-'unverified' history from tracks truth MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Complements the AcoustID-scan-time heal: re-links library_history rows still showing 'unverified' to the verified/human_verified status their file already carries in the tracks table — matching exact path AND basename, so a file that moved (media-server import / reorganize) heals even though the stored history path is frozen. Upgrade-only and non-destructive (no deletes, no bulk migration). Why this is needed on top of the scan-time fix: - It clears the EXISTING backlog (e.g. 5551 rows) on the next restart with NO re-fingerprinting and no AcoustID API calls — the file's status is already in tracks from the prior scan; this just propagates it to the frozen history row. - It covers human_verified files, which the AcoustID scan skips entirely (file_verif_status == 'human_verified' returns early), so their stale history rows would otherwise never heal. Runs once on DB init (cheap, idempotent). 5 real-sqlite tests. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01LWJk7EuM7YktQeNyqQwTZY --- database/music_database.py | 57 +++++++++ tests/test_unverified_history_reconcile.py | 140 +++++++++++++++++++++ 2 files changed, 197 insertions(+) create mode 100644 tests/test_unverified_history_reconcile.py diff --git a/database/music_database.py b/database/music_database.py index dd37aee3..07a85687 100644 --- a/database/music_database.py +++ b/database/music_database.py @@ -1000,6 +1000,10 @@ class MusicDatabase: self._init_manual_library_match_table() self._backfill_mirrored_track_source_ids() + # Self-heal the Unverified review queue: lift history rows stuck at + # 'unverified' whose file has since been verified (issue #934). Cheap, + # idempotent (only touches rows that need it), so it's safe every boot. + self.reconcile_unverified_history_from_tracks() def _backfill_mirrored_track_source_ids(self) -> int: """One-time, idempotent: assign a stable source_track_id to mirrored tracks @@ -13857,6 +13861,59 @@ class MusicDatabase: logger.error("Error querying unverified library history: %s", e) return [] + def reconcile_unverified_history_from_tracks(self) -> int: + """Heal library_history rows stuck at 'unverified' whose underlying file + has since been confirmed in the tracks table (AcoustID scan PASS or a + human decision). Matches by exact path AND basename — the same physical + file keeps its filename across path-form differences (relative vs + absolute, library moved/reorganized, different mount), which is why an + exact-path-only heal left thousands of already-verified files showing as + Unverified (issue #934). + + Upgrade-only and non-destructive: it only lifts 'unverified' rows to the + confirmed status, never downgrades and never deletes. Returns the number + of rows healed. Genuinely-unverified rows and orphans (no matching + track) are left untouched. + """ + healed = 0 + try: + conn = self._get_connection() + cursor = conn.cursor() + rank = {'verified': 1, 'human_verified': 2} + by_path = {} + by_base = {} + cursor.execute( + "SELECT file_path, verification_status FROM tracks " + "WHERE verification_status IN ('verified', 'human_verified') " + "AND file_path IS NOT NULL AND file_path != ''") + for fp, st in cursor.fetchall(): + if not fp: + continue + by_path[fp] = st + base = os.path.basename(fp) + if base and rank.get(st, 0) >= rank.get(by_base.get(base), 0): + by_base[base] = st + updates = [] + cursor.execute( + "SELECT id, file_path FROM library_history " + "WHERE verification_status = 'unverified' " + "AND file_path IS NOT NULL AND file_path != ''") + for rid, fp in cursor.fetchall(): + target = by_path.get(fp) or by_base.get(os.path.basename(fp or '')) + if target: + updates.append((target, rid)) + for status, rid in updates: + cursor.execute( + "UPDATE library_history SET verification_status = ? WHERE id = ?", + (status, rid)) + healed += 1 + if healed: + conn.commit() + logger.info("Reconciled %d unverified history rows from tracks truth", healed) + except Exception as e: + logger.error("Error reconciling unverified history: %s", e) + return healed + def get_library_history_stats(self): """Return counts per event_type and per download_source.""" try: diff --git a/tests/test_unverified_history_reconcile.py b/tests/test_unverified_history_reconcile.py new file mode 100644 index 00000000..e614b4d3 --- /dev/null +++ b/tests/test_unverified_history_reconcile.py @@ -0,0 +1,140 @@ +"""Issue #934 — one-time reconcile that clears the existing backlog of +``library_history`` rows stuck at 'unverified' even though the file has since +been verified (by an AcoustID scan, or human-confirmed). Heals from the +``tracks`` truth, matching exact path AND basename (so a reorganized/moved file +heals too), upgrade-only. Never deletes anything.""" + +import sqlite3 +import sys +import types + +if "spotipy" not in sys.modules: # match the suite's lightweight stubs + spotipy = types.ModuleType("spotipy") + spotipy.Spotify = object + oauth2 = types.ModuleType("spotipy.oauth2") + oauth2.SpotifyOAuth = object + oauth2.SpotifyClientCredentials = object + spotipy.oauth2 = oauth2 + sys.modules["spotipy"] = spotipy + sys.modules["spotipy.oauth2"] = oauth2 + +if "config.settings" not in sys.modules: + config_pkg = types.ModuleType("config") + settings_mod = types.ModuleType("config.settings") + + class _DummyConfigManager: + def get(self, key, default=None): + return default + + def get_active_media_server(self): + return "primary" + + settings_mod.config_manager = _DummyConfigManager() + config_pkg.settings = settings_mod + sys.modules["config"] = config_pkg + sys.modules["config.settings"] = settings_mod + +from database.music_database import MusicDatabase # noqa: E402 + + +class _NonClosingConn: + def __init__(self, real): + self._real = real + + def cursor(self): + return self._real.cursor() + + def commit(self): + return self._real.commit() + + def close(self): + pass + + def __enter__(self): + return self + + def __exit__(self, *args): + pass + + +class _InMemoryDB(MusicDatabase): + def __init__(self): + self._conn = sqlite3.connect(":memory:") + self._conn.row_factory = sqlite3.Row + self._conn.execute( + "CREATE TABLE tracks (id INTEGER PRIMARY KEY, file_path TEXT, " + "verification_status TEXT)" + ) + self._conn.execute( + "CREATE TABLE library_history (" + "id INTEGER PRIMARY KEY AUTOINCREMENT, event_type TEXT, title TEXT, " + "artist_name TEXT, album_name TEXT, file_path TEXT, " + "download_source TEXT, verification_status TEXT, " + "created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP)" + ) + + def _get_connection(self): + return _NonClosingConn(self._conn) + + def _add_track(self, tid, path, status): + self._conn.execute( + "INSERT INTO tracks (id, file_path, verification_status) VALUES (?,?,?)", + (tid, path, status)) + self._conn.commit() + + def _add_history(self, path, status, title="Song"): + self._conn.execute( + "INSERT INTO library_history (event_type, title, file_path, " + "verification_status) VALUES ('download', ?, ?, ?)", + (title, path, status)) + self._conn.commit() + + def _status_of(self, hid): + return self._conn.execute( + "SELECT verification_status FROM library_history WHERE id = ?", (hid,) + ).fetchone()[0] + + +def test_reconcile_heals_exact_path_match(): + db = _InMemoryDB() + db._add_track(1, "/lib/A/01 - Song.flac", "verified") + db._add_history("/lib/A/01 - Song.flac", "unverified") + healed = db.reconcile_unverified_history_from_tracks() + assert healed == 1 + assert db._status_of(1) == "verified" + + +def test_reconcile_heals_by_basename_when_path_form_differs(): + db = _InMemoryDB() + db._add_track(1, "/library/Artist/Album/01 - Song.flac", "verified") + # History stored the transfer-folder path; basename still matches. + db._add_history("/transfer/Artist - Album/01 - Song.flac", "unverified") + healed = db.reconcile_unverified_history_from_tracks() + assert healed == 1 + assert db._status_of(1) == "verified" + + +def test_reconcile_propagates_human_verified(): + db = _InMemoryDB() + db._add_track(1, "/lib/01 - Song.flac", "human_verified") + db._add_history("/lib/01 - Song.flac", "unverified") + db.reconcile_unverified_history_from_tracks() + assert db._status_of(1) == "human_verified" + + +def test_reconcile_leaves_genuinely_unverified_rows(): + db = _InMemoryDB() + db._add_track(1, "/lib/01 - Song.flac", "unverified") # track itself unconfirmed + db._add_history("/lib/01 - Song.flac", "unverified") + healed = db.reconcile_unverified_history_from_tracks() + assert healed == 0 + assert db._status_of(1) == "unverified" + + +def test_reconcile_leaves_orphans_untouched(): + db = _InMemoryDB() + # No track references this file at all (deleted / re-downloaded elsewhere). + db._add_history("/lib/gone.flac", "unverified") + healed = db.reconcile_unverified_history_from_tracks() + assert healed == 0 + assert db._status_of(1) == "unverified" From 027cf74d4758c6b196457696bb9eda9bb52f6e64 Mon Sep 17 00:00:00 2001 From: dev Date: Sat, 27 Jun 2026 19:27:28 +0200 Subject: [PATCH 12/39] ui(downloads): Unverified review rows get the Quarantine-style card design MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Quarantine sub-view was reworked into rich cards (artwork, source line, row-click to expand an inline detail panel, consistent action cluster) but the Unverified sub-view was left on the generic download-row layout that opened a modal on click. Bring it to parity: - dedicated _verifUnverifiedRowHtml / _verifUnverifiedRows renderer, used via a sibling branch in _adlRender (mirrors the quarantine sub-view branch). - row click toggles an inline details panel (why flagged, download source, quality, file, downloaded-at), open-state keyed by stable id so it survives the 2 s poll re-render — same pattern as verifQuarInspect. - reuses the existing verif-quar-* / verif-actions / adl-row CSS (no new styles) and the existing play/compare/audit/approve/delete handlers. - NO grouping: each unverified import is its own track (grouping only makes sense for the quarantine alternates), per design intent. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01LWJk7EuM7YktQeNyqQwTZY --- webui/static/pages-extra.js | 90 +++++++++++++++++++++++++++++++++++++ 1 file changed, 90 insertions(+) diff --git a/webui/static/pages-extra.js b/webui/static/pages-extra.js index 560ac261..06392b36 100644 --- a/webui/static/pages-extra.js +++ b/webui/static/pages-extra.js @@ -2760,6 +2760,84 @@ async function verifDelete(hid, btn) { } catch (e) { showToast && showToast('Delete failed', 'error'); } if (btn) btn.disabled = false; } + +// ---- Unverified review rows (Quarantine-style cards) ---- +// The Unverified sub-view used to piggyback on the generic download row and +// open a modal on click. These give it the same card design the Quarantine +// sub-view got — row click expands an inline detail panel in place — minus the +// grouping (each unverified import is its own track, nothing to group). +let _verifUnvData = []; +const _verifUnvOpenDetails = new Set(); + +function _verifUnvKey(dl) { + return verifHistoryId(dl) || dl.task_id || ''; +} + +function _verifUnvDetailRows(dl) { + const reason = dl.verification_status === 'force_imported' + ? 'Accepted as the best available candidate after the retry budget was exhausted (version-mismatch fallback). A library AcoustID scan reports these as informational.' + : 'AcoustID could not hard-confirm this file (ambiguous / cross-script metadata / no fingerprint match). Imported, but not verified.'; + const source = dl.batch_source || dl.batch_name || ''; + return [ + `
Why flagged: ${_adlEsc(reason)}
`, + source ? `
Download source: ${_adlEsc(source)}
` : '', + dl.quality ? `
Quality: ${_adlEsc(dl.quality)}
` : '', + dl.file_path ? `
File: ${_adlEsc(dl.file_path)}
` : '', + dl.created_at ? `
Downloaded: ${_adlEsc(dl.created_at)}
` : '', + ].filter(Boolean).join(''); +} + +function _verifUnverifiedRowHtml(dl, idx) { + const hid = verifHistoryId(dl); + const title = _adlEsc(dl.title || 'Unknown Track'); + const meta = [_adlEsc(dl.artist || ''), _adlEsc(dl.album || '')].filter(Boolean).join(' · '); + const source = _adlEsc(dl.batch_source || dl.batch_name || ''); + const timeAgo = _verifTimeAgo(dl.created_at); + const artHtml = dl.artwork + ? `` + : '
'; + const detailsOpen = _verifUnvOpenDetails.has(_verifUnvKey(dl)); + const actions = hid ? ` + + + + + ` : ''; + return `
+ ${artHtml} +
+
${title}
+ ${meta ? `
${meta}
` : ''} + ${source ? `
${source}
` : ''} +
${_verifUnvDetailRows(dl)}
+
+
+ ${_verifReasonBadge(dl)} + ${dl.quality ? `${_adlEsc(dl.quality)}` : ''} + ${timeAgo ? `${timeAgo}` : ''} + ${actions} +
+
`; +} + +function _verifUnverifiedRows(items) { + _verifUnvData = items; + if (!items.length) return ''; + return items.map((dl, idx) => _verifUnverifiedRowHtml(dl, idx)).join(''); +} + +function verifUnvInspect(idx) { + // Open-state lives in a Set keyed by the row's stable id (not the DOM) so it + // survives the 2 s polling re-render — same pattern as verifQuarInspect. + const dl = _verifUnvData[idx]; + if (!dl) return; + const key = _verifUnvKey(dl); + if (_verifUnvOpenDetails.has(key)) _verifUnvOpenDetails.delete(key); + else _verifUnvOpenDetails.add(key); + const el = document.getElementById(`verif-unv-details-${idx}`); + if (el) el.style.display = _verifUnvOpenDetails.has(key) ? '' : 'none'; +} + // ---- Quarantine sub-view inside the ⚠ filter ---- // The review queue covers BOTH kinds of suspect files: imported-but- // unconfirmed (unverified/force_imported) and not-imported-at-all @@ -3293,6 +3371,18 @@ function _adlRender() { return; } + // Unverified review sub-view: render the Quarantine-style cards (inline + // expandable details), not the generic download rows. + if (_adlFilter === 'unverified' && _verifSubView === 'unverified') { + const uhtml = _verifUnverifiedRows(filtered); + const uEmptyEl = document.getElementById('adl-empty'); + const uEmptyHtml = uEmptyEl ? uEmptyEl.outerHTML : ''; + list.innerHTML = uEmptyHtml + uhtml; + const uNewEmpty = document.getElementById('adl-empty'); + if (uNewEmpty) uNewEmpty.style.display = uhtml ? 'none' : ''; + return; + } + if (filtered.length === 0) { if (empty) empty.style.display = ''; // Clear any existing rows but keep the empty message From 28a539a8400744c700cfbd01799fdc1a7fee3ba6 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Sat, 27 Jun 2026 10:48:07 -0700 Subject: [PATCH 13/39] ui: default Background Particles OFF (#935) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit the full-page particle canvas runs a continuous requestAnimationFrame loop behind every page — real GPU cost, and multiple users hit GPU strain until they found the toggle. flip the default to off; the eye candy is opt-in now. - init.js: runtime flag defaults false unless localStorage is explicitly 'true' - settings.js: config read is now '=== true' (default off) instead of '!== false' - index.html: checkbox no longer 'checked' by default; hint reworded existing users who explicitly enabled it (localStorage/config 'true') keep it on; the existing '!== false' runtime guards still work since the flag is now always set explicitly. --- webui/index.html | 4 ++-- webui/static/init.js | 7 ++++--- webui/static/settings.js | 2 +- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/webui/index.html b/webui/index.html index 30925a59..cfc502c5 100644 --- a/webui/index.html +++ b/webui/index.html @@ -6263,10 +6263,10 @@
- Animated particle effects behind each page. Disable to reduce GPU usage. + Animated particle effects behind each page. Off by default — enable for the eye candy (uses GPU).