fix(download): reconcile the two AA redirect-loop rescues (#1213)

#1210 and #1212 both added a DDoS-Guard `?check=1` rescue, and #1212 was
branched before #1210 landed, so the merged result had two of them with
identical guards. #1212's inline handoff returns before the raise that
#1210's exception handler keys on, so the handler was shadowed and its
stale-cookie purge — the substance of #1210 — never ran. Its regression
test has been failing on main since the merge.

Fold both into one path:

- `_redirect_loop_handoff()` purges the host's stale clearance cookies,
  then bypasses, so the inline AA handoff and the exception handler
  cannot drift apart again.
- The exception handler keeps its own reason to exist: non-AA hosts run
with allow_redirects=True, so `requests` raises the loop itself and the
  manual AA follower never sees it. It now invokes the bypasser directly
  rather than setting a flag and continuing, which was a no-op at
  MAX_RETRY=1 for the same reason the 403 handoff was.
- An unrescuable loop returns empty instead of raising TooManyRedirects
into the retry path. That error is not retryable and carries no status,
  so `/dyn/md5/summary` (allow_bypasser_fallback=False) re-ran the full
  6-redirect loop on all 10 attempts: 60 requests to AA and ~30s of
  backoff, measured. Every AA mirror shares the challenge, so there is
  nothing to rotate to.
- `allow_bypasser_fallback` docs now describe what the flag actually
  gates; the old text predated #1198 and named the wrong callers.

shelfmark/config/migrations.py

@@ -67,7 +67,10 @@ def _pick_legacy_settings_restriction(config: dict[str, Any]) -> bool | None:
 def migrate_audiobook_formats(
     *,
     load_general_config: Callable[[], dict[str, Any]],
-    save_general_config: Callable[[dict[str, Any]], None],
+    # `object` rather than `None`: the result is discarded, and savers that report
+    # success (settings_registry.save_config_file returns bool) are not assignable to a
+    # `-> None` callable.
+    save_general_config: Callable[[dict[str, Any]], object],
     widened_formats: Sequence[str],
     logger: MigrationLogger,
 ) -> None:

shelfmark/download/http.py

@@ -272,8 +272,11 @@ def html_get_page(
         selector: Mirror selector used for AA mirror and DNS rotation.
         cancel_flag: Optional event used to abort retries early.
         status_callback: Optional callback for UI status updates.
-        allow_bypasser_fallback: If False, 403 errors will trigger mirror rotation
-            instead of switching to the bypasser. Use for search operations.
+        allow_bypasser_fallback: Whether a challenge may be handed to the bypasser.
+            If False, a 403 triggers mirror rotation instead, and an AA redirect loop
+            gives up immediately rather than waiting on a browser solve. Use False for
+            best-effort fetches whose result is optional (e.g. the download count on
+            the details modal); search and detail pages pass True.
         use_bypasser: Whether to start with the bypasser instead of direct HTTP.
         include_response_url: If True, return `(html, final_url)` to expose the
             resolved response URL after redirects.
@@ -319,6 +322,29 @@ def html_get_page(
         finally:
             release_activity_grace(status_callback)
 
+    def _bypass_handoff_allowed() -> bool:
+        """Whether a challenge on the current URL may be handed to the bypasser.
+
+        allow_bypasser_fallback is honoured for the same reason the 403 path honours it:
+        callers such as the /dyn/md5/summary fetch behind the details modal pass False
+        precisely so a best-effort request fails fast instead of holding the UI open for
+        a minutes-long browser solve.
+        """
+        return allow_bypasser_fallback and _is_cf_bypass_enabled() and not use_bypasser_now
+
+    def _redirect_loop_handoff(bypass_url: str) -> str | tuple[str, str]:
+        """Drop the host's stale clearance cookies, then bypass `bypass_url`.
+
+        A `?check=1` loop is how DDoS-Guard answers a clearance cookie that has gone
+        stale, so the dead cookie has to go before the solve — otherwise it is merged
+        back over the fresh one on the next request and the loop simply resumes. Purging
+        is internal-bypasser only; with an external one get_cf_cookies_for_domain()
+        already returns {}.
+        """
+        if not _is_using_external_bypasser():
+            _get_internal_bypasser().clear_cf_cookies(urlparse(bypass_url).hostname or "")
+        return _run_bypasser(bypass_url)
+
     configured_retry = normalize_positive_int(app_config.MAX_RETRY)
     retry_limit = (
         retry if retry is not None else (configured_retry if configured_retry is not None else 1)
@@ -433,25 +459,26 @@ def html_get_page(
                     redirects_followed += 1
                     if redirects_followed > _MAX_REDIRECTS:
                         # A same-host redirect loop on AA is not a network fault — it is
-                        # how an unsolved DDoS-Guard handshake presents: /search redirects
-                        # to /search&check=1, which redirects back, indefinitely. Raising
-                        # sends it down the retry path, which re-runs the whole loop on
-                        # every attempt (10 x 6 = ~60 requests to AA) and never once offers
-                        # the URL to the bypasser, so it can only ever fail. Hand it over
-                        # directly — not via `continue`, which would target this inner
-                        # redirect loop rather than the retry branch above.
-                        # allow_bypasser_fallback is honoured here for the same
-                        # reason the 403 path honours it: callers such as the
-                        # /dyn/md5/summary fetch behind the details modal pass False
-                        # precisely so a best-effort request fails fast instead of
-                        # holding the UI open for a minutes-long browser solve.
-                        if allow_bypasser_fallback and _is_cf_bypass_enabled() and not use_bypasser_now:
+                        # how DDoS-Guard presents a handshake that is unsolved, or whose
+                        # clearance cookie has gone stale: /search redirects to
+                        # /search&check=1, which redirects back, indefinitely. Hand it
+                        # straight to the bypasser rather than raising, which would send it
+                        # down the retry path to re-run the whole loop on every attempt
+                        # (10 x 6 = ~60 requests to AA) without ever offering the URL to the
+                        # bypasser. `continue` is no use here either — it would target this
+                        # inner redirect loop rather than the retry branch below.
+                        if _bypass_handoff_allowed():
                             logger.info(
-                                "redirect loop on %s; switching to bypasser", current_url
+                                "Redirect loop detected; switching to bypasser: %s", current_url
                             )
-                            use_bypasser_now = True
-                            return _run_bypasser(current_url)
-                        _raise_too_many_redirects(f"Too many redirects for {current_url}")
+                            return _redirect_loop_handoff(current_url)
+                        # No bypasser to hand it to. Every AA mirror shares the challenge,
+                        # so rotating only collects another loop — give up now instead of
+                        # raising and burning the same ~60 requests over the retry budget.
+                        logger.warning(
+                            "Redirect loop and no bypasser available, giving up: %s", current_url
+                        )
+                        return _result("", current_url)
                     current_url = redirect_url
                     continue
 
@@ -463,23 +490,14 @@ def html_get_page(
         except Exception as e:
             status = _get_status_code(e)
 
-            # DDoS-Guard also answers stale clearance cookies with an endless `?check=1`
-            # redirect. TooManyRedirects carries no status, so the 403 rescue below never
-            # fires and every retry re-sends the dead cookies.
-            if (
-                isinstance(e, requests.exceptions.TooManyRedirects)
-                and allow_bypasser_fallback
-                and _is_cf_bypass_enabled()
-                and not use_bypasser_now
-            ):
-                if not _is_using_external_bypasser():
-                    hostname = urlparse(current_url).hostname or ""
-                    _get_internal_bypasser().clear_cf_cookies(hostname)
+            # The same DDoS-Guard rescue for loops the manual AA follower above does not
+            # see: non-AA hosts keep allow_redirects=True, so `requests` follows the loop
+            # itself and raises, and an AA redirect missing its Location header raises
+            # too. TooManyRedirects carries no status, so the 403 rescue below never fires
+            # and every retry would re-send the dead cookies.
+            if isinstance(e, requests.exceptions.TooManyRedirects) and _bypass_handoff_allowed():
                 logger.info("Redirect loop detected; switching to bypasser: %s", current_url)
-                if status_callback:
-                    status_callback("resolving", "Bypassing protection...")
-                use_bypasser_now = True
-                continue
+                return _redirect_loop_handoff(current_url)
 
             # 403 = Cloudflare/DDoS-Guard protection
             if status == _HTTP_STATUS_FORBIDDEN:
@@ -505,12 +523,11 @@ def html_get_page(
                         )
                         continue
                     logger.info("403 detected; switching to bypasser: %s", current_url)
-                    # Invoke it here rather than setting the flag and continuing. The
-                    # branch that acts on the flag runs at the top of the *next* retry
-                    # attempt, so under the supported MAX_RETRY=1 there is no next
-                    # attempt and the bypasser was never reached — a 403 simply ended
-                    # the search. Same reasoning as the redirect-loop handoff below.
-                    use_bypasser_now = True
+                    # Invoke it here rather than setting use_bypasser_now and continuing.
+                    # The branch that acts on that flag runs at the top of the *next* retry
+                    # attempt, so under the supported MAX_RETRY=1 there is no next attempt
+                    # and the bypasser was never reached — a 403 simply ended the search.
+                    # Same reasoning as the redirect-loop handoffs.
                     return _run_bypasser(current_url)
                 logger.warning("403 error, giving up: %s", current_url)
                 return _result("", current_url)

tests/download/test_http_bypasser_fallbacks.py

@@ -283,3 +283,96 @@ def test_get_bypassed_page_uses_external_bypasser_when_enabled(monkeypatch):
 
     assert http.get_bypassed_page("https://example.com", selector, cancel_flag) == "EXT"
     assert calls == [("https://example.com", selector, cancel_flag)]
+
+
+def test_redirect_loop_gives_up_immediately_when_bypasser_not_allowed(monkeypatch):
+    """A loop the bypasser may not rescue must fail fast, not burn the retry budget.
+
+    Guards the regression where the unrescued loop raised TooManyRedirects into the
+    retry path: that error is not retryable and carries no status, so every attempt
+    re-ran the full 6-redirect loop for ~60 requests to AA before giving up.
+    """
+    import shelfmark.download.http as http
+
+    monkeypatch.setattr(http, "_is_cf_bypass_enabled", lambda: True)
+    monkeypatch.setattr(http, "_apply_cf_bypass", lambda _url, _headers: {})
+    monkeypatch.setattr(http, "get_proxies", lambda _url: {})
+    monkeypatch.setattr(http, "get_ssl_verify", lambda _url: True)
+    monkeypatch.setattr(http.network, "should_rotate_dns_for_url", lambda _url: True)
+    monkeypatch.setattr(http.network, "is_aa_auto_mode", lambda: True)
+    monkeypatch.setattr(http.time, "sleep", lambda _seconds: None)
+
+    requested: list[str] = []
+
+    def check_redirect(url: str, **_kwargs):
+        requested.append(url)
+        response = _FakeResponse(302, url=url)
+        response.is_redirect = True
+        response.headers = {"Location": f"{url}&check=1"}
+        return response
+
+    def unreachable_bypasser(*_args, **_kwargs):
+        msg = "bypasser must not run when allow_bypasser_fallback is False"
+        raise AssertionError(msg)
+
+    monkeypatch.setattr(http.requests, "get", check_redirect)
+    monkeypatch.setattr(http, "get_bypassed_page", unreachable_bypasser)
+
+    html = http.html_get_page(
+        "https://annas-archive.gl/dyn/md5/summary/abc",
+        retry=10,
+        allow_bypasser_fallback=False,
+        success_delay=0,
+    )
+
+    assert html == ""
+    # One pass through the redirect cap, not one pass per retry attempt.
+    assert len(requested) == http._MAX_REDIRECTS + 1
+
+
+def test_requests_raised_redirect_loop_still_reaches_bypasser(monkeypatch):
+    """Non-AA hosts keep allow_redirects=True, so `requests` raises the loop itself.
+
+    The AA follower above never sees that one, so the rescue in the exception path has
+    to stay — and it must purge the stale cookie exactly like the inline AA handoff.
+    """
+    import shelfmark.download.http as http
+
+    cleared: list[str] = []
+
+    class _FakeInternalBypasser:
+        @staticmethod
+        def clear_cf_cookies(domain: str) -> None:
+            cleared.append(domain)
+
+    monkeypatch.setattr(http, "_is_cf_bypass_enabled", lambda: True)
+    monkeypatch.setattr(http, "_is_using_external_bypasser", lambda: False)
+    monkeypatch.setattr(http, "_get_internal_bypasser", lambda: _FakeInternalBypasser)
+    monkeypatch.setattr(http, "_bypass_grace_seconds", lambda: 100.0)
+    monkeypatch.setattr(http, "_apply_cf_bypass", lambda _url, _headers: {})
+    monkeypatch.setattr(http, "get_proxies", lambda _url: {})
+    monkeypatch.setattr(http, "get_ssl_verify", lambda _url: True)
+    monkeypatch.setattr(http.network, "should_rotate_dns_for_url", lambda _url: False)
+    monkeypatch.setattr(http.time, "sleep", lambda _seconds: None)
+
+    def looping(_url: str, **_kwargs):
+        raise requests.exceptions.TooManyRedirects("too many redirects")
+
+    bypassed: list[str] = []
+    monkeypatch.setattr(http.requests, "get", looping)
+    monkeypatch.setattr(
+        http,
+        "get_bypassed_page",
+        lambda url, *_a, **_k: bypassed.append(url) or "<html>ok</html>",
+    )
+
+    html = http.html_get_page(
+        "https://z-lib.fm/s/dune",
+        retry=1,
+        allow_bypasser_fallback=True,
+        success_delay=0,
+    )
+
+    assert html == "<html>ok</html>"
+    assert cleared == ["z-lib.fm"]
+    assert bypassed == ["https://z-lib.fm/s/dune"]