From 21997f4b5cca0497e98f9d678123673ee5ac9f4c Mon Sep 17 00:00:00 2001 From: Byron Gamatos Date: Sat, 20 Jun 2026 00:22:43 +0200 Subject: [PATCH] Fix slow library cover loading: serve sloppak art without unpacking + revalidated caching (#534) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * sloppak: read cover without unpacking + serialize/cap zip unpacks Album art for a zip-form sloppak was served by resolve_source_dir(), which unpacks the ENTIRE archive (stems included, ~30 MB) to disk just to read cover.jpg. On the library grid that meant a full extraction per card on scroll. - read_cover_bytes(): opens only the cover member from the zip (or reads the file for dir-form), with zip-slip guarding. ~4 ms vs a full unpack. - resolve_source_dir(): per-file lock + bounded global semaphore so concurrent callers don't rmtree + re-extract the same dest at once (a race), and a burst can't saturate disk/CPU. 8 concurrent calls now dedupe to 1 unpack. Co-Authored-By: Claude Opus 4.8 (1M context) * server: serve sloppak art via read_cover_bytes + cache album-art responses - get_song_art() sloppak branch now reads the cover directly (no full unpack), off-thread via asyncio.to_thread. - All art responses carry Cache-Control: public, max-age=86400. URLs are already cache-busted with ?v=, so the browser stops re-fetching every cover on scroll-back; day bound self-heals any URL missing ?v. Co-Authored-By: Claude Opus 4.8 (1M context) * v3 library: lazy-load + async-decode card cover images The grid (24 cards/page) and artist-row thumbnails emitted plain with no loading hint, so a whole page of covers fetched + decoded at once on each scroll batch. Add loading="lazy" decoding="async" to defer off-screen fetches and keep image decode off the main thread. Co-Authored-By: Claude Opus 4.8 (1M context) * address Codex review: zip cover normalization + correct art revalidation Findings from the preflight Codex passes: - sloppak.read_cover_bytes (zip form) read the raw manifest cover string via zf.read(), so a non-canonical name like './cover.jpg' or 'art/../cover.jpg' 404'd. Normalize via safe_join → relative member; reject escape and the degenerate root-collapse case ('.', 'subdir/..') like _unpack_zip does. - Album-art caching is correctness-first: Cache-Control: no-cache plus a strong validator, with real conditional handling (Starlette FileResponse emits an ETag but doesn't evaluate If-None-Match). All three art paths route through _art_conditional/_file_art_response → bodyless 304 on a matching validator. A long immutable max-age was rejected because the frontend ?v= buster is only second-resolution and would pin a same-second rewrite. - The sloppak cover is validated by CONTENT (sha1 of the bytes), not a stat: a dir-form sloppak edited in place changes the cover file's mtime but not the directory's, so a dir-stat ETag could emit a stale 304. Content hashing is correct for both dir- and zip-form. get_song_art gained an optional request (internal get_art caller passes none — safe). Adds tests/test_sloppak_cover_art.py pinning read_cover_bytes (canonical, non-canonical, degenerate/escape, dir/zip, webp) and the endpoint's 304 contract incl. the dir-form in-place-edit no-stale-304 regression. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- lib/sloppak.py | 110 +++++++++++++++++++++++- server.py | 107 ++++++++++++++++++----- static/v3/songs.js | 4 +- tests/test_sloppak_cover_art.py | 148 ++++++++++++++++++++++++++++++++ 4 files changed, 341 insertions(+), 28 deletions(-) create mode 100644 tests/test_sloppak_cover_art.py diff --git a/lib/sloppak.py b/lib/sloppak.py index 0919fac..ac50785 100644 --- a/lib/sloppak.py +++ b/lib/sloppak.py @@ -54,6 +54,27 @@ def is_sloppak(path: Path) -> bool: _source_cache: dict[str, tuple[Path, float, int]] = {} _source_lock = threading.Lock() +# Full-archive unpacks (zip form) are expensive — they write every stem to +# disk. Cap how many run at once so a burst (e.g. many plays queued, or a stray +# caller looping the library) can't saturate disk/CPU, and serialize per-file so +# two callers never rmtree + re-extract the same dest simultaneously (which +# would corrupt the half-written dir the other is reading). +_UNPACK_MAX_CONCURRENCY = 2 +_unpack_semaphore = threading.BoundedSemaphore(_UNPACK_MAX_CONCURRENCY) +_unpack_locks: dict[str, threading.Lock] = {} +_unpack_locks_guard = threading.Lock() + + +def _unpack_lock_for(filename: str) -> threading.Lock: + """Return a stable per-file lock so concurrent unpacks of the same sloppak + serialize instead of racing on the same destination dir.""" + with _unpack_locks_guard: + lk = _unpack_locks.get(filename) + if lk is None: + lk = threading.Lock() + _unpack_locks[filename] = lk + return lk + def _unpack_zip(zip_path: Path, dest: Path) -> None: """Extract a sloppak zip archive into dest, replacing any previous contents. @@ -126,10 +147,26 @@ def resolve_source_dir( if path.is_dir(): resolved = path else: - # Zip form — unpack to the cache. + # Zip form — unpack to the cache. Serialize per-file (so concurrent + # callers don't rmtree + re-extract the same dest at once) and cap + # global unpack concurrency (so a burst can't saturate disk/CPU). dest = unpack_cache_root / _safe_id(filename) - _unpack_zip(path, dest) - resolved = dest + with _unpack_lock_for(filename): + # Re-check the cache inside the per-file lock — a prior holder may + # have just finished unpacking this exact (mtime, size). + with _source_lock: + cached = _source_cache.get(filename) + if ( + cached + and cached[1] == mtime + and cached[2] == size + and cached[0].exists() + ): + resolved = cached[0] + else: + with _unpack_semaphore: + _unpack_zip(path, dest) + resolved = dest with _source_lock: _source_cache[filename] = (resolved, mtime, size) @@ -179,6 +216,73 @@ def load_manifest(path: Path) -> dict: return _read_manifest_from_zip(path) +_COVER_MEDIA_TYPES = { + ".jpg": "image/jpeg", ".jpeg": "image/jpeg", + ".png": "image/png", ".webp": "image/webp", +} + + +def _cover_media_type(name: str) -> str: + return _COVER_MEDIA_TYPES.get(Path(name).suffix.lower(), "image/jpeg") + + +def read_cover_bytes( + path: Path, manifest: dict | None = None +) -> tuple[bytes, str] | None: + """Return ``(image_bytes, media_type)`` for a sloppak's cover, or ``None``. + + Reads ONLY the cover image. For a zipped sloppak this opens the single + cover member rather than unpacking the whole archive (stems included), so + serving album art on the library grid never triggers a full extraction — + the dominant cost behind slow cover loading on scroll. + """ + try: + if manifest is None: + manifest = load_manifest(path) + except Exception: + manifest = {} + cover_rel = str((manifest or {}).get("cover") or "cover.jpg") + + if path.is_dir(): + # Directory form — read the file, guarding against escape. + cover_path = (path / cover_rel).resolve() + try: + cover_path.relative_to(path.resolve()) + except ValueError: + return None + if cover_path.is_file(): + try: + return cover_path.read_bytes(), _cover_media_type(cover_path.name) + except OSError as e: + log.warning("sloppak: failed to read cover %r: %s", cover_path, e) + return None + + # Zip form — read just the cover member, no unpack. Normalize the manifest + # name the way the filesystem would (collapse './' and 'a/../b', backslash → + # slash) so a non-canonical-but-valid cover like './cover.jpg' still resolves + # to the archive member 'cover.jpg' — matching the old unpack-then-resolve + # behavior — and reject zip-slip escape before opening. + _zip_root = Path("/_root").resolve() + safe = safe_join(_zip_root, cover_rel) + # `safe is None` → escape; `safe == _zip_root` → a degenerate name like "." + # or "subdir/.." that collapses to the root (member would be "."). Reject + # both, mirroring _unpack_zip's degenerate-root guard. + if safe is None or safe == _zip_root: + log.warning("sloppak: rejected unsafe cover name %r in %r", cover_rel, path) + return None + member = safe.relative_to(_zip_root).as_posix() + try: + with zipfile.ZipFile(str(path), "r") as zf: + try: + data = zf.read(member) + except KeyError: + return None + return data, _cover_media_type(member) + except (OSError, zipfile.BadZipFile, RuntimeError) as e: + log.warning("sloppak: failed to read cover from zip %r: %s", path, e) + return None + + @dataclass class LoadedSloppak: """Result of loading a sloppak: the Song object plus stem descriptors.""" diff --git a/server.py b/server.py index 5498359..49e2b08 100644 --- a/server.py +++ b/server.py @@ -6298,13 +6298,72 @@ def diagnostics_hardware(): +def _if_none_match_hits(header: str | None, etag: str) -> bool: + """True if an If-None-Match header matches `etag` (weak comparison). + + Handles the `*` wildcard and comma-separated lists, and ignores a weak + `W/` prefix on either side — the standard semantics for a conditional GET. + """ + if not header: + return False + bare = etag.removeprefix("W/") + for tok in header.split(","): + t = tok.strip() + if t == "*" or t.removeprefix("W/") == bare: + return True + return False + + +# Album art is served with a strong validator (an ETag on the sloppak byte +# path; FileResponse's own ETag/Last-Modified on the file paths) and revalidated +# with `no-cache`. That keeps re-scroll cheap — a conditional GET returns a +# bodyless 304 — without ever serving a stale cover. A long `immutable` max-age +# was rejected: the frontend's `?v=` buster is only second-resolution, so +# a same-second cover rewrite would keep the URL and pin the old bytes for the +# cache lifetime. Validation cost is negligible for a localhost backend. +_ART_CACHE_HEADERS = {"Cache-Control": "no-cache"} + + +def _art_etag(path: Path) -> str | None: + """Strong validator for an art file: nanosecond mtime + size (so a + same-second rewrite still changes it). None if the file can't be stat'd.""" + try: + st = path.stat() + return f'"{st.st_mtime_ns}-{st.st_size}"' + except OSError: + return None + + +def _art_conditional(etag: str | None, request: Request | None): + """Return (headers, not_modified) for an art response. `not_modified` is + True when the client's If-None-Match already matches `etag` → caller should + return a bodyless 304. Starlette's FileResponse emits an ETag but does NOT + itself evaluate If-None-Match, so every art path routes through here to get + real conditional handling.""" + headers = dict(_ART_CACHE_HEADERS) + if etag: + headers["ETag"] = etag + inm = request.headers.get("if-none-match") if request is not None else None + return headers, bool(etag) and _if_none_match_hits(inm, etag) + + +def _file_art_response(path: Path, media_type: str, request: Request | None): + """FileResponse for an on-disk art file, with no-cache + ETag and a bodyless + 304 when the client's validator still matches.""" + headers, not_modified = _art_conditional(_art_etag(path), request) + if not_modified: + return Response(status_code=304, headers=headers) + return FileResponse(str(path), media_type=media_type, headers=headers) + + @app.get("/api/song/{filename:path}/art") -async def get_song_art(filename: str): +async def get_song_art(filename: str, request: Request = None): """Serve album art for a song. Dispatches by format and returns the appropriate media type: - - Sloppak: serves `cover.jpg` (or manifest-declared cover) from - the source dir as JPEG/PNG/WebP. + - Sloppak: serves `cover.jpg` (or manifest-declared cover) read directly + from the package (the single cover member for zip-form sloppaks — no + full unpack) as JPEG/PNG/WebP. - Loose folder: serves the discovered art file directly as JPEG/PNG/WebP. """ @@ -6318,27 +6377,29 @@ async def get_song_art(filename: str): if not song_path.exists(): return JSONResponse({"error": "not found"}, 404) - # Sloppak path: pull cover.jpg from the source dir (manifest-declared or default). + # Sloppak path: read the cover (manifest-declared or default) straight from + # the package. For a zip-form sloppak this opens just the cover member — + # NOT the whole archive — so the library grid never triggers a full unpack + # of stems just to paint a thumbnail. if sloppak_mod.is_sloppak(song_path): + # Read the cover (cheap — single member, no full unpack) and validate by + # its CONTENT. A stat-based ETag would be wrong for directory-form + # sloppaks: editing cover.jpg in place changes the file's mtime, not the + # directory's, so a dir-stat ETag could emit a stale 304. Content hashing + # is correct for both dir- and zip-form. Raw byte Response lacks + # FileResponse's validators, so we attach the ETag + honor If-None-Match. try: - src = sloppak_mod.resolve_source_dir(filename, dlc, SLOPPAK_CACHE_DIR) - manifest = sloppak_mod.load_manifest(song_path) - cover_rel = str(manifest.get("cover") or "cover.jpg") - cover_path = (src / cover_rel).resolve() - # Prevent escape and fall back to default name if missing. - try: - cover_path.relative_to(src.resolve()) - except ValueError: - return JSONResponse({"error": "forbidden"}, 403) - if cover_path.exists() and cover_path.is_file(): - mt = { - ".jpg": "image/jpeg", ".jpeg": "image/jpeg", - ".png": "image/png", ".webp": "image/webp", - }.get(cover_path.suffix.lower(), "image/jpeg") - return FileResponse(str(cover_path), media_type=mt) + art = await asyncio.to_thread(sloppak_mod.read_cover_bytes, song_path) except Exception: - pass - return JSONResponse({"error": "no art"}, 404) + art = None + if art is None: + return JSONResponse({"error": "no art"}, 404) + data, mt = art + etag = f'"{hashlib.sha1(data).hexdigest()}"' + headers, not_modified = _art_conditional(etag, request) + if not_modified: + return Response(status_code=304, headers=headers) + return Response(content=data, media_type=mt, headers=headers) # Loose folder path: serve art file directly. # song_path is already validated against DLC_DIR by _resolve_dlc_path. @@ -6358,7 +6419,7 @@ async def get_song_art(filename: str): ".jpg": "image/jpeg", ".jpeg": "image/jpeg", ".png": "image/png", ".webp": "image/webp", }.get(art_resolved.suffix.lower(), "image/jpeg") - return FileResponse(str(art_resolved), media_type=mt) + return _file_art_response(art_resolved, mt, request) return JSONResponse({"error": "no art"}, 404) # Custom art uploaded via /art/upload is cached as PNG under ART_CACHE_DIR; @@ -6367,7 +6428,7 @@ async def get_song_art(filename: str): safe_name = filename.replace("/", "_").replace(" ", "_") cached = art_cache / f"{safe_name}.png" if cached.exists(): - return FileResponse(str(cached), media_type="image/png") + return _file_art_response(cached, "image/png", request) return JSONResponse({"error": "no art"}, 404) diff --git a/static/v3/songs.js b/static/v3/songs.js index 6177ee5..269ef29 100644 --- a/static/v3/songs.js +++ b/static/v3/songs.js @@ -383,7 +383,7 @@ : ''; return '
' + '
' + - '' + + '' + tuning + checkbox + accuracyBadge(key) + fmtBadge(song) + overlay + '
' + inlineBtns + @@ -645,7 +645,7 @@ '
' + esc(al.name || 'Unknown') + '
' + (al.songs || []).map((s) => { const k = cardKey(s); const fl = fmtLabel(s); return ( '
' + - '' + + '' + '' + esc(s.title) + '' + (fl ? '' + fl + '' : '') + (state.accuracy[k] != null ? '' + Math.round(state.accuracy[k] * 100) + '%' : '') + diff --git a/tests/test_sloppak_cover_art.py b/tests/test_sloppak_cover_art.py new file mode 100644 index 0000000..1c2a0fc --- /dev/null +++ b/tests/test_sloppak_cover_art.py @@ -0,0 +1,148 @@ +"""Album-art fast path + conditional-caching contract. + +Covers the library cover-loading perf fix: `sloppak.read_cover_bytes` reads the +cover WITHOUT unpacking the whole archive, and `GET /api/song/{f}/art` serves it +with a content validator so re-scroll gets bodyless 304s — never a stale cover. + +Pins, so a future refactor can't silently reintroduce: + - the full-unpack-per-cover regression (covers served straight from the zip), + - the non-canonical manifest cover name (`./cover.jpg`) 404, + - zip-slip / degenerate cover names, + - dir-form sloppaks emitting a stale 304 after an in-place cover edit. +""" + +import importlib +import sys +import zipfile + +import pytest +import yaml +from fastapi.testclient import TestClient + +import sloppak as sloppak_mod + + +# ── Unit: read_cover_bytes ──────────────────────────────────────────────────── + +def _zip_sloppak(path, cover_name="cover.jpg", manifest_cover="cover.jpg", + cover_bytes=b"\xff\xd8\xff\xe0JPG", with_stem=True): + with zipfile.ZipFile(path, "w") as zf: + zf.writestr("manifest.yaml", yaml.safe_dump({"cover": manifest_cover})) + zf.writestr(cover_name, cover_bytes) + if with_stem: + # A big-ish stem so a regression that unpacks the whole archive + # would be doing real work, not just touching the cover. + zf.writestr("stems/full.ogg", b"OggS" + b"\x00" * 4096) + + +def _dir_sloppak(path, cover_bytes=b"\xff\xd8\xff\xe0JPG"): + path.mkdir(parents=True) + (path / "manifest.yaml").write_text(yaml.safe_dump({"cover": "cover.jpg"})) + (path / "cover.jpg").write_bytes(cover_bytes) + return path + + +def test_read_cover_from_zip(tmp_path): + z = tmp_path / "a.sloppak" + _zip_sloppak(z, cover_bytes=b"\xff\xd8\xff\xe0HELLO") + res = sloppak_mod.read_cover_bytes(z) + assert res is not None + data, mt = res + assert data == b"\xff\xd8\xff\xe0HELLO" + assert mt == "image/jpeg" + + +def test_read_cover_from_dir(tmp_path): + d = _dir_sloppak(tmp_path / "b.sloppak", cover_bytes=b"\xff\xd8\xff\xe0DIR") + res = sloppak_mod.read_cover_bytes(d) + assert res is not None and res[0] == b"\xff\xd8\xff\xe0DIR" and res[1] == "image/jpeg" + + +@pytest.mark.parametrize("manifest_cover", ["./cover.jpg", "art/../cover.jpg"]) +def test_noncanonical_manifest_cover_resolves(tmp_path, manifest_cover): + """A valid-but-non-canonical name must resolve to the real member, matching + the old unpack-then-resolve-on-filesystem behavior.""" + z = tmp_path / "c.sloppak" + _zip_sloppak(z, manifest_cover=manifest_cover, cover_bytes=b"\xff\xd8\xff\xe0X") + res = sloppak_mod.read_cover_bytes(z) + assert res is not None and res[0] == b"\xff\xd8\xff\xe0X" + + +@pytest.mark.parametrize("bad", ["../../escape.png", ".", "subdir/..", "/abs.png", ""]) +def test_unsafe_or_degenerate_cover_name_rejected(tmp_path, bad): + z = tmp_path / "d.sloppak" + # Put a real cover.jpg in the archive; the manifest points at the bad name. + _zip_sloppak(z, manifest_cover=bad if bad else "cover.jpg") + if bad == "": + # Empty falls back to the default cover.jpg (intended contract). + assert sloppak_mod.read_cover_bytes(z) is not None + else: + assert sloppak_mod.read_cover_bytes(z) is None + + +def test_webp_media_type(tmp_path): + z = tmp_path / "e.sloppak" + _zip_sloppak(z, cover_name="cover.webp", manifest_cover="cover.webp", + cover_bytes=b"RIFF....WEBP") + res = sloppak_mod.read_cover_bytes(z) + assert res is not None and res[1] == "image/webp" + + +# ── Endpoint: conditional caching ───────────────────────────────────────────── + +@pytest.fixture() +def dlc_client(tmp_path, monkeypatch): + """TestClient with a temp DLC_DIR; sync startup, no scan, no plugins.""" + dlc = tmp_path / "dlc" + dlc.mkdir() + config = tmp_path / "cfg" + config.mkdir() + monkeypatch.setenv("DLC_DIR", str(dlc)) + monkeypatch.setenv("CONFIG_DIR", str(config)) + monkeypatch.setenv("SLOPSMITH_SYNC_STARTUP", "1") + sys.modules.pop("server", None) + server = importlib.import_module("server") + server.sloppak_mod._source_cache.clear() + monkeypatch.setattr(server, "load_plugins", lambda *a, **kw: None) + monkeypatch.setattr(server, "startup_scan", lambda: None) + static_tmp = tmp_path / "static" + static_tmp.mkdir() + monkeypatch.setattr(server, "STATIC_DIR", static_tmp) + tc = TestClient(server.app, client=("127.0.0.1", 50000)) + try: + yield tc, server, dlc + finally: + tc.close() + meta_db = getattr(server, "meta_db", None) + conn = getattr(meta_db, "conn", None) + if conn is not None: + conn.close() + + +def test_zip_art_endpoint_conditional_304(dlc_client): + tc, _server, dlc = dlc_client + _zip_sloppak(dlc / "song.sloppak", cover_bytes=b"\xff\xd8\xff\xe0ZIP") + r1 = tc.get("/api/song/song.sloppak/art") + assert r1.status_code == 200 + assert r1.content == b"\xff\xd8\xff\xe0ZIP" + assert r1.headers["cache-control"] == "no-cache" + etag = r1.headers["etag"] + assert etag + r2 = tc.get("/api/song/song.sloppak/art", headers={"If-None-Match": etag}) + assert r2.status_code == 304 + assert r2.content == b"" + + +def test_dir_art_endpoint_no_stale_304_after_inplace_edit(dlc_client): + """Editing cover.jpg in place must invalidate the validator (the dir-form + staleness bug: a dir-stat ETag would wrongly 304 here).""" + tc, _server, dlc = dlc_client + pak = _dir_sloppak(dlc / "dir.sloppak", cover_bytes=b"\xff\xd8\xff\xe0OLD") + r1 = tc.get("/api/song/dir.sloppak/art") + assert r1.status_code == 200 and r1.content == b"\xff\xd8\xff\xe0OLD" + etag_old = r1.headers["etag"] + # Replace the cover content in place (same path). + (pak / "cover.jpg").write_bytes(b"\xff\xd8\xff\xe0NEW") + r2 = tc.get("/api/song/dir.sloppak/art", headers={"If-None-Match": etag_old}) + assert r2.status_code == 200 + assert r2.content == b"\xff\xd8\xff\xe0NEW"