fix(replay): Continue recording in session mode after buffer is triggered

CHANGELOG.md

@@ -19,6 +19,11 @@
 
 - Remove internal API status from get/setDistinctId ([#4708](https://github.com/getsentry/sentry-java/pull/4708))
 
+### Fixes
+
+- Session Replay: Fix `NoSuchElementException` in `BufferCaptureStrategy` ([#4717](https://github.com/getsentry/sentry-java/pull/4717))
+- Session Replay: Fix continue recording in Session mode after Buffer is triggered ([#4719](https://github.com/getsentry/sentry-java/pull/4719))
+
 ## 8.21.1
 
 ### Fixes

sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayCache.kt

@@ -42,11 +42,11 @@ public class ReplayCache(private val options: SentryOptions, private val replayI
   private val isClosed = AtomicBoolean(false)
   private val encoderLock = AutoClosableReentrantLock()
   private val lock = AutoClosableReentrantLock()
+  private val framesLock = AutoClosableReentrantLock()
   private var encoder: SimpleVideoEncoder? = null
 
   internal val replayCacheDir: File? by lazy { makeReplayCacheDir(options, replayId) }
 
-  // TODO: maybe account for multi-threaded access
   internal val frames = mutableListOf<ReplayFrame>()
 
   private val ongoingSegment = LinkedHashMap<String, String>()
@@ -98,9 +98,13 @@ public class ReplayCache(private val options: SentryOptions, private val replayI
    */
   public fun addFrame(screenshot: File, frameTimestamp: Long, screen: String? = null) {
     val frame = ReplayFrame(screenshot, frameTimestamp, screen)
-    frames += frame
+    framesLock.acquire().use { frames += frame }
   }
 
+  /** Returns the timestamp of the first frame if available in a thread-safe manner. */
+  internal fun firstFrameTimestamp(): Long? =
+    framesLock.acquire().use { frames.firstOrNull()?.timestamp }
+
   /**
    * Creates a video out of currently stored [frames] given the start time and duration using the
    * on-device codecs [android.media.MediaCodec]. The generated video will be stored in [videoFile]
@@ -134,7 +138,10 @@ public class ReplayCache(private val options: SentryOptions, private val replayI
     if (videoFile.exists() && videoFile.length() > 0) {
       videoFile.delete()
     }
-    if (frames.isEmpty()) {
+    // Work on a snapshot of frames to avoid races with writers
+    val framesSnapshot =
+      framesLock.acquire().use { if (frames.isEmpty()) emptyList() else frames.toList() }
+    if (framesSnapshot.isEmpty()) {
       options.logger.log(DEBUG, "No captured frames, skipping generating a video segment")
       return null
     }
@@ -156,9 +163,9 @@ public class ReplayCache(private val options: SentryOptions, private val replayI
 
     val step = 1000 / frameRate.toLong()
     var frameCount = 0
-    var lastFrame: ReplayFrame? = frames.first()
+    var lastFrame: ReplayFrame? = framesSnapshot.firstOrNull()
     for (timestamp in from until (from + (duration)) step step) {
-      val iter = frames.iterator()
+      val iter = framesSnapshot.iterator()
       while (iter.hasNext()) {
         val frame = iter.next()
         if (frame.timestamp in (timestamp..timestamp + step)) {
@@ -180,7 +187,7 @@ public class ReplayCache(private val options: SentryOptions, private val replayI
         // if we failed to encode the frame, we delete the screenshot right away as the
         // likelihood of it being able to be encoded later is low
         deleteFile(lastFrame.screenshot)
-        frames.remove(lastFrame)
+        framesLock.acquire().use { frames.remove(lastFrame) }
         lastFrame = null
       }
     }
@@ -240,14 +247,16 @@ public class ReplayCache(private val options: SentryOptions, private val replayI
    */
   internal fun rotate(until: Long): String? {
     var screen: String? = null
-    frames.removeAll {
-      if (it.timestamp < until) {
-        deleteFile(it.screenshot)
-        return@removeAll true
-      } else if (screen == null) {
-        screen = it.screen
+    framesLock.acquire().use {
+      frames.removeAll {
+        if (it.timestamp < until) {
+          deleteFile(it.screenshot)
+          return@removeAll true
+        } else if (screen == null) {
+          screen = it.screen
+        }
+        return@removeAll false
       }
-      return@removeAll false
     }
     return screen
   }

sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BaseCaptureStrategy.kt

@@ -60,7 +60,7 @@ internal abstract class BaseCaptureStrategy(
 
   protected val isTerminating = AtomicBoolean(false)
   protected var cache: ReplayCache? = null
-  protected var recorderConfig: ScreenshotRecorderConfig? by
+  internal var recorderConfig: ScreenshotRecorderConfig? by
     persistableAtomicNullable(propertyName = "") { _, _, newValue ->
       if (newValue == null) {
         // recorderConfig is only nullable on init, but never after

sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BufferCaptureStrategy.kt

@@ -146,6 +146,7 @@ internal class BufferCaptureStrategy(
     }
     // we hand over replayExecutor to the new strategy to preserve order of execution
     val captureStrategy = SessionCaptureStrategy(options, scopes, dateProvider, replayExecutor)
+    captureStrategy.recorderConfig = recorderConfig
     captureStrategy.start(
       segmentId = currentSegment,
       replayId = currentReplayId,
@@ -217,12 +218,10 @@ internal class BufferCaptureStrategy(
     val errorReplayDuration = options.sessionReplay.errorReplayDuration
     val now = dateProvider.currentTimeMillis
     val currentSegmentTimestamp =
-      if (cache?.frames?.isNotEmpty() == true) {
+      cache?.firstFrameTimestamp()?.let {
         // in buffer mode we have to set the timestamp of the first frame as the actual start
-        DateUtils.getDateTime(cache!!.frames.first().timestamp)
-      } else {
-        DateUtils.getDateTime(now - errorReplayDuration)
-      }
+        DateUtils.getDateTime(it)
+      } ?: DateUtils.getDateTime(now - errorReplayDuration)
     val duration = now - currentSegmentTimestamp.time
     val replayId = currentReplayId
 

sentry-android-replay/src/main/java/io/sentry/android/replay/capture/SessionCaptureStrategy.kt

@@ -98,7 +98,7 @@ internal class SessionCaptureStrategy(
       }
 
       if (currentConfig == null) {
-        options.logger.log(DEBUG, "Recorder config is not set, not recording frame")
+        options.logger.log(DEBUG, "Recorder config is not set, not capturing a segment")
         return@submitSafely
       }
 

sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayCacheTest.kt

@@ -22,6 +22,8 @@ import io.sentry.rrweb.RRWebInteractionEvent
 import io.sentry.rrweb.RRWebInteractionEvent.InteractionType.TouchEnd
 import io.sentry.rrweb.RRWebInteractionEvent.InteractionType.TouchStart
 import java.io.File
+import java.util.concurrent.CountDownLatch
+import java.util.concurrent.atomic.AtomicReference
 import kotlin.test.BeforeTest
 import kotlin.test.Test
 import kotlin.test.assertEquals
@@ -493,4 +495,106 @@ class ReplayCacheTest {
     assertTrue(replayCache.frames.isEmpty())
     assertTrue(replayCache.replayCacheDir!!.listFiles()!!.none { it.extension == "jpg" })
   }
+
+  @Test
+  fun `firstFrameTimestamp returns first timestamp when available`() {
+    val replayCache = fixture.getSut(tmpDir)
+
+    assertNull(replayCache.firstFrameTimestamp())
+
+    val bitmap = Bitmap.createBitmap(1, 1, ARGB_8888)
+    replayCache.addFrame(bitmap, 42)
+    replayCache.addFrame(bitmap, 1001)
+
+    assertEquals(42L, replayCache.firstFrameTimestamp())
+  }
+
+  @Test
+  fun `firstFrameTimestamp is safe under concurrent rotate and add`() {
+    val replayCache = fixture.getSut(tmpDir)
+
+    val bitmap = Bitmap.createBitmap(1, 1, ARGB_8888)
+    repeat(10) { i -> replayCache.addFrame(bitmap, (i + 1).toLong()) }
+
+    val start = CountDownLatch(1)
+    val done = CountDownLatch(2)
+    val error = AtomicReference<Throwable?>()
+
+    val tReader = Thread {
+      try {
+        start.await()
+        repeat(500) {
+          replayCache.firstFrameTimestamp()
+          Thread.yield()
+        }
+      } catch (t: Throwable) {
+        error.set(t)
+      } finally {
+        done.countDown()
+      }
+    }
+
+    val tWriter = Thread {
+      try {
+        start.await()
+        repeat(500) { i ->
+          if (i % 2 == 0) {
+            // delete all frames occasionally
+            replayCache.rotate(Long.MAX_VALUE)
+          } else {
+            // add a fresh frame
+            replayCache.addFrame(bitmap, System.currentTimeMillis())
+          }
+        }
+      } catch (t: Throwable) {
+        error.set(t)
+      } finally {
+        done.countDown()
+      }
+    }
+
+    tReader.start()
+    tWriter.start()
+    start.countDown()
+    done.await()
+
+    // No crash is success
+    assertNull(error.get())
+  }
+
+  @Test
+  fun `createVideoOf tolerates concurrent rotate without crashing`() {
+    ReplayShadowMediaCodec.framesToEncode = 3
+    val replayCache = fixture.getSut(tmpDir)
+
+    val bitmap = Bitmap.createBitmap(1, 1, ARGB_8888)
+    // prepare a few frames that might be deleted during encoding
+    replayCache.addFrame(bitmap, 1)
+    replayCache.addFrame(bitmap, 1001)
+    replayCache.addFrame(bitmap, 2001)
+
+    val start = CountDownLatch(1)
+    val done = CountDownLatch(1)
+    val error = AtomicReference<Throwable?>()
+
+    val tEncoder = Thread {
+      try {
+        start.await()
+        replayCache.createVideoOf(3000L, 0L, 0, 100, 200, 1, 20_000)
+      } catch (t: Throwable) {
+        error.set(t)
+      } finally {
+        done.countDown()
+      }
+    }
+
+    tEncoder.start()
+    start.countDown()
+    // rotate while encoding to simulate concurrent mutation
+    replayCache.rotate(Long.MAX_VALUE)
+    done.await()
+
+    // No crash is success
+    assertNull(error.get())
+  }
 }

sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationTest.kt

@@ -27,10 +27,12 @@ import io.sentry.android.replay.ReplayCache.Companion.SEGMENT_KEY_REPLAY_RECORDI
 import io.sentry.android.replay.ReplayCache.Companion.SEGMENT_KEY_REPLAY_TYPE
 import io.sentry.android.replay.ReplayCache.Companion.SEGMENT_KEY_TIMESTAMP
 import io.sentry.android.replay.ReplayCache.Companion.SEGMENT_KEY_WIDTH
+import io.sentry.android.replay.capture.BufferCaptureStrategy
 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.ReplayShadowMediaCodec
 import io.sentry.cache.PersistingScopeObserver
 import io.sentry.cache.tape.QueueFile
 import io.sentry.protocol.SentryException
@@ -43,6 +45,7 @@ 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 kotlin.test.BeforeTest
@@ -63,13 +66,14 @@ import org.mockito.kotlin.doAnswer
 import org.mockito.kotlin.eq
 import org.mockito.kotlin.mock
 import org.mockito.kotlin.never
+import org.mockito.kotlin.reset
 import org.mockito.kotlin.times
 import org.mockito.kotlin.verify
 import org.mockito.kotlin.whenever
 import org.robolectric.annotation.Config
 
 @RunWith(AndroidJUnit4::class)
-@Config(sdk = [26])
+@Config(sdk = [26], shadows = [ReplayShadowMediaCodec::class])
 class ReplayIntegrationTest {
   @get:Rule val tmpDir = TemporaryFolder()
 
@@ -726,6 +730,58 @@ class ReplayIntegrationTest {
     verify(recorder).resume()
   }
 
+  @Test
+  fun `continues recording after converting to session strategy without extra config change`() {
+    // Force buffer mode at start, but enable onError sample so captureReplay triggers
+    val recorder = mock<Recorder>()
+    val replay =
+      fixture.getSut(
+        context,
+        recorderProvider = { recorder },
+        replayCaptureStrategyProvider = { isFullSession ->
+          // Always start with buffer strategy regardless of sampling
+          BufferCaptureStrategy(
+            fixture.options,
+            fixture.scopes,
+            // make time jump so session strategy will immediately cut a segment on next frame
+            ICurrentDateProvider {
+              System.currentTimeMillis() + fixture.options.sessionReplay.sessionSegmentDuration
+            },
+            Random(),
+            // run tasks synchronously in tests
+            mock {
+              doAnswer { (it.arguments[0] as Runnable).run() }
+                .whenever(mock)
+                .submit(any<Runnable>())
+            },
+          ) { _ ->
+            fixture.replayCache
+          }
+        },
+      )
+
+    fixture.options.sessionReplay.sessionSampleRate = 0.0 // ensure buffer mode initially
+    fixture.options.sessionReplay.onErrorSampleRate = 1.0
+    fixture.options.cacheDirPath = tmpDir.newFolder().absolutePath
+
+    replay.register(fixture.scopes, fixture.options)
+    replay.start()
+
+    val config = ScreenshotRecorderConfig(100, 200, 1f, 1f, 1, 20_000)
+    replay.onConfigurationChanged(config)
+
+    // Trigger convert() via captureReplay
+    replay.captureReplay(false)
+
+    // Now, without invoking another config change, record a frame
+    // Reset interactions to assert only post-convert capture
+    reset(fixture.scopes)
+    replay.onScreenshotRecorded(mock())
+
+    // Should capture a session segment after conversion without additional config changes
+    verify(fixture.scopes).captureReplay(any(), any())
+  }
+
   @Test
   fun `closed replay cannot be started`() {
     val replay = fixture.getSut(context)