From 8abf4700184779bc4854926209fa514627a4aa03 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Sun, 28 Jun 2026 15:10:43 -0700 Subject: [PATCH] =?UTF-8?q?imports:=20never=20delete=20a=20file=20we=20cou?= =?UTF-8?q?ldn't=20quarantine=20=E2=80=94=20leave=20it=20for=20retry?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Discord (Shdjfgatdif): "if a track isn't imported it should remain there, not be deleted, so we can retry." He was seeing failed downloads disappear and having to re-download. Normally a rejected file is QUARANTINED (moved to ss_quarantine, preserved + retryable), not deleted. But all four quarantine blocks (integrity / silence / quality / acoustid) had the same fallback: if move_to_quarantine itself raised, os.remove(file_path). On a NAS that move can fail (cross-device / permissions), so the except fired and the user's download was DELETED — the worst outcome, and exactly the re-download pain he reported. fix: on quarantine failure, log and LEAVE the file in place — never delete. The task is still marked failed and the batch still notified (that code runs after the try/except and never touched the deleted file), so the only behaviour change is "preserved instead of destroyed". Reviewed every os.remove in the pipeline: the remaining ones are success-path cleanups (replacing an existing destination, or removing a redundant download when the track is already in the library at equal/better quality) — left untouched. regression test drives the REAL pipeline through integrity-rejection with quarantine forced to raise, and asserts the source file is preserved while the task is still failed + notified. 1311 imports/downloads/quality tests green, ruff clean. --- core/imports/pipeline.py | 48 ++++++++++-------- tests/imports/test_import_pipeline.py | 70 +++++++++++++++++++++++++++ 2 files changed, 97 insertions(+), 21 deletions(-) diff --git a/core/imports/pipeline.py b/core/imports/pipeline.py index e23f9590..ad433c0f 100644 --- a/core/imports/pipeline.py +++ b/core/imports/pipeline.py @@ -325,11 +325,14 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta _mark_task_quarantined(context, quarantine_path) logger.error(f"File quarantined due to integrity failure: {quarantine_path}") except Exception as quarantine_error: - logger.error(f"Quarantine failed ({quarantine_error}), deleting broken file: {file_path}") - try: - os.remove(file_path) - except Exception as del_error: - logger.error(f"Could not delete broken file either: {del_error}") + # Quarantine MOVE failed (e.g. cross-device / permission on a NAS). + # Do NOT delete — destroying a download we couldn't even quarantine is + # data loss and forces a re-download. Leave it in place so it can be + # retried; the task is still marked failed below either way (#kettui). + logger.error( + f"Quarantine failed ({quarantine_error}) — leaving file in place " + f"for retry (not deleting): {file_path}" + ) with matched_context_lock: if context_key in matched_downloads_context: @@ -383,11 +386,12 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta _mark_task_quarantined(context, quarantine_path) logger.warning("File quarantined — incomplete/silent audio: %s", quarantine_path) except Exception as quarantine_error: - logger.error(f"Quarantine failed ({quarantine_error}), deleting file: {file_path}") - try: - os.remove(file_path) - except Exception as del_error: - logger.debug("delete broken file fallback: %s", del_error) + # Don't delete a file we couldn't quarantine — leave it for retry + # instead of forcing a re-download (data loss). See integrity block. + logger.error( + f"Quarantine failed ({quarantine_error}) — leaving file in place " + f"for retry (not deleting): {file_path}" + ) with matched_context_lock: matched_downloads_context.pop(context_key, None) @@ -437,11 +441,12 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta _mark_task_quarantined(context, quarantine_path) logger.info(f"File quarantined due to quality mismatch: {quarantine_path}") except Exception as quarantine_error: - logger.error(f"Quarantine failed ({quarantine_error}), deleting file: {file_path}") - try: - os.remove(file_path) - except Exception as e: - logger.debug("delete quarantine fallback: %s", e) + # Don't delete a file we couldn't quarantine — leave it for retry + # instead of forcing a re-download (data loss). See integrity block. + logger.error( + f"Quarantine failed ({quarantine_error}) — leaving file in place " + f"for retry (not deleting): {file_path}" + ) context['_bitdepth_rejected'] = True task_id = context.get('task_id') @@ -535,12 +540,13 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta _mark_task_quarantined(context, quarantine_path) logger.error(f"File quarantined due to verification failure: {quarantine_path}") except Exception as quarantine_error: - logger.error(f"Quarantine failed ({quarantine_error}), deleting wrong file: {file_path}") - logger.error(f"Quarantine failed, deleting wrong file: {file_path}") - try: - os.remove(file_path) - except Exception as del_error: - logger.error(f"Could not delete wrong file either: {del_error}") + # Don't delete a file we couldn't quarantine — leave it for + # retry instead of forcing a re-download (data loss). The + # task is still marked failed / requeued below. See integrity. + logger.error( + f"Quarantine failed ({quarantine_error}) — leaving file " + f"in place for retry (not deleting): {file_path}" + ) context['_acoustid_quarantined'] = True context['_acoustid_failure_msg'] = verification_msg diff --git a/tests/imports/test_import_pipeline.py b/tests/imports/test_import_pipeline.py index 06bffb6d..9b17dee9 100644 --- a/tests/imports/test_import_pipeline.py +++ b/tests/imports/test_import_pipeline.py @@ -711,3 +711,73 @@ def test_exhaustive_single_source_exhausted_fails(monkeypatch): assert task["status"] == "failed" assert submitted == [] assert completion == [("rbatch", "rtask", False)] + + +def test_quarantine_failure_preserves_file_instead_of_deleting(tmp_path, monkeypatch): + """REGRESSION: when move_to_quarantine itself FAILS (e.g. a cross-device move on + a NAS), the rejected file must be LEFT IN PLACE for retry — never deleted. + + Deleting a download we couldn't even quarantine is data loss that forces a + re-download (Discord: Shdjfgatdif). The task is still marked failed + the batch + still notified — only the destructive os.remove is gone. Drives the real pipeline + through the integrity-rejection path with quarantine forced to raise.""" + source_path = tmp_path / "source.flac" + source_path.write_bytes(b"audio") + + context_key, task_id, batch_id = "ctx-q", "task-q", "batch-q" + context = { + "search_result": {"is_simple_download": True, "filename": "Album/source.flac", "album": "Album"}, + "track_info": {}, "original_search_result": {}, "is_album_download": False, + "task_id": task_id, "batch_id": batch_id, + } + completion_calls = [] + + snap = (dict(runtime_state.matched_downloads_context), dict(runtime_state.download_tasks), + dict(runtime_state.download_batches), set(runtime_state.processed_download_ids), + dict(runtime_state.post_process_locks)) + for d in (runtime_state.matched_downloads_context, runtime_state.download_tasks, + runtime_state.download_batches, runtime_state.processed_download_ids, + runtime_state.post_process_locks): + d.clear() + + runtime = types.SimpleNamespace( + automation_engine=None, + on_download_completed=lambda b, t, success: completion_calls.append((b, t, success)), + web_scan_manager=types.SimpleNamespace(request_scan=lambda r: None), + repair_worker=None, + ) + fake_acoustid = types.ModuleType("core.acoustid_verification") + fake_acoustid.AcoustIDVerification = _FakeAcoustidVerifier + fake_acoustid.VerificationResult = types.SimpleNamespace(FAIL="FAIL") + monkeypatch.setitem(sys.modules, "core.acoustid_verification", fake_acoustid) + + from core.imports.file_integrity import IntegrityResult + # Integrity FAILS → enters the quarantine block. + monkeypatch.setattr(import_pipeline, "check_audio_integrity", + lambda *_a, **_k: IntegrityResult(ok=False, reason="broken (test)", checks={})) + # The quarantine MOVE itself raises → exercises the except branch (the fix). + def _boom(*_a, **_k): + raise OSError("cross-device link not permitted (simulated NAS)") + monkeypatch.setattr(import_pipeline, "move_to_quarantine", _boom) + monkeypatch.setattr(import_paths, "_get_config_manager", lambda: _Config(str(tmp_path / "Transfer"))) + monkeypatch.setattr(import_pipeline, "add_activity_item", lambda *a, **k: None) + + runtime_state.matched_downloads_context[context_key] = context + runtime_state.download_tasks[task_id] = {"track_info": {}, "status": "running"} + + try: + import_pipeline.post_process_matched_download_with_verification( + context_key, context, str(source_path), task_id, batch_id, runtime) + + # THE regression: a file we couldn't quarantine is preserved, not deleted. + assert source_path.exists(), "file must be LEFT IN PLACE when quarantine fails" + # Downstream still correct — task failed, batch notified of failure. + assert runtime_state.download_tasks[task_id]["status"] == "failed" + assert completion_calls == [(batch_id, task_id, False)] + finally: + for d, original in zip( + (runtime_state.matched_downloads_context, runtime_state.download_tasks, + runtime_state.download_batches, runtime_state.processed_download_ids, + runtime_state.post_process_locks), snap): + d.clear() + d.update(original)