Repository navigation
Conversation
from it through a slskd instance, added as a self-contained source plugin with a full test suite. Soulseek is different from the torrent and usenet sources Shelfmark already supports: a search result is a file sitting on a specific peer, and only the slskd daemon that found it can fetch it, so search and download ship together as one plugin package under shelfmark/release_sources/slskd/ rather than as a new download-client protocol. The API layer wraps just the slice of slskd's REST interface Shelfmark needs and deals with its quirks (asynchronous searches that need polling and cleanup, transfer records that must be cancelled before they can be removed, older versions that return an empty body on enqueue). The source turns peer responses into releases that fit the existing release list, and the handler drives a download through slskd's transfer queue and then hands the finished files to Shelfmark's normal post-processing pipeline, so naming templates, library delivery and multi-user requests all work unchanged. Configuration is a new settings tab, and the completed-files location can be given either as a direct path or through the existing Remote Path Mappings mechanism. The main pieces: - API client — SlskdClient with API-key auth, connection test (verifies the key and that slskd is logged in to Soulseek), search create/poll/collect/delete, transfer enqueue/list/get/cancel, and lookup of slskd's downloads directory. - Release source (indexer) — runs title+author then bare-title queries; ebooks become one release per file, audiobooks group a peer's folder of audio files into one multi-file release (archives kept separate); locked files are skipped, results are filtered by your Supported Formats and ranked free slot → shortest queue → fastest upload; custom columns show peer, slot availability, format and size. - Download handler — enqueues every file of a release, polls progress with speed and remote queue position, fails fast on peer errors, honours cancel and a configurable queue timeout, locates completed files at slskd's <downloads>/<last remote folder>/<file> layout (via the Downloads Path setting or a path mapping for the new slskd client), stages multi-file releases into TMP_DIR so stranger files in a shared folder are never imported, and removes the transfer record and empty folder after a successful import; the download spec is persisted on the task so retries survive a restart. - Settings — "Soulseek (slskd)" tab: enable, URL, API key, Test Connection, downloads path, search wait, max peer responses, queue timeout, remove-completed toggle. - Supporting changes — ReleaseProtocol.SOULSEEK, source registration, slskd in the path-mapping client dropdown, frontend protocol type and dot colour, readme and environment-variable docs. - Test tooling — an slskd service in docker-compose.test-clients.yml and scripts/test_clients.py --write-slskd-config to bootstrap a config with a throwaway Soulseek login and API key. - Tests — 148 new tests covering the API client, release building, the full handler lifecycle against a scripted fake slskd (success, multi-file staging, peer failure, cancel, queue timeout, missing and unsafe paths, cleanup), the settings tab, and the queue → task → retry round-trip through the orchestrator, plus live tests that run against a real instance when SLSKD_TEST_URL/SLSKD_TEST_API_KEY are set. Everything was verified end to end against a running slskd 0.26.0 logged in to Soulseek — a real search returned 37 releases, one was downloaded and its size confirmed on disk, and cancelling a 15-file audiobook mid-transfer left no records behind — and the full suite of 3172 tests, ruff, basedpyright, vulture and the frontend typecheck all pass.
There was a problem hiding this comment.
🟡 Changes recommended
Critical import errors and unresolved search, transfer cleanup, and configuration issues block approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds Soulseek/slskd as a searchable release source and download handler with settings, retry support, cleanup, UI integration, documentation, and tests.
Changes:
- Adds the slskd API client, source, release grouping, and transfer handling.
- Adds configuration, path mappings, frontend support, Docker setup, and documentation.
- Adds unit, integration, and optional live-instance tests.
File summaries
| File | Summary and final review notes |
|---|---|
tests/slskd/test_source.py |
Tests search, filtering, grouping, and ranking. |
tests/slskd/test_settings.py |
Tests settings and connection actions. |
tests/slskd/test_orchestrator_integration.py |
Tests queue, task, and retry integration. |
tests/slskd/test_live_slskd.py |
Optional live slskd integration tests. |
tests/slskd/test_handler.py |
Tests download lifecycle and cleanup. |
tests/slskd/test_api.py |
Tests the slskd API client. |
tests/slskd/conftest.py |
Provides slskd test fixtures and stubs. |
tests/slskd/__init__.py |
Defines the test package. |
src/frontend/src/utils/colorMaps.ts |
Adds the Soulseek protocol color. |
src/frontend/src/types/index.ts |
Adds the Soulseek protocol type. |
shelfmark/release_sources/slskd/source.py |
Implements search and release construction. Moderate (2 votes): expanded searches can lose audiobook files during deduplication. Moderate (2 votes): timeout values below 5 seconds are accepted despite slskd rejecting them. Moderate (1 vote): enabled configurations without an API key are treated as available. |
shelfmark/release_sources/slskd/settings.py |
Defines Soulseek settings and connection testing. Moderate (1 vote): explicitly blank form values can fall back to persisted URL or API key values. |
shelfmark/release_sources/slskd/handler.py |
Implements transfer polling, staging, cancellation, and cleanup. Critical (1 vote): invalid exception-list syntax prevents import. Moderate (1 vote): filename lookups can target unrelated transfers. Moderate (2 votes): cancellation during file location can skip cleanup, including the line 571 path. Moderate (1 vote): missing files can leave other transfers running. |
shelfmark/release_sources/slskd/api.py |
Implements the slskd REST client. Critical (1 vote): invalid exception-list syntax prevents import. Moderate (3 votes): search timeout is sent in milliseconds instead of seconds. |
shelfmark/release_sources/slskd/__init__.py |
Initializes the slskd plugin. |
shelfmark/release_sources/__init__.py |
Registers the protocol and builtin source. |
shelfmark/config/settings.py |
Adds slskd path-mapping configuration. |
scripts/test_clients.py |
Adds slskd test configuration bootstrap support. |
readme.md |
Documents Soulseek setup. |
docs/environment-variables.md |
Documents slskd environment variables. |
docker-compose.test-clients.yml |
Adds the slskd test service. |
Review details
Suppressed comments (5)
shelfmark/release_sources/slskd/handler.py:474
- This lookup returns every non-removed transfer whose filename matches the release, including older failed/completed attempts and another task's transfer for the same peer and filename.
_snapshotalready handles multiple records per filename, so cancellation and post-import cleanup can cancel/delete unrelated records; retain and use only the IDs created for this download (or the newest fallback record per file).
wanted = set(spec.filenames)
return [
str(t["id"])
for t in transfers
if t.get("id") and str(t.get("filename") or "") in wanted and not t.get("removed")
shelfmark/release_sources/slskd/handler.py:573
- When post-processing fails, the orchestrator keeps
task.staged_pathso a retry can reuse the downloaded file, but this branch discards the only cleanup references. A later retry can therefore import the staged file successfully without removing its slskd transfer record or source folder; persist the transfer IDs or reconstruct cleanup from the task before dropping them.
refs = self._cleanup_refs.pop(task.task_id, None)
if refs is None or not success:
return
shelfmark/release_sources/slskd/handler.py:419
- When one file in a multi-file release disappears, this returns after the grace polls without cancelling the other matching transfers. The task reports an error while the remaining downloads continue in slskd, leaving an orphaned transfer consuming peer resources; perform the same release cleanup before returning.
if missing_polls >= MAX_MISSING_POLLS:
status_callback("error", "Transfer disappeared from slskd")
return None
shelfmark/release_sources/slskd/settings.py:25
- Using
ormakes an explicitly blank value from the current form fall back to persisted configuration. If a user clears a saved URL or API key and clicks Test Connection before saving, this tests the old value and can report success even though the form will save an unusable blank value; distinguish missing form keys from explicitly blank values.
raw_url = str(current_values.get("SLSKD_URL") or config.get("SLSKD_URL", "") or "")
api_key = str(current_values.get("SLSKD_API_KEY") or config.get("SLSKD_API_KEY", "") or "")
shelfmark/release_sources/slskd/source.py:485
is_available()treats an enabled URL with no API key as configured, even though the Soulseek settings action requires a key and the normal slskd authentication setup rejects headerless requests. The source then creates an empty-key client, catches the resulting authorization failure insearch(), and presents an empty result instead of an unavailable/misconfigured source. Require the same non-empty key here (as Prowlarr does atshelfmark/release_sources/prowlarr/source.py:1224-1230).
def is_available(self) -> bool:
if not config.get("SLSKD_ENABLED", False):
return False
return bool(normalize_http_url(str(config.get("SLSKD_URL", "") or "")))
- Files reviewed: 20/21 changed files
- Comments generated: 6
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| """Whether slskd currently has a logged-in Soulseek session.""" | ||
| try: | ||
| server = self.get_application().get("server") | ||
| except requests.exceptions.RequestException, SlskdError: |
|
|
||
| search_id = self.start_search( | ||
| search_text, | ||
| timeout_ms=int(wait_seconds * 1000), |
| if local_paths is None: | ||
| if cancel_flag.is_set(): | ||
| status_callback("cancelled", "Cancelled") |
| for release in candidates: | ||
| if release.source_id in seen_ids: | ||
| continue | ||
| seen_ids.add(release.source_id) | ||
| releases.append(release) |
| wait_seconds = _config_int( | ||
| "SLSKD_SEARCH_TIMEOUT", DEFAULT_SEARCH_WAIT_SECONDS, minimum=3, maximum=120 | ||
| ) |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
A syntax error blocks API imports, and several handler, configuration, availability, query, and grouping issues remain unresolved.
Review details
Suppressed comments (8)
Previously missed (1) — in code that hasn't changed since the last review.
shelfmark/release_sources/slskd/settings.py:25
- Using
ortreats an explicitly cleared form field as missing and silently reuses the saved URL/key. That makes it impossible for Test Connection to validate the values the user just entered (or cleared); distinguish a missing key from a present empty value before falling back to config.
shelfmark/release_sources/slskd/api.py:166
- This
exceptclause is invalid Python 3 syntax, so importingshelfmark.release_sources.slskd.apiraises aSyntaxErrorand prevents the new source/handler from loading at all. Wrap the exception types in parentheses.
except requests.exceptions.RequestException, SlskdError:
shelfmark/release_sources/slskd/handler.py:419
- When only part of a multi-file release disappears, this branch gives up without cancelling the transfers that are still present. Those files can continue downloading in slskd after Shelfmark has marked the task failed; the other terminal/error branches call
_cancel_transfersbefore returning. Cancel the release here as well (the call is harmless when all records have disappeared).
if snap.found < len(spec.files):
missing_polls += 1
if missing_polls >= MAX_MISSING_POLLS:
status_callback("error", "Transfer disappeared from slskd")
return None
shelfmark/release_sources/slskd/handler.py:377
- Once
_pollhas reported success, cancellation while_wait_for_filesis locating the completed files takes this branch without calling_cancel_transfers. The completed records (and potentially the files) are therefore left behind even though the task reportscancelled; perform the same removal pass here before returning.
if local_paths is None:
if cancel_flag.is_set():
status_callback("cancelled", "Cancelled")
return None
shelfmark/release_sources/slskd/source.py:485
is_available()treats an enabled source with a URL but no API key as usable, so the normal search flow includes it and then performs unauthenticated requests that fail instead of reporting the source as unconfigured. The settings action and setup both require an API key; include a non-emptySLSKD_API_KEYin this availability check (or make the client auth-optional consistently).
def is_available(self) -> bool:
if not config.get("SLSKD_ENABLED", False):
return False
return bool(normalize_http_url(str(config.get("SLSKD_URL", "") or "")))
shelfmark/release_sources/slskd/source.py:349
_plan_queriesflattens each variant into an author query plus fallback, then truncates that list to three strings. With multiple localized title variants, later variants (and even the second variant's bare-title fallback) are never searched, soexpand_search=Truedoes not run every variant as the surrounding comment promises. Let the wall-clock deadline cap the work, or cap variants before expanding each one.
return unique[:MAX_QUERY_VARIANTS]
shelfmark/release_sources/slskd/source.py:277
- An empty directory is a valid result from
split_remote_pathfor a bare filename, but this key groups every root-level audio match from the peer into one audiobook. Unrelated bare files returned by one search response can therefore be downloaded together; use a per-file key whenpeer_file.directoryis empty (or skip grouping for those files).
by_directory: dict[str, list[PeerFile]] = {}
for peer_file in matching:
by_directory.setdefault(peer_file.directory, []).append(peer_file)
shelfmark/release_sources/slskd/source.py:472
- With
expand_search=True, the title+author and bare-title searches can return different files from the same peer directory.directory_releaseuses(username, directory)as its source id, so this loop discards the later release wholesale and can produce an audiobook missing chapters found by the other query. Merge the files for the same peer/directory across query results before deduplicating releases.
for release in build_releases(responses, content_type=content_type, formats=formats):
if release.source_id in seen_ids:
continue
seen_ids.add(release.source_id)
releases.append(release)
- Files reviewed: 20/21 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
@dskvr I opened #1428, which is this PR rebased onto current |
|
Superceded by #1428 |
Summary
Adds Soulseek as a source, using a slskd instance for both search (indexer) and download. Unlike torrent/usenet sources, search and download are the same service here: a result is a file on a specific peer, and only slskd can fetch it, so this ships as one plugin package (
shelfmark/release_sources/slskd/) with its own handler rather than a newDownloadClientprotocol.What's included
Indexer —
SlskdSourceexpand_searchruns every variant.Downloader —
SlskdHandlerCompleted, Errored/Rejected/TimedOut/..., honours cancel, and gives up after a configurable queue timeout when the peer never starts sending.<downloads>/<last remote folder>/<file>(slskd's layout), via theSLSKD_DOWNLOAD_PATHoverride, or slskd's reported directory passed through Remote Path Mappings for the newslskdclient host.TMP_DIRso unrelated files that happen to share the same per-folder directory are never imported.retry_source_context, so retries survive a restart without an in-memory cache.Settings — new "Soulseek (slskd)" tab: enable, URL, API key, Test Connection (verifies the key and that slskd is logged in to Soulseek), downloads path, search wait, max peer responses, queue timeout, remove-completed toggle.
Small supporting changes —
ReleaseProtocol.SOULSEEK, builtin source registration,slskdin the path-mapping client dropdown, frontendprotocoltype + dot colour, readme and env-var docs,slskdservice indocker-compose.test-clients.yml, andscripts/test_clients.py --write-slskd-configto bootstrap a test config.slskd behaviour worth knowing
DELETE /transfers/downloads/{user}/{id}?remove=trueonly removes a completed transfer. On a live one it cancels and leaves aCompleted, Cancelledrecord, so cancellation does a second removal pass.POST /transfers/downloads/{user}body; older ones return an empty 201. Both are handled (fallback looks the transfers up by filename).searchTimeoutcounts from the last response, so the client also enforces a wall-clock deadline of its own.Testing
tests/slskd/: API client, release building, the full handler lifecycle against a scripted fake slskd (success, multi-file staging, peer failure, cancel, queue timeout, missing/unsafe paths, cleanup), settings, and a queue → task → retry round-trip through the orchestrator.tests/slskd/test_live_slskd.pyruns against a real instance whenSLSKD_TEST_URL/SLSKD_TEST_API_KEYare set (skipped otherwise).ruff check,ruff format,basedpyright,vultureand frontendtscclean.Setup for users
slskd.yml(web.authentication.api_keys.<name>.key, rolereadwrite).slskd.