From 3d3c5f2be94153109fd3d6b8785482502cb7c04c Mon Sep 17 00:00:00 2001 From: Josh Kasten Date: Tue, 27 Aug 2024 15:53:43 -0400 Subject: [PATCH 1/2] fix ending an already ended session # What Fixes an issue where invalid REST API requests were made to both outcomes/measure and User updates with session_time were bing made in the background. This is only present when location sharing is enabled in the SDK (in addition to the app and end-user allowing it) and the end-user opens the app at any point after receiving a notification. This resulted in 400 errors in the outcomes/measure case and no-op User updates that wasting resources, the later doesn't depend on receiving notifications. The outcomes/measure is also additive as the bad requests are cached and retried on app open. # How Add a state check to SessionService to prevent ending an already ended session. Also address a previous session never ending correctly when the app is cold started. Lastly, added comments pointing out the caveats of IBackgroundService to help prevent future wrong assumptions about when backgroundRun() is run. # Follow up Since there are cached invalid outcomes/measure requests a migration needs to be created in the SDK to remove them for apps who were affected by the bug. --- .../internal/background/IBackgroundService.kt | 13 ++++++---- .../session/internal/session/SessionModel.kt | 5 +++- .../internal/session/impl/SessionService.kt | 24 +++++++++++++++---- .../internal/session/SessionServiceTests.kt | 21 +++++++++++++--- .../background/LocationBackgroundService.kt | 4 ++++ 5 files changed, 55 insertions(+), 12 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/background/IBackgroundService.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/background/IBackgroundService.kt index 70ccbc4cb2..dee1e09d20 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/background/IBackgroundService.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/background/IBackgroundService.kt @@ -4,9 +4,9 @@ import androidx.annotation.WorkerThread /** * Implement and provide this interface as part of service registration to indicate the service - * wants to be instantiated and its [backgroundRun] function called when the app is in the background. The - * background process is initiated when the application is no longer in focus. Each background - * service's [scheduleBackgroundRunIn] will be analyzed to determine when [backgroundRun] should be called. + * wants to be instantiated and its [backgroundRun] function called when the app is in the background. + * Each background service's [scheduleBackgroundRunIn] will be analyzed to determine when + * [backgroundRun] should be called. */ interface IBackgroundService { /** @@ -16,7 +16,12 @@ interface IBackgroundService { val scheduleBackgroundRunIn: Long? /** - * Run the background service + * Run the background service. + * WARNING: This may not follow your scheduleBackgroundRunIn schedule: + * 1. May run more often as the lowest scheduleBackgroundRunIn + * value is used across the SDK. + * 2. Android doesn't guarantee exact timing on when the job is run, + * so it's possible for it to be delayed by a few minutes. */ @WorkerThread suspend fun backgroundRun() diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/SessionModel.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/SessionModel.kt index e49e96c26e..13c31ec50c 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/SessionModel.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/SessionModel.kt @@ -16,7 +16,10 @@ class SessionModel : Model() { } /** - * Whether the session is valid. + * Indicates if there is an active session. + * True when app is in the foreground. + * Also true in the background for a short period of time (default 30s) + * as a debouncing mechanism. */ var isValid: Boolean get() = getBooleanProperty(::isValid.name) { false } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/impl/SessionService.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/impl/SessionService.kt index d831ef54b1..a76425b9a3 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/impl/SessionService.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/impl/SessionService.kt @@ -47,18 +47,28 @@ internal class SessionService( private var config: ConfigModel? = null private var shouldFireOnSubscribe = false + // True if app has been foregrounded at least once since the app started + private var hasFocused = false + override fun start() { session = _sessionModelStore.model config = _configModelStore.model - // Reset the session validity property to drive a new session - session!!.isValid = false _applicationService.addApplicationLifecycleHandler(this) } + /** NOTE: This triggers more often than scheduleBackgroundRunIn defined above, + * as it runs on the lowest IBackgroundService.scheduleBackgroundRunIn across + * the SDK. + */ override suspend fun backgroundRun() { + endSession() + } + + private fun endSession() { + if (!session!!.isValid) return val activeDuration = session!!.activeDuration - // end the session Logging.debug("SessionService.backgroundRun: Session ended. activeDuration: $activeDuration") + session!!.isValid = false sessionLifeCycleNotifier.fire { it.onSessionEnded(activeDuration) } session!!.activeDuration = 0L @@ -75,6 +85,13 @@ internal class SessionService( */ override fun onFocus(firedOnSubscribe: Boolean) { Logging.log(LogLevel.DEBUG, "SessionService.onFocus() - fired from start: $firedOnSubscribe") + + // Treat app cold starts as a new session, we attempt to end any previous session to do this. + if (!hasFocused) { + hasFocused = true + endSession() + } + if (!session!!.isValid) { // As the old session was made inactive, we need to create a new session shouldFireOnSubscribe = firedOnSubscribe @@ -82,7 +99,6 @@ internal class SessionService( session!!.startTime = _time.currentTimeMillis session!!.focusTime = session!!.startTime session!!.isValid = true - Logging.debug("SessionService: New session started at ${session!!.startTime}") sessionLifeCycleNotifier.fire { it.onSessionStarted() } } else { diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/session/internal/session/SessionServiceTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/session/internal/session/SessionServiceTests.kt index 2a912d69e6..1daa409ac0 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/session/internal/session/SessionServiceTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/session/internal/session/SessionServiceTests.kt @@ -86,10 +86,10 @@ class SessionServiceTests : FunSpec({ // Then sessionModelStore.model.isValid shouldBe true - sessionModelStore.model.startTime shouldBe startTime + sessionModelStore.model.startTime shouldBe mocks.currentTime sessionModelStore.model.focusTime shouldBe mocks.currentTime - verify(exactly = 1) { mocks.spyCallback.onSessionActive() } - verify(exactly = 0) { mocks.spyCallback.onSessionStarted() } + verify(exactly = 0) { mocks.spyCallback.onSessionActive() } + verify(exactly = 1) { mocks.spyCallback.onSessionStarted() } } test("session active duration updated when unfocused") { @@ -140,4 +140,19 @@ class SessionServiceTests : FunSpec({ sessionModelStore.model.isValid shouldBe false verify(exactly = 1) { mocks.spyCallback.onSessionEnded(activeDuration) } } + + test("do not trigger onSessionEnd if session is not active") { + // Given + val mocks = Mocks() + mocks.sessionModelStore { it.isValid = false } + val sessionService = mocks.sessionService + sessionService.subscribe(mocks.spyCallback) + sessionService.start() + + // When + sessionService.backgroundRun() + + // Then + verify(exactly = 0) { mocks.spyCallback.onSessionEnded(any()) } + } }) diff --git a/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/background/LocationBackgroundService.kt b/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/background/LocationBackgroundService.kt index fcbb33ab9d..e0dc47fcbe 100644 --- a/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/background/LocationBackgroundService.kt +++ b/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/background/LocationBackgroundService.kt @@ -35,6 +35,10 @@ internal class LocationBackgroundService( return scheduleTime } + /** NOTE: This triggers more often than scheduleBackgroundRunIn defined above, + * as it runs on the lowest IBackgroundService.scheduleBackgroundRunIn across + * the SDK. + */ override suspend fun backgroundRun() { _capturer.captureLastLocation() } From 1ab8dcc76be3914d1e76d4c14d686940f779ff43 Mon Sep 17 00:00:00 2001 From: Josh Kasten Date: Tue, 27 Aug 2024 22:07:57 -0400 Subject: [PATCH 2/2] migration cleaning up invalid cached outcomes Clean up invalid cached os__session_duration outcome records with zero session_time produced in SDK versions 5.1.15 to 5.1.20 so we stop sending these requests to the backend. --- .../outcomes/impl/OutcomeEventsRepository.kt | 2 ++ .../RemoveZeroSessionTimeRecords.kt | 24 +++++++++++++++++++ 2 files changed, 26 insertions(+) create mode 100644 OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/outcomes/migrations/RemoveZeroSessionTimeRecords.kt diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/outcomes/impl/OutcomeEventsRepository.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/outcomes/impl/OutcomeEventsRepository.kt index c769f7d0ba..310824167b 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/outcomes/impl/OutcomeEventsRepository.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/outcomes/impl/OutcomeEventsRepository.kt @@ -8,6 +8,7 @@ import com.onesignal.session.internal.influence.Influence import com.onesignal.session.internal.influence.InfluenceChannel import com.onesignal.session.internal.influence.InfluenceType import com.onesignal.session.internal.influence.InfluenceType.Companion.fromString +import com.onesignal.session.internal.outcomes.migrations.RemoveZeroSessionTimeRecords import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.withContext import org.json.JSONArray @@ -101,6 +102,7 @@ internal class OutcomeEventsRepository( override suspend fun getAllEventsToSend(): List { val events: MutableList = ArrayList() withContext(Dispatchers.IO) { + RemoveZeroSessionTimeRecords.run(_databaseProvider) _databaseProvider.os.query(OutcomeEventsTable.TABLE_NAME) { cursor -> if (cursor.moveToFirst()) { do { diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/outcomes/migrations/RemoveZeroSessionTimeRecords.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/outcomes/migrations/RemoveZeroSessionTimeRecords.kt new file mode 100644 index 0000000000..93bab4d4a3 --- /dev/null +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/outcomes/migrations/RemoveZeroSessionTimeRecords.kt @@ -0,0 +1,24 @@ +package com.onesignal.session.internal.outcomes.migrations + +import com.onesignal.core.internal.database.IDatabaseProvider +import com.onesignal.session.internal.outcomes.impl.OutcomeEventsTable + +/** + * Purpose: Clean up invalid cached os__session_duration outcome records + * with zero session_time produced in SDK versions 5.1.15 to 5.1.20 so we stop + * sending these requests to the backend. + * + * Issue: SessionService.backgroundRun() didn't account for it being run more + * than one time in the background, when this happened it would create a + * outcome record with zero time which is invalid. + */ +object RemoveZeroSessionTimeRecords { + fun run(databaseProvider: IDatabaseProvider) { + databaseProvider.os.delete( + OutcomeEventsTable.TABLE_NAME, + OutcomeEventsTable.COLUMN_NAME_NAME + " = \"os__session_duration\"" + + " AND " + OutcomeEventsTable.COLUMN_NAME_SESSION_TIME + " = 0", + null, + ) + } +}