fix(worker): judge download completeness by the source's edition, not MB's
The completeness + duration guards used MusicBrainz's target.track_count / total_duration_s, but MB picks an arbitrary releases[0] that is often a deluxe/expanded edition with more tracks (and runtime) than the standard album a source legitimately delivers. So complete standard-edition downloads were falsely rejected — The Script 'No Sound Without Silence' (11 vs MB 12) as 'incomplete download', Skillet 'Unleashed' (12 vs MB's 20-track deluxe) as 'too short'. Now completeness is judged against the chosen source's own track count (winner.track_count): a truncated download (fewer files than the source promised) still fails, and the duration expectation is scaled to the delivered edition's size. A separate 'implausibly small vs MB' guard (source has <= half MB's tracks) still rejects a lone-track 'full album' video. Genuine partials (e.g. John Mayer, a Qobuz track failing mid-download) still correctly fail. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Jonathan
@@ -375,23 +375,39 @@ def run_pipeline(
|
||||
|
||||
# 5. verify completeness on the staging copy, then promote + tag
|
||||
_set_state(conn, job_id, "tagging", "tag")
|
||||
expected = target.track_count if target.track_count is not None else winner.track_count
|
||||
# Also count the NON-EMPTY audio actually on disk: a silently-skipped track leaves a
|
||||
# 0-byte placeholder that the adapter still counts, so trust the files when any are
|
||||
# staged. (`staged and ...` keeps fake adapters that stage nothing on the count path.)
|
||||
# Judge completeness against what the CHOSEN SOURCE offered, NOT MusicBrainz's canonical
|
||||
# release: MB picks an arbitrary releases[0] that is often a deluxe/expanded edition with
|
||||
# more tracks than the standard album a source legitimately delivers, so gating purely on
|
||||
# target.track_count falsely rejects complete standard-edition downloads (e.g. The Script
|
||||
# "No Sound Without Silence" 11 vs MB 12, Skillet "Unleashed" 12 vs MB's 20-track deluxe).
|
||||
expected = winner.track_count
|
||||
# (a) Truncated: fewer files than the source promised (a track failed mid-download).
|
||||
# Count the NON-EMPTY audio actually on disk too: a silently-skipped track leaves a 0-byte
|
||||
# placeholder the adapter still counts, so trust the files when any are staged. (`staged
|
||||
# and ...` keeps fake adapters that stage nothing on the count path.)
|
||||
staged = _count_staged_audio(staging)
|
||||
if result.track_count < expected or (staged and staged < expected):
|
||||
truncated = result.track_count < expected or bool(staged and staged < expected)
|
||||
# (b) Implausibly small vs MB's album — a lone-track "full album" video or a wrong match,
|
||||
# as distinct from a legitimately smaller edition (standard vs deluxe). A real edition
|
||||
# keeps more than half of MB's tracks; a 1-of-12 match does not.
|
||||
too_small = target.track_count is not None and expected * 2 <= target.track_count
|
||||
if truncated or too_small:
|
||||
_fail(conn, job_id, "incomplete download")
|
||||
return
|
||||
|
||||
# Duration check: even with the right track count, a truncated file or a short preview
|
||||
# substituted for a real track shows up as a total playtime well under MB's expected
|
||||
# total. Only applies when both are known (measured is None for the offline fakes).
|
||||
# substituted for a real track shows up as a total playtime well under the expected total.
|
||||
# Scale MB's total to the DELIVERED edition's size (source vs MB track count) so a smaller
|
||||
# standard edition isn't measured against a larger deluxe edition's runtime. Only applies
|
||||
# when the total is known (measured is None for the offline fakes).
|
||||
if target.total_duration_s:
|
||||
expected_total = target.total_duration_s
|
||||
if target.track_count:
|
||||
expected_total *= winner.track_count / target.track_count
|
||||
measured = measure_duration(staging)
|
||||
if measured is not None and measured < target.total_duration_s * _DURATION_MIN_RATIO:
|
||||
if measured is not None and measured < expected_total * _DURATION_MIN_RATIO:
|
||||
_fail(conn, job_id,
|
||||
f"download too short ({measured / 60:.0f} of ~{target.total_duration_s / 60:.0f} min)")
|
||||
f"download too short ({measured / 60:.0f} of ~{expected_total / 60:.0f} min)")
|
||||
return
|
||||
|
||||
final = album_dir(dest_root, target)
|
||||
|
||||
@@ -0,0 +1,101 @@
|
||||
"""MusicBrainz picks an arbitrary releases[0] that is often a deluxe/expanded edition with more
|
||||
tracks (and a longer runtime) than the standard album a source legitimately delivers. Completeness
|
||||
and the duration guard must be judged against what the CHOSEN SOURCE offered, not MB's canonical
|
||||
edition — otherwise a complete standard-edition download is falsely rejected as 'incomplete'
|
||||
(The Script "No Sound Without Silence": 11 delivered vs MB 12) or 'too short' (Skillet "Unleashed":
|
||||
12 delivered ~40 min vs MB's 20-track deluxe ~73 min). A genuinely short download must still fail.
|
||||
"""
|
||||
import os
|
||||
from dataclasses import replace
|
||||
|
||||
from lyra_worker.adapters.fakes import FakeQobuz
|
||||
from lyra_worker.claim import claim_next
|
||||
from lyra_worker.pipeline import run_pipeline
|
||||
from lyra_worker.types import DownloadResult, MBTarget
|
||||
from tests.conftest import insert_request
|
||||
|
||||
|
||||
class _StandardEditionQobuz(FakeQobuz):
|
||||
"""Qobuz-like source offering a fixed-size *standard* edition (source_tracks), independent of
|
||||
MB's possibly-larger deluxe count; stages that many real (non-empty) files and reports them."""
|
||||
def __init__(self, source_tracks):
|
||||
super().__init__()
|
||||
self._n = source_tracks
|
||||
|
||||
def search(self, target):
|
||||
return super().search(replace(target, track_count=self._n))
|
||||
|
||||
def download(self, candidate, dest, on_progress):
|
||||
os.makedirs(dest, exist_ok=True)
|
||||
for i in range(candidate.track_count):
|
||||
with open(os.path.join(dest, f"{i + 1:02d} Track.flac"), "wb") as fh:
|
||||
fh.write(b"audio")
|
||||
on_progress(1.0)
|
||||
return DownloadResult(ok=True, path=dest, track_count=candidate.track_count)
|
||||
|
||||
|
||||
class _PartialQobuz(FakeQobuz):
|
||||
"""Promises `promised` tracks but one fails mid-download: stages and reports `promised - 1`
|
||||
(the real John Mayer "Roll it on Home" IncompleteRead case)."""
|
||||
def __init__(self, promised):
|
||||
super().__init__()
|
||||
self._n = promised
|
||||
|
||||
def search(self, target):
|
||||
return super().search(replace(target, track_count=self._n))
|
||||
|
||||
def download(self, candidate, dest, on_progress):
|
||||
os.makedirs(dest, exist_ok=True)
|
||||
delivered = candidate.track_count - 1
|
||||
for i in range(delivered):
|
||||
with open(os.path.join(dest, f"{i + 1:02d} Track.flac"), "wb") as fh:
|
||||
fh.write(b"audio")
|
||||
return DownloadResult(ok=True, path=dest, track_count=delivered)
|
||||
|
||||
|
||||
class _DeluxeResolver:
|
||||
"""Resolver whose canonical release is a larger (deluxe) edition than the source delivers."""
|
||||
def __init__(self, mb_tracks, mb_total_s):
|
||||
self._t, self._s = mb_tracks, mb_total_s
|
||||
|
||||
def resolve(self, artist, album):
|
||||
return MBTarget(artist=artist, album=album, track_count=self._t, total_duration_s=self._s)
|
||||
|
||||
|
||||
def _state_error(conn, job_id):
|
||||
with conn.cursor() as cur:
|
||||
cur.execute('SELECT state, error FROM "Job" WHERE id = %s', (job_id,))
|
||||
return cur.fetchone()
|
||||
|
||||
|
||||
def test_standard_edition_not_rejected_as_incomplete(conn):
|
||||
# The Script: source delivers the 11-track standard edition; MB's canonical release is 12.
|
||||
job_id = insert_request(conn, artist="The Script", album="No Sound Without Silence")
|
||||
claim_next(conn)
|
||||
run_pipeline(conn, job_id, [_StandardEditionQobuz(source_tracks=11)],
|
||||
resolver=_DeluxeResolver(mb_tracks=12, mb_total_s=2943),
|
||||
measure_duration=lambda _s: 2700.0, dest_root="/tmp/lib-se")
|
||||
assert _state_error(conn, job_id) == ("imported", None)
|
||||
|
||||
|
||||
def test_smaller_edition_not_rejected_as_too_short(conn):
|
||||
# Skillet: source delivers the 12-track standard (~40 min); MB's release is a 20-track deluxe
|
||||
# (~73 min). The unscaled duration guard falsely rejects; scaling to the delivered edition
|
||||
# (12/20) must let it pass.
|
||||
job_id = insert_request(conn, artist="Skillet", album="Unleashed")
|
||||
claim_next(conn)
|
||||
run_pipeline(conn, job_id, [_StandardEditionQobuz(source_tracks=12)],
|
||||
resolver=_DeluxeResolver(mb_tracks=20, mb_total_s=4380),
|
||||
measure_duration=lambda _s: 2400.0, dest_root="/tmp/lib-se")
|
||||
assert _state_error(conn, job_id) == ("imported", None)
|
||||
|
||||
|
||||
def test_genuinely_incomplete_still_rejected(conn):
|
||||
# John Mayer: the source promised 12 tracks but one failed mid-download → only 11 land.
|
||||
# This is a real partial and must still be rejected (the fix must not swallow it).
|
||||
job_id = insert_request(conn, artist="John Mayer", album="The Search for Everything")
|
||||
claim_next(conn)
|
||||
run_pipeline(conn, job_id, [_PartialQobuz(promised=12)],
|
||||
resolver=_DeluxeResolver(mb_tracks=12, mb_total_s=2580),
|
||||
measure_duration=lambda _s: 2400.0, dest_root="/tmp/lib-se")
|
||||
assert _state_error(conn, job_id) == ("needs_attention", "incomplete download")
|
||||
Reference in New Issue
Block a user