Tech-debt batch 26: upload queue honours cancels during session lookup; releases finished files (#695)

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

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 |

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<RemoteSession | 'paused' | null> => {
       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<Outcome> => {
       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 {
@@ -250,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);
       });
     },

modules/file_storage/tests-js/file_storage_upload_queue_lifecycle.test.ts

@@ -0,0 +1,168 @@
+import { act, renderHook } from '@testing-library/react';
+import { beforeEach, expect, test, vi } from 'vitest';
+
+const { maps, findSession, runResumable, reload, discardSession } = vi.hoisted(() => ({
+  maps: [] as Map<string, File>[],
+  findSession: vi.fn(),
+  runResumable: vi.fn(),
+  reload: vi.fn(),
+  discardSession: vi.fn(),
+}));
+
+vi.mock('react', async (importOriginal) => {
+  const react = await importOriginal<typeof import('react')>();
+  return {
+    ...react,
+    useRef: <T>(initial: T) => {
+      const ref = react.useRef(initial);
+      if (initial instanceof Map && !maps.includes(ref.current as Map<string, File>)) {
+        maps.push(ref.current as Map<string, File>);
+      }
+      return ref;
+    },
+  };
+});
+vi.mock('@inertiajs/react', () => ({ router: { reload } }));
+vi.mock('../file_storage/pages/resumable-upload', () => ({
+  findSession,
+  runResumable,
+  discardSession,
+}));
+
+import type { RemoteSession } from '../file_storage/pages/resumable-upload';
+import { useUploadQueue } from '../file_storage/pages/upload-queue';
+
+function deferred<T>() {
+  let resolve!: (value: T) => void;
+  const promise = new Promise<T>((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();
+  discardSession.mockReset().mockResolvedValue(undefined);
+});
+
+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<RemoteSession | null>();
+  findSession.mockReturnValue(lookup.promise);
+  const { result } = renderHook(() => useUploadQueue({ directThresholdBytes: 1 }));
+  let batch!: ReturnType<typeof result.current.start>;
+  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<RemoteSession | null>();
+  findSession.mockReturnValue(lookup.promise);
+  let retry!: Promise<boolean>;
+  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<void>();
+  runResumable.mockReturnValue(pending.promise);
+  let batch!: ReturnType<typeof result.current.start>;
+  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);
+});
+
+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<void>();
+  discardSession.mockReturnValue(discard.promise);
+  let restarted!: Promise<boolean>;
+  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();
+});