Post-#13 follow-ups: fork version tracks upstream, qBittorrent cleanup contract

.github/workflows/fork-image.yml

@@ -25,6 +25,11 @@ concurrency:
   group: fork-image-${{ github.ref }}
   cancel-in-progress: true
 
+env:
+  # Same upstream as fork-upstream-sync.yml. Needed here for its release TAGS,
+  # which decide the base version - see the version job.
+  UPSTREAM_REPO: https://github.com/DroppedNeedle/DroppedNeedle.git
+
 jobs:
   backend:
     if: github.repository == 'alphyriver/DroppedNeedle'
@@ -83,6 +88,17 @@ jobs:
       - uses: actions/checkout@v4
         with:
           fetch-depth: 0
+      # next-fork-version.sh derives the base from the nearest reachable PLAIN
+      # upstream tag, but checkout only brings the FORK's tags and
+      # fork-upstream-sync.yml fetches upstream's branch without --tags. Upstream
+      # release tags therefore never exist here, so the base silently froze at the
+      # newest one that happened to be in the fork (v2.3.0) and every build minted
+      # another v2.3.0.postN while upstream had moved to 2.4.x. Fetch them so the
+      # base tracks what was actually merged. Tags land only in this ephemeral
+      # runner - the release job pushes the single computed tag, never --tags - so
+      # the fork's own tag namespace and its `tags: ["v*"]` trigger are untouched.
+      - name: Fetch upstream release tags
+        run: git fetch --tags --quiet "$UPSTREAM_REPO"
       - name: Compute next fork tag
         id: compute
         shell: bash

backend/repositories/qbittorrent/qbittorrent_download_client.py

@@ -11,11 +11,13 @@
 finished downloading is NEVER deleted (it must keep seeding; the import COPIES
 files, see ``TorrentStrategy``); only an incomplete torrent is removed WITH its
 partial data. ``inspect_materialization`` therefore reports no ``file_paths`` -
-a seeding torrent's bytes are not the attempt's to unlink. list_completed_files
-remaps qBittorrent's
-``content_path`` (its namespace) onto the DroppedNeedle downloads mount by
-stripping the ``save_path`` prefix, then enumerates audio files (the folder-based
-import source, D18).
+a seeding torrent's bytes are not the attempt's to unlink, so cleanup has nothing
+to remove. Note the asymmetry with ``list_completed_files``, which DOES return
+those files: the import reads them, cleanup must not delete them.
+
+list_completed_files remaps qBittorrent's ``content_path`` (its namespace) onto
+the DroppedNeedle downloads mount by stripping the ``save_path`` prefix, then
+enumerates audio files (the folder-based import source, D18).
 
 No ``from __future__ import annotations`` (the conformance test compares real
 signatures).
@@ -152,33 +154,19 @@ async def abort(self, handle: TaskHandle) -> bool:
         incomplete torrent is removed, WITH its partial data. A torrent that is
         already gone leaves nothing to stop, so that is success - returning False
         would wedge the cleanup journal in a retry loop."""
-        info = await self._find(handle)
-        if info is None:
-            return True
-        if info.progress >= 1.0:
-            logger.info(
-                "qbittorrent: leaving completed torrent %s seeding (no delete)", info.hash
-            )
-            return True
-        return await self._client.delete_torrents(info.hash, delete_files=True)
+        return await self._remove_unless_seeding(handle, action="abort")
 
     async def inspect_materialization(
         self, handle: TaskHandle
     ) -> DownloadMaterialization:
         """Resolve current torrent state and the local content path.
 
-        ``file_paths`` is deliberately left EMPTY even for a completed torrent.
-        Those bytes belong to the seeding torrent, not to the attempt, and the
-        cleanup journal unlinks every path reported here. ``workspace_path``
-        still carries the location as evidence."""
+        Every return goes through ``_materialization``, which structurally cannot
+        report ``file_paths`` - see its docstring for why that matters."""
         info = await self._find(handle)
         healthy = await self.downloads_mount_healthy()
         if info is None:
-            return DownloadMaterialization(
-                state="missing",
-                mount_root=str(self._mount),
-                mount_healthy=healthy,
-            )
+            return self._materialization(state="missing", mount_healthy=healthy)
         state = info.state.lower()
         if state in _FAILED_STATES:
             resolved = "failed"
@@ -187,12 +175,11 @@ async def inspect_materialization(
         else:
             resolved = "active"
         local = self._local_path(info) if info.content_path else None
-        return DownloadMaterialization(
+        return self._materialization(
             state=resolved,
+            mount_healthy=healthy,
             remote_storage=info.content_path or "",
-            mount_root=str(self._mount),
             workspace_path=str(local) if local is not None else "",
-            mount_healthy=healthy,
         )
 
     async def discard_client_artifacts(self, handle: TaskHandle) -> bool:
@@ -201,16 +188,10 @@ async def discard_client_artifacts(self, handle: TaskHandle) -> bool:
         For qBittorrent the "record" IS the live seeding session, so a completed
         torrent is retained on purpose and reported as success - there is nothing
         left that the attempt owns. An incomplete torrent has no seeding value, so
-        it is removed with its partial data (same rule as ``abort``)."""
-        info = await self._find(handle)
-        if info is None:
-            return True
-        if info.progress >= 1.0:
-            logger.info(
-                "qbittorrent: retaining completed torrent %s for seeding", info.hash
-            )
-            return True
-        return await self._client.delete_torrents(info.hash, delete_files=True)
+        it is removed with its partial data (same rule as ``abort``). A FAILED
+        torrent reaches this without an ``abort`` first - the cleanup service only
+        aborts an ``active`` one - so the removal branch is load-bearing here."""
+        return await self._remove_unless_seeding(handle, action="discard")
 
     async def list_completed_files(self, handle: TaskHandle) -> list[Path]:
         info = await self._find(handle)
@@ -281,6 +262,47 @@ def _ok() -> bool:
 
     # --- internals --------------------------------------------------------------
 
+    def _materialization(
+        self,
+        *,
+        state: str,
+        mount_healthy: bool,
+        remote_storage: str = "",
+        workspace_path: str = "",
+    ) -> DownloadMaterialization:
+        """The ONLY construction point for this adapter's ``DownloadMaterialization``.
+
+        There is deliberately no ``file_paths`` parameter. The acquisition cleanup
+        journal unlinks every path reported in ``file_paths``, and a seeding
+        torrent's bytes are never the attempt's to remove - the import COPIES them
+        out and the torrent keeps seeding under its category. Leaving the field out
+        of the signature means a caller cannot reintroduce it one return-path at a
+        time; anyone who needs to must change this factory and read this note first.
+        ``workspace_path`` still carries the location as cleanup evidence."""
+        return DownloadMaterialization(
+            state=state,
+            remote_storage=remote_storage,
+            mount_root=str(self._mount),
+            workspace_path=workspace_path,
+            mount_healthy=mount_healthy,
+        )
+
+    async def _remove_unless_seeding(self, handle: TaskHandle, *, action: str) -> bool:
+        """Shared body of ``abort``/``discard_client_artifacts`` (mirrors slskd's
+        ``_remove_transfer_records``): remove an incomplete torrent WITH its partial
+        data, never touch a completed one, and treat "already gone" as success."""
+        info = await self._find(handle)
+        if info is None:
+            return True
+        if info.progress >= 1.0:
+            logger.info(
+                "qbittorrent: %s left completed torrent %s seeding (no delete)",
+                action,
+                info.hash,
+            )
+            return True
+        return await self._client.delete_torrents(info.hash, delete_files=True)
+
     async def _recover(
         self, request: EnqueueRequest, correlation_id: str
     ) -> QbtTorrentInfo | None:

backend/services/native/acquisition_cleanup_service.py

@@ -315,20 +315,10 @@ async def _cleanup_source(self, attempt: DownloadAttempt) -> None:
             )
             return
 
-        if attempt.source == "torrent":
-            # A seeding torrent owns its bytes - the import COPIED them out, so the
-            # attempt has no local source to remove and must never reach the slskd
-            # unlink path below. discard_client_artifacts retains a completed torrent
-            # (seeding is a tracker obligation) and removes an incomplete one.
-            try:
-                discarded = await client.discard_client_artifacts(handle)
-            except Exception as error:  # noqa: BLE001 - repository errors stay internal
-                raise _RetryableCleanup("client_artifact_discard_failed") from error
-            if not discarded:
-                raise _RetryableCleanup("client_artifact_discard_failed")
-            await self._mark_complete(attempt)
-            return
-
+        # Every other source is evidence-driven: remove exactly the local paths the
+        # client reported as attempt-owned, then discard its records. A torrent
+        # reports none (see QbittorrentDownloadClient._materialization), so its
+        # seeded bytes are left in place without needing a source special-case here.
         await self._cleanup_slskd_files(
             attempt,
             mount_healthy=materialization.mount_healthy,

backend/tests/repositories/test_qbittorrent.py

@@ -27,12 +27,12 @@ def _info(**kw) -> QbtTorrentInfo:
     return QbtTorrentInfo(**base)
 
 
-def _client(infos=None):
+def _client(infos=None, mount=Path("/qbittorrent-downloads")):
     api = AsyncMock()
     api.torrents_info.return_value = infos if infos is not None else []
     api.delete_torrents.return_value = True
     return QbittorrentDownloadClient(
-        api, "http://qbt:8080", "api-key", Path("/qbittorrent-downloads")
+        api, "http://qbt:8080", "api-key", Path(mount)
     ), api
 
 
@@ -168,6 +168,49 @@ async def test_inspect_materialization_reports_no_attempt_owned_file_paths():
     assert result.mount_root == "/qbittorrent-downloads"
 
 
+@pytest.mark.asyncio
+@pytest.mark.parametrize(
+    ("state", "progress"),
+    [
+        ("uploading", 1.0),
+        ("stalledup", 1.0),
+        ("downloading", 0.4),
+        ("stalleddl", 0.0),
+        ("error", 0.4),
+        ("missingfiles", 0.9),
+        ("checkingup", 1.0),
+    ],
+)
+async def test_inspect_materialization_never_reports_file_paths(
+    state, progress, tmp_path
+):
+    """The cleanup journal unlinks every path in ``file_paths``; a seeding
+    torrent's bytes are never the attempt's to remove. Enforced structurally by
+    ``_materialization`` having no such parameter - pinned here across every
+    state so a regression cannot slip in behind one branch.
+
+    The mount is a REAL populated directory: with a non-existent path this test
+    would pass even if the adapter started enumerating the content folder."""
+    mount = tmp_path / "qbittorrent-downloads"
+    album = mount / "Album"
+    album.mkdir(parents=True)
+    (album / "01.flac").write_bytes(b"seeded")
+    client, _ = _client([_info(state=state, progress=progress)], mount=mount)
+    result = await client.inspect_materialization(_HANDLE)
+    assert result.file_paths == []
+    assert result.workspace_path == str(album)
+
+
+def test_materialization_factory_has_no_file_paths_parameter():
+    """Structural guard: the single construction point must not grow a
+    ``file_paths`` argument, or the invariant becomes opt-in again."""
+    import inspect
+
+    client, _ = _client()
+    params = inspect.signature(client._materialization).parameters
+    assert "file_paths" not in params
+
+
 @pytest.mark.asyncio
 async def test_inspect_materialization_missing_torrent():
     client, _ = _client([])

backend/tests/services/native/test_acquisition_cleanup_service.py

@@ -954,3 +954,128 @@ async def test_reconciliation_refuses_unsafe_mounts(tmp_path: Path, mount: Path)
     )
 
     assert await service.reconcile_legacy_mount() == 0
+
+
+@pytest.mark.asyncio
+async def test_real_qbittorrent_client_drives_cleanup_without_touching_seeded_bytes(
+    tmp_path: Path,
+):
+    """Integration: the REAL QbittorrentDownloadClient through the real cleanup
+    service. Proves the abort/inspect/discard contract wired end to end, not a
+    fake standing in for it - a completed torrent keeps both its files and its
+    qBittorrent record, and the attempt still reaches 'complete'."""
+    from unittest.mock import AsyncMock
+
+    from repositories.qbittorrent.qbittorrent_download_client import (
+        QbittorrentDownloadClient,
+    )
+    from repositories.qbittorrent.qbittorrent_models import QbtTorrentInfo
+
+    mount = tmp_path / "qbittorrent-downloads"
+    album = mount / "Album"
+    album.mkdir(parents=True)
+    seeded = album / "01.flac"
+    seeded.write_bytes(b"seeded-bytes")
+
+    api = AsyncMock()
+    api.torrents_info.return_value = [
+        QbtTorrentInfo(
+            hash="abc123",
+            name="droppedneedle-t1-0",
+            state="uploading",
+            progress=1.0,
+            size=12,
+            downloaded=12,
+            category="droppedneedle",
+            tags="droppedneedle-t1-0",
+            content_path="/data/torrents/droppedneedle/Album",
+            save_path="/data/torrents/droppedneedle",
+        )
+    ]
+    api.delete_torrents.return_value = True
+    client = QbittorrentDownloadClient(api, "http://qbt:8080", "key", mount)
+
+    store = _store(tmp_path)
+    attempt = await store.create_download_attempt(
+        task_id="b" * 32,
+        source="torrent",
+        candidate_index=0,
+        job_name="",
+        handle=TaskHandle(source="torrent", torrent_hash="abc123"),
+        now=1.0,
+    )
+    attempt = await store.schedule_download_attempt_cleanup(
+        attempt.id, disposition="discard", publisher_bundle_ids=[], now=2.0
+    )
+
+    service = AcquisitionCleanupService(
+        store, _LibraryStore(), lambda source: client, lambda: mount
+    )
+    await service.cleanup_now(attempt.id, worker_id="test")
+
+    # The seeding torrent keeps its bytes AND its client record.
+    assert seeded.read_bytes() == b"seeded-bytes"
+    api.delete_torrents.assert_not_awaited()
+    # ...and the cleanup debt is still discharged rather than retried forever.
+    final = await store.get_download_attempt(attempt.id)
+    assert final is not None
+    assert final.state == "complete"
+    # Evidence was recorded from the remapped local path, not qBittorrent's namespace.
+    assert final.workspace_path == str(album)
+    assert final.materialized_paths == []
+
+
+@pytest.mark.asyncio
+async def test_failed_torrent_is_removed_with_its_partial_data(tmp_path: Path):
+    """A FAILED torrent never reaches ``abort`` (the service aborts only 'active'),
+    so ``discard_client_artifacts`` is what removes it and its partial bytes."""
+    from unittest.mock import AsyncMock
+
+    from repositories.qbittorrent.qbittorrent_download_client import (
+        QbittorrentDownloadClient,
+    )
+    from repositories.qbittorrent.qbittorrent_models import QbtTorrentInfo
+
+    mount = tmp_path / "qbittorrent-downloads"
+    mount.mkdir(parents=True)
+
+    api = AsyncMock()
+    api.torrents_info.return_value = [
+        QbtTorrentInfo(
+            hash="dead01",
+            name="droppedneedle-t2-0",
+            state="error",
+            progress=0.3,
+            size=100,
+            downloaded=30,
+            category="droppedneedle",
+            tags="droppedneedle-t2-0",
+            content_path="/data/torrents/droppedneedle/Broken",
+            save_path="/data/torrents/droppedneedle",
+        )
+    ]
+    api.delete_torrents.return_value = True
+    client = QbittorrentDownloadClient(api, "http://qbt:8080", "key", mount)
+
+    store = _store(tmp_path)
+    attempt = await store.create_download_attempt(
+        task_id="c" * 32,
+        source="torrent",
+        candidate_index=0,
+        job_name="",
+        handle=TaskHandle(source="torrent", torrent_hash="dead01"),
+        now=1.0,
+    )
+    attempt = await store.schedule_download_attempt_cleanup(
+        attempt.id, disposition="discard", publisher_bundle_ids=[], now=2.0
+    )
+
+    service = AcquisitionCleanupService(
+        store, _LibraryStore(), lambda source: client, lambda: mount
+    )
+    await service.cleanup_now(attempt.id, worker_id="test")
+
+    api.delete_torrents.assert_awaited_once_with("dead01", delete_files=True)
+    final = await store.get_download_attempt(attempt.id)
+    assert final is not None
+    assert final.state == "complete"