feat(crossseed): finalize webhook check validation#1597
feat(crossseed): finalize webhook check validation#1597
Conversation
WalkthroughThe PR switches the cross-seed webhook check to accept base64-encoded torrent bytes (torrentData) and executes a full, non-mutating cross-seed validation using that payload; docs, OpenAPI, handler docs, service logic, models, and tests are updated to reflect the new payload and final/ready match semantics (200/202/404). Changes
Sequence Diagram(s)sequenceDiagram
participant Autobrr
participant API as "autobrr API\n/webhook/check"
participant Service as "cross-seed\nservice"
participant QB as "qBittorrent\ninstances"
Autobrr->>API: POST /api/cross-seed/webhook/check\n{ torrentData (base64) }
API->>Service: prepareCrossSeedEvaluation(torrentBytes)
Service->>Service: parse torrent bytes -> metadata
Service->>QB: query instances for candidates & files
QB-->>Service: candidate torrents, file lists, properties
Service->>Service: validate candidates (preflight, scoring)
Service-->>API: evaluation result (CanCrossSeed, Matches, Recommendation)
API-->>Autobrr: 200 / 202 / 404 with final validation summary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
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)
internal/services/crossseed/service.go (1)
9879-9939:⚠️ Potential issue | 🟠 MajorReuse the apply preflight before setting the webhook verdict.
Line 9881 only runs file-level candidate validation. This path never executes the same non-mutating gates that
processCrossSeedCandidateapplies beforeAddTorrentat Lines 3531-3576 and Lines 3750-4052, soCheckWebhookcan still returncanCrossSeed=true/recommendation=downloadfor torrents the real apply path would immediately markexists,blocked,skipped_recheck, orno_save_path. Please base the webhook verdict on a shared side-effect-free preflight instead of file matching alone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/crossseed/service.go` around lines 9879 - 9939, The webhook path currently only runs file-level validation and misses the same non-mutating gates used by processCrossSeedCandidate before AddTorrent, so extract the side-effect-free preflight logic into a shared function (e.g., preflightCandidateChecks or reuse an existing preflight helper) that reproduces the non-mutating gates (exists, blocked, skipped_recheck, no_save_path, etc.) used by processCrossSeedCandidate, have processCrossSeedCandidate call that helper before AddTorrent, and call the same helper from the CheckWebhook path (the loop building matches) to factor its result into canCrossSeed/recommendation so the webhook verdict reflects the same preflight decisions as the real apply path. Ensure the helper returns a structured result (status enum/reason) and is pure (no side effects) so the webhook uses it without mutating state.
🧹 Nitpick comments (2)
internal/services/crossseed/crossseed_test.go (1)
2082-2082: Avoid leaving permanently skipped legacy webhook tests in-place.These
t.Skip(...)calls make large test bodies unreachable, which can drift and hide coverage gaps over time. Since file-aware replacements now exist, prefer deleting these legacy bodies (or fully migrating them to torrentData fixtures) to keep test signal clean.Based on learnings: For changes under
internal/services/crossseedorinternal/qbittorrent, run targeted package tests first, then run fullmake testsuite.Also applies to: 2348-2348, 2411-2411, 2460-2460, 2854-2854, 4329-4329
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/crossseed/crossseed_test.go` at line 2082, Remove the permanently skipped legacy webhook tests by deleting the t.Skip("metadata-only webhook tests replaced by file-aware webhook tests") calls and their now-unreachable test bodies in the crossseed test file, and either delete those legacy test functions entirely or fully migrate them to use the new file-aware torrentData fixtures; search for the exact t.Skip string elsewhere in the package (the other occurrences referenced) and apply the same deletion/migration so tests remain reachable and coverage isn't hidden.internal/services/crossseed/webhook_check_test.go (1)
91-96: Optional: consider a shared test builder forServicesetup.
Serviceconstruction is repeated across several tests; a small helper could reduce boilerplate and make scenario intent even clearer.Also applies to: 176-182, 228-233, 250-271
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/crossseed/webhook_check_test.go` around lines 91 - 96, Several tests repeatedly construct a Service with the same boilerplate (instanceStore, syncManager via newFakeSyncManager, NewReleaseCache, stringNormalizer via stringutils.NewDefaultNormalizer); create a shared test helper (e.g., newTestService or serviceBuilder) that accepts varying inputs (instance, torrents, candidateFiles, optional store) and returns a configured *Service to reduce duplication and clarify intent; update tests at the shown call sites (and the ones noted at lines 176-182, 228-233, 250-271) to call this helper instead of repeating the struct literal so future changes to Service initialization stay in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/services/crossseed/service.go`:
- Around line 3119-3125: The refetch currently switches to qbt.TorrentFilterAll
when req.IncludeIncompleteCandidates is true, which pulls in
errored/missingFiles torrents; change the filter selection to broaden only to
valid in-progress candidates (e.g., use qbt.TorrentFilterInProgress or the
equivalent filter that excludes errored/missingFiles) instead of
qbt.TorrentFilterAll in the call to s.syncManager.GetTorrents, and apply the
same fix to the other similar block (the second refetch around lines 3142-3150);
leave shouldSkipErroredTorrent behavior unchanged so errored torrents are not
included in incomplete-candidate scans.
---
Outside diff comments:
In `@internal/services/crossseed/service.go`:
- Around line 9879-9939: The webhook path currently only runs file-level
validation and misses the same non-mutating gates used by
processCrossSeedCandidate before AddTorrent, so extract the side-effect-free
preflight logic into a shared function (e.g., preflightCandidateChecks or reuse
an existing preflight helper) that reproduces the non-mutating gates (exists,
blocked, skipped_recheck, no_save_path, etc.) used by processCrossSeedCandidate,
have processCrossSeedCandidate call that helper before AddTorrent, and call the
same helper from the CheckWebhook path (the loop building matches) to factor its
result into canCrossSeed/recommendation so the webhook verdict reflects the same
preflight decisions as the real apply path. Ensure the helper returns a
structured result (status enum/reason) and is pure (no side effects) so the
webhook uses it without mutating state.
---
Nitpick comments:
In `@internal/services/crossseed/crossseed_test.go`:
- Line 2082: Remove the permanently skipped legacy webhook tests by deleting the
t.Skip("metadata-only webhook tests replaced by file-aware webhook tests") calls
and their now-unreachable test bodies in the crossseed test file, and either
delete those legacy test functions entirely or fully migrate them to use the new
file-aware torrentData fixtures; search for the exact t.Skip string elsewhere in
the package (the other occurrences referenced) and apply the same
deletion/migration so tests remain reachable and coverage isn't hidden.
In `@internal/services/crossseed/webhook_check_test.go`:
- Around line 91-96: Several tests repeatedly construct a Service with the same
boilerplate (instanceStore, syncManager via newFakeSyncManager, NewReleaseCache,
stringNormalizer via stringutils.NewDefaultNormalizer); create a shared test
helper (e.g., newTestService or serviceBuilder) that accepts varying inputs
(instance, torrents, candidateFiles, optional store) and returns a configured
*Service to reduce duplication and clarify intent; update tests at the shown
call sites (and the ones noted at lines 176-182, 228-233, 250-271) to call this
helper instead of repeating the struct literal so future changes to Service
initialization stay in one place.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6826f952-7b59-4ab6-975e-5bb18c2d81cf
📒 Files selected for processing (10)
documentation/docs/features/cross-seed/autobrr.mddocumentation/docs/features/cross-seed/troubleshooting.mddocumentation/static/openapi.yamlinternal/api/handlers/crossseed.gointernal/api/handlers/crossseed_webhook_handler_test.gointernal/services/crossseed/crossseed_test.gointernal/services/crossseed/models.gointernal/services/crossseed/service.gointernal/services/crossseed/webhook_check_test.gointernal/web/swagger/openapi.yaml
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
internal/services/crossseed/crossseed_test.go (1)
3378-3394: Prefer empty-batch semantics here.The real batch file lookup treats missing entries as partial/empty results. Returning
not implementedwhenm.filesis empty pushes callers down an error path that production does not take.♻️ Proposed fix
func (m *mockRecoverSyncManager) GetTorrentFilesBatch(_ context.Context, _ int, hashes []string) (map[string]qbt.TorrentFiles, error) { - if len(m.files) == 0 { - return nil, errors.New("not implemented") - } - result := make(map[string]qbt.TorrentFiles, len(hashes)) + if len(m.files) == 0 { + return result, nil + } for _, hash := range hashes { files, ok := m.files[normalizeHash(hash)] if !ok { continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/crossseed/crossseed_test.go` around lines 3378 - 3394, The mockRecoverSyncManager/GetTorrentFilesBatch currently returns an error when m.files is empty, which differs from production semantics; change the function to not return errors for an empty m.files and instead return an empty map (map[string]qbt.TorrentFiles{}) so missing entries are treated as partial/empty results; keep the existing loop behavior (normalizeHash, copying files into result) but drop the early "not implemented" error path and ensure the function returns (result, nil) even when no hashes are found.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/services/crossseed/crossseed_test.go`:
- Around line 3667-3700: The test is passing due to missing file fixtures for
two hashes; update the test around svc.FindCandidates to seed mockSync.files
with entries for all four hashes (e.g., normalizeHash("complete"), "pending",
"errored", "missing") so file lookups return data for each hash, then assert
which hashes survive after the refetch logic (or alternatively rename the test
to reflect the current behavior). Locate and modify mockSync.files in the test,
and update the assertions on resp.Candidates[*].Torrents and mockSync.filters
(or expectations) to match the intended behavior when recheckCompletes = false
and TorrentFilterDownloading may resume errored/missing torrents.
In `@internal/services/crossseed/service.go`:
- Around line 3566-3579: processCrossSeedCandidate currently passes nil for
settings into preflightCandidateChecks causing preflightCandidateChecks to call
GetAutomationSettings repeatedly (hot spot: preflightCandidateChecks and
GetAutomationSettings); change the call chain so the already-loaded automation
settings are threaded into preflightCandidateChecks from
processCrossSeedCandidate (and any intermediate callers) and used directly
inside preflightCandidateChecks, keeping the existing lazy GetAutomationSettings
call only as a fallback when no settings were provided; update the signatures of
preflightCandidateChecks (and any helper functions it calls) to accept a
settings parameter and use it instead of always fetching settings.
- Around line 5130-5132: The required-check uses raw equality on req.TorrentData
which lets whitespace-only strings pass; update the validation in the handler
that checks req.TorrentData (the block with if req.TorrentData == "") to trim
whitespace first (e.g., use strings.TrimSpace on req.TorrentData or assign a
trimmed variable) and then check emptiness, returning the same
errors.New("torrent_data is required") when the trimmed value is empty so
whitespace-only payloads yield the proper required validation error.
- Around line 3131-3133: Replace the qbt.TorrentFilterDownloading argument to
s.syncManager.GetTorrents with qbt.TorrentFilterAll (in the call that sets
incompleteTorrents) so the client returns paused/queued/stalled incomplete
torrents, then client-side filter the resulting incompleteTorrents slice by
Progress < 1.0 and exclude any entries whose State indicates an
error/missing-files condition before using them for webhook matching; update any
logic that expects only downloading states (e.g., the loop that checks Progress
and builds candidates) to operate on the filtered list instead of relying on the
API to pre-filter.
---
Nitpick comments:
In `@internal/services/crossseed/crossseed_test.go`:
- Around line 3378-3394: The mockRecoverSyncManager/GetTorrentFilesBatch
currently returns an error when m.files is empty, which differs from production
semantics; change the function to not return errors for an empty m.files and
instead return an empty map (map[string]qbt.TorrentFiles{}) so missing entries
are treated as partial/empty results; keep the existing loop behavior
(normalizeHash, copying files into result) but drop the early "not implemented"
error path and ensure the function returns (result, nil) even when no hashes are
found.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e87b3842-93f7-4f8c-9950-6431e42e274a
📒 Files selected for processing (4)
internal/api/handlers/crossseed_webhook_handler_test.gointernal/services/crossseed/crossseed_test.gointernal/services/crossseed/service.gointernal/services/crossseed/webhook_check_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/api/handlers/crossseed_webhook_handler_test.go
| mockSync.files = map[string]qbt.TorrentFiles{ | ||
| normalizeHash("complete"): {{Name: "Movie.2025.1080p.BluRay.x264-GROUP.mkv", Size: 1}}, | ||
| normalizeHash("pending"): {{Name: "Movie.2025.1080p.BluRay.x264-GROUP.mkv", Size: 1}}, | ||
| } | ||
|
|
||
| svc := &Service{ | ||
| instanceStore: &fakeInstanceStore{ | ||
| instances: map[int]*models.Instance{ | ||
| instance.ID: instance, | ||
| }, | ||
| }, | ||
| syncManager: mockSync, | ||
| recoverErroredTorrentsEnabled: true, | ||
| releaseCache: NewReleaseCache(), | ||
| stringNormalizer: stringutils.NewDefaultNormalizer(), | ||
| } | ||
|
|
||
| resp, err := svc.FindCandidates(context.Background(), req) | ||
| require.NoError(t, err) | ||
| require.Len(t, resp.Candidates, 1) | ||
| require.Len(t, resp.Candidates[0].Torrents, 2) | ||
|
|
||
| hashes := []string{ | ||
| resp.Candidates[0].Torrents[0].Hash, | ||
| resp.Candidates[0].Torrents[1].Hash, | ||
| } | ||
| assert.ElementsMatch(t, []string{"complete", "pending"}, hashes) | ||
| require.GreaterOrEqual(t, len(mockSync.filters), 3) | ||
| assert.Equal(t, qbt.TorrentFilterAll, mockSync.filters[0]) | ||
| assert.Equal(t, []qbt.TorrentFilter{ | ||
| qbt.TorrentFilterCompleted, | ||
| qbt.TorrentFilterDownloading, | ||
| }, mockSync.filters[len(mockSync.filters)-2:]) | ||
| } |
There was a problem hiding this comment.
This regression test currently passes because two hashes have no file fixtures, not because the refetch excluded them.
With recheckCompletes = false, the mock still resumes errored and missing, so they can come back through the TorrentFilterDownloading fetch. Right now only complete and pending exist in mockSync.files, so the extra hashes fall out later during file lookup. If you want this test to lock in the refetch behavior, seed file data for all four hashes and assert which ones survive, or rename the test to match the current missing-file behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/services/crossseed/crossseed_test.go` around lines 3667 - 3700, The
test is passing due to missing file fixtures for two hashes; update the test
around svc.FindCandidates to seed mockSync.files with entries for all four
hashes (e.g., normalizeHash("complete"), "pending", "errored", "missing") so
file lookups return data for each hash, then assert which hashes survive after
the refetch logic (or alternatively rename the test to reflect the current
behavior). Locate and modify mockSync.files in the test, and update the
assertions on resp.Candidates[*].Torrents and mockSync.filters (or expectations)
to match the intended behavior when recheckCompletes = false and
TorrentFilterDownloading may resume errored/missing torrents.
| incompleteTorrents, incompleteErr := s.syncManager.GetTorrents(ctx, instanceID, qbt.TorrentFilterOptions{ | ||
| Filter: qbt.TorrentFilterDownloading, | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In github.com/autobrr/go-qbittorrent, what states are included in TorrentFilterDownloading? Does it include paused/inactive incomplete torrents, and which filter should be used to include all incomplete torrents while excluding errored/missing files?
💡 Result:
In github.com/autobrr/go-qbittorrent, TorrentFilterDownloading matches these TorrentStates: allocating, downloading, metaDL, checkingDL, forcedDL. [1]
It does not include paused/inactive-but-incomplete torrents such as pausedDL / stoppedDL (and it also excludes queuedDL and stalledDL, which are treated separately). [1]
To include all incomplete torrents while excluding errored / missing files, there isn’t a single built-in TorrentFilter that expresses “incomplete regardless of paused/stalled/queued” (filters are state-based, and errors/missing-files are states too). The practical approach is:
- query with
TorrentFilterAll, then - client-side filter to
progress < 1.0(oramount_left > 0), and exclude torrents whosestateiserrorormissingFiles. [2]
Sources
[1] filter.go state→filter mapping in autobrr/go-qbittorrent. (github.com)
[2] qBittorrent WebUI API: allowed filter values and state values (error, missingFiles, etc.). (github.com)
Citations:
- 1: https://github.com/autobrr/go-qbittorrent/blob/main/filter.go
- 2: https://github.com/qbittorrent/qBittorrent/wiki/WebUI-API-%28qBittorrent-5.0%29
🏁 Script executed:
# Check if the file exists and get its size
wc -l internal/services/crossseed/service.goRepository: autobrr/qui
Length of output: 99
🏁 Script executed:
# Check the specific lines mentioned: 3131-3133 and 3177-3180
sed -n '3130,3135p' internal/services/crossseed/service.go
echo "---"
sed -n '3176,3181p' internal/services/crossseed/service.goRepository: autobrr/qui
Length of output: 598
🏁 Script executed:
# Get more context around these lines to understand the webhook check purpose
sed -n '3115,3145p' internal/services/crossseed/service.go
echo "---"
sed -n '3160,3190p' internal/services/crossseed/service.goRepository: autobrr/qui
Length of output: 2365
🏁 Script executed:
# Find the shouldSkipErroredTorrent function
rg "func.*shouldSkipErroredTorrent" internal/services/crossseed/service.go -A 10Repository: autobrr/qui
Length of output: 426
TorrentFilterDownloading excludes paused/stalled incomplete torrents, causing webhook checks to miss valid candidates
At lines 3131-3133 and 3177-3180, TorrentFilterDownloading only includes allocating, downloading, metaDL, checkingDL, and forcedDL states—excluding pausedDL, stoppedDL, queuedDL, and stalledDL. Paused or queued incomplete torrents matching a cross-seed result will be skipped by the API-level filter and cannot be recovered by the subsequent Progress < 1.0 check, causing false "no match" outcomes in webhook dry-runs.
Use TorrentFilterAll instead and filter client-side for Progress < 1.0, excluding torrents with error or missing-files states.
Suggested adjustment
- incompleteTorrents, incompleteErr := s.syncManager.GetTorrents(ctx, instanceID, qbt.TorrentFilterOptions{
- Filter: qbt.TorrentFilterDownloading,
- })
+ incompleteTorrents, incompleteErr := s.syncManager.GetTorrents(ctx, instanceID, qbt.TorrentFilterOptions{
+ Filter: qbt.TorrentFilterAll,
+ })Then filter after the loop to only include incomplete torrents that aren't errored:
for _, torrent := range incompleteTorrents {
+ if torrent.Progress >= 1.0 {
+ continue
+ }
+ if torrent.State == qbt.TorrentStateError || torrent.State == qbt.TorrentStateMissingFiles {
+ continue
+ }
hashKey := normalizeHash(torrent.Hash)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/services/crossseed/service.go` around lines 3131 - 3133, Replace the
qbt.TorrentFilterDownloading argument to s.syncManager.GetTorrents with
qbt.TorrentFilterAll (in the call that sets incompleteTorrents) so the client
returns paused/queued/stalled incomplete torrents, then client-side filter the
resulting incompleteTorrents slice by Progress < 1.0 and exclude any entries
whose State indicates an error/missing-files condition before using them for
webhook matching; update any logic that expects only downloading states (e.g.,
the loop that checks Progress and builds candidates) to operate on the filtered
list instead of relying on the API to pre-filter.
| preflight, err := s.preflightCandidateChecks( | ||
| ctx, | ||
| candidate, | ||
| validation, | ||
| rejectReason, | ||
| torrentHash, | ||
| torrentHashV2, | ||
| torrentName, | ||
| req, | ||
| sourceRelease, | ||
| sourceFiles, | ||
| torrentInfo, | ||
| nil, | ||
| ) |
There was a problem hiding this comment.
Avoid per-candidate settings loads in preflight
processCrossSeedCandidate passes nil settings to preflightCandidateChecks (Line 3578), which then calls GetAutomationSettings internally (Line 5038). In multi-candidate runs this repeats the same settings lookup many times and adds avoidable latency/load.
Please pass already-loaded settings through the call chain and reserve lazy-loading as fallback only for callers that truly lack settings context.
Also applies to: 5037-5045
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/services/crossseed/service.go` around lines 3566 - 3579,
processCrossSeedCandidate currently passes nil for settings into
preflightCandidateChecks causing preflightCandidateChecks to call
GetAutomationSettings repeatedly (hot spot: preflightCandidateChecks and
GetAutomationSettings); change the call chain so the already-loaded automation
settings are threaded into preflightCandidateChecks from
processCrossSeedCandidate (and any intermediate callers) and used directly
inside preflightCandidateChecks, keeping the existing lazy GetAutomationSettings
call only as a fallback when no settings were provided; update the signatures of
preflightCandidateChecks (and any helper functions it calls) to accept a
settings parameter and use it instead of always fetching settings.
| if req.TorrentData == "" { | ||
| return nil, errors.New("torrent_data is required") | ||
| } |
There was a problem hiding this comment.
Trim before required-check for TorrentData
Line 5130 uses raw equality (req.TorrentData == ""). Whitespace-only payloads skip this guard and fail later as parse errors instead of a clear “required” validation error.
Small fix
- if req.TorrentData == "" {
+ if strings.TrimSpace(req.TorrentData) == "" {
return nil, errors.New("torrent_data is required")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if req.TorrentData == "" { | |
| return nil, errors.New("torrent_data is required") | |
| } | |
| if strings.TrimSpace(req.TorrentData) == "" { | |
| return nil, errors.New("torrent_data is required") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/services/crossseed/service.go` around lines 5130 - 5132, The
required-check uses raw equality on req.TorrentData which lets whitespace-only
strings pass; update the validation in the handler that checks req.TorrentData
(the block with if req.TorrentData == "") to trim whitespace first (e.g., use
strings.TrimSpace on req.TorrentData or assign a trimmed variable) and then
check emptiness, returning the same errors.New("torrent_data is required") when
the trimmed value is empty so whitespace-only payloads yield the proper required
validation error.
Summary by CodeRabbit
New Features
Documentation
Tests