Add device-based save synchronization #2917
Conversation
Implement device registration and save sync tracking to enable multi-device save management with conflict detection. - Device CRUD endpoints (POST/GET/PUT/DELETE /api/devices) - Save sync state tracking per device - Conflict detection on upload (409 when device has stale sync) - Download sync tracking (optimistic and confirmed modes) - Track/untrack saves per device - DEVICES_READ/WRITE scopes for authorization
Summary of ChangesHello @tmgast, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request lays the groundwork for robust multi-device save management. It introduces new data models and API endpoints to enable devices to register, track save synchronization status, and handle potential conflicts when uploading game saves. The changes are foundational, providing the necessary backend infrastructure for future client-side integration and advanced sync modes, ultimately enhancing the user experience for players across various platforms. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is a comprehensive pull request that introduces a foundational device-based save synchronization feature. The changes are well-structured, including new models, API endpoints, and an impressive suite of tests covering various scenarios like conflict detection and user isolation. My review focuses on a few key areas to enhance the robustness and security of the implementation. I've identified a critical issue in the database migration concerning an enum mismatch that could prevent the feature from working correctly. Additionally, there are some high-severity security concerns with incorrect scope definitions on new endpoints and a potential for subtle bugs in datetime handling. I've also included some medium-severity suggestions to improve code quality and maintainability. Overall, this is a strong contribution, and addressing these points will make it even better.
| def update_device( | ||
| request: Request, | ||
| device_id: str, | ||
| name: str | None = Body(None, embed=True), | ||
| platform: str | None = Body(None, embed=True), | ||
| client: str | None = Body(None, embed=True), | ||
| client_version: str | None = Body(None, embed=True), | ||
| ip_address: str | None = Body(None, embed=True), | ||
| mac_address: str | None = Body(None, embed=True), | ||
| hostname: str | None = Body(None, embed=True), | ||
| sync_enabled: bool | None = Body(None, embed=True), | ||
| ) -> DeviceSchema: |
There was a problem hiding this comment.
The update_device function accepts many optional parameters from the request body individually. This makes the function signature long and the logic to build update_data verbose.
Consider defining a Pydantic model for the update payload, for example:
from pydantic import BaseModel
class DeviceUpdatePayload(BaseModel):
name: str | None = None
platform: str | None = None
client: str | None = None
client_version: str | None = None
ip_address: str | None = None
mac_address: str | None = None
hostname: str | None = None
sync_enabled: bool | None = NoneThen, you can simplify the endpoint to:
@protected_route(router.put, "/{device_id}", [Scope.DEVICES_WRITE])
def update_device(
request: Request,
device_id: str,
payload: DeviceUpdatePayload,
) -> DeviceSchema:
# ... fetch device ...
update_data = payload.model_dump(exclude_unset=True)
if update_data:
# ... update logic ...
# ...This approach is cleaner, more maintainable, and leverages FastAPI's features more effectively. The same principle can be applied to the register_device endpoint.
| save_data = { | ||
| key: getattr(save, key) | ||
| for key in SaveSchema.model_fields | ||
| if key != "device_syncs" and hasattr(save, key) | ||
| } | ||
| save_data["device_syncs"] = device_syncs | ||
| return SaveSchema.model_validate(save_data) |
There was a problem hiding this comment.
The manual construction of save_data can be simplified. Since SaveSchema has from_attributes = True, you can directly validate the save ORM object and then assign the device_syncs list. This is more concise and less error-prone.
save_schema = SaveSchema.model_validate(save)
save_schema.device_syncs = device_syncs
return save_schema
|
I have been tinkering with a naive save sync in Grout for about a month now and look forward to implementing what you've shared here. Right now I just use timestamps but this scheme will offer much more control. I think this would also be the right time to take a look at adding the concept of "playthroughs" (name WIP). Right now, saves are essentially just a big bucket tied to game/user. A little extra organization will go a long way and I think will help make this syncing system even more robust. I just saw another user mentioned it a few months ago on the Sync RFC: #2199 (comment) Instead of registering saves we'd register playthroughs. Playthroughs could store a rolling list of saves (configurable maybe with a default of 5 or so) and will always return the metadata for the most recent save file associated with it. This would allow for the ability to rollback to be added in the future. I realize while this is technically achievable already with the single bucket of saves but I think being more explicit will prevent trouble down the road. Also I think there needs to be a sync audit log from the start (even if it isn't exposed anywhere user facing to begin). Just my two cents and happy to help where needed! |
|
It's already loosely possible to create playthrough with this, but I think it could be solidified in this spec as well. Including the history in RomM would take some of the burden off of the client for storage (though I still think it's a good idea to have a local mirror of the history for offline rollback). I also like the idea of auditing changes with a log that could double as the rollback state selection list. The hard part is updating the UI so the historic saves aren't all dumped into the UI. Seems like I'll probably have to address that to move forward with the improvements. |
No directly related to that conversion, but given how limited some of the targeted devices can be I think it's useful to shift the burden onto the server as much as possible. We can build modules in isolation for sync without affecting the wider system. |
|
Would we be better off overhauling current saves or, my preference, break synced playthroughs off to their own thing so we aren't muddying up the existing UI and model structures for save files? |
I think that's a conversation we need to include @zurdi15 in. IMO the current save system isn't very practical, since it requires manually uploading or interfacing with the API, and the proposal would handle things like dropping/syncing folders and ingesting those saves. All that to say, a single system for handling saves would be ideal from a user POV and simpler in the code base. I haven't looked as the PR closely yet so I'll comment further when that's done. |
|
Appreciate the input. I'd definitely like to get his opinion as well. I'm happy to put in the effort to make it happen. I'm going to work on a spec built on top of this branch over the next few days to see what would work best. I'll keep lower spec devices in mind too... |
|
I'm aligned with what Arcane is exposing here. A single saves/states system it's the ideal way of feeling it really as a central point where all your saves and satetes are not only managed but also that you can use any of them, anywhere. The UI side should remain the same in terms of merging the new sync work (or just rework what we already have and add the sync part). The user should feel everything is the same and not to have two different systems for different purposes. I also agree that right now using the API to upload/download saves is not too practical, but the details about that are way too long to be discussing it here now in a comment, we should use the RFC for that |
|
@tmgast i've got time this weekend to review this PR, anything you want to change before i dive in? |
|
I wanna add save revisions/history, but I have been too busy with the app to revisit this. |
|
Maybe that happens in another PR and we limit this one to what you've built so far? Would make it easier to review on our end. |
|
I'm good with that... though I'm debating my current implementation of naming here intended for save slots. Let me take another look over it today and make a few adjustments. |
4 participants
Description
Adds device-based save synchronization to enable multi-device save management. Devices (handhelds, PCs, etc.) can register with the server and track which saves they've synced, enabling conflict detection when a device tries to upload stale data.
Features
overwrite=trueNote
This is a foundational PR to set up an initial structure and API implementation for devices and save syncing in preparation for client application usage and eventual alternative sync modes and use-cases.
Known Gaps
New Models
Device
idstr(PK)user_idint(FK)namestr?platformstr?clientstr?client_versionstr?ip_addressstr?mac_addressstr?hostnamestr?sync_modeSyncModeapi,file_transfer,push_pullsync_enabledboollast_seendatetime?DeviceSaveSync
device_idstr(PK, FK)devices.idsave_idint(PK, FK)saves.idlast_synced_atdatetimeis_untrackedboolis_currentboolDeviceSchema Sample
{ "id": "abc-123-uuid", "user_id": 1, "name": "Steam Deck", "platform": "linux", "client": "RetroArch", "client_version": "1.17.0", "ip_address": "192.168.1.100", "mac_address": "AA:BB:CC:DD:EE:FF", "hostname": "steamdeck", "sync_mode": "api", "sync_enabled": true, "last_seen": "2025-01-18T14:30:00Z", "created_at": "2025-01-15T10:00:00Z", "updated_at": "2025-01-18T14:30:00Z" }SaveSchema Sample
{ "id": 42, "rom_id": 100, "user_id": 1, "file_name": "pokemon_emerald.sav", "file_name_no_tags": "pokemon_emerald", "file_name_no_ext": "pokemon_emerald", "file_extension": "sav", "file_path": "/saves/gba/100", "file_size_bytes": 131072, "full_path": "/saves/gba/100/pokemon_emerald.sav", "download_path": "/api/saves/42/content", "missing_from_fs": false, "created_at": "2025-01-10T08:00:00Z", "updated_at": "2025-01-18T14:00:00Z", "emulator": "mgba", "save_name": "Main Playthrough", "screenshot": null, "device_syncs": [ { "device_id": "abc-123-uuid", "device_name": "Steam Deck", "last_synced_at": "2025-01-17T10:00:00Z", "is_untracked": false, "is_current": false } ] }New API Endpoints
Devices
POST
/api/devicesGET
/api/devicesGET
/api/devices/{device_id}PUT
/api/devices/{device_id}DELETE
/api/devices/{device_id}Save Sync Operations
POST
/api/saves/{id}/trackPOST
/api/saves/{id}/untrackPOST
/api/saves/{id}/downloadedUpdated API Endpoints
POST
/api/savesGET
/api/savesGET
/api/saves/{id}GET
/api/saves/{id}/contentNew Scopes
Database Changes
Recommended Usage
Device Registration (First Launch)
User login -> register new device -> store device ID for save sync API calls
Start Game Flow
check sync status for current save file -> download if newer -> start game
GET /api/saves?rom_id={rom_id}&device_id={device_id}
device_syncs[0].is_currenttrue-> local save is current, safe to playfalse-> download latest before playingGET
/api/saves/{id}/content?device_id={device_id}optimistic=true(default), sync record updates automaticallyEnd Game Flow
check sync status for current save file -> upload new save file
GET
/api/saves/{id}?device_id={device_id}device_syncs[0].is_currentbefore uploadingPOST
/api/saves?rom_id={rom_id}&device_id={device_id}409 Conflict: prompt user to keep server / overwrite / cancelNote
To force overwrite:
POST
/api/saves?rom_id={rom_id}&device_id={device_id}&overwrite=trueChecklist