From 46d8e15674b4b7aa812425c2657cb00629e46464 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Fri, 1 May 2026 09:35:08 -0700 Subject: [PATCH] Prune slskd dedup orphans after import MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 "_" 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. --- core/imports/file_ops.py | 88 ++++++++++ core/imports/pipeline.py | 4 + tests/imports/test_dedup_orphan_cleanup.py | 188 +++++++++++++++++++++ webui/static/helper.js | 1 + 4 files changed, 281 insertions(+) create mode 100644 tests/imports/test_dedup_orphan_cleanup.py diff --git a/core/imports/file_ops.py b/core/imports/file_ops.py index 66b26244..f119eff5 100644 --- a/core/imports/file_ops.py +++ b/core/imports/file_ops.py @@ -4,16 +4,104 @@ from __future__ import annotations import logging import os +import re import shutil import subprocess import time from pathlib import Path +from typing import Iterable, List from config.settings import config_manager logger = logging.getLogger("imports.file_ops") +# slskd appends "_<19-digit unix-nanosecond timestamp>" to a downloaded +# filename when the destination already contains a file with the same +# name (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, etc.). The original +# canonical file usually gets imported and moved into the library while +# the timestamp-suffixed siblings sit orphaned in the downloads folder +# forever. Match the suffix conservatively (≥ 18 digits) so genuine +# user filenames containing trailing numbers don't get hit. +_SLSKD_DEDUP_SUFFIX_RE = re.compile(r"_\d{18,}$") + + +def _strip_slskd_dedup_suffix(stem: str) -> str: + """Return the canonical stem with any slskd dedup suffix removed.""" + return _SLSKD_DEDUP_SUFFIX_RE.sub("", stem) + + +def cleanup_slskd_dedup_siblings(source_path) -> List[str]: + """Remove orphan ``_.`` siblings of a just- + imported file from the source directory. + + Call this AFTER a successful import (the canonical file has already + moved away) using the path the canonical file came from. Looks at + siblings in the same directory whose stem, with the slskd dedup + suffix stripped, equals the imported file's canonical stem and the + same extension. Deletes them. + + Returns the list of deleted paths so the caller can log a summary. + Failures (permissions, racing reader, etc.) are swallowed + individually so a single locked file doesn't block the rest of the + cleanup. + """ + source = Path(source_path) + parent = source.parent + if not parent.is_dir(): + return [] + + canonical_name = source.name + canonical_stem, canonical_ext = os.path.splitext(canonical_name) + # If the imported file ITSELF already had a dedup suffix, the + # "canonical" name is the stripped form — every other sibling that + # also strips down to it is redundant. + canonical_stem = _strip_slskd_dedup_suffix(canonical_stem) + + deleted: List[str] = [] + try: + children: Iterable[Path] = list(parent.iterdir()) + except OSError as e: + logger.debug(f"[Dedup Cleanup] could not list {parent}: {e}") + return [] + + for sibling in children: + if not sibling.is_file(): + continue + # Skip the imported file itself if it's still on disk (it + # shouldn't be — caller invokes us after the move — but the + # check is cheap and keeps the function safe to call from + # other contexts later). + if sibling.name == canonical_name: + continue + sib_stem, sib_ext = os.path.splitext(sibling.name) + if sib_ext.lower() != canonical_ext.lower(): + continue + sib_canonical_stem = _strip_slskd_dedup_suffix(sib_stem) + if sib_canonical_stem != canonical_stem: + continue + # Defensive: don't delete a file that doesn't actually carry + # the slskd dedup suffix — that would imply it's a legitimate + # different file the user intentionally placed there. + if sib_stem == sib_canonical_stem: + continue + try: + sibling.unlink() + deleted.append(str(sibling)) + except OSError as e: + logger.debug(f"[Dedup Cleanup] could not remove {sibling}: {e}") + + if deleted: + logger.info( + "[Dedup Cleanup] removed %d slskd dedup orphan(s) for %r", + len(deleted), + canonical_name, + ) + return deleted + + def safe_move_file(src, dst): """Move a file safely across filesystems.""" src = Path(src) diff --git a/core/imports/pipeline.py b/core/imports/pipeline.py index b34b7483..a25c6be4 100644 --- a/core/imports/pipeline.py +++ b/core/imports/pipeline.py @@ -12,6 +12,7 @@ from typing import Any from config.settings import config_manager from core.imports.file_ops import ( cleanup_empty_directories, + cleanup_slskd_dedup_siblings, create_lossy_copy, downsample_hires_flac, get_audio_quality_string, @@ -238,6 +239,7 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta safe_move_file(file_path, destination) logger.info(f"Moved simple download to: {destination}") + cleanup_slskd_dedup_siblings(file_path) with matched_context_lock: if context_key in matched_downloads_context: @@ -402,6 +404,7 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta logger.info(f"Moving '{os.path.basename(file_path)}' to '{final_path}'") safe_move_file(file_path, final_path) context['_final_processed_path'] = final_path + cleanup_slskd_dedup_siblings(file_path) if config_manager.get('post_processing.replaygain_enabled', False): try: @@ -665,6 +668,7 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta raise FileNotFoundError(f"Source file vanished before move and destination does not exist: {file_path}") safe_move_file(file_path, final_path) + cleanup_slskd_dedup_siblings(file_path) if is_enhance_download and _enhance_source_info.get('original_file_path'): original_enhance_path = _enhance_source_info['original_file_path'] diff --git a/tests/imports/test_dedup_orphan_cleanup.py b/tests/imports/test_dedup_orphan_cleanup.py new file mode 100644 index 00000000..c355a2cb --- /dev/null +++ b/tests/imports/test_dedup_orphan_cleanup.py @@ -0,0 +1,188 @@ +"""Regression tests for slskd dedup-suffix orphan cleanup. + +Discord-reported (Shdjfgatdif): the downloads folder fills up with +files like ``Song_639067852665564677.flac`` over time. slskd appends +``_<19-digit unix-nanosecond timestamp>`` to a 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, etc.). + +The file-finder code already RECOGNIZES the suffix when matching a +download to its source. But after the canonical file is moved into +the library, the leftover ``_`` siblings sat orphaned in +the downloads folder forever. ``cleanup_slskd_dedup_siblings`` runs +at the end of each successful import and prunes them. +""" + +import os +from pathlib import Path + +import pytest + +from core.imports.file_ops import ( + _strip_slskd_dedup_suffix, + cleanup_slskd_dedup_siblings, +) + + +# --------------------------------------------------------------------------- +# Suffix-strip primitive +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "stem,expected", + [ + ("Song", "Song"), + ("Song_639067852665564677", "Song"), # 18 digits → match + ("Song_6390678526655646777", "Song"), # 19 digits → match + ("Song_63906785266556467770", "Song"), # 20 digits → match + ("Song_12345", "Song_12345"), # short → leave alone + ("Track 5", "Track 5"), # legitimate trailing digits → leave alone + ("Album 1995", "Album 1995"), # year suffix → leave alone + ("Mix_2024_639067852665564677", "Mix_2024"), # only the slskd suffix is stripped + ], +) +def test_strip_slskd_dedup_suffix(stem, expected) -> None: + assert _strip_slskd_dedup_suffix(stem) == expected + + +# --------------------------------------------------------------------------- +# Orphan cleanup +# --------------------------------------------------------------------------- + + +def test_removes_orphan_siblings_after_canonical_imported(tmp_path: Path) -> None: + """The reported scenario: canonical ``Song.flac`` was just imported + (moved out of the downloads folder), and ``Song_.flac`` + siblings should be deleted.""" + canonical = tmp_path / "Song.flac" + # Canonical file is GONE — caller invokes us after the move + sibling_a = tmp_path / "Song_639067852665564677.flac" + sibling_b = tmp_path / "Song_639067852665564999.flac" + sibling_a.write_bytes(b"orphan a") + sibling_b.write_bytes(b"orphan b") + + deleted = cleanup_slskd_dedup_siblings(canonical) + + assert len(deleted) == 2 + assert not sibling_a.exists() + assert not sibling_b.exists() + + +def test_does_not_touch_files_with_different_canonical_stem(tmp_path: Path) -> None: + """A sibling that strips down to a DIFFERENT canonical stem belongs + to a different track and must not be deleted.""" + canonical = tmp_path / "Song.flac" + other_track = tmp_path / "OtherSong_639067852665564677.flac" + other_track.write_bytes(b"different track") + + deleted = cleanup_slskd_dedup_siblings(canonical) + assert deleted == [] + assert other_track.exists() + + +def test_does_not_touch_files_with_different_extension(tmp_path: Path) -> None: + """Same canonical stem but different extension is a different + file (e.g. an .mp3 next to a .flac). Don't cross-delete.""" + canonical = tmp_path / "Song.flac" + different_ext = tmp_path / "Song_639067852665564677.mp3" + different_ext.write_bytes(b"different format") + + deleted = cleanup_slskd_dedup_siblings(canonical) + assert deleted == [] + assert different_ext.exists() + + +def test_does_not_touch_files_without_dedup_suffix(tmp_path: Path) -> None: + """A neighbouring file that happens to share the canonical stem + but doesn't have a slskd dedup suffix is a legitimate user file — + leave it alone, even though stripping it would match.""" + canonical = tmp_path / "Song.flac" + legit = tmp_path / "Song.flac" # If it existed, the canonical wouldn't be here + # Actually use a different shape — a file that strips to the same + # canonical stem but has no suffix at all + legit = tmp_path / "Song.flac" # Same as canonical name + # We're running cleanup AFTER the move so the canonical itself is gone. + # But guard against any case where it's still on disk for some reason. + legit.write_bytes(b"still here") + + deleted = cleanup_slskd_dedup_siblings(canonical) + assert deleted == [] + assert legit.exists() + + +def test_handles_canonical_file_that_itself_had_suffix(tmp_path: Path) -> None: + """If the imported file ITSELF had a slskd dedup suffix (because + slskd renamed our preferred copy when an earlier download landed + first), the cleanup must still find sibling orphans by stripping + suffixes on both sides for comparison.""" + canonical = tmp_path / "Song_639000000000000000.flac" # The one we imported + other = tmp_path / "Song_639067852665564677.flac" + other.write_bytes(b"other orphan") + + deleted = cleanup_slskd_dedup_siblings(canonical) + assert len(deleted) == 1 + assert not other.exists() + + +def test_returns_empty_list_when_directory_does_not_exist(tmp_path: Path) -> None: + """Defensive: caller passes a path whose parent dir was already + cleaned up (e.g. another worker pruned the empty folder). Must + not raise.""" + missing = tmp_path / "no" / "such" / "dir" / "Song.flac" + deleted = cleanup_slskd_dedup_siblings(missing) + assert deleted == [] + + +def test_returns_empty_list_when_no_orphans_exist(tmp_path: Path) -> None: + """The common case after most imports: nothing to clean up. Must + return [] without errors.""" + canonical = tmp_path / "Song.flac" + # Pre-existing unrelated files in the same directory + (tmp_path / "TotallyDifferent.flac").write_bytes(b"unrelated") + (tmp_path / "AnotherTrack.mp3").write_bytes(b"unrelated") + + deleted = cleanup_slskd_dedup_siblings(canonical) + assert deleted == [] + + +def test_skips_subdirectories(tmp_path: Path) -> None: + """A subdirectory whose name happens to match the dedup pattern + must not be deleted — only files.""" + canonical = tmp_path / "Song.flac" + subdir = tmp_path / "Song_639067852665564677.flac" + subdir.mkdir() # Subdirectory matching the orphan filename pattern + + deleted = cleanup_slskd_dedup_siblings(canonical) + assert deleted == [] + assert subdir.exists() + assert subdir.is_dir() + + +def test_continues_after_individual_unlink_failure(tmp_path: Path, monkeypatch) -> None: + """A locked file must not block cleanup of the rest. Replace + Path.unlink with a function that fails on a specific path and + succeeds otherwise.""" + canonical = tmp_path / "Song.flac" + locked = tmp_path / "Song_639067852665564677.flac" + cleanable = tmp_path / "Song_639067852665564999.flac" + locked.write_bytes(b"locked") + cleanable.write_bytes(b"cleanable") + + real_unlink = Path.unlink + + def fake_unlink(self): + if self.name == "Song_639067852665564677.flac": + raise PermissionError("locked") + return real_unlink(self) + + monkeypatch.setattr(Path, "unlink", fake_unlink) + + deleted = cleanup_slskd_dedup_siblings(canonical) + + assert len(deleted) == 1 + assert "Song_639067852665564999.flac" in deleted[0] + # The locked one stays + assert locked.exists() diff --git a/webui/static/helper.js b/webui/static/helper.js index 6be9cfb0..dcdc1eaa 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3452,6 +3452,7 @@ const WHATS_NEW = { { title: 'Stats Endpoints Lifted to core/stats', desc: 'internal — moved /api/stats/* and /api/listening-stats/* logic out of web_server.py into core/stats/queries.py with full test coverage. no behavior change. step toward breaking up the web_server.py monolith.' }, { title: 'Search Endpoints Lifted to core/search', desc: 'internal — moved /api/search and /api/enhanced-search/* logic into core/search/ (cache, sources, library_check, stream, basic, orchestrator). 612 fewer lines in web_server.py, 94 new tests. no behavior change.' }, { title: 'Automation Endpoints Lifted to core/automation', desc: 'internal — moved /api/automations/* CRUD + run + history routes, progress tracking helpers, and signal collection into core/automation/ (api, progress, signals). 383 fewer lines in web_server.py, 72 new tests. action handler registration stays put — those closures are tangled with feature implementations.' }, + { title: 'Clean Up slskd Dedup Orphans After Import', desc: 'slskd appends "_" to a download when the destination file already exists (e.g. retried partials, the same track in multiple playlists). the canonical file imported fine but the timestamp-suffixed siblings sat in the downloads folder forever. now they get pruned right after each successful import.', page: 'downloads' }, ], '2.4.0': [ // --- April 26, 2026 — Search & Artists unification + reorganize queue ---