fix(replay): Fix NoSuchElementException in BufferCaptureStrategy (#4717)

* fix(replay): Consider concurrent access to ReplayCache

* Changelog

* Remove failed frame from snapshot list too

Author: romtsn

CHANGELOG.md

@@ -19,6 +19,10 @@
 
 - Remove internal API status from get/setDistinctId ([#4708](https://github.com/getsentry/sentry-java/pull/4708))
 
+### Fixes
+
+- Fix `NoSuchElementException` in `BufferCaptureStrategy` ([#4717](https://github.com/getsentry/sentry-java/pull/4717))
+
 ### Dependencies
 
 - Bump Native SDK from v0.10.0 to v0.10.1 ([#4695](https://github.com/getsentry/sentry-java/pull/4695))

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()) mutableListOf() else frames.toMutableList() }
+    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,8 @@ 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) }
+        framesSnapshot.remove(lastFrame)
         lastFrame = null
       }
     }
@@ -240,14 +248,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/BufferCaptureStrategy.kt

@@ -217,12 +217,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/capture/BufferCaptureStrategyTest.kt

@@ -254,6 +254,74 @@ class BufferCaptureStrategyTest {
     assertEquals(ReplayType.BUFFER, converted.replayType)
   }
 
+  @Test
+  fun `createCurrentSegment uses first frame timestamp when available`() {
+    val now = System.currentTimeMillis()
+    val strategy = fixture.getSut(dateProvider = { now })
+    strategy.start()
+    strategy.onConfigurationChanged(fixture.recorderConfig)
+
+    // Stub first frame timestamp and capture the 'from' argument to createVideoOf
+    whenever(fixture.replayCache.firstFrameTimestamp()).thenReturn(1234L)
+
+    var capturedFrom: Long = -1
+    whenever(
+        fixture.replayCache.createVideoOf(
+          anyLong(),
+          anyLong(),
+          anyInt(),
+          anyInt(),
+          anyInt(),
+          anyInt(),
+          anyInt(),
+          any(),
+        )
+      )
+      .thenAnswer { invocation ->
+        capturedFrom = invocation.arguments[1] as Long
+        GeneratedVideo(File("0.mp4"), 5, VIDEO_DURATION)
+      }
+
+    strategy.pause()
+
+    assertEquals(1234L, capturedFrom)
+    assertEquals(1, strategy.currentSegment)
+  }
+
+  @Test
+  fun `createCurrentSegment falls back to buffer start when no frames`() {
+    val now = System.currentTimeMillis()
+    val strategy = fixture.getSut(dateProvider = { now })
+    strategy.start()
+    strategy.onConfigurationChanged(fixture.recorderConfig)
+
+    // No frames available
+    whenever(fixture.replayCache.firstFrameTimestamp()).thenReturn(null)
+
+    var capturedFrom: Long = -1
+    whenever(
+        fixture.replayCache.createVideoOf(
+          anyLong(),
+          anyLong(),
+          anyInt(),
+          anyInt(),
+          anyInt(),
+          anyInt(),
+          anyInt(),
+          any(),
+        )
+      )
+      .thenAnswer { invocation ->
+        capturedFrom = invocation.arguments[1] as Long
+        GeneratedVideo(File("0.mp4"), 5, VIDEO_DURATION)
+      }
+
+    strategy.pause()
+
+    assertEquals(now - fixture.options.sessionReplay.errorReplayDuration, capturedFrom)
+    assertEquals(1, strategy.currentSegment)
+  }
+
   @Test
   fun `captureReplay does not replayId to scope when not sampled`() {
     val strategy = fixture.getSut(onErrorSampleRate = 0.0)