fix(replay): Don't let a wedged video encoder freeze the app - #5842
Open
romtsn wants to merge 2 commits into
Open
fix(replay): Don't let a wedged video encoder freeze the app#5842romtsn wants to merge 2 commits into
romtsn wants to merge 2 commits into
Conversation
Some hardware encoders never emit BUFFER_FLAG_END_OF_STREAM after signalEndOfInputStream(), so SimpleVideoEncoder.drainCodec() spun forever while holding encoderLock. ReplayCache.close() then blocked on that lock, and since it runs inline under ReplayIntegration's lifecycleLock, the main thread froze until the system killed the process. Two bounds, both needed: the drain loop now gives up after 10 consecutive no-progress iterations (~1s), and close() only waits 2s for the encoder lock before skipping the release. The loop bound alone isn't enough -- a native dequeueOutputBuffer call can itself never return, since ALooper::awaitResponse has no deadline of its own. Adds AutoClosableReentrantLock.tryAcquire(timeout, unit) for the latter. Fixes getsentry/sentry-dart#3556 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
romtsn
requested review from
0xadam-brown,
adinauer,
markushi and
runningcode
as code owners
July 27, 2026 15:37
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
runningcode
reviewed
Jul 27, 2026
| * [MediaCodec.signalEndOfInputStream], which used to spin the drain loop forever while holding the | ||
| * encoder lock, wedging the whole replay pipeline (and with it the app's lifecycle callbacks). | ||
| */ | ||
| private const val MAX_EOS_STALL_ITERATIONS = 10 |
Contributor
There was a problem hiding this comment.
how did we determine 10 here? Just curious. How fast do these iterations happen?
📲 Install BuildsAndroid
|
Contributor
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 05aa61d | 326.06 ms | 385.46 ms | 59.40 ms |
| bb0ff41 | 315.84 ms | 350.76 ms | 34.92 ms |
| 806307f | 357.85 ms | 424.64 ms | 66.79 ms |
| d501a7e | 307.33 ms | 341.94 ms | 34.61 ms |
| 0ee65e9 | 321.06 ms | 361.24 ms | 40.18 ms |
| ed33deb | 334.19 ms | 362.30 ms | 28.11 ms |
| 9fbb112 | 401.87 ms | 515.87 ms | 114.00 ms |
| b8bd880 | 314.56 ms | 336.50 ms | 21.94 ms |
| 5b1a06b | 315.40 ms | 353.33 ms | 37.94 ms |
| 6edfca2 | 316.43 ms | 398.90 ms | 82.46 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 05aa61d | 0 B | 0 B | 0 B |
| bb0ff41 | 0 B | 0 B | 0 B |
| 806307f | 1.58 MiB | 2.10 MiB | 533.42 KiB |
| d501a7e | 0 B | 0 B | 0 B |
| 0ee65e9 | 0 B | 0 B | 0 B |
| ed33deb | 1.58 MiB | 2.13 MiB | 559.52 KiB |
| 9fbb112 | 1.58 MiB | 2.11 MiB | 539.18 KiB |
| b8bd880 | 1.58 MiB | 2.29 MiB | 722.92 KiB |
| 5b1a06b | 0 B | 0 B | 0 B |
| 6edfca2 | 1.58 MiB | 2.13 MiB | 559.07 KiB |
runningcode
reviewed
Jul 27, 2026
runningcode
left a comment
Contributor
There was a problem hiding this comment.
looks good, i left a question for a discussion!
| } | ||
| } | ||
| } catch (e: InterruptedException) { | ||
| Thread.currentThread().interrupt() |
Contributor
There was a problem hiding this comment.
do we need to surround this with try/catch as well?
| val token = encoderLock.tryAcquire(ENCODER_RELEASE_TIMEOUT_MS, MILLISECONDS) | ||
| if (token == null) { | ||
| options.logger.log( | ||
| WARNING, |
Contributor
There was a problem hiding this comment.
ok so just to check my understanding, we try to get the lock only to make sure nobody else is holding it before calling release but if we can't acquire the lock, what state are we in? is this safe to skip the release? won't we cause a memory leak?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📜 Description
Fixes an ANR where Android apps freeze on foreground/background transitions until the system kills the process.
A reporter pulled the full deobfuscated 131-thread dump from Play Console (the Sentry ANR event only carries
main, which is why the lock holder was invisible), giving us the exact chain:Two cooperating defects, so two bounds — both are needed:
SimpleVideoEncoder.drainCodec()had awhile (true)with no exit on theendOfStream = truepath. Some hardware encoders never emitBUFFER_FLAG_END_OF_STREAMaftersignalEndOfInputStream(), so the loop spun forever while holdingencoderLock. It now gives up after 10 consecutive no-progress iterations (~1s at the existing 100ms poll) and drops the remaining frames.ReplayCache.close()used an unboundedencoderLock.acquire(). It runs inline on the caller's thread fromBaseCaptureStrategy.stop(), whichReplayIntegration.stop()invokes while holdinglifecycleLock— that nesting is what promotes a stuck encoder into a frozen main thread. It now waits at most 2s and skips the release otherwise.The loop bound alone isn't sufficient: the native frames show
MediaCodec::dequeueOutputBuffer→AMessage::postAndAwaitResponse→ALooper::awaitResponsewithMediaCodec_looperidle.TIMEOUT_USECis handed to the codec looper, which is supposed to reply once it elapses — a dead looper never replies, andawaitResponsehas no deadline of its own. So a single call can never return, and only bounding the lock wait keeps the lifecycle path free.On the timeout path the (already-dead) codec is not released, leaking a native handle. That's the deliberate trade: leaking a handle in a process whose encoder is wedged beats freezing the app.
isClosed.set(true)still runs on every path, sincepersistSegmentValuesgates on it.The other four
encoderLock.acquire()sites inReplayCacheare unchanged — they all run on the replay worker, the thread that would be stuck, so a deadline there buys nothing.Adds
AutoClosableReentrantLock.tryAcquire(timeout, unit)(@ApiStatus.Internal, additive) for the second bound. Returns the token on success andnullon timeout, so callers must branch explicitly rather than silently no-op via?.use {}.Net effect: a wedged codec degrades to "replay stops working for this process" instead of "the app freezes."
💚 How did you test it?
ReplayShadowMediaCodecgained two hooks, since it ignorestimeoutUsand always eventually produces EOS:neverSignalEosandblockOnDequeue. New tests cover both bounds inReplayCacheTest, plus 4 fortryAcquireinAutoClosableReentrantLockTest.Both
ReplayCacheTestcases were mutation-checked — reverting each fix in turn makes the matching test fail with its assertion message rather than hang. Full:sentry-android-replay:testReleaseUnitTestpasses;apiDumpadds exactly the onetryAcquireline.No device repro is available (Redmi Note 14 Pro 5G / Pro+ 5G, Android 16, MediaTek), so the shadow-based tests are the verification of record.
📝 Checklist
sendDefaultPiiis enabled.🤖 Generated with Claude Code