Artist Sync: guard stale-removal against an unreachable mount + gate it admin-only

The enhanced-tab "Sync" button's stale-removal phase deleted any track whose file
wasn't on disk, with NO guard — so if the music storage was momentarily
unavailable (sleeping NAS, dropped mount, unmounted Docker volume, WSL hiccup),
os.path.exists returned False for EVERY file and one click wiped the whole artist
(tracks + their now-"empty" albums) from the DB. The deep-scan path already had a
50%-stale safety net (#828); this endpoint never got one.

- New core/library/stale_guard.py: is_implausible_stale_removal(missing, total) —
  a tested rule (skip removal when missing > 50% of a >=5-track set), centralised
  so every stale-removal site can share it.
- sync_artist_library: if the guard trips, SKIP removal (delete nothing), return
  removal_skipped + warn; the frontend shows "storage may be offline — skipped"
  instead of silently deleting. Empty-album cleanup now also only runs on the
  non-skipped path and uses `album_id IS NOT NULL` (fixes the NOT IN-with-NULL
  no-op). Frontend also refreshes the view on additions, not just removals.
- @admin_only on the endpoint — it deletes tracks + albums but was ungated, while
  the sibling delete_album endpoint is gated.

Deep scan was already safe (different mechanism: server-diff + its own 50% guard).

Tests: guard unit rules; endpoint skips removal when all files missing (keeps the
tracks), removes only the genuinely-gone few otherwise, and 403s for non-admins.
7 new tests pass.

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

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

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

web_server.py

@@ -12225,6 +12225,7 @@ def redownload_start(track_id):
 
 
 @app.route('/api/library/artist/<artist_id>/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("""
+            # Only prune empty albums when we actually removed tracks — never on the
-                DELETE FROM albums WHERE artist_id = ?
+            # skipped path, where "empty" would be a false reading of a down mount.
-                AND id NOT IN (SELECT DISTINCT album_id FROM tracks)
+            # ``album_id IS NOT NULL`` avoids the NOT IN-with-NULL gotcha that would
-            """, (db_artist_id,))
+            # otherwise no-op this cleanup whenever a track has a null album_id.
-            empty_albums_removed = cursor.rowcount
+            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:

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.removal_skipped) {
-                if (data.new_albums > 0) parts.push(`+${data.new_albums} albums`);
+                    // Storage looked unreachable — we deliberately did NOT delete.
-                if (data.new_tracks > 0) parts.push(`+${data.new_tracks} tracks`);
+                    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');
-                if (data.stale_removed > 0) parts.push(`${data.stale_removed} stale removed`);
+                } else {
-                if (data.empty_albums_removed > 0) parts.push(`${data.empty_albums_removed} empty albums cleaned`);
+                    const parts = [];
-                if (data.name_updated) parts.push('name updated');
+                    if (data.new_albums > 0) parts.push(`+${data.new_albums} albums`);
-                if (parts.length === 0) parts.push('Already in sync');
+                    if (data.new_tracks > 0) parts.push(`+${data.new_tracks} tracks`);
-                showToast(`${data.artist_name}: ${parts.join(', ')}`, 'success');
+                    if (data.stale_removed > 0) parts.push(`${data.stale_removed} stale removed`);
-                // Refresh enhanced view if anything changed
+                    if (data.empty_albums_removed > 0) parts.push(`${data.empty_albums_removed} empty albums cleaned`);
-                if (data.stale_removed > 0 || data.empty_albums_removed > 0) {
+                    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 {