Skip to content

fix: [SDK-4388] subscription state permanently stuck at "Never Subscribed" when login() is called before requestPermission() #2627

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
abdulraqeeb33 wants to merge 3 commits into
base: main
Choose a base branch
from
+319 −11
Open

fix: [SDK-4388] subscription state permanently stuck at "Never Subscribed" when login() is called before requestPermission() #2627

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
731623f
fix(subscriptions): patch existing subscription instead of post
fadi-george Apr 23, 2026
3da01c0
refactor(subscriptions): move non-local id check earlier
fadi-george Apr 23, 2026
24a3623
test(subscriptions): add coverage for non-local id Create paths
Apr 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 32 additions & 3 deletions ...va/com/onesignal/user/internal/operations/impl/executors/SubscriptionOperationExecutor.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,14 @@ internal class SubscriptionOperationExecutor(
val startingOp = operations.first()

return if (startingOp is CreateSubscriptionOperation) {
createSubscription(startingOp, operations)
// SDK-4388: if the subscription already exists on the backend (non-local id),
// POSTing to /subscriptions with that id is silently treated as a no-op and
// drops the merged enabled/status payload. Dispatch as an update instead.
if (!IDManager.isLocalId(startingOp.subscriptionId)) {
updateExistingSubscriptionFromCreate(startingOp, operations)
} else {
createSubscription(startingOp, operations)
}
Comment on lines 56 to +64
} else if (operations.any { it is DeleteSubscriptionOperation }) {
if (operations.size > 1) {
throw Exception("Only supports one operation! Attempted operations:\n$operations")
Expand All @@ -73,6 +80,29 @@ internal class SubscriptionOperationExecutor(
}
}

private suspend fun updateExistingSubscriptionFromCreate(
createOperation: CreateSubscriptionOperation,
operations: List<Operation>,
): ExecutionResponse {
// if there are any deletes all operations should be tossed, nothing to do.
if (operations.any { it is DeleteSubscriptionOperation }) {
return ExecutionResponse(ExecutionResult.SUCCESS)
}

val lastUpdateOperation = operations.lastOrNull { it is UpdateSubscriptionOperation } as UpdateSubscriptionOperation?
val patchOp =
UpdateSubscriptionOperation(
createOperation.appId,
createOperation.onesignalId,
createOperation.subscriptionId,
createOperation.type,
lastUpdateOperation?.enabled ?: createOperation.enabled,
lastUpdateOperation?.address ?: createOperation.address,
lastUpdateOperation?.status ?: createOperation.status,
)
return updateSubscription(patchOp, listOf(patchOp))
Comment on lines +83 to +103
Copy link
Copy Markdown

@claude claude Bot Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Routing every CreateSubscriptionOperation with a non-local subscriptionId through PATCH breaks the existing 404/MISSING recovery flow in updateSubscription(): outside the missing-retry window, updateSubscription returns FAIL_NORETRY with a follow-up CreateSubscriptionOperation that re-uses the same non-local subscriptionId, which is now re-routed back through updateExistingSubscriptionFromCreate -> PATCH -> 404 -> ..., looping indefinitely with no escape. Suggested fix: catch BackendException(404) inside updateExistingSubscriptionFromCreate and fall back to createSubscription (POST) so the rebuild-user / FAIL_NORETRY recovery in createSubscription can terminate the cycle (or null out the id on the recovery op so it forces POST).

Extended reasoning...

Bug

The new updateExistingSubscriptionFromCreate helper unconditionally dispatches every CreateSubscriptionOperation whose subscriptionId is non-local through updateSubscription (PATCH /subscriptions/{id}). When that PATCH returns 404 outside the missing-retry window, updateSubscription (lines 235-260) returns FAIL_NORETRY with a recovery op:

ExecutionResponse(
    ExecutionResult.FAIL_NORETRY,
    operations = listOf(
        CreateSubscriptionOperation(
            lastOperation.appId,
            lastOperation.onesignalId,
            lastOperation.subscriptionId, // <-- same non-local id
            ...
        ),
    ),
)

OperationRepo.executeOperations (OperationRepo.kt:314-322) enqueues response.operations at the front of the queue regardless of the FAIL_NORETRY result. On the next pass, startingOp is CreateSubscriptionOperation and IDManager.isLocalId(...) is still false, so it re-enters updateExistingSubscriptionFromCreate -> PATCH -> 404 -> another CreateSubscriptionOperation with the same id... loop.

Step-by-step proof

  1. Operation queue contains CreateSubscriptionOperation(subscriptionId = "remote-uuid", ...) (e.g. server-side deletion happened after the local model cached the server uuid).
  2. execute() (line 56) sees startingOp is CreateSubscriptionOperation and !IDManager.isLocalId("remote-uuid") is true -> calls updateExistingSubscriptionFromCreate.
  3. The helper builds a patchOp and calls updateSubscription(patchOp, listOf(patchOp)).
  4. Backend returns 404. _newRecordState.isInMissingRetryWindow(...) is false (window expired or never opened). updateSubscription enters the MISSING branch and returns FAIL_NORETRY + a CreateSubscriptionOperation re-using lastOperation.subscriptionId = "remote-uuid".
  5. OperationRepo.executeOperations removes the original op (FAIL_NORETRY clears retries) and if (response.operations != null) enqueues the new CreateSubscriptionOperation("remote-uuid") at the front.
  6. Loop repeats from step 2. Each iteration produces a fresh CreateSubscriptionOperation UUID but the same non-local subscriptionId, and retries never accumulates because each cycle is FAIL_NORETRY. Backoff is whatever delayBeforeNextExecution(0, null) gives - effectively nothing.

Why pre-PR code did not have this problem

Pre-PR, the recovery CreateSubscriptionOperation went through createSubscription (POST). On 404, createSubscription's MISSING handler calls _buildUserService.getRebuildOperationsIfCurrentUser(...), which either bootstraps a rebuild (terminating the create cycle for this id) or returns null -> terminating FAIL_NORETRY. Either way the cycle ends. The new helper has no such fallback and no 404 handling at all - it just delegates to updateSubscription and lets the recovery op come back to the same branch.

Impact

Trigger scenarios are uncommon but plausible: server-side subscription deletion (admin/GDPR cleanup, manual REST delete, server lifecycle), or any case where the cached server uuid no longer exists on the backend. Consequence is unbounded queue churn - the SDK will hammer PATCH /subscriptions/{id} indefinitely with no client-side termination, draining battery and traffic until the process dies. There is no test exercising this path.

Fix

Either:

  • Catch BackendException with 404 in updateExistingSubscriptionFromCreate and fall through to createSubscription (POST), restoring the rebuild-user recovery path; or
  • In updateSubscription's 404 handler, set subscriptionId = null on the rebuilt CreateSubscriptionOperation so it is forced through the local-id POST branch on re-execution.

A unit test exercising PATCH-404-outside-missing-retry-window -> recovery should be added.

🔬 also observed by copilot-pr-reviewer

}

private suspend fun createSubscription(
createOperation: CreateSubscriptionOperation,
operations: List<Operation>,
Expand All @@ -88,12 +118,11 @@ internal class SubscriptionOperationExecutor(
val enabled = lastUpdateOperation?.enabled ?: createOperation.enabled
val address = lastUpdateOperation?.address ?: createOperation.address
val status = lastUpdateOperation?.status ?: createOperation.status
val subId = if (!IDManager.isLocalId(createOperation.subscriptionId)) createOperation.subscriptionId else null

try {
val subscription =
SubscriptionObject(
id = subId,
id = null,
Copy link
Copy Markdown
Contributor

@nan-li nan-li Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#2354

convert(createOperation.type),
address,
enabled,
Expand Down
295 changes: 287 additions & 8 deletions ...rc/test/java/com/onesignal/user/internal/operations/SubscriptionOperationExecutorTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,11 +105,14 @@ class SubscriptionOperationExecutorTests :
}
}

test("create subscription includes the subscription ID if non-local") {
// SDK-4388: A CreateSubscriptionOperation can carry a non-local subscriptionId when
// a subscription that already exists on the backend is re-attached to a new local user
// (e.g. createAndSwitchToNewUser after login). POSTing to /subscriptions with that id is
// silently a no-op on the server, so we must PATCH the existing subscription instead.
test("create subscription PATCHes existing subscription when subscription ID is non-local") {
// Given
val mockSubscriptionBackendService = mockk<ISubscriptionBackendService>()
coEvery { mockSubscriptionBackendService.createSubscription(any(), any(), any(), any()) } returns
Pair(remoteSubscriptionId, rywData)
coEvery { mockSubscriptionBackendService.updateSubscription(any(), any(), any()) } returns rywData

val mockSubscriptionsModelStore = mockk<SubscriptionModelStore>()
val subscriptionModel = SubscriptionModel()
Expand Down Expand Up @@ -144,16 +147,22 @@ class SubscriptionOperationExecutorTests :
)

// When
subscriptionOperationExecutor.execute(operations)
val response = subscriptionOperationExecutor.execute(operations)

// Then
response.result shouldBe ExecutionResult.SUCCESS
coVerify(exactly = 0) {
mockSubscriptionBackendService.createSubscription(any(), any(), any(), any())
}
coVerify(exactly = 1) {
mockSubscriptionBackendService.createSubscription(
mockSubscriptionBackendService.updateSubscription(
appId,
IdentityConstants.ONESIGNAL_ID,
remoteOneSignalId,
remoteSubscriptionId,
withArg {
it.id shouldBe remoteSubscriptionId
it.type shouldBe SubscriptionObjectType.ANDROID_PUSH
it.enabled shouldBe true
it.token shouldBe "pushToken"
it.notificationTypes shouldBe SubscriptionStatus.SUBSCRIBED.value
},
)
}
Expand Down Expand Up @@ -823,4 +832,274 @@ class SubscriptionOperationExecutorTests :
mockConsistencyManager.setRywData(remoteOneSignalId, IamFetchRywTokenKey.SUBSCRIPTION, rywData)
}
}

// Repro for SDK-4388: when a CreateSubscriptionOperation is grouped with a follow-up
// UpdateSubscriptionOperation for a subscription that already exists on the server
// (non-local subscriptionId), the executor must PATCH the merged final state instead
// of POSTing (which the backend treats as a no-op, silently dropping enabled/status).
test("create subscription with non-local id PATCHes instead of POSTing when grouped with update") {
// Given
val mockSubscriptionBackendService = mockk<ISubscriptionBackendService>()
coEvery { mockSubscriptionBackendService.updateSubscription(any(), any(), any()) } returns rywData

val mockSubscriptionsModelStore = mockk<SubscriptionModelStore>()
val subscriptionModel = SubscriptionModel().apply { id = remoteSubscriptionId }
every { mockSubscriptionsModelStore.get(remoteSubscriptionId) } returns subscriptionModel

val mockBuildUserService = mockk<IRebuildUserService>()

val subscriptionOperationExecutor =
SubscriptionOperationExecutor(
mockSubscriptionBackendService,
MockHelper.deviceService(),
AndroidMockHelper.applicationService(),
mockSubscriptionsModelStore,
MockHelper.configModelStore(),
mockBuildUserService,
getNewRecordState(),
mockConsistencyManager,
)

val operations =
listOf<Operation>(
CreateSubscriptionOperation(
appId,
remoteOneSignalId,
remoteSubscriptionId,
SubscriptionType.PUSH,
false,
"pushToken",
SubscriptionStatus.NO_PERMISSION,
),
UpdateSubscriptionOperation(
appId,
remoteOneSignalId,
remoteSubscriptionId,
SubscriptionType.PUSH,
true,
"pushToken",
SubscriptionStatus.SUBSCRIBED,
),
)

// When
val response = subscriptionOperationExecutor.execute(operations)

// Then
response.result shouldBe ExecutionResult.SUCCESS
coVerify(exactly = 0) {
mockSubscriptionBackendService.createSubscription(any(), any(), any(), any())
}
coVerify(exactly = 1) {
mockSubscriptionBackendService.updateSubscription(
appId,
remoteSubscriptionId,
withArg {
it.type shouldBe SubscriptionObjectType.ANDROID_PUSH
it.enabled shouldBe true
it.token shouldBe "pushToken"
it.notificationTypes shouldBe SubscriptionStatus.SUBSCRIBED.value
},
)
}
}

// SDK-4388: when a CreateSubscriptionOperation with a non-local id is grouped with a
// DeleteSubscriptionOperation, the executor must short-circuit to SUCCESS without
// making any backend calls. Creating-then-deleting an already-existing subscription is
// a no-op, and a stray PATCH here would resurrect a subscription the caller intended
// to remove.
test("create subscription with non-local id then delete is a successful no-op") {
// Given
val mockSubscriptionBackendService = mockk<ISubscriptionBackendService>()

val mockSubscriptionsModelStore = mockk<SubscriptionModelStore>()
val subscriptionModel = SubscriptionModel().apply { id = remoteSubscriptionId }
every { mockSubscriptionsModelStore.get(remoteSubscriptionId) } returns subscriptionModel

val mockBuildUserService = mockk<IRebuildUserService>()

val subscriptionOperationExecutor =
SubscriptionOperationExecutor(
mockSubscriptionBackendService,
MockHelper.deviceService(),
AndroidMockHelper.applicationService(),
mockSubscriptionsModelStore,
MockHelper.configModelStore(),
mockBuildUserService,
getNewRecordState(),
mockConsistencyManager,
)

val operations =
listOf<Operation>(
CreateSubscriptionOperation(
appId,
remoteOneSignalId,
remoteSubscriptionId,
SubscriptionType.PUSH,
true,
"pushToken",
SubscriptionStatus.SUBSCRIBED,
),
DeleteSubscriptionOperation(appId, remoteOneSignalId, remoteSubscriptionId),
)

// When
val response = subscriptionOperationExecutor.execute(operations)

// Then
response.result shouldBe ExecutionResult.SUCCESS
coVerify(exactly = 0) {
mockSubscriptionBackendService.createSubscription(any(), any(), any(), any())
}
coVerify(exactly = 0) {
mockSubscriptionBackendService.updateSubscription(any(), any(), any())
}
coVerify(exactly = 0) {
mockSubscriptionBackendService.deleteSubscription(any(), any())
}
}

// SDK-4388: when the PATCH dispatched for a non-local-id Create fails with a retryable
// network error, the executor must propagate FAIL_RETRY (and any retryAfterSeconds) so
// OperationRepo can re-execute, rather than swallowing the error or routing back to a
// POST that the backend would silently no-op.
test("create subscription with non-local id fails with retry when there is a network condition") {
// Given
val mockSubscriptionBackendService = mockk<ISubscriptionBackendService>()
coEvery { mockSubscriptionBackendService.updateSubscription(any(), any(), any()) } throws
BackendException(408, retryAfterSeconds = 10)

val mockSubscriptionsModelStore = mockk<SubscriptionModelStore>()
val mockBuildUserService = mockk<IRebuildUserService>()

val subscriptionOperationExecutor =
SubscriptionOperationExecutor(
mockSubscriptionBackendService,
MockHelper.deviceService(),
AndroidMockHelper.applicationService(),
mockSubscriptionsModelStore,
MockHelper.configModelStore(),
mockBuildUserService,
getNewRecordState(),
mockConsistencyManager,
)

val operations =
listOf<Operation>(
CreateSubscriptionOperation(
appId,
remoteOneSignalId,
remoteSubscriptionId,
SubscriptionType.PUSH,
true,
"pushToken",
SubscriptionStatus.SUBSCRIBED,
),
)

// When
val response = subscriptionOperationExecutor.execute(operations)

// Then
response.result shouldBe ExecutionResult.FAIL_RETRY
response.retryAfterSeconds shouldBe 10
coVerify(exactly = 0) {
mockSubscriptionBackendService.createSubscription(any(), any(), any(), any())
}
coVerify(exactly = 1) {
mockSubscriptionBackendService.updateSubscription(
appId,
remoteSubscriptionId,
withArg {
it.type shouldBe SubscriptionObjectType.ANDROID_PUSH
it.enabled shouldBe true
it.token shouldBe "pushToken"
it.notificationTypes shouldBe SubscriptionStatus.SUBSCRIBED.value
},
)
}
}

// SDK-4388: when multiple UpdateSubscriptionOperations are batched with a non-local-id
// Create, the executor must collapse them into a single PATCH carrying the *last*
// update's values (not the create's, not the first update's). Locks in last-wins
// semantics so a future "merge updates" refactor can't silently change behavior on
// this path and re-introduce stale state on the backend.
test("create subscription with non-local id and multiple updates uses the last update's values") {
// Given
val mockSubscriptionBackendService = mockk<ISubscriptionBackendService>()
coEvery { mockSubscriptionBackendService.updateSubscription(any(), any(), any()) } returns rywData

val mockSubscriptionsModelStore = mockk<SubscriptionModelStore>()
val subscriptionModel = SubscriptionModel().apply { id = remoteSubscriptionId }
every { mockSubscriptionsModelStore.get(remoteSubscriptionId) } returns subscriptionModel

val mockBuildUserService = mockk<IRebuildUserService>()

val subscriptionOperationExecutor =
SubscriptionOperationExecutor(
mockSubscriptionBackendService,
MockHelper.deviceService(),
AndroidMockHelper.applicationService(),
mockSubscriptionsModelStore,
MockHelper.configModelStore(),
mockBuildUserService,
getNewRecordState(),
mockConsistencyManager,
)

val operations =
listOf<Operation>(
CreateSubscriptionOperation(
appId,
remoteOneSignalId,
remoteSubscriptionId,
SubscriptionType.PUSH,
false,
"pushTokenCreate",
SubscriptionStatus.NO_PERMISSION,
),
UpdateSubscriptionOperation(
appId,
remoteOneSignalId,
remoteSubscriptionId,
SubscriptionType.PUSH,
false,
"pushTokenIntermediate",
SubscriptionStatus.NO_PERMISSION,
),
UpdateSubscriptionOperation(
appId,
remoteOneSignalId,
remoteSubscriptionId,
SubscriptionType.PUSH,
true,
"pushTokenFinal",
SubscriptionStatus.SUBSCRIBED,
),
)

// When
val response = subscriptionOperationExecutor.execute(operations)

// Then
response.result shouldBe ExecutionResult.SUCCESS
coVerify(exactly = 0) {
mockSubscriptionBackendService.createSubscription(any(), any(), any(), any())
}
coVerify(exactly = 1) {
mockSubscriptionBackendService.updateSubscription(
appId,
remoteSubscriptionId,
withArg {
it.type shouldBe SubscriptionObjectType.ANDROID_PUSH
it.enabled shouldBe true
it.token shouldBe "pushTokenFinal"
it.notificationTypes shouldBe SubscriptionStatus.SUBSCRIBED.value
},
)
}
}
})
Loading