From 5ca24d873080bafb2d86919225c2e01f324d0081 Mon Sep 17 00:00:00 2001 From: danielcopper Date: Thu, 23 Jul 2026 18:08:51 +0200 Subject: [PATCH 1/7] fix(errors): report a definitive 404 as not_found, not server_unreachable RomM answers a request for an entity it no longer has with HTTP 404. Every exception-type-blind service site collapsed that onto server_unreachable, so the UI claimed "RomM offline" while the server was plainly answering. Route those sites through classify_error, which already maps RommNotFoundError to NOT_FOUND. Sites whose verdict is a partial-success flag rather than a reason slug peel the 404 off with a sibling except RommNotFoundError instead: - get_version_list returns the local-only list with server_query_failed False on a 404, so the picker stops feeding the global connection store. - get_save_status keeps server_query_failed True (the flag is what stops the matrix classifying local files as ready-to-upload against an empty server list) and gains the additive server_query_reason slug, so only an explicit server_unreachable drives the offline state. - list_file_versions and rollback_to_version gain a not_found arm, distinct from server_unreachable (retryable) and version_deleted (one save missing from a ROM the server still has). classify_error gains a narrow ConnectionError/TimeoutError/socket.gaierror branch so a raw socket failure keeps the unreachable slug. Deliberately not bare OSError: that also covers local disk I/O, and reporting a failed settings write as "server unreachable" is the same lie pointing the other way. Messages that asserted reachability are replaced ("RomM server not reachable.", "Cannot inspect slot - server unreachable"); messages already true for both branches are kept, since only the routing slug was wrong. scripts/check_404_not_unreachable.py locks this in: a catch-all handler in services/ may not bind a verdict key (reason/status/recommended_action) to a hardcoded SERVER_UNREACHABLE. The rule is positional rather than "does the handler call classify_error", so it also catches a handler that consults the funnel and then discards its verdict. It finds 14 sites against the previous tree. steamgrid.py is exempt with a recorded reason: it talks to SteamGridDB, whose failures classify_error has no branch for. --- py_modules/lib/errors.py | 13 +- py_modules/services/achievements.py | 8 +- py_modules/services/artwork.py | 8 +- py_modules/services/saves/slots/deletion.py | 10 +- py_modules/services/saves/slots/listing.py | 10 +- py_modules/services/saves/slots/switching.py | 5 +- py_modules/services/saves/status/service.py | 21 +++ .../services/saves/sync_engine/devices.py | 5 +- .../services/saves/sync_engine/rollback.py | 6 +- py_modules/services/saves/versions.py | 23 ++++ py_modules/services/version_switch.py | 27 +++- src/api/backend.ts | 5 +- src/components/CustomPlayButton.test.tsx | 33 ++++- src/components/CustomPlayButton.tsx | 16 ++- .../saves/VersionHistoryPanel.test.tsx | 28 ++++ src/components/saves/VersionHistoryPanel.tsx | 14 +- src/types/saves.ts | 16 ++- tests/contract/test_saves_read.py | 22 ++- tests/lib/test_errors.py | 31 +++++ .../saves/sync_engine/test_rollback.py | 36 ++++- tests/services/saves/test_service.py | 26 +++- tests/services/saves/test_slots.py | 127 +++++++++++++++++- tests/services/saves/test_status.py | 39 ++++++ tests/services/saves/test_versions.py | 61 +++++++++ tests/services/test_achievements.py | 45 +++++++ tests/services/test_artwork.py | 27 +++- tests/services/test_version_switch.py | 30 +++++ 27 files changed, 649 insertions(+), 43 deletions(-) diff --git a/py_modules/lib/errors.py b/py_modules/lib/errors.py index 9ca94b79..8b39dcd5 100644 --- a/py_modules/lib/errors.py +++ b/py_modules/lib/errors.py @@ -1,5 +1,6 @@ """RomM API error types for structured error handling.""" +import socket from typing import Any from lib.list_result import ErrorCode @@ -170,8 +171,8 @@ def classify_error(exc): ``reason`` is a canonical :class:`lib.list_result.ErrorCode` slug (returned as its string value). Several exception types fold onto one - slug \u2014 connection/timeout/SSL/5xx/generic-API all map to - ``server_unreachable``; 401 and 403 both map to ``auth_failed`` \u2014 but + slug \u2014 connection/timeout/SSL/5xx/generic-API plus raw socket errors + all map to ``server_unreachable``; 401/403 map to ``auth_failed`` \u2014 but each branch keeps a distinct human ``message``. In particular the 403 branch stays distinguishable from the 401 branch: a Cloudflare bot-fight 403 at the tunnel edge is not a wrong-credentials failure, so @@ -212,6 +213,14 @@ def classify_error(exc): ) if isinstance(exc, RommApiError): return ErrorCode.SERVER_UNREACHABLE.value, str(exc) + if isinstance(exc, (ConnectionError, TimeoutError, socket.gaierror)): + # A socket-level failure that never passed through the adapter's + # ``translate_http_error`` (a caller reaching the network outside the + # RomM client). Same reachability verdict as ``RommConnectionError``. + # Deliberately NOT bare ``OSError``: that also covers local disk I/O, + # so a failed settings write would report the server as unreachable — + # the same class of lie as reporting a 404 that way. + return ErrorCode.SERVER_UNREACHABLE.value, f"Server unreachable — {exc}" return ErrorCode.UNKNOWN.value, str(exc) diff --git a/py_modules/services/achievements.py b/py_modules/services/achievements.py index 9bc3c897..3ddca087 100644 --- a/py_modules/services/achievements.py +++ b/py_modules/services/achievements.py @@ -14,7 +14,7 @@ from typing import TYPE_CHECKING, Any from domain.achievements import extract_achievements_from_rom, extract_game_progress -from lib.list_result import ErrorCode +from lib.errors import classify_error if TYPE_CHECKING: import asyncio @@ -159,9 +159,10 @@ async def get_achievements(self, rom_id): "total": len(stale["achievements"]), "stale": True, } + reason, _message = classify_error(e) return { "success": False, - "reason": ErrorCode.SERVER_UNREACHABLE.value, + "reason": reason, "message": str(e), "achievements": [], "total": 0, @@ -219,9 +220,10 @@ async def get_achievement_progress(self, rom_id): stale_progress = self._achievements_cache.get(rom_id_str, {}).get("user_progress") if stale_progress: return {**self._progress_data_response(stale_progress), "stale": True} + reason, _message = classify_error(e) return { "success": False, - "reason": ErrorCode.SERVER_UNREACHABLE.value, + "reason": reason, "message": str(e), "earned": 0, "total": 0, diff --git a/py_modules/services/artwork.py b/py_modules/services/artwork.py index c8ea6e31..fc11fb82 100644 --- a/py_modules/services/artwork.py +++ b/py_modules/services/artwork.py @@ -56,7 +56,7 @@ ) from domain.cover_refresh import cover_ts_only_change, scan_cover_refresh_candidates from domain.sync_stage import SyncStage -from lib.errors import RommNotFoundError +from lib.errors import RommNotFoundError, classify_error from lib.list_result import ErrorCode @@ -823,9 +823,13 @@ async def refresh_cover(self, rom_id: int) -> dict[str, Any]: rom = await self._loop.run_in_executor(None, self._romm_api.get_rom, rom_id) except Exception as e: self._logger.warning(f"refresh_cover: failed to fetch rom {rom_id}: {e}") + reason, _message = classify_error(e) return { "success": False, - "reason": ErrorCode.SERVER_UNREACHABLE.value, + "reason": reason, + # Already true for both branches (the ROM is gone / the server + # is down), and more specific than the classifier's generic + # string — only the routing slug was ever wrong here. "message": "Could not fetch ROM from server", } if not rom: diff --git a/py_modules/services/saves/slots/deletion.py b/py_modules/services/saves/slots/deletion.py index 3c308f48..ef59524c 100644 --- a/py_modules/services/saves/slots/deletion.py +++ b/py_modules/services/saves/slots/deletion.py @@ -13,7 +13,7 @@ from typing import TYPE_CHECKING, Any from domain.save_slot import save_in_slot, slot_query_param -from lib.list_result import ErrorCode +from lib.errors import classify_error from services.saves._settings import save_sync_enabled if TYPE_CHECKING: @@ -128,10 +128,11 @@ async def get_slot_delete_info(self, rom_id: int, slot: str) -> dict[str, Any]: self._logger.warning( f"get_slot_delete_info: failed to list saves for slot '{slot}': {e}", ) + reason, message = classify_error(e) return { "success": False, - "reason": ErrorCode.SERVER_UNREACHABLE.value, - "message": "Cannot inspect slot — server unreachable", + "reason": reason, + "message": f"Cannot inspect slot — {message}", } # Local tracked files pointing to server saves in this slot @@ -178,9 +179,10 @@ async def _delete_server_slot_saves(self, rom_id: int, slot: str) -> dict[str, A return {"success": True, "count": len(save_ids), "ids": set(save_ids)} except Exception as e: self._logger.warning(f"delete_slot: server delete failed for slot '{slot}': {e}") + reason, _message = classify_error(e) return { "success": False, - "reason": ErrorCode.SERVER_UNREACHABLE.value, + "reason": reason, "message": f"Failed to delete server saves: {e}", } diff --git a/py_modules/services/saves/slots/listing.py b/py_modules/services/saves/slots/listing.py index c8556230..a66b1ef6 100644 --- a/py_modules/services/saves/slots/listing.py +++ b/py_modules/services/saves/slots/listing.py @@ -12,7 +12,7 @@ from domain.rom_save_sync_state import RomSaveSyncState from domain.save_slot import save_in_slot, slot_query_param -from lib.list_result import ErrorCode +from lib.errors import classify_error from services.saves._messages import SAVE_SYNC_DISABLED from services.saves._settings import resolve_default_slot, save_sync_enabled @@ -98,9 +98,12 @@ async def get_save_slots(self, rom_id: int) -> dict[str, Any]: ) except Exception as e: self._log_debug(f"Failed to fetch save slots for rom {rom_id}: {e}") + reason, _message = classify_error(e) return { "success": False, - "reason": ErrorCode.SERVER_UNREACHABLE, + "reason": reason, + # The raw exception text is neutral and carries the concrete + # detail; only the routing slug was ever wrong here. "message": str(e), "slots": [], "active_slot": active_slot, @@ -212,9 +215,10 @@ async def get_slot_saves(self, rom_id: int, slot: str) -> dict[str, Any]: ] return {"success": True, "slot": slot, "saves": saves} except Exception as e: + reason, _message = classify_error(e) return { "success": False, - "reason": ErrorCode.SERVER_UNREACHABLE, + "reason": reason, "message": str(e), "slot": slot, "saves": [], diff --git a/py_modules/services/saves/slots/switching.py b/py_modules/services/saves/slots/switching.py index 849a0417..b07c6e86 100644 --- a/py_modules/services/saves/slots/switching.py +++ b/py_modules/services/saves/slots/switching.py @@ -15,7 +15,7 @@ from domain.rom_save_sync_state import RomSaveSyncState from domain.save_layout import SAVE_SYNC_CONTENT_DIR_REASON from domain.save_slot import save_in_slot -from lib.list_result import ErrorCode +from lib.errors import classify_error from services.saves._helpers import newest_server_saves_by_target from services.saves._messages import SAVE_SYNC_IN_CONTENT_DIR from services.saves._settings import resolve_default_slot, save_sync_enabled @@ -237,9 +237,10 @@ async def switch_slot(self, rom_id: int, new_slot: str) -> dict[str, Any]: ), ) except Exception as e: + reason, _message = classify_error(e) return { "success": False, - "reason": ErrorCode.SERVER_UNREACHABLE.value, + "reason": reason, "message": str(e), } diff --git a/py_modules/services/saves/status/service.py b/py_modules/services/saves/status/service.py index dfab67bd..e2216cb9 100644 --- a/py_modules/services/saves/status/service.py +++ b/py_modules/services/saves/status/service.py @@ -15,6 +15,7 @@ status_from_action, ) from domain.sync_action import Conflict, Skip +from lib.errors import classify_error from services.saves._settings import save_sync_enabled if TYPE_CHECKING: @@ -170,6 +171,7 @@ def _get_save_status_io( server_saves: list[dict[str, Any]], *, server_query_failed: bool = False, + server_query_reason: str | None = None, ) -> dict[str, Any]: """Sync helper for get_save_status — runs in executor. @@ -309,6 +311,15 @@ def _get_save_status_io( "savefiles_in_content_dir": savefiles_in_content_dir, "save_sync_display": save_sync_display, "server_query_failed": server_query_failed, + # Why the query failed, as a :func:`classify_error` slug (None when + # it didn't). Additive alongside the flag, per CLAUDE.md's + # partial-success carve-out — do NOT collapse this into the + # canonical ``{success, reason, message}``: the response is a full + # payload the UI renders, and the binary ``success`` would erase + # that. The flag stays the "we lack the server's view" signal that + # drives ``status="unknown"``; this slug only says why, so a + # definitive 404 stops being reported as an offline server (#1570). + "server_query_reason": server_query_reason, # Interim #908 guard: a slot whose current save spans >1 distinct # file (e.g. Saturn .bkr/.bcr/.smpc) is an N-file *set*, not a # single file with a version history. Per-version rollback would @@ -337,6 +348,13 @@ async def get_save_status(self, rom_id: int) -> dict[str, Any]: the matrix-derived "ready to upload" label that an empty server list would otherwise produce — see ``_get_save_status_io``. + ``server_query_reason`` carries the :func:`classify_error` slug for + that failure (``None`` on success). The flag and the slug answer + different questions: the flag says the server's view is unknown — + true for a 404 as much as for an outage, and what keeps the matrix + from acting on an empty list — while the slug says *why*, so only an + explicit ``server_unreachable`` drives the UI's offline state (#1570). + The additive ``savefiles_in_content_dir: bool`` flag is ``True`` when RetroArch writes saves next to the ROM (the unsupported case): local probing is skipped and the display reads "Save sync off", @@ -353,6 +371,7 @@ async def get_save_status(self, rom_id: int) -> dict[str, Any]: server_saves: list[dict[str, Any]] = [] server_query_failed = False + server_query_reason: str | None = None try: device_id = await self._loop.run_in_executor(None, self._device_registry.get_device_id) server_saves = await self._loop.run_in_executor( @@ -362,6 +381,7 @@ async def get_save_status(self, rom_id: int) -> dict[str, Any]: except Exception as e: self._log_debug(f"Failed to fetch saves for rom {rom_id}: {e}") server_query_failed = True + server_query_reason, _message = classify_error(e) async with self._sync_engine.rom_lock(rom_id): return await self._loop.run_in_executor( @@ -370,6 +390,7 @@ async def get_save_status(self, rom_id: int) -> dict[str, Any]: rom_id, server_saves, server_query_failed=server_query_failed, + server_query_reason=server_query_reason, ), ) diff --git a/py_modules/services/saves/sync_engine/devices.py b/py_modules/services/saves/sync_engine/devices.py index c92bb227..4d997a6f 100644 --- a/py_modules/services/saves/sync_engine/devices.py +++ b/py_modules/services/saves/sync_engine/devices.py @@ -321,9 +321,12 @@ async def list_devices(self, *, loop: asyncio.AbstractEventLoop) -> dict[str, An return {"success": True, "devices": enriched} except Exception as e: self._log_debug(f"list_devices failed: {e}") + reason, _message = classify_error(e) return { "success": False, - "reason": ErrorCode.SERVER_UNREACHABLE.value, + "reason": reason, + # Neutral and true whichever way the call failed — only the + # routing slug was ever wrong here. "message": "Could not load devices", "devices": [], } diff --git a/py_modules/services/saves/sync_engine/rollback.py b/py_modules/services/saves/sync_engine/rollback.py index 01ad77a7..bc321e38 100644 --- a/py_modules/services/saves/sync_engine/rollback.py +++ b/py_modules/services/saves/sync_engine/rollback.py @@ -142,11 +142,11 @@ async def resolve( ), ) except Exception as e: - _code, _msg = classify_error(e) + reason, message = classify_error(e) return { "success": False, - "reason": ErrorCode.SERVER_UNREACHABLE.value, - "message": f"Failed to fetch saves: {_msg}", + "reason": reason, + "message": f"Failed to fetch saves: {message}", } active_slot = save_state.active_slot diff --git a/py_modules/services/saves/versions.py b/py_modules/services/saves/versions.py index 0f77e2cf..c8516bd1 100644 --- a/py_modules/services/saves/versions.py +++ b/py_modules/services/saves/versions.py @@ -20,6 +20,7 @@ from domain.save_layout import SAVE_SYNC_CONTENT_DIR_REASON from domain.save_slot import save_in_slot, slot_query_param from domain.save_status import compute_multi_file_slot +from lib.errors import RommNotFoundError from services.saves._helpers import local_save_target from services.saves._settings import resolve_default_slot @@ -143,6 +144,11 @@ async def list_file_versions(self, rom_id: int, slot: str, filename: str) -> dic ``list_saves`` call failed (network, server, auth, etc.). The frontend distinguishes this from an empty list so it can show a retry affordance instead of "no versions available". + - ``{"status": "not_found", "message": ...}`` if the ``list_saves`` + call drew a definitive 404 — RomM no longer has this ROM or the + registered device id. Distinct from ``server_unreachable``: the + server answered, so retrying is pointless and the panel says so + instead of blaming the connection (#1570). """ rom_id = int(rom_id) save_state, device_id = await self._loop.run_in_executor(None, self._read_inputs, rom_id) @@ -159,6 +165,11 @@ async def list_file_versions(self, rom_id: int, slot: str, filename: str) -> dic lambda: self._romm_api.list_saves(rom_id, device_id=device_id, slot=slot_query_param(slot)) ), ) + except RommNotFoundError as e: + # The server answered: it has no such ROM (or no such device id). A + # retry cannot change that, so the panel must not offer one (#1570). + self._log_debug(f"list_file_versions: server has no such entity: {e}") + return {"status": "not_found", "message": str(e)} except Exception as e: self._log_debug(f"list_file_versions: failed to list saves: {e}") return {"status": "server_unreachable", "message": str(e)} @@ -318,6 +329,11 @@ async def rollback_to_version(self, rom_id: int, slot: str, save_id: int) -> dic auth, etc.). The frontend distinguishes this from ``version_deleted`` so it can show a retry affordance instead of "version no longer on the server". + - ``{"status": "not_found", "message": ...}`` if that same call drew + a definitive 404 — RomM no longer has this ROM or the registered + device id. Distinct from both siblings: ``version_deleted`` is one + missing save inside a ROM the server still has, and + ``server_unreachable`` is a connection the user can retry (#1570). - ``{"status": "conflict_blocked", "conflicts": [...]}`` if the pre-flight surfaced a conflict on the currently-tracked save. The frontend resolves it via the standard conflict modal. @@ -387,6 +403,13 @@ async def rollback_to_version(self, rom_id: int, slot: str, save_id: int) -> dic lambda: self._romm_api.list_saves(rom_id, device_id=device_id, slot=slot_query_param(slot)) ), ) + except RommNotFoundError as e: + # The server answered: it has no such ROM (or no such device id). + # Not a connectivity verdict, so the toast must not blame the + # connection or invite a retry that cannot succeed (#1570). + self._log_debug(f"rollback_to_version: server has no such entity: {e}") + await self._loop.run_in_executor(None, self._write_save_state, rom_id, save_state) + return {"status": "not_found", "message": str(e)} except Exception as e: self._log_debug(f"rollback_to_version: failed to list saves: {e}") # Persist whatever the pre-flight mutated before bailing. diff --git a/py_modules/services/version_switch.py b/py_modules/services/version_switch.py index 1a6a8792..0697c5bc 100644 --- a/py_modules/services/version_switch.py +++ b/py_modules/services/version_switch.py @@ -35,6 +35,7 @@ from domain.sibling_group import compute_sibling_group_key, target_in_sibling_group from domain.sibling_resolution import AUTO_REGION, resolve_group_representative from domain.version_metadata import VersionMetadata +from lib.errors import RommNotFoundError, classify_error from lib.list_result import ErrorCode if TYPE_CHECKING: @@ -173,7 +174,10 @@ async def get_version_list(self, app_id: int) -> dict[str, Any]: switch the backend would reject. 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. + 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 + the local-only list carries ``server_query_failed: False`` and the picker + leaves the shared connection state alone (#1570). """ app_id = int(app_id) local = await self._loop.run_in_executor(None, self._read_local_group, app_id) @@ -182,6 +186,15 @@ async def get_version_list(self, app_id: int) -> dict[str, Any]: try: bound_detail = await self._loop.run_in_executor(None, self._romm_api.get_rom, local.bound_rom_id) + except RommNotFoundError: + # The server ANSWERED — it just no longer has the bound id. That is a + # statement about the ROM, not about reachability, so the local-only + # 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") + return self._build_version_list( + local, server_only_stubs=[], detail_by_id={}, stub_local_group_keys={}, 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( @@ -462,7 +475,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``; - an unreachable server on a server-only target fetch → ``server_unreachable``. + 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). """ app_id = int(app_id) target_rom_id = int(target_rom_id) @@ -518,12 +534,13 @@ 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: # surfaced as the canonical unreachable failure + except Exception as e: # classified: a 404 on the target is not an offline server self._logger.warning(f"Version switch: target fetch failed for rom {target_rom_id}: {e}") + reason, message = classify_error(e) return { "success": False, - "reason": ErrorCode.SERVER_UNREACHABLE.value, - "message": "RomM server not reachable.", + "reason": reason, + "message": message, } # Canonical compatibility against the bound key: the target's id at the diff --git a/src/api/backend.ts b/src/api/backend.ts index cdca121f..b915c49f 100755 --- a/src/api/backend.ts +++ b/src/api/backend.ts @@ -443,7 +443,10 @@ 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). + * (partial-success carve-out). A definitive 404 on the bound id is NOT such a + * failure — the server answered, it just no longer has that ROM — so the + * local-only list comes back with `server_query_failed: false` and the picker + * leaves the shared connection state alone (#1570). */ export interface VersionList { multi_version: boolean; diff --git a/src/components/CustomPlayButton.test.tsx b/src/components/CustomPlayButton.test.tsx index 6e2d485a..dc95de8f 100644 --- a/src/components/CustomPlayButton.test.tsx +++ b/src/components/CustomPlayButton.test.tsx @@ -82,7 +82,7 @@ vi.mock("../components/SyncConflictModal", () => ({ })); import { getCachedGameDetail } from "../utils/cachedGameDetailStore"; -import { setRommConnectionState, reportServerReachable } from "../utils/connectionState"; +import { setRommConnectionState, reportServerReachable, getRommConnectionState } from "../utils/connectionState"; import { setLaunchOptionsConfirmed } from "../utils/steamShortcuts"; import { markLaunchSkipped, consumeLaunchSkip } from "../utils/launchGate"; import { getMigrationState } from "../utils/migrationStore"; @@ -1265,6 +1265,37 @@ describe("CustomPlayButton — resolve conflict reads the known conflict (#1276) globalThis.removeEventListener("romm_data_changed", dataChanged); }); + it("server_query_failed with an unreachable reason DOES drive the store offline", async () => { + setRommConnectionState("connected"); + vi.mocked(backend.getSaveStatus).mockResolvedValue( + saveStatus({ server_query_failed: true, server_query_reason: "server_unreachable", conflicts: [] }), + ); + + const utils = await renderInConflict(); + await clickResolve(utils); + + // Post-catch state: the global store actually flipped. + expect(getRommConnectionState()).toBe("offline"); + await utils.findByText("Resolve Conflict"); + }); + + it("server_query_failed with a not_found reason leaves the store ALONE", async () => { + setRommConnectionState("connected"); + vi.mocked(backend.getSaveStatus).mockResolvedValue( + saveStatus({ server_query_failed: true, server_query_reason: "not_found", conflicts: [] }), + ); + + const utils = await renderInConflict(); + await clickResolve(utils); + + // The #1570 defect: a definitive 404 is the server ANSWERING, so feeding + // the global store off the bare flag blacked out the whole UI. The button + // must still refuse to claim the conflict was resolved. + expect(getRommConnectionState()).toBe("connected"); + expect(vi.mocked(handleConflicts)).not.toHaveBeenCalled(); + await utils.findByText("Resolve Conflict"); + }); + it("a thrown getSaveStatus keeps the button in conflict and toasts", async () => { vi.mocked(backend.getSaveStatus).mockRejectedValue(new Error("network down")); diff --git a/src/components/CustomPlayButton.tsx b/src/components/CustomPlayButton.tsx index 28207c20..bf1c954f 100644 --- a/src/components/CustomPlayButton.tsx +++ b/src/components/CustomPlayButton.tsx @@ -730,12 +730,18 @@ export const CustomPlayButton: FC = ({ appId }) => { // N new Promise((_, reject) => setTimeout(() => reject(new Error("timeout")), 15000)), ]); - // A connectivity blip during the status read leaves every file "unknown" - // and an empty server list; treating that as "resolved" would drop the - // user back to Play believing the conflict was cleared. Surface it and - // stay in conflict, exactly like the network-throw catch below (#1276). + // A failed status read leaves every file "unknown" and an empty server + // list; treating that as "resolved" would drop the user back to Play + // believing the conflict was cleared. Surface it and stay in conflict, + // exactly like the network-throw catch below (#1276). if (result.server_query_failed) { - reportServerReachable(false); + // Only an explicit unreachable verdict is a connectivity signal. A + // definitive 404 means the server ANSWERED, so feeding the global + // store off the bare flag would black out the whole UI over a ROM + // the server merely no longer has (#1570) — see connectionState.ts. + if (result.server_query_reason === "server_unreachable") { + reportServerReachable(false); + } detach(debugLog(`CustomPlayButton: resolve conflict — server query failed for rom ${romId}`)); toaster.toast({ title: "RomM Sync", body: "Couldn't reach server to resolve conflict" }); setState("conflict"); diff --git a/src/components/saves/VersionHistoryPanel.test.tsx b/src/components/saves/VersionHistoryPanel.test.tsx index 7cd701c2..fa04b956 100644 --- a/src/components/saves/VersionHistoryPanel.test.tsx +++ b/src/components/saves/VersionHistoryPanel.test.tsx @@ -147,6 +147,19 @@ describe("VersionHistoryPanel", () => { expect(getByText("Retry")).toBeInTheDocument(); }); + it("shows the entity-gone body, NOT the connection prompt, on a definitive 404", async () => { + vi.mocked(backend.savesListFileVersions).mockResolvedValue({ + status: "not_found", + message: "HTTP 404: Not Found", + }); + const { getByText, container } = render(); + fireEvent.click(getByText("Previous Versions")); + await flushAsync(); + expect(container.textContent).toContain("RomM no longer has this game's saves."); + // The server answered — blaming the connection is the #1570 defect. + expect(container.textContent).not.toContain("Couldn't reach RomM"); + }); + it("Retry button retriggers loadVersions", async () => { vi.mocked(backend.savesListFileVersions) .mockResolvedValueOnce({ status: "server_unreachable", message: "boom" }) @@ -368,6 +381,21 @@ describe("VersionHistoryPanel", () => { ); }); + it("status 'not_found' toasts the entity-gone message, not the connection prompt", async () => { + vi.mocked(backend.savesRollbackToVersion).mockResolvedValue({ + status: "not_found", + message: "HTTP 404: Not Found", + }); + const { getByText, onRestored } = await expand(); + fireEvent.click(getByText("Restore")); + await flushAsync(); + await flushAsync(); + expect(vi.mocked(toaster.toast)).toHaveBeenCalledWith( + expect.objectContaining({ body: "RomM no longer has this game's saves — nothing was restored." }), + ); + expect(onRestored).not.toHaveBeenCalled(); + }); + it("status 'unsupported' toasts the version requirement", async () => { vi.mocked(backend.savesRollbackToVersion).mockResolvedValue({ status: "unsupported", diff --git a/src/components/saves/VersionHistoryPanel.tsx b/src/components/saves/VersionHistoryPanel.tsx index 51d86393..82059327 100644 --- a/src/components/saves/VersionHistoryPanel.tsx +++ b/src/components/saves/VersionHistoryPanel.tsx @@ -50,6 +50,13 @@ export const VersionHistoryPanel: FC = ({ // siblings are components, not prior versions (#908). The panel is // hidden for multi-file slots anyway; this is the defensive backstop. setVersions(result.versions); + } else if (result.status === "not_found") { + // The server answered: it has no such ROM / device id. Saying + // "couldn't reach RomM" here would blame a connection that is + // plainly working (#1570). + detach(debugLog(`VersionHistoryPanel: server has no such entry for ${filename}: ${result.message}`)); + setVersions(null); + setLoadError("RomM no longer has this game's saves."); } else { detach(debugLog(`VersionHistoryPanel: server unreachable for ${filename}: ${result.message}`)); setVersions(null); @@ -148,7 +155,12 @@ export const VersionHistoryPanel: FC = ({ // we just couldn't reach the server to confirm. Prompt for retry // instead of telling the user the version is gone. toaster.toast({ title: "RomM Sync", body: "Couldn't reach RomM. Check your connection and try again." }); - // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- exhaustive final branch of the 8-member RollbackStatus union; an explicit check (vs. plain `else`) keeps the per-status symmetry and leaves any future-added status unhandled instead of silently routing it to the "unsupported" toast + } else if (result.status === "not_found") { + // The mirror image of the branch above: RomM answered that it no + // longer has this game's saves, so a retry cannot help and the + // toast must not send the user off to check their connection (#1570). + toaster.toast({ title: "RomM Sync", body: "RomM no longer has this game's saves — nothing was restored." }); + // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- exhaustive final branch of the 9-member RollbackStatus union; an explicit check (vs. plain `else`) keeps the per-status symmetry and leaves any future-added status unhandled instead of silently routing it to the "unsupported" toast } else if (result.status === "unsupported") { toaster.toast({ title: "RomM Sync", body: "Version history requires RomM 4.7+" }); } diff --git a/src/types/saves.ts b/src/types/saves.ts index 3f1cfe60..ad8e45ec 100644 --- a/src/types/saves.ts +++ b/src/types/saves.ts @@ -4,6 +4,8 @@ * Anything related to RomM save synchronization lives here. */ +import type { RommErrorCode } from "./api"; + export interface SaveSyncSettings { save_sync_enabled: boolean; sync_before_launch: boolean; @@ -91,6 +93,12 @@ export interface SaveStatus { * and surface a misleading uploads-pending indicator on what is in * fact a connectivity blip. */ server_query_failed?: boolean; + /** Why that query failed, as a backend `classify_error` slug (`null` when + * it didn't). The flag says the server's view is unknown — true for a + * definitive 404 as much as for an outage — while this says *why*, so + * only `"server_unreachable"` may drive the global connection store. + * Feeding the store off the bare flag is the #1570 defect. */ + server_query_reason?: RommErrorCode | null; /** True when the active slot's current save spans more than one distinct * file (e.g. Sega Saturn `.bkr`/`.bcr`/`.smpc`). Those siblings are * components of one game state, not prior versions — so the frontend @@ -225,6 +233,10 @@ export type RollbackStatus = | { status: "version_deleted" } | { status: "unsupported" } | { status: "server_unreachable"; message: string } + // The server ANSWERED with a definitive 404 — it no longer has this ROM or + // the registered device id. Distinct from `server_unreachable` (retryable) + // and from `version_deleted` (one missing save inside a ROM it still has). + | { status: "not_found"; message: string } | { status: "conflict_blocked"; conflicts: SyncConflict[] } | { status: "preflight_failed"; errors: string[] } | { status: "put_failed"; message: string }; @@ -232,7 +244,9 @@ export type RollbackStatus = export type ListFileVersionsResult = | { status: "ok"; versions: SaveVersionEntry[] } | { status: "multi_file_unsupported"; versions: SaveVersionEntry[] } - | { status: "server_unreachable"; message: string }; + | { status: "server_unreachable"; message: string } + // See RollbackStatus — the server answered, it just has no such entry. + | { status: "not_found"; message: string }; /** Discriminated-status result of `copySaveToSlot` — copies one server save into * a target slot, which becomes the ROM's active slot (the source is preserved). diff --git a/tests/contract/test_saves_read.py b/tests/contract/test_saves_read.py index 74deecd7..d382d3e4 100644 --- a/tests/contract/test_saves_read.py +++ b/tests/contract/test_saves_read.py @@ -25,7 +25,7 @@ import os from domain.rom_save_sync_state import RomSaveSyncState -from lib.errors import RommConnectionError +from lib.errors import RommConnectionError, RommNotFoundError from lib.list_result import ErrorCode from ._seed import enable_save_sync, seed_confirmed_slot, seed_install, seed_rom, seed_save_state, seed_server_save @@ -57,6 +57,7 @@ async def test_get_save_status_full_shape_and_partial_flag(harness): "savefiles_in_content_dir", "save_sync_display", "server_query_failed", + "server_query_reason", "multi_file", "component_files", "rollback_supported", @@ -68,6 +69,8 @@ async def test_get_save_status_full_shape_and_partial_flag(harness): # Partial-success carve-out: the additive flag is present and a bool. assert isinstance(result["server_query_failed"], bool) assert result["server_query_failed"] is False + # Its companion slug is None while nothing failed. + assert result["server_query_reason"] is None async def test_get_save_status_server_failure_keeps_full_payload(harness): @@ -78,11 +81,28 @@ async def test_get_save_status_server_failure_keeps_full_payload(harness): # The carve-out: a failure flag rides alongside the full payload, not a # bare {success: False}. The frontend still renders local state. assert result["server_query_failed"] is True + assert result["server_query_reason"] == "server_unreachable" assert result["rom_id"] == 42 assert "files" in result assert "save_sync_display" in result +async def test_get_save_status_definitive_404_is_not_unreachable(harness): + """A 404 keeps the flag but carries the not_found slug (#1570). + + Over the real wire: the flag must stay True (the matrix must not act on + an empty server list), while only the slug tells the frontend whether + this is a connectivity verdict. + """ + enable_save_sync(harness) + harness.romm.list_saves_side_effect = RommNotFoundError("HTTP 404: Not Found") + result = await harness.plugin.get_save_status(42) + assert result["server_query_failed"] is True + assert result["server_query_reason"] == "not_found" + assert result["server_query_reason"] != "server_unreachable" + assert result["rom_id"] == 42 + + async def test_get_save_status_pending_upload_display(harness): """A local file with no server save is pending upload → save_sync_display reports the new pending/'Local changes' value, never a green 'synced' (#1334). The nested diff --git a/tests/lib/test_errors.py b/tests/lib/test_errors.py index e2c25149..879f7ce5 100644 --- a/tests/lib/test_errors.py +++ b/tests/lib/test_errors.py @@ -1,3 +1,5 @@ +import socket + import pytest from lib.errors import ( @@ -237,6 +239,35 @@ def test_conflict_error_is_api_error(self): assert code == "server_unreachable" assert msg == "conflict" + def test_raw_connection_error_is_server_unreachable(self): + """A socket failure that never passed through the adapter's translation. + + AC of #1570: routing sites through classify_error must not downgrade a + genuine transport failure to ``unknown``. + """ + code, msg = classify_error(ConnectionError("connection refused")) + assert code == "server_unreachable" + assert "connection refused" in msg + + def test_raw_timeout_error_is_server_unreachable(self): + code, _msg = classify_error(TimeoutError("timed out")) + assert code == "server_unreachable" + + def test_dns_failure_is_server_unreachable(self): + code, _msg = classify_error(socket.gaierror("Name or service not known")) + assert code == "server_unreachable" + + def test_bare_oserror_stays_unknown(self): + """Deliberately NOT server_unreachable — OSError also covers local disk. + + A failed settings write raises OSError; classifying that as "the server + is unreachable" is the same class of lie #1570 removes, pointing the + other way. Only the socket-shaped subclasses above are transport. + """ + code, msg = classify_error(OSError("No space left on device")) + assert code == "unknown" + assert msg == "No space left on device" + def test_unknown_exception_value_error(self): code, msg = classify_error(ValueError("bad value")) assert code == "unknown" diff --git a/tests/services/saves/sync_engine/test_rollback.py b/tests/services/saves/sync_engine/test_rollback.py index a74411fb..935157e6 100644 --- a/tests/services/saves/sync_engine/test_rollback.py +++ b/tests/services/saves/sync_engine/test_rollback.py @@ -12,7 +12,7 @@ from domain.rom_save_sync_state import FileSyncState, RomSaveSyncState from domain.save_layout import ContentDir -from lib.errors import RommApiError +from lib.errors import RommApiError, RommNotFoundError from tests.services.saves._helpers import ( _create_save, _enable_sync_with_device, @@ -335,10 +335,44 @@ async def test_resolve_server_fetch_failure(self, tmp_path): ) assert result["success"] is False + assert result["reason"] == "server_unreachable" assert "Failed to fetch saves" in result["message"] # State left as-is — no mutation assert _get_save_state(svc, 42) == original_state + @pytest.mark.asyncio + async def test_resolve_list_saves_404_is_not_found(self, tmp_path): + """The classified verdict is USED, not discarded (#1570). + + This site already called classify_error and then threw its verdict + away, keeping only the message — so every failure, 404 included, + reported the server as unreachable. + """ + svc, fake = make_service(tmp_path) + _enable_sync_with_device(svc) + _install_rom(svc, tmp_path) + _create_save(tmp_path, content=b"x") + + original_state = RomSaveSyncState( + files={"pokemon.srm": FileSyncState(tracked_save_id=100, last_sync_hash="abc")}, + ) + _seed_save_state(svc, 42, original_state) + + fake.fail_on_next(RommNotFoundError("HTTP 404: Not Found")) + + result = await svc.resolve_sync_conflict( + rom_id=42, + filename="pokemon.srm", + server_save_id=100, + action="keep_local", + ) + + assert result["success"] is False + assert result["reason"] == "not_found" + assert result["reason"] != "server_unreachable" + # State left as-is — no mutation + assert _get_save_state(svc, 42) == original_state + @pytest.mark.asyncio async def test_resolve_no_server_saves_in_slot(self, tmp_path): """Empty slot post-fetch returns success=False with a clear message. diff --git a/tests/services/saves/test_service.py b/tests/services/saves/test_service.py index abeee08e..874f3082 100644 --- a/tests/services/saves/test_service.py +++ b/tests/services/saves/test_service.py @@ -16,7 +16,7 @@ from domain.iso_time import epoch_to_iso from domain.rom_save_sync_state import FileSyncState, RomSaveSyncState -from lib.errors import RommConnectionError +from lib.errors import RommConnectionError, RommNotFoundError from services.saves._settings import resolve_default_slot, sanitize_setting from tests.services.saves._helpers import ( _create_save, @@ -286,17 +286,37 @@ async def test_list_devices_save_sync_disabled(self, tmp_path): @pytest.mark.asyncio async def test_list_devices_adapter_error(self, tmp_path): - """Adapter raises — returns error response.""" + """Adapter raises a transport error — returns the unreachable response.""" fake = FakeSaveApi() svc, _ = make_service(tmp_path, fake_api=fake) svc._config.settings["save_sync_enabled"] = True - fake.fail_on_next(Exception("server unavailable")) + fake.fail_on_next(RommConnectionError("server unavailable")) result = await svc.list_devices() assert result["success"] is False assert result["reason"] == "server_unreachable" + @pytest.mark.asyncio + async def test_list_devices_not_found(self, tmp_path): + """A definitive 404 is the server ANSWERING — not an outage (#1570). + + The message stays neutral ("Could not load devices") because it is + already true either way; only the routing slug was wrong. + """ + fake = FakeSaveApi() + svc, _ = make_service(tmp_path, fake_api=fake) + svc._config.settings["save_sync_enabled"] = True + + fake.fail_on_next(RommNotFoundError("HTTP 404: Not Found")) + result = await svc.list_devices() + + assert result["success"] is False + assert result["reason"] == "not_found" + assert result["reason"] != "server_unreachable" + assert result["message"] == "Could not load devices" + assert result["devices"] == [] + @pytest.mark.asyncio async def test_list_devices_no_own_id_all_false(self, tmp_path): """No server_device_id in state — all is_current_device are False.""" diff --git a/tests/services/saves/test_slots.py b/tests/services/saves/test_slots.py index 985bc720..5878e02c 100644 --- a/tests/services/saves/test_slots.py +++ b/tests/services/saves/test_slots.py @@ -8,7 +8,7 @@ from domain.rom_save_sync_state import FileSyncState, RomSaveSyncState from domain.save_layout import ContentDir -from lib.errors import RommApiError, RommConnectionError +from lib.errors import RommApiError, RommConnectionError, RommNotFoundError from lib.list_result import ErrorCode from tests.services.saves._helpers import ( _create_save, @@ -129,7 +129,7 @@ async def test_get_save_slots_preserves_map_on_api_failure(self, tmp_path): ), ) - fake.fail_on_next(OSError("connection refused")) + fake.fail_on_next(ConnectionError("connection refused")) result = await svc.get_save_slots(123) @@ -142,6 +142,37 @@ async def test_get_save_slots_preserves_map_on_api_failure(self, tmp_path): # Persisted slot map is untouched (no merge / overwrite happened). assert _require_save_state(svc, 123).slots == original_slots + @pytest.mark.asyncio + async def test_get_save_slots_404_is_not_found_and_still_preserves_map(self, tmp_path): + """A 404 is not an outage — and the #625 map-preservation still holds (#1570).""" + svc, fake = make_service(tmp_path) + svc._config.settings["save_sync_enabled"] = True + _set_device_id(svc, "dev-1") + + original_slots = { + "default": {"source": "server", "count": 1, "latest_updated_at": "2026-04-17T10:00:00"}, + "save1": {"source": "server", "count": 3, "latest_updated_at": "2026-04-16T09:00:00"}, + } + _seed_save_state( + svc, + 123, + RomSaveSyncState( + active_slot="default", + slot_confirmed=True, + slots=dict(original_slots), + ), + ) + + fake.fail_on_next(RommNotFoundError("HTTP 404: Not Found")) + + result = await svc.get_save_slots(123) + + assert result["success"] is False + assert result["reason"] == "not_found" + assert result["reason"] != "server_unreachable" + # The #625 guard is unchanged by the reclassification. + assert _require_save_state(svc, 123).slots == original_slots + @pytest.mark.asyncio async def test_get_save_slots_empty_server_response_persists(self, tmp_path): """A genuine empty server response is success and persists correctly. @@ -1037,6 +1068,21 @@ async def test_server_error(self, tmp_path): assert result["saves"] == [] assert "connection timeout" in result["message"] + @pytest.mark.asyncio + async def test_definitive_404_is_not_found(self, tmp_path): + """A 404 is the server ANSWERING — the slot panel must not claim offline (#1570).""" + svc, fake = make_service(tmp_path) + svc._config.settings["save_sync_enabled"] = True + _set_device_id(svc, "server-dev-1") + fake.fail_on_next(RommNotFoundError("HTTP 404: Not Found")) + + result = await svc.get_slot_saves(42, "default") + + assert result["success"] is False + assert result["reason"] == "not_found" + assert result["reason"] != "server_unreachable" + assert result["saves"] == [] + @pytest.mark.asyncio async def test_sync_disabled(self, tmp_path): """Returns error response when save sync is disabled.""" @@ -1206,6 +1252,24 @@ async def test_server_unreachable(self, tmp_path): assert result["success"] is False assert result["reason"] == "server_unreachable" + @pytest.mark.asyncio + async def test_definitive_404_is_not_found(self, tmp_path): + """A 404 blocks the switch too, but as not_found — the server answered (#1570).""" + svc, fake = make_service(tmp_path) + svc._config.settings["save_sync_enabled"] = True + _install_rom(svc, tmp_path) + save_path = _create_save(tmp_path) + local_hash = _file_md5(str(save_path)) + _seed_save_state(svc, 42, self._synced_state(local_hash)) + + fake.fail_on_next(RommNotFoundError("HTTP 404: Not Found")) + + result = await svc.switch_slot(42, "desktop") + + assert result["success"] is False + assert result["reason"] == "not_found" + assert result["reason"] != "server_unreachable" + @pytest.mark.asyncio async def test_sync_disabled(self, tmp_path): """Save sync disabled → immediate error, no API calls.""" @@ -1951,13 +2015,16 @@ async def test_get_slot_delete_info_server_unreachable(self, tmp_path): "pokemon.srm": {"tracked_save_id": 10, "last_sync_hash": "abc"}, }, ) - fake.fail_on_next(OSError("connection refused")) + fake.fail_on_next(ConnectionError("connection refused")) result = await svc.get_slot_delete_info(42, "save1") assert result["success"] is False assert result["reason"] == "server_unreachable" assert "message" in result + # The message asserted reachability before #1570; it must stay honest + # while still refusing the destructive confirm. + assert "Cannot inspect slot" in result["message"] # Drift guard: the legacy duplicate ``error`` field was dropped in #652. # Frontend now reads ``reason`` only. Re-adding ``error`` would # reintroduce the dual-write that was deliberately removed. @@ -1967,6 +2034,34 @@ async def test_get_slot_delete_info_server_unreachable(self, tmp_path): assert "server_save_count" not in result assert "server_save_ids" not in result + @pytest.mark.asyncio + async def test_get_slot_delete_info_definitive_404_is_not_found(self, tmp_path): + """A 404 still refuses the confirm, but does not blame the connection (#1570). + + The #626 safety property is unchanged — no fake "0 saves" count reaches + the modal — only the slug and the message stop claiming an outage. + """ + svc, fake = make_service(tmp_path) + self._setup_state_with_slots( + svc, + tmp_path, + extra_slots={"save1": {"source": "server", "count": 3, "latest_updated_at": None}}, + files_state={ + "pokemon.srm": {"tracked_save_id": 10, "last_sync_hash": "abc"}, + }, + ) + fake.fail_on_next(RommNotFoundError("HTTP 404: Not Found")) + + result = await svc.get_slot_delete_info(42, "save1") + + assert result["success"] is False + assert result["reason"] == "not_found" + assert result["reason"] != "server_unreachable" + assert "unreachable" not in result["message"].lower() + # The #626 guard still holds. + assert "server_save_count" not in result + assert "server_save_ids" not in result + @pytest.mark.asyncio async def test_get_slot_delete_info_local_slot_unaffected_by_server_failure(self, tmp_path): """Local-source slots skip the API call and still succeed when the server is down.""" @@ -2159,6 +2254,32 @@ def fail_delete(save_ids): fake.delete_server_saves = original_delete + @pytest.mark.asyncio + async def test_delete_slot_server_404_is_not_found_and_rolls_back(self, tmp_path): + """A 404 on the delete is not an outage; the rollback guard is unchanged (#1570).""" + svc, fake = make_service(tmp_path) + self._setup_state_with_slots( + svc, + tmp_path, + extra_slots={"save1": {"source": "server", "count": 1, "latest_updated_at": None}}, + ) + fake.saves[10] = _server_save(save_id=10, rom_id=42, filename="pokemon.srm", slot="save1") + original_delete = fake.delete_server_saves + + def fail_delete(save_ids): + raise RommNotFoundError("HTTP 404: Not Found") + + fake.delete_server_saves = fail_delete + + result = await svc.delete_slot(42, "save1") + + assert result["success"] is False + assert result["reason"] == "not_found" + assert result["reason"] != "server_unreachable" + assert "save1" in _require_save_state(svc, 42).slots + + fake.delete_server_saves = original_delete + @pytest.mark.asyncio async def test_delete_slot_cleans_up_tracked_files(self, tmp_path): """Only file entries pointing to deleted saves are removed; unrelated entries preserved.""" diff --git a/tests/services/saves/test_status.py b/tests/services/saves/test_status.py index ec8cec51..b0117362 100644 --- a/tests/services/saves/test_status.py +++ b/tests/services/saves/test_status.py @@ -7,6 +7,7 @@ from domain.rom_save_sync_state import RomSaveSyncState from domain.save_layout import ContentDir, InSaveDir +from lib.errors import RommConnectionError, RommNotFoundError from tests.services.saves._helpers import ( _create_save, _do_sync, @@ -621,6 +622,43 @@ async def test_list_saves_failure_sets_flag_and_marks_status_unknown(self, tmp_p "last_sync_check_at": None, } + @pytest.mark.asyncio + async def test_transport_failure_carries_the_unreachable_reason(self, tmp_path): + """The flag says the server's view is unknown; the slug says why (#1570).""" + svc, fake = make_service(tmp_path) + _enable_sync_with_device(svc) + _install_rom(svc, tmp_path) + _create_save(tmp_path) + fake.fail_on_next(RommConnectionError("connection reset")) + + result = await svc.get_save_status(42) + + assert result["server_query_failed"] is True + assert result["server_query_reason"] == "server_unreachable" + + @pytest.mark.asyncio + async def test_definitive_404_keeps_the_flag_but_is_not_unreachable(self, tmp_path): + """A 404 must NOT read as offline — and must NOT clear the flag either. + + The flag is what suppresses the matrix's "ready to upload" verdict + against an empty server list, so it stays True: we genuinely do not + know the server's saves. Only the reason changes, which is what stops + the frontend flipping the whole UI to "RomM offline" (#1570). + """ + svc, fake = make_service(tmp_path) + _enable_sync_with_device(svc) + _install_rom(svc, tmp_path) + _create_save(tmp_path) + fake.fail_on_next(RommNotFoundError("HTTP 404: Not Found")) + + result = await svc.get_save_status(42) + + assert result["server_query_reason"] == "not_found" + assert result["server_query_reason"] != "server_unreachable" + # The safety behaviour is unchanged — this is a classification fix only. + assert result["server_query_failed"] is True + assert result["files"][0]["status"] == "unknown" + @pytest.mark.asyncio async def test_happy_path_flag_is_false(self, tmp_path): """Successful list_saves preserves the normal flow with the flag set False.""" @@ -635,6 +673,7 @@ async def test_happy_path_flag_is_false(self, tmp_path): result = await svc.get_save_status(42) assert result["server_query_failed"] is False + assert result["server_query_reason"] is None assert len(result["files"]) >= 1 # Real matrix verdict surfaced (not redacted). assert result["files"][0]["status"] != "unknown" diff --git a/tests/services/saves/test_versions.py b/tests/services/saves/test_versions.py index d4da0c3c..049edfd5 100644 --- a/tests/services/saves/test_versions.py +++ b/tests/services/saves/test_versions.py @@ -5,6 +5,7 @@ import pytest from domain.save_layout import ContentDir +from lib.errors import RommNotFoundError from tests.services.saves._helpers import ( _create_save, _enable_sync_with_device, @@ -221,6 +222,28 @@ async def test_api_error_returns_server_unreachable(self, tmp_path): # Must NOT be the same shape as the happy-path empty case. assert result != {"status": "ok", "versions": []} + @pytest.mark.asyncio + async def test_definitive_404_returns_not_found_not_server_unreachable(self, tmp_path): + """A 404 is the server ANSWERING — it must not read as unreachable (#1570). + + RomM 404s the saves query when it no longer has the ROM (or the + registered device id). Reporting that as ``server_unreachable`` + made the panel blame a connection that is plainly working and + offer a retry that can never succeed. + """ + svc, fake = make_service(tmp_path) + + fake.fail_on_next(RommNotFoundError("HTTP 404: Not Found")) + + result = await svc.list_file_versions(42, "default", "pokemon.srm") + + assert result["status"] == "not_found" + assert result["status"] != "server_unreachable" + assert "message" in result + # Must NOT be the same shape as the happy-path empty case: the panel + # distinguishes "the server has nothing" from "we know nothing". + assert result != {"status": "ok", "versions": []} + @pytest.mark.asyncio async def test_result_shape(self, tmp_path): """Each entry contains the required fields: id, updated_at, file_size_bytes, device_syncs.""" @@ -650,6 +673,44 @@ def fail_second_list(*args, **kwargs): # Critical: must NOT collide with the "genuinely deleted" case. assert result["status"] != "version_deleted" + @pytest.mark.asyncio + async def test_post_preflight_definitive_404_returns_not_found(self, tmp_path): + """The 404 twin of the test above — three outcomes stay distinct (#1570). + + ``version_deleted`` = one save gone from a ROM the server still has; + ``server_unreachable`` = we could not ask; ``not_found`` = the server + answered that it has no such ROM / device id at all. + """ + svc, fake = make_service(tmp_path) + + _create_save(tmp_path) + local_hash = _file_md5(str(tmp_path / "saves" / "gba" / "pokemon.srm")) + self._setup_state(svc, tmp_path, tracked_id=100, last_sync_hash=local_hash) + fake.saves[100] = self._tracked_save(100) + fake.saves[50] = _server_save(save_id=50, rom_id=42, slot="default", updated_at="2026-02-01T10:00:00Z") + + # Let the pre-flight ``list_saves`` succeed, then 404 on the next one. + original_list = fake.list_saves + call_count = {"n": 0} + + def fail_second_list(*args, **kwargs): + call_count["n"] += 1 + if call_count["n"] >= 2: + raise RommNotFoundError("HTTP 404: Not Found") + return original_list(*args, **kwargs) + + fake.list_saves = fail_second_list # type: ignore[method-assign] + + try: + result = await svc.rollback_to_version(42, "default", 50) + finally: + fake.list_saves = original_list # type: ignore[method-assign] + + assert result["status"] == "not_found" + assert result["status"] != "server_unreachable" + assert result["status"] != "version_deleted" + assert "message" in result + @pytest.mark.asyncio async def test_multi_file_slot_refuses_rollback(self, tmp_path): """A multi-file slot refuses per-version rollback (#908 interim guard). diff --git a/tests/services/test_achievements.py b/tests/services/test_achievements.py index 390ae4de..f87ba1f7 100644 --- a/tests/services/test_achievements.py +++ b/tests/services/test_achievements.py @@ -16,6 +16,7 @@ from adapters.steam_config import SteamConfigAdapter from domain.rom import Rom +from lib.errors import RommConnectionError, RommNotFoundError from services.achievements import AchievementsService, AchievementsServiceConfig from services.game_detail import GameDetailService, GameDetailServiceConfig from services.library import LibraryService, LibraryServiceConfig @@ -493,6 +494,33 @@ async def test_api_error_no_cache_returns_error(self, svc, plugin): assert result["total"] == 0 assert "Connection refused" in result["message"] + @pytest.mark.asyncio + async def test_transport_error_reason_is_server_unreachable(self, svc, plugin): + """A genuine transport failure keeps the offline slug the tab routes on.""" + _seed_rom(plugin._uow, 42, ra_id=9999) + + plugin._romm_api.get_rom.side_effect = RommConnectionError("Connection refused") + result = await svc.get_achievements(42) + + assert result["success"] is False + assert result["reason"] == "server_unreachable" + + @pytest.mark.asyncio + async def test_definitive_404_reason_is_not_found(self, svc, plugin): + """A 404 must not drive the achievements tab's offline line (#1570). + + RomMGameInfoPanel feeds the global connection store on + reason == "server_unreachable" from this very call. + """ + _seed_rom(plugin._uow, 42, ra_id=9999) + + plugin._romm_api.get_rom.side_effect = RommNotFoundError("HTTP 404: Not Found") + result = await svc.get_achievements(42) + + assert result["success"] is False + assert result["reason"] == "not_found" + assert result["reason"] != "server_unreachable" + @pytest.mark.asyncio async def test_rom_id_cast_to_int(self, svc, plugin): """rom_id is cast to int, so string input works too.""" @@ -674,6 +702,23 @@ async def test_api_error_no_cache_returns_error(self, svc, plugin): assert result["total"] == 0 assert "Network error" in result["message"] + @pytest.mark.asyncio + async def test_definitive_404_reason_is_not_found(self, svc, plugin): + """The progress call's 404 twin — same store-feed hazard (#1570).""" + _seed_ra_username_cache(svc) + _seed_rom(plugin._uow, 42, ra_id=9999) + svc._achievements_cache["42"] = { + "achievements": _sample_achievements(), + "cached_at": svc._clock.time(), + } + + plugin._romm_api.get_current_user.side_effect = RommNotFoundError("HTTP 404: Not Found") + result = await svc.get_achievement_progress(42) + + assert result["success"] is False + assert result["reason"] == "not_found" + assert result["reason"] != "server_unreachable" + @pytest.mark.asyncio async def test_empty_ra_progression(self, svc, plugin): """User data with empty ra_progression returns zeros.""" diff --git a/tests/services/test_artwork.py b/tests/services/test_artwork.py index 85065ddc..94e35490 100644 --- a/tests/services/test_artwork.py +++ b/tests/services/test_artwork.py @@ -1703,7 +1703,7 @@ async def test_server_unreachable_when_get_rom_raises( ): _seed_rom(uow, 42, app_id=999) steam_config.grid_dir.return_value = str(tmp_path) - romm_api.get_rom.side_effect = Exception("network down") + romm_api.get_rom.side_effect = RommConnectionError("network down") result = await artwork_service.refresh_cover(42) assert result["success"] is False @@ -1711,6 +1711,31 @@ async def test_server_unreachable_when_get_rom_raises( assert result["message"] == "Could not fetch ROM from server" romm_api.download_cover.assert_not_called() + @pytest.mark.asyncio + async def test_not_found_when_get_rom_404s( + self, + artwork_service, + uow, + steam_config, + romm_api, + tmp_path, + ): + """A definitive 404 is the server ANSWERING — not an outage (#1570). + + The message stays the same because it is already true either way; + only the routing slug was wrong. + """ + _seed_rom(uow, 42, app_id=999) + steam_config.grid_dir.return_value = str(tmp_path) + romm_api.get_rom.side_effect = RommNotFoundError("HTTP 404: Not Found") + + result = await artwork_service.refresh_cover(42) + assert result["success"] is False + assert result["reason"] == "not_found" + assert result["reason"] != "server_unreachable" + assert result["message"] == "Could not fetch ROM from server" + romm_api.download_cover.assert_not_called() + @pytest.mark.asyncio async def test_server_unreachable_when_get_rom_returns_none( self, diff --git a/tests/services/test_version_switch.py b/tests/services/test_version_switch.py index 04ac06c3..f2a9568c 100644 --- a/tests/services/test_version_switch.py +++ b/tests/services/test_version_switch.py @@ -13,6 +13,7 @@ from domain.rom import Rom from domain.rom_install import RomInstall +from lib.errors import RommNotFoundError from services.version_switch import VersionSwitchService, VersionSwitchServiceConfig _GROUP = "igdb:100:57" @@ -352,6 +353,22 @@ def test_server_unreachable_degrades_to_local_only(self, event_loop, service, uo assert result["server_query_failed"] is True assert {v["rom_id"] for v in result["versions"]} == {1, 2} + def test_bound_rom_404_degrades_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 + ROM the server merely no longer has would black out the whole UI. + The list still degrades to local-only — only the flag differs. + """ + _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("HTTP 404: Not Found") + + 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} + 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 # preferred_region re-heads it (Japan over the USA that would win by default). @@ -733,6 +750,19 @@ 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).""" + _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"] 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() + 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 # group, so membership passes) but carries an id the Rom aggregate rejects From 9eba6f75ff676d23c541044b20426d0f634ae9e1 Mon Sep 17 00:00:00 2001 From: danielcopper Date: Thu, 23 Jul 2026 18:09:06 +0200 Subject: [PATCH 2/7] ci(errors): gate the 404-vs-unreachable invariant Add scripts/check_404_not_unreachable.py to the lint task and the architecture-gate CI job, plus its row in the invariant register. check_failure_shape.py inspects key presence only, so a hardcoded server_unreachable is a perfectly canonical failure shape and every one of these sites was green there. This check inspects the slug choice instead. The class was swept once before and came back, which is why it gets a mechanical check rather than a review note. Document the rule where users meet it: the save-sync offline-behaviour paragraph and the troubleshooting offline-detection section now say that a "RomM no longer has this" answer leaves the offline badge clear, and that the fix in that case is server-side rather than with the network. --- .github/workflows/ci.yml | 3 + CLAUDE.md | 4 + docs/user-guide/save-sync.md | 12 +- docs/user-guide/troubleshooting.md | 6 + mise.toml | 1 + scripts/check_404_not_unreachable.py | 293 ++++++++++++++++ .../scripts/test_check_404_not_unreachable.py | 329 ++++++++++++++++++ 7 files changed, 644 insertions(+), 4 deletions(-) create mode 100644 scripts/check_404_not_unreachable.py create mode 100644 tests/scripts/test_check_404_not_unreachable.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 76b91f2c..a477272a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -146,6 +146,9 @@ jobs: - name: Check failure-shape dialect gate run: python scripts/check_failure_shape.py --check + - name: Check 404-vs-unreachable classification gate + run: python scripts/check_404_not_unreachable.py --check + - name: Check no bare ignores run: bash scripts/check_no_bare_ignores.sh diff --git a/CLAUDE.md b/CLAUDE.md index 39643b63..fb74c79e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -128,6 +128,10 @@ Format: **invariant** — tier — enforced by. - **Callable failures use `{success, reason, message}` (never `error` / `error_code`)** — check — `scripts/check_failure_shape.py --check` +- **A definitive 404 is `not_found`, never `server_unreachable` — a catch-all `except Exception` in `services/` may not + bind a verdict key (`reason` / `status` / `recommended_action`) to a hardcoded `SERVER_UNREACHABLE`; route the + exception through `classify_error`, or peel the 404 off with a sibling `except RommNotFoundError` where the verdict is + a partial-success flag** — check — `scripts/check_404_not_unreachable.py --check` - **Frontend↔backend callable parity (names + arity)** — check — `scripts/check_callable_manifest.py` - **Every backend `emit` event name has a frontend listener, and vice versa** — check — `scripts/check_event_parity.py` - **`settings.json` is written only by its owner (`adapters/persistence.py`)** — check — diff --git a/docs/user-guide/save-sync.md b/docs/user-guide/save-sync.md index 0ebdc794..cf44cf54 100644 --- a/docs/user-guide/save-sync.md +++ b/docs/user-guide/save-sync.md @@ -182,10 +182,14 @@ If the RomM server is unreachable when a sync is attempted: "Server offline" is reported **only** for a genuine reachability failure (the server can't be reached or times out). A sync that fails for another reason — for example an expired or revoked login token, or an SSL certificate problem — shows that specific reason instead (such as "Authentication failed — check your username and password"), so a working -server is never mislabelled as offline. This applies to both surfaces: the warning shown before launch and the "after -exit" toast both name the actual cause rather than a generic "failed to sync" message. When more than one save file -fails in the same sync, the toast shows the first file's reason followed by a "(+N more)" count so the message stays -short. If you see an authentication message, re-enter your server URL and sign in again in the plugin settings. +server is never mislabelled as offline. That includes the case where RomM answers that it no longer _has_ something the +plugin asked about: if the game was deleted and re-added on the server (which gives it a new id), or the RomM database +was wiped and this device's registration went with it, the server is answering perfectly well. You'll see a message +naming what's missing rather than "RomM is offline", and the offline badge stays clear. This applies to both surfaces: +the warning shown before launch and the "after exit" toast both name the actual cause rather than a generic "failed to +sync" message. When more than one save file fails in the same sync, the toast shows the first file's reason followed by +a "(+N more)" count so the message stays short. If you see an authentication message, re-enter your server URL and sign +in again in the plugin settings. After a failed sync the game-detail save panel reflects the honest state right away: a file whose upload failed shows a yellow **Local changes** badge (not a green "synced"), and its "Last synced" line keeps the time of the last diff --git a/docs/user-guide/troubleshooting.md b/docs/user-guide/troubleshooting.md index 2e9c47d2..eecb7b75 100644 --- a/docs/user-guide/troubleshooting.md +++ b/docs/user-guide/troubleshooting.md @@ -79,6 +79,12 @@ directions. If RomM goes away, the **RomM offline** badge appears on its own wit reachable again the badge clears, Download and slot switching re-enable, and the saves list, achievements, and setup screen reload themselves on the spot — no need to leave and re-open the page. +Only a genuine connection failure marks the plugin offline. If RomM replies that it no longer has the thing being asked +about — most often because the game was deleted and re-added on the server, which gives it a brand-new id, or because +the RomM database was reset and this device's registration disappeared with it — that is the server _answering_, so the +**RomM offline** badge stays clear and the surface tells you what's missing instead. If you see such a message while the +badge is clear, the fix is on the RomM side (re-sync the library, or re-check the game), not with your network. + ### Save file not found **Symptom**: The game detail page shows save status but no save file is being synced. diff --git a/mise.toml b/mise.toml index ba321615..fe6bbfc3 100755 --- a/mise.toml +++ b/mise.toml @@ -55,6 +55,7 @@ run = [ "python scripts/check_aggregate_field_assignment.py", "python scripts/check_uow_seam_nesting.py", "python scripts/check_failure_shape.py --check", + "python scripts/check_404_not_unreachable.py --check", "bash scripts/check_no_bare_ignores.sh", "python scripts/check_event_parity.py", "python scripts/check_settings_owner.py", diff --git a/scripts/check_404_not_unreachable.py b/scripts/check_404_not_unreachable.py new file mode 100644 index 00000000..70dc449f --- /dev/null +++ b/scripts/check_404_not_unreachable.py @@ -0,0 +1,293 @@ +#!/usr/bin/env python3 +"""404-vs-unreachable gate — exception-type-blind classification enforcement. + +RomM answers a request for an entity it no longer has with HTTP 404. That is +the server *answering*, not the server being unreachable, so it must reach the +frontend as ``not_found`` — never as ``server_unreachable``. The plugin already +owns the funnel that decides this (:func:`lib.errors.classify_error`, which maps +``RommNotFoundError`` to :data:`ErrorCode.NOT_FOUND`); the recurring defect is a +catch-all ``except Exception`` that ignores the funnel and hardcodes the +unreachable verdict for every exception it sees. The whole UI then claims "RomM +offline" while the server is plainly answering. + +This class of bug was swept once before (#971) and came back, because +``check_failure_shape.py`` inspects key *presence* only — a hardcoded +``server_unreachable`` is a perfectly canonical failure shape, so every one of +these sites is green there. This check inspects the slug *choice* instead. + +The rule +======== + +Inside ``py_modules/services/``, a **catch-all** exception handler (``except +Exception``, ``except BaseException``, or a bare ``except:``) may not *return* a +hardcoded ``server_unreachable`` verdict. A handler returns one when a dict +literal in its body binds a verdict key — ``reason``, ``status``, or +``recommended_action`` — to ``ErrorCode.SERVER_UNREACHABLE`` or to the bare +``"server_unreachable"`` string. Both spellings count: the canonical failure +shape uses the enum, while the discriminated-status and ``recommended_action`` +carve-outs spell the slug out as a literal. + +Binding the key to a *name* (``{"reason": reason}`` after ``reason, message = +classify_error(e)``) is the correct shape and is what this check is steering +towards, so it is never flagged. Keying on the verdict's position — rather than +on the mere mention of the slug anywhere in the handler — is deliberate: it +catches the sharpest form of this bug, a handler that calls ``classify_error`` +and then **discards** its verdict in favour of the hardcoded one, which a +"does the handler call the funnel?" test would wave through. + +One escape clears an otherwise-flagged handler: + +* **A sibling handler peels the 404 off first** — the same ``try`` carries an + ``except RommNotFoundError`` clause, so the definitive-404 case never reaches + the catch-all. This is the shape the partial-success carve-outs use, where the + verdict is a flag rather than a ``reason`` slug and there is no funnel to call. + +``EXEMPT`` holds service modules deliberately kept out of the scan — a module +that does not talk to RomM at all, so ``classify_error`` (a RomM-shaped funnel) +has nothing to say about its failures. Adding to it is a deliberate act that +shows up in review. + +Two modes: + + * report (default) — print every catch-all handler carrying an unreachable + verdict, grouped by verdict, then exit 0. Report mode never fails; it is the + inventory. + * ``--check`` — enforce mode. Exit 1 on any violation. + +A **typed** handler is never flagged. ``except RommConnectionError`` or +``except SaveSyncTimeoutError`` returning ``server_unreachable`` is a deliberate +statement about a known exception type, which is exactly what this check wants +more of. + +The AST heuristic is intentionally conservative, and a guardrail rather than a +prover: it reads one ``try`` statement at a time and does not follow a verdict +returned by a helper the handler calls, nor one assembled across statements +(``resp = {...}; resp["reason"] = ...``). A ``try`` nested *inside* a catch-all +handler contributes its own dict literals to that handler's scan, so a nested +``except RommNotFoundError`` peel can clear the outer handler too. +""" + +from __future__ import annotations + +import ast +import sys +from dataclasses import dataclass +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parent.parent +SERVICES_DIR = REPO_ROOT / "py_modules" / "services" + +# The canonical slug, in both spellings a service can write it. +UNREACHABLE_ENUM_MEMBER = "SERVER_UNREACHABLE" +UNREACHABLE_SLUG = "server_unreachable" + +# Dict keys whose value is the verdict a consumer routes on: the canonical +# failure shape's ``reason``, plus the two documented carve-out discriminants. +VERDICT_KEYS = frozenset({"reason", "status", "recommended_action"}) + +# The exception whose presence as a sibling clause proves the 404 was peeled off. +NOT_FOUND_EXCEPTION = "RommNotFoundError" + +# Catch-all handler types (``except:`` with no type is handled separately). +CATCH_ALL_NAMES = frozenset({"Exception", "BaseException"}) + +# Service modules deliberately out of scope, as ``: ``. Only for +# modules that never talk to RomM — classify_error cannot classify their errors. +EXEMPT: dict[str, str] = { + "steamgrid.py": ( + "talks to SteamGridDB, not RomM — SGDB failures arrive as SgdbApiError " + "(carrying its own status_code) and classify_error has no SGDB branch" + ), +} + + +@dataclass(frozen=True) +class Finding: + """One catch-all handler that hardcodes the unreachable verdict.""" + + path: Path + lineno: int + spelling: str + function: str + + @property + def rel(self) -> str: + return str(self.path.relative_to(REPO_ROOT)) + + def render(self) -> str: + return f"{self.rel}:{self.lineno} in {self.function}() [{self.spelling}]" + + +def _handler_is_catch_all(handler: ast.ExceptHandler) -> bool: + """True when *handler* catches everything (bare, Exception, BaseException). + + A tuple clause counts when any member is a catch-all name: ``except + (Exception, X)`` still swallows every exception. + """ + if handler.type is None: # bare ``except:`` + return True + candidates = handler.type.elts if isinstance(handler.type, ast.Tuple) else [handler.type] + return any(isinstance(node, ast.Name) and node.id in CATCH_ALL_NAMES for node in candidates) + + +def _handler_catches_not_found(handler: ast.ExceptHandler) -> bool: + """True when *handler* explicitly catches ``RommNotFoundError``.""" + if handler.type is None: + return False + candidates = handler.type.elts if isinstance(handler.type, ast.Tuple) else [handler.type] + return any(isinstance(node, ast.Name) and node.id == NOT_FOUND_EXCEPTION for node in candidates) + + +def _hardcoded_verdict_spelling(value: ast.expr) -> str | None: + """Return how *value* spells a hardcoded unreachable verdict, else None. + + ``ErrorCode.SERVER_UNREACHABLE`` and ``ErrorCode.SERVER_UNREACHABLE.value`` + both surface as an ``Attribute`` named ``SERVER_UNREACHABLE``; the carve-out + shapes write the bare ``"server_unreachable"`` string instead. A ``Name`` + (the classified slug held in a variable) is neither, and returns None. + """ + for node in ast.walk(value): + if isinstance(node, ast.Attribute) and node.attr == UNREACHABLE_ENUM_MEMBER: + return f"ErrorCode.{UNREACHABLE_ENUM_MEMBER}" + if isinstance(node, ast.Constant) and node.value == UNREACHABLE_SLUG: + return f'"{UNREACHABLE_SLUG}"' + return None + + +def _unreachable_spelling(handler: ast.ExceptHandler) -> str | None: + """Return how *handler* hardcodes the unreachable verdict, or None. + + Only a verdict *key* counts (see :data:`VERDICT_KEYS`) — mentioning the slug + anywhere else in the handler (a log line, a comparison against a classified + reason) is not a hardcoded verdict. + """ + for node in ast.walk(handler): + if not isinstance(node, ast.Dict): + continue + for key, value in zip(node.keys, node.values, strict=True): + if not (isinstance(key, ast.Constant) and key.value in VERDICT_KEYS): + continue + spelling = _hardcoded_verdict_spelling(value) + if spelling is not None: + return spelling + return None + + +def _enclosing_functions(tree: ast.AST) -> dict[int, str]: + """Map every node's line to the name of the function that contains it.""" + owner: dict[int, str] = {} + for node in ast.walk(tree): + if not isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): + continue + for child in ast.walk(node): + lineno = getattr(child, "lineno", None) + if lineno is not None: + # Inner functions win: they are visited after the outer one only + # when nested, so record the most specific (shortest) span. + owner[lineno] = node.name + return owner + + +def scan_source(path: Path, source: str) -> list[Finding]: + """Return every catch-all handler in *source* that hardcodes the verdict.""" + try: + tree = ast.parse(source, filename=str(path)) + except SyntaxError: + return [] + + function_of = _enclosing_functions(tree) + findings: list[Finding] = [] + + for node in ast.walk(tree): + if not isinstance(node, ast.Try): + continue + peels_not_found = any(_handler_catches_not_found(h) for h in node.handlers) + if peels_not_found: + continue + for handler in node.handlers: + if not _handler_is_catch_all(handler): + continue + spelling = _unreachable_spelling(handler) + if spelling is None: + continue + findings.append( + Finding( + path=path, + lineno=handler.lineno, + spelling=spelling, + function=function_of.get(handler.lineno, ""), + ) + ) + return findings + + +def scan_file(path: Path) -> list[Finding]: + try: + source = path.read_text(encoding="utf-8") + except OSError: + return [] + return scan_source(path, source) + + +def collect_findings(services_dir: Path = SERVICES_DIR) -> list[Finding]: + """Walk *services_dir* and return every hardcoded-verdict finding.""" + findings: list[Finding] = [] + if not services_dir.is_dir(): + return findings + for path in sorted(services_dir.rglob("*.py")): + if str(path.relative_to(services_dir)) in EXEMPT: + continue + findings.extend(scan_file(path)) + return findings + + +def _print_report(findings: list[Finding]) -> None: + print(f"=== CATCH-ALL HANDLERS HARDCODING server_unreachable ({len(findings)}) ===") + for finding in findings: + print(f" {finding.render()}") + print() + if EXEMPT: + print("=== EXEMPT MODULES ===") + for name, why in sorted(EXEMPT.items()): + print(f" {name} — {why}") + print() + print("=== SUMMARY ===") + print(f" TOTAL: {len(findings)}") + + +def _print_violations(findings: list[Finding]) -> None: + print(f"=== CATCH-ALL HANDLERS HARDCODING server_unreachable ({len(findings)}) ===") + for finding in findings: + print(f" {finding.render()}") + print() + print( + "ERROR: a catch-all 'except Exception' in py_modules/services/ must not hardcode " + "ErrorCode.SERVER_UNREACHABLE — a definitive 404 is the server ANSWERING, and must " + "reach the frontend as 'not_found'. Route the exception through classify_error() " + "(lib/errors.py), or peel the 404 off with a sibling 'except RommNotFoundError' " + "clause when the verdict is a partial-success flag rather than a reason slug " + "(CLAUDE.md → invariant register)." + ) + + +def main(argv: list[str]) -> int: + if any(a in {"-h", "--help"} for a in argv): + print(__doc__) + return 0 + + enforce = "--check" in argv + findings = collect_findings(SERVICES_DIR) + + if enforce: + if findings: + _print_violations(findings) + return 1 + print(f"OK: no catch-all handler hardcodes server_unreachable in {SERVICES_DIR.relative_to(REPO_ROOT)}.") + return 0 + + _print_report(findings) + return 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv[1:])) diff --git a/tests/scripts/test_check_404_not_unreachable.py b/tests/scripts/test_check_404_not_unreachable.py new file mode 100644 index 00000000..296383cf --- /dev/null +++ b/tests/scripts/test_check_404_not_unreachable.py @@ -0,0 +1,329 @@ +"""Tests for ``scripts/check_404_not_unreachable.py``. + +The check is loaded via ``importlib`` because ``scripts/`` is not on +``sys.path`` (and is excluded from ruff/basedpyright). Fixtures use +``tmp_path`` to lay out a small ``py_modules/services/`` tree the check +walks, passing that directory to ``collect_findings`` directly. + +Coverage centres on the positional rule — a catch-all handler is flagged +only when a *verdict key* (``reason`` / ``status`` / ``recommended_action``) +is bound to a hardcoded ``server_unreachable`` — plus the two ways a +handler stays clean (binding the key to the classified slug, or peeling +the 404 off with a sibling ``except RommNotFoundError``). +""" + +from __future__ import annotations + +import importlib.util +import sys +import textwrap +from pathlib import Path +from typing import TYPE_CHECKING, Any + +if TYPE_CHECKING: + from types import ModuleType + +_SCRIPT_PATH = Path(__file__).resolve().parents[2] / "scripts" / "check_404_not_unreachable.py" + + +def _load_check_module() -> ModuleType: + spec = importlib.util.spec_from_file_location("check_404_not_unreachable", _SCRIPT_PATH) + assert spec is not None and spec.loader is not None + module = importlib.util.module_from_spec(spec) + sys.modules[spec.name] = module + spec.loader.exec_module(module) + return module + + +check = _load_check_module() + + +def _make_services_tree(tmp_path: Path, files: dict[str, str]) -> Path: + """Build a fake ``py_modules/services/`` tree: ``name -> source``.""" + services_dir = tmp_path / "py_modules" / "services" + services_dir.mkdir(parents=True) + for name, source in files.items(): + path = services_dir / name + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(textwrap.dedent(source), encoding="utf-8") + return services_dir + + +def _scan(source: str) -> Any: + """Scan a single module source, returning its findings.""" + return check.scan_source(Path("mod.py"), textwrap.dedent(source)) + + +# ── Flagged: a verdict key bound to a hardcoded slug ───────────────────── + + +class TestHardcodedVerdictIsFlagged: + def test_reason_key_with_error_code_enum(self): + findings = _scan(""" + def f(): + try: + fetch() + except Exception as e: + return {"success": False, "reason": ErrorCode.SERVER_UNREACHABLE.value, "message": str(e)} + """) + assert len(findings) == 1 + assert findings[0].spelling == "ErrorCode.SERVER_UNREACHABLE" + assert findings[0].function == "f" + + def test_reason_key_without_dot_value(self): + findings = _scan(""" + def f(): + try: + fetch() + except Exception: + return {"success": False, "reason": ErrorCode.SERVER_UNREACHABLE, "message": "m"} + """) + assert len(findings) == 1 + + def test_status_discriminant_with_bare_slug(self): + """The discriminated-status carve-out spells the slug as a literal.""" + findings = _scan(""" + def f(): + try: + fetch() + except Exception as e: + return {"status": "server_unreachable", "message": str(e)} + """) + assert len(findings) == 1 + assert findings[0].spelling == '"server_unreachable"' + + def test_recommended_action_with_bare_slug(self): + """The partial-success carve-out routes on recommended_action.""" + findings = _scan(""" + def f(): + try: + fetch() + except Exception: + return {"recommended_action": "server_unreachable", "server_query_failed": True} + """) + assert len(findings) == 1 + + def test_classify_error_called_but_verdict_discarded_is_flagged(self): + """The sharpest form: consults the funnel, then hardcodes anyway. + + A "does the handler call classify_error?" test would wave this + through — the real defect at ``sync_engine/rollback.py`` before + #1570. + """ + findings = _scan(""" + def f(): + try: + fetch() + except Exception as e: + _code, _msg = classify_error(e) + return {"success": False, "reason": ErrorCode.SERVER_UNREACHABLE.value, "message": _msg} + """) + assert len(findings) == 1 + + def test_bare_except_is_catch_all(self): + findings = _scan(""" + def f(): + try: + fetch() + except: + return {"success": False, "reason": ErrorCode.SERVER_UNREACHABLE.value, "message": "m"} + """) + assert len(findings) == 1 + + def test_base_exception_is_catch_all(self): + findings = _scan(""" + def f(): + try: + fetch() + except BaseException: + return {"success": False, "reason": "server_unreachable", "message": "m"} + """) + assert len(findings) == 1 + + def test_tuple_clause_containing_exception_is_catch_all(self): + """``except (ValueError, Exception)`` still swallows everything.""" + findings = _scan(""" + def f(): + try: + fetch() + except (ValueError, Exception): + return {"success": False, "reason": "server_unreachable", "message": "m"} + """) + assert len(findings) == 1 + + def test_nested_dict_value_is_flagged(self): + """A verdict key bound to a nested literal still hardcodes the slug.""" + findings = _scan(""" + def f(): + try: + fetch() + except Exception: + return {"success": False, "reason": (ErrorCode.SERVER_UNREACHABLE.value), "message": "m"} + """) + assert len(findings) == 1 + + +# ── Clean: the shapes the check is steering towards ────────────────────── + + +class TestCorrectShapesAreClean: + def test_verdict_key_bound_to_classified_name(self): + findings = _scan(""" + def f(): + try: + fetch() + except Exception as e: + reason, message = classify_error(e) + return {"success": False, "reason": reason, "message": message} + """) + assert findings == [] + + def test_error_response_helper(self): + findings = _scan(""" + def f(): + try: + fetch() + except Exception as e: + return error_response(e) + """) + assert findings == [] + + def test_sibling_not_found_handler_peels_the_404(self): + """The partial-success shape: no slug to classify, so peel instead.""" + findings = _scan(""" + def f(): + try: + fetch() + except RommNotFoundError: + return {"recommended_action": "not_found", "server_query_failed": True} + except Exception: + return {"recommended_action": "server_unreachable", "server_query_failed": True} + """) + assert findings == [] + + def test_typed_handler_is_never_flagged(self): + """A deliberate statement about a known type is the goal, not the bug.""" + findings = _scan(""" + def f(): + try: + fetch() + except RommConnectionError: + return {"success": False, "reason": ErrorCode.SERVER_UNREACHABLE.value, "message": "offline"} + """) + assert findings == [] + + def test_slug_in_a_log_line_only(self): + findings = _scan(""" + def f(): + try: + fetch() + except Exception as e: + log("server_unreachable path taken") + return error_response(e) + """) + assert findings == [] + + def test_comparing_against_the_classified_slug(self): + """Branching on a classified reason is not a hardcoded verdict.""" + findings = _scan(""" + def f(): + try: + fetch() + except Exception as e: + reason, message = classify_error(e) + offline = reason == ErrorCode.SERVER_UNREACHABLE.value + return {"success": False, "reason": reason, "message": message, "offline": offline} + """) + assert findings == [] + + def test_non_verdict_key_is_not_a_verdict(self): + """The slug under a payload key routes nothing.""" + findings = _scan(""" + def f(): + try: + fetch() + except Exception as e: + reason, message = classify_error(e) + return {"success": False, "reason": reason, "message": message, "probe": "server_unreachable"} + """) + assert findings == [] + + def test_handler_with_no_return_dict(self): + findings = _scan(""" + def f(): + try: + fetch() + except Exception: + raise + """) + assert findings == [] + + +# ── Tree walking, EXEMPT, and the CLI ──────────────────────────────────── + + +class TestCollectFindings: + def test_walks_nested_packages(self, tmp_path: Path): + services_dir = _make_services_tree( + tmp_path, + { + "clean.py": ( + "def f():\n" + " try:\n" + " fetch()\n" + " except Exception as e:\n" + " return error_response(e)\n" + ), + "saves/slots/dirty.py": ( + "def g():\n" + " try:\n" + " fetch()\n" + " except Exception:\n" + ' return {"success": False, "reason": ErrorCode.SERVER_UNREACHABLE.value, "message": "m"}\n' + ), + }, + ) + findings = check.collect_findings(services_dir) + assert len(findings) == 1 + assert findings[0].path.name == "dirty.py" + assert findings[0].function == "g" + + def test_exempt_module_is_skipped(self, tmp_path: Path): + dirty = ( + "def f():\n" + " try:\n" + " fetch()\n" + " except Exception:\n" + ' return {"success": False, "reason": ErrorCode.SERVER_UNREACHABLE.value, "message": "m"}\n' + ) + services_dir = _make_services_tree(tmp_path, {"steamgrid.py": dirty, "other.py": dirty}) + findings = check.collect_findings(services_dir) + assert [f.path.name for f in findings] == ["other.py"] + + def test_exempt_entries_carry_a_reason(self): + """EXEMPT is a dict so every waiver states why, visibly in review.""" + assert check.EXEMPT + assert all(isinstance(why, str) and why for why in check.EXEMPT.values()) + + def test_missing_directory_yields_nothing(self, tmp_path: Path): + assert check.collect_findings(tmp_path / "nope") == [] + + def test_syntax_error_is_skipped(self, tmp_path: Path): + services_dir = _make_services_tree(tmp_path, {"broken.py": "def f( :\n"}) + assert check.collect_findings(services_dir) == [] + + +class TestCli: + def test_help_exits_zero(self, capsys): + assert check.main(["--help"]) == 0 + assert "404-vs-unreachable gate" in capsys.readouterr().out + + def test_report_mode_exits_zero_on_the_real_tree(self, capsys): + """Report mode is the inventory — it never fails.""" + assert check.main([]) == 0 + assert "SUMMARY" in capsys.readouterr().out + + def test_check_mode_passes_on_the_real_tree(self, capsys): + """The production services tree must satisfy the invariant.""" + assert check.main(["--check"]) == 0 + assert "OK:" in capsys.readouterr().out From 0845a97cddb79026c9b2f8acc6477f6d305ea47b Mon Sep 17 00:00:00 2001 From: danielcopper Date: Thu, 23 Jul 2026 18:32:08 +0200 Subject: [PATCH 3/7] fix(saves): tell the setup wizard a 404 is an answer, not an outage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit get_save_setup_info collapsed every server-saves failure onto recommended_action "server_unreachable", so a RomM database reset — which drops this device's registration and makes the saves query 404 — told the user their server was offline while it was answering fine. Add a fourth arm, "not_found", peeled off with a sibling except RommNotFoundError. It holds the wizard exactly as the unreachable branch does: a 404 leaves the server's slot inventory just as unproven as an outage, so auto-confirming the default slot could still clobber real server saves on the first post-confirmation sync. The launch gate likewise still aborts. Only the copy and the reachability verdict change. The copy deliberately does not say the game has no saves. The 404 can come from the device registration rather than the ROM, so that would swap one lie for a more specific one; it states what RomM could not find instead. SlotSetupWizard needed no change: it already tested for an explicit server_unreachable, so a not_found result now correctly reports the server reachable and leaves the reconnect gate disarmed. This clears the last site flagged by check_404_not_unreachable.py. --- docs/user-guide/troubleshooting.md | 7 +++ py_modules/services/saves/slots/setup.py | 74 +++++++++++++++++++----- src/components/SlotSetupWizard.test.tsx | 41 +++++++++++++ src/components/SlotSetupWizard.tsx | 8 +++ src/types/saves.ts | 10 ++-- src/utils/saveSetup.test.ts | 62 ++++++++++++++++++++ src/utils/saveSetup.ts | 49 ++++++++++++++-- tests/services/saves/test_slots.py | 28 +++++++++ 8 files changed, 257 insertions(+), 22 deletions(-) diff --git a/docs/user-guide/troubleshooting.md b/docs/user-guide/troubleshooting.md index eecb7b75..cb96ed14 100644 --- a/docs/user-guide/troubleshooting.md +++ b/docs/user-guide/troubleshooting.md @@ -85,6 +85,13 @@ the RomM database was reset and this device's registration disappeared with it **RomM offline** badge stays clear and the surface tells you what's missing instead. If you see such a message while the badge is clear, the fix is on the RomM side (re-sync the library, or re-check the game), not with your network. +The first-time save-slot setup screen is the clearest example. If RomM can't find the save data that setup needs, it +pauses with "RomM couldn't find the save data for this setup" rather than the "server is not reachable" message, and the +offline badge stays clear. Setup deliberately stops there instead of picking a slot for you: the plugin has no +trustworthy view of what's on the server, and choosing a slot on that basis could overwrite real saves on the first +sync. Re-check the game in RomM (or, after a server database reset, reconnect in the plugin's Connection settings so +this device is registered again), then tap **Retry**. + ### Save file not found **Symptom**: The game detail page shows save status but no save file is being synced. diff --git a/py_modules/services/saves/slots/setup.py b/py_modules/services/saves/slots/setup.py index 463d0618..5054bd03 100644 --- a/py_modules/services/saves/slots/setup.py +++ b/py_modules/services/saves/slots/setup.py @@ -18,7 +18,7 @@ from domain.rom_save_sync_state import RomSaveSyncState from domain.save_layout import SAVE_SYNC_CONTENT_DIR_REASON from domain.save_slot import save_in_slot -from lib.errors import classify_error +from lib.errors import RommNotFoundError, classify_error from services.saves._helpers import newest_server_saves_by_target from services.saves._messages import ( DEVICE_NOT_REGISTERED_REASON, @@ -96,11 +96,45 @@ def is_save_tracking_configured(self, rom_id: int) -> dict[str, Any]: active_slot = game_state.active_slot if (game_state and configured) else None return {"configured": configured, "active_slot": active_slot} + def _setup_query_failed( + self, + *, + local_files: list[dict[str, Any]], + local_file_info: list[dict[str, Any]], + game_state: RomSaveSyncState | None, + default_slot: str, + recommended_action: str, + ) -> dict[str, Any]: + """Build the held-wizard payload for a failed server-saves query. + + Every failure branch holds the wizard the same way — an unproven + server view must never auto-confirm the default slot, whatever the + cause. Only *recommended_action* differs, so the frontend can say + why without claiming the server is offline. + """ + slot_confirmed = bool(game_state.slot_confirmed) if game_state else False + return { + "has_local_saves": len(local_files) > 0, + "local_files": local_file_info, + "server_slots": [], + "default_slot": default_slot, + "slot_confirmed": slot_confirmed, + "active_slot": game_state.active_slot if (game_state and slot_confirmed) else None, + "recommended_action": recommended_action, + "server_query_failed": True, + } + async def get_save_setup_info(self, rom_id: int) -> dict[str, Any]: """Get info needed for the first-sync setup wizard. Fetches server saves, checks local files, determines which scenario (A-E) applies so the frontend can display the right UI. + + ``recommended_action`` carries the failure cause when the server-saves + query raises: ``server_unreachable`` for a transport failure, + ``not_found`` when the server answered that it has no such ROM or + device id. Both hold the wizard identically — only the copy and the + frontend's reachability verdict differ (#1570). """ rom_id = int(rom_id) @@ -126,22 +160,36 @@ async def get_save_setup_info(self, rom_id: int) -> dict[str, Any]: lambda: self._romm_api.list_saves(rom_id, device_id=device_id), ), ) + except RommNotFoundError as e: + # The server ANSWERED — it has no such ROM, or no such device id + # (#1560's shape: a RomM database wipe drops this device's + # registration). Reporting that as "unreachable" sends the user to + # check a connection that is plainly working, so it gets its own + # recommendation. The wizard still HOLDS: a 404 leaves the server's + # slot inventory just as unproven as an outage does, so + # auto-confirming the default slot could still clobber real server + # saves on the first post-confirmation sync. + self._logger.warning( + f"get_save_setup_info({rom_id}): server has no such rom or device: {e}", + ) + return self._setup_query_failed( + local_files=local_files, + local_file_info=local_file_info, + game_state=game_state, + default_slot=default_slot, + recommended_action="not_found", + ) except Exception as e: self._logger.warning( f"get_save_setup_info({rom_id}): failed to list server saves: {e}", ) - slot_confirmed = bool(game_state.slot_confirmed) if game_state else False - active_slot = game_state.active_slot if (game_state and slot_confirmed) else None - return { - "has_local_saves": len(local_files) > 0, - "local_files": local_file_info, - "server_slots": [], - "default_slot": default_slot, - "slot_confirmed": slot_confirmed, - "active_slot": active_slot, - "recommended_action": "server_unreachable", - "server_query_failed": True, - } + return self._setup_query_failed( + local_files=local_files, + local_file_info=local_file_info, + game_state=game_state, + default_slot=default_slot, + recommended_action="server_unreachable", + ) # Group server saves by slot slots_map: dict[str | None, list[dict[str, Any]]] = {} diff --git a/src/components/SlotSetupWizard.test.tsx b/src/components/SlotSetupWizard.test.tsx index 99263a3c..b4a4e6bc 100644 --- a/src/components/SlotSetupWizard.test.tsx +++ b/src/components/SlotSetupWizard.test.tsx @@ -1116,6 +1116,47 @@ describe("SlotSetupWizard", () => { expect(getRommConnectionState()).toBe("offline"); }); + it("does NOT report offline when the load returns recommended_action=not_found", async () => { + // #1560's path: the 404 means the server ANSWERED. Reporting offline + // here blacked out the whole UI while RomM was working fine (#1570). + setRommConnectionState("checking"); + vi.mocked(backend.getSaveSetupInfo).mockResolvedValue( + makeSetupInfo({ recommended_action: "not_found", server_query_failed: true }), + ); + const result = makeSetupInfo({ recommended_action: "not_found", server_query_failed: true }); + vi.mocked(backend.getSaveSetupInfo).mockResolvedValue(result); + render(); + await flushAsync(); + + // Post-load state: the store went CONNECTED (the server answered), and + // the result was still routed into the hold path rather than short- + // circuiting. The banner copy itself is covered in saveSetup.test.ts — + // this file stubs applyWizardInitialSetupResult. + expect(getRommConnectionState()).toBe("connected"); + expect(vi.mocked(applyWizardInitialSetupResult)).toHaveBeenCalledWith(result, expect.anything()); + }); + + it("leaves the reconnect gate disarmed after a not_found load", async () => { + // offlineHeldRef must stay false: there is no connection to wait for, so + // a later →connected edge must not trigger a re-fetch loop (#1570). + setRommConnectionState("checking"); + vi.mocked(backend.getSaveSetupInfo).mockResolvedValue( + makeSetupInfo({ recommended_action: "not_found", server_query_failed: true }), + ); + render(); + await flushAsync(); + const callsAfterLoad = vi.mocked(backend.getSaveSetupInfo).mock.calls.length; + + await act(async () => { + setRommConnectionState("offline"); + reportServerReachable(true); + await Promise.resolve(); + }); + await flushAsync(); + + expect(vi.mocked(backend.getSaveSetupInfo).mock.calls.length).toBe(callsAfterLoad); + }); + it("auto-reloads on reconnect after holding the offline error", async () => { setRommConnectionState("offline"); render(); diff --git a/src/components/SlotSetupWizard.tsx b/src/components/SlotSetupWizard.tsx index ccb7e27f..9e1b388a 100644 --- a/src/components/SlotSetupWizard.tsx +++ b/src/components/SlotSetupWizard.tsx @@ -518,6 +518,10 @@ function useSaveSetupInfo(romId: number, onComplete: () => void) { // definitive offline signal; any other resolved result proves the // server answered. A throw is a bridge/unknown error, not a verdict — // the catch leaves the store untouched. + // Only an explicit unreachable verdict means offline. A `not_found` + // result PROVES the server answered, so it reports reachable and + // leaves the reconnect gate disarmed — the wizard still holds, it + // just doesn't wait for a connection that was never lost (#1570). const reachable = result.recommended_action !== "server_unreachable"; reportServerReachable(reachable); offlineHeldRef.current = !reachable; @@ -576,6 +580,10 @@ function useSaveSetupInfo(romId: number, onComplete: () => void) { // Same conservative feed as the initial load (#1345): the manual // Retry re-probes reachability, so a server_unreachable result // re-arms offline and any other result reports the server back. + // Only an explicit unreachable verdict means offline. A `not_found` + // result PROVES the server answered, so it reports reachable and + // leaves the reconnect gate disarmed — the wizard still holds, it + // just doesn't wait for a connection that was never lost (#1570). const reachable = result.recommended_action !== "server_unreachable"; reportServerReachable(reachable); offlineHeldRef.current = !reachable; diff --git a/src/types/saves.ts b/src/types/saves.ts index ad8e45ec..ea0cd7d0 100644 --- a/src/types/saves.ts +++ b/src/types/saves.ts @@ -174,10 +174,12 @@ export interface SaveSetupInfo { // "server_unreachable" means the server-saves fetch failed — the wizard MUST // hold and offer a retry instead of treating the empty server_slots as // authoritative (auto-confirming default would clobber real server saves on - // first sync). See backend `get_save_setup_info`. - recommended_action: "auto_confirm_default" | "show_wizard" | "server_unreachable"; - // Mirrors recommended_action === "server_unreachable" — explicit flag for - // call sites that route on the boolean rather than the enum. + // first sync). "not_found" is the same hold for a different cause: the + // server ANSWERED that it has no such ROM or device id, so the wizard must + // not report the server offline (#1570). See backend `get_save_setup_info`. + recommended_action: "auto_confirm_default" | "show_wizard" | "server_unreachable" | "not_found"; + // True whenever the server-saves query failed, whichever way — an explicit + // flag for call sites that route on the boolean rather than the enum. server_query_failed?: boolean; } diff --git a/src/utils/saveSetup.test.ts b/src/utils/saveSetup.test.ts index d7fa7512..b2fa0f9c 100644 --- a/src/utils/saveSetup.test.ts +++ b/src/utils/saveSetup.test.ts @@ -12,6 +12,8 @@ import { wizardMigrationOutcomeToastBody, SERVER_UNREACHABLE_WIZARD_MESSAGE, SERVER_UNREACHABLE_TOAST_BODY, + NOT_FOUND_WIZARD_MESSAGE, + NOT_FOUND_TOAST_BODY, type LaunchGateSetupDeps, type SaveSetupOutcome, type WizardRetryDeps, @@ -45,6 +47,25 @@ describe("resolveSaveSetupOutcome", () => { expect(resolveSaveSetupOutcome(info)).toEqual({ kind: "server_unreachable" }); }); + it("routes 'not_found' to its own outcome, NOT to auto-confirm", () => { + // The empty server_slots is no more authoritative on a 404 than on an + // outage, so this must not fall through to the auto-confirm branch (#1570). + const info = makeInfo({ recommended_action: "not_found", server_query_failed: true }); + expect(resolveSaveSetupOutcome(info)).toEqual({ kind: "not_found" }); + }); + + it("routes 'not_found' even with local saves and an empty server list present", () => { + // The exact shape that would otherwise auto-confirm under 'show_wizard'. + const info = makeInfo({ + recommended_action: "not_found", + has_local_saves: true, + server_slots: [], + default_slot: "default", + }); + expect(resolveSaveSetupOutcome(info)).not.toEqual({ kind: "auto_confirm", slot: "default" }); + expect(resolveSaveSetupOutcome(info)).toEqual({ kind: "not_found" }); + }); + it("routes 'auto_confirm_default' to the auto-confirm outcome with the default slot", () => { const info = makeInfo({ recommended_action: "auto_confirm_default", default_slot: "main" }); expect(resolveSaveSetupOutcome(info)).toEqual({ kind: "auto_confirm", slot: "main" }); @@ -95,6 +116,17 @@ describe("applyLaunchGateSetupOutcome", () => { expect(deps.confirmSlotChoice).not.toHaveBeenCalled(); }); + it("aborts the launch on not_found too, with its own copy", async () => { + // The launch must NOT proceed with save tracking unconfigured just + // because the failure was a 404 rather than an outage (#1570). + const deps = makeLaunchGateDeps(); + const result = await applyLaunchGateSetupOutcome({ kind: "not_found" }, deps); + expect(result).toBe("abort"); + expect(deps.toast).toHaveBeenCalledWith(NOT_FOUND_TOAST_BODY); + expect(deps.dispatchSavesTab).toHaveBeenCalledOnce(); + expect(deps.confirmSlotChoice).not.toHaveBeenCalled(); + }); + it("calls confirmSlotChoice with the resolved slot and proceeds on auto_confirm", async () => { const deps = makeLaunchGateDeps(); const outcome: SaveSetupOutcome = { kind: "auto_confirm", slot: "main" }; @@ -190,6 +222,27 @@ describe("applyWizardInitialSetupResult", () => { expect(deps.onComplete).not.toHaveBeenCalled(); }); + it("sets the not-found banner and bails, never auto-confirming", async () => { + // Same hold as server_unreachable — the wizard must not configure a slot + // against an unproven server view just because the cause was a 404 (#1570). + const deps = makeWizardDeps(); + const result = makeInfo({ recommended_action: "not_found", server_query_failed: true }); + await applyWizardInitialSetupResult(result, deps); + expect(deps.setError).toHaveBeenCalledWith(NOT_FOUND_WIZARD_MESSAGE); + expect(deps.setConfirming).not.toHaveBeenCalled(); + expect(deps.confirmSlotChoice).not.toHaveBeenCalled(); + expect(deps.setInfo).not.toHaveBeenCalled(); + expect(deps.onComplete).not.toHaveBeenCalled(); + }); + + it("the not-found copy names what RomM could not find, not that saves are absent", () => { + // The 404 can come from the DEVICE registration, so claiming the game has + // no saves would swap one lie for a more specific one (#1570). + expect(NOT_FOUND_WIZARD_MESSAGE).not.toMatch(/no saves|has no save|without saves/i); + expect(NOT_FOUND_WIZARD_MESSAGE).not.toMatch(/unreachable|not reachable|offline/i); + expect(NOT_FOUND_TOAST_BODY).not.toMatch(/unreachable|not reachable|offline/i); + }); + it("auto-confirms and calls onComplete on 'auto_confirm_default'", async () => { const deps = makeWizardDeps(); const result = makeInfo({ recommended_action: "auto_confirm_default", default_slot: "alpha" }); @@ -292,6 +345,15 @@ describe("applyWizardRetrySetupResult", () => { expect(deps.setInfo).not.toHaveBeenCalled(); }); + it("sets the not-found banner and clears loading on 'not_found'", () => { + const deps = makeRetryDeps(); + const result = makeInfo({ recommended_action: "not_found" }); + applyWizardRetrySetupResult(result, deps); + expect(deps.setError).toHaveBeenCalledWith(NOT_FOUND_WIZARD_MESSAGE); + expect(deps.setLoading).toHaveBeenCalledWith(false); + expect(deps.setInfo).not.toHaveBeenCalled(); + }); + it("sets the fetched info and clears loading on a non-unreachable result", () => { const deps = makeRetryDeps(); const result = makeInfo({ recommended_action: "show_wizard" }); diff --git a/src/utils/saveSetup.ts b/src/utils/saveSetup.ts index 29909d89..de466daf 100644 --- a/src/utils/saveSetup.ts +++ b/src/utils/saveSetup.ts @@ -5,22 +5,33 @@ * (`CustomPlayButton.ensureTrackingConfigured`). Anything that lives here can * be unit-tested without rendering the component or stubbing React. * - * The "server_unreachable" branch exists because `recommended_action` carries - * the explicit failure mode from the backend (see `get_save_setup_info`); the - * call site MUST NOT treat an empty `server_slots` array as authoritative on - * that path or it risks clobbering real server saves on first sync. + * The "server_unreachable" and "not_found" branches exist because + * `recommended_action` carries the explicit failure mode from the backend (see + * `get_save_setup_info`); the call site MUST NOT treat an empty `server_slots` + * array as authoritative on either path or it risks clobbering real server + * saves on first sync. The two hold identically and differ only in copy — one + * blames the connection, the other says RomM could not find what setup needs. */ import type { SaveSetupInfo } from "../types"; export type SaveSetupOutcome = - { kind: "server_unreachable" } | { kind: "auto_confirm"; slot: string } | { kind: "needs_user_choice" }; + | { kind: "server_unreachable" } + | { kind: "not_found" } + | { kind: "auto_confirm"; slot: string } + | { kind: "needs_user_choice" }; /** Resolve a SaveSetupInfo into the action its callers should take. */ export function resolveSaveSetupOutcome(info: SaveSetupInfo): SaveSetupOutcome { if (info.recommended_action === "server_unreachable") { return { kind: "server_unreachable" }; } + // The server answered that it has no such ROM or device id. Same hold as + // above — an empty `server_slots` is no more authoritative here than on an + // outage, so this must NOT fall through to the auto-confirm branch (#1570). + if (info.recommended_action === "not_found") { + return { kind: "not_found" }; + } // Either the backend marked the response as "auto_confirm_default", or the // server is reachable but reports no saves on either side — both are safe // to auto-confirm with the default slot. @@ -44,6 +55,17 @@ export const SERVER_UNREACHABLE_WIZARD_MESSAGE = export const SERVER_UNREACHABLE_TOAST_BODY = "Cannot configure save slot — RomM server is not reachable. Open the Saves tab to retry."; +/** Copy for the `not_found` branch — the server answered, it just has no such + * ROM or device id. Deliberately does NOT say the game has no saves: the 404 + * can come from this device's registration (a RomM database reset drops it), + * so claiming the saves don't exist would swap one lie for a more specific + * one. It states what RomM could not find, and that setup is on hold. */ +export const NOT_FOUND_WIZARD_MESSAGE = + "RomM couldn't find the save data for this setup — setup paused. The game or this device may no longer be on the server."; + +export const NOT_FOUND_TOAST_BODY = + "Cannot configure save slot — RomM couldn't find the save data for this setup. Open the Saves tab."; + const NEEDS_USER_CHOICE_TOAST_BODY = "Configure save sync in the Saves tab first"; /** Fallback toast body when `confirmSlotChoice` resolves to `success: false` @@ -85,6 +107,13 @@ export async function applyLaunchGateSetupOutcome( deps.dispatchSavesTab(); return "abort"; } + // Same abort, honest cause. The launch must NOT proceed with save tracking + // unconfigured just because the failure was a 404 rather than an outage. + if (outcome.kind === "not_found") { + deps.toast(NOT_FOUND_TOAST_BODY); + deps.dispatchSavesTab(); + return "abort"; + } if (outcome.kind === "auto_confirm") { // Auto-confirm of a named/default slot — never migrate (so no conflict). const result = await deps.confirmSlotChoice(deps.rid, outcome.slot, false, null, false); @@ -143,6 +172,11 @@ export async function applyWizardInitialSetupResult(result: SaveSetupInfo, deps: deps.setError(SERVER_UNREACHABLE_WIZARD_MESSAGE); return; } + if (result.recommended_action === "not_found") { + // Same hold, honest cause — never auto-confirm on an unproven server view. + deps.setError(NOT_FOUND_WIZARD_MESSAGE); + return; + } if (result.recommended_action === "auto_confirm_default") { deps.setConfirming(true); try { @@ -191,6 +225,11 @@ export function applyWizardRetrySetupResult(result: SaveSetupInfo, deps: WizardR deps.setLoading(false); return; } + if (result.recommended_action === "not_found") { + deps.setError(NOT_FOUND_WIZARD_MESSAGE); + deps.setLoading(false); + return; + } deps.setInfo(result); deps.setLoading(false); } diff --git a/tests/services/saves/test_slots.py b/tests/services/saves/test_slots.py index 5878e02c..8f8b7deb 100644 --- a/tests/services/saves/test_slots.py +++ b/tests/services/saves/test_slots.py @@ -490,6 +490,34 @@ async def test_server_error_with_oserror_recommends_server_unreachable(self, tmp assert result["recommended_action"] == "server_unreachable" assert result["server_query_failed"] is True + @pytest.mark.asyncio + async def test_definitive_404_recommends_not_found_and_still_holds(self, tmp_path): + """#1560's path: the 404 is the server ANSWERING, not an outage (#1570). + + The 404 here can come from the DEVICE id (a RomM database wipe drops + the registration), not just the ROM — so the empty server view is no + more authoritative than on an outage and the wizard must still hold. + Only the recommendation changes, so the frontend stops reporting the + server offline. + """ + svc, fake = make_service(tmp_path) + svc._config.settings["save_sync_enabled"] = True + _set_device_id(svc, "dev-1") + _install_rom(svc, tmp_path) + _create_save(tmp_path) + fake.fail_on_next(RommNotFoundError("HTTP 404: Not Found")) + + result = await svc.get_save_setup_info(42) + + assert result["recommended_action"] == "not_found" + assert result["recommended_action"] != "server_unreachable" + # The safety property is unchanged: never auto-confirm on an unproven + # server view, and never present the empty list as authoritative. + assert result["recommended_action"] != "auto_confirm_default" + assert result["server_query_failed"] is True + assert result["server_slots"] == [] + assert result["has_local_saves"] is True + @pytest.mark.asyncio async def test_server_error_preserves_local_info_in_response(self, tmp_path): """On server failure we still surface what we know locally.""" From 6518b81a7fc5b13dc117c85ee4fe3dd450425ccc Mon Sep 17 00:00:00 2001 From: danielcopper Date: Thu, 23 Jul 2026 18:35:50 +0200 Subject: [PATCH 4/7] style(ui): tighten the new frontend comments to stay inside the size budget MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rollup ships src/ comments verbatim in dist/index.js, and the bundle sits close to its 800 kB size-limit: 796.76 kB before this branch. The new comments alone pushed it to 800.61 kB and failed `pnpm size`. Condense them to the load-bearing reasoning — every "why" is kept, only restatement is dropped. Two blocks in SlotSetupWizard went entirely: the surrounding comments already said "any other resolved result proves the server answered", which covers the new not_found case. 799.34 kB. --- src/api/backend.ts | 8 +++--- src/components/CustomPlayButton.tsx | 7 ++--- src/components/SlotSetupWizard.tsx | 14 +++------ src/components/saves/VersionHistoryPanel.tsx | 9 ++---- src/types/saves.ts | 30 +++++++++----------- src/utils/saveSetup.ts | 21 ++++++-------- 6 files changed, 36 insertions(+), 53 deletions(-) diff --git a/src/api/backend.ts b/src/api/backend.ts index b915c49f..cf2d90ef 100755 --- a/src/api/backend.ts +++ b/src/api/backend.ts @@ -443,10 +443,10 @@ 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 definitive 404 on the bound id is NOT such a - * failure — the server answered, it just no longer has that ROM — so the - * local-only list comes back with `server_query_failed: false` and the picker - * leaves the shared connection state alone (#1570). + * (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). */ export interface VersionList { multi_version: boolean; diff --git a/src/components/CustomPlayButton.tsx b/src/components/CustomPlayButton.tsx index bf1c954f..629252f6 100644 --- a/src/components/CustomPlayButton.tsx +++ b/src/components/CustomPlayButton.tsx @@ -735,10 +735,9 @@ export const CustomPlayButton: FC = ({ appId }) => { // N // believing the conflict was cleared. Surface it and stay in conflict, // exactly like the network-throw catch below (#1276). if (result.server_query_failed) { - // Only an explicit unreachable verdict is a connectivity signal. A - // definitive 404 means the server ANSWERED, so feeding the global - // store off the bare flag would black out the whole UI over a ROM - // the server merely no longer has (#1570) — see connectionState.ts. + // Only an explicit unreachable verdict is a connectivity signal: + // feeding the store off the bare flag blacked out the whole UI over a + // ROM the server merely no longer has (#1570). if (result.server_query_reason === "server_unreachable") { reportServerReachable(false); } diff --git a/src/components/SlotSetupWizard.tsx b/src/components/SlotSetupWizard.tsx index 9e1b388a..e60e552e 100644 --- a/src/components/SlotSetupWizard.tsx +++ b/src/components/SlotSetupWizard.tsx @@ -516,12 +516,10 @@ function useSaveSetupInfo(romId: number, onComplete: () => void) { if (cancelled) return; // Feed the shared store (#1345): a server_unreachable result is a // definitive offline signal; any other resolved result proves the - // server answered. A throw is a bridge/unknown error, not a verdict — - // the catch leaves the store untouched. - // Only an explicit unreachable verdict means offline. A `not_found` - // result PROVES the server answered, so it reports reachable and - // leaves the reconnect gate disarmed — the wizard still holds, it - // just doesn't wait for a connection that was never lost (#1570). + // server answered — including a `not_found`, which still holds the + // wizard but leaves the reconnect gate disarmed (#1570). A throw is a + // bridge/unknown error, not a verdict — the catch leaves the store + // untouched. const reachable = result.recommended_action !== "server_unreachable"; reportServerReachable(reachable); offlineHeldRef.current = !reachable; @@ -580,10 +578,6 @@ function useSaveSetupInfo(romId: number, onComplete: () => void) { // Same conservative feed as the initial load (#1345): the manual // Retry re-probes reachability, so a server_unreachable result // re-arms offline and any other result reports the server back. - // Only an explicit unreachable verdict means offline. A `not_found` - // result PROVES the server answered, so it reports reachable and - // leaves the reconnect gate disarmed — the wizard still holds, it - // just doesn't wait for a connection that was never lost (#1570). const reachable = result.recommended_action !== "server_unreachable"; reportServerReachable(reachable); offlineHeldRef.current = !reachable; diff --git a/src/components/saves/VersionHistoryPanel.tsx b/src/components/saves/VersionHistoryPanel.tsx index 82059327..a8608007 100644 --- a/src/components/saves/VersionHistoryPanel.tsx +++ b/src/components/saves/VersionHistoryPanel.tsx @@ -51,9 +51,7 @@ export const VersionHistoryPanel: FC = ({ // hidden for multi-file slots anyway; this is the defensive backstop. setVersions(result.versions); } else if (result.status === "not_found") { - // The server answered: it has no such ROM / device id. Saying - // "couldn't reach RomM" here would blame a connection that is - // plainly working (#1570). + // The server answered — blaming the connection here would be a lie (#1570). detach(debugLog(`VersionHistoryPanel: server has no such entry for ${filename}: ${result.message}`)); setVersions(null); setLoadError("RomM no longer has this game's saves."); @@ -156,9 +154,8 @@ export const VersionHistoryPanel: FC = ({ // instead of telling the user the version is gone. toaster.toast({ title: "RomM Sync", body: "Couldn't reach RomM. Check your connection and try again." }); } else if (result.status === "not_found") { - // The mirror image of the branch above: RomM answered that it no - // longer has this game's saves, so a retry cannot help and the - // toast must not send the user off to check their connection (#1570). + // Mirror of the branch above: RomM answered, so a retry cannot help + // and the toast must not send the user to check their connection. toaster.toast({ title: "RomM Sync", body: "RomM no longer has this game's saves — nothing was restored." }); // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- exhaustive final branch of the 9-member RollbackStatus union; an explicit check (vs. plain `else`) keeps the per-status symmetry and leaves any future-added status unhandled instead of silently routing it to the "unsupported" toast } else if (result.status === "unsupported") { diff --git a/src/types/saves.ts b/src/types/saves.ts index ea0cd7d0..ef574e75 100644 --- a/src/types/saves.ts +++ b/src/types/saves.ts @@ -93,11 +93,9 @@ export interface SaveStatus { * and surface a misleading uploads-pending indicator on what is in * fact a connectivity blip. */ server_query_failed?: boolean; - /** Why that query failed, as a backend `classify_error` slug (`null` when - * it didn't). The flag says the server's view is unknown — true for a - * definitive 404 as much as for an outage — while this says *why*, so - * only `"server_unreachable"` may drive the global connection store. - * Feeding the store off the bare flag is the #1570 defect. */ + /** Why it failed (`null` when it didn't). The flag says the server's view is + * unknown — true for a 404 as much as for an outage; this says why, so only + * `"server_unreachable"` may drive the connection store (#1570). */ server_query_reason?: RommErrorCode | null; /** True when the active slot's current save spans more than one distinct * file (e.g. Sega Saturn `.bkr`/`.bcr`/`.smpc`). Those siblings are @@ -171,15 +169,15 @@ export interface SaveSetupInfo { default_slot: string; slot_confirmed: boolean; active_slot: string | null; - // "server_unreachable" means the server-saves fetch failed — the wizard MUST - // hold and offer a retry instead of treating the empty server_slots as - // authoritative (auto-confirming default would clobber real server saves on - // first sync). "not_found" is the same hold for a different cause: the - // server ANSWERED that it has no such ROM or device id, so the wizard must + // "server_unreachable" and "not_found" both mean the server-saves fetch + // failed, so the wizard MUST hold and offer a retry instead of treating the + // empty server_slots as authoritative (auto-confirming default would clobber + // real server saves on first sync). They differ only in cause: "not_found" + // is the server ANSWERING that it has no such ROM or device id, so it must // not report the server offline (#1570). See backend `get_save_setup_info`. recommended_action: "auto_confirm_default" | "show_wizard" | "server_unreachable" | "not_found"; - // True whenever the server-saves query failed, whichever way — an explicit - // flag for call sites that route on the boolean rather than the enum. + // True whenever that query failed, either way — for call sites routing on a + // boolean rather than the enum. server_query_failed?: boolean; } @@ -235,9 +233,9 @@ export type RollbackStatus = | { status: "version_deleted" } | { status: "unsupported" } | { status: "server_unreachable"; message: string } - // The server ANSWERED with a definitive 404 — it no longer has this ROM or - // the registered device id. Distinct from `server_unreachable` (retryable) - // and from `version_deleted` (one missing save inside a ROM it still has). + // The server ANSWERED 404 — no such ROM or device id. Distinct from + // `server_unreachable` (retryable) and `version_deleted` (one save missing + // from a ROM it still has). | { status: "not_found"; message: string } | { status: "conflict_blocked"; conflicts: SyncConflict[] } | { status: "preflight_failed"; errors: string[] } @@ -246,8 +244,8 @@ export type RollbackStatus = export type ListFileVersionsResult = | { status: "ok"; versions: SaveVersionEntry[] } | { status: "multi_file_unsupported"; versions: SaveVersionEntry[] } - | { status: "server_unreachable"; message: string } // See RollbackStatus — the server answered, it just has no such entry. + | { status: "server_unreachable"; message: string } | { status: "not_found"; message: string }; /** Discriminated-status result of `copySaveToSlot` — copies one server save into diff --git a/src/utils/saveSetup.ts b/src/utils/saveSetup.ts index de466daf..865980da 100644 --- a/src/utils/saveSetup.ts +++ b/src/utils/saveSetup.ts @@ -9,8 +9,7 @@ * `recommended_action` carries the explicit failure mode from the backend (see * `get_save_setup_info`); the call site MUST NOT treat an empty `server_slots` * array as authoritative on either path or it risks clobbering real server - * saves on first sync. The two hold identically and differ only in copy — one - * blames the connection, the other says RomM could not find what setup needs. + * saves on first sync. They hold identically and differ only in copy. */ import type { SaveSetupInfo } from "../types"; @@ -26,9 +25,8 @@ export function resolveSaveSetupOutcome(info: SaveSetupInfo): SaveSetupOutcome { if (info.recommended_action === "server_unreachable") { return { kind: "server_unreachable" }; } - // The server answered that it has no such ROM or device id. Same hold as - // above — an empty `server_slots` is no more authoritative here than on an - // outage, so this must NOT fall through to the auto-confirm branch (#1570). + // Same hold: an empty `server_slots` is no more authoritative on a 404 than + // on an outage, so this must NOT fall through to auto-confirm (#1570). if (info.recommended_action === "not_found") { return { kind: "not_found" }; } @@ -55,11 +53,9 @@ export const SERVER_UNREACHABLE_WIZARD_MESSAGE = export const SERVER_UNREACHABLE_TOAST_BODY = "Cannot configure save slot — RomM server is not reachable. Open the Saves tab to retry."; -/** Copy for the `not_found` branch — the server answered, it just has no such - * ROM or device id. Deliberately does NOT say the game has no saves: the 404 - * can come from this device's registration (a RomM database reset drops it), - * so claiming the saves don't exist would swap one lie for a more specific - * one. It states what RomM could not find, and that setup is on hold. */ +/** Copy for the `not_found` branch. Deliberately does NOT say the game has no + * saves: the 404 can come from this device's registration (a RomM database + * reset drops it), so that would swap one lie for a more specific one. */ export const NOT_FOUND_WIZARD_MESSAGE = "RomM couldn't find the save data for this setup — setup paused. The game or this device may no longer be on the server."; @@ -107,8 +103,8 @@ export async function applyLaunchGateSetupOutcome( deps.dispatchSavesTab(); return "abort"; } - // Same abort, honest cause. The launch must NOT proceed with save tracking - // unconfigured just because the failure was a 404 rather than an outage. + // Same abort, honest cause — a 404 must not let the launch proceed with save + // tracking unconfigured. if (outcome.kind === "not_found") { deps.toast(NOT_FOUND_TOAST_BODY); deps.dispatchSavesTab(); @@ -173,7 +169,6 @@ export async function applyWizardInitialSetupResult(result: SaveSetupInfo, deps: return; } if (result.recommended_action === "not_found") { - // Same hold, honest cause — never auto-confirm on an unproven server view. deps.setError(NOT_FOUND_WIZARD_MESSAGE); return; } From 3d56e724ac706430729e8968cf2ae0ea689e9bac Mon Sep 17 00:00:00 2001 From: danielcopper Date: Thu, 23 Jul 2026 19:19:31 +0200 Subject: [PATCH 5/7] fix(ui): finish the 404-vs-unreachable sweep across the saves surfaces MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review caught three consumers that still spoke of an unreachable server on an answered 404, and three smaller defects: - computeSyncSummary (the Saves-tab slot header, #1560's own surface) printed a literal "Server unreachable" off the bare server_query_failed flag. Gate it on server_query_reason: only server_unreachable says so, otherwise "Save status unavailable". - CustomPlayButton's resolve-conflict toast body was left ungated inside the branch whose store feed was already gated, so it asserted reachability on a 404. Gate the copy on the same slug. - VersionHistoryPanel's load and restore copy claimed the game's saves are gone. The 404 can be the device registration, not the ROM (the backend says so in its own comment), so it now says RomM couldn't find the save data — matching the wizard's wording. Each of these three strings now carries a test asserting it never claims reachability nor that the saves are absent; the two that had unpinned copy were how the drift got in. Also: the 404 gate's nested-try handling was the reverse of its docstring — a nested peel leaked into the outer handler's scan and would false-positive. Scope the dict walk to skip nested try statements (each is scanned as its own statement) and pin both directions with tests. Move a misplaced union comment onto the member it describes, and drop a dead duplicate mock setup. --- scripts/check_404_not_unreachable.py | 29 ++++++++++++--- src/components/CustomPlayButton.test.tsx | 22 +++++++++--- src/components/CustomPlayButton.tsx | 16 ++++++--- src/components/SlotSetupWizard.test.tsx | 3 -- .../saves/VersionHistoryPanel.test.tsx | 19 ++++++---- src/components/saves/VersionHistoryPanel.tsx | 7 ++-- src/components/saves/helpers.test.ts | 28 +++++++++++++-- src/components/saves/helpers.ts | 12 ++++--- src/types/saves.ts | 2 +- .../scripts/test_check_404_not_unreachable.py | 36 +++++++++++++++++++ 10 files changed, 140 insertions(+), 34 deletions(-) diff --git a/scripts/check_404_not_unreachable.py b/scripts/check_404_not_unreachable.py index 70dc449f..aea35420 100644 --- a/scripts/check_404_not_unreachable.py +++ b/scripts/check_404_not_unreachable.py @@ -62,9 +62,10 @@ The AST heuristic is intentionally conservative, and a guardrail rather than a prover: it reads one ``try`` statement at a time and does not follow a verdict returned by a helper the handler calls, nor one assembled across statements -(``resp = {...}; resp["reason"] = ...``). A ``try`` nested *inside* a catch-all -handler contributes its own dict literals to that handler's scan, so a nested -``except RommNotFoundError`` peel can clear the outer handler too. +(``resp = {...}; resp["reason"] = ...``). A ``try`` nested *inside* a handler is +scanned as its own statement, with its own sibling-peel check, and its dict +literals are NOT attributed to the enclosing handler — otherwise a nested peel +would read as an unpeeled verdict on the outer one and false-positive. """ from __future__ import annotations @@ -73,6 +74,10 @@ import sys from dataclasses import dataclass from pathlib import Path +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + from collections.abc import Iterator REPO_ROOT = Path(__file__).resolve().parent.parent SERVICES_DIR = REPO_ROOT / "py_modules" / "services" @@ -154,6 +159,22 @@ def _hardcoded_verdict_spelling(value: ast.expr) -> str | None: return None +def _walk_outside_nested_try(node: ast.AST) -> Iterator[ast.AST]: + """Yield *node*'s descendants, never descending into a nested ``ast.Try``. + + The caller's top-level walk visits every ``ast.Try`` in the module, so a + nested one is scanned on its own terms — with its own sibling-peel check. + Attributing its dict literals to the enclosing handler as well would make a + nested ``except RommNotFoundError`` peel look like an unpeeled verdict on + the outer handler, which is a false positive. + """ + for child in ast.iter_child_nodes(node): + if isinstance(child, ast.Try): + continue + yield child + yield from _walk_outside_nested_try(child) + + def _unreachable_spelling(handler: ast.ExceptHandler) -> str | None: """Return how *handler* hardcodes the unreachable verdict, or None. @@ -161,7 +182,7 @@ def _unreachable_spelling(handler: ast.ExceptHandler) -> str | None: anywhere else in the handler (a log line, a comparison against a classified reason) is not a hardcoded verdict. """ - for node in ast.walk(handler): + for node in _walk_outside_nested_try(handler): if not isinstance(node, ast.Dict): continue for key, value in zip(node.keys, node.values, strict=True): diff --git a/src/components/CustomPlayButton.test.tsx b/src/components/CustomPlayButton.test.tsx index dc95de8f..e555cb1e 100644 --- a/src/components/CustomPlayButton.test.tsx +++ b/src/components/CustomPlayButton.test.tsx @@ -1245,7 +1245,9 @@ describe("CustomPlayButton — resolve conflict reads the known conflict (#1276) }); it("server_query_failed keeps the button in conflict and toasts (does NOT silently proceed)", async () => { - vi.mocked(backend.getSaveStatus).mockResolvedValue(saveStatus({ server_query_failed: true, conflicts: [] })); + vi.mocked(backend.getSaveStatus).mockResolvedValue( + saveStatus({ server_query_failed: true, server_query_reason: "server_unreachable", conflicts: [] }), + ); const dataChanged = vi.fn(); globalThis.addEventListener("romm_data_changed", dataChanged); @@ -1274,12 +1276,16 @@ describe("CustomPlayButton — resolve conflict reads the known conflict (#1276) const utils = await renderInConflict(); await clickResolve(utils); - // Post-catch state: the global store actually flipped. + // Post-catch state: the global store actually flipped, and the copy still + // names the connection — that IS the cause on this branch. expect(getRommConnectionState()).toBe("offline"); + expect(vi.mocked(toaster.toast)).toHaveBeenCalledWith( + expect.objectContaining({ body: expect.stringContaining("Couldn't reach server") }), + ); await utils.findByText("Resolve Conflict"); }); - it("server_query_failed with a not_found reason leaves the store ALONE", async () => { + it("server_query_failed with a not_found reason leaves the store ALONE and does not blame the connection", async () => { setRommConnectionState("connected"); vi.mocked(backend.getSaveStatus).mockResolvedValue( saveStatus({ server_query_failed: true, server_query_reason: "not_found", conflicts: [] }), @@ -1289,9 +1295,15 @@ describe("CustomPlayButton — resolve conflict reads the known conflict (#1276) await clickResolve(utils); // The #1570 defect: a definitive 404 is the server ANSWERING, so feeding - // the global store off the bare flag blacked out the whole UI. The button - // must still refuse to claim the conflict was resolved. + // the global store off the bare flag blacked out the whole UI. The toast + // must not assert reachability either, nor claim the saves are gone — the + // 404 can be the device registration rather than the ROM. expect(getRommConnectionState()).toBe("connected"); + const toastCalls = vi.mocked(toaster.toast).mock.calls; + const body = toastCalls[toastCalls.length - 1]?.[0].body ?? ""; + expect(body).not.toMatch(/couldn't reach|unreachable|not reachable|offline/i); + expect(body).not.toMatch(/no saves|has no save|without saves/i); + // Still refuses to claim the conflict was resolved. expect(vi.mocked(handleConflicts)).not.toHaveBeenCalled(); await utils.findByText("Resolve Conflict"); }); diff --git a/src/components/CustomPlayButton.tsx b/src/components/CustomPlayButton.tsx index 629252f6..71aab6bc 100644 --- a/src/components/CustomPlayButton.tsx +++ b/src/components/CustomPlayButton.tsx @@ -735,14 +735,20 @@ export const CustomPlayButton: FC = ({ appId }) => { // N // believing the conflict was cleared. Surface it and stay in conflict, // exactly like the network-throw catch below (#1276). if (result.server_query_failed) { - // Only an explicit unreachable verdict is a connectivity signal: - // feeding the store off the bare flag blacked out the whole UI over a - // ROM the server merely no longer has (#1570). - if (result.server_query_reason === "server_unreachable") { + // Only an explicit unreachable verdict is a connectivity signal — for + // the store AND the copy. Off the bare flag both blamed the connection + // for a ROM the server merely no longer has (#1570). + const unreachable = result.server_query_reason === "server_unreachable"; + if (unreachable) { reportServerReachable(false); } detach(debugLog(`CustomPlayButton: resolve conflict — server query failed for rom ${romId}`)); - toaster.toast({ title: "RomM Sync", body: "Couldn't reach server to resolve conflict" }); + toaster.toast({ + title: "RomM Sync", + body: unreachable + ? "Couldn't reach server to resolve conflict" + : "RomM couldn't find this game's save data — conflict left unresolved", + }); setState("conflict"); return; } diff --git a/src/components/SlotSetupWizard.test.tsx b/src/components/SlotSetupWizard.test.tsx index b4a4e6bc..b310d64e 100644 --- a/src/components/SlotSetupWizard.test.tsx +++ b/src/components/SlotSetupWizard.test.tsx @@ -1120,9 +1120,6 @@ describe("SlotSetupWizard", () => { // #1560's path: the 404 means the server ANSWERED. Reporting offline // here blacked out the whole UI while RomM was working fine (#1570). setRommConnectionState("checking"); - vi.mocked(backend.getSaveSetupInfo).mockResolvedValue( - makeSetupInfo({ recommended_action: "not_found", server_query_failed: true }), - ); const result = makeSetupInfo({ recommended_action: "not_found", server_query_failed: true }); vi.mocked(backend.getSaveSetupInfo).mockResolvedValue(result); render(); diff --git a/src/components/saves/VersionHistoryPanel.test.tsx b/src/components/saves/VersionHistoryPanel.test.tsx index fa04b956..7ebc8b2a 100644 --- a/src/components/saves/VersionHistoryPanel.test.tsx +++ b/src/components/saves/VersionHistoryPanel.test.tsx @@ -155,9 +155,12 @@ describe("VersionHistoryPanel", () => { const { getByText, container } = render(); fireEvent.click(getByText("Previous Versions")); await flushAsync(); - expect(container.textContent).toContain("RomM no longer has this game's saves."); - // The server answered — blaming the connection is the #1570 defect. - expect(container.textContent).not.toContain("Couldn't reach RomM"); + expect(container.textContent).toContain("RomM couldn't find this game's save data."); + // The server answered, so the copy must not blame the connection — and it + // must not claim the saves are gone either: the 404 can be the device + // registration rather than the ROM (#1570). + expect(container.textContent).not.toMatch(/couldn't reach|unreachable|not reachable|offline/i); + expect(container.textContent).not.toMatch(/no saves|has no save|without saves/i); }); it("Retry button retriggers loadVersions", async () => { @@ -390,9 +393,13 @@ describe("VersionHistoryPanel", () => { fireEvent.click(getByText("Restore")); await flushAsync(); await flushAsync(); - expect(vi.mocked(toaster.toast)).toHaveBeenCalledWith( - expect.objectContaining({ body: "RomM no longer has this game's saves — nothing was restored." }), - ); + const toastCalls = vi.mocked(toaster.toast).mock.calls; + const body = toastCalls[toastCalls.length - 1]?.[0].body ?? ""; + expect(body).toBe("RomM couldn't find this game's save data — nothing was restored."); + // Same rule as the wizard copy: no reachability claim, no claim that the + // game's saves are gone (the 404 can be the device id) — see #1570. + expect(body).not.toMatch(/couldn't reach|unreachable|not reachable|offline/i); + expect(body).not.toMatch(/no saves|has no save|without saves/i); expect(onRestored).not.toHaveBeenCalled(); }); diff --git a/src/components/saves/VersionHistoryPanel.tsx b/src/components/saves/VersionHistoryPanel.tsx index a8608007..2b1f16ae 100644 --- a/src/components/saves/VersionHistoryPanel.tsx +++ b/src/components/saves/VersionHistoryPanel.tsx @@ -54,7 +54,7 @@ export const VersionHistoryPanel: FC = ({ // The server answered — blaming the connection here would be a lie (#1570). detach(debugLog(`VersionHistoryPanel: server has no such entry for ${filename}: ${result.message}`)); setVersions(null); - setLoadError("RomM no longer has this game's saves."); + setLoadError("RomM couldn't find this game's save data."); } else { detach(debugLog(`VersionHistoryPanel: server unreachable for ${filename}: ${result.message}`)); setVersions(null); @@ -154,9 +154,8 @@ export const VersionHistoryPanel: FC = ({ // instead of telling the user the version is gone. toaster.toast({ title: "RomM Sync", body: "Couldn't reach RomM. Check your connection and try again." }); } else if (result.status === "not_found") { - // Mirror of the branch above: RomM answered, so a retry cannot help - // and the toast must not send the user to check their connection. - toaster.toast({ title: "RomM Sync", body: "RomM no longer has this game's saves — nothing was restored." }); + // Mirror of the branch above: RomM answered, so a retry cannot help. + toaster.toast({ title: "RomM Sync", body: "RomM couldn't find this game's save data — nothing was restored." }); // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- exhaustive final branch of the 9-member RollbackStatus union; an explicit check (vs. plain `else`) keeps the per-status symmetry and leaves any future-added status unhandled instead of silently routing it to the "unsupported" toast } else if (result.status === "unsupported") { toaster.toast({ title: "RomM Sync", body: "Version history requires RomM 4.7+" }); diff --git a/src/components/saves/helpers.test.ts b/src/components/saves/helpers.test.ts index 4312c8fd..d89c6138 100644 --- a/src/components/saves/helpers.test.ts +++ b/src/components/saves/helpers.test.ts @@ -291,9 +291,10 @@ describe("computeSyncSummary", () => { }); }); - it("short-circuits to 'Server unreachable' when server_query_failed is true", () => { + it("short-circuits to 'Server unreachable' when the reason is server_unreachable", () => { const status = makeStatus({ server_query_failed: true, + server_query_reason: "server_unreachable", files: [ { filename: "save.srm", @@ -317,8 +318,31 @@ describe("computeSyncSummary", () => { }); }); + it("does NOT claim the server is unreachable on a definitive 404", () => { + // The Saves tab is #1560's own surface: saying "Server unreachable" for a + // ROM the server merely answered 404 for is the exact lie #1570 removes. + // It still must not run the matrix against the empty server list. + const status = makeStatus({ + server_query_failed: true, + server_query_reason: "not_found", + files: [], + }); + const { syncSummaryText } = computeSyncSummary(true, status, []); + expect(syncSummaryText).not.toMatch(/unreachable|not reachable|offline/i); + expect(syncSummaryText).not.toMatch(/no saves|has no save|without saves/i); + expect(syncSummaryText).toBe("Save status unavailable"); + }); + + it("does not claim unreachable when the query failed without a reason slug", () => { + // Defensive: an older backend (or an unclassified failure) sends the flag + // with no reason. Silence about the cause beats asserting the wrong one. + const status = makeStatus({ server_query_failed: true, files: [] }); + const { syncSummaryText } = computeSyncSummary(true, status, []); + expect(syncSummaryText).not.toMatch(/unreachable|not reachable|offline/i); + }); + it("server_query_failed wins over conflicts", () => { - const status = makeStatus({ server_query_failed: true }); + const status = makeStatus({ server_query_failed: true, server_query_reason: "server_unreachable" }); const conflicts: SyncConflict[] = [ { type: "sync_conflict", diff --git a/src/components/saves/helpers.ts b/src/components/saves/helpers.ts index 54fdb071..0b8f35b1 100644 --- a/src/components/saves/helpers.ts +++ b/src/components/saves/helpers.ts @@ -123,9 +123,11 @@ export function statusLabel(status: string, lastSyncAt: string | null): { color: * Build the active slot's sync-summary header line. * * Returns the empty (null) state for inactive slots or missing status. When - * the backend signals `server_query_failed`, short-circuits with a neutral - * "Server unreachable" instead of running the matrix-derived classification - * (which would otherwise read an empty server list as "ready to upload"). + * the backend signals `server_query_failed`, short-circuits instead of running + * the matrix-derived classification (which would read an empty server list as + * "ready to upload"). Only an explicit `server_query_reason` of + * `server_unreachable` may say so in the text — on an answered 404 that would + * be false (#1570). * * The summary is derived from the per-file matrix statuses so it can never * disagree with the per-file badges (`statusLabel`): any file pending upload @@ -142,7 +144,9 @@ export function computeSyncSummary( if (!isActive || !saveStatus) return { syncSummaryText: null, syncSummaryColor: MUTED_COLOR }; if (saveStatus.server_query_failed) { - return { syncSummaryText: "Server unreachable", syncSummaryColor: MUTED_COLOR }; + const text = + saveStatus.server_query_reason === "server_unreachable" ? "Server unreachable" : "Save status unavailable"; + return { syncSummaryText: text, syncSummaryColor: MUTED_COLOR }; } const hasConflict = conflicts.length > 0; diff --git a/src/types/saves.ts b/src/types/saves.ts index ef574e75..9a44c2cc 100644 --- a/src/types/saves.ts +++ b/src/types/saves.ts @@ -244,8 +244,8 @@ export type RollbackStatus = export type ListFileVersionsResult = | { status: "ok"; versions: SaveVersionEntry[] } | { status: "multi_file_unsupported"; versions: SaveVersionEntry[] } - // See RollbackStatus — the server answered, it just has no such entry. | { status: "server_unreachable"; message: string } + // See RollbackStatus — the server answered, it just has no such entry. | { status: "not_found"; message: string }; /** Discriminated-status result of `copySaveToSlot` — copies one server save into diff --git a/tests/scripts/test_check_404_not_unreachable.py b/tests/scripts/test_check_404_not_unreachable.py index 296383cf..aab873de 100644 --- a/tests/scripts/test_check_404_not_unreachable.py +++ b/tests/scripts/test_check_404_not_unreachable.py @@ -248,6 +248,42 @@ def f(): """) assert findings == [] + def test_nested_try_peeling_the_404_does_not_flag_the_outer_handler(self): + """A nested ``try`` is scanned on its own terms, not the enclosing one. + + Attributing the inner dict literals to the outer handler made a nested + peel read as an unpeeled verdict — a false positive, which would + wrongly block a future PR. + """ + findings = _scan(""" + def f(): + try: + outer() + except Exception: + try: + fetch() + except RommNotFoundError: + return {"recommended_action": "not_found", "server_query_failed": True} + except Exception: + return {"recommended_action": "server_unreachable", "server_query_failed": True} + """) + assert findings == [] + + def test_nested_try_does_not_mask_the_outer_handlers_own_verdict(self): + """The exclusion is scoped: a verdict in the OUTER body is still found.""" + findings = _scan(""" + def f(): + try: + outer() + except Exception: + try: + cleanup() + except Exception: + pass + return {"success": False, "reason": ErrorCode.SERVER_UNREACHABLE.value, "message": "m"} + """) + assert len(findings) == 1 + def test_handler_with_no_return_dict(self): findings = _scan(""" def f(): From 59c6bdbfc2439aaff5e71738bed385fe88fb6979 Mon Sep 17 00:00:00 2001 From: danielcopper Date: Thu, 23 Jul 2026 21:56:34 +0200 Subject: [PATCH 6/7] fix(saves): classify a 404 as not_found in copy_save_to_slot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit copy_save_to_slot landed on main during PR 1's review; its post-preflight list_saves collapsed every failure onto server_unreachable — the same conflation this branch removes, which its own new gate now flags. The call is rom- AND device-scoped, so a definitive 404 is the #1560 family (dead ROM or dropped device registration): the server answered. Add a sibling except RommNotFoundError before the catch-all, returning the discriminated not_found status and persisting the pre-flight-mutated save_state first, exactly as the server_unreachable branch does — otherwise that work is lost. The catch-all keeps server_unreachable for genuine transport failures. Frontend: add the not_found member to CopySaveToSlotStatus and a case in useCopyToSlot that neither reports the server offline nor claims the game's saves are gone (the 404 can be the device registration). Tests pin the backend branch (incl. that the pre-flight timestamp was persisted) and the copy rule for the new string. --- py_modules/services/saves/copies.py | 18 +++++++-- src/components/saves/useCopyToSlot.test.tsx | 15 ++++++++ src/components/saves/useCopyToSlot.ts | 7 ++++ src/types/saves.ts | 2 + tests/services/saves/test_copies.py | 41 +++++++++++++++++++++ 5 files changed, 80 insertions(+), 3 deletions(-) diff --git a/py_modules/services/saves/copies.py b/py_modules/services/saves/copies.py index ecd34b1b..a46e40e9 100644 --- a/py_modules/services/saves/copies.py +++ b/py_modules/services/saves/copies.py @@ -20,7 +20,7 @@ from domain.save_layout import SAVE_SYNC_CONTENT_DIR_REASON from domain.save_slot import save_in_slot from domain.save_status import compute_multi_file_slot -from lib.errors import RommConflictError +from lib.errors import RommConflictError, RommNotFoundError from services.saves._helpers import local_save_target from services.saves._settings import resolve_default_slot @@ -256,7 +256,10 @@ async def copy_save_to_slot(self, rom_id: int, save_id: int, target_slot: str) - unconditionally, to protect its dirty local before the copy overwrites it — ``conflict_blocked`` / ``preflight_failed`` on a bad pre-flight. 7. ``list_saves`` with no slot filter (the source may be in any slot); - failure is ``server_unreachable``. + a transport failure is ``server_unreachable``, a definitive 404 (the + server answered that it has no such ROM or device id) is ``not_found`` + — distinct so the copy surface never reports an answering server + offline (#1570). 8. The copy proper runs in :meth:`_copy_save_to_slot_io`. A dedup pre-check between the ``list_saves`` and the copy short-circuits @@ -267,7 +270,7 @@ async def copy_save_to_slot(self, rom_id: int, save_id: int, target_slot: str) - Returns a discriminated-status dict (mirrors ``RollbackStatus``): ``ok | already_present{existing_id} | not_configured | invalid_slot_name | rom_not_installed | version_deleted | unsupported{reason?} | - server_unreachable{message} | conflict_blocked{conflicts} | + server_unreachable{message} | not_found{message} | conflict_blocked{conflicts} | preflight_failed{errors} | target_slot_busy{message} | copy_failed{message}``. """ rom_id = int(rom_id) @@ -332,6 +335,15 @@ async def copy_save_to_slot(self, rom_id: int, save_id: int, target_slot: str) - None, lambda: self._retry.with_retry(lambda: self._romm_api.list_saves(rom_id, device_id=device_id)), ) + except RommNotFoundError as e: + # The server ANSWERED: it has no such ROM (or no such device id). + # ``list_saves`` is rom- AND device-scoped, so this is the #1560 + # family. Not a connectivity verdict — the copy surface must not + # report the server offline (#1570). + self._log_debug(f"copy_save_to_slot: server has no such entity: {e}") + # Persist whatever the pre-flight mutated before bailing. + await self._loop.run_in_executor(None, self._write_save_state, rom_id, save_state) + return {"status": "not_found", "message": str(e)} except Exception as e: self._log_debug(f"copy_save_to_slot: failed to list saves: {e}") # Persist whatever the pre-flight mutated before bailing. diff --git a/src/components/saves/useCopyToSlot.test.tsx b/src/components/saves/useCopyToSlot.test.tsx index c7c364d4..ca1b6900 100644 --- a/src/components/saves/useCopyToSlot.test.tsx +++ b/src/components/saves/useCopyToSlot.test.tsx @@ -136,6 +136,7 @@ describe("useCopyToSlot", () => { { result: { status: "not_configured" }, needle: "Set up save slots" }, { result: { status: "invalid_slot_name" }, needle: "valid slot name" }, { result: { status: "server_unreachable", message: "boom" }, needle: "Couldn't reach RomM" }, + { result: { status: "not_found", message: "HTTP 404" }, needle: "couldn't find this game's save data" }, { result: { status: "preflight_failed", errors: ["net"] }, needle: "Sync failed before copy" }, { result: { status: "copy_failed", message: "oops" }, needle: "Couldn't copy the save" }, { result: { status: "unsupported", reason: "savefiles_in_content_dir" }, needle: "writes saves next to the ROM" }, @@ -151,6 +152,20 @@ describe("useCopyToSlot", () => { expect(dataChanged).not.toHaveBeenCalled(); }); + it("not_found: names what RomM couldn't find, never a reachability or no-saves claim", async () => { + // The 404 means the server ANSWERED (#1560 family). Same rule as every + // other 404 surface in #1570: no offline blame, and no claim the game's + // saves are gone (the 404 can be the device registration). + await runCopy({ status: "not_found", message: "HTTP 404: Not Found" }); + + const toastCalls = vi.mocked(toaster.toast).mock.calls; + const body = String(toastCalls[toastCalls.length - 1]![0].body ?? ""); + expect(body).not.toMatch(/couldn't reach|check your connection|unreachable|not reachable|offline/i); + expect(body).not.toMatch(/no saves|has no save|without saves/i); + // A refusal never refreshes the views. + expect(dataChanged).not.toHaveBeenCalled(); + }); + it("swallows a rejected copy call and surfaces the generic failure toast", async () => { vi.mocked(backend.copySaveToSlot).mockRejectedValue(new Error("network down")); const hook = renderHook(() => useCopyToSlot(42, SLOTS)); diff --git a/src/components/saves/useCopyToSlot.ts b/src/components/saves/useCopyToSlot.ts index 9da5b860..4d639582 100644 --- a/src/components/saves/useCopyToSlot.ts +++ b/src/components/saves/useCopyToSlot.ts @@ -60,6 +60,13 @@ async function handleResult(result: CopySaveToSlotStatus, target: string, romId: case "server_unreachable": toaster.toast({ title: TITLE, body: "Couldn't reach RomM. Check your connection and try again." }); return; + case "not_found": + // The server answered — it has no such ROM or device id (#1560 family). + // A retry can't help, so the copy must not send the user to check their + // connection, and must not claim the saves are gone: the 404 can be the + // device registration rather than the ROM (#1570). + toaster.toast({ title: TITLE, body: "RomM couldn't find this game's save data — nothing was copied." }); + return; case "version_deleted": toaster.toast({ title: TITLE, body: "This save no longer exists on the server." }); return; diff --git a/src/types/saves.ts b/src/types/saves.ts index 9a44c2cc..e6e71f23 100644 --- a/src/types/saves.ts +++ b/src/types/saves.ts @@ -260,6 +260,8 @@ export type CopySaveToSlotStatus = | { status: "version_deleted" } | { status: "unsupported"; reason?: string } | { status: "server_unreachable"; message: string } + // See RollbackStatus — the server answered, it just has no such entry. + | { status: "not_found"; message: string } | { status: "conflict_blocked"; conflicts: SyncConflict[] } | { status: "preflight_failed"; errors: string[] } | { status: "target_slot_busy"; message: string } diff --git a/tests/services/saves/test_copies.py b/tests/services/saves/test_copies.py index 4ad05bb5..e97b0e6c 100644 --- a/tests/services/saves/test_copies.py +++ b/tests/services/saves/test_copies.py @@ -12,6 +12,7 @@ import pytest from domain.save_layout import ContentDir +from lib.errors import RommNotFoundError from tests.services.saves._helpers import ( _create_save, _enable_sync_with_device, @@ -387,6 +388,46 @@ def fail_second_list(*args, **kwargs): assert "unreachable" in result.get("message", "").lower() assert "error" not in result + @pytest.mark.asyncio + async def test_definitive_404_on_post_preflight_list_is_not_found(self, tmp_path): + """A 404 on the post-preflight ``list_saves`` → not_found, not offline (#1570). + + ``list_saves`` is rom- AND device-scoped, so a definitive 404 is the + #1560 family (dead ROM or dropped device registration) — the server + answered. It must read as ``not_found`` rather than ``server_unreachable``, + and — like the transport branch — it must still persist whatever the + pre-flight mutated before bailing, or that work is lost. + """ + svc, fake = make_service(tmp_path) + _create_save(tmp_path) + local_hash = _file_md5(str(tmp_path / "saves" / "gba" / "pokemon.srm")) + _setup_configured(svc, tmp_path, active_slot="autosave", tracked_id=100, last_sync_hash=local_hash) + fake.saves[100] = _tracked_save(100, slot="autosave") + # Seeded state has no sync-check timestamp; the pre-flight sets one. + assert _require_save_state(svc, 42).last_sync_check_at is None + + original_list = fake.list_saves + call_count = {"n": 0} + + def fail_second_list(*args, **kwargs): + call_count["n"] += 1 + if call_count["n"] >= 2: + raise RommNotFoundError("HTTP 404: Not Found") + return original_list(*args, **kwargs) + + fake.list_saves = fail_second_list # type: ignore[method-assign] + try: + result = await svc.copy_save_to_slot(42, 100, TARGET) + finally: + fake.list_saves = original_list # type: ignore[method-assign] + + assert result["status"] == "not_found" + assert result["status"] != "server_unreachable" + assert "message" in result + # The pre-flight mutation was persisted before bailing (mark_sync_evaluated + # ran, so the seeded-None timestamp is now set on disk). + assert _require_save_state(svc, 42).last_sync_check_at is not None + @pytest.mark.asyncio async def test_target_slot_busy_on_409(self, tmp_path): """The target slot has a newer foreign save this device hasn't synced → 409 → target_slot_busy.""" From 90bd59ed1dac71bce8f8cc0b283ff816a9972991 Mon Sep 17 00:00:00 2001 From: danielcopper Date: Thu, 23 Jul 2026 22:18:10 +0200 Subject: [PATCH 7/7] test(errors): assert call count with toHaveLength (sonar S5906) --- src/components/SlotSetupWizard.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/SlotSetupWizard.test.tsx b/src/components/SlotSetupWizard.test.tsx index b310d64e..4b7133a7 100644 --- a/src/components/SlotSetupWizard.test.tsx +++ b/src/components/SlotSetupWizard.test.tsx @@ -1151,7 +1151,7 @@ describe("SlotSetupWizard", () => { }); await flushAsync(); - expect(vi.mocked(backend.getSaveSetupInfo).mock.calls.length).toBe(callsAfterLoad); + expect(vi.mocked(backend.getSaveSetupInfo).mock.calls).toHaveLength(callsAfterLoad); }); it("auto-reloads on reconnect after holding the offline error", async () => {