diff --git a/.changeset/fix-executor-mutex.md b/.changeset/fix-executor-mutex.md new file mode 100644 index 00000000..bfc7171f --- /dev/null +++ b/.changeset/fix-executor-mutex.md @@ -0,0 +1,5 @@ +--- +'@cloudflare/sandbox': patch +--- + +Fix executor mutex race condition and memory leak in code interpreter diff --git a/packages/sandbox-container/src/runtime/process-pool.ts b/packages/sandbox-container/src/runtime/process-pool.ts index e4e0f4dc..9092a5ae 100644 --- a/packages/sandbox-container/src/runtime/process-pool.ts +++ b/packages/sandbox-container/src/runtime/process-pool.ts @@ -151,10 +151,9 @@ export class ProcessPoolManager { } private getExecutorLock(executorId: string): Mutex { - let mutex = this.executorLocks.get(executorId); + const mutex = this.executorLocks.get(executorId); if (!mutex) { - mutex = new Mutex(); - this.executorLocks.set(executorId, mutex); + throw new Error(`No mutex found for executor ${executorId}`); } return mutex; } @@ -332,7 +331,9 @@ export class ProcessPoolManager { lastUsed: new Date() }; - // Register exit handler for cleanup (prevents memory leaks) + this.executorLocks.set(id, new Mutex()); + + // Register exit handler for cleanup const exitHandler = ( code: number | null, signal: NodeJS.Signals | null @@ -361,6 +362,8 @@ export class ProcessPoolManager { const index = available.indexOf(interpreterProcess); if (index > -1) available.splice(index, 1); } + + this.executorLocks.delete(id); }; interpreterProcess.exitHandler = exitHandler; @@ -557,7 +560,7 @@ export class ProcessPoolManager { // Remove from context ownership map this.contextExecutors.delete(contextId); - // Remove exit handler to prevent memory leak + // Remove exit handler since we're doing manual cleanup if (executor.exitHandler) { executor.process.removeListener('exit', executor.exitHandler); } @@ -668,6 +671,9 @@ export class ProcessPoolManager { process.process.kill(); available.splice(i, 1); + // Clean up executor lock + this.executorLocks.delete(process.id); + // Also remove from main pool const pool = this.pools.get(language); if (pool) { @@ -754,6 +760,7 @@ export class ProcessPoolManager { } this.pools.clear(); + this.executorLocks.clear(); } } diff --git a/tests/e2e/code-interpreter-workflow.test.ts b/tests/e2e/code-interpreter-workflow.test.ts index 8e5da7ea..7bb978dc 100644 --- a/tests/e2e/code-interpreter-workflow.test.ts +++ b/tests/e2e/code-interpreter-workflow.test.ts @@ -583,7 +583,8 @@ console.log('Sum:', sum); currentSandboxId = createSandboxId(); const headers = createTestHeaders(currentSandboxId); - // Create 12 contexts and execute same code that declares a variable + // Create 12 contexts + const contexts: CodeContext[] = []; for (let i = 0; i < 12; i++) { const ctxResponse = await fetch(`${workerUrl}/api/code/context/create`, { method: 'POST', @@ -593,13 +594,17 @@ console.log('Sum:', sum); expect(ctxResponse.status).toBe(200); const context = (await ctxResponse.json()) as CodeContext; + contexts.push(context); + } + // Set unique state in each context using mutable global + for (let i = 0; i < contexts.length; i++) { const execResponse = await fetch(`${workerUrl}/api/code/execute`, { method: 'POST', headers, body: JSON.stringify({ - code: 'const value = 2;', - options: { context } + code: `globalThis.contextValue = ${i}; contextValue;`, + options: { context: contexts[i] } }) }); @@ -607,10 +612,36 @@ console.log('Sum:', sum); const execution = (await execResponse.json()) as ExecutionResult; expect( execution.error, - `Context ${i + 1} should not have error` + `Context ${i} should not have error setting value` ).toBeUndefined(); + expect(execution.results![0].text).toContain(String(i)); + } - // Clean up immediately + // Verify each context still has its isolated state + for (let i = 0; i < contexts.length; i++) { + const execResponse = await fetch(`${workerUrl}/api/code/execute`, { + method: 'POST', + headers, + body: JSON.stringify({ + code: 'contextValue;', + options: { context: contexts[i] } + }) + }); + + expect(execResponse.status).toBe(200); + const execution = (await execResponse.json()) as ExecutionResult; + expect( + execution.error, + `Context ${i} should not have error reading value` + ).toBeUndefined(); + expect( + execution.results![0].text, + `Context ${i} should have its unique value ${i}` + ).toContain(String(i)); + } + + // Clean up all contexts + for (const context of contexts) { await fetch(`${workerUrl}/api/code/context/${context.id}`, { method: 'DELETE', headers