From 20ca4bb981ce472e3c5b5bec79480145a6406738 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Sun, 7 Jun 2026 23:02:34 -0700 Subject: [PATCH] Import: don't duration-quarantine manual imports against a re-resolved release (#804) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- core/imports/file_integrity.py | 20 ++++++++++++++ core/imports/pipeline.py | 9 ++++++- core/imports/routes.py | 3 +++ .../test_expected_duration_for_check.py | 27 +++++++++++++++++++ tests/imports/test_import_routes.py | 3 ++- 5 files changed, 60 insertions(+), 2 deletions(-) create mode 100644 tests/imports/test_expected_duration_for_check.py diff --git a/core/imports/file_integrity.py b/core/imports/file_integrity.py index 615af097..4891cfd1 100644 --- a/core/imports/file_integrity.py +++ b/core/imports/file_integrity.py @@ -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. diff --git a/core/imports/pipeline.py b/core/imports/pipeline.py index 369b4d66..792af6d6 100644 --- a/core/imports/pipeline.py +++ b/core/imports/pipeline.py @@ -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 diff --git a/core/imports/routes.py b/core/imports/routes.py index 1be49b49..c5f49943 100644 --- a/core/imports/routes.py +++ b/core/imports/routes.py @@ -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) diff --git a/tests/imports/test_expected_duration_for_check.py b/tests/imports/test_expected_duration_for_check.py new file mode 100644 index 00000000..62d312af --- /dev/null +++ b/tests/imports/test_expected_duration_for_check.py @@ -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 diff --git a/tests/imports/test_import_routes.py b/tests/imports/test_import_routes.py index cfc7d8ce..2e1b080d 100644 --- a/tests/imports/test_import_routes.py +++ b/tests/imports/test_import_routes.py @@ -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)