diff --git a/shelfmark/core/download_history_service.py b/shelfmark/core/download_history_service.py index e43998d7..86c8484e 100644 --- a/shelfmark/core/download_history_service.py +++ b/shelfmark/core/download_history_service.py @@ -16,6 +16,7 @@ from shelfmark.core.request_helpers import ( normalize_optional_text, now_utc_iso, ) +from shelfmark.core.utils import transform_cover_url logger = setup_logger(__name__) @@ -203,7 +204,7 @@ class DownloadHistoryService: "author": row.get("author"), "format": row.get("format"), "size": row.get("size"), - "preview": row.get("preview"), + "preview": transform_cover_url(row.get("preview"), str(row.get("task_id") or "")), "content_type": row.get("content_type"), "source": row.get("source"), "source_display_name": row.get("source_display_name"), @@ -282,6 +283,11 @@ class DownloadHistoryService: On first queue: inserts a new row. On retry (row already exists): resets the row back to 'active' so the normal finalize path works when the retry completes. + + A task id comes from the release, so a second request that chooses the same release + reuses the row. It then takes the row over: the request, requester and what the card + shows follow the new download, instead of the new download appearing under the + request that first used the release (with that request's notes and state). """ normalized_task_id = _normalize_task_id(task_id) normalized_user_id = normalize_optional_positive_int(user_id, "user_id") @@ -318,7 +324,18 @@ class DownloadHistoryService: download_path = NULL, retry_payload = excluded.retry_payload, terminal_at = ?, - downloads = excluded.downloads + downloads = excluded.downloads, + request_id = COALESCE(excluded.request_id, request_id), + user_id = COALESCE(excluded.user_id, user_id), + username = COALESCE(excluded.username, username), + title = excluded.title, + author = excluded.author, + format = excluded.format, + size = excluded.size, + preview = excluded.preview, + content_type = excluded.content_type, + origin = excluded.origin, + queued_at = excluded.queued_at """, ( normalized_task_id, @@ -423,6 +440,18 @@ class DownloadHistoryService: finally: conn.close() + def list_completed_request_ids(self) -> set[int]: + """Return the id of every request that has a download that finished.""" + conn = self._connect() + try: + rows = conn.execute( + "SELECT DISTINCT request_id FROM download_history " + "WHERE request_id IS NOT NULL AND final_status = 'complete'" + ).fetchall() + return {int(row["request_id"]) for row in rows} + finally: + conn.close() + def list_request_ids(self) -> set[int]: """Return the id of every request that has at least one download row.""" conn = self._connect() diff --git a/shelfmark/core/startup_reconcile.py b/shelfmark/core/startup_reconcile.py index 1ce984af..39df5078 100644 --- a/shelfmark/core/startup_reconcile.py +++ b/shelfmark/core/startup_reconcile.py @@ -30,6 +30,19 @@ logger = setup_logger(__name__) # it relabels a stale row that has none. INTERRUPTED_MESSAGE = "Interrupted" +# The message a row gets when its request was delivered by another download. +SUPERSEDED_MESSAGE = "Superseded by a completed download" + +# Delivery states of a download that is still working, as far as the request knows. +_IN_FLIGHT_STATES = frozenset( + { + QueueStatus.QUEUED, + QueueStatus.RESOLVING, + QueueStatus.LOCATING, + QueueStatus.DOWNLOADING, + } +) + # The failure reason a reopened request is given. REOPEN_REASON = "Interrupted by a restart" @@ -55,6 +68,14 @@ def _is_recent(value: object, *, now: datetime, window: timedelta) -> bool: return stamp is not None and stamp >= now - window +def _request_delivered(user_db: UserDB, request_id: int, completed: set[int]) -> bool: + """Whether the request already has its book: a finished download, or a delivered state.""" + if request_id in completed: + return True + request = user_db.get_request(request_id) + return bool(request) and request.get("delivery_state") == QueueStatus.COMPLETE + + def _request_stamp(request: dict[str, Any]) -> object: for key in ("delivery_updated_at", "reviewed_at", "created_at"): if request.get(key): @@ -89,8 +110,19 @@ def reconcile_interrupted_downloads( marked = 0 reopened = 0 + delivered = history_service.list_completed_request_ids() for row in history_service.list_active(): request_id = row.get("request_id") + if request_id and _request_delivered(user_db, int(request_id), delivered): + # The request already has its book. This row is an attempt that was overtaken, so + # it is closed as superseded, not shown as an interrupted failure with a retry. + history_service.finalize_download( + task_id=str(row["task_id"]), + final_status=QueueStatus.CANCELLED.value, + status_message=SUPERSEDED_MESSAGE, + ) + marked += 1 + continue if request_id: if not _is_recent(row.get("queued_at"), now=now, window=reopen_window): continue @@ -106,8 +138,9 @@ def reconcile_interrupted_downloads( ) marked += 1 - # A request can be "queued" with no history row at all, for instance when the row was - # never written. After a restart nothing is queued, so it is stranded the same way. A + # A request can be "queued" (or "downloading", when its row moved to another request) with + # no history row at all, for instance when the row was never written. After a restart + # nothing is in flight, so it is stranded the same way. A # request that has a row is not one of these: delivery_state is only synced while # something polls the queue, so a download that finished with nobody watching still says # "queued", and the activity API settles it from the row on its next read. @@ -115,7 +148,7 @@ def reconcile_interrupted_downloads( for request in user_db.list_requests(status=RequestStatus.FULFILLED): if int(request["id"]) in requests_with_history: continue - if request.get("delivery_state") != QueueStatus.QUEUED: + if request.get("delivery_state") not in _IN_FLIGHT_STATES: continue if _is_recent(_request_stamp(request), now=now, window=reopen_window) and ( user_db.reopen_failed_request(int(request["id"]), failure_reason=REOPEN_REASON) diff --git a/tests/core/test_download_history_service.py b/tests/core/test_download_history_service.py index 7b29ec7d..d8c8afeb 100644 --- a/tests/core/test_download_history_service.py +++ b/tests/core/test_download_history_service.py @@ -50,3 +50,79 @@ def test_record_download_stores_utc_iso_timestamps(): assert row is not None assert "+00:00" in row["queued_at"] assert "+00:00" in row["terminal_at"] + + +def _service(tmpdir): + db_path = os.path.join(tmpdir, "users.db") + user_db = UserDB(db_path) + user_db.initialize() + return user_db, DownloadHistoryService(db_path) + + +def _record(service, *, request_id, title, preview=None, user_id=None): + service.record_download( + task_id="release-1", + user_id=user_id, + username="reader" if user_id else None, + request_id=request_id, + source="audiobookbay", + source_display_name=None, + title=title, + author=None, + file_format=None, + size=None, + preview=preview, + content_type="audiobook", + downloads=None, + origin="requested" if request_id else "direct", + ) + + +def test_a_second_request_that_picks_the_same_release_takes_the_row_over(): + with tempfile.TemporaryDirectory() as tmpdir: + _user_db, service = _service(tmpdir) + _record(service, request_id=173, title="Cross and Sampson") + service.finalize_download(task_id="release-1", final_status="error", status_message="x") + + _record(service, request_id=168, title="Cross Down") + + row = service.get_by_task_id("release-1") + assert row["request_id"] == 168 + assert row["title"] == "Cross Down" + assert row["final_status"] == "active" + + +def test_a_retry_without_a_request_keeps_the_request_the_row_had(): + with tempfile.TemporaryDirectory() as tmpdir: + _user_db, service = _service(tmpdir) + _record(service, request_id=7, title="Book") + + _record(service, request_id=None, title="Book") + + assert service.get_by_task_id("release-1")["request_id"] == 7 + + +def test_the_download_payload_serves_the_cover_through_the_local_proxy(monkeypatch): + monkeypatch.setattr("shelfmark.config.env.is_covers_cache_enabled", lambda: True) + with tempfile.TemporaryDirectory() as tmpdir: + _user_db, service = _service(tmpdir) + _record( + service, + request_id=None, + title="Book", + preview="https://assets.hardcover.app/edition/1/a.jpg", + ) + row = service.get_by_task_id("release-1") + + payload = DownloadHistoryService.to_download_payload(row) + + assert payload["preview"].startswith("/api/covers/release-1?url=") + + +def test_the_download_payload_leaves_a_missing_cover_alone(): + assert ( + DownloadHistoryService.to_download_payload( + {"task_id": "t", "final_status": "error", "preview": None} + )["preview"] + is None + ) diff --git a/tests/core/test_startup_reconcile.py b/tests/core/test_startup_reconcile.py index 87a0a030..8c66ef9e 100644 --- a/tests/core/test_startup_reconcile.py +++ b/tests/core/test_startup_reconcile.py @@ -158,15 +158,17 @@ def test_a_download_with_no_request_is_closed_out_and_keeps_its_manual_retry(db) assert history.is_retry_available(row) is True -def test_a_download_whose_request_already_completed_is_left_alone(db): +def test_a_download_whose_request_already_completed_is_closed_without_reopening_it(db): user_db, history, path = db user = user_db.create_user(username="reader", role="user") request = _request(user_db, user["id"], state="complete") _download(history, path, task_id="t1", user_id=user["id"], request_id=request["id"]) - assert _run(user_db, history) == {"marked": 0, "reopened": 0} + assert _run(user_db, history) == {"marked": 1, "reopened": 0} - assert history.get_by_task_id("t1")["final_status"] == "active" + # Closed as superseded rather than left "active", which the activity list would show as an + # interrupted failure on a request that has its book. + assert history.get_by_task_id("t1")["final_status"] == "cancelled" assert user_db.get_request(request["id"])["delivery_state"] == "complete" @@ -248,3 +250,74 @@ def test_the_window_is_configurable(db): ) assert _run(user_db, history, reopen_window=timedelta(hours=48)) == {"marked": 1, "reopened": 1} + + +def test_a_stale_row_of_a_request_that_was_delivered_is_closed_as_superseded(db): + user_db, history, path = db + user = user_db.create_user(username="reader", role="user") + request = _request(user_db, user["id"], state="complete", hours_ago=300) + _download( + history, path, task_id="old", user_id=user["id"], request_id=request["id"], hours_ago=500 + ) + + assert _run(user_db, history) == {"marked": 1, "reopened": 0} + + row = history.get_by_task_id("old") + assert row["final_status"] == "cancelled" + assert row["status_message"] == startup_reconcile.SUPERSEDED_MESSAGE + assert user_db.get_request(request["id"])["status"] == "fulfilled" # not reopened + + +def test_a_stale_row_is_superseded_when_another_row_of_the_request_finished(db): + user_db, history, path = db + user = user_db.create_user(username="reader", role="user") + request = _request(user_db, user["id"], state="queued", hours_ago=300) + _download( + history, path, task_id="old", user_id=user["id"], request_id=request["id"], hours_ago=500 + ) + _download( + history, + path, + task_id="done", + user_id=user["id"], + request_id=request["id"], + hours_ago=400, + final_status="complete", + ) + + assert _run(user_db, history) == {"marked": 1, "reopened": 0} + assert history.get_by_task_id("old")["final_status"] == "cancelled" + + +def test_an_old_row_of_an_undelivered_request_is_still_left_alone(db): + user_db, history, path = db + user = user_db.create_user(username="reader", role="user") + request = _request(user_db, user["id"], state="queued", hours_ago=300) + _download( + history, path, task_id="old", user_id=user["id"], request_id=request["id"], hours_ago=500 + ) + + assert _run(user_db, history) == {"marked": 0, "reopened": 0} + assert history.get_by_task_id("old")["final_status"] == "active" + + +@pytest.mark.parametrize("state", ["queued", "resolving", "locating", "downloading"]) +def test_a_request_with_no_row_that_says_it_is_in_flight_is_reopened(db, state): + user_db, history, _path = db + user = user_db.create_user(username="reader", role="user") + request = _request(user_db, user["id"], state=state) + + assert _run(user_db, history) == {"marked": 0, "reopened": 1} + + assert user_db.get_request(request["id"])["status"] == "pending" + + +@pytest.mark.parametrize("state", ["complete", "error", "cancelled", "none"]) +def test_a_request_with_no_row_in_any_other_state_is_left_alone(db, state): + user_db, history, _path = db + user = user_db.create_user(username="reader", role="user") + request = _request(user_db, user["id"], state=state) + + assert _run(user_db, history) == {"marked": 0, "reopened": 0} + + assert user_db.get_request(request["id"])["status"] == "fulfilled"