The album-bundle path COPIES slskd's completed files into private staging (then
on to the library) but never removed slskd's originals, so they piled up in the
download folder. (copy, not move, is correct for the torrent/usenet bundle paths
— those clients keep seeding — so the shared copier can't just always delete.)
Add an opt-in remove_source to copy_audio_files_atomically that deletes each
source ONLY after it copies successfully (never on a failed stage), and set it
for the Soulseek path only. Torrent/usenet keep their originals.
Tests: keeps source by default / removes when requested / keeps on failed copy.
Compared my #730 fix against contributor PR #731 (same independent design).
Grafted their good idea — a confidence bonus when the album's full core phrase
appears intact in the release title (rescues long multi-word names whose token
coverage gets diluted) — and kept my accent-folding, which #731 lacks (their
normalize drops accented chars: Bjork -> 'bj rk').
IMPORTANT: implemented the phrase bonus WORD-BOUNDARY anchored, not as a raw
substring. My first cut used 'phrase in norm_title' (matching #731) and it
immediately reintroduced the substring bug #730 exists to fix — 'heroes'
matched 'superheroes' and the wrong album scored 0.9/passed. PR #731 has this
latent flaw. The regex anchors the phrase to word boundaries so the bonus
fires for real matches only.
Verified: substring trap (Superheroes/Scary Monsters) rejected; edition
suffixes + intact-phrase albums kept. +1 phrase-bonus test (incl. the
word-boundary guard). 126 plugin tests pass; ruff clean.
Co-authored-by: Tyler Richardson-LaPlume <170156756+IamGroot60@users.noreply.github.com>
Self-review found a false-negative in the title-relevance gate I just added:
it scored 'fraction of the ALBUM-NAME's words present in the title', so a
stored album name with an edition/remaster suffix the torrent lacks
('Currents (Deluxe)', 'Heroes (2017 Remaster)') scored BELOW the 0.6 floor and
the correct release was wrongly refused -> fell back to per-track. The very
first issue example ('Heroes 2017 Remaster') would have regressed.
Fix: strip edition/format/year NOISE words (deluxe, remaster, edition, flac,
years, bitrates, ...) before scoring, via _significant_words(), with a fallback
to the raw words so an album literally named '1989' or 'Deluxe' isn't emptied
to match-everything. Verified both directions: edition suffixes now KEPT, while
the wrong-album rejection (Scary Monsters for a Heroes request, Superheroes)
still scores 0.
Tests: +2 regression tests (edition-suffix kept; noise/number-only album name).
125 album-bundle/dispatch/plugin tests pass; compile + ruff clean.
Reporter (IamGroot60): requesting an album via a Prowlarr-backed source
(Usenet/Torrent) could download a DIFFERENT album — e.g. asking for Bowie's
'Heroes' downloaded 'Scary Monsters' because the picker ranked purely by
seeders/grabs -> quality -> size with NO title check, and the wrong album had
~16x the grabs. (Confirmed the old picker chose the wrong release on exactly
this scenario.)
Fix (the reporter's proposal):
- album_title_relevance(candidate_title, album_name): word-coverage match,
accent-folded (Bjork != bj rk) and WORD-BOUNDARY (Heroes != Superheroes), so
a wrong album that shares no title words scores 0.
- pick_best_album_release gains album_name/artist_name params and a relevance
gate (floor 0.6) applied BEFORE the seeders/quality/size ranking. When
album_name is given and NOTHING clears the floor, returns None.
- torrent.py + usenet.py call sites pass album_name/artist_name and set
result['fallback'] = True on None, so the dispatcher (source-agnostic
fallback routing) hands off to the per-track flow instead of grabbing a
wrong album. Matches what Soulseek already did via its preflight scorer.
No album_name -> no gating (old behavior preserved for callers without a
title). Tests: 9 new in test_album_bundle.py (relevance math incl. the
substring trap + accent fold, the exact Bowie refuse-and-fallback scenario,
None-when-no-match, and no-gate-without-name). 125 album-bundle/dispatch/plugin
tests pass; compile + ruff clean.
Follow-up to the poll fix, covering the two things that blocked a
successful end-to-end album import once the poll itself stopped
freezing:
1. Staging dir permissions
The album-bundle private staging path defaults to
'storage/album_bundle_staging' -> /app/storage, but /app/storage was
never created or chowned by the image (unlike /app/Staging,
/app/Transfer, etc.), and /app is root-owned. The copy failed with
"[Errno 13] Permission denied: 'storage'" under the non-root soulsync
UID. Added /app/storage to the Dockerfile build-time mkdir+chown and
the entrypoint PUID/PGID chown, exactly like the sibling runtime dirs.
2. Client->local path resolution
Usenet/torrent clients report save paths from inside THEIR OWN
container (e.g. SAB '/data/downloads/music/<album>'); SoulSync often
mounts the same files at a different point ('/app/downloads/<album>').
Feeding the client path straight to the audio walker yields
"No audio files found" though the files are physically present.
New resolve_reported_save_path():
a. use the reported path as-is if readable (mirrored mounts),
b. apply explicit download_source.usenet_path_mappings
({from,to}, Sonarr/Radarr-style) for non-shared layouts,
c. basename fallback under SoulSync's own download roots —
zero-config for the standard shared-volume arr setup.
Wired into both call sites in usenet.py AND torrent.py
(download_album_to_staging + _finalize_download), logging any
translation and including the resolved path in the no-audio error.
Tests: resolver verbatim / explicit-mapping / basename-fallback /
priority / not-found / empty / mapping-miss-then-basename. ruff +
compileall + pytest green (645 in the download suites).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The earlier #721 fix tolerated a ~10s "completed but no save_path"
window, but the real production stall sits upstream of that: SABnzbd
removes a finished download from the queue and runs par2 verify /
repair / unpack *in History*, exposing the live stage in the slot
`status` ('Verifying' / 'Repairing' / 'Extracting' / 'Moving' / ...)
with `storage` empty until the final move. `_parse_history_slot` mapped
EVERY non-'Failed' status to 'completed', so a still-extracting 1.7 GB
FLAC album looked "completed with no save_path" the instant download hit
100%. The poll burned its completed-no-path budget mid-PP and bailed,
freezing the UI on the last download emit (the stuck-at-99%/100%
signature). SAB then finished fine — which is why the job shows
Completed in History but SoulSync never staged it.
Root fix
- `_parse_history_slot` routes `status` through `_map_state`, so PP
stages stay NON-terminal: the poll keeps waiting (as 'downloading')
for as long as post-processing takes and only a real 'Completed'
flips to terminal success. `save_path` is trusted only on true
completion (mid-PP path fields may point at the incomplete dir).
Supporting / defensive
- `UsenetStatus.incomplete_path`: surfaced separately from save_path
(SAB `incomplete_path`) and used by the poll loops as a LAST RESORT
after the completed-no-path window, to recover the case where
`storage` never lands but the files are physically on disk.
- `poll_album_download`: dedicated, configurable completed-no-path
window (~120s via `download_source.album_bundle_completed_no_path_seconds`)
decoupled from the ~10s transient-miss window; incomplete_path
fallback; a 30s heartbeat log so the previously-silent poll loop is
diagnosable.
- `usenet.py` `_download_thread`: per-track parity — it was erroring
immediately on the first completed-no-path read.
- `album_bundle_dispatch.py` / `status.py` / `monitor.py`: use the
project `get_logger` so download-flow logs land in app.log under the
`soulsync.*` namespace (they were console-only before, which hid the
`[Album Bundle] flow failed` line during triage).
Tests
- PP-history state mapping; end-to-end Hunky Dory PP regression
(download -> Verifying/Extracting in History past both budgets ->
Completed+storage -> success); completed-no-path window +
incomplete_path fallback; per-track thread parity. ruff + compileall +
pytest all green (the only local failures are environmental: missing
tzdata + local tools/ffmpeg.exe, neither present on CI).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Follow-up to the 2.6.3 queue→history handoff fix (#706). User
@IamGroot60 reported in #721 that on 2.6.3 the bundle still gets
stuck mid-flight: SoulSync UI sits on "Usenet downloading release
61%" forever, SAB History shows the job as Completed 2+ minutes
ago, files are physically present in the slskd downloads folder
but never copied into ``storage/album_bundle_staging/<batch>/``.
Root cause: a second-stage gap in the SAB pipeline. SAB flips a
job's ``status`` to ``Completed`` in History as soon as par2 +
unrar finish, but its post-processing pipeline writes the final
``storage`` field a few seconds LATER (the move-to-final step).
``poll_album_download`` saw the first ``Completed`` read with
``save_path=None`` and bailed:
if status.state in complete_states:
return last_save_path # ← None at this point
``download_album_to_staging`` got ``save_path=None``, set
``result['error']`` and returned. The bundle was marked failed but
the LAST progress emit before the failure was ``downloading
progress=0.61``, so the UI froze on "61%" — the terminal ``failed``
emit never registered on the user's screen because the renderer
holds the last-known progress.
Fix
- ``poll_album_download`` now tracks a separate transient counter
for "complete state seen, save_path not yet set." Up to
``transient_miss_threshold`` (default 5) consecutive reads in
that state are tolerated before the poll bails. SAB writes the
``storage`` field within 2-10 seconds of the History flip in
practice — the default 5 × 2s = 10s window covers it.
- When save_path eventually lands, return it normally.
- When the threshold is exhausted with save_path still empty,
emit terminal ``failed`` with an explicit message pointing at
the missing save_path field — no more 6-hour silent spin.
- Earlier ``downloading`` reads with a non-empty ``save_path``
(qBit / Transmission set this from the start of the download)
remain "sticky" — if the eventual ``completed`` read has empty
save_path, the cached one applies. So torrent flows aren't
affected by the retry path.
SAB adapter (``_parse_history_slot``)
- Widened the save_path field fallback chain:
storage → path → download_path → dirname → incomplete_path
Covers SAB version differences (older builds populated ``path``)
and forks that expose ``download_path`` or ``dirname``.
``incomplete_path`` is the last resort — SAB's in-progress dir
before the final move — so the bundle plugin at least has a
path to scan when nothing else lands.
- Whitespace-only values are skipped.
- Loud debug log when none of the known fields land — users on
SAB versions / forks with novel field names need to see this in
logs so we can grow ``_HISTORY_SAVE_PATH_KEYS``.
Tests
- ``test_album_bundle.py`` (3 new):
- tolerates_completed_with_late_save_path_arrival — the #721
scenario; first Completed read has no save_path, third has
it; poll returns the path normally
- gives_up_when_completed_with_no_save_path_persists — past
the threshold the poll fails loudly instead of silent-spinning
- uses_save_path_from_earlier_downloading_emit_if_completed_lacks_one
— sticky save_path keeps torrent flows working
- ``test_usenet_client_adapters.py`` (6 new):
- falls back to ``path`` when ``storage`` empty
- falls back to ``download_path``
- prefers ``storage`` when multiple fields present
- returns ``None`` when all fields empty (the #721 gap window)
- ignores whitespace-only values
- uses ``incomplete_path`` as last resort
132 album-bundle + usenet tests pass.
Branch is on dev parented at 2.6.3 — user @IamGroot60 offered
to test on dev, so this is a candidate cherry-pick for either
a 2.6.4 hotfix or merge straight into dev for the next release.
Follow-up to f13d3395. Five gaps called out on self-review:
1. Per-track inline transient tolerance was duplicated between
usenet.py and torrent.py (~12 lines each, identical) and wasn't
directly tested. Extracted into ``TransientMissCounter`` in
``album_bundle.py`` — small class with ``record_miss()`` returning
True at threshold and ``reset()`` for successful reads. Both
per-track flows AND the lifted ``poll_album_download`` now use
the same counter, so the rule is in one place.
2. Threshold is now config-driven via
``download_source.album_bundle_transient_miss_threshold``
(default 5). Same defensive pattern as ``get_poll_interval`` /
``get_poll_timeout`` — non-positive / non-numeric falls back to
the default. Users with very slow servers (huge multi-disc box
sets, slow disks) can extend the tolerance window without
touching code.
3. SAB state map verified against the canonical Status enum in
``sabnzbd/constants.py`` (sabnzbd/constants.py:~95-118). Dropped
six entries I'd guessed at and couldn't verify in source
(``trying``, ``prop_paused``, ``prop_failed``, ``unpacking``,
``pp``, ``postprocessing``). Kept the verified ``deleted`` (lower-
cased from SAB's ``Deleted``) and added the one real state I'd
missed: ``Propagating`` (SAB's pre-download delay state — maps to
``queued`` since we're waiting on the NZB to be available, not
actively downloading).
4. SAB integration test exercising the queue→history gap end-to-end
through the real adapter HTTP layer. Mocks SAB's queue + history
endpoints with the exact response shapes SAB emits, runs three
gap polls (both endpoints empty), then a recovery poll where the
slot appears in history as Completed. Confirms the TransientMissCounter
absorbs the gap and ``poll_album_download`` returns the save_path
without emitting terminal failure. This was the path I had only
tested at the helper layer before — now pinned end-to-end through
the adapter.
5. SAB state mapping has new tests: every Status value from SAB's
canonical enum must map to a known adapter state (not the 'error'
default fallback), Propagating routes to queued, Deleted routes
to failed. Future SAB state additions that we miss will surface
as 'error' default → transient-miss tolerance → terminal failure
with a clear log line, but the explicit assertion list here means
we'll catch the omission in CI before users do.
Test count after: 537 download-suite tests pass; 21 new
(``TransientMissCounter`` ×4, ``get_transient_miss_threshold`` ×3,
SAB state-coverage ×3, SAB direct ``nzo_ids`` lookup ×5, SAB
queue→history integration ×1, plus the existing helper-layer
coverage from the parent commit). Ruff clean.
User reported usenet album downloads getting stuck on "downloading
release" while SABnzbd reported the job as complete. Container restart
did not help; reproducible on every usenet album download.
Three independent issues all causing the same symptom — the download
modal freezes mid-flow with no error surfaced to the user:
1. SAB queue → history transition window
SAB removes a slot from its queue BEFORE adding it to the history,
and on a busy server (par2 verify, unrar, multi-file move) that
window can span several poll iterations. The poll treated a single
None status as terminal failure ("disappeared from client") and
gave up. Now the poll tolerates up to ~10s of consecutive misses
(5 polls at the default 2s interval) before declaring the job gone.
2. SAB queue states like `Pp` were unmapped
`_SAB_QUEUE_STATE_MAP` didn't cover SAB's `Pp` (post-processing
summary), `Unpacking`, `Trying`, `Deleted`, or the `Prop_paused`
/ `Prop_failed` variants. Unmapped states fell through to the
default-'error' fallback, and the poll loop only treated explicit
'failed' / 'completed' as terminal — 'error' was neither, so the
loop spun until the 6-hour timeout. Map now covers every Status
value from SAB's `sabnzbd/api.py`, and the poll treats the default-
'error' fallback as a transient miss (warn-logged, retry within
the same tolerance window) so a brand-new unmapped state can't
infinite-loop the way `Pp` did here.
3. No terminal failure emit
The poll only logged on failure / timeout / disappeared — never
called the progress callback with 'failed', so the download modal
stayed at the last 'downloading' emit forever. Plumb a 'failed'
emit through every failure exit path so the UI flips out of the
downloading state when the poll gives up.
Plus:
4. SAB direct nzo_ids lookup instead of paging all-history
`_get_status_sync` was fetching the latest 50 history entries on
every poll and iterating to find the target nzo_id. On busy
servers (many recent downloads), the target job could roll past
the 50-entry window and look like a "disappeared" job. Replaced
with a targeted `mode=queue&nzo_ids=<id>` → `mode=history&nzo_ids=<id>`
chain. Falls back to the bulk path for SAB versions that pre-date
the nzo_ids filter — the transient-miss tolerance covers any
short-lived gap there too.
Implementation:
Lifted the album-bundle poll loop out of `usenet.py` and `torrent.py`
into `core/download_plugins/album_bundle.py:poll_album_download` —
near-duplicate implementations are now a single function with deps
injected so it's testable in isolation (kettui's extract-don't-AST-parse
standard; can't unit-test a `time.sleep` loop inside a plugin method).
The lifted helper takes:
- `get_status` callable bound to job_id, so the same loop works for
usenet UsenetStatus and torrent TorrentStatus shapes
- `complete_states` set so torrent's `{'seeding', 'completed'}` and
usenet's `{'completed'}` both Just Work
- `failed_states` set so torrent's `{'error'}` is terminal while
usenet's default-'error' fallback is transient
- `transient_miss_threshold` (default 5 ≈ 10s at 2s poll)
- `sleep` / `monotonic` injectables for deterministic tests
Per-track flows in both plugins gained the same transient-miss
tolerance inline — they don't use the emit pattern (update an
`active_downloads[id]` row dict via lock instead), so reusing the
helper would have required threading a no-op emit through. Inline
fix is small enough.
Tests:
- 11 new tests in `tests/test_album_bundle.py:poll_album_download`
cover the happy path, transient-miss tolerance with recovery,
hard-failure threshold, explicit-failed surface, timeout-emit,
default-'error' transient treatment, shutdown clean exit,
torrent's `seeding`-counts-as-complete, save_path captured across
iterations, and adapter-exception treated as transient miss.
- 521 download-suite tests pass (33 in test_album_bundle, others
pin existing torrent + usenet contracts).
- Ruff clean.
Closes#706.
Per code review: the album-bundle helpers (release picker + staging
collision suffix) were defined as private symbols in torrent.py and
imported by usenet.py through ``from core.download_plugins.torrent
import _pick_best_album_release, _unique_staging_path``. Sibling
plugins shouldn't reach into each other's private surface — leaky
module boundary, and the underscore prefix says don't import.
Also addressed two latent issues at the same time:
- The Auto-Import sweep race: my plugin copied audio files into
staging via plain ``shutil.copy2``, which exposes a partial file
at the audio extension for the duration of the copy. The Auto-
Import worker filters by audio extension when scanning Staging
(AUDIO_EXTENSIONS in core/auto_import_worker.py), so a mid-flight
scan could pick up a truncated file. Fix: copy to a
``.tmp.<random>`` sidecar first, then atomically rename via
``Path.replace`` (which is ``os.replace`` — atomic on the same
filesystem). Auto-Import sees the file either at its final name
or not at all.
- The 6-hour poll timeout was a hard-coded magic constant. Users
with slow private trackers or large box sets would silently time
out after 6h. Both the timeout and the poll interval are now
read from config (``download_source.album_bundle_timeout_seconds``
/ ``..._poll_interval_seconds``) with safe fallback to the
existing defaults when unset / non-numeric.
- core/download_plugins/album_bundle.py: new module owns the
shared surface — ``pick_best_album_release`` (with quality_guess
passed in as a parameter to avoid the circular import that would
result from this module trying to know about torrent.py's title
parser), ``unique_staging_path``, ``atomic_copy_to_staging``,
``copy_audio_files_atomically``, ``get_poll_interval``,
``get_poll_timeout``. Module-level size constants and quality
weights live here too. Usenet's grabs-as-popularity-proxy is
built into the picker so both plugins get the right behavior
without divergent local logic.
- core/download_plugins/torrent.py: drops the local helpers + the
hard-coded poll constants, imports from album_bundle. Per-track
download flow still uses module-level ``_POLL_TIMEOUT_SECONDS``
/ ``_POLL_INTERVAL_SECONDS`` aliases (read from config once at
import time, same as before from a per-track perspective).
- core/download_plugins/usenet.py: drops the imports of the
torrent.py private helpers; everything goes through album_bundle
now. Stops the cross-plugin private-import leak that started
this whole refactor.
- tests/test_album_bundle.py: 23 new tests covering the picker
heuristic (empty input, singleton drop, FLAC preference, grabs
fallback for usenet, size-floor / ceiling boundaries), the
collision-suffix logic, the atomic-copy invariant (concurrent
scanner thread asserts it never observes a partial audio file
during five sequential copies), the failure-skip behavior of the
batch copier, and the config-driven poll cadence including
garbage-input fallback.
- tests/test_torrent_usenet_plugins.py: existing picker tests
updated to call the new module-level helpers instead of the
former torrent.py privates.
BoulderBadgeDad