Fix album-bundle downloads landing every track as track 1
Soulseek album-bundle (and any other release-staging path) was
importing every file with ``track_number=1`` because the staging
metadata reader used the auto-import-flavor filename extractor:
``extract_track_number_from_filename`` returns 1 when the basename
has no ``NN -`` prefix. That's the right default for the loose
auto-import flow (single file in, no upstream metadata to lean
on), but completely wrong for staging-cache reads:
- For an album-bundle download the user has authoritative track
numbers in the Spotify track list flowing through to
``track_info`` for each task.
- ``try_staging_match`` in ``core/downloads/staging.py`` was
meant to use those numbers when the staged file's own metadata
doesn't have them.
- But the staging cache populated ``track_number=1`` for every
untagged bare-title file (e.g. ``Cha-La Head-Cha-La.flac``), the
album-bundle resolution branch reads file-side first, sees 1,
and short-circuits the rest of the chain.
Fix:
- New ``extract_explicit_track_number`` in
``core/imports/filename.py`` — strict variant that returns
``0`` when no numeric prefix is visible. Docstring explicitly
contrasts with the legacy 1-defaulting helper so future
callers pick the right one.
- ``read_staging_file_metadata`` in ``core/imports/staging.py``
now uses the strict extractor, so the staging file dict
carries ``track_number=0`` ("unknown") instead of ``1`` for
untagged bare-title files.
- The legacy ``extract_track_number_from_filename`` keeps its
1-default behavior so auto-import callers + the post-process
template fallbacks are unchanged; it's now implemented in
terms of the strict variant.
- Tag-side parsing also tightened to require ``> 0`` before
overriding the filename-derived value.
3 new tests pin the contracts:
- ``test_extract_explicit_track_number_returns_zero_when_no_prefix``
- ``test_read_staging_file_metadata_returns_zero_track_when_unknown``
- existing ``test_extract_track_number_from_filename_handles_common_patterns``
now explicitly comments why bare filenames keep returning 1.
758 tests across imports + downloads + repair + staging-provenance
suites green. WHATS_NEW entry added under 2.6.3.
Reported against an album-bundle download of Ryoto's
"Cha-La Head-Cha-La" where slskd staged 15 untagged FLAC files
named after the song titles only.
This commit is contained in:
parent
f758ae9330
commit
85426a210c
4 changed files with 88 additions and 8 deletions
|
|
@ -15,8 +15,32 @@ _TRACK_PATTERNS = (
|
||||||
|
|
||||||
|
|
||||||
def extract_track_number_from_filename(filename: str, title: str = None) -> int:
|
def extract_track_number_from_filename(filename: str, title: str = None) -> int:
|
||||||
"""Extract track number from a filename. Returns 1 if not found."""
|
"""Extract track number from a filename. Returns 1 if not found.
|
||||||
basename = os.path.splitext(os.path.basename(filename))[0].strip()
|
|
||||||
|
Use ``extract_explicit_track_number`` instead when the caller needs
|
||||||
|
to distinguish "track 1" from "unknown" — staging-file readers in
|
||||||
|
particular MUST NOT conflate a bare title (no numeric prefix) with
|
||||||
|
track 1, or every untagged album-bundle file gets imported as
|
||||||
|
``track_number=1`` and downstream callers can't recover the real
|
||||||
|
number from authoritative metadata (Spotify track list, etc.).
|
||||||
|
"""
|
||||||
|
num = extract_explicit_track_number(filename)
|
||||||
|
return num if num > 0 else 1
|
||||||
|
|
||||||
|
|
||||||
|
def extract_explicit_track_number(filename: str) -> int:
|
||||||
|
"""Extract a track number only when the filename visibly carries one.
|
||||||
|
|
||||||
|
Returns the parsed track number when the basename starts with a
|
||||||
|
recognizable numeric prefix (``"01 - Title"``, ``"1-03 Title"``,
|
||||||
|
``"(01) Title"``, ``"[01] Title"``); returns ``0`` when no such
|
||||||
|
prefix is present. This is the contract staging readers want —
|
||||||
|
"unknown" must stay unknown so a downstream consumer with better
|
||||||
|
info (Spotify metadata, MusicBrainz, etc.) can fill it in.
|
||||||
|
"""
|
||||||
|
basename = os.path.splitext(os.path.basename(str(filename or "")))[0].strip()
|
||||||
|
if not basename:
|
||||||
|
return 0
|
||||||
|
|
||||||
match = re.match(r"^\d[\-\.](\d{1,2})\s*[\-\.]\s*", basename)
|
match = re.match(r"^\d[\-\.](\d{1,2})\s*[\-\.]\s*", basename)
|
||||||
if match:
|
if match:
|
||||||
|
|
@ -30,7 +54,7 @@ def extract_track_number_from_filename(filename: str, title: str = None) -> int:
|
||||||
if 1 <= num <= 999:
|
if 1 <= num <= 999:
|
||||||
return num
|
return num
|
||||||
|
|
||||||
return 1
|
return 0
|
||||||
|
|
||||||
|
|
||||||
def parse_filename_metadata(filename: str) -> Dict[str, Any]:
|
def parse_filename_metadata(filename: str) -> Dict[str, Any]:
|
||||||
|
|
|
||||||
|
|
@ -7,7 +7,10 @@ import threading
|
||||||
from typing import Any, Dict, Iterable, List, Optional, Tuple
|
from typing import Any, Dict, Iterable, List, Optional, Tuple
|
||||||
|
|
||||||
from core.imports.paths import docker_resolve_path
|
from core.imports.paths import docker_resolve_path
|
||||||
from core.imports.filename import extract_track_number_from_filename
|
from core.imports.filename import (
|
||||||
|
extract_explicit_track_number,
|
||||||
|
extract_track_number_from_filename,
|
||||||
|
)
|
||||||
from utils.logging_config import get_logger
|
from utils.logging_config import get_logger
|
||||||
|
|
||||||
logger = get_logger("imports.staging")
|
logger = get_logger("imports.staging")
|
||||||
|
|
@ -103,12 +106,19 @@ def read_staging_file_metadata(file_path: str, filename: Optional[str] = None) -
|
||||||
if not albumartist:
|
if not albumartist:
|
||||||
albumartist = artist
|
albumartist = artist
|
||||||
|
|
||||||
track_number = extract_track_number_from_filename(filename or file_path)
|
# Use the strict extractor here: when the filename has no visible
|
||||||
|
# track-number prefix, return 0 instead of pretending it's track 1.
|
||||||
|
# Downstream consumers (staging match in core/downloads/staging.py)
|
||||||
|
# will then fall through to authoritative metadata (track_info from
|
||||||
|
# the original Spotify / API source) rather than locking the import
|
||||||
|
# to track_number=1 for every file in the bundle.
|
||||||
|
track_number = extract_explicit_track_number(filename or file_path)
|
||||||
try:
|
try:
|
||||||
# Preserve tag-based numbers when present, but still fall back to the filename parser.
|
|
||||||
tag_track_number = _first_tag("tracknumber", "track_number")
|
tag_track_number = _first_tag("tracknumber", "track_number")
|
||||||
if tag_track_number:
|
if tag_track_number:
|
||||||
track_number = int(str(tag_track_number).split("/")[0].strip() or track_number)
|
parsed_tag = int(str(tag_track_number).split("/")[0].strip())
|
||||||
|
if parsed_tag > 0:
|
||||||
|
track_number = parsed_tag
|
||||||
except (TypeError, ValueError):
|
except (TypeError, ValueError):
|
||||||
pass
|
pass
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -5,16 +5,40 @@ from core.imports.file_ops import (
|
||||||
cleanup_empty_directories,
|
cleanup_empty_directories,
|
||||||
safe_move_file,
|
safe_move_file,
|
||||||
)
|
)
|
||||||
from core.imports.filename import extract_track_number_from_filename
|
from core.imports.filename import (
|
||||||
|
extract_explicit_track_number,
|
||||||
|
extract_track_number_from_filename,
|
||||||
|
)
|
||||||
from core.imports.staging import read_staging_file_metadata
|
from core.imports.staging import read_staging_file_metadata
|
||||||
|
|
||||||
|
|
||||||
def test_extract_track_number_from_filename_handles_common_patterns():
|
def test_extract_track_number_from_filename_handles_common_patterns():
|
||||||
assert extract_track_number_from_filename("01 - Song.mp3") == 1
|
assert extract_track_number_from_filename("01 - Song.mp3") == 1
|
||||||
assert extract_track_number_from_filename("1-03 - Song.mp3") == 3
|
assert extract_track_number_from_filename("1-03 - Song.mp3") == 3
|
||||||
|
# Bare filename keeps the auto-import-friendly default of 1 — there's
|
||||||
|
# no upstream metadata to recover from in that flow.
|
||||||
assert extract_track_number_from_filename("Artist - Song.mp3") == 1
|
assert extract_track_number_from_filename("Artist - Song.mp3") == 1
|
||||||
|
|
||||||
|
|
||||||
|
def test_extract_explicit_track_number_returns_zero_when_no_prefix():
|
||||||
|
"""Staging readers need to distinguish 'track 1' from 'unknown'.
|
||||||
|
|
||||||
|
Pinned because:
|
||||||
|
- the legacy extractor defaults to 1 (auto-import semantics),
|
||||||
|
- staging file scanners that conflate the two end up writing every
|
||||||
|
file in an untagged album bundle to track_number=1.
|
||||||
|
"""
|
||||||
|
# Bare titles with no numeric prefix → 0 (unknown).
|
||||||
|
assert extract_explicit_track_number("Artist - Song.mp3") == 0
|
||||||
|
assert extract_explicit_track_number("Cha-La Head-Cha-La.flac") == 0
|
||||||
|
assert extract_explicit_track_number("") == 0
|
||||||
|
# Real prefixes still parse correctly.
|
||||||
|
assert extract_explicit_track_number("01 - Song.mp3") == 1
|
||||||
|
assert extract_explicit_track_number("(03) Song.mp3") == 3
|
||||||
|
# Disc-track format requires a separator after the track number.
|
||||||
|
assert extract_explicit_track_number("1-07 - Song.mp3") == 7
|
||||||
|
|
||||||
|
|
||||||
def test_safe_move_file_replaces_existing_destination(tmp_path):
|
def test_safe_move_file_replaces_existing_destination(tmp_path):
|
||||||
src = tmp_path / "source.flac"
|
src = tmp_path / "source.flac"
|
||||||
dst_dir = tmp_path / "dest"
|
dst_dir = tmp_path / "dest"
|
||||||
|
|
@ -92,6 +116,27 @@ def test_read_staging_file_metadata_falls_back_to_filename_track_number(monkeypa
|
||||||
assert metadata["disc_number"] == 1
|
assert metadata["disc_number"] == 1
|
||||||
|
|
||||||
|
|
||||||
|
def test_read_staging_file_metadata_returns_zero_track_when_unknown(monkeypatch, tmp_path):
|
||||||
|
"""Bare filename + no tags → track_number=0, not 1.
|
||||||
|
|
||||||
|
Pre-fix this returned 1 because the filename extractor's default
|
||||||
|
was 1. The bug caused every untagged file in an album-bundle
|
||||||
|
download to land in the staging cache with track_number=1, which
|
||||||
|
then short-circuited the downstream resolution chain that should
|
||||||
|
have picked up the real number from track_info.
|
||||||
|
"""
|
||||||
|
file_path = tmp_path / "Cha-La Head-Cha-La.flac"
|
||||||
|
file_path.write_text("fake")
|
||||||
|
|
||||||
|
fake_mutagen = types.ModuleType("mutagen")
|
||||||
|
fake_mutagen.File = lambda path, easy=True: None
|
||||||
|
monkeypatch.setitem(sys.modules, "mutagen", fake_mutagen)
|
||||||
|
|
||||||
|
metadata = read_staging_file_metadata(str(file_path), file_path.name)
|
||||||
|
|
||||||
|
assert metadata["track_number"] == 0
|
||||||
|
|
||||||
|
|
||||||
def test_read_staging_file_metadata_uses_filename_fallbacks_when_tags_are_invalid(monkeypatch, tmp_path):
|
def test_read_staging_file_metadata_uses_filename_fallbacks_when_tags_are_invalid(monkeypatch, tmp_path):
|
||||||
file_path = tmp_path / "02 - Song Three.flac"
|
file_path = tmp_path / "02 - Song Three.flac"
|
||||||
file_path.write_text("fake")
|
file_path.write_text("fake")
|
||||||
|
|
|
||||||
|
|
@ -3421,6 +3421,7 @@ const WHATS_NEW = {
|
||||||
{ title: 'ListenBrainz Sync tab', desc: 'new ListenBrainz tab on the Sync page, between Beatport and Import. lists your "For You" / "My Playlists" / "Collaborative" LB playlists in one place. clicking a card kicks off the same discovery → sync → mirror flow you already get from the Discover page (no duplicate UI behind the scenes, just a new entry point). once mirrored, LB playlists participate in Auto-Sync schedules + pipeline automations like any other source. needs ListenBrainz connected in Settings → Connections.', page: 'sync' },
|
{ title: 'ListenBrainz Sync tab', desc: 'new ListenBrainz tab on the Sync page, between Beatport and Import. lists your "For You" / "My Playlists" / "Collaborative" LB playlists in one place. clicking a card kicks off the same discovery → sync → mirror flow you already get from the Discover page (no duplicate UI behind the scenes, just a new entry point). once mirrored, LB playlists participate in Auto-Sync schedules + pipeline automations like any other source. needs ListenBrainz connected in Settings → Connections.', page: 'sync' },
|
||||||
{ title: 'Last.fm Radio Sync tab', desc: 'sibling to the ListenBrainz tab — lists your generated Last.fm Radio playlists alongside the rest of the Sync sources. same discovery → mirror flow under the hood, just a different entry point. new Last.fm radios are still generated from the Discover page by picking a seed track; this tab is for syncing existing ones. mirrors auto-trim when Last.fm Radio cache rotates so old radios don\'t pile up.', page: 'sync' },
|
{ title: 'Last.fm Radio Sync tab', desc: 'sibling to the ListenBrainz tab — lists your generated Last.fm Radio playlists alongside the rest of the Sync sources. same discovery → mirror flow under the hood, just a different entry point. new Last.fm radios are still generated from the Discover page by picking a seed track; this tab is for syncing existing ones. mirrors auto-trim when Last.fm Radio cache rotates so old radios don\'t pile up.', page: 'sync' },
|
||||||
{ title: 'SoulSync Discovery Sync tab', desc: 'last of the unified-tab trio. surfaces your personalized SoulSync Discovery playlists (decade mixes, hidden gems, popular picks, daily mixes, discovery shuffle, etc.) on the Sync page. clicking a card regenerates the playlist + mirrors it under a stable synthetic id, so the same mirror updates in place every Auto-Sync refresh. tracks come out already matched against Spotify / iTunes / Deezer so there\'s no discovery hop — straight to download / sync.', page: 'sync' },
|
{ title: 'SoulSync Discovery Sync tab', desc: 'last of the unified-tab trio. surfaces your personalized SoulSync Discovery playlists (decade mixes, hidden gems, popular picks, daily mixes, discovery shuffle, etc.) on the Sync page. clicking a card regenerates the playlist + mirrors it under a stable synthetic id, so the same mirror updates in place every Auto-Sync refresh. tracks come out already matched against Spotify / iTunes / Deezer so there\'s no discovery hop — straight to download / sync.', page: 'sync' },
|
||||||
|
{ title: 'Fix: album-bundle downloads all landing as track 1', desc: 'soulseek album-bundle downloads (and any other untagged release-staging path) were importing every track with track_number=1. the staging-file reader was using the auto-import\'s filename extractor that defaults to 1 when no NN- prefix is present in the filename — so for albums like Ryoto\'s "Cha-La Head-Cha-La" where slskd hands you bare titles, every file got "track 1" stamped on it. now the staging path uses a strict extractor that returns 0 when it can\'t see an explicit prefix, so the downstream resolver correctly falls through to the authoritative Spotify metadata and the right track numbers land in the library.' },
|
||||||
],
|
],
|
||||||
'2.6.2': [
|
'2.6.2': [
|
||||||
{ date: 'May 24, 2026 — 2.6.2 release' },
|
{ date: 'May 24, 2026 — 2.6.2 release' },
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue