Skip to content

fix(replay): Prevent ANR by caching connection status instead of blocking calls #4891

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

Merged
markushi merged 4 commits into from
Nov 11, 2025
Merged

fix(replay): Prevent ANR by caching connection status instead of blocking calls #4891

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7377ab0
fix(replay): Prevent ANR by caching connection status instead of bloc…
markushi Nov 10, 2025
ab2df97
docs(changelog): Add ANR fix entry for PR #4891
markushi Nov 10, 2025
b713bb5
Address PR feedback
markushi Nov 10, 2025
bbb5114
Apply suggestion from @markushi
markushi Nov 11, 2025
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 @@
- Fix visual artifacts for the Canvas strategy on some devices ([#4861](https://github.com/getsentry/sentry-java/pull/4861))
- [Config] Trim whitespace on properties path ([#4880](https://github.com/getsentry/sentry-java/pull/4880))
- Only set `DefaultReplayBreadcrumbConverter` if replay is available ([#4888](https://github.com/getsentry/sentry-java/pull/4888))
- Prevent ANR by caching connection status instead of blocking calls ([#4891](https://github.com/getsentry/sentry-java/pull/4891))

### Improvements

Expand Down
1 change: 1 addition & 0 deletions sentry-android-core/src/main/java/io/sentry/android/core/AndroidContinuousProfiler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ private void start() {
}

// If device is offline, we don't start the profiler, to avoid flooding the cache
// TODO .getConnectionStatus() may be blocking, investigate if this can be done async
if (scopes.getOptions().getConnectionStatusProvider().getConnectionStatus() == DISCONNECTED) {
logger.log(SentryLevel.WARNING, "Device is offline. Stopping profiler.");
// Let's stop and reset profiler id, as the profile is now broken anyway
Expand Down
1 change: 1 addition & 0 deletions sentry-android-core/src/main/java/io/sentry/android/core/DeviceInfoUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,7 @@ private void setDeviceIO(final @NotNull Device device, final boolean includeDyna
device.setBatteryTemperature(getBatteryTemperature(batteryIntent));
}

// TODO .getConnectionStatus() may be blocking, investigate if this can be done async
Boolean connected;
switch (options.getConnectionStatusProvider().getConnectionStatus()) {
case DISCONNECTED:
Expand Down
8 changes: 5 additions & 3 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 @@ -95,6 +95,7 @@ public class ReplayIntegration(
this.gestureRecorderProvider = gestureRecorderProvider
}

private var lastKnownConnectionStatus: ConnectionStatus = ConnectionStatus.UNKNOWN
Copy link
Copy Markdown

@cursor cursor bot Nov 10, 2025

Choose a reason for hiding this comment

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

Bug: Offline Replay: Connection Status Misinterpretation

lastKnownConnectionStatus initializes to UNKNOWN but resumeInternal() only blocks when the status equals DISCONNECTED. If resumeInternal() runs before the first onConnectionStatusChanged() callback fires, the replay will resume even when the device is actually offline, potentially flooding the cache with unsendable events.

Fix in Cursor Fix in Web

Copy link
Copy Markdown
Member Author

@markushi markushi Nov 11, 2025

Choose a reason for hiding this comment

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

That's fine, in worst case the disconnected change will be fired via onConnectionStatusChanged() shortly afterwards anyway.

private var debugMaskingEnabled: Boolean = false
Copy link
Copy Markdown

@sentry sentry bot Nov 10, 2025

Choose a reason for hiding this comment

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

Bug: lastKnownConnectionStatus is a mutable field accessed by multiple threads without proper synchronization, leading to a data race and potential stale reads.
Severity: HIGH | Confidence: 1.00

🔍 Detailed Analysis

The lastKnownConnectionStatus field is subject to a data race. It is a mutable field not declared as volatile and is accessed by multiple threads without consistent synchronization. Writes in onConnectionStatusChanged() occur without a lock, while reads in resumeInternal() occur within lifecycleLock. This inconsistent synchronization means writes may not be visible to other threads, leading to stale reads and unreliable connection status, which can cause the replay integration to fail pausing or resuming correctly.

💡 Suggested Fix

Declare lastKnownConnectionStatus as volatile or ensure both read and write operations are protected by the same lock (e.g., lifecycleLock).

🤖 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/ReplayIntegration.kt#L98-L99

Potential issue: The `lastKnownConnectionStatus` field is subject to a data race. It is
a mutable field not declared as `volatile` and is accessed by multiple threads without
consistent synchronization. Writes in `onConnectionStatusChanged()` occur without a
lock, while reads in `resumeInternal()` occur within `lifecycleLock`. This inconsistent
synchronization means writes may not be visible to other threads, leading to stale reads
and unreliable connection status, which can cause the replay integration to fail pausing
or resuming correctly.

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

private lateinit var options: SentryOptions
private var scopes: IScopes? = null
Expand Down Expand Up @@ -219,7 +220,7 @@ public class ReplayIntegration(

if (
isManualPause.get() ||
options.connectionStatusProvider.connectionStatus == DISCONNECTED ||
lastKnownConnectionStatus == DISCONNECTED ||
scopes?.rateLimiter?.isActiveForCategory(All) == true ||
scopes?.rateLimiter?.isActiveForCategory(Replay) == true
) {
Expand Down Expand Up @@ -335,6 +336,8 @@ public class ReplayIntegration(
}

override fun onConnectionStatusChanged(status: ConnectionStatus) {
lastKnownConnectionStatus = status

if (captureStrategy !is SessionCaptureStrategy) {
// we only want to stop recording when offline for session mode
return
Expand Down Expand Up @@ -375,8 +378,7 @@ public class ReplayIntegration(
private fun checkCanRecord() {
if (
captureStrategy is SessionCaptureStrategy &&
(options.connectionStatusProvider.connectionStatus == DISCONNECTED ||
scopes?.rateLimiter?.isActiveForCategory(All) == true ||
(scopes?.rateLimiter?.isActiveForCategory(All) == true ||
scopes?.rateLimiter?.isActiveForCategory(Replay) == true)
) {
pauseInternal()
Expand Down
8 changes: 2 additions & 6 deletions sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,6 @@ class ReplayIntegrationTest {
context: Context,
sessionSampleRate: Double = 1.0,
onErrorSampleRate: Double = 1.0,
isOffline: Boolean = false,
isRateLimited: Boolean = false,
recorderProvider: (() -> Recorder)? = null,
replayCaptureStrategyProvider: ((isFullSession: Boolean) -> CaptureStrategy)? = null,
Expand All @@ -130,9 +129,6 @@ class ReplayIntegrationTest {
options.run {
sessionReplay.onErrorSampleRate = onErrorSampleRate
sessionReplay.sessionSampleRate = sessionSampleRate
connectionStatusProvider = mock {
on { connectionStatus }.thenReturn(if (isOffline) DISCONNECTED else CONNECTED)
}
}
if (isRateLimited) {
whenever(rateLimiter.isActiveForCategory(any())).thenReturn(true)
Expand Down Expand Up @@ -623,13 +619,13 @@ class ReplayIntegrationTest {
context,
recorderProvider = { recorder },
replayCaptureStrategyProvider = { captureStrategy },
isOffline = true,
)

replay.register(fixture.scopes, fixture.options)
replay.start()
replay.onScreenshotRecorded(mock<Bitmap>())

replay.onConnectionStatusChanged(DISCONNECTED)
Copy link
Copy Markdown

@cursor cursor bot Nov 11, 2025

Choose a reason for hiding this comment

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

Bug: Offline Screenshot Pause: Test Logic Flawed.

The test calls onConnectionStatusChanged(DISCONNECTED) after onScreenshotRecorded(), so it verifies that onConnectionStatusChanged pauses the recorder rather than verifying that checkCanRecord() within onScreenshotRecorded detects offline status and pauses. The test passes for the wrong reason and doesn't actually test the intended behavior of pausing when a screenshot is recorded while offline.

Fix in Cursor Fix in Web

Copy link
Copy Markdown
Member Author

@markushi markushi Nov 11, 2025

Choose a reason for hiding this comment

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

Even reversing the order here wouldn't help, as at the time checkCanRecord() is called, we already would be in the paused state.

verify(recorder).pause()
}

Expand Down Expand Up @@ -903,10 +899,10 @@ class ReplayIntegrationTest {
context,
recorderProvider = { recorder },
replayCaptureStrategyProvider = { captureStrategy },
isOffline = true,
)

replay.register(fixture.scopes, fixture.options)
replay.onConnectionStatusChanged(DISCONNECTED)
replay.start()

replay.pause()
Expand Down
Loading