From 57b793169a4bc4121859c652b396ef5021692ad9 Mon Sep 17 00:00:00 2001 From: Evan Kazakin Date: Sun, 27 Sep 2026 21:11:49 -0400 Subject: [PATCH 1/2] feat(download): add Remove & Delete Files torrent completion action Closes #1027 --- docs/environment-variables.md | 6 ++-- shelfmark/download/clients/base_handler.py | 10 ++++-- shelfmark/download/clients/settings.py | 3 +- tests/prowlarr/test_handler.py | 38 ++++++++++++++++++++++ 4 files changed, 51 insertions(+), 6 deletions(-) diff --git a/docs/environment-variables.md b/docs/environment-variables.md index 6427daa4e..089eecd6f 100644 --- a/docs/environment-variables.md +++ b/docs/environment-variables.md @@ -1703,7 +1703,7 @@ How long to keep cached search results before they expire. | `RTORRENT_LABEL` | Label to assign to ebook downloads in rTorrent | string | `cwabd` | | `RTORRENT_AUDIOBOOK_LABEL` | Label to assign to audiobook downloads in rTorrent (falls back to Book Label if not set) | string | _none_ | | `RTORRENT_DOWNLOAD_DIR` | Server-side directory where torrents are downloaded (optional, uses rTorrent default if not specified) | string | _none_ | -| `PROWLARR_TORRENT_ACTION` | Choose whether to keep, remove, or move the torrent to another category or label after import | string (choice) | `keep` | +| `PROWLARR_TORRENT_ACTION` | What to do with the torrent in your client after a successful import. Remove stops seeding but keeps the downloaded files; Remove & Delete Files also deletes them from the client's download location. | string (choice) | `keep` | | `PROWLARR_TORRENT_POST_IMPORT_CATEGORY` | Category or label to assign after a successful import | string | _empty string_ | | `PROWLARR_USENET_CLIENT` | Choose which usenet client to use | string (choice) | _empty string_ | | `NZBGET_URL` | URL of your NZBGet instance | string | _none_ | @@ -2004,11 +2004,11 @@ Server-side directory where torrents are downloaded (optional, uses rTorrent def **Torrent Completion Action** -Choose whether to keep, remove, or move the torrent to another category or label after import +What to do with the torrent in your client after a successful import. Remove stops seeding but keeps the downloaded files; Remove & Delete Files also deletes them from the client's download location. - **Type:** string (choice) - **Default:** `keep` -- **Options:** `keep` (Keep), `remove` (Remove), `change_category` (Change Category) +- **Options:** `keep` (Keep), `remove` (Remove), `remove_and_delete` (Remove & Delete Files), `change_category` (Change Category) #### `PROWLARR_TORRENT_POST_IMPORT_CATEGORY` diff --git a/shelfmark/download/clients/base_handler.py b/shelfmark/download/clients/base_handler.py index c4ca3cbc6..eac3ba815 100644 --- a/shelfmark/download/clients/base_handler.py +++ b/shelfmark/download/clients/base_handler.py @@ -285,9 +285,15 @@ def post_process_cleanup(self, task: DownloadTask, *, success: bool) -> None: elif protocol == "torrent": torrent_action = config.get("PROWLARR_TORRENT_ACTION", "keep") - if torrent_action == "remove": + if torrent_action in ("remove", "remove_and_delete"): + # Only reached after a successful import, so the library already holds its + # own copy or hardlink of every file. The client deletes its own data, so + # remote path mappings don't matter here. try: - client.remove(download_id, delete_files=False) + client.remove( + download_id, + delete_files=torrent_action == "remove_and_delete", + ) except _CLIENT_CLEANUP_ERRORS as e: logger.warning( "Failed to remove torrent %s from %s: %s", diff --git a/shelfmark/download/clients/settings.py b/shelfmark/download/clients/settings.py index 451aec856..40aef6f9a 100644 --- a/shelfmark/download/clients/settings.py +++ b/shelfmark/download/clients/settings.py @@ -895,10 +895,11 @@ def prowlarr_clients_settings() -> list[SettingsField]: SelectField( key="PROWLARR_TORRENT_ACTION", label="Torrent Completion Action", - description="Choose whether to keep, remove, or move the torrent to another category or label after import", + description="What to do with the torrent in your client after a successful import. Remove stops seeding but keeps the downloaded files; Remove & Delete Files also deletes them from the client's download location.", options=[ {"value": "keep", "label": "Keep"}, {"value": "remove", "label": "Remove"}, + {"value": "remove_and_delete", "label": "Remove & Delete Files"}, {"value": "change_category", "label": "Change Category"}, ], default="keep", diff --git a/tests/prowlarr/test_handler.py b/tests/prowlarr/test_handler.py index f70a1a108..f4b9397fb 100644 --- a/tests/prowlarr/test_handler.py +++ b/tests/prowlarr/test_handler.py @@ -10,6 +10,8 @@ from threading import Event from unittest.mock import MagicMock, patch +import pytest + from shelfmark.core.models import DownloadTask from shelfmark.download.clients import ( DownloadState, @@ -1629,6 +1631,42 @@ def test_usenet_move_logs_cleanup_failure(self): assert args[2] == "nzbget" assert str(args[3]) == "offline" + @pytest.mark.parametrize( + ("torrent_action", "delete_files"), + [("remove", False), ("remove_and_delete", True)], + ) + def test_torrent_remove_actions_control_file_deletion(self, torrent_action, delete_files): + handler = ProwlarrHandler() + task = DownloadTask(task_id="torrent-remove", source="prowlarr", title="Test") + + mock_client = MagicMock() + mock_client.name = "qbittorrent" + handler._cleanup_refs[task.task_id] = (mock_client, "abc123", "torrent") + + with patch( + "shelfmark.download.clients.base_handler.config.get", return_value=torrent_action + ): + handler.post_process_cleanup(task, success=True) + + mock_client.remove.assert_called_once_with("abc123", delete_files=delete_files) + mock_client.set_category.assert_not_called() + + def test_torrent_remove_and_delete_skipped_when_import_fails(self): + handler = ProwlarrHandler() + task = DownloadTask(task_id="torrent-import-failed", source="prowlarr", title="Test") + + mock_client = MagicMock() + handler._cleanup_refs[task.task_id] = (mock_client, "abc123", "torrent") + + with patch( + "shelfmark.download.clients.base_handler.config.get", + return_value="remove_and_delete", + ): + handler.post_process_cleanup(task, success=False) + + mock_client.remove.assert_not_called() + assert task.task_id not in handler._cleanup_refs + def test_torrent_remove_logs_cleanup_failure(self): handler = ProwlarrHandler() task = DownloadTask(task_id="torrent-cleanup-failure", source="prowlarr", title="Test") From 427eb571093cded7aee8a9152d15c91b74d36826 Mon Sep 17 00:00:00 2001 From: Evan Kazakin Date: Tue, 29 Sep 2026 10:03:02 -0400 Subject: [PATCH 2/2] Handle unsupported torrent deletion and cleanup failures --- docs/environment-variables.md | 4 +-- shelfmark/download/clients/base_handler.py | 12 +++++--- shelfmark/download/clients/rtorrent.py | 35 +++++++--------------- shelfmark/download/clients/settings.py | 2 +- tests/prowlarr/test_handler.py | 25 ++++++++++++++++ tests/prowlarr/test_rtorrent_client.py | 18 +++++------ 6 files changed, 54 insertions(+), 42 deletions(-) diff --git a/docs/environment-variables.md b/docs/environment-variables.md index 089eecd6f..0a859c244 100644 --- a/docs/environment-variables.md +++ b/docs/environment-variables.md @@ -1703,7 +1703,7 @@ How long to keep cached search results before they expire. | `RTORRENT_LABEL` | Label to assign to ebook downloads in rTorrent | string | `cwabd` | | `RTORRENT_AUDIOBOOK_LABEL` | Label to assign to audiobook downloads in rTorrent (falls back to Book Label if not set) | string | _none_ | | `RTORRENT_DOWNLOAD_DIR` | Server-side directory where torrents are downloaded (optional, uses rTorrent default if not specified) | string | _none_ | -| `PROWLARR_TORRENT_ACTION` | What to do with the torrent in your client after a successful import. Remove stops seeding but keeps the downloaded files; Remove & Delete Files also deletes them from the client's download location. | string (choice) | `keep` | +| `PROWLARR_TORRENT_ACTION` | After a successful import, Remove keeps downloaded files in qBittorrent, Transmission, and Deluge; Remove & Delete Files also deletes them. rTorrent cannot delete download data, so this action leaves its torrent untouched. Blackhole does not support removal. Debrid clients delete temporary local files for either Remove action. | string (choice) | `keep` | | `PROWLARR_TORRENT_POST_IMPORT_CATEGORY` | Category or label to assign after a successful import | string | _empty string_ | | `PROWLARR_USENET_CLIENT` | Choose which usenet client to use | string (choice) | _empty string_ | | `NZBGET_URL` | URL of your NZBGet instance | string | _none_ | @@ -2004,7 +2004,7 @@ Server-side directory where torrents are downloaded (optional, uses rTorrent def **Torrent Completion Action** -What to do with the torrent in your client after a successful import. Remove stops seeding but keeps the downloaded files; Remove & Delete Files also deletes them from the client's download location. +After a successful import, Remove keeps downloaded files in qBittorrent, Transmission, and Deluge; Remove & Delete Files also deletes them. rTorrent cannot delete download data, so this action leaves its torrent untouched. Blackhole does not support removal. Debrid clients delete temporary local files for either Remove action. - **Type:** string (choice) - **Default:** `keep` diff --git a/shelfmark/download/clients/base_handler.py b/shelfmark/download/clients/base_handler.py index eac3ba815..6aac0e473 100644 --- a/shelfmark/download/clients/base_handler.py +++ b/shelfmark/download/clients/base_handler.py @@ -286,14 +286,18 @@ def post_process_cleanup(self, task: DownloadTask, *, success: bool) -> None: elif protocol == "torrent": torrent_action = config.get("PROWLARR_TORRENT_ACTION", "keep") if torrent_action in ("remove", "remove_and_delete"): - # Only reached after a successful import, so the library already holds its - # own copy or hardlink of every file. The client deletes its own data, so - # remote path mappings don't matter here. + # Import succeeded; the client handles its own download data. try: - client.remove( + removed = client.remove( download_id, delete_files=torrent_action == "remove_and_delete", ) + if not removed: + logger.warning( + "Failed to remove torrent %s from %s", + download_id, + getattr(client, "name", "client"), + ) except _CLIENT_CLEANUP_ERRORS as e: logger.warning( "Failed to remove torrent %s from %s: %s", diff --git a/shelfmark/download/clients/rtorrent.py b/shelfmark/download/clients/rtorrent.py index 8f0fc3fef..da3de5d5a 100644 --- a/shelfmark/download/clients/rtorrent.py +++ b/shelfmark/download/clients/rtorrent.py @@ -61,8 +61,6 @@ class _RTorrentDownloadProtocol(Protocol): def multicall2(self, *args: object) -> list[list[Any]]: ... - def delete_tied(self, download_id: str) -> object: ... - def erase(self, download_id: str) -> object: ... def stop(self, download_id: str) -> object: ... @@ -333,31 +331,20 @@ def get_status(self, download_id: str) -> DownloadStatus: return DownloadStatus.error(f"{error_type}: {e}") def remove(self, download_id: str, *, delete_files: bool = False) -> bool: - """Remove a torrent from rTorrent. - - Args: - download_id: Torrent info_hash - delete_files: Whether to also delete files - - Returns: - True if successful. + """Remove a torrent; rTorrent RPC cannot delete downloaded payloads.""" + if delete_files: + logger.warning( + "Cannot remove torrent %s with files: rTorrent RPC does not delete downloaded data", + download_id, + ) + return False - """ try: - # rtorrent is somehow case sensitive and requires uppercase hashes for look + # rTorrent lookups are case sensitive and require uppercase hashes. torrent_hash = download_id.upper() - if delete_files: - self._rpc.d.delete_tied(torrent_hash) - self._rpc.d.erase(torrent_hash) - else: - self._rpc.d.stop(torrent_hash) - self._rpc.d.erase(torrent_hash) - - logger.info( - "Removed torrent from rTorrent: %s%s", - download_id, - " (with files)" if delete_files else "", - ) + self._rpc.d.stop(torrent_hash) + self._rpc.d.erase(torrent_hash) + logger.info("Removed torrent from rTorrent: %s", download_id) except _RTORRENT_CLIENT_ERRORS as e: error_type = type(e).__name__ logger.exception("rTorrent remove failed (%s)", error_type) diff --git a/shelfmark/download/clients/settings.py b/shelfmark/download/clients/settings.py index 40aef6f9a..dba7e69d9 100644 --- a/shelfmark/download/clients/settings.py +++ b/shelfmark/download/clients/settings.py @@ -895,7 +895,7 @@ def prowlarr_clients_settings() -> list[SettingsField]: SelectField( key="PROWLARR_TORRENT_ACTION", label="Torrent Completion Action", - description="What to do with the torrent in your client after a successful import. Remove stops seeding but keeps the downloaded files; Remove & Delete Files also deletes them from the client's download location.", + description="After a successful import, Remove keeps downloaded files in qBittorrent, Transmission, and Deluge; Remove & Delete Files also deletes them. rTorrent cannot delete download data, so this action leaves its torrent untouched. Blackhole does not support removal. Debrid clients delete temporary local files for either Remove action.", options=[ {"value": "keep", "label": "Keep"}, {"value": "remove", "label": "Remove"}, diff --git a/tests/prowlarr/test_handler.py b/tests/prowlarr/test_handler.py index f4b9397fb..d621bf2c0 100644 --- a/tests/prowlarr/test_handler.py +++ b/tests/prowlarr/test_handler.py @@ -1689,6 +1689,31 @@ def test_torrent_remove_logs_cleanup_failure(self): assert args[2] == "qbittorrent" assert str(args[3]) == "offline" + @pytest.mark.parametrize("torrent_action", ["remove", "remove_and_delete"]) + def test_torrent_remove_logs_false_result(self, torrent_action): + handler = ProwlarrHandler() + task = DownloadTask(task_id="torrent-remove-false", source="prowlarr", title="Test") + mock_client = MagicMock() + mock_client.name = "rtorrent" + mock_client.remove.return_value = False + handler._cleanup_refs[task.task_id] = (mock_client, "abc123", "torrent") + + with ( + patch( + "shelfmark.download.clients.base_handler.config.get", + return_value=torrent_action, + ), + patch("shelfmark.download.clients.base_handler.logger.warning") as mock_warning, + ): + handler.post_process_cleanup(task, success=True) + + mock_client.remove.assert_called_once_with( + "abc123", delete_files=torrent_action == "remove_and_delete" + ) + mock_warning.assert_called_once_with( + "Failed to remove torrent %s from %s", "abc123", "rtorrent" + ) + def test_delete_local_download_data_ignores_path_lookup_failure(self): handler = ProwlarrHandler() mock_client = MagicMock() diff --git a/tests/prowlarr/test_rtorrent_client.py b/tests/prowlarr/test_rtorrent_client.py index aa9493f26..f1db46ecf 100644 --- a/tests/prowlarr/test_rtorrent_client.py +++ b/tests/prowlarr/test_rtorrent_client.py @@ -814,8 +814,8 @@ def test_remove_success(self, monkeypatch): mock_rpc.d.stop.assert_called_once_with("ABC123DEF456") mock_rpc.d.erase.assert_called_once_with("ABC123DEF456") - def test_remove_with_files(self, monkeypatch): - """Test torrent removal with file deletion.""" + def test_remove_with_files_is_unsupported(self, monkeypatch): + """Do not erase a torrent when rTorrent cannot delete its payload.""" config_values = { "RTORRENT_URL": "http://localhost:8080/RPC2", "RTORRENT_USERNAME": "", @@ -829,7 +829,6 @@ def test_remove_with_files(self, monkeypatch): ) mock_rpc = MagicMock() - mock_xmlrpc = create_mock_xmlrpc_module() mock_xmlrpc.ServerProxy.return_value = mock_rpc @@ -837,16 +836,13 @@ def test_remove_with_files(self, monkeypatch): if "shelfmark.download.clients.rtorrent" in sys.modules: del sys.modules["shelfmark.download.clients.rtorrent"] - from shelfmark.download.clients.rtorrent import ( - RTorrentClient, - ) + from shelfmark.download.clients.rtorrent import RTorrentClient client = RTorrentClient() - result = client.remove("abc123def456", delete_files=True) - - assert result is True - mock_rpc.d.delete_tied.assert_called_once_with("ABC123DEF456") - mock_rpc.d.erase.assert_called_once_with("ABC123DEF456") + assert client.remove("abc123def456", delete_files=True) is False + mock_rpc.d.stop.assert_not_called() + mock_rpc.d.erase.assert_not_called() + mock_rpc.d.delete_tied.assert_not_called() def test_remove_failure(self, monkeypatch): """Test failed torrent removal."""