From 13e583b3b107c220aac05a475026fa667dc49940 Mon Sep 17 00:00:00 2001 From: StateAntigen Date: Tue, 15 Sep 2026 22:16:03 -0700 Subject: [PATCH 01/42] docs(nip): add kind 30427 album cover proposal --- NIP-NAPSTR-COVER.md | 266 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 266 insertions(+) create mode 100644 NIP-NAPSTR-COVER.md diff --git a/NIP-NAPSTR-COVER.md b/NIP-NAPSTR-COVER.md new file mode 100644 index 0000000..90a21e0 --- /dev/null +++ b/NIP-NAPSTR-COVER.md @@ -0,0 +1,266 @@ +# Napstr cover event: kind `30427` + +> **Status:** draft proposal for upstream inclusion in +> [lnbits/napstr `PROTOCOL.md`](https://github.com/lnbits/napstr/blob/main/PROTOCOL.md). +> First implemented (consumer + NIP-07 curator publisher) by napstr.fm. +> Normative keywords follow the +> main document: **MUST**, **MUST NOT**, **SHOULD**, **MAY**. +> +> **Kind selection (surveyed 2026-09-10):** `30427` is the first free kind in +> the Napstr addressable block. `30424` and `30426` carry events from an +> unrelated game protocol (`story:*` markers, April–May 2026), and `30425` is +> `napstr-playlist`. Marker tags (`#t=napstr-cover`) would namespace either way, +> but a free kind avoids all cross-protocol event mixing. + +Album artwork is cosmetic metadata that every browsing client otherwise resolves +independently from rate-limited third-party APIs (MusicBrainz, Cover Art +Archive, iTunes Search). This event lets publishers share a resolution once so +every other client inherits it from relays alongside the catalogue. Cover events +are **additive assertions**: they never alter, suppress, or reclassify kind +`30421` catalogue entries, and a client that ignores this kind entirely remains +fully interoperable. + +Kind `30427` is a parameterized-replaceable (addressable) event. Its `d` tag is +a normalized album key, so the newest valid event from one author for that key +replaces the author's previous cover claim for that album. + +## Event shape + +Required tags: + +```json +[ + ["d", ""], + ["t", "napstr-cover"], + ["alt", "Napstr album cover assertion"] +] +``` + +Optional tags: + +```json +[ + ["x", ""], + ["thumb", ""], + ["m", "image/jpeg"], + ["client", "Napstr"] +] +``` + +- `d`: the **cover key** — see normalization rules below. +- `t`: the literal marker `napstr-cover`. Cover events carry no search-word + `t` tags; discovery is by marker + `d` filter only. +- `x`: when present, the lowercase SHA-256 of a **cover image file** that the + author also publishes as an ordinary kind `30421` catalogue entry (see + "Embedded covers" below). +- `thumb`: an HTTPS URL to a smaller rendition of the same image. Clients MAY + prefer it in dense grids and fall back to `art`. +- `m`: the MIME type of the `x` image (`image/jpeg`, `image/png`, or + `image/webp`). SHOULD be present whenever `x` is present. +- `alt`: human-readable event description, e.g. `Album cover for by + `. + +## Cover key normalization + +The `d` value is the album's display metadata joined by a single pipe: + +```text +coverKey = trim(artist) + "|" + trim(album), all lowercased +``` + +- Lowercasing uses Unicode-aware case folding (`toLowerCase()`). +- No other normalization is applied: whitespace runs, diacritics, punctuation, + and edition markers (`(Deluxe Edition)`, `[2023 Remaster]`, `- Disc 2`) are + preserved verbatim. Publishers MUST NOT strip or rewrite the embedded + metadata; the key exists to match catalogue display strings byte-for-byte, + not to canonicalize releases. +- The artist half is the track `artist` field as published in kind `30421` + content; the album half is the track `album` field. Both are required — an + event whose key is missing either side of the pipe is invalid. +- Total key length MUST NOT exceed 300 characters. + +## Canonical alias + +Real-world catalogues contain mis-tagged files: watermarks (`! www.example.tk !`), +filenames-as-albums, and edition markers (`(Deluxe Edition)`). A cover published +against a clean key would then miss those entries, and vice versa. + +The canonical alias recomputes the key from **cleaned** display metadata: + +```text +cleanArtist = strip leading URL/watermark tokens and bracket characters +cleanAlbum = strip "(Deluxe)", "[Remaster]", "- Disc N", etc. +canonicalKey = norm(cleanArtist) + "|" + norm(cleanAlbum) +``` + +Publishers SHOULD include `c=` when it differs from `d`. +Consumers SHOULD query both their verbatim cover key and their locally computed +canonical key in the same `#d` filter, and MUST treat a cover event arriving via +the canonical alias exactly as if it had matched the verbatim key. + +## Content + +The content is JSON with camel-case property names, at most 4 KiB: + +```json +{ + "protocol": "napstr/1", + "art": "https://is1-ssl.mzstatic.com/image/thumb/Music/.../600x600bb.jpg", + "mbid": "f4a7b0d2-...", + "year": "2007", + "genre": "Rock", + "collection": "City of Echoes", + "source": "itunes" +} +``` + +- `protocol`: the literal `napstr/1`. +- `art`: HTTPS URL of the front cover image. REQUIRED when the event carries + no `x` tag; OPTIONAL (as a hotlink mirror) when `x` is present. Only HTTPS + URLs are accepted; clients MUST reject `http:` and non-URL values. +- `mbid`: MusicBrainz **release-group** MBID the art was resolved from, when + known. Lets consumers deep-link or re-resolve at other sizes without a new + search. +- `year`: four-digit first-release year, when known. +- `genre`: single primary genre label, when known. +- `collection`: canonical release title (e.g. the MusicBrainz release-group + title), when the resolved release differs from the catalogue `album` string. +- `source`: provenance hint — `itunes`, `musicbrainz`, `embedded`, `manual`, + or another short lowercase token. Informational only; MUST NOT be used for + trust decisions (anyone can claim any source). + +Unknown properties MUST be ignored. + +## Linked covers + +The common case: the author resolved art from a third-party API and publishes +only the URL. Consumers fetch the image lazily (`` or +equivalent), SHOULD cache it locally, and MUST tolerate unreachable or +replaced URLs by falling back to their own external lookup or placeholder. + +Consumers SHOULD bound per-host image concurrency and MUST NOT treat a broken +`art` URL as a reason to penalize the author — URLs rot. + +## Embedded covers + +Publishers whose local audio files contain embedded artwork MAY share the +image itself through Napstr: + +1. Extract the front-cover image as its own file. +2. Compute its `fileId` (lowercase SHA-256 of the complete bytes) exactly as + for audio files. +3. Publish it as a normal kind `30421` catalogue entry so existing + availability heartbeats, search, and the Tor transfer protocol serve it + unchanged. The entry's `format`/`mime` describe the image (`JPG`, `PNG`, + `WEBP`). Clients that only accept audio formats simply ignore the entry. +4. Include the image's `fileId` in the cover event's `x` tag, with `m` set to + the image MIME type. + +A consumer that wants the embedded cover requests `` through the +normal NIP-17 download negotiation and transfer protocol from any active +seeder of that file, verifies the SHA-256, and validates the bytes as a +JPEG/PNG/WebP image **from their contents** (magic bytes), never from the +extension or MIME claim alone. `art`, when also present, is the fast path; `x` +is the self-healing path that survives third-party link rot. + +Image catalogue entries SHOULD stay small; publishers SHOULD downscale covers +to at most 1200×1200 before sharing. + +## Trust and conflict resolution + +Cover events are unsigned-attribution, signed-origin assertions: the signature +proves authorship, not accuracy. Any identity may publish a cover for any +album key. Consumers apply this order: + +1. An event whose author is a **currently active seeder of at least one track + of that album** (present in an unexpired kind `30422` heartbeat covering a + `30421` entry whose normalized `artist|album` equals the cover key) wins + over events from non-seeders. +2. Within the same trust class, newest `created_at` wins. +3. Events from locally blocked authors are ignored (see "Reports and local + blocking" in the main document). Wrong or abusive covers SHOULD be reported + with NIP-56 kind `1984` using an `e` tag on the cover event and an `x` tag + carrying the cover key. + +A consumer that holds a valid seeder-authored event MAY ignore later +non-seeder events for the same key until the seeder's event is replaced by its +author. + +## Consumption + +Typical flow for a catalogue browser: + +1. Subscribe once for the marker: + + ```json + {"kinds":[30427],"#t":["napstr-cover"]} + ``` + +2. As kind `30421` catalogue entries arrive, compute each album's cover key + **and canonical alias**, then batch-fetch misses by `d` filter, mirroring + the catalogue's bounded paging: + + ```json + {"kinds":[30427],"#t":["napstr-cover"],"#d":["","","",""],"limit":500} + ``` + + The reference consumer chunks `d` filters at 75 values and at most 4 + concurrent requests. + +3. Merge the winning event's fields into the local cover cache keyed by cover + key. A cover event satisfies the key: consumers SHOULD NOT additionally + query third-party APIs for art of an album a cover event already provides. + +4. External lookup (MusicBrainz, iTunes, etc.) remains the fallback for albums + with no cover event, and stays client-optional. + +## Withdrawal + +An author retracts a cover claim by replacing the same coordinate with the +catalogue withdrawal body: + +```json +{ + "kind": 30427, + "tags": [ + ["d", ""], + ["t", "napstr-cover"] + ], + "content": "{\"protocol\":\"napstr/1\",\"deleted\":true}" +} +``` + +Consumers MUST treat the latest event at that coordinate from that author as +withdrawn and MUST NOT offer the author's older claim. + +## Validation requirements + +An interoperable consumer MUST: + +- validate the event ID and signature per NIP-01; +- require `protocol` = `napstr/1`, the `napstr-cover` marker tag, and a cover + key containing exactly one `|` with non-empty, length-bounded halves; +- require `art` to be an HTTPS URL or absent in favour of `x`; +- require `x`, when present, to be a 64-character lowercase hex SHA-256; +- treat `year`, `genre`, `collection`, and `source` as untrusted display text; +- verify embedded-cover bytes by SHA-256 and image magic bytes after transfer. + +A publisher MUST NOT include local filesystem paths, private keys, or +transfer capabilities anywhere in the event. + +A publisher SHOULD refuse to publish covers for tracks whose embedded tags are +obviously junk: URL or watermark prefixes, bare filenames as album names, or +leading track numbers. A bad cover on the network is worse than no cover. + +## Rationale notes (non-normative) + +- **Why a new kind instead of an `image` tag on `30421`:** a cover belongs to + an *album*, not a file; duplicating the URL across every track event bloats + the catalogue, and addressability per album key gives clean replace/delete + semantics. +- **Why publishers and not just curators:** seeders are the only parties with + provable access to the actual audio (and its embedded art), which is also + the strongest trust signal available without a web-of-trust layer. +- **Why keep the messy edition markers in the key:** matching is against + catalogue display strings; canonicalization belongs in the optional `mbid` / + `collection` fields, where getting it wrong costs nothing. From 557b6e42ed5e51aaa2c516a918fc992c6b985624 Mon Sep 17 00:00:00 2001 From: StateAntigen Date: Tue, 15 Sep 2026 23:02:55 -0700 Subject: [PATCH 02/42] feat(cover): consume kind 30427 album cover events Adds the desktop consumer for NIP-NAPSTR-COVER. Cover events are additive assertions addressed by a normalized artist|album key, so every path degrades to 'no cover known' rather than failing a catalogue read. - cover.rs: key normalization, canonical alias, event validation, withdrawal tombstones, and the NIP trust order (active seeder of the album wins, then newest created_at, blocked authors dropped). - network.rs: NetworkService::album_covers resolves cache misses with 75-value #d batches and four concurrent relay queries, stores claims with a created_at guard so a stale replay cannot roll a coordinate back, and only records a miss for batches that actually succeeded. - Seeder authorship is resolved from remote_catalogue joined to the live 30422 availability heartbeats, keyed by the new cover_key and canonical_cover_key columns (backfilled once at migration). - network_covers Tauri command exposes the winners. --- src-tauri/src/cover.rs | 1427 ++++++++++++++++++++++++++++++++++++++ src-tauri/src/lib.rs | 10 + src-tauri/src/network.rs | 215 +++++- 3 files changed, 1646 insertions(+), 6 deletions(-) create mode 100644 src-tauri/src/cover.rs diff --git a/src-tauri/src/cover.rs b/src-tauri/src/cover.rs new file mode 100644 index 0000000..52efe3a --- /dev/null +++ b/src-tauri/src/cover.rs @@ -0,0 +1,1427 @@ +//! Consumer for the Napstr album cover event (kind `30427`). +//! +//! The wire format is specified by `NIP-NAPSTR-COVER.md`. Cover events are +//! additive assertions about an album's artwork, addressed by a normalized +//! `artist|album` key, so every path in this module degrades to "no cover +//! known" instead of failing a catalogue read. Covers never alter, suppress, or +//! reclassify kind `30421` entries. +//! +//! Two trust rules from the NIP shape the ranking here: +//! +//! 1. A claim whose author is a currently active seeder of at least one track +//! of that album outranks claims from everybody else. +//! 2. Inside the same trust class the newest `created_at` wins. +//! +//! Events from blocked authors are dropped before ranking, and a withdrawn +//! coordinate (`{"deleted":true}`) is stored as a tombstone so an older claim +//! from the same author can never be offered. + +use chrono::Utc; +use nostr_sdk::prelude::*; +use rusqlite::{params, Connection}; +use serde::{Deserialize, Serialize}; +use std::collections::{HashMap, HashSet}; + +pub const COVER_KIND: u16 = 30427; +pub const COVER_MARKER: &str = "napstr-cover"; +/// `content` is JSON with camel-case keys, at most 4 KiB. +pub const COVER_CONTENT_BYTE_LIMIT: usize = 4 * 1024; +/// A `d` value is `artist|album` and must not exceed 300 characters. +pub const COVER_KEY_CHARACTER_LIMIT: usize = 300; +/// Untrusted display text (`year`, `genre`, `collection`, `source`) is bounded. +const COVER_TEXT_CHARACTER_LIMIT: usize = 120; +const COVER_ART_URL_CHARACTER_LIMIT: usize = 2_048; +const COVER_MIME_CHARACTER_LIMIT: usize = 64; +const COVER_KEY_SEPARATOR: char = '|'; + +/// The raw JSON body of a kind `30427` event. Unknown properties are ignored, +/// as the NIP requires. +#[derive(Debug, Clone, Default, Deserialize)] +#[serde(rename_all = "camelCase")] +struct CoverContent { + protocol: String, + #[serde(default)] + art: String, + #[serde(default)] + mbid: String, + #[serde(default)] + year: String, + #[serde(default)] + genre: String, + #[serde(default)] + collection: String, + #[serde(default)] + source: String, + #[serde(default)] + deleted: bool, +} + +/// A winning cover for one album key. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct AlbumCover { + /// The verbatim cover key this cover answers, even when it was matched + /// through the canonical alias. + pub key: String, + /// HTTPS URL of the front cover. Empty when the publisher only shared an + /// embedded copy through the `x` tag. + pub art: String, + /// HTTPS URL of a smaller rendition of the same image, when published. + pub thumb: String, + /// MusicBrainz release-group MBID the art was resolved from, when known. + pub mbid: String, + pub year: String, + pub genre: String, + /// Canonical release title, when it differs from the catalogue `album`. + pub collection: String, + /// Provenance hint. Informational only; never a trust signal. + pub source: String, + /// Lowercase SHA-256 of an embedded cover image published as a kind `30421` + /// entry, when the publisher shared the bytes instead of only a URL. + pub cover_file_id: String, + pub mime: String, + /// Author of the winning claim. + pub author: String, + pub event_id: String, + pub created_at: u64, + /// True when the author is (or was) an active seeder of a track of this + /// album, which makes the claim win over non-seeder claims. + pub seeder: bool, +} + +/// The outcome of reading one kind `30427` event. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum CoverClaim { + /// A usable cover assertion. + Art(Box), + /// The coordinate was replaced with the withdrawal body. Consumers must not + /// offer this author's older claim. + Withdrawn { + key: String, + author: String, + event_id: String, + created_at: u64, + }, +} + +// --------------------------------------------------------------------------- +// Cover keys +// --------------------------------------------------------------------------- + +/// `trim(artist) + "|" + trim(album)`, Unicode-case-folded. +/// +/// The key exists to match catalogue display strings byte-for-byte, so no other +/// normalization happens: whitespace runs, diacritics, punctuation, and edition +/// markers are preserved. Returns `None` when either half is empty, when the +/// halves would not round-trip through a single separator, or when the result +/// exceeds the NIP's length bound. +pub fn cover_key(artist: &str, album: &str) -> Option { + normalise_cover_key(&format!( + "{artist}{COVER_KEY_SEPARATOR}{album}" + )) +} + +/// Re-normalize a key that came from a caller or from an event tag. +/// +/// A key is valid when it holds exactly one separator with non-empty, +/// length-bounded halves. +pub fn normalise_cover_key(value: &str) -> Option { + let mut halves = value.split(COVER_KEY_SEPARATOR); + let artist = halves.next()?.trim().to_lowercase(); + let album = halves.next()?.trim().to_lowercase(); + if halves.next().is_some() || artist.is_empty() || album.is_empty() { + return None; + } + let key = format!("{artist}{COVER_KEY_SEPARATOR}{album}"); + (key.chars().count() <= COVER_KEY_CHARACTER_LIMIT).then_some(key) +} + +/// `cover_key` recomputed from cleaned display metadata. +/// +/// Mis-tagged catalogues are common: watermarks, filenames used as album names, +/// and edition markers. Publishers share the cleaned key as `c=` +/// so those entries can still match. This is a heuristic on the NIP's "etc."; +/// it only ever widens lookup, it is never used to publish a `d` value. +pub fn canonical_cover_key(artist: &str, album: &str) -> Option { + cover_key(&clean_artist(artist), &clean_album(album)) +} + +/// Both `#d` selectors that can answer `key`: the verbatim key first, then the +/// canonical alias when it differs. +pub fn cover_lookup_keys(key: &str) -> Vec { + let Some(key) = normalise_cover_key(key) else { + return Vec::new(); + }; + let mut keys = vec![key.clone()]; + if let Some((artist, album)) = key_halves(&key) { + if let Some(canonical) = canonical_cover_key(artist, album) { + if canonical != key { + keys.push(canonical); + } + } + } + keys +} + +fn key_halves(key: &str) -> Option<(&str, &str)> { + let mut halves = key.split(COVER_KEY_SEPARATOR); + let artist = halves.next()?; + let album = halves.next()?; + (halves.next().is_none() && !artist.is_empty() && !album.is_empty()).then_some((artist, album)) +} + +/// Leading URL or watermark tokens, such as `www.example.tk` or +/// `https://example.tk`, that file tags often carry, plus the punctuation that +/// usually surrounds them. +fn strip_leading_noise_token(value: &str) -> Option<&str> { + let trimmed = value.trim_start(); + let token_end = trimmed.find(char::is_whitespace).unwrap_or(trimmed.len()); + let token = &trimmed[..token_end]; + if token.is_empty() { + return None; + } + // `! www.example.tk ! Real Artist` must clean to `Real Artist`, so loose + // punctuation is dropped alongside the watermark it wraps. + let punctuation_only = !token.chars().any(char::is_alphanumeric); + if !punctuation_only && !looks_like_domain(token) { + return None; + } + Some(&trimmed[token_end..]) +} + +fn looks_like_domain(token: &str) -> bool { + let bare = token.trim_matches(|character: char| { + !character.is_ascii_alphanumeric() && character != '.' && character != '-' + }); + if bare.contains("://") || bare.to_ascii_lowercase().starts_with("www.") { + return true; + } + let labels = bare.split('.').collect::>(); + labels.len() >= 2 + && labels.iter().all(|label| { + !label.is_empty() + && label + .chars() + .all(|character| character.is_ascii_alphanumeric() || character == '-') + }) + && labels + .last() + .is_some_and(|top| top.len() >= 2 && top.chars().all(|c| c.is_ascii_alphabetic())) +} + +fn clean_artist(value: &str) -> String { + let mut cleaned = value.trim().to_string(); + while let Some(rest) = strip_leading_noise_token(&cleaned) { + cleaned = rest.to_string(); + } + cleaned + .chars() + .filter(|character| !is_bracket_character(*character)) + .collect::() + .trim_matches(|character: char| character == '!' || character.is_whitespace()) + .to_string() +} + +/// Edition and packaging markers that carry no release identity. A group is +/// dropped only when one of these words appears inside it, so meaningful +/// parentheticals such as `(Live at Pompeii)` survive. +const ALBUM_NOISE_WORDS: &[&str] = &[ + "anniversary", + "bonus", + "cd", + "collector", + "collectors", + "deluxe", + "digital", + "disc", + "disk", + "edition", + "enhanced", + "explicit", + "expanded", + "hires", + "hi-res", + "limited", + "mono", + "promo", + "reissue", + "release", + "remaster", + "remastered", + "repack", + "special", + "stereo", + "version", + "vinyl", + "volume", +]; + +fn clean_album(value: &str) -> String { + let mut cleaned = value.trim().to_string(); + loop { + let trimmed = cleaned.trim_end(); + let Some(closing) = trimmed.chars().last().filter(|c| *c == ')' || *c == ']') else { + break; + }; + let opening = if closing == ')' { '(' } else { '[' }; + let Some(start) = trimmed.rfind(opening) else { + break; + }; + if !is_noise_group(&trimmed[start + 1..trimmed.len() - 1]) { + break; + } + cleaned = trimmed[..start].to_string(); + } + loop { + let trimmed = cleaned.trim_end(); + let Some(segment_start) = trimmed.rfind(" - ") else { + break; + }; + if !is_noise_group(&trimmed[segment_start + 3..]) { + break; + } + cleaned = trimmed[..segment_start].to_string(); + } + cleaned + .trim_end_matches(|character: char| { + character.is_whitespace() || character == '-' || character == '_' || character == ',' + }) + .trim() + .to_string() +} + +fn is_noise_group(value: &str) -> bool { + let lowered = value.trim().to_lowercase(); + if lowered.is_empty() { + return false; + } + lowered + .split(|character: char| !character.is_alphanumeric() && character != '-') + .filter(|word| !word.is_empty()) + .any(|word| { + // Only exact words match, so a meaningful parenthetical such as + // `(Epic Records)` is never mistaken for a packaging marker. + ALBUM_NOISE_WORDS.contains(&word) + || word + .strip_suffix('s') + .is_some_and(|singular| ALBUM_NOISE_WORDS.contains(&singular)) + }) +} + +fn is_bracket_character(character: char) -> bool { + matches!( + character, + '(' | ')' | '[' | ']' | '{' | '}' | '<' | '>' + ) +} + +// --------------------------------------------------------------------------- +// Event validation +// --------------------------------------------------------------------------- + +/// Validate a kind `30427` event and turn it into a claim filed under `key`. +/// +/// `key` is the verbatim key the caller wants answered. A claim published under +/// the canonical alias is stored and served under the verbatim key exactly as +/// if it had matched it, and a claim for any other album is refused. +pub fn cover_claim(event: &Event, key: &str) -> Option { + let key = normalise_cover_key(key)?; + if !valid_cover_key(key.as_str()) { + return None; + } + // The event's own coordinate must already be normalized and must be one of + // the selectors that can answer `key`. + let identifier = event.tags.identifier()?; + if normalise_cover_key(identifier).as_deref() != Some(identifier) + || !cover_lookup_keys(&key) + .iter() + .any(|selector| selector == identifier) + { + return None; + } + let content = serde_json::from_str::(&event.content).ok()?; + if !valid_cover_event(event, &content) { + return None; + } + let author = event.pubkey.to_hex(); + let event_id = event.id.to_hex(); + let created_at = event.created_at.as_secs(); + if content.deleted { + return Some(CoverClaim::Withdrawn { + key, + author, + event_id, + created_at, + }); + } + let cover_file_id = embedded_file_id(event).unwrap_or_default(); + Some(CoverClaim::Art(Box::new(AlbumCover { + key, + art: content.art.trim().to_string(), + thumb: tag_content(event, "thumb").unwrap_or_default(), + mbid: claim_text(&content.mbid), + year: claim_text(&content.year), + genre: claim_text(&content.genre), + collection: claim_text(&content.collection), + source: claim_text(&content.source), + cover_file_id, + mime: cover_mime(event), + author, + event_id, + created_at, + seeder: false, + }))) +} + +/// The event's own coordinate must be a valid cover key, so a claim can never +/// be filed under a coordinate the author did not sign for. +pub fn valid_cover_key(key: &str) -> bool { + key_halves(key).is_some() && key.chars().count() <= COVER_KEY_CHARACTER_LIMIT +} + +fn valid_cover_event(event: &Event, content: &CoverContent) -> bool { + if event.kind != Kind::from(COVER_KIND) + || event.verify().is_err() + || event.content.len() > COVER_CONTENT_BYTE_LIMIT + || content.protocol != "napstr/1" + || !event.tags.hashtags().any(|tag| tag == COVER_MARKER) + || !event + .tags + .identifier() + .is_some_and(|identifier| normalise_cover_key(identifier).is_some()) + { + return false; + } + // The withdrawal body carries neither an image nor a reference to one, so + // the art requirement applies only to real claims. + if content.deleted { + return true; + } + // A cover must resolve to image bytes: either a linked HTTPS URL or an + // embedded copy published as a catalogue entry. + match embedded_file_id(event) { + Some(_) => true, + None if content.art.is_empty() => false, + None => valid_cover_art_url(&content.art), + } +} + +/// Only HTTPS URLs are accepted; `http:` and non-URL values are rejected. +fn valid_cover_art_url(value: &str) -> bool { + let value = value.trim(); + if value.is_empty() + || value.chars().count() > COVER_ART_URL_CHARACTER_LIMIT + || value.chars().any(char::is_control) + { + return false; + } + Url::parse(value).is_ok_and(|url| url.scheme() == "https" && url.host_str().is_some()) +} + +fn embedded_file_id(event: &Event) -> Option { + let value = tag_content(event, "x")?; + let value = value.trim(); + // The NIP requires a 64-character lowercase hex SHA-256, so an uppercase + // claim is not accepted even though the bytes would be identical. + if value.len() != 64 + || !value + .chars() + .all(|character| character.is_ascii_digit() || ('a'..='f').contains(&character)) + { + return None; + } + Some(value.to_string()) +} + +/// Read the first value of a tag by its raw name, so optional tags such as +/// `thumb` and `m` do not need a dedicated `TagKind` constructor. +fn tag_content(event: &Event, name: &str) -> Option { + event + .tags + .iter() + .find(|tag| tag.kind() == TagKind::from(name)) + .and_then(|tag| tag.content()) + .map(|value| value.to_string()) +} + +/// Untrusted display text is bounded and stripped of control characters rather +/// than rejected, because it is informational only. +fn claim_text(value: &str) -> String { + value + .chars() + .filter(|character| !character.is_control()) + .take(COVER_TEXT_CHARACTER_LIMIT) + .collect::() + .trim() + .to_string() +} + +fn cover_mime(event: &Event) -> String { + tag_content(event, "m") + .map(|value| claim_text(&value)) + .filter(|value| { + value.chars().count() <= COVER_MIME_CHARACTER_LIMIT + && value + .to_ascii_lowercase() + .starts_with("image/") + }) + .map(|value| value.to_ascii_lowercase()) + .unwrap_or_default() +} + +// --------------------------------------------------------------------------- +// Ranking +// --------------------------------------------------------------------------- + +/// Pick the winning cover for one key. +/// +/// A claim from an active seeder of the album always outranks a claim from +/// anybody else; inside a trust class the newest `created_at` wins, with the +/// event id as a deterministic tie-break. +pub fn winning_cover(claims: &[AlbumCover]) -> Option<&AlbumCover> { + claims.iter().max_by(|left, right| { + (left.seeder, left.created_at, left.event_id.as_str()).cmp(&( + right.seeder, + right.created_at, + right.event_id.as_str(), + )) + }) +} + +/// Filter a requested key list down to valid, deduplicated keys. +pub fn normalised_request(keys: &[String], limit: usize) -> Vec { + let mut seen = HashSet::new(); + let mut normalised = Vec::new(); + for key in keys { + let Some(key) = normalise_cover_key(key) else { + continue; + }; + if seen.insert(key.clone()) { + normalised.push(key); + } + if normalised.len() >= limit { + break; + } + } + normalised +} + +// --------------------------------------------------------------------------- +// Storage +// --------------------------------------------------------------------------- + +const COVER_COLUMNS: &str = "cover_key,source_pubkey,art,thumb,mbid,year,genre,collection,source,cover_file_id,mime,event_id,created_at,deleted,seeder"; + +pub(crate) const COVER_MISS_LIFETIME_SECONDS: i64 = 30 * 60; +pub(crate) const COVER_HIT_LIFETIME_SECONDS: i64 = 6 * 60 * 60; + +/// Add the cover columns, indexes, and the album cover tables to an existing +/// database, then backfill catalogue keys once, when the column is new. +pub(crate) fn initialise_cover_schema(connection: &Connection) -> Result<(), String> { + let mut backfill = false; + for column in ["cover_key", "canonical_cover_key"] { + if !catalogue_column_exists(connection, column)? { + backfill = true; + } + super::ensure_column( + connection, + "remote_catalogue", + column, + "TEXT NOT NULL DEFAULT ''", + )?; + } + connection + .execute_batch( + "CREATE INDEX IF NOT EXISTS remote_catalogue_cover_key + ON remote_catalogue(cover_key); + CREATE INDEX IF NOT EXISTS remote_catalogue_canonical_cover_key + ON remote_catalogue(canonical_cover_key);", + ) + .map_err(|error| error.to_string())?; + if backfill { + backfill_catalogue_cover_keys(connection)?; + } + Ok(()) +} + +fn catalogue_column_exists(connection: &Connection, column: &str) -> Result { + let mut statement = connection + .prepare("PRAGMA table_info(remote_catalogue)") + .map_err(|error| error.to_string())?; + let columns = statement + .query_map([], |row| row.get::<_, String>(1)) + .map_err(|error| error.to_string())? + .collect::, _>>() + .map_err(|error| error.to_string())?; + Ok(columns.iter().any(|existing| existing == column)) +} + +fn backfill_catalogue_cover_keys(connection: &Connection) -> Result<(), String> { + const BATCH: usize = 1_000; + let mut last_rowid = 0i64; + loop { + let rows = { + let mut statement = connection + .prepare("SELECT rowid,artist,album FROM remote_catalogue WHERE rowid>?1 ORDER BY rowid LIMIT ?2") + .map_err(|error| error.to_string())?; + let rows = statement + .query_map(params![last_rowid, BATCH as i64], |row| { + Ok(( + row.get::<_, i64>(0)?, + row.get::<_, String>(1)?, + row.get::<_, String>(2)?, + )) + }) + .map_err(|error| error.to_string())? + .collect::, _>>() + .map_err(|error| error.to_string())?; + rows + }; + if rows.is_empty() { + return Ok(()); + } + let batch_len = rows.len(); + for (rowid, artist, album) in rows { + last_rowid = rowid; + connection + .execute( + "UPDATE remote_catalogue SET cover_key=?2,canonical_cover_key=?3 WHERE rowid=?1", + params![ + rowid, + cover_key(&artist, &album).unwrap_or_default(), + canonical_cover_key(&artist, &album).unwrap_or_default() + ], + ) + .map_err(|error| error.to_string())?; + } + if batch_len < BATCH { + return Ok(()); + } + } +} + +/// Keys that need a relay query: never checked, or checked longer ago than the +/// hit or miss lifetime allows. +pub(crate) fn stale_cover_keys( + connection: &Connection, + keys: &[String], +) -> Result, String> { + let now = Utc::now().timestamp(); + let mut stale = Vec::new(); + let mut statement = connection + .prepare("SELECT checked_at,hit FROM album_cover_queries WHERE cover_key=?1") + .map_err(|error| error.to_string())?; + for key in keys { + let checked: Option<(String, i64)> = statement + .query_row(params![key], |row| Ok((row.get(0)?, row.get(1)?))) + .ok(); + let fresh = checked.is_some_and(|(checked_at, hit)| { + let lifetime = if hit == 1 { + COVER_HIT_LIFETIME_SECONDS + } else { + COVER_MISS_LIFETIME_SECONDS + }; + let Ok(checked_at) = chrono::DateTime::parse_from_rfc3339(&checked_at) else { + return false; + }; + now - checked_at.timestamp() < lifetime + }); + if !fresh { + stale.push(key.clone()); + } + } + Ok(stale) +} + +pub(crate) fn mark_cover_keys_checked( + connection: &Connection, + keys: &[String], + hits: &HashSet, +) -> Result<(), String> { + let now = Utc::now().to_rfc3339(); + let mut statement = connection + .prepare( + "INSERT INTO album_cover_queries(cover_key,checked_at,hit) VALUES(?1,?2,?3) + ON CONFLICT(cover_key) DO UPDATE SET checked_at=excluded.checked_at,hit=excluded.hit", + ) + .map_err(|error| error.to_string())?; + for key in keys { + statement + .execute(params![key, now, i64::from(hits.contains(key))]) + .map_err(|error| error.to_string())?; + } + Ok(()) +} + +/// Store the newest claim from every author for the given key/event pairs. +/// +/// Withdrawals are stored as tombstones so this author's older claim is never +/// offered again, and a later re-claim replaces the tombstone by `created_at`. +pub(crate) fn store_cover_events( + connection: &Connection, + pairs: &[(String, Event)], +) -> Result<(), String> { + let now = Utc::now().to_rfc3339(); + for (key, event) in pairs { + let Some(claim) = cover_claim(event, key) else { + continue; + }; + match claim { + CoverClaim::Art(cover) => { + connection + .execute( + "INSERT INTO album_covers(cover_key,source_pubkey,art,thumb,mbid,year,genre,collection,source,cover_file_id,mime,event_id,created_at,deleted,seeder,seen_at) + VALUES(?1,?2,?3,?4,?5,?6,?7,?8,?9,?10,?11,?12,?13,0,0,?14) + ON CONFLICT(cover_key,source_pubkey) DO UPDATE SET + art=excluded.art,thumb=excluded.thumb,mbid=excluded.mbid,year=excluded.year, + genre=excluded.genre,collection=excluded.collection,source=excluded.source, + cover_file_id=excluded.cover_file_id,mime=excluded.mime,event_id=excluded.event_id, + created_at=excluded.created_at,deleted=0,seen_at=excluded.seen_at + WHERE excluded.created_at >= album_covers.created_at", + params![ + cover.key, + cover.author, + cover.art, + cover.thumb, + cover.mbid, + cover.year, + cover.genre, + cover.collection, + cover.source, + cover.cover_file_id, + cover.mime, + cover.event_id, + cover.created_at as i64, + now + ], + ) + .map_err(|error| error.to_string())?; + } + CoverClaim::Withdrawn { + key, + author, + event_id, + created_at, + } => { + connection + .execute( + "INSERT INTO album_covers(cover_key,source_pubkey,art,thumb,mbid,year,genre,collection,source,cover_file_id,mime,event_id,created_at,deleted,seeder,seen_at) + VALUES(?1,?2,'','','','','','','','','',?3,?4,1,0,?5) + ON CONFLICT(cover_key,source_pubkey) DO UPDATE SET + art='',thumb='',mbid='',year='',genre='',collection='',source='', + cover_file_id='',mime='',event_id=excluded.event_id, + created_at=excluded.created_at,deleted=1,seen_at=excluded.seen_at + WHERE excluded.created_at >= album_covers.created_at", + params![key, author, event_id, created_at as i64, now], + ) + .map_err(|error| error.to_string())?; + } + } + } + Ok(()) +} + +/// Winning, non-withdrawn cover for every requested key, honouring the local +/// block list. +pub(crate) fn load_cover_claims( + connection: &Connection, + keys: &[String], +) -> Result, String> { + if keys.is_empty() { + return Ok(Vec::new()); + } + let blocked = blocked_pubkeys(connection)?; + let mut statement = connection + .prepare(&format!( + "SELECT {} FROM album_covers WHERE cover_key=?1 AND deleted=0", + COVER_COLUMNS + )) + .map_err(|error| error.to_string())?; + let mut covers = Vec::new(); + for key in keys { + let claims = statement + .query_map(params![key], |row| { + Ok(AlbumCover { + key: row.get(0)?, + author: row.get(1)?, + art: row.get(2)?, + thumb: row.get(3)?, + mbid: row.get(4)?, + year: row.get(5)?, + genre: row.get(6)?, + collection: row.get(7)?, + source: row.get(8)?, + cover_file_id: row.get(9)?, + mime: row.get(10)?, + event_id: row.get(11)?, + created_at: row.get::<_, i64>(12)? as u64, + seeder: row.get::<_, i64>(14)? == 1, + }) + }) + .map_err(|error| error.to_string())? + .collect::, _>>() + .map_err(|error| error.to_string())?; + let claims = claims + .into_iter() + .filter(|claim| !blocked.contains(&claim.author)) + .collect::>(); + if let Some(winner) = winning_cover(&claims) { + covers.push(winner.clone()); + } + } + covers.sort_by(|left, right| left.key.cmp(&right.key)); + Ok(covers) +} + +/// Catalogue rows whose verbatim or canonical key matches, as +/// `(file_id, source_pubkey)` pairs. Availability heartbeats then decide which +/// of these publishers is an active seeder of the album. +pub(crate) fn album_seeder_candidates( + connection: &Connection, + keys: &[String], +) -> Result>, String> { + let mut candidates: HashMap> = HashMap::new(); + if keys.is_empty() { + return Ok(candidates); + } + let mut statement = connection + .prepare("SELECT file_id,source_pubkey FROM remote_catalogue WHERE cover_key=?1 OR canonical_cover_key=?1 LIMIT 500") + .map_err(|error| error.to_string())?; + for key in keys { + let rows = statement + .query_map(params![key], |row| { + Ok((row.get::<_, String>(0)?, row.get::<_, String>(1)?)) + }) + .map_err(|error| error.to_string())? + .collect::, _>>() + .map_err(|error| error.to_string())?; + if !rows.is_empty() { + candidates.insert(key.clone(), rows); + } + } + Ok(candidates) +} + +/// Remember a claim as seeder-authored so an offline read still prefers it. +pub(crate) fn mark_cover_seeder( + connection: &Connection, + key: &str, + author: &str, +) -> Result<(), String> { + connection + .execute( + "UPDATE album_covers SET seeder=1 WHERE cover_key=?1 AND source_pubkey=?2", + params![key, author], + ) + .map_err(|error| error.to_string())?; + Ok(()) +} + +fn blocked_pubkeys(connection: &Connection) -> Result, String> { + let mut statement = connection + .prepare("SELECT pubkey FROM blocked_pubkeys") + .map_err(|error| error.to_string())?; + let rows = statement + .query_map([], |row| row.get::<_, String>(0)) + .map_err(|error| error.to_string())?; + rows.collect::, _>>() + .map_err(|error| error.to_string()) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn signed_event(keys: &Keys, key: &str, content: serde_json::Value) -> Event { + EventBuilder::new(Kind::from(COVER_KIND), content.to_string()) + .tags(vec![ + Tag::identifier(key), + Tag::hashtag(COVER_MARKER), + Tag::parse(["alt", "Napstr album cover assertion"]).unwrap(), + ]) + .sign_with_keys(keys) + .unwrap() + } + + fn cover_content() -> serde_json::Value { + serde_json::json!({ + "protocol": "napstr/1", + "art": "https://is1-ssl.mzstatic.com/image/thumb/Music/cover.jpg", + "mbid": "f4a7b0d2-0000-0000-0000-000000000000", + "year": "2007", + "genre": "Rock", + "collection": "City of Echoes", + "source": "itunes", + }) + } + + #[test] + fn cover_keys_fold_case_and_reject_ambiguous_halves() { + assert_eq!( + cover_key(" Pink Floyd ", "Animals").as_deref(), + Some("pink floyd|animals") + ); + assert_eq!(cover_key("BEYONCÉ", "Lemonade").as_deref(), Some("beyoncé|lemonade")); + // Edition markers and whitespace runs are preserved verbatim. + assert_eq!( + cover_key("Artist", "Album (Deluxe Edition)").as_deref(), + Some("artist|album (deluxe edition)") + ); + assert_eq!(cover_key("", "Album"), None); + assert_eq!(cover_key("Artist", " "), None); + assert_eq!(cover_key("A|B", "Album"), None); + assert_eq!( + cover_key(&"a".repeat(299), "album"), + None, + "a key longer than 300 characters is not addressable" + ); + } + + #[test] + fn canonical_alias_cleans_watermarks_and_edition_markers() { + assert_eq!( + canonical_cover_key("! www.example.tk ! Real Artist", "Album (Deluxe Edition)") + .as_deref(), + Some("real artist|album") + ); + assert_eq!( + canonical_cover_key("Real Artist", "Album").as_deref(), + Some("real artist|album") + ); + // A leading bracket marker is packaging too, but the separator keeps the + // album half intact here; only the trailing `- Disc 2` is dropped. + assert_eq!( + canonical_cover_key("Artist", "[2023 Remaster] Album - Disc 2").as_deref(), + Some("artist|[2023 remaster] album") + ); + assert_eq!( + canonical_cover_key("Artist", "Live Album - Disc 2").as_deref(), + Some("artist|live album") + ); + // A meaningful parenthetical is kept. + assert_eq!( + canonical_cover_key("Artist", "Album (Live at Pompeii)").as_deref(), + Some("artist|album (live at pompeii)") + ); + } + + #[test] + fn lookup_keys_include_both_the_verbatim_key_and_the_alias() { + let keys = cover_lookup_keys("artist|album (deluxe edition)"); + assert_eq!( + keys, + vec!["artist|album (deluxe edition)".to_string(), "artist|album".to_string()] + ); + assert_eq!(cover_lookup_keys("not-a-key"), Vec::::new()); + assert_eq!( + normalised_request( + &[" Artist | Album ".to_string(), "artist|album".to_string(), "junk".to_string()], + 10 + ), + vec!["artist|album".to_string()] + ); + } + + #[test] + fn cover_claims_validate_the_marker_kind_and_shape() { + let keys = Keys::generate(); + let key = "artist|album"; + let event = signed_event(&keys, key, cover_content()); + assert!(matches!( + cover_claim(&event, key), + Some(CoverClaim::Art(_)) + )); + + let wrong_kind = EventBuilder::new(Kind::from(30421), "x") + .tags(vec![Tag::identifier(key), Tag::hashtag(COVER_MARKER)]) + .sign_with_keys(&keys) + .unwrap(); + assert!(cover_claim(&wrong_kind, key).is_none()); + + let missing_marker = EventBuilder::new(Kind::from(COVER_KIND), cover_content().to_string()) + .tag(Tag::identifier(key)) + .sign_with_keys(&keys) + .unwrap(); + assert!(cover_claim(&missing_marker, key).is_none()); + + let mut wrong_protocol = cover_content(); + wrong_protocol["protocol"] = serde_json::json!("napstr/2"); + assert!(cover_claim(&signed_event(&keys, key, wrong_protocol), key).is_none()); + + // The coordinate the author signed for must itself be a valid key. + for broken in ["artist", "artist|", "|album", "a|b|c"] { + let broken_event = signed_event(&keys, broken, cover_content()); + assert!( + cover_claim(&broken_event, broken).is_none(), + "{broken} is not an addressable cover key" + ); + } + + let mut padded = cover_content(); + padded["source"] = serde_json::json!("x".repeat(COVER_CONTENT_BYTE_LIMIT + 1)); + assert!(cover_claim(&signed_event(&keys, key, padded), key).is_none()); + } + + #[test] + fn covers_require_https_art_or_an_embedded_copy() { + let keys = Keys::generate(); + let key = "artist|album"; + + let mut insecure = cover_content(); + insecure["art"] = serde_json::json!("http://example.com/cover.jpg"); + assert!(cover_claim(&signed_event(&keys, key, insecure), key).is_none()); + + let mut relative = cover_content(); + relative["art"] = serde_json::json!("cover.jpg"); + assert!(cover_claim(&signed_event(&keys, key, relative), key).is_none()); + + let mut missing = cover_content(); + missing["art"] = serde_json::json!(""); + assert!(cover_claim(&signed_event(&keys, key, missing), key).is_none()); + + // ...but an embedded image published as a catalogue entry is enough. + let hash = "ab".repeat(32); + let embedded = EventBuilder::new( + Kind::from(COVER_KIND), + serde_json::json!({"protocol": "napstr/1", "source": "embedded"}).to_string(), + ) + .tags(vec![ + Tag::identifier(key), + Tag::hashtag(COVER_MARKER), + Tag::parse(["x", hash.as_str()]).unwrap(), + Tag::parse(["m", "image/jpeg"]).unwrap(), + ]) + .sign_with_keys(&keys) + .unwrap(); + let Some(CoverClaim::Art(cover)) = cover_claim(&embedded, key) else { + panic!("an embedded cover is a valid claim"); + }; + assert_eq!(cover.cover_file_id, hash); + assert_eq!(cover.art, ""); + assert_eq!(cover_mime(&embedded), "image/jpeg"); + + // The MIME hint is informational, so a non-image value is dropped + // rather than trusted, and a missing `m` leaves it empty. + let mistyped = EventBuilder::new( + Kind::from(COVER_KIND), + serde_json::json!({"protocol": "napstr/1"}).to_string(), + ) + .tags(vec![ + Tag::identifier(key), + Tag::hashtag(COVER_MARKER), + Tag::parse(["x", hash.as_str()]).unwrap(), + Tag::parse(["m", "text/html"]).unwrap(), + ]) + .sign_with_keys(&keys) + .unwrap(); + assert_eq!(cover_mime(&mistyped), ""); + let untyped = EventBuilder::new( + Kind::from(COVER_KIND), + serde_json::json!({"protocol": "napstr/1"}).to_string(), + ) + .tags(vec![ + Tag::identifier(key), + Tag::hashtag(COVER_MARKER), + Tag::parse(["x", hash.as_str()]).unwrap(), + ]) + .sign_with_keys(&keys) + .unwrap(); + assert_eq!(cover_mime(&untyped), ""); + + // A malformed or uppercase hash is not a usable embedded reference. + for bad_hash in ["ab".repeat(31), "AB".repeat(32), "zz".repeat(32)] { + let broken = EventBuilder::new( + Kind::from(COVER_KIND), + serde_json::json!({"protocol": "napstr/1"}).to_string(), + ) + .tags(vec![ + Tag::identifier(key), + Tag::hashtag(COVER_MARKER), + Tag::parse(["x", bad_hash.as_str()]).unwrap(), + ]) + .sign_with_keys(&keys) + .unwrap(); + assert!(cover_claim(&broken, key).is_none()); + } + } + + #[test] + fn untrusted_display_text_is_bounded_and_carries_no_controls() { + let keys = Keys::generate(); + let key = "artist|album"; + let mut content = cover_content(); + content["genre"] = serde_json::json!("Rock\u{202e}\u{0000}"); + content["collection"] = serde_json::json!("x".repeat(500)); + let Some(CoverClaim::Art(cover)) = cover_claim(&signed_event(&keys, key, content), key) + else { + panic!("display text must not invalidate a claim"); + }; + assert_eq!(cover.genre, "Rock\u{202e}"); + assert_eq!(cover.collection.chars().count(), COVER_TEXT_CHARACTER_LIMIT); + } + + #[test] + fn a_canonical_alias_claim_satisfies_the_verbatim_key() { + let keys = Keys::generate(); + let canonical = "artist|album"; + let verbatim = "artist|album (deluxe edition)"; + let event = signed_event(&keys, canonical, cover_content()); + assert!(cover_claim(&event, canonical).is_some()); + let Some(CoverClaim::Art(cover)) = cover_claim(&event, verbatim) else { + panic!("a canonical alias match must satisfy the verbatim key"); + }; + assert_eq!(cover.key, verbatim); + + // A claim for another album is never filed under this key. + let unrelated = signed_event(&keys, "artist|other", cover_content()); + assert!(cover_claim(&unrelated, verbatim).is_none()); + } + + #[test] + fn withdrawals_withdraw_the_authors_whole_coordinate() { + let keys = Keys::generate(); + let key = "artist|album"; + let withdrawal = EventBuilder::new( + Kind::from(COVER_KIND), + serde_json::json!({"protocol": "napstr/1", "deleted": true}).to_string(), + ) + .tags(vec![Tag::identifier(key), Tag::hashtag(COVER_MARKER)]) + .sign_with_keys(&keys) + .unwrap(); + let Some(CoverClaim::Withdrawn { + key: withdrawn_key, + author, + .. + }) = cover_claim(&withdrawal, key) + else { + panic!("a withdrawal body must be recognised"); + }; + assert_eq!(withdrawn_key, key); + assert_eq!(author, keys.public_key().to_hex()); + } + + #[test] + fn seeder_authored_covers_win_inside_a_key() { + let older_seeder = AlbumCover { + seeder: true, + created_at: 10, + event_id: "aa".into(), + ..seedless_cover("artist|album", 30, "bb") + }; + let newer_other = AlbumCover { + seeder: false, + created_at: 20, + event_id: "cc".into(), + ..seedless_cover("artist|album", 20, "cc") + }; + let newest_other = AlbumCover { + seeder: false, + created_at: 30, + event_id: "dd".into(), + ..seedless_cover("artist|album", 30, "dd") + }; + let claims = vec![newest_other.clone(), older_seeder.clone(), newer_other]; + assert_eq!( + winning_cover(&claims).map(|cover| cover.event_id.as_str()), + Some("aa") + ); + + // Inside one trust class the newest claim wins, and ties break on the + // event id so repeated reads agree. + let claims = vec![ + seedless_cover("artist|album", 20, "cc"), + seedless_cover("artist|album", 20, "bb"), + ]; + assert_eq!( + winning_cover(&claims).map(|cover| cover.event_id.as_str()), + Some("cc") + ); + assert_eq!(winning_cover(&[]), None); + } + + fn seedless_cover(key: &str, created_at: u64, event_id: &str) -> AlbumCover { + AlbumCover { + key: key.to_string(), + art: format!("https://example.com/{event_id}.jpg"), + thumb: String::new(), + mbid: String::new(), + year: String::new(), + genre: String::new(), + collection: String::new(), + source: "itunes".into(), + cover_file_id: String::new(), + mime: String::new(), + author: "ab".repeat(32), + event_id: event_id.to_string(), + created_at, + seeder: false, + } + } + + /// Mirrors `initialise_database`: the main schema owns the block lists, the + /// network schema owns the catalogue and the cover tables. + fn cover_database() -> Connection { + let connection = Connection::open_in_memory().unwrap(); + connection + .execute_batch( + "CREATE TABLE blocked_pubkeys (pubkey TEXT PRIMARY KEY, reason TEXT NOT NULL, created_at TEXT NOT NULL);", + ) + .unwrap(); + crate::network::initialise_network_schema(&connection).unwrap(); + connection + } + + fn insert_cover_row( + connection: &Connection, + key: &str, + author: &str, + created_at: u64, + art: &str, + deleted: bool, + seeder: bool, + ) { + let deleted = if deleted { 1 } else { 0 }; + let seeder = if seeder { 1 } else { 0 }; + connection + .execute( + "INSERT INTO album_covers(cover_key,source_pubkey,art,thumb,mbid,year,genre,collection,source,cover_file_id,mime,event_id,created_at,deleted,seeder,seen_at) + VALUES(?1,?2,?3,'','','','','','itunes','','',?4,?5,?6,?7,?8)", + params![ + key, + author, + art, + format!("{:064x}", created_at), + created_at as i64, + deleted, + seeder, + Utc::now().to_rfc3339() + ], + ) + .unwrap(); + } + + #[test] + fn stored_covers_follow_the_nip_trust_order() { + let connection = cover_database(); + let seeder_author = "aa".repeat(32); + let other = "bb".repeat(32); + let key = "artist|album".to_string(); + insert_cover_row( + &connection, + &key, + &other, + 200, + "https://example.com/newest.jpg", + false, + false, + ); + insert_cover_row( + &connection, + &key, + &seeder_author, + 100, + "https://example.com/seeder.jpg", + false, + true, + ); + + let covers = load_cover_claims(&connection, &[key.clone()]).unwrap(); + assert_eq!(covers.len(), 1); + assert_eq!( + covers[0].art, "https://example.com/seeder.jpg", + "a seeder-authored claim outranks a newer claim from somebody else" + ); + assert!(covers[0].seeder); + + // Inside one trust class the newest claim wins instead. + connection + .execute("UPDATE album_covers SET seeder=0", []) + .unwrap(); + let covers = load_cover_claims(&connection, &[key]).unwrap(); + assert_eq!(covers[0].art, "https://example.com/newest.jpg"); + } + + #[test] + fn withdrawals_and_blocks_hide_stored_covers() { + let connection = cover_database(); + let withdrawn = "aa".repeat(32); + let other = "bb".repeat(32); + let key = "artist|album".to_string(); + insert_cover_row( + &connection, + &key, + &withdrawn, + 200, + "https://example.com/withdrawn.jpg", + true, + false, + ); + insert_cover_row( + &connection, + &key, + &other, + 100, + "https://example.com/kept.jpg", + false, + false, + ); + + let covers = load_cover_claims(&connection, &[key.clone()]).unwrap(); + assert_eq!( + covers[0].art, "https://example.com/kept.jpg", + "a withdrawn coordinate is never offered, so the next author wins" + ); + + // Blocking an author withdraws their claim from this device too. + connection + .execute( + "INSERT INTO blocked_pubkeys(pubkey,reason,created_at) VALUES(?1,'test',?2)", + params![other, Utc::now().to_rfc3339()], + ) + .unwrap(); + assert!(load_cover_claims(&connection, &[key]).unwrap().is_empty()); + } + + #[test] + fn cover_events_replace_only_their_own_newer_coordinates() { + let connection = cover_database(); + let keys = Keys::generate(); + let author = keys.public_key().to_hex(); + let key = "artist|album".to_string(); + let event = signed_event(&keys, &key, cover_content()); + let withdrawal = EventBuilder::new( + Kind::from(COVER_KIND), + serde_json::json!({"protocol": "napstr/1", "deleted": true}).to_string(), + ) + .tags(vec![Tag::identifier(&key), Tag::hashtag(COVER_MARKER)]) + .sign_with_keys(&keys) + .unwrap(); + + // A claim that is newer than the event a relay just handed us survives, + // so a stale replay can never roll a coordinate backwards. + let far_future = 4_102_444_800; // year 2100, still a positive i64 + insert_cover_row( + &connection, + &key, + &author, + far_future, + "https://example.com/stored.jpg", + false, + false, + ); + store_cover_events(&connection, &[(key.clone(), event.clone())]).unwrap(); + let covers = load_cover_claims(&connection, &[key.clone()]).unwrap(); + assert_eq!(covers[0].art, "https://example.com/stored.jpg"); + + // The same holds for an older withdrawal body. + store_cover_events(&connection, &[(key.clone(), withdrawal.clone())]).unwrap(); + assert_eq!( + load_cover_claims(&connection, &[key.clone()]).unwrap().len(), + 1, + "an older withdrawal cannot retract a newer claim" + ); + + // Once the withdrawal is the newest event at the coordinate it wins, + // and a later claim replaces the tombstone. + connection + .execute( + "UPDATE album_covers SET created_at=1 WHERE cover_key=?1", + params![key], + ) + .unwrap(); + store_cover_events(&connection, &[(key.clone(), withdrawal)]).unwrap(); + assert!(load_cover_claims(&connection, &[key.clone()]) + .unwrap() + .is_empty()); + + store_cover_events(&connection, &[(key.clone(), event)]).unwrap(); + let covers = load_cover_claims(&connection, &[key]).unwrap(); + assert_eq!(covers[0].art, cover_content()["art"].as_str().unwrap()); + } + + #[test] + fn cover_queries_remember_misses_and_hits() { + let connection = cover_database(); + let key = "artist|album".to_string(); + assert_eq!( + stale_cover_keys(&connection, std::slice::from_ref(&key)).unwrap(), + vec![key.clone()], + "a key that was never queried is always stale" + ); + + let hits = HashSet::from([key.clone()]); + mark_cover_keys_checked(&connection, std::slice::from_ref(&key), &hits).unwrap(); + assert!(stale_cover_keys(&connection, std::slice::from_ref(&key)) + .unwrap() + .is_empty()); + + // A hit stays fresh for hours; a miss expires in half an hour. + let an_hour_ago = (Utc::now() - chrono::Duration::hours(1)).to_rfc3339(); + connection + .execute( + "UPDATE album_cover_queries SET checked_at=?1 WHERE cover_key=?2", + params![an_hour_ago, key], + ) + .unwrap(); + assert!(stale_cover_keys(&connection, std::slice::from_ref(&key)) + .unwrap() + .is_empty()); + connection + .execute( + "UPDATE album_cover_queries SET hit=0 WHERE cover_key=?1", + params![key], + ) + .unwrap(); + assert_eq!( + stale_cover_keys(&connection, std::slice::from_ref(&key)).unwrap(), + vec![key] + ); + } + + #[test] + fn catalogue_cover_keys_are_backfilled_for_existing_rows() { + let connection = Connection::open_in_memory().unwrap(); + // The shape of a database written before cover keys existed. + connection + .execute_batch( + "CREATE TABLE remote_catalogue ( + file_id TEXT NOT NULL, source_pubkey TEXT NOT NULL, filename TEXT NOT NULL, + title TEXT NOT NULL, artist TEXT NOT NULL, album TEXT NOT NULL, format TEXT NOT NULL, + mime TEXT NOT NULL, size INTEGER NOT NULL, license TEXT NOT NULL, event_id TEXT NOT NULL, + seen_at TEXT NOT NULL, PRIMARY KEY(file_id, source_pubkey) + ); + INSERT INTO remote_catalogue VALUES + ('aa','bb','song.mp3','Song',' Pink Floyd ','Animals (Deluxe Edition)','mp3','audio/mpeg',10,'unspecified','cc','now');", + ) + .unwrap(); + + initialise_cover_schema(&connection).unwrap(); + + let (key, canonical): (String, String) = connection + .query_row( + "SELECT cover_key,canonical_cover_key FROM remote_catalogue WHERE file_id='aa'", + [], + |row| Ok((row.get(0)?, row.get(1)?)), + ) + .unwrap(); + assert_eq!(key, "pink floyd|animals (deluxe edition)"); + assert_eq!(canonical, "pink floyd|animals"); + + // Re-running the migration is a no-op that must not rewrite the column. + connection + .execute( + "UPDATE remote_catalogue SET cover_key='sentinel' WHERE file_id='aa'", + [], + ) + .unwrap(); + initialise_cover_schema(&connection).unwrap(); + let key: String = connection + .query_row( + "SELECT cover_key FROM remote_catalogue WHERE file_id='aa'", + [], + |row| row.get(0), + ) + .unwrap(); + assert_eq!(key, "sentinel"); + } +} diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index e6dbbf6..c02b1f9 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -18,6 +18,7 @@ use tauri::{Emitter, Manager, State}; use walkdir::WalkDir; mod audio; +mod cover; mod mobile; mod network; mod player; @@ -1929,6 +1930,14 @@ async fn network_search_audiobooks( state.network.search_audiobooks(&query).await } +#[tauri::command] +async fn network_covers( + keys: Vec, + state: State<'_, AppState>, +) -> Result, String> { + state.network.album_covers(keys).await +} + #[tauri::command] async fn network_browse( cursor: Option, @@ -2235,6 +2244,7 @@ pub fn run() { publish_profile, network_search, network_search_audiobooks, + network_covers, network_browse, network_browse_user, resolve_catalogue_user, diff --git a/src-tauri/src/network.rs b/src-tauri/src/network.rs index b969662..967f86b 100644 --- a/src-tauri/src/network.rs +++ b/src-tauri/src/network.rs @@ -20,6 +20,9 @@ use tauri::Emitter; use tokio::sync::{Mutex, RwLock}; use uuid::Uuid; +use crate::cover; +pub use crate::cover::AlbumCover; + pub const CATALOGUE_KIND: u16 = 30421; pub const AVAILABILITY_KIND: u16 = 30422; pub const AUDIOBOOK_KIND: u16 = 30423; @@ -43,6 +46,13 @@ const EMPTY_SEARCH_RESULT_LIMIT: usize = 10_000; const EMPTY_SEARCH_PAGE_LIMIT: usize = 500; const CATALOGUE_IDENTIFIER_BATCH_SIZE: usize = 75; const CATALOGUE_IDENTIFIER_CONCURRENCY: usize = 8; +/// Album covers are cosmetic, so one call resolves a bounded number of albums +/// with the NIP's own batching (75 `#d` values, four concurrent queries). +const COVER_KEY_LIMIT: usize = 200; +const COVER_KEY_BATCH_SIZE: usize = 75; +const COVER_KEY_CONCURRENCY: usize = 4; +const COVER_QUERY_LIMIT: usize = 500; +const COVER_QUERY_TIMEOUT: Duration = Duration::from_secs(8); const CATALOGUE_BROWSE_SESSION_LIFETIME: Duration = Duration::from_secs(10 * 60); const CATALOGUE_BROWSE_SESSION_LIMIT: usize = 8; const NETWORK_SEARCH_RESULT_LIMIT: usize = 500; @@ -344,6 +354,136 @@ async fn fetch_catalogue_identifiers( Ok((events_by_id.into_values().collect(), retry_file_ids)) } +fn cover_lookup_filter(selectors: &[String]) -> Filter { + Filter::new() + .kind(Kind::from(cover::COVER_KIND)) + .hashtag(cover::COVER_MARKER) + .identifiers(selectors.iter().cloned()) + .limit(COVER_QUERY_LIMIT) +} + +/// Fetch cover claims for `keys`, querying each verbatim key together with its +/// canonical alias in the same `#d` filter, as the NIP requires. +/// +/// Returns the key/event pairs to store plus the keys whose query failed, so a +/// relay outage can never be remembered as "this album has no cover". +async fn fetch_cover_claims( + client: &Client, + keys: &[String], +) -> (Vec<(String, Event)>, HashSet) { + let mut selectors = Vec::new(); + let mut keys_by_selector: HashMap> = HashMap::new(); + for key in keys { + for selector in cover::cover_lookup_keys(key) { + keys_by_selector + .entry(selector.clone()) + .or_default() + .push(key.clone()); + selectors.push(selector); + } + } + selectors.sort_unstable(); + selectors.dedup(); + let batches = selectors + .chunks(COVER_KEY_BATCH_SIZE) + .map(|batch| batch.to_vec()) + .collect::>(); + let fetched = stream::iter(batches) + .map(|batch| { + let client = client.clone(); + async move { + let result = client + .fetch_events(cover_lookup_filter(&batch), COVER_QUERY_TIMEOUT) + .await; + (batch, result) + } + }) + .buffer_unordered(COVER_KEY_CONCURRENCY) + .collect::>() + .await; + let mut claims = Vec::new(); + let mut claimed = HashSet::new(); + let mut unresolved = HashSet::new(); + for (batch, result) in fetched { + match result { + Ok(events) => { + for event in events { + if event.kind != Kind::from(cover::COVER_KIND) { + continue; + } + // Only coordinates we asked for, matched exactly, are + // eligible; a relay may return anything. + let Some(answered) = event + .tags + .identifier() + .and_then(|identifier| keys_by_selector.get(identifier)) + else { + continue; + }; + for key in answered { + if claimed.insert((key.clone(), event.id)) { + claims.push((key.clone(), event.clone())); + } + } + } + } + Err(_) => { + for selector in batch { + if let Some(answered) = keys_by_selector.get(&selector) { + unresolved.extend(answered.iter().cloned()); + } + } + } + } + } + (claims, unresolved) +} + +/// Mark covers whose author is an active seeder of a track of that album. +/// +/// This is the only trust signal in the NIP that needs the catalogue and its +/// availability heartbeats rather than the cover event itself. Stored flags are +/// kept, so an offline read still prefers a claim that was proven +/// seeder-authored earlier. +fn apply_cover_seeders( + connection: &Connection, + mut covers: Vec, + availability: Option<&AvailabilitySnapshot>, +) -> Result, String> { + let Some(availability) = availability else { + return Ok(covers); + }; + let pending = covers + .iter() + .filter(|cover| !cover.seeder) + .map(|cover| cover.key.clone()) + .collect::>(); + if pending.is_empty() { + return Ok(covers); + } + let candidates = cover::album_seeder_candidates(connection, &pending)?; + for cover in &mut covers { + if cover.seeder { + continue; + } + let Some(rows) = candidates.get(&cover.key) else { + continue; + }; + let seeded = rows.iter().any(|(file_id, pubkey)| { + pubkey == &cover.author + && availability + .available_by_file + .get(file_id) + .is_some_and(|sources| sources.contains(pubkey)) + }); + if seeded { + cover.seeder = true; + cover::mark_cover_seeder(connection, &cover.key, &cover.author)?; + } + } + Ok(covers) +} + fn catalogue_search_tokens(fields: &[&str]) -> Vec { let mut seen = HashSet::new(); let field_tokens = fields @@ -1884,9 +2024,9 @@ impl NetworkService { ).map_err(|error| error.to_string())?; for chapter in &content.chapters { transaction.execute( - "INSERT OR REPLACE INTO remote_catalogue(file_id,source_pubkey,filename,title,artist,album,format,mime,size,license,description,tags,event_id,seen_at) - VALUES(?1,?2,?3,?4,?5,?6,?7,?8,?9,'unspecified','','audiobook',?10,?11)", - params![chapter.file_id, pubkey, chapter.filename, chapter.title, content.author, content.title, chapter.format, chapter.mime, chapter.size as i64, event.id.to_hex(), Utc::now().to_rfc3339()], + "INSERT OR REPLACE INTO remote_catalogue(file_id,source_pubkey,filename,title,artist,album,format,mime,size,license,description,tags,event_id,seen_at,cover_key,canonical_cover_key) + VALUES(?1,?2,?3,?4,?5,?6,?7,?8,?9,'unspecified','','audiobook',?10,?11,?12,?13)", + params![chapter.file_id, pubkey, chapter.filename, chapter.title, content.author, content.title, chapter.format, chapter.mime, chapter.size as i64, event.id.to_hex(), Utc::now().to_rfc3339(), super::cover::cover_key(&content.author, &content.title).unwrap_or_default(), super::cover::canonical_cover_key(&content.author, &content.title).unwrap_or_default()], ).map_err(|error| error.to_string())?; } aggregated @@ -2349,8 +2489,8 @@ impl NetworkService { }; let catalogue_name = content.filename.clone(); connection.execute( - "INSERT OR REPLACE INTO remote_catalogue (file_id,source_pubkey,filename,title,artist,album,format,mime,size,license,description,tags,event_id,seen_at) VALUES (?1,?2,?3,?4,?5,?6,?7,?8,?9,?10,?11,?12,?13,?14)", - params![content.file_id, pubkey, catalogue_name, content.title, content.artist, content.album, content.format, content.mime, content.size as i64, "unspecified", "", catalogue_tags, event.id.to_hex(), Utc::now().to_rfc3339()], + "INSERT OR REPLACE INTO remote_catalogue (file_id,source_pubkey,filename,title,artist,album,format,mime,size,license,description,tags,event_id,seen_at,cover_key,canonical_cover_key) VALUES (?1,?2,?3,?4,?5,?6,?7,?8,?9,?10,?11,?12,?13,?14,?15,?16)", + params![content.file_id, pubkey, catalogue_name, content.title, content.artist, content.album, content.format, content.mime, content.size as i64, "unspecified", "", catalogue_tags, event.id.to_hex(), Utc::now().to_rfc3339(), super::cover::cover_key(&content.artist, &content.album).unwrap_or_default(), super::cover::canonical_cover_key(&content.artist, &content.album).unwrap_or_default()], ).map_err(|error| error.to_string())?; cached_source_pairs.insert((pubkey, content.file_id.clone())); merge_catalogue_result(&mut aggregated, content, catalogue_tags, source); @@ -2478,6 +2618,56 @@ impl NetworkService { Ok((results, next_browse_cursor, total_available)) } + /// Winning album covers for `keys`, best effort. + /// + /// Covers are additive metadata, so this never fails a caller outright: if + /// a relay query is unavailable it answers with whatever was already + /// stored. A key is only refreshed when it is new, when a cached miss + /// expired, or when a cached hit aged out. + pub async fn album_covers(&self, keys: Vec) -> Result, String> { + let requested = cover::normalised_request(&keys, COVER_KEY_LIMIT); + if requested.is_empty() { + return Ok(Vec::new()); + } + let client = self.client.read().await.clone(); + // Every database block is scoped so no `Connection` is ever held across + // an await; `rusqlite::Connection` is not `Sync`. + let stale = { + let connection = super::open_connection(&self.db_path)?; + cover::stale_cover_keys(&connection, &requested)? + }; + let mut claims = Vec::new(); + let mut answered = Vec::new(); + if !stale.is_empty() { + if let Some(client) = client.as_ref() { + let (fetched, unresolved) = fetch_cover_claims(client, &stale).await; + answered = stale + .iter() + .filter(|key| !unresolved.contains(*key)) + .cloned() + .collect::>(); + claims = fetched; + } + } + // Availability heartbeats say whether a claim's author is an active + // seeder of the album, which is the NIP's strongest trust signal. + let availability = match client.as_ref() { + Some(client) => self.availability_snapshot(client).await.ok(), + None => None, + }; + let connection = super::open_connection(&self.db_path)?; + if !answered.is_empty() { + cover::store_cover_events(&connection, &claims)?; + let hits = claims + .iter() + .map(|(key, _)| key.clone()) + .collect::>(); + cover::mark_cover_keys_checked(&connection, &answered, &hits)?; + } + let covers = cover::load_cover_claims(&connection, &requested)?; + apply_cover_seeders(&connection, covers, availability.as_deref()) + } + pub async fn request_download( &self, file_id: String, @@ -3395,12 +3585,25 @@ pub fn initialise_network_schema(connection: &Connection) -> Result<(), String> event_id TEXT PRIMARY KEY, pubkey TEXT NOT NULL, event_json TEXT NOT NULL, created_at INTEGER NOT NULL ); CREATE INDEX IF NOT EXISTS trollbox_events_recent - ON trollbox_events(created_at DESC,event_id DESC);" + ON trollbox_events(created_at DESC,event_id DESC); + CREATE TABLE IF NOT EXISTS album_covers ( + cover_key TEXT NOT NULL, source_pubkey TEXT NOT NULL, art TEXT NOT NULL, thumb TEXT NOT NULL, + mbid TEXT NOT NULL, year TEXT NOT NULL, genre TEXT NOT NULL, collection TEXT NOT NULL, + source TEXT NOT NULL, cover_file_id TEXT NOT NULL, mime TEXT NOT NULL, + event_id TEXT NOT NULL, created_at INTEGER NOT NULL, deleted INTEGER NOT NULL DEFAULT 0, + seeder INTEGER NOT NULL DEFAULT 0, seen_at TEXT NOT NULL, + PRIMARY KEY(cover_key, source_pubkey) + ); + CREATE INDEX IF NOT EXISTS album_covers_key ON album_covers(cover_key); + CREATE TABLE IF NOT EXISTS album_cover_queries ( + cover_key TEXT PRIMARY KEY, checked_at TEXT NOT NULL, hit INTEGER NOT NULL DEFAULT 0 + );" ).map_err(|error| error.to_string()) .and_then(|_| super::ensure_column(connection, "remote_catalogue", "description", "TEXT NOT NULL DEFAULT ''")) .and_then(|_| super::ensure_column(connection, "remote_catalogue", "tags", "TEXT NOT NULL DEFAULT ''")) .and_then(|_| super::ensure_column(connection, "published_catalogue", "fingerprint", "TEXT NOT NULL DEFAULT ''")) .and_then(|_| super::ensure_column(connection, "network_downloads", "destination_folder", "TEXT NOT NULL DEFAULT ''")) + .and_then(|_| super::cover::initialise_cover_schema(connection)) } pub fn load_network_transfers(connection: &Connection) -> Result, String> { From ff5ccdc1d3e5c21f13ee7ff5d827a44f13818c8d Mon Sep 17 00:00:00 2001 From: StateAntigen Date: Tue, 15 Sep 2026 23:41:54 -0700 Subject: [PATCH 03/42] feat(napstrfy): fetch album art from the host over Iroh Napstrfy no longer resolves album art by itself. The paired host answers a new AlbumCovers request from its kind 30427 cover cache, which already applies the NIP's trust order, so the phone inherits seeder-authored claims and blocked-author filtering for free. - remote-protocol: RemoteAlbumCover plus the AlbumCovers request and response. MAX_COVER_KEYS bounds a full answer to one control frame, asserted by a test using maximum-length URLs. - host: serve the request through NetworkService::album_covers and allow it for read-only phones, which may show artwork but not download. - companion: remote_covers chunks the request and normalises keys with the same rule as the host, so both sides agree on what a key is. - artwork.ts: batched lookups replace the per-row MusicBrainz query. One call covers a whole page, the published thumbnail is preferred for tiles, and the external lookup stays as the fallback the NIP allows for albums no cover event answers. The old cache namespace is swept. --- android/src-tauri/src/lib.rs | 111 ++++++++++++++- android/src/lib/TrackArtwork.svelte | 11 +- android/src/lib/artwork.ts | 204 +++++++++++++++++++++++++--- remote-protocol/src/lib.rs | 106 ++++++++++++++- src-tauri/src/mobile.rs | 39 +++++- 5 files changed, 444 insertions(+), 27 deletions(-) diff --git a/android/src-tauri/src/lib.rs b/android/src-tauri/src/lib.rs index 272dd7d..2701739 100644 --- a/android/src-tauri/src/lib.rs +++ b/android/src-tauri/src/lib.rs @@ -1,8 +1,8 @@ use futures_util::StreamExt; use iroh::{endpoint::presets, Endpoint, EndpointAddr, EndpointId, SecretKey}; use napstr_remote_protocol::{ - ClientRequest, PairingTicket, RemoteAudiobook, RemoteAudiobookSummary, RemoteTrack, - RemoteTransfer, ServerResponse, ALPN, MAX_CONTROL_FRAME_BYTES, + ClientRequest, PairingTicket, RemoteAlbumCover, RemoteAudiobook, RemoteAudiobookSummary, + RemoteTrack, RemoteTransfer, ServerResponse, ALPN, MAX_CONTROL_FRAME_BYTES, MAX_COVER_KEYS, }; use quick_xml::{events::Event, Reader}; use serde::{Deserialize, Serialize}; @@ -1842,6 +1842,75 @@ async fn cached_library(state: State<'_, AppState>) -> Result, +) -> Result, String> { + let keys = normalise_cover_request(&keys); + let mut covers = Vec::new(); + for batch in keys.chunks(MAX_COVER_KEYS) { + let response = remote + .request(ClientRequest::AlbumCovers { + keys: batch.to_vec(), + }) + .await?; + match response { + ServerResponse::AlbumCovers { + covers: batch_covers, + } => covers.extend(batch_covers), + response => return Err(unexpected_response(&response)), + } + } + Ok(covers) +} + +#[tauri::command] +async fn remote_covers( + keys: Vec, + state: State<'_, AppState>, +) -> Result, String> { + companion_covers(&state.remote, keys).await +} + +/// Cover keys are `trim(artist)|trim(album)`, lowercased, exactly one +/// separator, at most 300 characters, exactly as the cover NIP defines them. +/// Rewriting here means the phone and the host always agree on the key, and a +/// sloppy caller cannot silently match nothing. +fn normalise_cover_request(keys: &[String]) -> Vec { + let mut seen = HashSet::new(); + let mut normalised = Vec::new(); + for key in keys { + let Some(key) = normalise_cover_key(key) else { + continue; + }; + if seen.insert(key.clone()) { + normalised.push(key); + } + if normalised.len() >= MAX_COVER_KEYS * 4 { + break; + } + } + normalised +} + +fn normalise_cover_key(value: &str) -> Option { + let mut halves = value.split('|'); + let artist = halves.next()?.trim().to_lowercase(); + let album = halves.next()?.trim().to_lowercase(); + if halves.next().is_some() || artist.is_empty() || album.is_empty() { + return None; + } + let key = format!("{artist}|{album}"); + (key.chars().count() <= 300).then_some(key) +} + #[tauri::command] async fn reconcile_audio_cache( protected_file_ids: Vec, @@ -2329,6 +2398,7 @@ pub fn run() { forget_desktop, remote_library, cached_library, + remote_covers, reconcile_audio_cache, remote_search, remote_audiobooks, @@ -2537,6 +2607,43 @@ mod tests { assert_eq!(clean_device_name("My\u{202e}Phone"), "MyPhone"); } + #[test] + fn cover_keys_match_the_cover_nip_normalisation() { + assert_eq!( + normalise_cover_key(" Pink Floyd |Animals ").as_deref(), + Some("pink floyd|animals") + ); + assert_eq!( + normalise_cover_key("BEYONCÉ|Lemonade").as_deref(), + Some("beyoncé|lemonade") + ); + // Edition markers are preserved: matching is against the catalogue's own + // display strings, and the host handles the canonical alias. + assert_eq!( + normalise_cover_key("Artist|Album (Deluxe Edition)").as_deref(), + Some("artist|album (deluxe edition)") + ); + // A key that is not addressable is dropped rather than sent on. + assert_eq!(normalise_cover_key("artist"), None); + assert_eq!(normalise_cover_key("artist|"), None); + assert_eq!(normalise_cover_key("|album"), None); + assert_eq!(normalise_cover_key("a|b|c"), None); + assert_eq!(normalise_cover_key(&format!("{}|album", "a".repeat(299))), None); + + let request = normalise_cover_request(&[ + " Artist | Album ".into(), + "artist|album".into(), + "junk".into(), + ]); + assert_eq!(request, vec!["artist|album".to_string()]); + + // A caller cannot buy an unbounded amount of relay work in one go. + let many = (0..MAX_COVER_KEYS * 5) + .map(|index| format!("artist|album{index}")) + .collect::>(); + assert_eq!(normalise_cover_request(&many).len(), MAX_COVER_KEYS * 4); + } + #[test] fn public_podcast_search_keeps_clean_unique_genres() { let payload = r#"{"results":[{"collectionId":42,"collectionName":"A Show","artistName":"A Host","feedUrl":"https://example.com/feed.xml","genres":["Technology","Podcasts","technology"],"primaryGenreName":"Science","trackCount":10}]}"#; diff --git a/android/src/lib/TrackArtwork.svelte b/android/src/lib/TrackArtwork.svelte index 86efb8e..ee446a8 100644 --- a/android/src/lib/TrackArtwork.svelte +++ b/android/src/lib/TrackArtwork.svelte @@ -1,5 +1,5 @@ diff --git a/android/src/lib/artwork.ts b/android/src/lib/artwork.ts index 8f823f9..109a8d8 100644 --- a/android/src/lib/artwork.ts +++ b/android/src/lib/artwork.ts @@ -1,54 +1,220 @@ +import { invoke } from '@tauri-apps/api/core'; import type { RemoteTrack } from './types'; -const CACHE_PREFIX = 'napstrfy-artwork:'; -let lookupQueue = Promise.resolve(); +/** + * Album artwork asserted by Napstr kind `30427` cover events. + * + * The paired host resolves these, because it owns the relay pool, the + * catalogue, and the availability heartbeats that decide which claim wins. + * Lookups are batched: a list page asks for every album it shows in one call + * instead of querying per row. + */ +export type AlbumCover = { + key: string; + /** Front cover URL. Empty when the publisher only shared an embedded copy. */ + art: string; + /** Smaller rendition of the same image, when the publisher supplied one. */ + thumb: string; + mbid: string; + year: string; + genre: string; + collection: string; + /** Provenance hint such as `itunes`, `embedded`, or `musicbrainz`. */ + source: string; + coverFileId: string; + mime: string; + author: string; + seeder: boolean; +}; + +const CACHE_PREFIX = 'napstrfy-cover:v2:'; +/** The pre-cover-event namespace, swept once so it cannot linger forever. */ +const LEGACY_CACHE_PREFIX = 'napstrfy-artwork:'; +/** Keys per companion call. Mirrors `MAX_COVER_KEYS * 4` in the phone crate. */ +const INVOKE_KEY_LIMIT = 160; +/** Albums nothing has artwork for are retried after this long. */ +const NEGATIVE_TTL_MS = 7 * 24 * 60 * 60 * 1000; +/** A page composes in stages: gather its keys briefly before asking. */ +const BATCH_DELAY_MS = 40; +/** MusicBrainz asks clients to stay near one request per second. */ +const LOOKUP_SPACING_MS = 1100; + +type CachedCover = { at: number; cover: AlbumCover | null }; +type Waiter = (cover: AlbumCover | null) => void; + +const pending = new Map(); +let flushHandle: number | null = null; +let lookupQueue: Promise = Promise.resolve(); let nextLookup = 0; -function readCache(key: string) { +function emptyCover(key: string): AlbumCover { + return { + key, + art: '', + thumb: '', + mbid: '', + year: '', + genre: '', + collection: '', + source: '', + coverFileId: '', + mime: '', + author: '', + seeder: false + }; +} + +function readCache(key: string): CachedCover | null { try { - return window.localStorage.getItem(CACHE_PREFIX + key); + const raw = window.localStorage.getItem(CACHE_PREFIX + key); + if (!raw) return null; + const cached = JSON.parse(raw) as CachedCover; + if (!cached.cover && Date.now() - cached.at > NEGATIVE_TTL_MS) return null; + return cached; } catch { return null; } } -function writeCache(key: string, value: string) { +function writeCache(key: string, cover: AlbumCover | null) { try { - window.localStorage.setItem(CACHE_PREFIX + key, value); + window.localStorage.setItem(CACHE_PREFIX + key, JSON.stringify({ at: Date.now(), cover })); } catch { // Artwork is cosmetic; a full or unavailable cache must not affect playback. } } +function dropLegacyCache() { + try { + const stale: string[] = []; + for (let index = 0; index < window.localStorage.length; index += 1) { + const name = window.localStorage.key(index); + if (name?.startsWith(LEGACY_CACHE_PREFIX)) stale.push(name); + } + for (const name of stale) window.localStorage.removeItem(name); + } catch { + // A cache that cannot be swept is only wasted space. + } +} + +dropLegacyCache(); + function pause(ms: number) { return new Promise((resolve) => window.setTimeout(resolve, ms)); } -export function artworkFor(track: RemoteTrack): Promise { - if (!track.artist || !track.album) return Promise.resolve(''); - const key = `${track.artist.toLocaleLowerCase()}\u0000${track.album.toLocaleLowerCase()}`; +/** + * The cover key from the cover NIP: `trim(artist)|trim(album)`, lowercased, + * preserved verbatim otherwise so it matches the catalogue display strings. + */ +export function coverKey(artist: string, album: string): string { + const artistHalf = artist.trim().toLowerCase(); + const albumHalf = album.trim().toLowerCase(); + if (!artistHalf || !albumHalf) return ''; + if (artistHalf.includes('|') || albumHalf.includes('|')) return ''; + const key = `${artistHalf}|${albumHalf}`; + return key.length <= 300 ? key : ''; +} + +export function coverFor(track: RemoteTrack): Promise { + const key = coverKey(track.artist ?? '', track.album ?? ''); + return key ? requestCover(key) : Promise.resolve(null); +} + +/** The URL for a small artwork tile, preferring the published thumbnail. */ +export async function artworkFor(track: RemoteTrack): Promise { + const cover = await coverFor(track); + if (!cover) return ''; + return cover.thumb || cover.art; +} + +/** Warm the cache for tracks about to be shown, so scrolling never stalls. */ +export function preloadCovers(tracks: RemoteTrack[]) { + for (const track of tracks) void coverFor(track); +} + +function requestCover(key: string): Promise { const cached = readCache(key); - if (cached !== null) return Promise.resolve(cached); + if (cached) return Promise.resolve(cached.cover); + return new Promise((resolve) => { + const waiters = pending.get(key); + if (waiters) waiters.push(resolve); + else pending.set(key, [resolve]); + if (flushHandle === null) flushHandle = window.setTimeout(flush, BATCH_DELAY_MS); + }); +} + +async function flush() { + flushHandle = null; + const batch = [...pending.keys()]; + const resolved = await resolveCovers(batch); + for (const key of batch) { + const waiters = pending.get(key) ?? []; + pending.delete(key); + const cover = resolved.get(key) ?? null; + writeCache(key, cover); + for (const resolve of waiters) resolve(cover); + } + // Keys requested while this batch was in flight wait for the next one. + if (pending.size) flushHandle = window.setTimeout(flush, BATCH_DELAY_MS); +} + +async function resolveCovers(keys: string[]): Promise> { + const resolved = new Map(); + const misses: string[] = []; + for (let index = 0; index < keys.length; index += INVOKE_KEY_LIMIT) { + const slice = keys.slice(index, index + INVOKE_KEY_LIMIT); + try { + const covers = await invoke('remote_covers', { keys: slice }); + for (const cover of covers) { + if (cover.art || cover.thumb || cover.coverFileId) resolved.set(cover.key, cover); + } + } catch { + // Unpaired, offline, or a host older than the cover NIP: the external + // lookup below is exactly the fallback that case is meant to use. + } + for (const key of slice) if (!resolved.has(key)) misses.push(key); + } + for (const key of misses) { + const url = await externalArtwork(key); + if (url) { + // A cover event satisfies its key, so only albums without one get here. + resolved.set(key, { ...emptyCover(key), art: url, thumb: url, source: 'musicbrainz' }); + } + } + return resolved; +} + +/** + * MusicBrainz release search followed by the Cover Art Archive, kept as the + * client-optional fallback the cover NIP allows for albums no cover event + * answers. Serialised and spaced out to respect the MusicBrainz rate limit. + */ +function externalArtwork(key: string): Promise { + const [artist, album] = key.split('|'); + if (!artist || !album) return Promise.resolve(''); const work = lookupQueue.then(async () => { const wait = Math.max(0, nextLookup - Date.now()); if (wait) await pause(wait); - nextLookup = Date.now() + 1100; + nextLookup = Date.now() + LOOKUP_SPACING_MS; try { - const query = `release:${JSON.stringify(track.album)} AND artist:${JSON.stringify(track.artist)}`; - const response = await fetch(`https://musicbrainz.org/ws/2/release/?fmt=json&limit=1&query=${encodeURIComponent(query)}`); + const query = `release:${JSON.stringify(album)} AND artist:${JSON.stringify(artist)}`; + const response = await fetch( + `https://musicbrainz.org/ws/2/release/?fmt=json&limit=1&query=${encodeURIComponent(query)}` + ); if (!response.ok) throw new Error('artwork lookup failed'); - const data = await response.json() as { releases?: { id?: string }[] }; + const data = (await response.json()) as { releases?: { id?: string }[] }; const candidate = data.releases?.[0]?.id ?? ''; const release = /^[0-9a-f]{8}-[0-9a-f-]{27}$/i.test(candidate) ? candidate : ''; - const url = release ? `https://coverartarchive.org/release/${release}/front-250` : ''; - writeCache(key, url); - return url; + return release ? `https://coverartarchive.org/release/${release}/front-250` : ''; } catch { - writeCache(key, ''); return ''; } }); - lookupQueue = work.then(() => undefined, () => undefined); + lookupQueue = work.then( + () => undefined, + () => undefined + ); return work; } diff --git a/remote-protocol/src/lib.rs b/remote-protocol/src/lib.rs index 2b500fe..b884e46 100644 --- a/remote-protocol/src/lib.rs +++ b/remote-protocol/src/lib.rs @@ -5,6 +5,9 @@ pub const ALPN: &[u8] = b"/napstr/mobile/1"; pub const PROTOCOL_VERSION: u16 = 1; pub const MAX_CONTROL_FRAME_BYTES: usize = 256 * 1024; pub const MAX_PAGE_SIZE: usize = 200; +/// Album covers per request. Bounded so a full answer always fits in one +/// control frame even when every URL is at its maximum length. +pub const MAX_COVER_KEYS: usize = 40; const PAIRING_URI_PREFIX: &str = "napstrfy://pair/"; const LEGACY_PAIRING_URI_PREFIX: &str = "nostrfy://pair/"; @@ -93,7 +96,35 @@ pub struct RemoteAudiobookSummary { pub total_size: u64, pub chapter_count: usize, } - +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +#[serde(rename_all = "camelCase")] +pub struct RemoteAlbumCover { + /// The verbatim `artist|album` key, lowercased and trimmed. This is what a + /// track's own key is computed into, so the phone can match without + /// understanding the canonical alias. + pub key: String, + /// HTTPS URL of the front cover. Empty when the publisher only shared an + /// embedded copy through an `x` tag. + pub art: String, + /// HTTPS URL of a smaller rendition of the same image, when published. + pub thumb: String, + pub mbid: String, + pub year: String, + pub genre: String, + pub collection: String, + /// Provenance hint such as `itunes` or `embedded`. Informational only. + pub source: String, + /// SHA-256 of the image as an ordinary catalogue entry, when the publisher + /// shared the bytes themselves. Fetch it through the normal transfer path + /// and verify the hash and the image magic bytes before display. + pub cover_file_id: String, + pub mime: String, + /// Author of the winning claim. + pub author: String, + /// True when the winning author is (or was) an active seeder of a track of + /// this album, which is the strongest trust signal in the cover NIP. + pub seeder: bool, +} #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] #[serde(rename_all = "camelCase")] pub struct RemoteTransfer { @@ -149,6 +180,12 @@ pub enum ClientRequest { Available { file_ids: Vec, }, + /// Album artwork asserted by kind `30427` events. The host resolves the + /// keys because it owns the relay pool, the catalogue, and the availability + /// heartbeats that decide which claim wins. + AlbumCovers { + keys: Vec, + }, Status, Ping, } @@ -194,6 +231,9 @@ pub enum ServerResponse { Available { file_ids: Vec, }, + AlbumCovers { + covers: Vec, + }, Status { library_revision: u64, #[serde(default)] @@ -304,4 +344,68 @@ mod tests { request ); } + + #[test] + fn album_covers_round_trip() { + let request = ClientRequest::AlbumCovers { + keys: vec!["artist|album".into()], + }; + assert_eq!( + serde_json::from_str::(&serde_json::to_string(&request).unwrap()) + .unwrap(), + request + ); + let response = ServerResponse::AlbumCovers { + covers: vec![RemoteAlbumCover { + key: "artist|album".into(), + art: "https://example.com/cover.jpg".into(), + thumb: String::new(), + mbid: String::new(), + year: "2007".into(), + genre: "Rock".into(), + collection: String::new(), + source: "itunes".into(), + cover_file_id: String::new(), + mime: String::new(), + author: "a".repeat(64), + seeder: true, + }], + }; + assert_eq!( + serde_json::from_str::(&serde_json::to_string(&response).unwrap()) + .unwrap(), + response + ); + } + + /// A host may answer with `MAX_COVER_KEYS` covers whose URL fields are all + /// at their documented maximum, and the answer still has to fit in one + /// control frame rather than being truncated mid-flight. + #[test] + fn a_full_cover_answer_fits_in_one_control_frame() { + let key = format!("{}|{}", "a".repeat(148), "b".repeat(150)); + let covers = (0..MAX_COVER_KEYS) + .map(|index| RemoteAlbumCover { + key: key.clone(), + // 2048 is the accepted maximum for `art` and `thumb`. + art: format!("https://example.com/{}.jpg", "x".repeat(2020)), + thumb: format!("https://example.com/{}.jpg", "y".repeat(2020)), + mbid: "0".repeat(36), + year: "2007".into(), + genre: "g".repeat(120), + collection: "c".repeat(120), + source: "s".repeat(32), + cover_file_id: format!("{index:064x}"), + mime: "image/jpeg".into(), + author: "a".repeat(64), + seeder: true, + }) + .collect::>(); + let payload = serde_json::to_vec(&ServerResponse::AlbumCovers { covers }).unwrap(); + assert!( + payload.len() <= MAX_CONTROL_FRAME_BYTES, + "a full cover answer is {} bytes, over the {MAX_CONTROL_FRAME_BYTES} byte frame limit", + payload.len() + ); + } } diff --git a/src-tauri/src/mobile.rs b/src-tauri/src/mobile.rs index 4e02e18..ed64df3 100644 --- a/src-tauri/src/mobile.rs +++ b/src-tauri/src/mobile.rs @@ -5,9 +5,9 @@ use crate::{ use chrono::Utc; use iroh::{endpoint::presets, Endpoint, SecretKey}; use napstr_remote_protocol::{ - ClientRequest, PairingTicket, RemoteAudiobook, RemoteAudiobookSummary, RemoteSource, - RemoteTrack, RemoteTransfer, ServerResponse, ALPN, MAX_CONTROL_FRAME_BYTES, MAX_PAGE_SIZE, - PROTOCOL_VERSION, + ClientRequest, PairingTicket, RemoteAlbumCover, RemoteAudiobook, RemoteAudiobookSummary, + RemoteSource, RemoteTrack, RemoteTransfer, ServerResponse, ALPN, MAX_CONTROL_FRAME_BYTES, + MAX_COVER_KEYS, MAX_PAGE_SIZE, PROTOCOL_VERSION, }; use qrcode::{render::svg, QrCode}; use rusqlite::{params, OptionalExtension}; @@ -710,6 +710,19 @@ impl MobileService { ) .await } + ClientRequest::AlbumCovers { keys } => { + if keys.len() > MAX_COVER_KEYS { + return Err("Too many album covers were requested at once".into()); + } + let covers = self + .network + .album_covers(keys) + .await? + .into_iter() + .map(remote_album_cover) + .collect(); + write_response(send, &ServerResponse::AlbumCovers { covers }).await + } ClientRequest::Status => { write_response( send, @@ -822,6 +835,7 @@ fn check_request_permission(stream_only: bool, request: &ClientRequest) -> Resul | ClientRequest::Audiobook { .. } | ClientRequest::FetchAudio { .. } | ClientRequest::Available { .. } + | ClientRequest::AlbumCovers { .. } | ClientRequest::Status | ClientRequest::Ping => Ok(()), _ => Err("This phone has read-only access. Downloads on the Napstr host are not permitted.".into()), @@ -832,6 +846,25 @@ fn is_sha256_file_id(value: &str) -> bool { value.len() == 64 && value.bytes().all(|byte| byte.is_ascii_hexdigit()) } +/// Trim the host's bookkeeping (event ids, timestamps) from a resolved cover +/// before it crosses the wire. +fn remote_album_cover(cover: crate::network::AlbumCover) -> RemoteAlbumCover { + RemoteAlbumCover { + key: cover.key, + art: cover.art, + thumb: cover.thumb, + mbid: cover.mbid, + year: cover.year, + genre: cover.genre, + collection: cover.collection, + source: cover.source, + cover_file_id: cover.cover_file_id, + mime: cover.mime, + author: cover.author, + seeder: cover.seeder, + } +} + fn remote_audiobook( book: crate::network::AudiobookResult, local_tracks: &std::collections::HashMap, From ba8e3e4c7953ff2172b87165fdb74faccd277bee Mon Sep 17 00:00:00 2001 From: StateAntigen Date: Wed, 16 Sep 2026 00:10:44 -0700 Subject: [PATCH 04/42] feat(napstrfy): expandable now-playing sheet Tapping the artwork or title in the now-playing bar slides a full-screen sheet up over it; dragging its header down (or the collapse button) dismisses it. - Album art fills the background as a blurred, darkened backdrop behind the cover, with the gradient tile still standing in when no cover event answers. - Title, artist, and album plus release year, all read from the cover event's untrusted metadata. - Transport controls, play mode, and seek, driving the same handlers as the compact bar so the two can never disagree. - Up next reflects the play mode rather than the raw queue: the wrapped remainder in 'play all', the current track in 'repeat', the single chosen index in 'random', and no wrap in 'play once'. Tapping a row jumps to that queue index and resets the random history. - Scoped to library playback: audiobook chapters share the player but are identified by the queue flag, and podcasts keep the compact bar. --- android/src/App.svelte | 181 ++++++++++++++++++++++++++++++++++++++++- android/src/app.css | 14 ++++ 2 files changed, 191 insertions(+), 4 deletions(-) diff --git a/android/src/App.svelte b/android/src/App.svelte index 7d68150..a37f7af 100644 --- a/android/src/App.svelte +++ b/android/src/App.svelte @@ -9,6 +9,7 @@ scan } from '@tauri-apps/plugin-barcode-scanner'; import TrackArtwork from './lib/TrackArtwork.svelte'; + import { artworkHue, coverFor, type AlbumCover } from './lib/artwork'; import type { AudiobookLibraryPage, CachedAudio, CompanionStatus, LibraryPage, PodcastDownload, PodcastEpisode, PodcastFeed, RemoteAudiobook, RemoteAudiobookSummary, RemoteTrack, RemoteTransfer } from './lib/types'; const musicChips = ['Rock', 'Soundtrack', 'Punk', 'Folk', 'Upbeat']; @@ -80,6 +81,14 @@ let podcastViewVersion = 0; let currentPodcast = $state(null); let activeMedia = $state<'music' | 'podcast'>('music'); + // The expandable now-playing sheet. Audiobook chapters run through the same + // player as music, so the library flag is what distinguishes "music only". + let showNowPlaying = $state(false); + let nowCover = $state(null); + let nowArtFailed = $state(false); + let sheetDragY = $state(0); + let sheetDragging = $state(false); + let sheetDragStart = 0; let audio: HTMLAudioElement; let lastSystemMediaSync = 0; @@ -661,6 +670,98 @@ if (audio) audio.volume = value; } + let upNext = $derived(upNextEntries()); + + // Track the sheet's cover alongside playback. The batched cache means this is + // free when the library list already resolved the same album. + $effect(() => { + const track = current; + nowArtFailed = false; + if (!track) { + nowCover = null; + return; + } + let alive = true; + void coverFor(track).then((cover) => { if (alive) nowCover = cover; }); + return () => { alive = false; }; + }); + + $effect(() => { + if (!current) showNowPlaying = false; + }); + + $effect(() => { + document.body.style.overflow = showNowPlaying ? 'hidden' : ''; + return () => { document.body.style.overflow = ''; }; + }); + + function nowPlayingAvailable() { + return activeMedia === 'music' && playerQueueLibraryVisible && !!current; + } + + function openNowPlaying() { + if (nowPlayingAvailable()) showNowPlaying = true; + } + + function closeNowPlaying() { + showNowPlaying = false; + sheetDragY = 0; + } + + function sheetCoverUrl() { + if (!nowCover || nowArtFailed) return ''; + return nowCover.art || nowCover.thumb; + } + + /** What actually plays next, honouring the current play mode. */ + function upNextEntries(): Array<{ index: number; track: RemoteTrack }> { + const entries: Array<{ index: number; track: RemoteTrack }> = []; + if (activeMedia !== 'music' || playerQueue.length < 2) return entries; + const push = (index: number) => { + const track = playerQueue[index]; + if (track) entries.push({ index, track }); + }; + if (playMode === 'random') { + push(randomUpcoming); + return entries; + } + if (playMode === 'repeat') { + push(playerIndex); + return entries; + } + for (let index = playerIndex + 1; index < playerQueue.length; index += 1) push(index); + if (playMode === 'all') { + for (let index = 0; index < playerIndex; index += 1) push(index); + } + return entries; + } + + async function playFromQueue(index: number) { + const track = playerQueue[index]; + if (!track) return; + playerIndex = index; + selected = track; + resetRandomOrder(); + await playTrack(track); + } + + function startSheetDrag(event: PointerEvent) { + sheetDragging = true; + sheetDragStart = event.clientY; + } + + function moveSheetDrag(event: PointerEvent) { + if (!sheetDragging) return; + sheetDragY = Math.max(0, event.clientY - sheetDragStart); + } + + function endSheetDrag() { + if (!sheetDragging) return; + sheetDragging = false; + if (sheetDragY > 90) closeNowPlaying(); + else sheetDragY = 0; + } + async function moveTrack(direction: -1 | 1) { if (activeMedia !== 'music' || playerQueue.length < 2) return; let next: number; @@ -1195,10 +1296,18 @@
- {#if activeMedia === 'podcast' && currentPodcast} - {#if currentPodcast.image}{:else}
◉
{/if} - {:else if current}{:else}
♪
{/if} -
{activeMedia === 'podcast' && currentPodcast ? currentPodcast.title : current ? title(current) : 'Choose something to play'}{activeMedia === 'podcast' && currentPodcast ? currentPodcast.feedTitle : current ? artist(current) : 'Music and podcasts, wherever you are'}
+
seek(Number(event.currentTarget.value))} disabled={!current && !currentPodcast} />{clock(currentTime)} / {clock(duration)}
@@ -1211,6 +1320,70 @@ {/if} +{#if showNowPlaying && current && activeMedia === 'music'} + +{/if} +
@@ -3779,7 +3815,6 @@ at once and the full cover fades in over it. --> { + // A phone, because a wide desktop window pins this sheet as a column and has + // no drawer to open. + await openApp(page, { library: [zzTop[0]], album: zzTop, platform: 'android' }); + await page.locator('.track-open').click(); + await page.locator('.now-open').click(); + const thumb = page.locator('.now-sheet-art img.now-sheet-art-thumb'); + const full = page.locator('.now-sheet-art img.now-sheet-art-full'); + // The tile that was tapped fetched the small rendition, so the drawer is a + // cover from its first frame; the full one is a fresh download behind it. + await expect(thumb).toHaveAttribute('src', coverThumb); + await expect(full).not.toHaveClass(/ready/); + await expect(full).toHaveClass(/ready/); + // The backdrop is blurred too far to show a bigger image, so it takes the + // small rendition rather than making a second request for the large one. + const backdrop = await page.locator('.now-sheet-backdrop').getAttribute('style'); + expect(backdrop).toContain(coverThumb); + expect(backdrop).not.toContain(coverFull); +}); From b29aafd13a96bc846c29d6be0535e2d5b96569a3 Mon Sep 17 00:00:00 2001 From: StateAntigen Date: Mon, 21 Sep 2026 12:32:26 -0700 Subject: [PATCH 34/42] fix(napstrfy): answer for back after every press, not only when the answer changes Back stopped working on the liked page after it had been used there once. The press was consumed - Android takes the flag when it hands a press to the page, because one press is one answer - and the page then published its answer again only if that answer had changed. Closing the liked page lands on the search page, which is itself somewhere back can go, so the answer was still "yes" and nothing derived from it fired. The flag stayed cleared, and the next press left the app. Which is why it worked the first time and never again: only a trip through the home tab, where the answer really does change, restored it. The page now counts the presses it handles and publishes its answer after each one. The Kotlin side is unchanged in what it does; what it does is now documented as the contract it is, since the page has to hold up its end of it. Covered by a test that presses back the way Android does - taking the flag when the press is consumed, and counting a press it was not given as the app being left - and walks out of each page the app advertises: the search tab, the liked page, an album preview and the player drawer. Removing the re-publish makes the walk fail at the press after the liked page, which is the fault reported. The drawer's artwork test also stopped racing the clock: the player bar fetches the full cover itself, so a fixed delay could expire before the drawer was even open. The test now holds that request open until it says so. --- android/native/android/BackBridge.kt | 18 +++-- android/src/App.svelte | 16 +++++ tests/browser/napstrfy-library.spec.mjs | 87 ++++++++++++++++++++----- 3 files changed, 99 insertions(+), 22 deletions(-) diff --git a/android/native/android/BackBridge.kt b/android/native/android/BackBridge.kt index 5d0ae55..fd8b54d 100644 --- a/android/native/android/BackBridge.kt +++ b/android/native/android/BackBridge.kt @@ -8,9 +8,10 @@ import java.lang.ref.WeakReference * Lets the page own the hardware back button. * * Kotlin cannot ask the page whether it has anything for back to close, so the - * webview pushes that flag on every transition. While it is set, a back press is - * handed to the page and consumed; otherwise the press goes back to the system, - * so back still leaves the app when the page has nothing open. + * webview pushes that flag on every transition and after every press it handles. + * While it is set, a back press is handed to the page and consumed; otherwise the + * press goes back to the system, so back still leaves the app when the page has + * nothing open. */ class BackBridge { @JavascriptInterface @@ -39,11 +40,16 @@ class BackBridge { webView.clear() } - /** True when the press was handed to the page rather than the system. */ + /** + * True when the press was handed to the page rather than the system. + * + * Clearing the flag here is what makes one press one answer: the page + * publishes again as soon as it has moved, which is why a page that still + * has somewhere to go - the search tab under the liked page - is not left + * with a flag that says otherwise and then goes unheard. + */ fun consumeBack(): Boolean { if (!backAvailable) return false - // Clearing it decides this one press only: the page publishes the flag - // again as soon as it has closed whatever it had. backAvailable = false webView.get()?.post { webView.get()?.evaluateJavascript( diff --git a/android/src/App.svelte b/android/src/App.svelte index aaf8d12..f171e8a 100644 --- a/android/src/App.svelte +++ b/android/src/App.svelte @@ -530,6 +530,10 @@ /** The hardware back button arrives as an event, not a callback. */ function handleSystemBack() { + // Android took the flag to hand us this press, so the answer below has to be + // published again: the states that follow this one are not always a change + // of answer, and a derived value that stays true would publish nothing. + backPresses += 1; if (showReport) { showReport = false; return; @@ -1716,8 +1720,20 @@ activeTab !== 'music' ); + /** + * Counted by every press the page handles. + * + * Android clears the flag when it gives the page a press, because one press is + * one answer. A press that closes the liked page lands on the search page, + * which is also somewhere back can go, so the answer does not change and + * nothing derived from it will fire on its own: without this counter the flag + * would stay cleared and the app would be left by the press after that. + */ + let backPresses = $state(0); + // The page, and every view above it, own the hardware back button. $effect(() => { + void backPresses; pushBackAvailability(backHasDestination); }); diff --git a/tests/browser/napstrfy-library.spec.mjs b/tests/browser/napstrfy-library.spec.mjs index ca5d474..7ea5162 100644 --- a/tests/browser/napstrfy-library.spec.mjs +++ b/tests/browser/napstrfy-library.spec.mjs @@ -37,21 +37,29 @@ const albumCover = { author: '', eventId: '', createdAt: 0, seeder: false }; -async function openApp(page, { library = [], album = null, cached = null, likes = null, platform = 'linux' } = {}) { +async function openApp(page, { library = [], album = null, cached = null, likes = null, platform = 'linux', holdFullCover = false } = {}) { + // A test that needs the full rendition still in flight holds it there, rather + // than racing the clock: the player bar fetches that same image, so a delay + // can expire before the drawer that is being tested is even open. + let releaseFullCover = () => {}; + const fullCoverGate = holdFullCover ? new Promise((resolve) => { releaseFullCover = resolve; }) : null; await mockNative(page, { platform }); await page.route('**/fixture.wav', serveAudio); // Registered after the blanket https route, so it wins for the covers. The // full rendition is held back to leave the thumbnail on screen on its own. await page.route(coverThumb, (route) => route.fulfill({ contentType: 'image/png', body: tinyPng })); await page.route(coverFull, async (route) => { - await new Promise((resolve) => setTimeout(resolve, 400)); + if (fullCoverGate) await fullCoverGate; + else await new Promise((resolve) => setTimeout(resolve, 400)); await route.fulfill({ contentType: 'image/png', body: tinyPng }); }); await page.addInitScript(({ library, album, cover, cached, likes }) => { if (likes) window.localStorage.setItem('napstrfy-liked-music', JSON.stringify(likes)); // The page talks to Kotlin through this object. The native side is not in the - // browser, so record the flag it is given instead of pressing a real button. + // browser, so record the flag it is given instead of pressing a real button, + // and count what Android would have done with a press it was not given. window.backAvailable = null; + window.appExits = 0; window.NapstrfyBack = { setBackAvailable: (available) => { window.backAvailable = available; }, setDrawerOpen: (open) => { window.backAvailable = open; } @@ -69,6 +77,7 @@ async function openApp(page, { library = [], album = null, cached = null, likes }; }, { library, album, cover: albumCover, cached, likes }); await page.goto('http://127.0.0.1:15174'); + return { releaseFullCover }; } /** The liked page is only reachable from the chip on the search tab. */ @@ -85,6 +94,26 @@ const searchPageIsBack = (page) => Promise.all([ expect(page.locator('.liked-close')).toHaveCount(0) ]); +const backFlag = (page) => page.evaluate(() => window.backAvailable); + +/** + * Presses the hardware back button the way Android does. The page is given the + * press only while it advertises a destination, and the flag is taken when the + * press is consumed - which is the page's cue to publish its answer again. A + * press with nothing behind it leaves the app. + */ +async function pressBack(page) { + return page.evaluate(() => { + if (!window.backAvailable) { + window.appExits += 1; + return 'exit'; + } + window.backAvailable = false; + window.dispatchEvent(new CustomEvent('napstrfy-back')); + return 'handled'; + }); +} + test('Napstrfy leaves the liked page from its close button', async ({ page }) => { await openApp(page, { likes: [likedSong] }); await openLikedPage(page); @@ -92,19 +121,44 @@ test('Napstrfy leaves the liked page from its close button', async ({ page }) => await searchPageIsBack(page); }); -test('Napstrfy walks back from the liked page to the search page and home, rather than out of the app', async ({ page }) => { - await openApp(page, { likes: [likedSong] }); +test('Napstrfy walks back out of every page it advertises, and is only left from home', async ({ page }) => { + // A phone, because a wide desktop window pins the player as a column that is + // never a drawer to close. + await openApp(page, { library: zzTop, album: zzTop, likes: [likedSong], platform: 'android' }); + // Home has nothing behind it, so this is the one press that leaves the app. + await expect.poll(() => backFlag(page)).toBe(false); + expect(await pressBack(page)).toBe('exit'); + + // The search tab has home behind it. + await page.locator('.bottom-nav button').nth(1).click(); + await expect.poll(() => backFlag(page)).toBe(true); + expect(await pressBack(page)).toBe('handled'); + await expect(page.locator('.library-heading h1')).toHaveText('Your music'); + + // The liked page has the search page behind it, and the press after this one is + // the one that used to leave the app: the flag is taken when a press is + // consumed, and the liked page closing onto another page that back can leave + // does not change the answer, so the page has to publish it again regardless. await openLikedPage(page); - // Back has somewhere to go, so the press must reach the page rather than the app. - await expect.poll(() => page.evaluate(() => window.backAvailable)).toBe(true); - await page.evaluate(() => window.dispatchEvent(new CustomEvent('napstrfy-back'))); + expect(await pressBack(page)).toBe('handled'); await searchPageIsBack(page); - // The search page has the home tab behind it, so back is still the page's. - await expect.poll(() => page.evaluate(() => window.backAvailable)).toBe(true); - await page.evaluate(() => window.dispatchEvent(new CustomEvent('napstrfy-back'))); + expect(await pressBack(page)).toBe('handled'); await expect(page.locator('.library-heading h1')).toHaveText('Your music'); - // Home is the end of the road: only from here does back leave the app. - await expect.poll(() => page.evaluate(() => window.backAvailable)).toBe(false); + + // An album preview, and the player drawer, each have the library behind them. + await page.locator('.album-open').click(); + await expect.poll(() => backFlag(page)).toBe(true); + expect(await pressBack(page)).toBe('handled'); + await expect(page.locator('.album-view')).toHaveCount(0); + + await page.locator('.track-open').first().click(); + await page.locator('.now-open').click(); + await expect.poll(() => backFlag(page)).toBe(true); + expect(await pressBack(page)).toBe('handled'); + await expect(page.locator('.now-sheet')).toHaveCount(0); + + // One press in the whole walk was not the page's to handle. + expect(await page.evaluate(() => window.appExits)).toBe(1); }); test('Napstrfy leaves the liked page with a right swipe, without playing what was under the finger', async ({ page }) => { @@ -163,15 +217,16 @@ test('Napstrfy opens an album on its thumbnail and fades the full cover in over test('Napstrfy opens the now-playing drawer on the thumbnail rather than a blank square', async ({ page }) => { // A phone, because a wide desktop window pins this sheet as a column and has // no drawer to open. - await openApp(page, { library: [zzTop[0]], album: zzTop, platform: 'android' }); - await page.locator('.track-open').click(); + const { releaseFullCover } = await openApp(page, { library: zzTop, album: zzTop, platform: 'android', holdFullCover: true }); + await page.locator('.track-open').first().click(); await page.locator('.now-open').click(); const thumb = page.locator('.now-sheet-art img.now-sheet-art-thumb'); const full = page.locator('.now-sheet-art img.now-sheet-art-full'); // The tile that was tapped fetched the small rendition, so the drawer is a - // cover from its first frame; the full one is a fresh download behind it. + // cover from its first frame while the full one is still on its way. await expect(thumb).toHaveAttribute('src', coverThumb); await expect(full).not.toHaveClass(/ready/); + releaseFullCover(); await expect(full).toHaveClass(/ready/); // The backdrop is blurred too far to show a bigger image, so it takes the // small rendition rather than making a second request for the large one. From e13c71cf57b6854161cc465cd74c6b001e91de34 Mon Sep 17 00:00:00 2001 From: StateAntigen Date: Mon, 21 Sep 2026 12:43:14 -0700 Subject: [PATCH 35/42] feat(napstrfy): ask for and fetch the next track's artwork as it plays The player drawer draws whatever it has for the track that starts, and the track that starts is often one whose tile is not on screen: a list is only asked about the albums near the viewport, and the playlist view only looks up the first twelve rows. Pressing next on such a track left the drawer with nothing to draw until an ask and a fetch had both come back. The track after the one playing is now resolved and its thumbnail fetched at the moment the one before it starts. That is the same moment the audio prefetch for the next track happens, so the work shares the next-track arithmetic the player already does - shuffle, repeat and the end of the queue included. The small rendition is what is fetched: a few kilobytes, and the one that holds the space in a large view. The full cover is the publisher's own upload, worth hundreds of kilobytes, and is left to the view that shows it, fading in over the thumbnail that is already there. Covered by a test on a forty-track library where the playlist row that is played is twenty-one tracks along: its successor's album has never been asked about by any tile or shelf, and the ask for it appears at the moment the predecessor starts. Without the preload the ask never happens. --- android/src/App.svelte | 5 ++++- android/src/lib/artwork.ts | 33 +++++++++++++++++++++++++++++++++ tests/browser/artwork.spec.mjs | 23 +++++++++++++++++++++++ 3 files changed, 60 insertions(+), 1 deletion(-) diff --git a/android/src/App.svelte b/android/src/App.svelte index f171e8a..ffcffbd 100644 --- a/android/src/App.svelte +++ b/android/src/App.svelte @@ -20,7 +20,7 @@ import SeekIcon from './lib/SeekIcon.svelte'; import { rateLimitedTask, safePosition, validDuration } from './lib/playback'; import appIcon from '../src-tauri/icons/icon.png'; - import { artworkHue, coverFor, coverKey, invalidateCoverNegatives, type AlbumCover } from './lib/artwork'; + import { artworkHue, coverFor, coverKey, invalidateCoverNegatives, preloadArtwork, type AlbumCover } from './lib/artwork'; import { reportReasons } from './lib/types'; import type { AudiobookLibraryPage, CachedAudio, CompanionStatus, CoverReport, LibraryPage, PlaybackCommand, PodcastDownload, PodcastEpisode, PodcastFeed, ReadOnlyTicketOffer, RemoteAudiobook, RemoteAudiobookSummary, RemotePlaybackState, RemoteRepeat, RemoteTrack, RemoteTransfer, ReportReason } from './lib/types'; @@ -1196,6 +1196,9 @@ libraryVisible }); } + // The same track's artwork is asked about and fetched now, so the player + // has a cover the moment it starts instead of after a round trip. + if (next) preloadArtwork(next); } catch (nextError) { playing = false; error = `Could not play ${title(track)}: ${String(nextError)}`; diff --git a/android/src/lib/artwork.ts b/android/src/lib/artwork.ts index 206d745..58849ab 100644 --- a/android/src/lib/artwork.ts +++ b/android/src/lib/artwork.ts @@ -192,6 +192,39 @@ export function coverFor(track: RemoteTrack): Promise { return requestCover(key); } +/** Thumbnails already fetched on the chance they would be wanted. */ +const warmedThumbs = new Set(); + +/** + * Resolve a track's cover and fetch its small rendition now, rather than when the + * screen that shows it appears. + * + * This is for the track coming next: by the time it plays, its cover has been + * asked about and its thumbnail is in the image cache, so the player is not the + * first place either happens. It costs one batched ask - free when the album is + * already known, as it is when the track was picked from a list - and a few + * kilobytes of thumbnail. + * + * The full rendition is deliberately not fetched: it is the publisher's own + * upload, worth hundreds of kilobytes, and it is left to the view that displays + * it, fading in over the thumbnail that is already there. + */ +export function preloadArtwork(track: RemoteTrack) { + void coverFor(track).then(preloadThumbnail); +} + +/** Fetch a small rendition into the image cache, once per URL. */ +function preloadThumbnail(cover: AlbumCover | null) { + const url = cover?.thumb; + if (!url || warmedThumbs.has(url)) return; + warmedThumbs.add(url); + // Nothing holds this element: the fetch it starts is the point, and whether + // the cover is still wanted when it lands is decided by the screen itself. + const image = new Image(); + image.decoding = 'async'; + image.src = url; +} + /** * Forget one album's cover because the image itself would not load, so the * next ask resolves a fresh URL instead of handing back the dead one. The diff --git a/tests/browser/artwork.spec.mjs b/tests/browser/artwork.spec.mjs index a82787d..74c5b67 100644 --- a/tests/browser/artwork.spec.mjs +++ b/tests/browser/artwork.spec.mjs @@ -73,6 +73,29 @@ test('Napstrfy asks the host about the artwork it can show, in batches by album, expect(await askCount(page)).toBe(asksBeforePlay); }); +test('Napstrfy asks about the artwork of the track next in the queue while the one before it plays', async ({ page }) => { + await mockNative(page); + await seedLibrary(page, 40); + await page.route('**/cover-*.png', (route) => route.fulfill({ contentType: 'image/png', body: PNG })); + await page.route('**/fixture.wav', serveAudio); + await page.goto('http://127.0.0.1:15174'); + const rows = page.locator('.track-row'); + await expect(rows).toHaveCount(40); + await rows.first().locator('.track-open').click(); + // The playlist looks its own artwork up for the first twelve rows only, and the + // library rows that far down have never been scrolled into view, so the album + // twenty-one tracks along has never been asked about by anything on screen. + await page.getByRole('button', { name: 'Open the playlist' }).click(); + const asked = async () => (await asks(page)).flat().join('\n'); + // The rows on screen have asked for their own albums by now, and the ones below + // the fold have not, so this is a real absence rather than a race not yet lost. + await expect.poll(asked).toContain('album 5'); + expect(await asked()).not.toContain('album 21'); + await page.locator('.queue-row').nth(20).locator('.queue-open').click(); + // Playing it is what asks on behalf of the track that will follow it. + await expect.poll(asked).toContain('album 21'); +}); + test('Napstrfy artwork recovers from a transient host failure while the screen stays open', async ({ page }) => { await mockNative(page); await seedLibrary(page, 1); From f70267b18ff0d5da480f97a1fb433de181332dc0 Mon Sep 17 00:00:00 2001 From: StateAntigen Date: Mon, 21 Sep 2026 12:55:10 -0700 Subject: [PATCH 36/42] fix(napstrfy): draw the player bar, and the cover behind it, from the thumbnail The bar's tile asked for the full front cover: a rendition hundreds of pixels wider than the tile, downloaded when the track starts, and the one thing the bar waited for. Dense rows had always taken the published thumbnail; the bar took the full one only because it is the biggest tile this component draws, which is a fact about the box, not about the image that belongs in it. Every tile takes the thumbnail now, and the views that show a cover big keep drawing the full one themselves, over a thumbnail rather than instead of one. The covered stretched behind the bar was the full cover too, blurred far past what a larger image could show, and it is now the same small rendition the tile that started the track already fetched. Covered by a test that never delivers the full rendition at all: the bar's tile and its stretched cover are drawn from the thumbnail regardless, which they cannot be if either of them is still asking for the other one. --- android/src/App.svelte | 8 +++++--- android/src/lib/TrackArtwork.svelte | 16 +++++++++++++--- tests/browser/napstrfy-library.spec.mjs | 12 ++++++++++++ 3 files changed, 30 insertions(+), 6 deletions(-) diff --git a/android/src/App.svelte b/android/src/App.svelte index ffcffbd..ada3417 100644 --- a/android/src/App.svelte +++ b/android/src/App.svelte @@ -331,13 +331,15 @@ ); /** The track the bar draws, which is the computer's when it is the source. */ let barTrack = $derived(playbackTarget === 'desktop' ? desktopTrackFromState() : null); - /** The cover the bar stretches behind itself, or '' when there is none. */ + /** The cover the bar stretches behind itself, or '' when there is none. It is + * blurred far past what a large image could show, so it takes the small + * rendition: the one the tile that started this track has already fetched. */ let barArtwork = $derived( playbackTarget === 'desktop' - ? sheetCoverUrl() + ? sheetThumbUrl() || sheetCoverUrl() : activeMedia === 'podcast' ? currentPodcast?.image ?? '' - : sheetCoverUrl() + : sheetThumbUrl() || sheetCoverUrl() ); /** The custom properties its stretched cover needs, inert when there is none. */ let barArtStyle = $derived( diff --git a/android/src/lib/TrackArtwork.svelte b/android/src/lib/TrackArtwork.svelte index b67486f..322ca97 100644 --- a/android/src/lib/TrackArtwork.svelte +++ b/android/src/lib/TrackArtwork.svelte @@ -2,6 +2,14 @@ import { ARTWORK_RETRY_MS, artworkHue, coverFor, coverRevision, dropCover } from './artwork'; import type { RemoteTrack } from './types'; + /** + * Every tile draws the small rendition. The full cover is the publisher's own + * upload, so the views that show it big - the album header, the drawer - draw + * it themselves, over a thumbnail rather than instead of one. + * + * `large` is the player bar's tile: the biggest one here, at the height of the + * bar, and it loads at once instead of when it is scrolled into view. + */ let { track, lookup = true, large = false, onartworkchange }: { track: RemoteTrack; lookup?: boolean; large?: boolean; onartworkchange?: (url: string) => void } = $props(); let image = $state(''); let failed = $state(false); @@ -52,9 +60,11 @@ retryLater(); return; } - // Dense grids prefer the published thumbnail; a large tile wants the - // full front cover, falling back to whichever the publisher gave us. - image = large ? cover.art || cover.thumb : cover.thumb || cover.art; + // The small rendition is what a tile wants, dense or not: the largest + // one here is the player bar's, at the height of the bar itself. The + // views that show a cover big - the album header, the drawer - draw the + // full one themselves, over the small one they start from. + image = cover.thumb || cover.art; }); } return () => { alive = false; clearTimeout(retryTimer); }; diff --git a/tests/browser/napstrfy-library.spec.mjs b/tests/browser/napstrfy-library.spec.mjs index 7ea5162..974b719 100644 --- a/tests/browser/napstrfy-library.spec.mjs +++ b/tests/browser/napstrfy-library.spec.mjs @@ -161,6 +161,18 @@ test('Napstrfy walks back out of every page it advertises, and is only left from expect(await page.evaluate(() => window.appExits)).toBe(1); }); +test('Napstrfy draws the player bar and the cover stretched behind it from the thumbnail', async ({ page }) => { + // The full rendition is never delivered in this test, so whatever is drawn on + // the bar is drawn from the small one the tile that was tapped already had. + const { releaseFullCover } = await openApp(page, { library: zzTop, album: zzTop, platform: 'android', holdFullCover: true }); + await page.locator('.track-open').first().click(); + await expect(page.locator('.now-playing .artwork img')).toHaveAttribute('src', coverThumb); + const bar = await page.locator('.now-playing').getAttribute('style'); + expect(bar).toContain(coverThumb); + expect(bar).not.toContain(coverFull); + releaseFullCover(); +}); + test('Napstrfy leaves the liked page with a right swipe, without playing what was under the finger', async ({ page }) => { await openApp(page, { likes: [likedSong] }); await openLikedPage(page); From 94bdfe26992e1a0c21f187b79b8ece605d8c5e83 Mon Sep 17 00:00:00 2001 From: StateAntigen Date: Mon, 21 Sep 2026 13:19:12 -0700 Subject: [PATCH 37/42] feat(napstrfy): preload the next track's full cover, and upgrade the lock screen to it The system's copy of the artwork was the full cover from the start, so the lock screen and the notification showed nothing until that rendition had been fetched and decoded. It is given the thumbnail first now, which is the rendition already on this phone, and the full cover replaces it as soon as it has loaded: the media service takes a new URL for the same art and re-posts without alerting again. A full cover that will not load leaves the thumbnail in place, which is what the thumbnail was always a fallback for. The preload for the track coming next now fetches both of its renditions rather than the small one alone. That is what makes the upgrade immediate rather than merely eventual - the full cover is in the image cache before the track starts - and it gives the drawer, which fades the full cover in over the thumbnail, the same head start. One fetch per URL however many callers want it, so a preload and a view that draws the same cover cost one download between them, and a rendition that 404s is remembered so it is not retried on every track. The cost of fetching the full one is noted where the decision is made, since it is the part of this worth revisiting on a metered connection. Covered by a test that holds the full rendition open, which makes the order the two are published in the assertion rather than a race: the thumbnail is what the media service is given while the bigger one is on its way, and the full cover afterwards. The queue test now asserts both renditions are fetched on behalf of the track that follows the one playing, and the artwork fixture gives each album two URLs so the two can be told apart. --- android/src/App.svelte | 46 +++++++++++++-- android/src/lib/artwork.ts | 74 +++++++++++++++++-------- tests/browser/artwork.spec.mjs | 42 ++++++++++---- tests/browser/napstrfy-library.spec.mjs | 27 ++++++++- 4 files changed, 145 insertions(+), 44 deletions(-) diff --git a/android/src/App.svelte b/android/src/App.svelte index ada3417..43698a4 100644 --- a/android/src/App.svelte +++ b/android/src/App.svelte @@ -20,7 +20,7 @@ import SeekIcon from './lib/SeekIcon.svelte'; import { rateLimitedTask, safePosition, validDuration } from './lib/playback'; import appIcon from '../src-tauri/icons/icon.png'; - import { artworkHue, coverFor, coverKey, invalidateCoverNegatives, preloadArtwork, type AlbumCover } from './lib/artwork'; + import { artworkHue, coverFor, coverKey, invalidateCoverNegatives, loadFullCover, preloadArtwork, type AlbumCover } from './lib/artwork'; import { reportReasons } from './lib/types'; import type { AudiobookLibraryPage, CachedAudio, CompanionStatus, CoverReport, LibraryPage, PlaybackCommand, PodcastDownload, PodcastEpisode, PodcastFeed, ReadOnlyTicketOffer, RemoteAudiobook, RemoteAudiobookSummary, RemotePlaybackState, RemoteRepeat, RemoteTrack, RemoteTransfer, ReportReason } from './lib/types'; @@ -194,6 +194,11 @@ /** The sheet's full-cover URL once that image has landed, so the small * rendition under it stays on screen until something better is drawn. */ let sheetArtLoaded = $state(''); + /** + * The best cover the system has been given for what is playing: the thumbnail + * to begin with, then the full cover once that one has landed. + */ + let lockScreenCover = $state(''); /** The track the resolved cover belongs to, so a new object for the same one * does not throw the artwork away and fetch it again. */ let nowCoverKey = ''; @@ -1357,7 +1362,7 @@ publishSystemMetadata({ title: state.title || 'Unknown track', artist: state.artist || status.desktopName || 'The computer', - artwork: sheetCoverUrl(), + artwork: lockScreenArtwork(), playing: state.playing, position: safePosition(remotePositionMs() / 1000, seconds || undefined), duration: seconds, @@ -1394,10 +1399,7 @@ artist: activeMedia === 'podcast' ? currentPodcast?.feedTitle ?? '' : current ? artist(current) : '', artwork: activeMedia === 'podcast' ? currentPodcast?.image ?? '' - // The full cover is what the lock screen shows, but a large rendition - // that will not load should not leave it bare: the thumbnail is the - // one rendition already proven to exist on this phone. - : nowCover ? (nowArtFailed ? nowCover.thumb : nowCover.art || nowCover.thumb) : '', + : lockScreenArtwork(), playing, position: safePosition(currentTime, seconds || undefined), duration: seconds, @@ -1654,6 +1656,25 @@ } }); + // The lock screen starts on the thumbnail, which is already here, and moves to + // the full cover once that has landed. For a track that was reached by playing + // the one before it, the preload means this is a cache hit and the upgrade + // follows within a frame or two of the notification appearing. + $effect(() => { + const cover = nowCover; + lockScreenCover = cover?.thumb ?? ''; + const full = cover?.art ?? ''; + if (!full || full === cover?.thumb) return; + let alive = true; + void loadFullCover(cover).then((landed) => { + if (!alive || !landed) return; + lockScreenCover = landed; + // Not forced: nothing here is wrong-looking until the coalescer comes round. + syncSystemMedia(); + }); + return () => { alive = false; }; + }); + // While a drag is in flight the browser must not claim the gesture as a // scroll: these listeners are deliberately non-passive so they can stop that. $effect(() => { @@ -1791,6 +1812,19 @@ return nowCover?.thumb ?? ''; } + /** + * The artwork the system is given for what is playing. + * + * The thumbnail goes first, because it is the rendition already on this phone, + * and the full cover replaces it as soon as that one has landed: the media + * service takes a new URL for the same art and re-posts without alerting again. + * A full cover that will not load leaves the thumbnail in place. + */ + function lockScreenArtwork(): string { + if (!nowCover) return ''; + return lockScreenCover || nowCover.thumb || nowCover.art; + } + async function playFromQueue(index: number) { const track = playerQueue[index]; if (!track) return; diff --git a/android/src/lib/artwork.ts b/android/src/lib/artwork.ts index 58849ab..ca9cb5e 100644 --- a/android/src/lib/artwork.ts +++ b/android/src/lib/artwork.ts @@ -192,37 +192,65 @@ export function coverFor(track: RemoteTrack): Promise { return requestCover(key); } -/** Thumbnails already fetched on the chance they would be wanted. */ -const warmedThumbs = new Set(); +/** Renditions already asked for, by URL, with the load that was started. */ +const renditionLoads = new Map>(); /** - * Resolve a track's cover and fetch its small rendition now, rather than when the - * screen that shows it appears. + * Fetch a rendition now and hand back what landed: its URL, or '' when the image + * would not load. * - * This is for the track coming next: by the time it plays, its cover has been - * asked about and its thumbnail is in the image cache, so the player is not the - * first place either happens. It costs one batched ask - free when the album is - * already known, as it is when the track was picked from a list - and a few - * kilobytes of thumbnail. + * One fetch per URL however many callers ask, so a view that draws the same + * cover, and a preload that has already fetched it, cost one download between + * them. A failure is remembered for the session, which is what keeps an album + * whose image 404s from being retried on every track. + */ +function loadRendition(url: string): Promise { + if (!url) return Promise.resolve(''); + const asked = renditionLoads.get(url); + if (asked) return asked; + const landed = new Promise((resolve) => { + // Nothing holds this element: the fetch it starts is the point, and whether + // the cover is still wanted when it lands is decided by the screen itself. + const image = new Image(); + image.decoding = 'async'; + image.onload = () => resolve(url); + image.onerror = () => resolve(''); + image.src = url; + }); + renditionLoads.set(url, landed); + return landed; +} + +/** + * Fetch both renditions of a track's cover now, rather than when the screens that + * show them appear. This is for the track coming next. * - * The full rendition is deliberately not fetched: it is the publisher's own - * upload, worth hundreds of kilobytes, and it is left to the view that displays - * it, fading in over the thumbnail that is already there. + * By the time it plays, its cover has been asked about, its thumbnail is in the + * image cache - so the player is a cover from its first frame - and its full + * cover has landed, so the drawer fades it in without waiting and the lock screen + * is able to move on to it at once. + * + * Fetching the full rendition is a deliberate cost, and one worth revisiting on a + * metered connection: every track that becomes the next one has its full cover + * downloaded whether or not anything gets to draw it. Dropping the second call + * below would leave everything else as it is, with the lock screen upgrading when + * its own fetch lands rather than before the track is even played. */ export function preloadArtwork(track: RemoteTrack) { - void coverFor(track).then(preloadThumbnail); + void coverFor(track).then((cover) => { + if (!cover) return; + void loadRendition(cover.thumb); + void loadRendition(cover.art); + }); } -/** Fetch a small rendition into the image cache, once per URL. */ -function preloadThumbnail(cover: AlbumCover | null) { - const url = cover?.thumb; - if (!url || warmedThumbs.has(url)) return; - warmedThumbs.add(url); - // Nothing holds this element: the fetch it starts is the point, and whether - // the cover is still wanted when it lands is decided by the screen itself. - const image = new Image(); - image.decoding = 'async'; - image.src = url; +/** + * The full cover of an album, fetched now and resolved when it has landed. The + * URL is handed back rather than a flag so a caller can publish the very image it + * waited for, and '' means the publisher's rendition would not load at all. + */ +export function loadFullCover(cover: AlbumCover | null): Promise { + return loadRendition(cover?.art ?? ''); } /** diff --git a/tests/browser/artwork.spec.mjs b/tests/browser/artwork.spec.mjs index 74c5b67..eb55335 100644 --- a/tests/browser/artwork.spec.mjs +++ b/tests/browser/artwork.spec.mjs @@ -8,14 +8,15 @@ const PNG = Buffer.from('iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR /** * `count` tracks, each its own album, so every tile has its own cover key. The - * host answers every key, with one of two URLs that says which album it was. + * host answers every key with a pair of renditions, a URL each, which says which + * album a request was for and which of the two was asked for. */ async function seedLibrary(page, count = 40) { await page.addInitScript((count) => { const invoke = window.__TAURI_INTERNALS__.invoke; - const makeCover = (key, art) => ({ key, art, thumb: art, mbid: '', year: '', genre: '', collection: '', - source: 'itunes', coverFileId: '', mime: 'image/png', author: '', seeder: false }); - const artFor = (key) => `/cover-${Number(key.split('|')[1].slice('album '.length)) % 2 ? 'b' : 'a'}.png`; + const makeCover = (key, index) => ({ key, art: `/cover-${index}.png`, thumb: `/cover-${index}-thumb.png`, + mbid: '', year: '', genre: '', collection: '', source: 'itunes', coverFileId: '', mime: 'image/png', author: '', seeder: false }); + const indexFor = (key) => Number(key.split('|')[1].slice('album '.length)); const tracks = Array.from({ length: count }, (_, index) => ({ fileId: index.toString(16).padStart(64, '0'), filename: `Song ${index}.wav`, title: `Song ${index}`, artist: 'Rancid', album: `Album ${index}`, @@ -29,7 +30,7 @@ async function seedLibrary(page, count = 40) { if (cmd === 'remote_covers') { window.coverAsks.push([...args.keys]); if (window.coverFails) throw new Error('Host is not reachable'); - return args.keys.map((key) => makeCover(key, artFor(key))); + return args.keys.map((key) => makeCover(key, indexFor(key))); } return invoke(cmd, args); }; @@ -47,7 +48,9 @@ test('Napstrfy asks the host about the artwork it can show, in batches by album, await page.goto('http://127.0.0.1:15174'); const rows = page.locator('.track-row'); await expect(rows).toHaveCount(40); - await expect(rows.first().locator('.artwork img')).toHaveAttribute('src', '/cover-a.png'); + // A row draws the publisher's thumbnail: the largest tile on a screen of rows is + // a few dozen pixels across. + await expect(rows.first().locator('.artwork img')).toHaveAttribute('src', '/cover-0-thumb.png'); // One batched ask for the albums the screen shows, rather than one call per // tile. The batch is whatever registered inside the flush window, so how many // albums it holds depends on how much of the list had laid out by then: the @@ -62,13 +65,15 @@ test('Napstrfy asks the host about the artwork it can show, in batches by album, // many batches that takes is timing, not behaviour, so only the fact that new // work happened is asserted. await rows.nth(39).scrollIntoViewIfNeeded(); - await expect(rows.nth(39).locator('.artwork img')).toHaveAttribute('src', '/cover-b.png'); + await expect(rows.nth(39).locator('.artwork img')).toHaveAttribute('src', '/cover-39-thumb.png'); await expect.poll(() => askCount(page)).toBeGreaterThan(1); // Playing it draws the same answer in the player, without leaving the screen - // and - the guarantee worth pinning - without asking the host again. + // and - the guarantee worth pinning - without asking the host again. The drawer + // is the thumbnail standing under the full cover, both of this same album. const asksBeforePlay = await askCount(page); await rows.nth(39).locator('.track-open').click(); - await expect(page.locator('.now-sheet-art img')).toHaveAttribute('src', '/cover-b.png'); + await expect(page.locator('.now-sheet-art img.now-sheet-art-thumb')).toHaveAttribute('src', '/cover-39-thumb.png'); + await expect(page.locator('.now-sheet-art img.now-sheet-art-full')).toHaveAttribute('src', '/cover-39.png'); await expect(rows).toHaveCount(40); expect(await askCount(page)).toBe(asksBeforePlay); }); @@ -76,7 +81,13 @@ test('Napstrfy asks the host about the artwork it can show, in batches by album, test('Napstrfy asks about the artwork of the track next in the queue while the one before it plays', async ({ page }) => { await mockNative(page); await seedLibrary(page, 40); - await page.route('**/cover-*.png', (route) => route.fulfill({ contentType: 'image/png', body: PNG })); + // Recorded rather than counted: both renditions of the album are asked for, and + // each has its own URL here so the two can be told apart. + const fetched = []; + await page.route('**/cover-*.png', (route) => { + fetched.push(new URL(route.request().url()).pathname); + return route.fulfill({ contentType: 'image/png', body: PNG }); + }); await page.route('**/fixture.wav', serveAudio); await page.goto('http://127.0.0.1:15174'); const rows = page.locator('.track-row'); @@ -91,9 +102,16 @@ test('Napstrfy asks about the artwork of the track next in the queue while the o // the fold have not, so this is a real absence rather than a race not yet lost. await expect.poll(asked).toContain('album 5'); expect(await asked()).not.toContain('album 21'); + expect(fetched).not.toContain('/cover-21-thumb.png'); + expect(fetched).not.toContain('/cover-21.png'); await page.locator('.queue-row').nth(20).locator('.queue-open').click(); - // Playing it is what asks on behalf of the track that will follow it. + // Playing it is what asks on behalf of the track that will follow it, and what + // fetches both of its renditions: the thumbnail so the player has a cover at + // once, and the full one so the drawer and the lock screen have nothing left to + // wait for. await expect.poll(asked).toContain('album 21'); + await expect.poll(() => fetched).toContain('/cover-21-thumb.png'); + await expect.poll(() => fetched).toContain('/cover-21.png'); }); test('Napstrfy artwork recovers from a transient host failure while the screen stays open', async ({ page }) => { @@ -111,7 +129,7 @@ test('Napstrfy artwork recovers from a transient host failure while the screen s await page.evaluate(() => { window.coverFails = false; }); await page.clock.runFor(61_000); await page.clock.runFor(500); - await expect(page.locator('.track-row .artwork img')).toHaveAttribute('src', '/cover-a.png'); + await expect(page.locator('.track-row .artwork img')).toHaveAttribute('src', '/cover-0-thumb.png'); expect(await askCount(page)).toBe(2); expect(await page.locator('.track-row').count()).toBe(1); }); diff --git a/tests/browser/napstrfy-library.spec.mjs b/tests/browser/napstrfy-library.spec.mjs index 974b719..3fd91d4 100644 --- a/tests/browser/napstrfy-library.spec.mjs +++ b/tests/browser/napstrfy-library.spec.mjs @@ -37,7 +37,7 @@ const albumCover = { author: '', eventId: '', createdAt: 0, seeder: false }; -async function openApp(page, { library = [], album = null, cached = null, likes = null, platform = 'linux', holdFullCover = false } = {}) { +async function openApp(page, { library = [], album = null, cached = null, likes = null, platform = 'linux', holdFullCover = false, recordMedia = false } = {}) { // A test that needs the full rendition still in flight holds it there, rather // than racing the clock: the player bar fetches that same image, so a delay // can expire before the drawer that is being tested is even open. @@ -53,8 +53,14 @@ async function openApp(page, { library = [], album = null, cached = null, likes else await new Promise((resolve) => setTimeout(resolve, 400)); await route.fulfill({ contentType: 'image/png', body: tinyPng }); }); - await page.addInitScript(({ library, album, cover, cached, likes }) => { + await page.addInitScript(({ library, album, cover, cached, likes, recordMedia }) => { if (likes) window.localStorage.setItem('napstrfy-liked-music', JSON.stringify(likes)); + if (recordMedia) { + // The media service is not in the browser either: keep what it is told, so a + // test can watch the lock screen's artwork arrive and then be replaced. + window.mediaUpdates = []; + window.NapstrfyMedia = { update: (payload) => window.mediaUpdates.push(JSON.parse(payload)), clear: () => {} }; + } // The page talks to Kotlin through this object. The native side is not in the // browser, so record the flag it is given instead of pressing a real button, // and count what Android would have done with a press it was not given. @@ -75,7 +81,7 @@ async function openApp(page, { library = [], album = null, cached = null, likes if (cmd === 'remote_library' && !args.query) return { tracks: library, total: library.length }; return invoke(cmd, args); }; - }, { library, album, cover: albumCover, cached, likes }); + }, { library, album, cover: albumCover, cached, likes, recordMedia }); await page.goto('http://127.0.0.1:15174'); return { releaseFullCover }; } @@ -173,6 +179,21 @@ test('Napstrfy draws the player bar and the cover stretched behind it from the t releaseFullCover(); }); +test('Napstrfy gives the lock screen the thumbnail, then the full cover once it has landed', async ({ page }) => { + // The full rendition is held open, so the order the two are published in is the + // assertion rather than a race between them. + const { releaseFullCover } = await openApp(page, { library: zzTop, album: zzTop, platform: 'android', recordMedia: true, holdFullCover: true }); + await page.locator('.track-open').first().click(); + const artwork = () => page.evaluate(() => window.mediaUpdates.at(-1)?.artwork ?? ''); + // What the lock screen is given while the bigger rendition is on its way is the + // one already on this phone. + await expect.poll(artwork).toBe(coverThumb); + releaseFullCover(); + // The full cover takes its place as soon as it has loaded, which the media + // service takes as a new URL for the same art. + await expect.poll(artwork).toBe(coverFull); +}); + test('Napstrfy leaves the liked page with a right swipe, without playing what was under the finger', async ({ page }) => { await openApp(page, { likes: [likedSong] }); await openLikedPage(page); From 62561ad4f1926b7ce270b45e026a0dcef4433c7c Mon Sep 17 00:00:00 2001 From: StateAntigen Date: Sun, 27 Sep 2026 14:11:51 -0700 Subject: [PATCH 38/42] fix(cover): find the art when the archive will not answer for the record Three reasons a record MusicBrainz demonstrably knows came back as one nobody has art for, all of them measured rather than imagined. A 5xx was read as back-pressure. The archive answers 500 for particular release groups while serving everything else, so an album that could have been resolved was parked and the person was told Napstr had been throttled when it never was. Only 429 and 503 mean "slow down" now; the rest are a fault with one record. A group the archive will not answer for is retried through the releases inside it, because some answer 500 on their own endpoint while the releases inside them answer perfectly. And where the item has moved, `archive.org/metadata` is asked where it lives now: Cover Art Archive's redirects still point at the node the item was ingested onto, which is how a cover that MusicBrainz itself reports as `artwork: true` became unreachable. A tagger's joint credit is asked for one name at a time. `artist:"The Chainsmokers, Oaks"` matches nothing, because MusicBrainz holds the credited names and a comma inside a quoted phrase is read as loose syntax. Any one name is enough to match, and the release group is then chosen by the name the tag lists first, so `KREAM / Korolova` still picks the record MusicBrainz credits to `KREAM` alone. A group the archive refused is listed in the manual picker with the reason rather than dropped, since that row is the answer to "why does this album keep coming back empty". --- src-tauri/src/cover_publish.rs | 681 +++++++++++++++++++++++++++++++-- src/lib/CoverPicker.svelte | 13 + 2 files changed, 664 insertions(+), 30 deletions(-) diff --git a/src-tauri/src/cover_publish.rs b/src-tauri/src/cover_publish.rs index a07551c..7877eea 100644 --- a/src-tauri/src/cover_publish.rs +++ b/src-tauri/src/cover_publish.rs @@ -71,6 +71,10 @@ const THROTTLE_BACKOFF_MAX: Duration = Duration::from_secs(5 * 60); const THROTTLE_BACKOFF_DOUBLINGS: u32 = 4; /// A transient failure parks the album for this long before it is offered again. const FAILED_LOOKUP_RETRY_SECONDS: i64 = 15 * 60; +/// How many of a release group's releases the archive fallback walks before it +/// gives up. A group with art has it on one of the first few, and every release +/// walked is another request to the archive. +const MAX_FALLBACK_RELEASES: usize = 3; /// The most albums the window may preview at once. const MAX_PREVIEW: usize = 50; /// A safety net rather than a cooldown: the worker is woken by real events, and @@ -670,8 +674,15 @@ fn classify( Answer::Failed(format!("{host} answered {status}")) }; } - // 503 is MusicBrainz's own back-pressure; 429 is the standard one. - if status == reqwest::StatusCode::TOO_MANY_REQUESTS || status.is_server_error() { + // 503 is MusicBrainz's own back-pressure and 429 is the standard one. Any + // other 5xx is the server having a bad day with one record rather than a + // request to slow down: read as back-pressure it makes Napstr wait out an + // album it could have resolved, and tells the person it was throttled when + // it never was. The archive really does answer 500 for particular release + // groups while serving everything else. + if status == reqwest::StatusCode::TOO_MANY_REQUESTS + || status == reqwest::StatusCode::SERVICE_UNAVAILABLE + { return Answer::Throttled { retry_after: retry_after.and_then(parse_retry_after), }; @@ -736,6 +747,40 @@ struct MusicBrainzSearch { release_groups: Vec, } +/// One release group asked for by id, with the releases it holds. +#[derive(Deserialize)] +struct MusicBrainzGroupLookup { + #[serde(default)] + releases: Vec, +} + +#[derive(Deserialize)] +struct MusicBrainzRelease { + #[serde(default)] + id: String, +} + +/// One item archive.org holds, as `/metadata/` describes it. +/// +/// The two fields that matter are where it is *now*: an item is ingested onto +/// one node and one directory, and archive.org moves it later without the +/// Cover Art Archive's redirects knowing. +#[derive(Deserialize)] +struct ArchiveOrgItem { + #[serde(default)] + server: String, + #[serde(default)] + dir: String, + #[serde(default)] + files: Vec, +} + +#[derive(Deserialize)] +struct ArchiveOrgFile { + #[serde(default)] + name: String, +} + #[derive(Deserialize, Clone)] struct MusicBrainzGroup { #[serde(default)] @@ -752,6 +797,27 @@ struct MusicBrainzGroup { score: u32, #[serde(default, rename = "secondary-types")] secondary_types: Vec, + /// The names MusicBrainz credits this group to, in the order it lists them. + #[serde(default, rename = "artist-credit")] + artist_credit: Vec, +} + +#[derive(Deserialize, Clone)] +struct MusicBrainzArtistCredit { + #[serde(default)] + name: String, +} + +impl MusicBrainzGroup { + /// Whether MusicBrainz credits this group to a name that means `name`. + /// + /// A tag with no artist at all matches nothing, so every candidate ties on + /// this and the other keys decide. + fn credited_to(&self, name: &str) -> bool { + self.artist_credit + .iter() + .any(|credit| alike(&credit.name, name)) + } } /// Pick the release group that really is this album. @@ -759,18 +825,28 @@ struct MusicBrainzGroup { /// A matching title is not enough. `St. Anger` the album, the EP and the single /// all share one, and MusicBrainz ranks them by search score rather than by what /// a listener means — a single's Cover Art Archive entry is usually empty, so -/// choosing one turns a record that has art into "no art anywhere" for a -/// fortnight. A music library means the album, so an album group wins over -/// anything else whose title matches. +/// choosing one turns a record that has art into "no art at all". +/// +/// The artist decides first, and the kind of release after it, because a music +/// library means the album. The artist is first because the query is deliberately +/// loose about it — any one of the names a tag lists is enough to match — so the +/// candidate credited to the *first* name a tag gives is the one a listener +/// means. MusicBrainz credits `Annihilation` to `KREAM` while the tag reads +/// `KREAM / Korolova`, and a same-titled record by the other name must not win +/// over it. fn best_release_group<'a>( groups: &'a [MusicBrainzGroup], album: &str, + artist: &str, ) -> Option<&'a MusicBrainzGroup> { + let first = artist_names(artist).into_iter().next().unwrap_or_default(); groups .iter() .filter(|group| !group.id.is_empty() && alike(&group.title, album)) .min_by_key(|group| { ( + // The artist the tag names first: the strongest signal there is. + u8::from(!group.credited_to(&first)), // A music library means the album, not the seven-inch single. u8::from(!group.primary_type.eq_ignore_ascii_case("album")), // An exact title beats one that merely contains it, so @@ -852,7 +928,9 @@ async fn resolve( // Only a release group whose title really is this album is worth publishing: // a cover on the wrong record is worse than a blank square. Among those, // the album itself is the one a music library means. - let Some(group) = best_release_group(&found.release_groups, &candidate.album).cloned() else { + let Some(group) = best_release_group(&found.release_groups, &candidate.album, &candidate.artist) + .cloned() + else { return Ok(None); }; @@ -874,12 +952,76 @@ async fn resolve( /// /// Also where the manual art tool starts, so a person editing a search sees /// exactly what the automatic lookup sent rather than a blank box. +/// +/// A tagger writes a joint credit as one string - `The Chainsmokers, Oaks` - and +/// asking MusicBrainz for that string finds nothing at all: the release group is +/// indexed as two credited artists, and a comma inside a quoted phrase is read as +/// loose syntax rather than as part of a name. So each name is asked for on its +/// own instead, and any *one* of them is enough - which matters in both +/// directions, because a tag names exactly what it names: MusicBrainz credits +/// `Annihilation` to `KREAM` alone while the tag reads `KREAM / Korolova`, and +/// requiring every name finds nothing. Choosing between the candidates that +/// leaves is [`best_release_group`]'s business, not the query's. pub(crate) fn default_query(artist: &str, album: &str) -> String { - format!( - "release:\"{}\" AND artist:\"{}\"", - escape_query(album), - escape_query(artist) - ) + let release = format!("release:\"{}\"", escape_query(album)); + let names = artist_names(artist); + match names.len() { + // An album with no artist tag is still worth asking about, and an empty + // `artist:""` clause is not a query MusicBrainz can parse. + 0 => release, + // One name is the ordinary case, and its query does not change. + 1 => format!("{release} AND artist:\"{}\"", escape_query(&names[0])), + _ => { + let credits = names + .iter() + .map(|name| format!("artist:\"{}\"", escape_query(name))) + .collect::>() + .join(" OR "); + format!("{release} AND ({credits})") + } + } +} + +/// The artists one tag claims, however a tagger joined them. +/// +/// `A, B`, `A & B`, `A; B` and `A feat. B` all mean two credited artists, and +/// MusicBrainz holds the credited names rather than the string a tagger wrote. +/// A single name - the ordinary case - comes back whole and alone, so the query +/// for it stays the plain one it has always been. +fn artist_names(artist: &str) -> Vec { + let mut names = Vec::new(); + for piece in artist.split([',', ';', '&']) { + let mut name: Vec<&str> = Vec::new(); + for word in piece.split_whitespace() { + if is_credit_connector(word) { + push_artist_name(&mut names, &mut name); + continue; + } + name.push(word); + } + push_artist_name(&mut names, &mut name); + } + names +} + +/// The words and marks a tagger uses to join credited artists, none of which is +/// part of a name: `feat.` and its spellings, and a slash written on its own. +/// `with` and `x` are deliberately not here, because both are also names - and a +/// slash *inside* a word is part of one too, so `AC/DC` stays one artist while +/// `KREAM / Korolova` is two. +fn is_credit_connector(word: &str) -> bool { + ["feat.", "feat", "ft.", "ft", "featuring", "/"] + .iter() + .any(|connector| word.eq_ignore_ascii_case(connector)) +} + +/// Close off the name being collected, if there is one, and start the next. +fn push_artist_name(names: &mut Vec, name: &mut Vec<&str>) { + let joined = name.join(" "); + name.clear(); + if !joined.is_empty() { + names.push(joined); + } } /// The client every cover lookup uses: the user agent MusicBrainz asks for, and @@ -892,6 +1034,20 @@ fn cover_http_client() -> Result { .map_err(|error| format!("Could not prepare the cover lookup client: {error}")) } +/// The same client, with redirects left alone so one can be read. +/// +/// The Cover Art Archive answers every image path with a redirect into +/// archive.org, and where that redirect points is the only place the file +/// names, and the item they belong to, are stated. +fn cover_http_client_stopping_at_redirects() -> Result { + reqwest::Client::builder() + .user_agent(USER_AGENT) + .timeout(HTTP_TIMEOUT) + .redirect(reqwest::redirect::Policy::none()) + .build() + .map_err(|error| format!("Could not prepare the cover lookup client: {error}")) +} + /// An image URL the NIP will accept, upgrading the archive's own `http://` links. /// /// The Cover Art Archive is inconsistent about the scheme: the identical request @@ -949,14 +1105,32 @@ fn front_image(archive: &CoverArtArchive) -> Option<(String, String, bool)> { } /// Ask the Cover Art Archive what art one release group has. +/// +/// A group the archive will not answer for is not necessarily a record with no +/// art: some answer `500` on their own endpoint while the releases inside them +/// answer perfectly. Measured, not hypothetical +/// (`fa59def2-1fee-4a58-8da6-079204abaf54`, "Love Is Kind" by The Chainsmokers, +/// refuses `/release-group/…` on every request and serves its front cover from +/// `/release/…`), and it is why a record MusicBrainz demonstrably knows about +/// could come back as "no art anywhere". So the releases it holds are asked +/// about instead. async fn archive_lookup( client: &reqwest::Client, mbid: &str, +) -> Result, LookupError> { + match archive_image(client, &format!("release-group/{mbid}")).await { + Err(LookupError::Failed(_)) => archive_lookup_via_releases(client, mbid).await, + answer => answer, + } +} + +/// The archive's answer for one of its own paths, as `(art, thumb, is_front)`. +async fn archive_image( + client: &reqwest::Client, + path: &str, ) -> Result, LookupError> { let response = client - .get(format!( - "https://coverartarchive.org/release-group/{mbid}" - )) + .get(format!("https://coverartarchive.org/{path}")) .send() .await .map_err(|error| LookupError::Failed(format!("Cover Art Archive lookup failed: {error}")))?; @@ -978,6 +1152,157 @@ async fn archive_lookup( Ok(front_image(&archive)) } +/// Look for a group's cover on the releases inside it. +/// +/// A front image is what the caller wants, so the first release that has one +/// wins; a release whose only image is not marked front is kept aside in case +/// nothing better turns up, exactly as [`front_image`] treats a group. +async fn archive_lookup_via_releases( + client: &reqwest::Client, + mbid: &str, +) -> Result, LookupError> { + // This is a second MusicBrainz request for one album, so it waits its turn: + // the caller paces one request per album and knows nothing about this one. + tokio::time::sleep(REQUEST_INTERVAL).await; + let mut url = reqwest::Url::parse(&format!( + "https://musicbrainz.org/ws/2/release-group/{mbid}" + )) + .map_err(|error| LookupError::Failed(format!("could not build the MusicBrainz query: {error}")))?; + url.query_pairs_mut() + .append_pair("inc", "releases") + .append_pair("fmt", "json"); + let response = client + .get(url) + .send() + .await + .map_err(|error| LookupError::Failed(format!("MusicBrainz lookup failed: {error}")))?; + match classify( + response.status(), + retry_after_header(&response), + "MusicBrainz", + false, + ) { + Answer::Success => {} + Answer::NotFound => return Ok(None), + Answer::Throttled { retry_after } => return Err(LookupError::Throttled { retry_after }), + Answer::Failed(message) => return Err(LookupError::Failed(message)), + } + let group: MusicBrainzGroupLookup = response.json().await.map_err(|error| { + LookupError::Failed(format!("MusicBrainz sent something unreadable: {error}")) + })?; + let mut without_a_front = None; + for release in group + .releases + .iter() + .filter(|release| !release.id.is_empty()) + .take(MAX_FALLBACK_RELEASES) + { + match archive_image(client, &format!("release/{}", release.id)).await { + Ok(Some(found)) if found.2 => return Ok(Some(found)), + Ok(Some(found)) => without_a_front = without_a_front.or(Some(found)), + Ok(None) => continue, + // The item is there - MusicBrainz reports its artwork - but the only + // address the Cover Art Archive gives for it answers 500 for every + // file in it. That is a moved item, not a record without art, and it + // is what made a cover that exists look like a cover nobody has. + Err(LookupError::Failed(_)) => { + if let Some(found) = archive_lookup_via_archive_org(client, &release.id).await? { + return Ok(Some(found)); + } + } + Err(refused) => return Err(refused), + } + } + Ok(without_a_front) +} + +/// Where the picture the Cover Art Archive names actually lives now. +/// +/// CAA answers every image path with a redirect into archive.org, and that +/// redirect carries the node and directory the item had when it was ingested. +/// Items move: the release this was written for (`de91dcf0-…`) sits at +/// `ia801509.us.archive.org/2/items/…` while CAA still says +/// `dn711003.ca.archive.org/0/items/…`, and the stale address answers 500 for +/// every file under it - which is how a cover that exists, and that MusicBrainz +/// itself reports as `artwork: true`, became unreachable. `archive.org/metadata` +/// is the live answer, and it lists the files, so the rendition CAA named is +/// asked for where it is now. +/// +/// `/front-1200` is the request on purpose: CAA decides which image is the +/// front one, so this can never publish a back cover as the front, and the +/// 1200-pixel rendition is what a claim wants as its `art`. +async fn archive_lookup_via_archive_org( + client: &reqwest::Client, + release_id: &str, +) -> Result, LookupError> { + let stopping = cover_http_client_stopping_at_redirects().map_err(LookupError::Failed)?; + let response = stopping + .get(format!( + "https://coverartarchive.org/release/{release_id}/front-1200" + )) + .send() + .await + .map_err(|error| LookupError::Failed(format!("Cover Art Archive lookup failed: {error}")))?; + let Some(location) = response + .headers() + .get(reqwest::header::LOCATION) + .and_then(|value| value.to_str().ok()) + else { + return Ok(None); + }; + let Some((item, file)) = archive_org_item_and_file(location) else { + return Ok(None); + }; + let metadata: ArchiveOrgItem = client + .get(format!("https://archive.org/metadata/{item}")) + .send() + .await + .map_err(|error| LookupError::Failed(format!("archive.org lookup failed: {error}")))? + .json() + .await + .map_err(|error| { + LookupError::Failed(format!("archive.org sent something unreadable: {error}")) + })?; + if metadata.server.is_empty() || metadata.dir.is_empty() { + return Ok(None); + } + // The file has to be one the item is actually holding, or the claim would + // name a URL that is no better than the one that failed. + if !metadata.files.iter().any(|listed| listed.name == file) { + return Ok(None); + } + let base = format!("https://{}{}", metadata.server, metadata.dir); + let art = format!("{base}/{file}"); + let smaller = archive_org_thumbnail(&file); + let thumb = metadata + .files + .iter() + .any(|listed| listed.name == smaller) + .then(|| format!("{base}/{smaller}")) + .unwrap_or_default(); + Ok(Some((art, thumb, true))) +} + +/// The item and the file one Cover Art Archive redirect points at. +/// +/// The path is `//items//`. The item is the segment +/// that names a MusicBrainz id, and it is the only part that means anything once +/// the file has moved to another node. +fn archive_org_item_and_file(location: &str) -> Option<(String, String)> { + let path = location.split(['?', '#']).next()?; + let segments: Vec<&str> = path.split('/').filter(|part| !part.is_empty()).collect(); + let item = *segments.iter().find(|part| part.starts_with("mbid-"))?; + let file = *segments.last()?; + // `index.json` describes the images rather than being one, and a path with + // no file after the item is not something to build a URL from. + (!file.is_empty() && file != "index.json").then(|| (item.to_string(), file.to_string())) +} + +/// The 250-pixel rendition of a 1200-pixel one the archive named. +fn archive_org_thumbnail(file: &str) -> String { + file.replace("_thumb1200", "_thumb250") +} + /// Ask MusicBrainz for the release groups one query matches. async fn search_groups( client: &reqwest::Client, @@ -1049,6 +1374,10 @@ pub struct CoverSearchHit { pub front: bool, /// True for the group the automatic lookup would have picked. pub chosen: bool, + /// Why this group has no art to show, when that is not simply "nobody has + /// scanned it": the archive refused, or the network did. Empty when the + /// answer is a picture or a plain absence. + pub note: String, } /// Art a person chose for an album, replacing whatever was there before. @@ -1097,26 +1426,39 @@ impl CoverPublisher { let groups = search_groups(&client, &query) .await .map_err(describe_lookup_error)?; - let chosen = best_release_group(&groups, album).map(|group| group.id.clone()); + let chosen = best_release_group(&groups, album, artist).map(|group| group.id.clone()); let mut hits = Vec::new(); + // Once the archive starts refusing there is nothing to be gained by + // asking it about the rest of the page, but every group still belongs in + // the answer: this is the search a person runs to find out that + // MusicBrainz knows the record they meant. Dropping a row because the + // archive would not talk about it is what made an album that is plainly + // there look absent. + let mut stopped: Option = None; for group in groups { if group.id.is_empty() { continue; } - let (art, thumb, front) = match archive_lookup(&client, &group.id).await { - Ok(Some(found)) => found, - // A group with no art is still worth showing: it is the answer to - // "why does this album keep coming back empty". - Ok(None) => (String::new(), String::new(), false), - Err(error) => { - // Whatever arrived before the throttle is still usable, and - // better than throwing away the search just run. - if hits.is_empty() { - return Err(describe_lookup_error(error)); + let mut note = stopped.clone().unwrap_or_default(); + let mut found = (String::new(), String::new(), false); + if note.is_empty() { + match archive_lookup(&client, &group.id).await { + Ok(Some(answer)) => found = answer, + // A group with no art is still worth showing: it is the + // answer to "why does this album keep coming back empty". + Ok(None) => {} + Err(error) => { + // A refusal applies to the whole page; a broken record + // only to itself. + let refused = matches!(error, LookupError::Throttled { .. }); + note = describe_lookup_error(error); + if refused { + stopped = Some(note.clone()); + } } - break; } - }; + } + let (art, thumb, front) = found; let mut types = group.primary_type.clone(); for extra in &group.secondary_types { if !extra.is_empty() { @@ -1135,6 +1477,7 @@ impl CoverPublisher { art, thumb, front, + note, }); } Ok(hits) @@ -1487,6 +1830,241 @@ mod tests { classify(reqwest::StatusCode::OK, None, "MusicBrainz", false), Answer::Success )); + // A 500 is one record the server cannot answer for, not a request to go + // away and come back. The archive really does this for particular release + // groups; read as back-pressure it made Napstr wait out an album it could + // have resolved, and told the person it had been throttled. + match classify( + reqwest::StatusCode::INTERNAL_SERVER_ERROR, + None, + "Cover Art Archive", + true, + ) { + Answer::Failed(message) => assert!( + message.contains("500"), + "the failure has to name the status: {message}" + ), + _ => panic!("an archive 500 is a failure, not back-pressure"), + } + } + + #[test] + fn a_release_group_lookup_reads_the_releases_it_holds() { + // What the archive fallback asks MusicBrainz for once a release group's + // own art endpoint has refused: the releases inside it, whose own + // endpoints answer. This is the shape measured for + // `fa59def2-1fee-4a58-8da6-079204abaf54` ("Love Is Kind", The Chainsmokers), + // which is the record that could not be looked up at all before. + let group: MusicBrainzGroupLookup = serde_json::from_str( + r#"{"id":"fa59def2-1fee-4a58-8da6-079204abaf54","title":"Love Is Kind", + "releases":[{"id":"de91dcf0-edd8-4e36-b78d-63570bbe718f","title":"Love Is Kind"}]}"#, + ) + .expect("the lookup MusicBrainz sends back has to parse"); + assert_eq!( + group + .releases + .iter() + .map(|release| release.id.as_str()) + .collect::>(), + vec!["de91dcf0-edd8-4e36-b78d-63570bbe718f"] + ); + // A group with no releases at all is an answer, not a parse failure, and + // it has to leave the fallback with nothing rather than an error. + let bare: MusicBrainzGroupLookup = serde_json::from_str(r#"{"id":"x"}"#) + .expect("a group the archive has no releases for still parses"); + assert!(bare.releases.is_empty()); + } + + #[test] + fn a_cover_redirect_names_the_item_that_survives_moving() { + // Measured: this is the redirect the Cover Art Archive gives for the + // release that could not be looked up, and the item it names lives on a + // different node today than the one in the URL. + let (item, file) = archive_org_item_and_file( + "https://dn711003.ca.archive.org/0/items/mbid-de91dcf0-edd8-4e36-b78d-63570bbe718f/mbid-de91dcf0-edd8-4e36-b78d-63570bbe718f-45066579437_thumb1200.jpg", + ) + .expect("a Cover Art Archive redirect names an item and a file"); + assert_eq!(item, "mbid-de91dcf0-edd8-4e36-b78d-63570bbe718f"); + assert_eq!( + file, + "mbid-de91dcf0-edd8-4e36-b78d-63570bbe718f-45066579437_thumb1200.jpg" + ); + // The item name carries the release id, which is why a broken group has + // to be walked down to its releases before this route exists at all. + assert!(item.ends_with("de91dcf0-edd8-4e36-b78d-63570bbe718f")); + // A listing is not an image, and an item this code cannot name is not + // worth building a URL from. + assert!(archive_org_item_and_file( + "https://dn711003.ca.archive.org/0/items/mbid-de91dcf0-x/index.json" + ) + .is_none()); + assert!(archive_org_item_and_file("https://dn711003.ca.archive.org/0/items/x/y.jpg") + .is_none()); + // The 250-pixel rendition of the 1200 the archive named is what a list + // draws, and it is the same file name with one part changed. + assert_eq!( + archive_org_thumbnail( + "mbid-de91dcf0-edd8-4e36-b78d-63570bbe718f-45066579437_thumb1200.jpg" + ), + "mbid-de91dcf0-edd8-4e36-b78d-63570bbe718f-45066579437_thumb250.jpg" + ); + // A name that is not a 1200 at all is left alone rather than mangled. + assert_eq!(archive_org_thumbnail("cover.jpg"), "cover.jpg"); + } + + #[test] + fn archive_org_metadata_says_where_the_item_lives_now() { + let item: ArchiveOrgItem = serde_json::from_str( + r#"{"server":"ia801509.us.archive.org","dir":"/2/items/mbid-de91dcf0-edd8-4e36-b78d-63570bbe718f", + "files":[{"name":"mbid-de91dcf0-edd8-4e36-b78d-63570bbe718f-45066579437.jpg","size":"7095212"}, + {"name":"mbid-de91dcf0-edd8-4e36-b78d-63570bbe718f-45066579437_thumb1200.jpg","size":"179023"}, + {"name":"mbid-de91dcf0-edd8-4e36-b78d-63570bbe718f-45066579437_thumb250.jpg","size":"12265"}]}"#, + ) + .expect("the metadata archive.org sends has to parse"); + // The whole point of asking: CAA says `dn711003.ca.archive.org` with a + // `/0/items` directory, and the item is here instead. + assert_eq!(item.server, "ia801509.us.archive.org"); + assert_eq!(item.dir, "/2/items/mbid-de91dcf0-edd8-4e36-b78d-63570bbe718f"); + let base = format!("https://{}{}", item.server, item.dir); + assert_eq!(base, "https://ia801509.us.archive.org/2/items/mbid-de91dcf0-edd8-4e36-b78d-63570bbe718f"); + assert!(item + .files + .iter() + .any(|file| file.name.ends_with("_thumb1200.jpg"))); + // An answer with no server is not a place, and has to read as nothing + // rather than as a URL with two slashes in it. + let empty: ArchiveOrgItem = serde_json::from_str(r#"{"files":[]}"#).unwrap(); + assert!(empty.server.is_empty() && empty.dir.is_empty()); + } + + #[test] + fn a_joint_credit_is_asked_for_one_name_at_a_time() { + // The tag that could not be looked up at all. MusicBrainz holds this + // record as two credited artists - `The Chainsmokers` and `Oaks` - and + // asking for the joined string finds no release group whatsoever. + assert_eq!( + default_query("The Chainsmokers, Oaks", "Love Is Kind"), + "release:\"Love Is Kind\" AND (artist:\"The Chainsmokers\" OR artist:\"Oaks\")" + ); + // However the tagger wrote the join, it means the same two artists. + for joined in [ + "A & B", + "A; B", + "A, B", + "A feat. B", + "A ft B", + "A FEATURING B", + // A slash written on its own is a join; one inside a word is not. + "A / B", + ] { + assert_eq!( + default_query(joined, "Album"), + "release:\"Album\" AND (artist:\"A\" OR artist:\"B\")", + "{joined}" + ); + } + // The other tag that could not be looked up: a tag that names more + // artists than MusicBrainz credits the record to. Either name is enough, + // and the ranking decides which candidate that leaves. + assert_eq!( + default_query("KREAM / Korolova", "Annihilation"), + "release:\"Annihilation\" AND (artist:\"KREAM\" OR artist:\"Korolova\")" + ); + // `AC/DC` is one artist, so a slash inside a word is left where it is. + assert_eq!( + default_query("AC/DC", "Back in Black"), + "release:\"Back in Black\" AND artist:\"AC/DC\"" + ); + // One artist is the ordinary case, and the query must not grow for it. + assert_eq!( + default_query("Rancid", "And Out Come the Wolves"), + "release:\"And Out Come the Wolves\" AND artist:\"Rancid\"" + ); + // A single crediting whose own name carries punctuation splits into its + // words, and that still matches: MusicBrainz indexes the credited name's + // tokens, so asking for them finds the same record. + assert_eq!( + default_query("Earth, Wind & Fire", "Album"), + "release:\"Album\" AND (artist:\"Earth\" OR artist:\"Wind\" OR artist:\"Fire\")" + ); + // A tag with no artist asks about the release alone rather than sending + // an empty artist clause that cannot be parsed. + assert_eq!(default_query("", "Album"), "release:\"Album\""); + assert_eq!(default_query(" ", "Album"), "release:\"Album\""); + // And nothing a tag says can smuggle an operator into the query. + assert_eq!( + default_query("A\" OR artist:\"B", "Album"), + "release:\"Album\" AND artist:\"A OR artistB\"" + ); + } + + /// The queries two tags produce, against the live MusicBrainz. Both of these + /// are searches that used to find nothing at all. + /// + /// Paced by hand, because MusicBrainz asks for about one request a second and + /// two back to back are throttled. The forms these replaced are measured + /// rather than asserted here - the joined credit `The Chainsmokers, Oaks` + /// answers with no release group, and `KREAM` AND `Korolova` with none either + /// - and `a_joint_credit_is_asked_for_one_name_at_a_time` is what keeps + /// anybody from building them again. + #[tokio::test] + #[ignore = "requires MusicBrainz"] + async fn the_live_query_finds_records_however_the_tag_words_the_credit() { + let _ = rustls::crypto::ring::default_provider().install_default(); + let client = cover_http_client().expect("a lookup client"); + // A joint credit, which MusicBrainz holds as two credited artists. + let joint = default_query("The Chainsmokers, Oaks", "Love Is Kind"); + let groups = search_groups(&client, &joint) + .await + .expect("MusicBrainz has to answer"); + assert!( + groups + .iter() + .any(|group| group.id == "fa59def2-1fee-4a58-8da6-079204abaf54"), + "the query has to find the record the tag means: {joint}" + ); + tokio::time::sleep(REQUEST_INTERVAL * 2).await; + // A credit the tag overstates: MusicBrainz knows no artist called + // Korolova at all, and credits this record to KREAM alone. + let overstated = default_query("KREAM / Korolova", "Annihilation"); + let groups = search_groups(&client, &overstated) + .await + .expect("MusicBrainz has to answer"); + assert!( + groups + .iter() + .any(|group| group.id == "e8fbaa62-ca7d-4a9b-8f6f-44939b3775f5"), + "the query has to find the record the tag means: {overstated}" + ); + } + + /// The live archive, for the one record that could not be looked up at all: + /// its release group answers 500, its release answers 500, and the picture + /// sits on a node neither of those addresses names. + /// + /// Ignored because it needs the network, and run by hand whenever this route + /// is touched: + /// `cargo test --ignored live_archive -- --nocapture` + #[tokio::test] + #[ignore = "requires the Cover Art Archive and archive.org"] + async fn the_live_archive_recovers_a_cover_whose_item_has_moved() { + // reqwest is built without a TLS provider, and the app installs one on + // start-up; nothing in a test binary has done that yet. + let _ = rustls::crypto::ring::default_provider().install_default(); + let client = cover_http_client().expect("a lookup client"); + let found = + archive_lookup_via_archive_org(&client, "de91dcf0-edd8-4e36-b78d-63570bbe718f") + .await + .expect("the archive.org route must answer rather than fail"); + let (art, thumb, front) = found.expect("this release has a front cover"); + assert!(front, "the front route answered, so it is the front cover"); + assert!(art.starts_with("https://"), "{art}"); + assert!(art.contains("mbid-de91dcf0-"), "the item name is the release id: {art}"); + assert!(art.contains("_thumb1200"), "the 1200 rendition is what a claim wants: {art}"); + assert!(thumb.contains("_thumb250"), "the small rendition: {thumb}"); + // Both URLs have to be the current home of the item, not the stale one + // Cover Art Archive redirects are stuck on. + assert!(!art.contains("dn711003"), "the stale node must not be named: {art}"); } #[test] @@ -1696,6 +2274,9 @@ mod tests { primary_type: primary_type.to_string(), score: 100, secondary_types: Vec::new(), + artist_credit: vec![MusicBrainzArtistCredit { + name: "Metallica".to_string(), + }], }; // MusicBrainz returns these in score order, and a single's Cover Art // Archive entry is usually empty, so taking the first title match would @@ -1706,7 +2287,7 @@ mod tests { group("album", "St. Anger", "Album"), ]; assert_eq!( - best_release_group(&groups, "St. Anger").map(|group| group.id.as_str()), + best_release_group(&groups, "St. Anger", "Metallica").map(|group| group.id.as_str()), Some("album") ); @@ -1716,7 +2297,7 @@ mod tests { group("first", "St. Anger", "Single"), ]; assert_eq!( - best_release_group(&singles, "St. Anger").map(|group| group.id.as_str()), + best_release_group(&singles, "St. Anger", "Metallica").map(|group| group.id.as_str()), Some("first") ); @@ -1726,7 +2307,47 @@ mod tests { group("", "St. Anger", "Album"), group("other", "Load", "Album"), ]; - assert!(best_release_group(&unusable, "St. Anger").is_none()); + assert!(best_release_group(&unusable, "St. Anger", "Metallica").is_none()); + } + + #[test] + fn the_first_artist_a_tag_names_beats_a_same_titled_record_by_another() { + let group = |id: &str, title: &str, primary_type: &str, credits: &[&str]| { + MusicBrainzGroup { + id: id.to_string(), + title: title.to_string(), + first_release_date: String::new(), + primary_type: primary_type.to_string(), + score: 100, + secondary_types: Vec::new(), + artist_credit: credits + .iter() + .map(|name| MusicBrainzArtistCredit { + name: name.to_string(), + }) + .collect(), + } + }; + // `Annihilation` is tagged `KREAM / Korolova`, and MusicBrainz credits + // the record to KREAM alone - it knows no artist called Korolova at all. + // The query asks for either name, so the ranking is what has to keep a + // same-titled record by the other one from winning. + let groups = vec![ + group("by-the-other", "Annihilation", "Album", &["Korolova"]), + group("the-record", "Annihilation", "Single", &["KREAM"]), + ]; + assert_eq!( + best_release_group(&groups, "Annihilation", "KREAM / Korolova") + .map(|group| group.id.as_str()), + Some("the-record") + ); + // And with neither name credited - a tag that is simply wrong - nothing + // matches, so the kind of release still decides as it always did. + assert_eq!( + best_release_group(&groups, "Annihilation", "Somebody / Else") + .map(|group| group.id.as_str()), + Some("by-the-other") + ); } #[test] diff --git a/src/lib/CoverPicker.svelte b/src/lib/CoverPicker.svelte index cb1297b..ad96f05 100644 --- a/src/lib/CoverPicker.svelte +++ b/src/lib/CoverPicker.svelte @@ -30,6 +30,8 @@ thumb: string; front: boolean; chosen: boolean; + /** Why there is no art here, when that is not simply "nobody scanned it". */ + note: string; }; let query = ''; @@ -170,6 +172,12 @@ {hit.types || 'unknown type'}{hit.year ? ` · ${hit.year}` : ''} · score {hit.score} {hit.front ? '' : ' · no front image'} + {#if hit.note} + + {hit.note} + {/if} {hit.mbid}
@@ -323,6 +331,11 @@ color: #8d9ab0; margin: 2px 0; } + /* Amber, not the green of the panel's own notice: this is not good news, it + is the reason a row that is obviously the right record has no picture. */ + .art-picker-meta small.art-picker-why { + color: #e0c07a; + } .art-picker-meta code { display: block; overflow: hidden; From ca6403e492a930f1ed9b06715d12c11ec389b8f4 Mon Sep 17 00:00:00 2001 From: StateAntigen Date: Sun, 27 Sep 2026 14:11:38 -0700 Subject: [PATCH 39/42] docs(cover): Apple names its rendition in the artwork URL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The size rule already said to ask for a bounded rendition rather than whatever the source hands back, and named the Cover Art Archive's 250/500/1200 as the example. Apple does the same thing in a less obvious form: the artwork URL ends in the rendition (`…/827568018151.jpg/100x100bb.jpg`) and the file above it is the same one at any size, so the 100-pixel thumbnail a search returns has to be rewritten to `1200x1200bb.jpg` before it is published as `art`. Written down because it is a publisher obligation either way, and the second one is only findable by comparing two URLs. --- NIP-NAPSTR-COVER.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/NIP-NAPSTR-COVER.md b/NIP-NAPSTR-COVER.md index 9ccb097..a06b391 100644 --- a/NIP-NAPSTR-COVER.md +++ b/NIP-NAPSTR-COVER.md @@ -157,6 +157,11 @@ megabytes — so a link to the original makes every consumer on the network pay detail no screen will draw. Where the source offers renditions, ask for one: the Cover Art Archive serves 250, 500 and 1200 pixel versions of every image, and the 1200 is what `art` wants. Note that its `large` alias means 500, so ask by number. +The iTunes catalogue offers the same thing in a less obvious form: its artwork +URLs name a rendition in the last path segment (`…/827568018151.jpg/100x100bb.jpg`) +and the file above it is the same one at any size, so the 100-pixel thumbnail it +returns from a search must be rewritten to `1200x1200bb.jpg` before it is published +as `art`. `thumb` is the counterpart for dense grids: 250 pixels on the long edge fills a shelf tile, a list row, or a blurred backdrop, and it is the rendition a client From 5f80646b9658a65b7c23bf9ded8fcc87ed98bdae Mon Sep 17 00:00:00 2001 From: StateAntigen Date: Sun, 27 Sep 2026 14:11:39 -0700 Subject: [PATCH 40/42] feat(cover): a second source, a list of hosts, a log, and patience MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four things the cover lookup needed, all of them about the same failure: an album that plainly has art somewhere being reported as one that has none. A second source. MusicBrainz and the Cover Art Archive are asked first, because they are open, they carry a stable identifier, and their licence is one a claim can be published under. When they come back empty — the ordinary case for a record nobody has scanned — the iTunes catalogue is asked before this computer decides the album has no art. `Annihilation` by KREAM & Korolova is a release group MusicBrainz knows and a 404 at the archive, and Apple sells the sleeve; no amount of query work on the metadata sources finds a picture that is not there. Apple has no identifier to match on, so a result is only accepted when the album name and one of the credited artist's names agree, and the claim keeps MusicBrainz's MBID while `source` becomes `itunes`. A failure to reach MusicBrainz no longer ends the lookup. It used to, which turned a flaky connection into "no art" while Apple was answering perfectly well. A refusal to be asked (429/503) is still honoured. A timeout is read as back-pressure, not as a verdict. Measured from one connection minutes apart, all `200`: the same search answered in 0.59s, 15.3s, 19.5s and 25.0s, with two requests that never answered at all, while the TCP connect stayed a steady 0.19s. MusicBrainz queues behind its own load rather than refusing, so a request that was sent and never answered is the same message as a 503 — and the same answer: wait, do not write the album off. The deadline for MusicBrainz is 45s (the archive and Apple keep 12s), a new `LookupError::Unanswered` gets the same growing waits a refusal gets, and six in a row end the pass with a reason instead of grinding through an evening. `hold_decision` is separate from the loop so that line is testable. One pace for every MusicBrainz caller. The release-group fallback slept its own interval and the pass loop slept its own, so the two could fire at the same instant — precisely the burst a queue-based limiter punishes. A reservation taken under a lock and slept outside it puts concurrent callers one interval apart. Every attempt is written to `cover_lookup_log`, with the reason, and the reason is what was missing before: `album_art_lookups` kept the verdict and the retry stamp and nothing kept the cause, so an outage and a blank square looked identical in the one place a person was reading. Transport failures now carry reqwest's own source chain too, because `error sending request for url (…)` is the URL without the reason. And `cover_missing` answers "what have I got no `30427` for", worst first: failed, never asked, considered "nobody has this", then resolved-here-but- unsigned. The order matters because a parked failure is the one state that hides work. The one-time migration drops "no art anywhere" verdicts written before there was a second source, since each of those is a verdict on half a search and would hide an album Apple sells for a fortnight. --- src-tauri/src/cover.rs | 269 ++++++- src-tauri/src/cover_publish.rs | 1310 ++++++++++++++++++++++++++++++-- src-tauri/src/lib.rs | 39 + src-tauri/src/network.rs | 22 +- 4 files changed, 1585 insertions(+), 55 deletions(-) diff --git a/src-tauri/src/cover.rs b/src-tauri/src/cover.rs index 2062f2f..f0e70b3 100644 --- a/src-tauri/src/cover.rs +++ b/src-tauri/src/cover.rs @@ -642,7 +642,26 @@ pub(crate) fn initialise_cover_schema(connection: &Connection) -> Result<(), Str album TEXT NOT NULL, noted_at TEXT NOT NULL ); - CREATE INDEX IF NOT EXISTS cover_watch_noted_at ON cover_watch(noted_at);", + CREATE INDEX IF NOT EXISTS cover_watch_noted_at ON cover_watch(noted_at); + -- One line per external lookup attempt, newest last, pruned to the + -- last few hundred. This is the only place the *reason* a lookup + -- failed is written down: `album_art_lookups` remembers the verdict + -- and when it may be asked again, and a verdict with no reason is + -- what made an outage look like an album with no art. Deliberately + -- without a cover-revision trigger: it is a record of what was + -- tried, not a change to what art exists. + CREATE TABLE IF NOT EXISTS cover_lookup_log ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + at TEXT NOT NULL, + cover_key TEXT NOT NULL DEFAULT '', + artist TEXT NOT NULL DEFAULT '', + album TEXT NOT NULL DEFAULT '', + outcome TEXT NOT NULL, + source TEXT NOT NULL DEFAULT '', + message TEXT NOT NULL DEFAULT '' + ); + CREATE INDEX IF NOT EXISTS cover_lookup_log_cover_key + ON cover_lookup_log(cover_key, id DESC);", ) .map_err(|error| error.to_string())?; // Art this computer resolved for itself is half of what it would report, so @@ -1141,6 +1160,254 @@ pub(crate) fn cover_from_art_lookup(lookup: &ArtLookup) -> AlbumCover { } } +// --------------------------------------------------------------------------- +// What the lookups actually did +// --------------------------------------------------------------------------- + +/// How many attempts the log keeps. Enough to cover a long pass over a library, +/// short enough that the table stays a log rather than a database. +pub(crate) const LOOKUP_LOG_LIMIT: i64 = 400; + +/// How much of a failure message is worth keeping. A reqwest error can carry a +/// URL and a cause; a stack of them is not a log line. +const LOOKUP_MESSAGE_LIMIT: usize = 400; + +/// One attempt to write into the log. +pub(crate) struct LookupLogEntry<'a> { + pub key: &'a str, + pub artist: &'a str, + pub album: &'a str, + /// `found`, `none`, `failed` or `throttled`. + pub outcome: &'a str, + /// Which source answered, when one did. + pub source: &'a str, + /// Why, when the answer was not a picture or a plain absence. + pub message: &'a str, +} + +/// One line of the lookup log, as the Covers tab shows it. +#[derive(Debug, Clone, Serialize)] +#[serde(rename_all = "camelCase")] +pub struct CoverLookupLogRow { + /// When, as an RFC 3339 stamp. The window formats it for the reader. + pub at: String, + pub key: String, + pub artist: String, + pub album: String, + pub outcome: String, + pub source: String, + pub message: String, +} + +/// Write one attempt, then drop whatever has fallen off the end. +/// +/// Failing to log must never fail a lookup, so every caller ignores the result. +/// The pruning is by id rather than by time, because the log is a fixed-size +/// ring and not a retention policy. +pub(crate) fn record_lookup_log( + connection: &Connection, + entry: LookupLogEntry<'_>, +) -> Result<(), String> { + connection + .execute( + "INSERT INTO cover_lookup_log(at,cover_key,artist,album,outcome,source,message) + VALUES(?1,?2,?3,?4,?5,?6,?7)", + params![ + Utc::now().to_rfc3339(), + entry.key, + entry.artist, + entry.album, + entry.outcome, + entry.source, + truncate(entry.message, LOOKUP_MESSAGE_LIMIT), + ], + ) + .map_err(|error| error.to_string())?; + connection + .execute( + "DELETE FROM cover_lookup_log + WHERE id <= (SELECT MAX(id) FROM cover_lookup_log) - ?1", + params![LOOKUP_LOG_LIMIT], + ) + .map_err(|error| error.to_string())?; + Ok(()) +} + +/// The newest `limit` attempts, newest first. +pub(crate) fn recent_lookup_log( + connection: &Connection, + limit: usize, +) -> Result, String> { + let mut statement = connection + .prepare( + "SELECT at,cover_key,artist,album,outcome,source,message + FROM cover_lookup_log ORDER BY id DESC LIMIT ?1", + ) + .map_err(|error| error.to_string())?; + let rows = statement + .query_map(params![limit.clamp(1, 1_000) as i64], |row| { + Ok(CoverLookupLogRow { + at: row.get(0)?, + key: row.get(1)?, + artist: row.get(2)?, + album: row.get(3)?, + outcome: row.get(4)?, + source: row.get(5)?, + message: row.get(6)?, + }) + }) + .map_err(|error| error.to_string())?; + rows.collect::, _>>() + .map_err(|error| error.to_string()) +} + +/// The most recent reason recorded for each album, in one pass. +/// +/// Used to explain a list of albums that have no cover: the verdict lives in +/// `album_art_lookups`, and the reason it was reached lives here. +pub(crate) fn last_lookup_messages( + connection: &Connection, +) -> Result, String> { + let mut statement = connection + .prepare( + "SELECT cover_key,message FROM cover_lookup_log + WHERE id IN (SELECT MAX(id) FROM cover_lookup_log + WHERE cover_key <> '' GROUP BY cover_key)", + ) + .map_err(|error| error.to_string())?; + let rows = statement + .query_map([], |row| { + Ok((row.get::<_, String>(0)?, row.get::<_, String>(1)?)) + }) + .map_err(|error| error.to_string())?; + let mut messages = HashMap::new(); + for row in rows { + let (key, message) = row.map_err(|error| error.to_string())?; + messages.insert(key, message); + } + Ok(messages) +} + +/// The verdict held for every album this computer has asked about. +/// +/// One pass, like [`suppressed_art_keys`], because a library-sized list is +/// answered from this: `found`, `none` or `error`. +pub(crate) fn lookup_outcomes(connection: &Connection) -> Result, String> { + let mut statement = connection + .prepare("SELECT cover_key,outcome FROM album_art_lookups") + .map_err(|error| error.to_string())?; + let rows = statement + .query_map([], |row| { + Ok((row.get::<_, String>(0)?, row.get::<_, String>(1)?)) + }) + .map_err(|error| error.to_string())?; + let mut outcomes = HashMap::new(); + for row in rows { + let (key, outcome) = row.map_err(|error| error.to_string())?; + outcomes.insert(key, outcome); + } + Ok(outcomes) +} + +/// Cut a message down to a log line, on a character boundary. +fn truncate(value: &str, limit: usize) -> String { + let trimmed = value.trim(); + if trimmed.chars().count() <= limit { + return trimmed.to_string(); + } + let mut kept: String = trimmed.chars().take(limit).collect(); + kept.push('\u{2026}'); + kept +} + +// --------------------------------------------------------------------------- +// Which hosts art may come from +// --------------------------------------------------------------------------- + +/// The setting holding the domains this computer will take cover art from. +/// +/// Stored as the user typed it, one domain per line, and deliberately *not* a +/// privacy switch: it is a spam and shock filter, and it is the interim answer +/// to art arriving from an address nobody vouched for. The real answer is the +/// trust model the NIP is heading for, where a claim's author is what counts. +pub(crate) const SETTING_ALLOWED_ART_HOSTS: &str = "cover_allowed_art_hosts"; + +/// The domains the user listed, lowercased and without a leading `.` or `*.`. +/// +/// An unreadable database or an absent row reads as an empty list, which means +/// "no restriction". That is what Napstr did before the setting existed, and a +/// filter that nobody configured must not blank a library: this is a +/// convenience, not a boundary, and it is documented that way in the Covers tab. +pub(crate) fn allowed_art_hosts(connection: &Connection) -> Vec { + let stored = connection + .query_row( + "SELECT value FROM settings WHERE key=?1", + [SETTING_ALLOWED_ART_HOSTS], + |row| row.get::<_, String>(0), + ) + .optional() + .unwrap_or(None); + parse_art_hosts(stored.as_deref().unwrap_or("")) +} + +/// One setting's worth of domains, however a person typed them. +/// +/// Commas, semicolons and whitespace all separate, because the setting is a +/// text box and a person will paste a list rather than type one domain. +pub(crate) fn parse_art_hosts(value: &str) -> Vec { + value + .split([',', ';', '\n', '\r', '\t', ' ']) + .map(|host| { + host.trim() + .trim_start_matches("*.") + .trim_start_matches('.') + .to_ascii_lowercase() + }) + .filter(|host| !host.is_empty()) + .collect() +} + +/// The host of an image URL, lowercased, without userinfo or a port. +/// +/// Kept separate from [`allowed_art_hosts`] so both the accepting side (a +/// pasted link) and the displaying side (somebody else's claim) ask the same +/// question of the same string. +pub(crate) fn art_url_host(url: &str) -> Option { + let trimmed = url.trim(); + let rest = trimmed + .strip_prefix("https://") + .or_else(|| trimmed.strip_prefix("http://"))?; + let authority = rest.split(['/', '?', '#']).next()?; + let host = authority + .rsplit('@') + .next()? + .split(':') + .next()? + .trim() + .to_ascii_lowercase(); + (!host.is_empty()).then_some(host) +} + +/// Whether `url` may be used as art when `hosts` is the configured list. +/// +/// An empty list allows any host, because that is the behaviour every existing +/// library was built with. A listed domain covers its subdomains, so +/// `mzstatic.com` admits `is1-ssl.mzstatic.com`, which is where Apple actually +/// serves artwork from. A URL with no host at all — or one that is not an +/// absolute HTTP address — is refused once a list exists, because there is +/// nothing to check it against. +pub(crate) fn art_host_allowed(hosts: &[String], url: &str) -> bool { + if hosts.is_empty() { + return true; + } + let Some(host) = art_url_host(url) else { + return false; + }; + hosts + .iter() + .any(|allowed| host == *allowed || host.ends_with(&format!(".{allowed}"))) +} + /// How many times what this computer would answer about album art has changed. /// /// A companion compares this against the value it last saw: any difference diff --git a/src-tauri/src/cover_publish.rs b/src-tauri/src/cover_publish.rs index 7877eea..1c08b9f 100644 --- a/src-tauri/src/cover_publish.rs +++ b/src-tauri/src/cover_publish.rs @@ -33,10 +33,20 @@ //! What the worker will not do is invent art. A release group is only used when //! its title really matches the album, and a claim is only published when no //! other author already has a winning one. +//! +//! Two art sources, in order. MusicBrainz plus the Cover Art Archive come first, +//! because they are open, they carry a stable identifier, and their licence is +//! the one a claim can be published under. Apple's catalogue is asked only when +//! the archive comes back empty, which is the ordinary case for a record nobody +//! has scanned and a commercial one for a record Apple sells: `Annihilation` by +//! KREAM & Korolova is `404` at the archive and a cover at Apple. That source +//! has no identifier to match on, so it is only ever accepted when the album +//! name and one of the credited artist's names agree, and the picture it returns +//! is published with `mbid` empty rather than with a guess. use crate::cover::{self, ArtLookup, CoverClaimFields}; use crate::network::NetworkService; -use rusqlite::params; +use rusqlite::{params, OptionalExtension}; use serde::{Deserialize, Serialize}; use std::{ collections::HashSet, @@ -63,6 +73,23 @@ const USER_AGENT: &str = concat!( /// *publishing*, which a relay will take as fast as it arrives. const REQUEST_INTERVAL: Duration = Duration::from_millis(1200); const HTTP_TIMEOUT: Duration = Duration::from_secs(12); +/// How long MusicBrainz is given, which is not the same question as how long the +/// archive is given. +/// +/// The public web service queues rather than refuses when it is busy, and the +/// queue is long: measured from one connection, minutes apart and all `200`, the +/// same search answered in 0.59s, 15.3s, 19.5s and 25.0s, with two requests that +/// never answered at all while the TCP connect stayed a steady 0.19s. A network +/// timeout shorter than that is not a round-trip guard, it is a coin toss that +/// throws away answers already on their way — which is exactly what turned a +/// busy service into thirty "failed" albums. +const MUSICBRAINZ_TIMEOUT: Duration = Duration::from_secs(45); +/// How many refusals or unanswered requests in a row end a pass. +/// +/// The waits grow to minutes each, so this is a bound on an evening spent +/// waiting for a service that has stopped answering rather than on politeness: +/// the albums stay pending, and the next pass picks up where this one stopped. +const MAX_CONSECUTIVE_HOLDS: u32 = 6; /// The first wait after a 503 or 429. Doubles with each consecutive refusal, so /// a MusicBrainz outage costs a handful of requests rather than one per album. const THROTTLE_BACKOFF_BASE: Duration = Duration::from_secs(30); @@ -75,6 +102,29 @@ const FAILED_LOOKUP_RETRY_SECONDS: i64 = 15 * 60; /// gives up. A group with art has it on one of the first few, and every release /// walked is another request to the archive. const MAX_FALLBACK_RELEASES: usize = 3; +/// The iTunes catalogue, asked only once the archive has come back empty. +/// +/// It exists for the records a volunteer archive simply never scanned: measured, +/// `Annihilation` by KREAM & Korolova is a release group MusicBrainz knows and +/// the Cover Art Archive answers `404` for, while Apple has the sleeve. Nothing +/// about that is a lookup bug, and no amount of query work on the metadata +/// sources finds a picture that is not there. +const ITUNES_SEARCH_ENDPOINT: &str = "https://itunes.apple.com/search"; +/// How many results one iTunes query may consider. Apple's own relevance order +/// has this record first; the extra rows are what makes a wrong match visible +/// rather than silently accepted. +const ITUNES_RESULT_LIMIT: usize = 25; +/// iTunes publishes no rate limit. This is politeness, not compliance: a library +/// of thousands is thousands of requests in one pass, and being throttled by a +/// service Napstr has no documented agreement with is worse than being slow. +const ITUNES_INTERVAL: Duration = Duration::from_millis(400); +/// The rendition to ask Apple for. Its artwork URLs end in a size, and the whole +/// picture is the same file path at a different one, so a cover that would +/// otherwise be a 100-pixel thumbnail is asked for at 1200. +const ITUNES_FULL_RENDITION: &str = "1200x1200bb.jpg"; +const ITUNES_THUMB_RENDITION: &str = "200x200bb.jpg"; +/// The most albums the missing-cover list will answer with. +const MAX_MISSING_LIST: usize = 2_000; /// The most albums the window may preview at once. const MAX_PREVIEW: usize = 50; /// A safety net rather than a cooldown: the worker is woken by real events, and @@ -86,6 +136,10 @@ const IDLE_RECHECK: Duration = Duration::from_secs(15 * 60); const REPORT_INTERVAL: Duration = Duration::from_millis(150); const SETTING_LOOKUP_EXTERNAL: &str = "cover_lookup_external"; const SETTING_PUBLISH_CLAIMS: &str = "cover_publish_claims"; +/// Set once the first start with a second art source has cleared the answers +/// that were reached without one. See +/// [`forget_answers_from_before_the_second_source`]. +const SETTING_SECOND_SOURCE_STARTED: &str = "cover_second_source_started"; /// Emitted on every meaningful step, so the window shows the worker live. pub const COVER_STATUS_EVENT: &str = "napstr-cover-status"; @@ -163,6 +217,11 @@ pub struct CoverStatus { pub backed_off: usize, /// True when a pass ended early because the switches were turned off. pub stopped: bool, + /// The domains this computer will take art from, one per line. Empty means + /// no restriction, which is what every library had before the setting + /// existed. Carried in the status so the Covers tab can edit it without a + /// second round trip. + pub allowed_art_hosts: String, pub message: String, } @@ -188,6 +247,16 @@ impl CoverPublisher { // session is still in force in this one. An unreadable database means // "off": a privacy switch is never turned on by a failure. let preferences = read_preferences(&db_path).unwrap_or_default(); + let allowed_art_hosts = read_allowed_art_hosts(&db_path).unwrap_or_default(); + // A "nobody has art for this" answer is trusted for a fortnight, and + // every one of them written before there was a second source is a + // verdict on half a search: it would keep an album Apple sells looking + // empty for the rest of the fortnight, which is exactly the report that + // started this. Cleared once. A failure here is not worth refusing to + // start over — the worker simply keeps its old answers. + if let Ok(connection) = crate::open_connection(&db_path) { + let _ = forget_answers_from_before_the_second_source(&connection); + } Arc::new(Self { db_path, network, @@ -198,6 +267,7 @@ impl CoverPublisher { status: Mutex::new(CoverStatus { lookup_external: preferences.lookup_external, publish_claims: preferences.publish_claims, + allowed_art_hosts, message: preferences.describe(), ..CoverStatus::default() }), @@ -281,11 +351,72 @@ impl CoverPublisher { Ok(self.report()) } + /// Record which hosts art may come from, then make the window re-ask. + /// + /// Stored normalised — one domain per line, lowercase, no `*.` — so the box + /// reads back the list that is actually in force rather than the string a + /// person typed, and so the same list cannot look different twice. + pub fn set_allowed_art_hosts(&self, hosts: &str) -> Result { + let normalised = cover::parse_art_hosts(hosts).join("\n"); + { + let connection = crate::open_connection(&self.db_path)?; + connection + .execute( + "INSERT OR REPLACE INTO settings (key, value) VALUES (?1, ?2)", + params![cover::SETTING_ALLOWED_ART_HOSTS, normalised], + ) + .map_err(|error| error.to_string())?; + } + if let Ok(mut status) = self.status.lock() { + status.allowed_art_hosts = normalised.clone(); + status.message = match cover::parse_art_hosts(&normalised).len() { + 0 => "Art from any HTTPS host is accepted".into(), + 1 => "Art is accepted only from 1 host".into(), + count => format!("Art is accepted only from {count} hosts"), + }; + } + Ok(self.report()) + } + /// Ask a running pass to stop. It stops at the next album boundary. pub fn cancel(&self) { self.cancel.store(true, Ordering::SeqCst); } + /// Write one attempt into the lookup log. + /// + /// Logging is diagnostics: a database that cannot be written must not stop + /// an album being resolved, so the result is dropped on purpose. + fn log(&self, candidate: &CoverCandidate, outcome: &str, source: &str, message: &str) { + if let Ok(connection) = crate::open_connection(&self.db_path) { + let _ = cover::record_lookup_log( + &connection, + cover::LookupLogEntry { + key: &candidate.key, + artist: &candidate.artist, + album: &candidate.album, + outcome, + source, + message, + }, + ); + } + } + + /// The newest lookup attempts, for the Covers tab. + pub fn lookup_log(&self, limit: usize) -> Result, String> { + let connection = crate::open_connection(&self.db_path)?; + cover::recent_lookup_log(&connection, limit) + } + + /// Every album this computer holds that no cover answers, worst first. + /// + /// See [`missing_albums`] for what "worst" means. + pub fn missing_albums(&self, limit: usize) -> Result, String> { + let connection = crate::open_connection(&self.db_path)?; + missing_albums(&connection, limit) + } + /// What the worker would act on right now, capped for display. pub fn preview(&self, limit: usize) -> Result, String> { let connection = crate::open_connection(&self.db_path)?; @@ -382,6 +513,8 @@ impl CoverPublisher { }; let mut throttle_streak = 0u32; + // Why this pass ended early, when it did. Empty means it finished. + let mut stop = String::new(); for candidate in &candidates { // The switches may have been turned off, or a stop asked for, while // this pass was running. @@ -411,21 +544,36 @@ impl CoverPublisher { Considered::NoArt => self.tally(|status| status.no_art += 1), } } - Err(LookupError::Throttled { retry_after }) => { + // A refusal and an unanswered request are the same message from + // the service — slow down — so they get the same answer: wait, + // and do not write the album off. Only the words differ, and the + // words matter when a person is reading them. + Err(error @ (LookupError::Throttled { .. } | LookupError::Unanswered { .. })) => { + let retry_after = match &error { + LookupError::Throttled { retry_after } => *retry_after, + _ => None, + }; throttle_streak += 1; - let delay = throttle_delay(throttle_streak, retry_after); + let (delay, given_up) = hold_decision(throttle_streak, retry_after); // Remember the pause, so a later pass does not walk straight // back into the same refusal. self.park(&candidate.key, delay); if let Ok(mut status) = self.status.lock() { status.backed_off += 1; - status.message = format!( - "MusicBrainz asked Napstr to slow down; waiting {}s", - delay.as_secs() - ); + status.message = + format!("{}; waiting {}s", describe_lookup_error(error), delay.as_secs()); } self.report(); tokio::time::sleep(delay).await; + // Waiting is polite; waiting all evening is not. The albums + // left stay pending, so the next pass resumes rather than + // restarts, and nothing is written off. + if given_up { + stop = format!( + "MusicBrainz has not answered {throttle_streak} times in a row; stopping this pass. Everything left is still waiting." + ); + break; + } } Err(LookupError::Failed(message)) => { // A transient fault is not worth retrying immediately, so @@ -442,7 +590,7 @@ impl CoverPublisher { } self.tick(); } - self.finish(String::new()); + self.finish(stop); } /// End a pass: state what happened, and say plainly when nothing was ready. @@ -513,24 +661,62 @@ impl CoverPublisher { { return Ok(Considered::AlreadyCovered); } - match resolve(client, candidate).await? { - Some(resolution) => { + match resolve(client, candidate).await { + Ok(Some(resolution)) => { + self.log(candidate, "found", &resolution.source, ""); self.record(&candidate.key, Some(&resolution))?; resolution } // MusicBrainz has nothing today. If this computer already // holds art for the album, keep it: one unhelpful answer is // no reason to throw away a working picture. - None => match self.stored_art(&candidate.key)? { - Some(previous) => { - self.record(&candidate.key, Some(&previous))?; - previous + Ok(None) => { + self.log(candidate, "none", "", "no art in any source"); + match self.stored_art(&candidate.key)? { + Some(previous) => { + self.record(&candidate.key, Some(&previous))?; + previous + } + None => { + self.record(&candidate.key, None)?; + return Ok(Considered::NoArt); + } } - None => { - self.record(&candidate.key, None)?; - return Ok(Considered::NoArt); - } - }, + } + // The reason is written down here, once, because this is the + // only moment it exists: the cache keeps the verdict and the + // parking, and nothing else keeps the cause. + Err(LookupError::Throttled { retry_after }) => { + self.log( + candidate, + "throttled", + "", + &match retry_after { + Some(delay) => format!("asked to wait {}s", delay.as_secs()), + None => "asked to wait".into(), + }, + ); + return Err(LookupError::Throttled { retry_after }); + } + // Logged as its own outcome, because "the service is busy" + // and "this album is wrong" are different things to see in a + // list, and only one of them is the album's problem. + Err(error @ LookupError::Unanswered { waited }) => { + self.log( + candidate, + "slow", + "", + &format!("no answer within {}s", waited.as_secs()), + ); + return Err(error); + } + Err(LookupError::Failed(message)) => { + // Parking is the pass loop's business: it happens once, + // for every failure including the ones this function + // never sees. + self.log(candidate, "failed", "", &message); + return Err(LookupError::Failed(message)); + } } } }; @@ -624,6 +810,94 @@ impl CoverPublisher { } } +/// An album this computer holds that no cover answers, and what is known about +/// why. +#[derive(Debug, Clone, Serialize)] +#[serde(rename_all = "camelCase")] +pub struct CoverGap { + pub key: String, + pub artist: String, + pub album: String, + pub tracks: usize, + /// `failed`, `not_looked_up`, `no_art` or `resolved_here`. + pub state: String, + /// Where the album came from: `library` for one this computer holds. + pub source: String, + /// The most recent reason recorded for it, when there is one. + pub note: String, +} + +/// Every album this computer holds that no `30427` answers, worst first. +/// +/// "Worst" is `failed`, then never asked, then a considered "nobody has this", +/// then one this computer resolved for itself and nobody has signed. A failure +/// comes first because it is the state that hides work: an album parked after a +/// transport error looks exactly like an album nobody has art for, and the order +/// is the only thing that tells them apart in a list. +/// +/// A claim from anybody takes an album off the list, including another author's: +/// the question is which albums have no kind `30427`, not which ones this +/// computer signed. +fn missing_albums(connection: &rusqlite::Connection, limit: usize) -> Result, String> { + let claimed = stored_cover_keys(connection)?; + let outcomes = cover::lookup_outcomes(connection)?; + let messages = cover::last_lookup_messages(connection)?; + let mut gaps = Vec::new(); + for (key, artist, album, tracks, source) in library_albums(connection)? { + if claimed.contains(&key) { + continue; + } + let outcome = outcomes.get(&key).map(String::as_str).unwrap_or(""); + let state = match outcome { + "error" => "failed", + "none" => "no_art", + // This computer holds a picture for it and nobody has signed a + // claim: the album is not missing art, it is missing a `30427`, and + // that is a publishing decision rather than a lookup to make. + "found" => "resolved_here", + // Never asked. + _ => "not_looked_up", + }; + gaps.push(CoverGap { + note: messages.get(&key).cloned().unwrap_or_default(), + key, + artist, + album, + tracks, + state: state.into(), + source: source.to_string(), + }); + } + gaps.sort_by(|left, right| { + ( + rank_of_state(&left.state), + left.artist.to_lowercase(), + left.album.to_lowercase(), + ) + .cmp(&( + rank_of_state(&right.state), + right.artist.to_lowercase(), + right.album.to_lowercase(), + )) + }); + gaps.truncate(limit.clamp(1, MAX_MISSING_LIST)); + Ok(gaps) +} + +/// How badly an album wants looking at, for sorting. +/// +/// A failure first, because it is the state that hides work. "Already resolved +/// here" last, because it is the one state where the reader is looking at a +/// publishing decision rather than missing art. +fn rank_of_state(state: &str) -> u8 { + match state { + "failed" => 0, + "not_looked_up" => 1, + "no_art" => 2, + _ => 3, + } +} + /// What one album produced. #[derive(Debug, Clone, Copy, PartialEq, Eq)] enum Considered { @@ -642,10 +916,63 @@ enum Considered { enum LookupError { /// MusicBrainz or the archive asked this client to slow down. Throttled { retry_after: Option }, + /// The request was sent and no answer came back in time. + /// + /// A separate case from failure because it is not an answer about the album: + /// a service that queues behind a busy queue is telling the client the same + /// thing a 503 is, and the client's response — wait, do not write the album + /// off — is the same too. Only the words are different, and the words matter + /// when a person is reading them. + Unanswered { waited: Duration }, /// Anything else that stopped this album being resolved. Failed(String), } +/// A transport failure, with the cause reqwest keeps inside it. +/// +/// `reqwest::Error`'s own Display is `error sending request for url (…)` and +/// nothing else. That is what a person was shown a screenful of while +/// MusicBrainz was unreachable: the URL they already knew, and no reason. The +/// reason is in the source chain — a DNS failure, a TLS failure, a connection +/// reset, a timeout — and it is the only part worth reading, so it is appended +/// rather than replaced. +fn request_error(what: &str, error: &reqwest::Error) -> LookupError { + LookupError::Failed(format!("{what}: {}", describe_causes(error))) +} + +/// Every cause in an error's chain, outermost first, without duplicates. +fn describe_causes(error: &(dyn std::error::Error + 'static)) -> String { + let mut parts: Vec = Vec::new(); + let mut current = Some(error); + while let Some(step) = current { + let text = step.to_string(); + // A cause that repeats its parent says nothing new. + if !text.trim().is_empty() && parts.last() != Some(&text) { + parts.push(text); + } + current = step.source(); + } + parts.join(" \u{2192} ") +} + +/// A MusicBrainz request that failed, with a timeout read as back-pressure. +/// +/// A request that was sent and never answered is not an answer about the album. +/// MusicBrainz queues behind its own load, so the honest reading of a timeout is +/// "the service is busy", and the honest response is to wait rather than to +/// write the album off — see [`MUSICBRAINZ_TIMEOUT`] for what was measured. +fn musicbrainz_error(error: &reqwest::Error) -> LookupError { + if error.is_timeout() { + return LookupError::Unanswered { + waited: MUSICBRAINZ_TIMEOUT, + }; + } + LookupError::Failed(format!( + "MusicBrainz lookup failed: {}", + describe_causes(error) + )) +} + /// How an HTTP status should be read. enum Answer { Success, @@ -690,6 +1017,44 @@ fn classify( Answer::Failed(format!("{host} answered {status}")) } +/// What to do about one album that could not be looked up, and whether it ends +/// the pass. +/// +/// Separate from the pass loop so the policy is testable without a publisher, a +/// database or a network: the waits grow to minutes, and a pass that grinds +/// through an evening of them is worse than one that stops and says why. +fn hold_decision(streak: u32, retry_after: Option) -> (Duration, bool) { + ( + throttle_delay(streak, retry_after), + streak >= MAX_CONSECUTIVE_HOLDS, + ) +} + +/// Reserve the next MusicBrainz slot and wait for it. +/// +/// The pace lives here rather than at each call site because three of them make +/// MusicBrainz requests — the automatic search, the release-group fallback and +/// the manual tool — and callers that each wait their own turn still collide +/// with each other. A collision is precisely the burst a queue-based limiter +/// punishes, and the punishment is the delay this exists to avoid. +/// +/// The reservation is taken under the lock and the sleeping happens outside it, +/// so concurrent callers come out one interval apart rather than all at once. +static MUSICBRAINZ_PACE: std::sync::OnceLock>> = + std::sync::OnceLock::new(); + +async fn pace_musicbrainz() { + let pace = MUSICBRAINZ_PACE.get_or_init(|| tokio::sync::Mutex::new(None)); + let now = Instant::now(); + let slot = { + let mut next = pace.lock().await; + let slot = next.map_or(now, |allowed| allowed.max(now)); + *next = Some(slot + REQUEST_INTERVAL); + slot + }; + tokio::time::sleep(slot.saturating_duration_since(Instant::now())).await; +} + /// `Retry-After` is either delta-seconds or an HTTP date. Only the numeric form /// is honoured; a date is treated as "no hint" and the exponential floor wins. fn parse_retry_after(value: &str) -> Option { @@ -883,15 +1248,94 @@ struct CoverArtThumbnails { } /// Ask MusicBrainz for the release group, then the Cover Art Archive for its -/// front image. `Ok(None)` is a considered answer: this album has no art there. +/// front image, then Apple's catalogue when the archive comes back empty. +/// `Ok(None)` is a considered answer: nobody this computer can reach has art. /// /// A 503 or 429 comes back as [`LookupError::Throttled`] rather than a plain /// failure, because the caller's correct response is to wait, not to give up or /// to try the next album immediately. +/// +/// The identifier is MusicBrainz's wherever there is one, even when the picture +/// comes from Apple: the release group is what was identified, and a claim that +/// names it is one a reader can follow. Where MusicBrainz has no matching group +/// at all, the claim is published with `mbid` empty rather than with a guess. async fn resolve( client: &reqwest::Client, candidate: &CoverCandidate, ) -> Result, LookupError> { + let mut lookup = ArtLookup { + key: candidate.key.clone(), + source: "musicbrainz".into(), + ..ArtLookup::default() + }; + // Only a release group whose title really is this album is worth publishing: + // a cover on the wrong record is worse than a blank square. Among those, + // the album itself is the one a music library means. + // + // A MusicBrainz that cannot be reached is not an answer about this album. It + // used to end the lookup, which turned a flaky connection into "no art" — + // the second source is a different service and may well be reachable, so a + // transport failure is carried past this step rather than returned from it. + // A refusal to be asked at all (429/503) is honoured, because that is a + // request to stop asking. + let mut unreachable = String::new(); + let identified = match resolve_release_group(client, candidate).await { + Ok(group) => group, + Err(LookupError::Failed(message)) => { + unreachable = message; + None + } + Err(refused) => return Err(refused), + }; + if let Some(group) = identified { + lookup.mbid = group.id.clone(); + lookup.year = group.first_release_date.chars().take(4).collect(); + lookup.collection = group.title.clone(); + if let Some((art, thumb, _)) = archive_lookup(client, &group.id).await? { + lookup.art = art; + lookup.thumb = thumb; + return Ok(Some(lookup)); + } + } + // The archive has nothing for this record — the ordinary case for a record + // nobody has scanned, and the case a volunteer archive can never fix by + // being asked differently. Apple sells a great many of them, so it is asked + // before this computer decides the album has no art at all. + let cover = itunes_lookup(client, &candidate.artist, &candidate.album).await; + // Nothing from Apple *and* nothing from MusicBrainz is not a considered + // answer: one of the two was never really asked. Reported as a failure, so + // the album is retried in a quarter of an hour rather than written off for a + // fortnight. + let Some(cover) = (match cover { + Ok(found) => found, + Err(LookupError::Failed(_)) if !unreachable.is_empty() => None, + Err(error) => return Err(error), + }) else { + return if unreachable.is_empty() { + Ok(None) + } else { + Err(LookupError::Failed(unreachable)) + }; + }; + lookup.art = cover.art; + lookup.thumb = cover.thumb; + lookup.source = "itunes".into(); + // Apple's own name and year are only used where MusicBrainz had neither: + // seeded from a real release group, that group's title is the better answer. + if lookup.collection.is_empty() { + lookup.collection = cover.title; + } + if lookup.year.is_empty() { + lookup.year = cover.year; + } + Ok(Some(lookup)) +} + +/// The release group MusicBrainz holds for this album, if it holds one. +async fn resolve_release_group( + client: &reqwest::Client, + candidate: &CoverCandidate, +) -> Result, LookupError> { let query = default_query(&candidate.artist, &candidate.album); // Encoded through `Url` rather than `RequestBuilder::query`, which reqwest // 0.13 puts behind its `query` feature. This needs no extra dependency and @@ -903,11 +1347,13 @@ async fn resolve( .append_pair("query", &query) .append_pair("fmt", "json") .append_pair("limit", "10"); + pace_musicbrainz().await; let search = client .get(search_url) + .timeout(MUSICBRAINZ_TIMEOUT) .send() .await - .map_err(|error| LookupError::Failed(format!("MusicBrainz lookup failed: {error}")))?; + .map_err(|error| musicbrainz_error(&error))?; match classify( search.status(), retry_after_header(&search), @@ -925,27 +1371,7 @@ async fn resolve( .json() .await .map_err(|error| LookupError::Failed(format!("MusicBrainz sent something unreadable: {error}")))?; - // Only a release group whose title really is this album is worth publishing: - // a cover on the wrong record is worse than a blank square. Among those, - // the album itself is the one a music library means. - let Some(group) = best_release_group(&found.release_groups, &candidate.album, &candidate.artist) - .cloned() - else { - return Ok(None); - }; - - let Some((art, thumb, _)) = archive_lookup(client, &group.id).await? else { - return Ok(None); - }; - Ok(Some(ArtLookup { - key: candidate.key.clone(), - art, - thumb, - mbid: group.id, - year: group.first_release_date.chars().take(4).collect(), - collection: group.title, - source: "musicbrainz".into(), - })) + Ok(best_release_group(&found.release_groups, &candidate.album, &candidate.artist).cloned()) } /// The MusicBrainz query Napstr asks for an album. @@ -1133,7 +1559,7 @@ async fn archive_image( .get(format!("https://coverartarchive.org/{path}")) .send() .await - .map_err(|error| LookupError::Failed(format!("Cover Art Archive lookup failed: {error}")))?; + .map_err(|error| request_error("Cover Art Archive lookup failed", &error))?; match classify( response.status(), retry_after_header(&response), @@ -1161,9 +1587,9 @@ async fn archive_lookup_via_releases( client: &reqwest::Client, mbid: &str, ) -> Result, LookupError> { - // This is a second MusicBrainz request for one album, so it waits its turn: - // the caller paces one request per album and knows nothing about this one. - tokio::time::sleep(REQUEST_INTERVAL).await; + // This is a second MusicBrainz request for one album, so it waits its turn — + // [`pace_musicbrainz`] is what makes that true, and it is why there is no + // sleep here: two sleeps would pace this call twice and nothing else once. let mut url = reqwest::Url::parse(&format!( "https://musicbrainz.org/ws/2/release-group/{mbid}" )) @@ -1171,11 +1597,13 @@ async fn archive_lookup_via_releases( url.query_pairs_mut() .append_pair("inc", "releases") .append_pair("fmt", "json"); + pace_musicbrainz().await; let response = client .get(url) + .timeout(MUSICBRAINZ_TIMEOUT) .send() .await - .map_err(|error| LookupError::Failed(format!("MusicBrainz lookup failed: {error}")))?; + .map_err(|error| musicbrainz_error(&error))?; match classify( response.status(), retry_after_header(&response), @@ -1242,7 +1670,7 @@ async fn archive_lookup_via_archive_org( )) .send() .await - .map_err(|error| LookupError::Failed(format!("Cover Art Archive lookup failed: {error}")))?; + .map_err(|error| request_error("Cover Art Archive lookup failed", &error))?; let Some(location) = response .headers() .get(reqwest::header::LOCATION) @@ -1257,7 +1685,7 @@ async fn archive_lookup_via_archive_org( .get(format!("https://archive.org/metadata/{item}")) .send() .await - .map_err(|error| LookupError::Failed(format!("archive.org lookup failed: {error}")))? + .map_err(|error| request_error("archive.org lookup failed", &error))? .json() .await .map_err(|error| { @@ -1283,6 +1711,196 @@ async fn archive_lookup_via_archive_org( Ok(Some((art, thumb, true))) } +// --------------------------------------------------------------------------- +// The second source: Apple's catalogue +// --------------------------------------------------------------------------- + +/// A front cover from the iTunes catalogue, with the name it was filed under. +#[derive(Debug, Clone, Default)] +struct ItunesCover { + /// The 1200-pixel rendition, or whatever Apple named if it is not the + /// usual shape. + art: String, + thumb: String, + /// The album's own name, with Apple's `- Single` marker removed. + title: String, + /// The year Apple files it under. + year: String, + /// Apple's collection id, so a person can be shown where the art came from. + collection_id: String, +} + +impl ItunesCover { + /// The id this hit is keyed by in the picker, and never a MusicBrainz id: + /// publishing Apple's collection id as an MBID would be a lie a reader + /// could follow somewhere real. + fn id(&self) -> String { + format!("itunes:{}", self.collection_id) + } +} + +#[derive(Deserialize)] +struct ItunesSearch { + #[serde(default)] + results: Vec, +} + +#[derive(Deserialize, Clone, Default)] +struct ItunesResult { + #[serde(default, rename = "collectionId")] + collection_id: i64, + #[serde(default, rename = "collectionName")] + collection_name: String, + #[serde(default, rename = "artistName")] + artist_name: String, + #[serde(default, rename = "artworkUrl100")] + artwork_url: String, + #[serde(default, rename = "releaseDate")] + release_date: String, +} + +impl ItunesResult { + /// The album's own name, without the marker Apple appends to a single. + /// + /// Apple files a one-track release as `Annihilation - Single`, and a + /// comparison against a tag reading `Annihilation` fails on that suffix + /// alone. `collectionType` does not help: it is `Album` for a single too. + fn title(&self) -> String { + let name = self.collection_name.trim(); + for suffix in [" - Single", " - EP"] { + if name.len() > suffix.len() + && name.to_ascii_lowercase().ends_with(&suffix.to_ascii_lowercase()) + { + return name[..name.len() - suffix.len()].trim().to_string(); + } + } + name.to_string() + } + + fn cover(&self) -> ItunesCover { + ItunesCover { + art: itunes_rendition(&self.artwork_url, ITUNES_FULL_RENDITION), + thumb: itunes_rendition(&self.artwork_url, ITUNES_THUMB_RENDITION), + title: self.title(), + year: self.release_date.chars().take(4).collect(), + collection_id: self.collection_id.to_string(), + } + } +} + +/// Apple's artwork URL at the size a cover wants. +/// +/// Every artwork URL ends in the rendition, `…/827568018151.jpg/100x100bb.jpg`, +/// and the file path above it is the same one whatever size is asked for. Only +/// that shape is rewritten: an address Apple formats some other way is left +/// exactly as it was rather than mangled into something that does not resolve. +fn itunes_rendition(artwork_url: &str, rendition: &str) -> String { + let trimmed = artwork_url.trim(); + if trimmed.is_empty() { + return String::new(); + } + // Apple's samples are all `https://`, but a claim may only carry HTTPS, so + // a `http://` one is repaired rather than discarded. + let upgraded = match trimmed.strip_prefix("http://") { + Some(rest) => format!("https://{rest}"), + None if trimmed.starts_with("https://") => trimmed.to_string(), + None => return String::new(), + }; + let Some(cut) = upgraded.rfind('/') else { + return upgraded; + }; + let last = &upgraded[cut + 1..]; + if !last.ends_with("bb.jpg") { + return upgraded; + } + format!("{}/{rendition}", &upgraded[..cut]) +} + +/// The result a music library means, among what Apple offered. +/// +/// There is no identifier to match on, so both halves of the name have to agree: +/// a title that is this album's, and an artist credit naming somebody the tag +/// names. `Annihilation` has namesakes, and returning the wrong one puts a +/// stranger's sleeve on a record, which is worse than returning nothing. +fn best_itunes_result<'a>( + results: &'a [ItunesResult], + album: &str, + artist: &str, +) -> Option<&'a ItunesResult> { + let wanted = artist_names(artist); + results + .iter() + .filter(|result| !result.artwork_url.trim().is_empty() && alike(&result.title(), album)) + .enumerate() + .min_by_key(|(position, result)| { + let credited = artist_names(&result.artist_name); + ( + // The tag names the artist; a result credited to somebody else + // is a different record that happens to share the name. + u8::from( + !wanted + .iter() + .any(|name| credited.iter().any(|credit| alike(credit, name))), + ), + // An exact title beats Apple's longer edition names, so + // `Annihilation` wins over `Annihilation (Remixes)`. + u8::from(fold_title(&result.title()) != fold_title(album)), + // Anything still tied keeps Apple's own relevance order. + *position, + ) + }) + .map(|(_, result)| result) +} + +/// Ask the iTunes catalogue for this album's art. +/// +/// `Ok(None)` is a considered answer — Apple does not sell this record, or what +/// it offered named a different artist — and the caller treats it exactly like +/// the archive's own "nobody has scanned this". +async fn itunes_lookup( + client: &reqwest::Client, + artist: &str, + album: &str, +) -> Result, LookupError> { + // Paced on its own interval, which does not count against MusicBrainz's: the + // archive has already been asked by the time this runs. + tokio::time::sleep(ITUNES_INTERVAL).await; + let mut url = reqwest::Url::parse(ITUNES_SEARCH_ENDPOINT) + .map_err(|error| LookupError::Failed(format!("could not build the iTunes query: {error}")))?; + // The name, not a Lucene query: Apple's search is a plain text match, so the + // album and the artist are simply offered to it together. + let term = format!("{} {}", album.trim(), artist.trim()); + url.query_pairs_mut() + .append_pair("term", term.trim()) + .append_pair("entity", "album") + .append_pair("limit", &ITUNES_RESULT_LIMIT.to_string()); + let response = client + .get(url) + .send() + .await + .map_err(|error| request_error("iTunes lookup failed", &error))?; + match classify( + response.status(), + retry_after_header(&response), + "iTunes", + // A 404 from a search endpoint means it moved, not that this album has + // no art, so it must not be remembered as a considered answer. + false, + ) { + Answer::Success => {} + Answer::NotFound => return Ok(None), + Answer::Throttled { retry_after } => return Err(LookupError::Throttled { retry_after }), + Answer::Failed(message) => return Err(LookupError::Failed(message)), + } + let found: ItunesSearch = response + .json() + .await + .map_err(|error| LookupError::Failed(format!("iTunes sent something unreadable: {error}")))?; + Ok(best_itunes_result(&found.results, album, artist) + .map(ItunesResult::cover) + .filter(|cover| !cover.art.is_empty())) +} + /// The item and the file one Cover Art Archive redirect points at. /// /// The path is `//items//`. The item is the segment @@ -1314,11 +1932,13 @@ async fn search_groups( .append_pair("query", query) .append_pair("fmt", "json") .append_pair("limit", "10"); + pace_musicbrainz().await; let response = client .get(url) + .timeout(MUSICBRAINZ_TIMEOUT) .send() .await - .map_err(|error| LookupError::Failed(format!("MusicBrainz lookup failed: {error}")))?; + .map_err(|error| musicbrainz_error(&error))?; match classify( response.status(), retry_after_header(&response), @@ -1340,6 +1960,10 @@ async fn search_groups( fn describe_lookup_error(error: LookupError) -> String { match error { LookupError::Failed(message) => message, + LookupError::Unanswered { waited } => format!( + "MusicBrainz did not answer within {}s \u{2014} the service is busy, so this album is still waiting", + waited.as_secs() + ), LookupError::Throttled { retry_after } => match retry_after { Some(delay) => format!( "MusicBrainz asked Napstr to slow down \u{2014} try again in about {} seconds", @@ -1354,7 +1978,7 @@ fn describe_lookup_error(error: LookupError) -> String { // Choosing art by hand // --------------------------------------------------------------------------- -/// One release group MusicBrainz offered, with the art the archive holds for it. +/// One candidate for this album's art, from either of the two sources. /// /// The automatic choice ranks by primary type and an exact title, which is right /// far more often than not but cannot know that a person meant a different @@ -1362,7 +1986,14 @@ fn describe_lookup_error(error: LookupError) -> String { #[derive(Debug, Clone, Serialize)] #[serde(rename_all = "camelCase")] pub struct CoverSearchHit { + /// What the row is keyed by in the window. It is *not* always an MBID: an + /// iTunes row is keyed by Apple's collection id, because there is no + /// MusicBrainz release group for it and inventing one would be a lie. + pub id: String, + /// The release-group MBID, empty when the art is not from MusicBrainz. pub mbid: String, + /// `musicbrainz` or `itunes`. + pub source: String, pub title: String, /// The primary type, with any `Live`/`Compilation` markers after it. pub types: String, @@ -1392,6 +2023,15 @@ pub struct CoverManualPick { /// The release group title, published as `collection`. pub title: String, pub year: String, + /// `hit` for a row this computer offered, `url` for a link a person typed. + /// A typed link is the one case where the art domain list is enforced, so a + /// window that omits this field is treated as a hit rather than as a link. + #[serde(default = "default_pick_source")] + pub source: String, +} + +fn default_pick_source() -> String { + "hit".into() } /// What applying a pick did. @@ -1467,7 +2107,9 @@ impl CoverPublisher { } hits.push(CoverSearchHit { chosen: chosen.as_deref() == Some(group.id.as_str()), + id: group.id.clone(), mbid: group.id, + source: "musicbrainz".into(), title: group.title, types: types .trim_matches(|character: char| character == ' ' || character == '\u{b7}') @@ -1480,6 +2122,47 @@ impl CoverPublisher { note, }); } + // The second source, always asked, because the question this tool + // answers is "what art exists for this album" and not "what does + // MusicBrainz think". Apple's answer is matched on the album and artist + // names alone, so it is offered with no MBID and is never marked as the + // automatic choice: the automatic lookup only falls back to it when the + // archive is empty, which is not something this list can know. + match itunes_lookup(&client, artist, album).await { + Ok(Some(cover)) => hits.push(CoverSearchHit { + chosen: false, + id: cover.id(), + mbid: String::new(), + source: "itunes".into(), + title: cover.title, + types: "matched by name".into(), + year: cover.year, + score: 0, + art: cover.art, + thumb: cover.thumb, + front: true, + note: String::new(), + }), + Ok(None) => {} + // The second source failing is worth a row of its own rather than a + // silently shorter list: "Apple was not reachable" and "Apple does + // not sell this" look identical in a list that only shows what + // worked, and the first one is worth pressing Fire again for. + Err(error) => hits.push(CoverSearchHit { + chosen: false, + id: "itunes:unavailable".into(), + mbid: String::new(), + source: "itunes".into(), + title: album.trim().to_string(), + types: "not reachable".into(), + year: String::new(), + score: 0, + art: String::new(), + thumb: String::new(), + front: false, + note: describe_lookup_error(error), + }), + } Ok(hits) } @@ -1488,6 +2171,13 @@ impl CoverPublisher { /// The key is computed from display metadata exactly as the automatic lookup /// computes it, so the `d` tag matches what a lookup would have produced and /// the claim is found again by anybody filtering on that key. + /// + /// A link a person typed is the one path where art arrives from an address + /// nobody vouched for, so it is also the one path the art domain list gates. + /// The list is a filter, not a boundary — the person configuring it is the + /// person it protects — and it deliberately does not apply to art this + /// computer looked up itself, which would break a list that omits the + /// sources Napstr uses on purpose. pub async fn apply_pick(&self, pick: CoverManualPick) -> Result { let key = cover::cover_key(&pick.artist, &pick.album) .ok_or("this album has no addressable cover key")?; @@ -1495,6 +2185,16 @@ impl CoverPublisher { if !art.starts_with("https://") { return Err("a cover needs an HTTPS image URL".into()); } + if pick.source.eq_ignore_ascii_case("url") { + let connection = crate::open_connection(&self.db_path)?; + let hosts = cover::allowed_art_hosts(&connection); + if !cover::art_host_allowed(&hosts, &art) { + let host = cover::art_url_host(&art).unwrap_or_else(|| art.clone()); + return Err(format!( + "{host} is not on the list of art domains this computer accepts. Add it in the Covers tab first." + )); + } + } let lookup = ArtLookup { key: key.clone(), art: art.clone(), @@ -1597,6 +2297,58 @@ fn read_preferences(db_path: &Path) -> Result { }) } +/// Forget "nobody has art for this" answers given before a second source +/// existed. +/// +/// `none` means "everything this computer asks was asked, and nothing came +/// back", and it is trusted for [`cover::ART_NONE_LIFETIME_SECONDS`]. That is +/// the right answer to keep — asking MusicBrainz the same question twice a day +/// is rude and pointless — but only while "everything this computer asks" means +/// the same thing. Once Apple's catalogue is part of the search, an older +/// verdict was reached without it, and leaving those rows alone is what would +/// make an album Napstr can now cover keep drawing a blank square until the +/// fortnight ran out. +/// +/// Guarded by its own marker rather than by a schema version, so it happens +/// exactly once per database and rows written after it keep their full life. A +/// parked failure is deliberately left alone: it is a fifteen-minute retry, not +/// a verdict, and it expires on its own. +fn forget_answers_from_before_the_second_source( + connection: &rusqlite::Connection, +) -> Result { + if read_flag(connection, SETTING_SECOND_SOURCE_STARTED) { + return Ok(0); + } + let cleared = connection + .execute("DELETE FROM album_art_lookups WHERE outcome='none'", []) + .map_err(|error| error.to_string())?; + connection + .execute( + "INSERT OR REPLACE INTO settings (key, value) VALUES (?1, '1')", + params![SETTING_SECOND_SOURCE_STARTED], + ) + .map_err(|error| error.to_string())?; + Ok(cleared) +} + +/// The art domain list exactly as it is stored, for showing back in the box. +/// +/// Read as text rather than normalized: the stored value is already normalized +/// by [`CoverPublisher::set_allowed_art_hosts`], and a row written by an older +/// build should look like what it is. +fn read_allowed_art_hosts(db_path: &Path) -> Result { + let connection = crate::open_connection(db_path)?; + let stored = connection + .query_row( + "SELECT value FROM settings WHERE key=?1", + [cover::SETTING_ALLOWED_ART_HOSTS], + |row| row.get::<_, String>(0), + ) + .optional() + .map_err(|error| error.to_string())?; + Ok(stored.unwrap_or_default().trim().to_string()) +} + fn read_flag(connection: &rusqlite::Connection, key: &str) -> bool { connection .query_row("SELECT value FROM settings WHERE key=?1", [key], |row| { @@ -2038,6 +2790,416 @@ mod tests { ); } + #[test] + fn apple_names_a_cover_the_archive_has_none_for() { + // Measured, not imagined: this release group answers 404 at the Cover + // Art Archive and Apple sells the sleeve. It is why a second source + // exists at all. + let result: ItunesResult = serde_json::from_str( + r#"{"collectionId":1895931386,"collectionName":"Annihilation - Single", + "artistName":"KREAM & Korolova","trackCount":1, + "releaseDate":"2026-05-22T07:00:00Z","collectionType":"Album", + "artworkUrl100":"https://is1-ssl.mzstatic.com/image/thumb/Music211/v4/7b/3e/00/7b3e00e6-bec7-e8cf-b86a-064eed9f87d6/827568018151.jpg/100x100bb.jpg"}"#, + ) + .unwrap(); + // Apple's `- Single` marker is not part of the album's name, and a + // comparison against a tag reading `Annihilation` fails on it alone. + assert_eq!(result.title(), "Annihilation"); + let cover = result.cover(); + assert_eq!(cover.title, "Annihilation"); + assert_eq!(cover.year, "2026"); + // The whole picture, not the 100-pixel thumbnail Apple's search returns. + assert!( + cover.art.ends_with("/1200x1200bb.jpg"), + "the full rendition is what a claim wants: {}", + cover.art + ); + assert!(cover.thumb.ends_with("/200x200bb.jpg"), "{}", cover.thumb); + assert!(cover.art.starts_with("https://"), "{}", cover.art); + // The id is Apple's, and it must never be mistaken for an MBID. + assert_eq!(cover.id(), "itunes:1895931386"); + } + + #[test] + fn an_apple_url_of_another_shape_is_left_alone() { + // Apple's artwork URLs end in the rendition, and only that shape is + // rewritten. Anything else is returned as it arrived rather than + // mangled into an address that does not resolve. + assert_eq!( + itunes_rendition("https://example.org/cover.jpg", ITUNES_FULL_RENDITION), + "https://example.org/cover.jpg" + ); + // A `http://` one is repaired, because a claim may only carry HTTPS. + assert_eq!( + itunes_rendition( + "http://is1-ssl.mzstatic.com/a/b/1.jpg/100x100bb.jpg", + ITUNES_FULL_RENDITION + ), + "https://is1-ssl.mzstatic.com/a/b/1.jpg/1200x1200bb.jpg" + ); + // Nothing usable is empty, not a half-built URL. + assert_eq!(itunes_rendition("", ITUNES_FULL_RENDITION), ""); + assert_eq!(itunes_rendition("ftp://host/a.jpg", ITUNES_FULL_RENDITION), ""); + } + + #[test] + fn the_apple_result_has_to_name_this_album_and_one_of_its_artists() { + let result = |collection: &str, artist: &str| ItunesResult { + collection_id: 1, + collection_name: collection.to_string(), + artist_name: artist.to_string(), + artwork_url: "https://is1-ssl.mzstatic.com/x/1.jpg/100x100bb.jpg".to_string(), + release_date: "2026-05-22T07:00:00Z".to_string(), + }; + // A namesake by another artist is a different record, and putting its + // sleeve on this one is worse than leaving the square blank. + let offered = vec![ + result("Annihilation", "Some Tribute Band"), + result("Annihilation", "KREAM & Korolova"), + result("Annihilation (Remixes)", "KREAM & Korolova"), + ]; + let picked = best_itunes_result(&offered, "Annihilation", "KREAM / Korolova") + .expect("the record the tag means is in there"); + assert_eq!(picked.artist_name, "KREAM & Korolova"); + assert_eq!(picked.title(), "Annihilation"); + + // A tag that joins its artists differently still matches: the words of + // the credited names are what agree, not the punctuation. + assert!(best_itunes_result(&offered, "Annihilation", "KREAM, Korolova").is_some()); + // A result with no artwork is no use whatever it is named. + let mut artless = result("Annihilation", "KREAM"); + artless.artwork_url = String::new(); + assert!(best_itunes_result(&[artless], "Annihilation", "KREAM").is_none()); + // And nothing that is not this album is accepted, however Apple ranks it. + assert!(best_itunes_result(&offered, "Elation", "KREAM").is_none()); + } + + #[test] + fn a_pasted_link_is_refused_unless_its_host_is_listed() { + // The setting is a text box, so a list arrives however a person wrote + // it: commas, newlines, capitals, or a wildcard they expect to work. + assert_eq!( + cover::parse_art_hosts(" CoverArtArchive.org ,\n*.MzStatic.com \n\n archive.org"), + vec!["coverartarchive.org", "mzstatic.com", "archive.org"] + ); + // An empty list accepts any host, which is what every library had before + // the setting existed. + assert!(cover::art_host_allowed(&[], "https://anything.example/a.jpg")); + let hosts = cover::parse_art_hosts("archive.org\ncoverartarchive.org"); + // A listed domain covers its subdomains, which is where the files + // actually live. + assert!(cover::art_host_allowed( + &hosts, + "https://ia801509.us.archive.org/2/items/x/a.jpg" + )); + assert!(cover::art_host_allowed( + &hosts, + "https://coverartarchive.org/release/x/front" + )); + // A domain that merely ends in a listed one is not that domain. + assert!(!cover::art_host_allowed(&hosts, "https://notarchive.org/a.jpg")); + assert!(!cover::art_host_allowed( + &hosts, + "https://archive.org.evil.example/a.jpg" + )); + // A link that is not an absolute HTTP address has nothing to check. + assert!(!cover::art_host_allowed(&hosts, "/a.jpg")); + assert!(!cover::art_host_allowed(&hosts, "data:image/png;base64,AAAA")); + // A port and userinfo are not part of the host. + assert!(cover::art_host_allowed( + &hosts, + "https://user@archive.org:8080/a.jpg" + )); + } + + #[test] + fn the_art_host_setting_round_trips_through_the_database() { + let connection = cover_database(); + assert_eq!(cover::allowed_art_hosts(&connection), Vec::::new()); + connection + .execute( + "INSERT OR REPLACE INTO settings (key,value) VALUES (?1,?2)", + params![cover::SETTING_ALLOWED_ART_HOSTS, "archive.org\nmzstatic.com"], + ) + .unwrap(); + assert_eq!( + cover::allowed_art_hosts(&connection), + vec!["archive.org".to_string(), "mzstatic.com".to_string()] + ); + } + + #[test] + fn a_stale_no_art_verdict_is_forgotten_once_there_is_a_second_source() { + let connection = cover_database(); + let found = art_lookup("covered|record", "https://archive.org/front.jpg"); + // Three answers, and only one of them is a verdict reached without + // asking Apple. + cover::record_art_lookup(&connection, "quiet|record", cover::ArtLookupOutcome::NoArt) + .unwrap(); + cover::record_art_lookup( + &connection, + "covered|record", + cover::ArtLookupOutcome::Found(&found), + ) + .unwrap(); + cover::record_art_lookup( + &connection, + "broken|record", + cover::ArtLookupOutcome::Failed { + retry_after_seconds: FAILED_LOOKUP_RETRY_SECONDS, + }, + ) + .unwrap(); + + assert_eq!( + forget_answers_from_before_the_second_source(&connection).unwrap(), + 1, + "only the verdict reached without the second source is discarded" + ); + // The covered album keeps its art, and the failure keeps its own + // fifteen-minute retry rather than being promoted to a verdict. + let mut statement = connection + .prepare("SELECT cover_key || ':' || outcome FROM album_art_lookups ORDER BY cover_key") + .unwrap(); + let outcomes: Vec = statement + .query_map([], |row| row.get(0)) + .unwrap() + .collect::, _>>() + .unwrap(); + assert_eq!(outcomes, vec!["broken|record:error", "covered|record:found"]); + + // It happens once: a "no art" answer recorded after the upgrade is a + // verdict on the whole search, and keeps its fortnight. + cover::record_art_lookup(&connection, "quiet|record", cover::ArtLookupOutcome::NoArt) + .unwrap(); + assert_eq!( + forget_answers_from_before_the_second_source(&connection).unwrap(), + 0 + ); + let suppressed = + cover::suppressed_art_keys(&connection, &["quiet|record".to_string()]).unwrap(); + assert_eq!(suppressed.len(), 1, "the new answer is still in force"); + } + + #[test] + fn the_log_keeps_the_reason_and_explains_the_missing_list() { + let connection = cover_database(); + insert_library_track(&connection, "aa", "Blue Foundation", "Blood Moon"); + insert_library_track(&connection, "bb", "KREAM", "Annihilation"); + insert_library_track(&connection, "cc", "Somebody", "Covered"); + insert_library_track(&connection, "dd", "Nobody", "Scanned"); + insert_library_track(&connection, "ee", "Quiet", "Held"); + // One album has a claim from somebody else, so it is not missing a + // `30427` and belongs nowhere near this list. + connection + .execute( + "INSERT INTO album_covers(cover_key,source_pubkey,art,thumb,mbid,year,genre,collection,source,cover_file_id,mime,event_id,created_at,deleted,seeder,seen_at) + VALUES('somebody|covered','aa','https://example.com/a.jpg','','','','','','musicbrainz','','','bb',1,0,0,'now')", + [], + ) + .unwrap(); + // One was asked about and has none, one was asked about and this + // computer holds the picture, and a lookup fell over on another. + cover::record_art_lookup(&connection, "nobody|scanned", cover::ArtLookupOutcome::NoArt) + .unwrap(); + cover::record_art_lookup( + &connection, + "quiet|held", + cover::ArtLookupOutcome::Found(&art_lookup("quiet|held", "https://archive.org/a.jpg")), + ) + .unwrap(); + cover::record_art_lookup( + &connection, + "kream|annihilation", + cover::ArtLookupOutcome::Failed { + retry_after_seconds: FAILED_LOOKUP_RETRY_SECONDS, + }, + ) + .unwrap(); + cover::record_lookup_log( + &connection, + cover::LookupLogEntry { + key: "kream|annihilation", + artist: "KREAM", + album: "Annihilation", + outcome: "failed", + source: "", + message: "MusicBrainz lookup failed: error sending request \u{2192} dns error", + }, + ) + .unwrap(); + cover::record_lookup_log( + &connection, + cover::LookupLogEntry { + key: "blue foundation|blood moon", + artist: "Blue Foundation", + album: "Blood Moon", + outcome: "found", + source: "itunes", + message: "", + }, + ) + .unwrap(); + + let log = cover::recent_lookup_log(&connection, 10).unwrap(); + assert_eq!(log.len(), 2); + // Newest first, with the source that answered kept. + assert_eq!(log[0].album, "Blood Moon"); + assert_eq!(log[0].outcome, "found"); + assert_eq!(log[0].source, "itunes"); + assert!(log[1].message.contains("dns error"), "{}", log[1].message); + + let gaps = missing_albums(&connection, 50).unwrap(); + assert_eq!( + gaps.iter().map(|gap| gap.state.as_str()).collect::>(), + // The failure first, then the albums in the order a person cares + // about them: work to do, written off, art nobody has signed. + vec!["failed", "not_looked_up", "no_art", "resolved_here"] + ); + assert_eq!(gaps[0].album, "Annihilation"); + assert!( + gaps[0].note.contains("dns error"), + "the list carries the last reason: {}", + gaps[0].note + ); + assert_eq!(gaps[1].artist, "Blue Foundation"); + assert_eq!(gaps[1].tracks, 1); + assert_eq!(gaps[3].album, "Held"); + // The claimed album is absent, not merely last. + assert!(!gaps.iter().any(|gap| gap.album == "Covered")); + + // The reason survives a restart because it is read back from the log, and + // the log stays bounded: a pass over a library may not grow it forever. + for index in 0..(cover::LOOKUP_LOG_LIMIT + 20) { + cover::record_lookup_log( + &connection, + cover::LookupLogEntry { + key: "blue foundation|blood moon", + artist: "Blue Foundation", + album: "Blood Moon", + outcome: "none", + source: "", + message: &format!("attempt {index}"), + }, + ) + .unwrap(); + } + let kept: i64 = connection + .query_row("SELECT COUNT(*) FROM cover_lookup_log", [], |row| row.get(0)) + .unwrap(); + assert_eq!(kept, cover::LOOKUP_LOG_LIMIT); + } + + /// A transport failure has to say *why*. reqwest's own message is the URL the + /// reader already had, and a screenful of those is what sent somebody looking + /// for a log page in the first place. + #[tokio::test] + async fn a_transport_failure_carries_the_cause_that_reqwest_hides() { + let _ = rustls::crypto::ring::default_provider().install_default(); + // A client that will not wait, against a port nothing listens on: the + // failure is a transport one, and nothing here leaves the machine, so + // this is not one of the live tests. + let client = reqwest::Client::builder() + .user_agent(USER_AGENT) + .timeout(Duration::from_secs(2)) + .build() + .expect("a client"); + let error = client + .get("http://127.0.0.1:1/") + .send() + .await + .expect_err("nothing listens on port 1"); + let LookupError::Failed(message) = request_error("MusicBrainz lookup failed", &error) else { + panic!("a transport failure is a failure, not a refusal"); + }; + assert!(message.starts_with("MusicBrainz lookup failed: "), "{message}"); + assert!( + message.contains('\u{2192}'), + "the cause has to be appended, not left inside the error: {message}" + ); + assert!( + message.len() > error.to_string().len(), + "reqwest's own message is what was already being shown: {message}" + ); + } + + /// The whole route for the album that started all of this: a release group + /// MusicBrainz knows, an archive that has no picture of it, and a sleeve on + /// sale at Apple. + /// + /// Ignored because it needs all three services: + /// `cargo test --ignored live_lookup_falls_through -- --nocapture` + #[tokio::test] + #[ignore = "requires MusicBrainz, the Cover Art Archive and iTunes"] + async fn the_live_lookup_falls_through_to_the_second_source() { + let _ = rustls::crypto::ring::default_provider().install_default(); + let client = cover_http_client().expect("a lookup client"); + let candidate = CoverCandidate { + key: "kream|annihilation".to_string(), + artist: "KREAM / Korolova".to_string(), + album: "Annihilation".to_string(), + track_count: 1, + source: "library".to_string(), + }; + let found = resolve(&client, &candidate) + .await + .expect("the lookup has to answer rather than fail") + .expect("Apple sells this single, so the album has a cover"); + assert_eq!(found.source, "itunes", "the picture came from the second source"); + // The identifier is still MusicBrainz's: the release group was + // identified there, and a reader can follow an MBID and nothing else. + assert_eq!(found.mbid, "e8fbaa62-ca7d-4a9b-8f6f-44939b3775f5"); + assert_eq!(found.collection, "Annihilation"); + assert_eq!(found.year, "2026"); + assert!(found.art.contains("mzstatic.com"), "{}", found.art); + assert!(found.art.ends_with("/1200x1200bb.jpg"), "{}", found.art); + assert_eq!(found.key, candidate.key); + } + + /// The live second source against the live iTunes catalogue, for the record the + /// archive has no picture of at all. + /// + /// Ignored because it needs the network: + /// `cargo test --ignored live_second_source -- --nocapture` + #[tokio::test] + #[ignore = "requires the iTunes catalogue"] + async fn the_live_second_source_covers_what_the_archive_never_scanned() { + let _ = rustls::crypto::ring::default_provider().install_default(); + let client = cover_http_client().expect("a lookup client"); + // The release group MusicBrainz knows and whose Cover Art Archive entry + // answers 404: `coverartarchive.org/release-group/e8fbaa62-…` says no + // cover art was found, and Apple sells the sleeve. + let found = itunes_lookup(&client, "KREAM / Korolova", "Annihilation") + .await + .expect("iTunes has to answer"); + let cover = found.expect("Apple sells this single, so it has a sleeve"); + assert_eq!(cover.title, "Annihilation"); + assert!(cover.art.ends_with("/1200x1200bb.jpg"), "{}", cover.art); + // The rendition has to actually resolve. A URL that answers 404 is worse + // than no cover at all: it is a claim that points at nothing. + let response = client + .get(&cover.art) + .send() + .await + .expect("the artwork has to be fetched"); + assert!( + response.status().is_success(), + "{} answered {}", + cover.art, + response.status() + ); + assert!( + response + .headers() + .get(reqwest::header::CONTENT_TYPE) + .and_then(|value| value.to_str().ok()) + .unwrap_or_default() + .starts_with("image/"), + "the URL has to be a picture: {}", + cover.art + ); + } + /// The live archive, for the one record that could not be looked up at all: /// its release group answers 500, its release answers 500, and the picture /// sits on a node neither of those addresses names. @@ -2067,6 +3229,52 @@ mod tests { assert!(!art.contains("dn711003"), "the stale node must not be named: {art}"); } + #[test] + fn a_service_that_stops_answering_ends_the_pass_rather_than_grinding() { + // The waits grow exactly as they do for a refusal, because it is the + // same message from the service. + assert_eq!(hold_decision(1, None), (Duration::from_secs(30), false)); + assert_eq!(hold_decision(3, None), (Duration::from_secs(120), false)); + // A server that names its own interval is honoured on the first hold. + assert_eq!( + hold_decision(1, Some(Duration::from_secs(120))), + (Duration::from_secs(120), false) + ); + // And then the pass ends: waiting an evening for a service that has + // stopped answering is not patience, and the albums left stay pending. + let (wait, given_up) = hold_decision(MAX_CONSECUTIVE_HOLDS, None); + assert_eq!(wait, THROTTLE_BACKOFF_MAX); + assert!(given_up); + assert!(!hold_decision(MAX_CONSECUTIVE_HOLDS - 1, None).1); + } + + #[test] + fn an_unanswered_request_reads_as_a_busy_service() { + let said = describe_lookup_error(LookupError::Unanswered { + waited: MUSICBRAINZ_TIMEOUT, + }); + // The wait it actually gave the service, named. + assert!(said.contains(&MUSICBRAINZ_TIMEOUT.as_secs().to_string()), "{said}"); + assert!(said.contains("busy"), "{said}"); + // And it must not read like a verdict on the album: that is the mistake + // this whole path was making. + assert!(!said.contains("no art"), "{said}"); + } + + /// The pace is shared, so no caller can slip in behind another's back. The + /// search, the release-group fallback and the manual tool all come through + /// here, and callers that each pace themselves still collide. + #[tokio::test] + async fn two_musicbrainz_callers_come_out_one_interval_apart() { + let started = Instant::now(); + tokio::join!(pace_musicbrainz(), pace_musicbrainz()); + assert!( + started.elapsed() >= REQUEST_INTERVAL, + "two callers were let through together: {:?}", + started.elapsed() + ); + } + #[test] fn backoff_grows_and_respects_a_retry_after_hint() { assert_eq!(throttle_delay(1, None), Duration::from_secs(30)); diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index 5740f5b..fee2d2d 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -2209,6 +2209,42 @@ fn nudge_cover_worker(state: State<'_, AppState>) { state.covers.nudge(); } +/// The domains this computer will take album art from, as the Covers tab edits +/// it. +/// +/// Stored normalised — one lowercase domain per line — so the box reads back the +/// list that is actually in force. An empty list accepts any HTTPS host, which +/// is how Napstr behaved before the list existed. +#[tauri::command] +fn set_allowed_art_hosts( + hosts: String, + state: State<'_, AppState>, +) -> Result { + state.covers.set_allowed_art_hosts(&hosts) +} + +/// The newest cover lookup attempts, newest first, for the Covers tab's log. +/// +/// This exists because a failed lookup used to leave nothing behind but a count: +/// the reason — a DNS failure, a refusal, an album nobody has art for — was +/// written nowhere, so an outage and a blank square looked identical. +#[tauri::command] +fn cover_lookup_log( + limit: usize, + state: State<'_, AppState>, +) -> Result, String> { + state.covers.lookup_log(limit) +} + +/// Every album this computer holds that no cover answers, failures first. +#[tauri::command] +fn cover_missing( + limit: usize, + state: State<'_, AppState>, +) -> Result, String> { + state.covers.missing_albums(limit) +} + /// Albums the results pane is showing, reported as it draws them. /// /// This is how "albums seen in search results" reaches the cover worker: the @@ -2439,6 +2475,9 @@ pub fn run() { cover_default_query, cover_search_candidates, cover_apply_pick, + set_allowed_art_hosts, + cover_lookup_log, + cover_missing, report_cover, set_downloads_paused, clear_all_transfers, diff --git a/src-tauri/src/network.rs b/src-tauri/src/network.rs index 6cc33b9..8578f50 100644 --- a/src-tauri/src/network.rs +++ b/src-tauri/src/network.rs @@ -2732,6 +2732,20 @@ impl NetworkService { pub async fn best_known_covers(&self, keys: Vec) -> Result, String> { let covers = self.album_covers(keys.clone()).await?; let requested = cover::normalised_request(&keys, COVER_KEY_LIMIT); + let connection = super::open_connection(&self.db_path)?; + // This is the one place a claim and a local resolution both become a + // picture, so it is where "which hosts will this computer take art + // from" is asked. Art on a host the list does not name is not drawn, + // and the album then falls back to whatever *is* acceptable for it — + // including nothing. An empty list accepts every host, which is what + // every library that never opened the setting has. + let hosts = cover::allowed_art_hosts(&connection); + let usable = |cover: &AlbumCover| { + [&cover.art, &cover.thumb] + .into_iter() + .all(|url| url.is_empty() || cover::art_host_allowed(&hosts, url)) + }; + let mut covers = covers.into_iter().filter(usable).collect::>(); let claimed = covers .iter() .map(|cover| cover.key.clone()) @@ -2743,9 +2757,11 @@ impl NetworkService { if missing.is_empty() { return Ok(covers); } - let connection = super::open_connection(&self.db_path)?; - let mut covers = covers; - covers.extend(cover::resolved_covers(&connection, &missing)?); + covers.extend( + cover::resolved_covers(&connection, &missing)? + .into_iter() + .filter(usable), + ); Ok(covers) } From 87b4fcf8395b8314ade270825f3c3994f894807b Mon Sep 17 00:00:00 2001 From: StateAntigen Date: Sun, 27 Sep 2026 14:11:39 -0700 Subject: [PATCH 41/42] feat(cover): paste a link, and see what the lookups did MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The window half of the same work. The art picker gains a second row: paste an image URL, see the picture before committing, then file it. It is the only path where art arrives from an address nobody vouched for, so it is the path the host's art domain list gates outright. Candidates the app looked up itself are exempt, because a list that omitted Napstr's own sources on purpose would break the feature beside it. The Covers tab gains two panels. A lookup log, newest first, with the outcome and the reason for each attempt — the tab could say that thirty albums failed and nothing about why. And the albums with no claim, failures first, with the last recorded reason on each row, because the queue above only ever showed the next twenty-five the worker would touch: an album parked after a transport error simply vanished from view. The art domain list itself is a text box, saved normalised, with a recommended-list button. Empty means any HTTPS host, which is what every library had before the setting existed, so nobody's covers disappear into a filter they never asked for. It is deliberately described as a filter and not a boundary: the person who sets it is the person it protects, and the real answer is the trust model the cover NIP is heading for. --- src/lib/CoverPicker.svelte | 141 +++++++++++++++++++++++++++--- src/routes/+page.svelte | 175 ++++++++++++++++++++++++++++++++++++- src/styles.css | 24 +++++ 3 files changed, 326 insertions(+), 14 deletions(-) diff --git a/src/lib/CoverPicker.svelte b/src/lib/CoverPicker.svelte index ad96f05..29ef0ea 100644 --- a/src/lib/CoverPicker.svelte +++ b/src/lib/CoverPicker.svelte @@ -21,7 +21,12 @@ export let onApplied: () => void = () => {}; type Hit = { + /** What the row is keyed by. Not always an MBID: an iTunes row is keyed by + * Apple's collection id, because there is no MusicBrainz group for it. */ + id: string; mbid: string; + /** `musicbrainz` or `itunes`. */ + source: string; title: string; types: string; year: string; @@ -35,6 +40,7 @@ }; let query = ''; + let imageUrl = ''; let hits: Hit[] = []; let busy = false; let error = ''; @@ -42,7 +48,7 @@ /** True once a search has actually run, so the empty list can be explained. */ let searched = false; /** The candidate whose art was just applied, to mark the row. */ - let appliedMbid = ''; + let appliedId = ''; async function loadDefaultQuery() { try { @@ -56,7 +62,7 @@ busy = true; error = ''; note = ''; - appliedMbid = ''; + appliedId = ''; try { hits = await invoke('cover_search_candidates', { artist, @@ -64,7 +70,7 @@ query: query.trim() ? query : null }); searched = true; - if (hits.length === 0) note = 'MusicBrainz returned nothing for that search.'; + if (hits.length === 0) note = 'Neither MusicBrainz nor iTunes returned anything for that album.'; } catch (failure) { error = String(failure); hits = []; @@ -74,6 +80,15 @@ } } + /** + * File a candidate from the list. + * + * `source: 'hit'` is what tells the host this art came from a row it offered + * itself, so the art domain list is not applied to it: that list exists to + * guard links a person types and art other people publish, and a list that + * omitted the sources Napstr uses on purpose would break the feature it sits + * beside. + */ async function use(hit: Hit) { busy = true; error = ''; @@ -89,11 +104,50 @@ art: hit.art, thumb: hit.thumb, title: hit.title, - year: hit.year + year: hit.year, + source: 'hit' + } + } + ); + appliedId = hit.id; + note = result.note ? `${result.note} Key: ${result.key}` : `Saved under ${result.key}`; + onApplied(); + } catch (failure) { + error = String(failure); + } finally { + busy = false; + } + } + + /** + * File art from a link a person pasted. + * + * This is art from an address nobody vouched for, so it is the case the art + * domain list in the Covers tab applies to, and the host is what refuses it — + * the window cannot be the only thing checking. + */ + async function useUrl() { + busy = true; + error = ''; + note = ''; + appliedId = ''; + try { + const result = await invoke<{ key: string; published: boolean; eventId: string; note: string }>( + 'cover_apply_pick', + { + pick: { + artist, + album, + mbid: '', + art: imageUrl.trim(), + thumb: '', + title: '', + year: '', + source: 'url' } } ); - appliedMbid = hit.mbid; + appliedId = 'pasted'; note = result.note ? `${result.note} Key: ${result.key}` : `Saved under ${result.key}`; onApplied(); } catch (failure) { @@ -146,9 +200,43 @@

Lucene syntax: release:, artist:, AND, OR. Swap in a different spelling, add country:US, or search for a release group by name — - the automatic lookup could not guess that. + the automatic lookup could not guess that. iTunes is always asked as well, by album and artist + name rather than by this query.

+ +
+ + { + if (event.key === 'Enter') { + event.preventDefault(); + void useUrl(); + } + }} + /> + +
+ {#if imageUrl.trim().startsWith('https://')} +
+ + What that link holds, if the host will serve it. +
+ {/if} + {#if currentArt}

The art shown now is the small square on the left of each row below; pick the one that matches.

{/if} @@ -157,8 +245,8 @@ {#if note}
{note}
{/if}
- {#each hits as hit (hit.mbid)} -
+ {#each hits as hit (hit.id)} +
{#if hit.thumb || hit.art} @@ -169,16 +257,22 @@
{hit.title} - {hit.types || 'unknown type'}{hit.year ? ` · ${hit.year}` : ''} · score {hit.score} + {hit.source === 'itunes' ? 'iTunes' : 'MusicBrainz'}{hit.types ? ` · ${hit.types}` : ''}{hit.year ? ` · ${hit.year}` : ''}{hit.score ? ` · score ${hit.score}` : ''} {hit.front ? '' : ' · no front image'} {#if hit.note} - + {hit.note} {/if} - {hit.mbid} + + {#if hit.mbid || hit.art} + {hit.id} + {/if}
{#if hit.chosen}automatic{/if} @@ -240,6 +334,7 @@ grid-template-columns: auto 1fr auto; align-items: center; gap: 10px; + margin-top: 10px; } .art-picker-query label { color: #8d9ab0; @@ -280,6 +375,26 @@ border: 1px solid rgba(60, 130, 90, 0.45); color: #b6e0c6; } + /* The pasted link, shown before it is filed: a person pasting a URL is + judging a picture, and judging it from a filename is not judging it. */ + .art-picker-preview { + margin-top: 10px; + display: flex; + align-items: center; + gap: 12px; + } + .art-picker-preview img { + width: 96px; + height: 96px; + object-fit: cover; + border-radius: 6px; + border: 1px solid #222b3a; + background: #06090d; + } + .art-picker-preview small { + color: #8d9ab0; + font-size: 12px; + } .art-picker-results { margin-top: 12px; display: grid; diff --git a/src/routes/+page.svelte b/src/routes/+page.svelte index 84b6960..9b4260d 100644 --- a/src/routes/+page.svelte +++ b/src/routes/+page.svelte @@ -1177,6 +1177,28 @@ // backend's worker acts on them by itself: there is no button to press for a // pass to happen, and leaving one on means it resumes after a restart. type CoverCandidate = { key: string; artist: string; album: string; trackCount: number; source: string }; + /** An album this computer holds that no cover answers. */ + type CoverGap = { + key: string; + artist: string; + album: string; + tracks: number; + /** `failed`, `not_looked_up` or `no_art`. */ + state: string; + source: string; + /** The most recent reason recorded for it, when there is one. */ + note: string; + }; + /** One attempt, the way the log kept it. */ + type CoverLogRow = { + at: string; + key: string; + artist: string; + album: string; + outcome: string; + source: string; + message: string; + }; type CoverStatus = { lookupExternal: boolean; publishClaims: boolean; @@ -1191,10 +1213,15 @@ failed: number; backedOff: number; stopped: boolean; + /** One domain per line. Empty means any HTTPS host. */ + allowedArtHosts: string; message: string; }; /** How many albums the Covers tab previews. The worker itself is unbounded. */ const COVER_QUEUE_PREVIEW = 25; + /** How many albums the missing-cover list shows, and how many log lines. */ + const COVER_GAP_PREVIEW = 400; + const COVER_LOG_PREVIEW = 120; // Deliberately plain `let`s, like every other variable in this file. A single // rune anywhere in the component compiles the whole thing in runes mode, which // silently makes every plain `let` here non-reactive - including the @@ -1205,6 +1232,22 @@ let coverStatus: CoverStatus | null = null; let coverLoading = false; let coverError = ''; + /** Albums no cover answers, worst first, and what each lookup actually did. */ + let coverMissing: CoverGap[] = []; + let coverLog: CoverLogRow[] = []; + // The art domain list is edited in a text box, so the box holds the draft and + // `allowedArtHostsSaved` holds what the host actually has. A background status + // refresh must not overwrite what somebody is halfway through typing, so the + // stored value is only adopted while the two still agree. + let allowedArtHosts = ''; + let allowedArtHostsSaved = ''; + /** + * The hosts Napstr itself reads art from, and the list to offer somebody who + * wants the filter on without having to work out the domains by hand. Apple + * is named at `mzstatic.com` because that is where its artwork redirects to: + * `itunes.apple.com` answers the search, not the picture. + */ + const RECOMMENDED_ART_HOSTS = ['coverartarchive.org', 'archive.org', 'mzstatic.com']; // Bumped when the worker produces art, which is what makes the tiles ask // again. A long pass would otherwise leave every square blank until it ended. let coverRevision = 0; @@ -1257,7 +1300,15 @@ coverError = ''; try { coverStatus = await invoke('cover_status'); + if (allowedArtHosts === allowedArtHostsSaved) { + allowedArtHosts = coverStatus.allowedArtHosts; + allowedArtHostsSaved = allowedArtHosts; + } coverQueue = await invoke('cover_candidates', { limit: COVER_QUEUE_PREVIEW }); + // The two diagnostic lists are read with the same call that refreshes the + // tab: an album that just failed is the thing a person came here to see. + coverMissing = await invoke('cover_missing', { limit: COVER_GAP_PREVIEW }); + coverLog = await invoke('cover_lookup_log', { limit: COVER_LOG_PREVIEW }); } catch (error) { coverError = String(error); } finally { @@ -1265,6 +1316,42 @@ } } + /** When a log line happened, in the reader's own clock. */ + function coverLogTime(at: string) { + const stamp = new Date(at); + return Number.isNaN(stamp.getTime()) ? at : stamp.toLocaleTimeString(); + } + + /** How a missing album's state reads to a person. */ + function coverGapState(state: string) { + if (state === 'failed') return 'lookup failed'; + if (state === 'no_art') return 'no art anywhere'; + if (state === 'resolved_here') return 'art here, no claim'; + return 'not looked up yet'; + } + + /** + * Save the art domain list. + * + * The host normalizes it — lowercase, one per line — and the box is refilled + * from what it stored, so the list on screen is the list in force rather than + * the string that was typed. Every cached square is then dropped, because a + * host that has just been removed from the list must stop being drawn at once + * rather than at the next restart. + */ + async function saveAllowedArtHosts(hosts: string) { + coverError = ''; + try { + coverStatus = await invoke('set_allowed_art_hosts', { hosts }); + allowedArtHosts = coverStatus.allowedArtHosts; + allowedArtHostsSaved = allowedArtHosts; + lastCoverRevisionAt = 0; + refreshCoverArtwork(); + } catch (error) { + coverError = String(error); + } + } + async function setCoverPreferences(lookupExternal: boolean, publishClaims: boolean) { coverError = ''; try { @@ -3022,7 +3109,7 @@
+ +
+
+ + {'One per line. A domain covers its subdomains: mzstatic.com admits is1-ssl.mzstatic.com.'} +
+ +
+ + + +
+

i {"Empty means any HTTPS host, which is what every library had before this list existed. With a list, art is drawn only when it is served from one of these domains or a subdomain of one — somebody else's published claim, art this computer looked up, and a link you paste here, which is refused outright if its host is not listed. It is a filter, not a boundary: the person who sets it is the person it protects, and the real answer is the trust model the cover NIP is heading for."}

+
+ {#if coverError}
{coverError}
{/if} {#if coverStatus} @@ -3066,6 +3185,60 @@

{coverOptIn(coverStatus) ? 'Nothing is waiting. New music and new browsing wake Napstr by themselves.' : 'Switch one of these on and Napstr starts on its own.'}

{/if}
+ + +
+
+ + {coverMissing.length} {coverMissing.length === 1 ? 'album' : 'albums'}{" of this computer's own music, worst first. A failure comes first because a lookup that fell over looks identical to an album nobody has art for."} +
+
+ {#each coverMissing as gap (gap.key)} +
+
{gap.album}{gap.artist}
+ {coverGapState(gap.state)} + {gap.tracks} {gap.tracks === 1 ? 'track' : 'tracks'} + + {#if gap.note}{gap.note}{/if} +
+ {/each} + {#if !coverLoading && coverMissing.length === 0} +

{'Every album this computer holds has a cover claim, or has not been scanned yet.'}

+ {/if} +
+
+ + +
+
+ + {'Newest first, last '}{COVER_LOG_PREVIEW}{' attempts, kept across restarts.'} +
+
+ {#each coverLog as line (line.at + line.key + line.outcome)} +
+ + {line.outcome} + {line.album}{line.artist ? ` — ${line.artist}` : ''} + {#if line.source}{line.source}{/if} + {#if line.message}{line.message}{/if} +
+ {/each} + {#if !coverLoading && coverLog.length === 0} +

{'No lookups have run yet.'}

+ {/if} +
+
{:else if activeView === 'Napstrfy'}
diff --git a/src/styles.css b/src/styles.css index ad05417..b301673 100644 --- a/src/styles.css +++ b/src/styles.css @@ -245,6 +245,30 @@ body.resizing-transfer-pane, body.resizing-transfer-pane * { cursor: ns-resize ! .cover-scan-status { margin: 4px; padding: 6px 10px; display: flex; flex-wrap: wrap; gap: 16px; background: #e6e6e6; border: 2px solid; border-color: #777 white white #777; } .cover-scan-status b { font-variant-numeric: tabular-nums; } .cover-scan-current, .cover-scan-message { margin: 0 8px 4px; color: #444; } +.cover-art-hosts { margin: 4px; padding: 8px 10px; background: #d5d5d5; border: 2px solid; border-color: white #777 #777 white; } +.cover-art-hosts-head { display: flex; flex-wrap: wrap; align-items: baseline; gap: 10px; } +.cover-art-hosts-head label { font-weight: bold; } +.cover-art-hosts-head span { color: #555; } +.cover-art-hosts textarea { width: 100%; margin-top: 6px; font-family: ui-monospace, SFMono-Regular, Menlo, monospace; resize: vertical; } +.cover-art-hosts-actions { margin-top: 6px; display: flex; flex-wrap: wrap; gap: 6px; } +.cover-gap-panel, .cover-log-panel { margin: 4px; padding: 8px 10px; background: #d5d5d5; border: 2px solid; border-color: white #777 #777 white; } +.cover-gap-list, .cover-log-list { margin-top: 6px; max-height: 320px; overflow: auto; background: #eee; border: 2px inset white; } +.cover-gap { padding: 6px 9px; display: grid; grid-template-columns: minmax(0, 1fr) 110px 70px auto; gap: 9px; align-items: center; border-bottom: 1px solid #b0b0b0; } +.cover-gap:last-child { border-bottom: 0; } +.cover-gap b, .cover-gap small { display: block; overflow: hidden; text-overflow: ellipsis; white-space: nowrap; } +.cover-gap small { color: #555; } +.cover-gap-state { font-size: 12px; color: #555; } +.cover-gap-state.cover-gap-failed { color: #a3241c; font-weight: bold; } +.cover-gap-state.cover-gap-held { color: #2f5c8a; } +.cover-gap-note { grid-column: 1 / -1; color: #7a4a12; } +.cover-log-line { padding: 5px 9px; display: grid; grid-template-columns: 78px 84px minmax(0, 1fr) auto; gap: 9px; align-items: baseline; border-bottom: 1px solid #ddd; } +.cover-log-line:last-child { border-bottom: 0; } +.cover-log-line time { color: #666; font-variant-numeric: tabular-nums; } +.cover-log-outcome { font-size: 12px; color: #555; } +.cover-log-outcome.cover-gap-failed { color: #a3241c; font-weight: bold; } +.cover-log-outcome.cover-log-throttled { color: #8a6a12; } +.cover-log-album, .cover-log-source { overflow: hidden; text-overflow: ellipsis; white-space: nowrap; } +.cover-log-source { color: #555; font-size: 12px; } .cover-candidate-list { margin: 4px; background: #eee; border: 2px inset white; } .cover-candidate { padding: 7px 9px; display: grid; grid-template-columns: minmax(0, 1fr) 84px minmax(0, 1fr); gap: 9px; align-items: center; border-bottom: 1px solid #b0b0b0; } .cover-candidate:last-child { border-bottom: 0; } From ba9b2cda0b858d989046ab2e5c59c6b1b5bc95ce Mon Sep 17 00:00:00 2001 From: StateAntigen Date: Sun, 27 Sep 2026 14:16:42 -0700 Subject: [PATCH 42/42] test(cover): wait out a busy MusicBrainz instead of failing on it The live tests asserted that MusicBrainz answers, which stopped being a safe assumption the moment its queue was understood: a 503 or an unanswered request is the service talking about itself, not about the query, and it really does send both often enough that a run failed on it minutes after passing. They now retry three times with a wait, so a passing run still means the query found the record and a failing one still means the service is down. This is what the tolerance is for: `Annihilation` came back right on the run after the failure, with nothing changed but the clock. --- src-tauri/src/cover_publish.rs | 61 ++++++++++++++++++++++++++++++---- 1 file changed, 54 insertions(+), 7 deletions(-) diff --git a/src-tauri/src/cover_publish.rs b/src-tauri/src/cover_publish.rs index 1c08b9f..3727e85 100644 --- a/src-tauri/src/cover_publish.rs +++ b/src-tauri/src/cover_publish.rs @@ -2750,6 +2750,57 @@ mod tests { ); } + /// How long a live test waits before asking a busy service again. + const LIVE_RETRY_WAIT: Duration = Duration::from_secs(20); + + /// Ask MusicBrainz for one query, waiting out a refusal instead of failing. + /// + /// A 503 and an unanswered request are the service talking about itself, not + /// about the query, and the live service really does send both — measured, + /// and often enough that these tests failed on it. Retried three times, so + /// the assertion still means something when it passes and still fails when + /// the service is genuinely down. + async fn live_groups(client: &reqwest::Client, query: &str) -> Vec { + let mut last = String::new(); + for attempt in 1..=3 { + match search_groups(client, query).await { + Ok(groups) => return groups, + Err(error) => { + last = format!("{error:?}"); + let back_pressure = matches!( + error, + LookupError::Throttled { .. } | LookupError::Unanswered { .. } + ); + if !back_pressure || attempt == 3 { + break; + } + tokio::time::sleep(LIVE_RETRY_WAIT).await; + } + } + } + panic!("MusicBrainz would not answer about {query}: {last}"); + } + + /// `resolve` under the same tolerance, for the tests that run the whole + /// chain: the refusal can come from any of its three requests. + async fn live_resolve( + client: &reqwest::Client, + candidate: &CoverCandidate, + ) -> Result, LookupError> { + for attempt in 1..=3 { + match resolve(client, candidate).await { + Err(error @ (LookupError::Throttled { .. } | LookupError::Unanswered { .. })) + if attempt < 3 => + { + let _ = error; + tokio::time::sleep(LIVE_RETRY_WAIT).await; + } + other => return other, + } + } + resolve(client, candidate).await + } + /// The queries two tags produce, against the live MusicBrainz. Both of these /// are searches that used to find nothing at all. /// @@ -2766,9 +2817,7 @@ mod tests { let client = cover_http_client().expect("a lookup client"); // A joint credit, which MusicBrainz holds as two credited artists. let joint = default_query("The Chainsmokers, Oaks", "Love Is Kind"); - let groups = search_groups(&client, &joint) - .await - .expect("MusicBrainz has to answer"); + let groups = live_groups(&client, &joint).await; assert!( groups .iter() @@ -2779,9 +2828,7 @@ mod tests { // A credit the tag overstates: MusicBrainz knows no artist called // Korolova at all, and credits this record to KREAM alone. let overstated = default_query("KREAM / Korolova", "Annihilation"); - let groups = search_groups(&client, &overstated) - .await - .expect("MusicBrainz has to answer"); + let groups = live_groups(&client, &overstated).await; assert!( groups .iter() @@ -3141,7 +3188,7 @@ mod tests { track_count: 1, source: "library".to_string(), }; - let found = resolve(&client, &candidate) + let found = live_resolve(&client, &candidate) .await .expect("the lookup has to answer rather than fail") .expect("Apple sells this single, so the album has a cover");