diff --git a/docs/dev/service-architecture.md b/docs/dev/service-architecture.md index ef207c33..e1591b13 100644 --- a/docs/dev/service-architecture.md +++ b/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 diff --git a/tests/tracks/test_attachment_download.py b/tests/tracks/test_attachment_download.py index 2e9cd5fe..4b59165c 100644 --- a/tests/tracks/test_attachment_download.py +++ b/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: diff --git a/unshackle/core/tracks/attachment.py b/unshackle/core/tracks/attachment.py index 091fce1c..158239fb 100644 --- a/unshackle/core/tracks/attachment.py +++ b/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(