Fix code context state isolation

[ghostwriternr]

Fixes state leakage between code contexts by assigning each context a dedicated executor process from creation to deletion. Removes maximum pool size limits to allow organic scaling.

Changes

• Context-to-executor 1:1 binding with dedicated process lifecycle
• Remove pool size caps for organic scaling
• Add test verifying isolation across 12+ contexts

[changeset-bot]

🦋 Changeset detected

Latest commit: 2feea4e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@cloudflare/sandbox	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[cloudflare-workers-and-pages]

Deploying with    Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status	Name	Latest Commit	Preview URL	Updated (UTC)
✅ Deployment successful! View logs	sandbox-site	8c8999c	Commit Preview URL Branch Preview URL	Nov 25 2025, 09:05 PM

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/cloudflare/sandbox-sdk/@cloudflare/sandbox@249

commit: 2feea4e

[github-actions]

🐳 Docker Image Published

FROM cloudflare/sandbox:0.0.0-pr-249-562e595

Version: 0.0.0-pr-249-562e595

You can use this Docker image with the preview package from this PR.

[claude]

Claude Code Review

The isolation fix architecture is sound and the two-level locking strategy is well-designed. However, there are two critical bugs in the locking implementation that could cause the exact state leakage this PR aims to fix.

Critical Issues (Must Fix Before Merge)

1. Race condition in getExecutorLock() creates duplicate mutexes - Two concurrent executions can create separate mutex instances for the same executor, allowing concurrent execution and defeating the isolation guarantee. See inline comment.

2. Memory leak: Executor locks never cleaned up - The exit handler doesn't remove mutexes from executorLocks map, causing unbounded growth in long-running containers. See inline comment.

Important Issues

3. Test coverage gap - The test at line 582 doesn't actually verify isolation (it only checks for errors). The code would pass even with the bug. See inline comment for better test approach.

4. Documentation needs - Force-delete behavior and unlimited process defaults should be documented prominently to prevent user confusion and resource exhaustion.

Architecture Assessment

The 1:1 context-to-executor binding is the right design for stateful code interpreter use cases. The borrow/return pattern for stateless execution is correct. The per-language pool locks properly protect structural changes. Once the critical locking bugs are fixed, this will be production-ready.

Verdict: Needs fixes before merge - the race condition in executor lock creation can cause state isolation failures.

[claude]

CRITICAL: Race condition in mutex creation

Two concurrent calls with the same executorId can both create separate Mutex instances:

1. Thread A checks executorLocks.get(id) → undefined
2. Thread B checks executorLocks.get(id) → undefined
3. Thread A creates Mutex1, sets it
4. Thread B creates Mutex2, sets it (overwrites Mutex1)
5. Thread A acquires Mutex1, Thread B acquires Mutex2
6. Both execute concurrently on same process → state corruption

This defeats the entire isolation fix.

Fix:

private executorLockCreationMutex = new Mutex();

private async getExecutorLock(executorId: string): Promise<Mutex> {
  let mutex = this.executorLocks.get(executorId);
  if (mutex) return mutex;
  
  return await this.executorLockCreationMutex.runExclusive(() => {
    // Double-check after acquiring lock
    let mutex = this.executorLocks.get(executorId);
    if (!mutex) {
      mutex = new Mutex();
      this.executorLocks.set(executorId, mutex);
    }
    return mutex;
  });
}

Update all call sites from getExecutorLock(id) to await getExecutorLock(id) (lines 219, 227).

[claude]

CRITICAL: Memory leak - executor locks never cleaned up

The exit handler removes the executor from all tracking structures but doesn't remove its mutex from executorLocks. Over time, this map grows unbounded with mutexes for dead processes.

Fix: Add cleanup after removing from available executors:

const available = this.availableExecutors.get(language);
if (available) {
  const index = available.indexOf(interpreterProcess);
  if (index > -1) available.splice(index, 1);
}

// Clean up executor lock
this.executorLocks.delete(interpreterProcess.id);

Note: releaseExecutorForContext at line 578 already has this cleanup, but the exit handler needs it too for cases where processes crash.

[claude]

Test coverage gap: Doesn't verify isolation

This test creates 12 contexts sequentially and executes const value = 2; in each. It only checks that executions succeed without errors.

Problem: The test would pass even if contexts shared state because:

• Each context declares a new const value in its own scope
• JavaScript allows this even if the process is shared
• No verification that state from context N doesn't leak to N+1

To actually test isolation:

for (let i = 0; i < 12; i++) {
  const context = await createContext();
  
  // Set unique value in global scope
  await executeCode(context, `globalThis.testValue_${i} = ${i};`);
  
  // Verify no leakage from previous contexts
  for (let j = 0; j < i; j++) {
    const checkResult = await executeCode(
      context, 
      `typeof globalThis.testValue_${j}`
    );
    expect(checkResult.results[0].text).toContain('undefined');
  }
  
  // Verify own value is set
  const ownResult = await executeCode(
    context,
    `globalThis.testValue_${i}`
  );
  expect(ownResult.results[0].text).toContain(String(i));
  
  await deleteContext(context);
}

This would catch the original bug where contexts shared executors after 10 executions.

[claude]

Memory leak in shutdown

The shutdown() method clears pools but doesn't clear the other tracking maps:

async shutdown(): Promise<void> {
  if (this.cleanupInterval) {
    clearInterval(this.cleanupInterval);
  }
  
  // Kill all processes
  for (const [executor, pool] of this.pools.entries()) {
    for (const process of pool) {
      process.process.kill();
    }
  }
  
  // Clear all tracking structures
  this.pools.clear();
  this.contextExecutors.clear();
  this.availableExecutors.clear();
  this.executorLocks.clear();
  this.poolLocks.clear();
}

Not critical for production (singleton pattern) but causes memory leaks in test suites.