fix: never cancel or remove transfers this item did not enqueue - #84
Conversation
- The watchdog skips file states the item doesn't own; a shared peer directory can put another item's transfers in FileStates, and a stuck foreign transfer could trip a bailout that cancels a live download - RemoveItemFilesAsync filters to owned transfers before cancelling, removing, or deleting local basenames
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. 📝 WalkthroughWalkthroughThe change restricts slskd transfer cancellation and file deletion to files owned by the current download item. New watchdog tests cover shared directories and rejected transfers. Changesslskd transfer ownership
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🔵 Low · up to This change prevents downloads from cancelling or deleting transfers and local files belonging to other items in most shared-directory cases. A bounded risk remains when separate transfers use the same filename, because filename-based ownership may still conflate them; merge is reasonable with explicit owner awareness of that edge case. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
Note for merge order: these two guards use OwnsFile, which counts files slskd rejected at enqueue as owned. #83 introduces OwnsAcceptedFile (enqueued AND accepted) after a CodeRabbit finding on the same class, and these sites want the stricter predicate too - a rejected file has no transfer of ours, so cancelling or removing one under that name would hit another items download. The predicate does not exist on main yet, so whichever PR merges first, I will follow up on the other so both paths share one definition of ownership. |
…nsfer-guards # Conflicts: # tests/Sleezer.Tests/Sleezer.Tests.csproj
OwnsAcceptedFile landed on main with the extras work; the watchdog and removal guards were written before it existed and still counted files slskd rejected at enqueue as ours. Those produce no transfer of ours, so cancelling or removing one under that name hits another item's download.
|
Merged main in and did the follow-up in 78bba77. The only conflict was both PRs appending Compile entries to the test csproj; sources auto-merged and both feature sets were verified intact afterwards. Now that OwnsAcceptedFile is on main, both guards use it instead of OwnsFile, so a file slskd rejected at enqueue is no longer treated as ours on the destructive paths — its only possible transfer belongs to another item, and cancelling or removing one would kill a live download. Added a watchdog test for the rejected-file case, verified failing against OwnsFile. 458 tests green. Deliberately left on the looser predicate: the attribution lookups in SlskdDownloadManager (stale-attempt eviction, FindItemOwningDirectory, FindGrabOwningDirectory). Those answer "which item asked for this file". FindItemOwningDirectory is arguably where the contamination originates and is worth revisiting on its own, since changing attribution has a wider blast radius than these guards. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
1 participant
When two downloads pull from the same peer directory,
ProcessUserTransfershands the whole per-directory transfer group to whichever tracked item owns the group's first file, so one item's transfer view can contain another item's files. #83 hardened the status/completion side of that; this fixes the two remaining paths that act destructively on the contaminated view:InspectAsynciterated every file state on the item, so a stuck foreign transfer could trip a bailout and cancel another download's transfer at slskd. It now skips files the item never enqueued.RemoveItemFilesAsynccancelled and removed every transfer in the merged directory view and deleted matching local basenames — including another item's, in a same-folder share. The file list is now filtered to owned transfers before anything is cancelled, removed, or deleted.Both guards use
SlskdDownloadItem.OwnsFile, the same ownership test the delete guards already rely on.protectBasenamessemantics and the ownership-guarded folder deletion are untouched. Independent of #83 — no shared hunks, merges either side of it.Summary by CodeRabbit
Bug Fixes
Tests