fix: login/logout race causes subsequent calls to target previous user by nan-li · Pull Request #2618 · OneSignal/OneSignal-Android-SDK

nan-li · 2026-04-17T23:13:35Z

Description

One Line Summary

(Not JWT related)
Implementation of #2600

Fix a race in OneSignal.login() and OneSignal.logout() where SDK calls made immediately after either operation (e.g. addTag) landed on the previous user's identity.

Details

Motivation

Both OneSignal.login() and OneSignal.logout() scheduled the local user switch asynchronously via suspendifyOnIO, so the API call returned before the identity/properties/subscription models were updated. Calls made right after targeted the OLD user.

Example reproduction:

OneSignal.login("alice")
OneSignal.User.addTag("name_aa", "alice")    // lands on old user, not alice

OneSignal.login("bob")
OneSignal.User.addTag("name_bb", "bob")      // lands on alice (or whoever), not bob

OneSignal.logout()
OneSignal.User.addTag("post_logout", "val")  // lands on bob, not the new anonymous user

Scope

Split both LoginHelper.login() and LogoutHelper.logout() into two phases each:

switchUser() — synchronous under the login/logout lock. Swaps the local identity/properties/subscription models immediately so subsequent SDK calls see the new user.
enqueueLogin() / enqueueLogout() — async. Enqueues the LoginUserOperation (and for login, awaits completion of the backend user creation).

OneSignalImp.login(), loginSuspend(), logout(), and logoutSuspend() all call switchUser() synchronously, then launch the enqueue in the coroutine scope.

Because switchUser returns before the enqueue runs, there's a brief window where RecoverFromDroppedLoginBug can observe the intermediate state (new externalId, local onesignalId, no LoginUserOperation in queue) and enqueue its own recovery login. When the real enqueue then adds another login op, the executor crashes on a grouped batch containing two LoginUserOperations with "Unrecognized operation".

To handle this:

OperationRepo.internalEnqueue dedupes LoginUserOperation by onesignalId. If a queued op already exists for the same user, the incoming op is dropped. The incoming op's existingOnesignalId is merged into the queued op first so the anonymous-user conversion link isn't lost (the recovery op always has existingOnesignalId = null, the real login op may carry the anon UUID needed for the backend merge).
The incoming op's waiter is transferred to the queued op so enqueueAndWait callers (e.g. loginSuspend) receive the real execution result.
LoginUserOperation.existingOnesignalId setter relaxed from private to internal to allow the in-place merge.

Also applies the FAIL_PAUSE_OPREPO waker fix from #2600: ops re-queued on pause detach their waiters (wake with false) so enqueueAndWait callers don't hang on a paused queue.

Related: #2600 made the same split on feat/identity_verification_5.8, but that branch hasn't merged. This port brings the fix to main so all users get the race fix in the next release, and extends the pattern symmetrically to logout.

Testing

Unit testing

Updated LoginHelperTests to use the new switchUser() + enqueueLogin() split; existing tests pass.
Updated LogoutHelperTests to use the new switchUser() + enqueueLogout() split; existing tests pass.
Added a new OperationRepoTests case for the dedupe+merge path: seed a LoginUserOperation(existingOnesignalId=null) in the queue (simulates RecoverFromDroppedLoginBug), enqueue an incoming op with the same onesignalId and a non-null existingOnesignalId, verify the queued op is merged in place and the incoming op is dropped. Uses waitForInternalEnqueue helper for determinism.
Updated the loadSavedOperations test to construct LoginUserOperation with full args (the previous no-args construction stopped working once the dedupe reads onesignalId).

Manual testing

Reproduced the original login bug in the demo app with back-to-back login() + addTag() calls — confirmed each tag landed on the wrong user before the fix.
After the fix: each tag lands on the intended user.
Verified the symmetric logout case: logout() + addTag() now attaches the tag to the new anonymous user instead of the previously logged-in one.
Reproduced the RecoverFromDroppedLoginBug race on fresh install + quick login() call — confirmed the executor no longer crashes with "Unrecognized operation" and the anonymous-user conversion link is preserved.

Affected code checklist

Checklist

Overview

I have filled out all REQUIRED sections above
PR does one thing
Any Public API changes are explained in the PR details and conform to existing APIs

Testing

I have included test coverage for these changes, or explained why they are not needed
All automated tests pass, or I explained why that is not possible
I have personally tested this on my device, or explained why that is not possible

Final pass

Code is as readable as possible.
I have reviewed this PR myself, ensuring it meets each checklist item

- Compare base.sha...head.sha on pull_request so the target branch matches the PR base. - Enforce >= threshold on aggregate touched executable lines; keep per-file detail informational. - Fail when main-source files in the diff cannot be mapped to JaCoCo. - Rename workflow step for clarity. Made-with: Cursor

- GET /apps/:app_id/sdk/features/android/:sdk_version with validated version label - FeatureFlagsJsonParser (kotlinx.serialization), metadata as sibling JsonObjects - FeatureFlagsRefreshService with ~10 min foreground polling; ConfigModelChangeTags.REMOTE_FEATURE_FLAGS - FeatureManager merges remote flags; IFeatureManager.remoteFeatureFlagMetadata() - SDK_BACKGROUND_THREADING / sdk_background_threading; CoreModule wiring and tests Made-with: Cursor

- Annotate pollJob with @volatile for cross-thread visibility - Remove duplicate onFocus from start(); ApplicationService already fires on subscribe - Apply remote flags via in-place model property updates (REMOTE_FEATURE_FLAGS tag) to avoid clobbering concurrent params hydration with a stale full-model replace - Use OneSignalDispatchers.launchOnIO so CancellationException is not logged as an error - Compare enabled key sets (order-insensitive) before writing Made-with: Cursor

nan-li · 2026-04-20T15:51:38Z

@claude review

…y casing - RemoteFeatureFlagsFetchOutcome Success/Unavailable; parseSuccessful distinguishes valid Turbine JSON (including empty features) from parse/contract failure; no cache wipe on Unavailable - Polling loop: rethrow CancellationException; catch Exception only for fetch errors - FeatureManager: drop dead REMOTE_FEATURE_FLAGS onModelReplaced branch; canonicalize flag keys - ConfigModelChangeTags + FeatureFlag KDoc/comment fixes - Tests: parseSuccessful, case-insensitive remote key Made-with: Cursor

nan-li · 2026-04-20T18:29:50Z

@claude review

Keep the "next fetch in …ms" and before/after diff debug logs in FeatureFlagsRefreshService, and keep mavenLocal() in the demo's allprojects repositories; just remove the TODO(revert-before-merge) markers now that these are staying. Made-with: Cursor

nan-li · 2026-04-22T18:17:03Z

@claude review

nan-li · 2026-04-22T18:23:10Z

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

nan-li · 2026-04-22T20:50:22Z

@claude review

sherwinski · 2026-04-23T19:54:35Z

+                    if (queueItem.waiter != null && existing.waiter == null) {
+                        existing.waiter = queueItem.waiter
+                    } else {
+                        queueItem.waiter?.wake(true)
+                    }


Might be worth writing a test for this (and L36) that confirms the waiters get switched.

Something like: seed the queue with an OperationQueueItem(queuedOp, waiter = seededWaiter, bucket = 0), then call enqueueAndWait for the same
onesignalId, let the executor run the queued op (mock it to return SUCCESS), and assert both the seeded waiter and the enqueueAndWait caller wake with true.

added new test

…ract-violating responses ConfigModelStoreListener.fetchParams now re-sources sdkRemoteFeatureFlags{,Metadata} from the live model immediately before replace(), so concurrent in-place writes from FeatureFlagsRefreshService survive Model.initializeFromModel's data.clear()+putAll(). FeatureFlagsJsonParser.parseRootStrict now distinguishes an authoritative empty features array from one that filtered down to empty due to element-type violations; the latter returns null -> Unavailable so callers preserve the cached list rather than overwriting it with []. Adds regression tests for both paths. Addresses PR #2612 review comments. Made-with: Cursor

`ConfigModelStoreListener.fetchParams` replaces the live `ConfigModel` with the HYDRATE tag during init, which fired `onModelReplaced` on `FeatureFlagsRefreshService` and triggered a second Turbine GET right after the focus-driven first one, even though the appId hadn't changed. Track the appId currently being polled and treat a re-trigger for the same appId as a no-op; genuine appId changes still cancel and restart. `onUnfocused` clears the cached appId so the next focus event re-arms polling. Pull the foreground cadence off the constructor and onto an `internal var refreshIntervalMs` (default 8 min, test-only override before `start`). The IoC reflection in `ServiceRegistrationReflection.resolve` cannot resolve a non-service `Long` parameter and was throwing "Could not instantiate service: ServiceRegistrationReflection@..." out of `StartupService.scheduleStart` once the service was registered. Made-with: Cursor

OneSignal.login() scheduled the user switch asynchronously via suspendifyOnIO, so the API call returned before the identity model was updated. Calls made right after login (e.g. addTag) applied to the OLD user because the switch hadn't happened yet. Split login into two phases: - switchUser(): synchronous under the login/logout lock. Swaps the local identity/properties/subscription models immediately so subsequent SDK calls see the new user. - enqueueLogin(): async. Enqueues the LoginUserOperation and awaits completion of the backend user creation. Because switchUser returns before enqueueLogin runs, there's a small window where RecoverFromDroppedLoginBug can observe the state (new externalId, local onesignalId, no LoginUserOperation in queue) and enqueue its own recovery login. When the real enqueueLogin then adds another login op, the executor crashes on a grouped batch containing two LoginUserOperations. Add a dedupe check in internalEnqueue: if a LoginUserOperation for the same onesignalId is already queued, drop the incoming one and wake its waiter optimistically with true so loginSuspend callers don't hang on enqueueAndWait. If the duplicate came from loadSavedOperations (addToStore = false), also remove it from the operation model store so we don't leave an orphan entry that lingers until the next cold start. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

When dedupe fires, the existing queued op came from RecoverFromDroppedLoginBug (always has existingOnesignalId=null) and the incoming op from enqueueLogin may carry the anonymous user's UUID needed to merge their tags/subscriptions into the newly- identified user. Previously the dedupe kept whichever op was first, silently dropping the merge info when the recovery op won the race. Merge the incoming op's existingOnesignalId into the queued op in place before dropping the duplicate. LoginUserOperation.existingOnesignalId setter relaxed from private to internal to allow the in-place update. Also initialize the LoginUserOperations in the loadSavedOperations test properly — the old dedupe only compared ids so uninitialized ops worked incidentally; the new dedupe reads onesignalId. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

- Change OperationQueueItem.waiter to var. On LoginUserOperation dedupe, transfer the incoming op's waiter to the already-queued item so the enqueueAndWait caller (loginSuspend) receives the real execution result instead of an optimistic true. Fall back to optimistic wake only in the unlikely case where both items already have waiters. - Add OperationRepoTests coverage for the dedupe+merge path with matching onesignalId and differing existingOnesignalId — the previous test setup used different onesignalIds so the merge branch had no regression guard.

When an operation execution returns FAIL_PAUSE_OPREPO, the repo is paused and the ops are re-queued with their original waiters still attached. enqueueAndWait callers (loginSuspend) would suspend on the waiter forever — the op stays in the queue but never executes while paused, and the waiter is never woken. Wake the waiter with false and re-queue with waiter=null, matching the pattern from #2600. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Replace the ad-hoc runBlocking { delay(100) } in the new LoginUserOperation dedupe test with the same waitForInternalEnqueue helper every other test in the file uses. Deterministic instead of a fixed 100ms race against CI.

Mirrors the login race fix for the logout flow. OneSignal.logout() scheduled the user switch asynchronously via suspendifyOnIO, so the API call returned before the identity/properties/subscription models were updated. Calls made right after logout() (e.g. addTag) targeted the pre-logout user. Split LogoutHelper.logout() into switchUser() (sync, under loginLogoutLock) and enqueueLogout() (async). OneSignalImp.logout() and logoutSuspend() call switchUser() synchronously, then launch enqueueLogout() in the coroutine scope — mirroring LoginHelper. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Guard the LoginUserOperation dedup+merge path with !IDManager.isLocalId. When upgrading from 5.0.0-5.1.7 with a dropped anon-user create, switchUser returns a local- existingOneSignalId. If RecoverFromDroppedLoginBug wins the race and its null-existingOnesignalId op is queued first, merging the local id flips canStartExecute to false and strands the op — the waiter (now transferred) never wakes, and the stuck op blocks future recovery.

When the queued LoginUserOperation carries a waiter, dedupe wakes the incoming enqueueAndWait caller immediately with true and the queued op's waiter receives the real execution result on SUCCESS. Existing dedupe coverage only exercised the waiter-transfer branch where the queued op had no waiter. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

AR Abdul Azeez added 5 commits April 9, 2026 13:37

Merge branch 'main' into sdk-4363-android-sdk-turbine-feature-flags

72890cc

feature flag PR tweaks

c63f686

claude Bot reviewed Apr 17, 2026

View reviewed changes

Comment thread ...DK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt Outdated

nan-li force-pushed the fix/login-race-condition-user-data-mixed branch 2 times, most recently from 10725ac to 2d9f1f3 Compare April 20, 2026 15:50

claude Bot reviewed Apr 20, 2026

View reviewed changes

nan-li force-pushed the fix/login-race-condition-user-data-mixed branch from 264a52a to 6c5f33c Compare April 20, 2026 18:23

claude Bot reviewed Apr 20, 2026

View reviewed changes

Comment thread ...DK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt

claude Bot reviewed Apr 20, 2026

View reviewed changes

Comment thread ...DK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt Outdated

AR Abdul Azeez added 2 commits April 21, 2026 13:44

more logging

c7a2720

nan-li force-pushed the fix/login-race-condition-user-data-mixed branch 2 times, most recently from 99334e8 to d79186a Compare April 22, 2026 17:52

claude Bot reviewed Apr 22, 2026

View reviewed changes

Comment thread OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt

nan-li changed the title ~~fix: login race causes subsequent calls to target previous user~~ fix: login/logout race causes subsequent calls to target previous user Apr 22, 2026

claude Bot reviewed Apr 22, 2026

View reviewed changes

Comment thread ...DK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt Outdated

nan-li mentioned this pull request Apr 22, 2026

closed #2617

Closed

9 tasks

claude Bot reviewed Apr 22, 2026

View reviewed changes

Comment thread ...DK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt

Addressed comments

ce19184

nan-li requested a review from a team April 23, 2026 16:56

sherwinski reviewed Apr 23, 2026

View reviewed changes

removing features node in ConfigModel

0a3d18e

nan-li force-pushed the fix/login-race-condition-user-data-mixed branch from 33b73fc to 2f615fc Compare April 23, 2026 22:40

nan-li changed the base branch from main to sdk-4363-android-sdk-turbine-feature-flags April 23, 2026 22:41

nit(style): fix spotless formatting and core detekt issues

68f2df3

nan-li force-pushed the sdk-4363-android-sdk-turbine-feature-flags branch from 441c409 to 68f2df3 Compare April 24, 2026 18:37

magic numbers

2861144

nan-li requested a review from sherwinski April 27, 2026 16:21

nan-li force-pushed the fix/login-race-condition-user-data-mixed branch from 6a69a4b to 3c9b55d Compare April 27, 2026 16:22

nan-li requested a review from abdulraqeeb33 April 27, 2026 16:23

AR Abdul Azeez and others added 11 commits April 27, 2026 14:07

tests: Use waitForInternalEnqueue helper in dedupe test

90bdb72

Replace the ad-hoc runBlocking { delay(100) } in the new LoginUserOperation dedupe test with the same waitForInternalEnqueue helper every other test in the file uses. Deterministic instead of a fixed 100ms race against CI.

nit: update some comments for clarity

9d6341a

nan-li force-pushed the fix/login-race-condition-user-data-mixed branch from 3c9b55d to dce3184 Compare April 27, 2026 22:55

nit: detekt

ea96c18

Base automatically changed from sdk-4363-android-sdk-turbine-feature-flags to main April 28, 2026 19:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: login/logout race causes subsequent calls to target previous user#2618

fix: login/logout race causes subsequent calls to target previous user#2618
nan-li wants to merge 24 commits intomainfrom
fix/login-race-condition-user-data-mixed

nan-li commented Apr 17, 2026 •

edited

Loading

Uh oh!

Uh oh!

nan-li commented Apr 20, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nan-li commented Apr 20, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nan-li commented Apr 22, 2026

Uh oh!

nan-li commented Apr 22, 2026

Uh oh!

Uh oh!

Uh oh!

nan-li commented Apr 22, 2026

Uh oh!

sherwinski Apr 23, 2026

Uh oh!

sherwinski Apr 23, 2026

Uh oh!

nan-li Apr 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

nan-li commented Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

One Line Summary

Details

Motivation

Scope

Testing

Unit testing

Manual testing

Affected code checklist

Checklist

Overview

Testing

Final pass

Uh oh!

Uh oh!

nan-li commented Apr 20, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nan-li commented Apr 20, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nan-li commented Apr 22, 2026

Uh oh!

nan-li commented Apr 22, 2026

Code review

Uh oh!

Uh oh!

Uh oh!

nan-li commented Apr 22, 2026

Uh oh!

sherwinski Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

sherwinski Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

nan-li Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nan-li commented Apr 17, 2026 •

edited

Loading