feat(slskd): variant matching, user-failure downrank, escalating backoff - #62
Merged
Conversation
Three source-selection improvements, the first two adapted from ideas in
lklynet/aurral's Soulseek client:
- Variant-profile matching: RemixSignaturesConflict now compares a
structured profile (live / acoustic / demo / extended / mono-stereo /
remix signature) between search and candidate, so a studio search no
longer matches "(Live)", "(Acoustic)" or "(Demos)" folders and an
Extended Mix no longer satisfies a Radio Edit. Live/acoustic/demo
detection is scoped to qualifier zones (brackets, live-at-venue
phrases, trailing words) so titles that merely contain the word
("Live Forever") are untouched. "(Extended Edition)" stays an
edition, not a variant.
- Per-user failure memory: releases from a user whose downloads failed
in the last 24h are downranked multiplicatively in CalculatePriority
(never zeroed) — previously only the failed release itself was
penalized while the same user's other shares kept full rank.
- Escalating retry backoff: the automatic-search skip for failed
releases now scales with the failure count (1h → 6h → 24h) instead of
a flat 24h, so a transiently busy source retries within the hour.
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR expands Soulseek retry handling with poisoned-history detection, escalating backoff, and per-user priority penalties. It also adds structured release-variant matching, propagates variant metadata through searches, improves title matching, and hardens search and download cleanup. ChangesFailure handling and priority scoring
Title variant matching and tagging
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SlskdRequestGenerator
participant SlskdSearchData
participant SlskdIndexerParser
participant SlskdFolderData
SlskdRequestGenerator->>SlskdSearchData: send TargetVariantTypes
SlskdSearchData->>SlskdIndexerParser: return search results
SlskdIndexerParser->>SlskdFolderData: assign RecentUserFailures
SlskdFolderData->>SlskdFolderData: calculate penalized priority
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 1
🤖 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/SlskdIndexerParser.cs`:
- Around line 312-323: Update the failure aggregation loop around
_historyService.Since to resolve each history entry’s latest grab before
updating perRelease or perUser, then apply the same protocol/indexer filters
used by GetGrabCounts so foreign failures are skipped. Preserve existing retry
grouping and username counting for matching Soulseek entries, and add a
regression test proving failures from another protocol or indexer are ignored.
🪄 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
Run ID: 6990aaf3-eb66-4be1-ab13-85f90b4854a1
📒 Files selected for processing (5)
src/Sleezer/Download/Clients/Soulseek/Models/SlskdDownloadItem.cssrc/Sleezer/Indexers/Soulseek/SlsdkRecords.cssrc/Sleezer/Indexers/Soulseek/SlskdIndexerParser.cssrc/Sleezer/Indexers/Soulseek/SlskdTextProcessor.cstests/Sleezer.Tests/SlskdLiveAuditFollowupTests.cs
…s (review) Unfiltered DownloadFailed history let torrent/usenet failures feed the per-user map — a foreign DownloadUrl's last path segment is not a Soulseek username. Apply the same protocol/indexer scoping as GetGrabCounts. (Regression test skipped: the parser needs the full Lidarr service graph, which the pure-unit test project deliberately excludes.)
… ids, poll/cancel races
Adversarial-audit follow-ups across tonight's changes:
Variant matching (false-REJECT class):
- A keyword confined to the artist prefix ("Lil Flip - Undaground
Legend") is a name, not a qualifier — only the title zone counts.
- Trailing-qualifier detection now survives "[FLAC]"/"(2017)" suffixes
and albums literally titled Live/Acoustic/Demos.
- MusicBrainz SecondaryTypes (Live/Demo/Remix) forgive candidate-side
qualifiers the target's title hides ("Apple Music Live: ...",
"Alive 2007") without ever demanding decoration; threaded from the
search criteria through SlskdSearchData to matcher and tagger.
- Guard also checks the parent path component ("Album (Live)\FLAC").
- Vocabulary: instrumentals/a cappella spellings/sped up/slowed/
nightcore/daycore/reverb/8d/mashup/cover-by.
Title-driven tagging:
- Fail closed on unknown multi-token qualifiers (2+ leftover tokens),
differing digit tokens ("Part 1" vs "Part 12"), TokenSortRatio
tie-break so exact matches beat token-subset saturation, feat-strip
before scoring, numeric-only filenames kept as titles.
- Various Artists album tags no longer disable the fallback; foreign-
artist exclusions are logged. Live/Remix/Demo-secondary singles skip
the fallback entirely (plain track titles can't verify the variant).
Retry ids and re-attach:
- Any terminal history event poisons an id (imported/ignored too, not
just failed); cap walk extended past -r9.
- Restart re-attach skips poisoned grabs (falls through to the newest
clean retry grab) and salting evicts stale same-release attempts from
tracking so they can't swallow transfer state.
Search lifecycle:
- Status-poll 5xx/timeouts feed the failed-poll counter instead of
aborting the tier chain; abnormal wait aborts still cancel; cancel
retries once on transient failure and always runs the terminal wait;
gate queue-wait cap sized to worst-case occupancy.
- Search records are deleted from a finally block so parse failures
no longer leak them; interactive slot moved to ConcurrentDictionary.
Failure state:
- History rows deduped by DownloadId (multi-album failures wrote one
row per album), users charged per bad release not per retry, 48h
lookback so the 24h backoff tier doesn't decay early, cache made
static across Lidarr's transient parser instances.
…om/chodeus/sleezer into feat/source-selection-improvements
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Sleezer/Indexers/Soulseek/SlskdRequestGenerator.cs (1)
293-325: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftThread a CancellationToken through this search path.
SlskdRequestGeneratorhas no token today, so the gate wait, retry delays, and HTTP calls keep running after cancellation and can hold the search slot longer than necessary. Pass a token from the caller and use it inWaitAsync,Task.Delay, and_client.ExecuteAsync(...).🤖 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/SlskdRequestGenerator.cs` around lines 293 - 325, Thread a CancellationToken from the caller through the relevant SlskdRequestGenerator search-finalization method and its helpers, including the shown cancellation/retry path. Apply it to the gate wait, both retry and terminal Task.Delay calls, and HTTP requests such as _client.ExecuteAsync; preserve existing retry, not-found, and completion behavior while allowing cancellation to stop promptly.Source: Path instructions
src/Sleezer/Indexers/Soulseek/SlskdIndexerParser.cs (1)
106-117: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard against a null
Usernamebefore the dictionary lookup.
recentlyFailedUsers.GetValueOrDefault(response.Username)throwsArgumentNullExceptionifresponse.Usernameis null — unlike theHashSet.Containscalls a few lines above,Dictionary<TKey,TValue>lookups don't tolerate a null key. Since this runs inside the single try/catch wrapping the whole response, one folder with a missing/null username would blank out every result for that search response, not just that folder.🛡️ Proposed fix
Token = response.Token, FileCount = response.FileCount, - RecentUserFailures = recentlyFailedUsers.GetValueOrDefault(response.Username) + RecentUserFailures = string.IsNullOrEmpty(response.Username) + ? 0 + : recentlyFailedUsers.GetValueOrDefault(response.Username)🤖 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/SlskdIndexerParser.cs` around lines 106 - 117, Guard the RecentUserFailures assignment in the SlskdFolderData initializer within the response-parsing flow so recentlyFailedUsers.GetValueOrDefault is never called with a null response.Username. Preserve the existing lookup for non-null usernames and use the appropriate empty/default failure collection when the username is null, allowing the current response processing to continue.
🤖 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/SlskdIndexerParser.cs`:
- Around line 241-249: Update the `_interactiveResults` access in the
surrounding search-id handling to use the same nullable definition pattern as
`GetRecentFailureState`, `GetGrabCounts`, and `GetQueuedCounts`. Safely obtain
`_indexer.Definition?.Id` and return from the fire-and-forget path when the
definition or ID is unavailable, before calling `AddOrUpdate`.
In `@src/Sleezer/Indexers/Soulseek/SlskdRequestGenerator.cs`:
- Around line 263-266: Update the catch block around the search polling flow to
handle failures from CancelInFlightSearchAsync separately: log any cleanup
exception without allowing it to replace the original polling exception, then
rethrow the original exception unchanged.
- Around line 420-432: Update the exception filter around GetSearchResultsAsync
so only transient HTTP statuses and network failures are converted to null
polls. Ensure authentication, permission, and other permanent errors are
rethrown instead of reaching the null-poll counter, preserving the existing
debug logging for genuinely transient failures.
- Around line 293-316: The cleanup flow surrounding GetSearchResultsAsync must
not proceed when polling returns an unconfirmed or unknown search state. Update
the terminal-wait logic to continue polling or leave the search record intact
for non-terminal responses, and only allow the subsequent cancellation/deletion
path after an explicit terminal state or confirmed 404; preserve the existing
handling for confirmed NotFound responses in the cancel loop.
---
Outside diff comments:
In `@src/Sleezer/Indexers/Soulseek/SlskdIndexerParser.cs`:
- Around line 106-117: Guard the RecentUserFailures assignment in the
SlskdFolderData initializer within the response-parsing flow so
recentlyFailedUsers.GetValueOrDefault is never called with a null
response.Username. Preserve the existing lookup for non-null usernames and use
the appropriate empty/default failure collection when the username is null,
allowing the current response processing to continue.
In `@src/Sleezer/Indexers/Soulseek/SlskdRequestGenerator.cs`:
- Around line 293-325: Thread a CancellationToken from the caller through the
relevant SlskdRequestGenerator search-finalization method and its helpers,
including the shown cancellation/retry path. Apply it to the gate wait, both
retry and terminal Task.Delay calls, and HTTP requests such as
_client.ExecuteAsync; preserve existing retry, not-found, and completion
behavior while allowing cancellation to stop promptly.
🪄 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
Run ID: 228126d5-9205-4c41-967c-c1d138f03ac3
📒 Files selected for processing (12)
README.mdsrc/Sleezer/Core/PostProcessing/PreImportTagger.cssrc/Sleezer/Core/PostProcessing/TrackTitleMatcher.cssrc/Sleezer/Download/Clients/Soulseek/Models/SlskdDownloadItem.cssrc/Sleezer/Download/Clients/Soulseek/SlskdDownloadManager.cssrc/Sleezer/Indexers/Soulseek/Search/Core/SearchContext.cssrc/Sleezer/Indexers/Soulseek/SlsdkRecords.cssrc/Sleezer/Indexers/Soulseek/SlskdIndexerParser.cssrc/Sleezer/Indexers/Soulseek/SlskdItemsParser.cssrc/Sleezer/Indexers/Soulseek/SlskdRequestGenerator.cssrc/Sleezer/Indexers/Soulseek/SlskdTextProcessor.cstests/Sleezer.Tests/SlskdLiveAuditFollowupTests.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Sleezer/Indexers/Soulseek/SlsdkRecords.cs
- src/Sleezer/Indexers/Soulseek/SlskdTextProcessor.cs
…ation, terminal-confirmed delete - Search-status poll now only swallows TRANSIENT failures (5xx/408/429/ network); auth/permission/permanent errors (401/403/4xx) propagate so a bad API key fails the search instead of reading as empty. - Abnormal-wait cleanup cancel is wrapped so a cleanup exception can't mask the original poll failure. - ExecuteRemovalAsync confirms terminal state (or 404) before DELETE, so a still-InProgress search is left for a later pass instead of being deleted mid-flight into the finalize race. - Null-guard _indexer.Definition?.Id in the interactive-slot Task.Run, matching the nullable handling used everywhere else.
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This was referenced Jul 24, 2026
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.
Three source-selection improvements; the first two adapt ideas from lklynet/aurral's Soulseek client, re-shaped for slskd/Lidarr album semantics.
Variant-profile matching
RemixSignaturesConflictnow compares a structured variant profile — live / acoustic / demo / extended / mono-vs-stereo / remix signature — between search target and candidate. A studio search no longer matches(Live),(Acoustic)or(Demos)folders, and an Extended Mix no longer satisfies a Radio Edit search. Live/acoustic/demo detection is scoped to qualifier zones (bracketed segments,live at/in/fromphrases, trailing words), so titles that merely contain the word — "Live Forever" — never trip it.(Extended Edition)is an edition, not a variant, and stays matchable. Applies everywhere the remix guard already ran: release matching, pre-import tagging, and title-driven track matching.Per-user failure memory
A user whose downloads failed in the last 24h ('File not shared.', remote cancel) now has ALL their releases downranked multiplicatively in
CalculatePriority(×0.65 per failure, floored at ×0.18, never zeroed so a sole source stays usable). Previously only the failed release itself was skipped while the same user's other shares kept full rank.Escalating retry backoff
The automatic-search skip for recently-failed releases escalates with the per-release failure count — 1h after the first failure, 6h after the second, 24h from the third — replacing the flat 24h window, so a transiently busy source retries within the hour while a dead share still sits out a day. Interactive searches remain unfiltered.
Deliberately NOT ported from aurral: the pre-grab minimum-file-size penalty (legitimate single low-bitrate files exist) and search-result caching (slskd owns the network legwork, and caching transient failures as negatives is a footgun).
README note: the Matching & Retry Behaviour section added in #61 describes the flat 24h window; one-line follow-up needed after both merge.
Tests
Variant-dimension conflict matrix (12 rows incl. the title-word guard and edition exclusions), qualifier-zone extraction, backoff escalation, and priority downrank bounds. 239 total, all passing.
Summary by CodeRabbit
Audit remediation (second commit wave)
An adversarial multi-agent audit of this branch plus v1.8.1/v1.9.0 surfaced 21 findings; the confirmed ones are fixed here:
Variant-guard false-rejects (the guard was over-firing): artist names containing keyword words ("Lil Flip") no longer mark the whole folder as a remix; trailing qualifiers survive
[FLAC]/(2017)suffixes; albums literally titled Live/Acoustic/Demos match their well-named folders; the qualifier is also read from the parent path component (Album (Live)\FLAClayouts). MusicBrainz SecondaryTypes now feed the guard asymmetrically: a Live/Demo/Remix-typed target forgives candidate-side decoration its own title hides ("Apple Music Live: …", "Alive 2007") while never demanding decoration and never loosening explicit string qualifiers.Tagging fail-closed hardening: TokenSetRatio's subset-saturation can no longer launder unknown variants — 2+ leftover tokens reject the pair, differing digit tokens reject ("Part 1" ≠ "Part 12"), TokenSortRatio breaks saturated ties so exact titles win their slots, feat-tags are stripped before scoring, and the variant vocabulary gains sped up/slowed/nightcore/daycore/reverb/8D/mashup/cover-by plus missing plural and a-cappella spellings. Various Artists album tags no longer disable the fallback; Live/Remix/Demo-secondary singles skip it entirely.
Retry-id/re-attach coherence: any terminal history event poisons an id (imported/ignored included), restart re-attach prefers the newest non-poisoned grab, and salting evicts stale same-release attempts from tracking.
Search-lifecycle races: status-poll 5xx/timeouts now feed the failed-poll counter instead of aborting the tier chain into a spurious backoff; cancels retry once and always run the terminal wait; search records are deleted from a
finallyso parse failures don't leak them; the interactive slot is a ConcurrentDictionary; the gate queue-wait cap is sized to worst-case occupancy.Failure-state accuracy: multi-album failures (one history row per album) dedupe by DownloadId; users are charged per bad release, not per retry; a 48h lookback keeps the 24h backoff tier from decaying early; the cache is static across Lidarr's transient parser instances.
Deliberately deferred: the native-blocklist repair chain (history-Data key casing + release Guid/InfoHash identity) — a separate PR after a live DB check, since it contradicts an in-repo comment and touches every protocol's blocklist.
Tests: 259 total, all passing.