Merge pull request #334 from happo/retry-signed-url

Retry S3 signed URL PUT requests

Author: lencioni

src/uploadAssets.js

@@ -1,3 +1,5 @@
+import retry from 'async-retry';
+
 import { logTag } from './Logger';
 import makeRequest from './makeRequest';
 
@@ -104,20 +106,43 @@ async function uploadAssetsWithSignedUrl(
     return signedUrlRes.path;
   }
 
-  // Upload the assets to the signed URL using node's built-in fetch.
-  const putRes = await fetch(signedUrlRes.signedUrl, {
-    method: 'PUT',
-    body: streamOrBuffer,
-    headers: {
-      'Content-Type': 'application/zip',
-    },
-  });
+  // Upload the assets to the signed URL using node's built-in fetch with
+  // retries
+  await retry(
+    async (bail) => {
+      const res = await fetch(signedUrlRes.signedUrl, {
+        method: 'PUT',
+        body: streamOrBuffer,
+        headers: {
+          'Content-Type': 'application/zip',
+        },
+      });
 
-  if (!putRes.ok) {
-    throw new Error(
-      `Failed to upload assets to S3 signed URL: ${putRes.status} ${putRes.statusText}`,
-    );
-  }
+      if (!res.ok) {
+        const error = new Error(
+          `Failed to upload assets to S3 signed URL: ${res.status} ${res.statusText}`,
+        );
+
+        if (res.status < 500 || res.status >= 600) {
+          // If it's not a 5xx error, bail immediately instead of retrying
+          bail(error);
+          return;
+        }
+
+        throw error;
+      }
+
+      return res;
+    },
+    {
+      retries: 3,
+      onRetry: (error, attempt) => {
+        logger.warn(
+          `${logTag(project)}PUT request attempt ${attempt} failed: ${error.message}. Retrying...`,
+        );
+      },
+    },
+  );
 
   // Finally, we need to tell Happo that we've uploaded the assets.
   const finalizeRes = await makeRequest(

test/integrations/react-test.js

@@ -500,4 +500,84 @@ describe('when HAPPO_SIGNED_URL is set', () => {
       expect(receivedRequests.length).toBe(2);
     });
   });
+
+  describe('when the signed URL 500s the first time', () => {
+    beforeEach(() => {
+      let counter = 0;
+      mockResponses['PUT /a-signed-url'] = {
+        method: 'PUT',
+        url: /\/a-signed-url$/,
+        handler: (req, res) => {
+          counter++;
+          if (counter === 1) {
+            res.statusCode = 500;
+            res.end('Internal Server Error');
+          } else {
+            res.statusCode = 200;
+            res.end('OK');
+          }
+        },
+      };
+    });
+
+    it('retries the request and proceeds', async () => {
+      await expect(subject()).resolves.not.toThrow();
+
+      expect(receivedRequests[0].method).toBe('GET');
+      expect(receivedRequests[0].url).toMatch(/\/assets\/[a-f0-9]+\/signed-url$/);
+
+      expect(receivedRequests[1].method).toBe('PUT');
+      expect(receivedRequests[1].url).toMatch(/\/a-signed-url$/);
+
+      expect(receivedRequests[2].method).toBe('PUT');
+      expect(receivedRequests[2].url).toMatch(/\/a-signed-url$/);
+
+      expect(receivedRequests[3].method).toBe('POST');
+      expect(receivedRequests[3].url).toMatch(
+        /\/assets\/[a-f0-9]+\/signed-url\/finalize$/,
+      );
+
+      // The request after the asset has been finalized is posting the report
+      expect(receivedRequests[4].method).toBe('POST');
+      expect(receivedRequests[4].url).toEqual('/api/reports/foobar');
+
+      expect(receivedRequests.length).toBe(5);
+    });
+  });
+
+  describe('when the signed URL 500s lots of times', () => {
+    beforeEach(() => {
+      mockResponses['PUT /a-signed-url'] = {
+        method: 'PUT',
+        url: /\/a-signed-url$/,
+        handler: (req, res) => {
+          res.statusCode = 500;
+          res.end('Internal Server Error');
+        },
+      };
+    });
+
+    it('retries the request a few times and then throws', async () => {
+      await expect(subject()).rejects.toThrow(
+        'Failed to upload assets to S3 signed URL',
+      );
+
+      expect(receivedRequests[0].method).toBe('GET');
+      expect(receivedRequests[0].url).toMatch(/\/assets\/[a-f0-9]+\/signed-url$/);
+
+      expect(receivedRequests[1].method).toBe('PUT');
+      expect(receivedRequests[1].url).toMatch(/\/a-signed-url$/);
+
+      expect(receivedRequests[2].method).toBe('PUT');
+      expect(receivedRequests[2].url).toMatch(/\/a-signed-url$/);
+
+      expect(receivedRequests[3].method).toBe('PUT');
+      expect(receivedRequests[3].url).toMatch(/\/a-signed-url$/);
+
+      expect(receivedRequests[4].method).toBe('PUT');
+      expect(receivedRequests[4].url).toMatch(/\/a-signed-url$/);
+
+      expect(receivedRequests.length).toBe(5);
+    }, 20000);
+  });
 });