✨ Retry specific request error codes (#76)

percy · Nov 6, 2020 · 6ac0633 · 6ac0633
1 parent 75e34e6
commit 6ac0633
Show file tree

Hide file tree

Showing 3 changed files with 65 additions and 35 deletions.
diff --git a/packages/client/src/utils.js b/packages/client/src/utils.js
@@ -97,6 +97,22 @@ export function httpAgentFor(url) {
   });
 }
 
+const RETRY_ERROR_CODES = [
+  'ECONNREFUSED', 'ECONNRESET', 'EPIPE',
+  'EHOSTUNREACH', 'EAI_AGAIN'
+];
+
+// Returns true or false if an error should cause the request to be retried
+function shouldRetryRequest(error) {
+  if (error.response) {
+    return error.response.status >= 500 && error.response.status < 600;
+  } else if (error.code) {
+    return RETRY_ERROR_CODES.includes(error.code);
+  } else {
+    return false;
+  }
+}
+
 // Returns a promise that resolves when the request is successful and rejects
 // when a non-successful response is received. The rejected error contains
 // response data and any received error details. Server 500 errors are retried
@@ -107,45 +123,37 @@ export function request(url, { body, ...options }) {
   options = { ...options, protocol, hostname, port, path: pathname + search };
 
   return retry((resolve, reject, retry) => {
+    let handleError = error => {
+      return shouldRetryRequest(error)
+        ? retry(error) : reject(error);
+    };
+
     http.request(options)
       .on('response', res => {
         let status = res.statusCode;
         let raw = '';
 
-        res.setEncoding('utf8');
-        res.on('data', chunk => {
-          raw += chunk;
-        });
+        res.setEncoding('utf8')
+          .on('data', chunk => (raw += chunk))
+          .on('error', handleError)
+          .on('end', () => {
+            let body = raw;
+            try { body = JSON.parse(raw); } catch (e) {}
 
-        res.on('end', () => {
-          let body = raw;
-
-          // attempt to parse json responses
-          try { body = JSON.parse(raw); } catch (e) {}
-
-          // success
-          if (status >= 200 && status < 300) {
-            resolve(body);
-          } else {
-            // error
-            let err = Object.assign(new Error(), {
-              response: { status, body },
-              // use first error detail or the status message
-              message: body?.errors?.find(e => e.detail)?.detail || (
-                `${status} ${res.statusMessage || raw}`
-              )
-            });
-
-            // retry 500s
-            if (status >= 500) {
-              retry(err);
+            if (status >= 200 && status < 300) {
+              resolve(body);
             } else {
-              reject(err);
+              handleError(Object.assign(new Error(), {
+                response: { status, body },
+                // use first error detail or the status message
+                message: body?.errors?.find(e => e.detail)?.detail || (
+                  `${status} ${res.statusMessage || raw}`
+                )
+              }));
             }
-          }
-        });
+          });
       })
-      .on('error', reject)
+      .on('error', handleError)
       .end(body);
   });
 }
diff --git a/packages/client/test/client.test.js b/packages/client/test/client.test.js
@@ -545,16 +545,33 @@ describe('PercyClient', () => {
       expect(mockAPI.requests['/snapshots/123/finalize']).toHaveLength(4);
     });
 
-    it('fails retrying after 5 attempts', async () => {
-      mockAPI.reply('/snapshots/123/finalize', () => [502, { success: false }]);
-      await expect(client.finalizeSnapshot(123)).rejects.toThrow('502 {"success":false}');
-      expect(mockAPI.requests['/snapshots/123/finalize']).toHaveLength(5);
+    it('retries certain request errors', async () => {
+      mockAPI.cleanAll().nock.persist(false)
+        .post('/snapshots/123/finalize').replyWithError({ code: 'ECONNREFUSED' })
+        .post('/snapshots/123/finalize').replyWithError({ code: 'EHOSTUNREACH' })
+        .post('/snapshots/123/finalize').replyWithError({ code: 'ECONNRESET' })
+        .post('/snapshots/123/finalize').replyWithError({ code: 'EAI_AGAIN' })
+        .post('/snapshots/123/finalize').reply(200, { success: true });
+
+      await expect(client.finalizeSnapshot(123)).resolves.toEqual({ success: true });
+      expect(mockAPI.nock.isDone()).toBe(true);
     });
 
-    it('does not retry request errors', async () => {
+    it('does not retry bad requests or unknown errors', async () => {
       mockAPI.reply('/snapshots/123/finalize', () => [400, { errors: [{ detail: 'Wrong' }] }]);
       await expect(client.finalizeSnapshot(123)).rejects.toThrow('Wrong');
       expect(mockAPI.requests['/snapshots/123/finalize']).toHaveLength(1);
+
+      mockAPI.cleanAll().nock.persist(false)
+        .post('/snapshots/123/finalize').replyWithError(new Error('Unknown'));
+      await expect(client.finalizeSnapshot(123)).rejects.toThrow('Unknown');
+      expect(mockAPI.nock.isDone()).toBe(true);
+    });
+
+    it('fails retrying after 5 attempts', async () => {
+      mockAPI.reply('/snapshots/123/finalize', () => [502, { success: false }]);
+      await expect(client.finalizeSnapshot(123)).rejects.toThrow('502 {"success":false}');
+      expect(mockAPI.requests['/snapshots/123/finalize']).toHaveLength(5);
     });
   });
 

diff --git a/packages/client/test/helper.js b/packages/client/test/helper.js
@@ -70,6 +70,11 @@ const mockAPI = {
     this.replies[path] = this.replies[path] || [];
     this.replies[path].push(handler);
     return this;
+  },
+
+  cleanAll() {
+    nock.cleanAll();
+    return this;
   }
 };