diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index b01ffbd0ed..cbe6081241 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -4861,6 +4861,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 diff --git a/sentry/src/main/java/io/sentry/cache/EnvelopeCache.java b/sentry/src/main/java/io/sentry/cache/EnvelopeCache.java index 618de65547..3aee522063 100644 --- a/sentry/src/main/java/io/sentry/cache/EnvelopeCache.java +++ b/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 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. + * + *

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,20 @@ 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); + if (written) { + lastPersistedSessionId = session.getSessionId(); + } } } diff --git a/sentry/src/test/java/io/sentry/cache/EnvelopeCacheTest.kt b/sentry/src/test/java/io/sentry/cache/EnvelopeCacheTest.kt index c80117aa99..1fce4763fb 100644 --- a/sentry/src/test/java/io/sentry/cache/EnvelopeCacheTest.kt +++ b/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,160 @@ 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 `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() + whenever(serializer.serialize(same(liveSession), any())) + .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()) + } + @Test fun `updates current file on session update and read it back`() { val cache = fixture.getSUT() @@ -491,14 +649,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,