diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a38f544c17..01f269da5b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,8 @@ - Ensure frame metrics listeners are registered/unregistered on the main thread ([#4582](https://github.com/getsentry/sentry-java/pull/4582)) - Do not report cached events as lost ([#4575](https://github.com/getsentry/sentry-java/pull/4575)) - Previously events were recorded as lost early despite being retried later through the cache +- Move and flush unfinished previous session on init ([#4624](https://github.com/getsentry/sentry-java/pull/4624)) + - This removes the need for unnecessary blocking our background queue for 15 seconds in the case of a background app start - Switch to compileOnly dependency for compose-ui-material ([#4630](https://github.com/getsentry/sentry-java/pull/4630)) - This fixes `StackOverflowError` when using OSS Licenses plugin diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 0f3600b77c2..4cf0cd34408 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -4346,6 +4346,7 @@ public class io/sentry/cache/EnvelopeCache : io/sentry/cache/IEnvelopeCache { public static final field SUFFIX_ENVELOPE_FILE Ljava/lang/String; protected static final field UTF_8 Ljava/nio/charset/Charset; protected final field cacheLock Lio/sentry/util/AutoClosableReentrantLock; + protected final field sessionLock Lio/sentry/util/AutoClosableReentrantLock; public fun (Lio/sentry/SentryOptions;Ljava/lang/String;I)V public static fun create (Lio/sentry/SentryOptions;)Lio/sentry/cache/IEnvelopeCache; public fun discard (Lio/sentry/SentryEnvelope;)V @@ -4353,6 +4354,7 @@ public class io/sentry/cache/EnvelopeCache : io/sentry/cache/IEnvelopeCache { public static fun getCurrentSessionFile (Ljava/lang/String;)Ljava/io/File; 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 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/MovePreviousSession.java b/sentry/src/main/java/io/sentry/MovePreviousSession.java new file mode 100644 index 00000000000..99b92d92653 --- /dev/null +++ b/sentry/src/main/java/io/sentry/MovePreviousSession.java @@ -0,0 +1,44 @@ +package io.sentry; + +import static io.sentry.SentryLevel.DEBUG; +import static io.sentry.SentryLevel.INFO; + +import io.sentry.cache.EnvelopeCache; +import io.sentry.cache.IEnvelopeCache; +import java.io.File; +import org.jetbrains.annotations.NotNull; + +final class MovePreviousSession implements Runnable { + + private final @NotNull SentryOptions options; + + MovePreviousSession(final @NotNull SentryOptions options) { + this.options = options; + } + + @Override + public void run() { + final String cacheDirPath = options.getCacheDirPath(); + if (cacheDirPath == null) { + options.getLogger().log(INFO, "Cache dir is not set, not moving the previous session."); + return; + } + + if (!options.isEnableAutoSessionTracking()) { + options + .getLogger() + .log(DEBUG, "Session tracking is disabled, bailing from previous session mover."); + return; + } + + final IEnvelopeCache cache = options.getEnvelopeDiskCache(); + if (cache instanceof EnvelopeCache) { + final File currentSessionFile = EnvelopeCache.getCurrentSessionFile(cacheDirPath); + final File previousSessionFile = EnvelopeCache.getPreviousSessionFile(cacheDirPath); + + ((EnvelopeCache) cache).movePreviousSession(currentSessionFile, previousSessionFile); + + ((EnvelopeCache) cache).flushPreviousSession(); + } + } +} diff --git a/sentry/src/main/java/io/sentry/Sentry.java b/sentry/src/main/java/io/sentry/Sentry.java index c462f91025b..bf96c5f3232 100644 --- a/sentry/src/main/java/io/sentry/Sentry.java +++ b/sentry/src/main/java/io/sentry/Sentry.java @@ -346,6 +346,8 @@ private static void init(final @NotNull SentryOptions options, final boolean glo options.setExecutorService(new SentryExecutorService(options)); options.getExecutorService().prewarm(); } + + movePreviousSession(options); // when integrations are registered on Scopes ctor and async integrations are fired, // it might and actually happened that integrations called captureSomething // and Scopes was still NoOp. @@ -497,6 +499,16 @@ private static void handleAppStartProfilingConfig( return options.getInternalTracesSampler().sample(appStartSamplingContext); } + @SuppressWarnings("FutureReturnValueIgnored") + private static void movePreviousSession(final @NotNull SentryOptions options) { + // enqueue a task to move previous unfinished session to its own file + try { + options.getExecutorService().submit(new MovePreviousSession(options)); + } catch (Throwable e) { + options.getLogger().log(SentryLevel.DEBUG, "Failed to move previous session.", e); + } + } + @SuppressWarnings("FutureReturnValueIgnored") private static void finalizePreviousSession( final @NotNull SentryOptions options, final @NotNull IScopes scopes) { diff --git a/sentry/src/main/java/io/sentry/cache/EnvelopeCache.java b/sentry/src/main/java/io/sentry/cache/EnvelopeCache.java index 1aca185d9cc..d2a32d44cd0 100644 --- a/sentry/src/main/java/io/sentry/cache/EnvelopeCache.java +++ b/sentry/src/main/java/io/sentry/cache/EnvelopeCache.java @@ -73,6 +73,7 @@ public class EnvelopeCache extends CacheStrategy implements IEnvelopeCache { private final @NotNull Map fileNameMap = new WeakHashMap<>(); protected final @NotNull AutoClosableReentrantLock cacheLock = new AutoClosableReentrantLock(); + protected final @NotNull AutoClosableReentrantLock sessionLock = new AutoClosableReentrantLock(); public static @NotNull IEnvelopeCache create(final @NotNull SentryOptions options) { final String cacheDirPath = options.getCacheDirPath(); @@ -123,20 +124,7 @@ private boolean storeInternal(final @NotNull SentryEnvelope envelope, final @Not } if (HintUtils.hasType(hint, SessionStart.class)) { - if (currentSessionFile.exists()) { - options.getLogger().log(WARNING, "Current session is not ended, we'd need to end it."); - - try (final Reader reader = - new BufferedReader( - new InputStreamReader(new FileInputStream(currentSessionFile), UTF_8))) { - final Session session = serializer.getValue().deserialize(reader, Session.class); - if (session != null) { - writeSessionToDisk(previousSessionFile, session); - } - } catch (Throwable e) { - options.getLogger().log(SentryLevel.ERROR, "Error processing session.", e); - } - } + movePreviousSession(currentSessionFile, previousSessionFile); updateCurrentSession(currentSessionFile, envelope); boolean crashedLastRun = false; @@ -329,17 +317,12 @@ private boolean writeEnvelopeToDisk( } private void writeSessionToDisk(final @NotNull File file, final @NotNull Session session) { - if (file.exists()) { + try (final OutputStream outputStream = new FileOutputStream(file); + final Writer writer = new BufferedWriter(new OutputStreamWriter(outputStream, UTF_8))) { options .getLogger() .log(DEBUG, "Overwriting session to offline storage: %s", session.getSessionId()); - if (!file.delete()) { - options.getLogger().log(SentryLevel.ERROR, "Failed to delete: %s", file.getAbsolutePath()); - } - } - try (final OutputStream outputStream = new FileOutputStream(file); - final Writer writer = new BufferedWriter(new OutputStreamWriter(outputStream, UTF_8))) { serializer.getValue().serialize(session, writer); } catch (Throwable e) { options @@ -454,4 +437,33 @@ public boolean waitPreviousSessionFlush() { public void flushPreviousSession() { previousSessionLatch.countDown(); } + + public void movePreviousSession( + final @NotNull File currentSessionFile, final @NotNull File previousSessionFile) { + try (final @NotNull ISentryLifecycleToken ignored = sessionLock.acquire()) { + if (previousSessionFile.exists()) { + options.getLogger().log(DEBUG, "Previous session file already exists, deleting it."); + if (!previousSessionFile.delete()) { + options + .getLogger() + .log(WARNING, "Unable to delete previous session file: %s", previousSessionFile); + } + } + + if (currentSessionFile.exists()) { + options.getLogger().log(INFO, "Moving current session to previous session."); + + try { + final boolean renamed = currentSessionFile.renameTo(previousSessionFile); + if (!renamed) { + options.getLogger().log(WARNING, "Unable to move current session to previous session."); + } + } catch (Throwable e) { + options + .getLogger() + .log(SentryLevel.ERROR, "Error moving current session to previous session.", e); + } + } + } + } } diff --git a/sentry/src/test/java/io/sentry/MovePreviousSessionTest.kt b/sentry/src/test/java/io/sentry/MovePreviousSessionTest.kt new file mode 100644 index 00000000000..0c361c3efe8 --- /dev/null +++ b/sentry/src/test/java/io/sentry/MovePreviousSessionTest.kt @@ -0,0 +1,160 @@ +package io.sentry + +import io.sentry.cache.EnvelopeCache +import io.sentry.cache.IEnvelopeCache +import io.sentry.transport.NoOpEnvelopeCache +import java.nio.file.Files +import java.nio.file.Path +import kotlin.test.AfterTest +import kotlin.test.BeforeTest +import kotlin.test.Test +import kotlin.test.assertFalse +import kotlin.test.assertTrue +import org.mockito.kotlin.any +import org.mockito.kotlin.mock +import org.mockito.kotlin.never +import org.mockito.kotlin.verify + +class MovePreviousSessionTest { + + private class Fixture { + val tempDir: Path = Files.createTempDirectory("sentry-move-session-test") + val options = + SentryOptions().apply { + isDebug = true + setLogger(SystemOutLogger()) + } + val cache = mock() + + fun getSUT( + cacheDirPath: String? = tempDir.toAbsolutePath().toFile().absolutePath, + isEnableSessionTracking: Boolean = true, + envelopeCache: IEnvelopeCache? = null, + ): MovePreviousSession { + options.cacheDirPath = cacheDirPath + options.isEnableAutoSessionTracking = isEnableSessionTracking + options.setEnvelopeDiskCache(envelopeCache ?: EnvelopeCache.create(options)) + return MovePreviousSession(options) + } + + fun cleanup() { + tempDir.toFile().deleteRecursively() + } + } + + private lateinit var fixture: Fixture + + @BeforeTest + fun setup() { + fixture = Fixture() + } + + @AfterTest + fun teardown() { + fixture.cleanup() + } + + @Test + fun `when cache dir is null, logs and returns early`() { + val sut = fixture.getSUT(cacheDirPath = null, envelopeCache = fixture.cache) + + sut.run() + + verify(fixture.cache, never()).movePreviousSession(any(), any()) + verify(fixture.cache, never()).flushPreviousSession() + } + + @Test + fun `when session tracking is disabled, logs and returns early`() { + val sut = fixture.getSUT(isEnableSessionTracking = false, envelopeCache = fixture.cache) + + sut.run() + + verify(fixture.cache, never()).movePreviousSession(any(), any()) + verify(fixture.cache, never()).flushPreviousSession() + } + + @Test + fun `when envelope cache is not EnvelopeCache instance, does nothing`() { + val sut = fixture.getSUT(envelopeCache = NoOpEnvelopeCache.getInstance()) + + sut.run() + + verify(fixture.cache, never()).movePreviousSession(any(), any()) + verify(fixture.cache, never()).flushPreviousSession() + } + + @Test + fun `integration test with real EnvelopeCache`() { + val sut = fixture.getSUT() + + // Create a current session file + val currentSessionFile = EnvelopeCache.getCurrentSessionFile(fixture.options.cacheDirPath!!) + val previousSessionFile = EnvelopeCache.getPreviousSessionFile(fixture.options.cacheDirPath!!) + + currentSessionFile.createNewFile() + currentSessionFile.writeText("session content") + + assertTrue(currentSessionFile.exists()) + assertFalse(previousSessionFile.exists()) + + sut.run() + + // Wait for flush to complete + (fixture.options.envelopeDiskCache as EnvelopeCache).waitPreviousSessionFlush() + + // Current session file should have been moved to previous + assertFalse(currentSessionFile.exists()) + assertTrue(previousSessionFile.exists()) + assert(previousSessionFile.readText() == "session content") + + fixture.cleanup() + } + + @Test + fun `integration test when current session file does not exist`() { + val sut = fixture.getSUT() + + val currentSessionFile = EnvelopeCache.getCurrentSessionFile(fixture.options.cacheDirPath!!) + val previousSessionFile = EnvelopeCache.getPreviousSessionFile(fixture.options.cacheDirPath!!) + + assertFalse(currentSessionFile.exists()) + assertFalse(previousSessionFile.exists()) + + sut.run() + + (fixture.options.envelopeDiskCache as EnvelopeCache).waitPreviousSessionFlush() + + assertFalse(currentSessionFile.exists()) + assertFalse(previousSessionFile.exists()) + + fixture.cleanup() + } + + @Test + fun `integration test when previous session file already exists`() { + val sut = fixture.getSUT() + + val currentSessionFile = EnvelopeCache.getCurrentSessionFile(fixture.options.cacheDirPath!!) + val previousSessionFile = EnvelopeCache.getPreviousSessionFile(fixture.options.cacheDirPath!!) + + currentSessionFile.createNewFile() + currentSessionFile.writeText("current session") + previousSessionFile.createNewFile() + previousSessionFile.writeText("previous session") + + assertTrue(currentSessionFile.exists()) + assertTrue(previousSessionFile.exists()) + + sut.run() + + (fixture.options.envelopeDiskCache as EnvelopeCache).waitPreviousSessionFlush() + + // Current session file should have been moved to previous + assertFalse(currentSessionFile.exists()) + assertTrue(previousSessionFile.exists()) + assert(previousSessionFile.readText() == "current session") + + fixture.cleanup() + } +} diff --git a/sentry/src/test/java/io/sentry/SentryTest.kt b/sentry/src/test/java/io/sentry/SentryTest.kt index 87f4ce17c00..3b8549ce00d 100644 --- a/sentry/src/test/java/io/sentry/SentryTest.kt +++ b/sentry/src/test/java/io/sentry/SentryTest.kt @@ -52,6 +52,7 @@ import org.mockito.kotlin.anyOrNull import org.mockito.kotlin.argThat import org.mockito.kotlin.check import org.mockito.kotlin.eq +import org.mockito.kotlin.inOrder import org.mockito.kotlin.mock import org.mockito.kotlin.never import org.mockito.kotlin.verify @@ -917,13 +918,6 @@ class SentryTest { .deserialize(previousSessionFile.bufferedReader(), Session::class.java)!! .environment, ) - - it.addIntegration { scopes, _ -> - // this is just a hack to trigger the previousSessionFlush latch, so the finalizer - // does not time out waiting. We have to do it as integration, because this is where - // the scopes is already initialized - scopes.startSession() - } it.sessionFlushTimeoutMillis = 100 } @@ -963,9 +957,6 @@ class SentryTest { } } - // to trigger previous session flush - Sentry.startSession() - await.untilTrue(triggered) assertFalse(previousSessionFile.exists()) } @@ -1540,6 +1531,73 @@ class SentryTest { verify(mockDialogHandler).showDialog(eq(associatedEventId), eq(configurator)) } + @Test + fun `init calls movePreviousSession before registering integrations`() { + val mockExecutorService = mock() + val mockIntegration = mock() + + initForTest { + it.dsn = dsn + it.cacheDirPath = getTempPath() + it.isEnableAutoSessionTracking = true + it.executorService = mockExecutorService + it.addIntegration(mockIntegration) + } + + // Verify that movePreviousSession is called before integration registration + // This ensures the session move happens early in the init process + val inOrder = inOrder(mockExecutorService, mockIntegration) + inOrder.verify(mockExecutorService).submit(any()) + inOrder.verify(mockIntegration).register(any(), any()) + } + + @Test + fun `init moves previous session to its own file`() { + val cacheDir = tmpDir.newFolder().absolutePath + lateinit var currentSessionFile: File + + val previousSessionFile = EnvelopeCache.getPreviousSessionFile(cacheDir) + assertFalse(previousSessionFile.exists()) + + initForTest { + it.dsn = dsn + it.isDebug = true + it.setLogger(SystemOutLogger()) + + it.release = "io.sentry.sample@2.0" + + it.cacheDirPath = tmpDir.newFolder().absolutePath + it.executorService = ImmediateExecutorService() + + currentSessionFile = EnvelopeCache.getCurrentSessionFile(it.cacheDirPath!!) + currentSessionFile.parentFile.mkdirs() + it.serializer.serialize( + Session(null, null, "release", "io.sentry.samples@2.0"), + currentSessionFile.bufferedWriter(), + ) + assertEquals( + "release", + it.serializer + .deserialize(currentSessionFile.bufferedReader(), Session::class.java)!! + .environment, + ) + + it.addIntegration { scopes, _ -> + // this is just a hack to assert previous session has been moved to its own file. + // Integrations are being registered after moving but before finalizing previous session + // (== deleting the file) so we can check it's been moved here + assertFalse(currentSessionFile.exists()) + assertTrue(previousSessionFile.exists()) + assertEquals( + "release", + it.serializer + .deserialize(previousSessionFile.bufferedReader(), Session::class.java)!! + .environment, + ) + } + } + } + private class InMemoryOptionsObserver : IOptionsObserver { var release: String? = null private set diff --git a/sentry/src/test/java/io/sentry/cache/EnvelopeCacheTest.kt b/sentry/src/test/java/io/sentry/cache/EnvelopeCacheTest.kt index f824c4fcf62..86fa209cfa2 100644 --- a/sentry/src/test/java/io/sentry/cache/EnvelopeCacheTest.kt +++ b/sentry/src/test/java/io/sentry/cache/EnvelopeCacheTest.kt @@ -404,4 +404,70 @@ class EnvelopeCacheTest { assertEquals(2, cache.directory.list()?.size) } + + @Test + fun `movePreviousSession moves current session file to previous session file`() { + val cache = fixture.getSUT() + + val currentSessionFile = EnvelopeCache.getCurrentSessionFile(fixture.options.cacheDirPath!!) + val previousSessionFile = EnvelopeCache.getPreviousSessionFile(fixture.options.cacheDirPath!!) + + // Create a current session file + currentSessionFile.createNewFile() + currentSessionFile.writeText("current session content") + + assertTrue(currentSessionFile.exists()) + assertFalse(previousSessionFile.exists()) + + // Call movePreviousSession directly + cache.movePreviousSession(currentSessionFile, previousSessionFile) + + // Current file should be moved to previous + assertFalse(currentSessionFile.exists()) + assertTrue(previousSessionFile.exists()) + assertEquals("current session content", previousSessionFile.readText()) + } + + @Test + fun `movePreviousSession does nothing when current session file does not exist`() { + val cache = fixture.getSUT() + + val currentSessionFile = EnvelopeCache.getCurrentSessionFile(fixture.options.cacheDirPath!!) + val previousSessionFile = EnvelopeCache.getPreviousSessionFile(fixture.options.cacheDirPath!!) + + assertFalse(currentSessionFile.exists()) + assertFalse(previousSessionFile.exists()) + + // Call movePreviousSession when no files exist + cache.movePreviousSession(currentSessionFile, previousSessionFile) + + // Nothing should happen + assertFalse(currentSessionFile.exists()) + assertFalse(previousSessionFile.exists()) + } + + @Test + fun `movePreviousSession deletes file and moves session when previous session file already exists`() { + val cache = fixture.getSUT() + + val currentSessionFile = EnvelopeCache.getCurrentSessionFile(fixture.options.cacheDirPath!!) + val previousSessionFile = EnvelopeCache.getPreviousSessionFile(fixture.options.cacheDirPath!!) + + // Create both files + currentSessionFile.createNewFile() + currentSessionFile.writeText("current session content") + previousSessionFile.createNewFile() + previousSessionFile.writeText("existing previous content") + + assertTrue(currentSessionFile.exists()) + assertTrue(previousSessionFile.exists()) + + // Call movePreviousSession when previous already exists + cache.movePreviousSession(currentSessionFile, previousSessionFile) + + // Current session should be moved to previous + assertFalse(currentSessionFile.exists()) + assertTrue(previousSessionFile.exists()) + assertEquals("current session content", previousSessionFile.readText()) + } }