diff --git a/core/downloads/orphan_history.py b/core/downloads/orphan_history.py new file mode 100644 index 00000000..1d5effae --- /dev/null +++ b/core/downloads/orphan_history.py @@ -0,0 +1,45 @@ +"""Identify dead review-queue history rows whose file is gone (#934 follow-up). + +The Unverified/Quarantine review queue is fed from ``library_history`` — an +append-only log that is never pruned. When a file is deleted, replaced, or +re-downloaded elsewhere, its old ``unverified`` row lingers forever and can +never be healed (there's no file left to confirm). Those are *orphans*. + +This decides which rows are orphans, given a ``resolve(row) -> path | None`` +the caller wires to the real filesystem lookup. Pure (no DB, no filesystem) so +the rules — including the safety gate — are unit-testable. + +Safety gate: a filesystem check mass-false-positives when the library mount is +down (every file looks missing). So if EVERY reviewed file is unreachable and +there are enough rows to judge, we flag it ``suspicious`` and the caller refuses +to delete — better to clean nothing than to wipe a healthy log during an outage. +""" + +from __future__ import annotations + +from typing import Any, Callable, Sequence + + +def find_orphan_history_ids( + rows: Sequence[dict], + resolve: Callable[[dict], Any], + *, + min_for_safety: int = 5, +) -> dict: + """Return ``{'orphan_ids', 'checked', 'suspicious'}``. + + A row is an orphan when it has a non-empty ``file_path`` but ``resolve`` can + find no file for it. ``suspicious`` is True when every checked row is + missing and there are at least ``min_for_safety`` of them — the mount-down + signature; the caller should refuse to delete in that case. + """ + orphan_ids = [] + checked = 0 + for row in rows: + if not str((row.get('file_path') or '')).strip(): + continue + checked += 1 + if resolve(row) is None: + orphan_ids.append(row.get('id')) + suspicious = checked >= min_for_safety and len(orphan_ids) == checked + return {'orphan_ids': orphan_ids, 'checked': checked, 'suspicious': suspicious} diff --git a/database/music_database.py b/database/music_database.py index dd37aee3..818146fa 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,87 @@ 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). + + A basename match is title-guarded: a shared track-number filename + ("01 - Intro.flac") must NOT heal a different song, so when both the + history row and the candidate track carry a title they have to agree + (alphanumeric-lowercase) — the same guard the AcoustID matcher uses. An + exact-path match is unambiguous and needs no guard. + + 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() + # Cheap early-out: skip the tracks scan entirely when nothing is stuck. + cursor.execute( + "SELECT 1 FROM library_history WHERE verification_status = 'unverified' LIMIT 1") + if cursor.fetchone() is None: + return 0 + + def _norm(value): + return ''.join(ch for ch in str(value or '').lower() if ch.isalnum()) + + rank = {'verified': 1, 'human_verified': 2} + by_path = {} # exact path -> status (unambiguous; no title guard) + by_base = {} # basename -> list of (norm_title, status) + cursor.execute( + "SELECT file_path, verification_status, title FROM tracks " + "WHERE verification_status IN ('verified', 'human_verified') " + "AND file_path IS NOT NULL AND file_path != ''") + for fp, st, ttitle in cursor.fetchall(): + if not fp: + continue + if rank.get(st, 0) >= rank.get(by_path.get(fp), 0): + by_path[fp] = st + base = os.path.basename(fp) + if base: + by_base.setdefault(base, []).append((_norm(ttitle), st)) + + updates = [] + cursor.execute( + "SELECT id, file_path, title FROM library_history " + "WHERE verification_status = 'unverified' " + "AND file_path IS NOT NULL AND file_path != ''") + for rid, fp, rtitle in cursor.fetchall(): + target = by_path.get(fp) + if not target: + want = _norm(rtitle) + best = 0 + for ttitle, st in by_base.get(os.path.basename(fp or ''), ()): + # Title guard: require agreement only when BOTH titles are + # present (legacy rows may lack one — fall back to filename). + if want and ttitle and want != ttitle: + continue + if rank.get(st, 0) >= best: + best = rank.get(st, 0) + target = st + 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_orphan_history.py b/tests/test_orphan_history.py new file mode 100644 index 00000000..7018ed0c --- /dev/null +++ b/tests/test_orphan_history.py @@ -0,0 +1,49 @@ +"""Pure orphan-detection rules for the review-queue cleanup (#934 follow-up).""" + +from core.downloads.orphan_history import find_orphan_history_ids + + +def _rows(*specs): + # specs: (id, file_path, exists?) + return [{'id': i, 'file_path': p, '_exists': e} for i, p, e in specs] + + +def _resolve(row): + # stand-in for _resolve_history_audio_path: returns a path or None + return '/on/disk' if row.get('_exists') else None + + +def test_only_missing_files_are_orphans(): + rows = _rows((1, '/a.flac', True), (2, '/b.flac', False), (3, '/c.flac', True)) + out = find_orphan_history_ids(rows, _resolve) + assert out['orphan_ids'] == [2] + assert out['checked'] == 3 + assert out['suspicious'] is False + + +def test_rows_without_path_are_skipped(): + rows = _rows((1, '', False), (2, None, False), (3, '/c.flac', False)) + out = find_orphan_history_ids(rows, _resolve) + assert out['checked'] == 1 + assert out['orphan_ids'] == [3] + + +def test_all_missing_with_enough_rows_is_suspicious(): + rows = _rows(*[(i, f'/x{i}.flac', False) for i in range(6)]) + out = find_orphan_history_ids(rows, _resolve) + assert out['suspicious'] is True # mount-down signature -> caller refuses + assert len(out['orphan_ids']) == 6 + + +def test_all_missing_but_few_rows_is_not_suspicious(): + rows = _rows((1, '/x.flac', False), (2, '/y.flac', False)) + out = find_orphan_history_ids(rows, _resolve) + assert out['suspicious'] is False # too few to suspect an outage + assert out['orphan_ids'] == [1, 2] + + +def test_some_present_is_never_suspicious(): + rows = _rows(*[(i, f'/x{i}.flac', False) for i in range(8)], (99, '/real.flac', True)) + out = find_orphan_history_ids(rows, _resolve) + assert out['suspicious'] is False # at least one file exists -> library is up + assert 99 not in out['orphan_ids'] diff --git a/tests/test_unverified_history_reconcile.py b/tests/test_unverified_history_reconcile.py new file mode 100644 index 00000000..b23ef324 --- /dev/null +++ b/tests/test_unverified_history_reconcile.py @@ -0,0 +1,174 @@ +"""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, " + "title 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, title="Song"): + self._conn.execute( + "INSERT INTO tracks (id, file_path, title, verification_status) VALUES (?,?,?,?)", + (tid, path, title, 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" + + +def test_reconcile_basename_collision_does_not_false_heal(): + """Two different songs share the track-number filename '01 - Intro.flac'. + Only one is a verified track; the OTHER's stale history row must NOT inherit + that verified status just because the filename collides (title guard).""" + db = _InMemoryDB() + db._add_track(1, "/lib/AlbumA/01 - Intro.flac", "verified", title="Intro A") + # A genuinely different, still-unverified song with the same filename. + db._add_history("/transfer/AlbumB/01 - Intro.flac", "unverified", title="Intro B") + healed = db.reconcile_unverified_history_from_tracks() + assert healed == 0 + assert db._status_of(1) == "unverified" + + +def test_reconcile_basename_heals_when_titles_agree(): + """Same filename, same song (path drifted) — titles agree, so it heals.""" + db = _InMemoryDB() + db._add_track(1, "/lib/AlbumA/01 - Intro.flac", "verified", title="Intro") + db._add_history("/transfer/old/01 - Intro.flac", "unverified", title="Intro") + healed = db.reconcile_unverified_history_from_tracks() + assert healed == 1 + assert db._status_of(1) == "verified" + + +def test_reconcile_basename_heals_when_history_title_missing(): + """Legacy history rows may have no title — fall back to filename-only match + (mirrors the scanner matcher's allowance) so they still heal.""" + db = _InMemoryDB() + db._add_track(1, "/lib/A/01 - Song.flac", "verified", title="Whatever") + db._add_history("/transfer/old/01 - Song.flac", "unverified", title="") + healed = db.reconcile_unverified_history_from_tracks() + assert healed == 1 + assert db._status_of(1) == "verified" diff --git a/web_server.py b/web_server.py index 34aa2044..93d0c4c3 100644 --- a/web_server.py +++ b/web_server.py @@ -8220,6 +8220,40 @@ def delete_verification_item(history_id): return jsonify({"success": False, "error": str(e)}), 500 +@app.route('/api/verification/clean-orphans', methods=['POST']) +@admin_only +def clean_orphan_verification_items(): + """Remove dead review-queue rows whose file no longer exists anywhere + (deleted / replaced / re-downloaded elsewhere). These are append-only + library_history rows that can never be healed — there's no file left to + confirm — so they linger in the Unverified list forever (#934). + + User-initiated only, never automatic: it does a filesystem check, which + would mass-false-positive if the library mount were down. The pure helper + flags that signature (every reviewed file unreachable) and we refuse. Only + history ROWS are deleted — the files are already gone; this never removes a + file. Admin-only: it mutates shared review state.""" + try: + from core.downloads.orphan_history import find_orphan_history_ids + db = get_database() + rows = db.get_library_history_unverified() or [] + result = find_orphan_history_ids(rows, _resolve_history_audio_path) + if result['suspicious']: + return jsonify({ + "success": False, + "error": "Every reviewed file is unreachable — your library may be " + "offline right now. Nothing was removed.", + }), 409 + orphan_ids = result['orphan_ids'] + removed = db.delete_library_history_rows(orphan_ids) if orphan_ids else 0 + logger.info("[Verification] Cleaned %d orphaned review rows (checked %d)", + removed, result['checked']) + return jsonify({"success": True, "removed": removed, "checked": result['checked']}) + except Exception as e: + logger.error(f"[Verification] Clean orphans failed: {e}") + return jsonify({"success": False, "error": str(e)}), 500 + + @app.route('/api/quarantine//recover', methods=['POST']) def recover_quarantine_item(entry_id): """Fallback for legacy thin sidecars: move file into Staging so the user diff --git a/webui/static/pages-extra.js b/webui/static/pages-extra.js index 560ac261..b66404d9 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 @@ -3106,6 +3184,31 @@ async function verifDeleteAll(btn) { _adlFetch(); } +async function verifCleanOrphans(btn) { + // Removes review entries whose file is GONE (deleted / replaced / re-downloaded + // elsewhere) — dead log rows that can never be healed. Server-side it does a + // filesystem check and refuses if the whole library looks offline. Removes log + // rows only, never a file. + if (!await showConfirmDialog({ + title: 'Clean orphaned entries', + message: 'Remove review entries whose file no longer exists on disk (deleted, replaced, or re-downloaded elsewhere)? This removes only the stale log rows — it never deletes a file. It checks the filesystem first and refuses if your library looks offline.', + confirmText: 'Clean up', + cancelText: 'Cancel', + })) return; + if (btn) btn.disabled = true; + try { + const r = await fetch('/api/verification/clean-orphans', { method: 'POST' }); + const d = await r.json(); + if (d.success) { + showToast && showToast(`Removed ${d.removed} orphaned entr${d.removed === 1 ? 'y' : 'ies'} (checked ${d.checked})`, 'success'); + _adlFetch(); + } else { + showToast && showToast(d.error || 'Clean-up failed', 'error'); + } + } catch (e) { showToast && showToast('Clean-up failed', 'error'); } + if (btn) btn.disabled = false; +} + async function verifQuarApproveAll(btn) { const entries = _verifQuarEntries.filter(q => q.has_full_context); if (!entries.length) { @@ -3266,6 +3369,7 @@ function _adlRender() { ? ` ` : ` + `; // Without an AcoustID key nothing ever gets a verification status — // hide the pointless Unverified pill and show quarantine only. @@ -3293,6 +3397,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