fix(core): remove temp files on background process exit#28394
fix(core): remove temp files on background process exit#28394Daksh7785 wants to merge 2 commits into
Conversation
Fixes google-gemini#28392 by registering an exit callback on the ShellExecutionService.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
🛑 Action Required: Evaluation ApprovalSteering changes have been detected in this PR. To prevent regressions, a maintainer must approve the evaluation run before this PR can be merged. Maintainers:
Once approved, the evaluation results will be posted here automatically. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a resource leak where temporary directories created for background shell processes were not being removed after process termination. By leveraging the existing onExit lifecycle hook, the system now ensures that these temporary files and directories are properly cleaned up once the background process exits, preventing unnecessary disk usage. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
📊 PR Size: size/S
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces cleanup logic for temporary files and directories when shell commands are executed in the background by registering an onExit handler. The review feedback highlights a potential resource leak if the background process fails to spawn or if an error occurs before the onExit handler is registered, and provides a code suggestion to ensure cleanup is safely handled in the finally block for these failure scenarios.
| if (tempFilePath || tempDir) { | ||
| ShellExecutionService.onExit(pid, () => { | ||
| if (tempFilePath) { | ||
| try { | ||
| fs.unlinkSync(tempFilePath); | ||
| } catch { | ||
| // Ignore | ||
| } | ||
| } | ||
| if (tempDir) { | ||
| try { | ||
| fs.rmSync(tempDir, { recursive: true, force: true }); | ||
| } catch { | ||
| // Ignore | ||
| } | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
There is a potential resource leak if the background process fails to spawn or if an error is thrown before the onExit handler is registered. In those cases, onExit is never registered, and the finally block (lines 1088-1103) will skip cleanup because this.params.is_background is true.
To fix this completely, we should clear tempFilePath and tempDir once the cleanup is successfully deferred to onExit, and update the finally block to clean up any remaining non-empty paths:
// In the finally block (around line 1088):
if (!this.params.is_background || tempFilePath || tempDir) {
if (tempFilePath) {
try {
await fsPromises.unlink(tempFilePath);
} catch {
// Ignore errors during unlink
}
}
if (tempDir) {
try {
await fsPromises.rm(tempDir, { recursive: true, force: true });
} catch {
// Ignore errors during rm
}
}
} if (tempFilePath || tempDir) {
const fileToClean = tempFilePath;
const dirToClean = tempDir;
ShellExecutionService.onExit(pid, () => {
if (fileToClean) {
try {
fs.unlinkSync(fileToClean);
} catch {
// Ignore
}
}
if (dirToClean) {
try {
fs.rmSync(dirToClean, { recursive: true, force: true });
} catch {
// Ignore
}
}
});
tempFilePath = '';
tempDir = '';
}
gemini-cli
Bot
added
the
area/core
1 participant
Fix: Temporary Directory Leak During Background Shell Execution
🐛 Description
This PR fixes a resource leak where the CLI permanently leaves behind temporary directories in the host OS's temp folder whenever a shell command is executed with
is_background: true.The temporary directory (e.g.
gemini-shell-*) is used to storebgpids.tmpfor background process tracking. Previously, cleanup was intentionally skipped because the background process could still be writing to the file, but ownership of the directory was never transferred, leaving it orphaned after the process exited.🛠️ Solution
This change uses the existing
ShellExecutionService.onExitlifecycle hook to perform cleanup when the background process terminates.finallyblock (no behavior change).ShellExecutionService.onExit.This preserves the current background execution behavior while ensuring temporary resources are always cleaned up.
📝 Changes
packages/core/src/tools/shell.tsShellExecutionService.onExitfor background executions.packages/core/src/tools/shell.test.tsShellExecutionService.onExitin unit tests to avoidTypeErrors and verify the new behavior.✅ Verification
npm test -w @google/gemini-cli-core.is_background: true) shell executions register the cleanup callback without affecting existing functionality.🔗 Related Issue
Fixes #28392