Skip to content

Commit 850f742

Browse files
fix(files): classify upload failures instead of matching their wording
Bugbot round 2. uploadWorkspaceFile had the same outer-catch rewrap that updateWorkspaceFileContent did, so a blown storage quota reached the route as a bare Error and the v2 handler recovered the status by substring-matching the message. Any rewording silently demoted a 413 to a 500. - uploadWorkspaceFile rethrows a classified failure untouched and attaches cause to the generic wrap. - FileConflictError is now an OrchestrationError('conflict'), so a duplicate name classifies like every other conflict. Its 'FILE_EXISTS' discriminator had no readers and is gone; the instanceof checks elsewhere still hold. - The v2 upload handler uses v2CaughtOrchestrationError, dropping all three string matches. Also documents that bulk-archive is best-effort: unknown or already-archived ids are skipped rather than failing the call, and deletedItems is what actually happened. That asymmetry with the single-id DELETE was undocumented.
1 parent ff1d964 commit 850f742

4 files changed

Lines changed: 49 additions & 17 deletions

File tree

apps/docs/openapi-v2-files-audit.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1051,7 +1051,7 @@
10511051
"post": {
10521052
"operationId": "bulkArchiveFileItems",
10531053
"summary": "Archive Files and Folders",
1054-
"description": "Archive (soft delete) files and/or folders in one call. Archiving a folder cascades to everything under it, so `deletedItems` reports totals larger than the selection. Archived items remain listable via `scope=archived` and can be restored.",
1054+
"description": "Archive (soft delete) files and/or folders in one call. Archiving a folder cascades to everything under it, so `deletedItems` reports totals larger than the selection. Archived items remain listable via `scope=archived` and can be restored.\n\n**This endpoint is best-effort and idempotent.** Ids that do not exist, belong to another workspace, or are already archived are skipped rather than failing the request — the call still returns `200`. `deletedItems` is what was actually archived, so compare it against your selection if you need to detect that something was skipped. The single-item `DELETE /api/v2/files/{fileId}` does return `404` for a missing id.",
10551055
"tags": ["Files"],
10561056
"x-codeSamples": [
10571057
{

apps/sim/app/api/v2/files/route.test.ts

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ vi.mock('@sim/audit', () => ({
5353
AuditResourceType: { FILE: 'file' },
5454
}))
5555

56+
import { OrchestrationError } from '@/lib/core/orchestration/types'
5657
import { GET, POST } from '@/app/api/v2/files/route'
5758

5859
const WS = 'workspace-1'
@@ -287,11 +288,35 @@ describe('POST /api/v2/files', () => {
287288
})
288289

289290
it('404s when the target folder does not exist', async () => {
290-
mockUploadWorkspaceFile.mockRejectedValue(new Error('Target folder not found'))
291+
mockUploadWorkspaceFile.mockRejectedValue(
292+
new OrchestrationError('not_found', 'Target folder not found')
293+
)
291294

292295
const res = await callUpload(`workspaceId=${WS}&folderId=missing`)
293296

294297
expect(res.status).toBe(404)
295298
expect((await res.json()).error.code).toBe('NOT_FOUND')
296299
})
300+
301+
it('413s on a blown storage quota by class, not by message wording', async () => {
302+
mockUploadWorkspaceFile.mockRejectedValue(
303+
new OrchestrationError('payload_too_large', 'Quota exceeded for this workspace')
304+
)
305+
306+
const res = await callUpload(`workspaceId=${WS}`)
307+
308+
expect(res.status).toBe(413)
309+
expect((await res.json()).error.code).toBe('PAYLOAD_TOO_LARGE')
310+
})
311+
312+
it('409s on a duplicate-name conflict by class', async () => {
313+
mockUploadWorkspaceFile.mockRejectedValue(
314+
new OrchestrationError('conflict', 'A file named "data.csv" already exists in this workspace')
315+
)
316+
317+
const res = await callUpload(`workspaceId=${WS}`)
318+
319+
expect(res.status).toBe(409)
320+
expect((await res.json()).error.code).toBe('CONFLICT')
321+
})
297322
})

apps/sim/app/api/v2/files/route.ts

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ import {
1515
} from '@/lib/core/utils/stream-limits'
1616
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
1717
import {
18-
FileConflictError,
1918
getWorkspaceFile,
2019
listWorkspaceFiles,
2120
uploadWorkspaceFile,
@@ -26,6 +25,7 @@ import { v2ApiGateError } from '@/app/api/v2/lib/gate'
2625
import {
2726
decodeCursor,
2827
encodeCursor,
28+
v2CaughtOrchestrationError,
2929
v2CursorList,
3030
v2Data,
3131
v2Error,
@@ -226,17 +226,12 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
226226
return v2Error('PAYLOAD_TOO_LARGE', error.message)
227227
}
228228

229-
const message = getErrorMessage(error, 'Failed to upload file')
230-
if (error instanceof FileConflictError || message.includes('already exists')) {
231-
return v2Error('CONFLICT', message)
232-
}
233-
if (message === 'Target folder not found') {
234-
return v2Error('NOT_FOUND', message)
235-
}
236-
if (message.includes('Storage limit') || message.includes('storage limit')) {
237-
return v2Error('PAYLOAD_TOO_LARGE', 'Storage limit exceeded')
238-
}
229+
// Conflicts, a missing target folder, and a blown storage quota all arrive classified
230+
// now, so the status comes off the error's code rather than its wording.
231+
const classified = v2CaughtOrchestrationError(error)
232+
if (classified) return classified
239233

234+
const message = getErrorMessage(error, 'Failed to upload file')
240235
logger.error('Error uploading file', { error: message })
241236
return v2Error('INTERNAL_ERROR', 'Internal server error')
242237
}

apps/sim/lib/uploads/contexts/workspace/workspace-file-manager.ts

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -52,10 +52,15 @@ const logger = createLogger('WorkspaceFileStorage')
5252

5353
export type WorkspaceFileScope = 'active' | 'archived' | 'all'
5454

55-
export class FileConflictError extends Error {
56-
readonly code = 'FILE_EXISTS' as const
55+
/**
56+
* An {@link OrchestrationError} so every surface reaches 409 by class rather than by
57+
* searching the message for "already exists". Carries the inherited `code: 'conflict'`;
58+
* the old `'FILE_EXISTS'` discriminator had no readers.
59+
*/
60+
export class FileConflictError extends OrchestrationError {
5761
constructor(name: string) {
58-
super(`A file named "${name}" already exists in this workspace`)
62+
super('conflict', `A file named "${name}" already exists in this workspace`)
63+
this.name = 'FileConflictError'
5964
}
6065
}
6166

@@ -424,8 +429,15 @@ export async function uploadWorkspaceFile(
424429
)
425430
continue
426431
}
432+
// A classified failure (a blown storage quota, a missing target folder) keeps its class:
433+
// re-wrapping it in a bare Error is what forced every caller to substring-match the
434+
// message to recover the status.
435+
const classified = asOrchestrationError(error)
436+
if (classified) throw classified
427437
logger.error(`Failed to upload workspace file ${fileName}:`, error)
428-
throw new Error(`Failed to upload file: ${getErrorMessage(error, 'Unknown error')}`)
438+
throw new Error(`Failed to upload file: ${getErrorMessage(error, 'Unknown error')}`, {
439+
cause: error,
440+
})
429441
}
430442
}
431443

0 commit comments

Comments
 (0)