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)