From 486116c34f37b309c91f3eddfd04343c3526dc7d Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Sat, 2 May 2026 14:26:46 -0700 Subject: [PATCH] Honor lossy_copy.delete_original after successful conversion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported case (CAL): with lossy_copy.enabled=True, lossy_copy.delete_original=True, and codec=mp3, every download left both the original FLAC AND the converted MP3 in the target folder. Users opting into a lossy-only library ended up dual-format on every import. Root cause: ``core/imports/file_ops.py:create_lossy_copy`` reads ``lossy_copy.codec`` and ``lossy_copy.bitrate`` from config but never reads ``lossy_copy.delete_original``. The setting is only consulted by the pre-move source-vanished check at ``core/imports/pipeline.py:651`` (so the pipeline knows to look for a lossy variant when the FLAC has already moved on), but no code path actually deletes the source after conversion. Fix: after ffmpeg returns success and the QUALITY tag is written, check ``lossy_copy.delete_original`` and ``os.remove`` the original when enabled. Belt-and-suspenders: - Same-path guard (``os.path.normpath(out_path) != os.path.normpath(final_path)``) prevents accidentally wiping the just-converted file if a future codec choice somehow resolves out_path to the source path. - ``FileNotFoundError`` is treated as success (concurrent worker / dedup cleanup got there first). - Other ``OSError`` (permission denied, locked file) is logged but doesn't propagate — the conversion already succeeded, the user just has to clean up the original manually. Failure paths skip the delete: - ffmpeg returns non-zero → returns None, original stays - lossy_copy.enabled=False → early return before conversion runs - delete_original=False (default) → original stays 7 regression tests cover honored-when-enabled, kept-when-disabled, default-keep, ffmpeg-failure-path, lossy-disabled-path, racing-delete, and locked-file paths. Full pytest 1563 passed; ruff clean. Note: this PR does NOT address the second bug CAL mentioned (track re-downloaded despite already existing on disk). That symptom is caused by stale album metadata on the user's existing files — the library DB has the track tagged on a different album than the metadata source reports — combined with wishlist.allow_duplicate_tracks defaulting to True. Same class of issue partially addressed in PR fix/watchlist-redownload-and-duplicate-detection but compilation- album drift is the only currently-handled case. Tracking separately. --- core/imports/file_ops.py | 22 ++ .../test_lossy_copy_delete_original.py | 214 ++++++++++++++++++ webui/static/helper.js | 1 + 3 files changed, 237 insertions(+) create mode 100644 tests/imports/test_lossy_copy_delete_original.py diff --git a/core/imports/file_ops.py b/core/imports/file_ops.py index f119eff5..2a082247 100644 --- a/core/imports/file_ops.py +++ b/core/imports/file_ops.py @@ -440,6 +440,28 @@ def create_lossy_copy(final_path): audio.save() except Exception as tag_err: logger.error(f"[Lossy Copy] Could not update QUALITY tag: {tag_err}") + + # Honor the lossy_copy.delete_original setting — without this + # the original FLAC was always kept alongside the converted + # MP3/OPUS/AAC even when the user explicitly opted into a + # lossy-only library (Discord-reported by CAL). + if config_manager.get("lossy_copy.delete_original", False): + if os.path.normpath(out_path) != os.path.normpath(final_path): + try: + os.remove(final_path) + logger.info( + f"[Lossy Copy] Deleted original lossless source after conversion: " + f"{os.path.basename(final_path)}" + ) + except FileNotFoundError: + # Already gone — concurrent cleanup or another worker + # handled it. Not an error. + pass + except Exception as del_err: + logger.error( + f"[Lossy Copy] Could not delete original after conversion " + f"({os.path.basename(final_path)}): {del_err}" + ) return out_path logger.error(f"[Lossy Copy] ffmpeg failed: {result.stderr[:200]}") diff --git a/tests/imports/test_lossy_copy_delete_original.py b/tests/imports/test_lossy_copy_delete_original.py new file mode 100644 index 00000000..6089736e --- /dev/null +++ b/tests/imports/test_lossy_copy_delete_original.py @@ -0,0 +1,214 @@ +"""Regression tests for lossy_copy.delete_original honoring. + +Discord-reported (CAL): with ``lossy_copy.enabled=True``, +``lossy_copy.delete_original=True``, and ``codec=mp3``, downloads +ended up with BOTH the original FLAC AND the converted MP3 in the +target folder. The setting was being read by the pre-move source- +vanished check at ``core/imports/pipeline.py`` but never acted on +during the actual conversion step. Result: a "lossy-only" library +ended up dual-format on every import. + +These tests pin the behavior so the regression doesn't return — they +exercise ``create_lossy_copy`` directly with ffmpeg stubbed via +monkeypatch, asserting the original is deleted only when the setting +is enabled and the conversion succeeded. +""" + +from __future__ import annotations + +import os +import subprocess +from pathlib import Path +from types import SimpleNamespace +from unittest.mock import MagicMock + +import pytest + +from core.imports import file_ops + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +@pytest.fixture +def fake_flac(tmp_path: Path) -> Path: + """A placeholder FLAC file — content doesn't matter, ffmpeg call is stubbed.""" + src = tmp_path / "01 - Track.flac" + src.write_bytes(b"FAKE-FLAC-CONTENT") + return src + + +def _stub_config(monkeypatch, **overrides): + """Patch the file_ops module's config_manager so each test controls + only the keys it cares about.""" + defaults = { + "lossy_copy.enabled": True, + "lossy_copy.codec": "mp3", + "lossy_copy.bitrate": "320", + "lossy_copy.delete_original": False, + } + defaults.update(overrides) + + fake_cfg = MagicMock() + fake_cfg.get.side_effect = lambda key, default=None: defaults.get(key, default) + monkeypatch.setattr(file_ops, "config_manager", fake_cfg) + return defaults + + +def _stub_ffmpeg_success(monkeypatch, fake_flac: Path): + """Stub shutil.which to report ffmpeg available + subprocess.run to + write a fake MP3 to out_path and return success.""" + monkeypatch.setattr(file_ops.shutil, "which", lambda _: "/fake/ffmpeg") + + def _fake_run(cmd, **_kwargs): + # cmd[-1] is the out_path (per ffmpeg invocation in create_lossy_copy) + out_path = cmd[-1] + Path(out_path).write_bytes(b"FAKE-MP3-CONTENT") + return SimpleNamespace(returncode=0, stderr="", stdout="") + + monkeypatch.setattr(file_ops.subprocess, "run", _fake_run) + + # Skip the mutagen tagging step — file is fake bytes, mutagen would + # raise. Accepting the silent-fail path is fine here; tests assert + # on file presence, not tag content. + monkeypatch.setattr( + "mutagen.File", + lambda _path: None, + raising=False, + ) + + +def _stub_ffmpeg_failure(monkeypatch): + """Stub ffmpeg to return non-zero so the conversion path bails out.""" + monkeypatch.setattr(file_ops.shutil, "which", lambda _: "/fake/ffmpeg") + monkeypatch.setattr( + file_ops.subprocess, + "run", + lambda cmd, **kw: SimpleNamespace(returncode=1, stderr="fake ffmpeg error", stdout=""), + ) + + +# --------------------------------------------------------------------------- +# delete_original honored after successful conversion +# --------------------------------------------------------------------------- + + +class TestDeleteOriginalHonored: + def test_original_flac_removed_when_setting_enabled(self, monkeypatch, fake_flac: Path): + _stub_config(monkeypatch, **{"lossy_copy.delete_original": True}) + _stub_ffmpeg_success(monkeypatch, fake_flac) + + out_path = file_ops.create_lossy_copy(str(fake_flac)) + + assert out_path is not None + assert out_path.endswith(".mp3") + assert Path(out_path).exists(), "MP3 should have been written" + assert not fake_flac.exists(), \ + "Original FLAC must be removed when lossy_copy.delete_original=True" + + def test_original_flac_kept_when_setting_disabled(self, monkeypatch, fake_flac: Path): + _stub_config(monkeypatch, **{"lossy_copy.delete_original": False}) + _stub_ffmpeg_success(monkeypatch, fake_flac) + + out_path = file_ops.create_lossy_copy(str(fake_flac)) + + assert out_path is not None + assert Path(out_path).exists() + assert fake_flac.exists(), \ + "Original FLAC must survive when lossy_copy.delete_original=False" + + def test_default_is_keep_original(self, monkeypatch, fake_flac: Path): + """When the user never set the option, default = keep original. + Defensive: a missing config value must not silently drop files.""" + # Don't override delete_original — picks up the default in _stub_config (False) + _stub_config(monkeypatch) + _stub_ffmpeg_success(monkeypatch, fake_flac) + + file_ops.create_lossy_copy(str(fake_flac)) + assert fake_flac.exists() + + +# --------------------------------------------------------------------------- +# delete_original NOT triggered when conversion fails +# --------------------------------------------------------------------------- + + +class TestDeleteOriginalSkippedOnFailure: + def test_original_kept_when_ffmpeg_fails(self, monkeypatch, fake_flac: Path): + """If ffmpeg returns non-zero, the conversion is treated as failed + and the original must NOT be deleted (would leave the user with + no audio file at all).""" + _stub_config(monkeypatch, **{"lossy_copy.delete_original": True}) + _stub_ffmpeg_failure(monkeypatch) + + out_path = file_ops.create_lossy_copy(str(fake_flac)) + + assert out_path is None, "Conversion failure must return None" + assert fake_flac.exists(), \ + "Original FLAC must survive a failed conversion regardless of delete_original" + + def test_original_kept_when_lossy_copy_disabled(self, monkeypatch, fake_flac: Path): + """The function early-returns when lossy_copy.enabled=False — so + delete_original cannot fire even if it's enabled.""" + _stub_config(monkeypatch, **{ + "lossy_copy.enabled": False, + "lossy_copy.delete_original": True, + }) + _stub_ffmpeg_success(monkeypatch, fake_flac) + + result = file_ops.create_lossy_copy(str(fake_flac)) + assert result is None + assert fake_flac.exists() + + +# --------------------------------------------------------------------------- +# Defensive paths +# --------------------------------------------------------------------------- + + +class TestDeleteOriginalDefensive: + def test_does_not_crash_when_original_already_gone(self, monkeypatch, fake_flac: Path): + """If something else (concurrent worker, dedup cleanup) removed + the original between conversion and deletion, that's not an + error — we just got our wish slightly early.""" + _stub_config(monkeypatch, **{"lossy_copy.delete_original": True}) + _stub_ffmpeg_success(monkeypatch, fake_flac) + + original_remove = os.remove + + def _remove_after_unlinking_first(path, *args, **kwargs): + # Simulate the source being gone before the delete call: pre- + # remove on first call, then defer to real os.remove. + if Path(path) == fake_flac and fake_flac.exists(): + fake_flac.unlink() + # Now raise FileNotFoundError as os.remove would on a missing path + raise FileNotFoundError(2, "No such file or directory", str(path)) + return original_remove(path, *args, **kwargs) + + monkeypatch.setattr(file_ops.os, "remove", _remove_after_unlinking_first) + + out_path = file_ops.create_lossy_copy(str(fake_flac)) + assert out_path is not None + assert Path(out_path).exists() + assert not fake_flac.exists() + + def test_handles_oserror_during_delete_without_propagating(self, monkeypatch, fake_flac: Path): + """If the actual unlink fails (permission error, locked file, FS + full), the conversion is still considered successful — the lossy + copy already exists. We log the error but return the out_path so + the import pipeline can continue. The original is left in place + for the user to clean up manually.""" + _stub_config(monkeypatch, **{"lossy_copy.delete_original": True}) + _stub_ffmpeg_success(monkeypatch, fake_flac) + + def _failing_remove(path, *args, **kwargs): + raise PermissionError(13, "Permission denied", str(path)) + + monkeypatch.setattr(file_ops.os, "remove", _failing_remove) + + out_path = file_ops.create_lossy_copy(str(fake_flac)) + assert out_path is not None, "Failed delete must not break conversion return value" + assert Path(out_path).exists() + assert fake_flac.exists(), "Failed delete leaves the original in place" diff --git a/webui/static/helper.js b/webui/static/helper.js index f6d5657a..778e40f2 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3445,6 +3445,7 @@ const WHATS_NEW = { // --- post-2.4.1 dev work — entries hidden by _getLatestWhatsNewVersion until the build version bumps --- { date: 'Unreleased — 2.4.2 dev cycle' }, { title: 'Fix Qobuz Connection Not Sticking After Login', desc: 'logging in via the qobuz connect button on settings showed "connected: (active)" but underneath an error said "qobuz not authenticated...", and the dashboard indicator stayed yellow. cause: two separate qobuz client instances run side by side (one for the auth flow, one for the enrichment worker) and login only updated the first one. now the worker\'s client gets synced from config the moment login / token / logout completes, so the dashboard indicator goes green and connection-test stops yelling.', page: 'settings' }, + { title: 'Fix Lossy Copy Not Deleting Original FLAC', desc: 'with lossy copy enabled and "delete original" turned on (you wanted an mp3-only library), every download still left both the flac and the converted mp3 sitting in the same folder. the setting was being read but never acted on during the conversion step. now the original gets removed right after a successful conversion, with a same-path safety check + graceful handling if the original is already gone or locked.', page: 'settings' }, { title: 'Sidebar Library Button Shows Artist Breadcrumb', desc: 'when you open an artist detail page (from library, search, or the global search popover), the sidebar Library button now lights up and rewrites its label to "Library / Artist Name" — long names truncate with an ellipsis and the full name shows on hover. revertes to plain "Library" when you leave. purely visual, no functionality change.', page: 'library' }, { title: 'Enrichment Bubble Routes Consolidated', desc: 'internal — every dashboard enrichment bubble (musicbrainz, spotify, itunes, deezer, discogs, audiodb, lastfm, genius, tidal, qobuz) used to hit its own per-service status / pause / resume route in web_server.py. unified them under a single registry-driven endpoint set: /api/enrichment//. spotify\'s rate-limit guard, lastfm/genius yield-override behavior, and tidal/qobuz extra status fields are encoded as data on the registry. 27 new tests cover the registry behavior.' }, { title: 'Drop Old Per-Service Enrichment Routes', desc: 'internal — followup to the registry consolidation. now that the dashboard has cut over to /api/enrichment//, deleted the 30 hand-rolled per-service routes from web_server.py (musicbrainz/audiodb/discogs/deezer/spotify/itunes/lastfm/genius/tidal/qobuz status+pause+resume). ~510 lines gone from the monolith, no behavior change.' },