mirror of https://github.com/Nezreka/SoulSync.git
Merge pull request #485 from Nezreka/fix/integrity-rejection-marks-task-failed
Fix: tasks showed Completed when file was quarantinedpull/486/head
commit
fbf4bad47a
@ -0,0 +1,204 @@
|
||||
"""Tighten the AcoustID "language/script" skip exemption.
|
||||
|
||||
User report (Mr. Morale download): three different track requests
|
||||
(Rich Interlude, Savior Interlude, Savior) each received the same
|
||||
WRONG audio file (Kendrick's R.O.T.C Interlude from his 2010 mixtape).
|
||||
AcoustID flagged the title mismatch but the verification logic
|
||||
SKIPPED rather than FAILED with the reason "likely same song in
|
||||
different language/script."
|
||||
|
||||
The old condition was:
|
||||
best_score >= 0.95 AND (title_sim >= 0.55 OR artist_sim >= match)
|
||||
|
||||
That OR-clause fired for English-vs-English titles by the same artist
|
||||
that share NO actual content — same artist + word "interlude" in both
|
||||
titles cleared the bar. The skip then trusted the wrong file as
|
||||
correct.
|
||||
|
||||
New condition: only skip when there's positive evidence the mismatch
|
||||
is a transliteration / language-script case:
|
||||
- (a) Either side of the comparison contains non-ASCII characters AND
|
||||
artist matches strongly. Real cases: Japanese kanji ↔ romaji,
|
||||
Korean hangul ↔ romaji, etc.
|
||||
- (b) BOTH title AND artist similarity are very high (>=0.80, ARTIST
|
||||
threshold). Real cases: title differs only by punctuation /
|
||||
casing that fell below strict-match thresholds.
|
||||
|
||||
For English-vs-English with very different titles by the same artist,
|
||||
the skip no longer fires — verification correctly returns FAIL,
|
||||
quarantining the wrong file.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from unittest.mock import patch
|
||||
|
||||
import pytest
|
||||
|
||||
from core.acoustid_verification import (
|
||||
AcoustIDVerification,
|
||||
VerificationResult,
|
||||
)
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def verifier(monkeypatch):
|
||||
"""A verifier with the network/fingerprint side stubbed so we can
|
||||
drive the title/artist comparison logic directly."""
|
||||
v = AcoustIDVerification()
|
||||
|
||||
# Stub availability check to avoid touching real chromaprint
|
||||
class _StubClient:
|
||||
def is_available(self):
|
||||
return True, 'available'
|
||||
|
||||
def fingerprint_and_lookup(self, path):
|
||||
# Each test injects its own desired return value via
|
||||
# monkeypatch on this method; default is empty.
|
||||
return None
|
||||
|
||||
v.acoustid_client = _StubClient()
|
||||
return v
|
||||
|
||||
|
||||
def _stub_lookup(verifier, *, recordings, best_score):
|
||||
"""Make `fingerprint_and_lookup` return a fabricated AcoustID result."""
|
||||
verifier.acoustid_client.fingerprint_and_lookup = lambda path: {
|
||||
'recordings': recordings,
|
||||
'best_score': best_score,
|
||||
'recording_mbids': [r.get('id') for r in recordings if r.get('id')],
|
||||
}
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# The headline regression — Rich Interlude vs R.O.T.C Interlude
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_english_titles_same_artist_no_longer_skipped(verifier):
|
||||
"""User's actual case: requested 'Rich (Interlude)' by Kendrick
|
||||
Lamar, AcoustID identified the file as 'R.O.T.C. (interlude)' by
|
||||
Kendrick Lamar. Same artist, same word 'interlude', but completely
|
||||
different songs. Old skip-logic let it pass; new logic must FAIL
|
||||
so the file gets quarantined."""
|
||||
_stub_lookup(verifier, recordings=[
|
||||
{'title': 'R.O.T.C. (interlude)', 'artist': 'Kendrick Lamar feat. BJ the Chicago Kid'},
|
||||
], best_score=0.96)
|
||||
|
||||
result, msg = verifier.verify_audio_file(
|
||||
'/fake/path.flac',
|
||||
'Rich (Interlude)',
|
||||
'Kendrick Lamar',
|
||||
)
|
||||
assert result == VerificationResult.FAIL
|
||||
# Message should be the wrong-file message, NOT the language/script skip
|
||||
assert 'mismatch' in msg.lower()
|
||||
assert 'language/script' not in msg.lower()
|
||||
|
||||
|
||||
def test_savior_request_returning_rotc_no_longer_skipped(verifier):
|
||||
"""Same bug surface, different track. Confirms the fix isn't
|
||||
Rich-Interlude-specific."""
|
||||
_stub_lookup(verifier, recordings=[
|
||||
{'title': 'R.O.T.C. (interlude)', 'artist': 'Kendrick Lamar feat. BJ the Chicago Kid'},
|
||||
], best_score=0.96)
|
||||
|
||||
result, _msg = verifier.verify_audio_file(
|
||||
'/fake/path.flac',
|
||||
'Savior',
|
||||
'Kendrick Lamar',
|
||||
)
|
||||
assert result == VerificationResult.FAIL
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# The legitimate skip cases — must STILL fire
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_japanese_kanji_to_romaji_still_skipped(verifier):
|
||||
"""Real language/script case: AcoustID's database has the kanji
|
||||
title, the user requested the romaji version. Same artist (in
|
||||
Latin script), high fingerprint confidence. Skip should still
|
||||
fire so a correct file isn't false-quarantined."""
|
||||
_stub_lookup(verifier, recordings=[
|
||||
{'title': '残酷な天使のテーゼ', 'artist': 'Yoko Takahashi'},
|
||||
], best_score=0.97)
|
||||
|
||||
result, msg = verifier.verify_audio_file(
|
||||
'/fake/path.flac',
|
||||
'Zankoku na Tenshi no Theze',
|
||||
'Yoko Takahashi',
|
||||
)
|
||||
assert result == VerificationResult.SKIP
|
||||
assert 'language/script' in msg.lower()
|
||||
|
||||
|
||||
def test_minor_punctuation_difference_passes_outright(verifier):
|
||||
"""Punctuation-only difference: both 'MAAD' and 'M.A.A.D' normalize
|
||||
similarly enough that the strict TITLE_MATCH_THRESHOLD is met and
|
||||
verification PASSES (better outcome than SKIP). Pin this so a
|
||||
future tightening of the strict thresholds doesn't accidentally
|
||||
push these into the FAIL bucket."""
|
||||
_stub_lookup(verifier, recordings=[
|
||||
{'title': 'M.A.A.D City', 'artist': 'Kendrick Lamar'},
|
||||
], best_score=0.97)
|
||||
|
||||
result, _msg = verifier.verify_audio_file(
|
||||
'/fake/path.flac',
|
||||
'MAAD City',
|
||||
'Kendrick Lamar',
|
||||
)
|
||||
# PASS or SKIP both fine — the critical assertion is "not FAIL".
|
||||
assert result != VerificationResult.FAIL
|
||||
|
||||
|
||||
def test_low_fingerprint_score_never_skipped(verifier):
|
||||
"""Below the 0.95 confidence floor, the skip exemption should
|
||||
never fire — even for plausibly-real language/script cases. We
|
||||
don't have enough signal to be sure the audio matches."""
|
||||
_stub_lookup(verifier, recordings=[
|
||||
{'title': '残酷な天使のテーゼ', 'artist': 'Yoko Takahashi'},
|
||||
], best_score=0.80) # below 0.95 floor
|
||||
|
||||
result, _msg = verifier.verify_audio_file(
|
||||
'/fake/path.flac',
|
||||
'Zankoku na Tenshi no Theze',
|
||||
'Yoko Takahashi',
|
||||
)
|
||||
assert result == VerificationResult.FAIL
|
||||
|
||||
|
||||
def test_high_score_but_artist_mismatch_no_longer_skipped(verifier):
|
||||
"""Even with high fingerprint AND non-ASCII chars present, if the
|
||||
artist DOESN'T match well, we don't have enough signal to skip.
|
||||
Could be a cover by a different artist."""
|
||||
_stub_lookup(verifier, recordings=[
|
||||
{'title': '残酷な天使のテーゼ', 'artist': 'Some Other Singer'},
|
||||
], best_score=0.97)
|
||||
|
||||
result, _msg = verifier.verify_audio_file(
|
||||
'/fake/path.flac',
|
||||
'Zankoku na Tenshi no Theze',
|
||||
'Yoko Takahashi',
|
||||
)
|
||||
assert result == VerificationResult.FAIL
|
||||
|
||||
|
||||
def test_old_loose_threshold_no_longer_fires_for_unrelated_titles(verifier):
|
||||
"""Pin the negative case for the old loose threshold (title_sim
|
||||
>= 0.55). 'Crown' vs 'Crown of Thorns' had similarity around 0.6
|
||||
in some normalizations — under old logic with high confidence
|
||||
and matching artist that would skip. New logic requires title_sim
|
||||
>= 0.80 OR non-ASCII presence."""
|
||||
_stub_lookup(verifier, recordings=[
|
||||
{'title': 'Crown of Thorns', 'artist': 'Kendrick Lamar'},
|
||||
], best_score=0.96)
|
||||
|
||||
result, _msg = verifier.verify_audio_file(
|
||||
'/fake/path.flac',
|
||||
'Crown',
|
||||
'Kendrick Lamar',
|
||||
)
|
||||
# User asked for 'Crown', got 'Crown of Thorns' — should FAIL now
|
||||
assert result == VerificationResult.FAIL
|
||||
@ -0,0 +1,202 @@
|
||||
"""Pin the contract: integrity rejection must mark the task as failed.
|
||||
|
||||
User report (Mr. Morale download): three tracks (Rich Interlude,
|
||||
Savior Interlude, Savior) showed ✅ Completed in the modal but were
|
||||
missing from disk. Log trace at line 932 of `core/imports/pipeline.py`
|
||||
revealed the bug:
|
||||
|
||||
No _final_processed_path in context for task <id> — cannot verify, assuming success
|
||||
|
||||
Inner ``post_process_matched_download`` quarantined the source file
|
||||
(integrity check rejected duration mismatch on a wrong-content file),
|
||||
which left no ``_final_processed_path`` in the context. The outer
|
||||
verification wrapper saw no path and fell through to the "assuming
|
||||
success" branch, marking the task as ✅ Completed even though the file
|
||||
was in quarantine and would never reach the destination.
|
||||
|
||||
Fix: the wrapper now explicitly checks for ``_integrity_failure_msg``
|
||||
and ``_race_guard_failed`` markers BEFORE the "assume success" branch.
|
||||
If any failure marker is set, the task is marked failed with a
|
||||
descriptive error message and the batch tracker is notified with
|
||||
``success=False``.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import threading
|
||||
import types
|
||||
from unittest.mock import patch
|
||||
|
||||
import pytest
|
||||
|
||||
import core.imports.pipeline as import_pipeline
|
||||
import core.runtime_state as runtime_state
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Test scaffolding
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def _isolate_state():
|
||||
"""Snapshot + restore the global runtime maps so this test can mutate
|
||||
them without polluting other tests."""
|
||||
snapshot = {
|
||||
'tasks': dict(runtime_state.download_tasks),
|
||||
'batches': dict(runtime_state.download_batches),
|
||||
'matched_ctx': dict(runtime_state.matched_downloads_context),
|
||||
}
|
||||
runtime_state.download_tasks.clear()
|
||||
runtime_state.download_batches.clear()
|
||||
runtime_state.matched_downloads_context.clear()
|
||||
yield
|
||||
runtime_state.download_tasks.clear()
|
||||
runtime_state.download_tasks.update(snapshot['tasks'])
|
||||
runtime_state.download_batches.clear()
|
||||
runtime_state.download_batches.update(snapshot['batches'])
|
||||
runtime_state.matched_downloads_context.clear()
|
||||
runtime_state.matched_downloads_context.update(snapshot['matched_ctx'])
|
||||
|
||||
|
||||
def _build_runtime(completion_calls):
|
||||
return types.SimpleNamespace(
|
||||
automation_engine=None,
|
||||
on_download_completed=lambda batch, task, success: completion_calls.append(
|
||||
(batch, task, success)
|
||||
),
|
||||
web_scan_manager=None,
|
||||
repair_worker=None,
|
||||
)
|
||||
|
||||
|
||||
def _seed_task(task_id: str = 't1', batch_id: str = 'b1') -> None:
|
||||
runtime_state.download_tasks[task_id] = {
|
||||
'task_id': task_id,
|
||||
'batch_id': batch_id,
|
||||
'status': 'downloading',
|
||||
'track_info': {'name': 'Rich (Interlude)'},
|
||||
}
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# The wrapper-level fix
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_integrity_failure_marker_marks_task_failed(_isolate_state):
|
||||
"""When inner code sets ``_integrity_failure_msg``, the wrapper
|
||||
must mark the task failed — NOT fall through to "assume success"."""
|
||||
completion_calls = []
|
||||
runtime = _build_runtime(completion_calls)
|
||||
|
||||
_seed_task('t1', 'b1')
|
||||
|
||||
context = {
|
||||
'task_id': 't1',
|
||||
'batch_id': 'b1',
|
||||
'context_key': 'test::ctx',
|
||||
# Simulate inner code's integrity-rejection state — file went to
|
||||
# quarantine, _final_processed_path NEVER got set.
|
||||
'_integrity_failure_msg': 'Duration mismatch: file is 163s, expected 152s (drift 11s)',
|
||||
}
|
||||
|
||||
# Inner post-processor is a no-op for this test — we're verifying the
|
||||
# wrapper-level state machine. Stub everything inside `with_verification`
|
||||
# that would otherwise touch real disk / acoustid / etc.
|
||||
with patch.object(import_pipeline, 'post_process_matched_download',
|
||||
lambda *a, **kw: None):
|
||||
import_pipeline.post_process_matched_download_with_verification(
|
||||
'test::ctx', context, '/fake/source.flac', 't1', 'b1', runtime,
|
||||
)
|
||||
|
||||
# Task explicitly marked failed with the integrity error message
|
||||
assert runtime_state.download_tasks['t1']['status'] == 'failed'
|
||||
assert 'integrity' in runtime_state.download_tasks['t1']['error_message'].lower()
|
||||
# Batch tracker notified with success=False
|
||||
assert ('b1', 't1', False) in completion_calls
|
||||
# Did NOT fall through to "assume success"
|
||||
assert ('b1', 't1', True) not in completion_calls
|
||||
|
||||
|
||||
def test_race_guard_failure_marker_marks_task_failed(_isolate_state):
|
||||
"""Same contract for the race-guard-failed marker (source file
|
||||
disappeared with no known destination)."""
|
||||
completion_calls = []
|
||||
runtime = _build_runtime(completion_calls)
|
||||
|
||||
_seed_task('t2', 'b2')
|
||||
|
||||
context = {
|
||||
'task_id': 't2',
|
||||
'batch_id': 'b2',
|
||||
'context_key': 'test::ctx2',
|
||||
'_race_guard_failed': True,
|
||||
}
|
||||
|
||||
with patch.object(import_pipeline, 'post_process_matched_download',
|
||||
lambda *a, **kw: None):
|
||||
import_pipeline.post_process_matched_download_with_verification(
|
||||
'test::ctx2', context, '/fake/source.flac', 't2', 'b2', runtime,
|
||||
)
|
||||
|
||||
assert runtime_state.download_tasks['t2']['status'] == 'failed'
|
||||
assert ('b2', 't2', False) in completion_calls
|
||||
|
||||
|
||||
def test_no_failure_markers_still_assumes_success(_isolate_state):
|
||||
"""The pre-existing "assume success" fallback must STILL fire when
|
||||
no failure markers are set — some legitimate flows complete without
|
||||
setting `_final_processed_path`. Don't regress that behavior."""
|
||||
completion_calls = []
|
||||
runtime = _build_runtime(completion_calls)
|
||||
|
||||
_seed_task('t3', 'b3')
|
||||
|
||||
context = {
|
||||
'task_id': 't3',
|
||||
'batch_id': 'b3',
|
||||
'context_key': 'test::ctx3',
|
||||
# No failure markers, no _final_processed_path
|
||||
}
|
||||
|
||||
with patch.object(import_pipeline, 'post_process_matched_download',
|
||||
lambda *a, **kw: None), \
|
||||
patch.object(import_pipeline, '_mark_task_completed',
|
||||
lambda task_id, ti: runtime_state.download_tasks[task_id].update(
|
||||
{'status': 'completed'}
|
||||
)):
|
||||
import_pipeline.post_process_matched_download_with_verification(
|
||||
'test::ctx3', context, '/fake/source.flac', 't3', 'b3', runtime,
|
||||
)
|
||||
|
||||
assert runtime_state.download_tasks['t3']['status'] == 'completed'
|
||||
assert ('b3', 't3', True) in completion_calls
|
||||
|
||||
|
||||
def test_integrity_failure_takes_priority_over_missing_final_path(_isolate_state):
|
||||
"""Integrity failure check must run BEFORE the missing-final-path
|
||||
fallback. Both conditions are true (no final path AND integrity
|
||||
failed); the failure wins."""
|
||||
completion_calls = []
|
||||
runtime = _build_runtime(completion_calls)
|
||||
|
||||
_seed_task('t4', 'b4')
|
||||
|
||||
context = {
|
||||
'task_id': 't4',
|
||||
'batch_id': 'b4',
|
||||
'context_key': 'test::ctx4',
|
||||
'_integrity_failure_msg': 'duration mismatch',
|
||||
# no _final_processed_path — would otherwise hit "assume success"
|
||||
}
|
||||
|
||||
with patch.object(import_pipeline, 'post_process_matched_download',
|
||||
lambda *a, **kw: None):
|
||||
import_pipeline.post_process_matched_download_with_verification(
|
||||
'test::ctx4', context, '/fake/source.flac', 't4', 'b4', runtime,
|
||||
)
|
||||
|
||||
assert runtime_state.download_tasks['t4']['status'] == 'failed'
|
||||
# Critical: must NOT have notified success
|
||||
assert ('b4', 't4', True) not in completion_calls
|
||||
Loading…
Reference in new issue