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.

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

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

Merged
merged 6 commits into from
Oct 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion packages/delivery-expo/delivery.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,14 @@ module.exports = (client, fetch = global.fetch) => {
}
client._logger.info(`Sending event ${event.events[0].errors[0].errorClass}: ${event.events[0].errors[0].errorMessage}`)
send(url, opts, err => {
if (err) return onerror(err, { url, opts }, 'event', cb)
if (err) {
// do not retry oversized payloads regardless of status code
if (body.length > 10e5) {
client._logger.warn(`Discarding over-sized event (${body.length / 10e5} MB) after failed delivery`)
err.isRetryable = false
}
return onerror(err, { url, opts }, 'event', cb)
}
cb(null)
})
} catch (e) {
Expand Down
37 changes: 37 additions & 0 deletions packages/delivery-expo/test/delivery.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,43 @@ describe('delivery: expo', () => {
})
})

it('does not attempt to re-send oversized payloads', done => {
// A 401 is considered retryable but this will be overridden by the payload size check
const { requests, server } = mockServer(401)
server.listen(err => {
expect(err).toBeUndefined()

const lotsOfEvents = []
while (JSON.stringify(lotsOfEvents).length < 10e5) {
lotsOfEvents.push({ errors: [{ errorClass: 'Error', errorMessage: 'long repetitive string'.repeat(1000) }] })
}
const payload = {
events: lotsOfEvents
}

const config = {
apiKey: 'aaaaaaaa',
endpoints: { notify: `http://0.0.0.0:${server.address().port}/notify/` },
redactedKeys: []
}

const logger = {
info: jest.fn(),
warn: jest.fn(),
error: jest.fn()
}

delivery({ _config: config, _logger: logger }, fetch).sendEvent(payload, (err) => {
expect(logger.warn).toHaveBeenCalledWith('Discarding over-sized event (1.014603 MB) after failed delivery')
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

expect(err).toBeTruthy()
expect(requests.length).toBe(0)
server.close()
done()
})
})
})

it('handles errors gracefully for sessions (ECONNREFUSED)', done => {
const payload = {
events: [{ errors: [{ errorClass: 'Error', errorMessage: 'foo is not a function' }] }]
Expand Down
Loading