feat(core): improve shell execution service reliability #10607
Conversation
This commit enhances the ShellExecutionService with several key improvements:
- Implements a graceful shutdown sequence on command abortion, sending SIGTERM followed by SIGKILL to allow processes to terminate cleanly.
- Ensures immediate promise resolution upon abortion, preventing the CLI from hanging while processing buffered output.
- Introduces a sliding window buffer for the child_process fallback to cap memory usage at 16MB for stdout and stderr, preventing potential
crashes with high-output commands.
- Refines rendering logic to ensure timely and accurate updates.
Summary of ChangesHello @galz10, 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 significantly enhances the robustness and reliability of the ShellExecutionService. It addresses issues related to command abortion by implementing a graceful shutdown mechanism and ensuring immediate responsiveness of the CLI. Furthermore, it prevents potential crashes caused by excessive memory consumption during high-output commands by introducing a memory cap for buffered output. 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 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 counter productive. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 significantly improves the robustness of the ShellExecutionService. The changes introduce a graceful shutdown mechanism (SIGTERM followed by SIGKILL), prevent the CLI from hanging on abort by resolving promises immediately, and cap memory usage for the child_process fallback to avoid crashes. The tests have been updated accordingly to cover these new behaviors. My review focuses on ensuring the new shutdown logic is fully robust against race conditions that could lead to unhandled errors.
|
Size Change: +3.08 kB (+0.02%) Total Size: 17.8 MB
ℹ️ View Unchanged
|
| const { Terminal } = pkg; | ||
|
|
||
| const SIGKILL_TIMEOUT_MS = 200; | ||
| const MAX_CHILD_PROCESS_BUFFER_SIZE = 16 * 1024 * 1024; // 16MB |
There was a problem hiding this comment.
can you comment on why 16MB?
There was a problem hiding this comment.
Figured we want to limit the size of the buffer so that it doesn't crash the cli , 16mb is ~3-5 millions lines of text .
|
|
||
| if (stream === 'stdout') { | ||
| stdout += decodedChunk; | ||
| if (stdout.length > MAX_CHILD_PROCESS_BUFFER_SIZE) { |
There was a problem hiding this comment.
we should notify the user&model if there is truncation. also consider making a helper method that safely appends content to the buffer truncating as needed. This logic is fine in general but would have edge case issues if stdout + decodedChunk was large enough to exceed the maximum string size we can handle. It would be instead by slightly more performance and robust to first truncate stdout by the amount needed to fit within the buffer size and only then add decodedChunk. For full robustness would also need to handle if decodedChunk is larger than MAX_CHILD_PROCESS_BUFFER_SIZE
There was a problem hiding this comment.
Added this
not sure if i should actually make it a warning/notification message that shows up as yellow.
warning This commit refactors the shell execution service's `child_process` fallback to improve how large outputs are handled. Key changes: - Implements a sliding window for stdout and stderr buffers. This ensures that if the output exceeds the 16MB limit, the most recent content is preserved, rather than just truncating the end. - Appends a clear warning message to the output when truncation occurs, informing the user that the output has been shortened. - Strengthens the associated tests, removing reliance on fake timers for abort scenarios and verifying the new truncation behavior and warning message.
When running shell commands that produce a large volume of rapid output, the UI would appear to freeze, only rendering the output upon cancellation or completion. This was due to the PTY rendering logic using a debounce mechanism, which was continuously reset by the high-frequency data stream, preventing the UI from updating. This commit replaces the debounced rendering with a throttled approach in `shellExecutionService.ts`. The UI is now guaranteed to render updates at a regular interval (every 17ms), ensuring responsiveness even during stdout floods. The redundant client-side throttling in `shellCommandProcessor.ts` has been removed to rely solely on the new, more efficient core implementation.
…e-gemini/gemini-cli into galzahavi/fix/shell-output
Previously, a race condition could occur where a user could not cancel a shell command. If the command's process exited but was still streaming a large amount of output, the cancellation signal would be ignored while the output processing completed, causing the CLI to hang. This change introduces a Promise.race in the onExit handler. It races the output processing promise against a promise that resolves on cancellation. This ensures that an abort signal is respected immediately, even during the final output processing phase, making command cancellation robust and preventing the application from becoming unresponsive.
2 participants
TLDR
This pull request improves the reliability of the
ShellExecutionServiceby implementing a graceful shutdown sequence for aborted commands, preventing the CLI from hanging on abortion, and capping memory usage to avoid crashes with high-output commands.Dive Deeper
This commit enhances the
ShellExecutionServicewith several key improvements:SIGTERMfollowed bySIGKILLto allow processes to terminate cleanly.child_processfallback to cap memory usage at 16MB forstdoutandstderr, preventing potential crashes with high-output commands.These changes make the shell execution more robust and prevent common issues related to process management and memory consumption.
Reviewer Test Plan
Test Graceful Shutdown & No Hanging:
sleep 10).sleepprocess should be terminated. You can verify withps aux | grep sleep.Test High-Output Command (Memory Capping):
yes | head -n 1000000).Test Aborting a High-Output Command:
yes).Testing Matrix
Linked issues / bugs