feat: sync offline achievements on reconnect#1268
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRecords appIds closed while offline into a persistent pending set; cancels pending sweep on disconnect/cleanup; after Steam login schedules a delayed sweep that snapshots pending appIds, scans GSE save dirs for offline achievement data, acquires per-app sync locks, calls Goldberg sync for eligible apps, and removes successful appIds. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as AchievementWatcher
participant Service as SteamService
participant Steam as SteamClient
participant FS as FileSystem
participant Sync as GoldbergSync
Client->>Service: addPendingSyncApp(appId) (when closeApp offline)
Steam->>Service: LoggedOnCallback(EResult.OK)
Service->>Service: schedule offlineAchievementSyncJob (delay)
Service->>Steam: check isConnected && isLoggedOn
alt connected
Service->>Service: snapshot pending appIds
loop per appId
Service->>FS: check GSE save dirs (achievements.json / stats/)
alt offline data present
Service->>Service: tryAcquireSync(appId)
alt lock acquired
Service->>Sync: syncAchievementsFromGoldberg(appId)
Sync-->>Service: success / failure
Service->>Service: releaseSync(appId)
alt success
Service->>Service: remove appId from pending set (update/delete file)
end
else lock busy
Service-->>Service: skip app
end
end
end
else not connected
Service-->>Service: abort sweep (retain pending set)
end
Service-->>Service: clear offlineAchievementSyncJob reference
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 25162fd8-01f5-4505-a620-8ee1ba044493
📒 Files selected for processing (1)
app/src/main/java/app/gamenative/service/SteamService.kt
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/service/SteamService.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/SteamService.kt:3538">
P1: This coroutine job is not stored or cancelled on disconnect/logout. If the connection drops or the user switches accounts while iterating installed apps, subsequent `syncAchievementsFromGoldberg` calls will proceed against a stale or different session. Store the `Job` reference (e.g., `offlineAchievementSyncJob`) and cancel it in `onDisconnected` and `clearValues`. Additionally, re-check `isConnected`/`isLoggedIn` inside the `for` loop to bail out promptly on session changes.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/service/SteamService.kt:3563">
P2: Reconnect offline-achievement sweep uses `listFiles()` for non-empty checks inside an all-app scan, causing avoidable memory/IO overhead at scale.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/service/SteamService.kt (1)
3598-3602: Null-out is reference-unsafe and may not fire on normal completion.Two small concerns with this cleanup:
- Inside the job's own
finally,offlineAchievementSyncJob?.isActiveis typically stilltrueon the normal completion path (the coroutine hasn't transitioned to completed yet while its finally runs), so the field often won't be nulled even when this run was the one that completed.- If a new invocation of
syncPendingOfflineAchievements()races in and reassignsofflineAchievementSyncJobto a fresh job right before this finally runs, a future change that toggles the condition could accidentally clear the new job reference.A reference-identity compare is safer and makes the intent clearer. Not a functional bug today (callers always reassign the field before launching), so this is a nice-to-have hardening:
Suggested tweak
-private fun syncPendingOfflineAchievements() { - offlineAchievementSyncJob?.cancel() - offlineAchievementSyncJob = scope.launch { +private fun syncPendingOfflineAchievements() { + offlineAchievementSyncJob?.cancel() + lateinit var thisJob: Job + thisJob = scope.launch { try { ... } finally { - if (offlineAchievementSyncJob?.isActive != true) { - offlineAchievementSyncJob = null - } + if (offlineAchievementSyncJob === thisJob) { + offlineAchievementSyncJob = null + } } - } + } + offlineAchievementSyncJob = thisJob }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/service/SteamService.kt` around lines 3598 - 3602, The finally block clears the job using a state check that can be true during the job's own finally or race with a new job assignment; change it to a reference-identity clear so we only null the original Job instance. In the finally in syncPendingOfflineAchievements() compare offlineAchievementSyncJob to the current coroutine/job instance (e.g. capture val current = offlineAchievementSyncJob at start or use the job reference available in the coroutine) and only set offlineAchievementSyncJob = null if offlineAchievementSyncJob === current to avoid clearing a newly assigned job.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/main/java/app/gamenative/service/SteamService.kt`:
- Around line 3598-3602: The finally block clears the job using a state check
that can be true during the job's own finally or race with a new job assignment;
change it to a reference-identity clear so we only null the original Job
instance. In the finally in syncPendingOfflineAchievements() compare
offlineAchievementSyncJob to the current coroutine/job instance (e.g. capture
val current = offlineAchievementSyncJob at start or use the job reference
available in the coroutine) and only set offlineAchievementSyncJob = null if
offlineAchievementSyncJob === current to avoid clearing a newly assigned job.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f8cfdaf8-307c-4c17-a3ab-3826c24d1689
📒 Files selected for processing (1)
app/src/main/java/app/gamenative/service/SteamService.kt
|
Will pull this down in the next day or so to test |
| try { | ||
| Timber.tag("achievements").i("Attempting reconnect achievement sync for appId=$appId") | ||
| syncAchievementsFromGoldberg(applicationContext, appId) | ||
| } catch (e: CancellationException) { | ||
| throw e | ||
| } catch (e: Exception) { |
There was a problem hiding this comment.
Do we need a double-catch? Or is this due to typing difficulties?
There was a problem hiding this comment.
Coroutine cancelation requires cancelation exception, ik it looks weird
|
This is not the correct way to do this. This will go through every single installed app on every steam reconnect (which could happen multiple times in a short span of time) - this is especially bad for external storage. I believe @phobos665 updated the syncing code so achievement uploads happen realtime. Is there a way to defer those calls to a time that we are connected and have a connection to Steam? Alternatively, when the user opens the game page, show them that they have achievements not synced and allow them to sync with a new button. Finally though, I am not sure this is worth the trouble - achievements are all uploaded on game close, so the user's achievements will be uploaded next time they close the game online. |
|
@utkarshdalal is it problematic to sync achievements in bulk though? These are very lightweight as opposed to something like save syncs, I could debounce to address your other concern but even on ext storage aren't the chieves stored in container which is always internal regardless? Also on next game launch I don't think is a great solution to rest on as user has to consciously run a game they don't intend to play just to get a sync they should take for granted. Let me know if you agree. Open to a different approach but I think core ux here is worth chasing, especially do WiFi only devices like handhelds that will be out of WiFi often |
yes it is absolutely problematic, iterating through each container on every reconnect, finding the achievements file on external storage is extremely slow. And generally speaking is not good design. |
|
And no, the files are stored on external storage. Only the wine prefix and other components are internal. |
I like this, probably should also consider saves as well. |
|
Can you help me understand when those files can be on external? Even if move game ext, the imagefs and filesDir are always internal. |
|
Also the achievements.json sweep is a roomdb query not a filesystem crawl, it's very cheap. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/service/SteamService.kt (1)
3524-3592: Sweep logic is sound; one minor observation on the completion-finally block.The structure is solid: the 2s delay + connectivity recheck, per-iteration
ensureActive()+ session/connectivity recheck,tryAcquireSync/releaseSyncper app, and the explicitCancellationExceptionre-throw pattern (Lines 3574-3576, 3582-3584) are all idiomatic for Kotlin coroutines. Removing the appId frompendingSyncAppIdsonly on success keeps unsynced apps queued for the next reconnect.Minor observation on Lines 3587-3589: during normal completion, the job is still considered active inside its own
finally, soisActive != truewill typically be false and the reference is left dangling on a completed job. This is harmless (cancellations on a completed job are no-ops, and the next call cancels-and-reassigns), but you could simplify withinvokeOnCompletion { offlineAchievementSyncJob = null }set on the capturedJobif you want deterministic null-out.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/service/SteamService.kt` around lines 3524 - 3592, The finally block leaves offlineAchievementSyncJob set to the completed Job because a Job is still active inside its own finally; to fix, capture the launched Job returned by scope.launch in syncPendingOfflineAchievements (offlineAchievementSyncJob = scope.launch { ... } should assign the Job), then call offlineAchievementSyncJob?.invokeOnCompletion { offlineAchievementSyncJob = null } immediately after creating it so the reference is deterministically cleared when the job completes or is cancelled; remove or simplify the existing finally branch that checks offlineAchievementSyncJob?.isActive.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/service/SteamService.kt`:
- Around line 264-265: pendingSyncAppIds is currently created with
Collections.synchronizedSet(mutableSetOf()), which still requires external
synchronization for safe iteration (see usage at pendingSyncAppIds.toSet() and
concurrent adds in closeApp); replace its declaration to use
ConcurrentHashMap.newKeySet() (like workshopPausedApps) to get a
weakly-consistent iterator safe for concurrent traversal, remove the redundant
java.util. qualifier, and add the import for
java.util.concurrent.ConcurrentHashMap (or fully-qualify only the constructor)
so all concurrent access sites (e.g., closeApp and the iteration at toSet()) are
safe without external synchronization; leave offlineAchievementSyncJob
untouched.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/service/SteamService.kt`:
- Around line 3524-3592: The finally block leaves offlineAchievementSyncJob set
to the completed Job because a Job is still active inside its own finally; to
fix, capture the launched Job returned by scope.launch in
syncPendingOfflineAchievements (offlineAchievementSyncJob = scope.launch { ... }
should assign the Job), then call offlineAchievementSyncJob?.invokeOnCompletion
{ offlineAchievementSyncJob = null } immediately after creating it so the
reference is deterministically cleared when the job completes or is cancelled;
remove or simplify the existing finally branch that checks
offlineAchievementSyncJob?.isActive.
🪄 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
Run ID: 7f3b1a71-c952-4f1c-823d-e7689dac88ff
📒 Files selected for processing (1)
app/src/main/java/app/gamenative/service/SteamService.kt
There was a problem hiding this comment.
2 issues found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/service/SteamService.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/SteamService.kt:265">
P1: `Collections.synchronizedSet` does not provide thread-safe iteration. `pendingSyncAppIds.toSet()` internally iterates the set without external synchronization while `closeApp` can concurrently `add` from a different coroutine on `Dispatchers.IO`, risking `ConcurrentModificationException`. Use `ConcurrentHashMap.newKeySet()` instead, which provides a weakly-consistent iterator safe for concurrent traversal.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/service/SteamService.kt:3535">
P2: Reconnect offline-achievement sync now relies on transient in-memory app IDs, so offline achievements can be silently skipped after service/process restart or non-standard app termination.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/service/SteamService.kt (1)
3572-3585: ReusecollectGseUnlocksAndStatsto avoid divergent scan logic.The check at lines 3578-3581 duplicates the scan that
collectGseUnlocksAndStats(lines 2879-2903) already performs and constructsFile(dir, "stats")twice. You can reuse the helper to keep both paths in sync (e.g., if the eligibility rule for "has offline data" later changes) and tighten the loop:Suggested refactor
- val hasOfflineAchievementData = gseSaveDirs.any { dir -> - File(dir, "achievements.json").exists() || - (File(dir, "stats").isDirectory && (File(dir, "stats").listFiles()?.isNotEmpty() == true)) - } - if (!hasOfflineAchievementData) { + val (unlocked, statsDir) = collectGseUnlocksAndStats(gseSaveDirs) + if (unlocked.isEmpty() && statsDir == null) { removePendingSyncApp(appId) continue }
syncAchievementsFromGoldbergwill recompute internally; the duplicated read is cheap relative to the network sync and removes the divergence risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/service/SteamService.kt` around lines 3572 - 3585, The code duplicates the offline-data scan logic by re-checking achievements.json and stats dir instead of reusing the existing helper; replace the inline check that builds hasOfflineAchievementData (after getting gseSaveDirs in syncAchievementsFromGoldberg) with a call to collectGseUnlocksAndStats(applicationContext, appId) (or a small wrapper that inspects its result) to decide whether to call removePendingSyncApp(appId) and continue, ensuring you reference the same logic used by collectGseUnlocksAndStats and keep calls to getGseSaveDirs, removePendingSyncApp and syncAchievementsFromGoldberg unchanged otherwise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/service/SteamService.kt`:
- Around line 3326-3331: clearValues() currently deletes pendingSyncFile which
removes the persistent queue the PR intended to survive service restarts; change
clearValues() (the block that calls runCatching { pendingSyncFile.delete() }) to
only cancel jobs and clear the in-memory pendingSyncAppIds set without deleting
pendingSyncFile, and move the file deletion logic into the logout/account-change
code paths such as performLogOffDuties or clearUserData so the file is only
removed on genuine logoff/account switch.
- Around line 3532-3544: The code races when writing pendingSyncFile because
addPendingSyncApp and removePendingSyncApp both compute
pendingSyncAppIds.joinToString(...) and call writeText(...) without
synchronization, causing lost updates; fix by serializing access to the
in-memory set and file writes using a shared lock/mutex (e.g. a Kotlin Mutex or
synchronized(this) guarding pendingSyncAppIds) so both addPendingSyncApp and
removePendingSyncApp acquire the same lock before mutating pendingSyncAppIds and
writing to pendingSyncFile, and additionally make the file write atomic by
writing to a temp file (pendingSyncFile.tmp) then renaming/moving it to
pendingSyncFile to avoid partial writes on process kill.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/service/SteamService.kt`:
- Around line 3572-3585: The code duplicates the offline-data scan logic by
re-checking achievements.json and stats dir instead of reusing the existing
helper; replace the inline check that builds hasOfflineAchievementData (after
getting gseSaveDirs in syncAchievementsFromGoldberg) with a call to
collectGseUnlocksAndStats(applicationContext, appId) (or a small wrapper that
inspects its result) to decide whether to call removePendingSyncApp(appId) and
continue, ensuring you reference the same logic used by
collectGseUnlocksAndStats and keep calls to getGseSaveDirs, removePendingSyncApp
and syncAchievementsFromGoldberg unchanged otherwise.
🪄 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
Run ID: 85c46839-124f-4b15-b39a-a5cf3462bfce
📒 Files selected for processing (1)
app/src/main/java/app/gamenative/service/SteamService.kt
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/service/SteamService.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/SteamService.kt:3331">
P2: Deleting the persisted offline-achievement queue during service cleanup can discard unsynced app IDs before the reconnect sweep runs.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/service/SteamService.kt:3535">
P2: Pending achievement-sync IDs are persisted with unsynchronized full-file write/delete operations, allowing concurrent races that can lose IDs across process restarts.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/service/SteamService.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/SteamService.kt:2640">
P2: Persisted pending achievement sync state is global and not reliably cleared on service teardown, allowing stale app IDs to be replayed in a later session.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| @@ -261,6 +261,10 @@ class SteamService : Service(), IChallengeUrlChanged { | |||
|
|
|||
There was a problem hiding this comment.
P2: Persisted pending achievement sync state is global and not reliably cleared on service teardown, allowing stale app IDs to be replayed in a later session.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/java/app/gamenative/service/SteamService.kt, line 2640:
<comment>Persisted pending achievement sync state is global and not reliably cleared on service teardown, allowing stale app IDs to be replayed in a later session.</comment>
<file context>
@@ -2636,7 +2637,7 @@ class SteamService : Service(), IChallengeUrlChanged {
private fun clearUserData(clearCloudSyncState: Boolean = false) {
PrefManager.clearSteamSessionPreferences()
-
+ instance?.clearPendingSync()
clearDatabase(clearCloudSyncState = clearCloudSyncState)
}
</file context>
There was a problem hiding this comment.
It's a task queue that's the idea
…evement-reconnect-sync
3 participants
Description
adds a lightweight post reconnect sweep for offline Steam achievement sync. After a successful logon, it checks installed apps for gbe achievement or stat output and reuses the existing sync path to push anything earned while offline.
Recording
Type of Change
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. If I skip both, I accept that this change may face delays in review, may not be reviewed at all, or may be closed.CONTRIBUTING.md.Summary by cubic
Automatically syncs achievements earned while offline when you reconnect to Steam. Apps that earned achievements while disconnected (including games closed offline) are tracked and persisted across restarts.
New Features
syncAchievementsFromGoldberg.Bug Fixes
clearUserData()(no loss on service stop/restart).Written for commit bdeeaa4. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Bug Fixes
Reliability