diff --git a/apps/sim/app/api/files/multipart/route.test.ts b/apps/sim/app/api/files/multipart/route.test.ts index 97a4a368759..5f8fb1f2892 100644 --- a/apps/sim/app/api/files/multipart/route.test.ts +++ b/apps/sim/app/api/files/multipart/route.test.ts @@ -52,19 +52,19 @@ vi.mock('@/lib/uploads/providers/blob/client', () => ({ })) vi.mock('@/lib/uploads/contexts/execution/utils', () => ({ - generateExecutionAttachmentKey: mockGenerateExecutionAttachmentKey, + generateUniqueExecutionFileKey: mockGenerateUniqueExecutionFileKey, })) vi.mock('@/lib/workspaces/permissions/utils', () => permissionsMock) const { mockCheckStorageQuota, - mockGenerateExecutionAttachmentKey, + mockGenerateUniqueExecutionFileKey, mockInitiateS3MultipartUpload, mockResolveStorageBillingContext, } = vi.hoisted(() => ({ mockCheckStorageQuota: vi.fn(), - mockGenerateExecutionAttachmentKey: vi.fn(), + mockGenerateUniqueExecutionFileKey: vi.fn(), mockInitiateS3MultipartUpload: vi.fn(), mockResolveStorageBillingContext: vi.fn(), })) @@ -250,7 +250,7 @@ describe('POST /api/files/multipart action=initiate quota enforcement', () => { mockResolveStorageBillingContext.mockResolvedValue(STORAGE_CONTEXT) mockCheckStorageQuota.mockResolvedValue({ allowed: true }) mockInitiateS3MultipartUpload.mockResolvedValue({ uploadId: 'up-1', key: 'k/file.bin' }) - mockGenerateExecutionAttachmentKey.mockImplementation( + mockGenerateUniqueExecutionFileKey.mockImplementation( ( context: { workspaceId: string; workflowId: string; executionId: string }, fileName: string @@ -311,7 +311,7 @@ describe('POST /api/files/multipart action=initiate quota enforcement', () => { }) it('allocates distinct multipart keys for duplicate execution attachment names', async () => { - mockGenerateExecutionAttachmentKey + mockGenerateUniqueExecutionFileKey .mockReturnValueOnce('execution/ws-1/wf-1/exec-1/one-output.bin') .mockReturnValueOnce('execution/ws-1/wf-1/exec-1/two-output.bin') mockInitiateS3MultipartUpload.mockImplementation(async ({ customKey }) => ({ diff --git a/apps/sim/app/api/files/multipart/route.ts b/apps/sim/app/api/files/multipart/route.ts index 38c1a396e58..1f20bad4141 100644 --- a/apps/sim/app/api/files/multipart/route.ts +++ b/apps/sim/app/api/files/multipart/route.ts @@ -215,10 +215,10 @@ export const POST = withRouteHandler(async (request: NextRequest) => { { status: 400 } ) } - const { generateExecutionAttachmentKey } = await import( + const { generateUniqueExecutionFileKey } = await import( '@/lib/uploads/contexts/execution/utils' ) - customKey = generateExecutionAttachmentKey( + customKey = generateUniqueExecutionFileKey( { workspaceId, workflowId, executionId }, fileName ) diff --git a/apps/sim/app/api/files/presigned/route.test.ts b/apps/sim/app/api/files/presigned/route.test.ts index 1fd1578beb3..ac43909b0fd 100644 --- a/apps/sim/app/api/files/presigned/route.test.ts +++ b/apps/sim/app/api/files/presigned/route.test.ts @@ -24,7 +24,7 @@ const { mockIsUsingCloudStorageUploads, mockGetUserEntityPermissions, mockGenerateWorkspaceFileKey, - mockGenerateExecutionAttachmentKey, + mockGenerateUniqueExecutionFileKey, mockInsertFileMetadata, mockCheckStorageQuotaForBillingContext, mockDecrementStorageUsageForBillingContext, @@ -52,7 +52,7 @@ const { mockGenerateWorkspaceFileKey: vi.fn( (workspaceId: string, fileName: string) => `workspace/${workspaceId}/${fileName}` ), - mockGenerateExecutionAttachmentKey: vi.fn( + mockGenerateUniqueExecutionFileKey: vi.fn( (ctx: { workspaceId: string; workflowId: string; executionId: string }, fileName: string) => `execution/${ctx.workspaceId}/${ctx.workflowId}/${ctx.executionId}/attachment-${fileName}` ), @@ -110,7 +110,7 @@ vi.mock('@/lib/uploads/contexts/workspace/workspace-file-manager', () => ({ })) vi.mock('@/lib/uploads/contexts/execution/utils', () => ({ - generateExecutionAttachmentKey: mockGenerateExecutionAttachmentKey, + generateUniqueExecutionFileKey: mockGenerateUniqueExecutionFileKey, })) vi.mock('@/lib/uploads/server/metadata', () => ({ @@ -752,7 +752,7 @@ describe('/api/files/presigned', () => { describe('execution uploads', () => { it('allocates distinct create-only keys for duplicate attachment names', async () => { setupFileApiMocks({ cloudEnabled: true, storageProvider: 's3' }) - mockGenerateExecutionAttachmentKey + mockGenerateUniqueExecutionFileKey .mockReturnValueOnce('execution/ws-1/wf-1/exec-1/one-output.txt') .mockReturnValueOnce('execution/ws-1/wf-1/exec-1/two-output.txt') diff --git a/apps/sim/app/api/files/presigned/route.ts b/apps/sim/app/api/files/presigned/route.ts index 63339421bc3..881784f2f23 100644 --- a/apps/sim/app/api/files/presigned/route.ts +++ b/apps/sim/app/api/files/presigned/route.ts @@ -11,7 +11,7 @@ import { getSession } from '@/lib/auth' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { CopilotFiles } from '@/lib/uploads' import { getServeStoragePrefix } from '@/lib/uploads/config' -import { generateExecutionAttachmentKey } from '@/lib/uploads/contexts/execution/utils' +import { generateUniqueExecutionFileKey } from '@/lib/uploads/contexts/execution/utils' import { generateKnowledgeBaseFileKey } from '@/lib/uploads/contexts/knowledge-base/knowledge-base-file-manager' import { generateWorkspaceFileKey } from '@/lib/uploads/contexts/workspace/workspace-file-manager' import { generatePresignedUploadUrl, hasCloudStorage } from '@/lib/uploads/core/storage-service' @@ -222,7 +222,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => { throw new ValidationError(fileValidationError.message) } - const customKey = generateExecutionAttachmentKey( + const customKey = generateUniqueExecutionFileKey( { workspaceId, workflowId, executionId }, fileName ) diff --git a/apps/sim/lib/execution/payloads/store.ts b/apps/sim/lib/execution/payloads/store.ts index d662447aaa8..90762f473ba 100644 --- a/apps/sim/lib/execution/payloads/store.ts +++ b/apps/sim/lib/execution/payloads/store.ts @@ -16,7 +16,7 @@ import { isValidLargeValueKey, readLargeValueRefFromStorage, } from '@/lib/execution/payloads/materialization.server' -import { generateExecutionFileKey } from '@/lib/uploads/contexts/execution/utils' +import { generateLargeValuePayloadKey } from '@/lib/uploads/contexts/execution/utils' const logger = createLogger('LargeExecutionPayloadStore') @@ -75,10 +75,7 @@ async function persistValue( return undefined } - const key = generateExecutionFileKey( - { workspaceId, workflowId, executionId }, - `large-value-${id}.json` - ) + const key = generateLargeValuePayloadKey({ workspaceId, workflowId, executionId }, id) try { const { StorageService } = await import('@/lib/uploads') diff --git a/apps/sim/lib/uploads/contexts/execution/execution-file-manager.test.ts b/apps/sim/lib/uploads/contexts/execution/execution-file-manager.test.ts index 496358dfdd3..4f0c5d007c5 100644 --- a/apps/sim/lib/uploads/contexts/execution/execution-file-manager.test.ts +++ b/apps/sim/lib/uploads/contexts/execution/execution-file-manager.test.ts @@ -23,7 +23,13 @@ vi.mock('@/lib/uploads/providers/s3/client', () => ({ import { uploadExecutionFile } from '@/lib/uploads/contexts/execution/execution-file-manager' -describe('uploadExecutionFile replacement compatibility', () => { +const context = { + workspaceId: 'workspace-1', + workflowId: 'workflow-1', + executionId: 'execution-1', +} + +describe('uploadExecutionFile key allocation', () => { beforeEach(() => { vi.clearAllMocks() resetDbChainMock() @@ -35,57 +41,27 @@ describe('uploadExecutionFile replacement compatibility', () => { type: contentType, })) mockGetPresignedUrlWithConfig.mockResolvedValue('https://example.com/download') + dbChainMockFns.limit.mockResolvedValue([]) + dbChainMockFns.returning.mockResolvedValue([{ id: 'file-1' }]) }) - it('allows changed bytes and content type at the same execution-scoped key', async () => { - const context = { - workspaceId: 'workspace-1', - workflowId: 'workflow-1', - executionId: 'execution-1', - } - const key = 'execution/workspace-1/workflow-1/execution-1/result.txt' - const existingMetadata = { - id: 'file-1', - key, - userId: 'user-1', - workspaceId: 'workspace-1', - folderId: null, - context: 'execution', - originalName: key, - displayName: key, - contentType: 'text/plain', - size: 3, - deletedAt: null, - } - - dbChainMockFns.limit - .mockResolvedValueOnce([]) - .mockResolvedValueOnce([]) - .mockResolvedValueOnce([existingMetadata]) - dbChainMockFns.returning.mockResolvedValueOnce([existingMetadata]) - + it('gives same-named files in one execution distinct keys', async () => { const first = await uploadExecutionFile( context, - Buffer.from('old'), - 'result.txt', - 'text/plain', + Buffer.alloc(13575), + 'image.png', + 'image/png', 'user-1' ) const second = await uploadExecutionFile( context, - Buffer.from('{"new":true}'), - 'result.txt', - 'application/json', + Buffer.alloc(37226), + 'image.png', + 'image/png', 'user-1' ) - expect(first.key).toBe(key) - expect(second).toMatchObject({ - key, - size: 12, - type: 'application/json', - }) - expect(mockUploadToS3).toHaveBeenCalledTimes(2) - expect(dbChainMockFns.insert).toHaveBeenCalledTimes(1) + expect(first.key).not.toBe(second.key) + expect(dbChainMockFns.insert).toHaveBeenCalledTimes(2) }) }) diff --git a/apps/sim/lib/uploads/contexts/execution/execution-file-manager.ts b/apps/sim/lib/uploads/contexts/execution/execution-file-manager.ts index 304da53a9c4..5e3bdf77141 100644 --- a/apps/sim/lib/uploads/contexts/execution/execution-file-manager.ts +++ b/apps/sim/lib/uploads/contexts/execution/execution-file-manager.ts @@ -3,7 +3,10 @@ import { getErrorMessage } from '@sim/utils/errors' import { isPayloadSizeLimitError } from '@/lib/core/utils/stream-limits' import { isUserFileWithMetadata } from '@/lib/core/utils/user-file' import type { ExecutionContext } from '@/lib/uploads/contexts/execution/utils' -import { generateExecutionFileKey, generateFileId } from '@/lib/uploads/contexts/execution/utils' +import { + generateFileId, + generateUniqueExecutionFileKey, +} from '@/lib/uploads/contexts/execution/utils' import type { UserFile } from '@/executor/types' const logger = createLogger('ExecutionFileStorage') @@ -78,7 +81,7 @@ export async function uploadExecutionFile( bufferSize: fileBuffer.length, }) - const storageKey = generateExecutionFileKey(context, fileName) + const storageKey = generateUniqueExecutionFileKey(context, fileName) const fileId = generateFileId() logger.info(`Generated storage key: "${storageKey}" for file: ${fileName}`) diff --git a/apps/sim/lib/uploads/contexts/execution/utils.test.ts b/apps/sim/lib/uploads/contexts/execution/utils.test.ts index d7d06b00784..da705e3ef34 100644 --- a/apps/sim/lib/uploads/contexts/execution/utils.test.ts +++ b/apps/sim/lib/uploads/contexts/execution/utils.test.ts @@ -3,8 +3,8 @@ */ import { describe, expect, it } from 'vitest' import { - generateExecutionAttachmentKey, - generateExecutionFileKey, + generateLargeValuePayloadKey, + generateUniqueExecutionFileKey, } from '@/lib/uploads/contexts/execution/utils' const context = { @@ -14,25 +14,20 @@ const context = { } describe('execution storage keys', () => { - it('retains deterministic keys for internal execution artifacts', () => { - expect(generateExecutionFileKey(context, 'result.json')).toBe( - 'execution/workspace-1/workflow-1/execution-1/result.json' - ) - expect(generateExecutionFileKey(context, 'result.json')).toBe( - 'execution/workspace-1/workflow-1/execution-1/result.json' - ) + it('retains deterministic keys for large-value payloads', () => { + const key = 'execution/workspace-1/workflow-1/execution-1/large-value-lv_abc123.json' + + expect(generateLargeValuePayloadKey(context, 'lv_abc123')).toBe(key) + expect(generateLargeValuePayloadKey(context, 'lv_abc123')).toBe(key) }) - it('allocates unique create-only keys for duplicate browser attachment names', () => { - const first = generateExecutionAttachmentKey(context, 'report final.pdf') - const second = generateExecutionAttachmentKey(context, 'report final.pdf') + it('allocates unique keys for duplicate file names, keeping the name as the final segment', () => { + const first = generateUniqueExecutionFileKey(context, 'report final.pdf') + const second = generateUniqueExecutionFileKey(context, 'report final.pdf') + const shape = /^execution\/workspace-1\/workflow-1\/execution-1\/[0-9a-f-]+\/report-final\.pdf$/ - expect(first).toMatch( - /^execution\/workspace-1\/workflow-1\/execution-1\/[0-9a-f-]+-report-final\.pdf$/ - ) - expect(second).toMatch( - /^execution\/workspace-1\/workflow-1\/execution-1\/[0-9a-f-]+-report-final\.pdf$/ - ) + expect(first).toMatch(shape) + expect(second).toMatch(shape) expect(first).not.toBe(second) }) }) diff --git a/apps/sim/lib/uploads/contexts/execution/utils.ts b/apps/sim/lib/uploads/contexts/execution/utils.ts index 11b4f04b925..b426d0515b3 100644 --- a/apps/sim/lib/uploads/contexts/execution/utils.ts +++ b/apps/sim/lib/uploads/contexts/execution/utils.ts @@ -13,28 +13,42 @@ export interface ExecutionContext { } /** - * Generate execution-scoped storage key with explicit prefix - * Format: execution/workspace_id/workflow_id/execution_id/filename + * Generate the deterministic storage key for a large-value execution payload. + * Format: execution/workspace_id/workflow_id/execution_id/large-value-.json + * + * Takes the payload id rather than a file name so no user-supplied name can + * reach a key without a uniquifier — that is what silently overwrote same-named + * files before {@link generateUniqueExecutionFileKey} existed. Determinism is + * load-bearing here: the cleanup job matches these keys by LIKE pattern and + * re-storing the same payload must be idempotent. */ -export function generateExecutionFileKey(context: ExecutionContext, fileName: string): string { +export function generateLargeValuePayloadKey(context: ExecutionContext, id: string): string { const { workspaceId, workflowId, executionId } = context - const safeFileName = sanitizeFileName(fileName) + const safeFileName = sanitizeFileName(`large-value-${id}.json`) return `execution/${workspaceId}/${workflowId}/${executionId}/${safeFileName}` } /** - * Generates a unique execution-scoped key for browser attachments. Browser - * uploads are create-only, and a single execution may contain multiple files - * with the same display name. Internal execution artifacts intentionally keep - * using {@link generateExecutionFileKey}'s deterministic replacement semantics. + * Generate a collision-free execution-scoped storage key. + * Format: execution/workspace_id/workflow_id/execution_id/unique_id/filename + * + * One execution routinely carries several files sharing a display name (two + * `image.png` screenshots in one Slack message, repeated tool outputs in a + * loop), which the deterministic key would overwrite. The unique id is its own + * path segment rather than a filename prefix so the last segment stays the + * original name — presigned URLs carry no content-disposition, so that segment + * is what a consumer sees. + * + * Large-value payloads, whose ids are already unique, keep using + * {@link generateLargeValuePayloadKey}. */ -export function generateExecutionAttachmentKey( +export function generateUniqueExecutionFileKey( context: ExecutionContext, fileName: string ): string { const { workspaceId, workflowId, executionId } = context const safeFileName = sanitizeFileName(fileName) - return `execution/${workspaceId}/${workflowId}/${executionId}/${generateId()}-${safeFileName}` + return `execution/${workspaceId}/${workflowId}/${executionId}/${generateId()}/${safeFileName}` } /** @@ -45,8 +59,7 @@ export function generateFileId(): string { } /** - * Check if a key matches execution file pattern - * Execution files have keys in format: execution/workspaceId/workflowId/executionId/filename + * Execution keys: execution/workspaceId/workflowId/executionId/[uniqueId/]filename */ function matchesExecutionFilePattern(key: string): boolean { if (!key || key.startsWith('/api/') || key.startsWith('http')) { @@ -65,7 +78,6 @@ function matchesExecutionFilePattern(key: string): boolean { /** * Check if a file is from execution storage based on its key pattern - * Execution files have keys in format: execution/workspaceId/workflowId/executionId/filename */ export function isExecutionFile(file: UserFile): boolean { if (!file.key) {