fix(scan): let update metadata scans refresh artwork, and record uploads explicitly

[sdornan]

Description

Part of #4002, and deliberately only part of it. Two related fixes to the
artwork path on an update metadata scan, smaller first. No provider-specific
machinery, no media hashing, no response schema touched, so no OpenAPI
regeneration.

The short version: an update scan structurally cannot replace provider-written
artwork, and the one signal protecting hand-uploaded covers can be destroyed by
an ordinary scan.

The atomic-write half that was originally here has shipped separately as #4230,
which is merged. This branch is rebased on top of it, so what remains is the
provenance work and nothing else.

Scope, on an update metadata scan:

Case	Before	After
scan.priority.cover / scan.priority.artwork changed	Never applied	Applied
A different source now wins the cover	Old source kept	New source wins
Provider changed both its art and its url (IGDB, MobyGames, LaunchBox)	Not refreshed	Refreshed
Hand-uploaded cover, after a scan ran with resources unreadable	Replaced by provider art	Kept
Hand-edited name / summary	Kept	Kept
Uploaded manual	Kept	Kept
ScreenScraper changed the art behind the same fixed url	Not refreshed	Not refreshed

That last row is the case #4028 was about and this PR does not address it.
ScreenScraper serves media from fixed endpoints, so the url is byte-identical
after new art is uploaded and the url comparison cannot see the change. It stays
reachable the way it already is, by clearing the resource files and rescanning,
which works precisely because the url is stable. Making that cheap needs the
per-media hashes ScreenScraper returns, and that is worth proposing separately
once the pipeline underneath actually refreshes.

---

1. Update scans can replace provider-written artwork urls

scan_handler.py pinned url_cover to the stored value whenever a cover file
existed, and pinned url_screenshots whenever any were stored:

"url_cover": (
    rom.url_cover
    if rom.path_cover_s
    else rom_attrs.get("url_cover") or None
),

The download step immediately below gates on whether the url changed
(endpoints/sockets/scan.py:604 on master):

overwrite=_added_rom.url_cover != rom.url_cover,

Because the block above forces those two equal, that comparison is
structurally always false. The freshly resolved url is discarded before the
gate ever sees it, so no update scan can move a cover regardless of which
source now wins.

The comment on that block says "Don't overwrite existing manually uploaded
cover image", but path_cover_s is set for any scraped cover too, so it
protects everything rather than just uploads.

This is provider-agnostic: it fixes IGDB, MobyGames, LaunchBox and the rest
just as much as ScreenScraper. It also fixes something easier to hit than stale
art: changing scan.priority.cover or scan.priority.artwork never applies
to already-scanned ROMs. Concretely, enabling box2d_back / box2d_side for
the interactive 3D box and setting ss first for covers, so the front face
matches the SS back and spine, currently appears to do nothing.

Screenshots have no upload path at all, so every stored url is
provider-written and the fresh set wins. Recording them was itself lossy:
get_rom_screenshots recorded a path per url whether or not the download
landed, and decided its early return from that recorded list, so a set left
short by a failed run was frozen in place by an unchanged url set. It now
records only what reached the disk and counts files on disk to decide, so a
short set is retried. That is reachable on master through new and complete
scans; this PR adds update scans as a third trigger, which is why it is fixed
here.

2. Hand-supplied artwork is recorded explicitly

The first fix needs to tell an uploaded cover from a scraped one. There is a
signal today, since uploading artwork stores the file and clears the url
(endpoints/roms/__init__.py:1862 on master), so a stored path with no url means the
user supplied it.

That signal cannot survive, and this is a live bug on master rather than
something the first fix introduces. path_cover_s is doing two incompatible
jobs. It is where the file is, which the scan reconciles against disk on every
run, and it is the provenance marker, which has to outlive the file. get_cover
returns no path when the cover is unreadable, and endpoints/sockets/scan.py:624 on master
writes that straight back. So:

1. A scan runs while the resources volume is unavailable.
2. Every cover reports missing, and path_cover_s is cleared for all of them.
3. Storage returns. The next scan reads those rows as having no cover at all,
   adopts the provider url, and downloads over the user's uploaded artwork.

A resources volume left out of a container recreate is enough to lose every
uploaded cover in a library, with no user-visible cause. Verified against both
master and this branch: identical behaviour, so it predates these changes.

locked_fields records provenance durably instead, independent of what is on
disk. Uploading artwork or a manual locks the field; removing the cover,
deleting or redownloading the manual, and naming an explicit source url all
release it again. A complete rescan clears the locks along with the resource
files it deletes, so a lock can never point at a file that is gone and block
its replacement.

This is the smallest honest version of the explicit locking direction from the
review on #4028, without the user-facing half. Nothing is exposed on the rom
response yet and there is no toggle, so it replaces a hidden inference with a
hidden fact and adds no new concept for users. The lock UI, and with it
name and summary, can build on the same column without another migration.

The migration backfill is the load-bearing part. Existing uploads are
recognisable only by the old inferred marker, and migration time is the last
point at which it can still be read. Without the backfill, the first scan after
upgrading would replace every uploaded cover in every library.

Releasing a lock

Only hand-supplied artwork is ever locked, so a lock is the one thing an update
scan could overwrite that no provider can give back. Releasing one is therefore
an explicit act rather than a scan option, and every route already exists in the
UI:

• Remove the cover, or name a different cover source in the edit or match
  dialog.
• Delete or redownload a manual, or name a different manual source.
• A complete rescan clears every lock in scope, as it already discards
  everything else.

Naming a source releases the lock only when the url actually changes, not
merely when one is sent. The client posts the stored urls on every save, and an
upload leaves url_manual populated with whatever was scraped before, so a
looser test would release a manual lock the first time the user edited anything
else on the rom. Saving a dialog without choosing a different source never
releases anything.

There is deliberately no scan-level "ignore locks" switch. It would be one
global action over unrecoverable user files with no preview, and it would make
a lock untrustworthy afterwards, since no locked cover could be assumed to still
be the user's. A bulk unlock action is the composable version of that and
belongs with the lock UI.

What this does not do

• name and summary stay pinned. Protecting a hand-edited title needs a lock
  the user can set, and marking a field on edit is the user_edited_fields
  approach that was rejected on fix(scan): refresh stale ScreenScraper data on update metadata scans #4028. They join once there is a UI.
• Manuals stay pinned and are not backfilled. An uploaded manual and a scraped
  one share a path and neither clears url_manual, so nothing distinguishes
  the existing ones and any guess would be wrong for half the rows. New uploads
  are marked from here on, so manuals can be unpinned later.

Follow-ups on #4002

#4002 stays open. This PR is the groundwork rather than the whole thing, and
each remaining piece is separable and can be judged on its own:

1. Lock UI. Expose locked_fields on the rom response, add a per-field
   toggle and a bulk unlock action. That is the user-facing half of the explicit
   locking direction from the fix(scan): refresh stale ScreenScraper data on update metadata scans #4028 review, and it needs no further migration.
2. name and summary unpinned, once (1) gives users a way to protect an
   edited title. This is the half of [Bug] ScreenScraper "Update metadata" scan only fills in missing data, it never refreshes stale data #4002 about text fields never refreshing.
3. Manuals unpinned, once enough libraries have marked their uploads for the
   distinction to be safe.
4. Cheap ScreenScraper refresh. The last row of the scope table. The
   per-media md5 in jeuInfos turns "is my copy stale" into a comparison
   instead of a redownload, and ScreenScraper's md5= request parameter may
   remove even the need to store hashes. Worth proposing on its own merits as an
   efficiency change, now that it is no longer carrying a correctness fix.

Happy to take these in whatever order suits, or to stop here if the groundwork
is all that is wanted.

Files modified

File	Change
backend/handler/scan_handler.py	Consult the lock instead of file existence, so provider-written covers can be replaced; carry locked_fields through the scan and clear it on a complete rescan.
backend/models/rom.py	locked_fields column and its read/with/without helpers.
backend/alembic/versions/0108_roms_locked_fields.py	Add the column and backfill existing uploads from the old inferred marker.
backend/handler/filesystem/resources_handler.py	Screenshots record only what reached the disk, and a short set is retried.
backend/endpoints/roms/__init__.py, .../manual.py	Take and release locks on upload, removal, deletion, redownload, and on naming a different source url.
backend/tests/**	Coverage for the artwork gate, the upload and text-field guardrails, the screenshot recording, and the lock lifecycle.

Testing notes

• Full backend suite against MariaDB on the rebased branch: 2952 passed,
  2 skipped. The 4 failures in my run are pre-existing and environmental,
  and reproduce identically on master: three TestPeriodicTask cases need a
  local Redis, and one Hypothesis case
  (test_valid_iso_dates_parse_to_timestamp) falsifies on an ambiguous
  DST-fold local time.
• black and ruff clean on every changed file.

New tests were each checked against master to confirm they fail for the right
reason rather than passing vacuously:

• test_update_scan_replaces_scraped_cover_url and
  test_update_scan_replaces_screenshot_urls fail on master.
• test_update_scan_keeps_locked_cover_with_no_stored_path is the regression
  test for the data loss in section 2.
• test_update_scan_keeps_uploaded_cover and
  test_update_scan_keeps_name_summary_and_manual pass both ways on purpose:
  they are guardrails proving the scope did not widen to uploads or text fields.
• test_failed_screenshot_is_not_recorded and
  test_short_screenshot_set_is_retried cover the screenshot recording fix.
• test_update_rom_artwork_locks_the_cover, test_remove_cover_releases_the_lock,
  test_saving_without_changing_urls_keeps_locks and
  test_naming_new_source_urls_releases_both_locks cover the lock lifecycle.

The backfill was verified against real databases, on both supported
dialects. A fresh test database has no rows in the old shape, so the suite
would never exercise it. A scratch database was migrated to 0107, populated
with rows in the pre-migration shape, then migrated to 0108, covering
uploaded, scraped, coverless, url-but-no-file, and uploaded-with-NULL-url
rows. All five land correctly on MariaDB and on PostgreSQL, the latter checked
separately because JSONB serialises differently from JSON.

One reported issue did not reproduce: a lock surviving unmatch_metadata does
not leave the rom coverless, because unmatching clears the stored paths but
never deletes the files, so get_cover re-derives them from disk.
test_cover_with_no_source_url_is_rederived_from_disk pins that, since it is
one remove_cover call away from becoming true.

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

AI assistance

This PR was written primarily by Claude Code (Opus 5), including the code, the
tests and this description. I reviewed the changes. The test suite and the
migration backfill checks described above were run locally against MariaDB and
PostgreSQL.

[greptile-apps]

Greptile Summary

This PR makes provider artwork refreshable during update scans, adds durable provenance locks for user-supplied resources, and publishes streamed downloads atomically.

• Adds and backfills the ROM locked_fields JSON column.
• Updates cover and manual routes to acquire or release resource locks.
• Uses newly resolved cover and screenshot URLs during update scans.
• Routes streamed resource downloads through temporary files and atomic replacement.
• Adds regression coverage for lock lifecycle, artwork refreshes, and interrupted writes.

Confidence Score: 3/5

The PR should not merge until metadata unmatching clears stale resource locks and failed screenshot refreshes remain retryable.

Durable locks can leave an unmatched ROM permanently coverless, and the newly enabled screenshot refresh path can persist missing files without retrying failed transfers; cancellation-related temporary-file leakage is additionally non-blocking.

Files Needing Attention: backend/handler/scan_handler.py, backend/endpoints/roms/init.py, backend/handler/filesystem/resources_handler.py, backend/handler/filesystem/base_handler.py

Important Files Changed

Filename	Overview
backend/handler/scan_handler.py	Refreshes provider artwork URLs and honors durable cover locks, but stale locks after metadata unmatching can permanently suppress cover restoration and failed screenshot refreshes can become non-retryable.
backend/handler/filesystem/base_handler.py	Converts streamed writes to atomic publication, though cooperative cancellation can bypass temporary-file cleanup.
backend/handler/filesystem/resources_handler.py	Adopts the atomic streaming context manager at all download sites, while screenshot paths are still recorded without confirming each download succeeded.
backend/alembic/versions/0108_roms_locked_fields.py	Adds and dialect-portably backfills locked_fields using the prior uploaded-cover marker.
backend/models/rom.py	Adds the nullable JSON lock column and None-safe helpers for adding, removing, and querying locks.
backend/endpoints/roms/init.py	Correctly updates cover locks for upload, removal, and explicit source selection, but the existing metadata-unmatch path is not coordinated with the new durable lock state.
backend/endpoints/roms/manual.py	Marks primary manual uploads and releases the lock on provider redownload or deletion.

Prompt To Fix All With AI

### Issue 1
backend/handler/scan_handler.py:1074-1079
**Stale lock blocks cover restoration**

When a ROM with an uploaded cover is unmatched, the endpoint clears its cover paths and URL but leaves `locked_fields` intact. The next scan sees the surviving `url_cover` lock and discards the resolved provider URL, leaving the ROM coverless until the user uploads another cover or explicitly supplies a source URL.

### Issue 2
backend/handler/scan_handler.py:1085-1088
**Failed screenshots become non-retryable**

When an update scan resolves changed screenshot URLs and a replacement download fails, `get_rom_screenshots` still records the expected destination path. On later scans the URL set is unchanged, so the failed replacement is not retried and the database continues pointing to a missing screenshot until the URLs change or resources are cleared.

### Issue 3
backend/handler/filesystem/base_handler.py:452-456
**Cancellation leaks atomic-write temp files**

When a streamed resource download is cancelled, `CancelledError` bypasses `_atomic_write`'s `except Exception` cleanup. Each cancelled download therefore leaves a `.romm_tmp_*` file in the resource directory, accumulating unused files across repeated scan cancellations or shutdowns.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

_{Reviews (1): Last reviewed commit: "fix(scan): record hand-supplied artwork ..." | Re-trigger Greptile}

[greptile-apps]

Stale lock blocks cover restoration

When a ROM with an uploaded cover is unmatched, the endpoint clears its cover paths and URL but leaves locked_fields intact. The next scan sees the surviving url_cover lock and discards the resolved provider URL, leaving the ROM coverless until the user uploads another cover or explicitly supplies a source URL.

Knowledge Base Used: ROM Scanning Flow

Prompt To Fix With AI

This is a comment left during a code review.
Path: backend/handler/scan_handler.py
Line: 1074-1079

Comment:
**Stale lock blocks cover restoration**

When a ROM with an uploaded cover is unmatched, the endpoint clears its cover paths and URL but leaves `locked_fields` intact. The next scan sees the surviving `url_cover` lock and discards the resolved provider URL, leaving the ROM coverless until the user uploads another cover or explicitly supplies a source URL.

**Knowledge Base Used:** [ROM Scanning Flow](https://app.greptile.com/romm/-/custom-context/knowledge-base/rommapp/romm/-/docs/rom-scanning-flow.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

[sdornan]

Does not reproduce, so leaving this open for a maintainer rather than resolving it myself.

The premise is right: unmatch_metadata clears path_cover_s, path_cover_l and url_cover while leaving locked_fields intact. The conclusion does not follow, because unmatching never deletes the cover files — remove_cover is only called on the remove_cover=true branch of update_rom.

So the next scan holds url_cover at "", get_cover skips the download, and then re-derives both paths from disk:

>>> path_cover_s='roms/1/1/cover/small.png' path_cover_l='roms/1/1/cover/big.png'
>>> files still on disk: ['big.png', 'small.png']

The rom keeps its cover, which is intended: unmatching drops provider matches, and a hand-uploaded cover is not one.

A genuinely coverless rom needs the files gone as well as the paths, which is the deliberate "my upload is mine" behaviour with documented exits (remove the cover, or name a different source). Since this is one remove_cover call away from becoming true, test_cover_with_no_source_url_is_rederived_from_disk now pins it.

Full reasoning in #4160 (comment).

[greptile-apps]

Thanks for the thorough write-up and for tracing it through. The key point I missed: get_cover re-derives paths from disk, so clearing path_cover_s in the DB doesn't make the cover disappear as long as the files are still there. The lock holds correctly in that state, and the rom keeps its cover.

The test_locked_cover_survives_unmatch test is exactly the right guardrail — it pins the one-remove_cover-call-away cliff edge so that distinction doesn't get accidentally collapsed later. Happy to defer to a maintainer on whether to resolve the original comment.

[greptile-apps]

Thanks for the detailed walkthrough — the reasoning is sound. The file-vs-path distinction is the key bit I missed: get_cover re-deriving from disk means the lock pointing at url_cover = "" is inert rather than blocking, because the paths come back from the filesystem check regardless.

The pinning test makes the one-remove_cover-call-away edge case explicit, which is exactly the right guard to have here. Happy to mark this resolved.

[Copilot]

Pull request overview

Improves the backend scan and resource pipeline so UPDATE metadata scans can refresh provider artwork URLs, resource downloads are written atomically (avoiding in-place truncation on interruption), and user-supplied artwork/manuals are protected via an explicit locked_fields marker (with migration backfill).

Changes:

• Add locked_fields to ROMs (model + migration backfill) and thread it through scan/update and upload flows to protect user-supplied resources.
• Allow UPDATE scans to adopt newly-resolved provider cover and screenshot URLs (while keeping manuals and text fields pinned).
• Make streamed resource downloads atomic by routing write_file_streamed through _atomic_write, updating call sites and tests.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
backend/handler/scan_handler.py	Carries locked_fields, clears locks on COMPLETE, and updates UPDATE scan URL pinning for cover/screenshots.
backend/models/rom.py	Adds locked_fields column plus helper methods (is_field_locked, locked_fields_with/without).
backend/alembic/versions/0108_roms_locked_fields.py	Adds locked_fields column and backfills existing uploaded covers based on the old inferred marker.
backend/handler/filesystem/base_handler.py	Makes write_file_streamed an async context manager using _atomic_write.
backend/handler/filesystem/resources_handler.py	Updates streamed-download call sites to the new context manager, removes now-incorrect partial-file cleanup calls.
backend/endpoints/roms/init.py	Locks/unlocks cover field on upload/removal and on explicit URL handover.
backend/endpoints/roms/manual.py	Locks/unlocks manual field on upload, redownload, and delete.
backend/tests/handler/test_fastapi.py	Adds scan-level tests proving UPDATE scans now refresh scraped cover/screenshot URLs while respecting locks.
backend/tests/handler/filesystem/test_base_handler.py	Updates streamed-write test to use the new context-manager interface.
backend/tests/handler/filesystem/test_resources_handler.py	Refactors disk-full and dropped-connection tests to exercise the real atomic streamed-write path.
backend/tests/endpoints/roms/test_rom.py	Adds endpoint tests for cover lock lifecycle (lock on upload, unlock on removal).
backend/tests/endpoints/roms/test_manual.py	Adds/updates endpoint tests to assert manual lock lifecycle behavior.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[sdornan]

Thanks @greptileai. Two of the three are fixed in 788a7ac; the third does not reproduce. Reasoning is on each thread.

• Cancellation leaks temp files (P2) — valid, fixed. CancelledError derives from BaseException, so except Exception never ran cleanup. Reproduced a stranded .romm_tmp_*.
• Failed screenshots become non-retryable (P1) — valid, fixed. Paths are now recorded only for downloads that reached the disk, and the early return counts files on disk rather than using "at least one", so a short set is retried.
• Stale lock blocks cover restoration (P1) — does not reproduce. Unmatching clears the stored paths but never deletes the cover files, so get_cover re-derives them from disk and the rom keeps its cover. Left unresolved for a maintainer to confirm rather than closed by me.

Backend suite green locally: 2834 passed, 2 skipped.

Written with Claude Code, as with the PR.

[greptile-apps]

All three look good.

P2 / P1 (screenshots): Both fixes are correct in the diff. The BaseException catch in _atomic_write is the right move — CancelledError being a BaseException subclass is one of those Python gotchas that bites any cleanup path that catches Exception. The screenshot fix does more than I reported: gating the early return on _stored_screenshot_count(rom) >= len(url_screenshots) rather than screenshots_exist means a manually deleted screenshot also heals on the next scan without needing to set overwrite=True. That's a better invariant than what the fix strictly needed.

P3 (stale lock after unmatch): The reasoning holds. The critical piece I missed is that unmatch_metadata clears the DB fields but the files stay on disk, so get_cover re-derives the paths from disk on the next scan and the locked cover is restored. The probe output makes this concrete, and test_locked_cover_survives_unmatch is the right pin — one file deletion away from being wrong is exactly the kind of subtle invariant that benefits from a named test rather than an inline comment.

The test_locked_cover_survives_unmatch test itself: it exercises get_cover directly rather than going through a full scan, which is fine since that's the exact call path. Worth confirming the test ROM starts with locked_fields=["url_cover"] so the assertion is actually doing work — I don't see that in the test body as shown, but if it doesn't set the lock the assertion on path_s / path_l still validates the file-derivation half, which is the point.

[sdornan]

Thanks @copilot. Both valid, fixed in 8499b9e. Reasoning is on the thread.

• Lock updates could clobber each other — only the cover block wrote locks, so nothing was lost yet, but it was one addition away from being wrong. Both blocks now mutate a single running set written back once.
• url_manual did not release the manual lock — correct, and it contradicted the PR description. The obvious fix would have been wrong: an upload leaves url_manual populated and the client posts stored urls on every save, so releasing on "a url was sent" would have unlocked the manual the first time a user edited anything else. Both fields now release only when the url actually changes.

Also fixed the store_ra_badge log line, which reported "fetch cover" while downloading a badge.

Backend suite green locally: 2834 passed, 2 skipped.

Written with Claude Code, as with the PR.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (1)

backend/handler/filesystem/resources_handler.py:528

• In get_rom_screenshots(), when overwrite=False but the on-disk set is short, the loop still calls _store_screenshot() for every URL, which overwrites screenshots that already exist. That contradicts the overwrite flag and can cause unnecessary downloads.

        # Download and store new screenshots
        path_screenshots: list[str] = []
        for idx, url_screenshot in enumerate(url_screenshots):
            await self._store_screenshot(rom, url_screenshot, idx)
            path = self._get_screenshot_path(rom, str(idx))

[greptile-apps]

The analysis is correct. Looking at get_cover in resources_handler.py:

# A single fetch writes both sizes
if url_cover and (overwrite or not has_cover_l):
    await self._store_cover(entity, url_cover)
...
# Return paths for existing covers
path_cover_s = (
    self._get_cover_path(entity, CoverSize.SMALL)
    if self.cover_exists(entity, CoverSize.SMALL)
    else None
)

When unmatch_metadata clears path_cover_s/path_cover_l in the DB but leaves files on disk, the next scan calls get_cover with url_cover="" (from the locked branch), which is falsy — so no download is attempted. But _get_cover_path globs the cover directory for existing files and re-derives the paths from whatever is there. The locked cover survives intact.

The only scenario where the lock would block restoration is if the files were also deleted — but that's a complete rescan, which already clears locked_fields in this PR (the "locked_fields": [] reset alongside the other resource resets). A lock can't outlast the files it's protecting.

test_locked_cover_survives_unmatch is the right guardrail — it's one remove_cover() call away from being a real regression if that ever changes, and the test would catch it. Happy to resolve this thread.

All three items acknowledged — two fixed, one confirmed non-reproducing with a pinning test. The PR looks good to me.