diff --git a/docs/handoff-2026-08-23.md b/docs/handoff-2026-08-23.md index 28f6bee..cccdef9 100644 --- a/docs/handoff-2026-08-23.md +++ b/docs/handoff-2026-08-23.md @@ -8,14 +8,15 @@ one continues from it. ## Where the project stands -`main` at `d36e929`. Tag `v2.0.0-beta.0` on `e19a117`, unmoved. No open pull -requests. 42 unit and 48 integration tests pass; `cargo fmt --all --check` and +`main` at `1d15c16`. Tag `v2.0.0-beta.0` on `e19a117`, unmoved. No open pull +requests. 42 unit and 53 integration tests pass; `cargo fmt --all --check` and `cargo clippy --all-targets --all-features -- -D warnings` are clean, and a third CI job now checks the crate against its declared minimum Rust version. -> **Updated 2026-08-24.** The mission list below has moved: §1 and the first -> half of §2 shipped, and a trap was added that cost `main` six hours of red. -> Everything above this line is the record of 2026-08-23 and stays as written. +> **Updated 2026-08-24.** The mission list below is closed except §4, which the +> other three have now made due: §1, §2 and §3 all shipped, as PRs #137, #139 +> and #140. A trap was added that cost `main` six hours of red. Everything above +> this line is the record of 2026-08-23 and stays as written. The release is real and verified from the registry rather than from a green tick: `ghcr.io/instazdll/waveflow-server` carries `2.0.0-beta.0` and `beta` — @@ -48,7 +49,8 @@ lesson, not the score. ## The mission from here -Nothing is blocking. In the order I would take them: +Written on 2026-08-23 in the order I would take them. Three of the four are +closed; what is left is §4, and the other three are what made it due. ### 1. Break up `src/` — done, PR #137 @@ -90,22 +92,50 @@ person has to check. that a stale hint "costs a missed relocation, never a wrong one" was written into `CLAUDE.md` and the migration before it was true; both were corrected. -- **Artist favourite remapping**, if judged worth the thirty lines. The - documentation currently states plainly that it does not exist, which is the - honest position until it does. - -### 3. Album output fields - -`originalReleaseDate`, `releaseDate`, `releaseTypes[]`, `recordLabels[]`, -`discTitles[]` are still absent, and honestly declared so under the presence -rule. `ItemKey::Label` and `ItemKey::Publisher` exist in lofty, so -`recordLabels[]` is the cheap one. - -### 4. Before a stable tag - -Replay the clients again if the wire moves. The rule that has paid twice: **never -conclude from what the client shows when the server can be asked.** It -disqualified three "server defects" reported in good faith in one evening. +- **~~Artist favourite remapping.~~** Done, PR #140. It was worth the thirty + lines. `reconcile_catalog_identity` remaps `user_star` and `user_rating` + when `pid.artist` changed, **before** requesting the rescan — the only moment + the old identifier and the name that produced it are both still on the + `artist` row. Albums are not remapped and cannot be: their spec reads + `albumversion` and `releasedate`, which live on the files. + + The review caught that moving the rows one at a time depends on the order they + came back in — one artist's new identifier can be another's old one, and the + first artist's favourite was then carried onto the second's row, or lost. Every + pair is now computed first and moved in two phases through a namespace no + identifier can occupy. No spec this engine can parse actually produces that + chain, because only `albumartistid` carries a value for an artist; the test + builds the rows by hand and says so, because the code cannot see that argument + and a wider `PidSource` would make it reachable. + +### 3. Album output fields — done, PR #140 + +All five. `originalReleaseDate`, `releaseDate`, `releaseTypes[]` and +`recordLabels[]` describe the release, so they sit on the album and fill the way +`year` does; `discTitles[]` holds one title per disc, so the tag lands on the +track and the album derives the list from its available tracks, grouped per disc +with `MIN`. `LABEL` first, `PUBLISHER` second. The three arrays are emitted empty +rather than absent — under both names an album is rendered by, `album` and, in a +directory, `child` — and the dates are omitted when unknown, as the reference +omits them. They are stored as the file spelled them and taken apart only at the +wire: `1998-11` is a year and a month, and a head that is not exactly four ASCII +digits is no date at all. + +The same pull request closed the four findings deferred from #137: the N+1 reads +in `playlists_on` and `now_playing`, `search3` paging in memory rather than in +SQL, unbounded identifier lists on `star`/`unstar`/`scrobble`, and unbounded +playlists. + +### 4. Before a stable tag — now due + +**The wire moved.** Five album fields are new Subsonic output, so the condition +this section names is met and replaying the four clients is no longer optional. +[`subsonic-compatibility.md`](subsonic-compatibility.md) still dates them +2026-08-23, against a model that has changed since. + +The rule that has paid twice: **never conclude from what the client shows when +the server can be asked.** It disqualified three "server defects" reported in +good faith in one evening. ## Traps this session paid for diff --git a/docs/subsonic-compatibility.md b/docs/subsonic-compatibility.md index 1b553fc..e2646e5 100644 --- a/docs/subsonic-compatibility.md +++ b/docs/subsonic-compatibility.md @@ -2,6 +2,18 @@ Automated protocol coverage is enforced by `tests/v2_foundations.rs` for XML, JSON, `.view`, GET, form POST, `u/p`, `u/t/s`, `apiKey`, catalogue isolation, all documented mutations, media and artwork. +> **The rows below are dated 2026-08-23 and the wire has moved since.** On +> 2026-08-24 an album gained five output fields — `originalReleaseDate`, +> `releaseDate`, `releaseTypes[]`, `recordLabels[]` and `discTitles[]` — under +> both names an album is rendered by, `album` and, in a directory, `child`. No +> field was removed and none changed shape, so nothing a client read on the 23rd +> reads differently today; but a row that says `pass` says it about a response +> that no longer carries exactly the same keys. **The campaign wants replaying +> before a stable tag**, which is what §4 of +> [`handoff-2026-08-23.md`](handoff-2026-08-23.md) asks for. Until it is, read +> every date in this table as naming the model the run exercised rather than the +> one `main` serves. + | Client | Version | Login | Browse/search | Native/transcode | Playlists/user data | Status | |---|---:|---:|---:|---:|---:|---| | Symfonium | 15.0.1 | pass | pass | limit | limit | Re-run 2026-08-23 against the aligned model, on an Android 17 emulator through the logging proxy. **This is a different major version from the 14.1.0 row it replaces**, so its differences from the previous run are not attributable to the server alone. Passed: a failed login decoded from its HTTP 200, 5 albums and 18 tracks, sleeve order, one canonical `Jazz` answering 4 tracks, both artists of a two-artist album holding it, the separator traps (`AC/DC` one artist, `Bach/Gounod` two composers, a padded slash two artists), and a title query returning no artists. Two limits, both established from the proxy log rather than from the client's display, and neither a server defect. **Seeking a lossless track fails**: the client re-requests the resource with no `Range` header at all and restarts from zero — in the whole session it sent exactly one byte-offset range, on an MP3, answered 206. It sent none in the 2026-08-19 run either, so no seek by this client was ever established. **Playlist creation never reaches the server**: no `createPlaylist` or `updatePlaylist` was sent. Both are contrasted directly: Feishin seeks the same seektable-less FLAC files and creates playlists against the same server. The client renders `artist`, `albumArtist` and `composer` and ignores the other ten roles, which the server emits in full at the `v=1.13.0` this client asks with. Bookmarks were exercised incidentally (5 `createBookmark`, one live). It never calls `getArtists`/`getIndexes`, syncing entirely through the match-all `search3`. | diff --git a/src/subsonic/protocol.rs b/src/subsonic/protocol.rs index 70cd24b..94f37f8 100644 --- a/src/subsonic/protocol.rs +++ b/src/subsonic/protocol.rs @@ -90,7 +90,13 @@ pub(super) fn node_json(node: &Node, parent: &str) -> Value { *name, "artists" | "genres" | "recordLabels" | "releaseTypes" | "discTitles" ), - EntryKind::Song => true, + // A song is not a release. `recordLabels`, `releaseTypes` and + // `discTitles` are in the list above because an album rendered + // inside a directory shares the `child` element name with a song, + // and this is where the two are told apart: injecting them here + // would have a recording answer `recordLabels: []`, which under the + // presence rule claims the server reads a label off a track. + EntryKind::Song => !matches!(*name, "recordLabels" | "releaseTypes" | "discTitles"), }; if !injected { continue; diff --git a/tests/v2_foundations.rs b/tests/v2_foundations.rs index 41bc09f..13420bd 100644 --- a/tests/v2_foundations.rs +++ b/tests/v2_foundations.rs @@ -3527,6 +3527,17 @@ async fn body_text(response: axum::response::Response) -> String { .unwrap() } +/// Whether a JSON object omits a key outright. +/// +/// `value[key].is_null()` cannot tell an absent key from one explicitly set to +/// `null`, and under the OpenSubsonic presence rule the two say different +/// things: absent means the server does not support the field at all, where a +/// null would be a value it chose to send. The server emits no explicit nulls +/// today, which is exactly why the assertion has to name which one it means. +fn omits(value: &serde_json::Value, key: &str) -> bool { + value.as_object().expect("a JSON object").get(key).is_none() +} + async fn subsonic_json( router: &axum::Router, method: &str, @@ -10653,6 +10664,15 @@ async fn an_album_reports_its_release_details_and_its_disc_titles() { .await .unwrap(); } + // What a real scan does after applying its rows, before closing the job: + // it builds the artist index and the role statistics a folder listing + // reads. Driving the pipeline in another order would prove something about + // an order nothing runs. + state + .db + .consolidate_catalog_derivations(library_id) + .await + .unwrap(); state.db.finish_scan_job(scan_id, 0).await.unwrap(); let album_id = state @@ -10722,6 +10742,15 @@ async fn an_album_reports_its_release_details_and_its_disc_titles() { .apply_catalog_track(library_id, bare_scan, &bare, None, false) .await .unwrap(); + // What a real scan does after applying its rows, before closing the job: + // it builds the artist index and the role statistics a folder listing + // reads. Driving the pipeline in another order would prove something about + // an order nothing runs. + state + .db + .consolidate_catalog_derivations(library_id) + .await + .unwrap(); state.db.finish_scan_job(bare_scan, 0).await.unwrap(); let bare_id = state .services @@ -10733,13 +10762,132 @@ async fn an_album_reports_its_release_details_and_its_disc_titles() { .find(|album| album.title == "Bare release") .unwrap() .id; + // A song is not a release. The three arrays share their element name with + // an album inside a directory, and a recording answering `recordLabels: []` + // would claim the server reads a label off a track. + let song = &album["song"][0]; + assert!(omits(song, "recordLabels")); + assert!(omits(song, "releaseTypes")); + assert!(omits(song, "discTitles")); + // Its own arrays are still there, empty rather than absent. + assert_eq!(song["moods"], serde_json::json!([])); + let bare = subsonic_json(&router, "getAlbum", api_key, &format!("&id={bare_id}")).await; let bare = &bare["subsonic-response"]["album"]; assert_eq!(bare["recordLabels"], serde_json::json!([])); assert_eq!(bare["releaseTypes"], serde_json::json!([])); assert_eq!(bare["discTitles"], serde_json::json!([])); - assert!(bare["originalReleaseDate"].is_null()); - assert!(bare["releaseDate"].is_null()); + assert!(omits(bare, "originalReleaseDate")); + assert!(omits(bare, "releaseDate")); + + // A third album carrying exactly one of each. This is the shape the array + // rule is for: with one child and no rule, a record label renders as a bare + // object instead of a list of one. + let solo_scan = state + .db + .create_scan_job(library_id, Some(owner), "manual") + .await + .unwrap(); + state.db.start_scan_job(solo_scan, 1, false).await.unwrap(); + let mut solo_input = catalog_input(9, "Solo Hand"); + solo_input.album = Some("Single imprint".into()); + solo_input.is_compilation = false; + solo_input.disc_number = Some(1); + solo_input.disc_subtitle = Some("Only disc".into()); + solo_input.release_types = Some("EP".into()); + solo_input.record_labels = Some("Solo Records".into()); + state + .db + .apply_catalog_track(library_id, solo_scan, &solo_input, None, false) + .await + .unwrap(); + // What a real scan does after applying its rows, before closing the job: + // it builds the artist index and the role statistics a folder listing + // reads. Driving the pipeline in another order would prove something about + // an order nothing runs. + state + .db + .consolidate_catalog_derivations(library_id) + .await + .unwrap(); + state.db.finish_scan_job(solo_scan, 0).await.unwrap(); + let solo_id = state + .services + .catalog_snapshot(owner, &[]) + .await + .unwrap() + .albums + .into_iter() + .find(|album| album.title == "Single imprint") + .unwrap() + .id; + let solo = subsonic_json(&router, "getAlbum", api_key, &format!("&id={solo_id}")).await; + let solo = &solo["subsonic-response"]["album"]; + assert_eq!( + solo["recordLabels"], + serde_json::json!([{"name": "Solo Records"}]), + "one record label is a list of one, not a bare object" + ); + assert_eq!(solo["releaseTypes"], serde_json::json!(["EP"])); + assert_eq!( + solo["discTitles"], + serde_json::json!([{"disc": 1, "title": "Only disc"}]) + ); + + // `getMusicDirectory` renames an album to `child`, and both the array rule + // and the injection guard are keyed on that name. Under it an album has to + // answer exactly what it answers as `album` — for several values, for one, + // and for none — or a client browsing folders reads a single record label + // as a bare object and an empty list as "not supported". + for (id, rendered) in [(album_id, album), (solo_id, solo), (bare_id, bare)] { + let artist_id: String = + sqlx::query_scalar("SELECT album_artist_id FROM album WHERE id = ?") + .bind(id.to_string()) + .fetch_one(state.db.pool()) + .await + .unwrap(); + let directory = subsonic_json( + &router, + "getMusicDirectory", + api_key, + &format!("&id={artist_id}"), + ) + .await; + let children = directory["subsonic-response"]["directory"]["child"] + .as_array() + .unwrap() + .clone(); + let child = children + .iter() + .find(|child| child["id"] == id.to_string()) + .unwrap_or_else(|| panic!("the folder of {artist_id} has to list {id}")); + for key in ["recordLabels", "releaseTypes", "discTitles"] { + assert_eq!( + child[key], rendered[key], + "{key} must read the same under `child` as under `album`" + ); + } + // And a song wearing that same element name carries none of the three. + // Browsed from the album's own folder rather than the artist's: an + // artist folder lists albums, so filtering it for songs would assert + // nothing at all. + let songs = + subsonic_json(&router, "getMusicDirectory", api_key, &format!("&id={id}")).await; + let songs = songs["subsonic-response"]["directory"]["child"] + .as_array() + .unwrap(); + assert!(!songs.is_empty(), "every album here has at least one track"); + for song in songs { + assert_eq!(song["isDir"], false); + for key in ["recordLabels", "releaseTypes", "discTitles"] { + assert!( + omits(song, key), + "a song must not answer {key}: {}", + song["title"] + ); + } + } + } } #[tokio::test]