Merge pull request #2185 from OneSignal/fix/outcome_measure_zero_session_time_400_errors

Fix ending an already ended session

Author: jkasten2

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()

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<OutcomeEventParams> {
         val events: MutableList<OutcomeEventParams> = ArrayList()
         withContext(Dispatchers.IO) {
+            RemoveZeroSessionTimeRecords.run(_databaseProvider)
             _databaseProvider.os.query(OutcomeEventsTable.TABLE_NAME) { cursor ->
                 if (cursor.moveToFirst()) {
                     do {

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,
+        )
+    }
+}

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 }

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,14 +85,20 @@ 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
             session!!.sessionId = UUID.randomUUID().toString()
             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 {

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()) }
+    }
 })

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()
     }