diff --git a/docs/configuration.md b/docs/configuration.md index 09d3bea1..8c9a2bc3 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -95,6 +95,11 @@ If the files are copied or synced into Shelfmark on a delay, increase **Complete in Settings -> Advanced. The default is 60 seconds; seedbox or remote-sync setups may need a value longer than the sync interval. +With qBittorrent, Shelfmark also reads the torrent's own file list and imports only once every file +the torrent downloads is there at its full size, so a sync that copies files in batches and pauses +between them does not cause a partial import. The same wait applies, and files the torrent skips +are not counted. + ## File Processing Options ### Hardlink Torrents (Torrent / Usenet Only) diff --git a/shelfmark/download/clients/__init__.py b/shelfmark/download/clients/__init__.py index f699a403..3b9e4151 100644 --- a/shelfmark/download/clients/__init__.py +++ b/shelfmark/download/clients/__init__.py @@ -178,6 +178,14 @@ class DownloadStatus: return self.state +@dataclass(frozen=True) +class ExpectedFile: + """A file the client will have on disk when a download is complete.""" + + name: str # Path relative to the client's save directory, "/" separated + size: int # Final size in bytes + + class DownloadClient(ABC): """Base class for external download clients. @@ -357,6 +365,15 @@ class DownloadClient(ABC): """ + def get_expected_files(self, download_id: str) -> list[ExpectedFile] | None: + """The files a finished download holds, or None when the client cannot say. + + Lets the handler tell a folder that has all of its files from one that stopped changing + while a sync to this host is still copying them in. Only files the client downloads + count: a file the user skipped is not expected. Default: the client cannot say. + """ + return None + def find_existing( self, url: str, category: str | None = None ) -> tuple[str, DownloadStatus] | None: diff --git a/shelfmark/download/clients/base_handler.py b/shelfmark/download/clients/base_handler.py index c30ca0fa..016f473e 100644 --- a/shelfmark/download/clients/base_handler.py +++ b/shelfmark/download/clients/base_handler.py @@ -20,6 +20,7 @@ from shelfmark.download.clients import ( DownloadClient, DownloadState, DownloadStatus, + ExpectedFile, get_client, list_configured_clients, ) @@ -34,7 +35,7 @@ from shelfmark.download.permissions_debug import log_path_permission_context from shelfmark.release_sources import DownloadHandler, HandoffResult if TYPE_CHECKING: - from collections.abc import Callable + from collections.abc import Callable, Sequence from threading import Event from shelfmark.core.models import DownloadTask @@ -251,6 +252,42 @@ def _completed_file_signature( return tuple(sorted(signature)) or None +def _count_expected_files_present(path: Path, expected: Sequence[ExpectedFile]) -> int: + """How many of the client's files are at ``path`` with their final size. + + ``path`` is the completed download: the torrent's top-level folder, or the file itself for a + single-file torrent. The client names files relative to its save directory, so they start + with that top-level folder, and the folder is tried with and without it. + """ + try: + single_file = path.is_file() + if single_file: + local = {path.name: path.stat().st_size} + elif path.is_dir(): + local = {} + for candidate in path.rglob("*"): + try: + if candidate.is_file(): + local[candidate.relative_to(path).as_posix()] = candidate.stat().st_size + except OSError: + continue + else: + return 0 + except OSError: + return 0 + + present = 0 + for item in expected: + names = [item.name] + if "/" in item.name: + names.append(item.name.split("/", 1)[1]) + if single_file: + names.append(item.name.rsplit("/", 1)[1]) + if any(local.get(name) == item.size for name in names): + present += 1 + return present + + class ExternalClientHandler(DownloadHandler, ABC): """Shared lifecycle handler for sources that hand off to torrent/usenet clients.""" @@ -864,9 +901,16 @@ class ExternalClientHandler(DownloadHandler, ABC): status_callback: Callable[[str, str | None], None], content_type: str | None = None, ) -> tuple[Path | None, str | None]: - """Wait for supported files to appear and stop changing on disk.""" + """Wait for supported files to appear and stop changing on disk. + + When the client can list the files of the download, every one of them must also be + present at its final size. A sync that copies the files in batches can pause long + enough for two samples to match while most of the files are still missing. + """ last_error: str | None = None previous_signature: tuple[tuple[str, int, int], ...] | None = None + expected_files: list[ExpectedFile] | None = None + expected_files_asked = False retry_interval = self._completed_path_retry_interval() timeout_seconds = self._completed_path_timeout_seconds() if retry_interval <= 0 or timeout_seconds <= 0: @@ -885,18 +929,38 @@ class ExternalClientHandler(DownloadHandler, ABC): log_details=log_details, ) if result.path: - if run_blocking_io(result.path.is_file): - return result.path, None - signature = run_blocking_io( - _completed_file_signature, - result.path, - content_type, + if not expected_files_asked: + expected_files = self._expected_files(client, download_id) + expected_files_asked = True + files_on_disk = ( + run_blocking_io(_count_expected_files_present, result.path, expected_files) + if expected_files + else None ) - # With no wait configured there is no second sample to compare against. - if signature is not None and (signature == previous_signature or max_attempts == 1): - return result.path, None - previous_signature = signature - last_error = "Completed download has no stable supported files yet." + if ( + expected_files + and files_on_disk is not None + and files_on_disk < len(expected_files) + ): + last_error = ( + "Completed download is not fully on disk yet " + f"({files_on_disk} of {len(expected_files)} files)." + ) + else: + if run_blocking_io(result.path.is_file): + return result.path, None + signature = run_blocking_io( + _completed_file_signature, + result.path, + content_type, + ) + # With no wait configured there is no second sample to compare against. + if signature is not None and ( + signature == previous_signature or max_attempts == 1 + ): + return result.path, None + previous_signature = signature + last_error = "Completed download has no stable supported files yet." if attempt < max_attempts: status_callback("locating", "Waiting for completed files...") logger.debug( @@ -943,6 +1007,18 @@ class ExternalClientHandler(DownloadHandler, ABC): return None, last_error + @staticmethod + def _expected_files(client: DownloadClient, download_id: str) -> list[ExpectedFile] | None: + """The client's list of files for this download, or None when it has none to give.""" + try: + expected = client.get_expected_files(download_id) + except _CLIENT_CLEANUP_ERRORS: + logger.debug("Could not read the file list of %s from %s", download_id, client.name) + return None + if not isinstance(expected, list) or not expected: + return None + return [item for item in expected if isinstance(item, ExpectedFile)] or None + @staticmethod def _seed_note(seeds: int | None, give_up_in: float | None) -> str: """What the card shows beside progress so nobody has to open the client to see the swarm.""" diff --git a/shelfmark/download/clients/qbittorrent.py b/shelfmark/download/clients/qbittorrent.py index 389655d2..869697cd 100644 --- a/shelfmark/download/clients/qbittorrent.py +++ b/shelfmark/download/clients/qbittorrent.py @@ -16,6 +16,7 @@ from shelfmark.core.logger import setup_logger from shelfmark.download.clients import ( DownloadClient, DownloadStatus, + ExpectedFile, register_client, ) from shelfmark.download.clients._coercion import ( @@ -800,6 +801,52 @@ class QBittorrentClient(DownloadClient): getattr(torrent, "name", ""), ) + def _get_with_auth(self, url: str, params: dict[str, str]) -> requests.Response: + """GET a Web API endpoint, signing in again once if the session was dropped.""" + self._ensure_authenticated() + resp = self._client._session.get(url, params=params, timeout=10) + if resp.status_code == _HTTP_STATUS_FORBIDDEN and self._can_reauthenticate: + logger.debug("qBittorrent returned 403; re-authenticating and retrying") + self._ensure_authenticated() + resp = self._client._session.get(url, params=params, timeout=10) + return resp + + def get_expected_files(self, download_id: str) -> list[ExpectedFile] | None: + """The files this torrent holds when finished, from `/api/v2/torrents/files`. + + Files with priority 0 are not downloaded and BEP 47 padding files are not real, so + neither is expected. None when the client does not answer or lists nothing. + """ + try: + resp = self._get_with_auth( + f"{self._base_url}/api/v2/torrents/files", {"hash": download_id} + ) + if resp.status_code == _HTTP_STATUS_NOT_FOUND: + return None + resp.raise_for_status() + payload = resp.json() + except _QBITTORRENT_CLIENT_ERRORS as e: + self._log_error("get_expected_files", e, level="debug") + return None + if not isinstance(payload, list): + return None + + expected: list[ExpectedFile] = [] + for entry in payload: + if not isinstance(entry, dict): + continue + name = entry.get("name") + size = entry.get("size") + if not isinstance(name, str) or not name or isinstance(size, bool): + continue + if not isinstance(size, int) or size <= 0 or entry.get("priority") == 0: + continue + normalized = name.replace("\\", "/") + if ".pad" in normalized.split("/")[:-1]: + continue + expected.append(ExpectedFile(normalized, size)) + return expected or None + def _derive_download_path_from_files( self, download_id: str ) -> str | _UnsafeQBittorrentPath | None: @@ -810,20 +857,11 @@ class QBittorrentClient(DownloadClient): """ import os - def get_with_auth(url: str, params: dict[str, str]) -> requests.Response: - self._ensure_authenticated() - resp = self._client._session.get(url, params=params, timeout=10) - if resp.status_code == _HTTP_STATUS_FORBIDDEN and self._can_reauthenticate: - logger.debug("qBittorrent returned 403; re-authenticating and retrying") - self._ensure_authenticated() - resp = self._client._session.get(url, params=params, timeout=10) - return resp - try: properties_url = f"{self._base_url}/api/v2/torrents/properties" files_url = f"{self._base_url}/api/v2/torrents/files" - props_resp = get_with_auth(properties_url, {"hash": download_id}) + props_resp = self._get_with_auth(properties_url, {"hash": download_id}) if props_resp.status_code == _HTTP_STATUS_NOT_FOUND: return None props_resp.raise_for_status() @@ -833,7 +871,7 @@ class QBittorrentClient(DownloadClient): if not isinstance(save_path, str) or not save_path: return None - files_resp = get_with_auth(files_url, {"hash": download_id}) + files_resp = self._get_with_auth(files_url, {"hash": download_id}) if files_resp.status_code == _HTTP_STATUS_NOT_FOUND: return None files_resp.raise_for_status() diff --git a/tests/prowlarr/test_qbittorrent_client.py b/tests/prowlarr/test_qbittorrent_client.py index 63aac331..4c97b33a 100644 --- a/tests/prowlarr/test_qbittorrent_client.py +++ b/tests/prowlarr/test_qbittorrent_client.py @@ -82,6 +82,73 @@ def test_set_category_creates_and_assigns_category(): ) +def _client_answering_files(payload, status_code=200): + from shelfmark.download.clients.qbittorrent import QBittorrentClient + + client = QBittorrentClient.__new__(QBittorrentClient) + client._base_url = "http://localhost:8080" + client._api_key = "key" # a bearer key cannot sign in again, so no retry + client._ensure_authenticated = MagicMock() + client._client = MagicMock() + response = MagicMock(status_code=status_code) + response.json.return_value = payload + response.raise_for_status = MagicMock() + client._client._session.get.return_value = response + return client + + +class TestGetExpectedFiles: + def test_lists_the_files_with_their_sizes(self): + client = _client_answering_files( + [ + {"name": "Book/01.mp3", "size": 100, "priority": 1}, + {"name": "Book/02.mp3", "size": 200, "priority": 7}, + ] + ) + + expected = client.get_expected_files("abc123") + + assert [(f.name, f.size) for f in (expected or [])] == [ + ("Book/01.mp3", 100), + ("Book/02.mp3", 200), + ] + assert client._client._session.get.call_args.kwargs["params"] == {"hash": "abc123"} + + def test_skips_files_the_client_is_not_downloading_and_padding_files(self): + client = _client_answering_files( + [ + {"name": "Book/01.mp3", "size": 100, "priority": 1}, + {"name": "Book/skipped.jpg", "size": 50, "priority": 0}, + {"name": "Book/.pad/0", "size": 4096, "priority": 1}, + {"name": "Book/empty.txt", "size": 0, "priority": 1}, + ] + ) + + expected = client.get_expected_files("abc123") + + assert [f.name for f in (expected or [])] == ["Book/01.mp3"] + + def test_normalizes_windows_separators(self): + client = _client_answering_files([{"name": "Book\\01.mp3", "size": 100, "priority": 1}]) + + assert [f.name for f in (client.get_expected_files("abc123") or [])] == ["Book/01.mp3"] + + @pytest.mark.parametrize( + ("payload", "status_code"), + [([], 200), ({"error": "x"}, 200), ([{"name": "a", "size": 0}], 200), ([], 404)], + ) + def test_none_when_there_is_nothing_to_go_on(self, payload, status_code): + assert _client_answering_files(payload, status_code).get_expected_files("abc123") is None + + def test_none_when_the_client_cannot_be_reached(self): + import requests + + client = _client_answering_files([]) + client._client._session.get.side_effect = requests.ConnectionError("down") + + assert client.get_expected_files("abc123") is None + + class TestQBittorrentClientIsConfigured: """Tests for QBittorrentClient.is_configured().""" diff --git a/tests/prowlarr/test_remote_path_mappings.py b/tests/prowlarr/test_remote_path_mappings.py index aea18b15..00eafcdc 100644 --- a/tests/prowlarr/test_remote_path_mappings.py +++ b/tests/prowlarr/test_remote_path_mappings.py @@ -365,12 +365,15 @@ def test_waits_for_supported_file_to_stop_growing_in_remapped_directory(monkeypa assert recorder.status_updates.count(("locating", "Waiting for completed files...")) == 3 -def _wait_for_local_directory(monkeypatch, directory: Path, *, timeout: object): +def _wait_for_local_directory( + monkeypatch, directory: Path, *, timeout: object, on_sleep=None, expected=None +): import shelfmark.download.clients.base_handler as base_handler mock_client = MagicMock() mock_client.name = "qbittorrent" mock_client.get_download_path.return_value = str(directory) + mock_client.get_expected_files.return_value = expected def config_get(key: str, default=""): if key == "PROWLARR_REMOTE_PATH_MAPPINGS": @@ -382,7 +385,7 @@ def _wait_for_local_directory(monkeypatch, directory: Path, *, timeout: object): handler = ProwlarrHandler() monkeypatch.setattr(base_handler.config, "get", config_get) monkeypatch.setattr(handler, "_completed_path_retry_interval", lambda: 0.01) - monkeypatch.setattr(base_handler.time, "sleep", lambda _seconds: None) + monkeypatch.setattr(base_handler.time, "sleep", on_sleep or (lambda _seconds: None)) return handler._wait_for_completed_path( mock_client, "download_id", @@ -429,6 +432,123 @@ def test_a_directory_without_book_files_still_fails_when_no_wait_is_configured( assert error == "Completed download has no stable supported files yet." +def _expected(*files: tuple[str, int]): + from shelfmark.download.clients import ExpectedFile + + return [ExpectedFile(name, size) for name, size in files] + + +def _batch_release(tmp_path, count: int, *, present: int): + """A 23-file style book folder that a sync has copied `present` files of.""" + release = tmp_path / "Release" + release.mkdir() + for number in range(1, present + 1): + (release / f"{number:02d}.epub").write_bytes(b"x" * (100 + number)) + expected = _expected(*((f"Release/{n:02d}.epub", 100 + n) for n in range(1, count + 1))) + return release, expected + + +def test_a_folder_with_only_some_of_the_clients_files_keeps_waiting(monkeypatch, tmp_path): + """The sync paused after 10 of 23 files and the folder stopped changing (#1561).""" + release, expected = _batch_release(tmp_path, 23, present=10) + + resolved_path, error = _wait_for_local_directory( + monkeypatch, release, timeout=1, expected=expected + ) + + assert resolved_path is None + assert error == "Completed download is not fully on disk yet (10 of 23 files)." + + +def test_a_truncated_file_does_not_count_as_present(monkeypatch, tmp_path): + release, expected = _batch_release(tmp_path, 3, present=3) + (release / "02.epub").write_bytes(b"x" * 5) + + resolved_path, error = _wait_for_local_directory( + monkeypatch, release, timeout=1, expected=expected + ) + + assert resolved_path is None + assert error == "Completed download is not fully on disk yet (2 of 3 files)." + + +def test_the_import_goes_ahead_once_the_rest_of_the_files_arrive(monkeypatch, tmp_path): + release, expected = _batch_release(tmp_path, 5, present=2) + sleeps = 0 + + def sync_more(_seconds: float) -> None: + nonlocal sleeps + sleeps += 1 + arriving = release / f"{2 + sleeps:02d}.epub" + if 2 + sleeps <= 5: + arriving.write_bytes(b"x" * (100 + 2 + sleeps)) + + resolved_path, error = _wait_for_local_directory( + monkeypatch, release, timeout=1, expected=expected, on_sleep=sync_more + ) + + assert resolved_path == release + assert error is None + assert sleeps >= 3 + + +def test_a_complete_folder_is_accepted(monkeypatch, tmp_path): + release, expected = _batch_release(tmp_path, 4, present=4) + + resolved_path, error = _wait_for_local_directory( + monkeypatch, release, timeout=1, expected=expected + ) + + assert resolved_path == release + assert error is None + + +def test_names_without_the_top_level_folder_also_match(monkeypatch, tmp_path): + """A torrent saved without a subfolder is listed without one.""" + release, _ = _batch_release(tmp_path, 3, present=3) + expected = _expected(*((f"{n:02d}.epub", 100 + n) for n in range(1, 4))) + + resolved_path, error = _wait_for_local_directory( + monkeypatch, release, timeout=1, expected=expected + ) + + assert resolved_path == release + assert error is None + + +def test_a_single_file_still_syncing_is_not_accepted(monkeypatch, tmp_path): + book = tmp_path / "book.epub" + book.write_bytes(b"part") + + resolved_path, error = _wait_for_local_directory( + monkeypatch, book, timeout=1, expected=_expected(("book.epub", 4000)) + ) + + assert resolved_path is None + assert error == "Completed download is not fully on disk yet (0 of 1 files)." + + +def test_a_single_file_at_its_final_size_is_accepted(monkeypatch, tmp_path): + book = tmp_path / "book.epub" + book.write_bytes(b"x" * 4000) + + resolved_path, error = _wait_for_local_directory( + monkeypatch, book, timeout=1, expected=_expected(("book.epub", 4000)) + ) + + assert resolved_path == book + assert error is None + + +def test_a_client_without_a_file_list_keeps_the_old_behaviour(monkeypatch, tmp_path): + release, _ = _batch_release(tmp_path, 23, present=10) + + resolved_path, error = _wait_for_local_directory(monkeypatch, release, timeout=1, expected=None) + + assert resolved_path == release + assert error is None + + def test_wait_for_completed_path_fails_fast_for_unsafe_mapping(monkeypatch, tmp_path): import shelfmark.download.clients.base_handler as base_handler