Fix manual album import losing source — issue #524
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.
This commit is contained in:
Broque Thomas
parent
2da1e8b2d9
commit
f58f202d32
3 changed files with 58 additions and 7 deletions
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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/<id>/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 ---
|
||||
|
|
|
|||
|
|
@ -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 `<div class="import-page-album-card" onclick="importPageSelectAlbum('${a.id}')">
|
||||
// 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 `<div class="import-page-album-card" onclick="importPageSelectAlbum('${_escAttr(a.id)}')">
|
||||
<img src="${a.image_url || '/static/placeholder.png'}" alt="${_escAttr(a.name)}" loading="lazy" onerror="this.src='/static/placeholder.png'">
|
||||
<div class="import-page-album-card-title" title="${_escAttr(a.name)}">${_esc(a.name)}</div>
|
||||
<div class="import-page-album-card-artist" title="${_escAttr(a.artist)}">${_esc(a.artist)}</div>
|
||||
|
|
@ -1245,14 +1258,20 @@ async function importPageSearchAlbum() {
|
|||
grid.innerHTML = '<div style="color:#888;text-align:center;padding:20px;">No albums found</div>';
|
||||
return;
|
||||
}
|
||||
grid.innerHTML = data.albums.map(a => `
|
||||
<div class="import-page-album-card" onclick="importPageSelectAlbum('${a.id}')">
|
||||
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 `
|
||||
<div class="import-page-album-card" onclick="importPageSelectAlbum('${_escAttr(a.id)}')">
|
||||
<img src="${a.image_url || '/static/placeholder.png'}" alt="${_escAttr(a.name)}" loading="lazy" onerror="this.src='/static/placeholder.png'">
|
||||
<div class="import-page-album-card-title" title="${_escAttr(a.name)}">${_esc(a.name)}</div>
|
||||
<div class="import-page-album-card-artist" title="${_escAttr(a.artist)}">${_esc(a.artist)}</div>
|
||||
<div class="import-page-album-card-meta">${a.total_tracks} tracks · ${a.release_date ? a.release_date.substring(0, 4) : ''}</div>
|
||||
</div>
|
||||
`).join('');
|
||||
</div>`;
|
||||
}).join('');
|
||||
document.getElementById('import-page-album-clear-btn').classList.remove('hidden');
|
||||
} catch (err) {
|
||||
grid.innerHTML = `<div style="color:#ef4444;text-align:center;padding:20px;">Error: ${err.message}</div>`;
|
||||
|
|
@ -1269,8 +1288,22 @@ async function importPageSelectAlbum(albumId) {
|
|||
matchList.innerHTML = '<div style="color:#888;text-align:center;padding:20px;">Matching files to tracklist...</div>';
|
||||
|
||||
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
|
||||
|
|
|
|||
Loading…
Reference in a new issue