fix(session): When capturing unhandled hybrid exception session shoul…

…d be ended (#3480)
getsentry · Jun 28, 2024 · 8b0dfba · 8b0dfba
1 parent cb9b3f6
commit 8b0dfba
Show file tree

Hide file tree

Showing 8 changed files with 151 additions and 27 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -13,6 +13,7 @@
 - Use SecureRandom in favor of Random for Metrics ([#3495](https://github.com/getsentry/sentry-java/pull/3495))
 - Fix UncaughtExceptionHandlerIntegration Memory Leak ([#3398](https://github.com/getsentry/sentry-java/pull/3398))
 - Fix duplicated http spans ([#3526](https://github.com/getsentry/sentry-java/pull/3526))
+- When capturing unhandled hybrid exception session should be ended and new start if need ([#3480](https://github.com/getsentry/sentry-java/pull/3480))
 
 ### Dependencies
 

diff --git a/sentry-android-core/api/sentry-android-core.api b/sentry-android-core/api/sentry-android-core.api
@@ -204,7 +204,7 @@ public abstract interface class io/sentry/android/core/IDebugImagesLoader {
 
 public final class io/sentry/android/core/InternalSentrySdk {
 	public fun <init> ()V
-	public static fun captureEnvelope ([B)Lio/sentry/protocol/SentryId;
+	public static fun captureEnvelope ([BZ)Lio/sentry/protocol/SentryId;
 	public static fun getAppStartMeasurement ()Ljava/util/Map;
 	public static fun getCurrentScope ()Lio/sentry/IScope;
 	public static fun serializeScope (Landroid/content/Context;Lio/sentry/android/core/SentryAndroidOptions;Lio/sentry/IScope;)Ljava/util/Map;

diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/InternalSentrySdk.java b/sentry-android-core/src/main/java/io/sentry/android/core/InternalSentrySdk.java
@@ -1,5 +1,9 @@
 package io.sentry.android.core;
 
+import static io.sentry.SentryLevel.DEBUG;
+import static io.sentry.SentryLevel.INFO;
+import static io.sentry.SentryLevel.WARNING;
+
 import android.content.Context;
 import android.content.pm.PackageInfo;
 import android.content.pm.PackageManager;
@@ -19,12 +23,14 @@
 import io.sentry.android.core.performance.ActivityLifecycleTimeSpan;
 import io.sentry.android.core.performance.AppStartMetrics;
 import io.sentry.android.core.performance.TimeSpan;
+import io.sentry.cache.EnvelopeCache;
 import io.sentry.protocol.App;
 import io.sentry.protocol.Device;
 import io.sentry.protocol.SentryId;
 import io.sentry.protocol.User;
 import io.sentry.util.MapObjectWriter;
 import java.io.ByteArrayInputStream;
+import java.io.File;
 import java.io.InputStream;
 import java.util.ArrayList;
 import java.util.HashMap;
@@ -148,7 +154,8 @@ public static Map<String, Object> serializeScope(
    *     captured
    */
   @Nullable
-  public static SentryId captureEnvelope(final @NotNull byte[] envelopeData) {
+  public static SentryId captureEnvelope(
+      final @NotNull byte[] envelopeData, final boolean maybeStartNewSession) {
     final @NotNull IHub hub = HubAdapter.getInstance();
     final @NotNull SentryOptions options = hub.getOptions();
 
@@ -184,6 +191,13 @@ public static SentryId captureEnvelope(final @NotNull byte[] envelopeData) {
       if (session != null) {
         final SentryEnvelopeItem sessionItem = SentryEnvelopeItem.fromSession(serializer, session);
         envelopeItems.add(sessionItem);
+        deleteCurrentSessionFile(
+            options,
+            // should be sync if going to crash or already not a main thread
+            !maybeStartNewSession || !hub.getOptions().getMainThreadChecker().isMainThread());
+        if (maybeStartNewSession) {
+          hub.startSession();
+        }
       }
 
       final SentryEnvelope repackagedEnvelope =
@@ -233,15 +247,15 @@ private static void addTimeSpanToSerializedSpans(TimeSpan span, List<Map<String,
       HubAdapter.getInstance()
           .getOptions()
           .getLogger()
-          .log(SentryLevel.WARNING, "Can not convert not-started TimeSpan to Map for Hybrid SDKs.");
+          .log(WARNING, "Can not convert not-started TimeSpan to Map for Hybrid SDKs.");
       return;
     }
 
     if (span.hasNotStopped()) {
       HubAdapter.getInstance()
           .getOptions()
           .getLogger()
-          .log(SentryLevel.WARNING, "Can not convert not-stopped TimeSpan to Map for Hybrid SDKs.");
+          .log(WARNING, "Can not convert not-stopped TimeSpan to Map for Hybrid SDKs.");
       return;
     }
 
@@ -252,6 +266,46 @@ private static void addTimeSpanToSerializedSpans(TimeSpan span, List<Map<String,
     spans.add(spanMap);
   }
 
+  private static void deleteCurrentSessionFile(
+      final @NotNull SentryOptions options, boolean isSync) {
+    if (!isSync) {
+      try {
+        options
+            .getExecutorService()
+            .submit(
+                () -> {
+                  deleteCurrentSessionFile(options);
+                });
+      } catch (Throwable e) {
+        options
+            .getLogger()
+            .log(WARNING, "Submission of deletion of the current session file rejected.", e);
+      }
+    } else {
+      deleteCurrentSessionFile(options);
+    }
+  }
+
+  private static void deleteCurrentSessionFile(final @NotNull SentryOptions options) {
+    final String cacheDirPath = options.getCacheDirPath();
+    if (cacheDirPath == null) {
+      options.getLogger().log(INFO, "Cache dir is not set, not deleting the current session.");
+      return;
+    }
+
+    if (!options.isEnableAutoSessionTracking()) {
+      options
+          .getLogger()
+          .log(DEBUG, "Session tracking is disabled, bailing from deleting current session file.");
+      return;
+    }
+
+    final File sessionFile = EnvelopeCache.getCurrentSessionFile(cacheDirPath);
+    if (!sessionFile.delete()) {
+      options.getLogger().log(WARNING, "Failed to delete the current session file.");
+    }
+  }
+
   @Nullable
   private static Session updateSession(
       final @NotNull IHub hub,
@@ -268,11 +322,14 @@ private static Session updateSession(
             if (updated) {
               if (session.getStatus() == Session.State.Crashed) {
                 session.end();
+                // Session needs to be removed from the scope, otherwise it will be send twice
+                // standalone and with the crash event
+                scope.clearSession();
               }
               sessionRef.set(session);
             }
           } else {
-            options.getLogger().log(SentryLevel.INFO, "Session is null on updateSession");
+            options.getLogger().log(INFO, "Session is null on updateSession");
           }
         });
     return sessionRef.get();

diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/InternalSentrySdkTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/InternalSentrySdkTest.kt
@@ -39,6 +39,7 @@ import java.util.concurrent.atomic.AtomicReference
 import kotlin.test.BeforeTest
 import kotlin.test.Test
 import kotlin.test.assertEquals
+import kotlin.test.assertNotEquals
 import kotlin.test.assertNotNull
 import kotlin.test.assertNull
 import kotlin.test.assertTrue
@@ -84,7 +85,7 @@ class InternalSentrySdkTest {
             capturedEnvelopes.clear()
         }
 
-        fun captureEnvelopeWithEvent(event: SentryEvent = SentryEvent()) {
+        fun captureEnvelopeWithEvent(event: SentryEvent = SentryEvent(), maybeStartNewSession: Boolean = false) {
             // create an envelope with session data
             val options = Sentry.getCurrentHub().options
             val eventId = SentryId()
@@ -103,7 +104,24 @@ class InternalSentrySdkTest {
             options.serializer.serialize(envelope, outputStream)
             val data = outputStream.toByteArray()
 
-            InternalSentrySdk.captureEnvelope(data)
+            InternalSentrySdk.captureEnvelope(data, maybeStartNewSession)
+        }
+
+        fun createSentryEventWithUnhandledException(): SentryEvent {
+            return SentryEvent(RuntimeException()).apply {
+                val mechanism = Mechanism()
+                mechanism.isHandled = false
+
+                val factory = SentryExceptionFactory(mock())
+                val sentryExceptions = factory.getSentryExceptions(
+                    ExceptionMechanismException(
+                        mechanism,
+                        Throwable(),
+                        Thread()
+                    )
+                )
+                exceptions = sentryExceptions
+            }
         }
 
         fun mockFinishedAppStart() {
@@ -313,7 +331,7 @@ class InternalSentrySdkTest {
 
     @Test
     fun `captureEnvelope fails if payload is invalid`() {
-        assertNull(InternalSentrySdk.captureEnvelope(ByteArray(8)))
+        assertNull(InternalSentrySdk.captureEnvelope(ByteArray(8), false))
     }
 
     @Test
@@ -337,27 +355,19 @@ class InternalSentrySdkTest {
     }
 
     @Test
-    fun `captureEnvelope correctly enriches the envelope with session data`() {
+    fun `captureEnvelope correctly enriches the envelope with session data and does not start new session`() {
         val fixture = Fixture()
         fixture.init(context)
 
-        // when capture envelope is called with an crashed event
-        fixture.captureEnvelopeWithEvent(
-            SentryEvent(RuntimeException()).apply {
-                val mechanism = Mechanism()
-                mechanism.isHandled = false
+        // keep reference for current session for later assertions
+        // we need to get the reference now as it will be removed from the scope
+        val sessionRef = AtomicReference<Session>()
+        Sentry.configureScope { scope ->
+            sessionRef.set(scope.session)
+        }
 
-                val factory = SentryExceptionFactory(mock())
-                val sentryExceptions = factory.getSentryExceptions(
-                    ExceptionMechanismException(
-                        mechanism,
-                        Throwable(),
-                        Thread()
-                    )
-                )
-                exceptions = sentryExceptions
-            }
-        )
+        // when capture envelope is called with an crashed event
+        fixture.captureEnvelopeWithEvent(fixture.createSentryEventWithUnhandledException())
 
         val capturedEnvelope = fixture.capturedEnvelopes.first()
         val capturedEnvelopeItems = capturedEnvelope.items.toList()
@@ -374,12 +384,52 @@ class InternalSentrySdkTest {
         )!!
         assertEquals(Session.State.Crashed, capturedSession.status)
 
-        // and the local session should be marked as crashed too
+        assertEquals(Session.State.Crashed, sessionRef.get().status)
+        assertEquals(capturedSession.sessionId, sessionRef.get().sessionId)
+    }
+
+    @Test
+    fun `captureEnvelope starts new session when enabled`() {
+        val fixture = Fixture()
+        fixture.init(context)
+
+        // when capture envelope is called with an crashed event
+        fixture.captureEnvelopeWithEvent(fixture.createSentryEventWithUnhandledException(), true)
+
         val scopeRef = AtomicReference<IScope>()
         Sentry.configureScope { scope ->
             scopeRef.set(scope)
         }
-        assertEquals(Session.State.Crashed, scopeRef.get().session!!.status)
+
+        // first envelope is the new session start
+        val capturedStartSessionEnvelope = fixture.capturedEnvelopes.first()
+        val capturedNewSessionStart = fixture.options.serializer.deserialize(
+            InputStreamReader(ByteArrayInputStream(capturedStartSessionEnvelope.items.toList()[0].data)),
+            Session::class.java
+        )!!
+        assertEquals(capturedNewSessionStart.sessionId, scopeRef.get().session!!.sessionId)
+        assertEquals(Session.State.Ok, capturedNewSessionStart.status)
+
+        val capturedEnvelope = fixture.capturedEnvelopes.last()
+        val capturedEnvelopeItems = capturedEnvelope.items.toList()
+
+        // there should be two envelopes session start and captured crash
+        assertEquals(2, fixture.capturedEnvelopes.size)
+
+        // then it should contain the original event + session
+        assertEquals(2, capturedEnvelopeItems.size)
+        assertEquals(SentryItemType.Event, capturedEnvelopeItems[0].header.type)
+        assertEquals(SentryItemType.Session, capturedEnvelopeItems[1].header.type)
+
+        // and then the sent session should be marked as crashed
+        val capturedSession = fixture.options.serializer.deserialize(
+            InputStreamReader(ByteArrayInputStream(capturedEnvelopeItems[1].data)),
+            Session::class.java
+        )!!
+        assertEquals(Session.State.Crashed, capturedSession.status)
+
+        // and the local session should be a new session
+        assertNotEquals(capturedSession.sessionId, scopeRef.get().session!!.sessionId)
     }
 
     @Test

diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api
@@ -667,6 +667,7 @@ public abstract interface class io/sentry/IScope {
 	public abstract fun clear ()V
 	public abstract fun clearAttachments ()V
 	public abstract fun clearBreadcrumbs ()V
+	public abstract fun clearSession ()V
 	public abstract fun clearTransaction ()V
 	public abstract fun clone ()Lio/sentry/IScope;
 	public abstract fun endSession ()Lio/sentry/Session;
@@ -1221,6 +1222,7 @@ public final class io/sentry/NoOpScope : io/sentry/IScope {
 	public fun clear ()V
 	public fun clearAttachments ()V
 	public fun clearBreadcrumbs ()V
+	public fun clearSession ()V
 	public fun clearTransaction ()V
 	public fun clone ()Lio/sentry/IScope;
 	public synthetic fun clone ()Ljava/lang/Object;
@@ -1592,6 +1594,7 @@ public final class io/sentry/Scope : io/sentry/IScope {
 	public fun clear ()V
 	public fun clearAttachments ()V
 	public fun clearBreadcrumbs ()V
+	public fun clearSession ()V
 	public fun clearTransaction ()V
 	public fun clone ()Lio/sentry/IScope;
 	public synthetic fun clone ()Ljava/lang/Object;

diff --git a/sentry/src/main/java/io/sentry/IScope.java b/sentry/src/main/java/io/sentry/IScope.java
@@ -352,6 +352,9 @@ public interface IScope {
   @Nullable
   Session getSession();
 
+  @ApiStatus.Internal
+  void clearSession();
+
   @ApiStatus.Internal
   void setPropagationContext(final @NotNull PropagationContext propagationContext);
 

diff --git a/sentry/src/main/java/io/sentry/NoOpScope.java b/sentry/src/main/java/io/sentry/NoOpScope.java
@@ -219,6 +219,10 @@ public void withTransaction(Scope.@NotNull IWithTransaction callback) {}
     return null;
   }
 
+  @ApiStatus.Internal
+  @Override
+  public void clearSession() {}
+
   @ApiStatus.Internal
   @Override
   public void setPropagationContext(@NotNull PropagationContext propagationContext) {}

diff --git a/sentry/src/main/java/io/sentry/Scope.java b/sentry/src/main/java/io/sentry/Scope.java
@@ -913,6 +913,12 @@ public SentryOptions getOptions() {
     return session;
   }
 
+  @ApiStatus.Internal
+  @Override
+  public void clearSession() {
+    session = null;
+  }
+
   @ApiStatus.Internal
   @Override
   public void setPropagationContext(final @NotNull PropagationContext propagationContext) {