diff --git a/__tests__/uploader/upload.e2e.spec.ts b/__tests__/uploader/upload.e2e.spec.ts index 7c56dd2ce..983859778 100644 --- a/__tests__/uploader/upload.e2e.spec.ts +++ b/__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((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((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(() => {}) diff --git a/lib/upload/uploader/UploadFile.spec.ts b/lib/upload/uploader/UploadFile.spec.ts index 4061a32eb..f0fdfd473 100644 --- a/lib/upload/uploader/UploadFile.spec.ts +++ b/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) => 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() + 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) + }) +}) diff --git a/lib/upload/uploader/UploadFile.ts b/lib/upload/uploader/UploadFile.ts index 938d52263..ce08f664a 100644 --- a/lib/upload/uploader/UploadFile.ts +++ b/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