chore(curl): Record envelope losses in the Curl transport#1175
Open
szokeasaurusrex wants to merge 2 commits into
Open
chore(curl): Record envelope losses in the Curl transport#1175szokeasaurusrex wants to merge 2 commits into
szokeasaurusrex wants to merge 2 commits into
Conversation
01e1dfc to
a400549
Compare
9e2510b to
6cce3c1
Compare
a400549 to
30b705d
Compare
6cce3c1 to
8f54fdc
Compare
d0fb9ec to
bd8072f
Compare
8f54fdc to
1a8ff61
Compare
bd8072f to
28f9398
Compare
31469a9 to
9c4fe2c
Compare
28f9398 to
1d70a92
Compare
1d70a92 to
d566a6d
Compare
9c4fe2c to
044bcda
Compare
044bcda to
5a39025
Compare
d566a6d to
0d1f9c7
Compare
5a39025 to
4bed875
Compare
0d1f9c7 to
78d07ac
Compare
4bed875 to
7b23f11
Compare
78d07ac to
c5ba012
Compare
7b23f11 to
fdfd1f2
Compare
c5ba012 to
de93acf
Compare
fdfd1f2 to
b4061c2
Compare
de93acf to
c80c467
Compare
Comment on lines
+123
to
+129
| envelope | ||
| .to_writer(&mut body) | ||
| .inspect_err(|_| { | ||
| client_report_recorder | ||
| .record_lost_envelope(&envelope, LossReason::InternalError) | ||
| }) | ||
| .expect("envelope should serialize successfully"); |
There was a problem hiding this comment.
Bug: The code records an envelope loss and then immediately panics via .expect(), crashing the background transport thread and preventing future envelope sends.
Severity: MEDIUM
Suggested Fix
The pattern of recording a loss and then panicking is inconsistent. The .expect() should be removed to allow the envelope to be dropped gracefully after the loss is recorded. This would prevent the transport thread from crashing and allow the application to continue attempting to send future events. Alternatively, if a panic is desired, the loss recording should be removed as it is redundant.
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: sentry/src/transports/curl.rs#L123-L129
Potential issue: The code in the curl transport has a logical inconsistency for handling
envelope serialization failures. In the event of a `to_writer()` failure, the code first
uses `inspect_err()` to record the envelope loss with `LossReason::InternalError`, but
then immediately calls `.expect()`, which causes a panic. This panic occurs within a
detached background transport thread that has no panic recovery mechanism. Consequently,
the thread will crash silently, which stops all subsequent envelopes from being sent for
the remainder of the application's session. While serialization failure is rare, this
handling is problematic because it records a loss and then terminates the transport.
Also affects:
sentry/src/transports/reqwest.rs:128~133
b4061c2 to
546b283
Compare
c80c467 to
5c62ea2
Compare
Record lost envelopes in the `curl` transport after the transport thread accepts them for sending. Curl execution failures are recorded as `network_error`; response-code lookup failures and non-`429` HTTP `4xx`/`5xx` responses are recorded as `send_error`. Keep existing rate-limit handling for `429` responses and the existing payload-too-large debug message for `413` responses. Closes [#1152](#1152) Closes [RUST-227](https://linear.app/getsentry/issue/RUST-227)
546b283 to
e1b2e92
Compare
5c62ea2 to
398ca7c
Compare
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
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.
Record lost envelopes in the
curltransport after the transport thread accepts them for sending. Curl execution failures are recorded asnetwork_error; response-code lookup failures and non-429HTTP4xx/5xxresponses are recorded assend_error.Keep existing rate-limit handling for
429responses and the existing payload-too-large debug message for413responses.Closes #1152
Closes RUST-227