From 88cc734db2c6bfe4b642f90cf4f12ff3c40ec7c2 Mon Sep 17 00:00:00 2001 From: splitsec2 <35583321+splitsec2@users.noreply.github.com> Date: Tue, 20 Oct 2026 21:20:04 -0600 Subject: [PATCH] fix(activity): a reused release follows the new request, and stale rows of a delivered request are closed (#1496) Two things that go wrong when a second request picks a release a first request already used. A task id comes from the release, so the history row is reused. **The row stays with the first request.** Recording a download kept the first request's id, notes and state. On my instance a card for one book showed up under the request for a different book, with that request's note on it. Recording a download now moves the row to the request that queued it and restarts its queue time. A retry with no request keeps the request the row had. **Old rows of a delivered request show as failures forever.** The startup sweep leaves an old request-linked row alone so its retry stays, but one whose request already has its book (delivered, or another download of it finished) is now closed as "Superseded by a completed download" instead of showing as an interrupted failure for ever. Also in the first commit: History cards used the saved cover URL as it was, an external address the page's own-origin image policy blocks, while running downloads use the cover proxy. They use it too. The second commit is about restarts. After a restart nothing is in flight, but a request in downloading, resolving or locating whose row had moved to another request was stranded, because only queued was reopened. Those states are reopened too. Tests cover the row takeover and the retry case, the cover through the proxy (and a missing cover), the superseded cases (delivered request, another row finished, an undelivered request is left alone) and the in-flight states with no row. Ruff and the backend suite pass. --- shelfmark/core/download_history_service.py | 33 ++++++++- shelfmark/core/startup_reconcile.py | 39 +++++++++- tests/core/test_download_history_service.py | 76 ++++++++++++++++++++ tests/core/test_startup_reconcile.py | 79 ++++++++++++++++++++- 4 files changed, 219 insertions(+), 8 deletions(-) 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"