diff --git a/config/settings.py b/config/settings.py index aa10bff5..5ff13f6a 100644 --- a/config/settings.py +++ b/config/settings.py @@ -620,7 +620,13 @@ class ConfigManager: "embed_tags": True }, "playlist_sync": { - "create_backup": True + "create_backup": True, + # How a re-sync writes to the server playlist: + # replace — delete + recreate (default; today's behavior) + # reconcile — edit in place (add/remove delta), preserving the + # playlist's custom image, description, and identity (#792) + # append — only add new tracks, never remove + "mode": "replace" }, "settings": { "audio_quality": "flac" diff --git a/core/discovery/sync.py b/core/discovery/sync.py index c3b513ce..4fd447ce 100644 --- a/core/discovery/sync.py +++ b/core/discovery/sync.py @@ -508,7 +508,13 @@ def run_sync_task( # don't want persisted to app.log. _synced = getattr(result, 'synced_tracks', 0) logger.info(f"[PLAYLIST IMAGE] has_image={bool(playlist_image_url)}, synced_tracks={_synced}") - if playlist_image_url and _synced > 0: + # In reconcile mode the whole point (#792) is to NOT touch the playlist's + # existing image — pushing the source image here would re-clobber a + # user's custom poster every sync, the exact bug reconcile fixes. So skip + # the auto image push for reconcile (the playlist keeps its own art). + if sync_mode == 'reconcile': + logger.info("[PLAYLIST IMAGE] reconcile mode — preserving existing playlist image") + elif playlist_image_url and _synced > 0: try: active_server = deps.config_manager.get_active_media_server() logger.info(f"[PLAYLIST IMAGE] active_server={active_server}") diff --git a/core/jellyfin_client.py b/core/jellyfin_client.py index 20a2594c..4937c2c0 100644 --- a/core/jellyfin_client.py +++ b/core/jellyfin_client.py @@ -1605,6 +1605,80 @@ class JellyfinClient(MediaServerClient): logger.error(f"Error appending to Jellyfin playlist '{playlist_name}': {e}") return False + def reconcile_playlist(self, playlist_name: str, tracks) -> bool: + """In-place reconcile (#792): add missing + remove gone on the existing + playlist (POST/DELETE /Playlists/{id}/Items) so its poster, name, and Id + survive — no delete/recreate. Removal needs each entry's PlaylistItemId, + which we fetch from /Playlists/{id}/Items. Creates the playlist if + missing. Returns False so the caller can fall back to replace.""" + if not self.ensure_connection(): + return False + try: + import requests + from core.sync.playlist_edit import plan_playlist_reconcile + existing = self.get_playlist_by_name(playlist_name) + if not existing: + logger.info(f"Jellyfin reconcile: '{playlist_name}' doesn't exist — creating") + return self.create_playlist(playlist_name, tracks) + + playlist_id = existing.id + # Entries carry both the track Id and the PlaylistItemId (entry id); + # removal is by EntryIds, not track Ids. + entries = [] # list of (track_id, entry_id) in playlist order + resp = self._make_request(f'/Playlists/{playlist_id}/Items', {'UserId': self.user_id}) + if resp: + for item in resp.get('Items', []): + tid = str(item.get('Id') or '') + eid = str(item.get('PlaylistItemId') or '') + if tid: + entries.append((tid, eid)) + + current_ids = [tid for tid, _ in entries] + desired_ids = [] + for t in tracks: + tid = (str(t.id) if hasattr(t, 'id') and t.id + else str(t.get('Id') or t.get('id') or '') if isinstance(t, dict) else '') + if tid and self._is_valid_guid(tid): + desired_ids.append(tid) + + plan = plan_playlist_reconcile(current_ids, desired_ids) + hdr = {'X-Emby-Token': self.api_key} + + if plan['add']: + for i in range(0, len(plan['add']), 100): + batch = plan['add'][i:i + 100] + r = requests.post( + f"{self.base_url}/Playlists/{playlist_id}/Items", + params={'Ids': ','.join(batch), 'UserId': self.user_id}, + headers=hdr, timeout=30, + ) + if r.status_code not in (200, 204): + logger.error(f"Jellyfin reconcile add failed: HTTP {r.status_code}") + return False + + if plan['remove']: + remove_set = set(plan['remove']) + entry_ids = [eid for tid, eid in entries if tid in remove_set and eid] + for i in range(0, len(entry_ids), 100): + batch = entry_ids[i:i + 100] + r = requests.delete( + f"{self.base_url}/Playlists/{playlist_id}/Items", + params={'EntryIds': ','.join(batch)}, + headers=hdr, timeout=30, + ) + if r.status_code not in (200, 204): + logger.error(f"Jellyfin reconcile remove failed: HTTP {r.status_code}") + return False + + logger.info( + f"Jellyfin reconcile '{playlist_name}': +{len(plan['add'])} / " + f"-{len(plan['remove'])} (playlist preserved)" + ) + return True + except Exception as e: + logger.error(f"Error reconciling Jellyfin playlist '{playlist_name}': {e}") + return False + def update_playlist(self, playlist_name: str, tracks) -> bool: """Update an existing playlist or create it if it doesn't exist""" if not self.ensure_connection(): diff --git a/core/navidrome_client.py b/core/navidrome_client.py index ee8c23ee..0374c4b7 100644 --- a/core/navidrome_client.py +++ b/core/navidrome_client.py @@ -1124,6 +1124,60 @@ class NavidromeClient(MediaServerClient): logger.error(f"Error appending to Navidrome playlist '{playlist_name}': {e}") return False + def reconcile_playlist(self, playlist_name: str, tracks) -> bool: + """In-place reconcile (#792): add missing + remove gone via Subsonic + updatePlaylist (songIdToAdd / songIndexToRemove), keeping the existing + playlist object so its comment/identity survive — no delete/recreate. + Creates the playlist if missing. Returns False so the caller can fall + back to replace on any failure.""" + if not self.ensure_connection(): + return False + try: + from core.sync.playlist_edit import plan_playlist_reconcile + existing_playlists = self.get_playlists_by_name(playlist_name) + if not existing_playlists: + logger.info(f"Navidrome reconcile: '{playlist_name}' doesn't exist — creating") + return self.create_playlist(playlist_name, tracks) + + primary = existing_playlists[0] + existing_tracks = self.get_playlist_tracks(primary.id) + current_ids = [str(t.id) for t in existing_tracks if getattr(t, 'id', None)] + desired_ids = [] + for t in tracks: + tid = (str(t.ratingKey) if hasattr(t, 'ratingKey') + else str(t.id) if getattr(t, 'id', None) + else str(t.get('id', '')) if isinstance(t, dict) else '') + if tid: + desired_ids.append(tid) + + plan = plan_playlist_reconcile(current_ids, desired_ids) + if not plan['add'] and not plan['remove']: + return True + + params = {'playlistId': primary.id} + if plan['add']: + params['songIdToAdd'] = plan['add'] + if plan['remove']: + # Indices into the CURRENT list; remove descending so earlier + # removals don't shift the indices of later ones. + remove_set = set(plan['remove']) + params['songIndexToRemove'] = sorted( + (i for i, cid in enumerate(current_ids) if cid in remove_set), + reverse=True, + ) + response = self._make_request('updatePlaylist', params) + if response and response.get('status') == 'ok': + logger.info( + f"Navidrome reconcile '{playlist_name}': +{len(plan['add'])} / " + f"-{len(plan['remove'])} (playlist preserved)" + ) + return True + logger.error(f"Navidrome reconcile failed for '{playlist_name}'") + return False + except Exception as e: + logger.error(f"Error reconciling Navidrome playlist '{playlist_name}': {e}") + return False + def update_playlist(self, playlist_name: str, tracks) -> bool: """Update an existing playlist or create it if it doesn't exist. Handles duplicates.""" if not self.ensure_connection(): diff --git a/core/plex_client.py b/core/plex_client.py index 9dcc0dda..29310ab9 100644 --- a/core/plex_client.py +++ b/core/plex_client.py @@ -667,6 +667,53 @@ class PlexClient(MediaServerClient): logger.error(f"Error appending to Plex playlist '{playlist_name}': {e}") return False + def reconcile_playlist(self, playlist_name: str, tracks: List[TrackInfo]) -> bool: + """In-place reconcile (#792): bring an existing playlist's tracks to match + ``tracks`` by adding the missing ones and removing the gone ones, WITHOUT + deleting/recreating the playlist — so its custom poster, summary, and + ratingKey survive the sync. Creates the playlist if it doesn't exist. + + Returns False on any failure so the caller can fall back to replace.""" + if not self.ensure_connection(): + return False + try: + from core.sync.playlist_edit import plan_playlist_reconcile + try: + existing = self.server.playlist(playlist_name) + except NotFound: + logger.info(f"Plex reconcile: '{playlist_name}' doesn't exist — creating") + return self.create_playlist(playlist_name, tracks) + + current_items = [i for i in existing.items() if hasattr(i, 'ratingKey')] + current_ids = [str(i.ratingKey) for i in current_items] + desired_ids = [str(t.ratingKey) for t in tracks if hasattr(t, 'ratingKey')] + plan = plan_playlist_reconcile(current_ids, desired_ids) + + add_set, remove_set = set(plan['add']), set(plan['remove']) + to_add = [t for t in tracks if hasattr(t, 'ratingKey') and str(t.ratingKey) in add_set] + to_remove = [i for i in current_items if str(i.ratingKey) in remove_set] + + if to_add: + existing.addItems(to_add) + if to_remove: + try: + existing.removeItems(to_remove) + except (AttributeError, TypeError): + # Older plexapi: no batch removeItems — drop one at a time. + for it in to_remove: + try: + existing.removeItem(it) + except Exception as one_err: + logger.debug("Plex reconcile: removeItem failed: %s", one_err) + logger.info( + f"Plex reconcile '{playlist_name}': +{len(to_add)} / -{len(to_remove)} " + f"(playlist preserved)" + ) + return True + except Exception as e: + logger.error(f"Error reconciling Plex playlist '{playlist_name}': {e}") + return False + def update_playlist(self, playlist_name: str, tracks: List[TrackInfo]) -> bool: if not self.ensure_connection(): return False diff --git a/core/sync/playlist_edit.py b/core/sync/playlist_edit.py index c2cf34d5..80325238 100644 --- a/core/sync/playlist_edit.py +++ b/core/sync/playlist_edit.py @@ -71,4 +71,64 @@ def remove_one_occurrence( return ids, False -__all__ = ["plan_playlist_add", "remove_one_occurrence"] +def plan_playlist_reconcile( + current_ids: List[str], + desired_ids: List[str], +) -> dict: + """Plan an in-place reconcile of a server playlist toward a desired tracklist. + + Used by ``sync_mode='reconcile'`` (#792): instead of deleting + recreating + the playlist (which destroys its custom image, description, and identity), + the caller keeps the existing playlist object and applies only the delta — + adding the tracks that are missing and removing the ones no longer in the + source. Pure, no I/O. + + Returns ``{'add': [...], 'remove': [...]}`` (both lists of string ids): + - ``add`` — desired ids not currently present, in desired order. + - ``remove`` — current ids no longer desired (each occurrence kept once; + duplicates of a still-desired id are left for the caller's + dedupe to handle, never mass-removed). + + Order-preserving and duplicate-safe: a desired id already present is not + re-added; a current id that's still desired is not removed even if it + appears more than once. + """ + desired = [str(t) for t in desired_ids] + current = [str(t) for t in current_ids] + current_set = set(current) + desired_set = set(desired) + add = [d for d in desired if d not in current_set] + # Preserve order of removal as it appears in the current list; one entry per + # id (the caller maps ids back to concrete playlist entries to delete). + seen_remove = set() + remove = [] + for c in current: + if c not in desired_set and c not in seen_remove: + seen_remove.add(c) + remove.append(c) + return {"add": add, "remove": remove} + + +VALID_SYNC_MODES = ("replace", "append", "reconcile") + + +def normalize_sync_mode(requested, configured, default: str = "replace") -> str: + """Resolve the effective playlist sync mode. + + An explicit per-request value wins; otherwise the configured default + (Settings > Playlist sync mode); anything unrecognized falls back to + ``default``. Keeping ``reconcile`` in ``VALID_SYNC_MODES`` is load-bearing — + a validation list that omits it silently downgrades reconcile to replace, + which is exactly the #792 regression this helper exists to prevent. + """ + mode = (requested or "") or (configured or "") or default + return mode if mode in VALID_SYNC_MODES else default + + +__all__ = [ + "plan_playlist_add", + "remove_one_occurrence", + "plan_playlist_reconcile", + "normalize_sync_mode", + "VALID_SYNC_MODES", +] diff --git a/services/sync_service.py b/services/sync_service.py index 072185d4..803f2e8e 100644 --- a/services/sync_service.py +++ b/services/sync_service.py @@ -195,6 +195,26 @@ class PlaylistSyncService: failed_tracks=failed_tracks )) + def _reconcile_or_replace(self, client, playlist_name: str, tracks) -> bool: + """Reconcile mode (#792): edit the playlist in place (add/remove delta, + preserving its image + description + identity). If the client lacks + reconcile or it fails, fall back to the destructive replace so the sync + still succeeds — logged loudly so a failing in-place edit is diagnosable. + """ + fn = getattr(client, 'reconcile_playlist', None) + if fn is None: + return client.update_playlist(playlist_name, tracks) + try: + if fn(playlist_name, tracks): + return True + logger.warning( + "Reconcile sync failed for '%s' — falling back to replace " + "(playlist will be recreated this once)", playlist_name) + except Exception as e: + logger.warning( + "Reconcile sync errored for '%s' (%s) — falling back to replace", playlist_name, e) + return client.update_playlist(playlist_name, tracks) + async def sync_playlist(self, playlist: SpotifyPlaylist, download_missing: bool = False, profile_id: int = None, sync_mode: str = 'replace') -> SyncResult: self._active_profile_id = profile_id # Check if THIS specific playlist is already syncing @@ -378,6 +398,8 @@ class PlaylistSyncService: ) if sync_mode == 'append': sync_success = media_client.append_to_playlist(playlist.name, valid_tracks) + elif sync_mode == 'reconcile': + sync_success = self._reconcile_or_replace(media_client, playlist.name, valid_tracks) else: sync_success = media_client.update_playlist(playlist.name, valid_tracks) diff --git a/tests/sync/test_reconcile_or_replace.py b/tests/sync/test_reconcile_or_replace.py new file mode 100644 index 00000000..2c884997 --- /dev/null +++ b/tests/sync/test_reconcile_or_replace.py @@ -0,0 +1,67 @@ +"""sync_mode='reconcile' dispatch + fallback (#792). + +_reconcile_or_replace tries the client's in-place reconcile and falls back to +the destructive replace only when reconcile is unavailable or fails, so a sync +always succeeds while preferring the non-destructive path. +""" + +import sys +import types + +# Stub optional Spotify dep so services.sync_service imports in the test env. +if 'spotipy' not in sys.modules: + sp = types.ModuleType('spotipy'); oa = types.ModuleType('spotipy.oauth2') + sp.Spotify = type('S', (), {}); oa.SpotifyOAuth = oa.SpotifyClientCredentials = type('O', (), {}) + sp.oauth2 = oa; sys.modules['spotipy'] = sp; sys.modules['spotipy.oauth2'] = oa + +from services.sync_service import PlaylistSyncService + + +def _service(): + return PlaylistSyncService.__new__(PlaylistSyncService) + + +class _Client: + def __init__(self, reconcile=None): + self._reconcile = reconcile + self.reconcile_calls = [] + self.replace_calls = [] + if reconcile is not None: + def reconcile_playlist(name, tracks): + self.reconcile_calls.append(name) + if isinstance(reconcile, Exception): + raise reconcile + return reconcile + self.reconcile_playlist = reconcile_playlist + + def update_playlist(self, name, tracks): + self.replace_calls.append(name) + return True + + +def test_reconcile_success_does_not_fall_back(): + c = _Client(reconcile=True) + assert _service()._reconcile_or_replace(c, 'P', []) is True + assert c.reconcile_calls == ['P'] + assert c.replace_calls == [] # never recreated + + +def test_reconcile_false_falls_back_to_replace(): + c = _Client(reconcile=False) + assert _service()._reconcile_or_replace(c, 'P', []) is True + assert c.reconcile_calls == ['P'] + assert c.replace_calls == ['P'] # fell back so the sync still happens + + +def test_reconcile_exception_falls_back_to_replace(): + c = _Client(reconcile=RuntimeError('boom')) + assert _service()._reconcile_or_replace(c, 'P', []) is True + assert c.reconcile_calls == ['P'] + assert c.replace_calls == ['P'] + + +def test_client_without_reconcile_uses_replace(): + c = _Client(reconcile=None) # no reconcile_playlist attribute + assert not hasattr(c, 'reconcile_playlist') + assert _service()._reconcile_or_replace(c, 'P', []) is True + assert c.replace_calls == ['P'] diff --git a/tests/test_playlist_edit.py b/tests/test_playlist_edit.py index 13a2f041..8cc0e10d 100644 --- a/tests/test_playlist_edit.py +++ b/tests/test_playlist_edit.py @@ -2,7 +2,12 @@ from __future__ import annotations -from core.sync.playlist_edit import plan_playlist_add, remove_one_occurrence +from core.sync.playlist_edit import ( + normalize_sync_mode, + plan_playlist_add, + plan_playlist_reconcile, + remove_one_occurrence, +) # ── plan_playlist_add: link must not duplicate ──────────────────────────── @@ -78,3 +83,72 @@ def test_remove_single_occurrence(): def test_remove_stringifies(): new_ids, removed = remove_one_occurrence([1, 2, 2, 3], 2) assert removed and new_ids == ["1", "2", "3"] + + +# ── plan_playlist_reconcile: in-place delta (#792) ──────────────────────── + +def test_reconcile_adds_new_keeps_existing(): + # 4-track playlist + a 5th in source → add only the 5th, remove nothing. + plan = plan_playlist_reconcile(['a', 'b', 'c', 'd'], ['a', 'b', 'c', 'd', 'e']) + assert plan == {'add': ['e'], 'remove': []} + + +def test_reconcile_removes_gone(): + # Source dropped 'b' → remove it, add nothing. + plan = plan_playlist_reconcile(['a', 'b', 'c'], ['a', 'c']) + assert plan == {'add': [], 'remove': ['b']} + + +def test_reconcile_add_and_remove_together(): + plan = plan_playlist_reconcile(['a', 'b', 'c'], ['a', 'c', 'd', 'e']) + assert plan['add'] == ['d', 'e'] # desired order preserved + assert plan['remove'] == ['b'] + + +def test_reconcile_noop_when_identical(): + assert plan_playlist_reconcile(['a', 'b'], ['a', 'b']) == {'add': [], 'remove': []} + + +def test_reconcile_empty_desired_removes_all(): + assert plan_playlist_reconcile(['a', 'b'], []) == {'add': [], 'remove': ['a', 'b']} + + +def test_reconcile_empty_current_adds_all(): + assert plan_playlist_reconcile([], ['a', 'b']) == {'add': ['a', 'b'], 'remove': []} + + +def test_reconcile_stringifies_ids(): + plan = plan_playlist_reconcile([1, 2], [2, 3]) + assert plan == {'add': ['3'], 'remove': ['1']} + + +def test_reconcile_duplicate_still_desired_not_removed(): + # 'a' appears twice and is still desired → never queued for removal; + # a gone id is listed once even if it had duplicates. + plan = plan_playlist_reconcile(['a', 'a', 'b', 'b'], ['a']) + assert plan == {'add': [], 'remove': ['b']} + + +# ── normalize_sync_mode: reconcile must survive resolution (#792 regression) ── + +def test_normalize_keeps_reconcile_from_config(): + # The bug: a validation list omitting 'reconcile' downgraded the configured + # setting to 'replace'. No per-request override → configured value wins. + assert normalize_sync_mode(None, 'reconcile') == 'reconcile' + assert normalize_sync_mode('', 'reconcile') == 'reconcile' + + +def test_normalize_request_overrides_config(): + assert normalize_sync_mode('append', 'reconcile') == 'append' + assert normalize_sync_mode('reconcile', 'replace') == 'reconcile' + + +def test_normalize_falls_back_for_unknown(): + assert normalize_sync_mode('bogus', 'replace') == 'replace' + assert normalize_sync_mode(None, None) == 'replace' + assert normalize_sync_mode(None, 'also-bogus') == 'replace' + + +def test_normalize_all_real_modes_pass_through(): + for m in ('replace', 'append', 'reconcile'): + assert normalize_sync_mode(m, 'replace') == m diff --git a/web_server.py b/web_server.py index a65cccb2..cb869f23 100644 --- a/web_server.py +++ b/web_server.py @@ -21053,10 +21053,17 @@ def _get_source_playlist_states(states, error_label, info_log_label=None): def _submit_sync_task(sync_playlist_id, playlist_name, spotify_tracks, playlist_image_url): """Submit a sync to the shared executor (closes over sync_executor / _run_sync_task / get_current_profile_id so the lifted start_sync helper - stays free of those globals).""" + stays free of those globals). + + Used by ALL per-source discovery syncs (Spotify-Public/Tidal/Deezer/Qobuz/ + YouTube/iTunes-link/ListenBrainz/Beatport). These have no per-request mode + selector, so honor the configured default (Settings > Playlist sync mode) — + otherwise they always ran 'replace' regardless of the setting (#792).""" + from core.sync.playlist_edit import normalize_sync_mode + _mode = normalize_sync_mode(None, config_manager.get('playlist_sync.mode', 'replace')) return sync_executor.submit( _run_sync_task, sync_playlist_id, playlist_name, spotify_tracks, - None, get_current_profile_id(), playlist_image_url, + None, get_current_profile_id(), playlist_image_url, _mode, ) @@ -23546,9 +23553,11 @@ def start_playlist_sync(): # playlist — only adds tracks that aren't there yet. Per-server clients # implement append via native add APIs (Plex addItems, Jellyfin POST # /Playlists//Items, Navidrome updatePlaylist?songIdToAdd=...). - sync_mode = data.get('sync_mode', 'replace') - if sync_mode not in ('replace', 'append'): - sync_mode = 'replace' + # Per-request sync_mode wins; otherwise use the configured default + # (Settings > Playlist sync mode). Default 'replace' keeps today's behavior. + from core.sync.playlist_edit import normalize_sync_mode + sync_mode = normalize_sync_mode(data.get('sync_mode'), + config_manager.get('playlist_sync.mode', 'replace')) if not all([playlist_id, playlist_name, tracks_json]): return jsonify({"success": False, "error": "Missing playlist_id, name, or tracks."}), 400 diff --git a/webui/index.html b/webui/index.html index 2ed8e0b2..aa21bbf2 100644 --- a/webui/index.html +++ b/webui/index.html @@ -6112,6 +6112,18 @@

Playlist Sync Settings

+
+ + +
+ "Replace" deletes and recreates the server playlist every sync, which wipes its custom image/description. "Reconcile" updates the same playlist in place (adds new tracks, removes ones no longer in the source) so your custom image and description survive. "Append" only ever adds. +
+
+