Skip to content

fix: resolve pre-existing test failures on main #2605

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
Open

fix: resolve pre-existing test failures on main #2605

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
c4eb74b
fix: resolve pre-existing test failures on main
Apr 7, 2026
f1a719d
chore: remove unused awaitIO import (Spotless)
Apr 9, 2026
1b4c0b2
fix(test): stabilize prevent-default-twice notification generation tests
Apr 9, 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
66 changes: 38 additions & 28 deletions OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ internal class OneSignalImp(
private val ioDispatcher: CoroutineDispatcher = Dispatchers.IO,
) : IOneSignal, IServiceProvider {

Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

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

suspendCompletion is now a mutable shared reference. Since internalInit sets initState = FAILED and only then calls notifyInitComplete(), another thread can start a new init attempt in between (FAILED is not "accessible"), replacing suspendCompletion. The first init attempt would then complete the new deferred, unblocking waiters for the second init prematurely. Consider keeping a per-init local CompletableDeferred (capture it in the launched block and complete that instance) and/or synchronizing state transitions + completion signaling under initLock to avoid cross-init races.

Suggested change
private val initLock = Any()

Copilot uses AI. Check for mistakes.
private val suspendCompletion = CompletableDeferred<Unit>()
@Volatile
private var suspendCompletion = CompletableDeferred<Unit>()

@Volatile
private var initState: InitState = InitState.NOT_STARTED
Expand Down Expand Up @@ -288,34 +289,30 @@ internal class OneSignalImp(
context: Context,
appId: String,
): Boolean {
Logging.log(LogLevel.DEBUG, "Calling deprecated initWithContextSuspend(context: $context, appId: $appId)")
Logging.log(LogLevel.DEBUG, "initWithContext(context: $context, appId: $appId)")

// do not do this again if already initialized or init is in progress
synchronized(initLock) {
if (initState.isSDKAccessible()) {
Logging.log(LogLevel.DEBUG, "initWithContext: SDK already initialized or in progress")
return true
}

initState = InitState.IN_PROGRESS
suspendCompletion = CompletableDeferred()
}

initFailureException = IllegalStateException("OneSignal initWithContext failed.")

Comment on lines 300 to +305
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

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

initState is set to IN_PROGRESS inside the initLock section, but initFailureException is assigned immediately after the lock is released. There’s a race where another thread can observe IN_PROGRESS, wait for init, and (on failure) throw a generic exception because initFailureException is still null. To make failure reporting deterministic, set initFailureException inside the same synchronized(initLock) block where initState/suspendCompletion are updated (or otherwise ensure it’s assigned-before publishing IN_PROGRESS).

Suggested change
initState = InitState.IN_PROGRESS
suspendCompletion = CompletableDeferred()
}
initFailureException = IllegalStateException("OneSignal initWithContext failed.")
initFailureException = IllegalStateException("OneSignal initWithContext failed.")
initState = InitState.IN_PROGRESS
suspendCompletion = CompletableDeferred()
}

Copilot uses AI. Check for mistakes.
// FeatureManager depends on ConfigModelStore/PreferencesService which requires appContext.
// Ensure app context is available before evaluating feature gates.
ensureApplicationServiceStarted(context)

if (isBackgroundThreadingEnabled) {
// init in background and return immediately to ensure non-blocking
suspendifyOnIO {
internalInit(context, appId)
}
return true
}

// Legacy FF-OFF behavior intentionally blocks caller thread until initialization completes.
return runBlocking(runtimeIoDispatcher) {
// Always dispatch init asynchronously so this method never blocks the caller.
// Callers that need to wait (accessors, login, logout) will block via suspendCompletion.
suspendifyOnIO {
internalInit(context, appId)
}
Comment on lines 300 to 314
Copy link
Copy Markdown

@claude claude Bot Apr 7, 2026

Choose a reason for hiding this comment

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

🔴 If ensureApplicationServiceStarted throws between setting initState=IN_PROGRESS and dispatching suspendifyOnIO, suspendCompletion is never completed and initState stays permanently IN_PROGRESS. Any subsequent accessor call hits the new IN_PROGRESS -> waitAndReturn() branch in getServiceWithFeatureGate, calls suspendCompletion.await(), and deadlocks forever. Fix: wrap the ensureApplicationServiceStarted + suspendifyOnIO block in a try/catch that sets initState = FAILED and calls notifyInitComplete() on any exception.

Extended reasoning...

What the bug is and how it manifests

In the new initWithContext(context, appId), initState is set to IN_PROGRESS and suspendCompletion is replaced with a fresh CompletableDeferred() inside synchronized(initLock). Immediately after the lock is released, ensureApplicationServiceStarted(context) is called on the caller thread -- before suspendifyOnIO is dispatched. If that call throws, the exception propagates out of initWithContext with no cleanup: initState stays IN_PROGRESS and the brand-new, never-completed suspendCompletion is abandoned.

The specific code path that triggers it

ensureApplicationServiceStarted calls (applicationService as ApplicationService).start(context), which casts context.applicationContext to Application (ApplicationService.kt ~line 81). In non-standard host environments (instrumentation tests that wrap the context, Robolectric with custom application factories, multi-process scenarios, etc.) this cast can throw ClassCastException. registerActivityLifecycleCallbacks called inside start() can also throw in restricted environments. Any unchecked runtime exception escaping ensureApplicationServiceStarted triggers the bug.

Why existing code does not prevent it

There is no try/catch around the gap between the synchronized(initLock) block (which writes initState = IN_PROGRESS and creates a new suspendCompletion) and the suspendifyOnIO { internalInit(...) } dispatch. After the throw: isSDKAccessible() returns true for IN_PROGRESS (InitState.kt lines 36-38), so every subsequent initWithContext call returns true immediately without retrying. Meanwhile getServiceWithFeatureGate now has an explicit IN_PROGRESS -> waitAndReturn(getter) branch (added by this PR) which calls waitForInit() -> runBlocking { suspendCompletion.await() } -- a deferred that will never be completed.

What the impact is

Every accessor property (user, notifications, session, location, inAppMessages) and every non-suspend call to login/logout blocks the calling thread forever, producing an ANR if called from the main thread. The application cannot recover: retrying initWithContext is a no-op (isSDKAccessible() returns true for IN_PROGRESS), and there is no API to reset state. The previous runBlocking-based FF-OFF path would have surfaced the exception to the caller so it could be caught and handled. This PR removes that last safety net by making init always-async.

Step-by-step proof

  1. App calls OneSignal.initWithContext(ctx, appId) where ctx.applicationContext is not an Application subclass.
  2. synchronized(initLock): initState = IN_PROGRESS, suspendCompletion = CompletableDeferred() (uncompleted).
  3. ensureApplicationServiceStarted(ctx) calls ApplicationService.start(ctx) which casts ctx.applicationContext as Application -- throws ClassCastException -- propagates out of initWithContext.
  4. suspendifyOnIO { internalInit(...) } is never reached; notifyInitComplete() is never called.
  5. initState is still IN_PROGRESS; isSDKAccessible() returns true.
  6. App calls OneSignal.notifications.permission -- getServiceWithFeatureGate -- IN_PROGRESS branch -- waitAndReturn(getter) -- runBlocking { suspendCompletion.await() } -- blocks forever (ANR on main thread).
  7. App retries OneSignal.initWithContext(ctx2, appId) -- synchronized(initLock) sees isSDKAccessible() == true -- returns true immediately -- initialization is never retried.

How to fix it

Wrap the call to ensureApplicationServiceStarted and the suspendifyOnIO dispatch in a try/catch block. In the catch handler: set initState = InitState.FAILED, assign initFailureException = e, and call notifyInitComplete() so any waiting accessor unblocks and rethrows the exception instead of hanging forever:

try {
    ensureApplicationServiceStarted(context)
    suspendifyOnIO { internalInit(context, appId) }
} catch (e: Exception) {
    initFailureException = e
    initState = InitState.FAILED
    notifyInitComplete()
    throw e
}

return true
}

/**
Expand Down Expand Up @@ -380,9 +377,7 @@ internal class OneSignalImp(
waitForInit(operationName = "login")
suspendifyOnIO { loginHelper.login(externalId, jwtBearerToken) }
} else {
if (!isInitialized) {
throw IllegalStateException("Must call 'initWithContext' before 'login'")
}
requireInitForOperation("login")
Thread {
runBlocking(runtimeIoDispatcher) {
loginHelper.login(externalId, jwtBearerToken)
Expand All @@ -398,9 +393,7 @@ internal class OneSignalImp(
waitForInit(operationName = "logout")
suspendifyOnIO { logoutHelper.logout() }
} else {
if (!isInitialized) {
throw IllegalStateException("Must call 'initWithContext' before 'logout'")
}
requireInitForOperation("logout")
Thread {
runBlocking(runtimeIoDispatcher) {
logoutHelper.logout()
Expand All @@ -417,6 +410,22 @@ internal class OneSignalImp(

override fun <T> getAllServices(c: Class<T>): List<T> = services.getAllServices(c)

/**
* Ensures initialization is complete before proceeding with an operation.
* Blocks if init is in progress; throws immediately if not started or failed.
*/
private fun requireInitForOperation(operationName: String) {
when (initState) {
InitState.NOT_STARTED ->
throw IllegalStateException("Must call 'initWithContext' before '$operationName'")
InitState.IN_PROGRESS -> waitForInit(operationName = operationName)
InitState.FAILED ->
throw initFailureException
?: IllegalStateException("Initialization failed before '$operationName'")
InitState.SUCCESS -> {}
}
}

/**
* Blocking version that waits for initialization to complete.
* Uses runBlocking to bridge to the suspend implementation.
Expand Down Expand Up @@ -514,14 +523,15 @@ internal class OneSignalImp(
}

private fun <T> getServiceWithFeatureGate(getter: () -> T): T {
return if (isBackgroundThreadingEnabled) {
waitAndReturn(getter)
} else {
if (isInitialized) {
getter()
} else {
throw IllegalStateException("Must call 'initWithContext' before use")
}
if (isBackgroundThreadingEnabled) {
return waitAndReturn(getter)
}
return when (initState) {
InitState.SUCCESS -> getter()
InitState.IN_PROGRESS -> waitAndReturn(getter)
InitState.FAILED -> throw initFailureException
?: IllegalStateException("Initialization failed. Cannot proceed.")
Comment on lines +529 to +533
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

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

In legacy mode (!isBackgroundThreadingEnabled), InitState.IN_PROGRESS now blocks via waitAndReturn(getter) instead of failing fast. Given waitForInit() uses runBlocking, calling an accessor on the main thread immediately after initWithContext() can now block the UI thread indefinitely (ANR risk), whereas previously legacy mode threw if not initialized. If this behavior change is required, consider adding a main-thread guard (throw with guidance to use the suspend API / call from background) or otherwise making the blocking behavior explicit and safe.

Copilot uses AI. Check for mistakes.
InitState.NOT_STARTED -> throw IllegalStateException("Must call 'initWithContext' before use")
}
}

Expand Down Expand Up @@ -626,10 +636,10 @@ internal class OneSignalImp(
}

initState = InitState.IN_PROGRESS
suspendCompletion = CompletableDeferred()
}

val result = internalInit(context, appId)
// initState is already set correctly in internalInit, no need to overwrite it
result
}
}
Expand Down
4 changes: 4 additions & 0 deletions .../onesignal/core/src/test/java/com/onesignal/core/internal/features/FeatureManagerTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ class FeatureManagerTests : FunSpec({
ThreadingMode.useBackgroundThreading = false
}

afterEach {
ThreadingMode.useBackgroundThreading = false
}

test("initial state should enable BACKGROUND_THREADING when feature is present") {
// Given
val initialModel = mockk<ConfigModel>()
Expand Down
11 changes: 5 additions & 6 deletions ...a/com/onesignal/notifications/internal/generation/NotificationGenerationProcessorTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import com.onesignal.common.threading.suspendifyOnIO
import com.onesignal.debug.LogLevel
import com.onesignal.debug.internal.logging.Logging
import com.onesignal.mocks.AndroidMockHelper
import com.onesignal.mocks.IOMockHelper
import com.onesignal.mocks.MockHelper
import com.onesignal.notifications.INotificationReceivedEvent
import com.onesignal.notifications.INotificationWillDisplayEvent
Expand Down Expand Up @@ -96,6 +97,8 @@ private class Mocks {
}

class NotificationGenerationProcessorTests : FunSpec({
listener(IOMockHelper)

beforeAny {
Logging.logLevel = LogLevel.NONE

Expand Down Expand Up @@ -285,10 +288,9 @@ class NotificationGenerationProcessorTests : FunSpec({
val willDisplayEvent = firstArg<INotificationWillDisplayEvent>()
willDisplayEvent.preventDefault(false)
suspendifyOnIO {
delay(100)
// Second preventDefault(true) wakes the waiter with false; avoid notification.display()
// which would wake(true) and overwrite the conflated channel (CI flake on fast runners).
willDisplayEvent.preventDefault(true)
delay(100)
willDisplayEvent.notification.display()
}
}

Expand All @@ -309,10 +311,7 @@ class NotificationGenerationProcessorTests : FunSpec({
val receivedEvent = firstArg<INotificationReceivedEvent>()
receivedEvent.preventDefault(false)
suspendifyOnIO {
delay(100)
receivedEvent.preventDefault(true)
delay(100)
receivedEvent.notification.display()
}
}

Expand Down
9 changes: 8 additions & 1 deletion OneSignalSDK/onesignal/testhelpers/src/main/java/com/onesignal/mocks/IOMockHelper.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package com.onesignal.mocks

import com.onesignal.common.threading.OneSignalDispatchers
import com.onesignal.common.threading.suspendifyOnIO
import com.onesignal.common.threading.suspendifyOnMain
import io.kotest.core.listeners.AfterSpecListener
import io.kotest.core.listeners.BeforeSpecListener
import io.kotest.core.listeners.BeforeTestListener
Expand Down Expand Up @@ -30,7 +31,8 @@ import java.util.concurrent.atomic.AtomicInteger
* (e.g., delay(50)) to wait for async work to finish.
*
* This helper avoids that by:
* - Mocking `suspendifyOnIO`, `launchOnIO`, and `launchOnDefault` so their blocks run immediately
* - Mocking `suspendifyOnIO`, `suspendifyOnMain`, and `OneSignalDispatchers.launchOnIO` /
* `launchOnDefault` so their blocks run immediately
* - Completing a `CompletableDeferred` when the async block finishes
* - Providing `awaitIO()` so tests can explicitly wait for all async work without sleeps
*
Expand Down Expand Up @@ -114,6 +116,11 @@ object IOMockHelper : BeforeSpecListener, AfterSpecListener, BeforeTestListener,
trackAsyncWork(block)
}

every { suspendifyOnMain(any<suspend () -> Unit>()) } answers {
val block = firstArg<suspend () -> Unit>()
trackAsyncWork(block)
}
Comment on lines +119 to +122
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

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

This mock makes suspendifyOnMain execute via trackAsyncWork, but trackAsyncWork currently catches Exception and neither rethrows nor records it (the comment says it logs, but it doesn’t). That means exceptions thrown inside a fireOnMain callback can be silently swallowed and the test may still pass. Consider capturing the exception and failing the test (e.g., complete the waiter exceptionally / rethrow on awaitIO()), or at least logging so failures aren’t hidden.

Copilot uses AI. Check for mistakes.

every { OneSignalDispatchers.launchOnIO(any<suspend () -> Unit>()) } answers {
val block = firstArg<suspend () -> Unit>()
trackAsyncWork(block)
Expand Down
Loading