diff --git a/apps/sim/executor/variables/resolver.ts b/apps/sim/executor/variables/resolver.ts index cc7838bcf2d..94c0bee918e 100644 --- a/apps/sim/executor/variables/resolver.ts +++ b/apps/sim/executor/variables/resolver.ts @@ -694,9 +694,10 @@ export class VariableResolver { return null } - // Reuse an existing marker for the same file so referencing one path twice - // mounts it once, rather than transferring a second copy under a - // collision-suffixed name and spending the mount budget twice. + // Reuse the marker already standing for this file so a path referenced twice + // costs one context variable rather than two. What keeps it to one mount is + // `planUserFileMounts`, which collapses by storage key across every source — + // this only keeps the duplicate out of the request body. const existing = Object.entries(contextVarAccumulator).find( ([, value]) => isSandboxFileMountRef(value) && value.file.key === file.key ) diff --git a/apps/sim/lib/execution/remote-sandbox/conformance.test.ts b/apps/sim/lib/execution/remote-sandbox/conformance.test.ts index 9bf4f80649d..e6f915f7dc3 100644 --- a/apps/sim/lib/execution/remote-sandbox/conformance.test.ts +++ b/apps/sim/lib/execution/remote-sandbox/conformance.test.ts @@ -868,6 +868,59 @@ describe.each(PROVIDERS)('sandbox conformance [%s]', (provider) => { ).rejects.toThrow(/over the 20-file export limit/) }) + it('spends one file ceiling across declared and harvested outputs', async () => { + // The limit is what an execution exports, not what one directory holds, so a + // request that both declares and harvests cannot take 20 of each. + stubCodeRun(provider, `__SIM_RESULT__=${JSON.stringify('done')}`) + stubOutputFileSizes(provider, 1, 1) + stubOutputDirListing( + Array.from({ length: MAX_SANDBOX_OUTPUT_FILES - 1 }, (_, index) => ({ + path: `/tmp/sim/outputs/file-${index}.txt`, + size: 1, + })) + ) + + await expect( + executeInSandbox({ + code: 'x', + language: CodeLanguage.Python, + timeoutMs: 1000, + outputSandboxPaths: ['/out/first.txt', '/out/second.txt'], + outputSandboxDir: '/tmp/sim/outputs', + }) + ).rejects.toThrow(/produced 21 files .* over the 20-file export limit/) + }) + + it('does not charge a declared path inside the harvest directory to the ceiling twice', async () => { + // The directory holds exactly the limit and the request names one of those + // files. Charging it on both sides would refuse a run exporting 20 files. + stubCodeRun(provider, `__SIM_RESULT__=${JSON.stringify('done')}`) + // One inspection for the declared path, then one per file actually read. + stubOutputFileSizes(provider, ...Array.from({ length: MAX_SANDBOX_OUTPUT_FILES + 1 }, () => 1)) + stubOutputDirListing( + Array.from({ length: MAX_SANDBOX_OUTPUT_FILES }, (_, index) => ({ + path: `/tmp/sim/outputs/file-${index}.txt`, + size: 1, + })) + ) + for (let index = 0; index < MAX_SANDBOX_OUTPUT_FILES; index += 1) { + stubOutputFileRead(provider, 'x') + } + + const result = await executeInSandbox({ + code: 'x', + language: CodeLanguage.Python, + timeoutMs: 1000, + outputSandboxPath: '/tmp/sim/outputs/file-0.txt', + outputSandboxDir: '/tmp/sim/outputs', + }) + + // Exported once as a declared path, rather than a second time as a harvest. + expect(Object.keys(result.exportedFiles ?? {})).toEqual(['/tmp/sim/outputs/file-0.txt']) + expect(result.collectedFiles).toHaveLength(MAX_SANDBOX_OUTPUT_FILES - 1) + expect(result.collectedFiles?.map((file) => file.relativePath)).not.toContain('file-0.txt') + }) + it('does not list the output directory when no harvest was requested', async () => { stubCodeRun(provider, `__SIM_RESULT__=${JSON.stringify('done')}`) diff --git a/apps/sim/lib/execution/remote-sandbox/index.ts b/apps/sim/lib/execution/remote-sandbox/index.ts index ec5a82cb1e0..3b4e92015b8 100644 --- a/apps/sim/lib/execution/remote-sandbox/index.ts +++ b/apps/sim/lib/execution/remote-sandbox/index.ts @@ -554,10 +554,16 @@ function requestedOutputSandboxPaths(req: { * too many files, or nesting past what the listing reaches — before a single * byte is read. Sorted so a multi-file result is stable run to run rather than * inheriting whatever order the provider happened to return. + * + * `declaredPaths` are the files the request already named. One sitting inside the + * directory is dropped rather than harvested a second time, and the rest count + * toward the ceiling: the limit is what one execution exports, not what one + * directory holds, so declaring and harvesting cannot spend it twice. */ async function listOutputDirectoryFiles( sandbox: SandboxHandle, outputSandboxDir: string, + declaredPaths: ReadonlySet, signal: AbortSignal ): Promise { let listed: SandboxDirectoryEntry[] @@ -593,9 +599,10 @@ async function listOutputDirectoryFiles( ) } - const files = entries.filter((entry) => entry.kind === 'file') - if (files.length > MAX_SANDBOX_OUTPUT_FILES) { - throw new SandboxOutputFileCountError(files.length, outputSandboxDir) + const files = entries.filter((entry) => entry.kind === 'file' && !declaredPaths.has(entry.path)) + const exported = declaredPaths.size + files.length + if (exported > MAX_SANDBOX_OUTPUT_FILES) { + throw new SandboxOutputFileCountError(exported, outputSandboxDir) } return files.sort((a, b) => a.path.localeCompare(b.path)) } @@ -640,16 +647,14 @@ async function collectExportedFiles( } // Sized into the same running total as the declared paths, so an execution - // cannot spend the ceiling twice by both declaring and harvesting. A declared - // path that happens to sit inside the harvest directory is dropped from the - // discovered set rather than counted again — double-billing it would reject a - // single output larger than half the ceiling as oversized. + // cannot spend the byte ceiling twice by both declaring and harvesting. The + // listing applies the same rule to the file-count ceiling and drops a declared + // path that happens to sit inside the harvest directory — double-billing it + // would reject a single output larger than half the ceiling as oversized. const declaredPaths = new Set(readablePaths) - const discovered = ( - req.outputSandboxDir - ? await listOutputDirectoryFiles(sandbox, req.outputSandboxDir, options.signal) - : [] - ).filter((entry) => !declaredPaths.has(entry.path)) + const discovered = req.outputSandboxDir + ? await listOutputDirectoryFiles(sandbox, req.outputSandboxDir, declaredPaths, options.signal) + : [] for (const entry of discovered) { totalOutputBytes += entry.size if (totalOutputBytes > MAX_SANDBOX_OUTPUT_BYTES) { diff --git a/apps/sim/lib/function-execution/sandbox-mounts.test.ts b/apps/sim/lib/function-execution/sandbox-mounts.test.ts index dffc26ead64..2f49c989885 100644 --- a/apps/sim/lib/function-execution/sandbox-mounts.test.ts +++ b/apps/sim/lib/function-execution/sandbox-mounts.test.ts @@ -88,7 +88,7 @@ describe('planUserFileMounts', () => { it('cannot be escaped by a traversal in the file name', () => { const planned = planUserFileMounts([ executionFile({ name: '../../etc/passwd' }), - executionFile({ id: 'file_2', name: '..' }), + executionFile({ id: 'file_2', key: 'execution/other', name: '..' }), ]) for (const { mountPath } of planned) { @@ -100,9 +100,9 @@ describe('planUserFileMounts', () => { it('suffixes colliding names so neither file is silently overwritten', () => { const planned = planUserFileMounts([ - executionFile({ id: 'file_1', name: 'report.csv' }), - executionFile({ id: 'file_2', name: 'report.csv' }), - executionFile({ id: 'file_3', name: 'report.csv' }), + executionFile({ id: 'file_1', key: 'execution/a/report.csv', name: 'report.csv' }), + executionFile({ id: 'file_2', key: 'execution/b/report.csv', name: 'report.csv' }), + executionFile({ id: 'file_3', key: 'execution/c/report.csv', name: 'report.csv' }), ]) expect(planned.map((entry) => entry.mountPath)).toEqual([ @@ -111,6 +111,24 @@ describe('planUserFileMounts', () => { '/tmp/sim/inputs/report-3.csv', ]) }) + + it('mounts one storage key once however many sources named it', () => { + // A caller listing the same file twice, and a `` marker for + // a file the caller also passed explicitly, both land in one list here. A + // second copy of identical bytes costs a presign and a duplicate transfer, + // and charges the byte budget and the 20-file ceiling twice over. + const planned = planUserFileMounts([ + executionFile({ id: 'file_1', name: 'report.csv' }), + executionFile({ id: 'file_1_again', name: 'report.csv' }), + executionFile({ id: 'file_2', name: 'renamed.csv' }), + workspaceFile(), + ]) + + expect(planned.map((entry) => entry.mountPath)).toEqual([ + '/tmp/sim/inputs/report.csv', + '/tmp/sim/inputs/brief.pdf', + ]) + }) }) describe('resolveUserFileMounts', () => { @@ -193,14 +211,16 @@ describe('resolveUserFileMounts', () => { await expect( resolveUserFileMounts({ - planned: planUserFileMounts([ - executionFile({ id: 'a', name: 'a.bin', size: 9 * 1024 * 1024 }), - executionFile({ id: 'b', name: 'b.bin', size: 9 * 1024 * 1024 }), - executionFile({ id: 'c', name: 'c.bin', size: 9 * 1024 * 1024 }), - executionFile({ id: 'd', name: 'd.bin', size: 9 * 1024 * 1024 }), - executionFile({ id: 'e', name: 'e.bin', size: 9 * 1024 * 1024 }), - executionFile({ id: 'f', name: 'f.bin', size: 9 * 1024 * 1024 }), - ]), + planned: planUserFileMounts( + ['a', 'b', 'c', 'd', 'e', 'f'].map((id) => + executionFile({ + id, + key: `execution/${WORKSPACE_ID}/${WORKFLOW_ID}/${EXECUTION_ID}/${id}/${id}.bin`, + name: `${id}.bin`, + size: 9 * 1024 * 1024, + }) + ) + ), context: executionContext, }) ).rejects.toThrow(/total mount limit/) diff --git a/apps/sim/lib/function-execution/sandbox-mounts.ts b/apps/sim/lib/function-execution/sandbox-mounts.ts index 678fac8c931..cbad1fa2859 100644 --- a/apps/sim/lib/function-execution/sandbox-mounts.ts +++ b/apps/sim/lib/function-execution/sandbox-mounts.ts @@ -227,16 +227,32 @@ function uniqueMountFileName(name: string, used: Set): string { * Assigns each file a deterministic mount path. Pure and I/O-free, so a caller * can decide whether an execution needs a sandbox filesystem before spending a * presign or a byte of transfer on a request that may still be refused. + * + * A storage key mounts once. The same object arrives from independent sources — + * a caller listing it twice, or listing one the code also asked for with + * `` — and a second copy of identical bytes costs a presign, a + * duplicate transfer, and a second charge against both the byte budget and the + * per-request file ceiling. First occurrence wins, so the name listed first is + * the one the code sees. */ export function planUserFileMounts( files: readonly UserFile[], mountDir: string = SANDBOX_INPUT_DIR ): PlannedUserFileMount[] { const used = new Set() - return files.map((userFile) => ({ - userFile, - mountPath: `${mountDir}/${uniqueMountFileName(userFile.name, used)}`, - })) + const mountedKeys = new Set() + const planned: PlannedUserFileMount[] = [] + + for (const userFile of files) { + if (mountedKeys.has(userFile.key)) continue + mountedKeys.add(userFile.key) + planned.push({ + userFile, + mountPath: `${mountDir}/${uniqueMountFileName(userFile.name, used)}`, + }) + } + + return planned } /**