From dd9906ee045ca63dacf0f8db8c7a7044c684a9ab Mon Sep 17 00:00:00 2001 From: Roland Date: Wed, 25 Mar 2026 16:41:26 -0600 Subject: [PATCH] Fixing bug where next download would be calculated before download was marked as complete, causing infinite retries. --- wrolpi/downloader.py | 29 +++++++++------- wrolpi/test/test_downloader.py | 61 ++++++++++++++++++++++++++++++++-- 2 files changed, 76 insertions(+), 14 deletions(-) diff --git a/wrolpi/downloader.py b/wrolpi/downloader.py index e1b5d16f..9116d2ee 100644 --- a/wrolpi/downloader.py +++ b/wrolpi/downloader.py @@ -1455,6 +1455,23 @@ async def signal_download_download(download_id: int, download_url: str): download.location = result.location or download.location or None # Clear any old errors if the download succeeded. download.error = result.error if result.error else None + + # Set status BEFORE calculating next_download. calculate_next_download uses + # the download's status and last_successful_download to determine scheduling. + # If status is still "pending" (stale), the overdue spread logic can return + # now() for the most overdue download, causing an infinite re-download loop. + if result.error and 'outside download window' in result.error: + # Download was paused due to window closing; reset to new so it resumes later. + download.renew() + download.error = result.error + elif try_again is False and not download.frequency: + # Only once-downloads can fail. + download.fail() + elif result.success: + download.complete() + else: + download.defer() + if isinstance(result.retry_seconds, int): download.next_download = now() + timedelta(seconds=result.retry_seconds) else: @@ -1468,18 +1485,6 @@ async def signal_download_download(download_id: int, download_url: str): download_manager.create_downloads(session, urls, downloader_name=download.sub_downloader, settings=child_settings) - if result.error and 'outside download window' in result.error: - # Download was paused due to window closing; reset to new so it resumes later. - download.renew() - download.error = result.error - elif try_again is False and not download.frequency: - # Only once-downloads can fail. - download.fail() - elif result.success: - download.complete() - else: - download.defer() - # Remove this domain from the running list. download_manager._delete_processing_domain(download_domain) # Allow the download to resume. diff --git a/wrolpi/test/test_downloader.py b/wrolpi/test/test_downloader.py index 91854d96..f5e16c9f 100644 --- a/wrolpi/test/test_downloader.py +++ b/wrolpi/test/test_downloader.py @@ -335,6 +335,63 @@ async def test_calculate_next_download_overdue_spreading(test_session, test_down assert d3_result == datetime(2000, 1, 16, 18, tzinfo=pytz.UTC) # now + 1.75 days +@pytest.mark.asyncio +async def test_overdue_recurring_download_not_requeued_after_completion(test_session, test_download_manager, fake_now, + test_downloader, await_switches): + """When multiple same-frequency downloads are overdue and the most overdue one completes, + it should NOT be immediately renewed. Reproduces a bug where next_download was set to now() + because calculate_next_download was called before complete(), leaving the download with stale + pending status and last_successful_download. This caused an infinite loop on production where + doomandbloom.net/feed reached 3,379 attempts.""" + test_downloader.set_test_success() + + # Create 3 daily recurring downloads. + d1 = test_download_manager.recurring_download(test_session, 'https://example.com/overdue1', + DownloadFrequency.daily, test_downloader.name) + d2 = test_download_manager.recurring_download(test_session, 'https://example.com/overdue2', + DownloadFrequency.daily, test_downloader.name) + d3 = test_download_manager.recurring_download(test_session, 'https://example.com/overdue3', + DownloadFrequency.daily, test_downloader.name) + + # Set them as if they were last downloaded at different times and are all overdue. + d1.last_successful_download = datetime(2020, 1, 1, tzinfo=pytz.UTC) + d1.next_download = datetime(2020, 1, 2, tzinfo=pytz.UTC) + d1.status = 'complete' + d2.last_successful_download = datetime(2020, 1, 2, tzinfo=pytz.UTC) + d2.next_download = datetime(2020, 1, 3, tzinfo=pytz.UTC) + d2.status = 'complete' + d3.last_successful_download = datetime(2020, 1, 3, tzinfo=pytz.UTC) + d3.next_download = datetime(2020, 1, 4, tzinfo=pytz.UTC) + d3.status = 'complete' + test_session.commit() + + # It's now Jan 10 — all three are overdue. + fake_now(datetime(2020, 1, 10, tzinfo=pytz.UTC)) + + # Renew overdue downloads and let one run. + test_download_manager.renew_recurring_downloads() + await test_download_manager.wait_for_all_downloads() + test_session.expire_all() + + # At least one download should have completed. + completed = [d for d in [d1, d2, d3] if d.is_complete] + assert len(completed) >= 1, \ + f'Expected at least one completed download, got statuses: {[d.status for d in [d1, d2, d3]]}' + + # Advance 30 seconds — simulating the real download cycle interval. In production, real time + # advances between when calculate_next_download sets next_download=now() and when + # renew_recurring_downloads checks if next_download < now(). + fake_now(datetime(2020, 1, 10, 0, 0, 30, tzinfo=pytz.UTC)) + + # The completed download should NOT be renewed — its next_download should be in the future. + test_download_manager.renew_recurring_downloads() + test_session.expire_all() + + for d in completed: + assert not d.is_new, \ + f'{d.url} was renewed 30 seconds after completing — infinite re-download loop!' + + @pytest.mark.asyncio async def test_calculate_next_download_single_overdue(test_session, test_download_manager, fake_now): """A single overdue download should use normal scheduling (not spread logic).""" @@ -438,8 +495,8 @@ async def test_recurring_downloads(test_session, test_download_manager, fake_now # Download is deferred, last successful download remains the same. assert download.is_deferred, download.status_code assert download.last_successful_download == now_ - # Download should be retried after the DEFAULT_RETRY_FREQUENCY. - expected = datetime(2020, 1, 1, 3, 0, 0, 997200, tzinfo=pytz.UTC) + # Download should be retried using exponential backoff: now + min(3^attempts * hour, frequency). + expected = datetime(2020, 1, 1, 3, 0, 1, tzinfo=pytz.UTC) assert download.next_download == expected # Try the download again, it finally succeeds.