fix(cross-seed): defer completion search while checking#1588
Open
fix(cross-seed): defer completion search while checking#1588
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdds injectable per-service completion polling timings and a per-instance polling/queueing mechanism that gates cross-seed completion searches until torrents leave checking states or time out; includes readiness helpers, deduped wait lanes, and expanded tests with a QBittorrent polling mock. Changes
Sequence DiagramsequenceDiagram
participant Handler as "HandleTorrentCompletion"
participant Waiter as "waitForCompletionTorrentReady"
participant Lane as "completionLane (poller)"
participant Getter as "getCompletionTorrent"
participant QBit as "QBittorrent API"
participant Search as "executeCompletionSearch"
Handler->>Waiter: waitForCompletionTorrentReady(ctx, instanceID, torrent)
Waiter->>Lane: register wait / ensure poller running
loop Poll until ready or timeout
Lane->>Getter: getCompletionTorrent(ctx, instanceID, hash)
Getter->>QBit: GetTorrents(ctx, hash)
QBit-->>Getter: torrent details
Getter-->>Lane: torrent snapshot
alt isCompletionCheckingState(snapshot)
Lane->>Lane: wait (poll interval) and retry
else ready or terminal state
Lane-->>Waiter: ready torrent or error
end
end
Handler->>Search: executeCompletionSearch(torrent)
Search-->>Handler: completion results
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)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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_completion_queue_test.go`:
- Around line 169-180: The test helper setCompletionCheckingTimings must stop
mutating package globals completionCheckingPollInterval and
completionCheckingTimeout; instead, add injectable duration fields (e.g.,
completionPollInterval and completionTimeout) to the Service or to the helper
used by waitForCompletionTorrentReady/HandleTorrentCompletion, wire those fields
through the Service constructor or helper call in tests, update
waitForCompletionTorrentReady and HandleTorrentCompletion to read durations from
the Service/helper instance rather than package-level variables, and update
tests to pass custom timings per-Service so no global state is changed or shared
between parallel tests.
In `@internal/services/crossseed/service.go`:
- Around line 1505-1566: In waitForCompletionTorrentReady, don't abort the whole
wait on a transient getCompletionTorrent error: inside the ticker.C case where
current, err = s.getCompletionTorrent(...) is called, replace the immediate
return on error with a logged warning (include instanceID, hash/name, and err)
and continue the loop so retries occur until completionCheckingTimeout; leave
the initial pre-loop getCompletionTorrent error handling unchanged but ensure
the retry-path does not return err on a single API hiccup.
- Around line 1467-1479: The code uses the stale event payload for skip checks
before calling waitForCompletionTorrentReady, so after
waitForCompletionTorrentReady returns you must re-run the same guard/filter
logic against the refreshed torrent to avoid searching on a torrent that
gained/removed the cross-seed tag or moved into an excluded category;
specifically, after waitForCompletionTorrentReady returns (readyTorrent) and
before calling executeCompletionSearchWithRetry, refresh/re-evaluate the skip
conditions (the same checks that previously decided to skip) using readyTorrent
(and/or by reloading any settings used in those checks) and return early if the
torrent no longer qualifies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4d63b694-1376-483c-8875-1d2522c863b5
📒 Files selected for processing (2)
internal/services/crossseed/service.gointernal/services/crossseed/service_completion_queue_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 1593-1601: The loop that calls s.getCompletionTorrent and retries
on error should stop polling immediately when getCompletionTorrent returns
ErrTorrentNotFound; modify the error handling around the getCompletionTorrent
call (the block handling current, err = s.getCompletionTorrent(...)) to check
for errors.Is(err, ErrTorrentNotFound) and in that case stop the polling for
this instance (e.g., break out / return / continue the outer workflow instead of
continuing the retry loop), logging a clear message with instanceID,
eventTorrent.Hash and eventTorrent.Name; leave other error cases to continue
retrying until completionTimeout as before.
- Around line 1444-1448: The code currently holds lane.mu across
waitForCompletionTorrentReady, which can block other completions; move the call
to waitForCompletionTorrentReady(ctx, instanceID, torrent) to before acquiring
the per-instance lane lock from getCompletionLane(instanceID), then only lock
lane.mu for the final revalidation and search steps; specifically, call
readyTorrent, err := s.waitForCompletionTorrentReady(...) first, then lane :=
s.getCompletionLane(instanceID); lane.mu.Lock()/defer lane.mu.Unlock() and
perform the revalidation + search using readyTorrent and torrent, ensuring any
necessary state is re-read under the lock to avoid races.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 07f36feb-9e79-4ab3-8e6c-7cf85380f908
📒 Files selected for processing (2)
internal/services/crossseed/service.gointernal/services/crossseed/service_completion_queue_test.go
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.
Summary
Rationale
Commit
5a3114be6902b40d5760fdbd0acefa5b61628a8b(fix(qbittorrent): stop reboot torrent_completed spam) changed qBittorrent completion detection to useCompletionOn > 0instead of requiringprogressplus a non-checking state. That made completion events arrive while some torrents were still incheckingResumeData, which then hit the older cross-seedprogress < 1.0guard and logged[CROSSSEED-COMPLETION] Failed to execute completion search.This change keeps the existing completeness requirement for completion searches, but defers execution while qBittorrent is still checking so the completion path matches the newer event timing.
Because the fix now waits by polling qBittorrent, it also tolerates transient refresh failures and re-checks skip conditions/settings after checking completes so completion searches still use current torrent state.
Testing
go test ./internal/services/crossseed ./internal/qbittorrentSummary by CodeRabbit
New Features
Bug Fixes
Tests