From cdeb79afc749e99250196dafff4ee122c31fbedc Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Thu, 13 Aug 2026 14:45:58 +0200 Subject: [PATCH 1/3] ref(android): Confine replay lifecycle to main thread Serialize replay lifecycle mutations on Android's main thread and keep replay cache cleanup ordered on the replay executor. Remove locks that could block lifecycle callbacks while preserving shutdown ordering. Refs JAVA-665 Co-Authored-By: OpenAI Codex --- .../io/sentry/android/replay/ReplayCache.kt | 87 ++---- .../android/replay/ReplayIntegration.kt | 276 ++++++++++-------- .../replay/capture/BaseCaptureStrategy.kt | 13 +- .../sentry/android/replay/ReplayCacheTest.kt | 42 --- .../android/replay/ReplayIntegrationTest.kt | 82 ++++++ .../sentry/android/replay/ReplaySmokeTest.kt | 41 --- .../capture/SessionCaptureStrategyTest.kt | 44 ++- .../replay/util/ReplayShadowMediaCodec.kt | 14 - sentry/api/sentry.api | 1 - .../util/AutoClosableReentrantLock.java | 14 - .../util/AutoClosableReentrantLockTest.kt | 50 ---- 11 files changed, 306 insertions(+), 358 deletions(-) diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayCache.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayCache.kt index 92d4a0c4018..541d6a3b439 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayCache.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayCache.kt @@ -22,7 +22,6 @@ import java.io.File import java.io.StringReader import java.util.Date import java.util.LinkedList -import java.util.concurrent.TimeUnit.MILLISECONDS import java.util.concurrent.atomic.AtomicBoolean /** @@ -41,7 +40,6 @@ import java.util.concurrent.atomic.AtomicBoolean public class ReplayCache(private val options: SentryOptions, private val replayId: SentryId) : Closeable { private val isClosed = AtomicBoolean(false) - private val encoderLock = AutoClosableReentrantLock() private val lock = AutoClosableReentrantLock() private val framesLock = AutoClosableReentrantLock() private var encoder: SimpleVideoEncoder? = null @@ -152,28 +150,26 @@ public class ReplayCache(private val options: SentryOptions, private val replayI } encoder = - encoderLock.acquire().use { - SimpleVideoEncoder( - options, - MuxerConfig( - file = videoFile, - recordingHeight = height, - recordingWidth = width, - frameRate = frameRate, - bitRate = bitRate, - ), - ) - .apply { - // the constructor already opened the MediaMuxer, so release it if start() fails, - // otherwise the encoder is never assigned and its resources leak (CloseGuard warning) - try { - start() - } catch (t: Throwable) { - release() - throw t - } + SimpleVideoEncoder( + options, + MuxerConfig( + file = videoFile, + recordingHeight = height, + recordingWidth = width, + frameRate = frameRate, + bitRate = bitRate, + ), + ) + .apply { + // the constructor already opened the MediaMuxer, so release it if start() fails, + // otherwise the encoder is never assigned and its resources leak (CloseGuard warning) + try { + start() + } catch (t: Throwable) { + release() + throw t } - } + } val step = 1000 / frameRate.toLong() var frameCount = 0 @@ -209,20 +205,15 @@ public class ReplayCache(private val options: SentryOptions, private val replayI if (frameCount == 0) { options.logger.log(DEBUG, "Generated a video with no frames, not capturing a replay segment") - encoderLock.acquire().use { - encoder?.release() - encoder = null - } + encoder?.release() + encoder = null deleteFile(videoFile) return null } - var videoDuration: Long - encoderLock.acquire().use { - encoder?.release() - videoDuration = encoder?.duration ?: 0 - encoder = null - } + encoder?.release() + val videoDuration = encoder?.duration ?: 0 + encoder = null rotate(until = (from + duration)) @@ -235,7 +226,7 @@ public class ReplayCache(private val options: SentryOptions, private val replayI } return try { val bitmap = BitmapFactory.decodeFile(frame.screenshot.absolutePath) - encoderLock.acquire().use { encoder?.encode(bitmap) } + encoder?.encode(bitmap) bitmap.recycle() true } catch (e: Throwable) { @@ -281,27 +272,10 @@ public class ReplayCache(private val options: SentryOptions, private val replayI } override fun close() { - // close() is called inline from the lifecycle path (ReplayIntegration.stop/close), which holds - // its own lock, so blocking here can freeze the main thread. If the encoder is wedged in a - // native MediaCodec call we'd never get the lock, so we give up instead: the already-dead codec - // is not released (leaking a native handle), which beats an ANR. try { - val token = encoderLock.tryAcquire(ENCODER_RELEASE_TIMEOUT_MS, MILLISECONDS) - if (token == null) { - options.logger.log( - WARNING, - "Timed out waiting for the video encoder, skipping its release to not block the caller", - ) - } else { - token.use { - encoder?.release() - encoder = null - } - } - } catch (e: InterruptedException) { - Thread.currentThread().interrupt() + encoder?.release() + encoder = null } finally { - // has to happen on all paths, callers rely on it to stop persisting segment values isClosed.set(true) } } @@ -333,13 +307,6 @@ public class ReplayCache(private val options: SentryOptions, private val replayI } internal companion object { - /** - * How long [close] waits for the video encoder to become available. Below Android's ~5s ANR - * budget, and above the encoder's own bail-out (see MAX_EOS_STALL_ITERATIONS), so an encoder - * that's merely slow is still awaited rather than abandoned. - */ - private const val ENCODER_RELEASE_TIMEOUT_MS = 2000L - internal const val ONGOING_SEGMENT = ".ongoing_segment" internal const val SEGMENT_KEY_HEIGHT = "config.height" diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayIntegration.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayIntegration.kt index 98333260c7d..b794b1b2144 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayIntegration.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayIntegration.kt @@ -47,7 +47,6 @@ import io.sentry.protocol.SentryId import io.sentry.transport.ICurrentDateProvider import io.sentry.transport.RateLimiter import io.sentry.transport.RateLimiter.IRateLimitObserver -import io.sentry.util.AutoClosableReentrantLock import io.sentry.util.FileUtils import io.sentry.util.HintUtils import io.sentry.util.IntegrationUtils.addIntegrationToSdkVersion @@ -55,8 +54,10 @@ import io.sentry.util.Random import java.io.Closeable import java.io.File import java.util.LinkedList +import java.util.concurrent.CountDownLatch import java.util.concurrent.Executors import java.util.concurrent.ThreadFactory +import java.util.concurrent.TimeUnit.MILLISECONDS import java.util.concurrent.atomic.AtomicBoolean public class ReplayIntegration( @@ -121,8 +122,8 @@ public class ReplayIntegration( internal val persistingExecutor by lazyPersistingExecutor internal val isEnabled = AtomicBoolean(false) - internal val isManualPause = AtomicBoolean(false) - private var captureStrategy: CaptureStrategy? = null + internal var isManualPause = false + @Volatile private var captureStrategy: CaptureStrategy? = null public val replayCacheDir: File? get() = captureStrategy?.replayCacheDir @@ -131,7 +132,6 @@ public class ReplayIntegration( private var replayCaptureStrategyProvider: ((isFullSession: Boolean) -> CaptureStrategy)? = null private var mainLooperHandler: MainLooperHandler = MainLooperHandler() private var gestureRecorderProvider: (() -> GestureRecorder)? = null - internal val lifecycleLock = AutoClosableReentrantLock() private val lifecycle = ReplayLifecycle() override fun register(scopes: IScopes, options: SentryOptions) { @@ -169,82 +169,84 @@ public class ReplayIntegration( lifecycle.currentState >= STARTED && lifecycle.currentState < STOPPED override fun start() { - lifecycleLock.acquire().use { - if (!isEnabled.get()) { - return - } - - if (!lifecycle.isAllowed(STARTED)) { - options.logger.log( - DEBUG, - "Session replay is already being recorded, not starting a new one", - ) - return - } + postOnMainThread { startInternal() } + } - val isFullSession = random.sample(options.sessionReplay.sessionSampleRate) - if (!isFullSession && !options.sessionReplay.isSessionReplayForErrorsEnabled) { - options.logger.log( - INFO, - "Session replay is not started, full session was not sampled and onErrorSampleRate is not specified", - ) - return - } + private fun startInternal() { + if (!isEnabled.get()) { + return + } - lifecycle.currentState = STARTED - captureStrategy = - replayCaptureStrategyProvider?.invoke(isFullSession) - ?: if (isFullSession) { - SessionCaptureStrategy( - options, - scopes, - dateProvider, - replayExecutor, - persistingExecutor, - replayCacheProvider, - ) - } else { - BufferCaptureStrategy( - options, - scopes, - dateProvider, - random, - replayExecutor, - persistingExecutor, - replayCacheProvider, - ) - } - recorder?.start() - captureStrategy?.start() + if (!lifecycle.isAllowed(STARTED)) { + options.logger.log( + DEBUG, + "Session replay is already being recorded, not starting a new one", + ) + return + } - registerRootViewListeners() + val isFullSession = random.sample(options.sessionReplay.sessionSampleRate) + if (!isFullSession && !options.sessionReplay.isSessionReplayForErrorsEnabled) { + options.logger.log( + INFO, + "Session replay is not started, full session was not sampled and onErrorSampleRate is not specified", + ) + return } + + lifecycle.currentState = STARTED + captureStrategy = + replayCaptureStrategyProvider?.invoke(isFullSession) + ?: if (isFullSession) { + SessionCaptureStrategy( + options, + scopes, + dateProvider, + replayExecutor, + persistingExecutor, + replayCacheProvider, + ) + } else { + BufferCaptureStrategy( + options, + scopes, + dateProvider, + random, + replayExecutor, + persistingExecutor, + replayCacheProvider, + ) + } + recorder?.start() + captureStrategy?.start() + + registerRootViewListeners() } override fun resume() { - isManualPause.set(false) - resumeInternal() + postOnMainThread { + isManualPause = false + resumeInternal() + } } private fun resumeInternal() { - lifecycleLock.acquire().use { - if (!isEnabled.get() || !lifecycle.isAllowed(RESUMED)) { - return - } - - if ( - isManualPause.get() || - lastKnownConnectionStatus == DISCONNECTED || - scopes?.rateLimiter?.isActiveForCategory(All) == true || - scopes?.rateLimiter?.isActiveForCategory(Replay) == true - ) { - return - } + if (!isEnabled.get() || !lifecycle.isAllowed(RESUMED)) { + return + } - lifecycle.currentState = RESUMED - captureStrategy?.resume() - recorder?.resume() + if ( + isManualPause || + lastKnownConnectionStatus == DISCONNECTED || + scopes?.rateLimiter?.isActiveForCategory(All) == true || + scopes?.rateLimiter?.isActiveForCategory(Replay) == true + ) { + return } + + lifecycle.currentState = RESUMED + captureStrategy?.resume() + recorder?.resume() } override fun captureReplay(isTerminating: Boolean?) { @@ -277,8 +279,10 @@ public class ReplayIntegration( override fun getBreadcrumbConverter(): ReplayBreadcrumbConverter = replayBreadcrumbConverter override fun pause() { - isManualPause.set(true) - pauseInternal() + postOnMainThread { + isManualPause = true + pauseInternal() + } } override fun enableDebugMaskingOverlay() { @@ -306,31 +310,31 @@ public class ReplayIntegration( } private fun pauseInternal() { - lifecycleLock.acquire().use { - if (!isEnabled.get() || !lifecycle.isAllowed(PAUSED)) { - return - } - - recorder?.pause() - captureStrategy?.pause() - lifecycle.currentState = PAUSED + if (!isEnabled.get() || !lifecycle.isAllowed(PAUSED)) { + return } + + recorder?.pause() + captureStrategy?.pause() + lifecycle.currentState = PAUSED } override fun stop() { - lifecycleLock.acquire().use { - if (!isEnabled.get() || !lifecycle.isAllowed(STOPPED)) { - return - } + postOnMainThread { stopInternal() } + } - unregisterRootViewListeners() - recorder?.reset() - recorder?.stop() - gestureRecorder?.stop() - captureStrategy?.stop() - captureStrategy = null - lifecycle.currentState = STOPPED + private fun stopInternal() { + if (!isEnabled.get() || !lifecycle.isAllowed(STOPPED)) { + return } + + unregisterRootViewListeners() + recorder?.reset() + recorder?.stop() + gestureRecorder?.stop() + captureStrategy?.stop() + captureStrategy = null + lifecycle.currentState = STOPPED } override fun onScreenshotRecorded(bitmap: Bitmap) { @@ -380,30 +384,37 @@ public class ReplayIntegration( } override fun close() { - lifecycleLock.acquire().use { - if (!isEnabled.get() || !lifecycle.isAllowed(CLOSED)) { - return - } + if (!isEnabled.get()) { + return + } - options.connectionStatusProvider.removeConnectionStatusObserver(this) - scopes?.rateLimiter?.removeRateLimitObserver(this) - stop() - recorder?.close() - recorder = null - rootViewsSpy.close() - lifecycle.currentState = CLOSED + val isMainThread = Looper.myLooper() == Looper.getMainLooper() + val closeCompleted = if (isMainThread) null else CountDownLatch(1) + postOnMainThread { + try { + closeInternal() + } finally { + closeCompleted?.countDown() + } } - // shutdown outside lock — awaiting termination while holding lifecycleLock deadlocks - // if any executor task tries to acquire the same lock + if (closeCompleted != null) { + // Wait until main-thread teardown queues replay cleanup before shutting down its executors. + try { + closeCompleted.await(options.shutdownTimeoutMillis, MILLISECONDS) + } catch (e: InterruptedException) { + Thread.currentThread().interrupt() + } + } + if (lazyReplayExecutor.isInitialized()) { - if (options.threadChecker.isMainThread) { + if (isMainThread) { replayExecutor.gracefulShutdown() } else { replayExecutor.shutdown() } } if (lazyPersistingExecutor.isInitialized()) { - if (options.threadChecker.isMainThread) { + if (isMainThread) { persistingExecutor.gracefulShutdown() } else { persistingExecutor.shutdown() @@ -411,32 +422,50 @@ public class ReplayIntegration( } } + private fun closeInternal() { + if (!lifecycle.isAllowed(CLOSED)) { + return + } + + options.connectionStatusProvider.removeConnectionStatusObserver(this) + scopes?.rateLimiter?.removeRateLimitObserver(this) + stopInternal() + recorder?.close() + recorder = null + rootViewsSpy.close() + lifecycle.currentState = CLOSED + } + override fun onConnectionStatusChanged(status: ConnectionStatus) { lastKnownConnectionStatus = status - if (captureStrategy !is SessionCaptureStrategy) { - // we only want to stop recording when offline for session mode - return - } + postOnMainThread { + if (captureStrategy !is SessionCaptureStrategy) { + // we only want to stop recording when offline for session mode + return@postOnMainThread + } - if (status == DISCONNECTED) { - pauseInternal() - } else { - // being positive for other states, even if it's NO_PERMISSION - resumeInternal() + if (status == DISCONNECTED) { + pauseInternal() + } else { + // being positive for other states, even if it's NO_PERMISSION + resumeInternal() + } } } override fun onRateLimitChanged(rateLimiter: RateLimiter) { - if (captureStrategy !is SessionCaptureStrategy) { - // we only want to stop recording when rate-limited for session mode - return - } + postOnMainThread { + if (captureStrategy !is SessionCaptureStrategy) { + // we only want to stop recording when rate-limited for session mode + return@postOnMainThread + } - if (rateLimiter.isActiveForCategory(All) || rateLimiter.isActiveForCategory(Replay)) { - pauseInternal() - } else { - resumeInternal() + if (rateLimiter.isActiveForCategory(All) || rateLimiter.isActiveForCategory(Replay)) { + pauseInternal() + } else { + resumeInternal() + } } } @@ -447,9 +476,8 @@ public class ReplayIntegration( captureStrategy?.onTouchEvent(event) } - // Runs [block] on the main thread. If already there, executes inline; otherwise posts via - // the main looper handler. Prevents deadlocks when lifecycle-lock-acquiring code (e.g. - // checkCanRecord -> pauseInternal) is called from the replay executor thread. + // Runs [block] on the main thread. If already there, executes inline; otherwise posts via the + // main looper handler so lifecycle mutations are serialized without locking. private inline fun postOnMainThread(crossinline block: () -> Unit) { if (Looper.myLooper() == Looper.getMainLooper()) { block() diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BaseCaptureStrategy.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BaseCaptureStrategy.kt index f505d21a151..deb51ecc006 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BaseCaptureStrategy.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BaseCaptureStrategy.kt @@ -119,10 +119,15 @@ internal abstract class BaseCaptureStrategy( override fun pause() = Unit override fun stop() { - cache?.close() - replayStartTimestamp.set(0) - segmentTimestamp = null - currentReplayId = SentryId.EMPTY_ID + // Keep cleanup behind queued frames; a later start uses a new capture strategy instance. + replayExecutor.submit( + ReplayRunnable("$TAG.stop") { + cache?.close() + replayStartTimestamp.set(0) + segmentTimestamp = null + currentReplayId = SentryId.EMPTY_ID + } + ) } protected fun createSegmentInternal( diff --git a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayCacheTest.kt b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayCacheTest.kt index 96e5a926af4..b3f3307837b 100644 --- a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayCacheTest.kt +++ b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayCacheTest.kt @@ -64,8 +64,6 @@ class ReplayCacheTest { ReplayShadowMediaCodec.framesToEncode = 5 ReplayShadowMediaCodec.throwOnStart = false ReplayShadowMediaCodec.neverSignalEos = false - ReplayShadowMediaCodec.blockOnDequeue = null - ReplayShadowMediaCodec.blockedOnDequeue = CountDownLatch(1) ReplayShadowMediaCodec.released = false ShadowBitmapFactory.setAllowInvalidImageData(true) } @@ -691,46 +689,6 @@ class ReplayCacheTest { assertThat(error.get()).isNull() } - @Test - fun `close does not block when the encoder is wedged, and still marks the cache closed`() { - val wedge = CountDownLatch(1) - ReplayShadowMediaCodec.blockOnDequeue = wedge - val replayCache = fixture.getSut(tmpDir) - - val bitmap = Bitmap.createBitmap(1, 1, ARGB_8888) - replayCache.addFrame(bitmap, 1) - - // parks inside MediaCodec while holding the encoder lock - val encoder = - thread(isDaemon = true) { replayCache.createVideoOf(1000L, 0L, 0, 100, 200, 1, 20_000) } - try { - assertWithMessage("the encoder never reached dequeueOutputBuffer") - .that(ReplayShadowMediaCodec.blockedOnDequeue.await(30, SECONDS)) - .isTrue() - - // on a separate thread so a regression fails the test instead of hanging the run - val closed = CountDownLatch(1) - thread(isDaemon = true) { - replayCache.close() - closed.countDown() - } - assertWithMessage("close() blocked on the wedged encoder") - .that(closed.await(30, SECONDS)) - .isTrue() - - // giving up on the lock still counts as closed, otherwise we'd keep persisting segments - replayCache.persistSegmentValues(SEGMENT_KEY_ID, "0") - assertThat(File(replayCache.replayCacheDir, ONGOING_SEGMENT).exists()).isFalse() - - assertWithMessage("encoder should not be released when the lock times out") - .that(ReplayShadowMediaCodec.released) - .isFalse() - } finally { - wedge.countDown() - encoder.join(SECONDS.toMillis(10)) - } - } - @Test fun `createVideoOf releases the encoder even when EOS is never signalled`() { ReplayShadowMediaCodec.neverSignalEos = true diff --git a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationTest.kt b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationTest.kt index 32b7f4e9285..1c8c6ece20f 100644 --- a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationTest.kt +++ b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationTest.kt @@ -4,8 +4,10 @@ import android.content.Context import android.graphics.Bitmap import android.graphics.Bitmap.CompressFormat.JPEG import android.graphics.Bitmap.Config.ARGB_8888 +import android.os.Looper import androidx.test.core.app.ApplicationProvider import androidx.test.ext.junit.runners.AndroidJUnit4 +import com.google.common.truth.Truth.assertThat import io.sentry.Breadcrumb import io.sentry.DateUtils import io.sentry.Hint @@ -50,11 +52,14 @@ import io.sentry.transport.RateLimiter import io.sentry.util.Random import java.io.ByteArrayOutputStream import java.io.File +import java.util.concurrent.CountDownLatch +import java.util.concurrent.TimeUnit import kotlin.test.BeforeTest import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFalse import kotlin.test.assertTrue +import org.awaitility.kotlin.await import org.junit.Rule import org.junit.rules.TemporaryFolder import org.junit.runner.RunWith @@ -73,6 +78,7 @@ import org.mockito.kotlin.reset import org.mockito.kotlin.times import org.mockito.kotlin.verify import org.mockito.kotlin.whenever +import org.robolectric.Shadows.shadowOf import org.robolectric.annotation.Config @RunWith(AndroidJUnit4::class) @@ -444,6 +450,82 @@ class ReplayIntegrationTest { assertFalse(replay.isRecording()) } + @Test + fun `background lifecycle calls run on main thread in order`() { + val calls = mutableListOf() + val captureStrategy = + mock { + doAnswer { calls += "start" }.whenever(mock).start(any(), any(), anyOrNull()) + doAnswer { calls += "pause" }.whenever(mock).pause() + doAnswer { calls += "resume" }.whenever(mock).resume() + doAnswer { calls += "stop" }.whenever(mock).stop() + } + val replay = fixture.getSut(context, replayCaptureStrategyProvider = { captureStrategy }) + replay.register(fixture.scopes, fixture.options) + + Thread { + replay.start() + replay.pause() + replay.resume() + replay.stop() + } + .apply { + start() + join() + } + + assertThat(calls).isEmpty() + shadowOf(Looper.getMainLooper()).idle() + assertThat(calls).containsExactly("start", "pause", "resume", "stop").inOrder() + } + + @Test + fun `background close waits for main thread teardown`() { + fixture.options.shutdownTimeoutMillis = TimeUnit.SECONDS.toMillis(30) + val recorder = mock() + val replay = fixture.getSut(context, recorderProvider = { recorder }) + replay.register(fixture.scopes, fixture.options) + replay.start() + + val closeThread = Thread { replay.close() }.apply { start() } + await.until { closeThread.state == Thread.State.TIMED_WAITING } + + verify(recorder, never()).close() + shadowOf(Looper.getMainLooper()).idle() + closeThread.join(TimeUnit.SECONDS.toMillis(2)) + + assertThat(closeThread.isAlive).isFalse() + verify(recorder).close() + } + + @Test + fun `main thread close does not wait for replay executor`() { + val replay = fixture.getSut(context, replayCaptureStrategyProvider = { mock() }) + replay.register(fixture.scopes, fixture.options) + replay.start() + + val running = CountDownLatch(1) + val release = CountDownLatch(1) + val finished = CountDownLatch(1) + replay.replayExecutor.submit { + running.countDown() + try { + release.await() + } finally { + finished.countDown() + } + } + assertThat(running.await(10, TimeUnit.SECONDS)).isTrue() + + try { + replay.close() + assertThat(finished.count).isEqualTo(1L) + } finally { + release.countDown() + } + assertThat(finished.await(10, TimeUnit.SECONDS)).isTrue() + } + @Test fun `onConfigurationChanged does nothing when not recording`() { val captureStrategy = mock() diff --git a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplaySmokeTest.kt b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplaySmokeTest.kt index b5e15b5534f..b84b1b53347 100644 --- a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplaySmokeTest.kt +++ b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplaySmokeTest.kt @@ -25,14 +25,12 @@ import io.sentry.transport.CurrentDateProvider import io.sentry.transport.ICurrentDateProvider import io.sentry.transport.RateLimiter import java.time.Duration -import java.util.concurrent.CountDownLatch import java.util.concurrent.Executors import java.util.concurrent.TimeUnit import java.util.concurrent.atomic.AtomicBoolean import kotlin.test.BeforeTest import kotlin.test.assertEquals import kotlin.test.assertNotEquals -import kotlin.test.assertTrue import org.awaitility.core.ConditionTimeoutException import org.awaitility.kotlin.await import org.junit.Rule @@ -256,45 +254,6 @@ class ReplaySmokeTest { assertNotEquals(falseReplay.rootViewsSpy, replay.rootViewsSpy) assertEquals(0, falseReplay.rootViewsSpy.listeners.size) } - - @Test - fun `close does not deadlock when executor task is waiting on lifecycleLock`() { - fixture.options.sessionReplay.sessionSampleRate = 1.0 - fixture.options.cacheDirPath = tmpDir.newFolder().absolutePath - - val replay = fixture.getSut(context) - replay.register(fixture.scopes, fixture.options) - replay.start() - - val taskBlocked = CountDownLatch(1) - val lockReleased = CountDownLatch(1) - - // hold lifecycleLock on this thread - val token = replay.lifecycleLock.acquire() - - // submit a task on the executor that tries to acquire the same lock — it will block - replay.replayExecutor.submit { - taskBlocked.countDown() - replay.lifecycleLock.acquire().use {} - } - - // wait for the executor task to actually be running and blocked - assertTrue(taskBlocked.await(2, TimeUnit.SECONDS)) - - // release the lock, then close — if shutdown were inside the lock this would deadlock - token.close() - - // close() must complete within a reasonable time - val closedInTime = AtomicBoolean(false) - val closeThread = Thread { - replay.close() - closedInTime.set(true) - } - closeThread.start() - closeThread.join(5000) - - assertTrue(closedInTime.get(), "close() deadlocked") - } } private class ExampleActivity : Activity() { diff --git a/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/SessionCaptureStrategyTest.kt b/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/SessionCaptureStrategyTest.kt index fc2354eb1c0..3e5198bea01 100644 --- a/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/SessionCaptureStrategyTest.kt +++ b/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/SessionCaptureStrategyTest.kt @@ -1,6 +1,7 @@ package io.sentry.android.replay.capture import android.graphics.Bitmap +import com.google.common.truth.Truth.assertThat import io.sentry.Breadcrumb import io.sentry.DateUtils import io.sentry.IScopes @@ -33,6 +34,7 @@ import io.sentry.transport.CurrentDateProvider import io.sentry.transport.ICurrentDateProvider import java.io.File import java.util.Date +import java.util.concurrent.ScheduledExecutorService import kotlin.test.Test import kotlin.test.assertContentEquals import kotlin.test.assertEquals @@ -108,20 +110,21 @@ class SessionCaptureStrategyTest { fun getSut( dateProvider: ICurrentDateProvider = CurrentDateProvider.getInstance(), replayCacheDir: File? = null, + replayExecutor: ScheduledExecutorService = mock { + doAnswer { invocation -> + (invocation.arguments[0] as Runnable).run() + null + } + .whenever(it) + .submit(any()) + }, ): SessionCaptureStrategy { replayCacheDir?.let { whenever(replayCache.replayCacheDir).thenReturn(it) } return SessionCaptureStrategy( options, scopes, dateProvider, - mock { - doAnswer { invocation -> - (invocation.arguments[0] as Runnable).run() - null - } - .whenever(it) - .submit(any()) - }, + replayExecutor, mock { doAnswer { invocation -> (invocation.arguments[0] as Runnable).run() @@ -213,6 +216,31 @@ class SessionCaptureStrategyTest { verify(fixture.replayCache).close() } + @Test + fun `stop closes cache after queued replay work`() { + val tasks = mutableListOf() + val calls = mutableListOf() + val replayExecutor = + mock { + doAnswer { + tasks += it.arguments[0] as Runnable + null + } + .whenever(mock) + .submit(any()) + } + doAnswer { calls += "close" }.whenever(fixture.replayCache).close() + val strategy = fixture.getSut(replayExecutor = replayExecutor) + strategy.start() + replayExecutor.submit(Runnable { calls += "encode" }) + + strategy.stop() + + verify(fixture.replayCache, never()).close() + tasks.forEach(Runnable::run) + assertThat(calls).containsExactly("encode", "close").inOrder() + } + @Test fun `captureReplay does nothing for non-crashed event`() { val strategy = fixture.getSut() diff --git a/sentry-android-replay/src/test/java/io/sentry/android/replay/util/ReplayShadowMediaCodec.kt b/sentry-android-replay/src/test/java/io/sentry/android/replay/util/ReplayShadowMediaCodec.kt index e0e13076ea0..114d9e5fd24 100644 --- a/sentry-android-replay/src/test/java/io/sentry/android/replay/util/ReplayShadowMediaCodec.kt +++ b/sentry-android-replay/src/test/java/io/sentry/android/replay/util/ReplayShadowMediaCodec.kt @@ -3,7 +3,6 @@ package io.sentry.android.replay.util import android.media.MediaCodec import android.media.MediaCodec.BufferInfo import java.nio.ByteBuffer -import java.util.concurrent.CountDownLatch import java.util.concurrent.TimeUnit.MICROSECONDS import java.util.concurrent.TimeUnit.MILLISECONDS import java.util.concurrent.atomic.AtomicBoolean @@ -21,15 +20,6 @@ class ReplayShadowMediaCodec : ShadowMediaCodec() { /** Simulates an encoder that never emits [MediaCodec.BUFFER_FLAG_END_OF_STREAM]. */ var neverSignalEos = false - /** - * When set, [dequeueOutputBuffer] awaits this latch, simulating a native call that never - * returns. [blockedOnDequeue] is counted down right before, so tests can wait until the codec - * is actually stuck. - */ - var blockOnDequeue: CountDownLatch? = null - - var blockedOnDequeue = CountDownLatch(1) - /** Set to `true` when [release] is called. */ var released = false } @@ -61,10 +51,6 @@ class ReplayShadowMediaCodec : ShadowMediaCodec() { @Implementation fun dequeueOutputBuffer(info: BufferInfo, timeoutUs: Long): Int { - blockOnDequeue?.let { - blockedOnDequeue.countDown() - it.await() - } val encoderStatus = super.native_dequeueOutputBuffer(info, timeoutUs) super.validateOutputByteBuffer(getOutputBuffers(), encoderStatus, info) if (encoderStatus == MediaCodec.INFO_TRY_AGAIN_LATER && !encoded.getAndSet(true)) { diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 54729fdb12b..915e4b95f6a 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -7646,7 +7646,6 @@ public final class io/sentry/util/AutoClosableReentrantLock : io/sentry/ISentryL public fun ()V public fun acquire ()Lio/sentry/ISentryLifecycleToken; public fun close ()V - public fun tryAcquire (JLjava/util/concurrent/TimeUnit;)Lio/sentry/ISentryLifecycleToken; } public final class io/sentry/util/CheckInUtils { diff --git a/sentry/src/main/java/io/sentry/util/AutoClosableReentrantLock.java b/sentry/src/main/java/io/sentry/util/AutoClosableReentrantLock.java index 617c1a5b4fa..cf53d860e08 100644 --- a/sentry/src/main/java/io/sentry/util/AutoClosableReentrantLock.java +++ b/sentry/src/main/java/io/sentry/util/AutoClosableReentrantLock.java @@ -1,7 +1,6 @@ package io.sentry.util; import io.sentry.ISentryLifecycleToken; -import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReferenceFieldUpdater; import java.util.concurrent.locks.ReentrantLock; import org.jetbrains.annotations.ApiStatus; @@ -39,19 +38,6 @@ public final class AutoClosableReentrantLock implements ISentryLifecycleToken { return this; } - /** - * Like {@link #acquire()}, but gives up after {@code timeout}. Use it when blocking forever would - * be worse than not doing the work at all, e.g. on a path that can run on the main thread. - * - * @return the token (this instance) if the lock was acquired, or {@code null} if it wasn't. A - * {@code null} return means the lock is not held, so {@link #close()} must not be - * called for it. - */ - public @Nullable ISentryLifecycleToken tryAcquire( - final long timeout, final @NotNull TimeUnit unit) throws InterruptedException { - return getOrCreateLock().tryLock(timeout, unit) ? this : null; - } - @Override public void close() { Objects.requireNonNull(lock, "close() called before acquire()").unlock(); diff --git a/sentry/src/test/java/io/sentry/util/AutoClosableReentrantLockTest.kt b/sentry/src/test/java/io/sentry/util/AutoClosableReentrantLockTest.kt index b46cfbaea50..943a2c2bf70 100644 --- a/sentry/src/test/java/io/sentry/util/AutoClosableReentrantLockTest.kt +++ b/sentry/src/test/java/io/sentry/util/AutoClosableReentrantLockTest.kt @@ -1,6 +1,5 @@ package io.sentry.util -import com.google.common.truth.Truth.assertThat import java.util.concurrent.CountDownLatch import java.util.concurrent.TimeUnit import java.util.concurrent.atomic.AtomicInteger @@ -42,55 +41,6 @@ class AutoClosableReentrantLockTest { assertFalse(lock.isLocked) } - @Test - fun `tryAcquire returns the lock itself as the token when free`() { - val lock = AutoClosableReentrantLock() - val token = lock.tryAcquire(1, TimeUnit.SECONDS) - assertThat(token).isSameInstanceAs(lock) - token!!.use { assertThat(lock.isLocked).isTrue() } - assertThat(lock.isLocked).isFalse() - } - - @Test - fun `tryAcquire does not allocate the underlying lock until first use`() { - val lock = AutoClosableReentrantLock() - assertThat(lock.isLockAllocated).isFalse() - lock.tryAcquire(1, TimeUnit.SECONDS)!!.use {} - assertThat(lock.isLockAllocated).isTrue() - } - - @Test - fun `tryAcquire returns null when another thread holds the lock past the timeout`() { - val lock = AutoClosableReentrantLock() - val acquired = CountDownLatch(1) - val release = CountDownLatch(1) - val holder = Thread { - lock.acquire().use { - acquired.countDown() - release.await() - } - } - holder.start() - try { - assertThat(acquired.await(10, TimeUnit.SECONDS)).isTrue() - assertThat(lock.tryAcquire(10, TimeUnit.MILLISECONDS)).isNull() - } finally { - release.countDown() - holder.join(TimeUnit.SECONDS.toMillis(10)) - } - assertThat(lock.isLocked).isFalse() - } - - @Test - fun `tryAcquire is reentrant from the same thread`() { - val lock = AutoClosableReentrantLock() - lock.acquire().use { - lock.tryAcquire(0, TimeUnit.MILLISECONDS)!!.use { assertThat(lock.isLocked).isTrue() } - assertThat(lock.isLocked).isTrue() - } - assertThat(lock.isLocked).isFalse() - } - @Test fun `mutually excludes concurrent threads`() { val lock = AutoClosableReentrantLock() From 43758cc2e7d4e76f5abc38038bd9ae2fdc3ee01b Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Thu, 13 Aug 2026 15:03:37 +0200 Subject: [PATCH 2/3] changelog --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 247a5e96d31..15fb056e0be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +### Fixes + +- Prevent a class of Session Replay deadlocks by confining lifecycle state changes to Android's main thread ([#5965](https://github.com/getsentry/sentry-java/pull/5965)) + ## 8.53.0 ### Features From c9a7891a5e6a4ab422777cdd8d466b1b80e7669d Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Fri, 14 Aug 2026 20:49:50 +0200 Subject: [PATCH 3/3] ref(android): Simplify replay main-thread state management Keep replay lifecycle state in one atomic value and serialize lifecycle transitions through the main looper. Return the replay ID synchronously so triggering events remain correlated while capture is deferred. Refs JAVA-665 Refs JAVA-656 Co-Authored-By: Codex --- CHANGELOG.md | 4 + .../api/sentry-android-replay.api | 2 +- .../android/replay/ReplayIntegration.kt | 251 ++++++++++++------ .../sentry/android/replay/ReplayLifecycle.kt | 48 ++-- .../replay/capture/BufferCaptureStrategy.kt | 17 -- .../android/replay/ReplayIntegrationTest.kt | 166 +++++++++++- .../ReplayIntegrationWithRecorderTest.kt | 18 +- .../android/replay/ReplayLifecycleTest.kt | 111 +++----- .../capture/BufferCaptureStrategyTest.kt | 20 +- sentry/api/sentry.api | 4 +- .../java/io/sentry/NoOpReplayController.java | 4 +- .../main/java/io/sentry/ReplayController.java | 7 +- .../src/main/java/io/sentry/SentryClient.java | 28 +- .../test/java/io/sentry/SentryClientTest.kt | 37 ++- 14 files changed, 451 insertions(+), 266 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 15fb056e0be..8a1c2d02e28 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ - Prevent a class of Session Replay deadlocks by confining lifecycle state changes to Android's main thread ([#5965](https://github.com/getsentry/sentry-java/pull/5965)) +### Performance + +- Defer starting Session Replay off the SDK initialization critical path ([#5965](https://github.com/getsentry/sentry-java/pull/5965)) + ## 8.53.0 ### Features diff --git a/sentry-android-replay/api/sentry-android-replay.api b/sentry-android-replay/api/sentry-android-replay.api index 3efee26e37d..0e4ce0461b0 100644 --- a/sentry-android-replay/api/sentry-android-replay.api +++ b/sentry-android-replay/api/sentry-android-replay.api @@ -58,7 +58,7 @@ public final class io/sentry/android/replay/ReplayIntegration : io/sentry/IConne public fun (Landroid/content/Context;Lio/sentry/transport/ICurrentDateProvider;)V public fun (Landroid/content/Context;Lio/sentry/transport/ICurrentDateProvider;Lkotlin/jvm/functions/Function0;Lkotlin/jvm/functions/Function1;)V public synthetic fun (Landroid/content/Context;Lio/sentry/transport/ICurrentDateProvider;Lkotlin/jvm/functions/Function0;Lkotlin/jvm/functions/Function1;ILkotlin/jvm/internal/DefaultConstructorMarker;)V - public fun captureReplay (Ljava/lang/Boolean;)V + public fun captureReplay (Ljava/lang/Boolean;)Lio/sentry/protocol/SentryId; public fun close ()V public fun disableDebugMaskingOverlay ()V public fun enableDebugMaskingOverlay ()V diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayIntegration.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayIntegration.kt index b794b1b2144..b9b17e12c8c 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayIntegration.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayIntegration.kt @@ -24,11 +24,11 @@ import io.sentry.SentryLevel.ERROR import io.sentry.SentryLevel.INFO import io.sentry.SentryOptions import io.sentry.TypeCheckHint -import io.sentry.android.replay.ReplayState.CLOSED -import io.sentry.android.replay.ReplayState.PAUSED -import io.sentry.android.replay.ReplayState.RESUMED -import io.sentry.android.replay.ReplayState.STARTED -import io.sentry.android.replay.ReplayState.STOPPED +import io.sentry.android.replay.ReplayLifecycleState.CLOSED +import io.sentry.android.replay.ReplayLifecycleState.PAUSED +import io.sentry.android.replay.ReplayLifecycleState.RESUMED +import io.sentry.android.replay.ReplayLifecycleState.STARTED +import io.sentry.android.replay.ReplayLifecycleState.STOPPED import io.sentry.android.replay.capture.BufferCaptureStrategy import io.sentry.android.replay.capture.CaptureStrategy import io.sentry.android.replay.capture.CaptureStrategy.ReplaySegment @@ -59,6 +59,7 @@ import java.util.concurrent.Executors import java.util.concurrent.ThreadFactory import java.util.concurrent.TimeUnit.MILLISECONDS import java.util.concurrent.atomic.AtomicBoolean +import java.util.concurrent.atomic.AtomicReference public class ReplayIntegration( private val context: Context, @@ -101,13 +102,13 @@ public class ReplayIntegration( this.gestureRecorderProvider = gestureRecorderProvider } - @Volatile private var lastKnownConnectionStatus: ConnectionStatus = ConnectionStatus.UNKNOWN + private var lastKnownConnectionStatus: ConnectionStatus = ConnectionStatus.UNKNOWN private var debugMaskingEnabled: Boolean = false private lateinit var options: SentryOptions private var scopes: IScopes? = null private var recorder: Recorder? = null private var gestureRecorder: GestureRecorder? = null - private val random by lazy { Random() } + private val random = ThreadLocal() internal val rootViewsSpy by lazy { RootViewsSpy.install() } internal val lazyReplayExecutor = lazy { val delegate = Executors.newSingleThreadScheduledExecutor(ReplayExecutorServiceThreadFactory()) @@ -123,16 +124,15 @@ public class ReplayIntegration( internal val isEnabled = AtomicBoolean(false) internal var isManualPause = false - @Volatile private var captureStrategy: CaptureStrategy? = null public val replayCacheDir: File? - get() = captureStrategy?.replayCacheDir + get() = state.get().captureStrategy?.replayCacheDir private var replayBreadcrumbConverter: ReplayBreadcrumbConverter = NoOpReplayBreadcrumbConverter.getInstance() private var replayCaptureStrategyProvider: ((isFullSession: Boolean) -> CaptureStrategy)? = null private var mainLooperHandler: MainLooperHandler = MainLooperHandler() private var gestureRecorderProvider: (() -> GestureRecorder)? = null - private val lifecycle = ReplayLifecycle() + private val state = AtomicReference(ReplayState()) override fun register(scopes: IScopes, options: SentryOptions) { this.options = options @@ -165,11 +165,10 @@ public class ReplayIntegration( finalizePreviousReplay() } - override fun isRecording(): Boolean = - lifecycle.currentState >= STARTED && lifecycle.currentState < STOPPED + override fun isRecording(): Boolean = state.get().isRecording override fun start() { - postOnMainThread { startInternal() } + enqueueOnMainThread { startInternal() } } private fun startInternal() { @@ -177,7 +176,8 @@ public class ReplayIntegration( return } - if (!lifecycle.isAllowed(STARTED)) { + val current = state.get() + if (!current.lifecycleState.isAllowed(STARTED)) { options.logger.log( DEBUG, "Session replay is already being recorded, not starting a new one", @@ -185,7 +185,7 @@ public class ReplayIntegration( return } - val isFullSession = random.sample(options.sessionReplay.sessionSampleRate) + val isFullSession = sample(options.sessionReplay.sessionSampleRate) if (!isFullSession && !options.sessionReplay.isSessionReplayForErrorsEnabled) { options.logger.log( INFO, @@ -194,8 +194,7 @@ public class ReplayIntegration( return } - lifecycle.currentState = STARTED - captureStrategy = + val strategy = replayCaptureStrategyProvider?.invoke(isFullSession) ?: if (isFullSession) { SessionCaptureStrategy( @@ -211,27 +210,36 @@ public class ReplayIntegration( options, scopes, dateProvider, - random, replayExecutor, persistingExecutor, replayCacheProvider, ) } recorder?.start() - captureStrategy?.start() + strategy.start() + val replayId: SentryId? = strategy.currentReplayId + state.set( + ReplayState( + generation = current.generation + 1, + lifecycleState = STARTED, + replayId = replayId ?: SentryId.EMPTY_ID, + captureStrategy = strategy, + ) + ) registerRootViewListeners() } override fun resume() { - postOnMainThread { + enqueueOnMainThread { isManualPause = false resumeInternal() } } private fun resumeInternal() { - if (!isEnabled.get() || !lifecycle.isAllowed(RESUMED)) { + val current = state.get() + if (!isEnabled.get() || !current.lifecycleState.isAllowed(RESUMED)) { return } @@ -244,33 +252,83 @@ public class ReplayIntegration( return } - lifecycle.currentState = RESUMED - captureStrategy?.resume() + current.captureStrategy?.resume() recorder?.resume() + state.set(current.copy(lifecycleState = RESUMED)) } - override fun captureReplay(isTerminating: Boolean?) { - if (!isEnabled.get() || !isRecording()) { - return + override fun captureReplay(isTerminating: Boolean?): SentryId { + val current = state.get() + if (!isEnabled.get() || !current.isRecording) { + return SentryId.EMPTY_ID } - if (SentryId.EMPTY_ID.equals(captureStrategy?.currentReplayId)) { + if (current.replayId == SentryId.EMPTY_ID) { options.logger.log(DEBUG, "Replay id is not set, not capturing for event") + return SentryId.EMPTY_ID + } + + if (current.isBuffering && !sample(options.sessionReplay.onErrorSampleRate)) { + options.logger.log( + INFO, + "Replay wasn't sampled by onErrorSampleRate, not capturing for event", + ) + return SentryId.EMPTY_ID + } + + // Set it synchronously so the event that triggered the flush picks it up before conversion. + scopes?.configureScope { it.replayId = current.replayId } + enqueueOnMainThread { + captureReplayInternal(current.generation, current.replayId, isTerminating == true) + } + return current.replayId + } + + private fun captureReplayInternal( + expectedGeneration: Long, + expectedReplayId: SentryId, + isTerminating: Boolean, + ) { + val current = state.get() + val strategy = current.captureStrategy + if (!current.matches(expectedGeneration, expectedReplayId) || strategy == null) { + options.logger.log( + INFO, + "Replay was stopped or restarted before capture could run, not capturing for event", + ) return } - captureStrategy?.captureReplay( - isTerminating == true, + var activeStrategy: CaptureStrategy = strategy + strategy.captureReplay( + isTerminating, onSegmentSent = { newTimestamp -> - captureStrategy?.currentSegment = captureStrategy?.currentSegment!! + 1 - captureStrategy?.segmentTimestamp = newTimestamp - captureStrategy?.isFlushed = true + enqueueOnMainThread { + val latest = state.get() + // The flush completes asynchronously; ignore it if this replay was stopped, restarted, + // or handed to another strategy in the meantime. + if ( + latest.matches(expectedGeneration, expectedReplayId) && + latest.captureStrategy === activeStrategy + ) { + activeStrategy.currentSegment++ + activeStrategy.segmentTimestamp = newTimestamp + activeStrategy.isFlushed = true + } + } }, ) - captureStrategy = captureStrategy?.convert() + activeStrategy = strategy.convert() + val replayId: SentryId? = activeStrategy.currentReplayId + state.set( + current.copy( + replayId = replayId ?: SentryId.EMPTY_ID, + captureStrategy = activeStrategy, + ) + ) } - override fun getReplayId(): SentryId = captureStrategy?.currentReplayId ?: SentryId.EMPTY_ID + override fun getReplayId(): SentryId = state.get().replayId override fun setBreadcrumbConverter(converter: ReplayBreadcrumbConverter) { replayBreadcrumbConverter = converter @@ -279,7 +337,7 @@ public class ReplayIntegration( override fun getBreadcrumbConverter(): ReplayBreadcrumbConverter = replayBreadcrumbConverter override fun pause() { - postOnMainThread { + enqueueOnMainThread { isManualPause = true pauseInternal() } @@ -296,35 +354,39 @@ public class ReplayIntegration( override fun isDebugMaskingOverlayEnabled(): Boolean = debugMaskingEnabled override fun registerTraceId(traceId: SentryId) { - if (!isEnabled.get() || !isRecording()) { + val current = state.get() + if (!isEnabled.get() || !current.isRecording) { return } - captureStrategy?.registerTraceId(traceId) + current.captureStrategy?.registerTraceId(traceId) } override fun registerSegmentName(segmentName: String) { - if (!isEnabled.get() || !isRecording()) { + val current = state.get() + if (!isEnabled.get() || !current.isRecording) { return } - captureStrategy?.registerSegmentName(segmentName) + current.captureStrategy?.registerSegmentName(segmentName) } private fun pauseInternal() { - if (!isEnabled.get() || !lifecycle.isAllowed(PAUSED)) { + val current = state.get() + if (!isEnabled.get() || !current.lifecycleState.isAllowed(PAUSED)) { return } recorder?.pause() - captureStrategy?.pause() - lifecycle.currentState = PAUSED + current.captureStrategy?.pause() + state.set(current.copy(lifecycleState = PAUSED)) } override fun stop() { - postOnMainThread { stopInternal() } + enqueueOnMainThread { stopInternal() } } private fun stopInternal() { - if (!isEnabled.get() || !lifecycle.isAllowed(STOPPED)) { + val current = state.get() + if (!isEnabled.get() || !current.lifecycleState.isAllowed(STOPPED)) { return } @@ -332,15 +394,20 @@ public class ReplayIntegration( recorder?.reset() recorder?.stop() gestureRecorder?.stop() - captureStrategy?.stop() - captureStrategy = null - lifecycle.currentState = STOPPED + current.captureStrategy?.stop() + state.set( + current.copy( + lifecycleState = STOPPED, + replayId = SentryId.EMPTY_ID, + captureStrategy = null, + ) + ) } override fun onScreenshotRecorded(bitmap: Bitmap) { var screen: String? = null scopes?.configureScope { screen = it.screen?.substringAfterLast('.') } - captureStrategy?.onScreenshotRecorded(bitmap) { frameTimeStamp -> + state.get().captureStrategy?.onScreenshotRecorded(bitmap) { frameTimeStamp -> val observer = options.sessionReplay.frameObserver if (observer != null) { val copy = bitmap.copy(bitmap.config!!, false) @@ -357,13 +424,13 @@ public class ReplayIntegration( } addFrame(bitmap, frameTimeStamp, screen) } - postOnMainThread { checkCanRecord() } + enqueueOnMainThread { checkCanRecord() } } override fun onScreenshotRecorded(screenshot: File, frameTimestamp: Long) { var screen: String? = null scopes?.configureScope { screen = it.screen?.substringAfterLast('.') } - captureStrategy?.onScreenshotRecorded { _ -> + state.get().captureStrategy?.onScreenshotRecorded { _ -> val observer = options.sessionReplay.frameObserver if (observer != null) { val bitmap = BitmapFactory.decodeFile(screenshot.absolutePath) @@ -380,7 +447,7 @@ public class ReplayIntegration( } addFrame(screenshot, frameTimestamp, screen) } - postOnMainThread { checkCanRecord() } + enqueueOnMainThread { checkCanRecord() } } override fun close() { @@ -390,11 +457,15 @@ public class ReplayIntegration( val isMainThread = Looper.myLooper() == Looper.getMainLooper() val closeCompleted = if (isMainThread) null else CountDownLatch(1) - postOnMainThread { - try { - closeInternal() - } finally { - closeCompleted?.countDown() + if (isMainThread) { + closeInternal() + } else { + mainLooperHandler.post { + try { + closeInternal() + } finally { + closeCompleted?.countDown() + } } } if (closeCompleted != null) { @@ -423,7 +494,7 @@ public class ReplayIntegration( } private fun closeInternal() { - if (!lifecycle.isAllowed(CLOSED)) { + if (!state.get().lifecycleState.isAllowed(CLOSED)) { return } @@ -433,16 +504,15 @@ public class ReplayIntegration( recorder?.close() recorder = null rootViewsSpy.close() - lifecycle.currentState = CLOSED + state.set(state.get().copy(lifecycleState = CLOSED)) } override fun onConnectionStatusChanged(status: ConnectionStatus) { - lastKnownConnectionStatus = status - - postOnMainThread { - if (captureStrategy !is SessionCaptureStrategy) { + enqueueOnMainThread { + lastKnownConnectionStatus = status + if (state.get().captureStrategy !is SessionCaptureStrategy) { // we only want to stop recording when offline for session mode - return@postOnMainThread + return@enqueueOnMainThread } if (status == DISCONNECTED) { @@ -455,10 +525,10 @@ public class ReplayIntegration( } override fun onRateLimitChanged(rateLimiter: RateLimiter) { - postOnMainThread { - if (captureStrategy !is SessionCaptureStrategy) { + enqueueOnMainThread { + if (state.get().captureStrategy !is SessionCaptureStrategy) { // we only want to stop recording when rate-limited for session mode - return@postOnMainThread + return@enqueueOnMainThread } if (rateLimiter.isActiveForCategory(All) || rateLimiter.isActiveForCategory(Replay)) { @@ -470,20 +540,16 @@ public class ReplayIntegration( } override fun onTouchEvent(event: MotionEvent) { - if (!isEnabled.get() || !lifecycle.isTouchRecordingAllowed()) { + val current = state.get() + if (!isEnabled.get() || !current.isTouchRecordingAllowed) { return } - captureStrategy?.onTouchEvent(event) + current.captureStrategy?.onTouchEvent(event) } - // Runs [block] on the main thread. If already there, executes inline; otherwise posts via the - // main looper handler so lifecycle mutations are serialized without locking. - private inline fun postOnMainThread(crossinline block: () -> Unit) { - if (Looper.myLooper() == Looper.getMainLooper()) { - block() - } else { - mainLooperHandler.post { block() } - } + // Lifecycle commands are always queued so calls from main cannot overtake earlier commands. + private inline fun enqueueOnMainThread(crossinline block: () -> Unit) { + mainLooperHandler.post { block() } } /** @@ -492,7 +558,7 @@ public class ReplayIntegration( */ private fun checkCanRecord() { if ( - captureStrategy is SessionCaptureStrategy && + state.get().captureStrategy is SessionCaptureStrategy && (lastKnownConnectionStatus == DISCONNECTED || scopes?.rateLimiter?.isActiveForCategory(All) == true || scopes?.rateLimiter?.isActiveForCategory(Replay) == true) @@ -590,7 +656,7 @@ public class ReplayIntegration( } override fun onWindowSizeChanged(width: Int, height: Int) { - if (!isEnabled.get() || !isRecording()) { + if (!isEnabled.get() || !state.get().isRecording) { return } if (options.sessionReplay.isTrackConfiguration) { @@ -601,18 +667,41 @@ public class ReplayIntegration( } public fun onConfigurationChanged(config: ScreenshotRecorderConfig) { - if (!isEnabled.get() || !isRecording()) { + val current = state.get() + if (!isEnabled.get() || !current.isRecording) { return } - captureStrategy?.onConfigurationChanged(config) + current.captureStrategy?.onConfigurationChanged(config) recorder?.onConfigurationChanged(config) // we have to restart recorder with a new config and pause immediately if the replay is paused - if (lifecycle.currentState == PAUSED) { + if (current.lifecycleState == PAUSED) { recorder?.pause() } } + private fun sample(rate: Double?): Boolean = + (random.get() ?: Random().also { random.set(it) }).sample(rate) + + private data class ReplayState( + val generation: Long = 0, + val lifecycleState: ReplayLifecycleState = ReplayLifecycleState.INITIAL, + val replayId: SentryId = SentryId.EMPTY_ID, + val captureStrategy: CaptureStrategy? = null, + ) { + val isBuffering: Boolean + get() = captureStrategy is BufferCaptureStrategy + + val isRecording: Boolean + get() = lifecycleState >= STARTED && lifecycleState < STOPPED + + val isTouchRecordingAllowed: Boolean + get() = lifecycleState == STARTED || lifecycleState == RESUMED + + fun matches(generation: Long, replayId: SentryId): Boolean = + isRecording && this.generation == generation && this.replayId == replayId + } + private class PreviousReplayHint : Backfillable { override fun shouldEnrich(): Boolean = false } diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayLifecycle.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayLifecycle.kt index 38d0ae8bda8..a237e3ae30d 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayLifecycle.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayLifecycle.kt @@ -1,6 +1,6 @@ package io.sentry.android.replay -internal enum class ReplayState { +internal enum class ReplayLifecycleState { /** * Initial state of a Replay session. This is the state when ReplayIntegration is constructed but * has not been started yet. @@ -38,29 +38,23 @@ internal enum class ReplayState { CLOSED, } -/** Class to manage state transitions for ReplayIntegration */ -internal class ReplayLifecycle { - @field:Volatile internal var currentState = ReplayState.INITIAL - - fun isAllowed(newState: ReplayState): Boolean = - when (currentState) { - ReplayState.INITIAL -> newState == ReplayState.STARTED || newState == ReplayState.CLOSED - ReplayState.STARTED -> - newState == ReplayState.PAUSED || - newState == ReplayState.STOPPED || - newState == ReplayState.CLOSED - ReplayState.RESUMED -> - newState == ReplayState.PAUSED || - newState == ReplayState.STOPPED || - newState == ReplayState.CLOSED - ReplayState.PAUSED -> - newState == ReplayState.RESUMED || - newState == ReplayState.STOPPED || - newState == ReplayState.CLOSED - ReplayState.STOPPED -> newState == ReplayState.STARTED || newState == ReplayState.CLOSED - ReplayState.CLOSED -> false - } - - fun isTouchRecordingAllowed(): Boolean = - currentState == ReplayState.STARTED || currentState == ReplayState.RESUMED -} +internal fun ReplayLifecycleState.isAllowed(newState: ReplayLifecycleState): Boolean = + when (this) { + ReplayLifecycleState.INITIAL -> + newState == ReplayLifecycleState.STARTED || newState == ReplayLifecycleState.CLOSED + ReplayLifecycleState.STARTED -> + newState == ReplayLifecycleState.PAUSED || + newState == ReplayLifecycleState.STOPPED || + newState == ReplayLifecycleState.CLOSED + ReplayLifecycleState.RESUMED -> + newState == ReplayLifecycleState.PAUSED || + newState == ReplayLifecycleState.STOPPED || + newState == ReplayLifecycleState.CLOSED + ReplayLifecycleState.PAUSED -> + newState == ReplayLifecycleState.RESUMED || + newState == ReplayLifecycleState.STOPPED || + newState == ReplayLifecycleState.CLOSED + ReplayLifecycleState.STOPPED -> + newState == ReplayLifecycleState.STARTED || newState == ReplayLifecycleState.CLOSED + ReplayLifecycleState.CLOSED -> false + } diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BufferCaptureStrategy.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BufferCaptureStrategy.kt index 4d7bcd64cf4..d20735e4128 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BufferCaptureStrategy.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BufferCaptureStrategy.kt @@ -18,12 +18,10 @@ import io.sentry.android.replay.ScreenshotRecorderConfig import io.sentry.android.replay.capture.CaptureStrategy.Companion.rotateEvents import io.sentry.android.replay.capture.CaptureStrategy.ReplaySegment import io.sentry.android.replay.util.ReplayRunnable -import io.sentry.android.replay.util.sample import io.sentry.clientreport.DiscardReason.RATELIMIT_BACKOFF import io.sentry.protocol.SentryId import io.sentry.transport.ICurrentDateProvider import io.sentry.util.FileUtils -import io.sentry.util.Random import java.io.File import java.util.Date import java.util.concurrent.ScheduledExecutorService @@ -46,7 +44,6 @@ internal class BufferCaptureStrategy( private val options: SentryOptions, private val scopes: IScopes?, private val dateProvider: ICurrentDateProvider, - private val random: Random, executor: ScheduledExecutorService, persistingExecutor: ScheduledExecutorService, replayCacheProvider: ((replayId: SentryId) -> ReplayCache)? = null, @@ -91,20 +88,6 @@ internal class BufferCaptureStrategy( } override fun captureReplay(isTerminating: Boolean, onSegmentSent: (Date) -> Unit) { - val sampled = random.sample(options.sessionReplay.onErrorSampleRate) - - if (!sampled) { - options.logger.log( - INFO, - "Replay wasn't sampled by onErrorSampleRate, not capturing for event", - ) - return - } - - // write replayId to scope right away, so it gets picked up by the event that caused buffer - // to flush - scopes?.configureScope { it.replayId = currentReplayId } - if (isTerminating) { this.isTerminating.set(true) // avoid capturing replay, because the video will be malformed diff --git a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationTest.kt b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationTest.kt index 1c8c6ece20f..394e1c00839 100644 --- a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationTest.kt +++ b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationTest.kt @@ -36,6 +36,7 @@ import io.sentry.android.replay.capture.CaptureStrategy import io.sentry.android.replay.capture.SessionCaptureStrategy import io.sentry.android.replay.capture.SessionCaptureStrategyTest.Fixture.Companion.VIDEO_DURATION import io.sentry.android.replay.gestures.GestureRecorder +import io.sentry.android.replay.util.MainLooperHandler import io.sentry.android.replay.util.ReplayShadowMediaCodec import io.sentry.cache.PersistingScopeObserver import io.sentry.cache.tape.QueueFile @@ -49,9 +50,9 @@ import io.sentry.rrweb.RRWebVideoEvent import io.sentry.transport.CurrentDateProvider import io.sentry.transport.ICurrentDateProvider import io.sentry.transport.RateLimiter -import io.sentry.util.Random import java.io.ByteArrayOutputStream import java.io.File +import java.util.Date import java.util.concurrent.CountDownLatch import java.util.concurrent.TimeUnit import kotlin.test.BeforeTest @@ -134,6 +135,14 @@ class ReplayIntegrationTest { replayCaptureStrategyProvider: ((isFullSession: Boolean) -> CaptureStrategy)? = null, gestureRecorderProvider: (() -> GestureRecorder)? = null, dateProvider: ICurrentDateProvider = CurrentDateProvider.getInstance(), + mainLooperHandler: MainLooperHandler = mock { + doAnswer { + (it.arguments[0] as Runnable).run() + true + } + .whenever(mock) + .post(any()) + }, ): ReplayIntegration { options.run { sessionReplay.onErrorSampleRate = onErrorSampleRate @@ -148,6 +157,7 @@ class ReplayIntegrationTest { recorderProvider, replayCacheProvider = { _ -> replayCache }, replayCaptureStrategyProvider = replayCaptureStrategyProvider, + mainLooperHandler = mainLooperHandler, gestureRecorderProvider = gestureRecorderProvider, ) } @@ -221,6 +231,25 @@ class ReplayIntegrationTest { assertTrue(replay.isRecording) } + @Test + fun `start is deferred when called on main`() { + val captureStrategy = mock() + val replay = + fixture.getSut( + context, + replayCaptureStrategyProvider = { captureStrategy }, + mainLooperHandler = MainLooperHandler(), + ) + replay.register(fixture.scopes, fixture.options) + + replay.start() + + assertThat(replay.isRecording).isFalse() + verify(captureStrategy, never()).start(any(), any(), anyOrNull()) + shadowOf(Looper.getMainLooper()).idle() + assertThat(replay.isRecording).isTrue() + } + @Test fun `starting two times does nothing`() { val captureStrategy = mock() @@ -343,6 +372,7 @@ class ReplayIntegrationTest { fun `captureReplay calls and converts strategy`() { val captureStrategy = mock { whenever(mock.currentReplayId).thenReturn(SentryId()) } + whenever(captureStrategy.convert()).thenReturn(captureStrategy) val replay = fixture.getSut(context, replayCaptureStrategyProvider = { captureStrategy }) replay.register(fixture.scopes, fixture.options) @@ -358,6 +388,115 @@ class ReplayIntegrationTest { verify(captureStrategy).convert() } + @Test + fun `captureReplay returns replay id and sets scope before queued capture`() { + val replayId = SentryId() + val captureStrategy = mock() + whenever(captureStrategy.currentReplayId).thenReturn(replayId) + whenever(captureStrategy.convert()).thenReturn(captureStrategy) + val replay = + fixture.getSut( + context, + sessionSampleRate = 0.0, + replayCaptureStrategyProvider = { captureStrategy }, + mainLooperHandler = MainLooperHandler(), + ) + replay.register(fixture.scopes, fixture.options) + replay.start() + shadowOf(Looper.getMainLooper()).idle() + + val returnedReplayId = replay.captureReplay(false) + + assertThat(returnedReplayId).isEqualTo(replayId) + assertThat(fixture.scope.replayId).isEqualTo(replayId) + verify(captureStrategy, never()).captureReplay(any(), any()) + shadowOf(Looper.getMainLooper()).idle() + verify(captureStrategy).captureReplay(eq(false), any()) + } + + @Test + fun `captureReplay returns empty id when error replay is not sampled`() { + val captureStrategy = mock() + whenever(captureStrategy.currentReplayId).thenReturn(SentryId()) + val replay = + fixture.getSut( + context, + sessionSampleRate = 0.0, + onErrorSampleRate = 0.0, + replayCaptureStrategyProvider = { captureStrategy }, + ) + replay.register(fixture.scopes, fixture.options) + replay.start() + + assertThat(replay.captureReplay(false)).isEqualTo(SentryId.EMPTY_ID) + verify(captureStrategy, never()).captureReplay(any(), any()) + } + + @Test + fun `capture queued after stop cannot resurrect replay`() { + val replayId = SentryId() + val captureStrategy = mock() + whenever(captureStrategy.currentReplayId).thenReturn(replayId) + val replay = + fixture.getSut( + context, + sessionSampleRate = 0.0, + replayCaptureStrategyProvider = { captureStrategy }, + mainLooperHandler = MainLooperHandler(), + ) + replay.register(fixture.scopes, fixture.options) + replay.start() + shadowOf(Looper.getMainLooper()).idle() + + replay.stop() + assertThat(replay.captureReplay(false)).isEqualTo(replayId) + shadowOf(Looper.getMainLooper()).idle() + + assertThat(replay.isRecording).isFalse() + assertThat(replay.replayId).isEqualTo(SentryId.EMPTY_ID) + verify(captureStrategy, never()).captureReplay(any(), any()) + } + + @Test + fun `stale capture callback cannot mutate restarted replay`() { + val oldReplayId = SentryId() + val newReplayId = SentryId() + var onSegmentSent: ((Date) -> Unit)? = null + val oldStrategy = mock() + whenever(oldStrategy.currentReplayId).thenReturn(oldReplayId) + whenever(oldStrategy.convert()).thenReturn(oldStrategy) + doAnswer { + @Suppress("UNCHECKED_CAST") + onSegmentSent = it.arguments[1] as (Date) -> Unit + } + .whenever(oldStrategy) + .captureReplay(any(), any()) + val newStrategy = mock() + whenever(newStrategy.currentReplayId).thenReturn(newReplayId) + whenever(newStrategy.currentSegment).thenThrow(AssertionError("stale callback")) + var starts = 0 + val replay = + fixture.getSut( + context, + sessionSampleRate = 0.0, + replayCaptureStrategyProvider = { if (starts++ == 0) oldStrategy else newStrategy }, + mainLooperHandler = MainLooperHandler(), + ) + replay.register(fixture.scopes, fixture.options) + replay.start() + shadowOf(Looper.getMainLooper()).idle() + replay.captureReplay(false) + shadowOf(Looper.getMainLooper()).idle() + + replay.stop() + replay.start() + shadowOf(Looper.getMainLooper()).idle() + onSegmentSent?.invoke(Date()) + shadowOf(Looper.getMainLooper()).idle() + + assertThat(replay.replayId).isEqualTo(newReplayId) + } + @Test fun `pause does nothing when not recording`() { val captureStrategy = mock() @@ -460,19 +599,22 @@ class ReplayIntegrationTest { doAnswer { calls += "resume" }.whenever(mock).resume() doAnswer { calls += "stop" }.whenever(mock).stop() } - val replay = fixture.getSut(context, replayCaptureStrategyProvider = { captureStrategy }) + val replay = + fixture.getSut( + context, + replayCaptureStrategyProvider = { captureStrategy }, + mainLooperHandler = MainLooperHandler(), + ) replay.register(fixture.scopes, fixture.options) - Thread { - replay.start() - replay.pause() - replay.resume() - replay.stop() - } + Thread { replay.start() } .apply { start() join() } + replay.pause() + replay.resume() + replay.stop() assertThat(calls).isEmpty() shadowOf(Looper.getMainLooper()).idle() @@ -483,7 +625,12 @@ class ReplayIntegrationTest { fun `background close waits for main thread teardown`() { fixture.options.shutdownTimeoutMillis = TimeUnit.SECONDS.toMillis(30) val recorder = mock() - val replay = fixture.getSut(context, recorderProvider = { recorder }) + val replay = + fixture.getSut( + context, + recorderProvider = { recorder }, + mainLooperHandler = MainLooperHandler(), + ) replay.register(fixture.scopes, fixture.options) replay.start() @@ -828,7 +975,6 @@ class ReplayIntegrationTest { ICurrentDateProvider { System.currentTimeMillis() + fixture.options.sessionReplay.sessionSegmentDuration }, - Random(), // run tasks synchronously in tests mock { whenever(mock.submit(any())).doAnswer { diff --git a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationWithRecorderTest.kt b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationWithRecorderTest.kt index 75626f4e4cf..44f1551ede2 100644 --- a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationWithRecorderTest.kt +++ b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationWithRecorderTest.kt @@ -33,6 +33,7 @@ import org.junit.runner.RunWith import org.mockito.kotlin.any import org.mockito.kotlin.anyOrNull import org.mockito.kotlin.check +import org.mockito.kotlin.doAnswer import org.mockito.kotlin.mock import org.mockito.kotlin.verify import org.mockito.kotlin.whenever @@ -51,7 +52,22 @@ class ReplayIntegrationWithRecorderTest { context: Context, recorder: Recorder, dateProvider: ICurrentDateProvider = CurrentDateProvider.getInstance(), - ): ReplayIntegration = ReplayIntegration(context, dateProvider, recorderProvider = { recorder }) + ): ReplayIntegration = + ReplayIntegration( + context, + dateProvider, + recorderProvider = { recorder }, + replayCacheProvider = null, + mainLooperHandler = + mock { + doAnswer { + (it.arguments[0] as Runnable).run() + true + } + .whenever(mock) + .post(any()) + }, + ) } private val fixture = Fixture() diff --git a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayLifecycleTest.kt b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayLifecycleTest.kt index 4b5e45d23d7..5bd897f9ec4 100644 --- a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayLifecycleTest.kt +++ b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayLifecycleTest.kt @@ -1,116 +1,67 @@ package io.sentry.android.replay import kotlin.test.Test -import kotlin.test.assertEquals import kotlin.test.assertFalse import kotlin.test.assertTrue class ReplayLifecycleTest { - @Test - fun `verify initial state`() { - val lifecycle = ReplayLifecycle() - assertEquals(ReplayState.INITIAL, lifecycle.currentState) - } - @Test fun `test transitions from INITIAL state`() { - val lifecycle = ReplayLifecycle() + assertTrue(ReplayLifecycleState.INITIAL.isAllowed(ReplayLifecycleState.STARTED)) + assertTrue(ReplayLifecycleState.INITIAL.isAllowed(ReplayLifecycleState.CLOSED)) - assertTrue(lifecycle.isAllowed(ReplayState.STARTED)) - assertTrue(lifecycle.isAllowed(ReplayState.CLOSED)) - - assertFalse(lifecycle.isAllowed(ReplayState.RESUMED)) - assertFalse(lifecycle.isAllowed(ReplayState.PAUSED)) - assertFalse(lifecycle.isAllowed(ReplayState.STOPPED)) + assertFalse(ReplayLifecycleState.INITIAL.isAllowed(ReplayLifecycleState.RESUMED)) + assertFalse(ReplayLifecycleState.INITIAL.isAllowed(ReplayLifecycleState.PAUSED)) + assertFalse(ReplayLifecycleState.INITIAL.isAllowed(ReplayLifecycleState.STOPPED)) } @Test fun `test transitions from STARTED state`() { - val lifecycle = ReplayLifecycle() - lifecycle.currentState = ReplayState.STARTED - - assertTrue(lifecycle.isAllowed(ReplayState.PAUSED)) - assertTrue(lifecycle.isAllowed(ReplayState.STOPPED)) - assertTrue(lifecycle.isAllowed(ReplayState.CLOSED)) + assertTrue(ReplayLifecycleState.STARTED.isAllowed(ReplayLifecycleState.PAUSED)) + assertTrue(ReplayLifecycleState.STARTED.isAllowed(ReplayLifecycleState.STOPPED)) + assertTrue(ReplayLifecycleState.STARTED.isAllowed(ReplayLifecycleState.CLOSED)) - assertFalse(lifecycle.isAllowed(ReplayState.RESUMED)) - assertFalse(lifecycle.isAllowed(ReplayState.INITIAL)) + assertFalse(ReplayLifecycleState.STARTED.isAllowed(ReplayLifecycleState.RESUMED)) + assertFalse(ReplayLifecycleState.STARTED.isAllowed(ReplayLifecycleState.INITIAL)) } @Test fun `test transitions from RESUMED state`() { - val lifecycle = ReplayLifecycle() - lifecycle.currentState = ReplayState.RESUMED + assertTrue(ReplayLifecycleState.RESUMED.isAllowed(ReplayLifecycleState.PAUSED)) + assertTrue(ReplayLifecycleState.RESUMED.isAllowed(ReplayLifecycleState.STOPPED)) + assertTrue(ReplayLifecycleState.RESUMED.isAllowed(ReplayLifecycleState.CLOSED)) - assertTrue(lifecycle.isAllowed(ReplayState.PAUSED)) - assertTrue(lifecycle.isAllowed(ReplayState.STOPPED)) - assertTrue(lifecycle.isAllowed(ReplayState.CLOSED)) - - assertFalse(lifecycle.isAllowed(ReplayState.STARTED)) - assertFalse(lifecycle.isAllowed(ReplayState.INITIAL)) + assertFalse(ReplayLifecycleState.RESUMED.isAllowed(ReplayLifecycleState.STARTED)) + assertFalse(ReplayLifecycleState.RESUMED.isAllowed(ReplayLifecycleState.INITIAL)) } @Test fun `test transitions from PAUSED state`() { - val lifecycle = ReplayLifecycle() - lifecycle.currentState = ReplayState.PAUSED - - assertTrue(lifecycle.isAllowed(ReplayState.RESUMED)) - assertTrue(lifecycle.isAllowed(ReplayState.STOPPED)) - assertTrue(lifecycle.isAllowed(ReplayState.CLOSED)) + assertTrue(ReplayLifecycleState.PAUSED.isAllowed(ReplayLifecycleState.RESUMED)) + assertTrue(ReplayLifecycleState.PAUSED.isAllowed(ReplayLifecycleState.STOPPED)) + assertTrue(ReplayLifecycleState.PAUSED.isAllowed(ReplayLifecycleState.CLOSED)) - assertFalse(lifecycle.isAllowed(ReplayState.STARTED)) - assertFalse(lifecycle.isAllowed(ReplayState.INITIAL)) + assertFalse(ReplayLifecycleState.PAUSED.isAllowed(ReplayLifecycleState.STARTED)) + assertFalse(ReplayLifecycleState.PAUSED.isAllowed(ReplayLifecycleState.INITIAL)) } @Test fun `test transitions from STOPPED state`() { - val lifecycle = ReplayLifecycle() - lifecycle.currentState = ReplayState.STOPPED + assertTrue(ReplayLifecycleState.STOPPED.isAllowed(ReplayLifecycleState.STARTED)) + assertTrue(ReplayLifecycleState.STOPPED.isAllowed(ReplayLifecycleState.CLOSED)) - assertTrue(lifecycle.isAllowed(ReplayState.STARTED)) - assertTrue(lifecycle.isAllowed(ReplayState.CLOSED)) - - assertFalse(lifecycle.isAllowed(ReplayState.RESUMED)) - assertFalse(lifecycle.isAllowed(ReplayState.PAUSED)) - assertFalse(lifecycle.isAllowed(ReplayState.INITIAL)) + assertFalse(ReplayLifecycleState.STOPPED.isAllowed(ReplayLifecycleState.RESUMED)) + assertFalse(ReplayLifecycleState.STOPPED.isAllowed(ReplayLifecycleState.PAUSED)) + assertFalse(ReplayLifecycleState.STOPPED.isAllowed(ReplayLifecycleState.INITIAL)) } @Test fun `test transitions from CLOSED state`() { - val lifecycle = ReplayLifecycle() - lifecycle.currentState = ReplayState.CLOSED - - assertFalse(lifecycle.isAllowed(ReplayState.INITIAL)) - assertFalse(lifecycle.isAllowed(ReplayState.STARTED)) - assertFalse(lifecycle.isAllowed(ReplayState.RESUMED)) - assertFalse(lifecycle.isAllowed(ReplayState.PAUSED)) - assertFalse(lifecycle.isAllowed(ReplayState.STOPPED)) - assertFalse(lifecycle.isAllowed(ReplayState.CLOSED)) - } - - @Test - fun `test touch recording is allowed only in STARTED and RESUMED states`() { - val lifecycle = ReplayLifecycle() - - // Initial state doesn't allow touch recording - assertFalse(lifecycle.isTouchRecordingAllowed()) - - // STARTED state allows touch recording - lifecycle.currentState = ReplayState.STARTED - assertTrue(lifecycle.isTouchRecordingAllowed()) - - // RESUMED state allows touch recording - lifecycle.currentState = ReplayState.RESUMED - assertTrue(lifecycle.isTouchRecordingAllowed()) - - // Other states don't allow touch recording - val otherStates = - listOf(ReplayState.INITIAL, ReplayState.PAUSED, ReplayState.STOPPED, ReplayState.CLOSED) - - otherStates.forEach { state -> - lifecycle.currentState = state - assertFalse(lifecycle.isTouchRecordingAllowed()) - } + assertFalse(ReplayLifecycleState.CLOSED.isAllowed(ReplayLifecycleState.INITIAL)) + assertFalse(ReplayLifecycleState.CLOSED.isAllowed(ReplayLifecycleState.STARTED)) + assertFalse(ReplayLifecycleState.CLOSED.isAllowed(ReplayLifecycleState.RESUMED)) + assertFalse(ReplayLifecycleState.CLOSED.isAllowed(ReplayLifecycleState.PAUSED)) + assertFalse(ReplayLifecycleState.CLOSED.isAllowed(ReplayLifecycleState.STOPPED)) + assertFalse(ReplayLifecycleState.CLOSED.isAllowed(ReplayLifecycleState.CLOSED)) } } diff --git a/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/BufferCaptureStrategyTest.kt b/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/BufferCaptureStrategyTest.kt index fc1981a84b1..0d12fd3c3ff 100644 --- a/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/BufferCaptureStrategyTest.kt +++ b/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/BufferCaptureStrategyTest.kt @@ -26,7 +26,6 @@ import io.sentry.protocol.SentryId import io.sentry.transport.CurrentDateProvider import io.sentry.transport.ICurrentDateProvider import io.sentry.transport.RateLimiter -import io.sentry.util.Random import java.io.File import kotlin.test.Test import kotlin.test.assertEquals @@ -110,17 +109,14 @@ class BufferCaptureStrategyTest { .orEmpty() fun getSut( - onErrorSampleRate: Double = 1.0, dateProvider: ICurrentDateProvider = CurrentDateProvider.getInstance(), replayCacheDir: File? = null, ): BufferCaptureStrategy { replayCacheDir?.let { whenever(replayCache.replayCacheDir).thenReturn(it) } - options.run { sessionReplay.onErrorSampleRate = onErrorSampleRate } return BufferCaptureStrategy( options, scopes, dateProvider, - Random(), mock { whenever(it.submit(any())).doAnswer { invocation -> (invocation.arguments[0] as Runnable).run() @@ -355,16 +351,6 @@ class BufferCaptureStrategyTest { assertEquals(1, strategy.currentSegment) } - @Test - fun `captureReplay does not replayId to scope when not sampled`() { - val strategy = fixture.getSut(onErrorSampleRate = 0.0) - strategy.start() - - strategy.captureReplay(false) {} - - assertEquals(SentryId.EMPTY_ID, fixture.scope.replayId) - } - @Test fun `captureReplay does not capture segments when rate-limited`() { val rateLimiter = mock { on { isActiveForCategory(any()) }.thenReturn(true) } @@ -378,9 +364,6 @@ class BufferCaptureStrategyTest { // neither the current nor the buffered segment should be sent while rate-limited verify(fixture.scopes, never()).captureReplay(any(), any()) - // the replayId is still set on the scope so the error that flushed the buffer stays linked to - // the replay that gets recorded once the rate limit lifts - assertEquals(strategy.currentReplayId, fixture.scope.replayId) } @Test @@ -412,7 +395,7 @@ class BufferCaptureStrategyTest { } @Test - fun `captureReplay sets replayId to scope and captures buffered segments`() { + fun `captureReplay captures buffered segments`() { var called = false val strategy = fixture.getSut() strategy.start() @@ -424,7 +407,6 @@ class BufferCaptureStrategyTest { // buffered + current = 2 verify(fixture.scopes, times(2)).captureReplay(any(), any()) - assertEquals(strategy.currentReplayId, fixture.scope.replayId) assertTrue(called) } diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 915e4b95f6a..233a83b12b8 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -1707,7 +1707,7 @@ public final class io/sentry/NoOpReplayBreadcrumbConverter : io/sentry/ReplayBre } public final class io/sentry/NoOpReplayController : io/sentry/ReplayController { - public fun captureReplay (Ljava/lang/Boolean;)V + public fun captureReplay (Ljava/lang/Boolean;)Lio/sentry/protocol/SentryId; public fun disableDebugMaskingOverlay ()V public fun enableDebugMaskingOverlay ()V public fun getBreadcrumbConverter ()Lio/sentry/ReplayBreadcrumbConverter; @@ -2361,7 +2361,7 @@ public abstract interface class io/sentry/ReplayBreadcrumbConverter { } public abstract interface class io/sentry/ReplayController : io/sentry/IReplayApi { - public abstract fun captureReplay (Ljava/lang/Boolean;)V + public abstract fun captureReplay (Ljava/lang/Boolean;)Lio/sentry/protocol/SentryId; public abstract fun getBreadcrumbConverter ()Lio/sentry/ReplayBreadcrumbConverter; public abstract fun getReplayId ()Lio/sentry/protocol/SentryId; public abstract fun isDebugMaskingOverlayEnabled ()Z diff --git a/sentry/src/main/java/io/sentry/NoOpReplayController.java b/sentry/src/main/java/io/sentry/NoOpReplayController.java index 2b8a09cb1d9..3f1e88b822b 100644 --- a/sentry/src/main/java/io/sentry/NoOpReplayController.java +++ b/sentry/src/main/java/io/sentry/NoOpReplayController.java @@ -32,7 +32,9 @@ public boolean isRecording() { } @Override - public void captureReplay(@Nullable Boolean isTerminating) {} + public @NotNull SentryId captureReplay(@Nullable Boolean isTerminating) { + return SentryId.EMPTY_ID; + } @Override public @NotNull SentryId getReplayId() { diff --git a/sentry/src/main/java/io/sentry/ReplayController.java b/sentry/src/main/java/io/sentry/ReplayController.java index 2fb7b1c83a5..630c0da3d50 100644 --- a/sentry/src/main/java/io/sentry/ReplayController.java +++ b/sentry/src/main/java/io/sentry/ReplayController.java @@ -17,7 +17,12 @@ public interface ReplayController extends IReplayApi { boolean isRecording(); - void captureReplay(@Nullable Boolean isTerminating); + /** + * Captures the buffered replay and returns its ID, or {@link SentryId#EMPTY_ID} if no replay was + * captured. + */ + @NotNull + SentryId captureReplay(@Nullable Boolean isTerminating); @NotNull SentryId getReplayId(); diff --git a/sentry/src/main/java/io/sentry/SentryClient.java b/sentry/src/main/java/io/sentry/SentryClient.java index 92037f6690b..9d2c4403194 100644 --- a/sentry/src/main/java/io/sentry/SentryClient.java +++ b/sentry/src/main/java/io/sentry/SentryClient.java @@ -254,16 +254,18 @@ private boolean shouldApplyScopeData(final @NotNull CheckIn event, final @NotNul } } if (shouldCaptureReplay) { - options.getReplayController().captureReplay(event.isCrashed()); - if (scope != null) { - final @Nullable SentryId replayId = scope.getReplayId(); - if (replayId != null && !replayId.equals(SentryId.EMPTY_ID)) { - final @Nullable ITransaction transaction = scope.getTransaction(); - if (transaction != null) { - final @Nullable Baggage baggage = transaction.getSpanContext().getBaggage(); - if (baggage != null) { - baggage.forceSetReplayId(replayId); - } + final @NotNull SentryId scopeReplayId = + scope != null ? scope.getReplayId() : SentryId.EMPTY_ID; + final @NotNull SentryId capturedReplayId = + options.getReplayController().captureReplay(event.isCrashed()); + final @NotNull SentryId replayId = + !capturedReplayId.equals(SentryId.EMPTY_ID) ? capturedReplayId : scopeReplayId; + if (scope != null && !replayId.equals(SentryId.EMPTY_ID)) { + final @Nullable ITransaction transaction = scope.getTransaction(); + if (transaction != null) { + final @Nullable Baggage baggage = transaction.getSpanContext().getBaggage(); + if (baggage != null) { + baggage.forceSetReplayId(replayId); } } } @@ -1272,8 +1274,10 @@ public void captureSession(final @NotNull Session session, final @Nullable Hint // If feedback already has a replayId, we don't want to overwrite it. if (feedback.getReplayId() == null) { - options.getReplayController().captureReplay(false); - final @NotNull SentryId replayId = scope.getReplayId(); + final @NotNull SentryId scopeReplayId = scope.getReplayId(); + final @NotNull SentryId capturedReplayId = options.getReplayController().captureReplay(false); + final @NotNull SentryId replayId = + !capturedReplayId.equals(SentryId.EMPTY_ID) ? capturedReplayId : scopeReplayId; if (!replayId.equals(SentryId.EMPTY_ID)) { feedback.setReplayId(replayId); } diff --git a/sentry/src/test/java/io/sentry/SentryClientTest.kt b/sentry/src/test/java/io/sentry/SentryClientTest.kt index 02623556498..e4c4b447cf6 100644 --- a/sentry/src/test/java/io/sentry/SentryClientTest.kt +++ b/sentry/src/test/java/io/sentry/SentryClientTest.kt @@ -3508,8 +3508,9 @@ class SentryClientTest { var called = false fixture.sentryOptions.setReplayController( object : ReplayController by NoOpReplayController.getInstance() { - override fun captureReplay(isTerminating: Boolean?) { + override fun captureReplay(isTerminating: Boolean?): SentryId { called = true + return SentryId.EMPTY_ID } } ) @@ -3524,8 +3525,9 @@ class SentryClientTest { var terminated: Boolean? = false fixture.sentryOptions.setReplayController( object : ReplayController by NoOpReplayController.getInstance() { - override fun captureReplay(isTerminating: Boolean?) { + override fun captureReplay(isTerminating: Boolean?): SentryId { terminated = isTerminating + return SentryId.EMPTY_ID } } ) @@ -3545,7 +3547,7 @@ class SentryClientTest { val replayId = SentryId() fixture.sentryOptions.setReplayController( object : ReplayController by NoOpReplayController.getInstance() { - override fun captureReplay(isTerminating: Boolean?) {} + override fun captureReplay(isTerminating: Boolean?): SentryId = replayId } ) val sut = fixture.getSut() @@ -3603,8 +3605,9 @@ class SentryClientTest { var called = false fixture.sentryOptions.setReplayController( object : ReplayController by NoOpReplayController.getInstance() { - override fun captureReplay(isTerminating: Boolean?) { + override fun captureReplay(isTerminating: Boolean?): SentryId { called = true + return SentryId.EMPTY_ID } } ) @@ -3625,8 +3628,9 @@ class SentryClientTest { var called = false fixture.sentryOptions.setReplayController( object : ReplayController by NoOpReplayController.getInstance() { - override fun captureReplay(isTerminating: Boolean?) { + override fun captureReplay(isTerminating: Boolean?): SentryId { called = true + return SentryId.EMPTY_ID } } ) @@ -3665,8 +3669,9 @@ class SentryClientTest { var called = false fixture.sentryOptions.setReplayController( object : ReplayController by NoOpReplayController.getInstance() { - override fun captureReplay(isTerminating: Boolean?) { + override fun captureReplay(isTerminating: Boolean?): SentryId { called = true + return SentryId.EMPTY_ID } } ) @@ -3685,8 +3690,9 @@ class SentryClientTest { var called = false fixture.sentryOptions.setReplayController( object : ReplayController by NoOpReplayController.getInstance() { - override fun captureReplay(isTerminating: Boolean?) { + override fun captureReplay(isTerminating: Boolean?): SentryId { called = true + return SentryId.EMPTY_ID } } ) @@ -3705,8 +3711,9 @@ class SentryClientTest { var called = false fixture.sentryOptions.setReplayController( object : ReplayController by NoOpReplayController.getInstance() { - override fun captureReplay(isTerminating: Boolean?) { + override fun captureReplay(isTerminating: Boolean?): SentryId { called = true + return SentryId.EMPTY_ID } } ) @@ -3721,8 +3728,9 @@ class SentryClientTest { var called = false fixture.sentryOptions.setReplayController( object : ReplayController by NoOpReplayController.getInstance() { - override fun captureReplay(isTerminating: Boolean?) { + override fun captureReplay(isTerminating: Boolean?): SentryId { called = true + return SentryId.EMPTY_ID } } ) @@ -3742,7 +3750,7 @@ class SentryClientTest { var receivedHint: Hint? = null fixture.sentryOptions.setReplayController( object : ReplayController by NoOpReplayController.getInstance() { - override fun captureReplay(isTerminating: Boolean?) {} + override fun captureReplay(isTerminating: Boolean?): SentryId = SentryId.EMPTY_ID } ) fixture.sentryOptions.sessionReplay.beforeErrorSampling = @@ -3765,8 +3773,9 @@ class SentryClientTest { var called = false fixture.sentryOptions.setReplayController( object : ReplayController by NoOpReplayController.getInstance() { - override fun captureReplay(isTerminating: Boolean?) { + override fun captureReplay(isTerminating: Boolean?): SentryId { called = true + return SentryId.EMPTY_ID } } ) @@ -3928,7 +3937,7 @@ class SentryClientTest { val replayController = mock() val replayId = SentryId() val scope = createScope() - whenever(replayController.captureReplay(any())).thenAnswer { run { scope.replayId = replayId } } + whenever(replayController.captureReplay(any())).thenReturn(replayId) val sut = fixture.getSut { it.setReplayController(replayController) } // When there is no replay id in the feedback sut.captureFeedback(Feedback("message"), null, scope) @@ -3938,7 +3947,7 @@ class SentryClientTest { val sentFeedback = sentEvent!!.contexts.feedback assertNotNull(sentFeedback) - // And the replay id is set to the one from the scope (coming from the replay controller) + // And the replay id returned by the replay controller is set assertEquals(replayId, sentFeedback.replayId) } @@ -3952,7 +3961,7 @@ class SentryClientTest { val replayController = mock() val replayId = SentryId() val scope = createScope() - whenever(replayController.captureReplay(any())).thenAnswer { run { scope.replayId = replayId } } + whenever(replayController.captureReplay(any())).thenReturn(replayId) val sut = fixture.getSut { it.setReplayController(replayController) } // When there is replay id in the feedback val feedback = Feedback("message")