fix(slskd): cover-art false positives, variant-recording matches, path-component variant judging - #77
Merged
Conversation
The corruption scanner runs ffmpeg with -err_detect explode. That sets AV_EF_EXPLODE, which makes libavformat abort INPUT OPEN when a FLAC picture block has an unreadable MIME field (flac_picture.c) — before -map 0:a can limit the scan to audio. The file is then deleted, blocklisted and re-searched even though its audio is intact. Verified against ffmpeg 8.1.2 with a hand-built FLAC carrying one malformed picture block: the scanner args exit 183 "Error opening input", while the decoded audio MD5 is byte-identical to the clean source. GLXY "Love Lost" was lost this way from two different peers, which is why it never imported. A failure carrying the artwork markers now re-verifies with a second decode without explode, so the demuxer skips the picture and the audio is judged on its own. That pass is the authority and stays fail-closed: timeout, non-zero exit, or any error output left after benign metadata noise is stripped all report corruption (the pass exits 0 on recoverable decoder errors, hence the stderr check). Failures without the markers — truncation, header damage — take the unchanged path.
A wanted title is a substring of its own variant, so MatchWantedTrackFiles counted "Proposition (Radio Edit)" as covering "Proposition". A radio-edit-only source therefore looked like complete coverage, won the grab, and was rejected at import on track length — burning a grab and a 24h failed-skip window each time. Live 2026-08-06: GLXY "Proposition / Mind Less" grabbed exactly that source; 22 albums across the last three weeks failed import holding variant-qualified files. Track matching now rejects a file whose variant qualifier conflicts with the wanted title, reusing the folder-level RemixSignaturesConflict so the rules stay in one place. Only pairs where one side is decorated pay for the check — plain-vs-plain can never conflict — and the target's MusicBrainz secondary types still forgive the box-set case where the release is marked Live but its track titles are plain.
Artwork: the live Love Lost stderr is recognised, artwork lines strip as benign, real decoder errors survive the strip (fail-closed), and truncation never reaches the re-verify path. Variants: radio-edit-only sources stop matching plain titles, plain and matching-variant sources still match, a mixed source counts as partial, and a Live-typed release with plain track titles still matches its live files. One test documents the pre-existing parent-folder conflict false negative.
|
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 adds FFmpeg recovery for attached-picture decode failures. It also adds qualifier-aware Soulseek track and folder matching, target variant propagation, and regression tests. ChangesFFmpeg artwork recovery
Soulseek variant matching
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)FFmpeg artwork recoverysequenceDiagram
participant CorruptionScanner
participant FfmpegErrorFormatter
participant FFmpeg
CorruptionScanner->>FfmpegErrorFormatter: classify stderr
FfmpegErrorFormatter-->>CorruptionScanner: attached-picture failure
CorruptionScanner->>FFmpeg: retry decode without -err_detect explode
FFmpeg-->>CorruptionScanner: exit status and stderr
Soulseek variant matchingsequenceDiagram
participant AlbumCreation
participant SlskdItemsParser
participant SlskdTextProcessor
AlbumCreation->>SlskdItemsParser: pass target variant types
SlskdItemsParser->>SlskdTextProcessor: compare title and path qualifiers
SlskdTextProcessor-->>SlskdItemsParser: qualifier conflict result
SlskdItemsParser-->>AlbumCreation: matched audio files and coverage
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
🧹 Nitpick comments (1)
src/Sleezer/Indexers/Soulseek/SlskdItemsParser.cs (1)
740-744: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReduce the added comment to a two-line gotcha.
Keep the reason for the qualifier check. Remove the detailed import-history explanation.
Proposed change
- // A wanted "Proposition" is CONTAINED in "Proposition (Radio - // Edit)" — a different recording that the import gate later - // rejects on track length. Only pairs where one side is - // decorated pay for the check; MB secondary types forgive a - // live/remix album whose track titles are plain. + // Do not let a qualified recording satisfy its base title. + // Target secondary types permit valid live/remix candidates.As per path instructions, comments must be navigational or gotcha comments of 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 740 - 744, Shorten the comment above the qualifier check to two lines, retaining only the gotcha that a wanted title can be contained in a decorated variant and that the check should apply only when one side is decorated. Remove the detailed discussion of import rejection, track length, and MusicBrainz secondary types.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 `@tests/Sleezer.Tests/SlskdVariantAndArtworkTests.cs`:
- Around line 164-177: Update
A_live_album_with_plain_track_titles_still_matches_live_files to add a separate
live target using AlbumType "Single" and RequireCoherentSingleSource enabled,
with both plain wanted track titles. Assert that this target requires coverage
of both live files, ensuring the test exercises MatchWantedTrackFiles rather
than relying on the matching folder.
---
Nitpick comments:
In `@src/Sleezer/Indexers/Soulseek/SlskdItemsParser.cs`:
- Around line 740-744: Shorten the comment above the qualifier check to two
lines, retaining only the gotcha that a wanted title can be contained in a
decorated variant and that the check should apply only when one side is
decorated. Remove the detailed discussion of import rejection, track length, and
MusicBrainz secondary types.
🪄 Autofix
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: 79e5caa2-c6c5-4b2b-a1b6-365853d1b580
📒 Files selected for processing (5)
src/Sleezer/Core/PostProcessing/CorruptionScanner.cssrc/Sleezer/Core/PostProcessing/FfmpegErrorFormatter.cssrc/Sleezer/Indexers/Soulseek/SlskdItemsParser.cssrc/Sleezer/Indexers/Soulseek/SlskdTextProcessor.cstests/Sleezer.Tests/SlskdVariantAndArtworkTests.cs
Adds the live album as a coherence-gated single so wanted-track coverage is the only route to a match, alongside the existing album-type case.
The variant check tested the leaf and the parent separately and rejected on either conflicting. That is wrong in both directions. A generic parent vetoed an album whose own title carries the qualifier: "Live at Wembley" under "Music\" conflicted because the parent lacks the word, so live-titled albums were unmatchable in the commonest layout there is. And the "Album (Live)\FLAC" rescue the comment promised never worked — the leaf's own missing-qualifier conflict rejected the folder before the parent was consulted. The qualifier may sit in any component, so the candidate profile is now the UNION over leaf and parent, judged once. The union is deliberately one-way: it can only ADD a qualifier, never cancel one a sibling carries, so an unwanted "(Live)" is still rejected whichever component holds it — a plain sibling can no longer excuse it. Concatenating the components would not work; trailing-word detection is position-sensitive and "One More Light Live" + "FLAC" stops reading as live.
chodeus
changed the title
Owner
Author
|
@coderabbitai review |
|
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 matching/quality fixes from the GLXY audit, each traced to root cause and verified against the real ffmpeg build (8.1.2) the scanner uses.
1. Malformed cover art deletes healthy files
The scanner decodes with
-err_detect explode, which setsAV_EF_EXPLODE. libavformat then aborts input open when a FLAC picture block has an unreadable MIME field (flac_picture.creturnsAVERROR_INVALIDDATAonly under EXPLODE) — before-map 0:acan limit the scan to audio. The file is deleted, blocklisted and re-searched despite intact audio.Reproduced with a hand-built FLAC carrying one malformed picture block:
-err_detect explode)Error opening input→ declared corrupt4c0c7e80…)The reproduced stderr matches the production strike text exactly. GLXY "Love Lost" was destroyed this way from two different peers, which is why that album never imported.
Fix — re-verify, never just ignore. The audio was never judged on the failing pass, so passing on that basis would be unsafe. When (and only when) the artwork markers appear, a second decode runs without explode so the demuxer skips the picture. That pass is the authority and stays fail-closed on all three axes: timeout, non-zero exit, or any error output left after benign metadata noise is stripped (needed because without explode ffmpeg exits 0 on recoverable decoder errors). Failures without the markers keep the unchanged path.
End-to-end against the real binary:
2. Variant recordings satisfy plain wanted tracks
A wanted title is a substring of its own variant, so
MatchWantedTrackFilescountedProposition (Radio Edit)as coveringProposition. A radio-edit-only source looked like full coverage, won the grab, and was rejected at import on track length — burning a grab plus a 24 h failed-skip window each time. 22 albums in the last three weeks failed import holding variant-qualified files.Matching now rejects a file whose qualifier conflicts with the wanted title, reusing the folder-level
RemixSignaturesConflictso the rules live in one place. Only pairs where one side is decorated pay for the check, and MusicBrainz secondary types still forgive the box-set case (release marked Live, track titles plain).3. Path components judged as one candidate
The folder check tested leaf and parent separately and rejected if either conflicted — wrong in both directions:
"Live at Wembley"underMusic\conflicted because the parent lacks the word. Live-titled albums were unmatchable in the commonest layout there is."Album (Live)\FLAC"rescue the old comment promised never worked — the leaf's own missing-qualifier conflict rejected the folder before the parent was ever consulted.The qualifier may sit in any component, so the candidate profile is now the union over leaf and parent, judged once. The union is deliberately one-way — it can only ADD a qualifier, never cancel one a sibling carries — so an unwanted
(Live)is still rejected whichever component holds it, and a plain sibling cannot excuse it. Concatenating the components would not work: trailing-word detection is position-sensitive, and"One More Light Live"+"FLAC"stops reading as live.Verification
415 tests pass (28 new). Each fix was driven from live evidence and confirmed empirically before coding, and the new tests were checked against the old code to prove they fail on it:
Summary by CodeRabbit
Bug Fixes
Tests