From 1e7b54c6e93318a751c3358a79a785d6d2e864be Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 30 Oct 2026 08:20:32 +0100 Subject: [PATCH 1/2] fix(file_storage): cancelled uploads stay cancelled; release finished File objects (#695) A cancel during the resumable-session lookup removed the row but the upload then started anyway; the queue now re-checks after each lookup. Completed rows released their File objects only on dismiss, so every uploaded file stayed referenced; they are dropped when the row clears. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 2 + docs/modules/file_storage.md | 5 + .../file_storage/pages/upload-queue.ts | 13 +- ...ile_storage_upload_queue_lifecycle.test.ts | 143 ++++++++++++++++++ 4 files changed, 161 insertions(+), 2 deletions(-) create mode 100644 modules/file_storage/tests-js/file_storage_upload_queue_lifecycle.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 8373db74c..e30e4ab6d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,8 @@ All notable changes to this project are documented in this file. The format is b ## [Unreleased] ### Fixed +- File storage: cancellation during upload-session lookup prevents transport from + starting; clearing completed upload rows releases their retained files (#695, #588). - **Behaviour change:** password resets revoke all browser sessions, access tokens and refresh tokens, sharing revocation with password changes and sign-out-everywhere (#686). - Defer single admin create/invite events and invitation-related emails until the diff --git a/docs/modules/file_storage.md b/docs/modules/file_storage.md index c47c19945..8e84b4cdb 100644 --- a/docs/modules/file_storage.md +++ b/docs/modules/file_storage.md @@ -142,6 +142,11 @@ opt-in setting, is not implemented. ### Direct, resumable uploads +Cancelling an upload while its session lookup is pending prevents it from starting +transport or offering a resume prompt. Completed queue rows and their retained +browser files are cleared after the file-list reload finishes; failed rows keep +their files available for retry (#695, #588). + Files above `direct_upload_threshold_bytes` (default 64 MiB) upload in parts through a *session*, so a dropped connection or a reload loses at most one part. The Files page does this automatically; API clients drive it themselves. All routes need `file_storage.upload` and act only on the caller's own sessions (anyone else's id is a 404). | Route | Purpose | diff --git a/modules/file_storage/file_storage/pages/upload-queue.ts b/modules/file_storage/file_storage/pages/upload-queue.ts index ec3dbc92c..dd4e8551b 100644 --- a/modules/file_storage/file_storage/pages/upload-queue.ts +++ b/modules/file_storage/file_storage/pages/upload-queue.ts @@ -41,7 +41,7 @@ function nextId(): string { * progress events in any current browser, and a large file uploading behind a * spinner with no feedback is the exact complaint this replaces. * - * The queue keeps the `File` object for every job it has seen, so a failed row + * The queue keeps the `File` object while its row remains, so a failed row * can be retried from the row itself — the alternative is asking the person to * find the file on disk again to answer a transient 502. */ @@ -91,6 +91,7 @@ export function useUploadQueue({ directThresholdBytes }: UploadQueueOptions = {} const maybePause = useCallback( async (jobId: string, file: File): Promise => { const session = await findSession(file); + if (!files.current.has(jobId)) return null; if (!session || session.parts.length === 0) return session; const sent = session.parts.reduce((sum, part) => sum + part.size, 0); waiting.current.set(jobId, session); @@ -144,6 +145,7 @@ export function useUploadQueue({ directThresholdBytes }: UploadQueueOptions = {} async (jobId: string, file: File, resumeAutomatically = false): Promise => { if (directThresholdBytes && file.size > directThresholdBytes) { const found = resumeAutomatically ? await findSession(file) : await maybePause(jobId, file); + if (!files.current.has(jobId)) return 'canceled'; if (found === 'paused') return 'paused'; return uploadInParts(jobId, file, found); } @@ -160,7 +162,14 @@ export function useUploadQueue({ directThresholdBytes }: UploadQueueOptions = {} // second batch dropped while this reload is in flight must not have its // progress rows swept away by the first batch's cleanup. const settleBatch = useCallback((uploaded: number) => { - const clearFinished = () => setJobs((current) => current.filter((j) => j.status !== 'done')); + const clearFinished = () => + setJobs((current) => + current.filter((job) => { + if (job.status !== 'done') return true; + files.current.delete(job.id); + return false; + }), + ); if (uploaded > 0) { router.reload({ only: RELOAD_PROPS, onFinish: clearFinished }); } else { diff --git a/modules/file_storage/tests-js/file_storage_upload_queue_lifecycle.test.ts b/modules/file_storage/tests-js/file_storage_upload_queue_lifecycle.test.ts new file mode 100644 index 000000000..e2ed05b4c --- /dev/null +++ b/modules/file_storage/tests-js/file_storage_upload_queue_lifecycle.test.ts @@ -0,0 +1,143 @@ +import { act, renderHook } from '@testing-library/react'; +import { beforeEach, expect, test, vi } from 'vitest'; + +const { maps, findSession, runResumable, reload } = vi.hoisted(() => ({ + maps: [] as Map[], + findSession: vi.fn(), + runResumable: vi.fn(), + reload: vi.fn(), +})); + +vi.mock('react', async (importOriginal) => { + const react = await importOriginal(); + return { + ...react, + useRef: (initial: T) => { + const ref = react.useRef(initial); + if (initial instanceof Map && !maps.includes(ref.current as Map)) { + maps.push(ref.current as Map); + } + return ref; + }, + }; +}); +vi.mock('@inertiajs/react', () => ({ router: { reload } })); +vi.mock('../file_storage/pages/resumable-upload', () => ({ + findSession, + runResumable, + discardSession: vi.fn(), +})); + +import type { RemoteSession } from '../file_storage/pages/resumable-upload'; +import { useUploadQueue } from '../file_storage/pages/upload-queue'; + +function deferred() { + let resolve!: (value: T) => void; + const promise = new Promise((done) => { + resolve = done; + }); + return { promise, resolve }; +} + +const file = () => new File(['payload'], 'example.txt'); + +beforeEach(() => { + maps.length = 0; + findSession.mockReset().mockResolvedValue(null); + runResumable.mockReset().mockResolvedValue(undefined); + reload.mockReset(); +}); + +const sessionDefaults = { + total_size: 7, + part_size: 2, + total_parts: 4, + status: 'active' as const, +}; +const sessions: (RemoteSession | null)[] = [ + null, + { ...sessionDefaults, id: 'empty', parts: [] }, + { ...sessionDefaults, id: 'partial', parts: [{ no: 1, etag: 'etag', size: 2 }] }, +]; + +test.each(sessions)('cancel during session lookup starts no transport (%j)', async (session) => { + const lookup = deferred(); + findSession.mockReturnValue(lookup.promise); + const { result } = renderHook(() => useUploadQueue({ directThresholdBytes: 1 })); + let batch!: ReturnType; + act(() => { + batch = result.current.start([file()]); + }); + expect(findSession).toHaveBeenCalledOnce(); + act(() => result.current.cancel(result.current.jobs[0].id)); + await act(async () => { + lookup.resolve(session); + expect(await batch).toEqual({ uploaded: 0, failed: [] }); + }); + expect(runResumable).not.toHaveBeenCalled(); + expect(result.current.jobs).toEqual([]); + expect(maps.every((map) => map.size === 0)).toBe(true); +}); + +test('cancel during retry session lookup starts no transport', async () => { + runResumable.mockRejectedValueOnce(new Error('connection lost')); + const { result } = renderHook(() => useUploadQueue({ directThresholdBytes: 1 })); + await act(async () => { + await result.current.start([file()]); + }); + const lookup = deferred(); + findSession.mockReturnValue(lookup.promise); + let retry!: Promise; + act(() => { + retry = result.current.retry(result.current.jobs[0].id); + }); + act(() => result.current.cancel(result.current.jobs[0].id)); + await act(async () => { + lookup.resolve(null); + expect(await retry).toBe(false); + }); + expect(runResumable).toHaveBeenCalledOnce(); + expect(result.current.jobs).toEqual([]); +}); + +test('successful batch releases files when the reload clears completed rows', async () => { + const { result } = renderHook(() => useUploadQueue({ directThresholdBytes: 1 })); + await act(async () => { + expect(await result.current.start([file(), file()])).toEqual({ uploaded: 2, failed: [] }); + }); + const files = maps[0]; + expect(files.size).toBe(2); + expect(result.current.jobs.every((job) => job.status === 'done')).toBe(true); + act(() => reload.mock.calls[0][0].onFinish()); + expect(result.current.jobs).toEqual([]); + expect(files.size).toBe(0); +}); + +test('batch cleanup keeps failed and concurrently uploading files for retry', async () => { + runResumable.mockRejectedValueOnce(new Error('connection lost')); + const { result } = renderHook(() => useUploadQueue({ directThresholdBytes: 1 })); + await act(async () => { + await result.current.start([file(), file()]); + }); + const pending = deferred(); + runResumable.mockReturnValue(pending.promise); + let batch!: ReturnType; + await act(async () => { + batch = result.current.start([file()]); + }); + act(() => reload.mock.calls[0][0].onFinish()); + expect(result.current.jobs.map((job) => job.status)).toEqual(['error', 'uploading']); + expect([...maps[0].keys()]).toEqual(result.current.jobs.map((job) => job.id)); + await act(async () => { + pending.resolve(); + await batch; + }); + act(() => reload.mock.calls[1][0].onFinish()); + expect(maps[0].size).toBe(1); + await act(async () => { + runResumable.mockResolvedValue(undefined); + expect(await result.current.retry(result.current.jobs[0].id)).toBe(true); + }); + act(() => reload.mock.calls[2][0].onFinish()); + expect(maps[0].size).toBe(0); +}); From 4827a5b89c097db343c62f5ae3915916f17d66ab Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 30 Oct 2026 09:01:39 +0100 Subject: [PATCH 2/2] fix(file_storage): a cancel during start-over's session discard stays cancelled (#695) Review follow-up: startOver awaited discardSession without re-checking for a cancel before starting the new upload. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../file_storage/pages/upload-queue.ts | 1 + ...ile_storage_upload_queue_lifecycle.test.ts | 29 +++++++++++++++++-- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/modules/file_storage/file_storage/pages/upload-queue.ts b/modules/file_storage/file_storage/pages/upload-queue.ts index dd4e8551b..c1b45bd07 100644 --- a/modules/file_storage/file_storage/pages/upload-queue.ts +++ b/modules/file_storage/file_storage/pages/upload-queue.ts @@ -259,6 +259,7 @@ export function useUploadQueue({ directThresholdBytes }: UploadQueueOptions = {} patch(id, { percent: 0, status: 'uploading' }); return finishResumed(id, async (file) => { if (session) await discardSession(file, session.id); + if (!files.current.has(id)) return 'canceled'; return uploadInParts(id, file, null); }); }, diff --git a/modules/file_storage/tests-js/file_storage_upload_queue_lifecycle.test.ts b/modules/file_storage/tests-js/file_storage_upload_queue_lifecycle.test.ts index e2ed05b4c..5bf75e06a 100644 --- a/modules/file_storage/tests-js/file_storage_upload_queue_lifecycle.test.ts +++ b/modules/file_storage/tests-js/file_storage_upload_queue_lifecycle.test.ts @@ -1,11 +1,12 @@ import { act, renderHook } from '@testing-library/react'; import { beforeEach, expect, test, vi } from 'vitest'; -const { maps, findSession, runResumable, reload } = vi.hoisted(() => ({ +const { maps, findSession, runResumable, reload, discardSession } = vi.hoisted(() => ({ maps: [] as Map[], findSession: vi.fn(), runResumable: vi.fn(), reload: vi.fn(), + discardSession: vi.fn(), })); vi.mock('react', async (importOriginal) => { @@ -25,7 +26,7 @@ vi.mock('@inertiajs/react', () => ({ router: { reload } })); vi.mock('../file_storage/pages/resumable-upload', () => ({ findSession, runResumable, - discardSession: vi.fn(), + discardSession, })); import type { RemoteSession } from '../file_storage/pages/resumable-upload'; @@ -46,6 +47,7 @@ beforeEach(() => { findSession.mockReset().mockResolvedValue(null); runResumable.mockReset().mockResolvedValue(undefined); reload.mockReset(); + discardSession.mockReset().mockResolvedValue(undefined); }); const sessionDefaults = { @@ -141,3 +143,26 @@ test('batch cleanup keeps failed and concurrently uploading files for retry', as act(() => reload.mock.calls[2][0].onFinish()); expect(maps[0].size).toBe(0); }); + +test('cancel while start-over discards the old session starts no transport', async () => { + const partial = sessions[2]; + runResumable.mockRejectedValueOnce(new Error('connection lost')); + findSession.mockResolvedValue(partial); + const { result } = renderHook(() => useUploadQueue({ directThresholdBytes: 1 })); + await act(async () => { + await result.current.start([file()]); + }); + expect(result.current.jobs[0].status).toBe('resumable'); + const discard = deferred(); + discardSession.mockReturnValue(discard.promise); + let restarted!: Promise; + act(() => { + restarted = result.current.startOver(result.current.jobs[0].id); + }); + act(() => result.current.cancel(result.current.jobs[0].id)); + await act(async () => { + discard.resolve(); + expect(await restarted).toBe(false); + }); + expect(runResumable).not.toHaveBeenCalled(); +});