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>
…shepherd] # Conflicts: # CHANGELOG.md Co-Authored-By: Markus Hintersteiner <markus.hintersteiner@sentry.io>
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.
|
|
||
| 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.
There was a problem hiding this comment.
Yes, it's usually not required for anything marked ApiStatus.Internal, I'll double check that the hybrid SDK are not using it.
![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.
…getsentry/sentry-java into fix/okhttp-unknown-length-response-body
|
|
||
| val breadcrumbData = mutableMapOf<String, Any?>() | ||
|
|
||
| val networkDetailData = httpNetworkDetails.remove(breadcrumb) |
There was a problem hiding this comment.
m: If a segment is built before the body is read, could the body from the second setResponseDetails() call get lost here? (It looks like the entry is removed when the replay converter first processes the breadcrumb.)
Could also be smart to include a test that converts the breadcrumb before the body is consumed.
| override fun close() { | ||
| // Closing the delegate notifies listeners, which may serialize the breadcrumb this capture | ||
| // belongs to, so report first. | ||
| delegate.use { reportCaptured() } |
There was a problem hiding this comment.
m: Should close() also close capturingSource if it was created? Without it, seems the app could keep reading buffered bytes.
|
|
||
| private fun reportCaptured() { | ||
| if (reported.compareAndSet(false, true)) { | ||
| onCaptured(synchronized(captured) { captured.clone().readByteArray() }) |
There was a problem hiding this comment.
m: Thoughts about draining instead of clone here + skip copying once reported? Otherwise a long SSE stream might keep the buffer alive and acquire the lock on every read.
| // This runs on the thread consuming the body, inside its read or close, so a failure in | ||
| // here must stay in here. | ||
| try { | ||
| recordResponse(networkDetailData, knownBodySize ?: capturedBytes.size.toLong()) { |
There was a problem hiding this comment.
l: When a body is closed early or a read fails, is the prefix treated as a complete body? Wondering whether a cut-off JSON body would show up as INVALID_JSON rather than truncated, and whether the size would be off...
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