Discord (Shdjfgatdif): "if a track isn't imported it should remain there, not be deleted, so we
can retry." He was seeing failed downloads disappear and having to re-download.
Normally a rejected file is QUARANTINED (moved to ss_quarantine, preserved + retryable), not
deleted. But all four quarantine blocks (integrity / silence / quality / acoustid) had the same
fallback: if move_to_quarantine itself raised, os.remove(file_path). On a NAS that move can fail
(cross-device / permissions), so the except fired and the user's download was DELETED — the worst
outcome, and exactly the re-download pain he reported.
fix: on quarantine failure, log and LEAVE the file in place — never delete. The task is still
marked failed and the batch still notified (that code runs after the try/except and never touched
the deleted file), so the only behaviour change is "preserved instead of destroyed". Reviewed
every os.remove in the pipeline: the remaining ones are success-path cleanups (replacing an
existing destination, or removing a redundant download when the track is already in the library at
equal/better quality) — left untouched.
regression test drives the REAL pipeline through integrity-rejection with quarantine forced to
raise, and asserts the source file is preserved while the task is still failed + notified.
1311 imports/downloads/quality tests green, ruff clean.
Race condition: cancelling the retry task didn't stop an already-in-flight
download from completing and creating a new quarantine entry via the pipeline.
- Approve endpoint now sets _quarantine_approved_alternative=True on the
cancelled task alongside status='cancelled'
- Pipeline completion handler checks this flag when _acoustid_quarantined
is set: immediately deletes the freshly-created quarantine entry and
returns, so no stale entry accumulates from the race
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- pipeline: use trigger='acoustid_unverified' (not 'acoustid') when
require_verified=ON rejects an unconfirmed track — quarantine badge now
shows "ACOUSTID UNVERIFIED" instead of "ACOUSTID MISMATCH"
- web_server: /api/verification/config now also returns require_verified
- pages-extra: collapse the Unverified sub-view to quarantine-only when
require_verified=true (same path as acoustid_enabled=false); new trigger
entry in _VERIF_QUAR_TRIGGERS for acoustid_unverified
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
New setting acoustid.require_verified (default off), shown under
Settings → Quality Profile only when AcoustID is enabled.
When on, an AcoustID SKIP (ran but couldn't confirm — no fingerprint match
or cross-script metadata, the ⚠ "unverified" case) is treated like a FAIL:
the file is quarantined and the next-best candidate is tried, instead of
importing an unverified file. Only a clean AcoustID PASS is kept.
Transient ERROR results (rate-limit / outage) are deliberately NOT blocked —
that would stall the whole pipeline during an AcoustID outage. Those still
import with their existing flag.
- pipeline.py: SKIP routes through the existing FAIL quarantine + retry path
(trigger 'acoustid') when require_verified is on.
- UI: checkbox under Quality Profile, visibility tied to acoustid-enabled via
syncAcoustidRequireVerifiedVisibility(); load/save wired in settings.js.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The audio-completeness guard (detect_broken_audio) is the only post-processing
step that fully DECODES the file with ffmpeg, making it the most CPU-heavy step.
Two changes reduce and gate that cost:
1. Single ffmpeg pass: astats (truncation) + silencedetect (silence) now run in
one chained -af filter over a single decode, instead of two full decodes.
~50% less CPU, no detection lost. Pure parsers unchanged.
2. Opt-in toggle: new post_processing.audio_completeness_check (default False).
The decode now only runs when the user enables it under
Settings → Post-processing → Core Features. Most preview/truncation cases are
already caught at the source (HiFi/Qobuz have their own guards), so the
expensive whole-file decode stays off unless explicitly turned on.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Confirmed from Sokhi's FLAC tags + screenshot: disc-2/3 tracks land in the 'Disc 1'
folder, collapsing every disc's track 3/4/5/6 into one folder. Root cause: the
import pipeline syncs the resolved TRACK number into album_info (so the folder
matches the tag — pipeline.py '[FIX] Updated album_info track_number') but never
did the same for DISC. So the 'Disc N' folder (built from album_info.disc_number,
often 1) used a different disc than the embedded tag (resolved per-track in
source.py — e.g. 2/3 from a MusicBrainz multi-medium release).
Fix: one SHARED resolver, resolve_disc_for_track(original_search, album_info),
used by BOTH source.py (the tag) and the pipeline (which now writes it back into
album_info before building the path). Same function + same inputs (the pipeline
pulls the identical get_import_original_search(context)), so folder and tag can
never disagree. Returns the first valid positive disc (per-track, then album),
else 1 — a falsy/unknown per-track disc falls through to the album instead of
flooring early.
Tests: resolver preference/fallback/floor + an explicit folder==tag lockstep check
incl. Sokhi's per-track-2/album-1 case. 2122 import/pipeline/metadata tests green.
Single tracks (esp. Deezer-sourced) imported as "01 - Title" regardless
of their real album position — e.g. Fly Away (track 2 of Greatest Hits)
landed as 01, littering album folders with duplicate "01" files.
Root cause: a Deezer single track is matched via /search/track, which
omits track_position, so the context never carried the real number; then
service.py + context.py fabricated a confident track_number=1 from that
gap. Because the resolver puts that first, the fake 1 beat the source.
It is source-agnostic (slskd-with-Deezer-metadata hits it too) — albums
work because /album/<id>/tracks DOES include positions.
Fix (at the shared import funnel, strictly additive):
- track_number.py: new read_embedded_track_number() (mutagen, local, no
network) + an optional embedded_track_number arg on resolve_track_number.
The downloaded file already carries the source-written position (deemix
wrote it); consult it LAST — only when metadata AND the "NN - Title"
filename both come up empty — so it can only fill the gap that would
otherwise hit the default-1 floor. Never overrides a value the pre-fix
resolver produced (no regression for correctly-named/mistagged files).
- pipeline.py: read the file tag at the resolve step and pass it in.
- De-poison: service.py:217 + context.py default to 0 (the existing
"unknown" sentinel, like total_tracks), NOT 1 — so the fake 1 no longer
blocks recovery. Frontend already treats falsy track_number as unknown
(omits it), so this also drops the bogus "1." in the UI.
13 new resolver tests incl. the no-regression precedence guards; full
imports + wishlist suites green (583), no behavior change for albums.
1) _resolve_library_file_path now tries the FULL relative path (index 0)
against each base dir, not just suffixes. The library scanner stores clean
"Artist/Album/Track.flac" paths; skipping index 0 dropped the artist folder
so the file never resolved — every quality-scanner probe failed ("20/20
could not be probed"). Now they resolve under the transfer/library dir.
2) Quality gate moved BEFORE the AcoustID check in post_process_matched_download.
- A wrong-quality file is rejected without paying for an AcoustID fingerprint.
- context['_audio_quality'] is set before either gate quarantines, so the
real quality is recorded on the sidecar for EVERY quarantine trigger —
it's known when reviewing/approving any quarantined file.
- force_import still never fires on a quality mismatch (only AcoustID).
normalize_import_context mutates in place, so the moved block keeps its
context fields. New test pins the order + that AcoustID isn't run on a
quality reject.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A file quarantined for QUALITY (e.g. MP3-VBR rejected by a FLAC-only profile)
showed up in BOTH the Completed and Quarantine tabs. Cause: the verification
wrapper (post_process_matched_download_with_verification) handled the
_acoustid_quarantined / _integrity_failure_msg / _race_guard_failed markers but
NOT the quality marker _bitdepth_rejected (nor _silence_rejected). A quality
quarantine leaves no _final_processed_path, so the wrapper hit the
"no final path — assuming success" branch and marked the task Completed.
Unlike acoustid/integrity (retry driven by the wrapper), the inner pipeline
already owns the quality/audio-guard outcome — it quarantines then re-queues the
next-best candidate or marks the task failed. So the wrapper now just returns
when it sees _bitdepth_rejected/_silence_rejected, without marking completed
(which clobbered both the quarantine state AND any successful retry).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Unverified review actions (play/audit/approve/delete) only rendered for
persistent-history rows, so a freshly-completed unverified download — still a
live task without a 'history-<id>' task_id — showed no buttons until it aged
into history (Quarantine always worked because it uses the quarantine entry
id). Thread the library_history row id from import through to the live task
(add_library_history_entry now returns lastrowid -> context._history_id ->
task.history_id -> /api/downloads/all), and resolve verifHistoryId from it.
Also surface the real probed audio quality (mutagen-read from the file, e.g.
'FLAC 24bit') on completed rows as a chip, so you can see what was actually
downloaded.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The actual HiFi/Monochrome bug isn't silence padding — it's a TRUNCATED file:
the container claims the full length (e.g. 3:08) but only ~30s of audio
decodes. silencedetect finds nothing (there's no silent audio, just missing
audio) and ffmpeg's time= even reports 0 with no error, so the duration and
quality guards all pass.
Detect it by decoding and comparing the real audio length (astats sample
count / sample rate) against the container duration: reject when the real
audio covers < 85% of the claimed length. detect_broken_audio() runs this
truncation check first, then the silence-ratio check. Wire it into the guard
that runs at the integrity/length verification point.
Verified on the real file: 'only ~30s actually decodes of a 188s file (16%)';
a normal 180s file is not flagged.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The silence guard sat after the quality guard, so a strict quality profile
quarantined every file before silence detection ever ran. Move it to right
after check_audio_integrity (where the length is verified) and before the
AcoustID/quality gates, so a mostly-silent file is caught regardless of its
quality verdict and reported with the correct reason. Same quarantine +
next-candidate retry pattern (trigger='silence').
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
HiFi/Monochrome HLS assembly can produce a file with the correct container
duration but only ~30s of real audio + silence padding — the duration and
quality guards both pass, so nothing caught it until you listened. Add
core/imports/silence.py: ffmpeg silencedetect over the audio, reject when the
silent fraction exceeds 50%. Wire it into the post-download pipeline with the
same quarantine + next-candidate retry pattern as the quality guard
(trigger='silence'), and surface it via import_rejection_reason. Fails open
when ffmpeg/mutagen are unavailable so tooling problems never quarantine a
legit file.
Also mark 'quality filter' and 'silence guard' failures as recoverable
quarantine rows in the downloads UI (were shown as plain failures).
Verified end-to-end: a 30s-tone + 180s-silence FLAC is flagged '86% silence
(only ~30s audible of 210s)'; a 210s tone passes. 7 parser unit tests.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replaces the Soulseek-only bit-depth heuristic with a source-agnostic
quality system that works across all download sources.
## core/quality/model.py (new)
- AudioQuality dataclass: format, bitrate, sample_rate, bit_depth
- QualityTarget: one ranked entry in the user's priority list
- filter_and_rank(): source-neutral candidate ranking
- rank_candidate(): scores any AudioQuality against ranked_targets
- v2_qualities_to_ranked_targets(): migration helper
## core/download_plugins/types.py
- SearchResult gains sample_rate + bit_depth fields
- audio_quality property returns unified AudioQuality
- AlbumResult gets audio_quality aggregated from tracks
## core/soulseek_client.py
- Parses slskd attributes array (type 4=sample_rate, type 5=bit_depth)
- Real values instead of kbps heuristic
- filter_results_by_quality_preference() replaced by filter_and_rank()
## database/music_database.py
- Quality profile v3 with ranked_targets list
- Auto-migration v2 → v3 on load
- Presets (audiophile/balanced/space_saver) updated to v3
## core/imports/file_ops.py
- probe_audio_quality(): reads actual downloaded file via mutagen
returns AudioQuality with ground-truth values
## core/imports/guards.py
- check_quality_target(): replaces check_flac_bit_depth
checks all formats/sources against ranked_targets
- check_flac_bit_depth() kept as backwards-compat wrapper
## core/imports/pipeline.py
- Uses check_quality_target() instead of check_flac_bit_depth()
- Quality mismatch triggers _requeue_quarantined_task_for_retry('quality')
so next-best candidate is tried before failing (same as AcoustID)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The download analysis already matches every track to a library row via
check_track_exists / manual match, then discarded the result. Keep it: each
analysis_results entry now carries matched_file_path + matched_track_id (the
owned file's real location, or None). Symmetrically, a completed download task
now records final_file_path (where the import landed).
Purely additive, no behavior change, no new matching, zero perf cost — just
stops throwing away what the pipeline already computed. This is the foundation
for playlist materialization: owned + downloaded tracks both report where their
real file is, so the folder can be built by name match, not source IDs.
The pipeline has three success exits (simple download, playlist folder mode,
main) but only the main one persisted the verification status — force-imported
playlist tracks got no tag, no history status, and never appeared in the
Unverified filter. Extracted _persist_verification_status() and call it at
every exit. One-time idempotent backfill derives status for existing history
rows from their recorded acoustid_result (pass->verified, skip->unverified).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CubeComming #804: importing Coldplay "Yellow" (the 269s Parachutes album track,
correctly tagged) was quarantined — "Duration mismatch: file is 269.2s, expected
266.0s (drift 3.2s > tolerance 3.0s)". The expected 266s came from a re-resolved
*single* edition, not the file's actual album. The duration-agreement integrity
check exists to catch truncated/wrong slskd TRANSFERS — but a manual import is
the user's own already-tagged file being sorted, so checking it against a
re-resolved release just manufactures false quarantines.
Fix: both manual-import paths (singles + album) now mark the context
is_local_import; the integrity check skips the duration-agreement leg for local
imports via expected_duration_for_check() (new pure helper). The size +
mutagen-parse legs still run, so genuinely broken files are still caught — only
the release-vs-file duration comparison is skipped, and only for manual imports.
slskd downloads are completely unaffected.
This does NOT change the deeper matching (file still groups under Singles vs the
Parachutes album — the #767 canonical-version family); it stops the false
quarantine so the file imports.
Tests: 4 on the helper (local skips, download keeps, zero/None/garbage, string
coercion) + updated the routes context assertion. 557 import/integrity tests pass.
CubeComming #804: since 2.6.7, importing already-tagged files (Bruno Mars,
Coldplay) blanked EVERY tag and filed them under "Unknown Artist". Root cause:
both metadata-enhancement blocks in post_process_matched_download did
`except Exception: wipe_source_tags(file_path)` — a full audio.tags.clear() +
strip + clear_pictures. But enhancement throwing means NO new tags were
written, so wiping just destroys the originals. A transient enhancement error
on a well-tagged file = total metadata loss. (The reported "bitrate change" is
a red herring: mutagen padding on re-save, not a re-encode — ReplayGain only
reads via ffmpeg and tags via mutagen.)
Fix: gate the failure-path wipe on should_wipe_tags_on_enhancement_failure()
(new pure, tested policy) — only wipe UNMATCHED downloads (likely junk source
tags); NEVER wipe a clean/matched import, preserve its existing tags + log.
Unmatched-download behavior is unchanged, so the only thing that changes is the
broken case.
Tests: 3 pin the policy (clean→preserve, unmatched→strip, falsey→strip).
1211 import/pipeline/metadata tests pass.
Measured during a live album download: ~4m15s per track in post-processing
(normal is ~20s), with the time vanishing silently inside embed_source_ids —
up to 5 MusicBrainz calls per track crawling against a degraded musicbrainz.org
while the MB enrichment worker kept eating the same ~1 req/s per-IP budget.
Only Spotify/Last.fm/Genius were in the yield set; MusicBrainz, Deezer, iTunes,
Discogs etc. kept grinding through downloads.
Policy (new core/enrichment/yield_policy, tested):
- downloads active -> ALL enrichment workers yield (post-processing touches
every metadata source). listening-stats (local-only) and repair
(user-scheduled) intentionally keep running.
- discovery active -> the API-contention five yield (spotify/itunes/deezer/
discogs/hydrabase) — discovery never paused anything before, despite the
pause helper literally defaulting to label='discovery'.
- user overrides and user-paused bookkeeping keep their existing semantics;
the dashboard yield_reason label now says WHICH foreground work caused it.
Observability (the 4-minute silence can never come back):
- every source lookup is timed; >2s logs a warning NAMING the source and
duration (core/metadata/source.py _call_source_lookup)
- the pipeline always logs "Metadata enhancement took X.Xs" per track
7 policy tests (incl. the motivating case: MB yields to downloads, keeps
running during discovery); 277 pipeline/enrichment tests pass.
When a file is quarantined (AcoustID / integrity / bit-depth), the source
is moved away and _mark_task_quarantined sets _quarantine_entry_id on the
context. A second post_process_matched_download call with the same
file_path (caused by a monitor re-poll or concurrent dispatch before the
context is cleaned up) then hit the race guard — "source file gone, no
known destination" — and overwrote the in-flight retry with a failed status.
Fix: check _quarantine_entry_id before firing the race guard. If it is
set, the file is legitimately in quarantine and this is a stale duplicate;
return silently so the quarantine retry already in flight can proceed.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Some tracks don't exist on the sources in the wanted cut — every copy is, say,
the instrumental. The retry engine correctly rejects each (version mismatch) and
gives up, leaving the track missing. New opt-in fallback: once a track's AcoustID
retries are fully exhausted, if every quarantined candidate for it failed the
SAME version mismatch (same matched version, e.g. all instrumental) and there are
>= N of them, accept the best (first-tried = oldest = highest-confidence) one.
Safety rules (core/imports/version_mismatch_fallback.py):
- Version mismatches only. Audio/artist mismatches (different recording) and
integrity/duration failures (truncated/wrong file) never participate.
- All qualifying entries must share the same matched version; a mix
(instrumental + live) is ambiguous → no acceptance.
- Re-import bypasses ONLY the AcoustID gate; integrity/duration/bit-depth still
run, so a truncated or genuinely wrong file is never let through here.
- Reuses the existing quarantine approve_quarantine_entry + re-verify dispatch.
Wired at the AcoustID give-up point in the verification wrapper. Two new
post_processing settings surfaced in the Retry Logic tile (default off):
accept_version_mismatch_fallback + version_mismatch_min_count.
Pure decision core + orchestration covered by tests (11). Acceptance logged at
WARNING with track + matched version.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
When a downloaded file is quarantined because AcoustID verification or the
integrity/duration check fails, the task no longer dead-ends as failed — it
re-runs the worker on the next-best candidate, skipping the quarantined source.
Reuses the monitor's existing transfer-error retry machinery (used_sources +
cached_candidates + worker re-dispatch), just triggered from the post-process
verification wrapper's two quarantine branches instead of only on transfer
errors. Universal across sources (Soulseek, HiFi, Tidal, etc.) since all
batch/sync downloads funnel through post_process_matched_download_with_verification.
- monitor.requeue_quarantined_task_for_retry(): marks bad source used, resets
task to searching, resubmits worker. Guards: manual picks, cancelled tasks,
missing source id, and a MAX_QUARANTINE_RETRIES=5 loop cap.
- Opt-out via post_processing.retry_next_candidate_on_mismatch (default on).
- Manual quarantine approve is unaffected (_skip_quarantine_check='all' bypasses
the checks, so no quarantine flag, so no retry).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The old per-download Retag Tool was limited (only native-pipeline downloads,
100-group cap, manual per-group) and did the wrong thing — it moved/reorganized
files instead of just tagging. It's superseded by the new Library Re-tag job
(whole-library, in-place) + the enhanced-library 'Write Tags' button.
Removed: the post-download record_retag_download ingestion hook (stops writing
retag_groups on every download), core/library/retag.py, the web_server state +
deps + /api/retag/* endpoints + the tool:retag WebSocket emit, the dashboard
card + both modals (index.html), the core.js socket handler, and the tools-page
wiring + help entry (wishlist-tools.js). Updated the import-pipeline test.
Verified: web_server parses, app + core imports OK, 392 tests pass, no live
references to removed symbols.
Left as inert (harmless) for a careful follow-up sweep: the retag_groups/
retag_tracks tables + their DB CRUD methods (no longer written/read), and the
now-orphaned retag JS helper functions (no entry point/wiring/socket calls them;
interspersed with wishlist functions, so not blind-deleted).
Persist organize_by_playlist on mirrored playlists and run playlist-folder
downloads from the auto-sync pipeline instead of the global wishlist phase.
Register SoulSync library rows after playlist-folder post-processing, route
failed organize batches to the wishlist correctly, and skip sync-time
unmatched wishlist only when organize download handles retries.
Invalidate stale playlist track caches on refresh (Spotify and Deezer ARL),
re-mirror on refetch, and improve standalone playlist modals (re-analysis,
Open in Mirrored). Add filesystem missing-track detection and tests.
Co-authored-by: Cursor <cursoragent@cursor.com>
The manual-import routes (album + singles) call post_process_matched_download
directly. When the pipeline quarantines a file — integrity / AcoustID / FLAC
bit-depth — or hits the race guard, it sets a context flag and RETURNS
NORMALLY (it only marks the task failed + notifies when there's a task_id,
which manual imports don't have). So the inner pipeline raised no exception,
and routes.py counted `processed += 1` for a file that had just been moved to
ss_quarantine, not the library. Result: the UI shows a green "Done" while the
track silently vanished — exactly the #764 report (Coldplay - Yellow.flac ->
ss_quarantine, but "Done").
The download path already handles this in
post_process_matched_download_with_verification (it reads the same flags and
marks the task failed); only the manual-import routes were missing the check.
Fix: new pure helper import_rejection_reason(context) returns a human-readable
reason for any terminal rejection (_integrity_failure_msg / _acoustid_quarantined
/ _bitdepth_rejected / _race_guard_failed) or None for a clean import. Both
manual-import routes now consult it: album_process reports the track in
`errors` instead of counting it processed; process_single_import_file returns
("error", reason) instead of ("ok", ...). Verified every move_to_quarantine
call site (4, all in pipeline.py) sets one of those flags, so no quarantine
path slips through. This also delivers the "direct display of the error" the
reporter asked for — the reason now surfaces in the response `errors` list.
Does NOT address the reverse symptom ("failed even though it moved correctly")
— not yet root-caused — nor the separate bit-depth hole on the download-path
wrapper.
Tests: tests/imports/test_import_rejection_reason.py (10) — each trigger
detected, falsy flags ignored, deterministic ordering, plus two route-level
tests driving the REAL process_single_import_file (quarantine -> "error";
clean -> "ok").
post_process_matched_download_with_verification pops task_id/batch_id out of
the context before running the inner pipeline (so the inner doesn't fire its
own task notifications). But _mark_task_quarantined runs inside that inner call
and reads context['task_id'] — which is now None — so it silently no-op'd.
Result: every download through this wrapper (album-bundle / staging path)
quarantined WITHOUT recording quarantine_entry_id on the task, so the UI had no
handle to manage the file (the status click just fell back to the search modal).
_mark_task_quarantined now also stashes the entry id on the context (survives
the pop), and the wrapper applies it to the real task in both quarantine
branches (integrity + AcoustID). Direct (non-wrapper) callers are unchanged.
Tests: unit coverage for the stash-with/without-task_id behavior, plus a
wrapper-level test proving the entry id reaches the task on integrity quarantine.
Real-world regression triggered by the album-bundle work earlier in
2.6.3. Tracks with full Spotify metadata were importing as
``01 - <title>`` under ``Artist - Album/`` (no year), even when the
source filename carried the correct track number and Spotify's
release_date was available.
Investigation via DB inspection of stored wishlist rows:
```
"Never Gonna Give You Up" → track_number=None, release_date=""
"idfc" → track_number=1, release_date=""
"No Sleep Till Brooklyn" → track_number=1, release_date=""
```
Source-of-truth Spotify metadata had release_date AND real track
positions, but the wishlist row was poisoned. Three regressions
compounded the loss:
**Fix A — ``track_object_to_dict`` (``core/wishlist/payloads.py:295``)
preserved only album.name during Track→dict conversion.**
Pre-fix:
```python
album_name = "Unknown Album"
if hasattr(track_object, "album") and track_object.album:
if hasattr(track_object.album, "name"):
album_name = track_object.album.name
else:
album_name = str(track_object.album)
result = {
...
"album": {"name": album_name}, # ← release_date / images / etc. all dropped
...
}
```
When a wishlist payload arrived as a Track dataclass instead of a
raw spotify_data dict, the Track→dict conversion stripped
release_date, images, album_type, total_tracks, id, and album-level
artists. Every wishlist row added through this path landed in the
DB with ``album={'name': X}`` only.
Post-fix: three branches handle the three album shapes
- ``album_attr`` is a dict → ``dict(album_attr)`` preserves every key
- ``album_attr`` is a sub-object → pull all common Album-dataclass
attrs (id, release_date, album_type, total_tracks, images, ...)
- ``album_attr`` is a bare string → build a dict from the track
object's adjacent attrs (release_date, album_id, album_type, ...)
and surface ``image_url`` as ``album.images``
**Fix B — ``core/discovery/playlist.py:309`` only added
``track_number`` / ``disc_number`` keys when truthy.**
Pre-fix:
```python
matched_data = { 'id': ..., 'name': ..., ... } # no track_number / disc_number
if track_number:
matched_data['track_number'] = track_number
if disc_number:
matched_data['disc_number'] = disc_number
```
Deezer-sourced matches always hit this branch with ``track_number=None``
because the cache enrichment at line 304 reads ``_raw.get('track_number')``
literally, but Deezer's raw shape uses ``track_position``. So the key
was omitted from ``matched_data``, downstream consumers couldn't
distinguish "missing key" from "value is 1", and the chain silently
filled 1.
Post-fix: keys are ALWAYS present (None when unknown). Also adds a
``best_match.track_number`` fallback so the Track-dataclass-mapped
value (which DOES include ``track_position``→``track_number``
mapping) gets used when the cache lookup misses.
**Fix C — Pipeline only consulted ``album_info.track_number`` before
falling to the filename (``core/imports/pipeline.py:645``).**
VA-collection source files like ``417 Fountains of Wayne - Stacys
Mom.flac`` have a leading playlist-position number that isn't the
album track number. The previous chain (album_info → filename →
floor-1) couldn't recover the real position because the filename
extractor either returned 417 (wrong) or None (caught by the floor).
But the wishlist payload's ``track_info.spotify_data.track_number``
HAD the right answer all along — Spotify says Stacy's Mom is track
3 on Welcome Interstate Managers.
Post-fix: resolution chain extracted into ``core/imports/track_number.py:resolve_track_number``
as a pure function:
1. ``album_info.track_number`` (album-bundle dispatch authoritative)
2. ``track_info.track_number`` (per-track flow payload)
3. ``track_info.spotify_data.track_number`` (nested fallback)
4. ``extract_explicit_track_number(file_path)`` (filename, returns
0 when no numeric prefix — vs the default helper that returns 1)
5. Caller (pipeline) applies the final >=1 floor
Each step coerces to a positive int or falls through to the next.
Pure function = unit-testable in isolation = single place to fix
the rule.
**Test coverage (37 new tests):**
- ``tests/wishlist/test_payloads.py`` (+4) — Track→dict conversion
preserves full album dict (dict / object / string album shapes) +
None-track-number stays None.
- ``tests/discovery/test_discovery_playlist.py`` (+2) — matched_data
always includes track_number/disc_number keys (None when unknown)
+ falls back to best_match attrs when cache misses.
- ``tests/imports/test_track_number_resolver.py`` (+16) — every
resolution-chain branch pinned: album_info-wins, track_info
fallback, spotify_data nested, JSON-string parsing, garbage-string
fall-through, zero / negative / non-numeric / string-numeric
coercion, filename fallback, explicit extractor vs default
extractor semantics, defensive None inputs, VA-collection
filename behaviour, all-sources-missing → None.
1571 wider-suite tests pass (wishlist + imports + discovery +
downloads + metadata). Ruff clean.
**Migration note:** existing wishlist rows that were saved under
the OLD ``track_object_to_dict`` (with stripped album metadata) still
have ``release_date=''`` in the DB blob. Those won't self-heal — the
next attempt loads from the poisoned blob. Users can remove + re-add
those tracks to refresh, or wait for the next sync run that
re-discovers them with full metadata. No automatic migration shipped
in this PR (scope creep — the forward path is fixed, backfill is a
separate concern).
Two confirmed-from-code-reading bugs in the wishlist retry chain.
Both cause downstream post-process to render every retried file as
``01 - <title>`` without year in the folder path, even when the
source slskd file had the correct track number embedded and Spotify
had the album release date.
**Bug A — track_number defaults to 1 at every link in the chain.**
Pre-fix: ``.get('track_number', 1)`` defaulted at four sites:
- ``core/wishlist/payloads.py:121`` ``ensure_wishlist_track_format``
- ``core/wishlist/payloads.py:282`` Track-object conversion
- ``core/imports/context.py:421`` legacy album-info builder
- ``core/imports/pipeline.py:645`` final processing read
Each step "filled in" 1 when the upstream had dropped the key. The
downstream filename-extract fallback at ``pipeline.py:652`` ONLY
runs when the value is None — pre-filled 1 never matched, so the
fallback never fired, so the source filename's track number (e.g.
``08. No Sleep Till Brooklyn.flac``) was discarded in favour of the
default-1.
Fix: change every default from ``1`` to ``None`` along the chain.
The pipeline already has the right detect-and-recover logic — it
just needs the chain to stop poisoning it. Final ``< 1`` floor at
``pipeline.py:660`` still defaults to 1 as last resort, so callers
that genuinely have nothing still produce a valid number.
**Bug B — release_date dropped from cancelled-task wishlist payload.**
Pre-fix: ``build_cancelled_task_wishlist_payload`` only ``setdefault``ed
``name`` / ``album_type`` / ``images`` on the album dict. The
release_date field copy was load-bearing (when input was a dict, the
``dict(album_raw)`` copy preserved it), but when input was a bare
string the constructed dict had only name + album_type — no
release_date / total_tracks / etc.
Fix:
- Explicit comment on the dict-shape branch that release_date survives
via the unconditional ``dict(album_raw)`` copy + setdefault
semantics — so a future refactor that switches to a stricter copy
doesn't silently strip the field.
- String-shape branch now pulls release_date from
``track_info.album_release_date`` or ``track_info.release_date``
when present so the round-trip preserves the year for the path
template.
- track_data shape itself now carries ``track_number`` / ``disc_number``
at the top level (Bug A intersect — was dropping it entirely).
**Tests:** 4 new in tests/wishlist/test_payloads.py:
- ``test_ensure_wishlist_track_format_preserves_real_track_number``
- ``test_ensure_wishlist_track_format_keeps_missing_track_number_as_none``
- ``test_build_cancelled_task_wishlist_payload_preserves_track_number``
- ``test_build_cancelled_task_wishlist_payload_string_album_pulls_release_date_from_track_info``
14 payload tests pass; 879 across wishlist + imports + downloads
suites still green; 1410 wider suite all pass. Ruff clean.
Commits 2 + 3 of 3 in PR 2/4 of the wishlist-album-bundle issue fix
series. Commit 1 (94ba1d73) instrumented staging-match so the next
wishlist run produces the evidence we need to diagnose bug C
(staging-match silently drops album-bundle wishlist tracks); that
fix lands in a follow-up PR after the user's next reproduction run.
Closes#584. Quarantined files used to sit in ss_quarantine/ with a
thin sidecar — no UI, no recovery, no way to see what got dropped.
This adds the management surface the user needs without going to the
filesystem.
UI: new "Quarantine" button on the downloads page header opens a
modal with every quarantined file (filename, expected track/artist,
reason, when, size). Three actions per row:
- Approve (one-click): restores the file, re-runs the post-process
pipeline with ONLY the failing check skipped, lands in the library
with full tags + lyrics + scan
- Recover (legacy fallback): moves to Staging for thin-sidecar
entries that lack the embedded context Approve needs
- Delete: permanent removal of file + sidecar
Per-check bypass: context['_skip_quarantine_check'] = 'integrity' /
'acoustid' / 'bit_depth'. Skips ONLY the named check — other quality
gates stay live. No blanket bypass-all flag.
Sidecar expansion: move_to_quarantine now persists the full
json-serializable context via serialize_quarantine_context (drops
non-JSON-safe values, walks nested dicts/lists/sets, str-coerces
unknown objects) plus the trigger name. Existing thin sidecars are
detected and routed to Recover instead of Approve.
Pure helpers in core/imports/quarantine.py: list_quarantine_entries
/ delete_quarantine_entry / approve_quarantine_entry /
recover_to_staging / serialize_quarantine_context. 27 tests pin
every shape: orphan files / orphan sidecars / corrupt sidecars /
collision-safe filename restoration / full-context vs thin-sidecar
dispatch / json round-trip safety.
Four new endpoints in web_server.py — thin glue around the helpers:
GET /api/quarantine/list, DELETE /api/quarantine/<id>,
POST /api/quarantine/<id>/approve, POST /api/quarantine/<id>/recover.
Download modal status differentiates "🛡️ Quarantined" from
"❌ Failed" so recoverable files are visible at a glance — checked
against the error_message text, no schema change needed.
Pipeline changes are three minimal per-check conditionals at the
existing quarantine sites in core/imports/pipeline.py. Each
move_to_quarantine call now passes its trigger name so the sidecar
records which check fired.
Full suite: 2992 passed.
Previously hardcoded at 3s (5s for tracks >10min) — files drifting
past that got quarantined with no user override. Live recordings,
alternate masterings, and some legitimate uploads routinely drift
further.
New setting `post_processing.duration_tolerance_seconds`. Default 0
means "use auto-scaled defaults" (unchanged behavior for users who
don't touch it). Positive value overrides the per-track defaults.
Capped at 60s — past that the check is effectively off.
Logic lifted to pure helper `resolve_duration_tolerance` in
file_integrity.py. Coerces every plausible input (None / empty /
zero / negative / unparseable / above-cap / numeric string / float)
to either a float override or None for auto. 12 tests pin every
shape.
Wired into `core/imports/pipeline.py` at the integrity-check call
site — runs for ALL matched downloads (Soulseek / Tidal / Qobuz /
HiFi / YouTube / Deezer-direct) since they all share that pipeline.
Settings UI input under Settings → Metadata → Post-Processing.
User caught downloading Kendrick Mr. Morale: three tracks (Rich
Interlude, Savior Interlude, Savior) showed ✅ Completed in the modal
but were missing on disk. Log forensics revealed two layered bugs.
Bug 1 — Verification wrapper assumed success on quarantined files
(`core/imports/pipeline.py`):
The outer `post_process_matched_download_with_verification` had a
fallback at the "no `_final_processed_path` in context" branch that
marked the task completed and notified `success=True`. The inner
post-processor sets `_final_processed_path` only when the file
actually reaches its destination. Integrity-rejected files
(`_integrity_failure_msg` set) and race-guard-failed files
(`_race_guard_failed` set) get quarantined or skipped without ever
setting `_final_processed_path`, so they fell straight into the
"assume success" branch.
Confirmed in user's log:
No _final_processed_path in context for task d5b88b84-... —
cannot verify, assuming success
That line fired for the same task right after the integrity check
quarantined the source file. Result: ✅ Completed in UI, file in
quarantine, never delivered.
Fix: explicit checks for `_integrity_failure_msg` and
`_race_guard_failed` markers BEFORE the assume-success fallback.
Either marker set → task status='failed' with descriptive
error_message + `_notify_download_completed(success=False)`. The
pre-existing assume-success behavior preserved when no failure
markers are set (some legitimate flows complete without setting
`_final_processed_path`).
Bug 2 — AcoustID skip-logic too lenient
(`core/acoustid_verification.py`):
The "language/script" exemption was:
if best_score >= 0.95 and (title_sim >= 0.55 or
artist_sim >= ARTIST_MATCH_THRESHOLD):
The OR-clause fired for English-vs-English titles by the same artist
that share NO actual content. Confirmed in user's log: requested
"Rich (Interlude)" by Kendrick Lamar, AcoustID identified the audio
as "R.O.T.C. (interlude)" by Kendrick Lamar (a totally different
song from his 2010 mixtape) — same artist scored ≥ARTIST threshold,
shared word "interlude" pushed title_sim above 0.55, skip fired.
Verification returned SKIP instead of FAIL, the wrong file was
accepted as the answer for three different track requests.
Fix: skip now requires positive evidence the mismatch is a real
language/script case:
(a) Non-ASCII chars present in either title AND artist matches strongly
→ real transliteration case (kanji ↔ romaji etc)
(b) BOTH title_sim >= 0.80 AND artist_sim >= ARTIST threshold
→ minor punctuation/casing differences
English-vs-English with very different titles by the same artist no
longer skipped — verification correctly returns FAIL, the wrong file
gets quarantined, the new wrapper logic above marks the task failed.
Tests:
- `tests/test_integrity_failure_marks_task_failed.py` — 4 cases
pinning the wrapper-level state machine: integrity marker → failed,
race-guard marker → failed, no markers → still assumes success
(legacy path preserved), integrity-failure-takes-priority over
missing-final-path fallback.
- `tests/test_acoustid_skip_logic.py` — 7 cases pinning the skip
exemption: user's R.O.T.C-vs-Rich case → FAIL (regression test),
Savior-vs-R.O.T.C → FAIL (same bug surface), Japanese kanji →
romaji → SKIP (real language case still works), MAAD vs M.A.A.D →
PASS or SKIP (punctuation tolerance), low fingerprint score →
never skipped, high score but artist mismatch → no longer skipped,
Crown vs Crown of Thorns → no longer skipped.
Verified: full suite 1793 pass (11 new), ruff clean.
WHATS_NEW entry under '2.4.2' dev cycle.
Discord report (fresh.dumbledore [VRN]): slskd sometimes ships broken files
(truncated transfers, corrupt FLAC, wrong file substituted on filename match).
They flowed through post-processing and only surfaced later — Plex/Jellyfin
scan failures, dead-air playback, duplicate detector tripping over the wrong
length. By that point the file was already tagged, copied, mirrored to the
media server, and recorded in provenance.
New module `core/imports/file_integrity.py`:
- `check_audio_integrity(path, expected_duration_ms=None) -> IntegrityResult`
- Three tiered checks, cheapest to most expensive:
1. File size sanity (catches 0-byte stubs and stub transfers)
2. Mutagen parse (catches header damage, wrong-format-with-right-extension)
3. Duration agreement vs. metadata source's expected length, ±3s tolerance
(5s for tracks over 10 minutes — long tracks naturally drift more)
- Returns IntegrityResult with `ok`, human-readable `reason`, and per-check
`checks` dict for debugging
- Never raises; pathological inputs return ok=False with explanation
Pipeline integration in `core/imports/pipeline.py:post_process_matched_download`:
- Hooks between the existing file-stability wait and AcoustID verification
- On failure: quarantine via existing `move_to_quarantine` helper, mark task
failed with descriptive error, clear matched-context, fire
`on_download_completed(success=False)` so the slot is released for retry
- Mirrors the existing AcoustID-failure path so retry behavior stays consistent
- Wrapped in try/except so an unexpected failure inside the check itself
cannot block downloads — logs and continues
This is intentionally tier 1: universal across formats, no external deps.
A future tier could verify FLAC STREAMINFO MD5 by decoding audio (needs
flac binary or libflac wrapper) — skipped for now since tier 1 catches the
dominant Discord-reported cases (truncated, 0-byte, wrong file).
Tests:
- `tests/imports/test_file_integrity.py` — 14 cases covering all three check
tiers, edge cases (zero/negative expected duration, long-track wider
tolerance, caller tolerance override), and the mutagen-unavailable
degradation path
- `tests/imports/test_import_pipeline.py` — two existing tests use 5-byte
fixture files that the new check would reject; they monkeypatch the
integrity check since they're testing plumbing (notification +
metadata_runtime forwarding), not integrity behavior
WHATS_NEW entry under '2.4.2' dev cycle.
slskd appends "_<19-digit unix-nanosecond timestamp>" to a downloaded
filename when the destination already contains a same-named file
(concurrent downloads of the same track, partial-file retries after a
connection drop, cancelled-then-redownloaded files, the same track
surfacing in multiple synced playlists). The file-finder code already
recognized the suffix when matching a download to its source — but
after the canonical file moved into the library, the leftover
"_<timestamp>" siblings sat orphaned in the downloads folder forever.
Reported on Discord by Shdjfgatdif.
cleanup_slskd_dedup_siblings() runs at the end of each successful
import (3 safe_move_file sites in pipeline.py) and prunes any
remaining siblings that strip down to the canonical stem with the
same extension. Conservative match (>= 18 trailing digits) keeps
legitimate filenames like "Track 5" and "Album 1995" untouched. Per-
file unlink failures are swallowed so a single locked file doesn't
block the rest.
17 regression tests cover the suffix-strip primitive, orphan removal,
no-op cases, mismatched extensions, subdirectories, and partial-failure
recovery.
- add core/wishlist as the home for wishlist payload, resolution, state, processing, reporting, and selection helpers
- move wishlist-specific tests into tests/wishlist alongside the new package layout
- keep web_server.py and the import/search callers as thin adapters for now
- 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
- 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
- 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.
- 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.
- 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
- 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
- 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
