From 946187b8f3f5b892b1d040748c831200a6f86d3e Mon Sep 17 00:00:00 2001 From: riveria94 Date: Sat, 15 Aug 2026 23:18:15 +0800 Subject: [PATCH 1/3] refactor(qbittorrent): factor the shared abort/discard body, cover cleanup end to end Follow-up to #13. Two consistency and coverage gaps in the qBittorrent cleanup contract that landed with the upstream merge. Structure: `abort` and `discard_client_artifacts` had near-identical bodies. `SlskdRepository` already factors exactly this pair into `_remove_transfer_records`, so mirror it with `_remove_unless_seeding(handle, action=...)` in the internals section. The public methods keep their distinct docstrings and signatures (the protocol conformance test compares real signatures, not just names). Coverage: the previous tests exercised the cleanup service against a fake client. Added two integration tests that drive the REAL `QbittorrentDownloadClient` through the real `AcquisitionCleanupService` and store: - a completed torrent keeps both its files and its qBittorrent record, evidence is recorded from the remapped local path, and the attempt still reaches `complete` rather than retrying forever; - a FAILED torrent is removed with its partial data. This path never calls `abort` (the service aborts only an `active` materialization), so `discard_client_artifacts` is what removes it - now pinned, and noted in the docstring. No behaviour change: `_remove_unless_seeding` is the same logic both methods already had, and the only visible difference is one merged log line that names the action. Backend suite green (6404 passed). Ruff clean. Co-Authored-By: Claude Opus 5 --- .../qbittorrent_download_client.py | 40 +++--- .../test_acquisition_cleanup_service.py | 125 ++++++++++++++++++ 2 files changed, 146 insertions(+), 19 deletions(-) diff --git a/backend/repositories/qbittorrent/qbittorrent_download_client.py b/backend/repositories/qbittorrent/qbittorrent_download_client.py index 37fc797f7..733ea1bbf 100644 --- a/backend/repositories/qbittorrent/qbittorrent_download_client.py +++ b/backend/repositories/qbittorrent/qbittorrent_download_client.py @@ -152,15 +152,7 @@ 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 @@ -201,16 +193,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 +267,22 @@ def _ok() -> bool: # --- internals -------------------------------------------------------------- + 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/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" From 9278da6bbc9e5a52c23d43942a272a6f680afba6 Mon Sep 17 00:00:00 2001 From: riveria94 Date: Sat, 15 Aug 2026 23:57:58 +0800 Subject: [PATCH 2/3] refactor(cleanup): enforce the torrent no-unlink invariant at the adapter The `if attempt.source == "torrent"` branch added in #13 was redundant: removing it leaves every cleanup test passing, because `_cleanup_slskd_files` iterates `materialized_paths` and a torrent reports none. It duplicated 14 lines of the generic fall-through and added a fourth source conditional to a service upstream rewrote by ~700 lines this sync - a recurring conflict for no behaviour. Removed it. Cleanup is now uniformly evidence-driven: unlink exactly the local paths the client reported as attempt-owned, then discard its records. The invariant that made the branch unnecessary is now enforced where it belongs. `inspect_materialization` builds its result through a single private factory, `_materialization`, which has NO `file_paths` parameter - so the field cannot be reintroduced one return-path at a time, and anyone who needs it must change the factory and read why it is absent. Guards, verified by deliberately regressing the adapter to enumerate the content directory: 7 parametrised adapter tests (one per qBittorrent state) plus the real-client integration test all fail on that change. The adapter tests now use a REAL populated mount - with a non-existent path they passed for the wrong reason. Backend suite green (6412 passed). Ruff clean. Co-Authored-By: Claude Opus 5 --- .../qbittorrent_download_client.py | 54 +++++++++++++------ .../native/acquisition_cleanup_service.py | 18 ++----- .../tests/repositories/test_qbittorrent.py | 47 +++++++++++++++- 3 files changed, 86 insertions(+), 33 deletions(-) diff --git a/backend/repositories/qbittorrent/qbittorrent_download_client.py b/backend/repositories/qbittorrent/qbittorrent_download_client.py index 733ea1bbf..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). @@ -159,18 +161,12 @@ async def inspect_materialization( ) -> 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" @@ -179,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: @@ -267,6 +262,31 @@ 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 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([]) From 5669f15695abe7d8a302bea0dffd0cd49caa8a80 Mon Sep 17 00:00:00 2001 From: riveria94 Date: Sun, 16 Aug 2026 00:13:39 +0800 Subject: [PATCH 3/3] fix(ci): base the fork version on upstream's tags, not the fork's stale copy The fork has been publishing v2.3.0.postN since 21 July while upstream shipped 2.4.0, 2.4.1 and 2.4.2. v2.3.0.post7 is in fact upstream v2.4.1 plus the fork's work - v2.4.1 tags 2f4d237, exactly the commit #13 merged. next-fork-version.sh is not at fault: it picks the nearest reachable PLAIN upstream tag, and its reset-on-new-upstream behaviour is already covered by test_fork_version_sequence_and_upstream_reset. The input was starved. `actions/checkout` brings only the FORK's tags, and fork-upstream-sync.yml fetches upstream with `main:refs/remotes/upstream/main` - no --tags. So upstream release tags never enter the fork, the base froze at the newest one that happened to be here (v2.3.0), and the counter kept climbing. Fetching them in the version job restores the intended behaviour: without the step: v2.3.0.post8 with the step: v2.4.1.post1 The tags land only in that ephemeral runner. The release job pushes the single computed tag (`git push origin "$TAG"`) and nothing anywhere runs `push --tags`, so the fork's published tag namespace and its `tags: ["v*"]` build trigger are unaffected. Mirroring upstream tags into the fork instead was considered and rejected: pushing v2.4.x would fire Fork Image once per upstream release. Co-Authored-By: Claude Opus 5 --- .github/workflows/fork-image.yml | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) 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