Skip to content

fix(resources): fetch each cover once instead of once per size - #4143

Merged
gantoine merged 2 commits into
rommapp:masterfrom
Spinnich:fix/cover-downloaded-twice
Aug 8, 2026
Merged

fix(resources): fetch each cover once instead of once per size#4143
gantoine merged 2 commits into
rommapp:masterfrom
Spinnich:fix/cover-downloaded-twice

Conversation

@Spinnich

@Spinnich Spinnich commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Description
Explain the changes or enhancements you are proposing with this pull request.

get_cover() called _store_cover() once for CoverSize.SMALL and once for CoverSize.BIG with the same URL, and each call performed a complete fetch. The small cover has no distinct source: it is the large image downloaded in full and then resized in place, so the second request only re-retrieved bytes already on disk.

Measured against the real code path with a local HTTP server counting inbound requests: 2 GETs per cover, to a byte-identical URL. Covers in this repo's romm_mock/resources average 556 KiB, so a 5,000-ROM library re-scraped with overwrite=True throws away roughly 2.6 GiB of downloads. On ScreenScraper it costs quota rather than just bandwidth, since media is served from the same authenticated api2 endpoints as metadata lookups and routes through the same limiters (media_download_slot), so every cover spends two requests from the account's allowance instead of one. Local sources (file:// / launchbox-file://) did two full file copies off the user's disk per cover.

_store_cover() no longer takes a size. It fetches once into big.png and derives small.png from it, the way store_artwork() already did. The local work (chroma-key check, resize, WebP conversion) still runs after the provider's request slot has been handed back. get_cover()'s signature and return value are unchanged, so all four call sites (scan socket, two collection endpoints, the ROM update endpoint) are untouched. No response schema change, so no regenerated types, no i18n, no frontend code.

A missing small cover no longer triggers a fetch at all. get_cover() downloads only when overwriting or when the large cover is absent; when the large one is already on disk and only the small is missing, _derive_small_cover() rebuilds it locally for zero requests. That keeps a good large cover out of the blast radius of a download that might fail, and drops the torn-pair case from one request to none.

What a reviewer should look at hardest, in order:

  1. The split between fetching and deriving in get_cover(). The condition is now "URL present and (overwriting or no large cover)", with the derive path as the elif. The important property is that _store_cover() only ever clobbers big.png when the caller asked for an overwrite or there was nothing there, so no failure path can cost a large cover that was already good. _derive_small_cover() resolves the source through _get_cover_path, so an uploaded big.jpg or a converted big.webp produces a matching small.jpg / small.webp rather than assuming .png.
  2. _derive_small_cover() deliberately does not discard an unreadable large cover, where _store_cover() does. The distinction: _store_cover() just wrote those bytes itself and knows they're bad, while the derive path found them on disk and can't rule out a format PIL lacks a plugin for. It logs and drops only the partial small.
  3. A failed overwrite=True refresh now leaves the old small.png behind (big.png, being truncated, is still discarded), where the old code destroyed both. get_cover then returns (small, None), and v2 resolves path_cover_large ?? path_cover_small, so the ROM detail page shows the upscaled thumbnail instead of the dark placeholder until the next scan refetches. Deliberate: a network blip no longer wipes working artwork. Trade is visible, so flagging it.
  4. UnidentifiedImageError now discards both files. Undecodable bytes satisfy cover_exists(), so leaving them on disk meant no later scan ever retried and the UI rendered a broken image permanently. Same reasoning as _discard_partial_file's docstring. An OSError in the resize/convert tail is now caught too; it previously escaped into the scan. Relatedly, a non-200 on the small fetch used to reach Image.open() on a file that was never written, raising an uncaught FileNotFoundError out into the scan; the downloaded guard removes that.
  5. allow_link=False on the local-file copy is still load-bearing, for a new reason: big.png is no longer resized in place, but an overwriting scan opens the destination "wb" via write_file_streamed, which would truncate the user's library file through a hardlink. Covered by an st_nlink == 1 assertion.

Fixes #4102

AI assistance disclosure: this PR was written primarily by Claude Code (Claude Opus 5), under my direction and review. AI generated the handler change, the tests, this description, and the commit message; I specified the approach, reviewed every line, and ran the verification below myself. PR responses will be written by me, with AI assistance disclosed if that changes.

Checklist
Please check all that apply.

  • I've tested the changes locally
  • I've updated relevant comments
  • I've assigned reviewers for this PR
  • I've added unit tests that cover the changes
Verification

Backend, from backend/:

uv run pytest tests/handler/filesystem/test_resources_handler.py -q
  baseline (before any edit):  104 passed
  tests written, no impl yet:   11 failed, 104 passed   <- failing for the right reason
  final:                       118 passed

uv run pytest -q     ->  2760 passed, 2 skipped in 814.96s (exit 0)
trunk fmt / trunk check <both changed files>  ->  no issues

Two tests asserted the old behaviour (assert mock_store.call_count == 2) and were deleted; mocking _store_cover is exactly what let this ship. The 16 replacements in TestCoverSingleFetch use a counting httpx client serving real PNG bytes, so the resize actually runs and the request count is the assertion: one fetch when neither cover exists, one when overwriting, one when only the small exists, zero when both exist, zero without a URL, zero when only the large exists (rebuilt from disk instead), one copy_file on the local branch, plus the chroma-key / dropped-connection / undecodable-bytes / unreadable-large-cover / non-PNG-extension / WebP / resize-ratio / collection-entity cases. One test pins the failure mode directly: with a large cover on disk and a provider that drops the connection, the file must come out byte-identical.

End-to-end against the real handler (not mocks), driving fs_resource_handler.get_cover() at a local server counting inbound GETs: 1 GET per cover with overwrite=True, 0 when covers are present, big.png 900x1200 and small.png 180x240. Serving ScreenScraper's green placeholder to the same path returns (None, None) and clears a stale small left by an earlier pass.

Browser (v2, light and dark, kiosk stack): platform gallery renders both covers with no failed /cover/ requests, ROM detail renders the large cover at 1000x1424. For the derive path specifically, small.png was deleted with big.png left in place and the real get_cover() run against a request-counting local server: 0 GETs, big.png byte-identical by SHA, and both small covers rebuilt at the same dimensions as the originals (200x284 and 198x272, covering both resize_cover_to_small ratio branches). The rebuilt files decode in-page at those sizes and render as clean downscales of the box art. Custom uploaded artwork goes through store_artwork(), which this diff doesn't touch.

npm run test:e2e → 1 failed, 19 passed. The failure (e2e/game-media-files.spec.ts:44) is pre-existing: stashing both changed files and re-running against master's resources_handler.py reproduces it identically.

get_cover() called _store_cover() once for CoverSize.SMALL and once for
CoverSize.BIG with the same URL, and each call performed a complete fetch.
The small cover has no distinct source: it is the large image downloaded in
full and then resized in place, so the second request only re-retrieved bytes
already on disk. Every cover cost twice the bandwidth, doubled the load on
every provider, and on ScreenScraper spent a second request from the
account's quota, since media is served from the same authenticated api2
endpoints as metadata lookups.

_store_cover() now takes no size: it fetches once into big.png and derives
small.png from it, the way store_artwork() already did.

A missing small cover no longer triggers a fetch at all. get_cover()
downloads only when overwriting or when the large cover is absent, and
otherwise rebuilds the small one from the large one already on disk via
_derive_small_cover(). That drops the half-written pair case from one request
to none, and keeps a good large cover out of the blast radius of a download
that might fail. The derive path resolves its source through _get_cover_path,
so an uploaded big.jpg or a converted big.webp yields a matching small file
rather than assuming a .png extension.

Failure paths now clean up both destinations. Undecodable bytes are
discarded rather than left on disk, where they satisfy cover_exists() and
stop any later scan from refetching a working cover, and a discarded
chroma-key placeholder also clears a small cover left by an earlier scan.
_derive_small_cover() is the exception: it did not write those bytes, so a
large cover it cannot decode is left in place and only the partial small one
is dropped.

Fixes rommapp#4102

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@greptile-apps

greptile-apps Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR consolidates cover retrieval into one fetch of the large image and derives the small image locally, including recovery of a missing small cover without network access.

  • Updates local and HTTP cover storage to write the large cover once and resize it into the small cover.
  • Adds cleanup for invalid or partially processed downloads while preserving unreadable pre-existing large covers during local derivation.
  • Adds focused tests for request counts, local copies, conversion, failure handling, image dimensions, and collection covers.

Confidence Score: 5/5

The PR appears safe to merge with no actionable changed-code defect identified.

The new single-fetch and local-derivation paths preserve existing cover lookup behavior and are covered across normal, overwrite, local-file, conversion, and failure scenarios.

Important Files Changed

Filename Overview
backend/handler/filesystem/resources_handler.py Consolidates duplicate cover downloads into one large-cover fetch followed by local small-cover derivation, with expanded cleanup and recovery handling.
backend/tests/handler/filesystem/test_resources_handler.py Replaces implementation-mocking assertions with end-to-end filesystem and HTTP-client tests covering single-fetch behavior, derivation, conversion, and failures.

Reviews (1): Last reviewed commit: "fix(resources): fetch each cover once in..." | Re-trigger Greptile

@Spinnich
Spinnich requested a review from gantoine August 6, 2026 22:41
@gantoine
gantoine merged commit 392cc9b into rommapp:master Aug 8, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Every cover image is downloaded twice, doubling scan traffic and ScreenScraper quota use

2 participants