fix: queue user and device ID changes for proper event ordering#349
fix: queue user and device ID changes for proper event ordering#349
Conversation
Queue identity updates and commit them in
|
ca80fc8 to
8b5f5ec
Compare
2ff8cdd to
c57c618
Compare
|
Converting to draft again, there is still a case that needs to be handled: |
android/src/main/java/com/amplitude/android/plugins/AnalyticsConnectorIdentityPlugin.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/amplitude/android/plugins/AndroidContextPlugin.kt
Show resolved
Hide resolved
5aab61a to
0139681
Compare
android/src/main/java/com/amplitude/android/plugins/AndroidContextPlugin.kt
Outdated
Show resolved
Hide resolved
ab4feba to
1a91523
Compare
1a91523 to
06a697d
Compare
- core's timeline is empty as it is, so focusing on Android for now
06a697d to
d18d353
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
|
|
||
| override fun setDeviceId(deviceId: String): Amplitude { | ||
| store.deviceId = deviceId | ||
| super.setDeviceId(deviceId) |
There was a problem hiding this comment.
Deferred identity callbacks can overwrite newer immediate state
Medium Severity
The dual-write architecture introduces a race: the Android setDeviceId/setUserId overrides write to store immediately, then super launches a deferred coroutine that commits to the identity manager, whose AnalyticsIdentityListener callback writes to store again. When two calls happen in succession with different values, the deferred callback from the first call can temporarily revert the store to the stale value, causing events enriched during that window (on storageIODispatcher) to pick up the wrong identity — the exact problem this PR aims to fix.
Additional Locations (1)
There was a problem hiding this comment.
It's a pre-existing race in core's coroutine dispatch + listener pattern, not introduced by this PR. Noted, in case it becomes an issue later.
There was a problem hiding this comment.
I think this is valid - state would be temporarily overwritten by AnalyticsIdentityListener, leading to the same mismatch between what was set and what we add to events. Does AnalyticsIdentityListener still have to update the state on initialization if we're already updating state from the user/device id setters?
There was a problem hiding this comment.
It looks to me that the onUserIdChange/onDeviceIdChange is the one flagged here.
So I made those noop, since we're updating state ourselves anyway.
There was a problem hiding this comment.
Can experiment edit the identity too? The callbacks on AnalyticsIdentityListener still may be used even if they aren't used here.
I do see a race condition here specifically in the IdentityUpdateType.Initialized path in AnalyticsIdentityListener -
init(deviceId: A) // sets state deviceId: A, async kicks off AnalyticsIdentityListener.init
setDeviceId: B // sets state deviceId: B
init async callback // sets state deviceId: A again
|
|
||
| override fun setDeviceId(deviceId: String): Amplitude { | ||
| store.deviceId = deviceId | ||
| super.setDeviceId(deviceId) |
There was a problem hiding this comment.
I think this is valid - state would be temporarily overwritten by AnalyticsIdentityListener, leading to the same mismatch between what was set and what we add to events. Does AnalyticsIdentityListener still have to update the state on initialization if we're already updating state from the user/device id setters?
| configuration.deviceId | ||
| ?: amplitude.store.deviceId?.takeIf { validDeviceId(it, allowAppSetId = false) } | ||
| ?: createDeviceId() | ||
| amplitude.setDeviceId(deviceId) |
There was a problem hiding this comment.
this saved to storage synchronously in the previous version - could you please maintain that behavior?
There was a problem hiding this comment.
The initializeDeviceId is no longer called internally, deprecated it just in case some external usage exists for some reason.
Now the actual init path is in buildInternal() which now persists directly
There was a problem hiding this comment.
Previously all the calls here went from setDeviceId -> amplitude.setDeviceIdInternal (via the AndroidContextPlugin override) -> idContainer.identityManager.editIdentity().setDeviceId(deviceId).commit(). It seems like this now waits for isBuilt in the core amplitude instance.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| if (updateType == IdentityUpdateType.Initialized) { | ||
| state.userId = identity.userId | ||
| state.deviceId = identity.deviceId | ||
| // TODO("update device id based on configuration") |
There was a problem hiding this comment.
No-op identity callbacks lose state after initialization overwrites
Medium Severity
Making onUserIdChange and onDeviceIdChange no-ops removes the feedback loop that corrected state after the Initialized callback. If setUserId("x") is called before build completes, store.userId is set to "x" immediately. Then createIdentityContainer fires onIdentityChanged(persisted, Initialized), overwriting the store with persisted values (e.g., null). After isBuilt completes, the deferred commit() fires onUserIdChange("x") — which is now a no-op — so the store permanently loses "x". The old code restored state correctly through this callback.
f365fb3 to
4203557
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| }, | ||
| ) | ||
| configuration.deviceId?.let { setDeviceId(it) } | ||
| add(ContextPlugin()) |
There was a problem hiding this comment.
Core build no longer initializes device ID
High Severity
buildInternal() now sets deviceId only when configuration.deviceId is provided. Since ContextPlugin no longer initializes identity in setup(), fresh core instances can start with store.deviceId == null, causing events enriched by ContextPlugin to carry a missing deviceId.


Describe what this PR is addressing
Identity changes
setUserId()/setDeviceId()/reset()followed bytrack()could send events with stale identity because identity changes and events were processed on different dispatchers with no ordering guarantee.amplitudeDispatcherstorageIODispatcherDescribe the solution
Dual-path architecture:
store.userId = valueupdates in-memory state synchronously for event enrichment and plugin notificationssuper.setUserId()dispatchesidContainer.commit()via coroutine for disk persistenceKey changes:
Amplitude.kt: Android overrides updatestoreimmediately then delegate to core for persistenceState.kt: UnifiedIdentityobject with synchronized access and idempotent observer notificationsAndroidContextPlugin.kt: Refactored to purecreateDeviceId()functionSteps to verify the change
Manual verification:
setUserId("user1")then immediatelytrack("event1")reset()then immediatelytrack("event2")Checklist
Note
Medium Risk
Touches core identity/state synchronization and device ID initialization paths, which can affect event attribution and persistence across platforms despite added test coverage.
Overview
Fixes cases where
setUserId()/setDeviceId()/reset()followed bytrack()could emit events with stale identity by updating in-memory identity (State) synchronously and only committing to the identity manager asynchronously.Android initialization now computes and persists
deviceIdduringbuildInternal(preferring configured ID, then stored non-AppSet IDs, then generated), whileAndroidContextPluginis refactored to a purecreateDeviceId()helper and deprecates plugin-driven identity setting. CoreStateis reworked to hold a synchronizedIdentityobject with idempotent observer notifications,ContextPluginno longer performs deviceId initialization, and new tests assert ordering forsetUserId,setDeviceId,identifyoptions, andreset().Written by Cursor Bugbot for commit 4203557. This will update automatically on new commits. Configure here.