diff --git a/.github/workflows/fork-image.yml b/.github/workflows/fork-image.yml index 008f407d7..13bede695 100644 --- a/.github/workflows/fork-image.yml +++ b/.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 diff --git a/backend/repositories/qbittorrent/qbittorrent_download_client.py b/backend/repositories/qbittorrent/qbittorrent_download_client.py index 37fc797f7..932177e5d 100644 --- a/backend/repositories/qbittorrent/qbittorrent_download_client.py +++ b/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: diff --git a/backend/services/native/acquisition_cleanup_service.py b/backend/services/native/acquisition_cleanup_service.py index 738d19810..731d868dd 100644 --- a/backend/services/native/acquisition_cleanup_service.py +++ b/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, diff --git a/backend/tests/repositories/test_qbittorrent.py b/backend/tests/repositories/test_qbittorrent.py index 53c62d9f9..9418a58bc 100644 --- a/backend/tests/repositories/test_qbittorrent.py +++ b/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([]) diff --git a/backend/tests/services/native/test_acquisition_cleanup_service.py b/backend/tests/services/native/test_acquisition_cleanup_service.py index c7a78486c..4d0bb4a54 100644 --- a/backend/tests/services/native/test_acquisition_cleanup_service.py +++ b/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"