Skip to content

Commit 0845a97

Browse files
committed
fix(saves): tell the setup wizard a 404 is an answer, not an outage
get_save_setup_info collapsed every server-saves failure onto recommended_action "server_unreachable", so a RomM database reset — which drops this device's registration and makes the saves query 404 — told the user their server was offline while it was answering fine. Add a fourth arm, "not_found", peeled off with a sibling except RommNotFoundError. It holds the wizard exactly as the unreachable branch does: a 404 leaves the server's slot inventory just as unproven as an outage, so auto-confirming the default slot could still clobber real server saves on the first post-confirmation sync. The launch gate likewise still aborts. Only the copy and the reachability verdict change. The copy deliberately does not say the game has no saves. The 404 can come from the device registration rather than the ROM, so that would swap one lie for a more specific one; it states what RomM could not find instead. SlotSetupWizard needed no change: it already tested for an explicit server_unreachable, so a not_found result now correctly reports the server reachable and leaves the reconnect gate disarmed. This clears the last site flagged by check_404_not_unreachable.py.
1 parent 9eba6f7 commit 0845a97

8 files changed

Lines changed: 257 additions & 22 deletions

File tree

docs/user-guide/troubleshooting.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,13 @@ the RomM database was reset and this device's registration disappeared with it
8585
**RomM offline** badge stays clear and the surface tells you what's missing instead. If you see such a message while the
8686
badge is clear, the fix is on the RomM side (re-sync the library, or re-check the game), not with your network.
8787

88+
The first-time save-slot setup screen is the clearest example. If RomM can't find the save data that setup needs, it
89+
pauses with "RomM couldn't find the save data for this setup" rather than the "server is not reachable" message, and the
90+
offline badge stays clear. Setup deliberately stops there instead of picking a slot for you: the plugin has no
91+
trustworthy view of what's on the server, and choosing a slot on that basis could overwrite real saves on the first
92+
sync. Re-check the game in RomM (or, after a server database reset, reconnect in the plugin's Connection settings so
93+
this device is registered again), then tap **Retry**.
94+
8895
### Save file not found
8996

9097
**Symptom**: The game detail page shows save status but no save file is being synced.

py_modules/services/saves/slots/setup.py

Lines changed: 61 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
from domain.rom_save_sync_state import RomSaveSyncState
1919
from domain.save_layout import SAVE_SYNC_CONTENT_DIR_REASON
2020
from domain.save_slot import save_in_slot
21-
from lib.errors import classify_error
21+
from lib.errors import RommNotFoundError, classify_error
2222
from services.saves._helpers import newest_server_saves_by_target
2323
from services.saves._messages import (
2424
DEVICE_NOT_REGISTERED_REASON,
@@ -96,11 +96,45 @@ def is_save_tracking_configured(self, rom_id: int) -> dict[str, Any]:
9696
active_slot = game_state.active_slot if (game_state and configured) else None
9797
return {"configured": configured, "active_slot": active_slot}
9898

99+
def _setup_query_failed(
100+
self,
101+
*,
102+
local_files: list[dict[str, Any]],
103+
local_file_info: list[dict[str, Any]],
104+
game_state: RomSaveSyncState | None,
105+
default_slot: str,
106+
recommended_action: str,
107+
) -> dict[str, Any]:
108+
"""Build the held-wizard payload for a failed server-saves query.
109+
110+
Every failure branch holds the wizard the same way — an unproven
111+
server view must never auto-confirm the default slot, whatever the
112+
cause. Only *recommended_action* differs, so the frontend can say
113+
why without claiming the server is offline.
114+
"""
115+
slot_confirmed = bool(game_state.slot_confirmed) if game_state else False
116+
return {
117+
"has_local_saves": len(local_files) > 0,
118+
"local_files": local_file_info,
119+
"server_slots": [],
120+
"default_slot": default_slot,
121+
"slot_confirmed": slot_confirmed,
122+
"active_slot": game_state.active_slot if (game_state and slot_confirmed) else None,
123+
"recommended_action": recommended_action,
124+
"server_query_failed": True,
125+
}
126+
99127
async def get_save_setup_info(self, rom_id: int) -> dict[str, Any]:
100128
"""Get info needed for the first-sync setup wizard.
101129
102130
Fetches server saves, checks local files, determines which
103131
scenario (A-E) applies so the frontend can display the right UI.
132+
133+
``recommended_action`` carries the failure cause when the server-saves
134+
query raises: ``server_unreachable`` for a transport failure,
135+
``not_found`` when the server answered that it has no such ROM or
136+
device id. Both hold the wizard identically — only the copy and the
137+
frontend's reachability verdict differ (#1570).
104138
"""
105139
rom_id = int(rom_id)
106140

@@ -126,22 +160,36 @@ async def get_save_setup_info(self, rom_id: int) -> dict[str, Any]:
126160
lambda: self._romm_api.list_saves(rom_id, device_id=device_id),
127161
),
128162
)
163+
except RommNotFoundError as e:
164+
# The server ANSWERED — it has no such ROM, or no such device id
165+
# (#1560's shape: a RomM database wipe drops this device's
166+
# registration). Reporting that as "unreachable" sends the user to
167+
# check a connection that is plainly working, so it gets its own
168+
# recommendation. The wizard still HOLDS: a 404 leaves the server's
169+
# slot inventory just as unproven as an outage does, so
170+
# auto-confirming the default slot could still clobber real server
171+
# saves on the first post-confirmation sync.
172+
self._logger.warning(
173+
f"get_save_setup_info({rom_id}): server has no such rom or device: {e}",
174+
)
175+
return self._setup_query_failed(
176+
local_files=local_files,
177+
local_file_info=local_file_info,
178+
game_state=game_state,
179+
default_slot=default_slot,
180+
recommended_action="not_found",
181+
)
129182
except Exception as e:
130183
self._logger.warning(
131184
f"get_save_setup_info({rom_id}): failed to list server saves: {e}",
132185
)
133-
slot_confirmed = bool(game_state.slot_confirmed) if game_state else False
134-
active_slot = game_state.active_slot if (game_state and slot_confirmed) else None
135-
return {
136-
"has_local_saves": len(local_files) > 0,
137-
"local_files": local_file_info,
138-
"server_slots": [],
139-
"default_slot": default_slot,
140-
"slot_confirmed": slot_confirmed,
141-
"active_slot": active_slot,
142-
"recommended_action": "server_unreachable",
143-
"server_query_failed": True,
144-
}
186+
return self._setup_query_failed(
187+
local_files=local_files,
188+
local_file_info=local_file_info,
189+
game_state=game_state,
190+
default_slot=default_slot,
191+
recommended_action="server_unreachable",
192+
)
145193

146194
# Group server saves by slot
147195
slots_map: dict[str | None, list[dict[str, Any]]] = {}

src/components/SlotSetupWizard.test.tsx

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1116,6 +1116,47 @@ describe("SlotSetupWizard", () => {
11161116
expect(getRommConnectionState()).toBe("offline");
11171117
});
11181118

1119+
it("does NOT report offline when the load returns recommended_action=not_found", async () => {
1120+
// #1560's path: the 404 means the server ANSWERED. Reporting offline
1121+
// here blacked out the whole UI while RomM was working fine (#1570).
1122+
setRommConnectionState("checking");
1123+
vi.mocked(backend.getSaveSetupInfo).mockResolvedValue(
1124+
makeSetupInfo({ recommended_action: "not_found", server_query_failed: true }),
1125+
);
1126+
const result = makeSetupInfo({ recommended_action: "not_found", server_query_failed: true });
1127+
vi.mocked(backend.getSaveSetupInfo).mockResolvedValue(result);
1128+
render(<SlotSetupWizard {...defaultProps()} />);
1129+
await flushAsync();
1130+
1131+
// Post-load state: the store went CONNECTED (the server answered), and
1132+
// the result was still routed into the hold path rather than short-
1133+
// circuiting. The banner copy itself is covered in saveSetup.test.ts —
1134+
// this file stubs applyWizardInitialSetupResult.
1135+
expect(getRommConnectionState()).toBe("connected");
1136+
expect(vi.mocked(applyWizardInitialSetupResult)).toHaveBeenCalledWith(result, expect.anything());
1137+
});
1138+
1139+
it("leaves the reconnect gate disarmed after a not_found load", async () => {
1140+
// offlineHeldRef must stay false: there is no connection to wait for, so
1141+
// a later →connected edge must not trigger a re-fetch loop (#1570).
1142+
setRommConnectionState("checking");
1143+
vi.mocked(backend.getSaveSetupInfo).mockResolvedValue(
1144+
makeSetupInfo({ recommended_action: "not_found", server_query_failed: true }),
1145+
);
1146+
render(<SlotSetupWizard {...defaultProps({ romId: 9 })} />);
1147+
await flushAsync();
1148+
const callsAfterLoad = vi.mocked(backend.getSaveSetupInfo).mock.calls.length;
1149+
1150+
await act(async () => {
1151+
setRommConnectionState("offline");
1152+
reportServerReachable(true);
1153+
await Promise.resolve();
1154+
});
1155+
await flushAsync();
1156+
1157+
expect(vi.mocked(backend.getSaveSetupInfo).mock.calls.length).toBe(callsAfterLoad);
1158+
});
1159+
11191160
it("auto-reloads on reconnect after holding the offline error", async () => {
11201161
setRommConnectionState("offline");
11211162
render(<SlotSetupWizard {...defaultProps({ romId: 5 })} />);

src/components/SlotSetupWizard.tsx

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -518,6 +518,10 @@ function useSaveSetupInfo(romId: number, onComplete: () => void) {
518518
// definitive offline signal; any other resolved result proves the
519519
// server answered. A throw is a bridge/unknown error, not a verdict —
520520
// the catch leaves the store untouched.
521+
// Only an explicit unreachable verdict means offline. A `not_found`
522+
// result PROVES the server answered, so it reports reachable and
523+
// leaves the reconnect gate disarmed — the wizard still holds, it
524+
// just doesn't wait for a connection that was never lost (#1570).
521525
const reachable = result.recommended_action !== "server_unreachable";
522526
reportServerReachable(reachable);
523527
offlineHeldRef.current = !reachable;
@@ -576,6 +580,10 @@ function useSaveSetupInfo(romId: number, onComplete: () => void) {
576580
// Same conservative feed as the initial load (#1345): the manual
577581
// Retry re-probes reachability, so a server_unreachable result
578582
// re-arms offline and any other result reports the server back.
583+
// Only an explicit unreachable verdict means offline. A `not_found`
584+
// result PROVES the server answered, so it reports reachable and
585+
// leaves the reconnect gate disarmed — the wizard still holds, it
586+
// just doesn't wait for a connection that was never lost (#1570).
579587
const reachable = result.recommended_action !== "server_unreachable";
580588
reportServerReachable(reachable);
581589
offlineHeldRef.current = !reachable;

src/types/saves.ts

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -174,10 +174,12 @@ export interface SaveSetupInfo {
174174
// "server_unreachable" means the server-saves fetch failed — the wizard MUST
175175
// hold and offer a retry instead of treating the empty server_slots as
176176
// authoritative (auto-confirming default would clobber real server saves on
177-
// first sync). See backend `get_save_setup_info`.
178-
recommended_action: "auto_confirm_default" | "show_wizard" | "server_unreachable";
179-
// Mirrors recommended_action === "server_unreachable" — explicit flag for
180-
// call sites that route on the boolean rather than the enum.
177+
// first sync). "not_found" is the same hold for a different cause: the
178+
// server ANSWERED that it has no such ROM or device id, so the wizard must
179+
// not report the server offline (#1570). See backend `get_save_setup_info`.
180+
recommended_action: "auto_confirm_default" | "show_wizard" | "server_unreachable" | "not_found";
181+
// True whenever the server-saves query failed, whichever way — an explicit
182+
// flag for call sites that route on the boolean rather than the enum.
181183
server_query_failed?: boolean;
182184
}
183185

src/utils/saveSetup.test.ts

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@ import {
1212
wizardMigrationOutcomeToastBody,
1313
SERVER_UNREACHABLE_WIZARD_MESSAGE,
1414
SERVER_UNREACHABLE_TOAST_BODY,
15+
NOT_FOUND_WIZARD_MESSAGE,
16+
NOT_FOUND_TOAST_BODY,
1517
type LaunchGateSetupDeps,
1618
type SaveSetupOutcome,
1719
type WizardRetryDeps,
@@ -45,6 +47,25 @@ describe("resolveSaveSetupOutcome", () => {
4547
expect(resolveSaveSetupOutcome(info)).toEqual({ kind: "server_unreachable" });
4648
});
4749

50+
it("routes 'not_found' to its own outcome, NOT to auto-confirm", () => {
51+
// The empty server_slots is no more authoritative on a 404 than on an
52+
// outage, so this must not fall through to the auto-confirm branch (#1570).
53+
const info = makeInfo({ recommended_action: "not_found", server_query_failed: true });
54+
expect(resolveSaveSetupOutcome(info)).toEqual({ kind: "not_found" });
55+
});
56+
57+
it("routes 'not_found' even with local saves and an empty server list present", () => {
58+
// The exact shape that would otherwise auto-confirm under 'show_wizard'.
59+
const info = makeInfo({
60+
recommended_action: "not_found",
61+
has_local_saves: true,
62+
server_slots: [],
63+
default_slot: "default",
64+
});
65+
expect(resolveSaveSetupOutcome(info)).not.toEqual({ kind: "auto_confirm", slot: "default" });
66+
expect(resolveSaveSetupOutcome(info)).toEqual({ kind: "not_found" });
67+
});
68+
4869
it("routes 'auto_confirm_default' to the auto-confirm outcome with the default slot", () => {
4970
const info = makeInfo({ recommended_action: "auto_confirm_default", default_slot: "main" });
5071
expect(resolveSaveSetupOutcome(info)).toEqual({ kind: "auto_confirm", slot: "main" });
@@ -95,6 +116,17 @@ describe("applyLaunchGateSetupOutcome", () => {
95116
expect(deps.confirmSlotChoice).not.toHaveBeenCalled();
96117
});
97118

119+
it("aborts the launch on not_found too, with its own copy", async () => {
120+
// The launch must NOT proceed with save tracking unconfigured just
121+
// because the failure was a 404 rather than an outage (#1570).
122+
const deps = makeLaunchGateDeps();
123+
const result = await applyLaunchGateSetupOutcome({ kind: "not_found" }, deps);
124+
expect(result).toBe("abort");
125+
expect(deps.toast).toHaveBeenCalledWith(NOT_FOUND_TOAST_BODY);
126+
expect(deps.dispatchSavesTab).toHaveBeenCalledOnce();
127+
expect(deps.confirmSlotChoice).not.toHaveBeenCalled();
128+
});
129+
98130
it("calls confirmSlotChoice with the resolved slot and proceeds on auto_confirm", async () => {
99131
const deps = makeLaunchGateDeps();
100132
const outcome: SaveSetupOutcome = { kind: "auto_confirm", slot: "main" };
@@ -190,6 +222,27 @@ describe("applyWizardInitialSetupResult", () => {
190222
expect(deps.onComplete).not.toHaveBeenCalled();
191223
});
192224

225+
it("sets the not-found banner and bails, never auto-confirming", async () => {
226+
// Same hold as server_unreachable — the wizard must not configure a slot
227+
// against an unproven server view just because the cause was a 404 (#1570).
228+
const deps = makeWizardDeps();
229+
const result = makeInfo({ recommended_action: "not_found", server_query_failed: true });
230+
await applyWizardInitialSetupResult(result, deps);
231+
expect(deps.setError).toHaveBeenCalledWith(NOT_FOUND_WIZARD_MESSAGE);
232+
expect(deps.setConfirming).not.toHaveBeenCalled();
233+
expect(deps.confirmSlotChoice).not.toHaveBeenCalled();
234+
expect(deps.setInfo).not.toHaveBeenCalled();
235+
expect(deps.onComplete).not.toHaveBeenCalled();
236+
});
237+
238+
it("the not-found copy names what RomM could not find, not that saves are absent", () => {
239+
// The 404 can come from the DEVICE registration, so claiming the game has
240+
// no saves would swap one lie for a more specific one (#1570).
241+
expect(NOT_FOUND_WIZARD_MESSAGE).not.toMatch(/no saves|has no save|without saves/i);
242+
expect(NOT_FOUND_WIZARD_MESSAGE).not.toMatch(/unreachable|not reachable|offline/i);
243+
expect(NOT_FOUND_TOAST_BODY).not.toMatch(/unreachable|not reachable|offline/i);
244+
});
245+
193246
it("auto-confirms and calls onComplete on 'auto_confirm_default'", async () => {
194247
const deps = makeWizardDeps();
195248
const result = makeInfo({ recommended_action: "auto_confirm_default", default_slot: "alpha" });
@@ -292,6 +345,15 @@ describe("applyWizardRetrySetupResult", () => {
292345
expect(deps.setInfo).not.toHaveBeenCalled();
293346
});
294347

348+
it("sets the not-found banner and clears loading on 'not_found'", () => {
349+
const deps = makeRetryDeps();
350+
const result = makeInfo({ recommended_action: "not_found" });
351+
applyWizardRetrySetupResult(result, deps);
352+
expect(deps.setError).toHaveBeenCalledWith(NOT_FOUND_WIZARD_MESSAGE);
353+
expect(deps.setLoading).toHaveBeenCalledWith(false);
354+
expect(deps.setInfo).not.toHaveBeenCalled();
355+
});
356+
295357
it("sets the fetched info and clears loading on a non-unreachable result", () => {
296358
const deps = makeRetryDeps();
297359
const result = makeInfo({ recommended_action: "show_wizard" });

0 commit comments

Comments
 (0)