fix(replay): Prevent rrweb recording from dying when the emit callback throws#21409
Merged
Conversation
Contributor
size-limit report 📦
|
Contributor
|
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you apply the label |
rrweb recording could die while the rest of the Replay integration kept working (breadcrumbs/network/console continued and segments kept flushing), so it looked like recording just froze. Every rrweb event flows through the `getHandleRecordingEmit` callback, which had no try/catch. Because our rrweb `errorHandler` returns `undefined`, rrweb re-throws anything that escapes the callback and can tear down recording or leave the mutation buffer permanently locked. Separately, the buffer->session conversion tears down rrweb and restarts it; if that restart failed (rrweb's `record()` can throw or silently return `undefined`), `_stopRecording` was left unset with no recovery while the integration stayed enabled. - Wrap the emit callback so an exception can never escape back into rrweb. - Detect a missing recorder after `startRecording()` and surface it. - Add a bounded flush-time watchdog that restarts a recorder which died while the integration is still enabled and unpaused. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
billyvg
changed the title
billyvg
force-pushed
the
fix/replay-recording-resilience
branch
from
July 8, 2026 21:59
ba7e40f to
2379448
Compare
billyvg
marked this pull request as ready for review
July 8, 2026 22:13
billyvg
requested review from
Lms24,
logaretm and
mydea
and removed request for
a team
July 8, 2026 22:13
![cursor[bot]](https://cdn.github.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 1280dd6. Configure here.
| }); | ||
| } catch (error) { | ||
| replay.handleException(error); | ||
| } |
There was a problem hiding this comment.
First-event flag after failed emit
Medium Severity
The emit callback sets hadFirstEvent to true before mirror sync, click handling, and addUpdate run. When the new try/catch swallows a throw from those steps, recording continues but the failed event was never buffered while hadFirstEvent stays true. Later events may no longer be treated as the first checkout unless rrweb passes _isCheckout, skipping checkout-only work like buffer resets, settings meta, and session-mode flush.
Reviewed by Cursor Bugbot for commit 1280dd6. Configure here.
![sentry[bot]](https://cdn.github.com/in/12637?s=60&v=4){kind=small}
| // `_isCheckout` is only set when the checkout is due to `checkoutEveryNms` | ||
| // We also want to treat the first event as a checkout, so we handle this specifically here | ||
| const isCheckout = _isCheckout || !hadFirstEvent; | ||
| hadFirstEvent = true; |
Contributor
There was a problem hiding this comment.
Bug: The hadFirstEvent flag is set prematurely. If the first event fails to process, subsequent events won't be treated as the initial checkout, potentially missing the initial snapshot.
Severity: LOW
Suggested Fix
Move the line this.hadFirstEvent = true; to the end of the try block, after this.replay.addUpdate(). This ensures the flag is only set after the first event has been successfully processed and added to the replay buffer, preventing a race condition where a failed first event corrupts the state.
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: packages/replay-internal/src/util/handleRecordingEmit.ts#L41
Potential issue: In `handleRecordingEmit`, the `hadFirstEvent` flag is set to `true` at
the beginning of the `try` block. If an error occurs later in the block, such as in
`syncMirrorAttributesFromMutationEvent` or `replay.addUpdate()`, the event processing
fails, but the flag remains `true`. Consequently, the next event will not be treated as
the first event (a "checkout"), which can lead to the initial recording segment being
flushed without a snapshot. While the system has mitigations, such as periodic checkouts
and logic to handle segments without checkouts, this race condition can still result in
a recording segment missing its initial state for a period of time, particularly in
buffer mode.
Did we get this right? 👍 / 👎 to inform future reviews.
Member
Author
There was a problem hiding this comment.
technically valid, but i want to keep the scope of this pr low.
logaretm
approved these changes
Jul 8, 2026
Lms24
approved these changes
Jul 9, 2026
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
3 participants
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.
rrweb session-replay recording could stop entirely while the rest of the Replay integration kept working — breadcrumbs, network, and console events continued and segments kept flushing, so it looked like "recording just froze." This hardens the emit callback so a throw inside it can no longer escape back into rrweb and tear recording down.
Root cause
Every rrweb event type flows through the single
getHandleRecordingEmitcallback, which had notry/catch. Because Sentry's rrweberrorHandlerreturnsundefined, rrweb re-throws any error that escapes the callback (callbackWrapper), which can tear down recording or leave the mutation buffer permanently locked.Changes
handleRecordingEmit.ts: wrap the emit callback body intry/catch→handleException, so an exception can never escape back into rrweb.Includes a real-rrweb integration reproduction of the upstream lock hazard plus unit coverage of the emit guard.
Follow-up
@sentry-internal/rrwebfork (wraptakeFullSnapshot's lock/unlock intry/finally, and unlock on the!nodeearly-return) will close the "stuck locked" hazard at the source._ensureRecordingIsRunning+ bounded retry) was split out of this PR to keep the scope focused and will follow separately.🤖 Generated with Claude Code