feat: improve device backup and restore#1124
Merged
Merged
Conversation
Adds a full-device backup system: portable local ZIP backups with transactional restore and Core restart, plus cloud backups through a linked Zaparoo Online account (device link flow, scheduled uploads, snapshot listing and restore, Warp availability gating). - backup service with coordinator, collector, remote client, and restore gate; scheduler for daily/weekly/manual cloud backups - API: settings.backup.* methods, auth.link flow, status with linked device identity; availability refresh runs in the background so status never blocks on the cloud API - TUI: Online page (account status, Warp state, feature discovery) and Backup page with Local/Cloud sections; section headers and delayed loading indicator for async page builds - MiSTer profile data collection for backup archives
…loss Backup work now follows the same media pause policy as indexing: a shared pauser throttles collection, hashing, packing, and uploads during most games, pauses them fully during storage-sensitive CD cores, and resumes when the game stops. Applies to scheduled and manual runs (API and TUI). The scheduler skips starting a run while fully paused so it does not hold the coordinator lease through a game session. A run interrupted by power loss left "running" in the status file, which delayed the next attempt by the full daily/weekly interval. Interrupted runs are now marked failed at startup so the 1-hour failure retry applies; content-addressed dedup means the retry skips objects already uploaded. The cloud upload action no longer hard-blocks on the cached Warp availability: the run's fresh server-side check is authoritative, so a just-activated subscription works immediately. Cached "unavailable" and "unknown" both use the 5-minute retry TTL; only "available" is cached for an hour. Adds a backup.state notification reporting a running operation's pause/throttle transitions so clients can display it.
Restructure Settings > Online > Cloud backup > View backups into a source-device list, per-device snapshot list, and a detailed restore confirmation. Account backups from any linked device (and previously linked, now-unlinked devices) are grouped by their source device: this device first, other linked devices next, unlinked last, each showing backup count and latest timestamp. Selecting a source lists its snapshots newest first with type and size; selecting a compatible snapshot confirms exactly what restore does — copies the immutable snapshot onto this device, leaves the source backup and device unchanged, keeps this device's own Online identity, and takes a local safety backup before restarting. Replaces the flat []map[string]any remote list with typed DTOs decoding the camelCase settings.backup.remote.list response, including the new sourceDevice metadata. Restore semantics are unchanged (existing settings.backup.remote.restore); no restore grants or replace-vs-copy modes. The link-success message now points at Cloud backup > View backups.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds portable and remote backup services, transactional restore recovery, authentication linking, scheduling, API methods and notifications, runtime coordination, and TUI settings workflows. It also adds configuration, database-transfer, platform-provider, dialog, and concurrency support with broad tests. ChangesBackup platform and persistence
API and runtime
TUI
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
pkg/config/configlaunchers_test.go (1)
774-785: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse afero for filesystem setup in both permission tests.
pkg/config/configlaunchers_test.go#L774-L785: use an afero OS filesystem for directory creation, file writing, andInstance.fs; keep direct chmod only for permission manipulation.pkg/config/configmappings_test.go#L38-L49: apply the same afero-backed setup and loading pattern.As per coding guidelines,
**/*.go: Use afero for filesystem operations in testable code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/config/configlaunchers_test.go` around lines 774 - 785, Update the permission-test setup in pkg/config/configlaunchers_test.go:774-785 to create the afero OS filesystem, use it for directory creation and file writing, and assign it to Instance.fs; retain direct os.Chmod only for permission manipulation. Apply the same afero-backed setup and loading pattern in pkg/config/configmappings_test.go:38-49.Source: Coding guidelines
pkg/ui/tui/settings_test.go (1)
1408-1433: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding coverage that selects a specific source group (with two or more groups present) and asserts the correct device's snapshots open. This is the one path that would catch the loop-variable capture behavior flagged in
pkg/ui/tui/settings.go(Line 1256).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ui/tui/settings_test.go` around lines 1408 - 1433, Extend TestRemoteBackupListPage_GroupsSources_Integration to select a specific source group while multiple groups are displayed, then assert that opening it shows only that device’s snapshots. Exercise the group-selection callback created by buildRemoteBackupListPage so the test detects loop-variable capture and verifies the selected group maps to the correct source.pkg/service/backup/coordinator/coordinator_test.go (1)
31-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
CancelRestore,RemoteUnlinked/SetRemoteUnlinked, andActive.These are new public methods on the coordinator with no tests here (e.g., cancelling only restore/recovery kinds while leaving other active leases running, or the read/write toggle for
remoteUnlinked).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/service/backup/coordinator/coordinator_test.go` around lines 31 - 108, Add focused tests for the coordinator’s CancelRestore, RemoteUnlinked, SetRemoteUnlinked, and Active methods. Verify CancelRestore cancels only restore/recovery operation leases while unrelated active leases continue running, and verify SetRemoteUnlinked updates the value returned by RemoteUnlinked. Also cover Active’s read/write state across relevant lease acquisition and release transitions.Source: Coding guidelines
pkg/service/profiles/dataswap.go (1)
307-311: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRestore-gate error is misreported as a data-swap failure.
When a restore completes successfully,
AcquireRestoreAccess()returnsErrRestoreRestartRequired. That error then falls through to thedefaultbranch below (Lines 334-341), emitting aProfilesDataFailednotification with reason"backup restore restart is pending"even though no swap was attempted and nothing was corrupted. Consider handling the restore-gate errors explicitly (skip the failure notification / log at debug) so clients don't surface a spurious failure right before the Core restart.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/service/profiles/dataswap.go` around lines 307 - 311, Update the error handling around AcquireRestoreAccess and the subsequent swap-result switch to recognize ErrRestoreRestartRequired as an expected restore-gate outcome. Skip the ProfilesDataFailed notification and use debug-level logging (or equivalent existing handling) for this case, while preserving failure reporting for actual ApplyProfile errors and other unexpected errors.pkg/api/methods/auth_link.go (1)
374-380: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
authLinkTerminalErrortype near the top of the file.This type is declared after the functions that use it. As per coding guidelines, "Define Go types and consts near the top of the file, before functions and methods."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/api/methods/auth_link.go` around lines 374 - 380, Move the authLinkTerminalError type and its Error method from the bottom of the file to the top-level declarations section, before the functions and methods that reference it. Preserve its fields and error behavior unchanged.Source: Coding guidelines
pkg/api/server_restore_gate_test.go (1)
72-87: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen this test to actually exercise gate contention.
TestHandleRequestAllowsRestoreMethodToOwnExclusiveGatecallshandleRequestforMethodSettingsBackupRestorewithout any restore gate being held. SinceTryAcquireRestoreAccesswould trivially succeed here even without theapiMethodManagesRestoreAccessspecial-case (there's no contention to reject), this test can't distinguish correct behavior from a regression that removes the skip-list entry for this method. Consider first callingst.BeginRestoreGate()(as the sibling test does) and then verifying the restore method can still run/complete while the shared gate is held.♻️ Suggested strengthening
func TestHandleRequestAllowsRestoreMethodToOwnExclusiveGate(t *testing.T) { t.Parallel() st, _ := state.NewState(nil, "test-boot") defer st.StopService() methodMap := &MethodMap{} require.NoError(t, methodMap.AddMethod( models.MethodSettingsBackupRestore, func(requests.RequestEnv) (any, error) { return "restore", nil }, )) + // Hold the shared restore gate to prove the restore method bypasses it + // rather than merely succeeding due to no contention. + finishRestore, err := st.BeginRestoreGate() + require.NoError(t, err) + defer finishRestore(false) result, rpcErr := handleRequest(methodMap, requests.RequestEnv{ Context: context.Background(), State: st, }, models.RequestObject{JSONRPC: "2.0", Method: models.MethodSettingsBackupRestore}) require.Nil(t, rpcErr) assert.Equal(t, "restore", result) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/api/server_restore_gate_test.go` around lines 72 - 87, Strengthen TestHandleRequestAllowsRestoreMethodToOwnExclusiveGate by calling st.BeginRestoreGate() before handleRequest, matching the sibling contention test, and defer or otherwise ensure the gate is released. Keep the existing assertions verifying the restore method completes successfully while the shared restore gate is already held.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/config/tui_test.go`:
- Around line 63-64: Update the test setup around the EncryptionPrompted and
other prompted field mappings to use separate boolean variables with distinct
true/false values. Assert each resulting field independently so the test
verifies both source-to-field mappings and catches wiring either field to the
wrong source.
In `@pkg/service/service.go`:
- Around line 63-83: Bound the second shutdown wait in waitForBackupShutdown
with a hard outer deadline instead of using context.Background(), while
preserving the existing lease-release grace period. When that deadline expires,
log or alert the timeout and return an error so StartResult.Stop cannot block
indefinitely; update related shutdown tests, including
TestWaitForBackupShutdownDoesNotTearDownBeforeLeaseRelease, to verify both lease
preservation and bounded termination.
---
Nitpick comments:
In `@pkg/api/methods/auth_link.go`:
- Around line 374-380: Move the authLinkTerminalError type and its Error method
from the bottom of the file to the top-level declarations section, before the
functions and methods that reference it. Preserve its fields and error behavior
unchanged.
In `@pkg/api/server_restore_gate_test.go`:
- Around line 72-87: Strengthen
TestHandleRequestAllowsRestoreMethodToOwnExclusiveGate by calling
st.BeginRestoreGate() before handleRequest, matching the sibling contention
test, and defer or otherwise ensure the gate is released. Keep the existing
assertions verifying the restore method completes successfully while the shared
restore gate is already held.
In `@pkg/config/configlaunchers_test.go`:
- Around line 774-785: Update the permission-test setup in
pkg/config/configlaunchers_test.go:774-785 to create the afero OS filesystem,
use it for directory creation and file writing, and assign it to Instance.fs;
retain direct os.Chmod only for permission manipulation. Apply the same
afero-backed setup and loading pattern in
pkg/config/configmappings_test.go:38-49.
In `@pkg/service/backup/coordinator/coordinator_test.go`:
- Around line 31-108: Add focused tests for the coordinator’s CancelRestore,
RemoteUnlinked, SetRemoteUnlinked, and Active methods. Verify CancelRestore
cancels only restore/recovery operation leases while unrelated active leases
continue running, and verify SetRemoteUnlinked updates the value returned by
RemoteUnlinked. Also cover Active’s read/write state across relevant lease
acquisition and release transitions.
In `@pkg/service/profiles/dataswap.go`:
- Around line 307-311: Update the error handling around AcquireRestoreAccess and
the subsequent swap-result switch to recognize ErrRestoreRestartRequired as an
expected restore-gate outcome. Skip the ProfilesDataFailed notification and use
debug-level logging (or equivalent existing handling) for this case, while
preserving failure reporting for actual ApplyProfile errors and other unexpected
errors.
In `@pkg/ui/tui/settings_test.go`:
- Around line 1408-1433: Extend
TestRemoteBackupListPage_GroupsSources_Integration to select a specific source
group while multiple groups are displayed, then assert that opening it shows
only that device’s snapshots. Exercise the group-selection callback created by
buildRemoteBackupListPage so the test detects loop-variable capture and verifies
the selected group maps to the correct source.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 551d0175-ded1-47be-9ab1-3fa408b7949d
📒 Files selected for processing (97)
docs/api/methods.mddocs/api/notifications.mdpkg/api/client/client.gopkg/api/client/client_test.gopkg/api/methods/auth.gopkg/api/methods/auth_link.gopkg/api/methods/auth_link_test.gopkg/api/methods/auth_test.gopkg/api/methods/backup.gopkg/api/methods/backup_test.gopkg/api/methods/permissions.gopkg/api/methods/settings.gopkg/api/methods/settings_test.gopkg/api/models/models.gopkg/api/models/params.gopkg/api/models/requests/requests.gopkg/api/models/responses.gopkg/api/notifications/notifications.gopkg/api/request_priority.gopkg/api/request_priority_test.gopkg/api/server.gopkg/api/server_post_test.gopkg/api/server_restore_gate_test.gopkg/api/server_startup_test.gopkg/api/server_ws_e2e_test.gopkg/api/ws_dispatcher.gopkg/api/ws_dispatcher_test.gopkg/cli/cli.gopkg/config/auth.gopkg/config/auth_test.gopkg/config/config.gopkg/config/configbackup.gopkg/config/configbackup_test.gopkg/config/configlaunchers.gopkg/config/configlaunchers_test.gopkg/config/configmappings.gopkg/config/configmappings_test.gopkg/config/configservice_test.gopkg/config/tui.gopkg/config/tui_test.gopkg/database/database.gopkg/database/userdb/backup.gopkg/database/userdb/backup_test.gopkg/database/userdb/clients.gopkg/database/userdb/sql.gopkg/platforms/mister/backup_test.gopkg/platforms/mister/platform.gopkg/platforms/mister/profiledata_backup.gopkg/platforms/mister/profiledata_backup_test.gopkg/platforms/mistex/platform.gopkg/platforms/platforms.gopkg/service/backup/backup.gopkg/service/backup/backup_test.gopkg/service/backup/collector.gopkg/service/backup/coordinator.gopkg/service/backup/coordinator/coordinator.gopkg/service/backup/coordinator/coordinator_test.gopkg/service/backup/mister_integration_test.gopkg/service/backup/remote.gopkg/service/backup/restore.gopkg/service/backup_scheduler.gopkg/service/backup_scheduler_test.gopkg/service/inbox/inbox.gopkg/service/indexpause.gopkg/service/indexpause_test.gopkg/service/profiles/dataswap.gopkg/service/profiles/dataswap_test.gopkg/service/profiles/profiles.gopkg/service/profiles/profiles_test.gopkg/service/queues.gopkg/service/service.gopkg/service/service_test.gopkg/service/state/state.gopkg/service/state/state_media_test.gopkg/service/state/state_test.gopkg/testing/helpers/db_mocks.gopkg/ui/tui/dialog.gopkg/ui/tui/dialog_test.gopkg/ui/tui/encryption_prompt.gopkg/ui/tui/encryption_prompt_test.gopkg/ui/tui/exportlog.gopkg/ui/tui/main.gopkg/ui/tui/mock_api_client_test.gopkg/ui/tui/online.gopkg/ui/tui/online_test.gopkg/ui/tui/profiles.gopkg/ui/tui/settings.gopkg/ui/tui/settings_components.gopkg/ui/tui/settings_components_test.gopkg/ui/tui/settings_service.gopkg/ui/tui/settings_test.gopkg/ui/tui/settings_tui.gopkg/ui/tui/utils.gopkg/ui/tui/utils_test.gopkg/zapscript/commands.gopkg/zapscript/launch.gopkg/zapscript/launch_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/service/service_test.go`:
- Around line 94-105: Bound the wait in the select around waitForBackupShutdown
so the test fails if completion exceeds the intended 50ms hard deadline. Replace
the one-second time.After case with a shorter upper timeout that includes
scheduling slack, such as 250ms, while preserving the existing error assertions
and timeout failure behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 21ce26af-4c31-464b-bf9f-99856d777079
📒 Files selected for processing (17)
pkg/api/methods/auth_link.gopkg/api/methods/backup_test.gopkg/api/server_restore_gate_test.gopkg/config/configlaunchers_test.gopkg/config/configmappings_test.gopkg/config/tui_test.gopkg/platforms/mister/profiledata_backup_test.gopkg/platforms/mister/profiledata_test.gopkg/service/backup/backup_test.gopkg/service/backup/coordinator/coordinator_test.gopkg/service/backup_scheduler_test.gopkg/service/profiles/dataswap.gopkg/service/profiles/dataswap_test.gopkg/service/service.gopkg/service/service_test.gopkg/ui/tui/settings_api_test.gopkg/ui/tui/settings_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- pkg/config/tui_test.go
- pkg/api/server_restore_gate_test.go
- pkg/service/backup_scheduler_test.go
- pkg/config/configmappings_test.go
- pkg/service/service.go
- pkg/ui/tui/settings_test.go
- pkg/config/configlaunchers_test.go
- pkg/api/methods/auth_link.go
- pkg/service/backup/backup_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Expands the existing UserDB-only backup into a full device backup and
restore, so a user can capture their setup and bring it back after a
reinstall, SD swap, or hardware move.
What's included
mappings, saves, and savestates.
rolling back to the previous state on failure — and Core restarts to apply.
Storage
and restore any compatible snapshot from the same account. These accounts
can schedule automatic backups (daily or weekly); scheduled runs throttle
while a game is running, pause fully during CD-based cores, and retry after
a power loss.
API
settings.backup.*methods and abackup.statenotification (seedocs/api).
Summary by CodeRabbit
auth.link.statusandbackup.statenotifications.