-
-
Notifications
You must be signed in to change notification settings - Fork 371
Do not retry 4xx uploads #6618
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Do not retry 4xx uploads #6618
Conversation
| if (response.statusCode >= 400 && response.statusCode < 500 && response.statusCode != 429) { | ||
| SENTRY_LOG_DEBUG(@"Received 4xx response code: %li", (long)response.statusCode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
happy to make this specific to 400 status codes if we don't want to go broader, but generally speaking 4xx is "client screwed up and shouldn't try again"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree that 400 usually means the client shouldn't retry again, but I still have to double-check if they might not have a good reason to keep the envelope.
The somewhat good news is that the SDK will delete the faulty envelope once the cache is full. The cache has a default max size of 100 envelopes. So when you log more than 100 errors, the SDK will send out all the other errors. Of course this isn't ideal, but at least the other envelopes end up in Sentry eventually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet. Happy to update this PR. Let me know what you find!
📜 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.