diff --git a/core/library/stale_guard.py b/core/library/stale_guard.py new file mode 100644 index 00000000..c614934c --- /dev/null +++ b/core/library/stale_guard.py @@ -0,0 +1,45 @@ +"""Guard against mass-deleting library rows when storage is unreachable. + +The library "sync" / cleanup paths mark a track stale when its file isn't on +disk and then delete the row. But ``os.path.exists`` returns False for EVERY +file when the music storage is momentarily unavailable — a sleeping NAS, a +dropped network mount, an unmounted Docker volume, a WSL mount hiccup. Without a +guard, one click then wipes the whole artist/library from the DB even though the +files are fine. + +This mirrors the safety the deep-scan path already had (``database_update_worker`` +skips removal when stale > 50% of a >100-track library — issue #828). Centralised +here so every stale-removal site can share one tested rule. +""" + +from __future__ import annotations + +# Don't second-guess tiny sets — a 2-track artist legitimately losing both files +# shouldn't be blocked. Above this, an implausibly large missing fraction almost +# always means "storage down", not "files actually deleted". +DEFAULT_MIN_TOTAL = 5 +DEFAULT_MAX_MISSING_FRACTION = 0.5 + + +def is_implausible_stale_removal( + missing_count: int, + total_count: int, + *, + min_total: int = DEFAULT_MIN_TOTAL, + max_fraction: float = DEFAULT_MAX_MISSING_FRACTION, +) -> bool: + """True when ``missing_count`` is too large a share of ``total_count`` to be a + real deletion — i.e. the storage is probably unreachable and the caller should + SKIP removal (and warn) rather than delete. + + Returns False for small sets (< ``min_total``) so normal cleanup of a few + genuinely-gone files still works. + """ + if total_count <= 0 or missing_count <= 0: + return False + if total_count < min_total: + return False + return missing_count > total_count * max_fraction + + +__all__ = ["is_implausible_stale_removal", "DEFAULT_MIN_TOTAL", "DEFAULT_MAX_MISSING_FRACTION"] diff --git a/tests/test_artist_sync_stale_guard.py b/tests/test_artist_sync_stale_guard.py new file mode 100644 index 00000000..6715df75 --- /dev/null +++ b/tests/test_artist_sync_stale_guard.py @@ -0,0 +1,77 @@ +"""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.""" + +from __future__ import annotations + +import os +import tempfile + +import pytest + +_TMP = tempfile.mkdtemp(prefix='soulsync-testdb-stale-') +os.environ['DATABASE_PATH'] = os.path.join(_TMP, 's.db') +os.environ['SOULSYNC_TEST_DB_READY'] = '1' + +web_server = pytest.importorskip('web_server') + + +@pytest.fixture +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.""" + db = web_server.get_database() + album_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 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 + conn.commit() + + +def _track_count(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'] + + +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 + assert body['stale_removed'] == 0 + assert _track_count(9001) == 8 # nothing deleted — storage looked down + + +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_sync_is_admin_only(client, tmp_path): + _seed(9003, missing=2, present=2, tmp_path=tmp_path) + 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 diff --git a/tests/test_stale_guard.py b/tests/test_stale_guard.py new file mode 100644 index 00000000..6259847e --- /dev/null +++ b/tests/test_stale_guard.py @@ -0,0 +1,28 @@ +"""Storage-unreachable guard for library stale-removal (artist sync, #828 pattern).""" + +from __future__ import annotations + +from core.library.stale_guard import is_implausible_stale_removal as g + + +def test_all_missing_in_a_real_collection_is_blocked(): + # 40/40 missing → almost certainly a down mount, not 40 real deletions. + assert g(40, 40) is True + assert g(30, 40) is True # 75% missing — still implausible + + +def test_a_few_genuinely_missing_files_are_allowed(): + assert g(3, 40) is False # normal cleanup of a few gone files + assert g(20, 40) is False # exactly 50% is NOT over the threshold + + +def test_tiny_sets_are_never_blocked(): + # A 2-track artist legitimately losing both must still clean up. + assert g(2, 2) is False + assert g(4, 4) is False # below min_total (5) + + +def test_edge_inputs(): + assert g(0, 0) is False + assert g(0, 100) is False # nothing missing + assert g(5, 5) is True # min_total met, all missing diff --git a/web_server.py b/web_server.py index 88973d39..9df659ad 100644 --- a/web_server.py +++ b/web_server.py @@ -12225,6 +12225,7 @@ def redownload_start(track_id): @app.route('/api/library/artist//sync', methods=['POST']) +@admin_only def sync_artist_library(artist_id): """Bidirectional sync: pull new content from media server AND remove stale entries.""" try: @@ -12336,6 +12337,7 @@ def sync_artist_library(artist_id): # ── Phase 2: Remove stale entries (files no longer on disk) ── stale_removed = 0 empty_albums_removed = 0 + removal_skipped = False with database._get_connection() as conn: cursor = conn.cursor() @@ -12352,16 +12354,31 @@ def sync_artist_library(artist_id): if not resolved or not os.path.exists(resolved): stale_ids.append(track['id']) - if stale_ids: + # 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)" + ) + 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) - cursor.execute(""" - DELETE FROM albums WHERE artist_id = ? - AND id NOT IN (SELECT DISTINCT album_id FROM tracks) - """, (db_artist_id,)) - empty_albums_removed = cursor.rowcount + # 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 = ( @@ -12372,7 +12389,8 @@ def sync_artist_library(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") + f"-{stale_removed} stale, -{empty_albums_removed} empty albums" + f"{' (removal skipped — storage unreachable)' if removal_skipped else ''}") return jsonify({ "success": True, @@ -12382,6 +12400,7 @@ def sync_artist_library(artist_id): "new_tracks": new_tracks, "stale_removed": stale_removed, "empty_albums_removed": empty_albums_removed, + "removal_skipped": removal_skipped, }) except Exception as e: diff --git a/webui/static/library.js b/webui/static/library.js index 34823ce6..e0731f18 100644 --- a/webui/static/library.js +++ b/webui/static/library.js @@ -3192,16 +3192,21 @@ function renderArtistMetaPanel(artist) { const res = await fetch(`/api/library/artist/${artist.id}/sync`, { method: 'POST' }); const data = await res.json(); if (data.success) { - 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.stale_removed > 0) parts.push(`${data.stale_removed} stale removed`); - if (data.empty_albums_removed > 0) parts.push(`${data.empty_albums_removed} empty albums cleaned`); - if (data.name_updated) parts.push('name updated'); - if (parts.length === 0) parts.push('Already in sync'); - showToast(`${data.artist_name}: ${parts.join(', ')}`, 'success'); - // Refresh enhanced view if anything changed - if (data.stale_removed > 0 || data.empty_albums_removed > 0) { + 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'); + } else { + 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.stale_removed > 0) parts.push(`${data.stale_removed} stale removed`); + if (data.empty_albums_removed > 0) parts.push(`${data.empty_albums_removed} empty albums cleaned`); + if (data.name_updated) parts.push('name updated'); + if (parts.length === 0) parts.push('Already in sync'); + showToast(`${data.artist_name}: ${parts.join(', ')}`, 'success'); + } + // Refresh enhanced view if anything changed (additions OR removals) + if (data.new_albums > 0 || data.new_tracks > 0 || data.stale_removed > 0 || data.empty_albums_removed > 0) { loadEnhancedViewData(artist.id); } } else {