fix(clients): import a qBittorrent torrent only when every listed file is on disk (#1577)

Fixes #1561, with thanks to @tmussche for the write-up, which had the
cause and the fix. When the files reach Shelfmark through a sync
(Resilio in the report), the wait for the completed path accepts the
folder once two samples a few seconds apart look the same. A sync that
copies in batches can pause longer than that, so 10 of 23 files were
imported and some of those were still partial copies.

For qBittorrent the client can say what the finished download holds, so
the wait now checks that. `DownloadClient` gets an optional
`get_expected_files()` that returns `ExpectedFile(name, size)` entries,
or None when the client cannot say, which is the default and keeps every
other client as it was. qBittorrent answers it from
`/api/v2/torrents/files`, leaving out files the torrent does not
download (priority 0), zero-byte entries and BEP 47 padding files.
`_wait_for_completed_path` asks once, and while any of those files is
missing or smaller than its final size it keeps waiting, using the same
timeout as before. When the wait runs out the error now says how far it
got, for example "Completed download is not fully on disk yet (10 of 23
files)." Once everything is there the existing stability check still
runs, so nothing is accepted that was accepted before.

Names from the client start with the torrent's top-level folder, so each
file is looked up with and without it, which also covers a torrent saved
without a subfolder. A single-file torrent is checked by size too, since
a file that is still syncing exists at a smaller size.

Two things I would like a second look at: the comparison is on exact
size, so a setup where something rewrites the files after the torrent
finishes would never match and would run into the timeout; and the file
list is read once per wait, not on every poll. If the client does not
answer, nothing changes.

I also moved the nested authenticated GET in the path derivation into a
`_get_with_auth` method so both calls share it, and added a sentence
about this to `docs/configuration.md`. Tests: eight for the wait
(partial folder, truncated file, files arriving during the wait,
complete folder, no top-level folder, single file both ways, and a
client without a list), and six for the qBittorrent file list. The wait
tests that depend on the new behaviour fail on current main. Full suite
passes.

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)

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:

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

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()

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()."""
 

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