Skip to content

Commit

Permalink
fix: renterd multipart etag error
Browse files Browse the repository at this point in the history
  • Loading branch information
alexfreska committed Mar 1, 2024
1 parent 72237e6 commit 033b2f2
Show file tree
Hide file tree
Showing 3 changed files with 79 additions and 11 deletions.
5 changes: 5 additions & 0 deletions .changeset/tricky-tables-know.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'renterd': minor
---

Uploads will now error and abort if responses are missing etags.
48 changes: 44 additions & 4 deletions apps/renterd/lib/multipartUpload.spec.ts
Original file line number Diff line number Diff line change
@@ -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 () => {
Expand Down Expand Up @@ -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({
Expand Down Expand Up @@ -255,6 +292,7 @@ function getMockedParams(params?: Partial<Record<keyof MultipartParams, any>>) {
type Failure = {
failCallIndex: number
failPartIndex: number
type?: 'partFailed' | 'missingEtag'
}

function buildMockApiWorkerUploadPart({
Expand Down Expand Up @@ -284,22 +322,24 @@ 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 {
if (loaded >= partSize) {
clearInterval(intervalId)
resolve({
status: 200,
headers: { etag: eTag },
headers: {
etag: failure?.type === 'missingEtag' ? undefined : eTag,
},
})
}
partIndex++
Expand Down
37 changes: 30 additions & 7 deletions apps/renterd/lib/multipartUpload.ts
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ export class MultipartUpload {
} catch (e) {
triggerErrorToast(e.message)
}
this.#resolve()
}

public setOnProgress(
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -282,26 +291,40 @@ 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
eTag: eTag.replace(/"/g, ''),
}

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'
}
}

0 comments on commit 033b2f2

Please sign in to comment.