diff --git a/tests/test_artist_sync_stale_guard.py b/tests/test_artist_sync_stale_guard.py index 6715df75..0da6a1ac 100644 --- a/tests/test_artist_sync_stale_guard.py +++ b/tests/test_artist_sync_stale_guard.py @@ -1,14 +1,16 @@ -"""Artist 'Sync' button (enhanced tab) must not wipe an artist when storage is -unreachable, and must stay admin-only (it deletes tracks + albums). #828 pattern.""" +"""Artist 'Sync' = a single-artist deep scan: stale removal is a server-diff +(tracks the media server no longer has), with the same safety net + admin gate as +the whole-library deep scan. #828 pattern.""" from __future__ import annotations import os import tempfile +from types import SimpleNamespace import pytest -_TMP = tempfile.mkdtemp(prefix='soulsync-testdb-stale-') +_TMP = tempfile.mkdtemp(prefix='soulsync-testdb-sync2-') os.environ['DATABASE_PATH'] = os.path.join(_TMP, 's.db') os.environ['SOULSYNC_TEST_DB_READY'] = '1' @@ -20,58 +22,86 @@ def client(): return web_server.app.test_client() -def _seed(artist_id, *, missing, present, tmp_path): - """Artist with server_source NULL (skips the media-server pull phase), an - album, ``present`` tracks pointing at real files + ``missing`` at dead paths.""" +def _seed(artist_id, track_ids): + """Plex artist + album + tracks (server_source='plex').""" db = web_server.get_database() - album_id = artist_id * 10 + aid, album_id = str(artist_id), artist_id * 10 with db._get_connection() as conn: - conn.execute("INSERT OR REPLACE INTO artists (id, name, server_source) VALUES (?, ?, NULL)", - (artist_id, f'Artist {artist_id}')) + conn.execute("INSERT OR REPLACE INTO artists (id, name, server_source) VALUES (?, ?, 'plex')", + (aid, f'Artist {aid}')) conn.execute("INSERT OR REPLACE INTO albums (id, title, artist_id) VALUES (?, 'Alb', ?)", - (album_id, artist_id)) - tid = artist_id * 1000 - for i in range(present): - p = tmp_path / f"{artist_id}_present_{i}.flac" - p.write_bytes(b'\x00\x00') - conn.execute("INSERT INTO tracks (id, album_id, artist_id, title, track_number, duration, file_path) " - "VALUES (?, ?, ?, ?, 1, 100, ?)", (tid, album_id, artist_id, f'P{i}', str(p))) - tid += 1 - for i in range(missing): - conn.execute("INSERT INTO tracks (id, album_id, artist_id, title, track_number, duration, file_path) " - "VALUES (?, ?, ?, ?, 1, 100, ?)", (tid, album_id, artist_id, f'M{i}', f'/nonexistent/{artist_id}_{i}.flac')) - tid += 1 + (album_id, aid)) + for tid in track_ids: + conn.execute("INSERT OR REPLACE INTO tracks (id, album_id, artist_id, title, track_number, " + "duration, file_path, server_source) VALUES (?, ?, ?, ?, 1, 100, ?, 'plex')", + (tid, album_id, aid, f'T{tid}', f'/m/{tid}.flac')) conn.commit() -def _track_count(artist_id): +def _track_ids(artist_id): db = web_server.get_database() with db._get_connection() as conn: - return conn.execute("SELECT COUNT(*) c FROM tracks WHERE artist_id = ?", (artist_id,)).fetchone()['c'] + return {r['id'] for r in conn.execute("SELECT id FROM tracks WHERE artist_id = ?", (str(artist_id),))} -def test_all_files_missing_skips_removal_and_keeps_tracks(client, tmp_path): - _seed(9001, missing=8, present=0, tmp_path=tmp_path) - body = client.post('/api/library/artist/9001/sync').get_json() - assert body['success'] is True - assert body['removal_skipped'] is True # guard tripped +def _mock_server_pull(monkeypatch, *, seen, success=True): + """Fake the media server returning `seen` track IDs for the artist.""" + class _FakeServer: + def fetchItem(self, _id): + return SimpleNamespace(title=None) # truthy artist, no name change + + class _FakePlex: + server = _FakeServer() + + class _FakeEngine: + def client(self, name): + return _FakePlex() if name == 'plex' else None + + monkeypatch.setattr(web_server, 'media_server_engine', _FakeEngine()) + + class _FakeWorker: + def __init__(self, *a, **k): + self.database = None + def _process_artist_with_content(self, server_artist, skip_existing_tracks=False, seen_track_ids=None): + if seen_track_ids is not None: + seen_track_ids.update(seen) + return (success, 'ok', 0, 0) + + monkeypatch.setattr('core.database_update_worker.DatabaseUpdateWorker', _FakeWorker) + + +def test_removes_tracks_the_server_no_longer_has(client, monkeypatch): + _seed(7001, [f't{i}' for i in range(1, 11)]) # t1..t10 in DB + _mock_server_pull(monkeypatch, seen={f't{i}' for i in range(1, 9)}) # server has t1..t8 + body = client.post('/api/library/artist/7001/sync').get_json() + assert body['success'] and body['removal_skipped'] is False + assert body['stale_removed'] == 2 # t9,t10 gone from server + assert _track_ids(7001) == {f't{i}' for i in range(1, 9)} + + +def test_guard_skips_when_most_tracks_unseen(client, monkeypatch): + _seed(7002, [f't{i}' for i in range(1, 11)]) + _mock_server_pull(monkeypatch, seen={'t1'}) # 9/10 unseen → flaky response + body = client.post('/api/library/artist/7002/sync').get_json() + assert body['removal_skipped'] is True assert body['stale_removed'] == 0 - assert _track_count(9001) == 8 # nothing deleted — storage looked down + assert len(_track_ids(7002)) == 10 # nothing deleted -def test_a_few_missing_files_are_removed(client, tmp_path): - _seed(9002, missing=2, present=8, tmp_path=tmp_path) - body = client.post('/api/library/artist/9002/sync').get_json() - assert body['success'] is True - assert body['removal_skipped'] is False - assert body['stale_removed'] == 2 # only the genuinely-gone ones - assert _track_count(9002) == 8 +def test_failed_pull_skips_removal(client, monkeypatch): + _seed(7003, [f't{i}' for i in range(1, 11)]) + _mock_server_pull(monkeypatch, seen=set(), success=False) # pull failed → no trustworthy view + body = client.post('/api/library/artist/7003/sync').get_json() + assert body['removal_skipped'] is True + assert body['stale_removed'] == 0 + assert len(_track_ids(7003)) == 10 -def test_sync_is_admin_only(client, tmp_path): - _seed(9003, missing=2, present=2, tmp_path=tmp_path) +def test_sync_is_admin_only(client, monkeypatch): + _seed(7004, ['t1', 't2']) + _mock_server_pull(monkeypatch, seen={'t1', 't2'}) nonadmin = web_server.get_database().create_profile(name=f'u_{os.urandom(3).hex()}') with client.session_transaction() as sess: sess['profile_id'] = nonadmin - assert client.post('/api/library/artist/9003/sync').status_code == 403 - assert _track_count(9003) == 4 # untouched + assert client.post('/api/library/artist/7004/sync').status_code == 403 + assert len(_track_ids(7004)) == 2 # untouched diff --git a/web_server.py b/web_server.py index 9df659ad..4e2e1ac0 100644 --- a/web_server.py +++ b/web_server.py @@ -12271,6 +12271,11 @@ def sync_artist_library(artist_id): new_albums = 0 new_tracks = 0 name_updated = False + # Single-artist deep scan: collect the server track IDs we see during the + # pull. Stale removal (Phase 2) is a server-diff against this set — the SAME + # mechanism the whole-library deep scan uses, just scoped to one artist. + seen_track_ids = set() + pull_succeeded = False if server_source: media_client = None @@ -12325,68 +12330,73 @@ def sync_artist_library(artist_id): artist_name = new_name name_updated = True - # Process artist content (deep scan mode — skip existing, preserve enrichment) + # Process artist content (deep scan mode — skip existing, + # preserve enrichment) and collect the server's track IDs + # for this artist into seen_track_ids. success, details, new_albums, new_tracks = worker._process_artist_with_content( - server_artist, skip_existing_tracks=True + server_artist, skip_existing_tracks=True, seen_track_ids=seen_track_ids ) + # Only a successful pull gives a trustworthy 'seen' set; a + # failure/partial would make every track look stale. + pull_succeeded = bool(success) logger.info(f"[Artist Sync] Server pull for {artist_name}: {details}") except Exception as e: logger.error(f"[Artist Sync] Server pull failed for {artist_name}: {e}") - # ── Phase 2: Remove stale entries (files no longer on disk) ── + # ── Phase 2: Remove stale entries (tracks the server no longer has) ── + # Server-diff, exactly like the whole-library deep scan: stale = this + # artist's DB tracks that were NOT seen on the server during the pull. stale_removed = 0 empty_albums_removed = 0 removal_skipped = False - with database._get_connection() as conn: - cursor = conn.cursor() - cursor.execute("SELECT id, file_path FROM tracks WHERE artist_id = ?", (db_artist_id,)) - tracks = cursor.fetchall() - - stale_ids = [] - for track in tracks: - fp = track['file_path'] - if not fp: - stale_ids.append(track['id']) - continue - resolved = _resolve_library_file_path(fp) - if not resolved or not os.path.exists(resolved): - stale_ids.append(track['id']) - - # Storage-unreachable guard (#828 pattern): if an implausibly large - # fraction of this artist's tracks look missing, the disk/mount is - # almost certainly down — don't wipe the artist over a flaky mount. - from core.library.stale_guard import is_implausible_stale_removal - if is_implausible_stale_removal(len(stale_ids), len(tracks)): - removal_skipped = True - logger.warning( - f"[Artist Sync] {artist_name}: {len(stale_ids)}/{len(tracks)} tracks look " - f"missing — skipping stale removal (storage likely unreachable)" + if not pull_succeeded: + # No trustworthy server view (no server configured, unreachable, or the + # pull failed) — without it we can't tell stale from "server was down", + # so we remove nothing rather than risk wiping the artist. + removal_skipped = True + logger.info(f"[Artist Sync] {artist_name}: server pull unavailable — skipping stale removal") + else: + with database._get_connection() as conn: + cursor = conn.cursor() + cursor.execute( + "SELECT id FROM tracks WHERE artist_id = ? AND server_source = ?", + (db_artist_id, server_source), ) - elif stale_ids: - placeholders = ','.join('?' for _ in stale_ids) - cursor.execute(f"DELETE FROM tracks WHERE id IN ({placeholders})", stale_ids) - stale_removed = len(stale_ids) + artist_track_ids = {row['id'] for row in cursor.fetchall()} + stale = artist_track_ids - seen_track_ids + + # Same safety net as deep scan (#828): if an implausibly large share + # of the artist's tracks went unseen, treat it as a flaky server + # response and skip rather than mass-delete. + from core.library.stale_guard import is_implausible_stale_removal + if is_implausible_stale_removal(len(stale), len(artist_track_ids)): + removal_skipped = True + logger.warning( + f"[Artist Sync] {artist_name}: {len(stale)}/{len(artist_track_ids)} tracks " + f"unseen on server — skipping stale removal (likely a flaky response)" + ) + elif stale: + stale_removed = database.delete_stale_tracks(stale, server_source) - # Only prune empty albums when we actually removed tracks — never on the - # skipped path, where "empty" would be a false reading of a down mount. - # ``album_id IS NOT NULL`` avoids the NOT IN-with-NULL gotcha that would - # otherwise no-op this cleanup whenever a track has a null album_id. if not removal_skipped: - cursor.execute(""" - DELETE FROM albums WHERE artist_id = ? - AND id NOT IN (SELECT DISTINCT album_id FROM tracks WHERE album_id IS NOT NULL) - """, (db_artist_id,)) - empty_albums_removed = cursor.rowcount - - cursor.execute(""" - UPDATE albums SET track_count = ( - SELECT COUNT(*) FROM tracks WHERE tracks.album_id = albums.id - ) WHERE artist_id = ? - """, (db_artist_id,)) - - conn.commit() + with database._get_connection() as conn: + cursor = conn.cursor() + # Prune albums left with no tracks. ``album_id IS NOT NULL`` + # avoids the NOT IN-with-NULL gotcha that would otherwise no-op + # this whenever a track has a null album_id. + cursor.execute(""" + DELETE FROM albums WHERE artist_id = ? + AND id NOT IN (SELECT DISTINCT album_id FROM tracks WHERE album_id IS NOT NULL) + """, (db_artist_id,)) + empty_albums_removed = cursor.rowcount + cursor.execute(""" + UPDATE albums SET track_count = ( + SELECT COUNT(*) FROM tracks WHERE tracks.album_id = albums.id + ) WHERE artist_id = ? + """, (db_artist_id,)) + conn.commit() logger.warning(f"[Artist Sync] {artist_name}: +{new_albums} albums, +{new_tracks} tracks, " f"-{stale_removed} stale, -{empty_albums_removed} empty albums" diff --git a/webui/static/library.js b/webui/static/library.js index e0731f18..4eaf6c03 100644 --- a/webui/static/library.js +++ b/webui/static/library.js @@ -3193,8 +3193,13 @@ function renderArtistMetaPanel(artist) { const data = await res.json(); if (data.success) { if (data.removal_skipped) { - // Storage looked unreachable — we deliberately did NOT delete. - showToast(`${data.artist_name}: most files looked missing — skipped removal in case your music storage is offline. Check it's mounted, then sync again.`, 'warning'); + // Couldn't get a trustworthy server view — we deliberately did NOT delete. + const parts = []; + if (data.new_albums > 0) parts.push(`+${data.new_albums} albums`); + if (data.new_tracks > 0) parts.push(`+${data.new_tracks} tracks`); + if (data.name_updated) parts.push('name updated'); + const added = parts.length ? ` (${parts.join(', ')})` : ''; + showToast(`${data.artist_name}: couldn't fully confirm against your media server — skipped removing tracks to be safe${added}.`, 'warning'); } else { const parts = []; if (data.new_albums > 0) parts.push(`+${data.new_albums} albums`);