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.
| 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()).
There was a problem hiding this comment.
to be honest I wouldn't, due to our single threaded executor this could lead to all kinds of weird timing issues. Whilst this will eventually triggers the creation of breadcrumbs, etc.. it's purely CPU bound, which should have a minimal effect compared to any IO bound work happening.
|
|
||
| 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
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