mirror of
https://github.com/got-feedBack/feedBack.git
synced 2026-08-15 13:17:25 +00:00
refactor(server): move the metadata-enrichment subsystem into lib/enrichment.py (R3) (#861)
* refactor(server): move the metadata-enrichment subsystem into lib/enrichment.py (R3)
MusicBrainz / Cover-Art-Archive / AcoustID transport, the match-scorer glue, and
the background enrichment worker (~930 lines, 61 defs) leave server.py as one
cohesive unit. Bodies are verbatim; the only changes are seam reads:
meta_db / config_dir / sloppak_cache_dir / art_cache_dir -> appstate.<slot>
_song_pack_art_exists / _art_override_paths (stay in server.py for the art +
delete routes) -> appstate.<callable> (new seam slots, injected by reference)
_env_flag -> env_compat.env_flag_compat (the existing identical helper)
_artist_title_from_filename -> imported from metadata_db (its home)
the User-Agent VERSION lookup: Path(__file__).parent ->
Path(__file__).resolve().parents[1] (lib/enrichment.py -> app root)
server.py drives the worker through the module (import enrichment; the routes +
scan lifecycle call enrichment.X). Tests that faked the network on `server`
(_mb_http_get, _enrich_network_enabled, _caa_http_get, ...) now patch the same
names on `enrichment` — the module attribute is resolved at call time, so one
setattr reaches both the routes and the worker's internal callers. Acyclic:
enrichment imports appstate/appconfig/dlc_paths/metadata_db/mb_match/
acoustid_match/sloppak/loosefolder, never server.
server.py: 6,917 -> 5,988 (-929).
Verified: pyflakes clean (bar the pre-existing File/safe_join/tuning_name/ET);
route table IDENTICAL (143); full pytest 2400 passed (140 enrichment/art cases
incl the offline-safety + transport-error-pauses-pass contracts that fake the
network); test_packaging 44 passed (enrichment.py resolves under lib/); eslint 0.
Boot smoke: /api/enrichment/status + POST /kick serve from the new module.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test: reset enrichment worker state between tests (CodeRabbit)
lib/enrichment.py now owns the worker, and it stays imported for the whole
session while the `server` fixtures pop-and-reimport `server` — so the cancel
Event / status dict / caches would leak across tests, and a stale `_enrich_cancel`
could short-circuit a later direct `_background_enrich()`. An autouse conftest
fixture clears that process-global state before each test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test: tighten the enrichment-reset fixture (CodeRabbit)
Narrow the import guard to ImportError (not blind Exception, BLE001), and stop
clearing _caa_index_locks — it's guarded by _caa_index_locks_guard, so an
unlocked clear() would race a still-alive worker, and its per-release mutexes
carry no test state.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Byron Gamatos
co-authored by
Claude Opus 4.8
Claude Opus 4.8
parent
508829c012
commit
165475d115
@@ -5,6 +5,7 @@ contracts it will inherit: rename-survivable idempotent hashing, manual rows
|
||||
never auto-reset, never purged on rescan, purged on explicit delete."""
|
||||
|
||||
import importlib
|
||||
import enrichment
|
||||
import sys
|
||||
|
||||
import pytest
|
||||
@@ -58,7 +59,7 @@ def test_pending_covers_new_unscanned_and_changed(server):
|
||||
_put(server, "a.archive")
|
||||
assert [r["filename"] for r in server.meta_db.enrichment_pending()] == ["a.archive"]
|
||||
# stubbed → still unscanned → still pending (the matcher hasn't run)
|
||||
server._background_enrich()
|
||||
enrichment._background_enrich()
|
||||
assert [r["filename"] for r in server.meta_db.enrichment_pending()] == ["a.archive"]
|
||||
# a matched row with the CURRENT hash is settled…
|
||||
h = server.meta_db.enrichment_content_hash("Artist", "Song", "", 100)
|
||||
@@ -76,7 +77,7 @@ def test_pending_covers_new_unscanned_and_changed(server):
|
||||
def test_hash_change_resets_matched_but_never_manual(server):
|
||||
_put(server, "a.archive")
|
||||
_put(server, "b.archive", title="Other")
|
||||
server._background_enrich()
|
||||
enrichment._background_enrich()
|
||||
with server.meta_db._lock:
|
||||
server.meta_db.conn.execute(
|
||||
"UPDATE song_enrichment SET match_state = 'matched' WHERE filename = 'a.archive'")
|
||||
@@ -86,7 +87,7 @@ def test_hash_change_resets_matched_but_never_manual(server):
|
||||
# identity edits…
|
||||
_put(server, "a.archive", title="Song v2")
|
||||
_put(server, "b.archive", title="Other v2")
|
||||
server._background_enrich()
|
||||
enrichment._background_enrich()
|
||||
a = server.meta_db.get_enrichment("a.archive")
|
||||
b = server.meta_db.get_enrichment("b.archive")
|
||||
# …drop a stale MATCH back to unscanned with the fresh hash
|
||||
@@ -99,7 +100,7 @@ def test_hash_change_resets_matched_but_never_manual(server):
|
||||
|
||||
def test_failed_rows_not_requeued_by_pending(server):
|
||||
_put(server, "a.archive")
|
||||
server._background_enrich()
|
||||
enrichment._background_enrich()
|
||||
with server.meta_db._lock:
|
||||
server.meta_db.conn.execute(
|
||||
"UPDATE song_enrichment SET match_state = 'failed' WHERE filename = 'a.archive'")
|
||||
@@ -113,7 +114,7 @@ def test_failed_rows_not_requeued_by_pending(server):
|
||||
def test_enrich_pass_stamps_every_song(server):
|
||||
for i in range(5):
|
||||
_put(server, f"s{i}.archive", title=f"Song {i}")
|
||||
server._background_enrich()
|
||||
enrichment._background_enrich()
|
||||
for i in range(5):
|
||||
row = server.meta_db.get_enrichment(f"s{i}.archive")
|
||||
assert row is not None
|
||||
@@ -126,7 +127,7 @@ def test_enrich_pass_stamps_every_song(server):
|
||||
|
||||
def test_rescan_never_purges_enrichment(server):
|
||||
_put(server, "a.archive")
|
||||
server._background_enrich()
|
||||
enrichment._background_enrich()
|
||||
server.meta_db.delete_missing(set()) # file vanished from a scan snapshot
|
||||
assert server.meta_db.get_enrichment("a.archive") is not None # row survives
|
||||
# …and is invisible in the read-time-filtered counts
|
||||
@@ -138,7 +139,7 @@ def test_rescan_never_purges_enrichment(server):
|
||||
def test_status_endpoint_counts(client, server):
|
||||
_put(server, "a.archive")
|
||||
_put(server, "b.archive", title="Other")
|
||||
server._background_enrich()
|
||||
enrichment._background_enrich()
|
||||
body = client.get("/api/enrichment/status").json()
|
||||
assert body["states"] == {"unscanned": 2}
|
||||
assert body["total_songs"] == 2
|
||||
@@ -147,7 +148,7 @@ def test_status_endpoint_counts(client, server):
|
||||
|
||||
|
||||
def test_art_cache_dir_created(server):
|
||||
d = server._enrichment_art_dir()
|
||||
d = enrichment._enrichment_art_dir()
|
||||
assert d.is_dir()
|
||||
assert d.name == "art_cache"
|
||||
|
||||
@@ -156,7 +157,7 @@ def test_art_cache_dir_created(server):
|
||||
|
||||
def test_states_for_returns_only_known_filenames(server):
|
||||
_put(server, "a.archive")
|
||||
server._background_enrich()
|
||||
enrichment._background_enrich()
|
||||
got = server.meta_db.enrichment_states_for(["a.archive", "nope.archive"])
|
||||
assert got == {"a.archive": "unscanned"} # unknown filename absent
|
||||
assert server.meta_db.enrichment_states_for([]) == {}
|
||||
@@ -165,7 +166,7 @@ def test_states_for_returns_only_known_filenames(server):
|
||||
def test_states_endpoint(client, server):
|
||||
_put(server, "a.archive")
|
||||
_put(server, "b.archive", title="Other")
|
||||
server._background_enrich()
|
||||
enrichment._background_enrich()
|
||||
body = client.post("/api/enrichment/states",
|
||||
json={"filenames": ["a.archive", "zzz.missing"]}).json()
|
||||
assert body["states"] == {"a.archive": "unscanned"}
|
||||
@@ -175,7 +176,7 @@ def test_states_endpoint(client, server):
|
||||
|
||||
def test_status_exposes_progress_fields(client, server):
|
||||
_put(server, "a.archive")
|
||||
server._background_enrich()
|
||||
enrichment._background_enrich()
|
||||
body = client.get("/api/enrichment/status").json()
|
||||
for k in ("total", "matched", "current", "cancelling"):
|
||||
assert k in body
|
||||
@@ -186,7 +187,7 @@ def test_cancel_is_noop_when_idle(client, server):
|
||||
body = client.post("/api/enrichment/cancel").json()
|
||||
assert body == {"ok": True, "was_running": False}
|
||||
# A no-op must not arm the flag (which would then poison the next pass).
|
||||
assert server._enrich_cancel.is_set() is False
|
||||
assert enrichment._enrich_cancel.is_set() is False
|
||||
|
||||
|
||||
def test_cancel_flag_halts_matching_loop_between_songs(server, monkeypatch):
|
||||
@@ -195,28 +196,28 @@ def test_cancel_flag_halts_matching_loop_between_songs(server, monkeypatch):
|
||||
# Force the matcher path on (the test env is offline by default) and stub the
|
||||
# per-song matcher so nothing touches the network — it just trips Stop after
|
||||
# the first song, exactly as the /cancel route would mid-pass.
|
||||
monkeypatch.setattr(server, "_enrich_network_enabled", lambda: True)
|
||||
monkeypatch.setattr(enrichment, "_enrich_network_enabled", lambda: True)
|
||||
calls = []
|
||||
|
||||
def fake_enrich_one(row, **_kw):
|
||||
calls.append(row["filename"])
|
||||
server._enrich_cancel.set()
|
||||
enrichment._enrich_cancel.set()
|
||||
|
||||
monkeypatch.setattr(server, "_enrich_one", fake_enrich_one)
|
||||
server._enrich_cancel.clear()
|
||||
server._background_enrich()
|
||||
monkeypatch.setattr(enrichment, "_enrich_one", fake_enrich_one)
|
||||
enrichment._enrich_cancel.clear()
|
||||
enrichment._background_enrich()
|
||||
# The loop checks cancel BEFORE each song, so exactly one is processed before
|
||||
# it breaks — not the whole 4-row queue.
|
||||
assert calls == ["s0.archive"]
|
||||
assert server._enrich_status["total"] == 4
|
||||
assert server._enrich_status["matched"] == 1
|
||||
assert enrichment._enrich_status["total"] == 4
|
||||
assert enrichment._enrich_status["matched"] == 1
|
||||
|
||||
|
||||
def test_rematch_requeues_visible_but_skips_manual(server, client):
|
||||
_put(server, "a.archive") # will be 'matched'
|
||||
_put(server, "b.archive", title="Other") # will be 'failed'
|
||||
_put(server, "c.archive", title="Pinned") # will be 'manual' — untouchable
|
||||
server._background_enrich()
|
||||
enrichment._background_enrich()
|
||||
with server.meta_db._lock:
|
||||
server.meta_db.conn.execute(
|
||||
"UPDATE song_enrichment SET match_state='matched' WHERE filename='a.archive'")
|
||||
@@ -240,7 +241,7 @@ def test_rematch_requeues_visible_but_skips_manual(server, client):
|
||||
# ── filename-derived artist/title fallback (blank-artist packs) ───────────────
|
||||
|
||||
def test_filename_artist_title_parse(server):
|
||||
f = server._artist_title_from_filename
|
||||
f = enrichment._artist_title_from_filename
|
||||
assert f("CDLC/0 - City Pop/Tatsuro-Yamashita_Ride-On-Time_v1_p.feedpak") == \
|
||||
{"artist": "Tatsuro Yamashita", "title": "Ride On Time"}
|
||||
assert f("Anri_Windy-Summer_v1_p.feedpak") == {"artist": "Anri", "title": "Windy Summer"}
|
||||
@@ -255,18 +256,18 @@ def test_blank_artist_seeds_match_from_filename(server, monkeypatch):
|
||||
server.meta_db.put("Tatsuro-Yamashita_Ride-On-Time_v1_p.feedpak", 0, 0, {
|
||||
"title": "Tatsuro-Yamashita_Ride-On-Time_v1_p", "artist": "", "album": "",
|
||||
"duration": 240, "arrangements": [{"name": "Bass", "index": 0}]})
|
||||
monkeypatch.setattr(server, "_enrich_network_enabled", lambda: True)
|
||||
monkeypatch.setattr(server, "_manifest_exact_ids", lambda fn: {})
|
||||
monkeypatch.setattr(enrichment, "_enrich_network_enabled", lambda: True)
|
||||
monkeypatch.setattr(enrichment, "_manifest_exact_ids", lambda fn: {})
|
||||
seen = {}
|
||||
|
||||
def fake_search(artist, title, limit=8):
|
||||
seen["artist"], seen["title"] = artist, title
|
||||
return []
|
||||
|
||||
monkeypatch.setattr(server, "_mb_search_recordings", fake_search)
|
||||
monkeypatch.setattr(enrichment, "_mb_search_recordings", fake_search)
|
||||
row = next(r for r in server.meta_db.enrichment_pending()
|
||||
if r["filename"].startswith("Tatsuro"))
|
||||
server._enrich_one(row)
|
||||
enrichment._enrich_one(row)
|
||||
# the blank pack artist was replaced by the filename-derived identity for
|
||||
# the search (this is exactly what rescues the 'failed' pile)
|
||||
assert seen == {"artist": "Tatsuro Yamashita", "title": "Ride On Time"}
|
||||
@@ -276,18 +277,18 @@ def test_present_artist_is_not_overridden_by_filename(server, monkeypatch):
|
||||
server.meta_db.put("Weird-Filename_x_y.feedpak", 0, 0, {
|
||||
"title": "Real Title", "artist": "Real Artist", "album": "", "duration": 100,
|
||||
"arrangements": [{"name": "Lead", "index": 0}]})
|
||||
monkeypatch.setattr(server, "_enrich_network_enabled", lambda: True)
|
||||
monkeypatch.setattr(server, "_manifest_exact_ids", lambda fn: {})
|
||||
monkeypatch.setattr(enrichment, "_enrich_network_enabled", lambda: True)
|
||||
monkeypatch.setattr(enrichment, "_manifest_exact_ids", lambda fn: {})
|
||||
seen = {}
|
||||
|
||||
def fake_search(artist, title, limit=8):
|
||||
seen["artist"], seen["title"] = artist, title
|
||||
return []
|
||||
|
||||
monkeypatch.setattr(server, "_mb_search_recordings", fake_search)
|
||||
monkeypatch.setattr(enrichment, "_mb_search_recordings", fake_search)
|
||||
row = next(r for r in server.meta_db.enrichment_pending()
|
||||
if r["filename"].startswith("Weird"))
|
||||
server._enrich_one(row)
|
||||
enrichment._enrich_one(row)
|
||||
# a pack that DOES carry an artist keeps it — the filename is never consulted
|
||||
assert seen == {"artist": "Real Artist", "title": "Real Title"}
|
||||
|
||||
@@ -295,7 +296,7 @@ def test_present_artist_is_not_overridden_by_filename(server, monkeypatch):
|
||||
def test_kick_clears_a_stale_cancel(server):
|
||||
# A cancelled-then-rekicked pass must start clean: _kick_enrich clears the
|
||||
# flag so the fresh pass isn't aborted the instant it checks.
|
||||
server._enrich_cancel.set()
|
||||
server._kick_enrich()
|
||||
enrichment._enrich_cancel.set()
|
||||
enrichment._kick_enrich()
|
||||
server._join_background_db_threads()
|
||||
assert server._enrich_cancel.is_set() is False
|
||||
assert enrichment._enrich_cancel.is_set() is False
|
||||
|
||||
Reference in New Issue
Block a user