From e8ec6cc715f002e7025bcd91328098b76b34ff6a Mon Sep 17 00:00:00 2001 From: splitsec2 <35583321+splitsec2@users.noreply.github.com> Date: Wed, 4 Nov 2026 00:41:03 -0600 Subject: [PATCH] fix(requests): reopening after a failed download remembers the release that failed (#1655) When a download fails, the terminal hook reopens its request through reopen_failed_request straight away, with no admin involved. That path cleared the release without noting that it had failed, so the next search could pick the same release again. Only the admin reopen and the status sync remembered it. Both paths now add the release to failed_releases before the reopen clears it, the same list the unattended retry already skips, and a release that is already listed is not added twice. A download that failed because its client could not be reached (the same four markers auto_download uses) is not held against the release, since the same one is fine once the client is back. This is the upstream shape of something I have been running on my own instance since mid October. ruff, basedpyright and the requests_service tests (87) pass, with five new tests for the two paths, the dedupe and the unreachable client. ## Summary by CodeRabbit * **Bug Fixes** * Failed requests are reopened after non-retryable delivery failures, with the failed release recorded when appropriate. * Connection failures caused by an unreachable client no longer mark the release as failed. * Reopening a request does not record the same release more than once or alter book details when no new release can be recorded. * Queue status updates apply the same release-tracking behavior, while requests that are not failed remain unchanged. --- shelfmark/core/requests_service.py | 25 +++- shelfmark/core/user_db.py | 46 +++++++- tests/core/test_requests_service.py | 169 +++++++++++++++++++++++++++- 3 files changed, 234 insertions(+), 6 deletions(-) diff --git a/shelfmark/core/requests_service.py b/shelfmark/core/requests_service.py index 189bba78..e2cdaa8b 100644 --- a/shelfmark/core/requests_service.py +++ b/shelfmark/core/requests_service.py @@ -197,6 +197,21 @@ _DELIVERY_PRECEDENCE: dict[str, int] = { } +# A download that failed because its client could not be reached says nothing about the release: +# the same one is fine once the client is back, and it may already be downloading there. +_CLIENT_UNREACHABLE_MARKERS = ( + "failed to connect", + "cannot connect", + "connection refused", + "connection aborted", +) + + +def _client_unreachable(message: object) -> bool: + text = str(message or "").lower() + return any(marker in text for marker in _CLIENT_UNREACHABLE_MARKERS) + + def sync_delivery_states_from_queue_status( user_db: UserDB, *, @@ -271,9 +286,12 @@ def sync_delivery_states_from_queue_status( if isinstance(raw_status_message, str) and raw_status_message.strip() else "Download failed" ) + # Reopening clears the release, so note on the request that this one failed: + # the next search skips it. reopened = user_db.reopen_failed_request( request_id, failure_reason=failure_reason, + remember_failed_release=not _client_unreachable(failure_reason), ) if reopened is not None: updated.append(reopened) @@ -700,8 +718,13 @@ def reopen_failed_request( request_id: int, failure_reason: str | None = None, ) -> dict[str, Any] | None: - """Reopen a failed fulfilled request so admins can re-approve with a new release.""" + """Reopen a failed fulfilled request so admins can re-approve with a new release. + + The release that failed is noted on the request, so the next search does not pick it again, + unless the failure was the client being unreachable, which says nothing about the release. + """ return user_db.reopen_failed_request( request_id, failure_reason=failure_reason, + remember_failed_release=not _client_unreachable(failure_reason), ) diff --git a/shelfmark/core/user_db.py b/shelfmark/core/user_db.py index a029eb64..04680ff6 100644 --- a/shelfmark/core/user_db.py +++ b/shelfmark/core/user_db.py @@ -132,6 +132,31 @@ def _require_loaded_user(user: dict[str, Any] | None) -> dict[str, Any]: return user +def _book_data_with_failed_release(request_row: dict[str, Any]) -> dict[str, Any] | None: + """The request's book data with the release it was fulfilled with added to ``failed_releases``. + + None when there is nothing to add: no readable release, or it is already listed. + """ + book_data = request_row.get("book_data") + release_data = request_row.get("release_data") + if not isinstance(book_data, dict) or not isinstance(release_data, dict): + return None + source = str(release_data.get("source") or "").strip() + source_id = str(release_data.get("source_id") or "").strip() + if not source or not source_id: + return None + tried = book_data.get("failed_releases") + tried = list(tried) if isinstance(tried, list) else [] + if any( + isinstance(entry, dict) + and entry.get("source") == source + and entry.get("source_id") == source_id + for entry in tried + ): + return None + return {**book_data, "failed_releases": [*tried, {"source": source, "source_id": source_id}]} + + def get_users_db_path(config_dir: str | None = None) -> str: """Return the configured users database path.""" root = config_dir or os.environ.get("CONFIG_DIR", "/config") @@ -919,8 +944,14 @@ class UserDB: request_id: int, *, failure_reason: str | None = None, + remember_failed_release: bool = False, ) -> dict[str, Any] | None: - """Reopen a failed fulfilled request so admins can re-approve it.""" + """Reopen a failed fulfilled request so admins can re-approve it. + + With ``remember_failed_release``, the release it was fulfilled with is added to + ``book_data["failed_releases"]`` in the same write that reopens it, so a search that + picks the request up as soon as it is pending already skips that release. + """ normalized_failure_reason = None if isinstance(failure_reason, str): normalized_failure_reason = failure_reason.strip() or None @@ -951,6 +982,16 @@ class UserDB: ): return None + failed_book_data = ( + _book_data_with_failed_release(current_request) + if remember_failed_release + else None + ) + book_data_json = ( + None + if failed_book_data is None + else self._serialize_json(failed_book_data, "book_data") + ) conn.execute( """ UPDATE download_requests @@ -958,12 +999,13 @@ class UserDB: delivery_state = 'none', delivery_updated_at = NULL, release_data = NULL, + book_data = COALESCE(?, book_data), last_failure_reason = ?, reviewed_by = NULL, reviewed_at = NULL WHERE id = ? """, - (normalized_failure_reason, request_id), + (book_data_json, normalized_failure_reason, request_id), ) updated_row = conn.execute( "SELECT * FROM download_requests WHERE id = ?", diff --git a/tests/core/test_requests_service.py b/tests/core/test_requests_service.py index 0870553c..c2397135 100644 --- a/tests/core/test_requests_service.py +++ b/tests/core/test_requests_service.py @@ -582,15 +582,16 @@ def test_reopen_failed_request_reverts_to_pending_from_queued_and_clears_on_refu def test_reopen_failed_request_delegates_to_user_db_public_api(): class StubUserDB: def __init__(self) -> None: - self.calls: list[tuple[int, str | None]] = [] + self.calls: list[tuple[int, str | None, bool]] = [] def reopen_failed_request( self, request_id: int, *, failure_reason: str | None = None, + remember_failed_release: bool = False, ) -> dict[str, object]: - self.calls.append((request_id, failure_reason)) + self.calls.append((request_id, failure_reason, remember_failed_release)) return {"id": request_id, "status": "pending"} user_db = StubUserDB() @@ -603,7 +604,7 @@ def test_reopen_failed_request_delegates_to_user_db_public_api(): assert reopened == {"id": 7, "status": "pending"} # Service layer passes failure_reason through; normalization happens in UserDB. - assert user_db.calls == [(7, " Download failed: Timeout ")] + assert user_db.calls == [(7, " Download failed: Timeout ", True)] def test_reopen_failed_request_does_not_reopen_completed_delivery(user_db): @@ -764,6 +765,76 @@ def test_sync_delivery_states_reopens_fulfilled_request_when_error_is_not_retrya assert refreshed["last_failure_reason"] == "Staged retry source no longer exists" +def test_sync_delivery_states_reopening_remembers_the_release_that_failed(user_db): + user = user_db.create_user(username="alice") + fulfilled_request = user_db.create_request( + user_id=user["id"], + source_hint="prowlarr", + content_type="ebook", + request_level="release", + policy_mode="request_release", + book_data=_book_data(), + release_data={"source": "prowlarr", "source_id": "dead-rel", "title": "Dead"}, + status="fulfilled", + delivery_state="queued", + ) + + sync_delivery_states_from_queue_status( + user_db, + queue_status={ + "error": { + "dead-rel": { + "id": "dead-rel", + "request_id": fulfilled_request["id"], + "retry_available": False, + "status_message": "No seeders: 0 found, 1 needed, nothing downloaded in 5 min", + }, + }, + }, + user_id=user["id"], + ) + + refreshed = user_db.get_request(fulfilled_request["id"]) + assert refreshed["release_data"] is None + assert refreshed["book_data"]["failed_releases"] == [ + {"source": "prowlarr", "source_id": "dead-rel"} + ] + + +def test_sync_delivery_states_does_not_blame_the_release_when_the_client_was_unreachable(user_db): + user = user_db.create_user(username="alice") + fulfilled_request = user_db.create_request( + user_id=user["id"], + source_hint="prowlarr", + content_type="ebook", + request_level="release", + policy_mode="request_release", + book_data=_book_data(), + release_data={"source": "prowlarr", "source_id": "fine-rel", "title": "Fine"}, + status="fulfilled", + delivery_state="queued", + ) + + sync_delivery_states_from_queue_status( + user_db, + queue_status={ + "error": { + "fine-rel": { + "id": "fine-rel", + "request_id": fulfilled_request["id"], + "retry_available": False, + "status_message": "Failed to add to qbittorrent: Failed to connect to qBittorrent", + }, + }, + }, + user_id=user["id"], + ) + + refreshed = user_db.get_request(fulfilled_request["id"]) + assert refreshed["status"] == "pending" # still reopened + assert "failed_releases" not in refreshed["book_data"] # but the release is not held against + + def test_sync_delivery_states_preserves_retryable_error_state(user_db): user = user_db.create_user(username="alice") fulfilled_request = user_db.create_request( @@ -923,6 +994,98 @@ def test_sync_delivery_states_an_empty_queue_skips_the_request_history( assert [row["delivery_state"] for row in updated] == ["downloading"] +def test_reopening_after_a_failed_download_remembers_the_release(user_db): + # The terminal hook reopens a request the moment its download errors, with no admin involved. + user = user_db.create_user(username="alice") + request = _fulfilled_with_release(user_db, user["id"], "rejected-release") + + reopened = reopen_failed_request( + user_db, + request_id=request["id"], + failure_reason="Peer transfer failed: Rejected (01.mp3)", + ) + + assert reopened["status"] == "pending" + assert reopened["book_data"]["failed_releases"] == [ + {"source": "slskd", "source_id": "rejected-release"} + ] + assert reopened["last_failure_reason"] == "Peer transfer failed: Rejected (01.mp3)" + + +def test_reopening_records_the_failed_release_in_the_same_write(user_db, monkeypatch): + # A second write would leave the request pending without failed_releases for a moment, and + # an automatic search in that window could pick the failed release again. + user = user_db.create_user(username="alice") + request = _fulfilled_with_release(user_db, user["id"], "rejected-release") + user_db.update_request(request["id"], book_data={**request["book_data"], "note": "kept"}) + + def no_second_write(*_args, **_kwargs): + raise AssertionError("book_data must be written by the reopen itself") + + monkeypatch.setattr(user_db, "update_request", no_second_write) + + reopened = reopen_failed_request( + user_db, request_id=request["id"], failure_reason="Download failed" + ) + + assert reopened["status"] == "pending" + assert reopened["book_data"]["failed_releases"] == [ + {"source": "slskd", "source_id": "rejected-release"} + ] + assert reopened["book_data"]["note"] == "kept" + + +def test_reopening_after_an_unreachable_client_does_not_blame_the_release(user_db): + user = user_db.create_user(username="alice") + request = _fulfilled_with_release(user_db, user["id"], "fine-release") + + reopened = reopen_failed_request( + user_db, + request_id=request["id"], + failure_reason="Failed to connect to qBittorrent", + ) + + assert reopened["status"] == "pending" + assert "failed_releases" not in reopened["book_data"] + + +def test_reopening_twice_lists_the_release_once(user_db): + user = user_db.create_user(username="alice") + request = _fulfilled_with_release(user_db, user["id"], "same-release") + reopen_failed_request(user_db, request_id=request["id"], failure_reason="Download failed") + user_db.update_request( + request["id"], + status="fulfilled", + delivery_state="queued", + release_data={"source": "slskd", "source_id": "same-release", "title": "x"}, + ) + + reopened = reopen_failed_request( + user_db, request_id=request["id"], failure_reason="Download failed" + ) + + assert len(reopened["book_data"]["failed_releases"]) == 1 + + +def test_reopening_a_request_that_is_not_failed_changes_nothing(user_db): + user = user_db.create_user(username="alice") + request = user_db.create_request( + user_id=user["id"], + source_hint=None, + content_type="ebook", + request_level="book", + policy_mode="request_book", + book_data=_book_data(), + status="pending", + ) + + assert ( + reopen_failed_request(user_db, request_id=request["id"], failure_reason="Download failed") + is None + ) + assert "failed_releases" not in user_db.get_request(request["id"])["book_data"] + + def test_create_request_rejects_non_dict_book_data(user_db): user = user_db.create_user(username="alice") with pytest.raises(RequestServiceError, match="book_data must be an object"):