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.
|
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. |
|
I'm wrapping up some hashing stuff now, since it's basically just moving over the process from Argosy to be handled by the server. I think this will be more reliable than having the clients deal with it and ensures the strategy is done correctly and consistently. The main concern is with hashing compressed content since just hashing the container would always be different, even when compressing the same contents. This solution hashes the inner content and combines that into a single MD5 for comparison and storage. I'll push the commit for review after a run some test files through it. |
- Add device registration and save synchronization - Implement slot-based save organization with datetime tagging - Add conflict detection for multi-device sync scenarios - Add content hash computation for save deduplication - Support ZIP inner-file hashing for consistent deduplication - Add confirm_download endpoint for sync state management - Add overwrite parameter to bypass conflict checks
tmgast
force-pushed
the
feature/device-registration-save-sync
branch
from
b06e897 to
a236123
Compare
gantoine
left a comment
gantoine
left a comment
There was a problem hiding this comment.
damn this is noice *chef kiss*
Add fingerprint-based detection for duplicate device registration with configurable behavior via new body params: - allow_existing: return existing device if fingerprint matches - allow_duplicate: skip fingerprint check, always create new device - reset_syncs: clear tracked saves when reclaiming existing device Fingerprint matching uses mac_address (primary) or hostname+platform (fallback). Returns 409 Conflict with device_id when duplicate detected without flags, 200 OK for existing device, 201 Created for new.
Replace individual Body() parameters with DeviceCreatePayload and DeviceUpdatePayload Pydantic models. This simplifies the function signatures and leverages model_dump(exclude_unset=True) for cleaner update handling.
Add order_by and order_dir parameters to get_saves() for flexible sorting. Supports "updated_at" and "created_at" fields with "asc" or "desc" direction (default: desc). Enables ascending order for pruning scenarios.
Move compute_file_hash, compute_zip_hash, and compute_content_hash from scan_handler.py to filesystem/assets_handler.py as standalone module-level functions. This follows the existing pattern for utility functions in filesystem handlers.
Move UTC datetime normalization to a dedicated utils module for reusability across the codebase.
Add tests for duplicate device registration scenarios (409 conflict, allow_existing, allow_duplicate, reset_syncs). Fix compute_file_hash function references after relocation to assets_handler.
Add model_validator to SaveSchema that safely handles SQLAlchemy lazy relationships by checking inspect(obj).unloaded before attribute access. This allows direct use of model_validate(orm_obj) instead of manually building a dict and excluding device_syncs.
…stration Change allow_existing default to True so duplicate fingerprint matches return the existing device (200) instead of 409 Conflict. Add model validator to force allow_existing=False when allow_duplicate is set.
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=trueslotparameter for categorizing saves into named slots[YYYY-MM-DD_HH-MM-SS]when slot providedautocleanup+autocleanup_limitto prune old slot saves/saves/summaryNote
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 [2025-01-18_14-00-00].sav", "file_name_no_tags": "pokemon_emerald", "file_name_no_ext": "pokemon_emerald [2025-01-18_14-00-00]", "file_extension": "sav", "file_path": "/saves/gba/100", "file_size_bytes": 131072, "full_path": "/saves/gba/100/pokemon_emerald [2025-01-18_14-00-00].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", "slot": "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 } ] }SaveSummarySchema Sample
{ "total_count": 6, "slots": [ { "slot": null, "count": 3, "latest": { "...SaveSchema..." } }, { "slot": "Main Playthrough", "count": 2, "latest": { "...SaveSchema..." } }, { "slot": "Nuzlocke Run", "count": 1, "latest": { "...SaveSchema..." } } ] }New API Endpoints
Devices
POST
/api/devicesGET
/api/devicesGET
/api/devices/{device_id}PUT
/api/devices/{device_id}DELETE
/api/devices/{device_id}Saves
GET
/api/saves/summary?rom_id={rom_id}Save Sync Operations
POST
/api/saves/{id}/trackPOST
/api/saves/{id}/untrackPOST
/api/saves/{id}/downloadedUpdated API Endpoints
POST
/api/savesdevice_idquery param to activate sync featuresslotquery param for slot categorization (optional)autocleanupquery param (default: false)autocleanup_limitquery param (default: 10)slotprovided, filename auto-tagged with[YYYY-MM-DD_HH-MM-SS]GET
/api/savesdevice_idquery paramslotquery param to filter by slotdevice_syncs[]with sync status whendevice_idprovidedGET
/api/saves/{id}device_idquery paramdevice_syncs[]with sync status whendevice_idprovidedGET
/api/saves/{id}/contentdevice_idquery paramoptimisticquery param (default: true)optimistic=true, updates sync record on downloadNew Scopes
Database Changes
0068_save_sync.pyRecommended Usage
Device Registration (First Launch)
User login -> register new device -> store device ID for save sync API calls
Discover Save Slots (Game Selected)
Fetch save summary to populate slot picker UI
GET
/api/saves/summary?rom_id={rom_id}Start Game Flow
If loading specific slot:
GET
/api/saves?rom_id={rom_id}&slot={slot_name}&device_id={device_id}device_syncs[0].is_currenton latesttrue-> 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 automaticallyIf loading latest across all slots:
Use summary's
latestfrom each slot to find most recent globallyEnd Game Flow
Check sync status -> upload new save file
GET
/api/saves/{id}?device_id={device_id}device_syncs[0].is_currentbefore uploadingtrue-> safe to uploadfalse-> warn user: server has newer savePOST
/api/saves?rom_id={rom_id}&device_id={device_id}&slot={slot_name}save [2025-01-18_14-30-00].sav)With autocleanup (recommended for autosave slots):
POST
/api/saves?rom_id={rom_id}&device_id={device_id}&slot=autosave&autocleanup=true&autocleanup_limit=10Note
To force overwrite on conflict:
POST
/api/saves?rom_id={rom_id}&device_id={device_id}&overwrite=trueSlot Feature Details
Datetime Tagging
When uploading with a
slotparameter:filename [YYYY-MM-DD_HH-MM-SS].extslotparameter are not tagged (backward compatible)Autocleanup
When uploading with
autocleanup=true:slotis also providedautocleanup_limit(default: 10)Summary Endpoint
GET
/api/saves/summary?rom_id={rom_id}returns:total_count: Total saves for this ROMslots[]: Array of slot summaries, each with:slot: Slot name (ornullfor unslotted saves)count: Number of saves in this slotlatest: Full SaveSchema of most recent save in slotChecklist