From 7d7fd4186749426b523cae255d7246693753a840 Mon Sep 17 00:00:00 2001 From: Thales Pereira <31625914+thcp@users.noreply.github.com> Date: Mon, 31 Aug 2026 18:14:43 +0100 Subject: [PATCH] fix(library): stop deleted songs coming back Reported on macOS: a song deleted by clearing the trash, or by Settings -> "Reset app data", returns later. Deletion had two halves and both swallowed their failures, so several independent paths produced the same symptom. The root cause is that restore() adopts any job-shaped directory it finds. That is right for a library whose registry was lost and wrong for a job the user deleted whose files outlived the delete. Nothing on the server knew the difference, so the only thing standing between a failed delete and a resurrected song was a client-side tombstone -- which "Reset app data" wipes on its way out. The registry now keeps its own deletion record. Orphan recovery skips those ids, so a directory that survives a delete stays gone regardless of what the client does. Records are pruned once their directory is finally absent, so the set stays bounded rather than growing for the life of the install. _rmtree_job reports whether the files actually went away instead of logging and returning None, and retries once: on macOS the usual 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. delete_job records the deletion either way and tells the caller when files remain. reset_all returns what it could not remove and records the survivors, and /api/reset reports the count instead of an unconditional {"ok": true} that the frontend took as licence to wipe its own tombstone. On the client, the tombstone write and the DELETE calls are both awaited. They were fire-and-forget with .catch(() => {}), so quitting soon after clearing the bin lost the tombstone, and a delete that failed -- a 409 on a job stuck in "queued", a 500 when files could not be removed -- was invisible. Failures now surface through notifyFailure, and the button is disabled while it runs. Verified: removing the deletion record makes the two resurrection tests fail. Refs #521 --- app/api/jobs.py | 41 ++++++-- app/core/registry.py | 64 +++++++++++- app/main.py | 11 +- static/js/catalog.js | 64 +++++++++--- static/js/i18n.js | 1 + tests/conftest.py | 4 + tests/test_deleted_jobs_stay_deleted.py | 132 ++++++++++++++++++++++++ tests/test_reset.py | 6 +- 8 files changed, 295 insertions(+), 28 deletions(-) create mode 100644 tests/test_deleted_jobs_stay_deleted.py diff --git a/app/api/jobs.py b/app/api/jobs.py index 4f16765f..7e40f3b8 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: @@ -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"} 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 9bd29fbe..54622795 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()