fix: skip spurious conflict when cache wiped but local matches cloud#1228
Conversation
destructive db migration wipes file_change_lists cache, making every steam game trigger conflict on first launch post-update. check if local state is byte-identical to remote manifest (by filename + SHA) before declaring conflict; if so, rehydrate cache and report UpToDate silently. also populates real timestamps on the genuine-divergence path (was showing epoch). test: dbCleared_localMatchesRemote_rehydratesSilently_noConflict
📝 WalkthroughWalkthroughThe sync logic now detects when the local cache is absent but local save files exist, builds case-normalized filepath maps for both local and remote files, compares their SHA hashes byte-for-byte, and silently rehydrates the cache database if they match exactly, avoiding unnecessary conflict marking. Changes
Sequence Diagram(s)sequenceDiagram
participant Local as Local Files
participant Cache as Cache/Database
participant Sync as Sync Engine
participant Remote as Remote Cloud
Sync->>Cache: Check if cache exists
alt Cache missing/empty
Sync->>Local: Read local save files
Sync->>Remote: Fetch remote manifest
Sync->>Sync: Build case-normalized maps<br/>(path → SHA hash)
Sync->>Sync: Compare local vs remote<br/>byte-for-byte
alt Files match exactly
Sync->>Cache: Silently rehydrate cache<br/>(insert files + change number)
Sync->>Sync: Set result: UpToDate<br/>Mark filesManaged: true
else Files differ
Sync->>Sync: Mark hasLocalChanges<br/>with conflict version
Sync->>Sync: Populate timestamps
Sync->>Sync: Trigger conflict resolution
end
else Cache exists
Sync->>Sync: Use normal sync flow
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
Conflicts Logic LGTM. But as the save's timestamp was fixed recently by #1199, I can imagine users who using 0.9.0 may still trigger conflict dialog. Let's say if you first run GN on 0.9.0, install a game and sync the saves once by using After that you rebase onto this PR changes, will there be any conflict triggered? |
|
@joshuatam I don't think so, the changes here are keyed on path and SHA. A timestamp comparison wasn't reliable enough. If you want to test it to double-check, though, that'd be good. |
| val localByPath = allLocalUserFiles.associate { | ||
| it.getAbsPath(prefixToPath).toString().lowercase() to it.sha | ||
| } | ||
| val remoteByPath = appFileListChange.files.associate { | ||
| getFullFilePath(it, appFileListChange).toString().lowercase() to it.shaFile | ||
| } | ||
| val localMatchesRemote = localByPath.keys == remoteByPath.keys && | ||
| localByPath.all { (path, sha) -> | ||
| sha.contentEquals(remoteByPath[path]) | ||
| } | ||
|
|
||
| if (localMatchesRemote) { | ||
| Timber.i("Cache absent but local matches remote — rehydrating cache silently") | ||
| with(steamInstance) { | ||
| db.withTransaction { | ||
| fileChangeListsDao.insert(appInfo.id, allLocalUserFiles) | ||
| changeNumbersDao.insert(appInfo.id, cloudAppChangeNumber) | ||
| } | ||
| } | ||
| syncResult = SyncResult.UpToDate | ||
| filesManaged = allLocalUserFiles.size | ||
| rehydratedSilently = true |
There was a problem hiding this comment.
we're checking the sha here twice, first here, and then when deciding whether to upload/download games. Can we reduce that duplication? Gonna happen twice on every game boot
There was a problem hiding this comment.
The SHA check is gated by hasUncachedLocalFiles, will only happen after a destructive DB update, not on every boot. If hasUncachedLocalFiles == true, there is either:
- rehydration and no 2nd SHA check, or
- conflict dialog, which would do a 2nd SHA check via
getFilesDifforhasHashConflictsdepending on the resolution.
So the potential duplication is only if there's been a destructive DB update, and only if there's a genuine conflict. I think the (significant) additional complexity for consolidation of those two checks isn't justified. Or are you referring to something else?
|
Hey @jeremybernstein & @utkarshdalal, I tested in this way:
Summary:
|
…hdalal#1228) destructive db migration wipes file_change_lists cache, making every steam game trigger conflict on first launch post-update. check if local state is byte-identical to remote manifest (by filename + SHA) before declaring conflict; if so, rehydrate cache and report UpToDate silently. also populates real timestamps on the genuine-divergence path (was showing epoch). test: dbCleared_localMatchesRemote_rehydratesSilently_noConflict
3 participants
Description
A destructive Room migration (e.g. when an auto-migration is missing and
fallbackToDestructiveMigrationfires) wipes thefile_change_listscache while leaving save files on disk intact. On next launch of any Steam game, the code detected "cache absent + local files present" and unconditionally declared a conflict, surfacing the save-conflict dialog with epoch timestamps (Date(0)on both sides). Users saw every game report a conflict after an app update, with identical dates, even though nothing had changed locally.This PR checks whether local state is byte-identical to the cloud manifest before declaring a conflict. If it matches, we rehydrate the cache silently and report
UpToDate. If it doesn't, we still declare a conflict — but now populate real timestamps.(pathPrefixIndex, basename)while local scan stores subpath-relative filenames). Keys are lowercased since Windows paths are case-insensitive and wine/cloud may disagree on case.UserFileInfos intoFileChangeListsDaoand the cloud change number intoChangeNumbersDao, setUpToDate, skip further work.remoteTimestamp/localTimestampfrom the cloud manifest + local mtimes so the dialog renders real dates instead of the epoch.Recording
n/a — backend-only, no UI change on the happy path (dialog is suppressed when it previously fired). The user-visible effect on the conflict path is the dialog showing real dates instead of "Thu Jan 01 01:00:00 1970" on both sides.
Type of Change
Test plan
dbCleared_localMatchesRemote_rehydratesSilently_noConflictthat wipes both cache tables, assertsUpToDate+ silent rehydrate. Verified it fails cleanly (fast) againstorigin/master.run-as ... rm databases/pluvia.db*, relaunch, open any Steam game. Before: every game shows conflict with epoch dates. After: silent rehydrate when state actually matches; real dates when it doesn't.Checklist
#code-changes, I have discussed this change there and it has been green-lighted. If I do not have access, I have still provided clear context in this PR.CONTRIBUTING.md.Summary by cubic
Prevents false Steam Cloud conflicts after a destructive DB migration by verifying local saves match the cloud and silently rebuilding the cache when they do. No more blanket conflict dialogs after updates; real conflicts now show real timestamps.
Written for commit 5021ecb. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests