From d876ded00f9254b13bf4a0587c5645a1edaccf67 Mon Sep 17 00:00:00 2001 From: Byron Gamatos Date: Mon, 13 Jul 2026 17:13:08 +0200 Subject: [PATCH] fix(sloppak): bound the unpack cache; add read_member_bytes() so callers stop unpacking whole songs (#950) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(sloppak): bound the unpack cache, and add a way to read a song without unpacking it A tester's sloppak_cache reached 60 GB from an 1800-song library — his entire library, unpacked, none of it played. Stems are already-compressed audio, so an unpacked pack is ~1.1x its zip: the cache is a second, DECOMPRESSED copy of every song it touches. It had no size cap, no LRU, and no cleanup of any kind — not even when the song itself was deleted. Two halves: 1. resolve_source_dir() now evicts least-recently-used songs to stay under a cap (FEEDBACK_SLOPPAK_CACHE_MAX_MB, default 4 GB ≈ 130 songs of recency; 0 disables). The sweep runs on unpack — the only moment the cache grows — so it can't drift. An evicted song is dropped from _source_cache too: get_cached_source_dir() is the only thing media.py consults before falling back, so a stale path there would 404 every stem for the rest of the process instead of re-unpacking. get_cached_source_dir() now also verifies the dir still exists, which makes "just delete sloppak_cache/ to reclaim disk" safe advice. 2. read_member_bytes() reads ONE file out of a pack without unpacking it — the same trick read_cover_bytes() uses so the library grid doesn't explode every pack to show a cover. Unpacking a whole song to read a few KB of JSON is ~45x write amplification; doing it in a loop over the library is what produced the 60 GB. rig_builder's library-wide tone batch is the caller that did exactly that (fixed separately); this gives it, and everyone else, the right primitive. Eviction is concurrency-safe: unpacks run 2-at-a-time, so a dir being written is marked in-flight and the sweep skips it — checked and rmtree'd under one hold of the guard, and the marker is released even if the unpack raises (a leaked marker would make that dir permanently un-evictable). read_member_bytes normalizes both the requested path AND the archive's stored member names through safe_join, taking the last match — so './arrangements/x.json', backslash members from Windows tooling, and duplicate members that normalize to the same path all read back exactly as unpack-then-read did. Zip-slip is rejected before anything is opened. Tests: tests/test_sloppak_unpack_cache.py. All bite-tested (reverted each fix, watched it fail) — including one that was passing vacuously: a freshly-unpacked dir is the most-recently-used, so the LRU never reaches it and the in-flight race test proved nothing until the packs were sized to force the sweep that far. * test: split semicolon-joined statements (E702) CodeRabbit on #950. Style only; no behaviour change. --- lib/sloppak.py | 284 +++++++++++++++++++++++--- tests/test_sloppak_unpack_cache.py | 309 +++++++++++++++++++++++++++++ 2 files changed, 565 insertions(+), 28 deletions(-) create mode 100644 tests/test_sloppak_unpack_cache.py diff --git a/lib/sloppak.py b/lib/sloppak.py index 798087d..1149369 100644 --- a/lib/sloppak.py +++ b/lib/sloppak.py @@ -15,6 +15,7 @@ from __future__ import annotations import logging import math +import os import shutil import threading import zipfile @@ -81,6 +82,116 @@ _unpack_semaphore = threading.BoundedSemaphore(_UNPACK_MAX_CONCURRENCY) _unpack_locks: dict[str, threading.Lock] = {} _unpack_locks_guard = threading.Lock() +# Destinations with an unpack in flight right now. Eviction MUST skip these: two +# unpacks run concurrently, so one finishing could otherwise rmtree the other's +# half-written directory and leave that resolver caching an incomplete song. +_unpacking: set[Path] = set() +_unpacking_guard = threading.Lock() + +# Cap the unpack cache. Stems are already-compressed audio, so an unpacked song +# is ~1.1x its zip — the cache is effectively a second, DECOMPRESSED copy of +# every song it holds, and it used to grow without any bound at all. A tester +# reached 60 GB from a 1800-song library: their whole library, unpacked, because +# one caller looped the library calling load_song(). Nothing ever deleted any of +# it — not even when the song itself was deleted. +# +# Default 4 GB ≈ 130 average songs of recency, which is far more than the "the +# song I'm playing, and the last few I played" that this cache actually exists +# to serve. Override with FEEDBACK_SLOPPAK_CACHE_MAX_MB (0 disables eviction). +def _unpack_cache_cap_bytes() -> int: + raw = os.environ.get("FEEDBACK_SLOPPAK_CACHE_MAX_MB", "").strip() + try: + mb = int(raw) if raw else 4096 + except ValueError: + mb = 4096 + return max(0, mb) * 1024 * 1024 + + +def _dir_size(path: Path) -> int: + total = 0 + for f in path.rglob("*"): + try: + if f.is_file(): + total += f.stat().st_size + except OSError: + continue + return total + + +def _touch(path: Path) -> None: + """Bump mtime so the LRU sweep below treats this song as recently used. + + Reading files out of an unpacked dir doesn't change the DIRECTORY's mtime, + so without this the song you are actively playing looks as stale as one you + unpacked days ago — and a burst of unpacks could evict it mid-song. + """ + try: + os.utime(path, None) + except OSError: + pass + + +def _evict_unpack_cache(root: Path, keep: Path | None = None) -> None: + """Bound the unpack cache: drop least-recently-used songs until under the cap. + + `keep` is never evicted — it's the song the caller just resolved, i.e. almost + certainly the one about to be played. + + Evicting a directory MUST also drop its `_source_cache` entry. Otherwise + get_cached_source_dir() keeps handing out a path that no longer exists and + the media route 404s on every stem instead of re-unpacking (it only falls + back to resolve_source_dir when the cache returns None). + """ + cap = _unpack_cache_cap_bytes() + if cap <= 0: + return + try: + entries = [] + total = 0 + for d in root.iterdir(): + if not d.is_dir(): + continue + try: + size = _dir_size(d) + mtime = d.stat().st_mtime + except OSError: + continue + entries.append((mtime, size, d)) + total += size + if total <= cap: + return + + keep_resolved = keep.resolve() if keep else None + entries.sort(key=lambda e: e[0]) # oldest first + for _mtime, size, d in entries: + if total <= cap: + break + try: + if keep_resolved and d.resolve() == keep_resolved: + continue + except OSError: + continue + # Check-and-delete under ONE hold of the guard. Releasing between the + # two would let a resolver mark this dest in-flight and start writing + # into it in the gap, and we'd rmtree a song mid-unpack. A resolver + # that blocks here simply proceeds afterwards — _unpack_zip recreates + # the directory anyway. + with _unpacking_guard: + if d in _unpacking: + continue # another thread is writing this + shutil.rmtree(d, ignore_errors=True) + if d.exists(): + continue # couldn't remove — don't claim the bytes back + total -= size + with _source_lock: + for fn, (cached_dir, _m, _s) in list(_source_cache.items()): + if cached_dir == d: + _source_cache.pop(fn, None) + log.info("sloppak: evicted %s from the unpack cache (%.0f MB)", + d.name, size / 1e6) + except OSError: + log.warning("sloppak: unpack-cache eviction failed", exc_info=True) + def _unpack_lock_for(filename: str) -> threading.Lock: """Return a stable per-file lock so concurrent unpacks of the same sloppak @@ -145,10 +256,17 @@ def resolve_source_dir( re-unpacks if mtime/size changed, then returns that dir. Caches the resolution so subsequent calls are ~free. + + NOTE: this writes the WHOLE pack — every stem — to disk. Only call it for a + song you are about to play. To read a *part* of a song (an arrangement, the + lyrics, a tone blob), use read_member_bytes(): unpacking a pack to read a few + KB of JSON is ~45x write amplification, and doing it in a loop over the + library fills the disk with a decompressed copy of every song. """ path = dlc_root / filename stat = path.stat() mtime, size = stat.st_mtime, stat.st_size + guarded: Path | None = None # a dir WE unpacked, shielded from eviction with _source_lock: cached = _source_cache.get(filename) @@ -159,42 +277,76 @@ def resolve_source_dir( and cached_size == size and cached_dir.exists() ): + # Mark it recently-used before returning — see _touch(). + if cached_dir != path: + _touch(cached_dir) return cached_dir - if path.is_dir(): - resolved = path - else: - # 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) - 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 + try: + if path.is_dir(): + resolved = path + else: + # 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) + 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: + # Shield `dest` from eviction from the moment we start writing + # until it is safely in _source_cache. `keep` only shields it + # from OUR OWN sweep — a concurrent resolver sweeping with a + # different `keep` would delete it, and we would then cache and + # return a path that no longer exists. The `finally` below + # releases it on EVERY exit, including a failed unpack: leaving + # a dest marked in-flight would make it un-evictable forever. + with _unpacking_guard: + _unpacking.add(dest) + guarded = dest + with _unpack_semaphore: + _unpack_zip(path, dest) + resolved = dest + # The only moment this cache grows. Sweep here rather than on a + # timer so it can never drift far past the cap. + _evict_unpack_cache(unpack_cache_root, keep=dest) - with _source_lock: - _source_cache[filename] = (resolved, mtime, size) - return resolved + with _source_lock: + _source_cache[filename] = (resolved, mtime, size) + return resolved + finally: + if guarded is not None: + with _unpacking_guard: + _unpacking.discard(guarded) def get_cached_source_dir(filename: str) -> Path | None: - """Return the cached source dir for a sloppak if one is known.""" + """Return the cached source dir for a sloppak if one is known AND still there. + + The existence check is load-bearing: callers (media.py) only fall back to + resolve_source_dir() when this returns None, so handing back a path that has + been evicted — or that the user deleted by hand to reclaim disk — would 404 + every stem for the rest of the process instead of re-unpacking. + """ with _source_lock: cached = _source_cache.get(filename) - return cached[0] if cached else None + if not cached: + return None + src = cached[0] + if not src.is_dir(): + _source_cache.pop(filename, None) + return None + _touch(src) + return src # ── Manifest + song loading ─────────────────────────────────────────────────── @@ -233,6 +385,82 @@ def load_manifest(path: Path) -> dict: return _read_manifest_from_zip(path) +_ZIP_ROOT = Path("/_root").resolve() + + +def _zip_member_key(name: str) -> str | None: + """Canonical lookup key for a zip member name, or None if it escapes the root. + + Collapses './', 'a/../b' and backslash separators — the same normalization + _unpack_zip()/safe_join() apply when extracting. Both the name the caller asks + for AND the names the archive actually stores must go through this, or a pack + that stores './arrangements/lead.json' unpacks fine but reads back as missing. + """ + safe = safe_join(_ZIP_ROOT, name or "") + # None → escapes the root; == root → a degenerate name like "." or "a/..". + if safe is None or safe == _ZIP_ROOT: + return None + return safe.relative_to(_ZIP_ROOT).as_posix() + + +def read_member_bytes(path: Path, rel: str) -> bytes | None: + """Return the bytes of ONE file inside a sloppak, or None if it isn't there. + + For a zipped sloppak this opens that single member instead of unpacking the + archive — the same trick read_cover_bytes() uses to keep the library grid + from exploding every pack just to show a cover. + + Reach for this whenever you want a *part* of a song (an arrangement's JSON, + the lyrics, a tone blob) rather than a song you're about to play. The + alternative, load_song(), calls resolve_source_dir() and writes the WHOLE + pack — every stem — into the unpack cache. That is a ~45x write amplification + when all you wanted was a few KB of JSON, and looping the library on it + unpacks the entire library (got-feedBack/feedBack: a tester hit 60 GB that + way). Stems are already-compressed audio, so an unpacked song is ~1.1x its + zip: the cache becomes a second, decompressed copy of everything it touches. + """ + rel = (rel or "").strip() + if not rel: + return None + + if path.is_dir(): + target = safe_join(path.resolve(), rel) + if target is None or not target.is_file(): + return None + try: + return target.read_bytes() + except OSError: + return None + + # Zip form — read just that member, no unpack. Zip-slip is rejected before we + # open anything, and both sides of the comparison are normalized, so a + # non-canonical-but-valid name ('./arrangements/lead.json') resolves the same + # way it did when we unpacked first. + member = _zip_member_key(rel) + if member is None: + log.warning("sloppak: rejected unsafe member name %r in %r", rel, path) + return None + try: + with zipfile.ZipFile(str(path), "r") as zf: + # Match on the NORMALIZED stored name, and take the LAST match — the + # archive may store './x' or a backslash path (Windows tooling), and + # if it stores two names that normalize to the same file, _unpack_zip + # writes them in order so the last one wins. Reading the raw member by + # exact name would miss the first case and return the wrong bytes in + # the second. A pack has a handful of members; the scan is free. + info = None + for cand in zf.infolist(): + if _zip_member_key(cand.filename) == member: + info = cand + if info is None or info.is_dir(): + return None + with zf.open(info) as f: + return f.read() + except (zipfile.BadZipFile, OSError, RuntimeError) as e: + log.warning("sloppak: failed to read %r from %s: %s", rel, path.name, e) + return None + + _COVER_MEDIA_TYPES = { ".jpg": "image/jpeg", ".jpeg": "image/jpeg", ".png": "image/png", ".webp": "image/webp", diff --git a/tests/test_sloppak_unpack_cache.py b/tests/test_sloppak_unpack_cache.py new file mode 100644 index 0000000..dda931f --- /dev/null +++ b/tests/test_sloppak_unpack_cache.py @@ -0,0 +1,309 @@ +"""The unpack cache is bounded, and reading part of a song doesn't explode it. + +`sloppak_cache/` holds every song ever unpacked, fully decompressed. Stems are +already-compressed audio, so an unpacked song is ~1.1x its zip — the cache is a +second copy of the library. It used to have no cap, no LRU, and no cleanup at +all: a tester reached 60 GB from an 1800-song library because one caller looped +the library calling load_song() (rig_builder's library-wide tone batch), which +unpacks the WHOLE pack — stems included — to read a few KB of tone JSON. + +Pins, so neither half can silently come back: + - resolve_source_dir() evicts LRU songs to stay under the cap, + - it never evicts the song the caller just asked for, + - an evicted song is dropped from _source_cache too (otherwise the media route + keeps serving a path that no longer exists and 404s every stem instead of + re-unpacking), + - get_cached_source_dir() self-heals if the cache dir is deleted by hand, + - read_member_bytes() reads one file WITHOUT unpacking anything. +""" + +import importlib +import zipfile + +import pytest +import yaml + +import sloppak as sloppak_mod + + +STEM = b"\x00" * (400 * 1024) # 400 KB of "audio" — the bulk of a real pack +ARR = b'{"tones": {"definitions": [{"Key": "clean"}]}}' + + +def _zip_pack(path, stem_bytes=STEM): + with zipfile.ZipFile(path, "w") as zf: + zf.writestr("manifest.yaml", yaml.safe_dump({ + "title": path.stem, + "arrangements": [{"file": "arrangements/lead.json", "name": "Lead"}], + "stems": [{"id": "full", "file": "stems/audio.ogg"}], + })) + zf.writestr("arrangements/lead.json", ARR) + zf.writestr("stems/audio.ogg", stem_bytes) + return path + + +@pytest.fixture(autouse=True) +def _fresh_module_state(): + # _source_cache is module state and would leak across tests. + importlib.reload(sloppak_mod) + yield + importlib.reload(sloppak_mod) + + +def _cap_mb(monkeypatch, mb): + monkeypatch.setenv("FEEDBACK_SLOPPAK_CACHE_MAX_MB", str(mb)) + + +def test_read_member_bytes_does_not_unpack(tmp_path, monkeypatch): + dlc = tmp_path / "dlc" + dlc.mkdir() + cache = tmp_path / "cache" + cache.mkdir() + pack = _zip_pack(dlc / "song.feedpak") + + data = sloppak_mod.read_member_bytes(pack, "arrangements/lead.json") + + assert data == ARR + assert list(cache.iterdir()) == [], ( + "reading one member must not unpack the pack — this is the whole point: " + "load_song() would have written the 400 KB stem to disk to get 45 bytes of JSON" + ) + + +def test_read_member_bytes_missing_member_is_none(tmp_path): + dlc = tmp_path / "dlc" + dlc.mkdir() + pack = _zip_pack(dlc / "song.feedpak") + assert sloppak_mod.read_member_bytes(pack, "arrangements/nope.json") is None + assert sloppak_mod.read_member_bytes(pack, "") is None + + +def test_unpack_cache_evicts_lru_to_stay_under_cap(tmp_path, monkeypatch): + dlc = tmp_path / "dlc" + dlc.mkdir() + cache = tmp_path / "cache" + cache.mkdir() + _cap_mb(monkeypatch, 1) # 1 MB — holds ~2 of our 400 KB packs + + for i in range(6): + _zip_pack(dlc / f"song{i}.feedpak") + + for i in range(6): + sloppak_mod.resolve_source_dir(f"song{i}.feedpak", dlc, cache) + + total = sum(f.stat().st_size for f in cache.rglob("*") if f.is_file()) + assert total <= 1 * 1024 * 1024, ( + f"unpack cache ran to {total/1e6:.1f} MB against a 1 MB cap — this is the " + "unbounded growth that reached 60 GB in the field" + ) + # The most recent song must survive; the oldest must not. + names = {d.name for d in cache.iterdir()} + assert "song5.feedpak" in names, "the song just resolved must never be evicted" + assert "song0.feedpak" not in names, "the least-recently-used song should go first" + + +def test_eviction_drops_the_source_cache_entry(tmp_path, monkeypatch): + """An evicted song must not keep being handed out by get_cached_source_dir(). + + media.py only falls back to resolve_source_dir() when this returns None. If a + stale path survives, every stem 404s for the rest of the process instead of + re-unpacking — a silently broken song, not a slow one. + """ + dlc = tmp_path / "dlc" + dlc.mkdir() + cache = tmp_path / "cache" + cache.mkdir() + _cap_mb(monkeypatch, 1) + + for i in range(6): + _zip_pack(dlc / f"song{i}.feedpak") + for i in range(6): + sloppak_mod.resolve_source_dir(f"song{i}.feedpak", dlc, cache) + + evicted = sloppak_mod.get_cached_source_dir("song0.feedpak") + assert evicted is None, "an evicted song must be dropped from _source_cache" + + # ...and asking for it again just re-unpacks it. Self-healing, not broken. + again = sloppak_mod.resolve_source_dir("song0.feedpak", dlc, cache) + assert (again / "stems" / "audio.ogg").is_file() + + +def test_get_cached_source_dir_self_heals_after_manual_delete(tmp_path, monkeypatch): + """Telling a user to delete sloppak_cache/ to reclaim disk must be safe.""" + import shutil + dlc = tmp_path / "dlc" + dlc.mkdir() + cache = tmp_path / "cache" + cache.mkdir() + _cap_mb(monkeypatch, 0) # eviction off — isolate the delete + _zip_pack(dlc / "song.feedpak") + + src = sloppak_mod.resolve_source_dir("song.feedpak", dlc, cache) + assert sloppak_mod.get_cached_source_dir("song.feedpak") == src + + shutil.rmtree(src) # the user clears the folder + + assert sloppak_mod.get_cached_source_dir("song.feedpak") is None, ( + "a path that no longer exists must not be served — the caller would 404 " + "every stem instead of re-unpacking" + ) + assert (sloppak_mod.resolve_source_dir("song.feedpak", dlc, cache) + / "stems" / "audio.ogg").is_file() + + +def test_cap_of_zero_disables_eviction(tmp_path, monkeypatch): + dlc = tmp_path / "dlc" + dlc.mkdir() + cache = tmp_path / "cache" + cache.mkdir() + _cap_mb(monkeypatch, 0) + for i in range(4): + _zip_pack(dlc / f"song{i}.feedpak") + for i in range(4): + sloppak_mod.resolve_source_dir(f"song{i}.feedpak", dlc, cache) + assert len(list(cache.iterdir())) == 4, "cap 0 must mean 'never evict'" + + +def test_read_member_bytes_normalizes_non_canonical_names(tmp_path): + """A manifest may name a member './arrangements/lead.json' — valid, and it + resolved fine once unpacked. Reading the zip member by the raw string would + KeyError and silently report no tones. Same trap read_cover_bytes already hit.""" + dlc = tmp_path / "dlc" + dlc.mkdir() + pack = _zip_pack(dlc / "song.feedpak") + + assert sloppak_mod.read_member_bytes(pack, "./arrangements/lead.json") == ARR + assert sloppak_mod.read_member_bytes(pack, "stems/../arrangements/lead.json") == ARR + assert sloppak_mod.read_member_bytes(pack, "arrangements\\lead.json") == ARR + + +def test_read_member_bytes_rejects_zip_slip(tmp_path): + dlc = tmp_path / "dlc" + dlc.mkdir() + pack = _zip_pack(dlc / "song.feedpak") + assert sloppak_mod.read_member_bytes(pack, "../../etc/passwd") is None + assert sloppak_mod.read_member_bytes(pack, "/etc/passwd") is None + assert sloppak_mod.read_member_bytes(pack, ".") is None + + +def test_eviction_never_deletes_an_in_flight_unpack(tmp_path, monkeypatch): + """Two unpacks run concurrently (_UNPACK_MAX_CONCURRENCY = 2). One finishing + must not rmtree the other's half-written dir — that resolver would then cache + an incomplete song and serve a broken pack. + + Sized so the sweep genuinely has to reach the in-flight directory: each pack + is ~700 KB against a 1 MB cap, so once `keep` is protected the sweep must + delete EVERY other dir to get under the cap — including the one being written. + (A naive version of this test passes even without the guard, because a + freshly-created dir is the most-recently-used and the sweep never gets to it.) + """ + import threading + + big = b"\x00" * (700 * 1024) + dlc = tmp_path / "dlc" + dlc.mkdir() + cache = tmp_path / "cache" + cache.mkdir() + _cap_mb(monkeypatch, 1) + for i in range(3): + _zip_pack(dlc / f"song{i}.feedpak", stem_bytes=big) + + victim = cache / "song2.feedpak" + started = threading.Event() + release = threading.Event() + real_unpack = sloppak_mod._unpack_zip + + def slow_unpack(zip_path, dest): + real_unpack(zip_path, dest) # dir now exists — "half written" + if dest == victim: + started.set() + release.wait(5) # hold it open while the other sweeps + + monkeypatch.setattr(sloppak_mod, "_unpack_zip", slow_unpack) + + t = threading.Thread(target=sloppak_mod.resolve_source_dir, + args=("song2.feedpak", dlc, cache)) + t.start() + assert started.wait(5), "victim unpack did not start" + + # song0 lands and sweeps: keep=song0, cache holds song0+song2 = 1.4 MB > 1 MB, + # so the sweep MUST try to delete song2 — which is still being written. + sloppak_mod.resolve_source_dir("song0.feedpak", dlc, cache) + in_flight_survived = victim.is_dir() + + release.set() + t.join(5) + + assert in_flight_survived, ( + "eviction deleted a directory another thread was still unpacking into — " + "that resolver caches an incomplete song and serves a broken pack" + ) + + +def test_read_member_bytes_finds_backslash_members(tmp_path): + """Windows-authored packs store members as 'arrangements\\lead.json'. + _unpack_zip() normalizes those on extract, so unpack-then-read found them. + An exact getinfo() would not — and we'd silently report the song has no tones.""" + dlc = tmp_path / "dlc" + dlc.mkdir() + pack = dlc / "win.feedpak" + with zipfile.ZipFile(pack, "w") as zf: + zf.writestr("manifest.yaml", yaml.safe_dump({"title": "w"})) + zf.writestr("arrangements\\lead.json", ARR) # backslash member name + + assert sloppak_mod.read_member_bytes(pack, "arrangements/lead.json") == ARR + + +def test_read_member_bytes_finds_non_canonical_STORED_names(tmp_path): + """The archive itself may store './arrangements/lead.json'. _unpack_zip() + normalizes stored names on extract, so unpack-then-read resolved it. Both the + requested path and the stored name must be normalized, or the tones vanish.""" + dlc = tmp_path / "dlc" + dlc.mkdir() + pack = dlc / "odd.feedpak" + with zipfile.ZipFile(pack, "w") as zf: + zf.writestr("manifest.yaml", yaml.safe_dump({"title": "o"})) + zf.writestr("./arrangements/lead.json", ARR) # stored non-canonically + + assert sloppak_mod.read_member_bytes(pack, "arrangements/lead.json") == ARR + + +def test_read_member_bytes_matches_unpack_last_write_wins(tmp_path): + """If a pack stores two names that normalize to the same file, _unpack_zip + writes them in order and the LAST one is what ends up on disk. Reading the + raw member by exact name would hand back the first — stale arrangement data + that no unpacked read would ever have produced.""" + dlc = tmp_path / "dlc" + dlc.mkdir() + pack = dlc / "dupe.feedpak" + with zipfile.ZipFile(pack, "w") as zf: + zf.writestr("manifest.yaml", yaml.safe_dump({"title": "d"})) + zf.writestr("arrangements/lead.json", b'{"tones": {"definitions": [{"Key": "STALE"}]}}') + zf.writestr("./arrangements/lead.json", ARR) # normalizes to the same path + + assert sloppak_mod.read_member_bytes(pack, "arrangements/lead.json") == ARR + + +def test_failed_unpack_does_not_leave_the_dir_un_evictable(tmp_path, monkeypatch): + """A dir marked in-flight is skipped by eviction. If a failed unpack leaves the + marker behind, that dir becomes permanently un-evictable — a slow leak of + exactly the thing this cap exists to prevent.""" + dlc = tmp_path / "dlc" + dlc.mkdir() + cache = tmp_path / "cache" + cache.mkdir() + _cap_mb(monkeypatch, 1) + _zip_pack(dlc / "boom.feedpak") + + def blow_up(zip_path, dest): + dest.mkdir(parents=True, exist_ok=True) + raise OSError("disk full") + + monkeypatch.setattr(sloppak_mod, "_unpack_zip", blow_up) + with pytest.raises(OSError): + sloppak_mod.resolve_source_dir("boom.feedpak", dlc, cache) + + assert not sloppak_mod._unpacking, ( + "a failed unpack left its destination marked in-flight — eviction will " + "skip it forever" + )