Make SyncStateStore map updates atomic - #120
Conversation
in-jun
left a comment
There was a problem hiding this comment.
Confirmed the race this targets: record() is called from SyncManager on worker/service dispatcher threads while forget() runs from MainViewModel.deletePair on the main thread, and the two plain _lastSync.value = _lastSync.value ... assignments weren't serialized against each other, so concurrent writers could read the same map and clobber one another.
Switching both to _lastSync.update { ... } fixes this cleanly — the CAS retry loop makes each read-modify-write atomic against concurrent writers, which is exactly the primitive this needs. These two assignments are the only non-atomic map updates in the class, so the fix is complete. Leaving the prefs.edit().apply() calls as-is is correct: SharedPreferences handles its own concurrency and those values self-heal on the next load anyway.
Minimal and well-targeted. Looks good.
SyncStateStore.record()andforget()performed a plain read-modify-write on the shared_lastSyncMutableStateFlowmap.record()runs on worker/service dispatcher threads whileforget()runs on the main thread fromMainViewModel.deletePair, and the two aren't serialized against each other, so concurrent writers could read the same old map and silently clobber one another's change._lastSync.value = _lastSync.value ...assignments with_lastSync.update { ... }so the read-modify-write is atomic against concurrent writers.Fixes #52