diff --git a/core/imports/pipeline.py b/core/imports/pipeline.py index fa196aa5..4263ba5a 100644 --- a/core/imports/pipeline.py +++ b/core/imports/pipeline.py @@ -110,6 +110,33 @@ def _mark_task_quarantined(context: dict, quarantine_path: str | None) -> None: download_tasks[task_id]['quarantine_entry_id'] = entry_id +def import_rejection_reason(context: dict) -> str | None: + """Human-readable reason if post-processing terminally rejected the file + (quarantine or race-guard), else ``None`` for a clean import. + + ``post_process_matched_download`` signals these outcomes by setting context + flags and returning normally — it only raises on unexpected errors. The + download path reads those flags in + ``post_process_matched_download_with_verification`` and marks the task + failed, but the MANUAL-import routes call ``post_process_matched_download`` + directly with no task_id, so without this check a quarantined file (now in + ss_quarantine, not the library) is counted as a successful import and the + UI shows a green "Done" (#764). Pure + testable: it only inspects the + context dict the inner pipeline populated.""" + if context.get('_integrity_failure_msg'): + return f"integrity check failed: {context['_integrity_failure_msg']}" + if context.get('_acoustid_quarantined'): + return ( + "AcoustID verification failed: " + f"{context.get('_acoustid_failure_msg', 'fingerprint mismatch')}" + ) + if context.get('_bitdepth_rejected'): + return "rejected by bit-depth filter" + if context.get('_race_guard_failed'): + return "source file disappeared before import completed" + return None + + def build_import_pipeline_runtime( *, automation_engine: Any | None = None, diff --git a/core/imports/routes.py b/core/imports/routes.py index edd38f08..1be49b49 100644 --- a/core/imports/routes.py +++ b/core/imports/routes.py @@ -11,6 +11,7 @@ from typing import Any, Callable, Dict from core.imports.album import build_album_import_context, build_album_import_match_payload, resolve_album_artist_context from core.imports.context import get_import_context_artist, get_import_track_info, normalize_import_context from core.imports.filename import parse_filename_metadata +from core.imports.pipeline import import_rejection_reason from core.imports.staging import ( AUDIO_EXTENSIONS, get_import_suggestions_cache, @@ -331,8 +332,17 @@ def album_process(runtime: ImportRouteRuntime, data: Dict[str, Any]) -> tuple[Di try: runtime.post_process_matched_download(context_key, context, file_path) - processed += 1 - runtime.logger.info("Import processed: %s. %s from %s", track_number, track_name, album_name) + # A quarantine/race-guard rejection returns normally (no + # exception) and leaves the file in ss_quarantine, NOT the + # library — so it must be reported as an error, not counted + # as a successful import (#764). + reject_reason = import_rejection_reason(context) + if reject_reason: + errors.append(f"{track_name}: {reject_reason}") + runtime.logger.warning("Import rejected: %s — %s", track_name, reject_reason) + else: + processed += 1 + runtime.logger.info("Import processed: %s. %s from %s", track_number, track_name, album_name) except Exception as proc_err: err_msg = f"{track_name}: {str(proc_err)}" errors.append(err_msg) @@ -422,6 +432,13 @@ def process_single_import_file(runtime: ImportRouteRuntime, file_info: Dict[str, context_key = f"import_single_{uuid.uuid4().hex[:8]}" runtime.post_process_matched_download(context_key, context, file_path) + # Quarantine/race-guard returns normally but the file is in + # ss_quarantine, not the library — report it as an error rather than + # "ok", else the UI shows a green "Done" for a file that vanished (#764). + reject_reason = import_rejection_reason(context) + if reject_reason: + runtime.logger.warning("Import single rejected: %s — %s", final_title, reject_reason) + return ("error", f"{final_title}: {reject_reason}") runtime.logger.info( "Import single processed: %s by %s (source=%s)", final_title, diff --git a/tests/imports/test_import_rejection_reason.py b/tests/imports/test_import_rejection_reason.py new file mode 100644 index 00000000..9377ff6a --- /dev/null +++ b/tests/imports/test_import_rejection_reason.py @@ -0,0 +1,145 @@ +"""Tests for import_rejection_reason — the manual-import quarantine guard. + +Regression for #764: the manual-import routes call +``post_process_matched_download`` directly. A quarantine (integrity / AcoustID +/ bit-depth) or race-guard rejection returns NORMALLY (no exception) and leaves +the file in ss_quarantine, not the library — but the routes counted any +no-exception return as a successful import, so the UI showed a green "Done" for +a file that had actually vanished. ``import_rejection_reason`` reads the context +flags the inner pipeline sets so the routes can report those as errors. +""" + +from __future__ import annotations + +import os +import tempfile + +from core.imports.pipeline import import_rejection_reason +from core.imports.routes import ImportRouteRuntime, process_single_import_file + + +def test_clean_import_returns_none(): + assert import_rejection_reason({}) is None + assert import_rejection_reason({'is_album': True, 'track_info': {}}) is None + + +def test_integrity_failure_detected(): + reason = import_rejection_reason({'_integrity_failure_msg': 'duration drift 12s'}) + assert reason is not None + assert 'integrity' in reason.lower() + assert 'duration drift 12s' in reason + + +def test_acoustid_quarantine_detected(): + reason = import_rejection_reason({ + '_acoustid_quarantined': True, + '_acoustid_failure_msg': 'wrong artist: got Oasis expected Coldplay', + }) + assert reason is not None + assert 'acoustid' in reason.lower() + assert 'Coldplay' in reason + + +def test_acoustid_quarantine_without_message_still_flags(): + # The flag alone must trip it even if no message was stashed. + reason = import_rejection_reason({'_acoustid_quarantined': True}) + assert reason is not None + assert 'acoustid' in reason.lower() + + +def test_bitdepth_rejection_detected(): + reason = import_rejection_reason({'_bitdepth_rejected': True}) + assert reason is not None + assert 'bit-depth' in reason.lower() + + +def test_race_guard_failure_detected(): + reason = import_rejection_reason({'_race_guard_failed': True}) + assert reason is not None + assert 'disappeared' in reason.lower() + + +def test_falsy_flags_do_not_trip(): + # A flag present but falsy (e.g. integrity passed) must NOT be a rejection. + ctx = { + '_integrity_failure_msg': '', + '_acoustid_quarantined': False, + '_bitdepth_rejected': False, + '_race_guard_failed': False, + } + assert import_rejection_reason(ctx) is None + + +def test_integrity_takes_precedence_when_multiple_set(): + # Deterministic ordering: integrity first. + reason = import_rejection_reason({ + '_integrity_failure_msg': 'truncated', + '_acoustid_quarantined': True, + '_bitdepth_rejected': True, + }) + assert 'integrity' in reason.lower() + + +# ── route-level wiring: a quarantine must NOT report as a successful import ── + + +def _runtime_with_post_process(post_process): + """Build an ImportRouteRuntime wired with stub resolvers + the supplied + post_process_matched_download. Resolvers return a shared context dict so a + flag the post-processor sets is what import_rejection_reason later reads.""" + ctx = {} + + return ImportRouteRuntime( + get_single_track_import_context=lambda *a, **k: {"context": ctx, "source": "local"}, + normalize_import_context=lambda c: c, + get_import_context_artist=lambda c: {"name": "Coldplay"}, + get_import_track_info=lambda c: {"name": "Yellow"}, + post_process_matched_download=post_process, + ), ctx + + +def _tmp_audio_file(): + fd, path = tempfile.mkstemp(suffix=".flac") + os.close(fd) + with open(path, "wb") as f: + f.write(b"fLaC") # only needs to exist for os.path.isfile + return path + + +def test_single_import_quarantine_reported_as_error(): + # post-processing quarantines the file (sets the flag, returns normally). + def quarantining_post_process(context_key, context, file_path): + context['_acoustid_quarantined'] = True + context['_acoustid_failure_msg'] = 'wrong track' + + runtime, _ctx = _runtime_with_post_process(quarantining_post_process) + path = _tmp_audio_file() + try: + outcome, payload = process_single_import_file( + runtime, {"full_path": path, "filename": "Coldplay - Yellow.flac", + "title": "Yellow", "artist": "Coldplay"}, + ) + finally: + os.remove(path) + + assert outcome == "error" # NOT "ok" -> route won't count it processed + assert "AcoustID" in payload + + +def test_single_import_clean_reports_ok(): + # post-processing succeeds (no flags) -> import counts as processed. + def clean_post_process(context_key, context, file_path): + return None + + runtime, _ctx = _runtime_with_post_process(clean_post_process) + path = _tmp_audio_file() + try: + outcome, payload = process_single_import_file( + runtime, {"full_path": path, "filename": "Coldplay - Yellow.flac", + "title": "Yellow", "artist": "Coldplay"}, + ) + finally: + os.remove(path) + + assert outcome == "ok" + assert payload == "Yellow"