fix(core): [Unhandled Sessions 2] Don't let a queued SessionStart overwrite a newer session

sentry/api/sentry.api

@@ -4880,6 +4880,7 @@ public class io/sentry/cache/EnvelopeCache : io/sentry/cache/IEnvelopeCache {
 	public static fun getPreviousSessionFile (Ljava/lang/String;)Ljava/io/File;
 	public fun iterator ()Ljava/util/Iterator;
 	public fun movePreviousSession (Ljava/io/File;Ljava/io/File;)V
+	public fun persistCurrentSession (Lio/sentry/Session;)V
 	public fun store (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)V
 	public fun storeEnvelope (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)Z
 	public fun waitPreviousSessionFlush ()Z

sentry/src/main/java/io/sentry/cache/EnvelopeCache.java

@@ -76,6 +76,12 @@ public class EnvelopeCache extends CacheStrategy implements IEnvelopeCache {
   protected final @NotNull AutoClosableReentrantLock cacheLock = new AutoClosableReentrantLock();
   protected final @NotNull AutoClosableReentrantLock sessionLock = new AutoClosableReentrantLock();
 
+  /**
+   * Session id last written to the current session file by {@link #persistCurrentSession(Session)},
+   * which bypasses the transport queue that every other write to that file goes through.
+   */
+  private @Nullable String lastPersistedSessionId;
+
   public static @NotNull IEnvelopeCache create(final @NotNull SentryOptions options) {
     final String cacheDirPath = options.getCacheDirPath();
     final int maxCacheItems = options.getMaxCacheItems();
@@ -118,8 +124,11 @@ private boolean storeInternal(final @NotNull SentryEnvelope envelope, final @Not
     final File previousSessionFile = getPreviousSessionFile(directoryPath);
 
     if (HintUtils.hasType(hint, SessionEnd.class)) {
-      if (!currentSessionFile.delete()) {
-        options.getLogger().log(WARNING, "Current envelope doesn't exist.");
+      try (final @NotNull ISentryLifecycleToken ignored = sessionLock.acquire()) {
+        lastPersistedSessionId = null;
+        if (!currentSessionFile.delete()) {
+          options.getLogger().log(WARNING, "Current envelope doesn't exist.");
+        }
       }
     }
 
@@ -129,8 +138,15 @@ private boolean storeInternal(final @NotNull SentryEnvelope envelope, final @Not
     }
 
     if (HintUtils.hasType(hint, SessionStart.class)) {
-      movePreviousSession(currentSessionFile, previousSessionFile);
-      updateCurrentSession(currentSessionFile, envelope);
+      try (final @NotNull ISentryLifecycleToken ignored = sessionLock.acquire()) {
+        final @Nullable Session startingSession = readSessionFromEnvelope(envelope);
+        if (!isAlreadyPersisted(startingSession)) {
+          movePreviousSession(currentSessionFile, previousSessionFile);
+          if (startingSession != null) {
+            writeSessionToDisk(currentSessionFile, startingSession);
+          }
+        }
+      }
 
       boolean crashedLastRun = false;
       final File crashMarkerFile = new File(options.getCacheDirPath(), NATIVE_CRASH_MARKER_FILE);
@@ -274,8 +290,7 @@ private void writeCrashMarkerFile() {
     }
   }
 
-  private void updateCurrentSession(
-      final @NotNull File currentSessionFile, final @NotNull SentryEnvelope envelope) {
+  private @Nullable Session readSessionFromEnvelope(final @NotNull SentryEnvelope envelope) {
     final Iterable<SentryEnvelopeItem> items = envelope.getItems();
 
     // we know that an envelope with a SessionStart hint has a single item inside
@@ -295,7 +310,7 @@ private void updateCurrentSession(
                     "Item of type %s returned null by the parser.",
                     item.getHeader().getType());
           } else {
-            writeSessionToDisk(currentSessionFile, session);
+            return session;
           }
         } catch (Throwable e) {
           options.getLogger().log(ERROR, "Item failed to process.", e);
@@ -309,10 +324,26 @@ private void updateCurrentSession(
                 item.getHeader().getType());
       }
     } else {
-      options
-          .getLogger()
-          .log(INFO, "Current envelope %s is empty", currentSessionFile.getAbsolutePath());
+      options.getLogger().log(INFO, "Current envelope is empty.");
+    }
+    return null;
+  }
+
+  /**
+   * Whether a {@link SessionStart} envelope refers to the session {@link
+   * #persistCurrentSession(Session)} already wrote to the current session file. That copy is the
+   * live session, so it is at least as advanced as this envelope. Rotating and overwriting it would
+   * file a running session as the previous one and roll back any unhandled error it has recorded
+   * since.
+   *
+   * <p>A null session id never matches, so sessions we cannot tell apart are rotated as before.
+   */
+  private boolean isAlreadyPersisted(final @Nullable Session startingSession) {
+    if (startingSession == null) {
+      return false;
     }
+    final @Nullable String startingSessionId = startingSession.getSessionId();
+    return startingSessionId != null && startingSessionId.equals(lastPersistedSessionId);
   }
 
   private boolean writeEnvelopeToDisk(
@@ -337,7 +368,7 @@ private boolean writeEnvelopeToDisk(
     return true;
   }
 
-  private void writeSessionToDisk(final @NotNull File file, final @NotNull Session session) {
+  private boolean writeSessionToDisk(final @NotNull File file, final @NotNull Session session) {
     try (final OutputStream outputStream = new FileOutputStream(file);
         final Writer writer = new BufferedWriter(new OutputStreamWriter(outputStream, UTF_8))) {
       options
@@ -349,6 +380,19 @@ private void writeSessionToDisk(final @NotNull File file, final @NotNull Session
       options
           .getLogger()
           .log(ERROR, e, "Error writing Session to offline storage: %s", session.getSessionId());
+      return false;
+    }
+    return true;
+  }
+
+  @ApiStatus.Internal
+  public void persistCurrentSession(final @NotNull Session session) {
+    try (final @NotNull ISentryLifecycleToken ignored = sessionLock.acquire()) {
+      final boolean written =
+          writeSessionToDisk(
+              getCurrentSessionFile(directory.getOrCreate().getAbsolutePath()), session);
+      // a failed write truncates the file, so there is no good copy left to protect
+      lastPersistedSessionId = written ? session.getSessionId() : null;
     }
   }
 

sentry/src/test/java/io/sentry/cache/EnvelopeCacheTest.kt

@@ -1,5 +1,6 @@
 package io.sentry.cache
 
+import com.google.common.truth.Truth.assertThat
 import io.sentry.DateUtils
 import io.sentry.Hint
 import io.sentry.ILogger
@@ -23,6 +24,7 @@ import io.sentry.hints.SessionStartHint
 import io.sentry.protocol.SentryId
 import io.sentry.util.HintUtils
 import java.io.File
+import java.io.Writer
 import java.nio.file.Files
 import java.nio.file.Path
 import java.util.Date
@@ -34,8 +36,10 @@ import kotlin.test.assertFalse
 import kotlin.test.assertNotNull
 import kotlin.test.assertTrue
 import org.mockito.kotlin.any
+import org.mockito.kotlin.eq
 import org.mockito.kotlin.mock
 import org.mockito.kotlin.same
+import org.mockito.kotlin.verify
 import org.mockito.kotlin.whenever
 
 class EnvelopeCacheTest {
@@ -160,6 +164,189 @@ class EnvelopeCacheTest {
     assertTrue(didStore)
   }
 
+  @Test
+  fun `delayed same SID SessionStart preserves newer unhandled snapshot`() {
+    val cache = fixture.getSUT()
+    val sid = SentryUUID.generateSentryId()
+    val currentSessionFile = EnvelopeCache.getCurrentSessionFile(fixture.options.cacheDirPath!!)
+    val previousSessionFile = EnvelopeCache.getPreviousSessionFile(fixture.options.cacheDirPath!!)
+    val newerSession = createSession(sessionId = sid)
+    newerSession.recordNonTerminatingUnhandledError()
+    cache.persistCurrentSession(newerSession)
+
+    val delayedStart = createSession(sessionId = sid)
+    val envelope = SentryEnvelope.from(fixture.options.serializer, delayedStart, null)
+    cache.storeEnvelope(envelope, HintUtils.createWithTypeCheckHint(SessionStartHint()))
+
+    val persistedSession =
+      fixture.options.serializer.deserialize(
+        currentSessionFile.bufferedReader(),
+        Session::class.java,
+      )!!
+    assertThat(persistedSession.sessionId).isEqualTo(sid)
+    assertThat(persistedSession.hasNonTerminatingUnhandledError()).isTrue()
+    assertThat(persistedSession.errorCount()).isEqualTo(1)
+    assertThat(previousSessionFile.exists()).isFalse()
+  }
+
+  @Test
+  fun `delayed same SID SessionStart preserves newer error count snapshot`() {
+    val cache = fixture.getSUT()
+    val sid = SentryUUID.generateSentryId()
+    val currentSessionFile = EnvelopeCache.getCurrentSessionFile(fixture.options.cacheDirPath!!)
+    val previousSessionFile = EnvelopeCache.getPreviousSessionFile(fixture.options.cacheDirPath!!)
+    val newerSession = createSession(sessionId = sid)
+    newerSession.update(null, null, true)
+    cache.persistCurrentSession(newerSession)
+
+    val delayedStart = createSession(sessionId = sid)
+    val envelope = SentryEnvelope.from(fixture.options.serializer, delayedStart, null)
+    cache.storeEnvelope(envelope, HintUtils.createWithTypeCheckHint(SessionStartHint()))
+
+    val persistedSession =
+      fixture.options.serializer.deserialize(
+        currentSessionFile.bufferedReader(),
+        Session::class.java,
+      )!!
+    assertThat(persistedSession.sessionId).isEqualTo(sid)
+    assertThat(persistedSession.hasNonTerminatingUnhandledError()).isFalse()
+    assertThat(persistedSession.errorCount()).isEqualTo(1)
+    assertThat(previousSessionFile.exists()).isFalse()
+  }
+
+  @Test
+  fun `failed persist stops the delayed same SID SessionStart from being skipped`() {
+    val cache = fixture.getSUT()
+    val sid = SentryUUID.generateSentryId()
+    val currentSessionFile = EnvelopeCache.getCurrentSessionFile(fixture.options.cacheDirPath!!)
+    val newerSession = createSession(sessionId = sid)
+    newerSession.recordNonTerminatingUnhandledError()
+    cache.persistCurrentSession(newerSession)
+
+    // a directory where the session file belongs makes the write fail
+    assertTrue(currentSessionFile.delete())
+    assertTrue(currentSessionFile.mkdir())
+    cache.persistCurrentSession(newerSession)
+    assertTrue(currentSessionFile.delete())
+
+    val delayedStart = createSession(sessionId = sid)
+    val envelope = SentryEnvelope.from(fixture.options.serializer, delayedStart, null)
+    cache.storeEnvelope(envelope, HintUtils.createWithTypeCheckHint(SessionStartHint()))
+
+    val persistedSession =
+      fixture.options.serializer.deserialize(
+        currentSessionFile.bufferedReader(),
+        Session::class.java,
+      )!!
+    assertThat(persistedSession.sessionId).isEqualTo(sid)
+    assertThat(persistedSession.hasNonTerminatingUnhandledError()).isFalse()
+    assertThat(persistedSession.errorCount()).isEqualTo(0)
+  }
+
+  @Test
+  fun `null SIDs on SessionStart rotate instead of preserving as same session`() {
+    val cache = fixture.getSUT()
+    val currentSession = createSession(sessionId = null)
+    currentSession.update(null, null, true)
+    cache.persistCurrentSession(currentSession)
+    val startingSession = createSession(sessionId = null)
+
+    val envelope = SentryEnvelope.from(fixture.options.serializer, startingSession, null)
+    cache.storeEnvelope(envelope, HintUtils.createWithTypeCheckHint(SessionStartHint()))
+
+    val currentSessionFile = EnvelopeCache.getCurrentSessionFile(fixture.options.cacheDirPath!!)
+    val previousSessionFile = EnvelopeCache.getPreviousSessionFile(fixture.options.cacheDirPath!!)
+    val persistedCurrent =
+      fixture.options.serializer.deserialize(
+        currentSessionFile.bufferedReader(),
+        Session::class.java,
+      )!!
+    val persistedPrevious =
+      fixture.options.serializer.deserialize(
+        previousSessionFile.bufferedReader(),
+        Session::class.java,
+      )!!
+    assertThat(persistedCurrent.sessionId).isNull()
+    assertThat(persistedCurrent.errorCount()).isEqualTo(0)
+    assertThat(persistedPrevious.sessionId).isNull()
+    assertThat(persistedPrevious.errorCount()).isEqualTo(1)
+  }
+
+  @Test
+  fun `different SID SessionStart rotates current session`() {
+    val cache = fixture.getSUT()
+    val currentSession = createSession()
+    cache.persistCurrentSession(currentSession)
+    val nextSession = createSession()
+
+    val envelope = SentryEnvelope.from(fixture.options.serializer, nextSession, null)
+    cache.storeEnvelope(envelope, HintUtils.createWithTypeCheckHint(SessionStartHint()))
+
+    val currentSessionFile = EnvelopeCache.getCurrentSessionFile(fixture.options.cacheDirPath!!)
+    val previousSessionFile = EnvelopeCache.getPreviousSessionFile(fixture.options.cacheDirPath!!)
+    val persistedCurrent =
+      fixture.options.serializer.deserialize(
+        currentSessionFile.bufferedReader(),
+        Session::class.java,
+      )!!
+    val persistedPrevious =
+      fixture.options.serializer.deserialize(
+        previousSessionFile.bufferedReader(),
+        Session::class.java,
+      )!!
+    assertThat(persistedCurrent.sessionId).isEqualTo(nextSession.sessionId)
+    assertThat(persistedPrevious.sessionId).isEqualTo(currentSession.sessionId)
+  }
+
+  @Test
+  fun `SessionEnd deleting the persisted session lets the delayed SessionStart write it again`() {
+    val cache = fixture.getSUT()
+    val sid = SentryUUID.generateSentryId()
+    val currentSessionFile = EnvelopeCache.getCurrentSessionFile(fixture.options.cacheDirPath!!)
+    cache.persistCurrentSession(createSession(sessionId = sid))
+
+    // the previous session's end envelope is still queued and deletes the file the live session
+    // was just written to
+    val endedSession = createSession()
+    cache.storeEnvelope(
+      SentryEnvelope.from(fixture.options.serializer, endedSession, null),
+      HintUtils.createWithTypeCheckHint(SessionEndHint()),
+    )
+    assertThat(currentSessionFile.exists()).isFalse()
+
+    val delayedStart = createSession(sessionId = sid)
+    cache.storeEnvelope(
+      SentryEnvelope.from(fixture.options.serializer, delayedStart, null),
+      HintUtils.createWithTypeCheckHint(SessionStartHint()),
+    )
+
+    val persistedSession =
+      fixture.options.serializer.deserialize(
+        currentSessionFile.bufferedReader(),
+        Session::class.java,
+      )!!
+    assertThat(persistedSession.sessionId).isEqualTo(sid)
+  }
+
+  @Test
+  fun `failed persist lets the delayed SessionStart write the session`() {
+    val sid = SentryUUID.generateSentryId()
+    val liveSession = createSession(sessionId = sid)
+    val delayedStart = createSession(sessionId = sid)
+    val serializer = mock<ISerializer>()
+    whenever(serializer.serialize(same(liveSession), any<Writer>()))
+      .thenThrow(RuntimeException("forced ex"))
+    whenever(serializer.deserialize(any(), eq(Session::class.java))).thenReturn(delayedStart)
+    val cache = fixture.getSUT { options -> options.setSerializer(serializer) }
+
+    cache.persistCurrentSession(liveSession)
+
+    val envelope = SentryEnvelope.from(SentryOptions.empty().serializer, delayedStart, null)
+    cache.storeEnvelope(envelope, HintUtils.createWithTypeCheckHint(SessionStartHint()))
+
+    verify(serializer).serialize(same(delayedStart), any<Writer>())
+  }
+
   @Test
   fun `updates current file on session update and read it back`() {
     val cache = fixture.getSUT()
@@ -491,14 +678,17 @@ class EnvelopeCacheTest {
     assertFalse(didStore)
   }
 
-  private fun createSession(started: Date? = null): Session =
+  private fun createSession(
+    started: Date? = null,
+    sessionId: String? = SentryUUID.generateSentryId(),
+  ): Session =
     Session(
       Ok,
       started ?: DateUtils.getCurrentDateTime(),
       DateUtils.getCurrentDateTime(),
       0,
       "dis",
-      SentryUUID.generateSentryId(),
+      sessionId,
       true,
       null,
       null,