From bad751fc9c2ded3f2ffe9d585ea13de676e081b6 Mon Sep 17 00:00:00 2001 From: Stefan Ernst <24554551+stefan-ernst@users.noreply.github.com> Date: Mon, 2 Nov 2026 15:30:04 +0100 Subject: [PATCH] fix(frontend): bound attachment upload concurrency (WI-1922) Attachment uploads ran either one file per request in series (a long waterfall for a large drop) or all at once (a burst that competes with foreground reads for the server's per-user concurrency budget), and each of the five paths carried its own loop and error policy. Add a shared runWithConcurrency pool (limit 3) and route every attachment path through it: the item attachments composable, the personal task detail, CreateModal's staged description images, the portal request form, and the Milkdown editor. A failed file is recorded, not thrown, so the rest of the batch still uploads; each caller surfaces failures it can (the personal task detail now toasts them instead of only logging). Post-upload reload behavior is unchanged. Uploads are POSTs, so they cannot use the GET-only readScheduler; the bounded pool is the upload-side budget. --- .../composables/useItemAttachments.svelte.js | 15 +-- frontend/src/lib/dialogs/CreateModal.svelte | 20 +++- .../src/lib/dialogs/RequestFormModal.svelte | 17 ++- .../src/lib/editors/MilkdownEditor.svelte | 109 +++++++++--------- .../personal/PersonalTaskDetail.svelte | 16 ++- frontend/src/lib/utils/concurrency.js | 45 ++++++++ 6 files changed, 139 insertions(+), 83 deletions(-) create mode 100644 frontend/src/lib/utils/concurrency.js diff --git a/frontend/src/lib/composables/useItemAttachments.svelte.js b/frontend/src/lib/composables/useItemAttachments.svelte.js index a38c15d3c..a6e13b00f 100644 --- a/frontend/src/lib/composables/useItemAttachments.svelte.js +++ b/frontend/src/lib/composables/useItemAttachments.svelte.js @@ -1,6 +1,7 @@ import { onDestroy } from 'svelte'; import { attachments as attachmentsApi } from '../api/misc.js'; import { attachmentStatus } from '../stores/attachmentStatus.svelte.js'; +import { runWithConcurrency } from '../utils/concurrency.js'; /** * Composable for managing item attachments @@ -140,13 +141,13 @@ export function useItemAttachments(getItemId, showError = console.error) { const itemId = getItemId(); if (!itemId) return; - for (const file of files) { - try { - await attachmentsApi.uploadToItem(itemId, file); - } catch (err) { - console.error('Upload error:', err); - showError('Failed to upload attachment', err.message || String(err)); - } + const results = await runWithConcurrency(files, (file) => + attachmentsApi.uploadToItem(itemId, file) + ); + for (const result of results) { + if (result.ok) continue; + console.error('Upload error:', result.error); + showError('Failed to upload attachment', result.error?.message || String(result.error)); } // Reload attachments after all uploads complete diff --git a/frontend/src/lib/dialogs/CreateModal.svelte b/frontend/src/lib/dialogs/CreateModal.svelte index 95008af15..a98f469b9 100644 --- a/frontend/src/lib/dialogs/CreateModal.svelte +++ b/frontend/src/lib/dialogs/CreateModal.svelte @@ -27,6 +27,7 @@ import { getShortcut, matchesShortcut, getDisplayString } from '../utils/keyboardShortcuts.js'; import { errorToast } from '../stores/toasts.svelte.js'; import { showCreatedItemToast } from '../utils/createdItemToast.js'; + import { runWithConcurrency } from '../utils/concurrency.js'; // Import form components import WorkItemForm from '../forms/WorkItemForm.svelte'; @@ -247,21 +248,28 @@ } async function uploadPendingDescriptionImages(itemId, description) { - if (workItemFormStore.pendingDescriptionImages.length === 0) { + const pending = workItemFormStore.pendingDescriptionImages; + if (pending.length === 0) { return description; } let updatedDescription = description || ''; - for (const image of workItemFormStore.pendingDescriptionImages) { - if (!updatedDescription.includes(image.url)) continue; - + const toUpload = pending.filter((image) => updatedDescription.includes(image.url)); + const results = await runWithConcurrency(toUpload, async (image) => { const attachment = await attachments.uploadToItem(itemId, image.file); const attachmentId = attachment?.id; if (!attachmentId) { throw new Error('Image upload failed'); } - - const downloadUrl = attachments.getContentUrl(attachmentId); + return { image, attachment }; + }); + + for (const result of results) { + if (!result.ok) throw result.error; + const value = result.value; + if (!value) throw new Error('Image upload failed'); + const { image, attachment } = value; + const downloadUrl = attachments.getContentUrl(attachment.id); updatedDescription = updatedDescription.split(image.url).join(downloadUrl); } diff --git a/frontend/src/lib/dialogs/RequestFormModal.svelte b/frontend/src/lib/dialogs/RequestFormModal.svelte index 7ffebce79..26a03ec59 100644 --- a/frontend/src/lib/dialogs/RequestFormModal.svelte +++ b/frontend/src/lib/dialogs/RequestFormModal.svelte @@ -24,6 +24,7 @@ initializeFormValues, validateFormStep, } from '../features/forms/formModel.js'; + import { runWithConcurrency } from '../utils/concurrency.js'; let { isOpen = $bindable(false), @@ -348,16 +349,12 @@ // fail the submission — the customer can retry from the timeline. if (stagedFiles.length > 0) { uploadingFiles = true; - const failures = []; - for (const file of stagedFiles) { - try { - await portal.addRequestAttachment(portalSlug, result.item_id, file); - } catch (err) { - console.error('Failed to upload attachment after submit:', err); - failures.push(file.name); - } - } - uploadFailures = failures; + const results = await runWithConcurrency(stagedFiles, (file) => + portal.addRequestAttachment(portalSlug, result.item_id, file) + ); + uploadFailures = results + .filter((entry) => !entry.ok) + .map((entry) => entry.item?.name ?? ''); uploadingFiles = false; } diff --git a/frontend/src/lib/editors/MilkdownEditor.svelte b/frontend/src/lib/editors/MilkdownEditor.svelte index cd6460784..354d6c6c4 100644 --- a/frontend/src/lib/editors/MilkdownEditor.svelte +++ b/frontend/src/lib/editors/MilkdownEditor.svelte @@ -39,6 +39,7 @@ import { t } from '../stores/i18n.svelte.js'; import { attachmentStatus } from '../stores/attachmentStatus.svelte.js'; import { navigate } from '../router.js'; + import { runWithConcurrency } from '../utils/concurrency.js'; let { content = $bindable(''), placeholder = '', readonly = false, @@ -382,63 +383,63 @@ return { nodes: [], attachments: [] }; } - const results = await Promise.all( - images.map(async (image) => { - try { - if (deferImageUploads && !effectiveEntityId && !customUploadFn) { - const src = URL.createObjectURL(image); - onDeferredImageUpload?.({ file: image, url: src }); - const node = schema?.nodes?.image?.createAndFill({ - src, - alt: image.name, - }) || null; - - return { - node, - attachment: null, - src, - alt: image.name, - }; - } + const settled = await runWithConcurrency(images, async (image) => { + try { + if (deferImageUploads && !effectiveEntityId && !customUploadFn) { + const src = URL.createObjectURL(image); + onDeferredImageUpload?.({ file: image, url: src }); + const node = schema?.nodes?.image?.createAndFill({ + src, + alt: image.name, + }) || null; + + return { + node, + attachment: null, + src, + alt: image.name, + }; + } - let result; - if (!customUploadFn && effectiveEntityType === 'item' && effectiveEntityId) { - const attachment = await attachmentsApi.uploadToItem(effectiveEntityId, image); - result = { success: true, attachment }; - } else { - const formData = new FormData(); - formData.append('file', image); - if (effectiveEntityId && effectiveEntityType) { - formData.append('entity_id', effectiveEntityId.toString()); - formData.append('entity_type', effectiveEntityType); - } - result = customUploadFn - ? await customUploadFn(formData) - : await attachmentsApi.upload(formData); - } - if (result.success && result.attachment) { - const src = downloadUrlBase === '/api/attachments' - ? attachmentsApi.getContentUrl(result.attachment.id) - : `${downloadUrlBase}/${result.attachment.id}/download`; - const node = schema?.nodes?.image?.createAndFill({ - src, - alt: image.name, - }) || null; - - return { - node, - attachment: result.attachment, - src, - alt: image.name, - }; + let result; + if (!customUploadFn && effectiveEntityType === 'item' && effectiveEntityId) { + const attachment = await attachmentsApi.uploadToItem(effectiveEntityId, image); + result = { success: true, attachment }; + } else { + const formData = new FormData(); + formData.append('file', image); + if (effectiveEntityId && effectiveEntityType) { + formData.append('entity_id', effectiveEntityId.toString()); + formData.append('entity_type', effectiveEntityType); } - return null; - } catch (error) { - console.error('[MilkdownEditor] Image upload error:', error); - return null; + result = customUploadFn + ? await customUploadFn(formData) + : await attachmentsApi.upload(formData); } - }) - ); + if (result.success && result.attachment) { + const src = downloadUrlBase === '/api/attachments' + ? attachmentsApi.getContentUrl(result.attachment.id) + : `${downloadUrlBase}/${result.attachment.id}/download`; + const node = schema?.nodes?.image?.createAndFill({ + src, + alt: image.name, + }) || null; + + return { + node, + attachment: result.attachment, + src, + alt: image.name, + }; + } + return null; + } catch (error) { + console.error('[MilkdownEditor] Image upload error:', error); + return null; + } + }); + + const results = settled.map((entry) => (entry.ok ? entry.value : null)); const nodes = []; const attachments = []; diff --git a/frontend/src/lib/features/personal/PersonalTaskDetail.svelte b/frontend/src/lib/features/personal/PersonalTaskDetail.svelte index f4452ce1c..d132d16b6 100644 --- a/frontend/src/lib/features/personal/PersonalTaskDetail.svelte +++ b/frontend/src/lib/features/personal/PersonalTaskDetail.svelte @@ -20,6 +20,8 @@ import ItemDetailBreadcrumbs from '../items/ItemDetailBreadcrumbs.svelte'; import { useItemEventStream } from '../../composables/useItemEventStream.svelte.js'; import { t } from '../../stores/i18n.svelte.js'; + import { errorToast } from '../../stores/toasts.svelte.js'; + import { runWithConcurrency } from '../../utils/concurrency.js'; let { itemId, @@ -261,12 +263,14 @@ async function handleAttachmentUploadFiles(data) { const { files } = data; - for (const file of files) { - try { - await attachmentsApi.uploadToItem(item.id, file); - } catch (err) { - console.error('Failed to upload:', err); - } + const results = await runWithConcurrency(files, (file) => + attachmentsApi.uploadToItem(item.id, file) + ); + const failed = results.filter((result) => !result.ok); + if (failed.length > 0) { + console.error('Failed to upload:', failed[0].error); + const names = failed.map((result) => result.item?.name).filter(Boolean).join(', '); + errorToast(t('dialogs.alerts.failedToUpload', { error: names || t('errors.UNKNOWN') })); } await loadAttachments(); } diff --git a/frontend/src/lib/utils/concurrency.js b/frontend/src/lib/utils/concurrency.js new file mode 100644 index 000000000..b12fc9ad1 --- /dev/null +++ b/frontend/src/lib/utils/concurrency.js @@ -0,0 +1,45 @@ +// Bounded concurrency pool for uploads and other per-item request batches. +// +// Attachment uploads used to run either one file per request in series (a long +// waterfall for a large drop) or all at once (a burst that competes with +// foreground reads for the server's per-user concurrency budget). Every path +// now funnels through `runWithConcurrency` with one small limit. + +export const UPLOAD_CONCURRENCY = 3; + +/** + * Run `worker` over `items` with at most `concurrency` calls in flight. + * + * Settles every item: a rejected worker is recorded, never thrown, so one + * failing file cannot cancel the rest of the batch. Callers inspect each + * result and decide how to report failures. + * + * @template T, R + * @param {Iterable} items + * @param {(item: T, index: number) => Promise} worker + * @param {{ concurrency?: number }} [options] + * @returns {Promise>} + * settled results in input order + */ +export async function runWithConcurrency(items, worker, { concurrency = UPLOAD_CONCURRENCY } = {}) { + const list = Array.from(items); + /** @type {Array<{ item: T, index: number, ok: boolean, value: R | null, error: any }>} */ + const results = new Array(list.length); + const limit = Math.max(1, Math.min(Math.floor(concurrency) || 1, list.length || 1)); + let cursor = 0; + + async function runWorker() { + while (cursor < list.length) { + const index = cursor++; + const item = list[index]; + try { + results[index] = { item, index, ok: true, value: await worker(item, index), error: null }; + } catch (error) { + results[index] = { item, index, ok: false, value: null, error }; + } + } + } + + await Promise.all(Array.from({ length: limit }, runWorker)); + return results; +}