mirror of
https://github.com/got-feedBack/feedBack.git
synced 2026-07-22 21:01:40 +00:00
* 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.
310 lines
12 KiB
Python
310 lines
12 KiB
Python
"""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"
|
|
)
|