mirror of
https://github.com/got-feedBack/feedBack.git
synced 2026-07-24 05:41:53 +00:00
Root cause of the flaky pytest segfault (exit 139): the background scan and enrichment daemon threads (_scan_runner/_enrich_runner) use the shared MetadataDB connection, but test fixtures closed that connection in teardown without stopping them. A daemon thread mid-query on a freed SQLite conn is a native use-after-free → SIGSEGV. The app's startup kicks a scan, so almost any app-booting fixture was vulnerable. It only surfaced now because got-feedback/feedBack#728 added a push trigger, so ci/test runs on every push to main. Fix: server.py retains the scan/enrich thread handles and adds _join_background_db_threads(); every test fixture now joins the workers before conn.close(). Verified: the full suite runs to completion (no segfault) where it previously crashed at ~25%. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
262 lines
12 KiB
Python
262 lines
12 KiB
Python
"""Tests for the R4a gap-fill write-back — the §7 contract made executable:
|
|
user-initiated, adds ABSENT keys only (author bytes preserved verbatim),
|
|
spec'd-keys allowlist, values only from a CONFIRMED match, atomic + .bak.
|
|
|
|
No network anywhere: matches are seeded straight into the enrichment cache
|
|
(as the P8 matcher would have written them).
|
|
"""
|
|
|
|
import importlib
|
|
import sys
|
|
import zipfile
|
|
|
|
import pytest
|
|
import yaml
|
|
from fastapi.testclient import TestClient
|
|
|
|
|
|
@pytest.fixture()
|
|
def server(tmp_path, monkeypatch, isolate_logging):
|
|
monkeypatch.setenv("CONFIG_DIR", str(tmp_path / "config"))
|
|
dlc = tmp_path / "dlc"
|
|
dlc.mkdir()
|
|
monkeypatch.setenv("DLC_DIR", str(dlc))
|
|
monkeypatch.setenv("FEEDBACK_SKIP_STARTUP_TASKS", "1")
|
|
sys.modules.pop("server", None)
|
|
srv = importlib.import_module("server")
|
|
try:
|
|
yield srv
|
|
finally:
|
|
conn = getattr(getattr(srv, "meta_db", None), "conn", None)
|
|
if conn is not None:
|
|
getattr(__import__("sys").modules.get("server"), "_join_background_db_threads", lambda: None)()
|
|
conn.close()
|
|
sys.modules.pop("server", None)
|
|
|
|
|
|
@pytest.fixture()
|
|
def client(server):
|
|
return TestClient(server.app)
|
|
|
|
|
|
BASE_MANIFEST = ("# my hand-made pack\n"
|
|
"title: Thunderstruck\n"
|
|
"artist: AC/DC # the real one\n"
|
|
"duration: 292\n"
|
|
"arrangements: []\n"
|
|
"stems: []\n")
|
|
|
|
|
|
def make_dir_sloppak(server, name, manifest=BASE_MANIFEST):
|
|
d = server.DLC_DIR / name
|
|
d.mkdir(parents=True)
|
|
(d / "manifest.yaml").write_text(manifest, encoding="utf-8")
|
|
_put_db(server, name)
|
|
return d
|
|
|
|
|
|
def make_zip_sloppak(server, name, manifest=BASE_MANIFEST):
|
|
p = server.DLC_DIR / name
|
|
with zipfile.ZipFile(p, "w", zipfile.ZIP_DEFLATED) as z:
|
|
z.writestr("manifest.yaml", manifest)
|
|
z.writestr("stems/full.ogg", b"OggS-fake")
|
|
_put_db(server, name)
|
|
return p
|
|
|
|
|
|
def _put_db(server, name):
|
|
server.meta_db.put(name, 0, 0, {
|
|
"title": "Thunderstruck", "artist": "AC/DC", "album": "", "year": "",
|
|
"duration": 292, "arrangements": [{"name": "Lead", "index": 0}],
|
|
})
|
|
|
|
|
|
def seed_match(server, fn, state="matched", **overrides):
|
|
"""Seed a confirmed enrichment row as the P8 matcher would have."""
|
|
cand = {"recording_id": "12345678-abcd-4ef0-9876-0123456789ab",
|
|
"release_id": "rel-1", "artist_id": "art-1",
|
|
"artist": "AC/DC", "title": "Thunderstruck",
|
|
"album": "The Razors Edge", "year": "1990",
|
|
"genres": ["hard rock", "rock"], "isrc": "AUAP09000045"}
|
|
cand.update(overrides)
|
|
song = server.meta_db.enrichment_song_row(fn)
|
|
h = server.meta_db.enrichment_content_hash(
|
|
song["artist"], song["title"], song["album"], song["duration"])
|
|
server.meta_db.apply_enrichment_match(
|
|
fn, h, state, source="text", score=1.0, cand=cand,
|
|
candidates=([cand] if state == "review" else None))
|
|
|
|
|
|
# ── preview ───────────────────────────────────────────────────────────────────
|
|
|
|
def test_preview_offers_only_absent_confirmed_keys(server, client):
|
|
make_dir_sloppak(server, "a.sloppak")
|
|
seed_match(server, "a.sloppak")
|
|
d = client.get("/api/song/a.sloppak/gap-fill").json()
|
|
assert d["eligible"] is True
|
|
got = {m["key"]: m["value"] for m in d["missing"]}
|
|
assert got == {"album": "The Razors Edge", "year": 1990,
|
|
"genres": ["hard rock", "rock"],
|
|
"mbid": "12345678-abcd-4ef0-9876-0123456789ab",
|
|
"isrc": "AUAP09000045"}
|
|
|
|
|
|
def test_preview_excludes_author_set_keys(server, client):
|
|
make_dir_sloppak(server, "a.sloppak",
|
|
BASE_MANIFEST + "album: Live Bootleg\nyear: 1991\n")
|
|
seed_match(server, "a.sloppak")
|
|
got = {m["key"] for m in client.get("/api/song/a.sloppak/gap-fill").json()["missing"]}
|
|
assert "album" not in got and "year" not in got
|
|
assert {"genres", "mbid", "isrc"} <= got
|
|
|
|
|
|
def test_preview_excludes_present_but_empty_keys(server, client):
|
|
"""Gap-fill is append-only, so a present-but-empty value (album: '',
|
|
year: 0) is NOT a gap the writer can fill — appending would duplicate the
|
|
key, and the never-clobber guard refuses any present key. The preview must
|
|
therefore not offer it (those are the metadata editor's job to re-serialize),
|
|
while genuinely-absent keys are still offered."""
|
|
make_dir_sloppak(server, "a.sloppak", BASE_MANIFEST + "album: ''\nyear: 0\n")
|
|
seed_match(server, "a.sloppak")
|
|
got = {m["key"] for m in client.get("/api/song/a.sloppak/gap-fill").json()["missing"]}
|
|
assert "album" not in got and "year" not in got
|
|
assert {"genres", "mbid", "isrc"} <= got
|
|
|
|
|
|
def test_write_present_but_empty_key_is_refused_not_500(server, client):
|
|
"""The preview↔writer contract must agree: a POST for a present-but-empty
|
|
key is turned away with a clean 409 (never offered → skipped), never a 500
|
|
from the writer's never-clobber guard, and the file is left untouched."""
|
|
d = make_dir_sloppak(server, "a.sloppak", BASE_MANIFEST + "album: ''\nyear: 0\n")
|
|
seed_match(server, "a.sloppak")
|
|
before = (d / "manifest.yaml").read_text(encoding="utf-8")
|
|
r = client.post("/api/song/a.sloppak/gap-fill", json={"keys": ["album", "year"]})
|
|
assert r.status_code == 409
|
|
assert sorted(r.json()["skipped"]) == ["album", "year"]
|
|
assert (d / "manifest.yaml").read_text(encoding="utf-8") == before
|
|
assert not (d / "manifest.yaml.bak").exists() # nothing written → no backup
|
|
# A genuinely-absent key alongside the empty ones still writes cleanly.
|
|
r = client.post("/api/song/a.sloppak/gap-fill", json={"keys": ["album", "genres"]})
|
|
assert r.status_code == 200
|
|
assert r.json() == {"ok": True, "written": {"genres": ["hard rock", "rock"]},
|
|
"skipped": ["album"]}
|
|
|
|
|
|
def test_preview_requires_confirmed_match(server, client):
|
|
make_dir_sloppak(server, "a.sloppak")
|
|
d = client.get("/api/song/a.sloppak/gap-fill").json()
|
|
assert d["eligible"] is False and d["reason"] == "no-match"
|
|
seed_match(server, "a.sloppak", state="review")
|
|
d = client.get("/api/song/a.sloppak/gap-fill").json()
|
|
assert d["eligible"] is False and d["reason"] == "review"
|
|
# A user-pinned match is confirmed.
|
|
seed_match(server, "a.sloppak", state="manual")
|
|
assert client.get("/api/song/a.sloppak/gap-fill").json()["eligible"] is True
|
|
|
|
|
|
# ── writing ───────────────────────────────────────────────────────────────────
|
|
|
|
def test_write_dir_form_appends_and_preserves_author_bytes(server, client):
|
|
d = make_dir_sloppak(server, "a.sloppak")
|
|
seed_match(server, "a.sloppak")
|
|
r = client.post("/api/song/a.sloppak/gap-fill",
|
|
json={"keys": ["album", "year", "genres", "mbid", "isrc"]})
|
|
assert r.status_code == 200
|
|
body = r.json()
|
|
assert set(body["written"]) == {"album", "year", "genres", "mbid", "isrc"}
|
|
text = (d / "manifest.yaml").read_text(encoding="utf-8")
|
|
# The author's original bytes — comments included — survive verbatim as a
|
|
# prefix; the additions are appended after them.
|
|
assert text.startswith(BASE_MANIFEST)
|
|
manifest = yaml.safe_load(text)
|
|
assert manifest["album"] == "The Razors Edge"
|
|
assert manifest["year"] == 1990
|
|
assert manifest["genres"] == ["hard rock", "rock"]
|
|
assert manifest["mbid"] == "12345678-abcd-4ef0-9876-0123456789ab"
|
|
assert manifest["isrc"] == "AUAP09000045"
|
|
# Backup + DB sync (the row must match what the scanner would derive).
|
|
assert (d / "manifest.yaml.bak").read_text(encoding="utf-8") == BASE_MANIFEST
|
|
row = client.get("/api/song/a.sloppak").json()
|
|
assert row["album"] == "The Razors Edge"
|
|
assert str(row["year"]) == "1990"
|
|
|
|
|
|
def test_write_zip_form_appends_with_backup(server, client):
|
|
p = make_zip_sloppak(server, "a.sloppak")
|
|
seed_match(server, "a.sloppak")
|
|
r = client.post("/api/song/a.sloppak/gap-fill", json={"keys": ["album", "mbid"]})
|
|
assert r.status_code == 200
|
|
with zipfile.ZipFile(p) as z:
|
|
text = z.read("manifest.yaml").decode("utf-8")
|
|
assert text.startswith(BASE_MANIFEST)
|
|
manifest = yaml.safe_load(text)
|
|
assert manifest["album"] == "The Razors Edge"
|
|
assert manifest["mbid"] == "12345678-abcd-4ef0-9876-0123456789ab"
|
|
assert "year" not in manifest # unrequested keys untouched
|
|
assert z.read("stems/full.ogg") == b"OggS-fake"
|
|
bak = p.with_name(p.name + ".bak")
|
|
assert bak.exists()
|
|
with zipfile.ZipFile(bak) as z:
|
|
assert z.read("manifest.yaml").decode("utf-8") == BASE_MANIFEST
|
|
|
|
|
|
def test_write_never_replaces_author_values(server, client):
|
|
d = make_dir_sloppak(server, "a.sloppak", BASE_MANIFEST + "album: Live Bootleg\n")
|
|
seed_match(server, "a.sloppak")
|
|
# Requesting a present key: skipped, not replaced; nothing else requested
|
|
# → 409 and the file is untouched.
|
|
r = client.post("/api/song/a.sloppak/gap-fill", json={"keys": ["album"]})
|
|
assert r.status_code == 409
|
|
assert r.json()["skipped"] == ["album"]
|
|
assert (d / "manifest.yaml").read_text(encoding="utf-8").endswith("album: Live Bootleg\n")
|
|
# Mixed request: the gap is written, the author value survives.
|
|
r = client.post("/api/song/a.sloppak/gap-fill", json={"keys": ["album", "year"]})
|
|
assert r.status_code == 200
|
|
assert r.json() == {"ok": True, "written": {"year": 1990}, "skipped": ["album"]}
|
|
manifest = yaml.safe_load((d / "manifest.yaml").read_text(encoding="utf-8"))
|
|
assert manifest["album"] == "Live Bootleg"
|
|
assert manifest["year"] == 1990
|
|
|
|
|
|
def test_writer_last_line_guard(server):
|
|
"""The lib-level never-clobber guard holds even if a caller skips the
|
|
proposal check."""
|
|
import songmeta
|
|
d = make_dir_sloppak(server, "a.sloppak", BASE_MANIFEST + "album: Kept\n")
|
|
with pytest.raises(ValueError):
|
|
songmeta.gap_fill_sloppak(d, {"album": "Clobber"})
|
|
assert "Kept" in (d / "manifest.yaml").read_text(encoding="utf-8")
|
|
|
|
|
|
def test_write_validates_keys(server, client):
|
|
make_dir_sloppak(server, "a.sloppak")
|
|
seed_match(server, "a.sloppak")
|
|
assert client.post("/api/song/a.sloppak/gap-fill",
|
|
json={"keys": ["title"]}).status_code == 400
|
|
assert client.post("/api/song/a.sloppak/gap-fill",
|
|
json={"keys": []}).status_code == 400
|
|
assert client.post("/api/song/a.sloppak/gap-fill", json={}).status_code == 400
|
|
|
|
|
|
def test_demo_mode_blocks_write(tmp_path, monkeypatch, isolate_logging):
|
|
"""The middleware turns the write route away before any handler runs —
|
|
demo visitors can never rewrite pack files."""
|
|
monkeypatch.setenv("CONFIG_DIR", str(tmp_path / "config"))
|
|
dlc = tmp_path / "dlc"
|
|
dlc.mkdir()
|
|
monkeypatch.setenv("DLC_DIR", str(dlc))
|
|
monkeypatch.setenv("FEEDBACK_SKIP_STARTUP_TASKS", "1")
|
|
monkeypatch.setenv("FEEDBACK_DEMO_MODE", "1")
|
|
sys.modules.pop("server", None)
|
|
srv = importlib.import_module("server")
|
|
try:
|
|
r = TestClient(srv.app).post("/api/song/a.sloppak/gap-fill",
|
|
json={"keys": ["album"]})
|
|
assert r.status_code == 403
|
|
finally:
|
|
conn = getattr(getattr(srv, "meta_db", None), "conn", None)
|
|
if conn is not None:
|
|
getattr(__import__("sys").modules.get("server"), "_join_background_db_threads", lambda: None)()
|
|
conn.close()
|
|
sys.modules.pop("server", None)
|