feat(crossseed): add pooled partial completion#1594
Open
feat(crossseed): add pooled partial completion#1594
Conversation
Contributor
WalkthroughAdds pooled partial completion for cross-seed: DB schema and models for partial-pool members, service-level partial-pool orchestration and restoration, API/settings plumbing, frontend controls, startup recovery, and extensive tests across service, model, API, migration, and UI layers. Changes
Sequence DiagramsequenceDiagram
participant Ext as External Event (Torrent Added)
participant SVC as CrossSeed Service
participant DB as Partial Pool Store
participant QBT as QBittorrent Sync Manager
participant WRK as Background Worker
Ext->>SVC: HandleTorrentAdded(ctx, instanceID, torrent)
SVC->>DB: GetByAnyHash(target_hash / v2)
DB-->>SVC: existing member? / nil
SVC->>DB: Upsert member (if registering)
SVC->>WRK: wake worker / signal
WRK->>DB: ListActive(now)
DB-->>WRK: active members
WRK->>SVC: processPartialPools()
SVC->>QBT: GetTorrents(hashes) / GetTorrentFilesBatch
QBT-->>SVC: torrent metadata & files
SVC->>SVC: build pool state & select downloader
SVC->>QBT: propagate files (hardlink or reflink)
QBT-->>SVC: propagation result
SVC->>DB: Update member timestamps / DeleteExpired
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 |
![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: 10
🧹 Nitpick comments (7)
internal/services/crossseed/matching_layout_test.go (1)
134-149: Strengthen tie precondition to avoid false-positive passesThis test can pass even if candidates are not truly tied for reasons unrelated to cross-seed tagging. Consider asserting both candidates produce the same base match before calling
findBestCandidateMatch.Suggested test hardening
sourceRelease := rls.Release{} sourceFiles := qbt.TorrentFiles{{Name: "payload.bin", Size: 4 << 30}} candidate := CrossSeedCandidate{ @@ } + taggedMatch := svc.getMatchType( + &sourceRelease, + svc.releaseCache.Parse("Minimal.Payload"), + sourceFiles, + svc.syncManager.(*candidateSelectionSyncManager).files["tagged"], + ) + untaggedMatch := svc.getMatchType( + &sourceRelease, + svc.releaseCache.Parse("Minimal.Payload"), + sourceFiles, + svc.syncManager.(*candidateSelectionSyncManager).files["untagged"], + ) + require.Equal(t, taggedMatch, untaggedMatch, "test setup should isolate cross-seed tie-breaker") + require.Equal(t, "exact", untaggedMatch) + filesByHash := svc.batchLoadCandidateFiles(context.Background(), candidate.InstanceID, candidate.Torrents) bestTorrent, files, matchType, _ := svc.findBestCandidateMatch(context.Background(), candidate, &sourceRelease, sourceFiles, filesByHash, 5.0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/crossseed/matching_layout_test.go` around lines 134 - 149, The test currently assumes the two torrents are tied but doesn't verify they produce the same base match; before calling svc.findBestCandidateMatch, explicitly compute and assert that both candidate torrents yield the same base match/match score using the service helper that evaluates a single torrent against the source (e.g. a computeBaseMatch/score function or the existing internal matcher), by passing each torrent from candidate.Torrents together with sourceRelease, sourceFiles and filesByHash (obtained via svc.batchLoadCandidateFiles), and require that their base match types/scores are equal; this ensures the test only exercises tag-based tie-breaking in findBestCandidateMatch.internal/services/crossseed/rootless_savepath_test.go (1)
456-533: Reflink fallback test may not reliably trigger the fallback scenario.Unlike the hardlink test (lines 389-392) which explicitly creates a conflict file in
managedRootto force hardlink mode to fail, this reflink test doesn't create any conflict in the managed destination. The test appears to rely on implicit failure (e.g., filesystem lacking CoW support) rather than an explicit trigger.This makes the test behavior environment-dependent — it may pass for the wrong reason on filesystems that don't support reflinks, or fail unexpectedly on CoW-capable filesystems where reflink mode succeeds.
Consider creating an explicit conflict file similar to the hardlink test to ensure the fallback path is deterministically exercised:
♻️ Suggested fix to add explicit conflict
tempDir := t.TempDir() baseDir := filepath.Join(tempDir, "managed") oldRoot := filepath.Join(baseDir, "Old Tracker", pathutil.IsolationFolderName("oldhash12345678", torrentName)) + managedRoot := filepath.Join(baseDir, "New Tracker", pathutil.IsolationFolderName(newHash, torrentName)) expectedRoot := oldRoot require.NoError(t, os.MkdirAll(oldRoot, 0o755)) + require.NoError(t, os.MkdirAll(managedRoot, 0o755)) require.NoError(t, os.WriteFile(filepath.Join(oldRoot, "Movie.mkv"), []byte("movie"), 0o600)) + require.NoError(t, os.WriteFile(filepath.Join(managedRoot, "Movie.mkv"), []byte("conflict"), 0o600))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/crossseed/rootless_savepath_test.go` around lines 456 - 533, The test TestProcessCrossSeedCandidate_ReflinkFallbackKeepsMatchedSavePath is environment-dependent because it never creates a conflicting file in the managed destination to force reflink to fail; modify the test to explicitly create a conflicting file in the managed savepath (the same path that Service.processCrossSeedCandidate would attempt to place the reflinked file) before calling service.processCrossSeedCandidate so reflink mode deterministically errors and the code falls back to regular copy; mirror the approach used in the hardlink test by creating the conflict file (e.g., write a dummy file at the managed destination path) so sync.addedOptions["savepath"] ends up set to the matched save path as asserted.internal/services/crossseed/service_candidate_cache_test.go (1)
14-47: Prefer a table-driven structure for these cache scenariosBoth tests share the same setup and differ mainly in case-specific actions/assertions. Consolidating them into table-driven cases will reduce duplication and make new cache scenarios easier to add.
As per coding guidelines, "Prefer table-driven test cases in Go tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/crossseed/service_candidate_cache_test.go` around lines 14 - 47, Refactor the two tests TestCacheAutomationCandidateResponse_BoundedSize and TestCacheAutomationCandidateResponse_DuplicateKeyDoesNotGrowOrder into a single table-driven test that reuses the same setup (creating automationContext with candidateCache and candidateOrder) and runs per-case actions: one case that inserts automationCandidateCacheMaxEntries+25 distinct keys and asserts candidateCache length equals automationCandidateCacheMaxEntries and the oldest key ("release-0") is evicted, and another case that inserts the same key twice and asserts candidateCache length is 1 and candidateOrder length is 1 with "same-release" at index 0; implement cases using a slice of structs with name, inputs, and expected assertions, and call cacheAutomationCandidateResponse inside the loop to exercise the behavior while keeping assertions per-case.internal/models/crossseed_partial_pool_test.go (1)
1-2: Missing copyright header.Other test files in this PR include the full copyright header (e.g.,
// Copyright (c) 2025-2026, s0up and the autobrr contributors.). This file only has the SPDX license identifier.Add copyright header for consistency
+// Copyright (c) 2025-2026, s0up and the autobrr contributors. // SPDX-License-Identifier: GPL-2.0-or-later🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/models/crossseed_partial_pool_test.go` around lines 1 - 2, Add the missing copyright header to the top of the test file so it matches other tests: insert the full multi-line copyright comment used elsewhere (e.g., the line starting with "// Copyright (c) 2025-2026, s0up and the autobrr contributors.") followed by the existing SPDX identifier ("// SPDX-License-Identifier: GPL-2.0-or-later") at the very top of internal/models/crossseed_partial_pool_test.go to ensure consistency with other test files.internal/web/swagger/openapi.yaml (1)
6227-6231: Expose the 100 MiB default as schema metadata.
maxMissingBytesAfterRecheckdocuments the default in prose, but generated docs/clients will miss it unless the schema also setsdefault: 104857600.📝 Suggested schema tweak
maxMissingBytesAfterRecheck: type: integer format: int64 + default: 104857600 minimum: 1048576 description: Maximum missing bytes allowed after recheck for pooled reflink automation before the torrent is left paused for manual review. Minimum 1 MiB, default 100 MiB.Apply the same addition in both
CrossSeedAutomationSettingsPatchandCrossSeedAutomationSettings.Also applies to: 6335-6339
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/web/swagger/openapi.yaml` around lines 6227 - 6231, The OpenAPI schema for maxMissingBytesAfterRecheck only documents the 100 MiB default in text; update the schema by adding default: 104857600 to the maxMissingBytesAfterRecheck property in both CrossSeedAutomationSettingsPatch and CrossSeedAutomationSettings so generated docs/clients receive the default value, ensuring the integer/format and minimum remain unchanged.internal/services/crossseed/hardlink_mode_test.go (1)
983-985: Assertstoppedtogether withpausedhere.These checks currently only pin
paused, so they would miss a regression where the add options stop sending the pairedstoppedflag.Based on learnings: The autobrr/qui project uses a custom go-qbittorrent client library (github.com/autobrr/go-qbittorrent) that supports both "paused" and "stopped" parameters when adding torrents via the options map. Both parameters should be set together when controlling torrent start state, as seen in internal/services/crossseed/service.go and throughout the codebase.✅ Tighten the assertions
assert.Equal(t, managedRoot, syncManager.addOptions["savepath"]) assert.Equal(t, "true", syncManager.addOptions["paused"]) +assert.Equal(t, "true", syncManager.addOptions["stopped"])Apply the same assertion in each success-path test that inspects paused add options.
Also applies to: 1127-1128, 1211-1212
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/crossseed/hardlink_mode_test.go` around lines 983 - 985, The test currently asserts that syncManager.addOptions["paused"] is "true" but misses asserting the paired "stopped" flag; update the assertions to also check syncManager.addOptions["stopped"] == "true" wherever you assert paused (e.g., the block with assert.Equal(t, "true", syncManager.addOptions["paused"]) and the other similar success-path checks later in the same test), ensuring both parameters are validated together to match the behavior in service.go and the go-qbittorrent client.web/src/pages/CrossSeedPage.tsx (1)
646-714: Keep these persisted managed-mode settings visible even when no instance currently uses managed mode.Line 646 unmounts this whole section, but the same fields are still kept in
globalSettingsand later serialized in the global PATCH payload. That makes previously enabled pooled/reflink settings easy to save accidentally and impossible to clear until someone re-enables hardlink or reflink on an instance. Prefer rendering the section disabled with an explanatory note instead of hiding it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/CrossSeedPage.tsx` around lines 646 - 714, The section is currently completely unmounted when managedModeSummary.hasManagedMode is false which lets persisted flags in globalSettings (enablePooledPartialCompletion, maxMissingBytesAfterRecheck, allowReflinkSingleFileSizeMismatch) remain set and be accidentally saved; instead always render the managed-mode UI block but disable its interactive controls and show an explanatory note when hasManagedMode is false. Change the top-level conditional from rendering the whole div to rendering it unconditionally, use managedModeSummary.hasManagedMode to set disabled on the Switches and the Input (and to grey out explanatory text), and add a short note inside the block when !hasManagedMode explaining “These settings are persisted but only applied when an instance enables hardlink/reflink; you can clear them here even if no instance currently uses managed mode.”
🤖 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/api/handlers/crossseed.go`:
- Around line 35-57: The PUT handler currently decodes into
automationSettingsRequest and overwrites the full
models.CrossSeedAutomationSettings via UpdateAutomationSettings, which clears
persisted fields (rssSource*, webhookSource*, source tags, skipAutoResume*,
skipPieceBoundarySafetyCheck, etc.) and makes MaxMissingBytesAfterRecheck
default to 0 causing validation errors for older clients; fix by loading the
existing models.CrossSeedAutomationSettings inside the PUT `/settings` handler,
decode the incoming payload into automationSettingsRequest, merge only the
provided/nullable fields from automationSettingsRequest into the loaded settings
(preserving all other existing fields such as rss/webhook sources, source tags,
skip flags, and skipPieceBoundarySafetyCheck), apply validation to the merged
settings (don’t treat omitted MaxMissingBytesAfterRecheck as zero-meaningful),
then call UpdateAutomationSettings with the merged model; reference
automationSettingsRequest and UpdateAutomationSettings to locate the relevant
code.
In `@internal/database/postgres_migrations/068_add_cross_seed_partial_pools.sql`:
- Around line 20-30: The Postgres migration for cross_seed_partial_pool_members
is missing the UPDATE trigger that refreshes updated_at (so UPSERTs leave stale
timestamps), causing partialPoolMemberMissingGraceActive() to behave
incorrectly; to fix, add a Postgres trigger and function that sets updated_at =
CURRENT_TIMESTAMP on UPDATE for the cross_seed_partial_pool_members table
(matching the SQLite 067 behavior), or alternatively update the DO UPDATE clause
in crossseed_partial_pool.go to include updated_at = CURRENT_TIMESTAMP in its
SET list; prefer adding the CREATE FUNCTION/CREATE TRIGGER pair for
cross_seed_partial_pool_members to mirror SQLite and ensure updated_at updates
on every UPDATE.
In `@internal/models/crossseed_partial_pool.go`:
- Around line 190-197: GetByAnyHash is currently matching only on
target_hash/target_hash_v2 and can return expired
cross_seed_partial_pool_members; update the SQL built in the query variable to
exclude expired rows by adding an expires_at liveness check (e.g., AND
(expires_at IS NULL OR expires_at > CURRENT_TIMESTAMP) or the project's
equivalent NOW() check) so it matches the same liveness rule as ListActive; make
the same change to the other query block referenced around the later lines (the
second query that also selects from cross_seed_partial_pool_members) so both
lookups ignore expired pool members.
- Around line 265-268: In DeleteExpired, don't swallow errors from
result.RowsAffected(): when calling result.RowsAffected() capture and propagate
the error instead of returning (0, nil); change the error handling around the
result.RowsAffected() call so it returns (0, err) (or a wrapped error) to
callers and/or logs the failure; locate the result.RowsAffected() invocation and
the rows variable in DeleteExpired and update the return to propagate the
reported error.
In `@internal/services/crossseed/ensure_cross_category_test.go`:
- Around line 66-73: Replace the manual wg.Add + go func() { defer wg.Done() }()
pattern with the new WaitGroup.Go method: remove the wg.Add(goroutines) call and
the explicit defer wg.Done() inside the goroutine, and call wg.Go(func() {
<-start; errCh <- svc.ensureCrossCategory(context.Background(), 1,
"movies.cross", "/downloads/movies") }) for each iteration so the WaitGroup
manages goroutine lifecycle via Go().
In `@internal/services/crossseed/hardlink_mode_test.go`:
- Around line 603-607: Replace the custom mapsClone usage inside
recheckConfirmationSyncManager.AddTorrent with the stdlib maps.Clone: add an
import for "maps" and change m.addOptions = mapsClone(options) to m.addOptions =
maps.Clone(options); remove or stop using the custom mapsClone helper (and
delete it if unused elsewhere) so the test uses the built-in maps.Clone from Go
1.21+.
- Around line 560-575: The GetTorrents stub in recheckConfirmationSyncManager
currently ignores the filter; update the method (GetTorrents) to respect the
provided qbt.TorrentFilterOptions when selecting which torrents to return (e.g.,
match by infohash or other fields from the filter) so tests reflect production
polling; remove the custom mapsClone helper and replace its usage with the
standard library maps.Clone(options) call; and update the test assertions that
validate added torrents to assert that both "paused" and "stopped" options are
present and set to "true" (matching production behavior) for the added-torrent
checks referenced in the tests.
In `@internal/services/crossseed/partial_pool_test.go`:
- Line 1: Replace the old context/cancel pattern and the C-style loop to satisfy
Go 1.22+ idioms: in the test where you currently do "ctx, cancel :=
context.WithCancel(context.Background()); defer cancel()", remove that and use
the testing T's context via "ctx := t.Context()" (keep uses of ctx unchanged);
and replace the loop "for i := 0; i < 5; i++ { ... }" with the new repeat form
"for range 5 { ... }" (remove the unused loop index i). Ensure these edits are
applied in the same test/function that declares ctx/cancel and the loop so go
fix passes.
In `@internal/services/crossseed/partial_pool.go`:
- Around line 1324-1337: The function dropPartialPoolMember currently calls
removePartialPoolMember even when pausePartialPoolHash fails; change the flow so
you first call pausePartialPoolHash (as is), and only if it succeeds call
removePartialPoolMember and emit the Info log about leaving it paused for manual
review; if pausePartialPoolHash returns an error, log the Debug/Error (already
present) and return without calling removePartialPoolMember or emitting the
success Info message, ensuring the manual-review marker isn't dropped on
transient qBittorrent/API failures (refer to Service.dropPartialPoolMember,
Service.pausePartialPoolHash, and Service.removePartialPoolMember).
In `@internal/services/crossseed/service_candidate_cache_test.go`:
- Line 23: The test uses the old C-style loop "for i := 0; i < total; i++";
update that loop to the Go 1.22+ range-over-int idiom by replacing the loop with
the range form that iterates over the integer "total" (i.e., use the new for i
:= range ... form) so the test compiles with go fix and matches current
idiomatic syntax; locate the loop that references the variable "total" in
service_candidate_cache_test.go and change its header accordingly while leaving
the loop body unchanged.
---
Nitpick comments:
In `@internal/models/crossseed_partial_pool_test.go`:
- Around line 1-2: Add the missing copyright header to the top of the test file
so it matches other tests: insert the full multi-line copyright comment used
elsewhere (e.g., the line starting with "// Copyright (c) 2025-2026, s0up and
the autobrr contributors.") followed by the existing SPDX identifier ("//
SPDX-License-Identifier: GPL-2.0-or-later") at the very top of
internal/models/crossseed_partial_pool_test.go to ensure consistency with other
test files.
In `@internal/services/crossseed/hardlink_mode_test.go`:
- Around line 983-985: The test currently asserts that
syncManager.addOptions["paused"] is "true" but misses asserting the paired
"stopped" flag; update the assertions to also check
syncManager.addOptions["stopped"] == "true" wherever you assert paused (e.g.,
the block with assert.Equal(t, "true", syncManager.addOptions["paused"]) and the
other similar success-path checks later in the same test), ensuring both
parameters are validated together to match the behavior in service.go and the
go-qbittorrent client.
In `@internal/services/crossseed/matching_layout_test.go`:
- Around line 134-149: The test currently assumes the two torrents are tied but
doesn't verify they produce the same base match; before calling
svc.findBestCandidateMatch, explicitly compute and assert that both candidate
torrents yield the same base match/match score using the service helper that
evaluates a single torrent against the source (e.g. a computeBaseMatch/score
function or the existing internal matcher), by passing each torrent from
candidate.Torrents together with sourceRelease, sourceFiles and filesByHash
(obtained via svc.batchLoadCandidateFiles), and require that their base match
types/scores are equal; this ensures the test only exercises tag-based
tie-breaking in findBestCandidateMatch.
In `@internal/services/crossseed/rootless_savepath_test.go`:
- Around line 456-533: The test
TestProcessCrossSeedCandidate_ReflinkFallbackKeepsMatchedSavePath is
environment-dependent because it never creates a conflicting file in the managed
destination to force reflink to fail; modify the test to explicitly create a
conflicting file in the managed savepath (the same path that
Service.processCrossSeedCandidate would attempt to place the reflinked file)
before calling service.processCrossSeedCandidate so reflink mode
deterministically errors and the code falls back to regular copy; mirror the
approach used in the hardlink test by creating the conflict file (e.g., write a
dummy file at the managed destination path) so sync.addedOptions["savepath"]
ends up set to the matched save path as asserted.
In `@internal/services/crossseed/service_candidate_cache_test.go`:
- Around line 14-47: Refactor the two tests
TestCacheAutomationCandidateResponse_BoundedSize and
TestCacheAutomationCandidateResponse_DuplicateKeyDoesNotGrowOrder into a single
table-driven test that reuses the same setup (creating automationContext with
candidateCache and candidateOrder) and runs per-case actions: one case that
inserts automationCandidateCacheMaxEntries+25 distinct keys and asserts
candidateCache length equals automationCandidateCacheMaxEntries and the oldest
key ("release-0") is evicted, and another case that inserts the same key twice
and asserts candidateCache length is 1 and candidateOrder length is 1 with
"same-release" at index 0; implement cases using a slice of structs with name,
inputs, and expected assertions, and call cacheAutomationCandidateResponse
inside the loop to exercise the behavior while keeping assertions per-case.
In `@internal/web/swagger/openapi.yaml`:
- Around line 6227-6231: The OpenAPI schema for maxMissingBytesAfterRecheck only
documents the 100 MiB default in text; update the schema by adding default:
104857600 to the maxMissingBytesAfterRecheck property in both
CrossSeedAutomationSettingsPatch and CrossSeedAutomationSettings so generated
docs/clients receive the default value, ensuring the integer/format and minimum
remain unchanged.
In `@web/src/pages/CrossSeedPage.tsx`:
- Around line 646-714: The section is currently completely unmounted when
managedModeSummary.hasManagedMode is false which lets persisted flags in
globalSettings (enablePooledPartialCompletion, maxMissingBytesAfterRecheck,
allowReflinkSingleFileSizeMismatch) remain set and be accidentally saved;
instead always render the managed-mode UI block but disable its interactive
controls and show an explanatory note when hasManagedMode is false. Change the
top-level conditional from rendering the whole div to rendering it
unconditionally, use managedModeSummary.hasManagedMode to set disabled on the
Switches and the Input (and to grey out explanatory text), and add a short note
inside the block when !hasManagedMode explaining “These settings are persisted
but only applied when an instance enables hardlink/reflink; you can clear them
here even if no instance currently uses managed mode.”
🪄 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: 1bcf8270-89e2-46d7-95b4-e6866d76b5e7
📒 Files selected for processing (28)
cmd/qui/main.godocumentation/docs/features/cross-seed/hardlink-mode.mddocumentation/docs/features/cross-seed/overview.mddocumentation/docs/features/cross-seed/rules.mddocumentation/docs/features/cross-seed/troubleshooting.mdinternal/api/handlers/crossseed.gointernal/api/handlers/crossseed_patch_test.gointernal/api/handlers/crossseed_settings_validation_test.gointernal/database/migrations/067_add_cross_seed_partial_pools.sqlinternal/database/postgres_integration_test.gointernal/database/postgres_migrations/068_add_cross_seed_partial_pools.sqlinternal/models/crossseed.gointernal/models/crossseed_partial_pool.gointernal/models/crossseed_partial_pool_test.gointernal/models/crossseed_test.gointernal/models/postgres_bool_args_test.gointernal/services/crossseed/ensure_cross_category_test.gointernal/services/crossseed/hardlink_mode_test.gointernal/services/crossseed/matching_layout_test.gointernal/services/crossseed/partial_pool.gointernal/services/crossseed/partial_pool_test.gointernal/services/crossseed/rootless_savepath_test.gointernal/services/crossseed/service.gointernal/services/crossseed/service_candidate_cache_test.gointernal/services/crossseed/service_completion_queue_test.gointernal/web/swagger/openapi.yamlweb/src/pages/CrossSeedPage.tsxweb/src/types/index.ts
internal/database/postgres_migrations/068_add_cross_seed_partial_pools.sql
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
internal/api/handlers/crossseed_settings_validation_test.go (1)
30-64: Collapse the PUT/PATCH rejection cases into one table-driven test.These two tests only vary by HTTP method and handler entrypoint, so a small table would remove duplicated request/response setup and make future validation cases cheaper to extend.
As per coding guidelines, "Prefer table-driven test cases in Go tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/handlers/crossseed_settings_validation_test.go` around lines 30 - 64, Collapse the two duplicate tests into a single table-driven test that iterates over the two variants (PUT -> UpdateAutomationSettings and PATCH -> PatchAutomationSettings) and asserts the same response; create a slice of test cases containing the method string and a function or name of the handler entrypoint to call (e.g., "PUT" / handler.UpdateAutomationSettings and "PATCH" / handler.PatchAutomationSettings), reuse the common request/recorder setup and expected assertions (status http.StatusBadRequest and JSON error body), and run each subtest with t.Run so both cases are exercised without duplicated setup.internal/api/handlers/crossseed.go (1)
35-35: Consider centralizing the min-byte threshold constant.
minMaxMissingBytesAfterRecheckis currently handler-local. Moving it to a shared model/service constant would reduce drift risk between API, service, and UI validation rules.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/handlers/crossseed.go` at line 35, The handler-local constant minMaxMissingBytesAfterRecheck should be moved to a shared constant in a common package (e.g., model or service constants) and referenced from there: add an exported constant (e.g., MinMaxMissingBytesAfterRecheck) in the shared package, replace the local const minMaxMissingBytesAfterRecheck in crossseed.go with that exported constant, and update any other API/service/UI places to use the shared constant to avoid drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@documentation/docs/features/cross-seed/hardlink-mode.md`:
- Around line 123-135: The documentation's "Single-File Size Mismatch Override"
section currently describes unconditional recheck/resume behavior and omits
caveats when other features interact; update the paragraph and any related table
rows to document that the override will trigger qBittorrent recheck only when
"Skip recheck" is disabled and that reflink mode may enter the recheck path for
reasons other than extras (e.g., this single-file size mismatch flow), and
clarify interactions with "pooled partial completion" and the "Allow reflink
single-file size mismatch" option in the "Hardlink / Reflink Mode" UI so users
understand recheck can be skipped if "Skip recheck" is enabled.
In `@internal/database/postgres_migrations/068_add_cross_seed_partial_pools.sql`:
- Around line 6-30: Add a per-instance uniqueness constraint/index for
target_hash_v2 on the cross_seed_partial_pool_members table and update the
upsert logic to respect it: modify the schema for table
cross_seed_partial_pool_members to create a unique index or constraint on
(target_instance_id, target_hash_v2) with a WHERE clause target_hash_v2 IS NOT
NULL (matching the existing UNIQUE(target_instance_id, target_hash) semantics),
and update the store upsert code that writes to cross_seed_partial_pool_members
to include target_hash_v2 in its conflict/ON CONFLICT targeting so
inserts/updates obey the same per-instance uniqueness rule for v2 hashes.
In `@internal/services/crossseed/partial_pool.go`:
- Around line 203-210: When automation settings are nil or
EnablePooledPartialCompletion is false the function currently returns without
removing existing pooled markers, leaving torrents treated as pooled by
partialPoolOwnsTorrent/partialPoolOwnsLiveTorrent; add a cleanup step here to
drain existing pool members: call a new or existing helper (e.g.
s.clearPooledMembers(ctx) or s.drainPooledMembers(ctx)) that deletes
DB/in-memory pool markers for this service and logs any error, then return;
ensure the helper touches the same storage used by
partialPoolOwnsTorrent/partialPoolOwnsLiveTorrent so paused/orphaned members are
removed immediately.
---
Nitpick comments:
In `@internal/api/handlers/crossseed_settings_validation_test.go`:
- Around line 30-64: Collapse the two duplicate tests into a single table-driven
test that iterates over the two variants (PUT -> UpdateAutomationSettings and
PATCH -> PatchAutomationSettings) and asserts the same response; create a slice
of test cases containing the method string and a function or name of the handler
entrypoint to call (e.g., "PUT" / handler.UpdateAutomationSettings and "PATCH" /
handler.PatchAutomationSettings), reuse the common request/recorder setup and
expected assertions (status http.StatusBadRequest and JSON error body), and run
each subtest with t.Run so both cases are exercised without duplicated setup.
In `@internal/api/handlers/crossseed.go`:
- Line 35: The handler-local constant minMaxMissingBytesAfterRecheck should be
moved to a shared constant in a common package (e.g., model or service
constants) and referenced from there: add an exported constant (e.g.,
MinMaxMissingBytesAfterRecheck) in the shared package, replace the local const
minMaxMissingBytesAfterRecheck in crossseed.go with that exported constant, and
update any other API/service/UI places to use the shared constant to avoid
drift.
🪄 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: f78187a9-a8d1-42ce-81c6-fdba9c235f19
📒 Files selected for processing (13)
documentation/docs/features/cross-seed/hardlink-mode.mddocumentation/docs/features/cross-seed/rules.mddocumentation/docs/features/cross-seed/troubleshooting.mdinternal/api/handlers/crossseed.gointernal/api/handlers/crossseed_settings_validation_test.gointernal/database/postgres_migrations/068_add_cross_seed_partial_pools.sqlinternal/models/crossseed_partial_pool.gointernal/models/crossseed_partial_pool_test.gointernal/services/crossseed/ensure_cross_category_test.gointernal/services/crossseed/hardlink_mode_test.gointernal/services/crossseed/partial_pool.gointernal/services/crossseed/partial_pool_test.gointernal/services/crossseed/service_candidate_cache_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/services/crossseed/service_candidate_cache_test.go
- documentation/docs/features/cross-seed/rules.md
- internal/models/crossseed_partial_pool_test.go
- documentation/docs/features/cross-seed/troubleshooting.md
Comment on lines
+6
to
+30
| CREATE TABLE cross_seed_partial_pool_members ( | ||
| id BIGINT GENERATED BY DEFAULT AS IDENTITY PRIMARY KEY, | ||
| source_instance_id BIGINT NOT NULL, | ||
| source_hash TEXT NOT NULL, | ||
| target_instance_id BIGINT NOT NULL, | ||
| target_hash TEXT NOT NULL, | ||
| target_hash_v2 TEXT, | ||
| target_added_on BIGINT NOT NULL DEFAULT 0, | ||
| target_name TEXT NOT NULL DEFAULT '', | ||
| mode TEXT NOT NULL, | ||
| managed_root TEXT NOT NULL, | ||
| source_piece_length BIGINT NOT NULL DEFAULT 0, | ||
| max_missing_bytes_after_recheck BIGINT NOT NULL DEFAULT 104857600, | ||
| source_files_json TEXT NOT NULL DEFAULT '[]', | ||
| created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| updated_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| expires_at TIMESTAMP NOT NULL, | ||
| UNIQUE(target_instance_id, target_hash) | ||
| ); | ||
|
|
||
| CREATE INDEX idx_cross_seed_partial_pool_members_source | ||
| ON cross_seed_partial_pool_members(source_instance_id, source_hash); | ||
|
|
||
| CREATE INDEX idx_cross_seed_partial_pool_members_expires | ||
| ON cross_seed_partial_pool_members(expires_at); |
Contributor
There was a problem hiding this comment.
Make target_hash_v2 unique per instance too.
The new store treats target_hash and target_hash_v2 as interchangeable identities, but this schema only guarantees uniqueness on (target_instance_id, target_hash). A hybrid torrent can therefore be persisted twice if one path keys it by v1 and another by v2, and the later any-hash lookup becomes nondeterministic. Add a per-instance unique index/constraint for target_hash_v2 when it is non-null, and update the store upsert to target the same rule.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/database/postgres_migrations/068_add_cross_seed_partial_pools.sql`
around lines 6 - 30, Add a per-instance uniqueness constraint/index for
target_hash_v2 on the cross_seed_partial_pool_members table and update the
upsert logic to respect it: modify the schema for table
cross_seed_partial_pool_members to create a unique index or constraint on
(target_instance_id, target_hash_v2) with a WHERE clause target_hash_v2 IS NOT
NULL (matching the existing UNIQUE(target_instance_id, target_hash) semantics),
and update the store upsert code that writes to cross_seed_partial_pool_members
to include target_hash_v2 in its conflict/ON CONFLICT targeting so
inserts/updates obey the same per-instance uniqueness rule for v2 hashes.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/services/crossseed/partial_pool.go (1)
800-809: Consider logging or returning a more specific error when directory creation fails.When
os.MkdirAllfails, the error is returned but doesn't indicate which path failed. This could make debugging harder in production.💡 Optional improvement
if err := os.MkdirAll(filepath.Dir(dstPath), 0o755); err != nil { - return err + return fmt.Errorf("create destination directory for %s: %w", dstName, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/crossseed/partial_pool.go` around lines 800 - 809, The os.MkdirAll call around filepath.Dir(dstPath) returns the raw error without context; change the error handling in the block that calls os.MkdirAll to wrap or log the error with the directory path (use filepath.Dir(dstPath) and dstPath) so the returned error clearly states which directory creation failed (e.g. include the directory string in the error message using fmt.Errorf or a logger), keeping the existing behavior in the subsequent Lstat/Remove handling in partial_pool.go.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/services/crossseed/partial_pool.go`:
- Around line 800-809: The os.MkdirAll call around filepath.Dir(dstPath) returns
the raw error without context; change the error handling in the block that calls
os.MkdirAll to wrap or log the error with the directory path (use
filepath.Dir(dstPath) and dstPath) so the returned error clearly states which
directory creation failed (e.g. include the directory string in the error
message using fmt.Errorf or a logger), keeping the existing behavior in the
subsequent Lstat/Remove handling in partial_pool.go.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4bf74e66-f925-43d9-a0c9-6362ada88d76
📒 Files selected for processing (6)
documentation/docs/features/cross-seed/hardlink-mode.mdinternal/services/crossseed/ensure_cross_category_test.gointernal/services/crossseed/partial_pool.gointernal/services/crossseed/partial_pool_test.gointernal/services/crossseed/service.goweb/src/pages/CrossSeedPage.tsx
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 by CodeRabbit
New Features
Documentation