fix(worker): match library ownership by rgMbid + retire stale failed attempts
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) <noreply@anthropic.com>
This commit is contained in:
@@ -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))
|
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:
|
with conn.cursor() as cur:
|
||||||
cur.execute(
|
cur.execute(
|
||||||
'SELECT 1 FROM "LibraryItem" WHERE artist = %s AND album = %s '
|
'SELECT 1 FROM "LibraryItem" WHERE "qualityClass" >= %s AND ('
|
||||||
'AND "qualityClass" >= %s LIMIT 1',
|
' ("rgMbid" IS NOT NULL AND "rgMbid" = %s) '
|
||||||
(artist, album, cutoff),
|
' OR ("rgMbid" IS NULL AND artist = %s AND album = %s)) LIMIT 1',
|
||||||
|
(cutoff, rg_mbid, artist, album),
|
||||||
)
|
)
|
||||||
return cur.fetchone() is not None
|
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:
|
def _enqueue_one(conn: psycopg.Connection, rel_id: str, artist: str, album: str) -> None:
|
||||||
with conn.cursor() as cur:
|
with conn.cursor() as cur:
|
||||||
cur.execute(
|
cur.execute(
|
||||||
@@ -118,7 +142,8 @@ def enqueue_due(conn: psycopg.Connection, cfg: MonitorConfig) -> int:
|
|||||||
cur.execute(
|
cur.execute(
|
||||||
'SELECT mr.id, mr."artistName", mr.album, mr.state, mr."currentQualityClass", '
|
'SELECT mr.id, mr."artistName", mr.album, mr.state, mr."currentQualityClass", '
|
||||||
' (mr."firstGrabbedAt" IS NOT NULL '
|
' (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 '
|
'FROM "MonitoredRelease" mr '
|
||||||
'WHERE mr.monitored = true '
|
'WHERE mr.monitored = true '
|
||||||
' AND NOT mr.ignored ' # user-ignored releases are never enqueued
|
' 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()
|
rows = cur.fetchall()
|
||||||
|
|
||||||
enqueued = 0
|
enqueued = 0
|
||||||
for rel_id, artist, album, state, quality, window_expired in rows:
|
for rel_id, artist, album, state, quality, window_expired, rg_mbid in rows:
|
||||||
if _in_library_at_cutoff(conn, artist, album, cfg.quality_cutoff):
|
# 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")
|
_set_state(conn, rel_id, "fulfilled")
|
||||||
continue
|
continue
|
||||||
if state == "grabbed" and (window_expired or (quality is not None and quality >= cfg.quality_cutoff)):
|
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:
|
with conn.cursor() as cur:
|
||||||
cur.execute(
|
cur.execute(
|
||||||
'UPDATE "MonitoredRelease" mr SET state = \'fulfilled\', '
|
'UPDATE "MonitoredRelease" mr SET state = \'fulfilled\', '
|
||||||
' "currentQualityClass" = li.q, '
|
' "currentQualityClass" = owned.q, '
|
||||||
' "firstGrabbedAt" = COALESCE(mr."firstGrabbedAt", now()) '
|
' "firstGrabbedAt" = COALESCE(mr."firstGrabbedAt", now()) '
|
||||||
'FROM (SELECT artist, album, max("qualityClass") AS q FROM "LibraryItem" '
|
'FROM ('
|
||||||
' GROUP BY artist, album) li '
|
' SELECT mr2.id AS mid, max(li."qualityClass") AS q '
|
||||||
'WHERE li.artist = mr."artistName" AND li.album = mr.album '
|
' FROM "MonitoredRelease" mr2 JOIN "LibraryItem" li ON ('
|
||||||
" AND mr.monitored = true AND mr.state <> 'fulfilled' AND li.q >= %s",
|
# 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,),
|
(cfg.quality_cutoff,),
|
||||||
)
|
)
|
||||||
fixed = cur.rowcount
|
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)."""
|
"""Feed a finished job's outcome back into its MonitoredRelease (if any)."""
|
||||||
with conn.cursor() as cur:
|
with conn.cursor() as cur:
|
||||||
cur.execute(
|
cur.execute(
|
||||||
'SELECT j.state, r."monitoredReleaseId", r.artist, r.album '
|
'SELECT j.state, r."monitoredReleaseId", r.artist, r.album, mr."rgMbid" '
|
||||||
'FROM "Job" j JOIN "Request" r ON r.id = j."requestId" WHERE j.id = %s',
|
'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,),
|
(job_id,),
|
||||||
)
|
)
|
||||||
row = cur.fetchone()
|
row = cur.fetchone()
|
||||||
if row is None:
|
if row is None:
|
||||||
return
|
return
|
||||||
state, rel_id, artist, album = row
|
state, rel_id, artist, album, rg_mbid = row
|
||||||
if rel_id is None or state != "imported":
|
if rel_id is None or state != "imported":
|
||||||
return # needs_attention (or in-flight) leaves the release wanted
|
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:
|
with conn.cursor() as cur:
|
||||||
cur.execute(
|
cur.execute(
|
||||||
'SELECT "qualityClass" FROM "LibraryItem" WHERE artist = %s AND album = %s',
|
'SELECT max("qualityClass") FROM "LibraryItem" WHERE '
|
||||||
(artist, album),
|
' ("rgMbid" IS NOT NULL AND "rgMbid" = %s) '
|
||||||
|
' OR ("rgMbid" IS NULL AND artist = %s AND album = %s)',
|
||||||
|
(rg_mbid, artist, album),
|
||||||
)
|
)
|
||||||
lib = cur.fetchone()
|
lib = cur.fetchone()
|
||||||
quality = lib[0] if lib else None
|
quality = lib[0] if lib else None
|
||||||
|
|||||||
@@ -72,6 +72,52 @@ def test_grabbed_past_window_is_fulfilled_not_enqueued(conn):
|
|||||||
assert _state(conn, rel_id) == "fulfilled"
|
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):
|
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",
|
rel_id = insert_monitored_release(conn, artist_name="A", album="B", rg_mbid="rg-1",
|
||||||
monitored=True, state="wanted")
|
monitored=True, state="wanted")
|
||||||
|
|||||||
@@ -22,17 +22,27 @@ def _make_job(conn, rel_id, artist, album, state):
|
|||||||
return req, job
|
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:
|
with conn.cursor() as cur:
|
||||||
cur.execute(
|
cur.execute(
|
||||||
'INSERT INTO "LibraryItem" (id, "requestId", artist, album, path, source, format, '
|
'INSERT INTO "LibraryItem" (id, "requestId", artist, album, path, source, format, '
|
||||||
'"qualityClass", "importedAt") '
|
'"qualityClass", "rgMbid", "importedAt") '
|
||||||
"VALUES (gen_random_uuid()::text, %s, %s, %s, '/m', 'qobuz', 'FLAC', %s, now())",
|
"VALUES (gen_random_uuid()::text, %s, %s, %s, '/m', 'qobuz', 'FLAC', %s, %s, now())",
|
||||||
(req, artist, album, quality),
|
(req, artist, album, quality, rg_mbid),
|
||||||
)
|
)
|
||||||
conn.commit()
|
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):
|
def _release(conn, rel_id):
|
||||||
with conn.cursor() as cur:
|
with conn.cursor() as cur:
|
||||||
cur.execute(
|
cur.execute(
|
||||||
@@ -106,6 +116,42 @@ def test_fulfill_owned_releases_ignores_below_cutoff(conn):
|
|||||||
assert _release(conn, rel)[0] == "wanted"
|
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):
|
def test_unlinked_job_is_ignored(conn):
|
||||||
with conn.cursor() as cur:
|
with conn.cursor() as cur:
|
||||||
cur.execute(
|
cur.execute(
|
||||||
|
|||||||
Reference in New Issue
Block a user