diff --git a/docs/architecture/backend-architecture.md b/docs/architecture/backend-architecture.md index b41a20c9..b748cfbe 100644 --- a/docs/architecture/backend-architecture.md +++ b/docs/architecture/backend-architecture.md @@ -142,6 +142,26 @@ excluded from default ranking. A bound-id 404 is a successful entity verdict (`b `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. +#### Version-switch target liveness + +`VersionSwitchService.switch_version` obtains a fresh exact-id verdict immediately before every actual binding move onto +an already-local target. The save-stranding guard runs first, so its initial soft block performs no target request; both +follow-up choices (`Sync now` retry and `Switch anyway`) re-enter the service and receive the same liveness guard. +`allow_stranded` bypasses only save stranding. The active-target no-op, invalid local context, active download, +non-member target, and target bound to another shortcut all stop before the request because none can enter the binding +write. + +The local-target request uses `RommRomReader.get_rom_once` on the worker executor: one attempt, three-second timeout, no +retry-progress event, and no open UoW. Only `RommNotFoundError` returns the canonical `version_vanished` refusal. +Timeout, transport/DNS/SSL, authentication, server errors, and malformed or empty successful payloads are logged and +fail open, preserving the fast offline switch path. The existing short write UoW still rechecks membership and +bound-elsewhere after the request; the request narrows the race but cannot form a transaction across RomM and SQLite. + +A server-only target still needs its full detail before it can be validated and persisted. That mandatory fetch keeps +the normal retry/classification policy and is not preceded by a redundant exact-id probe; only its typed 404 is peeled +into `version_vanished`. A refusal performs no binding, row/install/applied-launch-options write, launch-command +resolution, event, cache invalidation, sync, or completion-stamp update. + #### 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 e860e8a2..9551d022 100644 --- a/docs/architecture/steam-non-steam-shortcuts.md +++ b/docs/architecture/steam-non-steam-shortcuts.md @@ -151,6 +151,22 @@ a shortcut still bound to a vanished id show the retained context while the user tab likewise skips positively vanished inactive installs before checking local drift, then continues through later live candidates. +The list verdict is advisory UI state, not authority for a later write. Immediately before `switch_version` moves the +binding onto an already-local target, it checks that exact target id again through the same three-second, single-attempt +`get_rom_once` path. The request runs on the worker executor outside the write UoW and after the save-stranding guard +permits the attempt. Consequently an initial unsynced-save warning makes no target request, while both `Sync now` and +`Switch anyway` retries are protected; `allow_stranded` never bypasses liveness. A typed target 404 returns +`version_vanished` without changing the binding or any recorded launch state. Every other optional-probe outcome fails +open, so a local switch remains fast when RomM is uncertain or offline. The active-target no-op does not probe because +it moves no binding. + +A server-only target already requires its full RomM detail for membership validation and row construction. That fetch +keeps its normal retry policy, doubles as the liveness verdict, and receives no second probe. Its typed 404 produces the +same `version_vanished` refusal; other failures retain their ordinary classified reason. Network I/O remains outside the +short write UoW, whose fresh membership and bound-elsewhere checks still decide SQLite races. This leaves an unavoidable +cross-system interval after a successful response: the liveness check reduces stale-list risk but is not a transaction +with RomM. + ## 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 c2b470f7..acbf1864 100644 --- a/docs/user-guide/managing-games.md +++ b/docs/user-guide/managing-games.md @@ -69,6 +69,14 @@ title and Region/Languages rows update to reflect it, and its cover refreshes to keeps its name, its place in your collections, and its playtime — all tied to the shortcut, which never changes. A single-version game shows no Version control. +The plugin checks the selected version again immediately before moving the shortcut. If RomM now answers that the exact +version no longer exists, the switch is refused with **Could not switch version**, nothing about the shortcut changes, +and the picker refreshes directly so the row becomes unavailable without starting a library sync. This protection also +applies after **Sync now & switch** and **Switch anyway**; the latter bypasses only the unsynced-save warning. If RomM +cannot provide a definitive answer because of a timeout, connection/sign-in/server error, or malformed response, the +local switch is allowed to continue. In particular, the offline **Switch anyway** path does not wait through the normal +retry sequence. A target-specific refusal does not by itself change the plugin's global online/offline status. + Switching **never deletes anything.** ROM files already on disk stay put, and save files are never moved or deleted by a switch. What happens depends on whether the version you're leaving is downloaded and whether its saves are synced: diff --git a/py_modules/services/version_switch.py b/py_modules/services/version_switch.py index 644be65f..6bb2856e 100644 --- a/py_modules/services/version_switch.py +++ b/py_modules/services/version_switch.py @@ -7,6 +7,7 @@ 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), +rechecking a local target's exact-id liveness immediately before the write and persisting a server-only target first when needed. A version switch is a pure binding move: the shortcut's name and appId stay @@ -21,7 +22,8 @@ resolved by ``sibling_group_key`` over the migration-010 index; the server view comes from ``get_rom(bound).sibling_roms``. Server I/O and the relaunch resolver run outside the Unit of Work (ADR-0006) — read/close, then fetch/resolve, then a -short write UoW. +short write UoW. The liveness request narrows the race but cannot transact with +RomM and SQLite as one unit. """ from __future__ import annotations @@ -540,8 +542,10 @@ async def switch_version(self, app_id: int, target_rom_id: int, allow_stranded: first — the refusal carries ``server_reachable`` (from the reachability probe), ``unsynced_rom_id``, and ``unsynced_version_name`` so the frontend modal can offer "Sync now & switch" / "Switch anyway" / "Cancel". - ``allow_stranded`` is the "Switch anyway" override — honoured even offline - (the switch is purely local). Saves are never deleted or transferred. + ``allow_stranded`` is the "Switch anyway" override for the save-stranding + gate only. The local target still receives a short, single-attempt exact-id + liveness probe; only a typed 404 refuses it, while uncertainty fails open so + an offline switch stays fast. Saves are never deleted or transferred. On success returns ``{success: True, rom_id, target_installed, launch_options, app_id}``: @@ -553,10 +557,10 @@ async def switch_version(self, app_id: int, target_rom_id: int, allow_stranded: target outside the group → ``not_in_group``; target bound to a *different* shortcut (grandfathered duplicate, ADR-0021 §5) → ``bound_elsewhere``; a server-only target whose detail the aggregate rejects → ``invalid_target``; - a failed server-only target fetch → the :func:`classify_error` slug for the - exception, so a server that answered 404 for a dropped id reads - ``not_found`` and only a genuine transport failure reads - ``server_unreachable`` (#1570). + a definitive 404 for either target shape → ``version_vanished``; any other + failed mandatory server-only target fetch → the :func:`classify_error` slug + for the exception. The optional local probe instead fails open on every + non-404 outcome (#1570). """ app_id = int(app_id) target_rom_id = int(target_rom_id) @@ -588,21 +592,7 @@ async def switch_version(self, app_id: int, target_rom_id: int, allow_stranded: return self._switch_success(ctx.bound_rom_id, ctx.bound_installed, launch_options, app_id) if ctx.target_is_local: - # Fast reject an invalid local target before the drift/reachability - # probes; the write UoW re-checks these same facts (TOCTOU). - if not target_in_sibling_group( - bound_group_key=ctx.group_key, - target_group_key=ctx.target_group_key, - target_is_local=True, - target_is_server_sibling=False, - ): - return self._not_in_group() - if ctx.target_app_id is not None and ctx.target_app_id != app_id: - return self._bound_elsewhere(target_rom_id) - block = await self._save_stranding_block(ctx, allow_stranded) - if block is not None: - return block - return await self._switch_local(app_id, target_rom_id, ctx.group_key) + return await self._switch_to_local_target(app_id, target_rom_id, allow_stranded, ctx) # Target not persisted locally — the save-stranding gate on the bound # version applies first (allow_stranded skips it), then fetch the detail, @@ -612,7 +602,10 @@ async def switch_version(self, app_id: int, target_rom_id: int, allow_stranded: return block try: target_dict = await self._loop.run_in_executor(None, self._romm_api.get_rom, target_rom_id) - except Exception as e: # classified: a 404 on the target is not an offline server + except RommNotFoundError: + self._logger.warning(f"Version switch: target rom {target_rom_id} is gone from the server") + return self._version_vanished() + except Exception as e: # mandatory fetch: preserve every non-404 classified failure self._logger.warning(f"Version switch: target fetch failed for rom {target_rom_id}: {e}") reason, message = classify_error(e) return { @@ -640,6 +633,46 @@ async def switch_version(self, app_id: int, target_rom_id: int, allow_stranded: None, self._persist_and_bind, target_dict, app_id, ctx.platform_slug, ctx.group_key ) + async def _switch_to_local_target( + self, app_id: int, target_rom_id: int, allow_stranded: bool, ctx: _SwitchContext + ) -> dict[str, Any]: + """Validate a local target, guard its liveness, then enter the binding write.""" + # Fast reject invalid local state before drift/reachability probes; the + # write UoW re-checks these same membership/collision facts (TOCTOU). + if not target_in_sibling_group( + bound_group_key=ctx.group_key, + target_group_key=ctx.target_group_key, + target_is_local=True, + target_is_server_sibling=False, + ): + return self._not_in_group() + if ctx.target_app_id is not None and ctx.target_app_id != app_id: + return self._bound_elsewhere(target_rom_id) + block = await self._save_stranding_block(ctx, allow_stranded) + if block is not None: + return block + target_vanished = await self._loop.run_in_executor(None, self._probe_switch_target_vanished, target_rom_id) + if target_vanished: + return self._version_vanished() + return await self._switch_local(app_id, target_rom_id, ctx.group_key) + + def _probe_switch_target_vanished(self, rom_id: int) -> bool: + """Probe one local switch target once; only a typed 404 refuses it.""" + try: + response: Any = self._romm_api.get_rom_once(rom_id) + except RommNotFoundError: + return True + except Exception as e: + self._logger.warning(f"Version switch: target liveness probe failed open for rom {rom_id}: {e}") + return False + payload_id = response.get("id") if isinstance(response, dict) else None + if type(payload_id) is not int or payload_id != rom_id: + self._logger.warning( + f"Version switch: target liveness probe returned an untrustworthy payload for rom {rom_id}; " + "failing open" + ) + return False + async def _save_stranding_block(self, ctx: _SwitchContext, allow_stranded: bool) -> dict[str, Any] | None: """Soft-block the switch when the bound version has un-uploaded save drift. @@ -874,3 +907,11 @@ def _bound_elsewhere(target_rom_id: int) -> dict[str, Any]: "reason": "bound_elsewhere", "message": f"Version {target_rom_id} is already used by another shortcut.", } + + @staticmethod + def _version_vanished() -> dict[str, Any]: + return { + "success": False, + "reason": "version_vanished", + "message": "This version is no longer available on RomM.", + } diff --git a/src/api/backend.ts b/src/api/backend.ts index 01354b86..02477013 100755 --- a/src/api/backend.ts +++ b/src/api/backend.ts @@ -505,6 +505,7 @@ export interface SwitchVersionUnsyncedSaves { * `bound_elsewhere` (a grandfathered duplicate), * `invalid_target` (a server-only target the aggregate rejects), * `download_in_progress` (a group download is running — cancel it first), or + * `version_vanished` (RomM definitively no longer has the target), or * `server_unreachable`. All surface via the picker's toast fallback * (`result.message`). The literal reason union excludes `unsynced_saves` so the * soft-block variant narrows cleanly. @@ -512,7 +513,13 @@ export interface SwitchVersionUnsyncedSaves { export interface SwitchVersionFailure { success: false; reason: - "not_found" | "not_in_group" | "bound_elsewhere" | "invalid_target" | "download_in_progress" | "server_unreachable"; + | "not_found" + | "not_in_group" + | "bound_elsewhere" + | "invalid_target" + | "download_in_progress" + | "version_vanished" + | "server_unreachable"; message: string; } diff --git a/src/components/VersionPicker.test.tsx b/src/components/VersionPicker.test.tsx index 50ef1a6d..ffea139f 100644 --- a/src/components/VersionPicker.test.tsx +++ b/src/components/VersionPicker.test.tsx @@ -715,6 +715,280 @@ describe("VersionPicker — switching", () => { }); }); +describe("VersionPicker — switch target liveness (#1570)", () => { + const vanishedFailure = { + success: false as const, + reason: "version_vanished" as const, + message: "This version is no longer available on RomM.", + }; + + beforeEach(() => { + captured.menu = null; + vi.mocked(backend.getVersionList).mockReset(); + vi.mocked(backend.switchVersion).mockReset(); + vi.mocked(backend.fetchCoverBase64).mockReset().mockResolvedValue({ base64: null }); + vi.mocked(invalidateCachedGameDetail).mockReset(); + vi.mocked(setLaunchOptionsConfirmed).mockReset().mockResolvedValue(true); + vi.mocked(toaster.toast).mockReset(); + setRommConnectionState("checking"); + }); + + afterEach(() => setRommConnectionState("checking")); + + it("refuses the initial click, preserves detail, emits no success effects, and converges from a direct reload", async () => { + const refreshed = multiVersionList({ + versions: (multiVersionList().versions ?? []).map((v) => + v.rom_id === 2 ? { ...v, vanished: true, is_default: false } : v, + ), + }); + vi.mocked(backend.getVersionList).mockResolvedValueOnce(multiVersionList()).mockResolvedValueOnce(refreshed); + vi.mocked(backend.switchVersion).mockResolvedValue(vanishedFailure); + const setArt = vi.fn().mockResolvedValue(undefined); + vi.stubGlobal("SteamClient", { Apps: { SetCustomArtworkForApp: setArt } }); + + const { r, menu } = await renderAndOpen(); + vi.mocked(backend.fetchCoverBase64).mockClear(); + const dispatched = await captureDataChanged(() => clickRow(menu.container, "Game (Japan)")); + + expect(toaster.toast).toHaveBeenCalledWith({ + title: "RomM Sync", + body: "Could not switch version", + subtext: vanishedFailure.message, + }); + await waitFor(() => expect(backend.getVersionList).toHaveBeenCalledTimes(2)); + expect(setLaunchOptionsConfirmed).not.toHaveBeenCalled(); + expect(backend.fetchCoverBase64).not.toHaveBeenCalled(); + expect(setArt).not.toHaveBeenCalled(); + expect(invalidateCachedGameDetail).not.toHaveBeenCalled(); + expect(dispatched.some((e) => e.detail?.type === "version_switched")).toBe(false); + + captured.menu = null; + await act(async () => { + fireEvent.click(r.getByTestId("version-btn")); + await Promise.resolve(); + }); + const refreshedMenu = render(<>{captured.menu}); + const target = within(refreshedMenu.container) + .getAllByRole("menuitem") + .find((item) => item.textContent.includes("Game (Japan)")); + expect(target?.getAttribute("aria-disabled")).toBe("true"); + expect(target?.textContent).toContain("No longer available on RomM"); + }); + + it("leaves connection state alone for version_vanished itself", async () => { + vi.mocked(backend.getVersionList) + .mockResolvedValueOnce(multiVersionList()) + .mockResolvedValueOnce(multiVersionList({ bound_vanished: true })); + vi.mocked(backend.switchVersion).mockResolvedValue(vanishedFailure); + + const { menu } = await renderAndOpen(); + setRommConnectionState("checking"); + await clickRow(menu.container, "Game (Japan)"); + await waitFor(() => expect(backend.getVersionList).toHaveBeenCalledTimes(2)); + + expect(getRommConnectionState()).toBe("checking"); + }); + + it("reports only an explicit server_unreachable initial refusal as offline", async () => { + vi.mocked(backend.getVersionList).mockResolvedValue(multiVersionList()); + vi.mocked(backend.switchVersion).mockResolvedValue({ + success: false, + reason: "server_unreachable", + message: "Server unreachable", + }); + + const { menu } = await renderAndOpen(); + setRommConnectionState("checking"); + await clickRow(menu.container, "Game (Japan)"); + + expect(getRommConnectionState()).toBe("offline"); + expect(backend.getVersionList).toHaveBeenCalledTimes(1); + }); + + it("does not force a successful fail-open switch to connected", async () => { + vi.mocked(backend.getVersionList) + .mockResolvedValueOnce(multiVersionList()) + .mockResolvedValueOnce(multiVersionList({ bound_vanished: true })); + vi.mocked(backend.switchVersion).mockResolvedValue({ + success: true, + rom_id: 2, + target_installed: false, + launch_options: "", + app_id: APP_ID, + }); + + const { menu } = await renderAndOpen(); + setRommConnectionState("checking"); + await clickRow(menu.container, "Game (Japan)"); + await waitFor(() => expect(backend.getVersionList).toHaveBeenCalledTimes(2)); + + expect(getRommConnectionState()).toBe("checking"); + }); + + it("ignores a late refusal reload after a newer successful-switch reload settles", async () => { + let resolveRefusalReload!: (value: VersionList) => void; + const refusalReload = new Promise((resolve) => { + resolveRefusalReload = resolve; + }); + const switchedList = multiVersionList({ + server_query_failed: true, + versions: (multiVersionList().versions ?? []).map((v) => ({ ...v, active: v.rom_id === 3 })), + }); + vi.mocked(backend.getVersionList) + .mockResolvedValueOnce(multiVersionList()) + .mockReturnValueOnce(refusalReload) + .mockResolvedValueOnce(switchedList); + vi.mocked(backend.switchVersion).mockResolvedValueOnce(vanishedFailure).mockResolvedValueOnce({ + success: true, + rom_id: 3, + target_installed: false, + launch_options: "", + app_id: APP_ID, + }); + + const { r, menu } = await renderAndOpen(); + setRommConnectionState("checking"); + await clickRow(menu.container, "Game (Japan)"); + expect(backend.getVersionList).toHaveBeenCalledTimes(2); + + captured.menu = null; + await act(async () => { + fireEvent.click(r.getByTestId("version-btn")); + await Promise.resolve(); + }); + const nextMenu = render(<>{captured.menu}); + await clickRow(nextMenu.container, "Game (Europe)"); + await waitFor(() => expect(backend.getVersionList).toHaveBeenCalledTimes(3)); + await waitFor(() => expect(r.container.querySelector(".romm-throbber")).toBeNull()); + expect(getRommConnectionState()).toBe("offline"); + + await act(async () => { + resolveRefusalReload(multiVersionList()); + for (let i = 0; i < 6; i++) await Promise.resolve(); + }); + expect(getRommConnectionState()).toBe("offline"); + + vi.mocked(backend.switchVersion).mockClear().mockResolvedValue({ + success: false, + reason: "bound_elsewhere", + message: "test stop", + }); + captured.menu = null; + await act(async () => { + fireEvent.click(r.getByTestId("version-btn")); + await Promise.resolve(); + }); + const settledMenu = render(<>{captured.menu}); + await clickRow(settledMenu.container, "Game (USA)"); + + // If the late refusal snapshot had overwritten the newer list, USA would be + // marked active and handleSwitch would swallow this switch-back click. + expect(backend.switchVersion).toHaveBeenCalledWith(APP_ID, 1, false); + }); + + it("ignores a refusal reload completion from an obsolete appId lifetime", async () => { + const nextAppId = APP_ID + 1; + let resolveOldReload!: (value: VersionList) => void; + const oldReload = new Promise((resolve) => { + resolveOldReload = resolve; + }); + const nextAppList = multiVersionList({ + server_query_failed: true, + versions: (multiVersionList().versions ?? []).map((v) => ({ ...v, active: v.rom_id === 3 })), + }); + vi.mocked(backend.getVersionList) + .mockResolvedValueOnce(multiVersionList()) + .mockReturnValueOnce(oldReload) + .mockResolvedValueOnce(nextAppList); + vi.mocked(backend.switchVersion).mockResolvedValue(vanishedFailure); + + const { r, menu } = await renderAndOpen(); + await clickRow(menu.container, "Game (Japan)"); + expect(backend.getVersionList).toHaveBeenCalledTimes(2); + setRommConnectionState("checking"); + + r.rerender(); + await waitFor(() => expect(backend.getVersionList).toHaveBeenNthCalledWith(3, nextAppId)); + await waitFor(() => expect(getRommConnectionState()).toBe("offline")); + + await act(async () => { + resolveOldReload(multiVersionList()); + for (let i = 0; i < 6; i++) await Promise.resolve(); + }); + expect(getRommConnectionState()).toBe("offline"); + + vi.mocked(backend.switchVersion).mockClear().mockResolvedValue({ + success: false, + reason: "bound_elsewhere", + message: "test stop", + }); + captured.menu = null; + await act(async () => { + fireEvent.click(r.getByTestId("version-btn")); + await Promise.resolve(); + }); + const nextAppMenu = render(<>{captured.menu}); + await clickRow(nextAppMenu.container, "Game (USA)"); + + expect(backend.switchVersion).toHaveBeenCalledWith(nextAppId, 1, false); + }); + + it("ignores a refusal reload completion after unmount", async () => { + let resolveReload!: (value: VersionList) => void; + const reload = new Promise((resolve) => { + resolveReload = resolve; + }); + vi.mocked(backend.getVersionList).mockResolvedValueOnce(multiVersionList()).mockReturnValueOnce(reload); + vi.mocked(backend.switchVersion).mockResolvedValue(vanishedFailure); + + const { r, menu } = await renderAndOpen(); + await clickRow(menu.container, "Game (Japan)"); + expect(backend.getVersionList).toHaveBeenCalledTimes(2); + setRommConnectionState("checking"); + r.unmount(); + + await act(async () => { + resolveReload(multiVersionList({ server_query_failed: true })); + for (let i = 0; i < 6; i++) await Promise.resolve(); + }); + + expect(getRommConnectionState()).toBe("checking"); + }); + + it("keeps the refusal visible and releases the guard when its direct reload rejects", async () => { + const logWarnSpy = vi.spyOn(backend, "logWarn").mockImplementation(() => {}); + try { + vi.mocked(backend.getVersionList) + .mockResolvedValueOnce(multiVersionList()) + .mockRejectedValueOnce(new Error("refresh failed")); + vi.mocked(backend.switchVersion).mockResolvedValue(vanishedFailure); + + const { r, menu } = await renderAndOpen(); + setRommConnectionState("checking"); + const dispatched = await captureDataChanged(() => clickRow(menu.container, "Game (Japan)")); + + expect(toaster.toast).toHaveBeenCalledWith({ + title: "RomM Sync", + body: "Could not switch version", + subtext: vanishedFailure.message, + }); + expect(logWarnSpy).toHaveBeenCalledWith(expect.stringContaining("version-vanished list refresh failed")); + expect(getRommConnectionState()).toBe("checking"); + expect(r.container.querySelector(".romm-throbber")).toBeNull(); + expect(dispatched.some((e) => e.detail?.type === "version_switched")).toBe(false); + + captured.menu = null; + await act(async () => { + fireEvent.click(r.getByTestId("version-btn")); + await Promise.resolve(); + }); + expect(captured.menu).not.toBeNull(); + } finally { + logWarnSpy.mockRestore(); + } + }); +}); + describe("VersionPicker — unsynced-saves soft-block", () => { const block = { success: false as const, @@ -743,8 +1017,11 @@ describe("VersionPicker — unsynced-saves soft-block", () => { vi.mocked(setLaunchOptionsConfirmed).mockReset().mockResolvedValue(true); vi.mocked(showUnsyncedSavesModal).mockReset(); vi.mocked(toaster.toast).mockReset(); + setRommConnectionState("checking"); }); + afterEach(() => setRommConnectionState("checking")); + it("opens the modal with the stranded version's name + reachability", async () => { vi.mocked(backend.switchVersion).mockResolvedValue(block); vi.mocked(showUnsyncedSavesModal).mockResolvedValue("cancel"); @@ -817,6 +1094,110 @@ describe("VersionPicker — unsynced-saves soft-block", () => { expect(dispatched.some((e) => e.detail?.type === "version_switched")).toBe(true); }); + it("routes a version_vanished 'Switch anyway' retry through the common refusal path", async () => { + const vanished = { + success: false as const, + reason: "version_vanished" as const, + message: "This version is no longer available on RomM.", + }; + vi.mocked(backend.getVersionList) + .mockReset() + .mockResolvedValueOnce(multiVersionList()) + .mockResolvedValueOnce(multiVersionList({ bound_vanished: true })); + vi.mocked(backend.switchVersion).mockResolvedValueOnce(block).mockResolvedValueOnce(vanished); + vi.mocked(showUnsyncedSavesModal).mockResolvedValue("switch_anyway"); + const setArt = vi.fn().mockResolvedValue(undefined); + vi.stubGlobal("SteamClient", { Apps: { SetCustomArtworkForApp: setArt } }); + + const { menu } = await renderAndOpen(); + vi.mocked(backend.fetchCoverBase64).mockClear(); + setRommConnectionState("checking"); + const dispatched = await captureDataChanged(() => clickRow(menu.container, "Game (Japan)")); + + expect(backend.switchVersion).toHaveBeenNthCalledWith(2, APP_ID, 2, true); + expect(toaster.toast).toHaveBeenCalledWith({ + title: "RomM Sync", + body: "Could not switch version", + subtext: vanished.message, + }); + await waitFor(() => expect(backend.getVersionList).toHaveBeenCalledTimes(2)); + expect(getRommConnectionState()).toBe("connected"); + expect(setLaunchOptionsConfirmed).not.toHaveBeenCalled(); + expect(backend.fetchCoverBase64).not.toHaveBeenCalled(); + expect(setArt).not.toHaveBeenCalled(); + expect(invalidateCachedGameDetail).not.toHaveBeenCalled(); + expect(dispatched.some((e) => e.detail?.type === "version_switched")).toBe(false); + }); + + it("reports an explicit server_unreachable 'Switch anyway' retry as offline", async () => { + vi.mocked(backend.switchVersion).mockResolvedValueOnce(block).mockResolvedValueOnce({ + success: false, + reason: "server_unreachable", + message: "Server unreachable", + }); + vi.mocked(showUnsyncedSavesModal).mockResolvedValue("switch_anyway"); + + const { menu } = await renderAndOpen(); + setRommConnectionState("checking"); + await clickRow(menu.container, "Game (Japan)"); + + expect(getRommConnectionState()).toBe("offline"); + expect(backend.getVersionList).toHaveBeenCalledTimes(1); + }); + + it("routes a version_vanished post-save retry through the common refusal path", async () => { + const vanished = { + success: false as const, + reason: "version_vanished" as const, + message: "This version is no longer available on RomM.", + }; + vi.mocked(backend.getVersionList) + .mockReset() + .mockResolvedValueOnce(multiVersionList()) + .mockResolvedValueOnce(multiVersionList({ bound_vanished: true })); + vi.mocked(backend.switchVersion).mockResolvedValueOnce(block).mockResolvedValueOnce(vanished); + vi.mocked(backend.syncRomSaves).mockResolvedValue({ success: true, message: "", synced: 1 }); + vi.mocked(showUnsyncedSavesModal).mockResolvedValue("sync_and_switch"); + const setArt = vi.fn().mockResolvedValue(undefined); + vi.stubGlobal("SteamClient", { Apps: { SetCustomArtworkForApp: setArt } }); + + const { menu } = await renderAndOpen(); + vi.mocked(backend.fetchCoverBase64).mockClear(); + setRommConnectionState("checking"); + const dispatched = await captureDataChanged(() => clickRow(menu.container, "Game (Japan)")); + + expect(backend.switchVersion).toHaveBeenNthCalledWith(2, APP_ID, 2, false); + expect(toaster.toast).toHaveBeenCalledWith({ + title: "RomM Sync", + body: "Could not switch version", + subtext: vanished.message, + }); + await waitFor(() => expect(backend.getVersionList).toHaveBeenCalledTimes(2)); + expect(getRommConnectionState()).toBe("connected"); + expect(setLaunchOptionsConfirmed).not.toHaveBeenCalled(); + expect(backend.fetchCoverBase64).not.toHaveBeenCalled(); + expect(setArt).not.toHaveBeenCalled(); + expect(invalidateCachedGameDetail).not.toHaveBeenCalled(); + expect(dispatched.some((e) => e.detail?.type === "version_switched")).toBe(false); + }); + + it("reports an explicit server_unreachable post-save retry as offline", async () => { + vi.mocked(backend.switchVersion).mockResolvedValueOnce(block).mockResolvedValueOnce({ + success: false, + reason: "server_unreachable", + message: "Server unreachable", + }); + vi.mocked(backend.syncRomSaves).mockResolvedValue({ success: true, message: "", synced: 1 }); + vi.mocked(showUnsyncedSavesModal).mockResolvedValue("sync_and_switch"); + + const { menu } = await renderAndOpen(); + setRommConnectionState("checking"); + await clickRow(menu.container, "Game (Japan)"); + + expect(getRommConnectionState()).toBe("offline"); + expect(backend.getVersionList).toHaveBeenCalledTimes(1); + }); + it("aborts with a toast + save-status refresh when the pre-switch sync fails", async () => { vi.mocked(backend.switchVersion).mockResolvedValue(block); vi.mocked(backend.syncRomSaves).mockResolvedValue({ success: false, message: "boom", synced: 0 }); diff --git a/src/components/VersionPicker.tsx b/src/components/VersionPicker.tsx index 44a1d241..132828e0 100644 --- a/src/components/VersionPicker.tsx +++ b/src/components/VersionPicker.tsx @@ -35,7 +35,13 @@ import { logError, logWarn, } from "../api/backend"; -import type { VersionList, VersionInfo, SwitchVersionSuccess } from "../api/backend"; +import type { + VersionList, + VersionInfo, + SwitchVersionSuccess, + SwitchVersionFailure, + SwitchVersionUnsyncedSaves, +} from "../api/backend"; import { reportServerReachable } from "../utils/connectionState"; import { setLaunchOptionsConfirmed } from "../utils/steamShortcuts"; import { showUnsyncedSavesModal } from "./UnsyncedSavesSwitchModal"; @@ -53,6 +59,14 @@ interface VersionPickerProps { const ACTIVE_ACCENT = "#59b6ff"; const NEUTRAL_GREY = "#dcdedf"; +const reportVersionListReachability = (result: VersionList): void => { + if (result.server_query_failed) { + reportServerReachable(false); + } else if (result.multi_version && !result.bound_vanished) { + reportServerReachable(true); + } +}; + const BADGE_COLORS: Record<"accent" | "muted" | "good", { bg: string; fg: string }> = { accent: { bg: "rgba(89, 182, 255, 0.18)", fg: ACTIVE_ACCENT }, good: { bg: "rgba(91, 163, 43, 0.22)", fg: "#7ac74f" }, @@ -93,39 +107,49 @@ export const VersionPicker: FC = ({ appId }) => { // The group's member rom_ids from the last loaded list — lets the install-change // listeners below ignore events for other games without a fetch. const memberIdsRef = useRef>(new Set()); + const listRequestIdRef = useRef(0); + const loadVersionListRef = useRef<{ + appId: number; + load: (source?: "normal" | "vanished_refusal") => Promise; + } | null>(null); // Initial load + refresh on a version switch (this or another surface). The - // loader is defined inside the effect (shared with the event handler) so its - // post-`await` setState never reads as a synchronous effect-body write. + // effect owns the loader lifetime for this appId. All request sources share one + // generation so only the latest completion can publish list/reachability state. useEffect(() => { let cancelled = false; - const load = async (): Promise => { + const load = async (source: "normal" | "vanished_refusal" = "normal"): Promise => { + const requestId = ++listRequestIdRef.current; + const isCurrent = (): boolean => !cancelled && requestId === listRequestIdRef.current; try { const result = await getVersionList(appId); + if (!isCurrent()) return; // 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 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 && !result.bound_vanished) { - reportServerReachable(true); - } + reportVersionListReachability(result); memberIdsRef.current = new Set((result.versions ?? []).map((v) => v.rom_id)); - if (!cancelled) setVersionList(result); + setVersionList(result); } catch (e) { - logError(`VersionPicker: getVersionList failed: ${e}`); + if (!isCurrent()) return; + if (source === "vanished_refusal") { + logWarn(`VersionPicker: version-vanished list refresh failed: ${e}`); + } else { + logError(`VersionPicker: getVersionList failed: ${e}`); + } } finally { // The post-switch version_switched reload landing is the "switch fully // settled" signal — clear the in-flight guard here so the trigger // re-enables against a FRESH list, never a stale one (#1345 round-2 / E). // In the finally (not just on success) so a failed reload can't leave the // guard stuck; on the initial mount load switching is already false (no-op). - if (!cancelled) setSwitching(false); + if (source === "normal" && isCurrent()) setSwitching(false); } }; + loadVersionListRef.current = { appId, load }; detach(load()); const onDataChanged = (e: Event) => { @@ -153,6 +177,7 @@ export const VersionPicker: FC = ({ appId }) => { return () => { cancelled = true; + if (loadVersionListRef.current?.load === load) loadVersionListRef.current = null; globalThis.removeEventListener("romm_data_changed", onDataChanged); removeEventListener("download_complete", dlComplete); removeEventListener("download_failed", dlFailed); @@ -230,6 +255,19 @@ export const VersionPicker: FC = ({ appId }) => { ); }; + const refreshAfterVanishedRefusal = (): Promise => { + const loader = loadVersionListRef.current; + if (loader?.appId !== appId) return Promise.resolve(); + return loader.load("vanished_refusal"); + }; + + const handleSwitchFailure = (result: SwitchVersionFailure | SwitchVersionUnsyncedSaves): void => { + if (result.reason === "server_unreachable") reportServerReachable(false); + setSwitching(false); + toaster.toast({ title: "RomM Sync", body: "Could not switch version", subtext: result.message }); + if (result.reason === "version_vanished") detach(refreshAfterVanishedRefusal()); + }; + // Sync the stranded version's saves, then retry the switch. Any failure — // sync failed, sync surfaced conflicts, or the retry blocked again — aborts // with a short toast and re-runs the save-status refresh so the conflict UI @@ -264,7 +302,7 @@ export const VersionPicker: FC = ({ appId }) => { // race) — say so instead of the generic "couldn't switch". abort("Saves still unsynced — try again"); } else { - abort("Couldn't switch versions"); + handleSwitchFailure(retry); } } catch (e) { logError(`VersionPicker: sync-then-switch failed: ${e}`); @@ -287,7 +325,6 @@ export const VersionPicker: FC = ({ appId }) => { try { const result = await switchVersion(appId, target.rom_id, false); if (result.success) { - reportServerReachable(true); await applySwitchSuccess(result); return; } @@ -315,16 +352,13 @@ export const VersionPicker: FC = ({ appId }) => { if (forced.success) { await applySwitchSuccess(forced); } else { - setSwitching(false); - toaster.toast({ title: "RomM Sync", body: "Could not switch version", subtext: forced.message }); + handleSwitchFailure(forced); } return; } - if (result.reason === "server_unreachable") reportServerReachable(false); - setSwitching(false); // Keep the toast body short (Steam truncates it to one line) and put the // backend detail in the subtext so the reason is readable (#1359). - toaster.toast({ title: "RomM Sync", body: "Could not switch version", subtext: result.message }); + handleSwitchFailure(result); } catch (e) { setSwitching(false); logError(`VersionPicker: switchVersion failed: ${e}`); diff --git a/tests/contract/test_version_picker.py b/tests/contract/test_version_picker.py index 4bbcbb49..d50733dd 100644 --- a/tests/contract/test_version_picker.py +++ b/tests/contract/test_version_picker.py @@ -39,21 +39,24 @@ def _seed_rom( regions: tuple[str, ...] = (), is_main_sibling: bool = False, name: str | None = None, + applied_launch_options: str | None = None, ) -> None: with harness.uow_factory() as uow: - uow.roms.save( - Rom( - rom_id=rom_id, - platform_slug="snes", - name=name or f"Game {rom_id}", - fs_name=f"game_{rom_id}.sfc", - shortcut_app_id=app_id, - last_synced_at="2026-01-01T00:00:00", - sibling_group_key=group_key, - regions=regions, - is_main_sibling=is_main_sibling, - ) + rom = Rom( + rom_id=rom_id, + platform_slug="snes", + name=name or f"Game {rom_id}", + fs_name=f"game_{rom_id}.sfc", + shortcut_app_id=app_id, + last_synced_at="2026-01-01T00:00:00", + sibling_group_key=group_key, + regions=regions, + is_main_sibling=is_main_sibling, + applied_launch_options=applied_launch_options, ) + uow.roms.save(rom) + if applied_launch_options is not None: + uow.roms.set_applied_launch_options(rom_id, applied_launch_options) def _seed_install(harness, rom_id: int) -> None: @@ -329,6 +332,8 @@ async def test_switch_version_happy_moves_binding(harness): "launch_options": "", "app_id": _APP_ID, } + assert [args[0] for name, args, _kwargs in harness.romm.call_log if name == "get_rom_once"] == [2] + assert not any(name == "get_rom" for name, _args, _kwargs in harness.romm.call_log) with harness.uow_factory() as uow: assert uow.roms.get(2).shortcut_app_id == _APP_ID @@ -339,6 +344,87 @@ async def test_switch_version_happy_moves_binding(harness): assert by_id[2]["active"] is True +async def test_switch_version_local_404_has_canonical_shape_and_no_mutation(harness): + """A definitive local-target 404 refuses before every write/effect boundary.""" + _seed_rom(harness, rom_id=1, app_id=_APP_ID, applied_launch_options="old-command") + _seed_rom(harness, rom_id=2, app_id=None, applied_launch_options="target-command") + _seed_install(harness, 2) + harness.romm.get_rom_once_side_effect_by_id[2] = RommNotFoundError("gone") + harness.emit.reset_mock() + + result = await harness.plugin.switch_version(_APP_ID, 2, False) + + assert result == { + "success": False, + "reason": "version_vanished", + "message": "This version is no longer available on RomM.", + } + assert [args[0] for name, args, _kwargs in harness.romm.call_log if name == "get_rom_once"] == [2] + assert not any(name == "get_rom" for name, _args, _kwargs in harness.romm.call_log) + assert not any(name.startswith("list_") for name, _args, _kwargs in harness.romm.call_log) + harness.emit.assert_not_awaited() + with harness.uow_factory() as uow: + assert uow.roms.get(1).shortcut_app_id == _APP_ID + assert uow.roms.get(1).applied_launch_options == "old-command" + assert uow.roms.get(2).shortcut_app_id is None + assert uow.roms.get(2).applied_launch_options == "target-command" + assert uow.rom_installs.get(2) is not None + + +async def test_switch_version_local_probe_transport_failure_fails_open_once(harness): + """The real callable preserves the fast offline local-switch path.""" + _seed_rom(harness, rom_id=1, app_id=_APP_ID) + _seed_rom(harness, rom_id=2, app_id=None) + harness.romm.get_rom_once_side_effect_by_id[2] = ConnectionError("offline") + + result = await harness.plugin.switch_version(_APP_ID, 2, False) + + assert result["success"] is True + assert result["rom_id"] == 2 + assert [args[0] for name, args, _kwargs in harness.romm.call_log if name == "get_rom_once"] == [2] + assert not any(name == "get_rom" for name, _args, _kwargs in harness.romm.call_log) + + +async def test_switch_version_allow_stranded_still_refuses_local_404(harness): + """The frontend override bypasses save drift, never target liveness.""" + _seed_rom(harness, rom_id=1, app_id=_APP_ID) + _seed_rom(harness, rom_id=2, app_id=None) + harness.romm.get_rom_once_side_effect_by_id[2] = RommNotFoundError("gone") + + result = await harness.plugin.switch_version(_APP_ID, 2, True) + + assert result == { + "success": False, + "reason": "version_vanished", + "message": "This version is no longer available on RomM.", + } + with harness.uow_factory() as uow: + assert uow.roms.get(1).shortcut_app_id == _APP_ID + assert uow.roms.get(2).shortcut_app_id is None + + +async def test_switch_version_server_only_404_is_version_vanished_without_row(harness): + """Mandatory server-only detail loading peels only its typed 404.""" + _seed_rom(harness, rom_id=1, app_id=_APP_ID, applied_launch_options="old-command") + harness.romm.get_rom_side_effect = RommNotFoundError("gone") + harness.emit.reset_mock() + + result = await harness.plugin.switch_version(_APP_ID, 3, False) + + assert result == { + "success": False, + "reason": "version_vanished", + "message": "This version is no longer available on RomM.", + } + assert [args[0] for name, args, _kwargs in harness.romm.call_log if name == "get_rom"] == [3] + assert not any(name == "get_rom_once" for name, _args, _kwargs in harness.romm.call_log) + harness.emit.assert_not_awaited() + with harness.uow_factory() as uow: + assert uow.roms.get(1).shortcut_app_id == _APP_ID + assert uow.roms.get(1).applied_launch_options == "old-command" + assert uow.roms.get(3) is None + + async def test_switch_version_unknown_app_failure_shape(harness): """An unknown appId → canonical ``{success, reason, message}``.""" result = await harness.plugin.switch_version(999, 2, False) @@ -347,6 +433,7 @@ async def test_switch_version_unknown_app_failure_shape(harness): assert isinstance(result["message"], str) assert "error" not in result assert "error_code" not in result + assert not any(name == "get_rom_once" for name, _args, _kwargs in harness.romm.call_log) async def test_switch_version_not_in_group_failure_shape(harness): @@ -383,6 +470,7 @@ async def test_switch_version_blocked_by_active_download(harness): assert result["reason"] == "download_in_progress" assert isinstance(result["message"], str) assert "error" not in result and "error_code" not in result + assert not any(name == "get_rom_once" for name, _args, _kwargs in harness.romm.call_log) # Nothing switched — the binding is untouched. with harness.uow_factory() as uow: assert uow.roms.get(1).shortcut_app_id == _APP_ID @@ -431,6 +519,7 @@ async def test_switch_version_unsynced_saves_online_soft_blocks(harness): assert isinstance(result["unsynced_version_name"], str) assert isinstance(result["message"], str) assert "error" not in result and "error_code" not in result + assert not any(name == "get_rom_once" for name, _args, _kwargs in harness.romm.call_log) # Nothing switched. with harness.uow_factory() as uow: assert uow.roms.get(1).shortcut_app_id == _APP_ID @@ -464,15 +553,17 @@ async def test_switch_version_switch_anyway_overrides_online(harness): async def test_switch_version_switch_anyway_overrides_offline(harness): - """allow_stranded switches despite unsynced saves AND an unreachable server.""" + """allow_stranded skips save drift while target-probe uncertainty fails open.""" seed_group_member(harness, 1, group_key=_GROUP, shortcut_app_id=_APP_ID, installed=True, file_name="game.gba") seed_group_member(harness, 2, group_key=_GROUP, shortcut_app_id=None) _seed_bound_drift(harness, 1, baseline="stale-baseline-hash") harness.romm.heartbeat_side_effect = ConnectionError("offline") + harness.romm.get_rom_once_side_effect_by_id[2] = ConnectionError("offline") result = await harness.plugin.switch_version(_APP_ID, 2, True) assert result["success"] is True assert result["rom_id"] == 2 + assert [args[0] for name, args, _kwargs in harness.romm.call_log if name == "get_rom_once"] == [2] async def test_switch_version_sync_then_retry_succeeds(harness): @@ -489,6 +580,7 @@ async def test_switch_version_sync_then_retry_succeeds(harness): retried = await harness.plugin.switch_version(_APP_ID, 2, False) assert retried["success"] is True assert retried["rom_id"] == 2 + assert [args[0] for name, args, _kwargs in harness.romm.call_log if name == "get_rom_once"] == [2] async def test_switch_version_unbuildable_target_failure_shape(harness): diff --git a/tests/services/test_version_switch.py b/tests/services/test_version_switch.py index ea68c13c..a0294290 100644 --- a/tests/services/test_version_switch.py +++ b/tests/services/test_version_switch.py @@ -14,7 +14,14 @@ from domain.rom import Rom from domain.rom_install import RomInstall -from lib.errors import RommAuthError, RommNotFoundError, RommServerError, RommTimeoutError +from lib.errors import ( + RommAuthError, + RommConnectionError, + RommNotFoundError, + RommServerError, + RommSSLError, + RommTimeoutError, +) from services.version_switch import VersionSwitchService, VersionSwitchServiceConfig _GROUP = "igdb:100:57" @@ -84,6 +91,7 @@ def _seed_rom( revision: str = "", tags: tuple[str, ...] = (), is_main_sibling: bool = False, + applied_launch_options: str | None = None, ) -> None: uow.roms.save( Rom( @@ -99,6 +107,7 @@ def _seed_rom( revision=revision, tags=tags, is_main_sibling=is_main_sibling, + applied_launch_options=applied_launch_options, ) ) @@ -187,6 +196,10 @@ def _run(loop, coro): return loop.run_until_complete(coro) +def _romm_call_ids(romm: FakeRommApi, name: str) -> list[int]: + return [int(args[0]) for called_name, args, _kwargs in romm.call_log if called_name == name] + + # ── get_version_list ───────────────────────────────────────────────────── @@ -576,14 +589,15 @@ def _assert_success(result: dict[str, Any], *, rom_id: int, installed: bool, lau class TestSwitchVersion: - def test_unknown_app_id_not_found(self, event_loop, service): + def test_unknown_app_id_not_found(self, event_loop, service, romm): result = _run(event_loop, service.switch_version(999, 2, False)) assert result["success"] is False assert result["reason"] == "not_found" assert "error" not in result + assert _romm_call_ids(romm, "get_rom_once") == [] def test_f1_active_download_of_group_member_blocks_switch( - self, event_loop, service, uow, active_downloads, drift_probe + self, event_loop, service, uow, romm, active_downloads, drift_probe ): # A sibling (rom 2) is mid-download → the switch is refused with the # cancel-first shape, before the drift probe or any write. @@ -598,6 +612,7 @@ def test_f1_active_download_of_group_member_blocks_switch( assert isinstance(result["message"], str) assert "error" not in result and "error_code" not in result assert drift_probe.calls == [] # refused upstream of the drift probe + assert _romm_call_ids(romm, "get_rom_once") == [] with uow as u: assert u.roms.get(1).shortcut_app_id == _APP_ID # nothing switched assert u.roms.get(2).shortcut_app_id is None @@ -622,9 +637,9 @@ def test_f1_active_download_outside_group_does_not_block(self, event_loop, servi result = _run(event_loop, service.switch_version(_APP_ID, 2, False)) _assert_success(result, rom_id=2, installed=False, launch_options="") - def test_t2_switch_away_synced_saves_is_free(self, event_loop, service, uow, drift_probe, reachability_probe): - # Bound version installed, saves synced (no drift) → free switch, no prompt, - # and the local rebind makes NO server contact (reachability untouched). + def test_t2_switch_away_synced_saves_is_free(self, event_loop, service, uow, romm, drift_probe, reachability_probe): + # Bound version installed, saves synced (no drift) → free switch after one + # exact-id target probe; the retrying detail path stays untouched. _seed_rom(uow, rom_id=1, app_id=_APP_ID) _seed_rom(uow, rom_id=2, app_id=None) _seed_install(uow, 1) @@ -633,10 +648,106 @@ def test_t2_switch_away_synced_saves_is_free(self, event_loop, service, uow, dri _assert_success(result, rom_id=2, installed=False, launch_options="") assert drift_probe.calls == [1] # drift probed on the bound (installed) version assert reachability_probe.calls == 0 # no block → no reachability probe + assert _romm_call_ids(romm, "get_rom_once") == [2] + assert _romm_call_ids(romm, "get_rom") == [] with uow as u: assert u.roms.get(2).shortcut_app_id == _APP_ID assert u.roms.get(1).shortcut_app_id is None + def test_local_target_404_refuses_without_mutation(self, event_loop, service, uow, romm, relaunch_resolver): + _seed_rom(uow, rom_id=1, app_id=_APP_ID, applied_launch_options="old-command") + _seed_rom(uow, rom_id=2, app_id=None, applied_launch_options="target-command") + _seed_install(uow, 2) + romm.get_rom_once_side_effect_by_id[2] = RommNotFoundError("HTTP 404: Not Found") + + result = _run(event_loop, service.switch_version(_APP_ID, 2, False)) + + assert result == { + "success": False, + "reason": "version_vanished", + "message": "This version is no longer available on RomM.", + } + assert _romm_call_ids(romm, "get_rom_once") == [2] + assert _romm_call_ids(romm, "get_rom") == [] + assert relaunch_resolver.calls == [] + with uow as current: + assert current.roms.get(1).shortcut_app_id == _APP_ID + assert current.roms.get(1).applied_launch_options == "old-command" + assert current.roms.get(2).shortcut_app_id is None + assert current.roms.get(2).applied_launch_options == "target-command" + assert current.rom_installs.get(2) is not None + + def test_allow_stranded_does_not_bypass_local_target_404(self, event_loop, service, uow, romm, drift_probe): + _seed_rom(uow, rom_id=1, app_id=_APP_ID) + _seed_rom(uow, rom_id=2, app_id=None) + _seed_install(uow, 1) + drift_probe.drifted = True + romm.get_rom_once_side_effect_by_id[2] = RommNotFoundError("gone") + + result = _run(event_loop, service.switch_version(_APP_ID, 2, True)) + + assert result == { + "success": False, + "reason": "version_vanished", + "message": "This version is no longer available on RomM.", + } + assert drift_probe.calls == [] + assert _romm_call_ids(romm, "get_rom_once") == [2] + with uow as current: + assert current.roms.get(1).shortcut_app_id == _APP_ID + assert current.roms.get(2).shortcut_app_id is None + + @pytest.mark.parametrize( + "failure", + [ + RommTimeoutError("timeout"), + RommConnectionError("transport"), + RommSSLError("ssl"), + RommAuthError("unauthorized"), + RommServerError("server error", status_code=503), + ConnectionError("raw transport"), + ], + ids=["timeout", "transport", "ssl", "auth", "server-5xx", "raw-transport"], + ) + def test_local_target_probe_uncertainty_logs_and_fails_open(self, event_loop, service, uow, romm, caplog, failure): + _seed_rom(uow, rom_id=1, app_id=_APP_ID) + _seed_rom(uow, rom_id=2, app_id=None) + romm.get_rom_once_side_effect_by_id[2] = failure + + with caplog.at_level(logging.WARNING, logger="test_version_switch"): + result = _run(event_loop, service.switch_version(_APP_ID, 2, False)) + + _assert_success(result, rom_id=2, installed=False, launch_options="") + assert _romm_call_ids(romm, "get_rom_once") == [2] + assert _romm_call_ids(romm, "get_rom") == [] + assert "target liveness probe failed open for rom 2" in caplog.text + + @pytest.mark.parametrize( + "response", + [None, {}, [], "malformed", {"id": 999}], + ids=["none", "empty", "list", "string", "wrong-id"], + ) + def test_local_target_falsy_or_malformed_probe_payload_fails_open( + self, event_loop, service, uow, romm, monkeypatch, caplog, response + ): + _seed_rom(uow, rom_id=1, app_id=_APP_ID) + _seed_rom(uow, rom_id=2, app_id=None) + calls: list[int] = [] + + def get_rom_once(rom_id: int) -> Any: + calls.append(rom_id) + return response + + monkeypatch.setattr(romm, "get_rom_once", get_rom_once) + + with caplog.at_level(logging.WARNING, logger="test_version_switch"): + result = _run(event_loop, service.switch_version(_APP_ID, 2, False)) + + _assert_success(result, rom_id=2, installed=False, launch_options="") + assert calls == [2] + assert _romm_call_ids(romm, "get_rom") == [] + assert "target liveness probe returned an untrustworthy payload for rom 2; failing open" in caplog.text + def test_records_applied_launch_options_for_installed_target(self, event_loop, service, uow, relaunch_resolver): # Switching onto an installed version re-bakes its launch command (the # frontend writes it onto the sticky shortcut); recording it as the applied @@ -675,7 +786,9 @@ def test_t3_switch_back_to_installed_returns_launch_options(self, event_loop, se _assert_success(result, rom_id=2, installed=True, launch_options="flatpak run net.retrodeck.retrodeck /x") assert relaunch_resolver.calls == [2] - def test_t4_unsynced_saves_online_soft_blocks(self, event_loop, service, uow, drift_probe, reachability_probe): + def test_t4_unsynced_saves_online_soft_blocks( + self, event_loop, service, uow, romm, drift_probe, reachability_probe + ): _seed_rom(uow, rom_id=1, app_id=_APP_ID, fs_name="Game (USA).sfc") _seed_rom(uow, rom_id=2, app_id=None) _seed_install(uow, 1) @@ -690,6 +803,7 @@ def test_t4_unsynced_saves_online_soft_blocks(self, event_loop, service, uow, dr assert result["unsynced_version_name"] == "Game (USA)" assert isinstance(result["message"], str) assert "error" not in result and "error_code" not in result + assert _romm_call_ids(romm, "get_rom_once") == [] # Nothing switched — the binding is untouched. with uow as u: assert u.roms.get(1).shortcut_app_id == _APP_ID @@ -711,45 +825,84 @@ def test_t5_unsynced_saves_offline_still_soft_blocks( assert result["reason"] == "unsynced_saves" assert result["server_reachable"] is False - def test_t5_allow_stranded_switches_even_offline(self, event_loop, service, uow, drift_probe, reachability_probe): - # "Switch anyway" overrides the gate and is honoured offline — the drift and - # reachability probes are not even consulted (allow_stranded short-circuits). + def test_t5_allow_stranded_switches_even_offline( + self, event_loop, service, uow, romm, drift_probe, reachability_probe + ): + # "Switch anyway" overrides only the save gate. The target liveness probe + # still runs, but transport uncertainty fails open without a retry ladder. _seed_rom(uow, rom_id=1, app_id=_APP_ID) _seed_rom(uow, rom_id=2, app_id=None) _seed_install(uow, 1) drift_probe.drifted = True reachability_probe.online = False + romm.get_rom_once_side_effect_by_id[2] = ConnectionError("offline") result = _run(event_loop, service.switch_version(_APP_ID, 2, True)) _assert_success(result, rom_id=2, installed=False, launch_options="") assert drift_probe.calls == [] assert reachability_probe.calls == 0 + assert _romm_call_ids(romm, "get_rom_once") == [2] + assert _romm_call_ids(romm, "get_rom") == [] with uow as u: assert u.roms.get(2).shortcut_app_id == _APP_ID - def test_t6_synced_offline_switch_makes_no_server_contact( + def test_post_save_sync_retry_probes_target(self, event_loop, service, uow, romm, drift_probe): + _seed_rom(uow, rom_id=1, app_id=_APP_ID) + _seed_rom(uow, rom_id=2, app_id=None) + _seed_install(uow, 1) + drift_probe.drifted = True + + blocked = _run(event_loop, service.switch_version(_APP_ID, 2, False)) + assert blocked["reason"] == "unsynced_saves" + assert _romm_call_ids(romm, "get_rom_once") == [] + + drift_probe.drifted = False + retried = _run(event_loop, service.switch_version(_APP_ID, 2, False)) + + _assert_success(retried, rom_id=2, installed=False, launch_options="") + assert _romm_call_ids(romm, "get_rom_once") == [2] + + def test_switch_anyway_retry_probes_target(self, event_loop, service, uow, romm, drift_probe): + _seed_rom(uow, rom_id=1, app_id=_APP_ID) + _seed_rom(uow, rom_id=2, app_id=None) + _seed_install(uow, 1) + drift_probe.drifted = True + + blocked = _run(event_loop, service.switch_version(_APP_ID, 2, False)) + assert blocked["reason"] == "unsynced_saves" + assert _romm_call_ids(romm, "get_rom_once") == [] + + forced = _run(event_loop, service.switch_version(_APP_ID, 2, True)) + + _assert_success(forced, rom_id=2, installed=False, launch_options="") + assert _romm_call_ids(romm, "get_rom_once") == [2] + + def test_t6_synced_offline_switch_probe_fails_open( self, event_loop, service, uow, romm, drift_probe, reachability_probe ): - # Bound installed, saves synced, offline — switch is purely local, allowed, - # and touches neither the reachability probe nor the RomM transport. + # Bound installed, saves synced, offline — the optional exact-id probe is + # attempted once and fails open; no reachability or retrying detail call. _seed_rom(uow, rom_id=1, app_id=_APP_ID) _seed_rom(uow, rom_id=2, app_id=None) _seed_install(uow, 1) drift_probe.drifted = False reachability_probe.online = False + romm.get_rom_once_side_effect_by_id[2] = ConnectionError("offline") result = _run(event_loop, service.switch_version(_APP_ID, 2, False)) _assert_success(result, rom_id=2, installed=False, launch_options="") assert reachability_probe.calls == 0 - assert romm.call_log == [] + assert _romm_call_ids(romm, "get_rom_once") == [2] + assert _romm_call_ids(romm, "get_rom") == [] - def test_switch_to_active_is_noop_success(self, event_loop, service, uow, relaunch_resolver): + def test_switch_to_active_is_noop_success(self, event_loop, service, uow, romm, relaunch_resolver): _seed_rom(uow, rom_id=1, app_id=_APP_ID, name="Game 1") _seed_install(uow, 1) relaunch_resolver.items[1] = {"app_id": _APP_ID, "launch_options": "cmd"} result = _run(event_loop, service.switch_version(_APP_ID, 1, False)) _assert_success(result, rom_id=1, installed=True, launch_options="cmd") + assert _romm_call_ids(romm, "get_rom_once") == [] def test_resolver_miss_on_installed_target_still_succeeds(self, event_loop, service, uow, relaunch_resolver): # Installed target but the resolver unexpectedly returns nothing: the rebind @@ -764,21 +917,23 @@ def test_resolver_miss_on_installed_target_still_succeeds(self, event_loop, serv with uow as u: assert u.roms.get(2).shortcut_app_id == _APP_ID # binding committed - def test_bound_elsewhere_rejects(self, event_loop, service, uow): + def test_bound_elsewhere_rejects(self, event_loop, service, uow, romm): _seed_rom(uow, rom_id=1, app_id=_APP_ID) _seed_rom(uow, rom_id=2, app_id=777) # already a different shortcut result = _run(event_loop, service.switch_version(_APP_ID, 2, False)) assert result["success"] is False assert result["reason"] == "bound_elsewhere" + assert _romm_call_ids(romm, "get_rom_once") == [] - def test_local_target_other_group_not_in_group(self, event_loop, service, uow): + def test_local_target_other_group_not_in_group(self, event_loop, service, uow, romm): _seed_rom(uow, rom_id=1, app_id=_APP_ID, group_key=_GROUP) _seed_rom(uow, rom_id=2, app_id=None, group_key="igdb:999:57") result = _run(event_loop, service.switch_version(_APP_ID, 2, False)) assert result["success"] is False assert result["reason"] == "not_in_group" + assert _romm_call_ids(romm, "get_rom_once") == [] def test_not_in_group_message_is_human_readable(self, event_loop, service, uow): # The refusal message is human-readable and carries no raw internals (the @@ -824,6 +979,32 @@ async def __call__(self, rom_id: int) -> dict[str, Any]: assert result["success"] is False assert result["reason"] == "bound_elsewhere" + def test_probe_callback_race_still_hits_write_uow_bound_elsewhere_guard( + self, event_loop, service, uow, uow_factory, romm, monkeypatch + ): + _seed_rom(uow, rom_id=1, app_id=_APP_ID) + _seed_rom(uow, rom_id=2, app_id=None) + original_get_rom_once = romm.get_rom_once + + def rebind_during_probe(rom_id: int) -> dict[str, Any]: + response = original_get_rom_once(rom_id) + with uow_factory() as current: + target = current.roms.get(rom_id) + target.bind_shortcut(999) + current.roms.save(target) + return response + + monkeypatch.setattr(romm, "get_rom_once", rebind_during_probe) + + result = _run(event_loop, service.switch_version(_APP_ID, 2, False)) + + assert result["success"] is False + assert result["reason"] == "bound_elsewhere" + assert _romm_call_ids(romm, "get_rom_once") == [2] + with uow as current: + assert current.roms.get(1).shortcut_app_id == _APP_ID + assert current.roms.get(2).shortcut_app_id == 999 + def test_server_only_target_persisted_and_bound(self, event_loop, service, uow, romm): _seed_rom(uow, rom_id=1, app_id=_APP_ID, group_key=_GROUP) romm.roms[3] = { @@ -840,6 +1021,8 @@ def test_server_only_target_persisted_and_bound(self, event_loop, service, uow, result = _run(event_loop, service.switch_version(_APP_ID, 3, False)) _assert_success(result, rom_id=3, installed=False, launch_options="") + assert _romm_call_ids(romm, "get_rom_once") == [] + assert _romm_call_ids(romm, "get_rom") == [3] with uow as u: persisted = u.roms.get(3) assert persisted is not None @@ -851,6 +1034,51 @@ def test_server_only_target_persisted_and_bound(self, event_loop, service, uow, assert persisted.fs_size_bytes == 4_194_304 assert u.roms.get(1).shortcut_app_id is None + def test_vanished_bound_id_can_recover_to_live_local_target(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.get_rom_side_effect = RommNotFoundError("bound gone") + + before = _run(event_loop, service.get_version_list(_APP_ID)) + assert before["bound_vanished"] is True + + romm.call_log.clear() + romm.get_rom_side_effect = None + result = _run(event_loop, service.switch_version(_APP_ID, 2, False)) + + _assert_success(result, rom_id=2, installed=False, launch_options="") + assert _romm_call_ids(romm, "get_rom_once") == [2] + with uow as current: + assert current.roms.get(1).shortcut_app_id is None + assert current.roms.get(2).shortcut_app_id == _APP_ID + + def test_vanished_bound_id_can_recover_to_live_server_only_target(self, event_loop, service, uow, romm): + _seed_rom(uow, rom_id=1, app_id=_APP_ID) + romm.get_rom_side_effect = RommNotFoundError("bound gone") + before = _run(event_loop, service.get_version_list(_APP_ID)) + assert before["bound_vanished"] is True + + romm.call_log.clear() + romm.get_rom_side_effect = None + romm.roms[3] = { + "id": 3, + "name": "Game 3", + "fs_name": "game_3.sfc", + "platform_id": 57, + "platform_slug": "snes", + "igdb_id": 100, + "sibling_roms": [{"id": 1}], + } + + result = _run(event_loop, service.switch_version(_APP_ID, 3, False)) + + _assert_success(result, rom_id=3, installed=False, launch_options="") + assert _romm_call_ids(romm, "get_rom_once") == [] + assert _romm_call_ids(romm, "get_rom") == [3] + with uow as current: + assert current.roms.get(1).shortcut_app_id is None + assert current.roms.get(3).shortcut_app_id == _APP_ID + def test_uneven_coverage_server_only_switchable_and_adopts_bound_key(self, event_loop, service, uow, romm): # #1368: the bound group keys on igdb; a never-synced sibling matched only on # ss/hasheous (NO igdb) is canonical-compatible (absent at the canonical @@ -929,18 +1157,33 @@ def test_server_unreachable_on_target_fetch(self, event_loop, service, uow, romm assert result["reason"] == "server_unreachable" assert "error" not in result - def test_not_found_on_target_fetch(self, event_loop, service, uow, romm): - """A 404 on the switch target is not an offline server (#1570).""" + def test_server_only_target_auth_failure_keeps_classified_reason(self, event_loop, service, uow, romm): _seed_rom(uow, rom_id=1, app_id=_APP_ID) - romm.get_rom_side_effect = RommNotFoundError("HTTP 404: Not Found") + romm.get_rom_side_effect = RommAuthError("unauthorized") result = _run(event_loop, service.switch_version(_APP_ID, 3, False)) + assert result["success"] is False - assert result["reason"] == "not_found" - assert result["reason"] != "server_unreachable" - # The message must not claim reachability either — it used to read - # "RomM server not reachable." for every failure. - assert "not reachable" not in result["message"].lower() + assert result["reason"] == "auth_failed" + assert isinstance(result["message"], str) + assert _romm_call_ids(romm, "get_rom_once") == [] + assert _romm_call_ids(romm, "get_rom") == [3] + + def test_server_only_target_404_is_version_vanished_without_row(self, event_loop, service, uow, romm): + _seed_rom(uow, rom_id=1, app_id=_APP_ID) + romm.get_rom_side_effect = RommNotFoundError("HTTP 404: Not Found") + + result = _run(event_loop, service.switch_version(_APP_ID, 3, False)) + assert result == { + "success": False, + "reason": "version_vanished", + "message": "This version is no longer available on RomM.", + } + assert _romm_call_ids(romm, "get_rom_once") == [] + assert _romm_call_ids(romm, "get_rom") == [3] + with uow as current: + assert current.roms.get(1).shortcut_app_id == _APP_ID + assert current.roms.get(3) is None def test_server_target_unbuildable_is_invalid_target(self, event_loop, service, uow, romm): # The server detail is IN the group (its would-be key matches the bound