fix(download): remove a cancelled torrent that never started (#1421)

Cancelling a torrent download leaves the torrent in the client.
`_safe_remove_download` documents the rule: "torrents: never remove or
delete client data (avoid breaking seeding)". That makes sense for a
torrent that has data. A torrent that is still at 0% has nothing to seed
and nothing to resume, and leaving it behind holds a download slot in
qBittorrent's queue or sits there as a dead entry.

I hit this on a qBittorrent that is shared with Sonarr, Radarr and
Lidarr and has a download limit. Shelfmark cancelled downloads that were
queued (#1420) and left them in the client. Three torrents that never
fetched metadata held every active slot, and each torrent added after
them waited behind them. Removing those by hand freed the slots.

This removes the torrent and its files when a download is cancelled,
Shelfmark added the torrent, and the client reports 0% progress. A
torrent with any data is left alone as before. So is one that was
already in the client when the download started, since Shelfmark didn't
add it. If the removal fails it is logged and the cancel carries on.

Tests cover removal at 0% for a queued and a downloading torrent,
leaving one that has data, leaving one the user already had, and a
failing removal. The full suite passes, and I broke each rule on purpose
to check a test catches it.

This changes a rule you wrote down, so I've kept it apart from the queue
fix. If you'd rather have it as a setting, or only remove a torrent that
is still queued or fetching metadata, I'm happy to rework it.

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",

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