From e8483ee22b6ddd1b26d602fae46822a62ff8fe1e Mon Sep 17 00:00:00 2001 From: Jonathan Date: Mon, 20 Jul 2026 17:04:31 +0200 Subject: [PATCH] fix(worker): match library ownership by rgMbid + retire stale failed attempts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two related monitor bugs surfaced by owned albums that never leave the press: 1. Ownership was matched on (artist, album) strings, but MusicBrainz canonicalizes the artist on import ("Kanye West"->"Ye", "Ne-Yo"->"Ne‐Yo", "The Goo Goo Dolls" ->"Goo Goo Dolls"). The strings no longer matched, so reconcile/enqueue/backfill never recognized the album as owned — it stayed "wanted" and re-pressed every retry interval. Match on the release-group MBID when the library row has one (falling back to artist+album for scanned rows without an MBID) across _in_library_at_cutoff, fulfill_owned_releases, and reconcile. 2. Each failed attempt left a needs_attention Request behind, so a hard-to-match album piled up phantom "needs attention" cards on the press even after a later attempt imported it (e.g. Maroon 5 "V": 3 stale cards + 1 success). Retire prior failed attempts when a release is fulfilled or re-attempted (_retire_failed_attempts, monitor-owned requests only — manual requests have no monitoredReleaseId). Co-Authored-By: Claude Opus 4.8 (1M context) --- worker/lyra_worker/monitor.py | 76 ++++++++++++++++++++------ worker/tests/test_monitor_enqueue.py | 46 ++++++++++++++++ worker/tests/test_monitor_reconcile.py | 54 ++++++++++++++++-- 3 files changed, 155 insertions(+), 21 deletions(-) diff --git a/worker/lyra_worker/monitor.py b/worker/lyra_worker/monitor.py index c65c0d5..50886c3 100644 --- a/worker/lyra_worker/monitor.py +++ b/worker/lyra_worker/monitor.py @@ -86,16 +86,40 @@ def _set_state(conn: psycopg.Connection, rel_id: str, state: str) -> None: cur.execute('UPDATE "MonitoredRelease" SET state = %s WHERE id = %s', (state, rel_id)) -def _in_library_at_cutoff(conn: psycopg.Connection, artist: str, album: str, cutoff: int) -> bool: +def _in_library_at_cutoff( + conn: psycopg.Connection, artist: str, album: str, rg_mbid: str, cutoff: int +) -> bool: + """True if the album is owned at/above `cutoff`. Matches on the release-group MBID when the + library row has one — robust against MusicBrainz artist-name canonicalization (e.g. the + followed 'Kanye West' imports as 'Ye') — and falls back to exact artist+album for scanned + rows that predate MBID capture.""" with conn.cursor() as cur: cur.execute( - 'SELECT 1 FROM "LibraryItem" WHERE artist = %s AND album = %s ' - 'AND "qualityClass" >= %s LIMIT 1', - (artist, album, cutoff), + 'SELECT 1 FROM "LibraryItem" WHERE "qualityClass" >= %s AND (' + ' ("rgMbid" IS NOT NULL AND "rgMbid" = %s) ' + ' OR ("rgMbid" IS NULL AND artist = %s AND album = %s)) LIMIT 1', + (cutoff, rg_mbid, artist, album), ) return cur.fetchone() is not None +def _retire_failed_attempts(conn: psycopg.Connection, rel_id: str) -> None: + """Delete prior failed (needs_attention) Request rows for this release (Job/Candidate cascade). + + A hard-to-match release is re-pressed every retry interval; each failed attempt used to leave + a needs_attention Request behind, piling up phantom "needs attention" cards on the press even + after a later attempt imported the album. Retiring them when the release is fulfilled or + re-attempted keeps the press showing one live attempt at a time. Manual (non-monitor) requests + have no monitoredReleaseId and are never touched.""" + with conn.cursor() as cur: + cur.execute( + 'DELETE FROM "Request" r USING "Job" j ' + 'WHERE j."requestId" = r.id AND r."monitoredReleaseId" = %s ' + "AND j.state = 'needs_attention'", + (rel_id,), + ) + + def _enqueue_one(conn: psycopg.Connection, rel_id: str, artist: str, album: str) -> None: with conn.cursor() as cur: cur.execute( @@ -118,7 +142,8 @@ def enqueue_due(conn: psycopg.Connection, cfg: MonitorConfig) -> int: cur.execute( 'SELECT mr.id, mr."artistName", mr.album, mr.state, mr."currentQualityClass", ' ' (mr."firstGrabbedAt" IS NOT NULL ' - ' AND mr."firstGrabbedAt" < now() - make_interval(days => %s)) AS window_expired ' + ' AND mr."firstGrabbedAt" < now() - make_interval(days => %s)) AS window_expired, ' + ' mr."rgMbid" ' 'FROM "MonitoredRelease" mr ' 'WHERE mr.monitored = true ' ' AND NOT mr.ignored ' # user-ignored releases are never enqueued @@ -134,8 +159,11 @@ def enqueue_due(conn: psycopg.Connection, cfg: MonitorConfig) -> int: rows = cur.fetchall() enqueued = 0 - for rel_id, artist, album, state, quality, window_expired in rows: - if _in_library_at_cutoff(conn, artist, album, cfg.quality_cutoff): + for rel_id, artist, album, state, quality, window_expired, rg_mbid in rows: + # A due release is about to be fulfilled or re-attempted; either way any earlier failed + # attempt is superseded, so retire it before deciding. + _retire_failed_attempts(conn, rel_id) + if _in_library_at_cutoff(conn, artist, album, rg_mbid, cfg.quality_cutoff): _set_state(conn, rel_id, "fulfilled") continue if state == "grabbed" and (window_expired or (quality is not None and quality >= cfg.quality_cutoff)): @@ -154,12 +182,19 @@ def fulfill_owned_releases(conn: psycopg.Connection, cfg: MonitorConfig) -> int: with conn.cursor() as cur: cur.execute( 'UPDATE "MonitoredRelease" mr SET state = \'fulfilled\', ' - ' "currentQualityClass" = li.q, ' + ' "currentQualityClass" = owned.q, ' ' "firstGrabbedAt" = COALESCE(mr."firstGrabbedAt", now()) ' - 'FROM (SELECT artist, album, max("qualityClass") AS q FROM "LibraryItem" ' - ' GROUP BY artist, album) li ' - 'WHERE li.artist = mr."artistName" AND li.album = mr.album ' - " AND mr.monitored = true AND mr.state <> 'fulfilled' AND li.q >= %s", + 'FROM (' + ' SELECT mr2.id AS mid, max(li."qualityClass") AS q ' + ' FROM "MonitoredRelease" mr2 JOIN "LibraryItem" li ON (' + # match on rgMbid where the library row has one (survives artist-name canonicalization), + # else fall back to exact artist+album for scanned rows lacking an MBID + ' (li."rgMbid" IS NOT NULL AND li."rgMbid" = mr2."rgMbid") ' + ' OR (li."rgMbid" IS NULL AND li.artist = mr2."artistName" AND li.album = mr2.album)) ' + " WHERE mr2.monitored = true AND mr2.state <> 'fulfilled' " + ' GROUP BY mr2.id HAVING max(li."qualityClass") >= %s' + ') owned ' + 'WHERE mr.id = owned.mid', (cfg.quality_cutoff,), ) fixed = cur.rowcount @@ -171,21 +206,28 @@ def reconcile(conn: psycopg.Connection, job_id: str, cfg: MonitorConfig) -> None """Feed a finished job's outcome back into its MonitoredRelease (if any).""" with conn.cursor() as cur: cur.execute( - 'SELECT j.state, r."monitoredReleaseId", r.artist, r.album ' - 'FROM "Job" j JOIN "Request" r ON r.id = j."requestId" WHERE j.id = %s', + 'SELECT j.state, r."monitoredReleaseId", r.artist, r.album, mr."rgMbid" ' + 'FROM "Job" j JOIN "Request" r ON r.id = j."requestId" ' + 'LEFT JOIN "MonitoredRelease" mr ON mr.id = r."monitoredReleaseId" ' + 'WHERE j.id = %s', (job_id,), ) row = cur.fetchone() if row is None: return - state, rel_id, artist, album = row + state, rel_id, artist, album, rg_mbid = row if rel_id is None or state != "imported": return # needs_attention (or in-flight) leaves the release wanted + # A successful import supersedes any earlier failed attempts for this release. + _retire_failed_attempts(conn, rel_id) + with conn.cursor() as cur: cur.execute( - 'SELECT "qualityClass" FROM "LibraryItem" WHERE artist = %s AND album = %s', - (artist, album), + 'SELECT max("qualityClass") FROM "LibraryItem" WHERE ' + ' ("rgMbid" IS NOT NULL AND "rgMbid" = %s) ' + ' OR ("rgMbid" IS NULL AND artist = %s AND album = %s)', + (rg_mbid, artist, album), ) lib = cur.fetchone() quality = lib[0] if lib else None diff --git a/worker/tests/test_monitor_enqueue.py b/worker/tests/test_monitor_enqueue.py index f93dc44..c4318fb 100644 --- a/worker/tests/test_monitor_enqueue.py +++ b/worker/tests/test_monitor_enqueue.py @@ -72,6 +72,52 @@ def test_grabbed_past_window_is_fulfilled_not_enqueued(conn): assert _state(conn, rel_id) == "fulfilled" +def test_owned_by_rgmbid_despite_name_mismatch_is_fulfilled(conn): + # The monitor followed "Kanye West" but the library stores MB's canonical "Ye"; matching on + # rgMbid recognises ownership so the release is fulfilled, not re-pressed. + rel_id = insert_monitored_release(conn, artist_name="Kanye West", album="Graduation", + rg_mbid="rg-grad", monitored=True, state="wanted") + with conn.cursor() as cur: + cur.execute( + 'INSERT INTO "LibraryItem" (id, artist, album, path, source, format, ' + '"qualityClass", "rgMbid", "importedAt") ' + "VALUES (gen_random_uuid()::text, 'Ye', 'Graduation', '/m', 'soulseek', 'FLAC', 2, " + "'rg-grad', now())" + ) + conn.commit() + assert enqueue_due(conn, CFG) == 0 + assert _state(conn, rel_id) == "fulfilled" + + +def test_re_enqueue_retires_prior_failed_attempt(conn): + # A hard-to-match release re-pressed each retry interval must not pile up needs_attention + # Request rows; the prior failed attempt is retired when the next one is enqueued. + rel_id = insert_monitored_release(conn, artist_name="X", album="Y", rg_mbid="rg-1", + monitored=True, state="wanted") + with conn.cursor() as cur: + cur.execute( + 'INSERT INTO "Request" (id, artist, album, status, "monitoredReleaseId", "createdAt") ' + "VALUES (gen_random_uuid()::text, 'X', 'Y', 'needs_attention', %s, now()) RETURNING id", + (rel_id,), + ) + req = cur.fetchone()[0] + cur.execute( + 'INSERT INTO "Job" (id, "requestId", state, "currentStage", attempts, "createdAt", "updatedAt") ' + "VALUES (gen_random_uuid()::text, %s, 'needs_attention', 'rank', 1, now(), now())", + (req,), + ) + conn.commit() + assert enqueue_due(conn, CFG) == 1 + assert _job_count_for(conn, rel_id) == 1 # old failed attempt retired, one fresh attempt + with conn.cursor() as cur: + cur.execute( + 'SELECT j.state FROM "Job" j JOIN "Request" r ON r.id = j."requestId" ' + 'WHERE r."monitoredReleaseId" = %s', + (rel_id,), + ) + assert cur.fetchone()[0] == "requested" + + def test_already_in_library_at_cutoff_is_fulfilled(conn): rel_id = insert_monitored_release(conn, artist_name="A", album="B", rg_mbid="rg-1", monitored=True, state="wanted") diff --git a/worker/tests/test_monitor_reconcile.py b/worker/tests/test_monitor_reconcile.py index 42f7426..bb7cd02 100644 --- a/worker/tests/test_monitor_reconcile.py +++ b/worker/tests/test_monitor_reconcile.py @@ -22,17 +22,27 @@ def _make_job(conn, rel_id, artist, album, state): return req, job -def _add_library(conn, req, artist, album, quality): +def _add_library(conn, req, artist, album, quality, rg_mbid=None): with conn.cursor() as cur: cur.execute( 'INSERT INTO "LibraryItem" (id, "requestId", artist, album, path, source, format, ' - '"qualityClass", "importedAt") ' - "VALUES (gen_random_uuid()::text, %s, %s, %s, '/m', 'qobuz', 'FLAC', %s, now())", - (req, artist, album, quality), + '"qualityClass", "rgMbid", "importedAt") ' + "VALUES (gen_random_uuid()::text, %s, %s, %s, '/m', 'qobuz', 'FLAC', %s, %s, now())", + (req, artist, album, quality, rg_mbid), ) conn.commit() +def _needs_attention_count(conn, rel_id): + with conn.cursor() as cur: + cur.execute( + 'SELECT count(*) FROM "Request" r JOIN "Job" j ON j."requestId" = r.id ' + "WHERE r.\"monitoredReleaseId\" = %s AND j.state = 'needs_attention'", + (rel_id,), + ) + return cur.fetchone()[0] + + def _release(conn, rel_id): with conn.cursor() as cur: cur.execute( @@ -106,6 +116,42 @@ def test_fulfill_owned_releases_ignores_below_cutoff(conn): assert _release(conn, rel)[0] == "wanted" +def test_reconcile_matches_by_rgmbid_when_artist_name_canonicalized(conn): + # Real 'Kanye West' -> 'Ye' bug: the MonitoredRelease keeps the followed name, but the + # imported LibraryItem carries MusicBrainz's canonical artist. Matching on rgMbid lets the + # release reconcile to fulfilled instead of looking un-owned and re-pressing forever. + rel = insert_monitored_release(conn, artist_name="Kanye West", album="Graduation", + rg_mbid="rg-grad", monitored=True, state="wanted") + req, job = _make_job(conn, rel, "Kanye West", "Graduation", "imported") + _add_library(conn, req, "Ye", "Graduation", 3, rg_mbid="rg-grad") + reconcile(conn, job, CFG) + assert _release(conn, rel) == ("fulfilled", 3, True) + + +def test_reconcile_fulfilled_retires_failed_sibling_attempts(conn): + # 'V' by Maroon 5 phantom: earlier failed attempts leave needs_attention Request rows that + # keep showing on the press even after a later attempt imports. A successful import retires + # those failed siblings so the press stops showing an owned album as needing attention. + rel = insert_monitored_release(conn, artist_name="Maroon 5", album="V", rg_mbid="rg-v", + monitored=True, state="wanted") + _make_job(conn, rel, "Maroon 5", "V", "needs_attention") + _make_job(conn, rel, "Maroon 5", "V", "needs_attention") + req, job = _make_job(conn, rel, "Maroon 5", "V", "imported") + _add_library(conn, req, "Maroon 5", "V", 2) + reconcile(conn, job, CFG) + assert _release(conn, rel)[0] == "fulfilled" + assert _needs_attention_count(conn, rel) == 0 # failed siblings retired + + +def test_fulfill_owned_releases_matches_by_rgmbid(conn): + # Ne-Yo -> Ne‐Yo (typographic hyphen) name drift: still owned by rgMbid. + rel = insert_monitored_release(conn, artist_name="Ne-Yo", album="Because of You", + rg_mbid="rg-ney", monitored=True, state="grabbed") + _add_library(conn, None, "Ne‐Yo", "Because of You", 2, rg_mbid="rg-ney") + assert fulfill_owned_releases(conn, MonitorConfig(quality_cutoff=2)) == 1 + assert _release(conn, rel) == ("fulfilled", 2, True) + + def test_unlinked_job_is_ignored(conn): with conn.cursor() as cur: cur.execute(