Repository navigation
fix(session-recorder): prevent unbounded error loop from corrupted TextDecoder in emit - #305
NickSolante wants to merge 4 commits into
Conversation
🦋 Changeset detectedLatest commit: f66c0e4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
cb52d8d to
c43ff08
Compare
…xtDecoder in emit WebKit's TextDecoder can permanently corrupt after high cumulative decode volume and then throw `RangeError: Bad value` on valid input (https://bugs.webkit.org/show_bug.cgi?id=286266). Because emit() reused one module-level decoder with no error handling, a single corrupted instance turned every rrweb flush into an uncaught error (~40/sec) until page unload. - Skip the encode/decode round-trip entirely for single-chunk events, with a length*3 fast path that avoids encoding on the hot path at all - Use a fresh per-event TextDecoder for multi-chunk events, with { stream: true } so chunk boundaries no longer bisect multi-byte characters into U+FFFD - Retry a failed decode once with a new decoder - Stop the recorder after 10 consecutive emit failures instead of erroring on every event forever Circuit-breaker hardening (from review): - init() resets paused/consecutiveEmitFailures so a re-init after a breaker trip records again with the full failure tolerance - Every emit failure is surfaced via diag.error, not just in debug - All chunks are converted before any is sent, so a mid-loop failure can't export a partial (unreassemblable) chunk set - The breaker's deinit() can't throw back into rrweb, deinit() clears inited before calling stop, and init() finishes teardown if the trip happened during rrweb's synchronous initial snapshot - emit tests are now self-contained per it-block Fixes hyperdxio#303
c43ff08 to
ac43184
Compare
| expect(mockOnLog).toHaveBeenCalledTimes(1); | ||
| expect(mockStopRecording).toHaveBeenCalledTimes(1); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
I don't think these tests are actually running anywhere in CI. Mind updating the session-recorder/package.json to add add "ci:unit": "jest --ci" ? That way they will get run with the main ci:unit job on the PR.
| diag.error( | ||
| 'OpenTelemetry Session Recorder: stopping recording after repeated errors', | ||
| e, | ||
| ); |
There was a problem hiding this comment.
just a note here: diag.error only shows when otel-web's debug is true. Do we want these errors to be more visible to the user? If so, maybe we emit a span with this error instead.
There was a problem hiding this comment.
ah, thanks for letting me know. Yea I think we would definitely want the users to know if somethings started breaking.
|
@NickSolante Just a few comments/questions. Sorry for the delay! |
|
@NickSolante bump |
All good, apologies as well will get in this by eow @teeohhem |
…xtDecoder in emit WebKit's TextDecoder can permanently corrupt after high cumulative decode volume and then throw `RangeError: Bad value` on valid input (https://bugs.webkit.org/show_bug.cgi?id=286266). Because emit() reused one module-level decoder with no error handling, a single corrupted instance turned every rrweb flush into an uncaught error (~40/sec) until page unload. - Skip the encode/decode round-trip entirely for single-chunk events - Use a fresh per-event TextDecoder for multi-chunk events, with { stream: true } so chunk boundaries no longer bisect multi-byte characters into U+FFFD - Retry a failed decode once with a new decoder - Stop the recorder after 10 consecutive emit failures instead of erroring on every event forever
52a798e to
f66c0e4
Compare
Fixes #303
Problem
On Safari, the session recorder's
emit()could enter a permanent, unbounded error loop: WebKit'sTextDecodercan permanently corrupt after high cumulative decode volume and then throwRangeError: Bad valueon valid input (WebKit #286266, wasm-bindgen report).emit()reused one module-level decoder for the page lifetime with no error handling, so a single corrupted instance turned every rrweb mutation flush into an uncaught error (~40/sec, observed 64,606 errors over 26 minutes from one session) until page unload. Full telemetry in #303.Changes
TextDecoder. This also removes the cumulative decode volume that ages the decoder toward WebKit's corruption threshold.TextDecoderwith{ stream: true }, which also fixes a latent all-browser bug: byte-slice boundaries that bisected a multi-byte UTF-8 character previously produced U+FFFD replacement characters at chunk seams, silently corrupting reassembled replay JSON.emit()failures are caught (each dropped event is surfaced viaconsole.error), and after 10 consecutive failures the recorder stops instead of erroring on every event forever.init()resets the breaker state so a later recording lifetime starts clean.Tests
splitIntoChunksunit tests: single-chunk identity (no decode), lossless multi-chunk reassembly, a multi-byte character bisected at the chunk boundary (fails against the old non-streaming decode), and recovery via the fresh-decoder retry (first decoder instance always throwsRangeError: Bad value).emit()error-handling tests (mocked rrweb/exporter/tracer): failures don't throw into rrweb dispatch, the breaker trips on the 10th consecutive failure and stops recording, nothing is emitted after tripping, and recording works again after a re-init.compile:tscclean,jest10/10 passing.Notes
@hyperdx/otel-web-session-recorder).MutationRateLimiter's constructor starts asetIntervalthat is never cleared (it also survivesdeinit()); the emit tests use fake timers to work around it. Happy to fix in a follow-up.