diff --git a/shelfmark/core/activity_routes.py b/shelfmark/core/activity_routes.py index 1b8b925d..71820840 100644 --- a/shelfmark/core/activity_routes.py +++ b/shelfmark/core/activity_routes.py @@ -31,6 +31,7 @@ from shelfmark.core.request_helpers import ( populate_request_usernames, ) from shelfmark.core.request_validation import RequestStatus +from shelfmark.core.status_messages import explain_buckets, explain_request_rows from shelfmark.core.viewer_redaction import ( is_public_download_id, match_public_download_id, @@ -646,6 +647,8 @@ def register_activity_routes( user_id=actor.owner_scope, ) emit_request_updates(updated_requests) + # Plain wording for what the list shows, after the sync above has used the real text. + explain_buckets(status) request_rows = _list_visible_requests( user_db, is_admin=actor.is_admin, @@ -659,6 +662,7 @@ def register_activity_routes( if f"request:{request_id}" in hidden_item_keys: continue visible_request_rows.append(row) + visible_request_rows = explain_request_rows(visible_request_rows) if not actor.is_admin: # Redacted only now: the delivery-state sync above matches on real task ids. diff --git a/shelfmark/core/status_messages.py b/shelfmark/core/status_messages.py new file mode 100644 index 00000000..7bea99b4 --- /dev/null +++ b/shelfmark/core/status_messages.py @@ -0,0 +1,115 @@ +"""Plain wording for a download that failed or stopped: what happened and what comes next. + +The saved message is a log line ("Peer transfer failed: Errored"). Auto-download reads it to +tell a failed release from an interrupted one, so it is never rewritten; only what the activity +list shows is. +""" + +from __future__ import annotations + +from typing import Any + +from shelfmark.core.config import config as app_config +from shelfmark.core.request_helpers import coerce_int + +FAILURE_RETRY_MINUTES_SETTING = "AUTO_DOWNLOAD_FAILURE_RETRY_MINUTES" + +_SHOWN_FOR = frozenset({"error", "cancelled"}) + + +def _next_step(*, request_linked: bool, retry_minutes: int) -> str: + if request_linked and retry_minutes > 0: + return f" Another copy will be tried in about {retry_minutes} minutes." + if request_linked: + return " Use Retry to try again, or reopen the request to try another copy." + return " Use Retry to try again, or pick another copy." + + +def explain( + message: str | None, + *, + request_linked: bool, + retry_minutes: int | None = None, +) -> str | None: + """The message in plain words with the next step, or it unchanged when nothing is known.""" + if not isinstance(message, str) or not message.strip(): + return message + text = message.strip() + lowered = text.lower() + minutes = ( + retry_minutes + if retry_minutes is not None + else coerce_int(app_config.get(FAILURE_RETRY_MINUTES_SETTING, 0), 0) + ) + nxt = _next_step(request_linked=request_linked, retry_minutes=minutes) + + if lowered.startswith("superseded"): + return "This book was already delivered by another download." + if lowered.startswith("interrupted"): + return "Interrupted by a restart. Use Retry to run it again." + if lowered.startswith("peer transfer failed"): + detail = text.split(":", 1)[1].strip() if ":" in text else "" + suffix = f" ({detail})" if detail else "" + return f"The Soulseek peer dropped the transfer{suffix}." + nxt + if lowered.startswith("failed to add to slskd"): + if "direct or indirect" in lowered: + return ( + "Could not reach the Soulseek peer, directly or through the Soulseek server " + "(tried twice)." + nxt + ) + if '("' in text and "timed out" not in lowered: + # slskd gave a reason, and it is not a timeout. + return "slskd would not queue the download (tried twice)." + nxt + # A timeout, or the bare 500 an older attempt saved before slskd's reason was kept; every + # one of those that slskd's own log explained was a peer that did not answer in time. + return "The Soulseek peer did not answer slskd's queue request (tried twice)." + nxt + if lowered.startswith("download failed:"): + detail = text.split(":", 1)[1].strip() + return f"The download hit an internal error ({detail})." + nxt + if lowered.startswith("retrying after"): + return "Retrying automatically after a failed download." + if lowered.startswith("download stalled"): + return "Nothing was downloading, so it was stopped." + nxt + if lowered.startswith("no seeders"): + return text.rstrip(".") + "; the torrent was removed." + nxt + if lowered.startswith("all download sources failed"): + return "None of the download sources could provide this book." + nxt + return text + + +def explain_request_rows(rows: list[dict[str, Any]]) -> list[dict[str, Any]]: + """The request rows with ``last_failure_reason`` in plain words (copies; the stored text stays).""" + minutes = coerce_int(app_config.get(FAILURE_RETRY_MINUTES_SETTING, 0), 0) + explained: list[dict[str, Any]] = [] + for row in rows: + reason = row.get("last_failure_reason") + if not isinstance(reason, str) or not reason.strip(): + explained.append(row) + continue + # A request that was reopened after a restart is searched again by the next pass. + text = ( + "Interrupted by a restart; it will be tried again." + if reason.strip().lower().startswith("interrupted by a restart") + else explain(reason, request_linked=True, retry_minutes=minutes) + ) + explained.append({**row, "last_failure_reason": text}) + return explained + + +def explain_buckets(status: dict[str, dict[str, Any]]) -> None: + """Rewrite the messages of the error and cancelled entries of a status dict, in place.""" + minutes = coerce_int(app_config.get(FAILURE_RETRY_MINUTES_SETTING, 0), 0) + for bucket in _SHOWN_FOR: + entries = status.get(bucket, {}) + for task_id, payload in list(entries.items()): + if not isinstance(payload, dict): + continue + # A copy, so a payload the queue hands out again is never explained twice. + entries[task_id] = { + **payload, + "status_message": explain( + payload.get("status_message"), + request_linked=bool(payload.get("request_id")), + retry_minutes=minutes, + ), + } diff --git a/tests/core/test_activity_routes_api.py b/tests/core/test_activity_routes_api.py index cf1e33e5..5d651314 100644 --- a/tests/core/test_activity_routes_api.py +++ b/tests/core/test_activity_routes_api.py @@ -1404,7 +1404,8 @@ class TestActivityRoutes: assert response.status_code == 200 assert "stale-active-task" in response.json["status"]["error"] assert ( - response.json["status"]["error"]["stale-active-task"]["status_message"] == "Interrupted" + response.json["status"]["error"]["stale-active-task"]["status_message"] + == "Interrupted by a restart. Use Retry to run it again." ) def test_snapshot_preserves_retry_for_stale_active_requested_download( @@ -1453,7 +1454,9 @@ class TestActivityRoutes: assert response.status_code == 200 viewer_id = public_download_id(task_id) - assert response.json["status"]["error"][viewer_id]["status_message"] == "Interrupted" + assert response.json["status"]["error"][viewer_id]["status_message"].startswith( + "Interrupted by a restart" + ) assert response.json["status"]["error"][viewer_id]["retry_available"] is True def test_snapshot_includes_retry_available_for_live_terminal_downloads( diff --git a/tests/core/test_status_messages.py b/tests/core/test_status_messages.py new file mode 100644 index 00000000..76282dbc --- /dev/null +++ b/tests/core/test_status_messages.py @@ -0,0 +1,144 @@ +"""What the activity list says about a download that failed, and what happens next.""" + +from __future__ import annotations + +import pytest + +from shelfmark.core.status_messages import explain, explain_buckets + + +def _explain(message, *, linked=True, minutes=20): + return explain(message, request_linked=linked, retry_minutes=minutes) + + +@pytest.mark.parametrize( + ("raw", "expected"), + [ + ( + "Peer transfer failed: Errored", + "The Soulseek peer dropped the transfer (Errored). " + "Another copy will be tried in about 20 minutes.", + ), + ( + "Failed to add to slskd: 500 Server Error: Internal Server Error for url: http://x", + "The Soulseek peer did not answer slskd's queue request (tried twice). " + "Another copy will be tried in about 20 minutes.", + ), + ( + "Download stalled (no activity for 300s)", + "Nothing was downloading, so it was stopped. " + "Another copy will be tried in about 20 minutes.", + ), + ( + "No seeders: 0 found, 1 needed, for 5 min (12% downloaded)", + "No seeders: 0 found, 1 needed, for 5 min (12% downloaded); the torrent was removed. " + "Another copy will be tried in about 20 minutes.", + ), + ( + "All download sources failed", + "None of the download sources could provide this book. " + "Another copy will be tried in about 20 minutes.", + ), + ], +) +def test_a_failure_says_what_happened_and_what_comes_next(raw, expected): + assert _explain(raw) == expected + + +def test_interrupted_says_a_restart_did_it(): + assert _explain("Interrupted") == "Interrupted by a restart. Use Retry to run it again." + + +def test_a_superseded_row_says_the_book_was_delivered(): + assert ( + _explain("Superseded by a completed download") + == "This book was already delivered by another download." + ) + + +def test_without_the_quick_retry_a_request_is_pointed_at_retry_or_reopen(): + assert _explain("Peer transfer failed: TimedOut", minutes=0).endswith( + "Use Retry to try again, or reopen the request to try another copy." + ) + + +def test_a_manual_download_is_never_promised_an_automatic_retry(): + text = _explain("Peer transfer failed: Errored", linked=False) + + assert "will be tried" not in text + assert text.endswith("Use Retry to try again, or pick another copy.") + + +def test_an_unknown_message_and_an_empty_one_are_left_as_they_are(): + assert _explain("Something new broke") == "Something new broke" + assert _explain(None) is None + assert _explain(" ") == " " + + +def test_the_buckets_are_rewritten_without_touching_the_original_payload(): + original = {"id": "t", "request_id": 5, "status_message": "Peer transfer failed: Errored"} + status = {"error": {"t": original}, "cancelled": {}, "complete": {"c": {"status_message": "x"}}} + + explain_buckets(status) + + assert status["error"]["t"]["status_message"].startswith("The Soulseek peer dropped") + assert original["status_message"] == "Peer transfer failed: Errored" + assert status["complete"]["c"]["status_message"] == "x" + + +@pytest.mark.parametrize( + ("raw", "expected"), + [ + ( + 'Failed to add to slskd: 500 Server Error: Internal Server Error for url: http://x/batches ("Failed to connect to user u: Failed to establish a direct or indirect message connection to u (1.2.3.4:5)")', + "Could not reach the Soulseek peer, directly or through the Soulseek server (tried twice). " + "Another copy will be tried in about 20 minutes.", + ), + ( + 'Failed to add to slskd: 500 Server Error for url: http://x/batches ("The wait timed out after 5000 milliseconds")', + "The Soulseek peer did not answer slskd's queue request (tried twice). " + "Another copy will be tried in about 20 minutes.", + ), + ( + 'Failed to add to slskd: 500 Server Error for url: http://x/batches ("The peer is banned")', + "slskd would not queue the download (tried twice). " + "Another copy will be tried in about 20 minutes.", + ), + ( + "Download failed: TypeError", + "The download hit an internal error (TypeError). " + "Another copy will be tried in about 20 minutes.", + ), + ( + "Retrying after a 20-minute cooldown (failed; delivery was queued)", + "Retrying automatically after a failed download.", + ), + ], +) +def test_slskd_and_internal_failures_say_what_went_wrong(raw, expected): + assert _explain(raw) == expected + + +def test_a_request_reopened_after_a_restart_says_it_will_be_tried_again(): + from shelfmark.core.status_messages import explain_request_rows + + out = explain_request_rows([{"id": 1, "last_failure_reason": "Interrupted by a restart"}]) + + assert out[0]["last_failure_reason"] == "Interrupted by a restart; it will be tried again." + + +def test_request_failure_reasons_are_explained_on_copies(): + from shelfmark.core.status_messages import explain_request_rows + + rows = [ + {"id": 1, "last_failure_reason": "Download failed: TypeError"}, + {"id": 2, "last_failure_reason": None}, + {"id": 3}, + ] + + out = explain_request_rows(rows) + + assert out[0]["last_failure_reason"].startswith("The download hit an internal error") + assert out[1]["last_failure_reason"] is None + assert "last_failure_reason" not in out[2] + assert rows[0]["last_failure_reason"] == "Download failed: TypeError" # untouched