From c3f7cf795a444a0db71d01e4c4b6cad9e0133b26 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Sat, 30 May 2026 17:45:01 -0700 Subject: [PATCH] Image cache: reject truncated downloads instead of caching broken covers (#750) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reporter: album covers render as a top strip then solid grey ('break' on import) — and it happens regardless of the album-art toggles. That ruled out the import embed/cover.jpg paths (all toggle-gated) and pointed at the DISPLAY cache, which every cover view goes through. Root cause: ImageCache._fetch_and_store streamed the body to a tmp file and committed it as status='ok' with only a 'total <= 0' (empty) guard. A dropped/short connection makes requests' iter_content END EARLY WITHOUT raising, so a PARTIAL image was cached permanently and served forever as a half-decoded cover. The high-res art change in 2.6.4 (bigger images) makes a mid-stream cutoff more likely, especially on the reporter's LXC. Fix: capture the declared Content-Length and, after streaming, reject when fewer bytes arrived (unlink the tmp file, raise ImageCacheError) so nothing broken is cached and the next request retries fresh. When the server omits Content-Length (chunked), we can't detect truncation, so we don't reject — behavior unchanged there. Tests (tests/test_image_cache.py): truncated download raises + caches nothing + a later good fetch still works (differential-verified it's silently cached without the guard); positive control (declared==actual) caches normally; no-Content-Length still caches. 6 image-cache tests pass. Strong-candidate fix: it's a real defect that produces exactly this symptom, but I can't reproduce the reporter's LXC network to prove it's THE cause. --- core/image_cache.py | 25 +++++++++++-- tests/test_image_cache.py | 75 +++++++++++++++++++++++++++++++++++++-- 2 files changed, 95 insertions(+), 5 deletions(-) diff --git a/core/image_cache.py b/core/image_cache.py index 040c1126..e1087e34 100644 --- a/core/image_cache.py +++ b/core/image_cache.py @@ -172,11 +172,14 @@ class ImageCache: raise ImageCacheError(f"Upstream response is not an image: {mime_type}") declared_size = response.headers.get("Content-Length") + expected_bytes = None try: - if declared_size and int(declared_size) > self.max_download_bytes: - raise ImageCacheError("Image exceeds configured size limit") + if declared_size: + expected_bytes = int(declared_size) + if expected_bytes > self.max_download_bytes: + raise ImageCacheError("Image exceeds configured size limit") except ValueError: - pass + expected_bytes = None ext = mimetypes.guess_extension(mime_type) or ".img" if ext == ".jpe": @@ -205,6 +208,22 @@ class ImageCache: if total <= 0: raise ImageCacheError("Image response was empty") + # Truncation guard (#750): a dropped/short connection makes + # iter_content end early WITHOUT raising, so a partial image would + # otherwise be committed as status='ok' and cached permanently — + # rendering as a half-decoded cover (top strip, rest grey). If the + # server declared a Content-Length and we got fewer bytes, treat it + # as a failed download: discard the tmp file and don't cache it, so + # the next request retries fresh instead of serving a broken file. + if expected_bytes is not None and total < expected_bytes: + try: + tmp_path.unlink(missing_ok=True) + except Exception as cleanup_exc: + logger.debug("image_cache tmp cleanup failed: %s", cleanup_exc) + raise ImageCacheError( + f"Truncated image download: got {total} of {expected_bytes} bytes" + ) + os.replace(tmp_path, path) expires_at = now + self.ttl_seconds with self._db_lock: diff --git a/tests/test_image_cache.py b/tests/test_image_cache.py index f4556757..f4d21bde 100644 --- a/tests/test_image_cache.py +++ b/tests/test_image_cache.py @@ -4,12 +4,16 @@ from core.image_cache import ImageCache class FakeResponse: - def __init__(self, body: bytes, *, status_code: int = 200, content_type: str = "image/jpeg"): + def __init__(self, body: bytes, *, status_code: int = 200, content_type: str = "image/jpeg", + declared_length: int | None = None): self.body = body self.status_code = status_code + # declared_length lets a test simulate a truncated download: the server + # promises N bytes (Content-Length) but the body delivers fewer. + length = len(body) if declared_length is None else declared_length self.headers = { "Content-Type": content_type, - "Content-Length": str(len(body)), + "Content-Length": str(length), } self.closed = False @@ -51,6 +55,73 @@ def test_get_url_fetches_once_then_serves_cached_file(tmp_path): assert len(calls) == 1 +def test_truncated_download_is_rejected_not_cached(tmp_path): + """#750: a short/dropped download (body shorter than the declared + Content-Length) must NOT be committed as a good cache entry — otherwise the + half-decoded cover (top strip, rest grey) is served forever. It should raise + and leave nothing cached, so the next request retries fresh.""" + calls = [] + + def fetcher(url, **kwargs): + calls.append(url) + # Server promises 5000 bytes but only delivers 800 (connection dropped). + return FakeResponse(b"x" * 800, declared_length=5000) + + cache = ImageCache(tmp_path, fetcher=fetcher) + url = "https://images.example.test/big-cover.jpg" + + raised = False + try: + cache.get_url(url) + except Exception as exc: + raised = True + assert "Truncated" in str(exc) or "truncated" in str(exc) + assert raised, "Expected a truncated download to raise" + + # Nothing partial left on disk for this key. + import glob + key = ImageCache.key_for_url(url) + leftover = glob.glob(str(tmp_path / "**" / f"{key}*"), recursive=True) + leftover = [p for p in leftover if not p.endswith(".sqlite3")] + assert leftover == [], f"truncated file should not be cached, found: {leftover}" + + # A subsequent SUCCESSFUL fetch works (not poisoned by the failed attempt). + cache2 = ImageCache( + tmp_path, + fetcher=lambda u, **k: FakeResponse(b"complete-jpeg-bytes"), + ) + result = cache2.get_url(url) + assert result.path.read_bytes() == b"complete-jpeg-bytes" + + +def test_complete_download_with_content_length_succeeds(tmp_path): + """Positive control: a full download whose body matches Content-Length is + cached normally (the truncation guard doesn't false-positive).""" + cache = ImageCache( + tmp_path, + fetcher=lambda u, **k: FakeResponse(b"a-real-cover"), # declared==actual + ) + result = cache.get_url("https://images.example.test/ok-cover.jpg") + assert result.path.read_bytes() == b"a-real-cover" + assert result.status == "miss" + + +def test_no_content_length_still_caches(tmp_path): + """Some CDNs omit Content-Length (chunked transfer). With no declared size + we can't detect truncation, so we must NOT reject — cache as before.""" + class NoLengthResponse(FakeResponse): + def __init__(self, body): + super().__init__(body) + del self.headers["Content-Length"] + + cache = ImageCache( + tmp_path, + fetcher=lambda u, **k: NoLengthResponse(b"chunked-cover-bytes"), + ) + result = cache.get_url("https://images.example.test/chunked.jpg") + assert result.path.read_bytes() == b"chunked-cover-bytes" + + def test_get_url_rejects_non_image_responses(tmp_path): cache = ImageCache( tmp_path,