fix(metadata): identify archives and folder ROMs by the right file - #4146
Open
Spinnich wants to merge 1 commit into
Open
fix(metadata): identify archives and folder ROMs by the right file#4146Spinnich wants to merge 1 commit into
Spinnich wants to merge 1 commit into
Conversation
Since 4.9.0 a multi-file archive's RomFile carries a composite digest streamed across every entry, with per-member digests kept alongside in archive_members. Hasheous and Playmatch read their lookup digests straight off that record, but both databases index a multi-file archive by its largest internal member, so those ROMs stopped matching and fell back to filename matching. Add RomFile.lookup_hashes, which decides which digests a ROM database is queried with: a CHD's embedded disc-data SHA-1, else the largest archive member, else the file's own. Hasheous and Playmatch both read it. ScreenScraper deliberately stays on the composite, since its jeuInfos lookup falls back to romnom. Playmatch also picked its file with the first one that had bytes, unfiltered by is_top_level. iter_files is a bare os.walk with no sort and Rom.files has no order_by, so a multi-disc folder ROM was identified by an arbitrary file that could differ between machines scanning the same library. Select the largest top-level file instead, as Hasheous and ScreenScraper do, with a path tie-break for determinism. This half is pre-existing rather than a 4.9.0 regression, but leaving it would keep multi-disc folder ROMs failing on Playmatch. The manual-match suggestion shares that selection, and no longer falls back to the ROM-level composite: contributing nothing to a public index beats contributing a hash of no single file. Existing libraries need no re-hash; archive_members has been populated since 4.9.0, so a rescan is enough. Fixes rommapp#3742 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Greptile SummaryThis PR centralizes ROM-database lookup hash selection and updates Hasheous and Playmatch to identify archives by their largest member and CHDs by their embedded SHA-1. It also makes Playmatch file selection deterministic for multi-file folder ROMs and adds regression coverage for provider-specific behavior.
Confidence Score: 5/5The PR appears safe to merge with no concrete blocking or independently actionable non-blocking issues identified. The shared hash-selection behavior is consistently applied to Hasheous and Playmatch, deterministic file selection removes filesystem-order dependence, and focused tests cover the changed archive, CHD, and folder-ROM paths. Important Files Changed
Reviews (1): Last reviewed commit: "fix(metadata): identify archives and fol..." | Re-trigger Greptile |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Explain the changes or enhancements you are proposing with this pull request.
Fixes #3742.
Since 4.9.0 an archive's
RomFilecarries a composite digest streamed across everyentry in the archive (
a170649fe, PR #3412). The metadata handlers were never updatedand kept reading their lookup digests straight off that record, so they silently
switched from "the ROM inside the zip" to "a hash of the whole archive". No ROM database
indexes that composite, so any zip with more than one member (arcade/MAME sets, but also
an ordinary ROM zipped next to a readme or patch) stopped matching by hash and fell back
to filename matching. Single-file zips were unaffected, which is why most libraries
never noticed.
This is a re-land of #4059, which implemented exactly this and was closed unmerged on
2026-08-02. The review request on it is still open and no formal review was ever
submitted, so I've kept half A deliberately close to that diff to keep the re-land
legible.
What changed
Which hashes (the 4.9.0 regression). New
RomFile.lookup_hashesdecides what a ROMdatabase should be queried with:
chd_sha1_hashfor CHDs, else the largestarchive_membersentry, else the file's own digests. Hasheous and Playmatch both readit. CHD behaviour for Hasheous is byte-identical to before; only the archive case moves.
Which file (pre-existing, not a 4.9.0 regression). Playmatch picked its file with
next(file for file in files if file.file_size_bytes > 0)— the first file with bytes,not the largest, with no
is_top_levelfilter.iter_filesis a bareos.walkwith nosort and
Rom.fileshas noorder_by, so for a multi-disc folder ROM the file Playmatchwas asked about was arbitrary filesystem order and could differ between two machines
scanning the same library. It was also frequently the
.m3uplaylist, which carries nogame data and can only ever miss.
To be clear about scope: this half is not part of the 4.9.0 regression. 4.8.1 carries
the same three lines verbatim (
ca58028eb, "playmatch should just use the first file",2025-07-10). It's in here because fixing only the hash selection would leave multi-disc
folder ROMs still failing on Playmatch, and the two questions are the same question.
A new
_select_lookup_filereplaces both copies of that expression (lookup andsuggestion) with the largest top-level file, tie-broken on path.
Worth looking at hardest
_select_lookup_file'sis_top_levelfilter is a behaviour narrowing, not just atie-break. A folder ROM whose files all live in subdirectories (
Game/Disc 1/game.bin)now gets no Playmatch lookup at all, where before it got one against an arbitrary
nested file. Hasheous and ScreenScraper already drop those libraries, so this aligns
the three, but it is a real loss for that layout. Happy to add a last-resort fallback
to the largest sizeable file if you'd rather keep the old reach.
submit_manual_match_suggestionnow contributes the member's digests alongside thearchive's
fileName/fileSize. Harmless for lookups (a hash match outranksFileNameAndSize), but the contributed triple is internally inconsistent. Sending themember's name and size would be the fuller fix; I left it out of scope here and it's
worth its own issue. Relatedly, the suggestion no longer falls back to
rom.md5_hash/rom.sha1_hashwhen no file qualifies — it just returns. Contributingnothing to a public index beats contributing a composite that is a digest of no single
file.
Playmatch now looks CHDs up by
chd_sha1_hashinstead of the container digests. Thisis right in principle and matches Hasheous, but Playmatch's CHD behaviour is the one
leg I couldn't confirm with a live match.
Deliberately unchanged
ScreenScraper keeps sending the archive's own composite digests. Its
jeuInfoslookupfalls back to
romnom(the filename), which for an arcade zip is the set name, so thosestill resolve; moving it would shift match results across many libraries. There's a new
test pinning that, so a future change there has to be a choice rather than a side effect.
RetroAchievements is a separate RAHasher pipeline and is untouched.
No migration, no re-hash.
archive_membershas been populated since 4.9.0 andalready carries the member digests, so this reads correct data off rows already in the
database.
Correction to the issue body: #3742 claims the arcade fixtures demonstrate the
Playmatch half. They don't — Playmatch does not index MAME sets at all and returns
NoMatchfor them on either hash. An ordinary No-Intro ROM zipped with a readme iswhat reproduces the Playmatch half; both are covered below.
Checklist
Please check all that apply.
Verification
Live lookups against the real Hasheous and Playmatch APIs, driving the real
FSRomsHandler.get_rom_files()(onlyRAHasherService.calculate_hashpatched out) intothe real
lookup_rom:aerofgt.zip(8 members)c5347ceb…694853ae…NoMatch(expected)ga2.zip(19 members)c6ed4e03…0cd4f9af…NoMatch(expected)e883e0d4…SHA1igdb 1068 (unchanged)9f018a5b…e883e0d4…SHA1igdb 1068 (wasNoMatch)The two arcade
NoMatches are not a failure: Playmatch doesn't index MAME on eitherhash. The SMB3+readme row is the fixture that demonstrates the Playmatch half.
Multi-disc folder ROM (
Metal Gear Solid (USA) (Rev 1)/, two CHDs plus an.m3u):picks
Disc 1, not the playlist; sends the CHD's disc-data SHA-1, not the container's;identical selection across 4 list orderings (order-dependent on master).
Tests. 15 new tests. All 7 in
tests/models/test_rom.pyfailed on master withImportError: cannot import name 'LookupHashes'; the Hasheous payload test failedsending
compositemd5/sha1/crc; 6 of the 7 Playmatch tests failed on master (compositehashes,
fileName='game.m3u', order-dependent payload, a nested file selected, acomposite POSTed as a suggestion). Edge cases covered: single-member archive unchanged,
archive_membersNoneand[], a member with a missing orNonesize, CHDprecedence, nested-file exclusion, and no-eligible-file.
Full backend suite: 2762 passed, 2 skipped.
trunk fmt+trunk check --no-fixonall 7 files: no issues.
No OpenAPI change — proven, not assumed: dumped
app.openapi()from amasterworktree and from this branch and diffed them. Identical, 26,574 lines each. So no
npm run generateand no frontend typecheck.Screenshots (if applicable)
N/A — backend only. No UI, no i18n, no schema change. Evidence is the live-lookup table
above.