Fix #764: manual import reported quarantined files as a successful "Done"
The manual-import routes (album + singles) call post_process_matched_download directly. When the pipeline quarantines a file — integrity / AcoustID / FLAC bit-depth — or hits the race guard, it sets a context flag and RETURNS NORMALLY (it only marks the task failed + notifies when there's a task_id, which manual imports don't have). So the inner pipeline raised no exception, and routes.py counted `processed += 1` for a file that had just been moved to ss_quarantine, not the library. Result: the UI shows a green "Done" while the track silently vanished — exactly the #764 report (Coldplay - Yellow.flac -> ss_quarantine, but "Done"). The download path already handles this in post_process_matched_download_with_verification (it reads the same flags and marks the task failed); only the manual-import routes were missing the check. Fix: new pure helper import_rejection_reason(context) returns a human-readable reason for any terminal rejection (_integrity_failure_msg / _acoustid_quarantined / _bitdepth_rejected / _race_guard_failed) or None for a clean import. Both manual-import routes now consult it: album_process reports the track in `errors` instead of counting it processed; process_single_import_file returns ("error", reason) instead of ("ok", ...). Verified every move_to_quarantine call site (4, all in pipeline.py) sets one of those flags, so no quarantine path slips through. This also delivers the "direct display of the error" the reporter asked for — the reason now surfaces in the response `errors` list. Does NOT address the reverse symptom ("failed even though it moved correctly") — not yet root-caused — nor the separate bit-depth hole on the download-path wrapper. Tests: tests/imports/test_import_rejection_reason.py (10) — each trigger detected, falsy flags ignored, deterministic ordering, plus two route-level tests driving the REAL process_single_import_file (quarantine -> "error"; clean -> "ok").
This commit is contained in:
parent
3dfec8a157
commit
3c15041b88
3 changed files with 191 additions and 2 deletions
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
145
tests/imports/test_import_rejection_reason.py
Normal file
145
tests/imports/test_import_rejection_reason.py
Normal file
|
|
@ -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"
|
||||
Loading…
Reference in a new issue