From 2c62716c4c13bfe7f11e7ac0bd6fc61b2d60462a Mon Sep 17 00:00:00 2001 From: Rob Martin Date: Fri, 2 Oct 2026 16:29:24 -0600 Subject: [PATCH] fix(download): remove a cancelled torrent that never started Cancelling a torrent download leaves it in the client so one that already has data can keep seeding. A torrent that is still at 0% has nothing to seed and nothing to resume, and leaving it queued only holds a download slot and piles up dead entries. When a download is cancelled and its torrent has downloaded nothing, remove it from the client (and its files) if Shelfmark added it. A torrent with data is still left alone, and so is one that was already in the client before the download started. --- shelfmark/download/clients/base_handler.py | 51 ++++++++++- tests/download/test_torrent_removal.py | 100 +++++++++++++++++++++ 2 files changed, 148 insertions(+), 3 deletions(-) create mode 100644 tests/download/test_torrent_removal.py diff --git a/shelfmark/download/clients/base_handler.py b/shelfmark/download/clients/base_handler.py index c4ca3cbc6..bbd888ad6 100644 --- a/shelfmark/download/clients/base_handler.py +++ b/shelfmark/download/clients/base_handler.py @@ -451,6 +451,8 @@ def _handle_cancelled_download( download_id: str, protocol: str, status_callback: Callable[[str, str | None], None], + *, + remove_if_unstarted: bool = False, ) -> None: if protocol == "usenet": logger.info("Download cancelled, removing from %s: %s", client.name, download_id) @@ -464,6 +466,12 @@ def _handle_cancelled_download( client.name, e, ) + elif remove_if_unstarted and self._remove_unstarted_torrent(client, download_id): + logger.info( + "Download cancelled before it started, removed from %s: %s", + client.name, + download_id, + ) else: logger.info( "Download cancelled for protocol=%s; leaving in %s: %s", @@ -473,6 +481,24 @@ def _handle_cancelled_download( ) status_callback("cancelled", "Cancelled") + def _remove_unstarted_torrent(self, client: DownloadClient, download_id: str) -> bool: + """Remove a torrent that downloaded nothing; True if it was removed. + + Torrents are normally left in the client after a cancel so an already-downloaded one + can keep seeding. One at 0% has nothing to seed and nothing to resume, and leaving it + queued only holds a download slot and piles up dead entries. + """ + try: + status = client.get_status(download_id) + if status.progress > 0: # it has data, so it may be seeding or resumable + return False + return bool(client.remove(download_id, delete_files=True)) + except _CLIENT_CLEANUP_ERRORS as e: + logger.warning( + "Could not remove unstarted torrent %s from %s: %s", download_id, client.name, e + ) + return False + def _resolve_download_path_once( self, client: DownloadClient, @@ -908,6 +934,7 @@ def download( cancel_flag=cancel_flag, progress_callback=progress_callback, status_callback=status_callback, + added_by_shelfmark=existing is None, ) except Exception as e: @@ -924,8 +951,14 @@ def _poll_and_complete( cancel_flag: Event, progress_callback: Callable[[float], None], status_callback: Callable[[str, str | None], None], + *, + added_by_shelfmark: bool = False, ) -> str | None: - """Poll the download client for progress and handle completion.""" + """Poll the download client for progress and handle completion. + + ``added_by_shelfmark`` is False when the task joined a torrent the client already had; + such a torrent is never removed on cancel. + """ poll_interval = self._poll_interval() # Track consecutive "not found" errors - torrents may take time to appear in client not_found_count = 0 @@ -1026,7 +1059,13 @@ def _poll_and_complete( # Handle cancellation if cancel_flag.is_set(): - self._handle_cancelled_download(client, download_id, protocol, status_callback) + self._handle_cancelled_download( + client, + download_id, + protocol, + status_callback, + remove_if_unstarted=added_by_shelfmark, + ) return None # Handle completed file (wait briefly for files to appear) @@ -1038,7 +1077,13 @@ def _poll_and_complete( ) if not source_path_obj: if cancel_flag.is_set(): - self._handle_cancelled_download(client, download_id, protocol, status_callback) + self._handle_cancelled_download( + client, + download_id, + protocol, + status_callback, + remove_if_unstarted=added_by_shelfmark, + ) return None status_callback( "error", diff --git a/tests/download/test_torrent_removal.py b/tests/download/test_torrent_removal.py new file mode 100644 index 000000000..5ef531ca9 --- /dev/null +++ b/tests/download/test_torrent_removal.py @@ -0,0 +1,100 @@ +"""A cancelled torrent that never started is removed from the client.""" + +from __future__ import annotations + +from threading import Event +from unittest.mock import MagicMock, patch + +import pytest + +from shelfmark.core.models import DownloadTask +from shelfmark.download.clients import DownloadState, DownloadStatus +from shelfmark.release_sources.prowlarr.handler import ProwlarrHandler + + +def _status(state: DownloadState, progress: float = 0.0, **kw) -> DownloadStatus: + return DownloadStatus( + progress=progress, + state=state, + message="Queued" if state == DownloadState.QUEUED else None, + complete=kw.get("complete", False), + file_path=None, + ) + + +class _Recorder: + def __init__(self) -> None: + self.events: list[tuple[str, str | None]] = [] + + def status(self, status: str, message: str | None) -> None: + self.events.append((status, message)) + + +Q, D = DownloadState.QUEUED, DownloadState.DOWNLOADING + + +# --- removing a torrent that never started ----------------------------------------------- + + +def _cancel(status: DownloadStatus, *, existing: bool, protocol: str = "torrent") -> MagicMock: + client = MagicMock() + client.name = "qbittorrent" + client.find_existing.return_value = ("dl", status) if existing else None + client.add_download.return_value = "dl" + client.remove.return_value = True + cancel = Event() + + def get_status(_id: str) -> DownloadStatus: + cancel.set() # the user cancels while the download is being polled + return status + + client.get_status.side_effect = get_status + release = {"protocol": protocol, "magnetUrl": "magnet:?xt=urn:btih:abc123"} + with ( + patch("shelfmark.release_sources.prowlarr.handler.get_release", return_value=release), + patch("shelfmark.release_sources.prowlarr.handler.get_client", return_value=client), + patch("shelfmark.release_sources.prowlarr.handler.POLL_INTERVAL", 0.01), + ): + ProwlarrHandler().download( + task=DownloadTask(task_id="c", source="prowlarr", title="Book"), + cancel_flag=cancel, + progress_callback=lambda _p: None, + status_callback=_Recorder().status, + ) + return client + + +@pytest.mark.parametrize("state", [DownloadState.QUEUED, DownloadState.DOWNLOADING]) +def test_a_cancelled_torrent_that_downloaded_nothing_is_removed(state: DownloadState) -> None: + client = _cancel(_status(state, 0.0), existing=False) + + client.remove.assert_called_once_with("dl", delete_files=True) + + +def test_a_cancelled_torrent_with_data_is_left_for_seeding() -> None: + client = _cancel(_status(D, 12.0), existing=False) + + client.remove.assert_not_called() + + +def test_a_cancelled_torrent_the_user_already_had_is_never_removed() -> None: + client = _cancel(_status(Q, 0.0), existing=True) + + client.remove.assert_not_called() + + +def test_a_complete_or_seeding_torrent_is_not_removed() -> None: + client = _cancel(_status(DownloadState.SEEDING, 100.0, complete=True), existing=False) + + client.remove.assert_not_called() + + +def test_a_failed_removal_does_not_break_the_cancel() -> None: + status = _status(Q, 0.0) + client = MagicMock() + client.remove.side_effect = OSError("boom") + client.get_status.return_value = status + + ProwlarrHandler()._handle_cancelled_download( + client, "dl", "torrent", lambda *_a: None, remove_if_unstarted=True + ) # does not raise