From cf5461f2f1fcb0f6b5c9ed5a453ba52ca15f6be2 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Mon, 4 May 2026 08:55:13 -0700 Subject: [PATCH] Fix: maintenance findings badge inflated when scan dedup-skipped `_create_finding` silently dedup-skipped re-discovered issues but the caller incremented `findings_created` regardless. So a re-scan that found the same issues as a prior scan reported 364 findings in the badge while 0 NEW pending rows hit the db, leaving the findings tab empty. `_create_finding` now returns bool (True on insert, False on dedup-skip / db error). All 16 repair jobs updated to only increment `findings_created` on True. Added `findings_skipped_dedup` counter surfaced in scan log: "Done: X scanned, 0 fixed, 0 findings (363 already existed), 0 errors". Also fixed a missing `job_id` kwarg in album_tag_consistency that was silently breaking finding creation for that scan. --- core/repair_jobs/acoustid_scanner.py | 7 +- core/repair_jobs/album_completeness.py | 7 +- core/repair_jobs/album_tag_consistency.py | 8 +- core/repair_jobs/base.py | 1 + core/repair_jobs/dead_file_cleaner.py | 7 +- core/repair_jobs/discography_backfill.py | 15 +- core/repair_jobs/duplicate_detector.py | 7 +- core/repair_jobs/fake_lossless_detector.py | 7 +- core/repair_jobs/library_reorganize.py | 7 +- core/repair_jobs/live_commentary_cleaner.py | 7 +- core/repair_jobs/lossy_converter.py | 7 +- core/repair_jobs/mbid_mismatch_detector.py | 14 +- core/repair_jobs/metadata_gap_filler.py | 7 +- core/repair_jobs/missing_cover_art.py | 7 +- core/repair_jobs/orphan_file_detector.py | 7 +- core/repair_jobs/single_album_dedup.py | 7 +- core/repair_jobs/track_number_repair.py | 7 +- core/repair_jobs/unknown_artist_fixer.py | 7 +- core/repair_worker.py | 18 +- tests/metadata/test_metadata_gap_filler.py | 2 +- tests/test_album_completeness_job.py | 6 +- tests/test_create_finding_dedup_counter.py | 208 +++++++++++++++++++ tests/test_duplicate_detector_slskd_dedup.py | 2 + tests/test_missing_cover_art.py | 3 +- web_server.py | 4 +- webui/static/helper.js | 1 + 26 files changed, 332 insertions(+), 48 deletions(-) create mode 100644 tests/test_create_finding_dedup_counter.py diff --git a/core/repair_jobs/acoustid_scanner.py b/core/repair_jobs/acoustid_scanner.py index 60482a1d..ba3c1564 100644 --- a/core/repair_jobs/acoustid_scanner.py +++ b/core/repair_jobs/acoustid_scanner.py @@ -212,7 +212,7 @@ class AcoustIDScannerJob(RepairJob): ) if context.create_finding: severity = 'warning' if best_score >= 0.90 else 'info' - context.create_finding( + inserted = context.create_finding( job_id=self.job_id, finding_type='acoustid_mismatch', severity=severity, @@ -240,7 +240,10 @@ class AcoustIDScannerJob(RepairJob): 'track_number': expected.get('track_number'), } ) - result.findings_created += 1 + if inserted: + result.findings_created += 1 + else: + result.findings_skipped_dedup += 1 def _load_db_tracks(self, context: JobContext) -> dict: """Load all tracks from DB keyed by track ID.""" diff --git a/core/repair_jobs/album_completeness.py b/core/repair_jobs/album_completeness.py index c92363f3..24b1260f 100644 --- a/core/repair_jobs/album_completeness.py +++ b/core/repair_jobs/album_completeness.py @@ -234,7 +234,7 @@ class AlbumCompletenessJob(RepairJob): ) if context.create_finding: try: - context.create_finding( + inserted = context.create_finding( job_id=self.job_id, finding_type='incomplete_album', severity='info', @@ -264,7 +264,10 @@ class AlbumCompletenessJob(RepairJob): 'artist_thumb_url': artist_thumb or None, } ) - result.findings_created += 1 + 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) result.errors += 1 diff --git a/core/repair_jobs/album_tag_consistency.py b/core/repair_jobs/album_tag_consistency.py index c6705795..3a8abd14 100644 --- a/core/repair_jobs/album_tag_consistency.py +++ b/core/repair_jobs/album_tag_consistency.py @@ -288,7 +288,8 @@ class AlbumTagConsistencyJob(RepairJob): variants_str = ' vs '.join(f'"{v}"' for v in inc['variants'][:3]) desc_parts.append(f"{inc['field']}: {variants_str}") - context.create_finding( + inserted = context.create_finding( + job_id=self.job_id, finding_type='album_tag_inconsistency', severity='warning', entity_type='album', @@ -306,7 +307,10 @@ class AlbumTagConsistencyJob(RepairJob): 'file_path': t['file_path']} for t in tag_data], } ) - result.findings_created += 1 + if inserted: + result.findings_created += 1 + else: + result.findings_skipped_dedup += 1 if context.report_progress: context.report_progress( diff --git a/core/repair_jobs/base.py b/core/repair_jobs/base.py index f07b6019..4ac7f52b 100644 --- a/core/repair_jobs/base.py +++ b/core/repair_jobs/base.py @@ -11,6 +11,7 @@ class JobResult: """Result of a single job scan run.""" scanned: int = 0 findings_created: int = 0 + findings_skipped_dedup: int = 0 # Findings the worker already had a row for auto_fixed: int = 0 errors: int = 0 skipped: int = 0 diff --git a/core/repair_jobs/dead_file_cleaner.py b/core/repair_jobs/dead_file_cleaner.py index 39c60742..f0b3fb9d 100644 --- a/core/repair_jobs/dead_file_cleaner.py +++ b/core/repair_jobs/dead_file_cleaner.py @@ -120,7 +120,7 @@ class DeadFileCleanerJob(RepairJob): ) if context.create_finding: try: - context.create_finding( + inserted = context.create_finding( job_id=self.job_id, finding_type='dead_file', severity='warning', @@ -139,7 +139,10 @@ class DeadFileCleanerJob(RepairJob): 'artist_thumb_url': artist_thumb or None, } ) - result.findings_created += 1 + if inserted: + result.findings_created += 1 + else: + result.findings_skipped_dedup += 1 except Exception as e: logger.debug("Error creating dead file finding for track %s: %s", track_id, e) result.errors += 1 diff --git a/core/repair_jobs/discography_backfill.py b/core/repair_jobs/discography_backfill.py index 3170624a..2887a711 100644 --- a/core/repair_jobs/discography_backfill.py +++ b/core/repair_jobs/discography_backfill.py @@ -320,7 +320,7 @@ class DiscographyBackfillJob(RepairJob): # Create finding if context.create_finding: try: - context.create_finding( + inserted = context.create_finding( job_id=self.job_id, finding_type='missing_discography_track', severity='info', @@ -340,13 +340,18 @@ class DiscographyBackfillJob(RepairJob): 'source': source, }, ) - result.findings_created += 1 - missing_count += 1 + if inserted: + result.findings_created += 1 + missing_count += 1 + else: + result.findings_skipped_dedup += 1 # Auto-wishlist mode: also push to wishlist now. The # finding still gets created so the user has a log of - # what the backfill picked up. - if auto_add: + # what the backfill picked up. Only fire on a NEW + # finding — skip if dedup-suppressed (already on the + # wishlist or already auto-added in a prior scan). + if auto_add and inserted: try: context.db.add_to_wishlist( spotify_track_data=track_data, diff --git a/core/repair_jobs/duplicate_detector.py b/core/repair_jobs/duplicate_detector.py index b475e363..3b40f396 100644 --- a/core/repair_jobs/duplicate_detector.py +++ b/core/repair_jobs/duplicate_detector.py @@ -271,7 +271,7 @@ class DuplicateDetectorJob(RepairJob): if context.create_finding: try: group.sort(key=lambda t: (t['bitrate'] or 0), reverse=True) - context.create_finding( + inserted = context.create_finding( job_id=self.job_id, finding_type='duplicate_tracks', severity='info', @@ -295,7 +295,10 @@ class DuplicateDetectorJob(RepairJob): 'artist_thumb_url': group[0].get('artist_thumb_url'), } ) - result.findings_created += 1 + if inserted: + result.findings_created += 1 + else: + result.findings_skipped_dedup += 1 except Exception as e: logger.debug("Error creating duplicate finding: %s", e) result.errors += 1 diff --git a/core/repair_jobs/fake_lossless_detector.py b/core/repair_jobs/fake_lossless_detector.py index cc677ed3..dd8ffa5f 100644 --- a/core/repair_jobs/fake_lossless_detector.py +++ b/core/repair_jobs/fake_lossless_detector.py @@ -110,7 +110,7 @@ class FakeLosslessDetectorJob(RepairJob): log_type='error' ) if context.create_finding: - context.create_finding( + inserted = context.create_finding( job_id=self.job_id, finding_type='fake_lossless', severity='warning', @@ -134,7 +134,10 @@ class FakeLosslessDetectorJob(RepairJob): 'file_size': os.path.getsize(fpath), } ) - result.findings_created += 1 + if inserted: + result.findings_created += 1 + else: + result.findings_skipped_dedup += 1 except Exception as e: logger.debug("Error analyzing %s: %s", os.path.basename(fpath), e) diff --git a/core/repair_jobs/library_reorganize.py b/core/repair_jobs/library_reorganize.py index 3dc11245..7ab4aa9b 100644 --- a/core/repair_jobs/library_reorganize.py +++ b/core/repair_jobs/library_reorganize.py @@ -495,7 +495,7 @@ class LibraryReorganizeJob(RepairJob): rel_actual = os.path.relpath(actual_norm, transfer) rel_expected = os.path.relpath(expected_norm, transfer) if context.create_finding: - context.create_finding( + inserted = context.create_finding( job_id=self.job_id, finding_type='path_mismatch', severity='info', @@ -506,7 +506,10 @@ class LibraryReorganizeJob(RepairJob): description=f'From: {rel_actual}\nTo: {rel_expected}', details={'from': rel_actual, 'to': rel_expected} ) - result.findings_created += 1 + if inserted: + result.findings_created += 1 + else: + result.findings_skipped_dedup += 1 if context.report_progress: context.report_progress( scanned=i + 1, total=total, diff --git a/core/repair_jobs/live_commentary_cleaner.py b/core/repair_jobs/live_commentary_cleaner.py index b3532567..60de59b4 100644 --- a/core/repair_jobs/live_commentary_cleaner.py +++ b/core/repair_jobs/live_commentary_cleaner.py @@ -207,7 +207,7 @@ class LiveCommentaryCleanerJob(RepairJob): if context.create_finding: try: - context.create_finding( + inserted = context.create_finding( job_id=self.job_id, finding_type='unwanted_content', severity='info', @@ -239,7 +239,10 @@ class LiveCommentaryCleanerJob(RepairJob): 'artist_thumb_url': artist_thumb or None, } ) - result.findings_created += 1 + if inserted: + result.findings_created += 1 + else: + result.findings_skipped_dedup += 1 except Exception as e: logger.debug("Error creating finding: %s", e) result.errors += 1 diff --git a/core/repair_jobs/lossy_converter.py b/core/repair_jobs/lossy_converter.py index 1c577f98..da1f1646 100644 --- a/core/repair_jobs/lossy_converter.py +++ b/core/repair_jobs/lossy_converter.py @@ -150,7 +150,7 @@ class LossyConverterJob(RepairJob): if context.create_finding: try: file_size = os.path.getsize(resolved) - context.create_finding( + inserted = context.create_finding( job_id=self.job_id, finding_type='missing_lossy_copy', severity='info', @@ -177,7 +177,10 @@ class LossyConverterJob(RepairJob): 'artist_thumb_url': artist_thumb or None, } ) - result.findings_created += 1 + if inserted: + result.findings_created += 1 + else: + result.findings_skipped_dedup += 1 except Exception as e: logger.debug("Error creating finding for track %s: %s", track_id, e) result.errors += 1 diff --git a/core/repair_jobs/mbid_mismatch_detector.py b/core/repair_jobs/mbid_mismatch_detector.py index 4edf5c5f..ef6ad77c 100644 --- a/core/repair_jobs/mbid_mismatch_detector.py +++ b/core/repair_jobs/mbid_mismatch_detector.py @@ -616,7 +616,7 @@ class MbidMismatchDetectorJob(RepairJob): ) if context.create_finding: try: - context.create_finding( + inserted = context.create_finding( job_id=self.job_id, finding_type='album_mbid_mismatch', severity='warning', @@ -648,7 +648,10 @@ class MbidMismatchDetectorJob(RepairJob): 'artist_thumb_url': track['artist_thumb'] or None, } ) - result.findings_created += 1 + if inserted: + result.findings_created += 1 + else: + result.findings_skipped_dedup += 1 except Exception as e: logger.debug("Error creating album MBID mismatch finding for track %s: %s", track['track_id'], e) @@ -665,7 +668,7 @@ class MbidMismatchDetectorJob(RepairJob): ) if context.create_finding: try: - context.create_finding( + inserted = context.create_finding( job_id=self.job_id, finding_type='mbid_mismatch', severity='warning', @@ -692,7 +695,10 @@ class MbidMismatchDetectorJob(RepairJob): 'artist_thumb_url': artist_thumb or None, } ) - result.findings_created += 1 + if inserted: + result.findings_created += 1 + else: + result.findings_skipped_dedup += 1 except Exception as e: logger.debug("Error creating MBID mismatch finding for track %s: %s", track_id, e) result.errors += 1 diff --git a/core/repair_jobs/metadata_gap_filler.py b/core/repair_jobs/metadata_gap_filler.py index ead800b1..51389336 100644 --- a/core/repair_jobs/metadata_gap_filler.py +++ b/core/repair_jobs/metadata_gap_filler.py @@ -177,7 +177,7 @@ class MetadataGapFillerJob(RepairJob): if context.create_finding: try: field_names = ', '.join(found_fields.keys()) - context.create_finding( + inserted = context.create_finding( job_id=self.job_id, finding_type='metadata_gap', severity='info', @@ -202,7 +202,10 @@ class MetadataGapFillerJob(RepairJob): 'artist_thumb_url': artist_thumb or None, } ) - result.findings_created += 1 + if inserted: + result.findings_created += 1 + else: + result.findings_skipped_dedup += 1 except Exception as e: logger.debug("Error creating metadata gap finding for track %s: %s", track_id, e) result.errors += 1 diff --git a/core/repair_jobs/missing_cover_art.py b/core/repair_jobs/missing_cover_art.py index 1e406ee3..4c292873 100644 --- a/core/repair_jobs/missing_cover_art.py +++ b/core/repair_jobs/missing_cover_art.py @@ -138,7 +138,7 @@ class MissingCoverArtJob(RepairJob): # Create finding for user to approve if context.create_finding: try: - context.create_finding( + inserted = context.create_finding( job_id=self.job_id, finding_type='missing_cover_art', severity='info', @@ -156,7 +156,10 @@ class MissingCoverArtJob(RepairJob): 'artist_thumb_url': artist_thumb or None, } ) - result.findings_created += 1 + if inserted: + result.findings_created += 1 + else: + result.findings_skipped_dedup += 1 except Exception as e: logger.debug("Error creating cover art finding for album %s: %s", album_id, e) result.errors += 1 diff --git a/core/repair_jobs/orphan_file_detector.py b/core/repair_jobs/orphan_file_detector.py index b9f5371b..48717f91 100644 --- a/core/repair_jobs/orphan_file_detector.py +++ b/core/repair_jobs/orphan_file_detector.py @@ -217,7 +217,7 @@ class OrphanFileDetectorJob(RepairJob): stat = os.stat(fpath) ext = os.path.splitext(fpath)[1].lower().lstrip('.') if context.create_finding: - context.create_finding( + inserted = context.create_finding( job_id=self.job_id, finding_type='orphan_file', severity='warning' if mass_orphan else 'info', @@ -241,7 +241,10 @@ class OrphanFileDetectorJob(RepairJob): 'mass_orphan': mass_orphan, } ) - result.findings_created += 1 + if inserted: + result.findings_created += 1 + else: + result.findings_skipped_dedup += 1 except Exception as e: logger.debug("Error creating orphan finding for %s: %s", fpath, e) result.errors += 1 diff --git a/core/repair_jobs/single_album_dedup.py b/core/repair_jobs/single_album_dedup.py index cbcad948..a4ccdd5b 100644 --- a/core/repair_jobs/single_album_dedup.py +++ b/core/repair_jobs/single_album_dedup.py @@ -196,7 +196,7 @@ class SingleAlbumDedupJob(RepairJob): if context.create_finding: try: - context.create_finding( + inserted = context.create_finding( job_id=self.job_id, finding_type='single_album_redundant', severity='info', @@ -235,7 +235,10 @@ class SingleAlbumDedupJob(RepairJob): 'artist_thumb_url': best_album_match.get('artist_thumb_url') or single.get('artist_thumb_url'), } ) - result.findings_created += 1 + if inserted: + result.findings_created += 1 + else: + result.findings_skipped_dedup += 1 except Exception as e: logger.debug("Error creating finding: %s", e) result.errors += 1 diff --git a/core/repair_jobs/track_number_repair.py b/core/repair_jobs/track_number_repair.py index 6246ac2c..8a569de6 100644 --- a/core/repair_jobs/track_number_repair.py +++ b/core/repair_jobs/track_number_repair.py @@ -237,7 +237,7 @@ class TrackNumberRepairJob(RepairJob): details['album_title'] = art_info['album_title'] if art_info.get('artist_name'): details['artist_name'] = art_info['artist_name'] - context.create_finding( + inserted = context.create_finding( job_id=self.job_id, finding_type='track_number_mismatch', severity='warning', @@ -248,7 +248,10 @@ class TrackNumberRepairJob(RepairJob): description=finding['description'], details=details ) - result.findings_created += 1 + if inserted: + result.findings_created += 1 + else: + result.findings_skipped_dedup += 1 else: if _repair_single_track(fpath, fname, api_tracks, len(api_tracks), title_sim, context): result.auto_fixed += 1 diff --git a/core/repair_jobs/unknown_artist_fixer.py b/core/repair_jobs/unknown_artist_fixer.py index 5d2b5bba..53216ed0 100644 --- a/core/repair_jobs/unknown_artist_fixer.py +++ b/core/repair_jobs/unknown_artist_fixer.py @@ -190,7 +190,7 @@ class UnknownArtistFixerJob(RepairJob): desc_parts.append(f'Path: → {expected_rel}') if context.create_finding: - context.create_finding( + inserted = context.create_finding( job_id=self.job_id, finding_type='unknown_artist', severity='warning', @@ -217,7 +217,10 @@ class UnknownArtistFixerJob(RepairJob): 'cover_url': corrected.get('image_url', ''), } ) - result.findings_created += 1 + if inserted: + result.findings_created += 1 + else: + result.findings_skipped_dedup += 1 else: # Live mode — apply fix try: diff --git a/core/repair_worker.py b/core/repair_worker.py index 3c42ef1e..8f9d8d6e 100644 --- a/core/repair_worker.py +++ b/core/repair_worker.py @@ -655,8 +655,18 @@ class RepairWorker: # ------------------------------------------------------------------ def _create_finding(self, job_id: str, finding_type: str, severity: str, entity_type: str, entity_id: str, file_path: str, - title: str, description: str, details: dict = None): - """Create a repair finding in the database.""" + title: str, description: str, details: dict = None) -> bool: + """Create a repair finding in the database. + + Returns: + True — a NEW pending row was inserted. + False — dedup-skipped (an equivalent row already exists with + status pending/resolved/dismissed) OR a DB error + occurred. Callers should only increment their + ``findings_created`` counter when this returns True + so the badge / scan log reports REAL new findings, + not silently-skipped duplicates. + """ conn = None try: conn = self.db._get_connection() @@ -672,7 +682,7 @@ class RepairWorker: """, (job_id, finding_type, entity_type, entity_id, file_path)) if cursor.fetchone(): - return # Already exists or was already fixed + return False # Already exists or was already fixed cursor.execute(""" INSERT INTO repair_findings @@ -685,8 +695,10 @@ class RepairWorker: json.dumps(details) if details else '{}' )) conn.commit() + return True except Exception as e: logger.debug("Error creating finding: %s", e) + return False finally: if conn: conn.close() diff --git a/tests/metadata/test_metadata_gap_filler.py b/tests/metadata/test_metadata_gap_filler.py index 06a8a263..cd87bcdc 100644 --- a/tests/metadata/test_metadata_gap_filler.py +++ b/tests/metadata/test_metadata_gap_filler.py @@ -127,7 +127,7 @@ def _make_context(conn): report_progress=lambda *args, **kwargs: None, sleep_or_stop=lambda seconds: False, mb_client=_FakeMBClient(), - create_finding=lambda **kwargs: findings.append(kwargs), + create_finding=lambda **kwargs: (findings.append(kwargs) or True), findings=findings, ) diff --git a/tests/test_album_completeness_job.py b/tests/test_album_completeness_job.py index 05f5736b..8d58a03c 100644 --- a/tests/test_album_completeness_job.py +++ b/tests/test_album_completeness_job.py @@ -528,7 +528,7 @@ def test_scan_uses_cached_api_track_count_without_expected_total_lookup(monkeypa job._get_expected_total = spy findings = [] - context = _make_job_context(db, create_finding=lambda **kwargs: findings.append(kwargs)) + context = _make_job_context(db, create_finding=lambda **kwargs: (findings.append(kwargs) or True)) result = job.scan(context) @@ -568,7 +568,7 @@ def test_scan_falls_back_to_api_and_persists_count_on_cache_miss(monkeypatch): ) findings = [] - context = _make_job_context(db, create_finding=lambda **kwargs: findings.append(kwargs)) + context = _make_job_context(db, create_finding=lambda **kwargs: (findings.append(kwargs) or True)) job = AlbumCompletenessJob() result = job.scan(context) @@ -606,7 +606,7 @@ def test_scan_ignores_track_count_completely(monkeypatch): ) findings = [] - context = _make_job_context(db, create_finding=lambda **kwargs: findings.append(kwargs)) + context = _make_job_context(db, create_finding=lambda **kwargs: (findings.append(kwargs) or True)) job = AlbumCompletenessJob() result = job.scan(context) diff --git a/tests/test_create_finding_dedup_counter.py b/tests/test_create_finding_dedup_counter.py new file mode 100644 index 00000000..7701a678 --- /dev/null +++ b/tests/test_create_finding_dedup_counter.py @@ -0,0 +1,208 @@ +"""Pin the bug fix where `_create_finding` silently dedup-skipped while +the caller's `findings_created` counter incremented anyway, causing +the maintenance job badge to inflate (e.g. "364 findings" displayed +when 0 new pending rows existed in the DB). + +Now `_create_finding` returns: +- True — a NEW pending row was inserted. +- False — dedup-skipped (an equivalent row already exists with status + pending/resolved/dismissed) OR a DB error occurred. + +Callers must only increment `findings_created` on True. Skipped-dedup +counter exposed separately as `findings_skipped_dedup` for log +transparency. +""" + +from __future__ import annotations + +import os +import sqlite3 +import tempfile +from unittest.mock import MagicMock + +import pytest + +from core.repair_jobs.base import JobResult +from core.repair_worker import RepairWorker + + +@pytest.fixture +def repair_worker_with_temp_db(): + """A RepairWorker wired to a temporary SQLite db with the + `repair_findings` table created. Returns the worker; tears the db + down after the test.""" + fd, path = tempfile.mkstemp(suffix='.db') + os.close(fd) + + conn = sqlite3.connect(path) + conn.execute(""" + CREATE TABLE repair_findings ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + job_id TEXT NOT NULL, + finding_type TEXT NOT NULL, + severity TEXT NOT NULL, + status TEXT NOT NULL DEFAULT 'pending', + entity_type TEXT, + entity_id TEXT, + file_path TEXT, + title TEXT, + description TEXT, + details_json TEXT, + user_action TEXT, + resolved_at TEXT, + created_at TEXT DEFAULT CURRENT_TIMESTAMP, + updated_at TEXT DEFAULT CURRENT_TIMESTAMP + ) + """) + conn.commit() + conn.close() + + db_mock = MagicMock() + db_mock._get_connection = lambda: sqlite3.connect(path) + + worker = RepairWorker.__new__(RepairWorker) + worker.db = db_mock + + yield worker, path + + try: + os.remove(path) + except OSError: + pass + + +def test_create_finding_returns_true_on_first_insert(repair_worker_with_temp_db): + worker, _ = repair_worker_with_temp_db + result = worker._create_finding( + job_id='dup_detector', + finding_type='duplicate_tracks', + severity='info', + entity_type='track', + entity_id='123', + file_path='/foo/bar.mp3', + title='Test finding', + description='desc', + ) + assert result is True + + +def test_create_finding_returns_false_on_dedup_pending(repair_worker_with_temp_db): + worker, _ = repair_worker_with_temp_db + # First insert + worker._create_finding( + job_id='dup_detector', finding_type='duplicate_tracks', + severity='info', entity_type='track', entity_id='123', + file_path='/foo/bar.mp3', title='T', description='D', + ) + # Re-call with same args — should dedup + result = worker._create_finding( + job_id='dup_detector', finding_type='duplicate_tracks', + severity='info', entity_type='track', entity_id='123', + file_path='/foo/bar.mp3', title='T', description='D', + ) + assert result is False + + +def test_create_finding_returns_false_on_dedup_dismissed(repair_worker_with_temp_db): + """The user dismissed a finding in a prior session. A new scan + that re-discovers the same issue must NOT increment the badge — + the row exists with status='dismissed'.""" + worker, path = repair_worker_with_temp_db + + # Seed the DB with a dismissed finding + conn = sqlite3.connect(path) + conn.execute(""" + INSERT INTO repair_findings + (job_id, finding_type, severity, status, entity_type, entity_id, + file_path, title, description) + VALUES (?, ?, 'info', 'dismissed', 'track', '123', '/foo/bar.mp3', 'T', 'D') + """, ('dup_detector', 'duplicate_tracks')) + conn.commit() + conn.close() + + result = worker._create_finding( + job_id='dup_detector', finding_type='duplicate_tracks', + severity='info', entity_type='track', entity_id='123', + file_path='/foo/bar.mp3', title='T', description='D', + ) + assert result is False + + +def test_create_finding_returns_false_on_dedup_resolved(repair_worker_with_temp_db): + """An auto-fix or manual repair previously resolved a finding. + Re-discovery of the same issue should NOT inflate the badge.""" + worker, path = repair_worker_with_temp_db + + conn = sqlite3.connect(path) + conn.execute(""" + INSERT INTO repair_findings + (job_id, finding_type, severity, status, entity_type, entity_id, + file_path, title, description) + VALUES (?, ?, 'info', 'resolved', 'track', '123', '/foo/bar.mp3', 'T', 'D') + """, ('dup_detector', 'duplicate_tracks')) + conn.commit() + conn.close() + + result = worker._create_finding( + job_id='dup_detector', finding_type='duplicate_tracks', + severity='info', entity_type='track', entity_id='123', + file_path='/foo/bar.mp3', title='T', description='D', + ) + assert result is False + + +def test_create_finding_inserts_again_when_distinct_entity(repair_worker_with_temp_db): + """A different entity (different track id) is a NEW finding — + must NOT be dedup-blocked by an unrelated finding's existence.""" + worker, _ = repair_worker_with_temp_db + worker._create_finding( + job_id='dup_detector', finding_type='duplicate_tracks', + severity='info', entity_type='track', entity_id='123', + file_path='/foo/bar.mp3', title='T', description='D', + ) + result = worker._create_finding( + job_id='dup_detector', finding_type='duplicate_tracks', + severity='info', entity_type='track', entity_id='456', + file_path='/foo/baz.mp3', title='T', description='D', + ) + assert result is True + + +# --------------------------------------------------------------------------- +# JobResult — findings_skipped_dedup field is exposed +# --------------------------------------------------------------------------- + + +def test_job_result_has_skipped_dedup_field(): + result = JobResult() + assert result.findings_skipped_dedup == 0 + result.findings_skipped_dedup += 1 + assert result.findings_skipped_dedup == 1 + + +# --------------------------------------------------------------------------- +# End-to-end pattern: caller counts only true inserts +# --------------------------------------------------------------------------- + + +def test_caller_pattern_counts_only_real_inserts(repair_worker_with_temp_db): + """Simulate a job loop calling create_finding 5 times for the + same finding. Only the FIRST should count toward findings_created; + the remaining 4 should count toward findings_skipped_dedup. Badge + must reflect 1, not 5.""" + worker, _ = repair_worker_with_temp_db + result = JobResult() + + for _ in range(5): + inserted = worker._create_finding( + job_id='dup_detector', finding_type='duplicate_tracks', + severity='info', entity_type='track', entity_id='123', + file_path='/foo/bar.mp3', title='T', description='D', + ) + if inserted: + result.findings_created += 1 + else: + result.findings_skipped_dedup += 1 + + assert result.findings_created == 1 + assert result.findings_skipped_dedup == 4 diff --git a/tests/test_duplicate_detector_slskd_dedup.py b/tests/test_duplicate_detector_slskd_dedup.py index 72b7da38..53de9f69 100644 --- a/tests/test_duplicate_detector_slskd_dedup.py +++ b/tests/test_duplicate_detector_slskd_dedup.py @@ -66,6 +66,8 @@ class _FakeContext: def _create_finding(self, **kwargs): self.findings.append(kwargs) + # Mirror real `_create_finding` contract: True on insert. + return True # --------------------------------------------------------------------------- diff --git a/tests/test_missing_cover_art.py b/tests/test_missing_cover_art.py index cb89896c..5313633f 100644 --- a/tests/test_missing_cover_art.py +++ b/tests/test_missing_cover_art.py @@ -119,7 +119,8 @@ def _make_context(conn, prefer_source=None): wait_if_paused=lambda: False, update_progress=lambda *args, **kwargs: None, report_progress=lambda *args, **kwargs: None, - create_finding=lambda **kwargs: findings.append(kwargs), + # Mirror real `_create_finding` contract: True on insert. + create_finding=lambda **kwargs: (findings.append(kwargs) or True), findings=findings, ) diff --git a/web_server.py b/web_server.py index b9e18809..7034281c 100644 --- a/web_server.py +++ b/web_server.py @@ -33079,7 +33079,9 @@ try: state['status'] = status state['progress'] = 100 state['finished_at'] = datetime.now(timezone.utc).isoformat() - summary = f'Done: {result.scanned} scanned, {result.auto_fixed} fixed, {result.findings_created} findings, {result.errors} errors' + skipped_dedup = getattr(result, 'findings_skipped_dedup', 0) or 0 + existing_part = f' ({skipped_dedup} already existed)' if skipped_dedup else '' + summary = f'Done: {result.scanned} scanned, {result.auto_fixed} fixed, {result.findings_created} findings{existing_part}, {result.errors} errors' state['log'].append({'type': 'success' if status == 'finished' else 'error', 'text': summary}) try: socketio.emit('repair:progress', {job_id: dict(state)}) diff --git a/webui/static/helper.js b/webui/static/helper.js index 5bbc125c..60a73c0e 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3435,6 +3435,7 @@ const WHATS_NEW = { { title: 'Internal: Typed Metadata Foundation', desc: 'internal — first step of a multi-pr migration to give the metadata pipeline a real contract. the codebase historically grew duck-typed extractors (`_extract_lookup_value(album_data, "id", "album_id", "collectionId", "release_id", default=...)`) at every consumer site because each provider returns its own response shape. ~150 of those across the codebase. new `core/metadata/types.py` defines canonical typed `Album` / `Track` / `Artist` dataclasses with strict required fields. per-source classmethod converters (from_spotify_dict, from_itunes_dict, from_deezer_dict, from_discogs_dict, from_musicbrainz_dict, from_hydrabase_dict) are the SINGLE place that knows each provider\'s wire shape. zero behavior changes in this pr — pure additive foundation. follow-up prs migrate consumers one at a time. full migration plan documented at docs/metadata-types-migration.md.', page: 'library' }, { title: 'Internal: Migrate Album-Info Builders to Typed Path', desc: 'internal — steps 2+3 of the typed metadata migration in one pr. two album-info builders now route through `Album.from__dict()` when the caller passes a known source: `_build_album_info` (used by every album-tracks lookup) and the embedded album section of `_build_single_import_context_payload` (used by single-track import context resolution). legacy duck-typed extraction stays as the fallback when source is empty/unknown, raw input isn\'t a dict, or the typed converter raises — so a converter bug can\'t break album resolution or import context. caller-provided album_id / album_name / artist_name fallbacks apply on the typed path the same way they did on legacy. zero behavior change for existing callers since they don\'t pass a source yet — opt-in only. 22 new tests pin the typed path, the legacy fallback, and parametrized coverage across registered providers.' }, { title: 'Internal: Migrate Discography + Quality Scanner to Typed Path', desc: 'internal — next round of the typed metadata migration. three more album-shape consumers now route through `Album.from__dict()` when the caller passes a known source: `_build_discography_release_dict` (artist discography release cards), `_build_artist_detail_release_card` (artist detail page release cards), and `_normalize_track_album` (quality scanner result normalization). legacy duck-typed extraction stays as the fallback when source is empty/unknown, raw input isn\'t a dict, or the typed converter raises — same safety contract as the prior migration steps. 20 new tests pin the typed path + legacy fallback + parametrized coverage across registered providers.' }, + { title: 'Fix: Maintenance Findings Badge Showed Inflated Count With Empty Findings Tab', desc: 'discord report (husoyo): duplicate detector and cover art filler badges showed "364 findings" / "31 findings" after a scan, but clicking into the findings tab showed nothing. cause: `_create_finding` silently dedup-skipped re-discovered issues (when an equivalent row already existed with status pending/resolved/dismissed) but the caller incremented `result.findings_created` regardless of whether a row was actually inserted. so on a re-scan that found the same problems as a prior scan, the badge snapshot recorded 364 even though zero NEW pending rows hit the db. fix: `_create_finding` now returns a bool (True on insert, False on dedup-skip / db error). all 16 repair jobs updated to only increment `findings_created` on True. new `findings_skipped_dedup` counter added to job results and surfaced in the scan log: "Done: 2791 scanned, 0 fixed, 0 findings (363 already existed), 0 errors" — so re-scans show a real count, and you can see at a glance how many findings were carried over from prior scans. also fixed a missing `job_id` kwarg in the album tag consistency job that was silently breaking finding creation for that scan.', page: 'library' }, { title: 'Discogs Collection in "Your Albums"', desc: 'discord request: pull your discogs collection into the your albums section on discover, similar to spotify liked albums. set your discogs personal access token on settings → connections (already there from prior work) and add discogs as one of the configured sources via the gear button on your albums. background fetcher pulls your full collection (all folders, all pages — capped at 5000 releases), normalizes artist names (strips discogs `(N)` disambiguation suffix), dedupes against any spotify/tidal/deezer-saved versions of the same album. clicking a discogs-only album opens with discogs context — full release detail (year, format, label, country, tracklist) from the /releases endpoint. clicking an album that exists in both your spotify saved AND discogs collection prefers spotify (download flow is more direct). discogs is physical-media-first so many releases won\'t have streaming equivalents — those still show in the grid but the modal flow may need to fall back to a name search to find a downloadable digital version.', page: 'discover' }, { title: 'Drop Redundant "Your Spotify Library" Section on Discover', desc: 'discover page used to show two near-identical sections: "Your Albums" (cross-source aggregator across spotify/deezer/etc) AND "Your Spotify Library" (spotify-only). same UI, same grid, same filter / sort / download-missing controls — the spotify-only one was a strict subset of what your albums already covers. removed it. spotify saved albums still surface via the your albums section with spotify as one of its configured sources (gear button → configure sources). backend collection / storage is unchanged — the watchlist scanner still populates the spotify_library_albums cache for your albums to read.', page: 'discover' }, { title: 'Library Disk Usage on Stats Page', desc: 'discord request (samuel [KC]): show how much disk space the library takes. new card on stats → system statistics shows total bytes + per-format breakdown (FLAC vs MP3 vs M4A bars). data comes from `tracks.file_size` populated during deep scan from whatever the media server already returns (plex MediaPart.size, jellyfin MediaSources[].Size, navidrome song.size, soulsync standalone os.path.getsize) — zero filesystem walk overhead. existing libraries see "Run a Deep Scan to populate" until the next deep scan fills in sizes; partial coverage shown as "X tracks measured (+Y pending)". migration is additive (NULL on legacy rows) so upgrading users have nothing to do.', page: 'stats' },