Skip to content
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.

Sign up for GitHub

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

Jump to bottom

explicitly mark failed payloads >1MB as not retryable #65

Merged
merged 6 commits into from
Oct 19, 2023
Merged

explicitly mark failed payloads >1MB as not retryable #65

merged 6 commits into from
Oct 19, 2023

Conversation

djskinner
Copy link
Contributor

@djskinner djskinner commented Sep 29, 2022 •
edited
Loading

Goal

  • To prevent unexpected behaviour causing the retry of oversized payloads

Design

  • Add an explicit check on the payload size. If > ~1MB then set err.isRetryable = false.
  • Manual testing showed that the 400 response received from the pipeline for oversized payloads does not get treated as a retryable error. This additional check makes doubly sure regardless of the HTTP response code but it is not expected to occur in normal operation.

Testing

  • Manual testing
  • Unit testing

djskinner
djskinner commented Sep 29, 2022
View reviewed changes
packages/delivery-expo/test/delivery.test.js

delivery({ _config: config, _logger: { error: log, info: () => {} } }, fetch).sendEvent(payload, (err) => {
expect(didLog).toBe(true)
expect(enqueueSpy).not.toHaveBeenCalled()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests are going to fail here until https://github.com/bugsnag/bugsnag-js/pull/1823/files#diff-b6197bd8422edfa11579021015a96f02f31bc20634ad051b821c01e4ce178afdL18 lands in the version of @bugsnag/core used by bugsnag exp

@djskinner djskinner mentioned this pull request Oct 10, 2022
Merged
6 tasks
@djskinner djskinner changed the base branch from v46-next to v47/next February 22, 2023 15:11
@djskinner djskinner marked this pull request as ready for review February 23, 2023 09:43
@djskinner djskinner changed the base branch from v47/next to v49/next October 2, 2023 12:10
@tomlongridge
Copy link
Contributor

@yousif-bugsnag - this is now ready for review as the dependency in bugsnag-js is all there.

yousif-bugsnag
yousif-bugsnag approved these changes Oct 17, 2023
View reviewed changes
Copy link
Contributor

@yousif-bugsnag yousif-bugsnag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@djskinner djskinner merged commit dc25073 into v49/next Oct 19, 2023
3 checks passed
@djskinner djskinner deleted the plat-8731 branch October 19, 2023 09:59
@yousif-bugsnag yousif-bugsnag mentioned this pull request Oct 27, 2023
Merged
@gingerbenw gingerbenw mentioned this pull request Dec 6, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yousif-bugsnag yousif-bugsnag yousif-bugsnag approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@djskinner @tomlongridge @yousif-bugsnag