Skip to content

fix(replay): Post checkCanRecord to main thread to prevent deadlock #5837

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
romtsn wants to merge 3 commits into
base: main
Choose a base branch
from
+27 −3
Open

fix(replay): Post checkCanRecord to main thread to prevent deadlock #5837

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4af06f2
fix(replay): Post checkCanRecord to main thread to prevent deadlock
romtsn Jul 24, 2026
c181555
Merge branch 'main' into rz/fix/replay-checkcanrecord-deadlock
romtsn Jul 27, 2026
6257cfc
fix test
romtsn Jul 27, 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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

### Fixes

- Fix potential ANR/deadlock in Session Replay when `checkCanRecord` runs on the replay executor thread ([#5837](https://github.com/getsentry/sentry-java/pull/5837))
- Prevent concurrent PixelCopy access during Session Replay masking and bitmap cleanup ([#5808](https://github.com/getsentry/sentry-java/pull/5808))
- Release `MediaMuxer` when the replay video encoder fails to start to avoid a resource leak ([#5607](https://github.com/getsentry/sentry-java/pull/5607))

Expand Down
16 changes: 14 additions & 2 deletions sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayIntegration.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import android.content.Context
import android.graphics.Bitmap
import android.graphics.BitmapFactory
import android.os.Build
import android.os.Looper
import android.view.MotionEvent
import io.sentry.Breadcrumb
import io.sentry.DataCategory.All
Expand Down Expand Up @@ -352,7 +353,7 @@ public class ReplayIntegration(
}
addFrame(bitmap, frameTimeStamp, screen)
}
checkCanRecord()
postOnMainThread { checkCanRecord() }

@0xadam-brown 0xadam-brown Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

m: Looks like we have (at least) two choices for how we fix our deadlock:

  1. never allow code that may acquire lifecycleLock to run on the replay executor thread (ie, this PR's current approach), or
  2. don't allow code holding lifecycleLock to try to shut down the replay executor.

I like (2) because it's easier to confine to a single place (the close() method), both now and over time; it's reasonable to have a policy that prohibits trying to shut down executors (or do other potentially blocking things) while holding locks (because of deadlocks like this one); and tracking down exactly which code is actually executing onScreenshotRecorded() is...tricky.

That said, maybe (2) is a pain to implement? If so, I'm okay with (1), but my vote would be to rename postOnMainThread() -> dontBlockExecutorShutdown() (or something like that). The rename would put us on notice that we're being clever, and it'd also avoid the suggestion that use of the main thread is itself significant.

Background explaining why I think releasing lifecycleLock before shutting down the replay executor will prevent our deadlock:

AFAICT, our deadlock emerges when:

  1. Thread A holds lifecycleLock.
  2. Replay executor on thread B runs onScreenshotRecorded(), which calls checkCanRecord().
  3. checkCanRecord() tries to acquire lifecycleLock via pauseInternal() but can't because thread A holds it.
  4. Thread A finishes its work and – before releasing lifecycleLock – calls replayExecutor.shutdown().
  5. But shutdown() only completes if all tasks managed by the replayExecutor have terminated.
  6. The onScreenshotRecorded() task hasn't completed because its waiting for the lifecycleLock. --> and our deadlock is born.

@romtsn romtsn Jul 27, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

thanks, that's a good point, however I don't think replayExecutor.shutdown() alone is the problem. There's also this scenario (as we'd attempted to fix earlier: #4788):

  1. Thread A holds lifecycleLock (in pause(), stop(), start(), whatever)
  2. Executor thread runs onScreenshotRecorded → checkCanRecord() → pauseInternal() → tries to acquire lifecycleLock → blocks
  3. Thread A (still holding the lock) submits something to the same single-threaded executor → blocks waiting for a slot → deadlock

So I think to satisfy both, we actually need both option 1 and 2. I can add the 2nd one 👍

}

override fun onScreenshotRecorded(screenshot: File, frameTimestamp: Long) {
Expand All @@ -375,7 +376,7 @@ public class ReplayIntegration(
}
addFrame(screenshot, frameTimestamp, screen)
}
checkCanRecord()
postOnMainThread { checkCanRecord() }
}

override fun close() {
Expand Down Expand Up @@ -444,6 +445,17 @@ 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.

@0xadam-brown 0xadam-brown Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

l: If we keep approach (1), worth explicitly mentioning that the invariant we need to maintain is never to allow the replay executor to try to acquire lifecycleLock, as it could prevent the executor from being shut down.

private inline fun postOnMainThread(crossinline block: () -> Unit) {
if (Looper.myLooper() == Looper.getMainLooper()) {
block()
} else {
mainLooperHandler.post { block() }
}
}

/**
* Check if we're offline or rate-limited and pause for session mode to not overflow the envelope
* cache.
Expand Down
13 changes: 12 additions & 1 deletion sentry-android-replay/src/test/java/io/sentry/android/replay/ReplaySmokeTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import io.sentry.rrweb.RRWebMetaEvent
import io.sentry.rrweb.RRWebVideoEvent
import io.sentry.transport.CurrentDateProvider
import io.sentry.transport.ICurrentDateProvider
import io.sentry.transport.RateLimiter
import java.time.Duration
import java.util.concurrent.Executors
import java.util.concurrent.TimeUnit
Expand All @@ -41,6 +42,7 @@ import org.mockito.kotlin.any
import org.mockito.kotlin.anyOrNull
import org.mockito.kotlin.check
import org.mockito.kotlin.doAnswer
import org.mockito.kotlin.doReturn
import org.mockito.kotlin.mock
import org.mockito.kotlin.verify
import org.mockito.kotlin.whenever
Expand All @@ -61,11 +63,17 @@ class ReplaySmokeTest {
internal class Fixture {
val options = SentryOptions()
val scope = Scope(options)
val rateLimiter =
mock<RateLimiter> {
on { isActiveForCategory(any()) }.thenReturn(false)
}
val scopes =
mock<IScopes> {
doAnswer { (it.arguments[0] as ScopeCallback).run(scope) }
.whenever(it)
.configureScope(any())

on { rateLimiter }.doReturn(rateLimiter)
}

private class ImmediateHandler :
Expand All @@ -91,7 +99,10 @@ class ReplaySmokeTest {
mainLooperHandler =
mock {
whenever(mock.handler).thenReturn(ImmediateHandler())
whenever(mock.post(any())).then { (it.arguments[0] as Runnable).run() }
whenever(mock.post(any())).then {
(it.arguments[0] as Runnable).run()
true
}
whenever(mock.postDelayed(any(), anyLong())).then {
// have to use another thread here otherwise it will block the test thread
recordingThread.schedule(
Expand Down
Loading