From 7d946c3031b873120d8cdf7a020fcaab40472895 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Braulio=20Gonz=C3=A1lez=20Valido?= Date: Wed, 1 Jul 2026 13:53:17 +0100 Subject: [PATCH] fix(ai-builder): Prevent snapshot staging races during concurrent sandbox creation (#33342) Co-authored-by: Claude Opus 4.8 (1M context) --- .../__tests__/snapshot-image-context.test.ts | 37 +++++++---- .../src/workspace/snapshot-image-context.ts | 64 +++++++++++++------ .../src/workspace/snapshot-manager.ts | 16 ++--- 3 files changed, 75 insertions(+), 42 deletions(-) diff --git a/packages/@n8n/instance-ai/src/workspace/__tests__/snapshot-image-context.test.ts b/packages/@n8n/instance-ai/src/workspace/__tests__/snapshot-image-context.test.ts index 3415ddb19c6..aa829abcabf 100644 --- a/packages/@n8n/instance-ai/src/workspace/__tests__/snapshot-image-context.test.ts +++ b/packages/@n8n/instance-ai/src/workspace/__tests__/snapshot-image-context.test.ts @@ -38,7 +38,8 @@ describe('snapshot-image-context', () => { ).resolves.toBe('# Schedule'); }); - it('reuses a hash-keyed staging directory for the same cache key', async () => { + it('stages once per cache key and reuses the directory read-only', async () => { + // Same key ⇒ same content in prod, so the repeat reuses the first staging (filesB ignored). const filesA = new Map([[`${WORKSPACE_ROOT}/package.json`, '{"name":"a"}']]); const filesB = new Map([[`${WORKSPACE_ROOT}/package.json`, '{"name":"b"}']]); const cacheKey = 'aaaaaaaaaaaa-bbbbbbbbbbbb'; @@ -49,24 +50,36 @@ describe('snapshot-image-context', () => { expect(second.stagingDir).toBe(first.stagingDir); await expect(readFile(join(first.stagingDir, 'package.json'), 'utf-8')).resolves.toBe( - '{"name":"b"}', + '{"name":"a"}', ); }); - it('removes stale files when reusing a cache-keyed staging directory', async () => { + it('serves concurrent stagings for the same cache key from a single directory', async () => { const cacheKey = 'cccccccccccc-dddddddddddd'; - const filesWithExtra = new Map([ - [`${WORKSPACE_ROOT}/package.json`, '{"name":"a"}'], - [`${WORKSPACE_ROOT}/skills/removed/SKILL.md`, '# Removed'], + const files = new Map([ + [`${WORKSPACE_ROOT}/package.json`, '{"name":"concurrent"}'], + [`${WORKSPACE_ROOT}/skills/post-build-flow/SKILL.md`, '# Post build'], + [`${WORKSPACE_ROOT}/knowledge-base/templates/example.ts`, 'export const x = 1;'], ]); - const filesWithoutExtra = new Map([[`${WORKSPACE_ROOT}/package.json`, '{"name":"b"}']]); - const first = await stageWorkspaceFilesForImage(filesWithExtra, WORKSPACE_ROOT, cacheKey); - const second = await stageWorkspaceFilesForImage(filesWithoutExtra, WORKSPACE_ROOT, cacheKey); - tempDirs.push(first.stagingDir); + const results = await Promise.all( + Array.from( + { length: 8 }, + async () => await stageWorkspaceFilesForImage(files, WORKSPACE_ROOT, cacheKey), + ), + ); + tempDirs.push(results[0].stagingDir); - expect(second.stagingDir).toBe(first.stagingDir); - await expect(access(join(first.stagingDir, 'skills/removed/SKILL.md'))).rejects.toThrow(); + // All callers share one directory and every file is intact — no rm clobbered a write. + for (const result of results) { + expect(result.stagingDir).toBe(results[0].stagingDir); + } + await expect( + readFile(join(results[0].stagingDir, 'skills/post-build-flow/SKILL.md'), 'utf-8'), + ).resolves.toBe('# Post build'); + await expect( + readFile(join(results[0].stagingDir, 'knowledge-base/templates/example.ts'), 'utf-8'), + ).resolves.toBe('export const x = 1;'); }); it('disposeSnapshotImageContext removes the staging directory', async () => { diff --git a/packages/@n8n/instance-ai/src/workspace/snapshot-image-context.ts b/packages/@n8n/instance-ai/src/workspace/snapshot-image-context.ts index bc589eef5cd..8aac827cc9a 100644 --- a/packages/@n8n/instance-ai/src/workspace/snapshot-image-context.ts +++ b/packages/@n8n/instance-ai/src/workspace/snapshot-image-context.ts @@ -8,6 +8,13 @@ export interface SnapshotImageContext { stagingDir: string; } +/** + * Dedupe staging by key: the destructive `rm`+`mkdir`+`writeFile` runs once per + * key, and concurrent callers await it and reuse the directory read-only. Same key + * ⇒ same content, so reuse both avoids re-writing and prevents rm/write races. + */ +const stagingByCacheKey = new Map>(); + function workspaceRelativePath(filePath: string, workspaceRoot: string): string { const root = workspaceRoot.endsWith('/') ? workspaceRoot : `${workspaceRoot}/`; @@ -20,35 +27,52 @@ function workspaceRelativePath(filePath: string, workspaceRoot: string): string return filePath.slice(root.length); } -/** - * Writes sandbox workspace files to a host directory for Daytona `addLocalDir` / COPY. - * When `cacheKey` is set, reuses `os.tmpdir()/n8n-snapshot-context-` so repeated - * image builds do not accumulate temp directories. - */ -export async function stageWorkspaceFilesForImage( +async function writeStagedFiles( + stagingDir: string, files: Map, workspaceRoot: string, - cacheKey?: string, -): Promise { - let stagingDir: string; - - if (cacheKey) { - stagingDir = join(tmpdir(), `${SNAPSHOT_CONTEXT_PREFIX}${cacheKey}`); - - await rm(stagingDir, { recursive: true, force: true }); - await mkdir(stagingDir, { recursive: true }); - } else { - stagingDir = await mkdtemp(join(tmpdir(), `${SNAPSHOT_CONTEXT_PREFIX}temp-`)); - } - +): Promise { for (const [filePath, content] of files) { const targetPath = join(stagingDir, workspaceRelativePath(filePath, workspaceRoot)); await mkdir(dirname(targetPath), { recursive: true }); await writeFile(targetPath, content, 'utf-8'); } +} - return { stagingDir }; +/** + * Writes sandbox workspace files to a host directory for Daytona `addLocalDir` / COPY. + * With a `cacheKey`, stages into `os.tmpdir()/n8n-snapshot-context-` once per key + * (see `stagingByCacheKey`) so repeated and concurrent builds share one directory. + */ +export async function stageWorkspaceFilesForImage( + files: Map, + workspaceRoot: string, + cacheKey?: string, +): Promise { + if (!cacheKey) { + const stagingDir = await mkdtemp(join(tmpdir(), `${SNAPSHOT_CONTEXT_PREFIX}temp-`)); + await writeStagedFiles(stagingDir, files, workspaceRoot); + return { stagingDir }; + } + + const inFlightOrDone = stagingByCacheKey.get(cacheKey); + if (inFlightOrDone) return await inFlightOrDone; + + const staging = (async () => { + const stagingDir = join(tmpdir(), `${SNAPSHOT_CONTEXT_PREFIX}${cacheKey}`); + await rm(stagingDir, { recursive: true, force: true }); + await mkdir(stagingDir, { recursive: true }); + await writeStagedFiles(stagingDir, files, workspaceRoot); + return { stagingDir }; + })().catch((error) => { + // Evict a rejected staging so the next caller retries. + stagingByCacheKey.delete(cacheKey); + throw error; + }); + + stagingByCacheKey.set(cacheKey, staging); + return await staging; } export async function disposeSnapshotImageContext(stagingDir: string): Promise { diff --git a/packages/@n8n/instance-ai/src/workspace/snapshot-manager.ts b/packages/@n8n/instance-ai/src/workspace/snapshot-manager.ts index 2b326e122ed..4706783584c 100644 --- a/packages/@n8n/instance-ai/src/workspace/snapshot-manager.ts +++ b/packages/@n8n/instance-ai/src/workspace/snapshot-manager.ts @@ -26,7 +26,7 @@ import { builderTemplatesOptionsFromEnv, } from './builder-templates-service'; import { PACKAGE_JSON, TSCONFIG_JSON, BUILD_MJS } from './sandbox-setup'; -import { disposeSnapshotImageContext, stageWorkspaceFilesForImage } from './snapshot-image-context'; +import { stageWorkspaceFilesForImage } from './snapshot-image-context'; import { buildRuntimeSkillWorkspaceBundle } from '../skills/materialize-runtime-skills'; import { loadInstanceAiRuntimeSkillSource } from '../skills/runtime-skills'; @@ -52,8 +52,6 @@ export class SnapshotManager { private knowledgeBaseBundlePromise: Promise | null = null; - private stagingDir: string | null = null; - constructor( private readonly baseImage: string | undefined, private readonly logger: Logger, @@ -88,7 +86,6 @@ export class SnapshotManager { DAYTONA_WORKSPACE_ROOT, cacheKey, ); - this.stagingDir = stagingDir; const { Image } = loadDaytona(); const layoutDirs = SNAPSHOT_WORKSPACE_LAYOUT_DIRS.map( @@ -179,15 +176,14 @@ export class SnapshotManager { }); } - /** Invalidate cached image (e.g., when base image changes). */ + /** + * Invalidate the in-memory image/bundle caches. The shared staging dir is owned + * by the per-key cache in `stageWorkspaceFilesForImage` and intentionally not + * removed here (in-flight creations may still be reading it). + */ invalidate(): void { - const stagingDir = this.stagingDir; this.cachedImage = null; this.runtimeSkillBundlePromise = null; this.knowledgeBaseBundlePromise = null; - this.stagingDir = null; - if (stagingDir) { - void disposeSnapshotImageContext(stagingDir); - } } }