From f58f202d32fd1040cf682602a1b3202117a04295 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Fri, 8 May 2026 20:40:40 -0700 Subject: [PATCH] =?UTF-8?q?Fix=20manual=20album=20import=20losing=20source?= =?UTF-8?q?=20=E2=80=94=20issue=20#524?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit radoslav-orlov reported every imported album landing in the soulsync standalone library as "Unknown Artist" + the raw 10-digit album id as the title + 0 tracks. Audit traced it to the click handler in the import page dropping the source-of-the-album_id on its way to the backend match endpoint. Root cause: `importPageSelectAlbum(albumId)` (the onclick on every suggestion / search-result card) only passed the album_id string. The full search response carried `source`, `name`, and `artist` per row — the backend's `get_artist_album_tracks` needs source so it can route the lookup to the metadata source the id actually came from. Without it, the source chain tries each source's `get_album(id)` against an id shaped for a different source — a Deezer numeric id against Spotify's id format returns 404, against iTunes's collectionId range returns 404, etc. — and falls through to the failure-fallback dict in `get_artist_album_tracks`: { 'success': False, 'album': {'name': album_name or album_id, 'total_tracks': 0, 'release_date': '', ...}, # no artist field at all 'tracks': [], } That broken album dict then flowed through `build_album_import_context` → post-processing pipeline → `record_soulsync_library_entry`, writing "Unknown Artist" + album_id-as-title + 0 tracks rows into the soulsync standalone library tables. Why hybrid users hit it most: a Spotify-primary user searching for an album → search returns the Spotify result PLUS Deezer fallbacks (via `_search_albums_for_source`'s priority chain). Clicking a Deezer fallback row then sent only the Deezer id to /album/match without flagging that source — Spotify-first chain failed against the Deezer id and the broken fallback got written. Fix: Frontend (`webui/static/stats-automations.js`): - New `importPageState._albumLookup: { albumId: { id, name, artist, source } }` populated by both card renderers (`_renderSuggestionCard` + the search-results render block) before they emit the onclick. - `importPageSelectAlbum` reads source / name / artist from that cache and includes them in the match POST body, so the backend routes to the correct provider's `get_album` on the very first try. - `_escAttr` applied to album_id in the onclick (defensive — ids shouldn't contain quotes but `_escAttr` was already being used on every other field interpolated into onclick attributes). Backend (`web_server.py:import_album_match`): - Defensive log warning when source is missing from the request body. Catches any future regression where another caller (curl / third-party / new UI flow) drops source again — it'll show up as a visible warning in app.log instead of silently corrupting the library. Verification: - Full pytest suite: 2264 passed, 1 skipped, 0 failed - Ruff clean - JS syntax clean - Manual repro requires a real user flow (search albums on the import page → click one → import) which isn't covered by the existing unit tests; reviewer should verify against issue #524's steps before merge. --- web_server.py | 17 +++++++++++ webui/static/helper.js | 1 + webui/static/stats-automations.js | 47 ++++++++++++++++++++++++++----- 3 files changed, 58 insertions(+), 7 deletions(-) diff --git a/web_server.py b/web_server.py index 6d6a00be..bb692bcc 100644 --- a/web_server.py +++ b/web_server.py @@ -33968,6 +33968,23 @@ def import_album_match(): if not album_id: return jsonify({'success': False, 'error': 'Missing album_id'}), 400 + # Without `source`, the lookup chain has to guess which metadata + # source the album_id came from — and a Deezer numeric id will + # match nothing in Spotify/iTunes/Discogs/etc., resulting in the + # failure-fallback dict that github issue #524 surfaced as + # "Unknown Artist / album_id-as-title / 0 tracks / 1991". Frontend + # fix in the same PR populates source on every match POST; this + # log catches anything that still reaches us without it (curl, + # third-party, regression in another caller). + if not source: + logger.warning( + "[Import Match] Missing 'source' on album_id=%s — lookup will " + "guess via primary-source priority chain. If this fires " + "consistently, a frontend caller is dropping source from " + "the match POST body.", + album_id, + ) + payload = build_album_import_match_payload( album_id, album_name=album_name, diff --git a/webui/static/helper.js b/webui/static/helper.js index 1c78c87e..37e74f55 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3418,6 +3418,7 @@ const WHATS_NEW = { { date: 'Unreleased — 2.4.4 dev cycle' }, { title: 'Manual Search In The Failed-Track Candidates Modal', desc: 'when a download fails or returns "not found" the user can already click the status cell to open a modal showing whatever search candidates the auto-search left over and pick a different one. that modal now ALSO has a manual search bar. type any query, hit search, get a fresh round of results from the download sources without having to start the whole download flow over from the search page. solves the case where the auto-query was bad (featured artist not in title, parentheticals like "(remastered 2019)" tripping the matcher, slight artist-name variants) but the file genuinely exists on the source. source picker is smart per download mode: single-source mode (soulseek-only / youtube-only / etc) shows a "searching X" label, no dropdown; hybrid mode shows a dropdown with "all sources" default plus every configured source — picking "all" runs parallel searches across all of them and tags each result row with its source badge. only configured sources show up; unconfigured ones are hidden. results stream in as each source completes via NDJSON instead of blocking on the slowest source — the table starts populating the moment the first source returns. clicking a result reuses the existing retry-download flow → same path, same acoustid verification on the file when it lands, no shortcut around the safety net. additive in the truest sense: the existing modal layout / candidates table / download buttons are byte-identical when the user doesn\'t use manual search. backend extends the candidates endpoint with `download_mode` + `available_sources` + a `source` field per candidate (purely additive — old fields untouched), and adds a new `POST /api/downloads/task//manual-search` that streams NDJSON (one header line, one source_results line per source as completed, one done terminator) so the frontend renderer can append rows incrementally. 11 tests pin the streaming contract: query length / source whitelist / task 404 validation, single-source dispatch, parallel "all" dispatch, one-event-per-source streaming shape, unconfigured-source skip + reject, header metadata, and per-source exception isolation (one source raising emits a `source_error` event but doesn\'t fail the stream).', page: 'downloads' }, { title: 'Manual Picks Don\'t Auto-Retry Anymore (And The Modal Always Opens)', desc: 'three follow-on fixes to the manual-search feature once people started actually using it. (1) when the user picked a candidate and that download failed (e.g. soundcloud 404 on a stale track url), the auto-retry monitor would treat it like any other failed auto-attempt — yank the task back to "searching" and pick a different candidate. felt completely wrong from the user\'s perspective: "i picked THIS one, why is it searching for something else?" now manual picks are tagged with a `_user_manual_pick` flag and the auto-retry path bails on it. failure surfaces to the user instead of getting silently fallen-back. (2) non-soulseek manual picks (youtube / tidal / qobuz / hifi / deezer / soundcloud / lidarr) were getting stuck at "downloading 0%" forever even after their engine reported terminal failure. cause: status polling went into a "let monitor handle retry" branch that never fired because manual picks bail on retry — task was orphaned in downloading state. fix: when the engine reports Errored on a manual pick, mark the task failed directly, don\'t defer to the monitor. plus an engine-state fallback path covers the rare race where the orchestrator\'s pre-populated transfer lookup is missing the entry. (3) failed / not_found rows were only clickable when the auto-search had cached candidates — but the whole point of opening the modal now is to RUN a manual search, which doesn\'t need pre-existing candidates. now every failed / not_found / cancelled row opens the modal regardless. (4) one nasty deadlock fix in the process: the new "mark failed" path was synchronously calling `on_download_completed` while holding `tasks_lock`, which itself re-acquires the same lock — `threading.Lock` is non-reentrant so the polling thread wedged forever. while wedged the lock was held → every other endpoint that needed it (including /candidates → can\'t open OTHER modals) hung waiting. moved completion callbacks onto a daemon thread so the lock releases first. (5) manual download worker now runs on its own dedicated thread instead of competing with the batch\'s 3-worker `missing_download_executor` pool — saturated batches no longer queue manual picks indefinitely. all changes are scoped to manual picks only via the `_user_manual_pick` flag — auto-attempt flow is byte-identical to before. 17 unit tests pin the gate behavior (status engine fallback / monitor retry skip / IF-branch failure transition / auto-attempt skip).', page: 'downloads' }, + { title: 'Manual Import: Stop Writing "Unknown Artist / album_id / 0 tracks" Garbage', desc: 'github issue #524 (radoslav-orlov): clicking an album in the import page → all imported albums landed in the library as "Unknown Artist" with the raw 10-digit album id as the title and 0 tracks. cause: the click handler `importPageSelectAlbum(albumId)` was passing only the id to the `/api/import/album/match` POST. the search response carried `source` (which metadata source the album_id came from) + `album_name` + `album_artist`, but the click discarded everything except the id. backend `get_artist_album_tracks` then guessed the source via the configured primary-source priority chain — for a non-deezer-primary user clicking a deezer search result, the chain tries spotify/itunes/discogs first against a deezer numeric id, all return None, and the lookup falls through to the failure-fallback dict (`name = album_id`, no artist field, `total_tracks = 0`). that broken metadata then flowed through the import pipeline → soulsync standalone library got the garbage rows. fix: cache album lookup by id when the suggestions / search renderers run, then have `importPageSelectAlbum` pull `source` + `name` + `artist` from the cache and include them in the match POST. backend now also logs a clear warning when source is missing from the match request, so any future caller dropping it shows up in app.log instead of silently corrupting library imports.', page: 'import' }, ], '2.4.3': [ // --- May 8, 2026 — patch release --- diff --git a/webui/static/stats-automations.js b/webui/static/stats-automations.js index c29eb4b1..4976f471 100644 --- a/webui/static/stats-automations.js +++ b/webui/static/stats-automations.js @@ -12,6 +12,13 @@ const importPageState = { initialized: false, activeTab: 'album', tapSelectedChip: null, // for mobile tap-to-assign fallback + // Album lookup cache for click handlers. Populated by suggestions / + // search renderers; read by importPageSelectAlbum so the match POST + // can include `source` + `name` + `artist` (without these, backend + // can't look up cross-source IDs and falls back to a broken + // "Unknown Artist / album_id as title / 0 tracks" placeholder — + // github issue #524). + _albumLookup: {}, // { albumId: { id, name, artist, source } } }; // =============================== @@ -1218,7 +1225,13 @@ async function importPageLoadSuggestions() { } function _renderSuggestionCard(a) { - return `
+ // Cache the album lookup so importPageSelectAlbum can pull source + + // name + artist on click (the onclick can only carry the ID string + // — see github issue #524 root cause). + importPageState._albumLookup[a.id] = { + id: a.id, name: a.name || '', artist: a.artist || '', source: a.source || '', + }; + return `
${_escAttr(a.name)}
${_esc(a.name)}
${_esc(a.artist)}
@@ -1245,14 +1258,20 @@ async function importPageSearchAlbum() { grid.innerHTML = '
No albums found
'; return; } - grid.innerHTML = data.albums.map(a => ` -
+ grid.innerHTML = data.albums.map(a => { + // Cache album lookup so the click handler can include source + // + name + artist on the match POST (see #524). + importPageState._albumLookup[a.id] = { + id: a.id, name: a.name || '', artist: a.artist || '', source: a.source || '', + }; + return ` +
${_escAttr(a.name)}
${_esc(a.name)}
${_esc(a.artist)}
${a.total_tracks} tracks · ${a.release_date ? a.release_date.substring(0, 4) : ''}
-
- `).join(''); +
`; + }).join(''); document.getElementById('import-page-album-clear-btn').classList.remove('hidden'); } catch (err) { grid.innerHTML = `
Error: ${err.message}
`; @@ -1269,8 +1288,22 @@ async function importPageSelectAlbum(albumId) { matchList.innerHTML = '
Matching files to tracklist...
'; try { - // Include file_paths filter if matching from an auto-group - const matchBody = { album_id: albumId }; + // Include file_paths filter if matching from an auto-group. + // CRITICAL: include source + album_name + album_artist from the + // search/suggestion result. Without `source`, the backend can't + // route the lookup to the metadata source the album_id came + // from — for example a Deezer album_id needs Deezer's get_album + // call. Cross-source lookup fails silently, returns the + // failure-fallback dict with album_id-as-name + Unknown Artist + // + 0 tracks, then the import flow writes that broken metadata + // to the library DB (github issue #524). + const cached = importPageState._albumLookup[albumId] || {}; + const matchBody = { + album_id: albumId, + source: cached.source || '', + album_name: cached.name || '', + album_artist: cached.artist || '', + }; if (importPageState._autoGroupFilePaths) { matchBody.file_paths = importPageState._autoGroupFilePaths; importPageState._autoGroupFilePaths = null; // clear after use