diff --git a/app/api/jobs.py b/app/api/jobs.py index f2be8f41..e4c51310 100644 --- a/app/api/jobs.py +++ b/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: @@ -759,7 +775,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"} diff --git a/app/core/registry.py b/app/core/registry.py index 84467c40..eaffc421 100644 --- a/app/core/registry.py +++ b/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: diff --git a/app/main.py b/app/main.py index 99dc0ff6..75e6a5cf 100644 --- a/app/main.py +++ b/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"]) diff --git a/static/js/catalog.js b/static/js/catalog.js index c1257b3a..b3476c35 100644 --- a/static/js/catalog.js +++ b/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; } }); } diff --git a/static/js/i18n.js b/static/js/i18n.js index 0ec49ee8..4b55b361 100644 --- a/static/js/i18n.js +++ b/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…", diff --git a/tests/conftest.py b/tests/conftest.py index d6fa7048..9e489612 100644 --- a/tests/conftest.py +++ b/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) diff --git a/tests/test_deleted_jobs_stay_deleted.py b/tests/test_deleted_jobs_stay_deleted.py new file mode 100644 index 00000000..ce77aecf --- /dev/null +++ b/tests/test_deleted_jobs_stay_deleted.py @@ -0,0 +1,132 @@ +"""A deleted song must not come back (#521). + +restore() adopts any job-shaped directory it finds, which is the right +behaviour for a library whose registry was lost and the wrong behaviour for a +job the user deleted whose files outlived the delete. Reported on macOS via +Settings -> "Reset app data", where the frontend also wipes its own tombstone +on the strength of an unconditional {"ok": true}. +""" + +from __future__ import annotations + +import json + +import pytest + +from app.core import registry as _registry +from app.core.registry import registry_path + + +@pytest.fixture(autouse=True) +def _clean_registry(): + _registry._jobs.clear() + _registry._deleted.clear() + yield + _registry._jobs.clear() + _registry._deleted.clear() + + +def _simulate_restart(): + """Drop in-memory state the way a process restart does; restore() then + reads everything back from disk.""" + _registry._jobs.clear() + _registry._deleted.clear() + + +@pytest.fixture +def jobs_dir(tmp_path): + """A jobs root of our own. conftest puts _jobs_root next to tmp_path, and + reset_all iterates whatever it is handed.""" + d = tmp_path / "library" + d.mkdir() + return d + + +def _done_job_dir(jobs_dir, job_id="a1b2c3d4e5f6"): + """A directory shaped like a finished job, which is what restore() adopts.""" + stems = jobs_dir / job_id / "stems" + stems.mkdir(parents=True) + (stems / "vocals.wav").write_bytes(b"RIFF") + return jobs_dir / job_id + + +def test_an_orphan_directory_is_adopted_when_it_was_not_deleted(jobs_dir): + # The behaviour we must not break: a library whose registry was lost is + # rebuilt from the directories on disk. + _done_job_dir(jobs_dir) + + _registry.restore(jobs_dir) + + assert "a1b2c3d4e5f6" in _registry.all_jobs() + + +def test_a_deleted_job_is_not_re_adopted(jobs_dir): + # The bug: files outlive the delete, and the next start brings the song back. + _done_job_dir(jobs_dir) + _registry.mark_deleted("a1b2c3d4e5f6") + _registry.persist(jobs_dir) + + _simulate_restart() + _registry.restore(jobs_dir) + + assert _registry.all_jobs() == {}, "a deleted song came back from disk" + + +def test_the_deletion_record_survives_a_restart(jobs_dir): + _done_job_dir(jobs_dir) + _registry.mark_deleted("a1b2c3d4e5f6") + _registry.persist(jobs_dir) + + assert "a1b2c3d4e5f6" in json.loads(registry_path(jobs_dir).read_text())["deleted"] + + +def test_the_record_is_forgotten_once_the_directory_is_gone(jobs_dir): + # Otherwise the set grows for the life of the install. A record only has to + # outlive the directory it refers to. + job_dir = _done_job_dir(jobs_dir) + _registry.mark_deleted("a1b2c3d4e5f6") + _registry.persist(jobs_dir) + + import shutil + + shutil.rmtree(job_dir) + _registry.persist(jobs_dir) + + assert json.loads(registry_path(jobs_dir).read_text())["deleted"] == [] + + +def test_reset_reports_what_it_could_not_remove(jobs_dir, monkeypatch): + _done_job_dir(jobs_dir) + + def _boom(path, *a, **kw): + raise OSError("Directory not empty") + + monkeypatch.setattr("shutil.rmtree", _boom) + + undeleted = _registry.reset_all(jobs_dir) + + assert undeleted == ["a1b2c3d4e5f6"], "reset must not claim success it did not have" + + +def test_reset_records_survivors_so_they_cannot_come_back(jobs_dir, monkeypatch): + # This is the reported path: reset "succeeds", the frontend wipes its + # tombstone, and the surviving directory is adopted on the next start. + _done_job_dir(jobs_dir) + + def _boom(path, *a, **kw): + raise OSError("Directory not empty") + + monkeypatch.setattr("shutil.rmtree", _boom) + _registry.reset_all(jobs_dir) + monkeypatch.undo() + + _simulate_restart() + _registry.restore(jobs_dir) + + assert _registry.all_jobs() == {}, "a reset that half-worked refilled the library" + + +def test_a_clean_reset_reports_nothing_undeleted(jobs_dir): + _done_job_dir(jobs_dir) + + assert _registry.reset_all(jobs_dir) == [] diff --git a/tests/test_reset.py b/tests/test_reset.py index 0d80cc9f..0403dcf8 100644 --- a/tests/test_reset.py +++ b/tests/test_reset.py @@ -74,7 +74,8 @@ def test_reset_endpoint_works_without_desktop_mode(client, monkeypatch, tmp_path r = client.post("/api/reset") assert r.status_code == 200 - assert r.json() == {"ok": True} + # "undeleted" reports what reset could not remove; 0 on a clean reset (#521). + assert r.json() == {"ok": True, "undeleted": 0} assert _jobs == {} assert not (tmp_path / job.id).exists() @@ -99,7 +100,8 @@ def test_reset_endpoint_succeeds(client, monkeypatch, tmp_path): r = client.post("/api/reset") assert r.status_code == 200 - assert r.json() == {"ok": True} + # "undeleted" reports what reset could not remove; 0 on a clean reset (#521). + assert r.json() == {"ok": True, "undeleted": 0} assert _jobs == {} assert not (tmp_path / job.id).exists()