Import: don't duration-quarantine manual imports against a re-resolved release (#804)
CubeComming #804: importing Coldplay "Yellow" (the 269s Parachutes album track, correctly tagged) was quarantined — "Duration mismatch: file is 269.2s, expected 266.0s (drift 3.2s > tolerance 3.0s)". The expected 266s came from a re-resolved *single* edition, not the file's actual album. The duration-agreement integrity check exists to catch truncated/wrong slskd TRANSFERS — but a manual import is the user's own already-tagged file being sorted, so checking it against a re-resolved release just manufactures false quarantines. Fix: both manual-import paths (singles + album) now mark the context is_local_import; the integrity check skips the duration-agreement leg for local imports via expected_duration_for_check() (new pure helper). The size + mutagen-parse legs still run, so genuinely broken files are still caught — only the release-vs-file duration comparison is skipped, and only for manual imports. slskd downloads are completely unaffected. This does NOT change the deeper matching (file still groups under Singles vs the Parachutes album — the #767 canonical-version family); it stops the false quarantine so the file imports. Tests: 4 on the helper (local skips, download keeps, zero/None/garbage, string coercion) + updated the routes context assertion. 557 import/integrity tests pass.
This commit is contained in:
parent
2742f1fa47
commit
20ca4bb981
5 changed files with 60 additions and 2 deletions
|
|
@ -84,6 +84,26 @@ def resolve_duration_tolerance(value: Any) -> Optional[float]:
|
|||
return parsed
|
||||
|
||||
|
||||
def expected_duration_for_check(expected_ms: Any, is_local_import: bool) -> Optional[int]:
|
||||
"""The expected duration (ms) to run the duration-agreement leg against,
|
||||
or None to skip that leg.
|
||||
|
||||
The duration check exists to catch BROKEN slskd TRANSFERS (truncated /
|
||||
wrong-file downloads). A local/manual import is the user's own already-
|
||||
tagged file being sorted, not a transfer — duration-agreeing it against a
|
||||
re-resolved release is meaningless and produces false quarantines (#804:
|
||||
Coldplay "Yellow" album file, 269s, false-rejected against a *single*
|
||||
edition's 266s). So for local imports we skip the duration leg; the
|
||||
size + mutagen-parse legs still run and catch genuinely broken files.
|
||||
"""
|
||||
if is_local_import:
|
||||
return None
|
||||
try:
|
||||
return int(expected_ms) or None
|
||||
except (TypeError, ValueError):
|
||||
return None
|
||||
|
||||
|
||||
@dataclass
|
||||
class IntegrityResult:
|
||||
"""Outcome of an integrity check.
|
||||
|
|
|
|||
|
|
@ -32,7 +32,7 @@ from core.imports.context import (
|
|||
get_import_track_info,
|
||||
normalize_import_context,
|
||||
)
|
||||
from core.imports.file_integrity import check_audio_integrity, resolve_duration_tolerance
|
||||
from core.imports.file_integrity import check_audio_integrity, expected_duration_for_check, resolve_duration_tolerance
|
||||
from core.imports.filename import extract_track_number_from_filename
|
||||
from core.imports.guards import check_flac_bit_depth, move_to_quarantine
|
||||
from core.imports.quarantine import (
|
||||
|
|
@ -254,6 +254,13 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta
|
|||
_expected_duration_ms = int(_duration_track.get("duration_ms", 0) or 0) or None
|
||||
except Exception:
|
||||
_expected_duration_ms = None
|
||||
# Local/manual imports are the user's own files, not slskd transfers —
|
||||
# skip the duration-agreement leg (it would false-quarantine a file that
|
||||
# drifts from a re-resolved release; #804). Size + parse legs still run.
|
||||
_is_local_import = bool(context.get('is_local_import')) if isinstance(context, dict) else False
|
||||
_expected_duration_ms = expected_duration_for_check(_expected_duration_ms, _is_local_import)
|
||||
if _is_local_import and _expected_duration_ms is None:
|
||||
logger.debug("[Integrity] Local import — duration-agreement leg skipped for %s", _basename)
|
||||
|
||||
# User-configurable tolerance override. None = use built-in
|
||||
# auto-scaled defaults (3s normal / 5s for tracks >10min). Set
|
||||
|
|
|
|||
|
|
@ -329,6 +329,8 @@ def album_process(runtime: ImportRouteRuntime, data: Dict[str, Any]) -> tuple[Di
|
|||
total_discs=total_discs,
|
||||
source=source,
|
||||
)
|
||||
if isinstance(context, dict):
|
||||
context['is_local_import'] = True # user's own file, not an slskd transfer (#804)
|
||||
|
||||
try:
|
||||
runtime.post_process_matched_download(context_key, context, file_path)
|
||||
|
|
@ -425,6 +427,7 @@ def process_single_import_file(runtime: ImportRouteRuntime, file_info: Dict[str,
|
|||
override_source=manual_match_source,
|
||||
)
|
||||
context = runtime.normalize_import_context(resolved["context"])
|
||||
context['is_local_import'] = True # user's own file, not an slskd transfer (#804)
|
||||
artist_data = runtime.get_import_context_artist(context)
|
||||
track_data = runtime.get_import_track_info(context)
|
||||
final_title = track_data.get("name", title)
|
||||
|
|
|
|||
27
tests/imports/test_expected_duration_for_check.py
Normal file
27
tests/imports/test_expected_duration_for_check.py
Normal file
|
|
@ -0,0 +1,27 @@
|
|||
"""Duration-agreement leg is skipped for local/manual imports (#804).
|
||||
|
||||
The duration check catches truncated/wrong slskd downloads. A manual import is
|
||||
the user's own file being sorted — duration-agreeing it against a re-resolved
|
||||
release false-quarantines (Coldplay 'Yellow' album file vs a single's length).
|
||||
"""
|
||||
|
||||
from core.imports.file_integrity import expected_duration_for_check
|
||||
|
||||
|
||||
def test_local_import_skips_duration_leg():
|
||||
# Even with a valid expected duration, a local import returns None (skip).
|
||||
assert expected_duration_for_check(266000, is_local_import=True) is None
|
||||
|
||||
|
||||
def test_download_keeps_expected_duration():
|
||||
assert expected_duration_for_check(266000, is_local_import=False) == 266000
|
||||
|
||||
|
||||
def test_zero_or_missing_expected_is_none():
|
||||
assert expected_duration_for_check(0, is_local_import=False) is None
|
||||
assert expected_duration_for_check(None, is_local_import=False) is None
|
||||
assert expected_duration_for_check("nan", is_local_import=False) is None
|
||||
|
||||
|
||||
def test_string_numeric_expected_coerced():
|
||||
assert expected_duration_for_check("266000", is_local_import=False) == 266000
|
||||
|
|
@ -504,7 +504,8 @@ def test_process_single_import_file_resolves_and_posts_context(tmp_path):
|
|||
assert outcome == ("ok", "Song")
|
||||
assert len(post_calls) == 1
|
||||
assert post_calls[0][0].startswith("import_single_")
|
||||
assert post_calls[0][1] == {"track": {"name": "Song"}, "artist": {"name": "Artist"}}
|
||||
assert post_calls[0][1] == {"track": {"name": "Song"}, "artist": {"name": "Artist"},
|
||||
"is_local_import": True}
|
||||
assert post_calls[0][2] == str(audio_file)
|
||||
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue