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.

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