Image cache: reject truncated downloads instead of caching broken covers (#750)

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

@@ -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:

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,