Skip to content

Re-arm removals for downloads that never left the queue - #383

Merged
lolimmlost merged 2 commits into
ManiMatter:devfrom
qoole:fix/rearm-removals-still-in-queue
Sep 26, 2026
Merged

lolimmlost merged 2 commits into
ManiMatter:devfrom
qoole:fix/rearm-removals-still-in-queue

Conversation

@qoole

@qoole qoole commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Problem

A removal that does not actually take effect permanently disables every future removal of that download.

Observed on v2.1.0: one remove_orphans removal recorded a hash as deleted, but the torrent was never removed from qBittorrent. remove_metadata_missing then flagged the same download for ~19 hours, climbing to 74 strikes against max_strikes: 5, with zero removals and zero blocklist entries. Nothing in the log said the removal was being skipped.

Root cause

arr.tracker.deleted is process-lifetime state that gates every removal:

  • src/jobs/removal_handler.py:18 silently drops any download whose id is already in that list.
  • src/jobs/removal_handler.py:32 appends to it unconditionally, after the removal attempt, without checking whether the removal worked (remove_queue_item's return value is discarded at line 40).
  • The list is only ever cleared by Tracker.reset(), whose sole call site is src/job_manager.py:141 — the branch taken only when the *arr queue is completely empty.

So a download that cannot be removed keeps the queue non-empty, which keeps the list from ever being cleared, which keeps that download suppressed for the life of the process. Strikes keep counting up; nothing else happens. Same code on v2.1.0 and current dev.

Fix

Prune the tracker at the start of each run, inside _queue_has_items(), using the full queue it already fetched (no extra API call). If a download recorded as deleted is still in the queue, the removal did not take effect, so it becomes eligible again.

_queue_has_items() is already the tracker-lifecycle method — it holds the only Tracker.reset() call — so both branches of that one predicate now manage the list: queue empty resets everything, queue non-empty drops the entries the queue itself disproves.

obsolete_tag is exempt

obsolete_tag handling deliberately leaves the download in the *arr queue so qbit_manage can reap it later (README.md:441), and removal_handler.py:32 appends to tracker.deleted after that branch too. One list was carrying two different meanings: "we removed this" and "we already tagged this, leave it alone". Queue presence only disproves the first.

Tagged ids are therefore recorded in a separate Tracker.obsolete_tagged list (cleared by the same reset()) that the prune skips. Without this, an obsolete-tagged torrent is re-tagged and re-logged on every cycle for as long as it lingers — measured below.

Behaviour changes worth knowing

  • For the six jobs without max_strikes (remove_orphans, remove_unmonitored, remove_missing_files, remove_failed_downloads, remove_failed_imports, remove_bad_files), a download that genuinely cannot be removed is now retried once per cycle instead of once per process. That is the intended consequence of no longer suppressing failed removals, and matches what the strike-based jobs already do — see the existing tip at strikes_handler.py:199.
  • In test_run mode the DELETE is simulated, so the download stays in the queue and each candidate is now reported every cycle rather than once per process. This matches download_client_removal_job.py:99-104, which already logs "Would have removed" on every cycle.
  • The cross-cycle reader at strikes_handler.py:143-154 is unaffected: entries for downloads that really are gone are still kept, so "deleted in previous round" keeps classifying correctly.

Alternatives considered

Clearing tracker.deleted unconditionally each cycle is a one-liner and also fixes the bug, but it is strictly less precise and destroys the only cross-cycle consumer of the list: strikes_handler.py:143-154 distinguishes "deleted in previous round" (debug) from "no longer in queue" (verbose), and blind clearing reclassifies every successful removal into the wrong branch on the following cycle. VERBOSE is a documented level (README.md:389-392). The prune keeps that intact because it only forgets ids the queue proves are still present.

Gating the append on remove_queue_item's boolean is worth doing but is a different bug: in this incident the *arr returned 200 and accepted the DELETE — the removal just did not stick — so that change would not have helped here. It also has no defined meaning for the obsolete_tag branch, which produces no such boolean.

Testing

Locally on dev, Python 3.14, project venv.

  • Full suite: python -m pytest -o log_cli=false — 296 passed.
  • ruff format --check on both edited-heavily files: already formatted. (removal_handler.py and _instances.py report reformat suggestions, but those are pre-existing at HEAD and not on the changed lines — verified by stashing.)

Reproduction of the obsolete_tag case, driving JobManager._queue_has_items -> RemovalHandler.remove_downloads over three cycles with a real Tracker, one private stalled torrent held in the queue, private_tracker_handling: obsolete_tag, only the HTTP boundary mocked. Cumulative qbit.set_tag awaits per cycle:

prune without the exemption   [1, 2, 3]   re-tagged every cycle
prune with the exemption      [1, 1, 1]   tagged once, as before

remove_queue_item stayed at 0 in both runs.

Mutation checks — each of these was applied to the tree with the tests left in place, and each is caught:

Mutation Result
Revert src/job_manager.py to dev, keep the tests 2 failed
Drop the obsolete_tag exemption from the prune 1 failed
Drop the obsolete_tagged bookkeeping in removal_handler 1 failed
Invert the prune predicate (forget the ids that are gone) 3 failed
Query the normal queue instead of full 1 failed

The last one matters because remove_orphans — the job that poisoned the tracker in the original incident — only appears in the full queue, so the test now pins that scope explicitly.

Changes

  • src/job_manager.py — _forget_deleted_downloads_still_in_queue(), called from the non-empty branch of _queue_has_items().
  • src/settings/_instances.py — Tracker.obsolete_tagged, cleared alongside the rest in reset().
  • src/jobs/removal_handler.py — record tag-only ids in that list.
  • tests/jobs/test_job_manager.py — four tests: prune forgets a download still in the queue, that download is then removed on the next run, a genuinely removed download stays suppressed, and an obsolete-tagged download is tagged exactly once across two cycles.

No config, README, or behaviour-flag changes.

🤖 Generated with Claude Code

A removal that does not take effect permanently disables every future
removal of that download.

tracker.deleted gates every removal (removal_handler.py:18) and is
appended unconditionally (line 32) without checking whether the removal
worked, yet it is only ever cleared by Tracker.reset() when the *arr
queue is completely empty. A download that cannot be removed keeps the
queue non-empty, so the entry is never cleared and that download stays
suppressed for the life of the process. Observed on v2.1.0 as a stuck
torrent reaching 74 strikes against max_strikes 5 over ~19 hours with
zero removals and zero blocklist entries, and no log line explaining it.

Prune the tracker at the start of each run using the full queue that
_queue_has_items() already fetches: if a download recorded as deleted is
still in the queue, the removal did not take effect, so it becomes
eligible again. Entries for downloads that really are gone are kept, so
the cross-cycle reader in strikes_handler stays correct.

obsolete_tag handling deliberately leaves the download in the queue, so
queue presence does not disprove it. Tagged ids are recorded separately
in Tracker.obsolete_tagged and exempted from the prune; without this an
obsolete-tagged torrent is re-tagged on every cycle.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@lolimmlost lolimmlost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed and tested locally. LGTM.

The analysis is right: tracker.deleted is appended unconditionally and only cleared when the queue is empty, so a single removal that doesn't take effect suppresses that download for the life of the process. Pruning inside _queue_has_items() using the full queue that is already fetched is a clean fix with no extra API call, and exempting obsolete_tag downloads (which stay in the queue by design) avoids re-tagging them every run.

Behavioral note for anyone reading later: a download that genuinely cannot be removed will now be retried every cycle rather than silently skipped. That is the better failure mode, since it is at least visible in the logs.

Verified: full suite passes on top of current dev (297), and it merges cleanly together with #382 and #377 (300 passed).

Heads-up for #381: it changes the remove_queue_item call signature, so test_forgotten_download_is_removed_again_on_the_next_run from this PR will need its assertion updated to include remove_from_client=True once both are in. I'll flag it on #381.

@lolimmlost
lolimmlost merged commit 253431f into ManiMatter:dev Sep 26, 2026
4 checks passed
lolimmlost added a commit to sgtsquiggs/decluttarr that referenced this pull request Sep 29, 2026
ManiMatter#383 added this assertion before remove_queue_item gained the
remove_from_client keyword, so it fails once both are on dev.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015rK7uZPvqGrze3sxdhVnGw
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants