diff --git a/shelfmark/download/clients/base_handler.py b/shelfmark/download/clients/base_handler.py index eb8afc6b..17863a7c 100644 --- a/shelfmark/download/clients/base_handler.py +++ b/shelfmark/download/clients/base_handler.py @@ -466,6 +466,8 @@ class ExternalClientHandler(DownloadHandler, ABC): 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) @@ -479,6 +481,12 @@ class ExternalClientHandler(DownloadHandler, ABC): 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", @@ -488,6 +496,24 @@ class ExternalClientHandler(DownloadHandler, ABC): ) 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, @@ -923,6 +949,7 @@ class ExternalClientHandler(DownloadHandler, ABC): cancel_flag=cancel_flag, progress_callback=progress_callback, status_callback=status_callback, + added_by_shelfmark=existing is None, ) except Exception as e: @@ -939,8 +966,14 @@ class ExternalClientHandler(DownloadHandler, ABC): 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() queued_since: float | None = None grace_requested_at = 0.0 @@ -1070,7 +1103,13 @@ class ExternalClientHandler(DownloadHandler, ABC): # 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) @@ -1082,7 +1121,13 @@ class ExternalClientHandler(DownloadHandler, ABC): ) 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 00000000..5ef531ca --- /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