Skip to content

Commit 4cb1242

Browse files
committed
Fix race condition: stamp externalId synchronously before async enqueue
The externalId was being stamped inside internalEnqueue which runs on the OperationRepo coroutine thread. When LogoutHelper set isDisabledInternally=true (triggering an UpdateSubscriptionOperation) then immediately called createAndSwitchToNewUser(), the identity model's externalId could be cleared before the coroutine ran, leaving the operation with externalId=null and permanently blocked. Move stamping to enqueue()/enqueueAndWait() on the caller's thread so the externalId is captured at the moment the operation is created. Made-with: Cursor
1 parent ba938da commit 4cb1242

File tree

2 files changed

+78
-7
lines changed

2 files changed

+78
-7
lines changed

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,7 @@ internal class OperationRepo(
129129
Logging.log(LogLevel.DEBUG, "OperationRepo.enqueue(operation: $operation, flush: $flush)")
130130

131131
operation.id = UUID.randomUUID().toString()
132+
stampExternalId(operation)
132133
scope.launch {
133134
internalEnqueue(OperationQueueItem(operation, bucket = enqueueIntoBucket), flush, true)
134135
}
@@ -141,6 +142,7 @@ internal class OperationRepo(
141142
Logging.log(LogLevel.DEBUG, "OperationRepo.enqueueAndWait(operation: $operation, force: $flush)")
142143

143144
operation.id = UUID.randomUUID().toString()
145+
stampExternalId(operation)
144146
val waiter = WaiterWithValue<Boolean>()
145147
scope.launch {
146148
internalEnqueue(OperationQueueItem(operation, waiter, bucket = enqueueIntoBucket), flush, true)
@@ -154,19 +156,24 @@ internal class OperationRepo(
154156
*
155157
* @returns true if the OperationQueueItem was added, false if not
156158
*/
159+
/**
160+
* Capture the externalId from the current identity model onto the operation
161+
* synchronously on the caller's thread, before the async enqueue coroutine runs.
162+
* Operations that already set externalId in their constructor (e.g. LoginUserOperation)
163+
* are left unchanged.
164+
*/
165+
private fun stampExternalId(operation: Operation) {
166+
if (operation.externalId == null) {
167+
operation.externalId = _identityModelStore.model.externalId
168+
}
169+
}
170+
157171
private fun internalEnqueue(
158172
queueItem: OperationQueueItem,
159173
flush: Boolean,
160174
addToStore: Boolean,
161175
index: Int? = null,
162176
) {
163-
// Stamp externalId on new operations from the current identity model.
164-
// Operations loaded from persistence (addToStore=false) already have their externalId.
165-
// Operations that set externalId in their constructor (e.g. LoginUserOperation) are skipped.
166-
if (addToStore && queueItem.operation.externalId == null) {
167-
queueItem.operation.externalId = _identityModelStore.model.externalId
168-
}
169-
170177
synchronized(queue) {
171178
val hasExisting = queue.any { it.operation.id == queueItem.operation.id }
172179
if (hasExisting) {

OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -960,6 +960,70 @@ class OperationRepoTests : FunSpec({
960960
handlerCalledWith shouldBe "test-user"
961961
}
962962

963+
test("enqueue stamps externalId synchronously before async dispatch") {
964+
// Verifies the fix for a race condition where createAndSwitchToNewUser()
965+
// could clear the identity model's externalId before the async internalEnqueue
966+
// had a chance to stamp it.
967+
968+
// Given
969+
val identityModel = com.onesignal.user.internal.identity.IdentityModel()
970+
identityModel.id = "-singleton"
971+
identityModel.onesignalId = "onesignal-id"
972+
identityModel.externalId = "old-user"
973+
974+
val identityModelStore = mockk<com.onesignal.user.internal.identity.IdentityModelStore>(relaxed = true)
975+
every { identityModelStore.model } returns identityModel
976+
977+
val configModelStore =
978+
MockHelper.configModelStore {
979+
it.useIdentityVerification = true
980+
}
981+
val jwtTokenStore = mockk<JwtTokenStore>(relaxed = true)
982+
every { jwtTokenStore.getJwt("old-user") } returns "valid-jwt"
983+
984+
val operationModelStore =
985+
run {
986+
val operationStoreList = mutableListOf<Operation>()
987+
val mock = mockk<OperationModelStore>()
988+
every { mock.loadOperations() } just runs
989+
every { mock.list() } answers { operationStoreList.toList() }
990+
every { mock.add(any()) } answers { operationStoreList.add(firstArg<Operation>()) }
991+
every { mock.remove(any()) } answers {
992+
val id = firstArg<String>()
993+
operationStoreList.removeIf { it.id == id }
994+
}
995+
mock
996+
}
997+
998+
val executor = mockk<IOperationExecutor>()
999+
every { executor.operations } returns listOf("DUMMY_OPERATION")
1000+
coEvery { executor.execute(any()) } returns ExecutionResponse(ExecutionResult.SUCCESS)
1001+
1002+
val operationRepo =
1003+
spyk(
1004+
OperationRepo(
1005+
listOf(executor),
1006+
operationModelStore,
1007+
configModelStore,
1008+
Time(),
1009+
getNewRecordState(configModelStore),
1010+
jwtTokenStore,
1011+
identityModelStore,
1012+
),
1013+
recordPrivateCalls = true,
1014+
)
1015+
1016+
val operation = mockOperation()
1017+
// externalId starts null — stampExternalId should fill it from the identity model
1018+
1019+
// When — enqueue then immediately switch user (simulating LogoutHelper's pattern)
1020+
operationRepo.enqueue(operation)
1021+
identityModel.externalId = null
1022+
1023+
// Then — the operation should have captured "old-user" before the switch
1024+
operation.externalId shouldBe "old-user"
1025+
}
1026+
9631027
test("FAIL_UNAUTHORIZED drops operations for anonymous user") {
9641028
// Given
9651029
val mocks = Mocks()

0 commit comments

Comments
 (0)