From 4d7267e906c89ed8a626d3d2241ee1fcd8c23683 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Fri, 12 Jun 2026 17:08:27 -0700 Subject: [PATCH] Playlists: reconcile rebuilds touched playlists from the LIBRARY, not task fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reconcile read each completed task's final_file_path to find paths — but not every import path sets it (the verification worker marks the task completed without it), so tracks that imported via that path were silently dropped (user saw 3 of 5 symlinks). Root cause: leaning on a fragile per-task field. Now reconcile_batch_playlists identifies the organize playlists the batch touched (its own + any reached via a completed track's source_info provenance) and rebuilds each from CURRENT library ownership via _rebuild_one_from_db (check_track_exists over membership). It just asks the library what's owned, so it's robust to HOW a track imported (modal worker / slskd monitor / verification worker) and still prunes tracks that left. Takes a db handle; all three callers pass MusicDatabase(). Reconcile tests rewritten for the DB-rebuild form (organize batch, wishlist provenance, non-organize skip, plain no-op). 973 downloads/imports/playlist tests pass. --- core/downloads/lifecycle.py | 6 +- core/downloads/master.py | 3 +- core/playlists/materialize_service.py | 88 +++++++++++---------- tests/test_playlist_materialize_service.py | 90 +++++++++++----------- 4 files changed, 99 insertions(+), 88 deletions(-) diff --git a/core/downloads/lifecycle.py b/core/downloads/lifecycle.py index e8abeede..cd7e38fc 100644 --- a/core/downloads/lifecycle.py +++ b/core/downloads/lifecycle.py @@ -556,7 +556,8 @@ def on_download_completed(batch_id: str, task_id: str, success: bool, deps: Life # from the batch's own captured paths — non-fatal, derived view. try: from core.playlists.materialize_service import reconcile_batch_playlists - for _pl_name, _mat in reconcile_batch_playlists(batch, download_tasks, deps.config_manager): + from database.music_database import MusicDatabase + for _pl_name, _mat in reconcile_batch_playlists(MusicDatabase(), batch, download_tasks, deps.config_manager): logger.info( f"[Playlist Folder] Rebuilt '{_mat.playlist_dir}': " f"{_mat.linked} linked, {_mat.copied} copied, " @@ -758,7 +759,8 @@ def check_batch_completion_v2(batch_id: str, deps: LifecycleDeps) -> Optional[bo # get built for them. Path-independent, non-fatal, derived view. try: from core.playlists.materialize_service import reconcile_batch_playlists - for _pl_name, _mat in reconcile_batch_playlists(batch, download_tasks, deps.config_manager): + from database.music_database import MusicDatabase + for _pl_name, _mat in reconcile_batch_playlists(MusicDatabase(), batch, download_tasks, deps.config_manager): logger.info( f"[Playlist Folder] Rebuilt '{_mat.playlist_dir}': " f"{_mat.linked} linked, {_mat.copied} copied, " diff --git a/core/downloads/master.py b/core/downloads/master.py index b18b9b00..aac93bbf 100644 --- a/core/downloads/master.py +++ b/core/downloads/master.py @@ -792,9 +792,10 @@ def run_full_missing_tracks_process(batch_id, playlist_id, tracks_json, deps: Ma if effective_playlist_folder_mode: try: from core.playlists.materialize_service import reconcile_batch_playlists + from database.music_database import MusicDatabase as _MDB _batch = download_batches.get(batch_id) if _batch is not None: - for _pl_name, _mat in reconcile_batch_playlists(_batch, download_tasks, deps.config_manager): + for _pl_name, _mat in reconcile_batch_playlists(_MDB(), _batch, download_tasks, deps.config_manager): logger.info( f"[Playlist Folder] Rebuilt '{_mat.playlist_dir}' (all owned): " f"{_mat.linked} linked, {_mat.copied} copied, {_mat.removed_stale} stale removed" diff --git a/core/playlists/materialize_service.py b/core/playlists/materialize_service.py index bcc2c905..02219858 100644 --- a/core/playlists/materialize_service.py +++ b/core/playlists/materialize_service.py @@ -74,55 +74,63 @@ def materialize_playlist_from_batch(batch: dict, download_tasks: dict, config_ma return rebuild_playlist_folder(root, name, real_paths, mode) -def reconcile_batch_playlists(batch: dict, download_tasks: dict, config_manager): - """One post-batch step: drop the batch's newly-resolved tracks into the right - ``Playlists//`` folders. Path-independent — covers an organize-by-playlist - download AND a late wishlist arrival — using the per-track playlist provenance - each download already carries, plus the file paths the batch already captured - (so it needs no DB re-match and no waiting on the server scan/sync). +def reconcile_batch_playlists(db, batch: dict, download_tasks: dict, config_manager, *, profile_id: int = 1): + """One post-batch step: rebuild every organize-by-playlist playlist this batch + TOUCHED, from CURRENT library ownership. - - The batch's OWN organize playlist is fully rebuilt from the batch payload - (owned + downloaded), pruning tracks no longer present. - - A track that completed for a DIFFERENT playlist (e.g. a wishlist - fulfillment) is ADDED to that playlist's folder — no prune, since this - batch isn't that playlist's full membership (removals are handled by a - full rebuild on the next sync / the manual button). + Touched playlists = the batch's own organize playlist + any playlist a completed + track belongs to (via the per-track ``source_info`` provenance — covers a + wishlist batch fulfilling a track that belongs to an organize playlist). Each is + rebuilt via ``_rebuild_one_from_db`` (``check_track_exists`` over its membership), + so it's robust to HOW a track imported — modal worker, slskd monitor, or the + verification worker, which don't all set the same task fields. It simply asks the + library what's owned, and prunes tracks that have left the playlist. - Returns a list of ``(playlist_name, RebuildSummary)``. Callers wrap non-fatally.""" - from core.library.path_resolver import resolve_library_file_path + Returns ``[(playlist_name, RebuildSummary)]``. Callers wrap non-fatally.""" + import json as _json - results = [] - root = docker_resolve_path(config_manager.get("playlists.materialize_path", "./Playlists")) - mode = normalize_mode(config_manager.get("playlists.materialize_mode", "symlink")) - batch_name = batch.get("playlist_name") if batch.get("playlist_folder_mode") else None + # Collect the (ref, source) of every organize playlist this batch touched. + wanted, seen_ref = [], set() - # 1. The batch's own organize playlist → full materialize (prunes removed). - if batch_name: - summary = materialize_playlist_from_batch(batch, download_tasks, config_manager) - if summary: - results.append((batch_name, summary)) + def _want(ref, source): + ref = str(ref or "").strip() + if not ref: + return + key = (ref, source or "spotify") + if key not in seen_ref: + seen_ref.add(key) + wanted.append(key) + + if batch.get("playlist_folder_mode"): + _want(batch.get("source_playlist_ref") or batch.get("playlist_id"), + batch.get("batch_source") or "spotify") - # 2. Tracks that completed for OTHER playlists (e.g. wishlist fulfilling a - # track that belongs to an organize playlist) → add-only into that folder. - extra: dict = {} for tid in (batch.get("queue") or []): task = (download_tasks or {}).get(tid) or {} - if task.get("status") != "completed" or not task.get("final_file_path"): + if task.get("status") != "completed": continue - pname = (task.get("track_info") or {}).get("_playlist_name") - if pname and pname != batch_name: - extra.setdefault(pname, []).append(task["final_file_path"]) - - for name, stored in extra.items(): - real, seen = [], set() - for p in stored: - r = resolve_library_file_path(str(p), config_manager=config_manager) - if r and r not in seen: - seen.add(r) - real.append(r) - if real: - results.append((name, rebuild_playlist_folder(root, name, real, mode, prune_stale=False))) + si = (task.get("track_info") or {}).get("source_info") or {} + if isinstance(si, str): + try: + si = _json.loads(si) + except Exception: + si = {} + if isinstance(si, dict) and si.get("playlist_id"): + _want(si["playlist_id"], si.get("source") or "spotify") + results, seen_id = [], set() + for ref, source in wanted: + try: + pl = db.resolve_mirrored_playlist(ref, profile_id, default_source=source) + except Exception: + pl = None + if not pl or not pl.get("organize_by_playlist") or pl.get("id") in seen_id: + continue + seen_id.add(pl.get("id")) + try: + results.append(_rebuild_one_from_db(db, config_manager, pl)) + except Exception: + continue return results diff --git a/tests/test_playlist_materialize_service.py b/tests/test_playlist_materialize_service.py index b770ca15..4280d71b 100644 --- a/tests/test_playlist_materialize_service.py +++ b/tests/test_playlist_materialize_service.py @@ -30,8 +30,8 @@ class _RebuildDB: def get_mirrored_playlists(self, profile_id=1): return [ - {"id": 1, "name": "Mix", "organize_by_playlist": True}, - {"id": 2, "name": "Off", "organize_by_playlist": False}, + {"id": 1, "name": "Mix", "source_playlist_id": "PL1", "organize_by_playlist": True}, + {"id": 2, "name": "Off", "source_playlist_id": "PL2", "organize_by_playlist": False}, ] def get_mirrored_playlist_tracks(self, pid): @@ -46,6 +46,12 @@ class _RebuildDB: return pl return None + def resolve_mirrored_playlist(self, ref, profile_id=1, *, default_source="spotify"): + for pl in self.get_mirrored_playlists(): + if str(ref) in (pl["source_playlist_id"], str(pl["id"]), pl["name"]): + return pl + return None + def check_track_exists(self, title, artist, confidence_threshold=0.8, server_source=None, album=None, candidate_tracks=None): fp = self.owned.get(title) @@ -142,59 +148,53 @@ def test_materialize_skipped_when_not_organize(tmp_path: Path): assert not (tmp_path / "Playlists").exists() -def test_reconcile_organize_batch_full_rebuild(tmp_path: Path): - """An organize-by-playlist batch → its own folder is fully (re)built from the - batch payload.""" - owned, fresh = _mk(tmp_path, "Owned.mp3"), _mk(tmp_path, "Fresh.mp3") - batch = { - "playlist_folder_mode": True, - "playlist_name": "Mix", - "analysis_results": [{"found": True, "matched_file_path": owned[0]}], - "queue": ["t1"], - } - tasks = {"t1": {"status": "completed", "final_file_path": fresh[0], - "track_info": {"_playlist_name": "Mix"}}} # same playlist as batch +def test_reconcile_organize_batch_rebuilds_from_library(tmp_path: Path): + """An organize batch → its playlist is rebuilt from the LIBRARY (membership × + check_track_exists), not from fragile per-task fields. Owned members link, + non-owned drop out.""" + a = _mk(tmp_path, "A.mp3")[0] # Mix membership = A, Gone; only A owned + db = _RebuildDB({"A": a}) + batch = {"playlist_folder_mode": True, "playlist_name": "Mix", + "source_playlist_ref": "PL1", "batch_source": "spotify", "queue": []} cfg = _Cfg(str(tmp_path / "Playlists")) - results = reconcile_batch_playlists(batch, tasks, cfg) - assert len(results) == 1 # only Mix (no double-add) + results = reconcile_batch_playlists(db, batch, {}, cfg) + assert len(results) == 1 name, s = results[0] - assert name == "Mix" and s.linked == 2 - pdir = Path(s.playlist_dir) - assert (pdir / "Owned.mp3").exists() and (pdir / "Fresh.mp3").exists() + assert name == "Mix" and s.linked == 1 # A owned; Gone not in library → skipped + assert (tmp_path / "Playlists" / "Mix" / "A.mp3").exists() -def test_reconcile_wishlist_adds_to_other_playlist_without_pruning(tmp_path: Path): - """The wishlist gap: a wishlist batch (not organize) downloads a track that - belongs to an organize playlist → it's ADDED to that playlist's folder, and - the folder's existing entries are NOT pruned.""" - # Pre-seed Smack That with an existing symlink (a track from a prior batch). - existing = _mk(tmp_path, "Existing.mp3")[0] - from core.playlists.materialize import rebuild_playlist_folder - rebuild_playlist_folder(str(tmp_path / "Playlists"), "Smack That", [existing], "symlink") - - # A WISHLIST batch (playlist_folder_mode False) completes a track tagged for - # the "Smack That" organize playlist. - late = _mk(tmp_path, "Late.mp3")[0] - batch = {"playlist_folder_mode": False, "playlist_name": "wishlist", - "analysis_results": [], "queue": ["w1"]} - tasks = {"w1": {"status": "completed", "final_file_path": late, - "track_info": {"_playlist_name": "Smack That"}}} +def test_reconcile_wishlist_track_rebuilds_its_playlist(tmp_path: Path): + """The wishlist gap: a wishlist batch (not organize) completes a track whose + provenance points to an organize playlist → that playlist gets rebuilt from the + library, regardless of which import path set which task field.""" + a = _mk(tmp_path, "A.mp3")[0] + db = _RebuildDB({"A": a}) + batch = {"playlist_folder_mode": False, "playlist_name": "wishlist", "queue": ["w1"]} + tasks = {"w1": {"status": "completed", + "track_info": {"source_info": {"playlist_id": "PL1", "source": "spotify"}}}} cfg = _Cfg(str(tmp_path / "Playlists")) + results = reconcile_batch_playlists(db, batch, tasks, cfg) + assert len(results) == 1 and results[0][0] == "Mix" + assert (tmp_path / "Playlists" / "Mix" / "A.mp3").exists() # Mix rebuilt from the library - results = reconcile_batch_playlists(batch, tasks, cfg) - assert len(results) == 1 and results[0][0] == "Smack That" - pdir = tmp_path / "Playlists" / "Smack That" - assert (pdir / "Late.mp3").exists() # the wishlist track was ADDED - assert (pdir / "Existing.mp3").exists() # and the prior entry was NOT pruned + +def test_reconcile_skips_non_organized_provenance(tmp_path: Path): + """A completed track pointing to a NON-organize playlist (Off / PL2) is ignored.""" + db = _RebuildDB({"B": _mk(tmp_path, "B.mp3")[0]}) + batch = {"playlist_folder_mode": False, "playlist_name": "wishlist", "queue": ["w1"]} + tasks = {"w1": {"status": "completed", + "track_info": {"source_info": {"playlist_id": "PL2", "source": "spotify"}}}} + assert reconcile_batch_playlists(db, batch, tasks, _Cfg(str(tmp_path / "Playlists"))) == [] + assert not (tmp_path / "Playlists" / "Off").exists() def test_reconcile_noop_for_plain_batch(tmp_path: Path): """A normal (non-organize, no provenance) batch → nothing happens.""" - f = _mk(tmp_path, "X.mp3")[0] - batch = {"playlist_folder_mode": False, "playlist_name": "album", - "analysis_results": [], "queue": ["a1"]} - tasks = {"a1": {"status": "completed", "final_file_path": f, "track_info": {}}} - assert reconcile_batch_playlists(batch, tasks, _Cfg(str(tmp_path / "Playlists"))) == [] + db = _RebuildDB({}) + batch = {"playlist_folder_mode": False, "playlist_name": "album", "queue": ["a1"]} + tasks = {"a1": {"status": "completed", "track_info": {}}} # no source_info + assert reconcile_batch_playlists(db, batch, tasks, _Cfg(str(tmp_path / "Playlists"))) == [] assert not (tmp_path / "Playlists").exists()