From 50a143b4afe93bfdeb44ee7bbe793b5c2cc05d0d Mon Sep 17 00:00:00 2001 From: Benjamin Harder Date: Sun, 8 Jun 2025 03:13:31 +0200 Subject: [PATCH] High bandwith pauses slow tracking, not curing it --- src/jobs/remove_slow.py | 9 ++- src/jobs/strikes_handler.py | 51 +++++++++++------ tests/jobs/test_remove_slow.py | 16 +++--- tests/jobs/test_strikes_handler.py | 89 +++++++++++++++++++----------- 4 files changed, 107 insertions(+), 58 deletions(-) diff --git a/src/jobs/remove_slow.py b/src/jobs/remove_slow.py index f9ff841..44a7c1b 100644 --- a/src/jobs/remove_slow.py +++ b/src/jobs/remove_slow.py @@ -124,12 +124,17 @@ class RemoveSlow(RemovalJob): increment = speed = None return previous_progress, increment, speed - @staticmethod - def _high_bandwidth_usage(item): + def _high_bandwidth_usage(self, item): + download_id = item["downloadId"] + tracker_entry = self.arr.tracker.defective[self.job_name].get(download_id) + if tracker_entry: + tracker_entry["tracking_paused"] = False download_client=item["download_client"] download_client_type=item["download_client_type"] if download_client_type == "qbittorrent": if download_client.bandwidth_usage > DISABLE_OVER_BANDWIDTH_USAGE: + if tracker_entry: + tracker_entry["tracking_paused"] = True return True return False diff --git a/src/jobs/strikes_handler.py b/src/jobs/strikes_handler.py index 27a6838..2c57ed1 100644 --- a/src/jobs/strikes_handler.py +++ b/src/jobs/strikes_handler.py @@ -10,13 +10,13 @@ class StrikesHandler: self.tracker.defective.setdefault(job_name, {}) def check_permitted_strikes(self, affected_downloads): - recovered = self._recover_downloads(affected_downloads) + recovered, paused = self._recover_downloads(affected_downloads) affected_downloads = self._apply_strikes_and_filter(affected_downloads) if logger.isEnabledFor(logging.DEBUG): - self.log_change(recovered, affected_downloads) + self.log_change(recovered, paused, affected_downloads) return affected_downloads - def log_change(self, recovered, affected_items): + def log_change(self, recovered, paused, affected_items): tracker = self.tracker.defective[self.job_name] added = [] @@ -31,28 +31,45 @@ class StrikesHandler: removed = list(affected_items.keys()) logger.debug( - "Strike status changed | Added: %s | Incremented: %s | Recovered: %s | Removed: %s", + "Strike status changed | Added: %s | Incremented: %s | Recovered: %s | Tracking Paused: %s | Removed: %s", added or "None", incremented or "None", recovered or "None", + paused or "None", removed or "None", ) return added, incremented, recovered, removed def _recover_downloads(self, affected_downloads): - recovered = [ - d_id - for d_id in self.tracker.defective[self.job_name] - if d_id not in dict(affected_downloads) - ] - for d_id in recovered: - logger.info( - ">>> Download no longer marked as %s: %s", - self.job_name, - self.tracker.defective[self.job_name][d_id]["title"], - ) - del self.tracker.defective[self.job_name][d_id] - return recovered + """ + Identifies downloads that were previously tracked and are now no longer affected as recovered. + If a download is marked as tracking_paused, they are not recovered (will be recovered later potentially) + """ + recovered = [] + paused = [] + job_tracker = self.tracker.defective[self.job_name] + affected_ids = dict(affected_downloads) + + for d_id, entry in list(job_tracker.items()): + if d_id not in affected_ids: + if entry.get("tracking_paused", False): + logger.debug( + "strikes_handler.py/_recover_downloads: %s tracking is paused for this entry: %s (%s)", + self.job_name, + entry["title"], + d_id, + ) + paused.append(d_id) + else: + logger.info( + ">>> Download no longer marked as %s: %s", + self.job_name, + entry["title"], + ) + recovered.append(d_id) + del job_tracker[d_id] + + return recovered, paused def _apply_strikes_and_filter(self, affected_downloads): for d_id, affected_download in list(affected_downloads.items()): diff --git a/tests/jobs/test_remove_slow.py b/tests/jobs/test_remove_slow.py index ea6c6e6..4ffe802 100644 --- a/tests/jobs/test_remove_slow.py +++ b/tests/jobs/test_remove_slow.py @@ -237,15 +237,15 @@ async def test_get_progress_stats( @pytest.mark.parametrize( - "download_client_type, bandwidth_usage, expected", + ("download_id", "download_client_type", "bandwidth_usage", "expected"), [ - ("qbittorrent", 0.81, True), # above threshold 0.8 - ("qbittorrent", 0.8, False), # equal to threshold 0.8 - ("qbittorrent", 0.79, False), # below threshold 0.8 - ("other_client", 0.9, False), # different client type + (0, "qbittorrent", 0.81, True), # above threshold 0.8 + (1, "qbittorrent", 0.8, False), # equal to threshold 0.8 + (2, "qbittorrent", 0.79, False), # below threshold 0.8 + (3, "other_client", 0.9, False), # different client type ], ) -def test_high_bandwidth_usage(download_client_type, bandwidth_usage, expected): +def test_high_bandwidth_usage(download_id, download_client_type, bandwidth_usage, expected): """ Test RemoveSlow._high_bandwidth_usage method. @@ -263,8 +263,10 @@ def test_high_bandwidth_usage(download_client_type, bandwidth_usage, expected): item = { "download_client": DummyClient(bandwidth_usage), "download_client_type": download_client_type, + "downloadId": download_id, } - result = RemoveSlow._high_bandwidth_usage(item) + removal_job = shared_fix_affected_items(RemoveSlow) + result = removal_job._high_bandwidth_usage(item) assert result == expected diff --git a/tests/jobs/test_strikes_handler.py b/tests/jobs/test_strikes_handler.py index 551415a..f153592 100644 --- a/tests/jobs/test_strikes_handler.py +++ b/tests/jobs/test_strikes_handler.py @@ -3,7 +3,7 @@ from unittest.mock import MagicMock, patch import pytest from src.jobs.strikes_handler import StrikesHandler - +# pylint: disable=W0212 @pytest.mark.parametrize( ("before_recovery", "expected_remaining_in_tracker"), [ @@ -31,24 +31,49 @@ def test_recover_downloads(before_recovery, expected_remaining_in_tracker): handler._recover_downloads(affected_downloads) # pylint: disable=W0212 # Assert - assert sorted(tracker.defective["remove_stalled"].keys()) == sorted(expected_remaining_in_tracker) + assert sorted(tracker.defective["remove_stalled"].keys()) == sorted( + expected_remaining_in_tracker + ) +def test_recover_downloads2_no_patch(): + """Test if recovery correctly skips those items where tracking is paused.""" + handler = StrikesHandler( + job_name="remove_stalled", arr=MagicMock(), max_strikes=3 + ) + handler.tracker = MagicMock() + handler.tracker.defective = { + "remove_stalled": { + "id1": {"title": "Title1", "tracking_paused": False}, + "id2": {"title": "Title2", "tracking_paused": True}, + "id3": {"title": "Title3"}, # no paused flag = False + } + } + + affected_downloads = {"id3": {}} + + recovered, paused = handler._recover_downloads(affected_downloads) + + assert recovered == ["id1"] + assert paused == ["id2"] -# ---------- Test ---------- @pytest.mark.parametrize( ("strikes_before_increment", "max_strikes", "expected_in_affected_downloads"), [ (1, 3, False), # Below limit → should not be affected (2, 3, False), # Below limit → should not be affected - (3, 3, True), # At limit, will be pushed over limit → should be affected - (4, 3, True), # Over limit → should be affected + (3, 3, True), # At limit, will be pushed over limit → should be affected + (4, 3, True), # Over limit → should be affected ], ) -def test_apply_strikes_and_filter(strikes_before_increment, max_strikes, expected_in_affected_downloads): +def test_apply_strikes_and_filter( + strikes_before_increment, max_strikes, expected_in_affected_downloads +): job_name = "remove_stalled" tracker = MagicMock() - tracker.defective = {job_name: {"HASH1": {"title": "dummy", "strikes": strikes_before_increment}}} + tracker.defective = { + job_name: {"HASH1": {"title": "dummy", "strikes": strikes_before_increment}} + } arr = MagicMock() arr.tracker = tracker @@ -59,7 +84,9 @@ def test_apply_strikes_and_filter(strikes_before_increment, max_strikes, expecte "HASH1": {"title": "dummy"}, } - result = handler._apply_strikes_and_filter(affected_downloads) # pylint: disable=W0212 + result = handler._apply_strikes_and_filter( + affected_downloads + ) if expected_in_affected_downloads: assert "HASH1" in result else: @@ -71,30 +98,29 @@ def test_log_change_logs_expected_strike_changes(): handler.tracker = MagicMock() handler.tracker.defective = { "remove_stalled": { - "hash_new": {"title": "A", "strikes": 1}, # should show in added - "hash_inc": {"title": "B", "strikes": 2}, # should show in incremented + "hash_new": {"title": "A", "strikes": 1}, # should show in added + "hash_inc": {"title": "B", "strikes": 2}, # should show in incremented + "hash_paused": {"title": "C", "strikes": 2}, # should show in paused } } - recovered = ["hash_old"] + paused = ["hash_paused"] affected = {"hash_gone": {"title": "Gone"}} with patch("src.jobs.strikes_handler.logger") as mock_logger: - handler.log_change(recovered, affected) + handler.log_change(recovered, paused, affected) mock_logger.debug.assert_called_once() args, _ = mock_logger.debug.call_args log_msg = args[0] - assert "Added" in log_msg - assert "Incremented" in log_msg - assert "Recovered" in log_msg - assert "Removed" in log_msg + # Check keywords in the message string + for keyword in ["Added", "Incremented", "Recovered", "Removed", "Paused"]: + assert keyword in log_msg - assert "hash_new" in str(args) - assert "hash_inc" in str(args) - assert "hash_old" in str(args) - assert "hash_gone" in str(args) + # Check keys in the entire call arguments (as string) + for key in ["hash_new", "hash_inc", "hash_old", "hash_gone", "hash_paused"]: + assert key in str(args) @pytest.mark.parametrize( @@ -103,19 +129,20 @@ def test_log_change_logs_expected_strike_changes(): # max_strikes = 3 (3, 0, False), # 0 → 1 → 2 (3, 1, False), # 1 → 2 → 3 - (3, 2, True), # 2 → 3 → 4 - (3, 3, True), # 3 → 4 → 5 - + (3, 2, True), # 2 → 3 → 4 + (3, 3, True), # 3 → 4 → 5 # max_strikes = 2 (2, 0, False), # 0 → 1 → 2 - (2, 1, True), # 1 → 2 → 3 - (2, 2, True), # 2 → 3 → 4 - (2, 3, True), # 3 → 4 → 5 - ] + (2, 1, True), # 1 → 2 → 3 + (2, 2, True), # 2 → 3 → 4 + (2, 3, True), # 3 → 4 → 5 + ], ) -def test_strikes_handler_overall(max_strikes, initial_strikes, expected_removed_after_two_runs): +def test_strikes_handler_overall( + max_strikes, initial_strikes, expected_removed_after_two_runs +): """ - Verify that incrementing of strikes works and that + Verify that incrementing of strikes works and that based on its initial strikes and the max_strikes limit removal happens Note: The logging output does not show the strike where the removal will be triggered (ie., 4/3 if max strikes = 3) @@ -126,9 +153,7 @@ def test_strikes_handler_overall(max_strikes, initial_strikes, expected_removed_ tracker_mock = MagicMock() tracker_mock.defective = { - job_name: { - d_id: {"title": "Some Title", "strikes": initial_strikes} - } + job_name: {d_id: {"title": "Some Title", "strikes": initial_strikes}} } arr = MagicMock()