Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resume failed with aws-s3-multipart and shouldUseMultipart = true option #5230

Open
2 tasks done
hiromi2424 opened this issue Jun 5, 2024 · 1 comment
Open
2 tasks done
Labels

Comments

@hiromi2424
Copy link
Contributor

hiromi2424 commented Jun 5, 2024

Initial checklist

  • I understand this is a bug report and questions should be posted in the Community Forum
  • I searched issues and couldn’t find anything (or linked relevant results below)

Link to runnable example

No response

Steps to reproduce

  1. Use below versions:
{
    "@uppy/aws-s3-multipart": "3.11.1",
    "@uppy/core": "^3.11.3",
    "@uppy/golden-retriever": "^3.2.0"
}
  1. Setup Uppy with aws-s3-multipart and golden-retriever

Note: Some or all issues may be reproduced without setting shouldUseMultiPart always be true

import AwsS3Multipart from '@uppy/aws-s3-multipart';
import Uppy from '@uppy/core';
import GoldenRetriever from '@uppy/golden-retriever';

   new Uppy()
      .use(AwsS3Multipart, {
        // ..., 
        shouldUseMultipart() {
          return true;
        },
      })
      .use(GoldenRetriever, {
        serviceWorker: true, // needless
      });

Also: Setup companion server.

  1. Upload multiple files and crash browser
  • It will be easier to reproduce if you have about 10 files of 1MB or less
  • Reload browser can be used for crash (in this issue context)
  • Crash soon after starting upload(immediately, 1 second, 2 second, ...)
  • Having a heavy file, that is a part of upload files, may make continue uploading. Because finishing upload maybe will delete resumable metadata.
  • Restored from IndexedDB and ServiceWorker may "culm down" some of issues.
    • Delete database of IndexedDB, turn off ServiceWorker running and reload browser can reproduce "Browser crashed"
  1. Resume upload
uppy.emit('restore-confirmed');

or

Press resume button of dashboard(I don't use but it is perhaps same behavior)

Expected behavior

Completed upload files may not be affected

Actual behavior

  1. Files having completion flag will restart upload(!)

This is because aws-s3-multipart does not consider the completion flag.

Very fast solution is with patching file:
@uppy+aws-s3-multipart+3.11.1.patch

diff --git a/node_modules/@uppy/aws-s3-multipart/lib/index.js b/node_modules/@uppy/aws-s3-multipart/lib/index.js
index 027ee52..a12cd4f 100644
--- a/node_modules/@uppy/aws-s3-multipart/lib/index.js
+++ b/node_modules/@uppy/aws-s3-multipart/lib/index.js
@@ -177,6 +177,9 @@ export default class AwsS3Multipart extends BasePlugin {
             })();
             return uploadPromise;
           }
+          if (file.progress.uploadComplete) {
+            return Promise.resolve(`File ${file.id} was already uploaded`);
+          }
           return _classPrivateFieldLooseBase(this, _uploadLocalFile)[_uploadLocalFile](file);
         });
         const upload = await Promise.all(promises);
diff --git a/node_modules/@uppy/aws-s3-multipart/src/index.ts b/node_modules/@uppy/aws-s3-multipart/src/index.ts
index 2595d1b..d583114 100644
--- a/node_modules/@uppy/aws-s3-multipart/src/index.ts
+++ b/node_modules/@uppy/aws-s3-multipart/src/index.ts
@@ -958,6 +958,9 @@ export default class AwsS3Multipart<
 
         return uploadPromise
       }
+      if (file.progress.uploadComplete) {
+        return Promise.resolve(`File ${file.id} was already uploaded`)
+      }
 
       return this.#uploadLocalFile(file)
     })
  1. Completion flag is false

There is case that uploading to s3 have been finished(S3 complete endpoint was called) but flag of uploaded was not updated as true.

This case is caused by (maybe)two reason:

2-1. Golden retriever does not save last metadata before crash.
Golden retriever have to save metadata when file state was changed.
But with looking up the code:

node_modules/@uppy/golden-retriever/src/index.ts:86-90

    this.saveFilesStateToLocalStorage = throttle(
      this.saveFilesStateToLocalStorage.bind(this),
      500,
      { leading: true, trailing: true },
    )

It saves metadata with throttle. Crash causes throttle not to flush last invocation.

Hint: Perhaps uppy.getPlugin('GoldenRetriever').saveFilesStateToLocalStorage.flush() call on beforeunload event or so will save last state when browser was reloaded. But "real browser crash" does not send event anyway.

2-2. S3 complete endpoint was called but browser does not receive response

This makes completion of s3 multipart was done but metadata will not updated as finished upload.

  1. Reloading browser aborts upload

I found a few case that reloading browser aborts upload.
It will call DELETE /s3/multipart/:upload_id and upload ID of this will not be available anymore.
But metadata continues to have such upload id and S3 key.
This cause resuming upload will fail with invalid upload id.

@hiromi2424 hiromi2424 added the Bug label Jun 5, 2024
@hiromi2424
Copy link
Contributor Author

hiromi2424 commented Jun 6, 2024

Here is (excerpted part of) my userland implementation to resolve issues of "2." and "3." in actual behavior section above.

https://gist.github.com/hiromi2424/e2b7c5baf4f6e8e5a1c71445a296d6bb

I had to create new endpoint to return actual recent upload statuses.
This seems so hacky but works great to me.

It will be great if uppy library can focus this and internally implement or fix like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant