Skip to content

Commit e4175c3

Browse files
committed
Fix parallel upload resumability on 5xx status code
1 parent 083c003 commit e4175c3

File tree

2 files changed

+219
-2
lines changed

2 files changed

+219
-2
lines changed

lib/upload.ts

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,8 @@ export class BaseUpload {
119119
// upload options or HEAD response)
120120
private _uploadLengthDeferred: boolean
121121

122+
123+
122124
constructor(file: UploadInput, options: UploadOptions) {
123125
// Warn about removed options from previous versions
124126
if ('resume' in options) {
@@ -286,7 +288,7 @@ export class BaseUpload {
286288
*
287289
* @api private
288290
*/
289-
private async _startParallelUpload(): Promise<void> {
291+
private async _startParallelUpload(): Promise<void> {
290292
const totalSize = this._size
291293
let totalProgress = 0
292294
this._parallelUploads = []
@@ -376,6 +378,9 @@ export class BaseUpload {
376378

377379
// @ts-expect-error `value` is unknown and not an UploadInput
378380
const upload = new BaseUpload(value, options)
381+
382+
383+
379384
upload.start()
380385

381386
// Store the upload in an array, so we can later abort them if necessary.
@@ -715,6 +720,11 @@ export class BaseUpload {
715720
throw new DetailedError('tus: upload is currently locked; retry later', undefined, req, res)
716721
}
717722

723+
// For 5xx server errors, throw error to trigger retry instead of creating new upload
724+
if (inStatusCategory(status, 500)) {
725+
throw new DetailedError('tus: server error during resume, retrying', undefined, req, res)
726+
}
727+
718728
if (inStatusCategory(status, 400)) {
719729
// Remove stored fingerprint and corresponding endpoint,
720730
// on client errors since the file can not be found
@@ -731,7 +741,7 @@ export class BaseUpload {
731741
)
732742
}
733743

734-
// Try to create a new upload
744+
// Try to create a new upload (only for 4xx client errors and 3xx redirects)
735745
this.url = null
736746
await this._createUpload()
737747
}
@@ -762,6 +772,8 @@ export class BaseUpload {
762772
await this.options.onUploadUrlAvailable()
763773
}
764774

775+
776+
765777
await this._saveUploadInUrlStorage()
766778

767779
// Upload has already been completed and we do not need to send additional

test/spec/test-parallel-uploads.js

Lines changed: 205 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -578,5 +578,210 @@ describe('tus', () => {
578578
expect(options.onProgress).toHaveBeenCalledWith(5, 11)
579579
expect(options.onProgress).toHaveBeenCalledWith(11, 11)
580580
})
581+
582+
it('should preserve upload URL in partial uploads during retry', async () => {
583+
const testStack = new TestHttpStack()
584+
const file = getBlob('hello')
585+
586+
const options = {
587+
httpStack: testStack,
588+
parallelUploads: 1, // Use single parallel to focus on one upload
589+
retryDelays: [10],
590+
endpoint: 'https://tus.io/uploads',
591+
onSuccess: waitableFunction(),
592+
headers: { 'Upload-Concat': 'partial' }, // Force partial upload behavior
593+
storeFingerprintForResuming: false, // This is key - partial uploads don't store fingerprints
594+
}
595+
596+
const upload = new Upload(file, options)
597+
upload.start()
598+
599+
// Create partial upload
600+
let req = await testStack.nextRequest()
601+
expect(req.url).toBe('https://tus.io/uploads')
602+
expect(req.method).toBe('POST')
603+
expect(req.requestHeaders['Upload-Concat']).toBe('partial')
604+
605+
req.respondWith({
606+
status: 201,
607+
responseHeaders: {
608+
Location: 'https://tus.io/uploads/upload1',
609+
},
610+
})
611+
612+
// PATCH request fails
613+
req = await testStack.nextRequest()
614+
expect(req.url).toBe('https://tus.io/uploads/upload1')
615+
expect(req.method).toBe('PATCH')
616+
617+
req.respondWith({
618+
status: 500,
619+
})
620+
621+
// The key test: what happens on retry?
622+
req = await testStack.nextRequest()
623+
624+
// With the fix: should be HEAD to existing URL (resume)
625+
expect(req.url).toBe('https://tus.io/uploads/upload1')
626+
expect(req.method).toBe('HEAD')
627+
628+
// Without the fix: would be POST to create new upload
629+
// (because storeFingerprintForResuming: false and uploadUrl: null)
630+
631+
req.respondWith({
632+
status: 204,
633+
responseHeaders: {
634+
'Upload-Length': '5',
635+
'Upload-Offset': '0',
636+
},
637+
})
638+
639+
// Resume PATCH
640+
req = await testStack.nextRequest()
641+
expect(req.url).toBe('https://tus.io/uploads/upload1')
642+
expect(req.method).toBe('PATCH')
643+
644+
req.respondWith({
645+
status: 204,
646+
responseHeaders: {
647+
'Upload-Offset': '5',
648+
},
649+
})
650+
651+
await options.onSuccess.toBeCalled()
652+
})
653+
654+
it('should resume partial uploads on retry instead of creating fresh uploads', async () => {
655+
const testStack = new TestHttpStack()
656+
const file = getBlob('hello world')
657+
658+
// Track all requests to detect if fresh uploads are created
659+
const allRequests = []
660+
const originalCreateRequest = testStack.createRequest.bind(testStack)
661+
testStack.createRequest = function(method, url) {
662+
allRequests.push({ method, url })
663+
return originalCreateRequest(method, url)
664+
}
665+
666+
const options = {
667+
httpStack: testStack,
668+
parallelUploads: 2,
669+
retryDelays: [10],
670+
endpoint: 'https://tus.io/uploads',
671+
onSuccess: waitableFunction(),
672+
}
673+
674+
const upload = new Upload(file, options)
675+
upload.start()
676+
677+
// First partial upload creation
678+
let req = await testStack.nextRequest()
679+
expect(req.url).toBe('https://tus.io/uploads')
680+
expect(req.method).toBe('POST')
681+
expect(req.requestHeaders['Upload-Concat']).toBe('partial')
682+
expect(req.requestHeaders['Upload-Length']).toBe('5')
683+
684+
req.respondWith({
685+
status: 201,
686+
responseHeaders: {
687+
Location: 'https://tus.io/uploads/upload1',
688+
},
689+
})
690+
691+
// Second partial upload creation
692+
req = await testStack.nextRequest()
693+
expect(req.url).toBe('https://tus.io/uploads')
694+
expect(req.method).toBe('POST')
695+
expect(req.requestHeaders['Upload-Concat']).toBe('partial')
696+
expect(req.requestHeaders['Upload-Length']).toBe('6')
697+
698+
req.respondWith({
699+
status: 201,
700+
responseHeaders: {
701+
Location: 'https://tus.io/uploads/upload2',
702+
},
703+
})
704+
705+
// First PATCH request succeeds
706+
req = await testStack.nextRequest()
707+
expect(req.url).toBe('https://tus.io/uploads/upload1')
708+
expect(req.method).toBe('PATCH')
709+
expect(req.requestHeaders['Upload-Offset']).toBe('0')
710+
711+
req.respondWith({
712+
status: 204,
713+
responseHeaders: {
714+
'Upload-Offset': '5',
715+
},
716+
})
717+
718+
// Second PATCH request fails (network error)
719+
req = await testStack.nextRequest()
720+
expect(req.url).toBe('https://tus.io/uploads/upload2')
721+
expect(req.method).toBe('PATCH')
722+
expect(req.requestHeaders['Upload-Offset']).toBe('0')
723+
724+
req.respondWith({
725+
status: 500,
726+
})
727+
728+
// CRITICAL TEST: After retry delay, we should NOT see another POST to /uploads
729+
// The next request should be HEAD to the existing upload2 URL
730+
req = await testStack.nextRequest()
731+
732+
// Verify we're not creating a fresh upload (this is the key test)
733+
expect(req.method).not.toBe('POST')
734+
expect(req.url).toBe('https://tus.io/uploads/upload2')
735+
expect(req.method).toBe('HEAD')
736+
737+
req.respondWith({
738+
status: 204,
739+
responseHeaders: {
740+
'Upload-Length': '6',
741+
'Upload-Offset': '0',
742+
},
743+
})
744+
745+
// Resume with PATCH from current offset
746+
req = await testStack.nextRequest()
747+
expect(req.url).toBe('https://tus.io/uploads/upload2')
748+
expect(req.method).toBe('PATCH')
749+
expect(req.requestHeaders['Upload-Offset']).toBe('0')
750+
751+
req.respondWith({
752+
status: 204,
753+
responseHeaders: {
754+
'Upload-Offset': '6',
755+
},
756+
})
757+
758+
// Final concatenation
759+
req = await testStack.nextRequest()
760+
expect(req.url).toBe('https://tus.io/uploads')
761+
expect(req.method).toBe('POST')
762+
expect(req.requestHeaders['Upload-Concat']).toBe(
763+
'final;https://tus.io/uploads/upload1 https://tus.io/uploads/upload2',
764+
)
765+
766+
req.respondWith({
767+
status: 201,
768+
responseHeaders: {
769+
Location: 'https://tus.io/uploads/upload3',
770+
},
771+
})
772+
773+
await options.onSuccess.toBeCalled()
774+
775+
// Final verification: count how many POST requests to /uploads we made
776+
const postToUploads = allRequests.filter(r =>
777+
r.method === 'POST' && r.url === 'https://tus.io/uploads'
778+
)
779+
780+
// Should only be 3 POSTs: 2 partial + 1 final concatenation
781+
// If the bug exists, we'd see 4 POSTs (an extra one from the retry)
782+
expect(postToUploads.length).toBe(3)
783+
})
784+
785+
581786
})
582787
})

0 commit comments

Comments
 (0)