diff --git a/docs/environment-variables.md b/docs/environment-variables.md index 6427daa4e..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` | Choose whether to keep, remove, or move the torrent to another category or label after import | 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,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 +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` -- **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..6aac0e473 100644 --- a/shelfmark/download/clients/base_handler.py +++ b/shelfmark/download/clients/base_handler.py @@ -285,9 +285,19 @@ 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"): + # Import succeeded; the client handles its own download data. try: - client.remove(download_id, delete_files=False) + 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 451aec856..dba7e69d9 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="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"}, + {"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..d621bf2c0 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") @@ -1651,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."""