Fix executor mutex race condition and memory leak (#258)

* Fix executor mutex race condition and memory leak

* Add changeset

* Fix missing mutex cleanup in idle and shutdown paths

Added executorLocks.delete() calls in cleanupIdleProcesses() and
shutdown() to prevent memory leaks. Also clarified comment in
releaseExecutorForContext().

Co-authored-by: Naresh <ghostwriternr@users.noreply.github.com>

---------

Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Co-authored-by: Naresh <ghostwriternr@users.noreply.github.com>

Author: ghostwriternr

.changeset/fix-executor-mutex.md

@@ -0,0 +1,5 @@
+---
+'@cloudflare/sandbox': patch
+---
+
+Fix executor mutex race condition and memory leak in code interpreter

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();
   }
 }
 

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,24 +594,54 @@ 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] }
         })
       });
 
       expect(execResponse.status).toBe(200);
       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