Repository navigation
fix: address deletion safety, persistence and action issues #42–#52 - #53
Conversation
|
Implementation and final validation are complete for issues #42–#52 at
Review the explicit boundaries in the description: #49 uses one bounded complete catalogue when the supported Jellyfin API cannot filter provider IDs; #52 implements durable retention visibility, without automatic deferred removal. Shared-data checks use a consistent reported path namespace and do not resolve remote inode/mount aliases. Fresh real-service compatibility certification and published-image upgrade rehearsals remain release gates. No merge, tag, release or production change has been performed. The source checkout is clean and the linked issues remain open pending review/merge. |
mambastick
left a comment
mambastick
left a comment
There was a problem hiding this comment.
Review notes from Claude Code (high-effort pass over the diff; findings come from reading the code and were not reproduced against real services).
Overall the PR addresses #42–#52 well: the UUID fallback, preserving executed actions after late Seerr failures, blocking automatic replay after a partial mutation, the identifier-consistency check and the schema-7 migration coverage look solid.
The main concern is that several new fail-closed bounds trigger in normal installs and effectively disable deletion: one bad or stopped torrent anywhere blocks the whole fleet, the 1000-item Jellyfin cap affects webhook cascades, and the 1000-movie Radarr cap blocks every movie deletion. Details are inline.
| if ( | ||
| not isinstance(inventory.scopes, tuple) | ||
| or not isinstance(inventory.failures, tuple) | ||
| or inventory.failures |
There was a problem hiding this comment.
[High] One malformed torrent scope blocks data removal fleet-wide.
Any single failures entry in any client's inventory makes the whole assessment complete=False, and MultiDownloaderClient then marks every hash on every target ownership_unverifiable.
Scenario: one stalled magnet without metadata (Transmission/Deluge return files: [] → invalid_content_scope, downloaders.py:445), or one qBittorrent torrent with a Windows or 2-segment content_path, sits on any configured client. After that, no movie, series or season can be deleted anywhere until that unrelated torrent is fixed.
Suggestion: a scope with no files holds no data and cannot overlap, so it could be ignored. Otherwise, limit the impact of a failure to the client it came from, or to paths it could plausibly affect, instead of the whole fleet.
There was a problem hiding this comment.
Addressed in 9affed009576560d3c1be0ee50853b3f2d949832. Incomplete metadata with a validated root now creates a bounded uncertain scope, so it blocks overlapping targets rather than the whole fleet. Transmission/Deluge metadata-less magnets, and qBittorrent metaDL entries, are excluded from overlap only when strict protocol counters prove zero local payload; files: [] alone is never accepted as proof. Valid shallow POSIX inventory paths are supported.
Regressions cover unrelated uncertain roots, same-root zero-payload magnets, missing/nonzero/boolean counters, and overlap blocking. Truly unbounded inventories (including unresolved Windows/UNC namespace or escaping file paths) still block fleet data removal: isolating those by client would miss cross-client shared data. This remaining boundary is explicit in EN/RU Safety docs.
Validation: 471 backend tests pass (7 real-service tests skipped), 176 frontend unit tests and 40 Chromium scenarios pass; all five required CI jobs are green on this exact commit. Ready for the designated review/merge agent to re-check.
| failures.append(DownloaderContentInventoryFailure("invalid_content_scope")) | ||
| continue | ||
| try: | ||
| path = await self._data_path(info_hash, default_root=default_root) |
There was a problem hiding this comment.
[High] Closed or stopped rTorrent torrents make the fleet unverifiable.
rTorrent reports an empty d.base_path for downloads that are not open (stopped/closed). With the directory fallback removed, _data_path raises for every such torrent, the inventory gets an invalid_content_scope failure, and (see the data_safety.py:87 comment) every deletion on every client is blocked. test_rtorrent_content_inventory_rejects_missing_base_path codifies this behavior.
Stopped torrents are routine, so data removal becomes effectively unavailable for rTorrent users. Consider deriving the scope for closed torrents from d.directory / d.name / d.is_multi_file (still requiring it to be strictly below directory.default) for inventory purposes only, while keeping the strict non-empty base_path requirement for the rm -rf target itself.
There was a problem hiding this comment.
Addressed in 9affed009576560d3c1be0ee50853b3f2d949832. Closed rTorrent inventory derives a conservative child scope from validated d.directory, d.name and a strict boolean/0/1 d.is_multi_file, below the current directory.default. It cannot authorize deletion of that closed torrent. Actual rm still requires a nonempty validated d.base_path and an exact match to the inventory snapshot; an outside-root nonempty base path is never replaced by an inside-root guess.
Protocol/scope tests cover closed inventory, outside-root evidence, invalid flags, root refusal and changed-path rejection.
Validation: 471 backend tests pass (7 real-service tests skipped), 176 frontend unit tests and 40 Chromium scenarios pass; all five required CI jobs are green on this exact commit. Ready for the designated review/merge agent to re-check.
| total = payload.get("TotalRecordCount") | ||
| if total is None or isinstance(total, bool) or not isinstance(total, int) or total < 0: | ||
| raise ExternalServiceError("jellyfin", "Jellyfin returned invalid targeted lookup metadata.") | ||
| if not ids and total > _JELLYFIN_TARGETED_PROVIDER_MAX_ITEMS: |
There was a problem hiding this comment.
[High] The webhook cascade always fails closed for libraries with more than 1000 items of the type.
When an item is really deleted, the exact-ID lookup finds nothing, so the provider fallback runs. It reads the whole type catalogue with Limit=1000 and raises when TotalRecordCount > 1000, and _confirm_source_absent returns source_unverifiable.
Almost every TV library has more than 1000 Episode items, and many movie libraries have more than 1000 Movie items, so webhook-driven deletion is effectively disabled for typical installs. Before this PR, a bounded paginated scan (20k items) worked, so the #49 performance fix turns into a functional regression.
Suggestion: keep the fast Ids lookup, but for the provider phase fall back to the previous bounded pagination (_JELLYFIN_LIBRARY_MAX_ITEMS) instead of failing at 1000.
There was a problem hiding this comment.
Addressed in 9affed009576560d3c1be0ee50853b3f2d949832. The exact-ID fast lookup remains; provider fallback now uses complete validated pagination up to the existing 20,000-item bound. The new 1,000-item rejection is removed. Counts, duplicate IDs, item types, incomplete/changed pages and the existing upper bound still fail closed.
A 1,001-item catalogue regression finds a provider match on the third page. EN/RU docs and this PR description now describe the actual fallback and its performance boundary; a provider-filter optimization unsupported by the targeted Jellyfin API is not claimed.
Validation: 471 backend tests pass (7 real-service tests skipped), 176 frontend unit tests and 40 Chromium scenarios pass; all five required CI jobs are green on this exact commit. Ready for the designated review/merge agent to re-check.
| """Retain the entire cascade unless all configured Radarr histories isolate the hash.""" | ||
| if not hashes: | ||
| return True # The normal missing-hash guard below still blocks deletion. | ||
| if len(movies) > 1000: |
There was a problem hiding this comment.
[High] Radarr libraries with more than 1000 movies can never delete a movie.
len(movies) > 1000 → OWNERSHIP_UNVERIFIABLE → the plan is not confirmable (unsafe_plan), and webhook cascades are blocked too. Libraries of this size are common, and this limit is not mentioned in SAFETY.md, which only says the hash must be absent from every other movie's history.
Suggestion: Radarr v3+ supports filtering GET /api/v3/history?downloadId=<hash>. One request per hash per configured instance removes both the cap and the per-movie fan-out (see the comment on line 725).
There was a problem hiding this comment.
Addressed in 9affed009576560d3c1be0ee50853b3f2d949832 together with the request fan-out finding. Movie ownership now queries /history?downloadId=… once per normalized hash per configured Radarr instance, with complete bounded pagination, rather than requesting every other movie's history. The 1,000-movie cap is removed. Routed movie IDs preserve instance ownership; any other movie reference still blocks the pack.
The filter was verified in the official Radarr controller. Tests cover a catalogue over 1,000 movies, all-instance fan-out, URL encoding, shared hashes, and incomplete/invalid filtered history.
Validation: 471 backend tests pass (7 real-service tests skipped), 176 frontend unit tests and 40 Chromium scenarios pass; all five required CI jobs are green on this exact commit. Ready for the designated review/merge agent to re-check.
| self._seerr_resolver = StrictSeerrResolver() | ||
|
|
||
| @abstractmethod | ||
| async def handle(self, event: MediaDeletionEvent) -> ProcessingResult: |
There was a problem hiding this comment.
[Medium] A transient outage now permanently fails queued jobs as "plan changed" instead of retrying.
The new catch-all turns pre-mutation infrastructure errors (e.g. list_movies failing because Radarr is briefly down) into a returned plan with process_event FAILED instead of an exception. In ManualDeletionJobService._run_job (deletion_jobs.py:418), the re-check then sees a hash mismatch and calls _mark_failed("The deletion plan changed while the job was queued…") with error_code=plan_changed and no retry. Before this PR, the exception reached _retry_or_fail during the PLANNING phase and was retried.
Batches change in a similar way: preflight_failed becomes unsafe_plan.
Suggestion: only swallow exceptions once a mutation has been attempted, or have the job distinguish "re-check returned FAILED actions" from a real plan change and retry the former.
There was a problem hiding this comment.
Addressed in 9affed009576560d3c1be0ee50853b3f2d949832. The worker recognizes failed preflight actions before comparing plan hashes and enters its bounded retry path with preflight_failed. A healthy changed plan still gives plan_changed; batches classify failed preflight evidence as preflight_failed, preserving unsafe_plan for genuine safety outcomes.
A recovery regression fails the first worker re-check, succeeds on the second, verifies attempt_count == 2, exactly one mutation, and clearing the persisted error code on success. Existing post-mutation no-replay tests remain green.
Validation: 471 backend tests pass (7 real-service tests skipped), 176 frontend unit tests and 40 Chromium scenarios pass; all five required CI jobs are green on this exact commit. Ready for the designated review/merge agent to re-check.
| return any(record.download_id and record.download_id.upper() in hashes for record in records) | ||
|
|
||
| try: | ||
| shared = any(await asyncio.gather(*(references(candidate) for candidate in movies))) |
There was a problem hiding this comment.
[Medium / efficiency] One Radarr history request per movie on every movie plan.
list_movie_history is called for every movie in every instance (concurrency 4), and this happens on preview, on the submit preflight, on the worker re-check and in the live run. With 900 movies, one manual deletion costs about 3,600 history requests (pageSize 1000 each), and part of that runs while the global execution lock is held. A movie batch multiplies it.
Also, asyncio.gather without return_exceptions leaves the remaining requests running after the first failure.
Using a downloadId-filtered history lookup, or reading histories once per cascade, would avoid this.
There was a problem hiding this comment.
Addressed in 9affed009576560d3c1be0ee50853b3f2d949832. The per-movie history fan-out is replaced by the complete downloadId-filtered lookup across all instances. Typical single-hash evidence is one request per instance (additional pages only when that hash's own history requires them). The aggregate drains all started instance reads before raising; the strategy cancels and drains pending hash tasks on failure/cancellation.
Request-count, encoded ownership and draining regressions cover the new boundary.
Validation: 471 backend tests pass (7 real-service tests skipped), 176 frontend unit tests and 40 Chromium scenarios pass; all five required CI jobs are green on this exact commit. Ready for the designated review/merge agent to re-check.
| return [] | ||
|
|
||
| if delete_files: | ||
| safety_results = await self._content_safe_results(normalized) |
There was a problem hiding this comment.
[Medium / efficiency] The full content inventory is read twice per delete_hashes call.
_content_safe_results reads every client's full inventory. When there is no conflict, each client's own delete_hashes then reads its own full inventory again (lines 374 / 753 / 1042 and the qBittorrent equivalent).
Transmission and Deluge inventories list every file of every torrent (up to 200k paths), and rTorrent does one RPC per torrent. Preview, submit and the re-check each call delete_hashes(dry_run=True), so a single deletion triggers about 8 full scans per client.
Suggestion: pass the fleet assessment down, or skip the per-client re-read when called through the multi-client wrapper.
There was a problem hiding this comment.
Addressed in 9affed009576560d3c1be0ee50853b3f2d949832. The fleet assessment is passed through a private per-call adapter handoff for all four engines. Each adapter reuses that immutable inventory snapshot instead of reading its full inventory again; standalone calls still obtain their own safety evidence. Fresh target metadata is checked against the captured scope before mutation, including exact-path checks for qBittorrent/rTorrent.
Tests assert one full inventory per configured adapter, qBittorrent full-vs-target request counts, and rejection when the target scope changes between inventory and lookup. There is no shared/global assessment cache.
Validation: 471 backend tests pass (7 real-service tests skipped), 176 frontend unit tests and 40 Chromium scenarios pass; all five required CI jobs are green on this exact commit. Ready for the designated review/merge agent to re-check.
| continue | ||
| if field == "imdb_id": | ||
| expected, actual = expected.casefold(), actual.casefold() | ||
| elif field == "path": |
There was a problem hiding this comment.
[Low–Medium] The path veto compares Jellyfin and Arr paths verbatim.
path used to be only a weak last-resort fallback; now any difference is CONFLICTING_IDENTIFIERS. Webhook paths come from Jellyfin's namespace: usually a file path (/media/movies/X (2020)/X.mkv) and often a different mount. Arr paths are folder paths in Arr's namespace (/data/movies/X (2020)).
The README template sends "path": null, but any custom template that sends ItemPath will get every tmdb/imdb match vetoed, and all webhook cascades will be blocked.
Suggestion: apply the path veto only when both sides are known to share a namespace (e.g. the manual flow, where the fingerprint path comes from Arr itself), or compare only folder ancestry after an explicit mapping.
There was a problem hiding this comment.
Addressed in 9affed009576560d3c1be0ee50853b3f2d949832. Path conflict checks are now restricted to an internal path_namespace="arr" marker attached only by trusted manual Arr resolution and preserved through durable event encoding. Webhook input cannot set this marker, so a Jellyfin file/mount path does not veto matching provider IDs. Provider-ID contradictions still fail closed; exact-path fallback is unchanged.
Regressions cover movie/series different mount/file paths, trusted Arr path conflicts, conflicting provider IDs, forged webhook markers, and manual-event persistence.
Validation: 471 backend tests pass (7 real-service tests skipped), 176 frontend unit tests and 40 Chromium scenarios pass; all five required CI jobs are green on this exact commit. Ready for the designated review/merge agent to re-check.
| assessment = assess_content_inventory( | ||
| tuple((target.id, inventory) for target, inventory in zip(self._targets, inventories, strict=True)), hashes | ||
| ) | ||
| if not assessment.complete: |
There was a problem hiding this comment.
[Low] Blocked results claim the hash exists on every client.
On an incomplete or conflicting assessment, the wrapper creates existed=True results for every target × hash pair, including clients where the hash does not exist. The confirmed plan and the activity log then show the torrent as present and blocked on, for example, Transmission when it only lives in qBittorrent. That is false evidence in the safety plan, and it inflates action counts.
Suggestion: report existed from the actual matches, or emit one fleet-level ownership_unverifiable action.
There was a problem hiding this comment.
Addressed in 9affed009576560d3c1be0ee50853b3f2d949832. A globally unknown fleet assessment now produces one fleet-level blocked result per requested hash with client_id=None and existed=None, rather than claiming presence on every client. Known adapter results report actual matches/absence; malformed listing responses remain unknown and cannot mutate. The strategy explicitly blocks unknown existence even if no skip reason is supplied.
Truthful fleet evidence, nullable existence, malformed-list no-mutation and cascade-blocking regressions cover this.
Validation: 471 backend tests pass (7 real-service tests skipped), 176 frontend unit tests and 40 Chromium scenarios pass; all five required CI jobs are green on this exact commit. Ready for the designated review/merge agent to re-check.
| result=job.result, | ||
| error=job.error, | ||
| error_code=( | ||
| "partial_result_not_retryable" |
There was a problem hiding this comment.
[Low / maintainability] error_code is inferred from error-message prefixes.
interrupted_unknown and plan_changed are derived with job.error.startswith(...). AGENTS.md asks that backend strings not be the machine-readable contract. Rewording a message, or a redaction that changes the prefix, silently drops the code, and localizedJobError then falls back to raw English text in the RU UI.
Suggestion: store error_code on DeletionJobRecord when marking the job failed. Schema 7 is already being migrated in this PR.
There was a problem hiding this comment.
Addressed in 9affed009576560d3c1be0ee50853b3f2d949832. DeletionJobRecord.error_code is set directly on failure/retry/recovery transitions, cleared on successful completion, and persisted by ordered migration 8. Migration 7 remains unchanged. The response no longer derives codes from error text prefixes; old schema-6/7 rows remain readable with a nullable code.
Tests cover code round-trip, transient retry recovery, populated schema-7 upgrade/rerun/backup/restore and stable schema-6 upgrade. EN/RU copy and browser tests verify localized preflight errors without exposing raw service text.
Validation: 471 backend tests pass (7 real-service tests skipped), 176 frontend unit tests and 40 Chromium scenarios pass; all five required CI jobs are green on this exact commit. Ready for the designated review/merge agent to re-check.
|
Review follow-up is complete at
Unbounded/escaping/unresolved Windows/UNC inventory evidence still blocks fleet data removal because client-only isolation would miss cross-client shared data. Jellyfin provider fallback remains a complete paginated scan under the existing 20,000-item bound. No new real-service compatibility certification or automatic deferred-removal flow is claimed. Ready for the user's designated agent to review and merge. Review threads are left open for reviewer acceptance. No merge, tag, release or production change has been performed. |
mambastick
left a comment
mambastick
left a comment
There was a problem hiding this comment.
Re-review of 9affed0 by Claude Code.
All 10 findings from the previous review are addressed: the magnet and closed-torrent handling no longer blocks the whole fleet, Jellyfin uses a bounded paginated catalogue again, Radarr ownership uses a hash-scoped downloadId lookup, transient re-check failures retry as preflight_failed, the inventory is read once per operation, the path veto is namespace-gated, existed is no longer fabricated, and error_code is persisted (schema 8). Backend suite on this commit: 471 passed, 7 skipped.
Blocking: the new Radarr exclusivity validation rejects the negative routed IDs that MultiRadarrClient always produces, so every movie deletion in production fails as ownership_unverifiable (reproduced, details inline). The other comments are edge cases where the new fail-closed paths still trigger in normal setups, plus two classification/UX issues. Comments marked plausible were derived from the code and were not run against real services.
| if ( | ||
| not isinstance(record, RadarrHistoryRecord) | ||
| or isinstance(record.movie_id, bool) | ||
| or record.movie_id <= 0 |
There was a problem hiding this comment.
[Critical, reproduced] Routed Radarr movie IDs are rejected as malformed, so every movie deletion is blocked.
In production, Radarr is always wrapped in MultiRadarrClient whenever at least one Radarr instance is enabled (container.py:197). The router encodes every movie and history ID as a negative routed ID (_encode_id), including in the new list_download_history. The check record.movie_id <= 0 therefore raises TypeError("Radarr returned malformed hash-scoped history records.") → verify_download_ownership FAILED / ownership_unverifiable → the Radarr delete and Seerr cleanup are skipped.
Reproduced on 9affed0 with a single-instance MultiRadarrClient wrapping a fake Radarr that has one grabbed hash:
partial_failure [('radarr', 'verify_download_ownership', 'failed', ownership_unverifiable,
'Radarr returned malformed hash-scoped history records.'),
('radarr', 'delete_movie', 'skipped', downstream_error),
('seerr', 'cleanup_movie', 'skipped', downstream_error)]
The suite stays green (471 passed) because the strategy tests use raw fakes rather than the router.
Suggestion: drop the <= 0 check (or only reject 0), keep the record.movie_id != movie_id comparison in routed-ID space, and add a scenario test that runs MovieDeletionStrategy through MultiRadarrClient with one and with two instances.
There was a problem hiding this comment.
Fixed in ec6dba9. The guard now accepts nonzero integer IDs in the router's identity space; raw external Radarr IDs remain strictly positive at the HTTP adapter boundary. Added MovieDeletionStrategy scenarios through the actual MultiRadarrClient with one and two instances, a shared hash with colliding raw movie IDs, and malformed zero/bool/float/string identities. Normal routed deletion succeeds, while shared or malformed evidence preserves all downstream data.
| if not info_hash: | ||
| failures.append(DownloaderContentInventoryFailure("invalid_content_scope")) | ||
| continue | ||
| base_path = await self._rpc("d.base_path", info_hash) |
There was a problem hiding this comment.
[Medium, plausible] The rTorrent inventory makes sequential per-torrent RPCs under a 10-second budget.
list_content_inventory makes at least one awaited XML-RPC call per torrent (d.base_path), plus three more for closed torrents, one after another. read_content_inventory wraps the whole read in asyncio.wait_for(..., CONTENT_INVENTORY_TIMEOUT_SECONDS=10), and a timeout makes the fleet assessment incomplete.
Scenario: a seedbox rTorrent with about 1,500 torrents at about 10 ms RTT needs ≥15 s for d.base_path alone → timeout → inventory None → complete=False → every deletion on every client returns ownership_unverifiable.
Suggestion: fetch d.hash, d.base_path, d.directory, d.name and d.is_multi_file with a single d.multicall2 (or batch the calls via system.multicall).
There was a problem hiding this comment.
Fixed in ec6dba9. rTorrent inventory now uses one d.multicall2 for hash/base_path/directory/name/is_multi_file plus directory.default, so inventory RPC count is constant. Rows, canonical unique hashes, scope paths and the existing 20,000-scope bound are validated; malformed or failed evidence remains fail-closed. A 1,500-entry protocol regression verifies exactly these two calls and no per-torrent inventory RPCs. The 10-second fleet budget is preserved.
| raw_path = item.get("content_path") | ||
| path = validate_scope_path(raw_path, minimum_parts=2) | ||
| if info_hash and raw_path in (None, ""): | ||
| if item.get("state") == "metaDL" and all( |
There was a problem hiding this comment.
[Medium, plausible — please verify on real qBittorrent] metaDL detection relies on total_size == 0.
As far as I know, before metadata arrives qBittorrent reports total_size = -1 (TorrentInfo::totalSize() of an invalid torrent info), not 0. Such a torrent then falls through to an uncertain scope rooted at save_path, which is an ancestor of every torrent in that category directory → every deletion there becomes shared_path. That is the same per-category block this fix was meant to remove. The test fixture uses total_size: 0.
Also, the request on line 1319 asks for fields=hash,infohash_v1,infohash_v2,content_path, which omits state, total_size, completed, downloaded and save_path, all of which this branch reads. Current qBittorrent appears to ignore fields, but if any version or proxy honors it, every metaDL torrent becomes an inventory failure.
Suggestion: treat state == "metaDL" with a negative or zero size and zero progress as empty, and request the fields that are actually used.
There was a problem hiding this comment.
Verified on real qBittorrent 5.2.4 and fixed in ec6dba9. A synthetic metadata-free magnet actually returned state=metaDL, total_size=-1, completed=0, downloaded=0, progress=0 and content_path="". The official Arch package was signature-verified and extracted into a separate disposable profile; no owner torrents or production data were used and the process was stopped after capture. SHA-256: dc1b05fb4d22388e08e312f66336577eb6e9f99c9ae127b05e87c3caedd96f8a.
The empty-scope proof accepts only the documented size sentinel -1 or zero, with strict zero integer completed/downloaded counters and finite numeric zero progress in metaDL. The request now includes every consumed field, plus the sibling display name. Tests cover -1, zero, invalid progress/state and a proxy that actually projects requested fields. Real 5.2.4 returned a superset; this is a focused payload observation, not a new full compatibility certification.
| raise ExternalServiceError( | ||
| "jellyfin", "Jellyfin provider catalogue exceeds the safe bounded limit." | ||
| ) | ||
| elif total != expected_total: |
There was a problem hiding this comment.
[Medium, plausible] The provider catalogue scan fails when the Jellyfin total changes between pages.
Deletion webhooks typically arrive while Jellyfin is still removing items in the same library scan. When a season of 20 episodes is deleted, each ItemDeleted webhook pages through the whole Episode catalogue (about 25k items, 50 pages), and the total shrinks between pages → inconsistent provider catalogue metadata → source_unverifiable → PARTIAL_FAILURE. The webhook marker is then deleted, but Jellyfin does not redeliver, so those episodes are never cleaned up.
Suggestion: restart the scan once on a changed total, or accept a shrinking total as long as the final pass is internally consistent (no duplicates, every page within bounds).
There was a problem hiding this comment.
Fixed in ec6dba9. A changed authoritative total discards the entire catalogue pass and restarts from offset zero once; only a complete stable pass can authorize absence. Shrink below the current offset with an empty page is supported. Repeated shrink/expansion churn, duplicate/invalid rows and incomplete pages still fail closed. Added stable-shrink, shrink-below-offset and repeated-churn regressions. The existing 20,000-item safety bound remains; catalogues above it, including the 25k example, are still explicitly unverifiable.
| def _preflight_failure_code(plan: ProcessingResultResponse) -> str | None: | ||
| """Classify failed read/preflight evidence before treating it as plan drift.""" | ||
|
|
||
| return "preflight_failed" if any(action.status is ActionStatus.FAILED for action in plan.actions) else None |
There was a problem hiding this comment.
[Medium] Every FAILED action is classified as a transient preflight_failed.
Deterministic safety blocks are now also FAILED: verify_download_ownership, delete_hash with existed=None (ownership_unverifiable), and record_retention. With this helper they are retried as transient (3 attempts with backoff) and end up as preflight_failed. In batches, plan_blocked_code returns preflight_failed before unsafe_plan, so the UI tells the user the item "could not be preflighted" and implies a retry, instead of explaining the safety block.
Suggestion: classify by FailureReason. DOWNSTREAM_ERROR / AUTHENTICATION_FAILED on read actions are transient; OWNERSHIP_UNVERIFIABLE / SHARED_PATH / PACK_TORRENT are safety blocks that must not be retried or relabelled.
There was a problem hiding this comment.
Fixed in ec6dba9. preflight_failed is now limited to DOWNSTREAM_ERROR/AUTHENTICATION_FAILED on read actions. Failed safety reasons and unsafe skipped evidence take priority and become terminal unsafe_plan, including mixed transient + safety outcomes; batch previews retain that code. The worker does not retry or relabel these blockers as plan_changed. Retention-recording failure is terminal execution_failed even when it is not the first failed action. EN/RU job copy states that automatic retry is disabled. Tests cover one-attempt/no-runner safety failure, mixed evidence, transient recovery and retention failures; existing intentional safe-retention skip semantics are preserved.
| if len(owners) > 1: | ||
| for client_id, aliases, owner_uncertain in owners: | ||
| conflicts.update((client_id, hash_value) for hash_value in aliases.intersection(normalized_hashes)) | ||
| if owner_uncertain: |
There was a problem hiding this comment.
[Low] An uncertain scope at the download root blocks the whole directory under a misleading reason.
The fallback for unknown file lists (Transmission/Deluge files is None, empty qBittorrent content_path) uses the client's download root as an uncertain scope. That root is an ancestor of every torrent in it, so a single such torrent turns every deletion under that root into shared_path, i.e. "data is used by another torrent".
Fail-closed is fine here, but the reason is misleading and the user cannot tell which torrent is responsible. Suggestion: use a distinct reason (e.g. sibling_scope_unknown) and name the blocking torrent in the plan details.
There was a problem hiding this comment.
Fixed in ec6dba9. Known overlapping scopes still report shared_path; a known target overlapping an uncertain sibling now reports sibling_scope_unknown and carries the blocking sibling's client ID, canonical hash and sanitized name. All four adapters propagate the evidence into the plan. EN/RU preview and Activity copy explain incomplete metadata, and the preview identifies the sibling before technical details. Unknown scopes still block mutation and dependent cleanup. Backend cascade/adapter regressions and four light/dark/system desktop/mobile browser cases cover the new reason and disabled confirmation.
| ) | ||
|
|
||
| async def _data_path(self, hash_value: str) -> str: | ||
| async def _data_path(self, hash_value: str, *, default_root: object | None = None) -> str: |
There was a problem hiding this comment.
[Low] Stopped rTorrent target torrents can never be removed, and the plan doesn't say why.
Closed torrents now get a derived (uncertain) inventory scope, but _data_path still requires a non-empty d.base_path, and the target's own scope is uncertain. A movie whose torrent was stopped after seeding therefore always gets ownership_unverifiable, and the cascade is blocked indefinitely. Nothing tells the user that starting the torrent would unblock it.
Suggestion: either allow removal via the root-bounded derived path when it exactly matches the snapshot, or return a structured torrent_closed reason with EN/RU copy that explains the next step.
There was a problem hiding this comment.
Fixed in ec6dba9 using the structured-reason option. A closed rTorrent target with only derived uncertain evidence reports torrent_closed. EN/RU preview and Activity copy tell the administrator to open/resume it in rTorrent and refresh the preview. Actual data removal still requires a nonempty validated base_path strictly below the live default root; CleanArr does not auto-resume or substitute the download directory. The protocol regression asserts no execute.throw or d.erase for the closed target; cascade and browser tests verify the blocker and disabled confirmation.
|
Все 7 замечаний второго ревью исправлены; ответы оставлены в каждом inline thread. Финальный HEAD:
На реальном qBittorrent 5.2.4 отдельно подтверждён payload metadata-free magnet: metaDL, total_size=-1 и нулевые completed/downloaded/progress. Описание PR и связанные issue обновлены. Проверки upgrade rehearsal усилены под schema 8 и latest stable; устаревшая дата fixture заменена текущим UTC без ослабления assertions. Готово к независимому повторному ревью. Review threads оставлены открытыми для принятия; merge/tag/release/deployment не выполнялись. |
mambastick
left a comment
mambastick
left a comment
There was a problem hiding this comment.
Re-review of 7d211de by Claude Code.
All 7 findings from the previous round are addressed: routed Radarr IDs (with a router-level scenario test), the rTorrent d.multicall2 inventory, qBittorrent metaDL -1/0 sizes and requested fields, the Jellyfin churn restart, reason-based preflight classification, sibling_scope_unknown with the blocking torrents named, and torrent_closed. Backend suite: 504 passed, 7 skipped; CI is green.
Blocking: the new DELETED → unsafe_plan rule leaks into batch execution, so every successful real batch child is reported as FAILED (reproduced, details inline). Also, a single rTorrent torrent outside directory.default still makes the whole fleet unverifiable (reproduced). The other two comments are a plausible fail-open around rTorrent views and a minor ordering issue in the Jellyfin restart.
| def _preflight_failure_code(plan: ProcessingResultResponse) -> str | None: | ||
| """Classify failed preflight evidence before treating it as plan drift.""" | ||
|
|
||
| if any(action.status is ActionStatus.DELETED for action in plan.actions): |
There was a problem hiding this comment.
[Critical, reproduced] Every successful real batch child is reported as FAILED.
_preflight_failure_code now returns unsafe_plan whenever any action is DELETED. plan_blocked_code delegates to it, and ManualDeletionBatchService._run_batch calls plan_blocked_code(result) on the live execution result (deletion_batches.py:419). A real, successful child (status SUCCESS with DELETED actions) therefore becomes FAILED / unsafe_result, "The child reported a safety-attention outcome". The batch ends as FAILED with "No child deletion completed safely", although the data was actually deleted.
Reproduced on 7d211de:
plan_blocked_code(ProcessingResultResponse.from_domain(ProcessingResult(
event=ev, status=OverallStatus.SUCCESS, actions=(
ActionResult(system="qbittorrent", action="delete_hash", status=ActionStatus.DELETED, ...),
ActionResult(system="radarr", action="delete_movie", status=ActionStatus.DELETED, ...),
))))
# -> 'unsafe_plan'The suite stays green (504 passed) because no batch test executes a live child that produces DELETED actions.
Suggestion: the "DELETED means unsafe" rule only makes sense for a dry-run preflight plan. Keep plan_blocked_code / _preflight_failure_code for preflight plans, and classify execution results separately (SUCCESS → completed; safety SKIPPED/FAILED → unsafe_result). Please add a live-mode batch scenario test with mutations enabled.
There was a problem hiding this comment.
Confirmed and fixed in 0a83235. Batch execution no longer calls plan_blocked_code/_preflight_failure_code on a live result. The mutated-plan DELETED guard is confined to preflight validation (including confirmation and refreshed plans), while execution is classified separately. Successful DELETED actions complete the child and batch; safety SKIPPED/FAILED evidence with no applied mutation becomes unsafe_result. A partial outcome after actual deletions remains partial_result and preserves its actions without replay.
Added four live-mode batch scenarios through the real MovieDeletionStrategy with fake services: success (downloader and Radarr DELETED, child/parent COMPLETED), a late downstream read failure with retained DELETED actions, and safety SKIPPED/FAILED execution outcomes. Also covered mutated-preflight rejection and a failed mutation that must not retry; only transient read failures retain bounded retry eligibility. Local full suite: 524 passed, 7 opt-in live tests skipped. CI and the pinned live profile are running on the pushed HEAD.
| continue | ||
| if base_path not in (None, ""): | ||
| return DownloaderContentInventory( | ||
| failures=(DownloaderContentInventoryFailure("unbounded_content_scope"),) |
There was a problem hiding this comment.
[High, reproduced] One rTorrent torrent outside directory.default blocks data removal on every client.
When a single row has a non-empty base_path outside the root (or any row is malformed), the whole inventory returns failures=(unbounded_content_scope,) with no scopes. The fleet assessment then becomes incomplete, and MultiDownloaderClient reports ownership_unverifiable for every hash on qBittorrent, Transmission and Deluge as well.
Reproduced with directory.default=/srv/torrents and two rows, /srv/torrents/Movie (the target) and /mnt/disk2/Other: the result is scopes: 0, failures: ['unbounded_content_scope'] and assess_content_inventory(...).complete == False.
Torrents outside directory.default are common: second disks, ruTorrent per-label directories, Arr-set download directories. SAFETY.md says only "paths outside that root are blocked", but in practice everything is blocked.
Suggestion: keep each out-of-root torrent as its own scope (exact, or uncertain with a structured code), so that only targets overlapping it are blocked. Reserve whole-inventory failure for a truly unreadable response.
There was a problem hiding this comment.
Fixed in 0a83235. A valid absolute rTorrent base_path outside directory.default is now retained as an exact inventory scope. It participates in fleet overlap checks without making unrelated scopes incomplete. Recoverable malformed rows with a readable hash and bounded absolute path are retained as uncertain envelopes; default-root base paths are uncertain too. Rows whose identity/path cannot be recovered remain unreadable evidence and fail closed rather than silently hiding a sibling. Actual rTorrent rm authorization remains strictly below its live default root.
Protocol/fleet regressions prove that an outside-root sibling allows unrelated rTorrent-inside-root and qBittorrent targets to delete, blocks an overlapping qBittorrent target, and never authorizes rm of the outside-root rTorrent target. Added bounded/unrecoverable malformed-row cases. SAFETY and Torrent Clients docs were updated together in EN/RU.
| self._rpc( | ||
| "d.multicall2", | ||
| "", | ||
| "default", |
There was a problem hiding this comment.
[Medium, plausible — please verify] The inventory view may differ from the deletion path's download_list.
The inventory enumerates d.multicall2 '' default, while _delete_hashes_with_assessment checks existence via download_list with its implicit view. The target itself is cross-checked through known_existing, but siblings that are missing from the inventory view are never checked for overlap. If the default view excludes some torrents (custom view filters, or a different default across rTorrent versions), a cross-seed sibling that shares the target's directory is invisible. That is a fail-open path to rm -rf of shared data.
Suggestion: use the explicit main view in both calls, and cross-check the multicall hash set against download_list; any mismatch should make the inventory fail closed.
There was a problem hiding this comment.
Verified against the official XML-RPC target convention and fixed in 0a83235: download_list("", "main") and d.multicall2("", "main", ...). The inventory validates canonical unique hashes and requires full set equality between both snapshots. Missing/extra/invalid/duplicate IDs fail closed, including a target that appears absent in the inconsistent list. Destructive removal also checks fresh download_list membership against the recorded assessment before any mutation.
New wire-level protocol tests assert the exact target/view arguments, mismatch/invalid-sibling refusal, and membership changing after inventory; they assert no execute.throw or d.erase. The 1,500-entry regression still verifies constant inventory RPC count (now three calls for the additional membership proof). The pinned real-service workflow is running on this HEAD. Official reference: https://github.com/rakshasa/rtorrent-doc/blob/master/RPC-Setup-XMLRPC.md
| seen_ids.add(parsed.id) | ||
| result.append(parsed) | ||
| start_index += len(raw_items) | ||
| if start_index > total: |
There was a problem hiding this comment.
[Low] The churn restart cannot run when the total shrinks on the final pages.
start_index > total raises ExternalServiceError before the if changed: return [], True restart branch is reached. Example: total 25,000; Jellyfin removes 20 episodes while page 50 (start_index 24,500) still returns 500 items with TotalRecordCount=24,980. start_index becomes 25,000 > 24,980, the error "inconsistent provider catalogue metadata" is raised, and the cascade is blocked as source_unverifiable with no restart.
Suggestion: evaluate changed (and return the restart signal) before the start_index > total guard.
There was a problem hiding this comment.
Fixed in 0a83235. After validating/reading a returned page, the changed-total restart signal is now handled before the accumulated start_index > total guard. A regression reproduces 1000 -> 980 on the last 500-item page, then verifies one restart and successful completion of the stable 980-item pass. Stable inconsistent counts, malformed/duplicate rows, repeated churn and the existing 20,000-item bound remain fail-closed.
|
Все 4 замечания review 5518406362 исправлены в
Все пять quality gates прошли: Ruff format/check, mypy, 524 backend-теста, frontend lint/build, 178 Vitest-тестов, 44 Playwright-сценария, source/dependency scan, container scan/smoke и DEB/RPM installation smoke. В обычном pytest 7 opt-in live-тестов пропущены; отдельный real-service workflow выполнил все 7 и шесть upgrade/backup-rollback путей, включая v2.0.4. Browser report. Описание PR и EN/RU safety/client docs обновлены. UI source и migration history не менялись. Готово к следующему независимому проходу ревью; threads оставлены открытыми для принятия. Merge не выполнялся. |
mambastick
left a comment
mambastick
left a comment
There was a problem hiding this comment.
Re-review of 0a83235 by Claude Code.
All 4 findings from the previous round are addressed: batch execution results now use a separate _execution_result_block_code (with a live batch scenario test), out-of-root rTorrent torrents become their own scopes, both rTorrent calls use the explicit main view and are cross-checked (inventory_hash_mismatch), and the Jellyfin churn restart now runs before the bounds guard. CI is green.
Remaining issues are all in single-job execution-result classification (two of them reproduced, details inline):
- A live run where everything was blocked (
IGNORED) is reported as COMPLETED / "Deletion completed." - A transient downloader failure before any mutation is classified as a safety block and never retried (regression since
7d211de, which I missed in the previous pass). - (Plausible) The rTorrent snapshot equality check races with newly added torrents.
| job.status = ManualDeleteJobStatus.COMPLETED | ||
| job.phase = ManualDeleteJobPhase.COMPLETED | ||
| job.progress_percent = 100 | ||
| job.message = "Deletion completed." |
There was a problem hiding this comment.
[High, reproduced] A blocked live run is reported as "Deletion completed".
_run_job only inspects OverallStatus.PARTIAL_FAILURE. If cleanup is entirely blocked at execution time, the result is IGNORED (or SUCCESS with unsafe SKIPPED actions), and it falls through to COMPLETED / "Deletion completed.". ManualDeletionBatchService classifies the same result as unsafe_result, so the single-job path is now inconsistent with the batch path.
Reproduced on 0a83235: a live movie strategy whose downloader returns sibling_scope_unknown produces
ignored [delete_hash skipped sibling_scope_unknown,
delete_movie skipped downstream_error,
cleanup_movie skipped downstream_error]
Nothing is deleted, but the job is marked COMPLETED.
This becomes realistic now because blockers can appear between the pre-run re-check and the live call: a new uncertain sibling, an rTorrent snapshot mismatch (see the comment on downloaders.py:1326), or a seeding-policy change.
Suggestion: run _execution_result_block_code on every live result, not only on PARTIAL_FAILURE, and add a job test where the live run is blocked after a clean re-check.
There was a problem hiding this comment.
Fixed in 24ec8ee. _run_job classifies every live result before marking completion. A safety-blocked IGNORED result or nominal SUCCESS containing unsafe SKIPPED actions becomes FAILED/unsafe_plan with its blocking action evidence and no retry. Applied partial deletions remain terminal; all-safe intentional retention/already-absent results preserve idempotent completion.
The new real MovieDeletionStrategy job scenario keeps preview, submit recheck and worker recheck clean, then returns sibling_scope_unknown during live execution. It verifies FAILED/unsafe_plan, one attempt and no downloader or Radarr deletion. Additional job/batch regressions cover nominal SUCCESS with unsafe skips, safe retention, already-absent outcomes, and a dependent execution skip after a DELETED action.
Local mandatory gates pass: Ruff format/check, mypy, 558 backend tests and frontend lint/build/178 tests. The pinned live profile and six upgrade/rollback rehearsals passed on this exact HEAD. Quality CI has passed backend, frontend (44 Chromium scenarios), source/dependency and container checks; package installation smoke is still running. No merge performed.
| action.status is ActionStatus.FAILED | ||
| and ( | ||
| _action_reason(action) not in _TRANSIENT_PREFLIGHT_REASONS | ||
| or not _is_read_preflight_action(action.action) |
There was a problem hiding this comment.
[Medium, reproduced] A transient downloader outage is no longer retried (regression since 7d211de).
_execution_result_block_code returns unsafe_result for any FAILED action that is not a read action, and for any SKIPPED action with a non-safe reason, including the DOWNSTREAM_ERROR placeholders that _record_downloader_block adds.
Reproduced: qBittorrent times out in delete_hashes with nothing deleted:
partial_failure [delete_hashes FAILED downstream_error,
delete_movie SKIPPED downstream_error,
cleanup_movie SKIPPED downstream_error] -> unsafe_result
As a result, _mark_failed(error_code="unsafe_plan") with the message "Partial cleanup contained safety blockers" is called, and the job is not retried. #44 explicitly keeps the automatic retry for exactly this case: a downloader failure before any mutation.
Suggestion: do not count the _record_downloader_block (SKIPPED / DOWNSTREAM_ERROR) entries as safety blockers. Treat DOWNSTREAM_ERROR / AUTHENTICATION_FAILED on delete_hashes / delete_hash as retryable execution_failed when the result has no DELETED actions. Real safety reasons (OWNERSHIP_UNVERIFIABLE, SHARED_PATH, SIBLING_SCOPE_UNKNOWN, PACK_TORRENT, …) must stay non-retryable.
There was a problem hiding this comment.
Fixed in 24ec8ee. The actual _record_downloader_block dependency SKIPPED/downstream_error actions are execution errors rather than independent safety blockers. DOWNSTREAM_ERROR/AUTHENTICATION_FAILED on delete_hash or delete_hashes without any DELETED action retains bounded execution_failed retry eligibility and a fresh preflight each attempt. Genuine safety reasons take priority regardless of action order and remain terminal. Retention-recording failure, other failed mutation commands, unknown execution exceptions and applied partial outcomes cannot replay.
The real MovieDeletionStrategy scenario captures the persisted RETRY_WAIT/execution_failed record with no downloader/Radarr mutations, then verifies the fresh recheck and successful second attempt. Separate regressions cover authentication, mixed safety/transient ordering, deletion before a later failure, and dependency skips with no failed-action evidence (terminal execution_failed, not false completion).
Local mandatory gates pass: Ruff format/check, mypy, 558 backend tests and frontend lint/build/178 tests. The pinned live profile and six upgrade/rollback rehearsals passed on this exact HEAD. Quality CI has passed backend, frontend (44 Chromium scenarios), source/dependency and container checks; package installation smoke is still running. No merge performed.
| ) | ||
| if delete_files and assessment is not None: | ||
| snapshot_hashes = {value for client_id, value in assessment.observed if client_id == safety_client_id} | ||
| if not assessment.complete or set(torrent_ids) != snapshot_hashes: |
There was a problem hiding this comment.
[Medium, plausible] The rTorrent hash-set equality check races with torrents added in the meantime.
The delete path compares a fresh download_list with the hash set that was observed in an inventory read made earlier. In the fleet flow that read happens before all the other clients are read (each can take up to 10 s). Any torrent added or removed in between makes set(torrent_ids) != snapshot_hashes, and every target on that rTorrent becomes ownership_unverifiable. On busy clients (RSS / autobrr) this will fire regularly, and combined with the COMPLETED issue above the job still reports success.
Suggestion: require that the target hashes are present in both snapshots, and fail closed only when a hash added since the snapshot cannot be checked for overlap. Alternatively, re-read the inventory once on a mismatch before giving up.
There was a problem hiding this comment.
Fixed in 24ec8ee with one bounded inventory refresh on a valid membership mismatch. In fleet execution the reconciliation is a barrier before any adapter mutates: the refreshed assessment is shared with every client. A later rTorrent-only refresh is disabled in that flow, so newly discovered shared data cannot leave qBittorrent/Deluge on stale evidence. Standalone rTorrent calls retain the bounded refresh.
Stable unrelated additions/removals proceed. Target presence and original paths must remain unchanged; refreshed main-view multicall/list membership and the final listing must agree. Continued churn, unreadable evidence and changed targets fail closed. Baseline safety, other-client scopes, hybrid aliases, empty scopes and missing-known evidence are preserved.
Coordinated fleet tests add a sibling while another client is still reading: unrelated scopes permit deletion; shared/uncertain scopes block qBittorrent before mutation; unreadable scope blocks the fleet. Protocol tests cover target/path changes and repeated churn. EN/RU Safety and Roadmap are synchronized.
Local mandatory gates pass: Ruff format/check, mypy, 558 backend tests and frontend lint/build/178 tests. The pinned live profile and six upgrade/rollback rehearsals passed on this exact HEAD. Quality CI has passed backend, frontend (44 Chromium scenarios), source/dependency and container checks; package installation smoke is still running. No merge performed.
|
Все 3 замечания review 5519572790 исправлены и запушены в ту же ветку. HEAD:
Ответы опубликованы в обсуждении 1, обсуждении 2, обсуждении 3. EN/RU Safety и Roadmap синхронизированы. На этом точном HEAD: Ruff format/check, mypy, 558 backend / 178 frontend / 44 Chromium tests; все 5 quality gates зелёные. Pinned compatibility прошёл 7 live-тестов и 6 upgrade/backup-rollback сценариев. В регулярном pytest 7 opt-in live-тестов пропускаются и отдельно выполнены этим профилем. Готово для нового независимого ревью. PR открыт; merge не выполнялся. |
mambastick
left a comment
mambastick
left a comment
There was a problem hiding this comment.
Re-review of 24ec8ee by Claude Code.
All 3 findings from the previous round are addressed: every live result is now classified (a blocked run is no longer reported as COMPLETED), transient downloader failures without a mutation retry as execution_failed, and the rTorrent membership race is reconciled by re-reading the inventory before mutation. CI is green.
One remaining gap, reproduced and detailed inline: the retry fix does not cover the Jellyfin delete_item placeholder, so Library deletions with jellyfin_item_id (the normal series and season flow) still fail permanently as a safety block on a transient downloader error.
| } | ||
| ) | ||
| _RETRYABLE_EXECUTION_ACTIONS = frozenset({"delete_hash", "delete_hashes"}) | ||
| _DEPENDENCY_SKIP_ACTIONS = frozenset( |
There was a problem hiding this comment.
[Medium, reproduced] The retry fix misses the Jellyfin delete_item placeholder, so the normal Library flow still never retries.
When jellyfin_item_id is set and downstream cleanup is blocked, ManualDeletionService.execute appends jellyfin / delete_item / SKIPPED / downstream_error ("Kept the Jellyfin item because downstream cleanup is blocked"). delete_item is not in _DEPENDENCY_SKIP_ACTIONS, so _execution_result_block_code treats that placeholder as a safety blocker.
The Library UI sends jellyfin_item_id for Jellyfin-bound series and seasons (library-panel.tsx), so this is the common path.
Reproduced on 24ec8ee with a qBittorrent timeout in delete_hashes and nothing deleted:
jellyfin_item_id=None -> [delete_hashes FAILED, delete_movie SKIPPED, cleanup_movie SKIPPED] -> execution_failed (retryable)
jellyfin_item_id="jf-item-1" -> [delete_hashes FAILED, delete_movie SKIPPED, cleanup_movie SKIPPED, delete_item SKIPPED] -> unsafe_result
With jellyfin_item_id set, the job fails as unsafe_plan ("blocked by safety checks") and is never retried. The new test_job_movie_cleanup scenarios don't set jellyfin_item_id.
Suggestion: treat the Jellyfin "kept because downstream cleanup is blocked" placeholder as a dependency skip, either by adding delete_item with DOWNSTREAM_ERROR to the dependency set or by giving the placeholder a distinct marker. Add job and batch tests with jellyfin_item_id for both the transient-retry case and the safety-block case.
There was a problem hiding this comment.
Fixed in aa289d2. Jellyfin delete_item is included among dependent actions only for SKIPPED/DOWNSTREAM_ERROR. A transient delete_hash/delete_hashes failure with no DELETED action now remains execution_failed and receives the existing bounded job retry with a fresh preflight. A genuinely FAILED delete_item mutation remains terminal; shared_path/sibling_scope_unknown still take priority and block retry. A dependency placeholder alone, without a recorded failed upstream action, does not authorize replay even if the overall result says PARTIAL_FAILURE.
Added 18 scenario cases through actual ManualDeletionService.resolve/preview/execute for Jellyfin-bound Movie, Series and Season selections, including Library resource identity. They assert the real skipped Jellyfin action, persisted RETRY_WAIT/execution_failed with no downloader/Arr/Jellyfin mutations, fresh recheck and exactly one successful cleanup. Both safety reasons terminate jobs and batch children without mutation or retry. Batch semantics remain unchanged: it records FAILED/partial_result with execution_failed classification, then a new hash-confirmed submission succeeds; batches have no automatic RETRY_WAIT state.
The transient Movie/Series/Season regressions all fail as unsafe_plan on the previous HEAD 24ec8ee and pass with this fix. EN/RU Safety docs are synchronized.
Local mandatory gates pass: Ruff format/check, mypy, 580 backend tests and frontend lint/build/178 tests. Quality CI and the pinned real-service/upgrade profile are running on this exact HEAD. No merge performed.
|
Замечание review 5520161094 исправлено и запушено в ту же ветку. HEAD: Jellyfin 18 сценариев через настоящий ManualDeletionService проверяют Movie/Series/Season с На этом точном HEAD: Ruff format/check, mypy, 580 backend / 178 frontend / 44 Chromium tests; все 5 quality gates зелёные. Pinned compatibility прошёл 7 live-тестов и 6 upgrade/backup-rollback сценариев. EN/RU Safety и описание PR обновлены. PR открыт для нового независимого ревью. Merge не выполнялся. |
This change preserves partial mutation evidence and blocks unsafe cleanup while keeping ordinary catalogue sizes and routine stopped/magnet torrents usable. HTTP-origin submissions receive valid UUIDv4 keys, and deliberately retained torrents remain durably visible after Arr ownership records are removed.
Review corrections additionally reuse one immutable fleet assessment per call through all four adapters, preserve truthful present/absent/unknown evidence and reject changed target paths before mutation. Closed rTorrent inventory uses a conservative validated child scope while actual removal still requires a nonempty validated base path. Radarr instance reads and failed hash tasks are drained before returning. Routed negative Radarr identities now remain valid; real-router scenarios cover one/two instances and cross-instance shared hashes. rTorrent reads three inventory RPCs with explicit main-view membership verification. Unknown sibling scopes report sibling_scope_unknown with name/client/hash evidence; closed targets report torrent_closed with EN/RU recovery instructions. Safety and mixed transient/safety outcomes are terminal, while transient reads and downloader failures before any recorded deletion retain bounded retries with a fresh preflight.
Preflight and live execution results are classified separately. DELETED actions invalidate a mutation-free preview, but a successful live batch child completes normally. Safety-only execution blockers are unsafe_result; applied partial outcomes retain their actions and are never replayed. Every live result is classified, including IGNORED or nominal SUCCESS containing unsafe skips. Dependency SKIPPED/downstream_error placeholders are execution errors, not safety blockers. Transient delete_hash/delete_hashes failures without any DELETED action retry as execution_failed; true safety blockers, retention failures, other failed mutations and applied partial outcomes remain terminal. All-safe retained/already-absent results preserve successful idempotent completion.
rTorrent preserves valid absolute outside-root paths as exact overlap evidence and bounded malformed rows as uncertain envelopes. Unrecoverable identity/path evidence is not silently dropped. download_list and d.multicall2 explicitly use target "" and view main; normalized hash sets must agree. A stable unrelated membership change gets one bounded inventory refresh. In the fleet flow a reconciliation barrier propagates that refreshed evidence to every adapter before any mutation; rTorrent cannot perform a later local refresh that leaves other clients on an old snapshot. Original target presence/path and baseline safety evidence are preserved; overlap, unreadable scope, changed target or continued churn fails closed. Actual rm remains limited to validated children of directory.default.
Validation on
aa289d24c82e5f60f6750d9dd052dc01e4c4358e:Review boundaries:
Migration/rollback: migration 7 adds retention and ordered migration 8 adds durable job error codes; configuration remains schema 4. Before upgrading from v2.0.4/schema 6, export and verify the SQLite backup. Rollback is stop → preserve the schema-8 database → restore the schema-6 backup → run v2.0.4 and verify configuration/users/history. Older binaries reject newer schemas. EN/RU Safety, Operations, Compatibility and Roadmap documents are updated together.
Prepared for independent review and merge by the user's designated agent.
Fixes #42
Fixes #43
Fixes #44
Fixes #45
Fixes #46
Fixes #47
Fixes #48
Fixes #49
Fixes #50
Fixes #51
Fixes #52