fix(upload): do not finish chunked uploads after the first chunk

__tests__/uploader/upload.e2e.spec.ts

@@ -289,6 +289,87 @@ describe('Uploader (current API)', () => {
 		await expect(client.getFileContents('/files/admin/test-stats/large.txt', { format: 'text' })).resolves.toBe(content)
 	})
 
+	it('should report the progress of a chunked upload without exceeding the file size', async () => {
+		const client = getClient()
+		await client.deleteFile('/files/admin/test-chunked-progress').catch(() => {})
+		await client.createDirectory('/files/admin/test-chunked-progress')
+
+		const folder = new Folder({
+			owner: 'admin',
+			root: '/files/admin',
+			source: `${defaultRemoteURL}/files/admin/test-chunked-progress`,
+		})
+		const uploader = new Uploader(false, folder)
+
+		// Pause so all chunks are queued before the listeners are attached,
+		// this way no progress event can be missed.
+		await uploader.pause()
+
+		// 21 MiB exceeds the default 10 MiB chunk size, so this is uploaded in multiple chunks
+		const content = 'x'.repeat(21 * 1024 * 1024)
+		const upload = await uploader.upload('chunked.txt', new File([content], 'chunked.txt', { type: 'text/plain' }))
+		expect(upload.isChunked).toBe(true)
+
+		const observedBytes: number[] = []
+		upload.addEventListener('progress', () => {
+			observedBytes.push(upload.uploadedBytes)
+		})
+		const observedProgress: number[] = []
+		uploader.addEventListener('uploadProgress', () => {
+			observedProgress.push(uploader.statistics.progress)
+		})
+
+		const finishedPromise = new Promise<void>((resolve) => uploader.addEventListener('finished', () => resolve()))
+		uploader.start()
+		await finishedPromise
+
+		expect(upload.status).toBe(UploadStatus.FINISHED)
+		// Every chunk reported progress …
+		expect(observedBytes.length).toBeGreaterThan(1)
+		// … but no chunk ever accounted more than the total size of the file …
+		expect(Math.max(...observedBytes)).toBeLessThanOrEqual(upload.totalBytes)
+		expect(Math.max(...observedProgress)).toBeLessThanOrEqual(100)
+		// … and in the end all bytes are accounted for exactly once.
+		expect(upload.uploadedBytes).toBe(upload.totalBytes)
+		expect(Math.max(...observedProgress)).toBe(100)
+
+		await expect(client.getFileContents('/files/admin/test-chunked-progress/chunked.txt', { format: 'text' })).resolves.toBe(content)
+	})
+
+	it('should not report a chunked upload as finished before it is assembled', async () => {
+		const client = getClient()
+		await client.deleteFile('/files/admin/test-chunked-status').catch(() => {})
+		await client.createDirectory('/files/admin/test-chunked-status')
+
+		const folder = new Folder({
+			owner: 'admin',
+			root: '/files/admin',
+			source: `${defaultRemoteURL}/files/admin/test-chunked-status`,
+		})
+		const uploader = new Uploader(false, folder)
+		await uploader.pause()
+
+		const content = 'x'.repeat(21 * 1024 * 1024)
+		const upload = await uploader.upload('chunked.txt', new File([content], 'chunked.txt', { type: 'text/plain' }))
+		expect(upload.isChunked).toBe(true)
+
+		const observedStatuses: number[] = []
+		upload.addEventListener('progress', () => {
+			observedStatuses.push(upload.status)
+		})
+
+		const finishedPromise = new Promise<void>((resolve) => uploader.addEventListener('finished', () => resolve()))
+		uploader.start()
+		await finishedPromise
+
+		// While any chunk is still transferred the upload is uploading …
+		expect(observedStatuses).toContain(UploadStatus.UPLOADING)
+		// … a single finished chunk must not mark the whole file as finished …
+		expect(observedStatuses).not.toContain(UploadStatus.FINISHED)
+		// … only once the server assembled all chunks the upload is finished.
+		expect(upload.status).toBe(UploadStatus.FINISHED)
+	})
+
 	it('should cancel a queued upload', async () => {
 		const client = getClient()
 		await client.deleteFile('/files/admin/test-cancel').catch(() => {})

lib/upload/uploader/UploadFile.spec.ts

@@ -6,7 +6,7 @@
 
 import axios from '@nextcloud/axios'
 import { CanceledError } from 'axios'
-import { describe, expect, it, vi } from 'vitest'
+import { beforeEach, describe, expect, it, vi } from 'vitest'
 import { UploadStatus } from './Upload.ts'
 import { UploadFile } from './UploadFile.ts'
 
@@ -366,3 +366,128 @@ describe('upload status and events', () => {
 		expect(onFinish).toHaveBeenCalledOnce()
 	})
 })
+
+describe('chunked upload progress and status', () => {
+	// 4096 bytes with a maximum chunk size of 1024 bytes -> four chunks
+	const fileSize = 4096
+
+	/**
+	 * Create a queue stub that runs every task immediately and keeps the resulting promise.
+	 */
+	function immediateQueue() {
+		return { add: vi.fn((fn: () => Promise<void>) => fn()) }
+	}
+
+	/**
+	 * Create a chunked upload of `fileSize` bytes.
+	 */
+	function createUpload() {
+		return new UploadFile('/destination', new File(['x'.repeat(fileSize)], 'bigfile'), { noChunking: false })
+	}
+
+	beforeEach(() => {
+		vi.restoreAllMocks()
+		uploadDataMock.mockReset()
+		initChunkWorkspaceMock.mockReset()
+
+		isPublicShareMock.mockReturnValue(false)
+		getMaxChunksSizeMock.mockReturnValue(1024)
+		initChunkWorkspaceMock.mockResolvedValue('/tmp/temporary')
+	})
+
+	it('is not finished while other chunks are still uploading', async () => {
+		// the last of the four chunks never settles, all others succeed
+		const { promise: pendingChunk } = Promise.withResolvers<void>()
+		const delayed = () => new Promise((resolve) => setTimeout(resolve, 10))
+		uploadDataMock
+			.mockImplementationOnce(delayed)
+			.mockImplementationOnce(delayed)
+			.mockImplementationOnce(delayed)
+			.mockReturnValueOnce(pendingChunk)
+		vi.spyOn(axios, 'request').mockResolvedValue({} as never)
+
+		const uploadFile = createUpload()
+		const queue = immediateQueue()
+		await uploadFile.start(queue as never)
+
+		await vi.waitFor(() => expect(uploadDataMock).toHaveBeenCalledTimes(4))
+		// wait for the three succeeding chunks to settle
+		await new Promise((resolve) => setTimeout(resolve, 30))
+
+		expect(uploadFile.status).toBe(UploadStatus.UPLOADING)
+		expect(uploadFile.uploadedBytes).toBeLessThan(fileSize)
+	})
+
+	it('never reports more uploaded bytes than the file size', async () => {
+		uploadDataMock.mockImplementation((_url: string, chunk: Blob, options: any) => {
+			// the whole chunk was sent
+			options.onUploadProgress?.({ bytes: chunk.size })
+			return Promise.resolve()
+		})
+		vi.spyOn(axios, 'request').mockResolvedValue({} as never)
+
+		const uploadFile = createUpload()
+		const reported: number[] = []
+		uploadFile.addEventListener('progress', () => {
+			reported.push(uploadFile.uploadedBytes)
+		})
+
+		const queue = immediateQueue()
+		await uploadFile.start(queue as never)
+		await Promise.all(queue.add.mock.results.map((r) => r.value))
+
+		expect(Math.max(...reported)).toBeLessThanOrEqual(fileSize)
+		expect(uploadFile.uploadedBytes).toBe(fileSize)
+		expect(uploadFile.status).toBe(UploadStatus.FINISHED)
+	})
+
+	it('only discards the progress of the retried chunk', async () => {
+		uploadDataMock.mockImplementation((_url: string, chunk: Blob, options: any) => {
+			options.onUploadProgress?.({ bytes: chunk.size })
+			return Promise.resolve()
+		})
+		// the first chunk is uploaded without any retry …
+		uploadDataMock.mockImplementationOnce((_url: string, chunk: Blob, options: any) => {
+			options.onUploadProgress?.({ bytes: chunk.size })
+			return Promise.resolve()
+		})
+		// … while the second one has to be retried after it was already fully sent
+		uploadDataMock.mockImplementationOnce((_url: string, chunk: Blob, options: any) => {
+			options.onUploadProgress?.({ bytes: chunk.size })
+			options.onUploadRetry?.()
+			options.onUploadProgress?.({ bytes: chunk.size })
+			return Promise.resolve()
+		})
+		vi.spyOn(axios, 'request').mockResolvedValue({} as never)
+
+		const uploadFile = createUpload()
+		const reported: number[] = []
+		uploadFile.addEventListener('progress', () => {
+			reported.push(uploadFile.uploadedBytes)
+		})
+
+		const queue = immediateQueue()
+		await uploadFile.start(queue as never)
+		await Promise.all(queue.add.mock.results.map((r) => r.value))
+
+		// a retry of one chunk must not drop the progress of the other chunks
+		expect(Math.max(...reported)).toBeLessThanOrEqual(fileSize)
+		expect(uploadFile.uploadedBytes).toBe(fileSize)
+	})
+
+	it('does not overwrite a failed status with a later successful chunk', async () => {
+		// the first chunk fails, all other chunks succeed but only after the failure was handled
+		uploadDataMock.mockImplementation(() => new Promise((resolve) => setTimeout(resolve, 20)))
+		uploadDataMock.mockRejectedValueOnce(new Error('chunk failed'))
+		vi.spyOn(axios, 'request').mockResolvedValue({} as never)
+
+		const uploadFile = createUpload()
+		const queue = immediateQueue()
+		await uploadFile.start(queue as never)
+		await Promise.allSettled(queue.add.mock.results.map((r) => r.value))
+		// wait for the remaining chunks to settle
+		await new Promise((resolve) => setTimeout(resolve, 50))
+
+		expect(uploadFile.status).toBe(UploadStatus.FAILED)
+	})
+})

lib/upload/uploader/UploadFile.ts

@@ -95,6 +95,9 @@ export class UploadFile extends Upload implements IUpload {
 		const chunk = await getChunk(this.#file!, 0, this.#file!.size)
 		try {
 			await this.#uploadChunk(chunk, encodeUrl(this.source))
+			// Update progress - now we set the uploaded size to 100% of the file size
+			this.uploadedBytes = this.totalBytes
+			this.status = UploadStatus.FINISHED
 		} catch (error) {
 			if (!(error instanceof UploadCancelledError)) {
 				throw error
@@ -169,6 +172,12 @@ export class UploadFile extends Upload implements IUpload {
 	 * @param url - The target URL
 	 */
 	async #uploadChunk(chunk: Blob, url: string) {
+		// Bytes of this chunk that are already accounted for in `this.uploadedBytes`.
+		// This is tracked per chunk as other chunks might be uploaded in parallel.
+		let accountedBytes = 0
+		// Bytes of this chunk reported as sent by the current try
+		let sentBytes = 0
+
 		try {
 			await uploadData(
 				url,
@@ -177,13 +186,20 @@ export class UploadFile extends Upload implements IUpload {
 					signal: this.signal,
 					retries: this.options.retries,
 					onUploadProgress: ({ bytes }) => {
+						sentBytes += bytes
 						// As this is only the sent bytes not the processed ones we only count 90%.
-						// When the upload is finished (server acknowledged the upload) the remaining 10% will be correctly set.
-						this.uploadedBytes += bytes * 0.9
+						// When the chunk is uploaded (server acknowledged the upload) the remaining 10% will be correctly set.
+						// Rounding keeps `uploadedBytes` an integer so the accounting stays exact.
+						const accounted = Math.min(Math.round(sentBytes * 0.9), chunk.size)
+						this.uploadedBytes += accounted - accountedBytes
+						accountedBytes = accounted
 						this.dispatchTypedEvent('progress', new CustomEvent('progress', { detail: this }))
 					},
 					onUploadRetry: () => {
-						this.uploadedBytes = 0
+						// Only discard the progress of this chunk, any other chunk is not affected by this retry
+						this.uploadedBytes -= accountedBytes
+						accountedBytes = 0
+						sentBytes = 0
 					},
 					headers: {
 						...this.options.headers,
@@ -193,9 +209,9 @@ export class UploadFile extends Upload implements IUpload {
 				},
 			)
 
-			// Update progress - now we set the uploaded size to 100% of the file size
-			this.uploadedBytes = this.totalBytes
-			this.status = UploadStatus.FINISHED
+			// The server acknowledged this chunk, so account the remaining 10% of it
+			this.uploadedBytes += chunk.size - accountedBytes
+			this.dispatchTypedEvent('progress', new CustomEvent('progress', { detail: this }))
 		} catch (error) {
 			if (isRequestAborted(error)) {
 				this.status = UploadStatus.CANCELLED