fix(attachment): download through the track downloader with its retries

A failed attachment now gets the same retries and resume as a track before it stops the run; same-named attachments no longer share a temp file.

docs/dev/service-architecture.md

@@ -313,7 +313,9 @@ all documented for users in [Subtitles](../guide/subtitles.md).
 - **`Chapters`**: a sorted collection that auto-inserts a `00:00:00.000` chapter and
   reads/writes OGM Simple format.
 - **`Attachment`**: a font or image attached to the mux (for example, fonts an ASS subtitle
-  references), built from a local `path` or a `url`.
+  references), built from a local `path` or a `url`. A `url` attachment downloads through
+  the same downloader as a track, with the same retries. If it still fails, `dl` stops the
+  run with a one-line error.
 
 ### Selection, sorting, and muxing
 

tests/tracks/test_attachment_download.py

@@ -2,24 +2,73 @@
 
 from __future__ import annotations
 
+import importlib
+import threading
+from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer
+
+import pytest
 import requests
 
 import unshackle.core.tracks.track as track_module
+from unshackle.commands.dl import dl
 from unshackle.core.config import config
 from unshackle.core.tracks.attachment import Attachment
 
-URL = "https://cdn.example.com/images/thumb.jpg"
+downloader = importlib.import_module("unshackle.core.downloaders.requests")
+BODY = bytes(range(256)) * 64
 
 
-class FakeResponse:
-    def raise_for_status(self) -> None:
+class Handler(BaseHTTPRequestHandler):
+    """Serves BODY (honoring a Range start), or the next scripted status or "cut" first."""
+
+    def do_GET(self) -> None:
+        self.server.hits += 1  # type: ignore[attr-defined]
+        script = self.server.script  # type: ignore[attr-defined]
+        action = script.pop(0) if script else "ok"
+        if isinstance(action, int):
+            self.send_response(action)
+            self.send_header("Content-Length", "0")
+            self.end_headers()
+            return
+        rng = self.headers.get("Range")
+        start = int(rng.removeprefix("bytes=").split("-")[0]) if rng else 0
+        body = BODY[start:]
+        self.send_response(206 if rng else 200)
+        if rng:
+            self.send_header("Content-Range", f"bytes {start}-{len(BODY) - 1}/{len(BODY)}")
+        self.send_header("Content-Length", str(len(body)))
+        self.end_headers()
+        self.wfile.write(body[: len(body) // 2] if action == "cut" else body)
+
+    def log_message(self, *args: object) -> None:
         pass
 
-    def iter_content(self, chunk_size: int):
-        yield b"jpegdata"
+
+@pytest.fixture
+def server():
+    srv = ThreadingHTTPServer(("127.0.0.1", 0), Handler)
+    srv.hits = 0  # type: ignore[attr-defined]
+    srv.script = []  # type: ignore[attr-defined]
+    thread = threading.Thread(target=srv.serve_forever, daemon=True)
+    thread.start()
+    yield srv
+    srv.shutdown()
+    thread.join(timeout=5)
 
 
-class FakeSession(requests.Session):
+@pytest.fixture(autouse=True)
+def temp_and_no_wait(tmp_path, monkeypatch) -> None:
+    monkeypatch.setattr(config.directories, "temp", tmp_path)
+    monkeypatch.setattr(downloader, "retry_sleep", lambda exc, attempts: 0)
+
+
+def url(srv, path: str = "/images/thumb.jpg") -> str:
+    return f"http://127.0.0.1:{srv.server_address[1]}{path}"
+
+
+class Recorder(requests.Session):
+    __attrs__ = [*requests.Session.__attrs__, "got"]
+
     def __init__(self, proxies: dict | None = None) -> None:
         super().__init__()
         self.proxies = proxies or {}
@@ -27,66 +76,89 @@ class FakeSession(requests.Session):
 
     def get(self, url, **kwargs):  # type: ignore[override]
         self.got.append(url)
-        return FakeResponse()
+        return super().get(url, **kwargs)
 
 
 def test_url_attachment_defers_download() -> None:
-    session = FakeSession()
-    attachment = Attachment.from_url(URL, name="thumbnail", session=session)
+    session = Recorder()
+    attachment = Attachment.from_url("https://cdn.example.com/images/thumb.jpg", name="thumbnail", session=session)
     assert attachment.path is None
     assert not session.got
     assert attachment.mime_type == "image/jpeg"
 
 
-def test_download_without_flag_uses_proxied_session(tmp_path, monkeypatch) -> None:
-    monkeypatch.setattr(config.directories, "temp", tmp_path)
-    session = FakeSession(proxies={"https": "http://proxy:8080"})
-    attachment = Attachment.from_url(URL, name="thumbnail", session=session)
+def test_download_without_flag_uses_proxied_session(server) -> None:
+    session = Recorder(proxies={"https": "http://proxy:8080"})
+    attachment = Attachment.from_url(url(server), name="thumbnail", session=session)
     attachment.download(session)
-    assert session.got == [URL]
-    assert attachment.path is not None and attachment.path.read_bytes() == b"jpegdata"
+    assert session.got == [url(server)]
+    assert attachment.path is not None and attachment.path.read_bytes() == BODY
 
 
-def test_download_with_flag_bypasses_proxy(tmp_path, monkeypatch) -> None:
-    monkeypatch.setattr(config.directories, "temp", tmp_path)
-    direct = FakeSession()
+def test_download_with_flag_bypasses_proxy(server, monkeypatch) -> None:
+    direct = Recorder()
     monkeypatch.setattr(track_module, "direct_session", lambda session: direct)
-    proxied = FakeSession(proxies={"https": "http://proxy:8080"})
-    attachment = Attachment.from_url(URL, name="thumbnail", session=proxied)
+    proxied = Recorder(proxies={"https": "http://proxy:8080"})
+    attachment = Attachment.from_url(url(server), name="thumbnail", session=proxied)
     attachment.download(proxied, no_proxy_download=True)
     assert not proxied.got
-    assert direct.got == [URL]
-    assert attachment.path is not None and attachment.path.read_bytes() == b"jpegdata"
+    assert direct.got == [url(server)]
+    assert attachment.path is not None and attachment.path.read_bytes() == BODY
 
 
-def test_download_with_proxy_download_uses_only_that_proxy(tmp_path, monkeypatch) -> None:
-    monkeypatch.setattr(config.directories, "temp", tmp_path)
+def test_download_with_proxy_download_uses_only_that_proxy(server, monkeypatch) -> None:
     seen: dict = {}
 
     def fake_direct(session, proxy=None):
         seen["proxy"] = proxy
-        return FakeSession(proxies={"all": proxy})
+        return Recorder()
 
     monkeypatch.setattr(track_module, "direct_session", fake_direct)
-    proxied = FakeSession(proxies={"https": "http://manifest-proxy:1"})
-    attachment = Attachment.from_url(URL, name="thumbnail", session=proxied)
+    proxied = Recorder(proxies={"https": "http://manifest-proxy:1"})
+    attachment = Attachment.from_url(url(server), name="thumbnail", session=proxied)
     attachment.download(proxied, proxy_download="http://dl-proxy:2")
     assert not proxied.got
     assert seen["proxy"] == "http://dl-proxy:2"
     assert proxied.proxies == {"https": "http://manifest-proxy:1"}
 
 
-def test_no_proxy_download_wins_over_proxy_download(tmp_path, monkeypatch) -> None:
-    monkeypatch.setattr(config.directories, "temp", tmp_path)
+def test_no_proxy_download_wins_over_proxy_download(server, monkeypatch) -> None:
     seen: dict = {}
-    monkeypatch.setattr(
-        track_module, "direct_session", lambda s, proxy=None: seen.setdefault("p", proxy) or FakeSession()
-    )
-    plain = FakeSession()
-    Attachment.from_url(URL, name="t", session=plain).download(
+    monkeypatch.setattr(track_module, "direct_session", lambda s, proxy=None: seen.setdefault("p", proxy) or Recorder())
+    plain = Recorder()
+    Attachment.from_url(url(server), name="t", session=plain).download(
         plain, no_proxy_download=True, proxy_download="http://dl-proxy:2"
     )
-    assert "p" not in seen and plain.got == [URL]
+    assert "p" not in seen and plain.got == [url(server)]
+
+
+def test_body_cut_is_resumed(server) -> None:
+    server.script = ["cut"]
+    attachment = Attachment.from_url(url(server), name="thumbnail")
+    attachment.download(requests.Session())
+    assert server.hits == 2
+    assert attachment.path is not None and attachment.path.read_bytes() == BODY
+
+
+def test_spent_retries_fail_the_run_with_one_line(server) -> None:
+    server.script = [503] * downloader.MAX_ATTEMPTS
+    attachment = Attachment.from_url(url(server), name="thumbnail")
+    with pytest.raises(requests.HTTPError) as raised:
+        attachment.download(requests.Session())
+    assert server.hits == downloader.MAX_ATTEMPTS
+    assert attachment.path is None
+    _, line = dl.failure_lines(raised.value)
+    assert f"gave up after {downloader.MAX_ATTEMPTS} failed attempts" in line
+    assert "attachment thumbnail" in line
+
+
+def test_same_named_attachments_get_separate_files(server) -> None:
+    first = Attachment.from_url(url(server, "/a/image.jpg"))
+    second = Attachment.from_url(url(server, "/b/image.jpg"))
+    first.download(requests.Session())
+    second.download(requests.Session())
+    assert first.path is not None and second.path is not None
+    assert first.path != second.path
 
 
 def test_direct_session_routes_through_the_given_proxy() -> None:

unshackle/core/tracks/attachment.py

@@ -101,10 +101,11 @@ class Attachment:
         no_proxy_download: bool = False,
         proxy_download: Optional[str] = None,
     ) -> None:
-        """Download a URL-backed attachment to the temp directory."""
+        """Download a URL-backed attachment to the temp directory with the same retries as a track."""
         if self.path is not None or not self.url or DOWNLOAD_LICENCE_ONLY.is_set():
             return
 
+        from unshackle.core.downloaders.requests import download
         from unshackle.core.tracks.track import direct_session
 
         session = session or self.session or run_proxy.fallback_session()
@@ -114,18 +115,17 @@ class Attachment:
         elif proxy_download:
             session = direct_session(session, proxy_download)
 
-        download_path = config.directories.temp / (self.file_name or "attachment")
+        download_path = (
+            config.directories.temp / f"{crc32(self.url.encode('utf8')):08x}_{self.file_name or 'attachment'}"
+        )
         try:
-            response = session.get(self.url, stream=True)
-            response.raise_for_status()
-            download_path.parent.mkdir(parents=True, exist_ok=True)
-            with open(download_path, "wb") as f:
-                for chunk in response.iter_content(chunk_size=8192):
-                    f.write(chunk)
+            for _ in download(self.url, download_path, session=session):
+                pass
         except Exception as e:
-            raise ValueError(f"Failed to download attachment from URL: {e}")
-
-        self.path = download_path
+            e.add_note(f"attachment {self.name}")
+            raise
+        if download_path.exists():
+            self.path = download_path
 
     def __repr__(self) -> str:
         return "{name}({items})".format(