From 5771c5ba778042bb883fa46e0771e3047d8a27f7 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Wed, 27 May 2026 22:18:42 -0700 Subject: [PATCH] Album-bundle staging: clean Soulseek copies + sweep orphans at startup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two related leaks in ``storage/album_bundle_staging//``: 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 ```` 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 ````-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. --- core/downloads/lifecycle.py | 93 ++++++- .../test_album_bundle_staging_sweep.py | 237 ++++++++++++++++++ tests/downloads/test_downloads_lifecycle.py | 27 ++ web_server.py | 27 ++ webui/static/helper.js | 1 + 5 files changed, 380 insertions(+), 5 deletions(-) create mode 100644 tests/downloads/test_album_bundle_staging_sweep.py diff --git a/core/downloads/lifecycle.py b/core/downloads/lifecycle.py index 5891e8be..adec519e 100644 --- a/core/downloads/lifecycle.py +++ b/core/downloads/lifecycle.py @@ -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//`` — + 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 ``/`` 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.""" diff --git a/tests/downloads/test_album_bundle_staging_sweep.py b/tests/downloads/test_album_bundle_staging_sweep.py new file mode 100644 index 00000000..9105205e --- /dev/null +++ b/tests/downloads/test_album_bundle_staging_sweep.py @@ -0,0 +1,237 @@ +"""Tests for ``sweep_orphan_album_bundle_staging``. + +The album-bundle staging dir (``storage/album_bundle_staging//``) +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']) diff --git a/tests/downloads/test_downloads_lifecycle.py b/tests/downloads/test_downloads_lifecycle.py index 8be74876..f9da4861 100644 --- a/tests/downloads/test_downloads_lifecycle.py +++ b/tests/downloads/test_downloads_lifecycle.py @@ -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() diff --git a/web_server.py b/web_server.py index 2ba505b9..1c663427 100644 --- a/web_server.py +++ b/web_server.py @@ -36687,6 +36687,33 @@ def start_runtime_services(): else: logger.warning("No stuck flags detected - system healthy") + # Album-bundle staging sweep — remove orphan ```` + # 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() diff --git a/webui/static/helper.js b/webui/static/helper.js index dcf6a421..35123b25 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -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//``). (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 ```` 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 ``//`` 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' },