fix: Transport correctly handling 4xx and 5xx#6618
fix: Transport correctly handling 4xx and 5xx#6618philipphofmann merged 24 commits intogetsentry:mainfrom
Conversation
dfed
requested review from
itaybre,
noahsmartin,
philipphofmann and
philprime
as code owners
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@philipphofmann are you going to pick this up? |
|
@philprime, yes, I plan to next week. I mean, you or anybody else can also pick it up if you have bandwidth. |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
|
I manually tested capturing a message with the iOS-Swift sample app let message = Data(repeating: 0, count: 1024*1024*2) // 2 MB message
let string = message.base64EncodedString()
let eventId = SentrySDK.capture(message: string)And the transport successfully deleted the envelope when getting a 400 from Relay. Doing the same thing on the main branch, led to filling up the chache until the faulty envelope with the too large message finally gets deleted due to cache overflow. |
philipphofmann
changed the title
There was a problem hiding this comment.
Bug: Test helper missing new `send_error` discard reason case
The sentryDiscardReasonForString helper function is missing a case for kSentryDiscardReasonNameSendError, which was added by this PR. Since the function has a default case that calls fatalError("Unsupported reason: \(reason)"), any test that uses givenRecordedLostEvents() with a discarded event containing the send_error reason will crash at runtime. The function needs to handle .sendError to match the new discard reason added to the production code.
Tests/SentryTests/Networking/SentryHttpTransportTests.swift#L1055-L1077
sentry-cocoa/Tests/SentryTests/Networking/SentryHttpTransportTests.swift
Lines 1055 to 1077 in 596f651
|
Thank you all for getting this over the line! Excited for this 🚀 |
5 participants
📜 Description
If an upload receives a 4xx error, Sentry should assume that a retry will not succeed and drop it on the floor. Currently Sentry will retry the upload (seemingly forever), which means that a 4xx error on upload will prevent future uploads from succeeding.
💡 Motivation and Context
This PR represents the shortest-path resolution to #6612. I'd prefer a solution where a 4xx error didn't cause a silent failure, or a solution where breadcrumb were automatically truncated. But this change is simple and unsticks existing clients, which seems like a good thing.
💚 How did you test it?
I didn't.
📝 Checklist
You have to check all boxes before merging:
sendDefaultPIIis enabled.