fix(library): stop deleted songs coming back (recovers #527)

app/api/jobs.py

@@ -29,6 +29,7 @@
 from app.core.registry import all_jobs as registry_all_jobs
 from app.core.registry import get as registry_get
 from app.core.registry import get_proc as registry_get_proc
+from app.core.registry import mark_deleted as registry_mark_deleted
 from app.core.registry import pending_count as registry_pending_count
 from app.core.registry import persist as registry_persist
 from app.core.registry import register_if_capacity as registry_register_if_capacity
@@ -110,14 +111,29 @@ def _copy_to_dest(src_file: object, dest: Path) -> None:
         shutil.copyfileobj(src_file, out)  # type: ignore[arg-type]
 
 
-def _rmtree_job(job_id: str) -> None:
+def _rmtree_job(job_id: str) -> bool:
+    """Remove a job's directory. False means files are still on disk.
+
+    The outcome used to be swallowed, so delete_job dropped the registry entry
+    whether or not anything was actually deleted -- and restore() then adopted
+    the surviving directory on the next start, which is how deleted songs came
+    back (#521).
+
+    Retried once: on macOS the common failure is Finder or Spotlight creating
+    a .DS_Store between rmtree's scan and its final rmdir, which leaves
+    "Directory not empty" on a directory that is about to be empty again."""
     job_dir = JOBS_DIR / job_id
-    if not job_dir.is_dir():
-        return
-    try:
-        shutil.rmtree(job_dir)
-    except Exception:
-        logger.warning("failed to remove job dir %s", job_dir, exc_info=True)
+    for attempt in (1, 2):
+        if not job_dir.is_dir():
+            return True
+        try:
+            shutil.rmtree(job_dir)
+            return True
+        except Exception:
+            logger.warning(
+                "failed to remove job dir %s (attempt %d)", job_dir, attempt, exc_info=True
+            )
+    return not job_dir.is_dir()
 
 
 def _job_files_missing(job: Job) -> bool:
@@ -769,7 +785,16 @@ def delete_job(job_id: str) -> dict[str, str]:
         raise HTTPException(status_code=404, detail="job not found")
     if job.status not in ("done", "error", "cancelled"):
         raise HTTPException(status_code=409, detail="job is still running")
-    _rmtree_job(job_id)
+    removed = _rmtree_job(job_id)
+    # Recorded whether or not the files went away. The user asked for this job
+    # to be gone; without the record, a directory that outlived the delete is
+    # re-adopted by restore() on the next start and the track reappears.
+    registry_mark_deleted(job_id)
     registry_remove(job_id)
     registry_persist(JOBS_DIR)
+    if not removed:
+        raise HTTPException(
+            status_code=500,
+            detail="Removed from the library, but its files could not be deleted.",
+        )
     return {"job_id": job_id, "status": "deleted"}

app/core/registry.py

@@ -38,6 +38,14 @@
 # Ids restore() wants re-queued, drained once by the app lifespan.
 _pending_resume: list[str] = []
 
+# Job ids the user deleted. Orphan recovery in restore() adopts any job-shaped
+# directory it finds, which is what resurrected tracks whose files failed to
+# delete (#521). A directory that outlived its delete must not come back, and
+# the client-side tombstone cannot be relied on for that -- "Reset app data"
+# wipes it. Persisted with the registry; see _prune_deleted for why it stays
+# small.
+_deleted: set[str] = set()
+
 
 def register(job: Job) -> Job:
     with _lock:
@@ -105,6 +113,24 @@ def registry_path(jobs_dir: Path) -> Path:
     return jobs_dir / _REGISTRY_FILE
 
 
+def mark_deleted(job_id: str) -> None:
+    """Record that the user deleted this job, so restore() will not re-adopt
+    its directory if the files outlived the delete."""
+    with _lock:
+        _deleted.add(job_id)
+
+
+def _prune_deleted(jobs_dir: Path) -> None:
+    """Forget deletion records whose directory is finally gone.
+
+    A record only has to outlive the directory it refers to, so this keeps the
+    set naturally bounded instead of growing for the life of the install.
+    Caller must not hold _lock."""
+    with _lock:
+        stale = {job_id for job_id in _deleted if not (jobs_dir / job_id).exists()}
+        _deleted.difference_update(stale)
+
+
 def persist(jobs_dir: Path) -> None:
     """Persist terminal jobs so completed library entries survive restarts.
 
@@ -120,13 +146,24 @@ def persist(jobs_dir: Path) -> None:
         logger.warning("cannot create jobs dir %s; skipping persist", jobs_dir, exc_info=True)
         return
     path = registry_path(jobs_dir)
+    _prune_deleted(jobs_dir)
     with _lock:
         records = [
             job.to_record()
             for job in sorted(_jobs.values(), key=lambda item: item.created_at)
             if job.status in _PERSISTED
         ]
-        payload = json.dumps({"version": REGISTRY_VERSION, "jobs": records}, indent=2) + "\n"
+        payload = (
+            json.dumps(
+                {
+                    "version": REGISTRY_VERSION,
+                    "jobs": records,
+                    "deleted": sorted(_deleted),
+                },
+                indent=2,
+            )
+            + "\n"
+        )
         tmp = jobs_dir / f".registry.{uuid.uuid4().hex}.tmp"
         try:
             tmp.write_text(payload, encoding="utf-8")
@@ -145,6 +182,10 @@ def restore(jobs_dir: Path) -> None:
     if path.is_file():
         try:
             data = _migrate(json.loads(path.read_text(encoding="utf-8")))
+            recorded = data.get("deleted")
+            if isinstance(recorded, list):
+                with _lock:
+                    _deleted.update(str(job_id) for job_id in recorded)
             to_add = {}
             resume: list[Job] = []
             for record in data.get("jobs", []):
@@ -190,9 +231,14 @@ def restore(jobs_dir: Path) -> None:
     try:
         with _lock:
             known = set(_jobs)
+            deleted = set(_deleted)
         for job_dir in jobs_dir.iterdir():
             if not job_dir.is_dir() or not JOB_ID_RE.match(job_dir.name) or job_dir.name in known:
                 continue
+            if job_dir.name in deleted:
+                # The user deleted this and its files outlived the delete.
+                # Adopting it here is what brought songs back (#521).
+                continue
             recovered = _recover_done_job(job_dir)
             if recovered is not None:
                 with _lock:
@@ -318,7 +364,7 @@ def set_proc(job_id: str, proc: subprocess.Popen | None) -> None:
             _procs[job_id] = proc
 
 
-def reset_all(jobs_dir: Path) -> None:
+def reset_all(jobs_dir: Path) -> list[str]:
     """Delete every job directory and the registry file, clearing the
     in-memory registry too. Desktop-only factory reset (Settings -> General
     -> "Reset app data") -- the caller is responsible for checking no job is
@@ -328,7 +374,8 @@ def reset_all(jobs_dir: Path) -> None:
         _jobs.clear()
         _procs.clear()
     if not jobs_dir.is_dir():
-        return
+        return []
+    failed: list[str] = []
     for entry in jobs_dir.iterdir():
         try:
             if entry.is_dir():
@@ -337,6 +384,17 @@ def reset_all(jobs_dir: Path) -> None:
                 entry.unlink()
         except OSError:
             logger.warning("reset: could not remove %s", entry, exc_info=True)
+            failed.append(entry.name)
+    # Anything that survived is still a job-shaped directory on disk, so the
+    # next start would adopt it and the "reset" library would refill itself.
+    # Record it as deleted and persist that, which also recreates the registry
+    # file this loop just removed.
+    surviving = [name for name in failed if JOB_ID_RE.match(name)]
+    if surviving:
+        with _lock:
+            _deleted.update(surviving)
+        persist(jobs_dir)
+    return failed
 
 
 def get_proc(job_id: str) -> subprocess.Popen | None:

app/main.py

@@ -590,8 +590,15 @@ def reset_app_data() -> dict[str, object]:
     from app.pipeline import jobqueue
 
     jobqueue.clear()
-    reset_registry(JOBS_DIR)
-    return {"ok": True}
+    # Anything that could not be removed is reported rather than swallowed: the
+    # frontend used to take an unconditional {"ok": true} as licence to wipe its
+    # own deletion tombstone, and any surviving directory was then re-adopted on
+    # the next start with nothing left to suppress it (#521). reset_all also
+    # records the survivors server-side, so they stay deleted regardless.
+    undeleted = reset_registry(JOBS_DIR)
+    if undeleted:
+        _log.warning("reset left %d entries on disk: %s", len(undeleted), undeleted)
+    return {"ok": True, "undeleted": len(undeleted)}
 
 
 @app.get("/api/registry", tags=["settings"])

static/js/catalog.js

@@ -130,11 +130,21 @@ function getDeletedJobIds() {
   return _deletedJobIds;
 }
 
-function markJobsDeleted(ids) {
+async function markJobsDeleted(ids) {
   for (const id of ids) _deletedJobIds.add(id);
-  storeSet(DELETED_JOBS_KEY, [..._deletedJobIds]).catch((e) =>
-    console.warn("[catalog] failed to persist deleted jobs", e)
-  );
+  // Awaited by callers before they purge. Fire-and-forget meant that quitting
+  // soon after clearing the bin -- or one failed store write -- left no
+  // tombstone, and syncWithServer re-imported everything on the next launch
+  // (#521). The backend keeps its own deletion record now, so this is a second
+  // line of defence rather than the only one, but it still has to be written
+  // before the tracks are dropped from the local index.
+  try {
+    await storeSet(DELETED_JOBS_KEY, [..._deletedJobIds]);
+    return true;
+  } catch (e) {
+    console.warn("[catalog] failed to persist deleted jobs", e);
+    return false;
+  }
 }
 
 function normalizeFolderColor(color) {
@@ -1958,15 +1968,43 @@ function wireCatalogRailViews() {
   document.querySelector(".rail-favorites")?.addEventListener("click", () => setCatalogView("favorites"));
   document.querySelector(".rail-trash")?.addEventListener("click", () => setCatalogView("trash"));
   document.querySelector(".rail-queue")?.addEventListener("click", () => setCatalogView("queue"));
-  document.getElementById("clearBinBtn")?.addEventListener("click", () => {
-    const trash = getTrashFolder();
-    const toDelete = [...(trash?.items || [])];
-    markJobsDeleted(toDelete); // persist before purge so reload can't re-import
-    purgeTrash();
-    saveState();
-    render();
-    for (const id of toDelete) {
-      fetch(`/api/jobs/${id}`, { method: "DELETE" }).catch(() => {});
+  document.getElementById("clearBinBtn")?.addEventListener("click", async (e) => {
+    const btn = e.currentTarget;
+    if (btn.disabled) return;
+    btn.disabled = true;
+    try {
+      const trash = getTrashFolder();
+      const toDelete = [...(trash?.items || [])];
+      // Awaited: the tombstone has to be on disk before the tracks leave the
+      // local index, or a reload re-imports them (#521).
+      await markJobsDeleted(toDelete);
+      purgeTrash();
+      saveState();
+      render();
+
+      // Awaited too. These used to be fire-and-forget with .catch(() => {}),
+      // so a delete that failed -- a 409 on a job stuck in "queued", a 500
+      // when the files could not be removed -- was invisible, and the track
+      // came back the next time the registry was read from disk.
+      const failed = [];
+      for (const id of toDelete) {
+        try {
+          const res = await fetch(`/api/jobs/${id}`, { method: "DELETE" });
+          if (!res.ok && res.status !== 404) failed.push(id);
+        } catch (err) {
+          console.warn("[catalog] delete failed for", id, err);
+          failed.push(id);
+        }
+      }
+      if (failed.length) {
+        console.warn("[catalog] %d track(s) could not be deleted on the server", failed.length);
+        notifyFailure({
+          kind: "delete",
+          message: i18nT("library.deleteFailed", { count: failed.length }),
+        });
+      }
+    } finally {
+      btn.disabled = false;
     }
   });
 }

static/js/i18n.js

@@ -599,6 +599,7 @@ const en = {
   "settings.exportLogs.error": "Could not export the logs.",
   "status.unavailable": "unavailable",
 
+  "library.deleteFailed": "{count} track(s) could not be deleted. They stay out of your library, but their files may still be on disk.",
   "settings.resetData.title": "Reset app data",
   "settings.resetData.desc": "Permanently deletes every track, job, and library entry. On a shared server this affects everyone who uses it. Cannot be undone.",
   "settings.resetData.button": "Reset app data…",

tests/conftest.py

@@ -43,11 +43,15 @@ def _isolate_job_queue():
     # the app lifespan drains it into the queue, which would otherwise leak a
     # real job into a test's queue.
     _registry._pending_resume.clear()
+    # Deletion records are module-global too, so one test's deleted id would
+    # suppress another test's orphan recovery (#521).
+    _registry._deleted.clear()
     yield
     _jobqueue._queue.clear()
     _jobqueue._running_id = None
     _jobqueue._paused = False
     _registry._pending_resume.clear()
+    _registry._deleted.clear()
 
 
 @pytest.fixture(autouse=True)