From 1702afa3793ee249625256410e4d53c269cb7d29 Mon Sep 17 00:00:00 2001 From: Byron Gamatos Date: Wed, 15 Jul 2026 00:36:20 +0200 Subject: [PATCH] feat(career): extract the whole setlist before the gig starts (no more waiting between songs) (#971) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(career): extract the whole setlist before the gig starts A feedpak is a zip, and the first play of one pays for its extraction into sloppak_cache. Inside a set that cost landed BETWEEN songs: the player finished a number and then sat there waiting for the next one to unpack, mid-gig. A setlist is a known list up front, so unpack it all while the poster is still on screen. New POST /gigs/prepare walks the set through resolve_source_dir; the poster's Play button shows "Preparing set…" while it runs. Best-effort by design, at every level: - a corrupt pak in the set does not sink the prepare (it is reported in `failed`; the play itself surfaces the error exactly as it does outside a gig — slow beats blocked) - a host without the library resolvers degrades to a no-op rather than 500 - a failed request just falls through to the old lazy extraction Ordering matters and is pinned: the set is unpacked BEFORE the stage is borrowed (venue/viz overwritten) and before the queue starts, so a proposal cancelled while unpacking leaves nothing half-applied to unwind. Tests unpack REAL zips rather than mocking the extractor: every song of the set lands on disk before the first note, a re-prepare does not duplicate the unpack, one bad pak still leaves the good one prepared, and no-library / empty-setlist degrade cleanly. 18/18. NB the other half of the gig report — the per-song results popup interrupting the set (and worse, claimAutoExit'ing so the queue would not advance until it was dismissed) — is fixed in the note_detect plugin repo, which is not part of this checkout. * fix(career): bound the prepare request; validate the setlist (PR #971 review) Both CodeRabbit findings were right. 1. A HUNG PREPARE COULD BLOCK THE GIG FOREVER. `await fetch(...)` only rejects on a network ERROR. A server that accepts the connection and then never answers hangs indefinitely — and the gig would never start. That makes this optimisation the exact thing the PR promises it can never be: the reason you cannot play. The request is now bounded by an AbortController (PREPARE_TIMEOUT_MS, generous because unpacking a setlist is real work — but a CEILING, not a wait). Past it we start the gig and let the first play extract lazily, as it always did. The Play button is restored in a `finally`, so a timeout cannot strand the poster on "Preparing set…" with Play disabled — which would have been the same bug wearing a different hat. 2. THE `songs` BODY WAS UNVALIDATED. A str is iterable: "abc" would have prepared three one-character "songs". And the endpoint unpacks zips, so an arbitrary caller could ask for unbounded work. Now list-only, string entries, blanks dropped, capped at MAX_GIG_SONGS. Tests: the fetch is abortable and the button is re-enabled on EVERY path including the abort; non-list bodies, non-string/blank entries, and an oversized setlist. 50 career tests, JS 5/5, eslint clean. * fix(career): path-traversal guard on prepare; a cap test that actually tests the cap CodeRabbit again, and the first one is a real hole I put there. 1. PATH TRAVERSAL. sloppak.resolve_source_dir() does a bare `dlc_root / filename` with NO containment guard — so `../../x` walks straight out of the library, and my new endpoint handed it attacker-supplied filenames. Every filename now goes through _resolve_dlc_path first, the same check every other filename-bound handler applies. Pinned: `..`, backslash traversal, an absolute POSIX path and a Windows drive path are all refused, and nothing outside the library is unpacked. 2. THE CAP TEST WAS VACUOUS. It asserted `prepared == 0` against a fixture with no library — where the endpoint exits before extraction — so it passed whether or not MAX_GIG_SONGS existed. It now runs against a real library and asserts the endpoint CONSIDERED at most MAX_GIG_SONGS of the 82 it was handed. Verified to fail when the cap is removed. Same class of mistake as the notedetect gigBlock: a test that passes for the wrong reason. Worth saying out loud since it is twice in one day. 3. E702 — semicolon-joined statements in the new tests, split. 51 career tests; full suite green. --- plugins/career/routes.py | 61 ++++++++++ plugins/career/screen.js | 50 +++++++- tests/js/gig_song_end.test.js | 98 ++++++++++++++++ tests/plugins/career/test_routes.py | 173 ++++++++++++++++++++++++++++ 4 files changed, 380 insertions(+), 2 deletions(-) create mode 100644 tests/js/gig_song_end.test.js diff --git a/plugins/career/routes.py b/plugins/career/routes.py index 90be728..7f8b423 100644 --- a/plugins/career/routes.py +++ b/plugins/career/routes.py @@ -46,6 +46,8 @@ from pathlib import Path from fastapi import Body, HTTPException from fastapi.responses import FileResponse +import sloppak +from dlc_paths import _resolve_dlc_path from progression import instrument_for_arrangement PLUGIN_ID = "career" @@ -53,6 +55,9 @@ VENUE_ID_RE = re.compile(r"^[a-z0-9_-]{1,40}$") PACK_FILENAME_RE = re.compile(r"^[a-z0-9_-]{1,64}\.(mp4|webm|mp3|json)$") REQUIRED_LOOPS = ("bored", "neutral", "engaged", "ecstatic") DOWNLOAD_CHUNK = 1024 * 256 +# A setlist is a handful of songs; this endpoint unpacks zips, so cap the work an +# arbitrary caller can ask for. +MAX_GIG_SONGS = 32 _lock = threading.Lock() _state = { @@ -734,6 +739,62 @@ def setup(app, context): "snapshot": snapshot}) return {"ok": True} + @app.post(f"/api/plugins/{PLUGIN_ID}/gigs/prepare") + def prepare_gig(body: dict = Body(...)): + """Unpack every song of the set BEFORE the gig starts. + + A feedpak is a zip: the first play of one pays for its extraction into + sloppak_cache. Inside a set that cost landed BETWEEN songs — the player + finished a number and then sat waiting for the next one to unpack, mid- + gig. A set is a known list up front, so extract it all while the player + is still looking at the poster. + + Idempotent and cheap on a warm cache: resolve_source_dir() returns the + already-unpacked dir without rewriting it. Best-effort per song — one + bad feedpak must not block the set from starting (the play itself will + surface the error, exactly as it does outside a gig). + """ + raw = (body or {}).get("songs") + # A str is iterable: without the list check, "abc" would prepare three + # one-character "songs". Cap the count too — this endpoint unpacks zips, + # so an oversized list is real work, and a setlist is a handful of songs. + if not isinstance(raw, list): + return {"ok": True, "prepared": 0, "failed": []} + files = [f for f in raw if isinstance(f, str) and f.strip()][:MAX_GIG_SONGS] + if not files: + return {"ok": True, "prepared": 0, "failed": []} + + # .get, not []: a host that doesn't hand us the resolvers (or has no + # library configured) must degrade to "extract lazily, as before" — this + # is an optimisation, and it is never allowed to be the thing that stops + # a gig from starting. + get_dlc = context.get("get_dlc_dir") + get_cache = context.get("get_sloppak_cache_dir") + dlc_root = get_dlc() if callable(get_dlc) else None + cache_root = get_cache() if callable(get_cache) else None + if dlc_root is None or cache_root is None: + return {"ok": False, "prepared": 0, "failed": files, "error": "no library"} + + root = Path(dlc_root) + prepared, failed = 0, [] + for fn in files: + # CONTAINMENT FIRST. resolve_source_dir() does a bare + # `dlc_root / filename` with no guard, so a crafted `../..` would + # walk straight out of the library. Every other filename-bound + # handler validates through _resolve_dlc_path; so does this one. + safe = _resolve_dlc_path(root, fn) + if safe is None: + _state["log"].warning("career: gig pre-extract rejected unsafe path %r", fn) + failed.append(fn) + continue + try: + sloppak.resolve_source_dir(fn, root, Path(cache_root)) + prepared += 1 + except Exception as exc: # noqa: BLE001 — one bad pak can't sink the set + _state["log"].warning("career: gig pre-extract failed for %s: %s", fn, exc) + failed.append(fn) + return {"ok": True, "prepared": prepared, "failed": failed} + @app.post(f"/api/plugins/{PLUGIN_ID}/gigs/propose") def propose_gig(body: dict = Body(...)): inst = str((body or {}).get("instrument") or "") diff --git a/plugins/career/screen.js b/plugins/career/screen.js index c981625..7600735 100644 --- a/plugins/career/screen.js +++ b/plugins/career/screen.js @@ -12,6 +12,10 @@ 'use strict'; const API = '/api/plugins/career'; + // Unpacking a setlist is real work (zips, possibly on a slow/network drive), + // so this is generous — but it is a CEILING, not a wait. Past it we start the + // gig and let the first play extract lazily, as it always did. + const PREPARE_TIMEOUT_MS = 60000; const VENUE_OVERRIDE_KEY = 'feedBack-career-venue'; const NO_VENUE = '__none__'; const PREV_VIZ_KEY = 'feedBack-career-prev-viz'; @@ -1123,10 +1127,52 @@ sfx('page'); } - function startGig() { + // Unpack the whole set before the first note. + // + // A feedpak is a zip, and the first play of one pays for its extraction. In + // a set that cost landed BETWEEN songs: the player finished a number and + // then sat there waiting for the next one to unpack, mid-gig. The setlist is + // known up front, so warm it all while the poster is still on screen. + // + // Best-effort by design: a library that won't pre-extract must not stop the + // gig from starting — the play itself surfaces the error the same way it + // does outside a gig. Slow is better than blocked. + async function prepareGigSongs(prop, btn) { + const label = btn && btn.textContent; + if (btn) { btn.disabled = true; btn.textContent = 'Preparing set…'; } + // A bare `await fetch(...)` only rejects on a network ERROR — a server + // that accepts the connection and then never answers hangs forever, and + // the gig would never start. That would make this optimisation the very + // thing it promises never to be: the reason you cannot play. Give up + // waiting and let the first play extract lazily, exactly as before. + const ctrl = new AbortController(); + const timer = setTimeout(() => ctrl.abort(), PREPARE_TIMEOUT_MS); + try { + await fetch(`${API}/gigs/prepare`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ songs: prop.songs.map((s) => s.filename) }), + signal: ctrl.signal, + }); + } catch (_) { + // abort, offline, non-2xx — all the same: start the gig anyway. + } finally { + clearTimeout(timer); + if (btn) { btn.disabled = false; if (label) btn.textContent = label; } + } + } + + async function startGig(btn) { const prop = _ppGigProposal; const q = window.feedBack && window.feedBack.playQueue; if (!prop || !q || typeof q.start !== 'function' || typeof window.playSong !== 'function') return; + + // Extract the setlist BEFORE the stage is borrowed and the queue starts, + // so a failure here leaves nothing half-applied to unwind. + await prepareGigSongs(prop, btn); + // The poster's Play could have been cancelled while we were unpacking. + if (_ppGigProposal !== prop) return; + // The gig BORROWS the stage: stash whatever venue/viz the user had so // the set ending gives it back (unlike "Play here", which is an // explicit persistent choice on the venue card). @@ -1424,7 +1470,7 @@ } const gigBtn = e.target.closest('[data-pp-gig]'); if (gigBtn) { bookGig(gigBtn.dataset.ppGig); return; } - if (e.target.closest('[data-pp-gig-play]')) { startGig(); return; } + if (e.target.closest('[data-pp-gig-play]')) { startGig(e.target.closest('[data-pp-gig-play]')); return; } if (e.target.closest('[data-pp-gig-reroll]')) { if (_ppGigProposal) bookGig(_ppGigProposal.genre_key); return; diff --git a/tests/js/gig_song_end.test.js b/tests/js/gig_song_end.test.js new file mode 100644 index 0000000..40ab479 --- /dev/null +++ b/tests/js/gig_song_end.test.js @@ -0,0 +1,98 @@ +// A gig is a SET, not a run of unrelated songs. +// +// Reported from a live gig: the player finished the first song and had to sit +// through the per-song results popup before the next one would start, and then +// wait again while that song was extracted from its feedpak zip. +// +// This file covers the CORE half — career pre-extracts the whole setlist before +// the first note. The other half (note_detect must not show its per-song summary +// inside a gig) lives in the note_detect plugin repo, which is not part of this +// checkout: plugins/*/ is gitignored here and note_detect ships from +// feedBack-plugin-notedetect. A test reading it from core would pass on a dev +// box (where the plugin happens to be bundled) and fail in CI, which is worse +// than no test. +// +// The pre-extraction is tested for REAL behaviour — actually unpacking zips — in +// tests/plugins/career/test_routes.py. These are the wiring guards around it. + +'use strict'; + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const ROOT = path.join(__dirname, '..', '..'); +const CAREER = fs.readFileSync(path.join(ROOT, 'plugins', 'career', 'screen.js'), 'utf8'); +const CAREER_ROUTES = fs.readFileSync(path.join(ROOT, 'plugins', 'career', 'routes.py'), 'utf8'); + +function extractBlock(src, signature) { + const start = src.indexOf(signature); + assert.ok(start !== -1, `signature '${signature}' not found`); + const openBrace = src.indexOf('{', start); + let depth = 1; + let i = openBrace + 1; + while (i < src.length && depth > 0) { + const ch = src[i]; + if (ch === '{') depth++; + else if (ch === '}') depth--; + i++; + } + assert.ok(depth === 0, `unbalanced braces after '${signature}'`); + return src.slice(start, i); +} + +test('startGig extracts the whole setlist before starting the queue', () => { + const fn = extractBlock(CAREER, 'async function startGig('); + const prepIdx = fn.search(/await\s+prepareGigSongs\s*\(/); + const startIdx = fn.search(/q\.start\s*\(/); + assert.ok(prepIdx !== -1, 'startGig must pre-extract the set'); + assert.ok(startIdx !== -1, 'q.start not found'); + assert.ok(prepIdx < startIdx, + 'the set must be unpacked BEFORE the queue starts — otherwise the player ' + + 'waits between songs, which is the bug'); +}); + +test('the stage is only borrowed once the set is ready', () => { + const fn = extractBlock(CAREER, 'async function startGig('); + const prepIdx = fn.search(/await\s+prepareGigSongs\s*\(/); + const stageIdx = fn.search(/VENUE_OVERRIDE_KEY/); + assert.ok(prepIdx < stageIdx, + 'a gig cancelled while unpacking must not leave the venue/viz overwritten'); + assert.match(fn, /_ppGigProposal\s*!==\s*prop/, + 'a proposal dismissed while unpacking must not then start a gig'); +}); + +test('pre-extraction never blocks the gig from starting', () => { + const fn = extractBlock(CAREER, 'async function prepareGigSongs('); + assert.match(fn, /catch\s*\(/, + 'a failed prepare must fall through to the old lazy extraction, not abort the gig'); +}); + +test('the prepare route degrades instead of failing', () => { + assert.match(CAREER_ROUTES, /def prepare_gig/, 'prepare route missing'); + assert.match(CAREER_ROUTES, /context\.get\(\s*["']get_dlc_dir["']\s*\)/, + 'a host without the library resolvers must degrade, not 500 — pre-extraction ' + + 'is an optimisation and can never be why a gig will not start'); +}); + +// ── the prepare must never be able to BLOCK the gig (CodeRabbit, #971) ────── +// +// A bare `await fetch(...)` only rejects on a network error. A server that +// accepts the connection and then never answers hangs forever — and the gig +// would never start. That would make this optimisation the exact thing it +// promises never to be: the reason you cannot play. + +test('the prepare fetch is bounded — a hung server cannot block the gig', () => { + const fn = extractBlock(CAREER, 'async function prepareGigSongs('); + assert.match(fn, /AbortController/, 'the request must be abortable'); + assert.match(fn, /setTimeout\([\s\S]{0,40}abort\s*\(\s*\)/, + 'a hung request must be aborted, not awaited forever'); + assert.match(fn, /signal:\s*ctrl\.signal/, 'the signal must actually be passed to fetch'); + assert.match(fn, /clearTimeout/, 'the timer must be cleared on the happy path'); + assert.match(CAREER, /const\s+PREPARE_TIMEOUT_MS\s*=\s*\d+/, 'the ceiling must be named'); + // The button must be restored however we leave — otherwise a timeout strands + // the poster on "Preparing set…" with Play disabled: unplayable. + assert.match(fn, /finally\s*\{[\s\S]{0,220}btn\.disabled\s*=\s*false/, + 'the Play button must be re-enabled on EVERY path, including the abort'); +}); diff --git a/tests/plugins/career/test_routes.py b/tests/plugins/career/test_routes.py index e1ed626..1830f7a 100644 --- a/tests/plugins/career/test_routes.py +++ b/tests/plugins/career/test_routes.py @@ -174,3 +174,176 @@ def test_double_download_409s(client, monkeypatch): career_routes._state["downloads"]["bar"] = {"status": "running"} assert client.post("/api/plugins/career/packs/bar/download").status_code == 409 assert client.delete("/api/plugins/career/packs/bar").status_code == 409 + + +# ── gig pre-extraction (the wait between songs) ───────────────────────────── +# +# A feedpak is a zip: the first play of one pays for its extraction into +# sloppak_cache. Inside a set that cost landed BETWEEN songs — the player +# finished a number and then sat waiting for the next one to unpack, mid-gig. +# The setlist is known up front, so extract it all while the poster is up. + +def _career_client_with_library(tmp_path, meta_db, dlc, cache): + from fastapi import FastAPI + from fastapi.testclient import TestClient + import routes as career_routes + app = FastAPI() + career_routes.setup(app, { + "config_dir": str(tmp_path), + "meta_db": meta_db, + "get_dlc_dir": lambda: dlc, + "get_sloppak_cache_dir": lambda: cache, + }) + return TestClient(app) + + +def _write_feedpak(dlc, name, title="T"): + """A minimal but REAL feedpak zip, so resolve_source_dir genuinely unpacks.""" + import json as _json + import zipfile as _zip + p = dlc / name + with _zip.ZipFile(p, "w") as z: + z.writestr("manifest.json", _json.dumps({"title": title, "artist": "A", "arrangements": []})) + return p + + +def test_gig_prepare_extracts_every_song_up_front(tmp_path, meta_db): + dlc = tmp_path / "dlc" + dlc.mkdir() + cache = tmp_path / "cache" + cache.mkdir() + for n in ("one.feedpak", "two.feedpak", "three.feedpak"): + _write_feedpak(dlc, n) + + client = _career_client_with_library(tmp_path, meta_db, dlc, cache) + before = list(cache.iterdir()) + assert before == [], "nothing unpacked yet" + + res = client.post("/api/plugins/career/gigs/prepare", + json={"songs": ["one.feedpak", "two.feedpak", "three.feedpak"]}) + assert res.status_code == 200 + body = res.json() + assert body["ok"] is True + assert body["prepared"] == 3, body + assert body["failed"] == [] + # The point of the whole exercise: the set is on disk BEFORE the first note. + assert len(list(cache.iterdir())) == 3, "every song of the set must be unpacked" + + +def test_gig_prepare_is_idempotent_on_a_warm_cache(tmp_path, meta_db): + dlc = tmp_path / "dlc" + dlc.mkdir() + cache = tmp_path / "cache" + cache.mkdir() + _write_feedpak(dlc, "one.feedpak") + client = _career_client_with_library(tmp_path, meta_db, dlc, cache) + + first = client.post("/api/plugins/career/gigs/prepare", json={"songs": ["one.feedpak"]}).json() + second = client.post("/api/plugins/career/gigs/prepare", json={"songs": ["one.feedpak"]}).json() + assert first["prepared"] == second["prepared"] == 1 + assert len(list(cache.iterdir())) == 1, "a re-prepare must not duplicate the unpack" + + +def test_one_bad_feedpak_does_not_stop_the_set(tmp_path, meta_db): + # A corrupt pak in the setlist must not block the gig: the play itself will + # surface the error exactly as it does outside a gig. Slow beats blocked. + dlc = tmp_path / "dlc" + dlc.mkdir() + cache = tmp_path / "cache" + cache.mkdir() + _write_feedpak(dlc, "good.feedpak") + (dlc / "bad.feedpak").write_bytes(b"not a zip at all") + + client = _career_client_with_library(tmp_path, meta_db, dlc, cache) + body = client.post("/api/plugins/career/gigs/prepare", + json={"songs": ["good.feedpak", "bad.feedpak"]}).json() + assert body["ok"] is True, "a bad pak must not fail the whole prepare" + assert body["prepared"] == 1 + assert body["failed"] == ["bad.feedpak"] + + +def test_gig_prepare_degrades_without_a_library(tmp_path, meta_db, client): + # The stock fixture's context has no dlc/cache resolvers. That must be a + # graceful no-op, not a 500 — pre-extraction is an optimisation and can + # never be the reason a gig won't start. + res = client.post("/api/plugins/career/gigs/prepare", json={"songs": ["x.feedpak"]}) + assert res.status_code == 200 + assert res.json()["prepared"] == 0 + + +def test_gig_prepare_empty_setlist(tmp_path, meta_db, client): + res = client.post("/api/plugins/career/gigs/prepare", json={"songs": []}) + assert res.status_code == 200 + assert res.json() == {"ok": True, "prepared": 0, "failed": []} + + +def test_prepare_rejects_a_non_list_songs_value(tmp_path, meta_db, client): + # A str is iterable: without the list check, "abc" would prepare three + # one-character "songs". + for bad in ("abc", 42, {"a": 1}, None): + res = client.post("/api/plugins/career/gigs/prepare", json={"songs": bad}) + assert res.status_code == 200 + assert res.json()["prepared"] == 0 + + +def test_prepare_ignores_non_string_and_blank_entries(tmp_path, meta_db): + dlc = tmp_path / "dlc" + dlc.mkdir() + cache = tmp_path / "cache" + cache.mkdir() + _write_feedpak(dlc, "good.feedpak") + client = _career_client_with_library(tmp_path, meta_db, dlc, cache) + body = client.post("/api/plugins/career/gigs/prepare", + json={"songs": ["good.feedpak", "", " ", 7, None, {"x": 1}]}).json() + assert body["prepared"] == 1 + assert body["failed"] == [] + + +def test_prepare_caps_the_setlist(tmp_path, meta_db): + # This endpoint unpacks zips — an arbitrary caller must not be able to ask for + # unbounded work. + # + # The first version of this test asserted `prepared == 0` against a fixture + # with NO library: the endpoint exits before extraction there, so it passed + # whether or not the cap existed. Give it a real library, ask for far more than + # the cap, and assert the endpoint only ever considered MAX_GIG_SONGS of them. + import routes as career_routes + assert career_routes.MAX_GIG_SONGS <= 64 + + dlc = tmp_path / "dlc" + dlc.mkdir() + cache = tmp_path / "cache" + cache.mkdir() + client = _career_client_with_library(tmp_path, meta_db, dlc, cache) + + n = career_routes.MAX_GIG_SONGS + 50 + # None of these exist, so every song the endpoint LOOKS AT lands in `failed`. + # That makes `failed` an exact count of how many it considered. + body = client.post("/api/plugins/career/gigs/prepare", + json={"songs": [f"missing{i}.feedpak" for i in range(n)]}).json() + assert body["prepared"] == 0 + assert len(body["failed"]) == career_routes.MAX_GIG_SONGS, ( + f"the endpoint must consider at most MAX_GIG_SONGS " + f"({career_routes.MAX_GIG_SONGS}), not all {n}" + ) + + +def test_prepare_refuses_to_escape_the_library(tmp_path, meta_db): + # resolve_source_dir() does a bare `dlc_root / filename` with no containment + # guard, so a crafted path would walk straight out of the library. Every + # filename must go through _resolve_dlc_path first. + dlc = tmp_path / "dlc" + dlc.mkdir() + cache = tmp_path / "cache" + cache.mkdir() + (tmp_path / "outside.feedpak").write_bytes(b"secret") + client = _career_client_with_library(tmp_path, meta_db, dlc, cache) + + for evil in ("../outside.feedpak", "..\\outside.feedpak", + "a/../../outside.feedpak", "/etc/passwd", "C:/Windows/x.feedpak"): + body = client.post("/api/plugins/career/gigs/prepare", + json={"songs": [evil]}).json() + assert body["prepared"] == 0, f"{evil!r} must never be prepared" + assert body["failed"] == [evil] + # Nothing outside the library may have been unpacked. + assert list(cache.iterdir()) == []