From e5dd34ae0efa2033ad5caf8841ffc72e695685f9 Mon Sep 17 00:00:00 2001 From: CaliBrain Date: Thu, 17 Sep 2026 16:27:53 -0400 Subject: [PATCH] fix: unbreak main and follow up on the Blackhole handoff review (#1346) DownloadHistoryService.record_download and updated the single production caller, but not the eleven in the test suite, leaving main red with 32 failures. Pass None, which is what the pre-#1336 behaviour recorded. For the Blackhole handoff (#1345): add_download publishes the torrent before the cancel check runs, and BlackholeClient.remove() is a no-op, so the watcher picks the file up regardless. Reporting a bare "Cancelled" hid that from the user. Name the completed handoff in the cancellation message instead, drop the _handle_cancelled_download call whose usenet branch cannot apply to a handoff-only client, and record why the orchestrator no longer verifies HandoffResult.path. Finally, make tests/direct_download a package: test_libgen_extract.py imports tests.libgen.sample_html across test directories, so without an __init__.py pytest named its modules by bare basename and a same-named module elsewhere would collide. --- shelfmark/download/clients/base_handler.py | 20 +++++-- shelfmark/download/orchestrator.py | 9 +++- shelfmark/main.py | 7 ++- .../direct_download/annas_archive.py | 4 +- .../release_sources/direct_download/source.py | 5 +- .../components/activity/activityMappers.ts | 3 +- .../src/components/resultsViews/CardView.tsx | 11 +++- .../components/resultsViews/CompactView.tsx | 10 +++- .../src/components/resultsViews/ListView.tsx | 5 +- src/frontend/src/utils/releasePayload.ts | 28 ++++++---- src/frontend/src/utils/requestPayload.ts | 54 ++++++++++--------- tests/core/test_activity_routes_api.py | 8 +++ tests/core/test_download_api_guardrails.py | 2 + tests/core/test_download_history_service.py | 1 + tests/direct_download/__init__.py | 1 + tests/download/test_orchestrator_lifecycle.py | 9 +++- 16 files changed, 127 insertions(+), 50 deletions(-) create mode 100644 tests/direct_download/__init__.py diff --git a/shelfmark/download/clients/base_handler.py b/shelfmark/download/clients/base_handler.py index f52ea208..c4ca3cbc 100644 --- a/shelfmark/download/clients/base_handler.py +++ b/shelfmark/download/clients/base_handler.py @@ -873,11 +873,25 @@ class ExternalClientHandler(DownloadHandler, ABC): ) if getattr(client, "handoff_only", False) is True: if cancel_flag.is_set(): - self._handle_cancelled_download( - client, download_id, request.protocol, status_callback + # The file is published and unpublishing it would race a watcher + # that may already have consumed it, so the handoff stands even + # though the task is cancelled. Say so rather than leaving a bare + # "Cancelled" the user cannot act on. + logger.info( + "Cancelled after handoff to %s; leaving publication in place: %s", + client.name, + download_id, + ) + status_callback( + "cancelled", + f"Cancelled, but the torrent was already handed off to {client.name}", ) return None - # A watcher can consume the publication as soon as add_download returns. + # A watcher can consume the publication as soon as add_download returns, + # so the handoff completes here rather than in the poll loop. The + # orchestrator deliberately does not check that this path still exists: + # a consumed publication is indistinguishable from a bogus one, and + # treating it as an error is the failure this avoids (#1345). progress_callback(100) self._on_download_complete(task) return HandoffResult( diff --git a/shelfmark/download/orchestrator.py b/shelfmark/download/orchestrator.py index 845769ee..dc1e55cd 100644 --- a/shelfmark/download/orchestrator.py +++ b/shelfmark/download/orchestrator.py @@ -320,7 +320,14 @@ def queue_release( logger.info("Release already in queue: %s", task.title) return False, "Release is already in the download queue" - logger.info("Release queued with priority %s: %s (downloads=%s, release_data.downloads=%s, extra=%s)", priority, task.title, task.downloads, release_data.get("downloads"), extra) + logger.info( + "Release queued with priority %s: %s (downloads=%s, release_data.downloads=%s, extra=%s)", + priority, + task.title, + task.downloads, + release_data.get("downloads"), + extra, + ) # Broadcast status update via WebSocket if ws_manager: diff --git a/shelfmark/main.py b/shelfmark/main.py index 2af333a4..7a63edf2 100644 --- a/shelfmark/main.py +++ b/shelfmark/main.py @@ -1073,7 +1073,12 @@ def api_download_release() -> Response | tuple[Response, int]: release_payload = dict(data) release_payload["content_type"] = resolved_content_type - logger.info("Download request received. keys=%s downloads=%s extra.downloads=%s", list(data.keys()), data.get("downloads"), data.get("extra", {}).get("downloads") if isinstance(data.get("extra"), dict) else None) + logger.info( + "Download request received. keys=%s downloads=%s extra.downloads=%s", + list(data.keys()), + data.get("downloads"), + data.get("extra", {}).get("downloads") if isinstance(data.get("extra"), dict) else None, + ) priority = data.get("priority", 0) # Per-user download overrides diff --git a/shelfmark/release_sources/direct_download/annas_archive.py b/shelfmark/release_sources/direct_download/annas_archive.py index ff4e3717..1d2224a4 100644 --- a/shelfmark/release_sources/direct_download/annas_archive.py +++ b/shelfmark/release_sources/direct_download/annas_archive.py @@ -821,9 +821,7 @@ def _enrich_search_results_with_downloads(books: list[BrowseRecord]) -> None: # Fetch counts in parallel using the inline_info API (cheaper than summary) counts: dict[str, int] = {} with concurrent.futures.ThreadPoolExecutor(max_workers=5) as executor: - futures = { - executor.submit(_fetch_download_count_inline, bid): bid for bid in book_ids - } + futures = {executor.submit(_fetch_download_count_inline, bid): bid for bid in book_ids} for future in concurrent.futures.as_completed(futures): bid = futures[future] try: diff --git a/shelfmark/release_sources/direct_download/source.py b/shelfmark/release_sources/direct_download/source.py index 1f885767..040e0420 100644 --- a/shelfmark/release_sources/direct_download/source.py +++ b/shelfmark/release_sources/direct_download/source.py @@ -1,5 +1,6 @@ """Direct Download search and release-source integration.""" +import contextlib from pathlib import Path from typing import TYPE_CHECKING, ClassVar @@ -43,10 +44,8 @@ def _extract_downloads(record: BrowseRecord) -> int | None: if record.info and "Downloads" in record.info: downloads_value = record.info["Downloads"] if isinstance(downloads_value, list) and len(downloads_value) > 0: - try: + with contextlib.suppress(ValueError, TypeError): downloads = int(downloads_value[0]) - except (ValueError, TypeError): - pass elif isinstance(downloads_value, (int, float)): downloads = int(downloads_value) return downloads diff --git a/src/frontend/src/components/activity/activityMappers.ts b/src/frontend/src/components/activity/activityMappers.ts index 01cccaae..697c3ccf 100644 --- a/src/frontend/src/components/activity/activityMappers.ts +++ b/src/frontend/src/components/activity/activityMappers.ts @@ -89,7 +89,8 @@ export const downloadToActivityItem = (book: Book, statusKey: DownloadStatusKey) ? Math.trunc(book.request_id) : undefined; const downloadsCount = getDownloadsCount(book); - const downloadsText = downloadsCount != null ? `${downloadsCount.toLocaleString()} downloads` : undefined; + const downloadsText = + downloadsCount != null ? `${downloadsCount.toLocaleString()} downloads` : undefined; const metaLine = joinMetaParts([ toOptionalText(book.format)?.toUpperCase(), toOptionalText(book.size), diff --git a/src/frontend/src/components/resultsViews/CardView.tsx b/src/frontend/src/components/resultsViews/CardView.tsx index ef2f4536..e20f5917 100644 --- a/src/frontend/src/components/resultsViews/CardView.tsx +++ b/src/frontend/src/components/resultsViews/CardView.tsx @@ -211,7 +211,16 @@ export const CardView = ({ {book.size} )} - {searchMode !== 'universal' && (() => { const d = getDownloadsCount(book); return d != null && d > 0 ? <> • {d.toLocaleString()} : null; })()} + {searchMode !== 'universal' && + (() => { + const d = getDownloadsCount(book); + return d != null && d > 0 ? ( + <> + {' '} + • {d.toLocaleString()}{' '} + + ) : null; + })()} )} diff --git a/src/frontend/src/components/resultsViews/CompactView.tsx b/src/frontend/src/components/resultsViews/CompactView.tsx index 72eca0ba..6d074c6d 100644 --- a/src/frontend/src/components/resultsViews/CompactView.tsx +++ b/src/frontend/src/components/resultsViews/CompactView.tsx @@ -222,7 +222,15 @@ export const CompactView = ({ {book.size} )} - {(() => { const d = getDownloadsCount(book); return d != null && d > 0 ? <> • {d.toLocaleString()} : null; })()} + {(() => { + const d = getDownloadsCount(book); + return d != null && d > 0 ? ( + <> + {' '} + • {d.toLocaleString()}{' '} + + ) : null; + })()} )} diff --git a/src/frontend/src/components/resultsViews/ListView.tsx b/src/frontend/src/components/resultsViews/ListView.tsx index c1d90a3c..e457dcf7 100644 --- a/src/frontend/src/components/resultsViews/ListView.tsx +++ b/src/frontend/src/components/resultsViews/ListView.tsx @@ -291,7 +291,10 @@ export const ListView = ({ {/* Direct mode: Downloads - Desktop only */} {searchMode !== 'universal' && (
- {(() => { const d = getDownloadsCount(book); return d != null && d > 0 ? d.toLocaleString() : '-'; })()} + {(() => { + const d = getDownloadsCount(book); + return d != null && d > 0 ? d.toLocaleString() : '-'; + })()}
)} diff --git a/src/frontend/src/utils/releasePayload.ts b/src/frontend/src/utils/releasePayload.ts index da7a08ce..db29acc0 100644 --- a/src/frontend/src/utils/releasePayload.ts +++ b/src/frontend/src/utils/releasePayload.ts @@ -8,6 +8,23 @@ export interface ReleaseDownloadOptions { bookPlan?: PackBook[]; } +/** Download count for a release, preferring what the release itself reports. */ +const releaseDownloads = (book: Book, release: Release): number => { + if (typeof release.extra?.downloads === 'number') { + return release.extra.downloads; + } + if (typeof book.downloads === 'number' && book.downloads > 0) { + return book.downloads; + } + if (typeof book.extra?.downloads === 'number') { + return book.extra.downloads; + } + if (Array.isArray(book.info?.Downloads) && book.info.Downloads.length > 0) { + return Number(book.info.Downloads[0]) || 0; + } + return 0; +}; + /** Build the body for /api/releases/download (and /api/releases/inspect). */ export function buildReleaseDownloadPayload( book: Book, @@ -30,16 +47,7 @@ export function buildReleaseDownloadPayload( format: release.format, size: release.size, size_bytes: release.size_bytes, - downloads: - typeof release.extra?.downloads === 'number' - ? release.extra.downloads - : typeof book.downloads === 'number' && book.downloads > 0 - ? book.downloads - : typeof book.extra?.downloads === 'number' - ? book.extra.downloads - : Array.isArray(book.info?.Downloads) && book.info.Downloads.length > 0 - ? Number(book.info.Downloads[0]) || 0 - : 0, + downloads: releaseDownloads(book, release), download_url: release.download_url, protocol: release.protocol, indexer: release.indexer, diff --git a/src/frontend/src/utils/requestPayload.ts b/src/frontend/src/utils/requestPayload.ts index 0bf94324..b0a82ce5 100644 --- a/src/frontend/src/utils/requestPayload.ts +++ b/src/frontend/src/utils/requestPayload.ts @@ -95,30 +95,36 @@ export const buildReleaseDataFromMetadataRelease = ( }; }; - export const buildReleaseDataFromDirectBook = (book: Book) => { - const source = getBrowseSource(book); - const downloads = - typeof book.downloads === 'number' && book.downloads > 0 - ? book.downloads - : Array.isArray(book.info?.Downloads) && book.info.Downloads.length > 0 - ? Number(book.info.Downloads[0]) || 0 - : 0; - return { - source, - source_id: book.id, - title: book.title || 'Unknown title', - author: book.author, - year: book.year, - format: book.format, - size: book.size, - downloads, - preview: book.preview, - content_type: 'ebook' as const, - // Browsing a source directly means the book record IS the release record. - language: book.language, - search_mode: 'direct' as const, - }; - }; +/** Download count for a book queued straight from a browse result. */ +const directBookDownloads = (book: Book): number => { + if (typeof book.downloads === 'number' && book.downloads > 0) { + return book.downloads; + } + if (Array.isArray(book.info?.Downloads) && book.info.Downloads.length > 0) { + return Number(book.info.Downloads[0]) || 0; + } + return 0; +}; + +export const buildReleaseDataFromDirectBook = (book: Book) => { + const source = getBrowseSource(book); + const downloads = directBookDownloads(book); + return { + source, + source_id: book.id, + title: book.title || 'Unknown title', + author: book.author, + year: book.year, + format: book.format, + size: book.size, + downloads, + preview: book.preview, + content_type: 'ebook' as const, + // Browsing a source directly means the book record IS the release record. + language: book.language, + search_mode: 'direct' as const, + }; +}; export const buildDirectRequestPayload = (book: Book): CreateRequestPayload => { const bookData = buildDirectBookRequestData(book); diff --git a/tests/core/test_activity_routes_api.py b/tests/core/test_activity_routes_api.py index 7b64b1e1..ac6371d8 100644 --- a/tests/core/test_activity_routes_api.py +++ b/tests/core/test_activity_routes_api.py @@ -71,6 +71,7 @@ def _record_terminal_download( size="1 MB", preview=None, content_type="ebook", + downloads=None, origin=origin, ) svc.finalize_download( @@ -414,6 +415,7 @@ class TestActivityRoutes: size="1 MB", preview=None, content_type="ebook", + downloads=None, origin="direct", ) active_status = _sample_status_payload() @@ -597,6 +599,7 @@ class TestActivityRoutes: size="1 MB", preview=None, content_type="ebook", + downloads=None, origin="direct", ) @@ -652,6 +655,7 @@ class TestActivityRoutes: size="1 MB", preview=None, content_type="ebook", + downloads=None, origin="requested", retry_payload=retry_payload, ) @@ -1005,6 +1009,7 @@ class TestActivityRoutes: size="1 MB", preview=None, content_type="ebook", + downloads=None, origin="direct", ) @@ -1053,6 +1058,7 @@ class TestActivityRoutes: size="1 MB", preview=None, content_type="ebook", + downloads=None, origin="requested", retry_payload=retry_payload, ) @@ -1152,6 +1158,7 @@ class TestActivityRoutes: size="1 MB", preview=None, content_type="ebook", + downloads=None, origin="requested", retry_payload=retry_payload, ) @@ -1197,6 +1204,7 @@ class TestActivityRoutes: size="2 MB", preview=None, content_type="ebook", + downloads=None, origin="direct", ) diff --git a/tests/core/test_download_api_guardrails.py b/tests/core/test_download_api_guardrails.py index a7a708d3..04ad7573 100644 --- a/tests/core/test_download_api_guardrails.py +++ b/tests/core/test_download_api_guardrails.py @@ -538,6 +538,7 @@ class TestRetryDownloadEndpointGuardrails: size="1 MB", preview=None, content_type="ebook", + downloads=None, origin="direct", retry_payload=retry_payload, ) @@ -733,6 +734,7 @@ class TestRetryDownloadEndpointGuardrails: size="1 MB", preview=None, content_type="ebook", + downloads=None, origin="requested", retry_payload=retry_payload, ) diff --git a/tests/core/test_download_history_service.py b/tests/core/test_download_history_service.py index ba0e4eef..7b29ec7d 100644 --- a/tests/core/test_download_history_service.py +++ b/tests/core/test_download_history_service.py @@ -34,6 +34,7 @@ def test_record_download_stores_utc_iso_timestamps(): size=None, preview=None, content_type="ebook", + downloads=None, origin="direct", ) diff --git a/tests/direct_download/__init__.py b/tests/direct_download/__init__.py new file mode 100644 index 00000000..2bcaa185 --- /dev/null +++ b/tests/direct_download/__init__.py @@ -0,0 +1 @@ +"""Tests for the Direct Download source.""" diff --git a/tests/download/test_orchestrator_lifecycle.py b/tests/download/test_orchestrator_lifecycle.py index 011e823e..02e825f5 100644 --- a/tests/download/test_orchestrator_lifecycle.py +++ b/tests/download/test_orchestrator_lifecycle.py @@ -2,7 +2,7 @@ from __future__ import annotations from pathlib import Path from threading import Event -from unittest.mock import ANY, MagicMock +from unittest.mock import ANY, MagicMock, call import pytest @@ -298,3 +298,10 @@ def test_download_task_completes_blackhole_handoff_without_post_processing( queue.update_progress.assert_called_once_with(task.task_id, 100) else: queue.update_progress.assert_not_called() + if handoff == "cancelled": + # The publication is already out of our hands, so a bare "Cancelled" would + # misdescribe what the watcher is about to do with it. + assert ( + call(task.task_id, "Cancelled, but the torrent was already handed off to blackhole") + in queue.update_status_message.call_args_list + )