diff --git a/.github/workflows/docker-publish.yml b/.github/workflows/docker-publish.yml index a08b63a5..91daceac 100644 --- a/.github/workflows/docker-publish.yml +++ b/.github/workflows/docker-publish.yml @@ -9,9 +9,9 @@ on: workflow_dispatch: inputs: version_tag: - description: 'Version tag (e.g. 2.7.8)' + description: 'Version tag (e.g. 2.7.9)' required: true - default: '2.7.8' + default: '2.7.9' jobs: build-and-push: diff --git a/DISCOVER_BEST_IN_CLASS_PLAN.md b/DISCOVER_BEST_IN_CLASS_PLAN.md new file mode 100644 index 00000000..b406ad52 --- /dev/null +++ b/DISCOVER_BEST_IN_CLASS_PLAN.md @@ -0,0 +1,74 @@ +# discover page — best in class plan (#913 + full generator audit) + +morning notes. did the work overnight. tl;dr at top, details below, all of it `break nothing` + tested. + +## what i shipped tonight (done, tested, safe) + +### 1. listening recommendations (#913) — went from BROKEN to best-in-class + +the feature was silently producing **zero** recs on real data. dug in and found three stacked bugs in the generation: + +- **wrong id key (the killer).** `similar_artists.source_artist_id` is a *source* id (spotify/itunes/deezer), but the scanner built its id→name map from `artists.id` (the internal row id). so every edge resolved to nothing → 0 recs. proved it on your live db: internal-id join = 0 rows, spotify-id join = 71,636 rows. +- **consensus could never fire.** it fed the ranker `get_top_similar_artists`, which does `GROUP BY similar_artist_name` + `MAX(source_artist_id)` — collapsing every similar artist down to a *single* seed. the whole point of the ranker is "artist X is similar to 3 of your seeds = strong signal," and that signal was being flattened away before it ever reached the ranker. +- **similarity strength thrown away.** each edge stores a 1-10 closeness rank; it was ignored (everything weighted equally). + +the fix (all in the pure, tested core + thin scan wiring): +- build id→name from the **source-id columns**, query the **raw per-seed edges** (consensus preserved), and thread **similarity_rank** into the score so a seed's closest matches count for more. +- **recency-weighted seeds**: `weight = lifetime_plays + 1.5 × recent_30d_plays`. picks now track what you're into *now*, not just all-time totals. + +result on your actual library (simulated through the real code path): **40 recommendations, 13 with multi-seed consensus, all 40 with cached art.** top picks: Arcangel (Bad Bunny + Ozuna + J Balvin), Melanie Martinez (Ariana + Billie), Maluma, De La Ghetto — all coherent, all explainable. + +### 2. its own row on the discover page + +new row **"Based On Your Listening"** — play-weighted, consensus-ranked artist cards with a **"Because you listen to X, Y"** line. sits right above the library-driven "Recommended For You" row. purely additive: new endpoint `/api/discover/listening-recommendations`, new loader, hides itself when empty. + +**you need to run one watchlist scan** for the row to populate (the data regenerates during the scan — i did NOT touch your live db). before that scan the row just stays hidden; after it, it fills in. + +> note: this is deliberately different from the existing "Recommended For You" row. that one is driven by your *whole library / watchlist*. this one is driven by your *actual listening intensity* — the ~30 artists you really play, not the thousands you happen to own. + +### 3. Fresh Tape "only 5-10 tracks" — fixed + +root cause: `get_discovery_recent_albums` orders `release_date DESC`, so announced-but-unreleased albums sort to the *top* and ate the 50-album budget. the scanner skipped them *after* the budget was already spent → only a handful of released albums left → 5-10 tracks. fixed by fetching a generous budget (300) **and** excluding next-year albums at the query, so released albums fill the budget. the precise same-year `is_future_release` skip stays as a second guard. downstream caps (6/artist, top 75, take 50) unchanged. + +**tests:** 25 pure-core cases (consensus/similarity/recency) + 2 Fresh Tape regression tests, all green. full discovery suite (255) green. nothing else touched. + +--- + +## best-in-class roadmap for listening recs (next phases — your call) + +these are the levers to take it further. ordered by value-to-risk. none are required; tonight's work stands on its own. + +| phase | what | value | risk | notes | +|---|---|---|---|---| +| **3** | **playable track row** ✅ DONE | high | low-med | shipped: "🎧 Your Listening Mix" row — a track playlist (play/queue/download/sync) right under the artist row. stored as full render-ready dicts (not pool-hydrated, so it can't shrink on pool rotation like Fresh Tape does). | +| **4** | **direct top-tracks fetch** ✅ DONE | high | med | shipped: scan fetches each recommended artist's top tracks (Spotify/Deezer), resolving the artist id by name-search when the similar-artist row lacks one — guarded by a strict name-match so it never pulls the wrong artist. bounded (top 20 recs), per-call guarded, fail-soft to the pool. iTunes has no top-tracks API → pool-only there. needs a live scan to populate. | +| **5** | **genre-affinity boost** | med | low | we already compute your genre breakdown. boost recs whose genres match your top genres → tighter taste alignment. pure scoring add. | +| **6** | **adventurousness dial** | med | low | the ranker already supports `min_seed_count` (consensus floor). expose it as a "Safe ↔ Adventurous" slider on the row. | +| **7** | **diversity pass** | low-med | low | avoid 40 recs all orbiting your single heaviest seed — cap picks-per-seed so the row spans your taste. | + +the core is built to absorb all of these without re-plumbing — `similarity_from_rank`, `build_recency_weighted_seeds`, and the scoring formula are all pure + tested. + +--- + +## full discover-page generator audit (every soulsync-built row, excluding last.fm + listenbrainz) + +how each one is generated today, and whether it can be elevated. "clear win" = safe + additive. "product call" = needs your decision (changes the row's character). + +### curated (built during the scan, then hydrated) +- **Fresh Tape / Release Radar** — new releases from watchlist+similar artists. **FIXED tonight** (see above). one more *clear win* available: hydration silently drops any curated id no longer in the discovery pool — could fall back to the stored `track_data_json` blob so the row can't shrink at read time. +- **The Archives / Discovery Weekly** — strong already. nice 3-tier popularity split + serendipity scoring (boost never-played artists, penalize overplayed). same hydration-drop caveat as Fresh Tape; same cheap fallback fix. +- **Seasonal Mix** — cleanest of the bunch. hydrates from a dedicated `seasonal_tracks` table (carries its own data), so it doesn't suffer the pool-drop problem. no bug. + +### discovery-pool generators (live queries) +- **Popular Picks** — ranks by popularity DESC. solid. only nit: on iTunes (no popularity scale) it silently degrades to random — indistinguishable from Shuffle there. UI-label thing at most. +- **Hidden Gems** — *clear win*. currently `ORDER BY RANDOM()` over low-popularity tracks — so it's "random obscure," not "*best* obscure." a light ranking (popularity just under the threshold, or genre-affinity to you) would make it feel curated instead of arbitrary. (a deeper *product call*: add personalization like Archives has — bigger lift, changes its "pure underground" character.) +- **Genre Playlists** — good. pushes the genre match into SQL. `RANDOM()` ordering is fine for a browse; a popularity/affinity tiebreak (*clear win*) would make thin genres feel less arbitrary. +- **Discovery Shuffle** — random by design, correct. only possible add: exclude tracks already shown in other rows this refresh (needs a cross-section seen-set — medium plumbing). +- **Time Machine (by decade)** — *clear win, low risk*: decades are hardcoded, so a modern-only library shows 7 decade tabs, 5 empty. filter the tabs to decades that actually have pool data. +- **Daily Mix** — the weakest row. the "50% your library" half permanently returns nothing (library tracks have no source ids to play), so each Daily Mix is really just a relabeled Genre Playlist. real fix = backfill source ids into library rows (*schema-level, higher risk*) — worth a dedicated pass, not a quick tweak. also silently falls back to "top artists as pseudo-genres" when genre data is missing → "Daily Mix 1" becomes mislabeled artist-radio. gate/label that (*clear win*). + +### cross-cutting +- **hydration fragility** (Fresh Tape + Archives): both depend on curated ids still living in the pool at read time; misses are dropped silently. Seasonal already solved this with a dedicated table. giving the two spotify-style rows the same data-blob fallback is the single most robust cross-cutting fix. low risk, clear win. +- **RANDOM-ordering pattern** (Hidden Gems, Shuffle, Genre, Decade): intentional for variety, but leaves quality signal on the table for the non-shuffle rows. adding a light ranking pass to Hidden Gems + Genre is the biggest "best-in-class" lever after tonight's work. + +want me to take any of these? the Hidden Gems ranking + Time Machine empty-decade filter + the Fresh Tape/Archives hydration fallback are all safe, additive, same-shape-as-tonight wins i can knock out next. diff --git a/RELEASE_2.7.9_discord.md b/RELEASE_2.7.9_discord.md new file mode 100644 index 00000000..80651c39 --- /dev/null +++ b/RELEASE_2.7.9_discord.md @@ -0,0 +1,13 @@ +**SoulSync 2.7.9** is out 🎉 a big one. + +🎚️ **Best-quality downloads** — downloads now follow a ranked quality profile you drag to order (FLAC 24/192 → mp3). best-quality mode grabs the highest-quality copy across *every* source; priority mode gets an opt-in rank-based order toggle. quarantine is folded into the Downloads page + safer imports (AcoustID fail-closed, silence/truncation guards). + +🎧 **Discover got smart** — "Based On Your Listening" ranks artists from who you *actually play*, and "Your Listening Mix" is a playable track playlist of their top tracks (works on any source). Fresh Tape fills properly now. + +⚡ **Wing It Pool** — a new spot next to Discovery Pool to review + re-match the tracks Wing It guessed at (they used to be invisible). + +🔁 **Auto-Sync redesign** — the scheduling board is now clean horizontal lanes instead of a side-scrolling column wall. + +🐛 **Fixes** — multi-disc albums no longer show disc-2 as "missing" (#927), playlists no longer stuck on "Never Synced" (#925), and tracks can't import while quarantined (#928). thanks @ramonskie + @nick2000713 🙏 + +⚠️ **re-scan your library once** so the multi-disc fix can backfill disc numbers on existing tracks. enjoy! 🎶 diff --git a/config/settings.py b/config/settings.py index fe03096e..71e5428e 100644 --- a/config/settings.py +++ b/config/settings.py @@ -705,6 +705,12 @@ class ConfigManager: }, "import": { "staging_path": "./Staging", + # Master toggle for quality-filtering on import. On by default: + # downloaded files that don't meet the quality profile are + # quarantined instead of imported (same gate the download + # pipeline uses). Off → import everything regardless of quality; + # the library Quality Upgrade Scanner still flags them. + "quality_filter_enabled": True, "replace_lower_quality": False, # Use the top Staging folder as the artist (Artist/Album layouts, # mixtapes). On by default to preserve the long-standing import diff --git a/core/amazon_download_client.py b/core/amazon_download_client.py index 18e4bad1..7b0c398d 100644 --- a/core/amazon_download_client.py +++ b/core/amazon_download_client.py @@ -32,6 +32,8 @@ from config.settings import config_manager from core.amazon_client import AmazonClient, AmazonClientError from core.download_plugins.base import DownloadSourcePlugin from core.download_plugins.types import AlbumResult, DownloadStatus, TrackResult +from core.quality.model import AudioQuality +from core.quality.source_map import quality_tier_for_source from utils.logging_config import get_logger logger = get_logger("amazon_download_client") @@ -76,9 +78,12 @@ class AmazonDownloadClient(DownloadSourcePlugin): if download_path is None: download_path = config_manager.get("soulseek.download_path", "./downloads") self.download_path = Path(download_path) - self.download_path.mkdir(parents=True, exist_ok=True) + try: + self.download_path.mkdir(parents=True, exist_ok=True) + except OSError as e: + logger.warning(f"Could not verify download path {self.download_path}: {e}") - self._quality = config_manager.get("amazon_download.quality", "flac") + self._quality = quality_tier_for_source("amazon", default="flac") self._allow_fallback = config_manager.get("amazon_download.allow_fallback", True) self._client = AmazonClient(preferred_codec=self._quality) @@ -133,11 +138,17 @@ class AmazonDownloadClient(DownloadSourcePlugin): album_map: Dict[str, AlbumResult] = {} album_order: List[str] = [] preferred = self._client.preferred_codec + # Search results only carry the codec (real sample_rate arrives at + # stream time). Claim the format honestly — FLAC for the lossless + # codec, lossy otherwise — so audio_quality derives a real format + # instead of the display label ("Lossless"), and the post-download + # probe pins the actual sample_rate/bit_depth. + amazon_q = AudioQuality(format='flac' if _codec_key(preferred) == 'flac' else 'aac') for item in items: quality = _quality_label(preferred) if item.is_track: - track_results.append(TrackResult( + tr = TrackResult( username="amazon", filename=f"{item.asin}||{item.artist_name} - {item.title}", size=0, @@ -155,7 +166,9 @@ class AmazonDownloadClient(DownloadSourcePlugin): "album_asin": item.album_asin, "isrc": item.isrc, }, - )) + ) + tr.set_quality(amazon_q) + track_results.append(tr) elif item.is_album: album_asin = item.album_asin or item.asin if album_asin not in album_map: @@ -173,6 +186,7 @@ class AmazonDownloadClient(DownloadSourcePlugin): title=item.title, album=item.album_name, ) + placeholder.set_quality(amazon_q) album_map[album_asin] = AlbumResult( username="amazon", album_path=album_asin, diff --git a/core/deezer_download_client.py b/core/deezer_download_client.py index 165adcfd..e1830f64 100644 --- a/core/deezer_download_client.py +++ b/core/deezer_download_client.py @@ -21,6 +21,7 @@ from typing import Any, Dict, List, Optional, Tuple import requests from core.download_plugins.types import AlbumResult, DownloadStatus, TrackResult +from core.quality.source_map import quality_from_deezer, quality_tier_for_source from utils.logging_config import get_logger logger = get_logger("deezer_download") @@ -92,7 +93,10 @@ class DeezerDownloadClient(DownloadSourcePlugin): if download_path is None: download_path = config_manager.get('soulseek.download_path', './downloads') self.download_path = Path(download_path) - self.download_path.mkdir(parents=True, exist_ok=True) + try: + self.download_path.mkdir(parents=True, exist_ok=True) + except OSError as e: + logger.warning(f"Could not verify download path {self.download_path}: {e}") # Engine reference is populated by set_engine() at registration # time. None until orchestrator wires the registry. @@ -119,7 +123,7 @@ class DeezerDownloadClient(DownloadSourcePlugin): self._authenticated = False # Quality preference - self._quality = config_manager.get('deezer_download.quality', 'flac') + self._quality = quality_tier_for_source('deezer', default='flac') # Try to authenticate on init if ARL is configured arl = config_manager.get('deezer_download.arl', '') @@ -597,7 +601,7 @@ class DeezerDownloadClient(DownloadSourcePlugin): bitrate = 128 quality = 'mp3' - results.append(TrackResult( + tr = TrackResult( username='deezer_dl', filename=f"{track_id}||{artist} - {title}", size=est_size, @@ -611,7 +615,10 @@ class DeezerDownloadClient(DownloadSourcePlugin): title=title, album=album, track_number=item.get('track_position'), - )) + ) + # Stamp CD-quality FLAC (16/44.1) so lossless ranks correctly. + tr.set_quality(quality_from_deezer(self._quality)) + results.append(tr) logger.info(f"Deezer search for '{query}' returned {len(results)} results") return results, [] diff --git a/core/discovery/listening_recommendations.py b/core/discovery/listening_recommendations.py index 9af7b417..8769dbb5 100644 --- a/core/discovery/listening_recommendations.py +++ b/core/discovery/listening_recommendations.py @@ -46,6 +46,81 @@ def _get(row: object, attr: str): return getattr(row, attr, None) +def choose_mix_fetch_source(active_source: object, active_can_fetch: bool) -> str: + """Pick which source to fetch the "Listening Mix" top tracks from. + + The mix is a list of (artist, title) pairs acquired via Soulseek, so the fetch source need + NOT match the user's active metadata source. Use the active source when it can fetch top + tracks itself (Spotify/Deezer); otherwise fall back to Deezer, whose public ``artist/{id}/top`` + needs no auth and is available to every user — so iTunes / Discogs / MusicBrainz users still + get a full mix without switching sources. Pure. + """ + if str(active_source or "").lower() in ("spotify", "deezer") and active_can_fetch: + return str(active_source).lower() + return "deezer" + + +def names_match(a: object, b: object) -> bool: + """Strict artist-name equality after stripping case + non-alphanumerics. + + Used to verify a name-search result before fetching that artist's top tracks, so the + "Listening Mix" can never pull the WRONG artist's songs (e.g. a same-name act). Exact + alphanumeric match: "Tyler, The Creator" == "Tyler The Creator", but "Drake" != "Drake Bell". + Pure. + """ + def _alnum(x: object) -> str: + return "".join(ch for ch in str(x or "").lower() if ch.isalnum()) + na, nb = _alnum(a), _alnum(b) + return bool(na) and na == nb + + +def similarity_from_rank(rank: object, max_rank: int = 10) -> float: + """Turn a stored ``similarity_rank`` (1 = most similar … 10 = least) into a 0–1 weight. + + SoulSync stores each ``(seed → similar)`` edge with a 1–10 rank (``1`` is the closest + match). The ranker multiplies this into the score so a seed's *closest* matches count + for more than its long-tail ones. Linear decay over the documented range: rank 1 → 1.0, + rank 5 → 0.6, rank 10 → 0.1, with a 0.1 floor so a far match still contributes. A + missing/garbage rank falls back to 1.0 (treat as "no rank info, full weight"). Pure. + """ + try: + r = int(rank) + except (TypeError, ValueError): + return 1.0 + floor = round(1.0 / max_rank, 4) + if r <= 1: + return 1.0 + if r >= max_rank: + return floor + return round((max_rank - r + 1) / max_rank, 4) + + +def build_recency_weighted_seeds( + top_artists: Sequence[dict], + recent_play_counts: Optional[Dict[str, float]] = None, + *, + recency_factor: float = 1.5, +) -> List[dict]: + """Blend lifetime + recent play counts into seed weights — "what you're into NOW". + + ``weight = lifetime_plays + recency_factor × recent_plays``. An artist you've played a + lot *recently* outranks one you played a lot years ago, so the recommendations track + your current taste instead of your all-time history. ``recency_factor`` is the dial + (0 = pure lifetime). Returns ``[{'name', 'weight'}]`` for :func:`rank_recommended_artists`. + Pure — the caller supplies both play-count maps from the listening history. + """ + recent = {_norm(k): _positive_float(v, 0.0) for k, v in (recent_play_counts or {}).items()} + out: List[dict] = [] + for a in top_artists or (): + name = str(a.get("name") or "").strip() + if not name: + continue + lifetime = _positive_float(a.get("play_count", a.get("weight", 1.0))) + boost = recency_factor * recent.get(_norm(name), 0.0) + out.append({"name": name, "weight": lifetime + boost}) + return out + + def group_similars_by_seed( seeds: Sequence[dict], similar_rows: Sequence, @@ -53,14 +128,21 @@ def group_similars_by_seed( *, source_id_attr: str = "source_artist_id", similar_name_attr: str = "similar_artist_name", + rank_attr: Optional[str] = None, ) -> Dict[str, List[dict]]: - """Reshape flat ``similar_artists`` rows into ``{seed_name_lower: [{'name': similar}]}``. + """Reshape flat ``similar_artists`` rows into ``{seed_name_lower: [{'name', 'score'?}]}``. The stored rows key the similar artist by the SEED's source id (``source_artist_id``), not its name, so :func:`rank_recommended_artists` can't consume them directly. This resolves each row's source id to a name via ``id_to_name`` (``{source_artist_id: artist_name}`` for the library, built by the caller) and keeps only rows that resolve to one of the ``seeds``. Rows may be dataclass objects or dicts. Pure — no I/O. + + ``id_to_name`` MUST be keyed by whatever id the edges actually store — for SoulSync that + is the artist's SOURCE id (Spotify/iTunes/Deezer/MusicBrainz), NOT the internal row id. + When ``rank_attr`` is given, each row's rank is converted via :func:`similarity_from_rank` + and carried as ``score`` so closer matches weigh more; without it every similar comes out + score-less (the ranker then treats similarity as 1.0 — original behavior). """ seed_names = {_norm(s.get("name")) for s in seeds} seed_names.discard("") @@ -72,8 +154,12 @@ def group_similars_by_seed( if not seed_name or seed_name not in seed_names: continue sim_name = str(_get(row, similar_name_attr) or "").strip() - if sim_name: - out.setdefault(seed_name, []).append({"name": sim_name}) + if not sim_name: + continue + entry = {"name": sim_name} + if rank_attr is not None: + entry["score"] = similarity_from_rank(_get(row, rank_attr)) + out.setdefault(seed_name, []).append(entry) return out @@ -197,8 +283,57 @@ def aggregate_candidate_tracks( return out[:limit] +def to_mix_track(track: object, source: str) -> Optional[dict]: + """Shape one source "top tracks" API dict into the flat dict the Discover compact + playlist row renders + syncs (the "Listening Mix" #913 playlist). + + Spotify's ``artist_top_tracks`` and Deezer's ``get_artist_top_tracks`` both return the + same Spotify-shape object (``id, name, artists[], album{name,images[]}, duration_ms``). + This flattens that into the renderer's field names (``track_name/artist_name/album_name/ + album_cover_url/duration_ms``), keeps the original under ``track_data_json`` for sync, and + sets the source-specific id field. Returns None for anything without a usable id/title so + the caller can filter. A ``name`` key is kept so :func:`aggregate_candidate_tracks` can + dedup by title. Pure — no I/O. + """ + if not isinstance(track, dict): + return None + tid = track.get("id") + name = str(track.get("name") or "").strip() + if not tid or not name: + return None + artists = track.get("artists") or [] + artist_name = "" + if artists and isinstance(artists[0], dict): + artist_name = str(artists[0].get("name") or "").strip() + album = track.get("album") if isinstance(track.get("album"), dict) else {} + album_name = str(album.get("name") or "").strip() + images = album.get("images") or [] + cover = images[0].get("url") if images and isinstance(images[0], dict) else None + out = { + "track_id": str(tid), + "name": name, # for aggregate_candidate_tracks dedup + "track_name": name, # for the renderer + "artist_name": artist_name, + "album_name": album_name, + "album_cover_url": cover, + "duration_ms": track.get("duration_ms") or 0, + "track_data_json": track, # full payload for sync/download + "source": source, + } + id_field = {"spotify": "spotify_track_id", "deezer": "deezer_track_id", + "itunes": "itunes_track_id"}.get(source) + if id_field: + out[id_field] = str(tid) + return out + + __all__ = [ "RecommendedArtist", + "choose_mix_fetch_source", + "names_match", + "similarity_from_rank", + "build_recency_weighted_seeds", + "to_mix_track", "group_similars_by_seed", "rank_recommended_artists", "aggregate_candidate_tracks", diff --git a/core/discovery/quality_scanner.py b/core/discovery/quality_scanner.py index 7c689a38..4d2a2bdf 100644 --- a/core/discovery/quality_scanner.py +++ b/core/discovery/quality_scanner.py @@ -21,7 +21,11 @@ from core.metadata.registry import get_client_for_source from core.metadata.types import Album from core.wishlist.payloads import ensure_wishlist_track_format -logger = logging.getLogger(__name__) +# Use the project logger namespace ("soulsync.*") so the scanner's progress and +# diagnostics actually surface in the app log — plain getLogger(__name__) lands +# under "core.discovery.quality_scanner", which the app log view doesn't show. +from utils.logging_config import get_logger +logger = get_logger("discovery.quality_scanner") # Per-source typed converter dispatch — same registry pattern as diff --git a/core/download_engine/engine.py b/core/download_engine/engine.py index 8aaa07e8..de1aca38 100644 --- a/core/download_engine/engine.py +++ b/core/download_engine/engine.py @@ -25,6 +25,7 @@ big-bang switchover. from __future__ import annotations +import asyncio import threading from typing import Any, Dict, Iterator, List, Optional, Tuple @@ -391,6 +392,16 @@ class DownloadEngine: (tracks, albums) tuple, or ``([], [])`` when every source in the chain is exhausted. + 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). """ @@ -406,9 +417,10 @@ class DownloadEngine: try: logger.info(f"Trying {source_name} (priority {i+1}): {query}") tracks, albums = await plugin.search(query, timeout, progress_callback) - if tracks: - logger.info(f"{source_name} found {len(tracks)} tracks") - return (tracks, albums) + if not tracks: + continue + logger.info(f"{source_name} found {len(tracks)} tracks") + return (tracks, albums) except Exception as e: logger.warning(f"{source_name} search failed: {e}") @@ -418,6 +430,75 @@ class DownloadEngine: ) return ([], []) + async def search_all_sources(self, query: str, source_chain, + timeout=None, progress_callback=None, + exclude_sources=None): + """Best-quality mode: pool RAW tracks from EVERY configured source in + ``source_chain`` instead of stopping at the first satisfying one. + + Unlike :meth:`search_with_fallback`, no source short-circuits the + search — the caller (orchestrator/worker) ranks the combined pool + best→worst by actual audio quality. ``exclude_sources`` drops sources + whose per-source retry budget is already spent (so their candidates + never re-enter the pool). Unconfigured / unregistered / raising sources + are skipped exactly like the fallback path. Returns + ``(combined_tracks, combined_albums)``. + """ + excluded = {s.lower() for s in (exclude_sources or []) if s} + pooled_tracks = [] + pooled_albums = [] + # Per-source contribution for an honest pool log — e.g. a release-level + # source like usenet/torrent that returns nothing for a track-title + # query should read "usenet=0", not silently hide behind the chain name. + contributions = [] + + # Decide which sources to actually query, recording why the rest were + # skipped. Searches then run CONCURRENTLY so the pool waits only for the + # slowest source (e.g. usenet/Prowlarr, which can be slow) rather than + # the sum of every source's latency. + to_search = [] # (source_name, plugin) + for source_name in source_chain: + if source_name.lower() in excluded: + contributions.append(f"{source_name}=excluded") + continue + plugin = self._plugins.get(source_name) + if plugin is None: + logger.info(f"Skipping {source_name} (not available)") + contributions.append(f"{source_name}=unavailable") + continue + if hasattr(plugin, 'is_configured') and not plugin.is_configured(): + logger.info(f"Skipping {source_name} (not configured)") + contributions.append(f"{source_name}=unconfigured") + continue + to_search.append((source_name, plugin)) + + async def _one(plugin): + return await plugin.search(query, timeout, progress_callback) + + results = await asyncio.gather( + *[_one(plugin) for _, plugin in to_search], + return_exceptions=True, + ) + + for (source_name, _), result in zip(to_search, results, strict=True): + if isinstance(result, Exception): + logger.warning(f"{source_name} search failed: {result}") + contributions.append(f"{source_name}=error") + continue + tracks, albums = result + n = len(tracks) if tracks else 0 + if tracks: + pooled_tracks.extend(tracks) + if albums: + pooled_albums.extend(albums) + contributions.append(f"{source_name}={n}") + + logger.info( + "Best-quality pool: %d candidates [%s] for: %s", + len(pooled_tracks), ', '.join(contributions), query, + ) + return (pooled_tracks, pooled_albums) + async def download_with_fallback(self, username: str, filename: str, file_size: int, source_chain) -> Optional[str]: """Try each source in ``source_chain`` until one accepts the diff --git a/core/download_orchestrator.py b/core/download_orchestrator.py index 1984c0a6..5e08b496 100644 --- a/core/download_orchestrator.py +++ b/core/download_orchestrator.py @@ -28,6 +28,7 @@ from config.settings import config_manager from core.download_engine import DownloadEngine from core.download_plugins.registry import DownloadPluginRegistry, build_default_registry from core.download_plugins.types import TrackResult, AlbumResult, DownloadStatus +from core.quality.selection import load_search_mode logger = get_logger("download_orchestrator") @@ -102,12 +103,14 @@ class DownloadOrchestrator: deezer_dl = self.client('deezer_dl') if deezer_arl and deezer_dl: deezer_dl.reconnect(deezer_arl) - deezer_dl._quality = config_manager.get('deezer_download.quality', 'flac') + from core.quality.source_map import quality_tier_for_source + deezer_dl._quality = quality_tier_for_source('deezer', default='flac') # Reload Amazon quality preference (T2Tunes needs no reconnect — public proxy) amazon = self.client('amazon') if amazon: - quality = config_manager.get('amazon_download.quality', 'flac') + from core.quality.source_map import quality_tier_for_source + quality = quality_tier_for_source('amazon', default='flac') amazon._quality = quality amazon._allow_fallback = config_manager.get('amazon_download.allow_fallback', True) if hasattr(amazon, '_client') and amazon._client: @@ -140,7 +143,10 @@ class DownloadOrchestrator: continue if hasattr(client, 'download_path') and client.download_path != new_path: client.download_path = new_path - client.download_path.mkdir(parents=True, exist_ok=True) + try: + client.download_path.mkdir(parents=True, exist_ok=True) + except OSError as e: + logger.warning(f"Could not verify download path {new_path}: {e}") # YouTube also caches path in yt-dlp opts if hasattr(client, 'download_opts') and 'outtmpl' in client.download_opts: client.download_opts['outtmpl'] = str(new_path / '%(title)s.%(ext)s') @@ -342,6 +348,11 @@ class DownloadOrchestrator: if not chain: logger.warning("Hybrid search exhausted: no eligible sources after exclusion filter") return [], [] + if load_search_mode() == 'best_quality': + logger.info(f"Best-quality search ({' → '.join(chain)}): {query}") + return await self.engine.search_all_sources( + query, chain, timeout, progress_callback, + ) logger.info(f"Hybrid search ({' → '.join(chain)}): {query}") return await self.engine.search_with_fallback(query, chain, timeout, progress_callback) @@ -412,9 +423,17 @@ class DownloadOrchestrator: if scored: scored.sort(key=lambda x: x._match_confidence, reverse=True) - filtered_results = scored + # Match filter done (right track); now prefer the best quality + # among the confidence-passing survivors so streaming isn't + # quality-blind like Soulseek already isn't. Stable ranking + # keeps confidence order within an equal quality tier; the + # `or scored` fail-safe never leaves us with nothing to try. + from core.quality.selection import rank_for_profile + ranked, _ = rank_for_profile(scored) + filtered_results = ranked or scored logger.info(f"Streaming validation: {len(scored)}/{len(tracks)} passed " - f"(best: {scored[0]._match_confidence:.2f})") + f"(best: {scored[0]._match_confidence:.2f}, " + f"quality pick: {filtered_results[0].audio_quality.label()})") else: logger.warning(f"No streaming results passed validation for: {query}") return None diff --git a/core/download_plugins/types.py b/core/download_plugins/types.py index 59d52913..5c364da8 100644 --- a/core/download_plugins/types.py +++ b/core/download_plugins/types.py @@ -13,10 +13,11 @@ import from a neutral package per Cin's contract-first standard. from __future__ import annotations -from dataclasses import dataclass +from dataclasses import dataclass, field from typing import Any, Dict, List, Optional from core.imports.filename import parse_filename_metadata +from core.quality.model import AudioQuality @dataclass @@ -32,6 +33,36 @@ class SearchResult: upload_speed: int queue_length: int result_type: str = "track" # "track" or "album" + # Rich quality metadata — populated by sources that provide it. + # None means "unknown", not "absent". + sample_rate: Optional[int] = None # Hz (e.g. 44100, 96000, 192000) + bit_depth: Optional[int] = None # bits per sample (16, 24) + + @property + def audio_quality(self) -> AudioQuality: + """Unified quality descriptor derived from this result's fields.""" + return AudioQuality( + format=self.quality.lower() if self.quality else 'unknown', + bitrate=self.bitrate, + sample_rate=self.sample_rate, + bit_depth=self.bit_depth, + ) + + def set_quality(self, aq: AudioQuality) -> None: + """Merge a mapped :class:`AudioQuality` onto this result's fields. + + Used by streaming sources to stamp their claimed tier (Tidal/HiFi + tier strings, Qobuz API values, …) so ``audio_quality`` ranks + correctly. Mapper-provided fields win; a ``None`` from the mapper + leaves any already-reported value (e.g. a probed bitrate) intact. + """ + self.quality = aq.format + if aq.bitrate is not None: + self.bitrate = aq.bitrate + if aq.sample_rate is not None: + self.sample_rate = aq.sample_rate + if aq.bit_depth is not None: + self.bit_depth = aq.bit_depth @property def quality_score(self) -> float: @@ -127,6 +158,19 @@ class AlbumResult: queue_length: int = 0 result_type: str = "album" + @property + def audio_quality(self) -> AudioQuality: + """Unified quality descriptor derived from dominant track quality.""" + sample_rates = [t.sample_rate for t in self.tracks if t.sample_rate] + bit_depths = [t.bit_depth for t in self.tracks if t.bit_depth] + bitrates = [t.bitrate for t in self.tracks if t.bitrate] + return AudioQuality( + format=self.dominant_quality.lower() if self.dominant_quality else 'unknown', + bitrate=max(bitrates) if bitrates else None, + sample_rate=max(sample_rates) if sample_rates else None, + bit_depth=max(bit_depths) if bit_depths else None, + ) + @property def quality_score(self) -> float: """Calculate album quality score based on dominant quality and track count""" diff --git a/core/downloads/cancel.py b/core/downloads/cancel.py index b375f8fa..d8d0b820 100644 --- a/core/downloads/cancel.py +++ b/core/downloads/cancel.py @@ -83,9 +83,26 @@ def clear_completed_local() -> int: """ cleared = 0 with tasks_lock: + # Protect tasks belonging to a still-active batch. A batch is "active" + # while any of its queued tasks is non-terminal (still searching / + # downloading / queued / post-processing). Pruning a batch's completed + # or failed tasks mid-run would yank them out of the Downloads page — + # and failed/cancelled rows aren't recoverable from library_history — + # so the user would never see them until the batch ended. Keep the whole + # active batch intact; it gets cleaned by a later run once it finishes. + protected_task_ids: set = set() + for batch in download_batches.values(): + queue = batch.get('queue', []) if isinstance(batch, dict) else [] + batch_active = any( + download_tasks.get(tid, {}).get('status') not in _TERMINAL_STATUSES + for tid in queue if tid in download_tasks + ) + if batch_active: + protected_task_ids.update(queue) + task_ids_to_remove = [ tid for tid, task in download_tasks.items() - if task.get('status') in _TERMINAL_STATUSES + if task.get('status') in _TERMINAL_STATUSES and tid not in protected_task_ids ] for tid in task_ids_to_remove: del download_tasks[tid] diff --git a/core/downloads/candidates.py b/core/downloads/candidates.py index 1edca30d..1dfa231d 100644 --- a/core/downloads/candidates.py +++ b/core/downloads/candidates.py @@ -47,6 +47,55 @@ from core.runtime_state import ( logger = logging.getLogger(__name__) +def _priority_sort_key(r): + """Today's confidence-first key: never download a high-quality WRONG file.""" + return ( + getattr(r, 'confidence', 0) or 0, + getattr(r, 'quality_score', 0) or 0, + getattr(r, 'upload_speed', 0) or 0, + -(getattr(r, 'queue_length', 0) or 0), + getattr(r, 'free_upload_slots', 0) or 0, + getattr(r, 'size', 0) or 0, + ) + + +def _quality_first_sort_key(r, targets): + """Best-quality key: the user's profile quality rank dominates; all the + priority-mode signals (confidence, speed, …) become tiebreakers. + + Every candidate reaching this point already passed match filtering, so it + is "correct enough" — ordering by quality among correct candidates is safe. + Candidates with no usable quality info, or that match no target, sort last + (never dropped). Lower target index = better target, so it's negated to fit + the descending (reverse=True) sort. + """ + from core.quality.model import rank_candidate + + aq = getattr(r, 'audio_quality', None) + if aq is None or not targets: + target_idx, tier = (len(targets) if targets else 0), 0.0 + else: + try: + target_idx, tier = rank_candidate(aq, targets) + except Exception: + target_idx, tier = len(targets), 0.0 + return (-target_idx, tier) + _priority_sort_key(r) + + +def order_candidates(candidates, *, quality_first=False, targets=None): + """Return *candidates* ordered best-first for the download walk. + + ``quality_first=False`` (priority mode) → confidence-first, byte-for-byte + today's behaviour. ``quality_first=True`` (best-quality mode) → the user's + profile quality rank dominates, confidence/peer signals break ties. + """ + if quality_first: + key = lambda r: _quality_first_sort_key(r, targets or []) + else: + key = _priority_sort_key + return sorted(candidates, key=key, reverse=True) + + @dataclass class CandidatesDeps: """Bundle of cross-cutting deps the candidate-fallback logic needs.""" @@ -59,25 +108,25 @@ class CandidatesDeps: on_download_completed: Callable -def attempt_download_with_candidates(task_id, candidates, track, batch_id=None, deps: CandidatesDeps = None): +def attempt_download_with_candidates(task_id, candidates, track, batch_id=None, + deps: CandidatesDeps = None, *, + quality_first=False, quality_targets=None): """ Attempts to download with fallback candidate logic (matches GUI's retry_parallel_download_with_fallback). Returns True if successful, False if all candidates fail. + + ``quality_first`` (best-quality search mode) orders the walk by the user's + profile quality rank instead of confidence-first; ``quality_targets`` is the + profile target list used for that ranking. Defaults preserve priority-mode + behaviour exactly. """ - # Sort candidates by match confidence first, then peer quality. Upstream - # Soulseek validation already considers peer speed/slots/queue when scores - # are close; preserve that signal here instead of flattening ties back to - # arbitrary slskd response order. - candidates.sort( - key=lambda r: ( - getattr(r, 'confidence', 0) or 0, - getattr(r, 'quality_score', 0) or 0, - getattr(r, 'upload_speed', 0) or 0, - -(getattr(r, 'queue_length', 0) or 0), - getattr(r, 'free_upload_slots', 0) or 0, - getattr(r, 'size', 0) or 0, - ), - reverse=True, + # Sort candidates. Priority mode: confidence-first, then peer quality — + # upstream Soulseek validation already considers peer speed/slots/queue when + # scores are close; preserve that signal instead of flattening ties back to + # arbitrary slskd response order. Best-quality mode: profile quality rank + # dominates (all candidates here already passed match filtering). + candidates = order_candidates( + candidates, quality_first=quality_first, targets=quality_targets, ) with tasks_lock: diff --git a/core/downloads/lifecycle.py b/core/downloads/lifecycle.py index cd7e38fc..8ad2d773 100644 --- a/core/downloads/lifecycle.py +++ b/core/downloads/lifecycle.py @@ -26,6 +26,7 @@ Lifted verbatim from web_server.py. Dependencies injected via from __future__ import annotations import logging +import os import shutil import time import traceback @@ -44,6 +45,27 @@ from core.runtime_state import ( logger = logging.getLogger(__name__) +# A task that has been in 'post_processing' longer than this is treated as stuck. +# Post-processing (AcoustID + quality + import) is serialized, so a large batch +# legitimately backs up — keep this generous so genuinely-slow imports aren't +# cut off mid-flight (the old 5-min cutoff falsely "completed" queued tasks). +_POST_PROCESSING_STUCK_TIMEOUT = 1800 # 30 minutes + + +def _resolve_stuck_post_processing_status(task: dict) -> str: + """Decide the terminal status for a task stuck in post_processing. + + Only call it 'completed' if the import actually produced a file on disk + (``final_file_path`` is set at the end of successful post-processing). Without + a real file, force-completing is a lie — the task shows as a downloaded track + that isn't anywhere. Mark those 'failed' so they're retryable and honest. + """ + final_path = task.get('final_file_path') + if final_path and os.path.exists(final_path): + return 'completed' + return 'failed' + + def _safe_batch_dirname(batch_id: str) -> str: safe = ''.join(ch if ch.isalnum() or ch in ('-', '_') else '_' for ch in str(batch_id or 'batch')) return safe or 'batch' @@ -435,9 +457,15 @@ def on_download_completed(batch_id: str, task_id: str, success: bool, deps: Life retrying_count += 1 elif task_status == 'post_processing': task_age = current_time - task.get('status_change_time', current_time) - if task_age > 300: # 5 minutes (post-processing should be fast) - logger.info(f"⏰ [Stuck Detection] Task {queue_task_id} stuck in post_processing for {task_age:.0f}s - forcing completion") - task['status'] = 'completed' # Assume it worked if file verification is taking too long + if task_age > _POST_PROCESSING_STUCK_TIMEOUT: + new_status = _resolve_stuck_post_processing_status(task) + if new_status == 'completed': + logger.info(f"⏰ [Stuck Detection] Task {queue_task_id} stuck in post_processing for {task_age:.0f}s but file exists — completing") + task['status'] = 'completed' + else: + logger.warning(f"⏰ [Stuck Detection] Task {queue_task_id} stuck in post_processing for {task_age:.0f}s with no output file — marking failed") + task['status'] = 'failed' + task['error_message'] = 'Post-processing timed out without producing a file' finished_count += 1 else: retrying_count += 1 @@ -667,9 +695,15 @@ def check_batch_completion_v2(batch_id: str, deps: LifecycleDeps) -> Optional[bo retrying_count += 1 elif task_status == 'post_processing': task_age = current_time - task.get('status_change_time', current_time) - if task_age > 300: # 5 minutes (post-processing should be fast) - logger.info(f"⏰ [Stuck Detection V2] Task {task_id} stuck in post_processing for {task_age:.0f}s - forcing completion") - task['status'] = 'completed' # Assume it worked if file verification is taking too long + if task_age > _POST_PROCESSING_STUCK_TIMEOUT: + new_status = _resolve_stuck_post_processing_status(task) + if new_status == 'completed': + logger.info(f"⏰ [Stuck Detection V2] Task {task_id} stuck in post_processing for {task_age:.0f}s but file exists — completing") + task['status'] = 'completed' + else: + logger.warning(f"⏰ [Stuck Detection V2] Task {task_id} stuck in post_processing for {task_age:.0f}s with no output file — marking failed") + task['status'] = 'failed' + task['error_message'] = 'Post-processing timed out without producing a file' finished_count += 1 else: retrying_count += 1 diff --git a/core/downloads/monitor.py b/core/downloads/monitor.py index 60ab4585..81a8ef22 100644 --- a/core/downloads/monitor.py +++ b/core/downloads/monitor.py @@ -760,7 +760,8 @@ class WebUIDownloadMonitor: # used_sources keys are formatted as "{username}_{filename}", so startswith is exact. is_tidal = any(s.startswith('tidal_') for s in tried_sources) if is_tidal: - tidal_quality = config_manager.get('tidal_download.quality', 'lossless') + from core.quality.source_map import quality_tier_for_source + tidal_quality = quality_tier_for_source('tidal', default='lossless') allow_fb = config_manager.get('tidal_download.allow_fallback', True) if tidal_quality == 'hires' and not allow_fb: task['error_message'] = ( diff --git a/core/downloads/post_processing.py b/core/downloads/post_processing.py index 2c17e90f..5dbbae97 100644 --- a/core/downloads/post_processing.py +++ b/core/downloads/post_processing.py @@ -171,6 +171,21 @@ def run_post_processing_worker(task_id: str, batch_id: str, deps: PostProcessDep logger.info(f"[Post-Processing] Task {task_id} already completed by stream processor, skipping verification") return + # RACE GUARD: the monitor sets status -> 'post_processing' immediately + # before submitting this worker. If the status is now anything else, the + # browser-poll post-processor already took ownership of this task — e.g. + # it quarantined the file and requeued the next-best candidate (status + # -> 'searching', source identity cleared). Bail WITHOUT marking failed + # or notifying batch completion: otherwise we clobber that in-flight + # retry with a bogus "missing file or source information" failure while a + # parallel attempt is importing the song. + if task['status'] != 'post_processing': + logger.info( + f"[Post-Processing] Task {task_id} no longer in 'post_processing' " + f"(now '{task['status']}') — another path took over, skipping" + ) + return + # Extract file information for verification track_info = task.get('track_info', {}) task_filename = task.get('filename') or track_info.get('filename') diff --git a/core/downloads/status.py b/core/downloads/status.py index f425cceb..8735d4c4 100644 --- a/core/downloads/status.py +++ b/core/downloads/status.py @@ -94,6 +94,10 @@ class StatusDeps: run_async: Optional[Callable] = None on_download_completed: Optional[Callable[[str, str, bool], None]] = None get_persistent_download_history: Optional[Callable[[int], list[dict]]] = None + # Returns ALL library_history rows with verification_status in + # ('unverified', 'force_imported') — no recency limit, so historical + # entries are never buried by the general history tail cap. + get_unverified_download_history: Optional[Callable[[], list[dict]]] = None # Streaming sources the engine fallback applies to. Soulseek goes through @@ -796,6 +800,13 @@ def build_unified_downloads_response(limit: int, deps: StatusDeps) -> dict: 'progress': progress, 'error': task.get('error_message'), 'verification_status': task.get('verification_status'), + # library_history row id (set at import) so the Unverified review + # queue can act on a still-live completed task before it becomes + # a persistent-history row. + 'history_id': task.get('history_id'), + # Real probed audio quality (mutagen-read from the actual file), + # surfaced so the Downloads page can show what was downloaded. + 'quality': task.get('quality') or '', 'retry_info': task.get('retry_info'), 'retry_trigger': task.get('retry_trigger'), 'batch_id': batch_id, @@ -812,6 +823,32 @@ def build_unified_downloads_response(limit: int, deps: StatusDeps) -> dict: 'is_persistent_history': False, }) + # --- Unverified history (unconditional, no limit) --- + # Always load every library_history row that still needs human confirmation + # (verification_status IN ('unverified', 'force_imported')). This is NOT + # gated on len(items) < limit so that historical entries from past batches + # are visible even during a large active batch that would otherwise exhaust + # the limit before the history tail is read. Dedup against live tasks by + # identity so a track currently in post-processing isn't shown twice. + if deps.get_unverified_download_history is not None: + try: + unverified_entries = deps.get_unverified_download_history() or [] + except Exception as exc: + logger.debug("[Downloads] unverified history lookup failed: %s", exc) + unverified_entries = [] + + for entry in unverified_entries: + item = _build_history_download_item(entry) + identity = _download_identity(item.get('title'), item.get('artist'), item.get('album')) + if identity in live_identities: + continue + items.append(item) + live_identities.add(identity) + + # --- General recent-history tail (capped, recency-ordered) --- + # Fills in the completed/verified tail so the full Downloads list looks + # populated. Gated on len(items) < limit so a busy batch doesn't trigger + # an extra DB round-trip when we're already at capacity. if deps.get_persistent_download_history is not None and len(items) < limit: history_limit = min(limit - len(items), _PERSISTENT_HISTORY_TAIL_LIMIT) try: @@ -832,7 +869,14 @@ def build_unified_downloads_response(limit: int, deps: StatusDeps) -> dict: live_identities.add(identity) appended_history += 1 - # Sort: active first (by priority), then by timestamp desc within each group + # Sort: active first (by priority), then by timestamp desc within each group. + # NOTE: the array order is presentation-only — the Downloads page filters + # client-side per tab. What matters is that EVERY live task is present: an + # earlier `items[:limit]` truncation (active-first) starved completed/failed/ + # unverified rows off the end during a busy batch, so those tabs stayed empty + # until the batch drained. `limit` now bounds only the persistent-history + # tail (handled above); live in-memory tasks are always returned in full + # (they're already bounded by the 5-min cleanup automation). items.sort(key=lambda x: (x['priority'], -x['timestamp'])) # Build batch summaries for the batch context panel @@ -860,7 +904,7 @@ def build_unified_downloads_response(limit: int, deps: StatusDeps) -> dict: return { 'success': True, - 'downloads': items[:limit], + 'downloads': items, 'total': len(items), 'batches': batch_summaries, 'timestamp': time.time(), diff --git a/core/downloads/task_worker.py b/core/downloads/task_worker.py index 71e49933..f615de47 100644 --- a/core/downloads/task_worker.py +++ b/core/downloads/task_worker.py @@ -54,6 +54,37 @@ def _cand_user_file(candidate): return getattr(candidate, 'username', None), getattr(candidate, 'filename', None) +def _candidate_ordering(): + """Return ``(quality_first, targets)`` for the active search mode + toggle. + + The candidate walk is ordered by the user's profile quality rank + (best→worst) instead of confidence-first when EITHER: + - best-quality search mode is active (always quality-first), OR + - priority mode and the ``rank_candidates_by_quality`` toggle is on + (opt-in; default off keeps the byte-for-byte confidence-first walk). + + Quality-first ordering also makes the version-mismatch force-import pick + the highest-quality candidate, because that fallback accepts the + first-tried (= best-ordered) quarantined entry. + + Fails closed to confidence-first ordering on any error so a profile/DB + hiccup never blocks a download. See + docs/superpowers/specs/2026-06-14-best-quality-search-mode-design.md. + """ + try: + from core.quality.selection import ( + load_search_mode, + load_profile_targets, + load_rank_candidates_by_quality, + ) + if load_search_mode() == 'best_quality' or load_rank_candidates_by_quality(): + targets, _ = load_profile_targets() + return True, targets + except Exception as exc: + logger.debug("[Modal Worker] quality ordering unavailable: %s", exc) + return False, None + + def _try_cached_candidates(task_id, batch_id, track, deps): """Quarantine-retry fast path: attempt the already-found candidates before re-searching anything. @@ -91,7 +122,10 @@ def _try_cached_candidates(task_id, batch_id, track, deps): f"[Modal Worker] Quarantine retry: trying {len(remaining)} cached " f"candidate(s) before re-searching (task {task_id})" ) - return deps.attempt_download_with_candidates(task_id, remaining, track, batch_id) + _qf, _qt = _candidate_ordering() + return deps.attempt_download_with_candidates( + task_id, remaining, track, batch_id, quality_first=_qf, quality_targets=_qt, + ) def _private_album_bundle_staging_miss_reason(batch_id: Optional[str], deps: Any) -> Optional[str]: @@ -369,6 +403,11 @@ def download_track_worker(task_id: str, batch_id: Optional[str], deps: TaskWorke logger.info(f"[Modal Worker] Generated {len(search_queries)} smart search queries for '{track.name}': {search_queries}") logger.info(f"[Modal Worker] About to start search loop for task {task_id} (track: '{track.name}')") + # Best-quality search mode: the orchestrator already pooled candidates + # across every source for each query, so order the candidate walk by the + # user's profile quality rank (best→worst). Computed once per task. + _best_quality, _quality_targets = _candidate_ordering() + # 2. Sequential Query Search (matches GUI's start_search_worker_parallel logic) search_diagnostics = [] # Track what happened per query for detailed error messages all_raw_results = [] # Collect raw results across queries for candidate review modal @@ -495,7 +534,10 @@ def download_track_worker(task_id: str, batch_id: Optional[str], deps: TaskWorke download_tasks[task_id]['cached_candidates'] = candidates # Try to download with these candidates - success = deps.attempt_download_with_candidates(task_id, candidates, track, batch_id) + success = deps.attempt_download_with_candidates( + task_id, candidates, track, batch_id, + quality_first=_best_quality, quality_targets=_quality_targets, + ) if success: # Download initiated successfully - let the download monitoring system handle completion if batch_id: @@ -529,7 +571,10 @@ def download_track_worker(task_id: str, batch_id: Optional[str], deps: TaskWorke # === HYBRID FALLBACK: If primary source failed, try remaining sources directly === # The orchestrator's hybrid search stops at the first source with results, even if # those results all fail quality filtering. Try remaining sources individually. - if getattr(deps.download_orchestrator, 'mode', '') == 'hybrid': + # + # Best-quality mode already searched EVERY source per query (the pool), so this + # block would only re-search the same sources — skip it there. + if not _best_quality and getattr(deps.download_orchestrator, 'mode', '') == 'hybrid': try: orch = deps.download_orchestrator hybrid_order = getattr(orch, 'hybrid_order', None) or [] diff --git a/core/hifi_client.py b/core/hifi_client.py index 0b380132..f103e136 100644 --- a/core/hifi_client.py +++ b/core/hifi_client.py @@ -36,9 +36,34 @@ import requests as http_requests from utils.logging_config import get_logger from config.settings import config_manager from core.download_plugins.types import TrackResult, AlbumResult, DownloadStatus +from core.quality.source_map import quality_from_tidal_tier, quality_tier_for_source logger = get_logger("hifi_client") +# A media playlist whose total runtime is below this fraction of the track's +# real duration is a preview (some Monochrome instances only have 30s Tidal +# DOWNLOAD access — a 220s track comes back as ~30s of segments + ENDLIST). +_PREVIEW_DURATION_RATIO = 0.85 +_EXTINF_RE = re.compile(r'#EXTINF:\s*([0-9.]+)') + + +def hls_total_seconds(playlist_text: str) -> float: + """Sum the ``#EXTINF`` segment durations in an HLS media playlist.""" + return sum(float(x) for x in _EXTINF_RE.findall(playlist_text or '')) + + +def is_preview_playlist(playlist_s: float, track_s: float, + ratio: float = _PREVIEW_DURATION_RATIO) -> bool: + """True when the playlist runtime is far shorter than the track's real + duration (a preview). Returns False when either duration is unknown, so a + missing reference never false-positives — the post-download audio guard is + the safety net. + """ + if not playlist_s or not track_s or track_s <= 0: + return False + return playlist_s < track_s * ratio + + # HLS quality presets mapping to /trackManifests/ format parameters HLS_QUALITY_MAP = { 'hires': { @@ -232,7 +257,10 @@ class HiFiClient(DownloadSourcePlugin): if download_path is None: download_path = config_manager.get('soulseek.download_path', './downloads') self.download_path = Path(download_path) - self.download_path.mkdir(parents=True, exist_ok=True) + try: + self.download_path.mkdir(parents=True, exist_ok=True) + except OSError as e: + logger.warning(f"Could not verify download path {self.download_path}: {e}") self._instances = [] self._instance_lock = threading.Lock() @@ -697,7 +725,8 @@ class HiFiClient(DownloadSourcePlugin): return init_uri, segment_uris - def _get_hls_manifest(self, track_id: int, quality: str = 'lossless') -> Optional[Dict]: + def _get_hls_manifest(self, track_id: int, quality: str = 'lossless', + expected_duration_s: float = 0) -> Optional[Dict]: q_info = HLS_QUALITY_MAP.get(quality, HLS_QUALITY_MAP['lossless']) formats = q_info['formats'] @@ -751,6 +780,20 @@ class HiFiClient(DownloadSourcePlugin): logger.warning(f"Failed to fetch variant playlist for track {track_id}: {e}") return None + # Preview detection — some instances only have 30s Tidal DOWNLOAD + # access, returning a playlist far shorter than the real track. Decline + # it (and rotate the instance) so the orchestrator falls through to a + # real source instead of fetching a 30s file that gets quarantined. + playlist_s = hls_total_seconds(media_text) + if is_preview_playlist(playlist_s, expected_duration_s): + logger.warning( + f"HiFi manifest for track {track_id} ({quality}) is a " + f"{playlist_s:.0f}s preview of a {expected_duration_s:.0f}s track — " + f"declining this instance" + ) + self._rotate_instance(self._current_instance) + return None + if init_uri: logger.info(f"HiFi HLS manifest for track {track_id}: " f"init segment + {len(segment_uris)} segments ({quality})") @@ -873,13 +916,18 @@ class HiFiClient(DownloadSourcePlugin): loop = asyncio.get_event_loop() tracks = await loop.run_in_executor(None, lambda: self.search_raw(query)) - quality_key = config_manager.get('hifi_download.quality', 'lossless') + quality_key = quality_tier_for_source('hifi', default='lossless') q_info = HLS_QUALITY_MAP.get(quality_key, HLS_QUALITY_MAP['lossless']) + # HiFi is Tidal-backed; stamp the configured tier so the global + # ranker sees real sample_rate/bit_depth, not just 'flac'. + tier_quality = quality_from_tidal_tier(quality_key) + results = [] for t in tracks: try: tr = self._to_track_result(t, q_info) + tr.set_quality(tier_quality) results.append(tr) except Exception as e: logger.debug(f"Skipping track result conversion: {e}") @@ -950,7 +998,7 @@ class HiFiClient(DownloadSourcePlugin): ) def _download_sync(self, download_id: str, track_id: int, display_name: str) -> Optional[str]: - quality_key = config_manager.get('hifi_download.quality', 'lossless') + quality_key = quality_tier_for_source('hifi', default='lossless') chain = ['hires', 'lossless', 'high', 'low'] start = chain.index(quality_key) if quality_key in chain else 1 allow_fallback = config_manager.get('hifi_download.allow_fallback', True) @@ -958,21 +1006,26 @@ class HiFiClient(DownloadSourcePlugin): MIN_AUDIO_SIZE = 100 * 1024 - # Expected track length (for the preview/truncation guards). Best-effort: a 0 - # here just disables the duration checks for this track, never rejects. + # Expected track length, drives every preview/truncation guard here: + # * _get_hls_manifest's pre-download is_preview_playlist check + # * the pre-download is_short_audio manifest check + # * the post-download is_preview_download faked-header decode check + # Best-effort: a 0 here just disables the duration checks, never rejects. expected_s = 0.0 try: info = self.get_track_info(track_id) or {} expected_s = float(info.get('duration_s') or 0) except Exception: expected_s = 0.0 + expected_duration_s = expected_s # alias for _get_hls_manifest's param name for q_key in chain: if self.shutdown_check and self.shutdown_check(): logger.info("Shutdown detected, aborting HiFi download") return None - manifest_info = self._get_hls_manifest(track_id, quality=q_key) + manifest_info = self._get_hls_manifest(track_id, quality=q_key, + expected_duration_s=expected_duration_s) if ( not manifest_info or ( diff --git a/core/imports/file_ops.py b/core/imports/file_ops.py index 9923c39f..747d65e2 100644 --- a/core/imports/file_ops.py +++ b/core/imports/file_ops.py @@ -230,6 +230,9 @@ def get_audio_quality_string(file_path): if ext == ".flac": from mutagen.flac import FLAC audio = FLAC(file_path) + sr = getattr(audio.info, "sample_rate", 0) or 0 + if sr: + return f"FLAC {audio.info.bits_per_sample}bit/{sr / 1000:g}kHz" return f"FLAC {audio.info.bits_per_sample}bit" if ext == ".mp3": @@ -263,6 +266,106 @@ def get_audio_quality_string(file_path): return "" +def probe_audio_quality(file_path: str): + """Read the actual file and return an AudioQuality with real measured values. + + Uses mutagen to extract sample_rate, bit_depth, and bitrate from the + downloaded file — these are ground-truth values, not estimates. + Returns None when the file cannot be read. + """ + from core.quality.model import AudioQuality + try: + ext = os.path.splitext(file_path)[1].lower().lstrip('.') + + if ext == 'flac': + from mutagen.flac import FLAC + audio = FLAC(file_path) + return AudioQuality( + format='flac', + bitrate=audio.info.bitrate // 1000 if audio.info.bitrate else None, + sample_rate=audio.info.sample_rate, + bit_depth=audio.info.bits_per_sample, + ) + + if ext == 'mp3': + from mutagen.mp3 import MP3 + audio = MP3(file_path) + return AudioQuality( + format='mp3', + bitrate=audio.info.bitrate // 1000, + sample_rate=audio.info.sample_rate, + ) + + if ext in ('m4a', 'aac', 'mp4'): + from mutagen.mp4 import MP4 + audio = MP4(file_path) + # .m4a can carry AAC (lossy) OR ALAC (lossless) — only the real + # codec tells them apart, which is why extension-based classification + # defaults to 'aac' and we correct it here from the probed file. + codec = (getattr(audio.info, 'codec', '') or '').lower() + if 'alac' in codec: + return AudioQuality( + format='alac', + bitrate=audio.info.bitrate // 1000 if audio.info.bitrate else None, + sample_rate=audio.info.sample_rate, + bit_depth=getattr(audio.info, 'bits_per_sample', None) or None, + ) + return AudioQuality( + format='aac', + bitrate=audio.info.bitrate // 1000, + sample_rate=audio.info.sample_rate, + ) + + if ext == 'ogg': + from mutagen.oggvorbis import OggVorbis + audio = OggVorbis(file_path) + return AudioQuality( + format='ogg', + bitrate=audio.info.bitrate // 1000, + sample_rate=audio.info.sample_rate, + ) + + if ext == 'opus': + from mutagen.oggopus import OggOpus + audio = OggOpus(file_path) + return AudioQuality( + format='opus', + bitrate=audio.info.bitrate // 1000, + sample_rate=audio.info.sample_rate, + ) + + if ext in ('wav', 'aiff', 'aif'): + # AIFF must use mutagen.aiff.AIFF — WAVE() can't parse it and would + # raise, making the file fail open and silently bypass the quality + # filter. Both are uncompressed PCM, so they share the 'wav' tier. + if ext == 'wav': + from mutagen.wave import WAVE + audio = WAVE(file_path) + else: + from mutagen.aiff import AIFF + audio = AIFF(file_path) + return AudioQuality( + format='wav', + bitrate=audio.info.bitrate // 1000 if audio.info.bitrate else None, + sample_rate=audio.info.sample_rate, + bit_depth=getattr(audio.info, 'bits_per_sample', None), + ) + + if ext == 'wma': + from mutagen.asf import ASF + audio = ASF(file_path) + return AudioQuality( + format='wma', + bitrate=audio.info.bitrate // 1000 if audio.info.bitrate else None, + sample_rate=getattr(audio.info, 'sample_rate', None), + ) + + return None + except Exception as e: + logger.debug("probe_audio_quality failed for %s: %s", file_path, e) + return None + + def get_quality_tier_from_extension(file_path): """Classify a file extension into a quality tier.""" if not file_path: diff --git a/core/imports/guards.py b/core/imports/guards.py index f4519a5d..12bc9afb 100644 --- a/core/imports/guards.py +++ b/core/imports/guards.py @@ -105,30 +105,72 @@ def move_to_quarantine(file_path: str, context: dict, reason: str, automation_en def check_flac_bit_depth(file_path: str, context: dict) -> Optional[str]: - """Return a rejection message if a FLAC file violates the configured bit depth.""" - if not context.get("_audio_quality", "").startswith("FLAC"): + """Legacy wrapper — delegates to check_quality_target. + + Kept for callers that still pass trigger='bit_depth'; the new guard + covers bit_depth as part of the full quality target check. + """ + return check_quality_target(file_path, context) + + +def check_quality_target(file_path: str, context: dict) -> Optional[str]: + """Return a rejection message when the downloaded file does not satisfy + the user's quality priority list. + + Probes the actual file with mutagen (ground-truth sample_rate, + bit_depth, bitrate) and checks it against the profile's + ``ranked_targets``. Falls back gracefully when fallback_enabled=True. + + Works for all formats and all download sources — no Soulseek-specific + logic here. + """ + from core.imports.file_ops import probe_audio_quality + from core.quality.selection import targets_from_profile, quality_meets_profile + + # Master toggle (Settings → Import). When OFF, the quality check is skipped + # entirely and files import regardless of quality — the user opted out of + # quality-filtering on import. Default ON preserves existing behaviour. The + # library Quality Upgrade scanner still flags below-profile files either way. + if _get_config_manager().get("import.quality_filter_enabled", True) is False: + logger.debug( + "[QualityGuard] import.quality_filter_enabled=False — skipping quality " + "filter for %s", os.path.basename(file_path), + ) return None - quality_profile = MusicDatabase().get_quality_profile() - flac_config = quality_profile.get("qualities", {}).get("flac", {}) - flac_pref = flac_config.get("bit_depth", "any") - if flac_pref == "any": + aq = probe_audio_quality(file_path) + if aq is None: + logger.debug("[QualityGuard] Could not probe %s — skipping check", os.path.basename(file_path)) return None - actual_bits = context["_audio_quality"].replace("FLAC ", "").replace("bit", "") - if actual_bits == flac_pref: + profile = MusicDatabase().get_quality_profile() + targets, fallback_enabled = targets_from_profile(profile) + + if not targets: return None - flac_fallback = flac_config.get("bit_depth_fallback", True) downsample_enabled = _get_config_manager().get("lossy_copy.downsample_hires", False) + + matched = quality_meets_profile(aq, targets) + track_info = context.get("track_info", {}) track_name = track_info.get("name", os.path.basename(file_path)) + actual_label = aq.label() - if flac_fallback or downsample_enabled: - if downsample_enabled: - logger.info("[FLAC Downsample] Accepted %s-bit FLAC (will be downsampled to %s-bit): %s", actual_bits, flac_pref, track_name) - else: - logger.warning("[FLAC Fallback] Accepted %s-bit FLAC (preferred %s-bit): %s", actual_bits, flac_pref, track_name) + if matched: + logger.info("[QualityGuard] %s meets profile: %s", track_name, actual_label) return None - return f"FLAC bit depth mismatch: file is {actual_bits}-bit, preference is {flac_pref}-bit" + # No target matched + best_label = targets[0].label if targets else "?" + if fallback_enabled or downsample_enabled: + logger.warning( + "[QualityGuard] %s did not match any target (got %s, wanted %s) — accepting via fallback", + track_name, actual_label, best_label, + ) + return None + + return ( + f"Quality mismatch: file is {actual_label}, " + f"does not satisfy any configured target (best wanted: {best_label})" + ) diff --git a/core/imports/pipeline.py b/core/imports/pipeline.py index 700bc51a..e23f9590 100644 --- a/core/imports/pipeline.py +++ b/core/imports/pipeline.py @@ -34,9 +34,11 @@ from core.imports.context import ( ) 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.guards import check_flac_bit_depth, check_quality_target, move_to_quarantine +from core.imports.silence import detect_broken_audio from core.imports.quarantine import ( approve_quarantine_entry, + delete_quarantine_entry, entry_id_from_quarantined_filename, list_quarantine_entries, ) @@ -160,7 +162,9 @@ def import_rejection_reason(context: dict) -> str | None: f"{context.get('_acoustid_failure_msg', 'fingerprint mismatch')}" ) if context.get('_bitdepth_rejected'): - return "rejected by bit-depth filter" + return "rejected by quality filter" + if context.get('_silence_rejected'): + return "rejected by audio guard (incomplete or silent audio)" if context.get('_race_guard_failed'): return "source file disappeared before import completed" return None @@ -353,6 +357,116 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta f"drift={integrity.checks.get('length_drift_s', 'n/a')})" ) + # Audio-completeness guard — runs right where the length is verified, + # BEFORE the AcoustID/quality gates, so a truncated file (container + # claims the full length but only ~30s actually decodes, e.g. HiFi/ + # Monochrome HLS assembly) or a mostly-silent file is caught regardless + # of its quality verdict and gets the right reason. Same quarantine + + # next-candidate retry pattern as the integrity check. + # + # Opt-in (default OFF): this is the one check that fully DECODES the file + # with ffmpeg, so it is the most CPU-expensive step in post-processing. + # Most preview/truncation cases are already caught at the source + # (HiFi/Qobuz have their own guards), so it stays off unless the user + # turns it on under Settings → Post-processing. + _audio_guard_enabled = config_manager.get('post_processing.audio_completeness_check', False) + _skip_audio = (not _audio_guard_enabled) or _should_skip_quarantine_check(context, 'silence') + audio_reason = None if _skip_audio else detect_broken_audio(file_path) + if audio_reason: + logger.error(f"[AudioGuard] Rejected {_basename}: {audio_reason}") + context['_silence_rejected'] = True + try: + quarantine_path = move_to_quarantine( + file_path, context, audio_reason, automation_engine, + trigger='silence', + ) + _mark_task_quarantined(context, quarantine_path) + logger.warning("File quarantined — incomplete/silent audio: %s", 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 del_error: + logger.debug("delete broken file fallback: %s", del_error) + + with matched_context_lock: + matched_downloads_context.pop(context_key, None) + + task_id = context.get('task_id') + batch_id = context.get('batch_id') + # Try the next-best candidate before giving up — same pattern as + # the integrity / AcoustID / quality failures. + if _requeue_quarantined_task_for_retry(task_id, batch_id, 'silence'): + logger.info( + "Incomplete/silent audio for task %s — retrying next-best candidate: %s", + task_id, audio_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"Audio guard: {audio_reason}" + if task_id and batch_id: + _notify_download_completed(batch_id, task_id, success=False) + 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') if _skip_acoustid: logger.info(f"[AcoustID] Skipped (user approval) for {_basename}") @@ -390,14 +504,33 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta logger.info(f"AcoustID verification result: {verification_result.value} - {verification_msg}") context['_acoustid_result'] = verification_result.value - if verification_result == VerificationResult.FAIL: + # Fail-closed mode: when the user requires a hard AcoustID + # PASS, a SKIP (ran but couldn't confirm — no fingerprint + # match / cross-script metadata) is treated like a FAIL: + # quarantine + try the next candidate, instead of importing + # an unverified file. ERROR (rate-limit / infra) is NOT + # blocked — that would stall the whole pipeline during an + # outage; those still import with their existing flag. + require_verified = config_manager.get('acoustid.require_verified', False) + _skip_as_fail = ( + require_verified + and verification_result == VerificationResult.SKIP + ) + if _skip_as_fail: + verification_msg = ( + f"AcoustID could not confirm the track and 'require verified' " + f"is on — rejecting unverified file ({verification_msg})" + ) + logger.warning("[AcoustID] Require-verified: SKIP treated as FAIL — %s", verification_msg) + + if verification_result == VerificationResult.FAIL or _skip_as_fail: try: quarantine_path = move_to_quarantine( file_path, context, verification_msg, automation_engine, - trigger='acoustid', + trigger='acoustid_unverified' if _skip_as_fail else 'acoustid', ) _mark_task_quarantined(context, quarantine_path) logger.error(f"File quarantined due to verification failure: {quarantine_path}") @@ -577,48 +710,6 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta 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_bit_depth = _should_skip_quarantine_check(context, 'bit_depth') - rejection_reason = None if _skip_bit_depth else check_flac_bit_depth(file_path, context) - if _skip_bit_depth: - logger.info(f"[BitDepth] Skipped (user approval) for {_basename}") - if rejection_reason: - try: - quarantine_path = move_to_quarantine( - file_path, - context, - rejection_reason, - automation_engine, - trigger='bit_depth', - ) - _mark_task_quarantined(context, quarantine_path) - logger.info(f"File quarantined due to bit depth filter: {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 - with matched_context_lock: - if context_key in matched_downloads_context: - del matched_downloads_context[context_key] - - task_id = context.get('task_id') - batch_id = context.get('batch_id') - 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"Bit depth 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] clean_track_name = get_import_clean_title( context, @@ -987,6 +1078,22 @@ def post_process_matched_download_with_verification(context_key, context, file_p if original_batch_id: context['batch_id'] = original_batch_id + # Quality / audio-guard quarantine. Unlike acoustid/integrity (whose + # retry+fail is driven by THIS wrapper below), the inner pipeline fully + # owns the quality and audio-guard outcome: it quarantines the file and + # then either re-queues the next-best candidate or marks the task failed + # and notifies. The wrapper must NOT continue to the "assume success" + # fall-through — that marked the quarantined file Completed, so the same + # track showed in BOTH Completed and Quarantine (e.g. an MP3-VBR rejected + # by a FLAC-only profile). It must also NOT mark it failed here, which + # would clobber a successful next-candidate retry. Just return. + if context.get('_bitdepth_rejected') or context.get('_silence_rejected'): + logger.info( + f"Task {task_id} quarantined by the quality/audio guard — inner " + f"pipeline already handled retry/fail; wrapper not marking completed" + ) + return + if context.get('_race_guard_failed'): logger.info(f"Race guard: source file gone for task {task_id} — marking as failed") with tasks_lock: @@ -1000,6 +1107,33 @@ def post_process_matched_download_with_verification(context_key, context, file_p return if context.get('_acoustid_quarantined'): + # Race-condition guard: if the user approved an alternative quarantine + # entry for this track while this download was already in flight, the + # pipeline will have created a new quarantine entry for the just-finished + # file. Delete that entry and exit — the user's choice already won. + _approved_alt = False + if task_id: + with tasks_lock: + _ct = download_tasks.get(task_id) + if isinstance(_ct, dict) and _ct.get('_quarantine_approved_alternative'): + _approved_alt = True + if _approved_alt: + _eid = context.get('_quarantine_entry_id') + if _eid: + try: + from core.imports.guards import _get_config_manager + _dl_dir = _get_config_manager().get('soulseek.download_path', './downloads') + _qdir = os.path.join(docker_resolve_path(_dl_dir), 'ss_quarantine') + delete_quarantine_entry(_qdir, _eid) + logger.info( + f"[Quarantine] Discarded late entry {_eid} — task {task_id} " + f"was cancelled by prior quarantine approval" + ) + except Exception as _dqe: + logger.debug(f"[Quarantine] Could not clean up late entry {_eid}: {_dqe}") + _notify_download_completed(batch_id, task_id, success=False) + return + failure_msg = context.get('_acoustid_failure_msg', 'AcoustID verification failed') # Before failing outright, try the next-best candidate. The wrong # file was just quarantined; re-running the worker (with the bad @@ -1116,6 +1250,10 @@ def post_process_matched_download_with_verification(context_key, context, file_p _mark_task_completed(task_id, context.get('track_info')) if context.get('_verification_status'): download_tasks[task_id]['verification_status'] = context['_verification_status'] + if context.get('_history_id'): + download_tasks[task_id]['history_id'] = context['_history_id'] + if context.get('_audio_quality'): + download_tasks[task_id]['quality'] = context['_audio_quality'] with matched_context_lock: if context_key in matched_downloads_context: del matched_downloads_context[context_key] @@ -1130,6 +1268,10 @@ def post_process_matched_download_with_verification(context_key, context, file_p download_tasks[task_id]['metadata_enhanced'] = True if context.get('_verification_status'): download_tasks[task_id]['verification_status'] = context['_verification_status'] + if context.get('_history_id'): + download_tasks[task_id]['history_id'] = context['_history_id'] + if context.get('_audio_quality'): + download_tasks[task_id]['quality'] = context['_audio_quality'] redownload_ctx = download_tasks[task_id].get('_redownload_context') with matched_context_lock: diff --git a/core/imports/quarantine.py b/core/imports/quarantine.py index 7dd43be6..dd249f4d 100644 --- a/core/imports/quarantine.py +++ b/core/imports/quarantine.py @@ -168,16 +168,15 @@ def quarantine_group_key( fetch — NOT the downloaded file's own tags. That matters: the file's metadata is frequently *wrong* (that's why it failed acoustid / integrity), whereas the target is fixed and identical across every - alternative for one song (they're all Soulseek uploads of the *same* - source track), so grouping by it is an exact relationship, not a fuzzy - metadata guess. + alternative for one song. - Prefers a stable target-track id from the sidecar `context.track_info` - when present — isrc, then source id, then uri — since those are exact - and constant across siblings. Falls back to the normalized - artist|track name only for legacy/thin sidecars that carry no context. - Keys are kind-prefixed so an id-based key never collides with a - name-based one. + Uses ISRC when available (truly universal across sources and batches). + Falls back to normalized artist|track name, which is stable across + different batches and sources. + + Source-specific IDs (Spotify track id, Qobuz id, uri) are intentionally + NOT used: the same song imported from different playlists or sources gets + different source IDs, so id-based keys break cross-batch sibling matching. Returns ``None`` when nothing identifies the target (no usable id and both name fields empty). Callers treat a ``None`` key as "its own @@ -191,12 +190,6 @@ def quarantine_group_key( isrc = str(ti.get("isrc") or "").strip().lower() if isrc: return f"isrc:{isrc}" - tid = str(ti.get("id") or "").strip() - if tid: - return f"id:{tid}" - uri = str(ti.get("uri") or "").strip() - if uri: - return f"uri:{uri}" artist = " ".join(str(expected_artist or "").split()).lower() track = " ".join(str(expected_track or "").split()).lower() if not artist and not track: @@ -302,6 +295,10 @@ def list_quarantine_entries(quarantine_dir: str) -> List[Dict[str, Any]]: "source_username": source_username, "source_filename": source_filename, "thumb_url": _extract_context_thumb(ctx), + # Real probed audio quality (recorded on the context before the + # quality/AcoustID gates) so the review UI shows what the file + # actually is when deciding to approve/delete. + "quality": ctx.get("_audio_quality", "") if isinstance(ctx, dict) else "", } ) diff --git a/core/imports/side_effects.py b/core/imports/side_effects.py index 867329c3..f2e21efa 100644 --- a/core/imports/side_effects.py +++ b/core/imports/side_effects.py @@ -252,7 +252,7 @@ def record_library_history_download(context: Dict[str, Any]) -> None: origin, origin_context = derive_download_origin(context) db = get_database() - db.add_library_history_entry( + _history_id = db.add_library_history_entry( event_type="download", title=title, artist_name=artist_name, @@ -270,6 +270,10 @@ def record_library_history_download(context: Dict[str, Any]) -> None: origin_context=origin_context, verification_status=context.get("_verification_status"), ) + # Stash the row id so the live download task can link to its + # library_history row (the Unverified review queue needs it). + if isinstance(_history_id, int) and _history_id > 0: + context["_history_id"] = _history_id except Exception as e: logger.debug("library history record failed: %s", e) diff --git a/core/imports/silence.py b/core/imports/silence.py new file mode 100644 index 00000000..ecbe048f --- /dev/null +++ b/core/imports/silence.py @@ -0,0 +1,277 @@ +"""Audio-completeness guard — detect files whose container duration looks +right but whose REAL audio is far shorter, or mostly silence. + +Motivating bug: HiFi/Monochrome HLS assembly can yield a file whose container +claims the full track length (e.g. 3:08) while only ~30s of audio actually +decodes — the rest is missing. The duration-agreement and quality guards both +pass (mutagen reads the container's 3:08 and the format/bitrate are fine), so +nothing catches it until you listen. ffmpeg's ``time=`` even reports 0 with no +error on such a file, so the robust signal is to DECODE the audio and compare +the real duration (sample count / sample rate, via ``astats``) against the +container duration. A separate ``silencedetect`` pass also flags genuine +silence-padding. + +The parsers here are pure and unit-tested; the ffmpeg invocations are +integration glue that fails open (returns None) when ffmpeg or mutagen can't +run, so a tooling problem never blocks a legitimate import. +""" + +from __future__ import annotations + +import re +import subprocess +from typing import Optional + +from utils.logging_config import get_logger + +logger = get_logger("imports.silence") + +# Real decoded audio must cover at least this fraction of the container +# duration. A legit file decodes to ~100% (encoder padding aside); a truncated +# file decodes to a small fraction (the Blossom file: 30s of a 188s container +# = 16%). 0.85 leaves generous headroom against false positives. +DEFAULT_MIN_DURATION_RATIO = 0.85 + +_SAMPLES_RE = re.compile(r"Number of samples:\s*([0-9]+)") + +# Defaults: treat audio below -50 dB lasting >= 2s as silence, and reject when +# more than half the track is silent. A normal song — even with quiet intros/ +# outros — sits far below 0.5; a 30s-real + padded-silence file sits near 0.85. +DEFAULT_NOISE_DB = -50 +DEFAULT_MIN_SILENCE_S = 2.0 +DEFAULT_THRESHOLD = 0.5 + +_SILENCE_DURATION_RE = re.compile(r"silence_duration:\s*([0-9]+(?:\.[0-9]+)?)") + + +def silence_ratio_from_output(ffmpeg_stderr: str, total_duration_s: float) -> float: + """Fraction of *total_duration_s* covered by detected silence. + + Sums every ``silence_duration: X`` reported by ffmpeg ``silencedetect`` + and divides by the track length. Capped at 1.0; returns 0.0 when the + duration is unknown/zero or no silence was reported. + """ + if not total_duration_s or total_duration_s <= 0: + return 0.0 + total_silence = sum(float(m) for m in _SILENCE_DURATION_RE.findall(ffmpeg_stderr or "")) + if total_silence <= 0: + return 0.0 + return min(total_silence / total_duration_s, 1.0) + + +def is_mostly_silent_reason( + ffmpeg_stderr: str, + total_duration_s: float, + *, + threshold: float = DEFAULT_THRESHOLD, +) -> Optional[str]: + """Return a rejection reason when the silent fraction meets *threshold*.""" + ratio = silence_ratio_from_output(ffmpeg_stderr, total_duration_s) + if ratio >= threshold: + pct = round(ratio * 100) + audible_s = round(total_duration_s * (1 - ratio)) + return ( + f"Audio is mostly silent: {pct}% silence (only ~{audible_s}s audible of " + f"{round(total_duration_s)}s) — likely a truncated/preview file padded " + f"to full length" + ) + return None + + +def _ffmpeg_available() -> bool: + try: + subprocess.run( + ["ffmpeg", "-version"], + stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, + timeout=10, check=True, + ) + return True + except (FileNotFoundError, subprocess.SubprocessError, OSError): + return False + + +def _probe_duration_s(file_path: str) -> Optional[float]: + try: + from mutagen import File as MutagenFile + audio = MutagenFile(file_path) + if audio and audio.info and getattr(audio.info, "length", None): + return float(audio.info.length) + except Exception as exc: # pragma: no cover - defensive + logger.debug("silence guard duration probe failed for %s: %s", file_path, exc) + return None + + +def detect_mostly_silent( + file_path: str, + *, + threshold: float = DEFAULT_THRESHOLD, + noise_db: int = DEFAULT_NOISE_DB, + min_silence_s: float = DEFAULT_MIN_SILENCE_S, +) -> Optional[str]: + """Run ffmpeg ``silencedetect`` over *file_path* and return a rejection + reason when the file is mostly silence, else None. + + Fails open: returns None when ffmpeg/mutagen are unavailable or error, so + a tooling problem never quarantines a legitimate file. + """ + if not _ffmpeg_available(): + logger.debug("silence guard skipped — ffmpeg not available") + return None + + total_duration_s = _probe_duration_s(file_path) + if not total_duration_s: + return None + + try: + proc = subprocess.run( + [ + "ffmpeg", "-hide_banner", "-nostats", "-i", file_path, + "-af", f"silencedetect=noise={noise_db}dB:d={min_silence_s}", + "-f", "null", "-", + ], + stdout=subprocess.DEVNULL, stderr=subprocess.PIPE, + timeout=120, + ) + except (subprocess.SubprocessError, OSError) as exc: + logger.debug("silence guard ffmpeg run failed for %s: %s", file_path, exc) + return None + + stderr = proc.stderr.decode("utf-8", errors="replace") if proc.stderr else "" + return is_mostly_silent_reason(stderr, total_duration_s, threshold=threshold) + + +# ── Truncation: real decoded duration vs container duration ──────────────── + +def measured_duration_from_astats(astats_stderr: str, sample_rate: int) -> Optional[float]: + """Real decoded audio duration in seconds from ffmpeg ``astats`` output. + + ``astats`` reports the per-channel ``Number of samples``; dividing by the + sample rate gives the true decoded length. Returns None when the sample + count or sample rate is unavailable. + """ + if not sample_rate or sample_rate <= 0: + return None + m = _SAMPLES_RE.search(astats_stderr or "") + if not m: + return None + return int(m.group(1)) / float(sample_rate) + + +def incomplete_audio_reason( + measured_s: Optional[float], + container_s: Optional[float], + *, + min_ratio: float = DEFAULT_MIN_DURATION_RATIO, +) -> Optional[str]: + """Return a rejection reason when the real decoded duration falls short of + the container duration (a truncated file whose metadata over-states length). + """ + if not measured_s or not container_s or container_s <= 0: + return None + if measured_s >= container_s * min_ratio: + return None + pct = round(measured_s / container_s * 100) + return ( + f"Incomplete audio: only ~{round(measured_s)}s actually decodes of a " + f"{round(container_s)}s file ({pct}%) — truncated/broken download " + f"(container duration over-states the real audio)" + ) + + +def _measured_audio_duration_s(file_path: str, sample_rate: int) -> Optional[float]: + try: + proc = subprocess.run( + ["ffmpeg", "-hide_banner", "-nostats", "-i", file_path, + "-af", "astats=metadata=1", "-f", "null", "-"], + stdout=subprocess.DEVNULL, stderr=subprocess.PIPE, timeout=120, + ) + except (subprocess.SubprocessError, OSError) as exc: + logger.debug("astats run failed for %s: %s", file_path, exc) + return None + stderr = proc.stderr.decode("utf-8", errors="replace") if proc.stderr else "" + return measured_duration_from_astats(stderr, sample_rate) + + +def detect_incomplete_audio( + file_path: str, + *, + min_ratio: float = DEFAULT_MIN_DURATION_RATIO, +) -> Optional[str]: + """Decode the file and reject when the real audio is far shorter than the + container claims. Fails open when ffmpeg/mutagen are unavailable. + """ + if not _ffmpeg_available(): + return None + try: + from mutagen import File as MutagenFile + audio = MutagenFile(file_path) + if not (audio and audio.info): + return None + container_s = float(getattr(audio.info, "length", 0) or 0) + sample_rate = int(getattr(audio.info, "sample_rate", 0) or 0) + except Exception as exc: # pragma: no cover - defensive + logger.debug("container probe failed for %s: %s", file_path, exc) + return None + + measured_s = _measured_audio_duration_s(file_path, sample_rate) + return incomplete_audio_reason(measured_s, container_s, min_ratio=min_ratio) + + +def detect_broken_audio( + file_path: str, + *, + min_ratio: float = DEFAULT_MIN_DURATION_RATIO, + threshold: float = DEFAULT_THRESHOLD, + noise_db: int = DEFAULT_NOISE_DB, + min_silence_s: float = DEFAULT_MIN_SILENCE_S, +) -> Optional[str]: + """Combined post-download audio guard: reject a file that is truncated + (real audio far shorter than the container) or mostly silence. Returns the + first failure reason, or None when the audio looks complete. + + Runs a SINGLE ffmpeg decode pass with both the ``astats`` (truncation) and + ``silencedetect`` (silence) filters chained — one decode of the file feeds + both checks instead of two full decodes. Halves the CPU cost versus running + ``detect_incomplete_audio`` and ``detect_mostly_silent`` back to back. + + Fails open: returns None when ffmpeg/mutagen are unavailable or error, so a + tooling problem never quarantines a legitimate file. + """ + if not _ffmpeg_available(): + logger.debug("audio guard skipped — ffmpeg not available") + return None + + try: + from mutagen import File as MutagenFile + audio = MutagenFile(file_path) + if not (audio and audio.info): + return None + container_s = float(getattr(audio.info, "length", 0) or 0) + sample_rate = int(getattr(audio.info, "sample_rate", 0) or 0) + except Exception as exc: # pragma: no cover - defensive + logger.debug("container probe failed for %s: %s", file_path, exc) + return None + + try: + proc = subprocess.run( + [ + "ffmpeg", "-hide_banner", "-nostats", "-i", file_path, + "-af", f"astats=metadata=1,silencedetect=noise={noise_db}dB:d={min_silence_s}", + "-f", "null", "-", + ], + stdout=subprocess.DEVNULL, stderr=subprocess.PIPE, timeout=120, + ) + except (subprocess.SubprocessError, OSError) as exc: + logger.debug("audio guard ffmpeg run failed for %s: %s", file_path, exc) + return None + + stderr = proc.stderr.decode("utf-8", errors="replace") if proc.stderr else "" + + # Truncation check first (real audio far shorter than the container). + measured_s = measured_duration_from_astats(stderr, sample_rate) + reason = incomplete_audio_reason(measured_s, container_s, min_ratio=min_ratio) + if reason: + return reason + + # Then silence-padding (mostly-silent file). + return is_mostly_silent_reason(stderr, container_s, threshold=threshold) diff --git a/core/imports/version_mismatch_fallback.py b/core/imports/version_mismatch_fallback.py index 3e0ef92d..2bf37388 100644 --- a/core/imports/version_mismatch_fallback.py +++ b/core/imports/version_mismatch_fallback.py @@ -103,9 +103,12 @@ def select_version_mismatch_fallback( # don't guess which the user wants. return None - # First tried = oldest = highest-confidence (the retry walks candidates - # best-first). The id is a "_