danvics/soulsync
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 1

Artist Sync: unify with deep scan — server-diff stale removal, scoped to one artist

Browse source 
Per the original intent, "Sync" is now a single-artist deep scan: it uses the SAME
reconciliation source as the whole-library deep scan instead of a separate
disk-existence check.

- Phase 1 already calls the deep-scan worker's _process_artist_with_content; now it
  passes seen_track_ids so the pull collects the server's current track IDs for the
  artist (existing + new), exactly as the library deep scan does.
- Phase 2 stale = (artist's DB tracks for this server) − seen, then
  delete_stale_tracks(server_source) — identical mechanism to deep scan, scoped to
  one artist. The old os.path.exists disk check (which could mass-delete on an
  unreachable mount) is gone.
- Removal only runs when the server pull SUCCEEDED — no trustworthy 'seen' set
  (no server, unreachable, or a failed pull) → skip, never delete. The
  is_implausible_stale_removal guard (>50% unseen) stays as the same safety net
  deep scan has for a flaky response. @admin_only retained.

Tests rewritten for the server-diff model: removes only tracks the server no longer
has; guard skips when most are unseen; a failed pull skips removal entirely;
admin-only. 8 tests pass.
This commit is contained in:
BoulderBadgeDad 2026-06-10 19:43:25 -07:00
parent 4d1b9a5639
commit d82d02b921
3 changed files with 135 additions and 90 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

110
tests/test_artist_sync_stale_guard.py
View file

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

106
web_server.py
View file

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

9
webui/static/library.js
View file

@ -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`);