fix(replay): Skip buffer-mode replay capture when rate-limited (DART-313) - #5813
fix(replay): Skip buffer-mode replay capture when rate-limited (DART-313)#5813runningcode wants to merge 5 commits into
Conversation
…313) In buffer (on-error) mode the recorder keeps running while rate-limited so the rolling buffer stays warm, but capturing on an error still encoded the current and buffered segments and handed them to the transport, which then dropped them. That wasted CPU, I/O, and MediaMuxer file descriptors on envelopes that could never be sent. Bail out of BufferCaptureStrategy.captureReplay when the Replay (or All) category is rate-limited, mirroring the guard session mode already applies. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
📲 Install BuildsAndroid
|
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 |
| } | ||
|
|
||
| override fun captureReplay(isTerminating: Boolean, onSegmentSent: (Date) -> Unit) { | ||
| if (isReplayRateLimited()) { |
There was a problem hiding this comment.
h: I think we're calling this a bit early -- should likely do it after the if (isTerminating) check, otherwise we'd be dropping replays for crashed events (which are anyway not being sent in this process, but only on next launch, where the rate limit likely will have already been lifted).
Another side effect of not setting isTerminating would be that the following convert() call would convert to session mode and try to keep recording while the process is terminating. Probs, needs a test for this to not regress in the future too
| // the segment envelopes would be dropped by the transport anyway, so don't waste resources | ||
| // encoding videos that will only be discarded | ||
| options.logger.log(INFO, "Replay is rate-limited, not capturing for event") | ||
| return |
There was a problem hiding this comment.
m: another thing -- previously we'd record a client report with the reason ratelimit_backoff on the envelope being dropped, but now we're not doing it. I think after addressing the point above (moving this to after if (isTerminating), we could also record the ratelimit_backoff client report for a genuinely lost event (which would be also called after sampling) inside the if (isReplayRateLimited()) block.
romtsn
left a comment
romtsn
left a comment
There was a problem hiding this comment.
pre-approving, but 2 things I'd want to have addressed
…imited (DART-313) Skipping the encode when rate-limited meant the segments never reached the transport, so RateLimiter.filter never recorded them as lost. Replay drops in buffer mode silently vanished from client reports. Record a RATELIMIT_BACKOFF lost event for the Replay category at the bail-out, and move the rate-limit check below the sampling and isTerminating guards so we only report replays that would genuinely have been sent — a replay dropped by onErrorSampleRate is not a rate-limit loss, and a terminating one is deferred to the next launch rather than lost. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
| // segment plus every buffered one, but a flush only ever loses a single replay from the | ||
| // user's perspective. Under-reporting here is preferable to making replay look like it | ||
| // dropped data it never held. | ||
| options.clientReportRecorder.recordLostEvent(RATELIMIT_BACKOFF, Replay) |
There was a problem hiding this comment.
@romtsn is this what you were thinking? Is it okay that we only report 1 lost event here even if there are more in the buffer? This is something my clanker said was a possibility.
There was a problem hiding this comment.
yeah, theoretically we could've used bufferedSegments.size + 1 here, but I agree it's better to under-report because for users it looks like a single replay lost, so let's keep it as-is
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![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.
Reviewed by Cursor Bugbot for commit 91a65bb. Configure here.
Bailing out of BufferCaptureStrategy.captureReplay while rate-limited left isTerminating unset, so ReplayIntegration's unconditional convert() still swapped in a SessionCaptureStrategy. That discarded the rolling buffer, and the next recorded frame then hit checkCanRecord(), which pauses session mode when rate-limited and encodes a segment on the way out - exactly the work the bail-out was meant to avoid. It also left recording paused for the rest of the rate-limit window, contradicting onRateLimitChanged, which deliberately keeps buffer mode running. Stay in buffer mode while rate-limited so the buffer keeps rolling and the next error after the limit expires can send a complete replay. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
romtsn
left a comment
romtsn
left a comment
There was a problem hiding this comment.
double-checked everything, it all makes sense, approving!
2 participants
📜 Description
In buffer (on-error) session replay mode, the recorder intentionally keeps running while the SDK is rate-limited so the rolling buffer stays warm. However, when an error triggered
captureReplay, the strategy still encoded the current segment and every buffered segment and handed the resulting envelopes to the transport — which then discarded them because of the active rate limit. That wasted CPU, disk I/O, andMediaMuxerfile descriptors on replays that could never be sent.This adds a guard at the top of
BufferCaptureStrategy.captureReplay: if theReplay(orAll) data category is rate-limited, we skip encoding and capturing entirely. This mirrors the guard session mode already applies viaonRateLimitChanged/checkCanRecord. The buffer keeps rolling cheaply in the background and captures normally once the rate limit expires.💡 Motivation and Context
Follow-up to the session replay resource-leak work in #5583 and #5607. Those fixed the
MediaMuxerFD leak during encoding; this closes the remaining item called out in getsentry/sentry-dart#3528 (getsentry/sentry-dart#3528): while rate-limited, the Android SDK kept creating and encoding segments that were only going to be discarded.💚 How did you test it?
Added a unit test (
captureReplay does nothing when rate-limited) asserting that, even with a buffered segment present, no segment is sent to the transport and noreplayIdis written to the scope while rate-limited.📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
Optionally extend the same guard to the
pauseandonConfigurationChangedencode paths in buffer mode if that churn proves significant.