core: don't strand a torrent when remove() aborts after the handle is destroyed - #516
Open
nopoz wants to merge 1 commit into
Open
core: don't strand a torrent when remove() aborts after the handle is destroyed#516nopoz wants to merge 1 commit into
nopoz wants to merge 1 commit into
Conversation
…le dies TorrentManager.remove() does its destructive work before the validation that can throw. Once session.remove_torrent() has run the libtorrent handle is dead, but a torrent missing from queued_torrents then raised InvalidTorrentError, so `del self.torrents[torrent_id]` never ran. Deluge kept a Torrent whose every get_torrent_status raised "invalid torrent handle used" for the life of the daemon. on_alert_torrent_finished reaches that state on its own: with a move_completed path set it queues the move and leaves is_finished False while still dropping the id from queued_torrents. If the move never finishes, a remove hits the KeyError path with the id already gone. An inconsistent bookkeeping set is not a reason to abandon a removal that libtorrent has already performed, so log it and carry on, matching what on_alert_torrent_finished already does with the same KeyError. Also let a torrent with an already-invalid handle be removed. remove_torrent() raises on a dead handle, and returning False there left any torrent already in this state unremovable short of restarting the daemon.
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.
A failed torrent removal can leave Deluge holding a torrent that no longer works.
Every later
get_torrent_statusfor that id raisesRuntimeError: invalid torrent handle used [libtorrent:20], the torrent cannot be removed, and it stays that way for the life of the daemon. Restarting is the only way out.Cause.
TorrentManager.remove()does its destructive work before a validation that can throw. Oncesession.remove_torrent()has run the handle is dead, so if the id is then missing fromqueued_torrents,remove()raisesInvalidTorrentErroranddel self.torrents[torrent_id]never runs.Fix. Complete the removal rather than abandoning it, and add a recovery path so a torrent already stuck in this state can be cleared without a restart.
How it is reached
on_alert_torrent_finishedgets there on its own. Withmove_completedset and the paths differing, it queues the move and leavesis_finishedFalse, while still dropping the id fromqueued_torrentsat the end of the handler. If the move never completes,storage_moved_alertnever fires,is_finishedstays False, and a remove hits theKeyErrorpath with the id already gone.Observed by re-adding a torrent whose data was already at the move-completed destination: it completed immediately, queued a move onto an existing file, stuck in
Moving, and removing it produced exactly one unreachable torrent.Why completing the removal is the right call
An inconsistent bookkeeping set is not a reason to abandon a removal libtorrent has already performed, so the
KeyErroris now logged and the removal completes.That is already what
on_alert_torrent_finisheddoes with the sameKeyErroron the same set, so this makes the two paths agree.Recovery
remove()returned False whenremove_torrent()raised, andremove_torrent()raises on an invalid handle, so a torrent already in this state could not be cleared without restarting the daemon.It now checks
handle.is_valid()and finishes the cleanup when the handle is already gone. The guard is deliberately narrow: a genuineremove_torrentfailure still returns False.Tests
Two regression tests in
deluge/tests/test_torrentmanager.py, both driving a real session and a real libtorrent handle rather than mocks.pytest deluge/tests/test_torrentmanager.pygives8 passed, 1 skipped.Not a regression: the behaviour is the same on 2.2.0. Found while profiling a daemon with ~900 torrents.