chore(release): v0.3.0

Reviewed-on: #6

README.md

@@ -29,7 +29,14 @@ Two tiers, and no third.
 | `name` | Normalised artist and title | Everything the first tier could not carry |
 | `none` | — | Recorded as a miss, never guessed at |
 
-The normalisation is the load-bearing part, because the two sides disagree in
+The name tier does almost all of the work. A recording MBID is exact when it
+lands, but MusicBrainz holds a separate recording per release, and Last.fm and
+Lidarr rarely pick the same one: on a real library, two thirds of scrobbles
+carry a recording id and barely a twentieth of them join on it. The report
+counts how many carried an id and matched on name anyway, which is the measure
+of that disagreement.
+
+So the normalisation is the load-bearing part, because the two sides disagree in
 predictable ways. It folds case and accents, drops guest credits (`Yellowcard
 feat. Tay Jardine` against a tag of `Yellowcard`), strips a trailing
 version suffix (`(Remastered 2011)`, `- Live`), expands `&`, and removes a
@@ -42,6 +49,56 @@ It leans towards collapsing too much. A false match makes something look
 played; a missed match makes something look abandoned. Only one of those
 deletes music.
 
+### Indexing quirks
+
+Albums are fetched from the **unfiltered** `GET /api/v1/album` first: one
+request, and the only path that skips albums whose artist metadata is missing
+rather than dereferencing it.
+
+That is not enough on its own. Every album endpoint maps through a resource
+that picks the release with `SingleOrDefault(x => x.Monitored)`, which throws
+for an album with **two monitored releases** and takes the whole response with
+it:
+
+```
+HTTP 500: Sequence contains more than one element
+```
+
+When the bulk call dies that way, the indexer falls back to one request per
+artist. It cannot avoid the exception, but it confines it to whichever artist
+owns the offending album and names them in the log — which is the only
+practical way to find it in a large library. Open that artist in Lidarr and
+check the Releases tab of each album: exactly one release may be monitored.
+
+Losing an artist's albums does not cost their tracks, which come from a
+different endpoint with a different mapper, so matching is unaffected. A cull
+would not be, and the report says so.
+
+### Talking to Lidarr
+
+Indexing is two requests per artist, and more when the album fallback fires. On
+a large library that is thousands of requests in a few minutes. `urllib` opens a
+new TCP connection and performs a new DNS lookup for every one of them, which is
+enough to exhaust a container's resolver and produce `[Errno -3] Try again` on
+everything at once. The client therefore holds one connection open per host and
+resolves once.
+
+Transient failures — a dropped connection, a resolver hiccup, `429`, `502`,
+`503`, `504` — are retried with a backoff. An HTTP `500` is not: it is an
+unhandled exception inside Lidarr's own serialisation and will be raised again
+identically. That distinction also decides whether a failure is worth
+investigating; a library-wide outage is not probed artist by artist, because
+doing so multiplies the load that caused it.
+
+A local address is preferable to a public hostname here. It removes DNS, the
+reverse proxy and its timeouts from a path that needs none of them.
+
+Tracks and files have no unfiltered endpoint — Lidarr rejects a call with no
+filter — so they stay per artist. If one artist cannot be served, that artist is
+skipped and the run continues, but the count is recorded and the coverage report
+says so loudly. A missing artist makes their played music look cold, so an
+incomplete index must never be culled against.
+
 ### Reading the coverage report
 
 Matched against unmatched is the wrong comparison — most unmatched listening is
@@ -52,10 +109,18 @@ matcher. The line to watch is:
 unmatched by an artist the library holds: N pairs, M plays
 ```
 
-That is a track that was played, sitting beside a file it should have matched.
+That is a track that was played by an artist the library holds. It is then
-Those are the matcher's real misses, and every one is a candidate for being
+split again, because owning an artist is a weak proxy for owning a track:
-wrongly called cold in stage four. The report lists the worst fifteen by play
+
-count so they can be eyeballed.
+- **the title exists under another artist** — an attribution disagreement, a
+  remixer or a guest billed as the artist. These are the genuine misses, and
+  each is a candidate for being wrongly called cold in stage four. The report
+  names the artist the library files them under.
+- **the title is nowhere in the library** — never bought. No amount of matching
+  conjures a file that does not exist.
+
+Counting both as matcher failures overstates the problem and would over-block
+the cull. The report lists the worst of each by play count.
 
 ## How the ingest works
 

music_curator.py

@@ -17,6 +17,8 @@ cursor, so an interrupted run resumes from what it actually has.
 
 import argparse
 import fcntl
+import http.client
+import io
 import json
 import logging
 import os
@@ -48,6 +50,11 @@ PAGE_SIZE = 200
 RETRYABLE_ERRORS = {8, 11, 16, 29}
 RETRYABLE_STATUS = {429, 500, 502, 503, 504}
 
+# Lidarr's own list, and 500 is deliberately absent. A 500 from Lidarr is an
+# unhandled exception inside its serialisation, not a busy server; it will be
+# raised again identically, and retrying only delays finding that out.
+LIDARR_RETRYABLE_STATUS = {429, 502, 503, 504}
+
 # Last.fm asks for no more than five requests a second averaged over five
 # minutes. A full backfill is thousands of requests, so it is worth staying
 # well inside that rather than discovering error 29 halfway through.
@@ -180,18 +187,28 @@ VERSION_WORDS = (
     "anniversary",
     "reissue",
     "instrumental",
+    # Drum and bass and its neighbours mark versions their own way.
+    "vip",
+    "bootleg",
+    "rework",
+    "extended",
 )
 _VERSIONS = "|".join(VERSION_WORDS)
 BRACKETED_VERSION = re.compile(
     rf"\s*[\(\[][^\)\]]*\b(?:{_VERSIONS})\b[^\)\]]*[\)\]]\s*$", re.IGNORECASE
 )
-TRAILING_VERSION = re.compile(rf"\s+-\s+[^-]*\b(?:{_VERSIONS})\b.*$", re.IGNORECASE)
+# The suffix is matched lazily rather than as a run of non-hyphens, because the
+# thing being stripped frequently contains hyphens of its own -- "Gold Dust -
+# Shy FX Re-Edit", "Back To Your Roots - Friction & K-Tee Remix".
+TRAILING_VERSION = re.compile(rf"\s+-\s+.*?\b(?:{_VERSIONS})\b.*$", re.IGNORECASE)
 
-# Last.fm routinely carries the guest credit in the artist field where the file
+# Last.fm routinely carries the guest credit where the file tag holds only the
-# tag holds only the primary artist -- "Yellowcard feat. Tay Jardine" against a
+# primary artist -- "Yellowcard feat. Tay Jardine" against a tag of
-# tag of "Yellowcard". `with` is deliberately absent: it appears in far too many
+# "Yellowcard", or "Self vs Self (feat. In Flames)" against "Self vs Self". The
-# real titles to cut on sight.
+# opening bracket has to be allowed for: requiring whitespace immediately before
-GUEST_CREDIT = re.compile(r"\s+(?:feat|ft|featuring)\b.*$", re.IGNORECASE)
+# the word misses every bracketed credit, which is most of them. `with` is
+# deliberately absent -- it appears in far too many real titles to cut on sight.
+GUEST_CREDIT = re.compile(r"[\s(\[]+(?:feat|ft|featuring)\b.*$", re.IGNORECASE)
 
 LEADING_ARTICLE = re.compile(r"^the\s+")
 # Deleted rather than spaced, so "Don't" and "Dont" agree. Every other mark
@@ -207,7 +224,18 @@ class LastfmError(Exception):
 
 
 class LidarrError(Exception):
-    """A Lidarr request that failed."""
+    """A Lidarr request that failed.
+
+    `transient` separates "the network or the server had a moment" from "this
+    request will fail identically forever". The distinction matters twice: only
+    the first is worth retrying, and only the second is worth investigating,
+    since probing a library-wide outage artist by artist multiplies the load
+    that caused it.
+    """
+
+    def __init__(self, message, transient=False):
+        super().__init__(message)
+        self.transient = transient
 
 
 def normalise(text):
@@ -645,33 +673,140 @@ def sync_loved(client, store, user):
     return len(rows)
 
 
+class KeepAlive:
+    """A transport that holds one connection open per host.
+
+    urllib opens a fresh TCP connection -- and performs a fresh DNS lookup --
+    for every request it makes. Indexing a library is two requests per artist,
+    which on a large collection is thousands of lookups inside a few minutes.
+    That is enough to exhaust a container's resolver, and the failure it
+    produces is `[Errno -3] Try again` on everything at once. Resolving once and
+    reusing the socket removes the cause rather than papering over it, and is
+    considerably faster besides.
+    """
+
+    def __init__(self, timeout=60):
+        self.timeout = timeout
+        self._connections = {}
+
+    def __call__(self, url, timeout=None, headers=None):
+        parsed = urllib.parse.urlparse(url)
+        key = (parsed.scheme, parsed.hostname, parsed.port)
+        target = parsed.path + (f"?{parsed.query}" if parsed.query else "")
+        request_headers = {**(headers or {}), "Accept": "application/json"}
+
+        # Two attempts, because a kept-alive connection the server has since
+        # closed fails on use rather than announcing itself. The second attempt
+        # is on a fresh socket.
+        for attempt in (1, 2):
+            connection = self._connections.get(key)
+            if connection is None:
+                connection = self._connect(parsed, timeout or self.timeout)
+                self._connections[key] = connection
+            try:
+                connection.request("GET", target, headers=request_headers)
+                response = connection.getresponse()
+                body = response.read()
+            except (http.client.HTTPException, OSError) as error:
+                self.close(key)
+                if attempt == 2:
+                    raise urllib.error.URLError(error) from error
+                continue
+
+            if response.status >= 300:
+                # Includes redirects: this client does not follow them, and one
+                # here means the URL is pointing somewhere unintended.
+                raise urllib.error.HTTPError(
+                    url, response.status, response.reason, response.headers, io.BytesIO(body)
+                )
+            return body.decode("utf-8")
+        raise urllib.error.URLError("unreachable")
+
+    @staticmethod
+    def _connect(parsed, timeout):
+        if parsed.scheme == "https":
+            return http.client.HTTPSConnection(parsed.hostname, parsed.port, timeout=timeout)
+        return http.client.HTTPConnection(parsed.hostname, parsed.port, timeout=timeout)
+
+    def close(self, key=None):
+        for handle in [self._connections.pop(key, None)] if key else self._connections.values():
+            if handle is not None:
+                handle.close()
+        if key is None:
+            self._connections.clear()
+
+
 class Lidarr:
     """Minimal read-only Lidarr client.
 
-    No retries: Lidarr is on the same LAN as this, and a failure there means it
+    Retries only what is worth retrying. A dropped connection or a resolver
-    is down or the key is wrong, neither of which improves on a second attempt.
+    hiccup is transient; an HTTP 500 out of Lidarr is an exception in its own
+    serialisation and will be thrown again identically, so spending three
+    attempts on it only slows down finding out.
     """
 
-    def __init__(self, url, api_key, timeout=60, transport=None):
+    def __init__(self, url, api_key, timeout=60, attempts=3, backoff=1.0, transport=None):
         self.root = url.rstrip("/")
         self.api_key = api_key
         self.timeout = timeout
-        self.transport = transport or http_get
+        self.attempts = attempts
+        self.backoff = backoff
+        self.transport = transport or KeepAlive(timeout)
 
     def get(self, path, params=None):
         """Return the decoded response for one API path."""
         query = urllib.parse.urlencode(params or {})
         url = f"{self.root}/api/v1/{path}" + (f"?{query}" if query else "")
-        try:
+
-            body = self.transport(url, timeout=self.timeout, headers={"X-Api-Key": self.api_key})
+        for attempt in range(1, self.attempts + 1):
-        except urllib.error.HTTPError as error:
+            try:
-            raise LidarrError(f"{path}: HTTP {error.code}") from error
+                body = self.transport(
-        except (urllib.error.URLError, TimeoutError) as error:
+                    url, timeout=self.timeout, headers={"X-Api-Key": self.api_key}
-            raise LidarrError(f"{path}: {error}") from error
+                )
-        try:
+            except urllib.error.HTTPError as error:
-            return json.loads(body)
+                detail = error_detail(error)
-        except json.JSONDecodeError as error:
+                message = f"GET {url}: HTTP {error.code}{': ' + detail if detail else ''}"
-            raise LidarrError(f"{path}: malformed response") from error
+                if error.code not in LIDARR_RETRYABLE_STATUS:
+                    raise LidarrError(message)
+                if attempt >= self.attempts:
+                    raise LidarrError(message, transient=True)
+            except (urllib.error.URLError, TimeoutError) as error:
+                message = f"GET {url}: {error}"
+                if attempt >= self.attempts:
+                    raise LidarrError(message, transient=True) from error
+            else:
+                try:
+                    return json.loads(body)
+                except json.JSONDecodeError as error:
+                    raise LidarrError(f"GET {url}: malformed response") from error
+
+            pause = min(self.backoff * 2 ** (attempt - 1), BACKOFF_CEILING_SECONDS)
+            logger.warning("%s; retrying in %.0fs", message, pause)
+            time.sleep(pause)
+
+        raise LidarrError(f"GET {url}: gave up after {self.attempts} attempts", transient=True)
+
+
+def error_detail(error, limit=300):
+    """Return whatever the server said about a failure, for the log.
+
+    A bare status code sends you looking in the wrong place; Lidarr puts the
+    actual exception in the body.
+    """
+    try:
+        body = error.read().decode("utf-8", "replace").strip()
+    except Exception:  # noqa: BLE001 - diagnostics must never raise
+        return ""
+    if not body:
+        return ""
+    try:
+        payload = json.loads(body)
+    except json.JSONDecodeError:
+        return body[:limit]
+    for key in ("message", "description", "error"):
+        if payload.get(key):
+            return str(payload[key])[:limit]
+    return body[:limit]
 
 
 def parse_added(value):
@@ -684,6 +819,82 @@ def parse_added(value):
         return None
 
 
+def find_bad_albums(client, artist_id, artist_name):
+    """Name the specific albums Lidarr cannot serialise for one artist.
+
+    Runs only once that artist's album fetch has already failed, so the extra
+    requests are spent on a problem that already exists. Tracks come from an
+    endpoint that still works, and their album ids give a list to probe one at a
+    time; the ones that throw are the culprits. A track title from each is
+    enough to recognise the album in the UI, which the id alone is not.
+    """
+    try:
+        tracks = client.get("track", {"artistId": artist_id})
+    except LidarrError as error:
+        logger.warning("could not probe %s for the offending album: %s", artist_name, error)
+        return []
+
+    sample = {}
+    for track in tracks:
+        sample.setdefault(track.get("albumId"), track.get("title") or "")
+
+    bad = []
+    for album_id, title in sorted(sample.items(), key=lambda item: item[0] or 0):
+        if not album_id:
+            continue
+        try:
+            client.get("album", {"albumIds": album_id})
+        except LidarrError:
+            bad.append((album_id, title))
+    return bad
+
+
+def fetch_albums(client, artists):
+    """Return albums grouped by artist id, plus the artists whose albums failed.
+
+    Every album endpoint maps through a resource that picks the release with
+    `SingleOrDefault(x => x.Monitored)`, which throws for an album with two
+    monitored releases -- "Sequence contains more than one element" -- and takes
+    the entire response down with it.
+
+    The unfiltered endpoint is one request and is the only one that also skips
+    albums whose artist metadata is missing, so it is tried first. When it dies,
+    asking per artist confines the loss to whichever artist owns the offending
+    album, and names them, which is the only way to find it.
+    """
+    try:
+        grouped = {}
+        for album in client.get("album"):
+            grouped.setdefault(album.get("artistId"), []).append(album)
+        return grouped, []
+    except LidarrError as error:
+        logger.warning("fetching all albums failed (%s)", error)
+        logger.warning("falling back to one request per artist to isolate the bad album")
+
+    grouped, failed = {}, []
+    for artist in artists:
+        artist_id = artist["id"]
+        try:
+            grouped[artist_id] = client.get("album", {"artistId": artist_id})
+        except LidarrError as error:
+            name = artist.get("artistName") or str(artist_id)
+            logger.warning("could not fetch albums for %s: %s", name, error)
+            failed.append(name)
+            # Only worth probing a deterministic failure. When the network or
+            # the resolver is the problem, every artist fails, and probing each
+            # of them album by album multiplies the load that caused it.
+            if not error.transient:
+                for album_id, sample in find_bad_albums(client, artist_id, name):
+                    logger.warning(
+                        "  album id %d is the one Lidarr cannot serialise (it holds the"
+                        " track %r). Open it in Lidarr and leave exactly one release"
+                        " monitored.",
+                        album_id,
+                        sample,
+                    )
+    return grouped, failed
+
+
 def index_library(client, store):
     """Rebuild the library index from Lidarr. Returns (artists, tracks).
 
@@ -693,6 +904,9 @@ def index_library(client, store):
     """
     artists = client.get("artist")
     artist_rows, album_rows, track_rows = [], [], []
+    skipped = []
+
+    albums_by_artist, album_failures = fetch_albums(client, artists)
 
     for artist in artists:
         artist_id = artist["id"]
@@ -708,7 +922,7 @@ def index_library(client, store):
             )
         )
 
-        for album in client.get("album", {"artistId": artist_id}):
+        for album in albums_by_artist.get(artist_id, []):
             album_rows.append(
                 (
                     album["id"],
@@ -720,11 +934,24 @@ def index_library(client, store):
                 )
             )
 
-        # Paths and the date a track landed live on the file, not the track.
+        # Tracks and files have no unfiltered endpoint -- Lidarr rejects a call
-        files = {
+        # with no filter at all -- so these stay per artist. One artist it
-            handle["id"]: handle for handle in client.get("trackfile", {"artistId": artist_id})
+        # cannot serve must not cost the whole index, but it cannot pass
-        }
+        # silently either: their tracks end up absent, and every scrobble of
-        for track in client.get("track", {"artistId": artist_id}):
+        # theirs then reads as unmatched.
+        try:
+            # Paths and the date a track landed live on the file, not the track.
+            files = {
+                handle["id"]: handle
+                for handle in client.get("trackfile", {"artistId": artist_id})
+            }
+            tracks = client.get("track", {"artistId": artist_id})
+        except LidarrError as error:
+            logger.warning("could not index %s: %s", name or artist_id, error)
+            skipped.append(name or str(artist_id))
+            continue
+
+        for track in tracks:
             handle = files.get(track.get("trackFileId") or 0) or {}
             title = track.get("title") or ""
             track_rows.append(
@@ -744,6 +971,8 @@ def index_library(client, store):
             )
 
     store.replace_library(artist_rows, album_rows, track_rows)
+    store.set_state("index_skipped", str(len(skipped)))
+    store.set_state("index_albums_skipped", str(len(album_failures)))
     logger.info(
         "library indexed: %d artists, %d albums, %d tracks (%d with files)",
         len(artist_rows),
@@ -751,6 +980,20 @@ def index_library(client, store):
         len(track_rows),
         sum(row[7] for row in track_rows),
     )
+    if skipped:
+        logger.warning(
+            "%d artists could not be indexed, so their tracks are missing: %s",
+            len(skipped),
+            ", ".join(sorted(skipped)[:10]),
+        )
+    if album_failures:
+        logger.warning(
+            "no albums indexed for %d artists: %s."
+            " In Lidarr, open each one and check the Releases tab of their albums:"
+            " exactly one release per album may be monitored.",
+            len(album_failures),
+            ", ".join(sorted(album_failures)[:10]),
+        )
     return len(artist_rows), len(track_rows)
 
 
@@ -817,6 +1060,20 @@ def coverage_report(store):
         return
 
     logger.info("--- match coverage ---")
+    skipped = int(store.get_state("index_skipped") or 0)
+    if skipped:
+        logger.warning(
+            "the index is incomplete: %d artists are missing their tracks, so every"
+            " figure below is a floor. Do not cull against it.",
+            skipped,
+        )
+    albums_skipped = int(store.get_state("index_albums_skipped") or 0)
+    if albums_skipped:
+        logger.warning(
+            "%d artists have no albums indexed. Matching is unaffected -- it runs off"
+            " tracks -- but a cull cannot be run until this is fixed.",
+            albums_skipped,
+        )
     for method in ("mbid", "name", "none"):
         row = store.connection.execute(
             "SELECT COUNT(*) AS pairs, COALESCE(SUM(plays), 0) AS plays"
@@ -831,18 +1088,86 @@ def coverage_report(store):
             row["plays"],
         )
 
+    # Why the mbid tier performs the way it does. A recording id on both sides
+    # that still fails to join means the two disagree about which recording the
+    # song is -- MusicBrainz holds a separate recording per release, and Last.fm
+    # and Lidarr need not have picked the same one. That is not a fault to fix
+    # in the matcher; it is the reason the name tier has to carry the load.
+    library_with_mbid = store.scalar(
+        "SELECT COUNT(*) FROM lidarr_track WHERE recording_mbid IS NOT NULL"
+    )
+    logger.info(
+        "library tracks carrying a recording MBID: %d of %d (%.1f%%)",
+        library_with_mbid,
+        tracks,
+        100 * library_with_mbid / tracks,
+    )
+    disagreed = store.connection.execute(
+        "SELECT COUNT(*) AS pairs, COALESCE(SUM(plays), 0) AS plays FROM scrobble_key"
+        " WHERE track_mbid IS NOT NULL AND method = 'name'"
+    ).fetchone()
+    logger.info(
+        "carried a recording MBID, joined on name instead: %d pairs, %d plays"
+        " -- both sides know the song, they disagree on which recording it is",
+        disagreed["pairs"],
+        disagreed["plays"],
+    )
+
     suspect = store.connection.execute(
         "SELECT COUNT(*) AS pairs, COALESCE(SUM(plays), 0) AS plays FROM scrobble_key k"
         " WHERE k.track_id IS NULL"
         "   AND EXISTS (SELECT 1 FROM lidarr_artist a WHERE a.norm_name = k.norm_artist)"
     ).fetchone()
     logger.info(
-        "unmatched by an artist the library holds: %d pairs, %d plays -- these are the"
+        "unmatched by an artist the library holds: %d pairs, %d plays",
-        " matcher's misses, not music you do not own",
         suspect["pairs"],
         suspect["plays"],
     )
 
+    # Owning an artist is a weak proxy for owning a track, so that figure alone
+    # overstates the matcher's failings. Split it. A title the library holds
+    # under some other artist is an attribution disagreement -- a remixer
+    # credited as the artist, a guest billed as one -- and is a real miss. A
+    # title the library does not hold at all was simply never bought, and no
+    # amount of matching will conjure it.
+    attribution = store.connection.execute(
+        "SELECT COUNT(*) AS pairs, COALESCE(SUM(plays), 0) AS plays FROM scrobble_key k"
+        " WHERE k.track_id IS NULL"
+        "   AND EXISTS (SELECT 1 FROM lidarr_artist a WHERE a.norm_name = k.norm_artist)"
+        "   AND EXISTS (SELECT 1 FROM lidarr_track t WHERE t.norm_title = k.norm_track)"
+    ).fetchone()
+    logger.info(
+        "  of those, the title exists under another artist: %d pairs, %d plays"
+        " -- attribution disagreements, and the genuine misses",
+        attribution["pairs"],
+        attribution["plays"],
+    )
+    logger.info(
+        "  the rest, %d pairs, %d plays: you own the artist but not the track",
+        suspect["pairs"] - attribution["pairs"],
+        suspect["plays"] - attribution["plays"],
+    )
+
+    mismatched = store.connection.execute(
+        "SELECT k.artist, k.track, k.plays,"
+        "       (SELECT a.name FROM lidarr_track t"
+        "          JOIN lidarr_artist a ON a.id = t.artist_id"
+        "         WHERE t.norm_title = k.norm_track LIMIT 1) AS filed_under"
+        "  FROM scrobble_key k"
+        " WHERE k.track_id IS NULL"
+        "   AND EXISTS (SELECT 1 FROM lidarr_artist a WHERE a.norm_name = k.norm_artist)"
+        "   AND EXISTS (SELECT 1 FROM lidarr_track t WHERE t.norm_title = k.norm_track)"
+        " ORDER BY k.plays DESC, k.artist LIMIT 10"
+    ).fetchall()
+    for position, row in enumerate(mismatched, start=1):
+        logger.info(
+            "  attribution %2d: %-45s %4d plays, filed under %s",
+            position,
+            f"{row['artist']} - {row['track']}"[:45],
+            row["plays"],
+            row["filed_under"],
+        )
+
     with_files = store.scalar("SELECT COUNT(*) FROM lidarr_track WHERE has_file = 1")
     played = store.scalar(
         "SELECT COUNT(DISTINCT k.track_id) FROM scrobble_key k"

pyproject.toml

@@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta"
 
 [project]
 name = "music-curator"
-version = "0.2.0"
+version = "0.3.0"
 description = "Ingest a Last.fm listening history and curate a music library from it"
 readme = "README.md"
 requires-python = ">=3.11"

tests/conftest.py

@@ -1,6 +1,10 @@
+import http.server
+import io
 import json
 import os
 import sys
+import threading
+import urllib.error
 import urllib.parse
 
 import pytest
@@ -128,7 +132,10 @@ class FakeLidarr:
     exercise the same stitching the real thing does.
     """
 
-    def __init__(self, artists=()):
+    def __init__(self, artists=(), fail=()):
+        # (path, artistId) pairs the fake refuses to serve, standing in for the
+        # 500s Lidarr returns on data it cannot hydrate.
+        self.fail = set(fail)
         self.artists, self.albums, self.tracks, self.files = [], [], [], []
         for artist_index, entry in enumerate(artists, start=1):
             artist_id = artist_index
@@ -190,10 +197,39 @@ class FakeLidarr:
         }
         self.calls.append((path, query))
 
+        artist_id = int(query.get("artistId", 0))
+        if (path, artist_id) in self.fail:
+            raise urllib.error.HTTPError(
+                url, 500, "Internal Server Error", {}, io.BytesIO(b'{"message": "boom"}')
+            )
+
+        album_ids = query.get("albumIds")
+        if path == "album" and album_ids:
+            album_id = int(album_ids)
+            if ("albumid", album_id) in self.fail:
+                raise urllib.error.HTTPError(
+                    url,
+                    500,
+                    "Internal Server Error",
+                    {},
+                    io.BytesIO(b'{"message": "Sequence contains more than one element"}'),
+                )
+            return json.dumps([row for row in self.albums if row["id"] == album_id])
+
         if path == "artist":
             return json.dumps(self.artists)
-        artist_id = int(query.get("artistId", 0))
         source = {"album": self.albums, "track": self.tracks, "trackfile": self.files}[path]
+        if path == "album" and not artist_id:
+            # Lidarr's unfiltered album endpoint returns the lot.
+            return json.dumps(source)
+        if not artist_id:
+            raise urllib.error.HTTPError(
+                url,
+                400,
+                "Bad Request",
+                {},
+                io.BytesIO(b'{"message": "artistId must be provided"}'),
+            )
         return json.dumps([row for row in source if row["artistId"] == artist_id])
 
 
@@ -207,3 +243,53 @@ def now_playing():
         "mbid": "",
         "@attr": {"nowplaying": "true"},
     }
+
+
+class _CountingServer(http.server.ThreadingHTTPServer):
+    """Counts accepted connections, which is what connection reuse is about."""
+
+    daemon_threads = True
+
+    def __init__(self, *args, **kwargs):
+        self.connections = 0
+        super().__init__(*args, **kwargs)
+
+    def process_request(self, request, client_address):
+        self.connections += 1
+        super().process_request(request, client_address)
+
+
+class _Handler(http.server.BaseHTTPRequestHandler):
+    # Without HTTP/1.1 the server closes after every response and no client
+    # could reuse anything, which would make the test prove nothing.
+    protocol_version = "HTTP/1.1"
+
+    def do_GET(self):
+        if self.path.startswith("/boom"):
+            body, status = b'{"message": "boom"}', 500
+        else:
+            body = json.dumps(
+                {"path": self.path, "key": self.headers.get("X-Api-Key")}
+            ).encode()
+            status = 200
+        self.send_response(status)
+        self.send_header("Content-Type", "application/json")
+        self.send_header("Content-Length", str(len(body)))
+        self.end_headers()
+        self.wfile.write(body)
+
+    def log_message(self, *args):
+        pass
+
+
+@pytest.fixture
+def http_server():
+    """A real local HTTP server, for the one component that talks sockets."""
+    server = _CountingServer(("127.0.0.1", 0), _Handler)
+    thread = threading.Thread(target=server.serve_forever, daemon=True)
+    thread.start()
+    try:
+        yield server, f"http://127.0.0.1:{server.server_port}"
+    finally:
+        server.shutdown()
+        server.server_close()

tests/test_music_curator.py

@@ -1,3 +1,4 @@
+import json
 import urllib.error
 
 import pytest
@@ -500,3 +501,265 @@ def test_matching_is_redone_when_new_scrobbles_arrive(tmp_path):
     music_curator.run_once(client_for(api), store, "lyra", NOW, 0)
 
     assert verdict(store, "Yellowcard", "Transmission Home")[0] == "name"
+
+
+def test_albums_come_from_the_unfiltered_endpoint(tmp_path):
+    """One request, and the only path that skips albums it cannot hydrate."""
+    api = FakeLidarr(LIBRARY)
+    store = store_at(tmp_path)
+
+    music_curator.index_library(music_curator.Lidarr("http://lidarr", "key", transport=api), store)
+
+    album_calls = [query for path, query in api.calls if path == "album"]
+    assert album_calls == [{}]
+    assert store.scalar("SELECT COUNT(*) FROM lidarr_album") == 3
+
+
+def test_a_bad_album_falls_back_to_asking_per_artist(tmp_path):
+    """An album with two monitored releases throws in the resource mapper, so
+    the bulk call dies wholesale. Per artist, only its owner is lost."""
+    # The bulk call fails because artist 2 owns the offending album.
+    api = FakeLidarr(LIBRARY, fail=[("album", 0), ("album", 2)])
+    store = store_at(tmp_path)
+
+    music_curator.index_library(music_curator.Lidarr("http://lidarr", "key", transport=api), store)
+
+    assert store.scalar("SELECT COUNT(*) FROM lidarr_album WHERE artist_id = 2") == 0
+    # The other two artists keep their albums.
+    assert store.scalar("SELECT COUNT(*) FROM lidarr_album") == 2
+    assert store.get_state("index_albums_skipped") == "1"
+
+
+def test_a_bad_album_does_not_cost_that_artist_their_tracks(tmp_path):
+    """Tracks come from a different endpoint with a different mapper, so
+    matching survives an album Lidarr cannot serialise."""
+    api = FakeLidarr(LIBRARY, fail=[("album", 0), ("album", 2)])
+    store = store_at(tmp_path)
+
+    music_curator.index_library(music_curator.Lidarr("http://lidarr", "key", transport=api), store)
+
+    assert store.scalar("SELECT COUNT(*) FROM lidarr_track WHERE artist_id = 2") == 1
+    assert store.get_state("index_skipped") == "0"
+
+
+def test_an_artist_lidarr_cannot_serve_does_not_kill_the_index(tmp_path):
+    # Artist 2 is AC/DC in LIBRARY; its track lookup fails.
+    api = FakeLidarr(LIBRARY, fail=[("track", 2)])
+    store = store_at(tmp_path)
+
+    music_curator.index_library(music_curator.Lidarr("http://lidarr", "key", transport=api), store)
+
+    assert store.scalar("SELECT COUNT(*) FROM lidarr_artist") == 3
+    assert store.scalar("SELECT COUNT(*) FROM lidarr_track WHERE artist_id = 2") == 0
+    # The other two artists are indexed in full.
+    assert store.scalar("SELECT COUNT(*) FROM lidarr_track") == 3
+    assert store.get_state("index_skipped") == "1"
+
+
+def test_a_skipped_artist_is_recorded_so_the_report_can_disown_the_numbers(tmp_path):
+    """An incomplete index makes played music look cold. It has to be loud."""
+    api = FakeLidarr(LIBRARY, fail=[("trackfile", 1)])
+    store = store_at(tmp_path)
+
+    music_curator.index_library(music_curator.Lidarr("http://lidarr", "key", transport=api), store)
+    music_curator.match_library(store)
+
+    assert store.get_state("index_skipped") == "1"
+
+
+def test_a_clean_index_records_no_skips(tmp_path):
+    store = indexed(tmp_path, [])
+
+    assert store.get_state("index_skipped") == "0"
+
+
+def test_a_lidarr_error_carries_the_url_and_what_the_server_said():
+    """A bare status code sends you looking at the wrong thing entirely."""
+    api = FakeLidarr(LIBRARY, fail=[("album", 0)])
+    client = music_curator.Lidarr("http://lidarr:8686", "key", transport=api)
+
+    with pytest.raises(music_curator.LidarrError) as raised:
+        client.get("album")
+
+    message = str(raised.value)
+    assert "http://lidarr:8686/api/v1/album" in message
+    assert "HTTP 500" in message
+    assert "boom" in message
+
+
+def test_the_offending_album_is_named_not_just_its_artist(tmp_path, caplog):
+    """An artist's whole discography is too much to click through by hand."""
+    # AC/DC is artist 2; its only album is id 201, holding "Hells Bells".
+    api = FakeLidarr(LIBRARY, fail=[("album", 0), ("album", 2), ("albumid", 201)])
+    store = store_at(tmp_path)
+
+    with caplog.at_level("WARNING"):
+        music_curator.index_library(
+            music_curator.Lidarr("http://lidarr", "key", transport=api), store
+        )
+
+    assert "album id 201" in caplog.text
+    assert "Hells Bells" in caplog.text
+
+
+def test_a_transient_failure_is_not_probed_album_by_album(tmp_path, caplog):
+    """When the resolver is the problem every artist fails, and probing each of
+    them multiplies the load that caused it."""
+
+    def unresolvable(url, timeout=None, headers=None):
+        if "artistId" in url:
+            raise urllib.error.URLError("[Errno -3] Try again")
+        return json.dumps(FakeLidarr(LIBRARY).artists) if url.endswith("artist") else "[]"
+
+    store = store_at(tmp_path)
+    client = music_curator.Lidarr("http://lidarr", "key", transport=unresolvable, backoff=0)
+
+    with caplog.at_level("WARNING"):
+        music_curator.index_library(client, store)
+
+    assert "Try again" in caplog.text
+    assert "cannot serialise" not in caplog.text
+
+
+def test_a_transient_failure_is_retried():
+    attempts = []
+
+    def flaky(url, timeout=None, headers=None):
+        attempts.append(url)
+        if len(attempts) < 3:
+            raise urllib.error.URLError("[Errno -3] Try again")
+        return "[]"
+
+    client = music_curator.Lidarr("http://lidarr", "key", transport=flaky, backoff=0)
+
+    assert client.get("artist") == []
+    assert len(attempts) == 3
+
+
+def test_a_lidarr_500_is_not_retried():
+    """It is an exception inside Lidarr's serialisation, not a busy server."""
+    api = FakeLidarr(LIBRARY, fail=[("album", 0)])
+    client = music_curator.Lidarr("http://lidarr", "key", transport=api, backoff=0)
+
+    with pytest.raises(music_curator.LidarrError) as raised:
+        client.get("album")
+
+    assert raised.value.transient is False
+    assert len(api.calls) == 1
+
+
+def test_keep_alive_uses_one_connection_for_many_requests(http_server):
+    """The point of the whole class: one DNS lookup and one socket, not N."""
+    server, base = http_server
+    transport = music_curator.KeepAlive()
+
+    try:
+        for index in range(5):
+            body = transport(f"{base}/api/v1/artist?n={index}", headers={"X-Api-Key": "key"})
+            assert json.loads(body)["key"] == "key"
+    finally:
+        transport.close()
+
+    assert server.connections == 1
+
+
+def test_keep_alive_maps_an_error_status_onto_httperror(http_server):
+    _, base = http_server
+    transport = music_curator.KeepAlive()
+
+    try:
+        with pytest.raises(urllib.error.HTTPError) as raised:
+            transport(f"{base}/boom", headers={"X-Api-Key": "key"})
+        assert raised.value.code == 500
+        assert music_curator.error_detail(raised.value) == "boom"
+    finally:
+        transport.close()
+
+
+def test_lidarr_talks_to_a_real_server_through_keep_alive(http_server):
+    server, base = http_server
+    client = music_curator.Lidarr(base, "secret")
+
+    try:
+        assert client.get("artist", {"x": 1})["key"] == "secret"
+        assert client.get("album")["path"] == "/api/v1/album"
+    finally:
+        client.transport.close()
+
+    assert server.connections == 1
+
+
+# Titles taken verbatim from a real coverage report's unmatched list. Each one
+# was a genuine miss before the normaliser handled it.
+@pytest.mark.parametrize(
+    ("scrobbled", "tagged"),
+    [
+        ("Self vs Self (feat. In Flames)", "Self vs Self"),
+        ("Grime Battle of Hastings (feat. The Town Crier)", "Grime Battle of Hastings"),
+        ("Gold Dust - Shy FX Re-Edit", "Gold Dust"),
+        ("Back To Your Roots - Friction & K-Tee Remix", "Back To Your Roots"),
+        ("Constellations - Forza Horizon 3 VIP", "Constellations"),
+        ("Everyday (Netsky Remix)", "Everyday"),
+        ("Voodoo People [Pendulum Remix] [Live At Brixton Academy]", "Voodoo People"),
+    ],
+)
+def test_real_unmatched_titles_now_agree_with_their_tags(scrobbled, tagged):
+    assert music_curator.normalise(scrobbled) == music_curator.normalise(tagged)
+
+
+@pytest.mark.parametrize(
+    "title",
+    [
+        "Dancing with Myself",
+        "(Don't Fear) The Reaper",
+        "Live and Let Die",
+        "Radio Ga Ga",
+        "Editors",
+        "Mixed Emotions",
+        "Vipassana",
+    ],
+)
+def test_the_version_words_do_not_eat_ordinary_titles(title):
+    """Every one of these contains a version word and must survive intact."""
+    assert music_curator.normalise(title) == music_curator.normalise(title.lower())
+    assert len(music_curator.normalise(title).split()) == len(title.split())
+
+
+def test_a_bracketed_guest_credit_matches_the_bare_tag(tmp_path):
+    """Whitespace-then-feat misses the bracketed form, which is most of them."""
+    store = indexed(tmp_path, [scrobble_of("Yellowcard", "Here I Am Alive (feat. Someone)")])
+
+    method, track_id = verdict(store, "Yellowcard", "Here I Am Alive (feat. Someone)")
+    assert method == "name"
+    assert track_id is not None
+
+
+def test_misses_are_split_by_whether_the_library_holds_the_title(tmp_path):
+    """Owning an artist is a weak proxy for owning a track. Counting both as
+    matcher failures overstates the problem and would over-block the cull."""
+    store = indexed(
+        tmp_path,
+        [
+            # The library holds "Hells Bells", but under AC/DC, not Yellowcard:
+            # an attribution disagreement, and a real miss.
+            scrobble_of("Yellowcard", "Hells Bells"),
+            # Yellowcard is in the library; this track is not, under any artist.
+            scrobble_of("Yellowcard", "A Single She Never Bought"),
+        ],
+    )
+
+    def count(extra):
+        return store.connection.execute(
+            "SELECT COUNT(*) FROM scrobble_key k WHERE k.track_id IS NULL"
+            "   AND EXISTS (SELECT 1 FROM lidarr_artist a WHERE a.norm_name = k.norm_artist)"
+            f"  {extra}"
+        ).fetchone()[0]
+
+    assert count("") == 2
+    assert count("AND EXISTS (SELECT 1 FROM lidarr_track t WHERE t.norm_title = k.norm_track)") == 1
+
+
+def test_the_report_survives_the_attribution_split(tmp_path):
+    store = indexed(tmp_path, [scrobble_of("Yellowcard", "Hells Bells")])
+
+    music_curator.report(store, NOW)