Repository navigation
fix(core): clean up temporary directory when background shell execution exits - #29437
Conversation
|
📊 PR Size: size/L
|
GitHub Actions
Error
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 management issue where temporary directories created for background shell processes were not being cleaned up after the processes finished. By transferring ownership of these directories to the ShellExecutionService, the system now ensures that temporary files are reliably removed regardless of whether the process finishes quickly or runs for an extended period. 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
|
![gemini-code-assist[bot]](/web-api/iframe-proxy?url=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 automatic cleanup of temporary directories associated with background shell executions by transferring their ownership to ShellExecutionService, which deletes them upon process exit. Feedback on these changes highlights three key improvements: using synchronous fs.rmSync in test teardown contexts to avoid race conditions, adding a defensive guard in ShellExecutionService.background to prevent directory leaks for untracked processes, and wrapping the backgrounding logic in a try-catch block to ensure proper cleanup if backgrounding fails.
gemini-cli
Bot
added
priority/p1
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces automatic cleanup of temporary directories (tempDir) created during shell execution when a process is moved to the background. Ownership of the temporary directory is transferred from ShellToolInvocation to ShellExecutionService once a process is backgrounded. ShellExecutionService then ensures the directory is deleted when the background process exits, when the service is destroyed, or immediately if the process is untracked or already exited. Corresponding unit and integration tests have been added to verify this behavior. There are no review comments, and I have no additional feedback to provide.
🚨 Action Required: Eval Regressions DetectedModel: The following trustworthy evaluations passed on
The check passed or was cleared for 72 other trustworthy evaluations. 🛠️ Troubleshooting & Fix Instructions1. Ask Gemini CLI to fix it (Recommended)Copy and paste this prompt to the agent: 2. Reproduce LocallyRun the following command to see the failure trajectory: GEMINI_MODEL=gemini-3-flash-preview npm run test:all_evals -- evals/ask_user.eval.ts --testNamePattern="Agent uses AskUser tool to clarify ambiguous requirements"3. Manual FixSee the Fixing Guide for detailed troubleshooting steps. This PR modifies files that affect the model's behavior (prompts, tools, or instructions).
This is an automated guidance message triggered by steering logic signatures. |
GitHub Actions
Error
20f7075
This branch was successfully deployed
2 participants
Ensures temporary directories (
gemini-shell-*) created to store background process ID files (bgpids.tmp) during shell execution are transferred toShellExecutionServicewhen a command runs in the background and automatically removed once the background process completes.Details
ShellExecutionService.background(pid, sessionId, command, tempDir)andShellExecutionConfiginpackages/core/src/services/shellExecutionService.tsto accept an optionaltempDirpath and store it alongside background process metadata (BackgroundProcessRecord.tempDirandbackgroundTempDirs).ShellExecutionService.cleanupLogStream(pid)andShellExecutionService.resetForTest()to remove the associated temporary directory viafsPromises.rm(tempDir, { recursive: true, force: true })when a background process exits or is terminated.ShellToolInvocation.executeinpackages/core/src/tools/shell.tsto passtempDirtoShellExecutionService.backgroundand clean uptempDirin thefinallyblock whenever ownership was not transferred (preserving foreground cleanup and ensuring fast-exitingis_background: truecommands that finish before the promotion delay also clean up their temporary directory).packages/core/src/tools/shell.test.tsto verifytempDirownership transfer toShellExecutionService.backgroundand cleanup when a command completes before the promotion timer elapses.packages/core/src/services/shellExecutionService.test.tsverifyingtempDirstorage and deletion on background process exit.packages/core/src/tools/shellBackgroundTools.integration.test.tsthat starts a short-lived background command, waits for it to exit, and verifies the temporary directory is deleted.Related Issues
Fixes #28392
GH-28392
How to Validate
ShellTool,ShellExecutionService, and background tools:npm test -w @google/gemini-cli-core -- src/tools/shell.test.ts src/services/shellExecutionService.test.ts src/tools/shellBackgroundTools.integration.test.tsPre-Merge Checklist