ref(android): Confine replay lifecycle to main thread - #5965
Conversation
Serialize replay lifecycle mutations on Android's main thread and keep replay cache cleanup ordered on the replay executor. Remove locks that could block lifecycle callbacks while preserving shutdown ordering. Refs JAVA-665 Co-Authored-By: OpenAI Codex <noreply@openai.com>
📲 Install BuildsAndroid
|
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 9e60aca | 316.18 ms | 345.04 ms | 28.86 ms |
| 6b019b7 | 403.90 ms | 546.09 ms | 142.19 ms |
| d15471f | 310.66 ms | 368.19 ms | 57.53 ms |
| d217708 | 375.27 ms | 415.68 ms | 40.41 ms |
| 22f4345 | 314.79 ms | 375.02 ms | 60.23 ms |
| fcec2f2 | 328.91 ms | 387.75 ms | 58.84 ms |
| d501a7e | 307.33 ms | 341.94 ms | 34.61 ms |
| 7414e9b | 315.69 ms | 367.66 ms | 51.97 ms |
| fcec2f2 | 314.96 ms | 373.66 ms | 58.70 ms |
| e2dce0b | 308.96 ms | 360.10 ms | 51.14 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 9e60aca | 0 B | 0 B | 0 B |
| 6b019b7 | 0 B | 0 B | 0 B |
| d15471f | 1.58 MiB | 2.13 MiB | 559.54 KiB |
| d217708 | 1.58 MiB | 2.10 MiB | 532.97 KiB |
| 22f4345 | 1.58 MiB | 2.29 MiB | 719.83 KiB |
| fcec2f2 | 1.58 MiB | 2.12 MiB | 551.50 KiB |
| d501a7e | 0 B | 0 B | 0 B |
| 7414e9b | 0 B | 0 B | 0 B |
| fcec2f2 | 1.58 MiB | 2.12 MiB | 551.50 KiB |
| e2dce0b | 0 B | 0 B | 0 B |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| lifecycle.currentState = RESUMED | ||
| captureStrategy?.resume() | ||
| recorder?.resume() | ||
| } | ||
|
|
||
| override fun captureReplay(isTerminating: Boolean?) { |
There was a problem hiding this comment.
Bug: A race condition exists where captureReplay() can be called from a background thread while stopInternal() nullifies captureStrategy on the main thread, leading to inconsistent state.
Severity: HIGH
Suggested Fix
Re-introduce a lock or use another thread-safe mechanism to guard the read, modification, and write operations on the captureStrategy object across captureReplay() and stopInternal(). Simply using @Volatile is not sufficient for these compound operations.
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#L247-L252
Potential issue: The `captureReplay()` method can be invoked from any thread, but it is
not thread-safe. A race condition occurs if one thread calls `captureReplay()` while
another thread (e.g., the main thread during shutdown) calls `stopInternal()`, which
sets the `captureStrategy` field to `null`. The lambda passed to
`captureStrategy?.captureReplay()` can execute after `captureStrategy` has been
nullified, causing operations within the lambda to access a stale or null reference.
This leads to inconsistent replay state and potential data corruption. The `@Volatile`
annotation on `captureStrategy` is insufficient to protect this sequence of non-atomic
operations.
Did we get this right? 👍 / 👎 to inform future reviews.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 43758cc. Configure here.
| // the main looper handler. Prevents deadlocks when lifecycle-lock-acquiring code (e.g. | ||
| // checkCanRecord -> pauseInternal) is called from the replay executor thread. | ||
| // Runs [block] on the main thread. If already there, executes inline; otherwise posts via the | ||
| // main looper handler so lifecycle mutations are serialized without locking. |
There was a problem hiding this comment.
Lifecycle calls race across threads
Medium Severity
postOnMainThread runs inline when already on the main thread, so those calls jump ahead of lifecycle work previously posted from a background thread. A main-thread stop or resume can therefore run while state is still INITIAL/PAUSED and become a no-op, after which the queued start or stop still executes. LifecycleWatcher already mixes main-thread start/resume with timer-thread stop, so a session can end up STOPPED in the foreground or keep recording after stop.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 43758cc. Configure here.
| } catch (t: Throwable) { | ||
| release() | ||
| throw t | ||
| } |
There was a problem hiding this comment.
do you know why we catch and rethrow the throwable here instead of a finally ?
| } catch (t: Throwable) { | |
| release() | |
| throw t | |
| } | |
| } finally { | |
| release() | |
| } |
| recorder = null | ||
| rootViewsSpy.close() | ||
| lifecycle.currentState = CLOSED | ||
| val isMainThread = Looper.myLooper() == Looper.getMainLooper() |
There was a problem hiding this comment.
how come we don't use options.threadChecker.isMainThread here ?
| ) | ||
| return | ||
| } | ||
| postOnMainThread { startInternal() } |
There was a problem hiding this comment.
postOnMainThread is a bit of a misnomer since it doesn't post if we're already on the main thread.
Two issues:
- when called during startup, it doesn't post it for later meaning it doesn't improve the startup time.
- calls can get out of order. not sure if this was an explicit goal, if we always post to the main thread, then the events are queue in order, if we sometimes post and sometimes execute inline then we dno't have any ordering guarantee if these are called from different threads.
2 participants
📜 Description
Confine Session Replay lifecycle mutations to Android's main thread and keep replay cache cleanup ordered on the replay executor. Background shutdown waits only for main-thread teardown to queue cleanup before stopping the executors.
This also removes the lifecycle and encoder locks that are no longer needed, together with the unused
tryAcquire()helper.💡 Motivation and Context
Prepare Session Replay for public start and stop APIs without allowing lifecycle calls from arbitrary threads to race or block Android lifecycle callbacks.
JAVA-656 remains related follow-up work because automatic replay startup is still synchronous when SDK initialization already runs on the main thread.
Refs JAVA-665
Refs JAVA-656
💚 How did you test it?
./gradlew spotlessApply apiDump.📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps