Merge branch 'next' into je/PLAT-10810-auto-retry-builds

CHANGELOG.md

@@ -6,6 +6,8 @@
 
 - (electron) Fix startup crash when using Electron v26+ on Linux [#2022](https://github.com/bugsnag/bugsnag-js/pull/2022)
 
+- (electron) Fix unhandled secondary errors during delivery [#2025](https://github.com/bugsnag/bugsnag-js/pull/2025)
+
 ## 7.22.0 (2023-09-13)
 
 ### Changes

docker-compose.yml

@@ -6,6 +6,7 @@ x-common-environment: &common-environment
   BUILDKITE_BUILD_CREATOR:
   BUILDKITE_BUILD_NUMBER:
   BUILDKITE_BUILD_URL:
+  BUILDKITE_JOB_ID:
   BUILDKITE_LABEL:
   BUILDKITE_MESSAGE:
   BUILDKITE_PIPELINE_NAME:

packages/delivery-electron/delivery.js

@@ -5,29 +5,43 @@ const PayloadDeliveryLoop = require('./payload-loop')
 const NetworkStatus = require('@bugsnag/electron-network-status')
 
 const delivery = (client, filestore, net, app) => {
+  const noop = () => {}
+
   const send = (opts, body, cb) => {
-    const errorHandler = err => {
-      err.isRetryable = true
-      cb(err)
+    let errorCallback = (error, response) => {
+      // an error can still happen on both the request and response even after a response is received,
+      // so we noop on subsequent calls to ensure this is only handled once
+      errorCallback = noop
+
+      if (response) {
+        error.isRetryable = isRetryable(response.statusCode)
+        // do not retry oversized payloads regardless of status code
+        if (body.length > 10e5) {
+          client._logger.warn(`Discarding over-sized event (${(body.length / 10e5).toFixed(2)} MB) after failed delivery`)
+          error.isRetryable = false
+        }
+      } else {
+        error.isRetryable = true
+      }
+
+      cb(error)
     }
 
     const req = net.request(opts, response => {
-      req.removeListener('error', errorHandler)
+      // handle errors on the response stream
+      response.on('error', err => {
+        errorCallback(err, response)
+      })
+
       if (isOk(response)) {
         cb(null)
       } else {
         const err = new Error(`Bad status code from API: ${response.statusCode}`)
-        err.isRetryable = isRetryable(response.statusCode)
-        // do not retry oversized payloads regardless of status code
-        if (body.length > 10e5) {
-          client._logger.warn(`Discarding over-sized event (${(body.length / 10e5).toFixed(2)} MB) after failed delivery`)
-          err.isRetryable = false
-        }
-        cb(err)
+        errorCallback(err, response)
       }
     })
 
-    req.on('error', errorHandler)
+    req.on('error', err => errorCallback(err))
 
     try {
       req.write(body)

packages/delivery-electron/test/delivery.test-main.ts

@@ -337,6 +337,60 @@ describe('delivery: electron', () => {
     })
   })
 
+  it('ignores secondary request/response errors after a response is received', done => {
+    const payload = {
+      events: [{ errors: [{ errorClass: 'Error', errorMessage: 'foo is not a function' }] }]
+    } as unknown as EventDeliveryPayload
+
+    const secondaryError = new Error('Error after response received')
+
+    // create a mock response object that emits an error event
+    const response = {
+      statusCode: 407,
+      statusMessage: STATUS_CODES[407],
+      on (event: any, cb: any) {
+        if (event === 'error') {
+          cb(secondaryError)
+        }
+      }
+    }
+
+    let requestErrorCallback: any = () => {}
+
+    // create a mock net instance that will return a response and also emit errors
+    const net = {
+      request: (opts: any, responseCallback: any) => ({
+        on (event: any, cb: any) {
+          if (event === 'error') {
+            requestErrorCallback = cb
+          }
+        },
+        write () {},
+        end () {
+          responseCallback(response)
+          requestErrorCallback(secondaryError)
+        }
+      })
+    }
+
+    const logger = { error: jest.fn(), info: jest.fn() }
+    const config = {
+      apiKey: 'aaaaaaaa',
+      endpoints: { notify: 'http://localhost:9999/events/' },
+      redactedKeys: []
+    }
+
+    delivery(filestore, net, app)(makeClient(config, logger)).sendEvent(payload, (err: any) => {
+      expect(err).toBe(secondaryError)
+      expect(err.isRetryable).toBe(false)
+      expect(logger.error).toHaveBeenCalledWith('event failed to send…\n', err.stack)
+      expect(logger.error).toHaveBeenCalledTimes(1)
+      expect(enqueueSpy).not.toHaveBeenCalled()
+
+      done()
+    })
+  })
+
   it('handles uncaught exceptions during event delivery', done => {
     const payload = {
       events: [{ errors: [{ errorClass: 'Error', errorMessage: 'foo is not a function' }] }]

packages/plugin-electron-deliver-minidumps/send-minidump.js

@@ -8,6 +8,8 @@ module.exports = (net, client) => {
   const send = (opts, formData) => {
     return new Promise((resolve, reject) => {
       const req = net.request(opts, response => {
+        response.on('error', reject)
+
         if (isOk(response)) {
           resolve()
         } else {

packages/plugin-electron-deliver-minidumps/test/send-minidump.test.ts

@@ -24,7 +24,7 @@ describe('electron-minidump-delivery: sendMinidump', () => {
   it('sends minidump successfully', async () => {
     const net = {
       request: jest.fn().mockImplementation((_, handle) => {
-        handle({ statusCode: 200 })
+        handle({ statusCode: 200, on: (event, cb) => {} })
       })
     }
 
@@ -48,7 +48,7 @@ describe('electron-minidump-delivery: sendMinidump', () => {
   it('marks server error as retry', async () => {
     const net = {
       request: jest.fn().mockImplementation((opts, handle) => {
-        handle({ statusCode: 500 })
+        handle({ statusCode: 500, on: (event, cb) => {} })
       })
     }
 
@@ -62,7 +62,7 @@ describe('electron-minidump-delivery: sendMinidump', () => {
   it('marks bad request as no-retry', async () => {
     const net = {
       request: jest.fn().mockImplementation((opts, handle) => {
-        handle({ statusCode: 400 })
+        handle({ statusCode: 400, on: (event, cb) => {} })
       })
     }
 
@@ -77,7 +77,7 @@ describe('electron-minidump-delivery: sendMinidump', () => {
     let minidumpFile
     const net = {
       request: jest.fn().mockImplementation((_, handle) => {
-        handle({ statusCode: 200 })
+        handle({ statusCode: 200, on: (event, cb) => {} })
       })
     }