@tus/s3-store: finalize incomplete parts

[atticoos]

Fixes #501

When an upload completes with only a single incomplete part having been uploaded, uploadPart has never run to establish the first part.

As a result, parts are empty, and inalizeMultipartUpload fails with a 500.

Fix

Wasn't sure of the bets way to accomplish this, since the final step is an empty PATCH with just Upload-Length header, so the main processUpload routine that normally calls uploadPart and uploadIncompletePart will not run a final time -- so I performed the final uploadPart as part of the write procedure, ensuring the first part will always exist for finishMultipartUpload to run.

[mitjap]

@atticoos Thank you for your contribution. This PR resolves an issue when there are no existing parts, but does not handle a more general situation when there are some existing parts and incomplete part as well.

Probable reason why there are no parts when length is deferred is discussed in #503.

[atticoos]

Will need to take a closer look at this patch after the recent changes in 1.1.0

For an unknown reason, with those changes, calling uploadPart with the incompletePart result in the following error

  tus-node-server:stores:s3store [tus-test/test-0.7704705088837052] failed to finish upload NotImplemented: A header you provided implies functionality that is not implemented
    at throwDefaultError (<path>/node_modules/.pnpm/@[email protected]/node_modules/@smithy/smithy-client/dist-cjs/default-error-handler.js:8:22)
    at <path>/node_modules/.pnpm/@[email protected]/node_modules/@smithy/smithy-client/dist-cjs/default-error-handler.js:18:39
    at de_UploadPartCommandError (<path>//node_modules/.pnpm/@[email protected]/node_modules/@aws-sdk/client-s3/dist-cjs/protocols/Aws_restXml.js:5971:12)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async <path>/node_modules/.pnpm/@[email protected]/node_modules/@smithy/middleware-serde/dist-cjs/deserializerMiddleware.js:7:24
    at async <path>/node_modules/.pnpm/@[email protected]/node_modules/@aws-sdk/middleware-signing/dist-cjs/awsAuthMiddleware.js:14:20
    at async <path>/node_modules/.pnpm/@[email protected]/node_modules/@smithy/middleware-retry/dist-cjs/retryMiddleware.js:27:46
    at async <path>/node_modules/.pnpm/@[email protected]/node_modules/@aws-sdk/middleware-flexible-checksums/dist-cjs/flexibleChecksumsMiddleware.js:63:20
    at async <path>/node_modules/.pnpm/@[email protected]/node_modules/@aws-sdk/middleware-sdk-s3/dist-cjs/region-redirect-endpoint-middleware.js:14:24
    at async <path>/node_modules/.pnpm/@[email protected]/node_modules/@aws-sdk/middleware-sdk-s3/dist-cjs/region-redirect-middleware.js:9:20 {
  '$fault': 'client',
  '$metadata': {
    httpStatusCode: 501,
    requestId: '<omitted>',
    extendedRequestId: '<omitted>',
    cfId: undefined,
    attempts: 1,
    totalRetryDelay: 0
  },
  Code: 'NotImplemented',
  Header: 'Transfer-Encoding',
  RequestId: '<omitted>',
  HostId: '<omitted>'
}

[mitjap]

Not sure what is the problem with the error you get. Regarding the failed tests (https://github.com/tus/tus-node-server/pull/502/checks?check_run_id=18095601809) you need to fix test should store in between chunks under the minimum part size and prepend it to the next call. You added additional call to getIncompletePart, so you need to update this test at line 79. It need to be assert.equal(getIncompletePart.calledThrice, true).

I ran your tests and everything seems to work.

[mitjap]

Suggested change

	// If no parts exist yet, then the incomplete part needs to be completed
	// If there is any incomplete part it needs to be completed

[mitjap]

I prepared another test where we test if it works when there is a one complete part and one incomplete part.

  it('completes an incomplete part when deferred length becomes resolved', async function () {
    const store = this.datastore
    const firstPartSize = 8 * 1024 * 1024 // 8MB
    const secondPartSize = 2 * 1024 * 1024 // 2MB

    const uploadPart = sinon.spy(store, 'uploadPart')
    const getIncompletePart = sinon.spy(store, 'getIncompletePart')
    const uploadIncompletePart = sinon.spy(store, 'uploadIncompletePart')
    const finishMultipartUpload = sinon.spy(store, 'finishMultipartUpload')

    sinon.stub(store, 'calcOptimalPartSize').callsFake(() => firstPartSize)

    const upload = new Upload({
      id: 'deferred-incomplete-part-test-' + Uid.rand(),
      // Deferred length
      size: undefined,
      offset: 0,
    })

    await store.create(upload)

    // Upload a single chunk large enough to create a part
    {
      upload.offset = await store.write(
        Readable.from(Buffer.alloc(firstPartSize)),
        upload.id,
        upload.offset
      )
      assert.equal(uploadPart.called, true)
      assert.equal(uploadIncompletePart.called, false)
    }

    // Upload a single chunk small enough to create an incomplete part
    {
      uploadPart.resetHistory()
      uploadIncompletePart.resetHistory()

      upload.offset = await store.write(
        Readable.from(Buffer.alloc(secondPartSize)),
        upload.id,
        upload.offset
      )
      assert.equal(uploadIncompletePart.called, true)
      assert.equal(uploadPart.called, false)
    }

    // Simulate the completion PATCH of a deferred length multipart upload
    {
      getIncompletePart.resetHistory()
      uploadIncompletePart.resetHistory()

      // Resolve the deferred length
      await store.declareUploadLength(upload.id, firstPartSize + secondPartSize)
      // Notify the store to complete the multipart upload (empty payload)
      await store.write(Readable.from(Buffer.alloc(0)), upload.id, upload.offset)
      // Final write needs to check for incomplete part
      assert.equal(getIncompletePart.called, true)
      // The incomplete part will now be completed
      assert.equal(uploadPart.called, true)
      // The multipart upload will now be completed
      assert.equal(finishMultipartUpload.called, true)
    }
  })

[Murderlon]

@atticoos still interested in getting this over the finish line?

[atticoos]

I welcome contributions if anyone wants to take over

[Murderlon]

@fenos has this been superseded by #561?

[fenos]

Yes