diff --git a/chat-sdk/README.md b/chat-sdk/README.md index daa4f94fc0..02d8a2707f 100644 --- a/chat-sdk/README.md +++ b/chat-sdk/README.md @@ -252,9 +252,10 @@ message lists them in `attachments`, as `{ input, s3, filename }` references. Th extension is corrected to the file's media type for PNG, JPEG and PDF, because the worker reads the type off the key. Files need message text to go with them. A failed upload rejects `sendMessage` before any -run starts, and `stop()` during the upload aborts it; both leave the transcript as it was. The workspace needs object -storage set up. With Enterprise advanced storage permissions, the user needs read, write -and delete on `windmill_uploads/*`, which the default rules grant. The upload goes through +run starts, and `stop()` during the upload aborts it; both leave the transcript as it was. +The chat never deletes uploads, so files of a send that did not run stay in storage. The +workspace needs object storage set up. With Enterprise advanced storage permissions, the +user needs read and write on `windmill_uploads/*`, which the default rules grant. The upload goes through `job_helpers`, so a restricted token needs `job_helpers:write`; a sandboxed raw app cannot request that scope today, so attachments are not available there yet. diff --git a/chat-sdk/src/api.ts b/chat-sdk/src/api.ts index d878959abb..a512649ec0 100644 --- a/chat-sdk/src/api.ts +++ b/chat-sdk/src/api.ts @@ -232,11 +232,6 @@ export class WindmillChatApi { return (await res.json()) as { file_key: string } } - /** Removes an object from the workspace's object storage. */ - async deleteFile(fileKey: string): Promise { - await this.#request('job_helpers/delete_s3_file', { method: 'DELETE', query: { file_key: fileKey } }) - } - async deleteConversation(conversationId: string): Promise { await this.#request(`flow_conversations/delete/${encodeURIComponent(conversationId)}`, { method: 'DELETE' diff --git a/chat-sdk/src/attachments.ts b/chat-sdk/src/attachments.ts index 77da49c804..ac5e71c7d3 100644 --- a/chat-sdk/src/attachments.ts +++ b/chat-sdk/src/attachments.ts @@ -65,11 +65,6 @@ function dataUrlToBlob(dataUrl: string, fallbackType: string): Blob { return new Blob([bytes], { type: mediaType }) } -/** Delete uploads no run will read. Best effort: a delete that fails leaves that object behind. */ -export async function discardUploads(api: WindmillChatApi, uploaded: UploadedAttachment[]): Promise { - await Promise.all(uploaded.map((u) => api.deleteFile(u.s3).catch(() => {}))) -} - /** * Put each attachment in the workspace's object storage and hand back what the agent reads. * The key's turn prefix and per-file index keep two files with the same name, in this turn or @@ -82,9 +77,8 @@ export async function uploadAttachments( signal?: AbortSignal ): Promise { const prefix = `${CHAT_UPLOADS_PREFIX}/${turnId}` - // One failed upload aborts the rest, and whatever already landed is deleted: no run will - // read it, and a resend uploads under a fresh prefix. Best effort, so a delete that fails - // leaves that object behind rather than masking the upload error. + // One failed upload aborts the rest. Nothing already stored is deleted: the chat never + // removes objects from the workspace's storage, so a send that does not run leaves them. if (signal?.aborted) throw abortError() const batch = new AbortController() const abortBatch = () => batch.abort() @@ -109,11 +103,11 @@ export async function uploadAttachments( } }) ) - const uploaded = results.flatMap((r) => (r.status === 'fulfilled' ? [r.value] : [])) const reasons = results.flatMap((r) => (r.status === 'rejected' ? [r.reason] : [])) // A stop that lands once every upload has answered still withdraws the batch. - if (reasons.length === 0 && !signal?.aborted) return uploaded - await discardUploads(api, uploaded) + if (reasons.length === 0 && !signal?.aborted) { + return results.flatMap((r) => (r.status === 'fulfilled' ? [r.value] : [])) + } // The failure that started it, not the aborts it caused in the other uploads. throw reasons.find((reason) => !isAbortError(reason)) ?? reasons[0] ?? abortError() } finally { diff --git a/chat-sdk/src/chat.ts b/chat-sdk/src/chat.ts index d357e340e1..bf6a06e1c3 100644 --- a/chat-sdk/src/chat.ts +++ b/chat-sdk/src/chat.ts @@ -8,7 +8,7 @@ import { import { resolveConfig, type ResolvedConfig } from './config' import { followJob } from './follow' import { createLocalHistory, type LocalHistory } from './history' -import { discardUploads, uploadAttachments } from './attachments' +import { uploadAttachments } from './attachments' import type { AgentStreamEvent } from './stream' import type { Chat, @@ -171,13 +171,9 @@ class ChatImpl implements Chat { messages: this.#state.messages.map((m) => (m.id === turn.userMessageId ? { ...m, attachments: carried } : m)) }) } - // A subscriber told of those attachments may stop the turn right here. - if (turn.controller.signal.aborted) { - await discardUploads(this.#api, uploaded) - throw abortError() - } } - // Nothing may start once stop() or a conversation switch has withdrawn the turn. + // Nothing may start once stop() or a conversation switch has withdrawn the turn, including + // a stop from a subscriber told of the attachments just above. if (turn.controller.signal.aborted) throw abortError() turn.started = true // Listed only once the run is asked for: a send that never runs (an upload that failed diff --git a/chat-sdk/test/attachments.test.ts b/chat-sdk/test/attachments.test.ts index 0862eec77d..130b720035 100644 --- a/chat-sdk/test/attachments.test.ts +++ b/chat-sdk/test/attachments.test.ts @@ -259,7 +259,7 @@ describe('sendMessage with attachments', () => { expect((await reloaded.loadConversations()).map((c) => c.id)).not.toContain(opened) }) - test('a failed upload deletes the files of the same batch that did land', async () => { + test('a failed upload aborts the rest of its batch and deletes nothing', async () => { let first: (r: Response) => void = () => {} const { fetch, calls } = fetchMock( (c) => { @@ -270,10 +270,6 @@ describe('sendMessage with attachments', () => { setTimeout(() => first(json({ file_key: keys()[0] })), 5) return text('quota exceeded', 507) }, - (c) => - c.method === 'DELETE' && c.url.pathname === '/api/w/ws/job_helpers/delete_s3_file' - ? json('deleted') - : undefined, run, answer ) @@ -288,10 +284,7 @@ describe('sendMessage with attachments', () => { attachmentsInput: { name: 'files', multiple: true } }) ).rejects.toThrow('quota exceeded') - const deletes = calls - .filter((c) => c.method === 'DELETE') - .map((c) => c.url.searchParams.get('file_key')) - expect(deletes).toEqual([keys()[0]]) + expect(calls.filter((c) => c.method === 'DELETE')).toEqual([]) expect(runs(calls)).toHaveLength(0) }) @@ -414,16 +407,8 @@ describe('sendMessage with attachments', () => { expect(call.url.searchParams.get('content_type')).toBe('application/pdf') }) - test('stop() after the uploads land but before the run starts deletes them and runs nothing', async () => { - const { fetch, calls } = fetchMock( - upload, - (c) => - c.method === 'DELETE' && c.url.pathname === '/api/w/ws/job_helpers/delete_s3_file' - ? json('deleted') - : undefined, - run, - answer - ) + test('stop() after the uploads land but before the run starts runs nothing', async () => { + const { fetch, calls } = fetchMock(upload, run, answer) const chat = createChat(options(fetch)) const sending = chat.sendMessage('read this', { attachments: [{ name: 'contract.pdf', data: pdf }], @@ -434,23 +419,12 @@ describe('sendMessage with attachments', () => { await chat.stop() await expect(sending).rejects.toMatchObject({ name: 'AbortError' }) expect(runs(calls)).toHaveLength(0) - const key = uploads(calls)[0].url.searchParams.get('file_key') - expect( - calls.filter((c) => c.method === 'DELETE').map((c) => c.url.searchParams.get('file_key')) - ).toEqual([key]) + expect(calls.filter((c) => c.method === 'DELETE')).toEqual([]) expect(chat.getState()).toMatchObject({ status: 'idle', messages: [], conversations: [] }) }) - test('a subscriber stopping when the attachments appear still discards the uploads', async () => { - const { fetch, calls } = fetchMock( - upload, - (c) => - c.method === 'DELETE' && c.url.pathname === '/api/w/ws/job_helpers/delete_s3_file' - ? json('deleted') - : undefined, - run, - answer - ) + test('a subscriber stopping when the attachments appear runs nothing', async () => { + const { fetch, calls } = fetchMock(upload, run, answer) const chat = createChat(options(fetch)) chat.subscribe((s) => { if (s.messages.some((m) => m.attachments)) void chat.stop() @@ -462,10 +436,7 @@ describe('sendMessage with attachments', () => { }) ).rejects.toMatchObject({ name: 'AbortError' }) expect(runs(calls)).toHaveLength(0) - const key = uploads(calls)[0].url.searchParams.get('file_key') - expect( - calls.filter((c) => c.method === 'DELETE').map((c) => c.url.searchParams.get('file_key')) - ).toEqual([key]) + expect(calls.filter((c) => c.method === 'DELETE')).toEqual([]) expect(chat.getState()).toMatchObject({ status: 'idle', messages: [], conversations: [] }) }) })