From 300acf2594eab4a107a3cfa1b5ffc56c903f09ee Mon Sep 17 00:00:00 2001 From: danielcopper Date: Thu, 23 Jul 2026 22:57:53 +0200 Subject: [PATCH 1/3] fix(saves): re-register the device when RomM no longer has its id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When RomM's database is wiped or restored, the device id the plugin cached no longer exists server-side, so every device-scoped save call 404s. The best-effort update_device touch in ensure_device_registered already makes that 404 the natural, zero-extra-request healing point, but it swallowed the error and trusted the dead id forever. Peel a definitive RommNotFoundError off that touch: on a 404, forget the dead id (clearing the kv_config row) and fall through to the existing registration path so a fresh id is minted and persisted. Every other touch failure stays swallowed best-effort — a transport blip must not throw away a valid id and churn a re-registration. If the re-registration itself fails mid-heal, the method returns the classified failure with an empty id and the dead id left cleared, a clean state the next call retries. Healing fires on every plugin load, so the fix applies itself the moment the release is installed — no user action required. Closes #1560. --- docs/user-guide/troubleshooting.md | 9 +- .../services/saves/sync_engine/devices.py | 44 +++-- .../saves/sync_engine/test_devices.py | 155 +++++++++++++++++- 3 files changed, 195 insertions(+), 13 deletions(-) diff --git a/docs/user-guide/troubleshooting.md b/docs/user-guide/troubleshooting.md index cb96ed14..4e8eaaac 100644 --- a/docs/user-guide/troubleshooting.md +++ b/docs/user-guide/troubleshooting.md @@ -85,12 +85,17 @@ the RomM database was reset and this device's registration disappeared with it **RomM offline** badge stays clear and the surface tells you what's missing instead. If you see such a message while the badge is clear, the fix is on the RomM side (re-sync the library, or re-check the game), not with your network. +When the missing thing is this device's own registration — the RomM database was wiped or restored and the id this +device was issued no longer exists — you don't have to do anything. The next time the plugin loads (installing an update +counts, since that reloads it) it notices the id is gone, registers this device afresh, and save sync carries on under +the new id. A new entry shows up in RomM's device list; the old, dead one can be ignored. + 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**. +sync. Re-check the game in RomM (a server database reset re-registers this device on its own the next time the plugin +loads — see above), then tap **Retry**. ### Save file not found diff --git a/py_modules/services/saves/sync_engine/devices.py b/py_modules/services/saves/sync_engine/devices.py index 4d997a6f..46c162e0 100644 --- a/py_modules/services/saves/sync_engine/devices.py +++ b/py_modules/services/saves/sync_engine/devices.py @@ -16,7 +16,7 @@ import contextlib from typing import TYPE_CHECKING, Any -from lib.errors import classify_error +from lib.errors import RommNotFoundError, classify_error from lib.list_result import ErrorCode from services.saves._settings import save_sync_enabled @@ -182,6 +182,14 @@ async def ensure_device_registered( fingerprint sent as the RomM ``hostname`` so the server dedupes this device across reinstalls. When the machine id is unreadable (``None``) the call degrades to no-fingerprint registration. + + A cached device id is trusted only as far as the best-effort + ``update_device`` touch below: a definitive 404 from that touch + means the server no longer has this device (its database was wiped + or restored out from under the cached id), so the dead id is + forgotten and a fresh one is registered in its place. Every other + touch failure is a transient miss that leaves the cached id in + force — a server blip must never churn a re-registration. """ if not save_sync_enabled(self._settings): return { @@ -217,22 +225,38 @@ async def ensure_device_registered( device_id = await loop.run_in_executor(None, self.get_device_id) if device_id: server_id_str = str(device_id) - # Best-effort touch of the server-side client_version. A failure here - # is non-fatal (the device is already registered), but log it at debug - # so the swallow leaves a breadcrumb instead of vanishing silently. + # Best-effort touch of the server-side client_version, with one + # exception peeled off: a definitive ``RommNotFoundError`` (404) + # means the server no longer has this device (its DB was wiped or + # restored), so the cached id is dead — forget it and fall through + # to the registration path below to mint a fresh one (#1560). Every + # OTHER failure is non-fatal (the device is still registered): keep + # the cached id and return it, logging at debug so the swallow + # leaves a breadcrumb. A transient server blip must NEVER throw away + # a valid id and churn a spurious re-registration. + dead_device = False try: await loop.run_in_executor( None, lambda: self._romm_api.update_device(server_id_str, client_version=self._plugin_version), ) + except RommNotFoundError as e: + self._log_debug( + f"ensure_device_registered: server no longer has device {server_id_str} (404), re-registering: {e}" + ) + await loop.run_in_executor(None, self.forget_device) + dead_device = True except Exception as e: self._log_debug(f"ensure_device_registered: update_device failed (non-fatal): {e}") - return { - "success": True, - "device_id": device_id, - "device_name": self._get_device_name() or "", - "server_device_id": device_id, - } + if not dead_device: + return { + "success": True, + "device_id": device_id, + "device_name": self._get_device_name() or "", + "server_device_id": device_id, + } + # dead_device: the cached id was forgotten above; fall through to + # registration so a fresh id is minted and persisted. hostname = hostname_provider.get() machine_id = machine_id_provider.get() diff --git a/tests/services/saves/sync_engine/test_devices.py b/tests/services/saves/sync_engine/test_devices.py index 2971f321..f38a4b31 100644 --- a/tests/services/saves/sync_engine/test_devices.py +++ b/tests/services/saves/sync_engine/test_devices.py @@ -14,7 +14,15 @@ from fakes.fake_save_api import FakeSaveApi from fakes.fake_unit_of_work import FakeUnitOfWorkFactory -from lib.errors import RommApiError, RommAuthError, RommConnectionError, RommForbiddenError, RommSSLError +from lib.errors import ( + RommApiError, + RommAuthError, + RommConnectionError, + RommForbiddenError, + RommNotFoundError, + RommSSLError, + RommTimeoutError, +) from lib.list_result import ErrorCode from services.saves.sync_engine.devices import DeviceRegistry from tests.services.saves._helpers import ( @@ -303,6 +311,151 @@ async def test_update_device_failure_logs_at_debug_and_still_succeeds(self, tmp_ assert any("update_device failed" in m and "boom" in m for m in debug_log) +class TestEnsureDeviceRegisteredReRegistersDeadDevice: + """#1560: after a RomM database wipe/restore the cached device id no longer + exists server-side, so the best-effort ``update_device`` touch gets a + definitive 404. That 404 (and ONLY that) forgets the dead id and + re-registers a fresh one; every other touch failure stays a best-effort + swallow that keeps the cached id, so a server blip never churns a + re-registration.""" + + @pytest.mark.asyncio + async def test_live_cached_id_touch_succeeds_no_reregistration(self, tmp_path): + svc, fake = make_service(tmp_path) + _enable_sync_with_device(svc, "server-uuid") + # Stamp a version so the pre-register heartbeat probe is skipped — the + # touch lands on update_device, which succeeds against a live id. + fake.set_version("4.9.0") + + result = await svc.ensure_device_registered() + + assert result["success"] is True + assert result["device_id"] == "server-uuid" + # The live touch fired and kept the id — no re-registration, id intact. + assert any(c[0] == "update_device" for c in fake.call_log) + assert _register_call(fake) is None + assert _get_device_id(svc) == "server-uuid" + + @pytest.mark.asyncio + async def test_dead_cached_id_forgets_and_reregisters(self, tmp_path): + debug_log: list[str] = [] + svc, fake = make_service(tmp_path, log_debug=debug_log.append) + _enable_sync_with_device(svc, "server-uuid") + fake.set_version("4.9.0") + # The server no longer has this device → the touch 404s (the #1560 wedge). + fake.fail_on_next(RommNotFoundError("Device with ID server-uuid not found")) + + result = await svc.ensure_device_registered() + + # A fresh id was minted and persisted, and it is the one returned — not + # the stale, dead one. + assert result["success"] is True + new_id = result["device_id"] + assert new_id + assert new_id != "server-uuid" + # register_device ran (the forget fell through to registration)... + assert _register_call(fake) is not None + # ...and the persisted kv_config id is now the fresh id, not the dead one. + assert _get_device_id(svc) == new_id + assert result["server_device_id"] == new_id + # The heal took the 404 branch (naming the dead id), not the generic swallow. + assert any("server no longer has device server-uuid" in m for m in debug_log) + + @pytest.mark.asyncio + async def test_transport_failure_on_touch_is_not_a_heal(self, tmp_path): + svc, fake = make_service(tmp_path) + _enable_sync_with_device(svc, "server-uuid") + fake.set_version("4.9.0") + # A transport blip on the touch — NOT a definitive 404. The device is + # still registered; the id must be kept, not forgotten + re-registered. + fake.fail_on_next(RommConnectionError("Connection refused")) + + result = await svc.ensure_device_registered() + + assert result["success"] is True + assert result["device_id"] == "server-uuid" + # The cached id was KEPT — no forget, no re-registration. + assert _get_device_id(svc) == "server-uuid" + assert _register_call(fake) is None + + @pytest.mark.asyncio + async def test_timeout_on_touch_is_not_a_heal(self, tmp_path): + svc, fake = make_service(tmp_path) + _enable_sync_with_device(svc, "server-uuid") + fake.set_version("4.9.0") + fake.fail_on_next(RommTimeoutError("timed out")) + + result = await svc.ensure_device_registered() + + assert result["success"] is True + assert result["device_id"] == "server-uuid" + assert _get_device_id(svc) == "server-uuid" + assert _register_call(fake) is None + + @pytest.mark.asyncio + async def test_generic_exception_on_touch_is_not_a_heal(self, tmp_path): + """Edge: only a RommNotFoundError heals — a non-RomM error stays a swallow. + + Guards the discriminator: broadening the peel to ``except Exception`` + would re-register on any error, throwing away a valid id. + """ + svc, fake = make_service(tmp_path) + _enable_sync_with_device(svc, "server-uuid") + fake.set_version("4.9.0") + fake.fail_on_next(RuntimeError("unexpected")) + + result = await svc.ensure_device_registered() + + assert result["success"] is True + assert result["device_id"] == "server-uuid" + assert _get_device_id(svc) == "server-uuid" + assert _register_call(fake) is None + + @pytest.mark.asyncio + async def test_heal_then_reregister_fails_leaves_clean_state(self, tmp_path): + svc, fake = make_service(tmp_path) + _enable_sync_with_device(svc, "server-uuid") + fake.set_version("4.9.0") + + # The touch 404s (dead id) → forget → but the server goes away mid-heal, + # so the re-registration itself fails. The touch is monkeypatched to 404 + # (leaving the one-shot ``fail_on_next`` for the register_device that + # follows), since a single ``fail_on_next`` can only arm one call. + def _touch_404(*_args, **_kwargs): + raise RommNotFoundError("Device with ID server-uuid not found") + + fake.update_device = _touch_404 + fake.fail_on_next(RommConnectionError("Connection refused")) + + result = await svc.ensure_device_registered() + + # Clean, recoverable state: classified failure, empty id, dead id cleared. + assert result["success"] is False + assert result["reason"] == ErrorCode.SERVER_UNREACHABLE.value + assert result["message"] + assert result["device_id"] == "" + # The dead id was forgotten and NOT replaced — kv_config is left cleared, + # so the next ensure_device_registered retries cleanly from None. + assert _get_device_id(svc) is None + + @pytest.mark.asyncio + async def test_no_cached_id_goes_straight_to_registration(self, tmp_path): + """First-time path is unchanged: no cached id → no touch, register runs.""" + svc, fake = make_service(tmp_path) + svc._config.settings["save_sync_enabled"] = True + fake.set_version("4.9.0") + # No device_id persisted → registration branch. + + result = await svc.ensure_device_registered() + + assert result["success"] is True + assert result["device_id"] + # The update_device touch is only for an existing id — it must not run on + # the first-time path. + assert not any(c[0] == "update_device" for c in fake.call_log) + assert _register_call(fake) is not None + + class TestPermissionDegradedNeverDropsDeviceId: """#1437 regression guard: a permission-degraded (403) registration must NEVER delete ``kv_config["device_id"]``. The only in-process deleter is From 6e07a57690e4d0ea0f9d1069b964bd16078dfaed Mon Sep 17 00:00:00 2001 From: danielcopper Date: Thu, 23 Jul 2026 23:41:57 +0200 Subject: [PATCH 2/3] fix(saves): run the device liveness heal before every sync, not just on absence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The four sync callers gated the device-registration heal on presence, not liveness: a dead-but-present device id (kv_config still holds it, but the server 404s it after a database wipe/restore) is truthy, so the `if not get_device_id()` guard skipped ensure_device_registered and the run went straight into list_saves with the dead id — "Downloaded 0 saves, 1 error". That is the same presence-not-liveness bug as the update_device touch itself, now one level up in the callers. Extract a shared _ensure_device_live_or_fail helper and call it from all four syncs (pre_launch, post_exit, sync_rom_saves, sync_all_saves). It runs ensure_device_registered UNCONDITIONALLY, so the update_device liveness touch validates and — via the 404 heal — re-registers a dead id BEFORE the sync reads it. The sync then uses the fresh id: _run_rom_sync reads it through _read_sync_inputs, and sync_all_saves re-reads it for the bulk negotiate, both AFTER the heal, so neither carries a stale value. A live id costs one extra 200 touch per sync, the accepted price of correctness. Docs: the troubleshooting note now says a device the server no longer has is re-registered both on plugin load and before every save-sync. --- docs/user-guide/troubleshooting.md | 12 +- .../services/saves/sync_engine/engine.py | 75 ++++--- .../services/saves/sync_engine/test_engine.py | 193 +++++++++++++++++- 3 files changed, 242 insertions(+), 38 deletions(-) diff --git a/docs/user-guide/troubleshooting.md b/docs/user-guide/troubleshooting.md index 4e8eaaac..ad8b4343 100644 --- a/docs/user-guide/troubleshooting.md +++ b/docs/user-guide/troubleshooting.md @@ -86,16 +86,18 @@ the RomM database was reset and this device's registration disappeared with it badge is clear, the fix is on the RomM side (re-sync the library, or re-check the game), not with your network. When the missing thing is this device's own registration — the RomM database was wiped or restored and the id this -device was issued no longer exists — you don't have to do anything. The next time the plugin loads (installing an update -counts, since that reloads it) it notices the id is gone, registers this device afresh, and save sync carries on under -the new id. A new entry shows up in RomM's device list; the old, dead one can be ignored. +device was issued no longer exists — you don't have to do anything. Both when the plugin loads (installing an update +counts, since that reloads it) and before every save-sync (before a game launches, after it exits, or a manual sync), +the plugin first checks that this device's id still exists on the server; the moment it finds the id is gone it +registers this device afresh and carries on under the new id. A new entry shows up in RomM's device list; the old, dead +one can be ignored. 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 (a server database reset re-registers this device on its own the next time the plugin -loads — see above), then tap **Retry**. +sync. Re-check the game in RomM (a server database reset re-registers this device automatically — see above), then tap +**Retry**. ### Save file not found diff --git a/py_modules/services/saves/sync_engine/engine.py b/py_modules/services/saves/sync_engine/engine.py index 1d11a73e..91a27188 100644 --- a/py_modules/services/saves/sync_engine/engine.py +++ b/py_modules/services/saves/sync_engine/engine.py @@ -399,6 +399,37 @@ async def ensure_device_registered(self) -> dict[str, Any]: machine_id_provider=self._machine_id_provider, ) + async def _ensure_device_live_or_fail(self) -> dict[str, Any] | None: + """Register-or-heal the server device id before a sync consumes it. + + Runs :meth:`ensure_device_registered` UNCONDITIONALLY — not only when + the id is absent. Its best-effort ``update_device`` touch doubles as a + liveness probe, so a dead-but-present cached id (e.g. after a RomM + database wipe/restore, where ``kv_config`` still holds an id the server + now 404s) is detected and re-registered HERE, before + :meth:`_run_rom_sync` reads the id and calls ``list_saves`` with a dead + one (#1560). Gating the heal on ``if not get_device_id()`` — presence, + not liveness — is the exact bug one level up from the touch's own + presence-not-liveness gate. A live id costs one extra 200 touch per + sync: the accepted price of correctness (no "recently validated" + caching). + + Returns the canonical ``DEVICE_NOT_REGISTERED`` failure dict when a live + registration could not be established (the caller returns it verbatim), + else ``None`` — the caller proceeds and the sync reads the fresh, + possibly-healed id through :meth:`_read_sync_inputs` (and, for the bulk + sweep, the re-read at ``sync_all_saves``), never a value captured before + the heal. + """ + reg = await self.ensure_device_registered() + if not reg.get("success"): + return { + "success": False, + "reason": DEVICE_NOT_REGISTERED_REASON, + "message": DEVICE_NOT_REGISTERED, + } + return None + async def list_devices(self) -> dict[str, Any]: """List all devices registered with the RomM server for this user.""" return await self._devices.list_devices(loop=self._loop) @@ -725,14 +756,9 @@ async def pre_launch_sync(self, rom_id: int) -> dict[str, Any]: except Exception as e: return self._heartbeat_failure_result("pre_launch_sync", e) - if not self.get_device_id(): - reg = await self.ensure_device_registered() - if not reg.get("success"): - return { - "success": False, - "reason": DEVICE_NOT_REGISTERED_REASON, - "message": DEVICE_NOT_REGISTERED, - } + failure = await self._ensure_device_live_or_fail() + if failure is not None: + return failure uploaded, downloaded, errors, conflicts = await self._run_rom_sync(rom_id) synced = uploaded + downloaded @@ -810,14 +836,9 @@ async def post_exit_sync(self, rom_id: int) -> dict[str, Any]: except Exception as e: return self._heartbeat_failure_result("post_exit_sync", e) - if not self.get_device_id(): - reg = await self.ensure_device_registered() - if not reg.get("success"): - return { - "success": False, - "reason": DEVICE_NOT_REGISTERED_REASON, - "message": DEVICE_NOT_REGISTERED, - } + failure = await self._ensure_device_live_or_fail() + if failure is not None: + return failure uploaded, downloaded, errors, conflicts = await self._run_rom_sync(rom_id) synced = uploaded + downloaded @@ -893,14 +914,9 @@ async def sync_rom_saves(self, rom_id: int) -> dict[str, Any]: if self._save_sync_blocked(): return self._content_dir_skip() - if not self.get_device_id(): - reg = await self.ensure_device_registered() - if not reg.get("success"): - return { - "success": False, - "reason": DEVICE_NOT_REGISTERED_REASON, - "message": DEVICE_NOT_REGISTERED, - } + failure = await self._ensure_device_live_or_fail() + if failure is not None: + return failure uploaded, downloaded, errors, conflicts = await self._run_rom_sync(rom_id) synced = uploaded + downloaded @@ -1004,14 +1020,9 @@ async def sync_all_saves(self) -> dict[str, Any]: if self._save_sync_blocked(): return self._content_dir_skip(all_saves=True) - if not self.get_device_id(): - reg = await self.ensure_device_registered() - if not reg.get("success"): - return { - "success": False, - "reason": DEVICE_NOT_REGISTERED_REASON, - "message": DEVICE_NOT_REGISTERED, - } + failure = await self._ensure_device_live_or_fail() + if failure is not None: + return failure # One whole-device transport-only negotiate session wraps the # sweep (ADR-0017); when it can't open, each confirmed non-legacy diff --git a/tests/services/saves/sync_engine/test_engine.py b/tests/services/saves/sync_engine/test_engine.py index 642d76f5..6b7e9782 100644 --- a/tests/services/saves/sync_engine/test_engine.py +++ b/tests/services/saves/sync_engine/test_engine.py @@ -23,12 +23,18 @@ RommAuthError, RommConnectionError, RommForbiddenError, + RommNotFoundError, RommSSLError, RommSyncDisabledError, RommTimeoutError, ) from lib.list_result import ErrorCode -from services.saves._messages import DEVICE_SYNC_DISABLED, DEVICE_SYNC_DISABLED_REASON +from services.saves._messages import ( + DEVICE_NOT_REGISTERED, + DEVICE_NOT_REGISTERED_REASON, + DEVICE_SYNC_DISABLED, + DEVICE_SYNC_DISABLED_REASON, +) from services.saves.sync_engine.engine import _first_error_reason, _summarize_sync_result from tests.services.saves._helpers import ( _create_save, @@ -2052,3 +2058,188 @@ def malformed(device_id, saves): assert not any(c[0] == "complete_sync_session" for c in fake.call_log) assert any(c[0] == "list_saves" for c in fake.call_log) assert result["synced"] == 1 + + +class TestSyncPathsHealDeadDevice: + """#1560 (caller-level): the four sync callers gate the device-registration + heal on liveness, not mere presence. ``_ensure_device_live_or_fail`` runs + ``ensure_device_registered`` UNCONDITIONALLY before each sync, so a + dead-but-present cached id (kv_config holds it, the server 404s it) is + healed by the ``update_device`` liveness touch BEFORE ``_run_rom_sync`` + reads the id — and the sync then queries the server with the fresh, healed + id (read via ``_read_sync_inputs`` / the bulk re-read), never the dead one. + """ + + # ------------------------------------------------------------------ + # Helper-level: the single point of logic the four callers share. + # ------------------------------------------------------------------ + + @pytest.mark.asyncio + async def test_helper_dead_id_heals_and_exposes_new_id(self, tmp_path): + svc, fake = make_service(tmp_path) + _enable_sync_with_device(svc, "dead-uuid") + fake.set_version("4.9.0") + # The present id is dead → the update_device liveness touch 404s. + fake.fail_on_next(RommNotFoundError("Device with ID dead-uuid not found")) + + failure = await svc._sync_engine._ensure_device_live_or_fail() + + # Heal succeeded → no failure returned, and get_device_id (what + # _read_sync_inputs reads) now yields the fresh id, not the dead one. + assert failure is None + new_id = svc._sync_engine.get_device_id() + assert new_id is not None + assert new_id != "dead-uuid" + assert _get_device_id(svc) == new_id + + @pytest.mark.asyncio + async def test_helper_live_id_touches_and_keeps_id(self, tmp_path): + svc, fake = make_service(tmp_path) + _enable_sync_with_device(svc, "live-device") + fake.set_version("4.9.0") + + failure = await svc._sync_engine._ensure_device_live_or_fail() + + # Live id: the liveness touch fired, no re-registration, id unchanged. + assert failure is None + assert any(c[0] == "update_device" and c[1][0] == "live-device" for c in fake.call_log) + assert not any(c[0] == "register_device" for c in fake.call_log) + assert svc._sync_engine.get_device_id() == "live-device" + + @pytest.mark.asyncio + async def test_helper_reregister_failure_returns_device_not_registered(self, tmp_path): + svc, fake = make_service(tmp_path) + _enable_sync_with_device(svc, "dead-uuid") + fake.set_version("4.9.0") + + # Dead id heals into a re-register, but the server goes away mid-heal so + # registration itself fails. Touch monkeypatched to 404 so the one-shot + # fail_on_next can arm the register_device that follows. + def _touch_404(*_args, **_kwargs): + raise RommNotFoundError("Device with ID dead-uuid not found") + + fake.update_device = _touch_404 + fake.fail_on_next(RommConnectionError("server gone")) + + failure = await svc._sync_engine._ensure_device_live_or_fail() + + # The caller is handed the canonical DEVICE_NOT_REGISTERED failure... + assert failure is not None + assert failure["success"] is False + assert failure["reason"] == DEVICE_NOT_REGISTERED_REASON + assert failure["message"] == DEVICE_NOT_REGISTERED + # ...and the dead id was cleared (not left to poison the next sync). + assert _get_device_id(svc) is None + + # ------------------------------------------------------------------ + # Per-caller integration: the sync's server call carries the healed id. + # ------------------------------------------------------------------ + + @pytest.mark.asyncio + async def test_pre_launch_sync_heals_dead_id_and_queries_with_new_id(self, tmp_path): + svc, fake = make_service(tmp_path) + svc._config.settings["save_sync_enabled"] = True + _set_device_id(svc, "dead-uuid") + _install_rom(svc, tmp_path) + fake.set_version("4.9.0") + fake.saves[100] = _server_save() + fake.fail_on_next(RommNotFoundError("Device with ID dead-uuid not found")) + + result = await svc.pre_launch_sync(42) + + assert result["success"] is True + new_id = _get_device_id(svc) + assert new_id is not None and new_id != "dead-uuid" + list_calls = [c for c in fake.call_log if c[0] == "list_saves"] + assert list_calls # the sync actually reached the server + assert all(c[2]["device_id"] == new_id for c in list_calls) + assert not any(c[2]["device_id"] == "dead-uuid" for c in list_calls) + + @pytest.mark.asyncio + async def test_post_exit_sync_heals_dead_id_and_queries_with_new_id(self, tmp_path): + svc, fake = make_service(tmp_path) + svc._config.settings["save_sync_enabled"] = True + _set_device_id(svc, "dead-uuid") + _install_rom(svc, tmp_path) + _create_save(tmp_path, content=b"local save bytes") + fake.set_version("4.9.0") + fake.fail_on_next(RommNotFoundError("Device with ID dead-uuid not found")) + + result = await svc.post_exit_sync(42) + + assert result["success"] is True + new_id = _get_device_id(svc) + assert new_id is not None and new_id != "dead-uuid" + list_calls = [c for c in fake.call_log if c[0] == "list_saves"] + assert list_calls + assert all(c[2]["device_id"] == new_id for c in list_calls) + assert not any(c[2]["device_id"] == "dead-uuid" for c in list_calls) + + @pytest.mark.asyncio + async def test_sync_rom_saves_heals_dead_id_and_queries_with_new_id(self, tmp_path): + svc, fake = make_service(tmp_path) + svc._config.settings["save_sync_enabled"] = True + _set_device_id(svc, "dead-uuid") + _install_rom(svc, tmp_path) + fake.set_version("4.9.0") + fake.saves[100] = _server_save() + fake.fail_on_next(RommNotFoundError("Device with ID dead-uuid not found")) + + result = await svc.sync_rom_saves(42) + + assert result["success"] is True + new_id = _get_device_id(svc) + assert new_id is not None and new_id != "dead-uuid" + list_calls = [c for c in fake.call_log if c[0] == "list_saves"] + assert list_calls + assert all(c[2]["device_id"] == new_id for c in list_calls) + assert not any(c[2]["device_id"] == "dead-uuid" for c in list_calls) + + @pytest.mark.asyncio + async def test_sync_all_saves_heals_dead_id_and_bulk_re_read_uses_new_id(self, tmp_path): + svc, fake = make_service(tmp_path) + svc._config.settings["save_sync_enabled"] = True + _set_device_id(svc, "dead-uuid") + _install_rom(svc, tmp_path, rom_id=1, system="gba", file_name="game1.gba") + _seed_save_state(svc, 1, RomSaveSyncState(system="gba", slot_confirmed=True, active_slot="default")) + _create_save(tmp_path, system="gba", rom_name="game1", content=b"s1") + fake.set_version("4.9.0") + fake.fail_on_next(RommNotFoundError("Device with ID dead-uuid not found")) + + result = await svc.sync_all_saves() + + assert result["success"] is True + new_id = _get_device_id(svc) + assert new_id is not None and new_id != "dead-uuid" + # The bulk pre-negotiate re-reads the id AFTER the heal (sync_all_saves), + # so the whole-device negotiate carries the fresh id — not the dead one. + neg_calls = [c for c in fake.call_log if c[0] == "negotiate_sync"] + assert neg_calls + assert all(c[1][0] == new_id for c in neg_calls) + assert not any(c[1][0] == "dead-uuid" for c in neg_calls) + # And each per-ROM matrix query carries the fresh id too. + list_calls = [c for c in fake.call_log if c[0] == "list_saves"] + assert list_calls + assert all(c[2]["device_id"] == new_id for c in list_calls) + + @pytest.mark.asyncio + async def test_sync_rom_saves_live_id_touches_and_syncs_unchanged(self, tmp_path): + svc, fake = make_service(tmp_path) + svc._config.settings["save_sync_enabled"] = True + _set_device_id(svc, "live-device") + _install_rom(svc, tmp_path) + fake.set_version("4.9.0") + fake.saves[100] = _server_save() + + result = await svc.sync_rom_saves(42) + + # The unconditional liveness touch fired on the live id, no re-register, + # and the sync proceeded exactly as before under the same present id. + assert result["success"] is True + assert result["synced"] == 1 + assert any(c[0] == "update_device" and c[1][0] == "live-device" for c in fake.call_log) + assert not any(c[0] == "register_device" for c in fake.call_log) + assert _get_device_id(svc) == "live-device" + list_calls = [c for c in fake.call_log if c[0] == "list_saves"] + assert list_calls + assert all(c[2]["device_id"] == "live-device" for c in list_calls) From d5e1e80c0d54c60db58c8f6c7584f4bb7733658e Mon Sep 17 00:00:00 2001 From: danielcopper Date: Fri, 24 Jul 2026 13:42:25 +0200 Subject: [PATCH 3/3] test(saves): prove every sync path aborts without a live device registration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The four sync entry points call _ensure_device_live_or_fail and return its failure verbatim, but nothing pinned what happens when no live registration can be established. Each entry point now has a test that drives the failure through the real DeviceRegistry and asserts both halves: the canonical device_not_registered dict comes back, and the sync itself never reaches the server (no list_saves, negotiate_sync, upload_save or download_save_content). The absence is the point — a sync that ran anyway would attribute every save it moved to a device the server does not have. The registration failure is modelled as a 200 whose body carries no device id, via a new one-shot arm on FakeSaveApi. It leaves the device unregistered without a transport error, so no reachability guard upstream of the device gate can account for the abort. Also covers resolve_core's install gate: an uninstalled ROM resolves no core and never consults the active-core seam. --- tests/fakes/fake_save_api.py | 17 +++ .../services/saves/sync_engine/test_engine.py | 142 ++++++++++++++++++ 2 files changed, 159 insertions(+) diff --git a/tests/fakes/fake_save_api.py b/tests/fakes/fake_save_api.py index 83a2b997..ff856b21 100644 --- a/tests/fakes/fake_save_api.py +++ b/tests/fakes/fake_save_api.py @@ -60,6 +60,9 @@ def __init__(self, save_file_store: SaveFileStore | None = None) -> None: # One-shot: arm the next slot POST to model add_save's content-dedup # early-return against this save_id (see ``arm_add_save_dedup``). self._dedup_next_upload_save_id: int | None = None + # One-shot: arm the next device registration to answer without an id + # (see ``arm_register_device_without_id``). + self._register_without_id: bool = False # Native play-session store (ADR-0018): rom_id -> stored session dicts. # ``ingest_play_sessions`` appends here and dedupes on # ``(device_id, rom_id, start_time)``. @@ -133,6 +136,17 @@ def arm_add_save_dedup(self, save_id: int) -> None: """ self._dedup_next_upload_save_id = save_id + def arm_register_device_without_id(self) -> None: + """One-shot: model a 200 registration whose body carries no device id. + + RomM answers ``POST /devices`` with the DeviceSchema; a proxy or a + version mismatch can answer 200 with a body the client finds no ``id`` + in. The next ``register_device`` returns such a body and records no + device, so registration yields nothing usable without any transport + error — the client is left unregistered. + """ + self._register_without_id = True + def seed_foreign_save( self, rom_id: int, @@ -435,6 +449,9 @@ def register_device( ) -> dict[str, Any]: self.call_log.append(("register_device", (name, platform, client, client_version), {"hostname": hostname})) self._check_fail() + if self._register_without_id: + self._register_without_id = False + return {"name": name, "created_at": datetime.now(UTC).isoformat()} device_id = f"device-{self._next_device_id}" self._next_device_id += 1 device = {"id": device_id, "name": name, "created_at": datetime.now(UTC).isoformat()} diff --git a/tests/services/saves/sync_engine/test_engine.py b/tests/services/saves/sync_engine/test_engine.py index 6b7e9782..5088af98 100644 --- a/tests/services/saves/sync_engine/test_engine.py +++ b/tests/services/saves/sync_engine/test_engine.py @@ -15,6 +15,7 @@ import zipfile import pytest +from fakes.fake_active_core_resolver import FakeActiveCoreResolver from domain.rom_save_sync_state import RomSaveSyncState from domain.save_layout import ContentDir, InSaveDir @@ -1384,6 +1385,29 @@ async def test_list_devices_delegates_to_device_registry(self, tmp_path): assert result["devices"][0]["is_current_device"] is True +class TestResolveCore: + """``resolve_core`` gates on the install record before consulting the + per-game active-core seam — an uninstalled ROM has no launch to stamp an + upload's emulator tag against, so the core lookup never runs for it.""" + + def test_uninstalled_rom_resolves_no_core(self, tmp_path): + # A core is configured for every ROM, so ``None`` can only come from the + # install gate. + active_core = FakeActiveCoreResolver(default=("mgba_libretro.so", "mGBA")) + svc, _ = make_service(tmp_path, active_core=active_core) + + assert svc._sync_engine.resolve_core(42) is None + assert active_core.calls == [] + + def test_installed_rom_resolves_the_active_core(self, tmp_path): + active_core = FakeActiveCoreResolver(per_rom={42: ("mgba_libretro.so", "mGBA")}) + svc, _ = make_service(tmp_path, active_core=active_core) + _install_rom(svc, tmp_path) + + assert svc._sync_engine.resolve_core(42) == "mgba_libretro.so" + assert active_core.calls == [42] + + class TestSaveSyncContentDirGate: """All four public sync entry points hard-gate save sync when RetroArch writes saves to the content dir (savefiles_in_content_dir=true). The gate @@ -2243,3 +2267,121 @@ async def test_sync_rom_saves_live_id_touches_and_syncs_unchanged(self, tmp_path list_calls = [c for c in fake.call_log if c[0] == "list_saves"] assert list_calls assert all(c[2]["device_id"] == "live-device" for c in list_calls) + + +# The server calls only a *running* sync makes. Device registration and +# heartbeat traffic is deliberately excluded — those fire on the way TO the +# sync, while these four are the sync itself reaching the server. +_SYNC_SERVER_CALLS = ("list_saves", "negotiate_sync", "upload_save", "download_save_content") + + +def _sync_server_calls(fake) -> list[str]: + """Names of the sync's own server calls recorded on *fake*, in order.""" + return [call[0] for call in fake.call_log if call[0] in _SYNC_SERVER_CALLS] + + +class TestSyncPathsAbortWhenDeviceUnregisterable: + """#1560 (caller-level, failure half): when no live device registration can + be established, each of the four sync entry points returns the canonical + ``DEVICE_NOT_REGISTERED`` failure and runs no sync at all. + + The guarantee under test is the *absence*: a sync must never reach the + server without a live device id, because every save it moved would be + attributed to a device the server does not have. Each test mirrors the setup + of its :class:`TestSyncPathsHealDeadDevice` twin — same install, same + local/server save — where a live id IS established and the sync does call + the server, so the contrast isolates the abort from a sync that had nothing + to do anyway. + + The registration failure modelled here is a 200 whose body carries no device + id: it leaves the device unregistered without a transport error, so no + reachability guard upstream of the device gate can account for the abort. + """ + + @pytest.mark.asyncio + async def test_pre_launch_sync_aborts_and_downloads_nothing(self, tmp_path): + svc, fake = make_service(tmp_path) + svc._config.settings["save_sync_enabled"] = True + _install_rom(svc, tmp_path) + fake.set_version("4.9.0") + fake.saves[100] = _server_save() + fake.arm_register_device_without_id() + + result = await svc.pre_launch_sync(42) + + assert result == { + "success": False, + "reason": DEVICE_NOT_REGISTERED_REASON, + "message": DEVICE_NOT_REGISTERED, + } + # Registration was attempted and yielded no id — the abort is the device + # gate's, not an upstream guard's. + assert any(c[0] == "register_device" for c in fake.call_log) + assert _get_device_id(svc) is None + # The server save the registered twin downloads was never even queried. + assert _sync_server_calls(fake) == [] + + @pytest.mark.asyncio + async def test_post_exit_sync_aborts_and_uploads_nothing(self, tmp_path): + svc, fake = make_service(tmp_path) + svc._config.settings["save_sync_enabled"] = True + _install_rom(svc, tmp_path) + _create_save(tmp_path, content=b"local save bytes") + fake.set_version("4.9.0") + fake.arm_register_device_without_id() + + result = await svc.post_exit_sync(42) + + assert result == { + "success": False, + "reason": DEVICE_NOT_REGISTERED_REASON, + "message": DEVICE_NOT_REGISTERED, + } + assert any(c[0] == "register_device" for c in fake.call_log) + assert _get_device_id(svc) is None + # The local save the registered twin uploads stays put — nothing is + # pushed to the server unattributed. + assert _sync_server_calls(fake) == [] + + @pytest.mark.asyncio + async def test_sync_rom_saves_aborts_and_syncs_nothing(self, tmp_path): + svc, fake = make_service(tmp_path) + svc._config.settings["save_sync_enabled"] = True + _install_rom(svc, tmp_path) + fake.set_version("4.9.0") + fake.saves[100] = _server_save() + fake.arm_register_device_without_id() + + result = await svc.sync_rom_saves(42) + + assert result == { + "success": False, + "reason": DEVICE_NOT_REGISTERED_REASON, + "message": DEVICE_NOT_REGISTERED, + } + assert any(c[0] == "register_device" for c in fake.call_log) + assert _get_device_id(svc) is None + assert _sync_server_calls(fake) == [] + + @pytest.mark.asyncio + async def test_sync_all_saves_aborts_before_the_bulk_negotiate(self, tmp_path): + svc, fake = make_service(tmp_path) + svc._config.settings["save_sync_enabled"] = True + _install_rom(svc, tmp_path, rom_id=1, system="gba", file_name="game1.gba") + _seed_save_state(svc, 1, RomSaveSyncState(system="gba", slot_confirmed=True, active_slot="default")) + _create_save(tmp_path, system="gba", rom_name="game1", content=b"s1") + fake.set_version("4.9.0") + fake.arm_register_device_without_id() + + result = await svc.sync_all_saves() + + assert result == { + "success": False, + "reason": DEVICE_NOT_REGISTERED_REASON, + "message": DEVICE_NOT_REGISTERED, + } + assert any(c[0] == "register_device" for c in fake.call_log) + assert _get_device_id(svc) is None + # The sweep aborts ahead of the whole-device negotiate — no session is + # opened for a device the server never issued an id for. + assert _sync_server_calls(fake) == []