sentry-cocoa icon indicating copy to clipboard operation
sentry-cocoa copied to clipboard

Do not retry 4xx uploads

Open dfed opened this issue 1 month ago • 2 comments

:scroll: 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.

:bulb: 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.

:green_heart: How did you test it?

I didn't.

:pencil: Checklist

You have to check all boxes before merging:

  • [ ] I added tests to verify the changes.
  • [x] No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • [x] I updated the docs if needed.
  • [x] I updated the wizard if needed.
  • [ ] Review from the native team if needed.
  • [x] No breaking change or entry added to the changelog.
  • [x] No breaking change for hybrid SDKs or communicated to hybrid SDKs.

dfed avatar Oct 30 '25 22:10 dfed

@philipphofmann are you going to pick this up?

philprime avatar Dec 11 '25 16:12 philprime

@philprime, yes, I plan to next week. I mean, you or anybody else can also pick it up if you have bandwidth.

philipphofmann avatar Dec 12 '25 10:12 philipphofmann

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 avatar Dec 16 '25 14:12 philipphofmann

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 84.922%. Comparing base (8bed6db) to head (4af25a0). :warning: Report is 1 commits behind head on main. :white_check_mark: All tests successful. No failed tests found.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #6618       +/-   ##
=============================================
+ Coverage   84.885%   84.922%   +0.036%     
=============================================
  Files          457       457               
  Lines        27616     27624        +8     
  Branches     12140     12150       +10     
=============================================
+ Hits         23442     23459       +17     
+ Misses        4131      4126        -5     
+ Partials        43        39        -4     
Files with missing lines Coverage Δ
Sources/Sentry/SentryDiscardReasonMapper.m 100.000% <100.000%> (ø)
Sources/Sentry/SentryHttpTransport.m 96.744% <100.000%> (+0.093%) :arrow_up:

... and 4 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8bed6db...4af25a0. Read the comment docs.

codecov[bot] avatar Dec 16 '25 14:12 codecov[bot]

Back to draft because some tests are failing.

philipphofmann avatar Dec 16 '25 15:12 philipphofmann

Thank you all for getting this over the line! Excited for this 🚀

dfed avatar Dec 18 '25 18:12 dfed