From 44954b1e4cbf4e7a97d39ed7adbcd5950b730cd0 Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Fri, 16 Oct 2026 00:26:39 +0200 Subject: [PATCH 1/6] feat(enrichment): artist portraits from deezer, through a narrow fetcher Deezer becomes a provider for portraits only. The picture it names is fetched under RFC-011 decision 8: an allowlisted host, https, every resolved address public and the checked one pinned for the connection, no redirect, a size ceiling, and a type confirmed by the bytes. The artwork row, the cache row and every artist.artwork_hash sharing the name are written in one transaction under the writer gate, and an artist event is recorded only for rows whose hash moved. The cache row keeps the hash, so an artist that lost its link is linked again without a second download. RFC-007 decision 6 is amended accordingly. Closes #268. Signed-off-by: InstaZDLL --- docs/operations.md | 21 +- docs/rfcs/RFC-007-library-event-stream.md | 12 + docs/rfcs/RFC-011-metadata-enrichment.md | 8 +- .../20261016000000_metadata_artist_image.sql | 10 + src/config.rs | 12 +- src/database.rs | 124 ++++++- src/enrichment.rs | 204 ++++++++++- src/lib.rs | 7 +- src/outbound.rs | 8 +- src/portrait.rs | 327 ++++++++++++++++++ src/services/artist_enrichment.rs | 1 + src/services/artwork.rs | 6 + tests/enrichment.rs | 292 ++++++++++++++++ 13 files changed, 996 insertions(+), 36 deletions(-) create mode 100644 migrations-v2/20261016000000_metadata_artist_image.sql create mode 100644 src/portrait.rs diff --git a/docs/operations.md b/docs/operations.md index e3e15231..9d668213 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -182,9 +182,9 @@ WAVEFLOW_METADATA_LASTFM_KEY=… # your own Last.fm API application's key | Variable | Default | What it does | | --- | --- | --- | -| `WAVEFLOW_METADATA_PROVIDERS` | empty | The switch: an ordered, comma-separated list. Empty is off. Today the only provider is `lastfm` | +| `WAVEFLOW_METADATA_PROVIDERS` | empty | The switch: an ordered, comma-separated list. Empty is off. The providers are `lastfm` (biographies and similar artists) and `deezer` (artist portraits, no key) | | `WAVEFLOW_METADATA_LASTFM_KEY` | — | Required when `lastfm` is listed; the server refuses to start without it. No key ships in the binary | -| `WAVEFLOW_METADATA_IMAGES` | on | `off` keeps the lookups and refuses artist image downloads | +| `WAVEFLOW_METADATA_IMAGES` | on | `off` keeps the lookups and refuses artist image downloads; `deezer` is then not asked at all, since portraits are all it is asked for | | `WAVEFLOW_METADATA_TTL_DAYS` | 30 | How long an answer with content is kept before it is asked again | | `WAVEFLOW_METADATA_MISS_TTL_DAYS` | 7 | How long "the provider knows nothing about this artist" is kept | | `WAVEFLOW_METADATA_RATE_PER_SEC` | 4 | Requests per second, per provider | @@ -201,9 +201,22 @@ request carries the artist's name and your key, and no redirect is followed. Requests are spaced to the rate above, and simultaneous visits to one artist share one request. +With `deezer` listed, also `api.deezer.com`, which needs no key, and then the +picture it names — but only from Deezer's image hosts +(`e-cdns-images.dzcdn.net`, `cdn-images.dzcdn.net`), only over https, only at +a public address (the address checked is the one connected to), only a JPEG, +PNG or WebP by its own bytes, and at most 10 MiB. A picture that fails any of +these is not fetched and the artist is remembered as having none until the +short TTL runs out. + **What it stores.** In `waveflow.db`, for each provider and each artist name -asked about: the biography, the ranked list of similar artists by name, whether -the provider knew the artist, and when to ask again. Nothing in it belongs to an +asked about: the biography, the ranked list of similar artists by name, the +portrait's hash, whether the provider knew the artist, and when to ask again. +The portrait itself goes in the artwork store beside the covers, and every +artist with that name shows it through the `coverArt` it already carries — +**back it up with the rest of `artwork_dir`**: unlike a cover, it cannot be +extracted again from your files, only downloaded again once the cache entry +expires. Nothing in it belongs to an account, and nothing records who asked. A failed request stores nothing, so the next visit asks again. diff --git a/docs/rfcs/RFC-007-library-event-stream.md b/docs/rfcs/RFC-007-library-event-stream.md index c7a5a34a..74a8017d 100644 --- a/docs/rfcs/RFC-007-library-event-stream.md +++ b/docs/rfcs/RFC-007-library-event-stream.md @@ -231,6 +231,18 @@ over this server and **written nowhere**. An artist photo cannot change because there never is one. The question reopens the day something writes it, and on that day this is the line to re-read. +> **Amended 2026-10-16 (#268): an artist now emits one, for its portrait only.** +> [RFC-011](RFC-011-metadata-enrichment.md) decision 8 writes +> `artist.artwork_hash` — a portrait fetched from a metadata provider — so the +> case the desktop reserved exists. The rule is the album's: an `upsert` with an +> empty payload, emitted **only when the hash actually moved**, by the same +> `UPDATE … WHERE artwork_hash IS NOT ? RETURNING` pattern, so a refresh that +> fetched the same picture says nothing. Artist rows are per library, so a +> portrait shared by a name emits one event per row, each in its own library. +> `library_event.entity_type` admitted `'artist'` from the start, so no +> migration was needed. The counters stay derived and still emit nothing: the +> reasoning above holds for everything but the photo. + ## Decision 7 — two bounds, because one alone is always wrong > **Built.** See the *Implemented by* line in the header. diff --git a/docs/rfcs/RFC-011-metadata-enrichment.md b/docs/rfcs/RFC-011-metadata-enrichment.md index 4f471ef0..32cb3619 100644 --- a/docs/rfcs/RFC-011-metadata-enrichment.md +++ b/docs/rfcs/RFC-011-metadata-enrichment.md @@ -6,9 +6,11 @@ InstaZDLL/WaveFlow#855 (the core's clients accept a client, an address and a key), #270 (the pin moved onto them), #271 (the cache and the engine that fills it), #272 (`getArtistInfo`, `getArtistInfo2` and the native artist - page read it), and the pull request closing #267 (`getSimilarSongs` and - `getSimilarSongs2` draw from it). Portraits are still to come. The *Status* field above never flips in this - project; this line is the one that says what is running. + page read it), #273 and #274 (`getSimilarSongs` and `getSimilarSongs2` draw + from it), and the pull request closing #268 (portraits from Deezer). The web + client and the compatibility campaign are still to come. The *Status* field + above never flips in this project; this line is the one that says what is + running. - **Date**: 2026-10-14 - **Revised**: 2026-10-15, while implementing the cache (#265). Decisions 4, 6 and 10 rested on attachment points the code does not hold; each correction diff --git a/migrations-v2/20261016000000_metadata_artist_image.sql b/migrations-v2/20261016000000_metadata_artist_image.sql new file mode 100644 index 00000000..1dfac03f --- /dev/null +++ b/migrations-v2/20261016000000_metadata_artist_image.sql @@ -0,0 +1,10 @@ +-- RFC-011 decision 8: the portrait a provider pointed at, once fetched. +-- +-- The hash is kept on the provider's cache row as well as on `artist`, so an +-- artist re-identified by a PID spec change is linked again without a second +-- download. `ON DELETE SET NULL` is what makes that safe: the sweeper counts +-- `artist`, `album` and `track` as references and not this column, so an +-- artwork row nobody shows may go, and the cache row then forgets it rather +-- than naming a file that no longer exists. The next refresh fetches it again. +ALTER TABLE metadata_artist + ADD COLUMN image_hash TEXT REFERENCES artwork(hash) ON DELETE SET NULL; diff --git a/src/config.rs b/src/config.rs index 0f3b3d0a..a1f2062c 100644 --- a/src/config.rs +++ b/src/config.rs @@ -200,7 +200,8 @@ pub struct ScrobbleDestination { #[derive(Clone)] pub struct MetadataSettings { /// `WAVEFLOW_METADATA_PROVIDERS`: an ordered, comma-separated list, empty - /// by default. The order is the order fields are merged in when read. + /// by default, of `lastfm` and `deezer`. The order is the order fields are + /// merged in when read. pub providers: Vec, /// `WAVEFLOW_METADATA_IMAGES`, on by default, `off` to keep the lookups and /// refuse decision 8's image fetch. A narrowing only: it switches nothing @@ -1246,6 +1247,15 @@ mod tests { assert!(parse_metadata_providers("lastfm", false).is_err()); assert!(parse_metadata_providers("lastfm,lastfm", true).is_err()); assert!(parse_metadata_providers("last.fm", true).is_err()); + // Deezer asks for no key, so none is required of it. + assert_eq!( + parse_metadata_providers("lastfm, deezer", true).unwrap(), + [Provider::LastFm, Provider::Deezer] + ); + assert_eq!( + parse_metadata_providers("deezer", false).unwrap(), + [Provider::Deezer] + ); } #[test] diff --git a/src/database.rs b/src/database.rs index d0557d46..e18fe92e 100644 --- a/src/database.rs +++ b/src/database.rs @@ -1917,7 +1917,7 @@ impl Database { name_canonical: &str, ) -> Result, sqlx::Error> { let rows = sqlx::query( - "SELECT provider, outcome, bio_summary, bio_full, fetched_at, expires_at \ + "SELECT provider, outcome, bio_summary, bio_full, image_hash, fetched_at, expires_at \ FROM metadata_artist WHERE name_canonical = ?", ) .bind(name_canonical) @@ -1938,6 +1938,7 @@ impl Database { outcome, bio_summary: row.try_get("bio_summary")?, bio_full: row.try_get("bio_full")?, + image_hash: row.try_get("image_hash")?, fetched_at: row.try_get("fetched_at")?, expires_at: row.try_get("expires_at")?, }); @@ -1970,12 +1971,51 @@ impl Database { .collect() } - /// Records what a provider answered: its row and its whole neighbour list, + /// Links the portrait `hash` to every artist called `name_canonical` that + /// does not show it yet, emitting one artist event per row it moved. + /// + /// Read first without the writer gate, since this runs on the read path + /// and almost always finds nothing to do. The artwork row is checked in + /// the same statement as the link: the sweeper may have taken it since + /// the cache row was read. + pub(crate) async fn link_artist_portrait( + &self, + name_canonical: &str, + hash: &str, + ) -> Result<(), sqlx::Error> { + let unlinked: bool = sqlx::query_scalar( + "SELECT EXISTS (SELECT 1 FROM artist \ + WHERE canonical_name = ? AND artwork_hash IS NOT ?)", + ) + .bind(name_canonical) + .bind(hash) + .fetch_one(&self.pool) + .await?; + if !unlinked { + return Ok(()); + } + let _writer = self.writer_guard().await; + let mut tx = self.pool.begin().await?; + link_portrait_in( + &mut tx, + name_canonical, + hash, + crate::authentication::now_ms(), + ) + .await?; + tx.commit().await + } + + /// Records what a provider answered: its row, its whole neighbour list, + /// and its portrait — the artwork row and every artist it belongs to — /// in one transaction. /// - /// The writer gate is taken here and nowhere earlier — the request that - /// produced `answer` is over before this is called, so no network I/O ever - /// runs under it. + /// One transaction is what keeps the sweeper from ever seeing the artwork + /// row unreferenced: it deletes under this same gate, so it runs wholly + /// before or wholly after. The writer gate is taken here and nowhere + /// earlier — the requests that produced `answer` are over, and the + /// portrait's file is already in the store, so no network or file I/O + /// ever runs under it. pub(crate) async fn write_metadata_artist( &self, provider: crate::enrichment::Provider, @@ -1986,24 +2026,41 @@ impl Database { ) -> Result<(), sqlx::Error> { let _writer = self.writer_guard().await; let mut tx = self.pool.begin().await?; + if let Some(image) = &answer.image { + sqlx::query( + "INSERT INTO artwork (hash, format, source, byte_size, created_at) \ + VALUES (?, ?, 'deezer', ?, ?) ON CONFLICT (hash) DO NOTHING", + ) + .bind(&image.hash) + .bind(image.format) + .bind(image.byte_size) + .bind(fetched_at) + .execute(&mut *tx) + .await?; + } sqlx::query( "INSERT INTO metadata_artist \ - (provider, name_canonical, outcome, bio_summary, bio_full, fetched_at, expires_at) \ - VALUES (?, ?, ?, ?, ?, ?, ?) \ + (provider, name_canonical, outcome, bio_summary, bio_full, image_hash, \ + fetched_at, expires_at) \ + VALUES (?, ?, ?, ?, ?, ?, ?, ?) \ ON CONFLICT (provider, name_canonical) DO UPDATE SET \ outcome = excluded.outcome, bio_summary = excluded.bio_summary, \ - bio_full = excluded.bio_full, fetched_at = excluded.fetched_at, \ - expires_at = excluded.expires_at", + bio_full = excluded.bio_full, image_hash = excluded.image_hash, \ + fetched_at = excluded.fetched_at, expires_at = excluded.expires_at", ) .bind(provider.as_str()) .bind(name_canonical) .bind(answer.outcome().as_str()) .bind(answer.bio_summary.as_deref()) .bind(answer.bio_full.as_deref()) + .bind(answer.image.as_ref().map(|image| image.hash.as_str())) .bind(fetched_at) .bind(expires_at) .execute(&mut *tx) .await?; + if let Some(image) = &answer.image { + link_portrait_in(&mut tx, name_canonical, &image.hash, fetched_at).await?; + } sqlx::query( "DELETE FROM metadata_artist_similar WHERE provider = ? AND name_canonical = ?", ) @@ -2030,6 +2087,55 @@ impl Database { } } +/// Points every artist called `name_canonical` at the portrait `hash`, and +/// records one artist event for each row that actually moved — RFC-007 +/// decision 6's rule for albums, which an artist now follows: a refresh that +/// fetched the same picture changes nothing and says nothing. +/// +/// Artist rows are per library, and so are events, so each moved row carries +/// its own library. A row whose artwork row is gone is not linked: the cache +/// may name a hash the sweeper has since taken. +async fn link_portrait_in( + tx: &mut sqlx::Transaction<'_, sqlx::Sqlite>, + name_canonical: &str, + hash: &str, + now: i64, +) -> Result<(), sqlx::Error> { + let moved = sqlx::query( + "UPDATE artist SET artwork_hash = ?, updated_at = ? \ + WHERE canonical_name = ? AND artwork_hash IS NOT ? \ + AND EXISTS (SELECT 1 FROM artwork WHERE artwork.hash = ?) \ + RETURNING id, library_id", + ) + .bind(hash) + .bind(now) + .bind(name_canonical) + .bind(hash) + .bind(hash) + .fetch_all(&mut **tx) + .await?; + for row in moved { + let id: String = row.try_get("id")?; + let library_id: String = row.try_get("library_id")?; + let parse = + |value: &str| Uuid::parse_str(value).map_err(|error| sqlx::Error::Decode(error.into())); + crate::catalog::record_library_event( + tx, + crate::catalog::LibraryChange { + library_id: parse(&library_id)?, + entity_type: "artist", + entity_id: parse(&id)?, + action: "upsert", + payload: serde_json::json!({}), + changed_at: now, + origin_device_id: None, + }, + ) + .await?; + } + Ok(()) +} + #[cfg(test)] mod tests { use super::*; diff --git a/src/enrichment.rs b/src/enrichment.rs index 8bf656c3..a8f6b439 100644 --- a/src/enrichment.rs +++ b/src/enrichment.rs @@ -21,19 +21,36 @@ //! URL nor a response body is ever written to a log (decision 9). use std::{ + path::PathBuf, sync::Arc, time::{Duration, Instant}, }; use dashmap::DashMap; -use waveflow_core::metadata::{lastfm::LastfmClient, LookupError}; +use waveflow_core::metadata::{ + deezer::{DeezerClient, DeezerError}, + lastfm::LastfmClient, + name_match::{normalize_name, select_by_name}, + LookupError, +}; -use crate::{config::MetadataSettings, database::Database}; +use crate::{ + config::MetadataSettings, + database::Database, + portrait::{self, AddressPolicy, Refusal}, +}; /// Last.fm's API root. A constant, not a setting (decision 9): there is no /// operator field, so there is no URL to validate. const LASTFM_BASE_URL: &str = "https://ws.audioscrobbler.com/2.0/"; +/// Deezer's API root, a constant for the same reason. It asks for no key. +const DEEZER_BASE_URL: &str = "https://api.deezer.com"; + +/// The hosts Deezer serves its pictures from (decision 8). A picture URL +/// naming any other host is dropped without being contacted. +const DEEZER_IMAGE_HOSTS: [&str; 2] = ["e-cdns-images.dzcdn.net", "cdn-images.dzcdn.net"]; + /// How many neighbours are asked for and kept. Subsonic's `getArtistInfo` /// defaults to twenty; asking for more leaves room for the ones a caller cannot /// see, which decision 7 drops at read time. @@ -53,12 +70,16 @@ pub const MAX_IN_FLIGHT: usize = 64; #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub enum Provider { LastFm, + /// Portraits only: Deezer has no biography, and its related artists are + /// not asked for. + Deezer, } impl Provider { pub fn parse(value: &str) -> Option { match value { "lastfm" => Some(Self::LastFm), + "deezer" => Some(Self::Deezer), _ => None, } } @@ -67,6 +88,7 @@ impl Provider { pub fn as_str(self) -> &'static str { match self { Self::LastFm => "lastfm", + Self::Deezer => "deezer", } } } @@ -103,10 +125,21 @@ pub struct CachedArtist { pub outcome: Outcome, pub bio_summary: Option, pub bio_full: Option, + /// The portrait this provider supplied, in the artwork store. Cleared by + /// the database when the sweeper takes the artwork row. + pub image_hash: Option, pub fetched_at: i64, pub expires_at: i64, } +/// A portrait already in the artwork store, waiting for its row. +#[derive(Debug)] +pub(crate) struct StoredPortrait { + pub(crate) hash: String, + pub(crate) format: &'static str, + pub(crate) byte_size: i64, +} + /// One neighbour in a provider's cached list. #[derive(Debug, Clone)] pub struct CachedSimilar { @@ -121,12 +154,17 @@ pub(crate) struct ArtistAnswer { pub(crate) bio_summary: Option, pub(crate) bio_full: Option, pub(crate) similar: Vec, + pub(crate) image: Option, } impl ArtistAnswer { /// A hit carries something; an answer with nothing in it is a miss. pub(crate) fn outcome(&self) -> Outcome { - if self.bio_summary.is_some() || self.bio_full.is_some() || !self.similar.is_empty() { + if self.bio_summary.is_some() + || self.bio_full.is_some() + || !self.similar.is_empty() + || self.image.is_some() + { Outcome::Hit } else { Outcome::Miss @@ -140,12 +178,21 @@ impl ArtistAnswer { #[derive(Debug, Clone)] pub struct Endpoints { pub lastfm: String, + pub deezer: String, + /// The hosts a Deezer picture may be fetched from. + pub deezer_image_hosts: Vec, + /// The addresses a picture may be fetched from. Only the test suite, + /// whose image host is on loopback, sets anything but the default. + pub image_addresses: AddressPolicy, } impl Default for Endpoints { fn default() -> Self { Self { lastfm: LASTFM_BASE_URL.to_owned(), + deezer: DEEZER_BASE_URL.to_owned(), + deezer_image_hosts: DEEZER_IMAGE_HOSTS.map(str::to_owned).to_vec(), + image_addresses: AddressPolicy::default(), } } } @@ -167,6 +214,9 @@ struct Engine { ttl_ms: i64, miss_ttl_ms: i64, request_timeout: Duration, + /// Where fetched portraits are stored: the artwork store. + artwork_dir: PathBuf, + image_addresses: AddressPolicy, /// The fills in flight, by provider and canonical name. A visit that finds /// its key here schedules nothing: the fill already running answers it. in_flight: DashMap<(Provider, String), ()>, @@ -182,7 +232,14 @@ struct Source { } enum Client { - LastFm { client: LastfmClient, key: String }, + LastFm { + client: LastfmClient, + key: String, + }, + Deezer { + client: DeezerClient, + image_hosts: Vec, + }, } impl Enrichment { @@ -192,18 +249,24 @@ impl Enrichment { } /// The engine `settings` describe, against the providers' real addresses. - pub fn new(db: Database, settings: &MetadataSettings) -> anyhow::Result { - Self::with_endpoints(db, settings, Endpoints::default()) + pub fn new( + db: Database, + settings: &MetadataSettings, + artwork_dir: PathBuf, + ) -> anyhow::Result { + Self::with_endpoints(db, settings, artwork_dir, Endpoints::default()) } /// [`Self::new`], with the providers reached at `endpoints`. /// /// Only the configured providers are built. A key placed with no provider /// naming it builds nothing — the provider list is the switch, and a key - /// is not consent (decision 3). + /// is not consent (decision 3). With `WAVEFLOW_METADATA_IMAGES=off`, + /// Deezer is not built either: portraits are all it is asked for. pub fn with_endpoints( db: Database, settings: &MetadataSettings, + artwork_dir: PathBuf, endpoints: Endpoints, ) -> anyhow::Result { let mut sources = Vec::new(); @@ -220,6 +283,11 @@ impl Enrichment { key, } } + Provider::Deezer if !settings.images => continue, + Provider::Deezer => Client::Deezer { + client: DeezerClient::with_http(http, endpoints.deezer.clone()), + image_hosts: endpoints.deezer_image_hosts.clone(), + }, }; sources.push(Source { provider, @@ -237,6 +305,8 @@ impl Enrichment { ttl_ms: duration_ms(settings.ttl), miss_ttl_ms: duration_ms(settings.miss_ttl), request_timeout: settings.request_timeout, + artwork_dir, + image_addresses: endpoints.image_addresses, in_flight: DashMap::new(), running: tokio::sync::watch::channel(0).0, })), @@ -280,6 +350,12 @@ impl Enrichment { } ordered.extend(row.cloned()); } + // A portrait already fetched, and an artist row that does not show + // it: an artist re-identified by a PID spec change, or one scanned + // since. Linked again from the cache, without a second download. + if let Some(hash) = ordered.iter().find_map(|row| row.image_hash.as_deref()) { + engine.db.link_artist_portrait(&canonical, hash).await?; + } Ok(ordered) } @@ -336,6 +412,10 @@ impl Engine { let source = &self.sources[index]; let answer = match &source.client { Client::LastFm { client, key } => self.ask_lastfm(source, client, key, name).await, + Client::Deezer { + client, + image_hosts, + } => self.ask_deezer(source, client, image_hosts, name).await, }; let answer = match answer { Ok(answer) => answer, @@ -414,18 +494,75 @@ impl Engine { bio_summary, bio_full, similar, + image: None, + }) + } + + /// The artist search, then the picture it names — decision 8. + /// + /// No artist by that name, or one without a picture, is a miss. So is a + /// picture the fetcher refuses for what it is — off the allowlist, at a + /// refused address, not an image, too large — since asking again would + /// get the same answer. A picture host that could not be reached is + /// unreached, like the search, and writes nothing. + async fn ask_deezer( + &self, + source: &Source, + client: &DeezerClient, + image_hosts: &[String], + name: &str, + ) -> Result { + source.pace.wait().await; + let hits = self.bounded(client.search_artist(name)).await?; + let picture = select_by_name(hits, &normalize_name(name), |hit| Some(hit.name.as_str())) + .and_then(|hit| hit.best_picture()); + let Some(picture) = picture else { + return Ok(ArtistAnswer::default()); + }; + let fetched = portrait::fetch( + &picture, + image_hosts, + self.image_addresses, + self.request_timeout, + ) + .await; + let portrait = match fetched { + Ok(portrait) => portrait, + Err(refusal) if refusal.is_answer() => { + tracing::warn!( + provider = source.provider.as_str(), + verdict = %refusal, + "artist portrait refused; cached as a miss" + ); + return Ok(ArtistAnswer::default()); + } + Err(refusal) => return Err(Verdict::Image(refusal)), + }; + portrait::store(&self.artwork_dir, &portrait) + .await + .map_err(|_| Verdict::Store)?; + Ok(ArtistAnswer { + image: Some(StoredPortrait { + hash: portrait.hash, + format: portrait.format, + byte_size: portrait.bytes.len() as i64, + }), + ..ArtistAnswer::default() }) } /// One request, under the request timeout; anything but an answer is a /// verdict. - async fn bounded( + async fn bounded( &self, - request: impl std::future::Future>, - ) -> Result { + request: impl std::future::Future>, + ) -> Result + where + Verdict: From, + { match tokio::time::timeout(self.request_timeout, request).await { Ok(Ok(answer)) => Ok(answer), - Ok(Err(error)) => Err(Verdict::of(&error)), + Ok(Err(error)) => Err(Verdict::from(error)), Err(_) => Err(Verdict::Timeout), } } @@ -450,25 +587,55 @@ impl Drop for Flight { enum Verdict { Timeout, Transport, - Refused { code: i64 }, - Status { status: u16 }, + Refused { + code: i64, + }, + Status { + status: u16, + }, TooLarge, Undecodable, + RateLimited, + /// The picture host, not the provider, was not reached. + Image(Refusal), + /// The picture came back and could not be written to the artwork store. + Store, } -impl Verdict { - fn of(error: &LookupError) -> Self { +impl From for Verdict { + fn from(error: LookupError) -> Self { match error { LookupError::Http(error) if error.is_timeout() => Self::Timeout, LookupError::Http(_) => Self::Transport, - LookupError::Refused { code } => Self::Refused { code: *code }, - LookupError::Status { status } => Self::Status { status: *status }, + LookupError::Refused { code } => Self::Refused { code }, + LookupError::Status { status } => Self::Status { status }, LookupError::TooLarge { .. } => Self::TooLarge, LookupError::Decode(_) => Self::Undecodable, } } } +impl From for Verdict { + fn from(error: DeezerError) -> Self { + match error { + DeezerError::Http(error) if error.is_timeout() => Self::Timeout, + DeezerError::Http(error) => { + error + .status() + .map_or(Self::Transport, |status| Self::Status { + status: status.as_u16(), + }) + } + DeezerError::Api(error) => Self::Refused { + code: error.code.unwrap_or_default(), + }, + DeezerError::RateLimited => Self::RateLimited, + DeezerError::TooLarge { .. } => Self::TooLarge, + DeezerError::Decode(_) => Self::Undecodable, + } + } +} + impl std::fmt::Display for Verdict { fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { @@ -478,6 +645,9 @@ impl std::fmt::Display for Verdict { Self::Status { status } => write!(formatter, "answered HTTP {status}"), Self::TooLarge => formatter.write_str("answer over the size ceiling"), Self::Undecodable => formatter.write_str("answer not understood"), + Self::RateLimited => formatter.write_str("rate-limited"), + Self::Image(refusal) => refusal.fmt(formatter), + Self::Store => formatter.write_str("portrait could not be stored"), } } } diff --git a/src/lib.rs b/src/lib.rs index 1e408ca4..f79df843 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -13,6 +13,7 @@ pub mod oauth; pub mod outbound; pub mod pid; pub mod playlist_file; +pub mod portrait; pub mod scanner; pub mod scrobblers; pub mod security; @@ -683,7 +684,11 @@ pub async fn initialize(config: &Config) -> anyhow::Result { services.reconcile_scrobble_links().await?; // Off unless the operator named a provider, and then it still makes no // request until somebody asks about an artist (RFC-011 decisions 3 and 6). - services.use_enrichment(enrichment::Enrichment::new(db.clone(), &config.metadata)?); + services.use_enrichment(enrichment::Enrichment::new( + db.clone(), + &config.metadata, + config.artwork_dir.clone(), + )?); Ok(AppState { db, auth, diff --git a/src/outbound.rs b/src/outbound.rs index 2f9ac95a..936510c1 100644 --- a/src/outbound.rs +++ b/src/outbound.rs @@ -38,10 +38,16 @@ const CONNECT_SHARE: u32 = 3; /// Which of the two fired was a coin toss, so which protection applied was a /// coin toss. pub fn client(timeout: Duration) -> Result { + builder(timeout).build() +} + +/// The same rules, left open for one caller to add to: the portrait fetcher +/// pins the address it checked (`ClientBuilder::resolve`), which is a property +/// of one connection rather than of every client. +pub(crate) fn builder(timeout: Duration) -> reqwest::ClientBuilder { reqwest::Client::builder() .redirect(reqwest::redirect::Policy::none()) .connect_timeout(timeout / CONNECT_SHARE) .no_proxy() .user_agent(concat!("WaveFlowServer/", env!("CARGO_PKG_VERSION"))) - .build() } diff --git a/src/portrait.rs b/src/portrait.rs new file mode 100644 index 00000000..e9fa7c51 --- /dev/null +++ b/src/portrait.rs @@ -0,0 +1,327 @@ +//! The narrow image fetcher of RFC-011 decision 8. +//! +//! A provider's answer names an image URL, and that URL is untrusted input: it +//! came out of a response, not out of the operator's configuration. So the +//! fetcher dereferences it only when every one of these holds: +//! +//! - **https**, and the host is on the provider's **allowlist**, compared +//! exactly. A URL pointing anywhere else is dropped, not fetched. +//! - **Every address the name resolves to is public.** Loopback, private, +//! link-local, shared, unspecified and multicast ranges are refused, IPv4 +//! inside IPv6 included. +//! - **The address checked is the address connected to.** The client is built +//! for this one fetch with the checked address pinned +//! (`ClientBuilder::resolve`), so the name is not looked up a second time by +//! a resolver whose answer could differ. +//! - **No redirect** (the shared client's policy), a **size ceiling** enforced +//! while reading, and a **content type** among the three the artwork route +//! serves — confirmed by the bytes themselves, not taken from the header. +//! +//! The address policy is a parameter for one reason: the test suite's image +//! host lives on loopback, which the default refuses. [`AddressPolicy::default`] +//! is production, and a test asserts it refuses loopback, so the suite cannot +//! prove the fetcher only with its main protection switched off. + +use std::{ + net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr}, + path::Path, + time::Duration, +}; + +/// The largest portrait accepted. Deezer's largest size is a 1000×1000 JPEG, +/// well under a mebibyte; ten leaves room without letting a provider fill the +/// disk. +pub const MAX_IMAGE_BYTES: usize = 10 * 1024 * 1024; + +/// Which addresses a fetch may connect to. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub enum AddressPolicy { + /// Public addresses only, over https. Production. + #[default] + Public, + /// Public addresses, and loopback over plain http as well. For the test + /// suite's listeners, and for nothing else. + AllowLoopback, +} + +impl AddressPolicy { + /// Whether a connection to `ip` is allowed. + pub fn admits(self, ip: IpAddr) -> bool { + let ip = match ip { + IpAddr::V6(v6) => v6.to_ipv4_mapped().map_or(ip, IpAddr::V4), + v4 => v4, + }; + if ip.is_loopback() { + return self == Self::AllowLoopback; + } + match ip { + IpAddr::V4(v4) => is_public_v4(v4), + IpAddr::V6(v6) => is_public_v6(v6), + } + } +} + +fn is_public_v4(ip: Ipv4Addr) -> bool { + let [a, b, ..] = ip.octets(); + !(ip.is_private() + || ip.is_link_local() + || ip.is_unspecified() + || ip.is_broadcast() + || ip.is_multicast() + || ip.is_documentation() + // 100.64.0.0/10, carrier-grade NAT: another operator's private range. + || (a == 100 && (64..128).contains(&b)) + // 0.0.0.0/8, "this network". + || a == 0) +} + +fn is_public_v6(ip: Ipv6Addr) -> bool { + let first = ip.segments()[0]; + !(ip.is_unspecified() + || ip.is_multicast() + // fc00::/7, unique local. + || (first & 0xfe00) == 0xfc00 + // fe80::/10, link local. + || (first & 0xffc0) == 0xfe80) +} + +/// A fetched portrait, checked and hashed, not yet stored. +#[derive(Debug)] +pub struct Portrait { + /// BLAKE3 of the bytes, hexadecimal: the artwork store's key. + pub hash: String, + /// `jpg`, `png` or `webp`, from the bytes. + pub format: &'static str, + pub bytes: Vec, +} + +/// Why a portrait was not fetched. Carries nothing from the URL or the answer. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Refusal { + /// Not https, or a host off the allowlist: the provider pointed somewhere + /// it should not have. Nothing was contacted. + Destination, + /// The name resolved to an address the policy refuses. Nothing was + /// contacted. + Address, + /// The answer is not one of the three image types, by header or by bytes. + NotAnImage, + /// Over [`MAX_IMAGE_BYTES`]. + TooLarge, + /// The name did not resolve, the connection failed, or the deadline passed. + Unreached, + /// The host answered something other than success. + Status(u16), +} + +impl Refusal { + /// Whether this says something about the image itself, which a cache may + /// remember, rather than about the way to it, which the next visit should + /// try again. + pub fn is_answer(self) -> bool { + matches!( + self, + Self::Destination | Self::Address | Self::NotAnImage | Self::TooLarge + ) + } +} + +impl std::fmt::Display for Refusal { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Destination => formatter.write_str("image destination not allowed"), + Self::Address => formatter.write_str("image address not allowed"), + Self::NotAnImage => formatter.write_str("answer is not an image"), + Self::TooLarge => formatter.write_str("image over the size ceiling"), + Self::Unreached => formatter.write_str("image host unreached"), + Self::Status(status) => write!(formatter, "image host answered HTTP {status}"), + } + } +} + +/// Fetches `url` under every rule above, within `timeout` overall. +pub async fn fetch( + url: &str, + hosts: &[String], + policy: AddressPolicy, + timeout: Duration, +) -> Result { + tokio::time::timeout(timeout, fetch_within(url, hosts, policy, timeout)) + .await + .unwrap_or(Err(Refusal::Unreached)) +} + +async fn fetch_within( + url: &str, + hosts: &[String], + policy: AddressPolicy, + timeout: Duration, +) -> Result { + let url = url::Url::parse(url).map_err(|_| Refusal::Destination)?; + let plain = match url.scheme() { + "https" => false, + "http" if policy == AddressPolicy::AllowLoopback => true, + _ => return Err(Refusal::Destination), + }; + let host = url + .host_str() + .map(str::to_ascii_lowercase) + .ok_or(Refusal::Destination)?; + if !hosts + .iter() + .any(|allowed| allowed.eq_ignore_ascii_case(&host)) + { + return Err(Refusal::Destination); + } + let port = url.port_or_known_default().ok_or(Refusal::Destination)?; + let addresses: Vec = match url.host() { + Some(url::Host::Ipv4(ip)) => vec![SocketAddr::new(IpAddr::V4(ip), port)], + Some(url::Host::Ipv6(ip)) => vec![SocketAddr::new(IpAddr::V6(ip), port)], + _ => tokio::net::lookup_host((host.as_str(), port)) + .await + .map_err(|_| Refusal::Unreached)? + .collect(), + }; + // Every answer, not the first: a name that resolves to one public and one + // private address is refused, whichever the client would have picked. + if addresses.is_empty() { + return Err(Refusal::Unreached); + } + if !addresses.iter().all(|address| policy.admits(address.ip())) { + return Err(Refusal::Address); + } + let pinned = addresses[0]; + if plain && !pinned.ip().is_loopback() { + return Err(Refusal::Destination); + } + let client = crate::outbound::builder(timeout) + .resolve(&host, pinned) + .build() + .map_err(|_| Refusal::Unreached)?; + let mut response = client + .get(url) + .send() + .await + .map_err(|_| Refusal::Unreached)?; + if !response.status().is_success() { + return Err(Refusal::Status(response.status().as_u16())); + } + let declared = response + .headers() + .get(reqwest::header::CONTENT_TYPE) + .and_then(|value| value.to_str().ok()) + .and_then(|value| value.split(';').next()) + .map(|value| value.trim().to_ascii_lowercase()) + .and_then(|value| format_of_mime(&value)) + .ok_or(Refusal::NotAnImage)?; + if response + .content_length() + .is_some_and(|length| length > MAX_IMAGE_BYTES as u64) + { + return Err(Refusal::TooLarge); + } + let mut bytes = Vec::new(); + while let Some(chunk) = response.chunk().await.map_err(|_| Refusal::Unreached)? { + if bytes.len() + chunk.len() > MAX_IMAGE_BYTES { + return Err(Refusal::TooLarge); + } + bytes.extend_from_slice(&chunk); + } + let format = sniff(&bytes).ok_or(Refusal::NotAnImage)?; + if format != declared { + return Err(Refusal::NotAnImage); + } + Ok(Portrait { + hash: blake3::hash(&bytes).to_hex().to_string(), + format, + bytes, + }) +} + +fn format_of_mime(mime: &str) -> Option<&'static str> { + match mime { + "image/jpeg" | "image/jpg" => Some("jpg"), + "image/png" => Some("png"), + "image/webp" => Some("webp"), + _ => None, + } +} + +/// The format the bytes are, among the three the artwork route serves. +fn sniff(bytes: &[u8]) -> Option<&'static str> { + if bytes.starts_with(&[0xff, 0xd8, 0xff]) { + Some("jpg") + } else if bytes.starts_with(b"\x89PNG\r\n\x1a\n") { + Some("png") + } else if bytes.len() >= 12 && &bytes[..4] == b"RIFF" && &bytes[8..12] == b"WEBP" { + Some("webp") + } else { + None + } +} + +/// Puts `portrait` in the artwork store, `.` under +/// `artwork_dir`, unless it is already there. +/// +/// Written beside the target and renamed into place, so a reader never sees +/// half a file. Called before the writer gate is taken: this is file I/O. +pub async fn store(artwork_dir: &Path, portrait: &Portrait) -> std::io::Result<()> { + let target = artwork_dir.join(format!("{}.{}", portrait.hash, portrait.format)); + if tokio::fs::try_exists(&target).await? { + return Ok(()); + } + tokio::fs::create_dir_all(artwork_dir).await?; + let staging = artwork_dir.join(format!("{}.part", uuid::Uuid::new_v4().simple())); + tokio::fs::write(&staging, &portrait.bytes).await?; + if let Err(error) = tokio::fs::rename(&staging, &target).await { + let _ = tokio::fs::remove_file(&staging).await; + // Another fill stored the same bytes first; the content is identical. + if tokio::fs::try_exists(&target).await.unwrap_or(false) { + return Ok(()); + } + return Err(error); + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + /// The protection the suite cannot exercise end to end, since its image + /// host is on loopback: the default refuses loopback and every private + /// range, and admits a public address. + #[test] + fn the_default_policy_refuses_a_loopback_address() { + let policy = AddressPolicy::default(); + for refused in [ + "127.0.0.1", + "::1", + "::ffff:127.0.0.1", + "10.1.2.3", + "172.16.0.1", + "192.168.1.1", + "169.254.169.254", + "100.64.0.1", + "0.0.0.0", + "::", + "fd00::1", + "fe80::1", + "::ffff:192.168.1.1", + ] { + assert!(!policy.admits(refused.parse().unwrap()), "{refused}"); + } + assert!(policy.admits("93.184.216.34".parse().unwrap())); + assert!(policy.admits("2606:4700::1111".parse().unwrap())); + assert!(AddressPolicy::AllowLoopback.admits("127.0.0.1".parse().unwrap())); + assert!(!AddressPolicy::AllowLoopback.admits("10.0.0.1".parse().unwrap())); + } + + #[test] + fn the_bytes_decide_the_format() { + assert_eq!(sniff(&[0xff, 0xd8, 0xff, 0xe0]), Some("jpg")); + assert_eq!(sniff(b"\x89PNG\r\n\x1a\n...."), Some("png")); + assert_eq!(sniff(b"RIFF\0\0\0\0WEBPVP8 "), Some("webp")); + assert_eq!(sniff(b""), None); + } +} diff --git a/src/services/artist_enrichment.rs b/src/services/artist_enrichment.rs index bd4ada49..3ebb5d59 100644 --- a/src/services/artist_enrichment.rs +++ b/src/services/artist_enrichment.rs @@ -353,6 +353,7 @@ mod tests { outcome, bio_summary: bio.map(str::to_owned), bio_full: None, + image_hash: None, fetched_at, expires_at: fetched_at + 1, } diff --git a/src/services/artwork.rs b/src/services/artwork.rs index de696999..cdd15103 100644 --- a/src/services/artwork.rs +++ b/src/services/artwork.rs @@ -40,6 +40,12 @@ //! reported as a dead link meanwhile, rather than bytes that no longer exist //! anywhere. //! +//! An artist portrait (RFC-011 decision 8) is the exception: its bytes came +//! from a provider, not from a file on disk, and no scan writes them back. The +//! window costs it more — a dead link until the cache entry expires and the +//! next visit downloads it again, up to the hit TTL — and is the same width, +//! since its row is committed in one transaction under this same gate. +//! //! That is an argument for tolerating the window, not for pretending it is //! shut. It is narrow, it needs a scan and a sweep to interleave at one //! instant, and no test here forces that ordering. diff --git a/tests/enrichment.rs b/tests/enrichment.rs index 322ae0e6..530c66c5 100644 --- a/tests/enrichment.rs +++ b/tests/enrichment.rs @@ -30,6 +30,7 @@ use axum::{ use waveflow_server::{ config::MetadataSettings, enrichment::{Endpoints, Enrichment, Outcome, Provider}, + portrait::AddressPolicy, }; #[derive(Clone, Copy)] @@ -152,8 +153,10 @@ fn engine( Enrichment::with_endpoints( state.db.clone(), settings, + state.artwork_dir.clone(), Endpoints { lastfm: listener.base_url.clone(), + ..Endpoints::default() }, ) .unwrap() @@ -446,8 +449,10 @@ async fn the_key_never_reaches_the_log() { let closed = Enrichment::with_endpoints( state.db.clone(), &settings, + state.artwork_dir.clone(), Endpoints { lastfm: "http://127.0.0.1:1/2.0/".to_owned(), + ..Endpoints::default() }, ) .unwrap(); @@ -1061,3 +1066,290 @@ async fn a_neighbour_s_album_of_guests_takes_one_share() { assert_eq!(songs.len(), 4); assert_eq!(own, 2); } + +// ── Portraits: Deezer and the narrow fetcher (decision 8) ── + +/// A fake Deezer: the artist search, and the picture it points at, served by +/// the same loopback listener under whatever host name the test chooses. +#[derive(Clone)] +struct Deezer { + searches: Arc, + images: Arc, + picture: Arc, + bytes: Arc>, +} + +struct DeezerListener { + fake: Deezer, + base_url: String, +} + +/// A JPEG by its first bytes, which is all the fetcher checks; the rest is +/// made up for the run so two runs never share a hash. +fn portrait_bytes() -> Vec { + let mut bytes = vec![0xff, 0xd8, 0xff, 0xe0]; + bytes.extend_from_slice(run_key().as_bytes()); + bytes +} + +async fn deezer_search(State(fake): State) -> Response { + fake.searches.fetch_add(1, Ordering::SeqCst); + json( + serde_json::json!({"data": [ + {"id": 1, "name": "Björk", "picture_xl": *fake.picture} + ]}) + .to_string(), + ) +} + +async fn deezer_picture(State(fake): State) -> Response { + fake.images.fetch_add(1, Ordering::SeqCst); + ( + [(header::CONTENT_TYPE, "image/jpeg")], + (*fake.bytes).clone(), + ) + .into_response() +} + +async fn fake_deezer(picture_host: &str) -> DeezerListener { + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let port = listener.local_addr().unwrap().port(); + let fake = Deezer { + searches: Arc::default(), + images: Arc::default(), + picture: Arc::new(format!("http://{picture_host}:{port}/img/portrait.jpg")), + bytes: Arc::new(portrait_bytes()), + }; + let router = Router::new() + .route("/search/artist", get(deezer_search)) + .route("/img/portrait.jpg", get(deezer_picture)) + .with_state(fake.clone()); + tokio::spawn(async move { axum::serve(listener, router).await.unwrap() }); + DeezerListener { + fake, + base_url: format!("http://127.0.0.1:{port}"), + } +} + +/// An engine asking the fake Deezer, fetching pictures from `hosts` under +/// `policy`. +fn deezer_engine( + state: &waveflow_server::AppState, + settings: &MetadataSettings, + listener: &DeezerListener, + hosts: &[&str], + policy: AddressPolicy, +) -> Enrichment { + Enrichment::with_endpoints( + state.db.clone(), + settings, + state.artwork_dir.clone(), + Endpoints { + deezer: listener.base_url.clone(), + deezer_image_hosts: hosts.iter().map(|host| (*host).to_owned()).collect(), + image_addresses: policy, + ..Endpoints::default() + }, + ) + .unwrap() +} + +/// Björk in a library of each of two accounts: two artist rows, one name. +async fn bjork_twice() -> ( + tempfile::TempDir, + waveflow_server::AppState, + uuid::Uuid, + uuid::Uuid, +) { + let (temp, config, state) = support::test_app().await; + let owner = account(&state, "portrait-owner").await; + let other = account(&state, "portrait-other").await; + library_of(&state, &config, owner, "mine", &["Björk"]).await; + library_of(&state, &config, other, "theirs", &["Björk"]).await; + (temp, state, owner, other) +} + +async fn artist_events(state: &waveflow_server::AppState) -> i64 { + sqlx::query_scalar("SELECT COUNT(*) FROM library_event WHERE entity_type = 'artist'") + .fetch_one(state.db.pool()) + .await + .unwrap() +} + +async fn portrait_of(state: &waveflow_server::AppState, user: uuid::Uuid) -> Option { + let bjork = artist_id(state, user, "Björk").await; + state + .services + .artist(user, bjork) + .await + .unwrap() + .artist + .artwork_hash +} + +/// Decision 8 end to end: the picture is fetched once, stored in the artwork +/// store under its hash, and every artist row with the name shows it — one +/// event for each, in its own library. +#[tokio::test] +async fn a_portrait_is_fetched_once_and_shown_on_every_artist_with_the_name() { + let (_temp, state, owner, other) = bjork_twice().await; + let listener = fake_deezer("127.0.0.1").await; + let enrichment = deezer_engine( + &state, + &settings(vec![Provider::Deezer], &run_key()), + &listener, + &["127.0.0.1"], + AddressPolicy::AllowLoopback, + ); + enrichment.artist("Björk").await.unwrap(); + enrichment.idle().await; + + let mine = portrait_of(&state, owner).await; + let hash = mine.clone().expect("the portrait is linked"); + assert_eq!(portrait_of(&state, other).await, mine); + assert!(state.artwork_dir.join(format!("{hash}.jpg")).is_file()); + let source: String = sqlx::query_scalar("SELECT source FROM artwork WHERE hash = ?") + .bind(&hash) + .fetch_one(state.db.pool()) + .await + .unwrap(); + assert_eq!(source, "deezer"); + assert_eq!(artist_events(&state).await, 2); + assert_eq!(listener.fake.images.load(Ordering::SeqCst), 1); +} + +/// A picture URL naming a host off the allowlist is dropped without being +/// contacted, and the answer is cached as a miss rather than retried. +#[tokio::test] +async fn a_host_outside_the_allowlist_is_never_contacted() { + let (_temp, state, owner, _other) = bjork_twice().await; + let listener = fake_deezer("127.0.0.1").await; + let enrichment = deezer_engine( + &state, + &settings(vec![Provider::Deezer], &run_key()), + &listener, + &["localhost"], + AddressPolicy::AllowLoopback, + ); + enrichment.artist("Björk").await.unwrap(); + enrichment.idle().await; + + assert_eq!(listener.fake.searches.load(Ordering::SeqCst), 1); + assert_eq!(listener.fake.images.load(Ordering::SeqCst), 0); + let rows = enrichment.artist("Björk").await.unwrap(); + assert_eq!(rows[0].outcome, Outcome::Miss); + assert_eq!(portrait_of(&state, owner).await, None); +} + +/// Production's policy, with the fake's own host allowlisted: still never +/// contacted, because it is on loopback. +#[tokio::test] +async fn the_default_policy_does_not_reach_a_loopback_host() { + let (_temp, state, _owner, _other) = bjork_twice().await; + let listener = fake_deezer("127.0.0.1").await; + let enrichment = deezer_engine( + &state, + &settings(vec![Provider::Deezer], &run_key()), + &listener, + &["127.0.0.1"], + AddressPolicy::default(), + ); + enrichment.artist("Björk").await.unwrap(); + enrichment.idle().await; + + assert_eq!(listener.fake.searches.load(Ordering::SeqCst), 1); + assert_eq!(listener.fake.images.load(Ordering::SeqCst), 0); +} + +/// The artwork row, the cache row and the artist link are one transaction: +/// when the link cannot be written, neither is the artwork row — nothing is +/// left for the sweeper to find unreferenced, and nothing claims an image. +#[tokio::test] +async fn the_artwork_row_and_the_hash_land_together_or_not_at_all() { + let (_temp, state, _owner, _other) = bjork_twice().await; + sqlx::query( + "CREATE TRIGGER refuse_portrait BEFORE UPDATE OF artwork_hash ON artist \ + BEGIN SELECT RAISE(ABORT, 'refused by the test'); END", + ) + .execute(state.db.pool()) + .await + .unwrap(); + let listener = fake_deezer("127.0.0.1").await; + let enrichment = deezer_engine( + &state, + &settings(vec![Provider::Deezer], &run_key()), + &listener, + &["127.0.0.1"], + AddressPolicy::AllowLoopback, + ); + enrichment.artist("Björk").await.unwrap(); + enrichment.idle().await; + + assert_eq!(listener.fake.images.load(Ordering::SeqCst), 1); + let artwork: i64 = sqlx::query_scalar("SELECT COUNT(*) FROM artwork WHERE source = 'deezer'") + .fetch_one(state.db.pool()) + .await + .unwrap(); + let cached: i64 = sqlx::query_scalar("SELECT COUNT(*) FROM metadata_artist") + .fetch_one(state.db.pool()) + .await + .unwrap(); + assert_eq!((artwork, cached), (0, 0)); +} + +/// RFC-007 decision 6's rule: an event only when `artwork_hash` moved. The +/// row expires at once, so the second visit fetches the same picture again, +/// and says nothing about it. +#[tokio::test] +async fn an_unchanged_portrait_emits_no_event() { + let (_temp, state, _owner, _other) = bjork_twice().await; + let listener = fake_deezer("127.0.0.1").await; + let enrichment = deezer_engine( + &state, + &MetadataSettings { + ttl: Duration::ZERO, + ..settings(vec![Provider::Deezer], &run_key()) + }, + &listener, + &["127.0.0.1"], + AddressPolicy::AllowLoopback, + ); + enrichment.artist("Björk").await.unwrap(); + enrichment.idle().await; + assert_eq!(artist_events(&state).await, 2); + enrichment.artist("Björk").await.unwrap(); + enrichment.idle().await; + + assert_eq!(listener.fake.images.load(Ordering::SeqCst), 2); + assert_eq!(artist_events(&state).await, 2); +} + +/// An artist row that lost its portrait — re-identified by a PID spec change, +/// or scanned since — is linked again from the cache on the next visit, +/// without a second download. +#[tokio::test] +async fn a_cached_portrait_is_linked_again_without_a_second_download() { + let (_temp, state, owner, _other) = bjork_twice().await; + let listener = fake_deezer("127.0.0.1").await; + let enrichment = deezer_engine( + &state, + &settings(vec![Provider::Deezer], &run_key()), + &listener, + &["127.0.0.1"], + AddressPolicy::AllowLoopback, + ); + enrichment.artist("Björk").await.unwrap(); + enrichment.idle().await; + let linked = portrait_of(&state, owner).await; + sqlx::query("UPDATE artist SET artwork_hash = NULL") + .execute(state.db.pool()) + .await + .unwrap(); + + enrichment.artist("Björk").await.unwrap(); + enrichment.idle().await; + + assert!(linked.is_some()); + assert_eq!(portrait_of(&state, owner).await, linked); + assert_eq!(listener.fake.images.load(Ordering::SeqCst), 1); +} From fb89e79fcf2ba23a0cb7eedf6e40bfed7d151ec6 Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Fri, 16 Oct 2026 00:30:27 +0200 Subject: [PATCH 2/6] fix(enrichment): record the artwork source from the provider, not a literal Signed-off-by: InstaZDLL --- src/database.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/database.rs b/src/database.rs index e18fe92e..2c9c4063 100644 --- a/src/database.rs +++ b/src/database.rs @@ -2029,10 +2029,11 @@ impl Database { if let Some(image) = &answer.image { sqlx::query( "INSERT INTO artwork (hash, format, source, byte_size, created_at) \ - VALUES (?, ?, 'deezer', ?, ?) ON CONFLICT (hash) DO NOTHING", + VALUES (?, ?, ?, ?, ?) ON CONFLICT (hash) DO NOTHING", ) .bind(&image.hash) .bind(image.format) + .bind(provider.as_str()) .bind(image.byte_size) .bind(fetched_at) .execute(&mut *tx) From 371b3bda1fc9085c65fa972265fde0a9b87d9c0b Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Fri, 16 Oct 2026 00:32:13 +0200 Subject: [PATCH 3/6] fix(enrichment): refuse the ranges that carry an ipv4 address to a gateway Signed-off-by: InstaZDLL --- src/portrait.rs | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/src/portrait.rs b/src/portrait.rs index e9fa7c51..5c86d18b 100644 --- a/src/portrait.rs +++ b/src/portrait.rs @@ -72,17 +72,30 @@ fn is_public_v4(ip: Ipv4Addr) -> bool { // 100.64.0.0/10, carrier-grade NAT: another operator's private range. || (a == 100 && (64..128).contains(&b)) // 0.0.0.0/8, "this network". - || a == 0) + || a == 0 + // 198.18.0.0/15, benchmarking: routed inside some private networks. + || (a == 198 && (b == 18 || b == 19)) + // 240.0.0.0/4, reserved. + || a >= 240) } fn is_public_v6(ip: Ipv6Addr) -> bool { - let first = ip.segments()[0]; + let segments = ip.segments(); + let first = segments[0]; !(ip.is_unspecified() || ip.is_multicast() // fc00::/7, unique local. || (first & 0xfe00) == 0xfc00 - // fe80::/10, link local. - || (first & 0xffc0) == 0xfe80) + // fe80::/10, link local, and fec0::/10, the deprecated site local. + || (first & 0xffc0) == 0xfe80 + || (first & 0xffc0) == 0xfec0 + // Prefixes that carry an IPv4 address a gateway connects to, which + // may be a private one: 64:ff9b::/96 (NAT64), 64:ff9b:1::/48 (local + // NAT64) and 2002::/16 (6to4). Refused whole rather than decoded. + || (first == 0x0064 && segments[1] == 0xff9b) + || first == 0x2002 + // ::/96, the deprecated IPv4-compatible form. + || segments[..6].iter().all(|segment| *segment == 0)) } /// A fetched portrait, checked and hashed, not yet stored. @@ -308,6 +321,13 @@ mod tests { "fd00::1", "fe80::1", "::ffff:192.168.1.1", + "198.18.0.1", + "240.0.0.1", + "fec0::1", + "64:ff9b::a00:1", + "64:ff9b:1::1", + "2002:a00:1::1", + "::10.0.0.1", ] { assert!(!policy.admits(refused.parse().unwrap()), "{refused}"); } From ae9dd01eb0d193b28cfe31cd4531c0db8872a966 Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Fri, 16 Oct 2026 00:34:09 +0200 Subject: [PATCH 4/6] fix(enrichment): admit ipv6 by global unicast rather than by a denylist Signed-off-by: InstaZDLL --- src/portrait.rs | 42 +++++++++++++++++++++++++----------------- 1 file changed, 25 insertions(+), 17 deletions(-) diff --git a/src/portrait.rs b/src/portrait.rs index 5c86d18b..10e99017 100644 --- a/src/portrait.rs +++ b/src/portrait.rs @@ -76,26 +76,28 @@ fn is_public_v4(ip: Ipv4Addr) -> bool { // 198.18.0.0/15, benchmarking: routed inside some private networks. || (a == 198 && (b == 18 || b == 19)) // 240.0.0.0/4, reserved. - || a >= 240) + || a >= 240 + // 192.0.0.0/24, protocol assignments, and 192.88.99.0/24, the 6to4 + // relay anycast. + || ip.octets()[..3] == [192, 0, 0] + || ip.octets()[..3] == [192, 88, 99]) } +/// IPv6 is admitted by what it is, not by what it is not: global unicast, +/// `2000::/3`, is the only space a public host answers from, and everything +/// outside it — unique and link local, site local, NAT64, IPv4-mapped, +/// IPv4-compatible, discard — is refused without being listed. Inside it, +/// the prefixes that carry an IPv4 address a gateway connects to (which may +/// be a private one) are refused whole rather than decoded. fn is_public_v6(ip: Ipv6Addr) -> bool { - let segments = ip.segments(); - let first = segments[0]; - !(ip.is_unspecified() - || ip.is_multicast() - // fc00::/7, unique local. - || (first & 0xfe00) == 0xfc00 - // fe80::/10, link local, and fec0::/10, the deprecated site local. - || (first & 0xffc0) == 0xfe80 - || (first & 0xffc0) == 0xfec0 - // Prefixes that carry an IPv4 address a gateway connects to, which - // may be a private one: 64:ff9b::/96 (NAT64), 64:ff9b:1::/48 (local - // NAT64) and 2002::/16 (6to4). Refused whole rather than decoded. - || (first == 0x0064 && segments[1] == 0xff9b) - || first == 0x2002 - // ::/96, the deprecated IPv4-compatible form. - || segments[..6].iter().all(|segment| *segment == 0)) + let [first, second, ..] = ip.segments(); + (first & 0xe000) == 0x2000 + // 2001::/32, Teredo. + && !(first == 0x2001 && second == 0) + // 2001:db8::/32, documentation. + && !(first == 0x2001 && second == 0x0db8) + // 2002::/16, 6to4. + && first != 0x2002 } /// A fetched portrait, checked and hashed, not yet stored. @@ -328,6 +330,12 @@ mod tests { "64:ff9b:1::1", "2002:a00:1::1", "::10.0.0.1", + "192.0.0.1", + "192.88.99.1", + "2001:0:a00:1::1", + "2001:db8::1", + "100::1", + "::ffff:0:a00:1", ] { assert!(!policy.admits(refused.parse().unwrap()), "{refused}"); } From 771c364821426ef7a36cc90b982f98df8c01c6fc Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Fri, 16 Oct 2026 11:10:40 +0200 Subject: [PATCH 5/6] fix(enrichment): keep serving the artist when the portrait relink fails Signed-off-by: InstaZDLL --- src/enrichment.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/enrichment.rs b/src/enrichment.rs index a8f6b439..e62f6eb8 100644 --- a/src/enrichment.rs +++ b/src/enrichment.rs @@ -352,9 +352,13 @@ impl Enrichment { } // A portrait already fetched, and an artist row that does not show // it: an artist re-identified by a PID spec change, or one scanned - // since. Linked again from the cache, without a second download. + // since. Linked again from the cache, without a second download. A + // repair, not the answer: if it fails, the page is still served and + // the next visit tries again. if let Some(hash) = ordered.iter().find_map(|row| row.image_hash.as_deref()) { - engine.db.link_artist_portrait(&canonical, hash).await?; + if let Err(error) = engine.db.link_artist_portrait(&canonical, hash).await { + tracing::warn!(%error, "cached artist portrait could not be linked"); + } } Ok(ordered) } From e41d6e14c40b6e7f1135c87cd4b60dcc4d33da8c Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Fri, 16 Oct 2026 11:34:01 +0200 Subject: [PATCH 6/6] fix(enrichment): relink only a portrait the cache still names Signed-off-by: InstaZDLL --- src/database.rs | 74 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 73 insertions(+), 1 deletion(-) diff --git a/src/database.rs b/src/database.rs index 2c9c4063..ca122210 100644 --- a/src/database.rs +++ b/src/database.rs @@ -2095,7 +2095,9 @@ impl Database { /// /// Artist rows are per library, and so are events, so each moved row carries /// its own library. A row whose artwork row is gone is not linked: the cache -/// may name a hash the sweeper has since taken. +/// may name a hash the sweeper has since taken. Nor is a hash the cache no +/// longer names: a read that saw the old row, then repairs after a fill +/// replaced it, would put the old portrait back and announce it. async fn link_portrait_in( tx: &mut sqlx::Transaction<'_, sqlx::Sqlite>, name_canonical: &str, @@ -2106,6 +2108,8 @@ async fn link_portrait_in( "UPDATE artist SET artwork_hash = ?, updated_at = ? \ WHERE canonical_name = ? AND artwork_hash IS NOT ? \ AND EXISTS (SELECT 1 FROM artwork WHERE artwork.hash = ?) \ + AND EXISTS (SELECT 1 FROM metadata_artist \ + WHERE name_canonical = ? AND image_hash = ?) \ RETURNING id, library_id", ) .bind(hash) @@ -2113,6 +2117,8 @@ async fn link_portrait_in( .bind(name_canonical) .bind(hash) .bind(hash) + .bind(name_canonical) + .bind(hash) .fetch_all(&mut **tx) .await?; for row in moved { @@ -2648,4 +2654,70 @@ mod tests { ); } } + + /// A read takes the cache row without the gate, so a fill can replace and + /// link a newer portrait before the read's repair runs. That repair must + /// not put the hash it read back over the newer one, nor announce it. + #[tokio::test] + async fn a_repair_does_not_restore_a_portrait_the_cache_replaced() { + let (_temp, database) = migrated_database().await; + const ACCOUNT: &str = "6f1d1b6e-0f1a-4a3d-9c21-000000000021"; + const LIBRARY: &str = "6f1d1b6e-0f1a-4a3d-9c21-000000000022"; + const ARTIST: &str = "6f1d1b6e-0f1a-4a3d-9c21-000000000023"; + let read = "a".repeat(64); + let replaced = "b".repeat(64); + for (statement, binds) in [ + ( + "INSERT INTO account (id, username, password_hash, role, created_at, updated_at) \ + VALUES (?, 'owner', 'this-account-never-authenticates', 'admin', 0, 0)", + vec![ACCOUNT], + ), + ( + "INSERT INTO library (id, owner_user_id, name, root_path, visibility, \ + created_at, updated_at) \ + VALUES (?, ?, 'Library', '/music', 'private', 0, 0)", + vec![LIBRARY, ACCOUNT], + ), + ( + "INSERT INTO artwork (hash, format, source, byte_size, created_at) \ + VALUES (?, 'jpg', 'deezer', 1, 0), (?, 'jpg', 'deezer', 1, 0)", + vec![read.as_str(), replaced.as_str()], + ), + ( + "INSERT INTO artist (id, library_id, name, canonical_name, artwork_hash, \ + created_at, updated_at) \ + VALUES (?, ?, 'Nova Kern', 'nova kern', ?, 0, 0)", + vec![ARTIST, LIBRARY, replaced.as_str()], + ), + ( + "INSERT INTO metadata_artist (provider, name_canonical, outcome, image_hash, \ + fetched_at, expires_at) \ + VALUES ('deezer', 'nova kern', 'hit', ?, 0, 0)", + vec![replaced.as_str()], + ), + ] { + let mut query = sqlx::query(statement); + for bind in binds { + query = query.bind(bind.to_owned()); + } + query.execute(&database.pool).await.expect("seed row"); + } + + database + .link_artist_portrait("nova kern", &read) + .await + .expect("repair"); + + let linked: String = sqlx::query_scalar("SELECT artwork_hash FROM artist WHERE id = ?") + .bind(ARTIST) + .fetch_one(&database.pool) + .await + .expect("artist row"); + assert!(linked == replaced, "the newer portrait was replaced"); + let events: i64 = sqlx::query_scalar("SELECT COUNT(*) FROM library_event") + .fetch_one(&database.pool) + .await + .expect("event count"); + assert_eq!(events, 0); + } }