From d25adb63d505e23a0ee30162220cee34d74405a5 Mon Sep 17 00:00:00 2001 From: danielcopper Date: Fri, 24 Jul 2026 15:33:45 +0200 Subject: [PATCH 1/3] fix(version-picker): disable versions missing from romm --- docs/architecture/backend-architecture.md | 14 ++ .../architecture/steam-non-steam-shortcuts.md | 26 +++ docs/user-guide/managing-games.md | 13 +- docs/user-guide/save-sync.md | 5 + py_modules/adapters/romm/http.py | 7 +- py_modules/adapters/romm/romm_api.py | 9 +- py_modules/services/artwork.py | 2 +- py_modules/services/protocols/transport.py | 4 + py_modules/services/version_switch.py | 138 ++++++++++--- src/api/backend.ts | 15 +- src/components/RomMPlaySection.test.tsx | 7 +- src/components/SavesTab.test.tsx | 30 ++- src/components/SavesTab.tsx | 2 +- src/components/VersionPicker.test.tsx | 104 +++++++++- src/components/VersionPicker.tsx | 25 ++- tests/adapters/romm/test_romm_api.py | 12 ++ tests/contract/test_fetch_cover.py | 12 ++ tests/contract/test_version_picker.py | 65 +++++- tests/fakes/fake_romm_api.py | 14 ++ tests/fakes/fake_save_api.py | 3 + tests/fakes/test_fake_romm_api.py | 16 ++ tests/services/test_artwork.py | 4 +- tests/services/test_version_switch.py | 189 +++++++++++++++++- 23 files changed, 645 insertions(+), 71 deletions(-) diff --git a/docs/architecture/backend-architecture.md b/docs/architecture/backend-architecture.md index d9fff037..f835d9cd 100644 --- a/docs/architecture/backend-architecture.md +++ b/docs/architecture/backend-architecture.md @@ -127,6 +127,20 @@ the rest are single modules. A service over ~700 LOC is the decomposition signal | `connection.py` | ConnectionService — connection test + RomM minimum-version gate + Client API Token lifecycle (mint/establish via credentials, validate/store a user-pasted token, or exchange a short-lived pairing code for a token — the latter two for OIDC accounts; host-bound to the minting origin; see [ConnectionService notes](#connectionservice-notes)) | | `protocols/` | Protocol interfaces grouped by concern (see [Protocol Interfaces](#protocol-interfaces)) | +#### Version-list liveness + +`VersionSwitchService.get_version_list` separates retained local membership from current RomM availability. On each lazy +Game Page load, the bound detail request proves the bound id live and supplies the direct sibling ids; every other local +group member is exact-id probed through `RommRomReader.get_rom_once`, the adapter's single-attempt short-timeout +`request_once` path, concurrently on the existing executor fan-out. Only `RommNotFoundError` produces `vanished: true`. +All other probe failures and malformed/falsy responses fail open. No result is persisted. + +The response carries `vanished` on every version and `bound_vanished` even when `multi_version` is false. Retained +vanished rows remain in the payload and keep the independently-computed `switchable` membership verdict, but are +excluded from default ranking. A bound-id 404 is a successful entity verdict (`bound_vanished: true`, +`server_query_failed: false`), not a global reachability signal. Artwork and save reads do not contribute availability +evidence; `fetch_cover_base64` remains a nullable data callable. + #### LibraryService decomposition (`services/library/`) The library sync subsystem is a façade over three sub-services that coordinate through a shared `LibrarySyncStateBox`: diff --git a/docs/architecture/steam-non-steam-shortcuts.md b/docs/architecture/steam-non-steam-shortcuts.md index 49754138..d679db32 100644 --- a/docs/architecture/steam-non-steam-shortcuts.md +++ b/docs/architecture/steam-non-steam-shortcuts.md @@ -125,6 +125,32 @@ Two guards keep a re-import from wiping a freshly-bound shortcut (`#1036`): the run just re-bound onto the new `rom_id` is never emitted for removal. The `get_by_app_id` reverse lookup orders `rom_id DESC LIMIT 1` so it resolves the live (newest) binding for any pre-migration edge state. +### Retained-row availability in the version picker + +ADR-0007 keeps a `roms` row after RomM stops returning that id because the row can anchor local-only saves, playtime, +and an installed ROM. Retention does not imply that RomM still offers the row as a playable version. The Game Page's +lazy `get_version_list` load therefore recomputes availability every time; there is no liveness column, migration, +cache, or persisted verdict. + +The existing detail request for the bound id supplies its own answer and RomM's current direct `sibling_roms` view. The +bound id and every local id positively present in that view are live. A local group member absent from the direct view +is only a suspect because RomM sibling membership can be transitive, so each suspect is checked by an exact-id +`get_rom_once` request. These checks fan out concurrently on the worker executor and use the short timeout with no +retry, keeping them off the event loop and out of the initial Game Page render. + +Only a typed `RommNotFoundError` from an exact id marks that entry `vanished`. A successful response is live; timeout, +transport, authentication, server errors, and malformed or empty data all fail open and leave it available. If the bound +detail itself 404s, `bound_vanished` is true, `server_query_failed` stays false, and every other local member is checked +individually. That entity-specific 404 is not fed into the global connection store; a genuine explicit +server-unreachable result still is. Cover and save endpoints are not liveness authorities. + +Vanished rows stay visible with their active and downloaded markers, but are disabled and excluded before the existing +default-resolution kernel runs. `vanished` does not change `switchable` or +`domain.sibling_group.target_in_sibling_group`: availability and sibling membership remain separate verdicts. This lets +a shortcut still bound to a vanished id show the retained context while the user selects a live alternative. The Saves +tab likewise skips positively vanished inactive installs before checking local drift, then continues through later live +candidates. + ## Sync-start reconcile of Steam-UI-deleted shortcuts A user can delete a RomM shortcut through **Steam's own UI** (remove from library), which the plugin never observes. The diff --git a/docs/user-guide/managing-games.md b/docs/user-guide/managing-games.md index 16fb0ecb..c2b470f7 100644 --- a/docs/user-guide/managing-games.md +++ b/docs/user-guide/managing-games.md @@ -54,6 +54,16 @@ a not-yet-synced version whose match conflicts, are both shown (so you know they either would move the shortcut onto a different game. To make one selectable, fix its metadata match in RomM so it agrees with this game's, then re-sync. +A retained local version that RomM has positively confirmed it no longer has is also shown dimmed and disabled, with the +separate label _"No longer available on RomM"_. Its **active** and **Downloaded** markers remain visible so it is clear +what the shortcut and local files still refer to, but it never receives the **Default** badge. If the shortcut is still +active on that unavailable id, choose any live alternative in the same list to recover the game. + +This availability check is fresh each time the version list loads and is not saved in the plugin database. Only a 404 +for that exact ROM id establishes that it is gone. Timeouts, sign-in/permission errors, server errors, and malformed +responses fail open: the plugin does not disable a version merely because it could not get a trustworthy answer. A +ROM-specific 404 also does not make the rest of the plugin report that RomM is offline. + Selecting a different version **rebinds** the game to it: the Download button now fetches that version, the panel's title and Region/Languages rows update to reflect it, and its cover refreshes to the new version. The Steam shortcut keeps its name, its place in your collections, and its playtime — all tied to the shortcut, which never changes. A @@ -71,7 +81,8 @@ switch. What happens depends on whether the version you're leaving is downloaded drops the sync option and offers only **Switch anyway** and **Cancel**, explaining why. When you leave a downloaded version with unsynced saves behind, the **Saves** tab shows a reminder banner ("switch back -to sync them") so those saves aren't forgotten. +to sync them") so those saves aren't forgotten. It does not recommend switching back to a version positively confirmed +as no longer available on RomM; it skips that retained row and continues checking any later live versions. While a download of the game is running, switching is blocked with a short message — cancel the running download first. diff --git a/docs/user-guide/save-sync.md b/docs/user-guide/save-sync.md index cf44cf54..7ae2d614 100644 --- a/docs/user-guide/save-sync.md +++ b/docs/user-guide/save-sync.md @@ -40,6 +40,11 @@ plugin warns you before you leave such a version and shows a reminder banner on forgotten (see [Managing Games → Versions](managing-games.md#versions)). Nothing is lost — the saves just don't sync while their version is inactive. +The reminder never tells you to switch back to a retained version that the current version-list check positively +confirmed is [no longer available on RomM](managing-games.md#the-switch-version-control). Such a row may still keep its +local files and saves, but it cannot be selected; the Saves tab skips it and continues looking for drift on later live +inactive versions. + ## Important: Use Your Own RomM Account Save files in RomM are tied to the authenticated user account. If multiple people share the same RomM account, their diff --git a/py_modules/adapters/romm/http.py b/py_modules/adapters/romm/http.py index 6661c65e..deeb73fb 100644 --- a/py_modules/adapters/romm/http.py +++ b/py_modules/adapters/romm/http.py @@ -348,10 +348,9 @@ def request_once(self, path: str, *, timeout: int): """GET a JSON resource in a SINGLE attempt with a SHORT *timeout*. Deliberately bypasses :meth:`with_retry` and the 30s urlopen timeout used - by :meth:`request`. The launch-gate reachability probe needs a fast - offline verdict (~3s, one shot) instead of waiting through 3 retry - attempts and up to ~90s of accumulated remote timeouts. The real sync - paths keep the retrying :meth:`request`. + by :meth:`request`. Point probes need a fast verdict (~3s, one shot) + instead of waiting through 3 retry attempts and up to ~90s of accumulated + remote timeouts. The real sync paths keep the retrying :meth:`request`. """ return self._build_get(path, timeout=timeout)() diff --git a/py_modules/adapters/romm/romm_api.py b/py_modules/adapters/romm/romm_api.py index fa836eb9..ff8320f6 100644 --- a/py_modules/adapters/romm/romm_api.py +++ b/py_modules/adapters/romm/romm_api.py @@ -101,9 +101,8 @@ def get_version(self) -> str | None: # ── Server / Auth ───────────────────────────────────────────────── - # Fast-fail reachability probe: a single ~3s attempt, no retry. Keeps the - # launch gate's "offline" verdict snappy instead of waiting through the - # retrying heartbeat (3 attempts + up to ~90s of accumulated timeouts). + # Fast-fail point probes: a single ~3s attempt, no retry. Keeps verdicts + # snappy instead of waiting through 3 attempts and accumulated timeouts. _PROBE_TIMEOUT_SECONDS = 3 def heartbeat(self) -> dict[str, Any]: @@ -130,6 +129,10 @@ def get_current_user(self) -> dict[str, Any]: def get_rom(self, rom_id: int) -> dict[str, Any]: return self._client.request(f"/api/roms/{rom_id}") + def get_rom_once(self, rom_id: int) -> dict[str, Any]: + """Single-attempt, short-timeout exact-ROM read for liveness probes.""" + return self._client.request_once(f"/api/roms/{rom_id}", timeout=self._PROBE_TIMEOUT_SECONDS) + def list_roms(self, platform_id: int, limit: int = LIST_PAGE_SIZE, offset: int = 0) -> dict[str, Any]: return self._client.request( f"/api/roms?platform_ids={platform_id}&limit={limit}&offset={offset}{_LIST_AGGREGATIONS_DISABLED}" diff --git a/py_modules/services/artwork.py b/py_modules/services/artwork.py index fc11fb82..2e3861da 100644 --- a/py_modules/services/artwork.py +++ b/py_modules/services/artwork.py @@ -835,7 +835,7 @@ async def refresh_cover(self, rom_id: int) -> dict[str, Any]: if not rom: return { "success": False, - "reason": ErrorCode.SERVER_UNREACHABLE.value, + "reason": ErrorCode.NOT_FOUND.value, "message": "Could not fetch ROM from server", } diff --git a/py_modules/services/protocols/transport.py b/py_modules/services/protocols/transport.py index 1d064f7c..6c728dae 100644 --- a/py_modules/services/protocols/transport.py +++ b/py_modules/services/protocols/transport.py @@ -145,6 +145,10 @@ def get_rom(self, rom_id: int) -> dict[str, Any]: """ ... + def get_rom_once(self, rom_id: int) -> dict[str, Any]: + """Fetch a single ROM by ID once with the short probe timeout.""" + ... + def list_roms(self, platform_id: int, limit: int = LIST_PAGE_SIZE, offset: int = 0) -> dict[str, Any]: """List ROMs for a platform with pagination. diff --git a/py_modules/services/version_switch.py b/py_modules/services/version_switch.py index 0697c5bc..61bb1122 100644 --- a/py_modules/services/version_switch.py +++ b/py_modules/services/version_switch.py @@ -4,9 +4,10 @@ ``get_version_list`` reports every version of a sibling group — the local rows (rich version dimensions) merged with the server's live ``sibling_roms`` view (slim stubs for versions not yet synced) — with the active / downloaded / default -markers the picker paints. ``switch_version`` moves the group's Steam-shortcut -binding to a chosen sibling (the active version), persisting a server-only target -first when needed. +markers the picker paints. Retained local rows are checked for ephemeral liveness +on each read; only an exact-ROM 404 marks one vanished. ``switch_version`` moves +the group's Steam-shortcut binding to a chosen sibling (the active version), +persisting a server-only target first when needed. A version switch is a pure binding move: the shortcut's name and appId stay sticky (ADR-0021 §2) and no save state migrates (ADR-0021 §4). Switching a @@ -158,12 +159,13 @@ def __init__(self, *, config: VersionSwitchServiceConfig) -> None: async def get_version_list(self, app_id: int) -> dict[str, Any]: """Report the version picker's state for the group bound to ``app_id``. - Returns ``{"multi_version": False}`` when the appId is unknown/unbound or - the group has a single version (the frontend renders no picker). Otherwise - ``{"multi_version": True, "versions": [...], "server_query_failed": bool}`` - — one entry per version with ``rom_id`` / ``label`` / ``name`` / the - version dimensions and the ``synced`` / ``installed`` / ``active`` / - ``is_default`` / ``switchable`` markers. Local rows always appear; the + Returns ``multi_version: False`` when the appId is unknown/unbound or the + group has a single version (the frontend renders no picker). Every result + carries ``bound_vanished``; every known-group result also carries the + server-query verdict, including the single-row shape. A multi-version + result has one entry per version with ``rom_id`` / ``label`` / ``name`` / + the version dimensions and the ``synced`` / ``installed`` / ``active`` / + ``is_default`` / ``switchable`` / ``vanished`` markers. Local rows always appear; the server's ``sibling_roms`` add any not-yet-synced versions (``synced: False``). ``switchable`` is the same membership authority ``switch_version`` decides by (:func:`target_in_sibling_group`): a RomM @@ -171,8 +173,12 @@ async def get_version_list(self, app_id: int) -> dict[str, Any]: under a different group key (#1359) or never synced but carrying a *different* id at the bound group's canonical source (#1360) — is listed but ``switchable: False``, so the picker disables it instead of offering a - switch the backend would reject. When the server view can't be - fetched the local-only list is returned with the additive + switch the backend would reject. Liveness is recomputed per call: the + successful bound detail makes that id live, local ids in its direct + sibling view are live, and absent local ids are exact-id probed once on + the executor. Only :class:`RommNotFoundError` sets ``vanished``; every + other probe outcome fails open. When the server view can't be fetched the + local-only list is returned with the additive ``server_query_failed: True`` flag (partial-success carve-out) — the picker still works over what's synced. A definitive 404 on the bound id is NOT such a failure: the server answered, it just no longer has that ROM, so @@ -182,7 +188,7 @@ async def get_version_list(self, app_id: int) -> dict[str, Any]: app_id = int(app_id) local = await self._loop.run_in_executor(None, self._read_local_group, app_id) if local is None: - return {"multi_version": False} + return {"multi_version": False, "server_query_failed": False, "bound_vanished": False} try: bound_detail = await self._loop.run_in_executor(None, self._romm_api.get_rom, local.bound_rom_id) @@ -192,19 +198,46 @@ async def get_version_list(self, app_id: int) -> dict[str, Any]: # list must not raise the ``server_query_failed`` flag the picker # funnels into the global connection store (#1570). self._logger.warning(f"Version list: bound rom {local.bound_rom_id} is gone from the server") + vanished_ids = {local.bound_rom_id} + vanished_ids.update(await self._probe_vanished(local.member_ids - {local.bound_rom_id})) return self._build_version_list( - local, server_only_stubs=[], detail_by_id={}, stub_local_group_keys={}, server_query_failed=False + local, + server_only_stubs=[], + detail_by_id={}, + stub_local_group_keys={}, + vanished_ids=vanished_ids, + bound_vanished=True, + server_query_failed=False, ) except Exception as e: # transport failure degrades to a local-only list self._logger.warning(f"Version list: sibling fetch failed for rom {local.bound_rom_id}: {e}") return self._build_version_list( - local, server_only_stubs=[], detail_by_id={}, stub_local_group_keys={}, server_query_failed=True + local, + server_only_stubs=[], + detail_by_id={}, + stub_local_group_keys={}, + vanished_ids=set(), + bound_vanished=False, + server_query_failed=True, ) - stubs = bound_detail.get("sibling_roms") or [] - server_only_stubs = [ - s for s in stubs if int(s.get("id", 0)) not in local.member_ids and int(s.get("id", 0)) > 0 - ] + bound_payload: Any = bound_detail + raw_stubs = bound_payload.get("sibling_roms") if isinstance(bound_payload, dict) else None + stubs: list[dict[str, Any]] = [] + if isinstance(raw_stubs, list): + for stub in raw_stubs: + if not isinstance(stub, dict): + continue + try: + rom_id = int(stub.get("id", 0)) + except (TypeError, ValueError): + continue + if rom_id > 0: + stubs.append(stub) + direct_sibling_ids = {int(s["id"]) for s in stubs} + suspect_ids = local.member_ids - direct_sibling_ids - {local.bound_rom_id} + vanished_ids = await self._probe_vanished(suspect_ids) + server_only_stubs = [s for s in stubs if int(s["id"]) not in local.member_ids] detail_by_id = await self._fetch_stub_details(server_only_stubs) stub_local_group_keys = await self._loop.run_in_executor( None, self._read_stub_local_group_keys, [int(s["id"]) for s in server_only_stubs] @@ -214,9 +247,31 @@ async def get_version_list(self, app_id: int) -> dict[str, Any]: server_only_stubs=server_only_stubs, detail_by_id=detail_by_id, stub_local_group_keys=stub_local_group_keys, + vanished_ids=vanished_ids, + bound_vanished=False, server_query_failed=False, ) + async def _probe_vanished(self, rom_ids: set[int]) -> set[int]: + """Return ids whose single-attempt exact-ROM probe produced a typed 404.""" + ordered_ids = sorted(rom_ids) + if not ordered_ids: + return set() + verdicts = await asyncio.gather( + *(self._loop.run_in_executor(None, self._probe_rom_vanished, rom_id) for rom_id in ordered_ids) + ) + return {rom_id for rom_id, vanished in zip(ordered_ids, verdicts, strict=True) if vanished} + + def _probe_rom_vanished(self, rom_id: int) -> bool: + """Probe one exact id once; every outcome except a typed 404 fails open.""" + try: + self._romm_api.get_rom_once(rom_id) + except RommNotFoundError: + return True + except Exception as e: + self._logger.warning(f"Version list: liveness probe failed open for rom {rom_id}: {e}") + return False + async def _fetch_stub_details(self, stubs: list[dict[str, Any]]) -> dict[int, dict[str, Any]]: """Fetch each server-only sibling's full dims concurrently (best-effort). @@ -306,6 +361,8 @@ def _build_version_list( server_only_stubs: list[dict[str, Any]], detail_by_id: dict[int, dict[str, Any]], stub_local_group_keys: dict[int, str | None], + vanished_ids: set[int], + bound_vanished: bool, server_query_failed: bool, ) -> dict[str, Any]: """Merge local rows + server-only stubs into the picker's version list. @@ -315,8 +372,10 @@ def _build_version_list( one is available (else empty). Every entry carries ``switchable`` — the single authority (:func:`target_in_sibling_group`) that ``switch_version`` also decides by, so a listed row the switch would reject is rendered - non-switchable rather than a dead-end. A single-version group renders no - picker. + non-switchable rather than a dead-end. ``vanished`` is independent of + membership: retained rows keep their switchable verdict but are excluded + from default ranking and disabled by the frontend. A single-version group + renders no picker while preserving its bound/server verdicts. """ entries: list[dict[str, Any]] = [ { @@ -329,6 +388,7 @@ def _build_version_list( "tags": m.tags, "synced": True, "installed": m.installed, + "vanished": m.rom_id in vanished_ids, # Members share the bound key by construction (iter_by_group_key), # so they are always switchable — routed through the shared # predicate to keep a single authority. @@ -346,14 +406,23 @@ def _build_version_list( ] if len(entries) <= 1: - return {"multi_version": False} + return { + "multi_version": False, + "server_query_failed": server_query_failed, + "bound_vanished": bound_vanished, + } - switchable_ids = {e["rom_id"] for e in entries if e["switchable"]} - default_rom_id = self._resolve_default(local, server_only_stubs, detail_by_id, switchable_ids) + default_candidate_ids = {e["rom_id"] for e in entries if e["switchable"] and not e["vanished"]} + default_rom_id = self._resolve_default(local, server_only_stubs, detail_by_id, default_candidate_ids) for e in entries: e["active"] = e["rom_id"] == local.bound_rom_id e["is_default"] = e["rom_id"] == default_rom_id - return {"multi_version": True, "versions": entries, "server_query_failed": server_query_failed} + return { + "multi_version": True, + "versions": entries, + "server_query_failed": server_query_failed, + "bound_vanished": bound_vanished, + } def _server_only_entry( self, @@ -385,6 +454,9 @@ def _server_only_entry( "tags": list(meta.get("tags") or []), "synced": False, "installed": False, + # Presence in the bound ROM's current direct sibling response is a + # positive server statement, so a server-only stub is live. + "vanished": False, "switchable": target_in_sibling_group( bound_group_key=local.group_key, target_group_key=stub_local_group_keys.get(rom_id) if target_is_local else None, @@ -399,7 +471,7 @@ def _resolve_default( local: _LocalGroup, server_only_stubs: list[dict[str, Any]], detail_by_id: dict[int, dict[str, Any]], - switchable_ids: set[int], + candidate_ids: set[int], ) -> int | None: """The version the resolution chain would pick as the group's default. @@ -407,12 +479,12 @@ def _resolve_default( filters, so the badge marks the *natural* default — RomM's ``is_main_sibling`` else the 1G1R + preferred-region ranking — independent of which version is currently bound (marking that would be circular with - the ``active`` flag). Ranks over the group's *switchable* versions only - (``switchable_ids``): a cross-group RomM sibling is not a real member here, - so it must not win the Default badge. A server-only stub with no fetched - detail ranks with only its label. Returns ``None`` if the chain somehow - can't decide (never, for a non-empty group) so the badge is simply absent - rather than the call failing. + the ``active`` flag). Ranks only over the caller's candidates: switchable, + live versions. A cross-group RomM sibling and a retained vanished row must + not win the Default badge. A server-only stub with no fetched detail ranks + with only its label. Returns ``None`` when no candidate remains or the + chain cannot decide, so the badge is simply absent rather than the call + failing. """ ranking: list[dict[str, Any]] = [ { @@ -424,11 +496,11 @@ def _resolve_default( "fs_name_no_ext": m.label, } for m in local.members - if m.rom_id in switchable_ids + if m.rom_id in candidate_ids ] for stub in server_only_stubs: rom_id = int(stub["id"]) - if rom_id not in switchable_ids: + if rom_id not in candidate_ids: continue detail = detail_by_id.get(rom_id) meta = extract_version_metadata(detail) if detail is not None else {} diff --git a/src/api/backend.ts b/src/api/backend.ts index cf2d90ef..5ebf9596 100755 --- a/src/api/backend.ts +++ b/src/api/backend.ts @@ -420,7 +420,9 @@ export const selectDisc = callable<[number, string | null], SelectDiscResult>("s * this game's — a locally-synced ROM under a different group key (#1359) or a * not-yet-synced sibling carrying a different id at the group's canonical source * (#1360) — the picker lists it but disables the row, because switch_version would - * reject it; every other row is switchable (#1368). + * reject it; every other row is switchable (#1368). `vanished` is a separate, + * ephemeral liveness verdict: RomM answered 404 for that exact local id during + * this list load. The retained row stays visible but cannot be selected. */ export interface VersionInfo { rom_id: number; @@ -435,6 +437,7 @@ export interface VersionInfo { active: boolean; is_default: boolean; switchable: boolean; + vanished: boolean; } /** @@ -443,15 +446,17 @@ export interface VersionInfo { * the picker renders nothing. When `true`, `versions` lists every version * (local + server-only) with markers; `server_query_failed` is `true` when the * live `sibling_roms` view could not be fetched, so the list is local-only - * (partial-success carve-out). A 404 on the bound id is NOT such a failure — - * the server answered — so the local-only list comes back with - * `server_query_failed: false` and the picker leaves the connection state - * alone (#1570). + * (partial-success carve-out). `bound_vanished` is required even on a + * single-version/unknown result so a bound-id 404 is never lost when the picker + * itself does not render. A 404 on the bound id is NOT a query failure — the + * server answered — and the picker does not feed that entity verdict into the + * global connection state (#1570). */ export interface VersionList { multi_version: boolean; versions?: VersionInfo[]; server_query_failed?: boolean; + bound_vanished: boolean; } /** diff --git a/src/components/RomMPlaySection.test.tsx b/src/components/RomMPlaySection.test.tsx index 1e49c806..1da0e364 100644 --- a/src/components/RomMPlaySection.test.tsx +++ b/src/components/RomMPlaySection.test.tsx @@ -609,7 +609,7 @@ describe("RomMPlaySection", () => { tags: ["Demo"], }); // Single-version group → the compact switcher is absent too. - vi.mocked(backend.getVersionList).mockResolvedValue({ multi_version: false }); + vi.mocked(backend.getVersionList).mockResolvedValue({ multi_version: false, bound_vanished: false }); const { container } = render(); await flushAsync(); @@ -622,6 +622,7 @@ describe("RomMPlaySection", () => { vi.mocked(backend.getVersionList).mockResolvedValue({ multi_version: true, server_query_failed: false, + bound_vanished: false, versions: [ { rom_id: 1, @@ -636,6 +637,7 @@ describe("RomMPlaySection", () => { active: true, is_default: true, switchable: true, + vanished: false, }, { rom_id: 2, @@ -650,6 +652,7 @@ describe("RomMPlaySection", () => { active: false, is_default: false, switchable: true, + vanished: false, }, ], }); @@ -672,7 +675,7 @@ describe("RomMPlaySection", () => { it("renders no version trigger for a single-version group", async () => { vi.mocked(cachedStore.getCachedGameDetail).mockResolvedValue({ found: true, rom_id: 12 }); - vi.mocked(backend.getVersionList).mockResolvedValue({ multi_version: false }); + vi.mocked(backend.getVersionList).mockResolvedValue({ multi_version: false, bound_vanished: false }); const { container } = render(); await flushAsync(); diff --git a/src/components/SavesTab.test.tsx b/src/components/SavesTab.test.tsx index a6380674..b87dcf9e 100644 --- a/src/components/SavesTab.test.tsx +++ b/src/components/SavesTab.test.tsx @@ -132,7 +132,7 @@ describe("SavesTab", () => { setRommConnectionState("connected"); // Stranded-version banner probes (#1298) — default to "no other versions" so // the banner is absent for the unrelated slot/legacy tests. - vi.mocked(backend.getVersionList).mockResolvedValue({ multi_version: false }); + vi.mocked(backend.getVersionList).mockResolvedValue({ multi_version: false, bound_vanished: false }); vi.mocked(backend.checkLocalDrift).mockResolvedValue({ drifted: false, rom_id: 0 }); installDomEventListenerSpy(); }); @@ -653,6 +653,7 @@ describe("SavesTab", () => { active: false, is_default: false, switchable: true, + vanished: false, ...overrides, }; } @@ -660,6 +661,7 @@ describe("SavesTab", () => { // Active = Japan (installed); an inactive USA build is also on disk. const listWithStranded: import("../api/backend").VersionList = { multi_version: true, + bound_vanished: false, versions: [ makeVersion({ rom_id: 2, label: "Game (Japan)", active: true, installed: true }), makeVersion({ rom_id: 5, label: "Game (USA)", active: false, installed: true }), @@ -668,6 +670,7 @@ describe("SavesTab", () => { // Active = Japan (installed); the other version is NOT downloaded. const listNoStranded: import("../api/backend").VersionList = { multi_version: true, + bound_vanished: false, versions: [ makeVersion({ rom_id: 2, label: "Game (Japan)", active: true, installed: true }), makeVersion({ rom_id: 5, label: "Game (USA)", active: false, installed: false }), @@ -724,6 +727,7 @@ describe("SavesTab", () => { // drifted. The banner must probe past USA and name Europe. vi.mocked(backend.getVersionList).mockResolvedValue({ multi_version: true, + bound_vanished: false, versions: [ makeVersion({ rom_id: 2, label: "Game (Japan)", active: true, installed: true }), makeVersion({ rom_id: 5, label: "Game (USA)", active: false, installed: true }), @@ -747,6 +751,30 @@ describe("SavesTab", () => { expect(container.textContent).not.toContain("Game (USA)"); }); + it("skips vanished installs and continues to a later live drifted version", async () => { + vi.mocked(backend.getVersionList).mockResolvedValue({ + multi_version: true, + bound_vanished: false, + versions: [ + makeVersion({ rom_id: 2, label: "Game (Japan)", active: true, installed: true }), + makeVersion({ rom_id: 5, label: "Gone (USA)", installed: true, vanished: true }), + makeVersion({ rom_id: 6, label: "Game (Europe)", installed: true }), + ], + }); + vi.mocked(backend.checkLocalDrift).mockResolvedValue({ drifted: true, rom_id: 6 }); + + const { container } = render(); + + await waitFor(() => + expect(container.textContent).toContain( + 'Version "Game (Europe)" has saves that were never uploaded — switch back to sync them.', + ), + ); + expect(backend.checkLocalDrift).not.toHaveBeenCalledWith(5); + expect(backend.checkLocalDrift).toHaveBeenCalledWith(6); + expect(container.textContent).not.toContain("Gone (USA)"); + }); + it("logs a warning and hides the banner when the version-list probe rejects", async () => { const logWarnSpy = vi.spyOn(backend, "logWarn").mockImplementation(() => {}); try { diff --git a/src/components/SavesTab.tsx b/src/components/SavesTab.tsx index 938c6fdd..553acbb1 100644 --- a/src/components/SavesTab.tsx +++ b/src/components/SavesTab.tsx @@ -102,7 +102,7 @@ export const SavesTab: FC = ({ const list = await getVersionList(appId); if (isCancelled()) return; const inactiveInstalled = list.multi_version - ? (list.versions ?? []).filter((v) => v.installed && !v.active) + ? (list.versions ?? []).filter((v) => v.installed && !v.active && !v.vanished) : []; // Probe every inactive-installed sibling (any of them can hold stranded // saves), and surface the first one that actually drifted. diff --git a/src/components/VersionPicker.test.tsx b/src/components/VersionPicker.test.tsx index 4a60b795..50ef1a6d 100644 --- a/src/components/VersionPicker.test.tsx +++ b/src/components/VersionPicker.test.tsx @@ -15,6 +15,7 @@ import { toaster } from "@decky/api"; import { VersionPicker } from "./VersionPicker"; import * as backend from "../api/backend"; import type { VersionList } from "../api/backend"; +import { getRommConnectionState, setRommConnectionState } from "../utils/connectionState"; import { emitDeckyEvent } from "../test-utils/decky-api-mock"; import type { DownloadCompleteEvent } from "../types"; import { @@ -69,6 +70,7 @@ function multiVersionList(overrides: Partial = {}): VersionList { return { multi_version: true, server_query_failed: false, + bound_vanished: false, versions: [ { rom_id: 1, @@ -83,6 +85,7 @@ function multiVersionList(overrides: Partial = {}): VersionList { active: true, is_default: false, switchable: true, + vanished: false, }, { rom_id: 2, @@ -97,6 +100,7 @@ function multiVersionList(overrides: Partial = {}): VersionList { active: false, is_default: true, switchable: true, + vanished: false, }, { rom_id: 3, @@ -111,6 +115,7 @@ function multiVersionList(overrides: Partial = {}): VersionList { active: false, is_default: false, switchable: true, + vanished: false, }, ], ...overrides, @@ -136,7 +141,7 @@ describe("VersionPicker — render gate", () => { }); it("renders nothing for a single-version group (multi_version:false)", async () => { - vi.mocked(backend.getVersionList).mockResolvedValue({ multi_version: false }); + vi.mocked(backend.getVersionList).mockResolvedValue({ multi_version: false, bound_vanished: false }); const { container } = render(); @@ -212,6 +217,7 @@ describe("VersionPicker — non-switchable rows (#1359)", () => { active: true, is_default: true, switchable: true, + vanished: false, }, { rom_id: 5, @@ -226,6 +232,7 @@ describe("VersionPicker — non-switchable rows (#1359)", () => { active: false, is_default: false, switchable: false, + vanished: false, }, ], }); @@ -261,6 +268,101 @@ describe("VersionPicker — non-switchable rows (#1359)", () => { }); }); +describe("VersionPicker — vanished retained rows (#1570)", () => { + beforeEach(() => { + captured.menu = null; + vi.mocked(backend.getVersionList).mockReset(); + vi.mocked(backend.switchVersion).mockReset(); + vi.mocked(backend.fetchCoverBase64).mockResolvedValue({ base64: null }); + vi.mocked(toaster.toast).mockReset(); + }); + + function listWithBoundVanished(): VersionList { + const versions = (multiVersionList().versions ?? []).slice(0, 2).map((v) => ({ ...v })); + versions[0] = { ...versions[0]!, active: true, installed: true, is_default: false, vanished: true }; + versions[1] = { ...versions[1]!, active: false, is_default: true, vanished: false }; + return multiVersionList({ bound_vanished: true, versions }); + } + + it("keeps a vanished active row visible, dimmed, disabled, labelled, and marked", async () => { + vi.mocked(backend.getVersionList).mockResolvedValue(listWithBoundVanished()); + + const { menu } = await renderAndOpen(); + + const items = within(menu.container).getAllByRole("menuitem"); + const vanishedRow = items.find((i) => i.textContent.includes("Game (USA)")); + expect(vanishedRow).toBeTruthy(); + expect(vanishedRow?.getAttribute("aria-disabled")).toBe("true"); + expect(vanishedRow?.textContent).toContain("No longer available on RomM"); + expect(vanishedRow?.textContent).toContain("Downloaded"); + expect(vanishedRow?.textContent).toContain("✓"); + expect((vanishedRow?.firstElementChild as HTMLElement | null)?.style.opacity).toBe("0.55"); + // The live recovery target remains selectable. + const liveRow = items.find((i) => i.textContent.includes("Game (Japan)")); + expect(liveRow?.getAttribute("aria-disabled")).toBeNull(); + }); + + it("clicking an inactive vanished row is inert in code", async () => { + const list = listWithBoundVanished(); + list.bound_vanished = false; + list.versions = (list.versions ?? []).map((v) => + v.rom_id === 1 ? { ...v, active: false } : { ...v, active: true }, + ); + vi.mocked(backend.getVersionList).mockResolvedValue(list); + + const { menu } = await renderAndOpen(); + await clickRow(menu.container, "Game (USA)"); + + expect(backend.switchVersion).not.toHaveBeenCalled(); + expect(toaster.toast).not.toHaveBeenCalled(); + }); + + it("allows recovery by selecting a live alternative to the vanished binding", async () => { + vi.mocked(backend.getVersionList).mockResolvedValue(listWithBoundVanished()); + vi.mocked(backend.switchVersion).mockResolvedValue({ + success: false, + reason: "bound_elsewhere", + message: "test stop", + }); + + const { menu } = await renderAndOpen(); + await clickRow(menu.container, "Game (Japan)"); + + expect(backend.switchVersion).toHaveBeenCalledWith(APP_ID, 2, false); + }); +}); + +describe("VersionPicker — liveness connection signals (#1570)", () => { + beforeEach(() => { + captured.menu = null; + vi.mocked(backend.getVersionList).mockReset(); + vi.mocked(backend.fetchCoverBase64).mockResolvedValue({ base64: null }); + setRommConnectionState("checking"); + }); + + afterEach(() => setRommConnectionState("checking")); + + it("does not feed a bound-id 404 into the global connection store", async () => { + vi.mocked(backend.getVersionList).mockResolvedValue( + multiVersionList({ bound_vanished: true, server_query_failed: false }), + ); + + render(); + await waitFor(() => expect(backend.getVersionList).toHaveBeenCalledWith(APP_ID)); + + expect(getRommConnectionState()).toBe("checking"); + }); + + it("preserves the explicit server-unreachable feed", async () => { + vi.mocked(backend.getVersionList).mockResolvedValue( + multiVersionList({ bound_vanished: false, server_query_failed: true }), + ); + + render(); + await waitFor(() => expect(getRommConnectionState()).toBe("offline")); + }); +}); + describe("VersionPicker — per-version covers (#1346)", () => { beforeEach(() => { captured.menu = null; diff --git a/src/components/VersionPicker.tsx b/src/components/VersionPicker.tsx index 31456c77..3f778966 100644 --- a/src/components/VersionPicker.tsx +++ b/src/components/VersionPicker.tsx @@ -104,11 +104,13 @@ export const VersionPicker: FC = ({ appId }) => { const result = await getVersionList(appId); // get_version_list touches the server for the sibling view (#1345): an // explicit server_query_failed means offline; a multi-version list that - // loaded without failure proves the server is reachable. A single/unbound - // group carries no reachability signal. + // loaded without failure proves the server is reachable. A bound-id 404 + // is an entity verdict, not a connection signal, so it feeds neither + // direction into the global store. A single/unbound group carries no + // reachability signal. if (result.server_query_failed) { reportServerReachable(false); - } else if (result.multi_version) { + } else if (result.multi_version && !result.bound_vanished) { reportServerReachable(true); } memberIdsRef.current = new Set((result.versions ?? []).map((v) => v.rom_id)); @@ -276,7 +278,7 @@ export const VersionPicker: FC = ({ appId }) => { // guard once the fresh list lands — so the trigger never re-enables against a // stale list. const handleSwitch = async (target: VersionInfo): Promise => { - if (target.active) return; + if (target.active || target.vanished) return; // A non-switchable row is a RomM sibling that lives in a different local group // (#1359) — its row is rendered disabled, and this guard makes a click a no-op // (defense-in-depth), so switch_version's rejection can never reach a toast. @@ -361,17 +363,16 @@ export const VersionPicker: FC = ({ appId }) => { showContextMenu( {versions.map((v) => ( - detach(handleSwitch(v))}> + detach(handleSwitch(v))}> {rowCover(v)} @@ -379,7 +380,11 @@ export const VersionPicker: FC = ({ appId }) => { {v.is_default ? : null} {v.installed ? : null} {v.switchable && !v.synced ? : null} - {v.switchable ? null : ( + {v.vanished ? ( + + No longer available on RomM + + ) : v.switchable ? null : ( conflicting metadata match in RomM diff --git a/tests/adapters/romm/test_romm_api.py b/tests/adapters/romm/test_romm_api.py index a45ed5b1..a49c330f 100644 --- a/tests/adapters/romm/test_romm_api.py +++ b/tests/adapters/romm/test_romm_api.py @@ -78,6 +78,18 @@ def test_calls_rom_endpoint(self): assert result["id"] == 42 +class TestGetRomOnce: + def test_uses_single_attempt_short_timeout_request(self): + api, client = _make_api() + client.request_once.return_value = {"id": 42, "name": "Zelda"} + + result = api.get_rom_once(42) + + client.request_once.assert_called_once_with("/api/roms/42", timeout=3) + client.request.assert_not_called() + assert result == {"id": 42, "name": "Zelda"} + + class TestListRoms: def test_includes_platform_id_and_pagination(self): api, client = _make_api() diff --git a/tests/contract/test_fetch_cover.py b/tests/contract/test_fetch_cover.py index f7f5e256..b0db8b41 100644 --- a/tests/contract/test_fetch_cover.py +++ b/tests/contract/test_fetch_cover.py @@ -14,6 +14,8 @@ import base64 +from lib.errors import RommNotFoundError + async def test_fetch_cover_base64_downloads_and_returns_bytes(harness): """A cache miss fetches the ROM's cover from RomM and returns its bytes.""" @@ -56,6 +58,16 @@ async def test_fetch_cover_base64_server_unreachable_returns_null(harness): assert "success" not in result and "reason" not in result +async def test_fetch_cover_base64_rom_404_returns_null_data_shape(harness): + """Artwork remains a data query, not a ROM-liveness authority.""" + harness.romm.get_rom_side_effect = RommNotFoundError("HTTP 404: Not Found") + + result = await harness.plugin.fetch_cover_base64(7) + + assert result == {"base64": None} + assert "success" not in result and "reason" not in result + + async def test_fetch_cover_base64_rom_without_cover_returns_null(harness): """A ROM with no cover URL returns ``{"base64": None}`` silently.""" harness.romm.roms[7] = {"id": 7, "name": "No Cover"} diff --git a/tests/contract/test_version_picker.py b/tests/contract/test_version_picker.py index d94055eb..4bbcbb49 100644 --- a/tests/contract/test_version_picker.py +++ b/tests/contract/test_version_picker.py @@ -21,6 +21,7 @@ from domain.rom import Rom from domain.rom_install import RomInstall from domain.rom_save_sync_state import RomSaveSyncState +from lib.errors import RommNotFoundError from ._seed import seed_group_member @@ -81,6 +82,7 @@ async def test_get_version_list_happy_shape(harness): result = await harness.plugin.get_version_list(_APP_ID) assert result["multi_version"] is True assert result["server_query_failed"] is False + assert result["bound_vanished"] is False by_id = {v["rom_id"]: v for v in result["versions"]} assert set(by_id) == {1, 2} assert by_id[1]["active"] is True @@ -90,6 +92,22 @@ async def test_get_version_list_happy_shape(harness): assert by_id[1]["synced"] is True and by_id[2]["synced"] is True # Both local members share the bound key, so both are switchable. assert by_id[1]["switchable"] is True and by_id[2]["switchable"] is True + assert by_id[1]["vanished"] is False and by_id[2]["vanished"] is False + assert set(by_id[1]) == { + "rom_id", + "name", + "label", + "regions", + "languages", + "revision", + "tags", + "synced", + "installed", + "switchable", + "vanished", + "active", + "is_default", + } async def test_get_version_list_cross_group_sibling_not_switchable(harness): @@ -213,13 +231,21 @@ async def test_get_version_list_solo_group_not_multi(harness): _seed_rom(harness, rom_id=1, app_id=_APP_ID) harness.romm.roms[1] = {"id": 1, "sibling_roms": []} result = await harness.plugin.get_version_list(_APP_ID) - assert result == {"multi_version": False} + assert result == { + "multi_version": False, + "server_query_failed": False, + "bound_vanished": False, + } async def test_get_version_list_unknown_app_not_multi(harness): """An unknown / unbound appId renders no picker.""" result = await harness.plugin.get_version_list(999) - assert result == {"multi_version": False} + assert result == { + "multi_version": False, + "server_query_failed": False, + "bound_vanished": False, + } async def test_get_version_list_server_fail_partial_shape(harness): @@ -231,7 +257,42 @@ async def test_get_version_list_server_fail_partial_shape(harness): result = await harness.plugin.get_version_list(_APP_ID) assert result["multi_version"] is True assert result["server_query_failed"] is True + assert result["bound_vanished"] is False assert {v["rom_id"] for v in result["versions"]} == {1, 2} + assert all(v["vanished"] is False for v in result["versions"]) + + +async def test_get_version_list_bound_404_mixed_local_liveness_shape(harness): + """A bound 404 is explicit list state; each other local id gets its own verdict.""" + _seed_rom(harness, rom_id=1, app_id=_APP_ID) + _seed_rom(harness, rom_id=2, app_id=None) + _seed_rom(harness, rom_id=3, app_id=None) + harness.romm.get_rom_side_effect = RommNotFoundError("bound gone") + harness.romm.get_rom_once_side_effect_by_id[2] = RommNotFoundError("sibling gone") + harness.romm.roms[3] = {"id": 3} + + result = await harness.plugin.get_version_list(_APP_ID) + + assert set(result) == {"multi_version", "versions", "server_query_failed", "bound_vanished"} + assert result["multi_version"] is True + assert result["server_query_failed"] is False + assert result["bound_vanished"] is True + assert {v["rom_id"]: v["vanished"] for v in result["versions"]} == {1: True, 2: True, 3: False} + assert {v["rom_id"]: v["switchable"] for v in result["versions"]} == {1: True, 2: True, 3: True} + + +async def test_get_version_list_single_bound_404_preserves_non_multi_verdict(harness): + """The no-picker shape must not silently discard a definitive bound-id 404.""" + _seed_rom(harness, rom_id=1, app_id=_APP_ID) + harness.romm.get_rom_side_effect = RommNotFoundError("bound gone") + + result = await harness.plugin.get_version_list(_APP_ID) + + assert result == { + "multi_version": False, + "server_query_failed": False, + "bound_vanished": True, + } # ── switch_version ─────────────────────────────────────────────────────── diff --git a/tests/fakes/fake_romm_api.py b/tests/fakes/fake_romm_api.py index 25cda875..90c02a8c 100644 --- a/tests/fakes/fake_romm_api.py +++ b/tests/fakes/fake_romm_api.py @@ -139,6 +139,8 @@ def __init__(self) -> None: self.get_firmware_side_effect: Exception | None = None self.download_firmware_side_effect: Exception | None = None self.get_rom_side_effect: Exception | None = None + self.get_rom_once_side_effect: Exception | None = None + self.get_rom_once_side_effect_by_id: dict[int, Exception] = {} self.list_roms_side_effect: Exception | None = None self.list_roms_updated_after_side_effect: Exception | None = None self.list_collections_side_effect: Exception | None = None @@ -280,6 +282,18 @@ def get_rom(self, rom_id: int) -> dict[str, Any]: return {"id": rom_id} return dict(rom) + def get_rom_once(self, rom_id: int) -> dict[str, Any]: + """Single-attempt exact-ROM fake with per-id failure injection.""" + self._log("get_rom_once", (rom_id,)) + self._check_fail(self.get_rom_once_side_effect) + per_id = self.get_rom_once_side_effect_by_id.get(rom_id) + if per_id is not None: + raise per_id + rom = self.roms.get(rom_id) + if rom is None: + return {"id": rom_id} + return dict(rom) + def _paginate(self, items: list[dict[str, Any]], limit: int, offset: int) -> dict[str, Any]: sliced = items[offset : offset + limit] return {"items": [dict(r) for r in sliced], "total": len(items)} diff --git a/tests/fakes/fake_save_api.py b/tests/fakes/fake_save_api.py index ff856b21..d852e9b5 100644 --- a/tests/fakes/fake_save_api.py +++ b/tests/fakes/fake_save_api.py @@ -314,6 +314,9 @@ def get_current_user(self) -> dict[str, Any]: def get_rom(self, rom_id: int) -> dict[str, Any]: raise NotImplementedError + def get_rom_once(self, rom_id: int) -> dict[str, Any]: + raise NotImplementedError + def list_roms(self, platform_id: int, limit: int = 50, offset: int = 0) -> dict[str, Any]: raise NotImplementedError diff --git a/tests/fakes/test_fake_romm_api.py b/tests/fakes/test_fake_romm_api.py index aaefe767..99571916 100644 --- a/tests/fakes/test_fake_romm_api.py +++ b/tests/fakes/test_fake_romm_api.py @@ -124,6 +124,22 @@ def test_get_rom_falls_back_to_id_only_for_unknown(self) -> None: api = FakeRommApi() assert api.get_rom(42) == {"id": 42} + def test_get_rom_once_has_a_distinct_observable_call(self) -> None: + api = FakeRommApi() + api.roms[42] = {"id": 42, "name": "Zelda"} + + assert api.get_rom_once(42) == {"id": 42, "name": "Zelda"} + assert api.call_log == [("get_rom_once", (42,), {})] + + def test_get_rom_once_supports_per_id_mixed_failures(self) -> None: + api = FakeRommApi() + api.get_rom_once_side_effect_by_id[41] = OSError("missing") + api.roms[42] = {"id": 42} + + with pytest.raises(OSError, match="missing"): + api.get_rom_once(41) + assert api.get_rom_once(42) == {"id": 42} + def test_list_play_sessions_returns_seeded_history(self) -> None: api = FakeRommApi() api.play_sessions = {1: [{"id": 100, "rom_id": 1, "duration_ms": 900}]} diff --git a/tests/services/test_artwork.py b/tests/services/test_artwork.py index 94e35490..7626f333 100644 --- a/tests/services/test_artwork.py +++ b/tests/services/test_artwork.py @@ -1737,7 +1737,7 @@ async def test_not_found_when_get_rom_404s( romm_api.download_cover.assert_not_called() @pytest.mark.asyncio - async def test_server_unreachable_when_get_rom_returns_none( + async def test_not_found_when_get_rom_returns_none( self, artwork_service, uow, @@ -1751,7 +1751,7 @@ async def test_server_unreachable_when_get_rom_returns_none( result = await artwork_service.refresh_cover(42) assert result["success"] is False - assert result["reason"] == "server_unreachable" + assert result["reason"] == "not_found" @pytest.mark.asyncio async def test_no_cover_url_in_rom_payload( diff --git a/tests/services/test_version_switch.py b/tests/services/test_version_switch.py index f2a9568c..ea68c13c 100644 --- a/tests/services/test_version_switch.py +++ b/tests/services/test_version_switch.py @@ -4,6 +4,7 @@ import asyncio import logging +import threading from typing import Any import pytest @@ -13,7 +14,7 @@ from domain.rom import Rom from domain.rom_install import RomInstall -from lib.errors import RommNotFoundError +from lib.errors import RommAuthError, RommNotFoundError, RommServerError, RommTimeoutError from services.version_switch import VersionSwitchService, VersionSwitchServiceConfig _GROUP = "igdb:100:57" @@ -191,12 +192,20 @@ def _run(loop, coro): class TestGetVersionList: def test_unknown_app_id_not_multi(self, event_loop, service): - assert _run(event_loop, service.get_version_list(999)) == {"multi_version": False} + assert _run(event_loop, service.get_version_list(999)) == { + "multi_version": False, + "server_query_failed": False, + "bound_vanished": False, + } def test_solo_group_not_multi(self, event_loop, service, uow, romm): _seed_rom(uow, rom_id=1, app_id=_APP_ID) romm.roms[1] = {"id": 1, "sibling_roms": []} - assert _run(event_loop, service.get_version_list(_APP_ID)) == {"multi_version": False} + assert _run(event_loop, service.get_version_list(_APP_ID)) == { + "multi_version": False, + "server_query_failed": False, + "bound_vanished": False, + } def test_local_members_listed_with_markers(self, event_loop, service, uow, romm): _seed_rom(uow, rom_id=1, app_id=_APP_ID, regions=("USA",)) @@ -206,10 +215,12 @@ def test_local_members_listed_with_markers(self, event_loop, service, uow, romm) result = _run(event_loop, service.get_version_list(_APP_ID)) assert result["multi_version"] is True assert result["server_query_failed"] is False + assert result["bound_vanished"] is False by_id = {v["rom_id"]: v for v in result["versions"]} assert by_id[1]["active"] is True assert by_id[2]["active"] is False assert by_id[1]["synced"] is True and by_id[2]["synced"] is True + assert by_id[1]["vanished"] is False and by_id[2]["vanished"] is False # Default badge ignores the current binding → the is_main_sibling wins. assert by_id[2]["is_default"] is True assert by_id[1]["is_default"] is False @@ -225,6 +236,7 @@ def test_server_only_stub_marked_unsynced(self, event_loop, service, uow, romm): result = _run(event_loop, service.get_version_list(_APP_ID)) by_id = {v["rom_id"]: v for v in result["versions"]} assert by_id[5]["synced"] is False + assert by_id[5]["vanished"] is False assert by_id[5]["label"] == "Game (Japan)" assert by_id[5]["regions"] == ["Japan"] @@ -351,9 +363,11 @@ def test_server_unreachable_degrades_to_local_only(self, event_loop, service, uo result = _run(event_loop, service.get_version_list(_APP_ID)) assert result["multi_version"] is True assert result["server_query_failed"] is True + assert result["bound_vanished"] is False assert {v["rom_id"] for v in result["versions"]} == {1, 2} + assert all(v["vanished"] is False for v in result["versions"]) - def test_bound_rom_404_degrades_without_claiming_offline(self, event_loop, service, uow, romm): + def test_bound_rom_404_marks_only_bound_without_claiming_offline(self, event_loop, service, uow, romm): """A 404 on the bound id must NOT raise server_query_failed (#1570). The picker funnels that flag into the GLOBAL connection store, so a @@ -367,7 +381,172 @@ def test_bound_rom_404_degrades_without_claiming_offline(self, event_loop, servi result = _run(event_loop, service.get_version_list(_APP_ID)) assert result["multi_version"] is True assert result["server_query_failed"] is False - assert {v["rom_id"] for v in result["versions"]} == {1, 2} + assert result["bound_vanished"] is True + by_id = {v["rom_id"]: v for v in result["versions"]} + assert by_id[1]["vanished"] is True + assert by_id[2]["vanished"] is False + + def test_direct_local_sibling_is_live_and_only_absent_local_id_is_probed(self, event_loop, service, uow, romm): + _seed_rom(uow, rom_id=1, app_id=_APP_ID) + _seed_rom(uow, rom_id=2, app_id=None) + _seed_rom(uow, rom_id=3, app_id=None) + romm.roms[1] = {"id": 1, "sibling_roms": [{"id": 2}]} + romm.roms[3] = {"id": 3} + # If rom 2 were probed this would mark it vanished; direct presence is + # already a positive live statement and must suppress that probe. + romm.get_rom_once_side_effect_by_id[2] = RommNotFoundError("stale injected 404") + + result = _run(event_loop, service.get_version_list(_APP_ID)) + + by_id = {v["rom_id"]: v for v in result["versions"]} + assert by_id[2]["vanished"] is False + assert by_id[3]["vanished"] is False + probe_ids = [args[0] for name, args, _kwargs in romm.call_log if name == "get_rom_once"] + assert probe_ids == [3] + + def test_absent_transitive_member_exact_404_is_vanished(self, event_loop, service, uow, romm): + _seed_rom(uow, rom_id=1, app_id=_APP_ID) + _seed_rom(uow, rom_id=2, app_id=None) + romm.roms[1] = {"id": 1, "sibling_roms": []} + romm.get_rom_once_side_effect_by_id[2] = RommNotFoundError("HTTP 404: Not Found") + + result = _run(event_loop, service.get_version_list(_APP_ID)) + + by_id = {v["rom_id"]: v for v in result["versions"]} + assert by_id[2]["vanished"] is True + # Liveness does not rewrite sibling membership semantics. + assert by_id[2]["switchable"] is True + assert [args[0] for name, args, _kwargs in romm.call_log if name == "get_rom_once"] == [2] + + @pytest.mark.parametrize( + "failure", + [ + RommTimeoutError("timeout"), + RommAuthError("unauthorized"), + RommServerError("server error", status_code=503), + ConnectionError("transport"), + ], + ) + def test_non_404_probe_failures_fail_open(self, event_loop, service, uow, romm, failure): + _seed_rom(uow, rom_id=1, app_id=_APP_ID) + _seed_rom(uow, rom_id=2, app_id=None) + romm.roms[1] = {"id": 1, "sibling_roms": []} + romm.get_rom_once_side_effect_by_id[2] = failure + + result = _run(event_loop, service.get_version_list(_APP_ID)) + + by_id = {v["rom_id"]: v for v in result["versions"]} + assert by_id[2]["vanished"] is False + assert sum(1 for name, _args, _kwargs in romm.call_log if name == "get_rom_once") == 1 + + def test_falsy_exact_probe_response_is_live_fail_open(self, event_loop, service, uow, romm): + _seed_rom(uow, rom_id=1, app_id=_APP_ID) + _seed_rom(uow, rom_id=2, app_id=None) + romm.roms[1] = {"id": 1, "sibling_roms": []} + romm.roms[2] = {} + + result = _run(event_loop, service.get_version_list(_APP_ID)) + + by_id = {v["rom_id"]: v for v in result["versions"]} + assert by_id[2]["vanished"] is False + + def test_falsy_bound_200_fails_open_and_probes_other_local_ids(self, event_loop, service, uow, romm, monkeypatch): + _seed_rom(uow, rom_id=1, app_id=_APP_ID) + _seed_rom(uow, rom_id=2, app_id=None) + monkeypatch.setattr(romm, "get_rom", lambda _rom_id: None) + romm.roms[2] = {"id": 2} + + result = _run(event_loop, service.get_version_list(_APP_ID)) + + assert result["server_query_failed"] is False + assert result["bound_vanished"] is False + by_id = {v["rom_id"]: v for v in result["versions"]} + assert by_id[1]["vanished"] is False + assert by_id[2]["vanished"] is False + assert [args[0] for name, args, _kwargs in romm.call_log if name == "get_rom_once"] == [2] + + def test_bound_404_probes_every_other_local_member_and_allows_mixed_verdicts(self, event_loop, service, uow, romm): + _seed_rom(uow, rom_id=1, app_id=_APP_ID) + _seed_rom(uow, rom_id=2, app_id=None) + _seed_rom(uow, rom_id=3, app_id=None) + romm.get_rom_side_effect = RommNotFoundError("bound gone") + romm.get_rom_once_side_effect_by_id[2] = RommNotFoundError("sibling gone") + romm.roms[3] = {"id": 3} + + result = _run(event_loop, service.get_version_list(_APP_ID)) + + assert result["bound_vanished"] is True + assert result["server_query_failed"] is False + by_id = {v["rom_id"]: v for v in result["versions"]} + assert {rom_id: row["vanished"] for rom_id, row in by_id.items()} == {1: True, 2: True, 3: False} + probe_ids = [args[0] for name, args, _kwargs in romm.call_log if name == "get_rom_once"] + assert sorted(probe_ids) == [2, 3] + + def test_single_bound_404_preserves_bound_verdict_in_non_multi_shape(self, event_loop, service, uow, romm): + _seed_rom(uow, rom_id=1, app_id=_APP_ID) + romm.get_rom_side_effect = RommNotFoundError("bound gone") + + assert _run(event_loop, service.get_version_list(_APP_ID)) == { + "multi_version": False, + "server_query_failed": False, + "bound_vanished": True, + } + + def test_vanished_stale_main_sibling_never_receives_default_badge(self, event_loop, service, uow, romm): + _seed_rom(uow, rom_id=1, app_id=None, is_main_sibling=True) + _seed_rom(uow, rom_id=10, app_id=_APP_ID) + romm.roms[10] = {"id": 10, "sibling_roms": []} + romm.get_rom_once_side_effect_by_id[1] = RommNotFoundError("old id gone") + + by_id = {v["rom_id"]: v for v in _run(event_loop, service.get_version_list(_APP_ID))["versions"]} + + assert by_id[1]["vanished"] is True + assert by_id[1]["is_default"] is False + assert by_id[10]["is_default"] is True + + def test_vanished_lower_id_never_wins_metadata_tie(self, event_loop, service, uow, romm): + _seed_rom(uow, rom_id=1, app_id=None, name="Same", fs_name="same.sfc") + _seed_rom(uow, rom_id=10, app_id=_APP_ID, name="Same", fs_name="same.sfc") + romm.roms[10] = {"id": 10, "sibling_roms": []} + romm.get_rom_once_side_effect_by_id[1] = RommNotFoundError("old id gone") + + by_id = {v["rom_id"]: v for v in _run(event_loop, service.get_version_list(_APP_ID))["versions"]} + + assert by_id[1]["is_default"] is False + assert by_id[10]["is_default"] is True + + def test_liveness_is_recomputed_on_every_load(self, event_loop, service, uow, romm): + _seed_rom(uow, rom_id=1, app_id=_APP_ID) + _seed_rom(uow, rom_id=2, app_id=None) + romm.roms[1] = {"id": 1, "sibling_roms": []} + romm.get_rom_once_side_effect_by_id[2] = RommNotFoundError("temporarily gone") + + first = _run(event_loop, service.get_version_list(_APP_ID)) + assert {v["rom_id"]: v["vanished"] for v in first["versions"]}[2] is True + + romm.get_rom_once_side_effect_by_id.clear() + romm.roms[2] = {"id": 2} + second = _run(event_loop, service.get_version_list(_APP_ID)) + assert {v["rom_id"]: v["vanished"] for v in second["versions"]}[2] is False + assert sum(1 for name, _args, _kwargs in romm.call_log if name == "get_rom_once") == 2 + + def test_multiple_exact_probes_run_concurrently(self, event_loop, service, uow, romm, monkeypatch): + _seed_rom(uow, rom_id=1, app_id=_APP_ID) + _seed_rom(uow, rom_id=2, app_id=None) + _seed_rom(uow, rom_id=3, app_id=None) + romm.roms[1] = {"id": 1, "sibling_roms": []} + barrier = threading.Barrier(2) + + def get_rom_once(rom_id: int) -> dict[str, Any]: + barrier.wait(timeout=2) + return {"id": rom_id} + + monkeypatch.setattr(romm, "get_rom_once", get_rom_once) + + result = _run(event_loop, service.get_version_list(_APP_ID)) + + assert result["multi_version"] is True + assert all(v["vanished"] is False for v in result["versions"]) def test_preferred_region_heads_default(self, event_loop, service, uow, romm, settings): # No is_main_sibling → the default falls to the 1G1R region ranking, and From 891ce0548e7af0e3313966c8d9188763531d6bc7 Mon Sep 17 00:00:00 2001 From: danielcopper Date: Fri, 24 Jul 2026 15:54:41 +0200 Subject: [PATCH 2/3] docs(version-picker): clarify liveness probe selection --- docs/architecture/backend-architecture.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/docs/architecture/backend-architecture.md b/docs/architecture/backend-architecture.md index f835d9cd..b41a20c9 100644 --- a/docs/architecture/backend-architecture.md +++ b/docs/architecture/backend-architecture.md @@ -130,10 +130,11 @@ the rest are single modules. A service over ~700 LOC is the decomposition signal #### Version-list liveness `VersionSwitchService.get_version_list` separates retained local membership from current RomM availability. On each lazy -Game Page load, the bound detail request proves the bound id live and supplies the direct sibling ids; every other local -group member is exact-id probed through `RommRomReader.get_rom_once`, the adapter's single-attempt short-timeout -`request_once` path, concurrently on the existing executor fan-out. Only `RommNotFoundError` produces `vanished: true`. -All other probe failures and malformed/falsy responses fail open. No result is persisted. +Game Page load, the bound detail request proves the bound id live and supplies the direct sibling ids; only non-bound +local members absent from that direct sibling view are exact-id probed through `RommRomReader.get_rom_once`, the +adapter's single-attempt short-timeout `request_once` path, concurrently on the existing executor fan-out. Only +`RommNotFoundError` produces `vanished: true`. All other probe failures and malformed/falsy responses fail open. No +result is persisted. The response carries `vanished` on every version and `bound_vanished` even when `multi_version` is false. Retained vanished rows remain in the payload and keep the independently-computed `switchable` membership verdict, but are From 7b2021e8f98e4905afd988b45fb06be565bf4dc1 Mon Sep 17 00:00:00 2001 From: danielcopper Date: Fri, 24 Jul 2026 17:00:56 +0200 Subject: [PATCH 3/3] refactor(version-picker): simplify liveness handling --- py_modules/services/version_switch.py | 32 ++++++++++++++++----------- src/components/VersionPicker.tsx | 24 ++++++++++++-------- 2 files changed, 34 insertions(+), 22 deletions(-) diff --git a/py_modules/services/version_switch.py b/py_modules/services/version_switch.py index 61bb1122..644be65f 100644 --- a/py_modules/services/version_switch.py +++ b/py_modules/services/version_switch.py @@ -221,19 +221,7 @@ async def get_version_list(self, app_id: int) -> dict[str, Any]: server_query_failed=True, ) - bound_payload: Any = bound_detail - raw_stubs = bound_payload.get("sibling_roms") if isinstance(bound_payload, dict) else None - stubs: list[dict[str, Any]] = [] - if isinstance(raw_stubs, list): - for stub in raw_stubs: - if not isinstance(stub, dict): - continue - try: - rom_id = int(stub.get("id", 0)) - except (TypeError, ValueError): - continue - if rom_id > 0: - stubs.append(stub) + stubs = self._valid_sibling_stubs(bound_detail) direct_sibling_ids = {int(s["id"]) for s in stubs} suspect_ids = local.member_ids - direct_sibling_ids - {local.bound_rom_id} vanished_ids = await self._probe_vanished(suspect_ids) @@ -252,6 +240,24 @@ async def get_version_list(self, app_id: int) -> dict[str, Any]: server_query_failed=False, ) + @staticmethod + def _valid_sibling_stubs(bound_detail: Any) -> list[dict[str, Any]]: + """Return dict-shaped sibling stubs carrying a positive integer id.""" + raw_stubs = bound_detail.get("sibling_roms") if isinstance(bound_detail, dict) else None + stubs: list[dict[str, Any]] = [] + if not isinstance(raw_stubs, list): + return stubs + for stub in raw_stubs: + if not isinstance(stub, dict): + continue + try: + rom_id = int(stub.get("id", 0)) + except (TypeError, ValueError): + continue + if rom_id > 0: + stubs.append(stub) + return stubs + async def _probe_vanished(self, rom_ids: set[int]) -> set[int]: """Return ids whose single-attempt exact-ROM probe produced a typed 404.""" ordered_ids = sorted(rom_ids) diff --git a/src/components/VersionPicker.tsx b/src/components/VersionPicker.tsx index 3f778966..44a1d241 100644 --- a/src/components/VersionPicker.tsx +++ b/src/components/VersionPicker.tsx @@ -356,6 +356,20 @@ export const VersionPicker: FC = ({ appId }) => { return ; }; + const rowAvailabilityHint = (v: VersionInfo): ReactNode => { + let text: string; + if (v.vanished) { + text = "No longer available on RomM"; + } else if (!v.switchable) { + text = "conflicting metadata match in RomM"; + } else { + return null; + } + return ( + {text} + ); + }; + const openMenu = (e: MouseEvent): void => { // Blocked while a switch is in flight — the list is stale until the reload // lands, so opening it now would let a click act against the wrong versions. @@ -380,15 +394,7 @@ export const VersionPicker: FC = ({ appId }) => { {v.is_default ? : null} {v.installed ? : null} {v.switchable && !v.synced ? : null} - {v.vanished ? ( - - No longer available on RomM - - ) : v.switchable ? null : ( - - conflicting metadata match in RomM - - )} + {rowAvailabilityHint(v)} {v.active ? ✓ : null}