From 94fa43b3a7e329674b389b57875a85fc597f47e8 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 11 Aug 2026 15:38:52 -0700 Subject: [PATCH 1/3] fix(uploads): restore archive extraction folder parity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Archive extraction into workspace files/ was rewritten onto the authorized application-operation boundary, and three behavioral regressions came with that move. Together they broke every archive containing a subdirectory, and 100% of copilot extract() calls (materialize-file always passes rootFolderSegments: [baseName], and its catch only handles ArchiveError). 1. Non-canonical folder path. The extractor joined the folder segments with "/" and passed the result as `path` to createWorkspaceFileFolderOperation. That path reaches requireNonRootFolderPath -> parseFolderPath, which requires a leading "/" and byte-for-byte canonical per-segment encoding, so "bundle/data" threw FolderPathError before anything was written — and a folder name containing a space or a reserved character would still have thrown after merely prefixing a slash. 2. exactName: true. createWorkspaceFileFromBuffer was told to demand the exact leaf name, which sets maxAttempts = 1 and raises FileConflictError when the name already exists. The extractor's rollback then deleted every file written so far, so one colliding name destroyed the whole extraction. Reachable today for flat archives through the unzip action of POST /api/tools/file/manage. Restored to auto-suffixing via allocateUniqueWorkspaceFileName. 3. Wrong folder primitive. createWorkspaceFileFolderAtPath creates exactly one leaf, conflicts on an existing path, and requires the parent to exist already. The extractor never creates intermediates and caches by full path, so the first nested entry asked for a folder whose parent was never created. The correct semantics are ensureWorkspaceFileFolderPath: walk every segment, reuse what exists, create only what is missing. Rather than bypass the operation boundary by calling the manager primitive directly, this adds ensureWorkspaceFileFolderPathOperation — an authorized application use case under files.folders.create that expresses "ensure this whole chain exists" — and routes the extractor through it with raw decoded segments, so no path string is built and no encoding can be malformed. archive.test.ts previously mocked the folder operation and asserted the broken shape (path: 'bundle'), which is why this shipped. The suite now fakes the workspace-file store in memory while enforcing the real rules: folder paths run through the production parseFolderPath family, the create-one-leaf operation conflicts and requires a parent, and exactName governs conflict vs auto-suffix. Nested, reuse, encoded-name, and collision cases are covered and each fails against the pre-fix code. --- apps/sim/lib/uploads/archive.test.ts | 211 ++++++++++++++++-- apps/sim/lib/uploads/archive.ts | 20 +- .../application/workspace-file-folders.ts | 42 ++++ 3 files changed, 246 insertions(+), 27 deletions(-) diff --git a/apps/sim/lib/uploads/archive.test.ts b/apps/sim/lib/uploads/archive.test.ts index c4180dbba8e..67af3d51668 100644 --- a/apps/sim/lib/uploads/archive.test.ts +++ b/apps/sim/lib/uploads/archive.test.ts @@ -5,15 +5,34 @@ import { Buffer } from 'buffer' import JSZip from 'jszip' import { beforeEach, describe, expect, it, vi } from 'vitest' -const { mockEnsureFolder, mockUpload, mockDelete } = vi.hoisted(() => ({ - mockEnsureFolder: vi.fn(), +/** + * The workspace-file store is faked in memory rather than stubbed, because the + * defects this suite guards against live in the *contract* between the extractor + * and the folder/file layers, not in the extractor's own arithmetic. The fake + * therefore enforces the real rules: + * + * - folder paths are validated with the production {@link parseFolderPath} family, + * so a non-canonical path (no leading slash, unencoded segment) throws exactly + * as `requireNonRootFolderPath` does in `workspace-file-folder-manager`; + * - `createWorkspaceFileFolderOperation` creates ONE leaf and rejects an existing + * path or a missing parent, mirroring `createWorkspaceFileFolderAtPath`; + * - `exactName: true` throws `FileConflictError` on a duplicate leaf name, while + * `exactName: false` auto-suffixes, mirroring `uploadWorkspaceFile`. + */ +const { store, mockUpload, mockDelete, mockCreateFolder, mockEnsureFolder } = vi.hoisted(() => ({ + store: { + folderIdByPath: new Map(), + fileKeys: new Set(), + sequence: 0, + }, mockUpload: vi.fn(), mockDelete: vi.fn(), + mockCreateFolder: vi.fn(), + mockEnsureFolder: vi.fn(), })) vi.mock('@/lib/workspace-files/application/workspace-file-folders', () => ({ - createWorkspaceFileFolderOperation: { - execute: mockEnsureFolder, - }, + createWorkspaceFileFolderOperation: { execute: mockCreateFolder }, + ensureWorkspaceFileFolderPathOperation: { execute: mockEnsureFolder }, })) vi.mock('@/lib/workspace-files/application/create-workspace-file', () => ({ createWorkspaceFileFromBuffer: { @@ -26,6 +45,7 @@ vi.mock('@/lib/workspace-files/application/delete-workspace-file', () => ({ }, })) +import { buildFolderPath, requireNonRootFolderPath } from '@/lib/folders/paths' import { decompressArchiveBufferToWorkspaceFiles, MAX_ARCHIVE_CENTRAL_DIR_EXTRA_BYTES, @@ -83,21 +103,88 @@ function craftCentralDirectory(records: number, extraPerRecord: number): Buffer return buffer } +/** Mirrors `allocateUniqueWorkspaceFileName`'s " (n)" suffixing. */ +function allocateUniqueName(folderKey: string, name: string): string { + const dot = name.lastIndexOf('.') + const base = dot > 0 ? name.slice(0, dot) : name + const extension = dot > 0 ? name.slice(dot) : '' + for (let attempt = 1; ; attempt++) { + const candidate = `${base} (${attempt})${extension}` + if (!store.fileKeys.has(`${folderKey}|${candidate}`)) return candidate + } +} + +/** Pre-seeds an already-existing workspace file so a later leaf name collides. */ +function seedExistingFile(folderId: string | null, name: string): void { + store.fileKeys.add(`${folderId ?? ''}|${name}`) +} + beforeEach(() => { vi.clearAllMocks() - mockEnsureFolder.mockResolvedValue({ folder: { id: 'folder_1' } }) + store.folderIdByPath.clear() + store.fileKeys.clear() + store.sequence = 0 + + mockCreateFolder.mockImplementation(async ({ input }: { input: { path: string } }) => { + const segments = requireNonRootFolderPath(input.path) + if (store.folderIdByPath.has(input.path)) { + throw new Error(`A folder named "${segments[segments.length - 1]}" already exists`) + } + const parentPath = buildFolderPath(segments.slice(0, -1)) + if (parentPath !== '/' && !store.folderIdByPath.has(parentPath)) { + throw new Error('Parent folder not found') + } + const id = `folder_${++store.sequence}` + store.folderIdByPath.set(input.path, id) + return { folder: { id, path: input.path } } + }) + + mockEnsureFolder.mockImplementation(async ({ input }: { input: { pathSegments: string[] } }) => { + let folderId: string | null = null + const walked: string[] = [] + for (const segment of input.pathSegments) { + walked.push(segment) + const path = buildFolderPath(walked) + const existing = store.folderIdByPath.get(path) + if (existing) { + folderId = existing + continue + } + folderId = `folder_${++store.sequence}` + store.folderIdByPath.set(path, folderId) + } + return { folderId } + }) + mockDelete.mockResolvedValue(undefined) mockUpload.mockImplementation( - async ({ input }: { input: { content: Buffer; name: string } }) => ({ - file: { - id: `f_${input.name}`, - name: input.name, - url: `/api/files/serve/${input.name}`, - key: `workspace/ws/${input.name}`, - size: input.content.length, - type: 'text/plain', - }, - }) + async ({ + input, + }: { + input: { content: Buffer; name: string; folderId?: string | null; exactName: boolean } + }) => { + const folderKey = input.folderId ?? '' + let name = input.name + if (store.fileKeys.has(`${folderKey}|${name}`)) { + if (input.exactName) { + const conflict = new Error(`A file named "${name}" already exists`) + conflict.name = 'FileConflictError' + throw conflict + } + name = allocateUniqueName(folderKey, name) + } + store.fileKeys.add(`${folderKey}|${name}`) + return { + file: { + id: `f_${name}`, + name, + url: `/api/files/serve/${name}`, + key: `workspace/ws/${name}`, + size: input.content.length, + type: 'text/plain', + }, + } + } ) }) @@ -118,11 +205,99 @@ describe('decompressArchiveBufferToWorkspaceFiles', () => { expect(leafNames).toEqual(['report.txt', 'sheet.csv']) // Entries are rooted under the archive's folder; nested paths are preserved. expect(mockEnsureFolder).toHaveBeenCalledWith( - expect.objectContaining({ input: { workspaceId: 'ws', path: 'bundle' } }) + expect.objectContaining({ input: { workspaceId: 'ws', pathSegments: ['bundle'] } }) ) expect(mockEnsureFolder).toHaveBeenCalledWith( - expect.objectContaining({ input: { workspaceId: 'ws', path: 'bundle/data' } }) + expect.objectContaining({ input: { workspaceId: 'ws', pathSegments: ['bundle', 'data'] } }) ) + // Every folder in the chain is materialized, intermediates included. + expect([...store.folderIdByPath.keys()].sort()).toEqual(['/bundle', '/bundle/data']) + }) + + it('creates intermediate folders for a deeply nested archive', async () => { + // `createWorkspaceFileFolderAtPath` semantics would ask for the full leaf path + // whose parents were never created and fail with "Parent folder not found"; + // extraction must ensure the whole chain instead. + const buffer = await buildZip({ 'src/deep/nested/leaf.txt': 'x' }) + + const result = await decompressArchiveBufferToWorkspaceFiles(buffer, { + workspaceId: 'ws', + principal: TEST_PRINCIPAL, + rootFolderSegments: ['bundle'], + }) + + expect(result.extracted).toHaveLength(1) + expect([...store.folderIdByPath.keys()].sort()).toEqual([ + '/bundle', + '/bundle/src', + '/bundle/src/deep', + '/bundle/src/deep/nested', + ]) + expect(mockUpload.mock.calls[0][0].input.folderId).toBe( + store.folderIdByPath.get('/bundle/src/deep/nested') + ) + }) + + it('reuses a folder that already exists instead of failing on conflict', async () => { + // Two entries in the same directory, plus a directory that a previous + // extraction already created — neither may raise a folder conflict. + store.folderIdByPath.set('/bundle', 'preexisting-folder') + const buffer = await buildZip({ 'top.txt': 't', 'docs/a.txt': 'a', 'docs/b.txt': 'b' }) + + const result = await decompressArchiveBufferToWorkspaceFiles(buffer, { + workspaceId: 'ws', + principal: TEST_PRINCIPAL, + rootFolderSegments: ['bundle'], + }) + + expect(result.extracted).toHaveLength(3) + expect([...store.folderIdByPath.keys()].sort()).toEqual(['/bundle', '/bundle/docs']) + expect(store.folderIdByPath.get('/bundle')).toBe('preexisting-folder') + const folderIdByFileName = new Map( + mockUpload.mock.calls.map(([args]) => [args.input.name, args.input.folderId]) + ) + expect(folderIdByFileName.get('top.txt')).toBe('preexisting-folder') + expect(folderIdByFileName.get('a.txt')).toBe(store.folderIdByPath.get('/bundle/docs')) + expect(folderIdByFileName.get('b.txt')).toBe(store.folderIdByPath.get('/bundle/docs')) + }) + + it('extracts into a folder whose name needs path encoding', async () => { + // A space (and other reserved characters) must never be handed to the folder + // layer as a raw path segment — `parseFolderPath` round-trip-checks the + // encoding and rejects "my report" while accepting "my%20report". + const buffer = await buildZip({ 'my report/q1 & q2.txt': 'x' }) + + const result = await decompressArchiveBufferToWorkspaceFiles(buffer, { + workspaceId: 'ws', + principal: TEST_PRINCIPAL, + rootFolderSegments: ['bundle v2'], + }) + + expect(result.extracted).toHaveLength(1) + expect([...store.folderIdByPath.keys()].sort()).toEqual([ + '/bundle%20v2', + '/bundle%20v2/my%20report', + ]) + expect(mockUpload.mock.calls[0][0].input.name).toBe('q1 & q2.txt') + }) + + it('auto-suffixes a leaf whose name already exists instead of rolling back', async () => { + // One colliding name must not destroy an otherwise valid extraction: the + // upload layer allocates a unique name, nothing is deleted, and every entry + // still lands. + seedExistingFile(null, 'report.txt') + const buffer = await buildZip({ 'report.txt': 'hi', 'other.txt': 'yo' }) + + const result = await decompressArchiveBufferToWorkspaceFiles(buffer, { + workspaceId: 'ws', + principal: TEST_PRINCIPAL, + }) + + expect(result.extracted.map((file) => file.name).sort()).toEqual([ + 'other.txt', + 'report (1).txt', + ]) + expect(mockDelete).not.toHaveBeenCalled() }) it('marks extracted files unknown when an archive has secret provenance', async () => { diff --git a/apps/sim/lib/uploads/archive.ts b/apps/sim/lib/uploads/archive.ts index e019a22956d..598d577a2ea 100644 --- a/apps/sim/lib/uploads/archive.ts +++ b/apps/sim/lib/uploads/archive.ts @@ -7,7 +7,7 @@ import type { WorkspaceFileSecretProvenance } from '@/lib/uploads/contexts/works import { getFileExtension, getMimeTypeFromExtension } from '@/lib/uploads/utils/file-utils' import { createWorkspaceFileFromBuffer } from '@/lib/workspace-files/application/create-workspace-file' import { deleteWorkspaceFileOperation } from '@/lib/workspace-files/application/delete-workspace-file' -import { createWorkspaceFileFolderOperation } from '@/lib/workspace-files/application/workspace-file-folders' +import { ensureWorkspaceFileFolderPathOperation } from '@/lib/workspace-files/application/workspace-file-folders' import type { UserFile } from '@/executor/types' /** @@ -362,15 +362,15 @@ export async function decompressArchiveBufferToWorkspaceFiles( const folderKey = folderSegments.join('/') let folderId = folderIdCache.get(folderKey) if (folderId === undefined) { - if (folderSegments.length === 0) { - folderId = null - } else { - const result = await createWorkspaceFileFolderOperation.execute({ + // Ensure-semantics, not create-semantics: an archive addresses every folder + // by its full chain, so intermediates must be materialized and any folder + // that already exists (from a sibling entry or an earlier extraction) reused. + folderId = ( + await ensureWorkspaceFileFolderPathOperation.execute({ principal, - input: { workspaceId, path: folderSegments.join('/') }, + input: { workspaceId, pathSegments: folderSegments }, }) - folderId = result.folder.id - } + ).folderId folderIdCache.set(folderKey, folderId) } @@ -384,7 +384,9 @@ export async function decompressArchiveBufferToWorkspaceFiles( name: leafName, contentType: mimeType, folderId, - exactName: true, + // Auto-suffix on collision: one leaf name that already exists must not + // roll back an otherwise valid extraction. + exactName: false, secretProvenance: extractedSecretProvenance, }, }) diff --git a/apps/sim/lib/workspace-files/application/workspace-file-folders.ts b/apps/sim/lib/workspace-files/application/workspace-file-folders.ts index c8fb8f9d2ff..6e5d97cfb6f 100644 --- a/apps/sim/lib/workspace-files/application/workspace-file-folders.ts +++ b/apps/sim/lib/workspace-files/application/workspace-file-folders.ts @@ -9,6 +9,7 @@ import { createWorkspaceFileFolder, createWorkspaceFileFolderAtPath, deleteWorkspaceFileFolderByPath, + ensureWorkspaceFileFolderPath, listWorkspaceFileFolders, loadWorkspaceFileOperationContext, relocateWorkspaceFileFolderByPath, @@ -46,6 +47,17 @@ export interface CreateWorkspaceFileFolderResult { folder: WorkspaceFileFolderRecord } +export interface EnsureWorkspaceFileFolderPathInput { + workspaceId: string + /** Decoded folder names, outermost first. An empty list resolves to the root. */ + pathSegments: string[] +} + +export interface EnsureWorkspaceFileFolderPathResult { + /** Id of the deepest folder, or `null` when the path resolves to the root. */ + folderId: string | null +} + export interface UpdateWorkspaceFileFolderInput { workspaceId: string folderId?: string @@ -148,6 +160,22 @@ async function executeCreateWorkspaceFileFolder(args: { return { folder } } +async function executeEnsureWorkspaceFileFolderPath(args: { + principal: Parameters[0] + input: EnsureWorkspaceFileFolderPathInput + context: FolderOperationContext +}): Promise { + const attribution = resolvePrincipalAttribution(args.principal, { + workspaceBillingOwnerUserId: args.context.billedAccountUserId, + }) + const folderId = await ensureWorkspaceFileFolderPath({ + workspaceId: args.context.workspaceId, + userId: attribution.attributedUserId, + pathSegments: args.input.pathSegments, + }) + return { folderId } +} + async function executeUpdateWorkspaceFileFolder(args: { input: UpdateWorkspaceFileFolderInput context: FolderOperationContext @@ -240,6 +268,20 @@ export const createWorkspaceFileFolderOperation = defineAuthorizedWorkspaceFileU }, }) +/** + * Idempotently materializes a whole folder chain, reusing every folder that already + * exists and creating only the missing ones. Unlike {@link createWorkspaceFileFolderOperation} + * — which creates exactly one leaf and fails on an existing path or a missing parent — + * this is the primitive for writers that materialize a tree (archive extraction, workspace + * import), where intermediate folders and repeat runs are expected rather than exceptional. + */ +export const ensureWorkspaceFileFolderPathOperation = defineAuthorizedWorkspaceFileUseCase({ + operation: fileOperations.createFolder, + resolveContext: (args: { input: EnsureWorkspaceFileFolderPathInput }) => + resolveFolderContext(args), + execute: executeEnsureWorkspaceFileFolderPath, +}) + export const updateWorkspaceFileFolderOperation = defineAuthorizedWorkspaceFileUseCase({ operation: fileOperations.updateFolder, resolveContext: (args: { input: UpdateWorkspaceFileFolderInput }) => resolveFolderContext(args), From 0077285e18282ba091eabe7723a5a3a277ec4433 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 11 Aug 2026 15:56:01 -0700 Subject: [PATCH 2/3] chore(files): tidy archive extraction cleanup --- apps/sim/lib/uploads/archive.test.ts | 27 +++---------------- .../application/workspace-file-folders.ts | 4 +-- 2 files changed, 6 insertions(+), 25 deletions(-) diff --git a/apps/sim/lib/uploads/archive.test.ts b/apps/sim/lib/uploads/archive.test.ts index 67af3d51668..e996ec3f18a 100644 --- a/apps/sim/lib/uploads/archive.test.ts +++ b/apps/sim/lib/uploads/archive.test.ts @@ -11,15 +11,12 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' * and the folder/file layers, not in the extractor's own arithmetic. The fake * therefore enforces the real rules: * - * - folder paths are validated with the production {@link parseFolderPath} family, - * so a non-canonical path (no leading slash, unencoded segment) throws exactly - * as `requireNonRootFolderPath` does in `workspace-file-folder-manager`; - * - `createWorkspaceFileFolderOperation` creates ONE leaf and rejects an existing - * path or a missing parent, mirroring `createWorkspaceFileFolderAtPath`; + * - folder keys are built with the production {@link buildFolderPath}, so a segment + * that needs encoding is rejected or encoded exactly as the folder layer would; * - `exactName: true` throws `FileConflictError` on a duplicate leaf name, while * `exactName: false` auto-suffixes, mirroring `uploadWorkspaceFile`. */ -const { store, mockUpload, mockDelete, mockCreateFolder, mockEnsureFolder } = vi.hoisted(() => ({ +const { store, mockUpload, mockDelete, mockEnsureFolder } = vi.hoisted(() => ({ store: { folderIdByPath: new Map(), fileKeys: new Set(), @@ -27,11 +24,9 @@ const { store, mockUpload, mockDelete, mockCreateFolder, mockEnsureFolder } = vi }, mockUpload: vi.fn(), mockDelete: vi.fn(), - mockCreateFolder: vi.fn(), mockEnsureFolder: vi.fn(), })) vi.mock('@/lib/workspace-files/application/workspace-file-folders', () => ({ - createWorkspaceFileFolderOperation: { execute: mockCreateFolder }, ensureWorkspaceFileFolderPathOperation: { execute: mockEnsureFolder }, })) vi.mock('@/lib/workspace-files/application/create-workspace-file', () => ({ @@ -45,7 +40,7 @@ vi.mock('@/lib/workspace-files/application/delete-workspace-file', () => ({ }, })) -import { buildFolderPath, requireNonRootFolderPath } from '@/lib/folders/paths' +import { buildFolderPath } from '@/lib/folders/paths' import { decompressArchiveBufferToWorkspaceFiles, MAX_ARCHIVE_CENTRAL_DIR_EXTRA_BYTES, @@ -125,20 +120,6 @@ beforeEach(() => { store.fileKeys.clear() store.sequence = 0 - mockCreateFolder.mockImplementation(async ({ input }: { input: { path: string } }) => { - const segments = requireNonRootFolderPath(input.path) - if (store.folderIdByPath.has(input.path)) { - throw new Error(`A folder named "${segments[segments.length - 1]}" already exists`) - } - const parentPath = buildFolderPath(segments.slice(0, -1)) - if (parentPath !== '/' && !store.folderIdByPath.has(parentPath)) { - throw new Error('Parent folder not found') - } - const id = `folder_${++store.sequence}` - store.folderIdByPath.set(input.path, id) - return { folder: { id, path: input.path } } - }) - mockEnsureFolder.mockImplementation(async ({ input }: { input: { pathSegments: string[] } }) => { let folderId: string | null = null const walked: string[] = [] diff --git a/apps/sim/lib/workspace-files/application/workspace-file-folders.ts b/apps/sim/lib/workspace-files/application/workspace-file-folders.ts index 6e5d97cfb6f..e402994b043 100644 --- a/apps/sim/lib/workspace-files/application/workspace-file-folders.ts +++ b/apps/sim/lib/workspace-files/application/workspace-file-folders.ts @@ -272,8 +272,8 @@ export const createWorkspaceFileFolderOperation = defineAuthorizedWorkspaceFileU * Idempotently materializes a whole folder chain, reusing every folder that already * exists and creating only the missing ones. Unlike {@link createWorkspaceFileFolderOperation} * — which creates exactly one leaf and fails on an existing path or a missing parent — - * this is the primitive for writers that materialize a tree (archive extraction, workspace - * import), where intermediate folders and repeat runs are expected rather than exceptional. + * this is the primitive for writers that materialize a tree (archive extraction), where + * intermediate folders and repeat runs are expected rather than exceptional. */ export const ensureWorkspaceFileFolderPathOperation = defineAuthorizedWorkspaceFileUseCase({ operation: fileOperations.createFolder, From 427b26063a4b5230d2adb0d996e6241554d9b68b Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 11 Aug 2026 16:19:15 -0700 Subject: [PATCH 3/3] fix(uploads): roll back folders archive extraction created MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extraction now materializes folders before uploading files, but the failure path only deleted the extracted files — every folder the call created was left behind. That is not cosmetic: `materialize_file` guards re-extraction by looking up the root folder path and refusing when it has any child, so a half-extracted nested archive turned every retry into "already extracted — delete that folder first" until a human cleaned up the tree by hand. The rollback must delete only folders this call actually inserted, never one it reused: extracting into an existing path is normal (a sibling entry, an earlier successful extraction), and deleting a pre-existing folder would destroy unrelated user data. `ensureWorkspaceFileFolderPath` already distinguishes the two while walking the segment chain, so it (and its application operation) now reports `createdFolderIds` alongside the leaf id. The extractor accumulates those ids in creation order and, on failure, deletes them in reverse — parents are recorded before their children, so reverse order is deepest-first and a parent is never removed out from under a child. Folder cleanup is best-effort like the existing file cleanup, so a cleanup failure never masks the original error. --- .../copilot/tools/handlers/vfs-mutate.test.ts | 5 +- .../lib/copilot/vfs/resource-writer.test.ts | 10 +- apps/sim/lib/uploads/archive.test.ts | 123 +++++++++++++++++- apps/sim/lib/uploads/archive.ts | 39 ++++-- .../workspace-file-folder-manager.ts | 23 +++- .../application/workspace-file-folders.ts | 8 +- .../write-workspace-file-by-path.ts | 4 +- 7 files changed, 190 insertions(+), 22 deletions(-) diff --git a/apps/sim/lib/copilot/tools/handlers/vfs-mutate.test.ts b/apps/sim/lib/copilot/tools/handlers/vfs-mutate.test.ts index 6d2b9e98eb6..e19935bb9a4 100644 --- a/apps/sim/lib/copilot/tools/handlers/vfs-mutate.test.ts +++ b/apps/sim/lib/copilot/tools/handlers/vfs-mutate.test.ts @@ -237,7 +237,10 @@ describe('vfs mv/cp', () => { return mocks.getWorkspaceFileByName('ws-1', segments.at(-1), { folderId: null }) }) mocks.findWorkspaceFileFolderIdByPath.mockResolvedValue(null) - mocks.ensureWorkspaceFileFolderPath.mockResolvedValue('ensured-folder') + mocks.ensureWorkspaceFileFolderPath.mockResolvedValue({ + folderId: 'ensured-folder', + createdFolderIds: [], + }) mocks.ensureCopilotFileFolderPath.mockResolvedValue('ensured-folder') mocks.moveWorkspaceFileItems.mockResolvedValue({ movedItems: { files: 1, folders: 0 } }) mocks.updateWorkspaceFileFolder.mockResolvedValue({ folder: { name: 'Reports 2025' } }) diff --git a/apps/sim/lib/copilot/vfs/resource-writer.test.ts b/apps/sim/lib/copilot/vfs/resource-writer.test.ts index 41f7350773e..d133cf95cdf 100644 --- a/apps/sim/lib/copilot/vfs/resource-writer.test.ts +++ b/apps/sim/lib/copilot/vfs/resource-writer.test.ts @@ -53,7 +53,10 @@ import { describe('resource writer', () => { beforeEach(() => { vi.clearAllMocks() - mocks.ensureWorkspaceFileFolderPath.mockResolvedValue('folder-id') + mocks.ensureWorkspaceFileFolderPath.mockResolvedValue({ + folderId: 'folder-id', + createdFolderIds: [], + }) mocks.admitCreateWorkspaceFile.mockResolvedValue(undefined) }) @@ -107,7 +110,10 @@ describe('resource writer', () => { }) it('auto-creates missing parent folders for plain workspace file creates', async () => { - mocks.ensureWorkspaceFileFolderPath.mockResolvedValue('folder-nested') + mocks.ensureWorkspaceFileFolderPath.mockResolvedValue({ + folderId: 'folder-nested', + createdFolderIds: [], + }) mocks.createWorkspaceFileBufferByPath.execute.mockResolvedValue({ id: 'file-report', name: 'summary.csv', diff --git a/apps/sim/lib/uploads/archive.test.ts b/apps/sim/lib/uploads/archive.test.ts index e996ec3f18a..91fe18f2de0 100644 --- a/apps/sim/lib/uploads/archive.test.ts +++ b/apps/sim/lib/uploads/archive.test.ts @@ -16,18 +16,22 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' * - `exactName: true` throws `FileConflictError` on a duplicate leaf name, while * `exactName: false` auto-suffixes, mirroring `uploadWorkspaceFile`. */ -const { store, mockUpload, mockDelete, mockEnsureFolder } = vi.hoisted(() => ({ +const { store, mockUpload, mockDelete, mockEnsureFolder, mockDeleteFolder } = vi.hoisted(() => ({ store: { folderIdByPath: new Map(), fileKeys: new Set(), + /** Paths passed to the folder-delete operation, in call order. */ + deletedFolderPaths: [] as string[], sequence: 0, }, mockUpload: vi.fn(), mockDelete: vi.fn(), mockEnsureFolder: vi.fn(), + mockDeleteFolder: vi.fn(), })) vi.mock('@/lib/workspace-files/application/workspace-file-folders', () => ({ ensureWorkspaceFileFolderPathOperation: { execute: mockEnsureFolder }, + deleteWorkspaceFileFolderOperation: { execute: mockDeleteFolder }, })) vi.mock('@/lib/workspace-files/application/create-workspace-file', () => ({ createWorkspaceFileFromBuffer: { @@ -109,6 +113,21 @@ function allocateUniqueName(folderKey: string, name: string): string { } } +/** Reverse lookup of the fake folder store: id -> path, or `undefined` if gone. */ +function folderPathById(folderId: string | undefined): string | undefined { + for (const [path, id] of store.folderIdByPath) { + if (id === folderId) return path + } + return undefined +} + +/** Pre-seeds a folder chain that existed before extraction ran. */ +function seedExistingFolders(...paths: string[][]): void { + for (const segments of paths) { + store.folderIdByPath.set(buildFolderPath(segments), `preexisting_${++store.sequence}`) + } +} + /** Pre-seeds an already-existing workspace file so a later leaf name collides. */ function seedExistingFile(folderId: string | null, name: string): void { store.fileKeys.add(`${folderId ?? ''}|${name}`) @@ -118,11 +137,15 @@ beforeEach(() => { vi.clearAllMocks() store.folderIdByPath.clear() store.fileKeys.clear() + store.deletedFolderPaths.length = 0 store.sequence = 0 mockEnsureFolder.mockImplementation(async ({ input }: { input: { pathSegments: string[] } }) => { let folderId: string | null = null const walked: string[] = [] + // Only the segments this call actually inserts are reported as created; a + // segment resolved from the store was reused and must never be rolled back. + const createdFolderIds: string[] = [] for (const segment of input.pathSegments) { walked.push(segment) const path = buildFolderPath(walked) @@ -133,10 +156,28 @@ beforeEach(() => { } folderId = `folder_${++store.sequence}` store.folderIdByPath.set(path, folderId) + createdFolderIds.push(folderId) } - return { folderId } + return { folderId, createdFolderIds } }) + mockDeleteFolder.mockImplementation( + async ({ input }: { input: { folderId?: string; recursive?: boolean } }) => { + const path = folderPathById(input.folderId) + // Mirrors `deleteWorkspaceFileFolderOperation`, which raises `not_found` when + // nothing was archived — deleting a parent before its children would make the + // child's own delete hit this. + if (!path) throw new Error('Folder not found') + store.deletedFolderPaths.push(path) + for (const [candidate] of store.folderIdByPath) { + if (candidate === path || candidate.startsWith(`${path}/`)) { + store.folderIdByPath.delete(candidate) + } + } + return { deletedItems: { files: 0, folders: 1 } } + } + ) + mockDelete.mockResolvedValue(undefined) mockUpload.mockImplementation( async ({ @@ -443,6 +484,84 @@ describe('decompressArchiveBufferToWorkspaceFiles', () => { ) }) + it('rolls back the folders it created when an upload fails mid-extraction', async () => { + // `materialize_file` refuses to re-extract into a root folder that still has any + // child, so a folder left behind by a failed run turns every retry into + // "already extracted" until a human deletes the tree by hand. + const buffer = await buildZip({ 'a/one.txt': 'first', 'b/two.txt': 'second' }) + mockUpload + .mockResolvedValueOnce({ + file: { id: 'f_one', name: 'one.txt', url: '/one', key: 'k/one', size: 5 }, + }) + .mockRejectedValueOnce(new Error('storage quota exceeded')) + + await expect( + decompressArchiveBufferToWorkspaceFiles(buffer, { + workspaceId: 'ws', + principal: TEST_PRINCIPAL, + rootFolderSegments: ['bundle'], + }) + ).rejects.toThrow('storage quota exceeded') + + expect([...store.folderIdByPath.keys()]).toEqual([]) + expect(mockDeleteFolder).toHaveBeenCalledWith( + expect.objectContaining({ + input: expect.objectContaining({ workspaceId: 'ws', recursive: true }), + }) + ) + }) + + it('leaves a folder that already existed before the call untouched on rollback', async () => { + // Extracting into an existing path is normal — a sibling entry, or an earlier + // successful extraction. Deleting a reused folder would destroy unrelated data. + seedExistingFolders(['bundle'], ['bundle', 'keep']) + const preexistingIds = [...store.folderIdByPath.values()] + const buffer = await buildZip({ 'keep/kept.txt': 'a', 'fresh/new.txt': 'b' }) + mockUpload + .mockResolvedValueOnce({ + file: { id: 'f_kept', name: 'kept.txt', url: '/kept', key: 'k/kept', size: 1 }, + }) + .mockRejectedValueOnce(new Error('storage quota exceeded')) + + await expect( + decompressArchiveBufferToWorkspaceFiles(buffer, { + workspaceId: 'ws', + principal: TEST_PRINCIPAL, + rootFolderSegments: ['bundle'], + }) + ).rejects.toThrow('storage quota exceeded') + + expect([...store.folderIdByPath.keys()].sort()).toEqual(['/bundle', '/bundle/keep']) + expect(store.deletedFolderPaths).toEqual(['/bundle/fresh']) + const deletedIds = mockDeleteFolder.mock.calls.map(([args]) => args.input.folderId) + for (const preexistingId of preexistingIds) { + expect(deletedIds).not.toContain(preexistingId) + } + }) + + it('deletes rolled-back folders deepest-first', async () => { + // A parent removed before its children would make the children's own deletes + // fail (nothing left to archive), so the unwind walks creation order backwards. + const buffer = await buildZip({ 'x/y/z/leaf.txt': 'a' }) + mockUpload.mockRejectedValueOnce(new Error('storage quota exceeded')) + + await expect( + decompressArchiveBufferToWorkspaceFiles(buffer, { + workspaceId: 'ws', + principal: TEST_PRINCIPAL, + rootFolderSegments: ['bundle'], + }) + ).rejects.toThrow('storage quota exceeded') + + expect(store.deletedFolderPaths).toEqual([ + '/bundle/x/y/z', + '/bundle/x/y', + '/bundle/x', + '/bundle', + ]) + expect([...store.folderIdByPath.keys()]).toEqual([]) + }) + it('does not count noise entries toward the extraction cap when they are being skipped', async () => { // macOS Finder zips carry a __MACOSX/._* shadow per file, doubling the raw // entry count. 501 files + 501 shadows = 1002 raw entries — over the diff --git a/apps/sim/lib/uploads/archive.ts b/apps/sim/lib/uploads/archive.ts index 598d577a2ea..561d48330f0 100644 --- a/apps/sim/lib/uploads/archive.ts +++ b/apps/sim/lib/uploads/archive.ts @@ -7,7 +7,10 @@ import type { WorkspaceFileSecretProvenance } from '@/lib/uploads/contexts/works import { getFileExtension, getMimeTypeFromExtension } from '@/lib/uploads/utils/file-utils' import { createWorkspaceFileFromBuffer } from '@/lib/workspace-files/application/create-workspace-file' import { deleteWorkspaceFileOperation } from '@/lib/workspace-files/application/delete-workspace-file' -import { ensureWorkspaceFileFolderPathOperation } from '@/lib/workspace-files/application/workspace-file-folders' +import { + deleteWorkspaceFileFolderOperation, + ensureWorkspaceFileFolderPathOperation, +} from '@/lib/workspace-files/application/workspace-file-folders' import type { UserFile } from '@/executor/types' /** @@ -345,9 +348,14 @@ export async function decompressArchiveBufferToWorkspaceFiles( // Pass 2 — extract: the archive is proven within caps; inflate again and upload. // Uploads themselves can still fail mid-loop (storage/DB errors, quota crossed - // by another writer), so a failure rolls back every file written so far — - // callers and their retries must never observe a partial tree. + // by another writer), so a failure rolls back every file written so far *and* + // every folder this call materialized — callers and their retries must never + // observe a partial tree. Leftover folders are not cosmetic: `materialize_file` + // refuses to re-extract into a root folder that still has any child, so a + // half-extracted tree would make every retry fail until a human deletes it. const folderIdCache = new Map() + /** Only folders this call inserted, in creation order — never a reused one. */ + const createdFolderIds: string[] = [] const extracted: UserFile[] = [] let totalBytes = 0 try { @@ -365,12 +373,12 @@ export async function decompressArchiveBufferToWorkspaceFiles( // Ensure-semantics, not create-semantics: an archive addresses every folder // by its full chain, so intermediates must be materialized and any folder // that already exists (from a sibling entry or an earlier extraction) reused. - folderId = ( - await ensureWorkspaceFileFolderPathOperation.execute({ - principal, - input: { workspaceId, pathSegments: folderSegments }, - }) - ).folderId + const ensured = await ensureWorkspaceFileFolderPathOperation.execute({ + principal, + input: { workspaceId, pathSegments: folderSegments }, + }) + folderId = ensured.folderId + createdFolderIds.push(...ensured.createdFolderIds) folderIdCache.set(folderKey, folderId) } @@ -413,6 +421,19 @@ export async function decompressArchiveBufferToWorkspaceFiles( // the original error is what the caller needs to see. } } + // Deepest-first (creation order records parents before children), so a parent is + // never removed out from under a child that is still being cleaned up. + for (let index = createdFolderIds.length - 1; index >= 0; index--) { + try { + await deleteWorkspaceFileFolderOperation.execute({ + principal, + input: { workspaceId, folderId: createdFolderIds[index], recursive: true }, + }) + } catch { + // Best-effort: a folder whose cleanup fails is still deletable by hand; + // the original error is what the caller needs to see. + } + } throw error } diff --git a/apps/sim/lib/uploads/contexts/workspace/workspace-file-folder-manager.ts b/apps/sim/lib/uploads/contexts/workspace/workspace-file-folder-manager.ts index c1c745d9d0b..fc4d874057d 100644 --- a/apps/sim/lib/uploads/contexts/workspace/workspace-file-folder-manager.ts +++ b/apps/sim/lib/uploads/contexts/workspace/workspace-file-folder-manager.ts @@ -564,18 +564,31 @@ export async function createWorkspaceFileFolder(params: { return mapFolderWithPath(params.workspaceId, folder) } +/** + * Outcome of {@link ensureWorkspaceFileFolderPath}. `createdFolderIds` lists only the + * folders this call actually inserted, outermost-first, so a caller that has to unwind + * a partial write can delete exactly what it added (reverse the list for deepest-first) + * without ever touching a folder that was merely reused. + */ +export interface EnsureWorkspaceFileFolderPathOutcome { + /** Id of the deepest folder, or `null` when the path resolves to the root. */ + folderId: string | null + /** Ids inserted by this call, in creation order (parents before children). */ + createdFolderIds: string[] +} + export async function ensureWorkspaceFileFolderPath(params: { workspaceId: string userId: string pathSegments: string[] -}): Promise { - if (params.pathSegments.length === 0) return null +}): Promise { + if (params.pathSegments.length === 0) return { folderId: null, createdFolderIds: [] } // Fast path: the whole chain already exists (the common case for repeated // writes into known folders) — per-segment indexed lookups instead of // loading the workspace's entire folder table. const existing = await findWorkspaceFileFolderIdByPath(params.workspaceId, params.pathSegments) - if (existing) return existing + if (existing) return { folderId: existing, createdFolderIds: [] } // Load all active folders once and build a lookup keyed by "name|parentId" // so we can resolve existing segments without a per-segment SELECT. @@ -597,6 +610,7 @@ export async function ensureWorkspaceFileFolderPath(params: { } let parentId: string | null = null + const createdFolderIds: string[] = [] for (const rawSegment of params.pathSegments) { const name = normalizeWorkspaceFileItemName(rawSegment, 'Folder') @@ -629,6 +643,7 @@ export async function ensureWorkspaceFileFolderPath(params: { updatedAt: created.updatedAt, }) parentId = created.id + createdFolderIds.push(created.id) } catch (error) { if ( error instanceof WorkspaceFileFolderConflictError || @@ -654,7 +669,7 @@ export async function ensureWorkspaceFileFolderPath(params: { } } - return parentId + return { folderId: parentId, createdFolderIds } } export async function updateWorkspaceFileFolder(params: { diff --git a/apps/sim/lib/workspace-files/application/workspace-file-folders.ts b/apps/sim/lib/workspace-files/application/workspace-file-folders.ts index e402994b043..6e9948ae4d7 100644 --- a/apps/sim/lib/workspace-files/application/workspace-file-folders.ts +++ b/apps/sim/lib/workspace-files/application/workspace-file-folders.ts @@ -56,6 +56,11 @@ export interface EnsureWorkspaceFileFolderPathInput { export interface EnsureWorkspaceFileFolderPathResult { /** Id of the deepest folder, or `null` when the path resolves to the root. */ folderId: string | null + /** + * Ids this call inserted, outermost-first — never a folder it reused. Callers that + * materialize a tree use it to unwind exactly their own writes on failure. + */ + createdFolderIds: string[] } export interface UpdateWorkspaceFileFolderInput { @@ -168,12 +173,11 @@ async function executeEnsureWorkspaceFileFolderPath(args: { const attribution = resolvePrincipalAttribution(args.principal, { workspaceBillingOwnerUserId: args.context.billedAccountUserId, }) - const folderId = await ensureWorkspaceFileFolderPath({ + return ensureWorkspaceFileFolderPath({ workspaceId: args.context.workspaceId, userId: attribution.attributedUserId, pathSegments: args.input.pathSegments, }) - return { folderId } } async function executeUpdateWorkspaceFileFolder(args: { diff --git a/apps/sim/lib/workspace-files/application/write-workspace-file-by-path.ts b/apps/sim/lib/workspace-files/application/write-workspace-file-by-path.ts index 51601a3a33c..eaae6c28d71 100644 --- a/apps/sim/lib/workspace-files/application/write-workspace-file-by-path.ts +++ b/apps/sim/lib/workspace-files/application/write-workspace-file-by-path.ts @@ -85,7 +85,7 @@ async function executeCreate({ const folderUserId = await resolveFolderAttributionUserId(principal, input.workspaceId) - const folderId = await ensureWorkspaceFileFolderPath({ + const { folderId } = await ensureWorkspaceFileFolderPath({ workspaceId: input.workspaceId, userId: folderUserId, pathSegments: parsed.folderSegments, @@ -145,7 +145,7 @@ async function executeCreateBuffer({ const parsed = parseWorkspaceFileCreatePath(input.path) await admitCreateWorkspaceFile(principal, input.workspaceId) const folderUserId = await resolveFolderAttributionUserId(principal, input.workspaceId) - const folderId = await ensureWorkspaceFileFolderPath({ + const { folderId } = await ensureWorkspaceFileFolderPath({ workspaceId: input.workspaceId, userId: folderUserId, pathSegments: parsed.folderSegments,