[BY-3684]: surface and retry lost response bodies

README.MD

@@ -194,6 +194,8 @@ Handlers have a signature with two items:
 
 Every error carries the request that failed as `e.request` (`{ method, url }`), and HTTP failures also carry `e.status` and the parsed response as `e.body`. Errors thrown by `fetch` itself, such as network failures or aborts, have `e.request` but no `e.status`.
 
+If the connection drops while a successful response body is downloading, the request fails with an error that reaches the `default` handler, with the original error on `e.cause`. Invalid JSON still resolves `undefined`, as before.
+
 ctx can be whatever you want really - it is whatever you pass in as `context(...)`.
 
 However, if the `context` object you pass in has a `fetch` function, this is used as the `fetch` for XHR requests.
@@ -289,7 +291,7 @@ Api.configure({ retry: false })                                // never retry
 Good to know:
 
 - Timeouts and HTTP error responses (4xx, 5xx) aren't retried.
-- Retries only apply server-side, as browsers don't expose network error codes.
+- Retries only apply server-side, as browsers don't expose network error codes. The exception is a response body lost mid-download, which is retried in both environments, but only for GET, HEAD and OPTIONS.
 - Each retry logs a warning, e.g. `Got ECONNRESET when calling https://…/api/v1/apps. Retrying request (1/1)`.
 
 ## Parsing error payloads

lib/index.js

@@ -64,6 +64,17 @@ class BadDataError extends HttpError {}
 /** 429 Too Many Requests error */
 class TooManyRequestsError extends HttpError {}
 
+/** Response body lost mid-download */
+class BodyReadError extends HttpError {
+  /**
+   * @param {Error} cause
+   */
+  constructor (cause) {
+    super(cause.message)
+    this.cause = cause
+  }
+}
+
 /**
  * Map of HTTP status codes to error classes
  * @type {Record<number, typeof HttpError>}
@@ -96,6 +107,8 @@ function getErrorByCode (code) {
   return errorMapping[code] || HttpError
 }
 
+const SAFE_METHODS = [ 'GET', 'HEAD', 'OPTIONS' ]
+
 /**
  * @param {ApiOptions['retry']} retry
  * @returns {{ attempts: number, errors: string[], methods: string[] }}
@@ -108,7 +121,7 @@ function retrySettings (retry) {
   const {
     attempts = 1,
     errors = [ 'ECONNRESET' ],
-    methods = [ 'GET', 'HEAD', 'OPTIONS' ]
+    methods = SAFE_METHODS
   } = retry || {}
 
   return { attempts, errors, methods }
@@ -313,6 +326,9 @@ class Api {
           try {
             json = await r.json()
           } catch (e) {
+            if (e instanceof TypeError) {
+              throw new BodyReadError(e)
+            }
             // @ts-ignore
             console.error('Unable to parse response json', e.message)
           }
@@ -336,11 +352,14 @@ class Api {
       const code = e.code || (e.cause && e.cause.code)
       const retriesCount = attempt - 1
       const hasAttemptsLeft = retriesCount < retry.attempts
-      const isRetryableError = retry.errors.includes(code)
-      const isRetryableMethod = retry.methods.includes(options.method)
+      const isBodyReadError = e instanceof BodyReadError
+      const isRetryableError = isBodyReadError || retry.errors.includes(code)
+      const isRetryableMethod = retry.methods.includes(options.method) &&
+        (!isBodyReadError || SAFE_METHODS.includes(options.method))
 
       if (hasAttemptsLeft && isRetryableError && isRetryableMethod) {
-        console.warn(`Got ${code} when calling ${endpoint}. Retrying request (${attempt}/${retry.attempts})`)
+        const reason = isBodyReadError ? `Lost response body (${e.message})` : `Got ${code}`
+        console.warn(`${reason} when calling ${endpoint}. Retrying request (${attempt}/${retry.attempts})`)
         return this.#doQuery(++attempt, client, endpoint, options)
       }
 

lib/index.spec.js

@@ -20,6 +20,10 @@ function fetchFailed (code) {
   return new TypeError('fetch failed', { cause })
 }
 
+function bodyFailed (error) {
+  return response({ json: stub().rejects(error) })
+}
+
 function errorWithCode (code) {
   const error = new Error(`read ${code}`)
   error.code = code
@@ -237,7 +241,7 @@ describe('util/api', () => {
 
       it('logs through the context logger before the default handler runs', async () => {
         const calls = []
-        const logger = { error: (details, message) => calls.push(['log', details, message]) }
+        const logger = { error: (details, message) => calls.push([ 'log', details, message ]) }
 
         const api = createApi()
 
@@ -246,7 +250,7 @@ describe('util/api', () => {
         await api
           .context({ fetch: clientStub, logger })
           .endpoint('some/url')
-          .default(() => calls.push(['handler']))
+          .default(() => calls.push([ 'handler' ]))
           .get()
 
         expect(calls.map(c => c[0])).to.equal([ 'log', 'handler' ])
@@ -294,6 +298,22 @@ describe('util/api', () => {
       })
     })
 
+    context('body read failure', function () {
+      beforeEach(async function () {
+        const clientStub = stub().resolves(bodyFailed(new TypeError('Failed to fetch')))
+
+        await createApi({ retry: false })
+          .context({ fetch: clientStub })
+          .endpoint('some/url')
+          .default(e => { this.received = e })
+          .get()
+      })
+
+      it('passes the dropped connection to the default handler as the cause', function () {
+        expect(this.received.cause).to.be.an.error(TypeError, 'Failed to fetch')
+      })
+    })
+
     context('responds ok', () => {
       let api
       let clientStub
@@ -768,6 +788,88 @@ describe('util/api', () => {
       })
     })
 
+    context('body read failure retry', function () {
+      beforeEach(function () {
+        this.warn = stub(console, 'warn')
+        this.clientStub = stub()
+        this.clientStub.onSecondCall().resolves(response({ body: { foo: 'bar' } }))
+      })
+
+      afterEach(function () {
+        this.warn.restore()
+      })
+
+      context('GET with a lost body', function () {
+        beforeEach(async function () {
+          this.clientStub.onFirstCall().resolves(bodyFailed(new TypeError('Failed to fetch')))
+
+          this.res = await createApi()
+            .context({ fetch: this.clientStub })
+            .endpoint('some/url')
+            .get(({ foo }) => foo)
+        })
+
+        it('retries once', function () {
+          expect(this.clientStub.callCount).to.equal(2)
+        })
+
+        it('returns the retried value', function () {
+          expect(this.res).to.equal('bar')
+        })
+
+        it('warns with the cause message', function () {
+          expect(this.warn.firstCall.args[0]).to.equal('Lost response body (Failed to fetch) when calling https://example.com/api/v1/some/url. Retrying request (1/1)')
+        })
+      })
+
+      context('GET with a body that is not JSON', function () {
+        beforeEach(async function () {
+          this.clientStub.onFirstCall().resolves(bodyFailed(new SyntaxError('Unexpected token <')))
+
+          await createApi()
+            .context({ fetch: this.clientStub })
+            .endpoint('some/url')
+            .get()
+        })
+
+        it('does not retry', function () {
+          expect(this.clientStub.callCount).to.equal(1)
+        })
+      })
+
+      context('POST', function () {
+        beforeEach(async function () {
+          this.clientStub.onFirstCall().resolves(bodyFailed(new TypeError('Failed to fetch')))
+
+          await createApi()
+            .context({ fetch: this.clientStub })
+            .endpoint('some/url')
+            .default(() => {})
+            .post()
+        })
+
+        it('does not retry', function () {
+          expect(this.clientStub.callCount).to.equal(1)
+        })
+      })
+
+      context('POST with methods configured', function () {
+        beforeEach(async function () {
+          this.clientStub.onFirstCall().resolves(bodyFailed(new TypeError('Failed to fetch')))
+
+          await createApi({ retry: { methods: [ 'POST' ] } })
+            .context({ fetch: this.clientStub })
+            .endpoint('some/url')
+            .default(() => {})
+            .post()
+        })
+
+        it('does not retry', function () {
+          expect(this.clientStub.callCount).to.equal(1)
+        })
+      })
+    })
+
     context('error thrown in success handler is not caught', () => {
       const clientStub = stub()
       const defaultErrorHandler = stub()

lib/types.ts

@@ -24,7 +24,7 @@ export type ApiOptions = {
 export type RetryOptions = {
   /** Number of retries after the first request. 0 never retries. Defaults to 1 */
   attempts?: number;
-  /** Node network error codes to retry on, matched on the error or its cause. Defaults to ['ECONNRESET'] */
+  /** Node network error codes to retry on, matched on the error or its cause. Defaults to ['ECONNRESET']. A response body lost mid-download on GET, HEAD, OPTIONS always retries, regardless of this list */
   errors?: string[];
   /** HTTP methods that may be retried. Defaults to ['GET', 'HEAD', 'OPTIONS'] */
   methods?: string[];