diff --git a/docs/subsonic-compatibility.md b/docs/subsonic-compatibility.md index c1c1f1b..4ff00e7 100644 --- a/docs/subsonic-compatibility.md +++ b/docs/subsonic-compatibility.md @@ -4,26 +4,42 @@ Automated protocol coverage is enforced by `tests/v2_foundations.rs` for XML, JS | Client | Version | Login | Browse/search | Native/transcode | Playlists/user data | Status | |---|---:|---:|---:|---:|---:|---| -| Symfonium | 14.1.0 | pass | pass | pass | pass | Validated 2026-08-09 on Android 17 through an ephemeral `cloudflared` HTTPS tunnel: account and API-key authentication, full catalogue sync, native MP3/FLAC Range playback, Opus at 64 kbit/s, album favorite, scrobbles and playlist create/update. | -| Feishin | 1.15.1 | pass | pass | pass | pass | Validated 2026-08-02: native playback, Opus cache, playlist create/add, favorite, rating, scrobble and queue. | -| Substreamer | 8.0.91 | pass | pass | pass | pass* | Validated 2026-08-02 from the official release source: native playback, Opus/128 cache, playlist add, favorite, rating and scrobble. `*` Its playback queue is local-only and the client never calls `getPlayQueue`/`savePlayQueue`; those endpoints remain covered by fixtures and Feishin. | -| DSub | 5.5.3 (F-Droid 208) | pass | pass | pass | pass* | Validated 2026-08-02 on Android 14, then revalidated 2026-08-15 on the current Android environment: authentication, catalogue, artwork, native playback, seek and playlist. `*` Scrobbling was disabled in the original run; the endpoint remains covered by fixtures and other clients. | -| Juliet | iOS build tested 2026-08-15 | pass | pass | native pass; transcode not run | not run | Current iOS compatibility check: authentication, catalogue, artwork and native playback succeeded. Unsupported/unexercised surfaces are not inferred as passes. | +| Symfonium | 14.1.0 | pass | pass | pass | pass | Re-run 2026-08-19 against the current contract, on an Android 17 emulator through an ephemeral `cloudflared` HTTPS tunnel. Every observable change of the six batches was checked against server state rather than against the client's own display: sleeve order in both the album and a synced playlist, two distinct album artists with no composite entity in the artist list, two album genres, a genre spelled four ways answering as one (4 tracks), the extended fields including `explicitStatus`, and a failed login decoded correctly now that it arrives as HTTP 200. User data round-tripped: track and album ratings, track/album/artist favorites, 21 scrobbles, a bookmark resumed at 37 s, three playlists owned by the authenticated user, and 13 Opus cache entries across the 64 and 128 kbit/s ceilings. The earlier 2026-08-09 run on a physical Android 17 device additionally covered API-key authentication, which this one did not re-run. | +| Feishin | 1.15.1 | pass | pass | pass | pass | Re-run 2026-08-19 against the current contract, on Windows desktop. **This run found the two server defects fixed in the same change**: a cold transcode refused the `Range: bytes=0-` that a browser sends to open any resource, so playback failed on every track no earlier client had transcoded; and `createPlaylist` appended its `songId` values instead of replacing the list, so every playlist edit looked lost. Both were then replayed against the fixed server through a logging proxy and confirmed from server state: cold transcode, cache hit and seek all answer, a removal drops to one track, a reorder lands. The rest of the run exercised search (23 `search3`), 145 cover-art reads, genre browsing including 8 `getSongsByGenre`, artists, `getStarred`, `getRandomSongs`, 9 scrobbles, favorites on tracks/album/artists and ratings — all read back from the server rather than from the client. `getAlbumInfo`/`getAlbumInfo2` were never called by this version, so they stay unexercised here rather than counted as a pass, as do the bookmark and play-queue methods. `getTopSongs`, `getArtistInfo` and `getInternetRadioStations` answer empty containers, which is what this server has to say about them. | +| Substreamer | 8.0.91 | pass | pass | pass | pass* | Validated 2026-08-02 from the official release source: native playback, Opus/128 cache, playlist add, favorite, rating and scrobble. `*` Its playback queue is local-only and the client never calls `getPlayQueue`/`savePlayQueue`; those endpoints remain covered by fixtures, and by Feishin's 2026-08-02 run. | +| DSub | 5.5.3 (F-Droid 208) | pass | pass | pass | pass | Re-run 2026-08-19 against the current contract, on an Android 17 emulator, read from `logcat -s RESTMusicService` and DSub's own disk cache rather than from its interface. **This run found the album-artist defect fixed in the same change**: an album credited to two artists hung off a third entity named after the joined string, and browsing to either real artist found no album. The rest passed: sleeve order in both browse modes, two album genres, one canonical `Jazz` answering 4 tracks, artwork, native FLAC and MP3 with seek, a failed login decoded as an error now that it arrives as HTTP 200, playlist create/add/remove/rename/delete — the removal going through `songIndexToRemove`, by index — and a rating. It settles a question Symfonium left open: DSub does submit the scrobble of the last track in a queue. Two of its three unique surfaces needed the brief corrected. The legacy `getAlbumList` is used **only** when "Browse By Tags" is off, `getAlbumList2` otherwise; both were exercised. And `maxBitRate` is never sent above the source — DSub emits `min(setting, track bitrate)` — so "ceiling above the source" is not expressible from this client; the two reachable cases are correct (128 ceiling on a 128 kbit/s MP3 streams natively, 64 transcodes). The generic `star?id` form resolved a track, an album and an artist, all three surviving a resync. Unresolved, and not a server fault: one FLAC (`Drift`) is fetched repeatedly and never completed by the client, though the server answers 200 with the exact `content-length` and the bytes arrive md5-identical to the source file through the same tunnel. Absent from the client, recorded as absent: the explicit marker, and any separate display of two album artists. | +| Juliet | 1.5 (iOS 26.6) | pass | pass | pass | pass | Re-run 2026-08-19 against the current contract, on a physical iPhone — the previous entry was a narrower check that never reached transcoding or user data. **This run found the second half of the cold-transcode defect**: iOS AVFoundation opens a resource with a `bytes=0-1` probe, which the first fix still classified as a seek and refused, so the first play of each track failed and the second succeeded off the cache the failed request had built. Fixed in the same change and verified by replaying the client's exact sequence: `bytes=0-1` cold answers 200, warm answers 206 with two bytes, a real seek answers 206, and a seek into a cold transcode is still refused. The rest of the run: 306 cover-art reads, 115 streams, 64 scrobbles, `getAlbumList2`, `search3`, `getRandomSongs`, `getSongsByGenre`, `getSimilarSongs2`, `getLyrics`, favorites, and playlist create/update/delete. It is also the first client in the replayed set to call `savePlayQueue`/`getPlayQueue` - 48 times - where the matrix otherwise rests on fixtures and on Feishin's historical 2026-08-02 run, its 2026-08-19 re-run having never called them. Absent from the client, recorded as absent: Juliet has no rating feature, so `setRating` was never sent; and it saves its queue with `position=0` every time, so playback restarts at zero — the server stores and returns what it is given, queue, current track and `changedBy` included. | -**Every row above predates the move to HTTP 200 on every `/rest` answer.** Each -records a real run against the previous status behaviour and is kept as a -historical result; none of them validates the current Subsonic contract. The M3 -real-client gate was closed on that earlier behaviour, so it is not a satisfied -gate for the next tag: every client must be re-run against the current contract -and its row re-dated first. Missing client features stay covered by automated -fixtures and another real client rather than inferred as passes. Creating any tag -or release remains a separate action requiring an explicit operator request. +**Symfonium, Feishin, DSub and Juliet were all re-run on 2026-08-19 against the +current contract**, each on a real device or desktop and each read from server +state rather than from the client's own display. Substreamer is out of the +replayed set for the reason given below, and its row remains a historical +result against the previous status behaviour. + +The replay found **four server defects**, all fixed in the same change and none +of which the automated suite could have produced: it took a browser, a client +that edits a playlist by sending back what remains, a client that browses by +artist index, and an iOS player that probes a resource before reading it. +Missing client features stay covered by automated fixtures and another real +client rather than inferred as passes. + +Feishin and Juliet found their defect rather than passing it, so each one +re-played the fixed path itself rather than being credited on a replayed +request sequence: Feishin's MP3 transcode now starts, caches and seeks, and +Juliet's two-byte probe answers 200 cold with no refusal left in the log. The +five rows therefore record the behaviour of the server as it stands. Creating +any tag or release remains a separate action requiring an explicit operator +request. The Substreamer row records the successful 2026-08-02 run. It could not be reinstalled on the current Android 17 device during the 2026-08-15 revalidation -because the store marks that legacy build incompatible; Juliet provides the -current iOS sanity check instead. The historical Substreamer evidence is kept, -not silently rewritten as a new run. +because the store marks that legacy build incompatible, and the 2026-08-19 +re-run confirmed it: that build no longer launches on the current emulator. It +is therefore **out of the replayed set** — its row stays as historical evidence +of a real run against the old contract, and is not counted toward the next tag. +Juliet on a physical iPhone takes its place as the fifth client, which also +moves the iOS check off an emulator. The historical Substreamer evidence is +kept, not silently rewritten as a new run. For Substreamer, the Android Media3 session completed the 11-track validation album and the server recorded the corresponding start/submission scrobbles. With the client's streaming profile set to Opus at 128 kbit/s, WaveFlow produced 11 distinct cache entries; `ffprobe` identified the output as an Ogg container with an Opus audio stream. Its playlist mutation, track/album favorites and 4/5 rating were also read back from WaveFlow rather than inferred from local UI state. @@ -37,12 +53,11 @@ The contract audit additionally covers administrative folder access: `createUser Protocol failures now answer HTTP 200 and report the outcome in the body (`status="failed"` plus an `error` code), as the Subsonic contract requires. -Previously WaveFlow also set 401, 403, 404, 409 or 429. All five clients in the -matrix above were validated against that old behaviour. Each reads `error/code`, -so none is expected to break — but that is an expectation, not a result, and the -matrix records runs rather than inferences: **every row must be re-run and -re-dated before the next tag**. Range responses keep 206/416, and `/share` and -`/api/v2` are unchanged. +Previously WaveFlow also set 401, 403, 404, 409 or 429. That this would not +break the clients was an expectation until the 2026-08-19 replay made it a +result: **each re-run row above was given a deliberate wrong password and +decoded it as an authentication error**. Range responses keep 206/416, and +`/share` and `/api/v2` are unchanged. The same release adds `tokenInfo` — the half of `apiKeyAuthentication` that was advertised but never served — and `getAlbumInfo`/`getAlbumInfo2`, so Feishin @@ -51,8 +66,8 @@ opens. `playlist.owner` and `share.username` now carry the authenticated username instead of an empty string, which is what Feishin reads to decide whether a playlist is editable. -Five further batches of wire changes have landed against the same unvalidated -matrix, and the re-run covers them all: media items gained the remaining +Five further batches of wire changes landed while the matrix was still +unvalidated, and the 2026-08-19 replay covers them all: media items gained the remaining OpenSubsonic fields under the presence rule (`moods`, `explicitStatus`, `isrc`, `replayGain`, `bpm` and the rest); `startScan`, `getScanStatus`, `search2`, `getStarred` and the bookmark methods were added or backed by real state; diff --git a/src/catalog.rs b/src/catalog.rs index 7d4b605..37821bf 100644 --- a/src/catalog.rs +++ b/src/catalog.rs @@ -414,6 +414,84 @@ impl Database { .bind(library_id.to_string()) .execute(&mut *tx) .await?; + // Re-derive which artist an album hangs off, for the same reason the + // identifiers above are re-derived: a library indexed by an earlier + // version has its albums pointing at an entity named after the joined + // credit, and its files have not changed, so nothing would ever be + // reindexed. Deriving it here repairs those albums on the next scan + // instead of requiring a rebuilt library. + let joined: Vec<(String, String, String)> = sqlx::query_as( + "SELECT id, canonical_title, album_artist_name FROM album \ + WHERE library_id = ? AND album_artist_name LIKE '%;%'", + ) + .bind(library_id.to_string()) + .fetch_all(&mut *tx) + .await?; + let now = now_ms(); + for (album_id, canonical_title, credit) in joined { + let names = split_values(Some(credit.as_str())); + let mut first = None; + for name in &names { + let id = upsert_artist(&mut tx, library_id, name, now).await?; + first.get_or_insert(id); + } + let Some(first) = first else { continue }; + // The identity key embeds the album artist id, so moving the album + // without recomputing it would leave a row that indexing can no + // longer find: the next reindexed track would compute the new key, + // miss, and insert a duplicate album. + let identity = album_identity(&canonical_title, Some(first), Some(credit.as_str())); + let collision: Option = sqlx::query_scalar( + "SELECT id FROM album WHERE library_id = ? AND identity_key = ? AND id <> ?", + ) + .bind(library_id.to_string()) + .bind(&identity) + .bind(&album_id) + .fetch_optional(&mut *tx) + .await?; + match collision { + // Indexing already created the album this one is becoming, so + // the two are one record: the tracks move over and the stale + // row goes. Updating in place would violate the identity + // uniqueness and fail the whole scan. + Some(target) => { + sqlx::query("UPDATE track SET album_id = ? WHERE album_id = ?") + .bind(&target) + .bind(&album_id) + .execute(&mut *tx) + .await?; + sqlx::query("DELETE FROM album WHERE id = ?") + .bind(&album_id) + .execute(&mut *tx) + .await?; + } + None => { + sqlx::query( + "UPDATE album SET album_artist_id = ?, identity_key = ?, updated_at = ? \ + WHERE id = ?", + ) + .bind(first.to_string()) + .bind(&identity) + .bind(now) + .bind(&album_id) + .execute(&mut *tx) + .await?; + } + } + } + // An artist row outlives the tag that created it: rename a credit, or + // fix a server that used to mint one entity per joined album-artist + // string, and the old name stays in the index forever. An artist that + // holds neither a track nor an album is no longer part of the + // catalogue, so it goes with the same scan that made it unreferenced. + sqlx::query( + "DELETE FROM artist WHERE library_id = ? \ + AND NOT EXISTS (SELECT 1 FROM album al WHERE al.album_artist_id = artist.id) \ + AND NOT EXISTS (SELECT 1 FROM track_artist ta WHERE ta.artist_id = artist.id)", + ) + .bind(library_id.to_string()) + .execute(&mut *tx) + .await?; tx.commit().await?; Ok(()) } @@ -608,10 +686,20 @@ impl Database { .map(str::to_owned) .or_else(|| input.is_compilation.then(|| "Various Artists".to_owned())) .or_else(|| artist_names.first().cloned()); - let album_artist_id = match album_artist.as_deref() { - Some(name) => Some(upsert_artist(tx, library_id, name, now).await?), - None => None, - }; + // The album artist tag carries the same multi-value separator as the + // track artist tag, so it is split the same way. Feeding the whole + // string to upsert_artist created one entity named after the joined + // credit, gave it the album, and left the real artists holding + // nothing: browsing to either of them found no album at all. + // + // The album keeps the joined string as its display name, and hangs + // off the first credited artist. + let album_artist_names = split_values(album_artist.as_deref()); + let mut album_artist_id = None; + for name in &album_artist_names { + let id = upsert_artist(tx, library_id, name, now).await?; + album_artist_id.get_or_insert(id); + } let album_id = upsert_album( tx, library_id, @@ -960,13 +1048,7 @@ async fn upsert_album( return Ok(None); }; let canonical = waveflow_core::scanner::canonical_name(title); - let identity = format!( - "{}:{}", - canonical, - album_artist_id - .map(|id| id.to_string()) - .unwrap_or_else(|| "none".into()) - ); + let identity = album_identity(&canonical, album_artist_id, album_artist); let id = Uuid::new_v4(); let value: String = sqlx::query_scalar("INSERT INTO album (id, library_id, title, canonical_title, identity_key, album_artist_id, album_artist_name, is_compilation, year, artwork_hash, created_at, updated_at) \ VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) ON CONFLICT (library_id, identity_key) DO UPDATE SET title=excluded.title, album_artist_name=excluded.album_artist_name, \ @@ -977,6 +1059,40 @@ async fn upsert_album( parse_uuid(value).map(Some) } +/// How an album is recognised across scans. It embeds the album artist, so any +/// code that moves an album to another artist has to recompute it. +/// +/// A joined credit adds its whole canonical form. Hanging every album off its +/// first credited artist would otherwise merge two different records that share +/// a title and a lead: "Live" by `A; B` and "Live" by `A; C` are one key +/// without it. A single credit keeps the historical two-part shape, so the keys +/// already stored for the overwhelming majority of albums still match what +/// indexing computes. +fn album_identity( + canonical_title: &str, + album_artist_id: Option, + album_artist: Option<&str>, +) -> String { + let artist = album_artist_id + .map(|id| id.to_string()) + .unwrap_or_else(|| "none".into()); + let credits = split_values(album_artist); + if credits.len() > 1 { + // Canonicalised one credit at a time and rejoined on a separator the + // canonical form cannot contain. Canonicalising the joined string + // instead would erase where one credit ends and the next begins, so + // `A; B C` and `A; B; C` would answer the same key and merge. + let joined = credits + .iter() + .map(|credit| waveflow_core::scanner::canonical_name(credit)) + .collect::>() + .join(";"); + format!("{canonical_title}:{artist}:{joined}") + } else { + format!("{canonical_title}:{artist}") + } +} + fn split_values(raw: Option<&str>) -> Vec { raw.into_iter() .flat_map(|value| value.split(';')) diff --git a/src/media.rs b/src/media.rs index 2298534..b33680b 100644 --- a/src/media.rs +++ b/src/media.rs @@ -174,9 +174,14 @@ impl MediaService { return serve_file(&cache_path, range, mime_for_format(query.format)).await; } - // A byte range has no stable meaning until a transcode is complete. - // Clients seek a live transcode with offset_ms instead. - if range.is_some() { + // A byte range has no stable meaning until a transcode is complete, so + // seeking a live transcode goes through offset_ms instead. A range + // that starts at the first byte is not a seek though: it is how every + // player opens a resource. Refusing it made a client fail on the first + // play of a track and succeed on the second, once the cache existed — + // Feishin with `bytes=0-`, then Juliet with the `bytes=0-1` probe iOS + // sends before anything else. + if range.is_some_and(|value| !starts_at_the_first_byte(value)) { return Err(MediaError::RangeNotSatisfiable(0)); } @@ -205,7 +210,10 @@ impl MediaService { self.inner .cache_access .insert(cache_path.clone(), SystemTime::now()); - return serve_file(&cache_path, None, mime_for_format(query.format)).await; + // Whoever held the lock finished the transcode, so the range that + // had no meaning a moment ago has one now, and the second caller + // gets the same partial answer it would have got from the cache. + return serve_file(&cache_path, range, mime_for_format(query.format)).await; } self.stream_transcode( @@ -849,6 +857,29 @@ async fn serve_partial( Ok(response) } +/// Whether a `Range` header starts at the first byte, which is how a player +/// opens a resource rather than how it seeks into one. Browsers send `bytes=0-`; +/// iOS AVFoundation first probes with `bytes=0-1` to learn the size and type. +/// Both mean "begin here", and neither can be refused without breaking the +/// first play of every track. +/// +/// The answer to such a request is the whole stream rather than the bytes +/// asked for, which HTTP allows: a server may always ignore `Range` and answer +/// 200. Before the transcode exists there is no total length to put in a +/// `Content-Range`, so a partial answer is not available to give. +fn starts_at_the_first_byte(value: &str) -> bool { + value.strip_prefix("bytes=").is_some_and(|spec| { + // A multipart range is several ranges, and this answers with one body. + let Some((start, end)) = spec.trim().split_once('-') else { + return false; + }; + start.trim().parse::() == Ok(0) + // A malformed end - a second hyphen, a word - is not a range this + // understands, so it is refused rather than read as an open. + && (end.trim().is_empty() || end.trim().parse::().is_ok()) + }) +} + fn parse_range(value: &str, size: u64) -> Option<(u64, u64)> { if size == 0 { return None; @@ -857,7 +888,10 @@ fn parse_range(value: &str, size: u64) -> Option<(u64, u64)> { if value.contains(',') { return None; } - let (start, end) = value.split_once('-')?; + let (start, end) = value.trim().split_once('-')?; + // Trimmed on both bounds, like `starts_at_the_first_byte`: a spelling the + // cold path accepts must not be refused once the cache exists. + let (start, end) = (start.trim(), end.trim()); if start.is_empty() { let suffix = end.parse::().ok()?.min(size).min(MAX_RANGE_BYTES); if suffix == 0 { @@ -989,7 +1023,9 @@ mod tests { use dashmap::DashMap; use tokio::sync::Semaphore; - use super::{parse_range, prune_cache, secure_track_path, MediaInner}; + use super::{ + parse_range, prune_cache, secure_track_path, starts_at_the_first_byte, MediaInner, + }; #[test] fn ranges_are_bounded_and_validated() { @@ -999,6 +1035,30 @@ mod tests { assert_eq!(parse_range("bytes=10-", 10), None); assert_eq!(parse_range("bytes=5-2", 10), None); assert_eq!(parse_range("bytes=0-1,4-5", 10), None); + // Whitespace is tolerated on both bounds, so that a range the cold + // transcode path accepts is still satisfiable from the cache. + assert_eq!(parse_range("bytes= 0- ", 10), Some((0, 9))); + assert_eq!(parse_range("bytes= 2 - 5 ", 10), Some((2, 5))); + } + + #[test] + fn a_range_from_the_first_byte_opens_a_resource() { + // How a browser opens an audio element, and how iOS probes one. + assert!(starts_at_the_first_byte("bytes=0-")); + assert!(starts_at_the_first_byte("bytes=0-1")); + assert!(starts_at_the_first_byte("bytes=0-1023")); + assert!(starts_at_the_first_byte("bytes= 0- ")); + // Starting anywhere else is a seek, and has no meaning yet. + assert!(!starts_at_the_first_byte("bytes=1-")); + assert!(!starts_at_the_first_byte("bytes=19924-429882")); + assert!(!starts_at_the_first_byte("bytes=-3")); + // One body cannot answer several ranges. + assert!(!starts_at_the_first_byte("bytes=0-1,4-5")); + assert!(!starts_at_the_first_byte("items=0-")); + assert!(!starts_at_the_first_byte("bytes=00x-1")); + // A malformed end is not an open either. + assert!(!starts_at_the_first_byte("bytes=0-1-2")); + assert!(!starts_at_the_first_byte("bytes=0-invalid")); } #[tokio::test] diff --git a/src/services.rs b/src/services.rs index cc8a38d..0187ec8 100644 --- a/src/services.rs +++ b/src/services.rs @@ -503,6 +503,11 @@ pub struct ShareClear { #[derive(Debug, Clone, Copy, Default)] pub struct PlaylistClear { pub comment: bool, + /// Drop the existing track list before applying `add`, which turns an + /// update into a replacement. Subsonic's `createPlaylist` needs it: given a + /// `playlistId`, its `songId` values are the whole playlist rather than + /// additions to it. The native surface does not expose it. + pub tracks: bool, } #[derive(Debug, Clone)] @@ -1940,18 +1945,22 @@ impl DomainServices { let mut removes = remove_indexes.to_vec(); removes.sort_unstable_by(|a, b| b.cmp(a)); removes.dedup(); - let intent = MutationIntent::new( - "update", - &format!("playlist:{id}"), - &serde_json::json!({ - "name": name.map(str::trim), - "comment": comment, - "public": public, - "add": add, - "remove_indexes": &removes, - "clear_comment": clear.comment, - }), - ); + let mut intent_payload = serde_json::json!({ + "name": name.map(str::trim), + "comment": comment, + "public": public, + "add": add, + "remove_indexes": &removes, + "clear_comment": clear.comment, + }); + // Added to the payload only when set. The intent is hashed and compared + // on replay, so naming a new field unconditionally would change the + // hash of every update this server version ever saw before, and turn a + // client's retry across an upgrade into a conflict. + if clear.tracks { + intent_payload["clear_tracks"] = serde_json::Value::Bool(true); + } + let intent = MutationIntent::new("update", &format!("playlist:{id}"), &intent_payload); let _writer = self.db.writer_guard().await; let mut tx = self.db.pool().begin().await?; if let OperationClaim::Replayed(receipt) = self @@ -1969,7 +1978,11 @@ impl DomainServices { validate_name(name)?; } self.songs_by_ids_on(&mut tx, user_id, add).await?; - let mut ids = self.playlist_track_ids_on(&mut tx, user_id, id).await?; + let mut ids = if clear.tracks { + Vec::new() + } else { + self.playlist_track_ids_on(&mut tx, user_id, id).await? + }; for index in removes { if index >= ids.len() { return Err(ServiceError::Invalid); diff --git a/src/subsonic.rs b/src/subsonic.rs index 2d6a707..7444025 100644 --- a/src/subsonic.rs +++ b/src/subsonic.rs @@ -1143,8 +1143,13 @@ async fn create_playlist( let playlist = if let Some(id) = params.uuid_optional("playlistId")? { state .services + // Given a playlistId, songId names every song of the playlist, so + // the call replaces the track list rather than adding to it. A + // client that removes a song sends back what remains, and would + // otherwise see nothing change. + // // The Subsonic contract is frozen: it has no way to ask for a - // field to be blanked, so clearing stays off on this surface. + // text field to be blanked, so clearing the comment stays off. .update_playlist( principal.id, id, @@ -1153,7 +1158,10 @@ async fn create_playlist( None, &ids, &[], - Default::default(), + crate::services::PlaylistClear { + comment: false, + tracks: true, + }, ) .await .map_err(service_protocol)? diff --git a/tests/v2_foundations.rs b/tests/v2_foundations.rs index a4714d1..b3f48f8 100644 --- a/tests/v2_foundations.rs +++ b/tests/v2_foundations.rs @@ -1514,6 +1514,80 @@ async fn media_streaming_ranges_transcodes_caches_and_isolates_tenants() { "duplicate consumers must create only one file per cache key" ); + // A browser audio element opens every resource with `Range: bytes=0-`, so + // a cold transcode must answer it rather than refuse the range. Refusing + // made a web client fail on the first play of a track and succeed on the + // second, once the cache existed. + let cold_open = router + .clone() + .oneshot( + Request::get(format!("{uri}?format=mp3&bitrate=128")) + .header("authorization", format!("Bearer {owner_token}")) + .header("range", "bytes=0-") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(cold_open.status(), StatusCode::OK); + assert_eq!(cold_open.headers()["content-type"], "audio/mpeg"); + assert!( + cold_open + .into_body() + .collect() + .await + .unwrap() + .to_bytes() + .len() + > 100 + ); + + // iOS probes a resource with a two-byte range before it plays anything, so + // a bounded range from zero has to open the stream too. Juliet failed on + // the first play of every track until it did. + let cold_probe = router + .clone() + .oneshot( + Request::get(format!("{uri}?format=mp3&bitrate=136")) + .header("authorization", format!("Bearer {owner_token}")) + .header("range", "bytes=0-1") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(cold_probe.status(), StatusCode::OK); + assert_eq!(cold_probe.headers()["content-type"], "audio/mpeg"); + // Drained and awaited: an unread transcode holds its per-user permit, and + // the checks below would meet 429 instead of what they are testing. + cold_probe.into_body().collect().await.unwrap(); + for _ in 0..100 { + if media.active_transcodes() == 0 { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(10)).await; + } + assert_eq!( + media.active_transcodes(), + 0, + "a drained transcode must release its permit" + ); + + // A range that actually seeks still has no meaning before the transcode + // exists, and keeps its refusal. + let cold_seek = router + .clone() + .oneshot( + Request::get(format!("{uri}?format=mp3&bitrate=144")) + .header("authorization", format!("Bearer {owner_token}")) + .header("range", "bytes=64-") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(cold_seek.status(), StatusCode::RANGE_NOT_SATISFIABLE); + let live_seek = router .clone() .oneshot( @@ -2296,6 +2370,32 @@ async fn subsonic_xml_json_auth_catalog_and_user_data_are_compatible() { created["subsonic-response"]["playlist"]["owner"], "sub-admin" ); + // Given a playlistId, songId names every song of the playlist. A client + // that removes a song sends back what remains, so treating those ids as + // additions left the removed song in place and the edit looked lost. + let replaced = subsonic_json( + &router, + "createPlaylist", + api_key, + &format!("&playlistId={playlist}&songId={no_artist_song}"), + ) + .await; + assert_eq!(replaced["subsonic-response"]["playlist"]["songCount"], 1); + let reread = subsonic_json(&router, "getPlaylist", api_key, &format!("&id={playlist}")).await; + let entries = reread["subsonic-response"]["playlist"]["entry"] + .as_array() + .unwrap(); + assert_eq!(entries.len(), 1); + assert_eq!(entries[0]["id"], no_artist_song.to_string()); + // Put the original song back, so the checks below see the playlist they + // were written against. + subsonic_json( + &router, + "createPlaylist", + api_key, + &format!("&playlistId={playlist}&songId={song}"), + ) + .await; for (method, extra) in [ ("getPlaylists", String::new()), ("getPlaylist", format!("&id={playlist}")), @@ -4713,6 +4813,312 @@ async fn bookmarks_round_trip_sync_and_isolate_tenants() { subsonic_json(&router, "deleteBookmark", api_key, &format!("&id={track}")).await; } +/// An album artist tag holding two credits is two artists, not one artist whose +/// name contains a semicolon. Feeding the joined string to the artist table +/// minted an entity named after it, gave that entity the album, and left both +/// real artists with nothing: DSub browsed to either of them and found no +/// album. The association is re-derived at the end of every scan, because a +/// library indexed by the earlier version has unchanged files and would +/// otherwise never be repaired. +#[tokio::test] +async fn an_album_hangs_off_its_first_credited_artist_not_the_joined_string() { + let (_temp, config, state) = test_app().await; + let owner = state + .db + .create_account( + "credit-owner", + &security::hash_password("correct horse battery staple").unwrap(), + AccountRole::Admin, + now_ms(), + ) + .await + .unwrap(); + let music = config.data_dir.join("credit-music"); + std::fs::create_dir_all(&music).unwrap(); + let library = state + .db + .create_library( + owner, + "Credits", + &std::fs::canonicalize(&music).unwrap(), + LibraryVisibility::Private, + now_ms(), + ) + .await + .unwrap(); + let scan = state + .db + .create_scan_job(library, Some(owner), "manual") + .await + .unwrap(); + state.db.start_scan_job(scan, 1).await.unwrap(); + let mut input = browse_input( + 800, + "Paired", + "Convergence", + "Nova Kern; Lior Sand", + Some(1), + None, + ); + input.relative_path = "credit-0.flac".into(); + input.quick_hash = format!("{:064x}", 81_000); + input.full_hash = format!("{:064x}", 82_000); + state + .db + .apply_catalog_track(library, scan, &input, None, false) + .await + .unwrap(); + state.db.finish_scan_job(scan, 0).await.unwrap(); + + let names = |state: waveflow_server::AppState| async move { + state + .services + .list_artists(owner, None, Default::default()) + .await + .unwrap() + .into_iter() + .map(|summary| (summary.artist.name, summary.artist.id)) + .collect::>() + }; + // Checked before the post-scan pass runs, so indexing itself has to be + // right rather than leaning on the repair below. + let artists = names(state.clone()).await; + assert_eq!( + artists.keys().cloned().collect::>(), + vec!["Lior Sand".to_owned(), "Nova Kern".to_owned()], + "the joined credit must not become an artist of its own" + ); + state.db.consolidate_musicbrainz_ids(library).await.unwrap(); + let albums = state.services.catalog_snapshot(owner, &[]).await.unwrap(); + assert_eq!(albums.albums[0].artist_id, Some(artists["Nova Kern"])); + // The album still displays the whole credit; only the entity it hangs off + // is the first artist. + assert_eq!( + albums.albums[0].artist.as_deref(), + Some("Nova Kern; Lior Sand") + ); + + // Now the repair path. Put the catalogue back into the shape the earlier + // version produced - a third artist named after the joined credit, holding + // the album - and let a scan pass fix it. + let stale = uuid::Uuid::new_v4(); + sqlx::query( + "INSERT INTO artist (id, library_id, name, canonical_name, created_at, updated_at) \ + VALUES (?, ?, 'Nova Kern; Lior Sand', ?, ?, ?)", + ) + .bind(stale.to_string()) + .bind(library.to_string()) + // What the production canonicaliser makes of the joined credit: every + // non-alphanumeric character becomes a space, so no semicolon survives. + .bind(waveflow_core::scanner::canonical_name( + "Nova Kern; Lior Sand", + )) + .bind(now_ms()) + .bind(now_ms()) + .execute(state.db.pool()) + .await + .unwrap(); + // The identity key embeds the album artist, so the stale state has the + // stale key too - that is what indexing would later fail to match. + sqlx::query( + "UPDATE album SET album_artist_id = ?, identity_key = canonical_title || ':' || ? \ + WHERE library_id = ?", + ) + .bind(stale.to_string()) + .bind(stale.to_string()) + .bind(library.to_string()) + .execute(state.db.pool()) + .await + .unwrap(); + assert_eq!(names(state.clone()).await.len(), 3); + + state.db.consolidate_musicbrainz_ids(library).await.unwrap(); + + let repaired = names(state.clone()).await; + assert_eq!( + repaired.keys().cloned().collect::>(), + vec!["Lior Sand".to_owned(), "Nova Kern".to_owned()], + "the stale entity holds nothing once the album moves, so it goes" + ); + let albums = state.services.catalog_snapshot(owner, &[]).await.unwrap(); + assert_eq!(albums.albums[0].artist_id, Some(repaired["Nova Kern"])); + // The sweep drops what holds nothing, not what merely holds no album: the + // second credited artist keeps its tracks and stays. + assert!(repaired.contains_key("Lior Sand")); + assert_eq!( + state + .services + .artist(owner, repaired["Lior Sand"]) + .await + .unwrap() + .albums + .len(), + 0 + ); + + // Reindexing a track of a stale album is the case that made recomputing the + // identity key necessary: indexing creates the album under the new key + // while the stale row still carries the old one, and the repair has to + // merge them rather than fail the scan on the uniqueness constraint. + let rescan = state + .db + .create_scan_job(library, Some(owner), "manual") + .await + .unwrap(); + state.db.start_scan_job(rescan, 1).await.unwrap(); + // The sweep above removed the stale entity, so put it back to rebuild the + // starting state. + sqlx::query( + "INSERT INTO artist (id, library_id, name, canonical_name, created_at, updated_at) \ + VALUES (?, ?, 'Nova Kern; Lior Sand', ?, ?, ?)", + ) + .bind(stale.to_string()) + .bind(library.to_string()) + .bind(waveflow_core::scanner::canonical_name( + "Nova Kern; Lior Sand", + )) + .bind(now_ms()) + .bind(now_ms()) + .execute(state.db.pool()) + .await + .unwrap(); + sqlx::query( + "UPDATE album SET album_artist_id = ?, identity_key = canonical_title || ':' || ? \ + WHERE library_id = ?", + ) + .bind(stale.to_string()) + .bind(stale.to_string()) + .bind(library.to_string()) + .execute(state.db.pool()) + .await + .unwrap(); + input.file_size += 1; + let existing = state + .services + .catalog_snapshot(owner, &[]) + .await + .unwrap() + .songs[0] + .id; + state + .db + .apply_catalog_track(library, rescan, &input, Some(existing), false) + .await + .unwrap(); + state.db.finish_scan_job(rescan, 0).await.unwrap(); + assert_eq!( + state + .services + .catalog_snapshot(owner, &[]) + .await + .unwrap() + .albums + .len(), + 2, + "indexing under the new key leaves the stale album behind" + ); + + state.db.consolidate_musicbrainz_ids(library).await.unwrap(); + + let merged = state.services.catalog_snapshot(owner, &[]).await.unwrap(); + assert_eq!(merged.albums.len(), 1, "the two rows are one record"); + assert_eq!(merged.songs.len(), 1); + assert_eq!(merged.songs[0].album_id, Some(merged.albums[0].id)); + assert_eq!( + merged.albums[0].artist_id, + Some(names(state.clone()).await["Nova Kern"]) + ); + + // Two records can share a title and a lead credit and still be different + // albums. Hanging both off the first artist would give them one identity + // key and silently merge them, so the key carries the rest of the credit. + let split = state + .db + .create_scan_job(library, Some(owner), "manual") + .await + .unwrap(); + state.db.start_scan_job(split, 2).await.unwrap(); + for (index, credit) in [("Nova Kern; Ivy Trench"), ("Nova Kern; Rue Delacour")] + .into_iter() + .enumerate() + { + let mut split_input = + browse_input(810 + index, "Facing", "Split Bill", credit, Some(1), None); + split_input.relative_path = format!("split-{index}.flac"); + split_input.quick_hash = format!("{:064x}", 83_000 + index); + split_input.full_hash = format!("{:064x}", 84_000 + index); + state + .db + .apply_catalog_track(library, split, &split_input, None, false) + .await + .unwrap(); + } + state.db.finish_scan_job(split, 0).await.unwrap(); + state.db.consolidate_musicbrainz_ids(library).await.unwrap(); + + let all = state.services.catalog_snapshot(owner, &[]).await.unwrap(); + let split_bills = all + .albums + .iter() + .filter(|album| album.title == "Split Bill") + .count(); + assert_eq!( + split_bills, 2, + "same title and same lead credit, different second artist: two albums" + ); + + // Where the credits divide matters as much as what they say: canonicalising + // the joined string would flatten both of these to the same words and merge + // two records that credit different people. + let boundary = state + .db + .create_scan_job(library, Some(owner), "manual") + .await + .unwrap(); + state.db.start_scan_job(boundary, 2).await.unwrap(); + for (index, credit) in ["Vale; Ivy Trench", "Vale; Ivy; Trench"] + .into_iter() + .enumerate() + { + let mut edge = browse_input(820 + index, "Edge", "Boundary", credit, Some(1), None); + // The two tracks share a track artist, so only the album credit can be + // what tells the records apart. + edge.artist = Some("Vale".to_owned()); + edge.album_artist = Some(credit.to_owned()); + edge.relative_path = format!("boundary-{index}.flac"); + edge.quick_hash = format!("{:064x}", 85_000 + index); + edge.full_hash = format!("{:064x}", 86_000 + index); + state + .db + .apply_catalog_track(library, boundary, &edge, None, false) + .await + .unwrap(); + } + state.db.finish_scan_job(boundary, 0).await.unwrap(); + state.db.consolidate_musicbrainz_ids(library).await.unwrap(); + + let boundaries = state + .services + .catalog_snapshot(owner, &[]) + .await + .unwrap() + .albums + .into_iter() + .filter(|album| album.title == "Boundary") + .filter_map(|album| album.artist) + .collect::>(); + assert_eq!( + boundaries, + [ + "Vale; Ivy Trench".to_owned(), + "Vale; Ivy; Trench".to_owned() + ] + .into_iter() + .collect::>(), + "`A; B C` and `A; B; C` are different credits and different albums" + ); +} + /// A release identifier belongs to the release, not to whichever file was /// scanned last. Tracks of one album routinely disagree — a library assembled /// over years holds files tagged against different releases of the same record