fix(crossseed): prevent false cross-seed delete warnings #1148
Conversation
Local cross-seed detection now verifies file overlap for ambiguous `content_path == save_path` cases.
Adds `strict=true` to `/cross-seed/torrents/{instanceID}/{hash}/local-matches` for delete dialogs to fail safe on overlap check errors, with best-effort mode elsewhere. OpenAPI updated.
|
Warning Rate limit exceeded@s0up4200 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 7 minutes and 10 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds a boolean Changes
Sequence Diagram%%{init: {"themeVariables":{"actorBackground":"#E6F4FF","actorBorder":"#8FC9FF","noteBackground":"#FFF3E0","noteBorder":"#FFCC80"}}}%%
sequenceDiagram
participant Client as Web Client
participant Handler as API Handler
participant Service as Cross-seed Service
participant QBT as qBittorrent Instances
participant Cache as localMatchContext (lazy cache)
Client->>Handler: GET /api/.../local-matches?strict=true
Handler->>Service: FindLocalMatches(ctx, instanceID, hash, strict=true)
Service->>QBT: Fetch source torrent & candidates
Service->>Cache: init matchCtx (empty cache)
rect rgb(230, 255, 240)
note over Service,Cache: Per-candidate evaluation (loop)
loop for each candidate
Service->>Service: determineLocalMatchType(candidate, matchCtx)
alt ambiguous content_path
Service->>Cache: getSourceFiles() (lazy)
Cache->>QBT: Fetch source files if not loaded
Service->>Service: candidateSharesSourceFiles() -> overlap%
alt overlap >= threshold
Service->>Service: mark as match
else
Service->>Service: no match
end
else non-ambiguous
Service->>Service: direct match decision
end
end
end
alt strict=true AND overlap fetch errors occurred
Service->>Handler: return error
Handler->>Client: 4xx/5xx
else
Service->>Handler: return LocalMatchesResponse (partial/complete)
Handler->>Client: 200 OK
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 0
🧹 Nitpick comments (5)
internal/services/crossseed/service.go (2)
489-538:collectLocalMatchesrefactor is clean, but consider early context cancellation checksThe helper cleanly centralizes per‑instance iteration, self‑match skipping, and
LocalMatchconstruction. You might optionally add aif ctx.Err() != nil { return matches }check inside the outer or inner loop to avoid doing potentially expensive cached‑torrent scans when the request is already canceled, especially on large deployments.
540-589: Lazy source file loading vialocalMatchContextis efficient and safeThe single‑shot
getSourceFilesimplementation (withfetchedguard, explicit empty‑file error, and normalized name+size keys) avoids redundant API calls and makes strict‑mode failures explicit, without impacting non‑ambiguous paths. One minor improvement would be to factor the key construction (strings.ToLower(normalizePath(name)) + "|" + strconv.FormatInt(size, 10)) into a small helper to keep the logic shared with candidate overlap code in one place.internal/services/crossseed/service_local_matches_test.go (1)
85-167: localMatchSyncManager fake is appropriately minimal and robustThe fake sync manager covers just enough of the production interface for these tests and keeps behavior explicit:
GetTorrentFilesBatchhandles both normalized and raw-lowercase hashes, which makes the tests resilient to internal hash-normalization details.- Other methods are harmless no-ops returning zero values, avoiding unintended side effects.
This is a good balance between realism and test isolation.
If you ever need to validate interface drift, consider adding a compile-time assertion (e.g.
var _ qbittorrentSync = (*localMatchSyncManager)(nil)) next to the type to catch missing methods early.internal/web/swagger/openapi.yaml (1)
3163-3178: Newstrictquery parameter is consistent and well documentedThe
strictboolean query parameter on/api/cross-seed/torrents/{instanceID}/{hash}/local-matches(defaultfalse) matches the handler docs and clearly describes the “fail if overlap checks cannot complete” behavior for delete dialogs. This keeps the API backward compatible while exposing stricter semantics when desired.If strict failures map to specific error types (e.g., “partial results due to qBittorrent file API issues”), consider mentioning that nuance in the
400/500descriptions in a follow-up to help API consumers distinguish configuration vs. backend failures.internal/api/handlers/crossseed.go (1)
453-481: Strict flag plumbed correctly from HTTP to service; consider slightly more robust parsing
GetLocalMatchesnow:
- Documents
strictin the swagger annotation.- Parses
strictfromr.URL.Query()and passes it through toFindLocalMatches.This cleanly exposes the new strict mode to callers while keeping the non-strict default when the param is omitted. The behavior aligns with the OpenAPI change.
If you want to be a bit more forgiving to non-UI clients, you could parse the query value with a case-insensitive or boolean parser (so
?strict=TRUEor?strict=1are also accepted) instead of a strict equality with"true", but that’s purely optional.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
internal/api/handlers/crossseed.gointernal/services/crossseed/service.gointernal/services/crossseed/service_local_matches_test.gointernal/web/swagger/openapi.yamlweb/src/hooks/useCrossSeedWarning.tsweb/src/lib/api.ts
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: s0up4200
Repo: autobrr/qui PR: 641
File: internal/services/crossseed/service.go:209-212
Timestamp: 2025-11-28T20:32:30.126Z
Learning: Repo: autobrr/qui PR: 641
File: internal/services/crossseed/service.go
Learning: The cross-seed recheck-resume worker intentionally runs for the process lifetime and keys pending entries by hash only. This is acceptable under the current constraint that background seeded-search runs operate on a single instance at a time; graceful shutdown and instanceID|hash keying are deferred by design.
📚 Learning: 2025-11-28T20:32:30.126Z
Learnt from: s0up4200
Repo: autobrr/qui PR: 641
File: internal/services/crossseed/service.go:209-212
Timestamp: 2025-11-28T20:32:30.126Z
Learning: Repo: autobrr/qui PR: 641
File: internal/services/crossseed/service.go
Learning: The cross-seed recheck-resume worker intentionally runs for the process lifetime and keys pending entries by hash only. This is acceptable under the current constraint that background seeded-search runs operate on a single instance at a time; graceful shutdown and instanceID|hash keying are deferred by design.
Applied to files:
internal/api/handlers/crossseed.goweb/src/hooks/useCrossSeedWarning.tsinternal/services/crossseed/service_local_matches_test.gointernal/services/crossseed/service.go
📚 Learning: 2025-11-28T22:21:20.730Z
Learnt from: s0up4200
Repo: autobrr/qui PR: 641
File: internal/services/crossseed/service.go:2415-2457
Timestamp: 2025-11-28T22:21:20.730Z
Learning: Repo: autobrr/qui PR: 641
File: internal/services/crossseed/service.go
Learning: The determineSavePath function intentionally includes a contentLayout string parameter for future/content-layout branching and API consistency. Its presence is by design even if unused in the current body; do not flag as an issue in reviews.
Applied to files:
internal/api/handlers/crossseed.gointernal/services/crossseed/service_local_matches_test.gointernal/services/crossseed/service.go
📚 Learning: 2025-12-28T18:44:10.496Z
Learnt from: s0up4200
Repo: autobrr/qui PR: 876
File: internal/logstream/hub_test.go:188-192
Timestamp: 2025-12-28T18:44:10.496Z
Learning: In Go 1.25 (Aug 2025), use wg.Go(func()) to spawn a goroutine and automate the Add/Done lifecycle. Replace manual patterns like wg.Add(1); go func(){ defer wg.Done(); ... }() with wg.Go(func(){ ... }). Ensure the codebase builds with Go 1.25+ and apply this in relevant Go files (e.g., internal/logstream/hub_test.go). If targeting older Go versions, maintain the existing pattern.
Applied to files:
internal/api/handlers/crossseed.gointernal/services/crossseed/service_local_matches_test.gointernal/services/crossseed/service.go
📚 Learning: 2025-11-06T11:59:21.390Z
Learnt from: Audionut
Repo: autobrr/qui PR: 553
File: web/src/components/torrents/TorrentTableOptimized.tsx:1510-1515
Timestamp: 2025-11-06T11:59:21.390Z
Learning: In the qui project, the API layer in web/src/lib/api.ts normalizes backend snake_case responses to camelCase for frontend consumption. For CrossSeed search results, the backend's download_url field is transformed to downloadUrl in the searchCrossSeedTorrent method, so frontend code should always use the camelCase variant (result.downloadUrl).
Applied to files:
web/src/hooks/useCrossSeedWarning.tsweb/src/lib/api.ts
📚 Learning: 2025-11-25T22:46:03.762Z
Learnt from: s0up4200
Repo: autobrr/qui PR: 632
File: internal/backups/service.go:1401-1404
Timestamp: 2025-11-25T22:46:03.762Z
Learning: In qui's backup service (internal/backups/service.go), background torrent downloads initiated during manifest import intentionally use a fire-and-forget pattern with the shared service context (s.ctx). Per-run cancellation is not needed, as orphaned downloads completing after run deletion are considered harmless and acceptable. This design prioritizes simplicity over per-run lifecycle management for background downloads.
Applied to files:
internal/services/crossseed/service.go
🧬 Code graph analysis (2)
internal/services/crossseed/service_local_matches_test.go (4)
internal/services/crossseed/service.go (1)
Service(252-321)internal/services/crossseed/release_cache.go (1)
NewReleaseCache(13-15)internal/qbittorrent/sync_manager.go (2)
CrossInstanceTorrentView(99-103)TorrentView(93-96)pkg/stringutils/normalize.go (1)
NewDefaultNormalizer(35-37)
internal/services/crossseed/service.go (1)
internal/services/crossseed/models.go (2)
LocalMatchesResponse(293-295)LocalMatch(298-313)
🔇 Additional comments (11)
internal/services/crossseed/service.go (2)
426-487: Strict mode error propagation inFindLocalMatcheslooks correctThe strict flag behavior (bubbling up any source/candidate file‑overlap error vs logging and returning partial results) is well aligned with the delete‑dialog use case, and the separation between structural errors and best‑effort mode is clear and robust.
595-637: Ambiguouscontent_pathhandling via file-overlap threshold is well designedThe new logic that treats
content_path == save_pathas ambiguous and then gatesmatchTypeContentPathoncandidateSharesSourceFiles(≥90% of the smaller torrent by name+size) should eliminate the noisy “same directory” false positives without regressing clear directory matches. The overlap helper correctly uses integer math, shared key normalization, and treats empty file lists as hard errors so strict mode can fail safely instead of silently reporting “no cross‑seeds found”.Also applies to: 656-701
internal/services/crossseed/service_local_matches_test.go (5)
18-50: Rootless storage-dir test correctly guards against false positivesThis test nicely encodes the “ambiguous root directory + no file-level data ⇒ no match” behavior by passing a nil
matchCtxand asserting an emptymatchType, which should prevent spurious cross-seed matches on/downloads-style roots. No issues here.
52-83: Non-ambiguous content_path test is well targetedUsing a specific
content_path(file path rather than directory) with a nilmatchCtxto assertmatchTypeContentPathkeeps the fast-path behavior for clear cases while reserving strict overlap checks for ambiguous ones. The assertions look correct and aligned with the intended semantics.
168-350: Ambiguous-directory overlap tests clearly capture 0%, 100%, and sub-threshold casesThe three
TestDetermineLocalMatchType_AmbiguousDir_*cases:
- Disjoint files → no
content_pathmatch.- Identical file lists →
matchTypeContentPath.- 10% shared bytes (100MB of 1GB) → no match.
These are well chosen to pin down the ≥90% overlap invariant for ambiguous directories and to exercise the lazy
localMatchContextpath. The construction ofsourceHash/candidateHash, normalized file maps, andmatchCtxlooks sound.
352-427: candidateSharesSourceFiles tests correctly cover equality, disjoint, and “episode-in-pack” semanticsThe three
TestCandidateSharesSourceFiles_*tests validate:
- Exact key equality (
name|size) → shares.- Disjoint keys → does not share.
- A single-episode candidate fully contained in a season-pack source (using lowercased keys) → shares based on 100% of the smaller torrent.
This gives good confidence that the overlap logic respects both total-byte ratios and the “100% of the smaller side” rule for episode-in-pack scenarios. No issues spotted.
429-592: Strict error handling for empty/missing file lists is well specifiedThe tests around
getSourceFilesanddetermineLocalMatchTypeerror paths are strong:
TestGetSourceFiles_EmptyFileListReturnsErrorasserts an explicit error, zero totals, nil keys, and storage onmatchCtx.sourceFilesErr, which avoids silent false negatives.TestDetermineLocalMatchType_EmptyCandidateFiles_StoresErrorandTestDetermineLocalMatchType_CandidateFetchError_StoresErrorboth ensure nocontent_pathmatch is emitted when candidate metadata is bad, and thatcandidateFilesErrcaptures the underlying cause for strict-mode bubbling.This behavior directly supports the PR goal of avoiding misleading delete warnings when overlap checks can’t complete. The expectations and error-message substring checks look appropriate.
web/src/hooks/useCrossSeedWarning.ts (1)
86-89: Strict mode wiring for delete warnings is appropriatePassing
strict = trueintoapi.getLocalCrossSeedMatchesin this hook is consistent with its delete-warning responsibility: the dialog should fail (and surface"error"state) rather than silently proceed on partial overlap data. The added comment clearly documents this intent. No further changes needed here.web/src/lib/api.ts (3)
52-52: LGTM!The type import is correctly added and required for the method return type.
774-780: Well-documented API enhancement with backward compatibility.The addition of the
strictparameter with a default value offalseensures existing callers continue to work without modification, while the clear documentation explains when strict mode should be enabled.
803-805: LGTM!The query parameter construction is clean and correct. The conditional approach ensures the
strict=trueparameter is only added when needed, keeping URLs clean for the default case.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/services/crossseed/service.go (1)
571-620: Ambiguouscontent_path == save_pathhandling and overlap check look correct; consider tiny diagnostics tweak
localMatchContext.getSourceFilesandcandidateSharesSourceFilescorrectly treat empty file lists as hard errors (for strict mode) and use a consistentpath|sizekey, so the 90% “smaller total size” overlap threshold should reliably distinguish true directory‑sharing from mere shared download roots.determineLocalMatchTypepreserves the old fast path for non‑ambiguous content paths while only invoking the heavier overlap logic when either side hascontent_path == save_path, which addresses the false positives this PR targets without regressing other match strategies.If you ever need to debug why ambiguous dir matches don’t promote to
content_pathmatches, a low‑volume debug log branch in the “no overlap / fallthrough” path (e.g., whensharesFiles == false) could be useful, but not required.Also applies to: 624-669, 687-732
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/api/handlers/crossseed.gointernal/services/crossseed/service.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/api/handlers/crossseed.go
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: s0up4200
Repo: autobrr/qui PR: 641
File: internal/services/crossseed/service.go:209-212
Timestamp: 2025-11-28T20:32:30.126Z
Learning: Repo: autobrr/qui PR: 641
File: internal/services/crossseed/service.go
Learning: The cross-seed recheck-resume worker intentionally runs for the process lifetime and keys pending entries by hash only. This is acceptable under the current constraint that background seeded-search runs operate on a single instance at a time; graceful shutdown and instanceID|hash keying are deferred by design.
📚 Learning: 2025-11-28T20:32:30.126Z
Learnt from: s0up4200
Repo: autobrr/qui PR: 641
File: internal/services/crossseed/service.go:209-212
Timestamp: 2025-11-28T20:32:30.126Z
Learning: Repo: autobrr/qui PR: 641
File: internal/services/crossseed/service.go
Learning: The cross-seed recheck-resume worker intentionally runs for the process lifetime and keys pending entries by hash only. This is acceptable under the current constraint that background seeded-search runs operate on a single instance at a time; graceful shutdown and instanceID|hash keying are deferred by design.
Applied to files:
internal/services/crossseed/service.go
📚 Learning: 2025-11-28T22:21:20.730Z
Learnt from: s0up4200
Repo: autobrr/qui PR: 641
File: internal/services/crossseed/service.go:2415-2457
Timestamp: 2025-11-28T22:21:20.730Z
Learning: Repo: autobrr/qui PR: 641
File: internal/services/crossseed/service.go
Learning: The determineSavePath function intentionally includes a contentLayout string parameter for future/content-layout branching and API consistency. Its presence is by design even if unused in the current body; do not flag as an issue in reviews.
Applied to files:
internal/services/crossseed/service.go
📚 Learning: 2025-11-25T22:46:03.762Z
Learnt from: s0up4200
Repo: autobrr/qui PR: 632
File: internal/backups/service.go:1401-1404
Timestamp: 2025-11-25T22:46:03.762Z
Learning: In qui's backup service (internal/backups/service.go), background torrent downloads initiated during manifest import intentionally use a fire-and-forget pattern with the shared service context (s.ctx). Per-run cancellation is not needed, as orphaned downloads completing after run deletion are considered harmless and acceptable. This design prioritizes simplicity over per-run lifecycle management for background downloads.
Applied to files:
internal/services/crossseed/service.go
📚 Learning: 2025-12-28T18:44:10.496Z
Learnt from: s0up4200
Repo: autobrr/qui PR: 876
File: internal/logstream/hub_test.go:188-192
Timestamp: 2025-12-28T18:44:10.496Z
Learning: In Go 1.25 (Aug 2025), use wg.Go(func()) to spawn a goroutine and automate the Add/Done lifecycle. Replace manual patterns like wg.Add(1); go func(){ defer wg.Done(); ... }() with wg.Go(func(){ ... }). Ensure the codebase builds with Go 1.25+ and apply this in relevant Go files (e.g., internal/logstream/hub_test.go). If targeting older Go versions, maintain the existing pattern.
Applied to files:
internal/services/crossseed/service.go
🔇 Additional comments (2)
internal/services/crossseed/service.go (2)
426-487: Strict vs best‑effort local match behavior matches the intended UXThe new
strictflag, lazylocalMatchContext, and aggregation ofsourceFilesErr/candidateFilesErrcleanly implement “fail safe on any overlap‑check error” for delete dialogs while keeping the existing best‑effort semantics (warn + partial results) for other callers. The logging context (instanceID + normalized hash) is also sufficient for debugging. No functional issues here.
489-569: Refactor intocollectLocalMatches/matchTorrentsInInstance/newLocalMatchis soundThe per‑instance iteration and early
ctx.Err()checks keep behavior predictable and avoid unnecessary work, and centralizingLocalMatchconstruction innewLocalMatchreduces duplication. PassingnormalizedSourceHashdown avoids repeated normalization. This refactor is a net readability win with no apparent behavioral regressions.
Local cross-seed detection now verifies file overlap for ambiguous
content_path == save_pathcases. Addsstrict=trueto/cross-seed/torrents/{instanceID}/{hash}/local-matchesfor delete dialogs to fail safe on overlap check errors, with best-effort mode elsewhere. OpenAPI updated.Solves #1145
Summary by CodeRabbit
New Features
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.