Album-bundle staging: clean Soulseek copies + sweep orphans at startup

Two related leaks in ``storage/album_bundle_staging/<batch_id>/``:

1. **Soulseek bundle cleanup was excluded.** The per-batch cleanup
   at the end of a bundle download gated on:
       (album_bundle_source or '').lower() in ('torrent', 'usenet')
   The comment justified it as "slskd keeps its own completed
   folders" — but the Soulseek bundle path ALSO copies completed
   files into the private staging dir (``soulseek_client.py:1599``,
   ``copy_audio_files_atomically(completed, Path(staging_dir))``)
   for the per-track workers to claim. Those copies persisted
   forever; long-running installs accumulated stale GB. Extended
   the cleanup gate's allow-list to include ``soulseek`` so the
   per-batch dir is removed on bundle completion — same code path
   that already worked for torrent / usenet.

2. **No sweep for orphan dirs.** Any leftover ``<batch_id>``
   subdir from a previous-session crash, an errored batch, or a
   pre-fix Soulseek bundle stayed on disk forever. Added
   ``sweep_orphan_album_bundle_staging(staging_root, active_batch_ids)``
   that runs ONCE at server startup, before any batch can register
   a staging dir. Removes every ``<batch_id>``-shaped subdir
   whose id isn't in the active set. Safe by construction:
     - Only touches subdirs of the configured staging root.
     - Name-shape check (``entry.name == _safe_batch_dirname(entry.name)``)
       rejects hand-placed dirs like ``.git`` or stray docs.
     - ``shutil.rmtree`` errors log + continue — sweep must not
       crash app startup over a permission glitch.
     - active_batch_ids normalised through ``_safe_batch_dirname``
       so colon-bearing batch_ids match their on-disk form.
   Wired into the web_server startup right after the stuck-flags
   diagnostic so it fires before anything else touches batches.

Tests
- ``test_downloads_lifecycle.py`` gained one regression test
  pinning that Soulseek bundles now have their staging dir
  cleaned (sibling to the existing torrent test).
- ``test_album_bundle_staging_sweep.py`` (NEW, 11 tests)
  covers: orphan removal with no actives, active dirs preserved,
  special-char batch_id normalisation, no-op on missing /empty
  /empty-string staging root, non-dir entries skipped, unsafe-
  name dirs preserved (.git etc.), partial rmtree failure doesn't
  abort the rest, listdir failure returns 0 cleanly, default
  None active set, defensive against empty / None entries in
  the active set.

488 downloads tests pass.

For users with an existing "clean up old files" automation pointed
at this dir: stop pointing it there if you want — the auto-cleanup
+ startup sweep cover it now. Or leave it as belt-and-suspenders
with a relaxed (1h+) mtime threshold so it can't race a mid-batch
download.
This commit is contained in:
Broque Thomas 2026-05-27 22:18:42 -07:00
parent 4ae5aee528
commit 5771c5ba77
5 changed files with 380 additions and 5 deletions

View file

@ -49,15 +49,28 @@ def _safe_batch_dirname(batch_id: str) -> str:
return safe or 'batch'
def _cleanup_private_album_bundle_staging(batch_id: str, batch: dict) -> None:
"""Best-effort cleanup for torrent/usenet private staging copies.
_ALBUM_BUNDLE_CLEANED_SOURCES = ('soulseek', 'torrent', 'usenet')
The torrent/usenet clients keep their own completed download folders.
This only removes SoulSync's per-batch copy under album-bundle staging.
def _cleanup_private_album_bundle_staging(batch_id: str, batch: dict) -> None:
"""Best-effort cleanup for album-bundle private staging copies.
Fires when a batch reaches a terminal state. SoulSync's per-batch
copy lives at ``storage/album_bundle_staging/<batch_id>/``
safe to remove because by the time the batch is "complete" the
per-track workers have already claimed their files out of staging
via ``try_staging_match`` and moved them to the Transfer dir.
Pre-fix this only ran for torrent / usenet bundles because the
comment was "slskd keeps its own completed folders" but the
Soulseek bundle path ALSO copies files into the private staging
dir (``soulseek_client.py:1599``), so slskd bundle copies were
leaking forever. Coverage extended to all three bundle-capable
sources.
"""
if not batch.get('album_bundle_private_staging'):
return
if (batch.get('album_bundle_source') or '').lower() not in ('torrent', 'usenet'):
if (batch.get('album_bundle_source') or '').lower() not in _ALBUM_BUNDLE_CLEANED_SOURCES:
return
staging_path = batch.get('album_bundle_staging_path')
@ -85,6 +98,76 @@ def _cleanup_private_album_bundle_staging(batch_id: str, batch: dict) -> None:
logger.warning("[Album Bundle] Could not clean private staging folder %s: %s", staging_path, exc)
def sweep_orphan_album_bundle_staging(
staging_root: str,
*,
active_batch_ids: Optional[set] = None,
) -> int:
"""Remove orphan per-batch dirs from album-bundle staging.
An orphan is a ``<staging_root>/<dirname>`` subdir whose ``dirname``
matches no batch_id in the current ``download_batches`` runtime
state. Happens when:
- The app crashed mid-bundle (cleanup never fired).
- A batch errored on a non-completion code path.
- A pre-extension Soulseek bundle (where cleanup was gated to
torrent/usenet) left a copy behind.
Intended to run ONCE at server startup, before any new batch can
register an active staging dir. That guarantees ``active_batch_ids``
is genuinely empty / pre-existing; we don't race a starting batch.
Returns the count of dirs removed. Safe-by-design:
- Only touches subdirs of the configured staging root.
- Each candidate goes through the same ``_safe_batch_dirname``
name-guard as the per-batch cleanup, so escape-via-symlink
isn't possible.
- Refuses to act on non-directories.
- ``shutil.rmtree`` errors are logged, not raised sweep must
not crash app startup over a permission glitch.
"""
if not staging_root:
return 0
root = Path(staging_root)
if not root.exists() or not root.is_dir():
return 0
active = active_batch_ids if active_batch_ids is not None else set()
# Normalize active batch ids to their on-disk dirname form so the
# set lookup matches what's actually on disk.
active_dirnames = {_safe_batch_dirname(bid) for bid in active if bid}
removed = 0
try:
entries = list(root.iterdir())
except OSError as exc:
logger.warning("[Album Bundle Sweep] Could not list staging root %s: %s", staging_root, exc)
return 0
for entry in entries:
if not entry.is_dir():
continue
# The directory name MUST match what _safe_batch_dirname
# produces — anything else was hand-created and we leave it
# alone. Defensive against stray dirs the user might have
# placed in the staging root.
if entry.name != _safe_batch_dirname(entry.name):
continue
if entry.name in active_dirnames:
continue
try:
shutil.rmtree(entry)
removed += 1
logger.info("[Album Bundle Sweep] Removed orphan staging dir: %s", entry)
except OSError as exc:
logger.warning("[Album Bundle Sweep] Could not remove orphan staging dir %s: %s", entry, exc)
if removed:
logger.info("[Album Bundle Sweep] Cleaned %d orphan staging dir(s) under %s", removed, staging_root)
return removed
@dataclass
class LifecycleDeps:
"""Bundle of cross-cutting deps the batch lifecycle needs."""

View file

@ -0,0 +1,237 @@
"""Tests for ``sweep_orphan_album_bundle_staging``.
The album-bundle staging dir (``storage/album_bundle_staging/<batch_id>/``)
accumulates orphan subdirs when:
- The app crashed mid-bundle (per-batch cleanup never fired).
- A batch errored on a code path the cleanup gate didn't catch.
- A pre-fix Soulseek bundle ran (the cleanup gate was torrent/usenet
only slskd bundle copies leaked).
The sweep runs once at server startup, BEFORE any new batch can
register a staging dir, so ``active_batch_ids`` is empty / pre-existing.
Tests pin: orphans removed, active dirs preserved, name-guard rejects
escape attempts, sweep no-ops gracefully on missing/empty roots,
non-dir entries skipped.
"""
from __future__ import annotations
from pathlib import Path
import pytest
from core.downloads.lifecycle import sweep_orphan_album_bundle_staging
def _make_batch_dir(root: Path, name: str, with_file: bool = True) -> Path:
"""Create ``root/name/`` with optional placeholder content."""
bd = root / name
bd.mkdir(parents=True, exist_ok=True)
if with_file:
(bd / 'leftover.flac').write_bytes(b'audio')
return bd
# ---------------------------------------------------------------------------
# Happy path — orphans removed, active preserved.
# ---------------------------------------------------------------------------
def test_removes_orphan_dirs_when_no_active_batches(tmp_path):
"""No batches running → every dir under staging root is orphan."""
root = tmp_path / 'album_bundle_staging'
a = _make_batch_dir(root, 'b_abc123')
b = _make_batch_dir(root, 'b_def456')
removed = sweep_orphan_album_bundle_staging(str(root))
assert removed == 2
assert not a.exists()
assert not b.exists()
assert root.exists() # Root itself preserved.
def test_preserves_active_batch_dirs(tmp_path):
"""Dirs whose batch_id is in the active set must NOT be removed.
Belt-and-suspenders sweep runs at startup before batches exist,
but the active-set guard protects against a runtime re-sweep too."""
root = tmp_path / 'album_bundle_staging'
active = _make_batch_dir(root, 'b_running')
orphan = _make_batch_dir(root, 'b_dead')
removed = sweep_orphan_album_bundle_staging(
str(root), active_batch_ids={'b_running'},
)
assert removed == 1
assert active.exists()
assert not orphan.exists()
def test_active_batch_id_with_special_chars_normalises_for_match(tmp_path):
"""The on-disk dirname is ``_safe_batch_dirname(batch_id)``
(alphanumeric + ``-`` + ``_``). The sweep normalises the
provided active batch ids the same way so a batch_id like
``user:123`` (which lands on disk as ``user_123``) still gets
correctly excluded from the orphan set."""
root = tmp_path / 'album_bundle_staging'
active = _make_batch_dir(root, 'user_123')
removed = sweep_orphan_album_bundle_staging(
str(root), active_batch_ids={'user:123'},
)
assert removed == 0
assert active.exists()
# ---------------------------------------------------------------------------
# Safe-by-design — defensive guards against escape / weird state.
# ---------------------------------------------------------------------------
def test_no_op_when_staging_root_missing(tmp_path):
"""Staging root doesn't exist (fresh install) → no-op, no error."""
missing = tmp_path / 'nope'
removed = sweep_orphan_album_bundle_staging(str(missing))
assert removed == 0
def test_no_op_when_staging_root_empty(tmp_path):
"""Root exists but empty → returns 0 cleanly."""
root = tmp_path / 'album_bundle_staging'
root.mkdir()
removed = sweep_orphan_album_bundle_staging(str(root))
assert removed == 0
def test_no_op_when_staging_root_path_empty():
"""Empty string config value → no-op."""
assert sweep_orphan_album_bundle_staging('') == 0
def test_skips_non_directory_entries(tmp_path):
"""Stray files in the staging root must NOT be removed — sweep
only touches batch-id-shaped subdirs."""
root = tmp_path / 'album_bundle_staging'
root.mkdir()
stray_file = root / 'README.txt'
stray_file.write_text('do not delete')
orphan = _make_batch_dir(root, 'b_orphan')
removed = sweep_orphan_album_bundle_staging(str(root))
assert removed == 1
assert stray_file.exists()
assert not orphan.exists()
def test_skips_dirs_with_unsafe_names(tmp_path):
"""A dir whose name doesn't round-trip through
``_safe_batch_dirname`` (e.g. contains ``..`` or a colon) must
be left alone defensive against hand-placed dirs the user
might have created under the staging root for other purposes."""
root = tmp_path / 'album_bundle_staging'
root.mkdir()
# ``.git`` would normalise to ``_git`` so the name doesn't
# round-trip → sweep ignores it.
hand_made = root / '.git'
hand_made.mkdir()
(hand_made / 'config').write_text('[core]')
orphan = _make_batch_dir(root, 'b_orphan')
removed = sweep_orphan_album_bundle_staging(str(root))
assert removed == 1
assert hand_made.exists() # Unsafe-name dir preserved.
assert not orphan.exists()
def test_partial_failure_does_not_abort_remaining(tmp_path, monkeypatch):
"""If shutil.rmtree fails on one dir (permission denied etc.),
the sweep logs and continues must not abort and leak the
rest."""
root = tmp_path / 'album_bundle_staging'
blocking = _make_batch_dir(root, 'b_blocked')
okay = _make_batch_dir(root, 'b_ok')
import core.downloads.lifecycle as lc_mod
real_rmtree = lc_mod.shutil.rmtree
def _selective_rmtree(path, *args, **kwargs):
if Path(path).name == 'b_blocked':
raise OSError('permission denied')
return real_rmtree(path, *args, **kwargs)
monkeypatch.setattr(lc_mod.shutil, 'rmtree', _selective_rmtree)
removed = sweep_orphan_album_bundle_staging(str(root))
assert removed == 1
assert blocking.exists()
assert not okay.exists()
def test_returns_zero_when_listdir_raises(tmp_path, monkeypatch):
"""If the staging root can't be iterated (rare, e.g. permission
issue), sweep logs + returns 0 instead of crashing startup."""
root = tmp_path / 'album_bundle_staging'
root.mkdir()
_make_batch_dir(root, 'b_orphan')
import core.downloads.lifecycle as lc_mod
real_iterdir = Path.iterdir
def _broken_iterdir(self):
if self == root:
raise OSError('listdir blew up')
return real_iterdir(self)
monkeypatch.setattr(Path, 'iterdir', _broken_iterdir)
removed = sweep_orphan_album_bundle_staging(str(root))
assert removed == 0
# ---------------------------------------------------------------------------
# active_batch_ids edge cases.
# ---------------------------------------------------------------------------
def test_none_active_batch_ids_treated_as_empty(tmp_path):
"""``active_batch_ids=None`` (the default) → every dir is orphan."""
root = tmp_path / 'album_bundle_staging'
a = _make_batch_dir(root, 'b_a')
b = _make_batch_dir(root, 'b_b')
removed = sweep_orphan_album_bundle_staging(str(root), active_batch_ids=None)
assert removed == 2
assert not a.exists()
assert not b.exists()
def test_active_set_ignores_empty_or_none_entries(tmp_path):
"""Defensive — caller may pass a set containing None / empty
strings from a partially-initialised state. Skip them so they
don't accidentally match the dirname ``batch`` (the
``_safe_batch_dirname`` fallback)."""
root = tmp_path / 'album_bundle_staging'
orphan = _make_batch_dir(root, 'b_orphan')
removed = sweep_orphan_album_bundle_staging(
str(root), active_batch_ids={'', None, 'b_other'},
)
assert removed == 1
assert not orphan.exists()
if __name__ == '__main__':
pytest.main([__file__, '-v'])

View file

@ -350,6 +350,33 @@ def test_batch_completion_cleans_private_album_bundle_staging(tmp_path):
assert not staging_dir.exists()
def test_batch_completion_cleans_soulseek_bundle_staging(tmp_path):
"""Regression: Soulseek bundles also copy files into the private
staging dir (``soulseek_client.py:1599``). Pre-fix the cleanup
gate excluded ``soulseek`` because of an outdated comment about
slskd "keeping its own completed folders" so slskd bundle
copies leaked under storage/album_bundle_staging forever.
Now soulseek is in the cleanup set alongside torrent / usenet."""
staging_dir = tmp_path / 'b_slskd'
staging_dir.mkdir()
(staging_dir / 'leftover.flac').write_bytes(b'audio')
download_tasks['t1'] = {'status': 'completed', 'track_info': {'name': 'X'}}
download_batches['b_slskd'] = {
'queue': ['t1'], 'queue_index': 1, 'active_count': 1,
'max_concurrent': 1, 'permanently_failed_tracks': [],
'cancelled_tracks': set(),
'album_bundle_private_staging': True,
'album_bundle_source': 'soulseek',
'album_bundle_staging_path': str(staging_dir),
}
deps, _ = _build_deps()
lc.on_download_completed('b_slskd', 't1', True, deps)
assert not staging_dir.exists()
def test_batch_completion_keeps_unexpected_staging_path(tmp_path):
staging_dir = tmp_path / 'shared-staging'
staging_dir.mkdir()

View file

@ -36687,6 +36687,33 @@ def start_runtime_services():
else:
logger.warning("No stuck flags detected - system healthy")
# Album-bundle staging sweep — remove orphan ``<batch_id>``
# dirs left behind by previous-session crashes, errored
# batches, or pre-fix Soulseek bundles that the per-batch
# cleanup gate excluded. Runs once at startup, before any
# new batch can register a staging dir, so we can't race a
# starting batch. download_batches is empty at this point
# (no batches survive a process restart) — every dir on
# disk is by definition an orphan.
try:
from core.downloads.lifecycle import sweep_orphan_album_bundle_staging
_staging_root = config_manager.get(
'download_source.album_bundle_staging_path',
'storage/album_bundle_staging',
) or 'storage/album_bundle_staging'
_swept = sweep_orphan_album_bundle_staging(
_staging_root,
active_batch_ids=set(download_batches.keys()),
)
if _swept:
logger.warning(
"[Startup] Swept %d orphan album-bundle staging dir(s) from %s",
_swept, _staging_root,
)
except Exception as _sweep_err:
# Sweep must not crash startup — log and continue.
logger.warning("[Startup] Album-bundle staging sweep failed: %s", _sweep_err)
# Start simple background monitor when server starts
logger.info("Starting simple background monitor...")
start_simple_background_monitor()

View file

@ -3415,6 +3415,7 @@ function closeHelperSearch() {
const WHATS_NEW = {
'2.6.3': [
{ unreleased: true },
{ title: 'Album-bundle staging: clean up Soulseek copies + startup sweep for orphan dirs', desc: 'two related cleanup issues with the album-bundle staging dir (``storage/album_bundle_staging/<batch_id>/``). (1) Pre-fix the per-batch cleanup at the end of a download only ran for torrent / usenet bundles — Soulseek bundles were excluded with a comment about slskd "keeping its own completed folders", but Soulseek\'s bundle path ALSO copies completed files into the private staging dir for the per-track workers to claim. Those copies leaked forever; users with active Soulseek album downloads accumulated stale GB under the staging root over time. Extended the cleanup gate to include ``soulseek`` so the per-batch dir is removed once the bundle completes — same code path that already worked for torrent / usenet. (2) Added a startup sweep that removes any orphan ``<batch_id>`` subdirs left behind by previous-session crashes, errored batches, or pre-fix Soulseek bundles. Sweep runs once at server boot before any batch can register a staging dir, so it can\'t race a starting batch — safe by construction. Defensive guards: name-shape check rejects non-batch-id dirs (so a hand-placed ``.git`` or ``README.txt`` is left alone), only acts inside the configured staging root, ``shutil.rmtree`` errors log + continue instead of crashing startup. For users who already have a "clean up old files" automation pointed at this dir: stop pointing it there if you want, the auto cleanup + startup sweep cover it now; or leave it as belt-and-suspenders with a relaxed 1h+ mtime threshold. 11 new unit tests pin every edge case.', page: 'downloads' },
{ title: 'Sync page: collapse tabs to brand-logo chips, active swells into a label pill', desc: 'with 14 sync sources now (Spotify, Spotify Link, iTunes Link, Tidal, Qobuz, Deezer, Deezer Link, YouTube, Beatport, ListenBrainz, Last.fm, SoulSync Discovery, Import, Mirrored, plus Server Playlists), the old "row of equal-width labeled pills" tab strip ran out of horizontal room — labels wrapped to 2 lines, the active pill ate disproportionate space, the whole row felt cramped. Restyled the tab strip as a row of circular brand-logo chips (40px on desktop, smaller on tablet/mobile); the currently-active tab swells into a pill with its label inline. Hover tooltip surfaces the source name via the title attribute. Each chip carries its source\'s brand color as a hover ring + active fill (Spotify green, Tidal orange, Qobuz blue, Deezer purple, iTunes coral, YouTube red, Beatport green, LB orange, Last.fm red, SSD teal). To disambiguate the three duplicate-logo sources (Spotify Link / Deezer Link / iTunes Link share a logo with their native-source siblings), each "Link" variant carries a small chain-link badge bottom-right. CSS-only swap — zero JS / behavior changes, same tab click handler, same data-tab routing. The active tab\'s label still shows so context is never lost; non-active tabs trade their label for ~70% horizontal savings.', page: 'sync' },
{ title: 'Fix: Soulseek album downloads stuck on "failed" after slskd finished the release (#715)', desc: 'when an album was downloaded via the Soulseek album-bundle path (the release-first flow added in 2.6), the batch would frequently mark itself as failed even though slskd had successfully downloaded every track of the album. Worst on users running slskd with the common ``directories.downloads.username = true`` config — files landed at ``<download_dir>/<username>/<filename>`` instead of the three hard-coded candidate paths the bundle resolver probed, so every file looked locally missing and the poll spun until the ~30 minute deadline expired. Per-track Soulseek downloads were unaffected because they already routed through a recursive walk-by-basename helper. Lifted that helper into ``core/downloads/file_finder.py`` and rewired the bundle path to use it — same logic the per-track flow has used since 2.5.9. Also: when the bundle poll detects that slskd reports every transfer Completed but no local file resolves within a 45-second grace window, it now exits with a clear log line pointing at the likely ``soulseek.download_path`` mismatch instead of silently spinning until the master deadline. Misleading "(0 tracks, quality=)" log on the preflight-reuse path also fixed — was reading attrs off a None object. 17 new unit tests pin every slskd layout shape (flat, username-prefixed, full-tree-preserved, deep nested, dedup-suffix, quarantine-skip, YouTube/Tidal encoded names, transfer-dir fallback, fuzzy punctuation variants). Closes #715.', page: 'downloads' },
{ title: 'Auto-Sync manager: full visual overhaul to match the dashboard vibe', desc: 'the Auto-Sync manager modal had been carrying its original visual treatment forward unchanged while the rest of the app moved toward the glassy / accent-radial / gradient-border aesthetic the dashboard now sets. Restyled every surface inside the modal to match — selector-based override layer at the end of style.css so functionality is untouched (zero JS or HTML changes). Touched surfaces: modal shell (glass shell with thin accent border + corner radial wash + inner top-edge highlight), header (gradient text title + accent-tinted hairline separator + spinning-X close), KPI summary tiles (dashboard-style gradient tiles with accent top-edge glow on hover + gradient stat numbers), live monitor strip (accent-tinted glass card with status-colored borders), refresh / intro buttons (pill primary with accent fill + glow on hover), tabs (underline-style with accent fill + soft radial glow on the active tab), sidebar (glass panel with accent-tinted source-group cards, accent border on scheduled playlists, accent ring on the filter input focus), board area (subtle accent radial spotlight + column cards with gradient headers + drag-over accent glow), drop zones (animated dashed pull with accent radial wash + accent-tinted text on drag-over), scheduled cards (accent left-edge stripe, gradient pill timing badges, pill "Run now" primary + rotating ghost X), run history rows (dashboard recent-activity aesthetic with accent hover lift + pill status badges), bulk-schedule popover, weekly editor (full glass modal matching the version-modal vibe, day-toggle pills, accent-pill save button), and empty / monitor-empty states. Soft accent-tinted scrollbars throughout. Delete the v2 block at the bottom of style.css to revert.', page: 'automations' },