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..cb96ed14 100644 --- a/docs/user-guide/troubleshooting.md +++ b/docs/user-guide/troubleshooting.md @@ -79,6 +79,19 @@ 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. + +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/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/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/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/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/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/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/scripts/check_404_not_unreachable.py b/scripts/check_404_not_unreachable.py new file mode 100644 index 00000000..aea35420 --- /dev/null +++ b/scripts/check_404_not_unreachable.py @@ -0,0 +1,314 @@ +#!/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 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 + +import ast +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" + +# 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 _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. + + 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 _walk_outside_nested_try(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/src/api/backend.ts b/src/api/backend.ts index cdca121f..cf2d90ef 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 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.test.tsx b/src/components/CustomPlayButton.test.tsx index 6e2d485a..e555cb1e 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"; @@ -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); @@ -1265,6 +1267,47 @@ 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, 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 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: [] }), + ); + + 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 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"); + }); + 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..71aab6bc 100644 --- a/src/components/CustomPlayButton.tsx +++ b/src/components/CustomPlayButton.tsx @@ -730,14 +730,25 @@ 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 — 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 99263a3c..4b7133a7 100644 --- a/src/components/SlotSetupWizard.test.tsx +++ b/src/components/SlotSetupWizard.test.tsx @@ -1116,6 +1116,44 @@ 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"); + 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).toHaveLength(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..e60e552e 100644 --- a/src/components/SlotSetupWizard.tsx +++ b/src/components/SlotSetupWizard.tsx @@ -516,8 +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. + // 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; diff --git a/src/components/saves/VersionHistoryPanel.test.tsx b/src/components/saves/VersionHistoryPanel.test.tsx index 7cd701c2..7ebc8b2a 100644 --- a/src/components/saves/VersionHistoryPanel.test.tsx +++ b/src/components/saves/VersionHistoryPanel.test.tsx @@ -147,6 +147,22 @@ 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 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 () => { vi.mocked(backend.savesListFileVersions) .mockResolvedValueOnce({ status: "server_unreachable", message: "boom" }) @@ -368,6 +384,25 @@ 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(); + 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(); + }); + 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..2b1f16ae 100644 --- a/src/components/saves/VersionHistoryPanel.tsx +++ b/src/components/saves/VersionHistoryPanel.tsx @@ -50,6 +50,11 @@ 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 — 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 couldn't find this game's save data."); } else { detach(debugLog(`VersionHistoryPanel: server unreachable for ${filename}: ${result.message}`)); setVersions(null); @@ -148,7 +153,10 @@ 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") { + // 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/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 3f1cfe60..e6e71f23 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,10 @@ export interface SaveStatus { * and surface a misleading uploads-pending indicator on what is in * fact a connectivity blip. */ server_query_failed?: boolean; + /** 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 * components of one game state, not prior versions — so the frontend @@ -163,13 +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). 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. + // "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 that query failed, either way — for call sites routing on a + // boolean rather than the enum. server_query_failed?: boolean; } @@ -225,6 +233,10 @@ export type RollbackStatus = | { status: "version_deleted" } | { status: "unsupported" } | { status: "server_unreachable"; message: string } + // 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[] } | { 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). @@ -246,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/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..865980da 100644 --- a/src/utils/saveSetup.ts +++ b/src/utils/saveSetup.ts @@ -5,22 +5,31 @@ * (`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. They hold identically and differ only in copy. */ 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" }; } + // 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" }; + } // 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 +53,15 @@ 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. 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."; + +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 +103,13 @@ export async function applyLaunchGateSetupOutcome( deps.dispatchSavesTab(); return "abort"; } + // 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(); + 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 +168,10 @@ export async function applyWizardInitialSetupResult(result: SaveSetupInfo, deps: deps.setError(SERVER_UNREACHABLE_WIZARD_MESSAGE); return; } + if (result.recommended_action === "not_found") { + deps.setError(NOT_FOUND_WIZARD_MESSAGE); + return; + } if (result.recommended_action === "auto_confirm_default") { deps.setConfirming(true); try { @@ -191,6 +220,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/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/scripts/test_check_404_not_unreachable.py b/tests/scripts/test_check_404_not_unreachable.py new file mode 100644 index 00000000..aab873de --- /dev/null +++ b/tests/scripts/test_check_404_not_unreachable.py @@ -0,0 +1,365 @@ +"""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_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(): + 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 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_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.""" 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..8f8b7deb 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. @@ -459,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.""" @@ -1037,6 +1096,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 +1280,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 +2043,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 +2062,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 +2282,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