Repository navigation
Conversation
Capturing a response body for session replay peeked it up front with `Response.peekBody(MAX_NETWORK_BODY_SIZE + 1)`. OkHttp implements a peek as `request(byteCount)`, which keeps reading until that many bytes are buffered or the stream ends. A response with no Content-Length never satisfies either condition, so for server-sent events, long-poll and any chunked endpoint that stays open the interceptor never returned and the caller never received the response at all. Bodies with a known length still end on their own, so they keep the existing up-front capture. Bodies of unknown length are now wrapped in a body that copies what the application consumes into a capped buffer and reports it once no more bytes can arrive: when the stream ends, when the application closes the body, or when the cap is reached. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Split the capture wrapping into its own function to keep a single return, and suppress TooManyFunctions on the interceptor as done elsewhere in this module.
A response body has a single consumer, and neither Http1ExchangeCodec.cancel() nor Http2ExchangeCodec.cancel() closes the body, so nothing reaches this class from another thread. The AtomicBoolean already guarantees the capture is reported exactly once.
A streamed body is only known once it has been consumed, which can be after the NetworkRequestData carrying it was handed to the scope, so the replay thread can read it while it is still being written. Keeping the three response values behind one volatile reference means a reader sees either nothing or the complete set.
📲 Install BuildsAndroid
|
…shepherd] Co-Authored-By: Markus Hintersteiner <markus.hintersteiner@sentry.io>
|
🐑 PR Shepherd — diagnosed head
Actions
|
…shepherd] # Conflicts: # CHANGELOG.md Co-Authored-By: Markus Hintersteiner <markus.hintersteiner@sentry.io>
|
PR shepherd (skill)
|
markushi
changed the title
There was a problem hiding this comment.
Thanks a bunch for this 💯 ! A few comments from my end below.
Also, out of scope for now, but it looks like there might be a request-side version of this bug, in that the request body is still read into memory before the request is sent (link). Presumably that means streaming, duplex, or large uploads to an allowlisted URL can hang / lose data / trigger an OOM.
Let me know if you want me to open an issue 👍
| * @param delegate the body to capture from. | ||
| * @param maxBytes the maximum number of bytes to retain; capture stops once it is reached. | ||
| * @param onCaptured invoked at most once, synchronously, on the thread that finishes the body — so | ||
| * it must not block. A body that is abandoned without being read to the end or closed never |
There was a problem hiding this comment.
l: Double-checking that the onCaptured KDoc is correct? I see, eg, that we invoke the callback from CapturedResponseBody.read() if we hit maxBytes or encounter an IOException.
| override fun source(): BufferedSource = capturingSource | ||
|
|
||
| override fun close() { | ||
| try { |
There was a problem hiding this comment.
nit: we can replace try + finally with delegate.use if you like
| super.read(sink, byteCount) | ||
| } catch (e: IOException) { | ||
| // What arrived is the evidence for this very failure, and a caller handling it is not | ||
| // obliged to close the body. |
There was a problem hiding this comment.
nit: Fwiw, I can't quite figure out what the LLM was trying to say here.
| // the application may not have taken everything in the sink yet, hence the offset | ||
| val toTake = minOf(maxBytes - captured.size, sink.size - sinkBefore) | ||
| if (toTake > 0L) { | ||
| sink.copyTo(captured, sinkBefore, toTake) |
There was a problem hiding this comment.
Buffer.copyTo() method that takes an OutputStream rather than the one that takes a Buffer.
Looks like Buffer.copyTo(Buffer, ...) doesn't immediately copy bytes, but instead (eventually) calls Segment.sharedCopy() (link). That means:
- each captured section will keep a reference to the sink segment's entire 8 KB byte array (link), and
- the segments won't be returned to the segment pool (link)
My LLM tells me that for SSE, long-poll, or chunked streams with small events, each small event will be forced to retain a full 8 KB array. With 10-byte heartbeats, that makes for ~120 MB of memory leaked per stream.
The Buffer.copyTo() method that takes an OutputStream doesn't share the segment, and so we avoid leaking it (link).
| return newBuilder() | ||
| .body( | ||
| CapturedResponseBody(responseBody, captureCap) { capturedBytes -> | ||
| // This runs on the thread consuming the body, inside its read or close, so a failure in |
There was a problem hiding this comment.
h: Thoughts about calling recordResponse() from our executor service?
I'm worried about delaying delivery of SSE events when the onCaptured callback here is invoked from CapturedResponseBody.read(), and causing app frames to stutter when called from .close() (as apps often cancel a stream or close a response from onDestroy()).
| ExceptionUtils.rethrowIfFatal(e) | ||
| logger.log( | ||
| io.sentry.SentryLevel.ERROR, | ||
| "Failed to capture the http response body for Network Details: ${e.message}", |
There was a problem hiding this comment.
l: We could also log e to pick up the stack traces.
| } | ||
|
|
||
| @Test | ||
| fun `a failing capture callback does not reach the application`() { |
There was a problem hiding this comment.
l: Maybe: closes the delegate even when the capture callback throws? (Looks like the current name is the opposite of what it should be, given that we assert wrapper.close() is called)
|
|
||
| public static <T> @NotNull ReplayNetworkRequestOrResponse createResponse( | ||
| @NotNull final T httpObject, | ||
| @Nullable final Integer statusCode, |
There was a problem hiding this comment.
Might be worth calling the binary incompatible changes out in the CHANGELOG since they show up in sentry rather than sentry-okhttp??
That said, happy to defer to you, esp if we always assume breaking changes to ApiStatus.Internal types are allowed.
|
|
||
| ### Fixes | ||
|
|
||
| - Fix `SentryOkHttpInterceptor` hanging forever on responses whose body has no known length, such as Server-Sent Events or a gzipped response ([#6231](https://github.com/getsentry/sentry-java/pull/6231)) |
There was a problem hiding this comment.
l: Might want to add a "Session Replay:" prefix to this so we don't suggest this was happening to everyone (could also add a qualification that it only impacted folks who had network details turned on if you think that'd be helpful).
![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 2 potential issues.
❌ 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 4dbcbfa. Configure here.
| // the application may not have taken everything in the sink yet, hence the offset | ||
| val toTake = minOf(maxBytes - captured.size, sink.size - sinkBefore) | ||
| if (toTake > 0L) { | ||
| sink.copyTo(captured, sinkBefore, toTake) |
There was a problem hiding this comment.
Shared segments inflate capture memory
High Severity
Buffer.copyTo(Buffer) shares each Okio segment instead of copying bytes, so every small SSE/chunked read retains an entire 8 KB array. A stream that arrives as many small events can pin tens of megabytes until the cap is hit, and reportCaptured never clears captured, so that memory stays for the life of the body.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 4dbcbfa. Configure here.
| io.sentry.SentryLevel.ERROR, | ||
| "Failed to capture the http response body for Network Details: ${e.message}", | ||
| ) | ||
| } |
There was a problem hiding this comment.
New broad Throwable catch added
Low Severity
This new catch (Throwable) wraps SDK code (recordResponse and NetworkBodyParser.fromBytes), not a user or third-party callback. The project rule allows that pattern only at those entry points, even when rethrowIfFatal is used.
Triggered by project rule: PR Review Guidelines for Cursor Bugbot
Reviewed by Cursor Bugbot for commit 4dbcbfa. Configure here.
This branch has not been deployed
3 participants
📜 Description
SentryOkHttpInterceptorcaptured the response body withpeekBody(MAX_NETWORK_BODY_SIZE + 1). A peekis
request(n), which waits fornbytes or for the end of the stream — a body with noContent-Lengthdelivers neither, so the interceptor never returned and the application neverreceived the response.
Every response body now goes through the new
CapturedResponseBody, which copies the bytes theapplication reads into a capped buffer. Nothing is read for the capture itself. The status code and
the headers are recorded before anything is consumed, so a stream that stays open still produces a
usable breadcrumb.
💡 Motivation and Context
With Session Replay network details enabled for a URL, every Server-Sent Events, long-poll or open
chunked endpoint hung forever or failed with a
SocketTimeoutException. Gzipped responses wereaffected too: OkHttp strips
Content-Lengthwhen it decompresses, so an application interceptor sees-1.Continues carlonzo#1 by @carlonzo
Fixes #6138
💚 How did you test it?
Added unit tests, including a regression test that hangs without the fix.
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps