Skip to content

fix: guard executor shutdown in BaseCaptureStrategy.stop() #5627

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tsushanth wants to merge 2 commits into
base: main
Choose a base branch
from
+54 −5
Open

fix: guard executor shutdown in BaseCaptureStrategy.stop() #5627

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3f176a7
fix: guard executor shutdown in BaseCaptureStrategy.stop()
tsushanth Jun 25, 2026
5b032f2
style: apply spotless formatting
tsushanth Jun 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 22 additions & 5 deletions sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BaseCaptureStrategy.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,21 @@ internal abstract class BaseCaptureStrategy(
private const val MAX_TRACE_IDS = 100
}

private val persistingExecutor: ScheduledExecutorService by lazy {
val delegate =
Executors.newSingleThreadScheduledExecutor(ReplayPersistingExecutorServiceThreadFactory())
ReplayExecutorService(delegate, options)
}
// Explicit holder so we can detect whether the executor was ever initialised and shut it down
// without initialising it as a side-effect (which is what a plain `lazy` delegate would do).
private var persistingExecutorHolder: ScheduledExecutorService? = null
private val persistingExecutor: ScheduledExecutorService
get() {
return persistingExecutorHolder
?: run {
val delegate =
Executors.newSingleThreadScheduledExecutor(
ReplayPersistingExecutorServiceThreadFactory()
)
ReplayExecutorService(delegate, options).also { persistingExecutorHolder = it }
}
}
Comment on lines +64 to +74

@sentry sentry Bot Jun 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The custom getter for persistingExecutor is not thread-safe. Concurrent access can lead to a race condition, causing multiple executor instances to be created and resulting in a thread leak.
Severity: HIGH

Suggested Fix

Synchronize access to the persistingExecutor getter to ensure only one instance is created. This can be achieved by using a synchronized block around the check-and-create logic or by adding the @Volatile annotation to the persistingExecutorHolder field and using a double-checked locking pattern. The original lazy delegate, which is thread-safe by default, is a good model for a correct implementation.

Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location:
sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BaseCaptureStrategy.kt#L63-L74

Potential issue: The custom getter for the `persistingExecutor` property implements an
unsynchronized lazy initialization pattern. Multiple threads, including the main thread
and background `replayExecutor` threads, can access this getter concurrently. If two
threads access the getter when `persistingExecutorHolder` is `null`, both may proceed to
create a new `ScheduledExecutorService` instance. Due to the race condition, only one
instance will be stored in `persistingExecutorHolder`, while the other will be abandoned
without being shut down. This results in a thread leak, which can degrade application
performance and lead to resource exhaustion over time. The backing field
`persistingExecutorHolder` also lacks a `@Volatile` annotation, creating potential
memory visibility issues.

Did we get this right? 👍 / 👎 to inform future reviews.


private val gestureConverter = ReplayGestureConverter(dateProvider)

protected val isTerminating = AtomicBoolean(false)
Expand Down Expand Up @@ -123,6 +133,13 @@ internal abstract class BaseCaptureStrategy(
replayStartTimestamp.set(0)
segmentTimestamp = null
currentReplayId = SentryId.EMPTY_ID
// Shut down the persisting executor only if it was actually initialised. We must not call the
// blocking ReplayExecutorService.shutdown() here because stop() may run on the main thread,
// and blocking there risks an ANR. shutdownNow() is non-blocking: it cancels queued tasks and
// interrupts any running one, which is acceptable because the tasks are best-effort persistence
// writes and the cache has already been closed above.
persistingExecutorHolder?.shutdownNow()
persistingExecutorHolder = null
}

protected fun createSegmentInternal(
Expand Down
32 changes: 32 additions & 0 deletions ...droid-replay/src/test/java/io/sentry/android/replay/capture/SessionCaptureStrategyTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import io.sentry.rrweb.RRWebMetaEvent
import io.sentry.rrweb.RRWebOptionsEvent
import io.sentry.transport.CurrentDateProvider
import io.sentry.transport.ICurrentDateProvider
import io.sentry.util.thread.IThreadChecker
import java.io.File
import java.util.Date
import kotlin.test.Test
Expand Down Expand Up @@ -554,4 +555,35 @@ class SessionCaptureStrategyTest {
any(),
)
}

@Test
fun `stop shuts down persisting executor so no SentryReplayPersister threads leak across cycles`() {
// Force isMainThread() to return true so that property-reset writes in stop() are dispatched
// to the persistingExecutor, which is the code path that used to leak threads.
fixture.options.threadChecker =
mock<IThreadChecker> {
on { isMainThread() }.thenReturn(true)
on { isMainThread(any<Thread>()) }.thenReturn(true)
on { isMainThread(anyLong()) }.thenReturn(false)
on { currentThreadName }.thenReturn("main")
on { currentThreadSystemId() }.thenReturn(0L)
}

repeat(3) {
val strategy = fixture.getSut()
strategy.start(0, SentryId())
strategy.onConfigurationChanged(fixture.recorderConfig)
strategy.stop()
}

// Allow time for thread termination after shutdownNow().
Thread.sleep(200)

val leakedThreads =
Thread.getAllStackTraces().keys.filter { it.name.startsWith("SentryReplayPersister-") }
assertTrue(
leakedThreads.isEmpty(),
"Expected no SentryReplayPersister threads after stop(), found: ${leakedThreads.map { it.name }}",
)
}
}
Loading