From be6ad905b1820cb3d747ede5d97aa407daccdead Mon Sep 17 00:00:00 2001 From: Georges-Antoine Assi <3247106+gantoine@users.noreply.github.com> Date: Thu, 8 Oct 2026 19:12:12 -0400 Subject: [PATCH] fix(sync): serve the newest on-disk RetroArch state in a slot (#5193) * fix(sync): resolve RetroArch paths across emulators sharing a core folder Several RomM emulator ids map to one RetroArch folder (mednafen_psx_hw, beetle_psx_hw and the raw "Beetle PSX HW" all sync through saves/Beetle PSX HW/). The manifest built paths from whichever id an asset carried, but GET parsed the folder back to the canonical id and matched it exactly, so an asset filed under another alias was advertised and then 404'd. Uploads also started a second autosave history beside the alias's. The manifest now lists the newest asset per folder, and GET, PUT and DELETE resolve a path across every alias. Rewrites of an existing unslotted save or state stay under that row's own emulator. Co-Authored-By: Claude Opus 5.5 (1M context) * fix(sync): prefer on-disk alias saves and share newest-per-key grouping pr-ready step 2 (code-review --fix). Co-Authored-By: Claude Opus 5.5 (1M context) * refactor(sync): one alias helper and a lineage computed once pr-ready step 3 (simplify). Co-Authored-By: Claude Opus 5.5 (1M context) * test(sync): type the alias fixture's asset kind pr-ready step 4 (review-polish). Co-Authored-By: Claude Opus 5.5 (1M context) * test(sync): isolate the asset dir per RetroArch sync test The alias tests write different bytes to the same asset paths, and user and ROM ids repeat across xdist worker databases, so parallel runs raced on those files. Co-Authored-By: Claude Opus 5.5 (1M context) * fix(sync): serve the newest on-disk RetroArch state in a slot Missing states are dropped before slot grouping for the manifest and reads, so a newer missing state (or alias) no longer hides an older one still on disk. A PUT still matches the missing row and revives it in place. Co-Authored-By: Claude Opus 5.5 (1M context) * fix(sync): fall back past a RetroArch file that vanished unflagged Addresses Greptile's review on #5193: a state whose file was deleted before `missing_from_fs` was set still won its slot. The manifest now rebuilds when hashing flags an asset, and GET flags a missing file and resolves again, so an older asset at the path is listed and served. Co-Authored-By: Claude Opus 5.5 (1M context) --------- Co-authored-by: Claude Opus 5.5 (1M context) --- backend/endpoints/sync/retroarch.py | 8 ++ .../handler/sync/retroarch/sync_handler.py | 39 +++++++- .../tests/endpoints/test_sync_retroarch.py | 92 +++++++++++++++++-- docs/BACKEND_ARCHITECTURE.md | 5 +- 4 files changed, 129 insertions(+), 15 deletions(-) diff --git a/backend/endpoints/sync/retroarch.py b/backend/endpoints/sync/retroarch.py index 410efef57..b9520fba6 100644 --- a/backend/endpoints/sync/retroarch.py +++ b/backend/endpoints/sync/retroarch.py @@ -444,6 +444,13 @@ async def retroarch_sync_get(request: Request, file_path: str) -> Response: return _empty(status.HTTP_404_NOT_FOUND) asset = await _get_asset(request.user, rom, parsed) + # A file deleted outside RomM hides any older asset at this path until flagged. + while ( + asset + and not asset.missing_from_fs + and await asyncio.to_thread(sync_handler.flag_if_missing, asset) + ): + asset = await _get_asset(request.user, rom, parsed) if not asset: return _empty(status.HTTP_404_NOT_FOUND) @@ -581,6 +588,7 @@ async def retroarch_sync_put(request: Request, file_path: str) -> Response: rom, parsed.emulator, file_name, + prefer_on_disk=False, ) write_file_name = existing.file_name if existing else file_name emulator = existing.emulator if existing else parsed.emulator diff --git a/backend/handler/sync/retroarch/sync_handler.py b/backend/handler/sync/retroarch/sync_handler.py index 51fd6cdba..8feeb796d 100644 --- a/backend/handler/sync/retroarch/sync_handler.py +++ b/backend/handler/sync/retroarch/sync_handler.py @@ -221,12 +221,21 @@ def states_in_slot( def resolve_state_by_slot( - user: User, rom: Rom, emulator: str | None, requested_file_name: str + user: User, + rom: Rom, + emulator: str | None, + requested_file_name: str, + prefer_on_disk: bool = True, ) -> State | None: - """The state the slot a canonical name points at serves, whatever its name.""" + """The state the slot a canonical name points at serves, whatever its name. + + Args: + prefer_on_disk: False ranks a `missing_from_fs` row by recency alone, so + an upload revives the newest state in place. + """ return max( states_in_slot(user, rom, emulator, requested_file_name), - key=on_disk_recency_key, + key=on_disk_recency_key if prefer_on_disk else recency_key, default=None, ) @@ -631,6 +640,22 @@ def _mark_missing_from_fs(asset: Save | State | Screenshot) -> None: db_state_handler.update_state(asset.id, update, touch=False) else: db_screenshot_handler.update_screenshot(asset.id, update) + asset.missing_from_fs = True + + +def flag_if_missing(asset: Save | State | Screenshot) -> bool: + """Flag ``asset`` `missing_from_fs` when its file is gone. + + Returns: + Whether it was gone. + """ + try: + if fs_asset_handler.validate_path(asset.full_path).is_file(): + return False + except ValueError: + return False + _mark_missing_from_fs(asset) + return True async def _asset_md5(asset: Save | State | Screenshot) -> str | None: @@ -760,8 +785,12 @@ async def build_manifest( """The server manifest RetroArch diffs against, sorted by path.""" # Off the event loop: on a large library the queries run for seconds and # would stall every other request the worker is serving. - assets, saves = await asyncio.to_thread(_manifest_assets, user, can_see) - digests = await asset_md5s([entry.asset for entry in assets]) + while True: + assets, saves = await asyncio.to_thread(_manifest_assets, user, can_see) + digests = await asset_md5s([entry.asset for entry in assets]) + # Hashing flags a vanished file, which can unhide an older asset at its path. + if not any(entry.asset.missing_from_fs for entry in assets): + break hashed = [ (entry, digest) for entry, digest in zip(assets, digests, strict=True) if digest ] diff --git a/backend/tests/endpoints/test_sync_retroarch.py b/backend/tests/endpoints/test_sync_retroarch.py index 38488966e..1d4dcc649 100644 --- a/backend/tests/endpoints/test_sync_retroarch.py +++ b/backend/tests/endpoints/test_sync_retroarch.py @@ -1214,15 +1214,18 @@ class TestRetroArchSyncCoreAliases: ) def _state( - self, admin_user: User, rom: Rom, emulator: str, content: bytes + self, + admin_user: User, + rom: Rom, + emulator: str, + content: bytes, + file_name: str = "test_rom.state", ) -> State: - path = self._write( - admin_user, rom, "states", emulator, "test_rom.state", content - ) + path = self._write(admin_user, rom, "states", emulator, file_name, content) return factories.make_state( rom, admin_user, - "test_rom.state", + file_name, file_path=path, file_size_bytes=len(content), emulator=emulator, @@ -1348,16 +1351,87 @@ class TestRetroArchSyncCoreAliases: assert self._manifest_paths(client) == ["states/Beetle PSX HW/test_rom.state"] assert client.get(self.STATE_URL, auth=ADMIN_AUTH).content == b"newer" + def _missing_state( + self, admin_user: User, rom: Rom, emulator: str, file_name: str + ) -> State: + state = self._state(admin_user, rom, emulator, b"gone", file_name) + return db_state_handler.update_state( + state.id, {"missing_from_fs": True}, touch=False + ) + + @_mock_asset_md5() + def test_a_state_on_disk_wins_over_a_newer_missing_one_in_its_slot( + self, _asset_md5: mock.AsyncMock, client, admin_user: User, rom: Rom + ): + self._state(admin_user, rom, "beetle_psx_hw", b"older") + self._missing_state( + admin_user, rom, "beetle_psx_hw", "test_rom [2026-01-01 00-00-00].state" + ) + + assert self._manifest_paths(client) == ["states/Beetle PSX HW/test_rom.state"] + assert client.get(self.STATE_URL, auth=ADMIN_AUTH).content == b"older" + @_mock_asset_md5() def test_a_state_on_disk_wins_over_a_newer_missing_alias( self, _asset_md5: mock.AsyncMock, client, admin_user: User, rom: Rom ): - self._state(admin_user, rom, "beetle_psx_hw", b"argosy") - missing = self._state(admin_user, rom, "mednafen_psx_hw", b"gone") - db_state_handler.update_state(missing.id, {"missing_from_fs": True}) + self._state(admin_user, rom, "beetle_psx_hw", b"older") + self._missing_state(admin_user, rom, "mednafen_psx_hw", "test_rom.state") assert self._manifest_paths(client) == ["states/Beetle PSX HW/test_rom.state"] - assert client.get(self.STATE_URL, auth=ADMIN_AUTH).content == b"argosy" + assert client.get(self.STATE_URL, auth=ADMIN_AUTH).content == b"older" + + def _vanished_state(self, admin_user: User, rom: Rom, emulator: str) -> State: + state = self._state(admin_user, rom, emulator, b"gone") + fs_asset_handler.validate_path(state.full_path).unlink() + return state + + def test_the_manifest_falls_back_when_the_newest_state_file_vanished( + self, client, admin_user: User, rom: Rom + ): + self._state(admin_user, rom, "beetle_psx_hw", b"older") + vanished = self._vanished_state(admin_user, rom, "mednafen_psx_hw") + + assert self._manifest_paths(client) == ["states/Beetle PSX HW/test_rom.state"] + state = db_state_handler.get_state(user_id=admin_user.id, id=vanished.id) + assert state is not None and state.missing_from_fs + + def test_get_falls_back_when_the_newest_state_file_vanished( + self, client, admin_user: User, rom: Rom + ): + self._state(admin_user, rom, "beetle_psx_hw", b"older") + vanished = self._vanished_state(admin_user, rom, "mednafen_psx_hw") + + assert client.get(self.STATE_URL, auth=ADMIN_AUTH).content == b"older" + state = db_state_handler.get_state(user_id=admin_user.id, id=vanished.id) + assert state is not None and state.missing_from_fs + + def test_get_404s_when_every_state_file_in_the_slot_vanished( + self, client, admin_user: User, rom: Rom + ): + self._vanished_state(admin_user, rom, "beetle_psx_hw") + self._vanished_state(admin_user, rom, "mednafen_psx_hw") + + response = client.get(self.STATE_URL, auth=ADMIN_AUTH) + + assert response.status_code == status.HTTP_404_NOT_FOUND + + def test_uploading_to_a_slot_revives_its_newest_missing_state( + self, client, admin_user: User, rom: Rom + ): + older = self._state(admin_user, rom, "beetle_psx_hw", b"older") + missing = self._missing_state( + admin_user, rom, "mednafen_psx_hw", "test_rom.state" + ) + + response = client.put(self.STATE_URL, content=b"fresh", auth=ADMIN_AUTH) + + assert response.status_code == status.HTTP_204_NO_CONTENT + states = {s.id: s for s in db_state_handler.get_states(user_id=admin_user.id)} + assert states.keys() == {older.id, missing.id} + assert not states[missing.id].missing_from_fs + assert states[missing.id].file_size_bytes == len(b"fresh") + assert client.get(self.STATE_URL, auth=ADMIN_AUTH).content == b"fresh" @_mock_asset_md5() def test_delete_removes_states_under_every_alias_in_the_slot( diff --git a/docs/BACKEND_ARCHITECTURE.md b/docs/BACKEND_ARCHITECTURE.md index 6a9d4b5e6..039f2c06f 100644 --- a/docs/BACKEND_ARCHITECTURE.md +++ b/docs/BACKEND_ARCHITECTURE.md @@ -1033,7 +1033,10 @@ World`), so a name shared across platforms resolves to the lowest visible ROM id claims it in Redis, so concurrent or same-second uploads from any endpoint never share a file. Every slot prune is scoped to the uploaded save's lineage. - **Manifest:** assets whose file is gone are flagged `missing_from_fs`. Hashes - are MD5s of the bytes on disk, Redis-cached by path, size and mtime. + are MD5s of the bytes on disk, Redis-cached by path, size and mtime. A state + slot lists and serves its newest state on disk, and a PUT revives a newer + missing one in place. A manifest or GET that finds a file gone flags it and + resolves again, so an older asset at that path takes its place. - **PSP:** PPSSPP's `PSP/SAVEDATA//` files are stored as one zipped `Save` per folder. A folder whose title matches no ROM is buffered in the user's `retroarch/psp_pending/` folder until it resolves, or mapped through