From 033b2f265c37d5c112a5d83af2e5bfc0ee1c0481 Mon Sep 17 00:00:00 2001 From: Alex Freska Date: Fri, 1 Mar 2024 08:46:39 -0500 Subject: [PATCH] fix: renterd multipart etag error --- .changeset/tricky-tables-know.md | 5 +++ apps/renterd/lib/multipartUpload.spec.ts | 48 ++++++++++++++++++++++-- apps/renterd/lib/multipartUpload.ts | 37 ++++++++++++++---- 3 files changed, 79 insertions(+), 11 deletions(-) create mode 100644 .changeset/tricky-tables-know.md diff --git a/.changeset/tricky-tables-know.md b/.changeset/tricky-tables-know.md new file mode 100644 index 000000000..db2e946fc --- /dev/null +++ b/.changeset/tricky-tables-know.md @@ -0,0 +1,5 @@ +--- +'renterd': minor +--- + +Uploads will now error and abort if responses are missing etags. diff --git a/apps/renterd/lib/multipartUpload.spec.ts b/apps/renterd/lib/multipartUpload.spec.ts index 99daee8b7..3e0a9a544 100644 --- a/apps/renterd/lib/multipartUpload.spec.ts +++ b/apps/renterd/lib/multipartUpload.spec.ts @@ -1,5 +1,9 @@ import { Response, delay } from '@siafoundation/react-core' -import { MultipartParams, MultipartUpload } from './multipartUpload' +import { + ErrorNoETag, + MultipartParams, + MultipartUpload, +} from './multipartUpload' describe('MultipartUpload', () => { it('should report progress and complete with serial parts', async () => { @@ -190,6 +194,39 @@ describe('MultipartUpload', () => { expect(elapsedTime).toBeGreaterThanOrEqual(3500) }, 10_000) + it('should abort the entire upload and error if a part is missing an etag', async () => { + // note that the upload mock is configured to report progress 2 times per part + const partSize = 2 + const params = getMockedParams({ + file: new File(['012456'], 'test.txt', { type: 'text/plain' }), + partSize, + apiWorkerUploadPart: buildMockApiWorkerUploadPart({ + partSize, + failures: [{ failCallIndex: 1, failPartIndex: 1, type: 'missingEtag' }], + }), + maxConcurrentParts: 1, + }) + const multipartUpload = new MultipartUpload(params) + await multipartUpload.create() + await multipartUpload.start() + expect(params.onProgress.mock.calls.length).toBe(4) + expect( + params.onProgress.mock.calls.map(([params]) => [ + params.sent, + params.total, + params.percentage, + ]) + ).toEqual([ + [1, 6, 17], // call 0 + [2, 6, 33], + [3, 6, 50], // call 1 + [4, 6, 67], // fail + ]) + expect(params.apiBusUploadComplete.post).not.toHaveBeenCalled() + expect(params.onComplete).not.toHaveBeenCalled() + expect(params.onError).toHaveBeenCalledWith(expect.any(ErrorNoETag)) + }) + it('should handle an upload create error', async () => { const params = getMockedParams() const multipartUpload = new MultipartUpload({ @@ -255,6 +292,7 @@ function getMockedParams(params?: Partial>) { type Failure = { failCallIndex: number failPartIndex: number + type?: 'partFailed' | 'missingEtag' } function buildMockApiWorkerUploadPart({ @@ -284,14 +322,14 @@ function buildMockApiWorkerUploadPart({ reject(new Error('Abort')) return } - const shouldFail = failures.find( + const failure = failures.find( (failure) => callIndex === failure.failCallIndex && partIndex === failure.failPartIndex ) loaded += progressPartSize onUploadProgress({ type: 'progress', loaded, total }) - if (shouldFail) { + if ((failure && !failure.type) || failure?.type === 'partFailed') { clearInterval(intervalId) reject(new Error('Upload failed')) } else { @@ -299,7 +337,9 @@ function buildMockApiWorkerUploadPart({ clearInterval(intervalId) resolve({ status: 200, - headers: { etag: eTag }, + headers: { + etag: failure?.type === 'missingEtag' ? undefined : eTag, + }, }) } partIndex++ diff --git a/apps/renterd/lib/multipartUpload.ts b/apps/renterd/lib/multipartUpload.ts index 051d097fb..9c7879455 100644 --- a/apps/renterd/lib/multipartUpload.ts +++ b/apps/renterd/lib/multipartUpload.ts @@ -139,6 +139,7 @@ export class MultipartUpload { } catch (e) { triggerErrorToast(e.message) } + this.#resolve() } public setOnProgress( @@ -195,7 +196,15 @@ export class MultipartUpload { // On successful upload, reset the delay this.#resetDelay() } catch (error) { - if (error.name === 'canceled') { + // if the upload was canceled, don't retry + if (error instanceof ErrorCanceledRequest) { + return + } + // if the upload failed due to a missing ETag, abort the entire upload + // and report the error + if (error instanceof ErrorNoETag) { + await this.abort() + this.#onError(error) return } this.#pendingPartNumbers.push(partNumber) @@ -282,15 +291,17 @@ export class MultipartUpload { // errors such as aborted/canceled request if (response.error) { + if (response.error === 'canceled') { + throw new ErrorCanceledRequest() + } throw new Error(response.error) } const eTag = response.headers['etag'] if (!eTag) { - throw new Error( - 'No ETag in response, add ETag to Access-Control-Expose-Headers list' - ) + throw new ErrorNoETag() } + const uploadedPart = { partNumber: partNumber, // removing the " enclosing characters from the raw ETag @@ -298,10 +309,22 @@ export class MultipartUpload { } this.#uploadedParts.push(uploadedPart) + } finally { delete this.#activeConnections[partNumber] - } catch (e) { - delete this.#activeConnections[partNumber] - throw e } } } + +export class ErrorCanceledRequest extends Error { + constructor() { + super('canceled') + this.name = 'CanceledError' + } +} + +export class ErrorNoETag extends Error { + constructor() { + super('No ETag in response, add ETag to Access-Control-Expose-Headers list') + this.name = 'NoETagError' + } +}