fix: resolve relative library paths fully + run quality gate before AcoustID
1) _resolve_library_file_path now tries the FULL relative path (index 0)
against each base dir, not just suffixes. The library scanner stores clean
"Artist/Album/Track.flac" paths; skipping index 0 dropped the artist folder
so the file never resolved — every quality-scanner probe failed ("20/20
could not be probed"). Now they resolve under the transfer/library dir.
2) Quality gate moved BEFORE the AcoustID check in post_process_matched_download.
- A wrong-quality file is rejected without paying for an AcoustID fingerprint.
- context['_audio_quality'] is set before either gate quarantines, so the
real quality is recorded on the sidecar for EVERY quarantine trigger —
it's known when reviewing/approving any quarantined file.
- force_import still never fires on a quality mismatch (only AcoustID).
normalize_import_context mutates in place, so the moved block keeps its
context fields. New test pins the order + that AcoustID isn't run on a
quality reject.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
parent
ff061324ba
commit
8a22b3f8ba
3 changed files with 104 additions and 52 deletions
|
|
@ -403,6 +403,62 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta
|
||||||
_notify_download_completed(batch_id, task_id, success=False)
|
_notify_download_completed(batch_id, task_id, success=False)
|
||||||
return
|
return
|
||||||
|
|
||||||
|
# QUALITY GATE — runs BEFORE AcoustID so (1) a wrong-quality file is
|
||||||
|
# rejected without paying for an AcoustID fingerprint, and (2) the real
|
||||||
|
# audio quality is recorded on the context (→ quarantine sidecar) for
|
||||||
|
# EVERY trigger, so it's known when reviewing/approving any quarantined
|
||||||
|
# file. force_import still never fires on a quality mismatch.
|
||||||
|
context['_audio_quality'] = get_audio_quality_string(file_path)
|
||||||
|
if context['_audio_quality']:
|
||||||
|
logger.info(f"Audio quality detected: {context['_audio_quality']}")
|
||||||
|
|
||||||
|
_skip_quality = _should_skip_quarantine_check(context, 'bit_depth') or \
|
||||||
|
_should_skip_quarantine_check(context, 'quality')
|
||||||
|
rejection_reason = None if _skip_quality else check_quality_target(file_path, context)
|
||||||
|
if _skip_quality:
|
||||||
|
logger.info(f"[QualityGuard] Skipped (user approval) for {_basename}")
|
||||||
|
if rejection_reason:
|
||||||
|
try:
|
||||||
|
quarantine_path = move_to_quarantine(
|
||||||
|
file_path,
|
||||||
|
context,
|
||||||
|
rejection_reason,
|
||||||
|
automation_engine,
|
||||||
|
trigger='quality',
|
||||||
|
)
|
||||||
|
_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)
|
||||||
|
|
||||||
|
context['_bitdepth_rejected'] = True
|
||||||
|
task_id = context.get('task_id')
|
||||||
|
batch_id = context.get('batch_id')
|
||||||
|
with matched_context_lock:
|
||||||
|
matched_downloads_context.pop(context_key, None)
|
||||||
|
|
||||||
|
# Try the next-best candidate before giving up — same pattern
|
||||||
|
# as AcoustID and integrity failures.
|
||||||
|
if _requeue_quarantined_task_for_retry(task_id, batch_id, 'quality'):
|
||||||
|
logger.info(
|
||||||
|
"Quality mismatch for task %s — retrying next-best candidate: %s",
|
||||||
|
task_id, rejection_reason,
|
||||||
|
)
|
||||||
|
return
|
||||||
|
|
||||||
|
if task_id:
|
||||||
|
with tasks_lock:
|
||||||
|
if task_id in download_tasks:
|
||||||
|
download_tasks[task_id]['status'] = 'failed'
|
||||||
|
download_tasks[task_id]['error_message'] = f"Quality filter: {rejection_reason}"
|
||||||
|
if task_id and batch_id:
|
||||||
|
_notify_download_completed(batch_id, task_id, success=False)
|
||||||
|
return
|
||||||
|
|
||||||
_skip_acoustid = _should_skip_quarantine_check(context, 'acoustid')
|
_skip_acoustid = _should_skip_quarantine_check(context, 'acoustid')
|
||||||
if _skip_acoustid:
|
if _skip_acoustid:
|
||||||
logger.info(f"[AcoustID] Skipped (user approval) for {_basename}")
|
logger.info(f"[AcoustID] Skipped (user approval) for {_basename}")
|
||||||
|
|
@ -627,57 +683,6 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta
|
||||||
album_info.get('album_name', 'None'),
|
album_info.get('album_name', 'None'),
|
||||||
)
|
)
|
||||||
|
|
||||||
context['_audio_quality'] = get_audio_quality_string(file_path)
|
|
||||||
if context['_audio_quality']:
|
|
||||||
logger.info(f"Audio quality detected: {context['_audio_quality']}")
|
|
||||||
|
|
||||||
_skip_quality = _should_skip_quarantine_check(context, 'bit_depth') or \
|
|
||||||
_should_skip_quarantine_check(context, 'quality')
|
|
||||||
rejection_reason = None if _skip_quality else check_quality_target(file_path, context)
|
|
||||||
if _skip_quality:
|
|
||||||
logger.info(f"[QualityGuard] Skipped (user approval) for {_basename}")
|
|
||||||
if rejection_reason:
|
|
||||||
try:
|
|
||||||
quarantine_path = move_to_quarantine(
|
|
||||||
file_path,
|
|
||||||
context,
|
|
||||||
rejection_reason,
|
|
||||||
automation_engine,
|
|
||||||
trigger='quality',
|
|
||||||
)
|
|
||||||
_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)
|
|
||||||
|
|
||||||
context['_bitdepth_rejected'] = True
|
|
||||||
task_id = context.get('task_id')
|
|
||||||
batch_id = context.get('batch_id')
|
|
||||||
with matched_context_lock:
|
|
||||||
matched_downloads_context.pop(context_key, None)
|
|
||||||
|
|
||||||
# Try the next-best candidate before giving up — same pattern
|
|
||||||
# as AcoustID and integrity failures.
|
|
||||||
if _requeue_quarantined_task_for_retry(task_id, batch_id, 'quality'):
|
|
||||||
logger.info(
|
|
||||||
"Quality mismatch for task %s — retrying next-best candidate: %s",
|
|
||||||
task_id, rejection_reason,
|
|
||||||
)
|
|
||||||
return
|
|
||||||
|
|
||||||
if task_id:
|
|
||||||
with tasks_lock:
|
|
||||||
if task_id in download_tasks:
|
|
||||||
download_tasks[task_id]['status'] = 'failed'
|
|
||||||
download_tasks[task_id]['error_message'] = f"Quality filter: {rejection_reason}"
|
|
||||||
if task_id and batch_id:
|
|
||||||
_notify_download_completed(batch_id, task_id, success=False)
|
|
||||||
return
|
|
||||||
|
|
||||||
file_ext = os.path.splitext(file_path)[1]
|
file_ext = os.path.splitext(file_path)[1]
|
||||||
clean_track_name = get_import_clean_title(
|
clean_track_name = get_import_clean_title(
|
||||||
context,
|
context,
|
||||||
|
|
|
||||||
|
|
@ -235,6 +235,48 @@ def test_post_process_matched_download_forwards_separate_metadata_runtime(tmp_pa
|
||||||
# write to the task directly — it stashes on context and the wrapper applies it)
|
# write to the task directly — it stashes on context and the wrapper applies it)
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
def test_quality_gate_runs_before_acoustid(tmp_path, monkeypatch):
|
||||||
|
"""The quality check must run BEFORE AcoustID: a wrong-quality file is
|
||||||
|
quarantined with trigger='quality' and AcoustID is never fingerprinted (so
|
||||||
|
quality is known on every quarantine entry, and no wasted AcoustID call)."""
|
||||||
|
src = tmp_path / "source.flac"
|
||||||
|
src.write_bytes(b"fLaC")
|
||||||
|
|
||||||
|
# Reach the quality gate: bypass integrity + silence guards.
|
||||||
|
from core.imports.file_integrity import IntegrityResult
|
||||||
|
monkeypatch.setattr(import_pipeline, "check_audio_integrity",
|
||||||
|
lambda *_a, **_kw: IntegrityResult(ok=True, checks={}))
|
||||||
|
monkeypatch.setattr(import_pipeline, "detect_broken_audio", lambda *_a, **_kw: None)
|
||||||
|
|
||||||
|
# Wrong quality → rejection.
|
||||||
|
monkeypatch.setattr(import_pipeline, "get_audio_quality_string", lambda fp: "FLAC 16bit/44.1kHz")
|
||||||
|
monkeypatch.setattr(import_pipeline, "check_quality_target", lambda fp, ctx: "Quality mismatch: FLAC 16bit")
|
||||||
|
|
||||||
|
triggers = []
|
||||||
|
monkeypatch.setattr(import_pipeline, "move_to_quarantine",
|
||||||
|
lambda fp, ctx, reason, eng, trigger=None: triggers.append(trigger) or "/q/x.flac.quarantined")
|
||||||
|
monkeypatch.setattr(import_pipeline, "_mark_task_quarantined", lambda *a, **k: None)
|
||||||
|
monkeypatch.setattr(import_pipeline, "_requeue_quarantined_task_for_retry", lambda *a, **k: False)
|
||||||
|
|
||||||
|
# Spy: AcoustID must NOT be constructed when quality already rejected.
|
||||||
|
acoustid_constructed = []
|
||||||
|
fake_mod = types.SimpleNamespace(
|
||||||
|
AcoustIDVerification=lambda *a, **k: acoustid_constructed.append(True),
|
||||||
|
VerificationResult=types.SimpleNamespace(FAIL="fail"),
|
||||||
|
)
|
||||||
|
monkeypatch.setitem(sys.modules, "core.acoustid_verification", fake_mod)
|
||||||
|
|
||||||
|
runtime = types.SimpleNamespace(automation_engine=None, on_download_completed=None,
|
||||||
|
web_scan_manager=None, repair_worker=None)
|
||||||
|
context = {"track_info": {}, "task_id": None, "batch_id": None}
|
||||||
|
|
||||||
|
import_pipeline.post_process_matched_download("ctx", context, str(src), runtime)
|
||||||
|
|
||||||
|
assert triggers == ["quality"] # quarantined for quality
|
||||||
|
assert acoustid_constructed == [] # AcoustID never ran
|
||||||
|
assert context.get("_audio_quality") == "FLAC 16bit/44.1kHz" # recorded for the sidecar
|
||||||
|
|
||||||
|
|
||||||
def test_mark_task_quarantined_stashes_entry_id_when_task_id_absent():
|
def test_mark_task_quarantined_stashes_entry_id_when_task_id_absent():
|
||||||
ctx = {} # wrapper popped task_id before the inner pipeline ran
|
ctx = {} # wrapper popped task_id before the inner pipeline ran
|
||||||
import_pipeline._mark_task_quarantined(ctx, "/q/20260514_120000_song.flac.quarantined")
|
import_pipeline._mark_task_quarantined(ctx, "/q/20260514_120000_song.flac.quarantined")
|
||||||
|
|
|
||||||
|
|
@ -11796,7 +11796,12 @@ def _resolve_library_file_path(file_path):
|
||||||
for base_dir in [transfer_dir, download_dir] + list(library_dirs):
|
for base_dir in [transfer_dir, download_dir] + list(library_dirs):
|
||||||
if not base_dir or not os.path.isdir(base_dir):
|
if not base_dir or not os.path.isdir(base_dir):
|
||||||
continue
|
continue
|
||||||
for i in range(1, len(path_parts)):
|
# Start at index 0 so a clean relative path ("Artist/Album/Track.flac")
|
||||||
|
# is tried in FULL first — the library scanner stores exactly that, and
|
||||||
|
# skipping index 0 dropped the artist folder so it never matched. A
|
||||||
|
# Windows drive-letter part 0 ("E:") simply fails this attempt and falls
|
||||||
|
# through to the shorter suffixes below, so this is safe for both.
|
||||||
|
for i in range(0, len(path_parts)):
|
||||||
found = find_on_disk(base_dir, path_parts[i:])
|
found = find_on_disk(base_dir, path_parts[i:])
|
||||||
if found:
|
if found:
|
||||||
return found
|
return found
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue