Skip to content

fix(core): ensure robust sandbox cleanup in all process execution paths #24763

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
6 commits merged into from
Apr 8, 2026
Merged

fix(core): ensure robust sandbox cleanup in all process execution paths #24763

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8afca80
fix(core): ensure sandbox cleanup in all process execution paths
emilyhedlund Apr 6, 2026
ca86cfb
refactor(core): use try...finally for sandbox cleanup robustness
emilyhedlund Apr 6, 2026
20b5bca
refactor(core): use try...finally in shellExecutionService for cleanup
emilyhedlund Apr 6, 2026
ce17efa
fix(core): revert try...finally for async shellExecution methods
emilyhedlund Apr 6, 2026
fa2903d
chore(cli): revert unintended snapshot modifications
emilyhedlund Apr 6, 2026
09a5c63
Merge branch 'main' into fix-memory-leaks-tests
spencer426 Apr 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
174 changes: 91 additions & 83 deletions packages/core/src/services/sandboxedFileSystemService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,52 +59,56 @@ export class SandboxedFileSystemService implements FileSystemService {
},
});

return new Promise((resolve, reject) => {
// Direct spawn is necessary here for streaming large file contents.

const child = spawn(prepared.program, prepared.args, {
cwd: this.cwd,
env: prepared.env,
});

let output = '';
let error = '';

child.stdout?.on('data', (data) => {
output += data.toString();
});

child.stderr?.on('data', (data) => {
error += data.toString();
});

child.on('close', (code) => {
if (code === 0) {
resolve(output);
} else {
const isEnoent =
error.toLowerCase().includes('no such file or directory') ||
error.toLowerCase().includes('enoent') ||
error.toLowerCase().includes('could not find file') ||
error.toLowerCase().includes('could not find a part of the path');
const err = new Error(
`Sandbox Error: read_file failed for '${filePath}'. Exit code ${code}. ${error ? 'Details: ' + error : ''}`,
);
if (isEnoent) {
Object.assign(err, { code: 'ENOENT' });
try {
return await new Promise((resolve, reject) => {
// Direct spawn is necessary here for streaming large file contents.

const child = spawn(prepared.program, prepared.args, {
Comment thread
This conversation was marked as resolved.
cwd: this.cwd,
env: prepared.env,
});

let output = '';
let error = '';

child.stdout?.on('data', (data) => {
output += data.toString();
});

child.stderr?.on('data', (data) => {
error += data.toString();
});

child.on('close', (code) => {
if (code === 0) {
resolve(output);
} else {
const isEnoent =
error.toLowerCase().includes('no such file or directory') ||
error.toLowerCase().includes('enoent') ||
error.toLowerCase().includes('could not find file') ||
error.toLowerCase().includes('could not find a part of the path');
const err = new Error(
`Sandbox Error: read_file failed for '${filePath}'. Exit code ${code}. ${error ? 'Details: ' + error : ''}`,
);
if (isEnoent) {
Object.assign(err, { code: 'ENOENT' });
}
reject(err);
}
reject(err);
}
});
});

child.on('error', (err) => {
reject(
new Error(
`Sandbox Error: Failed to spawn read_file for '${filePath}': ${err.message}`,
),
);
child.on('error', (err) => {
reject(
new Error(
`Sandbox Error: Failed to spawn read_file for '${filePath}': ${err.message}`,
),
);
});
});
});
} finally {
prepared.cleanup?.();
}
}

async writeTextFile(filePath: string, content: string): Promise<void> {
Expand All @@ -124,53 +128,57 @@ export class SandboxedFileSystemService implements FileSystemService {
},
});

return new Promise((resolve, reject) => {
// Direct spawn is necessary here for streaming large file contents.
try {
return await new Promise((resolve, reject) => {
// Direct spawn is necessary here for streaming large file contents.

const child = spawn(prepared.program, prepared.args, {
cwd: this.cwd,
env: prepared.env,
});
const child = spawn(prepared.program, prepared.args, {
cwd: this.cwd,
env: prepared.env,
});

child.stdin?.on('error', (err) => {
// Silently ignore EPIPE errors on stdin, they will be caught by the process error/close listeners
if (isNodeError(err) && err.code === 'EPIPE') {
return;
}
debugLogger.error(
`Sandbox Error: stdin error for '${filePath}': ${
err instanceof Error ? err.message : String(err)
}`,
);
});

child.stdin?.write(content);
child.stdin?.end();

let error = '';
child.stderr?.on('data', (data) => {
error += data.toString();
});
child.stdin?.on('error', (err) => {
// Silently ignore EPIPE errors on stdin, they will be caught by the process error/close listeners
if (isNodeError(err) && err.code === 'EPIPE') {
return;
}
debugLogger.error(
`Sandbox Error: stdin error for '${filePath}': ${
err instanceof Error ? err.message : String(err)
}`,
);
});

child.stdin?.write(content);
child.stdin?.end();

let error = '';
child.stderr?.on('data', (data) => {
error += data.toString();
});

child.on('close', (code) => {
if (code === 0) {
resolve();
} else {
reject(
new Error(
`Sandbox Error: write_file failed for '${filePath}'. Exit code ${code}. ${error ? 'Details: ' + error : ''}`,
),
);
}
});

child.on('close', (code) => {
if (code === 0) {
resolve();
} else {
child.on('error', (err) => {
reject(
new Error(
`Sandbox Error: write_file failed for '${filePath}'. Exit code ${code}. ${error ? 'Details: ' + error : ''}`,
`Sandbox Error: Failed to spawn write_file for '${filePath}': ${err.message}`,
),
);
}
});
});

child.on('error', (err) => {
reject(
new Error(
`Sandbox Error: Failed to spawn write_file for '${filePath}': ${err.message}`,
),
);
});
});
} finally {
prepared.cleanup?.();
}
}
}
38 changes: 22 additions & 16 deletions packages/core/src/services/shellExecutionService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,21 +510,24 @@ export class ShellExecutionService {
shellExecutionConfig: ShellExecutionConfig,
isInteractive: boolean,
): Promise<ShellExecutionHandle> {
let cmdCleanup: (() => void) | undefined;
try {
const isWindows = os.platform() === 'win32';

const {
program: finalExecutable,
args: finalArgs,
env: finalEnv,
cwd: finalCwd,
cleanup: cmdCleanup,
} = await this.prepareExecution(
const prepared = await this.prepareExecution(
commandToExecute,
cwd,
shellExecutionConfig,
isInteractive,
);
cmdCleanup = prepared.cleanup;

const {
program: finalExecutable,
args: finalArgs,
env: finalEnv,
cwd: finalCwd,
} = prepared;

const child = cpSpawn(finalExecutable, finalArgs, {
cwd: finalCwd,
Expand Down Expand Up @@ -811,6 +814,7 @@ export class ShellExecutionService {
} catch (e) {
// eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion
const error = e as Error;
cmdCleanup?.();
return {
pid: undefined,
result: Promise.resolve({
Expand All @@ -826,7 +830,6 @@ export class ShellExecutionService {
};
}
}

private static async executeWithPty(
commandToExecute: string,
cwd: string,
Expand All @@ -840,23 +843,26 @@ export class ShellExecutionService {
throw new Error('PTY implementation not found');
}
let spawnedPty: IPty | undefined;
let cmdCleanup: (() => void) | undefined;

try {
const cols = shellExecutionConfig.terminalWidth ?? 80;
const rows = shellExecutionConfig.terminalHeight ?? 30;

const {
program: finalExecutable,
args: finalArgs,
env: finalEnv,
cwd: finalCwd,
cleanup: cmdCleanup,
} = await this.prepareExecution(
const prepared = await this.prepareExecution(
commandToExecute,
cwd,
shellExecutionConfig,
true,
);
cmdCleanup = prepared.cleanup;

const {
program: finalExecutable,
args: finalArgs,
env: finalEnv,
cwd: finalCwd,
} = prepared;

// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
const ptyProcess = ptyInfo.module.spawn(finalExecutable, finalArgs, {
Expand Down Expand Up @@ -1237,6 +1243,7 @@ export class ShellExecutionService {
} catch (e) {
// eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion
const error = e as Error;
cmdCleanup?.();

if (spawnedPty) {
try {
Expand Down Expand Up @@ -1270,7 +1277,6 @@ export class ShellExecutionService {
}
}
}

/**
* Writes a string to the pseudo-terminal (PTY) of a running process.
*
Expand Down
36 changes: 22 additions & 14 deletions packages/core/src/tools/grep.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,7 @@ class GrepToolInvocation extends BaseToolInvocation<
let finalCommand = checkCommand;
let finalArgs = checkArgs;
let finalEnv = process.env;
let cleanup: (() => void) | undefined;

if (sandboxManager) {
try {
Expand All @@ -338,6 +339,7 @@ class GrepToolInvocation extends BaseToolInvocation<
finalCommand = prepared.program;
finalArgs = prepared.args;
finalEnv = prepared.env;
cleanup = prepared.cleanup;
} catch (err) {
debugLogger.debug(
`[GrepTool] Sandbox preparation failed for '${command}':`,
Expand All @@ -346,21 +348,27 @@ class GrepToolInvocation extends BaseToolInvocation<
}
}

return await new Promise((resolve) => {
const child = spawn(finalCommand, finalArgs, {
stdio: 'ignore',
shell: true,
env: finalEnv,
});
child.on('close', (code) => resolve(code === 0));
child.on('error', (err) => {
debugLogger.debug(
`[GrepTool] Failed to start process for '${command}':`,
err.message,
);
resolve(false);
try {
return await new Promise((resolve) => {
const child = spawn(finalCommand, finalArgs, {
stdio: 'ignore',
shell: true,
env: finalEnv,
});
child.on('close', (code) => {
resolve(code === 0);
});
child.on('error', (err) => {
debugLogger.debug(
`[GrepTool] Failed to start process for '${command}':`,
err.message,
);
resolve(false);
});
});
});
} finally {
cleanup?.();
}
} catch {
return false;
}
Expand Down
Loading
Loading