Merge pull request #938 from nick2000713/fix/unverified-acoustid-934

Fix/unverified acoustid 934
This commit is contained in:
BoulderBadgeDad 2026-06-27 22:14:01 -07:00 committed by GitHub
commit d84fba14ba
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
6 changed files with 503 additions and 0 deletions

View file

@ -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}

View file

@ -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:

View file

@ -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']

View file

@ -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"

View file

@ -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/<entry_id>/recover', methods=['POST'])
def recover_quarantine_item(entry_id):
"""Fallback for legacy thin sidecars: move file into Staging so the user

View file

@ -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 [
`<div><span class="verif-detail-label">Why flagged:</span> ${_adlEsc(reason)}</div>`,
source ? `<div><span class="verif-detail-label">Download source:</span> ${_adlEsc(source)}</div>` : '',
dl.quality ? `<div><span class="verif-detail-label">Quality:</span> ${_adlEsc(dl.quality)}</div>` : '',
dl.file_path ? `<div><span class="verif-detail-label">File:</span> ${_adlEsc(dl.file_path)}</div>` : '',
dl.created_at ? `<div><span class="verif-detail-label">Downloaded:</span> ${_adlEsc(dl.created_at)}</div>` : '',
].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
? `<img class="adl-row-art" src="${_adlEsc(dl.artwork)}" alt="" onerror="this.style.display='none'">`
: '<div class="adl-row-art adl-row-art-empty"></div>';
const detailsOpen = _verifUnvOpenDetails.has(_verifUnvKey(dl));
const actions = hid ? `
<button class="verif-act verif-act-play" onclick="verifPlay('${hid}')" title="Play the downloaded file in the media player"></button>
<button class="verif-act" onclick="verifCompare('${hid}', this)" title="Find this track on Soulseek/streaming sources and play it in the media player — compare against your file"></button>
<button class="verif-act" onclick="verifAudit('${hid}')" title="Open the full audit trail for this download (lifecycle, embedded tags, lyrics)">🔍</button>
<button class="verif-act verif-act-ok" onclick="verifApprove('${hid}', this)" title="Approve: mark as human-verified (tag + DB). The AcoustID scanner will skip it."></button>
<button class="verif-act verif-act-del" onclick="verifDelete('${hid}', this)" title="Wrong file: delete it from disk and remove this entry">🗑</button>` : '';
return `<div class="adl-row adl-row-completed verif-quar-row" data-task-id="${_adlEsc(dl.task_id || '')}" onclick="verifUnvInspect(${idx})" title="Click to show/hide details (why it was flagged, source, quality, file)">
${artHtml}
<div class="adl-row-info">
<div class="adl-row-title">${title}</div>
${meta ? `<div class="adl-row-meta">${meta}</div>` : ''}
${source ? `<div class="adl-row-batch">${source}</div>` : ''}
<div class="verif-quar-details" id="verif-unv-details-${idx}" style="display:${detailsOpen ? '' : 'none'}">${_verifUnvDetailRows(dl)}</div>
</div>
<div class="verif-actions" onclick="event.stopPropagation()">
${_verifReasonBadge(dl)}
${dl.quality ? `<span class="adl-quality-chip" title="Audio quality of the downloaded file (read from the file itself)">${_adlEsc(dl.quality)}</span>` : ''}
${timeAgo ? `<span class="verif-time">${timeAgo}</span>` : ''}
${actions}
</div>
</div>`;
}
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() {
? `<button class="adl-filter-banner-clear" onclick="verifQuarApproveAll(this)" title="Approve + re-import every quarantined file (marked human-verified)">✔ Approve all</button>
<button class="adl-filter-banner-clear verif-bulk-danger" onclick="verifQuarClearAll(this)" title="Permanently delete every quarantined file">🗑 Clear all</button>`
: `<button class="adl-filter-banner-clear" onclick="verifApproveAll(this)" title="Mark every listed entry as human-verified">✔ Approve all</button>
<button class="adl-filter-banner-clear" onclick="verifCleanOrphans(this)" title="Remove dead entries whose file no longer exists (deleted / replaced). Removes log rows only — never a file.">🧹 Clean orphaned</button>
<button class="adl-filter-banner-clear verif-bulk-danger" onclick="verifDeleteAll(this)" title="Delete every listed file from disk and remove its entry">🗑 Delete all</button>`;
// 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