From 2f3b704b908d92b6e71092e69eaceed20f34d2f5 Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Sun, 30 Aug 2026 14:03:08 -0700 Subject: [PATCH] fix(sandbox): spend each file ceiling once across every source MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two ceilings on a Function run's sandbox files were charged per source rather than per execution. Mounts: planUserFileMounts assigned a path per element, so one storage key named by two sources became two mounts. `files` is `user-or-llm` and deduped nowhere, so a model repeating an id — or naming a file the code also references with `` — produced a duplicate that cost a presign, a second transfer of identical bytes, and a second charge against both the byte budget and the 20-file mount ceiling, either of which then refuses a request that fits. Collapse by storage key, first occurrence wins. The contract already requires a non-empty key, so there is no keyless case to carry. Exports: MAX_SANDBOX_OUTPUT_FILES is documented as what one execution may export "whether declared by path or discovered by harvesting", and collectExportedFiles already runs the byte ceiling that way. The count ceiling did not, so a request declaring paths and harvesting a directory could export 20 of each. Count declared and discovered together, with a declared path inside the directory dropped from the discovered set so it is not billed on both sides. With no declared paths — every call execute-request makes, since it sets outputSandboxDir only when nothing declares a sandboxPath — the check and its message are unchanged. The resolver's marker reuse is no longer what keeps a twice-referenced file to one mount; its comment said otherwise. Fixture keys in sandbox-mounts.test.ts were identical across files the tests meant to be distinct; they now differ, which is what those tests always claimed to set up. Co-Authored-By: Claude Opus 5 (1M context) --- apps/sim/executor/variables/resolver.ts | 7 +-- .../remote-sandbox/conformance.test.ts | 53 +++++++++++++++++++ .../sim/lib/execution/remote-sandbox/index.ts | 29 +++++----- .../function-execution/sandbox-mounts.test.ts | 44 ++++++++++----- .../lib/function-execution/sandbox-mounts.ts | 24 +++++++-- 5 files changed, 126 insertions(+), 31 deletions(-) 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 } /**