feat(slskd): single-source matching + fingerprint import verification - #65
Merged
Conversation
Stops slskd single/EP grabs from hauling whole albums or stitching a single together from files scattered across different sources, and adds acoustic (AcoustID) verification so a wrong file whose filename merely matches is rejected. Matching (indexer): - Pluck only the matched track(s) from a larger album share for a single/EP target instead of downloading the whole album — but only when the source is genuinely larger and every target track is matchable, so a track whose title normalizes below the 4-char floor (e.g. "The End" -> "end") is never silently dropped. - Reject a source that holds none of the wanted tracks for a single/EP. - Source coherence for multi-track singles/EPs: prefer a source holding every wanted track, downrank a partial one, and optionally reject it (new "Require Coherent Single Source" setting). Coverage is measured against the titles that survive normalization, not the raw track count. - The recently-failed skip now hashes the plucked download set so the id matches the real downloadId (a plucked single is no longer re-offered every search after a failure). Import verification (pre-import tagger): - Fingerprint-gate the title-driven fallback using Lidarr's bundled AcoustID/fpcalc: tag only when the audio resolves to the wanted MusicBrainz recording (old recording ids accepted, mirroring Lidarr's own scorer). Fail-safe — a file AcoustID can't identify, an outage, or a missing fpcalc falls back to the existing match rather than blocking the import. - New "Verify Imports With Fingerprint" download-client setting extends the check to every file. Both checks are scoped to slskd; the Deezer and Tidal post-processing paths are unaffected. Download client: - On a retry that supersedes a failed attempt, delete the stale attempt's partial files so failover no longer interleaves two sources into one name-keyed folder (mixed editions / duplicate track numbers). Deferred until after a successful enqueue and time-bounded, so a failed enqueue never destroys content with no retry in flight and a slow slskd can't stall the grab. 293 tests pass (7 new covering pluck, coverage-reject, coherence ranking, require-coherent, and the sub-4-char-title edge case).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds fingerprint-gated completed-download tagging, deferred cleanup for superseded Soulseek attempts, and bounded transfer deletion. Small Soulseek targets now use matched-file narrowing and coherence scoring, with updated failure identity hashing and expanded tests. ChangesFingerprint-Gated Post-Processing
Soulseek Source Selection
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SlskdDownloadManager
participant PreImportTagger
participant IFingerprintingService
participant WantedTrack
SlskdDownloadManager->>PreImportTagger: Tag completed download with fingerprint flags
PreImportTagger->>IFingerprintingService: VerifyRecording(file, track)
IFingerprintingService-->>PreImportTagger: Verified, Mismatch, or Unverifiable
PreImportTagger->>WantedTrack: Tag verified candidate
PreImportTagger-->>SlskdDownloadManager: Return tagging counts including skipped files
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/Sleezer/Core/PostProcessing/PreImportTagger.cs (1)
61-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCondense the new narrative comment blocks.
src/Sleezer/Core/PostProcessing/PreImportTagger.cs#L61-L65: reduce to the fallback-vs-mismatch invariant.src/Sleezer/Core/PostProcessing/PreImportTagger.cs#L89-L92: reduce to the old-recording-ID compatibility reason.src/Sleezer/Core/PostProcessing/PreImportTagger.cs#L399-L404: reduce to the title-match fingerprint guard.src/Sleezer/Download/Clients/Soulseek/SlskdDownloadManager.cs#L192-L196: reduce to the deferred-deletion invariant.src/Sleezer/Download/Clients/Soulseek/SlskdDownloadManager.cs#L249-L255: reduce to the shared-folder cleanup risk.src/Sleezer/Download/Clients/Soulseek/SlskdDownloadManager.cs#L1165-L1167: reduce to the cleanup timeout rationale.As per path instructions, comments must be concise 1–2 line navigational/gotcha notes; added narrative essays should be flagged.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Sleezer/Core/PostProcessing/PreImportTagger.cs` around lines 61 - 65, Condense the comments at PreImportTagger.cs lines 61-65, 89-92, and 399-404 to concise 1–2 line notes covering respectively the fallback-vs-mismatch invariant, old-recording-ID compatibility, and title-match fingerprint guard. Condense the comments at SlskdDownloadManager.cs lines 192-196, 249-255, and 1165-1167 to concise notes covering deferred deletion, shared-folder cleanup risk, and cleanup timeout rationale; remove narrative explanations while preserving these navigational/gotcha details.Source: Path instructions
src/Sleezer/Indexers/Soulseek/SlskdItemsParser.cs (1)
160-176: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCondense the added narrative comments.
src/Sleezer/Indexers/Soulseek/SlskdItemsParser.cs#L160-L176: retain only the non-obvious plucking/coherence invariant.src/Sleezer/Indexers/Soulseek/SlskdItemsParser.cs#L662-L668: keep concise constant intent.src/Sleezer/Indexers/Soulseek/SlskdIndexerParser.cs#L151-L163: remove historical incident detail; retain the file-set hash invariant.As per path instructions, comments must be navigational/gotcha-only and limited to 1–2 lines.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Sleezer/Indexers/Soulseek/SlskdItemsParser.cs` around lines 160 - 176, Condense the comments at src/Sleezer/Indexers/Soulseek/SlskdItemsParser.cs lines 160-176 to retain only the non-obvious plucking/coherence invariant in 1–2 lines; at lines 662-668, keep only concise constant intent; and at src/Sleezer/Indexers/Soulseek/SlskdIndexerParser.cs lines 151-163, remove historical incident details while retaining the file-set hash invariant. No code behavior changes are needed.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Sleezer/Download/Clients/Soulseek/SlskdDownloadManager.cs`:
- Around line 249-257: The superseded-attempt cleanup can delete files belonging
to the newly enqueued retry, and its delete tasks continue after the timeout.
Update CleanupSupersededAttemptsAsync and its callers at
src/Sleezer/Download/Clients/Soulseek/SlskdDownloadManager.cs:249-257 and
:1163-1177 to propagate cancellation into each delete operation and ensure
timed-out cleanup cannot continue deleting files in the background; preserve
deferred cleanup after successful retry enqueue.
In `@src/Sleezer/Indexers/Soulseek/SlskdItemsParser.cs`:
- Around line 699-710: Update the title/file matching loop in the parser so each
wanted title is assigned to a distinct filename, preventing one file from
satisfying multiple overlapping titles; derive both coveredTitles and matched
from these one-to-one assignments. Preserve the requirement that every target
title is represented, and add a regression test covering overlapping titles such
as “Falling” and “Falling Slowly” to verify incomplete album shares are
rejected.
- Around line 181-185: Reject sources with no covered tracks unconditionally in
the matching branch around coveredTrackCount, removing the audioFileCount >
expectedTrackCount requirement while preserving the existing logging and
matchedSearchCriteria update. In tests/Sleezer.Tests/SlskdSearchMatchingTests.cs
lines 229-238, replace the fixture with an unrelated source whose album name
matches and whose file count equals the expected count, so the test verifies
this boundary; no other sites require changes.
---
Nitpick comments:
In `@src/Sleezer/Core/PostProcessing/PreImportTagger.cs`:
- Around line 61-65: Condense the comments at PreImportTagger.cs lines 61-65,
89-92, and 399-404 to concise 1–2 line notes covering respectively the
fallback-vs-mismatch invariant, old-recording-ID compatibility, and title-match
fingerprint guard. Condense the comments at SlskdDownloadManager.cs lines
192-196, 249-255, and 1165-1167 to concise notes covering deferred deletion,
shared-folder cleanup risk, and cleanup timeout rationale; remove narrative
explanations while preserving these navigational/gotcha details.
In `@src/Sleezer/Indexers/Soulseek/SlskdItemsParser.cs`:
- Around line 160-176: Condense the comments at
src/Sleezer/Indexers/Soulseek/SlskdItemsParser.cs lines 160-176 to retain only
the non-obvious plucking/coherence invariant in 1–2 lines; at lines 662-668,
keep only concise constant intent; and at
src/Sleezer/Indexers/Soulseek/SlskdIndexerParser.cs lines 151-163, remove
historical incident details while retaining the file-set hash invariant. No code
behavior changes are needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2c508839-488b-4147-b265-e842ea0083c6
📒 Files selected for processing (7)
src/Sleezer/Core/PostProcessing/PreImportTagger.cssrc/Sleezer/Download/Clients/Soulseek/SlskdDownloadManager.cssrc/Sleezer/Download/Clients/Soulseek/SlskdProviderSettings.cssrc/Sleezer/Indexers/Soulseek/SlskdIndexerParser.cssrc/Sleezer/Indexers/Soulseek/SlskdItemsParser.cssrc/Sleezer/Indexers/Soulseek/SlskdSettings.cstests/Sleezer.Tests/SlskdSearchMatchingTests.cs
…afe cleanup - MatchWantedTrackFiles: one-to-one title→file assignment so a lone "Falling Slowly" file can't satisfy both "Falling" and "Falling Slowly" and make an incomplete source look complete. - Take the filename basename after normalizing backslashes — slskd paths are backslash-delimited, which Path.* doesn't split on Linux, so the folder name (often == the single title) was leaking into the track-title haystack and inflating coverage. - Reject a zero-coverage source regardless of file count, so a same-sized source that only matched by name is no longer let through. - Failover cleanup: protect the active retry's own basenames from deletion (a retry sharing the folder can't lose its files), keeping the deferred/ bounded behaviour. - Condense the added comments to short navigational notes. Tests: 294 pass — adds overlapping-title and name-match-no-track regressions and de-vacuums the zero-coverage test.
Owner
Author
|
Addressed the review in
While writing the zero-coverage test it surfaced a real bug: slskd filenames are backslash-delimited, which 294 tests pass. |
Owner
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Sleezer/Core/PostProcessing/PreImportTagger.cs`:
- Around line 61-100: Update VerifyRecording to accept the caller’s cancellation
token and thread it through the fingerprint verification flow, including
_fingerprintingService.Lookup or its cancellation-aware equivalent. Ensure
cancellation propagates promptly instead of being converted into
FingerprintVerdict.Unverifiable, while preserving existing handling for ordinary
lookup errors and verdicts.
In `@src/Sleezer/Download/Clients/Soulseek/SlskdDownloadManager.cs`:
- Around line 1164-1178: Update CleanupSupersededAttemptsAsync and the
RemoveItemFilesAsync/DeleteTransferAsync call chain to accept and honor the
cleanup CancellationToken, so timed-out deletes are actually cancelled before
retry proceeds. Handle OperationCanceledException caused by the 20-second
timeout separately from other exceptions, and log non-timeout API or network
failures with their original error context rather than labeling them as
timeouts.
In `@src/Sleezer/Indexers/Soulseek/SlskdItemsParser.cs`:
- Around line 668-709: Update MatchWantedTrackFiles when constructing named so
it filters files through the existing audio-file predicate before title
matching. Ensure non-audio files cannot be claimed or added to matched, while
preserving the current one-to-one title matching behavior and audio-file
handling when OnlyAudioFiles is disabled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 72d7d9fc-a007-4a06-bed5-04b3fc579f88
📒 Files selected for processing (7)
src/Sleezer/Core/PostProcessing/PreImportTagger.cssrc/Sleezer/Download/Clients/Soulseek/SlskdDownloadManager.cssrc/Sleezer/Download/Clients/Soulseek/SlskdProviderSettings.cssrc/Sleezer/Indexers/Soulseek/SlskdIndexerParser.cssrc/Sleezer/Indexers/Soulseek/SlskdItemsParser.cssrc/Sleezer/Indexers/Soulseek/SlskdSettings.cstests/Sleezer.Tests/SlskdSearchMatchingTests.cs
- MatchWantedTrackFiles: match against audio files only, so a same-named .cue/.nfo can't be claimed (and then plucked) ahead of the real track. - VerifyRecording: take a CancellationToken; run the non-cancellable IFingerprintingService.Lookup off-thread and stop waiting when the token trips, propagating cancellation instead of swallowing it — a slow fpcalc/AcoustID lookup no longer blocks the import loop past the post-processing timeout. - Superseded-attempt cleanup: distinguish the time bound (retry files stay protected) from real API/network errors instead of logging both as a timeout, and bound each slskd delete request to 15s so detached deletes drain quickly. IHttpClient/Lookup take no token, so this is the feasible cancellation path. 295 tests pass (adds a non-audio-file pluck regression).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Sleezer/Indexers/Soulseek/SlskdItemsParser.cs (1)
694-699: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMatch title phrases, not arbitrary substrings.
Name.Contains(title)makes a wanted title likeHomematchHometown. That false positive can mark an incomplete source as covered and pluck the wrong audio file. Match the normalized title as a whole token sequence, while still allowing suffixes such asLive.Proposed fix
- if (claimed.Contains(i) || !named[i].Name.Contains(title)) + if (claimed.Contains(i) || + !($" {named[i].Name} ").Contains($" {title} ", StringComparison.Ordinal)) continue;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Sleezer/Indexers/Soulseek/SlskdItemsParser.cs` around lines 694 - 699, Update the title comparison in the parsing loop over `titles` and `named` so it matches the normalized title as a whole token sequence rather than using arbitrary substring matching. Preserve valid suffix matches such as “Live”, while preventing titles like “Home” from matching “Hometown” and incorrectly claiming the source.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Sleezer/Indexers/Soulseek/SlskdItemsParser.cs`:
- Around line 681-683: Update IsAudioFile, used by the named selection in the
parser, so an empty Extension explicitly falls back to the filename-derived
extension using the same string.IsNullOrEmpty behavior as
GetMostCommonExtension; preserve the existing Extension value when non-empty.
Add a regression fixture covering a valid audio filename such as track.flac with
an empty Extension.
---
Outside diff comments:
In `@src/Sleezer/Indexers/Soulseek/SlskdItemsParser.cs`:
- Around line 694-699: Update the title comparison in the parsing loop over
`titles` and `named` so it matches the normalized title as a whole token
sequence rather than using arbitrary substring matching. Preserve valid suffix
matches such as “Live”, while preventing titles like “Home” from matching
“Hometown” and incorrectly claiming the source.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4adb9b77-7d99-418e-a010-c515d524fe1e
📒 Files selected for processing (5)
src/Sleezer/Core/PostProcessing/PreImportTagger.cssrc/Sleezer/Download/Clients/Soulseek/SlskdApiClient.cssrc/Sleezer/Download/Clients/Soulseek/SlskdDownloadManager.cssrc/Sleezer/Indexers/Soulseek/SlskdItemsParser.cstests/Sleezer.Tests/SlskdSearchMatchingTests.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/Sleezer.Tests/SlskdSearchMatchingTests.cs
- src/Sleezer/Download/Clients/Soulseek/SlskdDownloadManager.cs
- src/Sleezer/Core/PostProcessing/PreImportTagger.cs
`f.Extension ?? Path.GetExtension(...)` only falls back on null, so an EMPTY Extension left a valid track.flac unrecognized as audio — which, now that MatchWantedTrackFiles filters on IsAudioFile, could drop every match and reject an otherwise-valid small-target source. Use the same IsNullOrEmpty filename fallback as GetMostCommonExtension. 296 tests pass (adds an empty-Extension fixture).
Owner
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Sleezer/Core/PostProcessing/PreImportTagger.cs (1)
116-138: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCancellation is swallowed before it reaches the caller's "timed out" handling.
VerifyRecording(and thect.ThrowIfCancellationRequested()calls inTagInternal) rethrowOperationCanceledExceptionon cancellation — but the only entry point,TagCompletedDownloadAsync, catches it with a blanketcatch (Exception ex)and converts it into a normal completedTask<TaggingResult>(TaggingResult(0, 0, 1)), logged via_logger.Error. This meansSlskdDownloadManager.EnqueuePostProcess's dedicatedcatch (OperationCanceledException) { _logger.Warn("[post-process] Timed out for {ItemId}", ...); }never fires for a post-processing timeout that occurs during tagging — every cancellation is instead reported as a tagging failure with a full error stack trace. This negates the intent of the previously-fixed cancellation propagation (VerifyRecording rethrowingOperationCanceledExceptionso the import loop stops promptly and is distinguishable from a real failure).🐛 Proposed fix
public Task<TaggingResult> TagCompletedDownloadAsync( ... try { TaggingResult result = TagInternal(album, artist, albumRelease, sourceId, completedFolderPath, confidenceThreshold, stripFeaturedArtists, verifyAllWithFingerprint, fingerprintTitleFallback, ct); return Task.FromResult(result); } + catch (OperationCanceledException) + { + throw; + } catch (Exception ex) { _logger.Error(ex, "Pre-import tagging failed for {SourceId}", sourceId); return Task.FromResult(new TaggingResult(0, 0, 1)); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Sleezer/Core/PostProcessing/PreImportTagger.cs` around lines 116 - 138, Update TagCompletedDownloadAsync so OperationCanceledException is rethrown to the caller instead of being converted into TaggingResult(0, 0, 1) by the blanket exception handler. Preserve the existing logging and failure result for non-cancellation exceptions, allowing EnqueuePostProcess’s cancellation-specific handling to receive tagging timeouts.src/Sleezer/Download/Clients/Soulseek/SlskdDownloadManager.cs (1)
1191-1229: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep local cleanup running if the remote delete fails
src/Sleezer/Download/Clients/Soulseek/SlskdDownloadManager.cs:1195-1198— the twoDeleteTransferAsyncawaits run before the local delete block, so one thrown delete skips cleanup for that file. In the superseded-attempts path, that leaves stale files behind when the transfer is already gone. Wrap each remote delete and continue to local cleanup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Sleezer/Download/Clients/Soulseek/SlskdDownloadManager.cs` around lines 1191 - 1229, The per-file cleanup flow in the Task.WhenAll block must continue to local deletion when either _apiClient.DeleteTransferAsync call fails. Wrap each remote delete await independently so failures are handled without aborting the file’s cleanup, while preserving the existing delay and remove=true behavior.
🧹 Nitpick comments (1)
src/Sleezer/Core/PostProcessing/PreImportTagger.cs (1)
295-302: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFingerprint mismatch skips are conflated with weak-match skips in the metric/log.
Both the album-level (295-301) and title-fallback (405-411) paths increment
skipped, which is reported asTaggingResult.SkippedWeakMatchand logged underskipped_weak_match=(line 327). The doc-comment onTaggingResult(lines 109-114) definesSkippedWeakMatchspecifically as "identification confidence below the threshold" — a fingerprint-confirmed different-recording is a materially different (and more actionable) outcome than a merely-weak match, but operators can no longer distinguish them from the summary log or the result tuple.Consider a separate counter (e.g.
FingerprintMismatch) or at least a distinct log line, so troubleshooting fingerprint-gated imports isn't muddied with ordinary weak-match skips.Also applies to: 405-411
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Sleezer/Core/PostProcessing/PreImportTagger.cs` around lines 295 - 302, Separate fingerprint-confirmed mismatches from ordinary weak-match skips in the pre-import tagging results. Update the album-level and title-fallback fingerprint mismatch paths in the relevant tagging method to use a dedicated counter/result field and distinct summary logging, while leaving SkippedWeakMatch for confidence-threshold failures only. Ensure TaggingResult and its documentation expose the new outcome consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Sleezer/Download/Clients/Soulseek/SlskdDownloadManager.cs`:
- Around line 1202-1204: Update the filename normalization in the cleanup logic
surrounding fileName to handle a null file.Filename consistently with the
equivalent computation near line 254. Use an empty-string fallback before
replacing path separators, preserving basename extraction and allowing local
cleanup to proceed without a null dereference.
- Around line 247-259: Change the superseded-attempt cleanup in the enqueue flow
to run fire-and-forget instead of awaiting CleanupSupersededAttemptsAsync.
Preserve the existing supersededAttempts guard and retryBasenames construction,
and follow the established CleanStaleDirectoriesAsync invocation pattern while
retaining the cleanup method’s own timeout and failure handling.
---
Outside diff comments:
In `@src/Sleezer/Core/PostProcessing/PreImportTagger.cs`:
- Around line 116-138: Update TagCompletedDownloadAsync so
OperationCanceledException is rethrown to the caller instead of being converted
into TaggingResult(0, 0, 1) by the blanket exception handler. Preserve the
existing logging and failure result for non-cancellation exceptions, allowing
EnqueuePostProcess’s cancellation-specific handling to receive tagging timeouts.
In `@src/Sleezer/Download/Clients/Soulseek/SlskdDownloadManager.cs`:
- Around line 1191-1229: The per-file cleanup flow in the Task.WhenAll block
must continue to local deletion when either _apiClient.DeleteTransferAsync call
fails. Wrap each remote delete await independently so failures are handled
without aborting the file’s cleanup, while preserving the existing delay and
remove=true behavior.
---
Nitpick comments:
In `@src/Sleezer/Core/PostProcessing/PreImportTagger.cs`:
- Around line 295-302: Separate fingerprint-confirmed mismatches from ordinary
weak-match skips in the pre-import tagging results. Update the album-level and
title-fallback fingerprint mismatch paths in the relevant tagging method to use
a dedicated counter/result field and distinct summary logging, while leaving
SkippedWeakMatch for confidence-threshold failures only. Ensure TaggingResult
and its documentation expose the new outcome consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e784a72a-7d60-432e-ab5a-5228adc8edf9
📒 Files selected for processing (8)
src/Sleezer/Core/PostProcessing/PreImportTagger.cssrc/Sleezer/Download/Clients/Soulseek/SlskdApiClient.cssrc/Sleezer/Download/Clients/Soulseek/SlskdDownloadManager.cssrc/Sleezer/Download/Clients/Soulseek/SlskdProviderSettings.cssrc/Sleezer/Indexers/Soulseek/SlskdIndexerParser.cssrc/Sleezer/Indexers/Soulseek/SlskdItemsParser.cssrc/Sleezer/Indexers/Soulseek/SlskdSettings.cstests/Sleezer.Tests/SlskdSearchMatchingTests.cs
- CleanupSupersededAttemptsAsync is now fire-and-forget — it self-bounds (20s), protects the retry's own basenames, and handles its own errors, so awaiting it only added up to 20s to a successful enqueue for no benefit. - Null-guard file.Filename in RemoveItemFilesAsync's basename derivation, matching the retry-basename computation and the rest of the file.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
For a monitored single/EP, the slskd indexer would match any album folder that merely contained the wanted track (track-title evidence forces the match), then download the whole album and keep the one file whose title matched. That produced:
And imports were trusted on filename/tag title only — a wrong file whose name matched got tagged as the wanted track, with no content check.
Changes
Matching (indexer)
The End→end) is never silently dropped; those sources download whole.downloadIdand a plucked single isn't re-offered every search after a failure.Import verification (pre-import tagger)
fpcalc: tag only when the audio resolves to the wanted MusicBrainz recording (old recording ids accepted, mirroringDistanceCalculator). Fail-safe — a file AcoustID can't identify, an outage, or a missingfpcalcfalls back to the existing match rather than blocking the import.Download client
Settings (defaults preserve current behaviour where it matters)
Require Coherent Single Source(indexer) andVerify Imports With Fingerprint(download client) are opt-in.Testing
dotnet test— 293 pass, 0 fail (7 new: pluck, coverage-reject, coherent-vs-partial ranking, require-coherent, and the sub-4-char-title edge case).Summary by CodeRabbit