731 Commits (f5ccc55bfc2b6fd5bf5bbb38ba236770146ff09d)

Author	SHA1	Message	Date
Broque Thomas	3c1f614b6e	fix: cast duration_ms to int before :02d format in discovery workers ... yt_dlp sometimes returns float `duration_ms` for YouTube tracks. The discovery workers format the duration with `f"{x // 60000}:{(x % 60000) // 1000:02d}"` — and `:02d` requires an int. When the duration is a float, the format string raises: Unknown format code 'd' for object of type 'float' Caught when running YouTube discovery on a real playlist (bbno$ tracks) — every track failed with status='Error'. Pre-existing bug, surfaced now because of yt_dlp returning float durations on this playlist. Fixed at all 8 sites by casting through `int()` before the `// 60000` and `% 60000` operations: - core/discovery/youtube.py: 2 sites in run_youtube_discovery_worker (cache hit + main result construction). - web_server.py L29238/L29372: 2 sites in _run_listenbrainz_discovery_worker. - web_server.py L40112/L40136/L40161/L40178: 4 sites in the YouTube retry/pre-discovered results assembly path. The `if duration_ms` / `if dur` guard already protects against None and 0, so `int(...)` is only called on truthy numeric values. Tests: 1 new regression test under tests/discovery/test_discovery_youtube.py (`test_float_duration_does_not_crash_format`) — passes a float duration_ms and asserts the worker completes without an error result. Ruff clean.	1 month ago
Broque Thomas	27fa96fe97	PR5b: lift _run_youtube_discovery_worker to core/discovery/youtube.py ... Second lift in the PR5 discovery-workers series. Pulls the 332-line YouTube discovery worker out of `web_server.py` into its own focused module under `core/discovery/`. Pure 1:1 lift — wrappers keep the original entry-point name so the two callers (`youtube_discovery_executor.submit(_run_youtube_discovery_worker, ...)`) continue to work without changes. What the YouTube discovery worker does: 1. Pause enrichment workers (release shared resources). 2. For each YouTube playlist track: - Cancellation check (phase != 'discovering' aborts). - Discovery cache lookup; cache hit short-circuits the search. - Strategy 1: matching_engine search queries with confidence scoring against Spotify (preferred) or iTunes (fallback). - Strategy 2: swapped artist/title query. - Strategy 3: raw (untokenized) query. - Strategy 4: extended search with limit=50. - On match → save to discovery cache. - On miss → build Wing It stub from raw source data. 3. After loop: phase='discovered', sort results by index, and for mirrored playlists write extra_data back to the DB. 4. Activity feed entry with match summary. 5. On error → state['status']='error', phase='fresh'. 6. Finally: resume enrichment workers. Dependencies injected via `YoutubeDiscoveryDeps` (16 fields) — youtube_playlist_states, spotify_client, matching_engine, plus 13 callable helpers (pause/resume enrichment, get_active_discovery_source, get_metadata_fallback_client, discovery cache key/validate, extract artist name, spotify_rate_limited, discovery_score_candidates, get_metadata_cache, build_discovery_wing_it_stub, get_database, add_activity_item). Diff vs original after `deps.X` → global X normalization is **zero differences** — 332 lines orig = 332 lines lifted, byte-identical body (including all whitespace). Pre-existing bug preserved (not fixed): if `youtube_playlist_states[url_hash]` raises KeyError on entry, the outer except handler tries to mutate `state` which is unbound → secondary UnboundLocalError. Same bug in the original. Documented here for future cleanup but out of scope for the lift. Tests: 14 new under tests/discovery/test_discovery_youtube.py covering cache hit short-circuit, Strategy 1 confidence match, Wing It fallback, iTunes fallback path (Spotify unauthenticated and rate-limited), cancellation (phase changed), skip_discovery flag, completion phase update, activity feed entry, mirrored playlist DB writeback, non-mirrored no-writeback, enrichment workers pause/resume, error-during-loop resume, results sorted by index after retry. Full suite: 1082 passing (was 1068). Ruff clean.	1 month ago
Broque Thomas	bdb7a3139d	PR5a: lift _run_sync_task to core/discovery/sync.py ... First lift in the new PR5 discovery-workers series. Pulls the 448-line playlist sync background worker out of `web_server.py` into its own focused module under `core/discovery/`. Pure 1:1 lift — wrappers keep the original entry-point name so the four callers (`sync_executor.submit(_run_sync_task, ...)`) continue to work without changes. What the sync worker does: 1. Convert frontend JSON tracks → SpotifyTrack/SpotifyPlaylist objects. 2. Normalize artist/album shapes for downstream wishlist parity. 3. Wire a progress_callback that updates `sync_states` + automation card. 4. Patch sync_service for database-only fallback when no media server is connected. 5. `run_async(sync_service.sync_playlist(...))` and capture the result. 6. Update sync_states to 'finished', push playlist poster image to Plex / Jellyfin / Emby, record sync history (with re-sync vs new-sync branching), emit `playlist_synced` event for automation engine, and persist sync status with a tracks_hash for smart-skip on the next scheduled sync. 7. On exception → mark error in sync_states + automation; finally clear progress callback + drop `_original_tracks_map` from sync_service. Dependencies injected via `SyncDeps` (11 fields) — config_manager, sync_service, plex_client, jellyfin_client, automation_engine, run_async, record_sync_history_start, update_automation_progress, update_and_save_sync_status, sync_states dict, sync_lock. The only structural drift from a pure paste is the top-of-function variable binding: original used `global sync_states, sync_service`, lifted version rebinds them as locals from deps (`sync_states = deps.sync_states` etc.) since the names aren't module-level in the new file. Same behaviour otherwise — diff against the original after `deps.X` → global X normalization is **zero differences**. Tests: 18 new under tests/discovery/test_discovery_sync.py covering sync history recording (new + resync), setup error path (with and without automation_id), missing sync_service handling, sync_playlist exception handling, successful sync state transition, unmatched-tracks summary, playlist image upload (plex + jellyfin + zero-synced gate), automation engine emit, automation progress finished call, sync history DB persistence (completion + match_details), tracks_hash persistence, and finally-block cleanup (callback clear + map drop). Full suite: 1068 passing (was 1050). Ruff clean. Kicks off the PR5 series — 9 discovery workers totaling ~2,400 lines across `_run_sync_task`, `_run_*_discovery_worker` family, `_run_quality_scanner`, and `_process_watchlist_scan_automatically`. Wishlist-related extractions deliberately skipped to avoid overlap with kettui's planned `core/wishlist/` package.	1 month ago
Broque Thomas	1fb2f34955	PR4h fixup: restore strict 1:1 in master.py ... Three drifts caught in line-by-line review against original — all reverted so the lifted body is now byte-identical to the web_server.py original (after deps.X → global X normalization): 1. `import traceback` was hoisted to module-level imports. Original imports it inline inside the except block. Moved back to inline (same behavior, but strict location parity). 2. Trailing whitespace on the blank line after analysis_results.append. 3. Trailing whitespace on the blank line inside the wishlist except handler. Diff against original now produces ZERO differences. Tests still 21/21 passing, ruff clean.	1 month ago
Broque Thomas	fa29ee2195	PR4h: lift _run_full_missing_tracks_process to core/downloads/master.py ... Final extraction in the download orchestrator series. Lifts the 586-line master worker that drives the entire missing-tracks pipeline from `web_server.py` into `core/downloads/master.py`. Pure 1:1 lift — wrappers keep the original entry-point name so the three callers (`missing_download_executor.submit(_run_full_missing_tracks_process, ...)`) continue to work without changes. What the master worker does: 1. PHASE 1 ANALYSIS — per-track DB ownership check with album fast path (lookup album by name+artist, match tracks within it) plus a MusicBrainz release-cache preflight so per-track post-processing all uses the same release MBID (prevents Navidrome album splits). 2. Wishlist removal for tracks already in the library. 3. Explicit-content filter. 4. PHASE 2 transition — if nothing missing, mark batch complete, update per-source playlist phases, kick auto-wishlist completion handler. 5. Soulseek album pre-flight — search for a complete album folder before falling back to track-by-track search, cache the source for reuse. 6. Wishlist album grouping — derive per-album disc counts and resolve ONE artist context per album so collab albums don't fold-split. 7. Task creation with explicit album/artist context injection + playlist-folder-mode flag propagation. 8. Hand off to download_monitor + start_next_batch_of_downloads. 9. Error handler — phase=error, reset YouTube playlist phase to 'discovered', reset auto-wishlist globals on auto-initiated batches. Dependencies injected via `MasterDeps` (21 fields) — wide surface covering config, MB caches/locks, soulseek client, source-page state dicts, multiple callbacks (wishlist removal, explicit filter, executor + auto-completion fn, monitor, start_next_batch). The only behaviour difference from a pure paste is `import traceback` hoisted to module scope (was inline in the except block) — same behaviour. Trailing whitespace on two blank lines also got normalized away by the editor; neither has any runtime effect. `reset_wishlist_auto_processing` callback wraps the `global wishlist_auto_processing, wishlist_auto_processing_timestamp` write + `wishlist_timer_lock` since `global` can't reach back into web_server.py from a separate module. Tests: 21 new under tests/downloads/test_downloads_master.py covering analysis-phase state, force_download_all, found-track wishlist removal, explicit filter, no-missing complete + per-source state updates, auto wishlist completion submit, album fast path (direct + fallthrough), MB preflight (caches both keys, no-mb-worker no-op), task creation (queue + tasks dict, explicit context for albums, wishlist album grouping consistency, playlist folder mode), monitor + next-batch handoff, multi-disc total_discs computation, error handler (phase set, youtube reset, auto wishlist reset), and batch-removed-mid-flight defensive path. Full suite: 1050 passing (was 1029). Ruff clean. End of the PR4 series — `web_server.py` lost ~590 lines on this commit alone; total trim across PR4a–PR4h is ~2900 lines of orchestrator code moved into focused `core/downloads/*.py` modules.	1 month ago
Broque Thomas	0a6d1759b7	PR4g: lift batch lifecycle to core/downloads/lifecycle.py ... Seventh sub-PR in the download orchestrator series. Strict 1:1 lift — zero behavior change. ~570 lines moved out of web_server.py. What moved (lifted as 3 tightly-coupled functions in one module): - _start_next_batch_of_downloads → start_next_batch_of_downloads - _on_download_completed → on_download_completed - _check_batch_completion_v2 → check_batch_completion_v2 Dependencies bundled in `LifecycleDeps` (15+ refs): - config_manager, automation_engine, download_monitor, repair_worker, mb_worker (live globals) - is_shutting_down (lambda over IS_SHUTTING_DOWN flag) - get_batch_lock (web_server helper for batch_locks dict) - submit_download_track_worker (lambda wrapping missing_download_executor.submit + _download_track_worker) - submit_failed_to_wishlist + submit_failed_to_wishlist_with_auto_completion (async, used by on_download_completed) AND process_failed_to_wishlist + process_failed_to_wishlist_with_auto_completion (sync, used by check_batch_completion_v2 — direct call matches original v2 behavior; the non-v2 path always submitted to executor) - ensure_spotify_track_format, get_track_artist_name, check_and_remove_from_wishlist, regenerate_batch_m3u (web_server helpers — large, will lift in follow-up PRs) - youtube_playlist_states, tidal_discovery_states, deezer_discovery_states, spotify_public_discovery_states (per-source playlist state dicts — phase transitions on batch completion) Direct imports for already-lifted helpers: - core.runtime_state.{download_tasks, download_batches, tasks_lock, add_activity_item} - core.downloads.history.record_sync_history_completion (PR4a) - core.album_consistency.run_album_consistency - core.metadata.common.get_file_lock Behavior parity verified line-by-line: - start_next_batch: same batch lock acquisition, same shutdown gate, same V2-cancelled-task skip, same searching-status-set-before-submit, same submit-fails-no-ghost-worker semantics - on_download_completed: same duplicate-call detection (skip decrement but still check completion), same failed-track tracking with spotify_track formatting + activity items + automation event emission, same wishlist removal on success, same active_count decrement, same stuck-detection (searching > 10min → not_found, post_processing > 5min → completed), same M3U regeneration + repair worker hand-off + album consistency pass + wishlist failed-tracks submission - check_batch_completion_v2: same finished-count tally, same stuck detection, same already-complete short-circuit returning True, same per-source playlist phase updates, same album consistency pass, same DIRECT (sync) wishlist processing call (NOT submit-to-executor — matches original v2 which called process_* functions directly) CRITICAL drift caught + fixed during review: - Initial lift had v2 routing wishlist calls through submit_* deps (async). Original v2 called process_* directly (sync). Added separate process_* deps to LifecycleDeps and routed v2 to them. Tests updated. Two minor defensive additions documented: - `is_auto_batch = False` initialized before conditional in v2 (Python scope rules made this unnecessary in original, but explicit is safer) - Variable rename inside the queue-completion-check loop in on_download_completed: `task_id` → `queue_task_id` to avoid shadowing the outer parameter. Log output preserves the same task ID. Tests: 28 new under tests/downloads/test_downloads_lifecycle.py covering start-next (early-returns, shutdown gate, max_concurrent, cancelled-task skip, searching-status-set, submit-failure-no-ghost, orphan task), on-complete (decrement, duplicate skip, failed/cancelled tracking, automation emit, wishlist removal, batch completion + emit + source phase update, stuck detection, auto vs manual routing), check-v2 (missing batch, not-complete, complete-marking, already- complete, auto routing, exception handling). Full suite: 1029 passing (was 1001). Ruff clean.	1 month ago
Broque Thomas	f0955420c3	PR4f: lift _download_track_worker to core/downloads/task_worker.py ... Sixth sub-PR in the download orchestrator series. Strict 1:1 lift — zero behavior change. ~333 lines moved out of web_server.py. What moved: - _download_track_worker → download_track_worker Dependencies bundled in `TaskWorkerDeps` (10 callbacks): - soulseek_client (with .mode + .hybrid_order + subclient attrs for hybrid fallback: .soulseek/.youtube/.tidal/.qobuz/.hifi/.deezer_dl) - matching_engine (.generate_download_queries) - run_async - try_source_reuse, store_batch_source, try_staging_match, get_valid_candidates, attempt_download_with_candidates, recover_worker_slot (web_server.py helpers — large, will lift in follow-up PRs) - on_download_completed (deferred to PR4g batch lifecycle) Direct imports for already-lifted: download_tasks, tasks_lock from core.runtime_state. SpotifyTrack from core.spotify_client. Behavior parity: - Same control flow: missing-task short-circuit → cancellation checkpoint with V2/legacy split → SpotifyTrack reconstruction with artist/album normalization → source-reuse shortcut → staging-match shortcut → searching-state init → query generation (matching engine + legacy fallbacks: track+first-artist-word with The-prefix handling, track-only, paren/bracket-cleaned) → case-insensitive dedup → sequential query loop with cancellation checks before/ during/after each search → hybrid fallback across remaining sources using first 2 queries → not_found marking with diagnostics → 2-tier exception recovery (failed marking + emergency worker_slot recovery) - Same logger messages text-for-text (so log filters keep working) - Same locking pattern (tasks_lock around every download_tasks read/ write, with the 2.0s timeout fallback in the exception path) - Same `cached_candidates` storage for retry fallback + raw-results storage for candidate review modal (top 20 per query without valid candidates) - Same V2 detection via `playlist_id` field — V2 tasks don't trigger on_download_completed for cancellation (V2 atomic cancel handles the worker slot itself) Tests: 19 new under tests/downloads/test_downloads_task_worker.py covering early-return guards (missing/cancelled-V2/cancelled-legacy/ cancelled-no-batch), source reuse + staging shortcuts, search loop happy path, no-results not_found, raw-results-stored-when-no-valid- candidates, attempt-download-failure-falls-through, cancellation mid- flight returns without completion, hybrid fallback (with + without hybrid mode), critical exception with + without recovery callback, query generation edge cases (The-prefix, paren cleanup, dedup). Full suite: 1001 passing (was 982). Ruff clean.	1 month ago
Broque Thomas	2d271cfacf	PR4e: lift status helpers + 3 routes to core/downloads/status.py ... Fifth sub-PR in the download orchestrator series. Strict 1:1 lift — zero behavior change. What moved: - _build_batch_status_data → build_batch_status_data - get_batch_download_status route body → build_single_batch_status - get_batched_download_statuses route body → build_batched_status - get_all_downloads_unified route body → build_unified_downloads_response - Status priority dict → module-level _STATUS_PRIORITY constant Dependencies bundled in `StatusDeps` dataclass: - config_manager, docker_resolve_path, find_completed_file, make_context_key, submit_post_processing (lambda wrapping missing_download_executor.submit + _run_post_processing_worker), get_cached_transfer_data Direct imports from core.runtime_state for download_tasks / download_batches / tasks_lock (already lifted by kettui). Behavior parity: - Same response payload shape across all 3 endpoints - Same safety-valve mutation: stuck downloading task with file recovered → status='post_processing' + submit worker; stuck searching → not_found; stuck downloading no file → failed - Same live transfer state mapping (Cancelled/Canceled, Failed/Errored/ Rejected/TimedOut, Completed/Succeeded with byte-mismatch verification, InProgress, default queued) - Same intermediate post_processing status promotion + single-shot worker submission (only when status != 'post_processing') - Same 'Errored' handling: keeps current status to let monitor retry - Same 17-key item dict in unified response with same field order - Same artist/album/artwork normalization (handles string, dict, list, list-of-dicts, list-of-strings variants) - Same sort: (priority asc, -timestamp desc) - Same batch summary aggregation - Same items[:limit] slicing - Same logger messages text-for-text - Same lock scope (single tasks_lock per call) — no new contention Pre-existing bug preserved (will fix in follow-up PR): - batched_status `debug_info` block iterates `response["batches"]` and guards with `if "error" not in batch_status`. Every successful payload includes `"error": batch.get('error')` (key always present, value usually None) so the guard is always False and debug_info never populates in production. Test documents the buggy behavior so the next PR can flip the check to `batch_status.get('error') is None`. Tests: 32 new under tests/downloads/test_downloads_status.py covering phase routing (analysis vs downloading vs unknown), task formatting + sort + V2 fields, every live transfer state mapping (Cancelled, Succeeded with full + partial bytes, InProgress, Errored, terminal- not-overridden), safety valve (stuck searching → not_found, stuck downloading recovered → post_processing, stuck downloading no file → failed), all 3 route helpers (single, batched, unified), unified artist/album/artwork normalization, batch summary aggregation, limit slicing, plus debug_info bug documentation. Full suite: 982 passing (was 950). Ruff clean.	1 month ago
Broque Thomas	a133448a6e	PR4d: lift _run_post_processing_worker to core/downloads/post_processing.py ... Fourth sub-PR in the download orchestrator series. Strict 1:1 lift — zero behavior change. ~407 lines moved out of web_server.py. What moved: - _run_post_processing_worker → run_post_processing_worker The lifted function is intentionally kept as one ~400-line block to preserve byte-for-byte parity with the original. Refactoring it into smaller helpers (context lookup, file search loop, transfer-folder handler, downloads-folder handler) gets its own follow-up PR. Dependencies: 9 callbacks bundled in `PostProcessDeps` dataclass. - config_manager, soulseek_client, run_async (live refs) - docker_resolve_path, extract_filename, make_context_key (small utilities still in web_server.py — will lift in a future PR alongside other shared utilities) - find_completed_file (file search helper, still in web_server.py) - enhance_file_metadata, wipe_source_tags (web_server wrappers around core.metadata.enrichment) - post_process_with_verification (web_server wrapper around core.imports.pipeline) - mark_task_completed (wraps runtime_state.mark_task_completed + session counter) - on_download_completed (deferred to PR4g batch lifecycle) Direct imports for already-lifted helpers (no injection needed): - core.imports.album_naming.resolve_album_group - core.imports.context.{get_import_clean_title, get_import_clean_album, get_import_original_search, get_import_context_artist, get_import_context_album, normalize_import_context} - core.imports.filename.extract_track_number_from_filename - core.metadata.enrichment (re-exported as metadata_enrichment) - core.runtime_state.{download_tasks, tasks_lock, matched_downloads_context, matched_context_lock} Behavior parity: - Same control flow: missing-task short-circuit → cancelled/completed short-circuit → missing-filename failure → docker path resolution → context lookup with fuzzy fallback → expected filename generation → YouTube special-case path resolution → 5-attempt search loop with Strategy 1 (original filename in download+transfer) and Strategy 2 (expected final filename in transfer) → file-not-found failure → transfer-folder handler with metadata enhancement → downloads-folder handler with full post-process verification - Same retry count (5), sleep duration (5s), per-attempt logging - Same album_info dict construction with is_album=True for explicit album downloads - Same album grouping skip when context.is_album_download is True - Same wipe_source_tags fallback when enhancement context missing - Same matched_downloads_context cleanup on success - Same exception swallowing at processing-error and critical-error layers, both setting status='failed' + error_message + calling on_download_completed(b, t, success=False) - Every logger message text preserved verbatim (so log filters keep working) Tests: 16 new under tests/downloads/test_downloads_post_processing.py covering missing task, cancelled, already-completed, stream_processed, missing filename + username, file-not-found-after-retries with sleep mocked, stream-processor-completes-mid-search, transfer-folder with metadata enhanced + with no context (wipes tags), downloads-folder with + without context, processing exception, critical outer exception, YouTube special path, fuzzy context matching. Full suite: 950 passing (was 934). Ruff clean.	1 month ago
Broque Thomas	039f152f31	PR4c: lift _automatic_wishlist_cleanup_after_db_update to core/downloads/cleanup.py ... Third sub-PR in the download orchestrator series. Strict 1:1 lift — zero behavior change. What moved: - _automatic_wishlist_cleanup_after_db_update → cleanup_wishlist_after_db_update The lifted fn takes config_manager as an arg (so core/downloads/cleanup.py doesn't need to import web_server). Other deps (wishlist_service, MusicDatabase, get_database) stay as in-function imports — matches the original deferred-import pattern. The single caller in web_server.py (missing_download_executor.submit at L18028) keeps using the same wrapper name with no signature change. Behavior parity: - Same per-profile iteration via get_all_profiles() - Same essential-field skip (no name / no artists / no spotify_track_id) - Same artist normalization (string / dict / fallback to str()) - Same 0.7 confidence threshold for db match - Same break-on-first-artist-match semantics - Same album extraction (dict.name vs string passthrough) - Same active_server pulled via config_manager.get_active_media_server() - Same per-track exception swallowing inside the loops - Same top-level exception swallow with traceback.print_exc() - Same logger messages (exact text match for "[Auto Cleanup]" prefix) Tests: 13 new under tests/downloads/test_downloads_cleanup.py covering empty wishlist short-circuit, found-in-db removal, missed track stays, low-confidence skip, missing-fields skip, dict + string artist formats, break-on-first-match, multi-profile walk, album dict/string handling, db check failure continuing to next artist, top-level exception swallow, active server propagation. Full suite: 934 passing (was 921). Ruff clean.	1 month ago
Broque Thomas	dc2835eecc	PR4b: lift cancel + clear download routes to core/downloads/cancel.py ... Second sub-PR in the download orchestrator series. Strict 1:1 lift — zero behavior change. What moved: - cancel_download (single slskd cancel) → cancel_single_download - cancel_all_downloads (cancel + clear + sweep) → cancel_all_active - clear_finished_downloads (slskd clear + sweep) → clear_finished_active - clear_completed_downloads (local task tracker prune) → clear_completed_local Slskd-touching helpers take (soulseek_client, run_async, sweep_callback) explicitly so the route layer wires the live client + the existing _sweep_empty_download_directories helper. The local-state helper imports download_tasks/download_batches/batch_locks/tasks_lock straight from core.runtime_state since those are module-level shared globals. Prep change: `batch_locks` dict moved from web_server.py global into core/runtime_state.py alongside the other download globals. web_server.py re-imports from runtime_state so the ~3 existing call sites in web_server.py keep resolving without modification. Identity preserved (same dict across all importers). Out of scope (deferred to PR4g batch lifecycle): - cancel_download_task (calls _on_download_completed) - cancel_task_v2 + _atomic_cancel_task + _find_task_by_playlist_track (manipulate batch active_count directly, deeply coupled to lifecycle) Behavior parity: - Same response shapes + status codes on each route - Same call order (cancel_all → clear_all_completed → sweep) - Same conditional sweep on clear_finished (skipped on failure) - Same sweep ALWAYS runs after cancel_all even if clear_all returns False (matches original — clear failure was non-fatal in cancel_all path) - Same TERMINAL_STATUSES set: completed/failed/not_found/cancelled/skipped/ already_owned (lifted to module-level constant) - Same empty-batch pruning + same batch_locks cleanup - Same lock acquisition pattern (single tasks_lock) Tests: 14 new under tests/downloads/test_downloads_cancel.py covering single cancel, cancel-all happy + failure paths, clear-finished + sweep gate, local task pruning across all 7 active/terminal states, batch queue trimming, batch_locks cleanup. Full suite: 921 passing (was 907). Ruff clean.	1 month ago
Broque Thomas	3ce25310a3	PR4a: lift sync history recording to core/downloads/history.py ... First sub-PR in the download orchestrator series. Strict 1:1 lift — zero behavior change. What moved: - _record_sync_history_start → record_sync_history_start - _record_sync_history_completion → record_sync_history_completion - _detect_sync_source → detect_sync_source - Source prefix map → module-level _SOURCE_PREFIX_MAP constant What stayed: - web_server.py keeps three thin wrappers (_detect_sync_source, _record_sync_history_start, _record_sync_history_completion) that delegate into core/downloads/history.py. ~60 callers of these names in web_server.py keep resolving without touching every site. Each lifted function takes `database` as an arg (was `db = MusicDatabase()` inline). The wrappers construct `MusicDatabase()` per call to mirror the exact original behavior — each invocation got a fresh DB connection. Behavior parity: - Same SQL UPDATE statement (preserves the in-place update path when a sync_history entry already exists for the playlist_id) - Same JSON serialization with ensure_ascii=False - Same thumb URL extraction order (album_context.images → image_url → first track album.images) - Same per-track result shape (index, name, artist, album, image_url, duration_ms, source_track_id, status, confidence, matched_track, download_status) - Same status mapping (found/not_found, completed/failed) - Same best-effort exception swallowing (sync history failure must never break the actual download) - Reads `download_tasks` from core.runtime_state (already lifted by kettui in PR378) Tests: 34 new under tests/downloads/test_downloads_history.py covering source detection (16 prefixes), start happy paths + thumb extraction + duplicate-update + DB error swallowing, completion stats + per-track results JSON shape + edge cases. Full suite: 907 passing (was 873). Ruff clean.	1 month ago
Broque Thomas	c121582557	MusicBrainz genres: fall back to release then artist when recording is empty ... User report: SoulSync was only pulling MusicBrainz genres from the recording (track-level) endpoint. Most MB recordings don't carry genres at the track level — they live on the release (album) or artist. So the MB tier was contributing nothing to the genre merge for the overwhelming majority of tracks. Fix: - Added `'genres'` to the release-detail `includes` (was missing). - After release-detail processing, if pp['mb_genres'] is still empty, populate from release_detail['genres'] (sorted by count desc). - If still empty AND artist_mbid is set, fetch artist with `includes=['genres']` and use those. No extra API call when the recording (or release) already had genres — the artist fetch only fires when both upstream tiers came back empty. The downstream genre merge in _embed_metadata_genres is unchanged; this just makes the MB feed into it richer. Tests: 4 new (recording present, recording empty → release, recording + release empty → artist, all empty → []). Full suite 873 passing. Ruff clean. Reported by @kcaoyef421 in Discord.	1 month ago
Broque Thomas	a8319156ce	Lift /api/automations/blocks static config into core/automation/blocks.py ... The endpoint was returning a 200-line literal dict inline. Moved the three lists (TRIGGERS, ACTIONS, NOTIFICATIONS) to module-level constants in core/automation/blocks.py. Route shrinks to 7 lines. Data is now importable for tests + future docs. Added 8 shape tests so a typo in the dict (missing 'type', wrong field type, missing options on a select, etc.) gets caught by CI instead of breaking the builder UI silently. The `known_signals` field stays computed at request time via _collect_known_signals(database) since it's dynamic. No behavior change. Same response shape. 869 tests passing (was 861). Ruff clean.	1 month ago
Broque Thomas	6cdcf778f3	Lift /api/automations/* into core/automation/ ... Routes moved to thin parse-args/jsonify handlers; logic now lives in three focused modules under core/automation/. 436 lines deleted from web_server.py; 53 added back as wrappers. Module split: - core/automation/api.py — CRUD + run + history helpers. Each function takes (database, automation_engine, ...) explicitly and returns (response_body, http_status). Includes signal cycle detection preflight checks for create + update. - core/automation/progress.py — owns the in-memory progress state dict + lock (mirroring the original web_server.py globals as module-level shared state so all callers see one view), init/update/history helpers, and the WebSocket emit loop. - core/automation/signals.py — collect_known_signals for the builder autocomplete. Out of scope (deferred): - _register_automation_handlers — the 23+ action handler closures stay in web_server.py because each one is tightly coupled to feature- specific implementations (wishlist, watchlist, library scan, etc.). - Worker functions (_process_wishlist_automatically, etc.) — belong with their feature lifts. - _run_sync_task / _run_playlist_discovery_worker — sync + discovery PRs. Behavior preserved 1:1: - Same route response shapes + status codes - Same JSON field hydration (trigger_config, action_config, notify_config, last_result, then_actions) - Same backward-compat: empty then_actions + notify_type set → synthesize then_actions from notify_type/notify_config - Same signal cycle detection behavior on create + update - Same system-automation protection on delete + duplicate - Same reschedule/cancel logic on toggle + bulk-toggle + update - Same progress state shape (status, progress, phase, current_item, log capped at 50, started_at/finished_at, action_type) - Same emit-on-finish socketio push from update_progress - Same emit loop semantics (1s tick, snapshot active states, reap finished after window) Pre-existing bugs preserved (will fix in follow-up PRs): - emit_progress_loop uses naive datetime.now() against tz-aware started_at/finished_at, so the timeout-zombie check raises TypeError → caught → never fires, and the cleanup-after-window check raises → caught → state is reaped on FIRST tick regardless of the window. Tests document this behavior so the next PR can flip them to the corrected expectation. Tests: 72 new under tests/automation/ (signals 10, progress 24, api 38). Full suite: 861 passing (was 789). Ruff clean.	1 month ago
Broque Thomas	b94cbd7dd7	Search lift: pre-merge parity polish for cin's review ... Three drifts caught in line-by-line review against the pre-lift web_server.py. All addressed for strict 1:1 behavior parity. 1. /api/enhanced-search/source/<src> now returns plain JSON `{"artists":[],"albums":[],"tracks":[],"available":false}` (or `{"videos":[],"available":false}` for youtube_videos) when the source's client isn't available, matching the original endpoint contract. Previously streamed an NDJSON `{"type":"done"}` line instead. Restructured by splitting the orchestrator into resolve+stream helpers: - `resolve_client(source_name, deps)` — already existed, used for /api/enhanced-search single-source mode - `resolve_youtube_videos_client(deps)` — new, returns the soulseek_client.youtube subclient or None - `stream_metadata_source(source_name, query, client)` — pure NDJSON generator, caller resolves client first - `stream_youtube_videos(query, youtube_client, run_async)` — same shape for the yt-dlp path The route now decides plain-JSON-vs-stream based on resolution result, mirroring the original control flow exactly. 2. core/search/library_check.py — reverted the defensive `(x or '')` and `getattr(plex_client, 'server', None) is not None` patterns to original byte-for-byte (`x.get('name', '')`, `plex_client.server`, no try/except around `get_plex_config`). Lift PR shouldn't change crash semantics; if the original raises on malformed input, mine should too. Pre-existing edge cases get their own follow-up PR. 3. core/search/stream.py — same revert: `soulseek_client.youtube` instead of `getattr(..., 'youtube', None)` etc. Also removed the module-level `EMPTY_SOURCE` from sources.py and moved its (per-call) duplicate into _fan_out_response as a local — the original used a per-request local dict and the identity-check behavior depends on that. Module-level was a footgun for future mutations. 789 tests still pass (95 search), ruff clean.	1 month ago
Broque Thomas	47b4663091	Search cache: preserve falsy provider returns to match original behavior ... Line-by-line review of the search lift caught one drift: cache.get_cache_key was coercing falsy provider returns ('', None, 0) to 'unknown' / False. Original web_server.py only fell back to those sentinels on exception, not on falsy success values. Real-world impact: low — get_active_media_server() and get_primary_source() return non-empty strings in practice. But cache keys are tuples with no schema enforcement, so any drift here can silently fragment the cache. Restored 1:1 parity with original semantics. Added test covering the falsy-success path so this can't drift again. 789 tests pass, ruff clean.	1 month ago
Broque Thomas	fd7b56e58c	Lift /api/search and /api/enhanced-search/* into core/search/ ... Routes moved to thin parse-args/jsonify handlers; logic now lives in six focused modules under core/search/. 720 lines deleted from web_server.py; 109 added back as wrappers; ~700 lines of new core code plus ~700 lines of tests. Module split: - core/search/cache.py — TTL+LRU cache for enhanced-search responses, keyed by (query, active_server, fallback_source, hydrabase_active, source_tag) so config changes don't poison stale entries. - core/search/sources.py — per-kind metadata search (artists/albums/ tracks) and the multi-kind ThreadPoolExecutor that fans them out. - core/search/library_check.py — library + wishlist presence check with Plex thumb URL resolution; profile-aware wishlist with legacy fallback for older DBs missing the profile_id column. - core/search/stream.py — single-track preview search; effective stream mode resolution, query-variant generation, retry walk, matching engine integration. - core/search/basic.py — flat Soulseek file search, quality-sorted. - core/search/orchestrator.py — main enhanced-search dispatch (short-query fast path, single-source bypass, hydrabase-primary fan out, alternate source list builder), NDJSON streaming generator for /source/<src>, and the SearchDeps dataclass that bundles the cross-cutting deps. Routes pass clients (spotify, hydrabase, hydrabase_worker, soulseek) and helpers (config_manager, fix_artist_image_url, _is_hydrabase_active, _get_metadata_fallback_*, _run_background_ comparison, run_async, dev_mode_enabled_provider) into core/search via a SearchDeps bundle built per-request. fix_artist_image_url stays in web_server.py because it touches 31 other call sites. Behavior preserved 1:1: - Same response shapes (db_artists, spotify_artists, spotify_albums, spotify_tracks, primary_source, metadata_source, alternate_sources, source_available) - Same NDJSON line ordering (artists/albums/tracks as they finish, plus done marker) - Same per-kind exception swallowing - Same hydrabase-worker mirror on dev mode - Same cache key shape (5-tuple) and TTL/LRU semantics - Same stream-track effective-mode resolution including the Soulseek-coerce-to-YouTube edge case - Same library-check Plex thumb URL rewriting and wishlist fallback for older DBs Tests: 94 new (cache TTL/LRU/key, sources happy/partial/all-fail, library presence with library + wishlist + thumbs, stream effective mode + query gen + retry, orchestrator client resolution + short query + single source + fan-out alternates + hydrabase primary + NDJSON drain). Full suite: 788 passing (was 694). Ruff clean.	1 month ago
Broque Thomas	f51b75da7e	Lift /api/stats/* and /api/listening-stats/* into core/stats/ ... Stats route logic moves into core/stats/queries.py as pure-ish functions that take dependencies (database, image-url fixer, listening worker) as arguments. The 13 route handlers in web_server.py shrink to thin parse-args / jsonify wrappers. What moved to core/stats/queries.py: - stats_cached: 3-key metadata cache lookup + image url fix-up - stats_overview / timeline / genres / library_health / db_storage - stats_top_artists / top_albums / top_tracks: top-N + DB enrichment - stats_recent: listening_history readback - stats_resolve_track: title+artist -> file_path lookup for playback - listening_stats_sync: spawns daemon thread that runs worker._poll - listening_stats_status: stats payload, with None-worker fallback shape No behavior change. Same response shapes, same error handling, same silent-except on per-row enrichment failure. fix_artist_image_url stays in web_server.py and is passed through as a callback so we don't have to lift its config_manager / media-server dependencies in this PR. Adds tests/stats/test_stats_queries.py — 27 tests covering happy paths, edge cases, image-url plumbing, worker glue. Ruff clean. 694 tests pass (was 667 + 27 new).	1 month ago
Broque Thomas	313b5677a5	Drop stale post-PR378 redefs and fix B009 ... Lifted-then-not-deleted leftovers from the PR378 merge: - web_server.py `_resolve_album_group` and `_build_final_path_for_track` were already imported at module top from `core/imports/`. Removed the shadowing local copies. - Mutagen reimports (FLAC/MP4/OggVorbis) at L17736-17738 shadowed the top-of-file imports. Picture/MP4Cover/MP4FreeForm were unused. Dropped the whole block. - core/imports/context.py: `getattr(artist, "name")` -> `artist.name` (B009). Ruff clean, 667 tests pass.	1 month ago
Antti Kettunen	02305096a3	Tighten metadata and import safety ... - Normalize album import track display handling so queue labels and match rows stay consistent - Bound MusicBrainz caches and avoid caching transient lookup failures - Stop swallowing programmer errors in source enrichment helpers - Restore import config test seams without reintroducing lazy imports - Guard task completion calls and fix the Windows path test expectation - Keep file lock tracking from growing without bound	1 month ago
Antti Kettunen	9315e74bea	Broaden import and metadata test coverage ... - Cover search_result fallback normalization and ambiguous album detection. - Add staging metadata, multi-disc path, and MusicBrainz enrichment cases. - Move the single-track context test next to the imports code it exercises.	1 month ago
Antti Kettunen	4f236baa6d	Fix import normalization and task completion locking ... - Promote legacy _source into source during import normalization. - Keep the normalized import context neutral after stripping aliases. - Avoid re-entering tasks_lock when marking completed download tasks.	1 month ago
Antti Kettunen	4c819681a1	Move single-track resolver; fix wishlist cleanup ... - keep single-track import lookup in imports/resolution.py - normalize simple-download search_result data before wishlist matching - run wishlist cleanup for simple-download post-processing - keep source-only artist detail on resolved names and MB short-circuit	1 month ago
Antti Kettunen	d04573f397	Fix single import source handling ... - pass the selected manual match through singles import - keep the import context source-aware so artist and album stay correct - avoid treating non-Spotify IDs as wishlist Spotify IDs - make wishlist logging and local variable names source-neutral	1 month ago
Antti Kettunen	9b2b6d856f	Split runtime builders into owning modules ... - Move the import pipeline runtime factory into core.imports.pipeline - Move the metadata runtime factory into core.metadata.enrichment - Keep the web server wiring thin and drop the shared glue module - Add contract tests that keep the two runtime bundles separate	1 month ago
Antti Kettunen	9e496397da	Move shared metadata helpers into package ... - Relocate the shared metadata helper module from core/metadata_common.py into core/metadata/common.py. - Update the new metadata package, the import pipeline, and the web entrypoint to use the package-scoped helper. - Keep the shared config, mutagen, file-lock, and tag-writing helpers centralized without touching unrelated files.	1 month ago
Antti Kettunen	9656dbd46a	Thread runtime through metadata enrichment ... - Pass the live runtime bundle into the shared metadata facade so worker-backed source enrichment can actually run. - Forward runtime from the import pipeline and web-server wrapper into embed_source_ids. - Add a regression test that verifies the runtime object reaches the source-ID embedding path.	1 month ago
Antti Kettunen	8319c6679f	Move new metadata helpers into a package ... - Keep existing metadata_cache and metadata_service at the top level for now - Move the new branch-local metadata helpers under core/metadata - Share MusicBrainz release cache state from core.metadata.source and update import sites	1 month ago
Antti Kettunen	bdef127dd6	Lift shared runtime state into core ... - Move app-wide task and activity registries out of core/imports - Share one runtime-state module across the web server, API, and import pipeline - Keep import-specific helpers focused on context and post-processing	1 month ago
Antti Kettunen	e10df4caf2	Rehome import helpers into core/imports ... - Move import flow modules into a dedicated package - Update app and test imports to the new namespace - Group the import-focused tests under tests/imports	1 month ago
Antti Kettunen	b9269b4f16	Tighten metadata helper boundaries ... - remove stale wrapper helpers from web_server and metadata_common - import provider helpers directly in metadata_source - keep the metadata modules' public surface explicit	1 month ago
Antti Kettunen	edd9048f86	Checkpoint metadata runtime cleanup ... - remove runtime from metadata helper APIs where it only carried config, logger, mutagen, and database access - keep runtime only for the source-ID enrichment path that still needs live worker handles - add the new metadata helper modules and update the tests to match the slimmer interfaces	1 month ago
Antti Kettunen	6872e5080d	Refine import module boundaries ... - Move filename and staging helpers into their canonical modules - Extract album naming and grouping from path handling - Update import and test call sites to the new layout	1 month ago
Antti Kettunen	0bbf44809f	Move the import flows and related post-processing pipelines into separate modules ... - Extract the import pipeline, album import, staging, path, file ops, guards, runtime state, side effects, and metadata enrichment out of . - Canonicalize the refactored import path around and remove legacy , , , and request shapes from the import endpoints. - Make album and track metadata lookups follow the configured provider priority instead of hard-coding Spotify, while still falling back when needed. - Update the import routes and frontend payloads to use the new core helpers. - Add coverage for the extracted helpers and the refactored import flows. PS. apologies to anyone who might check this commit out - the intention was to start small, but things kinda snowballed out of control at some point since the logic just kept going on and on, and everything kinda had to be changed all at once for it all to make any sense	1 month ago
Broque Thomas	dd4cf130d7	Socket.IO CORS: handle self-review nits ... Six items from a Cin-style line-by-line pass on PR #383: - resolve_cors_origins: list of non-string entries (`[None, 123]`) now drops them instead of coercing to junk strings like `'None'`/`'123'`. - will_reject: backwards-compat shim removed. Production callers always pass `request.scheme` (Flask-guaranteed); the shim only existed for tests/non-Flask callers and made the production code path branchier than necessary. Tests now pass scheme explicitly. - maybe_log: redundant `if not origin` early-return dropped. will_reject handles missing origin (engineio's own behavior — server.py:207). - RejectionLogger.__init__: `int(dedup_cap)` wrapped in try/except so bad-type input falls back to DEFAULT_DEDUP_CAP instead of raising. - web_server.py: docstring on the before_request hook explains why the hook fires on every request (Flask doesn't scope before_request to a path prefix; the early-return string compare is the cheapest option). - settings.js: cors-origins URL regex tightened from `[^\s/]+` to `[^\s/?#]+` so query/fragment chars don't pass validation. Engineio would silently fail to match those anyway; better to flag at save. Test changes: - parametrize gained an explicit `scheme` column (12 cases updated). - New explicit case: scheme-mismatch rejects (engineio compares full `{scheme}://{host}` strings). - `test_will_reject_falls_back_to_host_only_when_no_scheme_info` deleted — the shim it tested is gone. - `test_will_reject_honors_x_forwarded_host` now passes scheme info. Net: -9 production lines, -3 test lines. Production code path is straight-line. 603 tests pass.	1 month ago
Broque Thomas	0f24739e27	Socket.IO CORS: polish — match engineio exactly, bound dedup, validate URLs ... Self-review pass on the security fix uncovered five issues, all fixed here: 1. will_reject scheme handling. Engineio compares full {scheme}://{host} strings, not just hostnames. A TLS-terminating proxy can leave the backend seeing http while the browser's Origin is https — engineio rejects, but the original predictor said "allow" → no helpful log line. Added request_scheme + forwarded_proto params, build full candidate strings to match engineio. 2. EITHER-forwarded-header rule. Engineio adds the forwarded candidate when EITHER X-Forwarded-Proto OR X-Forwarded-Host is present (it falls back to HTTP_HOST for the missing one). The original predictor only added it when forwarded_host was set — false negative for misconfigs sending only X-Forwarded-Proto. Now mirrors engineio. 3. will_reject incorrectly rejected missing-Origin requests. Engineio (server.py:207: `if origin: validate`) skips CORS validation when no Origin header is sent — non-browser clients (curl etc.) are intentionally permitted. The original code rejected them. Test was asserting the wrong behavior. Both fixed. 4. RejectionLogger had unbounded dedup set growth. A hostile actor opening connections from many distinct fake origins would fill memory unboundedly. Capped at 100 unique origins (configurable); when cap hit, one overflow notice is emitted and further rejections are silently dropped until restart. 5. Lock pattern: the overflow log path called logger.warning() while holding the dedup lock, inconsistent with the normal path. Fixed to pick the message under the lock and log after release. Critical section is now minimal and uniform. Plus polish: - Stale module docstring fixed (said "empty list" instead of "None"). - settings.js validates each cors_origins line against a URL regex on save; toasts a one-shot warning if entries are malformed (resolver silently filters them, but user gets feedback now). - web_server.py wiring passes request.scheme + X-Forwarded-Proto so the predictor has full proxy info. Tests: - 51 unit tests in tests/test_socketio_cors.py (was 45). New cases: * scheme comparison (5 cases including TLS-terminating proxies) * forwarded_proto-alone misconfig * missing-origin matches engineio (was asserting wrong behavior) * dedup cap with overflow + reset * default cap is reasonable (uses public DEFAULT_DEDUP_CAP constant) Engineio behavior independently verified by reading engineio/server.py and engineio/base_server.py source. Predictor mirrors both files. 604 tests pass.	1 month ago
Broque Thomas	013eebf350	Lock down Socket.IO CORS — same-origin default + opt-in allow-list ... Closes #366 (reported by JohnBaumb). Socket.IO was initialized with `cors_allowed_origins='*'`, accepting WebSocket connections from any origin. A malicious site could open a WS to a user's local SoulSync instance and exfiltrate live progress / toast / activity events. This commit: - Defaults to engineio's same-origin behavior (`cors_allowed_origins=None`), which automatically honors X-Forwarded-Host so reverse proxies that send that header (Caddy / Traefik by default, properly-configured Nginx) work transparently. - Adds a `security.cors_origins` config setting + Settings → Security textarea where users behind unusual proxies / Electron wrappers / cross-origin integrations can whitelist their origin. Accepts comma or newline separated values; `*` on its own line opts back into the legacy wildcard with a startup-warning log. - Logs a clear warning the first time engineio rejects each unique origin, naming the rejected Origin and request Host and pointing users to the settings field. Without this, engineio silently 403s the upgrade and the user just sees a half-broken UI with no clue why. Threadsafe dedup so a hostile origin can't spam logs. Logic lives in `core/socketio_cors.py` (resolver, rejection predictor, dedup logger class, startup-status emitter) — pure functions, no Flask dependency. `web_server.py` adds 23 lines of wiring and imports. Important catch during review: my first pass used `cors_allowed_origins=[]` as the "secure default." Reading engineio's source revealed `[]` actually means "DISABLE CORS HANDLING" (engineio/server.py:202: `if cors_allowed_origins != []:`) — identical security to `'*'`. Fixed to use `None` (engineio's actual same-origin sentinel) and pinned with a regression test that asserts the resolver never returns `[]` for any input shape. Tests: - tests/test_socketio_cors.py — 45 unit tests covering 19 resolver shape cases (None, empty, whitespace, comma, newline, garbage types, lists), the `[]`-must-never-be-returned security regression, 12 rejection prediction cases, X-Forwarded-Host handling, dedup logger behavior, threadsafe race (8 threads × 50 hammers → exactly 1 warning), and startup-status emitter outputs. Frontend: - Settings → Security gains an "Allowed WebSocket Origins" textarea with help text explaining same-origin default + when to add a domain + the `*` opt-out. - helper.js — new '2.4.1' WHATS_NEW block (hidden until version bump) with a chill-voice entry describing the change. Conftest.py left at `'*'` — test environment, no security concern. 598 tests pass.	1 month ago
Broque Thomas	37aefd2ff1	Reorganize queue: race + dedupe fixes from kettui review ... Five issues kettui flagged on PR #377: - Worker race (reorganize_queue.py): _next_queued() picked an item and released the lock, then re-acquired to flip status='running'. A cancel() landing in that window marked the item cancelled but the worker still ran it. Replaced with _claim_next_or_wait() that picks AND flips under one lock acquisition. - Wakeup race (reorganize_queue.py): _wakeup.clear() after the empty check could lose an enqueue's _wakeup.set(), parking a freshly-queued album for up to 60 seconds. Replaced Lock + Event with a single threading.Condition; cond.wait() releases and re-acquires atomically on notify. - Bulk dedupe (reorganize_queue.py:enqueue_many): looped single-item enqueue, so a duplicate album_id later in the same batch could slip through if the worker finished the first copy before the loop reached the second. Now holds the lock for the whole batch and tracks a per-batch seen set, so intra-batch duplicates dedupe against each other and not just pre-existing items. - Preview button stuck disabled (library.js:loadReorganizePreview): early returns and thrown errors skipped the re-enable line. Moved state into a canApply flag committed in finally, so any exit path lands the button correctly. - DB helpers swallowing failures (music_database.py): get_album_display_meta and get_artist_albums_for_reorganize used to catch every Exception and return None / [], so a real DB outage masqueraded as "album not found" / "no albums". Now lets exceptions bubble; the route layer already wraps them as 500. Tests: - test_cancel_and_run_are_mutually_exclusive — hammers enqueue+cancel pairs and asserts the invariant that no successfully-cancelled item ever ran (catches regressions to the atomic pick). - test_enqueue_many_dedupes_batch_internal_duplicates — pins the intra-batch dedupe. - test_get_album_display_meta_propagates_db_errors and test_get_artist_albums_for_reorganize_propagates_db_errors — pin the bubble-up behavior. Changelog updated in helper.js and version modal.	1 month ago
Broque Thomas	d6094a3587	Library reorganize: FIFO queue with live status panel ... Replaces the single-slot "one reorganize at a time, return 409 on collision" model with a per-user FIFO queue. Buttons stay clickable, "Reorganize All" is one backend call instead of an N-call JS loop, and a status panel mounted at the top of the artist actions bar shows live progress (active item, queued count, recent completions) with per-item cancel buttons. Backend - core/reorganize_queue.py: singleton queue + worker thread, dedupe-on- enqueue, cancel rules (queued cancellable, running not), enqueue_many for bulk operations, progress fan-out via update_active_progress - core/reorganize_runner.py: factory builds the worker's runner closure with injected dependencies. Reads config per-call so changing the download path in Settings takes effect on the next reorganize without a server restart - database/music_database.py: get_album_display_meta and get_artist_albums_for_reorganize — moves the SQL out of route handlers - web_server.py: thin enqueue/snapshot/cancel/clear endpoints, runner registration at module load. Old _reorganize_state globals + status endpoint deleted. Static-asset cache buster (?v=<server-start>) added so JS/CSS updates ship live without users clearing cache Frontend - webui/static/library.js: status panel mount, polling (1.5s when active, 8s when idle), expand/collapse, per-item cancel, debounced enhanced-view reload (one reload per artist batch instead of N). Per-album reorganize button paints with queued/running indicator and short-circuits to a toast when the album is already in queue - webui/static/style.css: panel + button styling matching the existing glass-UI accents - webui/static/helper.js + version modal: WHATS_NEW entry Tests (22 new) - tests/test_reorganize_queue.py (19 tests): FIFO order, dedupe, per-item source, cancel rules, continue-on-failure, snapshot shape, progress propagation, bulk enqueue - tests/test_reorganize_runner.py (4 tests): per-call config reads, setup-failure summary, dependency injection, progress fan-out - tests/test_reorganize_db_methods.py (7 tests): SQL JOIN behavior, ordering, fallback for blank strings, artist isolation Full suite 549 passed in 27s.	1 month ago
Broque Thomas	98c85f928e	Merge remote-tracking branch 'origin/dev' into fix/reorganize-via-post-process-pipeline ... # Conflicts: # webui/static/helper.js	1 month ago
Broque Thomas	7e1c4c26ec	Reorganize: fix moved-count + status/total UX issues from PR #377 review ... Four changes addressing kettui's PR #377 review comments: 1. **`_finalize_track` no longer over-counts on DB failure (🔴 bug).** The function previously bailed on DB-update failure but `_process_one_track` still incremented `summary['moved']` unconditionally — overstating how many tracks the UI knows are at their new locations. Fixed by: - `_finalize_track` now returns ``bool`` (True only when DB row was updated AND original was dealt with) - Caller checks the return; on False, records as a failed track with a clear message ("Track landed at new location but DB update failed — file is at both old and new paths until library scan re-indexes") - Existing `test_db_update_failure_leaves_original_in_place` now also asserts `moved == 0`, `failed == 1`, and that the error message names the cause 2. **`executeReorganize` toast no longer says "undefined tracks" (🐛 bug).** `/reorganize` doesn't return `result.total` anymore (the track count is determined server-side after planning), so the "Reorganizing undefined tracks..." string was meaningless. Now uses `result.message` from the backend instead. 3. **`_pollReorganizeStatus` distinguishes completed from skipped (🟡 risk).** Backend now propagates the orchestrator's status (`completed` / `no_source_id` / `no_album` / `no_tracks` / `setup_failed` / `error`) into `_reorganize_state['result_status']` so the frontend can warn appropriately. Two new helpers: - `_classifyReorganizeOutcome(state)` — returns 'success' only when `result_status === 'completed'` AND `failed === 0`; 'warning' otherwise - `_formatReorganizeResultMessage(state)` — returns a message specific to the outcome ("Reorganize skipped — album has no metadata source ID. Run enrichment first." for `no_source_id`, etc.) Zero-failure non-completed runs now show as warnings instead of green checkmarks. 4. **Bulk mode no longer counts skipped albums as succeeded (🟡 risk).** `_executeReorganizeAll`'s loop was treating any HTTP 200 response as success, ignoring the orchestrator's actual outcome for that album. Fixed by: - `_waitForReorganizeComplete()` now resolves with the final state object (was: void) - Loop checks `finalState.result_status === 'completed'` AND `finalState.failed === 0` before counting `succeeded++`; otherwise increments `skipped` (with a per-album warning toast) or `failed` accordingly - Final summary toast now reads "Reorganized N of M albums, K skipped, J failed" and only shows green when nothing was skipped or failed All four addressed in a single commit because they form one coherent UX-correctness fix — the bug bug (#1) and the count- overstatement bug (#4) both made the user see "everything succeeded" when reality was different. Together they make the UI honestly reflect what actually happened. Files: - core/library_reorganize.py — `_finalize_track` returns bool, `_process_one_track` reads it - web_server.py — `_reorganize_state['result_status']` populated from orchestrator's summary on success and on exception - webui/static/library.js — `_classifyReorganizeOutcome` / `_formatReorganizeResultMessage` helpers, single-album + bulk-mode flows both consume them - tests/test_library_reorganize_orchestrator.py — strengthened the existing DB-failure test to assert moved/failed counts Credit: kettui — four PR #377 review comments named all of these precisely with line numbers and severity.	1 month ago
Broque Thomas	6c90d68de3	Discogs: count rows with empty type_ as real tracks too ... Reported by kettui on PR #374 review: the inline filter that backed `set_album_api_track_count` only counted rows where `type_ == 'track'`, but `discogs_client.get_album_tracks` itself accepts both `'track'` AND empty `type_` as real songs (line 660: `type_ in ('track', '')`). Releases where Discogs returns some real tracks with an empty `type_` field would be undercounted, which would silently disagree with the repair job's fallback `_get_expected_total` path (which calls into `get_album_tracks_for_source` and therefore uses the client's count). Extracted the filter into `count_discogs_real_tracks(tracklist)` — single source of truth for the rule, testable in isolation, and the worker call site is now a one-liner that names what it's doing. Also defensive about the input shape: `type_ == None`, missing field, and empty/None tracklist all handled cleanly. 10 tests pin the behavior: - empty/missing/None type_ all count as a real track (the kettui case) - 'heading', 'index', 'sub_track' excluded - unknown future type strings excluded conservatively - realistic multi-disc tracklist with mixed shapes counts correctly - empty/None input returns 0 without raising Credit: kettui — the PR #374 review comment that flagged this.	1 month ago
Broque Thomas	cb67773998	Merge remote-tracking branch 'origin/dev' into fix/album-completeness-api-track-count ... # Conflicts: # webui/static/helper.js	1 month ago
Broque Thomas	2b15260b88	Reorganize: route library files through the post-processing pipeline ... Reported on Discord by winecountrygames. The library "Reorganize" tool had several layered bugs that all traced to the same root cause: the endpoint reinvented every wheel post-processing already turns — its own template engine, its own disc-number resolution from file tags, its own sidecar sweep, its own collision detection — and each had drifted from the canonical path used by fresh downloads. Reported symptoms: - 3-disc Aerosmith deluxe collapsed to a flat single-disc layout - Half the tracks on other albums silently skipped, no error / no count - Re-runs left empty leftover album folders cluttering the artist dir Architecture: stop reinventing wheels. Route reorganize through exactly the same pipeline downloads use. Per-album: 1. Fetch the canonical tracklist from a metadata source (Spotify / iTunes / Deezer / Discogs / Hydrabase) using the album's stored source IDs. New `core/library_reorganize.py::plan_album_reorganize` does this — primary-source-first, fall through priority chain unless the user picked a specific source in the modal (strict mode). 2. For each local track, find the matching API entry via a scored candidate matcher. Score components: exact-title (100), substring-with-length-ratio (40-90), track-number agreement (20). Hard reject when the two titles have different version differentiators (Remix vs no-remix means different recordings, not annotation drift). Below threshold = unmatched, surfaced as "not in source's tracklist, left in place" rather than silently mis-routing. 3. Copy the file to a per-album staging directory, build the same context dict the import flow builds (`spotify_album` / `track_info` / etc. with `is_album_download=True` so the path builder enters ALBUM mode, not SINGLE mode), call `_post_process_matched_download(...)` — same function fresh downloads use. Post-process handles tagging, multi-disc subfolder decisions, sidecar regeneration, AcoustID verification. 4. Read `context['_final_processed_path']` to learn where it landed. Update `tracks.file_path` in the DB BEFORE removing the original (DB-update failure leaves the file at both locations, recoverable via library scan; the reverse would orphan the row). Delete per-track sidecars (post-process recreates them at the new destination). 3 concurrent workers per album via ThreadPoolExecutor, matching the download path's per-batch worker count. State mutations all guarded by a single lock; staging filenames carry a UUID prefix so concurrent copies of identically-named source files don't overwrite each other. Source picker in the modal lets the user choose which source to read the tracklist from. Two endpoints feed it: - `/api/library/album/<id>/reorganize/sources` — sources for THIS album that are both authed AND have a stored ID. For the per- album modal. - `/api/library/reorganize/sources` — all authed sources globally. For the bulk "Reorganize All" modal where per-album ID coverage varies. When the user picks a specific source, the orchestrator runs in `strict_source=True` mode (no fallback chain) — picking Spotify means "use Spotify or fail", not "use Spotify and silently fall back." Preview endpoint shares the same planning logic as apply via `preview_album_reorganize` — the destination path comes from the same `_build_final_path_for_track` post-process uses, so what you see in the preview is exactly what you get on apply. Empty destination folders (from earlier failed runs OR from the current run when post-process creates a dir then fails AcoustID) get cleaned up after each successful run: walk up to the artist folder from any successful destination, prune empty album-sibling folders one level deep. Bounded scope = won't touch unrelated user dirs. Web_server.py shrinks by ~450 net lines. The endpoint handler is now a thin wrapper that builds injected callables (path resolver, post- process function, DB updater, empty-dir cleaner), spawns a thread that calls `reorganize_album()`, and returns. All actual logic lives in `core/library_reorganize.py` where it's unit-testable without spinning up Flask. Frontend cleanup: the per-call template input in both reorganize modals (per-album and bulk) was redundant — the backend always uses the configured global download template. Removed the input and the variables-grid reference UI it was for. 39 new unit tests pin every contract: - source resolution (no_source_id when album has none, fallthrough chain when primary returns nothing, strict mode bypasses fallback) - matcher scoring (exact / substring / multi-disc disambiguation / smart-quote tolerance / dash-vs-parens / bonus-track substring / Remix-vs-original differentiator rejection / "Real" doesn't false- match "Real Real Real" / track-number-only no longer fires) - file safety (DB-update failure leaves original in place, post- process failure leaves original in place, post-process exception caught and original preserved, success removes original AND updates DB in the right order) - sidecar handling (per-track .lrc/.nfo deleted on success, kept on failure; album-level cover.jpg/folder.jpg cleaned only when directory has no remaining audio) - staging cleanup (recreated between tracks because post-process nukes it, dir cleaned up on success AND on failure) - destination-dir prune (empty siblings removed, real album with files preserved, no recursive sweep) - source picker (only authed-with-stored-ID sources for per-album, all authed sources for bulk; strict mode doesn't fall back) - concurrency (3 workers in flight, state stays consistent under races, stop_check cuts off pending tasks) - preview parity (preview produces same destination as apply for multi-disc; ALBUM mode not SINGLE mode; unmatched/no-path tracks surfaced with reasons) Limitations (deliberate punts, NOT in this PR): - Renamed local titles on multi-disc albums where track_number also disagrees: matcher returns nothing (track is "not in source"). Fixable by using duration_ms as a tertiary signal. - Per-track in-modal source switching with per-album track-count hints (would need a second API call before opening the modal). - UI status panel on the artist page during a run — currently just toasts. Documented as a follow-up PR. Files: - core/library_reorganize.py — new module: plan_album_reorganize, preview_album_reorganize, reorganize_album, available_sources_for_album, authed_sources, _score_candidate, helpers for staging/post- processing/finalizing, sidecar + dest-dir cleanup - core/metadata_service.py — no changes; reused get_album_for_source, get_album_tracks_for_source, get_source_priority, get_client_for_source - web_server.py — three endpoints (preview / apply / sources GETs) are thin wrappers; -450 net lines - tests/test_library_reorganize_orchestrator.py — 39 tests covering every contract above - webui/static/library.js — source picker UI in both modals; dead template input + variables-grid removed - webui/static/style.css — dropdown option styling fix (white-on- white was unreadable) Reported on Discord by winecountrygames — his bug report named the trigger button (Enhanced view → Reorganize All) and both symptoms (multi-disc collapse, half-album skip), which let the diagnosis go straight to the architectural problem.	1 month ago
Broque Thomas	252121ca96	Bump Spotify post-ban cooldown from 5 min to 30 min ... Reported on Discord by winecountrygames — Spotify auth granted, then re-banned for 4 hours within ~30 seconds, repeatedly. Trace from his captured log: < 12:05 [pre-log] Spotify ban active when log starts 15:21:27 First ban EXPIRED → 5-minute post-ban cooldown begins 15:26:27 Cooldown ends, spotify_client.is_authenticated() probe allowed again → client initialized 15:26:59 First Spotify API call after cooldown — get_artist_albums for an artist whose discography a background worker was enriching — gets 429 immediately with no Retry-After header → new ban activated for 14400s (4 hours) Root cause: `_POST_BAN_COOLDOWN = 300` (5 minutes) is shorter than Spotify's actual server-side memory of the previous offense. The cooldown exists specifically to prevent the "ban expires → we probe → re-ban" cycle (`spotify_client.py:65-68` documents that intent explicitly), but the value was wrong: Spotify's server still considered this user banned 5 minutes after our local ban window ended, so the very first call after cooldown got slapped. The 4-hour re-ban itself is correct behavior — `_BASE_MAX_RETRIES_BAN` fires when spotipy reports "max retries", which means the client exhausted its internal retry budget on 429s before raising. That's a severe-ban signal and a long default is the right response. Fix: bump `_POST_BAN_COOLDOWN` to 1800 seconds (30 min). This is the smallest change that addresses the immediate "re-probe → re-ban" loop in the report. 30 minutes is an empirical floor — long enough for Spotify to actually clear its server-side memory in the cases we've observed, short enough not to keep functional users locked out beyond necessary. Can be revisited if reports persist. What this PR does NOT fix (important context for the same user): This bump only helps the "ban expires → we re-probe → re-ban" loop. It does NOT help winecountrygames's other symptom — Spotify being banned within 30 seconds of his FIRST EVER authorization (no prior ban). That's a separate failure mode: on first auth, enrichment workers immediately fan out across the user's library (250 artists in his case), hammering Spotify endpoints with bulk get_artist_albums calls before any rate-limit feedback can land. Spotify's hidden per-endpoint daily quotas — which BoulderBadgeDad has empirically documented but the global rate limiter doesn't see — flag the burst and impose a multi-hour cooldown that LOOKS like a bot-detection ban to us. A proper fix needs a fresh-auth ramp-up: start with very low Spotify QPS for the first N minutes, scale up only if no rate-limit feedback arrives. That's a separate PR. Documented as additional follow-ups (NOT in this change): - Adaptive cooldown that scales with the size of the previous ban — a 4-hour MAX_RETRIES ban probably warrants a 1-hour cooldown, while a 60-second Retry-After-honored ban can resume in 5 minutes. The system already distinguishes these in `_set_global_rate_limit`, it just doesn't propagate the distinction to cooldown duration. - Probe-with-light-call pattern — make the first post-cooldown call a single inexpensive endpoint (`current_user`) rather than allowing a background worker's heavy `get_artist_albums` to be the canary. Failed probe extends cooldown silently instead of triggering a fresh 4-hour ban. - Fresh-auth ramp-up (per the limitation above). Files: - core/spotify_client.py — `_POST_BAN_COOLDOWN` 300 → 1800. Comment expanded to cite the report so the value isn't bumped back without context. - webui/static/helper.js — WHATS_NEW entry under 2.40 explaining the change for affected users. No tests added — the cooldown logic itself is unchanged, only the constant. Tests asserting on a constant value are theater. Reported on Discord by winecountrygames — his captured log made the "ban-expires-to-re-ban" timing chain unambiguous.	1 month ago
Broque Thomas	b3afed1599	Fix Tidal device-auth link opening SoulSync instead of link.tidal.com ... The "Link Tidal Account" device-flow UI displayed a verification URL like `link.tidal.com/XBXYT` that, when clicked, navigated back to the SoulSync origin (e.g. `http://localhost:8889/link.tidal.com/XBXYT`) instead of to Tidal's activation page. Root cause: tidalapi returns `login.verification_uri_complete` as a schemeless string. settings.js drops it straight into `<a href>`, and browsers treat schemeless hrefs as same-origin relative URLs. Normalize the URI in `start_device_auth` — if it doesn't already start with `http://` or `https://`, prepend `https://`. Same treatment for the `link.tidal.com/{user_code}` fallback so the defensive path stays well-formed too.	1 month ago
Broque Thomas	a9f827ef42	Reject Tidal streams that silently downgrade from the requested quality ... Reported on Discord by Netti93: with Tidal configured for "HiRes only" and "Allow Quality Fallback" disabled, tracks were still downloading successfully — as m4a 320kbps files. Some "successful" downloads were less than half the file size of the same track pulled via Tidarr/tiddl from the same Tidal account. Root cause: Tidal's API silently degrades to the best quality your account + the track + your region permits. Setting `session.audio_quality = Quality.hi_res_lossless` and calling `track.get_stream()` on a track that's only available in AAC returns an AAC stream with no error. The downloader wrote the m4a file to disk, the ~7MB size sailed past the 100KB stub threshold, and the download reported success. The pre-existing "verify quality wasn't silently downgraded" block only LOGGED a warning when this happened; it did not fail the tier. Two knock-on effects: - Users with "HiRes only, no fallback" got m4a files anyway, which defeats the setting entirely. - The worker-level fallback chain (hires → lossless → high → low) couldn't advance past the first tier, because every tier "succeeded" at whatever Tidal happened to serve. Fix: after `track.get_stream()`, compare `stream.audio_quality` against the tier we asked for using a rank-based ordering: LOW < HIGH < LOSSLESS < HI_RES < HI_RES_LOSSLESS - Same tier or higher → accept (so the occasional Tidal upgrade doesn't get rejected just because it's not an exact match). - Lower tier → reject THIS tier. The loop `continue`s and the next fallback tier is tried, or the whole download fails honestly when the user has fallback disabled. The existing final-error log already has a hint directing users to enable fallback if they want automatic Lossless substitution. - Unrecognized `audioQuality` value (e.g. a new Tidal tier we haven't mapped) → reject conservatively, so the next fallback tier gets a chance and the diagnostic log names the unknown value. Why the rank-based approach instead of strict equality: Tidal's API doesn't technically promise an exact-tier match on serving; on tracks that are flagged in its catalog as a higher tier, it can serve higher than the session setting. Rejecting higher-than-asked quality would be user-hostile. And the `HI_RES` (legacy MQA) value — not in tidalapi's modern `Quality` enum but possibly still present on old catalog entries — needs to rank below `HI_RES_LOSSLESS`: users asking for true lossless HiRes should reject MQA since MQA is a lossy format. tidalapi's `Quality` enum is a `str` subclass whose VALUES (not member names) match what the Tidal API returns in the `audioQuality` field (e.g. `Quality.hi_res_lossless.value == 'HI_RES_LOSSLESS'`, `Quality.low_320k.value == 'HIGH'`). Both sides of the comparison are coerced to `str` before use, so the check is robust to whichever tidalapi version exposes the served quality as an enum or a plain string. The check is extracted as `_verify_stream_tier(stream, q_info, q_key) -> (ok, reason)` at module scope — a pure function with no I/O, unit-tested independently. Ten tests: match, three upgrade cases (LOSSLESS → HI_RES_LOSSLESS, LOSSLESS → HI_RES, LOW → any higher), three downgrade cases (the reported HiRes → AAC, HiRes Lossless → MQA HiRes, Lossless → AAC), one unrecognized-tier case, and two defensive paths for older tidalapi builds without `audio_quality` on the stream object and for QUALITY_MAP entries that lack `tidal_quality` (e.g. tidalapi wasn't importable at module load). Test stub updated to use uppercase `Quality` values matching real tidalapi so case-sensitivity regressions get caught. Also removed the old codec-string-based warning block — the new tier check is strictly stronger, and keeping the warning around would just be dead code waiting to drift out of sync. Deliberately NOT tackling in this PR (documented as follow-ups): - Bit-depth verification of HiRes FLAC files via mutagen. The `stream.audio_quality` tier check catches the main "HiRes requested, got AAC" case; bit-depth would only matter if Tidal labeled a stream HI_RES_LOSSLESS but served a 16-bit FLAC (`Stream.bit_depth` isn't reliable for this — tidalapi defaults missing `bitDepth` fields to 16, so a trust-the-stream check would spuriously reject valid HiRes whenever Tidal omits the field). A proper fix runs mutagen post-download to inspect the actual file, then decides whether to delete + retry the next tier — a whole new failure mode with design trade-offs that deserve their own PR. The support logs don't show this happening. - The "manual remap still says Not Found" symptom. Might be downstream of this same bug (silent-AAC "success" hitting a later rejection), might be a separate task-state issue. Not guessing without logs from the retry path. - Quality-aware stub threshold. 100KB is a reasonable floor for real stub/preview detection and there's no evidence the universal threshold is misfiring in the wild. Field-verified status: desk-verified via unit tests and empirical checks against a live tidalapi import (confirming the `Quality` enum's str-subclass behavior). Not yet smoke-tested end-to-end against a real Tidal account with a HiRes-only-no-fallback setting — Netti93 or anyone else with that config should notice either the fix working (non-HiRes tracks fail honestly with a clear log line) or any regression before wider release. Files: - core/tidal_download_client.py — new `_verify_stream_tier` helper and `_QUALITY_RANK` table at module scope, called in the download loop after the stream is fetched and before any bandwidth is spent. Removed the old inline codec-based warning since the new check supersedes it. - tests/test_tidal_stream_tier_verification.py — ten tests covering match / upgrade / downgrade / unknown / defensive paths. - tests/test_tidal_search_shortening.py — fake `Quality` values brought in line with tidalapi's real values so both files share a consistent stub regardless of pytest collection order. - webui/static/helper.js — WHATS_NEW entry under 2.40 describing the rank-based tier comparison. Reported on Discord by Netti93 — the "same account works via Tidarr" comparison narrowed the cause to SoulSync's download path rather than an account/region issue.	1 month ago
Broque Thomas	a60546929e	Fix Album Completeness job reporting zero findings for every album ... Reported by sassmastawillis: the Album Completeness maintenance job scans 3127 albums in 0.1 seconds and reports 0 findings — for every user, regardless of whether their library is actually complete. Restoring an older DB surfaced 7 correct findings, so the code logic works; the DB state is what's making everything look complete. Root cause: `albums.track_count` is only ever written by server-sync paths — Plex's `leafCount`/`childCount` and SoulSync standalone's `len(tracks)`. It's the OBSERVED count of tracks SoulSync has indexed, which is always exactly what `COUNT(tracks)` returns for that album. The completeness job treated it as the EXPECTED total and compared it against the observed count. They're equal by construction, so `actual >= expected` is always true: skip, 0.1s scan, 0 findings. Fix: new `api_track_count INTEGER` column on `albums`, written only by metadata-source code paths. Populated in two places so the scan is fast and the fallback is robust. 1. Enrichment workers — shared helper `set_album_api_track_count` in `core/worker_utils.py`. Called by each worker's existing `_update_album` method alongside its other album-column UPDATEs: - spotify_worker: `album_obj.total_tracks` from the Spotify Album dataclass (already in hand, zero new API calls) - itunes_worker: same, from the iTunes Album dataclass - deezer_worker: `nb_tracks` from full_data, falling back to search_data when the full lookup didn't run - discogs_worker: count of tracklist rows where `type_=='track'` (Discogs tracklists interleave heading and index rows that shouldn't count as songs) Helper skips the write on zero/None/negative/non-numeric inputs so a source lacking track info can't clobber a good value a different source already wrote. Caller owns the transaction — helper just queues an UPDATE on the caller's cursor without committing, so it batches cleanly with each worker's existing multi-UPDATE pattern. Hydrabase worker deliberately not touched — it's a P2P mirror that doesn't write album metadata to the local DB. Hydrabase- primary users hit the fallback path below. 2. Album Completeness repair job — new `al.api_track_count` column in the SELECT, read first in the scan loop. On miss (album never enriched, or enrichment workers haven't run yet on a fresh install), falls through to the existing `_get_expected_total()` API lookup and persists the result via the same shared helper (wrapped in connection/commit management since the repair job runs outside a worker's batched transaction). Also removed `al.track_count` from the scan's SELECT — now unused since the observed count was the whole source of this bug, and leaving a dead SELECT would invite a future engineer to re-introduce the same comparison. Help text on the job card was reworded so it honestly describes current behavior ("counts cached during normal enrichment are used when available; otherwise the job queries a metadata source directly") rather than the old "active provider first, then others as fallback" phrasing, which doesn't match how the cache actually fills — any enrichment worker that runs can populate it, and the last writer wins. Document-only follow-up if this edge case ever bites in practice: add a `api_track_count_source` column so the scan can prefer the configured primary source's count over others (e.g. deluxe vs. standard edition mismatches). Not worth the complexity today. For existing users, the first completeness scan after upgrade is fast to the extent their library is already enriched: the workers already ran and populated `api_track_count` on their normal schedule. For brand-new installs, the scan's fallback path handles the cold start — slower, but correct, and subsequent scans are fast. Does NOT affect: - Download / post-processing / wishlist / sync code paths — none of them read `track_count` for completeness semantics. - Plex / Jellyfin / Navidrome / standalone sync — still write `track_count` exactly as before; `api_track_count` is a separate column they never touch. - Other repair jobs. - Any UI path — same finding schema, just correct counts now. Files: - database/music_database.py — idempotent migration adding `api_track_count INTEGER DEFAULT NULL` to the existing album-column check block. - core/worker_utils.py — new `set_album_api_track_count` helper with the documented skip-on-bad-input contract. - core/spotify_worker.py, itunes_worker.py, deezer_worker.py, discogs_worker.py — one-liner call from each `_update_album`. - core/repair_jobs/album_completeness.py — scan uses the cache; fallback path persists API-lookup results via the shared helper; help text updated to match actual behavior. - tests/test_worker_utils_album_track_count.py — 9 tests covering the helper's write/skip contract + no-commit invariant. - tests/test_album_completeness_job.py — 2 tests for the repair job's fallback-path wrapper. - webui/static/helper.js — WHATS_NEW entry. Credit: sassmastawillis spotted the bug; the "restored older DB finds 7 albums" signal pinpointed DB state over code logic and made the diagnosis tractable.	1 month ago
Broque Thomas	c454b1ebaf	MusicBrainz: Dedupe same-named homonyms in artist search results ... Typing "michael jackson" returned 7 identical-looking cards because MusicBrainz has many different PEOPLE sharing a canonical name — the King of Pop plus a NZ poet, a photographer, a mashup artist, a didgeridoo player, and more, all scoring 80+ on exact-name match. All 7 passed the score filter. All 7 rendered with the same fallback image because iTunes/Deezer only know the famous one. Fix dedupes by normalized (lowercase, whitespace-trimmed) name before building Artist dataclasses. Keeps the highest-scoring entry per name, so the King of Pop (score 100) wins over the others (all score 80-81). Artists with genuinely different names stay separate — a search for "the beatles" still surfaces tribute bands if they're above threshold. Implementation note: fetch `max(limit*3, 10)` from MB instead of `limit` directly, so the dedup pool is large enough to still return `limit` distinct artists after collapsing duplicates. Previously the raw fetch was capped at the caller's limit, which would have left fewer-than-requested results after dedup for common names. 3 new tests (49 total): - Dedupe collapses 5 same-named entries to 1 (keeps highest score). - Dedup key is case-insensitive and whitespace-normalized. - Dedup preserves distinct names ("The Beatles" vs "The Beatles Revival" stay separate). Live-verified: "michael jackson" now returns 1 card, "kendrick lamar" returns 1 card. Credit: kettui spotted duplicate Michael Jackson cards in the search UI.	1 month ago