@uppy/aws-s3-multipart: retry signature request (#4691)

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Co-authored-by: Mikael Finstad <finstaden@gmail.com>

e2e/cypress/integration/dashboard-aws-multipart.spec.ts

@@ -72,12 +72,20 @@ describe('Dashboard with @uppy/aws-s3-multipart', () => {
     cy.get('.uppy-StatusBar-statusPrimary').should('contain', 'Upload failed')
 
     cy.intercept('POST', '/s3/multipart', { statusCode: 200, times: 1, body: JSON.stringify({ key:'mocked-key-attempt3', uploadId:'mocked-uploadId-attempt3' }) }).as('createMultipartUpload-attempt3')
-    cy.intercept('GET', '/s3/multipart/mocked-uploadId-attempt3/1?key=mocked-key-attempt3', {
-      statusCode: 200,
-      headers: {
-        ETag: 'ETag-attempt3',
-      },
-      body: JSON.stringify({ url:'/put-success-attempt3', expires:8 }),
+    let intercepted = 0
+    cy.intercept('GET', '/s3/multipart/mocked-uploadId-attempt3/1?key=mocked-key-attempt3', (req) => {
+      if (intercepted++ < 2) {
+        // Ensure that Uppy can recover from at least 2 network errors at this stage.
+        req.destroy()
+        return
+      }
+      req.reply({
+        statusCode: 200,
+        headers: {
+          ETag: 'ETag-attempt3',
+        },
+        body: JSON.stringify({ url:'/put-success-attempt3', expires:8 }),
+      })
     }).as('signPart-attempt3')
     cy.intercept('PUT', '/put-success-attempt3', {
       statusCode: 200,
@@ -92,7 +100,7 @@ describe('Dashboard with @uppy/aws-s3-multipart', () => {
       }),
     }).as('completeMultipartUpload-attempt3')
     cy.get('.uppy-StatusBar-actions > .uppy-c-btn').click()
-    cy.wait(['@createMultipartUpload-attempt3', '@signPart-attempt3', '@put-attempt3', '@completeMultipartUpload-attempt3'])
+    cy.wait(['@createMultipartUpload-attempt3', ...Array(3).fill('@signPart-attempt3'), '@put-attempt3', '@completeMultipartUpload-attempt3'])
     cy.get('.uppy-StatusBar-statusPrimary').should('contain', 'Complete')
   })
 

packages/@uppy/aws-s3-multipart/src/index.js

@@ -76,7 +76,7 @@ class HTTPCommunicationQueue {
 
   #requests
 
-  #retryDelayIterator
+  #retryDelays
 
   #sendCompletionRequest
 
@@ -112,7 +112,7 @@ class HTTPCommunicationQueue {
       this.#sendCompletionRequest = requests.wrapPromiseFunction(options.completeMultipartUpload, { priority:1 })
     }
     if ('retryDelays' in options) {
-      this.#retryDelayIterator = options.retryDelays?.values()
+      this.#retryDelays = options.retryDelays ?? []
     }
     if ('uploadPartBytes' in options) {
       this.#uploadPartBytes = requests.wrapPromiseFunction(options.uploadPartBytes, { priority:Infinity })
@@ -122,7 +122,7 @@ class HTTPCommunicationQueue {
     }
   }
 
-  async #shouldRetry (err) {
+  async #shouldRetry (err, retryDelayIterator) {
     const requests = this.#requests
     const status = err?.source?.status
 
@@ -137,7 +137,7 @@ class HTTPCommunicationQueue {
         // more than one request in parallel, to give slower connection a chance
         // to catch up with the expiry set in Companion.
         if (requests.limit === 1 || this.#previousRetryDelay == null) {
-          const next = this.#retryDelayIterator?.next()
+          const next = retryDelayIterator.next()
           if (next == null || next.done) {
             return false
           }
@@ -156,7 +156,7 @@ class HTTPCommunicationQueue {
     } else if (status === 429) {
       // HTTP 429 Too Many Requests => to avoid the whole download to fail, pause all requests.
       if (!requests.isPaused) {
-        const next = this.#retryDelayIterator?.next()
+        const next = retryDelayIterator.next()
         if (next == null || next.done) {
           return false
         }
@@ -175,7 +175,7 @@ class HTTPCommunicationQueue {
       }
     } else {
       // Other error code means the request can be retried later.
-      const next = this.#retryDelayIterator?.next()
+      const next = retryDelayIterator.next()
       if (next == null || next.done) {
         return false
       }
@@ -348,14 +348,36 @@ class HTTPCommunicationQueue {
   async uploadChunk (file, partNumber, chunk, signal) {
     throwIfAborted(signal)
     const { uploadId, key } = await this.getUploadId(file, signal)
-    throwIfAborted(signal)
+
+    const signatureRetryIterator = this.#retryDelays.values()
+    const chunkRetryIterator = this.#retryDelays.values()
+    const shouldRetrySignature = () => {
+      const next = signatureRetryIterator.next()
+      if (next == null || next.done) {
+        return null
+      }
+      return next.value
+    }
+
     for (;;) {
+      throwIfAborted(signal)
       const chunkData = chunk.getData()
       const { onProgress, onComplete } = chunk
+      let signature
 
-      const signature = await this.#fetchSignature(this.#getFile(file), {
-        uploadId, key, partNumber, body: chunkData, signal,
-      }).abortOn(signal)
+      try {
+        signature = await this.#fetchSignature(this.#getFile(file), {
+          uploadId, key, partNumber, body: chunkData, signal,
+        }).abortOn(signal)
+      } catch (err) {
+        const timeout = shouldRetrySignature()
+        if (timeout == null || signal.aborted) {
+          throw err
+        }
+        await new Promise(resolve => setTimeout(resolve, timeout))
+        // eslint-disable-next-line no-continue
+        continue
+      }
 
       throwIfAborted(signal)
       try {
@@ -366,7 +388,7 @@ class HTTPCommunicationQueue {
           }).abortOn(signal),
         }
       } catch (err) {
-        if (!await this.#shouldRetry(err)) throw err
+        if (!await this.#shouldRetry(err, chunkRetryIterator)) throw err
       }
     }
   }

packages/@uppy/aws-s3-multipart/src/index.test.js

@@ -390,7 +390,7 @@ describe('AwsS3Multipart', () => {
 
       await expect(core.upload()).rejects.toEqual({ source: { status: 500 } })
 
-      expect(awsS3Multipart.opts.uploadPartBytes.mock.calls.length).toEqual(2)
+      expect(awsS3Multipart.opts.uploadPartBytes.mock.calls.length).toEqual(3)
       expect(mock.mock.calls.length).toEqual(1)
     })
   })