Playlists: reconcile rebuilds touched playlists from the LIBRARY, not task fields
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.
This commit is contained in:
BoulderBadgeDad
parent
7fb1b115f0
commit
4d7267e906
4 changed files with 99 additions and 88 deletions
|
|
@ -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, "
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
|
|
|
|||
|
|
@ -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/<name>/`` 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
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue