Always start a new session span after a manual session end (#1167)

## Goal

Manual session ends only started a new session span if background activity is enabled. This is because there's ambiguity in how we decided whether we should new session span.

To address this, I've consolidated the decision in the `FinalEvelopeParam` definition and removed it from the `SessionSnapshotType`, what is defined in FinalEvelopeParam is the ultimate arbitor.

## Testing

Added additional testing to verify the correctness at the PayloadFactory level as well as the PayloadMessageCollator level.

embrace-android-core/src/main/kotlin/io/embrace/android/embracesdk/internal/session/message/FinalEnvelopeParams.kt

@@ -12,7 +12,7 @@ public class FinalEnvelopeParams(
     public val initial: SessionZygote,
     public val endType: SessionSnapshotType,
     public val logger: EmbLogger,
-    backgroundActivityEnabled: Boolean,
+    continueMonitoring: Boolean,
     crashId: String? = null,
 ) {
 
@@ -21,5 +21,5 @@ public class FinalEnvelopeParams(
         else -> crashId
     }
 
-    public val startNewSession: Boolean = endType.shouldStartNewSession && backgroundActivityEnabled
+    public val startNewSession: Boolean = continueMonitoring
 }

embrace-android-core/src/main/kotlin/io/embrace/android/embracesdk/internal/session/orchestrator/SessionSnapshotType.kt

@@ -14,26 +14,21 @@ public enum class SessionSnapshotType(
      * Whether the session process experienced a force quit/unexpected termination.
      */
     public val forceQuit: Boolean,
-
-    /**
-     * Whether this type of end should lead to the start of a new session
-     */
-    public val shouldStartNewSession: Boolean
 ) {
 
     /**
      * The end session happened in the normal way (i.e. process state changes or manual/timed end).
      */
-    NORMAL_END(endedCleanly = true, forceQuit = false, shouldStartNewSession = true),
+    NORMAL_END(endedCleanly = true, forceQuit = false),
 
     /**
      * The end session is being constructed so that it can be periodically cached. This avoids
      * the scenario of data loss in the event of NDK crashes.
      */
-    PERIODIC_CACHE(endedCleanly = false, forceQuit = true, shouldStartNewSession = false),
+    PERIODIC_CACHE(endedCleanly = false, forceQuit = true),
 
     /**
      * The end session is being constructed because of a JVM crash.
      */
-    JVM_CRASH(endedCleanly = false, forceQuit = false, shouldStartNewSession = false)
+    JVM_CRASH(endedCleanly = false, forceQuit = false)
 }

embrace-android-sdk/src/main/java/io/embrace/android/embracesdk/internal/session/message/PayloadFactoryImpl.kt

@@ -61,7 +61,7 @@ internal class PayloadFactoryImpl(
                 initial = initial,
                 endType = SessionSnapshotType.NORMAL_END,
                 logger = logger,
-                backgroundActivityEnabled = isBackgroundActivityEnabled(),
+                continueMonitoring = true,
             )
         )
     }
@@ -104,7 +104,7 @@ internal class PayloadFactoryImpl(
                 initial = initial,
                 endType = SessionSnapshotType.NORMAL_END,
                 logger = logger,
-                backgroundActivityEnabled = isBackgroundActivityEnabled(),
+                continueMonitoring = isBackgroundActivityEnabled(),
             )
         )
     }
@@ -121,7 +121,7 @@ internal class PayloadFactoryImpl(
                 initial = initial,
                 endType = SessionSnapshotType.NORMAL_END,
                 logger = logger,
-                backgroundActivityEnabled = true,
+                continueMonitoring = true,
             )
         )
     }
@@ -135,7 +135,7 @@ internal class PayloadFactoryImpl(
                 initial = initial,
                 endType = SessionSnapshotType.JVM_CRASH,
                 logger = logger,
-                backgroundActivityEnabled = isBackgroundActivityEnabled(),
+                continueMonitoring = false,
                 crashId = crashId
             )
         )
@@ -153,7 +153,7 @@ internal class PayloadFactoryImpl(
                 initial = initial,
                 endType = SessionSnapshotType.JVM_CRASH,
                 logger = logger,
-                backgroundActivityEnabled = true,
+                continueMonitoring = false,
                 crashId = crashId
             )
         )
@@ -168,7 +168,7 @@ internal class PayloadFactoryImpl(
                 initial = initial,
                 endType = SessionSnapshotType.PERIODIC_CACHE,
                 logger = logger,
-                backgroundActivityEnabled = isBackgroundActivityEnabled()
+                continueMonitoring = true
             )
         )
     }
@@ -182,7 +182,7 @@ internal class PayloadFactoryImpl(
                 initial = initial,
                 endType = SessionSnapshotType.PERIODIC_CACHE,
                 logger = logger,
-                backgroundActivityEnabled = true
+                continueMonitoring = true
             )
         )
     }

embrace-android-sdk/src/test/java/io/embrace/android/embracesdk/fakes/FakeSessionPayloadSource.kt

@@ -7,9 +7,14 @@ import io.embrace.android.embracesdk.internal.session.orchestrator.SessionSnapsh
 internal class FakeSessionPayloadSource : SessionPayloadSource {
 
     var sessionPayload: SessionPayload = SessionPayload()
+    var lastStartNewSession: Boolean? = null
+
     override fun getSessionPayload(
         endType: SessionSnapshotType,
         startNewSession: Boolean,
         crashId: String?
-    ) = sessionPayload
+    ): SessionPayload {
+        lastStartNewSession = startNewSession
+        return SessionPayload()
+    }
 }

embrace-android-sdk/src/test/java/io/embrace/android/embracesdk/session/message/PayloadFactoryImplTest.kt

@@ -8,10 +8,13 @@ import io.embrace.android.embracesdk.fakes.FakePreferenceService
 import io.embrace.android.embracesdk.fakes.FakeSessionPayloadSource
 import io.embrace.android.embracesdk.fakes.injection.FakeInitModule
 import io.embrace.android.embracesdk.internal.envelope.session.SessionEnvelopeSourceImpl
+import io.embrace.android.embracesdk.internal.session.lifecycle.ProcessState
 import io.embrace.android.embracesdk.internal.session.lifecycle.ProcessState.BACKGROUND
 import io.embrace.android.embracesdk.internal.session.lifecycle.ProcessState.FOREGROUND
 import io.embrace.android.embracesdk.internal.session.message.PayloadFactoryImpl
 import io.embrace.android.embracesdk.internal.session.message.PayloadMessageCollatorImpl
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertNotNull
 import org.junit.Assert.assertNull
 import org.junit.Assert.assertTrue
 import org.junit.Before
@@ -20,20 +23,22 @@ import org.junit.Test
 internal class PayloadFactoryImplTest {
 
     private lateinit var configService: FakeConfigService
+    private lateinit var sessionPayloadSource: FakeSessionPayloadSource
     private lateinit var factory: PayloadFactoryImpl
 
     @Before
     fun setUp() {
-        configService = FakeConfigService()
         val initModule = FakeInitModule()
+        configService = FakeConfigService()
+        sessionPayloadSource = FakeSessionPayloadSource()
         val collator = PayloadMessageCollatorImpl(
             gatingService = FakeGatingService(),
             preferencesService = FakePreferenceService(),
             currentSessionSpan = initModule.openTelemetryModule.currentSessionSpan,
             sessionEnvelopeSource = SessionEnvelopeSourceImpl(
                 FakeEnvelopeMetadataSource(),
                 FakeEnvelopeResourceSource(),
-                FakeSessionPayloadSource()
+                sessionPayloadSource
             )
         )
         factory = PayloadFactoryImpl(
@@ -50,29 +55,36 @@ internal class PayloadFactoryImplTest {
     }
 
     @Test
-    fun `start payload with state generate payloads with valid session IDs if ba is enabled`() {
+    fun `verify expected payloads with ba enabled`() {
         configService.backgroundActivityCaptureEnabled = true
-        assertTrue(checkNotNull(factory.startPayloadWithState(FOREGROUND, 0, false)).sessionId.isNotBlank())
-        assertTrue(checkNotNull(factory.startPayloadWithState(BACKGROUND, 0, false)).sessionId.isNotBlank())
+        verifyPayloadWithState(state = FOREGROUND, zygoteCreated = true, startNewSession = true)
+        verifyPayloadWithState(state = BACKGROUND, zygoteCreated = true, startNewSession = true)
+        verifyPayloadWithManual()
     }
 
     @Test
-    fun `start payload with state generate payloads with valid session IDs only for foreground if ba is disabled`() {
+    fun `verify expected payloads with ba disabled`() {
         configService.backgroundActivityCaptureEnabled = false
-        assertTrue(checkNotNull(factory.startPayloadWithState(FOREGROUND, 0, false)).sessionId.isNotBlank())
-        assertNull(factory.startPayloadWithState(BACKGROUND, 0, false))
+        verifyPayloadWithState(state = FOREGROUND, zygoteCreated = true, startNewSession = false)
+        verifyPayloadWithState(state = BACKGROUND, zygoteCreated = false, startNewSession = false)
+        verifyPayloadWithManual()
     }
 
-    @Test
-    fun `start payload with manual generate payloads with valid session IDs if ba is enabled`() {
-        configService.backgroundActivityCaptureEnabled = true
-        assertTrue(checkNotNull(factory.startSessionWithManual(100L)).sessionId.isNotBlank())
-        assertTrue(checkNotNull(factory.startPayloadWithState(BACKGROUND, 0, false)).sessionId.isNotBlank())
+    private fun verifyPayloadWithState(state: ProcessState, zygoteCreated: Boolean, startNewSession: Boolean) {
+        val zygote = factory.startPayloadWithState(state, 0, false)
+        if (zygoteCreated) {
+            assertTrue(checkNotNull(zygote).sessionId.isNotBlank())
+            assertNotNull(factory.endPayloadWithState(state, 0, zygote))
+            assertEquals(startNewSession, sessionPayloadSource.lastStartNewSession)
+        } else {
+            assertNull(zygote)
+        }
     }
 
-    @Test
-    fun `start payload with manual generate payloads with valid session IDs if ba is disabled`() {
-        configService.backgroundActivityCaptureEnabled = false
-        assertTrue(checkNotNull(factory.startSessionWithManual(100L)).sessionId.isNotBlank())
+    private fun verifyPayloadWithManual() {
+        val zygote = factory.startSessionWithManual(0)
+        assertTrue(zygote.sessionId.isNotBlank())
+        assertNotNull(factory.endSessionWithManual(0, zygote))
+        assertEquals(true, sessionPayloadSource.lastStartNewSession)
     }
 }

embrace-android-sdk/src/test/java/io/embrace/android/embracesdk/session/message/PayloadMessageCollatorImplTest.kt

@@ -96,7 +96,7 @@ internal class PayloadMessageCollatorImplTest {
                 initial = startMsg,
                 endType = SessionSnapshotType.NORMAL_END,
                 logger = initModule.logger,
-                backgroundActivityEnabled = true,
+                continueMonitoring = true,
                 crashId = "crashId"
             )
         )
@@ -123,7 +123,7 @@ internal class PayloadMessageCollatorImplTest {
                 initial = startMsg,
                 endType = SessionSnapshotType.NORMAL_END,
                 logger = initModule.logger,
-                backgroundActivityEnabled = true,
+                continueMonitoring = true,
                 crashId = "crashId",
             )
         )