fix(song-info): call load_song for the stem payload — do not reimplement it

CodeRabbit caught a real bug, and it would have hit most real libraries.

load_song() falls back to the DEPRECATED `original_audio:` key when a pack has no
reserved `full` stem — which is every pack written before feedpak 1.15.0. My
payload rebuilt the full-mix rule from extract_meta and returned None for those:
REST would say "no full mix" while the WS said there was one.

Worse than a wrong field: the plugin would preload a graph WITHOUT the pristine
mix and — because the stem signature still matched — never rebuild. Unity
playback would silently downgrade to the lossy stem recombination.

That is exactly the drift this PR claims to prevent, and my test had a hole: I
only covered packs that carry a `full` stem.

So stop reimplementing. The payload now calls load_song, whose LoadedSloppak
already carries the partitioned stems and the resolved full mix, and builds the
URLs exactly as ws_highway does. Drift is now impossible by construction rather
than by agreement. extract_meta is reverted to its original shape (it never
needed to change), and the shared stem_default_on helper stays as the one place
`default: off` is resolved.

Tests rewritten to compare against load_song — the WS's own function — for a
reserved-`full` pack, a LEGACY original_audio pack (the case that was broken), and
a single-`full` pack. Also documents the `?stems=1` contract in CHANGELOG.md.
Full suite green.
This commit is contained in:
byrongamatos
2026-07-15 00:17:35 +02:00
parent b7b28deb53
commit 98a270255b
4 changed files with 124 additions and 116 deletions
+83 -74
View File
@@ -21,60 +21,52 @@ import yaml
import sloppak
def _pak(tmp_path, stems, full=None, name="song.feedpak"):
def _pak(tmp_path, stems, full=None, name="song.feedpak", original_audio=None):
manifest = {
"title": "T", "artist": "A", "duration": 10.0,
"arrangements": [],
"stems": stems,
"stems": stems + ([full] if full else []),
}
if full:
manifest["stems"] = stems + [full]
if original_audio:
# The deprecated pre-1.15.0 shape: the mixdown lives outside `stems`.
manifest["original_audio"] = original_audio
p = tmp_path / name
with zipfile.ZipFile(p, "w") as z:
# Real packs carry manifest.yaml — a JSON manifest is not read at all.
z.writestr("manifest.yaml", yaml.safe_dump(manifest))
# _legacy_full_mix only returns a path that actually EXISTS on disk.
if original_audio:
z.writestr(original_audio, b"\0" * 16)
return p
def test_extract_meta_carries_file_and_default(tmp_path):
p = _pak(tmp_path, [
{"id": "guitar", "file": "stems/guitar.ogg"}, # absent => on
{"id": "vocals", "file": "stems/vocals.ogg", "default": False},
{"id": "drums", "file": "stems/drums.ogg", "default": "off"}, # string form
])
meta = sloppak.extract_meta(p)
by_id = {s["id"]: s for s in meta["stems"]}
assert by_id["guitar"]["default"] is True, "absent default means ON"
assert by_id["vocals"]["default"] is False
assert by_id["drums"]["default"] is False, "'off' must be honoured"
assert by_id["guitar"]["file"] == "stems/guitar.ogg"
def _payload(tmp_path, pak):
from routers.song import _playable_stems_payload
import appstate
cache = tmp_path / "cache"
cache.mkdir(exist_ok=True)
appstate.sloppak_cache_dir = cache
return _playable_stems_payload(pak.name, tmp_path)
def test_the_mixdown_is_lifted_out_of_the_stem_list(tmp_path):
# `full` is the mixdown, not a layer (spec 5.3). Listing it beside the
# instruments would make the plugin play the whole song ON TOP of the stems.
p = _pak(tmp_path,
[{"id": "guitar", "file": "stems/guitar.ogg"},
{"id": "bass", "file": "stems/bass.ogg"}],
full={"id": "full", "file": "stems/full.ogg"})
meta = sloppak.extract_meta(p)
ids = [s["id"] for s in meta["stems"]]
assert ids == ["guitar", "bass"], "the mixdown must not be a layer"
assert meta["full_mix_file"] == "stems/full.ogg", "...but it must still be reachable"
def _ws_payload(tmp_path, pak):
"""Rebuild the WS `ready` stems payload exactly as ws_highway.py does."""
from urllib.parse import quote
cache = tmp_path / "cache"
cache.mkdir(exist_ok=True)
loaded = sloppak.load_song(pak.name, tmp_path, cache)
q = quote(pak.name, safe="")
return {
"stems": [
{"id": s["id"], "url": f"/api/sloppak/{q}/file/{quote(s['file'])}",
"default": s["default"]}
for s in loaded.stems
],
"full_mix_url": f"/api/sloppak/{q}/file/{quote(loaded.full_mix)}" if loaded.full_mix else None,
}
def test_a_single_full_pack_keeps_full_as_its_only_stem(tmp_path):
# A pack whose ONLY stem is `full` is a single-mix pack: there is nothing to
# be pristine against, so `full` stays playable and no mixdown is surfaced.
p = _pak(tmp_path, [{"id": "full", "file": "stems/full.ogg"}])
meta = sloppak.extract_meta(p)
assert [s["id"] for s in meta["stems"]] == ["full"]
assert meta["full_mix_file"] is None
def test_default_resolution_is_shared_with_load_song(tmp_path):
# The whole point: REST and the WS must not drift. Both go through
# stem_default_on, so pin the helper's contract directly.
def test_default_resolution_is_shared_with_load_song():
assert sloppak.stem_default_on(True) is True
assert sloppak.stem_default_on(False) is False
assert sloppak.stem_default_on("off") is False
@@ -85,47 +77,64 @@ def test_default_resolution_is_shared_with_load_song(tmp_path):
assert sloppak.stem_default_on(1) is True
def test_rest_payload_matches_what_the_ws_would_build(tmp_path):
"""The safety property, pinned end to end.
def test_rest_matches_the_ws_for_a_reserved_full_stem(tmp_path):
pak = _pak(tmp_path,
[{"id": "guitar", "file": "stems/guitar.ogg"},
{"id": "vocals", "file": "stems/vocals.ogg", "default": "off"}],
full={"id": "full", "file": "stems/full.ogg"},
name="Iron Maiden - Phantom.feedpak")
rest = _payload(tmp_path, pak)
assert rest == _ws_payload(tmp_path, pak)
assert [s["id"] for s in rest["stems"]] == ["guitar", "vocals"], "the mixdown is not a layer"
assert rest["full_mix_url"].endswith("stems/full.ogg")
assert rest["stems"][1]["default"] is False
Rebuild the WS's stems_payload from load_song exactly as ws_highway does,
and require the REST helper to produce the identical list.
def test_rest_matches_the_ws_for_a_LEGACY_original_audio_pack(tmp_path):
"""The one CodeRabbit caught, and the one that matters most in practice.
load_song falls back to the DEPRECATED `original_audio:` key when a pack has
no reserved `full` stem — which is every pack written before feedpak 1.15.0,
i.e. most of a real library. My first version of this payload reimplemented
the full-mix rule from extract_meta and silently returned None for them: REST
would say "no full mix" while the WS said there was one. The plugin would then
preload a graph WITHOUT the pristine mix and, because the signature still
matched, never rebuild — unity playback silently downgraded to the lossy
recombination.
The payload now calls load_song itself, so this cannot drift. Pinned anyway.
"""
from urllib.parse import quote
from routers.song import _playable_stems_payload
pak = _pak(tmp_path, [
{"id": "guitar", "file": "stems/guitar.ogg"},
{"id": "bass", "file": "stems/bass.ogg"},
], name="Legacy Pack.feedpak", original_audio="original/full.ogg")
p = _pak(tmp_path,
[{"id": "guitar", "file": "stems/guitar.ogg"},
{"id": "vocals", "file": "stems/vocals.ogg", "default": "off"}],
full={"id": "full", "file": "stems/full.ogg"},
name="Iron Maiden - Phantom.feedpak")
cache = tmp_path / "cache"
cache.mkdir()
loaded = sloppak.load_song(p.name, tmp_path, cache)
q_fn = quote(p.name, safe="")
ws_stems = [
{"id": s["id"], "url": f"/api/sloppak/{q_fn}/file/{quote(s['file'])}",
"default": s["default"]}
for s in loaded.stems
]
ws_full = f"/api/sloppak/{q_fn}/file/{quote(loaded.full_mix)}" if loaded.full_mix else None
rest = _playable_stems_payload(p, p.name)
assert rest["stems"] == ws_stems, (
"REST and the WS must publish the SAME stem list — a mismatch means the "
"plugin preloads a graph and then rebuilds it, which is worse than not "
"preloading at all"
rest = _payload(tmp_path, pak)
assert rest == _ws_payload(tmp_path, pak)
assert rest["full_mix_url"] is not None, (
"a pre-1.15.0 pack's full mix must survive — dropping it downgrades unity "
"playback to the lossy stem recombination, silently"
)
assert rest["full_mix_url"] == ws_full
assert rest["full_mix_url"].endswith("original/full.ogg")
def test_rest_matches_the_ws_for_a_single_full_pack(tmp_path):
# Its ONE stem IS the mixdown: nothing to be pristine against, so `full` stays
# the sole playable stem and no separate mixdown is surfaced.
pak = _pak(tmp_path, [{"id": "full", "file": "stems/full.ogg"}], name="Single.feedpak")
rest = _payload(tmp_path, pak)
assert rest == _ws_payload(tmp_path, pak)
assert [s["id"] for s in rest["stems"]] == ["full"]
assert rest["full_mix_url"] is None
def test_a_broken_pack_yields_an_empty_list_not_an_error(tmp_path):
# Preloading is an optimisation. A pack we cannot read must fall back to the
# Preloading is an optimisation: an unreadable pack must fall back to the
# normal WS-driven path, never break the song-info request.
from routers.song import _playable_stems_payload
bad = tmp_path / "bad.feedpak"
bad.write_bytes(b"not a zip")
assert _playable_stems_payload(bad, "bad.feedpak") == {"stems": [], "full_mix_url": None}
import appstate
cache = tmp_path / "cache"
cache.mkdir()
appstate.sloppak_cache_dir = cache
(tmp_path / "bad.feedpak").write_bytes(b"not a zip")
assert _playable_stems_payload("bad.feedpak", tmp_path) == {"stems": [], "full_mix_url": None}