From 342857b14b43bee89cdeb523adb7b7d4442dfe6f Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 2 Jun 2026 08:52:25 +0000 Subject: [PATCH] fix(roms): repair multi-file ROM downloads broken by deferred file stats PR #3425 dropped `lazy="joined"` from `RomFile.rom` and removed the `joinedload(RomFile.rom)` from the ROM loaders to speed up the gallery query. That left the `RomFile.rom` backref unpopulated. Single-file downloads only read `RomFile.full_path` (built from `file_path`/ `file_name`), so they kept working, but multi-file (game folder) downloads call `file_name_for_download()` / `is_top_level`, which read `self.rom.full_path`. With no eager-loaded backref, that triggered a lazy load on a detached instance once the handler session closed, raising `DetachedInstanceError` and returning a 500. Rather than reverting the loader changes (and the gallery gains), wire the `RomFile.rom` backref up in Python from the parent ROM we already hold in memory, via `set_committed_value`. This is zero extra DB cost and only runs on the detail/download paths (`with_details` and `get_roms_by_fs_name`); the optimized `filter_roms` gallery query is untouched. https://claude.ai/code/session_01PSXKmejPRzdxLFMN6P2QQ4 --- backend/handler/database/roms_handler.py | 26 ++++++++++++++++- backend/tests/handler/test_db_handler.py | 36 +++++++++++++++++++++++- 2 files changed, 60 insertions(+), 2 deletions(-) diff --git a/backend/handler/database/roms_handler.py b/backend/handler/database/roms_handler.py index b91be4e16..4a75bb96a 100644 --- a/backend/handler/database/roms_handler.py +++ b/backend/handler/database/roms_handler.py @@ -30,6 +30,7 @@ from sqlalchemy.orm import ( selectinload, undefer, ) +from sqlalchemy.orm.attributes import set_committed_value from sqlalchemy.sql.elements import ColumnElement from sqlalchemy.sql.selectable import Select @@ -123,6 +124,26 @@ def _create_metadata_id_case( ) +def _link_rom_files_to_parent(result: "Rom | Iterable[Rom] | None") -> None: + """Populate the `RomFile.rom` backref from the already-loaded parent ROM. + + The detail/download loaders eager-load `Rom.files` but not the reverse + `RomFile.rom` relationship. `RomFile.full_path` / `is_top_level` / + `file_name_for_download` read `self.rom`, which would otherwise trigger a + lazy load that fails once the session closes (`DetachedInstanceError`, + surfacing as a 500 on multi-file ROM downloads). We already hold the parent + in memory, so wire the backref up directly instead of re-adding a per-file + `joinedload` (which would undo the gallery query gains from #3425). + """ + if result is None: + return + + roms = [result] if isinstance(result, Rom) else result + for rom in roms: + for file in rom.files: + set_committed_value(file, "rom", rom) + + def with_details(func): @functools.wraps(func) def wrapper(*args, **kwargs): @@ -158,7 +179,9 @@ def with_details(func): undefer(Rom.multi_file), undefer(Rom.top_level_file_count), ) - return func(*args, **kwargs) + result = func(*args, **kwargs) + _link_rom_files_to_parent(result) + return result return wrapper @@ -993,6 +1016,7 @@ class DBRomsHandler(DBBaseHandler): .all() ) + _link_rom_files_to_parent(roms) return {rom.fs_name: rom for rom in roms} @begin_session diff --git a/backend/tests/handler/test_db_handler.py b/backend/tests/handler/test_db_handler.py index df1389a4d..54b7f85f1 100644 --- a/backend/tests/handler/test_db_handler.py +++ b/backend/tests/handler/test_db_handler.py @@ -14,7 +14,7 @@ from handler.database import ( ) from models.assets import Save, Screenshot, State from models.platform import Platform -from models.rom import Rom +from models.rom import Rom, RomFile from models.user import Role, User @@ -73,6 +73,40 @@ def test_roms(rom: Rom, platform: Platform): assert len(roms) == 1 +def test_multi_file_rom_backref_survives_session_close(rom: Rom, platform: Platform): + """Multi-file ROM downloads read `file.rom.full_path` after the handler + session closes. The detail loaders eager-load `Rom.files` but not the + reverse `RomFile.rom` relationship, so without the backref being populated + this raises `DetachedInstanceError` (a 500 on the download endpoint). + """ + folder_path = f"{rom.fs_path}/{rom.fs_name}" + for file_name in ("disc1.bin", "disc2.bin"): + db_rom_handler.add_rom_file( + RomFile( + rom_id=rom.id, + file_name=file_name, + file_path=folder_path, + file_size_bytes=1, + ) + ) + + fetched = db_rom_handler.get_rom(rom.id) + assert fetched is not None + assert len(fetched.files) == 2 + + # These all dereference `file.rom` on a now-detached instance. + for file in fetched.files: + assert file.rom.full_path == folder_path + assert file.is_top_level + assert file.file_name_for_download() == file.file_name + + # `get_roms_by_ids` (used by the bulk zip download) must behave the same. + by_ids = db_rom_handler.get_roms_by_ids([rom.id]) + assert len(by_ids) == 1 + for file in by_ids[0].files: + assert file.rom.full_path == folder_path + + def test_filter_last_played(rom: Rom, platform: Platform, admin_user: User): second_rom = db_rom_handler.add_rom( Rom(