diff --git a/core/download_engine/engine.py b/core/download_engine/engine.py index e8fb9d12..1d555432 100644 --- a/core/download_engine/engine.py +++ b/core/download_engine/engine.py @@ -29,7 +29,6 @@ import asyncio import threading from typing import Any, Dict, Iterator, List, Optional, Tuple -from core.quality.selection import load_profile_targets, rank_with_targets from utils.logging_config import get_logger logger = get_logger("download_engine") @@ -393,21 +392,19 @@ class DownloadEngine: (tracks, albums) tuple, or ``([], [])`` when every source in the chain is exhausted. - Quality-aware: a source is only accepted when it can deliver a - candidate that meets a real target in the user's quality profile. - A source that meets no target is skipped in favour of the next one - (source priority still wins among satisfying sources). RAW tracks are - returned — the per-source ``satisfied`` check is a coarse gate; - match-filtering and final quality ranking happen in the orchestrator. - When no source satisfies a target, the first source's results are - returned as a fallback (unless the profile disables fallback). + Priority mode is deliberately quality-AGNOSTIC at search time — source + order is king and the first source that returns any tracks wins, exactly + matching pre-quality-system behaviour byte-for-byte (#896 review #3). + Quality-gating the priority path would deprioritise e.g. a soulseek + mp3 whose bitrate slskd omitted (``bitrate=None`` → "unsatisfied"), + changing which source wins and adding latency for users who never opted + in. Cross-source quality pooling is the job of best_quality mode + (``search_all_sources``); final per-result ranking still happens in the + orchestrator's match/quality filter. RAW tracks are returned. Replaces orchestrator's hand-rolled hybrid search loop. The chain is ordered (most-preferred first). """ - targets, fallback_enabled = load_profile_targets() - first_fallback = None # (tracks, albums) of the first source with hits - for i, source_name in enumerate(source_chain): plugin = self._plugins.get(source_name) if plugin is None: @@ -422,41 +419,13 @@ class DownloadEngine: tracks, albums = await plugin.search(query, timeout, progress_callback) if not tracks: continue - try: - _, satisfied = rank_with_targets( - tracks, targets, fallback_enabled=fallback_enabled, - ) - except Exception as rank_err: - # Fail open: a ranking bug must never drop a source's real - # download results. Accept them as-is. - logger.warning( - f"Quality ranking failed for {source_name} — accepting " - f"its results unranked: {rank_err}" - ) - satisfied = True - if satisfied: - logger.info( - f"{source_name} found {len(tracks)} tracks (meets quality target)" - ) - return (tracks, albums) - logger.info( - f"{source_name} found {len(tracks)} tracks but none meet a " - f"quality target — trying next source" - ) - if first_fallback is None: - first_fallback = (tracks, albums) + logger.info(f"{source_name} found {len(tracks)} tracks") + return (tracks, albums) except Exception as e: logger.warning(f"{source_name} search failed: {e}") - if fallback_enabled and first_fallback is not None: - logger.info( - "Hybrid search: no source met a quality target — falling back to " - "first source's results" - ) - return first_fallback - logger.warning( - "Hybrid search: all sources (%s) found nothing acceptable for: %s", + "Hybrid search: all sources (%s) found nothing for: %s", ', '.join(source_chain), query, ) return ([], []) diff --git a/core/quality/model.py b/core/quality/model.py index 1ce4ab67..c827028c 100644 --- a/core/quality/model.py +++ b/core/quality/model.py @@ -67,21 +67,33 @@ class AudioQuality: if self.sample_rate < target.min_sample_rate: return False else: - # No sample-rate info — fall back to kbps heuristic for FLAC. + # No sample-rate metadata (common on slskd FLAC). Use the kbps + # heuristic when a bitrate is present; otherwise we CANNOT + # confirm the spec, so fail the strict target rather than + # over-claim it — an unknown-spec FLAC must not outrank a known + # 16/44 FLAC under a hi-res target (#896 review #4). It falls to + # the plain-flac bucket instead. # 16-bit/44.1 kHz ≈ 1411 kbps; 24-bit/96 kHz ≈ 4608 kbps. if self.format.lower() == 'flac' and self.bitrate: required_kbps = _sample_rate_to_min_kbps(target.min_sample_rate, target.bit_depth or 24) if self.bitrate < required_kbps: return False + else: + return False if target.bit_depth: if self.bit_depth is not None: if self.bit_depth < target.bit_depth: return False else: - # No bit-depth info — use kbps heuristic. - # 16-bit FLAC ≈ ≤1450 kbps; 24-bit starts ~1500 kbps. - if self.format.lower() == 'flac' and target.bit_depth == 24: - if self.bitrate and self.bitrate < 1450: + # No bit-depth metadata. A hi-res (>=24-bit) target needs proof: + # use the kbps heuristic if a bitrate is present, else fail + # rather than over-claim. The 16-bit baseline still matches an + # unknown-spec FLAC (any FLAC is at least CD quality). #896 review #4. + if self.format.lower() == 'flac' and target.bit_depth >= 24: + if self.bitrate: + if self.bitrate < 1450: + return False + else: return False return True diff --git a/tests/quality/test_engine_fallback.py b/tests/quality/test_engine_fallback.py index e85af0c3..9e8d2128 100644 --- a/tests/quality/test_engine_fallback.py +++ b/tests/quality/test_engine_fallback.py @@ -1,15 +1,18 @@ -"""Engine search_with_fallback is quality-aware: it falls through to the next -source when the current source can deliver no target-satisfying quality, but -returns RAW tracks (match-filtering happens later in the orchestrator). +"""Engine ``search_with_fallback`` is the PRIORITY-mode search path and is +deliberately quality-agnostic: the first source in the chain that returns any +tracks wins (source order is king), byte-for-byte like the pre-quality-system +hybrid loop (#896 review #3). It skips unconfigured/unavailable sources, +swallows per-source exceptions, and returns RAW tracks (match-filtering + +final quality ranking happen later). Cross-source quality pooling lives in +best_quality mode (``search_all_sources``), not here. """ import asyncio import pytest -from core.download_engine import engine as engine_mod from core.download_engine.engine import DownloadEngine -from core.quality.model import AudioQuality, QualityTarget +from core.quality.model import AudioQuality class _Cand: @@ -19,21 +22,25 @@ class _Cand: class _FakePlugin: - def __init__(self, tracks): + def __init__(self, tracks, configured=True, raises=False): self._tracks = tracks + self._configured = configured + self._raises = raises self.searched = False def is_configured(self): - return True + return self._configured async def search(self, query, timeout=None, progress_callback=None): self.searched = True + if self._raises: + raise RuntimeError("boom") return (self._tracks, []) FLAC = AudioQuality('flac', sample_rate=44100, bit_depth=16) MP3 = AudioQuality('mp3', bitrate=320) -WANT_FLAC_ONLY = [QualityTarget(label='FLAC 16', format='flac', bit_depth=16)] +MP3_NO_BITRATE = AudioQuality('mp3') # slskd often omits bitrate def _engine_with(plugins): @@ -42,67 +49,79 @@ def _engine_with(plugins): return eng -def _patch_profile(monkeypatch, targets, fallback_enabled): - monkeypatch.setattr( - engine_mod, 'load_profile_targets', - lambda: (targets, fallback_enabled), - ) - - -def test_escalates_to_next_source_when_first_cannot_meet_target(monkeypatch): - _patch_profile(monkeypatch, WANT_FLAC_ONLY, True) +def test_returns_first_source_with_tracks_source_priority_king(): first = _FakePlugin([_Cand(MP3, 'a-mp3')]) second = _FakePlugin([_Cand(FLAC, 'b-flac')]) eng = _engine_with({'first': first, 'second': second}) tracks, _ = asyncio.run(eng.search_with_fallback('q', ['first', 'second'])) - assert [t.name for t in tracks] == ['b-flac'] # escalated to FLAC source - assert second.searched is True + assert [t.name for t in tracks] == ['a-mp3'] # first source wins regardless of quality + assert second.searched is False # never queried — priority is king -def test_stops_on_first_satisfying_source(monkeypatch): - _patch_profile(monkeypatch, WANT_FLAC_ONLY, True) - first = _FakePlugin([_Cand(FLAC, 'a-flac')]) +def test_metadata_less_mp3_still_wins_in_priority_mode(): + """An mp3 whose bitrate slskd omitted must NOT be deprioritised in priority + mode — the whole point of #896 review #3.""" + first = _FakePlugin([_Cand(MP3_NO_BITRATE, 'a-mp3')]) second = _FakePlugin([_Cand(FLAC, 'b-flac')]) eng = _engine_with({'first': first, 'second': second}) tracks, _ = asyncio.run(eng.search_with_fallback('q', ['first', 'second'])) - assert [t.name for t in tracks] == ['a-flac'] - assert second.searched is False # never queried — source priority king + assert [t.name for t in tracks] == ['a-mp3'] + assert second.searched is False -def test_returns_raw_tracks_not_pruned(monkeypatch): - # A source satisfied by one candidate must still return ALL its tracks so - # the orchestrator's match filter can pick the correct one. - _patch_profile(monkeypatch, WANT_FLAC_ONLY, True) - first = _FakePlugin([_Cand(MP3, 'wrong-but-present'), _Cand(FLAC, 'flac')]) +def test_skips_to_next_source_when_first_empty(): + first = _FakePlugin([]) + second = _FakePlugin([_Cand(FLAC, 'b-flac')]) + eng = _engine_with({'first': first, 'second': second}) + + tracks, _ = asyncio.run(eng.search_with_fallback('q', ['first', 'second'])) + + assert [t.name for t in tracks] == ['b-flac'] + assert second.searched is True + + +def test_returns_raw_tracks_not_pruned(): + # All of the winning source's tracks come back so the orchestrator's match + # filter can pick the correct one. + first = _FakePlugin([_Cand(MP3, 'one'), _Cand(FLAC, 'two')]) eng = _engine_with({'first': first}) tracks, _ = asyncio.run(eng.search_with_fallback('q', ['first'])) - names = {t.name for t in tracks} - assert names == {'wrong-but-present', 'flac'} # nothing pruned + assert {t.name for t in tracks} == {'one', 'two'} -def test_no_source_satisfies_fallback_on_returns_first_source(monkeypatch): - _patch_profile(monkeypatch, WANT_FLAC_ONLY, True) - first = _FakePlugin([_Cand(MP3, 'a-mp3')]) - second = _FakePlugin([_Cand(MP3, 'b-mp3')]) +def test_skips_unconfigured_source(): + first = _FakePlugin([_Cand(FLAC, 'a')], configured=False) + second = _FakePlugin([_Cand(MP3, 'b')]) eng = _engine_with({'first': first, 'second': second}) tracks, _ = asyncio.run(eng.search_with_fallback('q', ['first', 'second'])) - assert [t.name for t in tracks] == ['a-mp3'] # source priority for fallback + assert [t.name for t in tracks] == ['b'] + assert first.searched is False -def test_no_source_satisfies_fallback_off_returns_empty(monkeypatch): - _patch_profile(monkeypatch, WANT_FLAC_ONLY, False) - first = _FakePlugin([_Cand(MP3, 'a-mp3')]) - eng = _engine_with({'first': first}) +def test_swallows_per_source_exception_and_continues(): + first = _FakePlugin([], raises=True) + second = _FakePlugin([_Cand(MP3, 'b')]) + eng = _engine_with({'first': first, 'second': second}) - tracks, albums = asyncio.run(eng.search_with_fallback('q', ['first'])) + tracks, _ = asyncio.run(eng.search_with_fallback('q', ['first', 'second'])) + + assert [t.name for t in tracks] == ['b'] + + +def test_returns_empty_when_all_sources_empty(): + first = _FakePlugin([]) + second = _FakePlugin([]) + eng = _engine_with({'first': first, 'second': second}) + + tracks, albums = asyncio.run(eng.search_with_fallback('q', ['first', 'second'])) assert tracks == [] assert albums == [] diff --git a/tests/quality/test_model.py b/tests/quality/test_model.py index 62e5887a..56ba0db4 100644 --- a/tests/quality/test_model.py +++ b/tests/quality/test_model.py @@ -58,6 +58,32 @@ def test_format_mismatch_rejected(): assert AudioQuality('mp3', bitrate=320).matches_target(t) is False +# ── metadata-less FLAC must not over-claim a hi-res target (#896 review #4) ─ + +def test_metadata_less_flac_does_not_overclaim_hires_target(): + """A FLAC with no sample_rate/bit_depth metadata (common on slskd) must NOT + satisfy a strict hi-res target — otherwise it outranks and discards a real + 16/44 FLAC. Unknown spec fails the high tier, falling to a plain flac bucket.""" + hires = QualityTarget(format='flac', bit_depth=24, min_sample_rate=192000) + assert AudioQuality('flac').matches_target(hires) is False + + +def test_metadata_less_flac_does_not_overclaim_bit_depth_only_hires(): + """Same guard when the hi-res target constrains only bit depth.""" + hires = QualityTarget(format='flac', bit_depth=24) + assert AudioQuality('flac').matches_target(hires) is False + + +def test_metadata_less_flac_matches_plain_flac_target(): + """A bare FLAC still matches a plain 'flac (any)' target (the baseline).""" + assert AudioQuality('flac').matches_target(QualityTarget(format='flac')) is True + + +def test_metadata_less_flac_matches_16bit_baseline_target(): + """A bare FLAC satisfies the 16-bit baseline (any FLAC is >= CD quality).""" + assert AudioQuality('flac').matches_target(QualityTarget(format='flac', bit_depth=16)) is True + + # ── v2 -> v3 migration preserves the user's priority order ───────────────── def test_v2_to_v3_preserves_order_and_maps_fields():