Post-#13 follow-ups: fork version tracks upstream, qBittorrent cleanup contract - #14
Merged
Merged
Conversation
…eanup 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 <noreply@anthropic.com>
|
AI review is unavailable: DeepSeek API error: DEEPSEEK_API_KEY is not set Please review this PR manually. |
…pter 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 <noreply@anthropic.com>
…le 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 <noreply@anthropic.com>
alphyriver
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Post-merge follow-ups to #13. Combines the qBittorrent cleanup-contract work with the fork-version fix, because merging the code alone would trigger Fork Image and mint another mislabelled release (supersedes #15).
1. Fork version tracks upstream again
The fork published
v2.3.0.postNfrom 21 July while upstream shipped 2.4.0/2.4.1/2.4.2.v2.3.0.post7was in fact upstream v2.4.1 plus the fork's work — v2.4.1 tags2f4d237, exactly the commit #13 merged.next-fork-version.shwas not at fault: it picks the nearest reachable plain upstream tag, and its reset-on-new-upstream behaviour is already covered bytest_fork_version_sequence_and_upstream_reset. Its input was starved —actions/checkoutbrings only the fork's tags, andfork-upstream-sync.ymlfetches upstream asmain:refs/remotes/upstream/mainwith no--tags. Upstream release tags never entered the fork, so the base froze at v2.3.0 and the counter climbed.Fetching them in the
versionjob restores intended behaviour. Verified by running the exact CI sequence:Tags land only in that ephemeral runner; the release job pushes the single computed tag and nothing runs
push --tags, so the fork's published tag namespace and itstags: ["v*"]trigger are untouched. Mirroring upstream tags into the fork was considered and rejected — pushingv2.4.xwould fire Fork Image once per upstream release.2. qBittorrent cleanup contract — consistency
abortanddiscard_client_artifactshad near-identical bodies.SlskdRepositoryalready factors that exact pair into_remove_transfer_records, so this mirrors it with_remove_unless_seeding(handle, action=...). All three adapters now match exactly: same 12 members, same kinds, signatures identical toDownloadClientProtocol.3. The no-unlink invariant moved to the adapter
The
if attempt.source == "torrent"branch added in #13 is removed. It was byte-for-byte the generic fall-through minus_cleanup_slskd_files— itself a no-op whenmaterialized_pathsis empty — verified by deleting it and watching every cleanup test still pass. It bought nothing and added a fourth source conditional to a service upstream rewrote ~700 lines of this sync.The invariant now lives where it belongs:
inspect_materializationroutes every return through a single_materializationfactory with nofile_pathsparameter, so it cannot be reintroduced one return-path at a time. Cleanup stays purely evidence-driven — unlink exactly what the client reported, nothing more.Guards, verified by deliberately regressing the adapter
Made
_materializationenumerate the content directory; 8 tests fail:test_inspect_materialization_never_reports_file_pathsacross 7 torrent states, against a real populated mount (as first written it used a non-existent path and passed for the wrong reason);test_materialization_factory_has_no_file_paths_parameterpins the signature;Plus a second integration test for the failed torrent path, which never calls
abort(the service aborts only anactivematerialization) — sodiscard_client_artifactsis what removes it.Tests
Backend suite green (6412 passed). Ruff clean. Frontend untouched.
🤖 Generated with Claude Code