From d8960a8f6a49abfee86c97638c8f878b351a1be8 Mon Sep 17 00:00:00 2001 From: Anish Shrestha Date: Sat, 22 Aug 2026 21:10:57 +1000 Subject: [PATCH] fix(wardrobe): split bulk upload chunks that exceed the server's configured limit max_bulk_upload_count is admin-configurable but never exposed to the client, so a chunk sized for the default 20 got the whole request rejected on any instance where it's lowered. Split and retry within the server-reported limit instead. --- frontend/lib/hooks/use-items.ts | 36 ++++++++++- frontend/lib/upload-manager.ts | 46 +++++++++++++- frontend/tests/bulk-upload.test.ts | 89 ++++++++++++++++++++++++++- frontend/tests/upload-manager.test.ts | 44 +++++++++++++ 4 files changed, 209 insertions(+), 6 deletions(-) diff --git a/frontend/lib/hooks/use-items.ts b/frontend/lib/hooks/use-items.ts index 6d6e4c07..d52d33c9 100644 --- a/frontend/lib/hooks/use-items.ts +++ b/frontend/lib/hooks/use-items.ts @@ -835,6 +835,40 @@ function uploadBulkItemsChunk( }); } +const BULK_LIMIT_ERROR = /^Maximum (\d+) images per bulk upload$/; + +// Same server-side cap upload-manager.ts's durable path works around: a +// chunk sized for the default 20 gets the whole request rejected (not just +// the excess files) on an instance where an admin lowered +// MAX_BULK_UPLOAD_COUNT. Split and retry within the limit the server just +// reported instead of failing every file in the chunk. +export async function uploadFilesWithinServerLimit( + files: File[], + skipAi: boolean, + token: string | null | undefined, + onProgress: (percent: number) => void +): Promise { + try { + return await uploadBulkItemsChunk(files, skipAi, token, onProgress); + } catch (error) { + const match = + error instanceof ApiError && error.status === 400 + ? error.message.match(BULK_LIMIT_ERROR) + : null; + const limit = match ? Number(match[1]) : null; + if (limit && limit > 0 && limit < files.length) { + const responses: BulkUploadResponse[] = []; + for (let i = 0; i < files.length; i += limit) { + responses.push( + await uploadFilesWithinServerLimit(files.slice(i, i + limit), skipAi, token, onProgress) + ); + } + return mergeBulkUploadResponses(responses); + } + throw error; + } +} + export function mergeBulkUploadResponses(responses: BulkUploadResponse[]): BulkUploadResponse { return responses.reduce( (acc, response) => ({ @@ -916,7 +950,7 @@ export function useBulkCreateItems() { for (let i = 0; i < chunks.length; i++) { const chunkFiles = chunks[i]; try { - const response = await uploadBulkItemsChunk(chunkFiles, skipAi, token, (chunkPercent) => { + const response = await uploadFilesWithinServerLimit(chunkFiles, skipAi, token, (chunkPercent) => { const overall = ((i + chunkPercent / 100) / chunks.length) * 100; setUploadProgress(Math.round(overall)); }); diff --git a/frontend/lib/upload-manager.ts b/frontend/lib/upload-manager.ts index 32cd486f..4c3ff9fe 100644 --- a/frontend/lib/upload-manager.ts +++ b/frontend/lib/upload-manager.ts @@ -12,11 +12,26 @@ import { dismiss as dismissRecord, type QueuedUpload, } from '@/lib/upload-queue'; -import type { BulkUploadResponse } from '@/lib/hooks/use-items'; +import { mergeBulkUploadResponses, type BulkUploadResponse } from '@/lib/hooks/use-items'; const BULK_UPLOAD_CHUNK_SIZE = 20; const MAX_ATTEMPTS = 5; const RETRY_BACKOFF_BASE_MS = 5000; +const BULK_LIMIT_ERROR = /^Maximum (\d+) images per bulk upload$/; + +class BulkLimitExceededError extends Error { + constructor(public readonly limit: number) { + super(`Server bulk upload limit is ${limit}`); + } +} + +// The server's configured max_bulk_upload_count (admin-tunable, self-hosted) +// isn't exposed to the client, so a chunk sized for the default of 20 gets +// the WHOLE request rejected - not just the excess files - on an instance +// where an admin lowered it below 20. Cached at module scope so once the +// real limit is learned, later drain passes stop re-discovering it via a +// failed request on every pass. +let effectiveChunkSize = BULK_UPLOAD_CHUNK_SIZE; export interface TerminalRecord { id: string; @@ -71,6 +86,7 @@ async function emit(): Promise { export function init(client: QueryClient): void { queryClient = client; + effectiveChunkSize = BULK_UPLOAD_CHUNK_SIZE; } export function subscribe(listener: Listener): () => void { @@ -97,11 +113,35 @@ async function uploadChunk(chunk: QueuedUpload[]): Promise { }); if (!response.ok) { + if (response.status === 400) { + const body = await response.json().catch(() => null); + const match = + typeof body?.detail === 'string' ? body.detail.match(BULK_LIMIT_ERROR) : null; + if (match) { + throw new BulkLimitExceededError(Number(match[1])); + } + } throw new Error(`Bulk upload request failed with status ${response.status}`); } return response.json(); } +async function uploadChunkWithinServerLimit(chunk: QueuedUpload[]): Promise { + try { + return await uploadChunk(chunk); + } catch (error) { + if (error instanceof BulkLimitExceededError && error.limit > 0 && error.limit < chunk.length) { + effectiveChunkSize = Math.min(effectiveChunkSize, error.limit); + const responses: BulkUploadResponse[] = []; + for (let i = 0; i < chunk.length; i += error.limit) { + responses.push(await uploadChunkWithinServerLimit(chunk.slice(i, i + error.limit))); + } + return mergeBulkUploadResponses(responses); + } + throw error; + } +} + function errorMessage(error: unknown): string { return error instanceof Error ? error.message : 'Upload failed'; } @@ -129,7 +169,7 @@ async function drainOnce(): Promise { // A chunk request carries one skip_ai value - keep chunks homogeneous // rather than threading a per-file flag through the endpoint. const skipAi = actionable[0].skipAi; - const chunk = actionable.filter((r) => r.skipAi === skipAi).slice(0, BULK_UPLOAD_CHUNK_SIZE); + const chunk = actionable.filter((r) => r.skipAi === skipAi).slice(0, effectiveChunkSize); for (const record of chunk) { await markUploading(record.id); @@ -137,7 +177,7 @@ async function drainOnce(): Promise { await emit(); try { - const response = await uploadChunk(chunk); + const response = await uploadChunkWithinServerLimit(chunk); await Promise.all( response.results.map(async (result, idx) => { const record = chunk[idx]; diff --git a/frontend/tests/bulk-upload.test.ts b/frontend/tests/bulk-upload.test.ts index a455f1f4..2353911a 100644 --- a/frontend/tests/bulk-upload.test.ts +++ b/frontend/tests/bulk-upload.test.ts @@ -1,7 +1,29 @@ -import { describe, it, expect } from 'vitest' -import { mergeBulkUploadResponses } from '@/lib/hooks/use-items' +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' +import { mergeBulkUploadResponses, uploadFilesWithinServerLimit } from '@/lib/hooks/use-items' import type { BulkUploadResponse } from '@/lib/hooks/use-items' +// Minimal fake XHR standing in for uploadBulkItemsChunk's XMLHttpRequest use - +// queues one canned response per call, replayed on send() via a `load` event. +class FakeXhr { + static queue: Array<{ status: number; body: unknown }> = [] + upload = { addEventListener: () => {} } + status = 0 + responseText = '' + private listeners: Record void>> = {} + open() {} + setRequestHeader() {} + addEventListener(event: string, handler: () => void) { + ;(this.listeners[event] ??= []).push(handler) + } + send() { + const next = FakeXhr.queue.shift() + if (!next) throw new Error('FakeXhr.queue exhausted') + this.status = next.status + this.responseText = JSON.stringify(next.body) + this.listeners['load']?.forEach((fn) => fn()) + } +} + describe('mergeBulkUploadResponses', () => { it('should sum counts and concatenate results across chunks', () => { const chunkA: BulkUploadResponse = { @@ -51,3 +73,66 @@ describe('mergeBulkUploadResponses', () => { expect(merged.results).toHaveLength(2) }) }) + +describe('uploadFilesWithinServerLimit', () => { + let originalXhr: typeof XMLHttpRequest + + beforeEach(() => { + originalXhr = globalThis.XMLHttpRequest + FakeXhr.queue = [] + // @ts-expect-error - test double stands in for the real constructor + globalThis.XMLHttpRequest = FakeXhr + }) + + afterEach(() => { + globalThis.XMLHttpRequest = originalXhr + }) + + it('splits a chunk on the server-reported limit instead of failing every file', async () => { + const files = [ + new File(['a'], 'a.jpg'), + new File(['b'], 'b.jpg'), + new File(['c'], 'c.jpg'), + ] + + FakeXhr.queue.push( + { status: 400, body: { detail: 'Maximum 2 images per bulk upload' } }, + { + status: 201, + body: { + total: 2, + successful: 2, + failed: 0, + results: [ + { filename: 'a.jpg', success: true }, + { filename: 'b.jpg', success: true }, + ], + }, + }, + { + status: 201, + body: { + total: 1, + successful: 1, + failed: 0, + results: [{ filename: 'c.jpg', success: true }], + }, + } + ) + + const result = await uploadFilesWithinServerLimit(files, false, 'token', vi.fn()) + + expect(result.total).toBe(3) + expect(result.successful).toBe(3) + expect(result.results.map((r) => r.filename)).toEqual(['a.jpg', 'b.jpg', 'c.jpg']) + }) + + it('propagates a non-limit error without splitting', async () => { + const files = [new File(['a'], 'a.jpg')] + FakeXhr.queue.push({ status: 500, body: { detail: 'Internal error' } }) + + await expect(uploadFilesWithinServerLimit(files, false, 'token', vi.fn())).rejects.toThrow( + 'Internal error' + ) + }) +}) diff --git a/frontend/tests/upload-manager.test.ts b/frontend/tests/upload-manager.test.ts index 5fca5c71..65c8afa9 100644 --- a/frontend/tests/upload-manager.test.ts +++ b/frontend/tests/upload-manager.test.ts @@ -114,6 +114,50 @@ describe('startDrain', () => { } }) + it('splits and retries within the server limit instead of failing the whole chunk', async () => { + await enqueueFiles( + [makeFile('a.jpg'), makeFile('b.jpg'), makeFile('c.jpg'), makeFile('d.jpg')], + false + ) + + // setup.ts assigns global.fetch = vi.fn() once per file; the per-test + // vi.spyOn in this file's beforeEach layers on top of that same mock, so + // its call history isn't fully reset between tests without an explicit + // mockReset() here. + vi.mocked(fetch).mockReset() + let n = 0 + vi.mocked(fetch).mockImplementation(async () => { + n += 1 + if (n === 1) return jsonResponse({ detail: 'Maximum 2 images per bulk upload' }, false, 400) + if (n === 2) + return jsonResponse({ + total: 2, + successful: 2, + failed: 0, + results: [ + { filename: 'a.jpg', success: true }, + { filename: 'b.jpg', success: true }, + ], + }) + if (n === 3) + return jsonResponse({ + total: 2, + successful: 2, + failed: 0, + results: [ + { filename: 'c.jpg', success: true }, + { filename: 'd.jpg', success: true }, + ], + }) + throw new Error(`unexpected extra fetch call #${n}`) + }) + + await manager.startDrain() + + expect(await getPendingUploads()).toHaveLength(0) + expect(fetch).toHaveBeenCalledTimes(3) + }) + it('is idempotent - a concurrent call while draining does not start a second loop', async () => { // isDraining is set synchronously before startDrain's first await, so // the guard doesn't depend on fetch timing - no need to hand-pause the